diff --git a/docs/reviews/2026-05-31-NEO-110.md b/docs/reviews/2026-05-31-NEO-110.md new file mode 100644 index 0000000..61b044d --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-110.md @@ -0,0 +1,62 @@ +# 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.