various: redesign settings menu again - #588
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds persisted settings layout configuration, categorized settings navigation, responsive fan and top-bar layouts, extracted settings sections, desktop tools, localized labels, supporting Tailwind utilities, and a macOS Helium bundle identifier update. ChangesSettings layout
Helium bundle identifier
Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant SettingsNavigation
participant SettingsSection
participant AppConfig
participant PlayerController
SettingsPage->>SettingsNavigation: Render selected category
SettingsNavigation->>SettingsPage: Dispatch category selection
SettingsPage->>SettingsSection: Render category content
SettingsSection->>AppConfig: Update settings
SettingsSection->>PlayerController: Apply audio settings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
crates/pages/src/settings.rs (1)
402-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup scattered
Generalcategory content together.The
Generalcategory content is split into two separate blocks: lines 784-805 (auto_check_updates,minimize_to_tray) and lines 917-927 (back_behavior). A largeCustomizationblock (807-916, about 110 lines) sits between them. General, Customization, and Library also share oneSettingsSectionwhose title is chosen by amatchonactive_category()(lines 404-408); adding a new category to the outermatches!()gate without updating thismatchsilently falls back to"general".Move all
if active_category() == SettingsCategory::General { ... }blocks together, and do the same forCustomization. This reduces the risk that a future setting is added under the wrong category, or that the titlematchand the outer category gate drift out of sync.Also applies to: 784-805, 917-927
🤖 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 `@crates/pages/src/settings.rs` around lines 402 - 408, Reorganize the settings rendering blocks so all General content, including auto_check_updates, minimize_to_tray, and back_behavior, is contiguous, and all Customization content is contiguous; keep Library in its existing category section. Update the shared SettingsSection structure and its active_category() title selection so the outer category gate and displayed title remain synchronized, avoiding a fallback to "general" when categories change.
🤖 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 `@crates/kopuz/assets/themes.css`:
- Around line 500-509: Update the .settings-workspace declarations to add a
stylelint-disable-next-line comment immediately before the intentional
--settings-fan-radius fallback redeclaration, and insert the required blank
lines before the flagged declarations in both the fan-navigation block and the
corresponding later block around the min-height declarations. Preserve the
existing vh/dvh fallback order and values.
---
Nitpick comments:
In `@crates/pages/src/settings.rs`:
- Around line 402-408: Reorganize the settings rendering blocks so all General
content, including auto_check_updates, minimize_to_tray, and back_behavior, is
contiguous, and all Customization content is contiguous; keep Library in its
existing category section. Update the shared SettingsSection structure and its
active_category() title selection so the outer category gate and displayed title
remain synchronized, avoiding a fallback to "general" when categories change.
🪄 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: 14bb1688-4b05-4532-afae-ae7038551b15
📒 Files selected for processing (34)
crates/config/src/lib.rscrates/config/src/views.rscrates/i18n/locales/ar.ftlcrates/i18n/locales/de.ftlcrates/i18n/locales/en.ftlcrates/i18n/locales/es.ftlcrates/i18n/locales/fil.ftlcrates/i18n/locales/fr.ftlcrates/i18n/locales/gr.ftlcrates/i18n/locales/he.ftlcrates/i18n/locales/hu.ftlcrates/i18n/locales/id.ftlcrates/i18n/locales/it.ftlcrates/i18n/locales/ja.ftlcrates/i18n/locales/ko.ftlcrates/i18n/locales/ml.ftlcrates/i18n/locales/nl.ftlcrates/i18n/locales/pl.ftlcrates/i18n/locales/pt-BR.ftlcrates/i18n/locales/pt-PT.ftlcrates/i18n/locales/ro.ftlcrates/i18n/locales/ru.ftlcrates/i18n/locales/sv.ftlcrates/i18n/locales/ta.ftlcrates/i18n/locales/tok-SP.ftlcrates/i18n/locales/tok.ftlcrates/i18n/locales/tr.ftlcrates/i18n/locales/uk.ftlcrates/i18n/locales/vi-VN.ftlcrates/i18n/locales/zh-CN.ftlcrates/kopuz/assets/tailwind.csscrates/kopuz/assets/themes.csscrates/pages/src/settings.rscrates/server/src/spotify/host.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pages/src/settings.rs (1)
697-702: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the DB debug hooks out of the conditional Tools branch.
hooks::debug_db_section()is only called whenactive_category() == SettingsCategory::Tools, but the function runs Dioxus hooks (use_context::db::Db,db_reactivity::use_generations(), anduse_signal). Switching into and out of Tools changes those hook calls, so move this function to a parent component/scope or otherwise ensure its hooks are never conditional.🤖 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 `@crates/pages/src/settings.rs` around lines 697 - 702, Move the hooks::debug_db_section() invocation out of the active_category() == SettingsCategory::Tools conditional so its Dioxus hooks always execute in a stable parent scope, while keeping the debug section’s visible placement or equivalent rendering behavior for the Tools category.
🧹 Nitpick comments (3)
crates/pages/src/settings/navigation.rs (1)
64-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
label_tiltfromangleand remove the duplicate prop.Every item passes the same string for
angleandlabel_tilt. The two values cannot diverge today, and a future edit to one value can silently desynchronize the button rotation from the label rotation. Reuseanglefor the label tilt and delete thelabel_tiltprop.♻️ Proposed refactor to remove the duplicated geometry prop
fn SettingsFanItem( category: SettingsCategory, selected: SettingsCategory, label: String, icon: &'static str, angle: &'static str, label_x: &'static str, label_y: &'static str, - label_tilt: &'static str, on_select: EventHandler<SettingsCategory>, ) -> Element {span { class: "settings-fan-label {selected_class}", - style: "--label-x: {label_x}; --label-y: {label_y}; --label-tilt: {label_tilt};", + style: "--label-x: {label_x}; --label-y: {label_y}; --label-tilt: {angle};", aria_hidden: "true",Then delete the
label_tilt:line from each of the eightSettingsFanItemcall sites.🤖 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 `@crates/pages/src/settings/navigation.rs` around lines 64 - 153, Update the SettingsFanItem definition and its call sites in the settings navigation construction to derive label tilt from the existing angle value instead of accepting a separate label_tilt prop. Remove label_tilt from all eight SettingsFanItem initializers while preserving each item’s current angle and rotation behavior.crates/pages/src/settings/sections.rs (1)
97-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
AppSelectfor consistency with the other settings dropdowns.The rest of the settings page renders dropdowns through
AppSelectwithclass: "settings-select", for example theui_styleandplayer_bar_positionselectors incrates/pages/src/settings.rs.AppSelectsupplies typeahead, arrow-key navigation, and the shared visual style. This rawselectkeeps a different appearance inside the same page. The same pattern appears inMetadataSectionat lines 153-183.♻️ Proposed refactor for the download-quality dropdown
control: rsx! { - select { - class: "bg-white/10 text-white rounded-lg px-3 py-2 text-sm border border-white/10 focus:outline-none focus:border-white/25", - onchange: move |evt| { - config.write().offline_quality = OfflineQuality::from_value_str(&evt.value()); - }, - for q in OfflineQuality::ALL { - option { - value: q.value_str(), - selected: *q == config.read().offline_quality, - "{q.label()}" - } - } - } + AppSelect { + class: "settings-select", + value: config.read().offline_quality.value_str().to_string(), + options: OfflineQuality::ALL + .iter() + .map(|q| (q.value_str().to_string(), q.label().to_string())) + .collect::<Vec<_>>(), + on_change: move |value: String| { + config.write().offline_quality = OfflineQuality::from_value_str(&value); + }, + } }Add
AppSelectto the import list at line 1.🤖 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 `@crates/pages/src/settings/sections.rs` around lines 97 - 109, Replace the raw select for OfflineQuality in the settings section with the shared AppSelect component, adding the import and using class "settings-select" to match the ui_style, player_bar_position, and MetadataSection dropdowns. Preserve the existing OfflineQuality options, selected value, and onchange update behavior.crates/pages/src/settings/desktop_tools.rs (1)
62-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport an export failure to the user.
If
export_logsfails, the code writes atracing::warn!and the UI does not change. The user cannot tell whether the export succeeded. This panel is a diagnostic tool, so the log record is the least visible place to report the failure. Hold the result in a signal and render it next to the button.♻️ Proposed change to surface the export result
logs_sectionis a plain function, so it cannot calluse_signaldirectly. Convert it to a component first, then track the error:-#[cfg(not(target_os = "android"))] -pub(super) fn logs_section(mut config: Signal<AppConfig>) -> Element { +#[cfg(not(target_os = "android"))] +#[component] +pub(super) fn LogsSection(mut config: Signal<AppConfig>) -> Element { + let mut export_error = use_signal(|| Option::<String>::None); rsx! {onclick: move |_| { spawn(async move { if let Some(file) = rfd::AsyncFileDialog::new() .set_file_name("kopuz-logs.txt") .save_file() .await && let Err(e) = utils::logs::export_logs(file.path()) { tracing::warn!(error = %e, "failed to export logs"); + export_error.set(Some(e.to_string())); } }); }, i { class: "fa-solid fa-file-export" } "{i18n::t(\"export_logs\")}" }} + if let Some(err) = export_error() { + p { class: "px-5 pb-4 text-xs text-red-400", "{err}" } + } } } }Update the call site in
crates/pages/src/settings.rsat line 699 toLogsSection { config }, and rename the Android stub to match.🤖 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 `@crates/pages/src/settings/desktop_tools.rs` around lines 62 - 78, Convert logs_section into a component so it can use a signal to track export failures, and update its export_logs handler to store the error instead of only emitting tracing::warn!. Render the stored failure message beside the export button, update the call site to LogsSection { config }, and rename the Android stub consistently.
🤖 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 `@crates/pages/src/settings/desktop_tools.rs`:
- Around line 93-99: Remove the stale debug database panel doc comment
immediately above the Android `logs_section` stub, leaving the no-op
`#[cfg(target_os = "android")]` implementation unchanged.
In `@crates/pages/src/settings/navigation.rs`:
- Around line 31-38: In the navigation button markup, replace the aria_pressed
attribute with aria_current so the single active category is exposed as the
current navigation item. Preserve the existing is_selected state and selection
behavior in the button’s onclick handler.
- Around line 44-50: Update the styling for span.settings-fan-label in the
settings navigation so it uses pointer-events: none, allowing clicks on the
label area to reach the sibling button while preserving the existing label
content and positioning.
---
Outside diff comments:
In `@crates/pages/src/settings.rs`:
- Around line 697-702: Move the hooks::debug_db_section() invocation out of the
active_category() == SettingsCategory::Tools conditional so its Dioxus hooks
always execute in a stable parent scope, while keeping the debug section’s
visible placement or equivalent rendering behavior for the Tools category.
---
Nitpick comments:
In `@crates/pages/src/settings/desktop_tools.rs`:
- Around line 62-78: Convert logs_section into a component so it can use a
signal to track export failures, and update its export_logs handler to store the
error instead of only emitting tracing::warn!. Render the stored failure message
beside the export button, update the call site to LogsSection { config }, and
rename the Android stub consistently.
In `@crates/pages/src/settings/navigation.rs`:
- Around line 64-153: Update the SettingsFanItem definition and its call sites
in the settings navigation construction to derive label tilt from the existing
angle value instead of accepting a separate label_tilt prop. Remove label_tilt
from all eight SettingsFanItem initializers while preserving each item’s current
angle and rotation behavior.
In `@crates/pages/src/settings/sections.rs`:
- Around line 97-109: Replace the raw select for OfflineQuality in the settings
section with the shared AppSelect component, adding the import and using class
"settings-select" to match the ui_style, player_bar_position, and
MetadataSection dropdowns. Preserve the existing OfflineQuality options,
selected value, and onchange update behavior.
🪄 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: 799d4d4e-a483-44ed-803c-ee6be75bd2ed
📒 Files selected for processing (4)
crates/pages/src/settings.rscrates/pages/src/settings/desktop_tools.rscrates/pages/src/settings/navigation.rscrates/pages/src/settings/sections.rs
| /// Debug-build-only database panel: reset / load release DB / seed / re-run | ||
| /// import / vacuum / info, all against the disposable debug DB with a live | ||
| /// pool swap (no restart). English-only by design (dev tool). | ||
| #[cfg(target_os = "android")] | ||
| pub(super) fn logs_section(_config: Signal<AppConfig>) -> Element { | ||
| rsx! {} | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale doc comment on the Android stub.
The doc comment describes a debug database panel with reset, seed, vacuum, and pool-swap behavior. The item it annotates is the Android no-op stub for logs_section, which returns an empty rsx!. The comment appears to have been carried over from the inline debug_db_section implementation that this refactor removed. A reader of the Android build path gets a wrong description of the function.
As per coding guidelines: "Keep comments focused on the non-obvious reason why something exists; do not restate the code."
🧹 Proposed fix to drop the stale comment
-/// Debug-build-only database panel: reset / load release DB / seed / re-run
-/// import / vacuum / info, all against the disposable debug DB with a live
-/// pool swap (no restart). English-only by design (dev tool).
#[cfg(target_os = "android")]
pub(super) fn logs_section(_config: Signal<AppConfig>) -> Element {
rsx! {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Debug-build-only database panel: reset / load release DB / seed / re-run | |
| /// import / vacuum / info, all against the disposable debug DB with a live | |
| /// pool swap (no restart). English-only by design (dev tool). | |
| #[cfg(target_os = "android")] | |
| pub(super) fn logs_section(_config: Signal<AppConfig>) -> Element { | |
| rsx! {} | |
| } | |
| #[cfg(target_os = "android")] | |
| pub(super) fn logs_section(_config: Signal<AppConfig>) -> Element { | |
| rsx! {} | |
| } |
🤖 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 `@crates/pages/src/settings/desktop_tools.rs` around lines 93 - 99, Remove the
stale debug database panel doc comment immediately above the Android
`logs_section` stub, leaving the no-op `#[cfg(target_os = "android")]`
implementation unchanged.
Source: Coding guidelines
| button { | ||
| r#type: "button", | ||
| class: "settings-fan-item {selected_class}", | ||
| style: "--fan-angle: {angle};", | ||
| aria_label: "{label}", | ||
| aria_pressed: is_selected, | ||
| title: "{label}", | ||
| onclick: move |_| on_select.call(category), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use aria_current instead of aria_pressed for single-select navigation.
The buttons sit inside a nav and only one category can be active. aria_pressed describes a toggle button. Screen readers then announce each unselected category as "not pressed" rather than as an unselected navigation item. aria_current reports the active item correctly for this pattern.
♿ Proposed fix for the navigation state semantics
class: "settings-fan-item {selected_class}",
style: "--fan-angle: {angle};",
aria_label: "{label}",
- aria_pressed: is_selected,
+ aria_current: if is_selected { Some("page") } else { None },
title: "{label}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| button { | |
| r#type: "button", | |
| class: "settings-fan-item {selected_class}", | |
| style: "--fan-angle: {angle};", | |
| aria_label: "{label}", | |
| aria_pressed: is_selected, | |
| title: "{label}", | |
| onclick: move |_| on_select.call(category), | |
| button { | |
| r#type: "button", | |
| class: "settings-fan-item {selected_class}", | |
| style: "--fan-angle: {angle};", | |
| aria_label: "{label}", | |
| aria_current: if is_selected { Some("page") } else { None }, | |
| title: "{label}", | |
| onclick: move |_| on_select.call(category), |
🤖 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 `@crates/pages/src/settings/navigation.rs` around lines 31 - 38, In the
navigation button markup, replace the aria_pressed attribute with aria_current
so the single active category is exposed as the current navigation item.
Preserve the existing is_selected state and selection behavior in the button’s
onclick handler.
| span { | ||
| class: "settings-fan-label {selected_class}", | ||
| style: "--label-x: {label_x}; --label-y: {label_y}; --label-tilt: {label_tilt};", | ||
| aria_hidden: "true", | ||
| i { class: "fa-solid {icon}" } | ||
| span { "{label}" } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the fan navigation styles for pointer-event and hit-area handling.
set -euo pipefail
fd -t f -e css | while IFS= read -r f; do
rg -n -C 8 'settings-fan-label|settings-fan-item|settings-fan-disc' "$f"
doneRepository: Kopuz-org/kopuz
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate CSS files:"
fd -t f -e css . || true
echo
echo "Search settings-fan-label/fan/disc in tracked files:"
git ls-files | while IFS= read -r f; do
case "$f" in
*.css|*.rs|*.ron|*.toml|*.html|*.md)
if rg -n 'settings-fan-label|settings-fan-item|settings-fan-disc|settings-fan' "$f" >/dev/null 2>&1; then
echo "--- $f"
rg -n -C 6 'settings-fan-label|settings-fan-item|settings-fan-disc|settings-fan' "$f"
fi
;;
esac
done
echo
echo "Navigation file context:"
cat -n crates/pages/src/settings/navigation.rs | sed -n '1,120p'Repository: Kopuz-org/kopuz
Length of output: 15934
Make the fan label non-interactive so label clicks reach the button.
span.settings-fan-label currently has no pointer-events behavior, so clicks on the fan labels can be blocked from the sibling button action. Add pointer-events: none to .settings-fan-label, or ensure the button covers the label area.
🤖 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 `@crates/pages/src/settings/navigation.rs` around lines 44 - 50, Update the
styling for span.settings-fan-label in the settings navigation so it uses
pointer-events: none, allowing clicks on the label area to reach the sibling
button while preserving the existing label content and positioning.
2 participants
redesigned settings blah blah blah yeah frontend so i coped with the miss copex
Sanity Checking
rules.
contribution guidelines, or this pull request did not use AI assistance.
Style and Consistency
style.
cargo fmt --all --checkorcargo fmt --allas appropriate.cargo clippy --workspace --all-targets -- -D warnings, orexplained why it could not be run.
this change depends on them.
Testing
Tested on platform(s):
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin