neon-sprawl/docs/reviews/2026-06-07-NEO-122.md

Code review — NEO-122 (E7M1-11)

Date: 2026-06-07
Scope: Branch NEO-122-client-quest-progress-accept-hud vs origin/main — commits 150cb73 … e37671d
Base: origin/main

Verdict

Approve with nits — suggestions 1 and 3 and nits 1–2 below are done (strikethrough + Done.); suggestion 2 deferred to NEO-123.

Summary

NEO-122 delivers the Godot client slice for Epic 7 quest visibility: quest_progress_client.gd (GET progress + POST accept with separate sync/accept HTTP and busy guards), quest_definitions_client.gd (cached display names + catalog-order iteration), HUD labels QuestProgressLabel / QuestAcceptFeedbackLabel, and main.gd wiring for boot hydrate, Q / Shift+Q accept, and event-driven GET refresh after gather, craft success, targetDefeated, and accept. Implementation mirrors encounter_progress_client.gd / recipe_definitions_client.gd precedents and respects server authority (client displays GET snapshots only; Shift+Q prerequisite check is UX-only). Twelve GdUnit cases cover parse, sync, 404, accept happy/deny, and definitions display names with full AAA layout. Documentation is thorough: implementation plan reconciliation, backlog checkboxes, E7.M1 module snapshot, alignment register, client/README.md, and docs/manual-qa/NEO-122.md. Risk is low; human manual QA remains the main pre-merge gate.

Documentation checked

Path	Result
docs/plans/NEO-122-implementation-plan.md	Matches — kickoff decisions adopted (defs client, Q/Shift+Q semantics, four-quest HUD); reconciliation accurate; manual QA item correctly left open.
docs/plans/E7M1-prototype-backlog.md (E7M1-11)	Matches — acceptance checkboxes checked; landed note + README/manual QA links.
docs/decomposition/modules/E7_M1_QuestStateMachine.md	Matches — NEO-122 client HUD bullet + links.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E7M1-11 / NEO-122 landed note appended.
docs/decomposition/modules/module_dependency_register.md	Matches — E7.M1 row stays Planned until NEO-123 capstone (correct).
docs/decomposition/modules/client_server_authority.md	Matches — client displays server GET snapshot; accept is intent POST; no local objective math.
client/README.md	Matches — quest HUD subsection: routes, keys, refresh triggers, test file list.
docs/manual-qa/NEO-122.md	Matches — Godot-only checklist; Shift+Q combat-intro caveat documented.
Full-stack epic decomposition	Matches — paired client issue NEO-122; does not claim E7 Slice 1 capstone complete (NEO-123).

Register / tracking: E7.M1 correctly remains Planned until client capstone NEO-123; no further register status change required for this merge.

Blocking issues

(none)

Suggestions

1. Surface definitions_sync_failed on HUD — _quest_defs_error is set in _on_quest_definitions_sync_failed but never read in _render_quest_progress_label. Recipe defs follow the visible pattern (Recipes: error — {reason} on CraftFeedbackLabel). Consider painting e.g. Quests:\nerror — definitions {reason} (or a suffix on the header) so a defs-only failure is visible; today the HUD silently falls back to raw quest ids from the progress snapshot. Done. _render_quest_progress_label paints definitions error — {reason} when defs sync fails (during loading or alongside progress rows).

2. Extract quest HUD wiring from main.gd — ~250 lines of quest setup/render/accept logic were added to an already large composer. Per godot-client-script-organization, a child quest_hud_controller.gd (signals in, label text out) would match NEO-110-era precedent deferral but reduce merge conflict surface before NEO-123 capstone work. Non-blocking for this story. Deferred to NEO-123 capstone or follow-up refactor.

3. GdUnit: quest_accept_failed + accept-busy guard — Client tests cover sync 404 and structured deny (accepted: false), but not transport/HTTP failure on accept (quest_accept_failed) or request_accept returning false when _accept_busy. Small additions would lock the error path NEO-122 manual QA step 9 exercises. Done. test_http_404_on_accept_emits_quest_accept_failed and test_request_accept_returns_false_when_accept_busy in quest_progress_client_test.gd.

Nits

1. Nit: test_request_accept_emits_deny_with_reason_code omits await assert_signal(...) used elsewhere; safe with the synchronous mock transport today, but inconsistent with test_request_sync_emits_quest_progress_received. Deferred. Synchronous mock transport emits before await assert_signal can observe; deny test keeps sync capture (same pattern as craft_client_test.gd).

2. Nit: _render_quest_progress_labels() is a one-line wrapper around _render_quest_progress_label() — harmless; could inline unless more quest labels land in NEO-123. Done. Wrapper removed; callers invoke _render_quest_progress_label() directly.

3. Nit: Shift+Q may offer prototype_quest_combat_intro before refine when gather is still active (documented in manual QA Notes) — server deny is authoritative; acceptable prototype UX.

Verification

# GdUnit (requires Godot + gdUnit4 in PATH)
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

# GDScript lint (if .venv-gd installed)
gdlint client/scripts/quest_progress_client.gd client/scripts/quest_definitions_client.gd client/scripts/main.gd
gdformat --check client/scripts/quest_progress_client.gd client/scripts/quest_definitions_client.gd

Manual (per docs/manual-qa/NEO-122.md):

1. Fresh server + Godot F5 — four quests not started; idle accept hint visible.
2. Q accept gather → active step 1 + success feedback.
3. Gather ×3 → counter updates; gather completed.
4. Shift+Q before/after gather complete — deny vs refine accept.
5. Craft refine_scrap_standard → refine completed.
6. Duplicate Q on completed gather → denied — already_completed (or already_active).
7. Optional: server down → Quests: error — … or Quest accept: failed — ….

Server regression (no server changes, sanity):

dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Quest"