feat: add support for federated graph schema changes#2566
Merged
JivusAyrus merged 24 commits intomainfrom Mar 4, 2026
Merged
Conversation
…te check summary logic
… related schema handling
JivusAyrus
requested review from
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
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:
WalkthroughThis PR introduces federated graph schema breaking changes tracking by extending the schema check system to detect, store, and report breaking changes that occur at the composed federated graph level, distinct from subgraph-level changes. Changes span database migrations, protobuf definitions, backend services, CLI handling, and frontend UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controlplane/src/core/bufservices/check/getCheckSummary.ts (1)
216-237:⚠️ Potential issue | 🟠 MajorMissing federated-change context for non-current affected graphs
Line 216 calls
checkDetailswithout options, socomposedSchemaBreakingChangesis always empty in this loop. That under-reportsisBreakingon Line 234 and can incorrectly mark affected graphs as successful.Suggested fix
- const checkDetails = await subgraphRepo.checkDetails(req.checkId, fedGraph.targetId); + const checkDetails = await subgraphRepo.checkDetails(req.checkId, fedGraph.targetId, { + federatedGraphId: fedGraph.id, + federatedGraphName: fedGraph.name, + namespaceId: namespace.id, + schemaCheckRepo, + operationsRepo, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/check/getCheckSummary.ts` around lines 216 - 237, The call to subgraphRepo.checkDetails(req.checkId, fedGraph.targetId) omits the federated-change context so checkDetails.composedSchemaBreakingChanges is empty for non-current graphs; update the subgraphRepo.checkDetails invocation (within the loop over fedGraph) to pass the proper federated-change context/options (e.g., include the federatedChangeId or enable composed schema changes) so checkDetails returns composedSchemaBreakingChanges and isBreaking is computed correctly for affected graphs.controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
1135-1172:⚠️ Potential issue | 🟠 MajorUse per-federated-graph router compatibility for composed diffs
Line 1135 derives one compatibility version for all graphs, then reuses it on Line 1168-1172. If federated graphs have different
routerCompatibilityVersion, composed diff results can be wrong.Suggested fix
- const routerCompatibilityVersion = getFederatedGraphRouterCompatibilityVersion(federatedGraphs); + const routerCompatibilityByGraphId = new Map( + federatedGraphs.map((fg) => [fg.id, fg.routerCompatibilityVersion]), + ); @@ - const federatedSchemaDiff = await getDiffBetweenGraphs( + const routerCompatibilityVersion = + routerCompatibilityByGraphId.get(composition.id) ?? + getFederatedGraphRouterCompatibilityVersion([composition as any]); + + const federatedSchemaDiff = await getDiffBetweenGraphs( oldSchemaVersion.clientSchema, composition.federatedClientSchema, routerCompatibilityVersion, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/SchemaCheckRepository.ts` around lines 1135 - 1172, The code computes a single routerCompatibilityVersion once (via getFederatedGraphRouterCompatibilityVersion(federatedGraphs)) and reuses it for all composed graphs, causing incorrect diffs if graphs have different compatibility; instead compute the router compatibility per federated graph inside the composedGraphs loop and pass that per-graph value to getDiffBetweenGraphs. Concretely, inside the for (const composition of composedGraphs) loop call getFederatedGraphRouterCompatibilityVersion for the specific composition (or find the matching entry in federatedGraphs by composition.name/targetID) into a local variable (e.g., perFedRouterCompatibilityVersion) and use that when invoking getDiffBetweenGraphs; keep other logic (fedGraphRepo.getLatestValidSchemaVersion, composition.federatedClientSchema, etc.) unchanged.controlplane/src/core/repositories/SubgraphRepository.ts (1)
2198-2220:⚠️ Potential issue | 🟠 MajorDo not skip federated traffic inspection for new subgraphs
Line 2198 short-circuits on
!subgraph, which suppresses traffic checks even whenfedGraphInspectorChangesexists. That can incorrectly keephasClientTrafficfalse for composed federated breaking changes.Suggested fix
- if (composedGraph.errors.length > 0 || combinedInspectorChanges.length === 0 || skipTrafficCheck || !subgraph) { + if (composedGraph.errors.length > 0 || combinedInspectorChanges.length === 0 || skipTrafficCheck) { continue; } - - const result = await trafficInspector.inspect(combinedInspectorChanges, { - daysToConsider: limit, - federatedGraphId: composedGraph.id, - organizationId: this.organizationId, - subgraphId: subgraph.id, - }); + let result: Map<string, InspectorOperationResult[]> = new Map(); + if (subgraph) { + result = await trafficInspector.inspect(combinedInspectorChanges, { + daysToConsider: limit, + federatedGraphId: composedGraph.id, + organizationId: this.organizationId, + subgraphId: subgraph.id, + }); + } else if (fedGraphInspectorChanges.length > 0) { + result = await trafficInspector.inspect(fedGraphInspectorChanges, { + daysToConsider: limit, + federatedGraphId: composedGraph.id, + organizationId: this.organizationId, + }); + }Based on learnings: In the Cosmo schema check system,
hasClientTrafficindicates unsafe/problematic impacted traffic and is used as a failure condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/SubgraphRepository.ts` around lines 2198 - 2220, The early continue currently skips traffic inspection whenever subgraph is falsy (the `!subgraph` clause), which prevents inspecting federated changes for new subgraphs; change the short-circuit condition so we only skip when there is no subgraph AND there are no inspector changes — i.e. replace the `!subgraph` check in the if at the top (the block referencing composedGraph.errors, combinedInspectorChanges, skipTrafficCheck, subgraph) with a combined check such as `(!subgraph && combinedInspectorChanges.length === 0)` so that `trafficInspector.inspect(...)` (and subsequent logic around storedBreakingChanges, storedFedGraphBreakingChanges, and schemaCheckRepo.checkClientTrafficAgainstOverrides with namespace.id) still runs when federated inspector changes exist for a new subgraph.
🧹 Nitpick comments (2)
controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts (1)
4-4: ImportMockas a type-only symbol.
Mockis imported at line 4 but used only as a type in a type assertion at line 679. Usetype Mockto follow TypeScript best practices.🔧 Proposed fix
-import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, Mock } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, type Mock } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts` at line 4, Change the runtime import of Mock to a type-only import: replace the current import that brings Mock from 'vitest' with a type import (e.g., "type Mock") so Mock is removed from emitted JS; update the import statement that currently lists Mock alongside other runtime imports (afterAll, beforeAll, etc.) to import Mock as a type-only symbol, since Mock is only used in a type assertion (the Mock type used in the test file).controlplane/test/check-subgraph-schema.test.ts (1)
2561-2705: Consider simplifying the mock setup.The test has 16 sequential mock responses which makes the test brittle and hard to maintain. Consider using
mockResolvedValue(withoutOnce) to return the same data for all calls, or restructure to reduce the number of mocked calls needed.♻️ Suggested simplification
- // Mock traffic - 8 calls for source subgraphs and 8 calls for target subgraphs - (chClient.queryPromise as Mock) - .mockResolvedValueOnce([ - { - operationHash: 'hash1', - operationName: 'op1', - operationType: 'query', - firstSeen: Date.now() / 1000, - lastSeen: Date.now() / 1000, - }, - ]) - .mockResolvedValueOnce([...]) - // ... 14 more similar calls + // Mock traffic - return same data for all calls + (chClient.queryPromise as Mock).mockResolvedValue([ + { + operationHash: 'hash1', + operationName: 'op1', + operationType: 'query', + firstSeen: Date.now() / 1000, + lastSeen: Date.now() / 1000, + }, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/check-subgraph-schema.test.ts` around lines 2561 - 2705, The test is brittle due to 16 repeated .mockResolvedValueOnce(...) calls on chClient.queryPromise; replace them with a single stable mock (e.g. chClient.queryPromise.mockResolvedValue(<response>) or chClient.queryPromise.mockImplementation(() => Promise.resolve(<response>))) or set a local constant response and use mockResolvedValue to cover all calls, updating the mock in the test around chClient.queryPromise so all invocations return the same operation object instead of chaining many .mockResolvedValueOnce calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/handle-check-result.ts`:
- Around line 121-126: The modified blocks in handle-check-result (the
destructuring of resp into breakingChanges, operationUsageStats,
clientTrafficCheckSkipped, composedSchemaBreakingChanges and the destructure of
operationUsageStats into totalOperations, safeOperations, firstSeenAt,
lastSeenAt, plus the creation of warningMessage using logSymbols and pc.bold)
need Prettier-style reformatting to satisfy CI; re-run the project's formatter
(or manually adjust) so spacing, line breaks and punctuation match existing code
style for these expressions and the other flagged blocks around the same file
(the blocks near the warningMessage creation and the ranges noted 145-147 and
291-293), ensuring consistent indentation, proper semicolon/comma placement and
line wrapping for arrays/template strings and keeping the same variable names
(breakingChanges, composedSchemaBreakingChanges, operationUsageStats,
warningMessage, logSymbols, pc.bold) so only formatting changes are made.
In `@controlplane/migrations/0136_powerful_sasquatch.sql`:
- Around line 1-6: The table schema_check_federated_graph_changes allows
duplicate pairs of (schema_check_federated_graph_id,
schema_check_change_action_id); add a composite uniqueness constraint (or unique
index) on those two columns to enforce mapping integrity and prevent duplicate
federated-breaking entries. Modify the CREATE TABLE or add an ALTER TABLE ...
ADD CONSTRAINT ... UNIQUE (schema_check_federated_graph_id,
schema_check_change_action_id) (or CREATE UNIQUE INDEX ...) so inserts that
would duplicate that pair fail; apply the same change for the analogous tables
referenced in the comment (rows 21–22).
In `@controlplane/src/bin/get-config.ts`:
- Line 10: The fallback Keycloak URL in get-config.ts is inconsistent with infra
(should be port 8080); change the hardcoded fallback value for apiUrl (the
expression using process.env.KC_API_URL) back to 'http://localhost:8080' so it
matches KC_API_URL across environment files, docker-compose, Helm values and
scripts (or, if the intent was to move to 8090, update all those configs
instead—ensure KC_API_URL usage is consistent project-wide and keep the apiUrl
fallback in get-config.ts aligned).
In `@controlplane/src/core/repositories/SchemaCheckRepository.ts`:
- Around line 165-189: Wrap the two dependent inserts in
createFederatedGraphSchemaChanges in a single DB transaction so the mapping
insert cannot fail separately from the schema change insert: start a transaction
(using your DB client transaction API), call createSchemaCheckChanges within
that transaction (pass a transactional DB/context into createSchemaCheckChanges
or add an overload to accept tx) to insert into schema_check_change_action, then
insert into schema.schemaCheckFederatedGraphChanges using the same tx; commit
the transaction and return the inserted changes, rolling back on any error to
avoid orphaned schema_check_change_action rows.
In
`@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts`:
- Line 158: Wrap each test's server lifecycle in a try/finally so server.close()
always runs: for each test that creates/starts a server (the variable named
server in the test bodies), move the test logic into a try block and call await
server.close() in the finally block (also handle cases where server may be
undefined/null by checking before closing). Apply this pattern to all
occurrences that currently call await server.close() only on success (the tests
containing server and the solitary await server.close() calls).
- Around line 40-42: The afterEach currently calls vi.clearAllMocks(), which
only clears call history and allows mock implementations (e.g.,
mockResolvedValue set earlier) to persist across tests; replace that call with
vi.resetAllMocks() in the same afterEach block so all mock implementations and
states are fully reset between tests, preventing the stale implementation from
leaking into subsequent chClient instances.
In `@studio/src/components/checks/composed-schema-changes-table.tsx`:
- Around line 119-143: The current Button uses asChild to render a Link while
passing disabled, but anchors ignore disabled so keyboard users can still
focus/activate it; update the render to conditionally choose between two
branches: when path is truthy render the existing Button with asChild wrapping
the Link (keep href building using organizationSlug, namespace,
router.query.slug and typename derived from path.split(".")[0]) and when path is
falsy render a plain disabled Button (remove asChild and Link) that uses the
Button's disabled prop and/or aria-disabled and tabIndex=-1 to prevent keyboard
focus/activation; target the Button/Link/GlobeIcon JSX in
composed-schema-changes-table.tsx to implement this conditional rendering.
In
`@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:
- Around line 1637-1655: The current conditional renders an EmptyState whenever
data.changes.length === 0, which is misleading if there are composed schema
breaking changes; update the rendering logic around the ChangesTable /
EmptyState block (the JSX that checks data.changes.length and renders
<ChangesTable> or <EmptyState>) to treat composed schema breaking changes as a
non-empty result — e.g., render the ChangesTable (or a combined view) when
either data.changes.length > 0 OR data.composedSchemaBreakingChanges.length > 0;
do the same adjustment for the similar conditional that covers the
federated-breaking/composed section (the second block around the checks for
composedSchemaBreakingChanges) so the page never shows an EmptyState while
composedSchemaBreakingChanges are present.
---
Outside diff comments:
In `@controlplane/src/core/bufservices/check/getCheckSummary.ts`:
- Around line 216-237: The call to subgraphRepo.checkDetails(req.checkId,
fedGraph.targetId) omits the federated-change context so
checkDetails.composedSchemaBreakingChanges is empty for non-current graphs;
update the subgraphRepo.checkDetails invocation (within the loop over fedGraph)
to pass the proper federated-change context/options (e.g., include the
federatedChangeId or enable composed schema changes) so checkDetails returns
composedSchemaBreakingChanges and isBreaking is computed correctly for affected
graphs.
In `@controlplane/src/core/repositories/SchemaCheckRepository.ts`:
- Around line 1135-1172: The code computes a single routerCompatibilityVersion
once (via getFederatedGraphRouterCompatibilityVersion(federatedGraphs)) and
reuses it for all composed graphs, causing incorrect diffs if graphs have
different compatibility; instead compute the router compatibility per federated
graph inside the composedGraphs loop and pass that per-graph value to
getDiffBetweenGraphs. Concretely, inside the for (const composition of
composedGraphs) loop call getFederatedGraphRouterCompatibilityVersion for the
specific composition (or find the matching entry in federatedGraphs by
composition.name/targetID) into a local variable (e.g.,
perFedRouterCompatibilityVersion) and use that when invoking
getDiffBetweenGraphs; keep other logic
(fedGraphRepo.getLatestValidSchemaVersion, composition.federatedClientSchema,
etc.) unchanged.
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 2198-2220: The early continue currently skips traffic inspection
whenever subgraph is falsy (the `!subgraph` clause), which prevents inspecting
federated changes for new subgraphs; change the short-circuit condition so we
only skip when there is no subgraph AND there are no inspector changes — i.e.
replace the `!subgraph` check in the if at the top (the block referencing
composedGraph.errors, combinedInspectorChanges, skipTrafficCheck, subgraph) with
a combined check such as `(!subgraph && combinedInspectorChanges.length === 0)`
so that `trafficInspector.inspect(...)` (and subsequent logic around
storedBreakingChanges, storedFedGraphBreakingChanges, and
schemaCheckRepo.checkClientTrafficAgainstOverrides with namespace.id) still runs
when federated inspector changes exist for a new subgraph.
---
Nitpick comments:
In `@controlplane/test/check-subgraph-schema.test.ts`:
- Around line 2561-2705: The test is brittle due to 16 repeated
.mockResolvedValueOnce(...) calls on chClient.queryPromise; replace them with a
single stable mock (e.g. chClient.queryPromise.mockResolvedValue(<response>) or
chClient.queryPromise.mockImplementation(() => Promise.resolve(<response>))) or
set a local constant response and use mockResolvedValue to cover all calls,
updating the mock in the test around chClient.queryPromise so all invocations
return the same operation object instead of chaining many .mockResolvedValueOnce
calls.
In
`@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts`:
- Line 4: Change the runtime import of Mock to a type-only import: replace the
current import that brings Mock from 'vitest' with a type import (e.g., "type
Mock") so Mock is removed from emitted JS; update the import statement that
currently lists Mock alongside other runtime imports (afterAll, beforeAll, etc.)
to import Mock as a type-only symbol, since Mock is only used in a type
assertion (the Mock type used in the test file).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (20)
cli/src/commands/mcp/tools/subgraph-verify-schema-changes.tscli/src/handle-check-result.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0136_powerful_sasquatch.sqlcontrolplane/migrations/meta/0136_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/bin/get-config.tscontrolplane/src/core/bufservices/check/getCheckSummary.tscontrolplane/src/core/bufservices/federated-graph/checkFederatedGraph.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/services/SchemaUsageTrafficInspector.tscontrolplane/src/db/schema.tscontrolplane/src/types/index.tscontrolplane/test/check-subgraph-schema.test.tscontrolplane/test/proposal/proposal-federated-schema-breaking-changes.test.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/components/checks/composed-schema-changes-table.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx
controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts
Show resolved
Hide resolved
controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controlplane/src/db/schema.ts (1)
975-1000: Index prefix inconsistency with table comment convention.The table comment abbreviation is
scfgcbut the index names usescfgsc_prefix. Other tables in this schema consistently use the comment abbreviation as the index prefix (e.g.,scfg_forschemaCheckFederatedGraphs).Additionally, consider whether a unique constraint on
(schemaCheckFederatedGraphId, schemaCheckChangeActionId)is needed to prevent duplicate mappings.🔧 Suggested fix for index naming
(t) => { return { - schemaCheckFederatedGraphIdIndex: index('scfgsc_schema_check_federated_graph_id_idx').on( + schemaCheckFederatedGraphIdIndex: index('scfgc_schema_check_federated_graph_id_idx').on( t.schemaCheckFederatedGraphId, ), - schemaCheckChangeActionIdIndex: index('scfgsc_schema_check_change_action_id_idx').on(t.schemaCheckChangeActionId), + schemaCheckChangeActionIdIndex: index('scfgc_schema_check_change_action_id_idx').on(t.schemaCheckChangeActionId), }; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/db/schema.ts` around lines 975 - 1000, The index names on the schemaCheckFederatedGraphChanges pgTable use the wrong prefix (they currently start with "scfgsc_"); rename the index identifiers used in the index(...) calls (schemaCheckFederatedGraphIdIndex and schemaCheckChangeActionIdIndex) to use the table comment abbreviation "scfgc_" to match convention, and also add a unique constraint or unique index on (schemaCheckFederatedGraphId, schemaCheckChangeActionId) to prevent duplicate mappings (add a unique index in the same table definition referencing those two columns).cli/src/handle-check-result.ts (1)
125-150: Optional: extract combined breaking-change count into a local constant.This removes repetition and makes the warning/final-statement logic easier to maintain.
♻️ Suggested cleanup
- if (breakingChanges.length > 0 || composedSchemaBreakingChanges.length > 0) { + const totalBreakingChanges = breakingChanges.length + composedSchemaBreakingChanges.length; + if (totalBreakingChanges > 0) { const warningMessage = [ logSymbols.warning, - ` Found ${pc.bold(breakingChanges.length + composedSchemaBreakingChanges.length)} breaking changes.`, + ` Found ${pc.bold(String(totalBreakingChanges))} breaking changes.`, ]; @@ - finalStatement = `This check has encountered ${pc.bold(`${breakingChanges.length + composedSchemaBreakingChanges.length}`)} breaking changes${ + finalStatement = `This check has encountered ${pc.bold(String(totalBreakingChanges))} breaking changes${ clientTrafficCheckSkipped ? `.` : ` that would break operations from existing client traffic.` }`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/handle-check-result.ts` around lines 125 - 150, Define a local constant for the combined breaking-change count (e.g., const totalBreaking = breakingChanges.length + composedSchemaBreakingChanges.length) near the top of the block and replace all repeated expressions with that constant in the warningMessage construction and in finalStatement; update references in the conditional message pieces that currently use breakingChanges.length + composedSchemaBreakingChanges.length and ensure the pc.bold(...) uses totalBreaking, keeping clientTrafficCheckSkipped, totalOperations, safeOperations, firstSeenAt and lastSeenAt logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/src/handle-check-result.ts`:
- Around line 125-150: Define a local constant for the combined breaking-change
count (e.g., const totalBreaking = breakingChanges.length +
composedSchemaBreakingChanges.length) near the top of the block and replace all
repeated expressions with that constant in the warningMessage construction and
in finalStatement; update references in the conditional message pieces that
currently use breakingChanges.length + composedSchemaBreakingChanges.length and
ensure the pc.bold(...) uses totalBreaking, keeping clientTrafficCheckSkipped,
totalOperations, safeOperations, firstSeenAt and lastSeenAt logic unchanged.
In `@controlplane/src/db/schema.ts`:
- Around line 975-1000: The index names on the schemaCheckFederatedGraphChanges
pgTable use the wrong prefix (they currently start with "scfgsc_"); rename the
index identifiers used in the index(...) calls (schemaCheckFederatedGraphIdIndex
and schemaCheckChangeActionIdIndex) to use the table comment abbreviation
"scfgc_" to match convention, and also add a unique constraint or unique index
on (schemaCheckFederatedGraphId, schemaCheckChangeActionId) to prevent duplicate
mappings (add a unique index in the same table definition referencing those two
columns).
…-should-also-be-done-on
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
studio/src/components/checks/composed-schema-changes-table.tsx (2)
78-78: Remove redundantkeyprop on TableRow.The
keyonTableRowhas no effect for React's reconciliation since theRowcomponent is already keyed at the parent map level (line 43). If you adopt the composite key suggestion above, you can remove this redundant prop.♻️ Suggested change
- <TableRow key={changeType + message + federatedGraphName} className="group"> + <TableRow className="group">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/composed-schema-changes-table.tsx` at line 78, Remove the redundant key prop from the TableRow element: the parent Row in the mapped list already supplies the React key (use the composite key formed from changeType, message and federatedGraphName at the map level), so delete the key attribute from TableRow (symbol: TableRow) and ensure the parent mapping (the Row component used to render each item) uses the composite key for stable identity.
41-50: Consider using a composite key instead of array index.The
Rowcomponent useskey={i}but constructs a composite key inside on line 78. Using index as key can cause subtle reconciliation issues if thechangesarray is reordered or filtered. Since a natural key exists, move it to the map level:♻️ Suggested improvement
<TableBody> {changes.map((c, i) => ( <Row - key={i} + key={`${c.changeType}-${c.message}-${c.federatedGraphName}`} changeType={c.changeType} message={c.message} isBreaking={c.isBreaking} path={c.path} federatedGraphName={c.federatedGraphName} /> ))} </TableBody>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/checks/composed-schema-changes-table.tsx` around lines 41 - 50, The map over changes currently uses key={i}; replace the index with the same composite key logic used inside Row (combine changeType, path and federatedGraphName or another stable unique identifier from each change) so the map uses a stable natural key instead of i; update the changes.map call to set key to that composite (e.g. `${c.changeType}:${c.path}:${c.federatedGraphName}`) while leaving the Row props (changeType, message, isBreaking, path, federatedGraphName) unchanged.controlplane/test/check-subgraph-schema.test.ts (1)
2562-2706: Consider simplifying the repetitive mock setup.The mock setup contains 16 nearly identical
mockResolvedValueOncecalls. This is verbose and harder to maintain. You could simplify using a helper function or a loop.♻️ Suggested simplification
- // Mock traffic - 8 calls for source subgraphs and 8 calls for target subgraphs - (chClient.queryPromise as Mock) - .mockResolvedValueOnce([ - { - operationHash: 'hash1', - operationName: 'op1', - operationType: 'query', - firstSeen: Date.now() / 1000, - lastSeen: Date.now() / 1000, - }, - ]) - // ... 15 more identical calls ... + // Mock traffic - 16 calls for source and target subgraphs + const mockOperation = { + operationHash: 'hash1', + operationName: 'op1', + operationType: 'query', + firstSeen: Date.now() / 1000, + lastSeen: Date.now() / 1000, + }; + for (let i = 0; i < 16; i++) { + (chClient.queryPromise as Mock).mockResolvedValueOnce([mockOperation]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/check-subgraph-schema.test.ts` around lines 2562 - 2706, Replace the 16 repeated .mockResolvedValueOnce calls on chClient.queryPromise with a concise loop or helper: create a single mock response object (e.g. const mockOp = { operationHash: 'hash1', operationName: 'op1', operationType: 'query', firstSeen: Date.now() / 1000, lastSeen: Date.now() / 1000 }) and then call (chClient.queryPromise as Mock).mockResolvedValueOnce(mockOpArray) inside a for loop or use Array.from to iterate 16 times, or replace with a single .mockResolvedValue that returns an array of 16 identical responses; update the test where chClient.queryPromise is mocked so it uses the loop/helper instead of repeating .mockResolvedValueOnce.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/repositories/SchemaCheckRepository.ts`:
- Around line 1228-1272: The loop over checkSubgraphs is re-running
composition-level federated inspections and writes because
fedGraphInspectorChanges and storedFedGraphBreakingChanges are processed inside
the checkSubgraphs loop; refactor so federated-level work runs once per
composition: before or after the for (const [subgraphName, checkSubgraph] of
checkSubgraphs.entries()) loop, if fedGraphInspectorChanges.length > 0 call
trafficInspector.inspect(fedGraphInspectorChanges, { daysToConsider: limit,
federatedGraphId: composition.id, organizationId }) once, run
checkClientTrafficAgainstOverrides using storedFedGraphBreakingChanges and the
single federated inspector result, and call createOperationUsage once for that
federated result; inside the existing loop keep only subgraph-specific
inspection (using combinedInspectorChanges or checkSubgraph.subgraph to inspect
per-subgraph) and only call createOperationUsage for subgraph-level results so
you no longer duplicate federated operation-usage writes.
---
Nitpick comments:
In `@controlplane/test/check-subgraph-schema.test.ts`:
- Around line 2562-2706: Replace the 16 repeated .mockResolvedValueOnce calls on
chClient.queryPromise with a concise loop or helper: create a single mock
response object (e.g. const mockOp = { operationHash: 'hash1', operationName:
'op1', operationType: 'query', firstSeen: Date.now() / 1000, lastSeen:
Date.now() / 1000 }) and then call (chClient.queryPromise as
Mock).mockResolvedValueOnce(mockOpArray) inside a for loop or use Array.from to
iterate 16 times, or replace with a single .mockResolvedValue that returns an
array of 16 identical responses; update the test where chClient.queryPromise is
mocked so it uses the loop/helper instead of repeating .mockResolvedValueOnce.
In `@studio/src/components/checks/composed-schema-changes-table.tsx`:
- Line 78: Remove the redundant key prop from the TableRow element: the parent
Row in the mapped list already supplies the React key (use the composite key
formed from changeType, message and federatedGraphName at the map level), so
delete the key attribute from TableRow (symbol: TableRow) and ensure the parent
mapping (the Row component used to render each item) uses the composite key for
stable identity.
- Around line 41-50: The map over changes currently uses key={i}; replace the
index with the same composite key logic used inside Row (combine changeType,
path and federatedGraphName or another stable unique identifier from each
change) so the map uses a stable natural key instead of i; update the
changes.map call to set key to that composite (e.g.
`${c.changeType}:${c.path}:${c.federatedGraphName}`) while leaving the Row props
(changeType, message, isBreaking, path, federatedGraphName) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controlplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/test/check-subgraph-schema.test.tsstudio/src/components/checks/composed-schema-changes-table.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx
…o-be-done-on' of github.com:wundergraph/cosmo into suvij/eng-8924-controlplane-breaking-changes-should-also-be-done-on
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
controlplane/test/check-subgraph-schema.test.ts (1)
108-108:⚠️ Potential issue | 🟠 MajorUse
try/finallyfor reliable server cleanup in new tests.At Line 108, Line 1636, Line 1712, Line 1791, Line 1884, Line 1957, Line 2081, and Line 2173, teardown is success-path only. Any earlier assertion failure can leave the server open.
🔧 Suggested pattern
test('...', async () => { const { client, server } = await SetupTest({ dbname, chClient }); - // test flow... - await server.close(); + try { + // test flow... + } finally { + await server.close(); + } });Also applies to: 1636-1636, 1712-1712, 1791-1791, 1884-1884, 1957-1957, 2081-2081, 2173-2173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/check-subgraph-schema.test.ts` at line 108, Wrap each test's server teardown in a try/finally so the server is always closed even if assertions fail: move the await server.close() into a finally block paired with the test body (the scope where the test creates/uses the server variable), e.g., replace the current success-path-only await server.close() with a try { /* existing test logic */ } finally { await server.close(); } for every occurrence of server.close() in controlplane/test/check-subgraph-schema.test.ts (lines where server is created/used and closed).controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts (1)
158-158:⚠️ Potential issue | 🟠 MajorGuarantee server teardown with
try/finallyin each test.At Line 158, Line 284, Line 382, Line 484, Line 617, Line 744, and Line 849,
await server.close()is only reached on success paths. A failed assertion before those lines can leak open handles and make the suite flaky.🔧 Suggested pattern
test('...', async () => { const { client, server } = await SetupTest({ dbname, chClient, ... }); - // assertions and test flow... - await server.close(); + try { + // assertions and test flow... + } finally { + await server.close(); + } });Also applies to: 284-284, 382-382, 484-484, 617-617, 744-744, 849-849
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts` at line 158, Tests currently call await server.close() only on success paths which can leak the server on assertion failures; wrap the test body that starts/uses server in a try/finally and move await server.close() into the finally block so it always runs. Locate the usages of server and the trailing await server.close() in proposal-federated-schema-breaking-changes.test.ts (around the references to server at the places noted) and change each test to start/initialize the server, run the assertions inside the try, and perform await server.close() inside finally to guarantee teardown even on failures.
🧹 Nitpick comments (1)
controlplane/test/check-subgraph-schema.test.ts (1)
58-58: Align test title with what is actually validated.The parameterized title says the role can create/publish/check, but role impersonation is only applied before check. Consider renaming the test to reflect check-only authorization, or move impersonation before create/publish if that behavior is intended to be validated.
Also applies to: 81-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/check-subgraph-schema.test.ts` at line 58, The test title string "%s should be able to create a subgraph, publish the schema and then check with new schema" is misleading because role impersonation is only applied before the check step; either update the title to reflect that only the "check" is role-validated (e.g., "%s should be able to check with new schema") or move the impersonation/authorization setup for the parameterized role (the code that impersonates `role`) to occur before the create/publish calls so the test actually validates create/publish/check under that role; adjust the occurrences of this title (the parameterized test at the shown location and the similar cases at lines 81–84) and ensure the impersonation helper is applied around create/publish if you choose the latter approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@controlplane/test/check-subgraph-schema.test.ts`:
- Line 108: Wrap each test's server teardown in a try/finally so the server is
always closed even if assertions fail: move the await server.close() into a
finally block paired with the test body (the scope where the test creates/uses
the server variable), e.g., replace the current success-path-only await
server.close() with a try { /* existing test logic */ } finally { await
server.close(); } for every occurrence of server.close() in
controlplane/test/check-subgraph-schema.test.ts (lines where server is
created/used and closed).
In
`@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts`:
- Line 158: Tests currently call await server.close() only on success paths
which can leak the server on assertion failures; wrap the test body that
starts/uses server in a try/finally and move await server.close() into the
finally block so it always runs. Locate the usages of server and the trailing
await server.close() in proposal-federated-schema-breaking-changes.test.ts
(around the references to server at the places noted) and change each test to
start/initialize the server, run the assertions inside the try, and perform
await server.close() inside finally to guarantee teardown even on failures.
---
Nitpick comments:
In `@controlplane/test/check-subgraph-schema.test.ts`:
- Line 58: The test title string "%s should be able to create a subgraph,
publish the schema and then check with new schema" is misleading because role
impersonation is only applied before the check step; either update the title to
reflect that only the "check" is role-validated (e.g., "%s should be able to
check with new schema") or move the impersonation/authorization setup for the
parameterized role (the code that impersonates `role`) to occur before the
create/publish calls so the test actually validates create/publish/check under
that role; adjust the occurrences of this title (the parameterized test at the
shown location and the similar cases at lines 81–84) and ensure the
impersonation helper is applied around create/publish if you choose the latter
approach.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/test/check-subgraph-schema.test.tscontrolplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts
… changes in SchemaCheckRepository
… modifications in breaking change logic
…24-controlplane-breaking-changes-should-also-be-done-on
…y in SchemaCheckRepository and SubgraphRepository
… composed schema breaking changes
…toggleChangeOverridesForAllOperations
JivusAyrus
deleted the
suvij/eng-8924-controlplane-breaking-changes-should-also-be-done-on
branch
5 tasks
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
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
Release Notes
Checklist