fix(catalog): the report tab stops dead-ending cross-account and anonymous (chat#1912 row 1) - #1915
Conversation
…ymous (chat#1912 row 1) Measurements are account-scoped while the catalog is not, so the api returns 404 both when the owner's measurement has not landed yet and when someone else measured the catalog. Both collapsed into "No valuation found for this catalog ... Run a valuation from recoupable.dev", which sent a signed-in customer back to the marketing site to re-run a valuation already in flight. Anonymous visitors got "Something went wrong loading the measurements", because the auth-gated query never runs and a disabled query read as failure. Adds getCatalogReportState (loading | signed-out | measuring | other-account | error | ready) and getCatalogReportEmptyCopy, so each case tells the truth and keeps the customer in the app. While measuring, the report polls every 5s and resolves on its own instead of leaving an empty page. Replaces CatalogReportError with CatalogReportEmptyState. 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: 39 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 (3)
📒 Files selected for processing (8)
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: e293be08d2
ℹ️ 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".
| ownsCatalog: !!catalogId && !!data?.catalogs?.some((c) => c.id === catalogId), | ||
| isResolved: !isLoading, |
There was a problem hiding this comment.
Because isResolved is only !isLoading, every non-loading catalog-query state is treated as a definitive ownership answer, including when useCatalogs is disabled before userData.account_id exists, has errored, or is serving a still-fresh cached list from before an out-of-band catalog claim. In those cases a signed-in owner whose measurements endpoint returns the expected 404 for a freshly seeded catalog is passed through as ownsCatalog: false, so the report shows the cross-account copy and skips the 5s measuring poll until/unless the catalog list later refreshes. Keep this unresolved until the catalogs read has actually succeeded with current data, or surface an error instead of using absence from an unresolved list as proof of non-ownership.
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.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // The owner's measurement is still running, so pull again instead of leaving | ||
| // them on an empty report to re-run the whole valuation (chat#1912 row 1). | ||
| const { refetch } = measurementsQuery; | ||
| useEffect(() => { | ||
| if (state !== "measuring") return; | ||
| const id = setInterval(() => refetch(), MEASURING_POLL_MS); | ||
| return () => clearInterval(id); | ||
| }, [state, refetch]); |
There was a problem hiding this comment.
OCP
- actual: net new hooks added to existing component
- required: put useEffects in a hook like useOwnsCatalog.
There was a problem hiding this comment.
Fixed in 0bb24e0d.
The component no longer owns any of it. Composition instead:
hooks/useCatalogReport.tscomposes the measurements, songs, auth and ownership reads and derives the single report state.hooks/useMeasuringPoll.tsowns the while-measuring interval, so theuseEffectlives in a hook rather than in the component.CatalogReportContentis presentational again: it calls one hook and renders what the state describes. Its only remaining hook is the pre-existinguseMemofor the release rollups.
Net effect on the component: usePrivy, useCatalogMeasurements, useCatalogReportSongs, useOwnsCatalog, getCatalogReportState and the polling effect all left it, and one useCatalogReport call replaced them.
…(chat#1915 review) Review: net new hooks and a useEffect had been added straight into CatalogReportContent. Extends by composition instead (OCP), so the component is presentational again. - useCatalogReport composes measurements, songs, auth and ownership, and derives the single report state - useMeasuringPoll owns the while-measuring interval - CatalogReportContent renders what the state describes Also fixes three review findings: - useOwnsCatalog derives resolution from isSuccess/isError, never !isLoading. useCatalogs is enabled: !!accountId && authenticated, and a disabled TanStack v5 query reports isPending true / isFetching false, so !isLoading was already true while the account resolved and told a signed-in owner their own catalog was measured by another account. - a failed catalog list now reports ownershipUnknown and renders the error state rather than guessing "other-account" - the missing-measurement check matches the HTTP 404 status prefix instead of any "404" substring, so a 500 whose body mentions 404 stays an error - signed-out visitors no longer wait on the songs read before the sign-in CTA Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Confidence score: 4/5
- In
hooks/useCatalogReport.ts, a failed catalog-songs request can still produce a “ready” report with an empty releases table, which hides a real data-fetch failure and can mislead users into trusting incomplete results; includesongsQuery.errorin the derivederrorstate so the report shows its error path instead.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useCatalogReport.ts">
<violation number="1" location="hooks/useCatalogReport.ts:37">
P2: A catalog-songs request failure still renders a ready report with an empty releases table, rather than the report error state. Include `songsQuery.error` when deriving `error` so failed rollup data is surfaced instead of presented as no releases.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ? measurementsQuery.isLoading || songsQuery.isLoading || !isResolved | ||
| : false, | ||
| hasMeasurements: !!measurementsQuery.data, | ||
| error: measurementsQuery.error, |
There was a problem hiding this comment.
P2: A catalog-songs request failure still renders a ready report with an empty releases table, rather than the report error state. Include songsQuery.error when deriving error so failed rollup data is surfaced instead of presented as no releases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/useCatalogReport.ts, line 37:
<comment>A catalog-songs request failure still renders a ready report with an empty releases table, rather than the report error state. Include `songsQuery.error` when deriving `error` so failed rollup data is surfaced instead of presented as no releases.</comment>
<file context>
@@ -0,0 +1,52 @@
+ ? measurementsQuery.isLoading || songsQuery.isLoading || !isResolved
+ : false,
+ hasMeasurements: !!measurementsQuery.data,
+ error: measurementsQuery.error,
+ ownsCatalog,
+ ownershipUnknown,
</file context>
Preview verification — 2026-07-30Preview https://chat-6eev0xop0-recoup.vercel.app, confirmed built from head Fixtures from chat#1912: catalog A = Results
ScreenshotsAnonymous on catalog A — the state that used to read "Something went wrong":
Signed in as account B on catalog A — the headline fix:
Account B's own catalog — regression control, report renders exactly as before:
Row 3: the measuring state was not reproduced, and the acceptance criterion was wrongI could not reach it on the preview, and checking why turned up a mistake in how chat#1912 specified this. The Works-when step assumed a window where a freshly seeded catalog exists but has not been measured yet. That window does not exist on the seeding path. So the state is real but reachable by a different route: a catalog that has songs and no measurements for this viewer — songs added through Manage songs or a CSV upload, then measured later. That is worth verifying before the row is closed, and it needs a fixture I did not want to fabricate on prod data. What is verified: the state machine that produces it is unit-tested ( Recommendation: keep row 1 open for the measuring case, and re-word the issue's Works-when step 3 to the CSV/Manage-songs route. I have not edited the issue yet — say the word and I will. Row 4 is a real gap, deliberately not closed here
Review findingsAll four validated against the code before acting, none rubber-stamped.
Also added, not from a review: a failed catalog list now reports Suite 314 passed / 78 files, |
1 participant
Closes row 1 of chat#1912.
Why
Measurements are account-scoped; the catalog is not. So
GET /api/catalogs/{id}/measurementsreturns 404 in two very different situations, and the report tab collapsed both into one message:That is wrong in both directions:
playcount_snapshotsruns, 209s apart).A third state was wrong too: anonymous visitors got "Something went wrong loading the measurements", because
useCatalogMeasurementsis gated onauthenticatedand the query never runs. A disabled query is not a failure.What changed
lib/catalog/getCatalogReportState.ts— pure decision returningloading | signed-out | measuring | other-account | error | ready. The auth check deliberately precedes the error checks, since a disabled query leaves neither data nor error behind.lib/catalog/getCatalogReportEmptyCopy.ts— copy per state, kept out of the component so the wording is testable.hooks/useOwnsCatalog.ts— resolves ownership from the viewer's own catalog list, which is what separates "measuring" from "other-account". ExposesisResolvedso a still-loading list is never mistaken for non-ownership.CatalogReportEmptyStatereplacesCatalogReportError(deleted, no other callers).New copy:
/catalogsDeliberately not in this PR
The songs/report access mismatch.
GET /api/catalogs/songsreturns 200 unauthenticated, so the Manage songs tab serves a stranger all 24 tracks and ISRCs on the same page that withholds the valuation. The issue's Works-when step 4 covers it. Closing that is an api change and a product decision about whether catalogs are public, so it is not bundled into a chat-side copy-and-state fix. Row 1'sother-accountcopy is written to be honest either way ("The songs are listed under Manage songs, but the play counts and valuation belong to the account that measured them"). Flagging explicitly so this does not merge as if the access model were settled.Tests
TDD red→green (both RED as module-not-found first):
lib/catalog/__tests__/getCatalogReportState.test.ts— 8 cases, one per branch, including the anonymous no-error case and 500-vs-404.lib/catalog/__tests__/getCatalogReportEmptyCopy.test.ts— 16 cases, including a guard that no state's copy or CTA containsrecoupable.dev(the architecture decision recorded on the issue) and a house-style em-dash guard.Full suite 306 passed / 77 files,
tsc --noEmitclean.Verification
Preview verification against the issue's four-step Works when script will be posted as a PR comment once the preview builds. Note it needs the api
testbranch synced withmainfirst, since chat previews resolve totest-recoup-api.🤖 Generated with Claude Code
Summary by cubic
Fixes the Catalog report tab so anonymous and cross‑account viewers no longer dead‑end, and owners mid‑measurement see clear progress with auto‑refresh. Refactors state and polling into composed hooks and addresses review feedback (chat#1912 row 1, chat#1915).
Bug Fixes
getCatalogReportStateto clearly handle: loading, signed‑out, measuring, other‑account, error, ready; signed‑out no longer reads as an error and shows “Sign in” immediately.Refactors
useCatalogReportanduseMeasuringPoll, keepingCatalogReportContentpresentational.useOwnsCatalogwithownershipUnknown; moved empty‑state copy togetCatalogReportEmptyCopy.CatalogReportErrorwithCatalogReportEmptyState.Written for commit 0bb24e0. Summary will update on new commits.