feat: Windows optional features toggle tab#288
Conversation
- Add WindowsFeature model with category classification - Add WindowsFeaturesService wrapping Get-WindowsOptionalFeature PowerShell - Add WindowsFeaturesViewModel with scan, toggle, filter, admin check - Add WindowsFeaturesView.xaml with DataGrid, category/state/toggle columns - Replace WipWindowsFeatures placeholder with real implementation - Register WindowsFeaturesService and VM in DI container - Add 10 unit tests (ParseFeatureList, CategorizeFeature, HumanizeName, VM state) - Update README: remove ⚙️ from Windows Features, add feature description - Update CHANGELOG with v0.46.0 entry Closes #5
📝 WalkthroughWalkthroughThis PR implements a complete Windows optional features management tab for the System group, adding PowerShell-backed functionality to list, toggle, and track Windows features with admin requirements, confirmation prompts, and reboot-required status indicators. The feature includes search/filtering and comprehensive unit test coverage. ChangesWindows Features Management Tab
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! |
| foreach (var line in lines) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(line)) continue; | ||
|
|
||
| var parts = line.Split('|', 2); | ||
| if (parts.Length < 2) continue; | ||
|
|
||
| var name = parts[0].Trim(); | ||
| var state = parts[1].Trim(); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(name)) continue; | ||
|
|
||
| var isEnabled = state.Equals("Enabled", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| features.Add(new WindowsFeature | ||
| { | ||
| Name = name, | ||
| DisplayName = HumanizeName(name), | ||
| IsEnabled = isEnabled, | ||
| Category = WindowsFeature.CategorizeFeature(name) | ||
| }); | ||
| } |
| if (feature.IsEnabled) | ||
| { | ||
| (success, reboot) = await _service.DisableFeatureAsync(feature.Name, _cts.Token); | ||
| } | ||
| else | ||
| { | ||
| (success, reboot) = await _service.EnableFeatureAsync(feature.Name, _cts.Token); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@README.md`:
- Around line 145-152: Update the "Windows Features" docs to include the
fallback "Other" category so docs match the feature model/service; specifically
edit the "### Windows Features" section where the categories are listed (the
line that currently reads "Categorized: Virtualization, Networking, Development,
Media & Print, Legacy") and append or insert "Other" (e.g., "Categorized:
Virtualization, Networking, Development, Media & Print, Legacy, Other") and add
a brief note that "Other" is the fallback bucket used by the feature
model/service for uncategorized items.
In `@SysManager/SysManager/Services/WindowsFeaturesService.cs`:
- Line 131: The current mapping sets isEnabled only when state.Equals("Enabled",
...), which misses values like "Enabled with Payload Removed" or pending
variants; update the logic in WindowsFeaturesService.cs where the local variable
isEnabled is set from state to consider any state that starts with or contains
"Enabled" (e.g., use state?.Trim().StartsWith("Enabled",
StringComparison.OrdinalIgnoreCase) or state?.IndexOf("Enabled",
StringComparison.OrdinalIgnoreCase) >= 0) and ensure you null-check state before
calling methods so those variants are treated as enabled.
- Around line 33-41: The shared PowerShellRunner.LineReceived event handler
(attached in WindowsFeaturesService methods around RunProcessAsync with the
Collect handler) can be invoked concurrently across multiple async calls causing
cross-talk; serialize access by either acquiring the existing
OperationLockService from WindowsFeaturesViewModel before invoking
WindowsFeaturesService methods (follow the same lock pattern used elsewhere) or
add a private SemaphoreSlim (e.g., _runSemaphore) to WindowsFeaturesService and
await _runSemaphore.WaitAsync()/Release() around the sections that attach/detach
LineReceived and call RunProcessAsync so only one Collect handler is active at a
time.
In `@SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs`:
- Around line 50-52: The code disposes and replaces the CancellationTokenSource
field `_cts` without calling `_cts.Cancel()`, which can leave in-flight
`ScanAsync` operations using an orphaned token; before calling `_cts.Dispose()`
and assigning a new `CancellationTokenSource`, call `_cts.Cancel()` (check for
null) to signal any running work, then dispose and replace it; apply this change
in both places where `_cts` is replaced (the existing block around
`_cts?.Dispose(); _cts = new CancellationTokenSource();` and the other
replacement near the same pattern) so `ToggleFeatureAsync`/`ScanAsync`
interactions are properly cancelled.
In `@SysManager/SysManager/Views/WindowsFeaturesView.xaml`:
- Around line 45-52: Replace the emoji-only status markers in the two TextBlock
elements so they use plain, accessible labels (and optional icon controls)
instead of leading emoji; specifically update the TextBlocks that are inside the
Border elements bound to PendingReboot and IsElevated (the TextBlocks currently
with Text="⚠ Reboot pending" and Text="✓ Administrator") to use descriptive
plain text like "Reboot pending" and "Administrator" and, if an icon is desired,
use an Image/Icon control or an accessible glyph with AutomationProperties.Name
set, ensuring the PendingReboot and IsElevated bindings remain unchanged.
🪄 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: 7be4ff2d-1372-485d-a34e-8734c0e9a94d
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdSysManager/SysManager.Tests/WindowsFeaturesTests.csSysManager/SysManager/Models/WindowsFeature.csSysManager/SysManager/ServiceRegistration.csSysManager/SysManager/Services/WindowsFeaturesService.csSysManager/SysManager/ViewModels/MainWindowViewModel.csSysManager/SysManager/ViewModels/WindowsFeaturesViewModel.csSysManager/SysManager/Views/WindowsFeaturesView.xamlSysManager/SysManager/Views/WindowsFeaturesView.xaml.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)
CHANGELOG.md (1)
9-17: LGTM!README.md (1)
46-46: LGTM!SysManager/SysManager/ServiceRegistration.cs (1)
56-57: LGTM!SysManager/SysManager/Views/WindowsFeaturesView.xaml.cs (1)
1-12: LGTM!SysManager/SysManager/ViewModels/MainWindowViewModel.cs (1)
41-42: LGTM!Also applies to: 104-105, 138-139, 187-188, 348-349
SysManager/SysManager/Views/WindowsFeaturesView.xaml (1)
1-44: LGTM!Also applies to: 53-175
SysManager/SysManager/Services/WindowsFeaturesService.cs (1)
1-130: LGTM!Also applies to: 132-161
SysManager/SysManager/Models/WindowsFeature.cs (1)
1-55: LGTM!SysManager/SysManager.Tests/WindowsFeaturesTests.cs (1)
1-132: LGTM!SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs (1)
1-49: LGTM!Also applies to: 53-98, 102-181
| ### Windows Features | ||
| - Lists all Windows optional features with current state (Enabled/Disabled) | ||
| - Toggle enable/disable per feature with confirmation dialog | ||
| - Categorized: Virtualization, Networking, Development, Media & Print, Legacy | ||
| - Shows reboot-required status after toggling | ||
| - Search/filter across all features | ||
| - Requires administrator privileges for modifications | ||
|
|
There was a problem hiding this comment.
Include the “Other” category in the feature docs.
Line 148 lists categories but omits the fallback Other bucket used by the feature model/service, so docs can diverge from what users see in the UI.
Suggested doc tweak
-- Categorized: Virtualization, Networking, Development, Media & Print, Legacy
+- Categorized: Virtualization, Networking, Development, Media & Print, Legacy, Other📝 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.
| ### Windows Features | |
| - Lists all Windows optional features with current state (Enabled/Disabled) | |
| - Toggle enable/disable per feature with confirmation dialog | |
| - Categorized: Virtualization, Networking, Development, Media & Print, Legacy | |
| - Shows reboot-required status after toggling | |
| - Search/filter across all features | |
| - Requires administrator privileges for modifications | |
| ### Windows Features | |
| - Lists all Windows optional features with current state (Enabled/Disabled) | |
| - Toggle enable/disable per feature with confirmation dialog | |
| - Categorized: Virtualization, Networking, Development, Media & Print, Legacy, Other | |
| - Shows reboot-required status after toggling | |
| - Search/filter across all features | |
| - Requires administrator privileges for modifications |
🤖 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 `@README.md` around lines 145 - 152, Update the "Windows Features" docs to
include the fallback "Other" category so docs match the feature model/service;
specifically edit the "### Windows Features" section where the categories are
listed (the line that currently reads "Categorized: Virtualization, Networking,
Development, Media & Print, Legacy") and append or insert "Other" (e.g.,
"Categorized: Virtualization, Networking, Development, Media & Print, Legacy,
Other") and add a brief note that "Other" is the fallback bucket used by the
feature model/service for uncategorized items.
| _runner.LineReceived += Collect; | ||
| try | ||
| { | ||
| await _runner.RunProcessAsync("powershell", | ||
| "-NoProfile -Command \"Get-WindowsOptionalFeature -Online | " + | ||
| "Select-Object FeatureName, State | " + | ||
| "ForEach-Object { $_.FeatureName + '|' + $_.State }\"", ct); | ||
| } | ||
| finally { _runner.LineReceived -= Collect; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect PowerShellRunner implementation for serialization and event dispatch semantics
fd -i "PowerShellRunner.cs" --exec sed -n '1,320p' {}
# 2) Locate synchronization primitives and LineReceived usage
rg -nP --type=cs -C3 'class\s+PowerShellRunner|event\s+.*LineReceived|RunProcessAsync\s*\(|SemaphoreSlim|lock\s*\('
# 3) Confirm whether any callers assume exclusive output capture via event subscription
rg -nP --type=cs -C2 'LineReceived\s*\+=|LineReceived\s*-='Repository: laurentiu021/SystemManager
Length of output: 50382
🏁 Script executed:
# Check if WindowsFeaturesService or its callers use OperationLockService
rg -nP --type=cs -C3 "WindowsFeaturesService|OperationLockService" | head -100Repository: laurentiu021/SystemManager
Length of output: 9427
🏁 Script executed:
# Search for actual concurrent calls to RunProcessAsync in the codebase
# Look for any async methods calling RunProcessAsync without await or Task.WhenAll scenarios
rg -nP --type=cs -B2 -A2 "RunProcessAsync" SysManager/SysManager/Services/WindowsFeaturesService.csRepository: laurentiu021/SystemManager
Length of output: 815
🏁 Script executed:
# Check if WindowsFeaturesService methods are decorated with synchronization attributes or use locks
sed -n '1,150p' SysManager/SysManager/Services/WindowsFeaturesService.csRepository: laurentiu021/SystemManager
Length of output: 5469
Prevent concurrent LineReceived event capture across multiple async operations on the shared PowerShellRunner instance.
Lines 33, 64, and 98 attach temporary event handlers during RunProcessAsync calls; if multiple service methods execute concurrently (which is currently possible since WindowsFeaturesViewModel does not use OperationLockService), output from unrelated processes will bleed between collectors, causing silent parsing failures or data corruption.
Recommended fixes:
- Add
OperationLockServicesynchronization inWindowsFeaturesViewModel(pattern used consistently elsewhere in codebase), or - Introduce a
SemaphoreSliminWindowsFeaturesServiceto serialize its public methods
🤖 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/WindowsFeaturesService.cs` around lines 33 -
41, The shared PowerShellRunner.LineReceived event handler (attached in
WindowsFeaturesService methods around RunProcessAsync with the Collect handler)
can be invoked concurrently across multiple async calls causing cross-talk;
serialize access by either acquiring the existing OperationLockService from
WindowsFeaturesViewModel before invoking WindowsFeaturesService methods (follow
the same lock pattern used elsewhere) or add a private SemaphoreSlim (e.g.,
_runSemaphore) to WindowsFeaturesService and await
_runSemaphore.WaitAsync()/Release() around the sections that attach/detach
LineReceived and call RunProcessAsync so only one Collect handler is active at a
time.
|
|
||
| if (string.IsNullOrWhiteSpace(name)) continue; | ||
|
|
||
| var isEnabled = state.Equals("Enabled", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Handle non-exact enabled states when mapping IsEnabled.
Line 131 only treats exact "Enabled" as enabled; states like "Enabled with Payload Removed" (and pending variants) will be shown as disabled.
Suggested parsing adjustment
- var isEnabled = state.Equals("Enabled", StringComparison.OrdinalIgnoreCase);
+ var isEnabled =
+ state.StartsWith("Enabled", StringComparison.OrdinalIgnoreCase) ||
+ state.StartsWith("Enable Pending", StringComparison.OrdinalIgnoreCase);📝 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.
| var isEnabled = state.Equals("Enabled", StringComparison.OrdinalIgnoreCase); | |
| var isEnabled = | |
| state.StartsWith("Enabled", StringComparison.OrdinalIgnoreCase) || | |
| state.StartsWith("Enable Pending", StringComparison.OrdinalIgnoreCase); |
🤖 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/WindowsFeaturesService.cs` at line 131, The
current mapping sets isEnabled only when state.Equals("Enabled", ...), which
misses values like "Enabled with Payload Removed" or pending variants; update
the logic in WindowsFeaturesService.cs where the local variable isEnabled is set
from state to consider any state that starts with or contains "Enabled" (e.g.,
use state?.Trim().StartsWith("Enabled", StringComparison.OrdinalIgnoreCase) or
state?.IndexOf("Enabled", StringComparison.OrdinalIgnoreCase) >= 0) and ensure
you null-check state before calling methods so those variants are treated as
enabled.
| _cts?.Dispose(); | ||
| _cts = new CancellationTokenSource(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'WindowsFeaturesViewModel.cs|WindowsFeaturesView.xaml|ViewModelBase.cs' SysManager
echo "---- CTS replacement sites ----"
rg -n -C2 '_cts\?\.Dispose\(\)|new CancellationTokenSource\(\)|_cts\?\.Cancel\(\)' \
SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs
echo "---- Command enablement / busy gating in view ----"
rg -n -C3 'ScanCommand|ToggleFeatureCommand|CancelCommand|IsEnabled|IsBusy|CanExecute' \
SysManager/SysManager/Views/WindowsFeaturesView.xaml \
SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs \
SysManager/SysManager/ViewModels/ViewModelBase.csRepository: laurentiu021/SystemManager
Length of output: 13682
🏁 Script executed:
cat -n SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs | sed -n '77,88p'Repository: laurentiu021/SystemManager
Length of output: 436
🏁 Script executed:
rg -n '\[RelayCommand\]|CanExecute|NotifyCanExecuteChanged' SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs | head -20Repository: laurentiu021/SystemManager
Length of output: 137
🏁 Script executed:
cat -n SysManager/SysManager/ViewModels/ViewModelBase.csRepository: laurentiu021/SystemManager
Length of output: 1314
Add explicit Cancel() before disposing/replacing the CancellationTokenSource
At lines 50–51 and 99–100, _cts is disposed and replaced without an explicit Cancel() call. This creates a concurrency risk: if ToggleFeatureAsync is invoked while ScanAsync is running, the new CancellationTokenSource assignment invalidates the cancellation token held by the in-flight ScanAsync operation, potentially allowing stale work to continue undetected.
Proposed fix
- _cts?.Dispose();
+ _cts?.Cancel();
+ _cts?.Dispose();
_cts = new CancellationTokenSource();Apply the same fix at lines 99–100.
📝 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.
| _cts?.Dispose(); | |
| _cts = new CancellationTokenSource(); | |
| _cts?.Cancel(); | |
| _cts?.Dispose(); | |
| _cts = new CancellationTokenSource(); |
🤖 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/WindowsFeaturesViewModel.cs` around lines 50
- 52, The code disposes and replaces the CancellationTokenSource field `_cts`
without calling `_cts.Cancel()`, which can leave in-flight `ScanAsync`
operations using an orphaned token; before calling `_cts.Dispose()` and
assigning a new `CancellationTokenSource`, call `_cts.Cancel()` (check for null)
to signal any running work, then dispose and replace it; apply this change in
both places where `_cts` is replaced (the existing block around
`_cts?.Dispose(); _cts = new CancellationTokenSource();` and the other
replacement near the same pattern) so `ToggleFeatureAsync`/`ScanAsync`
interactions are properly cancelled.
| <Border Background="#2D1F1F" CornerRadius="4" Padding="6,2" Margin="4,0" | ||
| Visibility="{Binding PendingReboot, Converter={StaticResource BoolToVis}}"> | ||
| <TextBlock Text="⚠ Reboot pending" FontSize="11" Foreground="#F59E0B"/> | ||
| </Border> | ||
| <Border Background="#1F2D1F" CornerRadius="4" Padding="6,2" Margin="4,0" | ||
| Visibility="{Binding IsElevated, Converter={StaticResource BoolToVis}}"> | ||
| <TextBlock Text="✓ Administrator" FontSize="11" Foreground="#4ADE80"/> | ||
| </Border> |
There was a problem hiding this comment.
Avoid emoji-only status cues in badge text.
Line 47 and Line 51 include emoji markers (⚠, ✓) in user-facing status labels; plain text (and optional icon controls) is more accessible and consistent.
Suggested tweak
- <TextBlock Text="⚠ Reboot pending" FontSize="11" Foreground="#F59E0B"/>
+ <TextBlock Text="Reboot pending" FontSize="11" Foreground="#F59E0B"/>
- <TextBlock Text="✓ Administrator" FontSize="11" Foreground="#4ADE80"/>
+ <TextBlock Text="Administrator" FontSize="11" Foreground="#4ADE80"/>🤖 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/Views/WindowsFeaturesView.xaml` around lines 45 - 52,
Replace the emoji-only status markers in the two TextBlock elements so they use
plain, accessible labels (and optional icon controls) instead of leading emoji;
specifically update the TextBlocks that are inside the Border elements bound to
PendingReboot and IsElevated (the TextBlocks currently with Text="⚠ Reboot
pending" and Text="✓ Administrator") to use descriptive plain text like "Reboot
pending" and "Administrator" and, if an icon is desired, use an Image/Icon
control or an accessible glyph with AutomationProperties.Name set, ensuring the
PendingReboot and IsElevated bindings remain unchanged.
## Summary SFC and DISM auto-relaunched the application with admin privileges without asking the user first. This is a security concern — the user must always be in control of privilege elevation. ## Changes ### CleanupViewModel.cs - **RunSfcAsync**: Replaced silent auto-elevation with a Yes/No MessageBox confirmation dialog. If the user declines, the operation is cancelled with a clear status message. - **RunDismAsync**: Same pattern — explicit consent required before elevation. ## Security impact - Users are no longer surprised by UAC prompts or unexpected app restarts - Declining elevation gracefully cancels the operation instead of leaving the app in an undefined state ## Testing - Build: 0 errors - When not elevated + user clicks Yes: app restarts with admin (existing behavior, now with consent) - When not elevated + user clicks No: operation cancelled, status message shown - When already elevated: SFC/DISM runs normally (no dialog shown) Closes #264 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
- Add WindowsFeature model with category classification - Add WindowsFeaturesService wrapping Get-WindowsOptionalFeature PowerShell - Add WindowsFeaturesViewModel with scan, toggle, filter, admin check - Add WindowsFeaturesView.xaml with DataGrid, category/state/toggle columns - Replace WipWindowsFeatures placeholder with real implementation - Register WindowsFeaturesService and VM in DI container - Add 10 unit tests (ParseFeatureList, CategorizeFeature, HumanizeName, VM state) - Update README: remove ⚙️ from Windows Features, add feature description - Update CHANGELOG with v0.46.0 entry Closes #5 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Summary
Replaces the Windows Features placeholder with a fully functional tab that lists, enables, and disables Windows optional features.
What changed
Features
Testing
Closes #5
Summary by CodeRabbit
Release Notes
New Features
Tests