fix: audit findings — memory leaks, silent catches, UI flicker, consistency#601
Merged
Conversation
…stency - AboutViewModel: dispose ManagementObject in all 5 WMI loops - ThemeService: log errors instead of empty catch blocks - DashboardViewModel: replace bare catch(Exception) with logged exceptions - BulkInstallerViewModel: FilteredApps → BulkObservableCollection - ShortcutCleanerView: DataGrid Background/BorderThickness fix
📝 WalkthroughWalkthroughVersion 1.17.2 patch adds exception logging to theme service and dashboard alert scanners, wraps WMI ManagementObject queries in using blocks to fix memory leaks, converts BulkInstallerViewModel.FilteredApps to BulkObservableCollection with batch update support, and updates build metadata and changelog documentation. ChangesRelease 1.17.2 maintenance patch
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
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}
| File.WriteAllText(SettingsPath, json); | ||
| } | ||
| catch { } | ||
| catch (Exception ex) { Log.Debug("Theme save failed: {Error}", ex.Message); } |
| } | ||
| } | ||
| catch { } | ||
| catch (Exception ex) { Log.Debug("Theme load failed: {Error}", ex.Message); } |
| } | ||
| catch (Exception) | ||
| catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// | ||
| { |
| } | ||
| catch (Exception) | ||
| catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// | ||
| { |
| } | ||
| catch (Exception) | ||
| catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// | ||
| { |
![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 (3)
SysManager/SysManager/ViewModels/DashboardViewModel.cs (3)
461-468:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: fallback block is not inside the catch.
Identical issue to lines 373 and 428. The catch at line 461 contains only the
Log.Debugcall; lines 462-468 form an unconditional block that always runs, forcing the Windows features alert to "unavailable" even when the scan succeeds.🐛 Proposed fix: move fallback code inside catch block
- catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// - { + catch (Exception ex) + { + Log.Debug("Alert scan failed: {Error}", ex.Message); System.Windows.Application.Current?.Dispatcher.BeginInvoke(() => { alert.Title = "Feature check unavailable";🤖 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/ViewModels/DashboardViewModel.cs` around lines 461 - 468, The fallback block that sets alert.Title and alert.Severity is currently outside the catch and always executes; move that Dispatcher.BeginInvoke block inside the catch for the exception handler so the fallback runs only when the catch in DashboardViewModel (the catch handling "Alert scan failed") is triggered—i.e., ensure the catch block contains both Log.Debug("Alert scan failed: {Error}", ex.Message) and the System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) block so the "Feature check unavailable" alert is only set on exceptions.
428-435:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: fallback block is not inside the catch.
Identical issue to line 373. The catch at line 428 contains only the
Log.Debugcall; lines 429-435 form an unconditional block that always runs, forcing the Event Log alert to "unavailable" even when the scan succeeds.🐛 Proposed fix: move fallback code inside catch block
- catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// - { + catch (Exception ex) + { + Log.Debug("Alert scan failed: {Error}", ex.Message); System.Windows.Application.Current?.Dispatcher.BeginInvoke(() => { alert.Title = "Event Log check unavailable";🤖 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/ViewModels/DashboardViewModel.cs` around lines 428 - 435, The fallback block that sets alert.Title and alert.Severity is placed outside the catch and thus always executes; move the block currently after the catch (the System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) that sets alert.Title = "Event Log check unavailable" and alert.Severity = AlertSeverity.Green) inside the catch that contains Log.Debug("Alert scan failed: {Error}", ex.Message) so that it only runs when an exception occurs, and remove the stray unconditional braces; locate this in DashboardViewModel.cs around the catch that logs via Log.Debug and wrap the Dispatcher.BeginInvoke fallback inside that catch.
373-380:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical syntax error: fallback block is not inside the catch.
The catch statement at line 373 is a single-line catch containing only the
Log.Debugcall. The//comment terminates the statement. Lines 374-380 form a separate, unconditional block that always executes, overwriting successful scan results with "App update check unavailable."This breaks the app update alert — it will never display actual update counts.
🐛 Proposed fix: move fallback code inside catch block
- catch (Exception ex) { Log.Debug("Alert scan failed: {Error}", ex.Message); }// - { + catch (Exception ex) + { + Log.Debug("Alert scan failed: {Error}", ex.Message); System.Windows.Application.Current?.Dispatcher.BeginInvoke(() => { alert.Title = "App update check unavailable";🤖 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/ViewModels/DashboardViewModel.cs` around lines 373 - 380, The fallback block that sets alert.Title and alert.Severity is currently outside the catch (the catch currently only contains Log.Debug("Alert scan failed: {Error}", ex.Message);) so it always runs; move the System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) block (the lines that set alert.Title = "App update check unavailable" and alert.Severity = AlertSeverity.Green) into the catch(Exception ex) body (and remove the stray trailing comment marker) so the fallback executes only when the alert scan throws, keeping Log.Debug(...) and the dispatcher invocation together.
🤖 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/ViewModels/AboutViewModel.cs`:
- Around line 278-292: The ManagementObjectCollection returned by
ManagementObjectSearcher.Get() is not disposed; change the pattern in the CPU
block (and the other WMI query blocks: RAM, GPU, Display, Windows) to capture
the collection into an IDisposable local (e.g., var coll = cpuSearch.Get()) and
wrap it in a using (or using var) so the collection is disposed after
enumeration, while still disposing each ManagementObject inside the loop; update
the code that currently calls cpuSearch.Get() directly in the foreach to use
this using-scoped collection for full cleanup of unmanaged WMI resources.
---
Outside diff comments:
In `@SysManager/SysManager/ViewModels/DashboardViewModel.cs`:
- Around line 461-468: The fallback block that sets alert.Title and
alert.Severity is currently outside the catch and always executes; move that
Dispatcher.BeginInvoke block inside the catch for the exception handler so the
fallback runs only when the catch in DashboardViewModel (the catch handling
"Alert scan failed") is triggered—i.e., ensure the catch block contains both
Log.Debug("Alert scan failed: {Error}", ex.Message) and the
System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) block so the
"Feature check unavailable" alert is only set on exceptions.
- Around line 428-435: The fallback block that sets alert.Title and
alert.Severity is placed outside the catch and thus always executes; move the
block currently after the catch (the
System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) that sets
alert.Title = "Event Log check unavailable" and alert.Severity =
AlertSeverity.Green) inside the catch that contains Log.Debug("Alert scan
failed: {Error}", ex.Message) so that it only runs when an exception occurs, and
remove the stray unconditional braces; locate this in DashboardViewModel.cs
around the catch that logs via Log.Debug and wrap the Dispatcher.BeginInvoke
fallback inside that catch.
- Around line 373-380: The fallback block that sets alert.Title and
alert.Severity is currently outside the catch (the catch currently only contains
Log.Debug("Alert scan failed: {Error}", ex.Message);) so it always runs; move
the System.Windows.Application.Current?.Dispatcher.BeginInvoke(...) block (the
lines that set alert.Title = "App update check unavailable" and alert.Severity =
AlertSeverity.Green) into the catch(Exception ex) body (and remove the stray
trailing comment marker) so the fallback executes only when the alert scan
throws, keeping Log.Debug(...) and the dispatcher invocation together.
🪄 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: 4686e1df-293e-4e89-bdd6-0018c27e2f5a
📒 Files selected for processing (7)
CHANGELOG.mdSysManager/SysManager/Services/ThemeService.csSysManager/SysManager/SysManager.csprojSysManager/SysManager/ViewModels/AboutViewModel.csSysManager/SysManager/ViewModels/BulkInstallerViewModel.csSysManager/SysManager/ViewModels/DashboardViewModel.csSysManager/SysManager/Views/ShortcutCleanerView.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 (8)
SysManager/SysManager/ViewModels/BulkInstallerViewModel.cs (1)
28-28: LGTM!Also applies to: 347-347
SysManager/SysManager/SysManager.csproj (1)
13-15: LGTM!SysManager/SysManager/Views/ShortcutCleanerView.xaml (1)
59-61: AI summary inconsistency with PR objectives.The AI-generated summary for this file states "No property set changes or data-binding logic changes—this is an indentation/line-wrapping adjustment only," but the PR objectives explicitly describe functional changes: "ShortcutCleanerView: DataGrid updated to
Background="Transparent"andBorderThickness="0"to match other views and avoid visible white background/border."The PR objectives indicate these are deliberate property additions to fix visual consistency issues, not mere formatting changes.
CHANGELOG.md (1)
9-17: LGTM!SysManager/SysManager/Services/ThemeService.cs (2)
220-220: LGTM!
251-251: LGTM!SysManager/SysManager/ViewModels/DashboardViewModel.cs (1)
176-178: LGTM!SysManager/SysManager/ViewModels/AboutViewModel.cs (1)
280-292: LGTM!Also applies to: 301-309, 318-329, 346-359, 383-390
Comment on lines
278
to
+292
| using var cpuSearch = new System.Management.ManagementObjectSearcher( | ||
| "SELECT Name,NumberOfCores,NumberOfLogicalProcessors,MaxClockSpeed FROM Win32_Processor"); | ||
| foreach (System.Management.ManagementObject mo in cpuSearch.Get()) | ||
| { | ||
| var name = mo["Name"]?.ToString()?.Trim() ?? "unknown"; | ||
| var cores = mo["NumberOfCores"]; | ||
| var threads = mo["NumberOfLogicalProcessors"]; | ||
| var mhz = mo["MaxClockSpeed"]; | ||
| sb.Append("CPU: ").Append(name); | ||
| if (cores is not null) sb.Append($" ({cores}c/{threads}t)"); | ||
| if (mhz is uint speed) sb.Append($" @ {speed / 1000.0:F1} GHz"); | ||
| sb.AppendLine(); | ||
| break; | ||
| } | ||
| using (mo) | ||
| { | ||
| var name = mo["Name"]?.ToString()?.Trim() ?? "unknown"; | ||
| var cores = mo["NumberOfCores"]; | ||
| var threads = mo["NumberOfLogicalProcessors"]; | ||
| var mhz = mo["MaxClockSpeed"]; | ||
| sb.Append("CPU: ").Append(name); | ||
| if (cores is not null) sb.Append($" ({cores}c/{threads}t)"); | ||
| if (mhz is uint speed) sb.Append($" @ {speed / 1000.0:F1} GHz"); | ||
| sb.AppendLine(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Complete the resource cleanup by disposing ManagementObjectCollection.
The ManagementObjectSearcher.Get() method returns a ManagementObjectCollection which also implements IDisposable and holds unmanaged WMI resources. While disposing each ManagementObject addresses the immediate leak, not disposing the collection itself leaves the fix incomplete.
♻️ Recommended pattern for complete disposal
Apply this pattern to all five WMI queries (CPU, RAM, GPU, Display, Windows):
using var cpuSearch = new System.Management.ManagementObjectSearcher(
"SELECT Name,NumberOfCores,NumberOfLogicalProcessors,MaxClockSpeed FROM Win32_Processor");
-foreach (System.Management.ManagementObject mo in cpuSearch.Get())
+using var cpuResults = cpuSearch.Get();
+foreach (System.Management.ManagementObject mo in cpuResults)
using (mo)
{🤖 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/ViewModels/AboutViewModel.cs` around lines 278 - 292,
The ManagementObjectCollection returned by ManagementObjectSearcher.Get() is not
disposed; change the pattern in the CPU block (and the other WMI query blocks:
RAM, GPU, Display, Windows) to capture the collection into an IDisposable local
(e.g., var coll = cpuSearch.Get()) and wrap it in a using (or using var) so the
collection is disposed after enumeration, while still disposing each
ManagementObject inside the loop; update the code that currently calls
cpuSearch.Get() directly in the foreach to use this using-scoped collection for
full cleanup of unmanaged WMI resources.
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
Fixes all High and Medium findings from the full audit run:
using (mo). Prevents COM object leaks.catch { }blocks now log errors via Serilog. Prevents silent disk/permission failures.catch (Exception)in alert scanners now log the exception message for diagnostics.FilteredAppsconverted fromObservableCollectiontoBulkObservableCollectionwithReplaceWith(). Eliminates N+1 change notifications (UI flicker on filter).Background="Transparent"andBorderThickness="0"matching all other views in the app.Test plan
Summary by CodeRabbit
Bug Fixes
Style