feat(artists): POST /api/artists resolve-or-create by spotify_artist_id (chat#1889 row 8) - #793
Merged
Merged
Conversation
…r a spotify_artist_id One canonical artist per Spotify id, enforced where rows are born (chat#1889 rows 8-10, decision 2026-07-29). The chat add path created a new artist row per signup, so every downstream canonical lookup was ambiguous -- the root cause of the roster doubles that api#791 patched downstream and api#792 (closed unmerged) tried to patch with account context. Optional spotify_artist_id on the create body: canonical exists -> link to the account, return 200; none -> create + attach the SPOTIFY social server-side (non-fatal, same contract as chat#1892), return 201. Contract: docs#278. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
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. |
📝 WalkthroughWalkthroughArtist creation now accepts an optional Spotify ID, resolves existing canonical artists and roster links, creates missing artists, and reports 201 for new records or 200 for existing canonical artists. ChangesArtist resolve-or-create
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant createArtistPostHandler
participant validateCreateArtistBody
participant resolveOrCreateArtist
participant ArtistDB
participant account_artist_ids
Client->>createArtistPostHandler: POST artist request
createArtistPostHandler->>validateCreateArtistBody: validate body
validateCreateArtistBody-->>createArtistPostHandler: validated fields
createArtistPostHandler->>resolveOrCreateArtist: resolve or create artist
resolveOrCreateArtist->>ArtistDB: find canonical artist by Spotify ID
alt Existing canonical artist
resolveOrCreateArtist->>account_artist_ids: link artist to account
resolveOrCreateArtist-->>createArtistPostHandler: artist, created=false
createArtistPostHandler-->>Client: HTTP 200
else New artist
resolveOrCreateArtist->>ArtistDB: create artist row
resolveOrCreateArtist-->>createArtistPostHandler: artist, created=true
createArtistPostHandler-->>Client: HTTP 201
end
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 |
34 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/artists/resolveOrCreateArtist.ts (2)
57-64: 🩺 Stability & Availability | 🔵 TrivialKeep non-fatal social failures observable.
The intentional
catchpreserves the successful create response, but an empty catch makes failed Spotify attachments indistinguishable from successful ones. Emit a structured warning or metric without logging raw request data; keep the operation non-fatal as documented.🤖 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 `@lib/artists/resolveOrCreateArtist.ts` around lines 57 - 64, Update the catch block around updateArtistSocials in resolveOrCreateArtist to emit a structured warning or metric identifying the failed Spotify attachment without including raw request data. Preserve the documented non-fatal behavior and successful artist creation response.
35-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the artist request flow small and focused.
All three functions combine multiple responsibilities and exceed the 20-line guideline. Extract focused helpers for validation/auth, canonical resolution/linking, social attachment, and HTTP response mapping.
lib/artists/resolveOrCreateArtist.ts#L35-L67: split lookup/linking, creation, and social attachment.lib/artists/validateCreateArtistBody.ts#L35-L68: split body validation from authorization and result mapping.lib/artists/createArtistPostHandler.ts#L29-L47: split response/error mapping from route orchestration.As per coding guidelines: “Flag functions longer than 20 lines” and “Keep functions small and focused.”
🤖 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 `@lib/artists/resolveOrCreateArtist.ts` around lines 35 - 67, Reduce the oversized artist request flow by extracting focused helpers: in lib/artists/resolveOrCreateArtist.ts lines 35-67, separate canonical lookup/linking, artist creation, and Spotify social attachment while preserving behavior; in lib/artists/validateCreateArtistBody.ts lines 35-68, separate request-body validation, authorization, and result mapping; in lib/artists/createArtistPostHandler.ts lines 29-47, separate response/error mapping from route orchestration. Keep each public function under the 20-line guideline and reuse the existing symbols and behavior.Source: Coding guidelines
lib/artists/validateCreateArtistBody.ts (1)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the request contract from the Zod schema.
ValidatedCreateArtistRequestis handwritten, so the schema and TypeScript contract can drift. Export az.infer<typeof createArtistBodySchema>type and compose it with the normalized authorization fields as needed.As per path instructions: “For validation functions, ensure: Use Zod for schema validation … Export inferred types for validated data.”
🤖 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 `@lib/artists/validateCreateArtistBody.ts` around lines 16 - 21, Update ValidatedCreateArtistRequest to derive its request fields from createArtistBodySchema using z.infer, then compose that inferred input with the normalized authorization fields accountId and organizationId as needed. Remove the duplicated handwritten request properties while preserving the optional spotifyArtistId contract and export the resulting validated type.Source: Path instructions
🤖 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 `@lib/artists/resolveOrCreateArtist.ts`:
- Around line 40-55: Make the Spotify artist resolution and creation flow in
resolveOrCreateArtist atomic: enforce a unique canonical Spotify social identity
in the schema, then use a transaction or atomic upsert to combine artist
creation with Spotify social attachment so concurrent requests cannot create
duplicate artists. Replace the selectAccountArtistId/insertAccountArtistId
sequence with an idempotent or conflict-safe roster-link insert, preserving the
existing return behavior for the canonical artist.
In `@lib/artists/validateCreateArtistBody.ts`:
- Line 9: Update the spotify_artist_id schema in validateCreateArtistBody to
trim input before validation, reject values that become empty after trimming,
and pass the trimmed value through to resolveOrCreateArtist so whitespace-only
IDs are not treated as present during social-link creation.
---
Nitpick comments:
In `@lib/artists/resolveOrCreateArtist.ts`:
- Around line 57-64: Update the catch block around updateArtistSocials in
resolveOrCreateArtist to emit a structured warning or metric identifying the
failed Spotify attachment without including raw request data. Preserve the
documented non-fatal behavior and successful artist creation response.
- Around line 35-67: Reduce the oversized artist request flow by extracting
focused helpers: in lib/artists/resolveOrCreateArtist.ts lines 35-67, separate
canonical lookup/linking, artist creation, and Spotify social attachment while
preserving behavior; in lib/artists/validateCreateArtistBody.ts lines 35-68,
separate request-body validation, authorization, and result mapping; in
lib/artists/createArtistPostHandler.ts lines 29-47, separate response/error
mapping from route orchestration. Keep each public function under the 20-line
guideline and reuse the existing symbols and behavior.
In `@lib/artists/validateCreateArtistBody.ts`:
- Around line 16-21: Update ValidatedCreateArtistRequest to derive its request
fields from createArtistBodySchema using z.infer, then compose that inferred
input with the normalized authorization fields accountId and organizationId as
needed. Remove the duplicated handwritten request properties while preserving
the optional spotifyArtistId contract and export the resulting validated type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8bb0c6b-ac5c-4fe8-9454-226a05cb9f8b
⛔ Files ignored due to path filters (3)
lib/artists/__tests__/createArtistPostHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/artists/__tests__/resolveOrCreateArtist.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/artists/__tests__/validateCreateArtistBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
lib/artists/createArtistPostHandler.tslib/artists/resolveOrCreateArtist.tslib/artists/validateCreateArtistBody.ts
Comment on lines
+40
to
+55
| if (spotifyArtistId) { | ||
| const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId); | ||
| if (canonicalId) { | ||
| const alreadyRostered = await selectAccountArtistId(accountId, canonicalId); | ||
| if (!alreadyRostered) { | ||
| await insertAccountArtistId(accountId, canonicalId); | ||
| } | ||
| const artist = await selectAccountWithSocials(canonicalId); | ||
| return { | ||
| artist: artist ? { ...artist, account_id: artist.id } : null, | ||
| created: false, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const created = await createArtistInDb(name, accountId, organizationId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C 5 'findCanonicalArtistBySpotifyId|createArtistInDb|selectAccountArtistId|insertAccountArtistId' lib
rg -n -C 3 'spotify|account_artist_ids|unique' supabase libRepository: recoupable/api
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files containing resolveOrCreateArtist:"
fd -a 'resolveOrCreateArtist\.ts$' . | sed 's#^\./##'
file="$(fd 'resolveOrCreateArtist\.ts$' . | head -n 1)"
echo "---- $file ----"
wc -l "$file"
ast-grep outline "$file" --view expanded || true
sed -n '1,180p' "$file" | cat -n
echo "---- Supabase table definitions around artists/socials/account_artist_ids ----"
rg -n -C 8 'artists|spotify_id|account_artist|account_social|i[._-]?new|index|unique|constraint|account_socials' supabase lib | head -n 260Repository: recoupable/api
Length of output: 23226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate artifact files:"
git ls-files | rg '(^|/)(artist|artists|account_artist|artist_social|account_social|account_socials|spotify).*(sql|ts)$|supabase/.*/.*(artist|spotify|social).*' | head -n 300
for f in $(git ls-files | rg '(^|/)(artist|artists|spotify|socials?).*\.(sql|ts)$' | head -n 80); do
echo "---- $f ----"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
doneRepository: recoupable/api
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate Supabase schema and migrations:"
git ls-files | rg '(^|/)(supabase|migrations|.*\.sql$)' | head -n 200
echo "---- Files directly named around artists/socials/account_artist_ids sql ----"
git ls-files | rg '(artist|artists|account_artist|artist_social|account_social|socials).*\.sql$' | sort
echo "---- Relevant SQL snippets ----"
for f in $(git ls-files | rg '(artist|artists|account_artist|artist_social|account_social|socials).*\.sql$' | sort | head -n 80); do
echo "---- $f ----"
rg -n -C 8 'artists|spotify|unique|unique .*spotify|account_artist|account_social|unique .*account.*social|constraint.*account.*social|i[._-]?new|create table' "$f" || true
doneRepository: recoupable/api
Length of output: 11060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- candidate value/spotify canonical files ----"
git ls-files | rg '(^|/)findCanonicalArtistBySpotifyId\.ts$|spotify|artist' | head -n 200
echo "---- implementation files ----"
for f in $(git ls-files | rg '(^|/)findCanonicalArtistBySpotifyId\.ts$|(^|/)createArtistInDb\.ts$|(^|/)(insertAccountArtistId|selectAccountArtistId|updateArtistSocials|insertAccountSocial|socials).*\.ts$' | sort); do
echo "---- $f ----"
wc -l "$f"
ast-grep outline "$f" --view expanded || true
sed -n '1,220p' "$f" | cat -n
done
echo "---- schema searches for SPOTIFY/social/account_artist constraints ----"
rg -n -C 6 'SPOTIFY|account_socials|socials|account_artist_ids|spotify|create table|constraint|unique|create unique|i[_-]?new|unique constraint|unique' lib/supabase/lib/supabase | head -n 350Repository: recoupable/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- target implementation files ----"
for f in \
lib/valuation/findCanonicalArtistBySpotifyId.ts \
lib/artists/createArtistInDb.ts \
lib/supabase/account_artist_ids/selectAccountArtistId.ts \
lib/supabase/account_artist_ids/insertAccountArtistId.ts \
lib/supabase/accounts/selectAccountWithSocials.ts \
lib/supabase/account_socials/insertAccountSocial.ts \
lib/supabase/socials/selectSocials.ts \
lib/supabase/socials/upsertSocials.ts
do
if [ -f "$f" ]; then
echo "---- $f ----"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
else
echo "MISSING: $f"
fi
done
echo "---- schema/migration SQL files ----"
git ls-files \*.{sql|ddl}| head -n 200
for f in $(git ls-files \*.{sql|ddl} | head -n 200); do
echo "---- $f: relevant schema matches ----"
rg -n -C 5 'create table|account_socials|socials|account_artist_ids|spotify|unique constraint|unique|SPOTIFY|profile_url' "$f" | head -n 180
doneRepository: recoupable/api
Length of output: 12821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- all SQL files ----"
git ls-files | rg '\.sql$' | sort
echo "---- schema/migration relevant constraints ----"
for f in $(git ls-files | rg '\.sql$' | sort); do
matches=$(rg -n -C 5 'account_socials|socials|account_artist_ids|spotify|unique constraint|unique|SPOTIFY|profile_url' "$f" || true)
if [ -n "$matches" ]; then
echo "---- $f ----"
echo "$matches"
fi
done
echo "---- deterministic race-order probe ----"
python3 - <<'PY'
sequence = [
("A", "findCanonical", None),
("B", "findCanonical", None),
("A", "createArtistInDb", "artist-A"),
("B", "createArtistInDb", "artist-B"),
("A", "updateArtistSocials", "artist-A"),
("B", "updateArtistSocials", "artist-B"),
]
for step in sequence:
print(f"{step[0]}: {step[1]} -> {step[2]}")
print("\nTwo artist rows exist after interleaved createArtistInDb calls, each with same Spotify social: duplicate canonical artist rows.")
PYRepository: recoupable/api
Length of output: 176
Make canonical artist lookup, social attach, and roster linking atomic.
findCanonicalArtistBySpotifyId() only searches socials by substring, while the artist/social row may not exist until after createArtistInDb() returns. Concurrent requests can each miss the canonical artist, insert separate artist rows, then attach the same Spotify social. The later selectAccountArtistId() / insertAccountArtistId() sequence has the same issue and can also generate duplicate roster links.
Enforce one canonical Spotify artist in the schema, use an atomic upsert/transaction for create + social attach, and use an idempotent or conflict-safe insert for account_artist_ids.
🤖 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 `@lib/artists/resolveOrCreateArtist.ts` around lines 40 - 55, Make the Spotify
artist resolution and creation flow in resolveOrCreateArtist atomic: enforce a
unique canonical Spotify social identity in the schema, then use a transaction
or atomic upsert to combine artist creation with Spotify social attachment so
concurrent requests cannot create duplicate artists. Replace the
selectAccountArtistId/insertAccountArtistId sequence with an idempotent or
conflict-safe roster-link insert, preserving the existing return behavior for
the canonical artist.
|
|
||
| export const createArtistBodySchema = z.object({ | ||
| name: z.string({ message: "name is required" }).min(1, "name cannot be empty"), | ||
| spotify_artist_id: z.string().min(1, "spotify_artist_id cannot be empty").optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'validateCreateArtistBody\.ts$' . || true
echo "== file outline / contents =="
file="$(fd 'validateCreateArtistBody\.ts$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo "== search related spotify_artist_id usages =="
rg -n "spotify_artist_id|artist.socials|socials|spotify|createArtist|create" -S --glob '*.ts' --glob '*.js' lib app 2>/dev/null || true
echo "== deterministic zod-like whitespace probe if zod/schema is present (readonly) =="
(node - <<'JS' || true)
const candidate = ` spotify_artist_id: z.string().min(1, "spotify_artist_id cannot be empty").optional(),`;
console.log("has min1_optional:", !!candidate.match(/z\.string\(\)\.min\(1[^\n]*\.optional\(\)/));
console.log("has trim:", !!candidate.includes(".trim()"));
JSRepository: recoupable/api
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'validateCreateArtistBody\.ts$' . | tail -n1)"
echo "== schema and validation body =="
sed -n '1,90p' "$file" | cat -n
echo "== handler pass-through =="
sed -n '1,70p' lib/artists/createArtistPostHandler.ts | cat -n
echo "== resolveOrCreateArtist implementation =="
sed -n '1,90p' lib/artists/resolveOrCreateArtist.ts | cat -n
echo "== tests around allowed empty ids and whitespace =="
sed -n '213,290p' lib/artists/__tests__/validateCreateArtistBody.test.ts | cat -nRepository: recoupable/api
Length of output: 11836
Reject whitespace-only Spotify IDs.
.min(1) still allows " "; this value reaches resolveOrCreateArtist() as a present spotifyArtistId and is interpolated into the Spotify URL rather than being treated as missing. Trim in the schema and pass the trimmed value through so empty/whitespace IDs are rejected before social-link creation.
🤖 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 `@lib/artists/validateCreateArtistBody.ts` at line 9, Update the
spotify_artist_id schema in validateCreateArtistBody to trim input before
validation, reject values that become empty after trimming, and pass the trimmed
value through to resolveOrCreateArtist so whitespace-only IDs are not treated as
present during social-link creation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 6 files
Confidence score: 2/5
- In
lib/artists/resolveOrCreateArtist.ts, the canonical lookup/create flow is not atomic, so concurrent requests can create multiple artist accounts for the same entity and fragment ownership/linking state — switch this path to a single transactional upsert or DB-enforced atomic recheck. - In
lib/artists/resolveOrCreateArtist.ts(and input handling inlib/artists/validateCreateArtistBody.ts),spotifyArtistIdcan be treated as anILIKEpattern (%/_or whitespace-only), which can resolve and attach the wrong artist; this is a concrete mis-link risk for callers — enforce strict Spotify ID format and trim before validation/querying. - In
lib/artists/resolveOrCreateArtist.ts, the roster existence-check then insert can race under concurrent reuse requests, leading to duplicateaccount_artist_idsrows or a uniqueness-conflict 500 — make the link step idempotent withinsert ... on conflict/upsert semantics. - In
lib/artists/createArtistPostHandler.ts, database error text can leak directly to clients on 500s, and related tests inlib/artists/__tests__/resolveOrCreateArtist.test.tsandlib/artists/__tests__/createArtistPostHandler.test.tsno longer verify key behavior, increasing regression risk — return a fixed generic error message and tighten assertions around social-attach attempts andorganizationIdforwarding.
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/artists/createArtistPostHandler.ts">
<violation number="1" location="lib/artists/createArtistPostHandler.ts:29">
P2: A failed canonical-to-account link can now return raw database error text to API clients through this new resolver call. The 500 path should log the exception server-side and return a fixed generic message such as `Internal server error`.</violation>
</file>
<file name="lib/artists/resolveOrCreateArtist.ts">
<violation number="1" location="lib/artists/resolveOrCreateArtist.ts:41">
P1: A non-empty `spotifyArtistId` containing `%` or `_` is treated as an `ILIKE` wildcard, so a caller can resolve and link an unrelated artist instead of the requested ID. Restrict the ID to the accepted Spotify-ID format or escape `ILIKE` metacharacters before lookup and URL construction.</violation>
<violation number="2" location="lib/artists/resolveOrCreateArtist.ts:41">
P1: Concurrent requests can both observe no canonical and create separate artist accounts because the lookup and creation are not atomic; the later social upsert does not make the artist rows unique. An atomic database resolve/create operation with a unique Spotify identity is needed to preserve the one-canonical-per-ID contract.</violation>
<violation number="3" location="lib/artists/resolveOrCreateArtist.ts:43">
P2: Concurrent reuse requests can race between the roster existence check and insert, causing duplicate `account_artist_ids` rows or a 500 on a uniqueness conflict. An idempotent database upsert/insert-on-conflict operation would make the reuse path safe.</violation>
</file>
<file name="lib/artists/__tests__/createArtistPostHandler.test.ts">
<violation number="1" location="lib/artists/__tests__/createArtistPostHandler.test.ts:56">
P3: Test 'passes organization_id to createArtistInDb' title now references a function that is no longer the public API, and the assertion no longer verifies organizationId was forwarded. The old test explicitly checked the org UUID in the third arg; the new assertion only checks name and accountId, so it would pass even if organizationId were dropped. Either add organizationId to the objectContaining matcher or rename the test to reflect what it actually validates.</violation>
</file>
<file name="lib/artists/validateCreateArtistBody.ts">
<violation number="1" location="lib/artists/validateCreateArtistBody.ts:9">
P2: Add `.trim()` to `spotify_artist_id` validation to reject whitespace-only strings — without it a whitespace-only value would pass `.min(1)`, produce a broad `ilike` query that could match unrelated social rows, and create a broken Spotify social URL.</violation>
</file>
<file name="lib/artists/__tests__/resolveOrCreateArtist.test.ts">
<violation number="1" location="lib/artists/__tests__/resolveOrCreateArtist.test.ts:51">
P2: Test "returns the created artist when the social attach fails" verifies resilience to a rejected updateArtistSocials but doesn't assert the social attach was attempted. If a regression causes the function to silently skip updateArtistSocials (no call), the mock rejection never fires and the test passes trivially because it only checks the return value. Add `expect(updateArtistSocials).toHaveBeenCalledWith("new-1", { SPOTIFY: \`https://open.spotify.com/artist/${SPOTIFY_ID}\` });` to confirm the action was attempted before the catch.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant API as createArtistPostHandler
participant Validator as validateCreateArtistBody
participant Resolver as resolveOrCreateArtist
participant DB as Database
Note over Client,DB: POST /api/artists – Resolve-or-create by Spotify ID
Client->>API: POST /api/artists<br/>{name, spotify_artist_id?, account_id?, organization_id?}
API->>Validator: validateCreateArtistBody(request)
Validator->>DB: Validate auth context & body
DB-->>Validator: Account + parsed body
alt Validation fails
Validator-->>API: NextResponse (400)
API-->>Client: Error response
else Valid
Validator-->>API: {name, accountId, organizationId, spotifyArtistId?}
API->>Resolver: resolveOrCreateArtist({name, accountId, organizationId, spotifyArtistId})
alt spotifyArtistId provided
Resolver->>DB: findCanonicalArtistBySpotifyId(spotifyArtistId)
alt Canonical exists
DB-->>Resolver: canonicalId
Resolver->>DB: selectAccountArtistId(accountId, canonicalId)
opt Not already rostered
DB-->>Resolver: null
Resolver->>DB: insertAccountArtistId(accountId, canonicalId)
end
Resolver->>DB: selectAccountWithSocials(canonicalId)
DB-->>Resolver: artist data
Resolver-->>API: {artist, created: false}
else No canonical exists
DB-->>Resolver: null
Resolver->>DB: createArtistInDb(name, accountId, organizationId)
alt Artist created
DB-->>Resolver: artist
opt spotifyArtistId present
Resolver->>DB: updateArtistSocials(artist.id,<br/>{SPOTIFY: "https://..."})
alt Update fails
DB->>Resolver: error
Note over Resolver: Non‑fatal – swallow
end
end
Resolver-->>API: {artist, created: true}
else Creation fails
DB-->>Resolver: null
Resolver-->>API: {artist: null, created: true}
end
end
else No spotifyArtistId (legacy plain create)
Resolver->>DB: createArtistInDb(name, accountId, organizationId)
DB-->>Resolver: artist
Resolver-->>API: {artist, created: true}
end
alt artist is null
API-->>Client: 500 – "Failed to create artist"
else created === false
API-->>Client: 200 – {artist}
else created === true
API-->>Client: 201 – {artist}
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const { name, accountId, organizationId, spotifyArtistId } = params; | ||
|
|
||
| if (spotifyArtistId) { | ||
| const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId); |
There was a problem hiding this comment.
P1: A non-empty spotifyArtistId containing % or _ is treated as an ILIKE wildcard, so a caller can resolve and link an unrelated artist instead of the requested ID. Restrict the ID to the accepted Spotify-ID format or escape ILIKE metacharacters before lookup and URL construction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/resolveOrCreateArtist.ts, line 41:
<comment>A non-empty `spotifyArtistId` containing `%` or `_` is treated as an `ILIKE` wildcard, so a caller can resolve and link an unrelated artist instead of the requested ID. Restrict the ID to the accepted Spotify-ID format or escape `ILIKE` metacharacters before lookup and URL construction.</comment>
<file context>
@@ -0,0 +1,68 @@
+ const { name, accountId, organizationId, spotifyArtistId } = params;
+
+ if (spotifyArtistId) {
+ const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId);
+ if (canonicalId) {
+ const alreadyRostered = await selectAccountArtistId(accountId, canonicalId);
</file context>
| const { name, accountId, organizationId, spotifyArtistId } = params; | ||
|
|
||
| if (spotifyArtistId) { | ||
| const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId); |
There was a problem hiding this comment.
P1: Concurrent requests can both observe no canonical and create separate artist accounts because the lookup and creation are not atomic; the later social upsert does not make the artist rows unique. An atomic database resolve/create operation with a unique Spotify identity is needed to preserve the one-canonical-per-ID contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/resolveOrCreateArtist.ts, line 41:
<comment>Concurrent requests can both observe no canonical and create separate artist accounts because the lookup and creation are not atomic; the later social upsert does not make the artist rows unique. An atomic database resolve/create operation with a unique Spotify identity is needed to preserve the one-canonical-per-ID contract.</comment>
<file context>
@@ -0,0 +1,68 @@
+ const { name, accountId, organizationId, spotifyArtistId } = params;
+
+ if (spotifyArtistId) {
+ const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId);
+ if (canonicalId) {
+ const alreadyRostered = await selectAccountArtistId(accountId, canonicalId);
</file context>
| validated.accountId, | ||
| validated.organizationId, | ||
| ); | ||
| const { artist, created } = await resolveOrCreateArtist({ |
There was a problem hiding this comment.
P2: A failed canonical-to-account link can now return raw database error text to API clients through this new resolver call. The 500 path should log the exception server-side and return a fixed generic message such as Internal server error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/createArtistPostHandler.ts, line 29:
<comment>A failed canonical-to-account link can now return raw database error text to API clients through this new resolver call. The 500 path should log the exception server-side and return a fixed generic message such as `Internal server error`.</comment>
<file context>
@@ -26,11 +26,12 @@ export async function createArtistPostHandler(request: NextRequest): Promise<Nex
- validated.accountId,
- validated.organizationId,
- );
+ const { artist, created } = await resolveOrCreateArtist({
+ name: validated.name,
+ accountId: validated.accountId,
</file context>
| if (spotifyArtistId) { | ||
| const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId); | ||
| if (canonicalId) { | ||
| const alreadyRostered = await selectAccountArtistId(accountId, canonicalId); |
There was a problem hiding this comment.
P2: Concurrent reuse requests can race between the roster existence check and insert, causing duplicate account_artist_ids rows or a 500 on a uniqueness conflict. An idempotent database upsert/insert-on-conflict operation would make the reuse path safe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/resolveOrCreateArtist.ts, line 43:
<comment>Concurrent reuse requests can race between the roster existence check and insert, causing duplicate `account_artist_ids` rows or a 500 on a uniqueness conflict. An idempotent database upsert/insert-on-conflict operation would make the reuse path safe.</comment>
<file context>
@@ -0,0 +1,68 @@
+ if (spotifyArtistId) {
+ const canonicalId = await findCanonicalArtistBySpotifyId(spotifyArtistId);
+ if (canonicalId) {
+ const alreadyRostered = await selectAccountArtistId(accountId, canonicalId);
+ if (!alreadyRostered) {
+ await insertAccountArtistId(accountId, canonicalId);
</file context>
|
|
||
| export const createArtistBodySchema = z.object({ | ||
| name: z.string({ message: "name is required" }).min(1, "name cannot be empty"), | ||
| spotify_artist_id: z.string().min(1, "spotify_artist_id cannot be empty").optional(), |
There was a problem hiding this comment.
P2: Add .trim() to spotify_artist_id validation to reject whitespace-only strings — without it a whitespace-only value would pass .min(1), produce a broad ilike query that could match unrelated social rows, and create a broken Spotify social URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/validateCreateArtistBody.ts, line 9:
<comment>Add `.trim()` to `spotify_artist_id` validation to reject whitespace-only strings — without it a whitespace-only value would pass `.min(1)`, produce a broad `ilike` query that could match unrelated social rows, and create a broken Spotify social URL.</comment>
<file context>
@@ -6,6 +6,7 @@ import { z } from "zod";
export const createArtistBodySchema = z.object({
name: z.string({ message: "name is required" }).min(1, "name cannot be empty"),
+ spotify_artist_id: z.string().min(1, "spotify_artist_id cannot be empty").optional(),
account_id: z.uuid({ message: "account_id must be a valid UUID" }).optional(),
organization_id: z.uuid({ message: "organization_id must be a valid UUID" }).optional(),
</file context>
Suggested change
| spotify_artist_id: z.string().min(1, "spotify_artist_id cannot be empty").optional(), | |
| spotify_artist_id: z.string().trim().min(1, "spotify_artist_id cannot be empty").optional(), |
| expect(updateArtistSocials).toHaveBeenCalledWith("new-1", { | ||
| SPOTIFY: `https://open.spotify.com/artist/${SPOTIFY_ID}`, | ||
| }); | ||
| expect(result).toEqual({ artist: created, created: true }); |
There was a problem hiding this comment.
P2: Test "returns the created artist when the social attach fails" verifies resilience to a rejected updateArtistSocials but doesn't assert the social attach was attempted. If a regression causes the function to silently skip updateArtistSocials (no call), the mock rejection never fires and the test passes trivially because it only checks the return value. Add expect(updateArtistSocials).toHaveBeenCalledWith("new-1", { SPOTIFY: \https://open.spotify.com/artist/${SPOTIFY_ID}\` });` to confirm the action was attempted before the catch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/__tests__/resolveOrCreateArtist.test.ts, line 51:
<comment>Test "returns the created artist when the social attach fails" verifies resilience to a rejected updateArtistSocials but doesn't assert the social attach was attempted. If a regression causes the function to silently skip updateArtistSocials (no call), the mock rejection never fires and the test passes trivially because it only checks the return value. Add `expect(updateArtistSocials).toHaveBeenCalledWith("new-1", { SPOTIFY: \`https://open.spotify.com/artist/${SPOTIFY_ID}\` });` to confirm the action was attempted before the catch.</comment>
<file context>
@@ -0,0 +1,107 @@
+ expect(updateArtistSocials).toHaveBeenCalledWith("new-1", {
+ SPOTIFY: `https://open.spotify.com/artist/${SPOTIFY_ID}`,
+ });
+ expect(result).toEqual({ artist: created, created: true });
+ });
+
</file context>
Comment on lines
+56
to
58
| expect(mockResolveOrCreateArtist).toHaveBeenCalledWith( | ||
| expect.objectContaining({ name: "Test Artist", accountId: "api-key-account-id" }), | ||
| ); |
There was a problem hiding this comment.
P3: Test 'passes organization_id to createArtistInDb' title now references a function that is no longer the public API, and the assertion no longer verifies organizationId was forwarded. The old test explicitly checked the org UUID in the third arg; the new assertion only checks name and accountId, so it would pass even if organizationId were dropped. Either add organizationId to the objectContaining matcher or rename the test to reflect what it actually validates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/__tests__/createArtistPostHandler.test.ts, line 56:
<comment>Test 'passes organization_id to createArtistInDb' title now references a function that is no longer the public API, and the assertion no longer verifies organizationId was forwarded. The old test explicitly checked the org UUID in the third arg; the new assertion only checks name and accountId, so it would pass even if organizationId were dropped. Either add organizationId to the objectContaining matcher or rename the test to reflect what it actually validates.</comment>
<file context>
@@ -45,18 +45,16 @@ describe("createArtistPostHandler", () => {
- "Test Artist",
- "api-key-account-id",
- undefined,
+ expect(mockResolveOrCreateArtist).toHaveBeenCalledWith(
+ expect.objectContaining({ name: "Test Artist", accountId: "api-key-account-id" }),
);
</file context>
Suggested change
| expect(mockResolveOrCreateArtist).toHaveBeenCalledWith( | |
| expect.objectContaining({ name: "Test Artist", accountId: "api-key-account-id" }), | |
| ); | |
| expect(mockResolveOrCreateArtist).toHaveBeenCalledWith( | |
| expect.objectContaining({ name: "Test Artist", accountId: "api-key-account-id", organizationId: "660e8400-e29b-41d4-a716-446655440001" }), | |
| ); |
Contributor
Author
Preview verification — all three contract paths live, SQL cross-checked
SQL ground truth (prod — api previews share the prod DB)That last number is the model working end to end: Delacey is rostered by this account and by the Docs ↔ API ↔ live reconciliationField-by-field match with the merged contract: Known and already flagged in the PR body: with existing duplicates (Del Water Gap, Ana Bárbara) the canonical picked is newest-first until database#49 collapses them and api#794 makes the pick oldest-first — new adds stop duplicating as of this PR regardless. Ready to merge. Next in order: this → chat#1901 (after a test-branch sync so chat previews see the new param). |
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.
Row 8 of the canonical-artist trilogy on chat#1889 — dedup at creation. Contract: docs#278. Merge order: docs#278 → this → chat#TBD.
Why
The chat add path (
POST /api/artistsname-only, then a clientPATCHfor the social) creates a new artist row per signup, so multiple rows exist per Spotify id and every canonical lookup downstream is ambiguous — the root cause of the roster doubles. api#791 deduped the valuation fallback (necessary, insufficient); api#792 tried account-scoped preference and was closed unmerged on the 2026-07-29 decision: canonical artists exist across all accounts; a canonical lookup should not need an accountId. Creation is the only place dedup can be enforced.What changed
validateCreateArtistBody: optionalspotify_artist_id(min 1), threaded asspotifyArtistId.resolveOrCreateArtist(SRP): canonical exists →insertAccountArtistId(skip if rostered) + return it,created: false; elsecreateArtistInDb+ attach theSPOTIFYsocial server-side (non-fatal, same contract as chat#1892's guard),created: true.createArtistPostHandler: 200 on reuse, 201 on create; response shape{ artist }unchanged.Verification
TDD red → green, three units (validator 2 new tests, resolver 5, handler rewritten + 2 new — all RED first: two files failed collection, validator failed assertions). Full api suite 786 files / 4262 tests pass;
tscat the 236 baseline; eslint clean.Preview verification to follow on this PR (200-reuse against a known canonical, 201-create for a fresh artist, cross-account link assertion via SQL).
Reviewer notes
artist_organization_idswhenorganization_idis passed alongside a resolvedspotify_artist_id. Flagging rather than silently deciding; happy to add if you want org parity.Tracked in chat#1889 (matrix row 8).
Summary by cubic
POST
/api/artistsnow resolves or creates byspotify_artist_idto prevent duplicate artist rows and keep one canonical per Spotify id. If a canonical exists it’s linked to the account (200); otherwise a new artist is created and enriched with the SPOTIFY social (best‑effort) (201).spotify_artist_id(must be non-empty).account_artist_idsand skip if already rostered.{ artist }.Written for commit 293d712. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes