-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add cursor based pagination #3184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR centralizes pagination by adding getPaginationOptions (cursor + offset), updates Zod query schemas to expose cursor pagination (marking offset deprecated), refactors API callsites to use the helper, adds composite DB indexes including id as a tiebreaker, and adds comprehensive pagination tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Web API
participant Pager as getPaginationOptions
participant DB as Prisma/Database
Client->>API: GET /... with query params (page,pageSize,startingAfter,endingBefore,sortBy,...)
API->>Pager: parse filters → getPaginationOptions(filters)
Pager-->>API: pagination options (take, skip or cursor, orderBy, cursorId)
API->>DB: Prisma.findMany({...filters..., ...paginationOptions})
DB-->>API: results
API-->>Client: 200 + items (pagination metadata if applicable)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas needing attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/tests/links/list-links.test.ts (1)
9-42: Fix temporal dead zone inGET /linkstest cleanup
onTestFinishedcloses overfirstLinkbefore it is declared:
- Line 15–17 use
firstLink.idin the cleanup.const { data: firstLink } = ...is only defined starting at line 19.Because
constbindings are in the temporal dead zone until their declaration runs, this will throw aReferenceErrorwhen the cleanup callback is registered or executed.A simple fix is to register the cleanup after
firstLinkis created:- onTestFinished(async () => { - await h.deleteLink(firstLink.id); - }); - - const { data: firstLink } = await http.post<Link>({ + const { data: firstLink } = await http.post<Link>({ path: "/links", body: { url, domain }, }); + + onTestFinished(async () => { + await h.deleteLink(firstLink.id); + });This keeps the behavior identical while avoiding the TDZ issue.
🧹 Nitpick comments (11)
packages/prisma/schema/commission.prisma (1)
48-58: New(programId, createdAt, id)index aligns with cursor-pagination access patternThis index matches the expected ordering for cursor-based pagination by
createdAtwithidas a tiebreaker under a givenprogramId, which should improve query performance and stability. You still retain the more complex[programId, createdAt, status, amount, earnings]index for other queries; you can revisit index overlap later if needed.apps/web/lib/zod/schemas/links.ts (1)
7-18: Cursor + deprecated page-based pagination composition ingetLinksQuerySchemaBaselooks soundMerging
getCursorPaginationQuerySchemaahead of the deprecatedgetPaginationQuerySchemacleanly exposes both cursor params (startingAfter,endingBefore) and legacypage/pageSize, which matches howgetPaginationOptionsexpects filters.One minor follow-up to consider:
linksExportQuerySchemaomitspageandpageSizebut still inheritsstartingAfter/endingBeforefromgetLinksQuerySchemaBase. If the export endpoint never uses cursor pagination, you might want to omit those as well for clearer docs and validation, or explicitly document that they’re ignored for exports.Also applies to: 160-179, 272-292
apps/web/lib/zod/schemas/customers.ts (1)
1-8: Customers query schema now cleanly supports cursor + deprecated page-based pagination
getCustomersQuerySchemacorrectly layers:
- core filters and sort fields,
- cursor pagination (
startingAfter/endingBeforewith a concrete example), and- deprecated page-based pagination with
CUSTOMERS_MAX_PAGE_SIZE.That matches the expectations of
getPaginationOptionsand clearly nudges clients towards cursor-based pagination.For
getCustomersCountQuerySchema, you omitpage,pageSize,sortBy, andsortOrder, but cursor params still pass through. If the/customers/countendpoint never uses cursors, you might eventually tighten this by omittingstartingAfter/endingBeforeas well so the count contract is unambiguously “no pagination here”.Also applies to: 13-73, 85-93
apps/web/lib/zod/schemas/commissions.ts (1)
131-136: Consider omitting cursor fields from count/export query schemas (optional)
getCommissionsCountQuerySchemaandcommissionsExportQuerySchemastill allowstartingAfter/endingBeforeviagetCommissionsQuerySchema, but those values are typically irrelevant for a global count or full export. If these endpoints never use cursor pagination, consider omitting cursor fields here as well to make the API surface clearer and avoid confusing no-op parameters.Also applies to: 301-325
apps/web/tests/customers/pagination.test.ts (2)
18-30: Tests assume at least 10 customers in the E2E workspaceSeveral tests access
baseline[4],baseline[5], and compare slices up to index 10. This is fine with the current seeded data, but will start failing if the fixture workspace ever has fewer than 10 customers. If you want the suite to be more robust to fixture changes, you could either seed the required minimum inside the tests or explicitly assertbaseline.length >= 10with a clear failure message.Also applies to: 32-78, 163-255
258-277: Deduplicate pagination helper assertions across test suites (optional)
expectSortedByCreatedAt,expectSortedByCreatedAtAsc, andexpectNoOverlapare mirrored in the commissions pagination tests. Consider moving these helpers into a shared test utility (for example undertests/utils/pagination.ts) to avoid duplication and keep future changes to pagination assertions in one place.apps/web/lib/api/links/get-links-for-workspace.ts (1)
17-39: Relying on parsed filters for pagination fields (optional clarification)
getPaginationOptionsassumespage,pageSize,sortBy, andsortOrderare present and valid onfilters. As long as all callers pass in objects produced bygetLinksQuerySchemaExtended.parse, that’s guaranteed via Zod defaults and enums. If this function is ever reused with unvalidated inputs, you may want to either export and reuse theFilterstype frompagination.tsor document that pre-validation is required.Also applies to: 156-157
apps/web/lib/api/pagination.ts (1)
4-20: Document or generalize theid-based cursor assumption (optional)
PaginationOptions.cursorand the secondaryorderBykey are hard-coded toid, which matches your current models but is an implicit requirement. If you ever paginate on a model whose cursor column is notid, you’ll need to extend this helper. Consider either documenting this assumption in a comment or parameterizing the cursor field name in the future.Also applies to: 24-55
apps/web/lib/zod/schemas/misc.ts (1)
31-66: Optional: push mutual-exclusion check into Zod as wellYou currently enforce
startingAfter/endingBeforemutual exclusivity ingetPaginationOptions, which is sufficient for runtime behavior. If you want clients of the Zod schema alone (e.g. non-HTTP callers) to see validation errors earlier, you could add a.refineongetCursorPaginationQuerySchemato reject payloads that specify both fields.Also applies to: 68-93
apps/web/tests/commissions/pagination.test.ts (2)
18-29: Tests rely on having at least 10 commissions in the E2E workspaceAs with the customers suite, several expectations depend on
baseline[4],baseline[5], and slices up to index 10. That’s fine with current fixtures but will break if the seeded workspace ever has fewer commissions. If you’d like these tests to be more resilient, you could either seed the necessary minimum inside the suite or assertbaseline.length >= 10with a clear failure reason.Also applies to: 31-77, 180-280
283-302: Extract shared pagination assertion helpers (optional)
expectSortedByCreatedAt,expectSortedByCreatedAtAsc, andexpectNoOverlapare duplicated from the customers pagination tests. Pulling them into a shared test helper module would reduce duplication and keep pagination-specific assertions consistent across suites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/app/(ee)/api/customers/route.ts(3 hunks)apps/web/lib/api/commissions/get-commissions.ts(2 hunks)apps/web/lib/api/links/get-links-for-workspace.ts(3 hunks)apps/web/lib/api/pagination.ts(1 hunks)apps/web/lib/zod/schemas/commissions.ts(2 hunks)apps/web/lib/zod/schemas/customers.ts(2 hunks)apps/web/lib/zod/schemas/links.ts(2 hunks)apps/web/lib/zod/schemas/misc.ts(2 hunks)apps/web/tests/commissions/pagination.test.ts(1 hunks)apps/web/tests/customers/pagination.test.ts(1 hunks)apps/web/tests/links/list-links.test.ts(2 hunks)packages/prisma/schema/commission.prisma(1 hunks)packages/prisma/schema/customer.prisma(1 hunks)packages/prisma/schema/link.prisma(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-24T16:13:00.387Z
Learnt from: TWilson023
Repo: dubinc/dub PR: 2872
File: packages/prisma/schema/partner.prisma:151-153
Timestamp: 2025-09-24T16:13:00.387Z
Learning: In the Dub codebase, Prisma schemas use single-column indexes without brackets (e.g., `@index(partnerId)`) and multi-column indexes with brackets (e.g., `@index([programId, partnerId])`). This syntax pattern is consistently used throughout their schema files and works correctly with their Prisma version.
Applied to files:
packages/prisma/schema/commission.prismapackages/prisma/schema/link.prismapackages/prisma/schema/customer.prisma
📚 Learning: 2025-06-06T07:59:03.120Z
Learnt from: devkiran
Repo: dubinc/dub PR: 2177
File: apps/web/lib/api/links/bulk-create-links.ts:66-84
Timestamp: 2025-06-06T07:59:03.120Z
Learning: In apps/web/lib/api/links/bulk-create-links.ts, the team accepts the risk of potential undefined results from links.find() operations when building invalidLinks arrays, because existing links are fetched from the database based on the input links, so matches are expected to always exist.
Applied to files:
packages/prisma/schema/link.prismaapps/web/lib/zod/schemas/links.tsapps/web/lib/api/links/get-links-for-workspace.tsapps/web/tests/links/list-links.test.ts
📚 Learning: 2025-10-06T15:48:45.956Z
Learnt from: TWilson023
Repo: dubinc/dub PR: 2935
File: packages/prisma/schema/workspace.prisma:21-36
Timestamp: 2025-10-06T15:48:45.956Z
Learning: In the Dub repository (dubinc/dub), Prisma schema changes are not managed with separate migration files. Do not flag missing Prisma migration files when schema changes are made to files like `packages/prisma/schema/workspace.prisma` or other schema files.
Applied to files:
packages/prisma/schema/customer.prisma
📚 Learning: 2025-10-17T08:18:19.278Z
Learnt from: devkiran
Repo: dubinc/dub PR: 0
File: :0-0
Timestamp: 2025-10-17T08:18:19.278Z
Learning: In the apps/web codebase, `@/lib/zod` should only be used for places that need OpenAPI extended zod schema. All other places should import from the standard `zod` package directly using `import { z } from "zod"`.
Applied to files:
apps/web/lib/api/links/get-links-for-workspace.tsapps/web/lib/zod/schemas/customers.ts
🧬 Code graph analysis (9)
apps/web/app/(ee)/api/customers/route.ts (2)
apps/web/lib/zod/schemas/customers.ts (1)
getCustomersQuerySchemaExtended(75-83)apps/web/lib/api/pagination.ts (1)
getPaginationOptions(24-84)
apps/web/tests/customers/pagination.test.ts (2)
apps/web/tests/utils/integration.ts (1)
IntegrationHarness(14-118)apps/web/lib/types.ts (1)
Customer(426-426)
apps/web/lib/api/pagination.ts (2)
packages/prisma/client.ts (1)
Prisma(30-30)apps/web/lib/api/errors.ts (1)
DubApiError(58-75)
apps/web/lib/api/commissions/get-commissions.ts (1)
apps/web/lib/api/pagination.ts (1)
getPaginationOptions(24-84)
apps/web/lib/zod/schemas/links.ts (1)
apps/web/lib/zod/schemas/misc.ts (2)
getCursorPaginationQuerySchema(69-93)getPaginationQuerySchema(32-66)
apps/web/lib/zod/schemas/commissions.ts (1)
apps/web/lib/zod/schemas/misc.ts (2)
getCursorPaginationQuerySchema(69-93)getPaginationQuerySchema(32-66)
apps/web/lib/api/links/get-links-for-workspace.ts (1)
apps/web/lib/api/pagination.ts (1)
getPaginationOptions(24-84)
apps/web/tests/links/list-links.test.ts (1)
apps/web/tests/utils/integration.ts (1)
IntegrationHarness(14-118)
apps/web/tests/commissions/pagination.test.ts (2)
apps/web/tests/utils/integration.ts (1)
IntegrationHarness(14-118)apps/web/lib/types.ts (1)
CommissionResponse(443-443)
🔇 Additional comments (10)
packages/prisma/schema/link.prisma (1)
94-103: Addingidas tiebreaker in composite index for cursor pagination looks correctIncluding
idaftercreatedAt(sort: Desc)makes ordering deterministic and aligns with the new cursor-based pagination that sorts bycreatedAtthenid. Syntax and column order are consistent with existing Prisma index patterns.packages/prisma/schema/customer.prisma (1)
27-35: Extending customer index withidis consistent with cursor pagination requirementsUpdating the index to
[projectId, createdAt, id]gives deterministic ordering and supports keyset pagination oncreatedAtwithidas a tiebreaker, without affecting existing filters.apps/web/lib/api/commissions/get-commissions.ts (1)
6-10: Switch togetPaginationOptionscleanly centralizes commissions paginationDelegating
skip/takeandorderBytogetPaginationOptions(filters)keeps this query focused on filtering and inclusion logic, and aligns it with the new cursor-based pagination behavior (deterministic sort bysortBy+id). Just ensuregetCommissionsQuerySchemacontinues to define sensible defaults forpage,pageSize,sortBy, andsortOrdersofiltersalways satisfy the helper’s expectations.Also applies to: 35-73
apps/web/app/(ee)/api/customers/route.ts (1)
4-15: Unified pagination viagetPaginationOptionsin customers route looks correctParsing
filtersonce withgetCustomersQuerySchemaExtendedand then spreading...getPaginationOptions(filters)intoprisma.customer.findManyis a clean refactor:
- Keeps filtering logic focused on business fields (
externalId,search,country,linkId,customerIds).- Delegates page vs cursor behavior and deterministic ordering (
sortBy+id) to a shared helper.- Lines up with the new customer index on
[projectId, createdAt, id]for the default sort.Just ensure that any future changes to the customers pagination schema (e.g., new sort fields) are mirrored in the Prisma indexes where needed to preserve performance characteristics.
Also applies to: 40-83
apps/web/lib/zod/schemas/commissions.ts (1)
5-8: Cursor + offset pagination schema layering looks consistentMerging the cursor schema first and the paginated (deprecated) page/pageSize schema afterwards cleanly exposes both pagination styles, and the dedicated
COMMISSIONS_MAX_PAGE_SIZEkeeps the per-endpoint limit explicit. No functional issues spotted.Also applies to: 56-129
apps/web/tests/customers/pagination.test.ts (1)
5-207: Strong end-to-end coverage of new pagination behaviorThis suite does a good job validating offset vs cursor pagination, mixed-parameter precedence, sort order in both directions, and error handling for invalid combinations and large page values. The use of a common baseline plus ID-slice comparisons makes regressions around ordering and overlap easy to catch.
apps/web/lib/api/links/get-links-for-workspace.ts (1)
17-39: Centralizing link pagination viagetPaginationOptionslooks goodSwitching to a single
filtersparameter and spreadinggetPaginationOptions(filters)into the Prisma query cleanly delegates all pagination and ordering concerns to the shared helper while preserving the existing filtering logic (tags, folders, search, dates, etc.). This keeps the endpoint aligned with the new pagination behavior used by other routes.Also applies to: 41-42, 60-157
apps/web/lib/api/pagination.ts (1)
4-12: Shared pagination helper correctly models offset + cursor semanticsThe
getPaginationOptionsimplementation cleanly handles:
- Mutual exclusion of
startingAfter/endingBeforewith a clearDubApiError.- Enforcing a maximum page value.
- Determining
takeandskipfor both offset and cursor modes.- Using a stable two-field
orderBy(primarysortBy, secondaryid) for deterministic pagination.This should be a solid foundation for reusing across different Prisma models that have an
idprimary key.Also applies to: 22-84
apps/web/lib/zod/schemas/misc.ts (1)
31-66: Pagination + cursor Zod schemas are well-structured and composableThe extended
getPaginationQuerySchemaoptions (custompageSize,deprecatedflag) and the newgetCursorPaginationQuerySchemagive you a nice building block for endpoints that need both legacy page-based pagination and the new cursor style, with clear OpenAPI docs and examples. No issues spotted with the Zod setup.Also applies to: 68-93
apps/web/tests/commissions/pagination.test.ts (1)
5-280: Commissions pagination behavior is thoroughly coveredThis suite mirrors the customers pagination tests and gives solid coverage for offset vs cursor flows, mixed-parameter precedence, invalid cursor combinations, large page values, and both sort directions. Using a shared baseline and ID-slice checks should make any future regressions in ordering or pagination logic very visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/tests/customers/pagination.test.ts (1)
52-65: Consider adding explicit sort parameters for deterministic results.The cursor-based pagination tests (lines 52-65, 67-79, 138-149, 152-162) omit
sortByandsortOrderparameters while comparing results against a baseline fetched with explicitsortBy: "createdAt"andsortOrder: "desc". If the API's default sort order differs or changes, these tests could become flaky.For example, update the startingAfter test:
test("Cursor forward (startingAfter)", async () => { const firstPage = baseline.slice(0, 5); const lastId = firstPage[4].id; const { status, data } = await http.get<Customer[]>({ path: "/customers", - query: { pageSize: "5", startingAfter: lastId }, + query: { + pageSize: "5", + startingAfter: lastId, + sortBy: "createdAt", + sortOrder: "desc" + }, }); expect(status).toEqual(200); expectSortedByCreatedAt(data); expect(data.map((c) => c.id)).toEqual(baselineIds.slice(5, 10)); });Apply similar changes to the other cursor-based tests for consistency.
Also applies to: 67-79, 138-149, 152-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/tests/commissions/pagination.test.ts(1 hunks)apps/web/tests/customers/pagination.test.ts(1 hunks)apps/web/tests/links/list-links.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/tests/links/list-links.test.ts
- apps/web/tests/commissions/pagination.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/tests/customers/pagination.test.ts (2)
apps/web/tests/utils/integration.ts (1)
IntegrationHarness(14-118)apps/web/lib/types.ts (1)
Customer(426-426)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (3)
apps/web/tests/customers/pagination.test.ts (3)
1-31: LGTM! Well-structured test setup.The test setup correctly establishes a baseline dataset with explicit sort parameters and validates the ordering before running tests. This ensures deterministic test behavior.
81-256: Excellent error handling and sort order coverage!The tests comprehensively cover:
- Validation of mutually exclusive cursor parameters
- Page size limits with helpful error messages
- Invalid cursor ID handling (returning empty arrays)
- Both ascending and descending sort orders with explicit parameters
The explicit inclusion of
sortByandsortOrderin the ascending order tests (lines 164-256) ensures deterministic behavior.
259-278: LGTM! Helper functions are correctly implemented.The helper functions properly validate:
- Monotonically decreasing timestamps for descending order (using
>=)- Monotonically increasing timestamps for ascending order (using
<=)- No overlapping customer IDs between pages
Summary by CodeRabbit
New Features
Refactor
Performance
Tests
✏️ Tip: You can customize this high-level summary in your review settings.