Repository navigation
fix(desktop): share terminal key handler with v2 - #3968
Merged
Merged
Conversation
Contributor
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTerminal keyboard event handling is refactored from a single ChangesTerminal Keyboard Event Handler Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
Greptile SummaryConsolidates terminal keyboard handling into a shared Two implicit behaviour changes in v1 are worth confirming:
Confidence Score: 4/5Safe to merge with awareness of the intentional Shift+Enter behaviour change in v1. No critical bugs; the refactoring is clean and well-tested. Two P2-level behaviour differences (Shift+Enter removal, isExitedRef bypass) appear intentional but are undocumented, keeping the score at 4. useTerminalLifecycle.ts and terminalKeyboardHandler.ts — both carry the implicit v1 behaviour changes.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.ts | New shared key handler module – clean extraction of the v2 logic into a testable, platform-injected factory; no logic regressions found. |
| apps/desktop/src/renderer/lib/terminal/clipboard-shortcuts.ts | Clipboard shortcut helpers moved to shared lib location; logic is identical to the old file they replace. |
| apps/desktop/src/renderer/lib/terminal/line-edit-translations.ts | Adds Mac Cmd+Enter → ESC+CR mapping; order of checks ensures it fires before the Cmd-bubble branch in the outer handler. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts | Reduced to a one-line wrapper around installTerminalKeyEventHandler; the old KeyboardHandlerOptions (onShiftEnter/onWrite callbacks) are gone. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | Drops onShiftEnter/onWrite callbacks from setupKeyboardHandler call; line-edit writes now go via terminal.input instead of handleWrite, bypassing the isExitedRef guard. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Replaces inline createKeyEventHandler with installTerminalKeyEventHandler; cleanup return value is discarded, matching the previous behaviour. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.ts | Now a pure re-export shim pointing to the shared lib module; maintains backward compatibility for any remaining consumers. |
| apps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.test.ts | New unit tests covering Cmd+Enter, unhandled Cmd chord bubbling, and ordinary key pass-through; good coverage of the key handler factory. |
| apps/desktop/src/renderer/lib/terminal/line-edit-translations.test.ts | New tests for Cmd+Enter, Cmd+Shift+Enter (no-op), and non-Mac guard; covers the new mapping cleanly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent] --> B{resolveHotkeyFromEvent?}
B -- registered hotkey --> C[bubble: return false]
B -- no match --> D{translateLineEditChord?}
D -- Mac Cmd+Backspace/Arrow/Enter\nWin Ctrl+Arrow --> E[terminal.input translation, true\npreventDefault → return false]
D -- no match --> F{shouldSelectAllShortcut?\nMac Cmd+A}
F -- yes --> G[terminal.selectAll\npreventDefault → return false]
F -- no --> H{shouldBubbleClipboardShortcut?}
H -- Mac: any metaKey\nWin/Linux: Ctrl+V, Ctrl+Shift+C, etc --> I[bubble: return false]
H -- no match --> J[xterm handles normally: return true]
subgraph Shared module terminal-key-event-handler.ts
B
D
F
H
end
subgraph v1 entry-point
K[setupKeyboardHandler] --> L[installTerminalKeyEventHandler]
end
subgraph v2 entry-point
M[createRuntime] --> L
end
L --> A
Comments Outside Diff (1)
-
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts, line 10-11 (link)Shift+Enter no longer intercepted in v1
The old handler had an explicit
isShiftEnterbranch (shiftKey && !metaKey && !ctrlKey && !altKey) that sent\x1b\rand blocked xterm. The new shared handler has no such branch —Shift+Enternow returnstrueand is processed by xterm, which (with kitty protocol active) encodes it as\x1b[13;2uinstead. The analogous sequence (\x1b\r) is now only sent forCmd+Enteron Mac. If v1 users rely onShift+Enterto submit a newline to chat TUIs, that shortcut silently breaks here.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts Line: 10-11 Comment: **Shift+Enter no longer intercepted in v1** The old handler had an explicit `isShiftEnter` branch (`shiftKey && !metaKey && !ctrlKey && !altKey`) that sent `\x1b\r` and blocked xterm. The new shared handler has no such branch — `Shift+Enter` now returns `true` and is processed by xterm, which (with kitty protocol active) encodes it as `\x1b[13;2u` instead. The analogous sequence (`\x1b\r`) is now only sent for `Cmd+Enter` on Mac. If v1 users rely on `Shift+Enter` to submit a newline to chat TUIs, that shortcut silently breaks here. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts:758
**v1 line-edit writes now bypass `isExitedRef` guard**
Previously, line-edit chord bytes were delivered via `handleWrite`, which short-circuits when `isExitedRef.current` is true. The new shared handler sends them directly with `terminal.input(translation, true)`, which triggers `onData` unconditionally. If a line-edit key is pressed in the brief window between PTY exit and terminal teardown, the `onData` callback could attempt to write to a closed PTY. This was not an issue in v2 (`terminal-runtime.ts`) because it never used `handleWrite`, but it is a subtle regression for v1.
### Issue 2 of 2
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalKeyboardHandler.ts:10-11
**Shift+Enter no longer intercepted in v1**
The old handler had an explicit `isShiftEnter` branch (`shiftKey && !metaKey && !ctrlKey && !altKey`) that sent `\x1b\r` and blocked xterm. The new shared handler has no such branch — `Shift+Enter` now returns `true` and is processed by xterm, which (with kitty protocol active) encodes it as `\x1b[13;2u` instead. The analogous sequence (`\x1b\r`) is now only sent for `Cmd+Enter` on Mac. If v1 users rely on `Shift+Enter` to submit a newline to chat TUIs, that shortcut silently breaks here.
Reviews (1): Last reviewed commit: "fix(desktop): share terminal key handler..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2
🤖 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/lib/terminal/terminal-key-event-handler.test.ts`:
- Around line 4-18: The test assigns a fake global electronTRPC and never
restores it; save the original value of globalThis.electronTRPC before
overwriting and restore it in a teardown (afterAll or afterEach) so other tests
aren’t polluted. Specifically, capture the current (possibly undefined)
globalThis.electronTRPC into a variable at the top of the suite, then after the
suite finishes set globalThis.electronTRPC back to that saved value; apply this
around the block that stubs electronTRPC and around usage of
createTerminalKeyEventHandler to ensure cleanup.
In `@apps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.ts`:
- Around line 33-35: The platform string from options.platform can be "darwin"
which incorrectly sets isWindows true via "darwin".includes("win"); update
normalization: ensure resolvePlatform or the local platform variable maps
"darwin" to "mac" (or otherwise normalizes known aliases) before computing isMac
and isWindows; then derive isMac = platform.includes("mac") and isWindows =
platform.includes("win") so mac-specific key paths are enabled for "darwin"
inputs (refer to resolvePlatform, options.platform, isMac, isWindows).
🪄 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: 969e55c9-b5d6-4ade-861a-395200459cf9
📒 Files selected for processing (9)
apps/desktop/src/renderer/lib/terminal/clipboard-shortcuts.tsapps/desktop/src/renderer/lib/terminal/line-edit-translations.test.tsapps/desktop/src/renderer/lib/terminal/line-edit-translations.tsapps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.test.tsapps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/clipboardShortcuts.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.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
fix-cmd-enter-newline-v1
branch
from
May 1, 2026 23:23
b24c117 to
83c6dc7
Compare
Kitenite
force-pushed
the
fix-cmd-enter-newline-v1
branch
from
May 1, 2026 23:27
83c6dc7 to
0244aa0
Compare
After 0244aa0 these v1 files were 1-line wrappers around the shared lib/terminal modules. Inline the install call in useTerminalLifecycle and move the clipboard-shortcuts test next to the impl it covers.
A Node-style platform string ("darwin") would match the "win" substring
and silently flip isMac/isWindows. Doesn't fire in production
(navigator.platform returns "MacIntel") but bites any test or future
caller passing process.platform directly. Also restore globalThis.
electronTRPC after the test suite to avoid order-dependent pollution.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
added a commit
that referenced
this pull request
May 2, 2026
Auto-fixable lint issues introduced in #3968 (terminal key handler share between v1/v2). Not related to this PR's pane-registry work, but blocks CI; folding the trivial reformat into the rebase here so PR3 can land. Should be a no-op once main resolves it.
saddlepaddle
pushed a commit
that referenced
this pull request
May 2, 2026
PR #3968 (terminal key handler refactor) introduced two biome violations on main: an import sort order and a quote-style nit. Both auto-fixed by `biome check --write`. No behavior change.
saddlepaddle
added a commit
that referenced
this pull request
May 2, 2026
* feat(cli): add --assignee filter to `tasks list` Brings the CLI in line with the SDK (`assigneeId`) and MCP-v2 (`assigneeId`) which both already exposed assignee-by-user-id filtering on `task.list`. The tRPC procedure has supported it all along — the CLI was the only surface missing it. * chore: fix lint regressions on main from #3968 PR #3968 (terminal key handler refactor) introduced two biome violations on main: an import sort order and a quote-style nit. Both auto-fixed by `biome check --write`. No behavior change. --------- Co-authored-by: Town Hall <codetown@Town-Hall.local>
Kitenite
added a commit
that referenced
this pull request
May 2, 2026
* feat(desktop): workspace pane store registry (v2 PR 3)
PR 3 of the canonical workspace.create() refactor — see
plans/20260430-pane-store-registry-pr3.md.
Decouples pane state ownership from the workspace route mount. The V2
workspace pane store now lives in a module-level registry keyed by
workspaceId, rather than being created inside `useV2WorkspacePaneLayout`
on mount. This unblocks PR 4 (`workspace.create()`) returning a list of
already-started sessions that the renderer can write directly into the
store before — or without — navigating.
Files
- New: apps/desktop/src/renderer/lib/workspace-pane-registry/
- workspace-pane-registry.ts: initWorkspacePaneRegistry(deps),
getOrCreateWorkspacePaneStore(workspaceId), dropWorkspacePaneStore,
test reset helper. Bidirectional sync wired at store creation:
seeds from v2WorkspaceLocalState.paneLayout, writes back on store
changes (when row exists), pushes external row changes into store,
snapshot guard prevents echo.
- addLaunchPanes.ts: takes the doc-shape launches array, dedupes by
terminalId / chatSessionId, focuses. Attach-only — no
initialCommand. Existing presets / pending-launch path keep using
addTab directly.
- 14 unit tests against an in-memory localStorage-backed collection.
- Modified: useV2WorkspacePaneLayout — drops 87 lines of useState +
useLiveQuery + persistence effects, replaced by a single
getOrCreateWorkspacePaneStore call. ensureWorkspaceInSidebar still
runs on mount.
- Modified: CollectionsProvider — initializes the registry inside the
collections useMemo (synchronously, before children render) so the
workspace route's useState initializer sees a ready registry. Reinits
on org switch.
Out of scope (deferred)
- The pending-row launch-adoption path (useConsumePendingLaunch) is
unchanged. PR 5 deletes it when the new workspace modal moves onto
workspace.create().
- TerminalPaneData.initialCommand is already optional in current code
and TerminalPane already handles undefined; no change needed.
* fix(panes,desktop): two regressions found while testing PR3
Manual testing of the modal → new workspace → agent flow on PR3 turned
up two bugs that combined to wipe the agent's `initialCommand` before
the terminal pane could send it.
panes/Tab.tsx — add `key={pane.id}` to <Pane>:
- Without a key, React reconciles Pane instances by position. Switching
the active tab to one whose Pane occupies the same layout slot reuses
the previous Pane's component instance. Hooks (notably
`useRef(paneData.initialCommand)` in TerminalPane) keep stale values
from the prior pane, so the agent's initialCommand never reached
terminalRuntimeRegistry.connect.
- Latent bug — didn't manifest before PR3 because the legacy
useV2WorkspacePaneLayout's effect-based "load row → replaceState"
timing happened to give Pane a fresh mount each time. PR3's registry
seeds the store before the route mounts, exposing the issue.
workspace-pane-registry.ts — strengthen the row→store guard:
- Compare incoming row state to the *current* store snapshot, not the
tracked `lastSyncedSnapshot`. Two reasons:
1. Tanstack DB delivers existing rows as initial-state `insert`
events on subscribe, with the post-our-write value. The old
guard mistakenly thought these were unrelated external updates.
2. Immer's `update(draft => { draft.paneLayout = ... })` writes do
not preserve key insertion order, so JSON.stringify produced
different strings for structurally equal store/row states.
- Also introduce `deepSortKeys`-based stable serialization so snapshot
equality is structural rather than sensitive to key order.
Validated end-to-end: modal-create → agent terminal opens → "claude
…" prompt runs and Claude responds. 14/14 unit tests still pass.
* fix(desktop): drop pane store on workspace deletion
Calls dropWorkspacePaneStore(workspaceId) from
useDashboardSidebarState.removeWorkspaceFromSidebar so registry entries
don't accumulate forever when workspaces are removed. Pairs with the
existing v2WorkspaceLocalState.delete + cleanupWorkspacePaneRuntimes
calls in the same function.
* fix(desktop): address PR review on pane-store registry
Three valid review comments on the registry:
1. Idempotency check compared the wrapper object reference, not the
collection instance (greptile P2). With the wrapper-only check, every
call site passing a fresh `{ v2WorkspaceLocalState: ... }` literal
would have torn down all stores even when the underlying collection
hadn't changed. Switched to instance-level comparison; passing the
same collection instance is now a true no-op.
2. Pre-mount addLaunchPanes panes weren't reliably persisted (greptile
P2 + cubic P1). When `addLaunchPanes` runs before the v2WorkspaceLocal
State row exists, the store's write-back is skipped (no row), then
`ensureWorkspaceInSidebar` later inserts the row with EMPTY paneLayout.
The previous code's "compare incoming row to current store" guard
would have called `replaceState(EMPTY)` and wiped the in-memory tabs.
New logic uses three-way reconciliation: compare incoming, store, and
the last-synced agreement to decide direction. If the store has
unsynced mutations vs lastSyncedSnapshot, push store→row; otherwise
pull row→store. Added a regression test.
3. `__resetWorkspacePaneRegistryForTests` removed from the public
barrel (greptile P2). Tests now import it directly from the
implementation file.
The useMemo-as-side-effect concern (cubic P2 + greptile P2) is
intentional for the synchrony requirement (see expanded comment in
CollectionsProvider). Mitigation: #1 above ensures any unintended memo
recomputation receiving the same collection is a true no-op.
* chore(desktop): tighten pane-registry comments
- addLaunchPanes JSDoc: remove the now-stale 'persists when the next
store change writes back' line. The registry's three-way
reconciliation persists pre-mount panes the moment the row is
inserted; no follow-up mutation needed.
- Test-only helper imports: standardize the explanatory comment to one
short line in both test files.
* chore: format two terminal-key-event-handler files (CI unblocker)
Auto-fixable lint issues introduced in #3968 (terminal key handler
share between v1/v2). Not related to this PR's pane-registry work,
but blocks CI; folding the trivial reformat into the rebase here so
PR3 can land. Should be a no-op once main resolves it.
darklow
added a commit
to darklow/superset
that referenced
this pull request
May 4, 2026
Drop redundant local shortcut fixes superseded upstream: - f07b041 (bubble-only-registered hotkeys) — now in PR ParadevAI#3968 via lib/terminal/terminal-key-event-handler.ts - ecb3545 (readline shortcuts in TERMINAL_RESERVED) — subsumed by PR ParadevAI#3968 translateLineEditChord + reserved-chord canonicalization Keep: - 4131e08 Cmd+Q sync dialog fix (still missing upstream); merged with upstream's await runDevQuitCleanup by making handler async - setupCopyOnSelect (local-only feature) - Renumber upstream's duplicate 0040/0041 local-db migrations to 0042/0043
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
* fix(desktop): share terminal key handler with v2 * refactor(desktop): drop now-empty v1 keyboard/clipboard shims After 0244aa0 these v1 files were 1-line wrappers around the shared lib/terminal modules. Inline the install call in useTerminalLifecycle and move the clipboard-shortcuts test next to the impl it covers. * fix(desktop): normalize "darwin" as Mac in terminal key handler A Node-style platform string ("darwin") would match the "win" substring and silently flip isMac/isWindows. Doesn't fire in production (navigator.platform returns "MacIntel") but bites any test or future caller passing process.platform directly. Also restore globalThis. electronTRPC after the test suite to avoid order-dependent pollution.
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
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Share a single terminal key handler between v1 and v2, fix macOS Cmd+Enter so TUIs get a clean newline, and ensure clipboard shortcuts/app hotkeys are handled consistently. Also normalize platform detection so
"darwin"is treated as Mac.Bug Fixes
Cmd+EnterasESC+CRto TUIs; Mac-only and not when Shift is held.Cmd+Aselect-all; let app hotkeys bypass xterm."darwin"as Mac in the handler to avoid misrouted keys in tests/Node callers.Refactors
renderer/lib/terminal/terminal-key-event-handlerand installed it directly; removed v1 keyboard/clipboard shims and inlined install inuseTerminalLifecycle.renderer/lib/terminal; added key-handler and line-edit tests and cleaned up test globals.Written for commit 08a7d78. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Refactor
Tests