feat(setbuilder): pool import — 4 sources with source tagging + dedupe (#388)#414
Merged
Conversation
…emoval, import builders
…-scoped, rate-limited
…moval flows, badges
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a full pool candidate-track surface: DB migration and ORM models, pool service (import builders, dedupe, removal), API endpoints and types, dashboard ApiClient methods, UI components (PoolPanel, ImportModal, badges), CSS, and comprehensive backend + frontend tests. ChangesWrzDJSet Pool Import Feature
Sequence Diagram(s) sequenceDiagram
participant Browser
participant PoolPanel
participant ApiClient
participant SetbuilderAPI
participant PoolService
participant Database
Browser->>PoolPanel: open Import modal / enter playlist URL
PoolPanel->>ApiClient: previewPoolUrl(setId, url)
ApiClient->>SetbuilderAPI: POST /api/setbuilder/sets/{set_id}/pool/url-preview
SetbuilderAPI->>PoolService: preview_public_playlist(provider, playlist_id)
PoolService->>SetbuilderAPI: preview metadata
SetbuilderAPI-->>ApiClient: PoolUrlPreview
ApiClient-->>PoolPanel: preview data
Browser->>PoolPanel: confirm import
PoolPanel->>ApiClient: importPoolUrl(setId, url)
ApiClient->>SetbuilderAPI: POST /api/setbuilder/sets/{set_id}/pool/import/url
SetbuilderAPI->>PoolService: candidates_from_public_url(provider, playlist_id)
PoolService->>Database: get_or_create_source + import_candidates
Database-->>PoolService: inserted rows + dedupe counts
PoolService-->>SetbuilderAPI: PoolImportResult
SetbuilderAPI-->>ApiClient: PoolImportResult with pool snapshot
ApiClient-->>PoolPanel: import result -> update UI / show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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.
🧹 Nitpick comments (1)
server/tests/test_setbuilder_pool_service.py (1)
108-109: ⚡ Quick winConsider using position-independent assertions.
Lines 108-109 (and similar patterns at lines 125) assume
tracks[0]is the first inserted track. Ifpool.get_pool()doesn't include an explicitORDER BY, this could be flaky across different database engines or states.Recommend either:
- Verifying
get_poolhas explicit ordering (e.g.,ORDER BY created_at, id), or- Using a dictionary lookup pattern like the API tests (line 107-108 in
test_setbuilder_pool_api.py):by_title = {t.title: t for t in tracks} assert by_title["Song A"].camelot == "8B"🤖 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/tests/test_setbuilder_pool_service.py` around lines 108 - 109, The assertions in test_setbuilder_pool_service.py that use tracks[0] (e.g., checks of tracks[0].camelot and tracks[0].source_id) are brittle because get_pool() has no guaranteed ordering; replace position-dependent checks with a position-independent lookup: build a mapping from a stable key (e.g., title or id) to track objects (like by_title = {t.title: t for t in tracks}) and assert against by_title["Song A"].camelot and by_title["Song A"].source_id; also update the other similar assertion block (the one near the later check around line 125) to use the same dictionary lookup pattern, or alternatively add an explicit ORDER BY in the get_pool implementation if you prefer deterministic ordering.
🤖 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.
Nitpick comments:
In `@server/tests/test_setbuilder_pool_service.py`:
- Around line 108-109: The assertions in test_setbuilder_pool_service.py that
use tracks[0] (e.g., checks of tracks[0].camelot and tracks[0].source_id) are
brittle because get_pool() has no guaranteed ordering; replace
position-dependent checks with a position-independent lookup: build a mapping
from a stable key (e.g., title or id) to track objects (like by_title =
{t.title: t for t in tracks}) and assert against by_title["Song A"].camelot and
by_title["Song A"].source_id; also update the other similar assertion block (the
one near the later check around line 125) to use the same dictionary lookup
pattern, or alternatively add an explicit ORDER BY in the get_pool
implementation if you prefer deterministic ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc4b32e8-97a1-45f3-b917-f634375d39a3
📒 Files selected for processing (21)
dashboard/app/(dj)/setbuilder/[setId]/page.tsxdashboard/app/(dj)/setbuilder/components/ImportModal.tsxdashboard/app/(dj)/setbuilder/components/PoolBadges.tsxdashboard/app/(dj)/setbuilder/components/PoolPanel.tsxdashboard/app/(dj)/setbuilder/components/__tests__/PoolPanel.test.tsxdashboard/app/(dj)/setbuilder/setbuilder.module.cssdashboard/lib/api-types.generated.tsdashboard/lib/api-types.tsdashboard/lib/api.tsdocs/superpowers/plans/2026-06-09-setbuilder-pool-import.mdserver/alembic/versions/053_add_setbuilder_pool_tables.pyserver/app/api/setbuilder.pyserver/app/models/__init__.pyserver/app/models/set.pyserver/app/models/set_pool.pyserver/app/schemas/setbuilder.pyserver/app/services/setbuilder/playlist_url.pyserver/app/services/setbuilder/pool.pyserver/openapi.jsonserver/tests/test_setbuilder_pool_api.pyserver/tests/test_setbuilder_pool_service.py
Raises frontend branch coverage above the 68% CI threshold (67.23% -> 69.64%) by covering event picker, Tidal/Beatport playlist, public-URL validate+import, and manual search flows.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dashboard/app/(dj)/setbuilder/components/__tests__/ImportModal.test.tsx (1)
144-144: ⚡ Quick winPrefer
data-testidover class-based querySelector for the backdrop.The
[class*="modalBackdrop"]selector couples the test to CSS module implementation details. If the class name changes during refactoring, this test will break.♻️ Recommended fix using data-testid
Add
data-testid="modal-backdrop"to the backdrop element inImportModal.tsx, then update the test:- fireEvent.click(container.querySelector('[class*="modalBackdrop"]')!); + fireEvent.click(screen.getByTestId('modal-backdrop'));Alternatively, if the backdrop has an accessible role, prefer
getByRole.🤖 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/`(dj)/setbuilder/components/__tests__/ImportModal.test.tsx at line 144, Update the backdrop selection to avoid coupling to CSS class names: add a stable attribute (e.g., data-testid="modal-backdrop") to the backdrop element in the ImportModal component (ImportModal.tsx) and change the test in ImportModal.test.tsx to use the testing-library query (getByTestId('modal-backdrop') or screen.getByTestId) or, better, an accessible query like getByRole if applicable instead of container.querySelector('[class*="modalBackdrop"]'). This removes reliance on CSS-module class names and makes the test more robust.
🤖 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.
Nitpick comments:
In `@dashboard/app/`(dj)/setbuilder/components/__tests__/ImportModal.test.tsx:
- Line 144: Update the backdrop selection to avoid coupling to CSS class names:
add a stable attribute (e.g., data-testid="modal-backdrop") to the backdrop
element in the ImportModal component (ImportModal.tsx) and change the test in
ImportModal.test.tsx to use the testing-library query
(getByTestId('modal-backdrop') or screen.getByTestId) or, better, an accessible
query like getByRole if applicable instead of
container.querySelector('[class*="modalBackdrop"]'). This removes reliance on
CSS-module class names and makes the test more robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9fd1d67-6204-4fbb-9775-f6680ce43175
📒 Files selected for processing (1)
dashboard/app/(dj)/setbuilder/components/__tests__/ImportModal.test.tsx
…links) - keep both additive sections in schemas/setbuilder.py, api.ts, api-types.ts, builder page imports - re-anchor migration 053 onto 054 (share-token migration merged via PR #413) - regenerate openapi.json + api-types.generated.ts from merged backend - verified: alembic upgrade head + check clean, backend 2638 passed (87.64%), frontend tsc + 1037 tests green
thewrz
added a commit
that referenced
this pull request
Jun 9, 2026
#414 merged to main (migration 053, pool feature) after #413 (054), making main's chain 052 -> 054 -> 053. Resolved additive conflicts (curve + pool + share sections coexist) in setbuilder schemas/router, models __init__, api client, api-types, and the builder page. Regenerated openapi.json and api-types.generated.ts. Re-anchored 055 down_revision -> 053; verified single head and alembic upgrade/check on a fresh database (052 -> 054 -> 053 -> 055).
This was referenced Jun 11, 2026
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.
Closes #388
Summary
set_pool_sources,set_pool_tracks) — migration 053, cascade undersets; every track tagged with its import source (importedViachip),track_idfollows the TrackVibe namespaced-string convention (tidal:123,request:7, …)server/app/services/setbuilder/pool.py): dedupe-on-import (exact ISRC first, then normalized artist+title signature viatrack_normalizer; first import wins, original source tag preserved), per-set source reuse, removal flows (by ids / by source), candidate builders for all five flowsplaylist_url.py): https-only, exact-host allowlist, strict ID charsets, no userinfo/port tricks — the user URL is never fetched (SSRF defense); IDs are extracted and pulled via official APIs (spotipy client-credentials for Spotify, DJ's connected Tidal session for Tidal)/api/setbuilder):GET …/pool,GET /playlists(picker),POST …/pool/import/{event,tidal,beatport,url,manual},POST …/pool/url-preview,POST …/pool/tracks/remove,DELETE …/pool/sources/{id}— allget_current_active_user, owner-or-404, rate-limited, Pydantic-constrained inputsdashboard/app/(dj)/setbuilder/components/):PoolPanel(sources accordion w/ click-to-filter + hover-× remove, type tabs w/ live counts, search, multi-select footer, right-click context menu, toast "N new · M de-duped"),ImportModal(event picker / Tidal / Beatport / public-URL validate→preview→import / manual debounced search),PoolBadges(Camelot vialib/camelot-colors, BPM, energy bars, source icons)Design decisions
supported: falsewith a clear message — no public-API credentials exist in this codebase for them. Beatport URLs route DJs to the OAuth picker instead.UNIQUE(set_id, dedupe_sig)keeps dedupe honest under concurrent imports; ISRC matching is handled at the service layer (normalized, dash-stripped, uppercased).energyis nullable — TrackVibe enrichment is WrzDJSet: TrackVibe LLM enrichment + community vibe display (read) #391; the badge renders empty bars until then.artwork_urlmust be https (Pydantic pattern) — defensive against scheme tricks in stored/rendered image URLs."Song (Club Mix)") so named remixes don't fuzzy-collide with originals (the normalizer preserves named remixes).Migration note
Migration 053 anchored on 052; sibling PR for #398 uses slot 054 — second to merge re-anchors.
Test plan
alembic upgrade head && alembic checkclean on a fresh DB🤖 Generated with Claude Code
Summary by CodeRabbit