Skip to content

fix(tui): patch nanostores map.setKey to mutate $session in place#762

Merged
kelsonpw merged 1 commit into
mainfrom
fix/env-picker-race-real-fix
May 13, 2026
Merged

fix(tui): patch nanostores map.setKey to mutate $session in place#762
kelsonpw merged 1 commit into
mainfrom
fix/env-picker-race-real-fix

Conversation

@kelsonpw
Copy link
Copy Markdown
Member

@kelsonpw kelsonpw commented May 13, 2026

Summary

PR #760 added the pendingEnvSelection flag and gated every post-Auth show: predicate on it, but the live bug still reproduced. Real root cause is a stale-reference bug between bin.ts and the WizardStore:

  • bin.ts holds a session ref that it mutates in-place during checkpoint hydration, resolveCredentials, and applyEnvSelectionDeferral, then calls tui.store.session = session to re-emit.
  • The store's $session = map<WizardSession>(...). Nanostores' map.setKey reassigns $session.value to a fresh { ...prev, [key]: value } object on every key change.
  • The first in-store setter (typically concludeIntro fired when the user clicks Resume on the checkpoint prompt while resolveCredentials is still awaiting) detaches $session.value from the bin.ts session ref.
  • After that point, bin.ts's later in-place mutations — including the deferral that sets pendingEnvSelection = true and clears credentials — never reach the store.
  • The final tui.store.session = session then OVERWRITES the store's accumulated state with the stale bin.ts ref, wiping the user's introConcluded = true progress and producing either a bump back to Intro or a stall with no env-picker surface.

PR #760's flag write site (session.pendingEnvSelection = true) is correct in isolation — it just lands on a session object the store already stopped reading from.

The fix

Patch $session.setKey to mutate $session.value in place instead of allocating a fresh spread. notify() still fires per key, so the React useSyncExternalStore bridge (which reads $version) is unaffected. The wizard's UI doesn't use listenKeys anywhere, so the per-listener oldValue arg semantics change is non-observable.

With the patch, $session.value and the bin.ts session ref point at the same object for the entire run. External mutations propagate transparently; store setters don't clobber them.

Regression test

Added two tests in src/ui/tui/__tests__/flow-invariants.test.ts:

  1. preserves the bin.ts session reference after an in-store setKey — pins ref stability across concludeIntro.
  2. propagates in-place external mutations after an in-store setKey, including pendingEnvSelection — replays the exact bin.ts sequence: build session → startTUI → concludeIntro → in-place deferral mutations → final store.session = session. Asserts currentScreen === Auth (env picker surface) and that introConcluded survives the resync.

Both tests fail on origin/main (verified by stashing the store change) and pass with this commit.

Test plan

  • pnpm exec tsc --noEmit clean
  • pnpm exec eslint . clean (one pre-existing warning unrelated to this change)
  • pnpm build succeeds (smoke test passes)
  • pnpm exec vitest run --pool=forks --maxWorkers=1 --testTimeout=30000 — 3955/3955 passing
  • Regression test fails on origin/main without the store change

🤖 Generated with Claude Code


Note

Medium Risk
Changes core TUI session state update semantics by overriding nanostores map.setKey, which could affect any code relying on object identity or listenKeys old/new value behavior; however the change is scoped to the Wizard session store and covered by new regression tests.

Overview
Fixes a TUI race where WizardStore session updates could detach from the bin.ts session reference, causing in-place mutations (notably pendingEnvSelection) to be missed and later store.session = session re-emits to clobber progress.

WizardStore now builds $session via a custom createSessionStore() that patches nanostores map.setKey to mutate the existing session object in-place while still calling notify().

Adds regression tests in flow-invariants.test.ts to assert (1) session reference stability across concludeIntro() and (2) propagation of external in-place mutations plus preservation of introConcluded and correct routing back to Screen.Auth.

Reviewed by Cursor Bugbot for commit d8f31a7. Bugbot is set up for automated code reviews on this repo. Configure here.

PR #760 added the pendingEnvSelection flag and gated every post-Auth
show: predicate on it, but the live bug still reproduced. Root cause:
bin.ts holds a session ref that it mutates in-place during checkpoint
hydration, resolveCredentials, and applyEnvSelectionDeferral. The
WizardStore exposes that same ref via store.session and re-syncs it
at the end of the credential block via tui.store.session = session.

Nanostores' map.setKey reassigns $session.value to a fresh
{ ...prev, [key]: value } object on every key change. The first
in-store setter (typically concludeIntro fired when the user picks
Resume on the checkpoint prompt while resolveCredentials is still
awaiting) detaches $session.value from the bin.ts session ref. After
that point bin.ts's later in-place mutations — including the deferral
that sets pendingEnvSelection = true and clears credentials — never
reach the store. The final tui.store.session = session then OVERWRITES
the store's accumulated state with the stale bin.ts ref, wiping the
user's introConcluded = true progress and producing either a bump back
to Intro or a stall with no env-picker surface (the bug PR #760 was
supposed to fix).

Patching setKey to mutate $session.value in place keeps it and the
bin.ts session ref pointing at the same object for the entire run, so
external mutations propagate transparently and store setters don't
clobber them. notify() still fires per key for the React
useSyncExternalStore bridge (which reads $version); listenKeys
subscribers would see the new value via both args of their callback,
but the wizard's UI doesn't use listenKeys anywhere.

