Make shortcuts reconfigurable via the config file.#417
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configurable TUI keyboard shortcuts via config.json so users can avoid conflicts with terminal/shell key chords.
Changes:
- Introduce
config.KeyBindingsConfigand wire it through config resolution into TUIOptions. - Add parsing/matching for user-defined keybinding strings and use them in
model.Updatedispatch. - Update the
?help overlay and idle hints to display remapped shortcut labels; add tests for parsing/help substitution.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/sidebar.go | Adds sidebarToggleAllowed() to decouple sidebar toggle eligibility from sidebar content. |
| internal/tui/options.go | Adds Options.KeyBindings to pass resolved bindings into the model. |
| internal/tui/model.go | Uses configurable bindings for several actions; updates hint/notice text to reflect remaps. |
| internal/tui/keybindings.go | New parsing/labeling/matching logic for configurable keybindings. |
| internal/tui/keybinding_help.go | Switches help bindings list to be built per-model so configurable labels can be shown. |
| internal/tui/keybinding_help_test.go | Updates tests to use the new buildKeybindingGroups() method. |
| internal/tui/binding_test.go | Adds tests for parsing, matching, and config→binding pipeline. |
| internal/config/types.go | Defines KeyBindingsConfig and includes it in FileConfig/resolved config types. |
| internal/config/resolver.go | Merges/propagates KeyBindingsConfig through resolution and overrides. |
| internal/cli/app.go | Passes resolved keybindings into TUI options for interactive runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case keyCtrl(msg, 'c'): | ||
| return m.handleCtrlC() | ||
| case keyCtrl(msg, 'o'): | ||
| case m.keyMatch(m.keyBindings.toggleDetailed, msg, func(tea.KeyMsg) bool { return keyCtrl(msg, 'o') }): |
| // ctrl+letter fast path — the terminal may report this as a control | ||
| // character code (e.g. ctrl+o → 0x0F), so use keyCtrl which handles | ||
| // both the code and ModCtrl representations. | ||
| if mod == tea.ModCtrl && p.code >= 'a' && p.code <= 'z' { | ||
| return func(msg tea.KeyMsg) bool { | ||
| return keyCtrl(msg, p.code) | ||
| } | ||
| } | ||
|
|
||
| if p.text != "" { | ||
| return func(msg tea.KeyMsg) bool { | ||
| return msg.Key().Text == p.text && msg.Key().Mod.Contains(mod) | ||
| } | ||
| } | ||
|
|
||
| return func(msg tea.KeyMsg) bool { | ||
| return msg.Key().Code == p.code && msg.Key().Mod.Contains(mod) | ||
| } |
| if p.ctrl && len(keyPart) == 1 && keyPart[0] >= 'a' && keyPart[0] <= 'z' { | ||
| // ctrl+<letter> — store the code so the match is exact | ||
| p.code = []rune(keyPart)[0] | ||
| p.text = "" | ||
| return p | ||
| } |
| // Single character, any modifier context | ||
| if len(keyPart) == 1 { | ||
| p.code = []rune(keyPart)[0] | ||
| } |
| if m.mouseReleased { | ||
| return m.appendSystemNotice("Mouse released — drag to select and copy text. Press Ctrl+E again to re-enable mouse interaction (clicks, right-click paste)."), nil | ||
| mouseKey := labelOr(m.keyBindings.toggleMouse, "Ctrl+E") | ||
| return m.appendSystemNotice(fmt.Sprintf("Mouse released — drag to select and copy text. Press %s again to re-enable mouse interaction (clicks, right-click paste).", mouseKey)), nil | ||
| } |
| case m.keyMatch(m.keyBindings.toggleSidebar, msg, func(tea.KeyMsg) bool { return keyCtrl(msg, 'b') }): | ||
| // Ctrl+B / Ctrl+U collapses / restores the right context sidebar. Only acts when | ||
| // the sidebar would otherwise be on screen (managed mode, wide enough, | ||
| // real conversation) so it's a no-op — not a confusing notice — on the | ||
| // home screen or a narrow terminal. Hiding reflows the chat to full |
WalkthroughAdds configurable TUI keybindings: new ChangesConfigurable Keybindings Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant keyMatch
participant parsedBinding
User->>Model: key press (e.g. Option+O)
Model->>keyMatch: keyMatch(configuredBinding, defaultFn, msg)
keyMatch->>parsedBinding: isZero check
alt binding configured
parsedBinding-->>keyMatch: Matcher()(msg) result
else binding unset
keyMatch->>Model: fall back to defaultFn(msg)
end
keyMatch-->>Model: matched true/false
Model->>Model: dispatch action (toggleDetailed, toggleMouse, etc.)
Possibly related PRs
Suggested reviewers: Nothing broken, nothing red — just Ctrl+B now answers to a config file instead. Reviewers, do check the empty-binding fallback paths and the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
1293-1307: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSidebar toggle still gated by
sidebarAvailable(), not the newsidebarToggleAllowed().The PR added
sidebarToggleAllowed()insidebar.gospecifically so Ctrl+B can toggle the show/hide preference even when the sidebar is auto-hidden due to no content (sidebarHasContent()is false). But this handler still callsm.sidebarAvailable(), which includes that content check — so Ctrl+B remains a no-op whenever there are no agents/plan yet, exactly the behaviorsidebarToggleAllowedwas meant to fix. This also explains why golangci-lint flagssidebarToggleAllowedas unused ininternal/tui/sidebar.go.🐛 Proposed fix
- if m.noBlockingModal() && m.sidebarAvailable() { + if m.noBlockingModal() && m.sidebarToggleAllowed() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 1293 - 1307, Ctrl+B is still using the stricter sidebar availability check instead of the new toggle- अनुमति helper, so the preference cannot be toggled when the sidebar is auto-hidden for empty content. Update the key handler in m.keyMatch(m.keyBindings.toggleSidebar, ...) to use m.sidebarToggleAllowed() rather than m.sidebarAvailable(), keeping the existing noBlockingModal() guard and hide/show behavior intact. This will let the new helper in sidebar.go be exercised and remove the unused-symbol lint issue.Source: Linters/SAST tools
♻️ Duplicate comments (1)
internal/tui/sidebar.go (1)
50-77: 🎯 Functional Correctness | 🟠 MajorUnused function — not yet wired into the toggle-sidebar key handler.
golangci-lint flags this as unused. Root cause and fix are noted in
internal/tui/model.go'stoggleSidebarcase, which still callssidebarAvailable()instead of this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/sidebar.go` around lines 50 - 77, The new sidebarToggleAllowed helper is currently unused because the toggle-sidebar key handler still uses sidebarAvailable. Update the toggleSidebar case in model.go to call sidebarToggleAllowed instead, so the keybinding follows the intended allow/deny rules while preserving the existing render-time sidebarAvailable checks. Keep the change scoped to the toggle path and use the existing sidebarToggleAllowed and sidebarAvailable symbols to ensure the wiring is correct.Source: Linters/SAST tools
🧹 Nitpick comments (4)
internal/config/resolver.go (1)
707-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd merge-precedence tests for keybindings.
Logic is correct, but
mergeKeyBindingsis now invoked from four different layers (user, project, provider-command viamergeConfig, and CLI overrides). This kind of layered "last non-empty wins" merge is exactly where precedence bugs sneak in silently (e.g., project config unintentionally clobbering an intended override). Worth a couple of table-driven cases inresolver_test.goasserting override > project > user precedence for at least one field (e.g.ToggleSidebar).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/resolver.go` around lines 707 - 724, Add table-driven merge-precedence tests for keybindings around mergeKeyBindings/mergeConfig in resolver_test.go, since this path is used by user, project, provider-command, and CLI layers. Cover at least one representative field such as ToggleSidebar and assert last non-empty wins with override > project > user precedence. Make the cases explicit so a project-level value cannot accidentally clobber a higher-priority override.internal/tui/keybindings.go (2)
251-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
builtinBindings— it's dead code and fails lint.The var is all-zero and, per its own comment, is never matched through the
Matcherpath. golangci-lint flags it as unused, which will break CI. Remove it unless you intend to wire actual default matchers here.♻️ Proposed removal
-// builtinBindings holds the default hardcoded bindings. keybinding config -// overrides the built-in on a per-action basis when the field is non-empty. -var builtinBindings = keyBindings{ - // All zero: the model.go dispatch uses hardcoded key-case comparisons. - // These are the canonical defaults and are never matched through the - // parsedBinding.Matcher path — the config-override path is separate. -} -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/keybindings.go` around lines 251 - 257, Remove the dead `builtinBindings` variable from `keybindings.go`; it is an all-zero `keyBindings` value that is never used by the `Matcher` path and is causing the unused-variable lint failure. Keep the existing hardcoded dispatch behavior in `model.go` and only retain default bindings here if you plan to actually wire them into `parsedBinding.Matcher` and reference `builtinBindings` from the keybinding lookup flow.Source: Linters/SAST tools
203-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent fallback on unrecognized keys hides config typos.
Anything not matched (e.g.
f5,ins, or a typo likectlr+o) falls through withcode == 0, soresolveKeyBindingstreats it as "use default" with zero feedback. Users will think their remap took effect. Consider surfacing invalid keybinding strings (log/warn at resolve time) so misconfiguration is visible.Want me to sketch a validation path that reports unparseable bindings?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/keybindings.go` around lines 203 - 214, The keybinding parser in parseKeyBinding silently returns a zero code for unrecognized inputs, which makes resolveKeyBindings treat invalid remaps as defaults without any feedback. Update parseKeyBinding and/or resolveKeyBindings to detect unparseable key strings (for example f5, ins, or typos like ctlr+o) and emit a warning or error when a binding cannot be resolved, using the existing keybinding resolution path to report the invalid entry instead of silently falling back.internal/tui/binding_test.go (1)
64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test can't fail — it only logs.
TestOptionOMatchesComposedCharactershas no assertion path; whether the matcher returns true or false it justt.Logfs (and the log only fires on thetruebranch). It documents macOS compose behavior but guards nothing. Either assert the intended behavior forø(match or not-match) or convert it to a comment so it isn't mistaken for coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/binding_test.go` around lines 64 - 73, TestOptionOMatchesComposedCharacters currently has no assertion and can never fail, so it does not validate matcher behavior. Update the test to use matcher against the tea.KeyPressMsg for ø with an explicit assertion for the intended outcome, or remove the test logic and leave only a comment if it is meant to be documentation. Use TestOptionOMatchesComposedCharacters, parseBinding, and matcher to locate the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/keybindings.go`:
- Around line 124-132: The keybinding matcher in the shortcut predicate uses
Mod.Contains(mod), which allows modifier supersets to match and can trigger
unintended bindings. Update the anonymous functions returned from the text/code
branches in the keybinding logic to require exact modifier equality against the
configured mod, using the existing p.text and p.code checks as-is but replacing
the modifier containment test with an exact match.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 1293-1307: Ctrl+B is still using the stricter sidebar availability
check instead of the new toggle- अनुमति helper, so the preference cannot be
toggled when the sidebar is auto-hidden for empty content. Update the key
handler in m.keyMatch(m.keyBindings.toggleSidebar, ...) to use
m.sidebarToggleAllowed() rather than m.sidebarAvailable(), keeping the existing
noBlockingModal() guard and hide/show behavior intact. This will let the new
helper in sidebar.go be exercised and remove the unused-symbol lint issue.
---
Duplicate comments:
In `@internal/tui/sidebar.go`:
- Around line 50-77: The new sidebarToggleAllowed helper is currently unused
because the toggle-sidebar key handler still uses sidebarAvailable. Update the
toggleSidebar case in model.go to call sidebarToggleAllowed instead, so the
keybinding follows the intended allow/deny rules while preserving the existing
render-time sidebarAvailable checks. Keep the change scoped to the toggle path
and use the existing sidebarToggleAllowed and sidebarAvailable symbols to ensure
the wiring is correct.
---
Nitpick comments:
In `@internal/config/resolver.go`:
- Around line 707-724: Add table-driven merge-precedence tests for keybindings
around mergeKeyBindings/mergeConfig in resolver_test.go, since this path is used
by user, project, provider-command, and CLI layers. Cover at least one
representative field such as ToggleSidebar and assert last non-empty wins with
override > project > user precedence. Make the cases explicit so a project-level
value cannot accidentally clobber a higher-priority override.
In `@internal/tui/binding_test.go`:
- Around line 64-73: TestOptionOMatchesComposedCharacters currently has no
assertion and can never fail, so it does not validate matcher behavior. Update
the test to use matcher against the tea.KeyPressMsg for ø with an explicit
assertion for the intended outcome, or remove the test logic and leave only a
comment if it is meant to be documentation. Use
TestOptionOMatchesComposedCharacters, parseBinding, and matcher to locate the
check.
In `@internal/tui/keybindings.go`:
- Around line 251-257: Remove the dead `builtinBindings` variable from
`keybindings.go`; it is an all-zero `keyBindings` value that is never used by
the `Matcher` path and is causing the unused-variable lint failure. Keep the
existing hardcoded dispatch behavior in `model.go` and only retain default
bindings here if you plan to actually wire them into `parsedBinding.Matcher` and
reference `builtinBindings` from the keybinding lookup flow.
- Around line 203-214: The keybinding parser in parseKeyBinding silently returns
a zero code for unrecognized inputs, which makes resolveKeyBindings treat
invalid remaps as defaults without any feedback. Update parseKeyBinding and/or
resolveKeyBindings to detect unparseable key strings (for example f5, ins, or
typos like ctlr+o) and emit a warning or error when a binding cannot be
resolved, using the existing keybinding resolution path to report the invalid
entry instead of silently falling back.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8d8eb71-9291-4d24-becd-38223ba09735
📒 Files selected for processing (10)
internal/cli/app.gointernal/config/resolver.gointernal/config/types.gointernal/tui/binding_test.gointernal/tui/keybinding_help.gointernal/tui/keybinding_help_test.gointernal/tui/keybindings.gointernal/tui/model.gointernal/tui/options.gointernal/tui/sidebar.go
| if p.text != "" { | ||
| return func(msg tea.KeyMsg) bool { | ||
| return msg.Key().Text == p.text && msg.Key().Mod.Contains(mod) | ||
| } | ||
| } | ||
|
|
||
| return func(msg tea.KeyMsg) bool { | ||
| return msg.Key().Code == p.code && msg.Key().Mod.Contains(mod) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
charm bubbletea v2 KeyMod Contains method semantics
💡 Result:
In Bubble Tea v2, the KeyMod type—which represents keyboard modifier states such as Ctrl, Alt, and Shift—is implemented as a bitmask (an alias for a type from the underlying input handling library) [1]. While some legacy or pattern-based documentation might suggest checking modifiers using bitwise operations (e.g., key.Mod & tea.ModCtrl != 0) [2][3], the official v2 upgrade guide specifically references the Contains method for modifier checking [4]. The KeyMod type provides a Contains method that accepts a specific modifier constant to determine if that modifier is active [4]. Example usage: if msg.Mod.Contains(tea.ModAlt) { // Alt key was held } The v2 update significantly changed key handling [4][5]. Modifiers, which were previously separate boolean fields (e.g., msg.Alt), have been consolidated into the msg.Mod field [4]. You should use msg.Mod.Contains() to verify the presence of specific modifiers like tea.ModShift, tea.ModAlt, tea.ModCtrl, or tea.ModMeta [4][1]. For simple key combinations, you can also use the msg.String() method (which returns strings like "ctrl+c" or "shift+enter") or msg.Keystroke() for more precise modifier-inclusive string representations [4][5].
Citations:
- 1: https://github.com/charmbracelet/bubbletea/blob/main/mod.go
- 2: https://charmbracelet-bubbletea-43.mintlify.app/guides/input-handling
- 3: https://www.mintlify.com/charmbracelet/bubbletea/guides/input-handling
- 4: https://github.com/charmbracelet/bubbletea/blob/main/UPGRADE_GUIDE_V2.md
- 5: https://github.com/charmbracelet/bubbletea/releases/tag/v2.0.0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline internal/tui/keybindings.go --view expanded || true
printf '\n== relevant lines ==\n'
sed -n '1,220p' internal/tui/keybindings.go | cat -n
printf '\n== dependency references ==\n'
rg -n "bubbletea|tea\.Mod|KeyMod|Contains\(" go.mod go.sum internal -S || true
printf '\n== local tests/usages for key matching ==\n'
rg -n "msg\.Key\(\)\.(Text|Code).*Contains|Contains\(mod\)" internal -S || trueRepository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== keybindings slice ==\n'
sed -n '100,170p' internal/tui/keybindings.go | cat -n
printf '\n== go.mod bubbletea lines ==\n'
rg -n 'bubbletea|charmbracelet' go.mod go.sum -S
printf '\n== any local keybinding tests/usages ==\n'
rg -n 'KeyMsg|KeyMod|Contains\(mod\)|msg\.Key\(\)\.(Text|Code)' internal -SRepository: Gitlawb/zero
Length of output: 8633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Bubble Tea module dir ==\n'
go list -f '{{.Dir}}' charm.land/bubbletea/v2 2>/dev/null || true
printf '\n== Bubble Tea Contains source if cached ==\n'
moddir="$(go list -f '{{.Dir}}' charm.land/bubbletea/v2 2>/dev/null || true)"
if [ -n "$moddir" ] && [ -f "$moddir/mod.go" ]; then
rg -n 'func .*Contains|type KeyMod|ModAlt|ModShift|ModCtrl' "$moddir/mod.go" -n -S
sed -n '1,220p' "$moddir/mod.go" | cat -n
fi
printf '\n== binding tests ==\n'
sed -n '1,260p' internal/tui/binding_test.go | cat -n
printf '\n== keybindings matcher-related docs ==\n'
sed -n '80,150p' internal/tui/keybindings.go | cat -nRepository: Gitlawb/zero
Length of output: 9479
Exact-match the configured modifiers here. Mod.Contains(mod) accepts supersets, so an alt+o binding can also fire on ctrl+alt+o. That widens the shortcut surface and can reintroduce collisions; use == mod for these branches.
🔒️ Proposed exact-match
if p.text != "" {
return func(msg tea.KeyMsg) bool {
- return msg.Key().Text == p.text && msg.Key().Mod.Contains(mod)
+ return msg.Key().Text == p.text && msg.Key().Mod == mod
}
}
return func(msg tea.KeyMsg) bool {
- return msg.Key().Code == p.code && msg.Key().Mod.Contains(mod)
+ return msg.Key().Code == p.code && msg.Key().Mod == mod
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if p.text != "" { | |
| return func(msg tea.KeyMsg) bool { | |
| return msg.Key().Text == p.text && msg.Key().Mod.Contains(mod) | |
| } | |
| } | |
| return func(msg tea.KeyMsg) bool { | |
| return msg.Key().Code == p.code && msg.Key().Mod.Contains(mod) | |
| } | |
| if p.text != "" { | |
| return func(msg tea.KeyMsg) bool { | |
| return msg.Key().Text == p.text && msg.Key().Mod == mod | |
| } | |
| } | |
| return func(msg tea.KeyMsg) bool { | |
| return msg.Key().Code == p.code && msg.Key().Mod == mod | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/keybindings.go` around lines 124 - 132, The keybinding matcher
in the shortcut predicate uses Mod.Contains(mod), which allows modifier
supersets to match and can trigger unintended bindings. Update the anonymous
functions returned from the text/code branches in the keybinding logic to
require exact modifier equality against the configured mod, using the existing
p.text and p.code checks as-is but replacing the modifier containment test with
an exact match.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Nice feature, and the defaults are regression-safe — with no custom config every binding falls through to the identical hardcoded chord, and malformed config fails safe (no panics). Two things block merge:
1. CI will fail on the gofmt gate. internal/tui/model.go, keybindings.go, and binding_test.go aren't gofmt-clean (the CI "Check formatting" step runs gofmt -l . and exits non-zero on any output). Please run gofmt -w.
2. CodeRabbit's change-request is valid and unaddressed. The Ctrl+B handler still gates on m.sidebarAvailable() rather than the new m.sidebarToggleAllowed(), which is never called — dead code — so the toggle-while-auto-hidden feature this PR adds doesn't actually work.
Non-blocking correctness gaps for configured (non-default) keys: Mod.Contains matches supersets (a configured alt+o also fires on alt+shift+o; a plain-key binding fires while modifiers are held) — consider exact ==; and single-char parsing keys on byte length, so a 2-byte UTF-8 key (e.g. é) won't bind. builtinBindings is also unused. Defaults are unaffected by all of these.
Summary
Allows to configure keyboard shortcuts mapping to prevent conflicts with terminal/shell.
Example
config.jsonLinked issue
Fixes #
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
New Features
Bug Fixes