fix(playlist): decouple guess autocomplete pool from track playability (closes #22) - #23
Conversation
Closes #22. On a large playlist most tracks could not be guessed at all. The dropdown is the only submission path, so the player was forced to skip and lose the round. Root cause is a desync between two differently-timed queries, and it is not a rare race — it is the default path for any playlist over ~100 tracks: 1. A full fetch stores every track in playlist_tracks, but only the ~100 previews the playlist embed returns are resolved at that moment. 2. fetch_complete is computed from the stored track list, so it is TRUE ("we have the whole playlist") while ~90% of previews are still pending. 3. HomePage bailed out of its progress poll on exactly that flag, conflating "we have the whole track list" with "every preview resolved". No progress bar, and freshestPlaylist() never re-fetched before starting. 4. The background fill kept resolving previews, so /sessions/start drew answers from a live pool several times larger than the client's snapshot. Measured on a real cached playlist (1251 tracks, 588 playable at session start, 96 in the client's snapshot): 3 of 25 server-drawn answers were typeable. After this change, 25 of 25. The pool is now structurally a superset of the answer set rather than one that happens to match when the clocks agree: - load_playlist returns every track linked to the playlist. Playability is reported as counts only (total_tracks, pending_preview_retry, pool_size), never per track, and answers are still drawn only from playable tracks. Verified across the 40 largest cached playlists: 0 answer candidates fall outside the new pool. - Both Spotify fetch paths emit that same shape via _public_track, and _finalize_and_save now always serves the DB read-back, so there is exactly one builder for the user-facing pool. - HomePage keys its poll off the exact pending count the API returns instead of inferring it from totals that don't line up past the fetch cap. Two omissions from the response are deliberate: preview_url (the CDN URL identifies a clip by comparing bytes) and any per-track playable flag (only playable tracks can be answers, so it would let a client filter the pool down to candidates). Neither was read by any client code. Supporting changes, needed because the pool grew ~2x and would otherwise regress latency and Spotify egress: - Rank dropdown matches (exact > prefix > word-start > substring, title over artist) instead of taking the first 7 found. Without this the exact title loses its slot to incidental substring hits on a large pool — the same symptom, different cause. Covered by unit tests. - /playlist returns stored album art inline, so the whole-pool art prefetch drops from 13 requests to 0 on that playlist. - /tracks/art is a pure DB read. It used to scrape one Spotify embed page per missing track, which at pool scale meant ~1000 scrapes per game start, competing with the preview fill for the same endpoint. The fill now harvests cover art from the embed page it already loads, so misses resolve on their own. - save_album_art_to_db does one batched UPDATE instead of one per track. - Use the existing Fisher-Yates shuffleArray for the pool; sort() with a random comparator is not a uniform shuffle. - Report the playable/total split in the pre-game UI, counted against the pool actually held rather than Spotify's stated total, so it never promises tracks we don't have. Net payload on that playlist: +36 KB gzipped for 2.1x the pool, against 13 fewer sequential round-trips. Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe playlist API now returns the complete guessable track pool while separating playable and pending-preview counts. Preview recovery also backfills album art. The frontend ranks autocomplete suggestions and displays localized playable-versus-total playlist information. ChangesPlaylist pool and guessing flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HomePage
participant PlaylistAPI
participant PreviewFill
participant Database
HomePage->>PlaylistAPI: request playlist pool and counts
PlaylistAPI->>Database: load all playlist tracks
Database-->>PlaylistAPI: tracks, playable count, pending retries
PlaylistAPI-->>HomePage: playlist response
HomePage->>PreviewFill: poll while retries remain
PreviewFill->>Database: save recovered previews and album art
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/db_playlists.py (1)
47-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the shared retry cap in the pending-count queries.
backend/db_playlists.pynow usesMAX_PREVIEW_RETRIES, but the direct “pending” queries inbackend/server.py(/playlist-statusand/playlist/.../wait-for-previews) still hard-codet.preview_retry_count < 5. If the retry limit is tuned inbackend/preview_store.py, those endpoints can return a differentpendingvalue thanpending_preview_retry, creating pool/count desync for clients.🤖 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 `@backend/db_playlists.py` around lines 47 - 115, Update the direct pending-count queries in the /playlist-status and /playlist/.../wait-for-previews handlers in backend/server.py to use the shared MAX_PREVIEW_RETRIES value instead of the hard-coded retry limit. Ensure both endpoints apply the same cap as the pending_count calculation in the playlist-loading flow, preserving synchronized pending and pool counts when the cap changes.
🧹 Nitpick comments (1)
backend/server.py (1)
236-267: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a short negative-cache TTL for art misses.
A cache miss that's also a DB miss triggers a
get_album_art_from_dbquery every call, with no negative caching. During an active preview fill on a large playlist, the frontend polls status/art repeatedly while many tracks are still missing art, so the same not-yet-found IDs can hit the DB on every poll cycle until they resolve.Caching a brief "not found yet" sentinel (short TTL) alongside the existing positive cache would cut this repeated DB traffic without delaying art appearance meaningfully.
🤖 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 `@backend/server.py` around lines 236 - 267, Add a short-lived negative-cache sentinel to fetch_album_art_for_tracks for track IDs absent from both art_cache and get_album_art_from_db, and skip those IDs on subsequent calls until the sentinel expires. Preserve existing positive-cache behavior and ensure discovered URLs replace any negative entries so art appears promptly.
🤖 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 `@backend/server.py`:
- Around line 641-679: Guard the result of parse_embed_next_data in
_scrape_one_preview before accessing nested .get() calls. Treat a falsy or
malformed parse result as a per-track failure and return the existing ("fail",
tid, "", "") tuple, ensuring the preview fill continues without raising;
preserve the current successful extraction behavior for valid embed data.
---
Outside diff comments:
In `@backend/db_playlists.py`:
- Around line 47-115: Update the direct pending-count queries in the
/playlist-status and /playlist/.../wait-for-previews handlers in
backend/server.py to use the shared MAX_PREVIEW_RETRIES value instead of the
hard-coded retry limit. Ensure both endpoints apply the same cap as the
pending_count calculation in the playlist-loading flow, preserving synchronized
pending and pool counts when the cap changes.
---
Nitpick comments:
In `@backend/server.py`:
- Around line 236-267: Add a short-lived negative-cache sentinel to
fetch_album_art_for_tracks for track IDs absent from both art_cache and
get_album_art_from_db, and skip those IDs on subsequent calls until the sentinel
expires. Preserve existing positive-cache behavior and ensure discovered URLs
replace any negative entries so art appears promptly.
🪄 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: 890f8d3b-4bd5-44a7-9637-e84f12b76a12
📒 Files selected for processing (12)
backend/db_playlists.pybackend/server.pydocs/api.mdfrontend/src/i18n/en.jsfrontend/src/i18n/es.jsfrontend/src/i18n/hi.jsfrontend/src/i18n/ja.jsfrontend/src/i18n/ko.jsfrontend/src/lib/search.jsfrontend/src/lib/search.test.jsfrontend/src/pages/GamePage.jsfrontend/src/pages/HomePage.js
…a fill Addresses CodeRabbit review on #23. The nested `.get()` chain over the embed payload could raise AttributeError, and because it sits inside an asyncio.gather fan-out without return_exceptions, that raise discarded the whole chunk's results: none of mark_recovered/mark_unavailable/mark_failed ran, so the offending track's retry counter never advanced and it could knock out the same playlist's fill on every subsequent attempt. Credit to the review for tracing that consequence. The suggested patch (guard `if not next_data`) closes only part of it. Falsy payloads were already safe — `{}` chains fine. The actual vectors are: - truthy non-dict from json.loads: `"str"`, `42`, `true` - an explicit null at an intermediate level, e.g. `{"props": null}`, because `.get(k, {})` returns the default only for a MISSING key, not a null value - a non-dict `entity`, which survives the chain and then raises downstream in `.get("audioPreview")` / _extract_cover_art Of ten payload shapes, the current chain raises on six; the suggested guard fixes two of those six. So the fix is type-checked at every hop instead: - _embed_entity() walks to the entity node and always returns a dict. - _extract_cover_art() uses `or {}` / `or []` and skips non-dict sources, since a null coverArt or sources would otherwise raise on the next access. - The parse/extract in _scrape_one_preview is wrapped so the function is total rather than total-by-inspection. - gather() takes return_exceptions=True with the exceptions filtered and logged, per the reviewer's offer. Redundant with a total scraper, but this is the exact point where a raise turns into silent loss of persisted work, so the structural guard is worth its three lines. Verified: a raising task now costs one track instead of all 48 in its chunk. Also from the review: /playlist-status and /playlist/{id}/wait-for-previews hard-coded `preview_retry_count < 5` while load_playlist uses MAX_PREVIEW_RETRIES. #23 made that consequential — the client now starts polling off load_playlist's pending count and reads progress off /playlist-status's, so a change to the constant would leave the progress bar unable to finish. Both now take the shared constant. (sessions.py has the same literal; left alone to avoid churn against the open #24.) Adds a `python server.py` self-check covering all ten payload shapes and the cover-art edge cases, in the same style as matching.py and sessions.py. uvicorn imports the module and never runs __main__. Confirmed the check fails when the falsiness-only guard is substituted. Declined the negative-cache nitpick for /tracks/art: #23 makes that path cold (album art now ships inline with /playlist, so the endpoint is only hit for tracks art hasn't reached yet), a DB miss is one indexed lookup, and a negative sentinel would actively delay art that the preview fill is concurrently backfilling into art_cache. Co-Authored-By: Claude <noreply@anthropic.com>
|
Worked through all three. Two were valid and are fixed in 23434c5; the nitpick I'm declining with reasoning. 1. Unguarded
|
| payload | current chain | if not next_data guard |
now |
|---|---|---|---|
| valid | ok | ok | ok |
{} |
ok | ok | ok |
null |
AttributeError | ok | ok |
[] |
AttributeError | ok | ok |
"unexpected" |
AttributeError | AttributeError | ok |
42 |
AttributeError | AttributeError | ok |
{"props":null} |
AttributeError | AttributeError | ok |
{"props":{"pageProps":{"state":null}}} |
AttributeError | AttributeError | ok |
entity: null |
ok → non-dict | ok → non-dict | ok |
entity: [] |
ok → non-dict | ok → non-dict | ok |
Six of ten raise today; the suggested guard fixes two of those six. The misses are (a) truthy non-dicts from json.loads, and (b) an explicit null at an intermediate level — .get(k, {}) applies its default only when the key is missing, not when its value is null. The last two rows don't raise in the chain but return a non-dict that raises downstream in .get("audioPreview") / _extract_cover_art.
So it's type-checked at every hop instead:
def _embed_entity(next_data) -> dict:
node = next_data
for key in _EMBED_ENTITY_PATH:
if not isinstance(node, dict):
return {}
node = node.get(key)
return node if isinstance(node, dict) else {}Plus _extract_cover_art hardened for null coverArt/sources and non-dict entries, and the parse/extract wrapped so _scrape_one_preview is total by construction rather than total-by-inspection.
And yes to your return_exceptions=True offer. Strictly redundant with a total scraper, but this fan-out is the precise point where a raise converts into silent loss of already-completed work, so the structural guard earns its three lines. Verified the difference directly — with a deliberately raising task in a 5-task chunk:
OLD: whole chunk lost -> RuntimeError
-> mark_recovered/mark_failed never run; retry counters never advance
NEW: 4/5 results survived; recovered=['t1','t2','t3','t4']
2. Hard-coded retry cap in /playlist-status and /wait-for-previews — valid, fixed
Both now take MAX_PREVIEW_RETRIES (already imported). This mattered more than it looks, and specifically because of this PR: the client now triggers its poll off load_playlist's pending_preview_retry (which uses the constant) and reads progress off /playlist-status's pending (which didn't), so bumping the constant would have left the progress bar unable to finish.
sessions.py:185 carries the same literal — left alone deliberately, since #24 is open against that file and this isn't worth the cross-PR churn. Tracked for that batch.
3. Negative-cache sentinel for fetch_album_art_for_tracks — declining
Three reasons:
- The path is now cold. This PR ships album art inline with
/playlist, soGamePageonly calls/tracks/artfor tracks art hasn't reached yet. Measured on a real 1251-track playlist: 1251/1251 had art inline, so the call count went 13 → 0. The optimisation targets requests that mostly no longer happen. - A miss costs one indexed lookup —
WHERE track_id = ANY($1), ≤100 ids, once per chunk per game. - It would fight the fix. The preview fill now backfills cover art into both the DB and
art_cacheas it scrapes. A negative sentinel would suppress lookups for exactly the tracks that are actively resolving, delaying art rather than speeding anything up. The "a miss just means not fetched yet and resolves on its own" property is deliberate.
Happy to revisit if the endpoint ever shows up hot in practice.
Validation
- New
python server.pyself-check covers all ten payload shapes above plus the cover-art edge cases (nullcoverArt, nullsources, non-dict entries, the 200–400px pick, and the fallback when nothing is in range). Same__main__convention asmatching.py; uvicorn imports the module and never runs it. - Confirmed the check has teeth: substituting the falsiness-only guard makes it fail with
AttributeError: 'str' object has no attribute 'get'; it passes with the type-checked version. matching.pyself-check passes, 32 frontend tests pass,craco buildcompiles clean.
Also picked up the RUF002 en-dash in that docstring. Leaving the two except Exception blocks (BLE001) as-is: catching broadly is the intended contract here — "any problem with this one track means mark it failed and move on" — and third-party HTML failure modes aren't enumerable. There's no ruff/lint config in the repo, so this isn't an enforced rule.
…or docs Addresses CodeRabbit review on #24. reveal construction (line 313) ------------------------------ _is_guess_correct reads `entry.get("artist") or ""`, then seven lines later the same dict was subscripted directly as entry["name"] / entry["artist"]. Two functions reading one dict disagreeing about whether its keys are guaranteed is the inconsistency worth removing — my own diff introduced it. To be accurate about severity: the KeyError is not reachable today. Every entry start_session writes carries name and artist, sourced from tracks columns that are NOT NULL DEFAULT '', and sessions expire after 2h so no older JSONB shape survives. This is consistency, not a live fix. It is still worth doing: `entry` is deserialized JSONB whose shape has changed before (see _META_KEYS), and the failure mode would be a 500 on a guess the player already committed to. Extracted _reveal_for(tid, entry) rather than inlining .get() calls, so the logic is testable without a database — same reasoning that produced _is_guess_correct in this PR. Response contract is unchanged: the same four keys, in the same shape the frontend reads. The review asked for an endpoint-level test. Declining that specific form: there is no pytest, no test database and no CI in this repo, so it would mean standing up the first backend HTTP test harness inside a review fixup, which is a scope decision for the owner rather than something to smuggle in here. The extracted helper is covered by the existing python sessions.py self-check instead, at the layer the bug would live in — full entry, empty entry, all-null-values entry, and partial entry. Confirmed the check fails with KeyError: 'name' when the direct subscripts are restored. separator docstring (line 80) ----------------------------- The list rendered `×` and `x` adjacent to each other. They are two different rules in _ARTIST_SEP — the multiplication sign splits anywhere, the letter x only at word boundaries — so a reader who cannot tell them apart loses the point of the sentence. Spelled the symbols out in words and pointed at matching._ARTIST_SEP for the exact pattern. The regex itself keeps the literal `×`; it is functional. also ---- Followed through on the MAX_PREVIEW_RETRIES consistency I said I would pick up in this batch when it came up on #23: start_session's pending_count was the third site hard-coding `preview_retry_count < 5`. It feeds the client's "waiting for previews" retry, so it has to mean the same thing as the preview store and /playlist-status. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Closes #22.
The bug
On a large playlist most tracks can't be guessed at all. The dropdown is the only submission path, so the player is forced to skip and lose the round.
The reporter flagged the pool/answer desync as a "secondary concern" they hadn't reproduced. It is actually the primary cause, and it isn't a rare race — it's the default path for any playlist over ~100 tracks:
playlist_tracks, but only the ~100 previews Spotify's playlist embed returns are resolved at that moment.fetch_completeis computed from the stored track list, so it'sTRUE("we have the whole playlist") while ~90% of previews are still pending —server.py:943.HomePagebailed out of its progress poll on exactly that flag (HomePage.js:182), conflating "we have the whole track list" with "every preview resolved". So: no progress bar, andfreshestPlaylist()never re-fetched before starting./sessions/startdrew answers from a live pool several times larger than the client's snapshot.The server already knew — it returns
pending_preview_retry: 850and awarningstring.HomePagenever rendered either.The fix
Make the pool structurally a superset of the answer set, rather than one that happens to match when the clocks agree.
load_playlistreturns every track linked to the playlist. Playability is reported as counts only (total_tracks,pending_preview_retry,pool_size) — never per track. Answers are still drawn only from playable tracks; that filter stays where it belongs._public_track, and_finalize_and_savealways serves the DB read-back — so there is exactly one builder for the user-facing pool and it can't differ by which path served the request.HomePagekeys its poll off the exact pending count the API returns instead of inferring it from totals that don't line up past the fetch cap.Two omissions from the response are deliberate:
preview_url(the CDN URL identifies a clip by comparing bytes) and any per-trackplayableflag (only playable tracks can be answers, so it would let a client filter the pool down to candidates). Neither was read by any client code.Verification
Read-only queries against the production DB, plus a replay of the real
GamePagedropdown logic over a real pool and real server-drawn answers.Invariant — across the 40 largest cached playlists, answer candidates falling outside the new pool: 0. Across those same playlists, 5,400 tracks can currently become an answer while absent from an old-style pool (worst case 638 of 1500 — 43%).
End-to-end, playlist
61gF4ehWZJT8A4tnTV3Kxa(1251 tracks, 588 playable at session start, 96 in the client's snapshot):That 88% failure rate matches the reporter's experience. Typing half a title surfaces the answer 25/25.
Supporting changes
Needed because the pool roughly doubled and would otherwise regress latency and Spotify egress:
/playlistreturns stored album art inline — the whole-pool art prefetch drops from 13 requests to 0 on that playlist./tracks/artis now a pure DB read. It used to scrape one Spotify embed page per missing track, which at pool scale is ~1000 scrapes per game start, competing with the preview fill for the same endpoint and inviting the 429 that stalls previews. The fill now harvests cover art from the embed page it already loads, so misses resolve on their own.save_album_art_to_dbdoes one batched UPDATE instead of one per track.shuffleArray;sort()with a random comparator is not a uniform shuffle (the correct helper was already defined and unused).Net payload: +36 KB gzipped for 2.1× the pool, against 13 fewer sequential round-trips.
Notes for review
craco buildclean; 32 frontend tests pass (5 new);matching.pyself-check passes.sessions.pyhand-rolls artist matching instead of usingmatching.py(breaks everyfeat./&collab in artist mode), the round-audio prefetch starts the next round's scoring timer on the reveal screen, andALLOW_LEGACY_SCORE_SUBMITstill defaults on.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation