Skip to content

feat(tui): hide Hotbar until explicit opt-in (#3807)#3815

Merged
Hmbown merged 1 commit into
mainfrom
codex/0866-3807-hotbar-hidden-default
Jun 30, 2026
Merged

feat(tui): hide Hotbar until explicit opt-in (#3807)#3815
Hmbown merged 1 commit into
mainfrom
codex/0866-3807-hotbar-hidden-default

Conversation

@Hmbown

@Hmbown Hmbown commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Closes #3807.

Problem

A fresh config with no hotbar key still rendered the built-in default slots. For v0.8.66, Hotbar should stay available but hidden until a user explicitly opts in.

Change

  • Treat an absent hotbar key as no bindings.
  • Preserve explicit hotbar = [] and explicit [[hotbar]] configs.
  • Make /hotbar on persist the recommended defaults explicitly instead of deleting the config key.
  • Keep /hotbar off writing hotbar = [].
  • Update config/sidebar/UI coverage to match the opt-in contract.

This was split from origin/codex/3799-inline-approval so #3807 can be reviewed independently from the approval-layout fix.

Verification

  • cargo fmt
  • cargo test -p codewhale-config --locked hotbar
  • cargo test -p codewhale-tui --bin codewhale-tui --locked hotbar
  • git diff --check

A fresh install (no `hotbar` key) showed the eight default Hotbar slots,
because `resolve_hotbar_bindings` mapped an absent key to the built-in
defaults. The Hotbar is now an opt-in personalization feature, so a fresh
config must render no panel.

- config: `resolve_hotbar_bindings(None, ...)` now resolves to no bindings
  instead of the defaults, so the panel, the Alt+1-8 dispatch, and the
  setup wizard are all empty until the user opts in. An explicit
  `[[hotbar]]` config (configured users) and `hotbar = []` (already
  disabled) are byte-for-byte unaffected — only the meaning of an absent
  key changes, so no settings migration is needed.
- Add `default_hotbar_bindings_toml()` so `/hotbar on` can persist the
  recommended slots explicitly, keeping `DEFAULT_HOTBAR_ACTIONS` the
  single source of truth.
- `/hotbar on` (`restore_hotbar_defaults`) now writes the explicit default
  bindings instead of deleting the key (which would now hide the Hotbar).
  `/hotbar off` still writes `hotbar = []`; `/hotbar` still opens the
  wizard. Removed the now-unused `remove_hotbar_from_config` and its tests.

Updated the tests that asserted the old default-visible behaviour to the
opt-in contract and added round-trip coverage for `/hotbar on`.
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 3m 39s —— View job


PR Review: feat(tui): hide Hotbar until explicit opt-in (#3807)

  • Gather context and read changed files
  • Review correctness and logic
  • Review reuse/simplification
  • Review test coverage
  • Post final findings

Summary

Clean, well-scoped change. The core semantic inversion (NoneVec::new() instead of None → default bindings) is correct and propagates consistently across config, persistence, sidebar render, and hotbar setup. No correctness bugs. Two minor issues below.


Findings

1. Dead code: is_hotbar_disabled in the && guard — sidebar.rs:69

let hotbar_enabled = hotbar_panel_enabled(app, config) && !is_hotbar_disabled(config);

After this PR, hotbar_panel_enabled (which checks resolved_hotbar_bindings().is_empty()) already returns false for both None and Some([]). That means !is_hotbar_disabled(config) can never flip hotbar_enabled from false to true — the && always short-circuits before it's evaluated on the interesting paths, and when hotbar_panel_enabled is true the bindings are non-empty so is_hotbar_disabled is always false anyway. The guard is now unreachable dead code in practice.

Before this PR the distinction mattered: None produced default bindings, so is_hotbar_disabled blocked the "implicit-defaults" case from showing. Now both hidden states collapse to empty bindings. The expression is equivalent to just hotbar_panel_enabled(app, config).

is_hotbar_disabled still has independent value as a persistence predicate, but it's doing nothing in this expression.

Fix this →

2. Stale comment in surviving test — sidebar.rs:3462

// A missing `hotbar` key means "use defaults" — NOT disabled.
assert!(!is_hotbar_disabled(&Config::default()));

None no longer means "use defaults" — it now means "hidden." The assertion is still correct (the function still returns false for None), but the comment contradicts the semantics this PR establishes and will confuse future readers.

Fix this →


What's well done

  • default_hotbar_bindings_toml() as the single source of truth for what /hotbar on writes is the right abstraction; DEFAULT_HOTBAR_ACTIONSdefault_hotbar_bindings()default_hotbar_bindings_toml() keeps the chain clean.
  • Deleting remove_hotbar_from_config is correct; the "restore defaults" path no longer needs key deletion.
  • The new persist_default_hotbar_bindings_round_trips_for_hotbar_on test covers the full write → parse → resolve cycle for the opt-in path.
  • Wizard test fix (checked_action_ids().is_empty()) accurately reflects that no slots are pre-checked before opt-in.
  • HotbarConfigResolution comment on None => Vec::new() is precise and explains the three-state model (None / Some([]) / Some([…])) clearly for future readers.

@Hmbown Hmbown merged commit 9caffa0 into main Jun 30, 2026
19 checks passed
@Hmbown Hmbown deleted the codex/0866-3807-hotbar-hidden-default branch June 30, 2026 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v0.8.66: Ship Hotbar hidden by default until setup opt-in

1 participant