fix(guest): paginate collect leaderboard + join list (drop 200/50 hard caps)#411
Merged
Conversation
…d caps)
The collect leaderboard hard-capped at `q.limit(200).all()` and the join
request list at `.limit(50)`, each returning `total=len(rows)`. Any event
with more songs was silently truncated AND the displayed song count froze
at the cap — why ELZ2G2 showed exactly 200.
Replace both with proper offset/limit pagination:
- Add `limit` (default 100, max 500) + `offset` query params to
GET /api/public/collect/{code}/leaderboard and
GET /api/public/events/{code}/requests
- Compute the true total via COUNT(*) before pagination; add `total` to
GuestRequestListResponse (the collect response already had it)
- New app/core/pagination.py documents the bounds as a payload guard, not
a display ceiling
- Regenerate OpenAPI schema + TS types
Frontend uses a growing-window model: `displayLimit` grows on "Load more"
and the fetch always reads from offset 0, so the existing poll-and-replace
loop (collect 5s, join 10s + SSE) keeps live vote updates correct without
client-side dedup or offset-drift bugs. Count badges on both pages now show
the real total instead of the capped page length.
Tests: 6 backend (offset/limit slicing, honest total beyond the page,
oversized-limit rejection) + 4 frontend (query-param URL building).
Collaborator
Author
|
@coderabbitai full review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds client-controlled pagination: server endpoints accept optional limit/offset and return full total counts; shared pagination bounds added; frontend API and collect/join pages request growing windows and show a capped "LOAD MORE"; tests cover URL construction, backend pagination, totals, validation, and ordering stability. ChangesClient-Controlled Pagination for Leaderboards and Request Queues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/app/api/collect.py (1)
142-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a deterministic tie-breaker to paginated ordering.
With
offsetpagination, ties in current sort keys can reshuffle rows between requests. At Line 142-149, append a stable secondary key (e.g.,SongRequest.id) in both tabs to prevent duplicate/missing rows across pages.Suggested patch
if tab == "trending": q = q.filter(SongRequest.vote_count >= 1).order_by( - SongRequest.vote_count.desc(), SongRequest.created_at.desc() + SongRequest.vote_count.desc(), + SongRequest.created_at.desc(), + SongRequest.id.desc(), ) else: # "All" is the discovery view — alphabetical makes it easy to scan # and upvote existing submissions rather than recency bias. - q = q.order_by(func.lower(SongRequest.song_title).asc()) + q = q.order_by( + func.lower(SongRequest.song_title).asc(), + SongRequest.id.asc(), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/api/collect.py` around lines 142 - 149, The paginated ordering can be non-deterministic because ties on the primary sort keys can reorder between offset pages; update both query.order_by calls where q is built to append a stable tie-breaker using SongRequest.id (e.g., after SongRequest.vote_count.desc() and SongRequest.created_at.desc() append SongRequest.id.asc(), and after func.lower(SongRequest.song_title).asc() append SongRequest.id.asc()) so rows with identical primary keys remain consistently ordered across paginated requests.dashboard/app/join/[code]/page.tsx (1)
888-888:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse true queue total in
SongDetailSheetdenominator.
totalCountstill uses loaded rows (guestRequests.length), which under-reports rank context after pagination.Suggested fix
- totalCount={guestRequests.length} + totalCount={total}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/app/join/`[code]/page.tsx at line 888, Replace the paginated row count used for the SongDetailSheet denominator — currently totalCount={guestRequests.length} — with the true queue total value returned by the server/pagination metadata (e.g., use guestRequestsTotal or the pagination response's totalCount/meta.totalCount) so the denominator reflects the full queue size; update the prop passed into SongDetailSheet (totalCount) to use that server-side total instead of guestRequests.length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dashboard/app/collect/`[code]/page.tsx:
- Around line 433-446: The Load More button currently increments displayLimit
past the backend cap and causes 422 errors; fix by clamping both the increment
and the visibility to the backend max (e.g., BACKEND_MAX = 500): change the
onClick updater passed to setDisplayLimit to setDisplayLimit(d => Math.min(d +
PAGE_SIZE, BACKEND_MAX)) and change the conditional that renders the button and
the "MORE" count to compare against Math.min(leaderboard?.total ?? 0,
BACKEND_MAX) (use a single BACKEND_MAX constant near PAGE_SIZE for clarity).
Ensure references: displayLimit, PAGE_SIZE, setDisplayLimit, leaderboard.total,
leaderboard.requests to locate the changes.
In `@dashboard/app/join/`[code]/page.tsx:
- Around line 787-800: The "LOAD MORE" button can grow displayLimit past the
backend max (500) causing 422s; update the onClick for setDisplayLimit to cap
increments with a backend max constant (e.g., BACKEND_MAX_PAGE_SIZE = 500) by
using setDisplayLimit(d => Math.min(d + PAGE_SIZE, BACKEND_MAX_PAGE_SIZE)), and
change the render condition so the button is hidden when displayLimit >=
BACKEND_MAX_PAGE_SIZE (in addition to guestRequests.length >= total) so it won't
try to request beyond the server max; reference activeTab, guestRequests, total,
setDisplayLimit, PAGE_SIZE, displayLimit, and tabRows when making this change.
In `@server/app/api/public.py`:
- Around line 286-289: The current query built in base_q uses ordering by
SongRequest.vote_count.desc() and SongRequest.created_at.desc(), which can lead
to unstable offset pagination when ties occur; update the ordering chain on
base_q to add a deterministic unique tiebreaker such as SongRequest.id.desc()
(i.e., ensure final sort keys are vote_count.desc(), created_at.desc(),
id.desc()) so offset/limit pagination yields stable, non-overlapping pages and
duplicates/skips are avoided.
---
Outside diff comments:
In `@dashboard/app/join/`[code]/page.tsx:
- Line 888: Replace the paginated row count used for the SongDetailSheet
denominator — currently totalCount={guestRequests.length} — with the true queue
total value returned by the server/pagination metadata (e.g., use
guestRequestsTotal or the pagination response's totalCount/meta.totalCount) so
the denominator reflects the full queue size; update the prop passed into
SongDetailSheet (totalCount) to use that server-side total instead of
guestRequests.length.
In `@server/app/api/collect.py`:
- Around line 142-149: The paginated ordering can be non-deterministic because
ties on the primary sort keys can reorder between offset pages; update both
query.order_by calls where q is built to append a stable tie-breaker using
SongRequest.id (e.g., after SongRequest.vote_count.desc() and
SongRequest.created_at.desc() append SongRequest.id.asc(), and after
func.lower(SongRequest.song_title).asc() append SongRequest.id.asc()) so rows
with identical primary keys remain consistently ordered across paginated
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2469ba05-a706-476f-b423-8982b4440a54
📒 Files selected for processing (11)
dashboard/app/collect/[code]/page.tsxdashboard/app/join/[code]/page.tsxdashboard/lib/__tests__/api.test.tsdashboard/lib/api-types.generated.tsdashboard/lib/api.tsserver/app/api/collect.pyserver/app/api/public.pyserver/app/core/pagination.pyserver/openapi.jsonserver/tests/test_collect_public.pyserver/tests/test_public.py
Addresses CodeRabbit review on #411: - Clamp the growing-window displayLimit to PUBLIC_PAGE_MAX (500) on both the collect and join pages. Both the +PAGE_SIZE increment and the "Load more" visibility now cap at the backend max, so the UI can't request limit>500 and trip the 422 -> global error path. The cap is exported once from lib/api.ts (mirrors server/app/core/pagination.py MAX_PAGE_SIZE) to keep both pages in sync rather than duplicating the literal. - Add SongRequest.id.desc() as a unique final ORDER BY on both the join request list and the collect leaderboard (trending + all tabs) so offset pagination yields stable pages with no dup/skip when rows tie on the primary sort key. Tests: 2 backend regression tests asserting deterministic id-desc tie ordering (test_tiebreaker_orders_by_id_desc, test_collect_leaderboard_tiebreaker_orders_by_id_desc); updated the two fully-mocked api modules in the page tests to export PUBLIC_PAGE_MAX.
This was referenced Jun 18, 2026
Closed
thewrz
added a commit
that referenced
this pull request
Jun 18, 2026
GET /api/events/{code}/pending-review returned total=len(rows) after an
undocumented 200-row cap — the #411 failure mode applied to the Pre-Event
Voting tab. Replace it with the paginated envelope (requests/total/limit/
offset/sort/direction): total is counted before pagination so the DJ sees the
real pending count, and limit/offset drive a #411 growing window.
Reuse PR2's sort machinery — extract apply_field_sort and make key_sorted
public in request_sort.py — so pending-review shares one ORDER BY / Camelot
implementation with the DJ list. The default (no sort param) preserves the
vote-ranked review order (votes desc, age asc, id desc); best_match falls back
to it since priority scoring is meaningless pre-event. The Pre-Event tab gains
a focused Sort selector (Review order + upvotes/date/title/artist), "showing
X of Y", and Load More. Bulk actions (Accept top N / Reject remaining) keep
operating server-side against the full filtered set, not the loaded page.
Part of #478 (PR3/4). Builds on PR2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
GET /api/events/{code}/pending-review returned total=len(rows) after an
undocumented 200-row cap — the #411 failure mode applied to the Pre-Event
Voting tab. Replace it with the paginated envelope (requests/total/limit/
offset/sort/direction): total is counted before pagination so the DJ sees the
real pending count, and limit/offset drive a #411 growing window.
Reuse PR2's sort machinery — extract apply_field_sort and make key_sorted
public in request_sort.py — so pending-review shares one ORDER BY / Camelot
implementation with the DJ list. The default (no sort param) preserves the
vote-ranked review order (votes desc, age asc, id desc); best_match falls back
to it since priority scoring is meaningless pre-event. The Pre-Event tab gains
a focused Sort selector (Review order + upvotes/date/title/artist), "showing
X of Y", and Load More. Bulk actions (Accept top N / Reject remaining) keep
operating server-side against the full filtered set, not the loaded page.
Part of #478 (PR3/4). Builds on PR2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
…ting
Replace the bare list at GET /api/events/{code}/requests with a paginated
envelope (requests/total/limit/offset/sort/direction) and add deterministic
DJ-facing sorting across eight fields: date_requested, date_accepted, upvotes,
bpm, key (harmonic Camelot order), title, artist, and best_match (priority).
The backend computes a true total before pagination so the dashboard never
infers the count from a page length (the #411 failure mode). SQL-expressible
sorts run as ORDER BY ... NULLS LAST, id DESC + LIMIT/OFFSET; key and best_match
sort in Python (Camelot ordinal / priority scoring) over the full filtered set.
Every sort ends with an id DESC tie-breaker so pages never duplicate or skip
rows between polls.
The frontend adopts the #411 growing-window model for the DJ list: a unified
Sort selector (Best Match plus the seven fields) with a direction toggle, a
displayLimit that grows on Load More up to PUBLIC_PAGE_MAX, a truthful
"showing X of Y" from the real total, and every poll/SSE refresh re-fetching
from offset=0 to avoid drift when rows reorder. Best Match replaces the old
priority checkbox as a sort option.
Part of #478 (PR2/4). Builds on PR1 (accepted_at) and #411.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
GET /api/events/{code}/pending-review returned total=len(rows) after an
undocumented 200-row cap — the #411 failure mode applied to the Pre-Event
Voting tab. Replace it with the paginated envelope (requests/total/limit/
offset/sort/direction): total is counted before pagination so the DJ sees the
real pending count, and limit/offset drive a #411 growing window.
Reuse PR2's sort machinery — extract apply_field_sort and make key_sorted
public in request_sort.py — so pending-review shares one ORDER BY / Camelot
implementation with the DJ list. The default (no sort param) preserves the
vote-ranked review order (votes desc, age asc, id desc); best_match falls back
to it since priority scoring is meaningless pre-event. The Pre-Event tab gains
a focused Sort selector (Review order + upvotes/date/title/artist), "showing
X of Y", and Load More. Bulk actions (Accept top N / Reject remaining) keep
operating server-side against the full filtered set, not the loaded page.
Part of #478 (PR3/4). Builds on PR2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
Introduce a nullable, indexed `accepted_at` column stamped the first time a request enters ACCEPTED and preserved through later status changes. It is the truthful backing field for the upcoming DJ "date accepted" sort (issue #478) — unlike `updated_at`, which moves on every metadata refresh or play and would reorder rows long after acceptance. Stamping is centralized in one idempotent `mark_accepted` helper reused at all five accept sites (the status-transition path, accept-all, and the three pre-event bulk-review branches). Exposed via RequestOut and regenerated OpenAPI/TS types. Migration 061 backfills existing accepted/playing/played rows best-effort from updated_at; rows never accepted stay NULL and sort null-last. Part of #478 (PR1/4: foundation). Builds on #411. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
…ting
Replace the bare list at GET /api/events/{code}/requests with a paginated
envelope (requests/total/limit/offset/sort/direction) and add deterministic
DJ-facing sorting across eight fields: date_requested, date_accepted, upvotes,
bpm, key (harmonic Camelot order), title, artist, and best_match (priority).
The backend computes a true total before pagination so the dashboard never
infers the count from a page length (the #411 failure mode). SQL-expressible
sorts run as ORDER BY ... NULLS LAST, id DESC + LIMIT/OFFSET; key and best_match
sort in Python (Camelot ordinal / priority scoring) over the full filtered set.
Every sort ends with an id DESC tie-breaker so pages never duplicate or skip
rows between polls.
The frontend adopts the #411 growing-window model for the DJ list: a unified
Sort selector (Best Match plus the seven fields) with a direction toggle, a
displayLimit that grows on Load More up to PUBLIC_PAGE_MAX, a truthful
"showing X of Y" from the real total, and every poll/SSE refresh re-fetching
from offset=0 to avoid drift when rows reorder. Best Match replaces the old
priority checkbox as a sort option.
Part of #478 (PR2/4). Builds on PR1 (accepted_at) and #411.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thewrz
added a commit
that referenced
this pull request
Jun 19, 2026
GET /api/events/{code}/pending-review returned total=len(rows) after an
undocumented 200-row cap — the #411 failure mode applied to the Pre-Event
Voting tab. Replace it with the paginated envelope (requests/total/limit/
offset/sort/direction): total is counted before pagination so the DJ sees the
real pending count, and limit/offset drive a #411 growing window.
Reuse PR2's sort machinery — extract apply_field_sort and make key_sorted
public in request_sort.py — so pending-review shares one ORDER BY / Camelot
implementation with the DJ list. The default (no sort param) preserves the
vote-ranked review order (votes desc, age asc, id desc); best_match falls back
to it since priority scoring is meaningless pre-event. The Pre-Event tab gains
a focused Sort selector (Review order + upvotes/date/title/artist), "showing
X of Y", and Load More. Bulk actions (Accept top N / Reject remaining) keep
operating server-side against the full filtered set, not the loaded page.
Part of #478 (PR3/4). Builds on PR2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7 tasks
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
1 participant
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.
Problem
Production event ELZ2G2 only showed 200 songs on the collect page, with more clearly submitted. Investigation found it was a backend hard cap, not a display bug — and there were two:
GET /api/public/collect/{code}/leaderboardq.limit(200).all()GET /api/public/events/{code}/requests.limit(50)Both returned
total=len(rows), so beyond the cap the list was silently truncated and the displayed song count froze at the cap (the count badge literally couldn't exceed 200). Neither endpoint accepted any paging params, so the client had no way to reach later rows.Fix — proper offset/limit pagination
Backend
limit(default 100, max 500) +offsetquery params on both endpointstotalviaCOUNT(*)computed before pagination (notlen(rows))totaladded toGuestRequestListResponse(the collect response already had it)server/app/core/pagination.pydocuments the bounds as a payload guard, not a display ceilingFrontend — growing-window model
displayLimitstate grows on "Load more"; the fetch always reads fromoffset=0offsetpaging for any future non-polling consumer.totalinstead of the capped page lengthTrade-off
MAX_PAGE_SIZE=500bounds a single request's payload. The product ceiling is gone (count is honest;offsetcan reach any row), but the frontend's growing window tops out at 500 displayed rows — far above any realistic single event. If an event is ever expected to exceed 500 distinct requests, the frontend would need multi-page fetching (backend already supports it).Tests
totalhonest beyond the returned page (regression refs the old.limit(200)/total=len(rows)), oversized-limit→ 422 — for both endpointsgetPublicRequests/getCollectLeaderboardappendlimit/offsetcorrectly and omit the query string when unsetVerification (local CI, all green)
2065 passed, coverage 88.07% (≥85% gate), ruff + format + bandit clean952 passed,tscclean, ESLint cleanManual test plan
/collect/{code}— count badge shows the true total; list shows 100 with a Load more · N more button that loads the rest/join/{code}— QUEUE pill shows true total; Load more works on LEADERBOARD/RECENTSummary by CodeRabbit
New Features
Tests