40 lines
2.9 KiB
Markdown
40 lines
2.9 KiB
Markdown
# 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`.
|