Skip to content

feat: user configurable tui keybindings#2415

Draft
joshbarrington wants to merge 8 commits into
docker:mainfrom
joshbarrington:tui-configurable-keybindings
Draft

feat: user configurable tui keybindings#2415
joshbarrington wants to merge 8 commits into
docker:mainfrom
joshbarrington:tui-configurable-keybindings

Conversation

@joshbarrington
Copy link
Copy Markdown
Contributor

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.

@joshbarrington joshbarrington requested a review from a team as a code owner April 14, 2026 21:43
@joshbarrington joshbarrington force-pushed the tui-configurable-keybindings branch from f9fd084 to ac8aadf Compare April 20, 2026 10:59
@joshbarrington joshbarrington changed the title allow user to set tui keybindings in config file feat: user configurable tui keybings Apr 27, 2026
@joshbarrington joshbarrington force-pushed the tui-configurable-keybindings branch 2 times, most recently from 6d04cff to dd0d120 Compare April 29, 2026 11:32
@aheritier
Copy link
Copy Markdown
Contributor

/review

Any thoughts @krissetto ?

Copy link
Copy Markdown
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Type naming: KeybindingsKeybinding (singular for a single entry)
  2. Undocumented behavioral change: The quit key now opens an exit confirmation dialog — this changes existing UX and should be explicitly noted/discussed
  3. 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".

Comment thread pkg/userconfig/userconfig.go
Comment thread pkg/tui/tui.go Outdated
Comment thread pkg/tui/core/keys.go Outdated
Comment thread pkg/tui/core/keys.go
Comment thread pkg/tui/core/keys.go Outdated
Comment thread pkg/tui/dialog/exit_confirmation.go Outdated
Comment thread pkg/tui/core/keys.go
Comment thread pkg/tui/core/keys_test.go
@docker-agent
Copy link
Copy Markdown

docker-agent Bot commented May 6, 2026

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

@joshbarrington joshbarrington changed the title feat: user configurable tui keybings feat: user configurable tui keybindings May 6, 2026
@aheritier aheritier added kind/feat PR adds a new feature (maps to feat: commit prefix) area/tui For features/issues/fixes related to the TUI priority:medium go Pull requests that update go code and removed priority:medium labels May 6, 2026
@joshbarrington joshbarrington force-pushed the tui-configurable-keybindings branch from dd0d120 to 9642489 Compare May 7, 2026 21:32
@joshbarrington
Copy link
Copy Markdown
Contributor Author

@aheritier this can be re-reviewed 👍

@aheritier
Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

This PR introduces user-configurable TUI keybindings — a clean, well-structured implementation with good test coverage. Three issues were found in the new code:

  1. Data race in ResetKeys() — writes keysOnce without a mutex while GetKeys() reads it concurrently (flagged by Go's race detector, and blocks safe hot-reload use).
  2. Conflict detection not enforced in validateKeys — conflicting keys are logged but still bound to both actions, leading to ambiguous key dispatch.
  3. Elicitation dialog exits without confirmation — the quit key in ElicitationDialog.handleKeyPress calls tea.Quit directly rather than stacking the exit confirmation dialog like every other quit path does.

A low-severity edge case was also noted: if a user maps quit to n/N, the exit confirmation dialog's Yes binding would capture it before the No binding.

Comment thread pkg/tui/core/keys.go Outdated
Comment thread pkg/tui/core/keys.go
Comment thread pkg/tui/dialog/elicitation.go Outdated
@aheritier aheritier removed effort:medium go Pull requests that update go code labels May 12, 2026
@joshbarrington joshbarrington force-pushed the tui-configurable-keybindings branch from 4f08c66 to 038379a Compare May 12, 2026 22:32
@aheritier aheritier marked this pull request as draft May 28, 2026 17:31
@aheritier aheritier added the status/needs-rebase PR has merge conflicts or is out of date with main label May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tui For features/issues/fixes related to the TUI kind/feat PR adds a new feature (maps to feat: commit prefix) status/needs-rebase PR has merge conflicts or is out of date with main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants