Skip to content

Commit 96ad7a5

Browse files
Copilotsawka
Copilot
and
sawka
authored
Wire deterministic context-menu close signaling across Electron and renderer (#2932)
Context-menu opens previously produced renderer callbacks only on selection (0 or 1 events). This change makes the lifecycle deterministic: every menu open now emits exactly one completion signal, with `null` on cancel and item id on selection. - **Main process: single terminal callback per popup** - Updated `emain/emain-menu.ts` to use `menu.popup({ callback })`. - Tracks whether a menu item click occurred during the popup. - Emits `contextmenu-click: null` only when the menu closes without selection. - Suppresses duplicate close events when a click already fired. - **Preload + API typing: nullable context-menu callback payload** - Updated preload bridge and `frontend/types/custom.d.ts` so: - `onContextMenuClick` now accepts `(id: string | null) => void`. - Keeps existing channel semantics while allowing explicit cancel signal. - **Renderer context menu model: close/select/cancel hooks** - Extended `showContextMenu` with optional `opts`: - `onSelect?: (item) => void` - `onCancel?: () => void` - `onClose?: (item: MenuItem | null) => void` - Execution order on selection: 1. original item `click` 2. `onSelect` 3. `onClose` - Execution order on cancel: 1. `onCancel` 2. `onClose(null)` - **Targeted behavior tests** - Expanded `frontend/app/store/contextmenu.test.ts` to verify: - singleton wiring still initializes once, - selection path ordering (`click -> onSelect -> onClose`), - cancel path ordering (`onCancel -> onClose(null)`). ```ts ContextMenuModel.getInstance().showContextMenu(menu, e, { onSelect: (item) => { /* after item.click */ }, onCancel: () => { /* close without selection */ }, onClose: (itemOrNull) => { /* always called */ }, }); ``` <screenshot> Not applicable — this is a behavioral/API flow change in native context menu lifecycle rather than a visual UI update. </screenshot> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: sawka <[email protected]> Co-authored-by: sawka <[email protected]>
1 parent 752265f commit 96ad7a5

File tree

5 files changed

+154
-26
lines changed

5 files changed

+154
-26
lines changed

‎emain/emain-menu.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -408,27 +408,24 @@ function getWebContentsByWorkspaceOrBuilderId(workspaceOrBuilderId: string): ele
408408

409409
function convertMenuDefArrToMenu(
410410
webContents: electron.WebContents,
411-
menuDefArr: ElectronContextMenuItem[]
411+
menuDefArr: ElectronContextMenuItem[],
412+
menuState: { hasClick: boolean }
412413
): electron.Menu {
413414
const menuItems: electron.MenuItem[] = [];
414415
for (const menuDef of menuDefArr) {
415416
const menuItemTemplate: electron.MenuItemConstructorOptions = {
416417
role: menuDef.role as any,
417418
label: menuDef.label,
418419
type: menuDef.type,
419-
click: (_, window) => {
420-
const wc = getWindowWebContents(window) ?? webContents;
421-
if (!wc) {
422-
console.error("invalid window for context menu click handler:", window);
423-
return;
424-
}
425-
wc.send("contextmenu-click", menuDef.id);
420+
click: () => {
421+
menuState.hasClick = true;
422+
webContents.send("contextmenu-click", menuDef.id);
426423
},
427424
checked: menuDef.checked,
428425
enabled: menuDef.enabled,
429426
};
430427
if (menuDef.submenu != null) {
431-
menuItemTemplate.submenu = convertMenuDefArrToMenu(webContents, menuDef.submenu);
428+
menuItemTemplate.submenu = convertMenuDefArrToMenu(webContents, menuDef.submenu, menuState);
432429
}
433430
const menuItem = new electron.MenuItem(menuItemTemplate);
434431
menuItems.push(menuItem);
@@ -439,19 +436,27 @@ function convertMenuDefArrToMenu(
439436
electron.ipcMain.on(
440437
"contextmenu-show",
441438
(event, workspaceOrBuilderId: string, menuDefArr: ElectronContextMenuItem[]) => {
439+
const webContents = getWebContentsByWorkspaceOrBuilderId(workspaceOrBuilderId);
440+
if (!webContents) {
441+
console.error("invalid window for context menu:", workspaceOrBuilderId);
442+
event.returnValue = true;
443+
return;
444+
}
442445
if (menuDefArr.length === 0) {
446+
webContents.send("contextmenu-click", null);
443447
event.returnValue = true;
444448
return;
445449
}
446450
fireAndForget(async () => {
447-
const webContents = getWebContentsByWorkspaceOrBuilderId(workspaceOrBuilderId);
448-
if (!webContents) {
449-
console.error("invalid window for context menu:", workspaceOrBuilderId);
450-
return;
451-
}
452-
453-
const menu = convertMenuDefArrToMenu(webContents, menuDefArr);
454-
menu.popup();
451+
const menuState = { hasClick: false };
452+
const menu = convertMenuDefArrToMenu(webContents, menuDefArr, menuState);
453+
menu.popup({
454+
callback: () => {
455+
if (!menuState.hasClick) {
456+
webContents.send("contextmenu-click", null);
457+
}
458+
},
459+
});
455460
});
456461
event.returnValue = true;
457462
}

‎emain/preload.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ contextBridge.exposeInMainWorld("api", {
2121
showWorkspaceAppMenu: (workspaceId) => ipcRenderer.send("workspace-appmenu-show", workspaceId),
2222
showBuilderAppMenu: (builderId) => ipcRenderer.send("builder-appmenu-show", builderId),
2323
showContextMenu: (workspaceId, menu) => ipcRenderer.send("contextmenu-show", workspaceId, menu),
24-
onContextMenuClick: (callback) => ipcRenderer.on("contextmenu-click", (_event, id) => callback(id)),
24+
onContextMenuClick: (callback: (id: string | null) => void) =>
25+
ipcRenderer.on("contextmenu-click", (_event, id: string | null) => callback(id)),
2526
downloadFile: (filePath) => ipcRenderer.send("download", { filePath }),
2627
openExternal: (url) => {
2728
if (url && typeof url === "string") {

‎frontend/app/store/contextmenu.test.ts‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@ import { describe, expect, it, vi } from "vitest";
22

33
describe("ContextMenuModel", () => {
44
it("initializes only when getInstance is called", async () => {
5+
let contextMenuCallback: (id: string | null) => void;
56
const onContextMenuClick = vi.fn();
7+
onContextMenuClick.mockImplementation((callback) => {
8+
contextMenuCallback = callback;
9+
});
610
const getApi = vi.fn(() => ({
711
onContextMenuClick,
812
showContextMenu: vi.fn(),
@@ -24,5 +28,107 @@ describe("ContextMenuModel", () => {
2428
expect(firstInstance).toBe(secondInstance);
2529
expect(getApi).toHaveBeenCalledTimes(1);
2630
expect(onContextMenuClick).toHaveBeenCalledTimes(1);
31+
expect(contextMenuCallback).toBeTypeOf("function");
32+
});
33+
34+
it("runs select and close callbacks after item handler", async () => {
35+
let contextMenuCallback: (id: string | null) => void;
36+
const showContextMenu = vi.fn();
37+
const onContextMenuClick = vi.fn((callback) => {
38+
contextMenuCallback = callback;
39+
});
40+
const getApi = vi.fn(() => ({
41+
onContextMenuClick,
42+
showContextMenu,
43+
}));
44+
const workspace = { oid: "workspace-1" };
45+
46+
vi.resetModules();
47+
vi.doMock("./global", () => ({
48+
atoms: { workspace: "workspace", builderId: "builderId" },
49+
getApi,
50+
globalStore: {
51+
get: vi.fn((atom) => {
52+
if (atom === "workspace") {
53+
return workspace;
54+
}
55+
return "builder-1";
56+
}),
57+
},
58+
}));
59+
60+
const { ContextMenuModel } = await import("./contextmenu");
61+
const model = ContextMenuModel.getInstance();
62+
const order: string[] = [];
63+
const itemClick = vi.fn(() => {
64+
order.push("item");
65+
});
66+
const onSelect = vi.fn((item) => {
67+
order.push(`select:${item.label}`);
68+
});
69+
const onClose = vi.fn((item) => {
70+
order.push(`close:${item?.label ?? "null"}`);
71+
});
72+
73+
model.showContextMenu(
74+
[{ label: "Open", click: itemClick }],
75+
{ stopPropagation: vi.fn() } as any,
76+
{ onSelect, onClose }
77+
);
78+
const menuId = showContextMenu.mock.calls[0][1][0].id;
79+
contextMenuCallback(menuId);
80+
81+
expect(order).toEqual(["item", "select:Open", "close:Open"]);
82+
expect(itemClick).toHaveBeenCalledTimes(1);
83+
expect(onSelect).toHaveBeenCalledTimes(1);
84+
expect(onClose).toHaveBeenCalledTimes(1);
85+
});
86+
87+
it("runs cancel and close callbacks when no item is selected", async () => {
88+
let contextMenuCallback: (id: string | null) => void;
89+
const showContextMenu = vi.fn();
90+
const onContextMenuClick = vi.fn((callback) => {
91+
contextMenuCallback = callback;
92+
});
93+
const getApi = vi.fn(() => ({
94+
onContextMenuClick,
95+
showContextMenu,
96+
}));
97+
const workspace = { oid: "workspace-1" };
98+
99+
vi.resetModules();
100+
vi.doMock("./global", () => ({
101+
atoms: { workspace: "workspace", builderId: "builderId" },
102+
getApi,
103+
globalStore: {
104+
get: vi.fn((atom) => {
105+
if (atom === "workspace") {
106+
return workspace;
107+
}
108+
return "builder-1";
109+
}),
110+
},
111+
}));
112+
113+
const { ContextMenuModel } = await import("./contextmenu");
114+
const model = ContextMenuModel.getInstance();
115+
const order: string[] = [];
116+
const onCancel = vi.fn(() => {
117+
order.push("cancel");
118+
});
119+
const onClose = vi.fn((item) => {
120+
order.push(`close:${item == null ? "null" : item.label}`);
121+
});
122+
123+
model.showContextMenu(
124+
[{ label: "Open", click: vi.fn() }],
125+
{ stopPropagation: vi.fn() } as any,
126+
{ onCancel, onClose }
127+
);
128+
contextMenuCallback(null);
129+
130+
expect(order).toEqual(["cancel", "close:null"]);
131+
expect(onCancel).toHaveBeenCalledTimes(1);
132+
expect(onClose).toHaveBeenCalledTimes(1);
27133
});
28134
});

‎frontend/app/store/contextmenu.ts‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,16 @@
33

44
import { atoms, getApi, globalStore } from "./global";
55

6+
type ShowContextMenuOpts = {
7+
onSelect?: (item: ContextMenuItem) => void;
8+
onCancel?: () => void;
9+
onClose?: (item: ContextMenuItem | null) => void;
10+
};
11+
612
class ContextMenuModel {
713
private static instance: ContextMenuModel;
8-
handlers: Map<string, () => void> = new Map(); // id -> handler
14+
handlers: Map<string, ContextMenuItem> = new Map(); // id -> item
15+
activeOpts: ShowContextMenuOpts | null = null;
916

1017
private constructor() {
1118
getApi().onContextMenuClick(this.handleContextMenuClick.bind(this));
@@ -18,11 +25,19 @@ class ContextMenuModel {
1825
return ContextMenuModel.instance;
1926
}
2027

21-
handleContextMenuClick(id: string): void {
22-
const handler = this.handlers.get(id);
23-
if (handler) {
24-
handler();
28+
handleContextMenuClick(id: string | null): void {
29+
const opts = this.activeOpts;
30+
this.activeOpts = null;
31+
const item = id != null ? this.handlers.get(id) : null;
32+
this.handlers.clear();
33+
if (item == null) {
34+
opts?.onCancel?.();
35+
opts?.onClose?.(null);
36+
return;
2537
}
38+
item.click?.();
39+
opts?.onSelect?.(item);
40+
opts?.onClose?.(item);
2641
}
2742

2843
_convertAndRegisterMenu(menu: ContextMenuItem[]): ElectronContextMenuItem[] {
@@ -43,7 +58,7 @@ class ContextMenuModel {
4358
electronItem.enabled = false;
4459
}
4560
if (item.click) {
46-
this.handlers.set(electronItem.id, item.click);
61+
this.handlers.set(electronItem.id, item);
4762
}
4863
if (item.submenu) {
4964
electronItem.submenu = this._convertAndRegisterMenu(item.submenu);
@@ -53,9 +68,10 @@ class ContextMenuModel {
5368
return electronMenuItems;
5469
}
5570

56-
showContextMenu(menu: ContextMenuItem[], ev: React.MouseEvent<any>): void {
71+
showContextMenu(menu: ContextMenuItem[], ev: React.MouseEvent<any>, opts?: ShowContextMenuOpts): void {
5772
ev.stopPropagation();
5873
this.handlers.clear();
74+
this.activeOpts = opts;
5975
const electronMenuItems = this._convertAndRegisterMenu(menu);
6076

6177
const workspace = globalStore.get(atoms.workspace);

‎frontend/types/custom.d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ declare global {
9393
showWorkspaceAppMenu: (workspaceId: string) => void; // workspace-appmenu-show
9494
showBuilderAppMenu: (builderId: string) => void; // builder-appmenu-show
9595
showContextMenu: (workspaceId: string, menu: ElectronContextMenuItem[]) => void; // contextmenu-show
96-
onContextMenuClick: (callback: (id: string) => void) => void; // contextmenu-click
96+
onContextMenuClick: (callback: (id: string | null) => void) => void; // contextmenu-click
9797
onNavigate: (callback: (url: string) => void) => void;
9898
onIframeNavigate: (callback: (url: string) => void) => void;
9999
downloadFile: (path: string) => void; // download

0 commit comments

Comments
 (0)