feat(collect): inline song preview embed in detail sheet#287
Conversation
Add CollectPreviewResponse type and getCollectPreview() API method. CollectDetailSheet fetches preview data and renders Spotify/Tidal iframes or a Beatport external link below the suggested-by section.
📝 WalkthroughWalkthroughAdds a Collect preview feature: new backend schema and endpoint to return a request's preview (source + source_url), frontend API client method, CollectDetailSheet prop/state to fetch and render embeds or a Beatport link, updated page wiring, tests, and design/plan docs. ChangesCollect Song Preview Feature
Sequence DiagramsequenceDiagram
participant UI as Browser UI
participant Comp as CollectDetailSheet
participant Client as ApiClient
participant Server as Collect API
participant DB as Database
UI->>Comp: open detail sheet (row.id, code)
Comp->>Client: getCollectPreview(code, row.id)
Client->>Server: GET /api/public/collect/{code}/requests/{id}/preview
Server->>DB: query SongRequest for request_id within event
DB-->>Server: SongRequest (source, source_url)
Server-->>Client: 200 { source, source_url }
Client-->>Comp: preview data
Comp->>UI: render previewSection (iframe or Beatport link) / no preview
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 (1)
dashboard/app/collect/[code]/components/CollectDetailSheet.test.tsx (1)
31-374: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding tests for the preview section rendering.
The design spec (lines 115-120) lists frontend tests for iframe rendering (Spotify/Tidal) and external link rendering (Beatport). These aren't covered by the current test suite. Consider adding tests that mock
getCollectPreviewto return embeddable sources and verify the iframe/link renders correctly.Do you want me to generate test cases for the preview section rendering?
🤖 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/collect/`[code]/components/CollectDetailSheet.test.tsx around lines 31 - 374, Add unit tests for the preview section in CollectDetailSheet by mocking the getCollectPreview call to return embeddable and external preview results and asserting the correct DOM render; specifically, add tests that (1) mock getCollectPreview to return an embed (e.g., { source: 'spotify', url: 'https://open.spotify...' }) and render <CollectDetailSheet ... /> then assert an iframe exists with the expected src, and (2) mock getCollectPreview to return an external link (e.g., { source: 'beatport', url: 'https://beatport...' }) then render and assert an anchor with the correct href and target exists; use the same test setup utilities (render, act, vi.fn()) and the existing mockRow, spy or vi.mock the module that exports getCollectPreview, and ensure mocks are restored between tests so other tests (e.g., those referencing bpm/key/artist) are unaffected.
🤖 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]/components/CollectDetailSheet.tsx:
- Around line 167-214: The iframe in the previewSection of
CollectDetailSheet.tsx is missing a title attribute; update the iframe rendered
inside the embedUrl branch (in the previewSection IIFE) to include a meaningful
title, e.g. use preview.title if available or fallback to a string like
`${source} preview` (so: title={preview.title || `${source} preview`}), to
improve screen reader accessibility.
In `@docs/superpowers/plans/2026-05-05-collect-song-preview.md`:
- Around line 147-153: The rate-limit decorator on request_preview currently
uses the default keying; update the limiter.limit call on request_preview to
explicitly key on guest_id (not IP/global) by passing the limiter's key_func/key
argument that extracts the guest_id from the request or from the
require_verified_human_soft dependency (e.g., a get_guest_id(request) helper),
and add a runtime check in request_preview (or the dependency) to verify
guest_id is present (raise/log if missing) so throttling cannot silently fall
back to a global/IP key.
- Around line 77-130: Add two negative-path tests in
server/tests/test_collect_public.py: one named like
test_collect_preview_rejects_without_human_cookie that uses the existing
_enable_collection, picks a request from collection_requests and calls the same
preview endpoint without setting the wrzdj_human cookie (and also with an
explicitly invalid value) asserting the response is a rejection (expect
401/unauthorized); and another named like
test_collect_preview_rate_limit_exceeded that enables collection, uses the same
guest identity (same cookies/guest token) and issues >10 GETs to
f"/api/public/collect/{test_event.code}/requests/{req.id}/preview" in a loop,
asserting that after the 10th request subsequent responses return rate-limited
(expect 429). Ensure both tests reuse collection_requests/ test_event fixtures
and the exact preview URL used elsewhere.
---
Outside diff comments:
In `@dashboard/app/collect/`[code]/components/CollectDetailSheet.test.tsx:
- Around line 31-374: Add unit tests for the preview section in
CollectDetailSheet by mocking the getCollectPreview call to return embeddable
and external preview results and asserting the correct DOM render; specifically,
add tests that (1) mock getCollectPreview to return an embed (e.g., { source:
'spotify', url: 'https://open.spotify...' }) and render <CollectDetailSheet ...
/> then assert an iframe exists with the expected src, and (2) mock
getCollectPreview to return an external link (e.g., { source: 'beatport', url:
'https://beatport...' }) then render and assert an anchor with the correct href
and target exists; use the same test setup utilities (render, act, vi.fn()) and
the existing mockRow, spy or vi.mock the module that exports getCollectPreview,
and ensure mocks are restored between tests so other tests (e.g., those
referencing bpm/key/artist) are unaffected.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f20fc353-c88d-4177-b57e-59fd48ee2bcc
📒 Files selected for processing (9)
dashboard/app/collect/[code]/components/CollectDetailSheet.test.tsxdashboard/app/collect/[code]/components/CollectDetailSheet.tsxdashboard/app/collect/[code]/page.tsxdashboard/lib/api.tsdocs/superpowers/plans/2026-05-05-collect-song-preview.mddocs/superpowers/specs/2026-05-05-collect-song-preview-design.mdserver/app/api/collect.pyserver/app/schemas/collect.pyserver/tests/test_collect_public.py
| - [ ] **Step 1: Write the failing test** | ||
|
|
||
| Add to `server/tests/test_collect_public.py`: | ||
|
|
||
| ```python | ||
| def test_collect_preview_returns_source_url(client, db, test_event, collection_requests): | ||
| """Preview endpoint returns source + source_url for a valid request.""" | ||
| _enable_collection(db, test_event) | ||
| req = collection_requests[0] | ||
| req.source_url = "https://open.spotify.com/track/abc123" | ||
| db.commit() | ||
|
|
||
| r = client.get( | ||
| f"/api/public/collect/{test_event.code}/requests/{req.id}/preview" | ||
| ) | ||
| assert r.status_code == 200 | ||
| body = r.json() | ||
| assert body["source"] == "spotify" | ||
| assert body["source_url"] == "https://open.spotify.com/track/abc123" | ||
|
|
||
|
|
||
| def test_collect_preview_null_source_url_for_manual(client, db, test_event, collection_requests): | ||
| """Preview endpoint returns source_url=null for manual entries.""" | ||
| _enable_collection(db, test_event) | ||
| req = collection_requests[0] | ||
| req.source = "manual" | ||
| req.source_url = None | ||
| db.commit() | ||
|
|
||
| r = client.get( | ||
| f"/api/public/collect/{test_event.code}/requests/{req.id}/preview" | ||
| ) | ||
| assert r.status_code == 200 | ||
| body = r.json() | ||
| assert body["source"] == "manual" | ||
| assert body["source_url"] is None | ||
|
|
||
|
|
||
| def test_collect_preview_404_wrong_event(client, db, test_event, collection_requests): | ||
| """Preview endpoint returns 404 when request belongs to a different event.""" | ||
| _enable_collection(db, test_event) | ||
| req = collection_requests[0] | ||
|
|
||
| r = client.get(f"/api/public/collect/ZZZZZZ/requests/{req.id}/preview") | ||
| assert r.status_code == 404 | ||
|
|
||
|
|
||
| def test_collect_preview_404_nonexistent_request(client, db, test_event): | ||
| """Preview endpoint returns 404 for nonexistent request ID.""" | ||
| _enable_collection(db, test_event) | ||
|
|
||
| r = client.get(f"/api/public/collect/{test_event.code}/requests/99999/preview") | ||
| assert r.status_code == 404 | ||
| ``` |
There was a problem hiding this comment.
Add negative-path tests for human verification and rate limiting.
The plan’s test set validates only happy-path/404 behavior, but it doesn’t verify the abuse controls called out in the PR goals. Please add explicit cases for: (1) missing/invalid wrzdj_human cookie (expected rejection) and (2) exceeding the 10/min limit for the same guest identity.
🤖 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 `@docs/superpowers/plans/2026-05-05-collect-song-preview.md` around lines 77 -
130, Add two negative-path tests in server/tests/test_collect_public.py: one
named like test_collect_preview_rejects_without_human_cookie that uses the
existing _enable_collection, picks a request from collection_requests and calls
the same preview endpoint without setting the wrzdj_human cookie (and also with
an explicitly invalid value) asserting the response is a rejection (expect
401/unauthorized); and another named like
test_collect_preview_rate_limit_exceeded that enables collection, uses the same
guest identity (same cookies/guest token) and issues >10 GETs to
f"/api/public/collect/{test_event.code}/requests/{req.id}/preview" in a loop,
asserting that after the 10th request subsequent responses return rate-limited
(expect 429). Ensure both tests reuse collection_requests/ test_event fixtures
and the exact preview URL used elsewhere.
There was a problem hiding this comment.
Acknowledged — these are comments on the plan doc, not the implementation code. The plan is a reference artifact; the actual endpoint has require_verified_human_soft and rate limiting in place. Adding negative-path tests for human verification would require mocking the Turnstile cookie infrastructure in the test client, which is tested separately in the human verification test suite. Rate limit tests are similarly covered by the slowapi integration tests.
There was a problem hiding this comment.
@thewrz, understood — that's a fair distinction. Since the plan doc is a reference artifact and the actual endpoint already enforces require_verified_human_soft and rate limiting, with coverage delegated to the dedicated Turnstile cookie and slowapi integration test suites respectively, there's no need to duplicate those negative-path cases here. Thanks for the context!
✏️ Learnings added
Learnt from: thewrz
Repo: thewrz/WrzDJ PR: 287
File: docs/superpowers/plans/2026-05-05-collect-song-preview.md:77-130
Timestamp: 2026-05-06T03:50:52.353Z
Learning: In the WrzDJ codebase, plan docs under `docs/superpowers/plans/` are reference artifacts only and are not expected to include exhaustive test coverage. Negative-path tests for human verification (Turnstile `wrzdj_human` cookie) are maintained in a separate human verification test suite, and rate-limiting tests are covered by slowapi integration tests — not duplicated in feature-level test files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @limiter.limit("10/minute") | ||
| def request_preview( | ||
| code: str, | ||
| request_id: int, | ||
| request: Request, | ||
| _human: int | None = Depends(require_verified_human_soft), | ||
| db: Session = Depends(get_db), |
There was a problem hiding this comment.
Specify and enforce rate-limit keying by guest_id (not implicit defaults).
@limiter.limit("10/minute") alone doesn’t document or guarantee that throttling is per guest_id. Since the objective is per-guest limiting, the plan should call out the key function/source explicitly and include a verification step so this doesn’t silently fall back to IP/global behavior.
🤖 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 `@docs/superpowers/plans/2026-05-05-collect-song-preview.md` around lines 147 -
153, The rate-limit decorator on request_preview currently uses the default
keying; update the limiter.limit call on request_preview to explicitly key on
guest_id (not IP/global) by passing the limiter's key_func/key argument that
extracts the guest_id from the request or from the require_verified_human_soft
dependency (e.g., a get_guest_id(request) helper), and add a runtime check in
request_preview (or the dependency) to verify guest_id is present (raise/log if
missing) so throttling cannot silently fall back to a global/IP key.
There was a problem hiding this comment.
Rate limiting uses the global key_func=get_client_ip configured in core/rate_limit.py:88 — consistent with every other collect endpoint. The plan doc's "per guest" language loosely maps to per-IP, which is the standard pattern across all public endpoints in this codebase. No change needed in the implementation.
There was a problem hiding this comment.
@thewrz, understood — thanks for the clarification. Since the global key_func=get_client_ip in core/rate_limit.py is applied consistently across all public collect endpoints, the PR doc's "per guest" wording is simply informal shorthand for per-IP, and no explicit keying change is needed here.
✏️ Learnings added
Learnt from: thewrz
Repo: thewrz/WrzDJ PR: 287
File: docs/superpowers/plans/2026-05-05-collect-song-preview.md:147-153
Timestamp: 2026-05-06T03:50:56.859Z
Learning: In the WrzDJ codebase, rate limiting for all public collect endpoints uses a globally configured `key_func=get_client_ip` defined in `server/core/rate_limit.py` (around line 88). "Per guest" language in documentation/plans is informal shorthand for per-IP limiting, which is the standard pattern across all public endpoints. No per-request key_func override is needed on individual endpoint decorators.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Add title attribute to embed iframe for accessibility - Add tests for Spotify, Tidal, Beatport, and manual preview branches - Fixes branch coverage threshold (69.92% → 70.11%)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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]/components/CollectDetailSheet.tsx:
- Around line 49-57: Reset the stale preview state before starting and on failed
fetch: inside the useEffect that depends on code and row.id, call
setPreview(null) immediately before invoking apiClient.getCollectPreview(code,
row.id), and in the catch handler call setPreview(null) as well (while
preserving the cancelled flag logic). This ensures the preview state managed by
preview/setPreview is cleared when row.id or code changes and when
getCollectPreview throws.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0d0cdee2-a419-49c1-9609-bdb877b67fee
📒 Files selected for processing (2)
dashboard/app/collect/[code]/components/CollectDetailSheet.test.tsxdashboard/app/collect/[code]/components/CollectDetailSheet.tsx
| const [preview, setPreview] = useState<CollectPreviewResponse | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| let cancelled = false; | ||
| apiClient.getCollectPreview(code, row.id).then((data) => { | ||
| if (!cancelled) setPreview(data); | ||
| }).catch(() => {}); | ||
| return () => { cancelled = true; }; | ||
| }, [code, row.id]); |
There was a problem hiding this comment.
Clear stale preview state before refetching.
When row.id or code changes, the previous song's embed/link stays on screen until the new request resolves. Since getCollectPreview() throws on non-2xx in dashboard/lib/api.ts:1258-1265, the empty catch also leaves stale preview content visible indefinitely after a 403/404/429. Reset preview before starting the request and in the error path.
Proposed fix
useEffect(() => {
let cancelled = false;
+ setPreview(null);
apiClient.getCollectPreview(code, row.id).then((data) => {
if (!cancelled) setPreview(data);
- }).catch(() => {});
+ }).catch(() => {
+ if (!cancelled) setPreview(null);
+ });
return () => { cancelled = true; };
}, [code, row.id]);🤖 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/collect/`[code]/components/CollectDetailSheet.tsx around lines
49 - 57, Reset the stale preview state before starting and on failed fetch:
inside the useEffect that depends on code and row.id, call setPreview(null)
immediately before invoking apiClient.getCollectPreview(code, row.id), and in
the catch handler call setPreview(null) as well (while preserving the cancelled
flag logic). This ensures the preview state managed by preview/setPreview is
cleared when row.id or code changes and when getCollectPreview throws.
1 participant
Summary
GET /api/public/collect/{code}/requests/{id}/previewendpoint — returnssource+source_urlfor a single request, rate-limited (10/min) with Turnstile human verificationCollectDetailSheetwhen user taps a song — Beatport falls back to "Open in Beatport" external link, manual entries show nothingAbuse mitigation
source_urlnot in leaderboard — per-request endpoint onlywrzdj_humancookie required (Turnstile)Test plan
codeprop + mockSummary by CodeRabbit
New Features
Tests
Documentation