fix: address 6 high-priority review findings (LEAK-001/002/004, BUG-001, THR-001, PERF-001) - #322
Merged
Merged
Conversation
…01, THR-001, PERF-001) LEAK-001: BatteryService — using(mo) on WMI ManagementObject foreach loops LEAK-002: ShortcutCleanerService — remove double ReleaseComObject LEAK-004: UninstallerViewModel — named handler + unsubscribe in Dispose BUG-001: WindowsFeaturesViewModel — NotifyCanExecuteChanged on IsBusy change THR-001: ProcessStatusToBrushConverter — freeze static brushes PERF-001: BoolToElevationBadgeBrushConverter — pre-created frozen brush instances Closes #303 (partial), Closes #304, Closes #306, Closes #307, Closes #309
📝 WalkthroughWalkthroughThis PR bundles six targeted fixes for v0.52.0 addressing resource leaks in WMI/COM operations, thread-safety gaps in UI converters, and improper event handler/command state management. Changes span BatteryService, ShortcutCleanerService, OutputKindToBrushConverter, UninstallerViewModel, WindowsFeaturesViewModel, and release notes. ChangesResource Management and UI Behavioral Fixes for v0.52.0
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SysManager/SysManager/Helpers/OutputKindToBrushConverter.cs (1)
89-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHexToBrushConverter still allocates non-frozen brushes per binding evaluation.
This keeps the hot-path allocation/thread-affinity problem unresolved for hex-based brushes. Cache and freeze converted brushes here as well.
💡 Proposed fix
+using System.Collections.Concurrent; using System.Globalization; using System.Windows; using System.Windows.Data; using System.Windows.Media; using SysManager.Models; @@ public class HexToBrushConverter : IValueConverter { + private static readonly ConcurrentDictionary<string, Brush> BrushCache = + new(StringComparer.OrdinalIgnoreCase); + public object Convert(object value, Type targetType, object parameter, CultureInfo culture) { if (value is string s && !string.IsNullOrWhiteSpace(s)) { - try { return new SolidColorBrush((Color)ColorConverter.ConvertFromString(s)); } + try + { + var key = s.Trim(); + return BrushCache.GetOrAdd(key, static k => + { + var brush = new SolidColorBrush((Color)ColorConverter.ConvertFromString(k)); + brush.Freeze(); + return brush; + }); + } catch { /* fall through */ } } return Brushes.Gray; }🤖 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/Helpers/OutputKindToBrushConverter.cs` around lines 89 - 97, The Convert method in OutputKindToBrushConverter currently allocates a new non-frozen SolidColorBrush on every binding evaluation; change it to use a shared, thread-safe cache of brushes keyed by the hex string (e.g., a static ConcurrentDictionary<string, SolidColorBrush>) and on first creation convert the color string with ColorConverter.ConvertFromString, construct the SolidColorBrush, call Freeze() on it, store the frozen brush in the cache, and return the cached instance thereafter; update the Convert method (and any helper methods in the OutputKindToBrushConverter class) to consult this cache and fall back to Brushes.Gray when parsing fails.
🤖 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 `@SysManager/SysManager/Helpers/OutputKindToBrushConverter.cs`:
- Around line 89-97: The Convert method in OutputKindToBrushConverter currently
allocates a new non-frozen SolidColorBrush on every binding evaluation; change
it to use a shared, thread-safe cache of brushes keyed by the hex string (e.g.,
a static ConcurrentDictionary<string, SolidColorBrush>) and on first creation
convert the color string with ColorConverter.ConvertFromString, construct the
SolidColorBrush, call Freeze() on it, store the frozen brush in the cache, and
return the cached instance thereafter; update the Convert method (and any helper
methods in the OutputKindToBrushConverter class) to consult this cache and fall
back to Brushes.Gray when parsing fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca0c0653-8eb2-422d-94f6-c37b1b38fd1e
📒 Files selected for processing (6)
CHANGELOG.mdSysManager/SysManager/Helpers/OutputKindToBrushConverter.csSysManager/SysManager/Services/BatteryService.csSysManager/SysManager/Services/ShortcutCleanerService.csSysManager/SysManager/ViewModels/UninstallerViewModel.csSysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs
📜 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 (6)
SysManager/SysManager/Services/ShortcutCleanerService.cs (1)
153-157: LGTM!SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs (1)
97-97: LGTM!Also applies to: 150-150
CHANGELOG.md (1)
9-23: LGTM!SysManager/SysManager/Services/BatteryService.cs (1)
31-46: LGTM!Also applies to: 67-71, 86-90, 105-109
SysManager/SysManager/ViewModels/UninstallerViewModel.cs (1)
20-21: LGTM!Also applies to: 36-37, 159-159
SysManager/SysManager/Helpers/OutputKindToBrushConverter.cs (1)
76-83: LGTM!Also applies to: 109-117
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced May 15, 2026
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…01, THR-001, PERF-001) (#322) LEAK-001: BatteryService — using(mo) on WMI ManagementObject foreach loops LEAK-002: ShortcutCleanerService — remove double ReleaseComObject LEAK-004: UninstallerViewModel — named handler + unsubscribe in Dispose BUG-001: WindowsFeaturesViewModel — NotifyCanExecuteChanged on IsBusy change THR-001: ProcessStatusToBrushConverter — freeze static brushes PERF-001: BoolToElevationBadgeBrushConverter — pre-created frozen brush instances Closes #303 (partial), Closes #304, Closes #306, Closes #307, Closes #309 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
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
2 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.
Closes #303 (partial), Closes #304, Closes #306, Closes #307, Closes #309. Build: 0 errors.
Summary by CodeRabbit
Release Notes v0.52.0
Bug Fixes
Performance