Open
Conversation
Aenimus
requested review from
a team,
JivusAyrus,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces persisted-directive infrastructure with federated-directive structures, adds directive-definition-data modules and validation, introduces BatchNormalizer for batch normalization, refactors types/results/imports across normalization/federation/schema-building, and updates utilities, errors, and tests to use the new federated directive model. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~105 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
composition/src/errors/types/types.ts (1)
3-6: Prefer an interface for this exported object shape.As per coding guidelines,
*.ts: Prefer interfaces over type aliases for object shapes in TypeScript. Converting this to an interface keeps the API identical while matching the repo convention.♻️ Proposed fix
-export type InvalidRootTypeFieldEventsDirectiveData = { +export interface InvalidRootTypeFieldEventsDirectiveData { definesDirectives: boolean; invalidDirectiveNames: Array<DirectiveName>; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/errors/types/types.ts` around lines 3 - 6, Convert the exported type alias InvalidRootTypeFieldEventsDirectiveData into an exported interface with the same properties (definesDirectives: boolean; invalidDirectiveNames: Array<DirectiveName>), i.e. replace "export type InvalidRootTypeFieldEventsDirectiveData = { ... }" with "export interface InvalidRootTypeFieldEventsDirectiveData { ... }" so the public API stays identical and matches the repo convention; ensure all existing references/imports continue to work (no other code changes required).composition/src/validation/types/params.ts (1)
1-26: Prefer interfaces for these exported parameter shapes.As per coding guidelines,
*.ts: Prefer interfaces over type aliases for object shapes in TypeScript. Switching these contracts to interfaces keeps the API stable and matches the rest of the refactor.♻️ Proposed fix
-export type IsArgumentValueValidParams = { +export interface IsArgumentValueValidParams { argumentValue: ConstValueNode; parentDefinitionDataByTypeName: Map<TypeName, ParentDefinitionData>; typeNode: TypeNode; -}; +} -export type ValidateCustomDirectiveParams = { +export interface ValidateCustomDirectiveParams { argumentDataByName: Map<ArgumentName, DirectiveArgumentData>; directiveNode: ConstDirectiveNode; parentDefinitionDataByTypeName: Map<TypeName, ParentDefinitionData>; requiredArgumentNames: Array<ArgumentName>; -}; +} -export type ValidateDirectivesParams = { +export interface ValidateDirectivesParams { data: NodeData | SchemaData; directiveCoords: string; directiveDefinitionData: DirectiveDefinitionData; directiveNodes: Array<ConstDirectiveNode>; parentDefinitionDataByTypeName: Map<TypeName, ParentDefinitionData>; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/validation/types/params.ts` around lines 1 - 26, Replace the exported object-shaped type aliases with exported interfaces to follow the project guideline: change the declarations for IsArgumentValueValidParams, ValidateCustomDirectiveParams, and ValidateDirectivesParams from "export type ..." to "export interface ..." keeping the exact same property names and types (argumentValue, parentDefinitionDataByTypeName, typeNode; argumentDataByName, directiveNode, parentDefinitionDataByTypeName, requiredArgumentNames; data, directiveCoords, directiveDefinitionData, directiveNodes, parentDefinitionDataByTypeName) and leave all imports unchanged so the public shape and usage remain identical.composition/src/subgraph/types/params.ts (1)
4-7: Prefer an interface for this params object.This is a plain object shape, so an interface fits the repo TS guideline better and keeps the type open for extension.
♻️ Proposed refactor
-export type InternalSubgraphFromNormalizationParams = { +export interface InternalSubgraphFromNormalizationParams { normalization: NormalizationSuccess; subgraphName: SubgraphName; -}; +}As per coding guidelines, prefer interfaces over type aliases for object shapes in TypeScript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/subgraph/types/params.ts` around lines 4 - 7, Replace the exported type alias InternalSubgraphFromNormalizationParams with an exported interface of the same name describing the same members (normalization: NormalizationSuccess; subgraphName: SubgraphName) so the object shape is open for extension; update any imports/usages if needed to continue referencing InternalSubgraphFromNormalizationParams unchanged.composition/src/directive-definition-data/types/types.ts (1)
13-46: Prefer interfaces for these exported object shapes.♻️ Proposed refactor
-export type DirectiveArgumentData = { +export interface DirectiveArgumentData {-export type DirectiveDefinitionData = { +export interface DirectiveDefinitionData {As per coding guidelines, prefer interfaces over type aliases for object shapes in TypeScript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/directive-definition-data/types/types.ts` around lines 13 - 46, Replace the two exported object type aliases with interfaces: convert DirectiveArgumentData and DirectiveDefinitionData from "export type" definitions to "export interface" declarations while keeping all existing properties, optional fields (defaultValue, description), union literal kinds (e.g., kind: Kind.ARGUMENT), Maps/Sets, and referenced names (ArgumentName, DirectiveName, MutableInputValueNode, DirectiveDefinitionNode, etc.) unchanged; update any imports/uses if your linter requires interface-specific behavior but otherwise preserve the exact shape and exported names so existing consumers compile.composition/src/v1/normalization/batch-normalization/types/params.ts (1)
3-7: Prefer an interface for this params object.♻️ Proposed refactor
-export type HandleOverridesParams = { +export interface HandleOverridesParams { originalTypeNameByRenamedTypeName: Map<TypeName, TypeName>; overriddenFieldNamesByParentTypeNameByTargetSubgraphName: Map<SubgraphName, Map<TypeName, Set<FieldName>>>; subgraphName: SubgraphName; -}; +}As per coding guidelines, prefer interfaces over type aliases for object shapes in TypeScript.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/batch-normalization/types/params.ts` around lines 3 - 7, Replace the exported type alias HandleOverridesParams with an exported interface of the same name describing the same properties (originalTypeNameByRenamedTypeName, overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the object shape uses an interface instead of a type alias; keep the exact property names and types (Map<TypeName, TypeName>, Map<SubgraphName, Map<TypeName, Set<FieldName>>>, SubgraphName) and preserve the export.composition/src/schema-building/utils.ts (1)
211-216: Use strict equality for the printed default comparison.Both sides are strings here, so
===is the safer and repo-standard check.As per coding guidelines, "Always use strict equality (
===and!==) instead of loose equality (==and!=)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/utils.ts` around lines 211 - 216, The comparison between printed default values uses loose equality; change the if condition that checks existingDefaultValueString == incomingDefaultValueString to use strict equality (===) so existingDefaultValueString === incomingDefaultValueString; update the conditional in the block where print(existingData.defaultValue) and print(incomingData.defaultValue) are compared (symbols: existingDefaultValueString, incomingDefaultValueString, print) to follow the repo standard.composition/src/schema-building/types/params.ts (1)
13-75: Preferinterfacefor these exported parameter bags.These exports are all object shapes, so using
interfacehere would align this module with the repo’s TS convention and keep the public API consistent with the other typed DTO-style definitions in the codebase.As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/types/params.ts` around lines 13 - 75, Replace the exported type aliases for the parameter bags with exported interfaces while preserving the exact property names and types; specifically convert IsTypeValidImplementationParams, GetRouterPersistedDirectiveNodesParams, GetValidArgumentNodesParams, GetValidExecutableDirectiveArgumentNodesParams, DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams, RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams, RouterSchemaNodeFromDataParams, and CompareAndValidateInputDefaultValuesParams from "export type = { ... }" to "export interface ... { ... }" so the public API and shapes remain identical but follow the repo convention of using interfaces for object shapes. Ensure no structural changes to properties like Map<TypeName, Set<TypeName>>, NodeData, DirectiveDefinitionData, ParentDefinitionData, MutableInputValueNode, StringValueNode, etc., and keep all exports unchanged.composition/src/v1/normalization/normalization-factory.ts (1)
773-780: Add an explicit return type tohandleExecutableDirectives.Please annotate this method with
: voidto keep type contracts explicit and consistent with TS guideline enforcement.As per coding guidelines, "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 773 - 780, The method handleExecutableDirectives lacks an explicit return type; update its declaration to include an explicit ": void" return annotation (i.e., change the signature of handleExecutableDirectives to handleExecutableDirectives(): void) to comply with the TypeScript guideline; locate the method where it iterates over directiveDefinitionDataByName and assigns to federatedDirectiveDataByName and add the annotation without changing the body.composition/src/v1/federation/federation-factory.ts (1)
1058-1088: Remove stale persisted-directive commented blocks (and dead no-op method).These commented sections and the empty
upsertPersistedDirectiveDefinitionData()method look like migration leftovers and add noise in a critical path. Please remove or move to commit history to keep the new federated-directive flow easier to maintain.Also applies to: 1507-1549, 1749-1761
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` around lines 1058 - 1088, Remove commented-out persistedDirectivesData blocks and the dead no-op method to eliminate migration noise: in the copyDirectiveArgumentData function remove the commented persistedDirectivesData block (the lines referencing newPersistedDirectivesData and extractFederatedDirectives) and any similar commented blocks at the other locations called out, and delete the unused upsertPersistedDirectiveDefinitionData() method entirely (or move its implementation to history) so only the active federatedDirectivesData flow remains; ensure imports referencing newPersistedDirectivesData or the removed method are cleaned up if now unused.composition/src/index.ts (1)
41-42: Remove the duplicate barrel re-exports.
./v1/constants/constantsand./directive-definition-data/directive-definition-dataare both exported twice here. It is harmless at runtime, but it adds noise to the public surface and makes future barrel diffs harder to audit.♻️ Proposed cleanup
export * from './v1/constants/constants'; -export * from './v1/constants/constants'; export * from './v1/constants/directive-definitions'; @@ export * from './v1/federation/utils'; export * from './v1/federation/walkers'; export * from './v1/normalization/batch-normalization/batch-normalizer'; -export * from './directive-definition-data/directive-definition-data'; export * from './v1/normalization/utils';Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/index.ts` around lines 41 - 42, Duplicate barrel re-exports are present (e.g., the two identical export lines for './v1/constants/constants' and similarly for './directive-definition-data/directive-definition-data'); remove the duplicated export entries so each module is exported only once, leaving a single export statement for './v1/constants/constants' and a single export statement for './directive-definition-data/directive-definition-data' in composition/src/index.ts to clean the public surface.composition/src/schema-building/types/types.ts (1)
181-186: MakeFederatedDirectivesDataan interface.This is another new object-shaped contract, so it should follow the same
interfaceconvention as the surrounding schema-building types.♻️ Proposed cleanup
-export type FederatedDirectivesData = { +export interface FederatedDirectivesData { deprecatedReason: string; directivesByName: Map<DirectiveName, Array<ConstDirectiveNode>>; isDeprecated: boolean; tagDirectiveByName: Map<string, ConstDirectiveNode>; -}; +}As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/types/types.ts` around lines 181 - 186, Replace the object-shaped type alias FederatedDirectivesData with an exported interface of the same name preserving all existing properties (deprecatedReason: string; directivesByName: Map<DirectiveName, Array<ConstDirectiveNode>>; isDeprecated: boolean; tagDirectiveByName: Map<string, ConstDirectiveNode>), i.e. change the declaration from "export type FederatedDirectivesData = { ... }" to "export interface FederatedDirectivesData { ... }" so it follows the project's interface convention and keeps the public API unchanged.composition/src/schema-building/types/results.ts (1)
54-57: Use aninterfacefor this result shape.This is a new object contract, so
interfacematches the repo convention better than atypealias here.♻️ Proposed cleanup
-export type ExtractDirectiveLocationsResult = { +export interface ExtractDirectiveLocationsResult { errors: Array<Error>; locations: Set<DirectiveLocation>; -}; +}As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/schema-building/types/results.ts` around lines 54 - 57, Replace the exported type alias ExtractDirectiveLocationsResult with an exported interface of the same name that declares the same properties (errors: Array<Error>; locations: Set<DirectiveLocation>), so the object contract follows the repo convention; update any references if needed to continue using ExtractDirectiveLocationsResult as a type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/directive-definition-data/types/types.ts`:
- Around line 1-11: Update the imports that currently use the non-canonical
subpath 'graphql/index' to import from the package entry 'graphql' instead;
e.g., replace the import that brings in ConstDirectiveNode, ConstValueNode,
DirectiveDefinitionNode, Kind, StringValueNode in the types.ts file so it reads
import type { ConstDirectiveNode, ConstValueNode, DirectiveDefinitionNode, Kind,
StringValueNode } from 'graphql'; and make the same change in the corresponding
directive-definition-data utils and params modules where the same graphql types
are imported.
In `@composition/src/errors/errors.ts`:
- Around line 390-397: The error message in invalidArgumentValueError currently
prefixes the argument name with a dollar sign by using "$${argumentName}", which
renders as a GraphQL-style variable (e.g., "$name"); update the template string
in invalidArgumentValueError to remove the extra dollar sign and interpolate the
argument name directly (use "${argumentName}" or plain concatenation) so the
message reads the actual argument name instead of a variable; keep the rest of
the wording and interpolation for expectedTypeString and value unchanged.
In `@composition/src/schema-building/utils.ts`:
- Around line 597-602: The MutableInputValueNode can retain an old AST default
when includeDefaultValue is false; update the code that sets
data.node.defaultValue so it clears the property when data.includeDefaultValue
is false (e.g., delete data.node.defaultValue or set it to undefined) instead of
leaving the stale value, ensuring behavior aligns with
compareAndValidateInputDefaultValues and using the includeDefaultValue flag on
the MutableInputValueNode.
In `@composition/src/v1/constants/strings.ts`:
- Line 150: The IMPORT_VERSION_REGEX currently allows matching substrings (e.g.,
inside v1.2.3); update the regex used in the constant IMPORT_VERSION_REGEX to
require the whole token to be exactly vX.Y by anchoring it (use start ^ and end
$ anchors) so validation aligns with the subsequent
parseFloat(versionString.substring(1)) and the "not of the form v1.2" error
message.
In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts`:
- Around line 94-98: The version-major comparison currently uses loose
inequality; replace the `!=` operator with strict inequality `!==` in the block
that compares existingMajorVersion and incomingMajorVersion (the variables
computed via Math.trunc) so the condition reads `if (existingMajorVersion !==
incomingMajorVersion) {
this.errors.push(nonEqualComposeDirectiveMajorVersionError(directiveName)); }`;
update the comparison in the function/method where these variables are defined
in batch-normalizer.ts.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 739-742: Guard the use of startsWith on argNode.value.value by
first verifying the AST kind is a string (or that argNode.value && typeof
argNode.value.value === "string") before calling startsWith; if the value is
missing or not a string, push an error (use noLeadingAtComposeDirectiveNameError
or a new descriptive error) to this.errors and return, otherwise assign rawValue
and proceed to check rawValue.startsWith(LITERAL_AT); reference the argNode,
rawValue, LITERAL_AT, startsWith call, and
this.errors.push/noLeadingAtComposeDirectiveNameError when making the fix.
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 118-130: The loc?: Location annotations on ComposeDirectiveNode
and ComposeDirectiveArgumentNode refer to the GraphQL AST Location type but
Location is not imported; fix by adding an import for Location from the
'graphql' package alongside the existing AST imports (e.g., where Kind,
NameNode, StringValueNode are imported) so that the Location type is available
for the loc fields in ComposeDirectiveNode and ComposeDirectiveArgumentNode.
In `@composition/src/v1/normalization/utils.ts`:
- Around line 532-536: The current branch replaces the stored definition with
directiveData and thus erases an older description when the newer version omits
it; change the update so you only overwrite description when
directiveData.description is present (e.g., existingData.description =
directiveData.description ?? existingData.description) and update the map with
the merged existingData (existingDataByName.set(directiveName, existingData))
instead of storing directiveData directly.
- Around line 615-636: The validation branches call error factories
(invalidFieldLinkDirectiveImportObjectError,
unknownFieldLinkDirectiveImportObjectError) but discard the Error objects —
capture the returned Error into a variable and record it (e.g., push into the
import-object's error collection like data.errors or call the existing
error-aggregation helper), set data.success = false, and skip applying the
invalid field (use continue/return as appropriate) so malformed import objects
are correctly marked failed instead of being treated as success; update both the
AS and NAME invalid-kind branches and the default unknown-field branch to
perform this recording and flow control.
In `@composition/src/validation/validation.ts`:
- Around line 118-122: The current branch in isArgumentValueValid() simply
returns true for argumentValue.kind === Kind.OBJECT, which skips field-level
validation; instead, retrieve the input object definition from parentData
(verify parentData.kind === Kind.INPUT_OBJECT_TYPE_DEFINITION) and iterate its
fields to (1) ensure no unknown fields are present on argumentValue.fields
(reject extras), (2) detect duplicate field names by tracking seen names, (3)
ensure all required fields (non-null without default) are present, and (4) for
each provided field validate its value recursively by calling
isArgumentValueValid() with the corresponding field type node (handling nested
INPUT_OBJECT, LIST, ENUM, SCALAR as the function already supports); use the
input object definition's field list (e.g., parentData.fields and field.type) to
drive these checks and return false on any mismatch.
- Around line 227-229: The repeated-directive validation is rejecting legitimate
multi-subgraph usages because several federated directives are created with
isComposed:false; update the initialization so `@authenticated`, `@inaccessible`,
`@requiresScopes`, `@semanticNonNull`, `@cost`, `@listSize`, and `@specifiedBy` are marked
isComposed:true (either by setting isComposed:true in FEDERATED_DIRECTIVE_DATAS
entries or by passing isComposed:true to newDirectiveDefinitionData when those
directives are created), or ensure
upsertExecutableDirectiveDatas()/executableDirectiveDatasByName explicitly flips
isComposed to true for these directive names so the check (directiveNodes.length
> 1 && !isRepeatable && !isComposed) no longer incorrectly pushes
invalidRepeatedDirectiveError for them.
In `@composition/tests/v1/directives/compose-directive.test.ts`:
- Line 236: Implement the previously-empty test "that different core features
can import the same named directive if only one is composed" by constructing two
mock core features (both declaring/importing the same named directive),
composing only one of them, then asserting the composition result/federated
schema contains that directive exactly once and that composition succeeds
without errors for the non-composed core; update the test body to use the
existing test helpers used elsewhere in this file (create mock features and run
the composition/composition assertion helpers) and add assertions that the
directive is present in the composed schema and not duplicated or required from
the non-composed feature.
---
Nitpick comments:
In `@composition/src/directive-definition-data/types/types.ts`:
- Around line 13-46: Replace the two exported object type aliases with
interfaces: convert DirectiveArgumentData and DirectiveDefinitionData from
"export type" definitions to "export interface" declarations while keeping all
existing properties, optional fields (defaultValue, description), union literal
kinds (e.g., kind: Kind.ARGUMENT), Maps/Sets, and referenced names
(ArgumentName, DirectiveName, MutableInputValueNode, DirectiveDefinitionNode,
etc.) unchanged; update any imports/uses if your linter requires
interface-specific behavior but otherwise preserve the exact shape and exported
names so existing consumers compile.
In `@composition/src/errors/types/types.ts`:
- Around line 3-6: Convert the exported type alias
InvalidRootTypeFieldEventsDirectiveData into an exported interface with the same
properties (definesDirectives: boolean; invalidDirectiveNames:
Array<DirectiveName>), i.e. replace "export type
InvalidRootTypeFieldEventsDirectiveData = { ... }" with "export interface
InvalidRootTypeFieldEventsDirectiveData { ... }" so the public API stays
identical and matches the repo convention; ensure all existing
references/imports continue to work (no other code changes required).
In `@composition/src/index.ts`:
- Around line 41-42: Duplicate barrel re-exports are present (e.g., the two
identical export lines for './v1/constants/constants' and similarly for
'./directive-definition-data/directive-definition-data'); remove the duplicated
export entries so each module is exported only once, leaving a single export
statement for './v1/constants/constants' and a single export statement for
'./directive-definition-data/directive-definition-data' in
composition/src/index.ts to clean the public surface.
In `@composition/src/schema-building/types/params.ts`:
- Around line 13-75: Replace the exported type aliases for the parameter bags
with exported interfaces while preserving the exact property names and types;
specifically convert IsTypeValidImplementationParams,
GetRouterPersistedDirectiveNodesParams, GetValidArgumentNodesParams,
GetValidExecutableDirectiveArgumentNodesParams,
DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams,
RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams,
RouterSchemaNodeFromDataParams, and CompareAndValidateInputDefaultValuesParams
from "export type = { ... }" to "export interface ... { ... }" so the public API
and shapes remain identical but follow the repo convention of using interfaces
for object shapes. Ensure no structural changes to properties like Map<TypeName,
Set<TypeName>>, NodeData, DirectiveDefinitionData, ParentDefinitionData,
MutableInputValueNode, StringValueNode, etc., and keep all exports unchanged.
In `@composition/src/schema-building/types/results.ts`:
- Around line 54-57: Replace the exported type alias
ExtractDirectiveLocationsResult with an exported interface of the same name that
declares the same properties (errors: Array<Error>; locations:
Set<DirectiveLocation>), so the object contract follows the repo convention;
update any references if needed to continue using
ExtractDirectiveLocationsResult as a type.
In `@composition/src/schema-building/types/types.ts`:
- Around line 181-186: Replace the object-shaped type alias
FederatedDirectivesData with an exported interface of the same name preserving
all existing properties (deprecatedReason: string; directivesByName:
Map<DirectiveName, Array<ConstDirectiveNode>>; isDeprecated: boolean;
tagDirectiveByName: Map<string, ConstDirectiveNode>), i.e. change the
declaration from "export type FederatedDirectivesData = { ... }" to "export
interface FederatedDirectivesData { ... }" so it follows the project's interface
convention and keeps the public API unchanged.
In `@composition/src/schema-building/utils.ts`:
- Around line 211-216: The comparison between printed default values uses loose
equality; change the if condition that checks existingDefaultValueString ==
incomingDefaultValueString to use strict equality (===) so
existingDefaultValueString === incomingDefaultValueString; update the
conditional in the block where print(existingData.defaultValue) and
print(incomingData.defaultValue) are compared (symbols:
existingDefaultValueString, incomingDefaultValueString, print) to follow the
repo standard.
In `@composition/src/subgraph/types/params.ts`:
- Around line 4-7: Replace the exported type alias
InternalSubgraphFromNormalizationParams with an exported interface of the same
name describing the same members (normalization: NormalizationSuccess;
subgraphName: SubgraphName) so the object shape is open for extension; update
any imports/usages if needed to continue referencing
InternalSubgraphFromNormalizationParams unchanged.
In `@composition/src/v1/federation/federation-factory.ts`:
- Around line 1058-1088: Remove commented-out persistedDirectivesData blocks and
the dead no-op method to eliminate migration noise: in the
copyDirectiveArgumentData function remove the commented persistedDirectivesData
block (the lines referencing newPersistedDirectivesData and
extractFederatedDirectives) and any similar commented blocks at the other
locations called out, and delete the unused
upsertPersistedDirectiveDefinitionData() method entirely (or move its
implementation to history) so only the active federatedDirectivesData flow
remains; ensure imports referencing newPersistedDirectivesData or the removed
method are cleaned up if now unused.
In `@composition/src/v1/normalization/batch-normalization/types/params.ts`:
- Around line 3-7: Replace the exported type alias HandleOverridesParams with an
exported interface of the same name describing the same properties
(originalTypeNameByRenamedTypeName,
overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the
object shape uses an interface instead of a type alias; keep the exact property
names and types (Map<TypeName, TypeName>, Map<SubgraphName, Map<TypeName,
Set<FieldName>>>, SubgraphName) and preserve the export.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 773-780: The method handleExecutableDirectives lacks an explicit
return type; update its declaration to include an explicit ": void" return
annotation (i.e., change the signature of handleExecutableDirectives to
handleExecutableDirectives(): void) to comply with the TypeScript guideline;
locate the method where it iterates over directiveDefinitionDataByName and
assigns to federatedDirectiveDataByName and add the annotation without changing
the body.
In `@composition/src/validation/types/params.ts`:
- Around line 1-26: Replace the exported object-shaped type aliases with
exported interfaces to follow the project guideline: change the declarations for
IsArgumentValueValidParams, ValidateCustomDirectiveParams, and
ValidateDirectivesParams from "export type ..." to "export interface ..."
keeping the exact same property names and types (argumentValue,
parentDefinitionDataByTypeName, typeNode; argumentDataByName, directiveNode,
parentDefinitionDataByTypeName, requiredArgumentNames; data, directiveCoords,
directiveDefinitionData, directiveNodes, parentDefinitionDataByTypeName) and
leave all imports unchanged so the public shape and usage remain identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 275311b1-7813-4a2b-9225-bf8d0489502b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
composition/package.jsoncomposition/src/directive-definition-data/directive-definition-data.tscomposition/src/directive-definition-data/types/params.tscomposition/src/directive-definition-data/types/types.tscomposition/src/directive-definition-data/utils.tscomposition/src/errors/errors.tscomposition/src/errors/types/params.tscomposition/src/errors/types/types.tscomposition/src/federation/types/params.tscomposition/src/federation/types/results.tscomposition/src/federation/types/types.tscomposition/src/index.tscomposition/src/normalization/normalization.tscomposition/src/normalization/types.tscomposition/src/schema-building/ast.tscomposition/src/schema-building/params.tscomposition/src/schema-building/types/params.tscomposition/src/schema-building/types/results.tscomposition/src/schema-building/types/types.tscomposition/src/schema-building/utils.tscomposition/src/subgraph/types.tscomposition/src/subgraph/types/params.tscomposition/src/subgraph/utils.tscomposition/src/types/results.tscomposition/src/types/types.tscomposition/src/utils/params.tscomposition/src/utils/string-constants.tscomposition/src/utils/utils.tscomposition/src/v1/constants/strings.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/types/params.tscomposition/src/v1/federation/utils.tscomposition/src/v1/federation/walkers.tscomposition/src/v1/normalization/batch-normalization/batch-normalizer.tscomposition/src/v1/normalization/batch-normalization/types/params.tscomposition/src/v1/normalization/batch-normalization/types/results.tscomposition/src/v1/normalization/batch-normalization/types/types.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/params.tscomposition/src/v1/normalization/types/results.tscomposition/src/v1/normalization/types/types.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/normalization/walkers.tscomposition/src/v1/schema-building/type-merging.tscomposition/src/v1/utils/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/src/validation/types/params.tscomposition/src/validation/types/results.tscomposition/src/validation/validation.tscomposition/tests/v1/directives/compose-directive.test.tscomposition/tests/v1/directives/override.test.tscomposition/tests/v1/events.test.tscomposition/tests/v1/federation-factory.test.tscomposition/tests/v1/router-configuration.test.ts
💤 Files with no reviewable changes (2)
- composition/src/schema-building/params.ts
- composition/src/v1/normalization/directive-definition-data.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2804 +/- ##
===========================================
- Coverage 65.71% 47.44% -18.27%
===========================================
Files 254 1104 +850
Lines 26527 149497 +122970
Branches 0 10244 +10244
===========================================
+ Hits 17431 70925 +53494
- Misses 7683 76771 +69088
- Partials 1413 1801 +388
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
composition/src/v1/normalization/normalization-factory.ts (1)
733-745:⚠️ Potential issue | 🔴 CriticalGuard non-string
@composeDirective(name:)values before string operations.
validateDirectives()records invalid argument values, but normalization still reaches this method. Ifnameis not a string,argNode.value.value.startsWith(...)can throw and short-circuit normal error reporting.🐛 Proposed fix
handleComposeDirective(node: ComposeDirectiveNode): void { const argNode = node.arguments[0]; if (!argNode) { return; } + if (argNode.value.kind !== Kind.STRING) { + return; + } + const rawValue = argNode.value.value; if (!rawValue.startsWith(LITERAL_AT)) { this.errors.push(noLeadingAtComposeDirectiveNameError(rawValue)); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 733 - 745, handleComposeDirective currently assumes argNode.value.value is a string and calls startsWith(LITERAL_AT), which throws for non-string values; add a guard before doing string ops: retrieve rawValue from argNode.value.value, check typeof rawValue === 'string' (or validate node kind) and if not push the existing validation error (match the pattern used by validateDirectives) and return; only then perform startsWith(LITERAL_AT) and substring(1) to compute directiveName so non-string inputs are short-circuited safely.
🧹 Nitpick comments (1)
composition/src/v1/normalization/batch-normalization/batch-normalizer.ts (1)
80-198: Add explicit: voidreturn types to the internal helper methods.These four mutating helpers have no return statements and should explicitly annotate their return type to match the repository's TypeScript coding guidelines.
Suggested fix
- handleLinkImports(importDataByDirectiveName: Map<DirectiveName, LinkImportData>) { + handleLinkImports(importDataByDirectiveName: Map<DirectiveName, LinkImportData>): void { - handleEntityData(entityDataByTypeName: Map<TypeName, EntityData>, subgraphName: SubgraphName) { + handleEntityData(entityDataByTypeName: Map<TypeName, EntityData>, subgraphName: SubgraphName): void { - handleOverrides({ + handleOverrides({ originalTypeNameByRenamedTypeName, overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName, - }: HandleOverridesParams) { + }: HandleOverridesParams): void { - handleOverrideConfigurationData() { + handleOverrideConfigurationData(): void {Per coding guideline: "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts` around lines 80 - 198, The four internal mutating helper methods handleLinkImports, handleEntityData, handleOverrides, and handleOverrideConfigurationData currently have no return statements—add explicit TypeScript return type annotations ": void" to each method signature (e.g., change "handleLinkImports(...)" to "handleLinkImports(...): void") to comply with the repository guideline for explicit return types; ensure you update the signatures for the exact methods named above so callers and the class definition remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts`:
- Around line 176-183: The code applies override configuration even when some
subgraph normalizations failed, causing unknownSubgraphNameError(...) for
subgraphs that exist but failed validation; fix by making
handleOverrideConfigurationData() no-op when there are any validation errors
(e.g., check this.validationErrors?.length > 0 and return early) or
alternatively stop calling handleOverrideConfigurationData() after appending
validationErrors — ensure you reference handleOverrideConfigurationData(),
internalSubgraphBySubgraphName, and unknownSubgraphNameError() when locating the
logic to change.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 755-770: The code mutates shared directive definition singletons
from this.directiveDefinitionDataByName by writing to definitionData.version and
definitionData.isComposed; instead clone the definitionData object (e.g.,
shallow copy) after retrieving it and before setting version/isComposed, then
put the cloned object into this.federatedDirectiveDataByName; keep the existing
guards around unknownComposeDirectiveNameError(rawValue) and
unimportedComposeDirectiveNameError(rawValue) and only modify the block that
currently assigns to definitionData so the module-level singletons are not
mutated.
In `@composition/src/v1/normalization/utils.ts`:
- Around line 583-598: The code currently converts versionString via
parseFloat(versionString.substring(1)) which collapses multi-digit minors (e.g.
"1.10" → 1.1) and breaks numeric ordering in upsertFederatedDirectiveData;
instead preserve a semver-safe representation: remove the leading char as you
already do but return the raw version string (e.g. "1.10") or parse it into a
tuple/object of integers (major/minor/patch) so comparisons are exact. Update
the return in the function that computes coreUrl/featureName to set version to
that semver-safe value (instead of the parseFloat result) and, if you need
numeric comparisons elsewhere, use a proper semver compare or compare the tuple
rather than using parseFloat.
---
Duplicate comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 733-745: handleComposeDirective currently assumes
argNode.value.value is a string and calls startsWith(LITERAL_AT), which throws
for non-string values; add a guard before doing string ops: retrieve rawValue
from argNode.value.value, check typeof rawValue === 'string' (or validate node
kind) and if not push the existing validation error (match the pattern used by
validateDirectives) and return; only then perform startsWith(LITERAL_AT) and
substring(1) to compute directiveName so non-string inputs are short-circuited
safely.
---
Nitpick comments:
In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts`:
- Around line 80-198: The four internal mutating helper methods
handleLinkImports, handleEntityData, handleOverrides, and
handleOverrideConfigurationData currently have no return statements—add explicit
TypeScript return type annotations ": void" to each method signature (e.g.,
change "handleLinkImports(...)" to "handleLinkImports(...): void") to comply
with the repository guideline for explicit return types; ensure you update the
signatures for the exact methods named above so callers and the class definition
remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cab6ef9-250a-4f6e-8574-addb02eeb9e3
📒 Files selected for processing (10)
composition-go/index.global.jscomposition/src/directive-definition-data/types/params.tscomposition/src/directive-definition-data/types/types.tscomposition/src/directive-definition-data/utils.tscomposition/src/errors/errors.tscomposition/src/v1/constants/strings.tscomposition/src/v1/normalization/batch-normalization/batch-normalizer.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/tests/v1/directives/compose-directive.test.ts
✅ Files skipped from review due to trivial changes (2)
- composition/src/v1/constants/strings.ts
- composition/src/directive-definition-data/types/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- composition/tests/v1/directives/compose-directive.test.ts
- composition/src/directive-definition-data/types/params.ts
- composition/src/directive-definition-data/utils.ts
Comment on lines
+176
to
+183
| handleOverrideConfigurationData() { | ||
| for (const [targetSubgraphName, overriddenFieldNamesByParentTypeName] of this | ||
| .overriddenFieldNamesByParentTypeNameByTargetSubgraphName) { | ||
| const internalSubgraph = this.internalSubgraphBySubgraphName.get(targetSubgraphName); | ||
| if (!internalSubgraph) { | ||
| this.errors.push(unknownSubgraphNameError(targetSubgraphName)); | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Skip override application once any subgraph normalization failed.
After Line 290 appends validationErrors, Line 291 still runs handleOverrideConfigurationData(). Because internalSubgraphBySubgraphName only contains successful normalizations, Line 181 can emit unknownSubgraphNameError(...) for a subgraph that actually exists but failed validation, which adds a misleading follow-up error on top of the real subgraphValidationError(...).
Suggested fix
this.errors.push(...this.validationErrors);
+ if (this.validationErrors.length > 0) {
+ return {
+ errors: this.errors,
+ success: false,
+ warnings: this.warnings,
+ };
+ }
this.handleOverrideConfigurationData();
if (this.errors.length > 0) {
return {
errors: this.errors,Also applies to: 290-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts`
around lines 176 - 183, The code applies override configuration even when some
subgraph normalizations failed, causing unknownSubgraphNameError(...) for
subgraphs that exist but failed validation; fix by making
handleOverrideConfigurationData() no-op when there are any validation errors
(e.g., check this.validationErrors?.length > 0 and return early) or
alternatively stop calling handleOverrideConfigurationData() after appending
validationErrors — ensure you reference handleOverrideConfigurationData(),
internalSubgraphBySubgraphName, and unknownSubgraphNameError() when locating the
logic to change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
composition/src/v1/normalization/normalization-factory.ts (2)
755-770:⚠️ Potential issue | 🔴 CriticalClone per-subgraph directive state before mutating it.
initializeDirectiveDefinitionDatas()seeds this map with imported*_DEFINITION_DATAsingletons. WritingversionandisComposedontodefinitionDatahere leaks compose metadata into laterNormalizationFactoryinstances.Suggested fix
const importData = this.importDataByDirectiveName.get(rawValue); if (!importData) { this.errors.push(unimportedComposeDirectiveNameError(rawValue)); return; } - definitionData.version = importData.version; - definitionData.isComposed = true; - - this.federatedDirectiveDataByName.set(directiveName, definitionData); + const composedDefinitionData: DirectiveDefinitionData = { + ...definitionData, + isComposed: true, + isReferenced: false, + version: importData.version, + }; + + this.directiveDefinitionDataByName.set(directiveName, composedDefinitionData); + this.federatedDirectiveDataByName.set(directiveName, composedDefinitionData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 755 - 770, The code is mutating shared singleton directive definition objects from initializeDirectiveDefinitionDatas() by setting definitionData.version and definitionData.isComposed; instead, when retrieving definitionData from directiveDefinitionDataByName (and before writing to federatedDirectiveDataByName), create a clone/copy of the definition object (shallow copy preserving other fields), set the cloned object's version and isComposed properties using importData from importDataByDirectiveName, and then store the cloned object into federatedDirectiveDataByName so the original singleton in directiveDefinitionDataByName remains unchanged for other NormalizationFactory instances.
739-741:⚠️ Potential issue | 🔴 CriticalGuard non-string
name:values before callingstartsWith.
schemaNodeFromData()still reaches this path after validation has recorded errors, so malformed@composeDirective(name: ...)values can makerawValueundefined and throw here instead of returning the collected validation error.Suggested fix
const argNode = node.arguments[0]; if (!argNode) { return; } + if (argNode.value.kind !== Kind.STRING) { + return; + } const rawValue = argNode.value.value; if (!rawValue.startsWith(LITERAL_AT)) { this.errors.push(noLeadingAtComposeDirectiveNameError(rawValue)); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 739 - 741, schemaNodeFromData currently assumes argNode.value.value is a string before calling startsWith, which can throw for malformed inputs; before using rawValue in normalization-factory.ts (around the block referencing rawValue, argNode, LITERAL_AT, and noLeadingAtComposeDirectiveNameError) guard that rawValue is a string (e.g., typeof rawValue === 'string') and if not, push an appropriate validation error into this.errors (or reuse noLeadingAtComposeDirectiveNameError with a safe message) and skip the startsWith check/processing for that argNode so the function returns the collected validation errors rather than throwing.composition/src/v1/normalization/batch-normalization/batch-normalizer.ts (1)
297-298:⚠️ Potential issue | 🟠 MajorStop before applying overrides when any subgraph normalization failed.
After Line 297 adds
subgraphValidationError(...), Line 298 still runshandleOverrideConfigurationData(). Failed subgraphs never enterinternalSubgraphBySubgraphName, so this can add misleadingunknownSubgraphNameError(...)on top of the real validation failure.Suggested fix
this.errors.push(...this.validationErrors); + if (this.validationErrors.length > 0) { + return { + errors: this.errors, + success: false, + warnings: this.warnings, + }; + } this.handleOverrideConfigurationData();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts` around lines 297 - 298, The code currently pushes validation errors and then calls handleOverrideConfigurationData(), which causes unknownSubgraphNameError(...) to be added for subgraphs that failed normalization; change the control flow so that after pushing this.validationErrors (and after subgraphValidationError(...) paths) you do not call handleOverrideConfigurationData() if any subgraph normalization failed—e.g., check for any validation errors (this.validationErrors.length > 0 or presence of subgraphValidationError entries) and return/skip calling handleOverrideConfigurationData(); keep references to this.errors, this.validationErrors, handleOverrideConfigurationData(), subgraphValidationError(...), and internalSubgraphBySubgraphName to locate and update the logic.
🧹 Nitpick comments (1)
composition/src/v1/federation/federation-factory.ts (1)
1081-1088: Remove leftover commented-out code.These commented-out lines reference the old
persistedDirectivesDatapattern and should be removed as part of the refactor cleanup.🧹 Proposed fix
node: { directives: [], kind: Kind.INPUT_VALUE_DEFINITION, name: stringToNameNode(sourceData.name), type: sourceData.type, }, originalCoords: sourceData.originalCoords, - // persistedDirectivesData: this.extractFederatedDirectives({ - // data: newPersistedDirectivesData(), - // directivesByName: sourceData.directivesByName, - // }), requiredSubgraphNames: new Set(sourceData.requiredSubgraphNames),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` around lines 1081 - 1088, Remove the leftover commented-out code that references the old persistedDirectivesData pattern: delete the commented block containing persistedDirectivesData, extractFederatedDirectives({ data: newPersistedDirectivesData(), directivesByName: sourceData.directivesByName, }), and the related newPersistedDirectivesData() call in the object being built (near requiredSubgraphNames/subgraphNames/type). Leave the surrounding object fields (requiredSubgraphNames, subgraphNames, type) intact and ensure no other dead references to persistedDirectivesData or extractFederatedDirectives remain in federation-factory.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/federation/federation-factory.ts`:
- Around line 1507-1549: The method upsertPersistedDirectiveDefinitionData
contains only commented-out code and should be removed; delete the entire method
declaration from the class (including its signature and empty body) and ensure
any references/calls to upsertPersistedDirectiveDefinitionData are updated or
removed to avoid unresolved symbol errors (search for
upsertPersistedDirectiveDefinitionData in the file to adjust usages or tests
accordingly).
- Around line 1713-1728: The properties executableLocations and locations in the
object stored to federatedDirectiveDataByName are currently sharing the same Set
reference (new Set<DirectiveLocation>(data.executableLocations) assigned to
both), which causes later mutations to executableLocations to also affect
locations; fix this by creating two distinct Set instances (e.g., assign
executableLocations: new Set(data.executableLocations) and locations: new
Set(data.executableLocations)) so subsequent deletions/updates to
executableLocations do not mutate locations while keeping the same initial
values.
---
Duplicate comments:
In `@composition/src/v1/normalization/batch-normalization/batch-normalizer.ts`:
- Around line 297-298: The code currently pushes validation errors and then
calls handleOverrideConfigurationData(), which causes
unknownSubgraphNameError(...) to be added for subgraphs that failed
normalization; change the control flow so that after pushing
this.validationErrors (and after subgraphValidationError(...) paths) you do not
call handleOverrideConfigurationData() if any subgraph normalization
failed—e.g., check for any validation errors (this.validationErrors.length > 0
or presence of subgraphValidationError entries) and return/skip calling
handleOverrideConfigurationData(); keep references to this.errors,
this.validationErrors, handleOverrideConfigurationData(),
subgraphValidationError(...), and internalSubgraphBySubgraphName to locate and
update the logic.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 755-770: The code is mutating shared singleton directive
definition objects from initializeDirectiveDefinitionDatas() by setting
definitionData.version and definitionData.isComposed; instead, when retrieving
definitionData from directiveDefinitionDataByName (and before writing to
federatedDirectiveDataByName), create a clone/copy of the definition object
(shallow copy preserving other fields), set the cloned object's version and
isComposed properties using importData from importDataByDirectiveName, and then
store the cloned object into federatedDirectiveDataByName so the original
singleton in directiveDefinitionDataByName remains unchanged for other
NormalizationFactory instances.
- Around line 739-741: schemaNodeFromData currently assumes argNode.value.value
is a string before calling startsWith, which can throw for malformed inputs;
before using rawValue in normalization-factory.ts (around the block referencing
rawValue, argNode, LITERAL_AT, and noLeadingAtComposeDirectiveNameError) guard
that rawValue is a string (e.g., typeof rawValue === 'string') and if not, push
an appropriate validation error into this.errors (or reuse
noLeadingAtComposeDirectiveNameError with a safe message) and skip the
startsWith check/processing for that argNode so the function returns the
collected validation errors rather than throwing.
---
Nitpick comments:
In `@composition/src/v1/federation/federation-factory.ts`:
- Around line 1081-1088: Remove the leftover commented-out code that references
the old persistedDirectivesData pattern: delete the commented block containing
persistedDirectivesData, extractFederatedDirectives({ data:
newPersistedDirectivesData(), directivesByName: sourceData.directivesByName, }),
and the related newPersistedDirectivesData() call in the object being built
(near requiredSubgraphNames/subgraphNames/type). Leave the surrounding object
fields (requiredSubgraphNames, subgraphNames, type) intact and ensure no other
dead references to persistedDirectivesData or extractFederatedDirectives remain
in federation-factory.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a00c3305-4251-4987-b9aa-e8a8c509f1b0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
composition/package.jsoncomposition/src/directive-definition-data/types/params.tscomposition/src/directive-definition-data/types/types.tscomposition/src/directive-definition-data/utils.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/normalization/batch-normalization/batch-normalizer.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/utils.tscomposition/tests/v1/directives/compose-directive.test.ts
✅ Files skipped from review due to trivial changes (4)
- composition/package.json
- composition/src/directive-definition-data/types/types.ts
- composition/src/directive-definition-data/utils.ts
- composition/src/directive-definition-data/types/params.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- composition/tests/v1/directives/compose-directive.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
composition/src/v1/normalization/types/types.ts (1)
125-136:⚠️ Potential issue | 🟠 MajorImport GraphQL's
Locationtype for these AST shapes.
loc?: Locationcurrently binds to the ambient DOMLocation, not GraphQL's ASTLocation. That makes the exported node types inaccurate and can break consumers outside DOM-lib builds.Suggested fix
import { type ConstDirectiveNode, type DocumentNode, type InputValueDefinitionNode, + type Location, type Kind, type NameNode, type StringValueNode, type ValueNode, } from 'graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/types/types.ts` around lines 125 - 136, The AST node types ComposeDirectiveNode and ComposeDirectiveArgumentNode currently use the ambient DOM Location; import and use GraphQL's AST Location type instead: add an import for Location from 'graphql' (or 'graphql/language' depending on project imports) and replace the ambient reference so that the loc?: Location fields on ComposeDirectiveNode and ComposeDirectiveArgumentNode refer to graphql's Location type.composition/src/v1/normalization/normalization-factory.ts (2)
745-753:⚠️ Potential issue | 🔴 CriticalGuard
@composeDirectivebefore reading a string value.
schemaNodeFromData()passes a castedConstDirectiveNodehere, soargNode.valueis not runtime-guaranteed to be aKind.STRING. If validation has already recorded an invalidname:value, this path still executes andstartsWithcan throw before normal error reporting completes.Suggested fix
const argNode = node.arguments[0]; if (!argNode) { return; } - const rawValue = argNode.value.value; + if (argNode.value.kind !== Kind.STRING) { + return; + } + const rawValue = argNode.value.value; if (!rawValue.startsWith(LITERAL_AT)) { this.errors.push(noLeadingAtComposeDirectiveNameError(rawValue)); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 745 - 753, In handleComposeDirective, guard that the directive argument's value is actually a string before calling startsWith: check argNode.value.kind === Kind.STRING (or otherwise verify typeof argNode.value.value === 'string') and only then read rawValue and run startsWith(LITERAL_AT); if it's not a string, push or let existing validation handle the appropriate error (e.g., do not call startsWith and avoid throwing). Ensure you reference argNode, argNode.value, rawValue, LITERAL_AT, and the errors.push(noLeadingAtComposeDirectiveNameError(...)) path when implementing the guard.
767-782:⚠️ Potential issue | 🔴 CriticalDon't mutate shared directive definition singletons here.
directiveDefinitionDataByNameis seeded from module-level*_DEFINITION_DATAobjects, so writingversionandisComposedintodefinitionDataleaks per-subgraph state acrossNormalizationFactoryinstances and can taint later compositions. Clone before applying per-subgraph fields.Suggested fix
const definitionData = this.directiveDefinitionDataByName.get(directiveName); if (!definitionData) { this.errors.push(unknownComposeDirectiveNameError(rawValue)); return; } @@ - definitionData.version = importData.version; - definitionData.isComposed = true; - - this.federatedDirectiveDataByName.set(directiveName, definitionData); + const federatedDirectiveData: DirectiveDefinitionData = { + ...definitionData, + argumentDataByName: new Map(definitionData.argumentDataByName), + executableLocations: new Set(definitionData.executableLocations), + locations: new Set(definitionData.locations), + optionalArgumentNames: new Set(definitionData.optionalArgumentNames), + requiredArgumentNames: new Set(definitionData.requiredArgumentNames), + subgraphNames: new Set(definitionData.subgraphNames), + version: importData.version, + isComposed: true, + }; + + this.federatedDirectiveDataByName.set(directiveName, federatedDirectiveData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 767 - 782, The code mutates shared directive definition singletons from directiveDefinitionDataByName by setting definitionData.version and definitionData.isComposed; instead, clone the retrieved definitionData (e.g., shallow copy/object spread or a dedicated clone helper) before assigning per-subgraph fields and before storing into federatedDirectiveDataByName so module-level *_DEFINITION_DATA objects are not modified; keep the existing checks using unknownComposeDirectiveNameError and unimportedComposeDirectiveNameError and assign definitionData.version = importData.version and definitionData.isComposed = true on the cloned object, then set the clone into federatedDirectiveDataByName.composition/src/v1/normalization/utils.ts (1)
590-605:⚠️ Potential issue | 🟠 MajorDon't compare core spec versions via
parseFloat.
v1.10becomes1.1, sov1.10sorts belowv1.2.upsertFederatedDirectiveData()later does numeric version ordering, so multi-digit minor versions can pick the wrong directive definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/utils.ts` around lines 590 - 605, The code uses parseFloat on versionString (see parseFloat(versionString.substring(1))) which collapses multi-digit minors (e.g., v1.10 -> 1.1) and breaks numeric ordering; change parsing to produce a proper version tuple or normalized string instead of a float: strip the leading 'v', split on '.', parse each segment with parseInt (or use a semver parser) and return a version representation that preserves multi-digit minors (e.g., {major, minor} or a zero-padded string) so later numeric comparisons in upsertFederatedDirectiveData() will sort correctly; update places returning and consuming version (the code returning version in this block) to use the new structured/normalized version format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/normalization/utils.ts`:
- Around line 531-544: When replacing an older directive entry with a newer
version in the version-check branch, carry forward any existing isComposed=true
so it isn't lost: before calling existingDataByName.set(directiveName,
directiveData) inside the existingData.version < directiveData.version block,
merge the composed flag (e.g., compute mergedIsComposed =
existingData.isComposed || directiveData.isComposed) and ensure the object you
store has isComposed set to that merged value so a previously composed directive
remains composed even if the newer directiveData.isComposed is false.
---
Duplicate comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 745-753: In handleComposeDirective, guard that the directive
argument's value is actually a string before calling startsWith: check
argNode.value.kind === Kind.STRING (or otherwise verify typeof
argNode.value.value === 'string') and only then read rawValue and run
startsWith(LITERAL_AT); if it's not a string, push or let existing validation
handle the appropriate error (e.g., do not call startsWith and avoid throwing).
Ensure you reference argNode, argNode.value, rawValue, LITERAL_AT, and the
errors.push(noLeadingAtComposeDirectiveNameError(...)) path when implementing
the guard.
- Around line 767-782: The code mutates shared directive definition singletons
from directiveDefinitionDataByName by setting definitionData.version and
definitionData.isComposed; instead, clone the retrieved definitionData (e.g.,
shallow copy/object spread or a dedicated clone helper) before assigning
per-subgraph fields and before storing into federatedDirectiveDataByName so
module-level *_DEFINITION_DATA objects are not modified; keep the existing
checks using unknownComposeDirectiveNameError and
unimportedComposeDirectiveNameError and assign definitionData.version =
importData.version and definitionData.isComposed = true on the cloned object,
then set the clone into federatedDirectiveDataByName.
In `@composition/src/v1/normalization/types/types.ts`:
- Around line 125-136: The AST node types ComposeDirectiveNode and
ComposeDirectiveArgumentNode currently use the ambient DOM Location; import and
use GraphQL's AST Location type instead: add an import for Location from
'graphql' (or 'graphql/language' depending on project imports) and replace the
ambient reference so that the loc?: Location fields on ComposeDirectiveNode and
ComposeDirectiveArgumentNode refer to graphql's Location type.
In `@composition/src/v1/normalization/utils.ts`:
- Around line 590-605: The code uses parseFloat on versionString (see
parseFloat(versionString.substring(1))) which collapses multi-digit minors
(e.g., v1.10 -> 1.1) and breaks numeric ordering; change parsing to produce a
proper version tuple or normalized string instead of a float: strip the leading
'v', split on '.', parse each segment with parseInt (or use a semver parser) and
return a version representation that preserves multi-digit minors (e.g., {major,
minor} or a zero-padded string) so later numeric comparisons in
upsertFederatedDirectiveData() will sort correctly; update places returning and
consuming version (the code returning version in this block) to use the new
structured/normalized version format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d38ed321-6df8-4bda-88e4-ec49ced1b913
⛔ Files ignored due to path filters (15)
connect-go/gen/proto/wg/cosmo/common/common.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/node/v1/nodev1connect/node.connect.gois excluded by!**/gen/**connect-go/gen/proto/wg/cosmo/notifications/events.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**graphqlmetrics/gen/proto/wg/cosmo/common/common.pb.gois excluded by!**/*.pb.go,!**/gen/**graphqlmetrics/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.gois excluded by!**/*.pb.go,!**/gen/**graphqlmetrics/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetricsv1connect/graphqlmetrics.connect.gois excluded by!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlrouter/gen/proto/wg/cosmo/common/common.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetrics.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/graphqlmetrics/v1/graphqlmetricsv1connect/graphqlmetrics.connect.gois excluded by!**/gen/**router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/node/v1/nodev1connect/node.connect.gois excluded by!**/gen/**
📒 Files selected for processing (5)
composition/package.jsoncomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/types/types.tscomposition/src/v1/normalization/utils.tscomposition/tsconfig.json
✅ Files skipped from review due to trivial changes (2)
- composition/package.json
- composition/tsconfig.json
Aenimus
requested review from
Noroth,
SkArchon,
devsergiy,
dkorittki,
endigma and
pepol
as code owners
github-actions
Bot
added
studio
controlplane
cli
protocol
and removed
graphqlmetrics
labels
Aenimus
force-pushed
the
david/eng-3812-implement-composedirective-directive
branch
from
049fa04 to
66cd7fc
Compare
github-actions
Bot
removed
studio
controlplane
cli
protocol
connect
router
docs-website
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.