Skip to content

Commit ce0d3af

Browse files
anandgupta42claude
andcommitted
fix: scope the shouldSkipOnboardingAtStartup discriminator to a genuine first-run completion
cubic (3986532221) correctly flagged this as a regression introduced by the round-7 shouldSkipOnboardingAtStartup fix, not a pre-existing follow-up: `setupComplete()` is a global flag set by ANY model selection (`markSetupComplete()`, called from `DialogModel` on every pick), not only a first-run/impatient-picker completion. A RETURNING user (e.g. an existing Big Pickle user) who did an ordinary `/model` switch while app.tsx's startup effect was still settling made `setupComplete()` true too, falling through to the `onboardingReady()` branch and firing `onboarding_started`/`onboarding_completed`/`scan_gate_shown` telemetry plus `openScanGate()` for a routine model change — not a genuine first run. Fix: `firstRunOpenedThisLaunch` (altimate-onboarding.tsx) is a new one-way latch, set only when the first-run picker actually opens THIS launch — app.tsx's own startup fallthrough, or the prompt gate's equivalent (component/prompt/index.tsx now calls `markFirstRunActive()` there too, closing the gap cubic named explicitly). app.tsx's `shouldSkipOnboardingAtStartup` call site now passes `setupComplete() && firstRunOpenedThisLaunch()` instead of the bare global flag — true only for a genuine same-launch first-run completion, so a returning user's routine switch reads false and keeps skipping exactly as before this whole discriminator existed. Tests: two new tests in local.test.ts exercise the real production signals (markSetupComplete/markFirstRunActive/firstRunOpenedThisLaunch) against the actual discriminator computation app.tsx performs — a returning user's routine switch must skip (no telemetry, no scan gate), a genuine first-run completion must not skip. Confirmed the second test fails when the `firstRunOpenedThisLaunch` latch is temporarily removed, then passes again restored. Removed the corresponding item from tracking issue #1303 (fixed directly, not deferred). Co-Authored-By: Claude Fable 5.1 <[email protected]>
1 parent b01a139 commit ce0d3af

4 files changed

Lines changed: 129 additions & 12 deletions

File tree

