fix(onboarding): stop resetting users to the start when a step disappears#3105
Conversation
…ears Onboarding kept kicking users back to the welcome screen mid-flow. The conditional import-config step flickers in while the getSummary query is pending (undefined data read as "importable") and out when it settles; anyone standing on it was reset to activeSteps[0]. Analytics show 65 users landed on import-config since June 10 and zero ever completed it. - useOnboardingFlow now moves to the nearest surviving step (forward in canonical order, then backward) instead of the first step - useHasImportableConfig only includes the step once the summary has resolved with total > 0 — loading/error no longer count as importable - electron-trpc createIPCHandler accepts an onError hook, wired to a host-trpc scoped logger in main, so procedure failures (like the getSummary rejections behind this loop) finally show up in main.log Generated-By: PostHog Code Task-Id: 53087b3c-6633-4aaf-b1cb-4b840f010503
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Reviews (1): Last reviewed commit: "fix(onboarding): stop resetting users to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const candidate = ONBOARDING_STEPS[i]; | ||
| if (activeSteps.includes(candidate)) return candidate; | ||
| } | ||
| return activeSteps[0]; |
There was a problem hiding this comment.
undefined when activeSteps is empty
activeSteps[0] is undefined at runtime when the array is empty, but the return type is OnboardingStep. The two loops together cover every canonical index except step itself, so this line is only reached when activeSteps is empty. computeActiveSteps always keeps "welcome", so this is unreachable today, but the type contract is misleading — a future caller passing an empty array would get a silent undefined with no TypeScript warning unless noUncheckedIndexedAccess is enabled.
| test("reports procedure failures through onError and still responds", async () => { | ||
| const failingRouter = t.router({ | ||
| failingQuery: t.procedure.query(() => { | ||
| throw new Error("db exploded"); | ||
| }), | ||
| }); | ||
| const onError = vi.fn(); | ||
| const event = makeEvent({ | ||
| reply: vi.fn(), | ||
| sender: { | ||
| isDestroyed: () => false, | ||
| on: () => {}, | ||
| }, | ||
| }); | ||
|
|
||
| await handleIPCMessage({ | ||
| createContext: async () => ({}), | ||
| event, | ||
| internalId: "1-1:1", | ||
| message: { | ||
| method: "request", | ||
| operation: { | ||
| context: {}, | ||
| id: 1, | ||
| input: undefined, | ||
| path: "failingQuery", | ||
| type: "query", | ||
| signal: undefined, | ||
| }, | ||
| }, | ||
| router: failingRouter, | ||
| operations: new Map(), | ||
| onError, | ||
| }); | ||
|
|
||
| expect(onError).toHaveBeenCalledOnce(); | ||
| expect(onError.mock.lastCall?.[0]).toMatchObject({ | ||
| path: "failingQuery", | ||
| type: "query", | ||
| }); | ||
| expect(onError.mock.lastCall?.[0].error.cause?.message).toBe("db exploded"); | ||
| expect(event.reply.mock.lastCall?.[1]).toMatchObject({ | ||
| id: 1, | ||
| error: expect.anything(), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
onError is only exercised on the query/outer-catch path
The new test proves onError fires when a synchronous query throws (the outer try/catch at line 233 of handleIPCMessage.ts). There are two other onError call sites — the subscription iterator error branch (next instanceof Error, line 160) and the run().catch() handler (line 211) — that remain uncovered. Given that getSummary is a query the PR description identifies as perpetually failing, the subscription paths could easily hide the same silent-failure problem that motivated this change.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Root cause of getSummary never succeeding: the renderer is typed against HostRouter (which has onboardingImport), but the Electron app serves its own hand-assembled trpcRouter, and the #2442 package split dropped onboardingImport from that assembly while keeping it in the type. Every getSummary call since June 10 has died with NOT_FOUND — zero users ever completed the import-config step. Registers the route again and adds a compile-time guard asserting the app router serves every HostRouter route; a future drop now fails typecheck with an error naming the missing route. Generated-By: PostHog Code Task-Id: 53087b3c-6633-4aaf-b1cb-4b840f010503
…subscription onError test
Addresses the two Greptile P2 review comments:
- nearestActiveStep returned activeSteps[0] as its final fallback, which
is undefined when the list is empty despite the OnboardingStep return
type. It now returns the input step unchanged ("stay put") — the empty
list is unreachable today since computeActiveSteps always keeps
welcome, but the contract is honest now. Adds an empty-array test.
- onError in electron-trpc was only tested on the query/outer-catch
path. Adds a test for the subscription iterator-error branch: an
observable that emits an error mid-stream must invoke onError with
path/type and still serialize an error envelope to the renderer
(which arrives before the final "stopped" message).
Generated-By: PostHog Code
Task-Id: 53087b3c-6633-4aaf-b1cb-4b840f010503
2 participants
The bug
Onboarding kept kicking users back to the beginning (Paul's report). Reconstructed from his logs + our own analytics (
Onboarding step viewed/completedevents, project 2):useHasImportableConfigreturneddata?.total !== 0, which istruewhile theonboardingImport.getSummaryquery has no data yet (undefined !== 0). The import-config step ("Your Claude Code environment is ready") therefore flickers intoactiveStepsduring every pending/retry window and out when the query settles at 0 or errors. WithrefetchOnWindowFocus: trueand a query that keeps rejecting, every alt-tab back into the app re-arms the trap (TanStack v5 resets a no-data query topending/error: nullwhen a refetch starts).useOnboardingFlowdidsetCurrentStep(activeSteps[0])→ welcome. Paul hit this twice in one session (10:22:20 and 10:22:54 his time), matching his screenshots to the second.hasCodeAccessresolves late.Root cause of the failing query
The app never serves
onboardingImport. The renderer and@posthog/uiare typed againstHostRouter(the merged router inpackages/host-router, which has the route), but the Electron main process serves the hand-assembledtrpcRouterinapps/code/src/main/trpc/router.ts. Before the #2442 package split (June 10) that file registeredonboardingImportand the feature worked; the split moved the router file intopackages/host-router, added it to the mergedhostRouter(used only for its type), and dropped it from the served assembly. EverygetSummarycall since has died instantly withNOT_FOUND— silently, because IPC procedure errors were never logged. The analytics corroborate:import-configstep views begin June 9, the zero-completion pattern begins exactly June 10.The fixes
apps/coderouter: registeronboardingImportagain, and add a compile-time guard (Exclude<keyof HostRouter, keyof TrpcRouter>must benever) so dropping any HostRouter route from the app assembly now fails typecheck with an error that names the missing route.useOnboardingFlow: when the current step drops out ofactiveSteps, move to the nearest surviving step (forward in canonical order, then backward) via a new purenearestActiveStephelper in@posthog/core— never back to the start.useHasImportableConfig: only include the step once the summary has resolved withtotal > 0. Loading and error states no longer conjure a step the user can be dropped into.electron-trpc:createIPCHandlernow accepts anonErrorhook, wired inapps/codemain to ahost-trpcscoped logger, so host procedure failures show up in main.log instead of vanishing (this is why Paul's logs were empty through the whole loop).Tests
nearestActiveStepunit tests (forward, backward, still-active cases) inpackages/coreonErrortransport test inpackages/electron-trpctscfails withType 'true' is not assignable to type '"onboardingImport"'; with it registered, cleanpnpm --filter @posthog/core|@posthog/ui|@posthog/electron-trpc testand typechecks for core/ui/electron-trpc/apps-code all pass; biome clean🤖 Generated with Claude Code