don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-07-NEO-131.md

5.5 KiB
Raw Blame History

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.
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.

  • 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

# 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.