NEO-74: add code review for client craft UI branch.
parent
c16b610468
commit
9b6abf093b
|
|
@ -0,0 +1,56 @@
|
|||
# Code review — NEO-74 client craft UI + recipe list
|
||||
|
||||
**Date:** 2026-05-24
|
||||
**Scope:** Branch `NEO-74-e3m2-client-craft-ui-recipe-list` · commits `dff58ad`–`c16b610` vs `origin/main`
|
||||
**Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits** — implementation matches the adopted plan and E3S5-03 acceptance criteria; all nine new GdUnit cases pass headless. Remaining items are non-blocking (test `.uid` companions, optional recipe-sync failure HUD).
|
||||
|
||||
## Summary
|
||||
|
||||
The branch adds **`recipe_definitions_client.gd`**, **`craft_client.gd`**, and **`craft_recipe_panel.gd`**, wires them from **`main.gd`** with **`CraftFeedbackLabel`**, a scrollable eight-recipe panel, **refine** row on **`SkillProgressionLabel`**, and post-success inventory + skill refresh (deny leaves bag unchanged). Three GdUnit suites cover POST payload/parse, recipe GET parse, and refresh policy. Manual QA, plan reconciliation, E3.M2 module note, alignment register, and E3S5 backlog checkboxes are updated. Design follows NEO-72/73/31 patterns (thin HTTP clients, `_busy` guard, injectable transport, authority config copy, dedicated feedback label). HTTP handlers correctly use **`PackedStringArray`** headers (NEO-72 lesson applied).
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Document | Result |
|
||||
|----------|--------|
|
||||
| [`docs/plans/NEO-74-implementation-plan.md`](../plans/NEO-74-implementation-plan.md) | **Matches** — kickoff decisions (POST **`outputsGranted`** copy, dedicated **`CraftFeedbackLabel`**, left scroll panel, qty **1**, deny refresh policy) reflected; acceptance checklist and reconciliation complete. |
|
||||
| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) · **E3S5-03** | **Matches** — acceptance criteria checked; scope/out-of-scope honored; does not claim capstone (NEO-75) complete. |
|
||||
| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — NEO-74 client slice **landed** note added. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row notes NEO-74 landed; NEO-75 cited as remaining Slice 5 client work. |
|
||||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — client sends craft **intent** via POST; inventory/skill display refreshed from server GET after success. |
|
||||
| [`docs/manual-qa/NEO-74.md`](../manual-qa/NEO-74.md) | **Matches** — deny/success/refine XP walkthrough; optional **`inventory_full`** spot-check. |
|
||||
| [`client/README.md`](../../client/README.md) craft UI section | **Matches** — endpoints, HUD labels, refresh policy; automated test Scope lists NEO-74 suites. |
|
||||
| Full-stack epic decomposition | **Matches** — E3S5-03 **client** counterpart for server NEO-68/NEO-70; Godot manual QA present. |
|
||||
|
||||
## Blocking issues
|
||||
|
||||
(none)
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **Wire `recipes_sync_failed` in `main.gd`** — `RecipeDefinitionsClient` emits **`recipes_sync_failed`** (tested in GdUnit) but **`main.gd`** never connects it. On boot failure the recipe panel stays empty with no HUD line (inventory/skill clients show errors). Connect to **`CraftFeedbackLabel`** or **`push_warning`** + a one-line error for parity with **`inventory_sync_failed`** / **`progression_sync_failed`**.
|
||||
|
||||
2. **Add `.gd.uid` companions for new GdUnit test scripts** — `craft_client_test.gd`, `recipe_definitions_client_test.gd`, and `craft_feedback_refresh_test.gd` have no tracked **`*.gd.uid`** files (NEO-73 established this for new tests under `client/test/`). Generate in Godot and commit alongside the tests per [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md).
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`craft_recipe_panel.gd`** declares **`@onready var _scroll`** but only uses **`_vbox`** — remove unused binding or use **`_scroll`** for max-height policy.
|
||||
- Nit: **`craft_feedback_refresh_test.gd`** harness duplicates refresh logic from **`main.gd`** (intentional isolation) — acceptable; no **`main.gd`** integration test remains (same gap as NEO-73 gather refresh test).
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# Server (separate terminal)
|
||||
cd server/NeonSprawl.Server && dotnet run
|
||||
|
||||
# NEO-74 GdUnit suites (from client/)
|
||||
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
|
||||
-a res://test/craft_client_test.gd \
|
||||
-a res://test/recipe_definitions_client_test.gd \
|
||||
-a res://test/craft_feedback_refresh_test.gd
|
||||
```
|
||||
|
||||
Manual: follow [`docs/manual-qa/NEO-74.md`](../manual-qa/NEO-74.md) — deny **`insufficient_materials`**, **`refine_scrap_standard`** success, **`make_field_stim_mk0`**, **`refine`** XP delta without pressing **I** on success.
|
||||
Loading…
Reference in New Issue