Repository navigation
refactor: clear the open SonarQube code smells and enforce them locally - #102
Conversation
SonarQube's develop analysis carried 23 open issues: 22 VSTHRD202 (async/await that only forwards a Task) and one S3776 (ConnectionSupervisor.RunAsync at cognitive complexity 18 over the 15 limit). Neither failed the local build: VSTHRD202 ships at info severity and S3776 is not enabled by default in the SonarAnalyzer.CSharp package, so Sonar found what `dotnet build` let through. Raise both to warning in .editorconfig; with TreatWarningsAsErrors that makes them build errors, so the smells cannot come back unnoticed. Fixes: - Return the inner Task directly at the 22 VSTHRD202 sites (query forwarders on ConnectionSupervisor, GetCultureAsync helpers, the cancel buttons, the marker command handlers, EnsureChannelOrderAsync and two tests). MarkerReply.ForAsync now returns Task<string?> so the handlers can forward it under the ICommandHandler signature. - Extract the connect try/catch out of RunAsync into ConnectAsync, which maps a stopping unwind to null and a throwing source to Unreachable. Behaviour is unchanged; RunAsync drops to complexity 12. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active issues remain, as the change is a behavior-preserving mechanical refactor that clears all 23 outstanding SonarQube code smells without introducing functional concerns.
No open issues 🎉
What changed in this PR
This PR clears the 23 open SonarQube code smells on develop (22 × VSTHRD202 "async/await that only forwards a Task", 1 × S3776 cognitive complexity) and makes both rules build-breaking locally so they can't silently return. The changes are a behavior-preserving mechanical refactor across connection, relay, command, pairing, and workspace features, plus a cognitive-complexity reduction in the connection supervisor's run loop.
Changes:
- Convert redundant
async/awaitforwarders to return the innerTaskdirectly (usingTask.FromResult/Task.CompletedTaskfor early returns);MarkerReply.ForAsyncwidened toTask<string?>so the marker handlers can forward it under theICommandHandlersignature. - Extract the connect try/catch out of
ConnectionSupervisor.RunAsyncinto a newConnectAsynchelper (null ⇒ stopping,Unreachable⇒ throwing source), dropping cognitive complexity from 18 to 12 with loop behavior unchanged. - Raise
S3776andVSTHRD202towarningin.editorconfig, turning them into build errors underTreatWarningsAsErrors.
I verified the ConnectAsync extraction preserves the original control flow exactly (stopping unwind disposes and ends the loop without publishing status; a throwing source is logged and folded to Unreachable, with disposal/back-off handled by the existing loop branch), that outcome's nullable comparisons remain valid after the early null return, and that the MarkerReply.ForAsync nullable widening is safe for its only callers (the dispatcher tolerates a null reply). No functional or behavioral issues were found.
| File | Description |
|---|---|
.editorconfig |
Raises S3776 and VSTHRD202 to warning so they fail the build. |
src/.../Supervisor/ConnectionSupervisor.cs |
Query forwarders return Task directly; connect try/catch extracted into ConnectAsync to cut complexity. |
src/.../Gateway/DiscordWorkspaceGateway.cs |
EnsureChannelOrderAsync returns Task.CompletedTask/the reorder Task directly. |
src/.../Commands/Handlers/MarkerReply.cs |
ForAsync return type widened to Task<string?>. |
src/.../Handlers/{Cargo,Heli,Chinook}CommandHandler.cs |
Forward MarkerReply.ForAsync directly instead of awaiting. |
src/.../Switches/Relaying/SwitchStateRelay.cs |
GetCultureAsync returns the store Task directly. |
src/.../StorageMonitors/Relaying/StorageMonitorStateRelay.cs |
GetCultureAsync returns the store Task directly. |
src/.../Alarms/Relaying/AlarmRefresher.cs |
GetCultureAsync returns the store Task directly. |
src/.../Alarms/Pairing/AlarmPairingCoordinator.cs |
GetCultureAsync returns the store Task directly. |
src/.../Pairing/Modules/CredentialModule.cs |
DisconnectCancelAsync returns RespondAsync directly. |
src/.../Pairing/Hosting/PairingHostedService.cs |
StopAsync returns supervisor.StopAllAsync() directly after unsubscribing. |
src/.../Connections/Modules/ServerRemovalModule.cs |
RemoveCancelAsync returns RespondAsync directly. |
tests/.../Map.Tests/MapComposerTests.cs |
Helper returns the compose Task directly. |
tests/.../Chat.Tests/Hosting/ChatHostedServiceTests.cs |
StopAsync_without_a_start_completes returns the Task directly. |
Review effort: Balanced
Summary
SonarQube's develop analysis carried 23 open issues and the quality gate failed on them (coverage and duplication were fine):
ConnectionSupervisorquery forwarders,GetCultureAsynchelpers, cancel buttons, marker command handlers,EnsureChannelOrderAsync, two testsConnectionSupervisor.RunAsyncNeither failed the local build: VSTHRD202 ships at info severity and S3776 is not enabled by default in the SonarAnalyzer.CSharp package.
Changes
.editorconfig: both rules raised towarning. WithTreatWarningsAsErrorsthey are now build errors, so they cannot come back unnoticed.Taskdirectly.MarkerReply.ForAsyncnow returnsTask<string?>so the handlers can forward it under theICommandHandlersignature.RunAsync: the connect try/catch is extracted intoConnectAsync, which maps a stopping unwind tonulland a throwing source toUnreachable. Loop behaviour is unchanged; complexity drops to 12.Verification
dotnet formatand the ReSharper cleanup from the pre-push hook report no changes on the touched files.🤖 Generated with Claude Code