-
-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(webapp): Fix backwards nextCursor for runs edge case #3073
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,11 @@ export class PostgresRunsRepository implements IRunsRepository { | |
| previousCursor = reversedRuns.at(1)?.id ?? null; | ||
| nextCursor = reversedRuns.at(options.page.size)?.id ?? null; | ||
|
Comment on lines
75
to
76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Same pre-existing overflow issue exists in the Postgres repository The Postgres backward+hasMore branch at (Refers to lines 74-76) Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } else { | ||
| nextCursor = reversedRuns.at(options.page.size - 1)?.id ?? null; | ||
| // Use the last item (oldest run) as the forward cursor. | ||
| // We can't use a fixed index (pageSize - 1) because the result set | ||
| // may have fewer items than pageSize (e.g., when new runs were created | ||
| // while the user was browsing, shifting page boundaries). | ||
| nextCursor = reversedRuns.at(reversedRuns.length - 1)?.id ?? null; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
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.
🚩 Pre-existing: backward+hasMore branch has inconsistent overflow item handling
In the
hasMorebackward case (not changed by this PR), the overflow/sentinel item handling appears inconsistent with the forward case.For forward+hasMore: the query returns
[D, C, B, A](DESC), the code keeps[D, C, B]viaruns.slice(0, pageSize), and A is the overflow item (furthest from cursor, oldest). This is correct.For backward+hasMore: the query returns
[A, B, C, D](ASC), but the code keeps[B, C, D]viaruns.slice(1, pageSize+1)atclickhouseRunsRepository.server.ts:117, discarding A (closest to cursor). By analogy with forward, the overflow should be D (furthest from cursor, newest), and the page should display[A, B, C].This also affects cursor values in the
hasMorebackward branch atclickhouseRunsRepository.server.ts:101-102:previousCursor = reversedRunIds.at(1)— points to the second-newest item (C), but since "Previous" in the UI triggersdirection=backward(seeListPagination.tsx:35), using C as cursor would re-fetch D which is already on the displayed page.nextCursor = reversedRunIds.at(options.page.size)— points to the overflow item A rather than the oldest displayed item B.This may not cause visible issues in practice if page boundaries overlap by one item (the UI would just show a duplicated boundary item), but it's worth investigating whether users see repeated runs when navigating backward through multiple pages.
(Refers to lines 100-102)
Was this helpful? React with 👍 or 👎 to provide feedback.