Adds two regression tests in flow-invariants.test.ts that pin both
halves of the regression: ref stability across in-store setters AND
propagation of in-place external mutations after a setter has fired.
Both tests fail on origin/main and pass with this change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kelsonpw kelsonpw requested a review from a team as a code owner May 13, 2026 15:07
@kelsonpw kelsonpw merged commit 2d9e603 into main May 13, 2026
11 checks passed
kelsonpw added a commit that referenced this pull request May 14, 2026
…n when pendingEnvSelection gets clobbered (#775)

Three prior PRs claimed to fix the "stuck on Setup with no env-picker"
bug — #747 cleared session.credentials when resolveCredentials returned
needs_user_choice/environment_selection, #760 added the
pendingEnvSelection flag and gated every post-Auth show: predicate on
it, and #762 patched nanostores' map.setKey to mutate in-place so the
bin.ts session reference stays aligned with $session.value. Each ship
passed its unit tests. The bug kept reproducing in user-reported runs
(pnpm try:prod --install-dir=~/excalidraw → successful run → git reset
--hard → rerun → wizard navigates the TUI, stops at "✓ Welcome ─ ✓
Auth ─ ● Setup ←", and hangs).

Per-run logs end at "[intro] no framework matched — falling back to
Generic" and nothing fires after. Static analysis of every write to
pendingEnvSelection across src/ found exactly one direct set-true path
(applyEnvSelectionDeferral) and four explicit set-false paths (all
inside AuthScreen, gated on the user picking an env). None of them
explain how the flag flips back to false before the user has
interacted with the picker — but the journey stepper's "● Setup ←"
state requires both Auth.isComplete=true AND pendingEnvSelection=false,
so something is clobbering the flag through a path we don't yet
understand.

Rather than ship a fourth speculative root-cause fix, this commit adds
a structural fallback gate to Auth.isComplete: when pendingOrgs is
populated AND the resolved project has ≥ 2 environments with API keys
AND selectedEnvName is null, Auth.isComplete returns false regardless
of pendingEnvSelection's value. That predicate reads observable
session shape — pendingOrgs is set by resolveCredentials in the
deferred path, selectedEnvName is cleared by applyEnvSelectionDeferral
— so there's no mutable flag for a future regression to clobber. The
flag stays in place as the primary gate; the structural check is a
second, redundant safety net.

Scoped carve-outs:
- pendingOrgs===null short-circuits to false so the manual-API-key
  path (apiKeyNotice resolver outcome) passes through unchanged.
- Single-env projects don't trigger the gate (no picker needed).
- Stale checkpoint selectedProjectId not in pendingOrgs short-circuits
  to false; the existing stale-org useEffect in AuthScreen handles
  that case.

Regression coverage in src/ui/tui/__tests__/env-picker-repro.test.ts
includes the smoking-gun scenario (multi-env + selectedEnvName=null +
pendingEnvSelection=false → must still land on Auth) plus four
carve-out cases (env picked, manual key, single-env, stale checkpoint)
to guard the fix's scope. All six fail without the change and pass
with it.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kelsonpw added a commit that referenced this pull request May 15, 2026
…stale selectedOrgId/ProjectId (#797)

Prior PRs #747 / #760 / #762 / #775 / #778 / #780 each tried to fix the
recurring "stuck on Setup with no env-picker" hang after `git reset --hard`
on a previously-instrumented project. Each landed a gate that worked at
the unit-test level, but the live repro kept reproducing.

This pins the actual gap PR #780 left behind. PR #780's structural fallback
(`needsEnvPickStillRequired` → `pendingOrgs[0].projects[0]`) only fires
when BOTH `selectedOrgId` AND `selectedProjectId` are null. In the real
restart-after-reset sequence:

  1. `resolveCredentials` populates `pendingOrgs` (first fetch) and defers
     on multi-env.
  2. `applyEnvSelectionDeferral` sets `pendingEnvSelection=true`.
  3. AuthScreen mounts and its single-org/single-project auto-resolve
     effect writes `selectedOrgId='org-1'`, `selectedProjectId='proj-1'`
     from that first snapshot.
  4. `authTask` runs OAuth, `fetchAmplitudeUser` fetches a SECOND time,
     and `setOAuthComplete` REPLACES `pendingOrgs` with the fresh data.
     If the two snapshots' IDs don't match (re-fetch race, ordering,
     account changes, stale in-memory IDs), the existing
     `pendingOrgs.find(o => o.id === selectedOrgId)` lookup returns
     `undefined`.
  5. The structural gate short-circuits to `false`. If anything clobbers
     `pendingEnvSelection=false` (the recurring failure-mode the 6 prior
     PRs chased — every fix relied on the flag staying true), Auth.isComplete
     returns true and the router walks past Auth into the Setup-bucket
     screens. User sees `✓ Auth ─ ● Setup ←` with no env picker on
     screen.

Fix: when stale `selectedOrgId/ProjectId` don't resolve a project in
`pendingOrgs`, fall through to the same `pendingOrgs[0].projects[0]`
heuristic instead of bailing to `false`. The structural gate is now
load-bearing across all three states:

  (a) no IDs picked yet (PR #780)
  (b) IDs picked and valid in pendingOrgs (existing happy path)
  (c) IDs picked but stale relative to fresh pendingOrgs (this PR)

Regression test: src/ui/tui/__tests__/env-picker-restart-after-reset-hang.test.ts
drives the complete bin.ts startup sequence — buildSession → store.session →
resolveCredentials multi-env defer → applyEnvSelectionDeferral →
concludeIntro → setOAuthComplete → AuthScreen's setOrgAndProject — and
asserts the router parks on Auth at every step. The new "stale IDs"
test fails without the fix (router resolves to data-setup) and passes
with it.

Pre-existing test in env-picker-repro.test.ts that asserted the OPPOSITE
behavior ("does not block when the resolved project is missing from
pendingOrgs") was inverted to match the new (correct) behavior — that
test was inadvertently encoding the bug as expected behavior.

Co-authored-by: Claude Opus 4.7 (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.

1 participant