fix: version list endpoint respects limit param#1119
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe deployment version listing endpoint refactors CEL filtering from a fixed in-memory approach (1000 candidates) to bounded batch scanning with incremental CEL evaluation and pagination. The handler now uses a new helper that scans ChangesCEL Batch-Scan Deployment Version Listing
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant ListHelper as listDeploymentVersionsMatchingCel
participant DB as Database
participant CELEval as CEL Evaluator
Client->>Handler: GET /versions?cel=filter&limit=10&offset=0
Handler->>ListHelper: listDeploymentVersionsMatchingCel(cel, 10, 0, "desc")
loop Scan batches until 10 items or scan cap
ListHelper->>DB: SELECT * FROM deploymentVersion<br/>ORDER BY createdAt DESC<br/>LIMIT CEL_BATCH_SIZE OFFSET window_offset
DB-->>ListHelper: batch of rows
ListHelper->>CELEval: filterDeploymentVersions(batch, cel)
CELEval-->>ListHelper: matching rows
ListHelper->>ListHelper: accumulate items, increment total count
end
ListHelper-->>Handler: {items: [...10 matches], total: 750}
Handler-->>Client: {items, total, limit, offset}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 329-332: The ordering uses only schema.deploymentVersion.createdAt
which can produce unstable pagination; update the orderBy expression (the const
orderBy used around the deploymentVersion query) to include a deterministic
tiebreaker by adding the unique id field as a secondary sort key (e.g.
schema.deploymentVersion.id) in both branches (asc and desc). Apply the same
change to the other occurrence around lines 372-375 so both ascending and
descending sorts use [createdAt, id] (or equivalent multi-column ordering) to
ensure stable pagination.
- Around line 339-359: The loop that pages through deploymentVersion using
CEL_MAX_SCAN can stop early and return an incorrect total (it currently returns
{ items, total: matched })—update the function that iterates over
schema.deploymentVersion (the while loop using CEL_MAX_SCAN, CEL_BATCH_SIZE,
scanned, matched and filterDeploymentVersions) to detect when the scan cap was
hit (scanned >= CEL_MAX_SCAN or batch.length === CEL_BATCH_SIZE on final
iteration) and surface truncation instead of returning a potentially wrong
total: either return a nullable/unknown total (e.g., total: null) or add a
truncated flag (e.g., truncated: true) alongside total so callers know the count
is incomplete; ensure the change is applied where the function currently returns
{ items, total: matched } and document the new contract for consumers.
In `@e2e/tests/api/deployment-version-list.spec.ts`:
- Around line 15-47: Replace the imperative setup/teardown in
test.beforeAll/test.afterAll by declaring the base entities in a companion
.spec.yaml and using the existing importEntitiesFromYaml and
cleanupImportedEntities helpers; remove the manual POST to create systems (and
the manual DELETE loop and direct DB deletion of schema.deploymentVersion) and
instead call importEntitiesFromYaml at the start of the suite to obtain systemId
and deploymentIds, then call cleanupImportedEntities in afterAll to tear them
down. Ensure references to deploymentIds and systemId are populated from the
import response and remove any direct DB.delete(schema.deploymentVersion) usage
so cleanup is handled by cleanupImportedEntities.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7eff9208-3338-4f7e-b692-e7ea5aee9b34
📒 Files selected for processing (3)
apps/api/src/routes/v1/workspaces/deployments.tse2e/package.jsone2e/tests/api/deployment-version-list.spec.ts
| while (scanned < CEL_MAX_SCAN) { | ||
| const batch = await db | ||
| .select() | ||
| .from(schema.deploymentVersion) | ||
| .where(eq(schema.deploymentVersion.deploymentId, deploymentId)) | ||
| .orderBy(orderBy) | ||
| .limit(CEL_BATCH_SIZE) | ||
| .offset(scanned); | ||
| if (batch.length === 0) break; | ||
|
|
||
| for (const version of filterDeploymentVersions(batch, cel)) { | ||
| if (matched >= offset && matched < pageEnd) items.push(version); | ||
| matched++; | ||
| } | ||
|
|
||
| scanned += batch.length; | ||
| if (batch.length < CEL_BATCH_SIZE) break; | ||
| } | ||
|
|
||
| return { items, total: matched }; | ||
| } |
There was a problem hiding this comment.
CEL_MAX_SCAN can silently return partial total and incomplete pages.
When the scan cap is reached, the function returns total: matched as if complete, even though more matching rows may exist beyond the cap. That breaks total correctness and can hide valid results at higher offsets. Please surface truncation explicitly (or adopt a continuation strategy that preserves correctness).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/v1/workspaces/deployments.ts` around lines 339 - 359, The
loop that pages through deploymentVersion using CEL_MAX_SCAN can stop early and
return an incorrect total (it currently returns { items, total: matched
})—update the function that iterates over schema.deploymentVersion (the while
loop using CEL_MAX_SCAN, CEL_BATCH_SIZE, scanned, matched and
filterDeploymentVersions) to detect when the scan cap was hit (scanned >=
CEL_MAX_SCAN or batch.length === CEL_BATCH_SIZE on final iteration) and surface
truncation instead of returning a potentially wrong total: either return a
nullable/unknown total (e.g., total: null) or add a truncated flag (e.g.,
truncated: true) alongside total so callers know the count is incomplete; ensure
the change is applied where the function currently returns { items, total:
matched } and document the new contract for consumers.
| test.beforeAll(async ({ api, workspace }) => { | ||
| const systemRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/systems", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: `ver-list-system-${faker.string.alphanumeric(8)}` }, | ||
| }, | ||
| ); | ||
| expect(systemRes.response.status).toBe(202); | ||
| systemId = systemRes.data!.id; | ||
| }); | ||
|
|
||
| test.afterAll(async ({ api, workspace }) => { | ||
| // FK cascade from deployment → deployment_version was dropped in migration | ||
| // 0139, so versions inserted directly must be deleted explicitly before | ||
| // their owning deployment. | ||
| if (deploymentIds.length > 0) { | ||
| await db | ||
| .delete(schema.deploymentVersion) | ||
| .where(inArray(schema.deploymentVersion.deploymentId, deploymentIds)); | ||
|
|
||
| for (const deploymentId of deploymentIds) { | ||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", { | ||
| params: { path: { workspaceId: workspace.id, systemId } }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Align this E2E setup/teardown with YAML fixture workflow.
This spec currently provisions and cleans entities imperatively. Please migrate the base entities to .spec.yaml + importEntitiesFromYaml/cleanupImportedEntities to match the repo’s E2E fixture contract and reduce bespoke cleanup logic.
As per coding guidelines, "E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to tear them down".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/api/deployment-version-list.spec.ts` around lines 15 - 47, Replace
the imperative setup/teardown in test.beforeAll/test.afterAll by declaring the
base entities in a companion .spec.yaml and using the existing
importEntitiesFromYaml and cleanupImportedEntities helpers; remove the manual
POST to create systems (and the manual DELETE loop and direct DB deletion of
schema.deploymentVersion) and instead call importEntitiesFromYaml at the start
of the suite to obtain systemId and deploymentIds, then call
cleanupImportedEntities in afterAll to tear them down. Ensure references to
deploymentIds and systemId are populated from the import response and remove any
direct DB.delete(schema.deploymentVersion) usage so cleanup is handled by
cleanupImportedEntities.
There was a problem hiding this comment.
Pull request overview
Fixes the deployment version list endpoint behavior when cel is provided so that limit/offset are applied to the filtered result set (instead of limiting the DB query first and then filtering in-memory), addressing issue #1118.
Changes:
- Added batched scanning logic for CEL-filtered deployment version listing so paging is applied over matches.
- Added comprehensive Playwright E2E coverage for CEL paging (including multi-batch behavior) and non-CEL paging.
- Added
pnpmscripts to run the new deployment-version list E2E tests (including a heavy opt-in variant).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
apps/api/src/routes/v1/workspaces/deployments.ts |
Implements batched CEL scanning to make limit/offset apply to filtered matches and returns a match count. |
e2e/tests/api/deployment-version-list.spec.ts |
Adds E2E tests validating CEL paging correctness, ordering, totals, and isolation between deployments. |
e2e/package.json |
Adds convenience scripts to run the new E2E test suite (standard and heavy). |
Comments suppressed due to low confidence (1)
apps/api/src/routes/v1/workspaces/deployments.ts:381
- The non-CEL list path also queries/counts deployment_version rows using only deploymentId and ignores workspaceId from the route params. For tenant isolation consistency with other endpoints in this file, scope these queries to the requested workspace (e.g., join deployment and filter by deployment.workspaceId).
const { total } = await db
.select({ total: count() })
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId))
.then(takeFirst);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const batch = await db | ||
| .select() | ||
| .from(schema.deploymentVersion) | ||
| .where(eq(schema.deploymentVersion.deploymentId, deploymentId)) | ||
| .orderBy(orderBy) |
| const orderBy = | ||
| order === "asc" | ||
| ? asc(schema.deploymentVersion.createdAt) | ||
| : desc(schema.deploymentVersion.createdAt); | ||
|
|
| const CEL_BATCH_SIZE = 500; | ||
| const CEL_MAX_SCAN = 100_000; | ||
|
|
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", { | ||
| params: { path: { workspaceId: workspace.id, systemId } }, | ||
| }); |
fixes #1118
Summary by CodeRabbit
New Features
Tests