feat(state,ui): theme preference persistence + live switching - #79
Merged
Conversation
- Add System variant to Theme enum (maps to Dark colors; portal detection TBD) - Derive Serialize/Deserialize on Theme; add setting_index/from_setting_index helpers - Add theme field to AppConfig with #[serde(default)] for backward compat - Remove hardcoded Theme::Dark from HonkHonk::theme() and all view functions - Add Message::ThemeChanged(Theme) + update handler that saves config - Wire SettingId::Theme into SETTINGS_REGISTRY as Radio([Light, Dark, System]) - Add Radio arm to render_setting_row (pill buttons, active = inverted ink) - Wire get_setting_value + setting_message for Theme - Expand view_appearance_section to render registry rows - 94/94 tests pass
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds serializable Theme (Light/Dark/System), stores theme in AppConfig, registers a Theme radio in SETTINGS_REGISTRY, wires settings UI to emit ThemeChanged, and applies/persists the chosen theme across app views. ChangesTheme Persistence and Live Switching
Sequence DiagramsequenceDiagram
participant User
participant SettingsUI as Settings UI
participant GetValue as get_setting_value
participant SetMsg as setting_message
participant AppUpdate as app update
participant Config as AppConfig
participant ThemeMethod as app theme method
participant Views as Render Views
User->>SettingsUI: click theme option
SettingsUI->>GetValue: current theme index
GetValue->>Config: read config theme
GetValue-->>SettingsUI: index value
SettingsUI->>SettingsUI: render highlighted
User->>SettingsUI: select new theme
SettingsUI->>SetMsg: setting and index
SetMsg-->>AppUpdate: ThemeChanged message
AppUpdate->>Config: save new theme
Views->>ThemeMethod: request iced theme
ThemeMethod->>Config: read config theme
ThemeMethod-->>Views: mapped iced theme
Views->>Views: render with new theme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/settings.rs (1)
237-250:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnhandled setting mappings should not fall back to
RescanLibrary.Line 249 can trigger a real rescan when a new setting is added but not wired yet. That’s a risky default action for an “unhandled” path.
Safer fallback pattern
// src/app.rs (Message enum) + NoOp, // src/app.rs (update match) + Message::NoOp => Task::none(), // src/ui/settings.rs pub fn setting_message(id: SettingId, value: SettingValue) -> Message { match (id, value) { (SettingId::RescanLibrary, _) => Message::RescanLibrary, (SettingId::Theme, SettingValue::Index(i)) => { Message::ThemeChanged(crate::ui::theme::Theme::from_setting_index(i)) } other => { debug_assert!( false, "setting_message: unhandled ({:?}) — add an arm here when wiring a backend", other ); - Message::RescanLibrary + Message::NoOp } } }🤖 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.rs` around lines 237 - 250, The match fallback in setting_message currently returns Message::RescanLibrary for unhandled cases, which can trigger a real rescan; change the fallback arm (the `other` pattern in setting_message) to return a safe no-op message instead (e.g., add and return Message::NoOp or Message::UnhandledSetting) and keep the debug_assert to notify during development; update any consumers if needed to handle the new no-op variant and avoid performing side-effects for unwired SettingId values.
🧹 Nitpick comments (1)
src/state/config.rs (1)
21-23: ⚡ Quick winAdd an explicit legacy-config test for missing
themefield.
#[serde(default)]is the compatibility guarantee here; a focused test prevents regressions if serde attributes change later.Proposed test addition
#[test] fn default_config_has_expected_values() { let config = AppConfig::default(); assert_eq!(config.volume, 0.85); assert_eq!(config.window_width, 900); assert_eq!(config.window_height, 600); + assert_eq!(config.theme, Theme::Dark); } + +#[test] +fn deserialize_legacy_config_without_theme_defaults_to_dark() { + let legacy = r#" + { + "sound_directories": ["/tmp/sounds"], + "volume": 0.5, + "window_width": 1024, + "window_height": 768 + } + "#; + + let parsed: AppConfig = serde_json::from_str(legacy).unwrap(); + assert_eq!(parsed.theme, Theme::Dark); +}Also applies to: 143-208
🤖 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/state/config.rs` around lines 21 - 23, Add a focused legacy-config test that ensures deserializing a config JSON without the theme field still yields the default Theme via serde(default): create a test (e.g., in the module tests for src/state/config.rs) that deserializes a JSON string missing "theme" into your Config struct and assert config.theme == Theme::default() (or matches the known default variant); include similar tests for the other serde(default) fields referenced around lines 143-208 to prevent regressions. Ensure the test imports Config and Theme and uses the same serde deserialization path your code uses so it verifies the compatibility guarantee.
🤖 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 586-592: In update(), avoid mutating self.config in place for
Message::ThemeChanged; instead construct a new Config instance (copying existing
fields with the new theme), compare it to the current self.config and only
assign and call self.config.save() when the theme actually changed; reference
the Message::ThemeChanged match arm, the update() method, and the
self.config.save() call to locate where to replace the in-place mutation with an
immutable new-config assignment and guarded save.
---
Outside diff comments:
In `@src/ui/settings.rs`:
- Around line 237-250: The match fallback in setting_message currently returns
Message::RescanLibrary for unhandled cases, which can trigger a real rescan;
change the fallback arm (the `other` pattern in setting_message) to return a
safe no-op message instead (e.g., add and return Message::NoOp or
Message::UnhandledSetting) and keep the debug_assert to notify during
development; update any consumers if needed to handle the new no-op variant and
avoid performing side-effects for unwired SettingId values.
---
Nitpick comments:
In `@src/state/config.rs`:
- Around line 21-23: Add a focused legacy-config test that ensures deserializing
a config JSON without the theme field still yields the default Theme via
serde(default): create a test (e.g., in the module tests for
src/state/config.rs) that deserializes a JSON string missing "theme" into your
Config struct and assert config.theme == Theme::default() (or matches the known
default variant); include similar tests for the other serde(default) fields
referenced around lines 143-208 to prevent regressions. Ensure the test imports
Config and Theme and uses the same serde deserialization path your code uses so
it verifies the compatibility guarantee.
🪄 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: 19fd690a-3e8e-48d3-b80f-c9dac7fede30
📒 Files selected for processing (5)
src/app.rssrc/settings/mod.rssrc/state/config.rssrc/ui/settings.rssrc/ui/theme.rs
Collaborator
Author
|
Fixed CodeRabbit suggestion in 38bf905: |
5 tasks
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.
Closes #69.
What
Themeenum gainsSystemvariant +serdederives +setting_index/from_setting_indexhelpersAppConfig.theme: Themefield added with#[serde(default)]— existingconfig.jsonfiles without the field load asDarkHonkHonk::theme()now maps fromself.config.themeinstead of hardcodingTheme::Darktheme::Theme::Darkinview_main,view()SlotManager arm, andview_settingscall replaced withself.config.themeMessage::ThemeChanged(Theme)added; handler updatesconfig.themeand saves to diskSettingId::Themewired intoSETTINGS_REGISTRYasRadio(&["Light", "Dark", "System"])render_setting_rowgains aRadioarm rendering pill-buttons (active = ink-inverted)get_setting_valueandsetting_messagehandleThemeview_appearance_sectionnow renders registry rows instead of empty columnSystem theme
Systemmaps to Dark colors for now (the same palette asDark). Portal-based DE color scheme detection (#TBD) will refine this in a future sub-MVP.Backward compat
#[serde(default)]onAppConfig.thememeans any existingconfig.jsonwithout the field deserializes cleanly withTheme::Dark.Test plan
cargo test— 94/94 passcargo clippy -- -D warnings— zero warnings~/.config/honkhonk/config.jsonfor"theme": "Light")config.jsonwithoutthemekey loads as Dark (backward compat)Summary by CodeRabbit