diff --git a/docs/reviews/2026-05-25-NEO-85.md b/docs/reviews/2026-05-25-NEO-85.md new file mode 100644 index 0000000..2885d53 --- /dev/null +++ b/docs/reviews/2026-05-25-NEO-85.md @@ -0,0 +1,62 @@ +# Code review — NEO-85 client combat feedback + target HP HUD + +**Date:** 2026-05-25 +**Scope:** Branch `NEO-85-client-combat-feedback-hp-hud` · commits `f03e689`–`faf3fe1` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-85 delivers **E5M1-11**: the Godot client parses nested **`combatResolution`** on cast accept, extends **`CastFeedbackLabel`** with damage/defeat copy, adds **`CombatTargetsClient`** for **`GET /game/world/combat-targets`**, and wires **`CombatTargetHpLabel`** with event-driven refresh on successful cast and target lock change. Implementation follows NEO-28/NEO-73 thin-client patterns (injectable HTTP, `_busy` guard, harness tests) and keeps combat math server-authoritative. Fifteen new/extended GdUnit cases pass locally. Residual risk is low — client-only HUD wiring atop landed NEO-82/NEO-83 server contracts; brief HP label lag between cast feedback and GET completion is an adopted trade-off. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-85-implementation-plan.md`](../plans/NEO-85-implementation-plan.md) | **Matches** — kickoff decisions (event-driven refresh, extend `CastFeedbackLabel`, dedicated HP label), scope, acceptance checklist, and reconciliation align with code. | +| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-11** | **Matches** — AC checked; landed note cites clients + manual QA. | +| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Partially matches** — Status line cites E5M1-11 NEO-85 landed; **Related implementation slices** lacks an **E5M1-11** entry (NEO-78–NEO-84 each have one). | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Conflicts** — E5.M1 note still ends with **Pending client: E5M1-11 NEO-85** despite landed implementation. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row cites NEO-85 landed + manual QA + client README. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — no client-side damage math; HP from server GET snapshot; cast feedback from wire fields only. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E1.M4** | **Partially matches** — NEO-85 extends **`ability_cast_client.gd`** / **`CastFeedbackLabel`** (E1.M4 cast funnel) but E1.M4 row stops at NEO-32; optional cross-reference only. | +| [`client/README.md`](../../client/README.md) | **Matches** — combat HUD subsection documents cast resolution copy, HP label, refresh triggers, dev fixture reset. | +| [`docs/manual-qa/NEO-85.md`](../manual-qa/NEO-85.md) | **Matches** — Godot steps cover lock → cast → HP stepping → defeat → deny → clear lock; regression bullets for NEO-28/NEO-32. | +| Full-stack epic decomposition | **Matches** — client half of NEO-82/NEO-83 stack; manual QA is Godot-first (not Bruno-only). | + +## Blocking issues + +None. + +## Suggestions + +1. **Stale `module_dependency_register` E5.M1 note** — Replace **Pending client: E5M1-11 NEO-85** with a **NEO-85 landed** one-liner (mirror the E5.M1 alignment row: `combat_targets_client.gd`, `CombatTargetHpLabel`, event-driven GET, manual QA link). + +2. **Missing E5M1-11 Related implementation slice** — Add an **E5M1-11 (NEO-85)** bullet under **Related implementation slices** in `E5_M1_CombatRulesEngine.md` (client combat HUD; link plan, manual QA, `client/README.md` combat section) so module readers stay consistent with E5M1-04–E5M1-10 entries. + +3. **HTTP error-path test gap** — Plan §Tests lists **404/invalid schema** failure for `combat_targets_client_test.gd`; implementation covers schema mismatch only. Add a mock **404** (or non-2xx) case asserting **`targets_sync_failed`** with **`HTTP 404`** for parity with other world GET clients. + +## Nits + +- Nit: **`CombatRefreshHarness`** duplicates `_locked_target_id_from_state` / refresh logic from `main.gd` rather than testing `main.gd` directly — matches **`craft_feedback_refresh_test.gd`** precedent; acceptable for prototype scope. + +- Nit: Clearing lock (Esc) still fires a combat-target GET even though the HP label already renders **no lock** — harmless extra HTTP; skip refresh when `new_lock` is empty if chatter matters later. + +- Nit: Between cast accept and GET completion, **`CastFeedbackLabel`** shows authoritative **`targetRemainingHp`** from **`combatResolution`** while **`CombatTargetHpLabel`** may still show the prior snapshot row — adopted plan trade-off; optional future polish could render HP from resolution until GET lands. + +## Verification + +```bash +cd /home/don/neon-sprawl/client +godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \ + -a res://test/ability_cast_client_test.gd \ + -a res://test/combat_targets_client_test.gd \ + -a res://test/combat_feedback_refresh_test.gd +``` + +**GdUnit (local run):** 15/15 passed (8 + 4 + 3 suites). + +**Manual:** [`docs/manual-qa/NEO-85.md`](../manual-qa/NEO-85.md) — server + Godot; Tab lock **`prototype_target_alpha`**, digit cast **`prototype_pulse`**, verify cast resolution copy and HP label stepping through defeat; optional **`POST /game/__dev/combat-targets-fixture`** between runs.