Add keyboard copy mode for terminal scrollback#792
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a terminal "keyboard copy mode": new shortcut and default binding, TabManager routing, Ghostty surface toggle and C APIs, comprehensive key-to-action mapping and state, UI badge indicator, and extensive tests for mappings and behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AppDelegate
participant TabManager
participant TerminalSurface as TerminalSurface/GhosttyNSView
participant KeyboardHandler as Key Event Handler
participant Clipboard
User->>AppDelegate: Trigger Cmd+Shift+M
AppDelegate->>TabManager: toggleFocusedTerminalCopyMode()
TabManager->>TerminalSurface: toggleKeyboardCopyMode()
TerminalSurface->>TerminalSurface: set keyboardCopyModeActive / init state
TerminalSurface-->>TabManager: return handled (true)
TabManager-->>AppDelegate: return true
Note over User,KeyboardHandler: In copy mode, keyboard drives navigation & selection
User->>KeyboardHandler: Press navigation/key (e.g., j,k,G,y,Esc)
KeyboardHandler->>TerminalSurface: terminalKeyboardCopyModeAction(...) -> action
TerminalSurface->>TerminalSurface: apply action (scroll/select/copy/exit)
TerminalSurface->>Clipboard: copy selected text (on copy)
TerminalSurface->>TerminalSurface: clear selection & deactivate copy mode (on exit)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 unit tests (beta)
Comment |
Greptile SummaryThis PR implements a keyboard-driven copy mode for terminal scrollback with vi-style navigation. Users can toggle copy mode with Cmd+Shift+M, navigate using j/k/h/l and arrow keys, start selection with 'v', and copy with 'y'. Key Changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User presses Cmd+Shift+M] --> B[AppDelegate.keyDown]
B --> C[TabManager.toggleFocusedTerminalCopyMode]
C --> D[TerminalSurface.toggleKeyboardCopyMode]
D --> E[GhosttyNSView.toggleKeyboardCopyMode]
E --> F{Toggle keyboardCopyModeActive}
F -->|Mode ON| G[Copy mode activated]
F -->|Mode OFF| H[ghostty_surface_clear_selection]
G --> I[User presses key in copy mode]
I --> J[GhosttyNSView.keyDown]
J --> K{handleKeyboardCopyModeIfNeeded}
K -->|Command modifier| L[Bypass copy mode]
L --> M[Normal shortcut handling]
K -->|Other key| N[terminalKeyboardCopyModeAction]
N --> O{Determine action}
O -->|j/k/arrows| P[Scroll lines or adjust selection]
O -->|v| Q[Start/clear selection]
O -->|y| R[Copy and exit]
O -->|q/Esc| S[Exit copy mode]
O -->|g/G| T[Scroll to top/bottom]
O -->|Ctrl+u/d| U[Page up/down]
P --> V[performBindingAction]
Q --> W[ghostty_surface_select_cursor_cell]
R --> X[performBindingAction copy_to_clipboard]
X --> Y[Exit copy mode]
Last reviewed commit: e14914f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
1337-1337: RenametestVGYMappingfor precision.Line 1337 validates
v/y;gis covered in a separate test. Renaming improves scanability when failures are triaged.Proposed rename
- func testVGYMapping() { + func testVYMapping() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` at line 1337, Rename the test function testVGYMapping to reflect that it only validates 'v' and 'y' (for example testVYMapping or testVAndYMapping) so it accurately describes its coverage; update the function name in the declaration and any references to testVGYMapping (e.g., in test registration or call sites) to the new name to keep tests discoverable and failures easier to triage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 1388-1399: Add a regression test that verifies pressing the "q"
key maps to .exit in copy mode: create a new test (e.g., testQAlwaysExits)
alongside testEscapeAlwaysExits that calls terminalKeyboardCopyModeAction with
charactersIgnoringModifiers: "q" (or "Q" if uppercase handling required),
keyCode appropriate for "q" (or use character-based assertion), empty
modifierFlags, and hasSelection: false, and assert the result equals .exit to
cover the q-to-exit mapping in terminalKeyboardCopyModeAction.
In `@Sources/AppDelegate.swift`:
- Around line 5974-5980: The new copy-mode keyboard branch doesn't emit the
unified debug trace; add a debug log call using the project's dlog("...")
wrapper when the shortcut is matched — e.g., inside the if that calls
matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for:
.toggleTerminalCopyMode)) add a dlog message noting the shortcut hit and whether
toggleFocusedTerminalCopyMode() handled it; ensure the dlog call is wrapped in
`#if` DEBUG / `#endif` as required by the Swift debug logging convention and place
it near the call to tabManager?.toggleFocusedTerminalCopyMode() so key-event
observability is consistent.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3306-3308: The keyUp handler must not forward release events for
keys already consumed by copy mode; update the keyUp path (the method that
currently forwards release events) to consult handleKeyboardCopyModeIfNeeded or
track consumed keys and suppress forwarding when copy mode consumed the
key—reference the existing handleKeyboardCopyModeIfNeeded(surface:) call used in
keyDown and either call it from keyUp before forwarding or maintain a per-key
consumed set updated in keyDown and cleared in keyUp to ensure press/release
symmetry and prevent release-only events from reaching the terminal.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Line 1337: Rename the test function testVGYMapping to reflect that it only
validates 'v' and 'y' (for example testVYMapping or testVAndYMapping) so it
accurately describes its coverage; update the function name in the declaration
and any references to testVGYMapping (e.g., in test registration or call sites)
to the new name to keep tests discoverable and failures easier to triage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftghosttyghostty.h
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
1388-1399:⚠️ Potential issue | 🟡 MinorAdd explicit regression coverage for
qexit mapping.Line 1388 currently validates
Esconly;qis also an exit path and should be locked by test.Proposed test addition
func testEscapeAlwaysExits() { XCTAssertEqual( terminalKeyboardCopyModeAction( keyCode: 53, charactersIgnoringModifiers: "", modifierFlags: [], hasSelection: false ), .exit ) } + + func testQAlwaysExits() { + XCTAssertEqual( + terminalKeyboardCopyModeAction( + keyCode: 12, // kVK_ANSI_Q + charactersIgnoringModifiers: "q", + modifierFlags: [], + hasSelection: false + ), + .exit + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 1388 - 1399, Add an explicit regression test that asserts the `terminalKeyboardCopyModeAction` maps the 'q' key to `.exit` (in addition to the existing Esc test). Create a new test (e.g., `testQAlwaysExits` or extend `testEscapeAlwaysExits`) that calls `terminalKeyboardCopyModeAction` with the appropriate parameters for the 'q' key (charactersIgnoringModifiers set to "q", empty modifierFlags, hasSelection false) and XCTAssertEqual the result to `.exit`, referencing the `terminalKeyboardCopyModeAction` function and the current `testEscapeAlwaysExits` test as the model.Sources/GhosttyTerminalView.swift (1)
3332-3334:⚠️ Potential issue | 🟠 MajorSuppress release events for keys consumed by copy mode.
Line 3332 can consume
keyDown, butkeyUpstill forwards a release unconditionally (Lines 3538-3552), causing press/release asymmetry and release-only leakage into terminal input.🔧 Proposed fix
override func keyUp(with event: NSEvent) { guard let surface = ensureSurfaceReadyForInput() else { super.keyUp(with: event) return } + if keyboardCopyModeActive, + !terminalKeyboardCopyModeShouldBypassForShortcut(modifierFlags: event.modifierFlags) { + return + } // Build release events from the same translation path as keyDown so // consumers that depend on precise key identity (for example Space // hold/release flows) receive consistent metadata. var keyEvent = ghosttyKeyEvent(for: event, surface: surface)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3332 - 3334, handleKeyboardCopyModeIfNeeded(event, surface:) can consume keyDown events but keyUp currently always forwards releases, causing release-only leakage; fix by tracking which keys copy-mode consumed and suppress their keyUp forwarding: add a scoped Set (e.g., consumedKeysByCopyMode) keyed by the event identity you use (keyCode, keyIdentifier, or charactersIgnoringModifiers) and when handleKeyboardCopyModeIfNeeded(...) returns true in the keyDown path add that identity to the set and return; in the keyUp handler check the same identity against consumedKeysByCopyMode, and if present remove it and do not forward the release to the terminal. Ensure the lookup uses the same identity logic in both keyDown and keyUp and clean up entries to avoid leaks.
🧹 Nitpick comments (1)
Sources/GhosttyTerminalView.swift (1)
2372-2376: Avoid double copy-mode state propagation during toggle.
GhosttyNSView.toggleKeyboardCopyMode()already callsterminalSurface?.setKeyboardCopyModeActive(active), andTerminalSurface.toggleKeyboardCopyMode()callssetKeyboardCopyModeActive(...)again. Consider a single owner for this update path to avoid redundant UI/state churn.Also applies to: 2912-2925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2372 - 2376, The toggleKeyboardCopyMode path is causing duplicate state propagation: GhosttyNSView.toggleKeyboardCopyMode() calls surfaceView.toggleKeyboardCopyMode() and then calls setKeyboardCopyModeActive(...) again, but surfaceView/TerminalSurface already update the UI via terminalSurface?.setKeyboardCopyModeActive(active); remove the redundant call in GhosttyTerminalView.toggleKeyboardCopyMode so only surfaceView.toggleKeyboardCopyMode() drives the change (and mirror the same fix for the similar block around lines 2912-2925). Ensure you keep the return value (handled) and rely on TerminalSurface.toggleKeyboardCopyMode()/terminalSurface?.setKeyboardCopyModeActive to perform the UI update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 1388-1399: Add an explicit regression test that asserts the
`terminalKeyboardCopyModeAction` maps the 'q' key to `.exit` (in addition to the
existing Esc test). Create a new test (e.g., `testQAlwaysExits` or extend
`testEscapeAlwaysExits`) that calls `terminalKeyboardCopyModeAction` with the
appropriate parameters for the 'q' key (charactersIgnoringModifiers set to "q",
empty modifierFlags, hasSelection false) and XCTAssertEqual the result to
`.exit`, referencing the `terminalKeyboardCopyModeAction` function and the
current `testEscapeAlwaysExits` test as the model.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3332-3334: handleKeyboardCopyModeIfNeeded(event, surface:) can
consume keyDown events but keyUp currently always forwards releases, causing
release-only leakage; fix by tracking which keys copy-mode consumed and suppress
their keyUp forwarding: add a scoped Set (e.g., consumedKeysByCopyMode) keyed by
the event identity you use (keyCode, keyIdentifier, or
charactersIgnoringModifiers) and when handleKeyboardCopyModeIfNeeded(...)
returns true in the keyDown path add that identity to the set and return; in the
keyUp handler check the same identity against consumedKeysByCopyMode, and if
present remove it and do not forward the release to the terminal. Ensure the
lookup uses the same identity logic in both keyDown and keyUp and clean up
entries to avoid leaks.
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2372-2376: The toggleKeyboardCopyMode path is causing duplicate
state propagation: GhosttyNSView.toggleKeyboardCopyMode() calls
surfaceView.toggleKeyboardCopyMode() and then calls
setKeyboardCopyModeActive(...) again, but surfaceView/TerminalSurface already
update the UI via terminalSurface?.setKeyboardCopyModeActive(active); remove the
redundant call in GhosttyTerminalView.toggleKeyboardCopyMode so only
surfaceView.toggleKeyboardCopyMode() drives the change (and mirror the same fix
for the similar block around lines 2912-2925). Ensure you keep the return value
(handled) and rely on
TerminalSurface.toggleKeyboardCopyMode()/terminalSurface?.setKeyboardCopyModeActive
to perform the UI update.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77f4a06508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sources/AppDelegate.swift
Outdated
| if !handled { | ||
| NSSound.beep() | ||
| } | ||
| return true |
There was a problem hiding this comment.
This handler always returns true once the shortcut matches, even when toggleFocusedTerminalCopyMode() fails (for example, when a browser panel or another non-terminal responder is focused), so the local shortcut monitor swallows Cmd+Shift+M and the focused control never receives it. In practice this breaks browser/web-app shortcuts in those contexts and only plays a beep; returning false when no terminal panel is actionable would preserve expected responder behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/GhosttyTerminalView.swift (1)
3819-3833:⚠️ Potential issue | 🟠 MajorSuppress
keyUpforwarding for keys consumed by copy mode.
keyDownis consumed in copy mode (Line 3613), butkeyUpstill always emits release events (Line 3819 onward). That leaks release-only events into terminal handling and breaks press/release symmetry.🔧 Proposed fix
override func keyUp(with event: NSEvent) { guard let surface = ensureSurfaceReadyForInput() else { super.keyUp(with: event) return } + if keyboardCopyModeActive, + !terminalKeyboardCopyModeShouldBypassForShortcut(modifierFlags: event.modifierFlags) { + return + } // Build release events from the same translation path as keyDown so // consumers that depend on precise key identity (for example Space // hold/release flows) receive consistent metadata. var keyEvent = ghosttyKeyEvent(for: event, surface: surface)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3819 - 3833, keyUp is currently always forwarding release events (using ghosttyKeyEvent(...) and sendGhosttyKey(...)) even when keyDown consumed the event in copy mode; replicate the same consume-check used in keyDown (or track consumed key identities on keyDown) and early-return to super.keyUp when the key was consumed, instead of building and sending a GHOSTTY_ACTION_RELEASE event. Concretely: either call the same copy-mode check from keyDown inside keyUp (using ensureSurfaceReadyForInput() and the surface's copy-mode predicate) or maintain a small Set of consumed key identifiers populated in keyDown and consulted/cleared in keyUp before calling ghosttyKeyEvent(...) / sendGhosttyKey(...).cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
1544-1555:⚠️ Potential issue | 🟡 MinorAdd a regression assertion for
q-to-exit mapping.Line 1544 currently verifies
Esconly; copy-mode exit behavior also includesq, so this leaves a coverage gap.Proposed test addition
func testEscapeAlwaysExits() { XCTAssertEqual( terminalKeyboardCopyModeAction( keyCode: 53, charactersIgnoringModifiers: "", modifierFlags: [], hasSelection: false ), .exit ) } + + func testQAlwaysExits() { + XCTAssertEqual( + terminalKeyboardCopyModeAction( + keyCode: 12, // kVK_ANSI_Q + charactersIgnoringModifiers: "q", + modifierFlags: [], + hasSelection: false + ), + .exit + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 1544 - 1555, Add a sibling regression assertion to testEscapeAlwaysExits that verifies the 'q' key also maps to .exit: call terminalKeyboardCopyModeAction with charactersIgnoringModifiers "q" (and an appropriate keyCode for 'q', no modifierFlags, hasSelection: false) and assert the result equals .exit; mirror the existing test structure so both Esc and 'q' coverage exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3224-3227: The scroll handlers for .scrollLines and .scrollPage
perform binding actions but don't refresh the copy-mode viewport anchor
(keyboardCopyModeViewportRow), causing stale targets for yy/Y; after the
performBindingAction call in the .scrollLines and .scrollPage cases, invoke the
method that recomputes the copy-mode viewport anchor (e.g.,
refreshKeyboardCopyModeViewportRow() or updateKeyboardCopyModeViewportRow()) so
the keyboardCopyModeViewportRow is recalculated immediately after scrolling;
ensure you call this for both branches (the one using the ignored return and the
one using repeatCount: count).
---
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 1544-1555: Add a sibling regression assertion to
testEscapeAlwaysExits that verifies the 'q' key also maps to .exit: call
terminalKeyboardCopyModeAction with charactersIgnoringModifiers "q" (and an
appropriate keyCode for 'q', no modifierFlags, hasSelection: false) and assert
the result equals .exit; mirror the existing test structure so both Esc and 'q'
coverage exists.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3819-3833: keyUp is currently always forwarding release events
(using ghosttyKeyEvent(...) and sendGhosttyKey(...)) even when keyDown consumed
the event in copy mode; replicate the same consume-check used in keyDown (or
track consumed key identities on keyDown) and early-return to super.keyUp when
the key was consumed, instead of building and sending a GHOSTTY_ACTION_RELEASE
event. Concretely: either call the same copy-mode check from keyDown inside
keyUp (using ensureSurfaceReadyForInput() and the surface's copy-mode predicate)
or maintain a small Set of consumed key identifiers populated in keyDown and
consulted/cleared in keyUp before calling ghosttyKeyEvent(...) /
sendGhosttyKey(...).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91b01e53-52c8-4b23-9552-8456b93d15d3
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1186d5cf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| keyboardCopyModeInputState.reset() | ||
| keyboardCopyModeActive = active | ||
| if active, let surface { |
There was a problem hiding this comment.
setKeyboardCopyModeActive(_:) resets the copy-mode input parser but leaves keyboardCopyModeConsumedKeyUps untouched, even though that set is only drained in keyUp(with:). If a copy-mode-consumed key-down is followed by a focus/responder transition (so its key-up is never delivered here), the stale key code survives and the next normal key-up for that code gets dropped, creating a press-without-release sequence in Ghostty. Clearing the suppression set when copy mode is toggled (and on focus transitions) prevents this stuck-key behavior.
Useful? React with 👍 / 👎.
| // Only consume when a focused terminal actually handled the toggle. | ||
| // Otherwise allow the event to continue through the responder chain. | ||
| return handled |
There was a problem hiding this comment.
This branch consumes Cmd+Shift+M at the app shortcut layer (return handled), which prevents GhosttyNSView.keyDown from sending a matching press for keycode 46, but GhosttyNSView.keyUp still emits an unconditional GHOSTTY_ACTION_RELEASE unless that key was marked by the copy-mode handler. When a terminal is focused and this toggle shortcut succeeds, terminal apps can receive a release-without-press m event; this shortcut path should also register key-up suppression.
Useful? React with 👍 / 👎.
|
Hi, thanks for implementing this feature! May I know when I enter the Copy Mode, is there a way to show where the cursor is? Currently when using |
2 participants
Summary
Dependency
Verification
Closes #788
Summary by CodeRabbit
New Features
Tests