feat(config): add hotbar slot persistence#2873
Conversation
Add durable [[hotbar]] config bindings for slots 1-8, including default bindings when no hotbar config is present. Validate bindings without panicking: skip out-of-range slots, use the last duplicate slot, and preserve unknown actions so future UI layers can show disabled placeholders.
|
Thanks @reidliu41 for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
There was a problem hiding this comment.
Code Review
This pull request introduces customizable 1-8 sidebar hotbar bindings, allowing users to configure custom actions and labels with fallback defaults. The implementation includes validation logic to handle out-of-range slots, duplicate bindings, and unknown actions, supported by new unit tests. Feedback on the changes suggests optimizing the binding resolution loop in crates/config/src/lib.rs to avoid cloning the HotbarBinding struct on every insertion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Some(previous) = by_slot.insert(binding.slot, binding.clone()) { | ||
| warnings.push(HotbarConfigWarning::DuplicateSlot { | ||
| slot: binding.slot, | ||
| previous_action: previous.action, | ||
| replacement_action: binding.action, | ||
| }); | ||
| } |
There was a problem hiding this comment.
To avoid cloning the HotbarBinding struct (which contains heap-allocated Strings) on every single successful insertion, you can move binding directly into by_slot.insert. Since previous is an owned value returned by insert, the borrow on by_slot ends immediately, allowing you to safely retrieve the replacement_action from the map only when a duplicate is actually detected.
let slot = binding.slot;
if let Some(previous) = by_slot.insert(slot, binding) {
warnings.push(HotbarConfigWarning::DuplicateSlot {
slot,
previous_action: previous.action,
replacement_action: by_slot[&slot].action.clone(),
});
}| } => write!( | ||
| f, | ||
| "hotbar slot {slot} was bound to '{previous_action}' more than once; using '{replacement_action}'" | ||
| ), | ||
| Self::UnknownAction { slot, action } => write!( | ||
| f, | ||
| "hotbar slot {slot} references unknown action '{action}'; keeping binding" | ||
| ), |
There was a problem hiding this comment.
The UnknownAction warning fires before the duplicate-slot check, so the message says "keeping binding" even in the scenario where the unknown-action entry is the earlier of two identical slots and therefore gets discarded by the last-wins logic. A user who configures slot 2 with custom.action (unknown) first and mode.plan (known) second will see "keeping binding" for custom.action, yet custom.action won't appear in the final resolved bindings — only the later DuplicateSlot warning explains the loss. Consider rewording to "not rejecting binding" or "binding preserved unless overwritten" to avoid implying a guarantee about the final resolved state.
| replacement_action, | ||
| } => write!( | ||
| f, | ||
| "hotbar slot {slot} was bound to '{previous_action}' more than once; using '{replacement_action}'" |
There was a problem hiding this comment.
DuplicateSlot message reads "was bound to '{previous_action}' more than once" which is grammatically ambiguous — it sounds like previous_action was bound repeatedly rather than the slot being reassigned. A clearer phrasing would be "already bound to '{previous_action}'; replaced by '{replacement_action}'".
| "hotbar slot {slot} was bound to '{previous_action}' more than once; using '{replacement_action}'" | |
| "hotbar slot {slot} already bound to '{previous_action}'; replaced by '{replacement_action}'" |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /// Optional 1-8 hotbar slot bindings (#2064). When absent, future hotbar | ||
| /// UI slices use the built-in defaults from `codewhale_config`. | ||
| #[serde(default)] | ||
| pub hotbar: Option<Vec<codewhale_config::HotbarBindingToml>>, |
There was a problem hiding this comment.
skip_serializing_if missing on TUI hotbar field
ConfigToml.hotbar uses #[serde(default, skip_serializing_if = "Option::is_none")], but the mirroring Config.hotbar field here only has #[serde(default)]. If a TUI Config is ever round-tripped through toml::to_string, the None case may serialize as an explicit null/empty entry depending on the TOML serializer version, adding noise to written config files. Adding skip_serializing_if = "Option::is_none" keeps parity with the codewhale_config type.
| memory: override_cfg.memory.or(base.memory), | ||
| speech: override_cfg.speech.or(base.speech), | ||
| auto: override_cfg.auto.or(base.auto), | ||
| hotbar: override_cfg.hotbar.or(base.hotbar), |
There was a problem hiding this comment.
override_cfg.hotbar.or(base.hotbar) means any project/workspace config that defines even a single [[hotbar]] entry completely discards the user's global hotbar bindings. For layered configs this may be surprising — a user who sets trust.toggle on slot 8 globally will lose it entirely if a project config ships even one hotbar entry. There's no per-slot merging. This follows the same pattern as other Option config fields, but given that hotbar bindings feel user-personal, it may be worth a comment explaining the intentional all-or-nothing semantics.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Adds the config/schema foundation for the Hotbar tracking work.
This implements
[[hotbar]]persistence for slots 1-8 so later UI and key-dispatch slices can render and trigger user-configured hotbar actions instead of hardcoding slot bindings.
Refs #2064
Part of #2061
Scope
[[hotbar]]config parsing for:slotactionlabel[[hotbar]]table is configured.config.example.toml.Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR adds the config/schema foundation for hotbar slot persistence: a new
[[hotbar]]TOML array-of-tables that maps slots 1–8 to action IDs, with validation that skips out-of-range slots, warns on duplicates (last wins), and preserves unknown actions for future UI feedback. The same schema is mirrored into the TUI runtimeConfigand wired intomerge_config.crates/config/src/lib.rs): newHotbarBindingToml,HotbarBinding,HotbarConfigWarning, andHotbarConfigResolutiontypes,resolve_hotbar_bindingsfree function,default_hotbar_bindingsreturning the 8 built-in defaults, all guarded by three tests covering defaults, TOML round-trip, and edge-case validation.crates/tui/src/config.rs): mirrors thehotbarfield intoConfig, delegatesresolve_hotbar_bindingstocodewhale_config, and adds the field tomerge_configwith all-or-nothingOption::orsemantics (any override with even one slot fully replaces the base config's hotbar).config.example.toml): two commented-out[[hotbar]]examples with the full default slot list described in the header comment.Confidence Score: 4/5
Safe to merge; the new hotbar config path is additive and feature-flagged by absence — existing users see no change until they add [[hotbar]] tables.
The implementation is well-structured with thorough tests. The only substantive concern is that merge_config uses all-or-nothing Option::or for hotbar, meaning a project-level config with a single slot silently discards all global hotbar bindings. The UnknownAction warning message can also be misleading when the entry is the earlier of two duplicate-slot entries and gets overwritten. Neither issue affects correctness of the resolved bindings, only observability and merge-time ergonomics.
crates/config/src/lib.rs (warning message clarity) and crates/tui/src/config.rs (all-or-nothing merge semantics, missing skip_serializing_if)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["User TOML config\n[[hotbar]] tables"] -->|"serde deserialize"| B["Option<Vec<HotbarBindingToml>>"] B -->|"None"| C["default_hotbar_bindings()\nslots 1-8 with built-in actions"] B -->|"Some(entries)"| D["Iterate entries"] C --> E["BTreeMap<u8, HotbarBinding>"] D --> F{"slot in 1..=8?"} F -->|"No"| G["SlotOutOfRange warning\nskip entry"] F -->|"Yes"| H{"action in\nknown_action_ids?"} H -->|"No (and known non-empty)"| I["UnknownAction warning\nkeep entry"] H -->|"Yes"| J["insert into BTreeMap"] I --> J J --> K{"slot already\nin map?"} K -->|"Yes"| L["DuplicateSlot warning\nlast entry wins"] K -->|"No"| M["entry stored"] L --> M E --> N["BTreeMap::into_values()\n(sorted by slot)"] M --> N N --> O["HotbarConfigResolution\n{ bindings, warnings }"]Reviews (1): Last reviewed commit: "feat(config): add hotbar slot persistenc..." | Re-trigger Greptile