Conversation
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:
WalkthroughPropagates Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2680 +/- ##
===========================================
+ Coverage 64.27% 86.05% +21.77%
===========================================
Files 305 20 -285
Lines 43266 4445 -38821
Branches 4628 984 -3644
===========================================
- Hits 27811 3825 -23986
+ Misses 15433 600 -14833
+ Partials 22 20 -2
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (1)
protographic/tests/sdl-to-proto/04-federation.test.ts (1)
1449-1458: Assert the concrete fragment fields as well.This still passes if the generator emits the
oneofwrapper and emptyPalletItem/ContainerItemmessages. Please also checkname,pallet_count, andcontainer_sizeso the test protects the actual@requirespayload shape, not just the type shell.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-to-proto/04-federation.test.ts` around lines 1449 - 1458, The test only verifies the presence of the oneof wrapper and empty message names, so update the assertions in the test (the expectations around protoText and RequireStorageItemInfoById/RequireStorageItemInfoByIdFields) to also assert that the concrete fragment messages include the expected fields: check that 'PalletItem' contains 'name' and 'pallet_count' and that 'ContainerItem' contains 'name' and 'container_size' (i.e., add expect(protoText).toContain('name'), expect(protoText).toContain('pallet_count'), and expect(protoText).toContain('container_size') or equivalent assertions referencing the same protoText used in the test) so the test verifies the actual `@requires` payload shape rather than just the type shells.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 138-156: The code currently enforces __typename for every inline
fragment; change it to only require __typename when the enclosing field's type
is abstract (interface or union). In the block that begins with if
(!this.isInlineFragment(ctx.parent)) return;, consult the enclosing field type
(e.g., a tracked this.currentFieldType or add a helper like
isAbstractType(type)) and skip the __typename requirement for concrete object
types; only run the selectionSetContainsTypename checks and call
this.ensureTypenameInSelection or push to this.validationResult.errors when the
enclosing field type is an interface or union. Use existing symbols like
isInlineFragment, selectionSetContainsTypename, currentFieldSelectionSet,
ensureTypenameInSelection, and validationResult.errors to locate and implement
the conditional.
- Around line 190-192: When popping the child selection set in the block that
checks this.isFieldNode(ctx.parent), don't leave currentFieldSelectionSet
pointing at the popped child; after calling this.fieldSelectionSetStack.pop()
restore currentFieldSelectionSet to the new top of the stack (peek the last
element or set to undefined if stack is empty) so the parent selection-set
context is correctly re-established for subsequent inline fragments; update the
code around this.isFieldNode, currentFieldSelectionSet, and
fieldSelectionSetStack.pop() to pop first then assign currentFieldSelectionSet =
this.fieldSelectionSetStack[this.fieldSelectionSetStack.length - 1] ||
undefined.
---
Nitpick comments:
In `@protographic/tests/sdl-to-proto/04-federation.test.ts`:
- Around line 1449-1458: The test only verifies the presence of the oneof
wrapper and empty message names, so update the assertions in the test (the
expectations around protoText and
RequireStorageItemInfoById/RequireStorageItemInfoByIdFields) to also assert that
the concrete fragment messages include the expected fields: check that
'PalletItem' contains 'name' and 'pallet_count' and that 'ContainerItem'
contains 'name' and 'container_size' (i.e., add
expect(protoText).toContain('name'),
expect(protoText).toContain('pallet_count'), and
expect(protoText).toContain('container_size') or equivalent assertions
referencing the same protoText used in the test) so the test verifies the actual
`@requires` payload shape rather than just the type shells.
🪄 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: 4f115c0b-a433-4ef1-8b5c-6315a207696b
📒 Files selected for processing (7)
protographic/src/abstract-selection-rewriter.tsprotographic/src/required-fields-visitor.tsprotographic/src/sdl-validation-visitor.tsprotographic/src/selection-set-validation-visitor.tsprotographic/tests/abstract-selection-rewriter/02-advanced.test.tsprotographic/tests/sdl-to-proto/04-federation.test.tsprotographic/tests/sdl-validation/02-required-validation.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
protographic/src/sdl-validation-visitor.ts (1)
154-160: Rule description is misleading.The description says
"Validates usage of __typename field which is not supported"but this rule actually enforces that__typenamemust be present in inline fragments within@requiresfield sets. The current wording suggests the opposite.📝 Suggested fix
const compositeTypeReflectionRule: LintingRule<Kind.FIELD_DEFINITION> = { name: 'use-of-typename', - description: 'Validates usage of __typename field which is not supported', + description: 'Validates that __typename is present in `@requires` selection sets containing inline fragments', enabled: true, nodeKind: Kind.FIELD_DEFINITION, validationFunction: (ctx) => this.validateCompositeTypeReflection(ctx), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-validation-visitor.ts` around lines 154 - 160, The rule defined as compositeTypeReflectionRule (name 'use-of-typename', validationFunction -> this.validateCompositeTypeReflection) has a misleading description claiming "__typename field ... is not supported" while the validator actually enforces that __typename must be present in inline fragments for `@requires` field sets; update the description string to accurately state that the rule requires __typename to be present in inline fragments used in `@requires` field sets (and optionally rename the rule from 'use-of-typename' to something like 'require-typename-in-requires' for clarity) so the name/description match the behaviour of validateCompositeTypeReflection.protographic/tests/sdl-validation/03-sdl-validation-visitor.test.ts (1)
103-109: Fragile test relies on implementation details.This test accesses and modifies the private
schemafield via(visitor as any)to force a parse error invisit(). This approach is brittle—if the internal property name changes, the test may silently stop testing the intended behavior.Consider testing the error path by subclassing or using a schema that becomes invalid only during the
visit()traversal, or document that this is intentionally testing internal error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-validation/03-sdl-validation-visitor.test.ts` around lines 103 - 109, The test should not mutate the private schema field via (visitor as any); instead trigger the visit() error path without touching internals by either passing an SDL string that will become invalid during traversal (use cleanSdl variant that contains a construct causing parse-time failure) or create a test-only subclass of SDLValidationVisitor that overrides visit() to simulate a parse failure (e.g., class TestVisitor extends SDLValidationVisitor { visit() { throw new TypeError('simulated parse error'); } }). Replace the direct (visitor as any).schema assignment with one of these approaches so the test still asserts that visitor.visit() throws TypeError while avoiding brittle access to the private schema field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 194-200: The call to
this.parsedSchema.getType(parentNode.name.value) may return undefined and is
being force-cast to GraphQLObjectType before passing into
SelectionSetValidationVisitor; add a null/undefined check and a runtime type
guard: call const parentType = this.parsedSchema.getType(parentNode.name.value);
if (!parentType || !(parentType instanceof GraphQLObjectType)) then handle
gracefully (throw a descriptive error or return/skip validation for that node)
otherwise pass parentType into new SelectionSetValidationVisitor; ensure the
error message mentions parentNode.name.value for easier debugging.
---
Nitpick comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 154-160: The rule defined as compositeTypeReflectionRule (name
'use-of-typename', validationFunction -> this.validateCompositeTypeReflection)
has a misleading description claiming "__typename field ... is not supported"
while the validator actually enforces that __typename must be present in inline
fragments for `@requires` field sets; update the description string to accurately
state that the rule requires __typename to be present in inline fragments used
in `@requires` field sets (and optionally rename the rule from 'use-of-typename'
to something like 'require-typename-in-requires' for clarity) so the
name/description match the behaviour of validateCompositeTypeReflection.
In `@protographic/tests/sdl-validation/03-sdl-validation-visitor.test.ts`:
- Around line 103-109: The test should not mutate the private schema field via
(visitor as any); instead trigger the visit() error path without touching
internals by either passing an SDL string that will become invalid during
traversal (use cleanSdl variant that contains a construct causing parse-time
failure) or create a test-only subclass of SDLValidationVisitor that overrides
visit() to simulate a parse failure (e.g., class TestVisitor extends
SDLValidationVisitor { visit() { throw new TypeError('simulated parse error'); }
}). Replace the direct (visitor as any).schema assignment with one of these
approaches so the test still asserts that visitor.visit() throws TypeError while
avoiding brittle access to the private schema field.
🪄 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: 352d4650-247b-4120-8847-266441b22cfa
📒 Files selected for processing (5)
protographic/.gitignoreprotographic/src/sdl-validation-visitor.tsprotographic/src/selection-set-validation-visitor.tsprotographic/tests/sdl-validation/03-sdl-validation-visitor.test.tsprotographic/vite.config.ts
✅ Files skipped from review due to trivial changes (2)
- protographic/vite.config.ts
- protographic/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/src/selection-set-validation-visitor.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
protographic/src/selection-set-validation-visitor.ts (1)
179-181:⚠️ Potential issue | 🟠 MajorRestore the parent selection-set context after popping.
pop()returns the child selection set you're leaving, so assigning it tocurrentFieldSelectionSetkeeps a stale pointer instead of the parent. This could cause subsequent inline fragments in the same enclosing scope to read the wrong__typenamecontext.🐛 Proposed fix
if (this.isFieldNode(ctx.parent)) { - this.currentFieldSelectionSet = this.fieldSelectionSetStack.pop(); + this.fieldSelectionSetStack.pop(); + this.currentFieldSelectionSet = this.fieldSelectionSetStack.at(-1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/selection-set-validation-visitor.ts` around lines 179 - 181, When leaving a field node in selection-set-validation-visitor.ts the code currently assigns the popped child selection set to this.currentFieldSelectionSet; instead, pop() should remove the child and then restore the parent. Change the logic in the isFieldNode branch: call this.fieldSelectionSetStack.pop() to remove the child, then set this.currentFieldSelectionSet to the stack's new top (e.g., this.fieldSelectionSetStack[this.fieldSelectionSetStack.length - 1] || undefined) so the parent selection-set context is correctly restored for later inline fragment checks.
🧹 Nitpick comments (2)
protographic/src/selection-set-validation-visitor.ts (1)
148-156: Mutating readonlyselectionsarray violates type contract.
SelectionSetNode.selectionsis typed asreadonly SelectionNode[]. Directly reassigning it bypasses TypeScript's immutability guarantees. While this works at runtime, it's a type violation that could cause issues if the AST is reused elsewhere.Consider using the visitor's return value mechanism to replace the node immutably:
♻️ Suggested approach using immutable replacement
Instead of mutating in
ensureTypenameInSelection, return a new node from the visitor:private onEnterSelectionSet(ctx: VisitContext<SelectionSetNode>): void { + // ... existing checks ... + if ( !this.selectionSetContainsTypename(ctx.node) && !this.selectionSetContainsTypename(this.currentFieldSelectionSet) ) { if (!this.fix) { // ... error handling ... return; } - this.ensureTypenameInSelection(ctx.node); + return { + ...ctx.node, + selections: [ + { kind: Kind.FIELD, name: { kind: Kind.NAME, value: '__typename' } }, + ...ctx.node.selections, + ], + }; } }This returns a new node rather than mutating in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/selection-set-validation-visitor.ts` around lines 148 - 156, ensureTypenameInSelection currently mutates SelectionSetNode.selections (typed readonly) which violates the AST immutability contract; change the logic to return a new SelectionSetNode instead of reassigning selections — construct a fresh node with the same properties but with selections = [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value: '__typename' } }, ...oldSelections] and have the visitor use that return value (i.e., return the new node from the visitor method that calls ensureTypenameInSelection) so the AST is replaced immutably rather than mutated in place.protographic/src/sdl-validation-visitor.ts (1)
154-160: Rule description is misleading.The description says
"Validates usage of __typename field which is not supported", but this rule actually requires__typenameto be present in inline fragments on composite types. Consider updating to match the actual validation behavior.📝 Suggested description
const compositeTypeReflectionRule: LintingRule<Kind.FIELD_DEFINITION> = { name: 'use-of-typename', - description: 'Validates usage of __typename field which is not supported', + description: 'Validates that __typename is present in inline fragments on composite types within `@requires` directives', enabled: true, nodeKind: Kind.FIELD_DEFINITION, validationFunction: (ctx) => this.validateCompositeTypeReflection(ctx), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-validation-visitor.ts` around lines 154 - 160, The rule compositeTypeReflectionRule currently has a misleading description ("Validates usage of __typename field which is not supported") while its validationFunction (validateCompositeTypeReflection) enforces that __typename must be present in inline fragments on composite types; update the description string on compositeTypeReflectionRule to clearly state that the rule requires/validates presence of __typename in inline fragments for composite types (and optionally adjust the rule name 'use-of-typename' if you want clearer naming) so the description matches the actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 179-181: When leaving a field node in
selection-set-validation-visitor.ts the code currently assigns the popped child
selection set to this.currentFieldSelectionSet; instead, pop() should remove the
child and then restore the parent. Change the logic in the isFieldNode branch:
call this.fieldSelectionSetStack.pop() to remove the child, then set
this.currentFieldSelectionSet to the stack's new top (e.g.,
this.fieldSelectionSetStack[this.fieldSelectionSetStack.length - 1] ||
undefined) so the parent selection-set context is correctly restored for later
inline fragment checks.
---
Nitpick comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 154-160: The rule compositeTypeReflectionRule currently has a
misleading description ("Validates usage of __typename field which is not
supported") while its validationFunction (validateCompositeTypeReflection)
enforces that __typename must be present in inline fragments on composite types;
update the description string on compositeTypeReflectionRule to clearly state
that the rule requires/validates presence of __typename in inline fragments for
composite types (and optionally adjust the rule name 'use-of-typename' if you
want clearer naming) so the description matches the actual behavior.
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 148-156: ensureTypenameInSelection currently mutates
SelectionSetNode.selections (typed readonly) which violates the AST immutability
contract; change the logic to return a new SelectionSetNode instead of
reassigning selections — construct a fresh node with the same properties but
with selections = [{ kind: Kind.FIELD, name: { kind: Kind.NAME, value:
'__typename' } }, ...oldSelections] and have the visitor use that return value
(i.e., return the new node from the visitor method that calls
ensureTypenameInSelection) so the AST is replaced immutably rather than mutated
in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c10d3ad-859a-45dd-9aca-85ea8c0ac97d
📒 Files selected for processing (3)
protographic/src/sdl-validation-visitor.tsprotographic/src/selection-set-validation-visitor.tsprotographic/tests/sdl-validation/02-required-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/tests/sdl-validation/02-required-validation.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protographic/src/sdl-validation-visitor.ts (1)
95-108:⚠️ Potential issue | 🟠 MajorMove schema parsing and building into
visit()to properly handle errors.
buildASTSchema(parse(schema))in the constructor can throw and will bypass the error normalization invisit(). SinceparsedSchemais only used during visitor execution, move the schema building to insidevisit()after parsing, and build from the already-parsedastNodeto avoid redundant parsing.Additionally, line 175 uses string concatenation instead of a template literal:
- const { error, documentNode } = safeParse('{' + fieldSetValue + '}'); + const { error, documentNode } = safeParse(`{${fieldSetValue}}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-validation-visitor.ts` around lines 95 - 108, The constructor currently calls parse() and buildASTSchema() and assigns to parsedSchema which can throw before visit() can normalize errors; move schema parsing/building into the visit() method: remove buildASTSchema(parse(schema)) from the constructor, in visit() use the already-parsed astNode (or call parse once there) and then call buildASTSchema(astNode, {assumeValid: true, assumeValidSDL: true}) to create parsedSchema just before any validation logic; update all uses of parsedSchema to reference the instance field populated in visit(); also replace the string concatenation at the location using '+' (around the former line 175) with a template literal for clarity.
🧹 Nitpick comments (1)
protographic/src/sdl-validation-visitor.ts (1)
174-176: Prefer a template literal for the wrapped field set.
safeParse(\{${fieldSetValue}}`)` is a little cleaner here and keeps the new path aligned with the repo style.As per coding guidelines "Use template literals instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-validation-visitor.ts` around lines 174 - 176, The code currently concatenates the wrapped field set when calling safeParse using '{' + fieldSetValue + '}', please replace that concatenation with a template literal so the call becomes safeParse(`{${fieldSetValue}}`); update the invocation around the fieldSetValue variable (the safeParse call) to use the template literal while preserving the existing semantics and surrounding error/documentNode handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protographic/SDL_PROTO_RULES.md`:
- Around line 342-345: The SDL block for the Storage type has an inline
`@requires`(fields: "...") example that the project formatter rewrites into a
canonical multi-line form; update the committed content for the Storage type's
itemInfo field (the itemInfo field on type Storage using `@requires` with nested
selection of primaryItem and inline fragments for PalletItem and ContainerItem)
to match the formatter's multi-line output so the repository no longer shows a
dirty diff—run the SDL formatter and commit its rewritten version for the
Storage/itemInfo `@requires` example.
- Around line 308-316: Open SDL_PROTO_RULES.md and update the two fenced code
blocks that show the example GraphQL snippets so they include the language
identifier "graphql"; locate the first block containing "media { id ... on Book
{ author } ... on Movie { director } }" and the second block containing "media {
... on Book { id author } ... on Movie { id director } }" and change their
fenced backticks to start with ```graphql (i.e., ```graphql ... ```).
---
Outside diff comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 95-108: The constructor currently calls parse() and
buildASTSchema() and assigns to parsedSchema which can throw before visit() can
normalize errors; move schema parsing/building into the visit() method: remove
buildASTSchema(parse(schema)) from the constructor, in visit() use the
already-parsed astNode (or call parse once there) and then call
buildASTSchema(astNode, {assumeValid: true, assumeValidSDL: true}) to create
parsedSchema just before any validation logic; update all uses of parsedSchema
to reference the instance field populated in visit(); also replace the string
concatenation at the location using '+' (around the former line 175) with a
template literal for clarity.
---
Nitpick comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 174-176: The code currently concatenates the wrapped field set
when calling safeParse using '{' + fieldSetValue + '}', please replace that
concatenation with a template literal so the call becomes
safeParse(`{${fieldSetValue}}`); update the invocation around the fieldSetValue
variable (the safeParse call) to use the template literal while preserving the
existing semantics and surrounding error/documentNode handling.
🪄 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: 3001a44d-c763-4a48-a1fb-4dc7f52dd2d6
📒 Files selected for processing (2)
protographic/SDL_PROTO_RULES.mdprotographic/src/sdl-validation-visitor.ts
endigma
requested changes
Mar 23, 2026
endigma
approved these changes
Mar 23, 2026
devsergiy
approved these changes
Mar 23, 2026
Noroth
deleted the
ludwig/eng-9277-include-__typename-for-composite-types-in-protographic
branch
5 tasks
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
3 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.
When using composite types in
@requiresin protobuf, we need to determine the type from the subgraph to properly set the proto message data. To ensure this is defined in the schema we add a new linting rule for field sets in@requiresdirectives.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.