94 lines
6.7 KiB
Markdown
94 lines
6.7 KiB
Markdown
# 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.
|
||
**Follow-up:** suggestions **1–2** below are **done** (strikethrough + **Done.**); suggestion **3** is manual regression spot-check for author/reviewer.
|
||
|
||
## 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**~~ **Matches** — NEO-122 follow-up reconciliation added (economy HUD layout refactor). **Done.** |
|
||
| `docs/manual-qa/NEO-122.md` | ~~**Partially matches**~~ **Matches** — duplicate step 9 fixed; acceptance includes craft visibility. **Done.** |
|
||
| `client/README.md` | ~~**Partially matches**~~ **Matches** — test-scope includes **`craft_recipe_panel_test.gd`**. **Done.** |
|
||
| `docs/manual-qa/NEO-74.md` / `NEO-75.md` / `NEO-110.md` | **Updated** — HudRootScroll / economy-pin regression notes added. **Done.** |
|
||
| `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.~~ **Done.** Follow-up subsection in plan reconciliation; tests table extended.
|
||
|
||
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**).~~ **Done.** Steps 1–12; acceptance covers steps 1–9 + craft visibility.
|
||
|
||
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.
|
||
|
||
## Bugbot follow-up (PR #161)
|
||
|
||
1. ~~**Shift+Q ignores progress sync failure** — After GET fails, `_request_eligible_accept` blocked accept when `_progress_error` is set; GdUnit **`test_shift_q_blocks_when_progress_sync_failed`**. **Done.**
|
||
|
||
2. ~~**Hud scroll minimum causes panel overlap** — Removed 80px floor in `_apply_hud_root_scroll_bounds`; clamp to `maxf(0.0, available_height)`; layout test asserts scroll height ≤ panel top − gap. **Done.**
|
||
|
||
3. ~~**GdUnit CI exit 101 (orphans)** — Rewrote **`quest_hud_controller_test.gd`** with `MockHttpTransport` / `auto_free()` on all nodes; full suite **247 tests, 0 orphans, exit 0**. **Done.**
|
||
|
||
4. ~~**Quest HUD stale after accept** — Merge accept-response `quest` row into snapshot immediately; re-apply patch when stale GET returns `not_started`; queue pending sync when GET busy. **Done.**
|
||
|
||
5. ~~**Accept hidden by progress error** — Clear `_progress_error` on successful accept merge; render snapshot rows with `sync error — …` banner instead of error-only early return. **Done.**
|
||
|
||
6. ~~**Sync fail drops accept merge** — `_on_sync_failed` no longer clears `_accept_quest_patch` or merged snapshot; rebuilds from patch when snapshot empty. **Done.**
|
||
|
||
7. ~~**Shift+Q hides definitions failure** — Block eligible accept when `_defs_error` is set and catalog is empty. **Done.**
|
||
|
||
8. ~~**Definitions ready skips HUD repaint** — `_on_definitions_ready` repaints when snapshot exists even if `_progress_error` is set so display names replace raw ids. **Done.**
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd client
|
||
godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd \
|
||
--ignoreHeadlessMode \
|
||
-a res://test
|
||
```
|
||
|
||
Full suite: **255** cases, **0** orphans (GdUnit exit **0**).
|
||
|
||
```bash
|
||
# Focused NEO-122 / layout subset
|
||
godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd \
|
||
--ignoreHeadlessMode \
|
||
-a test/quest_progress_client_test.gd \
|
||
test/quest_definitions_client_test.gd \
|
||
test/quest_hud_controller_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.
|