Skip to content

feat(onboarding): pre-auth onboarding with signup as the final save-your-plan step - #400

Merged
thomasluizon merged 5 commits into
mainfrom
issue-396
Jul 6, 2026
Merged

feat(onboarding): pre-auth onboarding with signup as the final save-your-plan step#400
thomasluizon merged 5 commits into
mainfrom
issue-396

Conversation

@thomasluizon

Copy link
Copy Markdown
Owner

Summary

Flips first-run so onboarding runs before auth on both platforms. Depends on the paired API PR thomasluizon/orbit-api#286 — merge/deploy that first.

  • Shared: onboarding draft-store shape, applyOnboarding request/response schemas (habit items narrowed to the fields the apply endpoint actually honors), onboardingApply + importPromptDismiss endpoints, hasSeenImportPrompt profile field, i18n in en + pt-BR.
  • Answers buffer into a persisted draft store; signup is the terminal step; after any successful auth the client flushes unconditionally to the idempotent apply endpoint and clears the store on 2xx. Failed flush retries on next authed mount; the post-auth overlay is suppressed while pending answers exist.
  • Mobile: three Stack.Protected groups (onboarding/auth/app), native splash held until auth + draft hydration resolve. Web: public /onboarding route + proxy carve-out. Step components are mode-blind via OnboardingActionsProvider (buffer pre-auth, live post-auth).
  • One-time post-login "import from another app" prompt gated on hasSeenImportPrompt.

Deliberate deviations

  • Template packs create untagged habits pre-auth — the apply contract carries no per-habit tag field, so the shared schema doesn't advertise one. Preserving pack tags pre-auth is a proposed follow-up (needs an API change).
  • Mobile routing rearchitected to Stack.Protected (replaces the imperative AuthGuard first-run redirect); expo-splash-screen added to hold the splash until auth + draft hydration resolve.

Tests

Shared/web/mobile type-check + lint + unit tests green (shared 1391, web 2118, mobile 940). Manual emulator/browser walkthrough still required (fresh-install flow, process-death resume, sign-out/deep-link/auth-callback) — listed in the report.

Closes #396
API PR: thomasluizon/orbit-api#286

🤖 Generated with Claude Code

…our-plan step

