fix(webapp): correct backward pagination slice in listRunIds#3867
Conversation
Backward pagination dropped the wrong end of the result set. listRunRows fetches page.size + 1 rows to detect hasMore; that extra row is the one farthest from the cursor in both directions (forward orders DESC, backward orders ASC), so it is always the trailing element. Forward correctly sliced rows[0..size); backward+hasMore sliced rows[1..size+1], dropping the row closest to the cursor and keeping the has-more sentinel. The result was a page that straddled two logical pages (one run from the correct previous page plus one from the page before it) — so paging "newer" across a boundary repeated and skipped runs. Both directions now slice rows[0..size). Forward pagination, the hasMore=false backward path, and all cursor values were already correct and are unchanged. Adds a regression test that walks forward across multiple pages, then walks backward from the last page and asserts each backward page exactly reproduces the corresponding forward page (and the full traversal covers every run once). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (13)
🧰 Additional context used📓 Path-based instructions (10)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,tsx,jsx,css,json,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.test.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (9)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-07T12:25:18.271ZApplied to files:
📚 Learning: 2026-05-28T20:02:10.647ZApplied to files:
📚 Learning: 2026-05-12T21:04:05.815ZApplied to files:
📚 Learning: 2026-05-18T14:40:02.173ZApplied to files:
📚 Learning: 2026-06-04T18:16:35.386ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis PR fixes an off-by-one bug in ClickHouseRunsRepository.listRunIds by unifying slice logic to always take rows.slice(0, page.size), adjusts cursor handling for partial backward pages, adds regression tests for multi-page and partial backward traversal, and updates server-changes documentation describing the change. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
In listRunIds' backward !hasMore branch, nextCursor was indexed at reversedRows[page.size - 1]. On a partial page (fewer than page.size rows — reachable via runs.list by passing a forward page's cursor as a backward cursor) that index overshoots and yields undefined, so nextCursor became null and forward navigation was stranded. Take the oldest row on the page (rows[0]) instead — equivalent for full pages, correct for partial ones. Adds a regression test: backward onto a partial first page must still expose a working forward cursor.
2 participants
Problem
Two backward-pagination bugs in
ClickHouseRunsRepository.listRunIds, both pre-existing (they predate the composite-cursor work in #3852 and were spotted during/after it):1. Wrong slice (straddled pages).
listRunRowsfetchespage.size + 1rows to detecthasMore. That extra row is the one farthest from the cursor in both directions (forward orders DESC; backward orders ASC), so it's always the trailing element. Forward correctly usedrows.slice(0, size), but backward+hasMoreusedrows.slice(1, size + 1)— dropping the row closest to the cursor and keeping the has-more sentinel. The page straddled two logical pages (one run from the correct previous page + one from the page before it), so paging "newer" across a boundary repeated and skipped runs.2. Stranded forward cursor on a partial backward page. In the backward
!hasMorebranch,nextCursorwasreversedRows.at(page.size - 1). On a partial page (fewer thanpage.sizerows — reachable viaruns.listby passing a forward page's cursor aspage[before]), that index overshoots →undefined→nextCursorbecomesnull, leaving no way to page forward again.Fix
rows.slice(0, size)(the sentinel is the trailing element either way).!hasMorebranch takes the oldest row on the page,rows.at(0), fornextCursor— equivalent to the old expression for full pages, correct for partial ones.Forward pagination, the cursor values for full pages, and the
hasMore === truepaths were already correct and are unchanged.Tests
runsRepositoryCursor.test.tsgains two cases (both fail onmain, pass here):mainreturns{b,c}instead of{c,d}), and the full traversal covers every run once.mainreturns anullnextCursor.The three existing cursor tests (forward completeness, backward round-trip, legacy cursor) still pass.