fix(desktop): adopt Ghostty keyboard model in v2 terminal#3700
Merged
Conversation
v2's terminal runtime only filtered app hotkeys. With kitty keyboard protocol enabled (needed for Shift+Enter disambiguation in claude-code, modifier reporting in neovim/helix), every Mac Cmd chord xterm saw got CSI-u encoded and leaked into TUIs as a literal char — and line-edit niceties like Cmd+Left/Right/Backspace and Option+Left/Right that v1 handles never worked at all. Mirror Ghostty's approach (src/input/key_encode.zig:534-545: "on macOS, command+keys do not encode text"): bubble every Mac Cmd chord out to the host before xterm's kitty encoder runs, then port v1's line-edit chord translators so shell navigation works the same in both renderers. Changes: - Broaden shouldBubbleClipboardShortcut's Mac branch to bubble all Cmd chords (not just Cmd+C/V with selection gating). v1 benefits too. - Port v1's line-edit translators into v2's custom key handler: Cmd+Left/Right/Backspace, Option+Left/Right, Windows Ctrl+Left/Right. Duplicates v1 for now; a follow-up can share the handler properly. - Wire shouldSelectAllShortcut into v2 so Cmd+A selects terminal buffer. - Use xterm.input(data, true) to inject translated sequences into the PTY (fires onData, forwarded by terminal-ws-transport). - Restructure tests around the new Mac rule.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the prior xterm key handler with a factory-produced handler attached to the terminal. The new handler short-circuits host hotkeys, translates platform line-edit chords into escape bytes and injects them, handles select-all, and conditionally bubbles clipboard shortcuts per platform/selection. Changes
Sequence DiagramsequenceDiagram
participant User as User Input
participant Handler as Custom Key Handler
participant HotKey as Host Hotkey Resolver
participant Translate as Line-Edit Translator
participant SelectAll as Select-All Checker
participant Clipboard as Clipboard-Bubble Checker
participant Terminal as Terminal/PTY
participant Host as Host App
User->>Handler: keydown
Handler->>HotKey: resolveHotkeyFromEvent(event)
alt Host hotkey detected
HotKey-->>Handler: hotkey
Handler->>Terminal: preventDefault (suppress xterm processing)
Handler->>Host: dispatch host hotkey action
else Not a host hotkey
HotKey-->>Handler: none
Handler->>Translate: match line-edit chord?
alt Line-edit matched
Translate-->>Handler: escape bytes
Handler->>Terminal: terminal.input(bytes, true) + preventDefault
else Not line-edit
Handler->>SelectAll: shouldSelectAllShortcut?
alt Select-all matched
SelectAll-->>Handler: true
Handler->>Terminal: terminal.selectAll() + preventDefault
else Not select-all
Handler->>Clipboard: shouldBubbleClipboardShortcut?
alt Should bubble
Clipboard-->>Handler: true
Handler->>Host: allow event to bubble (no preventDefault)
else Keep in terminal
Clipboard-->>Handler: false
Handler->>Terminal: preventDefault (consume)
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR adopts Ghostty's keyboard model in the v2 terminal: all Mac Cmd chords are bubbled to the host before xterm's kitty encoder runs, preventing CSI-u sequences from leaking into TUIs as literal characters (e.g. Cmd+V → Confidence Score: 5/5Safe to merge; all remaining findings are minor style/future-proofing suggestions with no correctness impact. The handler ordering (hotkey → line-edit translation → select-all → bubble → pass-through) is correct, escape sequences are hardcoded constants with no injection risk, the Linux Ctrl+Shift+C behavioural change is intentional and tested, and the test suite covers the new Mac rule comprehensively. Only P2 items remain. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Replaces the minimal isAppHotkey guard with a full createKeyEventHandler that translates Mac/Windows line-edit chords and bubbles all Mac Cmd chords (Ghostty pattern) before xterm's kitty encoder runs. Logic order (resolve hotkey → translate → selectAll → bubble → pass through) is correct. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.ts | Mac branch simplified to return event.metaKey (all Cmd chords bubble). Windows/Linux paths refactored slightly; Linux Ctrl+Shift+C now bubbles without requiring a selection — intentional behavioural change matching the new test assertions. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.test.ts | Tests restructured into three clear groups (Mac all-Cmd, Mac non-Cmd, Windows/Linux standard bindings). Coverage is thorough and correctly validates the new Ghostty-style rule and the unchanged platform-specific paths. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent fires] --> B{resolveHotkeyFromEvent?}
B -- yes --> C[return false\nbubble to react-hotkeys-hook]
B -- no --> D{translateLineEditChord?}
D -- match --> E[keydown? → terminal.input seq\nreturn false]
D -- no match --> F{shouldSelectAllShortcut\nCmd+A on Mac?}
F -- yes --> G[keydown? → terminal.selectAll\nreturn false]
F -- no --> H{shouldBubbleClipboardShortcut?}
H -- Mac: any metaKey\nWin/Linux: specific binds --> I[return false\nbubble to Electron menu / OS]
H -- no --> J[return true\nxterm handles normally]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 32-35
Comment:
**`navigator.platform` is deprecated**
`navigator.platform` was deprecated in the Living Standard and may be removed in future browser/Electron versions. The recommended replacement is `navigator.userAgentData?.platform` (available in Chromium-based Electron builds), with a graceful fallback.
```suggestion
const platform =
typeof navigator !== "undefined"
? (navigator.userAgentData?.platform ?? navigator.platform).toLowerCase()
: "";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 40-47
Comment:
**`translateLineEditChord` suppresses `keyup` for matched chords**
When a chord (e.g. Cmd+Left) is matched, the handler returns `false` for every event type, including `keyup`. Only `keydown` sends `terminal.input`, but xterm never sees the `keyup` for that chord. In practice xterm's custom key handler is primarily meaningful for `keydown`, so this is unlikely to cause problems, but a comment clarifying the intent would improve readability.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): adopt Ghostty keyboard mod..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+32
to
+35
| const platform = | ||
| typeof navigator !== "undefined" ? navigator.platform.toLowerCase() : ""; | ||
| const isMac = platform.includes("mac"); | ||
| const isWindows = platform.includes("win"); |
There was a problem hiding this comment.
navigator.platform is deprecated
navigator.platform was deprecated in the Living Standard and may be removed in future browser/Electron versions. The recommended replacement is navigator.userAgentData?.platform (available in Chromium-based Electron builds), with a graceful fallback.
Suggested change
| const platform = | |
| typeof navigator !== "undefined" ? navigator.platform.toLowerCase() : ""; | |
| const isMac = platform.includes("mac"); | |
| const isWindows = platform.includes("win"); | |
| const platform = | |
| typeof navigator !== "undefined" | |
| ? (navigator.userAgentData?.platform ?? navigator.platform).toLowerCase() | |
| : ""; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 32-35
Comment:
**`navigator.platform` is deprecated**
`navigator.platform` was deprecated in the Living Standard and may be removed in future browser/Electron versions. The recommended replacement is `navigator.userAgentData?.platform` (available in Chromium-based Electron builds), with a graceful fallback.
```suggestion
const platform =
typeof navigator !== "undefined"
? (navigator.userAgentData?.platform ?? navigator.platform).toLowerCase()
: "";
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+40
to
+47
| const translation = translateLineEditChord(event, { isMac, isWindows }); | ||
| if (translation !== null) { | ||
| if (event.type === "keydown") { | ||
| event.preventDefault(); | ||
| terminal.input(translation, true); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
translateLineEditChord suppresses keyup for matched chords
When a chord (e.g. Cmd+Left) is matched, the handler returns false for every event type, including keyup. Only keydown sends terminal.input, but xterm never sees the keyup for that chord. In practice xterm's custom key handler is primarily meaningful for keydown, so this is unlikely to cause problems, but a comment clarifying the intent would improve readability.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 40-47
Comment:
**`translateLineEditChord` suppresses `keyup` for matched chords**
When a chord (e.g. Cmd+Left) is matched, the handler returns `false` for every event type, including `keyup`. Only `keydown` sends `terminal.input`, but xterm never sees the `keyup` for that chord. In practice xterm's custom key handler is primarily meaningful for `keydown`, so this is unlikely to cause problems, but a comment clarifying the intent would improve readability.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)
80-164: Optional: collapse the repeated modifier checks into a small lookup table.Seven near-identical blocks differ only in
(platform, key, required-modifier, output). A table-driven form keeps the exact-modifier-match invariant in one helper and makes it easier to add chords later (e.g., Option+Backspace →\x1b\x7f) without another copy-paste.♻️ Proposed refactor sketch
-function translateLineEditChord( - event: KeyboardEvent, - options: { isMac: boolean; isWindows: boolean }, -): string | null { - const { isMac, isWindows } = options; - - if ( - isMac && - event.key === "Backspace" && - event.metaKey && - !event.ctrlKey && - !event.altKey && - !event.shiftKey - ) { - return "\x15\x1b[D"; - } - // ... six more near-identical blocks - return null; -} +type Mod = "meta" | "alt" | "ctrl"; +type ChordRule = { + platform: "mac" | "win"; + key: string; + mod: Mod; + seq: string; +}; + +const LINE_EDIT_CHORDS: readonly ChordRule[] = [ + { platform: "mac", key: "Backspace", mod: "meta", seq: "\x15\x1b[D" }, + { platform: "mac", key: "ArrowLeft", mod: "meta", seq: "\x01" }, + { platform: "mac", key: "ArrowRight", mod: "meta", seq: "\x05" }, + { platform: "mac", key: "ArrowLeft", mod: "alt", seq: "\x1bb" }, + { platform: "mac", key: "ArrowRight", mod: "alt", seq: "\x1bf" }, + { platform: "win", key: "ArrowLeft", mod: "ctrl", seq: "\x1bb" }, + { platform: "win", key: "ArrowRight", mod: "ctrl", seq: "\x1bf" }, +]; + +function hasOnlyModifier(event: KeyboardEvent, mod: Mod): boolean { + return ( + event.metaKey === (mod === "meta") && + event.altKey === (mod === "alt") && + event.ctrlKey === (mod === "ctrl") && + !event.shiftKey + ); +} + +function translateLineEditChord( + event: KeyboardEvent, + options: { isMac: boolean; isWindows: boolean }, +): string | null { + for (const rule of LINE_EDIT_CHORDS) { + const platformMatches = + (rule.platform === "mac" && options.isMac) || + (rule.platform === "win" && options.isWindows); + if ( + platformMatches && + event.key === rule.key && + hasOnlyModifier(event, rule.mod) + ) { + return rule.seq; + } + } + return null; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts` around lines 80 - 164, Refactor translateLineEditChord to replace the seven repeated if-blocks with a table-driven approach: define a mapping array of entries (referencing translateLineEditChord) where each entry specifies platform predicate (e.g., isMac/isWindows), key, exact modifier requirements (meta, ctrl, alt, shift) and the output string, and implement a small helper (e.g., matchExactModifiers(event, requiredModifiers)) that returns true only when each modifier exactly matches the required boolean; iterate the mapping and return the first matching entry's output or null. Ensure the helper checks exact equality for all four modifiers so behavior remains identical to the current checks and include existing outputs like "\x15\x1b[D", "\x01", "\x05", "\x1bb", "\x1bf".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 80-164: Refactor translateLineEditChord to replace the seven
repeated if-blocks with a table-driven approach: define a mapping array of
entries (referencing translateLineEditChord) where each entry specifies platform
predicate (e.g., isMac/isWindows), key, exact modifier requirements (meta, ctrl,
alt, shift) and the output string, and implement a small helper (e.g.,
matchExactModifiers(event, requiredModifiers)) that returns true only when each
modifier exactly matches the required boolean; iterate the mapping and return
the first matching entry's output or null. Ensure the helper checks exact
equality for all four modifiers so behavior remains identical to the current
checks and include existing outputs like "\x15\x1b[D", "\x01", "\x05", "\x1bb",
"\x1bf".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51216c8a-eec6-4ff8-8415-ffe9bcc2b0f8
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Matches VS Code (terminalInstance.ts:1116-1175) and Tabby (xtermFrontend.ts:199-214): call `event.preventDefault()` before returning false from the custom key handler so the browser's default action can't double-fire alongside the Electron `role: 'paste'`/`'copy'` accelerator registered in src/main/lib/menu.ts. Paste still flows — webContents.paste() dispatches the paste event on the focused textarea independently of the DOM keydown default, and xterm's paste listener picks it up. Keydown-only — preventDefault on keyup is a no-op and would suppress kitty protocol release events we still want xterm to handle.
Kitenite
force-pushed
the
speckled-jodhpur
branch
from
bf8ea08 to
36cf276
Compare
Seven near-identical 8-line blocks collapsed to a platform+modifier grouped structure with a tiny onlyMod helper. Same behavior, ~55 fewer lines, adding a new chord is one line instead of ten.
Broke Cmd+C/V. On Electron macOS, the browser's keydown → paste-command pipeline is what dispatches the paste event on xterm's textarea, and preventDefault blocks that pipeline. VS Code and Tabby can preventDefault because they implement paste themselves (command system / ClipboardAddon); we rely on xterm's built-in paste listener, so the default must run. Revert of 36cf276.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
11 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 24, 2026
sync: merge upstream/main through superset-sh#3700 (ours) to reduce behind count
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v2 terminal runtime only filtered app hotkeys, so with kitty keyboard protocol enabled (required for Shift+Enter disambiguation in claude-code, modifier reporting in neovim/helix), every Mac Cmd chord xterm saw got CSI-u encoded and leaked into TUIs as a literal char — Cmd+V →
v, Cmd+C →c. Line-edit niceties that v1 handles (Cmd+←/→/⌫, Option+←/→) never worked in v2 at all.Mirrors Ghostty's approach (
src/input/key_encode.zig:534-545: "on macOS, command+keys do not encode text"): bubble every Mac Cmd chord out to the host before xterm's kitty encoder runs, then port v1's line-edit chord translators so shell navigation is identical in both renderers.shouldBubbleClipboardShortcut's Mac branch: bubble all Cmd chords (not just Cmd+C/V with selection gating). v1 benefits too.shouldSelectAllShortcutinto v2 so Cmd+A selects the terminal buffer.xterm.input(data, true)to inject translated sequences into the PTY (firesonData, forwarded by terminal-ws-transport).This is a cherry-pick of 70bd191 from the debug branch — the fix is complementary to the TERM_PROGRAM=kitty change that already landed (TERM_PROGRAM=kitty lets TUIs parse CSI-u; this stops us from sending CSI-u for chords the host should own).
Related upstream fix against codex: https://github.com/openai/codex/pull/new/fix/cmd-key-text-insertion-on-macos — widens codex's "is this key text?" gate to treat SUPER as a shortcut modifier. Not needed once Cmd chords are bubbled at the xterm layer, but defends against other kitty-speaking terminals that don't.
Test plan
bun testforclipboardShortcuts.test.ts— 4 pass, 22 assertions.bun typecheckfor@superset/desktop— clean.vleak).cleak when nothing selected).Summary by cubic
Adopts Ghostty’s keyboard model in the v2 terminal so macOS Cmd chords never become CSI‑u input and v1 shell navigation matches. Restores reliable copy/paste, Select All, and line‑edit shortcuts across platforms.
Bug Fixes
preventDefaulton bubbled clipboard chords so the browser paste pipeline runs.xterm.input(..., true)so shells respond correctly.Refactors
onlyModhelper; behavior unchanged and easier to extend.Written for commit cd821a5. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests