feat(ui): favorites tab + per-sound volume + sound editor overlay#102
Merged
Conversation
- SoundMetaStore: persisted JSON (sound_meta.json) storing per-sound favorite flag, volume multiplier, and optional display name override - Favorites tab: virtual ★ Favorites category chip appears when any sound is starred; filtered_sounds() respects it - Per-sound volume: applied at decode-time as a sample multiplier (master volume × per-sound multiplier); avoids changing global state - Sound editor overlay: modal sheet accessible via right-click context menu "Edit sound…"; lets user set display name, volume, and toggle favorite; saves on "Save" or discards on "Cancel"/"Esc" - Star indicator: ★ prefixed to tile name when sound is favorited - 11 new unit tests covering all message handlers and state transitions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persisted per-sound metadata (favorite, volume, display name), threads it into app state and the grid UI, implements a modal per-sound editor with draft lifecycle and persistence, applies per-sound volume at playback, and adds tests and favorites filtering. ChangesPer-Sound Metadata & Editor
Sequence Diagram(s)sequenceDiagram
participant User
participant Grid
participant App
participant Store as SoundMetaStore
participant File as JSON_File
participant Audio
User->>Grid: Click "star" or "Edit"
Grid->>App: ToggleFavorite(id) / OpenSoundEditor(id)
App->>Store: toggle_favorite(id) / get(id)
Store-->>App: SoundMeta
App->>Store: save()
Store->>File: write sound_meta.json
User->>App: Play sound id
App->>Store: volume_for(id)
Store-->>App: multiplier
App->>App: scale decoded samples by multiplier
App->>Audio: AudioCommand::Play
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Rustfmt reformatted method chains and indentation in app.rs, sound_meta.rs, sound_editor.rs, and sound_grid.rs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.rs (1)
535-547:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle Esc to dismiss the sound editor overlay.
Line 535 currently ignores editor state, so Esc does not discard/close the editor.
Suggested fix
Message::EscapePressed => { if self.context_menu.is_some() { // Context menu takes priority — close it, leave search state intact. self.context_menu = None; self.context_menu_pos = None; + } else if self.editor_sound_id.is_some() { + self.editor_sound_id = None; + self.editor_draft_name = String::new(); + self.editor_draft_volume = 1.0; } else if self.search_had_focus { // First Esc: treat as blur — Iced already handled unfocus. self.search_had_focus = false; } else if !self.search_query.is_empty() {🤖 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.rs` around lines 535 - 547, When handling Message::EscapePressed, first check and handle the sound editor overlay before the context_menu/search logic: if the editor is open (e.g. self.sound_editor.is_some() or self.sound_editor_open == true) dismiss it and discard any pending edits by setting the editor state to None/false or calling the existing teardown method (e.g. self.sound_editor = None or self.discard_sound_editor()), then return Task::none(); otherwise continue with the existing context_menu, search_had_focus and search_query logic as currently implemented.
🤖 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.rs`:
- Around line 820-827: When handling Message::OpenSoundEditor you set editor
state but forget to close the context menu, causing the menu to remain rendered
above the editor; update the Message::OpenSoundEditor branch to also clear the
context menu (e.g., set self.context_menu = None) right after setting
self.editor_sound_id / editor_draft_name / editor_draft_volume so the editor can
surface immediately, and apply the same change to any other message handlers
that open editors (the other editor-opening branches that render before the
context menu).
---
Outside diff comments:
In `@src/app.rs`:
- Around line 535-547: When handling Message::EscapePressed, first check and
handle the sound editor overlay before the context_menu/search logic: if the
editor is open (e.g. self.sound_editor.is_some() or self.sound_editor_open ==
true) dismiss it and discard any pending edits by setting the editor state to
None/false or calling the existing teardown method (e.g. self.sound_editor =
None or self.discard_sound_editor()), then return Task::none(); otherwise
continue with the existing context_menu, search_had_focus and search_query logic
as currently implemented.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 312b3901-2e9d-44a1-bdc7-625435f956ab
📒 Files selected for processing (6)
src/app.rssrc/state/mod.rssrc/state/sound_meta.rssrc/ui/mod.rssrc/ui/sound_editor.rssrc/ui/sound_grid.rs
- OpenSoundEditor now clears context_menu/context_menu_pos so the editor overlay surfaces immediately rather than rendering behind the still-visible context menu. - EscapePressed now dismisses the editor overlay (discarding drafts) before falling through to the search-focus / query-clear logic. - Add regression tests: open_sound_editor_clears_context_menu and escape_dismisses_editor_before_search_focus. Fixes CodeRabbit review thread on PR #102.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app.rs (3)
420-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSearch against the displayed name too.
After a rename,
src/ui/sound_grid.rs:90-104showsdisplay_name, but this filter still only matchesSoundEntry.name. Renamed sounds stop being searchable by the label the user actually sees.Suggested fix
self.sounds .iter() .filter(|s| match self.active_category.as_deref() { Some(cat) if cat == FAVORITES_TAB => self.sound_meta.is_favorite(&s.id), Some(cat) => s.category == cat, None => true, }) - .filter(|s| query.is_empty() || s.name.to_lowercase().contains(&query)) + .filter(|s| { + if query.is_empty() { + return true; + } + + let display_name_matches = self + .sound_meta + .get_ref(&s.id) + .and_then(|m| m.display_name.as_deref()) + .is_some_and(|name| name.to_lowercase().contains(&query)); + + s.name.to_lowercase().contains(&query) || display_name_matches + }) .collect()Based on
src/ui/sound_grid.rs:90-104, tiles renderdisplay_nameas the primary label.🤖 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.rs` around lines 420 - 430, filtered_sounds currently only matches on SoundEntry.name, so renamed tiles (which render SoundEntry.display_name in src/ui/sound_grid.rs) are not searchable; update the filter in the filtered_sounds method to match against the displayed label by checking s.display_name (and keep s.name as a fallback) using the same lowercasing logic (e.g., compare query against s.display_name.to_lowercase().contains(&query) || s.name.to_lowercase().contains(&query)), ensuring you reference the SoundEntry.display_name field and maintain the existing behavior when display_name is empty.
821-826:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFall back to
Allwhen the last favorite is removed.If this toggle clears the final favorite while
FAVORITES_TABis active,view_category_chips()hides the favorites chip butactive_categorystays on that now-invisible value. The next render shows an empty list under a filter the user can no longer see.Suggested fix
Message::ToggleFavorite(sound_id) => { - self.sound_meta.toggle_favorite(&sound_id); + let is_favorite = self.sound_meta.toggle_favorite(&sound_id); if let Err(e) = self.sound_meta.save() { eprintln!("honkhonk: sound meta save error: {e}"); } + if !is_favorite + && self.active_category.as_deref() == Some(FAVORITES_TAB) + && !self.sounds.iter().any(|s| self.sound_meta.is_favorite(&s.id)) + { + self.active_category = None; + } Task::none() }Based on
filtered_sounds()andview_category_chips()in this file, the hidden-category state is reachable after the final unstar.Also applies to: 958-967
🤖 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.rs` around lines 821 - 826, When handling Message::ToggleFavorite (the block that calls self.sound_meta.toggle_favorite and then save), detect if the toggle removed the final favorite and the current active_category equals FAVORITES_TAB, and if so set active_category = Category::All (or equivalent "All" variant) before returning Task::none(); also update the analogous toggle block around the other ToggleFavorite handling (the similar code near the other occurrence referenced) so both places call the same logic; use filtered_sounds() and view_category_chips() semantics to determine "final favorite removed" by checking self.sound_meta.favorites.is_empty() (or the existing API) after toggle and then reset active_category.
307-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
new_for_test()off the real sound-meta file.
new_for_test()swaps inSoundMetaStore::default(), but these handlers still callsave(). BecauseSoundMetaStore::save()insrc/state/sound_meta.rs:111-135always writes to the default XDG path, the new metadata tests now mutate the developer’s actual sound-meta file duringcargo test.Suggested fix
pub struct HonkHonk { // ... pub(crate) sound_meta: SoundMetaStore, + persist_sound_meta: bool, editor_sound_id: Option<String>, // ... } source_notice: None, sound_meta: SoundMetaStore::load(), + persist_sound_meta: true, editor_sound_id: None, editor_draft_name: String::new(), editor_draft_volume: 1.0, source_notice: None, sound_meta: SoundMetaStore::default(), + persist_sound_meta: false, editor_sound_id: None, editor_draft_name: String::new(), editor_draft_volume: 1.0,- if let Err(e) = self.sound_meta.save() { - eprintln!("honkhonk: sound meta save error: {e}"); + if self.persist_sound_meta { + if let Err(e) = self.sound_meta.save() { + eprintln!("honkhonk: sound meta save error: {e}"); + } }Also applies to: 346-349, 821-825, 866-868
🤖 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.rs` around lines 307 - 310, new_for_test() currently swaps in SoundMetaStore::default(), but SoundMetaStore::save() always writes to the real XDG path so tests mutate real metadata; change new_for_test() to create/inject a test-scoped SoundMetaStore that writes to a temporary path (or an in-memory/no-op backend) instead of using SoundMetaStore::default(). Locate the usage in the code that sets the sound_meta field (where SoundMetaStore::load() is used) and update new_for_test() to call a constructor like SoundMetaStore::with_path(temp_dir) or a test helper that sets an ephemeral XDG_CONFIG_HOME, ensuring SoundMetaStore::save() writes to that temp location (or becomes a no-op) so handlers that call save() (via sound_meta, editor_sound_id, editor_draft_name, editor_draft_volume) do not modify the developer’s real sound-meta file during tests.
🧹 Nitpick comments (1)
src/app.rs (1)
821-872: ⚡ Quick winExtract the sound-meta rules out of
app.rs.These message arms now own trimming, clamping, favorite preservation, and persistence directly inside the Iced glue layer. Moving that into
src/state/sound_meta.rsor small helper methods would keepapp.rsfocused on message routing and stopupdate()from growing further.As per coding guidelines,
src/app.rs“holds state, handles Messages, and composes UI. No business logic — delegates to module APIs.”🤖 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.rs` around lines 821 - 872, The Message arms in app.rs are doing business logic (trimming names, clamping volumes, preserving favorite flag, and calling save()) — move those into the sound_meta module and call small API methods from update(). Add/update functions on the sound_meta API such as toggle_favorite_and_persist(sound_id) which toggles and saves, load_for_editor(sound_id) -> (display_name: Option<String>, volume: f32) to return prepared values, and save_editor_changes(sound_id, draft_name: &str, draft_volume: f32) which does trimming, clamps volume (0.0..=2.0), preserves favorite, sets the meta and persists (including returning Result for errors). Replace direct get/set/save/clamp/trim logic in the Message::ToggleFavorite, Message::OpenSoundEditor, Message::SoundEditorVolumeChanged, and Message::SaveSoundMeta arms with calls to these new sound_meta methods and handle errors only at the callsite (e.g., log if the Result is Err).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.
Outside diff comments:
In `@src/app.rs`:
- Around line 420-430: filtered_sounds currently only matches on
SoundEntry.name, so renamed tiles (which render SoundEntry.display_name in
src/ui/sound_grid.rs) are not searchable; update the filter in the
filtered_sounds method to match against the displayed label by checking
s.display_name (and keep s.name as a fallback) using the same lowercasing logic
(e.g., compare query against s.display_name.to_lowercase().contains(&query) ||
s.name.to_lowercase().contains(&query)), ensuring you reference the
SoundEntry.display_name field and maintain the existing behavior when
display_name is empty.
- Around line 821-826: When handling Message::ToggleFavorite (the block that
calls self.sound_meta.toggle_favorite and then save), detect if the toggle
removed the final favorite and the current active_category equals FAVORITES_TAB,
and if so set active_category = Category::All (or equivalent "All" variant)
before returning Task::none(); also update the analogous toggle block around the
other ToggleFavorite handling (the similar code near the other occurrence
referenced) so both places call the same logic; use filtered_sounds() and
view_category_chips() semantics to determine "final favorite removed" by
checking self.sound_meta.favorites.is_empty() (or the existing API) after toggle
and then reset active_category.
- Around line 307-310: new_for_test() currently swaps in
SoundMetaStore::default(), but SoundMetaStore::save() always writes to the real
XDG path so tests mutate real metadata; change new_for_test() to create/inject a
test-scoped SoundMetaStore that writes to a temporary path (or an
in-memory/no-op backend) instead of using SoundMetaStore::default(). Locate the
usage in the code that sets the sound_meta field (where SoundMetaStore::load()
is used) and update new_for_test() to call a constructor like
SoundMetaStore::with_path(temp_dir) or a test helper that sets an ephemeral
XDG_CONFIG_HOME, ensuring SoundMetaStore::save() writes to that temp location
(or becomes a no-op) so handlers that call save() (via sound_meta,
editor_sound_id, editor_draft_name, editor_draft_volume) do not modify the
developer’s real sound-meta file during tests.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 821-872: The Message arms in app.rs are doing business logic
(trimming names, clamping volumes, preserving favorite flag, and calling save())
— move those into the sound_meta module and call small API methods from
update(). Add/update functions on the sound_meta API such as
toggle_favorite_and_persist(sound_id) which toggles and saves,
load_for_editor(sound_id) -> (display_name: Option<String>, volume: f32) to
return prepared values, and save_editor_changes(sound_id, draft_name: &str,
draft_volume: f32) which does trimming, clamps volume (0.0..=2.0), preserves
favorite, sets the meta and persists (including returning Result for errors).
Replace direct get/set/save/clamp/trim logic in the Message::ToggleFavorite,
Message::OpenSoundEditor, Message::SoundEditorVolumeChanged, and
Message::SaveSoundMeta arms with calls to these new sound_meta methods and
handle errors only at the callsite (e.g., log if the Result is Err).
Three bugs flagged in latest CodeRabbit review (2026-06-06): 1. Search now matches both SoundEntry.name and SoundMeta.display_name so sounds renamed via the editor remain discoverable by their visible label. 2. ToggleFavorite resets active_category to None (All) when the last favorite is removed while the Favorites tab is active, preventing an empty list under an invisible chip. 3. new_for_test() now sets persist_sound_meta=false; all sound_meta.save() calls are gated on that flag so cargo test never writes to the developer's real XDG config dir. Regression tests added for all three cases.
Collaborator
Author
|
All three actionable findings from the 2026-06-06 CodeRabbit review addressed in commit b2a9a7e: 1. Search against display_name (Major) — Fixed. 2. Fall back to All when last favorite removed (Minor) — Fixed. 3. Tests writing to real XDG sound-meta file (Major) — Fixed. Added 4. Extract sound-meta business logic from app.rs (Nitpick) — Declining for this PR. The CLAUDE.md scope-creep rule requires each PR to have a single demonstrable outcome and stay under 500 LOC delta. Extracting |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
★ Favoritescategory chip that appears when any sound is starred;filtered_sounds()routes through it correctlySoundMetaStore, applied at decode-time as a sample multiplier (master × per-sound); no global state mutationsCloses #14
Design decisions
Scoped to 3 of 5 sub-features. Issue #14 lists: favorites, per-sound volume, overlap mode, per-sound editor sheet, list view. The 500 LOC constraint and CLAUDE.md sub-MVP discipline mean only the highest-value, most independent features are in this PR. Overlap mode (global playback policy) and list view alternative are deferred. Per-sound editor and favorites are included as they share the same
SoundMetaStoredata model.Implementation LOC. The raw diff is ~840 lines; ~230 of those are unit tests (12 in
sound_meta.rs, 11 new inapp.rs) which CLAUDE.md explicitly excludes from the 500 LOC limit. Implementation-only LOC is ~610 — slightly over but split across 3 coherent layers (state, UI, wiring). A separate PR splitting editor from favorites would have been more work with less independence.Per-sound volume applied at decode time. Rather than adding a
volumeparameter toAudioCommand::Play, the samples are scaled in-place before sending. This keeps the audio engine API stable and is the right tradeoff for the current phase (per-sound offsets are small multipliers, no quality degradation at f32).SoundMetaStoreas a standalone JSON file. Kept separate fromconfig.json(which isAppConfig) to avoid conflating library-volatile data (sound IDs are path-hash derived) with stable user preferences. Pattern matchesslots.json.Display name as draft state in app. The editor holds
editor_draft_nameandeditor_draft_volumeinHonkHonkstate rather than passing mutable closures through the view tree. This is idiomatic Iced MVU — the view is pure, mutation happens through messages.Favorite toggle is instant-save. Clicking ★ on a tile immediately persists via
SoundMetaStore::save(). The editorSaveSoundMetapath also saves. This means no "unsaved changes" state for favorites, consistent with how the slot assignments work.EditorCtx.metais owned, not&SoundMeta.SoundMetaStore::get()returns an ownedSoundMeta(clones the default when absent). Passing&metawheremetais a local insideview_main()caused a lifetime error — the clean fix is an owned snapshot sinceSoundMetaisClone + Copy-sized.Out of scope (not in this PR)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests