From 15fcc09493dcfbf5851d3ee9c019f4f0199a40f7 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 18:42:40 -0400 Subject: [PATCH] NEO-72: Add code review for client inventory snapshot HUD. --- docs/reviews/2026-05-24-NEO-72.md | 69 +++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-72.md diff --git a/docs/reviews/2026-05-24-NEO-72.md b/docs/reviews/2026-05-24-NEO-72.md new file mode 100644 index 0000000..3024376 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-72.md @@ -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).