Flips first-run so onboarding runs before auth on both platforms (#396). Depends on
the paired orbit-api PR (branch issue-396) — deploy that first.

- Shared: onboarding draft-store shape, applyOnboarding request/response schemas
  (habit items narrowed to the fields the apply endpoint actually honors), onboardingApply
  + importPromptDismiss endpoints, hasSeenImportPrompt profile field, i18n in en + pt-BR.
- Answers buffer into a persisted draft store; signup is the terminal step; after any
  successful auth the client flushes unconditionally to the idempotent apply endpoint and
  clears the store on 2xx. Failed flush retries on next authed mount; the post-auth overlay
  is suppressed while pending answers exist.
- Mobile: three Stack.Protected groups (onboarding/auth/app), native splash held until auth
  + draft hydration resolve; web: public /onboarding route + proxy carve-out. Step
  components are mode-blind via OnboardingActionsProvider (buffer pre-auth, live post-auth).
- One-time post-login "import from another app" prompt gated on hasSeenImportPrompt.

Template packs create untagged habits pre-auth (the apply contract carries no per-habit
tag field); preserving pack tags pre-auth is a proposed follow-up.

Closes #396

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
orbit-ui-mobile-web Ignored Ignored Jul 6, 2026 5:41pm

Request Review

Addresses PR #400 / #286 review — track the new user-facing surface
(pre-auth onboarding flow, one-time import prompt) in the Auth section.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #400 — feat(onboarding): pre-auth onboarding with signup as the final save-your-plan step

Recommendation: Request changes.

Architecture is sound (the Stack.Protected routing rearchitecture, splash/hydration gate, and shared-schema additions are careful and backward-compatible — no old-mobile-client breaks found). But the "save your plan" messaging that's the entire point of this PR has real gaps, two CTA-label overrides each got wired on only one platform, flush failures are swallowed silently on both platforms, and the mobile draft store isn't scoped per-account.

Critical

1. Mobile onboarding draft can leak across accounts on the same device

  • apps/mobile/hooks/use-onboarding-flush.ts:23 gates flush only on isAuthenticated && hasHydrated && pendingAnswers — no identity check. apps/mobile/stores/auth-store.ts login() resets every other piece of per-account state (queryClient.clear(), setQueryCacheScope, offlineQueue.clear(), clearOfflineState(), chat messages, review-reminder scope) but never touches useOnboardingDraftStore; logout() likewise never clears it (verified: no onboarding reference anywhere in auth-store.ts).
  • If a flush fails (offline, server error, app killed mid-flight) the buffered draft (habits, goal, color scheme, first log) survives logout. If a different user then logs into the same device, that stale draft flushes onto their account on the next authenticated mount with no confirmation.
  • fix: clear useOnboardingDraftStore in logout() (mirroring the other per-account resets), and/or tag the draft with the anonymous session that produced it and refuse to flush on identity mismatch.

2. Flush failures are silently swallowed on both platforms — permanent stuck state, no diagnostics

  • apps/mobile/hooks/use-onboarding-flush.ts:42 (catch {}, confirmed) and apps/web/hooks/use-onboarding-flush.ts:34 (.catch(() => { void 0 })) drop every apply failure with no logging and no distinction between transient and permanent errors. The draft only clears on success, so a permanently-rejected draft (schema drift, stale duplicate habit name) leaves the user authenticated with no habits, onboarding UI suppressed (web's suppressOnboardingOverlay stays true), and zero indication anything is wrong. Retry only happens on a fresh mount of the root layout, not an in-session retry.
  • fix: log/report the error at minimum (Sentry or existing logger); after N failures, stop suppressing the onboarding overlay and/or surface a "some answers didn't save" notice.
  • reference: root CLAUDE.md rule 8 ("never swallow errors silently").

High

3. "Complete" step tells pre-auth users their data is "already saved" when it's only a local draft

  • apps/web/components/onboarding/onboarding-complete.tsx:106 / apps/mobile/components/onboarding/onboarding-complete.tsx:168, packages/shared/src/i18n/en.json:1640complete.title/complete.subtitle ("You're all set! ... already saved.") render unconditionally, including in the pre-auth buffered flow, where nothing is saved server-side yet. On web this directly contradicts the CTA underneath it, which now reads "Create account & save."
  • fix: gate on useOnboardingIsLive() and show save-your-plan copy pre-auth, mirroring the finishLabel pattern.

4. Two CTA-label overrides each shipped on only one platform (mirror-image gap)

  • Mobile's EmailStep (apps/mobile/app/email-step.tsx:24,41,68) takes sendCodeLabel → "Create account & save" pre-auth; web's apps/web/app/(auth)/login/email-step.tsx:60 was never touched and still hardcodes t('auth.sendCode') (confirmed).
  • Web's onboarding-complete.tsx gained finishLabel?: string (lines 18/26/156); mobile's equivalent component/caller never got it — always shows "Start using Orbit" (confirmed, no finishLabel in mobile file).
  • fix: wire the missing override on each platform so both directions are covered.

5. Web's "save your plan" subtitle can show "0 habits ready to go"

  • apps/web/app/(auth)/login/login-sections.tsx:29-33 falls to habitSummary({count: 0}) when pendingHabitCount === 0, trivially reachable via the flow's own Skip affordance. Mobile (apps/mobile/app/login-sections.tsx:85-96) always shows the generic subtitle and only adds the habit-count line when count > 0.
  • fix: match mobile's structure.

6. AstraImportPrompt (mobile) doesn't suppress while a flush is pending

  • apps/mobile/components/onboarding/astra-import-prompt.tsx shouldShow never reads useOnboardingDraftStore().hasPendingAnswers() (confirmed absent), unlike the equivalent suppression wired for OnboardingFlow in _layout.tsx. A returning user whose account already has hasCompletedOnboarding: true can see the import sheet pop up independent of whether their just-buffered answers have actually flushed.
  • fix: add the same hasPendingAnswers() gate used elsewhere.

7. Template-pack creation lost its bulk-create atomicity

  • apps/web/components/onboarding/onboarding-template-packs.tsx:57-72 (and mobile's equivalent) replaced one atomic bulk-create call with a for loop of individually-awaited createHabit calls. In live (post-auth) mode, a mid-loop failure leaves already-created habits with no rollback; retrying re-creates duplicates for the ones that already succeeded.
  • fix: use the bulk endpoint, or track per-item success and skip already-created items on retry.

8. Dead export: useOnboardingDraftHydrated unused in apps/mobile

  • apps/mobile/stores/onboarding-draft-store.ts:41-43 is exported but has zero call sites (confirmed via grep); both consumers inline useOnboardingDraftStore((s) => s._hasHydrated) instead. Web's identical export IS used by its two call sites.
  • fix: delete the export, or use it in place of the inline selectors for parity with web.
  • reference: CLAUDE.md rule 2 ("delete unused code immediately").

9. FEATURES.md not updated for a funnel-level feature change

  • No onboarding row reflects this signup-funnel restructuring or the new hasSeenImportPrompt gate.

Medium

10. isRecord triplicated across packages/shared/src/stores/

  • Confirmed identical in onboarding-draft.ts:23, ui-store.ts:38, and engagement-prompt-store.ts:34 — third copy crosses this repo's own extraction threshold (rule 6: "extract on the third real use, not the second").
  • fix: move to a shared utils/type-guards.ts and import in all three.

11. migrateOnboardingDraft casts persisted storage without Zod validation at a trust boundary

  • packages/shared/src/stores/onboarding-draft.ts:66-96 only checks "is a non-null object" via isRecord, then force-casts to ApplyOnboardingHabit[] etc., with no applyOnboardingHabitSchema.parse. A malformed persisted entry (corrupted storage, stale shape from a prior version) silently becomes a well-typed payload and flows to the apply endpoint; since the draft only clears on 2xx, a permanently malformed entry could fail the flush on every launch.
  • fix: safeParse each recovered item and drop failures rather than blind-casting.

12. Web re-implements hasPendingAnswers instead of calling the shared store method

  • apps/web/stores/onboarding-draft-store.ts:52-63 duplicates the six-condition expression inline; the shared store already exposes state.hasPendingAnswers(), and mobile calls it correctly. Risk of silent drift if a new draft field is added.

13. onboarding-flow.tsx reinvents "is this pre-auth?" on both platforms instead of using useOnboardingIsLive()

  • Web: actions.onImport === undefined (onboarding-flow.tsx:39); Mobile: !isAuthenticated (lines 262/481). Both happen to be correct today only incidentally; onboarding-complete.tsx in this same diff already uses the purpose-built hook correctly.

14. New import-prompt branching has zero test coverage

  • apps/web/app/(app)/layout.tsx:131-212 (showImportPrompt/suppressOnboardingOverlay state machine) and retained-onboarding-overlay.tsx have no tests — this is the exact mechanism Critical #2 depends on.

15. Banned em dash in new string

  • packages/shared/src/i18n/en.json:1657 (onboarding.wizard.importDescription); pt-BR translation already avoids it.

Not verifiable in this CI run

  • orbit-api DTO side of ApplyOnboardingRequest/Response and whether hasSeenImportPrompt is actually returned by the deployed API yet (companion PR presumed in orbit-api, sibling repo not checked out here).
  • Whether the apply endpoint enforces Pro entitlements server-side — mobile's pre-auth provider hardcodes hasProAccess: true, so a free-tier signup could buffer Pro-gated answers before the server knows the plan.

What's good

Clean mode-blind actions abstraction; well-scoped, backward-compatible shared schema additions (verified field-by-field against createHabitRequestSchema); correct splash/hydration race handling on both platforms; i18n key parity confirmed exact across en.json/pt-BR.json; minimal, correctly-scoped proxy.ts carve-out for /onboarding; solid new-hook test coverage elsewhere (success/failure/edge cases).

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #400 — pre-auth onboarding with signup as the final "save your plan" step

Recommendation: Request changes

This flips onboarding to run before auth on both platforms, buffering answers in a shared Zustand draft store (packages/shared/src/stores/onboarding-draft.ts) and flushing them to a new idempotent POST /api/profile/onboarding/apply after any successful auth. The packages/shared contract changes are cleanly additive (no backward-compat break for shipped mobile clients), i18n is in sync in both locales, and the mode-blind OnboardingActionsProvider abstraction is a genuinely good shared design across web/mobile. Several findings below were independently confirmed by reading the actual code and need to be addressed before merge.

High-signal findings

1. [High] Buffered onboarding draft can silently apply onto the wrong account

  • Where: apps/mobile/hooks/use-onboarding-flush.ts:23-24, apps/web/hooks/use-onboarding-flush.ts:26-27; apps/mobile/stores/auth-store.ts login()/logout() (~lines 269-322, 324-351); apps/web/stores/auth-store.ts logout() (~lines 73-84)
  • Issue: The flush effect fires whenever hydrated && hasPendingAnswers() is true, on any successful auth transition — there is no check against profile.hasCompletedOnboarding and no identity binding to the draft. login() on mobile deliberately resets query cache, offline queue, chat store, and review-reminder scope precisely to avoid cross-account bleed from a prior session, but never resets useOnboardingDraftStore; neither platform's logout() clears it either.
  • Reachable path: start pre-auth onboarding as a guest, buffer a habit/goal/preference, then tap "I already have an account" and log into a different, pre-existing account (same passwordless email/code flow serves both signup and login) — the buffered answers are POSTed onto that unrelated, already-onboarded account with no confirmation.
  • Fix: gate the flush on !profile?.hasCompletedOnboarding in addition to hasPendingAnswers, and explicitly reset the onboarding draft store on logout() (and ideally on any login() that isn't the one immediately following finishOnboarding) on both platforms.

2. [High] Pre-auth color-scheme picker is dead on arrival on mobile

  • Where: apps/mobile/components/onboarding/onboarding-welcome.tsx:7,30,115
  • Issue: Every sibling onboarding step reads Pro-gating through the mode-blind useOnboardingHasProAccess() context hook (hardcoded true pre-auth in apps/mobile/app/(onboarding)/index.tsx:11). This file wasn't migrated — it still calls the old useHasProAccess() (apps/mobile/hooks/use-profile.ts:65-68), which depends on useProfile() gated by enabled: isAuthenticated. Pre-auth, isAuthenticated is false, so hasProAccess is always false and the {hasProAccess && (...)} color-scheme swatch block never renders.
  • Impact: One of the onboarding steps this PR explicitly introduces (color-scheme selection) is silently non-functional pre-auth on mobile, despite the draft store and apply payload fully supporting it. Also a parity gap — web threads hasProAccess correctly as a prop from the same context.
  • Fix: swap to useOnboardingHasProAccess() from ./onboarding-actions-context, matching every other migrated step.

3. [High] Template-pack creation drops habit tags and loses bulk-create atomicity (both platforms)

  • Where: apps/mobile/components/onboarding/onboarding-template-packs.tsx:67-90, apps/web/components/onboarding/onboarding-template-packs.tsx:59-83; compare packages/shared/src/utils/template-packs.ts buildBulkItemsFromPack (still returns tags)
  • Issue: Both platforms replaced one atomic useBulkCreateHabits({habits: items}) call with a for...of loop of sequential actions.createHabit(item) calls, and the per-item payload built from buildBulkItemsFromPack's output omits the tags field entirely. The PR's documented deviation only justifies dropping tags for the pre-auth apply-endpoint path (no tag field there), but this same loop also runs in the post-auth "retained overlay" live path, where the real single-habit endpoint does support tags — an undocumented regression there. The sequential loop also has no partial-failure handling: a mid-loop error leaves earlier habits already created, and a retry re-creates them (duplicates).
  • Fix: keep tags for the live/post-auth path (resolve/pass tagIds), and either restore a bulk-create path for live mode or track per-item success so retries don't duplicate.

4. [High] Web "save your plan" login header can render "0 habits ready to go" and drops the reassurance subtitle

  • Where: apps/web/app/(auth)/login/login-sections.tsx:29-33; compare apps/mobile/app/login-sections.tsx:85-98
  • Issue: emailSubtitle unconditionally renders t('onboarding.flow.saveYourPlan.habitSummary', {count: pendingHabitCount}) whenever count ≠ 1 — including 0 — which literally interpolates to "0 habits ready to go" (reachable via handleSkip in the onboarding flow, which lets a user skip past habit creation before hitting signup). Web also never references saveYourPlan.subtitle anywhere in the codebase, while mobile always renders it and only conditionally appends the habit-count line when count > 0.
  • Fix: guard the habit-count line behind pendingHabitCount > 0 and always render saveYourPlan.subtitle, mirroring mobile.

Medium

Web OnboardingWelcome no longer sources the displayed week-start-day from the live profile or draftapps/web/components/onboarding/onboarding-welcome.tsx:27 hardcodes useState<OnboardingWeekStartDay>(1) with no sync from profile?.weekStartDay (post-auth retained overlay) or the draft store (pre-auth back-navigation), unlike mobile's profile?.weekStartDay ?? draftWeekStartDay ?? 1. Cosmetic (nothing is written unless the user interacts with a chip) but a visible regression for returning users with a non-default week start.

Verified as clean

  • packages/shared contract changes are additive only (two new endpoints, two new schemas, one new profile field, one new enum value) — no backward-compat break for old mobile clients.
  • i18n: all new keys present in both en.json and pt-BR.json.
  • Stack.Protected routing rearchitecture on mobile and the public /onboarding proxy carve-out on web are structurally sound; no any, console.log, or narration-comment violations found in the diff.

Not verifiable in this job

  • Cross-repo contract alignment and the orbit-api side of the backward-compat guard — orbit-api is not checked out in this CI job. This includes whether POST /api/profile/onboarding/apply independently re-enforces Pro-tier gating server-side (pre-auth onboarding hardcodes hasProAccess={true} on both platforms since no account exists yet — worth confirming in the paired API PR #286 review).

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #400 — pre-auth onboarding with signup as the final save-your-plan step

Recommendation: REQUEST CHANGES

Context: Two prior automated reviews on this PR (2026-07-05 05:37 and 06:02 UTC) already requested changes. Only a merge of main (unrelated .claude/PRDs / .claude/stories tracking docs) has landed since — git diff b5dfebfe 74114a04 --stat touches zero onboarding source files. This review independently re-verified every carried-forward Critical/High claim against current source rather than re-quoting prior reports; all but one (a stale FEATURES.md nit, now resolved) still hold.

Severity Count
Critical 2
High 7
Medium 7

Critical

1. Onboarding draft can apply onto the wrong account.
apps/mobile/hooks/use-onboarding-flush.ts:23-24, apps/web/hooks/use-onboarding-flush.ts:26-27; apps/mobile/stores/auth-store.ts, apps/web/stores/auth-store.ts.
The flush effect fires on any successful auth transition once hydrated + pending answers exist — with no check against profile.hasCompletedOnboarding and no identity binding. grep "onboarding" on both auth stores returns zero matches: login()/logout() reset the query cache, offline queue, chat store, and review-reminder scope specifically to avoid cross-account bleed, but never touch useOnboardingDraftStore. Since the same passwordless flow serves signup and login ("I already have an account"), a user who buffers answers pre-auth and then logs into an unrelated existing account will have the buffered draft silently POSTed onto that account.
Fix: gate the flush on !profile?.hasCompletedOnboarding as well, and reset the onboarding draft store in logout() on both platforms.

2. Flush failures are silently swallowed on both platforms.
apps/mobile/hooks/use-onboarding-flush.ts:42 (empty catch {}), apps/web/hooks/use-onboarding-flush.ts:41-43 (.catch(() => { void 0 })).
Every apply failure is dropped with no logging and no transient/permanent distinction. The draft only clears on success, so a permanently-rejected draft leaves a newly authenticated user with no habits and no indication anything went wrong. Violates root CLAUDE.md rule 8 ("never swallow errors silently").
Fix: log the failure at minimum; after N retries stop suppressing the onboarding overlay and/or surface a "some answers didn't save" notice.

High

3. "Complete" step claims data is "already saved" for pre-auth (draft-only) users. onboarding-complete.tsx (both platforms) renders "Everything is real... already saved" (en.json:1639-1640) unconditionally, never gated on the already-imported useOnboardingIsLive(). Contradicts the "Create account & save" CTA on the same screen.

4. Two CTA-label overrides shipped on only one platform. Mobile's email-step.tsx/login.tsx add a sendCodeLabel override; web's email-step.tsx still hardcodes t('auth.sendCode'). Inversely, web's onboarding-complete.tsx adds a finishLabel override that mobile never references — mobile always shows "Start using Orbit".

5. Web's save-your-plan header can render "0 habits ready to go." apps/web/app/(auth)/login/login-sections.tsx:29-33 shows habitSummary({count: 0}) whenever count ≠ 1, and never references saveYourPlan.subtitle at all; mobile always shows the subtitle and only conditionally appends the count line.

6. Pre-auth color-scheme picker is dead on arrival on mobile. apps/mobile/components/onboarding/onboarding-welcome.tsx:7,30 still calls useHasProAccess() (gated on isAuthenticated, always false pre-auth) instead of the mode-blind useOnboardingHasProAccess() every sibling step uses. The swatch block never renders pre-auth despite the draft/apply payload supporting it.

7. Template-pack creation lost bulk atomicity and drops tags even in the live post-auth path. Both platforms' onboarding-template-packs.tsx replaced an atomic bulk-create with a sequential for...of loop of individual createHabit() calls carrying no tags, even though CreateHabitRequest supports tags and the live endpoint honors them. The documented deviation only justifies dropping tags pre-auth (no per-habit tag field on the apply contract), not in the live retained-overlay path. No partial-failure handling — a mid-loop error leaves partial habits with duplicate-on-retry risk.

8. AstraImportPrompt doesn't suppress while a flush is pending. apps/mobile/components/onboarding/astra-import-prompt.tsx:40-51shouldShow never checks hasPendingAnswers(), unlike the equivalent gate wired for OnboardingFlow in _layout.tsx.

9. Dead export useOnboardingDraftHydrated in apps/mobile. apps/mobile/stores/onboarding-draft-store.ts:41-43 has zero call sites; both consumers inline the selector instead. Web's identical export is actually used.

Medium

  1. isRecord is triplicated byte-identical across onboarding-draft.ts:23, ui-store.ts:38, engagement-prompt-store.ts:34 — extract to a shared type-guard util (third use).
  2. migrateOnboardingDraft (packages/shared/src/stores/onboarding-draft.ts:66-96) force-casts persisted storage after only an isRecord check, no Zod safeParse at this trust boundary.
  3. Web reimplements the 6-condition hasPendingAnswers inline (apps/web/stores/onboarding-draft-store.ts:53-63) instead of calling the shared store's state.hasPendingAnswers() (present at onboarding-draft.ts:142).
  4. onboarding-flow.tsx reinvents "is this pre-auth?" on both platforms (actions.onImport === undefined / !isAuthenticated) instead of the purpose-built useOnboardingIsLive() already used elsewhere in this diff.
  5. The new import-prompt state machine (apps/web/app/(app)/layout.tsx:131-212, retained-onboarding-overlay.tsx) has zero test coverage — the exact mechanism Critical #1/#2 depend on.
  6. Web OnboardingWelcome hardcodes useState<OnboardingWeekStartDay>(1) (onboarding-welcome.tsx:27) with no sync from profile?.weekStartDay/draft, unlike mobile's profile?.weekStartDay ?? draftWeekStartDay ?? 1.
  7. Banned em dash in en.json:1657 (onboarding.wizard.importDescription) — pt-BR already avoids it.

What's good

The mode-blind OnboardingActionsProvider abstraction is well-designed and reused correctly everywhere except finding #6. Stack.Protected routing rearchitecture and splash/hydration race handling are sound on both platforms. packages/shared additions are cleanly additive (no backward-compat break — confirmed). i18n key parity is exact. Solid new test coverage for the flush hook and draft-store persistence/migration paths.

Deferred (not verifiable in this CI job)

  • Contract drift on the orbit-api side, and server-side Pro-tier enforcement of the apply endpoint: sibling repo not checked out in this job; depends on paired PR thomasluizon/orbit-api#286.
  • Validation (lint/typecheck/tests): skipped per CI setup — Build / Unit Tests / SonarCloud run as separate required checks.

Recommendation

REQUEST CHANGES. Fix Critical #1 (cross-account draft leak) and #2 (silent flush-failure swallow) before merge — these can corrupt another user's account data or strand a signup with no habits and no error surfaced. The High findings are real UX/parity/dead-code gaps but not data-unsafe on their own.

const runningRef = useRef(false)

useEffect(() => {
if (!isAuthenticated || !hasHydrated || !pendingAnswers) return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical — cross-account draft leak. This flush fires on any successful auth transition (hydrated + pending answers), with no check against profile.hasCompletedOnboarding and no identity binding. Neither apps/mobile/stores/auth-store.ts nor apps/web/stores/auth-store.ts reset the onboarding draft store in login()/logout() (unlike the query cache, offline queue, chat store, and review-reminder scope, which all reset there specifically to avoid cross-account bleed). Since the same passwordless flow serves signup and login, a user who buffers answers pre-auth and then logs into an unrelated existing account will silently POST the buffered draft onto that account.

Fix: gate the flush on !profile?.hasCompletedOnboarding too, and reset useOnboardingDraftStore in logout() on both platforms.

queryClient.invalidateQueries({ queryKey: goalKeys.all })
queryClient.invalidateQueries({ queryKey: gamificationKeys.all })
queryClient.invalidateQueries({ queryKey: profileKeys.all })
} catch {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical — silent error swallow. This empty catch {} (and the equivalent .catch(() => { void 0 }) in apps/web/hooks/use-onboarding-flush.ts:41-43) drops every apply failure with no logging and no transient/permanent distinction. Since the draft only clears on success, a permanently-rejected draft leaves a newly authenticated user with no habits and zero indication anything went wrong. Violates root CLAUDE.md rule 8 ("never swallow errors silently").

Fix: log the failure at minimum; after N retries stop suppressing the onboarding overlay and/or surface a "some answers didn't save" notice.

…ath tags, dead-code + parity

- Clear onboarding draft on logout (both auth stores) and gate the flush on
  an un-onboarded profile so a guest's buffered answers can't post onto a
  different account
- Report flush failures via the existing Sentry path instead of swallowing;
  draft still only clears on success
- Mobile welcome color-scheme + complete-step copy now read onboarding
  pro-access / live mode so they render correctly pre-auth
- CTA-label parity: web EmailStep gains the "Create account & save" override;
  mobile OnboardingComplete gains finishLabel; both wired
- Web LoginHeader mirrors mobile: always show saveYourPlan.subtitle, gate the
  habit-count line on pendingHabitCount > 0
- Restore atomic bulk habit-create WITH tags in the live template-pack path via
  a shared createHabitsBulk action; pre-auth still buffers (tags dropped)
- Suppress import prompts while an onboarding flush is pending (both platforms)
- Use useOnboardingDraftHydrated / useOnboardingIsLive instead of inline
  selectors; dedupe web hasPendingAnswers to the shared store method
- Validate persisted draft items with Zod in migrateOnboardingDraft
- Extract shared isRecord util; remove em dash from importDescription copy
- Extract testable onboarding-overlay state machine + unit tests

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #400 — feat(onboarding): pre-auth onboarding with signup as the final save-your-plan step

Recommendation: APPROVE

Reviewed at the latest commit (5fc8a500 fix(onboarding): address PR #400 review — cross-account guard, live-path tags, dead-code + parity), a fix-round landed in response to three prior automated reviews on this PR (2026-07-05 05:37, 06:02, 17:24 UTC), all of which requested changes. This pass independently re-verified every Critical/High/Medium finding from those reviews against current source (direct code read, not just diff shape) rather than re-quoting them.

Critical — none (both fixed, verified)

  • Cross-account draft leak: apps/mobile/hooks/use-onboarding-flush.ts:30-36 / apps/web/hooks/use-onboarding-flush.ts:20-21 now gate the flush on !profile.hasCompletedOnboarding in addition to hasPendingAnswers; apps/mobile/stores/auth-store.ts:133,353 / apps/web/stores/auth-store.ts:81 now reset useOnboardingDraftStore on logout, mirroring the other per-account resets. New tests cover the gate on both platforms.
  • Silent flush-failure swallow: mobile now calls captureError(error), web calls Sentry.captureException(error); both asserted in updated tests.

High — none (all fixed, verified)

  • "Already saved" copy now gated on isLive in onboarding-complete.tsx (both platforms).
  • The two mirror-image CTA-label gaps are closed: web's email-step.tsx/login/page.tsx now wire sendCodeLabel; mobile's onboarding-complete.tsx/onboarding-flow.tsx now wire finishLabel.
  • Web's "0 habits ready to go" fixed — saveYourPlan.subtitle always renders, habit-count line only when > 0, matching mobile.
  • Mobile's dead pre-auth color-scheme picker now uses useOnboardingHasProAccess() (mode-blind context) instead of the auth-gated useHasProAccess().
  • Template-pack bulk-atomicity/tag-drop fixed on both platforms via a new createHabitsBulk action — live path uses the real bulk-create mutation with tags preserved; pre-auth buffer path still intentionally omits tags per the documented deviation.
  • AstraImportPrompt now suppresses while hasPendingAnswers() is true.
  • The previously-dead useOnboardingDraftHydrated export is now actually consumed in providers.tsx and use-onboarding-flush.ts.

Medium

  1. Web OnboardingWelcome still doesn't sync week-start-day from profile/draft (carried forward, not fixed) — apps/web/components/onboarding/onboarding-welcome.tsx:27 still hardcodes useState<OnboardingWeekStartDay>(1), unlike mobile's profile?.weekStartDay ?? draftWeekStartDay ?? 1. Cosmetic (nothing persists until interaction) but a visible regression for returning users with a non-default week start on web only. Non-blocking.

Low / Info

  1. Mobile has no dedicated component-level test for onboarding-complete.tsx/onboarding-complete-habit.tsx — a gap that predates this PR (web has had these test files longer than mobile has existed in its current form). This PR changed onboarding-complete.tsx behavior on both platforms but only web's test exercises the new copy-swap directly. Not a regression from this round; flagging for a follow-up.

Subagents

Agent Verdict
parity-checker PAIRED — every logic change in the fix commit landed on both platforms
i18n-syncer IN SYNC — full key parity for the touched onboarding.* keys in en.json/pt-BR.json
contract-aligner N/A — sibling orbit-api repo not checked out; fix commit touches no packages/shared/src/types/*/endpoints.ts (those only changed in the original PR commit, already additive-only per three prior reviews, re-confirmed here)
security-reviewer N/A — no orbit-api files in this diff

Deferred

  • Full DTO-level contract cross-check against orbit-api (sibling repo not available in this session) — depends on paired API PR thomasluizon/orbit-api#286 being merged/deployed first, as the PR body states as a hard dependency.
  • Server-side Pro-tier enforcement of the apply endpoint (both platforms hardcode hasProAccess: true pre-auth) — not verifiable from this repo alone.
  • Lint/typecheck/tests not re-run in this session (sandbox restrictions); not treated as blocking since required CI checks (Build/Unit Tests/SonarCloud) gate merge separately.

What's good

The fix commit is a clean, complete response to every prior Critical/High — each verified by direct code read. Error reporting now goes through each platform's actual logging tool rather than a generic swallow. The new apps/web/app/(app)/onboarding-overlay-state.ts extraction turns a previously-untested inline state machine into pure, well-tested functions. i18n stayed in sync throughout, and the mode-blind OnboardingActionsProvider abstraction remains solid across every fix.

Recommendation: APPROVE. The remaining Medium and Low items are non-blocking, pre-existing-shaped gaps rather than defects introduced by this round.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #400 — feat(onboarding): pre-auth onboarding with signup as the final save-your-plan step

Scope: Diff since the last review round (commit 5fc8a500, already APPROVED by a prior automated review after three CHANGES_REQUESTED rounds).

Recommendation: APPROVE

Summary

The branch head advanced from 5fc8a500 to f7933ce0 via exactly two commits: 89c07cda (an unrelated, already-separately-merged main commit, "achievements XP row collision + queued completions never counted toward review floor", #403) and f7933ce0 itself, the routine "merge main into issue-396" that pulled it in. git log 5fc8a500..f7933ce0 --oneline confirms these are the only two commits in that range — no new commit authored for issue-396/PR #400 landed after the prior APPROVE.

The merge is clean both mechanically (no conflict markers) and semantically. Of the 15 files 89c07cda touches, only 4 overlap with PR #400's file set (apps/mobile/app/_layout.tsx, packages/shared/src/i18n/en.json, packages/shared/src/i18n/pt-BR.json, packages/shared/src/utils/index.ts), and in every case the merge is a clean superset with no interference:

  • apps/mobile/app/_layout.tsx: the incoming change removes a showSharedCelebrations pathname gate (unrelated celebration-visibility fix). The onboarding-specific gating in the same file — !profile.hasCompletedOnboarding && !pendingOnboardingAnswers around OnboardingActionsProvider/OnboardingFlow, and hasCompletedOnboarding gating the celebration block — is untouched and intact.
  • en.json / pt-BR.json: purely additive new keys (chat.*.capability.*, errors.*) plus an unrelated achievements-copy edit; both locales stay in lockstep; no collision with onboarding's onboarding.* keys.
  • packages/shared/src/utils/index.ts: additive export only; the onboarding PR's own isRecord export from is-record.ts is confirmed still present post-merge.

No auth-store, draft-store, flush-hook, or contract/type files from the onboarding PR were touched by this range — the Critical fixes from the last review round (cross-account draft-leak guard, flush-failure error reporting) are unaffected.

Findings

  • Critical — None.
  • High — None.
  • Medium — None new. (The previously-flagged Medium — web OnboardingWelcome not syncing week-start-day from profile/draft, apps/web/components/onboarding/onboarding-welcome.tsx:27 — was already surfaced in the prior review round and is unchanged in this diff range; not re-flagged per instruction to avoid re-flagging already-addressed/carried concerns.)
  • Low / Info — None new. (Previously-flagged Low — missing mobile component-level test for onboarding-complete.tsx — likewise unchanged, not re-flagged.)

Subagents

Agent Verdict
parity-checker N/A — no apps/web/**/apps/mobile/** PR-400 logic changed in this range
i18n-syncer IN SYNC — the incoming keys are additive and identical in shape across en/pt-BR
contract-aligner Not verifiable in CI (sibling orbit-api repo not checked out); no packages/shared/src/types/*/endpoints.ts touched in this range anyway
security-reviewer N/A — no orbit-api files in this range

Validation

Skipped per CI setup — Build / Unit Tests / SonarCloud run as separate required checks on this PR.

Deferred

  • Full DTO-level contract cross-check against orbit-api — not verifiable in CI (sibling repo not checked out); unchanged from the prior review's deferral, still pending API PR orbit-api#286 merge/deploy before this PR ships.
  • The carried-forward Medium (web week-start-day sync) and Low (mobile onboarding-complete test gap) from the prior round remain open items but are outside the diff-since-last-round scope of this pass.

What's good

The branch stayed current with main via a clean merge that introduced zero risk to the onboarding surface — no conflicts, no accidental reversion of the cross-account guard or flush-error-reporting fixes from 5fc8a500, and i18n parity held through an unrelated locale-file change touching the same files.

Recommendation

APPROVE — no new Critical/High/actionable-Medium findings in the diff since the last review round. The prior APPROVE stands; nothing in this range changes that verdict.

@sonarqubecloud

sonarqubecloud Bot commented Jul 6, 2026

Copy link
Copy Markdown

@thomasluizon
thomasluizon merged commit 0645130 into main Jul 6, 2026
10 checks passed
@thomasluizon
thomasluizon deleted the issue-396 branch July 6, 2026 17:50
thomasluizon added a commit that referenced this pull request Jul 9, 2026
…ng screen) (#432)

* fix(mobile): stop trapping users on /privacy after login (#431)

The root <Stack> declared the public screens (privacy/terms/r/auth-callback)
before the Stack.Protected groups and set no initialRouteName, so Expo Router
used `privacy` as the stack anchor. On login the guards flip, the `login`
screen is removed out from under the user, and the navigator falls back to the
anchor -> /privacy, where the bottom nav is hidden and back cannot escape.

Declare the guarded groups first and move the public screens last so the anchor
resolves to the first AVAILABLE screen for the current auth/onboarding state:
authenticated -> (tabs), unauthenticated + onboarding pending -> (onboarding),
unauthenticated + onboarding done -> login. A WHY comment guards against the
reorder silently regressing again (regression from #400).

Mobile-only: apps/web is Next.js App Router with no Stack.Protected/anchor
concept and does not have this bug.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mobile): route logout and forced-logout to /login (#431)

After the screen reorder, a signed-out user anchors to the first available
screen. That is correct for login (lands on tabs), but on logout the teardown
also resets onboardingLocallyDone, so the anchor resolves to (onboarding) and
drops the user into the new-user flow instead of /login. The anchor cannot tell
a signed-out user from a first-run user, and onboardingLocallyDone is load-bearing
for the login-screen variant and the onboarding flush, so it cannot be repurposed.

Navigate explicitly to /login after teardown, at the component layer via a new
useLogout hook (profile, session-expiry warning, account deletion) and after the
forced 401 teardown in api-client. Navigation stays OUT of the auth store: an
imperative nav during store teardown caused a grey-screen crash (#170), and the
existing "logout does not imperatively navigate" store test remains green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mobile): navigate to /login even if logout teardown throws

Wrap the useLogout hook's teardown in try/finally so the redirect to /login
runs regardless of a rejection during session teardown, preventing a signed-out
user from being stranded on the onboarding anchor. Mirrors web, which already
hard-redirects to /login after logout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(mobile): cover forced-logout redirect on post-refresh 401

Add the missing api-client case where a refresh succeeds but the retried
request still returns 401: the session is torn down and the user is redirected
to /login. Covers the one previously-uncovered new line (the first teardown
branch's redirect), bringing this change to full new-code coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(mobile): redirect to /login on foreground session loss + cover throw-safety

Addresses code-review findings on the post-auth navigation fix.

The AppState 'active' handler in providers.tsx calls checkAuth() on every
foreground; a genuine refresh failure there clears the session (and resets
onboardingLocallyDone) inside the store with no redirect, so a user whose
refresh token expired while backgrounded is dropped into the onboarding anchor
instead of /login. This is the same root cause as the login and logout traps,
reached through ordinary session expiry.

Extract reconcileSessionOnForeground(): it re-validates the session and, if it
transitioned from authenticated to signed-out, routes to /login via the router
singleton (navigation stays out of the auth store, per #170). Also add the
missing test that logout teardown throwing still routes to /login.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Onboarding before auth: pre-auth flow with signup as the final "save your plan" step

1 participant