fix(desktop): keep OS clipboard chords (Cmd+C/V/X/A) bubbling out of v1 terminal#3407
fix(desktop): keep OS clipboard chords (Cmd+C/V/X/A) bubbling out of v1 terminal#3407AytuncYildizli wants to merge 2 commits into
Conversation
…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.
|
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)
📝 WalkthroughWalkthroughAdded platform-aware clipboard shortcut passthrough in the terminal keyboard handler so standard copy/paste/cut/select-all chords are allowed to bubble to the host before app-hotkey resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Keyboard
participant Handler as setupKeyboardHandler
participant ClipboardCheck as Clipboard Hotkey Check
participant AppHotkeys as App Hotkey Resolution
participant Terminal as Terminal/PTY
participant Host as Host (OS/Electron)
User->>Handler: press key combo (e.g., Cmd+C)
Handler->>ClipboardCheck: evaluate clipboard shortcut (platform-aware)
alt Clipboard match
ClipboardCheck->>Handler: match
Handler->>Host: return false (allow passthrough)
Host->>Host: perform system clipboard action
else No match
ClipboardCheck->>Handler: no match
Handler->>AppHotkeys: resolve app hotkey
alt App hotkey handled
AppHotkeys->>AppHotkeys: handle shortcut
else Unbound
AppHotkeys->>Terminal: deliver to PTY
Terminal->>Terminal: process input
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 regression from #3390 where OS clipboard chords ( Key changes:
Issues found:
Confidence Score: 3/5Safe to merge for macOS users, but non-Mac clipboard handling has two concrete bugs that leave the fix incomplete on Linux/Windows. The macOS path (Cmd+C/V/X/A) is correctly implemented and solves the primary regression. However, the non-Mac path has two P1 issues: Shift+Insert paste is documented as fixed but the outer guard never matches it, and Ctrl+Delete is incorrectly bubbled — it is a terminal delete-forward-word shortcut causing a new PTY editing regression on Linux/Windows. apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts — specifically the non-Mac clipboard chord block (lines 611–628) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent in xterm handler] --> B{isShiftEnter?}
B -- yes --> C[handle / return false]
B -- no --> D{isCmdBackspace/Left/Right or Option/Ctrl nav?}
D -- yes --> E[handle nav / return false]
D -- no --> F{isTerminalReservedEvent? Ctrl+C/D/Z/S/Q}
F -- yes --> G[return true to PTY]
F -- no --> H{matches CLEAR_TERMINAL?}
H -- yes --> I[call onClear / return false]
H -- no --> J{isMac and onlyMeta and C/V/X/A?}
J -- yes --> K[return false OS clipboard bubbles]
J -- no --> L{not isMac and onlyCtrl or ctrlShiftOnly?}
L -- Ctrl+Shift+C/V/X/A --> M[return false OS clipboard bubbles]
L -- onlyCtrl+Insert --> N[return false Ctrl+Insert Copy]
L -- onlyCtrl+Delete --> O[BUG: return false breaks delete-fwd-word]
L -- Shift+Insert shiftOnly --> P[outer guard never entered falls to PTY]
L -- no match --> Q{resolveHotkeyFromEvent not null?}
Q -- registered hotkey --> R[return false bubble]
Q -- unregistered --> S[return true PTY]
Reviews (1): Last reviewed commit: "fix(desktop): keep OS clipboard chords (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onlyCtrl && | ||
| (event.code === "Insert" || event.code === "Delete") | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Ctrl+Delete is a terminal editing shortcut, not a clipboard chord
Ctrl+Delete in terminal contexts sends a "delete forward word" escape sequence (e.g. \e[3;5~ in many shells/readline configs). Making it bubble (return false) prevents that sequence from reaching the PTY, regressing forward-word-delete for non-Mac users.
The standard CUA clipboard shortcuts that warrant bubbling are:
- Copy:
Ctrl+Insert✓ - Paste:
Shift+Insert(currently missing, see sibling comment) - Cut:
Shift+Delete(separateshiftOnlycase)
Ctrl+Delete is not part of that set and should be removed from this condition:
| onlyCtrl && | |
| (event.code === "Insert" || event.code === "Delete") | |
| ) { | |
| return false; | |
| } | |
| if ( | |
| onlyCtrl && | |
| event.code === "Insert" | |
| ) { | |
| return false; | |
| } |
| if (!isMac && (onlyCtrl || ctrlShiftOnly)) { | ||
| // Linux/Windows: Ctrl+Shift+C/V/X/A or Shift+Insert / Ctrl+Insert | ||
| if ( | ||
| ctrlShiftOnly && | ||
| (event.code === "KeyC" || | ||
| event.code === "KeyV" || | ||
| event.code === "KeyX" || | ||
| event.code === "KeyA") | ||
| ) { | ||
| return false; | ||
| } | ||
| if ( | ||
| onlyCtrl && | ||
| (event.code === "Insert" || event.code === "Delete") | ||
| ) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shift+Insert mentioned in comment but never enters the bubble block
The inline comment on line 612 and the PR description both list Shift+Insert (the Linux/Windows paste shortcut) as a case that should bubble. However, the outer guard if (!isMac && (onlyCtrl || ctrlShiftOnly)) requires ctrlKey to be set. For Shift+Insert, only shiftKey is pressed with no ctrlKey, so neither onlyCtrl nor ctrlShiftOnly is ever true — the block is never entered and Shift+Insert still falls through to the PTY path unchanged.
A shiftOnly variable (analogous to onlyCtrl) should be added to the outer condition, and Shift+Insert should be handled inside:
const shiftOnly =
event.shiftKey && !event.ctrlKey && !event.metaKey && !event.altKey;
if (!isMac && (onlyCtrl || ctrlShiftOnly || shiftOnly)) {
// Ctrl+Shift+C/V/X/A
if (ctrlShiftOnly && (event.code === "KeyC" || event.code === "KeyV" ||
event.code === "KeyX" || event.code === "KeyA")) {
return false;
}
// Ctrl+Insert = Copy
if (onlyCtrl && event.code === "Insert") { return false; }
// Shift+Insert = Paste
if (shiftOnly && event.code === "Insert") { return false; }
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Around line 611-627: The current modifier checks use onlyCtrl for
Insert/Delete which incorrectly prevents Shift+Insert from being handled and
allows Ctrl+Delete to bubble; update the condition so Insert/Delete are gated by
the Shift-only modifier instead of Ctrl (replace the onlyCtrl check with the
shift-only variable used in this keyboard handler, e.g., shiftOnly or onlyShift)
so the code becomes: if (shiftOnly && (event.code === "Insert" || event.code ===
"Delete")) return false; thereby mapping Shift+Insert/Shift+Delete to the
terminal clipboard/cut behavior and preventing Ctrl+Delete from being swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49b3a373-2400-4d92-bd71-d62f5c1e6e03
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
| if (!isMac && (onlyCtrl || ctrlShiftOnly)) { | ||
| // Linux/Windows: Ctrl+Shift+C/V/X/A or Shift+Insert / Ctrl+Insert | ||
| if ( | ||
| ctrlShiftOnly && | ||
| (event.code === "KeyC" || | ||
| event.code === "KeyV" || | ||
| event.code === "KeyX" || | ||
| event.code === "KeyA") | ||
| ) { | ||
| return false; | ||
| } | ||
| if ( | ||
| onlyCtrl && | ||
| (event.code === "Insert" || event.code === "Delete") | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix the non-mac Insert/Delete modifier mapping.
Line 611 excludes Shift+Insert, so that documented paste chord still reaches the PTY. Lines 622-625 also bubble Ctrl+Delete, which is a common terminal editing chord rather than a clipboard shortcut. If you want the Delete-based cut path too, that modifier should be Shift, not Ctrl.
🔧 Proposed fix
const onlyMeta =
event.metaKey && !event.ctrlKey && !event.altKey && !event.shiftKey;
const onlyCtrl =
event.ctrlKey && !event.metaKey && !event.altKey && !event.shiftKey;
+ const onlyShift =
+ event.shiftKey && !event.metaKey && !event.ctrlKey && !event.altKey;
const ctrlShiftOnly =
event.ctrlKey && event.shiftKey && !event.metaKey && !event.altKey;
@@
- if (!isMac && (onlyCtrl || ctrlShiftOnly)) {
+ if (!isMac && (onlyCtrl || onlyShift || ctrlShiftOnly)) {
// Linux/Windows: Ctrl+Shift+C/V/X/A or Shift+Insert / Ctrl+Insert
if (
ctrlShiftOnly &&
(event.code === "KeyC" ||
event.code === "KeyV" ||
event.code === "KeyX" ||
event.code === "KeyA")
) {
return false;
}
- if (
- onlyCtrl &&
- (event.code === "Insert" || event.code === "Delete")
- ) {
+ if (onlyCtrl && event.code === "Insert") {
+ return false;
+ }
+ if (onlyShift && event.code === "Insert") {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isMac && (onlyCtrl || ctrlShiftOnly)) { | |
| // Linux/Windows: Ctrl+Shift+C/V/X/A or Shift+Insert / Ctrl+Insert | |
| if ( | |
| ctrlShiftOnly && | |
| (event.code === "KeyC" || | |
| event.code === "KeyV" || | |
| event.code === "KeyX" || | |
| event.code === "KeyA") | |
| ) { | |
| return false; | |
| } | |
| if ( | |
| onlyCtrl && | |
| (event.code === "Insert" || event.code === "Delete") | |
| ) { | |
| return false; | |
| } | |
| if (!isMac && (onlyCtrl || onlyShift || ctrlShiftOnly)) { | |
| // Linux/Windows: Ctrl+Shift+C/V/X/A or Shift+Insert / Ctrl+Insert | |
| if ( | |
| ctrlShiftOnly && | |
| (event.code === "KeyC" || | |
| event.code === "KeyV" || | |
| event.code === "KeyX" || | |
| event.code === "KeyA") | |
| ) { | |
| return false; | |
| } | |
| if (onlyCtrl && event.code === "Insert") { | |
| return false; | |
| } | |
| if (onlyShift && event.code === "Insert") { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`
around lines 611 - 627, The current modifier checks use onlyCtrl for
Insert/Delete which incorrectly prevents Shift+Insert from being handled and
allows Ctrl+Delete to bubble; update the condition so Insert/Delete are gated by
the Shift-only modifier instead of Ctrl (replace the onlyCtrl check with the
shift-only variable used in this keyboard handler, e.g., shiftOnly or onlyShift)
so the code becomes: if (shiftOnly && (event.code === "Insert" || event.code ===
"Delete")) return false; thereby mapping Shift+Insert/Shift+Delete to the
terminal clipboard/cut behavior and preventing Ctrl+Delete from being swallowed.
Review feedback on superset-sh#3407 (Greptile P1 + CodeRabbit major): - Ctrl+Delete is a shell readline chord (delete-word-forward), not a clipboard shortcut. Removed it from the bubble path so it reaches the PTY again. - Shift+Insert is the documented Linux paste chord but the previous `onlyCtrl` guard excluded shift, so it never matched. Added an explicit Insert + (onlyCtrl || onlyShift) path that covers Ctrl+Insert (copy) and Shift+Insert (paste). Shift+Delete is intentionally excluded — terminals usually consume it as a readline edit, not as cut.
|
thanks @greptile-apps and @coderabbitai — both findings were valid. fixed in 9a2b3fa:
|
|
Tip For best results, initiate chat on the files or code changes.
The clipboard bubble path looks solid now. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
The fix looks correct. The relevant block now reads: if (event.code === "Insert" && (onlyCtrl || onlyShift)) {
return false;
}All three points check out:
The Tip: You can customize Greptile's behavior for this repo with |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts:611">
P2: `Shift+Insert` (standard Linux/Windows paste shortcut) is documented in the comment on line 612 and the PR description as a chord that should bubble, but the outer guard `if (!isMac && (onlyCtrl || ctrlShiftOnly))` requires `ctrlKey` to be set. For `Shift+Insert`, only `shiftKey` is pressed, so neither `onlyCtrl` nor `ctrlShiftOnly` is true — the block is never entered and `Shift+Insert` still falls through to the PTY. Add a `shiftOnly` modifier variable and include it in the outer condition.</violation>
<violation number="2" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts:624">
P2: `Ctrl+Delete` is a terminal editing shortcut (forward-word-delete, typically sends `\e[3;5~`), not a clipboard chord. Bubbling it out of the terminal handler breaks forward-word-delete for Linux/Windows users. The standard CUA clipboard shortcuts using these keys are `Ctrl+Insert` (copy) and `Shift+Insert` (paste) — `Ctrl+Delete` is not in that set. Remove `event.code === "Delete"` from this condition.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Follow-up note: PR #3415 now exists to handle the broader terminal shortcut cases around this same area. It tightens the platform guard so Linux fallback chords do not leak onto macOS, and aligns the v1 terminal behavior more closely with VS Code's terminal clipboard handling, including macOS Link: #3415 |
|
Thank you for reporting a fixing! |
2 participants
Summary / What changed
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts: short-circuit OS clipboard chords (Cmd+C/V/X/A on macOS, Ctrl+Shift+C/V/X/A and Shift+Insert / Ctrl+Insert on other platforms) so they bubble out of the terminal handler instead of falling through to the PTY.Why
#3390 made the v1 terminal forward any chord not registered as an app hotkey to the PTY. That fixed Claude Code TUI shortcuts but regressed standard OS clipboard combos:
Cmd+C,Cmd+V,Cmd+X,Cmd+Aare not registered as app hotkeys, so they now go straight to xterm and never reach Electron's edit menu / xterm.js Selection. Result: text selected in the terminal pane can't be copied; clipboard paste also doesn't reach the input line.The patch adds an explicit allow-bubble step before the "fall through to PTY" branch. Terminal-reserved chords (Ctrl+C SIGINT, Ctrl+D EOF, etc.) are still handled earlier in the same function, so PTY semantics are unchanged.
Tests
No unit tests touched — the change is platform-conditional event routing inside the live keyboard handler. Manual repro was decisive enough.
Real-world validation
Cmd+C, nothing happens;Cmd+Vdoes nothing. After: selection copies to the system clipboard; paste reaches the PTY input line as expected.Ctrl+Cin PTY still sends SIGINT (terminal-reserved path runs first).Issue
Regression of #3390. No standalone issue yet — happy to file one if maintainers prefer that flow.
Summary by cubic
Fixes the v1 terminal swallowing OS clipboard shortcuts so they bubble to the host, restoring copy, paste, cut, and select-all in terminal panes. PTY-reserved chords remain unchanged.
Cmd+C/V/X/A; Linux/WindowsCtrl+Shift+C/V/X/A,Ctrl+Insert(copy),Shift+Insert(paste). Adds explicitInserthandling and excludesCtrl+Delete/Shift+Deleteso readline edits reach the PTY.Written for commit 9a2b3fa. Summary will update on new commits.
Summary by CodeRabbit