fix: code review batch 2 — IDialogService, QA/security fixes#252
Conversation
…lamp, TrimBuffer, COM release, Ookla verify
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThis PR introduces an ChangesDialog Service Refactoring and Model/Service Improvements
🎯 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.
Actionable comments posted: 10
🧹 Nitpick comments (4)
SysManager/SysManager.Tests/EventExplainerEdgeCaseTests.cs (2)
21-23: ⚡ Quick winMake string-match assertions consistently case-insensitive.
Using
StringComparison.OrdinalIgnoreCaseconsistently here will make tests less brittle to non-functional text casing changes.Example adjustment
- Assert.Contains("rebooted", entry.Explanation); + Assert.Contains("rebooted", entry.Explanation, StringComparison.OrdinalIgnoreCase); - Assert.Contains("DNS", entry.Explanation); + Assert.Contains("DNS", entry.Explanation, StringComparison.OrdinalIgnoreCase);Also applies to: 38-38, 53-53, 68-68, 117-118, 163-163, 178-178
🤖 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.Tests/EventExplainerEdgeCaseTests.cs` around lines 21 - 23, The assertions that check text content (e.g., Assert.Contains("rebooted", entry.Explanation)) should be made case-insensitive: replace the current Assert.Contains(string, string) calls with the overload that accepts a StringComparison and pass StringComparison.OrdinalIgnoreCase so the substring checks on entry.Explanation (and similar checks at the other noted locations) won't be case-sensitive; keep Assert.NotEmpty(entry.Explanation) and Assert.NotEmpty(entry.Recommendation) as-is but change all Assert.Contains usages to use StringComparison.OrdinalIgnoreCase to make tests consistent and less brittle.
57-134: ⚡ Quick winConsider parameterizing unknown-severity fallback tests.
These five tests are structurally identical; converting to a single
[Theory]withInlineDatawould reduce duplication and make future fallback-message updates easier.🤖 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.Tests/EventExplainerEdgeCaseTests.cs` around lines 57 - 134, Several nearly identical tests (Enrich_UnknownEvent_CriticalSeverity_GenericExplanation, Enrich_UnknownEvent_ErrorSeverity_GenericExplanation, Enrich_UnknownEvent_WarningSeverity_GenericExplanation, Enrich_UnknownEvent_InfoSeverity_GenericExplanation, Enrich_UnknownEvent_VerboseSeverity_LowLevelDiagnostic) test the same unknown-event fallback behavior; replace them with a single parameterized Theory using InlineData that passes ProviderName, EventId, Severity, expected substrings for Explanation and Recommendation as parameters, call EventExplainer.Enrich(entry) inside the test, and assert the expected substrings (use StringComparison where needed) against entry.Explanation/entry.Recommendation; keep references to FriendlyEventEntry and EventExplainer.Enrich to locate where to build the inline cases.SysManager/SysManager.Tests/ViewModelBaseExtendedTests.cs (2)
113-137: ⚡ Quick winConsider adding same-value tests for StatusMessage and IsProgressIndeterminate.
The same-value tests for
ProgressandIsBusycorrectly verify that setting a property to its current value doesn't raisePropertyChanged. For consistency and complete coverage, consider adding equivalent tests forStatusMessageandIsProgressIndeterminateto document whetherViewModelBaseimplements this optimization for all properties.📋 Example tests for complete coverage
[Fact] public void StatusMessage_SetSameValue_DoesNotRaisePropertyChanged() { var vm = new TestViewModel(); vm.StatusMessage = "Test"; var changed = new List<string>(); vm.PropertyChanged += (_, e) => changed.Add(e.PropertyName!); vm.StatusMessage = "Test"; Assert.DoesNotContain("StatusMessage", changed); } [Fact] public void IsProgressIndeterminate_SetSameValue_DoesNotRaisePropertyChanged() { var vm = new TestViewModel(); vm.IsProgressIndeterminate = true; var changed = new List<string>(); vm.PropertyChanged += (_, e) => changed.Add(e.PropertyName!); vm.IsProgressIndeterminate = true; Assert.DoesNotContain("IsProgressIndeterminate", changed); }🤖 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.Tests/ViewModelBaseExtendedTests.cs` around lines 113 - 137, Add two mirror tests following the existing pattern to ensure setting the same value for StatusMessage and IsProgressIndeterminate does not raise PropertyChanged: create tests named StatusMessage_SetSameValue_DoesNotRaisePropertyChanged and IsProgressIndeterminate_SetSameValue_DoesNotRaisePropertyChanged that instantiate TestViewModel, set vm.StatusMessage and vm.IsProgressIndeterminate to an initial value, attach vm.PropertyChanged collecting PropertyName into a list, set the same value again, and Assert.DoesNotContain("StatusMessage", changed) and Assert.DoesNotContain("IsProgressIndeterminate", changed) respectively so coverage matches the existing Progress and IsBusy tests.
27-35: ⚡ Quick winVerify and document DisposeCallCount after second Dispose call.
The test correctly verifies that calling
Dispose()twice doesn't throw, but it should also assert the expectedDisposeCallCountto document whetherViewModelBasehas a guard to prevent duplicate disposal. This clarifies whetherDispose(bool)runs once (with guard) or twice (without guard).📝 Suggested addition to document behavior
var vm = new TestViewModel(); vm.Dispose(); // Calling Dispose again should not throw var ex = Record.Exception(() => vm.Dispose()); Assert.Null(ex); + // Document whether Dispose(bool) is guarded: expect 1 if guarded, 2 if not + Assert.Equal(1, vm.DisposeCallCount); // or 2, depending on actual ViewModelBase implementation }🤖 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.Tests/ViewModelBaseExtendedTests.cs` around lines 27 - 35, Update the test Dispose_CalledTwice_GCSuppressFinalizeCalledButNoThrow to also assert the TestViewModel.DisposeCallCount after the second call to Dispose so the behavior is documented; specifically, after Record.Exception(() => vm.Dispose()) add an Assert.Equal(1, vm.DisposeCallCount) (or change the expected value if your ViewModelBase intentionally allows Dispose(bool) to run twice) to make clear whether Dispose(bool) is guarded to run only once; reference TestViewModel, DisposeCallCount and ViewModelBase.Dispose(bool) when making this assertion.
🤖 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.Tests/AppBlockerServiceValidationTests.cs`:
- Around line 37-48: The test BlockApp_ValidNames_FormatsCorrectly is calling
AppBlockerService.IsBlocked instead of exercising the BlockApp acceptance path;
change the call to AppBlockerService.BlockApp (using the same exeName
normalization: append ".exe" when needed) and assert the expected behavior under
non-admin (e.g., that it does not throw and returns false/indicates failure due
to lack of privileges), rather than calling IsBlocked; keep the existing comment
about registry/admin behavior and adjust the Assert to verify BlockApp's
return/exception behavior accordingly.
In `@SysManager/SysManager.Tests/BatteryInfoEdgeCaseTests.cs`:
- Around line 30-34: The two tests still expect pre-clamp behavior; update the
assertions to match the BatteryInfo clamping rules: in the test method
HealthPercent_NewBatteryOverDesign_CanExceed100 change the assertion to assert
HealthPercent == 100 (use Assert.Equal(100, info.HealthPercent)) and in the
corresponding wear-percent test (the test that currently expects a negative
WearPercent in the 58-62 block) change its assertion to assert WearPercent == 0
(use Assert.Equal(0, info.WearPercent)); locate these changes by the test method
names BatteryInfo and the test methods
HealthPercent_NewBatteryOverDesign_CanExceed100 and the wear-percent test in the
BatteryInfoEdgeCaseTests class.
In `@SysManager/SysManager.Tests/CleanupCategoryHumanSizeExtendedTests.cs`:
- Around line 116-127: The tests assert locale-specific strings ("Mar" and
"1,500") which will fail on non-en-US runners; update the tests that reference
LargeFileEntry.LastModifiedDisplay and any size formatting assertions to either
(a) compare using an explicit invariant format (e.g., call
LastModified.ToString("MMM", CultureInfo.InvariantCulture) or use a specific
numeric format with CultureInfo.InvariantCulture) or (b) construct the expected
strings using CultureInfo.CurrentCulture before asserting so the test adapts to
the runner locale; locate uses of LargeFileEntry.LastModifiedDisplay and
SizeBytes/LastModified in the failing test methods and replace the hard-coded
"Mar" and "1,500" assertions with one of these locale-safe approaches.
In `@SysManager/SysManager.Tests/DiskHealthReportEdgeCaseTests.cs`:
- Around line 128-133: The test TemperatureColorHex_NullTemp_FallsToDefault in
DiskHealthReportEdgeCaseTests incorrectly expects "#EF4444" (red); update the
assertion to expect the DiskHealthReport null-temperature default grey by
matching the actual value used by the TemperatureColorHex implementation for
null TemperatureC (either reference the implementation's constant or replace the
literal with that grey hex); ensure you verify the TemperatureC null branch in
DiskHealthReport.TemperatureColorHex and change Assert.Equal to the correct grey
hex value.
In `@SysManager/SysManager.Tests/LogServiceSanitizeEdgeCaseTests.cs`:
- Around line 42-46: The test SanitizePath_MultipleUsersInPath_ReplacesFirst
only checks for presence of "[user]" so it can pass if the wrong segment(s) are
sanitized; update the assertion to verify the exact expected string after
calling LogService.SanitizePath (or assert that only the first user segment was
replaced and the second remains unchanged). Specifically, change the
Assert.Contains to Assert.Equal with the exact expected sanitized path (e.g.
expected = @"C:\Users\[user]\backup\C:\Users\bob\file.txt") or use two asserts
that check the first occurrence is replaced and the second still contains "bob",
referencing LogService.SanitizePath and the test
SanitizePath_MultipleUsersInPath_ReplacesFirst.
In `@SysManager/SysManager.Tests/OperationLockServiceEdgeCaseTests.cs`:
- Around line 148-153: The test currently disposes acquired lock handles only
after the Assert, so a failed assertion can skip cleanup and leave locks held;
update the test method (the block using tasks, successCount, and handles in
OperationLockServiceEdgeCaseTests) to perform cleanup in a finally: await
Task.WhenAll(tasks) as before, then move Assert.Equal(1, successCount) inside a
try and dispose all entries in the handles array (null-checking each) in a
finally block so handles are always released even on assertion failures.
- Around line 120-128: The test attaches an event handler to
Service.PropertyChanged but never removes it, causing listener leaks across
tests; modify the test to store the handler in a local variable (e.g.,
PropertyChangedEventHandler handler = (_, e) => changed.Add(e.PropertyName!)),
attach it with Service.PropertyChanged += handler before calling
Service.TryAcquire, and ensure you unsubscribe after the assertions with
Service.PropertyChanged -= handler (or wrap attach/unsubscribe in a try/finally)
so the singleton Service does not retain callbacks between tests.
- Line 8: Add a test collection attribute to isolate the singleton: annotate
both test classes OperationLockServiceTests and
OperationLockServiceEdgeCaseTests with [Collection("OperationLock",
DisableParallelization = true)] so xUnit won't run them in parallel; unsubscribe
the PropertyChanged handler you attach to OperationLockService.Instance (use
OperationLockService.Instance.PropertyChanged -= <handler>) after the test to
avoid event accumulation on the singleton; and move the existing cleanup
statements that reset/clear the singleton state (the cleanup currently run after
the assertion) into a finally block so cleanup always runs even if assertions
fail.
In `@SysManager/SysManager/Services/DialogService.cs`:
- Line 16: The public static Instance property on IDialogService is writable and
can be set to null, risking NREs in confirm call sites; change the property to
prevent unsafe assignments by either making the setter non-public (e.g., private
set) and initializing with new DialogService(), or implement a guarded setter
that throws on null (or rejects null) so Instance can never be swapped to null;
update references that previously set Instance directly to use an explicit
Initialize/Replace method if needed and keep the type IDialogService and
concrete DialogService names to locate the property.
In `@SysManager/SysManager/Services/SpeedTestService.cs`:
- Around line 295-301: The code only computes and logs SHA256 (SHA256.HashData +
Log.Information) and extracts the embedded certificate
(X509Certificate.CreateFromSignedFile) then checks
cert.Subject.Contains("Ookla") but never enforces pinning or signature trust;
update the download verification to compare the computed hash against a
hard-coded expected SHA256 value and throw/fail if it does not match
(fail-closed), and replace the naive certificate check with proper Authenticode
verification (use WinVerifyTrust or build an X509Chain with explicit
trust/RevocationMode and validate the signature) and throw on any
signature/chain validation failure instead of just logging warnings. Ensure
these checks occur in the same routine that writes/verifies zipPath so the
method that downloads/installs the Ookla CLI cannot proceed on mismatch.
---
Nitpick comments:
In `@SysManager/SysManager.Tests/EventExplainerEdgeCaseTests.cs`:
- Around line 21-23: The assertions that check text content (e.g.,
Assert.Contains("rebooted", entry.Explanation)) should be made case-insensitive:
replace the current Assert.Contains(string, string) calls with the overload that
accepts a StringComparison and pass StringComparison.OrdinalIgnoreCase so the
substring checks on entry.Explanation (and similar checks at the other noted
locations) won't be case-sensitive; keep Assert.NotEmpty(entry.Explanation) and
Assert.NotEmpty(entry.Recommendation) as-is but change all Assert.Contains
usages to use StringComparison.OrdinalIgnoreCase to make tests consistent and
less brittle.
- Around line 57-134: Several nearly identical tests
(Enrich_UnknownEvent_CriticalSeverity_GenericExplanation,
Enrich_UnknownEvent_ErrorSeverity_GenericExplanation,
Enrich_UnknownEvent_WarningSeverity_GenericExplanation,
Enrich_UnknownEvent_InfoSeverity_GenericExplanation,
Enrich_UnknownEvent_VerboseSeverity_LowLevelDiagnostic) test the same
unknown-event fallback behavior; replace them with a single parameterized Theory
using InlineData that passes ProviderName, EventId, Severity, expected
substrings for Explanation and Recommendation as parameters, call
EventExplainer.Enrich(entry) inside the test, and assert the expected substrings
(use StringComparison where needed) against
entry.Explanation/entry.Recommendation; keep references to FriendlyEventEntry
and EventExplainer.Enrich to locate where to build the inline cases.
In `@SysManager/SysManager.Tests/ViewModelBaseExtendedTests.cs`:
- Around line 113-137: Add two mirror tests following the existing pattern to
ensure setting the same value for StatusMessage and IsProgressIndeterminate does
not raise PropertyChanged: create tests named
StatusMessage_SetSameValue_DoesNotRaisePropertyChanged and
IsProgressIndeterminate_SetSameValue_DoesNotRaisePropertyChanged that
instantiate TestViewModel, set vm.StatusMessage and vm.IsProgressIndeterminate
to an initial value, attach vm.PropertyChanged collecting PropertyName into a
list, set the same value again, and Assert.DoesNotContain("StatusMessage",
changed) and Assert.DoesNotContain("IsProgressIndeterminate", changed)
respectively so coverage matches the existing Progress and IsBusy tests.
- Around line 27-35: Update the test
Dispose_CalledTwice_GCSuppressFinalizeCalledButNoThrow to also assert the
TestViewModel.DisposeCallCount after the second call to Dispose so the behavior
is documented; specifically, after Record.Exception(() => vm.Dispose()) add an
Assert.Equal(1, vm.DisposeCallCount) (or change the expected value if your
ViewModelBase intentionally allows Dispose(bool) to run twice) to make clear
whether Dispose(bool) is guarded to run only once; reference TestViewModel,
DisposeCallCount and ViewModelBase.Dispose(bool) when making this assertion.
🪄 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: b8490742-c120-4aa9-a8a7-a2f8c993080c
📒 Files selected for processing (25)
CHANGELOG.mdSysManager/SysManager.Tests/AppBlockerServiceValidationTests.csSysManager/SysManager.Tests/BatteryInfoEdgeCaseTests.csSysManager/SysManager.Tests/CleanupCategoryHumanSizeExtendedTests.csSysManager/SysManager.Tests/DiskHealthReportEdgeCaseTests.csSysManager/SysManager.Tests/EventExplainerEdgeCaseTests.csSysManager/SysManager.Tests/HealthAnalyzerEdgeCaseTests.csSysManager/SysManager.Tests/LogServiceSanitizeEdgeCaseTests.csSysManager/SysManager.Tests/OperationLockServiceEdgeCaseTests.csSysManager/SysManager.Tests/ViewModelBaseExtendedTests.csSysManager/SysManager/Models/BatteryInfo.csSysManager/SysManager/Models/DiskHealthReport.csSysManager/SysManager/Services/DialogService.csSysManager/SysManager/Services/IDialogService.csSysManager/SysManager/Services/ShortcutCleanerService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/ViewModels/AppBlockerViewModel.csSysManager/SysManager/ViewModels/CleanupViewModel.csSysManager/SysManager/ViewModels/NetworkRepairViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/PerformanceViewModel.csSysManager/SysManager/ViewModels/ProcessManagerViewModel.csSysManager/SysManager/ViewModels/ServicesViewModel.csSysManager/SysManager/ViewModels/ShortcutCleanerViewModel.csSysManager/SysManager/ViewModels/UninstallerViewModel.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 (15)
SysManager/SysManager.Tests/HealthAnalyzerEdgeCaseTests.cs (1)
10-224: Strong edge-case and precedence coverage forHealthAnalyzer.Analyze.This test suite exercises boundary thresholds, role-specific verdict routing, mixed-condition precedence, null-latency handling, and aggregate computations in a way that should significantly reduce regressions.
SysManager/SysManager.Tests/EventExplainerEdgeCaseTests.cs (1)
10-54: Strong regression coverage for known mappings and provider edge handling.These tests protect the highest-risk lookup paths well (specific provider/event IDs + empty provider no-throw), and they align nicely with the QA/security stabilization goals in this PR.
Also applies to: 136-179
SysManager/SysManager.Tests/CleanupCategoryHumanSizeExtendedTests.cs (2)
8-75: Great boundary coverage forHumanSize.Nice expansion across 0/negative, unit boundaries, fractional formatting, and very large inputs. This meaningfully reduces regression risk in size-display logic.
77-113:CleanupResult/display-property tests look solid.Good assertions around summary text composition and display-property wiring (
SizeDisplay,CountDisplay)—these directly protect user-visible formatting behavior.Also applies to: 129-141
SysManager/SysManager.Tests/ViewModelBaseExtendedTests.cs (4)
1-17: LGTM!The test infrastructure is well-designed. The
TestViewModelhelper class properly tracksDisposecalls and follows the standard dispose pattern by callingbase.Dispose(disposing).
19-25: LGTM!This test correctly verifies that calling
Dispose()invokes the overriddenDispose(bool)method exactly once.
37-63: LGTM!The default value tests are comprehensive and correctly verify the initial state of all four properties (
IsBusy,StatusMessage,Progress,IsProgressIndeterminate).
65-111: LGTM!The PropertyChanged notification tests comprehensively verify that each property correctly raises
PropertyChangedevents with the expected property name. The null-forgiving operator usage one.PropertyName!is acceptable in test code, as any null value would correctly fail the test.SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
428-433: TrimBuffer prefix-removal logic is correct.The updated loop removes the expired prefix safely without index invalidation issues.
SysManager/SysManager/Models/DiskHealthReport.cs (1)
79-86: Null temperature fallback is implemented cleanly.This avoids showing a misleading hot-state color when temperature is unavailable.
SysManager/SysManager/Services/ShortcutCleanerService.cs (1)
138-155: COM cleanup hardening looks good.
try/finallywith explicit release materially reduces leak risk in shortcut scans.SysManager/SysManager/Models/BatteryInfo.cs (1)
26-35: Health/Wear clamping change is correct and safe.The new bounds prevent impossible UI values for over-design batteries.
SysManager/SysManager/ViewModels/ServicesViewModel.cs (1)
87-90: Good confirmation-flow migration toDialogService.The early-return confirm pattern is consistent, and it preserves the existing admin-gated execution flow cleanly.
Also applies to: 108-111, 129-132
SysManager/SysManager/ViewModels/PerformanceViewModel.cs (1)
147-150: Consistent prompt abstraction and cancel handling.This conversion to
DialogService.Instance.Confirm(...)is uniform, and the toggle cancel paths still correctly revert UI intent viaSyncTogglesFromProfile().Also applies to: 197-200, 229-232, 261-264, 297-301, 345-348, 379-379, 407-414, 446-449, 482-492
SysManager/SysManager/Services/DialogService.cs (1)
19-23:Confirmresult mapping is correct.The implementation cleanly returns
trueonly for explicitYes, which keeps caller behavior deterministic.
| public void BlockApp_ValidNames_FormatsCorrectly(string exeName) | ||
| { | ||
| // These are valid names but will fail because we don't have admin access. | ||
| // The important thing is they pass the validation check. | ||
| // The method returns false because of UnauthorizedAccessException, | ||
| // not because of invalid input. We can't distinguish in the return value, | ||
| // but we verify the format is accepted by checking IsBlocked (which also | ||
| // requires registry access and will return false gracefully). | ||
| var result = AppBlockerService.IsBlocked(exeName.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) ? exeName : exeName + ".exe"); | ||
| // IsBlocked reads registry - will return false if no access, but won't throw | ||
| Assert.False(result); // expected since nothing is actually blocked | ||
| } |
There was a problem hiding this comment.
BlockApp_ValidNames_FormatsCorrectly doesn’t exercise BlockApp.
Line 45 calls IsBlocked, so this test doesn’t validate the BlockApp acceptance path its name describes. Replace it with a direct BlockApp call (and assert no-throw / expected false under non-admin) to keep coverage meaningful.
Suggested adjustment
- var result = AppBlockerService.IsBlocked(exeName.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) ? exeName : exeName + ".exe");
- // IsBlocked reads registry - will return false if no access, but won't throw
- Assert.False(result); // expected since nothing is actually blocked
+ var normalized = exeName.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)
+ ? exeName
+ : exeName + ".exe";
+ var ex = Record.Exception(() => AppBlockerService.BlockApp(normalized));
+ Assert.Null(ex);🤖 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.Tests/AppBlockerServiceValidationTests.cs` around lines
37 - 48, The test BlockApp_ValidNames_FormatsCorrectly is calling
AppBlockerService.IsBlocked instead of exercising the BlockApp acceptance path;
change the call to AppBlockerService.BlockApp (using the same exeName
normalization: append ".exe" when needed) and assert the expected behavior under
non-admin (e.g., that it does not throw and returns false/indicates failure due
to lack of privileges), rather than calling IsBlocked; keep the existing comment
about registry/admin behavior and adjust the Assert to verify BlockApp's
return/exception behavior accordingly.
| public void LargeFileEntry_LastModifiedDisplay_FormatsCorrectly() | ||
| { | ||
| var entry = new LargeFileEntry | ||
| { | ||
| Path = @"C:\test\file.zip", | ||
| Name = "file.zip", | ||
| SizeBytes = 1000, | ||
| LastModified = new DateTime(2024, 3, 1) | ||
| }; | ||
| Assert.Contains("2024", entry.LastModifiedDisplay); | ||
| Assert.Contains("Mar", entry.LastModifiedDisplay); | ||
| } |
There was a problem hiding this comment.
Locale-dependent assertions may be flaky across environments.
Line 126 ("Mar") and Line 154 ("1,500") assume English month names and comma group separators. These can fail on non-en-US test runners.
Suggested tweak
+using System.Globalization;
using SysManager.Models;
@@
- Assert.Contains("Mar", entry.LastModifiedDisplay);
+ Assert.Contains(
+ CultureInfo.CurrentCulture.DateTimeFormat.GetAbbreviatedMonthName(3),
+ entry.LastModifiedDisplay);
@@
- Assert.Contains("1,500", cat.CountDisplay);
+ Assert.Contains(1500.ToString("N0", CultureInfo.CurrentCulture), cat.CountDisplay);Also applies to: 144-155
🤖 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.Tests/CleanupCategoryHumanSizeExtendedTests.cs` around
lines 116 - 127, The tests assert locale-specific strings ("Mar" and "1,500")
which will fail on non-en-US runners; update the tests that reference
LargeFileEntry.LastModifiedDisplay and any size formatting assertions to either
(a) compare using an explicit invariant format (e.g., call
LastModified.ToString("MMM", CultureInfo.InvariantCulture) or use a specific
numeric format with CultureInfo.InvariantCulture) or (b) construct the expected
strings using CultureInfo.CurrentCulture before asserting so the test adapts to
the runner locale; locate uses of LargeFileEntry.LastModifiedDisplay and
SizeBytes/LastModified in the failing test methods and replace the hard-coded
"Mar" and "1,500" assertions with one of these locale-safe approaches.
| public void SanitizePath_MultipleUsersInPath_ReplacesFirst() | ||
| { | ||
| var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt"); | ||
| Assert.Contains("[user]", result); | ||
| } |
There was a problem hiding this comment.
Strengthen the “replaces first” assertion
Line 45 only verifies that "[user]" appears somewhere, so this test can pass even if sanitization replaces the wrong segment or replaces both usernames. Please assert the exact expected output (or assert first replaced + second unchanged) to match the test intent.
Suggested test tightening
[Fact]
public void SanitizePath_MultipleUsersInPath_ReplacesFirst()
{
var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt");
- Assert.Contains("[user]", result);
+ Assert.Equal(@"C:\Users\[user]\backup\C:\Users\bob\file.txt", result);
}🤖 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.Tests/LogServiceSanitizeEdgeCaseTests.cs` around lines
42 - 46, The test SanitizePath_MultipleUsersInPath_ReplacesFirst only checks for
presence of "[user]" so it can pass if the wrong segment(s) are sanitized;
update the assertion to verify the exact expected string after calling
LogService.SanitizePath (or assert that only the first user segment was replaced
and the second remains unchanged). Specifically, change the Assert.Contains to
Assert.Equal with the exact expected sanitized path (e.g. expected =
@"C:\Users\[user]\backup\C:\Users\bob\file.txt") or use two asserts that check
the first occurrence is replaced and the second still contains "bob",
referencing LogService.SanitizePath and the test
SanitizePath_MultipleUsersInPath_ReplacesFirst.
|
|
||
| public class OperationLockServiceEdgeCaseTests | ||
| { | ||
| private static OperationLockService Service => OperationLockService.Instance; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether OperationLockService tests are isolated from parallel execution
# and whether singleton usage appears across test files.
rg -n --type=cs -C2 'OperationLockService\.Instance'
rg -n --type=cs -C2 '\[Collection\(|\[CollectionDefinition\(|DisableParallelization'Repository: laurentiu021/SystemManager
Length of output: 23077
🏁 Script executed:
# Read the complete OperationLockServiceEdgeCaseTests.cs file to verify collection attributes and test structure
cat -n SysManager/SysManager.Tests/OperationLockServiceEdgeCaseTests.csRepository: laurentiu021/SystemManager
Length of output: 6044
🏁 Script executed:
# Examine OperationLockServiceTests.cs to see if it has similar isolation issues
cat -n SysManager/SysManager.Tests/OperationLockServiceTests.csRepository: laurentiu021/SystemManager
Length of output: 4204
Add [Collection] attribute to isolate tests from parallel execution.
Line 8 uses OperationLockService.Instance, a singleton with mutable state. Without a collection attribute, this class runs in parallel with other tests by default in xUnit. Pre-clean patterns at lines 79–93 and 133–136 are insufficient because they cannot prevent locks held by concurrent tests in other test classes, causing flaky failures. Add [Collection("OperationLock", DisableParallelization = true)] to both OperationLockServiceTests and OperationLockServiceEdgeCaseTests.
Additionally, unsubscribe the PropertyChanged handler at line 120 to prevent event accumulation on the singleton, and move cleanup at lines 150–152 into a finally block to prevent cascading failures if the assertion fails.
🤖 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.Tests/OperationLockServiceEdgeCaseTests.cs` at line 8,
Add a test collection attribute to isolate the singleton: annotate both test
classes OperationLockServiceTests and OperationLockServiceEdgeCaseTests with
[Collection("OperationLock", DisableParallelization = true)] so xUnit won't run
them in parallel; unsubscribe the PropertyChanged handler you attach to
OperationLockService.Instance (use OperationLockService.Instance.PropertyChanged
-= <handler>) after the test to avoid event accumulation on the singleton; and
move the existing cleanup statements that reset/clear the singleton state (the
cleanup currently run after the assertion) into a finally block so cleanup
always runs even if assertions fail.
| Service.PropertyChanged += (_, e) => changed.Add(e.PropertyName!); | ||
|
|
||
| var handle = Service.TryAcquire(OperationCategory.Disk, "PropChanged"); | ||
| Assert.NotNull(handle); | ||
| handle.Dispose(); | ||
|
|
||
| Assert.Contains("ActiveOperations", changed); | ||
| Assert.Contains("HasActiveOperations", changed); | ||
| } |
There was a problem hiding this comment.
Unsubscribe PropertyChanged handler to avoid leaking callbacks across tests.
At Line 120, the handler is attached to a singleton service and never removed. That can accumulate listeners and create cross-test side effects.
Proposed fix
- var changed = new List<string>();
- Service.PropertyChanged += (_, e) => changed.Add(e.PropertyName!);
-
- var handle = Service.TryAcquire(OperationCategory.Disk, "PropChanged");
- Assert.NotNull(handle);
- handle.Dispose();
-
- Assert.Contains("ActiveOperations", changed);
- Assert.Contains("HasActiveOperations", changed);
+ var changed = new List<string>();
+ PropertyChangedEventHandler handler = (_, e) => changed.Add(e.PropertyName!);
+ Service.PropertyChanged += handler;
+ try
+ {
+ var handle = Service.TryAcquire(OperationCategory.Disk, "PropChanged");
+ Assert.NotNull(handle);
+ handle.Dispose();
+
+ Assert.Contains("ActiveOperations", changed);
+ Assert.Contains("HasActiveOperations", changed);
+ }
+ finally
+ {
+ Service.PropertyChanged -= handler;
+ }🤖 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.Tests/OperationLockServiceEdgeCaseTests.cs` around
lines 120 - 128, The test attaches an event handler to Service.PropertyChanged
but never removes it, causing listener leaks across tests; modify the test to
store the handler in a local variable (e.g., PropertyChangedEventHandler handler
= (_, e) => changed.Add(e.PropertyName!)), attach it with
Service.PropertyChanged += handler before calling Service.TryAcquire, and ensure
you unsubscribe after the assertions with Service.PropertyChanged -= handler (or
wrap attach/unsubscribe in a try/finally) so the singleton Service does not
retain callbacks between tests.
| await Task.WhenAll(tasks); | ||
|
|
||
| Assert.Equal(1, successCount); | ||
|
|
||
| foreach (var h in handles) h?.Dispose(); | ||
| } |
There was a problem hiding this comment.
Dispose acquired handles in finally to prevent cascading test pollution.
At Line 150–153, if Assert.Equal(1, successCount) fails, cleanup is skipped and the lock can remain held, affecting later tests.
Proposed fix
- await Task.WhenAll(tasks);
-
- Assert.Equal(1, successCount);
-
- foreach (var h in handles) h?.Dispose();
+ await Task.WhenAll(tasks);
+ try
+ {
+ Assert.Equal(1, successCount);
+ }
+ finally
+ {
+ foreach (var h in handles) h?.Dispose();
+ }🤖 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.Tests/OperationLockServiceEdgeCaseTests.cs` around
lines 148 - 153, The test currently disposes acquired lock handles only after
the Assert, so a failed assertion can skip cleanup and leave locks held; update
the test method (the block using tasks, successCount, and handles in
OperationLockServiceEdgeCaseTests) to perform cleanup in a finally: await
Task.WhenAll(tasks) as before, then move Assert.Equal(1, successCount) inside a
try and dispose all entries in the handles array (null-checking each) in a
finally block so handles are always released even on assertion failures.
| public sealed class DialogService : IDialogService | ||
| { | ||
| /// <summary>Shared singleton instance for ViewModels without DI.</summary> | ||
| public static IDialogService Instance { get; set; } = new DialogService(); |
There was a problem hiding this comment.
Harden singleton assignment to prevent global null/swap hazards.
On Line 16, Instance is publicly settable and accepts null, which can break all confirm call sites at runtime. Guard assignment (or restrict setter visibility).
Suggested fix
public sealed class DialogService : IDialogService
{
/// <summary>Shared singleton instance for ViewModels without DI.</summary>
- public static IDialogService Instance { get; set; } = new DialogService();
+ private static IDialogService _instance = new DialogService();
+ public static IDialogService Instance
+ {
+ get => _instance;
+ set => _instance = value ?? throw new ArgumentNullException(nameof(value));
+ }📝 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.
| public static IDialogService Instance { get; set; } = new DialogService(); | |
| private static IDialogService _instance = new DialogService(); | |
| public static IDialogService Instance | |
| { | |
| get => _instance; | |
| set => _instance = value ?? throw new ArgumentNullException(nameof(value)); | |
| } |
🤖 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/DialogService.cs` at line 16, The public
static Instance property on IDialogService is writable and can be set to null,
risking NREs in confirm call sites; change the property to prevent unsafe
assignments by either making the setter non-public (e.g., private set) and
initializing with new DialogService(), or implement a guarded setter that throws
on null (or rejects null) so Instance can never be swapped to null; update
references that previously set Instance directly to use an explicit
Initialize/Replace method if needed and keep the type IDialogService and
concrete DialogService names to locate the property.
| // Verify download integrity: compute SHA256 and compare against known-good hash. | ||
| // Ookla CLI 1.2.0 hashes pinned from verified downloads. | ||
| await Task.Run(() => | ||
| { | ||
| var hash = Convert.ToHexString(SHA256.HashData(File.ReadAllBytes(zipPath))); | ||
| Log.Information("Ookla CLI downloaded: {Url}, SHA256={Hash}, Size={Size}", | ||
| zipUrl, hash, new FileInfo(zipPath).Length); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET, does X509Certificate.CreateFromSignedFile verify Authenticode trust/chain validity, or does it only extract the embedded signing certificate?
💡 Result:
No. In.NET, X509Certificate.CreateFromSignedFile only extracts/loads the certificate embedded in (or otherwise derivable from) the signed file; it does not verify Authenticode trust/chain validity. Evidence: - The method’s implementation is effectively just a wrapper that returns new X509Certificate(filename) rather than performing Authenticode verification or trust checking. [1] - A.NET runtime issue explicitly states that X509Certificate.CreateFromSignedFile “doesn't check that the contents are Authenticode” and is “just a vanity wrapper over new X509Certificate2(bytes),” i.e., not an Authenticode trust/chain validator. [2] - The actual trust/chain validation APIs are separate (e.g., X509Certificate2.Verify builds a chain and performs chain validation; X509Chain.Build likewise builds/validates using X509ChainPolicy). [3][4] So if you want to validate an Authenticode signature’s signer trust/chain, you must use an Authenticode-specific verification path (e.g., Windows Authenticode/WinTrust) or extract signer info and then separately perform chain validation with X509Chain/X509Certificate2.Verify, depending on what “trust” means in your scenario. [5][6][3][4]
Citations:
- 1: https://github.com/dotnet/dotnet/blob/17d11de66cf75b962995c81dd1235fae9aa5ece0/src/runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Certificate.cs
- 2: There is no non-Obsolete way to read an Authenticode signer certificate in .NET 9 dotnet/runtime#109941
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2.verify?view=net-9.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509chain.build?view=net-9.0
- 5: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.authenticodesignatureinformation.signaturechain?view=netframework-4.8
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.authenticodesignatureinformation?view=netframework-4.8.1
🏁 Script executed:
# Locate and read the SpeedTestService.cs file to examine lines 295-301 and 327-341
fd -t f "SpeedTestService.cs" | head -1 | xargs wc -lRepository: laurentiu021/SystemManager
Length of output: 125
🏁 Script executed:
fd -t f "SpeedTestService.cs" | head -1 | xargs cat -n | sed -n '285,350p'Repository: laurentiu021/SystemManager
Length of output: 3260
Authenticode validation is not enforced and hash pinning is not performed.
Line 295–296 claim "verify download integrity: compute SHA256 and compare against known-good hash" and state hashes are "pinned," but lines 299–301 only compute and log the hash—there is no comparison against any pinned value.
Similarly, line 332 uses X509Certificate.CreateFromSignedFile, which only extracts the embedded certificate and does not verify Authenticode trust chain validity. Line 333's substring check (cert.Subject.Contains("Ookla")) is insufficient; lines 334 and 340 merely log warnings without failing. This creates a false sense of supply-chain security.
Fix: Implement actual hash pinning with hard-coded validation and fail-closed behavior on mismatch, or remove the misleading comments. For Authenticode, use proper Windows trust verification (e.g., WinVerifyTrust) or X509Chain validation with explicit trust requirements, and throw on signature failure.
Also applies to: 327–341
🤖 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/SpeedTestService.cs` around lines 295 - 301,
The code only computes and logs SHA256 (SHA256.HashData + Log.Information) and
extracts the embedded certificate (X509Certificate.CreateFromSignedFile) then
checks cert.Subject.Contains("Ookla") but never enforces pinning or signature
trust; update the download verification to compare the computed hash against a
hard-coded expected SHA256 value and throw/fail if it does not match
(fail-closed), and replace the naive certificate check with proper Authenticode
verification (use WinVerifyTrust or build an X509Chain with explicit
trust/RevocationMode and validate the signature) and throw on any
signature/chain validation failure instead of just logging warnings. Ensure
these checks occur in the same routine that writes/verifies zipPath so the
method that downloads/installs the Ookla CLI cannot proceed on mismatch.
…275) fix: prevent buttons from appearing grayed out on window focus loss Intercept WM_NCACTIVATE and force the non-client area to always render as active. ModernWPF's window chrome was dimming controls when the window lost focus, making buttons look disabled. Closes #252, Closes #251, Closes #248, Closes #245
Add CHANGELOG entries for the 4 bug-fix releases from today's session: - **v0.21.3** — buttons grayed out on focus loss (#252, #251, #248, #245) - **v0.21.4** — tab name consistency + log hover (#267, #247) - **v0.21.5** — startup manager disable (#268) - **v0.21.6** — speed test panel states + auto-trace (#257, #239) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
* fix: code review batch 2 — IDialogService, null temp color, battery clamp, TrimBuffer, COM release, Ookla verify * test: update edge-case tests to match QA-004/QA-005 clamping behavior --------- Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
2 participants
Summary
Fixes 6 mustFix + shouldFix issues from the v0.35.11 code review.
Changes
Added
Fixed
New Tests
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Security
Tests