74 lines
6.3 KiB
Markdown
74 lines
6.3 KiB
Markdown
# Code review — NEO-73 client gather feedback on interact
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-73-e3m1-client-gather-feedback-on-interact` · commits `4682db4`–`b52428e` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits** — implementation matches the adopted plan and E3S5-02 acceptance criteria; GdUnit suites pass. Non-blocking gaps: alignment register still lists NEO-73 as planned, and `_pending_gather_feedback` can stick on interact HTTP failures until a later inventory refresh.
|
||
|
||
## Summary
|
||
|
||
The branch adds **`skill_progression_client.gd`**, **`prototype_interactable_picker.gd`**, gather/skill HUD labels, **`interaction_result_received`** on **`interaction_request_client.gd`**, and orchestration in **`main.gd`** (nearest-node **R**, inventory delta on **`GatherFeedbackLabel`**, salvage row on **`SkillProgressionLabel`**, auto-refresh after successful gather). Four new GdUnit suites extend interaction and client coverage; manual QA and plan reconciliation are present. Design follows NEO-72/NEO-28 patterns (thin HTTP clients, `_busy` guard, authority config copy, HUD error lines). **`main.gd`** grows by ~150 lines but the picker and skill client are appropriately extracted.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-73-implementation-plan.md`](../plans/NEO-73-implementation-plan.md) | **Matches** — kickoff decisions (nearest **R**, inventory delta copy, dedicated skill label) reflected; acceptance checklist and reconciliation complete. |
|
||
| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) · **E3S5-02** | **Matches** — acceptance criteria checked; scope/out-of-scope honored. |
|
||
| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Partially matches** — cites NEO-73 as Slice 5 client backlog but no “landed” client note yet (plan §8 deferred to merge). |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E3.M1 row still says **Slice 5 client (planned): gather feedback HUD NEO-73**; should flip to **landed** on merge. E3.M3 row still lists NEO-73 under “remaining.” |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — client displays outcomes; gather resolution stays server-side via **`POST …/interact`**. |
|
||
| [`docs/manual-qa/NEO-73.md`](../manual-qa/NEO-73.md) | **Matches** — four-node **R** walkthrough, depletion deny, terminal does not overwrite gather line, no manual **I**. |
|
||
| [`client/README.md`](../../client/README.md) gather feedback section | **Matches** — endpoints, **R** nearest-node rule, HUD labels, boot hydrate. Automated-tests **Scope** bullet still omits NEO-73 suites (see Suggestions). |
|
||
| Full-stack epic decomposition | **Matches** — E3S5-02 **client** counterpart for server NEO-63; Godot manual QA present; does not claim Slice 5 capstone (NEO-75) complete. |
|
||
|
||
## Blocking issues
|
||
|
||
(none)
|
||
|
||
## Suggestions
|
||
|
||
1. **Update alignment register + E3.M1 module note on merge** — Plan §8 explicitly deferred [`documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) E3.M1 row and E3.M1 client slice note until implementation completes. Add a **NEO-73 landed** clause (gather HUD, **`skill_progression_client`**, four-node **R** picker) and move NEO-73 off E3.M3 “remaining Slice 5” list.
|
||
|
||
2. **Clear `_pending_gather_feedback` when interact HTTP fails** — `InteractionRequestClient` only emits **`interaction_result_received`** on parsed **200**; non-200 paths print and return. `main.gd` sets **`_pending_gather_feedback = true`** in **`_forward_interact_resource_nearest`** before POST. If the request fails (500, malformed body, transport error), the flag stays set and a later **`inventory_received`** (e.g. manual **I**) can run **`_finalize_pending_gather_feedback`** with a stale **`_gather_pre_scrap_qty`**, painting a spurious gather delta. Clear the flag on non-200 (new signal or connect to a failure hook) or reset in **`_on_interaction_result_for_gather`** only after a matching success/deny **200**.
|
||
|
||
3. **`client/README.md` automated tests Scope** — Add **`skill_progression_client.gd`**, **`prototype_interactable_picker.gd`**, and extended **`interaction_request_client_test.gd`** to the Scope bullet list (same discoverability fix as NEO-72).
|
||
|
||
4. **GdUnit: deny path for gather refresh** — `gather_feedback_refresh_test.gd` only asserts the allow → dual-refresh path. A deny mock asserting **zero** inventory/skill sync calls would lock the “no refresh on **`node_depleted`**” contract.
|
||
|
||
## Nits
|
||
|
||
- Nit: **`Gather: ok — inventory refresh failed`** (`main.gd` **`_on_inventory_sync_failed`**) reads as success; consider **`Gather: inventory refresh failed`** for clarity.
|
||
- Nit: Rapid **R** while a gather POST is in flight overwrites **`_gather_pre_scrap_qty`**; delta can be wrong if two responses complete out of order. Prototype-acceptable; last-wins POST coalescing already exists on the interaction client.
|
||
- Nit: **`KNOWN_RESOURCE_NODE_IDS`** in **`prototype_interactable_picker.gd`** duplicates the frozen four-node table — intentional catalog-empty fallback; keep in sync with **`E3_M1`** if ids ever change (they are frozen).
|
||
|
||
## Verification
|
||
|
||
GdUnit (verified on review):
|
||
|
||
```bash
|
||
cd client
|
||
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
|
||
-a res://test/skill_progression_client_test.gd \
|
||
-a res://test/gather_feedback_refresh_test.gd \
|
||
-a res://test/prototype_interactable_picker_test.gd \
|
||
-a res://test/interaction_request_client_test.gd
|
||
```
|
||
|
||
**14/14** passed; no `PackedStringArray`/`PackedByteArray` header conversion errors on **`skill_progression_client.gd`**.
|
||
|
||
Manual (required before merge — real **`HTTPRequest`** + four nodes):
|
||
|
||
1. `cd server/NeonSprawl.Server && dotnet run`
|
||
2. Godot **F5** — follow [`docs/manual-qa/NEO-73.md`](../manual-qa/NEO-73.md) (boot salvage row, **R** at each anchor without **I**, depletion deny, terminal **E** leaves gather label unchanged).
|
||
|
||
Optional lint (pre-push hook parity):
|
||
|
||
```bash
|
||
gdlint client/scripts client/test
|
||
gdformat --check client/scripts client/test
|
||
```
|