From 64da7611d98480419fd3a9075b57d8591db151c1 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 13:35:06 -0400 Subject: [PATCH] NEO-122: Add follow-up code review for economy HUD layout. --- docs/reviews/2026-06-07-NEO-122-follow-up.md | 64 ++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 docs/reviews/2026-06-07-NEO-122-follow-up.md diff --git a/docs/reviews/2026-06-07-NEO-122-follow-up.md b/docs/reviews/2026-06-07-NEO-122-follow-up.md new file mode 100644 index 0000000..f8a2ce7 --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-122-follow-up.md @@ -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.