From 4e75ec6493e16c270bf9be24b872571ad25deaf9 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 12:44:35 -0400 Subject: [PATCH] NEO-122: Add code review for quest progress HUD. --- docs/reviews/2026-06-07-NEO-122.md | 78 ++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 docs/reviews/2026-06-07-NEO-122.md diff --git a/docs/reviews/2026-06-07-NEO-122.md b/docs/reviews/2026-06-07-NEO-122.md new file mode 100644 index 0000000..2ade946 --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-122.md @@ -0,0 +1,78 @@ +# 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** + +## 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. + +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](../../.cursor/rules/godot-client-script-organization.md), 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. + +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. + +## 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`. + +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. + +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 + +```bash +# 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): + +```bash +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Quest" +```