feat(onboarding): converge home onto /setup at the derived step (chat#1889 item 1, part 2) - #1890
Conversation
|
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.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughOnboarding now enters ChangesSetup routing and derived-step resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HomePage
participant SetupEntry
participant OnboardingState
participant Router
HomePage->>Router: replace /setup for incomplete onboarding
SetupEntry->>OnboardingState: read ready state and derived step
SetupEntry->>Router: replace canonical setup route
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
chat/components/Home/HomePage.tsx
Line 39 in 0b36f76
Avoid bootstrapping chat while redirecting to setup
When onboarding sources are already cached and resolved on entry to /, shouldEnterSetup is true on the first render, yet this still mounts NewChatBootstrap before the replacement navigation runs. Its mount effect starts the non-idempotent POST /api/sessions and POST /api/sandbox provisioning sequence, and the route change does not cancel those requests, leaving an unused session and sandbox for each such home visit. Keep the chat bootstrap unmounted while forwarding to setup.
ℹ️ 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".
| const SETUP_PATHS: Record<OnboardingStepId, string> = { | ||
| artists: "/setup/artists", | ||
| socials: "/setup/socials", | ||
| catalog: "/setup/catalog", |
There was a problem hiding this comment.
When the derived step is catalog, this sends an incomplete home visitor to /setup/catalog, but app/setup/catalog/page.tsx immediately redirects to /catalogs, where no SetupSkipLink is mounted. This removes the former sequence's escape hatch specifically for accounts without a catalog; /catalogs only displays “No catalogs found” for that state, and returning home starts the redirect loop again. Route this step to a surface that includes the skip control or add an equivalent escape hatch to the catalog destination.
Useful? React with 👍 / 👎.
| const SetupSkipLink = () => { | ||
| const router = useRouter(); | ||
| const { userData } = useUserProvider(); | ||
| const { skip } = useOnboardingSessionFlags(userData?.account_id ?? ""); |
There was a problem hiding this comment.
On a direct /setup/artists, /setup/socials, or /setup/tasks visit, this button renders while UserProvider may still be resolving userData. If the user clicks it then, the flag is written under the empty account key; after navigation, the home gate switches to the real account key, reads false, and forwards the user back into setup. Hide or disable the control until account_id is available rather than recording a skip for "".
Useful? React with 👍 / 👎.
| skip(); | ||
| router.push("/"); |
There was a problem hiding this comment.
When sessionStorage is denied or throws, skip() only updates this component's in-memory state because writeOnboardingFlag deliberately catches storage failures. The immediate navigation then unmounts that state, so the newly mounted home gate reads skipped: false and redirects straight back to setup, making the advertised soft gate unskippable in that supported failure mode. Carry the in-memory decision across navigation or provide a fallback that does not depend on the failed write.
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.
6 issues found across 16 files
Confidence score: 2/5
- In
lib/onboarding/getSetupPathForStep.ts, thecatalogroute appears to bypass the skip-capable flow used by other setup steps, which can trap users in setup with no equivalent escape path; aligncatalogwith a component/path that includesSetupSkipLinkbehavior. - In
components/Onboarding/SetupEntry.tsx, a failed or unavailable checkpoint fetch can leave/setupstuck on a loading skeleton indefinitely, effectively blocking onboarding progress; add an error/timeout fallback with a retry or alternate navigation path. - In
components/Onboarding/SetupSkipLink.tsx, skip persistence is fragile: clicks before account resolution write under an empty id, and storage write failures fall back to memory-only state, both of which can cause users to be redirected back into setup after navigation; gate skip writes on a resolved account id and add a durable fallback strategy whensessionStorageis unavailable. - In
components/Home/HomePage.tsx, incomplete accounts can briefly mount chat before redirecting to setup, creating a visible flash and unnecessary chat-session provisioning; add a pre-redirect loading/empty gate so setup-bound users never mount the home chat path.
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/SetupSkipLink.tsx">
<violation number="1" location="components/Onboarding/SetupSkipLink.tsx:21">
P2: If `sessionStorage` is unavailable (e.g. in private browsing on some older WebViews, or when storage quota is exceeded), `skip()` catches the write failure and only updates in-memory state. The immediate `router.push("/")` then unmounts this component — discarding that state — and the home gate reads `skipped: false`, redirecting right back into setup. This makes the soft gate unskippable in that failure mode.
Consider passing the skip intent as a query param (e.g. `/?skipped=1`) or verifying the write succeeded before navigating, so the skip survives even when storage is denied.</violation>
<violation number="2" location="components/Onboarding/SetupSkipLink.tsx:21">
P2: When account data has not resolved yet, clicking this link stores the skip flag under an empty account id, so the home gate will not recognize the skip for the actual account and can immediately redirect the user back into `/setup`. Guard the action until `userData?.account_id` exists (or disable the link while it is loading) before writing the session flag.</violation>
</file>
<file name="components/Onboarding/SetupEntry.tsx">
<violation number="1" location="components/Onboarding/SetupEntry.tsx:28">
P2: An errored or unavailable checkpoint request leaves `/setup` permanently showing the loading skeleton because `isReady` never becomes true and this entry has no fallback or escape hatch. The entry should expose a recoverable error/continue path (or otherwise distinguish errors from the initial loading state) instead of treating every non-ready state as indefinitely loading.</violation>
<violation number="2" location="components/Onboarding/SetupEntry.tsx:29">
P3: While setup data is loading, screen-reader users get no indication that the page is waiting because the skeleton container has no status semantics or accessible label. Adding a status region (for example `role="status"` with an accessible loading label) would expose the state conveyed visually.</violation>
</file>
<file name="components/Home/HomePage.tsx">
<violation number="1" location="components/Home/HomePage.tsx:33">
P2: Incomplete accounts still mount the home chat while this redirect is pending, causing a visible home/chat flash and an unnecessary chat-session provisioning request before the canonical setup flow loads. A loading/empty handoff should be rendered instead of `NewChatBootstrap` whenever the account is being redirected to `/setup`.</violation>
</file>
<file name="lib/onboarding/getSetupPathForStep.ts">
<violation number="1" location="lib/onboarding/getSetupPathForStep.ts:14">
P1: The `catalog` step routes to `/setup/catalog`, but unlike the artists/socials steps (which render `RosterSocialsFlow` with `SetupSkipLink`) and the task step (which renders `FirstTaskStep` with `SetupSkipLink`), the catalog destination doesn't appear to mount a `SetupSkipLink`. If `/setup/catalog` redirects to `/catalogs` — where the skip control isn't present — an account stuck at this step enters a redirect loop: home → `/setup` → `/setup/catalog` → `/catalogs` → (user navigates home) → repeat, with no escape hatch. Either wire `SetupSkipLink` into the catalog step's surface or route this to a wrapper that includes it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const SETUP_PATHS: Record<OnboardingStepId, string> = { | ||
| artists: "/setup/artists", | ||
| socials: "/setup/socials", | ||
| catalog: "/setup/catalog", |
There was a problem hiding this comment.
P1: The catalog step routes to /setup/catalog, but unlike the artists/socials steps (which render RosterSocialsFlow with SetupSkipLink) and the task step (which renders FirstTaskStep with SetupSkipLink), the catalog destination doesn't appear to mount a SetupSkipLink. If /setup/catalog redirects to /catalogs — where the skip control isn't present — an account stuck at this step enters a redirect loop: home → /setup → /setup/catalog → /catalogs → (user navigates home) → repeat, with no escape hatch. Either wire SetupSkipLink into the catalog step's surface or route this to a wrapper that includes it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/onboarding/getSetupPathForStep.ts, line 14:
<comment>The `catalog` step routes to `/setup/catalog`, but unlike the artists/socials steps (which render `RosterSocialsFlow` with `SetupSkipLink`) and the task step (which renders `FirstTaskStep` with `SetupSkipLink`), the catalog destination doesn't appear to mount a `SetupSkipLink`. If `/setup/catalog` redirects to `/catalogs` — where the skip control isn't present — an account stuck at this step enters a redirect loop: home → `/setup` → `/setup/catalog` → `/catalogs` → (user navigates home) → repeat, with no escape hatch. Either wire `SetupSkipLink` into the catalog step's surface or route this to a wrapper that includes it.</comment>
<file context>
@@ -0,0 +1,24 @@
+const SETUP_PATHS: Record<OnboardingStepId, string> = {
+ artists: "/setup/artists",
+ socials: "/setup/socials",
+ catalog: "/setup/catalog",
+ task: "/setup/tasks",
+};
</file context>
| const { skip } = useOnboardingSessionFlags(userData?.account_id ?? ""); | ||
|
|
||
| const handleSkip = () => { | ||
| skip(); |
There was a problem hiding this comment.
P2: If sessionStorage is unavailable (e.g. in private browsing on some older WebViews, or when storage quota is exceeded), skip() catches the write failure and only updates in-memory state. The immediate router.push("/") then unmounts this component — discarding that state — and the home gate reads skipped: false, redirecting right back into setup. This makes the soft gate unskippable in that failure mode.
Consider passing the skip intent as a query param (e.g. /?skipped=1) or verifying the write succeeded before navigating, so the skip survives even when storage is denied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupSkipLink.tsx, line 21:
<comment>If `sessionStorage` is unavailable (e.g. in private browsing on some older WebViews, or when storage quota is exceeded), `skip()` catches the write failure and only updates in-memory state. The immediate `router.push("/")` then unmounts this component — discarding that state — and the home gate reads `skipped: false`, redirecting right back into setup. This makes the soft gate unskippable in that failure mode.
Consider passing the skip intent as a query param (e.g. `/?skipped=1`) or verifying the write succeeded before navigating, so the skip survives even when storage is denied.</comment>
<file context>
@@ -0,0 +1,36 @@
+ const { skip } = useOnboardingSessionFlags(userData?.account_id ?? "");
+
+ const handleSkip = () => {
+ skip();
+ router.push("/");
+ };
</file context>
| router.replace(getSetupPathForStep(step)); | ||
| }, [isReady, step, router]); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
P2: An errored or unavailable checkpoint request leaves /setup permanently showing the loading skeleton because isReady never becomes true and this entry has no fallback or escape hatch. The entry should expose a recoverable error/continue path (or otherwise distinguish errors from the initial loading state) instead of treating every non-ready state as indefinitely loading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupEntry.tsx, line 28:
<comment>An errored or unavailable checkpoint request leaves `/setup` permanently showing the loading skeleton because `isReady` never becomes true and this entry has no fallback or escape hatch. The entry should expose a recoverable error/continue path (or otherwise distinguish errors from the initial loading state) instead of treating every non-ready state as indefinitely loading.</comment>
<file context>
@@ -0,0 +1,37 @@
+ router.replace(getSetupPathForStep(step));
+ }, [isReady, step, router]);
+
+ return (
+ <div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8">
+ <Skeleton className="h-8 w-2/3 rounded-lg" />
</file context>
| const { skip } = useOnboardingSessionFlags(userData?.account_id ?? ""); | ||
|
|
||
| const handleSkip = () => { | ||
| skip(); |
There was a problem hiding this comment.
P2: When account data has not resolved yet, clicking this link stores the skip flag under an empty account id, so the home gate will not recognize the skip for the actual account and can immediately redirect the user back into /setup. Guard the action until userData?.account_id exists (or disable the link while it is loading) before writing the session flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupSkipLink.tsx, line 21:
<comment>When account data has not resolved yet, clicking this link stores the skip flag under an empty account id, so the home gate will not recognize the skip for the actual account and can immediately redirect the user back into `/setup`. Guard the action until `userData?.account_id` exists (or disable the link while it is loading) before writing the session flag.</comment>
<file context>
@@ -0,0 +1,36 @@
+ const { skip } = useOnboardingSessionFlags(userData?.account_id ?? "");
+
+ const handleSkip = () => {
+ skip();
+ router.push("/");
+ };
</file context>
|
|
||
| useEffect(() => { | ||
| if (shouldEnterSetup) { | ||
| router.replace("/setup"); |
There was a problem hiding this comment.
P2: Incomplete accounts still mount the home chat while this redirect is pending, causing a visible home/chat flash and an unnecessary chat-session provisioning request before the canonical setup flow loads. A loading/empty handoff should be rendered instead of NewChatBootstrap whenever the account is being redirected to /setup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Home/HomePage.tsx, line 33:
<comment>Incomplete accounts still mount the home chat while this redirect is pending, causing a visible home/chat flash and an unnecessary chat-session provisioning request before the canonical setup flow loads. A loading/empty handoff should be rendered instead of `NewChatBootstrap` whenever the account is being redirected to `/setup`.</comment>
<file context>
@@ -18,21 +19,20 @@ const HomePage = ({ initialMessages }: { initialMessages?: UIMessage[] }) => {
+
+ useEffect(() => {
+ if (shouldEnterSetup) {
+ router.replace("/setup");
+ }
+ }, [shouldEnterSetup, router]);
</file context>
| }, [isReady, step, router]); | ||
|
|
||
| return ( | ||
| <div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8"> |
There was a problem hiding this comment.
P3: While setup data is loading, screen-reader users get no indication that the page is waiting because the skeleton container has no status semantics or accessible label. Adding a status region (for example role="status" with an accessible loading label) would expose the state conveyed visually.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupEntry.tsx, line 29:
<comment>While setup data is loading, screen-reader users get no indication that the page is waiting because the skeleton container has no status semantics or accessible label. Adding a status region (for example `role="status"` with an accessible loading label) would expose the state conveyed visually.</comment>
<file context>
@@ -0,0 +1,37 @@
+ }, [isReady, step, router]);
+
+ return (
+ <div className="mx-auto flex w-full max-w-xl flex-col gap-4 px-6 py-8">
+ <Skeleton className="h-8 w-2/3 rounded-lg" />
+ <Skeleton className="h-[72px] w-full rounded-xl" />
</file context>
Notes from shipping #1887 (merged 2026-07-27,
|
One canonical onboarding surface (chat#1889 item 1, part 2). Home hosted a second, card-based sequence whose cards only linked OUT to the generic app pages (/artists, /artists, /catalogs, /tasks), while the real interactive steps lived only under /setup/*. A direct signup — and anyone arriving from a marketing CTA — therefore never saw the flow the welcome email's /setup/* deep links reach. The step copy was also triplicated (getOnboardingStepContent, api's welcomeOnboardingSteps, each step's own heading). - HomePage forwards an incomplete account to /setup instead of rendering its own sequence; the pinned-checklist (skipped) path is unchanged. - /setup opens at the DERIVED step via new SetupEntry + getSetupPathForStep, instead of hard-redirecting to /setup/artists — a signup who had already confirmed their roster was being sent back to step 1. - Deleted the placeholder surface: OnboardingSequence, OnboardingStepCard and getOnboardingStepContent (its own docstring called it placeholder copy pending the real steps, which have since shipped). - OnboardingCheckpointList now reads titles from getOnboardingStepTitle. - SetupSkipLink carries the gate's escape hatch onto the /setup surface, so forwarding home doesn't turn the soft gate into a wall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0b36f76 to
1a62472
Compare
Rebased + preview verification — 2026-07-28Preview: https://chat-681ule3mz-recoup.vercel.app — built from RebaseThis branch was cut before chat#1887 and chat#1886 merged, and GitHub reported it DIRTY. Rebased onto Resolved in favour of this PR's version, and not just because it's newer: Post-rebase sanity: #1886's ResultsTwo accounts, chosen so the interesting behaviours sit at opposite ends — plus a third state created mid-test to prove the step is actually derived rather than hardcoded.
Row 7 is the one that proves the feature. Rows 3 and 9 are the two extremes and would both pass a naive "empty → step 1, complete → home" implementation. Adding a single artist mid-test moved Row 9 fixes a live prod bug. On Rows 4 + 5 matter as much as the convergence itself. A gate that can't be exited, or that re-traps on every reload, would be worse than the placeholder cards it replaces. It does neither — and the checklist on home (row 6) keeps the sequence reachable after skipping. Cold start — home converges on
After "Skip for now" — home renders, checklist keeps it reachable:
Derived step — one artist added,
Onboarded account — home untouched:
Onboarded account at
NoteRow 6 shows the checklist rendering exactly 4 checkpoints, matching |
1 participant
Part 2 of item 1 in chat#1889 — the keystone. Part 1 is #1887 (repoint referrers).
Why
Traced on
main: 4 entry paths land on 3 parallel implementations of the same onboarding steps./catalogs/{id}→CatalogReportCta/onboarding/first-task/setup/*(the interactive steps)appUrlrefs)OnboardingSequenceHome's cards only linked out to the generic app pages (
/artists,/artists,/catalogs,/tasks), so a direct signup never reached the interactive steps the welcome email's/setup/*links do.getOnboardingStepContent's own docstring admitted it: "Placeholder card copy … until then the card links to the existing page where the step can be completed today" — scaffolding from #1867 that was never replaced afterConfirmRosterStep/VerifySocialsStep/FirstTaskStepshipped.Consequence: every setup-flow fix (Spotify typeahead, delete-social, skeleton, valuation reward) currently reaches only email-entry users. This PR is the multiplier on all of them.
What changed
HomePageforwards an incomplete account to/setuprather than hosting a second sequence. The skipped/pinned-checklist path is unchanged./setupopens at the derived step — newSetupEntry+getSetupPathForStep. It previously hard-redirected to/setup/artists, so a signup who had already confirmed their roster was sent back to step 1. Renders a skeleton until every checkpoint source resolves, so a slow read can't flash the wrong step.OnboardingSequence,OnboardingStepCard,getOnboardingStepContent(+ its test).OnboardingCheckpointListreads titles from newgetOnboardingStepTitle(the checklist was the only remaining consumer of the deleted card copy).SetupSkipLinkcarries the gate's "skip for now" escape hatch onto the/setupsurface — without it, forwarding home would turn a deliberately soft gate into a wall. Mounted byRosterSocialsFlowandFirstTaskStep.Deliberately not in scope:
RosterSocialsFlow's local "Step 1 of 2" counter (part 3) and the welcome email's step list (part 4).Verification
TDD, red → green on both new units:
```
getSetupPathForStep — RED (module not found), then
✓ lib/onboarding/tests/getSetupPathForStep.test.ts (3 tests) 4ms
getOnboardingStepTitle — RED (module not found), then
✓ lib/onboarding/tests/getOnboardingStepTitle.test.ts (2 tests) 3ms
```
HomePage.onboarding.test.tsxwas rewritten to assert the new contract (forwards to/setup; renders no competing step surface; skip still yields the pinned checklist and does not forward; Continue re-arms the gate).Full onboarding + home suites:
```
pnpm exec vitest run components/Home lib/onboarding hooks/tests/useOnboardingSessionFlags.test.tsx
Test Files 20 passed (20)
Tests 80 passed (80)
```
pnpm exec tsc --noEmitclean for the touched files. Preview click-through pending.Merge order
After #1887 (no file overlap, so either order is safe, but #1887 removes the funnel regression first). Unblocks #1869 (catalog-is-home).
Tracked in chat#1889 (matrix row 5).
Summary by cubic
Unifies onboarding into a single
/setupflow that opens at the user’s derived step. Home now forwards incomplete accounts to/setup, keeping the gate soft with a skip link (chat#1889).New Features
SetupEntryto resolve the derived step and route viagetSetupPathForStep, showing a short skeleton while loading./setupinstead of hosting its own sequence; skip flags the session and returns to home with the checklist pinned viaSetupSkipLink(mounted inRosterSocialsFlowandFirstTaskStep).OnboardingCheckpointListnow usesgetOnboardingStepTitle; added unit tests for the new utilities and updated Home tests.Refactors
OnboardingSequence,OnboardingStepCard, andgetOnboardingStepContent(and its test)./setupmountsSetupEntryinstead of hard-redirecting to/setup/artists;HomePagereplaces the local sequence with a redirect to/setupfor incomplete accounts.Written for commit 1a62472. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements