fix(onboarding): /setup/valuation polls its way out of measuring (chat#1912 row 9) - #1919
Conversation
…t#1912 row 9)
The route rendered MeasuringCatalogPanel and never re-checked.
useCatalogMeasurements has no refetchInterval, and the only poller lived in
useCatalogReport, which row 1 built for /catalogs/{id}. A signup landing here
while a seeded catalog was still measuring sat on static text until they
refreshed by hand — the same "nothing is happening, try again" pressure this
issue exists to remove, on a route row 1 did not cover.
Adds getSetupValuationStatus (loading | redirect | measuring | ready) and
useSetupValuation, which composes the catalog and valuation reads, owns the
redirect, and reuses the existing useMeasuringPoll rather than adding a second
poller. SetupValuation renders what the status describes.
Also covers useMeasuringPoll directly: it polls while measuring, not
otherwise, and stops both when measuring ends and on unmount.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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: 13 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 (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSetup valuation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SetupValuation
participant useSetupValuation
participant CatalogQuery
participant HomeValuation
SetupValuation->>useSetupValuation: request status and valuation
useSetupValuation->>CatalogQuery: read catalog state
useSetupValuation->>HomeValuation: read valuation state
useSetupValuation->>CatalogQuery: invalidate measurements while measuring
useSetupValuation-->>SetupValuation: return status and valuation
Possibly related PRs
Suggested reviewers: 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: f79db01fbf
ℹ️ 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".
| queryClient.invalidateQueries({ queryKey: ["catalog-measurements"] }); | ||
| }, [queryClient]); | ||
|
|
||
| useMeasuringPoll(status === "measuring", refetchMeasurements); |
There was a problem hiding this comment.
Only gate this interval on an actual in-progress measurement, not every hidden valuation. useHomeValuation() returns { show: false } for permanent/non-measuring cases too, such as measurementsFailed, a pre-v2 response without measured_song_count, or an artist-scope mismatch in getValuationHeroState; with a catalog present those all become status === "measuring" here, so /setup/valuation keeps invalidating and refetching catalog-measurements every 5 seconds for as long as the tab is open even though the state cannot resolve by polling.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- In
lib/onboarding/getSetupValuationStatus.ts, failed measurements are currently mapped to "measuring," which can leave/setup/valuationstuck on a perpetual loading state and trigger repeated refetches every 5 seconds even for 500/auth failures; this is the main user-impact risk and should be addressed by adding a distinct failed/error status and stopping the polling loop on terminal errors. - In
components/Onboarding/SetupValuation.tsx, the|| !valuation.showcondition in the measuring branch is redundant with thestatuscontract, which raises a small maintainability/readability risk and could cause future branching confusion—remove the extra guard to keep rendering logic aligned with status semantics.
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/onboarding/getSetupValuationStatus.ts">
<violation number="1" location="lib/onboarding/getSetupValuationStatus.ts:31">
P2: A failed measurements request is treated as an in-progress measurement, so `/setup/valuation` stays on “Measuring” and invalidates the failed query every 5 seconds (including 500/authorization failures). Add a valuation-error status/input and reserve `measuring` for an absent, non-failed valuation.</violation>
</file>
<file name="components/Onboarding/SetupValuation.tsx">
<violation number="1" location="components/Onboarding/SetupValuation.tsx:34">
P3: The `|| !valuation.show` guard in the measuring check is redundant because the status already encodes whether the valuation is ready.
When `status` is "measuring", `valuation.show` is always `false` by the definition of `getSetupValuationStatus`. When `status` is "ready", `valuation.show` is always `true`. The early return for "loading" and "redirect" already ensures we only reach this line with those two remaining statuses, so `!valuation.show` can never alter the branch outcome.
Removing the redundant check would make the intent clearer — the status model is the single source of truth for what to render.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }: SetupValuationStatusInput): SetupValuationStatus { | ||
| if (catalogsPending) return "loading"; | ||
| if (catalogsFailed || !hasCatalog) return "redirect"; | ||
| return valuationReady ? "ready" : "measuring"; |
There was a problem hiding this comment.
P2: A failed measurements request is treated as an in-progress measurement, so /setup/valuation stays on “Measuring” and invalidates the failed query every 5 seconds (including 500/authorization failures). Add a valuation-error status/input and reserve measuring for an absent, non-failed valuation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/onboarding/getSetupValuationStatus.ts, line 31:
<comment>A failed measurements request is treated as an in-progress measurement, so `/setup/valuation` stays on “Measuring” and invalidates the failed query every 5 seconds (including 500/authorization failures). Add a valuation-error status/input and reserve `measuring` for an absent, non-failed valuation.</comment>
<file context>
@@ -0,0 +1,32 @@
+}: SetupValuationStatusInput): SetupValuationStatus {
+ if (catalogsPending) return "loading";
+ if (catalogsFailed || !hasCatalog) return "redirect";
+ return valuationReady ? "ready" : "measuring";
+}
</file context>
| // valuation is still measuring (chat#1889 row 8). | ||
| if (!valuation.show) return <MeasuringCatalogPanel />; | ||
| // valuation is still measuring (chat#1889 row 8). The hook polls it out. | ||
| if (status === "measuring" || !valuation.show) return <MeasuringCatalogPanel />; |
There was a problem hiding this comment.
P3: The || !valuation.show guard in the measuring check is redundant because the status already encodes whether the valuation is ready.
When status is "measuring", valuation.show is always false by the definition of getSetupValuationStatus. When status is "ready", valuation.show is always true. The early return for "loading" and "redirect" already ensures we only reach this line with those two remaining statuses, so !valuation.show can never alter the branch outcome.
Removing the redundant check would make the intent clearer — the status model is the single source of truth for what to render.
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 `|| !valuation.show` guard in the measuring check is redundant because the status already encodes whether the valuation is ready.
When `status` is "measuring", `valuation.show` is always `false` by the definition of `getSetupValuationStatus`. When `status` is "ready", `valuation.show` is always `true`. The early return for "loading" and "redirect" already ensures we only reach this line with those two remaining statuses, so `!valuation.show` can never alter the branch outcome.
Removing the redundant check would make the intent clearer — the status model is the single source of truth for what to render.</comment>
<file context>
@@ -47,8 +30,8 @@ const SetupValuation = () => {
- // valuation is still measuring (chat#1889 row 8).
- if (!valuation.show) return <MeasuringCatalogPanel />;
+ // valuation is still measuring (chat#1889 row 8). The hook polls it out.
+ if (status === "measuring" || !valuation.show) return <MeasuringCatalogPanel />;
return (
</file context>
| if (status === "measuring" || !valuation.show) return <MeasuringCatalogPanel />; | |
| if (status === "measuring") return <MeasuringCatalogPanel />; |
Preview verification found the fix missed the case that actually happens. Measured on a fresh signup: artist added 00:59:04, snapshot 00:59:07, catalog created 00:59:19. createSnapshotCatalog runs only after the measurements land, so for ~15s there is no catalog at all. Navigating to /setup/valuation in that window read the empty list as "nothing to value" and redirected to /catalogs, which renders "No catalogs found" — a worse dead end than the static panel this row set out to fix, and the exact window a signup following the flow arrives in. The window the route was originally written for, catalog exists but not yet measured, is about one second by comparison, because the measurements are written first. getSetupValuationStatus now takes hasArtists: an artist means seeding is in flight and a catalog is coming, so that state is measuring rather than redirect. Redirect is reserved for an account with neither. The poll invalidates the catalogs query as well as measurements, since during seeding it is the catalog that is missing. Removes a test whose premise this changes: "no catalog" alone no longer means redirect, and the two cases replacing it assert both halves. 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 2 unresolved issues from previous reviews.
Re-trigger cubic
Second preview run still redirected. The roster loads asynchronously, so on a fresh page load of /setup/valuation `sorted` is briefly empty, which read as "no artists" and redirected before seeding could be detected. That is the same trap the catalogs `isPending` comment in this file already warns about, repeated for the roster: pending is not empty. getSetupValuationStatus now takes artistsPending and reports loading until both reads resolve. 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 2 unresolved issues from previous reviews.
Re-trigger cubic
Preview verification — 2026-07-31Preview https://chat-xk0csc8g2-recoup.vercel.app, built from head Results
Timing for this account: artist added
Two defects this found that the unit tests passed straight throughBoth needed a real signup; the tests were green throughout because they supply already-resolved state. 1. The fix was aimed at the wrong window. The first run still redirected to an empty 2. A loading roster is not an empty one. The second run still redirected. The roster loads asynchronously, so A measurement error worth recordingMy first "has it resolved?" probe matched Tests
Full suite 349 passed / 83 files, ResidueThree test accounts ( |
1 participant
Closes row 9 of chat#1912.
Why
/setup/valuationrendersMeasuringCatalogPanel— "Measuring your catalog … this usually takes a minute" — and then never re-checks.useCatalogMeasurementssetsstaleTimeandrefetchOnWindowFocus: falseand has norefetchInterval. The only poller in the codebase lives inuseCatalogReport, which chat#1915 built for/catalogs/{id}. So a signup who lands here while their seeded catalog is still measuring sits on static text until they refresh by hand.That is the same dead end row 1 fixed, on a route row 1 did not cover — and this one is on the path the welcome email points at.
What changed
lib/onboarding/getSetupValuationStatus.ts— pureloading | redirect | measuring | ready.catalogsPendingis checked first, because a pending list is not an empty one:useCatalogsisenabled: !!accountId && authenticated, and a disabled TanStack v5 query reportsisPendingtrue /isFetchingfalse. Reading that as "no catalog" would redirect a signup away mid-load.hooks/useSetupValuation.ts— composes the catalog and valuation reads, owns the redirect, and reuses the existinguseMeasuringPollrather than introducing a second poller.SetupValuationrenders what the status describes; itsuseEffectmoved into the hook, per the OCP note on chat#1915.Tests
getSetupValuationStatus— 6 cases including the one that matters, "prefers loading over redirect when both could apply", which is the trap the original comment in this file warned about.useMeasuringPoll— 4 new cases, the first direct coverage this hook has had since row 1 introduced it: it polls while measuring, does not when idle, stops when measuring ends, and stops on unmount. The last two matter because a poll that outlives its state keeps hitting the api for as long as the tab is open.SetupValuationtests pass unchanged through the new composition. They needed one added mock: the component now reachesuseQueryClientthrough the hook, and those tests render it without a provider.Full suite 347 passed / 83 files,
tsc --noEmitandeslintclean.Verification
Preview verification against row 9's Works-when — add a first artist on a fresh account, go straight to
/setup/valuation, and do not touch the page; it should resolve to the baseline valuation on its own, and polling should stop once it does — will be posted as a PR comment.🤖 Generated with Claude Code
Summary by cubic
Fixes onboarding
/setup/valuationto poll catalogs and measurements and handle loading states correctly, so the page exits “Measuring your catalog” on its own and avoids redirects during seeding or while the roster is still loading.Bug Fixes
useMeasuringPoll; stops when measuring ends or on unmount.useCatalogsand the artist roster as loading, not “no catalog”/“no artists” (@tanstack/react-queryv5).["catalogs"]and["catalog-measurements"]while polling.Refactors
getSetupValuationStatus(loading | redirect | measuring | ready).useSetupValuationto compose reads, own redirect, and drive polling.SetupValuationnow renders solely based on status.Written for commit 1615d7f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes