Adapt all GUI Vim backends onto shared snapshot traits - #15664
Open
warp-agent-staging[bot] wants to merge 20 commits into
Open
Adapt all GUI Vim backends onto shared snapshot traits#15664warp-agent-staging[bot] wants to merge 20 commits into
warp-agent-staging[bot] wants to merge 20 commits into
Conversation
Extract the duplicated operator/visual/mode-change glue from the code editor and TUI handlers into crates/vim::handler, implemented against a buffer-ops trait. CodeEditorModel and NotebooksEditorModel provide the primitives; views stay thin adapters for registers, search, and LSP. Notebooks now enter Normal mode when the global Vim setting is on, reuse the shared command glue where primitives exist, and no-op unsupported ops.
Contributor
Author
|
This PR was generated with Warp. Comment    |
…uction. Operator support is now false by default so no-op primitives cannot still mutate selection. Code editor allows all operators; notebooks whitelist supported ones; TUI filters at the view because it shares CodeEditorModel. Notebooks enter Normal when Vim is enabled after the view exists.
![warp-factories[bot]](https://avatars.githubusercontent.com/in/2278701?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Overview
Solid refactor: operator/visual/mode glue is consolidated into a VimBufferOps trait shared by the code editor, the TUI prompt, and a genuinely new notebook implementation, backed by thorough characterization and new-feature tests. Net position: request changes, for two non-code-correctness gaps below.
Concerns
- Notebook Vim has no feature flag.
vim_mode_enabled(app/src/notebooks/editor/view.rs:1578) gates purely on the sharedAppEditorSettingstoggle, unlike the code editor'sFeatureFlag::VimCodeEditor(app/src/code/editor/view.rs:357). Any user who already enabled Vim for the code editor gets notebook Vim immediately on merge, with no staged rollout or kill switch. Consider adding a dedicated flag mirroringVimCodeEditor. - This is a user-facing behavioral change (new Vim keybindings/cursor behavior in notebooks) with no screenshots or recording attached; the PR description states the authoring environment couldn't capture the UI. Per this repo's review policy, environment limitations don't exempt user-visible changes from visual proof — please attach a short recording (e.g. a cloud agent with computer use, or a local
./script/run) showing normal-mode navigation, an operator likedd/yy, insert mode, and visual mode in a notebook.
Verdict
Checks: build n/a (no local toolchain in this review environment), tests reported passing by author (81 vim + 209 warp lib), CI pending/incomplete at review time (one "Check CI results" failure the author attributes to a draft-only gather-job quirk — worth reconfirming now that the PR is ready for review), visual proof missing
Found: 0 critical, 1 important, 0 suggestions, 2 nits
Char-cell vim_newline(false) leaves the cursor on the original line. The pre-refactor TUI change_mode called move_right after o; keep that surface quirk so typing after o goes on the new line.
The required backend is three primitives. Motions, operand ranges, operators, visual behavior, and insert-position policy live as shared algorithms over those primitives. Editors keep only irreducible Buffer-aware hooks (syntax comment/indent, smart open-line, visual tails, matching-bracket operand selection).
Toggling Vim off then on sent Escape while already in Normal, and the notebook VimHandler re-dispatched it, looping. Match the code editor: Normal-mode Escape is a no-op. Cover toggle and read-only notebooks.
Code editor, notebook, and TUI handlers now call vim::handler motion helpers instead of backend-local vim_navigate_*/vim_move_* methods. Operand ranges, visual expand, insert-position, and cursor-cap policy live on the snapshot; backends keep snapshot/set_selections/replace_ranges plus mechanical indent/comment/open-line hooks. Notebook vim_buffer.rs drops the duplicated motion clone. Visual keep_selection is stamped on VimEvent at emit time so queued visual motions still extend the origin.
warp-agent-staging
Bot
changed the title
Wire EditorModel to VimBufferOps with 0-based/1-based caret conversion, route motions/operators/mode changes through crates/vim, and delete the now-unused per-editor vim_select_* helpers.
Keep visual origin on vim_visual_tails, skip newlines for space/backspace, preserve goal column on j/k, and always store a trailing newline for linewise yanks.
warp-agent-staging
Bot
changed the title
Restore linewise EOF register newlines, paragraph object ranges, visual tails, charwise yank cursor policy, and G/wrapping quirks so EditorView matches pre-refactor tests without per-command glue.
Navigation, jumps, and visual text objects now live in VimHandler default methods via map_vim_snapshot. Views only implement snapshot apply, view-only intercepts, and edit/undo backends. Also share wrapping skip-vs-count and vertical goal-column, no-op counted notebook r when it would overrun the line, and delete the unused code-editor vim_select helper graph.
Notebook and code-editor snapshot adapters persist a logical Vim character goal column, so j/k restore after a short line without reading SelectionModel.goal_xs as character columns. Remove the unused vim_visual_tails helper graph and selection stash.
TUI Visual used raw inclusive carets as exclusive ranges, so empty Visual dropped the block-cursor character and Visual Line did not cover the line. Share expand_visual_range with operator expansion and paint those ranges. Independent selection/content changes now clear adapter goal columns, while Vim's own selection write is recognized by the heads it just applied.
4 tasks
ViewHandle has no model() method; go through view.update like nearby TUI tests.
Platform typed-character events can arrive as UserTyped even after Escape puts the FSA in Normal. That path inserted hjkl instead of running motions.
UserTyped and VimUserTyped now no-op when a child overlay owns input, matching the pre-unification should_handle_user_input gate.
Create-personal-notebook uses NotebookView, which now enters Normal when switching to edit. The editor paints NORMAL/INSERT/VISUAL so the pane matches other Vim surfaces. A lifecycle test constructs that pane, enables Vim, and checks off→on.
Escape now runs for Visual, Replace, pending Normal commands, and Normal itself. Server-confirmed baton grabs normalize only when the editor was not already editable, so optimistic Insert is kept.
Select-command-at-cursor was an editable Escape binding that outranked VimEscape, so Insert never left. Exclude Vim from those bindings and dispatch Escape through the keybinding matcher in a pane test.
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
Moves Vim navigation, jumps, visual ranges, and text objects into shared
VimHandlerdefaults built on immutable buffer snapshots and selection writes. Terminal input, code editor, notebook, and TUI now reuse the same iterator-based motion logic instead of maintaining editor-specificnavigate_*forwarding graphs.The shared snapshot carries normalized carets and logical goal columns. Each adapter owns only its transaction/selection bridge and irreducible product hooks such as terminal autosuggestions/history, registers, search, LSP, scrolling, and syntax-aware edits. Notebook and code-editor adapters keep logical Vim columns separate from native pixel goals and invalidate them after ordinary edits or cursor changes.
This also enables Vim in editable personal notebooks behind
FeatureFlag::VimNotebookand the existing Vim setting. Notebook edit entry resets the FSA to clean Normal, physical Escape wins over command-selection Escape, overlay-owned typing stays out of the document, and shared/read-only notebooks remain immutable. The feature flag is compiled into default/dev builds but is not enabled in dogfood, preview, or release feature sets until promotion.Tracks CODE-1949.
Computer-use video recordings (1)
Vim keybindings smoke test in Warp notebook: Creating a new personal notebook, then demonstrating Vim NORMAL/INSERT toggling with i/Escape, h/l caret movement, x delete, Visual mode via v+l, slash Find, and the long-short-long line j goal-column motion.
Computer-use screenshots (5)
Editable personal notebook in Vim NORMAL mode after typing 'abcdefghijklmnop' and pressing Escape (status bar bottom-right shows NORMAL)
Visual mode selection via 'v' then 'l' three times (not Shift+Arrow) — status bar shows VISUAL and 'abcd' is highlighted in the notebook line 'abcdefghijklmnop'
After pressing j from end of long line 'abcdefghijklmnop' (column 15), cursor clamps to 'y', the last character of the short line 'xy'
After second j onto the third long line 'ABCDEFGHIJKLMNOP', cursor restored to the original column 15 (character 'P'), demonstrating Vim's goal-column memory rather than staying clamped at the short line's last column
Shared read-only notebook 'vim-verify' remains unchanged after attempting i/TESTEDIT/hjkl/Escape — badge still shows 'Viewing', text still 'hello ll00xdwywpAxyz/world', mode remains NORMAL
Review guide
Start with
crates/vim/src/handler.rsandcrates/vim/src/vim.rsfor the snapshot/default-handler boundary. Then review the three GUI adapters andcrates/warp_tui/src/input/vim.rs; their remaining methods should correspond to backend transactions or product-specific effects. Finally, review notebook lifecycle, keymap, and feature-flag wiring.Remaining explicit
VimHandlerbehaviorLinked Issue
Linear: CODE-1949
Testing
Manually tested the exact final head in a built Warp GUI.
cargo test -p vim --lib: 91 passed.cargo test -p warp --lib vim_handler: 221 passed.Focused code-editor and TUI goal-column regressions passed after the CI correction; notebook, personal-notebook lifecycle, key-dispatch, and TUI range regressions passed during implementation.
./script/format: passed.cargo clippy --locked -p warp --all-targets --tests -- -D warnings: passed.cargo check -p warp_tui --lib: passed before the final correction; the final shared TUI path is covered by the Vim crate and platform CI tests.cargo build --bin warpat3dac5bde: passed; GUI verified Normal/Insert/Escape, motions, operators, Visual, Find overlay ownership, long→short→long goal-column restoration, and read-only behavior.Full
cargo test -p warp_tui --libremains blocked by unrelated missingget_cursor_shapeimplementations in WarpUI test delegates; CI platform test jobs cover the branch.Agent Mode
CHANGELOG-NONE