49 lines
3.9 KiB
Markdown
49 lines
3.9 KiB
Markdown
# Code review — NEO-23 (targeting API)
|
||
|
||
**Date:** 2026-04-17
|
||
|
||
**Scope:** Branch `NEO-23-e1m3-targetstate-selection-api`; `git` range `main...HEAD` (working tree clean at review time). Linear **NEO-23**.
|
||
|
||
**Base:** `main` (local `origin/main` implied).
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
The branch adds the NEO-23 targeting surface under `server/NeonSprawl.Server/Game/Targeting/`: prototype registry, in-memory per-player lock + sequence, horizontal reach–based validity (soft lock), `GET /game/players/{id}/target` and `POST …/target/select`, DTOs aligned with the plan (`selectionApplied`, optional `reasonCode`, always-present `lockedTargetId`), integration tests in the same spirit as interaction APIs, and server README coverage. Behavior matches the written implementation plan for denials (200 + `reasonCode` when not applied), 400/404 boundaries, clear via omitted/null `targetId`, and movement-driven `validity` without auto-clear. `dotnet test NeonSprawl.sln` passes (55 tests).
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/NEO-23-implementation-plan.md` (branch) | **Matches** — routes, DTO names as documented (`PlayerTargetStateResponse`, `TargetSelectRequest`/`Response`), soft lock, sequence rules, README scope, and acceptance bullets align with the code under review. |
|
||
| `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Matches** (post–follow-up) — Summary **Status**, **Implementation snapshot**, and NEO-23 `Game/Targeting/` + README pointers reflect the JSON v1 spike; later contracts remain explicitly open. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** (post–follow-up) — E1.M3 **In Progress**; note text covers NEO-23 partial slice vs remaining contracts. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** (post–follow-up) — E1.M3 row register + snapshot + plans cell aligned with NEO-23 merge and follow-on Linear issues. |
|
||
| `docs/decomposition/modules/contracts.md` (versioned JSON spike discipline) | **Matches** — explicit `schemaVersion`, throwaway JSON over HTTP, consistent with other prototype routes. |
|
||
|
||
**Register / tracking table:** ~~After merge, **should** update `module_dependency_register.md` and `documentation_and_implementation_alignment.md` (and optionally the E1.M3 module snapshot) so status and inventory match the new `Game/Targeting/` + README section.~~ **Done** on branch (same PR window as NEO-23).
|
||
|
||
## Blocking issues
|
||
|
||
(none)
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Doc alignment in merge train:** Update `module_dependency_register.md`, `documentation_and_implementation_alignment.md`, and the **Implementation snapshot** in `E1_M3_InteractionAndTargetingLayer.md` so E1.M3 is not described as having zero implementation once NEO-23 lands (per this repo’s own alignment policy). A follow-up doc-only PR is acceptable if called out explicitly for the merge.~~ **Done.**
|
||
|
||
2. ~~**Test gap — `GET` 404:** `PostSelect_ShouldReturnNotFound_WhenPlayerUnknown` covers unknown player on POST. Adding a parallel test for `GET /game/players/{id}/target` would lock the same contract as position/interact read paths.~~ **Done.**
|
||
|
||
3. ~~**`PlayerTargetStateReaderTests` depth:** The plan’s optional table called out inclusive-radius boundary cases (similar to `HorizontalReachTests`). Current unit tests cover `null` lock and unknown registry id only; adding one boundary assertion would better match the plan’s optional coverage intent.~~ **Done.**
|
||
|
||
## Nits
|
||
|
||
- **Nit:** Wire type name `PlayerTargetStateResponse` vs illustrative doc name `TargetState` is already explained in the NEO-23 plan and README; no change required if PR text points readers there once.
|
||
|
||
## Verification
|
||
|
||
- `dotnet test NeonSprawl.sln` (author ran: all passed).
|
||
- Manual: `curl` examples from `server/README.md` **Targeting (NEO-23)** against a running server (optional smoke).
|