feat(artists): add-by-Spotify-id rides POST /api/artists resolve-or-create (chat#1889 row 8) - #1901
Conversation
… (chat#1889 row 8) createRosterArtist passes spotify_artist_id and reports created (201) vs canonical-reused (200). addSpotifyArtist drops its client-side profileUrls PATCH -- the server attaches the social, and create-then- attach here is what minted a duplicate artist row per signup. The avatar is written only on 201: a reused canonical is shared state (chat#1866), and the image PATCH stays non-fatal (chat#1892). Contract: docs#278. API: api#793. Merge order docs -> api -> this. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesRoster artist flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant addSpotifyArtist
participant createRosterArtist
participant RosterAPI
participant saveArtist
addSpotifyArtist->>createRosterArtist: name, organization id, Spotify artist id
createRosterArtist->>RosterAPI: POST roster artist payload
RosterAPI-->>createRosterArtist: artist data and HTTP status
createRosterArtist-->>addSpotifyArtist: artist and created flag
addSpotifyArtist->>saveArtist: avatar image when created and available
saveArtist-->>addSpotifyArtist: update result or error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0a29e874
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { artist } = await createRosterArtist( | ||
| accessToken, | ||
| trimmed, | ||
| selectedOrgId, |
There was a problem hiding this comment.
In the /setup roster flow, the selected Spotify result's id is dropped before this call (AddArtistForm passes only name/profileUrl/image), so createRosterArtist still POSTs only { name, organization_id } and the hook then falls back to the old profileUrls PATCH. That means setup additions never use the new resolve-or-create-by-Spotify-id path and can still mint the duplicate canonical rows this change is trying to avoid; include a spotifyArtistId option here or route setup through addSpotifyArtist.
Useful? React with 👍 / 👎.
| const { artist: updated } = await saveArtist(accessToken, rosterArtist.account_id, { | ||
| image: imageUrl, | ||
| }); |
There was a problem hiding this comment.
Because this patch also changed the follow-up PATCH to contain only image, a client pointed at an API that ignores the new spotify_artist_id field will still get a successful create response but will no longer send the old profileUrls.SPOTIFY update. In that compatibility window the artist is added without any Spotify social, so catalog/enrichment flows that resolve Spotify from connected socials fail; either keep the old social PATCH until the API capability is guaranteed or gate on a response that proves the server attached the social.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- In
hooks/onboarding/useAddRosterArtist.ts,createRosterArtistis still called withoutspotifyArtistId, so the new resolve-or-create path may not run and onboarding can create incomplete/duplicate artist records; pass the Spotify ID through this call site and add a focused flow test around that branch. - In
lib/artists/addSpotifyArtist.ts, compatibility with older API deployments is fragile: artist creation can appear successful while silently dropping Spotify social linkage whenspotify_artist_idis unsupported, which risks confusing users and inconsistent data; add a capability-aware fallback (or enforce a coordinated API minimum version) so unsupported servers fail clearly or recover correctly.
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="hooks/onboarding/useAddRosterArtist.ts">
<violation number="1" location="hooks/onboarding/useAddRosterArtist.ts:48">
P2: The updated call site here still only passes `accessToken`, `trimmed`, and `selectedOrgId` to `createRosterArtist`, with no `spotifyArtistId` argument. Since `createRosterArtist` now supports resolve-or-create by Spotify id specifically to avoid minting duplicate canonical artist rows, the `/setup` roster flow (which has access to a Spotify id via `AddArtistForm`) will keep using the name-only path and can still create duplicate rows — the exact issue this PR is meant to fix.</violation>
</file>
<file name="lib/artists/addSpotifyArtist.ts">
<violation number="1" location="lib/artists/addSpotifyArtist.ts:23">
P2: When this client talks to an API deployment that does not yet support `spotify_artist_id`, adding a Spotify artist succeeds but its Spotify social is silently missing. Retaining a capability-aware fallback (or requiring the API deployment before using this client) would preserve the documented backward-compatible behavior without reintroducing duplicate creation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| throw new Error("Please sign in to add an artist"); | ||
| } | ||
| const artist = await createRosterArtist( | ||
| const { artist } = await createRosterArtist( |
There was a problem hiding this comment.
P2: The updated call site here still only passes accessToken, trimmed, and selectedOrgId to createRosterArtist, with no spotifyArtistId argument. Since createRosterArtist now supports resolve-or-create by Spotify id specifically to avoid minting duplicate canonical artist rows, the /setup roster flow (which has access to a Spotify id via AddArtistForm) will keep using the name-only path and can still create duplicate rows — the exact issue this PR is meant to fix.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/onboarding/useAddRosterArtist.ts, line 48:
<comment>The updated call site here still only passes `accessToken`, `trimmed`, and `selectedOrgId` to `createRosterArtist`, with no `spotifyArtistId` argument. Since `createRosterArtist` now supports resolve-or-create by Spotify id specifically to avoid minting duplicate canonical artist rows, the `/setup` roster flow (which has access to a Spotify id via `AddArtistForm`) will keep using the name-only path and can still create duplicate rows — the exact issue this PR is meant to fix.</comment>
<file context>
@@ -45,7 +45,7 @@ export function useAddRosterArtist() {
throw new Error("Please sign in to add an artist");
}
- const artist = await createRosterArtist(
+ const { artist } = await createRosterArtist(
accessToken,
trimmed,
</file context>
|
|
||
| const imageUrl = artist.images?.[0]?.url; | ||
| const { artist: updated } = await saveArtist( | ||
| const { artist: rosterArtist, created } = await createRosterArtist( |
There was a problem hiding this comment.
P2: When this client talks to an API deployment that does not yet support spotify_artist_id, adding a Spotify artist succeeds but its Spotify social is silently missing. Retaining a capability-aware fallback (or requiring the API deployment before using this client) would preserve the documented backward-compatible behavior without reintroducing duplicate creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/addSpotifyArtist.ts, line 23:
<comment>When this client talks to an API deployment that does not yet support `spotify_artist_id`, adding a Spotify artist succeeds but its Spotify social is silently missing. Retaining a capability-aware fallback (or requiring the API deployment before using this client) would preserve the documented backward-compatible behavior without reintroducing duplicate creation.</comment>
<file context>
@@ -4,27 +4,38 @@ import { createRosterArtist } from "@/lib/artists/createRosterArtist";
-
- const imageUrl = artist.images?.[0]?.url;
- const { artist: updated } = await saveArtist(
+ const { artist: rosterArtist, created } = await createRosterArtist(
accessToken,
- created.account_id,
</file context>
Preview verification — the new contract live from the client, cross-account reuse visible in the UI
The changed surface (
|
| Assertion | Result |
|---|---|
POST carries spotify_artist_id |
✅ in the body verbatim |
| Response 200 (reuse, not create) | ✅ |
Zero client PATCH with profileUrls |
✅ anyProfileUrlsPatch: false across the whole log |
| Zero image PATCH on the reused canonical | ✅ — created: false path, chat#1866 shared-state guard working |
| Rostered artist IS the cross-account canonical | ✅ GET /api/artists returns 0d5e37f8… with its server-attached social |
| SQL ground truth | 1 Grace Ives artist row globally, 2 roster links (the other account + this one) |
Grace Ives renders a placeholder avatar — this is correct, not a bug: her canonical was created via curl with no image, and this PR deliberately refuses to write the adding account's image onto a reused shared canonical. A safe follow-up if wanted: write the image when the canonical has none (filling a blank is not vandalising shared state).
Also observed, expected: the onboarding form still rides the old path on this preview
/setup/artists → AddArtistForm still calls useAddRosterArtist (POST name-only + PATCH profileUrls) — because the swap of that form to useAddSpotifyArtist is chat#1900, which is in draft. Until #1900 merges, onboarding adds still create per-signup rows (which the valuation then reuses server-side, so rosters stay clean — but table-level dedup for onboarding waits on #1900). Recommend merging #1900 promptly after this to close that window; I'll re-verify it on top of the trilogy first.
Suite recap
TDD red→green; chat suite 73 files / 275 tests; tsc at the 13 baseline; all four CI checks green.
Ready to merge. With this in, row 8's slice is complete; database#49 + api#794 close rows 9–10.
|
Follow-up on the placeholder-avatar note: tracked as chat#1889 matrix row 24, and the premise narrowed on investigation — Grace Ives' canonical self-healed ~30s after the screenshot: the seeded valuation's |
1 participant
chat leg of the row-8 trilogy on chat#1889. Merge order: docs#278 → api#793 → this. Depends on api#793 being deployed (falls back gracefully — an api without the param ignores it and behaves exactly as today).
createRosterArtistpassesspotify_artist_id, returns{ artist, created }(created: response.status === 201)addSpotifyArtistdrops its client-sideprofileUrlsPATCH — the server attaches the social; the client-side create-then-attach is what minted a duplicate row per signupuseAddRosterArtistcall-site updated for the new return shape (file is deleted by chat#1900, which stacks after this conceptually but shares no branch)TDD red→green (3 failed → green); full chat suite 73 files / 275 tests pass;
tscat the 13 baseline; eslint clean. Preview verification after api#793 merges + test-sync: two accounts adding the same Spotify artist must yield one artist row + two roster links (SQL-asserted).Tracked in chat#1889 (matrix row 8).
🤖 Generated with Claude Code
Summary by cubic
Add artists by Spotify ID via server-side resolve-or-create to ensure one canonical per Spotify artist and prevent duplicate rows. Drops client-side social patching and only writes the avatar when the artist is newly created.
New Features
createRosterArtistacceptsspotify_artist_idand returns{ artist, created }(createdis true on HTTP 201; false on 200).addSpotifyArtistpasses the ID and only PATCHes the image whencreatedis true; image PATCH errors are ignored.useAddRosterArtistupdated to consume the new return shape.Bug Fixes
profileUrlsPATCH that created duplicate artist rows on signup.Written for commit 3d0a29e. Summary will update on new commits.
Summary by CodeRabbit