refactor(keymap): own mappings through a slot registry - #505
Merged
Conversation
Every codediff mapping was installed with a bare vim.keymap.set from one of 38 call sites, and cleanup deleted whatever key names happened to be in config at the time. That model could not restore what it overwrote, and it could not remove what it had actually installed. Mappings now go through a per-session registry backed by a global slot arbiter keyed by (bufnr, mode, canonical lhs). Each slot snapshots the pre-existing buffer-local mapping on first claim and hands it back after the last claim is released, so ownership is reference counted and teardown is symmetric with setup. Fixes: - a pre-existing buffer-local mapping (q, and plugin maps such as gitsigns ]c / [c) is restored on close instead of being destroyed (#289, #334, and the keymap half of #394) - ih is released from operator-pending and visual mode, not just normal - the previous file's buffer is detached when the diff switches files - conflict mode no longer deletes the user's do/dp; the view layer withdraws its claim so the original mapping is restored for the merge - two sessions sharing one buffer no longer corrupt each other's snapshot - a mapping replaced by another plugin while codediff is active is left alone: the foreign mapping wins, and is never restored over g? is now derived from the registry rather than a hand-maintained list, which fixes three drifts: gm shown when diff.compute_moves is off, do/dp shown in conflict mode, and the hunk staging keys hidden outside explorer mode. Double click, the explorer auto-open keys and the compact fold wraps are accounted for; the fold wraps opt out of the popup because they are transparent wrappers over native Vim keys. Public config, defaults and behavior are unchanged. A golden matrix fixture pins role x mode x lhs x desc for every session shape and verifies each mapping is reachable through maparg, which catches mappings that are listed but encoded twice (the trap that briefly broke <2-LeftMouse>, <Down> and <Up>). A coverage spec asserts all 115 configured mappings resolve in every shape that provides them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An adversarial review of the registry found seven defects. Four of them already existed on main and were simply invisible before mappings became tracked claims; three were gaps in the new machinery itself. Pre-existing, now fixed: - compact's synced-fold wrappers sit on real diff buffers, so they must be released when the user leaves the tab. They were marked non-suspendable, which is only correct for codediff-owned panel buffers. - keymap setup could only add. Mappings from a previous session shape stayed installed: gm after switching to inline, conflict mappings after leaving a merge, and the old key after the quit binding was reconfigured. Setup passes now run inside a named scope and retire whatever the pass did not re-claim; leaving conflict mode retires the conflict scope outright. - a pane wiped while the session was suspended made resume_diff drop the session without disposing its registry, stranding codediff's mappings on whichever pane survived. TabEnter now validates the panes first, and resume_diff disposes before it orphans anything. Gaps in the new machinery: - the slot stored the configured spelling, so changing mapleader mid-session made the installed key unaddressable and the user's mapping was never restored. The API spelling is now frozen with keytrans at claim time, while the canonical bytes remain the slot identity. - "the foreign mapping wins" did not hold in two cases: deleting the restored mapping while codediff was suspended resurrected it on dispose, and re-mapping the same RHS with different options was overwritten on resume. Absence now counts as displacement when a snapshot exists, and mapping identity compares expr, noremap, silent, nowait, script, replace_keycodes and desc rather than the RHS alone. - owns() reported cached state, so g? could advertise a key another plugin had taken over. Ownership queries now verify against the installed mapping. BufWipeout is wired to drop slot bookkeeping for buffers that no longer exist; forget_buffer previously had no caller outside tests. Adds ten regression tests covering the transitions these defects hid in: mapleader change, foreign delete and options change while suspended, displacement, scope retirement, compact tab switch, layout toggle and reconfiguration. Verified against unmodified main, where all four pre-existing defects reproduce. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ui/view/keymaps.lua carried both jobs: it decided which key binds where, and
it implemented what every key does. At 866 lines, 77% of it was action
bodies, which made the binding rules hard to read and hid a real hazard —
each action closed over tabpage, both buffer numbers and the layout flags,
so a mapping installed for one diff could still be holding a buffer the
session had already replaced.
The action bodies move to ui/view/actions/ unchanged, and take an explicit
context instead of capturing it:
keymaps.lua 866 -> 214 declarations only
actions/hunk.lua 252 hunk lookup, patch building, stage/unstage/
discard, and the ih textobject
actions/panes.lua 156 explorer toggle and focus, open-in-previous-tab
actions/move.lua 147 gm alignment
actions/diffget.lua 118 do / dp
actions/stage.lua 70 toggle_stage, toggle_staged_view
Making the capture explicit is the point: the values an action depends on are
now visible in its signature rather than inherited from whichever setup pass
created it.
The other keymap modules are left alone. conflict/keymaps.lua already
delegates to its own action modules, and the explorer and history panels bind
panel-scoped objects that live and die with the panel, so they do not carry
the same hazard.
Behavior is unchanged: every extracted body is identical to its original
modulo the context parameter, and the golden keymap matrix still matches.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A second adversarial review found two more ways the registry could take a key that was no longer its own, plus a case where releasing one scope destroyed a mapping belonging to another. - installed_is_ours() compared only the callback or right-hand side, so a plugin that re-mapped the same RHS with different options — or reused codediff's own dispatcher — was still treated as ours and deleted on teardown. The slot now records the mapping exactly as Neovim stored it and compares the whole thing, matching what same_map already did elsewhere. - Claims were keyed by owner alone. Two setup passes in one session claiming the same key (a configured `view.toggle_compact = "zo"` alongside compact's own zo wrapper) meant the second replaced the first, and releasing it left the key unmapped instead of revealing the earlier claim. Claim identity is now (owner, scope), so overlapping scopes layer and unwind in order. - Scopes are tracked on a stack rather than a single mutable field, and end_scope takes the scope name so a pass that aborted before closing cannot leave the stack dirty. Nested passes now restore their parent. A claim made after an aborted pass is still attributed to it; that is inherent to an ambient scope and is noted in the code. Adds five regression tests: active remap with changed options, active remap reusing our callback, overlapping scope release, nested scopes, and an aborted pass not growing the stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a spec that follows the reproduction steps from each issue as written, so a future change that reopens one fails by issue number rather than as an unexplained assertion somewhere else: #289 / #334 gitsigns [c and ]c stop working after closing CodeDiff #211 / #224 keymaps not restored after navigating files then quitting #394 ih left mapped in operator-pending and visual mode, conflict keys never removed, and the gf escape Both halves of #289/#334 are covered. A global gitsigns mapping already recovered on main, because deleting codediff's buffer-local shadow reveals it; the buffer-local form gitsigns documents for on_attach did not, and that is the half this refactor fixes. The remaining checks are guard rails for issues fixed earlier — #428 close never calls qall, #412 cross-file hunk navigation stays off by default, #276 close_on_open_in_prev_tab stays false, #202 hunk navigation from the explorer, #322 gf from the explorer, #207 mappings keep nowait — so this work cannot silently reopen them. Verified against unmodified origin/main, where 5 of the 16 checks fail. Bumps the minor version: no public API, config key or default changes, but buffer-local mappings codediff overrides are now handed back on teardown, which is a visible behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
esmuellert
enabled auto-merge
August 1, 2026 18:34
esmuellert
disabled auto-merge
August 1, 2026 18:40
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
1 participant
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.
Summary
codediff installed every mapping with a bare
vim.keymap.setfrom 38 call sites across 9 files, and cleaned up by deleting whatever key names happened to be in config at the time. That model could not restore what it overwrote, and could not remove what it had actually installed.Mappings now go through a per-session registry backed by a global slot arbiter keyed by
(bufnr, mode, canonical lhs). Each slot snapshots the pre-existing buffer-local mapping on first claim and hands it back after the last claim is released, so ownership is reference counted and teardown is symmetric with setup.Closes #289
Closes #334
Closes #394
Reported issues fixed
Each is verified by following the reproduction steps from the issue as written, in
tests/ui/keymap/issue_regressions_spec.lua. The same checks were run against unmodifiedmainas a control:]crestored after close[crestored after closeihreleased from operator-pending and visual modedoA note on #289/#334: a global gitsigns mapping already recovered on
main, because deleting codediff's buffer-local shadow reveals it. The buffer-local form gitsigns documents foron_attachdid not, and that is the half this fixes. Both are now covered by tests.#394 also lists window options (
scrollbind/cursorbind). Those are already moot —scrollbindis now only ever set tofalse, having been replaced by structural scroll sync.Other defects fixed
Found by two adversarial audits and verified to reproduce on
main:gmsurvived a switch to inline, conflict mappings survived leaving a merge, and reconfiguringquitleft both the old and new key boundmapleadermid-session made the installed key unaddressable, so the user's mapping was never restoredview.toggle_compact = "zo", leaving compact removed the configured mapping instead of revealing itg?is generated from the registryThe help popup was a hand-maintained list and had drifted. It now filters candidates through actual ownership, which fixed three inaccuracies and cannot drift again:
gmwas advertised whendiff.compute_movesis off (the default)do/dpwere advertised in conflict mode, where they are not boundauto_open_on_cursorkeys were never listedStructure
ui/view/keymaps.luawent from 866 lines to 214. It now only declares which key binds where; behavior moved unchanged toui/view/actions/, taking an explicit context instead of capturing buffers and layout flags implicitly.Compatibility
No public API change. The config shape, every default key,
string/falsesemantics, the deprecatedkeymaps.explorer.toggle_stagefallback,nowait/noremap/silentdefaults, theCodeDiff*autocmd events and their payloads, and the exportednext_hunk/prev_hunk/next_file/prev_filefunctions are all unchanged.Minor version bump: no contract break, but buffer-local mappings codediff overrides are now handed back on teardown, which users will notice.
Testing
tests/fixtures/keymap_matrix.txt) pins role × mode × lhs × desc for every session shape and verifies each mapping is reachable throughmaparg, not merely present — that distinction caught a double-encoding bug that broke<2-LeftMouse>,<Down>and<Up>Known limitations
begin_scopeandend_scope, claims made before the next pass are attributed to the aborted scope and retired with it. Inherent to an ambient scope; an aborted setup is already a failed state.Deliberately out of scope
#407 (multiple keys per action), #455/#492 (custom explorer keymaps), #357 (same-LHS collision semantics, now diagnosable through the registry). The registry leaves extension points for each.