feat(tui): dispatch user keybindings - #511
Conversation
`keybindings.json` was loaded and `/keybindings` listed its contents, but `KeybindingRegistry::lookup` had no callers — so a user could write a binding, see it listed as active, and have the key do nothing. Worse than missing: it reported success. The registry is now consulted on every keypress. A bound chord runs its command or prompt through the normal submit path, so slash dispatch, queueing and mid-turn behaviour are identical to typing it. Only chords the user actually wrote are dispatched. The built-in defaults in the registry describe chords the hardcoded handler already owns, and routing those through the registry too would run them twice. `ctrl+c` and `esc` cannot be rebound. They are how you get out of a stuck state, including out of a binding that turned out to be a mistake. A bare printable character is not a bindable chord — binding `a` would make the composer unusable. Verified by mutation: with the dispatch call disabled, both end-to-end tests fail. The command test originally asserted the composer was empty, which is true whether or not the binding fires; it now asserts the effect of the bound command, which is the only thing that distinguishes the two.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b5b136704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| app.input = format!("/{}", command.trim_start_matches('/')); | ||
| app.cursor = app.input.len(); | ||
| app.submit(); |
There was a problem hiding this comment.
When the composer contains a partially written prompt, invoking a command binding replaces app.input and submit() then clears it, silently discarding the draft. This also occurs in the prompt and toggle branches; for example, binding ctrl+k to tasks loses anything the user was composing merely to open the tasks pane. Preserve and restore the existing composer around bound actions that do not intentionally submit it.
Useful? React with 👍 / 👎.
| // Shift is already encoded in the character the terminal | ||
| // reports, so it is not repeated as a modifier for letters. | ||
| let lower = c.to_ascii_lowercase().to_string(); | ||
| if mods.intersects(KeyModifiers::CONTROL | KeyModifiers::ALT) { | ||
| lower |
There was a problem hiding this comment.
When a terminal reports Ctrl+Shift+P as Char('P') with the SHIFT modifier, lowercasing the character while suppressing the modifier renders it as ctrl+p. Consequently a configured ctrl+shift+p binding can never fire, and the shifted chord can unexpectedly trigger a ctrl+p binding instead. Include shift+ for modified character events before normalizing the character's case.
Useful? React with 👍 / 👎.
| keybindings: std::sync::Arc::new( | ||
| crate::ui::keybindings::KeybindingRegistry::load(), | ||
| ), |
There was a problem hiding this comment.
After a user edits keybindings.json during a running session, the TUI continues dispatching this startup-cached registry, while /keybindings independently calls KeybindingRegistry::load() in commands/mod.rs and displays the newly edited file. The command therefore reports bindings as loaded even though they remain inactive until restart—the same misleading state this change is intended to eliminate. Make the listing use the active registry, reload the app registry, or clearly require a restart.
Useful? React with 👍 / 👎.
| #[test] | ||
| fn built_in_defaults_are_not_user_defined() { | ||
| let registry = KeybindingRegistry::load(); |
There was a problem hiding this comment.
This test invokes the production loader, so a developer whose real ~/.config/agent-code/keybindings.json overrides ctrl+c, ctrl+d, or ctrl+l gets a failing unit test even though the implementation is correct. from_user_bindings() similarly loads ambient configuration and can affect the new run-loop tests. Construct an in-memory defaults-only registry so the test suite remains independent of machine state.
AGENTS.md reference: AGENTS.md:L65-L65
Useful? React with 👍 / 👎.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…ermetic tests - restore the composer draft around user keybinding dispatch instead of discarding it - keep an explicitly reported SHIFT in character chords (ctrl+shift+p no longer collapses to ctrl+p) - list /keybindings in the TUI from the session-active registry and note that file edits apply on restart - build registry tests on a defaults-only constructor so the machine's real keybindings.json cannot affect them
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 779326d9b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| command_palette: None, | ||
| model_picker: None, | ||
| keybindings: std::sync::Arc::new( | ||
| crate::ui::keybindings::KeybindingRegistry::load(), |
There was a problem hiding this comment.
When tests run under an account that already has a keybindings.json, every App::new reads those real bindings; for example, a local ctrl+p override changes the behavior of existing tests that expect the command palette to open. With more than a hundred test constructions using this constructor, results now depend on host configuration, so startup should inject the loaded registry while test construction uses deterministic defaults.
AGENTS.md reference: AGENTS.md:L63-L67
Useful? React with 👍 / 👎.
|
|
||
| ## Custom keybindings | ||
|
|
||
| Put a `keybindings.json` in your config directory (`~/.config/agent-code/`): |
There was a problem hiding this comment.
On macOS and Windows, following this path creates a file the loader never reads: agent_config_dir() uses ~/Library/Application Support/agent-code on macOS and %APPDATA%\agent-code on Windows (crates/lib/src/config/mod.rs, lines 357–370), not ~/.config/agent-code. Qualify this as the Linux default and document the other platform locations or direct users to a command that prints the resolved path.
Useful? React with 👍 / 👎.
| self.user_defined.insert(key.clone()); | ||
| self.bindings.insert(key, binding); |
There was a problem hiding this comment.
When the file contains ctrl+c or esc, these entries are still stored and /keybindings labels their actions as active user bindings, even though action_for always rejects them. Modified variants are also inconsistent: ctrl+alt+c is accepted here but the earlier is_cancel_chord branch consumes it before user dispatch. Validate or drop every reserved/preempted chord while loading, or explicitly mark it inactive, so the listing does not report a binding that can never run.
Useful? React with 👍 / 👎.
- App construction installs built-in defaults only; the run loop loads the user file once, so tests no longer read machine config - reserved and preempted chords (esc with any modifier, ctrl+c variants without shift) are dropped at load with a warning instead of being listed as active - /keybindings prints the resolved file path; docs give per-platform config locations
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27ad22c266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // dispatch below, which is the point of customization. Reserved | ||
| // chords (Ctrl+C, Esc) are filtered out inside `action_for`, and | ||
| // they are handled above this line anyway. | ||
| if apply_user_keybinding(app, &key) { |
There was a problem hiding this comment.
When a terminal emits KeyEventKind::Repeat for a held shortcut, handle_key accepts it and this call executes the binding again on every repeat. For a prompt action, the first event starts a turn and subsequent repeats enqueue duplicate prompts that are automatically sent later, potentially triggering multiple unintended model calls and costs from one held key. Gate user-defined command/prompt dispatch to Press events while preserving the existing repeat behavior needed by built-in handlers.
Useful? React with 👍 / 👎.
A held key emits Repeat events; dispatching a binding per repeat queued duplicate prompt turns from one hold. Built-in handlers keep their repeat behavior.
…mization # Conflicts: # crates/cli/src/ui/modern/app.rs
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Closes D5-08.
keybindings.jsonwas loaded, and/keybindingsprinted its contents with an "Overrides file:" line — butKeybindingRegistry::lookuphad zero callers.So a user could write a binding, run
/keybindings, see it listed as active, and have the key do nothing at all. That is worse than the feature being absent — it reported success.Note this is a correction to the register row, which described it as "plumbing only, no config loader". The loader exists (
load_keybindings_file,keybindings_path) and so does the listing; what was missing is the one line that consults them on a keypress.What changed
chord_string(code, mods)renders a key event the way the config file spells it (ctrl+k,alt+shift+up,f5).toggleactions map onto the slash commands that own each setting rather than reaching into state directly.Three deliberate limits
Only user-written chords are dispatched. The built-in defaults in the registry (
ctrl+c,ctrl+d,ctrl+l) describe chords the hardcoded handler already owns; routing those through the registry as well would run them twice.ctrl+candescare reserved. They are how you get out of a stuck state — including out of a binding that turned out to be a mistake.a_binding_cannot_steal_ctrl_casserts a hostile binding onctrl+cstill cancels the turn and does not submit its prompt.A bare printable character is not a chord. Binding
awould make the composer unusable, sochord_stringreturnsNoneunless Ctrl or Alt is held.Verified by mutation
With the
apply_user_keybindingcall disabled, both end-to-end tests fail; restoring it passes them.That check earned its keep:
a_user_bound_chord_runs_its_commandoriginally asserted the composer was empty after the keypress — which is true whether or not the binding fires, so it passed with the dispatch disabled. It now asserts the effect of the bound command (/taskstoggling the pane), which is the only thing that distinguishes the two states.Also covered: chord rendering across letters/modifiers/function keys/arrows, plain characters not being chords, defaults not being treated as user-defined, and an unbound chord still reaching the built-in handler (
Ctrl+Pstill opens the palette).577 bin tests pass;
clippy --all-targets -- -D warningsandfmt --checkclean.docs/tui/KEYBINDINGS.mddocuments the file format, the chord syntax, and the reserved chords.