74 lines
6.4 KiB
Markdown
74 lines
6.4 KiB
Markdown
# Review — NEO-24
|
|
|
|
Date: 2026-04-21
|
|
Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main...HEAD`)
|
|
Base: `origin/main` (merge-base `37f44a494400f4fdd369077c52dfc012df73a5d5`)
|
|
Follow-up: blocking issue + suggestions below are **done** (strikethrough + **Done.**). See the [NEO-24 plan](../plans/NEO-24-implementation-plan.md) Decision 3 for the signal change.
|
|
|
|
## Verdict
|
|
|
|
~~Request changes~~ — **Addressed.** Blocking issue fixed; integration test added; doc-alignment row updated.
|
|
|
|
## Summary
|
|
|
|
This branch adds the client-side target selection node, HUD label, input bindings, tests, and story docs for NEO-24. The overall direction matches the E1.M3 plan well: selection intent is POSTed to the server, the HUD paints only server-acknowledged target state, and denial handling is intentionally authoritative.
|
|
|
|
The main risk is that the advertised movement-driven soft-lock refresh is not actually wired to normal locomotion. The new unit suite passes, but it only proves `TargetSelectionClient` reacts correctly when its refresh hook is called directly; it does not cover the real signal behavior of `PositionAuthorityClient`, so the branch can merge with a broken end-to-end acceptance criterion.
|
|
|
|
## Documentation checked
|
|
|
|
- `docs/plans/NEO-24-implementation-plan.md` — **conflicts**. Decision 2 and the acceptance criteria require movement-triggered refresh while a lock is held, but the current wiring only refreshes on boot sync / rejection resync, not on ordinary movement.
|
|
- `docs/manual-qa/NEO-24.md` — **conflicts**. Section 4 expects walking out of range to flip HUD validity within ~250 ms during normal locomotion; that flow is not reachable with the current signal source.
|
|
- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** for server-authoritative target state and denial reconciliation.
|
|
- `docs/decomposition/modules/client_server_authority.md` — **matches** for client-as-intent / server-as-truth behavior.
|
|
- `docs/decomposition/modules/module_dependency_register.md` — **matches**. `E1.M3` remains `In Progress`.
|
|
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches**. The `E1.M3` tracking row still says client tab-target is open; if/when this story lands, that snapshot should be updated to reflect NEO-24 progress.
|
|
|
|
## Blocking issues
|
|
|
|
1. ~~The movement-driven refresh path described by the plan, README, and manual QA is not reachable during normal locomotion. `main.gd` connects `TargetSelectionClient.on_authoritative_position_snap()` to `PositionAuthorityClient.authoritative_position_received`, but `position_authority_client.gd` only emits that signal from `_emit_position_from_response()` during `sync_from_server()` and the move-rejection recovery path. Successful `move-stream` POSTs explicitly do not emit the signal, so walking around with a held lock never triggers the throttled `GET /target` refresh that NEO-24 depends on. As a result, the HUD can stay stuck at `Validity: ok` after the player walks out of range, which breaks the plan's Decision 2, the acceptance criterion for soft-lock refresh, and manual QA section 4.~~ **Done.** Added a separate `authoritative_ack(world)` signal on `PositionAuthorityClient` that fires on both `BOOT_GET` 200 and successful `move-stream` 200 (extracted a shared `_parse_world_from_response` helper). `authoritative_position_received` keeps its current snap-only semantics so rubber-band suppression is untouched. `TargetSelectionClient.on_authoritative_ack(world)` replaces `on_authoritative_position_snap`; cooldown + lock gate unchanged. `main.gd` connects to the new signal. See `client/scripts/position_authority_client.gd`, `target_selection_client.gd`, `main.gd`, and plan Decision 3.
|
|
|
|
Relevant behavior in `client/scripts/position_authority_client.gd`:
|
|
|
|
```text
|
|
match _phase:
|
|
Phase.BOOT_GET:
|
|
_busy = false
|
|
if response_code == 200:
|
|
_emit_position_from_response(text, true)
|
|
Phase.STREAM_POST:
|
|
...
|
|
if response_code == 200:
|
|
# Do not emit `authoritative_position_received` here.
|
|
pass
|
|
```
|
|
|
|
Meanwhile `client/scripts/target_selection_client.gd` only refreshes from that signal:
|
|
|
|
```text
|
|
func on_authoritative_position_snap(_world: Vector3, apply_as_snap: bool) -> void:
|
|
if not apply_as_snap:
|
|
return
|
|
if not _has_lock():
|
|
return
|
|
...
|
|
request_sync_from_server()
|
|
```
|
|
|
|
The fix needs to either emit a suitable movement/reconciliation signal during normal locomotion or hook the target refresh to another authoritative movement event that actually occurs while moving.
|
|
|
|
## Suggestions
|
|
|
|
1. ~~Add an integration-level test for the real movement/targeting wiring. The current `client/test/target_selection_client_test.gd` suite calls `on_authoritative_position_snap()` directly, which is why it misses the broken connection to `PositionAuthorityClient`. A focused test around `main.gd` wiring or `PositionAuthorityClient` signal behavior would prevent this regression.~~ **Done.** Added `client/test/target_refresh_on_locomotion_test.gd`: wires a real `PositionAuthorityClient` and `TargetSelectionClient` with mock transports and the same `authoritative_ack` → `on_authoritative_ack` connection `main.gd` uses. Verifies (a) a `move-stream` 200 with a held lock produces exactly one throttled `GET /target` and the cached state flips to `out_of_range`, and (b) the same path is a no-op when no lock is held. Also added `test_sync_boot_get_200_emits_authoritative_ack` and `test_move_stream_post_200_emits_authoritative_ack` to `position_authority_client_test.gd` so renames on either side will break the per-script suites too.
|
|
2. ~~Update `docs/decomposition/modules/documentation_and_implementation_alignment.md` if this story lands. Its `E1.M3` snapshot still says client tab-target is open, which will be stale after merge.~~ **Done.** E1.M3 row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) with links to the plan and manual QA file.
|
|
|
|
## Nits
|
|
|
|
None.
|
|
|
|
## Verification
|
|
|
|
- Ran `godot --headless --import --path . --quit-after 10` from `client/`.
|
|
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed.
|
|
- I did not run the full manual client/server checklist, but `docs/manual-qa/NEO-24.md` section 4 is the critical end-to-end check to rerun after fixing the movement refresh trigger.
|