fix(web): destroy terminal shells on tab close with busy confirmation#1203
Conversation
Closing a terminal tab now stops the PTY and removes the shell instead of parking it in the background. A confirmation dialog appears when a command looks like it is still running. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR implements conditional terminal close confirmation by detecting terminal activity via busy-state registry tracking, replacing unconditional park/stop behavior with destroy, and integrating confirmation prompts across task-right-panel, terminal-tab, and mobile UI layers. ChangesTerminal Close Confirmation Refactor
Sequence DiagramsRight Panel Close Request FlowsequenceDiagram
participant UI
participant Confirmable
participant Registry
UI->>Confirmable: handleAskCloseTab
Confirmable->>Registry: shouldConfirmTerminalClose
Registry-->>Confirmable: needsConfirm
Confirmable->>UI: setPendingClose
UI->>UI: show dialog
UI->>Confirmable: handleConfirmClose
Confirmable->>Confirmable: destroyTerminal
Busy Tracking LifecyclesequenceDiagram
participant Component as Passthrough
participant Hook as Tracking
participant Terminal as xterm
participant Registry as busy-registry
Component->>Hook: enable with ref
Hook->>Terminal: wait for ready
Terminal-->>Hook: ready
Hook->>Terminal: subscribe onData
Hook->>Terminal: subscribe onWriteParsed
Terminal->>Hook: onData event
Hook->>Registry: markInput
Terminal->>Hook: onWriteParsed event
Hook->>Registry: markOutput
Component->>Hook: unmount
Hook->>Registry: clearBusy
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @carlosflorencio's task in 4m 24s —— View job Code Review
SummaryGood PR overall. The core mechanic — xterm The key discovery from reading the API layer: Findings (5 inline comments):
The missing |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/web/lib/terminal/terminal-busy-registry.ts | New module-level busy-state registry with PROMPT_TAIL regex; well-tested edge cases (blank lines, heredoc) are handled correctly and the SSR guard is in place. |
| apps/web/components/task/use-terminal-busy-tracking.ts | New hook wires xterm input/output to the registry; rAF retry handles late-attach correctly, though the loop is unbounded if xtermRef.current never resolves while mounted. |
| apps/web/components/task/terminal-tab.tsx | Dockview X-button close path is correctly intercepted via closeActionOverride; destroyAndClosePanel marks the panel before calling props.api.close() to prevent the fallback double-stop. The testid attachment via querySelector(".dv-default-tab-action") is coupled to dockview's internal CSS class. |
| apps/web/components/task/close-terminal-confirm-dialog.tsx | Shared confirm dialog extracted cleanly; void onConfirm() silently swallows async rejections, leaving the dialog open without any user feedback on failure. |
| apps/web/components/task/task-right-panel.tsx | Logic split into useTaskRightPanel hook and useConfirmableTerminalClose; confirmation and immediate-destroy paths correctly delegate to destroyTerminal or the original handleCloseTab. |
| apps/web/components/task/mobile/mobile-terminals-section.tsx | Migrated to shared CloseTerminalConfirmDialog and shouldConfirmTerminalClose; taskId is now threaded correctly to stopUserShell for audit purposes. |
| apps/web/components/task/dockview-layout-setup.ts | Fallback handleTerminalPanelClosed now always calls stopUserShell and removes the shell from the store; park path removed; terminate-mark skip mechanism preserved. |
| apps/web/hooks/domains/session/use-terminals.ts | Removed parkUserShell path from useCloseTab; ordinary terminals now go through destroyUserShell on close, consistent with the new PR policy. |
| apps/web/e2e/tests/terminal/terminal-dockview-ui.spec.ts | E2E test correctly inverted: now asserts the closed terminal does NOT reappear in the + menu (destroy) instead of asserting it does (park). |
Sequence Diagram
sequenceDiagram
participant U as User
participant DV as TerminalTab (dockview X)
participant RP as Right-Panel Strip X
participant MB as Mobile Picker X
participant Reg as terminal-busy-registry
participant API as user-shell-api
participant Store as App Store
participant DL as dockview-layout-setup
U->>DV: click X
DV->>Reg: "shouldConfirmTerminalClose(id, {kind})"
alt busy or script
Reg-->>DV: true
DV->>U: show CloseTerminalConfirmDialog
U->>DV: confirm
else idle
Reg-->>DV: false
end
DV->>API: destroyUserShell(env, id)
API-->>DV: ok
DV->>Store: removeUserShell(env, id)
DV->>DL: markTerminalPanelTerminateClose(panelId)
DV->>DL: props.api.close() → onDidRemovePanel
DL->>DL: sees terminate mark → skip stopUserShell
U->>RP: click X
RP->>Reg: "shouldConfirmTerminalClose(id, {type,kind})"
alt busy or script
Reg-->>RP: true
RP->>U: show CloseTerminalConfirmDialog
U->>RP: confirm
RP->>API: destroyTerminal(id)
else idle
Reg-->>RP: false
RP->>API: destroyUserShell(env, id)
end
API-->>RP: ok
RP->>Store: removeUserShell + removeTerminal
U->>MB: click X
MB->>Reg: "shouldConfirmTerminalClose(id, {type,kind})"
alt busy or script
Reg-->>MB: true
MB->>U: show CloseTerminalConfirmDialog
U->>MB: confirm
else idle
Reg-->>MB: false
end
MB->>API: stopUserShell(env, id)
API-->>MB: ok
MB->>Store: removeTerminal
Reviews (7): Last reviewed commit: "fix(web): tighten prompt regex bare-sigi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Thread taskId through mobile destroy, unify script/busy confirm gating, fix blank-line busy false-positive, and harden busy-tracking attach. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 20s —— View job Code Review
SummaryAll issues from the first review round were addressed in Blocker: Suggestion: |
FindingsAll issues flagged in the first review round have been addressed in `e1a08ac` (empty-line false-positive, script-terminal detection, mobile `taskId`, SSR guard, `beforeEach` leak, exhaustive-deps comment, blank terminal name). Two remaining items from this pass: Blocker (must fix before merge)Missing tests for new logic
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — add the missing hook test, then ready to merge. |
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Use initialCommand in confirm gating, pass taskId to useUserShells on mobile, and add use-terminal-busy-tracking hook tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 5m 57s —— View job Code Review
FindingsAll issues from the prior two review rounds are resolved. Three new findings from this pass: Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. No blockers — all prior review findings were addressed. The three suggestions above are polish; the file-length one (#3) is worth acting on soon to avoid a future CI surprise. |
Keep confirm dialog open until destroy settles, update stale park doc, and extract right-panel tab contents to stay under file line cap. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 10s —— View job Code Review
All prior findings from rounds 1–3 were addressed in |
FindingsAll findings from the prior three review rounds have been addressed in `1682bb1`. One new suggestion from this pass: Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. The suggestion above is a narrow edge case affecting legacy shells during the transition period; it doesn't block shipping. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/task/dockview-layout-setup.ts (1)
350-369:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix tab-close JSDoc to match actual API behavior
File: apps/web/components/task/dockview-layout-setup.ts (lines 350-369)
stopUserShellis an alias ofdestroyUserShell(both calluser_shell.destroy, with a fallback touser_shell.stopon unknown action), so the close-path semantic mismatch claim doesn’t hold.- The JSDoc “DB row removed” is not unconditionally true when the backend falls back to
user_shell.stop; adjust the wording to reflect the fallback (e.g., “destroy terminal; backend may fall back to stop”).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/task/dockview-layout-setup.ts` around lines 350 - 369, Update the JSDoc above handleTerminalPanelClosed to accurately reflect actual API behavior: note that stopUserShell is an alias of destroyUserShell (both delegate to user_shell.destroy and may fall back to user_shell.stop), so change the comment from “DB row removed” to something like “destroys terminal; backend may fall back to stop (DB row removal not guaranteed)”, and clarify that the handler skips already-terminated panels via terminalTerminateClosePanelIds; keep references to stopUserShell/destroyUserShell/user_shell.destroy/user_shell.stop so readers can see the exact behavior.
🧹 Nitpick comments (3)
apps/web/lib/terminal/terminal-busy-registry.ts (1)
42-47: ⚡ Quick winShort-circuit
markTerminalOutputwhen the terminal is already idle.
onWriteParsedfires on every parsed write, so this runsbufferLooksAtPrompt(buffer read +stripAnsi+ regex) on every output frame — including high-throughput streams like build logs — even when the terminal is not busy. The only meaningful transition here is busy→idle, so skipping the prompt check while idle avoids work on the I/O hot path with no behavior change.⚡ Proposed optimization
export function markTerminalOutput(terminalId: string, terminal: Terminal): void { if (typeof window === "undefined") return; + if (!busyByTerminalId.get(terminalId)) return; if (bufferLooksAtPrompt(terminal)) { busyByTerminalId.set(terminalId, false); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/terminal/terminal-busy-registry.ts` around lines 42 - 47, markTerminalOutput runs bufferLooksAtPrompt on every onWriteParsed event causing unnecessary work; add an early-return when the terminal is already marked idle to avoid re-checking prompts. Specifically, in markTerminalOutput check busyByTerminalId for terminalId (e.g., busyByTerminalId.get(terminalId) === false or !busyByTerminalId.has/true) and return immediately if not busy, otherwise proceed to call bufferLooksAtPrompt and set the map to false on transition; this ensures only busy→idle transitions invoke the expensive buffer/regex work.apps/web/hooks/domains/session/use-terminals.ts (1)
286-306: ⚡ Quick win
useCloseTabduplicatesuseManagedTerminalActions.destroyTerminal.Both now perform the exact same sequence —
destroyUserShell(env, id, taskID) → removeUserShellStore(env, id) → removeTerminal(id)— differing only in promise-chain vs async/await style. Consider extracting a shareddestroyAndRemove(environmentId, terminalId, taskID)helper so the close-tab and destroy paths can't drift apart.♻️ Sketch
function destroyAndRemove( environmentId: string, terminalId: string, taskID: string | null, removeUserShellStore: (e: string, t: string) => void, removeTerminal: (id: string) => void, ): Promise<void> { return destroyUserShell(environmentId, terminalId, taskID ?? undefined) .then(() => { removeUserShellStore(environmentId, terminalId); removeTerminal(terminalId); }) .catch((error) => console.error("Failed to destroy terminal:", error)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/hooks/domains/session/use-terminals.ts` around lines 286 - 306, Both useCloseTab and useManagedTerminalActions.destroyTerminal duplicate the same destroy/remove sequence; extract a shared async helper like destroyAndRemove(environmentId, terminalId, taskID, removeUserShellStore, removeTerminal) that calls destroyUserShell(environmentId, terminalId, taskID ?? undefined) and then invokes removeUserShellStore(environmentId, terminalId) and removeTerminal(terminalId) and logs errors, then replace the inline promise chain in useCloseTab and the async/await path in useManagedTerminalActions.destroyTerminal to call this helper to keep logic centralized and avoid drift.apps/web/components/task/close-terminal-confirm-dialog.tsx (1)
42-50: 💤 Low valueDialog dismisses before
onConfirmresolves.
onOpenChange(false)runs synchronously and the dialog closes immediately, whileonConfirm()(an async destroy in every call site) is fire-and-forget. The PR notes a final adjustment to "keep the confirm dialog open until destroy completes," but this component dismisses first. If that behavior is intended, consider awaiting/loading state here; otherwise the note and code diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/task/close-terminal-confirm-dialog.tsx` around lines 42 - 50, The confirm action currently calls onOpenChange(false) before onConfirm(), causing the dialog to close immediately; change the click handler on AlertDialogAction to await the async onConfirm() and only call onOpenChange(false) after it resolves (or handle errors), and add a temporary disabled/loading state on the AlertDialogAction while onConfirm is in progress so users can't re-submit; update the handler referenced (the AlertDialogAction onClick that calls onOpenChange and onConfirm) to perform async await, set a local loading flag, disable the button during the await, and then close the dialog when finished.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/components/task/dockview-layout-setup.ts`:
- Around line 350-369: Update the JSDoc above handleTerminalPanelClosed to
accurately reflect actual API behavior: note that stopUserShell is an alias of
destroyUserShell (both delegate to user_shell.destroy and may fall back to
user_shell.stop), so change the comment from “DB row removed” to something like
“destroys terminal; backend may fall back to stop (DB row removal not
guaranteed)”, and clarify that the handler skips already-terminated panels via
terminalTerminateClosePanelIds; keep references to
stopUserShell/destroyUserShell/user_shell.destroy/user_shell.stop so readers can
see the exact behavior.
---
Nitpick comments:
In `@apps/web/components/task/close-terminal-confirm-dialog.tsx`:
- Around line 42-50: The confirm action currently calls onOpenChange(false)
before onConfirm(), causing the dialog to close immediately; change the click
handler on AlertDialogAction to await the async onConfirm() and only call
onOpenChange(false) after it resolves (or handle errors), and add a temporary
disabled/loading state on the AlertDialogAction while onConfirm is in progress
so users can't re-submit; update the handler referenced (the AlertDialogAction
onClick that calls onOpenChange and onConfirm) to perform async await, set a
local loading flag, disable the button during the await, and then close the
dialog when finished.
In `@apps/web/hooks/domains/session/use-terminals.ts`:
- Around line 286-306: Both useCloseTab and
useManagedTerminalActions.destroyTerminal duplicate the same destroy/remove
sequence; extract a shared async helper like destroyAndRemove(environmentId,
terminalId, taskID, removeUserShellStore, removeTerminal) that calls
destroyUserShell(environmentId, terminalId, taskID ?? undefined) and then
invokes removeUserShellStore(environmentId, terminalId) and
removeTerminal(terminalId) and logs errors, then replace the inline promise
chain in useCloseTab and the async/await path in
useManagedTerminalActions.destroyTerminal to call this helper to keep logic
centralized and avoid drift.
In `@apps/web/lib/terminal/terminal-busy-registry.ts`:
- Around line 42-47: markTerminalOutput runs bufferLooksAtPrompt on every
onWriteParsed event causing unnecessary work; add an early-return when the
terminal is already marked idle to avoid re-checking prompts. Specifically, in
markTerminalOutput check busyByTerminalId for terminalId (e.g.,
busyByTerminalId.get(terminalId) === false or !busyByTerminalId.has/true) and
return immediately if not busy, otherwise proceed to call bufferLooksAtPrompt
and set the map to false on transition; this ensures only busy→idle transitions
invoke the expensive buffer/regex work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5026b40e-2585-435c-b129-53b27b7978c1
📒 Files selected for processing (13)
apps/web/components/task/close-terminal-confirm-dialog.tsxapps/web/components/task/dockview-layout-setup.tsapps/web/components/task/mobile/mobile-terminals-section.tsxapps/web/components/task/passthrough-terminal.tsxapps/web/components/task/task-right-panel-tab-contents.tsxapps/web/components/task/task-right-panel.tsxapps/web/components/task/terminal-tab.tsxapps/web/components/task/use-terminal-busy-tracking.test.tsapps/web/components/task/use-terminal-busy-tracking.tsapps/web/e2e/tests/terminal/terminal-dockview-ui.spec.tsapps/web/hooks/domains/session/use-terminals.tsapps/web/lib/terminal/terminal-busy-registry.test.tsapps/web/lib/terminal/terminal-busy-registry.ts
Extend Terminal with initialCommand from UserShellInfo so right-panel and mobile close paths confirm before destroying legacy script shells. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 2m 57s —— View job Code Review
FindingsAll prior blockers from rounds 1–4 are fully resolved. The Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. No blockers remain. The single suggestion is a minor perf optimization on the output hot path; it doesn't affect correctness. |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Drop initialCommand from confirm gating so ordinary shells with cd metadata close idle without a dialog; short-circuit busy output checks. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 23s —— View job Code Review
FindingsAll issues from the prior five review rounds have been addressed in Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. |
FindingsAll issues from the prior five review rounds have been addressed. One lingering suggestion from this pass: Suggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. The |
Prevent AlertDialogAction from closing early; exclude bash heredoc PS2 prompt from idle detection so busy shells still warn on close. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 22s —— View job Code Review
FindingsAll issues from the prior six review rounds have been addressed. Two observations from this pass: Suggestion (recommended, doesn't block)
Note (no change needed)
Summary
Verdict: Ready to merge. The |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Allow optional leading context before prompt sigils so virtualenv and fish-style PS1 strings clear busy state without reopening heredoc gaps. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 50s —— View job Code Review
FindingsAll issues from the prior seven review rounds are resolved. Note (no change needed)
Summary
Verdict: Ready to merge. The |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Only treat $/#/% as prompts at line start or after env markers, not when arbitrary output ends with a sigil after whitespace. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 2m 19s —— View job Code Review
FindingsAll issues from the prior eight review rounds are resolved.
Verified all documented cases:
The regression test for Summary
Verdict: Ready to merge. All eight prior review rounds have been fully addressed. Architecture, logic, tests, complexity limits, and regex correctness are all sound. |
1 participant
Summary
CloseTerminalConfirmDialogand a xterm-based busy registry so all close paths behave consistentlyTest plan
sleep 60), click tab X — confirmation dialog appears; Cancel keeps tab openMade with Cursor