69 lines
5.5 KiB
Markdown
69 lines
5.5 KiB
Markdown
# 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` | **Matches** — E7M2-08 / NEO-131 landed note in row body; references column includes [NEO-131 plan](../../plans/NEO-131-implementation-plan.md). |
|
||
| `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.~~ **Done.** `test_completion_transition_paints_item_grant_with_display_name` + **`StubItemDefsClient`** in `client/test/quest_hud_controller_test.gd`.
|
||
|
||
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.~~ **Done.** `test_definitions_ready_repaints_reward_label_with_display_name` in `client/test/quest_hud_controller_test.gd`.
|
||
|
||
## 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.~~ **Done.** References column now includes [NEO-131 plan](../../plans/NEO-131-implementation-plan.md).
|
||
|
||
- 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.
|