NEO-122: Add follow-up code review for economy HUD layout.
parent
632a7472de
commit
64da7611d9
|
|
@ -0,0 +1,64 @@
|
||||||
|
# Code review follow-up — NEO-122 (E7M1-11)
|
||||||
|
|
||||||
|
**Date:** 2026-06-07
|
||||||
|
**Scope:** Branch `NEO-122-client-quest-progress-accept-hud` vs `origin/main` — commits `b36fe6d` … `632a747` (delta after [initial review](2026-06-07-NEO-122.md))
|
||||||
|
**Base:** `origin/main`
|
||||||
|
**Prior review:** `docs/reviews/2026-06-07-NEO-122.md` — prior suggestions **1** / **3** and nits **1–2** addressed in `b36fe6d`; suggestion **2** (extract quest HUD) still deferred.
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
**Approve with nits**
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
Seven commits landed after the first review: code-review follow-up (quest-definitions error HUD + accept-failure GdUnit), then a **prototype economy/debug HUD layout refactor** so NEO-122 manual QA step 7 (craft **`refine_scrap_standard`** with recipes visible) is reliable. Changes include **`HudRootScroll`** wrapping debug labels, **`EconomyHudSection`** pinned to the viewport bottom with **`BodyScroll`**, **`craft_recipe_panel.gd`** flattened (no nested scroll; recipes-first in economy body), and layout clamping in **`prototype_economy_hud_section.gd`**. New/extended GdUnit: **`craft_recipe_panel_test.gd`**, viewport layout tests in **`prototype_economy_hud_section_test.gd`**. Quest client behavior from the first review remains sound; risk is mainly **plan/doc drift** and **cross-story HUD regression** (NEO-74/75/110), not quest contract correctness.
|
||||||
|
|
||||||
|
## Documentation checked
|
||||||
|
|
||||||
|
| Path | Result |
|
||||||
|
|------|--------|
|
||||||
|
| `docs/plans/NEO-122-implementation-plan.md` | **Partially matches** — quest HUD reconciliation still accurate; **economy HUD layout refactor** (HudRootScroll, pinned economy panel, recipe panel restructure, recipe-first ordering) **not** recorded in plan reconciliation or kickoff scope. |
|
||||||
|
| `docs/manual-qa/NEO-122.md` | **Partially matches** — step 7 craft visibility added; **duplicate step 9** numbering (two lines labeled **9.**). |
|
||||||
|
| `client/README.md` | **Matches** quest + economy node paths (`HudRootScroll`, `BodyScroll/Body`); test-scope paragraph omits new **`craft_recipe_panel_test.gd`**. |
|
||||||
|
| `docs/manual-qa/NEO-74.md` / `NEO-75.md` / `NEO-110.md` | **Not updated** — no HudRootScroll / economy-pin notes; spot-check recommended (non-blocking). |
|
||||||
|
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — unchanged since first review. |
|
||||||
|
| Full-stack epic decomposition | **Matches** — still client slice only; NEO-123 capstone deferred. |
|
||||||
|
|
||||||
|
## Blocking issues
|
||||||
|
|
||||||
|
(none)
|
||||||
|
|
||||||
|
## Suggestions
|
||||||
|
|
||||||
|
1. **Reconcile implementation plan** — Append a short **NEO-122 follow-up** bullet to `docs/plans/NEO-122-implementation-plan.md` **Implementation reconciliation**: economy HUD pinned layout, `HudRootScroll`, `craft_recipe_panel` VBox + `RecipeRows`, recipes-first in `BodyScroll/Body`, and manual QA step 7 rationale. Keeps [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) honest about scope expansion.
|
||||||
|
|
||||||
|
2. **Fix manual QA numbering** — In `docs/manual-qa/NEO-122.md`, renumber the second **9.** (server-down optional) to **10.** and the definitions-error optional to **11.**; update **Acceptance** if it should include the craft-visibility step (currently “Steps 1–8” while required steps now include **9**).
|
||||||
|
|
||||||
|
3. **Regression spot-check** — Run abbreviated passes of [NEO-74](../manual-qa/NEO-74.md), [NEO-75](../manual-qa/NEO-75.md), and [NEO-110](../manual-qa/NEO-110.md) after merge: collapsed economy toggle, gather/craft feedback still visible in **`HudRootScroll`**, encounter labels still scroll when debug stack is tall.
|
||||||
|
|
||||||
|
## Nits
|
||||||
|
|
||||||
|
1. Nit: `main.tscn` **`EconomyHudSection`** still has editor offsets (`offset_top = 400`) overwritten at runtime by **`_apply_viewport_layout`** — possible one-frame layout flash on boot; acceptable for prototype.
|
||||||
|
|
||||||
|
2. Nit: **`prototype_economy_hud_section.gd`** locates **`HudRootScroll`** by sibling name string — works with current scene tree and is covered by GdUnit; brittle if `UICanvas` children reorder.
|
||||||
|
|
||||||
|
3. Nit: Economy **`BodyScroll/Body`** child order is now **Recipes → Inventory → Skills → Gig XP** (was inventory-first). NEO-75 steps do not mandate order, but players accustomed to inventory-at-top may need a scroll on first open.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd client
|
||||||
|
godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd \
|
||||||
|
--ignoreHeadlessMode \
|
||||||
|
-a test/quest_progress_client_test.gd \
|
||||||
|
test/quest_definitions_client_test.gd \
|
||||||
|
test/craft_recipe_panel_test.gd \
|
||||||
|
test/prototype_economy_hud_section_test.gd
|
||||||
|
```
|
||||||
|
|
||||||
|
Manual (per updated `docs/manual-qa/NEO-122.md`):
|
||||||
|
|
||||||
|
1. **F5** with fresh server — quest labels visible in **`HudRootScroll`**; economy panel pinned bottom-left, not off-screen.
|
||||||
|
2. Steps 1–9 (craft step 7–8: **Refine Scrap (Standard)** first row, scroll at top).
|
||||||
|
3. Optional error paths (steps 10–11).
|
||||||
|
4. Toggle **Economy HUD** off/on — debug labels in **`HudRootScroll`** remain; recipe repopulate + scroll reset still works.
|
||||||
Loading…
Reference in New Issue