ci: add dotnet format check to CI and fix existing formatting violations (INFRA-002)#451
Merged
Conversation
📝 WalkthroughWalkthroughAdds a CI "Check formatting" step running ChangesCode Formatting and Quality Enforcement
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs (1)
108-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider unsubscribing PropertyChanged handlers before removing items.
For consistency with the new unsubscription pattern introduced at lines 51-52, the
DeleteSelectedmethod should unsubscribeOnShortcutPropertyChangedhandlers before removing shortcuts from the collection. This prevents potential memory leaks if removedBrokenShortcutobjects remain referenced elsewhere (e.g., in logging, caching, or error handling paths).🛡️ Proposed fix to add handler cleanup
// Remove deleted items from the list foreach (var s in selected.Where(s => !System.IO.File.Exists(s.ShortcutPath))) { + s.PropertyChanged -= OnShortcutPropertyChanged; BrokenShortcuts.Remove(s); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs` around lines 108 - 111, In DeleteSelected, before removing each BrokenShortcut from the BrokenShortcuts collection, unsubscribe its PropertyChanged handler (the OnShortcutPropertyChanged method) the same way done at lines 51-52; iterate the selected items that don't exist, call shortcut.PropertyChanged -= OnShortcutPropertyChanged (or equivalent) for each item, then remove it from BrokenShortcuts to avoid leaving dead event subscriptions on removed BrokenShortcut instances.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
45-46: 💤 Low valueConsider including test projects in formatting verification.
The format check only targets
SysManager.csproj, while two test projects are also built in this workflow. If formatting rules should apply to the entire codebase, consider adding format checks for the test projects as well.💡 Proposed extension
- name: Check formatting - run: dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes --verbosity diagnostic + run: | + dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes --verbosity diagnostic + dotnet format SysManager/SysManager.Tests/SysManager.Tests.csproj --verify-no-changes --verbosity diagnostic + dotnet format SysManager/SysManager.UITests/SysManager.UITests.csproj --verify-no-changes --verbosity diagnostic🤖 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 @.github/workflows/ci.yml around lines 45 - 46, The "Check formatting" step currently only runs dotnet format against SysManager/SysManager/SysManager.csproj; expand it to cover the test projects as well by either running dotnet format against the solution file or adding additional --verify-no-changes invocations for each test project (the same command pattern used for SysManager.csproj). Update the step that runs "dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes --verbosity diagnostic" to include the test project paths or the .sln so formatting verification applies to the entire codebase.
🤖 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.
Outside diff comments:
In `@SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs`:
- Around line 108-111: In DeleteSelected, before removing each BrokenShortcut
from the BrokenShortcuts collection, unsubscribe its PropertyChanged handler
(the OnShortcutPropertyChanged method) the same way done at lines 51-52; iterate
the selected items that don't exist, call shortcut.PropertyChanged -=
OnShortcutPropertyChanged (or equivalent) for each item, then remove it from
BrokenShortcuts to avoid leaving dead event subscriptions on removed
BrokenShortcut instances.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 45-46: The "Check formatting" step currently only runs dotnet
format against SysManager/SysManager/SysManager.csproj; expand it to cover the
test projects as well by either running dotnet format against the solution file
or adding additional --verify-no-changes invocations for each test project (the
same command pattern used for SysManager.csproj). Update the step that runs
"dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes
--verbosity diagnostic" to include the test project paths or the .sln so
formatting verification applies to the entire codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6e032cd-d434-429d-926f-5ba4d7aaa0a5
📥 Commits
Reviewing files that changed from the base of the PR and between 9203960 and b425e09d9c66d13901938c00b192cb2d314cd905.
📒 Files selected for processing (19)
.github/workflows/ci.ymlSysManager/SysManager/AssemblyInfo.csSysManager/SysManager/Models/FriendlyEventEntry.csSysManager/SysManager/Services/DeepCleanupService.csSysManager/SysManager/Services/DiskHealthService.csSysManager/SysManager/Services/EventExplainer.csSysManager/SysManager/Services/FixedDriveService.csSysManager/SysManager/Services/IconExtractorService.csSysManager/SysManager/Services/LargeFileScanner.csSysManager/SysManager/Services/SystemInfoService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/ViewModels/BatteryHealthViewModel.csSysManager/SysManager/ViewModels/DeepCleanupViewModel.csSysManager/SysManager/ViewModels/LogsViewModel.csSysManager/SysManager/ViewModels/MainWindowViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/ShortcutCleanerViewModel.csSysManager/SysManager/ViewModels/SystemHealthViewModel.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 (18)
SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs (1)
49-53: PR description inconsistency: This is a behavioral change, not just formatting.The PR objectives state "No changes to program logic; edits are limited to whitespace and reordering of using statements," but lines 49-52 add explicit event handler unsubscription logic (referenced as MEM-007) to prevent memory leaks. This is a runtime behavioral enhancement, not a formatting fix.
While the change itself is correct and valuable for preventing memory leaks during rescans, the PR description should accurately reflect that it includes a memory management improvement alongside the formatting changes.
SysManager/SysManager/Services/DeepCleanupService.cs (1)
45-50: LGTM!SysManager/SysManager/Services/DiskHealthService.cs (1)
187-215: LGTM!SysManager/SysManager/Services/EventExplainer.cs (1)
168-183: LGTM!SysManager/SysManager/Services/FixedDriveService.cs (1)
117-134: LGTM!SysManager/SysManager/Services/IconExtractorService.cs (1)
34-34: LGTM!SysManager/SysManager/Services/LargeFileScanner.cs (1)
68-68: LGTM!Also applies to: 85-107
SysManager/SysManager/Services/SystemInfoService.cs (1)
195-219: LGTM!Also applies to: 250-264
SysManager/SysManager/Services/UpdateService.cs (1)
24-24: LGTM!Also applies to: 370-377, 382-384
SysManager/SysManager/AssemblyInfo.cs (1)
17-17: LGTM!SysManager/SysManager/Models/FriendlyEventEntry.cs (1)
38-56: LGTM!SysManager/SysManager/ViewModels/BatteryHealthViewModel.cs (1)
5-10: LGTM!SysManager/SysManager/ViewModels/DeepCleanupViewModel.cs (1)
82-90: LGTM!SysManager/SysManager/ViewModels/LogsViewModel.cs (1)
85-106: LGTM!Also applies to: 319-328
SysManager/SysManager/ViewModels/MainWindowViewModel.cs (1)
250-258: LGTM!Also applies to: 261-278, 281-295, 298-314, 317-331, 334-345, 348-363, 366-379, 382-397, 400-413, 416-429, 432-445
SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
95-98: LGTM!SysManager/SysManager/ViewModels/SystemHealthViewModel.cs (1)
7-7: LGTM!SysManager/SysManager/ViewModels/UninstallerViewModel.cs (1)
209-218: LGTM!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
laurentiu021
force-pushed
the
ci/dotnet-format-check
branch
from
May 20, 2026 07:57
b425e09 to
109028b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
45-47: ⚡ Quick winConsider including test projects in the formatting check.
The formatting check currently only runs against
SysManager.csproj, but the restore step also pullsSysManager.Tests.csprojandSysManager.UITests.csproj. Including test projects would ensure consistent formatting standards across the entire codebase, since the.editorconfigat the repository root applies to all C# files.Update the step to check all projects restored in the build-and-test job:
📋 Proposed enhancement
- name: Check formatting - run: dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes --verbosity diagnostic + run: | + dotnet format SysManager/SysManager/SysManager.csproj --verify-no-changes --verbosity diagnostic + dotnet format SysManager/SysManager.Tests/SysManager.Tests.csproj --verify-no-changes --verbosity diagnostic + dotnet format SysManager/SysManager.UITests/SysManager.UITests.csproj --verify-no-changes --verbosity diagnostic🤖 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 @.github/workflows/ci.yml around lines 45 - 47, The "Check formatting" step currently runs dotnet format only against SysManager/SysManager/SysManager.csproj; update that step (the "Check formatting" run command) to include the test projects or the solution so all restored projects are checked — e.g. run dotnet format against SysManager.sln or include SysManager.Tests.csproj and SysManager.UITests.csproj alongside SysManager.csproj — ensuring the repository-level .editorconfig is applied to production and test code.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 45-47: The "Check formatting" step currently runs dotnet format
only against SysManager/SysManager/SysManager.csproj; update that step (the
"Check formatting" run command) to include the test projects or the solution so
all restored projects are checked — e.g. run dotnet format against
SysManager.sln or include SysManager.Tests.csproj and SysManager.UITests.csproj
alongside SysManager.csproj — ensuring the repository-level .editorconfig is
applied to production and test code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7d25b8d-b68a-44bb-af10-5027331a6d53
📥 Commits
Reviewing files that changed from the base of the PR and between b425e09d9c66d13901938c00b192cb2d314cd905 and 109028b.
📒 Files selected for processing (30)
.github/workflows/ci.ymlSysManager/SysManager/AssemblyInfo.csSysManager/SysManager/Models/FriendlyEventEntry.csSysManager/SysManager/Services/DeepCleanupService.csSysManager/SysManager/Services/DiskHealthService.csSysManager/SysManager/Services/EventExplainer.csSysManager/SysManager/Services/FixedDriveService.csSysManager/SysManager/Services/IconExtractorService.csSysManager/SysManager/Services/LargeFileScanner.csSysManager/SysManager/Services/SystemInfoService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/ViewModels/BatteryHealthViewModel.csSysManager/SysManager/ViewModels/DeepCleanupViewModel.csSysManager/SysManager/ViewModels/LogsViewModel.csSysManager/SysManager/ViewModels/MainWindowViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/ShortcutCleanerViewModel.csSysManager/SysManager/ViewModels/SystemHealthViewModel.csSysManager/SysManager/ViewModels/UninstallerViewModel.csSysManager/SysManager/Views/AppUpdatesView.xaml.csSysManager/SysManager/Views/CleanupView.xaml.csSysManager/SysManager/Views/DashboardView.xaml.csSysManager/SysManager/Views/DriversView.xaml.csSysManager/SysManager/Views/LogsView.xaml.csSysManager/SysManager/Views/NetworkRepairView.xaml.csSysManager/SysManager/Views/PingView.xaml.csSysManager/SysManager/Views/SpeedTestView.xaml.csSysManager/SysManager/Views/SystemHealthView.xaml.csSysManager/SysManager/Views/TracerouteView.xaml.csSysManager/SysManager/Views/WindowsUpdateView.xaml.cs
✅ Files skipped from review due to trivial changes (27)
- SysManager/SysManager/Services/IconExtractorService.cs
- SysManager/SysManager/Services/DiskHealthService.cs
- SysManager/SysManager/Views/DriversView.xaml.cs
- SysManager/SysManager/ViewModels/SystemHealthViewModel.cs
- SysManager/SysManager/Views/SpeedTestView.xaml.cs
- SysManager/SysManager/Views/AppUpdatesView.xaml.cs
- SysManager/SysManager/Views/SystemHealthView.xaml.cs
- SysManager/SysManager/Services/SystemInfoService.cs
- SysManager/SysManager/Views/LogsView.xaml.cs
- SysManager/SysManager/Views/DashboardView.xaml.cs
- SysManager/SysManager/Services/DeepCleanupService.cs
- SysManager/SysManager/Services/FixedDriveService.cs
- SysManager/SysManager/Views/WindowsUpdateView.xaml.cs
- SysManager/SysManager/Services/UpdateService.cs
- SysManager/SysManager/Models/FriendlyEventEntry.cs
- SysManager/SysManager/Views/PingView.xaml.cs
- SysManager/SysManager/ViewModels/UninstallerViewModel.cs
- SysManager/SysManager/Services/EventExplainer.cs
- SysManager/SysManager/ViewModels/ShortcutCleanerViewModel.cs
- SysManager/SysManager/AssemblyInfo.cs
- SysManager/SysManager/Views/NetworkRepairView.xaml.cs
- SysManager/SysManager/Views/TracerouteView.xaml.cs
- SysManager/SysManager/ViewModels/DeepCleanupViewModel.cs
- SysManager/SysManager/ViewModels/LogsViewModel.cs
- SysManager/SysManager/ViewModels/MainWindowViewModel.cs
- SysManager/SysManager/Views/CleanupView.xaml.cs
- SysManager/SysManager/ViewModels/NetworkSharedState.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 (2)
SysManager/SysManager/Services/LargeFileScanner.cs (1)
68-68: LGTM!Also applies to: 87-89, 103-105
SysManager/SysManager/ViewModels/BatteryHealthViewModel.cs (1)
5-5: LGTM!
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…ons (INFRA-002) (#451) * ci: add dotnet format check to CI and fix existing formatting violations (INFRA-002) * ci: fix whitespace formatting violations in View code-behind files --------- 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.
Adds a dotnet format --verify-no-changes step to the CI pipeline (after Restore, before Build). This ensures all future PRs conform to the .editorconfig formatting rules.
Also fixes 18 existing files that had minor formatting violations (whitespace, import ordering) so the new check passes on the current codebase.
No logic changes — purely whitespace and using-statement reordering.
Summary by CodeRabbit
Chores
Bug Fixes