# 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`.