feat(web): add explicit context-menu layout and focus options - #8458
Adamulek123 wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| cleanup(null, "interaction"); | ||
| return; | ||
| } | ||
| const action = contextMenuAcceleratorAction(items, event); |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:302
Pressing an accelerator attached to a header: true item returns the header's id and closes the fallback menu, even though headers are non-interactive and are stripped from native menus. contextMenuAcceleratorAction must skip header items before matching their accelerators.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around line 302:
Pressing an accelerator attached to a `header: true` item returns the header's `id` and closes the fallback menu, even though headers are non-interactive and are stripped from native menus. `contextMenuAcceleratorAction` must skip header items before matching their accelerators.
| if (event.key === "Escape") { | ||
| event.preventDefault(); | ||
| cleanup(null); | ||
| cleanup(null, "interaction"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:297
Pressing Escape closes the menu but also reaches GhosttyTerminalSurface.onKeyDown, so an interactive terminal command is cancelled. preventDefault() does not stop propagation; call stopPropagation() before cleanup, as for accelerator actions.
| if (event.key === "Escape") { | |
| event.preventDefault(); | |
| cleanup(null); | |
| cleanup(null, "interaction"); | |
| return; | |
| } | |
| if (event.key === "Escape") { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| cleanup(null, "interaction"); | |
| return; | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around lines 297-301:
Pressing Escape closes the menu but also reaches `GhosttyTerminalSurface.onKeyDown`, so an interactive terminal command is cancelled. `preventDefault()` does not stop propagation; call `stopPropagation()` before cleanup, as for accelerator actions.
| for (const item of items) { | ||
| if (item.disabled) continue; | ||
| if (item.children) { | ||
| const childAction = contextMenuAcceleratorAction(item.children, event); | ||
| if (childAction !== null) return childAction; | ||
| } | ||
| if (!item.accelerator) continue; | ||
|
|
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:206
When a parent item has children and an accelerator, pressing that accelerator resolves the menu with the parent id, even though the fallback only uses that item to open its submenu. After checking the children, skip the parent accelerator so only a matching leaf action is returned.
for (const item of items) {
- if (item.disabled) continue;
- if (item.children) {
+ if (item.disabled) continue;
+ if (item.children?.length) {
const childAction = contextMenuAcceleratorAction(item.children, event);
if (childAction !== null) return childAction;
+ continue;
}
if (!item.accelerator) continue;🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around lines 206-213:
When a parent item has `children` and an `accelerator`, pressing that accelerator resolves the menu with the parent `id`, even though the fallback only uses that item to open its submenu. After checking the children, skip the parent accelerator so only a matching leaf action is returned.
| cleanup(null, "interaction"); | ||
| return; | ||
| } | ||
| const action = contextMenuAcceleratorAction(items, event); |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:302
CommandOrControl+C and CmdOrCtrl+C never trigger an action in the fallback menu, so items using Electron’s documented cross-platform accelerator work natively but not in the browser menu. contextMenuAcceleratorAction only recognizes ctrl, alt, command, and cmd, causing both documented tokens to match neither ctrlKey nor metaKey; normalize or reject these accelerator strings before invoking the matcher.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around line 302:
`CommandOrControl+C` and `CmdOrCtrl+C` never trigger an action in the fallback menu, so items using Electron’s documented cross-platform accelerator work natively but not in the browser menu. `contextMenuAcceleratorAction` only recognizes `ctrl`, `alt`, `command`, and `cmd`, causing both documented tokens to match neither `ctrlKey` nor `metaKey`; normalize or reject these accelerator strings before invoking the matcher.
There was a problem hiding this comment.
Reviewed the changed web UI surfaces (apps/web/src/contextMenuFallback.ts, apps/web/src/localApi.ts) against the shared component system and token ownership.
Good: the new kbd hint reuses the exact MenuShortcut treatment from components/ui/menu.tsx (ms-auto font-medium font-sans text-secondary-label text-xs tracking-widest) and reads the runtime-adjusted --contrast-secondary-label role in the imperative style string, and dropping the inline min-height/font-size overrides makes the default rows finally match the MenuItem geometry (min-h-8 text-base sm:min-h-7 sm:text-sm).
Two findings on the new accelerator handling, where the fallback diverges from the accelerator grammar it shares with the Electron host and from the app's shared shortcut-label formatting.
Posted via Macroscope — UI Consistency
| function formatContextMenuAccelerator(accelerator: string): string { | ||
| const parts = accelerator.split("+"); | ||
| if (!parts.some((part) => part === "Command" || part === "Cmd")) return accelerator; | ||
| const key = parts.at(-1) ?? ""; | ||
| return `${parts.includes("Ctrl") ? "⌃" : ""}${parts.includes("Alt") ? "⌥" : ""}${ | ||
| parts.includes("Shift") ? "⇧" : "" | ||
| }⌘${key}`; | ||
| } |
There was a problem hiding this comment.
The hint notation is chosen by sniffing the accelerator string for Command/Cmd rather than from the platform, so presentation is inconsistent with the rest of the UI: on macOS an item declared "Ctrl+Shift+C" renders the literal text Ctrl+Shift+C, while Kbd/MenuShortcut consumers, the command palette and keybindings settings all render macOS hints as ⌃⇧C via formatShortcutLabel in keybindings.ts. The new test asserting ["Ctrl+Shift+C", "⇧⌘V"] shows two notations mixed inside one popup. The last part is also passed through unnormalized, so "Command+Shift+v" renders ⇧⌘v where the shared formatter would produce ⇧⌘V (and named keys like Escape/ArrowUp stay raw).
Suggest formatting through the shared keybindings label logic (platform from isMacPlatform(navigator.platform), symbol order ⌃⌥⇧⌘, formatShortcutKeyLabel-style key casing) instead of a local variant, so fallback menu hints match every other shortcut hint in the app.
Posted via Macroscope — UI Consistency
| const parts = item.accelerator.toLowerCase().split("+"); | ||
| if ( | ||
| event.key.toLowerCase() === parts.at(-1) && | ||
| event.altKey === parts.includes("alt") && | ||
| event.ctrlKey === parts.includes("ctrl") && | ||
| event.metaKey === (parts.includes("command") || parts.includes("cmd")) && | ||
| event.shiftKey === parts.includes("shift") | ||
| ) { |
There was a problem hiding this comment.
The modifier vocabulary here is narrower than the grammar this same accelerator string is handed to. ElectronMenu.ts now forwards item.accelerator straight to Electron's native menu, whose accelerator tokens include CommandOrControl/CmdOrCtrl, Control, Option and Super. This matcher only understands ctrl, alt, shift, command/cmd, so the canonical cross-platform form ("CmdOrCtrl+Shift+C") can never match in the styled/web host: metaKey === parts.includes("command") and ctrlKey === parts.includes("ctrl") are both false on every platform. The item then shows a shortcut hint that silently does nothing on web, while working on desktop.
Suggest normalizing modifier tokens once (control→ctrl, option→alt, cmdorctrl/commandorcontrol→meta on macOS else ctrl, super→meta) and sharing that helper with formatContextMenuAccelerator, so both hosts accept the same notation.
Posted via Macroscope — UI Consistency
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit efd2e94. Configure here.
| const onWheel = (event: WheelEvent) => { | ||
| if (!isNodeWithinMenuStack(event.target, menuStack)) { | ||
| cleanup(null, "interaction"); | ||
| } |
There was a problem hiding this comment.
Wheel dismiss races opening gesture
Medium Severity
The new wheel listener closes the fallback menu immediately, without the canDismissFromPointer delay used by pointerdown and contextmenu. Trackpad momentum or a two-finger right-click that also emits a small wheel delta can dismiss the menu in the same frame it opens, including for existing callers that never opted into the new layout or lifecycle options.
Reviewed by Cursor Bugbot for commit efd2e94. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a cross-host context-menu feature that adds browser keyboard interception, focus and cancellation lifecycle management, wheel dismissal, compact/styled presentation, and native accelerator wiring across shared production infrastructure. The breadth of runtime behavior and unresolved shortcut/dismissal concerns make human review appropriate. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. We are not adding explicit layout and focus options to the web context menu. The existing actions remain available, and these extra preferences are not a priority. If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |
2 participants
What Changed
Added backward-compatible context-menu options for compact layout, styled presentation, focus restoration, and owner cancellation. Menu accelerators now render and execute consistently in the browser fallback and Electron host.
Why
Callers with short action menus need a denser styled menu and deterministic cleanup, but those choices should stay in generic menu infrastructure. Existing callers keep the current default layout and native desktop behavior.
UI Changes
None for existing callers. This PR adds opt-in behavior and keeps every default unchanged.
Validation
no-this-aliaswarning in the fallback test harnessgit diff --checkChecklist
Implemented with GPT-5.6 Codex in T3 Code.
Note
Medium Risk
Capture-phase keyboard handling and focus restoration change input behavior while menus are open; styled fallback on desktop bypasses native menus when opted in.
Overview
Extends context menus with optional
acceleratoron items (contracts + IPC schema) and threads it through Electron native menus and the browser DOM fallback, where shortcuts render as<kbd>hints (including macOS symbol formatting) and run via capture-phasekeydownso they win over focused terminals.showContextMenuFallbackandLocalApi.contextMenu.showgain opt-inlayout: "compact",presentation: "styled"(use DOM menu even when a desktop bridge exists),restoreFocus(on-dismissvs action selection), andAbortSignalcancellation. Dismissal also reacts to wheel outside the menu stack;contextMenu.closealways tears down the styled fallback.Existing callers keep default layout and native desktop presentation unless they pass the new options.
Reviewed by Cursor Bugbot for commit efd2e94. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add accelerator, layout, and focus options to context menus
ContextMenuItemwith an optionalacceleratorstring andLocalApi.contextMenu.showwith anoptionsobject (layout,presentation,restoreFocus,signal).showContextMenuFallback) now renders accelerator hints, matches keyboard events to menu items, supports a compact layout, dismisses on scroll, and manages lifecycle viaAbortSignalwith configurable focus restoration.acceleratorthrough to native menu items.createBrowserLocalApi.contextMenu.showforwards supported options to the fallback and can force the styled fallback on desktop withpresentation: 'styled';closenow always callsdismissContextMenu.dismissContextMenuis now called even when a desktop bridge exists; callers passingrestoreFocusshould verify focus handling matches expectations, particularly inapps/web/src/localApi.ts.📊 Macroscope summarized efd2e94. 4 files reviewed, 5 issues evaluated, 0 issues filtered, 4 comments posted
🗂️ Filtered Issues
Before and after
Before:
before_terminal.mp4
After:
after_terminal.mp4