6.7 KiB

Raw Blame History

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)
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

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 honest about scope expansion. Done. Follow-up subsection in plan reconciliation; tests table extended.
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.
Regression spot-check — Run abbreviated passes of NEO-74, NEO-75, and NEO-110 after merge: collapsed economy toggle, gather/craft feedback still visible in HudRootScroll, encounter labels still scroll when debug stack is tall.

Nits

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.
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.
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)

~~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.
~~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.
~~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.
~~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.
~~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.
~~Sync fail drops accept merge — _on_sync_failed no longer clears _accept_quest_patch or merged snapshot; rebuilds from patch when snapshot empty. Done.
~~Shift+Q hides definitions failure — Block eligible accept when _defs_error is set and catalog is empty. Done.
~~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

cd client
godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd \
  --ignoreHeadlessMode \
  -a res://test

Full suite: 255 cases, 0 orphans (GdUnit exit 0).

# 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):

F5 with fresh server — quest labels visible in HudRootScroll; economy panel pinned bottom-left, not off-screen.
Steps 1–9 (craft step 7–8: Refine Scrap (Standard) first row, scroll at top).
Optional error paths (steps 10–11).
Toggle Economy HUD off/on — debug labels in HudRootScroll remain; recipe repopulate + scroll reset still works.

6.7 KiB Raw Blame History Unescape Escape