NEO-122: Add code review for quest progress HUD.
parent
e37671d815
commit
4e75ec6493
|
|
@ -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"
|
||||
```
|
||||
Loading…
Reference in New Issue