chore(sonar): hoist constant array arguments to static readonly fields (CA1861) - #387
Merged
Merged
Conversation
…s (CA1861) Drives external_roslyn:CA1861 "Avoid constant arrays as arguments" to zero across the test suite (53 SonarCloud-reported instances). Each constant array literal passed directly as an argument is hoisted into a descriptively-named `private static readonly T[]` field so it is allocated once instead of on every call. Identical literals are deduped to a single field. Behavior-preserving: same values, order, and element types; full suite green (5284 tests). 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 #387 orbit-api
Recommendation: APPROVE
Summary
Pure mechanical refactor across 15 xUnit test files (all under tests/, zero src/ changes): every constant array literal (new[] { ... }, new int[] { ... }) passed directly as a method/constructor argument is hoisted into a private static readonly T[] field, deduplicating identical literals within each class. This targets SonarCloud CA1861. No behavior, assertion, or production code changes.
Findings
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 (not posted per signal gate) |
Verification performed
- Confirmed all 15 changed files are under
tests/only (gh pr view --json files) — nosrc/Orbit.*production code touched. - Checked for shared-static-field mutation risk (the one correctness hazard this refactor pattern could introduce):
User.SetSelectedCalendars(src/Orbit.Domain/Entities/User.cs:163) copies its input via.ToList()before storing, so the hoistedSingleCalendarSelection/TwoCalendarSelection/SharedCalendarSelectionfields reused acrossSetSelectedCalendarsCommandHandlerTests.csandGetUserCalendarsQueryHandlerTests.cscan't leak mutations between tests.Habit.ReminderTimesstores the raw reference (Habit.cs:152,380) but exposes it only asIReadOnlyList<int>, so the reusedReminderTimesfield inReminderSchedulerServiceTests.cs(7 call sites) can't be mutated through that surface either. - Grepped all 15 files for reference-equality assertions (
BeSameAs/ReferenceEquals) that a shared-field reference could break — none found. - Grepped all 15 files for any remaining constant-array-literal arguments not hoisted: found exactly two (
AiHabitSuggestionServiceTests.cs:205,329), both containingnew string('a', 201/501)— a runtime, non-constant expression — so CA1861 legitimately doesn't apply and they were correctly left alone. This confirms the sweep is complete, not partial. - Spot-checked dedup naming: identical literals reused within a class (e.g.
MondayName,GamificationTodaySurfaces) always map to conceptually-identical call sites; no case of two different concepts being incorrectly collapsed onto one field. - No narration comments, no
null!, noconsole.log/Debug.WriteLine, no dead code introduced by the diff.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | N/A — gate ("any src/ code changed") not met; diff is tests/ only |
| contract-aligner | N/A — no DTO, Controller route, or shared-type change |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped per instructions; covered by separate CI required check |
| Tests (dotnet) | N/A — skipped per instructions; covered by separate CI required check; PR body reports 5284 passed / 0 failed locally |
Deferred — N/A dimensions
- DESIGN.md/AI-slop, Parity, i18n — N/A, no
apps/*files in this diff (frontend-only dimensions, orbit-ui-mobile not checked out). - Contract drift + backward-compat — N/A, no DTO/Zod/endpoint surface touched.
- Backend hard rules — N/A, diff touches only test files; no timezone/authz/logging/rollback logic changed.
- FEATURES.md parity — N/A, no user-facing behavior change (pure test-code refactor).
What's good
- Genuinely mechanical: values, order, and element types are byte-for-byte preserved at every call site.
- Correctly stopped at the CA1861 boundary — didn't hoist the two arrays containing a runtime-computed element, which would have been a no-op/incorrect fix.
- Sensible dedup of identical literals within a class rather than over-hoisting to shared/static cross-class constants.
Recommendation
Approve as-is. No changes requested.
thomasluizon
added a commit
that referenced
this pull request
Jul 14, 2026
…#402) Clears the last CA1861 code smell (external_roslyn:CA1861, introduced by #398's coverage tests): hoists the constant `{ "Monday", "Tuesday" }` array literal passed as the reschedule payload's `days` argument into a descriptively-named `private static readonly string[]` field, matching the #387/#397 pattern. Behavior-preserving: same values, order, and element type; full suite green. Refs thomasluizon/orbit-ui-mobile#243 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.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
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.



What
Drives Roslyn/SonarCloud rule
external_roslyn:CA1861— "Avoid constant arrays as arguments" to zero across the orbit-api test suite. SonarCloud reported 53 instances, all in xUnit test files.Each constant array literal passed directly as a method/constructor argument (e.g.
new[] { "Health", "Fitness" },new int[] { 0 },new[] { DayOfWeek.Monday }) is hoisted into a descriptively-namedprivate static readonly T[]field declared on the class, so the array is allocated once rather than on every call. Identical literals are deduped to a single shared field. A handful of additional in-file constant-array arguments beyond the 53 reported lines were hoisted in the same pass (same rule) — no new smells introduced.Rules + counts
external_roslyn:CA1861Behavior preservation
dotnet build— 0 errors (7 pre-existing NU1608 NuGet-version warnings only).dotnet test— 5284 passed, 0 failed (Analyzers 7 / Domain 512 / Application 2773 / Infrastructure 1992).#pragma/NOSONARsuppression.Refs thomasluizon/orbit-ui-mobile#243