refactor(web): model pre-chat onboarding as a declarative, capability-driven step list#33068
Conversation
…-mode special-case Replace the blanket `if (localMode) finish()` after tool selection with a per-screen capability gate. The prior-assistants step now gates on `canOfferPriorAssistants` (true outside local mode, or when a platform session exists), mirroring the existing `canOfferGoogleStep` / `showIOSAppStep` self-gates. Local mode falls out of the funnel through the capabilities rather than being hardcoded, so one build serves both local and managed modes without surface special-casing. Also corrects the stale recipe comment: the onboarding recipe is platform-only marketing-funnel data (resolved on the platform from a UTM campaign cookie / stored attribution). Local and native runtimes have no marketing cohort, so a null recipe is the correct, complete behavior — there is no recipe endpoint to fetch or build. Closes LUM-2000
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3438b763f
ℹ️ 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".
Replace the imperative, numeric-index navigation in the pre-chat onboarding flow with an ordered, capability-derived step list. The set of reachable steps is now a pure function of capabilities (local mode, funnel variant, connected tool, platform); each scenario is the resolved list rather than logic scattered across per-screen handlers. - Add prechat-steps.ts: pure resolveWebSteps/resolveNativeSteps plus nextStep/prevStep over enabled steps. Navigation keys off step ids, never indices, and back always lands on the previous enabled step. - Add prechat-context.ts: single buildPreChatContext builder shared by the control, pared-down, and native flows, collapsing the duplicated finish() and finishNativePreChat() context assembly. - Reduce the component to one currentStep state plus a render switch, with shared advance()/goBack() helpers driving all web steps. Because back resolves to the previous enabled step by construction, a back button can no longer reveal a step the forward path gated off — the class of back-target bug (e.g. back from Google jumping to a gated prior-assistants screen) is structurally impossible for every flow. User-visible behavior is otherwise unchanged across the web control, pared-down, and native flows. Part of LUM-2000.
Document the capability-driven behavior the review flagged as uncovered: a local user with no platform session but a cached platform assistant id still reaches the Google connect step when they pick a Google tool, and back from there lands on tools rather than the gated prior-assistants step.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1ebac2af0
ℹ️ 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".
…query the recipe Route the native iOS flow through the same advance/goBack helpers as web by making the terminal action flow-aware (web builds the handoff context, native routes to privacy) instead of hand-rolling setCurrentStep in the screen handlers. Native position persistence becomes a derived effect of currentStep rather than scattered persist calls, and the legacy sessionStorage value is recognized through a pure restoreNativeStep helper with its own tests. Replace the hand-rolled recipe fetch (useState + useEffect + cancellation guard) with a TanStack Query keyed by user: the recipe is platform-only server state, so it belongs in a query, not a bespoke loading flag. Behavior is preserved exactly (fetchOnboardingRecipe never throws), and the render now resolves the active step as steps.find(...) ?? steps[0] so the entry screen is always correct regardless of when isNative settles.
The react-query mock branched on the query key and only called the emulation hook for the recipe query, which trips react-hooks/rules-of-hooks (a hook called conditionally). Inline the useState/useEffect emulation directly into the mock useQuery so the hooks always run on every call; only the returned value branches on the query key.
…lity-driven-onboarding-screens
The inlined query emulation re-runs only when its enabled-state flips, like a real query; `options` is a fresh object each render, so it's intentionally excluded from the effect deps.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
✦ APPROVE
Value: The pre-chat onboarding flow stops being three near-duplicate index-based screens (web control / pared-down / native) and becomes one declarative, capability-derived step list. That collapses ~300 lines of conditional handlers, makes the local/managed/native paths legible, and structurally eliminates the "back reveals a gated step" bug class that produced the original LUM-2000 reviewer report.
What this does
- New pure modules under
apps/web/src/domains/onboarding/:prechat-steps.ts— source of truth for ordering + enabled predicates.resolveWebSteps(caps)/resolveNativeSteps()return ordered enabled steps;nextStep/prevStepwalk enabled steps by id;restoreNativeStep(saved)maps persisted sessionStorage back to a step id. No React, no hooks.prechat-context.ts— single builder for the onboarding handoff payload across all three modes, so the context shape can't drift between control / paredDown / native.
pre-chat-flow.tsx(-312 / +210, net -102): screen index state replaced withuseState<PreChatStepId>, everyonBackcollapses togoBack(activeStep.id), render switches onactiveStep.id. HardcodedsetScreen(N)calls — gone. The Devin reviewer's HEAD comment flaggingsetScreen(3)at line 629 is stale text: the file is 528 lines andgrep setScreenon HEAD returns zero hits. Devin-as-author's reply on the same thread is correct — back fromgooglewalksprevStepover the resolved enabled list, so whencanOfferPriorAssistants === falsethe prior-assistants step isn't in the list and back lands ontools.- 466 lines of new test coverage (
prechat-steps.test.ts,prechat-context.test.ts, expandedonboarding-lifecycle-sync.test.tsx) including the exact local-mode + cached-platform-assistant + Google-tool path the Devin reviewer flagged as undertested.
Anti-pattern check
- No
ascasts on runtime boundaries. Onlyashit in the diff is the word "as" in a JS comment onpre-chat-flow.tsx:152.prechat-context.tsreturns an earnedPreChatOnboardingContextfrom typed inputs; noJSON.parse/ API-response / postMessage casts. - No render-phase ref mutations in the new code.
pre-chat-flow.tsx's one effect that persists native position is properly gated onisNative+currentStepdeps withpersistNativeStepfrom the existing hook surface. - TanStack Query / Zustand patterns — flow state stays local (
useState), no new store. ExistinguseOnboardingLifecycleSyncand recipe query are untouched. - Backward-compat done right.
restoreNativeStepaccepts both"nativeVibe"(new) and"1"(legacy raw screen index) so a user who advanced on an older macOS / iOS shell build and then hits a WKWebView reload mid-onboarding doesn't get bounced to the name step. The legacy alias is justified — the platform and macOS/iOS shells don't ship together. Covered by direct unit tests.
Per-bot findings — verification
- Codex P2 / Devin 🚩 / Devin 🟡 (Google back gate) — all the same finding, all structurally fixed. The structural pivot in commit
f2483d04removed everysetScreen(N)call;goBack(activeStep.id)over the resolved enabled list is the only navigation surface. Testprechat-steps.test.ts"back never reveals a gated step" + the local-mode/cached-assistant Google path lock it in. - Codex P3 (legacy native restore) — addressed by
restoreNativeStepaccepting"1"alongside"nativeVibe". Three direct unit tests cover the matrix. - Devin 🚩 behavioral widening (local mode → Google screen) — Devin's reply is right: this is intentional and the whole point of LUM-2000. The old blanket
if (localMode) finish()was hiding a screen the capability gate (canOfferGoogleStepalready requireslocalPlatformAssistantId !== null) says is offerable. The genuinely separate "localPlatformAssistantIdcan be stale via outliving lockfile entries" concern is a capability-semantics question, not a navigation question, and correctly filed as follow-up rather than scope-creeping a behavior-preserving refactor.
Minor / non-blocking
- Naming nit only:
WebStepCapabilities.canOfferGoogleStepand theparedDown || caps.hasGoogleToolshort-circuit insideresolveWebStepsencode the same product rule from two angles. Not worth changing — the comment on lines explaining "the pared-down funnel has no tool-selection screen" makes it readable — but worth keeping an eye on if the funnel variants multiply.
Merge gate
| Gate | Status |
|---|---|
| Vex APPROVE | ✅ (this review) |
| Second approval | ✅ Codex 👍 reaction + "no major issues" at HEAD |
| CI checks | ✅ 7/7 green (Lint / Type Check / Test / Build / Lint Type Check & Build / 2× Socket) |
| Outstanding REQUEST_CHANGES | ✅ none |
Vellum Constitution — Distinct: replacing imperative index-based navigation with a declarative capability-derived step list is the local-first take on a problem the rest of the ecosystem solves with URL-routed wizards or XState. Steps as ids over indices makes the local / managed / native paths legible without bolting routing on top of state that genuinely belongs in-memory.
Prompt / plan
LUM-2000. The ticket began as "serve the onboarding recipe in local mode," but investigation (web onboarding flow + daemon/gateway + platform recipe endpoint) showed that framing was wrong: the recipe is platform-only marketing-funnel data and must stay
nullin local mode. Working through the real fix surfaced a deeper issue — the pre-chat flow's navigation was imperative and index-based, which is what produced the back-target bug a reviewer caught. So this PR does two things: it gates onboarding screens by capability instead of alocalModespecial-case, and it restructures the flow as a declarative, capability-derived step list so the local/managed/native paths are legible and the back-target bug class is structurally impossible.Why this shape
Multi-step flows with conditional steps are a solved problem. The ecosystem (use-stepper, react-use-wizard, the Formik / react-hook-form "wizard with conditional steps" recipe) converges on the same model: named steps, not numeric indices; a declarative
{ id, enabled }list; navigation = "go to the next/previous enabled step." Back falling out as "previous enabled step" is exactly why a back button can't reveal a gated step.Two heavier options were considered and rejected:
hasGoogleToolis chosen during the flow), so URL guards would re-derive the same enabled-logic and we'd keep the step-list as source of truth and bolt routing on top. Onboarding is also deliberately non-addressable (you don't want a bookmark to "step 4" with no collected answers), and native persists position insessionStorageprecisely because it isn't in the URL (WKWebView memory reclaim). More machinery, not less.The verified root issue wasn't "indices vs names" — it was that navigation policy was scattered across the rendering handlers. The fix separates flow policy (which steps, what order, enabled when, what "done" does) from rendering (which component draws the current step).
What changed
prechat-steps.ts(new, pure): the source of truth for step ordering and enabled predicates.resolveWebSteps(capabilities)/resolveNativeSteps()return the ordered, enabled steps;nextStep/prevStepwalk enabled steps by id;restoreNativeStep(saved)maps a persistedsessionStoragevalue back to a step id. No React, no hooks — fully unit-testable. Local mode, the funnel variant, the connected tool, and platform all "fall out" of the predicates here instead of being special-cased.prechat-context.ts(new, pure): onebuildPreChatContext(input)builder for all three modes (control / pared-down / native), collapsing the duplicatedfinish()+finishNativePreChat()context assembly so the payload shape can't drift between variants.pre-chat-flow.tsx(refactored): the component now holds a singlecurrentStepid plus a render switch. Sharedadvance()/goBack()helpers drive every step — web and native — and a single flow-aware terminal action (reachTerminal) finishes the flow: web builds the pre-chat context and routes to chat, native routes to the privacy step. The imperativesetScreen(0..5)indices, the per-handler capability checks, and native's separate hand-rolledsetCurrentStep(...)transitions are all gone. Web control and pared-down are now one step list distinguished only byenabled; native keeps its own list (its realNameStep/VibeStepcomponents, its terminal privacy route, itssessionStoragepersistence) but shares the same navigation engine.No daemon/gateway recipe endpoint, no new IPC, no platform change. The recipe stays
nullin local/native runtimes (platform-only marketing data resolved from a UTM campaign cookie / stored attribution; there is no endpoint to fetch or build).State-management cleanup (second audit pass)
A first-principles audit of the final state turned up three correctness/convention issues that are fixed here rather than deferred:
useState+ loading flag +useEffect.then()/cancellation guard — server state managed by hand, whichSTATE_MANAGEMENT.mdcalls out as the anti-pattern. It's now auseQuery({ queryKey: ["onboarding-recipe", userId], queryFn, enabled, staleTime: Infinity }), matching theactiveAssistantquery already in the same component. Behavior is identical (fetchOnboardingRecipecatches internally and never throws, soisLoading+datamap exactly onto the oldrecipeLoadState), with the manual loading state and cancellation logic deleted.persistNativeStep(...), a single effect writessessionStoragefromcurrentStep("nativeVibe"→ persisted, otherwise cleared). The writes are exactly the same as before; they just can't drift from the navigation anymore.steps.find((s) => s.id === currentStep) ?? steps[0], so ifcurrentStepis ever not in the resolved list (e.g.isNativesettling after mount under a future hydration model), it falls back to the first step of whichever flow resolved instead of rendering blank. TodayuseIsNativePlatform()is correct on first render (pure CSRcreateRoot,useSyncExternalStoresynchronous snapshot), so this is defense against a latent coupling, not a live bug.Bug fixed: back from Google could reveal a gated screen
What it was: the Google connect screen's Back handler hardcoded
setScreen(3)(prior-assistants). WhencanOfferPriorAssistantswas false but a selected Google tool routed the user to Google anyway (local mode without a platform session but with a cached platform assistant id), pressing Back exposed the prior-assistants import step the gate was meant to suppress — and continuing could persistpriorAssistantsinto the pre-chat context.Root cause analysis:
canOfferPriorAssistantsgate; the back handler did not.Bug fixed: native pre-chat lost its place across an app update
What it was: native persists the in-progress step in
sessionStorageso a WKWebView memory reclaim doesn't bounce the user to the start. Older builds stored the raw screen index ("1"); this refactor writes the step id ("nativeVibe"). A native user mid-onboarding when the app updated would have the legacy"1"on disk, which the new restore path didn't recognize — so they'd be sent back to the name step.Fix: restoration goes through the pure
restoreNativeStep(saved)helper, which accepts both the current id ("nativeVibe") and the legacy index ("1") as the vibe step and returnsnull(start from the top) for anything else. The macOS/iOS shell and the platform don't ship together, so a persisted value can outlive the build that wrote it — the legacy alias keeps the transition safe across that skew. Covered directly inprechat-steps.test.ts.Behavior preservation
User-visible behavior is unchanged across all flows; only the two back-target / restore bugs above are fixed.
!localMode):canOfferPriorAssistantsistrue→ identical sequence.canOfferGoogleStepalready accounted for a cached assistant id); back lands on tools, never the gated prior-assistants step. This combined path is now covered by a test.sessionStorageposition persistence, preserved exactly — now driven through the sharedadvance/goBackengine instead of bespoke handlers.No change crosses the macOS↔platform wire boundary (no HTTP/SSE/IPC/lockfile shape change), so it is safe across the release skew in both directions.
Test plan
prechat-steps.test.ts(new): every capability combination — full control funnel; local no-session collapse; local + session keeps prior-assistants; google only when a Google tool is picked / suppressed when unavailable; iOS-app only on iOS web; pared-down (name → google, and collapse to name); variant-specific funnel events;nextStep/prevStepwalks including "back never reveals a gated step"; andrestoreNativeStep(current id, legacy"1", and unknown/null→ start from top).prechat-context.test.ts(new): context shape per mode (control / pared-down / native), Google scope precedence (connecting action vs stored), prior-assistants capture, native isolation (other flows' selections don't leak), and initial-message resolution.onboarding-lifecycle-sync.test.tsx: existing integration suite still green — recipe never fetched in local mode, recipe loading gates the render, prior-assistants gated without a session and shown with one. The react-query mock emulates the recipe query (runs thequeryFn, tracksisLoading, honorsenabled) so the loading-gate tests stay meaningful after theuseQueryconversion.bunx tsc --noEmitandeslintclean; full CI (lint, type check, build, test) green.Follow-ups (filed in Linear)
pre-chat-flow.tsx), native routes to privacy after collecting answers. Preserved exactly here; it's a product/legal decision, not a code-shape one, so it's tracked separately rather than changed in a behavior-preserving refactor.localPlatformAssistantId(LUM-2180): a cachedcloud === "vellum"lockfile entry can outlive the platform session, makingcanOfferGoogleSteptrue for a local user. Orthogonal to navigation shape (it's about the capability's own correctness); tracked separately.pre-chat-flow.tsxsize (LUM-2181): down from 643 lines after extracting the pure step/context logic; the remaining effects (consent snapshot, navigation guard) are candidates for hook extraction, deferred to avoid mixing a behavior-risky change into this one.Closes LUM-2000
Link to Devin session: https://app.devin.ai/sessions/15bca57bd4c64a3085cfb80e1f26355a
Requested by: @ashleeradka