From 3c0665543f0e31db1e3425d4fcf9f4aea295a8ce Mon Sep 17 00:00:00 2001 From: maria Date: Wed, 19 Aug 2026 16:09:40 -0400 Subject: [PATCH] feat: refine thread action menus (#7476) --- .../desktop/src/electron/ElectronMenu.test.ts | 37 +++- apps/desktop/src/electron/ElectronMenu.ts | 19 +- .../components/threadActionMenu.logic.test.ts | 17 +- .../src/components/threadActionMenu.logic.ts | 49 +++-- apps/web/src/contextMenuFallback.test.ts | 91 +++++++++ apps/web/src/contextMenuFallback.ts | 174 ++++++++++++++++-- packages/contracts/src/ipc.ts | 4 + 7 files changed, 353 insertions(+), 38 deletions(-) diff --git a/apps/desktop/src/electron/ElectronMenu.test.ts b/apps/desktop/src/electron/ElectronMenu.test.ts index 58870bbab1db2b26ae40ac11eb2659e5a31f4acf..756274a614d750cea29d7243c8536c501611a919 100644 --- a/apps/desktop/src/electron/ElectronMenu.test.ts +++ b/apps/desktop/src/electron/ElectronMenu.test.ts @@ -98,7 +98,10 @@ describe("ElectronMenu", () => { const electronMenu = yield* ElectronMenu.ElectronMenu; const selectedItemId = yield* electronMenu.showContextMenu({ window: makeWindow(2), - items: [{ id: "copy", label: "Copy" }], + items: [ + { id: "copy", label: "Copy" }, + { id: "delete", label: "Delete", destructive: true, separatorBefore: true }, + ], position: Option.some({ x: 10.8, y: 20.2 }), }); @@ -110,6 +113,38 @@ describe("ElectronMenu", () => { enabled: true, click: buildFromTemplateMock.mock.calls[0]?.[0][0].click, }); + assert.deepEqual( + buildFromTemplateMock.mock.calls[0]?.[0].map( + (item: Electron.MenuItemConstructorOptions) => item.type ?? item.label, + ), + ["Copy", "separator", "Delete"], + ); + }).pipe(Effect.provide(TestLayer)), + ); + + it.effect("keeps a preceding non-destructive action in the destructive section", () => + Effect.gen(function* () { + buildFromTemplateMock.mockImplementation(() => ({ + popup: (options: Electron.PopupOptions) => options.callback?.(), + })); + + const electronMenu = yield* ElectronMenu.ElectronMenu; + yield* electronMenu.showContextMenu({ + window: makeWindow(), + items: [ + { id: "copy", label: "Copy" }, + { id: "archive", label: "Archive", separatorBefore: true }, + { id: "delete", label: "Delete", destructive: true }, + ], + position: Option.none(), + }); + + assert.deepEqual( + buildFromTemplateMock.mock.calls[0]?.[0].map( + (item: Electron.MenuItemConstructorOptions) => item.type ?? item.label, + ), + ["Copy", "separator", "Archive", "Delete"], + ); }).pipe(Effect.provide(TestLayer)), ); diff --git a/apps/desktop/src/electron/ElectronMenu.ts b/apps/desktop/src/electron/ElectronMenu.ts index 4d3e5a1c24160d9c0937a7cd09ea84f20274dda9..b241619cd296f32fffb6ba642efd9eed17c667c7 100644 --- a/apps/desktop/src/electron/ElectronMenu.ts +++ b/apps/desktop/src/electron/ElectronMenu.ts @@ -78,6 +78,7 @@ function normalizeContextMenuItems(source: readonly ContextMenuItem[]): ContextM label: sourceItem.label, destructive: sourceItem.destructive === true, disabled: sourceItem.disabled === true, + ...(sourceItem.separatorBefore === true ? { separatorBefore: true } : {}), }; if (sourceItem.children) { @@ -141,10 +142,24 @@ export const make = Effect.gen(function* () { ): Electron.MenuItemConstructorOptions[] => { const template: Electron.MenuItemConstructorOptions[] = []; let hasInsertedDestructiveSeparator = false; + let sectionStartedByExplicitSeparator = false; + const appendSeparator = () => { + if (template.length === 0 || template.at(-1)?.type === "separator") return; + template.push({ type: "separator" }); + }; for (const item of entries) { - if (item.destructive && !hasInsertedDestructiveSeparator && template.length > 0) { - template.push({ type: "separator" }); + if (item.separatorBefore) { + appendSeparator(); + sectionStartedByExplicitSeparator = true; + } + if ( + item.destructive && + !hasInsertedDestructiveSeparator && + !sectionStartedByExplicitSeparator && + template.length > 0 + ) { + appendSeparator(); hasInsertedDestructiveSeparator = true; } diff --git a/apps/web/src/components/threadActionMenu.logic.test.ts b/apps/web/src/components/threadActionMenu.logic.test.ts index c839ddc3be75e632182d5eb4e71f1468104e4cdf..96931bc8f8757b2fe8b47cd24092e0cf5535e017 100644 --- a/apps/web/src/components/threadActionMenu.logic.test.ts +++ b/apps/web/src/components/threadActionMenu.logic.test.ts @@ -20,6 +20,12 @@ function ids(state: ThreadActionMenuState): string[] { return buildThreadActionMenuItems(state).map((item) => item.id); } +function allIds(state: ThreadActionMenuState): string[] { + const flatten = (items: ReturnType): string[] => + items.flatMap((item) => [item.id, ...(item.children ? flatten(item.children) : [])]); + return flatten(buildThreadActionMenuItems(state)); +} + describe("buildThreadActionMenuItems", () => { it("hides lifecycle items when the environment lacks the capabilities", () => { expect( @@ -27,15 +33,15 @@ describe("buildThreadActionMenuItems", () => { ...baseState, supports: { settlement: false, snooze: false, pinning: false, titleRegeneration: false }, }), - ).toEqual(["rename", "mark-unread", "copy-path", "copy-thread-id", "archive", "delete"]); + ).toEqual(["rename", "mark-unread", "copy", "archive", "delete"]); }); it("includes branch items only for threads with a branch", () => { - const withBranch = ids({ ...baseState, branch: "feat/menu" }); + const withBranch = allIds({ ...baseState, branch: "feat/menu" }); expect(withBranch).toContain("new-thread-on-branch"); expect(withBranch).toContain("copy-branch"); - expect(ids(baseState)).not.toContain("new-thread-on-branch"); - expect(ids(baseState)).not.toContain("copy-branch"); + expect(allIds(baseState)).not.toContain("new-thread-on-branch"); + expect(allIds(baseState)).not.toContain("copy-branch"); }); it("flips lifecycle labels with thread state", () => { @@ -64,11 +70,12 @@ describe("buildThreadActionMenuItems", () => { const items = buildThreadActionMenuItems({ ...baseState, branch: "main" }); expect(items.at(-1)).toMatchObject({ id: "delete", destructive: true }); }); - it("offers archive as a non-destructive action right before delete", () => { const items = buildThreadActionMenuItems(baseState); const archiveItem = items.at(-2); expect(archiveItem?.id).toBe("archive"); + expect(archiveItem?.icon).toBe("archive"); + expect(archiveItem?.separatorBefore).toBe(true); expect(archiveItem?.destructive).toBeFalsy(); expect(items.at(-1)?.id).toBe("delete"); }); diff --git a/apps/web/src/components/threadActionMenu.logic.ts b/apps/web/src/components/threadActionMenu.logic.ts index 44c2e907ca554f2b21b1f1457f5c7b1d271fed53..df983ee86773f349e701ab9bb16f70f063c12274 100644 --- a/apps/web/src/components/threadActionMenu.logic.ts +++ b/apps/web/src/components/threadActionMenu.logic.ts @@ -18,6 +18,7 @@ export type ThreadActionMenuId = | "rename" | "regenerate-title" | "mark-unread" + | "copy" | "copy-path" | "copy-branch" | "copy-thread-id" @@ -56,14 +57,15 @@ export function buildThreadActionMenuItems( { id: "new-thread-on-branch" as const, label: `New thread on ${state.branch}`, + icon: "message-square-plus", }, ] : []), ...(state.supports.pinning ? [ state.isPinned - ? { id: "unpin" as const, label: "Unpin thread" } - : { id: "pin" as const, label: "Pin thread" }, + ? { id: "unpin" as const, label: "Unpin thread", icon: "pin-off" } + : { id: "pin" as const, label: "Pin thread", icon: "pin" }, ] : []), // Both lifecycle actions stay available on pinned threads: settling @@ -72,17 +74,18 @@ export function buildThreadActionMenuItems( ...(state.supports.settlement ? [ state.isSettled - ? { id: "unsettle" as const, label: "Un-settle thread" } - : { id: "settle" as const, label: "Settle thread" }, + ? { id: "unsettle" as const, label: "Un-settle thread", icon: "circle-check" } + : { id: "settle" as const, label: "Settle thread", icon: "circle-check" }, ] : []), ...(state.supports.snooze ? [ state.isSnoozed - ? { id: "unsnooze" as const, label: "Wake thread" } + ? { id: "unsnooze" as const, label: "Wake thread", icon: "clock" } : { id: "snooze" as const, label: "Snooze", + icon: "clock", disabled: !state.canSnoozeNow, children: state.snoozePresets.map((preset) => ({ id: `snooze:${preset.id}` as const, @@ -91,26 +94,48 @@ export function buildThreadActionMenuItems( }, ] : []), - { id: "rename", label: "Rename thread" }, + { id: "rename", label: "Rename thread", icon: "pencil", separatorBefore: true }, ...(state.supports.titleRegeneration ? [ { id: "regenerate-title" as const, label: state.isRegeneratingTitle ? "Regenerating…" : "Regenerate title", + icon: "refresh-cw", disabled: state.isRegeneratingTitle, }, ] : []), - { id: "mark-unread", label: "Mark unread" }, - { id: "copy-path", label: "Copy path", icon: "copy" }, - ...(state.branch ? [{ id: "copy-branch" as const, label: "Copy branch", icon: "copy" }] : []), - { id: "copy-thread-id", label: "Copy thread ID", icon: "copy" }, + { id: "mark-unread", label: "Mark unread", icon: "mail-open" }, + { + id: "copy", + label: "Copy", + icon: "copy", + separatorBefore: true, + children: [ + { id: "copy-path", label: "Path", icon: "folder" }, + ...(state.branch + ? [{ id: "copy-branch" as const, label: "Branch", icon: "git-branch" }] + : []), + { id: "copy-thread-id", label: "Thread ID", icon: "hash" }, + ], + }, // Archive removes the thread from the sidebar while keeping its // conversation under Settings > Archived threads — distinct from Settle // (stays visible in the Settled shelf) and Delete (clears history for // good), so it sits beside Delete without borrowing its destructive // styling. - { id: "archive", label: "Archive thread", disabled: state.isRunning }, - { id: "delete", label: "Delete", destructive: true, icon: "trash" }, + { + id: "archive", + label: "Archive thread", + icon: "archive", + disabled: state.isRunning, + separatorBefore: true, + }, + { + id: "delete", + label: "Delete", + destructive: true, + icon: "trash", + }, ]; } diff --git a/apps/web/src/contextMenuFallback.test.ts b/apps/web/src/contextMenuFallback.test.ts index d36f1a1d11b6abaaf5bbd1e7ecd2e2d27788122c..92efa723933bd61f6b58e1958bbf4803ce9ed344 100644 --- a/apps/web/src/contextMenuFallback.test.ts +++ b/apps/web/src/contextMenuFallback.test.ts @@ -24,14 +24,24 @@ class FakeElement { parent: FakeElement | null = null; style: Record & { cssText?: string } = {}; dataset: Record = {}; + attributes = new Map(); className = ""; disabled = false; + focused = false; type = ""; private textValue = ""; private readonly listeners = new Map(); constructor(readonly tagName: string) {} + get isConnected() { + let current: FakeElement | null = this; + while (current?.parent) { + current = current.parent; + } + return current?.tagName === "body"; + } + appendChild(child: FakeElement) { child.parent = this; this.children.push(child); @@ -55,6 +65,10 @@ class FakeElement { this.listeners.set(type, existing); } + setAttribute(name: string, value: string) { + this.attributes.set(name, value); + } + dispatchEvent(event: FakeDomEvent) { for (const listener of this.listeners.get(event.type) ?? []) { listener(event); @@ -62,6 +76,26 @@ class FakeElement { return true; } + focus() { + const fakeDocument = document as unknown as FakeDocument; + if (fakeDocument.activeElement === this) { + return; + } + fakeDocument.activeElement?.blur(); + fakeDocument.activeElement = this; + this.focused = true; + this.dispatchEvent(new FakeDomEvent("focus")); + } + + blur() { + const fakeDocument = document as unknown as FakeDocument; + if (fakeDocument.activeElement === this) { + fakeDocument.activeElement = null; + } + this.focused = false; + this.dispatchEvent(new FakeDomEvent("blur")); + } + set textContent(value: string) { this.textValue = value; } @@ -116,6 +150,7 @@ class FakeBody extends FakeElement { class FakeDocument { body = new FakeBody(); + activeElement: FakeElement | null = null; private readonly listeners = new Map(); createElement(tagName: string) { @@ -152,6 +187,7 @@ function findButton(label: string): FakeElement | undefined { beforeEach(() => { vi.stubGlobal("document", new FakeDocument()); + vi.stubGlobal("HTMLElement", FakeElement); vi.stubGlobal("window", { innerWidth: 1280, innerHeight: 800, @@ -183,6 +219,21 @@ afterEach(() => { }); describe("showContextMenuFallback", () => { + it("renders one separator between menu sections", async () => { + const selectionPromise = showContextMenuFallback([ + { id: "rename", label: "Rename" }, + { id: "archive", label: "Archive", separatorBefore: true }, + ]); + const separators = (document as unknown as FakeDocument) + .querySelectorAll("div") + .filter((element) => element.dataset.contextMenuSeparator === "true"); + + expect(separators).toHaveLength(1); + expect(separators[0]?.attributes.get("role")).toBe("separator"); + dismissContextMenu(); + await expect(selectionPromise).resolves.toBeNull(); + }); + it("resolves a clicked flat menu item", async () => { const selectionPromise = showContextMenuFallback([ { id: "rename", label: "Rename" }, @@ -235,6 +286,46 @@ describe("showContextMenuFallback", () => { await expect(selectionPromise).resolves.toBe("rename:project-b"); }); + + it("opens and focuses nested submenus when the parent is activated", async () => { + const invoker = (document as unknown as FakeDocument).createElement("button"); + (document as unknown as FakeDocument).body.appendChild(invoker); + invoker.focus(); + const selectionPromise = showContextMenuFallback([ + { + id: "copy:submenu", + label: "Copy", + children: [ + { id: "copy:path", label: "Path" }, + { id: "copy:branch", label: "Branch" }, + ], + }, + ]); + + const parentButton = findButton("Copy"); + expect(parentButton).toBeTruthy(); + expect(parentButton?.attributes.get("aria-haspopup")).toBe("menu"); + expect(parentButton?.attributes.get("aria-expanded")).toBe("false"); + parentButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(parentButton?.attributes.get("aria-expanded")).toBe("true"); + + const childButton = findButton("Path"); + const siblingButton = findButton("Branch"); + expect(childButton).toBeTruthy(); + expect(siblingButton).toBeTruthy(); + expect(childButton?.focused).toBe(true); + expect(childButton?.style.background).toBe("var(--accent)"); + expect(childButton?.style.color).toBe("var(--accent-foreground)"); + siblingButton?.dispatchEvent(new MouseEvent("mouseenter", { bubbles: true })); + expect(childButton?.focused).toBe(false); + expect(childButton?.style.background).toBe("transparent"); + expect(siblingButton?.focused).toBe(true); + expect(siblingButton?.style.background).toBe("var(--accent)"); + siblingButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + + await expect(selectionPromise).resolves.toBe("copy:branch"); + expect(invoker.focused).toBe(true); + }); }); describe("dismissContextMenu", () => { diff --git a/apps/web/src/contextMenuFallback.ts b/apps/web/src/contextMenuFallback.ts index 769826e3999c2113b16446bfa61309179d3b63d3..f2c7f42a0617ffeb18a605384e3a82ed4a389e0e 100644 --- a/apps/web/src/contextMenuFallback.ts +++ b/apps/web/src/contextMenuFallback.ts @@ -4,6 +4,20 @@ const SVG_NS = "http://www.w3.org/2000/svg"; // Inline Lucide-style icon paths (stroke-based, viewBox 0 0 24 24, strokeWidth 2). const ICON_PATHS: Record }>> = { + archive: [ + { tag: "rect", attrs: { width: "20", height: "5", x: "2", y: "3", rx: "1" } }, + { tag: "path", attrs: { d: "M4 8v11a2 2 0 0 0 2 2h12a2 2 0 0 0 2-2V8" } }, + { tag: "path", attrs: { d: "M10 12h4" } }, + ], + "chevron-right": [{ tag: "path", attrs: { d: "m9 19 7-7-7-7" } }], + "circle-check": [ + { tag: "circle", attrs: { cx: "12", cy: "12", r: "10" } }, + { tag: "path", attrs: { d: "m9 12 2 2 4-4" } }, + ], + clock: [ + { tag: "path", attrs: { d: "M12 6v6l4 2" } }, + { tag: "circle", attrs: { cx: "12", cy: "12", r: "10" } }, + ], pencil: [ { tag: "path", @@ -17,6 +31,71 @@ const ICON_PATHS: Record( position?: { x: number; y: number }, ): Promise { return new Promise((resolve) => { + const previouslyFocusedElement = + document.activeElement instanceof HTMLElement ? document.activeElement : null; const menuStack: HTMLDivElement[] = []; + const submenuTriggerStack: Array = []; let isDisposed = false; let canDismissFromPointer = false; @@ -142,9 +226,13 @@ export function showContextMenuFallback( document.removeEventListener("keydown", onKeyDown); document.removeEventListener("pointerdown", onPointerDown, true); document.removeEventListener("contextmenu", onContextMenu, true); + const shouldRestoreFocus = isNodeWithinMenuStack(document.activeElement, menuStack); for (const menu of menuStack) { menu.remove(); } + if (shouldRestoreFocus && previouslyFocusedElement?.isConnected) { + previouslyFocusedElement.focus({ preventScroll: true }); + } resolve(result); }; @@ -172,6 +260,7 @@ export function showContextMenuFallback( const closeMenusFromLevel = (level: number) => { while (menuStack.length > level) { + submenuTriggerStack.pop()?.setAttribute("aria-expanded", "false"); menuStack.pop()?.remove(); } }; @@ -181,6 +270,7 @@ export function showContextMenuFallback( preferredLeft: number, preferredTop: number, level: number, + parentTrigger?: HTMLButtonElement, ) => { closeMenusFromLevel(level); @@ -200,6 +290,15 @@ export function showContextMenuFallback( "max-height:min(24rem,70vh);min-width:0;max-width:24rem;overflow-x:hidden;overflow-y:auto;padding:0.25rem;"; for (const item of entries) { + if (item.separatorBefore === true && inner.children.length > 0) { + const separator = document.createElement("div"); + separator.className = "mx-2 my-1 h-px bg-border"; + separator.style.cssText = "height:1px;margin:0.25rem 0.5rem;background:var(--border);"; + separator.dataset.contextMenuSeparator = "true"; + separator.setAttribute("role", "separator"); + inner.appendChild(separator); + } + if (item.header === true) { const header = document.createElement("div"); header.className = "px-2 py-1.5 font-medium text-muted-foreground text-xs"; @@ -247,34 +346,63 @@ export function showContextMenuFallback( button.appendChild(label); if (hasChildren) { - const chevron = document.createElement("span"); - chevron.className = "ms-auto shrink-0 text-muted-foreground/80 text-sm leading-none"; - chevron.textContent = ">"; - button.appendChild(chevron); + button.setAttribute("aria-haspopup", "menu"); + button.setAttribute("aria-expanded", "false"); + const chevron = createIconElement("chevron-right", "neutral"); + if (chevron) { + chevron.setAttribute( + "class", + "-me-0.5 ms-auto size-4.5 shrink-0 text-muted-foreground opacity-80 sm:size-4", + ); + chevron.setAttribute("aria-hidden", "true"); + chevron.dataset.contextMenuChevron = "true"; + button.appendChild(chevron); + } } if (!isDisabled) { + let isHovered = false; + let isFocused = false; + const updateHighlight = () => { + const isHighlighted = isHovered || isFocused; + button.style.background = isHighlighted + ? isLeafDestructive + ? "color-mix(in srgb, var(--destructive) 10%, transparent)" + : "var(--accent)" + : "transparent"; + button.style.color = isHighlighted + ? isLeafDestructive + ? "var(--destructive-foreground)" + : "var(--accent-foreground)" + : isLeafDestructive + ? "var(--destructive-foreground)" + : "var(--foreground)"; + }; button.addEventListener("mouseenter", () => { - button.style.background = isLeafDestructive - ? "color-mix(in srgb, var(--destructive) 10%, transparent)" - : "var(--accent)"; - button.style.color = isLeafDestructive - ? "var(--destructive-foreground)" - : "var(--accent-foreground)"; + button.focus({ preventScroll: true }); + isHovered = true; + updateHighlight(); }); button.addEventListener("mouseleave", () => { - button.style.background = "transparent"; - button.style.color = isLeafDestructive - ? "var(--destructive-foreground)" - : "var(--foreground)"; + isHovered = false; + updateHighlight(); + }); + button.addEventListener("focus", () => { + isFocused = true; + updateHighlight(); + }); + button.addEventListener("blur", () => { + isFocused = false; + updateHighlight(); }); if (hasChildren) { - button.addEventListener("mouseenter", () => { + const openSubmenu = (focusFirstItem = false) => { const rect = button.getBoundingClientRect(); const nextLeft = rect.right + 4; const nextTop = rect.top; - openMenu(item.children!, nextLeft, nextTop, level + 1); + openMenu(item.children!, nextLeft, nextTop, level + 1, button); + button.setAttribute("aria-expanded", "true"); const childMenu = menuStack[level + 1]; if (!childMenu) { @@ -284,9 +412,18 @@ export function showContextMenuFallback( if (childRect.right > window.innerWidth) { clampMenuPosition(childMenu, rect.left - childRect.width - 4, rect.top); } + if (focusFirstItem) { + [...childMenu.querySelectorAll("button")] + .find((childButton) => !childButton.disabled) + ?.focus(); + } + }; + button.addEventListener("mouseenter", () => { + openSubmenu(); }); button.addEventListener("click", (event) => { event.preventDefault(); + openSubmenu(true); }); } else { button.addEventListener("mouseenter", () => { @@ -309,6 +446,7 @@ export function showContextMenuFallback( document.body.appendChild(menu); menuStack[level] = menu; + submenuTriggerStack[level] = parentTrigger; requestAnimationFrame(() => { clampMenuPosition(menu, preferredLeft, preferredTop); diff --git a/packages/contracts/src/ipc.ts b/packages/contracts/src/ipc.ts index 6a1f1c3209d507bfb0da81511bbcf547a81113a6..93f074f7be562fc352b792239a0a5a1ea99feb0c 100644 --- a/packages/contracts/src/ipc.ts +++ b/packages/contracts/src/ipc.ts @@ -111,6 +111,8 @@ export interface ContextMenuItem { header?: boolean; /** Icon keyword resolved by the web fallback. Stripped on desktop native menus. */ icon?: string; + /** Inserts a visual section divider immediately before this item. */ + separatorBefore?: boolean; children?: readonly ContextMenuItem[]; } @@ -121,6 +123,7 @@ export interface ContextMenuItemSchemaType { readonly disabled?: boolean; readonly header?: boolean; readonly icon?: string; + readonly separatorBefore?: boolean; readonly children?: readonly ContextMenuItemSchemaType[]; } @@ -131,6 +134,7 @@ export const ContextMenuItemSchema: Schema.Codec = Sc disabled: Schema.optionalKey(Schema.Boolean), header: Schema.optionalKey(Schema.Boolean), icon: Schema.optionalKey(Schema.String), + separatorBefore: Schema.optionalKey(Schema.Boolean), children: Schema.optionalKey( Schema.Array( Schema.suspend((): Schema.Codec => ContextMenuItemSchema), -- 2.54.0