Repository navigation
fix(cli): Stop dashboard revisions pagination from skipping rows - #1608
Merged
Merged
Conversation
`sentry dashboard revisions --limit 150` fetched two full pages of 100, trimmed the last 50 rows, and still stored the server cursor, so `-c next` silently skipped those 50 revisions. Later pages now request only the remaining budget, like the shared paginate() helper, and the cursor is dropped if an endpoint still returns more than requested. Ported from getsentry/cli#1118. Co-Authored-By: BYK <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
BYK
approved these changes
Oct 23, 2026
BYK
added a commit
that referenced
this pull request
Oct 23, 2026
Follow-up to #1608. That PR fixed one hand-rolled loop. This one makes the shared helper the only way to do limit-bounded pagination, and has lint and a property test enforce it. **The "remaining budget" approach skips rows on page-number paginators.** `OffsetPaginator` (used for org projects, teams, members, repositories, and dashboard revisions) encodes the page size in the cursor as `<per_page>:<page>:0`. If you send that cursor with a smaller `per_page`, the server reads the *end* of the old-sized window (`results[-(limit + 1):]` in `src/sentry/api/paginator.py`). So `--limit 150` came back as rows 0–99 plus 150–199, with 100–149 silently missing. Since #1615, `paginate()` and the loop from #1608 both did this. It wasn't caught because every test modeled cursors as absolute offsets. `paginate()` now: - keeps one `per_page` for the whole walk, and picks one that divides the limit (150 → 2×75, 250 → 5×50) so the last cursor still works for `-c next` - stops one page early instead of trimming when no reasonable divisor exists (e.g. 101). It returns fewer rows, but with a valid cursor - drops the cursor only when the server ignored `per_page` and rows had to be trimmed To keep this class of bug from coming back: - **Migrated** the six remaining hand-rolled limit loops to `paginate()`: alert issue and metric lists, dashboard revisions, `listIssuesAllPages`, `listIssueEvents`, and `queryEvents`. - **New lint rule** `sentry-cli/no-manual-pagination`: rejects any loop in `src/` (outside `infrastructure.ts`) that follows cursors and also counts toward a `limit`. It flags all six original sites, including the #1608 version of revisions. `dashboard/list.ts` keeps a disable with a reason, because it filters client-side and resumes mid-page with its own bookmarks. `prefer-paginate-helper` now covers all of `src/`. - **New property test** `test/lib/paginate.property.test.ts`: ports Sentry's `OffsetPaginator` and `GenericOffsetPaginator` cursor math and walks `-c next` until done, asserting every row comes back exactly once, in order. It fails against the old `paginate()` (counterexample: 102 rows, `--limit 101`). - **Docs:** `packages/cli/AGENTS.md` used to tell people to write exactly this loop by hand. It now points to `paginate()` and explains why. One behavior change: `listIssueEvents` used to keep the server cursor after an overshoot, on the theory that offset cursors would re-include the trimmed rows. `GenericOffsetPaginator` computes the next offset from the server's own page size, so keeping it would skip rows. It now follows the shared contract. Known gap, not addressed here and not new: resuming a page-number cursor with a *different* `--limit` still misaligns, because the cursor's page size no longer matches. Fixing that needs CLI-side cursors that carry the page size. <!-- junior-request-attribution:start --> via **BYK**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:local%3Aweb%3A4269baf0761dc42b4e7fc238 --> -- [View Junior Session in Sentry](https://sentry.sentry.io/explore/conversations/local%3Aweb%3A4269baf0761dc42b4e7fc238/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: BYK <[email protected]>
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sentry dashboard revisions --limit 150fetched two full pages of 100, trimmed the last 50, and still stored the server cursor.-c nextthen silently skipped those 50 revisions.Later pages now request only the remaining budget, the same way the shared
paginate()helper does since getsentry/cli#1615. If an endpoint still returns more than requested, the cursor is dropped, matchingautoPaginate.Ported from the dashboard part of getsentry/cli#1118, which is being closed with the repo archive.
via BYK.
--
View Junior Session in Sentry