# Code review — NEO-72 client inventory snapshot HUD

**Date:** 2026-05-24  
**Scope:** Branch `NEO-72-client-inventory-snapshot-hud` · commits `1273296`–`e7a61a1` vs `origin/main`  
**Base:** `origin/main`  
**Follow-up:** Review feedback addressed in `e7a61a1` (header types, tests, docs).

## Verdict

**Approve with nits** — blocking header-type bug fixed; integration tests now exercise handler payloads; remaining nits are deferred (HTTP 500 test, unused `inventorySlotKind` in HUD).

## 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). Follow-up commit **`e7a61a1`** corrects **`request_completed`** header typing so handlers run under real **`HTTPRequest`** and mock transports.

## 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) | **Matches** — E3.M3 note now includes NEO-72 client HUD (`e7a61a1`). |
| [`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; automated test scope updated (`e7a61a1`). |
| Full-stack epic decomposition | **Matches** — this is the E3S5-01 **client** counterpart; Godot manual QA present; does not claim Slice 5 capstone complete. |

## 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`**).~~ **Done.** `e7a61a1` — both clients use **`PackedStringArray`**; GdUnit suites pass with no conversion errors.

## 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.~~ **Done.** `e7a61a1` — inventory test captures snapshot and asserts 24 bag + 1 equipment slots.

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).~~ **Done.** `e7a61a1` — `test_display_name_for_after_request_sync`.

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).~~ **Done.** `e7a61a1`.

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.~~ **Done.** `e7a61a1`.

## 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.~~ **Done.** `e7a61a1` — removed duplicate parse tests.
- 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. **Deferred** — 404 + malformed JSON sufficient for merge.
- Nit: **`inventorySlotKind`** is cached but unused in HUD formatting (plan listed it as optional row prefix — fine to defer). **Deferred.**

## 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. **Verified** on `e7a61a1`.

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