From ca9a3f1b388da6f635af9c88d8cc35f23c9c2c41 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 23:01:27 -0400 Subject: [PATCH] NEO-131: Add code review for quest completion reward HUD. --- docs/reviews/2026-06-07-NEO-131.md | 68 ++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 docs/reviews/2026-06-07-NEO-131.md diff --git a/docs/reviews/2026-06-07-NEO-131.md b/docs/reviews/2026-06-07-NEO-131.md new file mode 100644 index 0000000..493f74e --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-131.md @@ -0,0 +1,68 @@ +# Code review — NEO-131 (E7M2-08) + +**Date:** 2026-06-07 +**Scope:** Branch `NEO-131-client-quest-completion-reward-hud` vs `origin/main` — commits `5578de9` … `ed7975c` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-131 adds Godot client surfacing for server-owned quest completion grants: **`completion_reward_summary`** helper on **`quest_progress_client.gd`**, transition-only detection in **`quest_hud_controller.gd`** (`_previous_status_by_quest` boot-seed + catalog-order tie-break for multi-complete GETs), and **`QuestRewardDeliveryLabel`** wired from **`main.gd` / `main.tscn`** with optional **`ItemDefinitionsClient`** display names. Behavior matches adopted kickoff decisions (separate label, transition-only paint, title-case skill XP, sync-error idle **`—`** on reward label while NEO-122 progress error persists). Four new GdUnit cases use full AAA layout. Documentation is complete: implementation plan reconciliation, E7M2-08 backlog checkboxes, E7.M2 module anchor, alignment register note, **`client/README.md`**, and **`docs/manual-qa/NEO-131.md`**. Client-only slice with NEO-129 server dependency already on `main`; does not claim E7 Slice 2 capstone (NEO-132). Risk is low. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-131-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. | +| `docs/plans/E7M2-prototype-backlog.md` (E7M2-08) | **Matches** — acceptance checkboxes checked; landed note + README/plan/manual QA links. | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-131 client HUD snapshot under implementation anchor. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7M2-08 / NEO-131 landed note in row body; references column omits explicit [NEO-131 plan](../../plans/NEO-131-implementation-plan.md) link (nit). | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register row change required. | +| `docs/decomposition/modules/client_server_authority.md` | **Matches** — client displays GET snapshot only; no local grant math. | +| `client/README.md` | **Matches** — quest completion reward HUD subsection with routes, triggers, error pattern. | +| `docs/manual-qa/NEO-131.md` | **Matches** — Godot-only checklist; boot idempotency + operator chain optional steps. | +| Full-stack epic decomposition | **Matches** — paired client issue NEO-131; server counterpart NEO-129 documented; does not claim E7M2-09 capstone complete. | + +**Register / tracking:** E7.M2 correctly remains **Planned** until client capstone NEO-132; no further status change required for this merge. + +## Blocking issues + +(none) + +## Suggestions + +1. **GdUnit: item grant + display name** — New HUD tests cover skill XP transition, boot idempotency, and sync-error idle state. A small case with a mock **`ItemDefinitionsClient`** (or stub **`display_name_for`**) asserting **`Survey Drone Kit ×1`** would lock the operator-chain path referenced in manual QA and the freeze table without requiring Godot gameplay. + +2. **GdUnit: `definitions_ready` reward repaint** — **`_on_definitions_ready`** repaints the reward label when **`_last_reward_quest_id`** is set (quest display name upgrade). Mirror the existing NEO-122 progress-label test pattern for the reward label so regressions in **`_display_name`** wiring are caught. + +## Nits + +- Nit: **`_format_reward_summary_lines`** appends **`(no grants)`** when summary dict is present but all grant rows are filtered out (empty arrays or zero qty). Server delivery snapshots should always include at least one grant when **`completionRewardSummary`** is emitted; harmless defensive copy. + +- Nit: **`documentation_and_implementation_alignment.md`** E7.M2 references pipe lists NEO-124–NEO-130 plan links but not **`NEO-131-implementation-plan.md`** — row body already cites NEO-131; add link for parity with other landed slices. + +- Nit: Encounter loot (**NEO-110**) repaints on every **`completed`** snapshot; quest rewards intentionally use transition-only detection — correct per plan; keep the distinction in mind when NEO-132 capstone asserts end-to-end economy HUD refresh. + +## Verification + +```bash +# GdUnit (requires GODOT_BIN) +export GODOT_BIN=/path/to/godot # or pass --godot_binary +cd client +./addons/gdUnit4/runtest.sh -a test/quest_progress_client_test.gd -a test/quest_hud_controller_test.gd + +# GDScript lint (if .venv-gd installed) +gdlint client/scripts/quest_hud_controller.gd client/scripts/quest_progress_client.gd client/scripts/main.gd +``` + +Manual (per `docs/manual-qa/NEO-131.md`): + +1. Fresh server + Godot **F5** — reward label **`Quest rewards:` / `—`** below accept feedback. +2. Accept gather intro, complete 3× scrap — **`Salvage +25 XP`** on transition; progress label shows **`completed`**. +3. Godot restart (server still running) — reward label stays **`—`** (no replay). +4. Optional: operator chain completion — item + skill XP lines; optional server stop — progress **`sync error`** + reward **`—`**. + +Regression: NEO-122 quest progress/accept HUD and NEO-110 encounter loot label remain independent.