fix(editor): make the Vim z-family scroll, and put zb/z- the way Vim has them - #479
Merged
Conversation
…has them `zz`, `zt`, `zb`, `z.`, `z-` and `z<CR>` have never done anything in Vim mode (#104, #393). Two defects, both upstream, both in monaco-vim 0.4.4. The scroll never happens. All six keys route to one action: scrollToCursor: function (cm, actionArgs) { var charCoords = cm.charCoords(new Pos(lineNum, 0), "local"); var y = charCoords.top; var lineHeight = charCoords.bottom - y; switch (actionArgs.position) { ... } // y becomes a pixel offset cm.moveCurrentLineTo(y); } and the adapter it hands that number to switches on the strings "top" / "center" / "bottom" with no default branch, so a number matches nothing and the call silently returns. The arithmetic is doubly meaningless in this adapter: its `charCoords` returns `{ top: pos.line, left: pos.ch }` — line numbers, not pixels, and with no `bottom` key at all, so `lineHeight` is NaN and `y` is NaN for `center` and `bottom`. Verified in the shipped `dist/index.mjs`, not from the write-up. `zb` and `z-` are also swapped. Vim (`:help scroll-cursor`) pairs each position with a letter form that keeps the cursor column and a punctuation form that moves it to the first non-blank: zt/z<CR> top, zz/z. centre, zb/z- bottom. 0.4.4 gets top and centre right and carries `motion: "moveToFirstNonWhiteSpaceCharacter"` on `zb` instead of `z-`. Nobody could see it while the first defect made the whole family dead — fixing the scroll is what would have made a wrong cursor visible, so both go together. ## Which of the three routes, and why not the other two ROUTE 2, the supported extension point. `VimMode.Vim.defineAction` replaces the action by name — the dispatcher looks it up as `actions[command.action]` at call time — and `Vim.mapCommand` unshifts onto the keymap the dispatcher takes its first full match from. Nothing in `node_modules` is touched, and `installVimScrollCommands` is a no-op that returns false if the API is not the shape it expects, so an upgrade that renames or removes it costs a silent return rather than a throw while the user is switching Vim mode on. ROUTE 1, an upgrade, was rejected because there is nothing to upgrade to. 0.4.4 is the latest release (2025-11-22) and `latest` on npm; upstream master has only dependency bumps since, and `moveCurrentLineTo` there is byte-for-byte the code above. The upstream tracker has no issue for it (#71, "Some Vim scrolling keybindings are not working", is about <C-f> and <C-b>). ROUTE 3, patching the dependency, was therefore never needed. ## Testing `scripts/vimScrollCommands.test.ts` presses the keys. It drives real keystrokes into `Vim.handleKey` on a real `CMAdapter` built from the real monaco-vim package — the same `dist/index.mjs` Vite hands the app — and asks the editor which reveal it was told to perform. Only `monaco-editor` is faked, and only because it cannot be imported under Node; monaco-vim imports it as two ordinary externals, so a module hook answers those two specifiers with the handful of value classes the adapter constructs. Everything downstream — the keymap, the dispatcher, the action registry, `CMAdapter` — is upstream's shipped code. No assertion in the file checks that a source file contains a string; the closest it comes is lifting Editor.svelte's own `import("monaco-vim").then` callback out of the component and running it. Two of the eight tests state what upstream does *un*repaired, so the day an upgrade fixes `scrollToCursor` they go red and say the shim can go — the only way anyone would find out, since a working `zz` looks the same either way. Not covered: that Monaco's `revealRangeInCenter` puts the line where a human would call the centre, and that a count (`10zz`) reaches the right line — upstream's `scrollToCursor` ignores `actionArgs.repeat` and this change does not add it. ## Falsification With the test kept and the two source files reverted to master, 6 of 8 go red. `zz should reveal the cursor line at the center`: + actual - expected + [] - [ { line: 2, position: 'center' } ] `z- should move the cursor to the first non-blank`: `13 !== 5`. The wiring test: `the commands must be in place before the adapter attaches`, `[- 'install', 'initVimMode']`. Two of the six fail on ERR_MODULE_NOT_FOUND rather than on a claim — they are the ones that test the module's own degradation guards, and there is no module to guard. The two that stay green are the drift guards above, which are about upstream and must not move. `npm audit` 0 vulnerabilities, `npm run check` 0 errors, `npm test` 708/708, `cargo test` 157/157. Touches the same file as #471, which rewrites this component's model handling; the two changes do not overlap, but whichever lands second may need a rebase. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The branch was cut before #471 and #474 landed, both of which touch Editor.svelte. The only conflict was two adjacent import lines; both are kept. Nothing else needed resolving: installVimScrollCommands registers on monaco-vim's module-level command tables and initVimMode binds to the editor, neither of which #471's per-tab models touch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
Closed
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.
Fixes #104. Second half of #393.
zz,zt,zb,z.,z-andz<CR>have never done anything in Vim mode. Two defects, both upstream inmonaco-vim0.4.4, and the second one only becomes visible once the first is fixed — so they go together.1. The scroll never happens
All six keys route to one action:
and the adapter it hands that number to is:
No default branch, so a number matches nothing and the call silently returns.
Read out of the shipped
dist/index.mjs, not from the write-up in #393 — and it turns out to be worse than described there. This adapter'scharCoordsreturns{ top: pos.line, left: pos.ch }: line numbers, not pixels, and with nobottomkey at all. SolineHeightisNaN,yisNaNforcenterandbottom, and the pixel arithmetic upstream is doing could not have produced a usable value even if the switch had accepted one.The fix hands
moveCurrentLineToone of the three strings its switch is written for and drops the arithmetic.2.
zbandz-are swappedVim (
:help scroll-cursor) pairs each position with a letter form that keeps the cursor column and a punctuation form that moves it to the first non-blank:ztz<CR>zzz.zbz-0.4.4 gets top and centre right and has bottom backwards — its
zbcarriesmotion: "moveToFirstNonWhiteSpaceCharacter"and itsz-does not. Nobody could see this while defect 1 made the whole family dead; fixing the scroll is exactly what would have made a wrong cursor column visible, which is why it is in this PR and not a follow-up.Which of the three routes
Route 2 — the supported extension point.
VimMode.Vim.defineActionreplaces an action by name (the dispatcher looks it up asactions[command.action]at call time) andVim.mapCommandunshifts onto the keymap the dispatcher takes its first full match from. Both are part of the package's publicVimAPI. Nothing innode_modulesis patched.Route 1 — an upgrade — was rejected because there is nothing to upgrade to. 0.4.4 is
lateston npm (published 2025‑11‑22). Upstream master has had only dependency bumps since, andmoveCurrentLineTothere is byte-for-byte the code above. There is no upstream issue for it either — brijeshb42/monaco-vim#71, "Some Vim scrolling keybindings are not working", is about<C-f>/<C-b>. I will open one upstream separately; it does not block this.Route 3 — patching the dependency — was therefore never needed.
Degrading
installVimScrollCommandsreturnsfalseand changes nothing ifVimMode.Vimis absent, is not an object, or has nodefineAction; ifdefineActionexists butmapCommanddoes not, the scroll is repaired and the swap is left alone. Nothing it does can throw, because it runs while the user is toggling Vim mode on and a throw there takes the editor with it.:mapcleardrops the two remapped keys and handszb/z-back to upstream — a scroll to the right place with the wrong cursor column, not a dead key.The remap is guarded by a
WeakSeton the Vim API object:defineActionoverwrites and is safe to repeat, butmapCommandunshifts onto a module-global keymap, so an install per Vim-mode toggle would grow it for the life of the window.Testing
scripts/vimScrollCommands.test.tspresses the keys. It drives real keystrokes intoVim.handleKeyon a realCMAdapterbuilt from the realmonaco-vimpackage — the samedist/index.mjsVite hands the app — and then asks the editor which reveal it was told to perform.Only
monaco-editoris faked, and only because it cannot be imported under Node. monaco-vim imports it as two ordinary externals rather than bundling it, so amodule.registerHookspair answers those two specifiers with the handful of value classes the adapter actually constructs. Everything downstream — the keymap, the command dispatcher, the action registry,CMAdapteritself — is upstream's own shipped code.No assertion in the file checks that a source file contains a string. The closest it comes is lifting Editor.svelte's own
import("monaco-vim").thencallback out of the component and running it, to establish that the commands are installed before the adapter attaches and that they are installed even on the disposed path.Two of the eight tests state what upstream does unrepaired. They are the drift guard: the day an upgrade fixes
scrollToCursorthese go red and say the shim can go, which is the only way anyone would find out — a workingzzlooks the same either way.What it does not establish: that Monaco's
revealRangeInCenterputs the line where a human would call the centre, that focus and key routing reach Vim mode in the running app, or anything about rendering. It establishes which reveal each key sequence asks the editor for, and where the cursor ends up.Not fixed here: a count (
10zz) still ignores the count. Upstream'sscrollToCursornever readactionArgs.repeat, this change does not add it, and it is a separate behaviour.Falsification
Test kept, both source changes reverted to master — 6 of 8 red.
The two green ones are the drift guards — they are about upstream, and they must not move when our fix is removed. Two of the six red ones fail on
ERR_MODULE_NOT_FOUNDrather than on a claim; those are the pair that test the module's own degradation guards, and with the module deleted there is nothing to guard. Every test that is aboutzzfails on whatzzdid, becauseloadInstaller()falls back to a no-op install rather than importing directly — the same reasoning asliftedinundoHistoryPerTab.ts.Verification
npm audit0 vulnerabilities ·npm run check0 errors, 647 files ·npm test708/708 ·cargo test157/157.Note on #471
This touches
Editor.svelte, which #471 also rewrites (one Monaco model per tab). I read #471's diff before starting; the two changes do not overlap — that one is aboutdocumentOptions,acquireTabModeland the activation effect, this one is two lines inside the Vim-mode effect plus a new util. Whichever lands second may still need a trivial rebase.