Vi mode: half-page scroll, visible cursor, gg fix#851
Conversation
Three changes to keyboard copy mode (vi mode): 1. Ctrl+U/D now scroll half-page (was full-page). Ctrl+B/F remain full-page. Uses Ghostty's scroll_page_fractional binding. 2. Entering copy mode now creates a 1-cell selection at the terminal cursor via select_cursor_cell, giving the user a visible cursor indicator. Visual mode (v) is tracked separately from Ghostty's has_selection so the cursor selection doesn't make every motion behave as visual. Exiting visual mode (v again) collapses back to the cursor cell. 3. Single 'g' is now a prefix key requiring 'gg' to scroll to top, matching standard vim behavior. Uses the same pendingG state machine pattern as pendingYankLine for 'yy'. Part of #846
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds vi-like copy-mode enhancements: a new fractional-scroll action ( Changes
Sequence DiagramsequenceDiagram
participant User as User (Keyboard)
participant Resolver as Input Resolver
participant State as Copy-Mode State
participant Selection as Selection Manager
participant View as Viewport
User->>Resolver: Press 'g'
Resolver->>State: check pendingG
alt pendingG == false
State->>State: set pendingG = true
State-->>Resolver: no action (prefix)
else pendingG == true
Resolver->>State: resolve 'gg'
State->>Selection: clear/adjust selection or set 1-cell cursor at top
Selection->>View: scroll to top / update viewport
View->>View: render
State->>State: pendingG = false
end
Note over Resolver,View: Ctrl+U/Ctrl+D -> Resolver emits scrollHalfPage(delta) -> State/View handle fractional scroll and preserve visual cursor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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: 66b1a4aac2
ℹ️ 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".
| // Create a 1-cell selection at the terminal cursor to serve as a | ||
| // visible cursor indicator in copy mode. | ||
| _ = ghostty_surface_select_cursor_cell(surface) |
There was a problem hiding this comment.
Creating a persistent 1-cell selection on copy-mode entry makes ghostty_surface_has_selection(surface) true even in non-visual mode, so refreshKeyboardCopyModeViewportRowFromVisibleAnchor now bails out on every scroll and stops updating keyboardCopyModeViewportRow. In that state, actions like j/k, Ctrl+U/D, and prompt jumps move the viewport, but Y/yy still copies from the stale row captured when copy mode started, which produces incorrect copied lines after navigation.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
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="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:3267">
P2: Keeping a permanent 1-cell cursor selection prevents viewport-row refresh logic from running, which can make line-copy operations use stale row positions after scrolling.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryImplemented three P0 vi mode improvements:
Critical Issue Found: Fix needed:
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: 66b1a4a |
Additional Comments (1)
After this PR, Also, the function should clear the old selection before calling |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
3317-3322:⚠️ Potential issue | 🟠 MajorViewport row refresh is blocked in all copy-mode states due to persistent 1-cell cursor selection
After copy mode entry creates a persistent 1-cell cursor selection (Line 3267),
refreshKeyboardCopyModeViewportRowFromVisibleAnchorexits immediately on the guard at Line 3318 becauseghostty_surface_has_selection(surface)is always true. This prevents viewport row updates after scroll, search, or jump actions, causing subsequent line-yank operations to use the initial (stale) row rather than the current cursor position.The fix should use
keyboardCopyModeVisualActiveto guard only actual user selections:🔧 Proposed fix
private func refreshKeyboardCopyModeViewportRowFromVisibleAnchor(surface: ghostty_surface_t) { - guard !ghostty_surface_has_selection(surface) else { return } + // Visual mode owns the user selection range. Outside visual mode we still keep + // a 1-cell cursor selection for visibility and should continue refreshing row state. + guard !keyboardCopyModeVisualActive else { return } guard let anchor = keyboardCopyModeSelectionAnchor(surface: surface) else { return } keyboardCopyModeViewportRow = anchor.row - _ = ghostty_surface_clear_selection(surface) + // Preserve the visible cursor indicator. + _ = ghostty_surface_select_cursor_cell(surface) }Regression test: Enter copy mode, move viewport (j, Ctrl+D, search), then yy should copy from updated row.
Also applies to: 3376-3378 (correct use of
keyboardCopyModeVisualActive), 3265-3267 (source of persistent selection)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3317 - 3322, The refresh function refreshKeyboardCopyModeViewportRowFromVisibleAnchor currently bails whenever ghostty_surface_has_selection(surface) is true, but the copy-mode creates a persistent 1-cell selection; change the guard to only return when there is an actual user visual selection by combining the checks (i.e. return only when ghostty_surface_has_selection(surface) is true AND keyboardCopyModeVisualActive is true). Update the same pattern at the analogous spots referenced (around the code that sets the persistent 1-cell selection and the block at lines ~3376-3378) so viewport updates happen when only the persistent cursor selection exists.
🧹 Nitpick comments (2)
Sources/GhosttyTerminalView.swift (1)
3420-3424: Use the existing repeat helper for fractional page scrollThis path duplicates repeat logic already centralized in
performBindingAction(_:repeatCount:).♻️ Suggested cleanup
case let .scrollHalfPage(delta): let fraction = delta > 0 ? 0.5 : -0.5 - for _ in 0 ..< count { - _ = performBindingAction("scroll_page_fractional:\(fraction)") - } + performBindingAction("scroll_page_fractional:\(fraction)", repeatCount: count) refreshKeyboardCopyModeViewportRowFromVisibleAnchor(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 3420 - 3424, The .scrollHalfPage(delta) arm duplicates looping logic; replace the manual for-loop with the existing helper by calling performBindingAction(_:repeatCount:) using the same action string ("scroll_page_fractional:\(fraction)") and the provided count so the repeat logic is centralized in performBindingAction(_:repeatCount:) while preserving the fraction computation from the case.cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
1661-1665: Consider asserting input-state reset afterggwith selection.Other
ggtests assert thatTerminalKeyboardCopyModeInputStatereturns to default; adding the same check here improves regression protection for pending-prefix leakage.Suggested test hardening
func testGGWithSelectionAdjustsToHome() { var state = TerminalKeyboardCopyModeInputState() XCTAssertEqual(resolve(5, chars: "g", hasSelection: true, state: &state), .consume) XCTAssertEqual(resolve(5, chars: "g", hasSelection: true, state: &state), .perform(.adjustSelection(.home), count: 1)) + XCTAssertEqual(state, TerminalKeyboardCopyModeInputState()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 1661 - 1665, Add an assertion in testGGWithSelectionAdjustsToHome to verify that the TerminalKeyboardCopyModeInputState is reset to its default after the two-key "gg" sequence when hasSelection is true; specifically, after calling resolve twice (function resolve in the test) and asserting the .perform(.adjustSelection(.home)), also assert that the mutable state variable state equals a fresh TerminalKeyboardCopyModeInputState() to catch any leftover pending-prefix leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3317-3322: The refresh function
refreshKeyboardCopyModeViewportRowFromVisibleAnchor currently bails whenever
ghostty_surface_has_selection(surface) is true, but the copy-mode creates a
persistent 1-cell selection; change the guard to only return when there is an
actual user visual selection by combining the checks (i.e. return only when
ghostty_surface_has_selection(surface) is true AND keyboardCopyModeVisualActive
is true). Update the same pattern at the analogous spots referenced (around the
code that sets the persistent 1-cell selection and the block at lines
~3376-3378) so viewport updates happen when only the persistent cursor selection
exists.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 1661-1665: Add an assertion in testGGWithSelectionAdjustsToHome to
verify that the TerminalKeyboardCopyModeInputState is reset to its default after
the two-key "gg" sequence when hasSelection is true; specifically, after calling
resolve twice (function resolve in the test) and asserting the
.perform(.adjustSelection(.home)), also assert that the mutable state variable
state equals a fresh TerminalKeyboardCopyModeInputState() to catch any leftover
pending-prefix leakage.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3420-3424: The .scrollHalfPage(delta) arm duplicates looping
logic; replace the manual for-loop with the existing helper by calling
performBindingAction(_:repeatCount:) using the same action string
("scroll_page_fractional:\(fraction)") and the provided count so the repeat
logic is centralized in performBindingAction(_:repeatCount:) while preserving
the fraction computation from the case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a13cee8f-51a5-48fc-833b-4d1ecffb7d5b
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
The 1-cell cursor selection made refreshKeyboardCopyModeViewportRowFromVisibleAnchor always bail (has_selection was always true). Guard on keyboardCopyModeVisualActive instead so viewport row updates work after scrolling in non-visual mode. Also re-creates the cursor cell after refresh to preserve visibility. Additionally: use performBindingAction(_:repeatCount:) for scrollHalfPage, add state-reset assertion to testGGWithSelectionAdjustsToHome. Addresses review feedback from CodeRabbit, Cubic, Codex, and Greptile.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
1668-1673: Add state-reset assertion in countedggtest for parity.
testCountedGGvalidates action/count, but not that input state is fully cleared afterward (unlike adjacentggtests). Adding that check tightens regression coverage.Suggested test tightening
func testCountedGG() { var state = TerminalKeyboardCopyModeInputState() XCTAssertEqual(resolve(22, chars: "5", hasSelection: false, state: &state), .consume) XCTAssertEqual(resolve(5, chars: "g", hasSelection: false, state: &state), .consume) XCTAssertEqual(resolve(5, chars: "g", hasSelection: false, state: &state), .perform(.scrollToTop, count: 5)) + XCTAssertEqual(state, TerminalKeyboardCopyModeInputState()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 1668 - 1673, The testCountedGG currently asserts the returned actions/counts but not that the TerminalKeyboardCopyModeInputState is reset afterward; update the testCountedGG to check that the input state (the variable state of type TerminalKeyboardCopyModeInputState) is cleared after the last resolve call by adding an assertion (e.g., XCTAssertEqual or XCTAssertTrue) that verifies state equals a fresh TerminalKeyboardCopyModeInputState() or that its internal fields are in their initial values; locate the testCountedGG function and add the state-reset assertion after the final resolve(...) assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 1668-1673: The testCountedGG currently asserts the returned
actions/counts but not that the TerminalKeyboardCopyModeInputState is reset
afterward; update the testCountedGG to check that the input state (the variable
state of type TerminalKeyboardCopyModeInputState) is cleared after the last
resolve call by adding an assertion (e.g., XCTAssertEqual or XCTAssertTrue) that
verifies state equals a fresh TerminalKeyboardCopyModeInputState() or that its
internal fields are in their initial values; locate the testCountedGG function
and add the state-reset assertion after the final resolve(...) assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b1e39b0-c72f-4989-9964-65956eddfb7d
📒 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: 2518d7c483
ℹ️ 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".
| setKeyboardCopyModeActive(false) | ||
| case .startSelection: | ||
| _ = ghostty_surface_select_cursor_cell(surface) | ||
| keyboardCopyModeVisualActive = true |
There was a problem hiding this comment.
In handleKeyboardCopyModeIfNeeded, the .startSelection branch now only sets keyboardCopyModeVisualActive = true, so it assumes the existing 1-cell cursor selection is already at the current copy-mode cursor. However, jump actions like gg/G (.scrollToTop/.scrollToBottom) do not call refreshKeyboardCopyModeViewportRowFromVisibleAnchor, so the stored cursor-cell indicator can be stale; starting visual mode after one of those jumps can extend/copy from the wrong location instead of the newly jumped row.
Useful? React with 👍 / 👎.
1 participant
Summary
scroll_page_fractionalbinding. Ctrl+B/F remain full-page.select_cursor_cellcreates a 1-cell selection as a cursor indicator when entering copy mode. Visual mode (v) tracked separately so the cursor doesn't make every motion extend a selection.gnow waits for next key.ggscrolls to top. SamependingGstate pattern aspendingYankLineforyy. (Supersedes Fix vi mode: require gg to scroll to top #843)Testing
Related
Summary by cubic
Improved vi copy mode with half-page scrolling, a visible cursor indicator, correct gg behavior, and reliable viewport row refresh in non-visual mode.
New Features
Bug Fixes
Written for commit 2518d7c. Summary will update on new commits.
Summary by CodeRabbit