feat: add a feature to link subgraphs across namespaces#2156
feat: add a feature to link subgraphs across namespaces#2156JivusAyrus merged 69 commits intomainfrom
Conversation
JivusAyrus
requested review from
Aenimus,
StarpTech,
endigma,
jensneuse and
wilsonrivera
as code owners
WalkthroughAdds subgraph link/unlink capabilities and linked-subgraph-aware schema checks across CLI, control plane services, repositories, DB schema/migrations, protobuf/connect bindings, Studio UI, utilities, and tests; introduces linked-check metadata and two flags that can short-circuit check success. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (16)
controlplane/src/db/schema.ts (3)
247-249: Nit: tighten comment grammar and claritySmall grammar tweaks improve readability.
Apply:
-// The link is a one way link from source to target. -// The source subgraph can be linked only to one target subgraph, thats why we have a unique constraint on the source subgraph. +// The link is a one-way link from source to target. +// A source subgraph can be linked to only one target subgraph; therefore we enforce a unique constraint on the source subgraph.
252-268: Consider a DB-level check to prevent self-linking (source == target)Unless enforced elsewhere, a self-link is likely invalid and could cause confusing behavior. A simple CHECK constraint would prevent it at the database level.
If you prefer schema enforcement, add a check via a migration, for example:
ALTER TABLE linked_subgraphs ADD CONSTRAINT ls_no_self_link CHECK (source_subgraph_id <> target_subgraph_id);If this is validated strictly in application code already, feel free to skip.
269-276: Optional: add relations for linkedSubgraphs to improve query ergonomicsFollowing the existing pattern, define relations to subgraphs and users to enable typed joins and eager-loading via Drizzle.
Example addition elsewhere in this file:
export const linkedSubgraphsRelations = relations(linkedSubgraphs, ({ one }) => ({ source: one(subgraphs, { fields: [linkedSubgraphs.sourceSubgraphId], references: [subgraphs.id] }), target: one(subgraphs, { fields: [linkedSubgraphs.targetSubgraphId], references: [subgraphs.id] }), createdBy: one(users, { fields: [linkedSubgraphs.createdById], references: [users.id] }), }));cli/src/commands/subgraph/commands/unlink.ts (1)
14-38: Handle network/client exceptions to avoid uncaught crashesThe command assumes unlinkSubgraph resolves successfully. If the request throws (network error, auth failure before a response, etc.), the process will crash without a graceful message. Wrap in try/catch for better UX and consistent exit codes.
Apply:
- command.action(async (name, options) => { - const spinner = ora(`The subgraph "${name}" is being unlinked...`).start(); - - const resp = await opts.client.platform.unlinkSubgraph( - { - sourceSubgraphName: name, - sourceSubgraphNamespace: options.namespace, - }, - { - headers: getBaseHeaders(), - }, - ); - - if (resp.response?.code === EnumStatusCode.OK) { - spinner.succeed('Subgraph was unlinked successfully.'); - } else { - spinner.fail('Failed to unlink subgraph.'); - if (resp.response?.details) { - console.log(pc.red(pc.bold(resp.response?.details))); - } - process.exitCode = 1; - // eslint-disable-next-line no-useless-return - return; - } - }); + command.action(async (name, options) => { + const spinner = ora(`The subgraph "${name}" is being unlinked...`).start(); + try { + const resp = await opts.client.platform.unlinkSubgraph( + { + sourceSubgraphName: name, + sourceSubgraphNamespace: options.namespace, + }, + { headers: getBaseHeaders() }, + ); + if (resp.response?.code === EnumStatusCode.OK) { + spinner.succeed('Subgraph was unlinked successfully.'); + } else { + spinner.fail('Failed to unlink subgraph.'); + if (resp.response?.details) { + console.log(pc.red(pc.bold(resp.response.details))); + } + process.exitCode = 1; + return; + } + } catch (err: any) { + spinner.fail('Failed to unlink subgraph.'); + console.log(pc.red(pc.bold(err?.message ?? String(err)))); + process.exitCode = 1; + } + });controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (2)
7-7: Drop unused importNamespaceRepository.It’s not used in this handler.
-import { DefaultNamespace, NamespaceRepository } from '../../repositories/NamespaceRepository.js'; +import { DefaultNamespace } from '../../repositories/NamespaceRepository.js';
43-51: Consider using ERR_NOT_FOUND when no link exists.Returning a generic
ERRwhen the source subgraph has no link is less precise thanERR_NOT_FOUND. Optional, but improves API semantics.- code: EnumStatusCode.ERR, + code: EnumStatusCode.ERR_NOT_FOUND,proto/wg/cosmo/platform/v1/platform.proto (2)
2859-2878: Message definitions look good; consider field naming consistency.The new messages are minimal and match usage. As a nit, the file mixes snake_case and camelCase field names; if you’re standardizing, consider aligning these to the preferred style.
3246-3249: RPCs added correctly; consider grouping near related subgraph RPCs.For discoverability, placing Link/Unlink close to other subgraph RPCs would help future maintenance. Functionally fine as-is.
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1723-1724: Minor: simplify where clause.
and(eq(...))is redundant with a single predicate.- await this.db.delete(linkedSubgraphs).where(and(eq(linkedSubgraphs.sourceSubgraphId, sourceSubgraphId))); + await this.db.delete(linkedSubgraphs).where(eq(linkedSubgraphs.sourceSubgraphId, sourceSubgraphId));controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (4)
38-46: Fix typo in user-facing error message (“source namespace”).“Sourcenamespace” is a typo and will surface to users. Recommend correcting to “source namespace”.
Proposed change:
- details: `The sourcenamespace "${req.sourceSubgraphNamespace}" was not found.`, + details: `The source namespace "${req.sourceSubgraphNamespace}" was not found.`,If you apply this change, update the corresponding test assertion in controlplane/test/subgraph/link-subgraph.test.ts Line 115 accordingly.
124-129: Handle concurrent link attempts gracefully (unique violation).There is a race between the “already linked” check and the insert. A concurrent request can still violate the unique constraint and bubble up a DB error. Catch the unique violation (Postgres code 23505) and return the same friendly error used above.
Sketch:
try { // link + audit in transaction (see previous comment) } catch (e: any) { if (e?.code === '23505') { const existing = await subgraphRepo.getLinkedSubgraph({ sourceSubgraphId: sourceSubgraph.id }); return { response: { code: EnumStatusCode.ERR, details: `The source subgraph "${req.sourceSubgraphName}" is already linked to the target subgraph "${existing?.[0]?.targetSubgraphName ?? ''}" in the namespace "${existing?.[0]?.targetSubgraphNamespace ?? ''}". Unlink the existing link first.`, }, }; } throw e; }
130-143: Enrich audit log with target subgraph details.You already set namespace context for the source (target of the action). Consider also setting
targetId,targetType, andtargetDisplayNameto capture the linked target subgraph for better traceability.Example:
await auditLogRepo.addAuditLog({ organizationId: authContext.organizationId, organizationSlug: authContext.organizationSlug, auditAction: 'subgraph.linked', action: 'linked', actorId: authContext.userId, auditableType: 'subgraph', auditableDisplayName: sourceSubgraph.name, + targetType: 'subgraph', + targetId: targetSubgraph.id, + targetDisplayName: targetSubgraph.name, actorDisplayName: authContext.userDisplayName, apiKeyName: authContext.apiKeyName, actorType: authContext.auth === 'api_key' ? 'api_key' : 'user', targetNamespaceId: sourceSubgraph.namespaceId, targetNamespaceDisplayName: sourceSubgraph.namespace, });
115-122: Redundant self-link check (unreachable with current rules).Given the same-namespace prohibition (Line 29),
targetSubgraph.id === sourceSubgraph.idcannot occur. Either remove this branch or move the same-namespace check later if you specifically want to emit this message.cli/src/commands/subgraph/commands/link.ts (1)
16-41: Harden error handling around the RPC call.If the RPC throws (network, auth, etc.), the spinner will hang and the process will exit with an unhandled rejection. Wrap in try/catch and render a clean failure.
command.action(async (name, options) => { - const spinner = ora(`The subgraph "${name}" is being linked to "${options.targetSubgraphName}"...`).start(); - - const resp = await opts.client.platform.linkSubgraph( - { - sourceSubgraphName: name, - sourceSubgraphNamespace: options.namespace, - targetSubgraphName: options.targetSubgraphName, - targetSubgraphNamespace: options.targetNamespace, - }, - { - headers: getBaseHeaders(), - }, - ); - - if (resp.response?.code === EnumStatusCode.OK) { - spinner.succeed('Subgraph was linked successfully.'); - } else { - spinner.fail('Failed to link subgraph.'); - if (resp.response?.details) { - console.log(pc.red(pc.bold(resp.response?.details))); - } - process.exitCode = 1; - // eslint-disable-next-line no-useless-return - return; - } + const spinner = ora(`The subgraph "${name}" is being linked to "${options.targetSubgraphName}"...`).start(); + try { + const resp = await opts.client.platform.linkSubgraph( + { + sourceSubgraphName: name, + sourceSubgraphNamespace: options.namespace, + targetSubgraphName: options.targetSubgraphName, + targetSubgraphNamespace: options.targetNamespace, + }, + { headers: getBaseHeaders() }, + ); + if (resp.response?.code === EnumStatusCode.OK) { + spinner.succeed('Subgraph was linked successfully.'); + } else { + spinner.fail('Failed to link subgraph.'); + if (resp.response?.details) { + console.log(pc.red(pc.bold(resp.response?.details))); + } + process.exitCode = 1; + return; + } + } catch (err: any) { + spinner.fail('Failed to link subgraph.'); + console.log(pc.red(pc.bold(err?.message ?? 'Unknown error'))); + process.exitCode = 1; + return; + } });controlplane/test/subgraph/link-subgraph.test.ts (1)
112-116: Align assertion with corrected error message (“source namespace”).If you fix the typo in the handler response, adjust this assertion accordingly.
- expect(linkResponse.response?.details).toBe('The sourcenamespace "nonexistent" was not found.'); + expect(linkResponse.response?.details).toBe('The source namespace "nonexistent" was not found.');controlplane/migrations/0129_redundant_silver_sable.sql (1)
1-9: Add a check constraint to prevent self-links at the DB layer.Defense-in-depth: ensure a link cannot point a source to itself, even if application logic regresses.
CREATE TABLE IF NOT EXISTS "linked_subgraphs" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "source_subgraph_id" uuid NOT NULL, "target_subgraph_id" uuid NOT NULL, "created_at" timestamp with time zone DEFAULT now() NOT NULL, "created_by_id" uuid, + CONSTRAINT "linked_subgraphs_no_self_link" CHECK ("source_subgraph_id" <> "target_subgraph_id"), CONSTRAINT "unique_source_subgraph" UNIQUE("source_subgraph_id") );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
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/**
📒 Files selected for processing (14)
cli/src/commands/subgraph/commands/link.ts(1 hunks)cli/src/commands/subgraph/commands/unlink.ts(1 hunks)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_connect.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(1 hunks)controlplane/migrations/0129_redundant_silver_sable.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/linkSubgraph.ts(1 hunks)controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts(1 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(2 hunks)controlplane/src/db/models.ts(2 hunks)controlplane/src/db/schema.ts(1 hunks)controlplane/test/subgraph/link-subgraph.test.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (15)
LinkSubgraphRequest(27381-27390)LinkSubgraphRequest(27405-27405)LinkSubgraphRequest(27420-27422)LinkSubgraphResponse(27452-27458)LinkSubgraphResponse(27473-27473)LinkSubgraphResponse(27488-27490)Response(712-720)Response(735-735)Response(750-752)UnlinkSubgraphRequest(27499-27506)UnlinkSubgraphRequest(27521-27521)UnlinkSubgraphRequest(27536-27538)UnlinkSubgraphResponse(27554-27560)UnlinkSubgraphResponse(27575-27575)UnlinkSubgraphResponse(27590-27592)
cli/src/commands/subgraph/commands/link.ts (3)
cli/src/commands/subgraph/commands/unlink.ts (1)
opts(8-41)cli/src/core/types/types.ts (1)
BaseCommandOptions(8-10)cli/src/core/config.ts (1)
getBaseHeaders(40-46)
controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (7)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
linkSubgraph(2624-2633)controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1710-1720)SubgraphRepository(56-1742)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
LinkSubgraphRequest(22466-22516)LinkSubgraphResponse(22521-22553)controlplane/src/core/util.ts (3)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)controlplane/src/core/repositories/NamespaceRepository.ts (2)
DefaultNamespace(7-7)NamespaceRepository(9-184)controlplane/src/core/repositories/AuditLogRepository.ts (1)
AuditLogRepository(28-137)
controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (6)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
unlinkSubgraph(1722-1724)SubgraphRepository(56-1742)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
UnlinkSubgraphRequest(22558-22596)UnlinkSubgraphResponse(22601-22633)controlplane/src/core/util.ts (3)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)controlplane/src/core/repositories/NamespaceRepository.ts (1)
DefaultNamespace(7-7)controlplane/src/core/repositories/AuditLogRepository.ts (1)
AuditLogRepository(28-137)
controlplane/test/subgraph/link-subgraph.test.ts (5)
controlplane/src/core/test-util.ts (6)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)createTestRBACEvaluator(173-175)createTestGroup(181-198)controlplane/test/test-util.ts (5)
DEFAULT_NAMESPACE(52-52)DEFAULT_SUBGRAPH_URL_ONE(49-49)DEFAULT_SUBGRAPH_URL_TWO(50-50)createSubgraph(704-716)createBaseAndFeatureSubgraph(718-737)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
createNamespace(74-83)controlplane/src/db/schema.ts (1)
users(1097-1103)controlplane/src/db/models.ts (1)
OrganizationRole(32-32)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
controlplane/src/db/schema.ts (3)
linkedSubgraphs(249-277)targets(549-588)subgraphs(218-245)
cli/src/commands/subgraph/commands/unlink.ts (3)
cli/src/commands/subgraph/commands/link.ts (1)
opts(8-45)cli/src/core/types/types.ts (1)
BaseCommandOptions(8-10)cli/src/core/config.ts (1)
getBaseHeaders(40-46)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (4)
controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (1)
linkSubgraph(12-151)controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1710-1720)unlinkSubgraph(1722-1724)connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
LinkSubgraphRequest(22466-22516)LinkSubgraphResponse(22521-22553)UnlinkSubgraphRequest(22558-22596)UnlinkSubgraphResponse(22601-22633)controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (1)
unlinkSubgraph(12-78)
connect/src/wg/cosmo/platform/v1/platform_connect.ts (1)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
LinkSubgraphRequest(22466-22516)LinkSubgraphResponse(22521-22553)UnlinkSubgraphRequest(22558-22596)UnlinkSubgraphResponse(22601-22633)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
controlplane/migrations/meta/_journal.json (1)
909-913: Migration journal entry looks consistent and well-formedIdx, version, tag, and breakpoints align with the project’s conventions. No structural issues spotted.
controlplane/src/db/models.ts (2)
94-97: Additions to AuditLogAction are consistent with feature semanticsNew literals 'linked' and 'unlinked' make sense for generic actions.
122-123: AuditLogFullAction entries align with action taxonomy'subgraph.linked' and 'subgraph.unlinked' fit existing naming and scope. No issues.
cli/src/commands/subgraph/commands/unlink.ts (1)
9-13: CLI UX matches link.ts usage patternArgument/option shape and default namespace behavior mirror the link command. Consistency appreciated.
connect/src/wg/cosmo/platform/v1/platform_connect.ts (2)
8-8: Imports updated to include new Link/Unlink messagesGenerated imports correctly add LinkSubgraphRequest/Response and UnlinkSubgraphRequest/Response. No issues.
1800-1821: New RPCs wired correctly in PlatformService (generated)Method names, I/O types, and MethodKind.Unary are consistent. Given this is generated code, nothing to change here.
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1726-1741: LGTM: getLinkedSubgraph scopes by organization and returns the right shape.Query is properly org-scoped and joins are correct. This aligns with the handler’s expectations.
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
9-9: Generated imports updated correctly.The new Link/Unlink request/response types are included; consistent with generated code patterns.
2619-2650: LGTM: Link/Unlink method descriptors added correctly.Unary methods, correct service typeName, and I/O types. No idempotency markers (appropriate for side-effecting calls).
connect/src/wg/cosmo/platform/v1/platform_pb.ts (5)
22463-22516: LGTM: LinkSubgraphRequest schema is consistent and correct
- Field names and numbers match the Go pb (1..4) exactly.
- Typing, constructor, and serialization helpers align with @bufbuild/protobuf patterns.
22518-22553: LGTM: LinkSubgraphResponse correctly wraps Response
- Optional Response field and field list mapping look correct.
- Type reference to Response is consistent with the existing message set in this module.
22555-22596: LGTM: UnlinkSubgraphRequest schema matches server definition
- Fields and tags match the Go schema (two string fields, tags 1 and 2).
- Generated helpers are standard and correct.
22598-22633: LGTM: UnlinkSubgraphResponse correctly wraps Response
- Structure mirrors LinkSubgraphResponse and the Go pb definition.
- No issues spotted in field metadata or helpers.
22463-22634: Generation Verified: Client Stubs & RPCs PresentAll checks passed—LinkSubgraph/UnlinkSubgraph and the Response message are exposed correctly in both TS and Go:
- Response class found in
connect/src/wg/cosmo/platform/v1/platform_pb.ts(line 429).- TS connect stubs expose
linkSubgraph/unlinkSubgraphRPCs in
platform_connect.ts(lines 1801–1810, 1812–1820)platform-PlatformService_connectquery.ts(lines 2620–2628, 2636–2644)- Go structs for
LinkSubgraphRequest|ResponseandUnlinkSubgraphRequest|Responseexist in
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go(around lines 27381–27554)No further action required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/migrations/0129_wild_ego.sql (2)
1-8: Enforce true one-to-one and prevent self-linking (add UNIQUE on target and CHECK).If links are truly “one-to-one,” you should also enforce uniqueness on
target_subgraph_id. Additionally, prevent self-links (source == target) at the DB level.Apply this diff to update the table definition:
CREATE TABLE IF NOT EXISTS "linked_subgraphs" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "source_subgraph_id" uuid NOT NULL, "target_subgraph_id" uuid NOT NULL, "created_at" timestamp with time zone DEFAULT now() NOT NULL, "created_by_id" uuid, - CONSTRAINT "linked_subgraphs_source_subgraph_id_unique" UNIQUE("source_subgraph_id") + CONSTRAINT "linked_subgraphs_source_subgraph_id_unique" UNIQUE("source_subgraph_id"), + CONSTRAINT "linked_subgraphs_target_subgraph_id_unique" UNIQUE("target_subgraph_id"), + CONSTRAINT "linked_subgraphs_source_target_distinct" CHECK ("source_subgraph_id" <> "target_subgraph_id") );Follow-up:
- If the intent is “one-to-many” (multiple sources may point to the same target), keep as-is and skip the target UNIQUE. Please confirm the intended cardinality.
- If links are undirected (A–B equivalent to B–A), consider a unique expression index to disallow both directions:
UNIQUE (LEAST(source_subgraph_id, target_subgraph_id), GREATEST(source_subgraph_id, target_subgraph_id)).
28-28: Redundant index on source_subgraph_id (already covered by UNIQUE constraint).The UNIQUE constraint on
source_subgraph_idcreates an index implicitly. The additional btree index duplicates it and wastes space.Apply this diff to remove the redundant index:
-CREATE INDEX IF NOT EXISTS "ls_source_subgraph_id_idx" ON "linked_subgraphs" USING btree ("source_subgraph_id");--> statement-breakpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
controlplane/migrations/0129_wild_ego.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/db/schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controlplane/migrations/meta/_journal.json
- controlplane/src/db/schema.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build-router
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (3)
controlplane/migrations/0129_wild_ego.sql (3)
10-20: FK constraints look solid and idempotent.Good use of idempotent DO blocks and appropriate cascading behaviors.
22-26: User FK with SET NULL is appropriate.This aligns with auditability while avoiding delete cascades from users.
29-30: Indexes on FKs LGTM.Indexing
target_subgraph_idandcreated_by_idwill help the common lookups.
…ing unused members and redundant checks
… database operations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/PlatformService.ts (2)
169-170: Group subgraph imports together for consistencyThese two imports belong with the existing subgraph imports block (Lines 131–144). Co-locating keeps domain modules grouped and the file easier to navigate.
Apply this diff here to remove the imports from this location:
- import { linkSubgraph } from './subgraph/linkSubgraph.js'; - import { unlinkSubgraph } from './subgraph/unlinkSubgraph.js';Then add them next to the other subgraph imports (right after Line 144) as:
import { linkSubgraph } from './subgraph/linkSubgraph.js'; import { unlinkSubgraph } from './subgraph/unlinkSubgraph.js';
845-851: Place link/unlink under Mutations instead of QueriesThese handlers mutate state and should live in the Mutations section (above Line 458) alongside the other subgraph mutations like publish/update/move. This keeps the Mutations/Queries separation intact.
Apply this diff to remove from the Queries section:
- linkSubgraph: (req, ctx) => { - return linkSubgraph(opts, req, ctx); - }, - - unlinkSubgraph: (req, ctx) => { - return unlinkSubgraph(opts, req, ctx); - },Then add them to the Mutations section (e.g., after updateSubgraph at Lines 322–324):
linkSubgraph: (req, ctx) => { return linkSubgraph(opts, req, ctx); }, unlinkSubgraph: (req, ctx) => { return unlinkSubgraph(opts, req, ctx); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
controlplane/src/core/bufservices/PlatformService.ts(2 hunks)controlplane/test/subgraph/link-subgraph.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/test/subgraph/link-subgraph.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
controlplane/src/core/bufservices/PlatformService.ts (3)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
linkSubgraph(2624-2633)unlinkSubgraph(2640-2649)controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1710-1720)unlinkSubgraph(1722-1724)controlplane/src/core/routes.ts (1)
opts(62-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
…raph linking functionality - Introduced a new column `is_subgraph_linked` in the `schema_checks` table to track linked subgraphs. - Updated the `checkSubgraphSchema`, `linkSubgraph`, and `unlinkSubgraph` functions to utilize the new column. - Enhanced the `getLinkedSubgraph` method to return a single linked subgraph or undefined. - Improved error handling and response messages for linking operations.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
controlplane/src/core/repositories/FederatedGraphRepository.ts (2)
785-787: Fix misleading docstring: this method returns composition membership, not cross-namespace links.The implementation queries federated_subgraphs (composition membership). Adjust the comment to avoid confusion with the new linked_subgraphs feature.
- /** - * bySubgraphId returns the federated graphs that the given subgraph is linked to. - */ + /** + * bySubgraphId returns federated graphs that currently include the given subgraph in their composition. + */
789-805: Avoid N+1 queries; fetch IDs then resolve in parallel or join.Current loop calls byId sequentially and each byId invokes additional queries, which can be slow for many graphs.
Minimal change: resolve in parallel.
- const federatedGraphs: FederatedGraphDTO[] = []; - - for (const fedGraph of res) { - const fg = await this.byId(fedGraph.federatedGraphId); - if (fg === undefined) { - continue; - } - - federatedGraphs.push(fg); - } - - return federatedGraphs; + const results = await Promise.all(res.map((r) => this.byId(r.federatedGraphId))); + return results.filter((fg): fg is FederatedGraphDTO => fg !== undefined);Further improvement (optional): perform a single SELECT with a join on federated_graphs and then map to DTOs to avoid calling byId entirely. I can draft that if you prefer.
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
85-86: Prefer nullish coalescing for booleans.Using || is fine here because the type is boolean | undefined, but ?? communicates intent better and avoids surprises if the type widens later.
- isSubgraphLinked: data.isSubgraphLinked || false, + isSubgraphLinked: data.isSubgraphLinked ?? false,controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (2)
23-23: Consider consistent defaults/validation for targetSubgraphNamespace.You default sourceSubgraphNamespace to “default” but not targetSubgraphNamespace. If the proto doesn’t require target, add a similar default or an explicit presence check to avoid “undefined” in error messages.
117-121: Handle unique-constraint races gracefully to avoid 500 on concurrent requests.Between the “already linked” read and insert, a concurrent link can win and trigger a unique violation on sourceSubgraphId. Catch and map to a typed/public error with a deterministic message.
- await txSubgraphRepo.linkSubgraph({ - sourceSubgraphId: sourceSubgraph.id, - targetSubgraphId: targetSubgraph.id, - createdById: authContext.userId, - }); + try { + await txSubgraphRepo.linkSubgraph({ + sourceSubgraphId: sourceSubgraph.id, + targetSubgraphId: targetSubgraph.id, + createdById: authContext.userId, + }); + } catch (e: any) { + // Postgres unique_violation + if (e?.code === '23505') { + return { + response: { + code: EnumStatusCode.ERR, + details: `The source subgraph "${req.sourceSubgraphName}" is already linked.`, + }, + }; + } + throw e; + }If your project exposes a PublicError helper, you can throw that instead and let handleError map it.
controlplane/src/db/schema.ts (2)
247-276: Name the unique constraint instead of using a column-level .unique().Column-level .unique() creates an anonymous unique index. A named unique constraint improves clarity and migration diffs. Consider removing the column-level unique and adding a table-level named unique instead.
export const linkedSubgraphs = pgTable( 'linked_subgraphs', // ls { id: uuid('id').primaryKey().defaultRandom(), sourceSubgraphId: uuid('source_subgraph_id') .notNull() .references(() => subgraphs.id, { onDelete: 'cascade', - }) - .unique(), + }), targetSubgraphId: uuid('target_subgraph_id') .notNull() .references(() => subgraphs.id, { onDelete: 'cascade', }), createdAt: timestamp('created_at', { withTimezone: true }).notNull().defaultNow(), createdById: uuid('created_by_id').references(() => users.id, { onDelete: 'set null', }), }, (t) => { return { + uniqueSourceSubgraph: unique('unique_source_subgraph').on(t.sourceSubgraphId), sourceSubgraphIdIndex: index('ls_source_subgraph_id_idx').on(t.sourceSubgraphId), targetSubgraphIdIndex: index('ls_target_subgraph_id_idx').on(t.targetSubgraphId), createdByIdIndex: index('ls_created_by_id_idx').on(t.createdById), }; }, );Note: Align the corresponding SQL migration to avoid duplicate uniques.
247-276: Tenant-safety: consider denormalizing organizationId onto linked_subgraphs.Application code enforces org scoping via repositories, but a denormalized organizationId with a FK (or a trigger-backed CHECK) would add DB-level safety against accidental cross-org links and simplify scoping joins.
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (2)
135-151: Linked subgraph resolution looks good; consider cycle/consistency guard and logging contextFetching the linked target once and shaping the DTO is fine. Two minor improvements:
- Add a defensive guard against accidental self-links or cycles (repo likely enforces this, but a runtime assert produces clearer errors).
- Add debug logging to surface which link was resolved (useful when checks span namespaces).
Would you like me to add a tiny assert and a debug log here? Also confirm
SubgraphRepository.getLinkedSubgraphalready prevents cycles.
481-509: Non-linked path OK; consider DRY-ing traffic inspection into a helperLogic mirrors pre-existing behavior and now executes only when not linked. You can reduce duplication with a small helper used by both paths.
Example helper (outside this range), reusing existing variables:
async function inspectAndRecordTrafficForGraph( federatedGraphId: string, subgraphId: string, ) { const result = await trafficInspector.inspect(inspectorChanges, { daysToConsider: limit, federatedGraphId, organizationId: authContext.organizationId, subgraphId, }); if (result.size === 0) return; const overrideCheck = await schemaCheckRepo.checkClientTrafficAgainstOverrides({ changes: storedBreakingChanges, inspectorResultsByChangeId: result, namespaceId: namespace.id, }); hasClientTraffic = overrideCheck.hasUnsafeClientTraffic; await schemaCheckRepo.createOperationUsage(overrideCheck.result, federatedGraphId); for (const resultElement of overrideCheck.result.values()) { inspectedOperations.push(...resultElement); } }Then call it in both branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
controlplane/migrations/0130_daffy_tusk.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(3 hunks)controlplane/src/core/bufservices/subgraph/linkSubgraph.ts(1 hunks)controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts(1 hunks)controlplane/src/core/repositories/FederatedGraphRepository.ts(1 hunks)controlplane/src/core/repositories/SchemaCheckRepository.ts(2 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(2 hunks)controlplane/src/db/schema.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controlplane/src/core/repositories/SubgraphRepository.ts
- controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts
🧰 Additional context used
🧬 Code graph analysis (3)
controlplane/src/core/repositories/FederatedGraphRepository.ts (2)
controlplane/src/types/index.ts (1)
FederatedGraphDTO(69-92)controlplane/src/db/schema.ts (1)
federatedGraphs(23-51)
controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (5)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1710-1720)SubgraphRepository(56-1748)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (3)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)controlplane/src/core/repositories/NamespaceRepository.ts (2)
DefaultNamespace(7-7)NamespaceRepository(9-184)controlplane/src/core/repositories/AuditLogRepository.ts (1)
AuditLogRepository(28-137)
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (1)
controlplane/src/db/schema.ts (1)
federatedGraphs(23-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
🔇 Additional comments (7)
controlplane/migrations/meta/_journal.json (1)
909-921: Verified: migrations 0129_wild_ego and 0130_daffy_tusk exist and contain the expected DDLBoth SQL files are present and contain the expected changes:
- controlplane/migrations/0129_wild_ego.sql — CREATE TABLE "linked_subgraphs" (id, source_subgraph_id, target_subgraph_id, created_at, created_by_id); UNIQUE("source_subgraph_id"); FK constraints to public.subgraphs (source/target) and public.users (created_by_id); indexes ls_source_subgraph_id_idx, ls_target_subgraph_id_idx, ls_created_by_id_idx.
- controlplane/migrations/0130_daffy_tusk.sql — ALTER TABLE "schema_checks" ADD COLUMN "is_subgraph_linked" boolean DEFAULT false; (no additional indexes added).
- controlplane/migrations/meta/_journal.json — journal entries for idx 129 ("0129_wild_ego") and idx 130 ("0130_daffy_tusk") are present.
- controlplane/src/db/schema.ts — schema includes isSubgraphLinked boolean mapping.
No action required from the author — resolving this review comment.
controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (3)
28-36: Confirm product requirement: disallow same-namespace linking.The PR headline emphasizes “across namespaces.” If same-namespace linking should be supported (or at least return a specific error code), confirm this behavior.
If intended to disallow, consider ERR_INVALID_ARGUMENT for clarity.
109-111: RBAC check on target subgraph read access looks correct.
113-137: Good: link + audit log wrapped in a single transaction.Prevents partial success on failures during audit logging.
controlplane/src/db/schema.ts (1)
821-821: No index needed foris_subgraph_linkedVerified that there are no queries filtering or sorting on the
is_subgraph_linkedcolumn in the current codebase. Since it’s only used for storing informational flags and never used in a WHERE or ORDER BY clause, adding a btree index isn’t necessary at this time.controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (2)
274-275: Persisting isSubgraphLinked on schema checks is correctPlumbing the flag into the initial schema-check row aligns with the migration and downstream reporting.
511-541: Verify operation-usage writes don't assume a schema_check_federated_graphs rowShort summary: createOperationUsage writes into schema_check_change_operation_usage and only FK-checks federated_graphs.id (not schema_check_federated_graphs). So you won't hit a DB FK error, but you can end up with operation-usage rows for a federated graph that were never registered for this schema check (i.e. no schema_check_federated_graphs row) — which can produce incoherent check records/aggregation.
Files to inspect / attention points:
- controlplane/src/core/repositories/SchemaCheckRepository.ts
- createOperationUsage implementation (inserts into schema_check_change_operation_usage; uses federatedGraphId). (~line ~152)
- createCheckedFederatedGraph (creates schema_check_federated_graphs rows). (~line ~294)
- controlplane/src/db/schema.ts
- schemaCheckChangeActionOperationUsage table: federatedGraphId references federated_graphs.id (no reference to schema_check_federated_graphs).
- controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
- Calls to createOperationUsage in both flows: non-linked (uses composedGraph.id) and linkedSubgraph branch (uses federatedGraph.id). (~lines ~500–540)
Recommended actions (pick one):
- Safer/faster: ensure a schema_check_federated_graphs row exists for the federatedGraph.id used in the linkedSubgraph branch before calling createOperationUsage (call schemaCheckRepo.createCheckedFederatedGraph(schemaCheckID, federatedGraph.id, limit) if missing).
- Or: change the operation-usage model/API to explicitly attach usage to the schema_check_federated_graphs id (add schemaCheckFederatedGraphId to the operation usage table and update createOperationUsage), so usages are unambiguously scoped to a specific check.
- Or: if you intentionally only need federated_graphs-level linkage, leave as-is but add a comment and unit/integration test to assert expected aggregation behavior.
Decision: I could not find a DB FK that would fail the insert — but I could not conclusively prove the linkedSubgraph flow always creates the matching schema_check_federated_graphs rows. Please verify which semantics you want (operation usage tied to federated_graphs vs tied to schema_check_federated_graphs) and apply one of the fixes above.
…nality - Introduced a new table `linked_schema_checks` to manage relationships between schema checks. - Implemented foreign key constraints to ensure referential integrity with the `schema_checks` table. - Created indexes for efficient querying on `schema_check_id` and `linked_schema_check_id`. - Updated migration files to reflect these changes and ensure smooth database transitions.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1734-1744: Make link insertion idempotent; validate inputs (self-link, org-scope).Two concurrent link requests can race and hit the unique constraint on source_subgraph_id, returning a 500. Also, prevent self-links and ensure both subgraphs exist in the same org before inserting.
public async linkSubgraph({ sourceSubgraphId, targetSubgraphId, createdById, }: { sourceSubgraphId: string; targetSubgraphId: string; createdById: string; }) { - await this.db.insert(linkedSubgraphs).values({ sourceSubgraphId, targetSubgraphId, createdById }); + if (sourceSubgraphId === targetSubgraphId) { + throw new Error('Source and target cannot be the same subgraph'); + } + // Optional but recommended: verify both subgraphs belong to this.organizationId + // and exist. If you want, I can wire this up. + await this.db + .insert(linkedSubgraphs) + .values({ sourceSubgraphId, targetSubgraphId, createdById }) + .onConflictDoNothing(); // idempotent under unique(source_subgraph_id) }If you want the handler to tailor UX, return a boolean indicating whether a new row was inserted.
🧹 Nitpick comments (7)
controlplane/migrations/0131_crazy_diamondback.sql (1)
8-18: Idempotency for constraints: consider guarding uniqueness as well.You wrapped FK creation in a DO/EXCEPTION block for idempotency. If this migration needs to add the unique constraint to an existing table on upgrades, you’ll also need an ALTER TABLE ADD CONSTRAINT guarded similarly. Right now uniqueness is only created inside CREATE TABLE IF NOT EXISTS. If the table exists without the unique, it will remain missing.
Would you like a follow-up ALTER TABLE block to add the unique when the table already exists?
controlplane/src/core/repositories/SubgraphRepository.ts (4)
1746-1748: Expose unlink outcome (rows affected) for better UX.Returning the number of rows deleted (or a boolean) helps callers distinguish “already unlinked” from “unlinked now.”
- public async unlinkSubgraph({ sourceSubgraphId }: { sourceSubgraphId: string }) { - await this.db.delete(linkedSubgraphs).where(and(eq(linkedSubgraphs.sourceSubgraphId, sourceSubgraphId))); - } + public async unlinkSubgraph({ sourceSubgraphId }: { sourceSubgraphId: string }): Promise<number> { + const result = await this.db + .delete(linkedSubgraphs) + .where(eq(linkedSubgraphs.sourceSubgraphId, sourceSubgraphId)); + // drizzle-postgres returns { rowCount } on .execute() — adjust depending on client + return (result as any)?.rowCount ?? 0; + }
1815-1817: Typo in variable name.Rename graphCompostionRepo to graphCompositionRepo for consistency.
- const graphCompostionRepo = new GraphCompositionRepository(this.logger, this.db); + const graphCompositionRepo = new GraphCompositionRepository(this.logger, this.db);And update its usages below (composeWithProposedSchemas constructor argument).
1990-2041: Construct the traffic inspector only when needed; guard when ClickHouse client is absent.You instantiate SchemaUsageTrafficInspector(chClient!) regardless of skipTrafficCheck/composition errors, relying on flow control to avoid usage. Prefer lazy construction and avoid non-null assertion to eliminate foot-guns in test or CH-disabled environments.
- const trafficInspector = new SchemaUsageTrafficInspector(chClient!); + const getTrafficInspector = () => + chClient ? new SchemaUsageTrafficInspector(chClient) : undefined; @@ - inspectorChanges = trafficInspector.schemaChangesToInspectorChanges( + inspectorChanges = getTrafficInspector() + ? getTrafficInspector()!.schemaChangesToInspectorChanges( schemaChanges.breakingChanges, storedBreakingChanges, - ); + ) + : []; @@ - const result = await trafficInspector.inspect(inspectorChanges, { + const inspector = getTrafficInspector(); + if (!inspector) { + continue; + } + const result = await inspector.inspect(inspectorChanges, {Alternatively, return early when skipTrafficCheck || !chClient || !subgraph.
1820-1827: Consider persisting the proposed SDL at check-level (optional).You pass proposedSubgraphSchemaSDL: '' to SchemaCheckRepository.create and store the SDL at subgraph-check level instead. That’s fine, but if you intend to query by proposed SDL at the check root, set it here to newSchemaSDL for easier indexing/filtering.
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (1)
213-216: Typo in comment.“conatins” → “contains”.
- // this new GraphQL schema conatins the location info + // this new GraphQL schema contains the location infocontrolplane/src/db/schema.ts (1)
247-276: Avoid duplicate unique constraints on linked_subgraphs.source_subgraph_id.If migration 0129 also defines a unique on source_subgraph_id (typical), keeping this column-level .unique() here and a table-level UNIQUE in SQL yields two unique indexes on the same column. Prefer a single, named unique constraint via unique('...').on(t.sourceSubgraphId) or keep it only in the SQL migration.
sourceSubgraphId: uuid('source_subgraph_id') .notNull() .references(() => subgraphs.id, { onDelete: 'cascade', }) - .unique(), + ,Follow-up: if you choose to keep the unique in code, drop it from the SQL migration. I can generate a migration patch if you confirm your preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
controlplane/migrations/0131_crazy_diamondback.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(6 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(3 hunks)controlplane/src/db/schema.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/migrations/meta/_journal.json
🧰 Additional context used
🧬 Code graph analysis (2)
controlplane/src/core/repositories/SubgraphRepository.ts (7)
controlplane/src/db/schema.ts (2)
linkedSubgraphs(249-276)subgraphs(218-245)controlplane/src/types/index.ts (3)
NamespaceDTO(741-751)SubgraphDTO(117-138)Label(56-59)connect/src/wg/cosmo/platform/v1/platform_pb.ts (3)
VCSContext(929-973)Label(386-424)CheckSubgraphSchemaResponse(2159-2271)controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(54-1087)controlplane/src/core/composition/schemaCheck.ts (1)
getDiffBetweenGraphs(48-113)controlplane/src/core/composition/composer.ts (1)
CheckSubgraph(260-271)controlplane/src/core/services/SchemaUsageTrafficInspector.ts (4)
SchemaUsageTrafficInspector(33-142)InspectorOperationResult(23-31)InspectorSchemaChange(6-14)collectOperationUsageStats(144-190)
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (1)
cli/src/commands/subgraph/commands/check.ts (1)
opts(13-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_test_node_matrix (22.x)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (5)
controlplane/migrations/0131_crazy_diamondback.sql (1)
20-21: LGTM on indexes.The covering btree indexes look appropriate for join/filter patterns.
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1750-1771: LGTM: organization scoping in getLinkedSubgraph.The join through targets and scoping by targets.organizationId prevents cross-org leakage. Good.
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (2)
112-129: LGTM: minimal linked-subgraph resolution with org/namespace context.The early resolution of a linked subgraph using SubgraphRepository.getLinkedSubgraph and the compact shape you pass downstream is clear.
279-299: Propagate non-OK response while preserving computed artifacts.This block preserves breaking/non-breaking changes and usage stats from the repository response. Good. Make sure proposalMatchMessage is present in both branches (it is), and that clientTrafficCheckSkipped reflects the request (it does).
controlplane/src/db/schema.ts (1)
821-821: isSubgraphLinked field addition looks good.This aligns with the repository’s create() call and supports downstream filtering/UIs.
…related functionality - Dropped the `is_subgraph_linked` column from the `schema_checks` table to streamline the schema. - Updated the `checkSubgraphSchema`, `SchemaCheckRepository`, and `SubgraphRepository` to remove references to the removed column. - Enhanced error handling in `checkSubgraphSchema` for linked subgraphs. - Updated migration files to reflect the removal of the column and ensure database integrity.
- Added NOT NULL constraints to the `schema_check_id` and `linked_schema_check_id` columns in the `linked_schema_checks` table to ensure data integrity. - Updated migration files to reflect these changes and maintain database consistency.
- Changed the assignment of labels in the `checkSubgraphSchema` function to use `req.labels` instead of `subgraph?.labels` for improved accuracy in handling request data.
- Changed the SchemaCheck message structure to replace the single linkedCheck field with a repeated linkedChecks field, allowing for multiple linked checks. - Updated related TypeScript and Go files to reflect this change, ensuring compatibility across the codebase. - Modified the logic in various services and repositories to handle multiple linked checks, enhancing the functionality of schema validation and proposal processes. - Adjusted UI components to display information for multiple linked checks, improving user experience and clarity.
- Updated the target subgraph option in the link command to use angle brackets for the string type, improving clarity and consistency in command usage.
- Modified the hasClientTraffic assignment to include a check for isLinkedTrafficCheckFailed, ensuring accurate traffic status reporting for subgraphs.
- Introduced a new boolean field `isForcedSuccess` to the SchemaCheck message in the proto definition, allowing for enhanced schema validation. - Updated TypeScript and Go files to reflect this addition, ensuring compatibility across the codebase. - Modified the SchemaCheckRepository to handle the new field and updated the LinkedCheckDTO interface accordingly. - Adjusted UI components to utilize the new `isForcedSuccess` field for improved user experience in displaying check statuses.
…d graphs - Introduced a new test to validate that checks are correctly associated with contracts in federated graphs. - The test includes creating a subgraph, a federated graph, and a contract, followed by schema validation and fetching checks for both the federated graph and the contract. - Ensured that the checks reflect the correct associations and details, enhancing the reliability of the schema validation process.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (4)
417-426: Fix grammar in toast messageUse “mark” (present tense), not “marked”.
- toast({ - description: `Could not marked check as successful. ${data.response?.details}`, - duration: 3000, - }); + toast({ + description: `Could not mark check as successful. ${data.response?.details}`, + duration: 3000, + });
425-428: Fix grammar in error toastUse “mark”.
- toast({ - description: "Could not marked check as successful. Please try again", - duration: 3000, - }); + toast({ + description: "Could not mark check as successful. Please try again", + duration: 3000, + });
1456-1460: Typo: “wanings” → “warnings”User-facing text.
- : "No composition wanings"} + : "No composition warnings"}
866-874: Add rel="noopener noreferrer" on external link opened in a new tabPrevents tabnabbing.
- <Link + <Link href={`https://github.com/${ghDetails.ownerSlug}/${ghDetails.repositorySlug}/commit/${ghDetails.commitSha}`} className="inline-flex items-center gap-2 text-xs" - aria-label="View on GitHub" - target="_blank" + aria-label="View on GitHub" + target="_blank" + rel="noopener noreferrer" >controlplane/src/core/repositories/SchemaCheckRepository.ts (3)
1067-1067: Bug: hasClientTraffic is overwritten, not accumulatedThe last composition’s result clobbers earlier ones; OR the values to preserve any failure.
- hasClientTraffic = overrideCheck.hasUnsafeClientTraffic; + hasClientTraffic = hasClientTraffic || overrideCheck.hasUnsafeClientTraffic;
674-681: Compute router compatibility after fetching graphs (wrong dependency order)
routerCompatibilityVersionis computed beforegraphsexist; use the just-fetchedgraphsfor accuracy.- const routerCompatibilityVersion = getFederatedGraphRouterCompatibilityVersion(federatedGraphs); - - const graphs = await fedGraphRepo.bySubgraphLabels({ + const graphs = await fedGraphRepo.bySubgraphLabels({ labels: subgraph?.labels || s.labels, namespaceId: namespace.id, }); + const routerCompatibilityVersion = getFederatedGraphRouterCompatibilityVersion( + graphs.length > 0 ? graphs : federatedGraphs + );
861-861: Fix “undefined” prefix in proposalMatchMessage aggregationAppending to possibly-undefined yields “undefined…”. Normalize before concatenation.
- proposalMatchMessage += `The subgraph ${subgraphName}'s schema does not match to this subgraph's schema in any approved proposal.\n`; + proposalMatchMessage = `${proposalMatchMessage ?? ''}The subgraph ${subgraphName}'s schema does not match to this subgraph's schema in any approved proposal.\n`;
♻️ Duplicate comments (11)
controlplane/migrations/meta/_journal.json (1)
909-921: Migrations journal updated — verify DDL exists for linked tables and constraints.Given previous feedback, confirm 0129/0130 files contain the expected tables and indexes.
#!/bin/bash set -euo pipefail echo "Check presence of migrations" fd -a '0129_*.sql' controlplane/migrations fd -a '0130_*.sql' controlplane/migrations echo "Verify DDL for linked_subgraphs" rg -n -C2 -iP 'create\s+table\s+.*linked[_ ]subgraphs' controlplane/migrations --glob '!**/node_modules/**' echo "Verify DDL for linked_schema_checks" rg -n -C2 -iP 'create\s+table\s+.*linked[_ ]schema[_ ]checks' controlplane/migrations --glob '!**/node_modules/**' echo "Verify indexes (examples)" rg -n -iP 'create\s+index.*(source_subgraph_id|target_subgraph_id|created_by_id)' controlplane/migrations --glob '!**/node_modules/**' rg -n -iP '(unique\s+index|constraint).*schema_check_id.*linked_schema_check_id' controlplane/migrations --glob '!**/node_modules/**'cli/src/commands/subgraph/commands/link.ts (2)
21-28: Harden target parsing: reject empty subgraph names.
rest.length > 0still allowsns/orns//to pass and yields an empty name. Guard against empty after join.- const [targetNamespace, ...rest] = options.targetSubgraph.split('/'); - if (!targetNamespace || rest.length === 0) { + const [targetNamespace, ...rest] = String(options.targetSubgraph).split('/'); + if (!targetNamespace || rest.length === 0 || rest.join('/').trim().length === 0) { program.error('Target subgraph must be in the format <namespace>/<subgraph-name>'); }
36-58: Wrap RPC call in try/catch to handle transport errors.Prevents unhandled rejections and gives users actionable output.
- const resp = await opts.client.platform.linkSubgraph( + try { + const resp = await opts.client.platform.linkSubgraph( { sourceSubgraphName: name, sourceSubgraphNamespace: options.namespace, targetSubgraphName, targetSubgraphNamespace: targetNamespace, }, { headers: getBaseHeaders(), }, - ); + ); - if (resp.response?.code === EnumStatusCode.OK) { - spinner.succeed('Subgraph was linked successfully.'); - } else { - spinner.fail('Failed to link subgraph.'); - if (resp.response?.details) { - console.log(pc.red(pc.bold(resp.response?.details))); - } - process.exitCode = 1; - // eslint-disable-next-line no-useless-return - return; - } + if (resp.response?.code === EnumStatusCode.OK) { + spinner.succeed('Subgraph was linked successfully.'); + } else { + spinner.fail('Failed to link subgraph.'); + if (resp.response?.details) { + console.log(pc.red(pc.bold(resp.response?.details))); + } + process.exitCode = 1; + return; + } + } catch (err) { + spinner.fail('Failed to link subgraph.'); + console.log(pc.red(pc.bold((err as Error)?.message ?? String(err)))); + process.exitCode = 1; + return; + }studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (2)
915-924: Overview tab should not add tab=overview to the URLKeep the URL clean for the default tab by omitting the tab param.
- <TabsTrigger - value="overview" - className="flex items-center gap-x-2" - asChild - > - <Link href={{ query: { ...router.query, tab: "overview" } }}> + <TabsTrigger + value="overview" + className="flex items-center gap-x-2" + asChild + > + <Link + href={{ + pathname: router.pathname, + query: Object.fromEntries( + Object.entries(router.query).filter(([k]) => k !== "tab"), + ), + }} + > <CheckCircleIcon className="h-4 w-4 flex-shrink-0" /> Overview </Link> </TabsTrigger>
1172-1176: Don’t apply the current check’s forced-success flag to other graphsPass a per-row flag (if available) or false; using the current check’s flag mislabels statuses.
- {getCheckBadge( - isCheckSuccessful, - data.check?.isForcedSuccess || false, - )} + {getCheckBadge( + isCheckSuccessful, + false, + )}controlplane/migrations/0129_strong_betty_brant.sql (1)
8-15: Prevent self-linking at the DB levelAdd a CHECK constraint to block
source_subgraph_id = target_subgraph_id.CREATE TABLE IF NOT EXISTS "linked_subgraphs" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "source_subgraph_id" uuid NOT NULL, "target_subgraph_id" uuid NOT NULL, "created_at" timestamp with time zone DEFAULT now() NOT NULL, "created_by_id" uuid, CONSTRAINT "linked_subgraphs_source_subgraph_id_unique" UNIQUE("source_subgraph_id") + ,CONSTRAINT "linked_subgraphs_no_self_link" CHECK ("source_subgraph_id" <> "target_subgraph_id") );connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
2232-2244: Tri-state semantics for linked-check failure flagsThese are optional booleans—good. Ensure producers only set them when a linked check actually ran; do not default to false at write-time. Consumers (CLI/Studio) should treat undefined as “N/A,” not “false.” This echoes earlier feedback.
Run to audit producers/consumers for improper defaulting:
#!/bin/bash rg -n -C2 -e '\bisLinked(Traffic|Pruning)CheckFailed\b' \ controlplane cli studio apps || true
22661-22831: Link/Unlink requests: add proto-level input validation (non-empty strings)Generated TS can’t enforce required fields. Add validators in platform.proto to reject empty names/namespaces across languages; regenerate.
Proto diff (buf-validate):
import "buf/validate/validate.proto"; message LinkSubgraphRequest { - string sourceSubgraphName = 1; - string sourceSubgraphNamespace = 2; - string targetSubgraphName = 3; - string targetSubgraphNamespace = 4; + string sourceSubgraphName = 1 [(buf.validate.field).string.min_len = 1]; + string sourceSubgraphNamespace = 2 [(buf.validate.field).string.min_len = 1]; + string targetSubgraphName = 3 [(buf.validate.field).string.min_len = 1]; + string targetSubgraphNamespace = 4 [(buf.validate.field).string.min_len = 1]; } message UnlinkSubgraphRequest { - string sourceSubgraphName = 1; - string sourceSubgraphNamespace = 2; + string sourceSubgraphName = 1 [(buf.validate.field).string.min_len = 1]; + string sourceSubgraphNamespace = 2 [(buf.validate.field).string.min_len = 1]; }Also verify the RPCs are exposed on PlatformService and connect stubs are regenerated.
#!/bin/bash rg -n -C2 'rpc\s+LinkSubgraph|rpc\s+UnlinkSubgraph' proto || true rg -n -C2 'LinkSubgraph(Request|Response)|UnlinkSubgraph(Request|Response)' connect/src connect-go || truecontrolplane/src/db/schema.ts (1)
829-855: Prevent self-link in linked_schema_checks.Similarly, add a DB-level CHECK to ensure schema_check_id != linked_schema_check_id (migration-level, since Drizzle lacks a helper). Keeps data sane and simplifies idempotent callers.
controlplane/src/core/repositories/SubgraphRepository.ts (2)
1755-1756: Make linking idempotent to avoid 500s on races.Two concurrent link requests can violate the unique(source_subgraph_id) constraint. Ignore duplicates at DB level.
- await this.db.insert(linkedSubgraphs).values({ sourceSubgraphId, targetSubgraphId, createdById }); + await this.db + .insert(linkedSubgraphs) + .values({ sourceSubgraphId, targetSubgraphId, createdById }) + .onConflictDoNothing();
2001-2013: Guard ClickHouse dependency; avoid non-null assertion crash.chClient! can be undefined (tests/local). Instantiate inspector conditionally and guard its usage.
- const trafficInspector = new SchemaUsageTrafficInspector(chClient!); + const trafficInspector = chClient ? new SchemaUsageTrafficInspector(chClient) : undefined; @@ - // For operations checks we only consider breaking changes - inspectorChanges = trafficInspector.schemaChangesToInspectorChanges( - schemaChanges.breakingChanges, - storedBreakingChanges, - ); + // For operations checks we only consider breaking changes + inspectorChanges = trafficInspector + ? trafficInspector.schemaChangesToInspectorChanges( + schemaChanges.breakingChanges, + storedBreakingChanges, + ) + : []; @@ - if (composedGraph.errors.length > 0 || inspectorChanges.length === 0 || skipTrafficCheck || !subgraph) { + if ( + composedGraph.errors.length > 0 || + inspectorChanges.length === 0 || + !trafficInspector || + skipTrafficCheck || + !subgraph + ) { continue; } @@ - const result = await trafficInspector.inspect(inspectorChanges, { + const result = await trafficInspector.inspect(inspectorChanges, {Also applies to: 2040-2050
🧹 Nitpick comments (32)
studio/src/components/info-tooltip.tsx (1)
21-28: A11y: make the tooltip trigger keyboard-focusable and labeled.
A span is not focusable; keyboard users can’t access the tooltip. Use a button or add role/tabIndex + key handlers.Apply this diff:
- <TooltipTrigger asChild> - <span - className={cn( - "text-sm text-muted-foreground", - props.tooltipTriggerClassName, - )} - > - <FiInfo /> - </span> - </TooltipTrigger> + <TooltipTrigger asChild> + <button + type="button" + aria-label="More info" + className={cn( + "text-sm text-muted-foreground bg-transparent p-0 border-0", + props.tooltipTriggerClassName, + )} + > + <FiInfo aria-hidden="true" /> + </button> + </TooltipTrigger>controlplane/src/core/util.ts (1)
568-593: Default linked-failure flags to false to avoid accidental truthy/undefined pitfallsMinor ergonomics/readability improvement; no behavior change.
Apply this diff:
export const isCheckSuccessful = ({ isComposable, isBreaking, hasClientTraffic, hasLintErrors, hasGraphPruningErrors, clientTrafficCheckSkipped, hasProposalMatchError, - isLinkedTrafficCheckFailed, - isLinkedPruningCheckFailed, + isLinkedTrafficCheckFailed = false, + isLinkedPruningCheckFailed = false, }: {controlplane/migrations/0130_skinny_solo.sql (1)
1-2: Make constraint drop idempotent; consider concurrent index creationUse IF EXISTS to avoid failures in partially applied environments. Optionally use CONCURRENTLY on large tables (only if your migration runner allows non-transactional DDL).
Apply this diff:
-ALTER TABLE "linked_schema_checks" DROP CONSTRAINT "linked_schema_checks_schema_check_id_unique";--> statement-breakpoint +ALTER TABLE "linked_schema_checks" DROP CONSTRAINT IF EXISTS "linked_schema_checks_schema_check_id_unique";--> statement-breakpointNote: If feasible in your migration setup, prefer:
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS ...
to reduce locking during index build.controlplane/src/core/bufservices/PlatformService.ts (1)
169-171: Expose LinkSubgraph/UnlinkSubgraph on the service — OK. Consider section placement.These are mutations but are currently listed after the Queries header. For consistency and easier discovery, move them into the Mutations block alongside other subgraph mutations.
- validateAndFetchPluginData: (req, ctx) => { + validateAndFetchPluginData: (req, ctx) => { return validateAndFetchPluginData(opts, req, ctx); }, - - linkSubgraph: (req, ctx) => { - return linkSubgraph(opts, req, ctx); - }, - - unlinkSubgraph: (req, ctx) => { - return unlinkSubgraph(opts, req, ctx); - }, + // ... keep other mutations together ...Also applies to: 845-851
controlplane/test/fetch-checks-by-federated-graph.test.ts (1)
790-946: Contract association test is solid and exercises the end-to-end path.Nice coverage ensuring check IDs align between source FG and contract.
Wrap server lifecycle in try/finally to guarantee cleanup on assertion failures.
- const { client, server } = await SetupTest({ dbname, chClient }); - // ... test body ... - await server.close(); + const { client, server } = await SetupTest({ dbname, chClient }); + try { + // ... test body ... + } finally { + await server.close(); + }controlplane/src/core/bufservices/subgraph/getSubgraphByName.ts (1)
57-90: Linked subgraph hydration in GetSubgraphByName — good addition.Optional mapping prevents leakage when no link exists.
Fetch members and link in parallel to shave a DB round-trip.
- const linkedSubgraph = await subgraphRepo.getLinkedSubgraph({ sourceSubgraphId: subgraph.id }); + const [linkedSubgraph, members] = await Promise.all([ + subgraphRepo.getLinkedSubgraph({ sourceSubgraphId: subgraph.id }), + subgraphRepo.getSubgraphMembers(subgraph.id), + ]); @@ - members: await subgraphRepo.getSubgraphMembers(subgraph.id), + members,cli/src/commands/subgraph/commands/link.ts (1)
29-32: Polish wording.Singular reads better.
- program.error('The source and target subgraphs cannot be the same subgraphs.'); + program.error('The source and target subgraphs cannot be the same subgraph.');cli/src/handle-check-result.ts (1)
231-244: Tighten user message and remove stray leading spaces.Improves readability; keeps details for traffic/pruning.
- if (resp.isLinkedTrafficCheckFailed || resp.isLinkedPruningCheckFailed) { - finalStatement += success - ? `\n\n But this schema change has been linked to a target subgraph and the target subgraph check has failed.` - : `\n\n This schema change has been linked to a target subgraph and the target subgraph check has failed.`; + if (resp.isLinkedTrafficCheckFailed || resp.isLinkedPruningCheckFailed) { + finalStatement += success + ? `\n\nHowever, this schema change is linked to a target subgraph, and the target's check failed.` + : `\n\nThis schema change is linked to a target subgraph, and the target's check failed.`; if (resp.isLinkedTrafficCheckFailed) { - finalStatement += `\n\n The target subgraph check has failed because of client traffic issues.`; + finalStatement += `\n\nReason: client traffic issues on the target.`; } if (resp.isLinkedPruningCheckFailed) { - finalStatement += `\n\n The target subgraph check has failed because of graph pruning issues.`; + finalStatement += `\n\nReason: graph pruning issues on the target.`; } success = false; }controlplane/src/types/index.ts (1)
175-186: Align property names with existing DTO conventions.Existing DTO uses
graphPruningSkipped, here it’sgraphPruningCheckSkipped. Prefer consistent naming unless proto/db differs.Would you confirm usages and, if appropriate, rename:
- graphPruningCheckSkipped: boolean; + graphPruningSkipped: boolean;Run to verify references:
#!/bin/bash rg -nP 'graphPruning(Check)?Skipped' -C2 --type ts --type tsxcontrolplane/src/core/bufservices/subgraph/linkSubgraph.ts (2)
70-77: Minor wording: “cannot” vs. “can not”.Polish user-facing errors.
- details: `The source subgraph "${req.sourceSubgraphName}" is a feature subgraph. Feature subgraphs can not be linked.`, + details: `The source subgraph "${req.sourceSubgraphName}" is a feature subgraph. Feature subgraphs cannot be linked.`,- details: `The target subgraph "${req.targetSubgraphName}" is a feature subgraph. Feature subgraphs can not be linked.`, + details: `The target subgraph "${req.targetSubgraphName}" is a feature subgraph. Feature subgraphs cannot be linked.`,Also applies to: 103-110
83-91: Race-proof linking; ensure DB uniqueness constraint exists.Between “already linked” check and insert, concurrent calls could create duplicates unless the table enforces uniqueness on
source_subgraph_id. Verify a unique index/constraint and handle conflict as a no-op or ERR.Script to confirm:
#!/bin/bash # Inspect schema/migrations for a unique constraint on linked subgraphs rg -nP '(linked[_-]?subgraphs|CREATE\s+TABLE\s+linked_subgraphs)' -C3 --type sql --type ts rg -nP 'UNIQUE\s*\(.?source_subgraph_id.?\)|uniqueIndex\(.?sourceSubgraphId.?\)' -C2 --type sql --type tsAlso applies to: 116-140
controlplane/src/core/repositories/ProposalRepository.ts (1)
638-647: Fetch subgraphs and linked checks concurrently; also double-check method/arg casingTiny perf win: avoid two awaits in series. Also confirm repository API name/casing (
getLinkedSchemaChecksvsgetLinkedSchemaCheck,schemaCheckIDvsschemaCheckId) to prevent compile/runtime errors.Proposed change (keeps your current arg names; adjust if repo expects
schemaCheckId):- const checkedSubgraphs = await schemaCheckRepo.getCheckedSubgraphsForCheckIdAndFederatedGraphId({ - checkId: c.id, - federatedGraphId, - }); - - const linkedChecks = await schemaCheckRepo.getLinkedSchemaChecks({ - schemaCheckID: c.id, - organizationId, - }); + const [checkedSubgraphs, linkedChecks] = await Promise.all([ + schemaCheckRepo.getCheckedSubgraphsForCheckIdAndFederatedGraphId({ + checkId: c.id, + federatedGraphId, + }), + schemaCheckRepo.getLinkedSchemaChecks({ + schemaCheckID: c.id, // verify correct casing + organizationId, + }), + ]);To verify repo method name and param casing:
#!/bin/bash rg -nP --type=ts 'class SchemaCheckRepository\b' -C1 rg -nP --type=ts '\bgetLinkedSchemaCheck(s)?\b' -C3 rg -nP --type=ts '\bschemaCheckI[dD]\b' -C2controlplane/test/check-subgraph-schema.test.ts (6)
52-106: Tests read well; prefer DEFAULT_NAMESPACE constant over string literalsMinor consistency nit: use
DEFAULT_NAMESPACEinstead of hardcoded"default"throughout to avoid drift.
131-134: Remove unused test arg
roleparam in this test isn’t used. Drop it to avoid confusion.-test('Should allow legacy fallback when checking graph', async (role) => { +test('Should allow legacy fallback when checking graph', async () => {
1225-1338: Linked subgraphs happy-path: assertions are solid; consider asserting summary linkageOptionally add a
getCheckSummaryassertion to validate linked results surface as expected end-to-end.
1403-1416: Skip-traffic scenarios: OK; mocks are unnecessary hereSince
skipTrafficCheck: trueignores traffic, you can omit CH mocks for clarity.- (chClient.queryPromise as Mock) - .mockResolvedValueOnce([...]) - .mockResolvedValueOnce([...]); + // No CH mocks needed when skipTrafficCheck=trueAlso applies to: 1484-1502
1611-1754: Reduce brittle CH mocking in deletion testChaining 16x
mockResolvedValueOnceis brittle. Prefer a single default mock or small helper.Example:
- (chClient.queryPromise as Mock) - .mockResolvedValueOnce([ { operationHash: 'hash1', ... } ]) - ... (many repeats) + (chClient.queryPromise as Mock).mockImplementation(async () => [ + { + operationHash: 'hash1', + operationName: 'op1', + operationType: 'query', + firstSeen: Date.now() / 1000, + lastSeen: Date.now() / 1000, + }, + ]);
1225-1769: Use DEFAULT_NAMESPACE constant throughoutMany occurrences of
"default"here; switching toDEFAULT_NAMESPACEimproves consistency.controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (2)
7-7: Remove unused import
NamespaceRepositoryisn’t used.-import { DefaultNamespace, NamespaceRepository } from '../../repositories/NamespaceRepository.js'; +import { DefaultNamespace } from '../../repositories/NamespaceRepository.js';
56-69: Audit context: include target for better forensicsConsider including the target subgraph in display data to make audits self-explanatory.
- auditableDisplayName: sourceSubgraph.name, + auditableDisplayName: `${sourceSubgraph.name} -> ${linkedSubgraph.targetSubgraphName}`,controlplane/test/subgraph/link-subgraph.test.ts (1)
35-75: Always close the test server via try/finally to prevent resource leaks on assertion failuresWrap each test body in try/finally so the server is closed even if an assertion throws.
- test('Should successfully link two subgraphs in different namespaces', async () => { - const { client, server } = await SetupTest({ dbname }); + test('Should successfully link two subgraphs in different namespaces', async () => { + const { client, server } = await SetupTest({ dbname }); + try { // Create two namespaces await createNamespace(client, 'prod'); ... - expect(linkResponse.response?.code).toBe(EnumStatusCode.OK); - - await server.close(); + expect(linkResponse.response?.code).toBe(EnumStatusCode.OK); + } finally { + await server.close(); + } });Apply the same pattern to the other tests in this file.
controlplane/src/core/bufservices/check/getCheckSummary.ts (1)
150-159: Compute affected-operations once to avoid redundant DB hitsThe same query is executed again inside the loop. Compute once and reuse.
- let hasAffectedOperations = false; - if (check.hasClientTraffic) { - const affectedOperations = await schemaCheckRepo.getAffectedOperationsByCheckId({ - checkId: req.checkId, - limit: 1, - offset: 0, - }); - hasAffectedOperations = affectedOperations.length > 0; - } + const hasAffectedOperations = + check.hasClientTraffic + ? (await schemaCheckRepo.getAffectedOperationsByCheckId({ + checkId: req.checkId, + limit: 1, + offset: 0, + })).length > 0 + : false;- if (check.hasClientTraffic) { - const affectedOperations = await schemaCheckRepo.getAffectedOperationsByCheckId({ - checkId: req.checkId, - limit: 1, - offset: 0, - }); - hasAffectedOperations = affectedOperations.length > 0; - } + // reuse hasAffectedOperations computed aboveAlso applies to: 205-212
controlplane/src/core/repositories/SchemaCheckRepository.ts (4)
751-753: Typo in comment (“conatins” → “contains”)- // this new GraphQL schema conatins the location info + // this new GraphQL schema contains the location info
1091-1149: Run linked-target checks with bounded concurrencyMultiple linked targets can be checked in parallel to reduce latency; use p-limit (already in deps).
- for (const linkedSubgraph of linkedSubgraphs) { - // ... body ... - } + const limitLinked = pLimit(3); + await Promise.all( + linkedSubgraphs.map((linkedSubgraph) => + limitLinked(async () => { + // ... body unchanged ... + }), + ), + );
1088-1104: Avoid re-creating NamespaceRepository inside the loopInstantiate once before iterating linked subgraphs.
- let isLinkedTrafficCheckFailed = false; - let isLinkedPruningCheckFailed = false; + let isLinkedTrafficCheckFailed = false; + let isLinkedPruningCheckFailed = false; + const namespaceRepo = new NamespaceRepository(this.db, organizationId); @@ - const namespaceRepo = new NamespaceRepository(this.db, organizationId); const targetNamespace = await namespaceRepo.byId(targetSubgraph.namespaceId);
1258-1261: Guard against missing target names in affectedGraphsIf a target was deleted or null, this can throw. Filter undefined safely.
- affectedGraphNames: check.affectedGraphs.map(({ federatedGraph }) => federatedGraph.target.name), + affectedGraphNames: check.affectedGraphs + .map(({ federatedGraph }) => federatedGraph?.target?.name) + .filter((n): n is string => !!n),proto/wg/cosmo/platform/v1/platform.proto (1)
516-527: Document LinkedCheck fields for generated-client clarityAdd concise comments describing each field’s semantics.
message LinkedCheck { - string id = 1; - repeated string affectedGraphNames = 2; - bool isCheckSuccessful = 3; - bool hasClientTraffic = 4; - bool hasGraphPruningErrors = 5; - bool clientTrafficCheckSkipped = 6; - bool graphPruningCheckSkipped = 7; - repeated string subgraphNames = 8; - string namespace = 9; - bool isForcedSuccess = 10; + // ID of the linked schema check. + string id = 1; + // Human-readable names of affected federated graphs. + repeated string affectedGraphNames = 2; + // Overall status for the linked check (own criteria only). + bool isCheckSuccessful = 3; + // True if unsafe client traffic was detected. + bool hasClientTraffic = 4; + // True if graph pruning errors were found. + bool hasGraphPruningErrors = 5; + // Whether client-traffic check was skipped. + bool clientTrafficCheckSkipped = 6; + // Whether graph-pruning check was skipped. + bool graphPruningCheckSkipped = 7; + // Subgraphs included in the linked check. + repeated string subgraphNames = 8; + // Namespace of the linked subgraph(s). + string namespace = 9; + // Whether the check result was force-overridden to success. + bool isForcedSuccess = 10; }connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
3983-3987: SchemaCheck.linkedChecks added—payload size considerationRepeated linked checks can get large. If this grows, consider a separate RPC to fetch linked checks on-demand to keep summaries slim.
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (2)
429-435: Commit status should respect skip for traffic.Today the commit check marks hasClientTraffic true even when skipped. Gate by skip to avoid surprising red checks.
- breakingChangesCount: breakingChanges.length, - hasClientTraffic: hasClientTraffic || isLinkedTrafficCheckFailed, + breakingChangesCount: breakingChanges.length, + hasClientTraffic: (!req.skipTrafficCheck && hasClientTraffic) || isLinkedTrafficCheckFailed,
441-460: Optional: include target graphs (linked) in composedGraphs/response.Operation usage for the linked target is persisted under its own check, but composedGraphs/checkedFederatedGraphs here only reflect the source. Consider merging/annotating target graphs for parity in UI/commit messages.
controlplane/src/db/schema.ts (1)
250-276: Prevent source=target self-link at the DB layer.Add a CHECK constraint (via SQL migration) to forbid linking a subgraph to itself (sourceSubgraphId != targetSubgraphId). This guards against accidental self-links regardless of app logic.
controlplane/src/core/repositories/SubgraphRepository.ts (1)
1826-1827: Nit: spelling of variable name.Rename graphCompostionRepo → graphCompositionRepo for consistency/readability.
- const graphCompostionRepo = new GraphCompositionRepository(this.logger, this.db); + const graphCompositionRepo = new GraphCompositionRepository(this.logger, this.db);And update references accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
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/**
📒 Files selected for processing (38)
cli/src/commands/mcp/tools/subgraph-verify-schema-changes.ts(3 hunks)cli/src/commands/subgraph/commands/link.ts(1 hunks)cli/src/commands/subgraph/commands/unlink.ts(1 hunks)cli/src/commands/subgraph/index.ts(2 hunks)cli/src/handle-check-result.ts(1 hunks)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_connect.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(11 hunks)controlplane/migrations/0129_strong_betty_brant.sql(1 hunks)controlplane/migrations/0130_skinny_solo.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/PlatformService.ts(2 hunks)controlplane/src/core/bufservices/check/getCheckSummary.ts(4 hunks)controlplane/src/core/bufservices/proposal/createProposal.ts(1 hunks)controlplane/src/core/bufservices/proposal/getProposalChecks.ts(1 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(1 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(7 hunks)controlplane/src/core/bufservices/subgraph/getSubgraphByName.ts(2 hunks)controlplane/src/core/bufservices/subgraph/linkSubgraph.ts(1 hunks)controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts(1 hunks)controlplane/src/core/repositories/ProposalRepository.ts(3 hunks)controlplane/src/core/repositories/SchemaCheckRepository.ts(10 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(7 hunks)controlplane/src/core/util.ts(2 hunks)controlplane/src/db/models.ts(2 hunks)controlplane/src/db/schema.ts(3 hunks)controlplane/src/types/index.ts(2 hunks)controlplane/test/check-subgraph-schema.test.ts(6 hunks)controlplane/test/fetch-checks-by-federated-graph.test.ts(1 hunks)controlplane/test/subgraph/link-subgraph.test.ts(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(7 hunks)studio/src/components/check-badge-icon.tsx(1 hunks)studio/src/components/info-tooltip.tsx(2 hunks)studio/src/components/layout/subgraph-layout.tsx(2 hunks)studio/src/components/ui/alert.tsx(1 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx(8 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx(2 hunks)studio/src/pages/[organizationSlug]/[namespace]/subgraph/[subgraphSlug]/index.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/types/index.tscli/src/handle-check-result.tscontrolplane/src/core/bufservices/subgraph/linkSubgraph.tscontrolplane/test/subgraph/link-subgraph.test.tsstudio/src/pages/[organizationSlug]/[namespace]/subgraph/[subgraphSlug]/index.tsxcli/src/commands/subgraph/index.tsstudio/src/components/layout/subgraph-layout.tsxcontrolplane/src/core/bufservices/subgraph/getSubgraphByName.tscontrolplane/migrations/0129_strong_betty_brant.sqlcontrolplane/test/check-subgraph-schema.test.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscli/src/commands/mcp/tools/subgraph-verify-schema-changes.tsproto/wg/cosmo/platform/v1/platform.protoconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/db/schema.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/test/fetch-checks-by-federated-graph.test.tscli/src/commands/subgraph/commands/link.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/proposal/createProposal.tscontrolplane/src/core/bufservices/subgraph/linkSubgraph.tscontrolplane/src/core/bufservices/subgraph/unlinkSubgraph.tscontrolplane/test/subgraph/link-subgraph.test.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
cli/src/handle-check-result.tscontrolplane/src/core/util.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsxcontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/ProposalRepository.tsstudio/src/components/check-badge-icon.tsxcli/src/commands/mcp/tools/subgraph-verify-schema-changes.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/bufservices/proposal/createProposal.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/test/check-subgraph-schema.test.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/ProposalRepository.tscontrolplane/src/db/schema.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-28T13:07:08.273Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:139-141
Timestamp: 2025-08-28T13:07:08.273Z
Learning: In the Cosmo schema check system, `hasClientTraffic` is a boolean flag that indicates unsafe/problematic client traffic that would be negatively impacted by schema changes. It represents a failure condition, not just the presence of traffic. When `hasClientTraffic` is true, it means there are operations that would be affected by breaking changes, making it appropriate to use directly as a failure indicator.
Applied to files:
controlplane/src/core/util.tscli/src/commands/mcp/tools/subgraph-verify-schema-changes.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: In the SubgraphRepository class, both byId() and byName() methods are equally valid for fetching subgraph data since they both use the same underlying getSubgraph() helper method. When getLinkedSubgraph() returns targetSubgraphName and targetSubgraphNamespace, using byName() is the appropriate choice rather than byId().
Applied to files:
controlplane/src/core/bufservices/subgraph/getSubgraphByName.tsproto/wg/cosmo/platform/v1/platform.protocontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/repositories/SubgraphRepository.ts
📚 Learning: 2025-09-04T13:08:10.330Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:246-277
Timestamp: 2025-09-04T13:08:10.330Z
Learning: The skipTrafficCheck flag in the schema check system controls whether traffic-related failures cause the overall check to fail, but traffic inspection always runs to collect usage data. When skipTrafficCheck is true, the system still performs traffic analysis but doesn't treat traffic issues (like hasClientTraffic=true) as blocking failures.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-09-03T21:12:18.149Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: cli/src/commands/subgraph/commands/link.ts:21-28
Timestamp: 2025-09-03T21:12:18.149Z
Learning: Subgraph names in the Cosmo platform can contain slashes, so when parsing formats like `<namespace>/<subgraph-name>`, only the first slash should be treated as the delimiter between namespace and subgraph name. The subgraph name portion can contain additional slashes.
Applied to files:
cli/src/commands/subgraph/commands/link.ts
🧬 Code graph analysis (18)
cli/src/commands/subgraph/commands/unlink.ts (3)
cli/src/commands/subgraph/commands/link.ts (1)
opts(8-62)cli/src/core/types/types.ts (1)
BaseCommandOptions(8-10)cli/src/core/config.ts (1)
getBaseHeaders(40-46)
controlplane/src/core/bufservices/PlatformService.ts (4)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1746-1756)unlinkSubgraph(1758-1760)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (2)
linkSubgraph(2624-2633)unlinkSubgraph(2640-2649)cli/src/commands/subgraph/index.ts (1)
opts(17-38)controlplane/src/core/routes.ts (1)
opts(62-67)
controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (5)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1746-1756)SubgraphRepository(80-2135)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (3)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)controlplane/src/core/repositories/NamespaceRepository.ts (2)
DefaultNamespace(7-7)NamespaceRepository(9-184)controlplane/src/core/repositories/AuditLogRepository.ts (1)
AuditLogRepository(28-137)
controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (5)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
unlinkSubgraph(1758-1760)SubgraphRepository(80-2135)controlplane/src/core/routes.ts (1)
RouterOptions(25-54)controlplane/src/core/util.ts (3)
getLogger(92-94)handleError(50-88)enrichLogger(96-113)controlplane/src/core/repositories/NamespaceRepository.ts (1)
DefaultNamespace(7-7)controlplane/src/core/repositories/AuditLogRepository.ts (1)
AuditLogRepository(28-137)
controlplane/test/subgraph/link-subgraph.test.ts (3)
controlplane/src/core/test-util.ts (6)
beforeAllSetup(38-45)afterAllSetup(47-51)genID(53-55)genUniqueLabel(57-59)createTestRBACEvaluator(173-175)createTestGroup(181-198)controlplane/test/test-util.ts (6)
SetupTest(66-417)DEFAULT_NAMESPACE(52-52)DEFAULT_SUBGRAPH_URL_ONE(49-49)DEFAULT_SUBGRAPH_URL_TWO(50-50)createSubgraph(704-716)createBaseAndFeatureSubgraph(718-737)controlplane/src/db/models.ts (1)
OrganizationRole(32-32)
studio/src/components/info-tooltip.tsx (1)
studio/src/lib/utils.ts (1)
cn(6-8)
studio/src/components/layout/subgraph-layout.tsx (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
GetSubgraphByNameResponse(3473-3523)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (3)
GetSubgraphByNameResponse(4551-4560)GetSubgraphByNameResponse(4575-4575)GetSubgraphByNameResponse(4590-4592)
controlplane/test/check-subgraph-schema.test.ts (3)
controlplane/test/test-util.ts (2)
SetupTest(66-417)DEFAULT_NAMESPACE(52-52)controlplane/src/core/test-util.ts (5)
genID(53-55)genUniqueLabel(57-59)createTestRBACEvaluator(173-175)createTestGroup(181-198)createAPIKeyTestRBACEvaluator(177-179)shared/src/utils/util.ts (1)
joinLabel(17-19)
controlplane/src/core/repositories/SchemaCheckRepository.ts (3)
controlplane/src/db/schema.ts (2)
linkedSubgraphs(249-276)linkedSchemaChecks(829-854)controlplane/src/core/repositories/NamespaceRepository.ts (1)
NamespaceRepository(9-184)controlplane/src/core/util.ts (2)
clamp(564-566)isCheckSuccessful(568-603)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (4)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
linkSubgraph(1746-1756)unlinkSubgraph(1758-1760)controlplane/src/core/bufservices/subgraph/linkSubgraph.ts (1)
linkSubgraph(12-148)connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
LinkSubgraphRequest(22664-22714)LinkSubgraphResponse(22719-22751)UnlinkSubgraphRequest(22756-22794)UnlinkSubgraphResponse(22799-22831)controlplane/src/core/bufservices/subgraph/unlinkSubgraph.ts (1)
unlinkSubgraph(12-77)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (21)
GetSubgraphByNameResponse_LinkedSubgraph(27690-27698)GetSubgraphByNameResponse_LinkedSubgraph(27713-27713)GetSubgraphByNameResponse_LinkedSubgraph(27728-27730)SchemaCheck_LinkedCheck(27905-27920)SchemaCheck_LinkedCheck(27935-27935)SchemaCheck_LinkedCheck(27950-27952)LinkSubgraphRequest(27415-27424)LinkSubgraphRequest(27439-27439)LinkSubgraphRequest(27454-27456)LinkSubgraphResponse(27486-27492)LinkSubgraphResponse(27507-27507)LinkSubgraphResponse(27522-27524)Response(712-720)Response(735-735)Response(750-752)UnlinkSubgraphRequest(27533-27540)UnlinkSubgraphRequest(27555-27555)UnlinkSubgraphRequest(27570-27572)UnlinkSubgraphResponse(27588-27594)UnlinkSubgraphResponse(27609-27609)UnlinkSubgraphResponse(27624-27626)
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (3)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(61-1281)controlplane/src/core/util.ts (1)
clamp(564-566)composition/src/ast/utils.ts (1)
parse(272-274)
connect/src/wg/cosmo/platform/v1/platform_connect.ts (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
LinkSubgraphRequest(22664-22714)LinkSubgraphResponse(22719-22751)UnlinkSubgraphRequest(22756-22794)UnlinkSubgraphResponse(22799-22831)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (12)
LinkSubgraphRequest(27415-27424)LinkSubgraphRequest(27439-27439)LinkSubgraphRequest(27454-27456)LinkSubgraphResponse(27486-27492)LinkSubgraphResponse(27507-27507)LinkSubgraphResponse(27522-27524)UnlinkSubgraphRequest(27533-27540)UnlinkSubgraphRequest(27555-27555)UnlinkSubgraphRequest(27570-27572)UnlinkSubgraphResponse(27588-27594)UnlinkSubgraphResponse(27609-27609)UnlinkSubgraphResponse(27624-27626)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (4)
studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/components/ui/table.tsx (7)
TableWrapper(127-127)Table(126-126)TableHeader(128-128)TableRow(132-132)TableHead(131-131)TableBody(129-129)TableCell(133-133)studio/src/components/check-badge-icon.tsx (3)
getCheckBadge(59-59)isCheckSuccessful(59-59)getCheckIcon(59-59)controlplane/src/core/util.ts (1)
isCheckSuccessful(568-603)
controlplane/src/core/repositories/SubgraphRepository.ts (10)
controlplane/src/db/schema.ts (2)
linkedSubgraphs(249-276)subgraphs(218-245)controlplane/src/types/index.ts (5)
NamespaceDTO(755-765)SubgraphDTO(117-138)Label(56-59)SchemaLintIssues(693-696)SchemaGraphPruningIssues(729-732)connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
Label(386-424)CheckSubgraphSchemaResponse(2159-2287)CompositionError(1701-1751)CompositionWarning(1756-1806)controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(61-1281)controlplane/src/core/repositories/ProposalRepository.ts (1)
ProposalRepository(14-767)controlplane/src/core/repositories/SchemaLintRepository.ts (1)
SchemaLintRepository(10-136)controlplane/src/core/repositories/SchemaGraphPruningRepository.ts (1)
SchemaGraphPruningRepository(26-225)controlplane/src/core/composition/schemaCheck.ts (1)
getDiffBetweenGraphs(48-113)controlplane/src/core/composition/composer.ts (1)
CheckSubgraph(260-271)controlplane/src/core/services/SchemaUsageTrafficInspector.ts (4)
SchemaUsageTrafficInspector(33-142)InspectorOperationResult(23-31)InspectorSchemaChange(6-14)collectOperationUsageStats(144-190)
controlplane/test/fetch-checks-by-federated-graph.test.ts (2)
controlplane/test/test-util.ts (2)
SetupTest(66-417)DEFAULT_NAMESPACE(52-52)controlplane/src/core/test-util.ts (1)
genID(53-55)
controlplane/src/core/bufservices/check/getCheckSummary.ts (3)
connect-go/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)controlplane/src/core/repositories/SubgraphRepository.ts (1)
checkDetails(1303-1359)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
LintSeverity(27-27)LintSeverity(56-58)LintSeverity(60-62)LintSeverity(69-71)
cli/src/commands/subgraph/commands/link.ts (3)
cli/src/commands/subgraph/commands/unlink.ts (1)
opts(8-41)cli/src/core/types/types.ts (1)
BaseCommandOptions(8-10)cli/src/core/config.ts (1)
getBaseHeaders(40-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- Changed the namespace option syntax in both the link and unlink commands to use angle brackets for the string type, ensuring consistency and clarity in command usage.
- Added new fields `isLinkedTrafficCheckFailed` and `isLinkedPruningCheckFailed` to the `CreateProposalResponse` and `UpdateProposalResponse` messages to indicate failures in linked checks. - Updated the `handleProposalResult` function to append relevant messages based on the status of linked checks. - Modified proposal creation and update processes to include linked check statuses. - Adjusted repository methods to retrieve linked check information and determine overall proposal success based on these checks.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
545-563: Guard against undefinedlinkedChecksto avoid runtime errorsOlder records or partial responses might omit
linkedChecks. Calling.someonundefinedwill crash. Default to an empty array at destructuring.- proposalMatch, - linkedChecks, + proposalMatch, + linkedChecks = [],
564-574: SynchronizeisCheckSuccessfulinvocation with its signature
In studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (lines 564–574), you’re passing nine positional args toisCheckSuccessful, but the imported function either:
- Defines exactly five positional booleans (
isComposable, isBreaking, hasClientTraffic, hasLintErrors, hasGraphPruningErrors), or- Expects a single destructured object with those five properties.
Adjust the call to match whichever overload you import: pass only the five booleans in the correct order, or switch to the object form:
isCheckSuccessful({ isComposable, isBreaking, hasClientTraffic, hasLintErrors, hasGraphPruningErrors, })cli/src/handle-proposal-result.ts (2)
86-103: Prevent false success when linked checks failEarly-return “no changes” ignores new linked-failure flags, producing a success even if a linked target check failed.
Apply:
- if ( + if ( resp.nonBreakingChanges.length === 0 && resp.breakingChanges.length === 0 && resp.compositionErrors.length === 0 && resp.lintErrors.length === 0 && resp.lintWarnings.length === 0 && resp.graphPruneErrors.length === 0 && - resp.graphPruneWarnings.length === 0 + resp.graphPruneWarnings.length === 0 && + !resp.isLinkedTrafficCheckFailed && + !resp.isLinkedPruningCheckFailed ) {
266-275: Return value is wrong on failure pathFailure branch returns
{ success: true }, which inverts outcome and breaks callers.Apply:
- return { success: true, message: successMessage }; + return { success: false, message: successMessage };
♻️ Duplicate comments (5)
proto/wg/cosmo/platform/v1/platform.proto (1)
2731-2734: Resolved: flags documented and naming consistent across layersThanks for addressing the earlier request to document these booleans and normalize naming.
Also applies to: 2810-2813
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
21372-21384: Same tri‑state guidance for proposal responses.Treat these as presence-based; don’t set them unless a linked check ran.
Use the script shared for Lines 2232–2244 to audit CreateProposalResponse producers as well.
21966-21978: Repeat: maintain presence semantics on update responses.Unset vs false should convey “N/A” vs “false”.
22693-22746: Add proto-level input validation for LinkSubgraphRequest (non-empty strings).Generated TS cannot enforce required-ness. Prefer buf-validate or PGV at the proto layer.
Proto example (buf-validate):
message LinkSubgraphRequest { - string sourceSubgraphName = 1; - string sourceSubgraphNamespace = 2; - string targetSubgraphName = 3; - string targetSubgraphNamespace = 4; + string sourceSubgraphName = 1 [(buf.validate.field).string.min_len = 1]; + string sourceSubgraphNamespace = 2 [(buf.validate.field).string.min_len = 1]; + string targetSubgraphName = 3 [(buf.validate.field).string.min_len = 1]; + string targetSubgraphNamespace = 4 [(buf.validate.field).string.min_len = 1]; }I can wire buf-validate/PGV into platform.proto and regen if you want.
2232-2244: Remove explicit defaulting of linked-check failure flags
SchemaCheckRepository initialises both flags to false (src/core/repositories/SchemaCheckRepository.ts:1088-1089), and bufservices sets them to false when no linkedSubgraph (src/core/bufservices/subgraph/checkSubgraphSchema.ts:325-327, 355-356, 412-413). This forces a false value instead of “unset” when no check ran—breaks tri-state semantics. Only set these flags when the linked check actually executes; leave them undefined otherwise. Update any tests (currently expecting default false) accordingly.
🧹 Nitpick comments (13)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
564-574: Minor: compute linked failure flags once for clarityAvoids two iterations and improves readability.
- const isSuccessful = isCheckSuccessful( + const isLinkedTrafficCheckFailed = linkedChecks.some((linkedCheck) => linkedCheck.hasClientTraffic); + const isLinkedPruningCheckFailed = linkedChecks.some((linkedCheck) => linkedCheck.hasGraphPruningErrors); + const isSuccessful = isCheckSuccessful( isComposable, isBreaking, hasClientTraffic, hasLintErrors, hasGraphPruningErrors, clientTrafficCheckSkipped, proposalMatch === "error", - linkedChecks.some((linkedCheck) => linkedCheck.hasClientTraffic), - linkedChecks.some((linkedCheck) => linkedCheck.hasGraphPruningErrors), + isLinkedTrafficCheckFailed, + isLinkedPruningCheckFailed, );controlplane/src/core/bufservices/proposal/updateProposal.ts (6)
494-496: Stabilize response booleans (avoid undefined).If these fields are optional upstream, default to boolean values in the response to keep the API contract predictable.
Apply:
- isLinkedPruningCheckFailed, - isLinkedTrafficCheckFailed, + isLinkedPruningCheckFailed: !!isLinkedPruningCheckFailed, + isLinkedTrafficCheckFailed: !!isLinkedTrafficCheckFailed,
493-496: GuardcheckUrlwhencheckIdis empty.Prevents emitting a dangling URL ending in
/checks/.Apply:
- checkUrl: `${process.env.WEB_BASE_URL}/${authContext.organizationSlug}/${namespace.name}/graph/${federatedGraph.name}/checks/${checkId}`, + checkUrl: checkId + ? `${process.env.WEB_BASE_URL}/${authContext.organizationSlug}/${namespace.name}/graph/${federatedGraph.name}/checks/${checkId}` + : '',
382-391: Normalize labels to an empty array.
labelsis typed asLabel[]. Passingundefinedcan propagate surprises to protobuf/connect layers; other call sites already use|| [].Apply:
proposalSubgraphs.push({ subgraphId: subgraph?.id, subgraphName: proposalSubgraph.name, schemaSDL: proposalSubgraph.schemaSDL, isDeleted: proposalSubgraph.isDeleted, isNew: !subgraph, currentSchemaVersionId: subgraph?.currentSchemaVersionId, - labels: proposalSubgraph.labels, + labels: proposalSubgraph.labels ?? [], });
456-465: Also default labels in the payload sent toProposalSubgraphmessages.Keeps repeated-field semantics consistent across layers.
Apply:
subgraphs: proposalSubgraphs.map( (subgraph) => new ProposalSubgraph({ name: subgraph.subgraphName, schemaSDL: subgraph.schemaSDL, - labels: subgraph.labels, + labels: subgraph.labels ?? [], isDeleted: subgraph.isDeleted, isNew: subgraph.isNew, }), ),
309-336: Reduce N+1 DB calls in the subgraph loop.Each iteration awaits
subgraphRepo.byNameserially. For proposals with many subgraphs this adds latency.You can parallelize lookups while preserving current validations:
- // Process subgraphs if they are provided - for (const proposalSubgraph of updatedSubgraphs) { - const subgraph = await subgraphRepo.byName(proposalSubgraph.name, req.namespace); - ... - proposalSubgraphs.push({ ... }); - } + // Process subgraphs in parallel + const lookedUp = await Promise.all( + updatedSubgraphs.map(async (psg) => ({ + psg, + subgraph: await subgraphRepo.byName(psg.name, req.namespace), + })), + ); + + for (const { psg: proposalSubgraph, subgraph } of lookedUp) { + ... + proposalSubgraphs.push({ ... }); + }Also applies to: 382-391
419-430: Nit: fix variable typo for readability.
graphCompostionRepo→graphCompositionRepo.Apply:
- const graphCompostionRepo = new GraphCompositionRepository(logger, opts.db); + const graphCompositionRepo = new GraphCompositionRepository(logger, opts.db); const trafficInspector = new SchemaUsageTrafficInspector(opts.chClient!); const composer = new Composer( logger, opts.db, fedGraphRepo, subgraphRepo, contractRepo, - graphCompostionRepo, + graphCompositionRepo, opts.chClient, );controlplane/src/core/repositories/ProposalRepository.ts (1)
659-663: Avoid N+1 for linked checks (optional)Per-check calls to getLinkedSchemaChecks introduce N+1 queries. Consider a batch repository method (e.g., getLinkedSchemaChecksForMany(ids, organizationId)) to fetch once and map in-memory.
cli/src/handle-proposal-result.ts (1)
239-252: Tighten message and avoid awkward leading spaces/newlinesSlight copy/formatting fixes make output clearer and consistent.
Apply:
- if (resp.isLinkedTrafficCheckFailed || resp.isLinkedPruningCheckFailed) { - finalStatement += success - ? `\n\n But this schema change has been linked to a target subgraph and the target subgraph check has failed.` - : `\n\n This schema change has been linked to a target subgraph and the target subgraph check has failed.`; - - if (resp.isLinkedTrafficCheckFailed) { - finalStatement += `\n\n The target subgraph check has failed because of client traffic issues.`; - } - - if (resp.isLinkedPruningCheckFailed) { - finalStatement += `\n\n The target subgraph check has failed because of graph pruning issues.`; - } - success = false; - } + if (resp.isLinkedTrafficCheckFailed || resp.isLinkedPruningCheckFailed) { + const prefix = success ? '\n\nBut ' : '\n\n'; + finalStatement += `${prefix}this schema change is linked to a target subgraph and its check failed.`; + if (resp.isLinkedTrafficCheckFailed) { + finalStatement += '\n\nReason: client traffic impact on the target subgraph.'; + } + if (resp.isLinkedPruningCheckFailed) { + finalStatement += '\n\nReason: graph pruning issues on the target subgraph.'; + } + success = false; + }proto/wg/cosmo/platform/v1/platform.proto (4)
448-458: Clarify what the nested LinkedSubgraph representsTiny doc helps consumers understand this is the target subgraph the current subgraph links to.
Apply:
- message LinkedSubgraph{ + // Target subgraph this subgraph is linked to. + message LinkedSubgraph{ string id = 1; string name = 2; string namespace = 3; }
516-527: Document LinkedCheck fields for generated-client clarityAdd brief comments so SDKs and UIs can surface these semantics accurately.
Apply:
- message LinkedCheck { - string id = 1; - repeated string affectedGraphNames = 2; - bool isCheckSuccessful = 3; - bool hasClientTraffic = 4; - bool hasGraphPruningErrors = 5; - bool clientTrafficCheckSkipped = 6; - bool graphPruningCheckSkipped = 7; - repeated string subgraphNames = 8; - string namespace = 9; - bool isForcedSuccess = 10; - } + message LinkedCheck { + // ID of the linked target check. + string id = 1; + // Names of federated graphs affected by the linked check. + repeated string affectedGraphNames = 2; + // True if the linked check as a whole succeeded. + bool isCheckSuccessful = 3; + // True if client traffic was detected on impacted operations in the linked target. + bool hasClientTraffic = 4; + // True if graph pruning errors were detected in the linked target. + bool hasGraphPruningErrors = 5; + // True if client traffic check was skipped for the linked target. + bool clientTrafficCheckSkipped = 6; + // True if graph pruning check was skipped for the linked target. + bool graphPruningCheckSkipped = 7; + // Names of source subgraphs whose changes initiated this linked check. + repeated string subgraphNames = 8; + // Namespace of the linked target subgraph. + string namespace = 9; + // True if the linked check was force-marked successful by a user override. + bool isForcedSuccess = 10; + }
550-551: Annotate linkedChecks fieldOne-liner doc improves discoverability in generated types.
Apply:
- repeated LinkedCheck linkedChecks = 22; + // Per-target results when this check triggers linked target checks. + repeated LinkedCheck linkedChecks = 22;
2897-2915: Add brief request docs to link/unlink messagesTiny comments make intent unambiguous for callers.
Apply:
-message LinkSubgraphRequest { - string sourceSubgraphName = 1; - string sourceSubgraphNamespace = 2; - string targetSubgraphName = 3; - string targetSubgraphNamespace = 4; -} +message LinkSubgraphRequest { + // Source subgraph (the one being linked from). + string sourceSubgraphName = 1; + string sourceSubgraphNamespace = 2; + // Target subgraph (the one being linked to). + string targetSubgraphName = 3; + string targetSubgraphNamespace = 4; +} @@ -message UnlinkSubgraphRequest { - string sourceSubgraphName = 1; - string sourceSubgraphNamespace = 2; -} +message UnlinkSubgraphRequest { + // Source subgraph to unlink (the link target is implied by the current link). + string sourceSubgraphName = 1; + string sourceSubgraphNamespace = 2; +}
📜 Review details
Configuration used: CodeRabbit 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 (9)
cli/src/handle-proposal-result.ts(1 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(15 hunks)controlplane/src/core/bufservices/proposal/createProposal.ts(2 hunks)controlplane/src/core/bufservices/proposal/getProposal.ts(1 hunks)controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts(1 hunks)controlplane/src/core/bufservices/proposal/updateProposal.ts(2 hunks)controlplane/src/core/repositories/ProposalRepository.ts(6 hunks)proto/wg/cosmo/platform/v1/platform.proto(9 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/bufservices/proposal/createProposal.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/proposal/getProposal.tscontrolplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.tscontrolplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/bufservices/proposal/getProposal.tscontrolplane/src/core/bufservices/proposal/updateProposal.tscontrolplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
controlplane/src/core/bufservices/proposal/updateProposal.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
cli/src/handle-proposal-result.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx
📚 Learning: 2025-09-10T09:59:17.220Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:59:17.220Z
Learning: When skipTrafficCheck is true in schema checks, the traffic inspection loop is completely bypassed (as shown in line 2040 of SubgraphRepository.ts with the continue statement), which means hasClientTraffic will be false. The traffic analysis doesn't run at all when skipTrafficCheck is true, unlike the previous understanding that it always ran to collect usage data.
Applied to files:
connect/src/wg/cosmo/platform/v1/platform_pb.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: In the SubgraphRepository class, both byId() and byName() methods are equally valid for fetching subgraph data since they both use the same underlying getSubgraph() helper method. When getLinkedSubgraph() returns targetSubgraphName and targetSubgraphNamespace, using byName() is the appropriate choice rather than byId().
Applied to files:
proto/wg/cosmo/platform/v1/platform.proto
🧬 Code graph analysis (2)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (21)
GetSubgraphByNameResponse_LinkedSubgraph(27726-27734)GetSubgraphByNameResponse_LinkedSubgraph(27749-27749)GetSubgraphByNameResponse_LinkedSubgraph(27764-27766)SchemaCheck_LinkedCheck(27941-27956)SchemaCheck_LinkedCheck(27971-27971)SchemaCheck_LinkedCheck(27986-27988)LinkSubgraphRequest(27451-27460)LinkSubgraphRequest(27475-27475)LinkSubgraphRequest(27490-27492)LinkSubgraphResponse(27522-27528)LinkSubgraphResponse(27543-27543)LinkSubgraphResponse(27558-27560)Response(712-720)Response(735-735)Response(750-752)UnlinkSubgraphRequest(27569-27576)UnlinkSubgraphRequest(27591-27591)UnlinkSubgraphRequest(27606-27608)UnlinkSubgraphResponse(27624-27630)UnlinkSubgraphResponse(27645-27645)UnlinkSubgraphResponse(27660-27662)
controlplane/src/core/repositories/ProposalRepository.ts (2)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(61-1281)controlplane/src/core/util.ts (1)
isCheckSuccessful(568-603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
🔇 Additional comments (25)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
564-574: LGTM: linked-checks correctly influence overall successPassing the derived linked-check failure booleans into
isCheckSuccessfulmatches the early-exit semantics and should cause checks to fail when any linked check reports usage or pruning errors.controlplane/src/core/bufservices/proposal/updateProposal.ts (2)
443-445: Good propagation of linked-check failure flags.Surfacing
isLinkedTrafficCheckFailedandisLinkedPruningCheckFailedfromcheckMultipleSchemasis correct and aligns with the end-to-end success semantics.
447-447: checkMultipleSchemas signature and call sites are correct.
The repository method now acceptsorganizationSlug(SchemaCheckRepository.ts 601–604), and bothupdateProposal.tsandcreateProposal.tspassorganizationSlugconsistently.controlplane/src/core/bufservices/proposal/getProposal.ts (1)
52-52: The scripts will dump the implementation ofgetLatestCheckForProposalwith surrounding lines, allowing us to verify if it filters byorganizationId.controlplane/src/core/bufservices/proposal/getProposalsByFederatedGraph.ts (1)
121-124: Approve org-scoped latest-check retrieval
Matches the implementation in getProposal.ts; no further changes needed.controlplane/src/core/repositories/ProposalRepository.ts (3)
8-8: Import addition looks goodImporting isCheckSuccessful is needed for the new success calculation; no issues spotted.
690-690: Including linkedChecks in response — LGTMSurface area matches the new DTO; this unblocks Studio/CLI to render linked-check details.
512-514: Resolved: All call sites updated to includeorganizationIdforgetLatestCheckForProposalproto/wg/cosmo/platform/v1/platform.proto (3)
281-284: LGTM: linked-failure flags with docsThe two booleans and comments clearly convey intent and are additive-safe.
576-581: LGTM: affected-graph flags are additive and usefulGood additions for UI summarization.
3284-3287: LGTM: new RPCsService surface matches the new messages; comments are clear.
connect/src/wg/cosmo/platform/v1/platform_pb.ts (14)
2268-2269: Field numbers and presence look correct.Numbers 15/16 align with the Go bindings and use proto3 optional; no collisions observed.
3505-3506: Descriptor entry for linkedSubgraph is consistent.opt: true is correct for conditional presence.
3525-3572: LinkedSubgraph payload LGTM.Minimal, cross-language parity (id, name, namespace) matches connect-go.
4017-4018: Descriptor for linkedChecks is correct.Repeated message wiring matches the new type.
4157-4246: SchemaCheck.LinkedCheck shape aligns with Go bindings.Booleans defaulting to false here is fine; presence is not required for these.
4528-4551: Additional summary booleans look good.Default-false fits UI/CLI expectations and is backward-compatible for unknown fields.
4565-4569: Descriptor entries match added fields.Numbers 5–9 are contiguous and consistent.
21411-21412: Descriptor entries OK.Optional flags wired with unique field numbers (18/19).
22002-22003: Descriptor entries OK.Numbers 15/16 consistent with other messages.
22748-22767: LinkSubgraphResponse wiring is fine.Carries Response; no additional fields needed.
22828-22863: UnlinkSubgraphResponse wiring is fine.Mirrors LinkSubgraphResponse.
3489-3493: linkedSubgraph already uses optional semantics and is omitted from JSON when unset
3983-3986: Confirm skip semantics are surfaced in linkedChecks
Verify that the generated SchemaCheck_LinkedCheck class in connect/src/wg/cosmo/platform/v1/platform_pb.ts exposes both clientTrafficCheckSkipped and graphPruningCheckSkipped so the global skipTrafficCheck/graphPruningSkip flags flow through.
22788-22826: Resolved: Omitting target is safe due to unique constraint on sourceSubgraphId
The linked_subgraphs schema enforces a unique(source_subgraph_id) constraint, ensuring each source can link to at most one target. No optional target parameter is needed.
- Modified the logic for determining `isLinkedTrafficCheckFailed` and `isLinkedPruningCheckFailed` across multiple files to include a check for `!linkedCheck.isForcedSuccess`. - Ensured consistent handling of linked checks in `getCheckSummary`, `ProposalRepository`, and various components in the studio pages.
- Reformatted the logic for `isLinkedTrafficCheckFailed` and `isLinkedPruningCheckFailed` in `getCheckSummary` and `ProposalRepository` for improved readability. - Ensured consistent code style across the files while maintaining the existing functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (4)
418-426: Fix grammar in user-facing toasts.“Could not marked” should be “Could not mark”.
Apply:
- description: `Could not marked check as successful. ${data.response?.details}`, + description: `Could not mark check as successful. ${data.response?.details}`, ... - toast({ - description: "Could not marked check as successful. Please try again", + toast({ + description: "Could not mark check as successful. Please try again",
1511-1543: Force Success action shows when already forced.Offer the action only when NOT forced and the check isn’t successful.
Apply:
- {data.check.isForcedSuccess ? ( + {!data.check.isForcedSuccess && !isSuccessful ? ( <div className="mt-2 flex space-x-2"> <ForceSuccess onSubmit={() => forceSuccess({ checkId: id, graphName: slug, namespace, }) } /> </div> - ) : null} + ) : null}
1556-1566: Remove duplicate Force Success CTA on success block (and missing namespace).Showing Force Success when the check passed is confusing; also this call omits namespace.
Apply:
- {data.check.isForcedSuccess ? ( - <div className="mt-2 flex space-x-2"> - <ForceSuccess - onSubmit={() => - forceSuccess({ - checkId: id, - graphName: slug, - }) - } - /> - </div> - ) : null} + {/* Force Success is only shown in the failure alert above */}
1451-1463: Typo: “wanings” → “warnings”.User-facing string.
Apply:
- : "No composition wanings"} + : "No composition warnings"}
♻️ Duplicate comments (4)
controlplane/src/core/repositories/ProposalRepository.ts (1)
562-572: isCheckSuccessful call wired correctly; proposal-match intentionally ignored here.
Setting hasProposalMatchError: false is appropriate for this method’s lifecycle (create/update-only), as previously discussed.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (3)
513-516: Overview tab query param removal is dead code.You never call setTab('overview'); the Link below adds tab=overview, so delete in setTab is ineffective. Apply Option A or B.
Option A:
- if (tab === "overview") { - delete query.tab; - } + if (tab === "overview") delete query.tab;And change the trigger (see below at Lines 912–926) to call setTab("overview") instead of Link.
912-926: Make Overview tab navigation consistent.Use setTab('overview') or omit the tab param entirely; current Link always adds tab=overview.
Apply Option A (state-driven):
-<TabsTrigger - value="overview" - className="flex items-center gap-x-2" - asChild -> - <Link href={{ query: { ...router.query, tab: "overview" } }}> - <CheckCircleIcon className="h-4 w-4 flex-shrink-0" /> - Overview - </Link> -</TabsTrigger> +<TabsTrigger + value="overview" + className="flex items-center gap-x-2" + onClick={() => setTab("overview")} +> + <CheckCircleIcon className="h-4 w-4 flex-shrink-0" /> + Overview +</TabsTrigger>Option B (URL-driven): keep Link but drop “tab” key from query.
1156-1179: Don’t use current check’s forced flag for other graphs’ badges.This mislabels their status. Pass false (or use a per-graph flag when available).
Apply:
- {getCheckBadge( - isCheckSuccessful, - data.check?.isForcedSuccess || false, - )} + {getCheckBadge( + isCheckSuccessful, + false, + )}
🧹 Nitpick comments (10)
controlplane/src/core/repositories/ProposalRepository.ts (2)
554-561: Linked-check failure predicates are scoped and respect forced-success — good.
- Using organizationId here aligns with tenant scoping in SchemaCheckRepository.
- Not checking the “skipped” flags is fine given has* will be false when skipped.
Minor naming consistency: consider schemaCheckId (camel-cased “Id”) across repos for symmetry with other “...CheckId” params.
659-663: Avoid N+1 linked-check queries (batch by check IDs).
Current code calls getLinkedSchemaChecks per check. Prefer a single batched lookup returning a Map<checkId, linkedChecks[]>.Apply within this block:
- const linkedChecks = await schemaCheckRepo.getLinkedSchemaChecks({ - schemaCheckID: c.id, - organizationId, - }); + const linkedChecks = linkedChecksById.get(c.id) ?? [];Add above the Promise.all (outside this hunk):
// Prefetch linked checks in one round-trip const linkedChecksById = await schemaCheckRepo.getLinkedSchemaChecksByIds({ schemaCheckIDs: checksList.map((x) => x.id), organizationId, });And implement in SchemaCheckRepository (outline):
public async getLinkedSchemaChecksByIds({ schemaCheckIDs, organizationId, }: { schemaCheckIDs: string[]; organizationId: string }) { // 1) fetch rows from linkedSchemaChecks where schemaCheckId IN schemaCheckIDs // 2) fetch all target checks with a single findMany + inArray // 3) apply the same org filter + projection as getLinkedSchemaChecks // 4) return new Map(checkId => linkedChecks[]) }Happy to draft the repository method if you want it in this PR.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (1)
572-581: Avoid double traversal of linkedChecks.Minor: .some() is called twice; compute once for clarity/perf and pass the booleans.
Apply:
- linkedChecks.some( - (linkedCheck) => - linkedCheck.hasClientTraffic && - !linkedCheck.isForcedSuccess, - ), - linkedChecks.some( - (linkedCheck) => - linkedCheck.hasGraphPruningErrors && - !linkedCheck.isForcedSuccess, - ), + isLinkedTrafficCheckFailed, + isLinkedPruningCheckFailed,And just above:
+ const isLinkedTrafficCheckFailed = linkedChecks.some( + (lc) => lc.hasClientTraffic && !lc.isForcedSuccess, + ); + const isLinkedPruningCheckFailed = linkedChecks.some( + (lc) => lc.hasGraphPruningErrors && !lc.isForcedSuccess, + );studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx (1)
189-200: Centralize linked-check failure derivation.You repeat this pattern across pages; extract a tiny helper to keep logic consistent.
Example:
- const isLinkedTrafficCheckFailed = linkedChecks.some( - (linkedCheck) => - linkedCheck.hasClientTraffic && - !linkedCheck.isForcedSuccess, - ); - const isLinkedPruningCheckFailed = linkedChecks.some( - (linkedCheck) => - linkedCheck.hasGraphPruningErrors && - !linkedCheck.isForcedSuccess, - ); + const isLinkedTrafficCheckFailed = linkedChecks.some( + (lc) => lc.hasClientTraffic && !lc.isForcedSuccess, + ); + const isLinkedPruningCheckFailed = linkedChecks.some( + (lc) => lc.hasGraphPruningErrors && !lc.isForcedSuccess, + );Optionally move into a shared util:
export const summarizeLinkedChecks = (linkedChecks: {hasClientTraffic:boolean; hasGraphPruningErrors:boolean; isForcedSuccess:boolean;}[]) => ({ traffic: linkedChecks.some(lc => lc.hasClientTraffic && !lc.isForcedSuccess), pruning: linkedChecks.some(lc => lc.hasGraphPruningErrors && !lc.isForcedSuccess), });studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (1)
457-465: Compute linked failures once.Same nit as other pages.
Apply:
-const isLinkedTrafficCheckFailed = data.check.linkedChecks.some( - (linkedCheck) => - linkedCheck.hasClientTraffic && !linkedCheck.isForcedSuccess, -); -const isLinkedPruningCheckFailed = data.check.linkedChecks.some( - (linkedCheck) => - linkedCheck.hasGraphPruningErrors && !linkedCheck.isForcedSuccess, -); +const isLinkedTrafficCheckFailed = data.check.linkedChecks.some( + (lc) => lc.hasClientTraffic && !lc.isForcedSuccess, +); +const isLinkedPruningCheckFailed = data.check.linkedChecks.some( + (lc) => lc.hasGraphPruningErrors && !lc.isForcedSuccess, +);controlplane/src/core/bufservices/check/getCheckSummary.ts (5)
127-146: Good guard; return early when the check isn’t for this graph.Minor UX: include graph + check identifiers to aid debugging.
- details: 'Check not found for the current graph', + details: `Check '${req.checkId}' not found for graph '${req.namespace}/${req.graphName}'`,
161-163: Correct semantics for linked-check failures.Using hasClientTraffic/hasGraphPruningErrors gated by !isForcedSuccess matches domain behavior. Optional null-safety for robustness:
- const isLinkedTrafficCheckFailed = check.linkedChecks.some((linkedCheck) => linkedCheck.hasClientTraffic && !linkedCheck.isForcedSuccess); - const isLinkedPruningCheckFailed = check.linkedChecks.some((linkedCheck) => linkedCheck.hasGraphPruningErrors && !linkedCheck.isForcedSuccess); + const linkedChecks = check.linkedChecks ?? []; + const isLinkedTrafficCheckFailed = linkedChecks.some((lc) => lc.hasClientTraffic && !lc.isForcedSuccess); + const isLinkedPruningCheckFailed = linkedChecks.some((lc) => lc.hasGraphPruningErrors && !lc.isForcedSuccess);
150-159: Avoid redundant affected-operations queries; compute once.getAffectedOperationsByCheckId is check-scoped, not per-graph; calling it inside the loop is unnecessary work. Compute once and reuse.
- const hasLintErrors = lintIssues.some((issue) => issue.severity === LintSeverity.error); - let hasAffectedOperations = false; - if (check.hasClientTraffic) { - const affectedOperations = await schemaCheckRepo.getAffectedOperationsByCheckId({ - checkId: req.checkId, - limit: 1, - offset: 0, - }); - hasAffectedOperations = affectedOperations.length > 0; - } + const hasLintErrors = lintIssues.some((issue) => issue.severity === LintSeverity.error); + const hasAffectedOperations = + check.hasClientTraffic + ? (await schemaCheckRepo.getAffectedOperationsByCheckId({ + checkId: req.checkId, + limit: 1, + offset: 0, + })).length > 0 + : false; @@ - if (check.hasClientTraffic) { - const affectedOperations = await schemaCheckRepo.getAffectedOperationsByCheckId({ - checkId: req.checkId, - limit: 1, - offset: 0, - }); - hasAffectedOperations = affectedOperations.length > 0; - } + // hasAffectedOperations is check-scoped; no need to recompute per graphAlso applies to: 205-212
176-184: Nice per-graph status enrichment.Tiny cleanup: factor repeated expressions for readability.
- isComposable: checkDetails.compositionErrors.length === 0, - isBreaking: checkDetails.changes.some((change) => change.isBreaking), + // local aliases improve readability and keep parity with the loop below + isComposable: checkDetails.compositionErrors.length === 0, + isBreaking: checkDetails.changes.some((c) => c.isBreaking),
225-233: Consistent propagation to other affected graphs.Same nit as above: consider local consts for isComposable/isBreaking/hasGraphPruningErrors to mirror the current-graph block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/src/core/bufservices/check/getCheckSummary.ts(4 hunks)controlplane/src/core/repositories/ProposalRepository.ts(6 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx(8 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx(2 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
📚 Learning: 2025-09-10T11:15:52.119Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/ProposalRepository.ts:562-572
Timestamp: 2025-09-10T11:15:52.119Z
Learning: The getLatestCheckForProposal function in controlplane/src/core/repositories/ProposalRepository.ts is only called during proposal creation or updates, so proposal match error checking (hasProposalMatchError) is not needed since the proposal is being modified itself rather than being matched against.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.tscontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-09-10T11:31:32.456Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/ProposalRepository.ts:0-0
Timestamp: 2025-09-10T11:31:32.456Z
Learning: When traffic checks or pruning checks are skipped in schema validation, the corresponding hasClientTraffic and hasGraphPruningErrors flags are automatically set to false by default. This means that checking clientTrafficCheckSkipped or graphPruningCheckSkipped flags in linked check failure conditions is redundant, as skipped checks will naturally have false values for their respective has* flags.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-09-10T09:59:17.220Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:59:17.220Z
Learning: When skipTrafficCheck is true in schema checks, the traffic inspection loop is completely bypassed (as shown in line 2040 of SubgraphRepository.ts with the continue statement), which means hasClientTraffic will be false. The traffic analysis doesn't run at all when skipTrafficCheck is true, unlike the previous understanding that it always ran to collect usage data.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-09-10T09:53:42.675Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.675Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx
📚 Learning: 2025-09-10T09:52:46.305Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:52:46.305Z
Learning: When skipTrafficCheck is true in schema checks, traffic analysis still runs to collect usage data, but hasClientTraffic can still be true if unsafe client traffic is detected. The difference is that traffic issues don't cause the overall check to fail, but the hasClientTraffic flag itself remains accurate based on the actual traffic analysis results.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsxcontrolplane/src/core/bufservices/check/getCheckSummary.ts
📚 Learning: 2025-08-28T13:07:08.273Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:139-141
Timestamp: 2025-08-28T13:07:08.273Z
Learning: In the Cosmo schema check system, `hasClientTraffic` is a boolean flag that indicates unsafe/problematic client traffic that would be negatively impacted by schema changes. It represents a failure condition, not just the presence of traffic. When `hasClientTraffic` is true, it means there are operations that would be affected by breaking changes, making it appropriate to use directly as a failure indicator.
Applied to files:
controlplane/src/core/bufservices/check/getCheckSummary.ts
🧬 Code graph analysis (3)
controlplane/src/core/repositories/ProposalRepository.ts (2)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(61-1281)controlplane/src/core/util.ts (1)
isCheckSuccessful(568-603)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (5)
studio/src/components/info-tooltip.tsx (1)
InfoTooltip(16-37)studio/src/components/ui/table.tsx (7)
TableWrapper(127-127)Table(126-126)TableHeader(128-128)TableRow(132-132)TableHead(131-131)TableBody(129-129)TableCell(133-133)studio/src/components/check-badge-icon.tsx (3)
getCheckBadge(59-59)isCheckSuccessful(59-59)getCheckIcon(59-59)controlplane/src/core/util.ts (1)
isCheckSuccessful(568-603)studio/src/lib/utils.ts (1)
cn(6-8)
controlplane/src/core/bufservices/check/getCheckSummary.ts (5)
connect-go/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)router/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)graphqlmetrics/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)controlplane/src/core/repositories/SubgraphRepository.ts (1)
checkDetails(1303-1359)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
LintSeverity(27-27)LintSeverity(56-58)LintSeverity(60-62)LintSeverity(69-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
controlplane/src/core/repositories/ProposalRepository.ts (4)
8-8: Importing isCheckSuccessful here is correct and consistent with usage.
583-592: Extending getChecksByProposalId signature with organizationId is appropriate.
690-691: Including linkedChecks in the response payload is aligned with UI/consumer needs.
511-514: Approve code changes: all call sites now pass the neworganizationIdargument.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/proposals/[proposalId]/index.tsx (2)
19-20: LGTM on trigger import/use.No issues spotted with DropdownMenuTrigger usage.
562-563: Good: safely consuming linkedChecks from API.Given repeated fields default to [], direct destructuring is fine.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx (1)
209-211: LGTM: linked failures fed into isCheckSuccessful.Matches the early-exit semantics in isCheckSuccessful.
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx (5)
40-48: LGTM: table components import.Consistent with usage below.
474-476: LGTM: linked failures influence overall success.Early-exit behavior preserved.
944-946: LGTM: badge shows errors + warnings.Matches the UI intent.
1320-1417: LGTM: Linked Checks section reads well and matches semantics.Status, routing, and skip handling look correct.
1060-1111: LGTM: Alert explains linked-check failure causes clearly.Good UX for mixed/linked failure scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
554-576: Linked-check gating logic looks good; respects forced successUsing
.some(l => l.hasClientTraffic && !l.isForcedSuccess)and the analogous pruning predicate aligns with the established behavior that skipped checks sethas*to false by default. Passing these intoisCheckSuccessfulfor early-fail is consistent with util semantics.
🧹 Nitpick comments (1)
controlplane/src/core/repositories/ProposalRepository.ts (1)
663-667: Potential N+1 on linked checks — consider a batched repository methodFetching
getLinkedSchemaChecksper check can be expensive for long histories. Consider adding a bulk method that accepts multiple check IDs and returns a map.Example shape:
-const linkedChecks = await schemaCheckRepo.getLinkedSchemaChecks({ schemaCheckID: c.id, organizationId }); +// New bulk API (pseudo): +// const linkedChecksById = await schemaCheckRepo.getLinkedSchemaChecksForCheckIds({ checkIds, organizationId }); +// const linkedChecks = linkedChecksById.get(c.id) ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/core/bufservices/check/getCheckSummary.ts(4 hunks)controlplane/src/core/repositories/ProposalRepository.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/bufservices/check/getCheckSummary.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
📚 Learning: 2025-09-10T11:15:52.119Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/ProposalRepository.ts:562-572
Timestamp: 2025-09-10T11:15:52.119Z
Learning: The getLatestCheckForProposal function in controlplane/src/core/repositories/ProposalRepository.ts is only called during proposal creation or updates, so proposal match error checking (hasProposalMatchError) is not needed since the proposal is being modified itself rather than being matched against.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T11:31:32.456Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/ProposalRepository.ts:0-0
Timestamp: 2025-09-10T11:31:32.456Z
Learning: When traffic checks or pruning checks are skipped in schema validation, the corresponding hasClientTraffic and hasGraphPruningErrors flags are automatically set to false by default. This means that checking clientTrafficCheckSkipped or graphPruningCheckSkipped flags in linked check failure conditions is redundant, as skipped checks will naturally have false values for their respective has* flags.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T09:59:17.220Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:59:17.220Z
Learning: When skipTrafficCheck is true in schema checks, the traffic inspection loop is completely bypassed (as shown in line 2040 of SubgraphRepository.ts with the continue statement), which means hasClientTraffic will be false. The traffic analysis doesn't run at all when skipTrafficCheck is true, unlike the previous understanding that it always ran to collect usage data.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-10T09:52:46.305Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:52:46.305Z
Learning: When skipTrafficCheck is true in schema checks, traffic analysis still runs to collect usage data, but hasClientTraffic can still be true if unsafe client traffic is detected. The difference is that traffic issues don't cause the overall check to fail, but the hasClientTraffic flag itself remains accurate based on the actual traffic analysis results.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-08-28T13:07:08.273Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:139-141
Timestamp: 2025-08-28T13:07:08.273Z
Learning: In the Cosmo schema check system, `hasClientTraffic` is a boolean flag that indicates unsafe/problematic client traffic that would be negatively impacted by schema changes. It represents a failure condition, not just the presence of traffic. When `hasClientTraffic` is true, it means there are operations that would be affected by breaking changes, making it appropriate to use directly as a failure indicator.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/core/repositories/ProposalRepository.ts
🧬 Code graph analysis (1)
controlplane/src/core/repositories/ProposalRepository.ts (2)
controlplane/src/core/repositories/SchemaCheckRepository.ts (1)
SchemaCheckRepository(61-1281)controlplane/src/core/util.ts (1)
isCheckSuccessful(568-603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
controlplane/src/core/repositories/ProposalRepository.ts (3)
8-8: Import addition is correct
isCheckSuccessfulis used below;normalizeLabelsis already referenced. No issues.
511-514: AllgetLatestCheckForProposalcalls includeorganizationId
Both invocations ingetProposal.tsandgetProposalsByFederatedGraph.tsnow pass the neworganizationIdargument.
694-695: No action needed:linkedChecksDTO and consumers are in sync —LinkedCheckDTOis declared in controlplane/src/types/index.ts (line 219), and multiple Studio pages already consumelinkedCheckswithout errors.
JivusAyrus
deleted the
suvij/eng-7794-create-subgraph-linking-on-cosmo
branch
2 participants
Summary by CodeRabbit
New Features
Checks
UI
CLI
Backend
Tests
Checklist
COMPLETES ENG-7909
COMPLETES ENG-7908