feat(tui): configurable leader key via keybindings.json#718
Conversation
Add auto-seeded keybindings.json next to config.json for leader prefix and slash-command chords. Reserve Ctrl+N/P for menu nav and Ctrl+G as Esc; move togglePlan default to Ctrl+Y.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds persisted, layered ChangesLeader keybindings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant TUI
User->>CLI: Start interactive TUI
CLI->>Config: Load user and project keybindings
Config-->>CLI: Resolve layered keybindings
CLI->>TUI: Provide resolved keybindings
User->>TUI: Press leader prefix and chord
TUI->>TUI: Execute mapped slash command
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/config/keybindings_file.go`:
- Around line 126-137: Update the keybindings creation flow around the existing
os.Stat check and writeKeybindingsFile so creation uses an atomic
create-if-absent operation, preventing concurrent processes from overwriting an
existing keybindings file. Preserve the current seed/default behavior, handle an
already-created file as a no-op, and add a test covering concurrent creation.
In `@internal/tui/leader_config.go`:
- Around line 44-53: Update the leader-key resolution flow around
leaderKeyConflictsToggle so the fallback selected after a conflict is not itself
reserved or conflicting with any global toggle. Sanitize the conflicting binding
or choose a genuinely available alternative, ensuring leader dispatch cannot
shadow a configured toggle and the existing notice behavior remains accurate.
In `@internal/tui/leader.go`:
- Around line 74-80: Update matchesLeaderKey to accept control-byte
representations of Ctrl-letter leader keys in addition to the resolved
leaderKey.Matcher() result. Preserve the existing keyCtrl(msg, 'x') fallback for
the built-in leader and ensure custom Ctrl-letter leaders also match when
terminals omit ModCtrl.
In `@internal/tui/model.go`:
- Around line 2804-2805: Update the resolved-leader hint returned by the
surrounding model-rendering method to avoid hardcoded “m model” and “p provider”
commands. Derive the displayed examples from m.leaderCommands so remapped or
unbound commands are reflected, or use a generic hint when no suitable commands
are configured; preserve the existing label and cancel/list guidance.
🪄 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: f0d2807f-df56-4f72-b630-2662b81d7289
📒 Files selected for processing (21)
README.mdinternal/cli/app.gointernal/config/keybindings_file.gointernal/config/keybindings_file_test.gointernal/config/types.gointernal/config/writer.gointernal/tui/binding_test.gointernal/tui/commands.gointernal/tui/input_compat.gointernal/tui/input_compat_test.gointernal/tui/keybinding_help.gointernal/tui/keybinding_help_test.gointernal/tui/keybindings.gointernal/tui/leader.gointernal/tui/leader_config.gointernal/tui/leader_config_test.gointernal/tui/leader_test.gointernal/tui/modal_selection_test.gointernal/tui/model.gointernal/tui/options.gointernal/tui/plan_panel.go
Use O_CREATE|O_EXCL so concurrent EnsureKeybindingsFile races cannot overwrite an existing keybindings file via rename.
When a configured leaderKey is rejected, pick a usable fallback (default Ctrl+X, or free it by clearing a remapped toggle that claimed it) so leader dispatch cannot collide with reserved chords or global toggles.
Show remapped/unbound leader examples from leaderCommands instead of hardcoded m/p shortcuts; keep ? list and Esc cancel guidance.
Some terminals emit Ctrl+letter as a C0 code without ModCtrl; accept that form for the default and custom ctrl-letter leader keys.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Took a careful look — this is solid, well-built work. go build/vet/gofmt clean, the config suite passes, and the file handling is genuinely careful: keybindings.json is seeded with O_CREATE|O_EXCL so a concurrent process or an existing user file is never overwritten, a missing file loads as empty (no hard fail), a parse error is surfaced instead of silently resetting, and the defaults←user←project overlay handles ""-to-unassign correctly. The leader sanitize path is thorough too — modifier required, reserved-chord and toggle-conflict rejection, deterministic duplicate-letter resolution, and soft-fail-to-default with user-facing notices rather than a startup crash. No correctness blockers from me.
Two things that are really the maintainer's call, not code issues — flagging so they're conscious decisions:
- Contribution gate. This is a ~1,400-line feature from an external contributor with no linked approved issue. It's clearly a continuation of already-merged work (the leader chords from #699) and it's good work, so I'd lean toward welcoming it — but per the contribution process that's kevin's nod to give, not an auto-yes.
- User-visible default change. The plan toggle moves
Ctrl+P→Ctrl+YsoCtrl+P/Ctrl+Ncan become list navigation (andCtrl+Gan Esc synonym). Sensible and conventional, but it does change an existing default and adds a new auto-seeded config file besideconfig.json— both worth a deliberate yes rather than sliding in unnoticed.
One tiny, optional nit: encodeKeybindingsJSON (keybindings_file.go:222-232) manually sorts the leader map "because maps encode in insertion order" — but encoding/json already sorts map keys alphabetically, so that block is redundant (output is identical either way). Harmless; drop it if you're in the file anyway.
Net: on correctness/safety this is ready — careful, tested, and it fails safe. My only holds are the two product/process calls above, which are the maintainer's to make.
|
hi @lfaoro please rebase to main and fix conflicts |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase this branch and resolve the current conflicts before it can be reviewed for merge.
The PR head798dfa5is no longer mergeable with currentmain: GitHub reportsmergeable: false/mergeable_state: dirty, and the merge has overlapping edits in feature-critical files includinginternal/cli/app.go,internal/config/types.go,internal/config/writer.go,internal/tui/commands.go, andinternal/tui/model.go. This also leaves the collaborator's request to rebase unresolved. Please rebase onto currentmain, resolve the resulting diff, and rerun the relevant tests; the resolved head needs a fresh review. -
[P1] Do not seed the project layer with a full set of overriding defaults.
internal/cli/app.go:898-918
On the first TUI launch in any workspace that has.zero/config.json,EnsureKeybindingsBesideConfigcreates a projectkeybindings.jsoncontaining every slash entry, thenResolveKeybindingsoverlays that generated map after the user's map. The generated""entries actively unbind user mappings and the generated defaults overwrite user remaps, even though the project has not customized keybindings. For example, a user mapping/modeltozis reset tom, and a user binding for/themeis cleared. Seed only an explicitly user-owned layer, or distinguish an absent project layer from an intentional project override. -
[P1] Load the documented standalone project keybindings file.
internal/cli/app.go:897-918
The README advertises<workspace>/.zero/keybindings.jsonas a project overlay, butprojectKBis set only when.zero/config.jsonexists.DefaultResolveOptionsintentionally leavesProjectConfigPathempty without that unrelated file, so a repository that supplies only.zero/keybindings.jsonsilently has its project bindings ignored. Derive and loadProjectKeybindingsPath(workspaceRoot)independently of whether a project config file exists. -
[P2] Make initial keybindings creation atomic with respect to readers.
internal/config/keybindings_file.go:256-278
O_CREATE|O_EXCLprevents replacement, but it exposes the final path before its JSON has been written and closed. A second Zero process can see that path, load an empty/truncated file, and fall back to defaults; interruption or a write failure can leave the partial final file permanently because future ensures treat any existing path as user-owned. Write and finalize a complete temporary file first, then atomically create/link the final name (or otherwise coordinate readers), without reintroducing the overwrite race this code is trying to avoid. -
[P2] Initialize the full slash catalog before every config-writing path can seed the file.
internal/config/writer.go:778-779
DefaultKeybindingsFile(nil)only includes the assigned defaults untilSetKeybindingsPrimarySlashes(tui.PrimarySlashNames())runs, but that registration happens only in interactive TUI startup (internal/cli/app.go:791). Provider/setup/config commands can writeconfig.jsonfirst, which now creates a sibling file missing every unassigned builtin command; because later TUI startup never overwrites an existing file, it remains an incomplete seed. Move the complete catalog into a dependency-safe shared source or pass it to all config writers before they seed. -
[P2] Surface keybindings read and parse failures instead of silently resetting shortcuts.
internal/cli/app.go:910-917
LoadKeybindingsFiledeliberately returns contextual errors for malformed JSON and read failures, but this caller replaces either failed layer with an empty struct and emits no notice. A typo, permission problem, or interrupted seed therefore makes the TUI start normally with default bindings while the user's configured leader/chords silently stop applying. Propagate a visible startup diagnostic (or fail startup with an actionable error) rather than swallowing these failures. -
[P2] Reject leader mapping values that dispatch cannot consume.
internal/tui/leader_config.go:99-110
The resolver accepts any one-rune value other than?, butleaderSecondKeyininternal/tui/leader.go:248-266emits only letters. A mapping such as"/model": "1"is accepted and displayed in the leader help overlay as a usable chord, yet pressing the chord only clears leader-pending and never runs/model. Requireunicode.IsLetterwhile resolving (with a notice) or extend second-key dispatch to the complete accepted character set.
4 participants
Makes the TUI leader prefix and slash-command chords configurable via a sibling
keybindings.jsonfile (notconfig.json), auto-seeded next to config when missing.keybindings.jsonat~/.config/zero/keybindings.json(and project.zero/keybindings.json)config.jsonon write and at TUI start; never overwrites an existing file""leaderKey+leadermap as slash → letter (or""to unassign)leaderKey,/edit, unknown slash, duplicate letters)Reserved chords (hardcoded)
Ctrl+P/Ctrl+NCtrl+GSummary by CodeRabbit
keybindings.json, with support for user and project/workspace overrides.keybindings.jsoninitialization/merging, including seeding sibling keybindings and avoiding overwriting existing edits.