neon-sprawl/docs/reviews/2026-04-30-NEO-32.md

Code review — NEO-32 (CooldownSnapshot sync + slot presentation)

Date: 2026-04-30

Scope: Branch NEO-32-cooldown-snapshot-sync-slot-presentation vs merge-base with origin/main (commits through 56f7ae8: cooldown snapshot API, cast on_cooldown, client HUD/sync, optional NEON_SPRAWL_API_LOG, HUD anchor fix for bad serverTimeUtc).

Base: origin/main at merge-base 0790a3398d1b7138643e8e91f4b28db4fd7c554e.

Verdict

Approve with nits

Summary

The branch delivers the planned NEO-32 slice: authoritative in-memory per-player slot cooldowns after an accepted cast, JSON deny on_cooldown, GET /game/players/{id}/cooldown-snapshot with v1 DTOs, FakeTimeProvider in the test host for deterministic time, Godot CooldownSnapshotClient + CooldownState with poll-while-cooling and a client-side cast guard aligned with the same reason string, plus Bruno and manual QA. Server tests pass (dotnet test). Overall risk is low for a prototype vertical slice; remaining notes are test-structure polish and a minor store hygiene detail.

Documentation checked

Document	Result
docs/plans/NEO-32-implementation-plan.md	Matches — dedicated GET, global duration, on_cooldown, in-memory store, client poll + guard, FakeTimeProvider, reconciliation section matches shipped filenames (cooldown_state_test.gd vs earlier table mentioning cooldown_snapshot_client_test.gd; reconciliation already reflects state tests).
docs/decomposition/modules/E1_M4_AbilityInputScaffold.md	Matches — current state paragraph describes route, DTOs, client scripts, HUD, and plan link.
docs/decomposition/modules/module_dependency_register.md	Matches — NEO-32 note alongside prior E1.M4 slices.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E1.M4 row includes NEO-32 pointers.
docs/decomposition/modules/client_server_authority.md	Matches — cooldown enforced on server; client guard is optional spam/UX layer, consistent with “server validates every intent.”
docs/decomposition/modules/E5_M1_CombatRulesEngine.md	N/A for conflict — still notes prototype accept does not imply full combat resolution; NEO-32 does not claim engine-level cooldown catalog.

Register / tracking: No change required beyond what the branch already updates; E1.M4 correctly stays In Progress.

Blocking issues

(none)

Suggestions

1. C# AAA layout on new integration tests — CooldownSnapshotApiTests.GetCooldownSnapshot_ShouldExposeSlotEnd_WhenCastAccepted_ThenClear_WhenAdvancedPastDuration and the two new cooldown tests in AbilityCastApiTests bundle HTTP calls and FakeClock.Advance inside a single // Act block. Per csharp-style, split so Act is only the invocations under assertion (or use clearly separated Act steps per phase) and keep clock advances in Arrange or interleaved with labeled sections. Low priority but keeps CI lint/review consistency for future edits.

2. Stale cooldown entries in the in-memory store — TryGetCooldownEndUtc does not prune expired ends; BuildSnapshot omits them from JSON, so wire behavior is correct. Expired rows linger until IsOnCooldown runs (e.g. next cast to that slot). For a long-lived prototype process this is negligible; if you ever extend GET-heavy paths, consider pruning in TryGetCooldownEndUtc or snapshot build for symmetry with IsOnCooldown. Done. TryGetCooldownEndUtc now takes DateTimeOffset now and shares pruning with IsOnCooldown; InMemoryPlayerAbilityCooldownStoreTests covers prune-on-read.

Nits

• Nit: Untracked Godot .uid files for new scripts appeared in the working tree (cooldown_snapshot_client.gd.uid, etc.). Decide whether to commit them (team Godot 4 convention) or ignore via .gitignore so CI/agents do not see noise.

Verification

• cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj — all tests passed (96) at review time.
• Manual: follow docs/manual-qa/NEO-32.md (HUD countdown, server deny, client guard, rehydrate); optional NEON_SPRAWL_API_LOG=1 / http+apiLog profile for stdout tracing.