77 lines
7.0 KiB
Markdown
77 lines
7.0 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`
|
||
**Follow-up:** Review feedback addressed in `434226a` (interaction failure signal, docs, tests).
|
||
|
||
## Verdict
|
||
|
||
**Approve** — implementation matches the adopted plan and E3S5-02 acceptance criteria; GdUnit suites pass. Follow-up cleared pending-gather flag on interact HTTP failures and updated alignment/module docs.
|
||
|
||
## 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) | **Matches** — NEO-73 client slice **landed** note added. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-73 landed; NEO-73 removed from E3.M3 Slice 5 remaining list. |
|
||
| [`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 test Scope lists NEO-73 suites. |
|
||
| 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.~~ **Done.**
|
||
|
||
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**.~~ **Done.** **`interaction_request_failed`** signal + **`_on_interaction_request_failed_for_gather`** clears pending flag.
|
||
|
||
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).~~ **Done.**
|
||
|
||
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.~~ **Done.**
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: **`Gather: ok — inventory refresh failed`** (`main.gd` **`_on_inventory_sync_failed`**) reads as success; consider **`Gather: inventory refresh failed`** for clarity.~~ **Done.**
|
||
- 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.
|
||
- ~~**Bugbot:** Gather feedback flag orphaned when interaction coalesced (**E** overwrites pending **R** POST) — stale **`_pending_gather_feedback`** could paint spurious delta on manual **I**.~~ **Done.** Track **`_gather_pending_interactable_id`**; clear session only when gather never completed.
|
||
- ~~**Bugbot:** Coalesced POST clears gather feedback before inventory arrives — terminal result after successful **R** cleared pending before inventory GET returned, dropping **`GatherFeedbackLabel`** delta.~~ **Done.** **`_gather_awaiting_inventory_finalize`** preserves session until inventory sync paints delta.
|
||
- 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
|
||
```
|
||
|
||
**16/16** passed; no `PackedStringArray`/`PackedByteArray` header conversion errors on **`skill_progression_client.gd`** (includes deny + HTTP failure tests after follow-up).
|
||
|
||
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
|
||
```
|