feat: vim-motion preset (--vim-motion, off by default)#147
Merged
Conversation
Introduce ActionScrollCenter, ActionScrollTop, and ActionScrollBottom constants with matching validActions entries and Navigation-section help descriptions. No default single-key bindings — the upcoming vim-motion interceptor will be the only way to reach them unless explicitly mapped by the user. Related to #138
Add two diff-pane helpers for the vim-motion preset: - bottomAlignViewportOnCursor mirrors topAlignViewportOnCursor, flipping the offset by viewport height so the cursor lands on the last visible row. - jumpToLineN moves the diff cursor to a 1-indexed line, clamped to [1, total], and centers the viewport on the new position.
route ActionScrollCenter, ActionScrollTop, and ActionScrollBottom to their corresponding viewport helpers (centerViewportOnCursor, topAlignViewportOnCursor, bottomAlignViewportOnCursor). diff-pane only per plan scope; handleTreeAction is untouched.
Insert m.interceptVimMotion call AFTER handleModalKey and BEFORE keymap.Resolve, guarded by m.modes.vimMotion. Modal-entry paths (search, annotate, overlay) consume keys first so modal textinputs receive digits and letters; the interceptor then preempts normal keymap resolution so vim chords and count prefixes win over single-key bindings. Add 8 integration tests covering the precedence matrix: vim-motion off skips the interceptor; vim-motion on accumulates digits; chord-second guard and pending-reload guard preempt vim; search/annotate/overlay modals preempt vim; non-vim keys fall through to normal dispatch.
Rename clearChordState to clearPendingInputState and reset vim-motion state (count, leader, hint) alongside chord state. Modal entry points (startSearch, startAnnotation, handleOverlayOpen) now cancel any pending vim count or leader so modal sessions never coexist with stale input state.
Add 14 integration tests driving the vim-motion interceptor through Model.Update to exercise the full handleKey pipeline (hint clear, pending-reload, chord-second, modal, interceptor, keymap.Resolve, dispatch). Covers counted motion (5j/5G), leader chords (gg/zz/zt/zb), quit aliases (ZZ/ZQ), leader cancel via esc, unknown-chord hint, count-then-unrelated-key fall-through, and pane-scope rules. Fix a fall-through bug in handleKey uncovered by the new tests: when the vim-motion interceptor clears state (e.g., count dropped after an unrelated key like "5q") and returns handled=false, handleKey now propagates the interceptor's updated model to the standard keymap path instead of discarding it.
Add synthetic Vim motion help section to overlay when --vim-motion is active, listing all 8 preset bindings with hardcoded key strings since these actions have no default keymap entries. Document the preset in README, site docs, and plugin reference docs (byte-identical across Claude and Codex plugins).
All 19 acceptance checks from Task 11 verified: CLI/env/ini parsing, motion behaviors, modal/chord/reload precedence, help overlay section, compose with compact/collapsed, test suite + linter + --dump-config all clean. No code changes required — verification pass over the work of Tasks 1-10.
Add CLAUDE.md Gotchas bullet documenting the vim-motion preset architecture: --vim-motion gate, vimState vs keyState separation, interceptor position in handleKey, diff-pane scope for motion vs pane-agnostic quit aliases. Update stale clearChordState reference in the chord keybindings bullet to clearPendingInputState. Move completed plan to docs/plans/completed/.
- repeatDiffAction: rewrite to loop only cursor moves and sync the viewport once at the end. The previous implementation looped through handleDiffAction, which calls syncViewportToCursor (and therefore renderDiff) on every step. For counts near the 9999 cap on large diffs this produced multi-second hangs; the new shape keeps the O(N) cursor walk but removes the O(L) per-step render cost. - startFileAnnotation: clear pending input state on entry to match startAnnotation. The helper's comment claimed all modal-entry paths call it; file-level annotation was the exception. - interceptVimMotion: document the cmd==nil fall-through invariant that handleKey relies on when propagating the interceptor's model.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “vim-motion” key interception layer to the TUI so vim-style counts + multi-key motions work in the diff pane, while remaining off by default and orthogonal to the existing ctrl/alt chord keybinding system.
Changes:
- Introduces a vim-motion interceptor (
--vim-motion/ config / env) with count prefixes,gg/G/zz/zt/zband quit aliasesZZ/ZQ. - Adds new navigation actions (
scroll_center,scroll_top,scroll_bottom) and wires them into diff navigation + help overlay (via a synthetic “Vim motion” section). - Updates docs and plugin reference copies to document the new preset and option.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/config.go | Adds --vim-motion option (env/config wiring). |
| app/config_test.go | Tests flag/env/config parsing for vim-motion. |
| app/main.go | Threads opts.VimMotion into model config. |
| app/keymap/keymap.go | Adds scroll actions + introduces NormalizeKey helper for non-Latin layout handling outside Resolve. |
| app/keymap/keymap_test.go | Tests new actions + NormalizeKey behavior. |
| app/ui/model.go | Adds vimState, ModelConfig.VimMotion, handleKey interceptor hook, and clearPendingInputState. |
| app/ui/model_test.go | Adds tests for vim-motion config, hint priority, pending-state clearing, and handleKey precedence. |
| app/ui/vimmotion.go | Implements vim-motion state machine (counts/leaders/chords) and repeat cursor motion. |
| app/ui/vimmotion_test.go | Extensive unit + “full flow via Update” coverage for vim-motion behavior (including layout alias cases). |
| app/ui/diffnav.go | Adds bottom-align + jump-to-line helpers; wires new scroll actions into diff action handling. |
| app/ui/diffnav_test.go | Tests bottom-align, jump-to-line clamping/collapsed behavior, and scroll actions. |
| app/ui/handlers.go | Appends synthetic “Vim motion” help section when enabled. |
| app/ui/handlers_test.go | Verifies help section presence/absence + contents. |
| app/ui/view.go | Adds vim hint as lowest-priority transient hint. |
| app/ui/search.go | Clears pending input state on modal entry. |
| app/ui/annotate.go | Clears pending input state on annotation modal entry (incl. file-level annotation). |
| README.md | Documents --vim-motion option and preset bindings. |
| site/docs.html | Mirrors README documentation + adds options entry/anchor. |
| .claude-plugin/skills/revdiff/references/config.md | Adds --vim-motion option to plugin config reference. |
| .claude-plugin/skills/revdiff/references/usage.md | Documents vim-motion preset in plugin usage reference. |
| plugins/codex/skills/revdiff/references/config.md | Mirrors plugin config reference update. |
| plugins/codex/skills/revdiff/references/usage.md | Mirrors plugin usage reference update. |
| docs/ARCHITECTURE.md | Updates chord-dispatch doc to reflect clearPendingInputState. |
| docs/plans/completed/20260423-vim-motion-preset.md | Adds completed implementation plan and rationale. |
| CLAUDE.md | Documents new vim-motion preset + state/ordering details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+289
to
+293
| func (m *Model) bottomAlignViewportOnCursor() { | ||
| cursorY := m.cursorViewportY() | ||
| m.layout.viewport.SetYOffset(max(0, cursorY-m.layout.viewport.Height+1)) | ||
| m.layout.viewport.SetContent(m.renderDiff()) | ||
| } |
There was a problem hiding this comment.
bottomAlignViewportOnCursor sets YOffset before updating viewport content. Because viewport.SetYOffset clamps based on the current content length (see the comment in syncViewportToCursor), this can mis-clamp (or even leave YOffset out of range) when the next renderDiff() call changes total line count (wrap/collapse/annotation changes). Consider setting SetContent(m.renderDiff()) first, then SetYOffset(...) (and ideally applying the same ordering to center/top align helpers used by the new scroll actions).
Comment on lines
+311
to
+316
| m.annot.cursorOnAnnotation = false | ||
| m.nav.diffCursor = n - 1 | ||
| m.adjustCursorIfHidden() | ||
| m.syncTOCActiveSection() | ||
| m.centerViewportOnCursor() | ||
| } |
There was a problem hiding this comment.
jumpToLineN sets nav.diffCursor = n-1 but does not ensure the target is selectable (non-ChangeDivider). Since regular cursor motion skips divider lines, G / <N>G can land the cursor on a divider (especially for G targeting the last line) and leave users unable to act on the intended last diff line. After setting the cursor, consider nudging to the nearest visible non-divider line (similar to the scan logic in moveDiffCursorToEnd / moveDiffCursorDown).
| - **External editor exec (`tea.ExecProcess`)**: long-running external commands (e.g. `$EDITOR` for multi-line annotations in `app/ui/editor.go`) suspend bubbletea and hand over the tty. Capture target state (target line, file-level flag, change type) at spawn time and pass it through the completion message — cursor movement during the exec window otherwise misroutes the result. Temp files are always cleaned up in the completion callback regardless of error path. | ||
| - Chord keybindings (`map ctrl+w>x mark_reviewed`): kitty-style 2-stage chords in `~/.config/revdiff/keybindings`. Leader must be `ctrl+*` or `alt+*` combo (plain letters rejected by design — sidesteps layout-resolve and prefix-conflict issues tracked on issue #138). Storage is flat strings in `Keymap.bindings` (`"ctrl+w>x" → Action`). Chord-prefix lookup is O(1) via lazy `chordPrefixCache` — invalidated by `Bind`/`Unbind` and at end of `Load()` conflict resolution. At load time, if both standalone `ctrl+w` and chord `ctrl+w>x` exist, the standalone is dropped with a warn. Dispatch state lives on `keyState` (chordPending + hint), NOT on `navigationState` — chord state is a key-dispatch concern, not a cursor/scroll concern. `handleKey` chord-second guard runs BEFORE `handleModalKey` (so active search/annotate textinputs cannot swallow the second key); chord-first guard runs AFTER `Resolve()` (only fires when the key has no standalone action, which is guaranteed by Load-time conflict resolution). Modal-entry sites (`startSearch`, `startAnnotation`, `handleOverlayOpen`) call `clearChordState()` to prevent coexistence. `ResolveChord` applies the same Latin layout-resolve fallback as `Resolve` so Cyrillic `ч` resolves a `ctrl+w>x` chord. Chord-second dispatch reaches pane-nav handlers via the shared `dispatchAction(action)` helper — both `handleKey` and `handleChordSecond` call it; `handleDiffAction`/`handleTreeAction` take the resolved action directly (no re-resolve from `msg.String()`). | ||
| - Chord keybindings (`map ctrl+w>x mark_reviewed`): kitty-style 2-stage chords in `~/.config/revdiff/keybindings`. Leader must be `ctrl+*` or `alt+*` combo (plain letters rejected by design — sidesteps layout-resolve and prefix-conflict issues tracked on issue #138). Storage is flat strings in `Keymap.bindings` (`"ctrl+w>x" → Action`). Chord-prefix lookup is O(1) via lazy `chordPrefixCache` — invalidated by `Bind`/`Unbind` and at end of `Load()` conflict resolution. At load time, if both standalone `ctrl+w` and chord `ctrl+w>x` exist, the standalone is dropped with a warn. Dispatch state lives on `keyState` (chordPending + hint), NOT on `navigationState` — chord state is a key-dispatch concern, not a cursor/scroll concern. `handleKey` chord-second guard runs BEFORE `handleModalKey` (so active search/annotate textinputs cannot swallow the second key); chord-first guard runs AFTER `Resolve()` (only fires when the key has no standalone action, which is guaranteed by Load-time conflict resolution). Modal-entry sites (`startSearch`, `startAnnotation`, `handleOverlayOpen`) call `clearPendingInputState()` to prevent coexistence (the helper also resets vim-motion state). `ResolveChord` applies the same Latin layout-resolve fallback as `Resolve` so Cyrillic `ч` resolves a `ctrl+w>x` chord. Chord-second dispatch reaches pane-nav handlers via the shared `dispatchAction(action)` helper — both `handleKey` and `handleChordSecond` call it; `handleDiffAction`/`handleTreeAction` take the resolved action directly (no re-resolve from `msg.String()`). | ||
| - Vim-motion preset (`--vim-motion` / `VIM_MOTION` / `vim-motion = true`): opt-in, off by default. Gated by `m.modes.vimMotion`; when off, the interceptor short-circuits and behavior is byte-identical to the non-vim path. State lives on `vimState` (count, leader, hint) — separate from `keyState`; the two are orthogonal dispatch layers. `interceptVimMotion` in `app/ui/vimmotion.go` runs in `handleKey` AFTER the chord-second guard and `handleModalKey` (modals own keys when active — digits/letters belong to their textinputs) and BEFORE `keymap.Resolve` (so vim counts/leaders preempt normal bindings). Priority list inside the interceptor: pending letter leader → digit accumulation → `G` motion → other count-consumer keys (`j`/`k`) → leader entry (`g`/`z`/`Z`) → fall through. Motion keys (`j`, `k`, `gg`, `G`, `N G`, `zz`/`zt`/`zb`) are diff-pane only — tree pane falls through. `ZZ`/`ZQ` are pane-agnostic. Counts clamped to 9999; `0` alone does NOT start an accumulator (falls through). Scroll action constants (`ActionScrollCenter`/`ActionScrollTop`/`ActionScrollBottom`) have NO default single-key bindings on purpose — the interceptor is the only reachable dispatch path unless the user explicitly maps them. `buildHelpSpec` appends a synthetic "Vim motion" section when `m.modes.vimMotion` is on (hardcoded key strings, since `KeysFor` returns empty for the unbound scroll actions). Composes cleanly with compact/collapsed modes — `jumpToLineN` operates on the currently-visible `diffLines`. |
There was a problem hiding this comment.
This doc line references the env var as VIM_MOTION, but the actual flag wiring uses REVDIFF_VIM_MOTION (see app/config.go). Please update this to avoid misleading contributors/users.
Suggested change
| - Vim-motion preset (`--vim-motion` / `VIM_MOTION` / `vim-motion = true`): opt-in, off by default. Gated by `m.modes.vimMotion`; when off, the interceptor short-circuits and behavior is byte-identical to the non-vim path. State lives on `vimState` (count, leader, hint) — separate from `keyState`; the two are orthogonal dispatch layers. `interceptVimMotion` in `app/ui/vimmotion.go` runs in `handleKey` AFTER the chord-second guard and `handleModalKey` (modals own keys when active — digits/letters belong to their textinputs) and BEFORE `keymap.Resolve` (so vim counts/leaders preempt normal bindings). Priority list inside the interceptor: pending letter leader → digit accumulation → `G` motion → other count-consumer keys (`j`/`k`) → leader entry (`g`/`z`/`Z`) → fall through. Motion keys (`j`, `k`, `gg`, `G`, `N G`, `zz`/`zt`/`zb`) are diff-pane only — tree pane falls through. `ZZ`/`ZQ` are pane-agnostic. Counts clamped to 9999; `0` alone does NOT start an accumulator (falls through). Scroll action constants (`ActionScrollCenter`/`ActionScrollTop`/`ActionScrollBottom`) have NO default single-key bindings on purpose — the interceptor is the only reachable dispatch path unless the user explicitly maps them. `buildHelpSpec` appends a synthetic "Vim motion" section when `m.modes.vimMotion` is on (hardcoded key strings, since `KeysFor` returns empty for the unbound scroll actions). Composes cleanly with compact/collapsed modes — `jumpToLineN` operates on the currently-visible `diffLines`. | |
| - Vim-motion preset (`--vim-motion` / `REVDIFF_VIM_MOTION` / `vim-motion = true`): opt-in, off by default. Gated by `m.modes.vimMotion`; when off, the interceptor short-circuits and behavior is byte-identical to the non-vim path. State lives on `vimState` (count, leader, hint) — separate from `keyState`; the two are orthogonal dispatch layers. `interceptVimMotion` in `app/ui/vimmotion.go` runs in `handleKey` AFTER the chord-second guard and `handleModalKey` (modals own keys when active — digits/letters belong to their textinputs) and BEFORE `keymap.Resolve` (so vim counts/leaders preempt normal bindings). Priority list inside the interceptor: pending letter leader → digit accumulation → `G` motion → other count-consumer keys (`j`/`k`) → leader entry (`g`/`z`/`Z`) → fall through. Motion keys (`j`, `k`, `gg`, `G`, `N G`, `zz`/`zt`/`zb`) are diff-pane only — tree pane falls through. `ZZ`/`ZQ` are pane-agnostic. Counts clamped to 9999; `0` alone does NOT start an accumulator (falls through). Scroll action constants (`ActionScrollCenter`/`ActionScrollTop`/`ActionScrollBottom`) have NO default single-key bindings on purpose — the interceptor is the only reachable dispatch path unless the user explicitly maps them. `buildHelpSpec` appends a synthetic "Vim motion" section when `m.modes.vimMotion` is on (hardcoded key strings, since `KeysFor` returns empty for the unbound scroll actions). Composes cleanly with compact/collapsed modes — `jumpToLineN` operates on the currently-visible `diffLines`. |
| **Config** — one new bool field on the existing options struct in `app/config.go`: | ||
|
|
||
| ```go | ||
| VimMotion bool `long:"vim-motion" env:"VIM_MOTION" description:"enable vim-style motion preset (counts, gg, G, zz/zt/zb, ZZ/ZQ)" ini-name:"vim-motion"` |
There was a problem hiding this comment.
The completed plan still shows the env tag as env:"VIM_MOTION", but the implementation uses REVDIFF_VIM_MOTION (see app/config.go and related docs). Please update the plan text so it matches the code and avoids propagating the wrong env var name.
Suggested change
| VimMotion bool `long:"vim-motion" env:"VIM_MOTION" description:"enable vim-style motion preset (counts, gg, G, zz/zt/zb, ZZ/ZQ)" ini-name:"vim-motion"` | |
| VimMotion bool `long:"vim-motion" env:"REVDIFF_VIM_MOTION" description:"enable vim-style motion preset (counts, gg, G, zz/zt/zb, ZZ/ZQ)" ini-name:"vim-motion"` |
Comment on lines
+147
to
+154
| case keymap.ActionDown: | ||
| for range n { | ||
| m.moveDiffCursorDown() | ||
| } | ||
| case keymap.ActionUp: | ||
| for range n { | ||
| m.moveDiffCursorUp() | ||
| } |
There was a problem hiding this comment.
repeatDiffAction loops up to n (capped at 9999) and calls moveDiffCursorDown/Up each iteration. Those helpers recompute hunks (findHunks()) and check annotations each time, so worst-case runtime becomes O(n * len(diff)) and can still hang on large diffs with large counts. Consider optimizing by precomputing any needed context once (e.g., hunks/annotationSet) and using a cursor-advance helper that doesn’t rescan the whole diff per step, or by computing the target cursor in a single pass and syncing viewport once.
- center/top/bottom viewport align helpers now SetContent before SetYOffset, matching syncViewportToCursor. viewport.SetYOffset clamps to current content length; rendering first avoids mis-clamping when the cursor move mutated render height (wrap/annotation rows). - jumpToLineN nudges off ChangeDivider rows after clamping. With the trailing divider feature shipped on master, G/NG could land the cursor on a divider row where no action applies. Now the cursor slides back (then forward) to the nearest non-divider line. - fix REVDIFF_VIM_MOTION env var name in CLAUDE.md and the completed plan file — the actual go-flags tag in app/config.go is REVDIFF_VIM_MOTION to match the revdiff env-var convention. - repeatDiffAction precomputes hunks once and delegates to new moveDiffCursorDownWithHunks / moveDiffCursorUpWithHunks variants, avoiding the O(N × len(diff)) rescan that would hang the UI on large counts (e.g. 9999j on a 10000-line diff). Addresses Copilot inline findings on #147.
Deploying revdiff with
|
| Latest commit: |
4c70c00
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c5d36940.revdiff.pages.dev |
| Branch Preview URL: | https://vim-motion-preset.revdiff.pages.dev |
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.
Adds an opt-in vim-style motion preset to revdiff. Activated via
--vim-motionCLI flag,VIM_MOTION=trueenv, orvim-motion = truein~/.config/revdiff/config. Off by default.Addresses issue #138 and retroactively covers the scope of the closed PR #63.
Bindings shipped behind the flag
Diff pane (motion + viewport):
<N>j/<N>k— move cursor N lines down / up (1-9999)gg— jump to first lineG/<N>G— jump to last line / goto line Nzz— center viewport on cursorzt/zb— align viewport top / bottomPane-agnostic:
ZZ— quit (alias for existing quit action)ZQ— discard & quitArchitecture
Self-contained vim-motion interceptor in
app/ui/vimmotion.go, orthogonal to the existing ctrl/alt chord engine (PR #143). NewvimStatesub-struct on Model holds the count accumulator, pending letter leader, and hint. Interceptor slots intohandleKeyAFTERhandleModalKeyand BEFOREkeymap.Resolve— modal dispatch preempts vim, so digits/letters typed into active search/annotate textinputs are never hijacked.State-machine rules follow a linear priority list: pending letter leader lookup → digit accumulation → count consumer (j/k/G) → leader entry. Count prefix and letter leader are mutually exclusive by construction; modal-entry paths call
clearPendingInputState()to guarantee no leaked state.Three new action constants:
scroll_center,scroll_top,scroll_bottom. No default single-key bindings — reachable only via the vim-motion interceptor or user keybindings file.Why these restrictions
Motion keys (j/k/gg/G + counts) are diff-pane only. Tree-pane viewport scroll would require adding center/top/bottom-align methods to the sidepane Tree type, saved for a later plan if users ask.
ZZ/ZQremain pane-agnostic since they just dispatch the existing quit actions.yyyank is intentionally out of scope — clipboard + ssh/OSC-52 fallback + scope decisions are a separate concern.Help overlay adds a conditional "Vim motion" section when the flag is on, listing all 8 bindings with the synthesized key strings. Without the flag, no section appears.
Tests: full state-machine matrix (priority 1–5), precedence ordering (vim vs chord-second vs pending-reload vs modal), all leader chords, count edge cases (0-prefix, 9999 cap, non-consumer key fallthrough), esc cancellation, unknown second key, interceptor off when flag is false. Plus integration tests for every full-flow binding.
Docs: README
--vim-motionflag + new keybindings table, site/docs.html mirrored, both plugin reference copies kept byte-identical.Plan: docs/plans/completed/20260423-vim-motion-preset.md.