Fix cursor pagination with optional tie-breaker#2080
Fix cursor pagination with optional tie-breaker#2080ravikiranvm wants to merge 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional composite cursor pagination (primary + secondary “tie-breaker” column) to prevent skipping/duplication when multiple rows share the same primary pagination value (OPS-3810), and wires it into Flow listing.
Changes:
- Extend
Paginatorto optionally encode/decode a secondary cursor field and apply composite cursor filtering. - Extend
buildPaginatoroptions to accept a secondary pagination column. - Add integration tests covering composite pagination behavior when many rows share the same primary timestamp.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/server/api/test/integration/helper/pagination/paginator.integration.test.ts | Adds integration coverage for composite pagination using a secondary ID tie-breaker. |
| packages/server/api/src/app/helper/pagination/paginator.ts | Implements secondary cursor key support and composite cursor filtering + ordering. |
| packages/server/api/src/app/helper/pagination/build-paginator.ts | Exposes secondary pagination configuration via builder options. |
| packages/server/api/src/app/flows/flow/flow.service.ts | Uses the new secondary tie-breaker pagination for flow listing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/server/api/src/app/helper/pagination/build-paginator.ts
Outdated
Show resolved
Hide resolved
|
bigfluffycookie
left a comment
bigfluffycookie
left a comment
There was a problem hiding this comment.
its good that you refactor things, but as a reviewer I now need to figure out which parts are changes for the fix, and which parts you just moved around. Please add comments explaining which parts are refactor and which parts are changes.
| cursors: CursorParam, | ||
| ): void { | ||
| const dbType = system.get(AppSystemProp.DB_TYPE); | ||
| const dbType = this.getSupportedDbType(); |
There was a problem hiding this comment.
why did you add this?
| return `${columnName} ${operator} :${paramName}`; | ||
| } | ||
|
|
||
| throw new Error('Unsupported database type'); |
There was a problem hiding this comment.
you already check with this.getSupportedDbType();
Fixes OPS-3810.
Additional Notes
We create benchmark workflows in bulk, so many rows share the same updated timestamp. With cursor pagination, page boundaries previously filtered using only updated < cursor, which caused rows with the same timestamp to be skipped across pages. This PR adds support for an optional secondary pagination key (used as a secondary sort key) to break ties when primary timestamps are identical.