fix: limit the number of entries shown in the table for schema checks on the cli #2417
Conversation
WalkthroughAdds a row/entry limit to schema checks: CLI option (-l/--limit) with validation, proto request field, control-plane enforcement and slicing, counts aggregation in responses, CLI truncation messaging, and regenerated protobuf bindings updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
SkArchon
requested review from
Aenimus,
JivusAyrus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2417 +/- ##
===========================================
- Coverage 90.55% 35.92% -54.63%
===========================================
Files 16 943 +927
Lines 3271 123021 +119750
Branches 871 4872 +4001
===========================================
+ Hits 2962 44200 +41238
- Misses 309 77265 +76956
- Partials 0 1556 +1556
🚀 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/src/commands/subgraph/commands/check.ts (1)
56-59: Consider extracting common limit validation logic.The limit validation logic (lines 56-59) is duplicated in
cli/src/commands/graph/monograph/commands/check.ts(lines 38-41). Consider extracting this into a shared utility function to follow the DRY principle.Example shared utility:
// In cli/src/core/validation.ts or similar export function validateCheckLimit(limit: string, maxLimit: number): number { const parsed = Number(limit); if (Number.isNaN(parsed) || parsed <= 0 || parsed > maxLimit) { program.error(pc.red(`The limit must be a valid number between 1 and ${maxLimit}. Received: '${limit}'`)); } return parsed; }Then use it in both check commands:
const limit = validateCheckLimit(options.limit, maxLimit);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (8)
cli/src/commands/graph/monograph/commands/check.ts(4 hunks)cli/src/commands/subgraph/commands/check.ts(4 hunks)cli/src/handle-check-result.ts(2 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(5 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(8 hunks)controlplane/src/core/util.ts(1 hunks)proto/README.md(1 hunks)proto/wg/cosmo/platform/v1/platform.proto(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2331
File: studio/src/components/operations/client-usage-table.tsx:65-65
Timestamp: 2025-11-19T11:26:26.657Z
Learning: In the Cosmo studio codebase (studio/src/components/operations/client-usage-table.tsx), converting BigInt request counts from the API to Number is acceptable because request counts are not expected to exceed JavaScript's safe integer limit (2^53 - 1).
📚 Learning: 2025-08-29T10:10:06.969Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/[checkId]/index.tsx:0-0
Timestamp: 2025-08-29T10:10:06.969Z
Learning: The isCheckSuccessful function in both controlplane/src/core/util.ts and studio/src/components/check-badge-icon.tsx includes early exits for linked check validation. It returns false immediately if either isLinkedTrafficCheckFailed or isLinkedPruningCheckFailed is true, which means linked check failures automatically cause the overall check to fail.
Applied to files:
cli/src/handle-check-result.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
📚 Learning: 2025-09-10T09:59:17.257Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:59:17.257Z
Learning: When skipTrafficCheck is true in schema checks, the traffic inspection loop is completely bypassed (as shown in line 2040 of SubgraphRepository.ts with the continue statement), which means hasClientTraffic will be false. The traffic analysis doesn't run at all when skipTrafficCheck is true, unlike the previous understanding that it always ran to collect usage data.
Applied to files:
cli/src/commands/subgraph/commands/check.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
📚 Learning: 2025-09-10T09:52:46.326Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts:417-419
Timestamp: 2025-09-10T09:52:46.326Z
Learning: When skipTrafficCheck is true in schema checks, traffic analysis still runs to collect usage data, but hasClientTraffic can still be true if unsafe client traffic is detected. The difference is that traffic issues don't cause the overall check to fail, but the hasClientTraffic flag itself remains accurate based on the actual traffic analysis results.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
🧬 Code graph analysis (5)
cli/src/handle-check-result.ts (1)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
CheckSubgraphSchemaResponse(2237-2383)
cli/src/commands/subgraph/commands/check.ts (1)
cli/src/handle-check-result.ts (1)
handleCheckResult(9-324)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (3)
SchemaCheckCounts(3256-3269)SchemaCheckCounts(3284-3284)SchemaCheckCounts(3299-3301)
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (1)
controlplane/src/core/util.ts (2)
clamp(578-580)limitCombinedArrays(487-507)
cli/src/commands/graph/monograph/commands/check.ts (1)
cli/src/handle-check-result.ts (1)
handleCheckResult(9-324)
🪛 GitHub Actions: wgc CI
cli/src/handle-check-result.ts
[error] 264-264: ESLint: 'eqeqeq' rule violated. Expected '!==' and instead saw '!='.
🪛 GitHub Check: build_test_node_matrix (20.x)
cli/src/handle-check-result.ts
[failure] 264-264:
Expected '!==' and instead saw '!='
🪛 GitHub Check: build_test_node_matrix (22.x)
cli/src/handle-check-result.ts
[failure] 264-264:
Expected '!==' and instead saw '!='
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
proto/README.md (1)
5-5: Documentation update is accurate and helpful.The
make generatecommand is verified in the Makefile, and both output paths (../connect for TypeScript and ../router/gen for Go) are correctly referenced from the proto/ directory. This documentation addition appropriately guides developers to regenerate protobuf bindings when proto definitions change, which is timely given the new proto fields (limit, counts, SchemaCheckCounts) added in this PR.cli/src/commands/subgraph/commands/check.ts (1)
13-14: LGTM! Limit configuration is well-defined.The maximum limit of 10,000 and default of 50 provide reasonable bounds for schema check output. The implementation correctly integrates with the existing CLI option pattern.
Also applies to: 36-36
cli/src/handle-check-result.ts (1)
9-9: Good implementation of row limiting with user feedback.The addition of the
rowLimitparameter and truncation messaging provides users with clear feedback when results are limited. The logic correctly evaluates grouped counts and provides helpful guidance to view full results in the studio dashboard.Note: This approval is contingent on fixing the
!=vs!==issue on Line 264.Also applies to: 252-268, 277-277, 285-285
proto/wg/cosmo/platform/v1/platform.proto (1)
106-106: LGTM! Protocol buffer definitions are well-structured.The new
limitfield andSchemaCheckCountsmessage properly extend the schema check API with count metadata. The field numbers are properly assigned and types are appropriate for their purposes.Also applies to: 288-300
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (3)
255-255: Verify limit defaults and maximum values are aligned.There's a discrepancy between the CLI and backend limit constraints:
- CLI default: 50, max: 10,000
- Backend default: 1, max: 100,000
This means the backend would accept limits up to 100,000 even though the CLI caps at 10,000. Consider whether these values should be aligned, or if the backend intentionally allows a higher maximum for other API clients.
Additionally, a default of 1 when no limit is provided seems quite restrictive. Was this intentional, or should it match the CLI default of 50?
291-307: LGTM! Array limiting logic correctly implements grouped behavior.The implementation correctly applies limits:
- Composition errors and warnings are limited separately (they appear in separate CLI tables)
- Breaking/non-breaking changes are grouped (limited together with priority to breaking)
- Lint errors/warnings are grouped (limited together with priority to errors)
- Graph prune errors/warnings are grouped (limited together with priority to errors)
This matches the PR objectives and provides sensible prioritization when limits are exceeded.
310-319: LGTM! Counts object correctly tracks original result sizes.The
countsobject properly captures the total counts fromcheckResultbefore any slicing occurs. This ensures that clients can detect when results have been truncated by comparing counts with the actual array lengths in the response.connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
1102-1105: LGTM! Generated protobuf code correctly implements the schema check limit and counts.The changes add:
- Optional
limitfield toCheckSubgraphSchemaRequestfor controlling result set size- Optional
countsfield toCheckSubgraphSchemaResponsefor returning total counts- New
SchemaCheckCountsmessage with 8 count fields matching the Go counterpartField numbers are consistent across TypeScript and Go implementations, and all structures follow standard protobuf-ts patterns.
Also applies to: 1124-1124, 2334-2337, 2365-2365, 2385-2462
…d-prune-issues-displayed-on-the
SkArchon
changed the title
…d-prune-issues-displayed-on-the
| isCheckExtensionSkipped, | ||
| checkExtensionErrorMessage, | ||
| } = await subgraphRepo.performSchemaCheck({ | ||
| const returnLimit = clamp(req.limit ?? defaultRowLimit, 1, maxRowLimit); |
There was a problem hiding this comment.
i feel if the limit is not passed, return everything
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR does the following
Introduces a limit parameter for schema checks to pass in a limit, we still do use a default limit if a value was not passed in, we also set a max number.
When determining if the limit was exceeded, we treat the following values as one grouping
We however DO NOT treat
compositionErrorsandcompositionWarningsas one entry because they are displayed in two tables on the CLI at the moment.However when returning the counts, we return values for every individual attribute, which can be found in the
SchemaCheckCountsdefinition. This includes for examplelintWarningsandlintErrorsbeing separate attributes, as they are part of the API, which would be harder to change later if we addlintWarningsAndErrorsand want to break it into two values.The CLI also has a new option for changing the limit with a default value, if there are any truncated entries we mention to the user in the CLI output and ask the user to refer the studio dashboard.
Note: This PR does not introduce pagination to studio.
TODO: Look at Control Panel tests
Summary by CodeRabbit
New Features
--limit(-l) option to schema check commands (default 50, max 10,000) with input validation.Documentation
make generate) in project docs.✏️ Tip: You can customize this high-level summary in your review settings.
Checklist