Repository navigation
feat: uninstaller local app support via registry UninstallString - #284
Merged
Merged
Conversation
- Add UninstallString and QuietUninstallString properties to InstalledApp model - Capture uninstall commands from registry during enrichment (HKLM + HKCU) - Add UninstallLocalAsync method that executes registry uninstall commands - Add ParseUninstallCommand parser handling quoted paths, MsiExec, rundll32 - ViewModel routes local apps (empty Source) to registry uninstall path - Update tooltip for Local badge in UninstallerView - Add 8 unit tests for ParseUninstallCommand covering all code paths - Update README Uninstaller section with local app support - Update CHANGELOG with v0.44.0 entry Closes #236
📝 WalkthroughWalkthroughThis PR extends the uninstaller to uninstall local applications not managed by winget. The implementation adds observable properties to capture registry uninstall strings, enriches the registry read with QuietUninstallString values, parses various uninstall command formats (quoted paths, MsiExec, rundll32), and routes local apps through a new UninstallLocalAsync execution path with comprehensive test coverage. ChangesLocal App Uninstallation via Registry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+111
to
+121
| if (string.IsNullOrWhiteSpace(app.Source) | ||
| && !string.IsNullOrWhiteSpace(app.UninstallString)) | ||
| { | ||
| // Local app — use registry UninstallString directly | ||
| code = await _service.UninstallLocalAsync(app, _cts.Token); | ||
| } | ||
| else | ||
| { | ||
| // Winget-managed app — use winget uninstall | ||
| code = await _service.UninstallAsync(app.Id, _cts.Token); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
SysManager/SysManager.Tests/UninstallerServiceTests.cs (1)
213-288: ⚡ Quick winGood test coverage, but consider adding edge-case tests for the MsiExec parsing.
The 8 new tests comprehensively cover the main branches of
ParseUninstallCommand. However, none of the tests verify that the/I→/Xreplacement (line 289 in the implementation) doesn't incorrectly match substrings.📝 Suggested additional test to catch substring replacement bug
[Fact] public void ParseUninstallCommand_MsiExecWithInstallationArg_DoesNotCorrupt() { // Ensure /I in longer words like /Installation is not replaced var (exe, args) = UninstallerService.ParseUninstallCommand( "MsiExec.exe /Installation /I{12345-GUID}"); Assert.Equal("MsiExec.exe", exe); Assert.Contains("/Installation", args); // Should NOT become /Xnstallation Assert.Contains("/X{12345-GUID}", args); // Should replace /I{GUID} }This test would fail with the current implementation (revealing the bug) and pass after applying the regex fix suggested in the earlier comment.
🤖 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/UninstallerServiceTests.cs` around lines 213 - 288, ParseUninstallCommand is currently replacing "/I" substrings too broadly (line with /I → /X replacement in UninstallerService.ParseUninstallCommand), which corrupts longer tokens like "/Installation"; fix by changing the replace logic to use a regex that matches only a standalone MSI install switch (case-insensitive "/I" or "/i" as a token immediately followed by a GUID brace or token boundary) and replace only those matches with "/X", leaving longer words intact; update ParseUninstallCommand accordingly and ensure the replacement occurs before adding "/quiet".
🤖 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/UninstallerService.cs`:
- Around line 280-295: The MSI argument transform in UninstallerService is using
args.Replace("/I", "/X", ...) which incorrectly replaces substrings; update the
logic in the MsiExec handling (the block using command, exe, args) to only
replace the standalone modify flag `/I` when it is followed by whitespace or a
`{` (GUID) — e.g. perform a targeted substitution using a regex like
`/I(?=[\s{])` or parse args tokens and replace only the token that equals `/I`
or starts with `/I{`; after that keep the existing checks that add ` /quiet
/norestart` when neither `/quiet` nor `/qn` are present.
- Around line 290-292: The current check using args.Contains("/quiet"...) can
match substrings; update the conditional in UninstallerService (the args
variable check) to detect standalone flags using precise matching (e.g.,
Regex.IsMatch with patterns like @"(?:^|\s)/quiet(?:\s|$)" and
@"(?:^|\s)/qn(?:\s|$)" or by tokenizing args on whitespace and comparing
tokens), and before appending " /norestart" ensure you only add it if a similar
standalone "/norestart" flag is not already present using the same precise
check.
---
Nitpick comments:
In `@SysManager/SysManager.Tests/UninstallerServiceTests.cs`:
- Around line 213-288: ParseUninstallCommand is currently replacing "/I"
substrings too broadly (line with /I → /X replacement in
UninstallerService.ParseUninstallCommand), which corrupts longer tokens like
"/Installation"; fix by changing the replace logic to use a regex that matches
only a standalone MSI install switch (case-insensitive "/I" or "/i" as a token
immediately followed by a GUID brace or token boundary) and replace only those
matches with "/X", leaving longer words intact; update ParseUninstallCommand
accordingly and ensure the replacement occurs before adding "/quiet".
🪄 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: ba4e648e-80af-4c52-be0b-ba8160e3d062
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdSysManager/SysManager.Tests/UninstallerServiceTests.csSysManager/SysManager/Models/InstalledApp.csSysManager/SysManager/Services/UninstallerService.csSysManager/SysManager/ViewModels/UninstallerViewModel.csSysManager/SysManager/Views/UninstallerView.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 (7)
README.md (1)
215-215: LGTM!CHANGELOG.md (1)
9-16: LGTM!SysManager/SysManager/Models/InstalledApp.cs (1)
24-25: LGTM!SysManager/SysManager/Services/UninstallerService.cs (2)
203-213: LGTM!
235-255: LGTM!SysManager/SysManager/ViewModels/UninstallerViewModel.cs (1)
110-121: LGTM!SysManager/SysManager/Views/UninstallerView.xaml (1)
142-142: LGTM!
Comment on lines
+280
to
+295
| // Case 2: MsiExec — common pattern: MsiExec.exe /I{GUID} or /X{GUID} | ||
| if (command.StartsWith("MsiExec", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var spaceIdx = command.IndexOf(' '); | ||
| if (spaceIdx > 0) | ||
| { | ||
| var exe = command[..spaceIdx]; | ||
| var args = command[(spaceIdx + 1)..].TrimStart(); | ||
| // Convert /I (modify) to /X (uninstall) if needed, add /quiet | ||
| args = args.Replace("/I", "/X", StringComparison.OrdinalIgnoreCase); | ||
| if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase) | ||
| && !args.Contains("/qn", StringComparison.OrdinalIgnoreCase)) | ||
| args += " /quiet /norestart"; | ||
| return (exe, args); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: string.Replace will incorrectly match "/I" as a substring.
Line 289 uses args.Replace("/I", "/X", ...) which replaces ALL occurrences of the substring "/I", even when it's part of a longer word. This will corrupt arguments like:
/Installation→/Xnstallation/interactive→/Xnteractive
The intent is to replace the /I flag (modify) with /X (uninstall) for MSI commands, but this requires checking that /I is followed by { (GUID) or whitespace.
🐛 Proposed fix using Regex for targeted /I → /X replacement
var exe = command[..spaceIdx];
var args = command[(spaceIdx + 1)..].TrimStart();
- // Convert /I (modify) to /X (uninstall) if needed, add /quiet
- args = args.Replace("/I", "/X", StringComparison.OrdinalIgnoreCase);
+ // Convert /I{GUID} (modify) to /X{GUID} (uninstall) if needed
+ args = System.Text.RegularExpressions.Regex.Replace(
+ args,
+ @"/I(?=[\s{])",
+ "/X",
+ System.Text.RegularExpressions.RegexOptions.IgnoreCase);
if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase)The regex /I(?=[\s{]) matches /I only when followed by whitespace or {, avoiding substring matches.
📝 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.
Suggested change
| // Case 2: MsiExec — common pattern: MsiExec.exe /I{GUID} or /X{GUID} | |
| if (command.StartsWith("MsiExec", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var spaceIdx = command.IndexOf(' '); | |
| if (spaceIdx > 0) | |
| { | |
| var exe = command[..spaceIdx]; | |
| var args = command[(spaceIdx + 1)..].TrimStart(); | |
| // Convert /I (modify) to /X (uninstall) if needed, add /quiet | |
| args = args.Replace("/I", "/X", StringComparison.OrdinalIgnoreCase); | |
| if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase) | |
| && !args.Contains("/qn", StringComparison.OrdinalIgnoreCase)) | |
| args += " /quiet /norestart"; | |
| return (exe, args); | |
| } | |
| } | |
| // Case 2: MsiExec — common pattern: MsiExec.exe /I{GUID} or /X{GUID} | |
| if (command.StartsWith("MsiExec", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var spaceIdx = command.IndexOf(' '); | |
| if (spaceIdx > 0) | |
| { | |
| var exe = command[..spaceIdx]; | |
| var args = command[(spaceIdx + 1)..].TrimStart(); | |
| // Convert /I{GUID} (modify) to /X{GUID} (uninstall) if needed | |
| args = System.Text.RegularExpressions.Regex.Replace( | |
| args, | |
| @"/I(?=[\s{])", | |
| "/X", | |
| System.Text.RegularExpressions.RegexOptions.IgnoreCase); | |
| if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase) | |
| && !args.Contains("/qn", StringComparison.OrdinalIgnoreCase)) | |
| args += " /quiet /norestart"; | |
| return (exe, args); | |
| } | |
| } |
🤖 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/UninstallerService.cs` around lines 280 - 295,
The MSI argument transform in UninstallerService is using args.Replace("/I",
"/X", ...) which incorrectly replaces substrings; update the logic in the
MsiExec handling (the block using command, exe, args) to only replace the
standalone modify flag `/I` when it is followed by whitespace or a `{` (GUID) —
e.g. perform a targeted substitution using a regex like `/I(?=[\s{])` or parse
args tokens and replace only the token that equals `/I` or starts with `/I{`;
after that keep the existing checks that add ` /quiet /norestart` when neither
`/quiet` nor `/qn` are present.
Comment on lines
+290
to
+292
| if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase) | ||
| && !args.Contains("/qn", StringComparison.OrdinalIgnoreCase)) | ||
| args += " /quiet /norestart"; |
There was a problem hiding this comment.
Minor: Contains check for "/quiet" and "/qn" will match substrings.
Lines 290-291 use Contains to check for "/quiet" and "/qn", which will also match variants like /quieter, /qnb, etc. While these are rare in practice, a more precise check would use word boundaries or ensure the flag is followed by whitespace or end-of-string.
Additionally, /norestart could be duplicated if already present in the command.
💡 Proposed fix using Regex for precise flag detection
- if (!args.Contains("/quiet", StringComparison.OrdinalIgnoreCase)
- && !args.Contains("/qn", StringComparison.OrdinalIgnoreCase))
- args += " /quiet /norestart";
+ // Add /quiet /norestart only if not already present
+ if (!System.Text.RegularExpressions.Regex.IsMatch(args, @"/quiet\b|\b/qn\b", System.Text.RegularExpressions.RegexOptions.IgnoreCase))
+ {
+ if (!args.Contains("/norestart", StringComparison.OrdinalIgnoreCase))
+ args += " /quiet /norestart";
+ else
+ args += " /quiet";
+ }The regex \b word boundaries ensure we match /quiet and /qn as standalone flags.
🤖 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/UninstallerService.cs` around lines 290 - 292,
The current check using args.Contains("/quiet"...) can match substrings; update
the conditional in UninstallerService (the args variable check) to detect
standalone flags using precise matching (e.g., Regex.IsMatch with patterns like
@"(?:^|\s)/quiet(?:\s|$)" and @"(?:^|\s)/qn(?:\s|$)" or by tokenizing args on
whitespace and comparing tokens), and before appending " /norestart" ensure you
only add it if a similar standalone "/norestart" flag is not already present
using the same precise check.
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
) ## Summary Offloads synchronous operations that were running on the WPF dispatcher thread to the thread pool, eliminating UI freezes of 1-10 seconds across three tabs. ## Changes ### PowerShellRunner.cs - **RunProcessAsync**: Wrap \Process.Start()\ + \BeginOutputReadLine()\ + \BeginErrorReadLine()\ in \Task.Run\ — process creation (especially \powershell.exe\ with its slow startup) no longer blocks the UI thread - **RunAsync**: Offload \ unspace.Open()\ to thread pool — in-process PowerShell runspace initialization no longer blocks the dispatcher ### SpeedTestService.cs - **EnsureOoklaAsync**: Move all synchronous file-system I/O (\Directory.CreateDirectory\, \File.Exists\, \FileInfo.Length\, \ZipFile.ExtractToDirectory\) to \Task.Run\ - **RunOoklaAsync**: Offload \Process.Start\ for \speedtest.exe\ to thread pool ### DeepCleanupViewModel.cs - **ScanAsync**: Separate \PropertyChanged\ event wiring from \Categories.Add()\ loop to reduce per-item UI re-renders during collection population ## Testing - Build: 0 errors (main project + tests) - Existing tests unaffected — changes are purely async scheduling, no behavior change - All \ConfigureAwait(false)\ applied where continuation doesn't need UI context Closes #261, Closes #258, Closes #249 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
- Add UninstallString and QuietUninstallString properties to InstalledApp model - Capture uninstall commands from registry during enrichment (HKLM + HKCU) - Add UninstallLocalAsync method that executes registry uninstall commands - Add ParseUninstallCommand parser handling quoted paths, MsiExec, rundll32 - ViewModel routes local apps (empty Source) to registry uninstall path - Update tooltip for Local badge in UninstallerView - Add 8 unit tests for ParseUninstallCommand covering all code paths - Update README Uninstaller section with local app support - Update CHANGELOG with v0.44.0 entry Closes #236 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
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
Adds the ability to uninstall local applications (apps not managed by winget) directly from the Uninstaller tab using their registry UninstallString.
What changed
How it works
Testing
Closes #236