feat: user configurable tui keybindings#2415
feat: user configurable tui keybindings#2415joshbarrington wants to merge 4 commits intodocker:mainfrom
Conversation
joshbarrington
force-pushed
the
tui-configurable-keybindings
branch
from
f9fd084 to
ac8aadf
Compare
joshbarrington
changed the title
joshbarrington
force-pushed
the
tui-configurable-keybindings
branch
2 times, most recently
from
6d04cff to
dd0d120
Compare
|
/review Any thoughts @krissetto ? |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Good foundation for configurable keybindings. The architecture (centralized KeyMap, defaults + override merge, caching) is sound and the refactoring of tui.go to use the shared KeyMap is a clear improvement.
Blocking items:
- Type naming:
Keybindings→Keybinding(singular for a single entry) - Undocumented behavioral change: The quit key now opens an exit confirmation dialog — this changes existing UX and should be explicitly noted/discussed
- Missing docs: No user-facing documentation for how to configure keybindings
CI: Still running at review time — please confirm tests pass.
Also noting (non-blocking): PR title has a typo — "keybings" → "keybindings".
| keys := core.GetKeys() | ||
| switch { | ||
| case key.Matches(msg, key.NewBinding(key.WithKeys("ctrl+z"))): | ||
| case key.Matches(msg, keys.Quit): |
There was a problem hiding this comment.
Blocking: This adds new behavior beyond configurable keybindings — pressing the quit key now opens an exit confirmation dialog instead of quitting directly (which was the previous ctrl+c behavior via bubbletea's default handling).
This is a meaningful UX change. Before: ctrl+c → immediate quit. After: ctrl+c → confirmation dialog → confirm → quit.
Should this behavioral change be in a separate commit or at minimum called out in the PR description? It's easy to miss during review otherwise.
There was a problem hiding this comment.
@aheritier can you confirm that this indeed does change the current behaviour? I also seem to get the confirmation dialog when building from main branch and pressing ctrl+c.
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
joshbarrington
changed the title
aheritier
added
kind/feat
joshbarrington
force-pushed
the
tui-configurable-keybindings
branch
from
dd0d120 to
9642489
Compare
|
@aheritier this can be re-reviewed 👍 |
2 participants
As referenced here: #1626 and the subsequent closed MR: #1629
First stage implementation for allowing user configurable keybindings.
@krissetto would like to get your thoughts and then we can discuss potential extensions/alternative implementations.