NEO-85: Add code review for client combat HUD.
parent
faf3fe1139
commit
7466f2bcba
|
|
@ -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.
|
||||
Loading…
Reference in New Issue