feat(onboarding): add artists by Spotify typeahead, not free text (chat#1889 row 8) - #1892
Conversation
|
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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 38 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 (3)
📝 WalkthroughWalkthrough
ChangesRoster artist onboarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AddArtistForm
participant SpotifyArtistSearch
participant useAddRosterArtist
participant createRosterArtist
participant saveArtist
AddArtistForm->>SpotifyArtistSearch: register onSelect and isBusy
SpotifyArtistSearch->>AddArtistForm: return selected Spotify artist
AddArtistForm->>useAddRosterArtist: addArtist(name, profileUrl, image)
useAddRosterArtist->>createRosterArtist: create roster artist
createRosterArtist-->>useAddRosterArtist: return account_id
useAddRosterArtist->>saveArtist: persist social and avatar enrichment
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 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: a332e38fe7
ℹ️ 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 added = await addArtist(artist.name, { | ||
| profileUrl: artist.external_urls.spotify, | ||
| image: artist.images?.[0]?.url, | ||
| }); |
There was a problem hiding this comment.
For a cold-start account with no catalogs, this selection passes only the artist name, profile URL, and image, discarding artist.id; the hook then performs only the artist POST/PATCH. Unlike hooks/useAddSpotifyArtist.ts, it never calls runValuation(accessToken, artist.id), so /setup/catalog can still redirect these users to a catalogs page that says "No catalogs found" despite the new copy promising that adding the artist gets the catalog valued.
Useful? React with 👍 / 👎.
| await saveArtist(accessToken, artist.account_id, { | ||
| ...(payload ? { profileUrls: payload.profileUrls } : {}), | ||
| ...(options.image ? { image: options.image } : {}), | ||
| }); |
There was a problem hiding this comment.
When the initial artist POST succeeds but this PATCH fails (for example, during a transient API/network error), control reaches the outer catch, skips getArtists(), and returns false. The form therefore remains open and invites the user to select the artist again, issuing another POST and potentially creating duplicate roster entries even though the first artist already exists; isolate enrichment failure from creation and still refresh/close after a successful create.
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.
1 issue found across 4 files
Confidence score: 2/5
- In
components/Onboarding/AddArtistForm.tsx, adding the first artist to an empty roster does not trigger catalog valuation, so onboarding can advance without the valuation step users were promised; this is a concrete flow regression with clear user impact — update the add-artist handler to start valuation when the firstartist.idis selected and add a regression test for the empty-roster path.
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="components/Onboarding/AddArtistForm.tsx">
<violation number="1" location="components/Onboarding/AddArtistForm.tsx:24">
P1: Adding the first artist from an empty onboarding roster still never starts catalog valuation, so users can continue past this step without the catalog the new copy promises. This handler has the selected `artist.id`; route it through `useAddSpotifyArtist` (or trigger the same first-roster valuation flow here) rather than the name/profile-only hook.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| setIsOpen(false); | ||
| } | ||
| const handleSelect = async (artist: SpotifyArtistSearchResult) => { | ||
| const added = await addArtist(artist.name, { |
There was a problem hiding this comment.
P1: Adding the first artist from an empty onboarding roster still never starts catalog valuation, so users can continue past this step without the catalog the new copy promises. This handler has the selected artist.id; route it through useAddSpotifyArtist (or trigger the same first-roster valuation flow here) rather than the name/profile-only hook.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/AddArtistForm.tsx, line 24:
<comment>Adding the first artist from an empty onboarding roster still never starts catalog valuation, so users can continue past this step without the catalog the new copy promises. This handler has the selected `artist.id`; route it through `useAddSpotifyArtist` (or trigger the same first-roster valuation flow here) rather than the name/profile-only hook.</comment>
<file context>
@@ -1,24 +1,31 @@
- setIsOpen(false);
- }
+ const handleSelect = async (artist: SpotifyArtistSearchResult) => {
+ const added = await addArtist(artist.name, {
+ profileUrl: artist.external_urls.spotify,
+ image: artist.images?.[0]?.url,
</file context>
chat#1889 matrix row 8 — the highest-value item after convergence. AddArtistForm took a free-text name and useAddRosterArtist posted only that, so an added artist had no Spotify id. Without one there is no catalog and no valuation, which makes the payoff structurally unreachable — and that is the only forward path for the cold-start cohort: of the 9 accounts that received a welcome email 2026-07-23..25, 6 had zero artists and had never run a valuation. - AddArtistForm uses the existing SpotifyArtistSearch typeahead (same component verify-socials adopted in #1882), so a pick resolves to a real Spotify id. - useAddRosterArtist attaches the profile URL (via buildSocialFixPayload) and the Spotify avatar through PATCH /api/artists/{id} after creation, since POST /api/artists accepts only a name. Enrichment failure is non-fatal. - ConfirmRosterStep no longer tells an empty roster we set it up "from your valuation" — false for two thirds of signups. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a332e38 to
dae3741
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/Onboarding/AddArtistForm.tsx (1)
10-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim implementation-history comments.
These blocks repeat product history and internal chat references rather than documenting non-obvious behavior.
components/Onboarding/AddArtistForm.tsx#L10-L18: replace with a brief description of Spotify selection supplying roster metadata.components/Onboarding/ConfirmRosterStep.tsx#L24-L26: remove the comment; the conditional copy is self-explanatory.hooks/onboarding/useAddRosterArtist.ts#L19-L29: retain only the concise API contract and non-fatal enrichment rationale.As per coding guidelines, “Add short comments only when necessary to clarify non-obvious logic” and “Write minimal code - use only the absolute minimum code needed.”
🤖 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 `@components/Onboarding/AddArtistForm.tsx` around lines 10 - 18, Trim implementation-history comments across the onboarding roster flow: in components/Onboarding/AddArtistForm.tsx lines 10-18, replace the history and chat references with a brief description that Spotify selection supplies roster metadata; in components/Onboarding/ConfirmRosterStep.tsx lines 24-26, remove the self-explanatory conditional-copy comment; in hooks/onboarding/useAddRosterArtist.ts lines 19-29, retain only the concise API contract and rationale for non-fatal enrichment.Source: Coding guidelines
🤖 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 `@hooks/onboarding/useAddRosterArtist.ts`:
- Around line 54-62: Update the enrichment save block in useAddRosterArtist
around saveArtist so its rejection is handled locally rather than reaching the
outer artist-creation catch. Show an enrichment-specific error message, then
refresh the roster and return success after the artist has already been created;
preserve normal success behavior when enrichment succeeds and avoid treating the
artist creation as failed.
---
Nitpick comments:
In `@components/Onboarding/AddArtistForm.tsx`:
- Around line 10-18: Trim implementation-history comments across the onboarding
roster flow: in components/Onboarding/AddArtistForm.tsx lines 10-18, replace the
history and chat references with a brief description that Spotify selection
supplies roster metadata; in components/Onboarding/ConfirmRosterStep.tsx lines
24-26, remove the self-explanatory conditional-copy comment; in
hooks/onboarding/useAddRosterArtist.ts lines 19-29, retain only the concise API
contract and rationale for non-fatal enrichment.
🪄 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: 113c845a-1cb5-40e8-932d-e36c9478282e
⛔ Files ignored due to path filters (1)
components/Onboarding/__tests__/AddArtistForm.test.tsxis excluded by!**/*.test.*and included bycomponents/**
📒 Files selected for processing (3)
components/Onboarding/AddArtistForm.tsxcomponents/Onboarding/ConfirmRosterStep.tsxhooks/onboarding/useAddRosterArtist.ts
| const payload = options.profileUrl | ||
| ? buildSocialFixPayload(options.profileUrl) | ||
| : null; | ||
| if (artist?.account_id && (payload || options.image)) { | ||
| await saveArtist(accessToken, artist.account_id, { | ||
| ...(payload ? { profileUrls: payload.profileUrls } : {}), | ||
| ...(options.image ? { image: options.image } : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep Spotify enrichment failures non-fatal.
A saveArtist rejection reaches the outer catch, so a successfully created artist is reported as failed and the roster is not refreshed. The form remains open, and retrying can create a duplicate artist. Catch only the PATCH failure, show an enrichment-specific message, then refresh and return success.
Proposed fix
if (artist?.account_id && (payload || options.image)) {
- await saveArtist(accessToken, artist.account_id, {
- ...(payload ? { profileUrls: payload.profileUrls } : {}),
- ...(options.image ? { image: options.image } : {}),
- });
+ try {
+ await saveArtist(accessToken, artist.account_id, {
+ ...(payload ? { profileUrls: payload.profileUrls } : {}),
+ ...(options.image ? { image: options.image } : {}),
+ });
+ } catch {
+ toast.error("Artist added, but Spotify details could not be saved");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = options.profileUrl | |
| ? buildSocialFixPayload(options.profileUrl) | |
| : null; | |
| if (artist?.account_id && (payload || options.image)) { | |
| await saveArtist(accessToken, artist.account_id, { | |
| ...(payload ? { profileUrls: payload.profileUrls } : {}), | |
| ...(options.image ? { image: options.image } : {}), | |
| }); | |
| } | |
| const payload = options.profileUrl | |
| ? buildSocialFixPayload(options.profileUrl) | |
| : null; | |
| if (artist?.account_id && (payload || options.image)) { | |
| try { | |
| await saveArtist(accessToken, artist.account_id, { | |
| ...(payload ? { profileUrls: payload.profileUrls } : {}), | |
| ...(options.image ? { image: options.image } : {}), | |
| }); | |
| } catch { | |
| toast.error("Artist added, but Spotify details could not be saved"); | |
| } | |
| } |
🤖 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 `@hooks/onboarding/useAddRosterArtist.ts` around lines 54 - 62, Update the
enrichment save block in useAddRosterArtist around saveArtist so its rejection
is handled locally rather than reaching the outer artist-creation catch. Show an
enrichment-specific error message, then refresh the roster and return success
after the artist has already been created; preserve normal success behavior when
enrichment succeeds and avoid treating the artist creation as failed.
Preview verification — rebased onto
|
| # | Check | Expected | Actual | |
|---|---|---|---|---|
| 1 | Free-text Artist name input |
gone | no such node in the a11y tree | ✅ |
| 2 | Replacement control | Spotify typeahead | textbox "Search Spotify for an artist" + listbox "Spotify artist results" |
✅ |
| 3 | Add submit button |
gone, Cancel remains |
exactly that | ✅ |
| 4 | Empty-roster copy | new string | "Search Spotify for the artists you manage… pick the real profile." | ✅ |
| 5 | Empty-state copy | new string | "No artists on your roster yet. Add your first artist below to get your catalog valued." | ✅ |
| 6 | Typeahead results | real Spotify artists | 20 results w/ avatars + follower counts via GET /api/spotify/search |
✅ |
| 7 | Fields the PR reads | populated | external_urls.spotify = …/artist/43qxAkuKFB6fMNSeS5dO7Z, images[0].url = i.scdn.co/…f04af436, followers.total = 1,797,355 |
✅ |
| 8 | Create call | POST /api/artists |
201 | ✅ |
| 9 | Enrichment call | PATCH /api/artists/{id} |
200 | ✅ |
| 10 | PATCH payload | profileUrls + image | {"profileUrls":{"SPOTIFY":"https://open.spotify.com/artist/43qxAkuKFB6fMNSeS5dO7Z"},"image":"https://i.scdn.co/image/ab6761610000e5ebf04af436ecc9ce9b74d715eb"} |
✅ |
| 11 | Spotify id persisted | yes | account_socials[0].profile_url = open.spotify.com/artist/43qxAkuKFB6fMNSeS5dO7Z, type: SPOTIFY |
✅ |
| 12 | Roster row avatar | Spotify image | renders; artist.image set to the scdn URL |
✅ |
| 13 | Form collapses on success | setIsOpen(false) |
collapsed, roster refreshed to 1 artist | ✅ |
The headline acceptance criterion — "an artist added during setup carries a Spotify id" — is met. Verified at the data layer, not just visually: GET /api/artists returns the id 43qxAkuKFB6fMNSeS5dO7Z on the created artist.
Deliberate negative probes against the enrichment endpoint:
| Probe | Result |
|---|---|
PATCH /api/artists/{id} no auth |
401 Exactly one of x-api-key or Authorization must be provided |
PATCH /api/artists/not-a-uuid |
400 id must be a valid UUID |
PATCH /api/artists/{unowned-uuid} |
403 Forbidden |
Screenshots
Cold-start empty roster (authenticated) — both new strings
Typeahead with live Spotify results
After selection — avatar + "1 social profile matched"
Verify-socials step
🔴 One defect, in scope — enrichment failure is not actually non-fatal
The docstring on useAddRosterArtist claims:
Enrichment failing is non-fatal — the artist is already on the roster and its socials can still be fixed in the next step.
The code doesn't do that. await saveArtist(...) sits inside the same try as createRosterArtist, so a PATCH failure lands in the shared catch:
const artist = await createRosterArtist(accessToken, trimmed, selectedOrgId);
if (artist?.account_id && (payload || options.image)) {
await saveArtist(accessToken, artist.account_id, {...}); // throws ⇒ shared catch
}
await getArtists(); // ← never runs
return true; // ← never runsOn a PATCH failure the user gets toast.error(...), addArtist returns false, so setIsOpen(false) never fires and getArtists() never refreshes — but POST /api/artists already succeeded. Net effect: the artist exists server-side, the UI says "Failed to add artist" and keeps the form open, and the obvious user response (pick again) creates a duplicate.
This isn't hypothetical — the probes above show the PATCH has three live failure modes (401 on a stale token, 400, 403), plus ordinary network flake. The 403 path is worth weighing given the connector-authority hardening in #1860 / #1865.
Fix is small and keeps the docstring honest:
if (artist?.account_id && (payload || options.image)) {
try {
await saveArtist(accessToken, artist.account_id, {
...(payload ? { profileUrls: payload.profileUrls } : {}),
...(options.image ? { image: options.image } : {}),
});
} catch {
// Non-fatal by design: the artist is created and its socials are
// fixable in verify-socials. Swallow so the roster still refreshes.
}
}Worth a test pinning it — "PATCH rejects ⇒ addArtist still resolves true and the roster refreshes" — since the current suite doesn't cover the partial-failure path.
🟡 The copy fix only covers the zero-artist branch
ConfirmRosterStep was changed for artists.length === 0. But as soon as the cold-start user adds their first artist through this PR's own typeahead, the ternary falls through to the original string and they're told:
"We set this artist up from your valuation. Add anyone else you manage — you can verify their socials next."
on an account that has never run a valuation. (Visible in the third screenshot above — that's the same fresh account, one artist in.) The PR body lists "fixes the false 'from your valuation' copy" as a goal, and it's half-fixed: false at 0 artists is gone, false at ≥1 remains. A hasValuation / catalog-count condition rather than a roster-length one would settle it.
⚪ Pre-existing, not a regression — @artist · 0 followers (#1897)
Verify-socials renders Spotify @artist · 0 followers for an artist with 1.8M followers, with a fallback initial instead of the social avatar. The PATCH sends a correct absolute profile URL; the api derives username from a URL path segment and stores followerCount: 0, avatar: null.
This is the gap #1886 flagged and #1897 owns — this is the fourth independent sighting, and the first confirmed through the new typeahead path, which reinforces the note on #1886 that #1897's scope must cover the handle, not just pfp + follower count. Note the artist-level image is set correctly by this PR; it's only the social row that's bare.
⚪ Scope note — the gate is removed, the payoff is not yet reached
Completing the flow leaves the account at "Roster verified" with GET /api/accounts/{id}/catalogs → {"catalogs": []} and /catalogs rendering "No catalogs found." So the chain is now unblocked (the Spotify id exists, which was the hard blocker) but nothing in the setup flow creates the catalog or shows a valuation — the flow is still "Step 1 of 2" and ends on the generic green check.
That's chat#1895 (row 7, valuation reward) and row 8 (persist est_catalog_value), not this PR. Recording it so row 6 isn't mistaken for delivering the payoff on its own.
Not verified
- The enrichment-failure path was probed at the endpoint level but not forced through the UI, so the 🔴 above is read from the code plus live failure modes rather than observed in-browser.
- No bot review exists on this PR: CodeRabbit hit the org review limit ("next review available in 37 minutes") at 04:48 on 07-27 and never re-ran; Bugbot is disabled on the account.
Recommendation
Core change works and is worth landing. I'd fix the 🔴 before merge (few lines + one test) and decide on the 🟡 — either widen the condition here or split it out. Verified on dae37413.
…tion that never ran
Two defects found verifying chat#1892 against its preview.
1. saveArtist sat inside the same try as createRosterArtist, so a failing
PATCH /api/artists/{id} took the shared catch: error toast, addArtist
returned false, the form stayed open and the roster never refreshed --
over an artist POST /api/artists had already created. Picking again
made a duplicate. The docstring already promised this was non-fatal;
now it is.
2. The copy fix only covered the empty roster. A cold-start account was
told 'We set this artist up from your valuation' the moment it added
its first artist through the new typeahead. getConfirmRosterCopy keys
the claim on a catalog existing rather than on roster length.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 `@components/Onboarding/ConfirmRosterStep.tsx`:
- Around line 18-20: Update the catalog state handling near useCatalogs and
hasValuation so data === undefined remains an unresolved state rather than being
treated as no valuation. Defer the valuation-specific copy until catalogsData is
known, while preserving the existing no-valuation behavior for a resolved empty
catalogs list.
🪄 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: a841ba04-1f76-45cf-ba87-b8bcaa0ec578
⛔ Files ignored due to path filters (2)
hooks/onboarding/__tests__/useAddRosterArtist.test.tsxis excluded by!**/*.test.*and included byhooks/**lib/onboarding/__tests__/getConfirmRosterCopy.test.tsis excluded by!**/*.test.*and included bylib/**
📒 Files selected for processing (3)
components/Onboarding/ConfirmRosterStep.tsxhooks/onboarding/useAddRosterArtist.tslib/onboarding/getConfirmRosterCopy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/onboarding/useAddRosterArtist.ts
| const { data: catalogsData } = useCatalogs(); | ||
| const artists = sorted.filter((artist) => !artist.isWorkspace); | ||
| const hasValuation = (catalogsData?.catalogs?.length ?? 0) > 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'ConfirmRosterStep\.tsx|.*catalogs.*|useCatalogs.*' . | sed 's#^\./##' | head -100
echo
echo "== ConfirmRosterStep outline =="
if [ -f components/Onboarding/ConfirmRosterStep.tsx ]; then
wc -l components/Onboarding/ConfirmRosterStep.tsx
ast-grep outline components/Onboarding/ConfirmRosterStep.tsx || true
echo
cat -n components/Onboarding/ConfirmRosterStep.tsx | sed -n '1,260p'
fi
echo
echo "== useCatalogs definitions/usages =="
rg -n "useCatalogs|type .*Catalog|interface .*Catalog|catalogsData|hasValuation" -S .Repository: recoupable/chat
Length of output: 10705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useCatalogs.ts =="
cat -n hooks/useCatalogs.ts | sed -n '1,120p'
echo
echo "== ConfirmRosterCopy =="
cat -n lib/onboarding/getConfirmRosterCopy.ts | sed -n '1,120p'
echo
echo "== Query behavior from source =="
rg -n "UseQueryResult|UseSuspense|staleTime|initialData|suspend|error\\b|isLoading\\b|isFetched\\b|refetchOnMount" -S hooks/lib components | head -200Repository: recoupable/chat
Length of output: 21063
Preserve the unresolved catalog state.
useCatalogs can render with data === undefined before the catalog query resolves, and hasValuation then treats that as false. A populated roster can briefly show “Add anyone else you manage” instead of “We set [subject] up from your valuation.” Distinguish loading from “no valuation” and defer this copy until catalog state is known.
🤖 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 `@components/Onboarding/ConfirmRosterStep.tsx` around lines 18 - 20, Update the
catalog state handling near useCatalogs and hasValuation so data === undefined
remains an unresolved state rather than being treated as no valuation. Defer the
valuation-specific copy until catalogsData is known, while preserving the
existing no-valuation behavior for a resolved empty catalogs list.
Both findings fixed —
|
| Test | Asserts |
|---|---|
| enriches a created artist | saveArtist called with {profileUrls: {SPOTIFY: …}, image: …}, roster refreshed |
| treats a failed enrichment as non-fatal | saveArtist rejects ⇒ addArtist still resolves true, getArtists() still runs, no error toast |
| still reports failure when the artist itself can't be created | createRosterArtist rejects ⇒ resolves false, toast fires, getArtists() not called |
The middle one is the regression guard. RED output before the fix, which is the defect reproduced:
FAIL hooks/onboarding/__tests__/useAddRosterArtist.test.tsx > treats a failed enrichment as non-fatal
AssertionError: expected false to be true
- Expected: true
+ Received: false
The third test matters as much as the second — it proves the new catch didn't swallow real create failures along with enrichment ones.
🟡 The valuation credit is now keyed on a valuation, not on roster length
Copy logic extracted to a pure lib/onboarding/getConfirmRosterCopy.ts and keyed on hasValuation (derived from useCatalogs() — a catalog is the artifact a valuation leaves behind) rather than artists.length:
| Roster | Valuation ran | Copy |
|---|---|---|
| 0 artists | — | "Search Spotify for the artists you manage…" |
| ≥1 | yes | "We set {this artist/these artists} up from your valuation. Add anyone else you manage — you can verify their socials next." |
| ≥1 | no | "Add anyone else you manage — you can verify their socials next." |
Pulling it out of the JSX means the three branches are unit-testable without rendering, matching how getOnboardingStepTitle / getSetupPathForStep already live in lib/onboarding/. The test asserts the negative directly — for rosters of 1, 2 and 5 with no valuation, the string must not match /valuation/i.
Verified on preview chat-hpp1arx60-recoup.vercel.app (built from fa7a630e)
Re-checked on the same account from the first pass — sweetman+july2820260924@recoupable.com, now sitting at 1 artist / 0 catalogs, which is precisely the state that produced the false claim. Same URL, same account, before and after:
| Copy rendered | |
|---|---|
dae37413 (before) |
"We set this artist up from your valuation. Add anyone else you manage — you can verify their socials next." |
fa7a630e (after) |
"Add anyone else you manage — you can verify their socials next." |
Suite
pnpm exec vitest run components/Onboarding lib/onboarding hooks/onboarding
Test Files 21 passed (21)
Tests 79 passed (79)
Up from 73/19 at the start of this pass. pnpm exec tsc --noEmit → 13 errors, identical to main's 13; pnpm exec eslint clean on all five touched files.
Still not verified
- The
hasValuation: truebranch is unit-tested but not browser-confirmed — this account has no catalog, and creating one to prove a copy string wasn't worth the side effects. The funnel-signup path renders it today onmain, unchanged by this PR. - The
@artist · 0 followersgap is untouched here and, per the audit below, has no PR fixing it — chat#1897 explicitly scopes it out ("needs an api resolve path that does not exist yet"), and there is no api PR. It's being given its own row in chat#1889 rather than staying a parenthetical inside row 17.
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Em dashes read as machine-written and cost trust, so the roster and socials steps use periods or commas instead. A unit test pins it for getConfirmRosterCopy so it cannot creep back. The /setup/tasks strings (FirstTaskStep, buildFirstTaskParams) carry the same problem but are left alone here: chat#1898 is editing FirstTaskStep and would conflict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Copy pass + in-browser verification of both fixes —
|
| Where | Before | After |
|---|---|---|
getConfirmRosterCopy tail |
"Add anyone else you manage — you can verify their socials next." | "Add anyone else you manage**,** you can verify their socials next." |
ConfirmRosterStep CTA |
"This is my artist — continue" | "This is my artist**,** continue" |
VerifySocialsStep sub-head |
"…point at the wrong account — reports and tasks pull from them." | "…point at the wrong account**.** Reports and tasks pull from them." |
VerifySocialsStep CTA |
"Looks good — continue" | "Looks good**,** continue" |
A unit test now pins it, so it can't creep back:
it("uses no em or en dashes in any branch", () => {
for (const hasValuation of [true, false])
for (const artistCount of [0, 1, 2, 5])
expect(getConfirmRosterCopy({ artistCount, hasValuation })).not.toMatch(/[—–]/);
});Asserted live rather than by eye — scanning the rendered innerText of both steps for /[—–]/ returns zero matches on each.
Roster step — the copy line, and the singular CTA:
Socials step — sub-head and CTA:
🔴 The non-fatal enrichment fix, now verified in-browser
The previous comment listed this as read-from-code rather than observed, since the failure couldn't be triggered through the UI. Closing that gap: window.fetch was patched in the page to force exactly the 403 the live endpoint returns for an unowned id on any PATCH /api/artists/*, then an artist was added through the typeahead.
Forced failure confirmed intercepted:
PATCH https://test-recoup-api.vercel.app/api/artists/c2425574-7306-4f4b-b1b9-cce8ea20d6a9 → 403 Forbidden (injected)
| Assertion | Result |
|---|---|
Artist appears on the roster (getArtists() ran) |
✅ "Selena" rendered |
Add form collapsed (addArtist resolved true) |
✅ formStillOpen: false |
| No error toast | ✅ no "Failed to add artist" anywhere in the DOM |
| Degrades honestly | ✅ row reads "No socials matched yet" — fixable in the next step, exactly as the docstring promises |
| Plural CTA branch | ✅ "These are my artists, continue" |
On dae37413 this same sequence produced "Failed to add artist", left the form open, and left Selena off the roster despite POST /api/artists having already created her — the duplicate trap. It now degrades to an un-enriched roster row, which is the documented contract.
Suite
pnpm exec vitest run components/Onboarding lib/onboarding hooks/onboarding
Test Files 21 passed (21)
Tests 80 passed (80)
pnpm exec tsc --noEmit → 13, unchanged from main. eslint clean on all touched files.
Deliberately left alone
FirstTaskStep.tsx L45 and buildFirstTaskParams.ts L26 ("Weekly Catalog Report — {artist}") carry the same em dash. Not fixed here because chat#1898 is editing FirstTaskStep.tsx and this would conflict with it. Confirmed safe to change on its own terms — findExistingWeeklyReportTask matches on task.enabled, not on the title — so it's a clean pickup for #1898 or a follow-up. Everything else matching — under components/Onboarding/ is code comments, not product text.
1 participant
Matrix row 8 in chat#1889 — the highest-value item after the convergence chain. Independent of #1890/#1891 (different files), so it can be reviewed in parallel.
Why: the payoff was structurally unreachable for most signups
Live cohort from
email_send_log, 2026-07-23 → 2026-07-26:The welcome email fires on account creation regardless of whether a valuation preceded it, so two-thirds of the cohort are cold-start signups. Their only forward path is
AddArtistForm— which took a free-text name and posted only{ name }. No Spotify id → no catalog → no valuation. The number those users were emailed about could never exist.What changed
AddArtistFormusesSpotifyArtistSearch— the same typeahead verify-socials adopted in feat(onboarding): Spotify typeahead in verify-socials (item 3b of #1881) #1882 — so a pick resolves to a real Spotify id, profile URL, and avatar.useAddRosterArtistnow takes{ profileUrl, image }and attaches them via the existingPATCH /api/artists/{id}path after creation, becausePOST /api/artistsaccepts onlyname/account_id/organization_id. ReusesbuildSocialFixPayloadfor the platform detection rather than hand-rolling the shape. Enrichment failure is deliberately non-fatal — the artist is already on the roster and socials can still be fixed in the next step.ConfirmRosterStepcopy: an empty roster was told "We set these artists up from your valuation" — false for 6 of 9 signups. It now reads as an instruction to search, and the empty state names the reward.No api change needed, so no docs/contract change either — this rides two existing endpoints.
Verification
TDD, red → green:
```
RED — free-text field still present, no typeahead
Test Files 1 failed (1)
Tests 2 failed (2)
GREEN
pnpm exec vitest run components/Onboarding lib/onboarding
Test Files 18 passed (18)
Tests 70 passed (70)
```
The new test asserts the free-text
Artist namefield is gone, the Spotify searchbox is present, and picking a result callsaddArtistwith the resolved profile URL + avatar.pnpm exec tsc --noEmitclean for the touched files. Preview click-through pending (needs an authed empty-roster account).Tracked in chat#1889 (matrix row 8).
Summary by cubic
Replace free-text artist input with a Spotify typeahead in onboarding so added artists have a real Spotify profile and avatar. This unblocks cold-start signups so they can reach catalog and valuation (chat#1889, row 8).
New Features
AddArtistFormnow usesSpotifyArtistSearch; selecting a result callsaddArtist(name, { profileUrl, image }).useAddRosterArtistattaches the Spotify profile and image viaPATCH /api/artists/{id}usingbuildSocialFixPayload, then refreshes the roster.ConfirmRosterStepcopy prompts Spotify search when empty and only credits a valuation if the account actually has a catalog.Bug Fixes
saveArtisterrors are caught so the form closes and the roster refreshes, avoiding duplicates.getConfirmRosterCopytests.Written for commit 617d7f9. Summary will update on new commits.
Summary by CodeRabbit