Stop And Delete Sessions#535
Merged
Merged
Conversation
…lk action Right-clicking a CLI or shell session (sidebar card OR work-area header) now offers a 'Stop and delete' option that stops the runtime and deletes the session in one confirmed step, alongside the existing two-step Stop runtime → Delete session flow. Chats keep their direct delete. For multi-select, a 'Stop & delete' bulk action appears whenever the selection includes a running runtime. It stops everything that needs stopping and then deletes every selected session — fixing the case where a mixed CLI+chat selection only deleted the chat. Both flows gate on a styled confirmation dialog. Header right-click is newly wired through WorkSurfaceHeader so it opens the same session context menu as the kebab and the sidebar card. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds "stop and delete" confirmation flows for CLI sessions in the Terminals page. It wires context-menu support through header components, adds UI controls across context menus, popovers, and selection toolbars, implements single and bulk deletion handlers with confirmation dialogs, and provides comprehensive test coverage for both success and cancellation paths. ChangesStop & Delete Confirmation Flow for CLI Sessions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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 |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 7, 2026 00:54
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx`:
- Around line 166-174: The label in SessionContextMenu.tsx is inconsistent with
SessionInfoPopover.tsx: change the button text in the conditional rendering
block (the button that calls onStopAndDelete(session) and onClose()) from "Stop
and delete" to exactly "Stop & delete" so the user-facing action wording matches
SessionInfoPopover; locate the JSX branch that checks isRunning && session.ptyId
&& !isChat and update the displayed string (also ensure the deletingSessionId
conditional remains intact).
In `@apps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsx`:
- Around line 295-307: Session label mismatch: in SessionInfoPopover (render
branch checking session.status === "running" && session.ptyId && !isChat) update
the action copy so both SmartTooltip label and the Button text use the
consistent phrase "Stop and delete" (change the Button text currently "Stop &
delete" and confirm SmartTooltip label is also "Stop and delete") to match the
label used in SessionContextMenu and keep
onStopAndDelete/deletingSessionId/session.id logic unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86e1ef48-1514-471c-90e6-4524adcc5dcf
📒 Files selected for processing (8)
apps/desktop/src/renderer/components/terminals/CliSessionWorkSurfaceHeader.tsxapps/desktop/src/renderer/components/terminals/SessionContextMenu.tsxapps/desktop/src/renderer/components/terminals/SessionInfoPopover.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/work/WorkSurfaceHeader.tsx
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
539
to
667
| expect(confirmSpy).toHaveBeenCalledWith(expect.stringContaining("Delete 2 selected sessions?")); | ||
| confirmSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it("stops and deletes a single running CLI session via the context menu", async () => { | ||
| const runningCli = workMocks.makeTerminalSession("cli-single", "lane-primary", "codex"); | ||
| const sessionDelete = vi.fn().mockResolvedValue(undefined); | ||
| const agentChatDelete = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| Object.defineProperty(window, "ade", { | ||
| configurable: true, | ||
| value: { | ||
| agentChat: { delete: agentChatDelete }, | ||
| builtInBrowser: { onEvent: vi.fn(() => vi.fn()) }, | ||
| sessions: { delete: sessionDelete }, | ||
| }, | ||
| }); | ||
| workMocks.currentWork = { | ||
| ...workMocks.baseWork, | ||
| sessions: [runningCli], | ||
| visibleSessions: [runningCli], | ||
| runningFiltered: [runningCli], | ||
| runningSessions: [runningCli], | ||
| filtered: [runningCli], | ||
| sessionsGroupedByLane: new Map([["lane-primary", [runningCli]]]), | ||
| closingPtyIds: new Set<string>(), | ||
| }; | ||
|
|
||
| render(<TerminalsPage />); | ||
|
|
||
| // Open the context menu for the session, then trigger stop-and-delete. | ||
| fireEvent.click(await screen.findByRole("button", { name: "context menu cli-single" })); | ||
| fireEvent.click(await screen.findByRole("button", { name: "context stop and delete cli-single" })); | ||
|
|
||
| // The styled confirmation dialog must gate the destructive single-session action. | ||
| fireEvent.click(await screen.findByRole("button", { name: "Stop & delete" })); | ||
|
|
||
| await waitFor(() => { | ||
| // The session-delete service stops the runtime and removes the record in one call; | ||
| // the chat-delete path must not be touched for a non-chat session. | ||
| expect(sessionDelete).toHaveBeenCalledWith({ sessionId: "cli-single" }); | ||
| expect(workMocks.currentWork.removeSessionFromList).toHaveBeenCalledWith("cli-single"); | ||
| expect(workMocks.currentWork.closeTab).toHaveBeenCalledWith("cli-single"); | ||
| }); | ||
| expect(agentChatDelete).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("does not delete when the stop-and-delete confirmation is dismissed", async () => { | ||
| const runningCli = workMocks.makeTerminalSession("cli-cancel", "lane-primary", "codex"); | ||
| const sessionDelete = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| Object.defineProperty(window, "ade", { | ||
| configurable: true, | ||
| value: { | ||
| agentChat: { delete: vi.fn() }, | ||
| builtInBrowser: { onEvent: vi.fn(() => vi.fn()) }, | ||
| sessions: { delete: sessionDelete }, | ||
| }, | ||
| }); | ||
| workMocks.currentWork = { | ||
| ...workMocks.baseWork, | ||
| sessions: [runningCli], | ||
| visibleSessions: [runningCli], | ||
| runningFiltered: [runningCli], | ||
| runningSessions: [runningCli], | ||
| filtered: [runningCli], | ||
| sessionsGroupedByLane: new Map([["lane-primary", [runningCli]]]), | ||
| closingPtyIds: new Set<string>(), | ||
| }; | ||
|
|
||
| render(<TerminalsPage />); | ||
|
|
||
| fireEvent.click(await screen.findByRole("button", { name: "context menu cli-cancel" })); | ||
| fireEvent.click(await screen.findByRole("button", { name: "context stop and delete cli-cancel" })); | ||
| fireEvent.click(await screen.findByRole("button", { name: "CANCEL" })); | ||
|
|
||
| await waitFor(() => | ||
| expect(screen.queryByRole("button", { name: "Stop & delete" })).toBeNull(), | ||
| ); | ||
| expect(sessionDelete).not.toHaveBeenCalled(); | ||
| expect(workMocks.currentWork.removeSessionFromList).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("stops and deletes a mixed selection of running CLI and chat sessions", async () => { | ||
| const runningCli = workMocks.makeTerminalSession("cli-running", "lane-primary", "codex"); | ||
| const runningChat = workMocks.makeTerminalSession("chat-running", "lane-primary", "codex-chat", { | ||
| ptyId: null, | ||
| }); | ||
| const agentChatDelete = vi.fn().mockResolvedValue(undefined); | ||
| const sessionDelete = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| Object.defineProperty(window, "ade", { | ||
| configurable: true, | ||
| value: { | ||
| agentChat: { delete: agentChatDelete }, | ||
| builtInBrowser: { onEvent: vi.fn(() => vi.fn()) }, | ||
| sessions: { delete: sessionDelete }, | ||
| }, | ||
| }); | ||
| workMocks.currentWork = { | ||
| ...workMocks.baseWork, | ||
| sessions: [runningCli, runningChat], | ||
| visibleSessions: [runningCli, runningChat], | ||
| runningFiltered: [runningCli, runningChat], | ||
| runningSessions: [runningCli, runningChat], | ||
| filtered: [runningCli, runningChat], | ||
| sessionsGroupedByLane: new Map([["lane-primary", [runningCli, runningChat]]]), | ||
| closingPtyIds: new Set<string>(), | ||
| }; | ||
|
|
||
| render(<TerminalsPage />); | ||
|
|
||
| fireEvent.click(await screen.findByRole("button", { name: "select cli-running" }), { metaKey: true }); | ||
| fireEvent.click(await screen.findByRole("button", { name: "select chat-running" }), { metaKey: true }); | ||
| fireEvent.click(await screen.findByRole("button", { name: "bulk stop and delete" })); | ||
|
|
||
| // The styled confirmation dialog gates the destructive action. | ||
| fireEvent.click(await screen.findByRole("button", { name: "Stop & delete" })); | ||
|
|
||
| await waitFor(() => { | ||
| // The running CLI session is stopped+deleted via the session-delete service, | ||
| // and the chat is removed via the chat delete flow — both in one action. | ||
| expect(sessionDelete).toHaveBeenCalledWith({ sessionId: "cli-running" }); | ||
| expect(agentChatDelete).toHaveBeenCalledWith({ sessionId: "chat-running" }); | ||
| expect(workMocks.currentWork.removeSessionFromList).toHaveBeenCalledWith("cli-running"); | ||
| expect(workMocks.currentWork.removeSessionFromList).toHaveBeenCalledWith("chat-running"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The three new TerminalsPage tests exercise the context-menu path and the bulk path, but there is no test verifying that clicking "Stop & delete" inside SessionInfoPopover triggers the same handler and shows the confirmation dialog. The popover mock (vi.mock("./SessionInfoPopover", ...)) currently renders null, so that surface is untested. A short test mirroring the context-menu confirm/cancel test but opening the popover first would catch any future divergence.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx
Line: 539-667
Comment:
**No test coverage for the info-popover stop-and-delete path**
The three new `TerminalsPage` tests exercise the context-menu path and the bulk path, but there is no test verifying that clicking "Stop & delete" inside `SessionInfoPopover` triggers the same handler and shows the confirmation dialog. The popover mock (`vi.mock("./SessionInfoPopover", ...)`) currently renders `null`, so that surface is untested. A short test mirroring the context-menu confirm/cancel test but opening the popover first would catch any future divergence.
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!
Owner
Author
There was a problem hiding this comment.
The info-popover path invokes the exact same handleStopAndDeleteSession handler that the context-menu confirm/cancel tests already exercise, and the popover→handler wiring is a typechecked one-liner. A render test of SessionInfoPopover would import TerminalView (xterm) and the localStorage-dependent terminal harness, making it brittle for near-zero added signal — so it's intentionally not added. No change.
| disabled={deletingSessionId === session.id} | ||
| onClick={() => { onStopAndDelete(session); onClose(); }} | ||
| > | ||
| {deletingSessionId === session.id ? "Deleting..." : "Stop and delete"} |
There was a problem hiding this comment.
disabled state prevents re-clicking during deletion, but the "Deleting..." text uses an ASCII ellipsis while the sibling SessionInfoPopover uses "Deleting…" (Unicode). Using the same character keeps the two surfaces consistent.
Suggested change
| {deletingSessionId === session.id ? "Deleting..." : "Stop and delete"} | |
| {deletingSessionId === session.id ? "Deleting…" : "Stop and delete"} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/terminals/SessionContextMenu.tsx
Line: 172
Comment:
The `disabled` state prevents re-clicking during deletion, but the `"Deleting..."` text uses an ASCII ellipsis while the sibling `SessionInfoPopover` uses `"Deleting…"` (Unicode). Using the same character keeps the two surfaces consistent.
```suggestion
{deletingSessionId === session.id ? "Deleting…" : "Stop and delete"}
```
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!
…u/popover Addresses CodeRabbit + Greptile review nits: standardize the action label to 'Stop & delete' and the in-progress text to 'Deleting…' (unicode) across the session context menu and info popover so all surfaces match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 7, 2026 01:11
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
1 participant
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
Adds Stop and delete for runtime-backed sessions in the Work tab.
Single session
Multi-select
Both flows are gated by a styled confirmation dialog (reusing
useConfirmDialog/ConfirmDialog).How it works
The session-delete service (
deleteTerminalSessionWithRuntimeCleanup) already stops a running runtime before removing the record, so a singlesessions.deletecall covers both steps for CLI/shell. The bulk handler routes chats →agentChat.deleteand everything else →sessions.delete.Tests
afterEach(cleanup)for isolation.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Greptile Summary
This PR adds a "Stop & delete" action for runtime-backed sessions, available from the sidebar context menu, the work-area header (right-click now wired), the session info popover, and as a bulk multi-select action. It also fixes a pre-existing bug where a mixed selection containing a running CLI session couldn't be bulk-deleted in one step.
handleStopAndDeleteSessionshows a styledConfirmDialog, then delegates towindow.ade.sessions.delete, which already stops the runtime server-side before removing the record — so one call covers both steps.handleBulkStopAndDeleteSelectedroutes chats toagentChat.deleteand everything else tosessions.delete, resolves them concurrently viaallSettledWithConcurrency, and clears the selection on success. Both flows guard the destructive action with the sharedstopAndDeleteConfirmdialog instance.SessionListPane.Confidence Score: 5/5
Safe to merge. The new stop-and-delete paths follow the same error handling and local-state cleanup patterns as the existing bulk-delete and single-delete handlers, and both flows are gated behind a styled confirmation dialog.
The new handlers are structurally consistent with existing delete handlers: they use allSettledWithConcurrency for bulk operations, clear selections and close menus on success, log errors without swallowing them, and rely on the server-side sessions.delete to handle runtime teardown atomically. No state mutation bypasses, unchecked branches, or missing cleanup steps were found in the changed code.
No files require special attention.
Important Files Changed
Reviews (2): Last reviewed commit: "ship: iteration 1 — unify 'Stop & delete..." | Re-trigger Greptile