feat: add server-side pagination to operation overrides with limit and offset parameters#2695
Merged
JivusAyrus merged 5 commits intomainfrom Mar 26, 2026
Conversation
…d offset parameters
JivusAyrus
requested review from
StarpTech,
endigma,
thisisnithin and
wilsonrivera
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdded request pagination (limit, offset) and response total_count to GetAllOverrides; server normalizes pagination and forwards it to the repository, repository returns paginated rows plus total count; tests added; frontend switched to manual pagination using totalCount. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
comatory
reviewed
Mar 26, 2026
![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: 1
🧹 Nitpick comments (4)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/overrides.tsx (1)
53-55: Minor: Inconsistent parseInt usage.Line 53 uses
parseInt()while line 54 usesNumber.parseInt(). Both work identically, but consider using a consistent style throughout the file.♻️ Suggested consistency fix
- const pageNumber = router.query.page ? parseInt(router.query.page as string, 10) : 1; - const pageSize = Number.parseInt((router.query.pageSize as string) || '10'); + const pageNumber = router.query.page ? Number.parseInt(router.query.page as string, 10) : 1; + const pageSize = Number.parseInt((router.query.pageSize as string) || '10', 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/overrides.tsx around lines 53 - 55, The file uses parseInt in pageNumber and Number.parseInt in pageSize; make them consistent by using the same parse function everywhere (e.g., replace Number.parseInt((router.query.pageSize as string) || '10') with parseInt((router.query.pageSize as string) || '10', 10) or switch pageNumber to Number.parseInt), ensuring both calls include the radix and reference the same router.query keys (pageNumber, pageSize).controlplane/test/override.test.ts (1)
244-246: AnnotatetestContextexplicitly.The new callback adds an inferred parameter type. Please type it as
TestContextso this stays aligned with the project’s TypeScript rule.Possible change
-import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, Mock } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi, Mock, TestContext } from 'vitest'; ... - test('Should paginate overrides with limit and offset', async (testContext) => { + test('Should paginate overrides with limit and offset', async (testContext: TestContext) => {As per coding guidelines, "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/override.test.ts` around lines 244 - 246, The test callback's parameter is currently untyped (async (testContext) => ...); change it to an explicit type by annotating the parameter as TestContext (async (testContext: TestContext) => ...) in the 'Should paginate overrides with limit and offset' test, and add or ensure the appropriate TestContext import is present (e.g., import type { TestContext } from 'ava' or your test framework) so the parameter type is explicit and satisfies the TypeScript rule.controlplane/src/core/repositories/OperationsRepository.ts (2)
511-515: Keep the page rows andtotalCounton the same snapshot.Lines 511-515 execute two independent selects. If an override is added or removed between them, the returned
overridesandtotalCountcan disagree for the same response. If that pair is expected to be internally consistent, run them in a transaction or fold the count into the paged query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` around lines 511 - 515, The overrides and totalCount are fetched by two independent queries (baseQuery and countQuery) which can yield inconsistent snapshots; in OperationsRepository make them consistent by executing both inside a single transaction or by folding the count into the paged query (e.g. use COUNT(*) OVER() or a single query that returns rows plus total) so that the returned overrides and totalCount are from the same snapshot; update the code that currently awaits Promise.all([baseQuery, countQuery]) and then returns { overrides, totalCount } to instead run the two operations in a transaction or replace them with a single paged query that produces both the rows and the totalCount.
446-446: Give this public method an explicit return contract.
getConsolidatedOverridesViewnow returns a paginated payload, but its shape is still inferred from the Drizzle query. A named interface plus an explicitPromise<...>return type will keep SQL inference from leaking into callers.Possible change
+interface ConsolidatedOverrideRow { + hash: string; + name: string; + updatedAt: string; + hasIgnoreAllOverride: boolean; + changesOverrideCount: number; +} + +interface ConsolidatedOverridesView { + overrides: ConsolidatedOverrideRow[]; + totalCount: number; +} ... - public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number }) { + public async getConsolidatedOverridesView( + data: { namespaceId: string; limit: number; offset: number }, + ): Promise<ConsolidatedOverridesView> {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 `@controlplane/src/core/repositories/OperationsRepository.ts` at line 446, The public method getConsolidatedOverridesView should declare an explicit interface for the paginated payload and use it as the function's return type to avoid leaking Drizzle SQL inference; create a named interface (e.g., ConsolidatedOverridesView and ConsolidatedOverridesViewPage) describing the object shape (items array and pagination metadata like limit/offset/total) and update the method signature to public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number }): Promise<ConsolidatedOverridesViewPage> so callers consume a stable, documented contract instead of an inferred type.
🤖 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/override.test.ts`:
- Around line 283-310: The pagination test currently only checks totalCount and
lengths—add explicit assertions that the returned override identities/hashes
match expected items to ensure offset/limit are applied correctly: after calling
client.getAllOverrides (variables allRes, page1, page2) assert the arrays of
override hashes (e.g. map each .overrides to .hash or unique id) equal the
expected ordered arrays (allRes hashes contain all three in insertion order,
page1 hashes equal the first two, page2 hashes equal the third) so handlers that
ignore offset/limit or apply defaults will fail.
---
Nitpick comments:
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 511-515: The overrides and totalCount are fetched by two
independent queries (baseQuery and countQuery) which can yield inconsistent
snapshots; in OperationsRepository make them consistent by executing both inside
a single transaction or by folding the count into the paged query (e.g. use
COUNT(*) OVER() or a single query that returns rows plus total) so that the
returned overrides and totalCount are from the same snapshot; update the code
that currently awaits Promise.all([baseQuery, countQuery]) and then returns {
overrides, totalCount } to instead run the two operations in a transaction or
replace them with a single paged query that produces both the rows and the
totalCount.
- Line 446: The public method getConsolidatedOverridesView should declare an
explicit interface for the paginated payload and use it as the function's return
type to avoid leaking Drizzle SQL inference; create a named interface (e.g.,
ConsolidatedOverridesView and ConsolidatedOverridesViewPage) describing the
object shape (items array and pagination metadata like limit/offset/total) and
update the method signature to public async getConsolidatedOverridesView(data: {
namespaceId: string; limit: number; offset: number }):
Promise<ConsolidatedOverridesViewPage> so callers consume a stable, documented
contract instead of an inferred type.
In `@controlplane/test/override.test.ts`:
- Around line 244-246: The test callback's parameter is currently untyped (async
(testContext) => ...); change it to an explicit type by annotating the parameter
as TestContext (async (testContext: TestContext) => ...) in the 'Should paginate
overrides with limit and offset' test, and add or ensure the appropriate
TestContext import is present (e.g., import type { TestContext } from 'ava' or
your test framework) so the parameter type is explicit and satisfies the
TypeScript rule.
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/overrides.tsx:
- Around line 53-55: The file uses parseInt in pageNumber and Number.parseInt in
pageSize; make them consistent by using the same parse function everywhere
(e.g., replace Number.parseInt((router.query.pageSize as string) || '10') with
parseInt((router.query.pageSize as string) || '10', 10) or switch pageNumber to
Number.parseInt), ensuring both calls include the radix and reference the same
router.query keys (pageNumber, pageSize).
🪄 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: 0113656e-1410-4438-9073-100a422b0a48
⛔ 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 (6)
connect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/check/getAllOverrides.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/override.test.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/overrides.tsx
comatory
approved these changes
Mar 26, 2026
thisisnithin
approved these changes
Mar 26, 2026
…ersistedOperations
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controlplane/src/core/repositories/OperationsRepository.ts (2)
446-446: Add explicit return type annotation.The method is missing an explicit return type annotation. As per coding guidelines, TypeScript functions should have explicit type annotations for return types.
Proposed fix
- public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number }) { + public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number }): Promise<{ + overrides: { + hash: string; + name: string; + updatedAt: string; + hasIgnoreAllOverride: boolean; + changesOverrideCount: number; + }[]; + totalCount: number; + }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` at line 446, The method getConsolidatedOverridesView is missing an explicit return type; update its signature to include the correct TypeScript return annotation (e.g., change "public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number })" to "public async getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset: number }): Promise<YourReturnType>" where YourReturnType matches the resolved value the function returns (use the existing domain type like ConsolidatedOverridesView[] or an appropriate interface/Promise<any> if necessary).
505-510: Cast count to int for consistency and type safety.The existing code on line 495 uses
cast(...as int)to ensure numeric types from PostgreSQL aggregate functions. PostgreSQL'sCOUNTreturnsbigint, and the postgres-js driver may return this as a string. For consistency with the established pattern and to ensure proper typing for the protoint32 total_countfield, apply the same casting approach here.Proposed fix
// For pagination, we need the total count of unique operations that have an override. This is obtained by counting the full join of the two tables. const countQuery = this.db .select({ - count: count(), + count: sql<number>`cast(count(*) as int)`, }) .from(change) .fullJoin(ignore, and(eq(change.hash, ignore.hash), eq(change.namespaceId, ignore.namespaceId)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` around lines 505 - 510, The count query in OperationsRepository building `countQuery` uses count() which yields a bigint/string from Postgres; change the select to cast the aggregate to int for consistency with the existing pattern (as used earlier on line ~495). Replace the select object to use cast(count(), 'int') (or the codebase's cast(... as int) helper) so the selected field remains an integer for the proto `int32 total_count` (references: OperationsRepository, countQuery, change, ignore, count()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Line 446: The method getConsolidatedOverridesView is missing an explicit
return type; update its signature to include the correct TypeScript return
annotation (e.g., change "public async getConsolidatedOverridesView(data: {
namespaceId: string; limit: number; offset: number })" to "public async
getConsolidatedOverridesView(data: { namespaceId: string; limit: number; offset:
number }): Promise<YourReturnType>" where YourReturnType matches the resolved
value the function returns (use the existing domain type like
ConsolidatedOverridesView[] or an appropriate interface/Promise<any> if
necessary).
- Around line 505-510: The count query in OperationsRepository building
`countQuery` uses count() which yields a bigint/string from Postgres; change the
select to cast the aggregate to int for consistency with the existing pattern
(as used earlier on line ~495). Replace the select object to use cast(count(),
'int') (or the codebase's cast(... as int) helper) so the selected field remains
an integer for the proto `int32 total_count` (references: OperationsRepository,
countQuery, change, ignore, count()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f40f79e-d04f-4fcf-885c-c304860c6d61
📒 Files selected for processing (1)
controlplane/src/core/repositories/OperationsRepository.ts
…tion-to-operation-overrides
JivusAyrus
deleted the
suvij/eng-9297-studio-add-server-side-pagination-to-operation-overrides
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
Checklist