don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-07-NEO-122-follow-u...

6.0 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

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

  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.

Verification

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

Full suite: 247 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):

  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.