feat: improve lifecycle scripts#1886
Conversation
Davidknp
changed the title
Greptile SummaryThis PR improves the task provisioning lifecycle by surfacing the setup script's PTY output directly in the bootstrap loading screen, adds a "Skip" button to abort a running setup script, and introduces Storybook infrastructure for renderer component development.
Confidence Score: 4/5The feature-level changes are well-structured and the critical setup-script PTY flow is correct; minor style and dead-code concerns exist but nothing blocks safe merging. The main provisioning path — prepare PTY, emit sessionId, show terminal in the UI, wait for exit — looks correct. The two areas worth watching are the exit && !waitForExit branch in LifecycleScriptService.runLifecycleScript (currently unreachable from any call site, so its multi-call behavior is unverified) and the use of non-null assertions in the bootstrap components against the project's explicit null-check convention. Neither is a blocking correctness issue today. src/main/core/workspaces/workspace-lifecycle-service.ts (dead exit-listener branch) and src/renderer/features/tasks/main-panel.tsx (non-null assertions)
|
| Filename | Overview |
|---|---|
| src/main/core/workspaces/workspace-lifecycle-service.ts | New LifecycleScriptService class refactors lifecycle script execution into prepare/run/prepareAndRun steps; the exit && !waitForExit code path is currently unreachable from any call site |
| src/main/core/workspaces/workspace-factory.ts | Wires the new LifecycleScriptService into workspace creation; emits setup script sessionId for PTY rendering; resolveIds is called redundantly after prepareLifecycleScript |
| src/renderer/features/tasks/main-panel.tsx | CreatingBootstrap and ProvisioningBootstrap now show a live PTY pane and Skip button during running-setup-script; uses non-null assertions on taskStore and setupSessionId against project conventions |
| src/renderer/features/tasks/task-bootstrap-pty.tsx | New file: isolates Electron IPC PTY rendering (BootstrapPtyView) and skip control (PtySkipButton) from Storybook-importable bootstrap view |
| src/renderer/lib/ui/stepped-loading-screen.tsx | New animated loading screen component with animated step label transitions and collapsible children area for PTY output |
| src/shared/events/taskEvents.ts | Adds optional sessionId field to taskProvisionProgressChannel so the renderer can connect a PTY pane to the setup script session |
Sequence Diagram
sequenceDiagram
participant F as workspace-factory
participant L as LifecycleScriptService
participant R as PtySessionRegistry
participant E as events (IPC)
participant TM as TaskManagerStore
participant UI as main-panel (renderer)
F->>L: prepareLifecycleScript(setup)
L->>R: spawnLifecycleScript - register sessionId+pty
F->>L: resolveIds(setup) to get sessionId
F->>E: emit taskProvisionProgressChannel with running-setup-script + sessionId
E-->>TM: store.setupSessionId = sessionId
TM-->>UI: re-render - isPtyStep becomes true
UI->>UI: render BootstrapPtyView + PtySkipButton
F->>L: runLifecycleScript(setup, waitForExit=true, exit=true)
L->>R: pty.write command
Note over UI: User sees live terminal output
opt User clicks Skip
UI->>E: rpc.pty.kill(sessionId)
end
R-->>L: pty-exit hook fires - exitPromise resolves
F->>L: setup done - proceeds to onCreate hooks
Comments Outside Diff (1)
-
src/main/core/workspaces/workspace-lifecycle-service.ts, line 97-102 (link)exit && !waitForExitbranch is dead code with an unverified multi-call invariantThe
exit: true, waitForExit: falsecombination is never passed by any call site in this PR —workspace-factory.tsalways uses{ waitForExit: true, exit: true }. When this path is eventually exercised (e.g. to auto-restart therunscript on exit), callingrunLifecycleScripta second time before the PTY exits would register a secondonExitlistener. IfPty.onExitis an additive emitter (rather than single-slot), both listeners would fire, triggering two concurrentprepareLifecycleScriptcalls and a duplicate PTY spawn.Prompt To Fix With AI
This is a comment left during a code review. Path: src/main/core/workspaces/workspace-lifecycle-service.ts Line: 97-102 Comment: **`exit && !waitForExit` branch is dead code with an unverified multi-call invariant** The `exit: true, waitForExit: false` combination is never passed by any call site in this PR — `workspace-factory.ts` always uses `{ waitForExit: true, exit: true }`. When this path is eventually exercised (e.g. to auto-restart the `run` script on exit), calling `runLifecycleScript` a second time before the PTY exits would register a second `onExit` listener. If `Pty.onExit` is an additive emitter (rather than single-slot), both listeners would fire, triggering two concurrent `prepareLifecycleScript` calls and a duplicate PTY spawn. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/renderer/features/tasks/main-panel.tsx:71-99
**Non-null assertions on `taskStore` and `setupSessionId`**
Both `CreatingBootstrap` and `ProvisioningBootstrap` use `taskStore!.setupSessionId!` inside JSX ternaries guarded by `isPtyStep`. While the guard makes this safe at runtime (if `isPtyStep` is falsy the truthy branch is never evaluated), the pattern violates the project's explicit convention — AGENTS.md states to use explicit null checks, never `!`. The same pattern appears in both components.
### Issue 2 of 3
src/main/core/workspaces/workspace-lifecycle-service.ts:97-102
**`exit && !waitForExit` branch is dead code with an unverified multi-call invariant**
The `exit: true, waitForExit: false` combination is never passed by any call site in this PR — `workspace-factory.ts` always uses `{ waitForExit: true, exit: true }`. When this path is eventually exercised (e.g. to auto-restart the `run` script on exit), calling `runLifecycleScript` a second time before the PTY exits would register a second `onExit` listener. If `Pty.onExit` is an additive emitter (rather than single-slot), both listeners would fire, triggering two concurrent `prepareLifecycleScript` calls and a duplicate PTY spawn.
### Issue 3 of 3
src/main/core/workspaces/workspace-factory.ts:173-194
**`resolveIds` called twice for the same setup script**
Inside `onCreate`, `prepareLifecycleScript` is awaited first (which internally calls `resolveIds` / `createScriptTerminalId` to derive the terminal and session IDs), then `resolveIds` is called again explicitly just to get the `sessionId` for the progress event. Since `createScriptTerminalId` is a pure, deterministic hash, the second call always produces the same result. It would be cleaner to have `prepareLifecycleScript` return the `sessionId`, or to call `resolveIds` once before `prepareLifecycleScript` and reuse it.
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isPtyStep = step === 'running-setup-script' && taskStore?.setupSessionId; | ||
|
|
||
| if (kind === 'teardown-error') { | ||
| return ( | ||
| <div className="flex h-full w-full flex-col items-center justify-center p-8"> | ||
| <div className="flex max-w-xs flex-col items-center text-center gap-2"> | ||
| <p className="text-sm font-medium font-mono text-foreground-destructive"> | ||
| Failed to tear down workspace | ||
| </p> | ||
| <p className="text-xs font-mono text-foreground-muted">{taskErrorMessage(taskStore)}</p> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
| return ( | ||
| <TaskBootstrapView | ||
| step={step} | ||
| actions={isPtyStep ? <PtySkipButton sessionId={taskStore!.setupSessionId!} /> : undefined} | ||
| > | ||
| {isPtyStep ? <BootstrapPtyView sessionId={taskStore!.setupSessionId!} /> : undefined} | ||
| </TaskBootstrapView> | ||
| ); | ||
| }); | ||
|
|
||
| if (kind === 'missing') { | ||
| return null; | ||
| } | ||
| const ProvisioningBootstrap = observer(function ProvisioningBootstrap({ | ||
| taskStore, | ||
| }: { | ||
| taskStore: TaskStore | undefined; | ||
| }) { | ||
| const rawStep = taskStore?.provisionStep ?? 'setting-up-workspace'; | ||
| const step = useDebouncedValue(rawStep, STEP_DEBOUNCE_MS); | ||
| const isPtyStep = step === 'running-setup-script' && taskStore?.setupSessionId; | ||
|
|
||
| return <ReadyTaskMainPanel />; | ||
| return ( | ||
| <TaskBootstrapView | ||
| step={step} | ||
| actions={isPtyStep ? <PtySkipButton sessionId={taskStore!.setupSessionId!} /> : undefined} | ||
| > | ||
| {isPtyStep ? <BootstrapPtyView sessionId={taskStore!.setupSessionId!} /> : undefined} | ||
| </TaskBootstrapView> | ||
| ); |
There was a problem hiding this comment.
taskStore and setupSessionId
Both CreatingBootstrap and ProvisioningBootstrap use taskStore!.setupSessionId! inside JSX ternaries guarded by isPtyStep. While the guard makes this safe at runtime (if isPtyStep is falsy the truthy branch is never evaluated), the pattern violates the project's explicit convention — AGENTS.md states to use explicit null checks, never !. The same pattern appears in both components.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/main-panel.tsx
Line: 71-99
Comment:
**Non-null assertions on `taskStore` and `setupSessionId`**
Both `CreatingBootstrap` and `ProvisioningBootstrap` use `taskStore!.setupSessionId!` inside JSX ternaries guarded by `isPtyStep`. While the guard makes this safe at runtime (if `isPtyStep` is falsy the truthy branch is never evaluated), the pattern violates the project's explicit convention — AGENTS.md states to use explicit null checks, never `!`. The same pattern appears in both components.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=c9624c9f-4bb0-42b3-aa19-4ea456b59255))
How can I resolve this? If you propose a fix, please make it concise.| onCreate: async (ws) => { | ||
| if (scripts?.setup) { | ||
| await ws.lifecycleService.prepareLifecycleScript({ | ||
| type: 'setup', | ||
| script: scripts.setup, | ||
| }); | ||
| const { sessionId } = await ws.lifecycleService.resolveIds({ | ||
| type: 'setup', | ||
| script: scripts.setup, | ||
| }); | ||
| events.emit(taskProvisionProgressChannel, { | ||
| taskId: context.task.id, | ||
| projectId: context.projectId, | ||
| step: 'running-setup-script', | ||
| message: 'Running setup script…', | ||
| sessionId, | ||
| }); | ||
| await ws.lifecycleService.runLifecycleScript( | ||
| { type: 'setup', script: scripts.setup }, | ||
| { waitForExit: true, exit: true } | ||
| ); | ||
| } |
There was a problem hiding this comment.
resolveIds called twice for the same setup script
Inside onCreate, prepareLifecycleScript is awaited first (which internally calls resolveIds / createScriptTerminalId to derive the terminal and session IDs), then resolveIds is called again explicitly just to get the sessionId for the progress event. Since createScriptTerminalId is a pure, deterministic hash, the second call always produces the same result. It would be cleaner to have prepareLifecycleScript return the sessionId, or to call resolveIds once before prepareLifecycleScript and reuse it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/workspaces/workspace-factory.ts
Line: 173-194
Comment:
**`resolveIds` called twice for the same setup script**
Inside `onCreate`, `prepareLifecycleScript` is awaited first (which internally calls `resolveIds` / `createScriptTerminalId` to derive the terminal and session IDs), then `resolveIds` is called again explicitly just to get the `sessionId` for the progress event. Since `createScriptTerminalId` is a pure, deterministic hash, the second call always produces the same result. It would be cleaner to have `prepareLifecycleScript` return the `sessionId`, or to call `resolveIds` once before `prepareLifecycleScript` and reuse it.
How can I resolve this? If you propose a fix, please make it concise.1 participant
No description provided.