feat(ui): add hotkey sort/filter search bar with settings-search extraction - #227
Conversation
Standalone sort-key groundwork for the Settings -> Shortcuts bindings list (#199): SlotSortKey lives in its own top-level module so the slot manager (#198) can reuse it later, and HotkeySortState composes it with the existing generic SortState. No callers wired up yet — that lands in follow-up tasks of #199. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Settings -> Shortcuts bindings list (#199): a row exists for every slot with a bound trigger, resolved defensively into a sound, macro, or placeholder (missing sound / deleted macro / unassigned) so the display name is never empty. hotkey_rows() rebuilds, filters, and sorts these rows fresh on every call (#196/#197's filter_items/SortState, reused verbatim) -- the acceptance-criterion test surface for #199. Also bridges HotkeyRow across the module-tree boundary between crate::app (owns the state) and the sibling crate::ui::settings tree that will render it in a follow-up task, via two new accessors (hotkey_filter_query/hotkey_sort_state, mirroring search_query()) plus a pub(crate) re-export at the app module root. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds five Hotkey-prefixed Message variants (HotkeySearchChanged, ToggleHotkeySortMenu, ToggleHotkeySortDirection, SelectHotkeySort, DismissHotkeySortMenu) and their update() match arms, delegating to new mutator methods on HonkHonk in src/app/hotkeys.rs: toggle_hotkey_sort_menu, toggle_hotkey_sort_direction, select_hotkey_sort, dismiss_hotkey_sort_menu, persist_hotkey_sort, and replace_hotkey_filter_query. Sort selection and direction persist to config.sort_prefs["shortcuts"], mirroring the tiles view's SoundSort messages; the filter query stays transient like the tiles filter. Pins the round trip with message-driven end-to-end tests covering persistence, menu-anchor dismiss, and the unknown-id fallback. Part of #199 (task 3/8). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracts FilterTarget + active_filter_target() into src/app/filtering/targets.rs (filtering.rs measured over the 400-line cap once the retargeted handlers landed inline) and retargets filter_context/handle_type_to_filter/handle_escape to dispatch through it. Typing and Escape now route to the Settings -> Shortcuts bindings list's own hotkey_filter while that section is active, and never touch the tiles filter (or vice versa) -- the staged settings search (#213) still takes priority over both. Adds invariant tests pinning the routing as total and mutually exclusive across every view/section/search-state combination, and moves filtering.rs's pre-existing suite into filtering/tests.rs to stay under the file-size cap. Part of #199 (task 4/8). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds view_hotkey_sort_overlay for the Settings -> Shortcuts sort chip (#199), gated on the Hotkeys settings section so a menu opened on Shortcuts doesn't keep rendering (or leak from a stale anchor) after switching sections. show_settings_section() now clears sort_menu_anchor on every section switch, since the anchor field is shared between the tiles and hotkeys sort menus. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a third thin search-input factory (view_hotkeys_search_bar) beside the existing tiles/settings ones, with its own stable widget id (HOTKEYS_INPUT_ID / hotkeys_input_id()) so the hotkeys section can gain its own filter input without colliding with the other two. Pure widget builder with no app-state coupling; pins id stability/uniqueness and build-without-panic invariants at the boundary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move view_hotkeys_section/hotkey_bindings out of ui/settings/other.rs into their own file, sourcing rows from the pure hotkey_rows() query surface (with search-bar filtering and sort-chip sorting) instead of raw slot_triggers. Adds a portal_status_badge() helper to keep the section under the too_many_lines clippy budget, and gives the per-row view an explicit 'static output lifetime so it doesn't illegally borrow the function-local Vec<HotkeyRow> that hotkey_rows() returns. other.rs now holds only the Appearance and About sections. Part of #199 (task 7/8). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires view_hotkey_sort_overlay into view_settings as a Stack layer, base kept at child 0 across open/close so switching sections or toggling the Shortcuts sort menu doesn't reset the settings scrollable's offset (mirrors view_main's #112 pattern). Drops the now-stale dead_code allowance on view_hotkey_sort_overlay since it is genuinely called. Pins the wiring with an iced_test GUI-harness invariant: the sort menu is absent until the chip is clicked, then renders its options once the Shortcuts section's sort chip is opened. Closes the #199 task chain: full suite, clippy -D warnings, and fmt all pass; file/function budgets hold on every touched file; src/ui/slot_manager.rs and src/shortcuts/config_ui.rs remain untouched; dangling-sound-path and macro-bound fallback rows spot-checked clean. Settings-scrollable-offset-survives-menu-toggle still needs a manual live-Wayland-session check (see honkhonk_gui_verify_recipe) — not provable by cargo test alone. Part of #199 (task 8/8). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SortState::sorted() reversed compare()'s whole return value under Descending, including the slot-index tie-break SlotSortKey::compare() baked into it — so two hotkey rows tied on a non-slot key (e.g. the same Tag) sorted with their tie broken descending instead of the intended ascending-by-slot-index. Split SortKey::compare() (direction- reversible primary) from a new SortKey::tie_break() (always ascending, applied by sorted() after direction) so ties stay stable regardless of sort direction; SlotSortKey now implements both. SoundSortKey (tiles view) is unaffected — it keeps its own internal tie-break and the new trait default is a no-op for it. Also strengthens hotkey sort coverage: pins HotkeyRow's own value_unknown wiring for Length/Modified/Added (previously only the generic sort mechanism had this covered) and clarifies a test comment that claimed to exercise unknown-sorts-last without actually checking order. Extracts the Message enum out of src/app/mod.rs into src/app/message.rs. CLAUDE.md flags mod.rs as a known, must-not-grow violation; #199 needed five new Message variants for the Settings -> Shortcuts filter/sort chip; this takes that growth here instead of compounding the frozen file, and reduces mod.rs by ~140 lines in the process. Addresses code-review findings on #199. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an independent filter and sort pipeline for Settings → Shortcuts, including target-aware keyboard routing, persisted sort preferences, shortcut-row resolution, dedicated search UI, and sort-menu overlay behavior. ChangesHotkeys filtering and sorting
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsView
participant HonkHonk
participant HotkeyRows
User->>SettingsView: type search or open sort menu
SettingsView->>HonkHonk: HotkeySearchChanged or sort message
HonkHonk->>HotkeyRows: build, filter, and sort rows
HotkeyRows-->>SettingsView: rendered shortcut rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
`FilterState`'s activation contract is "focus and seed the filter input", but the Shortcuts branch of `handle_type_to_filter` only seeded the query and returned `Task::none()`. The list filtered correctly, yet the input was never focused — so Backspace, Delete, arrow keys, and selection edits kept going to whatever widget held focus before typing started, leaving the query effectively uneditable. Return a focus operation for `search_bar::hotkeys_input_id()`, mirroring the tiles path. The regression test pins the invariant for both filter targets so neither can silently regress to a no-op task. Found by Codex (gpt-5.6-sol, xhigh) adversarial cross-review of PR #227. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This was written agentically; verify its assertions and edit accordingly: Adversarial cross-review — Codex (
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/app/filtering/targets.rs (1)
54-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest oracle mirrors the implementation.
expected_targetis a verbatim copy ofactive_filter_target's match, sorouting_is_total_and_mutually_exclusive_across_every_statecan only fail if the two copies diverge — it can't catch a wrong rule. Consider an explicit expectation table (view_mode, section, searching) → target instead.🤖 Prompt for 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. In `@src/app/filtering/targets.rs` around lines 54 - 88, Replace the implementation-mirroring expected_target helper in routing_is_total_and_mutually_exclusive_across_every_state with an explicit expectation table mapping each (view_mode, section, searching) combination to its intended FilterTarget or None. Assert active_filter_target(&app) against the table-driven expected value, preserving coverage of every state without duplicating active_filter_target’s match logic.src/app/settings/gui_tests.rs (1)
50-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the whitespace-sensitive label convention.
" Name"(two leading spaces) is what distinguishes an unselected sort-menu row from other "Name" text in the view; that's invisible to future readers and silently breaks if row formatting changes. Add a short comment, and a message on the bare assert at line 62.♻️ Suggested tweak
+ // Sort-menu rows are prefixed with "✓ " when selected and " " otherwise; + // the two-space prefix is what distinguishes a menu row from other labels. assert!( !harness.find(" Name"), "sort menu should be closed until the sort chip is clicked" ); @@ - assert!(harness.find(" Name")); + assert!( + harness.find(" Name"), + "the open sort menu should list unselected keys such as Name" + );🤖 Prompt for 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. In `@src/app/settings/gui_tests.rs` around lines 50 - 62, Add a concise comment near the whitespace-sensitive harness.find(" Name") assertion explaining that the two leading spaces identify an unselected sort-menu row, and give the bare assert!(harness.find(" Name")) a descriptive failure message. Keep the existing selector and test behavior unchanged.src/app/filtering/tests.rs (1)
149-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the focus target, not just a non-empty task.
units() > 0only proves something was scheduled; it won’t fail ifTypeToFilter("h")starts returning a different task instead offocus(search_bar::hotkeys_input_id()). A widget-level focus assertion would catch this regression more directly.🤖 Prompt for 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. In `@src/app/filtering/tests.rs` around lines 149 - 167, Update the typed_filter_text_focuses_the_targeted_search_input test to assert that TypeToFilter("h") schedules focus on the correct search input, rather than only checking that the returned task has nonzero units. Verify the main view targets the tiles search input and the settings hotkeys view targets hotkeys_input_id(), using the widget-level focus assertion mechanism.src/ui/settings/hotkeys.rs (1)
190-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the Iced view smoke test.
This directly tests Iced view composition; retain coverage at the hotkey app-state boundary instead.
As per coding guidelines, do not test Iced view rendering or third-party internals.
🤖 Prompt for 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. In `@src/ui/settings/hotkeys.rs` around lines 190 - 200, Remove the view_hotkeys_section smoke test function view_hotkeys_section_builds_for_the_default_state and its associated test-only setup if no longer needed. Retain coverage through the hotkey app-state boundary, including hotkey_rows(), without adding replacement tests for Iced view rendering.Source: Coding guidelines
🤖 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 `@src/app/hotkeys/view.rs`:
- Around line 20-24: Update change_settings_search to clear sort_menu_anchor
when a staged settings search begins, ensuring the Hotkeys sort overlay is
dismissed before search_results is shown. Preserve the existing search behavior
and use the existing sort_menu_anchor state.
In `@src/app/message.rs`:
- Around line 42-54: Update the MacroStepDecoded and Decoded message variants to
carry the audio module’s typed thiserror-based decode error instead of Result
payloads using String. Propagate that concrete error type through the audio/app
boundary and adjust affected handling paths to preserve the typed failures.
In `@src/ui/search_bar.rs`:
- Line 78: Update the doc comment above the hotkeys-section search builder to
describe it as type-to-filter activated, removing the inaccurate “click-only”
wording. Keep the existing description of the stable input stack and align the
wording with active_filter_target and handle_type_to_filter.
---
Nitpick comments:
In `@src/app/filtering/targets.rs`:
- Around line 54-88: Replace the implementation-mirroring expected_target helper
in routing_is_total_and_mutually_exclusive_across_every_state with an explicit
expectation table mapping each (view_mode, section, searching) combination to
its intended FilterTarget or None. Assert active_filter_target(&app) against the
table-driven expected value, preserving coverage of every state without
duplicating active_filter_target’s match logic.
In `@src/app/filtering/tests.rs`:
- Around line 149-167: Update the
typed_filter_text_focuses_the_targeted_search_input test to assert that
TypeToFilter("h") schedules focus on the correct search input, rather than only
checking that the returned task has nonzero units. Verify the main view targets
the tiles search input and the settings hotkeys view targets hotkeys_input_id(),
using the widget-level focus assertion mechanism.
In `@src/app/settings/gui_tests.rs`:
- Around line 50-62: Add a concise comment near the whitespace-sensitive
harness.find(" Name") assertion explaining that the two leading spaces identify
an unselected sort-menu row, and give the bare assert!(harness.find(" Name")) a
descriptive failure message. Keep the existing selector and test behavior
unchanged.
In `@src/ui/settings/hotkeys.rs`:
- Around line 190-200: Remove the view_hotkeys_section smoke test function
view_hotkeys_section_builds_for_the_default_state and its associated test-only
setup if no longer needed. Retain coverage through the hotkey app-state
boundary, including hotkey_rows(), without adding replacement tests for Iced
view rendering.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b93010b-171f-4eeb-af2a-ab337e898ac6
📒 Files selected for processing (17)
src/app/filtering.rssrc/app/filtering/targets.rssrc/app/filtering/tests.rssrc/app/hotkeys.rssrc/app/hotkeys/rows.rssrc/app/hotkeys/tests.rssrc/app/hotkeys/view.rssrc/app/message.rssrc/app/mod.rssrc/app/settings.rssrc/app/settings/gui_tests.rssrc/app/slot_sort.rssrc/ui/list_controls/sort.rssrc/ui/search_bar.rssrc/ui/settings/hotkeys.rssrc/ui/settings/mod.rssrc/ui/settings/other.rs
Resolves the `src/app/mod.rs` conflict between #199's `Message` extraction into `message.rs` and main's slot-manager module split (#169): - keep both `mod slot_sort;` (#199) and `mod slots;` (main) - drop main's inline `Message` enum; #199 moved it to `message.rs` - port main's new `Message::AssignMacroSlot(u8, String)` variant into `message.rs` so the macro-slot binding survives the extraction Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the CodeRabbit review on #227. The staged settings search swaps the section body for `search_results` but leaves `settings_ui.section()` on Hotkeys, so `view_hotkey_sort_overlay`'s section guard could not tell the two apart — an open sort menu kept stacking its overlay over the search results. `change_settings_search` now clears the shared `sort_menu_anchor`, matching `show_settings_section`. Also from the same review: - correct `view_hotkeys_search_bar`'s doc comment: the bar is type-to-filter activated, not click-only ("click-only" describes the staged settings search) - replace the mirrored test oracle in `active_filter_target`'s totality test with an explicit 30-row expectation table, plus a coverage test so a new ViewMode/SettingsSection fails loudly instead of going unrouted - route both type-to-filter focus branches through a named `filter_input_id` seam and pin the per-target input ids, since Iced's `Task` cannot be inspected to prove *which* widget a scheduled focus targets - document the whitespace-sensitive sort-menu row selector in the GUI test - drop the `view_hotkeys_section` smoke test: CLAUDE.md excludes Iced view rendering from the suite; `hotkey_rows()`'s boundary keeps the coverage Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to CodeRabbit's review of the EXPECTED_ROUTING table: the coverage test iterated SECTIONS/VIEW_MODES, which are themselves hand-maintained, so a newly added ViewMode or SettingsSection could be absent from both the arrays and the table without failing anything. The doc comment claimed otherwise. Neither enum exposes an all-variants list, so the guarantee now comes from exhaustive `const fn` index matches plus a `const` block asserting the arrays agree with them and that the table is exactly their product. Adding a variant stops the module compiling, on the arrays that must grow with it — verified by reordering SECTIONS, which fails const evaluation as intended. The runtime test's comment now claims only what it checks: full coverage of the arrays' product, with completeness of the arrays delegated to the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was written agentically; verify its assertions and edit accordingly:
Why
Issue #199 asks for the hotkeys settings section to gain the same search/sort/filter affordances already available elsewhere in settings, so users with large hotkey lists can find and reorder entries quickly.
What
Adds a search bar, sort-menu overlay, and portal-status badge to the hotkeys settings section, backed by a new
src/ui/settings/hotkeys.rsmodule and aFilterTarget-based routing refactor shared with the existing tile filtering.src/app/filtering/targets.rswas extracted to keepfiltering.rsunder the 400-line file budget.Design decisions
Pre-spike decisions 1-12 all held (see spike learnings §6 positive confirmations: shared sort_menu_anchor, Hotkey-prefixed Message variants, owned HotkeyRow, FilterTarget-based routing as a pure refactor for tiles, ui/settings/hotkeys.rs extraction) — none reversed. Five additional corrections made on contact with real code, all narrow and mechanical, none changing the architecture:
Cross-module-tree visibility bridge added (new, not in pre-spike design at all):
src/ui/settings/hotkeys.rssits in a sibling module tree tocrate::app, unlikesrc/app/header.rswhich is a descendant and can read private fields directly. Fixed with twopub(crate)accessor methods (hotkey_filter_query(),hotkey_sort_state(), mirroring the existingsearch_query()pattern) plus onepub(crate) use hotkeys::HotkeyRow;re-export at the app module root. This is the single most consequential spike finding — a design gap, not an implementation bug — and is now folded into structs/interfaces/todos as first-class elements rather than an afterthought.HotkeySortState visibility corrected from
pub(super)topub(crate)at its definition site. Rust's re-export rule only allows narrowing, never widening (E0365);pub(super)would make it unusable the moment anypub(crate)method (likehotkey_sort_state()) names it in a return-type position (private-interfaces violation). No corresponding re-export of the type itself was needed — it only flows through generic inference at the sort-chip call site, confirmed by spike, so no dead-code re-export was added.Per-row view function needs an explicit
'staticoutput lifetime, not Rust's elided&HotkeyRow-borrowed lifetime — otherwise the returnedVec<Element>illegally borrows from the function-localVec<HotkeyRow>thathotkey_rows()returns (E0515). Matches an established sibling pattern already in the same file (view_about_section(t) -> Element<'static, Message>), so this is a consistency fix, not a new idiom.view_hotkeys_sectionneeded one additional extraction (portal_status_badge()) beyond what the pre-spike design's todos flagged — it only called outhandle_type_to_filter/handle_escapeas complexity risks, but the inline search-bar + sort-chip + portal-badge + row-list body measured 62/50 lines. Same file-organization instinct as the design's other proactive extractions, just applied one level deeper.src/app/filtering/targets.rsextraction is now unconditional, not the design's original "if needed" hedge. Spike measuredfiltering.rsat 431/400 lines with the full retargeting landed inline. MovingFilterTarget+active_filter_target()out is now todo item 8, always executed, using Rust's file+dir sibling-module form (filtering.rsdeclaresmod targets;alongsidefiltering/targets.rs— nomod.rsneeded).All five corrections are visibility/lifetime/line-budget fixes fully compatible with the original architecture; no struct, Message variant, or module boundary was added, removed, or renamed as a result of the spike.
Testing
cargo build --releasepassescargo test(running in background at PR-creation time; verify CI is green)cargo clippy -- -D warnings(running in background at PR-creation time; verify CI is green)🤖 Co-authored by Claude Sonnet 5. Closes #199
Summary by CodeRabbit
New Features
Bug Fixes