NEO-72: Add code review for client inventory snapshot HUD.
parent
8e44d81537
commit
15fcc09493
|
|
@ -0,0 +1,69 @@
|
|||
# Code review — NEO-72 client inventory snapshot HUD
|
||||
|
||||
**Date:** 2026-05-24
|
||||
**Scope:** Branch `NEO-72-client-inventory-snapshot-hud` · commits `1273296`–`8e44d81` vs `origin/main`
|
||||
**Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Request changes** — plan, docs, and HUD wiring align with E3S5-01, but both HTTP clients declare the wrong `request_completed` header type (`PackedByteArray` instead of repo-standard `PackedStringArray`), so callbacks fail at runtime and GdUnit integration tests only pass while logging Godot errors.
|
||||
|
||||
## Summary
|
||||
|
||||
The branch adds **`inventory_client.gd`** and **`item_definitions_client.gd`**, boot wiring and **`InventoryLabel`** rendering in **`main.gd`**, **`I` → `inventory_refresh`**, GdUnit suites, manual QA, and module/alignment doc updates. The design follows NEO-32/NEO-24 patterns (thin clients, `_busy` guard, authority config copy, HUD error line + **`push_warning`**). Documentation is thorough and matches the adopted kickoff decisions (defs cache, display names, manual refresh). The header-type mismatch in **`_on_request_completed`** is a correctness blocker: mock transports and real **`HTTPRequest`** emit **`PackedStringArray`** headers (see **`cooldown_snapshot_client.gd`**, **`hotbar_loadout_client.gd`**), so inventory sync and defs load never complete in practice.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Document | Result |
|
||||
|----------|--------|
|
||||
| [`docs/plans/NEO-72-implementation-plan.md`](../plans/NEO-72-implementation-plan.md) | **Matches** — acceptance checklist complete; reconciliation section present; kickoff decisions reflected in code. |
|
||||
| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) · **E3S5-01** | **Matches** — acceptance criteria checked; scope/out-of-scope honored. |
|
||||
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — NEO-72 client HUD landed note added. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row notes NEO-72; Slice 5 remaining stories cited. |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3 Slice 5 note lists NEO-72; **E3.M3 note** still ends at NEO-56 with no NEO-72 client HUD line (alignment table was updated; register note was not). |
|
||||
| [`docs/manual-qa/NEO-72.md`](../manual-qa/NEO-72.md) | **Matches** — boot empty, seed + refresh, server-down error, **`I`** refresh. |
|
||||
| [`client/README.md`](../../client/README.md) inventory HUD section | **Matches** — endpoints, bindings, refresh contract for NEO-73. |
|
||||
| Full-stack epic decomposition | **Matches** — this is the E3S5-01 **client** counterpart; Godot manual QA present; does not claim Slice 5 capstone complete. |
|
||||
|
||||
Register/tracking: alignment table update is sufficient for merge after the code fix; optional **E3.M3 note** bump in dependency register for parity with alignment row.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
1. **`request_completed` header parameter type** — In `client/scripts/inventory_client.gd` and `client/scripts/item_definitions_client.gd`, `_on_request_completed` declares `_headers: PackedByteArray`. Every other HTTP client in `client/scripts/` uses `_headers: PackedStringArray` (e.g. `cooldown_snapshot_client.gd`, `hotbar_loadout_client.gd`, `position_authority_client.gd`). Godot **`HTTPRequest.request_completed`** emits **`PackedStringArray`** headers. GdUnit mock transports match that. Running the new suites logs:
|
||||
|
||||
`ERROR: Error calling from signal 'request_completed' to callable: … Cannot convert argument 3 from PackedStringArray to PackedByteArray.`
|
||||
|
||||
The handler never runs, so **`inventory_received`** / **`definitions_ready`** do not fire and the HUD stays on **Loading…** (or never refreshes display names). **Fix:** change both handlers to `_headers: PackedStringArray` (body stays **`PackedByteArray`**).
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **Re-run integration tests after header fix** — Confirm signal assertions still pass *and* Godot Output has no `request_completed` conversion errors. Consider asserting payload contents in `test_request_sync_emits_inventory_received` (e.g. `bagSlots.size() == 24`) so a silent handler failure cannot pass.
|
||||
|
||||
2. **`display_name_for` coverage** — Add one GdUnit test that loads defs via mock transport then calls **`display_name_for("scrap_metal_bulk")`** on the client instance (plan implied lookup helper coverage; only static **`build_definitions_map`** is exercised today).
|
||||
|
||||
3. **`module_dependency_register.md` E3.M3 note** — Append a NEO-72 client HUD landed clause to match the alignment register row (non-blocking doc parity).
|
||||
|
||||
4. **`client/README.md` automated tests scope** — The “Scope” bullet list under **Automated tests** still omits the new inventory/defs clients; add them when convenient so discoverability matches NEO-72.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `item_definitions_client_test.gd` has overlapping parse tests (`test_parse_mock_transport_json_body`, `test_handler_body_bytes_parse_builds_map`) that duplicate **`test_build_definitions_map_extracts_display_name`**; consolidate when touching tests for the header fix.
|
||||
- Nit: Plan tests table mentions HTTP **500** for **`inventory_sync_failed`**; only **404** is covered — acceptable for prototype once happy + 404 + malformed JSON paths work.
|
||||
- Nit: **`inventorySlotKind`** is cached but unused in HUD formatting (plan listed it as optional row prefix — fine to defer).
|
||||
|
||||
## Verification
|
||||
|
||||
After fixing header types:
|
||||
|
||||
```bash
|
||||
cd client
|
||||
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
|
||||
-a res://test/inventory_client_test.gd -a res://test/item_definitions_client_test.gd
|
||||
```
|
||||
|
||||
Confirm **no** `Cannot convert argument 3 from PackedStringArray to PackedByteArray` in output.
|
||||
|
||||
Manual (required before merge — validates real **`HTTPRequest`** path):
|
||||
|
||||
1. `cd server/NeonSprawl.Server && dotnet run`
|
||||
2. Godot **F5** — follow [`docs/manual-qa/NEO-72.md`](../manual-qa/NEO-72.md) (boot empty bag, curl seed, **`I`** refresh with display name, server stopped → HUD error).
|
||||
Loading…
Reference in New Issue