don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-04-25-NEO-29.md

3.7 KiB
Raw Blame History

Code review — NEO-29

  • Date: 2026-04-25
  • Scope: Branch NEO-29-hotbarloadout-v1-contract-baseline-persistence-path (origin/main...HEAD)
  • Base: origin/main

Verdict

Approve with nits.

Summary

This branch delivers the planned HotbarLoadout v1 vertical slice end-to-end: server GET/POST APIs with stable deny reason codes, persistence selection (Postgres when configured, in-memory fallback otherwise), client hydration wiring, and targeted tests/manual QA artifacts. The API behavior and persistence wiring align with the NEO-29 plan decisions (per-player scope, fixed eight slots, server-owned validation). Contract safety is good for a prototype slice: deny responses preserve authoritative loadout snapshots, and request validation covers out-of-bounds, unknown abilities, and duplicate slots. Overall runtime risk is low; most residual risk is documentation-state drift rather than code behavior.

Documentation checked

  • docs/plans/NEO-29-implementation-plan.md — matches (scope, API shape, persistence policy, deny reasons, tests/docs artifacts all present).
  • docs/plans/E1M4-prototype-backlog.md — matches (E1M4-01 slice framing and NEO-29 implementation note are consistent).
  • docs/decomposition/modules/E1_M4_AbilityInputScaffold.md — partially matches (implementation snapshot updated, but summary Status still Planned despite landed implementation).
  • docs/decomposition/modules/module_dependency_register.md — partially matches (E1.M4 row remains Planned; this story appears to satisfy “work started,” which should move to In Progress per alignment policy).
  • docs/decomposition/modules/documentation_and_implementation_alignment.md — conflicts with current status labels (rule says move module status from Planned to In Progress when implementation stories merge).
  • docs/decomposition/modules/contracts.md — matches for prototype scope (JSON/HTTP spike is acceptable for early slice; versioned envelope and stable reason codes are documented).
  • docs/decomposition/modules/client_server_authority.md — matches (server remains authority for persistent gameplay-relevant loadout state; client acts as view/sync).

Register/tracking follow-up needed after merge: update E1.M4 status in module_dependency_register.md, E1_M4_AbilityInputScaffold.md summary table, and the implementation tracking table entry expectations.

Blocking issues

None.

Suggestions

  1. Update decomposition status tracking for E1.M4 from Planned to In Progress in the register/module summary (and corresponding tracking inventory) so docs comply with documentation_and_implementation_alignment.md and do not under-report shipped progress. Done. Updated docs/decomposition/modules/module_dependency_register.md (row + note), docs/decomposition/modules/E1_M4_AbilityInputScaffold.md (summary status), and docs/decomposition/modules/documentation_and_implementation_alignment.md (new E1.M4 tracking row).

Nits

  • Nit: Consider adding one API test for slotIndex < 0 deny parity with the existing slotIndex == 8 test, to lock the lower-bound contract explicitly. Done. Added PostHotbarLoadout_ShouldDenyNegativeSlot_WithReasonCode in server/NeonSprawl.Server.Tests/Game/AbilityInput/HotbarLoadoutApiTests.cs.

Verification

  • dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutApiTests"
  • dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutPersistenceIntegrationTests"
  • godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_loadout_client_test.gd
  • Manual checklist: docs/manual-qa/NEO-29.md