Repository navigation
feat(desktop): confirm before closing a terminal pane with a running process - #5517
Merged
Merged
Conversation
…process Cmd+W (and close button / middle-click / context menu) on a terminal pane now asks for confirmation when a foreground command is actually running, with a persisted "Don't ask again" checkbox. Closing at an idle shell prompt still closes instantly. Detection compares the shell's controlling-terminal foreground process group to its own pgid (ps tpgid vs pgid), so it does not false-positive on suspended/background jobs. Exposed via a new host-service terminal.hasRunningProcess tRPC query and wired into the terminal pane's onBeforeClose guard.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds terminal close confirmation for running foreground processes, host-service process checks and cleanup, alert checkbox/dismiss support, persisted suppression, and guarded pane/tab closure. ChangesTerminal Close Confirmation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CloseGuard
participant HostService
participant Alert
participant Store
User->>CloseGuard: close terminal or tab
CloseGuard->>HostService: query foreground process state
HostService-->>CloseGuard: running / not running
CloseGuard->>Alert: show confirmation when running
Alert-->>CloseGuard: confirm, cancel, or dismiss
CloseGuard->>Store: persist suppression when selected
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit 58fb253 on Jul 15, 2026 6:56pm UTC. |
Contributor
Greptile SummaryThis PR adds a confirmation dialog when closing a terminal pane that has a foreground process running, covering all close paths (Cmd+W, close button, middle-click, context menu) via the pane's
Confidence Score: 4/5Safe to merge after addressing the missing workspace ownership check on the new tRPC endpoint. The core detection logic (
|
| Filename | Overview |
|---|---|
| packages/pty-daemon/src/process-tree.ts | Adds hasRunningForegroundProcess using ps -o tpgid= -o pgid= -p <pid>; correctly fails closed on parse errors, handles the -1 sentinel via isPositiveInteger, and follows the same spawnSync pattern used elsewhere in this file. |
| packages/host-service/src/trpc/router/terminal/terminal.ts | Adds hasRunningProcess query that accepts workspaceId but never validates it against the session, breaking the workspace-isolation pattern enforced by every other endpoint in this router. |
| packages/host-service/src/terminal/terminal.ts | Adds sessionHasRunningProcess which looks up the live session and delegates to hasRunningForegroundProcess; guard on session.exited is correct. |
| packages/ui/src/atoms/Alert/Alert.tsx | Extends alert() with an optional checkbox and onDismiss callback; the actionTaken flag correctly distinguishes user-initiated dismissals from programmatic closes, and existing callers that omit the ctx param in onClick remain type-compatible. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | Adds onBeforeClose guard that checks suppression, fetches running state, and shows a confirmation dialog; the Promise hangs if Alerter is absent, but all other paths (suppressed, error, idle) correctly return true without blocking. |
| apps/desktop/src/renderer/stores/terminal-close-confirm/store.ts | New persisted Zustand store for suppression state; straightforward with correct localStorage key versioning. |
| packages/pty-daemon/test/foreground-process.test.ts | Real-PTY integration test covering idle→running→idle cycle with appropriate timeouts and cleanup; waitFor polling pattern handles the async foreground group handoff correctly. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant onBeforeClose
participant ConfirmStore
participant tRPC as terminal.hasRunningProcess
participant HostService as sessionHasRunningProcess
participant ps as ps -o tpgid= -o pgid=
participant Alert
User->>onBeforeClose: Cmd+W / close button
onBeforeClose->>ConfirmStore: getState().suppressed?
alt suppressed
onBeforeClose-->>User: close immediately (true)
else not suppressed
onBeforeClose->>tRPC: "fetch({ terminalId, workspaceId })"
tRPC->>HostService: sessionHasRunningProcess(terminalId)
HostService->>ps: "spawnSync ps -o tpgid= -o pgid= -p pid"
ps-->>HostService: tpgid pgid
HostService-->>tRPC: "tpgid !== pgid"
tRPC-->>onBeforeClose: running
alt error or not running
onBeforeClose-->>User: close immediately (true)
else running
onBeforeClose->>Alert: show dialog with checkbox
alt User clicks Close terminal
Alert-->>onBeforeClose: resolve(true)
onBeforeClose-->>User: close pane
else User cancels or Escape
Alert-->>onBeforeClose: resolve(false)
onBeforeClose-->>User: keep pane open
end
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant onBeforeClose
participant ConfirmStore
participant tRPC as terminal.hasRunningProcess
participant HostService as sessionHasRunningProcess
participant ps as ps -o tpgid= -o pgid=
participant Alert
User->>onBeforeClose: Cmd+W / close button
onBeforeClose->>ConfirmStore: getState().suppressed?
alt suppressed
onBeforeClose-->>User: close immediately (true)
else not suppressed
onBeforeClose->>tRPC: "fetch({ terminalId, workspaceId })"
tRPC->>HostService: sessionHasRunningProcess(terminalId)
HostService->>ps: "spawnSync ps -o tpgid= -o pgid= -p pid"
ps-->>HostService: tpgid pgid
HostService-->>tRPC: "tpgid !== pgid"
tRPC-->>onBeforeClose: running
alt error or not running
onBeforeClose-->>User: close immediately (true)
else running
onBeforeClose->>Alert: show dialog with checkbox
alt User clicks Close terminal
Alert-->>onBeforeClose: resolve(true)
onBeforeClose-->>User: close pane
else User cancels or Escape
Alert-->>onBeforeClose: resolve(false)
onBeforeClose-->>User: keep pane open
end
end
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/trpc/router/terminal/terminal.ts:138-147
**Workspace ownership not verified for `hasRunningProcess`**
The `workspaceId` parameter is included in the input schema but is never used — `sessionHasRunningProcess` looks up the session by `terminalId` alone with no check that it belongs to the caller's workspace. Every other mutation/query in this router that involves a terminal (e.g., `killSession`, `listSessions`) validates workspace ownership before operating. As a result, any authenticated user who knows (or can guess) a `terminalId` UUID can probe whether a foreground process is running in another workspace's terminal. Compare to `killSession` which gates on `session.originWorkspaceId !== input.workspaceId` before proceeding.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:342-368
**Unresolvable promise if `Alerter` is not mounted**
`alert()` silently logs an error and returns `undefined` when `showAlertFn` is `null` (i.e., when `<Alerter />` is not in the tree). In that case the `Promise<boolean>` returned by `onBeforeClose` is never resolved or rejected, permanently blocking the pane close with no feedback to the user. Adding a fallback `resolve(true)` (or `false`) outside the `alert({…})` call would make the guard fail-open even if the dialog layer is unavailable.
Reviews (1): Last reviewed commit: "feat(desktop): confirm before closing a ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+138
to
+147
| hasRunningProcess: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| terminalId: z.string(), | ||
| workspaceId: z.string(), | ||
| }), | ||
| ) | ||
| .query(({ input }) => ({ | ||
| running: sessionHasRunningProcess(input.terminalId), | ||
| })), |
Contributor
There was a problem hiding this comment.
hasRunningProcess
The workspaceId parameter is included in the input schema but is never used — sessionHasRunningProcess looks up the session by terminalId alone with no check that it belongs to the caller's workspace. Every other mutation/query in this router that involves a terminal (e.g., killSession, listSessions) validates workspace ownership before operating. As a result, any authenticated user who knows (or can guess) a terminalId UUID can probe whether a foreground process is running in another workspace's terminal. Compare to killSession which gates on session.originWorkspaceId !== input.workspaceId before proceeding.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/terminal/terminal.ts
Line: 138-147
Comment:
**Workspace ownership not verified for `hasRunningProcess`**
The `workspaceId` parameter is included in the input schema but is never used — `sessionHasRunningProcess` looks up the session by `terminalId` alone with no check that it belongs to the caller's workspace. Every other mutation/query in this router that involves a terminal (e.g., `killSession`, `listSessions`) validates workspace ownership before operating. As a result, any authenticated user who knows (or can guess) a `terminalId` UUID can probe whether a foreground process is running in another workspace's terminal. Compare to `killSession` which gates on `session.originWorkspaceId !== input.workspaceId` before proceeding.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+342
to
+368
| return new Promise<boolean>((resolve) => { | ||
| alert({ | ||
| title: "A process is still running in this terminal", | ||
| description: | ||
| "Closing this terminal will end the running process.", | ||
| checkbox: { label: "Don't ask again" }, | ||
| onDismiss: () => resolve(false), | ||
| actions: [ | ||
| { | ||
| label: "Close terminal", | ||
| variant: "destructive", | ||
| onClick: ({ checkboxChecked }) => { | ||
| if (checkboxChecked) { | ||
| useTerminalCloseConfirmStore.getState().suppress(); | ||
| } | ||
| resolve(true); | ||
| }, | ||
| }, | ||
| { | ||
| label: "Cancel", | ||
| variant: "ghost", | ||
| onClick: () => resolve(false), | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
Alerter is not mounted
alert() silently logs an error and returns undefined when showAlertFn is null (i.e., when <Alerter /> is not in the tree). In that case the Promise<boolean> returned by onBeforeClose is never resolved or rejected, permanently blocking the pane close with no feedback to the user. Adding a fallback resolve(true) (or false) outside the alert({…}) call would make the guard fail-open even if the dialog layer is unavailable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
Line: 342-368
Comment:
**Unresolvable promise if `Alerter` is not mounted**
`alert()` silently logs an error and returns `undefined` when `showAlertFn` is `null` (i.e., when `<Alerter />` is not in the tree). In that case the `Promise<boolean>` returned by `onBeforeClose` is never resolved or rejected, permanently blocking the pane close with no feedback to the user. Adding a fallback `resolve(true)` (or `false`) outside the `alert({…})` call would make the guard fail-open even if the dialog layer is unavailable.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx (1)
326-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog errors in the catch block for observability.
The
catchsilently returnstrue, which is the correct fail-closed behavior. However, if thehasRunningProcessquery consistently fails (e.g., host-service issue), the confirmation dialog never appears and running processes are killed without warning. The existingonAfterClosehandler logs errors viaconsole.warnforkillTerminalSessionSilently— adding similar logging here would make failures debuggable and keep the pattern consistent.♻️ Suggested logging addition
} catch { - // If we can't tell, don't block the user from closing. + // If we can't tell, don't block the user from closing. + console.warn("Failed to check for running process", { + terminalId, + workspaceId, + }); return true; }🤖 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/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx around lines 326 - 368, The catch block in usePaneRegistry’s terminal onBeforeClose handler is failing closed correctly, but it swallows hasRunningProcess fetch errors without any observability. Update that catch to log the failure with context (using the same style as the onAfterClose / killTerminalSessionSilently warning), so repeated workspaceTrpcUtils.terminal.hasRunningProcess.fetch issues can be diagnosed while still returning true to avoid blocking close.
🤖 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.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:
- Around line 326-368: The catch block in usePaneRegistry’s terminal
onBeforeClose handler is failing closed correctly, but it swallows
hasRunningProcess fetch errors without any observability. Update that catch to
log the failure with context (using the same style as the onAfterClose /
killTerminalSessionSilently warning), so repeated
workspaceTrpcUtils.terminal.hasRunningProcess.fetch issues can be diagnosed
while still returning true to avoid blocking close.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1208884f-8cd0-4e4d-ae89-71f48fc1c63c
📒 Files selected for processing (9)
apps/desktop/plans/20260708-terminal-close-confirm.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/stores/terminal-close-confirm/store.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.tspackages/pty-daemon/package.jsonpackages/pty-daemon/src/process-tree.tspackages/pty-daemon/test/foreground-process.test.tspackages/ui/src/atoms/Alert/Alert.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Architecture diagram
sequenceDiagram
participant UI as Desktop UI (usePaneRegistry)
participant Store as TerminalCloseConfirm Store (localStorage)
participant Alert as Alert Dialog (ui/Alert)
participant TRPC as tRPC Client (terminal.hasRunningProcess)
participant Host as Host Service (sessionHasRunningProcess)
participant PTY as Pty Daemon (process-tree)
participant OS as OS (ps command)
Note over UI,OS: Terminal pane close flow
UI->>UI: onBeforeClose triggered (Cmd+W / close button / middle-click / context menu)
UI->>Store: Get suppressed state
Store-->>UI: suppressed: boolean
alt suppressed is true
UI-->>UI: Return true (close immediately)
else suppressed is false
UI->>TRPC: fetch({ terminalId, workspaceId })
TRPC->>Host: hasRunningProcess query
Host->>Host: Lookup session by terminalId
alt session not found or exited
Host-->>TRPC: running: false
else session found
Host->>PTY: hasRunningForegroundProcess(shellPid)
PTY->>OS: spawnSync("ps", ["-o", "tpgid=", "-o", "pgid=", "-p", shellPid])
OS-->>PTY: stdout with tpgid and pgid
PTY->>PTY: Compare tpgid !== pgid
alt ps error or invalid numbers
PTY-->>Host: false (fails closed)
else tpgid === pgid (idle prompt)
PTY-->>Host: false
else tpgid !== pgid (running command)
PTY-->>Host: true
end
Host-->>TRPC: { running: boolean }
end
TRPC-->>UI: { running: boolean }
alt error from fetch
UI-->>UI: Return true (don't block close)
else running is false
UI-->>UI: Return true (close immediately)
else running is true
UI->>Alert: show dialog with checkbox "Don't ask again"
alt User clicks "Cancel" or dismisses (Escape/outside-click)
Alert->>Alert: onDismiss fires
Alert-->>UI: resolve(false)
UI-->>UI: Return false (keep pane open)
else User clicks "Close terminal"
Alert->>UI: onClick({ checkboxChecked })
alt checkboxChecked is true
UI->>Store: suppress()
Store->>Store: persist to localStorage
end
Alert-->>UI: resolve(true)
UI-->>UI: Return true (close pane)
end
end
end
Note over UI,OS: Integration test flow (foreground-process.test.ts)
participant Test as Integration Test
participant Pty as Pty (node-pty)
Test->>Pty: spawn shell (bash --norc --noprofile -i)
Pty-->>Test: pty instance
Test->>PTY: hasRunningForegroundProcess(pty.pid)
PTY->>OS: ps query
OS-->>PTY: tpgid === pgid (idle)
PTY-->>Test: false
Test->>Pty: write "sleep 3\n"
Test->>PTY: hasRunningForegroundProcess(pty.pid)
PTY->>OS: ps query
OS-->>PTY: tpgid !== pgid (running)
PTY-->>Test: true
Test->>Test: Wait for sleep to finish
Test->>PTY: hasRunningForegroundProcess(pty.pid)
PTY->>OS: ps query
OS-->>PTY: tpgid === pgid (idle again)
PTY-->>Test: false
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
- Only prompt when the close will actually kill the session; skip the confirm for terminals being backgrounded (kept alive), which end nothing. - Security: gate terminal.hasRunningProcess on workspace ownership so one workspace can't probe another's terminals (session.workspaceId check). - Fail open if <Alerter /> isn't mounted so onBeforeClose can't hang on an unresolved promise; alert() now returns whether it showed. - Log the hasRunningProcess fetch failure in the fail-closed catch.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui/src/atoms/Alert/Alert.tsx">
<violation number="1" location="packages/ui/src/atoms/Alert/Alert.tsx:133">
P2: The `alert()` return value claims to indicate whether the dialog was shown, but `showAlertFn` is never cleared when `<Alerter />` unmounts. If the Alerter previously mounted and later unmounts, `alert()` returns `true` and calls a stale setter. In promise-based callers like the terminal close guard, this means neither `onDismiss` nor any action callback ever fires, so the returned promise hangs and the close operation deadlocks. Add a `useEffect` cleanup inside `Alerter` to reset `showAlertFn = null` on unmount so the return value accurately reflects mount state.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1,6 +1,7 @@ | |||
| "use client"; | |||
Contributor
There was a problem hiding this comment.
P2: The alert() return value claims to indicate whether the dialog was shown, but showAlertFn is never cleared when <Alerter /> unmounts. If the Alerter previously mounted and later unmounts, alert() returns true and calls a stale setter. In promise-based callers like the terminal close guard, this means neither onDismiss nor any action callback ever fires, so the returned promise hangs and the close operation deadlocks. Add a useEffect cleanup inside Alerter to reset showAlertFn = null on unmount so the return value accurately reflects mount state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/src/atoms/Alert/Alert.tsx, line 133:
<comment>The `alert()` return value claims to indicate whether the dialog was shown, but `showAlertFn` is never cleared when `<Alerter />` unmounts. If the Alerter previously mounted and later unmounts, `alert()` returns `true` and calls a stale setter. In promise-based callers like the terminal close guard, this means neither `onDismiss` nor any action callback ever fires, so the returned promise hangs and the close operation deadlocks. Add a `useEffect` cleanup inside `Alerter` to reset `showAlertFn = null` on unmount so the return value accurately reflects mount state.</comment>
<file context>
@@ -129,14 +129,16 @@ const Alerter = () => {
-const alert = (options: AlertOptions) => {
+/** Returns true if the dialog was shown, false if no <Alerter /> is mounted. */
+const alert = (options: AlertOptions): boolean => {
if (!showAlertFn) {
console.error(
</file context>
…on path The electron workspace-delete path only killed the main-process daemon's terminals, so a v2 workspace's host-service sessions — backgrounded, renderer-detached ones included — leaked in the pty-daemon (the reaper never catches them: their rows stay active and host-service is never told of the delete). The v2 dashboard delete already disposed them via workspaceCleanup.destroy; this closes the gap for the electron path used by remote-agent-driven deletes. - host-service: new terminal.disposeWorkspaceSessions procedure reusing disposeSessionsByWorkspaceId + clearing disposed rows. - renderer: useDeleteWorkspace best-effort-calls it on delete success via the local host-service client (non-throwing provider access; never blocks delete).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Extends the electron-path terminal cleanup to the remaining v1 paths so backgrounded host-service sessions don't leak: - close workspace (useCloseWorkspace) disposes by workspace id - delete closed worktree (DeleteWorktreeDialog) disposes by worktree path — a closed worktree has no workspace id, so host-service resolves sessions through the shared workspaces.worktreePath (new disposeWorktreeSessions procedure + disposeSessionsByWorktreePath). Shared the best-effort dispose helper across delete/close/deleteWorktree (renderer/lib/dispose-host-sessions). All calls are best-effort and never block the mutation; provider access is non-throwing.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 864-888: The worktree cleanup path only terminates daemon sessions
and leaves terminalSessions records behind, so closed worktree deletions can
orphan stale rows. Update disposeSessionsByWorktreePath to mirror the
session-row cleanup performed by disposeWorkspaceSessions by clearing
terminalSessions for each affected workspace/worktree before or alongside
calling disposeSessionsByWorkspaceId, and keep the terminated/failed counts
accurate while preserving the existing worktreePath-to-workspace lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4512cb7d-5955-42bd-b830-299fc62f0ae4
📒 Files selected for processing (7)
apps/desktop/src/renderer/lib/dispose-host-sessions.tsapps/desktop/src/renderer/react-query/workspaces/useCloseWorkspace.tsapps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.tsapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/DeleteWorktreeDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspacesListView/WorkspaceRow/WorkspaceRow.tsxpackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/lib/dispose-host-sessions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/react-query/workspaces/useDeleteWorkspace.ts
- packages/host-service/src/trpc/router/terminal/terminal.ts
…tree row cleanup Addresses code-review findings: - Tab close (tab X, Close Others/All, CLOSE_TAB hotkey) now runs the running-process confirm — previously onBeforeClose only fired on single-pane close, so the most common close gesture silently killed running terminals. Extracted a shared confirmCloseTerminals helper used by both the per-pane guard and a new composed tab-close guard (running terminals + dirty files), and routed the CLOSE_TAB hotkey through the guard. - hasRunningProcess is now probed with staleTime:0 so a repeated close doesn't read the 5s-stale query cache and show a spurious/missed prompt. - disposeWorktreeSessions now clears disposed session rows like the workspace variant, so worktree deletes don't leak rows in the host-service DB.
…pace delete The dispose helpers used the active org's host URL, so deleting/closing a workspace in a non-active org sent the request to the wrong host-service and the sessions leaked. Broadcast disposeWorkspaceSessions/disposeWorktreeSessions to every running local host-service instead (each no-ops for workspaces it doesn't own), so cleanup is correct regardless of which org is active. - coordinator.getConnections() returns every running local connection - new hostServiceCoordinator.getConnections tRPC query - dispose helpers fetch all connections, set each host's secret, and fire the dispose at every host (best-effort, never blocks the mutation) - drop the now-unused useLocalHostServiceOptional accessor
Contributor
There was a problem hiding this comment.
2 issues found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/confirm-close-terminals.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/confirm-close-terminals.ts:80">
P2: If saving the "Don't ask again" preference to localStorage throws (e.g. quota exceeded), the `resolve(true)` that permits the close never executes because it comes after `suppress()`. The Alert action boundary will catch the error and keep the dialog open, so the pane close promise never resolves and the operation appears to hang. Consider resolving the close before persisting the suppression flag, or wrapping `suppress()` in a local try/catch so the close always completes even if persistence fails.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:129">
P2: The new async `CLOSE_TAB` hotkey handler awaits `onBeforeCloseTab(tab)` inside an async callback, but `useHotkey` invokes handlers synchronously without awaiting or catching returned promises. If the terminal-status check, alert dialog, or dirty-tab guard rejects, the promise is unhandled. Since `CLOSE_TAB` was previously synchronous, this PR newly introduces an unhandled-rejection path. Add a `catch` block that logs the failure and resets the guard ref so the user sees a diagnostic instead of a silent crash.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| label: labels.confirmLabel, | ||
| variant: "destructive", | ||
| onClick: ({ checkboxChecked }) => { | ||
| if (checkboxChecked) { |
Contributor
There was a problem hiding this comment.
P2: If saving the "Don't ask again" preference to localStorage throws (e.g. quota exceeded), the resolve(true) that permits the close never executes because it comes after suppress(). The Alert action boundary will catch the error and keep the dialog open, so the pane close promise never resolves and the operation appears to hang. Consider resolving the close before persisting the suppression flag, or wrapping suppress() in a local try/catch so the close always completes even if persistence fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/lib/terminal/confirm-close-terminals.ts, line 80:
<comment>If saving the "Don't ask again" preference to localStorage throws (e.g. quota exceeded), the `resolve(true)` that permits the close never executes because it comes after `suppress()`. The Alert action boundary will catch the error and keep the dialog open, so the pane close promise never resolves and the operation appears to hang. Consider resolving the close before persisting the suppression flag, or wrapping `suppress()` in a local try/catch so the close always completes even if persistence fails.</comment>
<file context>
@@ -0,0 +1,92 @@
+ label: labels.confirmLabel,
+ variant: "destructive",
+ onClick: ({ checkboxChecked }) => {
+ if (checkboxChecked) {
+ useTerminalCloseConfirmStore.getState().suppress();
+ }
</file context>
| if (state.activeTabId) { | ||
| state.removeTab(state.activeTabId); | ||
| const isClosingTabRef = useRef(false); | ||
| useHotkey("CLOSE_TAB", async () => { |
Contributor
There was a problem hiding this comment.
P2: The new async CLOSE_TAB hotkey handler awaits onBeforeCloseTab(tab) inside an async callback, but useHotkey invokes handlers synchronously without awaiting or catching returned promises. If the terminal-status check, alert dialog, or dirty-tab guard rejects, the promise is unhandled. Since CLOSE_TAB was previously synchronous, this PR newly introduces an unhandled-rejection path. Add a catch block that logs the failure and resets the guard ref so the user sees a diagnostic instead of a silent crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts, line 129:
<comment>The new async `CLOSE_TAB` hotkey handler awaits `onBeforeCloseTab(tab)` inside an async callback, but `useHotkey` invokes handlers synchronously without awaiting or catching returned promises. If the terminal-status check, alert dialog, or dirty-tab guard rejects, the promise is unhandled. Since `CLOSE_TAB` was previously synchronous, this PR newly introduces an unhandled-rejection path. Add a `catch` block that logs the failure and resets the guard ref so the user sees a diagnostic instead of a silent crash.</comment>
<file context>
@@ -122,10 +125,21 @@ export function useWorkspaceHotkeys({
- if (state.activeTabId) {
- state.removeTab(state.activeTabId);
+ const isClosingTabRef = useRef(false);
+ useHotkey("CLOSE_TAB", async () => {
+ if (isClosingTabRef.current) return;
+ isClosingTabRef.current = true;
</file context>
…alog # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx # packages/pty-daemon/package.json
3 of 4 tasks
Laurence-NZ
added a commit
to Laurence-NZ/superset-pwsh
that referenced
this pull request
Jul 23, 2026
Upstream ParadevAI#5517 added hasRunningForegroundProcess, which spawns `ps` to check the tty foreground process group. On Windows there is no `ps` and no POSIX tty foreground-group concept, so add an explicit win32 early return (fail closed) — matching readProcessTable's existing win32 guard, whose intent is that Windows never attempts a `ps` spawn here.
5 tasks
This branch was previously deployed
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.
What & why
Closing a terminal pane by mistake (Cmd+W) silently kills whatever is running in it. This adds a confirmation dialog that appears only when a foreground command is actually running, with a persisted "Don't ask again" checkbox. Closing at an idle shell prompt still closes instantly — no nag.
Covers every close route through the pane's
onBeforeCloseguard: Cmd+W (CLOSE_PANE), the close button, middle-click, context menu, and browser-pane Cmd+W.How "a process is running" is detected
No such signal existed. It's computed on demand from the shell's controlling-terminal foreground process group via
ps -o tpgid -o pgid:==the shell's own group → not runningThis is precise: unlike a "shell has child processes" check it does not false-positive on suspended/background jobs. Fails closed (returns
false) on anypserror, missing tty, or if we can't reach the query — never blocks a close.Changes
packages/pty-daemon/src/process-tree.ts— newhasRunningForegroundProcess(shellPid)packages/host-service/…/terminal/terminal.ts—sessionHasRunningProcess(terminalId)(session pid → detection)packages/host-service/…/trpc/router/terminal/terminal.ts— newterminal.hasRunningProcessquerypackages/ui/…/Alert/Alert.tsx—alert()gains an optionalcheckbox+onDismiss(Escape/outside-click resolves as cancel instead of hanging)apps/desktop/…/stores/terminal-close-confirm/store.ts— persisted suppression flag (localStorage)apps/desktop/…/usePaneRegistry.tsx— terminalonBeforeCloseguardVerification
Driven end-to-end in the running desktop app (v2 workspace, over CDP):
sleep 300+ Cmd+Wsuppressed:truePlus a real-pty integration test (
test/foreground-process.test.ts) exercising idle→running→idle, mutation-verified to fail when the core comparison breaks. typecheck / lint / pty-daemon suite all pass.Scope
Pane-level close only. Closing a whole tab (
CLOSE_TAB, a different shortcut) that contains a running terminal does not warn yet — left as a follow-up.Summary by cubic
Adds a confirm dialog when closing a terminal pane or tab that has a running foreground command, with a “Don’t ask again” option. Also disposes host-service terminals on workspace close/delete and closed worktree delete, broadcasting cleanup to all local hosts and pruning disposed rows to prevent leaks.
New Features
ps(tpgidvspgid) in@superset/pty-daemon(hasRunningForegroundProcess), returning false on errors.sessionHasRunningProcessand tRPCterminal.hasRunningProcess(workspace-gated; probes live withstaleTime: 0).CLOSE_TABhotkey); skips backgrounded terminals; suppression persisted viauseTerminalCloseConfirmStore.@superset/uialert()adds optional checkbox,onDismiss, returns a boolean; probe or Alerter failures never block close.Bug Fixes
terminal.hasRunningProcessenforces workspace ownership.Written for commit 58fb253. Summary will update on new commits.
Summary by CodeRabbit