fix: ghost checkboxes, DNS elevation banner, startup column width, shredder empty state#502
Conversation
📝 WalkthroughWalkthroughAdds a RelaunchElevated RelayCommand and an elevation banner UI to the DNS hosts view; standardizes multiple DataGrid properties across several views (column resizing, header visibility styling, row addition, row backgrounds, and a template column width). ChangesDNS Admin Elevation UI and Relaunch
DataGrid UI Refinements Across Views
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…redder empty state
laurentiu021
force-pushed
the
fix/ui-round2
branch
from
bfa03bf to
e8fb979
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/ViewModels/DnsHostsViewModel.cs`:
- Around line 239-244: Wrap the RelaunchElevated method body in a try-catch that
calls AdminHelper.RelaunchAsAdmin() and only calls
System.Windows.Application.Current?.Shutdown() when the relaunch returns true,
and on any exception catch update the ViewModel's StatusMessage or HostsStatus
with a user-friendly error (and optionally log the exception) instead of letting
the exception propagate; reference the RelaunchElevated method,
AdminHelper.RelaunchAsAdmin(), System.Windows.Application.Current?.Shutdown(),
and the StatusMessage/HostsStatus properties when making the change.
- Around line 239-244: RelaunchElevated currently calls
AdminHelper.RelaunchAsAdmin() and immediately shuts down, risking loss of
unsaved HostEntries; update RelaunchElevated to first detect unsaved changes
(e.g. compare current HostEntries to last saved state or use an existing/added
"IsDirty" flag set by AddEntryCommand/RemoveEntryCommand), then either auto-save
by invoking the existing Save command/method or show a confirmation dialog
prompting the user to Save/Discard/Cancel; only call
AdminHelper.RelaunchAsAdmin() and Application.Current?.Shutdown() after the user
has saved or explicitly chosen to discard changes.
🪄 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: d765dcdd-0aa8-4866-bbe7-ec566999ad76
📒 Files selected for processing (6)
SysManager/SysManager/ViewModels/DnsHostsViewModel.csSysManager/SysManager/Views/DnsHostsView.xamlSysManager/SysManager/Views/FileShredderView.xamlSysManager/SysManager/Views/StartupView.xamlSysManager/SysManager/Views/UninstallerView.xamlSysManager/SysManager/Views/WindowsUpdateView.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: UI automation tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
SysManager/SysManager/Views/DnsHostsView.xaml (1)
13-18: LGTM!Also applies to: 27-41, 44-44, 79-79
SysManager/SysManager/ViewModels/DnsHostsViewModel.cs (1)
239-244: ConfirmAdminHelper.RelaunchAsAdmin()launch + return semantics used byDnsHostsViewModel.RelaunchElevated.
AdminHelper.RelaunchAsAdmin()requests UAC elevation by starting the current executable withProcessStartInfo.UseShellExecute = trueandVerb = "runas"(passingargumentHintviaArguments). It returnstruewhenProcess.Start(psi)is successfully called, andfalsewhenexePathis missing or when UAC/process start fails (caughtInvalidOperationException/Win32Exception). This matches theif (AdminHelper.RelaunchAsAdmin()) ... Shutdown();conditional (shutdown only after the elevated launch request is issued).SysManager/SysManager/Views/FileShredderView.xaml (1)
69-78: LGTM!SysManager/SysManager/Views/UninstallerView.xaml (1)
65-69: LGTM!SysManager/SysManager/Views/WindowsUpdateView.xaml (1)
129-129: LGTM!SysManager/SysManager/Views/StartupView.xaml (2)
102-102: AI summary inconsistency: This is the last column, not the first.The AI summary states "the first (icon/selection) DataGridTemplateColumn header column is resized" but the actual change is to the last column (line 102) containing the "Open" button. The first column at line 53 with the checkbox remains Width="40".
The code change itself is correct and matches the PR description.
102-102: LGTM!
| [RelayCommand] | ||
| private void RelaunchElevated() | ||
| { | ||
| if (AdminHelper.RelaunchAsAdmin()) | ||
| System.Windows.Application.Current?.Shutdown(); | ||
| } |
There was a problem hiding this comment.
Add error handling for the relaunch flow.
If AdminHelper.RelaunchAsAdmin() throws an exception (e.g., file not found, access denied), or if Application.Current?.Shutdown() fails, the exception will propagate uncaught. This could leave the app in an inconsistent state or crash without user feedback.
Wrap the calls in a try-catch block and update StatusMessage or HostsStatus on failure.
🛡️ Proposed error handling
[RelayCommand]
private void RelaunchElevated()
{
+ try
+ {
if (AdminHelper.RelaunchAsAdmin())
System.Windows.Application.Current?.Shutdown();
+ else
+ HostsStatus = "Failed to relaunch as administrator.";
+ }
+ catch (Exception ex)
+ {
+ HostsStatus = $"Error relaunching: {ex.Message}";
+ Log.Warning(ex, "Failed to relaunch elevated");
+ }
}🤖 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/DnsHostsViewModel.cs` around lines 239 -
244, Wrap the RelaunchElevated method body in a try-catch that calls
AdminHelper.RelaunchAsAdmin() and only calls
System.Windows.Application.Current?.Shutdown() when the relaunch returns true,
and on any exception catch update the ViewModel's StatusMessage or HostsStatus
with a user-friendly error (and optionally log the exception) instead of letting
the exception propagate; reference the RelaunchElevated method,
AdminHelper.RelaunchAsAdmin(), System.Windows.Application.Current?.Shutdown(),
and the StatusMessage/HostsStatus properties when making the change.
Prevent data loss from unsaved hosts file changes.
The RelaunchElevated method immediately shuts down the application without checking whether the user has unsaved modifications to HostEntries. If a user adds or removes entries (via AddEntryCommand/RemoveEntryCommand) but hasn't clicked "Save" before clicking "Run as administrator," those changes will be lost.
Consider checking for unsaved changes and either:
- Auto-save before relaunching, or
- Prompt the user to save, or
- Block relaunch and display a message directing the user to save first.
🛡️ Suggested approach to detect unsaved changes
One approach is to track a "dirty" flag:
+[ObservableProperty] private bool _hasUnsavedHostChanges;
[RelayCommand]
private void AddEntry()
{
try
{
var entry = _hostsService.AddEntry(NewIp, NewHostname);
HostEntries.Add(entry);
+ HasUnsavedHostChanges = true;
NewIp = "";
NewHostname = "";
HostsStatus = $"Added {entry.Hostname} ({entry.IpAddress}).";
}
catch (ArgumentException ex)
{
HostsStatus = ex.Message;
}
}
[RelayCommand]
private void RemoveEntry(HostsEntry? entry)
{
if (entry is null) return;
HostEntries.Remove(entry);
+ HasUnsavedHostChanges = true;
HostsStatus = $"Removed {entry.Hostname}.";
}
[RelayCommand]
private void SaveHosts()
{
if (!IsElevated)
{
HostsStatus = "Saving hosts file requires administrator privileges.";
return;
}
try
{
_hostsService.SaveHosts(HostEntries.ToList());
+ HasUnsavedHostChanges = false;
HostsStatus = $"Saved {HostEntries.Count} entries. Backup created at hosts.bak.";
Log.Information("Hosts file saved with {Count} entries", HostEntries.Count);
}
catch (UnauthorizedAccessException)
{
HostsStatus = "Access denied — run as administrator to save hosts file.";
}
catch (IOException ex)
{
HostsStatus = $"Error saving hosts file: {ex.Message}";
Log.Error(ex, "Failed to save hosts file");
}
}
[RelayCommand]
private void RelaunchElevated()
{
+ if (HasUnsavedHostChanges)
+ {
+ HostsStatus = "Please save your hosts file changes before relaunching.";
+ return;
+ }
if (AdminHelper.RelaunchAsAdmin())
System.Windows.Application.Current?.Shutdown();
}🤖 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/DnsHostsViewModel.cs` around lines 239 -
244, RelaunchElevated currently calls AdminHelper.RelaunchAsAdmin() and
immediately shuts down, risking loss of unsaved HostEntries; update
RelaunchElevated to first detect unsaved changes (e.g. compare current
HostEntries to last saved state or use an existing/added "IsDirty" flag set by
AddEntryCommand/RemoveEntryCommand), then either auto-save by invoking the
existing Save command/method or show a confirmation dialog prompting the user to
Save/Discard/Cancel; only call AdminHelper.RelaunchAsAdmin() and
Application.Current?.Shutdown() after the user has saved or explicitly chosen to
discard changes.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
SysManager/SysManager/ViewModels/DnsHostsViewModel.cs (1)
239-244:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle relaunch failures and exceptions in the command.
Line 242 currently only handles the success path; if relaunch fails or throws, users get no feedback and the command can fail noisily. Wrap the call in
try/catchand setHostsStatuson failure paths.Proposed patch
[RelayCommand] private void RelaunchElevated() { - if (AdminHelper.RelaunchAsAdmin()) - System.Windows.Application.Current?.Shutdown(); + try + { + if (AdminHelper.RelaunchAsAdmin()) + { + System.Windows.Application.Current?.Shutdown(); + } + else + { + HostsStatus = "Failed to relaunch as administrator."; + } + } + catch (Exception ex) + { + HostsStatus = $"Error relaunching: {ex.Message}"; + Log.Warning(ex, "Failed to relaunch elevated"); + } }🤖 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/DnsHostsViewModel.cs` around lines 239 - 244, The RelaunchElevated command currently only handles the success path; update the RelaunchElevated method to wrap the call to AdminHelper.RelaunchAsAdmin() in a try/catch, set the HostsStatus property to an appropriate failure message when RelaunchAsAdmin returns false, and in the catch block set HostsStatus to the caught exception message (and optionally log it) so the UI shows feedback on failures or exceptions instead of failing silently.
🤖 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.
Duplicate comments:
In `@SysManager/SysManager/ViewModels/DnsHostsViewModel.cs`:
- Around line 239-244: The RelaunchElevated command currently only handles the
success path; update the RelaunchElevated method to wrap the call to
AdminHelper.RelaunchAsAdmin() in a try/catch, set the HostsStatus property to an
appropriate failure message when RelaunchAsAdmin returns false, and in the catch
block set HostsStatus to the caught exception message (and optionally log it) so
the UI shows feedback on failures or exceptions instead of failing silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2135e68-4590-4e51-9413-b080736caefe
📒 Files selected for processing (12)
SysManager/SysManager/ViewModels/DnsHostsViewModel.csSysManager/SysManager/Views/AppUpdatesView.xamlSysManager/SysManager/Views/ContextMenuView.xamlSysManager/SysManager/Views/DnsHostsView.xamlSysManager/SysManager/Views/DriversView.xamlSysManager/SysManager/Views/FileShredderView.xamlSysManager/SysManager/Views/ServicesView.xamlSysManager/SysManager/Views/ShortcutCleanerView.xamlSysManager/SysManager/Views/StartupView.xamlSysManager/SysManager/Views/UninstallerView.xamlSysManager/SysManager/Views/WindowsFeaturesView.xamlSysManager/SysManager/Views/WindowsUpdateView.xaml
✅ Files skipped from review due to trivial changes (3)
- SysManager/SysManager/Views/ContextMenuView.xaml
- SysManager/SysManager/Views/ShortcutCleanerView.xaml
- SysManager/SysManager/Views/ServicesView.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 (8)
SysManager/SysManager/Views/DnsHostsView.xaml (1)
14-17: LGTM!Also applies to: 27-42, 44-44, 79-79
SysManager/SysManager/Views/AppUpdatesView.xaml (1)
66-74: LGTM!SysManager/SysManager/Views/DriversView.xaml (1)
46-55: LGTM!SysManager/SysManager/Views/FileShredderView.xaml (1)
65-79: LGTM!SysManager/SysManager/Views/StartupView.xaml (1)
49-50: LGTM!Also applies to: 103-103
SysManager/SysManager/Views/UninstallerView.xaml (1)
65-70: LGTM!SysManager/SysManager/Views/WindowsFeaturesView.xaml (1)
85-87: LGTM!SysManager/SysManager/Views/WindowsUpdateView.xaml (1)
128-131: LGTM!
1 participant
Fixes
CanUserAddRows="False"to DataGrids (WPF default adds empty "new item" row)Test plan
Summary by CodeRabbit
New Features
UI/UX Improvements