63 lines
4.7 KiB
Markdown
63 lines
4.7 KiB
Markdown
# Code review — NEO-110 (E5M3-11)
|
||
|
||
**Date:** 2026-05-31
|
||
**Scope:** Branch `NEO-110-client-encounter-progress-loot-feedback-hud` vs `c302220` (merge-base on `main`) — commits `3128f36` … `bcc9bf1`
|
||
**Base:** `c302220` (main at branch point, post NEO-109 merge)
|
||
|
||
## Verdict
|
||
|
||
**Request changes**
|
||
|
||
## Summary
|
||
|
||
NEO-110 adds the Godot client for **`GET /game/players/{id}/encounter-progress`**: **`encounter_progress_client.gd`** mirrors **`gig_progression_client.gd`** (injectable HTTP, `_busy` guard, v1 parse, signals), **`main.gd`** wires boot hydrate + defeat-triggered refresh, and **`EncounterProgressLabel`** / **`EncounterCompleteLabel`** sit in **`HudRoot`** after **`PlayerCombatHpLabel`**. GdUnit covers parse paths and 404; manual QA and README are thorough. Architecture matches server authority and NEO-108 contract. One **runtime bug** in loot rendering blocks merge: **`main.gd`** calls **`_item_display_name`**, which is not defined on the main script (only on **`craft_recipe_panel.gd`**). Completing the third NPC will hit that path and error when painting grant lines.
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/NEO-110-implementation-plan.md` | **Matches** — kickoff decisions adopted; reconciliation and acceptance checklist accurate. |
|
||
| `docs/plans/E5M3-prototype-backlog.md` (E5M3-11) | **Matches** — AC checked; landed note cites plan, manual QA, client README. |
|
||
| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — NEO-110 client HUD snapshot bullet added. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-110** client HUD; **E5M3-12** capstone still outstanding. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E5.M3 note still ends with **“E5M3-11+ client HUD outstanding.”** Should cite **NEO-110** landed (non-blocking doc drift). |
|
||
| `docs/manual-qa/NEO-110.md` | **Matches** — Godot-only steps, defeat chain, HUD expectations, inventory auto-refresh. |
|
||
| `client/README.md` | **Matches** — encounter HUD subsection; refresh triggers documented. |
|
||
| Full-stack epic decomposition | **Matches** — client issue **NEO-110** fulfills server **NEO-108** counterpart; not claiming E5M3-12 capstone. |
|
||
|
||
## Blocking issues
|
||
|
||
1. **`_item_display_name` missing on `main.gd`** — `_render_encounter_complete_label()` calls `_item_display_name(item_id)` (line ~820), but **`main.gd` defines only `_inventory_item_label()`**, which already wraps **`ItemDefinitionsClient.display_name_for`**. **`craft_recipe_panel.gd`** owns **`_item_display_name`** on a different node. On encounter **`completed`**, Godot will throw when iterating **`rewardGrantSummary`**. **Fix:** use **`_inventory_item_label(item_id)`** (plan’s NEO-72 precedent) or add a shared helper on **`main.gd`**.
|
||
|
||
## Suggestions
|
||
|
||
1. **`_on_item_definitions_ready` coupling to inventory error** — The handler returns early when **`_inventory_error`** is set, so a failed inventory GET prevents re-painting encounter loot after item defs load (plan adopted re-paint on **`definitions_ready`** when snapshot is **`completed`**). Consider re-painting encounter labels independently of inventory sync state.
|
||
|
||
2. **Update `module_dependency_register.md` E5.M3 note** — Replace **“E5M3-11+ client HUD outstanding”** with **NEO-110 landed** + pointer to client README / manual QA; keep **NEO-111** as outstanding capstone.
|
||
|
||
3. **GdUnit: invalid JSON / schema mismatch** — Optional test that **`parse_encounter_progress_json`** returns **`null`** and that a mock 200 with bad body emits **`encounter_sync_failed`**; improves parity with gig client coverage if added later.
|
||
|
||
## Nits
|
||
|
||
- Nit: Parse tests instantiate a throwaway **`EncounterProgressClient`** to call **`encounter_row`**; could use a static helper or test **`encounter_row`** via the client under test only — readability only.
|
||
|
||
- Nit: **`_busy`** drops overlapping GETs (e.g. boot + first defeat); plan accepted this for prototype; manual QA notes ~1 s lag — fine.
|
||
|
||
- Nit: **`main.gd`** grows by ~130 lines; plan keeps HUD format inline — acceptable until NEO-111 or a future extract.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# GdUnit (set Godot path first)
|
||
export GODOT_BIN=/path/to/godot
|
||
cd client && ./addons/gdUnit4/runtest.sh -a test/encounter_progress_client_test.gd
|
||
|
||
# Manual (primary for this story)
|
||
# docs/manual-qa/NEO-110.md — server restart, F5, defeat three NPCs,
|
||
# assert 0/3 → 1/3 → 2/3 → completed (3/3), loot lines, inventory without I
|
||
|
||
# After fix: confirm third-defeat does not error in Output when EncounterCompleteLabel renders grants
|
||
```
|
||
|
||
**Reviewer note:** GdUnit was not executed in this environment (`GODOT_BIN` unset). Run locally before merge.
|