fix(catalog): link the caller's account on catalog re-claim (chat#1867) - #775
fix(catalog): link the caller's account on catalog re-claim (chat#1867)#775sweetmantech wants to merge 2 commits into
Conversation
The re-claim branch of POST /api/catalogs returned an already-created
catalog without writing an account_catalogs row for the caller. Because
GET /api/catalogs/{id}/measurements 404s on a missing account_catalogs
link, a reader whose resolved account differs from the account that first
claimed the snapshot (account divergence / a prior double-account race)
got "No valuation found" on the post-valuation report — the funnel's
measurements are persisted, only the ownership link was missing.
Fix: call insertAccountCatalog in the re-claim branch, and make
insertAccountCatalog an idempotent upsert (onConflict account,catalog,
ignoreDuplicates) so linking an already-linked account is a no-op rather
than a unique-violation. The fresh-claim path already linked; this extends
the same guarantee to re-claims.
TDD red-first. Full suite 4123 pass, tsc clean on touched files, lint +
prettier clean. No API contract change.
Co-Authored-By: Claude Opus 4.8 (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: 51 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 Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe snapshot re-claim path now restores the authenticated account’s link to an existing catalog. Account-catalog links use idempotent upserts targeting the ChangesCatalog ownership
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/catalog/createCatalogHandler.ts (1)
31-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the re-claim logic to keep the function under 50 lines.
The
createCatalogHandlerfunction is currently 60 lines long, which exceeds the limit. As per path instructions forlib/**/*.ts, functions should be kept under 50 lines. Consider extracting the existing catalog re-claim logic into a separate helper function to improve readability and strictly adhere to the Single Responsibility Principle.♻️ Proposed refactoring
export async function createCatalogHandler(request: NextRequest): Promise<NextResponse> { try { const body = await request.json().catch(() => null); const validated = validateCreateCatalogBody(body); if (validated instanceof NextResponse) { return validated; } const authResult = await validateAuthContext(request); if (authResult instanceof NextResponse) { return authResult; } const { accountId } = authResult; if (!validated.snapshot) { const catalog = await insertCatalog(validated.name ?? DEFAULT_CATALOG_NAME); await insertAccountCatalog({ account: accountId, catalog: catalog.id }); return successResponse({ catalog, songs_added: 0 }); } const [snapshot] = await selectPlaycountSnapshots({ id: validated.snapshot }); if (!snapshot) { return errorResponse("Snapshot not found", 404); } if (snapshot.account !== accountId) { return errorResponse("Snapshot belongs to a different account", 403); } - // Idempotent re-claim: the run already produced a catalog. Link the caller - // to it (chat#1867): the report's measurements endpoint 404s on a missing - // account_catalogs row, so a reader whose account differs from the original - // claimer (account divergence / prior double-account race) otherwise sees - // "No valuation found". insertAccountCatalog is idempotent, so this heals - // an unlinked reader without duplicating an existing link. Still attach the - // canonical artist (chat#1850 P1); the attach is itself idempotent. - if (snapshot.catalog) { - const existing = await selectCatalogById(snapshot.catalog); - if (existing) { - await insertAccountCatalog({ account: accountId, catalog: existing.id }); - const measurements = await selectSongMeasurements({ snapshot: snapshot.id }); - const isrcs = [...new Set(measurements.map(m => m.song))]; - if (isrcs.length > 0) { - await attachCanonicalArtistToAccount({ accountId, isrcs }); - } - return successResponse({ catalog: existing, songs_added: 0 }); - } + const reClaimResponse = await handleReclaimCatalog(accountId, snapshot); + if (reClaimResponse) { + return reClaimResponse; } const { catalog, songsAdded } = await createSnapshotCatalog({ accountId, snapshot, name: validated.name, }); return successResponse({ catalog, songs_added: songsAdded }); } catch (error) { console.error("Error creating catalog:", error); return errorResponse("Internal server error", 500); } } + +async function handleReclaimCatalog(accountId: string, snapshot: any): Promise<NextResponse | null> { + if (!snapshot.catalog) return null; + + const existing = await selectCatalogById(snapshot.catalog); + if (!existing) return null; + + await insertAccountCatalog({ account: accountId, catalog: existing.id }); + const measurements = await selectSongMeasurements({ snapshot: snapshot.id }); + const isrcs = [...new Set(measurements.map(m => m.song))]; + + if (isrcs.length > 0) { + await attachCanonicalArtistToAccount({ accountId, isrcs }); + } + + return successResponse({ catalog: existing, songs_added: 0 }); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/catalog/createCatalogHandler.ts` around lines 31 - 90, Extract the existing snapshot.catalog re-claim block from createCatalogHandler into a focused helper that accepts the account ID and snapshot/catalog context, performs the idempotent account link and canonical-artist attachment, and returns the existing catalog response data. Update createCatalogHandler to call the helper while preserving its current behavior and reduce the function below 50 lines; do not alter validation, authorization, or new-catalog creation flows.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/catalog/createCatalogHandler.ts`:
- Around line 31-90: Extract the existing snapshot.catalog re-claim block from
createCatalogHandler into a focused helper that accepts the account ID and
snapshot/catalog context, performs the idempotent account link and
canonical-artist attachment, and returns the existing catalog response data.
Update createCatalogHandler to call the helper while preserving its current
behavior and reduce the function below 50 lines; do not alter validation,
authorization, or new-catalog creation flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f250f61-26de-4193-a5c6-e9caf4cb3814
⛔ Files ignored due to path filters (2)
lib/catalog/__tests__/createCatalogHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/supabase/account_catalogs/__tests__/insertAccountCatalog.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (2)
lib/catalog/createCatalogHandler.tslib/supabase/account_catalogs/insertAccountCatalog.ts
There was a problem hiding this comment.
KISS
- actual: lib/supabase/account_catalogs/insertAccountCatalog.ts
- required: lib/supabase/account_catalogs/upsertAccountCatalog.ts
There was a problem hiding this comment.
Done — renamed insertAccountCatalog → upsertAccountCatalog (file + function + all call sites + tests), matching the existing upsertAccountSnapshot convention. Pure rename, no behavior change; 21 tests green, tsc/lint clean. Pushed as ea1fdc19.
…tches the upsert) Per review (KISS): the function now upserts, so name it for what it does, matching the repo's existing upsertAccountSnapshot convention. Pure symbol + file rename across all call sites and tests; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Auto-approved: Fixes a missing account-catalog link on re-claim that caused a 404. Refactors insert to idempotent upsert. No schema, contract, or rollout changes; bounded bug fix with tests.
Re-trigger cubic
|
Closing — not needed. A DB inspection settled the diagnosis, and it invalidates the premise this PR was opened on. For catalog
So the claim worked and linked the correct account. My 404 was a cross-account test artifact: I ran the funnel/claim under one account and read the report on the preview logged in as a different fresh account — correct IDOR behavior, not a bug. The re-claim branch requires The |
1 participant
Prerequisite for the post-valuation catalog report (recoupable/chat#1867, the row above chat#1873 in that tracker). Fixes the confirmed onboarding break where a real signup runs a valuation, clicks "Get the full report", and the report renders "No valuation found for this catalog."
Root cause
GET /api/catalogs/{id}/measurementsreturns 404 only when the caller has noaccount_catalogsrow for the catalog (lib/catalog/getCatalogMeasurementsHandler.ts— missing measurement data returns 200-with-zeros, not 404). The measurement rows themselves are persisted by the funnel snapshot workflow.The gap:
createCatalogHandler's re-claim branch (snapshot.catalogalready set) returned the existing catalog without callinginsertAccountCatalog. The fresh-claim path (createSnapshotCatalog) links the caller; the re-claim path did not. So whenever the reader's resolved account differs from the account that first claimed the snapshot — account divergence, or a prior double-account race (chat#1875) — the reader is never linked and the report 404s.What changed
lib/catalog/createCatalogHandler.ts— the re-claim branch now callsinsertAccountCatalog({ account: accountId, catalog: existing.id })before returning, guaranteeing the caller owns the catalog (mirrors the fresh-claim path).lib/supabase/account_catalogs/insertAccountCatalog.ts—insert→ idempotentupsert(onConflict: "account,catalog", ignoreDuplicates: true, matching theaccount_catalogs_account_catalog_uniqueconstraint), so linking an already-linked account is a no-op instead of a unique-violation. Mirrors the siblinginsertCatalogSongsupsert pattern.No API contract change (response shapes unchanged) → no
docsPR needed.TDD / verification
lib/catalog/__tests__/createCatalogHandler.test.ts"re-claims link the caller's account…" (assertsinsertAccountCatalogis called on re-claim) and newlib/supabase/account_catalogs/__tests__/insertAccountCatalog.test.ts(idempotent upsert args + error path). Confirmed both fail against the old code.tsc --noEmitclean on touched files (repo has a large pre-existing tsc baseline in unrelated files; CI runspnpm test);eslint+prettierclean on touched files.Preview verification — pending (honest)
Not yet exercised end-to-end on a preview. The clean way to verify: authenticated
POST /api/catalogs { snapshot }re-claim →account_catalogsrow exists for the caller →GET /api/catalogs/{id}/measurementsreturns 200 with rows (was 404). Also recommend a one-time DB spot-check thatsong_measurementsrows exist for a stuck catalog (e.g.a0a4a769-…) to confirm the fix addresses the observed repro rather than a data gap.Note on the chat report preview: chat previews read
test-recoup-api, so verifying chat#1873's happy path against a chat preview needs this fix live on the api deployment that preview reads. Merging here lands it on prodapi; getting it ontotest-recoup-apiis a separate step.Part of recoupable/chat#1867 (does not close it).
🤖 Generated with Claude Code
Summary by cubic
Links the caller’s account when re-claiming a catalog so the measurements endpoint stops 404ing and the post-valuation report shows data instead of “No valuation found for this catalog.” Makes account–catalog linking idempotent to handle repeated claims safely.
Bug Fixes
Refactors
Written for commit ea1fdc1. Summary will update on new commits.
Summary by CodeRabbit