fix: address 6 medium bugs (#311)#351
Merged
Merged
Conversation
…ceManager, DiskAnalyzer, WindowsUpdateVM, TuneUpService (#311)
📝 WalkthroughWalkthroughThis PR resolves six targeted bugs across models and services: WastedBytes property notifications in DuplicateFileGroup, accurate access-denied detection in disk analysis, service restart state checking, recycle bin operation error validation, GitHub prerelease filtering, and null-safe application shutdown. ChangesMultiple Bug Fixes: Models, Services, and Utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
🤖 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/WindowsUpdateViewModel.cs`:
- Line 77: Several shutdown calls directly invoke
System.Windows.Application.Current.Shutdown(), which can throw
NullReferenceException in tests or non-standard hosts; update every occurrence
to use the null-conditional operator
(System.Windows.Application.Current?.Shutdown())—specifically replace the direct
calls in WindowsUpdateViewModel's relaunch/shutdown paths, the shutdown call in
DashboardViewModel, the call in AppUpdatesViewModel, and the call in
TrayIconService so each uses ?.Shutdown() to safely no-op when Current is null.
🪄 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: b36b11c7-99f8-49c4-9684-5255dc1aa92f
📒 Files selected for processing (9)
CHANGELOG.mdSysManager/SysManager.Tests/DiskAnalyzerServiceTests.csSysManager/SysManager.Tests/DuplicateFileGroupTests.csSysManager/SysManager/Models/DuplicateFileGroup.csSysManager/SysManager/Services/DiskAnalyzerService.csSysManager/SysManager/Services/ServiceManagerService.csSysManager/SysManager/Services/TuneUpService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/ViewModels/WindowsUpdateViewModel.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 (10)
SysManager/SysManager/Services/UpdateService.cs (1)
263-263: LGTM!SysManager/SysManager/Services/TuneUpService.cs (1)
221-223: LGTM!SysManager/SysManager/Models/DuplicateFileGroup.cs (1)
17-23: LGTM!SysManager/SysManager/Services/DiskAnalyzerService.cs (1)
85-86: LGTM!Also applies to: 96-97, 127-127, 132-132, 144-145, 147-148, 158-159, 171-172, 179-179
SysManager/SysManager/Services/ServiceManagerService.cs (1)
89-102: LGTM!CHANGELOG.md (1)
9-25: LGTM!SysManager/SysManager.Tests/DiskAnalyzerServiceTests.cs (1)
130-141: LGTM!SysManager/SysManager.Tests/DuplicateFileGroupTests.cs (3)
15-41: LGTM!
43-87: LGTM!
91-109: LGTM!
| { | ||
| if (AdminHelper.RelaunchAsAdmin()) | ||
| System.Windows.Application.Current.Shutdown(); | ||
| System.Windows.Application.Current?.Shutdown(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any non-null-safe shutdown calls still exist.
# Expected: no matches for direct Application.Current.Shutdown(...)
rg -nP --type=cs 'Application\.Current\.Shutdown\s*\('
rg -nP --type=cs 'Application\.Current\?\.\s*Shutdown\s*\('Repository: laurentiu021/SystemManager
Length of output: 1498
Apply null-guards to all relaunch shutdown paths.
Line 77 was fixed, but lines 114 and 350 in WindowsUpdateViewModel.cs still call System.Windows.Application.Current.Shutdown() directly. Additionally, the same null-reference vulnerability exists at:
- DashboardViewModel.cs:126
- AppUpdatesViewModel.cs:39
- TrayIconService.cs:118
Apply the null-coalescing operator (?.) to all of these calls to prevent null-reference exceptions in tests or non-standard hosting scenarios.
🤖 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/WindowsUpdateViewModel.cs` at line 77,
Several shutdown calls directly invoke
System.Windows.Application.Current.Shutdown(), which can throw
NullReferenceException in tests or non-standard hosts; update every occurrence
to use the null-conditional operator
(System.Windows.Application.Current?.Shutdown())—specifically replace the direct
calls in WindowsUpdateViewModel's relaunch/shutdown paths, the shutdown call in
DashboardViewModel, the call in AppUpdatesViewModel, and the call in
TrayIconService so each uses ?.Shutdown() to safely no-op when Current is null.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…event handler leaks (#351) ## Summary Fix memory leaks that cause the application to become unresponsive or crash during extended use. ## Root cause Three categories of resource leaks were identified through static analysis: ### 1. CancellationTokenSource leak (8 ViewModels, 15 locations) Every time a user triggered a refresh/scan/list operation, a new `CancellationTokenSource` was created without disposing the previous one. Each undisposed CTS holds an internal `WaitHandle`. Over extended use (dozens of refreshes), these accumulate and contribute to memory exhaustion. **Affected ViewModels:** WindowsUpdateViewModel (5), UninstallerViewModel (2), SystemHealthViewModel (2), AppUpdatesViewModel (2), DriversViewModel (1), LogsViewModel (1), DuplicateFileViewModel (1), DiskAnalyzerViewModel (1). **Fix:** Add `_cts?.Dispose()` before every `_cts = new CancellationTokenSource()`. ### 2. Process object leak in App.ActivateExistingInstance `Process.GetProcessesByName()` returns Process objects that must be disposed. Neither the enumerated processes nor the current process were being disposed. **Fix:** `using var current` + `try/finally { proc.Dispose(); }` in the loop. ### 3. PropertyChanged event handler leak in Network tab `AddTarget()` subscribed an anonymous lambda to `target.PropertyChanged`. When targets were removed via `RemoveTargetInternal()`, the handler was never unsubscribed — the lambda captured the outer scope, preventing GC from collecting removed targets. **Fix:** Store handlers in a `Dictionary<string, PropertyChangedEventHandler>` keyed by host. Unsubscribe in `RemoveTargetInternal()` before removing the target. ## Files changed (11) - `App.xaml.cs` — Process disposal - `NetworkSharedState.cs` — event handler tracking + unsubscribe - `NetworkViewModel.cs` — event handler tracking + unsubscribe - `WindowsUpdateViewModel.cs` — CTS disposal (5 locations) - `UninstallerViewModel.cs` — CTS disposal (2) - `SystemHealthViewModel.cs` — CTS disposal (2) - `AppUpdatesViewModel.cs` — CTS disposal (2) - `DriversViewModel.cs` — CTS disposal (1) - `LogsViewModel.cs` — CTS disposal (1) - `DuplicateFileViewModel.cs` — CTS disposal (1) - `DiskAnalyzerViewModel.cs` — CTS disposal (1) ## Testing - Build: 0 errors (both main project and test project) - All changes are additive Dispose/unsubscribe calls — no behavioral change - Existing tests unaffected Closes #161 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…Model (#358) ## Summary Add `_cts?.Dispose()` before each `new CancellationTokenSource()` for all four CTS fields in CleanupViewModel (`_tempCts`, `_binCts`, `_sfcCts`, `_dismCts`). ## Problem Found during closed-bug regression audit: the CTS disposal pattern from PR #351 (fix #161 memory leaks) was correctly applied in 8 other ViewModels but missed in CleanupViewModel. Repeated Clean TEMP / Empty Recycle Bin / SFC / DISM operations leak CancellationTokenSource handles. ## Fix Same one-line pattern already used everywhere else: ```csharp _cts?.Dispose(); _cts = new CancellationTokenSource(); ``` Applied to all 4 CTS fields in CleanupViewModel. ## Files changed - `SysManager/ViewModels/CleanupViewModel.cs` — 4 lines added Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
4 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
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 6 medium-priority bugs identified in the code review.
Changes
Tests
Closes #311
Summary by CodeRabbit
Bug Fixes
Tests