Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech,
jensneuse and
thisisnithin
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2764 +/- ##
==========================================
+ Coverage 41.55% 45.79% +4.23%
==========================================
Files 785 1037 +252
Lines 112455 139274 +26819
Branches 8667 8679 +12
==========================================
+ Hits 46730 63777 +17047
- Misses 65362 73771 +8409
- Partials 363 1726 +1363
🚀 New features to boost your workflow:
|
Router-nonroot image scan failed❌ Security vulnerabilities found in image: Please check the security vulnerabilities found in the PR. If you believe this is a false positive, please add the vulnerability to the |
JivusAyrus
approved these changes
Apr 14, 2026
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds explicit tracking and typing for interface-to-interface implementations across composition. Introduces new type aliases and constants, refactors type-validation to accept a params object, threads an 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. ✨ Finishing Touches📝 Generate docstrings
Comment |
…implemented-interface-fields
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
composition/src/utils/types.ts (1)
1-2: Remove unused imports.
InterfaceTypeDefinitionNodeandInterfaceTypeExtensionNodeare imported but not used anywhere in this file. OnlyArgumentNameis actually utilized (line 18).♻️ Proposed fix
-import { InterfaceTypeDefinitionNode, InterfaceTypeExtensionNode, type Kind } from 'graphql'; +import { type Kind } from 'graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/utils/types.ts` around lines 1 - 2, Remove the unused GraphQL imports: delete InterfaceTypeDefinitionNode and InterfaceTypeExtensionNode (and Kind if it's not referenced elsewhere) from the top import line and only import the types that are actually used; keep the existing import of ArgumentName (and SubgraphName if used) from '../types/types' and update the import statement so it only lists the required symbols (e.g., remove InterfaceTypeDefinitionNode, InterfaceTypeExtensionNode, and Kind if unused).composition/src/schema-building/params.ts (1)
4-9: Consider using an interface instead of a type alias.Per coding guidelines, interfaces are preferred over type aliases for object shapes.
♻️ Proposed fix
-export type IsTypeValidImplementationParams = { +export interface IsTypeValidImplementationParams { concreteTypeNamesByAbstractTypeName: Map<TypeName, Set<TypeName>>; implementationType: TypeNode; interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName, Set<InterfaceTypeName>>; originalType: TypeNode; -}; +}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/params.ts` around lines 4 - 9, Replace the type alias IsTypeValidImplementationParams with an interface of the same name to follow the guideline preferring interfaces for object shapes: declare interface IsTypeValidImplementationParams { concreteTypeNamesByAbstractTypeName: Map<TypeName, Set<TypeName>>; implementationType: TypeNode; interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName, Set<InterfaceTypeName>>; originalType: TypeNode; } and keep all field names unchanged so existing usages (references to IsTypeValidImplementationParams and its properties concreteTypeNamesByAbstractTypeName, implementationType, interfaceImplementationTypeNamesByInterfaceTypeName, originalType) continue to work without further edits.composition/src/v1/normalization/normalization-factory.ts (1)
458-458: Type inconsistency between factory and types definition.The map is typed as
Map<AbstractTypeName, Set<AbstractTypeName>>here, but incomposition/src/normalization/types.ts(line 77), theBatchNormalizationSuccessinterface types this property asMap<InterfaceTypeName, Set<InterfaceTypeName>>. Similarly,NormalizationSuccess(line 45) usesMap<AbstractTypeName, Set<AbstractTypeName>>.Consider aligning these types for consistency. Since this map specifically tracks interface-to-interface implementations,
InterfaceTypeNameseems more semantically accurate.♻️ Suggested type alignment
- interfaceImplementationTypeNamesByInterfaceTypeName = new Map<AbstractTypeName, Set<AbstractTypeName>>(); + interfaceImplementationTypeNamesByInterfaceTypeName = new Map<InterfaceTypeName, Set<InterfaceTypeName>>();You'll also need to update the corresponding usages and imports to use
InterfaceTypeName.🤖 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` at line 458, The map interfaceImplementationTypeNamesByInterfaceTypeName is declared with Map<AbstractTypeName, Set<AbstractTypeName>> but the BatchNormalizationSuccess type expects Map<InterfaceTypeName, Set<InterfaceTypeName>>; change the map declaration to Map<InterfaceTypeName, Set<InterfaceTypeName>> to match semantics, update any local imports/usages to use InterfaceTypeName instead of AbstractTypeName, and ensure the related types NormalizationSuccess/BatchNormalizationSuccess signatures and code paths that read/write interfaceImplementationTypeNamesByInterfaceTypeName are adjusted accordingly so the types align throughout.composition/src/normalization/types.ts (1)
45-45: Type inconsistency betweenNormalizationSuccessandBatchNormalizationSuccess.The
interfaceImplementationTypeNamesByInterfaceTypeNameproperty has different types:
- Line 45 (
NormalizationSuccess):Map<AbstractTypeName, Set<AbstractTypeName>>- Line 77 (
BatchNormalizationSuccess):Map<InterfaceTypeName, Set<InterfaceTypeName>>Since this map specifically tracks which interfaces implement other interfaces,
InterfaceTypeNameis more semantically accurate. Consider aligning both to useInterfaceTypeName.♻️ Suggested fix for consistency
- interfaceImplementationTypeNamesByInterfaceTypeName: Map<AbstractTypeName, Set<AbstractTypeName>>; + interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName, Set<InterfaceTypeName>>;Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/normalization/types.ts` at line 45, NormalizationSuccess's property interfaceImplementationTypeNamesByInterfaceTypeName uses Map<AbstractTypeName, Set<AbstractTypeName>> which conflicts with BatchNormalizationSuccess and is semantically wrong; change the property type in the NormalizationSuccess interface to Map<InterfaceTypeName, Set<InterfaceTypeName>> so both NormalizationSuccess and BatchNormalizationSuccess use InterfaceTypeName, and update any associated type references/imports (replace AbstractTypeName usage for this property) to keep types consistent across the two interfaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@composition/src/normalization/types.ts`:
- Line 45: NormalizationSuccess's property
interfaceImplementationTypeNamesByInterfaceTypeName uses Map<AbstractTypeName,
Set<AbstractTypeName>> which conflicts with BatchNormalizationSuccess and is
semantically wrong; change the property type in the NormalizationSuccess
interface to Map<InterfaceTypeName, Set<InterfaceTypeName>> so both
NormalizationSuccess and BatchNormalizationSuccess use InterfaceTypeName, and
update any associated type references/imports (replace AbstractTypeName usage
for this property) to keep types consistent across the two interfaces.
In `@composition/src/schema-building/params.ts`:
- Around line 4-9: Replace the type alias IsTypeValidImplementationParams with
an interface of the same name to follow the guideline preferring interfaces for
object shapes: declare interface IsTypeValidImplementationParams {
concreteTypeNamesByAbstractTypeName: Map<TypeName, Set<TypeName>>;
implementationType: TypeNode;
interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName,
Set<InterfaceTypeName>>; originalType: TypeNode; } and keep all field names
unchanged so existing usages (references to IsTypeValidImplementationParams and
its properties concreteTypeNamesByAbstractTypeName, implementationType,
interfaceImplementationTypeNamesByInterfaceTypeName, originalType) continue to
work without further edits.
In `@composition/src/utils/types.ts`:
- Around line 1-2: Remove the unused GraphQL imports: delete
InterfaceTypeDefinitionNode and InterfaceTypeExtensionNode (and Kind if it's not
referenced elsewhere) from the top import line and only import the types that
are actually used; keep the existing import of ArgumentName (and SubgraphName if
used) from '../types/types' and update the import statement so it only lists the
required symbols (e.g., remove InterfaceTypeDefinitionNode,
InterfaceTypeExtensionNode, and Kind if unused).
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Line 458: The map interfaceImplementationTypeNamesByInterfaceTypeName is
declared with Map<AbstractTypeName, Set<AbstractTypeName>> but the
BatchNormalizationSuccess type expects Map<InterfaceTypeName,
Set<InterfaceTypeName>>; change the map declaration to Map<InterfaceTypeName,
Set<InterfaceTypeName>> to match semantics, update any local imports/usages to
use InterfaceTypeName instead of AbstractTypeName, and ensure the related types
NormalizationSuccess/BatchNormalizationSuccess signatures and code paths that
read/write interfaceImplementationTypeNamesByInterfaceTypeName are adjusted
accordingly so the types align throughout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a873489a-78b6-48f1-b4a1-9a53ece7c7d6
📒 Files selected for processing (12)
composition-go/index.global.jscomposition/src/ast/utils.tscomposition/src/normalization/types.tscomposition/src/schema-building/params.tscomposition/src/schema-building/utils.tscomposition/src/types/types.tscomposition/src/utils/string-constants.tscomposition/src/utils/types.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/params.tscomposition/src/v1/normalization/normalization-factory.tscomposition/tests/v1/types/interfaces.test.ts
Aenimus
deleted the
david/eng-9373-allow-interface-subtypes-for-implemented-interface-fields
branch
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
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.