don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-04-24-NEO-25.md

4.7 KiB
Raw Blame History

Code review — NEO-25 (InteractableDescriptor list / fetch-driven client)

Date: 2026-04-24

Scope: Branch NEO-25-e1m3-interactable-descriptor-list vs main (full story: server GET /game/world/interactables, registry + two rows, Godot catalog + world build + dual keys, tests, Bruno, docs).

Base: main (inferred from git diff main).

Follow-up: Suggestions below addressed in-repo (decomposition alignment, catalog-failure warning, GdUnit public + last-wins tests).

Verdict

Approve with nits

Summary

The branch delivers the NEO-25 plan end-to-end: versioned GET /game/world/interactables with stable ascending ids, PrototypeInteractableRegistry extended with kind and a second anchor clear of the scene obstacle, integration tests for GET and per-id POST …/interact range behavior, Godot fetch-driven props and multi-row glow preview, interact / interact_secondary wired to fixed ids with main.gd forwarding keys through _unhandled_key_input (reasonable fix for embedded dock focus), and last-wins queuing on the interaction client when POST overlaps. Overall risk is low for a prototype spike; server authority for interact outcomes is unchanged. All 65 dotnet test solutions tests pass in this environment.

Documentation checked

Path Assessment
docs/plans/NEO-25-implementation-plan.md Matches — acceptance criteria, route shape, anchors, client option A, tests, Bruno, manual QA path align with the diff.
docs/manual-qa/NEO-25.md Matches — steps match implemented behavior and anchors.
docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md Matches — implementation snapshot updated to cite GET /game/world/interactables and client scripts.
docs/decomposition/modules/client_server_authority.md Matches (spot-check) — client preview + server POST …/interact authority preserved.
docs/decomposition/modules/contracts.md N/A for blocking — JSON v1 spike discipline unchanged; no contradiction.
docs/decomposition/modules/module_dependency_register.md Matches — E1.M3 note now includes NEO-25 partial (GET /game/world/interactables, DTOs/API, Godot catalog + builder); SelectionEvent / richer descriptor consumers called out as still in progress. (Was Partially matches at review time; register updated in follow-up.)
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E1.M3 snapshot documents NEO-25 list projection + open SelectionEvent / richer consumers. (Was Partially matches at review time; tracking row updated in follow-up.)

Blocking issues

(none)

Suggestions

  1. Tracking table / register copy — Update documentation_and_implementation_alignment.md (E1.M3 row) and optionally module_dependency_register.md (E1.M3 note) to record NEO-25: versioned GET /game/world/interactables, InteractablesListDtos / InteractablesWorldApi, and Godot interactables_catalog_client.gd + interactable_world_builder.gd, clarifying that full SelectionEvent / richer descriptor consumers remain future work. Done. Both files updated (E1.M3 snapshot + E1.M3 note; server README anchor links).
  2. main.gd catalog failure handler — _on_interactables_catalog_failed is intentionally empty (errors already come from InteractablesCatalogClient). A one-line push_warning or comment pointing to README “server first” would make the no-op obvious to future readers. Done. push_warning with reason + pointer to client README section “Interaction + range preview (NEO-9 + NEO-25)”; comment notes catalog client already push_errors.

Nits

  • Nit: InteractableWorldBuilder uses extends Object and is invoked via load(...).call("build_from_catalog", …) from main.gd; a RefCounted or Node-attached helper would avoid load at runtime, but this is consistent enough for prototype scope.
  • Nit: GdUnit interaction_request_client_test.gd calls _post_interact directly; optionally add cases for post_interact_terminal / post_interact_resource public entrypoints and for last-wins _pending_interactable_id behavior if regressions become a concern. Done. Added test_post_interact_*_public_entrypoint and test_last_wins_second_press_before_first_completes (HoldFirstThenAutoTransport).

Verification

  • dotnet test /path/to/NeonSprawl.sln (from repo root) — expect all tests green.
  • Manual: follow docs/manual-qa/NEO-25.md — curl GET, F5 with server, E/R at both anchors, server-down degradation.
  • Optional: run GdUnit client suites if your CI does not already.

Review produced per code review agent.