feat(onboarding): one progress model across the setup sequence (chat#1889 item 1, part 3)#1891
feat(onboarding): one progress model across the setup sequence (chat#1889 item 1, part 3)#1891sweetmantech wants to merge 2 commits into
Conversation
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>
chat#1889 item 1, part 3. RosterSocialsFlow rendered "Step 1 of 2" from its own local flow state while the pinned checklist counted 4 checkpoints — the same account was told two different things about how much setup was left. FirstTaskStep showed no progress at all. Both the numerator and the denominator now come from ONBOARDING_STEP_IDS via getOnboardingStepPosition, rendered by a shared SetupProgress component, so no surface can invent its own count. FirstTaskStep is step 4 of 4. 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: 50 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 (6)
📒 Files selected for processing (14)
✨ 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 |
![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: 9d28bde40f
ℹ️ 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".
| useEffect(() => { | ||
| if (shouldEnterSetup) { | ||
| router.replace("/setup"); | ||
| } |
There was a problem hiding this comment.
For every authenticated account whose onboarding is incomplete, this effect redirects only after HomePage has rendered NewChatBootstrap. That child immediately runs useProvisionChatSession, whose effect issues non-idempotent session and sandbox POSTs, so visiting home during onboarding creates an orphan chat/session/sandbox before navigation to /setup. Render a non-chat loading state while shouldEnterSetup is true, or otherwise gate bootstrap provisioning until the account is staying on home.
Useful? React with 👍 / 👎.
| const SETUP_PATHS: Record<OnboardingStepId, string> = { | ||
| artists: "/setup/artists", | ||
| socials: "/setup/socials", | ||
| catalog: "/setup/catalog", |
There was a problem hiding this comment.
When catalog is the first unmet checkpoint, this mapping sends the account to /setup/catalog, but that route immediately redirects to the generic /catalogs page. Unlike the roster/social and task setup components, that page mounts neither SetupProgress nor SetupSkipLink; consequently step 3 loses the old always-visible escape hatch, and returning home repeatedly routes the user back here until a catalog exists. Route this checkpoint through a setup surface that retains progress and the session-scoped skip control.
Useful? React with 👍 / 👎.
| 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 load, userData can still be loading while the new Skip button is already clickable. Clicking then calls skip() with accountId === "", storing the flag under the empty-account key before navigating home; once the real account loads, useOnboardingGate reads its account-specific key, sees no skip, and sends the user straight back into setup. Guard the action or hide/disable the control until userData.account_id exists.
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.
7 issues found across 20 files
Confidence score: 3/5
- In
components/Onboarding/SetupEntry.tsx, a failed checkpoint fetch can leave users stuck on the setup skeleton because error and loading are both treated as not-ready; that risks a dead-end onboarding state—separate unrecoverable error from loading and route to app or provide retry/error UI. - In
components/Home/HomePage.tsx, incomplete accounts can briefly mount home/chat before redirect, which may create unintended recoup-api chat sessions and visible UI flash for users—gate chat/home rendering behind a setup-transition state until onboarding eligibility is resolved. - In
components/Onboarding/SetupSkipLink.tsx, skip can fire beforeuserDataresolves and persist under an empty account id, so the real account still gets gated; users may think they skipped but get forced back into setup—disable skip until account id is available and navigate with history replace to avoid Back reopening setup. - In
lib/onboarding/getSetupPathForStep.ts, thecatalogstep appears to route without the same skip escape hatch as other steps, which can trap users in setup now that/redirects incomplete accounts—add a consistent skip path/exit on catalog as well.
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/SetupEntry.tsx">
<violation number="1" location="components/Onboarding/SetupEntry.tsx:24">
P2: A failed checkpoint request leaves `/setup` on this skeleton forever. `isReady` is also false on errors, so distinguish loading from an unrecoverable state and route that state to the normal app (or expose a retry/error UI) instead of returning indefinitely.</violation>
</file>
<file name="components/Onboarding/SetupProgress.tsx">
<violation number="1" location="components/Onboarding/SetupProgress.tsx:13">
P2: Screen readers won't announce the step change when the user advances between roster, socials, catalog, or task. Add `aria-live="polite"` (and optionally `role="status"`) to the `<p>` so assistive tech announces "Step 2 of 4" after the user completes a step — the rest of the page content changes too, so a polite region is enough without interrupting the new content's focus order.</violation>
</file>
<file name="components/Home/HomePage.tsx">
<violation number="1" location="components/Home/HomePage.tsx:28">
P2: When an incomplete account lands on `/`, the app home now flashes briefly before redirecting to `/setup`. The old synchronous conditional return rendered the onboarding sequence inline, so there was no flash. The new `useEffect`-based redirect means the full home UI (chat bootstrap, empty content area) is painted at least once before the post-render effect fires. Consider restoring a synchronous guard — even a minimal loading shell returned early while `isReady` is false would eliminate the flash while keeping the redirect architecture intact.</violation>
<violation number="2" location="components/Home/HomePage.tsx:33">
P2: Incomplete accounts now mount the chat during the redirect window, so they can provision a recoup-api chat session/sandbox even though they are immediately sent to setup. Render a loading/empty setup-transition state while `shouldEnterSetup` is true so `NewChatBootstrap` does not mount.</violation>
</file>
<file name="components/Onboarding/SetupSkipLink.tsx">
<violation number="1" location="components/Onboarding/SetupSkipLink.tsx:22">
P3: After skipping, browser Back reopens the setup step because this adds `/` on top of `/setup` history. Replacing the setup entry keeps Skip for now an exit from the sequence rather than a page users immediately return to.</violation>
<violation number="2" location="components/Onboarding/SetupSkipLink.tsx:28">
P2: Skipping before `userData` resolves does not skip the eventual account: the flag is written under the empty account id, then the real account id re-arms the gate. Disable this action until an account id is available (or guard `handleSkip`) so the escape hatch cannot redirect users into a loop.</violation>
</file>
<file name="lib/onboarding/getSetupPathForStep.ts">
<violation number="1" location="lib/onboarding/getSetupPathForStep.ts:14">
P2: The `catalog` entry routes to `/setup/catalog`, but unlike the roster/socials and task steps (which explicitly mount `SetupSkipLink`), the catalog surface doesn't appear to include the skip escape hatch. Since home now unconditionally forwards incomplete accounts into `/setup`, a user stuck at the catalog step has no way to skip—turning the soft gate into a wall. Either route through a setup wrapper that includes `SetupProgress` + `SetupSkipLink`, or mount those controls on the catalog page directly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const { isReady, step } = useOnboardingState(); | ||
|
|
||
| useEffect(() => { | ||
| if (!isReady) return; |
There was a problem hiding this comment.
P2: A failed checkpoint request leaves /setup on this skeleton forever. isReady is also false on errors, so distinguish loading from an unrecoverable state and route that state to the normal app (or expose a retry/error UI) instead of returning indefinitely.
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 24:
<comment>A failed checkpoint request leaves `/setup` on this skeleton forever. `isReady` is also false on errors, so distinguish loading from an unrecoverable state and route that state to the normal app (or expose a retry/error UI) instead of returning indefinitely.</comment>
<file context>
@@ -0,0 +1,37 @@
+ const { isReady, step } = useOnboardingState();
+
+ useEffect(() => {
+ if (!isReady) return;
+ router.replace(getSetupPathForStep(step));
+ }, [isReady, step, router]);
</file context>
| const SetupProgress = ({ step }: { step: OnboardingStepId }) => { | ||
| const { number, total } = getOnboardingStepPosition(step); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
P2: Screen readers won't announce the step change when the user advances between roster, socials, catalog, or task. Add aria-live="polite" (and optionally role="status") to the <p> so assistive tech announces "Step 2 of 4" after the user completes a step — the rest of the page content changes too, so a polite region is enough without interrupting the new content's focus order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Onboarding/SetupProgress.tsx, line 13:
<comment>Screen readers won't announce the step change when the user advances between roster, socials, catalog, or task. Add `aria-live="polite"` (and optionally `role="status"`) to the `<p>` so assistive tech announces "Step 2 of 4" after the user completes a step — the rest of the page content changes too, so a polite region is enough without interrupting the new content's focus order.</comment>
<file context>
@@ -0,0 +1,20 @@
+const SetupProgress = ({ step }: { step: OnboardingStepId }) => {
+ const { number, total } = getOnboardingStepPosition(step);
+
+ return (
+ <p className="text-xs text-muted-foreground">
+ Step {number} of {total}
</file context>
|
|
||
| useEffect(() => { | ||
| if (shouldEnterSetup) { | ||
| router.replace("/setup"); |
There was a problem hiding this comment.
P2: Incomplete accounts now mount the chat during the redirect window, so they can provision a recoup-api chat session/sandbox even though they are immediately sent to setup. Render a loading/empty setup-transition state while shouldEnterSetup is true so NewChatBootstrap does not mount.
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 now mount the chat during the redirect window, so they can provision a recoup-api chat session/sandbox even though they are immediately sent to setup. Render a loading/empty setup-transition state while `shouldEnterSetup` is true so `NewChatBootstrap` does not mount.</comment>
<file context>
@@ -18,21 +19,20 @@ const HomePage = ({ initialMessages }: { initialMessages?: UIMessage[] }) => {
+
+ useEffect(() => {
+ if (shouldEnterSetup) {
+ router.replace("/setup");
+ }
+ }, [shouldEnterSetup, router]);
</file context>
| return ( | ||
| <button | ||
| type="button" | ||
| onClick={handleSkip} |
There was a problem hiding this comment.
P2: Skipping before userData resolves does not skip the eventual account: the flag is written under the empty account id, then the real account id re-arms the gate. Disable this action until an account id is available (or guard handleSkip) so the escape hatch cannot redirect users into a loop.
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 28:
<comment>Skipping before `userData` resolves does not skip the eventual account: the flag is written under the empty account id, then the real account id re-arms the gate. Disable this action until an account id is available (or guard `handleSkip`) so the escape hatch cannot redirect users into a loop.</comment>
<file context>
@@ -0,0 +1,36 @@
+ return (
+ <button
+ type="button"
+ onClick={handleSkip}
+ className="self-start text-sm text-muted-foreground underline-offset-4 transition-colors hover:text-foreground hover:underline"
+ >
</file context>
| // so a direct signup never saw the interactive steps the welcome email's | ||
| // `/setup/*` links reach. Skip still drops to the app with the checklist | ||
| // pinned (the gate stays soft — see SetupSkipLink). | ||
| const shouldEnterSetup = |
There was a problem hiding this comment.
P2: When an incomplete account lands on /, the app home now flashes briefly before redirecting to /setup. The old synchronous conditional return rendered the onboarding sequence inline, so there was no flash. The new useEffect-based redirect means the full home UI (chat bootstrap, empty content area) is painted at least once before the post-render effect fires. Consider restoring a synchronous guard — even a minimal loading shell returned early while isReady is false would eliminate the flash while keeping the redirect architecture intact.
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 28:
<comment>When an incomplete account lands on `/`, the app home now flashes briefly before redirecting to `/setup`. The old synchronous conditional return rendered the onboarding sequence inline, so there was no flash. The new `useEffect`-based redirect means the full home UI (chat bootstrap, empty content area) is painted at least once before the post-render effect fires. Consider restoring a synchronous guard — even a minimal loading shell returned early while `isReady` is false would eliminate the flash while keeping the redirect architecture intact.</comment>
<file context>
@@ -18,21 +19,20 @@ const HomePage = ({ initialMessages }: { initialMessages?: UIMessage[] }) => {
+ // so a direct signup never saw the interactive steps the welcome email's
+ // `/setup/*` links reach. Skip still drops to the app with the checklist
+ // pinned (the gate stays soft — see SetupSkipLink).
+ const shouldEnterSetup =
+ onboarding.view === "sequence" && onboarding.step !== "complete";
+
</file context>
| const SETUP_PATHS: Record<OnboardingStepId, string> = { | ||
| artists: "/setup/artists", | ||
| socials: "/setup/socials", | ||
| catalog: "/setup/catalog", |
There was a problem hiding this comment.
P2: The catalog entry routes to /setup/catalog, but unlike the roster/socials and task steps (which explicitly mount SetupSkipLink), the catalog surface doesn't appear to include the skip escape hatch. Since home now unconditionally forwards incomplete accounts into /setup, a user stuck at the catalog step has no way to skip—turning the soft gate into a wall. Either route through a setup wrapper that includes SetupProgress + SetupSkipLink, or mount those controls on the catalog page directly.
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` entry routes to `/setup/catalog`, but unlike the roster/socials and task steps (which explicitly mount `SetupSkipLink`), the catalog surface doesn't appear to include the skip escape hatch. Since home now unconditionally forwards incomplete accounts into `/setup`, a user stuck at the catalog step has no way to skip—turning the soft gate into a wall. Either route through a setup wrapper that includes `SetupProgress` + `SetupSkipLink`, or mount those controls on the catalog page directly.</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 handleSkip = () => { | ||
| skip(); | ||
| router.push("/"); |
There was a problem hiding this comment.
P3: After skipping, browser Back reopens the setup step because this adds / on top of /setup history. Replacing the setup entry keeps Skip for now an exit from the sequence rather than a page users immediately return to.
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 22:
<comment>After skipping, browser Back reopens the setup step because this adds `/` on top of `/setup` history. Replacing the setup entry keeps Skip for now an exit from the sequence rather than a page users immediately return to.</comment>
<file context>
@@ -0,0 +1,36 @@
+
+ const handleSkip = () => {
+ skip();
+ router.push("/");
+ };
+
</file context>
1 participant
Part 3 of item 1 in chat#1889. Stacked on #1890 (part 2) — it touches the same two step containers, so it branches off that PR rather than conflicting with it. Review/merge after #1890.
Why
Three progress models disagreed on
main:ONBOARDING_STEP_IDS)RosterSocialsFlowlocaluseStateSame account, three answers to "how much is left."
FirstTaskStepshowed no progress at all, so the last step was unnumbered.What changed
getOnboardingStepPosition— takes both the numerator and the denominator fromONBOARDING_STEP_IDS, so a surface cannot invent its own count.SetupProgresscomponent renders the line;RosterSocialsFlowandFirstTaskStepboth use it.FirstTaskStepis now explicitly step 4 of 4.The local
useState<FlowStep>inRosterSocialsFlowstays — it is the intra-page state machine for roster→socials, which is legitimately local. What is gone is its private denominator.The email's 5-step list is part 4, handled separately in api (and per the 2026-07-26 correction on the issue, by dropping the valuation from the numbered steps rather than adding a 5th checkpoint — a
valuationcheckpoint isn't derivable, sincecatalogspersists no valuation).Verification
TDD, red → green:
```
getOnboardingStepPosition — RED (module not found), then
✓ lib/onboarding/tests/getOnboardingStepPosition.test.ts (3 tests) 5ms
```
New
SetupProgress.test.tsxincludes a regression guard assertingRosterSocialsFlowno longer renders an "of 2" denominator and now reads "Step 1 of 4".```
pnpm exec vitest run components/Onboarding lib/onboarding components/Home
Test Files 21 passed (21)
Tests 83 passed (83)
```
pnpm exec tsc --noEmitclean for the touched files. Preview click-through pending.Tracked in chat#1889 (matrix row 6).
Summary by cubic
Unifies onboarding progress across the
/setupsequence by deriving step number and total fromONBOARDING_STEP_IDS, and routes incomplete accounts into one canonical/setupsurface at their derived step (chat#1889).New Features
getOnboardingStepPositionandSetupProgressso all steps show “Step N of M” from one source.SetupEntryandgetSetupPathForStepto open/setupat the derived step.SetupSkipLinkso “skip for now” works on/setup/*.FirstTaskStepnow shows progress (step 4 of 4).Refactors
/setupinstead of hosting a second sequence.OnboardingSequence,OnboardingStepCard, andgetOnboardingStepContent.OnboardingCheckpointListnow usesgetOnboardingStepTitlefor labels.RosterSocialsFlowusesSetupProgressand drops its private “of 2” count.Written for commit 9d28bde. Summary will update on new commits.