Repository navigation
refactor: put the tui store on top of the session store - #1381
Conversation
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
3521f0c to
e5138a9
Compare
1e0bc81 to
45ecdf8
Compare
e5138a9 to
fd7ec0e
Compare
45ecdf8 to
f7978fd
Compare
f7978fd to
8883b18
Compare
fd7ec0e to
2b4fdc6
Compare
8883b18 to
e757985
Compare
2b4fdc6 to
62de363
Compare
johncwaters
left a comment
johncwaters
left a comment
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
| store.pushStatus('Signing you in to your new account instead...'); | ||
|
|
||
| return askForWizardLogin({ signup: false, baseUrl, localMcp }); | ||
| return askForWizardLogin({ store, signup: false, baseUrl, localMcp }); |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
[convergent: code + security + structure] MEDIUM
askForProvisioningSignup now takes programId and scopeAdditions, but both fallbacks into askForWizardLogin (here and at line 390) pass only { store, signup: false, baseUrl, localMcp }. When the provisioned account cannot be read back, or the email already has an account, the browser login asks for the bare WIZARD_OAUTH_SCOPES instead of the program's or tool's wider set, so later scope-gated calls fail or land in missingScopes. Forward programId and scopeAdditions (and projectId) in both calls.
| */ | ||
| reachStep(stepId: string): Promise<boolean> { | ||
| const program = this.router.activeProgram; | ||
| const steps = withAiOptInGate( |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
[convergent: code + security + structure] MEDIUM
withAiOptInGate(findProgramConfig(program), flowOwner(program).flow) is built separately in _initFromProgram (line 143), reachStep (here) and programSequence in src/tui/screen-sequences.ts:41, and reachStep re-derives the isComplete ?? gate default that createProgramSequence in flow.ts owns. The comment at line 139 says the AI opt-in gate and its screen "can't drift apart", but only three identical expressions keep that true. Extract one helper (for example programFlowSteps(programId)) and use it in all three places.
| * writes its run state to the session store, so its progress goes through | ||
| * `displayProgress`. | ||
| */ | ||
| export function scanProgress( |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
[structure/duplication] MEDIUM
scanProgress (with completeRun and setCompletionOutro) re-implements the run-state half of applyAgentProgress in src/programs/session/session-store.ts:551: lifecycle start/complete with the fallback success outro, status, tasks, url, handoff, and the live-URL-wins completion merge. The two already differ (ANSI stripping, batching), and the next change to the outro fallback or URL precedence will land in one only. Build scanProgress as applyAgentProgress(store.sessions, event) plus displayProgress(store)({ event }), move ANSI stripping into applyAgentProgress, and delete the copies.
| @@ -0,0 +1,106 @@ | |||
| /** | |||
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
[code/duplication] LOW
Nothing imports this new registry or src/tui/screen-sequences.ts at this head: src/tui/App.tsx:4 still imports createScreens/createServices from ../ui/tui/screen-registry.js, and the old src/ui/tui/screen-registry.tsx, screen-sequences.ts and ink-ui.ts still exist. Repoint App.tsx and delete the old files here, or confirm a later PR in the stack does.
e757985 to
76921c7
Compare
62de363 to
0ecdbe0
Compare
76921c7 to
9eb3762
Compare
0ecdbe0 to
7597ed2
Compare
Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
9eb3762 to
e6300c4
Compare
7597ed2 to
581e05a
Compare
edwinyjlim
left a comment
edwinyjlim
left a comment
There was a problem hiding this comment.
Review of #1381 at 581e05ae. Comments inline.
Drafted with Claude Code from a review of the whole Release C stack, re-verified at the stack tip 2e31e438. Line numbers are at this PR's head.
| return true; | ||
| case 'service-outage': | ||
| // The health-check screen shows the outage; the run stops. | ||
| store.setReadinessResult(step.readiness); |
There was a problem hiding this comment.
[P2] The service-outage step re-writes a readiness result runProgram already stored, so health check blocked is captured twice
checkReadiness calls store.setReadinessResult on the shared store before asking the workflow; the connector then calls it again on the same store. The setter runs captureHealthCheckBlocked on every blocked result, so one blocked start sends two events. This hits every flow without a health-check step (warehouse-source, source-maps).
Suggested fix: Delete the setReadinessResult line in the service-outage case and just return false; the screens read the same store. Or make the capture fire once per result object.
| * writes its run state to the session store, so its progress goes through | ||
| * `displayProgress`. | ||
| */ | ||
| export function scanProgress( |
There was a problem hiding this comment.
[P3] scanProgress keeps lifecycle and completion branches no scan can reach and that would derail the flow if one did
Partly fixed since the last push. The duplicate reducer is gone and scanProgress now runs through applyAgentProgress. A forwarded completion would still mark the run Completed before the run step. Unreachable today.
Every detect screen's scan goes through detectProjectsWithAgent, which drops lifecycle, completion and spinner events. If a future detector forwarded them, a scan would set the run phase to Completed and a success outro before the run step, and the router would skip the run screen.
Suggested fix: Treat those kinds as no-ops in scanProgress and remove completeRun and setCompletionOutro, or throw on them so a detector change surfaces in tests.
| /** | ||
| * Core screen ids + per-program screen sequences. | ||
| * | ||
| * Owns the core ScreenId enum and projects a program's flow into the |
There was a problem hiding this comment.
[P3] The screen-sequences header still says it owns the core ScreenId enum
Suggested fix: Reword to "re-exports the core ScreenId and projects a program's flow".
Phase 2 of Release C (#1320), PR 5 of 16, after #1380. Review PR. PRs 1 to 14 of this phase split one change into reviewable pieces and compile only as a stack: review them one by one, then land the whole phase together.
Problem
On main, the TUI's
WizardStoreowns the whole session, screen answers included, and the TUI reaches the rest of the wizard through the process-wideWizardUI(InkUI). The session store from #1376 needs the TUI on top of it, not around it.Changes
WizardStore(src/tui/store.ts) wraps aSessionStore(store.sessions) and addsTuiState(tui-state.ts): the screens' answers, overlay contents and launch choices. Screens end a run withstore.requestExit(code), and the host readsexitRequest.tuiWorkflow(workflow.ts) answers the stepsrunProgramwaits on: the AI opt-in gate, the outage screen, the settings override, andreachStepbefore each run.FlowStep(flow.ts) replacesProgramStepfor screen flows. Newscreen-registry.tsxandscreen-sequences.tsatsrc/tuimount the core screens and take each program's from its TUI entry.WizardRouter.resolvereads aTuiView.displayProgressandscanProgress(agent-progress.ts) put run and scan events on the screens. The browser login insrc/tui/auth(oauthLogin,getOrAskForProjectData, their code moved in refactor: move every file into its layer #1375) becomesrunProgram's credentials provider throughoauthCredentials(login.ts), andstartTUIno longer installs aWizardUIor exits.addTokenUsage(token-usage.ts) adds each turn's usage to the token HUD's estimate and leaves a reconciled total as it is.store.tsandtui-state.ts: which fields left the session, and that each write still re-resolves the screen and the gates.Test plan
pnpm typecheck0 errors,pnpm lint0 errors, 236 test files (3,515 tests) pass,pnpm buildok.Created with PostHog Desktop