fix(web,desktop): unify themed context menus with icons, warning tones, and text actions - #7998
fix(web,desktop): unify themed context menus with icons, warning tones, and text actions#7998bberka wants to merge 2 commits into
Conversation
…ones, and text editing actions
|
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:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| target.type, | ||
| ); | ||
| const isTextAreaElement = target instanceof HTMLTextAreaElement; | ||
| const isContentEditable = target instanceof HTMLElement && target.isContentEditable; |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:585
Select all selects only the clicked child element instead of the full contenteditable editing host when the editor contains nested elements. Because event.target is often a formatted child whose isContentEditable is inherited, range.selectNodeContents(target) scopes the selection to that fragment; resolve the nearest editing host and use it for the contenteditable selection.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around line 585:
`Select all` selects only the clicked child element instead of the full `contenteditable` editing host when the editor contains nested elements. Because `event.target` is often a formatted child whose `isContentEditable` is inherited, `range.selectNodeContents(target)` scopes the selection to that fragment; resolve the nearest editing host and use it for the contenteditable selection.
| if (window.desktopBridge) { | ||
| return window.desktopBridge.showContextMenu(items, position) as Promise<T | null>; | ||
| } | ||
| return showContextMenuFallback(items, position); |
There was a problem hiding this comment.
🟡 Medium src/localApi.ts:39
Electron contextMenu.show now uses showContextMenuFallback, whose keyboard handling only dismisses on Escape and does not support navigation or activation, so desktop users cannot select menu actions by keyboard. Route Electron through window.desktopBridge.showContextMenu and keep the fallback for non-Electron browsers.
| return showContextMenuFallback(items, position); | |
| if (window.desktopBridge) { | |
| return window.desktopBridge.showContextMenu(items, position) as Promise<T | null>; | |
| } | |
| return showContextMenuFallback(items, position); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/localApi.ts around line 39:
Electron `contextMenu.show` now uses `showContextMenuFallback`, whose keyboard handling only dismisses on `Escape` and does not support navigation or activation, so desktop users cannot select menu actions by keyboard. Route Electron through `window.desktopBridge.showContextMenu` and keep the fallback for non-Electron browsers.
| const isInputElement = | ||
| target instanceof HTMLInputElement && | ||
| !["hidden", "file", "checkbox", "radio", "button", "submit", "reset", "image"].includes( | ||
| target.type, | ||
| ); |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:579
installGlobalTextContextMenu disables Cut and Copy for selected values in inputs such as type="email", and Paste inserts at the end instead of at the caret or selection. The handler classifies every non-excluded input as editable even though selectionStart/selectionEnd are unavailable for those types, so restrict custom handling to selection-capable text input types and let the native menu handle the others.
- const isInputElement =
- target instanceof HTMLInputElement &&
- !["hidden", "file", "checkbox", "radio", "button", "submit", "reset", "image"].includes(
- target.type,
- );
+ const isInputElement =
+ target instanceof HTMLInputElement &&
+ ["text", "search", "url", "tel", "password"].includes(target.type);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/contextMenuFallback.ts around lines 579-583:
`installGlobalTextContextMenu` disables Cut and Copy for selected values in inputs such as `type="email"`, and Paste inserts at the end instead of at the caret or selection. The handler classifies every non-excluded input as editable even though `selectionStart`/`selectionEnd` are unavailable for those types, so restrict custom handling to selection-capable text input types and let the native menu handle the others.
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca23ff9. Configure here.
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); |
There was a problem hiding this comment.
Spellcheck menus never appear
High Severity
installGlobalTextContextMenu always calls preventDefault on editable right-clicks. In Electron that suppresses the main-process webContents context-menu event, so the native spellcheck / Copy Link / Copy Image path in DesktopWindow never runs for those targets—even though this PR kept that handler for those exclusive actions.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ca23ff9. Configure here.
| document.execCommand("copy"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Copy and cut lose selection
High Severity
The global text menu keeps a live window.getSelection() and runs Cut/Copy only after showContextMenuFallback resolves. Menu rows call focus on mouseenter, which collapses that selection, so Copy of highlighted page/composer text and execCommand("cut") on contenteditable usually no-op after the click.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ca23ff9. Configure here.
There was a problem hiding this comment.
UI consistency review of the themed context-menu changes. Four findings, all in apps/web/src/contextMenuFallback.ts: one interaction/Electron regression (the global handler cancels the gesture the desktop native menu still depends on), one CSS ownership issue (inline glass/shadow overriding the shared dropdown-glass utility, with no dark-mode shadow), and two smaller tone-resolution issues.
Posted via Macroscope — UI Consistency
| if (item.destructive === true || item.id === "delete") { | ||
| return "destructive"; | ||
| } | ||
| if (item.tone === "warning" || item.id === "archive") { | ||
| return "warning"; | ||
| } | ||
| if (item.tone === "destructive") { | ||
| return "destructive"; | ||
| } | ||
| return "neutral"; |
There was a problem hiding this comment.
The item.id === "archive" heuristic is evaluated before the explicit tone, so an item that opts into another tone (or any menu that reuses the id archive for a neutral action) is silently re-toned amber. Now that tone is part of the ContextMenuItem contract and every archive call site in this PR sets it, prefer reading the explicit field only.
| if (item.destructive === true || item.id === "delete") { | |
| return "destructive"; | |
| } | |
| if (item.tone === "warning" || item.id === "archive") { | |
| return "warning"; | |
| } | |
| if (item.tone === "destructive") { | |
| return "destructive"; | |
| } | |
| return "neutral"; | |
| if (item.destructive === true || item.tone === "destructive" || item.id === "delete") { | |
| return "destructive"; | |
| } | |
| if (item.tone === "warning") { | |
| return "warning"; | |
| } | |
| return "neutral"; |
Posted via Macroscope — UI Consistency
| // Only show our menu if right-clicking an editable element OR there is text selected. | ||
| if (!isEditable && !hasSelection) { | ||
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); |
There was a problem hiding this comment.
This cancels the contextmenu event for every editable target, which also cancels Chromium's context-menu request to the browser process — so window.webContents.on("context-menu", …) in DesktopWindow.ts never fires for editable text. That is exactly where the desktop side of this PR keeps its "Electron-exclusive" items (spellcheck suggestions, and Copy Link / Copy Image when the click lands on editable content), so those become unreachable in the desktop app; in the other direction, if the main-process handler does still run, the user gets the native menu and the themed menu from one right-click.
Suggest making one owner per gesture: either let the desktop bridge forward its native-only entries (suggestions / Copy Link / Copy Image) into the themed menu before it is shown, or skip interception when those native-only entries apply and keep the native path there. Worth a focused test for the Electron branch either way.
Posted via Macroscope — UI Consistency
|
|
||
| if (typeof item.icon === "string") { | ||
| const icon = createIconElement(item.icon, isLeafDestructive ? "destructive" : "neutral"); | ||
| const icon = createIconElement(item.icon, itemTone); |
There was a problem hiding this comment.
Disabled rows mute the label (--contrast-muted-foreground at opacity 0.64) but the icon is still built with the item's tone, so it keeps its full-strength tone color. With the new warning tone this is now reachable: Archive (n) is disabled while a thread is running, rendering an amber icon next to a muted label. Suggest muting the icon with the row.
| const icon = createIconElement(item.icon, itemTone); | |
| const icon = createIconElement(item.icon, isDisabled ? "neutral" : itemTone); |
Posted via Macroscope — UI Consistency
| menu.style.cssText = | ||
| "position:fixed;z-index:10000;min-width:8rem;max-width:24rem;overflow:hidden;border-radius:var(--radius-lg);background-clip:padding-box;color:var(--contrast-popover-foreground);outline:none;pointer-events:auto;"; | ||
| "position:fixed;z-index:10000;min-width:8rem;max-width:24rem;overflow:hidden;border-radius:var(--radius-lg);background-clip:padding-box;color:var(--contrast-popover-foreground);background:color-mix(in srgb, var(--popover) 18%, color-mix(in srgb, var(--popover) var(--glass-opacity,80%), transparent));-webkit-backdrop-filter:blur(var(--glass-blur,12px)) saturate(var(--glass-saturation,1.14));backdrop-filter:blur(var(--glass-blur,12px)) saturate(var(--glass-saturation,1.14));border:1px solid color-mix(in srgb, var(--contrast-foreground) 10%, transparent);box-shadow:0 16px 40px -18px rgb(0 0 0 / 55%);outline:none;pointer-events:auto;"; |
There was a problem hiding this comment.
The element already carries the dropdown-glass utility, and inline styles win over it, so these declarations take glass ownership away from the shared utility: the utility's @supports not (backdrop-filter) and any future dark/border tweaks no longer govern this surface, and the hardcoded --glass-* fallbacks duplicate values the utility already resolves. The box-shadow is also light-only — every other dropdown surface (ui/menu.tsx, ui/select.tsx, ui/combobox.tsx, ComposerCommandMenu) pairs 0 16px 40px -18px rgb(0 0 0/55%) with dark:shadow-[0_18px_44px_-18px_rgb(0_0_0/80%)], which an inline style cannot express, so this menu keeps the light shadow in dark mode.
Suggest leaving glass composition to dropdown-glass and expressing the shadow as classes (if the utility genuinely was not applying here, a comment saying why would help):
menu.className =
- "dropdown-glass fixed z-[10000] min-w-32 max-w-sm overflow-hidden rounded-lg bg-clip-padding text-popover-foreground outline-none";
+ "dropdown-glass fixed z-[10000] min-w-32 max-w-sm overflow-hidden rounded-lg bg-clip-padding text-popover-foreground shadow-[0_16px_40px_-18px_rgb(0_0_0/55%)] outline-none dark:shadow-[0_18px_44px_-18px_rgb(0_0_0/80%)]";
menu.style.cssText =
- "position:fixed;z-index:10000;min-width:8rem;max-width:24rem;overflow:hidden;border-radius:var(--radius-lg);background-clip:padding-box;color:var(--contrast-popover-foreground);background:color-mix(in srgb, var(--popover) 18%, color-mix(in srgb, var(--popover) var(--glass-opacity,80%), transparent));-webkit-backdrop-filter:blur(var(--glass-blur,12px)) saturate(var(--glass-saturation,1.14));backdrop-filter:blur(var(--glass-blur,12px)) saturate(var(--glass-saturation,1.14));border:1px solid color-mix(in srgb, var(--contrast-foreground) 10%, transparent);box-shadow:0 16px 40px -18px rgb(0 0 0 / 55%);outline:none;pointer-events:auto;";
+ "position:fixed;z-index:10000;min-width:8rem;max-width:24rem;overflow:hidden;border-radius:var(--radius-lg);background-clip:padding-box;color:var(--contrast-popover-foreground);outline:none;pointer-events:auto;";Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. Not approved because:
|
1 participant
What Changed
tone?: "neutral" | "destructive" | "warning"toContextMenuItem/ Schema contracts. Applied amber warning tone (var(--warning-foreground)) to Archive actions and preserved red destructive tone for Delete.archive,mail-open,refresh-cw,scissors,clipboard,check-check,circle-check,clock,pencil,pin,copy, etc.) across sidebar actions, thread action menus, and file browser entries.Why
UI Changes
Checklist
Note
Medium Risk
Touches Electron native context-menu suppression and clipboard cut/copy/paste on inputs. Behavior is UI-only but can regress right-click editing and desktop-only actions if intercepts fire incorrectly.
Overview
Desktop context menus now use the same themed glass fallback as the web client instead of the native OS popup, so icons, headers, and colors survive on Electron.
Archive gets an amber
warningtone (contracttonefield) so it is distinct from red Delete. Thread, bulk, and file-browser items gain Lucide-style icons.A global handler on inputs, textareas, contenteditable, and selected text shows styled Cut / Copy / Paste / Select All. Electron still pops a native menu only for spellcheck, Copy Link, and Copy Image, and always
preventDefaults so the web menu owns standard editing. Empty-space right-clicks stay unhandled.Reviewed by Cursor Bugbot for commit ca23ff9. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Unify themed context menus with icons, warning tones, and web-side text actions
installGlobalTextContextMenuin AppRoot.tsx; desktop now only shows Electron-native items like spellcheck and Copy Imagetone(neutral|destructive|warning) to context menu items across sidebar, thread actions, and file browser, surfaced through a newresolveItemToneutil and inline-styled icon rendering in contextMenuFallback.ts\n- Adds optionaltonetoContextMenuItemin ipc.ts so items carry tone hints through IPCcreateBrowserLocalApinow always usesshowContextMenuFallbackforshow()and always callsdismissContextMenuforclose(), regardless of desktop bridge presencenavigator.clipboardwithdocument.execCommandfallback in contextMenuFallback.ts📊 Macroscope summarized ca23ff9. 9 files reviewed, 3 issues evaluated, 0 issues filtered, 3 comments posted
🗂️ Filtered Issues