feat(keymap): key sequences and binding presets - #164
Merged
Conversation
Build now stores bindings longer than one chord instead of refusing them. Two flat maps carry them: the full canonical sequence to its action, and every proper prefix to one owning action. A trie buys nothing at ~50 actions, and the prefix set is consulted on every keypress whether bound or not, so an O(1) map hit keeps the probe free in the input path. MatchSeq is deliberately tier-agnostic. pane.close is a late-tier action, so binding it to "ctrl+b x" leaves the opening chord in neither tier's chord map; a tier-scoped probe would answer none, the key would fall through to the pane, and the sequence could never complete. The tier split governs exact resolution of single chords only. Sequences never enter the tier chord maps, so a head chord cannot fire on its own. A chord or sequence that is a proper prefix of a longer sequence is refused outright rather than warned about, because the probe would swallow it every time and the tables would be advertising a binding that dispatch can never reach. ConflictUnsupportedSequence is gone. It existed to keep Build honest while the syntax parsed but nothing dispatched it; now that sequences resolve it would warn about bindings that work.
Ctrl+B followed by c now runs a binding. The probe sits between the overlay guard and the early-tier lookup, where every mode that fully owns the keyboard has already returned: dialog, rename, pane-rename, context menu and overlay are inert by ordering alone, and the reconnect screen never reaches handleKey because freezeInput is called unconditionally in Update. Only the sidebar and an active selection sit downstream, so those two are named explicitly. The probe is tier-agnostic. A late-tier action bound to a sequence has an opening chord in neither tier's map, so a tier-scoped probe would let it fall through to the pane and the sequence could never complete. The tier split governs exact resolution of single chords only. A completed sequence sets a local seqAction that the existing lookups resolve through, rather than moving the two dispatch switches into functions. The switches stay byte-identical where they are, so the case order the tier tests pin cannot drift, and the source-scraping arm-coverage test keeps working. A pending sequence outranks a plugin's raw_keys claim on its final chord. This is the one deliberate precedence change and it is scoped to multi-step bindings: a single chord never sets seqAction, so no existing binding moves. Without it, pane.close = "ctrl+b x" is dead on any pane whose plugin claims x. Pressing the prefix twice sends one literal chord to the pane. Panes routinely ssh into hosts running tmux, and without it the inner tmux has no reachable prefix. Esc, a mouse click and a paste all cancel; the status bar shows the pending chords and names a dropped sequence. The optional timeout ships off, and its tick carries a generation so a cancelled sequence cannot clear the one started after it.
The notes-mode key split and notesKeyExempt were the last two dispatch readers comparing a pressed key against raw cfg.Keybindings strings. Both now resolve action IDs through the registry. This is a prerequisite rather than tidying. Once bindings live somewhere other than the [keybindings] table, a whole-string compare against those fields matches only the empty string: Alt+E would stop exiting notes mode and every structural key would stop flushing the editor before it fires, with nothing failing loudly. handleKey no longer reads cfg.Keybindings at all — the local binding is gone. One kbMatches call site is left in the tree, the reconnect resume key, which matches a hardcoded constant and has nothing to resolve. TestModel_NotesKeyExempt_AllowsGlobalShortcuts builds a Model directly and needed the keymap NewModel would have supplied; without it isAction answers false for every key and the test fails on wiring rather than on the exemption list it checks.
Bindings now resolve through stacked layers: the registry's shipped defaults underneath, a selected preset, then user overrides. Higher layers replace lower ones per action — absence inherits, presence replaces, and "" is an explicit unbind rather than a deletion that propagates upward, which is what lets a user reclaim an action a preset unbound. Prefix shadowing is resolved before insertion rather than after. A binding that is a strict prefix of a longer sequence can never fire, so one of the pair has to go: across layers the higher layer wins whichever is shorter, within a layer the shorter is refused. The asymmetry is the point — a user override must be able to reclaim the prefix key as a plain chord, and a length rule would let a preset veto that. Resolving on parsed sequences rather than on spec strings keeps a malformed spec visible to the per-action fallback, and avoids re-serialising bindings through a grammar that cannot express all of them. There is no default.toml. The registry already carries each action's shipped spec and Build already falls back to it, so a file would be a second copy free to drift from the one dispatch uses. The comma key is bindable as "comma". A literal "," separates alternatives and splits the spec before any chord is parsed, so tmux's rename-window binding was otherwise inexpressible. The canonical form stays "," because that is what a real press reports. Tab switching, next/previous tab and the shortcuts dialog are bindable actions now instead of fixed keys. Alt+1..9 leave the hardcoded-key table with them, so a binding that collides with one is an ordinary duplicate rather than a collision with built-in behaviour. They have no [keybindings] fields and get their defaults from the layer underneath the config, so the promotion changes no shipped key.
Bindings move out of config.toml into $QUIL_HOME/bindings.toml, carrying
a preset name, a prefix chord, a sequence timeout and per-action
overrides. config.Save serializes the whole struct, so a preset resolved
into KeybindingsConfig would be frozen the next time any unrelated
setting was edited — that trap is why the table has to leave.
Existing installs migrate on first launch, diffing against the shipped
defaults so an untouched config yields zero overrides. Copying the fields
instead would write 42 overrides and pin that user to today's defaults
permanently, which is the same trap by another route. The write is
O_EXCL, since two clients can attach at once, and it runs on the Config
that Load already returned so the in-memory legacy patches have applied.
A malformed config.toml refuses to migrate: it would resolve to pure
defaults and permanently discard the user's customizations, and the
migration is one-way. A MISSING config.toml is not that case — it is an
ordinary first launch, and refusing it would leave a fresh install
retrying and failing on every launch forever.
A preset supplies its own prefix as the default. Every tmux binding is
written "${prefix} x", so dropping the preset's prefix would expand all
of them against an empty string and drop every one, leaving the preset
apparently inert. A prefix in bindings.toml still wins.
config.Save keeps emitting [keybindings]. Until a release has passed, an
auto-update rollback lands a binary that cannot read bindings.toml, and
stripping the table now would reset that user's whole keymap with no
recovery. Tracked as tech debt for the release that can drop it.
Rewrites the two sections that described the old world: the "parsed but not yet active" note on sequences, and the "coming for tmux users" teaser. Both now document shipped behaviour, including the three things that otherwise only surface as a log warning — a prefix that collides with an inherited chord degrades quietly, presets replace rather than add, and rebinding paste to a single chord costs Windows users F8. Adds the changelog fragment, and a tech-debt entry for stripping [keybindings] from config.Save one release from now. That second half has no reminder in this PR's own diff, which is exactly why it needs a file. Extends the preset chord test to compare against real key presses. The tmux keymap is the only thing binding %, ", &, [ and ?, and a chord bubbletea spells differently is a dead key with the conflict checker green. It parses first and compares canonical forms, because that is what dispatch matches and the only way an aliased spelling can be checked.
internal/keymap is no longer stdlib-only: preset.go imports embed and BurntSushi/toml for the shipped presets. Both CLAUDE.md and the scoped rule still claimed otherwise, and the claim is load-bearing — it is the reason the package can be tested without a Model or a QUIL_HOME, so the replacement states which dependencies exist and which are still excluded rather than just dropping the sentence. Two further claims in the same paragraph had gone stale: that moving to bindings.toml would change nothing else in the TUI, which is not what happened, and that the reserved-key switch still handles alt+1..9, which left it when those became tab.switch_1..9 actions. The rule's paths globs listed individual TUI filenames and so did not match the new test files, including the one the rule's own sequence section is about. It loaded this time only because the diff also touched model.go; a later change to sequence_test.go alone would have skipped it.
A [bindings] table key is arbitrary text from the user's file, and
ExpandPrefix runs before Build filters unknown action IDs, so a raw key
could reach Conflict.String's %s verb. That string is rendered into F1 ->
Shortcuts and the log, and lipgloss measures ANSI as zero cells, so an
escape sequence survived every width budget and reached the terminal
intact — an OSC 52 payload could set the clipboard on one F1 press. This
is the attack validateBaseKey exists to stop, arriving through a field
the chord parser never sees.
Quoted with %q, and ExpandPrefix now leaves an unregistered ID to Build,
which reports it as an unknown action and already quotes. Every Conflict
raised there now carries one of our own ActionID constants.
Also from review:
- A completed sequence bypasses the notes block, so the structural
teardown that block performs is repeated on that path. Without it
pane.split_h = "${prefix} %" — what the tmux preset ships —
restructured the layout with notes still bound to a pane that moved.
- A pending sequence is dropped when the active pane changes under it. A
daemon broadcast can move the active pane with no keypress at all, and
completing the sequence then acted on a pane the user never armed it
in — the hazard the mouse-click cancel already covers, by a route no
input event sees.
- The dropped-sequence flash names the pending prefix, not the chord that
ended it. Ctrl+B is readline's backward-char, so the machine arms on an
ordinary keystroke, and the next character could be a password
character at an ssh prompt.
- A failed migration no longer discards the user's config bindings.
LoadBindings legitimately succeeds with defaults when no bindings.toml
exists, so a write failure silently replaced a customized keymap.
- ConflictShadowed said "shadowed by a longer sequence" while the
cross-layer loser is the longer one, and could name a winner that was
itself dropped later in the same pass.
- An unparseable key drops a pending prefix instead of leaving it armed;
the timeout arms only when something is pending; the flash clears on
the next keypress even in modes that skip the machine; a preset's
sequence_timeout is carried rather than parsed and discarded; and the
temp file in WriteBindings uses O_EXCL so a planted symlink cannot
redirect the write.
A dialog can appear with no keypress at all: MsgPluginError matches a pattern against a pane's PTY output, and the upgrade prompt is driven from a window resize. While one is up the view draws only the dialog, so the pending-sequence indicator is not on screen either — dismissing it left a prefix armed that the user had no way to know about, and the next character typed completed a sequence they never started. Under the tmux preset that character could be x (close pane) or d (quit). The pane-identity check added earlier covers the other half of this, where a broadcast moves the active pane, but not this one: the pane is unchanged across the dialog. Adds a regression guard asserting Conflict.String() emits no control bytes for either route a user-supplied string takes into one — a spec that fails to parse, and a prefix that fails to validate. Conflict.Detail is built from err.Error() on input that failed parsing, so it is the one field that bypasses validateBaseKey by construction; every constructor quotes its input today and nothing else pinned that. Verified by mutation: unquoting one error re-opens the path and fails the test. Also fixes two gofmt violations the field insertions introduced. Neither vet nor the test job checks formatting, so CI was green with both.
A completed sequence bypasses the notes block, so it has to reproduce
that block's handling arm for arm rather than only its structural half.
pane.left and pane.right are the two that bite: in notes mode they switch
focus between the editor and the bound pane, but run through the ordinary
late-tier arm they navigate to a different pane entirely — and the next
workspace broadcast re-syncs the active pane back to the bound one, so
the move silently undoes itself. Under a vim-style prefix keymap
(pane.right = "${prefix} l") that is the normal way to press them.
pane.notes_toggle and app.quit are handled here too, for the same reason.
Adds regression tests for the three behaviours that had none: the focus
switch, the structural teardown with a non-structural control, and the
pane-change and dialog cancels. Verified by mutation — disabling the
focus arms fails the focus test and nothing else.
Corrects the isAction comment, which claimed Esc still closes the context
menu and the overlay. It does for the menu; handleOverlayKey deliberately
forwards Esc to the running tool, so a sequence-bound overlay toggle
cannot close the overlay it opened and the tool's own quit key is the way
out. Stating a mitigation that does not exist is worse than stating the
limitation.
Documents the tmux preset against tmux's own default prefix table: 27 bindings match exactly, 12 have a close analogue on a different key, and 23 have no Quil equivalent. The last group is mostly concept gaps rather than unfinished work — tmux's layout algebra has nothing to point at in a binary split tree, and its buffer stack has nothing to point at when the clipboard holds one buffer — so each section says which it is rather than leaving a reader to guess. Two rows are called out as genuine gaps: last-window and last-pane. Both are plausible bindings that simply have no action behind them yet, and they are the ones most likely to catch tmux muscle memory. Also answers the question the preset raises and nothing else documented: switching keymaps means editing one line of bindings.toml and RESTARTING. bindings.toml is read once at startup, there is no in-app switcher and no hot reload, and a change made while Quil is running silently does nothing. F1 shows the keymap that is actually live, which is the way to confirm a switch took.
The restart requirement was stated only in the tmux comparison, which is
the least likely place anyone looks for it. Someone changing a preset
goes to keybindings.md, sees the one-line example, edits the file, and
watches nothing happen with no indication why.
Both primary pages now say it: bindings.toml is read once at startup,
there is no in-app switcher and no hot reload, and F1 renders the keymap
that is actually live so it doubles as the check that an edit took.
configuration.md already documented this precisely per setting elsewhere
("applies immediately — no restart", "the one Settings row that applies
immediately rather than on next launch"), so the omission also broke that
file's own convention.
Master gained a What's New dialog (#165) that reads a one-line headline out of each fragment's front matter, and the validator refuses a user-facing fragment without one. This branch predates that, so its fragment was valid when written and is not now. The local check passed throughout because the branch carried the older promote-changelog.sh; CI runs master's. Merging master is what surfaced it, which is the argument for merging before trusting a green local gate rather than after.
artyomsv
added a commit
that referenced
this pull request
Sep 6, 2026
## What Commits 28 files that were sitting untracked in the working tree, plus 7 updates to notes already tracked. **`docs/superpowers/` — 17 plan and spec records (2026-08-05 … 2026-08-19)** The tracked record jumps from `2026-08-01` straight to `2026-08-15`. Every one of the missing documents describes work that has since shipped: | Record | Shipped as | |---|---| | worktree-panes stage A + ledger, worktree-owned-panes design | #133 (v1.51.0) | | worktree-panes stage B | #134 | | sidebar-width-control, sidebar-worktree-name | #137 | | sidebar-attention-fixes (+ design) | #137 / #139 | | keybinding-registry-stage-1, keybinding-presets design | #138 | | overlay-lifecycle (+ design) | #153 | | desktop-notifications (+ design) | #154 / #193 | | keybinding-sequences-presets (+ design) | #164 | | alt-screen-replay-detection | #173 (issue #172) | **`.claude/agent-memory/` — 11 new review-agent notes, 7 updated** Same shape as the 33 notes already tracked and the `chore(memory)` commits in #206 / #207. ## Why now Found during a branch and worktree cleanup pass. Two of the memory notes existed only inside a merged worktree that was about to be deleted. ## Risk Documentation only. No file under `cmd/` or `internal/` is touched, so the changelog gate does not apply and the release workflow's denylist skips this entirely — no version bump.
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.
Stage 2 and Stage 3 of the keybinding work, on top of the action registry that shipped in #138.
Ctrl+Bthencnow opens a tab, and a whole keymap can be selected in one line of~/.quil/bindings.toml.What changed
Key sequences. A binding can be several chords pressed in order. The status bar shows the keys typed so far while one is pending;
Esc, a mouse click, and a paste all cancel; a combination bound to nothing says so rather than doing nothing quietly. Pressing the opening chord twice sends one literal chord to the pane, which is what keeps a tmux running inside a Quil pane reachable.bindings.toml. Bindings leaveconfig.tomlfor a file of their own, resolved in three layers — shipped defaults, selected preset, user overrides.config.Saverewrites the whole struct, so a preset resolved intoKeybindingsConfigwould be frozen the first time any unrelated setting was edited; that trap is why the table has to move. Existing installs migrate on first launch, diffing against the defaults so an untouched config yields zero overrides.A tmux preset.
preset = "tmux"givesprefix c,prefix %,prefix ",prefix z,prefix 1–9,prefix dand the rest. Presets replace rather than add, so it costsCtrl+TandCtrl+W; anything the preset does not name keeps its usual key.Twelve new bindable actions.
Alt+1–9were fixed keys no setting could reach; they are ordinary actions now, along with next/previous tab and the shortcuts list.Design notes worth review attention
The sequence probe is tier-agnostic, deliberately.
pane.closeis a late-tier action, so binding it toctrl+b xleaves the opening chord in neither tier's chord map. A tier-scoped probe answers "no match", the key falls through to the pane, and the sequence can never complete however many timesxis pressed. The tier split governs exact resolution of single chords only.The two dispatch switches were left where they are. Running a resolved sequence needs a way to invoke an action, and the obvious move is extracting both
switchblocks into methods. That was tried and abandoned: ~40returnsites would need reshaping, the multi-value ones (return m.toggleFocusForActiveTab()) cannot simply gain a, true, andTestHandleKey_EveryDispatchedActionHasACaseArmscrapeshandleKey's source text for case arms and their position relative to the tier boundary. A completed sequence instead sets a localseqActionthat the existing lookups consult — six small edits, and the case order the tier tests pin cannot move because the cases never move.Shadow resolution runs before insertion, on parsed sequences. A binding that is a strict prefix of a longer one can never fire, so one of the pair has to be dropped. Doing it after insertion leaves the loser's prefixes in the partial-match set, so the dropped binding still swallows its own first chord. Doing it on serialized specs re-splits on
,, which is unreadable once a binding uses the comma key, and hides a malformed spec from the per-action fallback.Cross-layer and intra-layer shadowing resolve differently, on purpose. Across layers the higher layer wins whichever is shorter — a user override has to be able to reclaim the prefix key as a plain chord, and a length rule would let a preset veto that. Within one layer the shorter is refused, because both sides tie on layer and length is then the only unambiguous tie-break.
One deliberate precedence change: a pending sequence outranks a plugin's
raw_keysclaim on its final chord. Scoped to multi-step bindings — a single chord never setsseqAction, so no existing binding moves. Without it,pane.close = "ctrl+b x"is dead on any pane whose plugin claimsx.There is no
presets/default.toml. The registry already carries each action's shipped spec andBuildalready falls back to it, so a file would be a second copy free to drift from the one dispatch uses.The comma key is bindable as
comma. A literal,separates alternatives and splits a spec before any chord is parsed, so tmux's rename-window binding was otherwise inexpressible. The canonical form stays,, because that is what a real key press reports.Behaviour changes users will notice
alt+1–9leave the hardcoded-key table, so a binding that collides with one is reported as an ordinary duplicate rather than a collision with built-in behaviour. Existing conflict messages for those keys change wording.config.toml's[keybindings]table is still written but no longer read.Verification
The tier-agnostic probe was checked by mutation: scoping it to the early tier fails 10 tests, including the two written for it.
Every chord in every shipped preset is validated against a real
tea.KeyPressMsg—%,",&,[,?and the comma alias included. That check lives ininternal/tuibecauseinternal/keymapdoes not import bubbletea and so can never build one.Deliberately out of scope
config.Savestill emits[keybindings]. Until a release has passed, an auto-update rollback lands a binary that cannot readbindings.toml, and stripping the table now would reset that user's whole keymap with no recovery. Tracked intechdebt/3-1-strip-legacy-keybindings-from-save.md, which names the guard the follow-up needs and the test to invert.Post-review changes
[bindings]table key is arbitrary text, andExpandPrefixruns beforeBuildfilters unknown IDs, so a raw key could reach a%sverb and from there F1 → Shortcuts.lipglossmeasures ANSI as zero cells, so an escape sequence survived every width budget — an OSC 52 payload could set the clipboard on one keypress. Quoted, and unregistered IDs are now left toBuild, which reports them as unknown actions and already quotes.pane.split_h = "${prefix} %"restructured the layout with the editor still bound to a pane that moved.Ctrl+Bis readline's backward-char, so the machine arms on ordinary typing and the next character could be part of a password.config.tomlbindings.LoadBindingslegitimately returns defaults when nobindings.tomlexists, so a write failure silently replaced a customized keymap with no indication why.ConflictShadowedno longer claims "shadowed by a longer sequence" in the cross-layer case, where the loser is the longer one, and no longer names a winner that was itself dropped later in the same pass.sequence_timeoutis carried rather than parsed and discarded;WriteBindings' temp file usesO_EXCL..claude/CLAUDE.mdand the scoped rule no longer describeinternal/keymapas stdlib-only, and the rule'spaths:globs now match the new test files.Test coverage added for the one deliberate precedence change (a completed sequence beating a plugin's
raw_keysclaim), verified by mutation: removing the guard fails the new test and only that test, with its control row still passing.Second review round
MsgPluginErrormatches a pattern against a pane's PTY output, and the upgrade prompt is driven from a window resize — and while one is up the view draws only the dialog, so the pending indicator isn't visible either. Dismissing it left a prefix armed with no way to know, and under the tmux preset the next character could bex(close pane) ord(quit). The pane-identity check covers the broadcast route; the pane is unchanged across a dialog.Conflict.String()emits no control bytes, for both routes user text takes into one.Conflict.Detailcomes fromerr.Error()on input that failed to parse, so it bypassesvalidateBaseKeyby construction — every constructor quotes today and nothing pinned it. Verified by mutation.vetnor the test job checks formatting, so CI was green with both present.pane.left/pane.rightswitch editor↔pane focus in notes mode; through the ordinary late-tier arm they navigated to a different pane, and the next broadcast re-synced it back — the move silently undid itself. Under a vim-style prefix keymap that's the normal way to press them.isActioncomment: Esc closes the context menu but not the overlay, which deliberately forwards it to the running tool — so a sequence-bound overlay toggle can't close what it opened, and the tool's own quit key is the way out.