NEO-97: add code review for E5M2-11 client telegraph HUD
parent
cfda0acd9a
commit
39ec50efa2
|
|
@ -0,0 +1,68 @@
|
|||
# Code review — NEO-97 (E5M2-11)
|
||||
|
||||
**Date:** 2026-05-29
|
||||
**Scope:** Branch `NEO-97-e5m2-11-client-telegraph-hud` vs `origin/main` — commits `b0d0c42` … `cfda0ac`
|
||||
**Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits**
|
||||
|
||||
## Summary
|
||||
|
||||
NEO-97 adds the Epic 5 Slice 2 **client half**: thin HTTP clients for **`GET /game/world/npc-runtime-snapshot`** and **`GET /game/players/{id}/combat-health`**, a RefCounted windup display mirror between polls, ~1 Hz combat-active polling wired from `main.gd`, and three HUD labels (`PlayerCombatHpLabel`, `NpcStateLabel`, `TelegraphLabel`). The implementation follows NEO-85/NEO-32 patterns (injectable HTTP, `_busy` guard, local tick-down smoothing) and matches kickoff decisions (combat-active gate, locked + incoming rows, interpolated windup). Four GdUnit suites cover parse/URL helpers, windup reconciliation, and combat-active / NPC state label logic via a test harness. Docs (plan, manual QA, README, E5M2 backlog, module + alignment register) are updated. Primary risk is **logic duplication** between `main.gd` and the test harness (drift over time) and a **stale module risk bullet** that still says “no client windup math.” No blocking correctness issues for the documented manual QA flows (lock → cast → telegraph → player HP drop).
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Path | Result |
|
||||
|------|--------|
|
||||
| `docs/plans/NEO-97-implementation-plan.md` | **Matches** — kickoff decisions (combat-active gate, locked + incoming HUD, interpolate windup, keep `prototype_target_markers.gd`) reflected in code; reconciliation section accurate; AC checked. |
|
||||
| `docs/plans/E5M2-prototype-backlog.md` (E5M2-11) | **Matches** — AC checked, landed note present; script name in backlog still says `npc_archetype_markers.gd` but plan kickoff explicitly adopted `prototype_target_markers.gd` (documented divergence). |
|
||||
| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Partially matches** — NEO-97 landed line under Related implementation slices is correct; **Risks** bullet still says “no client windup math” (conflicts with kickoff interpolate decision); NPC instance row still says “Breaking: Godot constants migrate in NEO-97” though constants landed in NEO-91. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-97 landed** with README/plan/manual QA links. |
|
||||
| `docs/manual-qa/NEO-97.md` | **Matches** — Godot steps cover lock, cast, telegraph countdown, player HP drop, incoming threat row, regressions. |
|
||||
| `client/README.md` | **Matches** — NPC runtime + telegraph HUD section documents poll gate, labels, tab order, script paths. |
|
||||
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — read-only GET polling; no client authority over combat/NPC state; poll advances server lazy tick (accepted per plan). |
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **E5.M2 Risks bullet** — Update “Telegraph desync: … no client windup math” to “display-only windup tick-down between polls; reconciled on each `npc-runtime-snapshot` GET” so module docs align with the NEO-97 kickoff decision and `npc_runtime_hud_state.gd`.
|
||||
|
||||
2. **E5.M2 NPC instance row** — Change “Breaking: Godot constants migrate in NEO-97” to note migration **landed in NEO-91**; NEO-97 adds HUD poll clients only.
|
||||
|
||||
3. **Test coverage vs plan** — `npc_combat_hud_refresh_test.gd` covers combat-active bool + `NpcStateLabel` copy but not **`TelegraphLabel`** formatting or **timer start/stop** (called out in the implementation plan test table). Add at least one telegraph line assertion (locked vs incoming fallback) and, if feasible without heavy scene wiring, a timer-gate test mirroring `_update_combat_poll_gate()`.
|
||||
|
||||
4. **Shared HUD helpers** — Combat-active gate, row resolution, and label formatters are duplicated in `main.gd` (~180 lines) and `NpcCombatHudHarness` in tests. Extract to a small RefCounted (e.g. `npc_combat_hud_helpers.gd`) used by both to prevent drift; optional follow-up given NEO-85 precedent of wiring in `main.gd`.
|
||||
|
||||
5. **Telegraph at zero** — `_format_telegraph_line` can show **`· 0.0s`** while the snapshot row still carries `activeTelegraph` until the next poll. Consider returning empty when `display_windup_remaining` ≤ 0 so `TelegraphLabel` shows **`—`** between local expiry and server reconcile.
|
||||
|
||||
6. **Runtime sync failure** — `_on_npc_runtime_sync_failed` re-renders from the last good snapshot without surfacing failure on `NpcStateLabel` / `TelegraphLabel` (player HP already shows error). Acceptable for prototype; optionally dash NPC/telegraph lines or append a sync-failed hint for QA clarity.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `npc_runtime_client.gd` stores `_last_snapshot` but static `npc_row()` does not fall back to it (unlike `combat_targets_client.target_row()`); field is write-only today.
|
||||
|
||||
- Nit: Duplicate `MockHttpTransport` inner classes in `npc_runtime_client_test.gd` and `player_combat_health_client_test.gd` — fine for now; a shared test helper would reduce copy-paste.
|
||||
|
||||
- Nit: `main.gd` grows ~250 lines for NEO-97; repo policy prefers thin `main.gd` — track for a future extract when the next HUD story lands.
|
||||
|
||||
- Nit: Plan text references `serverTimeUtc` anchor in `npc_runtime_hud_state.gd`; implementation uses receive tick + server `windupRemainingSeconds` only — sufficient for display smoothing; plan wording could be tightened.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# GdUnit (CI uses Godot 4.6 headless — not available locally in review env)
|
||||
cd client && ~/godot/Godot_v4.6-stable_linux.x86_64 --headless --path . \
|
||||
-s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test
|
||||
|
||||
# Lint / format (CI)
|
||||
pip install "gdtoolkit==4.5.0"
|
||||
gdlint client/scripts client/test
|
||||
gdformat --check client/scripts client/test
|
||||
```
|
||||
|
||||
**Manual (required before merge):** [`docs/manual-qa/NEO-97.md`](../manual-qa/NEO-97.md) — server + Godot F5; elite lock → cast → ~7.5 s telegraph window → player HP **75/100**; incoming threat row when locking a different NPC while elite holds aggro.
|
||||
Loading…
Reference in New Issue