[codex] Port v2 terminal hotkeys to v1#3724
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes refactor terminal keyboard handling by extracting logic into dedicated modules ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal
participant useTerminalHotkeys
participant terminalKeyboardHandler
participant xterm
User->>terminalKeyboardHandler: Keyboard Event
terminalKeyboardHandler->>terminalKeyboardHandler: Check resolveHotkeyFromEvent()
alt App Hotkey Registered
terminalKeyboardHandler-->>User: Return false (bubble hotkey)
else Shift+Enter
terminalKeyboardHandler->>terminalKeyboardHandler: preventDefault()
terminalKeyboardHandler->>useTerminalHotkeys: onShiftEnter()
terminalKeyboardHandler-->>User: Return false
else Line-Edit Chord
terminalKeyboardHandler->>terminalKeyboardHandler: translateLineEditChord()
terminalKeyboardHandler->>useTerminalHotkeys: onWrite(translation)
terminalKeyboardHandler-->>User: Return false
else Select All / Clipboard
terminalKeyboardHandler->>xterm: selectAll() or handle clipboard
terminalKeyboardHandler-->>User: Return false
else Other Keys
terminalKeyboardHandler-->>xterm: Return true (xterm handles)
end
alt CLEAR_TERMINAL Hotkey Triggered
useTerminalHotkeys->>Terminal: onClear()
Terminal->>xterm: clear()
Terminal->>Terminal: clearScrollbackRef(paneId)
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 ports the v2 terminal hotkey model to v1 by moving Confidence Score: 5/5Safe to merge; refactor is behaviorally correct and all changed paths are covered by new regression tests. No P0/P1 issues found. The key ordering change (resolveHotkeyFromEvent first) intentionally matches v2 and is explicitly tested. CLEAR_TERMINAL routing through useHotkey is clean. The only remaining items are pre-existing style asymmetries (e.g., isMac not guarding isCmdLeft/Right/Backspace) that are unchanged by this PR. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts | New module extracted from helpers.ts; key change is resolveHotkeyFromEvent checked first so registered app hotkeys always bubble before line-edit translations. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.test.ts | New test file; covers hotkey bubbling priority, override precedence, line-edit fallback, and Cmd+A select-all; all scenarios look correct. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalHotkeys.ts | CLEAR_TERMINAL wired through useHotkey hook with onClear callback; clean addition that correctly takes over from the removed handler-level check. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx | handleClearHotkey useCallback correctly accesses xtermRef.current at call time and is passed as onClear; no issues. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts | setupKeyboardHandler and KeyboardHandlerOptions removed; now imported from terminalKeyboardHandler; no leftover dead code. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | onClear removed from setupKeyboardHandler call; import updated to new module; handleClear still registered via registerClearCallbackRef for programmatic clearing. |
Sequence Diagram
sequenceDiagram
participant User
participant xterm
participant KeyHandler as terminalKeyboardHandler
participant Document
participant useTerminalHotkeys
User->>xterm: Keypress (e.g. CLEAR_TERMINAL chord)
xterm->>KeyHandler: attachCustomKeyEventHandler(event)
KeyHandler->>KeyHandler: resolveHotkeyFromEvent(event) !== null?
alt Registered app hotkey (e.g. CLEAR_TERMINAL)
KeyHandler-->>xterm: return false (bubble)
xterm-->>Document: Event propagates to document
Document->>useTerminalHotkeys: useHotkey("CLEAR_TERMINAL") fires
useTerminalHotkeys->>xterm: xterm.clear()
useTerminalHotkeys->>useTerminalHotkeys: clearScrollbackRef({ paneId })
else Line-edit translation (e.g. Cmd+Left, unregistered)
KeyHandler->>KeyHandler: Apply translation (Ctrl+A, Ctrl+E...)
KeyHandler-->>xterm: return false (handled in-process)
else Terminal-reserved chord (Ctrl+C/D/Z..., unregistered)
KeyHandler-->>xterm: return true (goes to PTY)
end
Reviews (1): Last reviewed commit: "port v2 terminal hotkeys to v1" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts (1)
183-187: Dead branch:isTerminalReservedEventcheck is unreachable in effect.Both arms return
true, so the conditional has no observable effect today. Either drop it or add a side-effect/log to make the intent meaningful. If kept as documentation-by-code, a brief comment plus a singlereturnwould be clearer.♻️ Suggested simplification
- // Unregistered terminal-reserved chords (ctrl+c/d/z/s/q) stay with xterm. - if (isTerminalReservedEvent(event)) return true; - - return true; + // Default: let xterm process the event (covers terminal-reserved chords + // like ctrl+c/d/z/s/q and any other unhandled keys). + return true;🤖 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/terminalKeyboardHandler.ts` around lines 183 - 187, The conditional that checks isTerminalReservedEvent(event) is dead because both branches return true; remove the redundant if and replace it with a single return true, keeping or expanding the inline comment that explains terminal-reserved chords should be handled by xterm (so the intent remains clear); update the handler function (the closure returned by terminalKeyboardHandler) to eliminate the unreachable branch and leave a concise comment referencing terminal-reserved chords.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.test.ts (2)
68-76: Optional: also restorenavigatorinafterEachfor cross-suite isolation.
originalOverridesis restored, butnavigatoris left mutated toMacIntel. Cheap to add a snapshot/restore for symmetry, in case future tests in the same worker depend on the real platform.♻️ Suggested restore
describe("setupKeyboardHandler", () => { let originalOverrides: Record<string, string | null>; + let originalNavigator: PropertyDescriptor | undefined; beforeEach(() => { + originalNavigator = Object.getOwnPropertyDescriptor(globalThis, "navigator"); setPlatform("MacIntel"); originalOverrides = useHotkeyOverridesStore.getState().overrides; useHotkeyOverridesStore.setState({ overrides: {} }); }); afterEach(() => { useHotkeyOverridesStore.setState({ overrides: originalOverrides }); + if (originalNavigator) { + Object.defineProperty(globalThis, "navigator", originalNavigator); + } });🤖 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/terminalKeyboardHandler.test.ts` around lines 68 - 76, The test mutates navigator via setPlatform("MacIntel") in beforeEach but never restores it; update the afterEach to also restore navigator to its original value by capturing the original navigator/platform before calling setPlatform (e.g., save current navigator or its platform value) and then call setPlatform with that saved value in afterEach so setPlatform and navigator state are symmetric with the useHotkeyOverridesStore restore; reference setPlatform, beforeEach, and afterEach in your change.
65-150: Consider adding coverage for Option+Arrow / Ctrl+Arrow word navigation.The PR’s primary regression surface is line-edit translation routing, but the Option+Left/Right (macOS) and Ctrl+Left/Right (Windows) branches in
terminalKeyboardHandler.tsare uncovered here. Adding one or two cases would lock in their preventDefault/onWrite("\x1bb"|"\x1bf")behavior and catch any future drift (see related comment on the handler file).🤖 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/terminalKeyboardHandler.test.ts` around lines 65 - 150, Add tests to cover Option+Arrow (macOS altKey) and Ctrl+Arrow (Windows ctrlKey) branches in the keyboard handler: in the existing describe("setupKeyboardHandler") block add cases that call setupKeyboardHandler(terminal.xterm, { onWrite }) and then synthesize ArrowLeft/ArrowRight key events with altKey=true (Option) and ctrlKey=true (Ctrl) via makeKeyEvent; assert terminal.invoke returns false, key.defaultPrevented is true, and the collected writes equal ["\x1bb"] for left and ["\x1bf"] for right. Reference the helper/test utilities makeXterm, makeKeyEvent, setupKeyboardHandler and the onWrite capture used in other tests to locate where to add these new assertions.
🤖 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/terminalKeyboardHandler.ts`:
- Around line 100-161: The Option/Ctrl word-navigation branches (the
isOptionLeft, isOptionRight, isCtrlLeft, isCtrlRight checks) are missing
event.preventDefault() before calling options.onWrite, causing leaked
browser/navigation behavior; update each branch so when event.type === "keydown"
and options.onWrite is called you first call event.preventDefault() and then
options.onWrite("\x1bb") or options.onWrite("\x1bf") as appropriate (keep the
same escape codes and return false behavior), matching how the Cmd branches
handle preventDefault.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.test.ts`:
- Around line 68-76: The test mutates navigator via setPlatform("MacIntel") in
beforeEach but never restores it; update the afterEach to also restore navigator
to its original value by capturing the original navigator/platform before
calling setPlatform (e.g., save current navigator or its platform value) and
then call setPlatform with that saved value in afterEach so setPlatform and
navigator state are symmetric with the useHotkeyOverridesStore restore;
reference setPlatform, beforeEach, and afterEach in your change.
- Around line 65-150: Add tests to cover Option+Arrow (macOS altKey) and
Ctrl+Arrow (Windows ctrlKey) branches in the keyboard handler: in the existing
describe("setupKeyboardHandler") block add cases that call
setupKeyboardHandler(terminal.xterm, { onWrite }) and then synthesize
ArrowLeft/ArrowRight key events with altKey=true (Option) and ctrlKey=true
(Ctrl) via makeKeyEvent; assert terminal.invoke returns false,
key.defaultPrevented is true, and the collected writes equal ["\x1bb"] for left
and ["\x1bf"] for right. Reference the helper/test utilities makeXterm,
makeKeyEvent, setupKeyboardHandler and the onWrite capture used in other tests
to locate where to add these new assertions.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts`:
- Around line 183-187: The conditional that checks
isTerminalReservedEvent(event) is dead because both branches return true; remove
the redundant if and replace it with a single return true, keeping or expanding
the inline comment that explains terminal-reserved chords should be handled by
xterm (so the intent remains clear); update the handler function (the closure
returned by terminalKeyboardHandler) to eliminate the unreachable branch and
leave a concise comment referencing terminal-reserved chords.
🪄 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: b48a9429-9e04-45a0-8d36-ac6ec07a917c
📒 Files selected for processing (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalHotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
10 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 25, 2026
upstream の 10 commits は #426 と #427 で fork 固有の差分を保ちながら個別に cherry-pick 取り込み済み。本 merge は ours strategy で **記録だけ** マージ済みに することで behind=0 を達成し、git 履歴上の追跡を正しくする。 Cherry-pick 取り込み済 (PR #426): - 5aab22a fix closed picker filters (superset-sh#3702) → cdb52f9 - 99db5be [codex] simplify workspace controls (superset-sh#3714) → f079606 - 186078a fix(chat): prevent ask_user question from shadowing sandbox access prompt (superset-sh#3662) → 09d6b57 - 47893c2 fix desktop workspace creation title clamp (superset-sh#3718) → 6a8c4ae - 09323ff Add diff pane file viewer action (superset-sh#3715) → 817ed8d - a5891c6 remove pending launch log (superset-sh#3721) → 0764d03 - c83de0c Add Tiptap table support (superset-sh#3719) → e67a885 - 486b621 [codex] Fix v2 terminal lifecycle after sleep (superset-sh#3711) → b71fbbb (+ #426 内 review fixups) Cherry-pick 取り込み済 (PR #427): - e07aef6 feat(desktop): play v2 notification hooks client-side (superset-sh#3675) → 27ac18a - eae6008 [codex] Port v2 terminal hotkeys to v1 (superset-sh#3724) → 05a77b8 (+ #427 内 Windows .ps1 v2 化) Fork 固有領域は変更ゼロで保持: 19 tRPC procedures (workspaces.githubExtended)、 AudioScheduler / Aivis TTS / notification-manager、terminal suggestion handler (新 terminalKeyboardHandler.ts に移植)、TERMINAL_OPTIONS、SUPERSET_WORKSPACE_NAME、 MainWindowEffects、INCEPTION_AUTH_PROVIDER_ID、v1MigrationState、TiptapPromptEditor、 electron-builder.ts (dmg.size="4g", fileAssociations)、Service Status Dashboard、 Linux daemon systemd、Worktree auto-sync、Windows support、DnD scratch route 他。
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
CLEAR_TERMINALthrough the shared terminal hotkey hook while still clearing xterm display and persisted scrollback.Root Cause
V1 still handled clear and some shortcut routing inside the terminal custom key handler. That meant terminal-specific handling could win before the shared hotkey resolver, while v2 had already moved to the safer model where registered app hotkeys escape xterm first.
The review also identified that consumed Option/Ctrl word-navigation translations wrote terminal bytes without calling
preventDefault(), which could leak browser navigation/editing behavior in Electron.Validation
bun run testfromapps/desktop- 1815 pass, 0 failbun run typecheckfromapps/desktopbunx biome checkon touched terminal/hotkey filesgit diff --checkSummary by CodeRabbit