fix(tui): stabilize intro screen layout during startup detection#857
Draft
bird-m wants to merge 16 commits into
Draft
fix(tui): stabilize intro screen layout during startup detection#857bird-m wants to merge 16 commits into
bird-m wants to merge 16 commits into
Conversation
Signed-in users saw four distinct visual states in the ~1-2s after launching the wizard: marketing tagline → welcome-back panel → red Generic glyph with "No framework detected" → green framework glyph with "(detected) · beta". Layout jumped on every transition. Three root causes, all fixed in this change: 1. `userEmail` / `region` / `selectedProjectName` were only populated by `resolveCredentials` (async). The TUI rendered the marketing tagline for ~1s before swapping to the welcome-back panel. Fix: new sync prefill `prePopulateDisplayFields` reads the same display-only fields from local cache inside `buildSessionFromOptions`, before `startTUI()`. Uses `tryResolveZone` (not `resolveZone`) so missing-signal users still hit `RegionSelect`. 2. `runFrameworkDetection` performed four separate store mutations (`setDetectionResults`, `setFrameworkConfig`, `setDetectedFramework`, `setDetectionComplete`) with an `emitChange` after each. The render between `setDetectionComplete` and `setFrameworkConfig` saw `detectionComplete=true && frameworkConfig=null`, which fired the IntroScreen `useEffect` that auto-selects Generic — briefly clobbering the real framework with a red glyph before the real config landed. Fix: new `WizardStore.applyDetectionResult()` writes all four fields under one `emitChange`. The autoFallback gate is also hardened to require evidence (`detectionResults` populated with no winner) instead of trusting the `detectionComplete && !frameworkConfig` intermediate state. 3. The Framework row was conditional on `!detecting && frameworkLabel`, so it materialized only after detection settled — a visible vertical shift. Paired with a separate "Scanning ~/path…" line that was redundant with the Target row directly above. Fix: Framework row always renders; during detection it shows an inline spinner + "Detecting…" in the value column. Separate "Scanning…" line removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Belt-and-suspenders for the IntroScreen "Detecting…" state. Normal operation is bounded by DETECTION_TIMEOUT_MS (10s per detector, parallel) so wall-clock detection completes in ~10s. This adds a 15s guard that forces a Generic fallback if `detecting` is still true past that threshold — covering the cases the per-detector race can't catch: - A future refactor that flips detectionComplete=false without re-running the runner (e.g. checkpoint resume paths, changeInstallDir variants in contexts where the redetector isn't wired). - A hung network filesystem where every detector's own race is defeated. - Any out-of-band state where `applyDetectionResult` never lands. When the guard fires, it uses `applyDetectionResult` (not `setFrameworkConfig`) so detectionComplete=true flips alongside the Generic config — downstream gates unblock. If the real detection eventually lands after the fallback, its atomic apply overwrites the forced Generic with the real result. Telemetry: `framework detection stuck fallback` carries the elapsed duration so we can measure how often this triggers in production. Tests: two new cases in IntroScreen.snap.test.tsx pin both behaviors — the fallback fires after the threshold, and a real detection result landing inside the window prevents the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… too long" This reverts commit 91a6e4a.
prePopulateDisplayFields (introduced in 2ff3492) was running in --ci and --agent modes too, silently populating session.region from cached zone signals (stored user / ampli.json). That defeats the gates in gateAgentSignupArguments and gateCiSignupAcceptToS — both use `session.region == null` as the sentinel for "user didn't pass --region" and emit signup_input_required / exit INVALID_ARGS when that's true. Concrete misroute: a user running `wizard --auth-onboarding create-account --agent --email X --full-name Y --accept-tos` (no --region) on a machine with any stored zone signal would silently proceed against the cached zone. Since the BE does not route cross-region, an EU-intending signup gets created in the US data center. The prefill only exists to populate the welcomeBack panel on the TUI's first frame; non-interactive modes don't render that panel. Early-return when ci || agent is true, restoring the contract that session.region stays null until the user signals intent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The startup display prefill was writing session.region from cached zone signals (stored user, ampli.json), violating the documented invariant that session.region only reflects explicit user intent. Even with the non-interactive guard from the previous commit, interactive runs still loosened the contract. This commit restores the invariant by computing the displayed region in IntroScreen via tryResolveZone (disk-tier read, memoized per render- critical session field) and threading it through as `displayRegion` to the welcomeBack panel, TargetSummary region row, and "Change region" menu option. session.region itself stays null until the user signals intent through RegionSelect, /region, or a CLI flag. Behavior: a returning user with a stored zone now sees their region in the welcomeBack panel from frame 1 (same as before the contract violation) because IntroScreen reads disk directly via tryResolveZone. The RegionSelect flow gate already used tryResolveZone for its skip check, so returning users continue to bypass the region picker without the prefill writing session.region. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
End-state assertions on the existing applyDetectionResult tests pass even if a future refactor splits the writes back into separate setKey calls — the field values land the same way, just over four emitChange events instead of one. The whole point of the method is the single notification, since IntroScreen subscribers observing the mid-write state `detectionComplete=true && frameworkConfig=null` fire the autoFallback effect prematurely. This test subscribes directly to the store and counts notifications. A regression that splits the bundled write would push the count to four and fail loudly. Also adds a precedence test for the pre-set label case (matches the Flask / FastAPI gatherContext flow where a variant detector sets a more specific label before the atomic apply lands with the bare framework name). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pins the four properties the previous two commits established: 1. Interactive mode prefills userEmail + selectedProjectName from local cache so the TUI's first frame shows the welcomeBack panel. 2. Interactive mode does NOT write session.region (the contract restoration from d35ce51 — IntroScreen reads region via tryResolveZone instead). 3. CI mode (--ci) skips the prefill entirely. 4. Agent mode (--agent) skips the prefill entirely — particularly load- bearing since session.region == null is the sentinel gateAgentSignupArguments uses to detect a missing --region flag. Drive-by: switch the interactive guard inside prePopulateDisplayFields from `session.ci || session.agent` to a direct executionMode check. session.agent is set in default.ts AFTER buildSessionFromOptions returns, so the previous check would have missed agent invocations — the new tests caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two comments in default.ts described the old `setFrameworkConfig` → `setDetectedFramework` → `setDetectionComplete` pipeline that was collapsed into a single atomic `applyDetectionResult` in commit 2ff3492. Update both to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the manual framework picker from two back-to-back setKey calls (`setFrameworkConfig` + `setDetectedFramework`) to a single `applyDetectionResult` call, matching the discipline established for auto-detection. Harmless before this change because the autoFallback gate already required `detectionFoundNothing` evidence rather than the mid-write `detectionComplete && !frameworkConfig` state, but consistent with the contract a future gate-tightening would assume. The manual pick semantically differs from auto-detection in one way: the user explicitly chose this framework, so any previously-detected variant label (e.g. "Flask-RESTX" from a real detection that the user is now overriding to Next.js) must be replaced — not preserved as the default precedence rule would do. Extend applyDetectionResult with an optional `overwriteLabel: true` flag for this case; auto-detection continues to default to the precedence-preserving behavior so `gatherContext` variant labels still win over the bare framework name. Reuses the existing detectionResults so the diagnostics table doesn't get wiped by a manual pick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review flagged two concerns about the displayRegion useMemo: 1. tryResolveZone runs disk I/O (Tier 2: ampli.json Zone, Tier 3: stored user) inside a React render path, which useResolvedZone.ts explicitly forbids and instructs callers to hoist instead. 2. The useMemo deps include session.userEmail as a proxy for "stored user changed," but Tier 3 can in theory mutate without userEmail changing (same-email re-login picking a different zone). Both observations are correct. The proper fix is to hoist zone resolution into a WizardStore derived atom — tracked by the existing project_zone_hoist_followup memory note (originally filed against PR #165's Bugbot comment) — but that's a cross-cutting change touching useResolvedZone and four other screen consumers, deserves its own review cycle, and is out of scope for this layout-stability PR. This commit makes the carve-out explicit: - Substantial inline comment on the useMemo explains why IntroScreen needs Tier 2/3 (renders before RegionSelect, so session.region is null and `{ readDisk: false }` mode would yield the wrong zone for EU users with cached signals). - Documents the dep-array imperfection and why it's practically inaccessible (re-login navigates the user away from Intro before the mutation can be observed; failure mode is a stale displayed string, no routing impact). - Points future readers at the memory follow-up so the carve-out is known debt with a tracked resolution, not silent debt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When commit 2ff3492 introduced applyDetectionResult and routed the detection runner through it, four granular setters became unused via this interface: setFrameworkConfig, setDetectedFramework, setDetectionResults, and setDetectionComplete. They were still declared on DetectionTargetStore, which is misleading — a future test-double author reading the interface would implement four methods the runner never calls. Drop them from the interface (the real WizardStore still exposes them for other call sites like the manual framework picker and the variant- detector flows in src/frameworks/*/utils.ts; structural typing means passing a WizardStore that has MORE methods than the interface declares continues to satisfy the contract). Also refresh the stale `setDetectionComplete()` reference in the abort RunFrameworkDetectionOptions JSDoc to `applyDetectionResult()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The autoFallback useEffect (triggered when detection ran and found no framework) was the last branch still using setFrameworkConfig directly — a two-setKey write — instead of the atomic applyDetectionResult path the runner and manual picker now use. Harmless today because detectionComplete is already true when this fires, so the autoFallback gate (detectionFoundNothing) requires positive evidence that detection ran rather than trusting the mid-write window. But unifying the write path closes a stale-contract gap: a future refactor tightening the gate would have a quiet inconsistency to walk into. label: null is preserved (Generic is a fallback, not a detection — the render derives its display string from the config's metadata.name without a "(detected)" suffix). Reuses the existing detectionResults so the diagnostics table survives the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Generic autoFallback effect (commit 0b8fa7b) explicitly passes label:null because Generic is a fallback, not a detection. The guard inside applyDetectionResult — `if (input.label && (overwriteLabel || !this.session.detectedFrameworkLabel))` — short-circuits on null input, so the existing label is preserved. The precedence and overwriteLabel tests already pin the truthy-label branches. This commit closes the falsy-label branch: - label:null does not clobber an existing label (the autoFallback's load-bearing property). - label:null + overwriteLabel:true also does not clobber (pins the AND-shape of the guard — overwriteLabel is "replace with this new value," not "clear"). Without this, a future refactor that flips the guard to `(overwriteLabel || (input.label && !existing))` would let a null+overwrite call wipe the label silently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DetectionTargetStore was trimmed in 681fa3c to drop four granular setters that runFrameworkDetection no longer calls (subsumed by applyDetectionResult). The corresponding mock stubs in cli.test.ts remained, with zero references in any test body — they would have misled a future test author into thinking the CLI flow still calls those methods. Remove setFrameworkConfig, setDetectedFramework, setDetectionComplete, and setDetectionResults from the mockStore literal. The applyDetectionResult stub stays (it IS exercised — the feature-discovery tests at lines 815 and 825 wait on it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the refactor that routed production callers through applyDetectionResult (commits 2ff3492, 94fa5ad, 0b8fa7b), three of the four granular setters on WizardStore have no production callers: - setFrameworkConfig — only tests now - setDetectionComplete — only tests now - setDetectionResults — only tests now Annotate each with `@internal Tests-only` and a pointer to applyDetectionResult so a future caller doesn't reach for them and silently reintroduce the multi-emit window the atomic write was designed to close. setDetectedFramework is NOT annotated — InkUI delegates `getUI() .setDetectedFramework('Flask-RESTX')` calls from variant detectors (src/frameworks/flask/utils.ts, src/frameworks/fastapi/utils.ts) through to store.setDetectedFramework, so it has real production callers. Doc comment updated to record that explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
bugbot run |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Auto-fallback overwrites manual framework pick on remount
- Added the
!session.frameworkConfigguard (and matching dep) back to the auto-fallbackuseEffectso a remount after a manual framework pick can't overwrite the selection with Generic.
- Added the
Or push these changes by commenting:
@cursor push e93b142757
Preview (e93b142757)
diff --git a/src/ui/tui/screens/IntroScreen.tsx b/src/ui/tui/screens/IntroScreen.tsx
--- a/src/ui/tui/screens/IntroScreen.tsx
+++ b/src/ui/tui/screens/IntroScreen.tsx
@@ -204,8 +204,20 @@
// Atomic apply (same discipline as the runner and the manual picker)
// keeps every detection-related write going through one path. Reuses
// the existing detectionResults so the diagnostics table survives.
+ //
+ // The `!session.frameworkConfig` guard is load-bearing: a manual pick
+ // via FrameworkPicker leaves `detectionResults` unchanged (no winner),
+ // so `detectionFoundNothing` stays true after the user chose e.g.
+ // Next.js. Without this gate, a component remount (ScreenErrorBoundary
+ // retry) would re-fire the effect and overwrite the manual selection
+ // with Generic.
useEffect(() => {
- if (detectionFoundNothing && !session.menu && !showResume) {
+ if (
+ detectionFoundNothing &&
+ !session.frameworkConfig &&
+ !session.menu &&
+ !showResume
+ ) {
void import('../../../lib/registry.js').then(({ FRAMEWORK_REGISTRY }) => {
const genericConfig = FRAMEWORK_REGISTRY[Integration.generic];
store.applyDetectionResult({
@@ -217,7 +229,12 @@
logToFile('[intro] no framework matched — falling back to Generic');
});
}
- }, [detectionFoundNothing, session.menu, showResume]);
+ }, [
+ detectionFoundNothing,
+ session.frameworkConfig,
+ session.menu,
+ showResume,
+ ]);
const showContinue =
session.frameworkConfig !== null && !detecting && !pickingFramework;You can send follow-ups to the cloud agent here.
Bugbot flagged a regression in the autoFallback effect's gate. The detectionFoundNothing predicate I introduced (commit 0b8fa7b's companion in 2ff3492) replaced the old `needsFrameworkPick = detectionComplete && !frameworkConfig` with a stricter "positive evidence" check, but dropped the `!frameworkConfig` half of the original guard in the process. Trace: 1. Detection completes with no winner → applyDetectionResult(null) → detectionResults populated, frameworkConfig still null. 2. detectionFoundNothing = true → autoFallback fires → Generic. 3. User picks "Change framework" → manual picker writes applyDetectionResult({integration: nextjs, ..., results: store.session.detectionResults ?? []}). Manual picks preserve the diagnostics table; detectionResults still has no winner. 4. detectionFoundNothing STILL true (the predicate only looked at detectionComplete + detectionResults). 5. IntroScreen remounts — ScreenErrorBoundary retry, any boundary- surfaced render error — and the useEffect re-fires. 6. Generic clobbers the user's manual pick. Fix: re-introduce !session.frameworkConfig in detectionFoundNothing. Both guards now apply: positive evidence (results.length > 0 && !some.detected) AND no framework currently set. Initial autoFallback fire works the same way (frameworkConfig is null at that point); post-manual-pick remounts bail because frameworkConfig is set. Test: simulates the manual-pick + remount shape and asserts the user's selection survives. Would fail before this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
bugbot run |
Contributor
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6a0250d. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Signed-in users saw four distinct visual states in the ~1-2s after launching the wizard: marketing tagline → welcome-back panel → red Generic glyph with "No framework detected" → green framework glyph with "(detected) · beta". Layout jumped on every transition.
Three root causes, all fixed:
userEmail/selectedProjectNamewere populated byresolveCredentials(async), so the TUI rendered the marketing tagline for ~1s before swapping to the welcome-back panel. Added a sync prefill (prePopulateDisplayFields) that runs insidebuildSessionFromOptionsand reads the same fields from local cache, gated to interactive mode only. Region is intentionally NOT prefilled —session.regionis reserved for explicit user intent, and populating it from cache silently defeatsgateAgentSignupArguments/gateCiSignupAcceptToS(would misroute signup into the wrong DC). IntroScreen instead reads the displayed region viatryResolveZonedirectly (documented carve-out from theuseResolvedZone"no disk in render" rule; long-term fix tracked inproject_zone_hoist_followupmemory).runFrameworkDetectionperformed four separate store mutations (setDetectionResults,setFrameworkConfig,setDetectedFramework,setDetectionComplete) with anemitChangeafter each. The render window betweensetDetectionCompleteandsetFrameworkConfigsawdetectionComplete=true && frameworkConfig=null, which fired IntroScreen'suseEffectthat auto-selects Generic — briefly clobbering the real framework with a red glyph before the real config landed. IntroducedWizardStore.applyDetectionResult()that writes all five fields under a singleemitChange. Also hardened the autoFallback gate to require positive evidence (detectionResultspopulated with no winner) instead of trusting thedetectionComplete && !frameworkConfigintermediate state.!detecting && frameworkLabel, so it appeared only after detection settled — a visible vertical shift, paired with a separate "Scanning ~/path…" line that duplicated the Target row. Framework row now always renders; during detection it shows an inline spinner + "Detecting…" in the value column.The atomic-write discipline is now applied uniformly: detection runner, manual framework picker, and the Generic autoFallback effect all route through
applyDetectionResult. AnoverwriteLabelflag distinguishes the manual-pick case (user explicitly replaces the label) from auto-detection (preserves variant labels like "Flask-RESTX").Test plan
pnpm test— full suite passes (3234 tests)pnpm lint— cleanpnpm build— type check + smoke test passpnpm tryas a signed-in user; verify the intro screen renders the welcome-back panel + Target/Framework rows in a single stable layout (no marketing-tagline flash, no red-glyph intermediate state, no Framework row materializing late)pnpm tryas a signed-out user; verify the marketing tagline still appears (welcomeBack is signed-in-only)wizard --auth-onboarding create-account --agent --email X --full-name Y --accept-tos(no --region) on a machine with a stored zone; verify it still emitssignup_input_requiredfor--regionrather than silently using the cached zoneNotable commits
2ff34925— root layout-stability fix (atomic write + Framework row reservation + prefill)fdf56c85— silent-misroute fix for agent/CI signup (skip prefill in non-interactive modes)d35ce512— keepsession.regionoff the prefill write list; read viatryResolveZonein IntroScreen0b8fa7b1/94fa5ad1— atomic-write discipline extended to autoFallback + manual picker25228cf6— reverted an earlier "stuck-detection timeout" guard after review surfaced a mid-flow framework-swap race; the existing atomic-write + autoFallback hardening already cover realistic failure modes🤖 Generated with Claude Code
Note
Medium Risk
Touches wizard startup/detection state handling and IntroScreen effects; mistakes could regress framework selection or region gating, though changes are well-covered by new unit/snapshot tests.
Overview
Stabilizes the IntroScreen startup experience by avoiding transient states during framework detection and by reserving the Framework row layout while detection runs.
Framework detection now commits results via a single atomic
store.applyDetectionResult(...)call (used byrunFrameworkDetection, the Generic auto-fallback, and the manual framework picker), preventing subscribers from seeing partial mid-write state and tightening the auto-fallback gate to only trigger when detection produced non-empty results with no winner.Adds an interactive-only cache prefill in
buildSessionFromOptionsto populate display fields (userEmail,selectedProjectName) for the first TUI frame without writingsession.region; IntroScreen instead resolves a display-only region viatryResolveZone. Tests were updated/added to pin the new atomic detection contract and the prefill/region invariants, plus snapshot updates for the new inline “Detecting…” UI.Reviewed by Cursor Bugbot for commit 6a0250d. Bugbot is set up for automated code reviews on this repo. Configure here.