feat: theme customization with Dark/Light/Custom presets and persistence#563
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements a complete runtime theme management system for a WPF application. It introduces a ChangesDynamic Theme System with Live Switching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
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 |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SysManager/SysManager/Views/LogsView.xaml (1)
350-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse theme-bound foregrounds for expander text content.
These sections still hardcode text colors, which can reduce readability on Light/Custom themes even though surrounding containers are dynamic.
Suggested fix
- Foreground="`#C0C6CC`" + Foreground="{DynamicResource TextPrimary}" @@ - Foreground="`#8A9198`" + Foreground="{DynamicResource TextSecondary}"🤖 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 `@SysManager/SysManager/Views/LogsView.xaml` around lines 350 - 370, The TextBox controls bound to SelectedEntry.FullMessage and SelectedEntry.Xml currently use hardcoded Foreground colors ("`#C0C6CC`" and "`#8A9198`"); replace those literal hex values with theme-bound DynamicResource brushes (e.g., Foreground="{DynamicResource TextPrimary}" or "{DynamicResource TextSecondary}") so the Expander content respects light/dark/custom themes; update the TextBox elements (the ones inside the "Full system message" and "Raw XML (for power users)" Expanders) to use the appropriate DynamicResource brush instead of hardcoded colors.
🧹 Nitpick comments (1)
SysManager/SysManager/Services/ThemeService.cs (1)
269-269: ⚡ Quick winExpose preset defaults as read-only to prevent accidental mutation.
ThemePreset.Defaultsis publicly mutable (Line 269). Any caller can alter built-in presets at runtime and break companion mapping/theme assumptions.🔧 Suggested fix
- public static readonly Dictionary<string, ThemePreset> Defaults = new() + private static readonly Dictionary<string, ThemePreset> _defaults = new() { ... }; + + public static IReadOnlyDictionary<string, ThemePreset> Defaults => _defaults;🤖 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 `@SysManager/SysManager/Services/ThemeService.cs` at line 269, ThemePreset.Defaults is currently a publicly mutable Dictionary allowing callers to modify built-in presets; change the API to expose a read-only view: keep an internal/private mutable Dictionary (e.g., _defaults) and initialize it with the presets, then replace the public member Defaults with a public static IReadOnlyDictionary<string, ThemePreset> (or return new ReadOnlyDictionary<string, ThemePreset>(_defaults)); update any code that referenced Defaults to rely on the IReadOnlyDictionary interface and leave mutation operations to internal methods only.
🤖 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 `@SysManager/SysManager/MainWindow.xaml`:
- Around line 270-286: Replace the clickable Border used for the theme launcher
with a real Button so it is keyboard-accessible: change the element named
ThemeBtn to a Button (keep x:Name="ThemeBtn" and the existing ThemeBtn_Click
handler, ToolTip, Width/Height/CornerRadius styling), move the TextBlock into
the Button's Content, transfer the Border styling into the Button.Template or
Button.Style (preserve Background, BorderBrush, BorderThickness and the
IsMouseOver trigger to set BorderBrush to Accent), and ensure the Button is
focusable/tab-stop so keyboard users can invoke ThemeBtn_Click via Enter/Space.
In `@SysManager/SysManager/Services/ThemeService.cs`:
- Around line 217-218: The empty catch blocks in ThemeService.Save() and
ThemeService.Load() are hiding errors; replace them with a catch(Exception ex)
that records the failure (use the existing logging mechanism in ThemeService,
e.g., _logger.LogError(ex, "ThemeService.Save failed for theme {ThemeName}") or
similar) and then rethrow (or return a failure result) so callers and telemetry
see the issue; update both Save() and Load() to log the exception and not
silently swallow it.
- Around line 229-231: Clamp the persisted shade value before assigning it to
ShadePosition to prevent out-of-range values from corrupted JSON; replace the
direct assignment "ShadePosition = data.ShadePosition" with a clamped value
(e.g., ShadePosition = Math.Clamp(data.ShadePosition, 0, 100) or a manual
min/max clamp if Math.Clamp isn't available) in the same method where
CurrentMode and CurrentPresetId are set (the block assigning CurrentMode,
ShadePosition, CurrentPresetId in ThemeService/ThemeService.cs).
In `@SysManager/SysManager/Views/AppUpdatesView.xaml`:
- Around line 31-41: The view root is using a static resource for its background
(Background="{StaticResource PageBg}") so it won't update when the app theme
changes; update the root element's Background to use a dynamic resource (e.g.,
Background="{DynamicResource PageBg}") or another theme-aware DynamicResource so
the banner and entire page react to Light/Custom mode at runtime—look for the
Background attribute on the XAML root (Page/UserControl) and replace
StaticResource with DynamicResource referencing the PageBg key.
In `@SysManager/SysManager/Views/CleanupView.xaml`:
- Around line 30-40: The view's theme switching is broken because the root Grid
uses a static resource for its background; open CleanupView.xaml, find the root
Grid element that sets Background="{StaticResource PageBg}" and change it to use
a dynamic resource (Background="{DynamicResource PageBg}") so the UI updates
when themes change; ensure any other static PageBg usages in CleanupView.xaml
are also switched to DynamicResource to enable live Light/Custom theme updates.
In `@SysManager/SysManager/Views/DeepCleanupView.xaml`:
- Line 28: The background is hardcoded to "`#122030`" in DeepCleanupView.xaml
which prevents theme changes from applying; replace the literal
Background="`#122030`" with a DynamicResource (e.g., Background="{DynamicResource
SafetyCardBackground}") on the same element so it reads from the app theme, and
then add the SafetyCardBackground key to your theme ResourceDictionary files
(light/dark/custom) with appropriate colors so the card follows themes; keep
BorderBrush="{DynamicResource Accent}" as-is.
In `@SysManager/SysManager/Views/PingView.xaml`:
- Line 125: The template currently uses a hardcoded dark background color
(`#0D6366F1`) which can cause low-contrast text in some theme presets; replace
that literal color in PingView.xaml with a theme brush resource (e.g.,
DynamicResource ThemeCardBackground or CardBackground) and update both
occurrences (the element using `#0D6366F1` at the template locations around the
TextBlock bindings) to reference that resource; then add corresponding resource
entries in your theme/resource dictionaries (App.xaml or your light/dark
resource dictionaries) with appropriate colors for light and dark themes so the
TextBlock Foreground (already using theme brushes) contrasts correctly in all
presets.
In `@SysManager/SysManager/Views/PlaceholderView.xaml`:
- Line 22: The view uses DynamicResource for Foreground (TextPrimary) but the
page Background is a hardcoded dark gradient causing light-mode contrast issues;
update the Background attributes on the root container and the elements
referenced (the background at the locations corresponding to lines 22, 34, 38)
to use a theme-aware DynamicResource (for example BackgroundPrimary or
PageBackground) instead of the hardcoded gradient, ensuring you reference the
existing resource key used elsewhere in the app; locate the Background
attributes in PlaceholderView.xaml (root panel / background rectangles) and
replace the hardcoded value with DynamicResource referencing the shared theme
background resource.
In `@SysManager/SysManager/Views/PrivacyView.xaml`:
- Line 100: The row container elements in PrivacyView.xaml are using fixed dark
colors for Background and BorderBrush (while Foreground already uses
DynamicResource TextPrimary); update the Background and BorderBrush setters on
the item-row containers (e.g., the Border/Grid or ItemContainerStyle wrapping
each row) to use DynamicResource theme keys instead of hard-coded colors—use
your app's existing theme resources (for example
BackgroundPrimary/BackgroundSecondary/Divider/Border) via DynamicResource so the
rows adapt to light/dark themes and avoid contrast regressions.
In `@SysManager/SysManager/Views/ShortcutCleanerView.xaml`:
- Line 51: This view uses the theme-driven TextSecondary brush while the page
container still uses a fixed dark gradient background; update the container in
ShortcutCleanerView.xaml (the root page container element—e.g., Grid/Border that
holds the view) to use a theme-aware background resource instead of the
hard-coded dark gradient so contrast is preserved in light mode; ensure the
background binds to an existing theme resource (e.g.,
ThemeBackground/WindowBackground or similar) and verify TextSecondary remains a
DynamicResource so both foreground and background respond to theme changes.
In `@SysManager/SysManager/Views/SpeedTestView.xaml`:
- Around line 95-99: The explanatory panel currently only themes text and icon
colors; update the panel container (the surrounding Border/Grid that wraps the
TextBlock/Runs) to use DynamicResource-based brushes for Background and
BorderBrush (e.g., PanelBackground, PanelBorder or similar) instead of
hard-coded colors so it adapts to light/dark presets, and add those keys to the
theme ResourceDictionary(s) with appropriate light and dark values; locate the
container element that encloses the TextBlock (the explanatory panel) and
replace fixed color attributes with Background="{DynamicResource
PanelBackground}" and BorderBrush="{DynamicResource PanelBorder}" and ensure the
resource keys are defined in the app/theme resource dictionaries.
In `@SysManager/SysManager/Views/ThemePopup.xaml.cs`:
- Around line 25-34: Reorder OnLoaded so that SyncUiToService() runs before
BuildPresetCards() to ensure the UI mode/state is applied before presets are
constructed, and stop wiring events every time Loaded fires by moving the
subscriptions for DarkMode.Checked, LightMode.Checked, CustomMode.Checked and
ShadeSlider.ValueChanged out of OnLoaded into a one-time place (e.g., the
control's constructor) or guard them with a boolean like _eventsAttached;
reference OnLoaded, BuildPresetCards, SyncUiToService, Mode_Changed,
Shade_Changed, DarkMode.Checked, LightMode.Checked, CustomMode.Checked and
ShadeSlider.ValueChanged when locating where to change ordering and event
subscription behavior.
In `@SysManager/SysManager/Views/WindowsUpdateView.xaml`:
- Around line 39-49: The root Grid in WindowsUpdateView.xaml is using the local
StaticResource "PageBg" which prevents runtime theme updates; update the Grid's
Background to use a DynamicResource from the shared theme (or remove the local
LinearGradientBrush x:Key="PageBg" if unused) so the view participates in theme
changes—locate the root <Grid> in WindowsUpdateView.xaml and replace
Background="{StaticResource PageBg}" with a DynamicResource reference to your
theme background resource (or delete the local PageBg definition).
---
Outside diff comments:
In `@SysManager/SysManager/Views/LogsView.xaml`:
- Around line 350-370: The TextBox controls bound to SelectedEntry.FullMessage
and SelectedEntry.Xml currently use hardcoded Foreground colors ("`#C0C6CC`" and
"`#8A9198`"); replace those literal hex values with theme-bound DynamicResource
brushes (e.g., Foreground="{DynamicResource TextPrimary}" or "{DynamicResource
TextSecondary}") so the Expander content respects light/dark/custom themes;
update the TextBox elements (the ones inside the "Full system message" and "Raw
XML (for power users)" Expanders) to use the appropriate DynamicResource brush
instead of hardcoded colors.
---
Nitpick comments:
In `@SysManager/SysManager/Services/ThemeService.cs`:
- Line 269: ThemePreset.Defaults is currently a publicly mutable Dictionary
allowing callers to modify built-in presets; change the API to expose a
read-only view: keep an internal/private mutable Dictionary (e.g., _defaults)
and initialize it with the presets, then replace the public member Defaults with
a public static IReadOnlyDictionary<string, ThemePreset> (or return new
ReadOnlyDictionary<string, ThemePreset>(_defaults)); update any code that
referenced Defaults to rely on the IReadOnlyDictionary interface and leave
mutation operations to internal methods only.
🪄 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: 1d59298c-894b-471c-8bfd-eae17e69ef88
📒 Files selected for processing (38)
CHANGELOG.mdSysManager/SysManager/App.xamlSysManager/SysManager/App.xaml.csSysManager/SysManager/MainWindow.xamlSysManager/SysManager/MainWindow.xaml.csSysManager/SysManager/Services/ThemeService.csSysManager/SysManager/Views/AboutView.xamlSysManager/SysManager/Views/AppAlertsView.xamlSysManager/SysManager/Views/AppBlockerView.xamlSysManager/SysManager/Views/AppUpdatesView.xamlSysManager/SysManager/Views/BatteryHealthView.xamlSysManager/SysManager/Views/BulkInstallerView.xamlSysManager/SysManager/Views/CleanupView.xamlSysManager/SysManager/Views/ContextMenuView.xamlSysManager/SysManager/Views/DashboardView.xamlSysManager/SysManager/Views/DeepCleanupView.xamlSysManager/SysManager/Views/DiskAnalyzerView.xamlSysManager/SysManager/Views/DnsHostsView.xamlSysManager/SysManager/Views/DriversView.xamlSysManager/SysManager/Views/DuplicateFileView.xamlSysManager/SysManager/Views/FileShredderView.xamlSysManager/SysManager/Views/LogsView.xamlSysManager/SysManager/Views/NetworkRepairView.xamlSysManager/SysManager/Views/PerformanceView.xamlSysManager/SysManager/Views/PingView.xamlSysManager/SysManager/Views/PlaceholderView.xamlSysManager/SysManager/Views/PrivacyView.xamlSysManager/SysManager/Views/ProcessManagerView.xamlSysManager/SysManager/Views/ServicesView.xamlSysManager/SysManager/Views/ShortcutCleanerView.xamlSysManager/SysManager/Views/SpeedTestView.xamlSysManager/SysManager/Views/StartupView.xamlSysManager/SysManager/Views/SystemHealthView.xamlSysManager/SysManager/Views/ThemePopup.xamlSysManager/SysManager/Views/ThemePopup.xaml.csSysManager/SysManager/Views/UninstallerView.xamlSysManager/SysManager/Views/WindowsFeaturesView.xamlSysManager/SysManager/Views/WindowsUpdateView.xaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (33)
CHANGELOG.md (1)
9-23: LGTM!SysManager/SysManager/App.xaml.cs (1)
79-79: LGTM!SysManager/SysManager/App.xaml (1)
43-47: LGTM!Also applies to: 64-66, 90-115, 121-121, 128-129, 134-134, 137-137, 145-147, 156-156, 160-160, 172-172, 192-192, 198-199, 212-212, 219-220, 228-228, 240-240, 242-242, 245-245, 248-248, 250-250, 285-285, 425-425, 437-445, 460-460, 463-463, 477-477, 484-484, 491-491, 497-497, 502-502, 517-517, 536-536, 549-550, 553-553, 555-555, 568-568, 574-574, 581-581, 592-594, 609-610, 614-614, 629-629, 648-649, 652-652, 667-667, 671-673, 679-680, 684-684, 708-708, 725-726, 745-745, 753-753, 760-760, 778-778
SysManager/SysManager/Views/ThemePopup.xaml (1)
1-162: LGTM!SysManager/SysManager/MainWindow.xaml.cs (1)
137-142: LGTM!SysManager/SysManager/Views/AboutView.xaml (1)
34-34: LGTM!Also applies to: 42-42, 44-44, 47-47, 77-77, 88-88, 90-95, 123-123, 125-125, 141-141, 166-166, 172-175, 177-177, 191-191
SysManager/SysManager/Views/AppAlertsView.xaml (1)
22-24: LGTM!Also applies to: 43-43, 58-58
SysManager/SysManager/Views/AppBlockerView.xaml (1)
10-10: LGTM!Also applies to: 29-30, 37-39, 82-82, 96-96
SysManager/SysManager/Views/BatteryHealthView.xaml (1)
10-10: LGTM!Also applies to: 47-47, 100-100
SysManager/SysManager/Views/BulkInstallerView.xaml (1)
10-10: LGTM!Also applies to: 30-30, 38-40, 73-73, 93-93, 102-102, 122-122, 135-135, 139-139, 170-171, 215-215, 224-224, 232-232
SysManager/SysManager/Views/ContextMenuView.xaml (1)
10-10: LGTM!Also applies to: 103-103, 115-115, 126-126
SysManager/SysManager/Views/DashboardView.xaml (1)
91-93: LGTM!Also applies to: 107-110, 119-135, 157-158, 163-164, 169-175, 182-184, 193-193, 203-203, 208-212, 221-223, 239-241, 265-267, 279-281, 297-299, 313-315, 320-325, 328-332, 335-339, 344-347, 361-361
SysManager/SysManager/Views/DeepCleanupView.xaml (1)
31-34: LGTM!Also applies to: 65-67, 84-85, 94-95, 111-113, 135-136, 163-169, 185-190
SysManager/SysManager/Views/DiskAnalyzerView.xaml (1)
10-10: LGTM!Also applies to: 61-61, 83-83, 117-120
SysManager/SysManager/Views/DnsHostsView.xaml (1)
28-28: LGTM!Also applies to: 36-39, 67-68, 123-124
SysManager/SysManager/Views/DriversView.xaml (1)
10-10: LGTM!Also applies to: 72-73, 84-85, 95-96
SysManager/SysManager/Views/DuplicateFileView.xaml (1)
10-10: LGTM!Also applies to: 78-79, 85-86, 89-90, 116-118
SysManager/SysManager/Views/FileShredderView.xaml (1)
53-53: LGTM!SysManager/SysManager/Views/LogsView.xaml (1)
160-161: LGTM!Also applies to: 165-166, 170-171, 175-176, 180-181, 188-189, 233-239, 271-272, 278-278, 300-302, 323-325, 337-338, 342-343, 350-353, 363-366
SysManager/SysManager/Views/NetworkRepairView.xaml (1)
25-26: LGTM!Also applies to: 33-36, 63-64, 89-90, 115-116, 141-142
SysManager/SysManager/Views/PerformanceView.xaml (1)
10-10: LGTM!Also applies to: 30-30, 38-40, 76-80, 201-202, 232-232, 250-254
SysManager/SysManager/Views/PingView.xaml (1)
63-63: LGTM!SysManager/SysManager/Views/PlaceholderView.xaml (1)
17-17: LGTM!Also applies to: 29-29
SysManager/SysManager/Views/PrivacyView.xaml (1)
10-10: LGTM!Also applies to: 29-29, 37-39
SysManager/SysManager/Views/ProcessManagerView.xaml (1)
10-10: LGTM!Also applies to: 38-38, 72-72, 87-87, 123-123, 139-139, 180-180
SysManager/SysManager/Views/ServicesView.xaml (1)
10-10: LGTM!Also applies to: 29-29, 37-39, 79-79
SysManager/SysManager/Views/SpeedTestView.xaml (1)
55-57: LGTM!Also applies to: 80-82
SysManager/SysManager/Views/StartupView.xaml (1)
10-10: LGTM!Also applies to: 38-38, 80-80, 92-92
SysManager/SysManager/Views/SystemHealthView.xaml (2)
167-172: LGTM!Also applies to: 216-224, 258-261, 270-273, 295-297, 300-303, 305-311
30-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix SystemHealthView page background to support live theme switching
SystemHealthViewstill prevents full runtime theme updates because the root<Grid>background is hard-coded: Line 14 usesBackground="{StaticResource PageBg}", where Line 7 definesPageBgwith fixed gradient stop colors.Suggested fix
- <Grid Background="{StaticResource PageBg}"> + <Grid Background="{DynamicResource Surface0}">- <UserControl.Resources> - <LinearGradientBrush x:Key="PageBg" StartPoint="0,0" EndPoint="1,1"> - <GradientStop Color="`#070A0F`" Offset="0"/> - <GradientStop Color="`#0B1220`" Offset="0.5"/> - <GradientStop Color="`#090D16`" Offset="1"/> - </LinearGradientBrush> - </UserControl.Resources> + <!-- Remove local fixed PageBg if no longer used -->> Likely an incorrect or invalid review comment.SysManager/SysManager/Views/UninstallerView.xaml (1)
10-10: LGTM!Also applies to: 30-30, 38-40, 75-75, 128-128, 143-143, 155-155, 184-184
SysManager/SysManager/Views/WindowsFeaturesView.xaml (1)
10-10: LGTM!Also applies to: 42-42, 67-67, 75-77, 118-118, 161-161
SysManager/SysManager/Views/WindowsUpdateView.xaml (1)
161-161: LGTM!Also applies to: 172-172, 193-193, 203-203
Comment on lines
+270
to
+286
| <Border x:Name="ThemeBtn" Width="36" Height="36" CornerRadius="10" | ||
| Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}" | ||
| BorderThickness="1" Cursor="Hand" MouseLeftButtonUp="ThemeBtn_Click" | ||
| ToolTip="Appearance"> | ||
| <Border.Style> | ||
| <Style TargetType="Border"> | ||
| <Style.Triggers> | ||
| <Trigger Property="IsMouseOver" Value="True"> | ||
| <Setter Property="BorderBrush" Value="{DynamicResource Accent}"/> | ||
| </Trigger> | ||
| </Style.Triggers> | ||
| </Style> | ||
| </Border.Style> | ||
| <TextBlock Text="" FontFamily="Segoe Fluent Icons,Segoe MDL2 Assets" | ||
| FontSize="16" Foreground="{DynamicResource TextSecondary}" | ||
| HorizontalAlignment="Center" VerticalAlignment="Center"/> | ||
| </Border> |
There was a problem hiding this comment.
Use a real Button for the theme trigger (keyboard accessibility).
The theme launcher is a clickable Border (Line 270-273), so it is not keyboard-invokable out of the box. This blocks non-mouse users from opening the theme popup.
🔧 Suggested fix
-<Border x:Name="ThemeBtn" Width="36" Height="36" CornerRadius="10"
- Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}"
- BorderThickness="1" Cursor="Hand" MouseLeftButtonUp="ThemeBtn_Click"
- ToolTip="Appearance">
+<Button x:Name="ThemeBtn"
+ Width="36" Height="36"
+ Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}"
+ BorderThickness="1"
+ Click="ThemeBtn_Click"
+ ToolTip="Appearance"
+ Style="{StaticResource SecondaryButton}">
...
-</Border>
+</Button>🤖 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 `@SysManager/SysManager/MainWindow.xaml` around lines 270 - 286, Replace the
clickable Border used for the theme launcher with a real Button so it is
keyboard-accessible: change the element named ThemeBtn to a Button (keep
x:Name="ThemeBtn" and the existing ThemeBtn_Click handler, ToolTip,
Width/Height/CornerRadius styling), move the TextBlock into the Button's
Content, transfer the Border styling into the Button.Template or Button.Style
(preserve Background, BorderBrush, BorderThickness and the IsMouseOver trigger
to set BorderBrush to Accent), and ensure the Button is focusable/tab-stop so
keyboard users can invoke ThemeBtn_Click via Enter/Space.
Comment on lines
+217
to
+218
| catch { } | ||
| } |
There was a problem hiding this comment.
Avoid silent failure on theme persistence/load errors.
Both Save() and Load() swallow all exceptions (Line 217, Line 248). This hides broken persistence and makes support/debugging difficult.
🔧 Suggested fix
- catch { }
+ catch (Exception ex)
+ {
+ LogService.Logger?.Warning(ex, "Failed to save theme settings to {Path}", SettingsPath);
+ }
...
- catch { }
+ catch (Exception ex)
+ {
+ LogService.Logger?.Warning(ex, "Failed to load theme settings from {Path}", SettingsPath);
+ }Also applies to: 248-249
🤖 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 `@SysManager/SysManager/Services/ThemeService.cs` around lines 217 - 218, The
empty catch blocks in ThemeService.Save() and ThemeService.Load() are hiding
errors; replace them with a catch(Exception ex) that records the failure (use
the existing logging mechanism in ThemeService, e.g., _logger.LogError(ex,
"ThemeService.Save failed for theme {ThemeName}") or similar) and then rethrow
(or return a failure result) so callers and telemetry see the issue; update both
Save() and Load() to log the exception and not silently swallow it.
Comment on lines
+229
to
+231
| CurrentMode = data.Mode; | ||
| ShadePosition = data.ShadePosition; | ||
| CurrentPresetId = data.PresetId; |
There was a problem hiding this comment.
Clamp persisted shade value before applying it.
ShadePosition is read from disk without bounds checking (Line 230). If the JSON is edited/corrupted, negative or very large values can produce invalid darken/lighten math and unexpected colors.
🔧 Suggested fix
- ShadePosition = data.ShadePosition;
+ ShadePosition = Math.Clamp(data.ShadePosition, 0d, 1d);📝 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.
Suggested change
| CurrentMode = data.Mode; | |
| ShadePosition = data.ShadePosition; | |
| CurrentPresetId = data.PresetId; | |
| CurrentMode = data.Mode; | |
| ShadePosition = Math.Clamp(data.ShadePosition, 0d, 1d); | |
| CurrentPresetId = data.PresetId; |
🤖 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 `@SysManager/SysManager/Services/ThemeService.cs` around lines 229 - 231, Clamp
the persisted shade value before assigning it to ShadePosition to prevent
out-of-range values from corrupted JSON; replace the direct assignment
"ShadePosition = data.ShadePosition" with a clamped value (e.g., ShadePosition =
Math.Clamp(data.ShadePosition, 0, 100) or a manual min/max clamp if Math.Clamp
isn't available) in the same method where CurrentMode and CurrentPresetId are
set (the block assigning CurrentMode, ShadePosition, CurrentPresetId in
ThemeService/ThemeService.cs).
Comment on lines
+31
to
+41
| <Border Grid.Row="1" Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}" | ||
| BorderThickness="1" CornerRadius="8" Padding="14,12" Margin="28,12,28,0" | ||
| Visibility="{Binding IsElevated, Converter={StaticResource FlexVis}, ConverterParameter=Inverse}"> | ||
| <DockPanel> | ||
| <Button Content="Run as administrator" Command="{Binding RelaunchAsAdminCommand}" | ||
| Style="{StaticResource AdminButton}" DockPanel.Dock="Right" Padding="14,8"/> | ||
| <StackPanel Orientation="Horizontal" VerticalAlignment="Center"> | ||
| <TextBlock Text="" FontFamily="Segoe Fluent Icons,Segoe MDL2 Assets" | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{StaticResource TextSecondary}"/> | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{DynamicResource TextSecondary}"/> | ||
| <TextBlock Text="Some package upgrades require administrator privileges for system-wide installations." | ||
| Foreground="{StaticResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> | ||
| Foreground="{DynamicResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> |
There was a problem hiding this comment.
Root view still pinned to a static dark background.
These dynamic banner updates won’t fully theme at runtime because the page root remains Background="{StaticResource PageBg}" (Line 14) with hardcoded dark colors. This view will stay visually dark in Light/Custom mode.
Suggested fix
- <UserControl.Resources>
- <LinearGradientBrush x:Key="PageBg" StartPoint="0,0" EndPoint="1,1">
- <GradientStop Color="`#070A0F`" Offset="0"/>
- <GradientStop Color="`#0B1220`" Offset="0.5"/>
- <GradientStop Color="`#090D16`" Offset="1"/>
- </LinearGradientBrush>
- </UserControl.Resources>
-
- <Grid Background="{StaticResource PageBg}">
+ <Grid Background="{DynamicResource Surface0}">🤖 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 `@SysManager/SysManager/Views/AppUpdatesView.xaml` around lines 31 - 41, The
view root is using a static resource for its background
(Background="{StaticResource PageBg}") so it won't update when the app theme
changes; update the root element's Background to use a dynamic resource (e.g.,
Background="{DynamicResource PageBg}") or another theme-aware DynamicResource so
the banner and entire page react to Light/Custom mode at runtime—look for the
Background attribute on the XAML root (Page/UserControl) and replace
StaticResource with DynamicResource referencing the PageBg key.
Comment on lines
+30
to
+40
| <Border Grid.Row="1" Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}" | ||
| BorderThickness="1" CornerRadius="8" Padding="14,12" Margin="28,12,28,0" | ||
| Visibility="{Binding IsElevated, Converter={StaticResource FlexVis}, ConverterParameter=Inverse}"> | ||
| <DockPanel> | ||
| <Button Content="Run as administrator" Command="{Binding RelaunchAsAdminCommand}" | ||
| Style="{StaticResource AdminButton}" DockPanel.Dock="Right" Padding="14,8"/> | ||
| <StackPanel Orientation="Horizontal" VerticalAlignment="Center"> | ||
| <TextBlock Text="" FontFamily="Segoe Fluent Icons,Segoe MDL2 Assets" | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{StaticResource TextSecondary}"/> | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{DynamicResource TextSecondary}"/> | ||
| <TextBlock Text="Running SFC and DISM system repairs requires administrator privileges." | ||
| Foreground="{StaticResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> | ||
| Foreground="{DynamicResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> |
There was a problem hiding this comment.
Dynamic banner updates are undermined by a static page background.
This section is theme-aware now, but the root grid still uses Background="{StaticResource PageBg}" (Line 14) with fixed dark colors, so live Light/Custom switching remains incomplete for this view.
Suggested fix
- <UserControl.Resources>
- <LinearGradientBrush x:Key="PageBg" StartPoint="0,0" EndPoint="1,1">
- <GradientStop Color="`#070A0F`" Offset="0"/>
- <GradientStop Color="`#0B1220`" Offset="0.5"/>
- <GradientStop Color="`#090D16`" Offset="1"/>
- </LinearGradientBrush>
- </UserControl.Resources>
-
- <Grid Background="{StaticResource PageBg}">
+ <Grid Background="{DynamicResource Surface0}">🤖 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 `@SysManager/SysManager/Views/CleanupView.xaml` around lines 30 - 40, The
view's theme switching is broken because the root Grid uses a static resource
for its background; open CleanupView.xaml, find the root Grid element that sets
Background="{StaticResource PageBg}" and change it to use a dynamic resource
(Background="{DynamicResource PageBg}") so the UI updates when themes change;
ensure any other static PageBg usages in CleanupView.xaml are also switched to
DynamicResource to enable live Light/Custom theme updates.
| <DataTemplate> | ||
| <TextBlock Text="{Binding Name}" FontWeight="Bold" FontSize="14" | ||
| Foreground="{StaticResource TextPrimary}" | ||
| Foreground="{DynamicResource TextPrimary}" |
There was a problem hiding this comment.
Use theme resources for item-row backgrounds to avoid contrast regressions.
On Line 100, Line 131, and Line 134 text is theme-driven, but the row container still uses fixed dark colors. In light mode this can produce low contrast. Convert those row background/border setters to DynamicResource values.
Suggested fix
- <Setter Property="Background" Value="`#0A0E14`"/>
- <Setter Property="BorderBrush" Value="`#1A1F2E`"/>
+ <Setter Property="Background" Value="{DynamicResource Surface2}"/>
+ <Setter Property="BorderBrush" Value="{DynamicResource Border1}"/>
- <Setter Property="Background" Value="`#10141D`"/>
- <Setter Property="BorderBrush" Value="`#2A3244`"/>
+ <Setter Property="Background" Value="{DynamicResource Surface3}"/>
+ <Setter Property="BorderBrush" Value="{DynamicResource Border2}"/>Also applies to: 131-131, 134-134
🤖 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 `@SysManager/SysManager/Views/PrivacyView.xaml` at line 100, The row container
elements in PrivacyView.xaml are using fixed dark colors for Background and
BorderBrush (while Foreground already uses DynamicResource TextPrimary); update
the Background and BorderBrush setters on the item-row containers (e.g., the
Border/Grid or ItemContainerStyle wrapping each row) to use DynamicResource
theme keys instead of hard-coded colors—use your app's existing theme resources
(for example BackgroundPrimary/BackgroundSecondary/Divider/Border) via
DynamicResource so the rows adapt to light/dark themes and avoid contrast
regressions.
| Style="{StaticResource GhostButton}" Margin="0,0,16,0"/> | ||
| <CheckBox Content="Move to Recycle Bin" IsChecked="{Binding MoveToRecycleBin}" | ||
| VerticalAlignment="Center" Foreground="{StaticResource TextSecondary}"/> | ||
| VerticalAlignment="Center" Foreground="{DynamicResource TextSecondary}"/> |
There was a problem hiding this comment.
Avoid mixed themed text on fixed dark page backgrounds.
On Line 51, TextSecondary is now theme-driven, but this view still uses a fixed dark gradient page background. In light mode this can reduce contrast. Use a theme resource background for the page container.
Suggested fix
- <Grid Background="{StaticResource PageBg}">
+ <Grid Background="{DynamicResource Surface0}">🤖 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 `@SysManager/SysManager/Views/ShortcutCleanerView.xaml` at line 51, This view
uses the theme-driven TextSecondary brush while the page container still uses a
fixed dark gradient background; update the container in ShortcutCleanerView.xaml
(the root page container element—e.g., Grid/Border that holds the view) to use a
theme-aware background resource instead of the hard-coded dark gradient so
contrast is preserved in light mode; ensure the background binds to an existing
theme resource (e.g., ThemeBackground/WindowBackground or similar) and verify
TextSecondary remains a DynamicResource so both foreground and background
respond to theme changes.
Comment on lines
+95
to
99
| FontSize="14" Foreground="{DynamicResource Info}" VerticalAlignment="Top" Margin="0,0,10,0"/> | ||
| <TextBlock TextWrapping="Wrap" Foreground="{DynamicResource TextSecondary}" FontSize="12"> | ||
| <Run FontWeight="SemiBold" Foreground="{DynamicResource TextPrimary}">Ookla</Run><Run> measures raw TCP throughput to a nearby server — most accurate for ISP bandwidth. </Run> | ||
| <Run FontWeight="SemiBold" Foreground="{DynamicResource TextPrimary}">HTTP</Run><Run> tests download/upload via Cloudflare CDN — reflects real web browsing speed but may show lower results due to HTTP overhead and CDN routing.</Run> | ||
| </TextBlock> |
There was a problem hiding this comment.
Theme the explanatory panel container, not just its text colors.
On Line 95 to Line 99, text and icon now use DynamicResource, but the panel container keeps fixed dark colors. In light presets this can create poor contrast. Move the panel background/border to theme resources as well.
Suggested fix
- <Border Grid.Row="1" Grid.ColumnSpan="2" Background="`#0F1A2E`" BorderBrush="`#1E3A5F`" BorderThickness="1"
+ <Border Grid.Row="1" Grid.ColumnSpan="2" Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}" BorderThickness="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 `@SysManager/SysManager/Views/SpeedTestView.xaml` around lines 95 - 99, The
explanatory panel currently only themes text and icon colors; update the panel
container (the surrounding Border/Grid that wraps the TextBlock/Runs) to use
DynamicResource-based brushes for Background and BorderBrush (e.g.,
PanelBackground, PanelBorder or similar) instead of hard-coded colors so it
adapts to light/dark presets, and add those keys to the theme
ResourceDictionary(s) with appropriate light and dark values; locate the
container element that encloses the TextBlock (the explanatory panel) and
replace fixed color attributes with Background="{DynamicResource
PanelBackground}" and BorderBrush="{DynamicResource PanelBorder}" and ensure the
resource keys are defined in the app/theme resource dictionaries.
Comment on lines
+25
to
+34
| private void OnLoaded(object sender, RoutedEventArgs e) | ||
| { | ||
| BuildPresetCards(); | ||
| SyncUiToService(); | ||
|
|
||
| DarkMode.Checked += Mode_Changed; | ||
| LightMode.Checked += Mode_Changed; | ||
| CustomMode.Checked += Mode_Changed; | ||
| ShadeSlider.ValueChanged += Shade_Changed; | ||
| } |
There was a problem hiding this comment.
Fix OnLoaded ordering and one-time event wiring.
Current flow builds cards before syncing mode, so a saved light/custom state can show dark presets initially (Line 27 vs Line 99-104). Also, handlers are attached on every Loaded (Line 30-33), which can duplicate callbacks.
🔧 Suggested fix
+ private bool _initialized;
+
private void OnLoaded(object sender, RoutedEventArgs e)
{
- BuildPresetCards();
SyncUiToService();
-
- DarkMode.Checked += Mode_Changed;
- LightMode.Checked += Mode_Changed;
- CustomMode.Checked += Mode_Changed;
- ShadeSlider.ValueChanged += Shade_Changed;
+ BuildPresetCards();
+
+ if (_initialized) return;
+ DarkMode.Checked += Mode_Changed;
+ LightMode.Checked += Mode_Changed;
+ CustomMode.Checked += Mode_Changed;
+ ShadeSlider.ValueChanged += Shade_Changed;
+ _initialized = true;
}Also applies to: 37-47, 91-105
🤖 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 `@SysManager/SysManager/Views/ThemePopup.xaml.cs` around lines 25 - 34, Reorder
OnLoaded so that SyncUiToService() runs before BuildPresetCards() to ensure the
UI mode/state is applied before presets are constructed, and stop wiring events
every time Loaded fires by moving the subscriptions for DarkMode.Checked,
LightMode.Checked, CustomMode.Checked and ShadeSlider.ValueChanged out of
OnLoaded into a one-time place (e.g., the control's constructor) or guard them
with a boolean like _eventsAttached; reference OnLoaded, BuildPresetCards,
SyncUiToService, Mode_Changed, Shade_Changed, DarkMode.Checked,
LightMode.Checked, CustomMode.Checked and ShadeSlider.ValueChanged when locating
where to change ordering and event subscription behavior.
Comment on lines
+39
to
+49
| <Border Grid.Row="1" Background="{DynamicResource Surface2}" BorderBrush="{DynamicResource Border1}" | ||
| BorderThickness="1" CornerRadius="8" Padding="14,12" Margin="28,12,28,0" | ||
| Visibility="{Binding IsElevated, Converter={StaticResource FlexVis}, ConverterParameter=Inverse}"> | ||
| <DockPanel> | ||
| <Button Content="Run as administrator" Command="{Binding RelaunchAsAdminCommand}" | ||
| Style="{StaticResource AdminButton}" DockPanel.Dock="Right" Padding="14,8"/> | ||
| <StackPanel Orientation="Horizontal" VerticalAlignment="Center"> | ||
| <TextBlock Text="" FontFamily="Segoe Fluent Icons,Segoe MDL2 Assets" | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{StaticResource TextSecondary}"/> | ||
| FontSize="16" Margin="0,0,10,0" Foreground="{DynamicResource TextSecondary}"/> | ||
| <TextBlock Text="Listing and installing Windows updates requires administrator privileges." | ||
| Foreground="{StaticResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> | ||
| Foreground="{DynamicResource TextSecondary}" FontSize="13" VerticalAlignment="Center"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining fixed page background in this file.
rg -n 'x:Key="PageBg"|Background="\{StaticResource PageBg\}"' SysManager/SysManager/Views/WindowsUpdateView.xamlRepository: laurentiu021/SystemManager
Length of output: 201
Fix WindowsUpdateView root background to participate in theme changes
The root <Grid> still uses {StaticResource PageBg} (local LinearGradientBrush x:Key="PageBg"), so the page background won’t update during runtime theme changes. Switch the root background to a {DynamicResource ...} from the theme (or remove the fixed local PageBg if no longer needed).
🤖 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 `@SysManager/SysManager/Views/WindowsUpdateView.xaml` around lines 39 - 49, The
root Grid in WindowsUpdateView.xaml is using the local StaticResource "PageBg"
which prevents runtime theme updates; update the Grid's Background to use a
DynamicResource from the shared theme (or remove the local LinearGradientBrush
x:Key="PageBg" if unused) so the view participates in theme changes—locate the
root <Grid> in WindowsUpdateView.xaml and replace Background="{StaticResource
PageBg}" with a DynamicResource reference to your theme background resource (or
delete the local PageBg definition).
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
3 participants
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
%AppData%/SysManager/theme.jsonbetween sessionsStaticResourcereferences converted toDynamicResourcefor live switchingDark presets
Midnight Indigo, Deep Ocean, Dark Forest, Neon Rose, Violet Night, Warm Ember
Light presets
Clean Indigo, Sky Breeze, Warm Sand, Mint Fresh, Soft Blossom, Lavender
Technical changes
ThemeServicesingleton (load/save/apply theme at runtime)ThemePopupUserControl (popup UI with mode tabs, presets, slider, custom inputs)ThemePresetrecord with full color definitions for each presetTest plan
Summary by CodeRabbit