Add native context menu to delete threads#12
Merged
juliusmarminge merged 1 commit intomainfrom Feb 11, 2026
Merged
Conversation
|
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:
WalkthroughAdds a cross-platform context menu: Electron main IPC handler and preload bridge, contract/type additions, web fallback UI, and web UI + store updates to show a thread context menu and delete threads when "Delete" is selected. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar as "Web UI (Sidebar)"
participant API as "NativeApi / Bridge"
participant Main as "Electron Main"
participant Store as "App Store"
User->>Sidebar: Right-click thread (x,y)
Sidebar->>API: contextMenu.show(items, position)
alt Desktop bridge available
API->>Main: IPC invoke "desktop:context-menu" with items
Main->>Main: Build native Menu and popup at focused window
User->>Main: Select option or dismiss
Main-->>API: Resolve selected item id or null
else Web fallback
API->>API: showContextMenuFallback(items, position)
User->>API: Select option or dismiss
API-->>Sidebar: Resolve selected item id or null
end
Sidebar->>Sidebar: If selected "Delete": stop session (if any)
Sidebar->>Store: Dispatch DELETE_THREAD(threadId)
Store->>Store: Remove thread, reassign activeThreadId if needed
Store-->>Sidebar: Updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Contributor
Add a native context menu for thread deletion and wire right-click in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main.ts`:
- Line 6: Replace usages of BrowserWindow.getFocusedWindow() when building/popup
context menus in the ipc handler with
BrowserWindow.fromWebContents(event.sender) so the menu is resolved from the IPC
sender's webContents; update the IPC handler that receives the menu payload (the
ipcMain.handle callback) to validate/parse the incoming array into the shared
ContextMenuItem type using the existing IPC validation utility before mapping to
Menu.buildFromTemplate, and pass BrowserWindow.fromWebContents(event.sender)
into Menu.popup to avoid null or wrong focused-window cases.
In `@apps/renderer/src/components/Sidebar.tsx`:
- Around line 87-112: handleThreadContextMenu currently swallows errors from
api.providers.stopSession and proceeds to dispatch DELETE_THREAD, which can
orphan an active session; change the flow in handleThreadContextMenu so that if
stopSession throws you do not delete the thread unless you can detect a harmless
"already stopped" condition—catch the error from api.providers.stopSession,
check the error message/code for an “already stopped” sentinel (or otherwise
surface the error via UI/api.logger) and abort the dispatch({ type:
"DELETE_THREAD", ... }) when stopSession failed, only allowing deletion when
stopSession succeeded or the error explicitly indicates the session was already
stopped; reference handleThreadContextMenu, api.providers.stopSession, and the
DELETE_THREAD dispatch to locate and update the logic.
In `@apps/renderer/src/store.ts`:
- Around line 45-46: The DELETE_THREAD action currently falls back to the first
remaining thread globally which can switch projects; change the reducer logic
that handles { type: "DELETE_THREAD"; threadId: string } so that if the deleted
thread equals the current active thread (activeThreadId) you find the next
active thread from the same project only (e.g., filter threads by projectId ===
deletedThread.projectId) and set activeThreadId to that thread's id or null if
none; apply the same same-project fallback change to the other deletion-handling
block referenced (the logic duplicated around the other DELETE_THREAD handling
at the second location).
| if (isQuitting || restartTimer) return; | ||
| import { spawn } from "node:child_process"; | ||
| import path from "node:path"; | ||
| import { BrowserWindow, Menu, app, dialog, ipcMain, session, shell } from "electron"; |
There was a problem hiding this comment.
Resolve the menu window from the IPC sender to avoid null focused-window cases.
BrowserWindow.getFocusedWindow() can be null or the wrong window in multi-window or background scenarios; using the sender’s webContents is safer. Also consider using the shared ContextMenuItem type and parsing payloads via the existing IPC validation pattern to avoid malformed item arrays.
🔧 Suggested update
-import {
- EDITORS,
- IPC_CHANNELS,
- type TerminalCommandInput,
- type TerminalCommandResult,
- agentConfigSchema,
- agentSessionIdSchema,
- newTodoInputSchema,
- providerInterruptTurnInputSchema,
- providerRespondToRequestInputSchema,
- providerSendTurnInputSchema,
- providerSessionStartInputSchema,
- providerStopSessionInputSchema,
- terminalCommandInputSchema,
- todoIdSchema,
-} from "@acme/contracts";
+import {
+ EDITORS,
+ IPC_CHANNELS,
+ type ContextMenuItem,
+ type TerminalCommandInput,
+ type TerminalCommandResult,
+ agentConfigSchema,
+ agentSessionIdSchema,
+ newTodoInputSchema,
+ providerInterruptTurnInputSchema,
+ providerRespondToRequestInputSchema,
+ providerSendTurnInputSchema,
+ providerSessionStartInputSchema,
+ providerStopSessionInputSchema,
+ terminalCommandInputSchema,
+ todoIdSchema,
+} from "@acme/contracts";
@@
- ipcMain.handle(IPC_CHANNELS.contextMenuShow, async (_event, items: { id: string; label: string }[]) => {
- const window = BrowserWindow.getFocusedWindow();
+ ipcMain.handle(IPC_CHANNELS.contextMenuShow, async (event, items: ContextMenuItem[]) => {
+ const window =
+ BrowserWindow.fromWebContents(event.sender) ?? BrowserWindow.getFocusedWindow();
if (!window) return null;Electron BrowserWindow.fromWebContents usage for Menu.popup — recommended window selection when showing a context menu from ipcMain.handle?
Also applies to: 152-169
🤖 Prompt for AI Agents
In `@apps/desktop/src/main.ts` at line 6, Replace usages of
BrowserWindow.getFocusedWindow() when building/popup context menus in the ipc
handler with BrowserWindow.fromWebContents(event.sender) so the menu is resolved
from the IPC sender's webContents; update the IPC handler that receives the menu
payload (the ipcMain.handle callback) to validate/parse the incoming array into
the shared ContextMenuItem type using the existing IPC validation utility before
mapping to Menu.buildFromTemplate, and pass
BrowserWindow.fromWebContents(event.sender) into Menu.popup to avoid null or
wrong focused-window cases.
Comment on lines
+87
to
+112
| const handleThreadContextMenu = useCallback( | ||
| async (threadId: string) => { | ||
| if (!api) return; | ||
| const clicked = await api.contextMenu.show([ | ||
| { id: "delete", label: "Delete" }, | ||
| ]); | ||
| if (clicked !== "delete") return; | ||
|
|
||
| const thread = state.threads.find((t) => t.id === threadId); | ||
| if (!thread) return; | ||
|
|
||
| // Stop active session if running | ||
| if (thread.session?.sessionId) { | ||
| try { | ||
| await api.providers.stopSession({ | ||
| sessionId: thread.session.sessionId, | ||
| }); | ||
| } catch { | ||
| // Session may already be stopped | ||
| } | ||
| } | ||
|
|
||
| dispatch({ type: "DELETE_THREAD", threadId }); | ||
| }, | ||
| [api, dispatch, state.threads], | ||
| ); |
There was a problem hiding this comment.
Don’t delete the thread if stopping the session fails.
Errors are swallowed and deletion proceeds, which can orphan a running provider session with no UI to stop it. Consider surfacing the error (or at least aborting deletion) unless you can distinguish “already stopped.”
🛡️ Safer flow
if (thread.session?.sessionId) {
try {
await api.providers.stopSession({
sessionId: thread.session.sessionId,
});
- } catch {
- // Session may already be stopped
+ } catch (error) {
+ // Consider surfacing error so we don't orphan a running session
+ return;
}
}
dispatch({ type: "DELETE_THREAD", threadId });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleThreadContextMenu = useCallback( | |
| async (threadId: string) => { | |
| if (!api) return; | |
| const clicked = await api.contextMenu.show([ | |
| { id: "delete", label: "Delete" }, | |
| ]); | |
| if (clicked !== "delete") return; | |
| const thread = state.threads.find((t) => t.id === threadId); | |
| if (!thread) return; | |
| // Stop active session if running | |
| if (thread.session?.sessionId) { | |
| try { | |
| await api.providers.stopSession({ | |
| sessionId: thread.session.sessionId, | |
| }); | |
| } catch { | |
| // Session may already be stopped | |
| } | |
| } | |
| dispatch({ type: "DELETE_THREAD", threadId }); | |
| }, | |
| [api, dispatch, state.threads], | |
| ); | |
| const handleThreadContextMenu = useCallback( | |
| async (threadId: string) => { | |
| if (!api) return; | |
| const clicked = await api.contextMenu.show([ | |
| { id: "delete", label: "Delete" }, | |
| ]); | |
| if (clicked !== "delete") return; | |
| const thread = state.threads.find((t) => t.id === threadId); | |
| if (!thread) return; | |
| // Stop active session if running | |
| if (thread.session?.sessionId) { | |
| try { | |
| await api.providers.stopSession({ | |
| sessionId: thread.session.sessionId, | |
| }); | |
| } catch (error) { | |
| // Consider surfacing error so we don't orphan a running session | |
| return; | |
| } | |
| } | |
| dispatch({ type: "DELETE_THREAD", threadId }); | |
| }, | |
| [api, dispatch, state.threads], | |
| ); |
🤖 Prompt for AI Agents
In `@apps/renderer/src/components/Sidebar.tsx` around lines 87 - 112,
handleThreadContextMenu currently swallows errors from api.providers.stopSession
and proceeds to dispatch DELETE_THREAD, which can orphan an active session;
change the flow in handleThreadContextMenu so that if stopSession throws you do
not delete the thread unless you can detect a harmless "already stopped"
condition—catch the error from api.providers.stopSession, check the error
message/code for an “already stopped” sentinel (or otherwise surface the error
via UI/api.logger) and abort the dispatch({ type: "DELETE_THREAD", ... }) when
stopSession failed, only allowing deletion when stopSession succeeded or the
error explicitly indicates the session was already stopped; reference
handleThreadContextMenu, api.providers.stopSession, and the DELETE_THREAD
dispatch to locate and update the logic.
Comment on lines
+45
to
+46
| | { type: "SET_RUNTIME_MODE"; mode: RuntimeMode } | ||
| | { type: "DELETE_THREAD"; threadId: string }; |
There was a problem hiding this comment.
Active-thread fallback should stay within the same project.
When deleting the active thread, picking the first remaining thread globally can jump the user into an unrelated project. Prefer the next thread in the same project (or null if none).
🔧 Suggested adjustment
case "DELETE_THREAD": {
+ const deleted = state.threads.find((t) => t.id === action.threadId);
const threads = state.threads.filter((t) => t.id !== action.threadId);
- const activeThreadId =
- state.activeThreadId === action.threadId
- ? (threads[0]?.id ?? null)
- : state.activeThreadId;
+ const fallback =
+ deleted?.projectId
+ ? threads.find((t) => t.projectId === deleted.projectId) ?? null
+ : threads[0] ?? null;
+ const activeThreadId =
+ state.activeThreadId === action.threadId
+ ? (fallback?.id ?? null)
+ : state.activeThreadId;
return { ...state, threads, activeThreadId };
}Also applies to: 332-338
🤖 Prompt for AI Agents
In `@apps/renderer/src/store.ts` around lines 45 - 46, The DELETE_THREAD action
currently falls back to the first remaining thread globally which can switch
projects; change the reducer logic that handles { type: "DELETE_THREAD";
threadId: string } so that if the deleted thread equals the current active
thread (activeThreadId) you find the next active thread from the same project
only (e.g., filter threads by projectId === deletedThread.projectId) and set
activeThreadId to that thread's id or null if none; apply the same same-project
fallback change to the other deletion-handling block referenced (the logic
duplicated around the other DELETE_THREAD handling at the second location).
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ]); | ||
| if (clicked !== "delete") return; | ||
|
|
||
| const thread = state.threads.find((t) => t.id === threadId); |
Contributor
There was a problem hiding this comment.
🟢 Low
components/Sidebar.tsx:171 The state.threads captured in the closure may become stale while awaiting api.contextMenu.show. Consider re-reading the thread from the current store state after the await returns, before stopping the session.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/components/Sidebar.tsx around line 171:
The `state.threads` captured in the closure may become stale while awaiting `api.contextMenu.show`. Consider re-reading the thread from the current store state after the await returns, before stopping the session.
| threadId: thread.id, | ||
| }) | ||
| } | ||
| onContextMenu={(e) => { |
Contributor
There was a problem hiding this comment.
🟢 Low
components/Sidebar.tsx:320 Consider only calling e.preventDefault() when api.contextMenu.show will actually display a menu. Currently in non-Electron web mode, the native context menu is blocked but nothing appears because desktopBridge is undefined.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/web/src/components/Sidebar.tsx around line 320:
Consider only calling `e.preventDefault()` when `api.contextMenu.show` will actually display a menu. Currently in non-Electron web mode, the native context menu is blocked but nothing appears because `desktopBridge` is undefined.
Greptile OverviewGreptile SummaryThis PR adds a native, platform-consistent context menu on sidebar thread items to support deleting threads. Changes span the IPC contract ( No merge-blocking issues were found in the reviewed changes; the IPC wiring and type contract are consistent end-to-end. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (Sidebar)
participant P as Preload (contextBridge)
participant M as Main Process (ipcMain)
participant E as Electron Menu
participant S as Store/Reducer
R->>P: nativeApi.contextMenu.show([{id,label}])
P->>M: ipcRenderer.invoke("context-menu:show", items)
M->>E: Menu.buildFromTemplate(items)
M->>E: menu.popup(window)
alt User clicks item
E-->>M: click handler (resolve(item.id))
M-->>P: Promise resolves with id
P-->>R: Promise resolves with id
R->>P: nativeApi.providers.stopSession({sessionId}) (optional)
P->>M: ipcRenderer.invoke("provider:session:stop", payload)
M-->>P: resolves
P-->>R: resolves
R->>S: dispatch({type:"DELETE_THREAD", threadId})
S-->>R: state updates (threads filtered, activeThreadId updated)
else User dismisses menu
E-->>M: popup callback (resolve(null))
M-->>P: Promise resolves null
P-->>R: Promise resolves null
end
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/contracts/src/ipc.ts`:
- Around line 28-31: Remove the unused exported interface ContextMenuItem from
the file: delete the `export interface ContextMenuItem { id: string; label:
string }` declaration and any related export references so the contract aligns
with the inline generic used by `contextMenu.show` (which relies on `{ id: T;
label: string }[]` for proper inference); also run a quick grep for
`ContextMenuItem` and remove any leftover imports/usages elsewhere if found.
Adds right-click → Delete on sidebar thread items using Electron's native Menu.popup() when running in desktop, with an imperative DOM fallback for browser environments. The contextMenu.show() API is generic so the returned item id is type-safe based on the items passed. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
juliusmarminge
force-pushed
the
juliusmarminge/sidebar-delete-thread
branch
from
ab360e3 to
eef2901
Compare
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.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit