chore(sonar): fix CA1859 concrete types + CA1826 indexable collections - #388
Conversation
Burn down two Roslyn performance smells flagged by SonarCloud, all
behavior-preserving:
- CA1859 (29): change interface-typed private locals, fields, parameters,
and return types to the concrete type (List<T>, Dictionary<K,V>, int[],
MemoryCache, MemoryDistributedCache, SystemPromptBuilder) where no
polymorphism is needed. Only Sonar-flagged members changed; callers
already flow concrete types.
- CA1826 (11): replace .FirstOrDefault() on IReadOnlyList repository
results with a Count/indexer check (x.Count > 0 ? x[0] : null), and the
null-conditional Results?.FirstOrDefault() with a { Count: > 0 } pattern.
Refs thomasluizon/orbit-ui-mobile#243
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #388
Scope: chore(sonar): fix CA1859 concrete types + CA1826 indexable collections
Recommendation: APPROVE
Summary
Mechanical SonarCloud smell burn-down: 29 CA1859 interface→concrete-type retypes and 11 CA1826 FirstOrDefault()→indexer rewrites across 24 Orbit.Application/Orbit.Infrastructure files plus 8 matching test-file updates. No DTOs/Controllers/routes/auth/validation touched. Every retype was traced to its call site; every FirstOrDefault()→Count > 0 ? x[0] : null rewrite was verified against the source collection's actual return type (IGenericRepository<T>.FindAsync/FindTrackedIgnoringFiltersAsync return Task<IReadOnlyList<T>>, guaranteeing Count/indexer support) — semantically identical to FirstOrDefault().
A dedicated security review of the three highest-risk files independently confirmed:
AgentTargetOwnershipService.AllOwnedAsync—idsretypedIReadOnlyCollection<Guid>→List<Guid>; theownedCount == ids.Count/ids.Count == 0checks are untouched;CollectGuidsstill returnsList<Guid>. No authorization-bypass risk.ContentModerationService—payload?.Results is { Count: > 0 } results ? results[0] : nullis behaviorally identical to the priorFirstOrDefault(); fail-open-only-on-empty-results semantics preserved, no swallowing of a flagged result.GetPublicProfileQuery,GetFriendProfileQuery,FriendGraphServicelookup rewrites all filter on unique keys (Id,PublicProfileSlug, normalizedHandle,ReferralCode);matches[0]selection is equivalent to the oldFirstOrDefault(), no ordering-dependence introduced.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info]
src/Orbit.Infrastructure/Services/PushNotificationService.cs:72—subs as List<T> ?? subs.ToList()—subsis already declaredList<Domain.Entities.PushSubscription>, so the cast always succeeds and the?? subs.ToList()fallback is unreachable. Pre-existing (previouslysubs as IList<T>), not introduced by this diff. No action needed.
Deferred / N/A dimensions
- Build/test (dotnet): N/A — CI runs Build/Unit Tests/SonarCloud as separate required checks; PR body states 0 build errors, 5284/5284 tests passed.
- Contract-aligner / cross-repo parity: N/A — no DTO, Controller route, or
packages/sharedtype touched; siblingorbit-ui-mobilerepo not checked out regardless. - Backend hard rules (timezone/authz/validation/logging/transactions): gated in, confirmed untouched — diff surface is type retyping only.
What's good
- Every retype verified compile-safe by checking the caller already produces the narrower concrete type, rather than trusting the "0 build errors" claim blindly.
FirstOrDefault()→indexer rewrites backed by the repository interface contract (IReadOnlyList<T>), provably behavior-identical.- No narration comments,
TODO/FIXME/HACK, or dead code introduced. - PR description is precise about scope and states verified build/test results.
Recommendation
Approve as-is. Clean, well-scoped, behavior-preserving Sonar cleanup with no correctness, security, or contract regressions found.



What
SonarCloud smell burn-down (
thomasluizon_orbit-api), two Roslyn performance rules, all behavior-preserving:external_roslyn:CA1859List<T>,Dictionary<K,V>,int[],MemoryCache,MemoryDistributedCache,SystemPromptBuilder) where no polymorphism is needed.external_roslyn:CA1826.FirstOrDefault()onIReadOnlyList<T>repository results with aCount/indexer check (x.Count > 0 ? x[0] : null); the null-conditionalResults?.FirstOrDefault()becomes a{ Count: > 0 }pattern.Notes
CreateChallengeCommand.VerifyInvitedFriendsAsync,MoveHabitParentCommand.GetSubtreeHeight,ExportUserDataQueryfreshStreakValues), the un-flagged declarations were left as-is to keep the diff surgical.CA1859on a caller (all call sites usevaror pass the result directly).dotnet build— 0 errors (13 pre-existing warnings, unrelated: NU1608/RS1038/CS9057/CS0618).dotnet test— 5284 passed, 0 failed, 0 skipped.Refs thomasluizon/orbit-ui-mobile#243