fix: address 18 medium-priority code review findings (BIND/MEM/BUG/SEC/PERF)#296
Conversation
…C/PERF) BIND-001..007: Add NotifyPropertyChangedFor on computed properties across 7 models MEM-006: DeepCleanupViewModel — named handler + unsubscribe on rescan/dispose MEM-007: ShortcutCleanerViewModel — named handler + unsubscribe on rescan/dispose BUG-002: MemoryTestService — ReverseDirection=true for correct cutoff break BUG-003: PerformanceService — embed description in script (AddParameter fix) BUG-004/005: Replace FlexVis misuse on IsEnabled with BoolInverterConverter SEC-006: ServiceManagerService — strict allowlist regex for sc.exe arguments PERF-002: LogsViewModel — CollectionView.Count instead of Cast().Count() PERF-003: NetworkSharedState — simplified buffer trimming PERF-004: MarkdownTextBlock — static compiled Regex PERF-006: DiskAnalyzerService — OrdinalIgnoreCase instead of ToLowerInvariant
📝 WalkthroughWalkthroughThis PR refactors binding converters across the WPF application by introducing a dedicated ChangesBinding Conversion, Model Notifications, Handler Cleanup, and Service Optimization
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 |
|
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs (1)
57-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispose previous
CancellationTokenSourceon each scan cycle.
_ctsis reassigned at line 57 without disposing the previous instance. SinceScanAsync()is called repeatedly per user scans, this accumulates undisposedCancellationTokenSourceobjects (which manage unmanaged wait handles) until the view-model is disposed at line 147. Dispose before reassigning and in the finally block.Suggested fix
- _cts = new CancellationTokenSource(); + _cts?.Dispose(); + _cts = new CancellationTokenSource(); @@ finally { + _cts?.Dispose(); + _cts = null; IsScanning = false; IsBusy = false; IsProgressIndeterminate = false; }🤖 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/ShortcutCleanerViewModel.cs` at line 57, The _cts field is reassigned in ScanAsync without disposing the previous CancellationTokenSource; before creating a new CancellationTokenSource in ScanAsync (and also in the view-model Dispose method), call _cts?.Cancel() and _cts?.Dispose() to release unmanaged handles, then assign a new _cts = new CancellationTokenSource(); additionally ensure the finally block in ScanAsync disposes the current _cts if appropriate (or leaves responsibility to Dispose()), and update the Dispose() implementation of the view-model to dispose _cts if not null.
🤖 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/Services/ServiceManagerService.cs`:
- Around line 126-131: The regex used in Regex.IsMatch for validating
serviceName currently uses \s and \w (which are Unicode/broad) so it allows
tabs/other whitespace and non-ASCII word chars; update the validation in the
serviceName check to use a regex that only permits ASCII letters and digits and
the explicit characters space, underscore, hyphen, dot and dollar sign (i.e.
replace \w with an explicit ASCII range for letters/digits/underscore and
replace \s with a literal space), keeping the same start/end anchors and overall
structure in the Regex.IsMatch call.
---
Outside diff comments:
In `@SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs`:
- Line 57: The _cts field is reassigned in ScanAsync without disposing the
previous CancellationTokenSource; before creating a new CancellationTokenSource
in ScanAsync (and also in the view-model Dispose method), call _cts?.Cancel()
and _cts?.Dispose() to release unmanaged handles, then assign a new _cts = new
CancellationTokenSource(); additionally ensure the finally block in ScanAsync
disposes the current _cts if appropriate (or leaves responsibility to
Dispose()), and update the Dispose() implementation of the view-model to dispose
_cts if not 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: 731236cc-857d-4742-85d6-30620e3be131
📒 Files selected for processing (27)
.gitignoreCHANGELOG.mdSysManager/SysManager/App.xamlSysManager/SysManager/Helpers/MarkdownTextBlock.csSysManager/SysManager/Helpers/OutputKindToBrushConverter.csSysManager/SysManager/Models/BatteryInfo.csSysManager/SysManager/Models/DiskHealthReport.csSysManager/SysManager/Models/DiskUsageEntry.csSysManager/SysManager/Models/FriendlyEventEntry.csSysManager/SysManager/Models/InstalledApp.csSysManager/SysManager/Models/PerformanceProfile.csSysManager/SysManager/Models/ProcessEntry.csSysManager/SysManager/Services/DiskAnalyzerService.csSysManager/SysManager/Services/MemoryTestService.csSysManager/SysManager/Services/PerformanceService.csSysManager/SysManager/Services/ServiceManagerService.csSysManager/SysManager/ViewModels/DeepCleanupViewModel.csSysManager/SysManager/ViewModels/LogsViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/ShortcutCleanerViewModel.csSysManager/SysManager/Views/AboutView.xamlSysManager/SysManager/Views/CleanupView.xamlSysManager/SysManager/Views/DeepCleanupView.xamlSysManager/SysManager/Views/NetworkRepairView.xamlSysManager/SysManager/Views/SpeedTestView.xamlSysManager/SysManager/Views/SystemHealthView.xamlSysManager/SysManager/Views/TracerouteView.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 (25)
SysManager/SysManager/Services/MemoryTestService.cs (1)
38-39: LGTM!SysManager/SysManager/ViewModels/LogsViewModel.cs (1)
70-73: LGTM!SysManager/SysManager/Services/DiskAnalyzerService.cs (1)
183-184: LGTM!SysManager/SysManager/Helpers/MarkdownTextBlock.cs (1)
84-87: LGTM!Also applies to: 95-95
SysManager/SysManager/Models/PerformanceProfile.cs (1)
17-23: LGTM!SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
428-432: LGTM!SysManager/SysManager/Models/FriendlyEventEntry.cs (1)
16-19: LGTM!Also applies to: 23-26
SysManager/SysManager/Services/PerformanceService.cs (1)
514-518: LGTM!SysManager/SysManager/Models/DiskUsageEntry.cs (1)
17-19: LGTM!SysManager/SysManager/ViewModels/DeepCleanupViewModel.cs (1)
115-122: LGTM!Also applies to: 128-129, 168-171, 175-175
SysManager/SysManager/Models/InstalledApp.cs (1)
21-23: LGTM!SysManager/SysManager/Models/ProcessEntry.cs (1)
19-21: LGTM!SysManager/SysManager/Helpers/OutputKindToBrushConverter.cs (1)
60-72: LGTM!SysManager/SysManager/App.xaml (1)
20-20: LGTM!SysManager/SysManager/Views/TracerouteView.xaml (1)
84-84: LGTM!SysManager/SysManager/Views/CleanupView.xaml (1)
87-87: LGTM!Also applies to: 89-89
SysManager/SysManager/Views/NetworkRepairView.xaml (1)
36-36: LGTM!Also applies to: 48-48, 60-60
SysManager/SysManager/Views/SpeedTestView.xaml (1)
46-46: LGTM!Also applies to: 71-71
SysManager/SysManager/Views/DeepCleanupView.xaml (1)
49-49: LGTM!Also applies to: 118-118, 149-149
SysManager/SysManager/Views/SystemHealthView.xaml (1)
210-210: LGTM!Also applies to: 281-281
SysManager/SysManager/Views/AboutView.xaml (1)
54-54: LGTM!Also applies to: 132-132
CHANGELOG.md (1)
9-51: LGTM!SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs (1)
49-53: LGTM!Also applies to: 66-67, 135-139, 143-148
SysManager/SysManager/Models/BatteryInfo.cs (1)
18-30: LGTM!SysManager/SysManager/Models/DiskHealthReport.cs (1)
20-23: LGTM!Also applies to: 24-29, 33-38, 40-42, 44-47, 49-52
| // SEC-006: Strict allowlist for service names — alphanumeric, spaces, | ||
| // hyphens, underscores, dots, and dollar signs only (covers all valid | ||
| // Windows service names including instance names like MSSQL$INSTANCE). | ||
| if (string.IsNullOrWhiteSpace(serviceName) || | ||
| !System.Text.RegularExpressions.Regex.IsMatch(serviceName, @"^[\w\s\-.$]+$")) | ||
| throw new ArgumentException("Invalid service name.", nameof(serviceName)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET regular expressions, does \smatch tabs/newlines (not just spaces), and is[A-Za-z0-9_.-$ ]+ a stricter way to allow only visible service-name characters plus spaces?
💡 Result:
Yes and (mostly) yes. 1) Does \s match tabs/newlines in.NET regex? In.NET regular expressions, \s matches any whitespace character, including tab (\t), newline (\n), carriage return (\r), vertical tab (\v), and form feed (\f), plus other Unicode whitespace/separator characters (e.g., Unicode separator categories like Zs, Zl, Zp). [1][2] 2) Is [A-Za-z0-9_.\-$ ]+ a stricter way to allow only visible service-name characters plus spaces? It is stricter than using \s, because it only allows: - ASCII letters A-Z/a-z - digits 0-9 - underscore _ - dot ., and - hyphen - - dollar $ - literal space and disallows all other whitespace/control characters (so no tabs/newlines). Character class membership is defined by the characters you list inside [...]. [3] However, it is not “Unicode-aware”: it allows only ASCII letters/digits. It also allows any number of those characters via +, but it does not enforce additional constraints beyond character set (for example, it allows spaces anywhere, and it doesn’t prevent consecutive spaces, leading/trailing spaces, etc.). If your goal is “service-name characters” that are typically “visible” plus spaces, your pattern is a reasonable approach, but you may want to tighten whitespace placement depending on your requirements (e.g., avoid leading/trailing spaces or collapse multiple spaces).
Citations:
- 1: https://docs.microsoft.com/en-us/dotnet/standard/base-types/character-classes-in-regular-expressions
- 2: https://docs.microsoft.com/en-us/dotnet/standard/base-types/regular-expression-language-quick-reference
- 3: https://learn.microsoft.com/en-us/dotnet/standard/base-types/regular-expression-language-quick-reference
🌐 Web query:
Windows service names allowed characters rules Microsoft documentation
💡 Result:
Windows service names (ServiceName passed to CreateService/CreateServiceW, also the “service key name” used by the SCM) are limited to 256 characters, comparisons are case-insensitive, the SCM does not treat “/” or “\” as valid characters in the service name, and it preserves the case that was originally provided [1]. Notes/related clarifications: - The “DisplayName” (what users see in UI) is separate from “ServiceName”, also limited to 256 characters, and is case-insensitive for comparisons [1]. - While the Microsoft Learn page explicitly calls out that forward-slash (/) and backslash () are not valid service name characters, it does not document additional forbidden characters beyond these in the cited text; for safety and portability, avoid “/” and “\” and keep the name within 256 characters [1].
Citations:
🏁 Script executed:
rg -i "SEC-006\|service.*name\|alphanumeric" SysManager/ -A 3 -B 3Repository: laurentiu021/SystemManager
Length of output: 52
🏁 Script executed:
rg "ServiceManagerService" --type cs -B 2 -A 2Repository: laurentiu021/SystemManager
Length of output: 8674
🏁 Script executed:
rg -i "servicename\s*=" SysManager/ --type cs | head -20Repository: laurentiu021/SystemManager
Length of output: 52
🏁 Script executed:
sed -n '110,145p' SysManager/SysManager/Services/ServiceManagerService.csRepository: laurentiu021/SystemManager
Length of output: 1824
🏁 Script executed:
ast-grep --pattern 'public.*($serviceName' SysManager/SysManager/Services/ServiceManagerService.csRepository: laurentiu021/SystemManager
Length of output: 52
Narrow \s to match the stated policy of spaces only.
The regex at line 130 uses \s, which matches tabs, newlines, and other Unicode whitespace characters beyond spaces. The SEC-006 comment explicitly requires "spaces only." Replace \s with a literal space character and use explicit ASCII character ranges to ensure predictable, locale-independent behavior:
🔒 Proposed fix
- if (string.IsNullOrWhiteSpace(serviceName) ||
- !System.Text.RegularExpressions.Regex.IsMatch(serviceName, @"^[\w\s\-.$]+$"))
+ if (string.IsNullOrWhiteSpace(serviceName) ||
+ !System.Text.RegularExpressions.Regex.IsMatch(
+ serviceName,
+ @"^[A-Za-z0-9_.\-$ ]+$",
+ System.Text.RegularExpressions.RegexOptions.CultureInvariant))
throw new ArgumentException("Invalid service name.", nameof(serviceName));🤖 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/ServiceManagerService.cs` around lines 126 -
131, The regex used in Regex.IsMatch for validating serviceName currently uses
\s and \w (which are Unicode/broad) so it allows tabs/other whitespace and
non-ASCII word chars; update the validation in the serviceName check to use a
regex that only permits ASCII letters and digits and the explicit characters
space, underscore, hyphen, dot and dollar sign (i.e. replace \w with an explicit
ASCII range for letters/digits/underscore and replace \s with a literal space),
keeping the same start/end anchors and overall structure in the Regex.IsMatch
call.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| VisibleCount = EntriesView.Cast<object>().Count(); | ||
| // PERF-002: Use CollectionView.Count directly instead of iterating | ||
| // the entire filtered collection via Cast<object>().Count(). | ||
| VisibleCount = ((CollectionView)EntriesView).Count; |
| public sealed class BoolInverterConverter : IValueConverter | ||
| { | ||
| public object Convert(object value, Type targetType, object parameter, CultureInfo culture) | ||
| => value is bool b ? !b : true; |
| => value is bool b ? !b : true; | ||
|
|
||
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | ||
| => value is bool b ? !b : true; |
[//]: # (dependabot-start)⚠️ **Dependabot is rebasing this PR**⚠️ Rebasing might not happen immediately, so don't worry if this takes some time. Note: if you make any changes to this PR yourself, they will take precedence over the rebase. --- [//]: # (dependabot-end) Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 4 to 5. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/actions/setup-dotnet/commit/c2fa09f4bde5ebb9d1777cf28262a3eb3db3ced7"><code>c2fa09f</code></a> Bump minimatch from 3.1.2 to 3.1.5 (<a href="https://github.com/actions/setup-dotnet/issues/705">#705</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/02574b18e2dc57a218ee4e11ba1e1603c67236e8"><code>02574b1</code></a> Add support for optional architecture input for cross-architecture .NET insta...</li> <li><a href="https://github.com/actions/setup-dotnet/commit/16c7b3c2fa55a0e394467d22512b84fda46adf63"><code>16c7b3c</code></a> Bump fast-xml-parser from 4.4.1 to 5.3.6 (<a href="https://github.com/actions/setup-dotnet/issues/671">#671</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/131b410979e0b49e2162c0718030257b22d6dc2c"><code>131b410</code></a> Add support for workloads input (<a href="https://github.com/actions/setup-dotnet/issues/693">#693</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/baa11fbfe1d6520db94683bd5c7a3818018e4309"><code>baa11fb</code></a> Bump test dependencies to resolve System.Net.Http vulnerability, update workf...</li> <li><a href="https://github.com/actions/setup-dotnet/commit/24ec4f204bea672fb937f8f9f3bfec35ba0dc7b5"><code>24ec4f2</code></a> Upgrade to latest actions packages (<a href="https://github.com/actions/setup-dotnet/issues/687">#687</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/4c100cb5e4b37e8b502c0ca5acbab36ee401b85f"><code>4c100cb</code></a> Fix icons (<a href="https://github.com/actions/setup-dotnet/issues/604">#604</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/25328d894dc34d01d191df4849cb7f5402e3f142"><code>25328d8</code></a> Bump actions/checkout from 5 to 6 (<a href="https://github.com/actions/setup-dotnet/issues/684">#684</a>)</li> <li><a href="https://github.com/actions/setup-dotnet/commit/937b8dd3814c642a65e599b368dfac491d9c83da"><code>937b8dd</code></a> Update README with note on setting DOTNET_INSTALL_DIR for Linux permission is...</li> <li><a href="https://github.com/actions/setup-dotnet/commit/2016bd2012dba4e32de620c46fe006a3ac9f0602"><code>2016bd2</code></a> Bump actions/publish-action from 0.3.0 to 0.4.0 and update macos-13 to macos-...</li> <li>Additional commits viewable in <a href="https://github.com/actions/setup-dotnet/compare/v4...v5">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…C/PERF) (#296) BIND-001..007: Add NotifyPropertyChangedFor on computed properties across 7 models MEM-006: DeepCleanupViewModel — named handler + unsubscribe on rescan/dispose MEM-007: ShortcutCleanerViewModel — named handler + unsubscribe on rescan/dispose BUG-002: MemoryTestService — ReverseDirection=true for correct cutoff break BUG-003: PerformanceService — embed description in script (AddParameter fix) BUG-004/005: Replace FlexVis misuse on IsEnabled with BoolInverterConverter SEC-006: ServiceManagerService — strict allowlist regex for sc.exe arguments PERF-002: LogsViewModel — CollectionView.Count instead of Cast().Count() PERF-003: NetworkSharedState — simplified buffer trimming PERF-004: MarkdownTextBlock — static compiled Regex PERF-006: DiskAnalyzerService — OrdinalIgnoreCase instead of ToLowerInvariant Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
3 participants
Summary
Addresses 18 medium-priority findings from the full code review:
Data Binding (BIND-001..007)
Memory Leaks (MEM-006, MEM-007)
Bugs (BUG-002..005)
Security (SEC-006)
Performance (PERF-002..006)
Other
Build: 0 errors (main + tests)
Files changed: 27
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance Improvements