feat(onboarding): end setup on the catalog valuation (chat#1889 row 10) - #1895
Conversation
|
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. |
📝 WalkthroughWalkthroughThe ChangesValuation onboarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SetupValuationPage
participant SetupValuation
participant Catalogs
participant HomeValuation
participant Router
SetupValuationPage->>SetupValuation: Render component
SetupValuation->>HomeValuation: Read valuation state
SetupValuation->>Catalogs: Read catalog state
Catalogs-->>SetupValuation: Pending, available, or error
SetupValuation->>Router: Redirect to /catalogs when unavailable
SetupValuation-->>SetupValuationPage: Render skeletons or valuation hero
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: 3047b82c60
ℹ️ 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".
| useEffect(() => { | ||
| if (isLoading) return; | ||
| if (!hasCatalog || isError) router.replace("/catalogs"); |
There was a problem hiding this comment.
On a direct visit from the welcome email, useCatalogs() is initially disabled because UserProvider has not asynchronously populated userData.account_id yet. In React Query v5 that disabled query has isLoading === false, so this effect sees hasCatalog === false and immediately replaces the new valuation route with /catalogs, including for accounts that do have a catalog. Gate the redirect on the catalog query actually reaching a terminal fetched state (and on account/auth readiness) rather than isLoading alone.
Useful? React with 👍 / 👎.
| if (!hasCatalog || isError) router.replace("/catalogs"); | ||
| }, [isLoading, hasCatalog, isError, router]); | ||
|
|
||
| if (isLoading || !valuation.show) { |
There was a problem hiding this comment.
When the catalog query succeeds but useHomeValuation() cannot produce a hero—for example, the catalog has no measured songs, the measurements request fails, or the selected artist has no scoped measurements—hasCatalog prevents the redirect while this branch renders skeletons forever. These are terminal show: false states in getValuationHeroState, not loading states, so affected users never get either the valuation or an actionable fallback.
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.
3 issues found across 4 files
Confidence score: 3/5
- In
components/Onboarding/SetupValuation.tsx, the same skeleton is used for both “still loading” and “valuation unavailable,” so accounts withvaluation.showpermanently false can appear stuck in loading and never get a clear next step—split these states and render an explicit unavailable/fallback view once data has settled. - In
components/Onboarding/RosterVerifiedPanel.tsx, users with an existing catalog can briefly see contradictory CTA text while valuation queries are still in flight, which risks confusing onboarding status—gate the verified/claim messaging behind a loading state until catalog/measurement lookup resolves. components/Onboarding/SetupValuation.tsxalso carries redirect lifecycle and multiple UI states in one oversized function, increasing the chance of state regressions as behavior changes—extract query/redirect logic or hero rendering into focused units to reduce follow-on risk.
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/SetupValuation.tsx">
<violation number="1" location="components/Onboarding/SetupValuation.tsx:23">
P3: This component exceeds the repository's 20-line function limit and mixes redirect lifecycle with two presentation states. Extract the redirect/query-state handling or the hero content into focused components/hooks to keep the onboarding UI maintainable.</violation>
<violation number="2" location="components/Onboarding/SetupValuation.tsx:34">
P1: The loading skeleton is shown for both "catalogs still loading" and "valuation not available" states without distinguishing between them. When the account has a catalog but `valuation.show` is permanently false (measurements API error, missing valuation data, or zero measured tracks), the user sees an infinite skeleton with no fallback or way to proceed.
The `useHomeValuation` hook can return `{ show: false }` permanently even when `hasCatalog` is true (e.g., `measurementsFailed`, `!measurements?.valuation`, `!measurements.measured_song_count`). At that point, the useEffect won't redirect (because `hasCatalog` is true and `isError` from `useCatalogs()` is false), and the skeleton renders forever.
`RosterVerifiedPanel` handles this exact scenario gracefully by falling back to the confirmation state with a pointer to `/setup/catalog`. `SetupValuation` should do the same — either show a fallback UI or redirect to a safe destination when the valuation data is genuinely unavailable despite having a catalog.</violation>
</file>
<file name="components/Onboarding/RosterVerifiedPanel.tsx">
<violation number="1" location="components/Onboarding/RosterVerifiedPanel.tsx:23">
P3: Completing socials can briefly show “Roster verified” and “Claim your catalog” for accounts that already have a catalog while valuation queries load. Render a loading state until the catalog/measurement lookup settles, then show this fallback only for a settled no-valuation result.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!hasCatalog || isError) router.replace("/catalogs"); | ||
| }, [isLoading, hasCatalog, isError, router]); | ||
|
|
||
| if (isLoading || !valuation.show) { |
There was a problem hiding this comment.
P1: The loading skeleton is shown for both "catalogs still loading" and "valuation not available" states without distinguishing between them. When the account has a catalog but valuation.show is permanently false (measurements API error, missing valuation data, or zero measured tracks), the user sees an infinite skeleton with no fallback or way to proceed.
The useHomeValuation hook can return { show: false } permanently even when hasCatalog is true (e.g., measurementsFailed, !measurements?.valuation, !measurements.measured_song_count). At that point, the useEffect won't redirect (because hasCatalog is true and isError from useCatalogs() is false), and the skeleton renders forever.
RosterVerifiedPanel handles this exact scenario gracefully by falling back to the confirmation state with a pointer to /setup/catalog. SetupValuation should do the same — either show a fallback UI or redirect to a safe destination when the valuation data is genuinely unavailable despite having a catalog.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupValuation.tsx, line 34:
<comment>The loading skeleton is shown for both "catalogs still loading" and "valuation not available" states without distinguishing between them. When the account has a catalog but `valuation.show` is permanently false (measurements API error, missing valuation data, or zero measured tracks), the user sees an infinite skeleton with no fallback or way to proceed.
The `useHomeValuation` hook can return `{ show: false }` permanently even when `hasCatalog` is true (e.g., `measurementsFailed`, `!measurements?.valuation`, `!measurements.measured_song_count`). At that point, the useEffect won't redirect (because `hasCatalog` is true and `isError` from `useCatalogs()` is false), and the skeleton renders forever.
`RosterVerifiedPanel` handles this exact scenario gracefully by falling back to the confirmation state with a pointer to `/setup/catalog`. `SetupValuation` should do the same — either show a fallback UI or redirect to a safe destination when the valuation data is genuinely unavailable despite having a catalog.</comment>
<file context>
@@ -0,0 +1,65 @@
+ if (!hasCatalog || isError) router.replace("/catalogs");
+ }, [isLoading, hasCatalog, isError, router]);
+
+ if (isLoading || !valuation.show) {
+ return (
+ <div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8">
</file context>
| * cold-start signup still lands somewhere actionable rather than on an empty | ||
| * hero. | ||
| */ | ||
| const SetupValuation = () => { |
There was a problem hiding this comment.
P3: This component exceeds the repository's 20-line function limit and mixes redirect lifecycle with two presentation states. Extract the redirect/query-state handling or the hero content into focused components/hooks to keep the onboarding UI maintainable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupValuation.tsx, line 23:
<comment>This component exceeds the repository's 20-line function limit and mixes redirect lifecycle with two presentation states. Extract the redirect/query-state handling or the hero content into focused components/hooks to keep the onboarding UI maintainable.</comment>
<file context>
@@ -0,0 +1,65 @@
+ * cold-start signup still lands somewhere actionable rather than on an empty
+ * hero.
+ */
+const SetupValuation = () => {
+ const router = useRouter();
+ const valuation = useHomeValuation();
</file context>
| const RosterVerifiedPanel = () => { | ||
| const valuation = useHomeValuation(); | ||
|
|
||
| if (valuation.show) { |
There was a problem hiding this comment.
P3: Completing socials can briefly show “Roster verified” and “Claim your catalog” for accounts that already have a catalog while valuation queries load. Render a loading state until the catalog/measurement lookup settles, then show this fallback only for a settled no-valuation result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/RosterVerifiedPanel.tsx, line 23:
<comment>Completing socials can briefly show “Roster verified” and “Claim your catalog” for accounts that already have a catalog while valuation queries load. Render a loading state until the catalog/measurement lookup settles, then show this fallback only for a settled no-valuation result.</comment>
<file context>
@@ -3,25 +3,66 @@
+const RosterVerifiedPanel = () => {
+ const valuation = useHomeValuation();
+
+ if (valuation.show) {
+ return (
+ <section className="flex flex-col items-center gap-4 py-8">
</file context>
chat#1889 matrix row 10 — the payoff this issue is named for. Finishing roster + socials ended on a generic green CheckCircle2, and /setup/valuation was a bare redirect to /catalogs, so the welcome email's "See your baseline valuation" link had no real destination. A signup converted on a number and never saw it again. - RosterVerifiedPanel renders the account's valuation with the shared homepage hero (useHomeValuation + ValuationHero), then points at the weekly report. - New SetupValuation mounts at /setup/valuation with the same hero, falling back to /catalogs only when there is no catalog to value. - The no-valuation fallback now points at the step that unlocks one (/setup/catalog) instead of dead-ending on "Back to Recoup". Reuses the existing hero rather than building a second valuation surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3047b82 to
402e476
Compare
Preview verification — rebased onto
|
| # | Check | Result | |
|---|---|---|---|
| 1 | Cold-start fallback replaces the generic green check | "Your artists and their socials are confirmed. Claim your catalog to see what it is worth." + Claim your catalog → /setup/catalog |
✅ |
| 2 | Valuation reward renders on roster-verified | ValuationHero with $389K, range $267K to $547K · 79 tracks measured, artist avatar + name |
✅ |
| 3 | Reward CTA | "Set up your weekly report" → /setup/tasks |
✅ |
| 4 | /setup/valuation with no catalog falls back |
redirects to /catalogs |
✅ |
| 5 | Copy is em-dash free | zero /[—–]/ in rendered text; the hero range reads "$267K to $547K" |
✅ |
Check 2 was driven by intercepting /catalogs + the measurements endpoint to return a valid catalog and band (this account has neither), so the hero path is proven to render — the numbers below are injected, not real:
Cold-start fallback, unmodified:
Side benefit confirmed: with a catalog present the roster copy correctly flips to "We set these artists up from your valuation", so #1892's hasValuation keying works against a real catalog rather than only in unit tests.
🔴 Blocker — /setup/valuation redirects to /catalogs on every cold load, even with a catalog
This is the route the welcome email's "See your baseline valuation" link points at, and the reason the PR exists. It cannot render the hero when reached that way.
SetupValuation gates its redirect on isLoading from useCatalogs:
useEffect(() => {
if (isLoading) return;
if (!hasCatalog || isError) router.replace("/catalogs");
}, [isLoading, hasCatalog, isError, router]);But useCatalogs is enabled: !!accountId && authenticated, and in TanStack Query v5 (^5.66.0) a disabled query is isPending: true / isFetching: false, so isLoading is false. While Privy is still resolving, the guard falls through with data === undefined, hasCatalog === false, and the redirect fires.
Instrumented on the preview — patched History.replaceState and fetch to timestamp both:
init @0ms
replaceState -> /setup/valuation @129ms
replaceState -> /catalogs @484ms ← redirect fires
catalogs-fetch-START @1312ms ← request starts 828ms LATER
catalogs-fetch-END status=200 @1851ms
The redirect happens 828ms before the catalogs request is even issued. I also re-ran it with /catalogs intercepted to return a populated catalog: still redirected, because the decision is made before any fetch.
Fix: gate on isPending rather than isLoading (or on status !== "success", or hold until authenticated && accountId). One line, but it's the difference between the feature working and never rendering from its primary entry point.
Second, related: once that's fixed, hasCatalog === true with valuation.show === false renders <Skeleton> forever — the redirect can't fire (there is a catalog) and the hero won't render. That's reachable for a freshly claimed catalog whose measurements haven't materialised, or when getValuationHeroState hides the hero because the artist-scope echo doesn't match. Worth a terminal state rather than an indefinite skeleton.
🟡 "Claim your catalog" moves the dead end one hop
The new cold-start CTA is the right idea, but its destination is empty. /setup/catalog is redirect("/catalogs"), and /catalogs for an account with no catalog renders "No catalogs found." with no create affordance — enumerating every button/a on the page returns only nav chrome (Recoupable, New Chat, Agents, Tasks, Files, account menu). There is nothing to click to claim anything.
So the flow still terminates without a payoff for the cohort it targets; it just terminates one screen later, with a promise attached. Not necessarily this PR's job to build the claim step — but as shipped, the CTA makes a promise the next screen doesn't keep. Options: point it somewhere that can actually create a catalog, or soften the copy until that step exists.
⚪ /setup/catalog repeats the redirect() stub pattern #1887 retired
GET /setup/catalog → 200 (no Location)
GET /setup/valuation → 200 (real page, expected)
GET /onboarding/roster → 308 → /setup/artists (config redirect from #1887)
/setup/catalog answers 200, not a 3xx — Next prerenders a page whose body is only redirect(), so the hop ships in the RSC payload and fires on hydration. That is verbatim the finding that made #1887 move to next.config.mjs redirects, recorded as a decision on chat#1889 (2026-07-27). It matters here because the welcome email links to /setup/catalog as step 3, so a crawler or link previewer gets a 200 and an empty shell. Pre-existing, not introduced by this PR, but this PR makes it the destination of a primary CTA. A next.config.mjs rule would settle it consistently with #1887.
Not verified
- The hero was rendered from injected catalog + measurement data; no live valuation exists on any account I can sign into. The real numbers path is unchanged from the homepage hero this reuses, but it has not been exercised end-to-end here.
isErroron the catalogs query was not exercised separately from!hasCatalog.
Recommendation
The reward itself is good and the fallback is the right shape. I'd fix the 🔴 before merge — it's one line and without it the email's payoff link never reaches the feature — and make a call on the 🟡, since a CTA that lands on "No catalogs found" reads worse than the green check it replaced. Verified on 402e4769.
useCatalogs is enabled: !!accountId && authenticated, and a disabled TanStack Query v5 query reports isPending true / isFetching false, so isLoading is false while Privy is still resolving. The redirect effect fired on that, bouncing every cold load of /setup/valuation to /catalogs before the catalogs request was even issued (measured on preview: redirect at 484ms, fetch start at 1312ms) -- including for accounts that do have a catalog. That route is where the welcome email's payoff link points, so the hero could never render from its primary entry point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/setup/valuation/page.tsx (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim duplicated implementation-history comments.
Keep only concise comments that explain local invariants; remove PR history, chat references, and repeated redirect rationale.
app/setup/valuation/page.tsx#L3-L9: reduce to a one-line route intent or remove.components/Onboarding/SetupValuation.tsx#L13-L22: retain only the catalog-availability redirect invariant.components/Onboarding/RosterVerifiedPanel.tsx#L10-L19: retain only the valuation/fallback UI invariant.As per coding guidelines, “Write minimal code” and “Add short comments only when necessary to clarify non-obvious logic.”
🤖 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 `@app/setup/valuation/page.tsx` around lines 3 - 9, Trim the implementation-history comments at app/setup/valuation/page.tsx#L3-L9 to a concise one-line route-intent comment or remove them. At components/Onboarding/SetupValuation.tsx#L13-L22, retain only the catalog-availability redirect invariant; at components/Onboarding/RosterVerifiedPanel.tsx#L10-L19, retain only the valuation/fallback UI invariant. Remove PR, chat, and repeated redirect rationale while preserving behavior.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 `@components/Onboarding/RosterVerifiedPanel.tsx`:
- Around line 58-63: Update the “Claim your catalog” Link in RosterVerifiedPanel
to target the setup flow that supports both creating and claiming a catalog,
rather than /setup/catalog’s catalog-less redirect. Preserve the existing CTA
styling and label while ensuring the no-valuation recovery path reaches an
actionable catalog creation or claim screen.
In `@components/Onboarding/SetupValuation.tsx`:
- Around line 39-45: Update the loading guard in SetupValuation so the skeleton
renders only while isPending is true; when valuation.show is false after loading
resolves, fall through to the existing no-valuation confirmation/CTA instead of
returning the skeleton.
- Around line 34-37: Update the useEffect redirect logic in SetupValuation so
catalog-query failures indicated by isError do not trigger router.replace.
Redirect only when the query has completed successfully and hasCatalog is false,
while rendering a recoverable error/retry state for failures using the
component’s existing query and UI patterns.
---
Nitpick comments:
In `@app/setup/valuation/page.tsx`:
- Around line 3-9: Trim the implementation-history comments at
app/setup/valuation/page.tsx#L3-L9 to a concise one-line route-intent comment or
remove them. At components/Onboarding/SetupValuation.tsx#L13-L22, retain only
the catalog-availability redirect invariant; at
components/Onboarding/RosterVerifiedPanel.tsx#L10-L19, retain only the
valuation/fallback UI invariant. Remove PR, chat, and repeated redirect
rationale while preserving behavior.
🪄 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: 9a2e6f29-fa8c-4454-a6fc-c4a59a0aa181
⛔ Files ignored due to path filters (2)
components/Onboarding/__tests__/RosterVerifiedPanel.test.tsxis excluded by!**/*.test.*and included bycomponents/**components/Onboarding/__tests__/SetupValuation.test.tsxis excluded by!**/*.test.*and included bycomponents/**
📒 Files selected for processing (3)
app/setup/valuation/page.tsxcomponents/Onboarding/RosterVerifiedPanel.tsxcomponents/Onboarding/SetupValuation.tsx
| <Link | ||
| href="/setup/catalog" | ||
| className={cn(buttonVariants(), "min-w-[200px]")} | ||
| > | ||
| Claim your catalog | ||
| </Link> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Point “Claim your catalog” to a flow that can create or claim one.
This PR makes /setup/catalog the fallback CTA, but it redirects catalog-less users to /catalogs, which has no catalog-creation action. The no-valuation recovery path is therefore blocked.
🤖 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/RosterVerifiedPanel.tsx` around lines 58 - 63, Update
the “Claim your catalog” Link in RosterVerifiedPanel to target the setup flow
that supports both creating and claiming a catalog, rather than /setup/catalog’s
catalog-less redirect. Preserve the existing CTA styling and label while
ensuring the no-valuation recovery path reaches an actionable catalog creation
or claim screen.
| useEffect(() => { | ||
| if (isPending) return; | ||
| if (!hasCatalog || isError) router.replace("/catalogs"); | ||
| }, [isPending, hasCatalog, isError, router]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not redirect on catalog-query failures.
isError means catalog availability is unknown, not that no catalog exists. Redirect only after a successful empty response; render a recoverable error/retry state for failures.
🤖 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/SetupValuation.tsx` around lines 34 - 37, Update the
useEffect redirect logic in SetupValuation so catalog-query failures indicated
by isError do not trigger router.replace. Redirect only when the query has
completed successfully and hasCatalog is false, while rendering a recoverable
error/retry state for failures using the component’s existing query and UI
patterns.
| if (isPending || !valuation.show) { | ||
| return ( | ||
| <div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8"> | ||
| <Skeleton className="h-8 w-1/2 rounded-lg" /> | ||
| <Skeleton className="h-[168px] w-full rounded-xl" /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle the resolved no-valuation state.
Once catalogs have resolved, an account with a catalog but valuation.show === false stays on this skeleton forever. Render the no-valuation confirmation/CTA instead of treating it as loading.
🤖 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/SetupValuation.tsx` around lines 39 - 45, Update the
loading guard in SetupValuation so the skeleton renders only while isPending is
true; when valuation.show is false after loading resolves, fall through to the
existing no-valuation confirmation/CTA instead of returning the skeleton.
🔴 Blocker fixed —
|
| Test | Asserts |
|---|---|
| does not redirect while the catalogs query has not resolved | isPending: true, isLoading: false ⇒ router.replace not called |
| redirects once settled with no catalog | isPending: false, catalogs: [] ⇒ replace("/catalogs") |
| redirects when the catalogs read fails | isError: true ⇒ replace("/catalogs") |
| renders the hero when a valuation is available | hero rendered, no redirect |
The last three guard the fix from over-correcting into "never redirects".
Verified on preview chat-akkmnwyyn-recoup.vercel.app (built from 61911a6a)
Same instrumentation as the original report — History.replaceState and fetch timestamped on a cold load.
With a catalog (intercepted, since no account I can reach has one) — previously redirected 100% of the time:
init @0ms
replaceState -> /setup/valuation @1336ms
catalogs-fetch @5768ms
measurements-fetch @5777ms
measurements-fetch @7086ms
No replaceState -> /catalogs at all. Final URL stays /setup/valuation, and the hero renders with its route-specific copy:
Without a catalog (this account's real state) — the fallback still works, and now happens in the right order:
init @0ms
replaceState -> /setup/valuation @4257ms
catalogs-fetch-START @10138ms
catalogs-fetch-END 200 @10776ms
replaceState -> /catalogs @16476ms ← after the read resolves
Compare with 402e4769, where the redirect fired at 484ms and the fetch didn't start until 1312ms. The decision is now made on data instead of on an unresolved query.
Full suite: 86 passed / 23 files. tsc --noEmit → 13, unchanged from main. eslint clean.
Still open on this PR
The 🟡 "Claim your catalog" dead end is unchanged — see the recommendation in the next comment. The ⚪ /setup/catalog returning 200 instead of a 3xx is also unchanged and is pre-existing.
The secondary skeleton risk I flagged (hasCatalog === true + valuation.show === false ⇒ indefinite skeleton) is still present — isPending fixes the auth race but not this case, which is reachable while a freshly created catalog is still being measured. Not a blocker, but worth a terminal state.
Recommendation on 🟡 "Claim your catalog" — don't fix the copy, seed the catalogDigging into why this account has two Spotify-id'd artists and still no catalog turned up the actual cause, and it isn't in this PR. A hook that already does the right thing exists.
The onboarding form doesn't use it. chat#1892 wired
So an artist added through What I'd doPoint
SequencingThis is its own change, not something to fold into this PR — it touches the add path, not the reward surface, and it deserves its own preview verification. This PR's fallback stays correct and useful either way: it is the right thing to show when there genuinely is no catalog (valuation failed, or an account that predates seeding). Interim: if the seeding change doesn't land soon, soften this PR's CTA copy, because "Claim your catalog to see what it is worth" → "No catalogs found" with no create affordance reads worse than the generic green check it replaced. But that is a stopgap; the swap is the fix. |
There was a problem hiding this comment.
0 issues found across 2 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 3 unresolved issues from previous reviews.
Re-trigger cubic
Final verification — all four surfaces re-tested on
|
| # | Surface | State | Result | |
|---|---|---|---|---|
| A | /setup/valuation |
no catalog (real) | falls back to /catalogs after the read resolves |
✅ |
| B | RosterVerifiedPanel |
no valuation (real) | "Claim your catalog to see what it is worth." + CTA | ✅ |
| C | RosterVerifiedPanel |
valuation present | ValuationHero + "Set up your weekly report" |
✅ |
| D | /setup/valuation |
catalog present, cold load | renders the hero, no redirect | ✅ |
D — the fix, on the final commit
This is the case that was broken. Cold load, catalog present:
init @0ms
replaceState -> /setup/valuation @122ms
catalogs-fetch @1492ms
No replaceState -> /catalogs anywhere in the trace, and location.pathname === '/setup/valuation' at the end. On 402e4769 this same load redirected at 484ms, before the catalogs request was even issued.
A — the fallback still fires, and now on data
Same route, this account's real state (no catalog). Lands on /catalogs, so the fix didn't over-correct into "never redirects" — the three guard tests in the suite cover the same property.
B — cold-start completion panel
Walked the real flow: /setup/artists → continue → /setup/socials → continue. Replaces the old "Back to Recoup" green check.
C — the reward, when a valuation exists
Same walk with a catalog + band injected. ValuationHero ends the flow instead of the check.
Also asserted on D: zero em dashes in the rendered text (the hero range reads "$267K to $547K").
Unchanged from the earlier report
- 🟡 "Claim your catalog" still dead-ends at "No catalogs found" with no create affordance. Recommendation is in this comment and is now tracked as row 8 on chat#1889 — the fix is to seed the catalog on first artist add via
useAddSpotifyArtist, not to reword this CTA. - ⚪
/setup/catalogstill answers 200 rather than a 3xx (redirect()page stub, the pattern refactor(onboarding): retire /onboarding/* into /setup/* via config redirects (chat#1889 item 1, part 1) #1887 retired). Pre-existing. - The
hasCatalog === true+valuation.show === falseindefinite-skeleton case is still reachable;isPendingfixed the auth race, not this one.
Suite at 61911a6a: 86 passed / 23 files, tsc --noEmit → 13 (identical to main), eslint clean.
1 participant
Matrix row 10 in chat#1889 — the payoff the issue is named for. Independent of the convergence chain.
Why
Two dead ends on the same number:
CheckCircle2and a "Back to Recoup" button./setup/valuationwas a bareredirect("/catalogs")— so the welcome email's "See your baseline valuation" link had no real destination.A signup converts on a valuation and then never sees it again inside the product.
What changed
RosterVerifiedPanelrenders the account's valuation using the existing homepage hero (useHomeValuation+ValuationHero) — no second valuation surface — then points at the weekly report that keeps it measured.SetupValuationmounts at/setup/valuationwith the same hero, and falls back to/catalogsonly when the account genuinely has no catalog to value./setup/catalog) instead of dead-ending on "Back to Recoup".Verification
TDD, red → green:
```
RED — panel still renders the generic check, no valuation
× ends setup on the catalog valuation, not a generic green check
Tests 1 failed | 1 passed (2)
GREEN
pnpm exec vitest run components/Onboarding
Test Files 1 passed (1)
Tests 2 passed (2)
```
Tests assert both directions: the valuation figure and measured-track count render when a valuation exists (
$1.4M,42 tracks measured), and the confirmation state still appears when it does not — so a cold-start account can't hit an empty hero.pnpm exec tsc --noEmitclean for the touched files. Preview click-through pending (needs an authed account with a measured catalog).Tracked in chat#1889 (matrix row 10).
Summary by cubic
End onboarding on the catalog valuation and make
/setup/valuationshow the same valuation instead of redirecting. Delivers chat#1889 row 10 and adds a guard to prevent cold-load bounces.New Features
RosterVerifiedPanelnow shows the account’s valuation usingValuationHero+useHomeValuation, with a CTA to/setup/tasks. If no valuation, show confirmation with a CTA to/setup/catalog.SetupValuationat/setup/valuationrenders the same hero and weekly report CTA; only redirects to/catalogswhen the account has no catalog. Tests cover the hero view, fallbacks, and/setup/valuationredirects.Bug Fixes
/setup/valuationonisPending(notisLoading) fromuseCatalogsto avoid redirecting before auth enables the query, preventing cold-load bounces from the welcome email link.Written for commit 61911a6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes