NEO-75: add code review for capstone integration branch

pull/109/head
VinPropane 2026-05-24 20:34:36 -04:00
parent 977bcc3b27
commit cf0ef2d23e
1 changed files with 60 additions and 0 deletions

View File

@ -0,0 +1,60 @@
# Code review — NEO-75 playable gather→refine→make loop (capstone)
**Date:** 2026-05-24
**Scope:** Branch `NEO-75-e3-playable-gather-refine-make-loop` · commits `fbae314``977bcc3` vs `origin/main`
**Base:** `origin/main`
## Verdict
**Approve with nits** — capstone docs, HUD collapse, and path updates match the adopted plan; GdUnit passes. Merge after human capstone manual QA; minor module/README doc consistency nits below.
## Summary
The branch completes Epic 3 Slice 5 capstone integration: collapsible **`EconomyHudSection`** wraps inventory/skills/recipe panel under a **`Economy HUD`** checkbox, **`main.gd`** `@onready` paths follow the reparent, and a focused **`prototype_economy_hud_section.gd`** script toggles body visibility. Docs deliver the zero-Bruno capstone script (`docs/manual-qa/NEO-75.md`), README end-to-end loop section, implementation plan reconciliation, alignment register updates, and E3S5-04 backlog checkboxes. No new HTTP routes or economy client logic — the gather→refine→make loop reuses NEO-7274 wiring as intended. Risk is low: layout-only client change plus documentation; automated coverage is the HUD toggle suite only (orchestration remains manual QA per plan).
## Documentation checked
| Document | Result |
|----------|--------|
| [`docs/plans/NEO-75-implementation-plan.md`](../plans/NEO-75-implementation-plan.md) | **Matches** — kickoff decisions (docs-first, implement HUD collapse, fresh server session, visible XP only) reflected; acceptance checklist and reconciliation complete. |
| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) · **E3S5-04** | **Matches** — capstone manual QA, README integration, optional HUD collapse, alignment cross-links; AC checked. |
| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — status notes NEO-74 + NEO-75 landed. |
| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Partially matches** — alignment register cites NEO-75 capstone; module **Status** row still lists Slice 2 only (no NEO-73/75 client note). |
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Partially matches** — alignment register cites NEO-72/75; module **Status** row still lists Slice 1 server stories only. |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1/M2/M3 rows note NEO-75 landed and Slice 5 client complete. |
| [`docs/decomposition/epics/epic_03_crafting_economy.md`](../decomposition/epics/epic_03_crafting_economy.md) | **Partially matches** — Slice 5 AC prose unchanged; plan listed optional epic status update when capstone ships. |
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — no client-side stack math; display-only HUD collapse. |
| [`docs/manual-qa/NEO-75.md`](../manual-qa/NEO-75.md) | **Matches** — single-session script, material math, collapse step; acceptance boxes left for human sign-off (expected). |
| [`client/README.md`](../../client/README.md) | **Partially matches** — NEO-75 loop section and NEO-72/74 paths updated; NEO-73 gather subsection still references bare **`InventoryLabel`** / **`SkillProgressionLabel`** paths. |
| Full-stack epic decomposition | **Matches** — E3S5-04 **client** capstone completes NEO-72→75 chain; Godot manual QA authoritative (not Bruno-only). |
## Blocking issues
(none)
## Suggestions
1. **Add Slice 5 capstone note to `epic_03_crafting_economy.md`** — Plan §4 called for a Slice 5 completion note when AC pass; alignment register and backlog are updated but the epic page still reads as open-ended. A one-line “Slice 5 client capstone (NEO-75) landed” under Slice 5 would close the loop.
2. **Refresh E3.M1 / E3.M3 module Status rows** — E3.M2 was updated; E3.M1 and E3.M3 module pages still show preSlice 5 status while the alignment register already cites NEO-73/72/75. Mirror the E3.M2 pattern (client slice landed + capstone) for doc parity.
3. **Align NEO-73 README HUD paths** — Update the gather section to `UICanvas/EconomyHudSection/Body/InventoryLabel` and `…/SkillProgressionLabel` so all three Slice 5 README sections reference the same hierarchy after the reparent.
## Nits
- Nit: **`test_toggle_collapses_body`** drives collapse via direct **`button_pressed = false`** rather than simulating a user click on **`ToggleButton`** — acceptable in headless GdUnit; behavior is covered.
- Nit: **`set_expanded`** and **`_on_toggle_toggled`** duplicate visibility logic — fine at this size; only matters if collapse rules grow (e.g. persist preference).
- Nit: Capstone AC for HUD collapse is checked in the implementation plan but not listed as a separate bullet in E3S5-04 backlog acceptance criteria — optional backlog tweak for traceability.
## Verification
```bash
# NEO-75 GdUnit suite (from client/)
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
-a res://test/prototype_economy_hud_section_test.gd
# Full client suite (CI parity)
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode
```
Manual (required before claiming capstone complete): follow [`docs/manual-qa/NEO-75.md`](../manual-qa/NEO-75.md) — **fresh server restart**, four-node gather (**11** scrap), two **`refine_scrap_standard`**, one **`make_field_stim_mk0`**, verify **`field_stim_mk0` ≥ 1**, salvage/refine XP movement, and economy HUD collapse/expand with populated panel.