fix(valuation): prefer an artist the account already rosters for a Spotify id (chat#1889 row 8 follow-up) - #792
Conversation
…otify id api#791 stopped the valuation minting a new artist row, but the chat add flow creates its own row (+ Spotify social) BEFORE the fire-and-forget valuation runs. When the global lookup then resolved a DIFFERENT canonical, insertAccountArtistId linked that one too and the account showed two roster entries again -- reproduced live on chat#1900's preview after api#791 merged, caught by Sweets in the artist sidebar. findCanonicalArtistBySpotifyId now collects every artist linked to the Spotify id and prefers one the requesting account already rosters; the global resolution is unchanged when the account rosters none of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 7 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. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- In
lib/valuation/findCanonicalArtistBySpotifyId.ts, loading the full account roster just to check candidate membership introduces O(n) behavior as rosters grow, which can slow valuation requests and increase DB/load pressure for larger accounts — switch to a targetedaccount_artist_idslookup filtered by both account and linked artist to keep this path constant-time.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/valuation/findCanonicalArtistBySpotifyId.ts">
<violation number="1" location="lib/valuation/findCanonicalArtistBySpotifyId.ts:44">
P2: Valuation now loads the requesting account's entire roster just to test candidate membership, making this path grow linearly with roster size. A targeted `account_artist_ids` query filtered by both account and linked artist IDs would avoid repeated full-roster reads.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client (Chat UI)
participant ChatAPI as POST /api/artists
participant Valuation as Valuation Job (fire-and-forget)
participant linkArtist as linkSearchedArtistToAccount
participant findCanonical as findCanonicalArtistBySpotifyId
participant DB as Supabase DB
Note over Client,DB: Before Fix – Duplicate Roster Entries
Client->>ChatAPI: Create artist row + Spotify social
ChatAPI->>DB: Insert artist, insert social, link via account_artist_ids
ChatAPI-->>Client: Row created
Note over Valuation: Runs ~20s later (async)
Valuation->>linkArtist: linkSearchedArtistToAccount(accountId, spotifyId)
linkArtist->>findCanonical: findCanonicalArtistBySpotifyId(spotifyId) <br/> (no accountId passed)
findCanonical->>DB: selectSocials(spotifyId)
DB-->>findCanonical: two socials (new + old)
findCanonical->>DB: selectAccountSocials(newSocialId)
DB-->>findCanonical: account_id of new artist
findCanonical->>DB: selectAccountSocials(oldSocialId)
DB-->>findCanonical: account_id of old canonical
Note over findCanonical: Returns first found (old canonical)
findCanonical-->>linkArtist: old-canonical-id
linkArtist->>DB: insertAccountArtistId(accountId, old-canonical-id)
Note over DB: Now account has two roster entries
Note over Client,DB: After Fix – Prefer Already-Rostered Artist
Client->>ChatAPI: Create artist row + Spotify social (same)
ChatAPI->>DB: Insert artist, insert social, link via account_artist_ids
ChatAPI-->>Client: Row created
Valuation->>linkArtist: linkSearchedArtistToAccount(accountId, spotifyId)
linkArtist->>findCanonical: CHANGED: findCanonicalArtistBySpotifyId(spotifyId, accountId)
findCanonical->>DB: selectSocials(spotifyId)
DB-->>findCanonical: two socials (new + old)
findCanonical->>DB: selectAccountSocials(newSocialId)
DB-->>findCanonical: account_id of new artist
findCanonical->>DB: selectAccountSocials(oldSocialId)
DB-->>findCanonical: account_id of old canonical
findCanonical->>DB: NEW: selectAccountArtistIds([accountId])
DB-->>findCanonical: array of already-rostered artist ids
alt accountId provided AND account already rosters one of the linked artists
Note over findCanonical: NEW: Prefer that artist (client-created row)
findCanonical-->>linkArtist: client-created artist id
else account rosters none of them (global reuse case)
Note over findCanonical: Fallback to first global (as before)
findCanonical-->>linkArtist: old-canonical-id
end
linkArtist->>DB: insertAccountArtistId (only if not already rostered)
Note over DB: Single roster entry preserved
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| return null; | ||
| if (accountId) { | ||
| const rostered = await selectAccountArtistIds([accountId]); |
There was a problem hiding this comment.
P2: Valuation now loads the requesting account's entire roster just to test candidate membership, making this path grow linearly with roster size. A targeted account_artist_ids query filtered by both account and linked artist IDs would avoid repeated full-roster reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/valuation/findCanonicalArtistBySpotifyId.ts, line 44:
<comment>Valuation now loads the requesting account's entire roster just to test candidate membership, making this path grow linearly with roster size. A targeted `account_artist_ids` query filtered by both account and linked artist IDs would avoid repeated full-roster reads.</comment>
<file context>
@@ -1,36 +1,53 @@
- return null;
+ if (accountId) {
+ const rostered = await selectAccountArtistIds([accountId]);
+ const rosteredIds = new Set(rostered.map(row => row.artist_id));
+ const alreadyRostered = linkedArtistIds.find(id => rosteredIds.has(id));
</file context>
|
Closing unmerged — superseded by the creation-side fix. Decision (Sweets, 2026-07-29): a canonical lookup must not need account context; the ambiguity this patch navigated exists because the chat add path still creates a row per signup. The real slice: (1) |
1 participant
Follow-up to api#791 — chat#1889 row 8. Found live by @sweetmantech during chat#1900's post-merge re-verification: the artist sidebar showed doubles again.
Why #791 wasn't enough
My "exactly 1 roster entry" assertion on the re-test raced the fire-and-forget valuation — it passed at +8s, and the valuation linked a second entry at ~+20s. Server truth after it completed:
#791 stops the fallback minting a new row. But the chat flow creates its own artist row before the valuation runs — so when the global lookup resolves a different canonical for the same Spotify id,
insertAccountArtistIdlinks that one too. No new row minted, two roster entries anyway. My #791 verification calledPOST /api/valuationdirectly, with no client-created row in the picture, which is exactly why it passed.What changed
findCanonicalArtistBySpotifyId(spotifyArtistId, accountId?)now collects every artist linked to the Spotify id and prefers one the requesting account already rosters — i.e. the row the client just created. Resolution stays global when the account rosters none of them (the cross-account reuse #791 verified is unchanged;accountIdis used only for preference, never to scope the search).Verification
TDD red → green:
The new preference test reproduces the live failure shape exactly (two socials, newest resolving to the client row, older to the old canonical, account rostering the client row). A second new test pins that global resolution survives when the account rosters nothing for the id.
tscat the 236 baseline; eslint clean.Live verification after merge + test-sync: I'll re-run the full chat#1900 cold-start walk and this time assert the roster count after the valuation completes, not before — with a DB cross-check.
Tracked in chat#1889 (matrix row 8, reopening).
Summary by cubic
Fixes duplicate artist roster entries by making the Spotify canonical lookup prefer an artist the requesting account already rosters. Global reuse stays the same when the account has none.
findCanonicalArtistBySpotifyIdto collect all artists linked to the Spotify id and return one already rostered by the requesting account; otherwise falls back to the first global match.linkSearchedArtistToAccountnow passesaccountIdto the lookup to avoid linking a second, different artist.Written for commit 18dbf01. Summary will update on new commits.