fix(desktop): forward unregistered ctrl/meta chords to v1 terminal PTY#3390
Conversation
v1's keyboard handler returned false for every ctrl/meta combo, starving TUIs like Claude Code of Ctrl+P/Ctrl+O and shell readline shortcuts like Ctrl+R/W/U. Mirror v2's pattern (terminal-runtime.ts:21): use resolveHotkeyFromEvent to only bubble chords actually registered as app hotkeys, letting everything else reach the PTY. Fixes #3385.
|
Caution Review failedPull request was closed or merged during review 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)
📝 WalkthroughWalkthroughUpdated hotkey handling in Terminal helpers to resolve keyboard shortcuts correctly. Changed the Changes
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 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 fixes a bug in the v1 terminal where all
Confidence Score: 5/5Safe to merge — minimal, well-reasoned one-line fix that exactly mirrors the proven v2 terminal pattern. The change is correct and complete. All existing safeguards (reserved events, CLEAR_TERMINAL intercept, cursor-motion overrides) execute before the new check and are unaffected. The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent fires in v1 terminal] --> B{Cursor-motion override?\nCmd+←/→, Opt+←/→, Ctrl+←/→ Win}
B -- yes --> C[Write escape seq to PTY\nreturn false]
B -- no --> D{isTerminalReservedEvent?\nCtrl+C/D/Z/S/Q/backslash}
D -- yes --> E[return true → PTY receives event]
D -- no --> F{CLEAR_TERMINAL binding match?}
F -- yes --> G[call onClear\nreturn false]
F -- no --> H{resolveHotkeyFromEvent ≠ null?\nRegistered app hotkey}
H -- yes --> I[return false → bubbles to\nreact-hotkeys-hook]
H -- no --> J[return true → PTY receives event\nCtrl+R, Ctrl+W, Ctrl+U, etc.]
Reviews (1): Last reviewed commit: "fix(desktop): forward unregistered ctrl/..." | Re-trigger Greptile |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…v1 terminal superset-sh#3390 changed the v1 terminal keyboard handler so any chord not registered as an app hotkey falls through to the PTY. That regressed standard OS clipboard shortcuts: Cmd+C / Cmd+V / Cmd+X / Cmd+A on macOS and the Linux/Windows equivalents (Ctrl+Shift+C/V/X/A, Shift+Insert) are not registered as app hotkeys, so they now reach xterm and never get a chance to copy/paste via Electron's edit menu — text selection in the terminal became impossible to copy. Add an explicit short-circuit before the "fall through to PTY" branch: when the chord is exactly an OS clipboard combination, return false so the event bubbles to the host. xterm.js' selection / Electron's edit role still get to do the work; SIGINT (Ctrl+C in PTY) and other terminal-reserved chords are unaffected because they're handled earlier in the same handler. Reproduced on Superset Desktop v1.5.1 (macOS, Apple Silicon, Turkish keyboard): Cmd+C in terminal pane silently dropped before this patch, copies selection after.
superset-sh#3390) v1's keyboard handler returned false for every ctrl/meta combo, starving TUIs like Claude Code of Ctrl+P/Ctrl+O and shell readline shortcuts like Ctrl+R/W/U. Mirror v2's pattern (terminal-runtime.ts:21): use resolveHotkeyFromEvent to only bubble chords actually registered as app hotkeys, letting everything else reach the PTY. Fixes superset-sh#3385.
1 participant
Summary
terminal-runtime.ts:21: useresolveHotkeyFromEventto only bubble chords actually registered as app hotkeys; everything else reaches the PTY.helpers.ts:597.Before / after
Before:
if (event.metaKey || event.ctrlKey) return false;— any modifier chord was blocked from xterm.After:
if (resolveHotkeyFromEvent(event) !== null) return false;— only registered app hotkeys bubble.Registered app hotkeys (FIND_IN_TERMINAL, SCROLL_TO_BOTTOM, CLEAR_TERMINAL, QUICK_OPEN, …) still work. Reserved chords (Ctrl+C/D/Z/S/Q) still go to xterm as before.
Test plan
(reverse-i-search)appearsclaudein a v1 terminal → Ctrl+P (model switcher) and Ctrl+O (extended output) workSummary by cubic
Forward unregistered Ctrl/Meta chords to the v1 terminal PTY so TUIs and shell shortcuts work again. Only registered app hotkeys bubble to the app; everything else reaches xterm.
Written for commit d5f06ce. Summary will update on new commits.
Summary by CodeRabbit
Release Notes