Merged
Conversation
Aenimus
requested review from
JivusAyrus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds RecomposeGraph protobuf/RPC and generated client bindings; implements server-side recomposeGraph handler and wires it into PlatformService; adds CLI recompose command for federated and monograph graphs with result/truncation handling; introduces truncation warning utility and updates composition-result logic; and adds many tests and test helpers, plus broad test harness cleanup changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2652 +/- ##
===========================================
- Coverage 64.21% 39.91% -24.31%
===========================================
Files 304 974 +670
Lines 43042 122288 +79246
Branches 4604 5514 +910
===========================================
+ Hits 27640 48811 +21171
- Misses 15380 71816 +56436
- Partials 22 1661 +1639
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
cli/src/utils.ts (1)
300-305: Use an interface and an explicitvoidreturn type for the new helper signature.This keeps the new helper aligned with the repo’s TypeScript conventions.
♻️ Suggested refactor
-type PrintTruncationWarningParams = { +interface PrintTruncationWarningParams { displayedErrorCounts: SubgraphPublishStats; totalErrorCounts?: SubgraphPublishStats; -}; +} -export function printTruncationWarning({ displayedErrorCounts, totalErrorCounts }: PrintTruncationWarningParams) { +export function printTruncationWarning( + { displayedErrorCounts, totalErrorCounts }: PrintTruncationWarningParams, +): void {As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript" and "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/utils.ts` around lines 300 - 305, Replace the type alias PrintTruncationWarningParams with an interface of the same shape (including displayedErrorCounts: SubgraphPublishStats and optional totalErrorCounts?: SubgraphPublishStats) and update the printTruncationWarning signature to accept that interface and declare an explicit return type of void; keep parameter names the same so changes are limited to replacing the type alias with an interface and adding ": void" to the function declaration.cli/src/commands/graph/common/recompose.ts (2)
48-59: Consider adding error handling for network failures.The
recomposeGraphAPI call is not wrapped in a try-catch block. If a network error occurs, the spinner will remain active and the error will propagate unhandled, potentially leaving the user without clear feedback.♻️ Proposed improvement
+ let resp; + try { - const resp = await opts.client.platform.recomposeGraph( + resp = await opts.client.platform.recomposeGraph( { disableResolvabilityValidation: options.disableResolvabilityValidation, isMonograph: opts.isMonograph ?? false, limit, name, namespace: options.namespace, }, { headers: getBaseHeaders(), }, ); + } catch (e) { + spinner.fail(`Failed to recompose ${graphType} "${pc.bold(name)}".`); + if (e instanceof Error) { + program.error(pc.red(e.message)); + } + throw e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/graph/common/recompose.ts` around lines 48 - 59, Wrap the call to opts.client.platform.recomposeGraph in a try/catch around the existing invocation (the call that passes disableResolvabilityValidation, isMonograph, limit, name, namespace and getBaseHeaders) so network/runtime errors are caught; in the catch ensure the CLI spinner used around the operation is stopped/failed (call spinner.stop()/spinner.fail() or equivalent), log a clear error message including the caught error details, and then rethrow or exit with a non-zero code so the process does not hang with an active spinner.
99-101: Empty catch block silently swallows errors.The catch block only sets the exit code but discards the error itself. If
handleCompositionResultthrows for an unexpected reason, debugging will be difficult. Consider logging the error or re-throwing after setting the exit code.♻️ Proposed improvement
} catch (e) { + // handleCompositionResult throws to signal failure process.exitCode = 1; }Adding a comment clarifies that the throw is intentional for signaling failure, or alternatively log the error if unexpected errors should be visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/graph/common/recompose.ts` around lines 99 - 101, The catch block around handleCompositionResult currently swallows errors; update it to capture the thrown error (e.g., catch (err)), set process.exitCode = 1 as before, and either log the error with context (e.g., console.error or the module logger with a message like "Error in handleCompositionResult") or re-throw the error after setting the exit code so unexpected failures are visible; locate this in recompose.ts around the handleCompositionResult call and replace the empty catch with the described behavior.cli/test/graph/federated-graph/recompose.test.ts (1)
2-9: Remove unused imports.Several imports are not used in this test file:
Command,PartialMessage,createPromiseClient,createRouterTransport,PlatformService,RecomposeGraphResponse,Client, andRecomposeCommand. These appear to be remnants from earlier development.♻️ Proposed cleanup
import { afterEach, beforeEach, describe, expect, test, vi, type MockInstance } from 'vitest'; -import { Command } from 'commander'; -import { type PartialMessage } from '@bufbuild/protobuf'; -import { createPromiseClient, createRouterTransport } from '@connectrpc/connect'; -import { PlatformService } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_connect'; -import { RecomposeGraphResponse } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_pb'; import { EnumStatusCode } from '@wundergraph/cosmo-connect/dist/common/common_pb'; -import { Client } from '../../../src/core/client/client.js'; -import RecomposeCommand from '../../../src/commands/graph/common/recompose.js'; import { runRecompose } from '../utils.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/graph/federated-graph/recompose.test.ts` around lines 2 - 9, Remove the unused imports listed in the test file: eliminate Command, PartialMessage, createPromiseClient, createRouterTransport, PlatformService, RecomposeGraphResponse, EnumStatusCode (if unused), Client, and RecomposeCommand from the import block so only imports actually referenced in recompose.test.ts remain; update the import statement accordingly (e.g., keep only the modules used by the test) and run the linter/tests to ensure no remaining references to those symbols (search for RecomposeCommand, Client, PlatformService, RecomposeGraphResponse, createPromiseClient, createRouterTransport, PartialMessage, Command to confirm removal).controlplane/src/core/bufservices/graph/recomposeGraph.ts (1)
86-87: Non-null assertion onchClientcould be unsafe.Using
opts.chClient!assumes ClickHouse client is always available. If it's undefined at runtime, this will cause unexpected behavior. Consider adding a guard or documenting why it's guaranteed to be present.♻️ Proposed improvement
+ if (!opts.chClient) { + throw new Error('ClickHouse client is required for graph composition'); + } + const { compositionErrors, compositionWarnings, deploymentErrors } = await federatedGraphRepo.composeAndDeployGraphs({ actorId: authContext.userId, admissionConfig: { cdnBaseUrl: opts.cdnBaseUrl, webhookJWTSecret: opts.admissionWebhookJWTSecret, }, blobStorage: opts.blobStorage, - chClient: opts.chClient!, + chClient: opts.chClient,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts` around lines 86 - 87, The code uses a non-null assertion on opts.chClient when setting the chClient property (chClient: opts.chClient!) which can crash if undefined; remove the bang and instead add a guard in recomposeGraph (or the surrounding initializer) that either throws a clear error (e.g., throw new Error("chClient required")) if chClient is mandatory or allows chClient to be undefined and adjust downstream logic; update any callers or consumers expecting a non-null chClient to handle the undefined case or rely on the new explicit error so the absence is detected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/handle-composition-result.ts`:
- Around line 161-169: The truncation counts passed to printTruncationWarning
are using compositionErrors instead of deployment errors in the
deployment-failure (fail-fast) branch; update the SubgraphPublishStats
construction so compositionErrors is 0 and deploymentErrors is set from the
actual deployment error count (e.g., use the deploymentErrors array/variable
length) while still passing totalErrorCounts and displayedErrorCounts to
printTruncationWarning (identify and modify the callsite that constructs new
SubgraphPublishStats in handle-composition-result.ts).
In `@cli/test/graph/federated-graph/recompose.test.ts`:
- Around line 108-120: The test description string for the test case using
runRecompose is incorrect: it mentions `--fail-on-composition-error` but the
test actually sets the `failOnAdmissionWebhookError` option; update the test
name string (the first argument to test, e.g., the description in the test
starting with "that recompose fails...") to accurately reflect that the case
verifies deployment errors cause exit code 1 when `failOnAdmissionWebhookError`
is true, keeping the existing call to runRecompose and the assertions unchanged.
In `@cli/test/graph/monograph/recompose.test.ts`:
- Around line 60-71: The test "that recompose fails but does not return exit
code 1 if the graph is not found" calls runRecompose without specifying
isMonograph, causing it to default to a federated graph; update the runRecompose
invocation in this test to include the option isMonograph: true so the mocked
response ("Monograph not found") matches the monograph code path (locate the
runRecompose call in this test and add isMonograph: true to its options).
In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts`:
- Around line 105-118: The auditAction and auditableType conditions are inverted
in the auditLogRepo.addAuditLog call: use req.isMonograph to select the
monograph values and the negation to select federated_graph values. Update the
auditAction and auditableType assignments so that when req.isMonograph is true
you set auditAction: 'monograph.recomposed' and auditableType: 'monograph', and
when false you set auditAction: 'federated_graph.recomposed' and auditableType:
'federated_graph' (modify the object passed to auditLogRepo.addAuditLog where
auditAction and auditableType are set).
- Around line 127-131: The computed counts object (counts) collecting
compositionErrors.length, compositionWarnings.length, and
deploymentErrors.length is unused but should be populated into the
RecomposeGraphResponse's optional errorCounts field; update the function that
builds/returns the RecomposeGraphResponse (where compositionErrors,
compositionWarnings, deploymentErrors are available) to set response.errorCounts
= { compositionErrors: counts.compositionErrors, compositionWarnings:
counts.compositionWarnings, deploymentErrors: counts.deploymentErrors } (or map
counts directly) so clients receive the errorCounts field as defined by
RecomposeGraphResponse in the protobuf.
---
Nitpick comments:
In `@cli/src/commands/graph/common/recompose.ts`:
- Around line 48-59: Wrap the call to opts.client.platform.recomposeGraph in a
try/catch around the existing invocation (the call that passes
disableResolvabilityValidation, isMonograph, limit, name, namespace and
getBaseHeaders) so network/runtime errors are caught; in the catch ensure the
CLI spinner used around the operation is stopped/failed (call
spinner.stop()/spinner.fail() or equivalent), log a clear error message
including the caught error details, and then rethrow or exit with a non-zero
code so the process does not hang with an active spinner.
- Around line 99-101: The catch block around handleCompositionResult currently
swallows errors; update it to capture the thrown error (e.g., catch (err)), set
process.exitCode = 1 as before, and either log the error with context (e.g.,
console.error or the module logger with a message like "Error in
handleCompositionResult") or re-throw the error after setting the exit code so
unexpected failures are visible; locate this in recompose.ts around the
handleCompositionResult call and replace the empty catch with the described
behavior.
In `@cli/src/utils.ts`:
- Around line 300-305: Replace the type alias PrintTruncationWarningParams with
an interface of the same shape (including displayedErrorCounts:
SubgraphPublishStats and optional totalErrorCounts?: SubgraphPublishStats) and
update the printTruncationWarning signature to accept that interface and declare
an explicit return type of void; keep parameter names the same so changes are
limited to replacing the type alias with an interface and adding ": void" to the
function declaration.
In `@cli/test/graph/federated-graph/recompose.test.ts`:
- Around line 2-9: Remove the unused imports listed in the test file: eliminate
Command, PartialMessage, createPromiseClient, createRouterTransport,
PlatformService, RecomposeGraphResponse, EnumStatusCode (if unused), Client, and
RecomposeCommand from the import block so only imports actually referenced in
recompose.test.ts remain; update the import statement accordingly (e.g., keep
only the modules used by the test) and run the linter/tests to ensure no
remaining references to those symbols (search for RecomposeCommand, Client,
PlatformService, RecomposeGraphResponse, createPromiseClient,
createRouterTransport, PartialMessage, Command to confirm removal).
In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts`:
- Around line 86-87: The code uses a non-null assertion on opts.chClient when
setting the chClient property (chClient: opts.chClient!) which can crash if
undefined; remove the bang and instead add a guard in recomposeGraph (or the
surrounding initializer) that either throws a clear error (e.g., throw new
Error("chClient required")) if chClient is mandatory or allows chClient to be
undefined and adjust downstream logic; update any callers or consumers expecting
a non-null chClient to handle the undefined case or rely on the new explicit
error so the absence is detected early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd2efcb6-49d9-4713-bd98-11a623fb8f45
⛔ 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 (15)
cli/src/commands/graph/common/recompose.tscli/src/commands/graph/federated-graph/index.tscli/src/commands/graph/monograph/index.tscli/src/commands/subgraph/commands/publish.tscli/src/handle-composition-result.tscli/src/utils.tscli/test/graph/federated-graph/recompose.test.tscli/test/graph/monograph/recompose.test.tscli/test/graph/utils.tsconnect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/graph/recomposeGraph.tscontrolplane/src/db/models.tsproto/wg/cosmo/platform/v1/platform.proto
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
controlplane/test/federated-graph/recompose.test.ts (1)
41-76: Ensureserver.close()runs even when assertions fail.Line 75 only closes the server on the success path. Wrap the test body in
try/finallyto avoid leaked handles on failures.Proposed refactor
test('that recomposing a published federated graph succeeds and triggers a new composition', async () => { const { client, server } = await SetupTest({ dbname, chClient }); - const namespace = genID('namespace').toLowerCase(); - await createNamespace(client, namespace); - const subgraphName = genID('subgraph'); - const fedGraphName = genID('fedGraph'); - const label = genUniqueLabel('label'); - const subgraphSchemaSDL = 'type Query { hello: String! }'; + try { + const namespace = genID('namespace').toLowerCase(); + await createNamespace(client, namespace); + const subgraphName = genID('subgraph'); + const fedGraphName = genID('fedGraph'); + const label = genUniqueLabel('label'); + const subgraphSchemaSDL = 'type Query { hello: String! }'; - await createThenPublishSubgraph( - client, - subgraphName, - namespace, - subgraphSchemaSDL, - [label], - 'http://localhost:8082', - ); + await createThenPublishSubgraph( + client, + subgraphName, + namespace, + subgraphSchemaSDL, + [label], + 'http://localhost:8082', + ); - await createFederatedGraph(client, fedGraphName, namespace, [joinLabel(label)], 'http://localhost:8080'); - await assertNumberOfCompositions(client, fedGraphName, 1, namespace); + await createFederatedGraph(client, fedGraphName, namespace, [joinLabel(label)], 'http://localhost:8080'); + await assertNumberOfCompositions(client, fedGraphName, 1, namespace); - const response = await client.recomposeGraph({ - name: fedGraphName, - namespace, - isMonograph: false, - }); + const response = await client.recomposeGraph({ + name: fedGraphName, + namespace, + isMonograph: false, + }); - expect(response.response).toBeDefined(); - expect(response.response!.code).toBe(EnumStatusCode.OK); - expect(response.compositionErrors).toHaveLength(0); - expect(response.deploymentErrors).toHaveLength(0); + expect(response.response).toBeDefined(); + expect(response.response!.code).toBe(EnumStatusCode.OK); + expect(response.compositionErrors).toHaveLength(0); + expect(response.deploymentErrors).toHaveLength(0); - await assertNumberOfCompositions(client, fedGraphName, 2, namespace); - - await server.close(); + await assertNumberOfCompositions(client, fedGraphName, 2, namespace); + } finally { + await server.close(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/federated-graph/recompose.test.ts` around lines 41 - 76, The test body in the 'that recomposing a published federated graph succeeds and triggers a new composition' case can leak the server when an assertion fails because server.close() is only called on the success path; wrap the test's runtime logic (everything after const { client, server } = await SetupTest(...) through the assertions and composition checks) in a try/finally and call await server.close() in the finally block so server.close() always runs regardless of failures; locate the code around SetupTest, the subsequent createNamespace/createThenPublishSubgraph/createFederatedGraph/assertNumberOfCompositions calls and the client.recomposeGraph/assert expectations to implement the try/finally.controlplane/test/monograph/recompose.test.ts (1)
41-79: Make server teardown failure-safe withtry/finally.Line 78 closes the server only if all prior assertions pass. Use
try/finallyto guarantee cleanup.Proposed refactor
test('that recomposing a published monograph succeeds and triggers a new composition', async () => { const { client, server } = await SetupTest({ dbname, chClient }); - const namespace = genID('namespace').toLowerCase(); - await createNamespace(client, namespace); - const monographName = genID('monograph'); - const schemaSDL = 'type Query { hello: String! }'; + try { + const namespace = genID('namespace').toLowerCase(); + await createNamespace(client, namespace); + const monographName = genID('monograph'); + const schemaSDL = 'type Query { hello: String! }'; - const createMonographResponse = await client.createMonograph({ - name: monographName, - namespace, - graphUrl: 'http://localhost:4000', - routingUrl: 'http://localhost:3002', - }); - expect(createMonographResponse.response?.code).toBe(EnumStatusCode.OK); + const createMonographResponse = await client.createMonograph({ + name: monographName, + namespace, + graphUrl: 'http://localhost:4000', + routingUrl: 'http://localhost:3002', + }); + expect(createMonographResponse.response?.code).toBe(EnumStatusCode.OK); - const publishMonographResponse = await client.publishMonograph({ - name: monographName, - namespace, - schema: schemaSDL, - }); - expect(publishMonographResponse.response?.code).toBe(EnumStatusCode.OK); + const publishMonographResponse = await client.publishMonograph({ + name: monographName, + namespace, + schema: schemaSDL, + }); + expect(publishMonographResponse.response?.code).toBe(EnumStatusCode.OK); - await assertNumberOfCompositions(client, monographName, 1, namespace); + await assertNumberOfCompositions(client, monographName, 1, namespace); - const response = await client.recomposeGraph({ - name: monographName, - namespace, - isMonograph: true, - }); + const response = await client.recomposeGraph({ + name: monographName, + namespace, + isMonograph: true, + }); - expect(response.response).toBeDefined(); - expect(response.response!.code).toBe(EnumStatusCode.OK); - expect(response.compositionErrors).toHaveLength(0); - expect(response.deploymentErrors).toHaveLength(0); + expect(response.response).toBeDefined(); + expect(response.response!.code).toBe(EnumStatusCode.OK); + expect(response.compositionErrors).toHaveLength(0); + expect(response.deploymentErrors).toHaveLength(0); - await assertNumberOfCompositions(client, monographName, 2, namespace); - - await server.close(); + await assertNumberOfCompositions(client, monographName, 2, namespace); + } finally { + await server.close(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/monograph/recompose.test.ts` around lines 41 - 79, The test currently only calls server.close() at the end of the happy path; wrap the test logic that starts with SetupTest(...) and uses server in a try/finally so server.close() always runs; specifically, after const { client, server } = await SetupTest(...) move the assertions and client calls into a try block and put an awaited server.close() (guarded with if (server) or similar null-check) inside finally so the test cleanly tears down even if an assertion throws; update references to SetupTest and server.close accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/test/federated-graph/recompose.test.ts`:
- Around line 18-23: The mock for ClickHouseClient in recompose.test.ts is using
the wrong module ID so Vitest won't apply it; update the vi.mock call to use the
exact module ID used by the import (match '../../src/core/clickhouse/index.js')
and keep the mocked symbol names (ClickHouseClient and its
prototype.queryPromise) the same so the test intercepts the real imported class.
In `@controlplane/test/monograph/recompose.test.ts`:
- Around line 18-23: The vitest mock specifier doesn't match the import so the
mock isn't applied; update the vi.mock call to use the exact import specifier
used at top of the file (the same string used to import ClickHouseClient) so the
mock registers correctly—ensure the mocked module name in vi.mock matches the
import specifier and keep the mocked symbols (ClickHouseClient and its
prototype.queryPromise) intact.
---
Nitpick comments:
In `@controlplane/test/federated-graph/recompose.test.ts`:
- Around line 41-76: The test body in the 'that recomposing a published
federated graph succeeds and triggers a new composition' case can leak the
server when an assertion fails because server.close() is only called on the
success path; wrap the test's runtime logic (everything after const { client,
server } = await SetupTest(...) through the assertions and composition checks)
in a try/finally and call await server.close() in the finally block so
server.close() always runs regardless of failures; locate the code around
SetupTest, the subsequent
createNamespace/createThenPublishSubgraph/createFederatedGraph/assertNumberOfCompositions
calls and the client.recomposeGraph/assert expectations to implement the
try/finally.
In `@controlplane/test/monograph/recompose.test.ts`:
- Around line 41-79: The test currently only calls server.close() at the end of
the happy path; wrap the test logic that starts with SetupTest(...) and uses
server in a try/finally so server.close() always runs; specifically, after const
{ client, server } = await SetupTest(...) move the assertions and client calls
into a try block and put an awaited server.close() (guarded with if (server) or
similar null-check) inside finally so the test cleanly tears down even if an
assertion throws; update references to SetupTest and server.close accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39dd947d-1036-450f-9db8-7c19aa4a3a74
📒 Files selected for processing (3)
controlplane/src/core/bufservices/PlatformService.tscontrolplane/test/federated-graph/recompose.test.tscontrolplane/test/monograph/recompose.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/handle-composition-result.ts (1)
200-234:⚠️ Potential issue | 🟡 MinorWrap
printTruncationWarningcall to skip in JSON mode.When
shouldOutputJsonis true,printTruncationWarningoutputs plaintext to console after the JSON response, corrupting machine-readable output. This follows the same pattern already used at line 203 for composition warnings.🔧 Proposed fix
- printTruncationWarning({ displayedErrorCounts, totalErrorCounts }); + if (!shouldOutputJson) { + printTruncationWarning({ displayedErrorCounts, totalErrorCounts }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/handle-composition-result.ts` around lines 200 - 234, The call to printTruncationWarning is currently executed even when shouldOutputJson is true, which prints plaintext after JSON output; wrap the printTruncationWarning invocation in the same conditional used for composition warnings so it only runs when !shouldOutputJson (and optionally !suppressWarnings if that fits the intent). Locate the variables displayedErrorCounts and totalErrorCounts and move the printTruncationWarning({ displayedErrorCounts, totalErrorCounts }) call inside a guard that checks shouldOutputJson is false to prevent corrupting machine-readable output.
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/graph/recomposeGraph.ts (2)
169-177: Use aninterfaceand explicit return type for the webhook helper contract.Please switch
SendOrgWebhooksParamsto aninterfaceand annotatesendOrgWebhookswith an explicit return type (voidorPromise<void>depending on implementation).♻️ Proposed change
-type SendOrgWebhooksParams = { +interface SendOrgWebhooksParams { authContext: AuthContext; graph: FederatedGraphDTO; hasErrors: boolean; isMonograph: boolean; orgWebhooks: OrganizationWebhookService; -}; +} -function sendOrgWebhooks({ authContext, graph, hasErrors, isMonograph, orgWebhooks }: SendOrgWebhooksParams) { +function sendOrgWebhooks({ + authContext, + graph, + hasErrors, + isMonograph, + orgWebhooks, +}: SendOrgWebhooksParams): void {As per coding guidelines:
**/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript; Use explicit type annotations for function parameters and return types in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts` around lines 169 - 177, Replace the type alias SendOrgWebhooksParams with an interface declaration (interface SendOrgWebhooksParams { ... }) preserving the same properties (authContext, graph, hasErrors, isMonograph, orgWebhooks) and update the function signature for sendOrgWebhooks to include an explicit return type (either void or Promise<void> depending on whether it awaits async work) so the contract is clear; keep the parameter destructuring the same and ensure any async behavior returns Promise<void>.
41-43: Avoid mutating the incoming RPC request object.Prefer deriving a local
namespaceconstant instead of reassigningreq.namespace; this keeps request handling side-effect free and easier to reason about.🧹 Suggested cleanup
- req.namespace = req.namespace || DefaultNamespace; + const namespace = req.namespace || DefaultNamespace; - const graph = await federatedGraphRepo.byName(req.name, req.namespace); + const graph = await federatedGraphRepo.byName(req.name, namespace);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts` around lines 41 - 43, The code mutates the incoming RPC request by assigning req.namespace = req.namespace || DefaultNamespace; avoid this by deriving a local constant instead (e.g., namespace = req.namespace || DefaultNamespace) and pass that local constant to federatedGraphRepo.byName(req.name, namespace) so the request object remains immutable and side-effect free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cli/src/handle-composition-result.ts`:
- Around line 200-234: The call to printTruncationWarning is currently executed
even when shouldOutputJson is true, which prints plaintext after JSON output;
wrap the printTruncationWarning invocation in the same conditional used for
composition warnings so it only runs when !shouldOutputJson (and optionally
!suppressWarnings if that fits the intent). Locate the variables
displayedErrorCounts and totalErrorCounts and move the printTruncationWarning({
displayedErrorCounts, totalErrorCounts }) call inside a guard that checks
shouldOutputJson is false to prevent corrupting machine-readable output.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/graph/recomposeGraph.ts`:
- Around line 169-177: Replace the type alias SendOrgWebhooksParams with an
interface declaration (interface SendOrgWebhooksParams { ... }) preserving the
same properties (authContext, graph, hasErrors, isMonograph, orgWebhooks) and
update the function signature for sendOrgWebhooks to include an explicit return
type (either void or Promise<void> depending on whether it awaits async work) so
the contract is clear; keep the parameter destructuring the same and ensure any
async behavior returns Promise<void>.
- Around line 41-43: The code mutates the incoming RPC request by assigning
req.namespace = req.namespace || DefaultNamespace; avoid this by deriving a
local constant instead (e.g., namespace = req.namespace || DefaultNamespace) and
pass that local constant to federatedGraphRepo.byName(req.name, namespace) so
the request object remains immutable and side-effect free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89ede0f6-9abd-449e-9f19-c17394be0414
📒 Files selected for processing (7)
cli/src/handle-composition-result.tscli/test/graph/federated-graph/recompose.test.tscli/test/graph/monograph/recompose.test.tscontrolplane/src/core/bufservices/graph/recomposeGraph.tscontrolplane/test/federated-graph/recompose.test.tscontrolplane/test/monograph/recompose.test.tscontrolplane/test/monograph/version.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- controlplane/test/monograph/recompose.test.ts
- controlplane/test/federated-graph/recompose.test.ts
- cli/test/graph/federated-graph/recompose.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts (2)
164-188:⚠️ Potential issue | 🟡 MinorRemove redundant
server.close()call.The cleanup is already registered via
testContext.onTestFinished(() => server.close())at line 166. The explicitawait server.close()at line 187 is redundant and will cause the server to be closed twice.Proposed fix
expect(publishFeatureSubgraphResponse.response?.details).toBe( `Base subgraph "${nonExistentBaseSubgraph}" does not exist in the namespace "default".`, ); - - await server.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts` around lines 164 - 188, The test "that publishFederatedSubgraph fails when base subgraph does not exist" registers cleanup with testContext.onTestFinished(() => server.close()), so remove the redundant explicit await server.close() at the end of the test to avoid double-closing the server; locate the server.close() call inside the test (after the expects) and delete that line, leaving the testContext.onTestFinished cleanup in place.
190-220:⚠️ Potential issue | 🟠 MajorMissing server cleanup registration.
This test accepts
testContextbut never registers cleanup. Unlike all other tests in this file,testContext.onTestFinished(() => server.close())is missing, which will leak the server resource.Proposed fix
test('that publishFederatedSubgraph fails when base subgraph exists in different namespace', async (testContext) => { const { client, server } = await SetupTest({ dbname }); + testContext.onTestFinished(() => server.close()); const baseSubgraphName = genID('baseSubgraph');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts` around lines 190 - 220, The test "that publishFederatedSubgraph fails when base subgraph exists in different namespace" creates a server via SetupTest but never registers cleanup, leaking the server; add testContext.onTestFinished(() => server.close()) immediately after const { client, server } = await SetupTest({ dbname }) so the server is closed when the test finishes (refer to testContext, server, SetupTest and server.close for locating the code).controlplane/test/subgraph/update-subgraph.test.ts (1)
126-155:⚠️ Potential issue | 🟡 MinorMissing server cleanup in test.each block.
This
test.eachblock creates a server viaSetupTestbut does not register any cleanup hook. Unlike the test.each at lines 27-52 which usesonTestFinished(), this block has no cleanup, which could lead to resource leaks during test runs.🔧 Proposed fix
])('%s should not be able to update subgraph', async (role) => { const { client, server, authenticator, users } = await SetupTest({ dbname }); + onTestFinished(() => server.close()); const subgraphName = genID('subgraph');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/update-subgraph.test.ts` around lines 126 - 155, The test.each block creates a server via SetupTest but never registers cleanup; after destructuring const { client, server, authenticator, users } = await SetupTest({ dbname }); register the same teardown used in the other tests (call onTestFinished({ server }) or the project’s test cleanup helper) so the server is stopped when the test finishes; place this call immediately after SetupTest inside the '%s should not be able to update subgraph' async test to mirror the cleanup used around lines 27-52 and prevent resource leaks.controlplane/test/subgraph/publish-subgraph.test.ts (2)
649-653:⚠️ Potential issue | 🟡 MinorRedundant server cleanup -
await server.close()is unnecessary after registeringonTestFinished.Line 652 already registers the cleanup hook via
testContext.onTestFinished(() => server.close()). The explicitawait server.close()at line 652 will close the server immediately within the test, which contradicts the intent of theonTestFinishedpattern.🛠️ Proposed fix
expect(publishResponse.response?.code).toBe(EnumStatusCode.ERR); expect(publishResponse.response?.details).toBe('The version and platforms are required for plugin subgraphs.'); - - await server.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/publish-subgraph.test.ts` around lines 649 - 653, The test currently calls await server.close() directly after assertions even though the server is already scheduled to be closed via testContext.onTestFinished(() => server.close()); remove the explicit await server.close() call so cleanup is consistently handled by testContext.onTestFinished; locate the server variable and the onTestFinished registration in publish-subgraph.test.ts and delete the redundant await server.close() line.
655-679:⚠️ Potential issue | 🟡 MinorMissing server cleanup in test.
This test does not register any cleanup hook (
testContext.onTestFinishedoronTestFinished), which could leave the server running after the test completes, potentially causing resource leaks or test interference.🛠️ Proposed fix
- test('Should fail to publish plugin with invalid version format', async () => { + test('Should fail to publish plugin with invalid version format', async (testContext) => { const { client, server } = await SetupTest({ dbname, setupBilling: { plan: 'launch@1' }, }); + testContext.onTestFinished(() => server.close()); const pluginName = genID('plugin');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/publish-subgraph.test.ts` around lines 655 - 679, The test "Should fail to publish plugin with invalid version format" calls SetupTest which returns { client, server } but doesn't register a cleanup hook; register a testContext.onTestFinished async cleanup that stops the server (and closes the client if applicable) to avoid leaving the server running. Inside the test, after obtaining { client, server } from SetupTest, add: testContext.onTestFinished(async () => { await server.stop(); /* await client.close() if client has a close method */ }); referencing SetupTest, server and client so the server is always shut down after the test.controlplane/test/feature-subgraph/create-feature-subgraph.test.ts (1)
689-725:⚠️ Potential issue | 🟡 MinorRedundant
await server.close()at line 723.The cleanup is already registered via
onTestFinished(() => server.close())at line 693. The explicitawait server.close()at line 723 is redundant and will close the server before the test officially ends.🛠️ Proposed fix
expect(getFeatureSubgraphResponse.response?.code).toBe(EnumStatusCode.OK); expect(getFeatureSubgraphResponse.graph?.name).toBe(featureSubgraphName); expect(getFeatureSubgraphResponse.graph?.routingURL).toBe(DEFAULT_SUBGRAPH_URL_TWO); expect(getFeatureSubgraphResponse.graph?.isFeatureSubgraph).toBe(true); - - await server.close(); }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/create-feature-subgraph.test.ts` around lines 689 - 725, Remove the redundant explicit shutdown at the end of the test: inside the test.each block (the '%s should be able to create feature subgraph' test using variables client, server, authenticator, users) delete the final "await server.close()" call — leave the registered cleanup onTestFinished(() => server.close()) to handle closing the server; no other changes to createSubgraph, client.createFederatedSubgraph, or getSubgraphByName are required.controlplane/test/proposal/update-proposal.test.ts (1)
1187-1194:⚠️ Potential issue | 🟠 MajorMissing server cleanup registration.
This test receives
testContextbut never registerstestContext.onTestFinished(() => server.close()). The server won't be closed after this test completes, which could cause resource leaks or test isolation issues.🐛 Proposed fix
test('should fetch proposal checks after updating a proposal', async (testContext) => { const { client, server } = await SetupTest({ dbname, chClient, setupBilling: { plan: 'enterprise' }, enabledFeatures: ['proposals'], }); + testContext.onTestFinished(() => server.close()); // Setup a federated graph with a single subgraph🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/update-proposal.test.ts` around lines 1187 - 1194, The test "should fetch proposal checks after updating a proposal" that calls SetupTest(...) creates a server but never registers cleanup; add a testContext.onTestFinished(() => server.close()) registration immediately after acquiring { client, server } so the server is closed when the test finishes (use the same variables returned from SetupTest to ensure server.close() is invoked).controlplane/test/subgraph/delete-subgraph.test.ts (1)
155-158:⚠️ Potential issue | 🟠 MajorSecond federated-graph assertion is querying the wrong graph.
getGraph2RespusesfederatedGraph1Namein both checks, so the test never verifies the state of graph 2 and can pass while graph-2 behavior is broken.✅ Proposed fix
- let getGraph2Resp = await client.getFederatedGraphByName({ - name: federatedGraph1Name, + let getGraph2Resp = await client.getFederatedGraphByName({ + name: federatedGraph2Name, namespace: 'default', }); ... - getGraph2Resp = await client.getFederatedGraphByName({ - name: federatedGraph1Name, + getGraph2Resp = await client.getFederatedGraphByName({ + name: federatedGraph2Name, namespace: 'default', });Also applies to: 187-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/delete-subgraph.test.ts` around lines 155 - 158, The test incorrectly queries the same graph twice: update the second call that sets getGraph2Resp to request federatedGraph2Name instead of federatedGraph1Name so it verifies graph-2 state; locate the getFederatedGraphByName invocation that assigns getGraph2Resp and replace the name parameter from federatedGraph1Name to federatedGraph2Name (also apply the same fix to the analogous call around the other occurrence mentioned).controlplane/test/proposal/create-proposal.test.ts (1)
866-880:⚠️ Potential issue | 🟠 MajorFix inconsistent server teardown and add type annotation for testContext parameter.
Two related issues in the teardown migration:
Double close: Line 872 registers
testContext.onTestFinished(() => server.close())but line 879 also callsawait server.close()manually.Missing teardown: The test starting at line 882 accepts
testContextbut never registers cleanup or closes the server, leaking resources.Additionally, all
testContextparameters lack type annotations. Per TypeScript guidelines, import and applyTestContextfrom vitest:async (testContext: TestContext) =>.Suggested patch
test('should fail to enable proposals with developer billing plan', async (testContext) => { const { client, server } = await SetupTest({ dbname, chClient, setupBilling: { plan: 'developer@1' }, }); testContext.onTestFinished(() => server.close()); // Try to enable proposals for the namespace const enableResponse = await enableProposalsForNamespace(client); expect(enableResponse.response?.code).toBe(EnumStatusCode.ERR_UPGRADE_PLAN); expect(enableResponse.response?.details).toContain('Upgrade to a launch plan to enable proposals'); - - await server.close(); }); test('should fail to create a proposal when proposals are not enabled for namespace', async (testContext) => { const { client, server } = await SetupTest({ dbname, chClient, setupBilling: { plan: 'enterprise' }, enabledFeatures: ['proposals'], }); + testContext.onTestFinished(() => server.close()); // Don't enable proposals for the namespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/create-proposal.test.ts` around lines 866 - 880, Remove the duplicate manual teardown and consistently register the test cleanup via testContext.onTestFinished: in the test using SetupTest(...) and enableProposalsForNamespace(...) remove the trailing await server.close() and rely on testContext.onTestFinished(() => server.close()); for the test that currently accepts testContext but never registers cleanup, add testContext.onTestFinished(() => server.close()) so the server is always closed; add a type annotation to the testContext parameter on both tests as async (testContext: TestContext) => and import TestContext from 'vitest' at the top of the file.
🧹 Nitpick comments (15)
controlplane/test/initialize-cosmo-user.test.ts (1)
3-3: Consider using consistenttestContext.onTestFinishedpattern for all tests.The migration to
onTestFinishedfor cleanup is good. However, there's a minor inconsistency: line 26 uses the globalonTestFinished, while other tests usetestContext.onTestFinished. This is becausetest.eachprovides the test data as the first parameter.For consistency, you could add
testContextas the second parameter to thetest.eachcallback:♻️ Proposed refactor for consistency
- test.each(['', ' '])('should return `Bad Request` when token is empty or whitespace', async (token: string) => { + test.each(['', ' '])('should return `Bad Request` when token is empty or whitespace', async (token: string, testContext) => { const { client, server } = await SetupTest({ dbname }); - onTestFinished(() => server.close()); + testContext.onTestFinished(() => server.close()); const response = await client.initializeCosmoUser({ token }); expect(response.response?.code).toBe(EnumStatusCode.ERR_BAD_REQUEST); });Also applies to: 24-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/initialize-cosmo-user.test.ts` at line 3, Tests using test.each should accept the testContext parameter and call testContext.onTestFinished instead of the global onTestFinished for consistency; update the test.each callbacks (the functions passed to test.each) to include testContext as the second parameter and replace any direct calls to onTestFinished(...) with testContext.onTestFinished(...), ensuring consistent cleanup handling across tests that use testContext and the onTestFinished API.controlplane/test/subgraph/get-subgraphs.test.ts (1)
26-26: Consider updating remaining tests to use the same cleanup pattern.Tests at lines 26 and 105 already accept the
testContextparameter but still use manualawait server.close()at the end. For consistency and improved robustness (cleanup on failure), consider updating them to usetestContext.onTestFinished(() => server.close())as well. Thetest.eachat line 189 could also be updated to accepttestContext.Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/get-subgraphs.test.ts` at line 26, Update the tests that currently call await server.close() manually to register cleanup via the testContext hook instead: add the testContext parameter to the affected test functions (e.g., the "Should be able to list subgraphs of different namespace" test and the other test that closes server manually), remove the explicit await server.close(), and call testContext.onTestFinished(() => server.close()) to ensure server.close() runs even on failures; also modify the test.each table-based test to accept testContext and use the same onTestFinished cleanup pattern.controlplane/test/apollo-federation.test.ts (1)
38-38: Redundant server cleanup detected.The test registers cleanup via
testContext.onTestFinished(() => server.close())at line 38, but also has an explicitawait server.close()at line 228. This is redundant since theonTestFinishedhook will also close the server. Other tests in this PR removed the explicitserver.close()calls when adopting the testContext-based pattern.Consider removing the explicit call at line 228 for consistency with the rest of the test suite.
♻️ Suggested fix
expect(graph.subgraphs[3]?.labels).toEqual([label]); expect(graph.subgraphs[3]?.lastUpdatedAt).toBeTruthy(); expect(graph.subgraphs[3]?.routingURL).toEqual('http://localhost:8084'); - - await server.close(); }); });Also applies to: 228-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/apollo-federation.test.ts` at line 38, The test registers cleanup via testContext.onTestFinished(() => server.close()), so remove the redundant explicit await server.close() call (the one that appears later in the test) and rely on the testContext hook for shutting down the server; locate the explicit await server.close() in the test body and delete that line to match the other tests using testContext.onTestFinished.controlplane/test/api-keys.test.ts (1)
1-1: Minor inconsistency in cleanup hook usage.The file mixes two equivalent patterns:
testContext.onTestFinished(() => server.close())(lines 28, 163, 191, 226)onTestFinished(() => server.close())(lines 107, 270, 305, 349)Both are functionally equivalent in Vitest, but using
onTestFinisheddirectly (imported at line 1) is likely necessary fortest.eachcallbacks where the test context isn't directly accessible. This is acceptable but worth noting for consistency awareness.Also applies to: 107-107, 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/api-keys.test.ts` at line 1, The test file mixes two equivalent cleanup patterns—using testContext.onTestFinished(() => server.close()) and the imported onTestFinished(() => server.close())—which is inconsistent; pick one approach and apply it consistently across the file (prefer using the imported onTestFinished for tests that rely on test.each and use testContext.onTestFinished only where testContext is explicitly available), and update all occurrences (references to testContext.onTestFinished, onTestFinished, and server.close) so a single style is used throughout.controlplane/test/subgraph/update-subgraph.test.ts (2)
49-51: Redundant server cleanup.Line 31 already registers
onTestFinished(() => server.close()), so the explicitawait server.close()at line 50 is redundant and can be removed.🧹 Suggested cleanup
expect(createFederatedSubgraphResp.response?.code).toBe(EnumStatusCode.OK); - - await server.close(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/update-subgraph.test.ts` around lines 49 - 51, The explicit await server.close() at the end of the test is redundant because server.close() is already registered via onTestFinished(() => server.close()); remove the final await server.close() statement to avoid duplicate cleanup; search for the server variable and the onTestFinished registration in update-subgraph.test.ts to locate and delete the extra await server.close() call.
74-76: Redundant server cleanup.Line 56 already registers
testContext.onTestFinished(() => server.close()), so the explicitawait server.close()at line 75 is redundant and can be removed.🧹 Suggested cleanup
expect(createFederatedSubgraphResp.response?.code).toBe(EnumStatusCode.OK); - - await server.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/update-subgraph.test.ts` around lines 74 - 76, The final explicit call to await server.close() is redundant because testContext.onTestFinished(() => server.close()) already schedules server shutdown; remove the trailing await server.close() from the test to avoid duplicate cleanup and rely on the registered testContext.onTestFinished teardown for the server instance referenced by server and its server.close() method.cli/test/graph/utils.ts (1)
23-50: Consider adding explicit return type annotation.Per coding guidelines, functions should have explicit type annotations for return types. While TypeScript can infer
Promise<void>, adding it explicitly improves readability and aligns with the guidelines.♻️ Proposed improvement
export async function runRecompose( response: PartialMessage<RecomposeGraphResponse>, opts: { isMonograph?: boolean; namespace?: string; failOnCompositionError?: boolean; failOnAdmissionWebhookError?: boolean; suppressWarnings?: boolean; } = {}, -): Promise<void> { +): Promise<void> {The return type is already present - this is correct!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/graph/utils.ts` around lines 23 - 50, The function runRecompose should have an explicit return type; verify that runRecompose is declared with the Promise<void> return annotation (as currently present) and if it's missing in the diff, add the explicit return type Promise<void> to the runRecompose signature; also ensure related exported symbol names (runRecompose, RecomposeCommand, createClient) remain unchanged so callers and tests continue to resolve correctly.controlplane/test/feature-flag/create-feature-flag.test.ts (1)
214-218: Consider usingtestContextconsistently withtest.eachcallbacks.The
test.eachtests use the importedonTestFinisheddirectly, while regular tests usetestContext.onTestFinished. Both work correctly, but for consistency, you could receivetestContextas the second parameter:test.each(['organization-admin', 'organization-developer'])( '%s should be able to create feature graph with a feature subgraph', async (role, testContext) => { const { client, server, authenticator, users } = await SetupTest({ dbname }); testContext.onTestFinished(() => server.close()); // ... }, );Alternatively, all tests could use the imported
onTestFinishedfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-flag/create-feature-flag.test.ts` around lines 214 - 218, The test uses test.each with the global onTestFinished helper instead of the per-test context method; update the test.each callback signature to accept the second parameter testContext and replace onTestFinished(() => server.close()) with testContext.onTestFinished(() => server.close()) so the test uses testContext consistently (locate the test defined with test.each(...) and the SetupTest call to modify the callback parameter and onTestFinished usage).controlplane/test/proposal/update-proposal.test.ts (1)
34-54: Consider using a proper type instead ofanyfor theclientparameter.Both helper functions use
client: any, which loses type safety. Consider using the actual client type from the test utilities or a more specific interface.♻️ Suggested improvement
If the client type is exported from your test utilities, you could type it properly:
-async function enableProposalsForNamespace(client: any, namespace = DEFAULT_NAMESPACE) { +async function enableProposalsForNamespace(client: ReturnType<typeof SetupTest> extends Promise<infer T> ? T['client'] : never, namespace = DEFAULT_NAMESPACE) {Or define a simpler interface for the methods used:
interface ProposalClient { enableProposalsForNamespace: (args: { namespace: string; enableProposals: boolean }) => Promise<{ response?: { code: EnumStatusCode } }>; createProposal: (args: any) => Promise<{ response?: { code: EnumStatusCode }; proposalName?: string; proposalId?: string }>; // ... other methods }As per coding guidelines: "Avoid
anytype in TypeScript; use specific types or generics".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/update-proposal.test.ts` around lines 34 - 54, The helpers enableProposalsForNamespace and createTestProposal currently accept client: any — replace the any with a concrete client type (import the exported test client type from your test utilities if available) or define a small ProposalClient interface in the test file that declares the exact methods used (e.g., enableProposalsForNamespace, createProposal and any other invoked RPCs) with their argument and Promise-return types; then update both function signatures to use that type so the compiler enforces correct method shapes and return types when calling enableProposalsForNamespace and createTestProposal.controlplane/test/integrations.test.ts (1)
45-45: Add explicittestContextand callback return types.These new TypeScript callback signatures currently rely on inference. Please annotate parameter and return types explicitly.
♻️ Suggested patch
-import { afterAll, afterEach, beforeAll, describe, expect, test } from 'vitest'; +import { afterAll, afterEach, beforeAll, describe, expect, test, type TestContext } from 'vitest'; @@ - test('Webhook meta for monograph and federated graph should be stored and retrieved correctly', async (testContext) => { + test('Webhook meta for monograph and federated graph should be stored and retrieved correctly', async (testContext: TestContext): Promise<void> => { @@ - test('Slack integration meta for monograph and federated graph should be stored and retrieved correctly', async (testContext) => { + test('Slack integration meta for monograph and federated graph should be stored and retrieved correctly', async (testContext: TestContext): Promise<void> => { @@ - test('that resolvability validation is disabled successfully', async (testContext) => { + test('that resolvability validation is disabled successfully', async (testContext: TestContext): Promise<void> => { @@ - test('that true external entity key errors can be ignored with the composition feature flag', async (testContext) => { + test('that true external entity key errors can be ignored with the composition feature flag', async (testContext: TestContext): Promise<void> => {As per coding guidelines
**/*.{ts,tsx}: Use explicit type annotations for function parameters and return types in TypeScript.Also applies to: 126-126, 197-197, 268-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/integrations.test.ts` at line 45, Annotate the AVA test callbacks to use explicit parameter and return types: change each async test callback from (testContext) => { ... } to (testContext: any): Promise<void> => { ... } (or replace "any" with the correct project TestContext type if available); update the occurrences for the 'test' callbacks including the one with the description "Webhook meta for monograph and federated graph should be stored and retrieved correctly" and the other test callbacks at the other noted locations so every test callback has an explicit parameter type for testContext and an explicit Promise<void> return type.controlplane/test/plugin/validate-and-fetch-plugin-data.test.ts (1)
4-4: Add explicitTestContextand return type annotations in updated test callbacks.The new callback signatures use untyped
testContextparameters. Please annotate parameter and return types to match the repository TS rule.Suggested patch
-import { afterAll, beforeAll, describe, expect, onTestFinished, test } from 'vitest'; +import { afterAll, beforeAll, describe, expect, onTestFinished, test, type TestContext } from 'vitest'; -test('Should successfully validate and fetch plugin data for new plugin', async (testContext) => { +test('Should successfully validate and fetch plugin data for new plugin', async (testContext: TestContext): Promise<void> => { // ... }); -test('Should increment version for existing plugin', async (testContext) => { +test('Should increment version for existing plugin', async (testContext: TestContext): Promise<void> => { // ... }); -// apply the same pattern to the other updated test callbacks +// apply the same annotation pattern to the remaining updated callbacksAs per coding guidelines, “Use explicit type annotations for function parameters and return types in TypeScript”.
Also applies to: 36-36, 60-60, 108-108, 131-131, 169-169, 194-194, 217-217, 249-249, 317-317, 344-344, 366-366, 388-388, 409-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/plugin/validate-and-fetch-plugin-data.test.ts` at line 4, The test callbacks currently accept untyped testContext parameters and lack explicit return types; update each callback signature used with vitest functions (beforeAll, afterAll, test, onTestFinished) to annotate the first parameter as TestContext and to have an explicit return type (use Promise<void> for async callbacks or void for synchronous ones). For example, change any occurrences like "beforeAll((testContext) => { ... })" or "test('...', (testContext) => { ... })" to "beforeAll((testContext: TestContext): Promise<void> => { ... })" or "...: void => { ... }" as appropriate, and add the TestContext import from 'vitest' where needed so symbols like TestContext, beforeAll, afterAll, test, and onTestFinished are properly typed. Ensure you apply this to all listed callback locations (lines referenced in the review) so every test callback has explicit parameter and return type annotations.controlplane/test/restore-organization.test.ts (1)
38-38: Add explicitTestContextand callback return type on the test function.Line 38 introduces an untyped parameter in a TypeScript callback.
Suggested change
-import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, type TestContext } from 'vitest'; -test('should removed queued job and keep organization', async (testContext) => { +test('should removed queued job and keep organization', async (testContext: TestContext): Promise<void> => {As per coding guidelines,
**/*.{ts,tsx}: "Use explicit type annotations for function parameters and return types in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/restore-organization.test.ts` at line 38, The test callback for test('should removed queued job and keep organization', async (testContext) => { ... }) has an untyped parameter and missing return type; change the signature to async (testContext: TestContext): Promise<void> and ensure the TestContext type is imported from the test framework (or local test types) so the parameter and the async return are explicitly annotated.controlplane/test/subgraph/delete-subgraph.test.ts (1)
196-196: Add explicittestContextparameter and callback return types in these modified TS test signatures.These callbacks currently rely on inference; annotate parameter and return type explicitly for consistency with repository TS rules.
✍️ Suggested typing update
-import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, type TestContext } from 'vitest'; ... -test('that deleting a subgraph also deletes any feature subgraphs for which it is the base subgraph', async (testContext) => { +test('that deleting a subgraph also deletes any feature subgraphs for which it is the base subgraph', async (testContext: TestContext): Promise<void> => { ... -test('Should be able to create a plugin and then delete it', async (testContext) => { +test('Should be able to create a plugin and then delete it', async (testContext: TestContext): Promise<void> => {As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript".
Also applies to: 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/subgraph/delete-subgraph.test.ts` at line 196, The test callbacks that currently rely on inference need explicit TypeScript annotations: update the test function signatures that accept the parameter named testContext (e.g., in the test titled "that deleting a subgraph also deletes any feature subgraphs for which it is the base subgraph" and the other modified test) to declare the parameter type (such as TestContext or the repository's test context interface) and add an explicit return type (Promise<void> if async); locate the callbacks passed to test(...) and annotate the parameter and the callback return type accordingly (e.g., (testContext: YourTestContextType): Promise<void> => { ... }).controlplane/test/contracts.test.ts (1)
54-56: Add explicitTestContexttype annotation to callback parameters.All 30 test functions in this file accept
testContextwithout explicit type annotation. ImportTestContextfrom vitest and annotate the parameter consistently across all test cases.Update the import:
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, type TestContext, vi } from 'vitest';Then annotate each test callback:
async (testContext: TestContext) =>This satisfies the TypeScript guideline for explicit parameter type annotations in
**/*.{ts,tsx}files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/contracts.test.ts` around lines 54 - 56, Add an explicit TestContext type to the vitest test callbacks: update the import list to include `type TestContext` from vitest (as suggested) and annotate every test callback parameter `testContext` as `testContext: TestContext` in the file (e.g., in the test functions such as the one invoking `SetupTest({ dbname, chClient })`), ensuring all ~30 test functions use the same typed signature and that the import includes `type TestContext`.controlplane/test/proposal/create-proposal.test.ts (1)
354-354: Add explicittestContexttype annotations in changed test callbacks.The newly introduced
testContextparameters are untyped. Please annotate them (and optionally return type) to match the TS guideline.Suggested pattern
-import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, onTestFinished, test, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, onTestFinished, test, vi, type TestContext } from 'vitest'; -test('...', async (testContext) => { +test('...', async (testContext: TestContext): Promise<void> => {As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript".
Also applies to: 488-488, 583-583, 677-677, 714-714, 866-866, 882-882, 942-942, 1035-1035, 1162-1162, 1214-1214, 1297-1297, 1487-1487, 1609-1609, 1674-1674, 1766-1766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/proposal/create-proposal.test.ts` at line 354, The test callbacks like the one in create-proposal.test.ts (e.g., the test named "should create a proposal with multiple subgraph changes" and other affected tests at the listed locations) use an untyped parameter testContext; add explicit TypeScript parameter (and optional return) type annotations (for example: testContext: TestContext or the project's preferred test fixture type) to each callback signature so the parameter is typed consistently across functions such as the test(...) callbacks at the mentioned positions; update all occurrences (lines referenced: 354, 488, 583, 677, 714, 866, 882, 942, 1035, 1162, 1214, 1297, 1487, 1609, 1674, 1766) to the same annotated form and ensure any imports for the TestContext type are added if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/test/analytics/get-operation-deprecated-fields.test.ts`:
- Line 37: Import the TestContext type from AVA and add explicit TypeScript
annotations to all test callback parameters and return types so each test uses
the typed signature (e.g., annotate the `testContext` parameter as `TestContext`
and the test callback return type as `Promise<void>`); update every test case
that currently declares `test('...', async (testContext) => {` (including the
occurrences referenced by `testContext`) to use the typed form and ensure the
import of `TestContext` is present at the top of the file so symbols like `test`
and `testContext` are properly typed.
In `@controlplane/test/analytics/get-operations.test.ts`:
- Line 41: The test callbacks in get-operations.test.ts accept an untyped
testContext parameter (e.g., the callback starting at the test named "Should
return ERR_ANALYTICS_DISABLED when ClickHouse client is not available") which
violates the TS guideline; fix this by importing TestContext from 'vitest' (add
import { TestContext } from 'vitest') and annotating each test callback
parameter as testContext: TestContext for all occurrences (lines referenced:
tests at names/positions around the callbacks starting at the shown test and the
other listed tests), updating the function signatures for each test block to use
the explicit TestContext type.
In `@controlplane/test/composition-warnings.test.ts`:
- Line 77: Fix the typo in the test titles: change "an warning" to "a warning"
in the test declaration strings (e.g., the test whose title is "that an warning
is returned if a V1 interface extension field is declared `@external`" and the
other occurrence around line 121). Update the test title strings in
composition-warnings.test.ts to read "that a warning is returned..." so both
test names are grammatically correct.
In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts`:
- Line 31: Import TestContext from 'vitest' and add explicit types to the async
test callbacks: annotate the parameter as testContext: TestContext and the
function return type as Promise<void> for each test callback that currently
accepts testContext (the five test(...) callbacks in
update-feature-subgraph.test.ts). Update the top-level imports to include
TestContext and modify each async arrow function signature to include the
parameter type and trailing : Promise<void> return type so TypeScript has
explicit types for testContext and the async callbacks.
In `@controlplane/test/fetch-checks-by-federated-graph.test.ts`:
- Around line 36-38: The test callbacks in
fetch-checks-by-federated-graph.test.ts are missing explicit TypeScript
annotations for the testContext parameter; import TestContext from 'ava' and
update each test callback signature (e.g., the test callback for "Should be able
to fetch checks for a federated graph that exists" and the other seven tests at
the noted locations) to declare testContext: TestContext and a return type of
Promise<void>; ensure the import (import type { TestContext } from 'ava') is
added at the top and apply the same signature change to all referenced test(...)
callbacks so each callback parameter is explicitly typed.
In `@controlplane/test/namespace/get-namespaces.test.ts`:
- Around line 54-56: The test callback parameter for the Jest/Mocha test
function is missing an explicit TypeScript type; update the test declaration
(the async callback passed to test in get-namespaces.test.ts) to annotate the
parameter and return type (e.g., annotate the callback parameter named
testContext with the appropriate type used in your test harness and declare the
function return type as Promise<void>) so the function signature is fully typed;
locate the test using the name "Should be able to retrieve all namespaces when
using legacy API key" and add the explicit types to that async test callback.
In `@controlplane/test/oidc-provider.test.ts`:
- Line 19: The test callback parameters named testContext in the various
test(...) calls are untyped; update each callback signature to explicitly type
the parameter as TestContext (e.g., async (testContext: TestContext) => { ... })
and ensure TestContext is imported or referenced from your test framework/types
(add an import or type alias if missing); look for occurrences in the file such
as the test functions starting at the lines with "Should be able to create an
OIDC provider" and the other test(...) calls and apply the same change
consistently.
In `@controlplane/test/organization-groups.test.ts`:
- Around line 19-21: Update every async test callback signature from async
(testContext) => { ... } to async (testContext: TestContext): Promise<void> => {
... } so each test callback has an explicit parameter type and return type;
ensure you import TestContext from the test runner (e.g., import type {
TestContext } from 'ava') if not already present; apply this change to all
test(...) usages in the file (the async callbacks that call SetupTest and use
testContext.onTestFinished).
In `@controlplane/test/proposal/proposal-data-isolation.test.ts`:
- Line 41: Add explicit TestContext type annotations to the vitest test
callbacks: import TestContext from 'vitest' at the top and change each test
callback parameter (e.g., the parameter currently named testContext in the
test(...) calls) to be typed as testContext: TestContext; update every
occurrence of the test(...) callbacks in this file (including the tests at the
noted locations) so their callback parameters are annotated, ensuring function
signatures like test('...', async (testContext: TestContext) => { ... }) and
preserving existing parameter names.
In
`@controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts`:
- Line 52: The test callbacks use untyped parameters like async (testContext)
=>; update each test callback to declare the parameter and return types
explicitly (e.g., annotate the testContext parameter with the appropriate
TestContext type used in your test harness and add an explicit Promise<void>
return type) so signatures become async (testContext: TestContext):
Promise<void> =>; apply this change consistently for the referenced tests that
use the testContext parameter (search for occurrences of the test callbacks
named in the diffs and update their signatures).
In `@controlplane/test/proposal/proposal-schema-match.test.ts`:
- Line 135: Import the TestContext type from 'vitest' and add explicit type
annotations to all test callback parameters named testContext (e.g., in the test
titled "should pass check with matching schema when proposal is approved and
check severity is set to warn" and the other tests referenced) so each callback
signature reads like (testContext: TestContext) => Promise<void> (or appropriate
return type); update the file-level imports to include TestContext and annotate
every test function parameter named testContext across the listed tests to
satisfy the TypeScript parameter-annotation guideline.
In `@controlplane/test/proposal/update-proposal.test.ts`:
- Line 1184: Remove the redundant explicit await server.close() call at the end
of the test; this test already registers cleanup via
testContext.onTestFinished(() => server.close()) (see testContext.onTestFinished
and server.close references) so delete the standalone await server.close() to
match the other tests and rely on the registered onTestFinished cleanup.
In `@controlplane/test/router/compatibility-version/list.test.ts`:
- Around line 35-37: The test callback parameter testContext in the test(...)
arrow function is missing an explicit type; update the signature to declare the
correct test framework context type (for example: test(testContext: TestContext)
=> ...) and add the corresponding import for that type at the top of the file
(e.g., import type { TestContext } from 'ava' or the project's test framework).
Ensure the parameter name remains testContext and that the rest of the code
(including SetupTest and testContext.onTestFinished) compiles with the newly
declared type.
In `@controlplane/test/subgraph/subgraph.test.ts`:
- Line 40: The test registers a teardown hook via testContext.onTestFinished(()
=> server.close()) but the first test also explicitly calls server.close()
later, causing a double close; remove the explicit server.close() call inside
the first test (the one that manually closes the server) so the server is only
closed by the registered testContext.onTestFinished cleanup; keep the
onTestFinished hook and delete the redundant server.close() invocation to avoid
teardown flakiness.
---
Outside diff comments:
In `@controlplane/test/feature-subgraph/create-feature-subgraph.test.ts`:
- Around line 689-725: Remove the redundant explicit shutdown at the end of the
test: inside the test.each block (the '%s should be able to create feature
subgraph' test using variables client, server, authenticator, users) delete the
final "await server.close()" call — leave the registered cleanup
onTestFinished(() => server.close()) to handle closing the server; no other
changes to createSubgraph, client.createFederatedSubgraph, or getSubgraphByName
are required.
In `@controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts`:
- Around line 164-188: The test "that publishFederatedSubgraph fails when base
subgraph does not exist" registers cleanup with testContext.onTestFinished(() =>
server.close()), so remove the redundant explicit await server.close() at the
end of the test to avoid double-closing the server; locate the server.close()
call inside the test (after the expects) and delete that line, leaving the
testContext.onTestFinished cleanup in place.
- Around line 190-220: The test "that publishFederatedSubgraph fails when base
subgraph exists in different namespace" creates a server via SetupTest but never
registers cleanup, leaking the server; add testContext.onTestFinished(() =>
server.close()) immediately after const { client, server } = await SetupTest({
dbname }) so the server is closed when the test finishes (refer to testContext,
server, SetupTest and server.close for locating the code).
In `@controlplane/test/proposal/create-proposal.test.ts`:
- Around line 866-880: Remove the duplicate manual teardown and consistently
register the test cleanup via testContext.onTestFinished: in the test using
SetupTest(...) and enableProposalsForNamespace(...) remove the trailing await
server.close() and rely on testContext.onTestFinished(() => server.close()); for
the test that currently accepts testContext but never registers cleanup, add
testContext.onTestFinished(() => server.close()) so the server is always closed;
add a type annotation to the testContext parameter on both tests as async
(testContext: TestContext) => and import TestContext from 'vitest' at the top of
the file.
In `@controlplane/test/proposal/update-proposal.test.ts`:
- Around line 1187-1194: The test "should fetch proposal checks after updating a
proposal" that calls SetupTest(...) creates a server but never registers
cleanup; add a testContext.onTestFinished(() => server.close()) registration
immediately after acquiring { client, server } so the server is closed when the
test finishes (use the same variables returned from SetupTest to ensure
server.close() is invoked).
In `@controlplane/test/subgraph/delete-subgraph.test.ts`:
- Around line 155-158: The test incorrectly queries the same graph twice: update
the second call that sets getGraph2Resp to request federatedGraph2Name instead
of federatedGraph1Name so it verifies graph-2 state; locate the
getFederatedGraphByName invocation that assigns getGraph2Resp and replace the
name parameter from federatedGraph1Name to federatedGraph2Name (also apply the
same fix to the analogous call around the other occurrence mentioned).
In `@controlplane/test/subgraph/publish-subgraph.test.ts`:
- Around line 649-653: The test currently calls await server.close() directly
after assertions even though the server is already scheduled to be closed via
testContext.onTestFinished(() => server.close()); remove the explicit await
server.close() call so cleanup is consistently handled by
testContext.onTestFinished; locate the server variable and the onTestFinished
registration in publish-subgraph.test.ts and delete the redundant await
server.close() line.
- Around line 655-679: The test "Should fail to publish plugin with invalid
version format" calls SetupTest which returns { client, server } but doesn't
register a cleanup hook; register a testContext.onTestFinished async cleanup
that stops the server (and closes the client if applicable) to avoid leaving the
server running. Inside the test, after obtaining { client, server } from
SetupTest, add: testContext.onTestFinished(async () => { await server.stop(); /*
await client.close() if client has a close method */ }); referencing SetupTest,
server and client so the server is always shut down after the test.
In `@controlplane/test/subgraph/update-subgraph.test.ts`:
- Around line 126-155: The test.each block creates a server via SetupTest but
never registers cleanup; after destructuring const { client, server,
authenticator, users } = await SetupTest({ dbname }); register the same teardown
used in the other tests (call onTestFinished({ server }) or the project’s test
cleanup helper) so the server is stopped when the test finishes; place this call
immediately after SetupTest inside the '%s should not be able to update
subgraph' async test to mirror the cleanup used around lines 27-52 and prevent
resource leaks.
---
Nitpick comments:
In `@cli/test/graph/utils.ts`:
- Around line 23-50: The function runRecompose should have an explicit return
type; verify that runRecompose is declared with the Promise<void> return
annotation (as currently present) and if it's missing in the diff, add the
explicit return type Promise<void> to the runRecompose signature; also ensure
related exported symbol names (runRecompose, RecomposeCommand, createClient)
remain unchanged so callers and tests continue to resolve correctly.
In `@controlplane/test/api-keys.test.ts`:
- Line 1: The test file mixes two equivalent cleanup patterns—using
testContext.onTestFinished(() => server.close()) and the imported
onTestFinished(() => server.close())—which is inconsistent; pick one approach
and apply it consistently across the file (prefer using the imported
onTestFinished for tests that rely on test.each and use
testContext.onTestFinished only where testContext is explicitly available), and
update all occurrences (references to testContext.onTestFinished,
onTestFinished, and server.close) so a single style is used throughout.
In `@controlplane/test/apollo-federation.test.ts`:
- Line 38: The test registers cleanup via testContext.onTestFinished(() =>
server.close()), so remove the redundant explicit await server.close() call (the
one that appears later in the test) and rely on the testContext hook for
shutting down the server; locate the explicit await server.close() in the test
body and delete that line to match the other tests using
testContext.onTestFinished.
In `@controlplane/test/contracts.test.ts`:
- Around line 54-56: Add an explicit TestContext type to the vitest test
callbacks: update the import list to include `type TestContext` from vitest (as
suggested) and annotate every test callback parameter `testContext` as
`testContext: TestContext` in the file (e.g., in the test functions such as the
one invoking `SetupTest({ dbname, chClient })`), ensuring all ~30 test functions
use the same typed signature and that the import includes `type TestContext`.
In `@controlplane/test/feature-flag/create-feature-flag.test.ts`:
- Around line 214-218: The test uses test.each with the global onTestFinished
helper instead of the per-test context method; update the test.each callback
signature to accept the second parameter testContext and replace
onTestFinished(() => server.close()) with testContext.onTestFinished(() =>
server.close()) so the test uses testContext consistently (locate the test
defined with test.each(...) and the SetupTest call to modify the callback
parameter and onTestFinished usage).
In `@controlplane/test/initialize-cosmo-user.test.ts`:
- Line 3: Tests using test.each should accept the testContext parameter and call
testContext.onTestFinished instead of the global onTestFinished for consistency;
update the test.each callbacks (the functions passed to test.each) to include
testContext as the second parameter and replace any direct calls to
onTestFinished(...) with testContext.onTestFinished(...), ensuring consistent
cleanup handling across tests that use testContext and the onTestFinished API.
In `@controlplane/test/integrations.test.ts`:
- Line 45: Annotate the AVA test callbacks to use explicit parameter and return
types: change each async test callback from (testContext) => { ... } to
(testContext: any): Promise<void> => { ... } (or replace "any" with the correct
project TestContext type if available); update the occurrences for the 'test'
callbacks including the one with the description "Webhook meta for monograph and
federated graph should be stored and retrieved correctly" and the other test
callbacks at the other noted locations so every test callback has an explicit
parameter type for testContext and an explicit Promise<void> return type.
In `@controlplane/test/plugin/validate-and-fetch-plugin-data.test.ts`:
- Line 4: The test callbacks currently accept untyped testContext parameters and
lack explicit return types; update each callback signature used with vitest
functions (beforeAll, afterAll, test, onTestFinished) to annotate the first
parameter as TestContext and to have an explicit return type (use Promise<void>
for async callbacks or void for synchronous ones). For example, change any
occurrences like "beforeAll((testContext) => { ... })" or "test('...',
(testContext) => { ... })" to "beforeAll((testContext: TestContext):
Promise<void> => { ... })" or "...: void => { ... }" as appropriate, and add the
TestContext import from 'vitest' where needed so symbols like TestContext,
beforeAll, afterAll, test, and onTestFinished are properly typed. Ensure you
apply this to all listed callback locations (lines referenced in the review) so
every test callback has explicit parameter and return type annotations.
In `@controlplane/test/proposal/create-proposal.test.ts`:
- Line 354: The test callbacks like the one in create-proposal.test.ts (e.g.,
the test named "should create a proposal with multiple subgraph changes" and
other affected tests at the listed locations) use an untyped parameter
testContext; add explicit TypeScript parameter (and optional return) type
annotations (for example: testContext: TestContext or the project's preferred
test fixture type) to each callback signature so the parameter is typed
consistently across functions such as the test(...) callbacks at the mentioned
positions; update all occurrences (lines referenced: 354, 488, 583, 677, 714,
866, 882, 942, 1035, 1162, 1214, 1297, 1487, 1609, 1674, 1766) to the same
annotated form and ensure any imports for the TestContext type are added if
necessary.
In `@controlplane/test/proposal/update-proposal.test.ts`:
- Around line 34-54: The helpers enableProposalsForNamespace and
createTestProposal currently accept client: any — replace the any with a
concrete client type (import the exported test client type from your test
utilities if available) or define a small ProposalClient interface in the test
file that declares the exact methods used (e.g., enableProposalsForNamespace,
createProposal and any other invoked RPCs) with their argument and
Promise-return types; then update both function signatures to use that type so
the compiler enforces correct method shapes and return types when calling
enableProposalsForNamespace and createTestProposal.
In `@controlplane/test/restore-organization.test.ts`:
- Line 38: The test callback for test('should removed queued job and keep
organization', async (testContext) => { ... }) has an untyped parameter and
missing return type; change the signature to async (testContext: TestContext):
Promise<void> and ensure the TestContext type is imported from the test
framework (or local test types) so the parameter and the async return are
explicitly annotated.
In `@controlplane/test/subgraph/delete-subgraph.test.ts`:
- Line 196: The test callbacks that currently rely on inference need explicit
TypeScript annotations: update the test function signatures that accept the
parameter named testContext (e.g., in the test titled "that deleting a subgraph
also deletes any feature subgraphs for which it is the base subgraph" and the
other modified test) to declare the parameter type (such as TestContext or the
repository's test context interface) and add an explicit return type
(Promise<void> if async); locate the callbacks passed to test(...) and annotate
the parameter and the callback return type accordingly (e.g., (testContext:
YourTestContextType): Promise<void> => { ... }).
In `@controlplane/test/subgraph/get-subgraphs.test.ts`:
- Line 26: Update the tests that currently call await server.close() manually to
register cleanup via the testContext hook instead: add the testContext parameter
to the affected test functions (e.g., the "Should be able to list subgraphs of
different namespace" test and the other test that closes server manually),
remove the explicit await server.close(), and call testContext.onTestFinished(()
=> server.close()) to ensure server.close() runs even on failures; also modify
the test.each table-based test to accept testContext and use the same
onTestFinished cleanup pattern.
In `@controlplane/test/subgraph/update-subgraph.test.ts`:
- Around line 49-51: The explicit await server.close() at the end of the test is
redundant because server.close() is already registered via onTestFinished(() =>
server.close()); remove the final await server.close() statement to avoid
duplicate cleanup; search for the server variable and the onTestFinished
registration in update-subgraph.test.ts to locate and delete the extra await
server.close() call.
- Around line 74-76: The final explicit call to await server.close() is
redundant because testContext.onTestFinished(() => server.close()) already
schedules server shutdown; remove the trailing await server.close() from the
test to avoid duplicate cleanup and rely on the registered
testContext.onTestFinished teardown for the server instance referenced by server
and its server.close() method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
controlplane/test/proposal/proposal-federated-schema-breaking-changes.test.ts
Show resolved
Hide resolved
Contributor
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
Contributor
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
Noroth
pushed a commit
that referenced
this pull request
Mar 18, 2026
This was referenced Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Checklist