‎packages/tui/src/app.tsx‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ import {
4545
useSetupComplete,
4646
markFirstRunActive,
4747
resetSetupComplete,
48+
useFirstRunOpenedThisLaunch,
4849
} from "./component/altimate-onboarding"
4950
// altimate_change end
5051
// altimate_change — Part 2 scan gate (fires once when Part 1 first completes)
@@ -607,6 +608,9 @@ function App(props: { onSnapshot?: () => Promise<string[]>; pluginHost: TuiPlugi
607608
const onboardingReady = useReady()
608609
// altimate_change — setup completion alone (no `connected()` term); see the scan-gate effect
609610
const setupComplete = useSetupComplete()
611+
// altimate_change — cubic review (3986532221): see `firstRunOpenedThisLaunch`'s declaration in
612+
// altimate-onboarding.tsx
613+
const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch()
610614
// altimate_change — onboarding funnel tracker (no-op when the host injected none)
611615
const trackOnboarding = useOnboardingTelemetry()
612616
// altimate_change end
@@ -729,17 +733,27 @@ function App(props: { onSnapshot?: () => Promise<string[]>; pluginHost: TuiPlugi
729733
// hits the picker again on their next submit and loses whatever they typed. See
730734
// `hasUsableFreeDefault`'s declaration in local.tsx for where the fold now lives.
731735
// altimate_change start — Kilo review round 6 (3986171188): `shouldSkipOnboardingAtStartup`
732-
// — see its declaration in local.tsx — added the `setupComplete()` discriminator. Without it,
733-
// an impatient first-run user who submits before this effect settles — the prompt gate
734-
// (component/prompt/index.tsx) opens the picker on its own, they pick a free Zen model,
735-
// `set()` marks it explicit/recent and `markSetupComplete()` runs — made
736-
// `hasUsableFreeDefault()` true by the time THIS effect finally runs, latching here and
737-
// returning before the `onboardingReady()` branch below (which exists for exactly this
736+
// — see its declaration in local.tsx — added a discriminator for "did first-run genuinely
737+
// complete THIS launch". Without it, an impatient first-run user who submits before this
738+
// effect settles — the prompt gate (component/prompt/index.tsx) opens the picker on its own,
739+
// they pick a free Zen model, `set()` marks it explicit/recent and `markSetupComplete()` runs
740+
// — made `hasUsableFreeDefault()` true by the time THIS effect finally runs, latching here
741+
// and returning before the `onboardingReady()` branch below (which exists for exactly this
738742
// impatient-user case, per its own comment) ever got a chance to fire the funnel telemetry
739-
// and `openScanGate()`. A genuine RETURNING user's `setupComplete()` is always false here —
740-
// it starts false every launch and is set only by a setup completed DURING this one (see the
741-
// comment on the `onboardingReady()` branch) — so this changes nothing for that case; it only
742-
// stops THIS branch from swallowing a same-launch setup that `onboardingReady()` needs to see.
743+
// and `openScanGate()`.
744+
//
745+
// cubic review (3986532221): a bare `setupComplete()` is NOT that discriminator — it is a
746+
// global flag `markSetupComplete()` sets for ANY model selection, first-run or not, so a
747+
// RETURNING user (`hasExistingLegacySelection()` or `hasUsableFreeDefault()` already true)
748+
// who does an ordinary `/model` switch while this effect is still settling ALSO makes
749+
// `setupComplete()` true — which used to fall through to the `onboardingReady()` branch and
750+
// fire onboarding telemetry + open the scan gate for a routine model change, not a first run.
751+
// `firstRunOpenedThisLaunch()` (see its declaration in altimate-onboarding.tsx) is the fix: a
752+
// one-way latch set only when the first-run picker itself actually opened THIS launch (this
753+
// effect's own fallthrough below, or the prompt gate's equivalent) — `setupComplete() &&
754+
// firstRunOpenedThisLaunch()` is true only for a GENUINE first-run completion. A returning
755+
// user's routine switch never sets `firstRunOpenedThisLaunch()`, so it correctly reads false
756+
// and this branch keeps skipping for them, exactly as before this whole discriminator existed.
743757
// A paid pick is unaffected either way: `hasUsableFreeDefault()` requires a free model, so it
744758
// was never true for one.
745759
if (
@@ -750,7 +764,7 @@ function App(props: { onSnapshot?: () => Promise<string[]>; pluginHost: TuiPlugi
750764
// early above unless `kv.ready`, so it is always a plain boolean by the time it runs
751765
// here; the explicit check just satisfies the union type without widening it elsewhere.
752766
local.model.hasUsableFreeDefault() === true,
753-
setupComplete(),
767+
setupComplete() && firstRunOpenedThisLaunch(),
754768
)
755769
) {
756770
startupDecisionHandled = true

‎packages/tui/src/component/altimate-onboarding.tsx‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,33 @@ const [setupComplete, setSetupComplete] = createSignal(false)
3535
// model switching, so it consults this before emitting any funnel event — otherwise every model
3636
// change for the life of the product would look like an onboarding provider choice.
3737
const [firstRunActive, setFirstRunActive] = createSignal(false)
38+
// altimate_change start — cubic review (3986532221): app.tsx's startup effect used
39+
// `setupComplete()` alone to decide whether THIS launch's model selection is a genuine
40+
// first-run/impatient-picker completion (worth firing onboarding telemetry + the scan gate for)
41+
// versus a RETURNING user's ordinary `/model` switch that merely raced the startup effect —
42+
// `markSetupComplete()` fires for BOTH cases identically. `firstRunActive` above cannot answer
43+
// this either: `markSetupComplete()` deliberately CLEARS it (so a later routine switch doesn't
44+
// look like onboarding), so by the time app.tsx's effect gets around to checking it, it has
45+
// already been reset to `false` for both a genuine first-run AND the very completion that would
46+
// prove it happened. `firstRunOpenedThisLaunch` is a separate, ONE-WAY latch: set whenever the
47+
// first-run picker actually opens THIS launch — either app.tsx's own startup fallthrough, or the
48+
// prompt gate's equivalent for an impatient submit before that effect settles (see
49+
// `component/prompt/index.tsx`'s `markFirstRunActive()` call) — and never cleared by
50+
// `markSetupComplete()`/`clearFirstRunActive()` (only by `resetSetupComplete()`, on `/logout`,
51+
// which returns the user to a genuinely fresh state). `setupComplete() &&
52+
// firstRunOpenedThisLaunch()` is the correct "did first-run genuinely complete this launch"
53+
// signal; a returning user's routine mid-race `/model` switch has `setupComplete() === true` but
54+
// `firstRunOpenedThisLaunch() === false`, so it reads as `false` and no longer fires anything.
55+
const [firstRunOpenedThisLaunch, setFirstRunOpenedThisLaunch] = createSignal(false)
56+
export function useFirstRunOpenedThisLaunch() {
57+
return firstRunOpenedThisLaunch
58+
}
59+
// altimate_change end
3860
export function markFirstRunActive() {
3961
setFirstRunActive(true)
62+
// altimate_change — see `firstRunOpenedThisLaunch`'s declaration above
63+
setFirstRunOpenedThisLaunch(true)
64+
// altimate_change end
4065
}
4166
/**
4267
* Clear without marking setup complete.
@@ -65,6 +90,10 @@ export function markSetupComplete() {
6590
export function resetSetupComplete() {
6691
setSetupComplete(false)
6792
setFirstRunActive(false)
93+
// altimate_change — see `firstRunOpenedThisLaunch`'s declaration above: /logout returns the
94+
// user to a genuinely fresh state, so a first run after it must be free to latch again.
95+
setFirstRunOpenedThisLaunch(false)
96+
// altimate_change end
6897
}
6998
export function useReady() {
7099
const connected = useConnected()

‎packages/tui/src/component/prompt/index.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ import { useDialog } from "../../ui/dialog"
4848
import { DialogProvider as DialogProviderConnect, WARNLIST } from "../dialog-provider"
4949
// altimate_change — first-run submit gate: open the curated welcome picker instead
5050
// of erroring when no model is ready yet (see altimate-onboarding.tsx).
51-
import { DialogModelWelcome, useReady, useReadyPending } from "../altimate-onboarding"
51+
import { DialogModelWelcome, markFirstRunActive, useReady, useReadyPending } from "../altimate-onboarding"
5252
import { DialogAlert } from "../../ui/dialog-alert"
5353
import { useToast } from "../../ui/toast"
5454
import { useKV } from "../../context/kv"
@@ -1103,6 +1103,13 @@ export function Prompt(props: PromptProps) {
11031103
deferredSubmit = true
11041104
return false
11051105
}
1106+
// altimate_change — cubic review (3986532221): this is the prompt-gate's own equivalent of
1107+
// app.tsx's first-run picker (an impatient submit before that startup effect settled), so
1108+
// it must latch the SAME "first-run opened this launch" signal — see
1109+
// `firstRunOpenedThisLaunch`'s declaration in altimate-onboarding.tsx for why app.tsx's
1110+
// startup effect needs this to tell a genuine first-run completion apart from a returning
1111+
// user's routine `/model` switch racing that same effect.
1112+
markFirstRunActive()
11061113
dialog.replace(() => (
11071114
<DialogModelWelcome
11081115
intro="First, let's connect your AI model — then I'll get right on that."

‎packages/tui/test/context/local.test.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
11
import { expect, test } from "bun:test"
2+
// altimate_change — cubic review (3986532221): the real, module-level onboarding signals, to
3+
// test the app.tsx call site's discriminator computation against the ACTUAL production functions
4+
// rather than only the pure `shouldSkipOnboardingAtStartup` predicate in isolation.
5+
import {
6+
markFirstRunActive,
7+
markSetupComplete,
8+
resetSetupComplete,
9+
useFirstRunOpenedThisLaunch,
10+
useSetupComplete,
11+
} from "../../src/component/altimate-onboarding"
212
import {
313
allowsManagedBaseDefault,
414
ALTIMATE_BASE_MODEL,
@@ -371,6 +381,63 @@ test("shouldSkipOnboardingAtStartup: a same-launch setup must not swallow the on
371381
})
372382
// altimate_change end
373383

384+
// altimate_change start — cubic review (3986532221): the fix above still passed a bare
385+
// `setupComplete()` at the app.tsx call site, which is a GLOBAL flag `markSetupComplete()` sets
386+
// for ANY model selection, not only a first-run one. A RETURNING user (existing
387+
// legacy/free-default selection) who does an ordinary `/model` switch while app.tsx's startup
388+
// effect is still settling made `setupComplete()` true too, which used to fall through to the
389+
// `onboardingReady()` branch and fire `onboarding_started`/`onboarding_completed`/
390+
// `scan_gate_shown` telemetry plus `openScanGate()` for a routine model change — not a first run.
391+
// `firstRunOpenedThisLaunch()` (altimate-onboarding.tsx) is a one-way latch set only when the
392+
// first-run picker itself actually opens THIS launch (app.tsx's own fallthrough, or the prompt
393+
// gate's equivalent in component/prompt/index.tsx); `setupComplete() && firstRunOpenedThisLaunch()`
394+
// — what app.tsx now actually passes — is the correct "did first-run genuinely complete this
395+
// launch" signal. These tests exercise the REAL production signals (not synthetic booleans),
396+
// asserting `shouldSkipOnboardingAtStartup` receives the correctly-computed discriminator: `skip
397+
// === true` means app.tsx's startup effect returns BEFORE ever reaching the telemetry/scan-gate
398+
// branch, so it is the direct proxy for "no onboarding telemetry, no scan gate" at this call site.
399+
test("a returning user's routine /model switch during the startup race window fires no onboarding telemetry or scan gate", () => {
400+
resetSetupComplete()
401+
try {
402+
// Returning user: has an existing legacy/free-default selection (`hasExistingLegacySelection`
403+
// true below). They switch models via an ORDINARY `/model` pick — NOT through the first-run
404+
// picker — while app.tsx's startup effect is still settling. `markSetupComplete()` fires for
405+
// this exactly as it does for every model pick, first-run or not.
406+
markSetupComplete()
407+
const setupComplete = useSetupComplete()
408+
const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch()
409+
expect(setupComplete()).toBe(true)
410+
expect(firstRunOpenedThisLaunch()).toBe(false)
411+
412+
// Mirrors app.tsx's actual call site exactly.
413+
const skip = shouldSkipOnboardingAtStartup(true, false, setupComplete() && firstRunOpenedThisLaunch())
414+
expect(skip).toBe(true)
415+
} finally {
416+
resetSetupComplete()
417+
}
418+
})
419+
420+
test("a genuine impatient first-run completion (the prompt gate opened this launch) still fires the onboardingReady() branch", () => {
421+
resetSetupComplete()
422+
try {
423+
// The prompt gate (component/prompt/index.tsx's `!ready()` branch) — or app.tsx's own
424+
// startup fallthrough — actually opened the first-run picker THIS launch...
425+
markFirstRunActive()
426+
// ...and the user picked a free model there, completing it.
427+
markSetupComplete()
428+
const setupComplete = useSetupComplete()
429+
const firstRunOpenedThisLaunch = useFirstRunOpenedThisLaunch()
430+
expect(setupComplete()).toBe(true)
431+
expect(firstRunOpenedThisLaunch()).toBe(true)
432+
433+
const skip = shouldSkipOnboardingAtStartup(false, true, setupComplete() && firstRunOpenedThisLaunch())
434+
expect(skip).toBe(false)
435+
} finally {
436+
resetSetupComplete()
437+
}
438+
})
439+
// altimate_change end
440+
374441
// altimate_change start — PR #1302 Codex review round 2, P2: `migrateLegacyDefault({ from })`'s
375442
// captured `from` must not bypass free-model validation entirely.
376443
test("isMigrationStillEligibleAfterCapture: only the launch-default-unchanged or registration-induced-Base transitions stay eligible", () => {

0 commit comments

Comments
 (0)