58 lines
5.3 KiB
Markdown
58 lines
5.3 KiB
Markdown
# Code review — chore/cleanup analyzer pass
|
||
|
||
**Date:** 2026-06-27
|
||
**Scope:** Branch `chore/cleanup` — commit `b233fa0` vs `2cf8bcd` (merge base on `main`)
|
||
**Base:** `2cf8bcd` (merge of PR #190)
|
||
|
||
**Follow-up:** Suggestions and nits below are **done** (strikethrough + **Done.**).
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
This is a broad mechanical refactor across `NeonSprawl.Server` and `NeonSprawl.Server.Tests` (231 files, net −524 lines): collection expressions, primary-constructor catalog types, concrete private helper return types (CA1859), logging `IsEnabled` guards (CA1873), record XML doc fixes (RCS1263/RCS1139), `nameof` for SQL parameters, quest-store mutator delegate → `Func` + `MutateRowResult` record struct, removal of redundant `using` directives via new `GlobalUsings.cs`, and a substantial `csharp-style.md` appendix documenting the patterns. No HTTP routes, request bodies, or game contracts change. Build is clean (0 warnings). Full `dotnet test` passed **905/906** locally with one intermittent Postgres integration failure under parallel load; the same Postgres specs pass in isolation on this branch and on the parent commit.
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/` (NEO-* / E7M4 stories) | **N/A** — unticketed `chore:` branch; no feature acceptance criteria |
|
||
| `.cursor/rules/csharp-style.md` | **Matches** — new sections document patterns applied in this diff (collection expressions, CA1859/CA1873, RCS1015, record XML docs, test GlobalUsings) |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no module behavior or authority changes |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A** — no implementation-status update required |
|
||
| `bruno/neon-sprawl-server/collection.bru` | **Matches** — ~~Partially matches~~ docs block removed (**Done.**) |
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Remove the Bruno `docs { }` block** in `bruno/neon-sprawl-server/collection.bru`. The note (“chore/cleanup analyzer pass: no HTTP route or request-body changes”) belongs in the PR/commit message, not in the Bruno collection metadata consumers may read as API documentation.~~ **Done.** Removed the `docs { }` block from `collection.bru`.
|
||
|
||
2. ~~**Preserve explicit `StringComparer.Ordinal` in quest objective counter copies.** In `PostgresPlayerQuestStateStore.MutateObjectiveCounter`, the old code used `new Dictionary<string, int>(row.ObjectiveCounters, StringComparer.Ordinal)`. The new `ToDictionary` call relies on default string equality (ordinal in practice). For consistency with the rest of the quests module and defense-in-depth, pass `StringComparer.Ordinal` to `ToDictionary` or keep the dictionary copy constructor.~~ **Done.** `ToDictionary(..., StringComparer.Ordinal)` in `PostgresPlayerQuestStateStore.cs`.
|
||
|
||
3. ~~**Confirm Postgres integration stability under parallel xUnit.** Full-suite runs intermittently fail one of `FactionStandingPersistenceIntegrationTests`, `GigProgressionGrantPersistenceIntegrationTests`, or `SkillProgressionGrantPersistenceIntegrationTests` (expected persisted value vs `0`), while filtered runs pass on both `2cf8bcd` and `b233fa0`. This looks like pre-existing collection/fixture contention rather than a logic regression from this diff, but worth a quick check that `[Collection("Postgres integration")]` isolation is sufficient before merge if CI runs the full suite in parallel.~~ **Done.** Root cause for gig flake: `GigProgressionGrantPersistenceIntegrationTests` used wall-clock `Task.Delay` while cooldown uses `TimeProvider`; casts could return HTTP 200 with `on_cooldown` (`Accepted: false`) without defeat. Fixed by registering `FakeTimeProvider` on `PostgresWebApplicationFactory` and advancing the fake clock in the gig persistence test (same pattern as `InMemoryWebApplicationFactory`). Full suite **906/906** after fix.
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: `GlobalUsings.cs` pulls in domain namespaces (`NeonSprawl.Server.Game.Npc`, `NeonSprawl.Server.Tests.Game.Npc`, etc.). Works today (0 build warnings), but a one-line file comment listing intentional globals would help future contributors avoid accidental name clashes.~~ **Done.** File header comment added.
|
||
- ~~Nit: `QuestProgressIds.cs` has a whitespace-only trailing blank line before the closing brace — harmless; could drop in a follow-up.~~ **Done.**
|
||
- Nit: `QuestStateOperations.DenyAccept` no longer threads `playerId` / `questId`; telemetry comment correctly says “capture at call site when wiring E9.M1.” Fine for now — just remember at E9.M1.
|
||
- Nit: `NpcRuntimeOperations.AdvanceAll` drops the post–idle→aggro `TryGet` refresh; the while-loop `TryGet` on the first iteration makes this equivalent. No action needed.
|
||
- Nit: `PostgresPositionStateStore` drops unused `IOptions<GamePositionOptions>` / startup validation — correct dead-code removal; DI registration unchanged.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd server/NeonSprawl.Server && dotnet build
|
||
cd server/NeonSprawl.Server.Tests && dotnet test
|
||
cd server/NeonSprawl.Server.Tests && dotnet test --filter "FullyQualifiedName~PersistenceIntegrationTests"
|
||
```
|
||
|
||
Expect **906** passed; if a single Postgres persistence test fails under full parallel run, re-run filtered as above to distinguish flake from regression.
|
||
|
||
Manual Godot QA: none (no client or player-visible surface changes).
|