63 lines
3.9 KiB
Markdown
63 lines
3.9 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
|
|
|
|
**Approve with nits**
|
|
|
|
## 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, 404, and schema mismatch; manual QA and README are thorough. Architecture matches server authority and NEO-108 contract. Review follow-up fixed loot rendering (**`_inventory_item_label`**), independent encounter re-paint on item defs, register drift, and extra GdUnit tests.
|
|
|
|
## 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` | **Matches** — E5.M3 note cites **NEO-110** landed; **NEO-111** capstone outstanding. Done. |
|
|
| `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()` now calls **`_inventory_item_label(item_id)`**. Done.
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**`_on_item_definitions_ready` coupling to inventory error**~~ — Inventory and encounter HUD re-paint independently when their respective snapshots are valid. Done.
|
|
|
|
2. ~~**Update `module_dependency_register.md` E5.M3 note**~~ — **NEO-110** landed + client README / manual QA pointers; **NEO-111** capstone outstanding. Done.
|
|
|
|
3. ~~**GdUnit: invalid JSON / schema mismatch**~~ — Added **`test_parse_encounter_progress_json_returns_null_for_schema_mismatch`** and **`test_invalid_schema_emits_encounter_sync_failed`**. Done.
|
|
|
|
## 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.
|