fix(web): make terminal selection actions reliable - #6555
Adamulek123 wants to merge 13 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:
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 |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR substantially changes terminal menu behavior and shared context-menu lifecycle handling across browser and desktop environments, including new shortcut dispatch, styled menus, cancellation, focus, and clipboard coordination. The cross-cutting runtime impact is broader than a small isolated bug fix and merits human review. You can add or adjust custom eligibility rules. Learn more. |
2cff9ae to
3c6c912
Compare
…menu # Conflicts: # apps/desktop/src/electron/ElectronMenu.test.ts # apps/web/src/components/ThreadTerminalDrawer.tsx # apps/web/src/contextMenuFallback.ts # apps/web/src/terminal/ghostty/surface.ts
There was a problem hiding this comment.
UI Consistency — findings
The imperative fallback menu in apps/web/src/contextMenuFallback.ts has so far mirrored the React menu primitives (MenuPopup, MenuItem, MenuSeparator, MenuGroupLabel) class-for-class. This PR adds a second, hand-tuned menu design inside the same function, keyed implicitly off whether any item has an accelerator, plus a new shortcut chip treatment that doesn't match MenuShortcut/Kbd. Comments inline. One dead handler in ThreadTerminalDrawer.tsx is also flagged.
Posted via Macroscope — UI Consistency
…menu # Conflicts: # apps/web/src/contextMenuFallback.ts
There was a problem hiding this comment.
UI Consistency
The previous round's findings are addressed: the compact layout is now an explicit layout: "compact" option instead of being inferred from accelerators, the shortcut hint reuses the MenuShortcut class contract, compact rows keep a responsive type step plus opacity-64 for disabled, and handleContextMenu is wired into the surface's onContextMenu.
Three remaining items on the changed lines:
apps/web/src/contextMenuFallback.ts— the accelerator's imperative style readsvar(--secondary-label)instead of the appearance-contrast-adjustedvar(--contrast-secondary-label), so the shortcut text splits away from the adjusted colors used everywhere else in this menu.apps/web/src/contextMenuFallback.ts—inner.dataset.compacthas no consumer and emitsdata-compact="false"on every non-compact menu.apps/web/src/terminal/ghostty/surface.ts—preventDefaultownership moved from the host into the surface, but theonContextMenuoption's doc comment still assigns that ownership to the host.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the new shortcut <kbd> reads the unadjusted --secondary-label role in its inline style, so it bypasses the contrast bridge that every other imperative color in this file goes through.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One interaction finding on the terminal context menu focus contract; everything flagged in earlier runs (contrast token for the shortcut hint, data-compact, the onContextMenu doc comment, the unused handleContextMenu, MenuShortcut class parity) looks resolved.
Posted via Macroscope — UI Consistency
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 475ca8e. Configure here.
|
This PR became too broad while covering several independent terminal-menu issues. It has been split into:
Each behavior now has a focused diff and its tests beside it. #8459 will be rebased onto main and marked ready after #8458 lands. |

What Changed
Terminal text could be selected but then become effectively stuck: Copy was disabled in the desktop fallback menu,
Ctrl+Shift+CandCtrl+Shift+Vdid nothing while the initial selection popup was open, Paste did not work, and Add to chat disappeared after dismissing the first popup. Scrolling the terminal could leave the popup inconsistent with its selection.This makes the initial selection popup and the reopened right-click menu use the same terminal-aware actions. Both expose Add to chat, Copy, and Paste; show the platform shortcut (
Ctrl+Shift+C/Von Windows and Linux,⌘C/Von macOS); execute those shortcuts while open; dismiss on outside clicks or terminal scrolling; and can be reopened without losing the selection.Why
Electron's generic edit menu reads Chromium edit flags, but the terminal renders and owns selection through its canvas-backed Ghostty surface. Those edit flags therefore disabled valid terminal actions and could not paste into the PTY. A shared in-app terminal menu keeps selection, clipboard handling, shortcut behavior, and lifecycle cancellation under the terminal's control while still routing through the local host API.
The menu is aborted when its terminal closes or is replaced, and stale requests cannot act on a removed terminal. Clipboard and chat-context eligibility are tracked separately so newline-only selections remain copyable without creating empty chat context.
UI Changes
Before
before_terminal.mp4
After
after_terminal.mp4
Validation
git diff --checkpassedChecklist
Generated with gpt-5.6-sol in the T3 Code Codex harness.
Note
Medium Risk
Changes terminal clipboard/menu lifecycle and extends
LocalApi.contextMenu.show; optional contract fields are backward compatible, but styled menus on desktop and abort-driven focus behavior need careful regression testing.Overview
Unifies the post-selection popup and right-click terminal menu so both show the same actions (Add to chat, Copy, Paste), platform shortcuts, and go through a shared
runTerminalMenuRequestpath withAbortControllercancellation, superseded-request guards, and focus restore only after copy/paste.Context menu platform changes:
ContextMenuItemgains optionalaccelerator, wired into Electron native menus and the web fallback (shortcut labels,contextMenuAcceleratorAction, compact layout).localApi.contextMenu.showacceptspresentation: "styled"(forces the DOM menu on desktop),signal, andrestoreFocus;closealways dismisses the fallback. The fallback also closes on outside scroll and owner abort.Terminal behavior: Selection eligibility splits
canCopyvscanAddToChat(e.g. newline-only selections stay copyable). Ghosttypaste()centralizes paste and bumpspasteShortcutTokenso menu paste races keyboard clipboard reads; the surface alwayspreventDefaulton context menu before the host handles it.Reviewed by Cursor Bugbot for commit 1c7827c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Make terminal context menu actions reliable with accelerators, abort support, and focus restoration
runTerminalMenuRequestin ThreadTerminalDrawer.tsx to orchestrate menu open/action/focus with abort signals and stale-request guards; only current requests perform actions or report errorsshowContextMenuFallbackin contextMenuFallback.ts to supportAbortSignal, accelerator key activation, outside-scroll dismissal, compact layout, and configurable focus restoration ('on-dismiss')GhosttyTerminalSurface.onContextMenuin surface.ts to always callevent.preventDefault()itself; centralizes paste through a newpaste()methodLocalApi.contextMenu.shownow routes to the styled web fallback whenpresentation: 'styled'is requested, even on desktop; otherwise delegates to native menuTerminalViewportno longer callsevent.preventDefault()on context menu — the surface now owns this.contextMenu.closealways dismisses the styled fallback regardless of desktop bridge presence.Macroscope summarized 1c7827c.