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-25-NEO-26.md

2.9 KiB
Raw Permalink Blame History

Code Review — NEO-26 SelectionEvent

  • Date: 2026-04-25
  • Scope: Branch NEO-26-selectionevent-surface-debug-consumer; diff origin/main...HEAD (NEO-26 SelectionEvent surface + debug consumer)
  • Base: origin/main

Verdict

Approve with nits.

Summary

This branch adds a focused SelectionEvent surface in TargetSelectionClient and keeps it intentionally narrow to lockedTargetId transitions, with explicit cause attribution (tab, clear, server_correction). The implementation is coherent with the NEO-26 plan: cause tagging is set at request start, stale/failed paths clear the pending cause, and emit is suppressed for id-unchanged updates. Coverage in client/test/selection_event_client_test.gd exercises both positive and no-op rules, and the targeted suite passes locally. Risk is low and mostly around documentation tracking consistency rather than runtime behavior.

Documentation checked

  • docs/plans/NEO-26-implementation-plan.md — matches (signal shape, cause rules, lock-id-only emit rule, print consumer, QA artifact all implemented).
  • docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md — matches (implementation snapshot updated to include NEO-26 selection_event prototype).
  • docs/decomposition/modules/module_dependency_register.md — partially matches (E1.M3 note still says "SelectionEvent ... remain in progress"; …). Addressed: E1.M3 note now records NEO-26 prototype SelectionEvent and separates follow-on InteractableDescriptor / multi-consumer work.
  • docs/decomposition/modules/documentation_and_implementation_alignment.md — partially matches (E1.M3 row still says "SelectionEvent ... still open"; …). Addressed: E1.M3 snapshot row records NEO-26 landed vs follow-on scope.

Implementation tracking update needed after merge: update the E1.M3 row wording in documentation_and_implementation_alignment.md (and optionally the E1.M3 note in module_dependency_register.md) so the docs reflect that prototype SelectionEvent now exists while richer downstream consumers remain open. Done.

Blocking issues

None.

Suggestions

  1. Update the E1.M3 tracking row in docs/decomposition/modules/documentation_and_implementation_alignment.md to explicitly record NEO-26 as landed, replacing the blanket "SelectionEvent still open" phrasing with "prototype landed; richer consumers follow-on." Done.

Nits

  • Nit: Consider adding one assertion for sequence in client/test/selection_event_client_test.gd to lock payload stability for downstream subscribers. Done. (sequence asserted on tab, clear, GET id-change, and validity-only cases.)

Verification

  • godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/selection_event_client_test.gd (from client/) — passed (5 test cases, 0 failures).
  • Manual checks documented in docs/manual-qa/NEO-26.md.