Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a helper that finds terminal panes with open connections and an exported confirmation function that prompts the user. The confirmation is invoked from the tab-close guard and the terminal pane onBeforeClose handler to prevent closing active terminals without explicit consent. ChangesTerminal Pane Close Confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 3 individual chapters for you:
Chapters generated by Stage for commit 8e94c8c on Jun 5, 2026 12:35am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/confirmTerminalClose.ts (1)
6-13: 💤 Low valueType-cast safety: Validate
TerminalPaneDatashape.Line 9 casts
pane.datatoTerminalPaneDataafter checkingkind === "terminal", but the kind check doesn't guarantee the data shape matches the type. If the pane kind is set incorrectly or the data is malformed, accessingterminalIdcould fail at runtime.Consider adding a runtime guard or using a type predicate to safely validate the cast:
🛡️ Suggested type-safe approach
function getActiveTerminalPanes(panes: Iterable<Pane<PaneViewerData>>) { return Array.from(panes).filter((pane) => { if (pane.kind !== "terminal") return false; - const { terminalId } = pane.data as TerminalPaneData; - if (!terminalId) return false; + const data = pane.data as TerminalPaneData; + const terminalId = data?.terminalId; + if (!terminalId) return false; return terminalRuntimeRegistry.has(terminalId); }); }Alternatively, define a type guard function to validate the structure more explicitly.
🤖 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/confirmTerminalClose.ts around lines 6 - 13, getActiveTerminalPanes currently casts pane.data to TerminalPaneData after only checking pane.kind === "terminal", which can still allow malformed data to cause runtime errors; add a runtime guard/type predicate (e.g., isTerminalPaneData) and use it in getActiveTerminalPanes to verify pane.data has the expected shape (presence and type of terminalId) before accessing terminalId and consulting terminalRuntimeRegistry; update the function to accept Pane<PaneViewerData> and return only when the guard passes and terminalRuntimeRegistry.has(terminalId).
🤖 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/confirmTerminalClose.ts:
- Around line 6-13: getActiveTerminalPanes currently casts pane.data to
TerminalPaneData after only checking pane.kind === "terminal", which can still
allow malformed data to cause runtime errors; add a runtime guard/type predicate
(e.g., isTerminalPaneData) and use it in getActiveTerminalPanes to verify
pane.data has the expected shape (presence and type of terminalId) before
accessing terminalId and consulting terminalRuntimeRegistry; update the function
to accept Pane<PaneViewerData> and return only when the guard passes and
terminalRuntimeRegistry.has(terminalId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca77d9ae-c016-46de-a645-8f1ff97a8ba6
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/confirmTerminalClose.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDirtyTabCloseGuard/useDirtyTabCloseGuard.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Architecture diagram
sequenceDiagram
participant User as User
participant TabGuard as Tab Close Guard (useDirtyTabCloseGuard)
participant PaneGuard as Pane Close Guard (onBeforeClose)
participant TerminalConfirm as confirmCloseActiveTerminalPanes
participant Alert as Alert Dialog
Note over User,Alert: User attempts to close a tab or a terminal pane
alt Tab close triggered
User->>TabGuard: close tab (passes all panes in tab)
TabGuard->>TerminalConfirm: NEW: confirmCloseActiveTerminalPanes(panes)
else Pane close triggered (terminal pane)
User->>PaneGuard: close terminal pane (single pane)
PaneGuard->>TerminalConfirm: NEW: confirmCloseActiveTerminalPanes([pane])
end
Note over TerminalConfirm: Filters panes where kind === 'terminal'<br/>and terminalRuntimeRegistry.has(terminalId)
alt No active terminal panes
TerminalConfirm-->>TabGuard: true
TerminalConfirm-->>PaneGuard: true
Note over TabGuard: Proceeds to existing dirty file check<br/>(unchanged)
Note over PaneGuard: Pane close allowed
else One or more active terminal panes
TerminalConfirm->>Alert: Show confirmation dialog<br/>("Close active terminal session(s)?")
alt User clicks "Close"
User->>Alert: Click "Close" (destructive action)
Alert-->>TerminalConfirm: resolve(true)
TerminalConfirm-->>TabGuard: true
TerminalConfirm-->>PaneGuard: true
Note over TabGuard: Proceeds to existing dirty file check
Note over PaneGuard: Pane close allowed
else User clicks "Cancel"
User->>Alert: Click "Cancel"
Alert-->>TerminalConfirm: resolve(false)
TerminalConfirm-->>TabGuard: false
TerminalConfirm-->>PaneGuard: false
Note over TabGuard,PaneGuard: Close aborted – no further checks
end
end
Greptile SummaryThis PR adds a close guard that detects active terminal panes and prompts the user before closing a tab or individual terminal pane, preventing accidental loss of in-progress agent sessions. The implementation introduces a shared helper (
Confidence Score: 3/5The close guard logic is wired correctly into both paths, but the confirmation dialog can reach a state where the underlying Promise never settles, permanently blocking the close operation for any user who dismisses the dialog with Escape. The new helper follows the same Promise-based alert pattern used elsewhere in the codebase, but the Alert component's dismiss path (Escape key / click-outside) calls handleClose() without resolving the promise. Any caller that awaits confirmCloseActiveTerminalPanes will hang indefinitely after such a dismissal, leaving the tab or pane stuck in a non-closeable state until the page is reloaded. The terminalRuntimeRegistry.has() signal also fires for terminals whose process has already exited, which could surface unnecessary prompts. confirmTerminalClose.ts deserves the most attention — the Promise lifecycle and the active-terminal detection heuristic both have issues that affect the two callers.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/confirmTerminalClose.ts | New helper that detects active terminal panes and shows a confirmation dialog; the alert Promise never resolves on Escape-dismiss, which can permanently stall the close sequence. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDirtyTabCloseGuard/useDirtyTabCloseGuard.ts | Tab close guard converted to async to await the terminal confirmation before the existing dirty-file check; integration is correct, though it inherits the unresolved-Promise issue from the new helper. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx | Adds onBeforeClose to the terminal pane registry entry, delegating to confirmCloseActiveTerminalPanes; one-liner hook-in is clean and consistent with the file-pane pattern. |
Sequence Diagram
sequenceDiagram
participant U as User
participant TG as useDirtyTabCloseGuard
participant CT as confirmCloseActiveTerminalPanes
participant A as Alert (dialog)
participant PR as usePaneRegistry (onBeforeClose)
Note over U,PR: Tab close path
U->>TG: close tab
TG->>CT: confirmCloseActiveTerminalPanes(tab.panes)
CT->>CT: getActiveTerminalPanes() via registry.has()
alt No active terminals
CT-->>TG: return true (sync)
else Has active terminals
CT->>A: "alert({ title, description, actions })"
A-->>U: Show confirmation dialog
alt User clicks Close
U->>A: onClick resolve(true)
A-->>CT: Promise resolves true
CT-->>TG: true
TG->>TG: Check dirty file panes
else User clicks Cancel
U->>A: onClick resolve(false)
A-->>CT: Promise resolves false
CT-->>TG: false abort close
else User presses Escape or clicks outside
U->>A: dismiss (no onClick)
Note over A,CT: Promise NEVER resolves — close stalls
end
end
Note over U,PR: Direct pane close path
U->>PR: close terminal pane
PR->>CT: confirmCloseActiveTerminalPanes([pane])
CT->>A: alert (if active)
A-->>U: Show confirmation dialog
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
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/confirmTerminalClose.ts:34-51
**Promise hangs forever when dialog is dismissed without a button click**
The `Promise<boolean>` created here only resolves when one of the two `onClick` handlers fires. Inspecting `Alert.tsx`, the `handleClose()` path — triggered by pressing Escape or clicking outside the modal — simply sets `isOpen = false` without ever calling any action's `onClick`. The promise is never resolved or rejected in that path.
When this function is called from `useDirtyTabCloseGuard`, the `await` at line 19 will stall indefinitely after an Escape-dismiss, permanently blocking the tab-close sequence. Consider treating a dialog-dismissed-without-action as a cancel (i.e., resolve with `false`) by adding an `onOpenChange` callback or a dedicated dismiss action that calls `resolve(false)`.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/confirmTerminalClose.ts:11
**`terminalRuntimeRegistry.has()` does not verify the terminal process is still running**
`has(terminalId)` returns `true` as long as an entry exists in `entryKeysByTerminalId` — which is true from the moment the pane is mounted until it is explicitly closed. A terminal whose underlying process has already exited (e.g., an agent run that completed) retains its registry entry until `onAfterClose` fires. This means the confirmation prompt will appear even for finished terminals, potentially confusing users who are closing a pane that was already idle. Consider also checking the connection state (`terminalRuntimeRegistry.getConnectionState(terminalId) === "open"`) to limit prompts to terminals with an active WebSocket connection.
Reviews (1): Last reviewed commit: "fix(desktop): confirm before closing act..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return new Promise<boolean>((resolve) => { | ||
| alert({ | ||
| title, | ||
| description, | ||
| actions: [ | ||
| { | ||
| label: "Close", | ||
| variant: "destructive", | ||
| onClick: () => resolve(true), | ||
| }, | ||
| { | ||
| label: "Cancel", | ||
| variant: "secondary", | ||
| onClick: () => resolve(false), | ||
| }, | ||
| ], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The Promise<boolean> created here only resolves when one of the two onClick handlers fires. Inspecting Alert.tsx, the handleClose() path — triggered by pressing Escape or clicking outside the modal — simply sets isOpen = false without ever calling any action's onClick. The promise is never resolved or rejected in that path.
When this function is called from useDirtyTabCloseGuard, the await at line 19 will stall indefinitely after an Escape-dismiss, permanently blocking the tab-close sequence. Consider treating a dialog-dismissed-without-action as a cancel (i.e., resolve with false) by adding an onOpenChange callback or a dedicated dismiss action that calls resolve(false).
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/confirmTerminalClose.ts
Line: 34-51
Comment:
**Promise hangs forever when dialog is dismissed without a button click**
The `Promise<boolean>` created here only resolves when one of the two `onClick` handlers fires. Inspecting `Alert.tsx`, the `handleClose()` path — triggered by pressing Escape or clicking outside the modal — simply sets `isOpen = false` without ever calling any action's `onClick`. The promise is never resolved or rejected in that path.
When this function is called from `useDirtyTabCloseGuard`, the `await` at line 19 will stall indefinitely after an Escape-dismiss, permanently blocking the tab-close sequence. Consider treating a dialog-dismissed-without-action as a cancel (i.e., resolve with `false`) by adding an `onOpenChange` callback or a dedicated dismiss action that calls `resolve(false)`.
How can I resolve this? If you propose a fix, please make it concise.| if (pane.kind !== "terminal") return false; | ||
| const { terminalId } = pane.data as TerminalPaneData; | ||
| if (!terminalId) return false; | ||
| return terminalRuntimeRegistry.has(terminalId); |
There was a problem hiding this comment.
terminalRuntimeRegistry.has() does not verify the terminal process is still running
has(terminalId) returns true as long as an entry exists in entryKeysByTerminalId — which is true from the moment the pane is mounted until it is explicitly closed. A terminal whose underlying process has already exited (e.g., an agent run that completed) retains its registry entry until onAfterClose fires. This means the confirmation prompt will appear even for finished terminals, potentially confusing users who are closing a pane that was already idle. Consider also checking the connection state (terminalRuntimeRegistry.getConnectionState(terminalId) === "open") to limit prompts to terminals with an active WebSocket connection.
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/confirmTerminalClose.ts
Line: 11
Comment:
**`terminalRuntimeRegistry.has()` does not verify the terminal process is still running**
`has(terminalId)` returns `true` as long as an entry exists in `entryKeysByTerminalId` — which is true from the moment the pane is mounted until it is explicitly closed. A terminal whose underlying process has already exited (e.g., an agent run that completed) retains its registry entry until `onAfterClose` fires. This means the confirmation prompt will appear even for finished terminals, potentially confusing users who are closing a pane that was already idle. Consider also checking the connection state (`terminalRuntimeRegistry.getConnectionState(terminalId) === "open"`) to limit prompts to terminals with an active WebSocket connection.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1087491 to
8e94c8c
Compare
|
Updated for review feedback:
|
Summary
Related issues
Test plan
bunx @biomejs/biome@2.4.2 checkon changed filesNotes
Summary by CodeRabbit