fix(catalog): cross-account copy stops promising the songs tab (chat#1912 row 6) - #1917
Conversation
…1912 row 6) Third link of the row 6 chain, after recoupable/docs#282 (contract) and recoupable/api#802 (enforcement). The other-account state said "The songs are listed under Manage songs, but the play counts and valuation belong to the account that measured them." Once catalog songs are account-scoped, that tab returns 403 for a non-owner, so the copy would be pointing them at something that now fails. Rewords to state what is true either way and offers an in-app next step instead. The existing guards still hold: no em dashes, and no CTA off to recoupable.dev. 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.
|
📝 WalkthroughWalkthroughThe catalog report empty state now receives the viewer’s catalog ownership status. For other-account reports, it updates the guidance and directs viewers without catalogs to the catalog valuation setup flow. ChangesCatalog report copy
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Updates cross-account empty-state copy to remove a dead reference to a tab that now fails, and adds a guard test. The change is bounded and clearly beneficial, with implementation already reviewed.
Re-trigger cubic
There was a problem hiding this comment.
Joy - How can we put an action button on the screen to help create a happy path rather than leave this as a dead end with text?
There was a problem hiding this comment.
Good catch, and it was worse than "just text" — there was a button, but it was only a happy path for people who already have catalogs. Go to your catalogs sends a stranger who followed a shared link to an empty page: a second dead end, one click further in. That is the exact shape of the original bug.
Fixed in e781cd6f by making the action adapt to the viewer:
| Viewer | Action |
|---|---|
| has catalogs | Go to your catalogs → /catalogs |
| has none (the shared-link stranger) | Value your catalog → /setup/artists |
/setup/artists is the step that actually creates a catalog, so the person who arrived curious about someone else's number is now one click from getting their own. It stays in-app, which keeps the architecture decision on this issue intact (the empty state never bounces anyone to recoupable.dev).
Body copy now leads into the action rather than explaining the refusal twice: "Its play counts and valuation belong to the account that measured it. Here is how to get this report for your own music."
useOwnsCatalog already loads the viewer's catalog list to decide ownership, so the flag costs no extra request.
Four tests cover it, including one asserting the cross-account state always has something to click, in either branch — so this cannot silently regress to a dead end again.
Review (@sweetmantech): the state should offer a happy path rather than a paragraph and a dead end. It already had a button, but "Go to your catalogs" is only a happy path for someone who has catalogs. A stranger following a shared catalog link usually has none, so that button landed them on an empty page — a second dead end. The action now adapts: viewers with catalogs still go to theirs, and viewers with none get "Value your catalog" pointing at /setup/artists, the step that creates one. Body copy leads into the action instead of explaining the refusal twice. useOwnsCatalog already loads the viewer's catalog list for the ownership check, so the flag costs no extra request. 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: 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/Catalog/report/CatalogReportEmptyState.tsx`:
- Around line 18-24: Define and export a TypeScript interface named
CatalogReportEmptyStateProps for the state and hasOwnCatalogs props, then
replace the inline object type in CatalogReportEmptyState’s signature with that
interface.
🪄 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: c4ea6a52-4ef0-468a-bba0-6482eb5a7019
⛔ Files ignored due to path filters (1)
lib/catalog/__tests__/getCatalogReportEmptyCopy.test.tsis excluded by!**/*.test.*and included bylib/**
📒 Files selected for processing (5)
components/Catalog/report/CatalogReportContent.tsxcomponents/Catalog/report/CatalogReportEmptyState.tsxhooks/useCatalogReport.tshooks/useOwnsCatalog.tslib/catalog/getCatalogReportEmptyCopy.ts
| const CatalogReportEmptyState = ({ | ||
| state, | ||
| hasOwnCatalogs, | ||
| }: { | ||
| state: EmptyState; | ||
| hasOwnCatalogs: boolean; | ||
| }) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use an exported props interface for the component contract.
Define CatalogReportEmptyStateProps and use it in the component signature instead of an inline object type.
Suggested change
+export interface CatalogReportEmptyStateProps {
+ state: EmptyState;
+ hasOwnCatalogs: boolean;
+}
+
const CatalogReportEmptyState = ({
state,
hasOwnCatalogs,
-}: {
- state: EmptyState;
- hasOwnCatalogs: boolean;
-}) => {
+}: CatalogReportEmptyStateProps) => {As per coding guidelines, component prop types must use explicit exported ComponentNameProps names; as per path instructions, React component props must use TypeScript interfaces.
📝 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 CatalogReportEmptyState = ({ | |
| state, | |
| hasOwnCatalogs, | |
| }: { | |
| state: EmptyState; | |
| hasOwnCatalogs: boolean; | |
| }) => { | |
| export interface CatalogReportEmptyStateProps { | |
| state: EmptyState; | |
| hasOwnCatalogs: boolean; | |
| } | |
| const CatalogReportEmptyState = ({ | |
| state, | |
| hasOwnCatalogs, | |
| }: CatalogReportEmptyStateProps) => { |
🤖 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/Catalog/report/CatalogReportEmptyState.tsx` around lines 18 - 24,
Define and export a TypeScript interface named CatalogReportEmptyStateProps for
the state and hasOwnCatalogs props, then replace the inline object type in
CatalogReportEmptyState’s signature with that interface.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Confidence score: 3/5
- In
lib/catalog/getCatalogReportEmptyCopy.ts, the “Value your catalog” CTA appears to route viewers with completed roster/social setup but no catalog back to the first onboarding screen, which can trap users in repeated setup and increase drop-off; update this branch to send them to the intended post-setup destination instead. - In
lib/catalog/__tests__/getCatalogReportEmptyCopy.test.ts, the new “sends a viewer who has catalogs to their own” test duplicates an existing cross-account in-app test, which weakens test clarity and can hide intent-specific regressions; consolidate or rename so each test covers a distinct scenario.
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/catalog/getCatalogReportEmptyCopy.ts">
<violation number="1" location="lib/catalog/getCatalogReportEmptyCopy.ts:59">
P2: Viewers who already completed roster/social setup but have no catalog are sent back to the first onboarding screen when they click “Value your catalog,” forcing them to repeat completed steps. Point this CTA at the derived `/setup` entry so the onboarding flow selects the viewer’s actual next step.</violation>
</file>
<file name="lib/catalog/__tests__/getCatalogReportEmptyCopy.test.ts">
<violation number="1" location="lib/catalog/__tests__/getCatalogReportEmptyCopy.test.ts:73">
P3: New test "sends a viewer who has catalogs to their own" duplicates the existing "keeps a cross-account viewer in-app" test — same function call, same assertion. Either rename the existing test to cover both intents or remove this duplicate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (state === "other-account" && !viewer.hasOwnCatalogs) { | ||
| return { | ||
| ...copy, | ||
| cta: { label: "Value your catalog", href: "/setup/artists" }, |
There was a problem hiding this comment.
P2: Viewers who already completed roster/social setup but have no catalog are sent back to the first onboarding screen when they click “Value your catalog,” forcing them to repeat completed steps. Point this CTA at the derived /setup entry so the onboarding flow selects the viewer’s actual next step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/getCatalogReportEmptyCopy.ts, line 59:
<comment>Viewers who already completed roster/social setup but have no catalog are sent back to the first onboarding screen when they click “Value your catalog,” forcing them to repeat completed steps. Point this CTA at the derived `/setup` entry so the onboarding flow selects the viewer’s actual next step.</comment>
<file context>
@@ -40,6 +45,20 @@ const COPY: Record<CatalogReportEmptyState, CatalogReportEmptyCopy> = {
+ if (state === "other-account" && !viewer.hasOwnCatalogs) {
+ return {
+ ...copy,
+ cta: { label: "Value your catalog", href: "/setup/artists" },
+ };
+ }
</file context>
| cta: { label: "Value your catalog", href: "/setup/artists" }, | |
| cta: { label: "Value your catalog", href: "/setup" }, |
| // way forward, not a paragraph. "Go to your catalogs" is only a happy path | ||
| // for someone who has catalogs — a stranger following a shared link usually | ||
| // has none, and would land on an empty page. So the action adapts. | ||
| it("sends a viewer who has catalogs to their own", () => { |
There was a problem hiding this comment.
P3: New test "sends a viewer who has catalogs to their own" duplicates the existing "keeps a cross-account viewer in-app" test — same function call, same assertion. Either rename the existing test to cover both intents or remove this duplicate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/catalog/__tests__/getCatalogReportEmptyCopy.test.ts, line 73:
<comment>New test "sends a viewer who has catalogs to their own" duplicates the existing "keeps a cross-account viewer in-app" test — same function call, same assertion. Either rename the existing test to cover both intents or remove this duplicate.</comment>
<file context>
@@ -38,28 +41,56 @@ describe("getCatalogReportEmptyCopy", () => {
+ // way forward, not a paragraph. "Go to your catalogs" is only a happy path
+ // for someone who has catalogs — a stranger following a shared link usually
+ // has none, and would land on an empty page. So the action adapts.
+ it("sends a viewer who has catalogs to their own", () => {
+ const copy = getCatalogReportEmptyCopy("other-account", withCatalogs);
+
</file context>
Dismissed because Cubic found issues in a newer review.
Preview verification — 2026-07-30Preview https://chat-7tdt85jib-recoup.vercel.app, built from head The api Results
Row 5 is the reason this PR exists. With api#802 live on Row 4 matters more than it looks. The old CTA ( ScreenshotsNon-owner on a shared catalog — honest state with a real next action:
Manage songs tab, same viewer — the tracklist and ISRCs are gone:
The CTA lands on the roster step, not an empty page:
Review@sweetmantech asked for a happy path rather than a dead end. Addressed in Suite 337 passed / 82 files, NoteThis is the last link of the row 6 chain (docs#282 → api#802 → this). Both earlier links are merged, so production currently refuses the songs tab while still telling non-owners to go there. This should merge promptly rather than sit. |
1 participant
Third and final link of row 6's chain in chat#1912.
Merge order: docs#282 → api#802 → this. It is safe to merge at any point (the new wording is true before and after), but it only becomes necessary once api#802 lands.
Why
chat#1915 shipped this copy for a cross-account viewer:
That sentence was written to be true either way while the access model was undecided — at the time,
GET /api/catalogs/songsreally did return 200 to anyone. api#802 makes it 403 for a non-owner, so the copy would be pointing a stranger at a tab that now fails.What changed
Same title, same
Go to your catalogsCTA. The replacement states only what remains true and offers a next step instead of a dead reference.Tests
TDD red→green: a new case asserts the
other-accountbody does not mention "Manage songs", so this cannot silently regress if the copy is edited again later. The existing guards in that file still pass unchanged — norecoupable.devin any state's copy or CTA, and no em dashes.Full suite 333 passed / 82 files,
tsc --noEmitandeslintclean.Verification
Nothing to verify on a preview until api#802 is merged — before that, the songs tab still returns 200 and both wordings are accurate. Row 6's Works-when step 4 ("re-read the other-account wording and confirm it does not describe behaviour the API no longer has") covers this once the chain is complete.
🤖 Generated with Claude Code
Summary by cubic
Rewords the cross-account catalog report empty state so it no longer points to “Manage songs”. Adds an adaptive CTA that keeps viewers in-app with a clear next step.
/catalogs); none → "Value your catalog" (/setup/artists).hasOwnCatalogstouseOwnsCatalog→useCatalogReport→CatalogReportEmptyState, and tests to guard wording, ensure cross-account always has a CTA, and keep measuring with no CTA.Written for commit e781cd6. Summary will update on new commits.
Summary by CodeRabbit