neon-sprawl/docs/plans/NEO-24-implementation-plan.md

# NEO-24 — Implementation plan

## Story reference

| Field | Value |
|--------|--------|
| **Key** | NEO-24 |
| **Title** | E1.M3: Client tab-target + lock UI synced to server |
| **Linear** | [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24/e1m3-client-tab-target-lock-ui-synced-to-server) |
| **Slug** | E1M3-02 |
| **Git branch** | `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` |
| **Parent context** | [Epic 1 — Core Player Runtime](https://linear.app/neon-sprawl/project/epic-1-core-player-runtime-client-controls-character-loop-66bd590cd016) · [E1M3 prototype backlog](E1M3-prototype-backlog.md) |
| **Depends on** | [NEO-23](NEO-23-implementation-plan.md) (targeting HTTP v1) |
| **Decomposition** | [E1.M3 — InteractionAndTargetingLayer](../decomposition/modules/E1_M3_InteractionAndTargetingLayer.md); authority notes: [client_server_authority — E1.M3](../decomposition/modules/client_server_authority.md#e1m3-interactionandtargetinglayer) |

## Goal, scope, and out-of-scope

**Goal:** **Tab** (or agreed input action) cycles **server-eligible** stub targets in a **documented deterministic order**; **HUD / debug overlay** reflects the last **server-acknowledged** `TargetState` (`lockedTargetId`, `validity`, `sequence`). Client sends **selection intent** via existing **`POST …/target/select`**; **optional optimistic** presentation **must** reconcile to the **authoritative** `targetState` in each **200** response (including denials), per E1.M3 authority doc.

**In scope**

- Godot: input → **`TargetSelectRequest`** v1 → parse **`TargetSelectResponse`** / **`PlayerTargetStateResponse`**; maintain **last acknowledged** target state for UI.
- **Tab cycle order:** match server stub contract — lexicographic ascending id over the **prototype registry ids** (`prototype_target_alpha`, then `prototype_target_beta`), wrapping from last → first; same ordering as [server README — Targeting](../../server/README.md#targeting-neo-23) / `PrototypeTargetRegistry` comments.
- **Clear lock:** **`POST`** with **`targetId`** omitted or JSON **`null`** (NEO-23 rule); binding documented in `client/project.godot` (proposed: **Esc** via `target_clear`).
- **UI:** readable label (or small overlay) showing **server** lock id (or **none** / empty), **`validity`**, and optionally **`sequence`** for debugging.
- **Soft lock / movement:** when the player moves, **`validity`** can become **`out_of_range`** without clearing **`lockedTargetId`**. Client must **refresh** authoritative state periodically or on hooks so the overlay stays truthful (see Technical approach).
- **Shared constants:** duplicate **stub target id list + order** in a small GDScript constants module aligned with `PrototypeTargetRegistry` (NEO-23 plan expectation for NEO-24 preview/cycle).

**Out of scope (per Linear / backlog)**

- Nearest-in-cone auto-target; sticky soft-target under latency.
- **`AbilityCastRequest`** / hotbar — [E1.M4](../decomposition/modules/E1_M4_AbilityInputScaffold.md).
- Server route or DTO changes (covered by NEO-23); **no** new ASP.NET endpoints unless a client bug reveals a contract gap (then narrow fix + plan update).

## Acceptance criteria checklist

- [x] With **≥2** stub targets, **tab** advances selection intent in **ascending id** order and **wraps** at the end of the ordered list. (`PrototypeTargetConstants.next_id_after` + `test_tab_from_no_lock_selects_first_id` / `test_tab_from_alpha_requests_beta` / `test_tab_wraps_from_beta_to_alpha`.)
- [x] After each successful **round-trip**, UI shows **`lockedTargetId`** (or **`—`** for none) and **`validity`** from the **response body** (`targetState` on POST, body on GET). (`TargetSelectionClient._extract_target_state_fields` + `main.gd:_on_target_state_changed`; `test_sync_get_200_emits_state`.)
- [x] On **`selectionApplied`: `false`**, UI shows **authoritative** `targetState` from the same response (no “stuck optimistic” lock id that the server denied). (`_update_state_from_post` replaces cached state from `targetState` on every 200, success or deny; `test_denial_reflects_authoritative_target_state`.)
- [x] **Clear** action clears lock server-side and UI updates from the response. (`request_clear_target` omits `targetId`; `test_clear_issues_post_without_target_id`.)
- [x] Input bindings for **tab cycle** and **clear** are documented in **`client/project.godot`** and in **`client/README.md`** (new “Target lock + tab cycle (NEO-24)” section).

Server-up manual QA steps live in the README section above; run the server + client to exercise the soft-lock out-of-range flow end to end.

## Technical approach

1. **`prototype_target_constants.gd` (or rename if clearer):** export ordered `Array[String]` of stub ids matching `PrototypeTargetRegistry` (`alpha`, `beta`) for tab math only; single source for “eligible cycle list” on the client.

2. **`target_selection_client.gd`:** Node analogous to `interaction_request_client.gd` / `position_authority_client.gd`: owns injectable HTTP transport, `base_url`, `dev_player_id`, `_busy` guard. Responsibilities:
   - **`request_sync_from_server()`** — `GET …/target` → parse `PlayerTargetStateResponse` v1 → cache + emit signal.
   - **`request_select_target_id(target_id: String)`** — `POST …/target/select` with body `{"schemaVersion":1,"targetId":…}`.
   - **`request_clear_target()`** — `POST` with no `targetId` / `null` per NEO-23.
   - **`request_tab_next()`** — read cached `lockedTargetId` + `validity`; compute **next** id in the ordered list (if no lock, choose **first** id in order); POST select; on deny, signal carries **server** `targetState` only (no client-only lock).
   - Signal e.g. **`target_state_changed(state: Dictionary)`** or typed fields for consumers (`lockedTargetId` Variant string-or-null, `validity` string, `sequence` int, `selection_applied` bool optional for last POST).
   - **Refresh policy (hybrid, movement-triggered — Decision 2):** no periodic polling. Issue **`GET …/target`** when (a) boot sync runs, (b) any `POST …/target/select` completes (covered by response body already — no extra GET needed), and (c) **while a lock is currently held**, `PositionAuthorityClient` emits a new **`authoritative_ack(world)`** signal — fired on **every** server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200). A small **cooldown** (e.g. **250 ms**) coalesces bursts so many consecutive `move-stream` acks cannot produce a GET per ack. No timer otherwise. Rationale: stays honest about `out_of_range` without duplicating radius math on the client or holding an always-on polling timer. Trade-off: will not pick up purely server-driven state changes (none exist in NEO-23; revisit if combat/PvP adds them). **Why `authoritative_ack` instead of `authoritative_position_received`:** the latter only fires on boot / move-rejection resync — normal `move-stream` 200s are intentionally silent to avoid RTT rubber-banding — so hooking the target refresh to it leaves the soft-lock path unreachable during WASD locomotion (blocking issue raised in [2026-04-21-NEO-24 review](../reviews/2026-04-21-NEO-24.md)). The new signal carries only the world position (no `apply_as_snap` flag) and is emitted after every authoritative position regardless of whether the client snaps.

3. **Optimistic highlight (optional slice):** If implemented, keep **pending** selection separate from **acknowledged** state; on **200** denial, drop pending and paint from **`targetState`**. If timeboxed, **v1** may ship **ack-only** UI (still satisfies “optional optimistic” — none used).

4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. **Also** connect `PositionAuthorityClient.authoritative_ack` → `target_selection_client.on_authoritative_ack(...)` (the handler is a no-op when no lock is held, so boot is naturally skipped; see step 2 refresh policy).

5. **`project.godot`:** `target_tab` → **Tab**; `target_clear` → **Escape** (locked — Decision 1).

6. **Manual QA:** Run server + client; tab between stubs; walk out of radius and confirm **`validity`** flips to **`out_of_range`** on overlay while id remains; clear with Esc.

## Files to add

| Path | Purpose |
|------|---------|
| `client/scripts/prototype_target_constants.gd` | Ordered stub **`targetId`** list + helpers for “next in cycle” matching server registry. |
| `client/scripts/target_selection_client.gd` | HTTP GET/POST targeting APIs; tab/clear entrypoints; throttled refresh; signals for UI. |
| `client/test/target_selection_test_double.gd` | Test subclass injecting mock HTTP (pattern from `position_authority_test_double.gd`). |
| `client/test/target_selection_client_test.gd` | GdUnit4: tab order wrap, POST success path, denial keeps server `lockedTargetId`, clear POST, GET parse. |

## Files to modify

| Path | Rationale |
|------|-----------|
| `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. Post-merge follow-up (Decision 4) also adds `World/PrototypeTargetMarkers` (colored mast + translucent radius ring per anchor). |
| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path; connect `authoritative_ack` → `on_authoritative_ack` (see Decision 2). Re-render `TargetLockLabel` each physics tick with live per-anchor distance lines from `PrototypeTargetConstants` (Decision 4). |
| `client/scripts/prototype_target_constants.gd` | Display-only mirror of `PrototypeTargetRegistry.cs` anchors + radii used by the markers and HUD distance readouts (Decision 4). |
| `client/scripts/prototype_target_markers.gd` | Spawns a visible mast + flat ring at every `PrototypeTargetConstants.ANCHORS` entry so players can reason about range visually (Decision 4). |
| `client/scripts/position_authority_client.gd` | Add `authoritative_ack(world)` signal and emit it from both `BOOT_GET` 200 and `STREAM_POST` 200 so held locks re-validate during normal WASD locomotion without re-introducing snap rubber-banding. Extracted `_parse_world_from_response(...)` helper to avoid duplicating JSON parse across the two paths. |
| `client/project.godot` | Register **`target_tab`** and **`target_clear`** input actions with defaults above. |
| `client/README.md` | One short subsection: tab-target prototype, bindings, and how to read the lock overlay (parity with interaction/move docs). |

## Tests

| Test file | What to cover |
|-----------|----------------|
| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`; **movement-triggered refresh:** calling `on_authoritative_ack(...)` while a lock is held fires a GET, while **no** lock is held fires **nothing**, and two back-to-back acks within cooldown produce **one** GET. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. |
| `client/test/position_authority_client_test.gd` | Extend the existing suite to assert that `authoritative_ack` is emitted on both `BOOT_GET` 200 and successful `move-stream` 200 (existing no-snap-on-stream-200 assertion stays). |
| `client/test/target_refresh_on_locomotion_test.gd` | **Integration** (added during review follow-up): wires a real `PositionAuthorityClient` + `TargetSelectionClient` with mock transports and the same signal wiring `main.gd` uses; asserts that with a lock held, a `move-stream` 200 produces exactly one throttled refresh `GET` on the target client, and that the same path is a no-op with no lock. Catches renames / mis-wires between the two scripts that the unit suites miss. |

No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests already live under `bruno/neon-sprawl-server/targeting/`; **no** new `.bru` required unless implementation discovers a **server** contract change (then add per repo [testing expectations](../../.cursor/rules/testing-expectations.md) for HTTP contract changes).

## Open questions / risks

- **Scene visuals:** This story does **not** require 3D reticles on stub meshes; overlay text satisfies AC. World-space highlights can be a follow-up.
- **Server-driven state changes:** The hybrid refresh policy (Decision 2) **will not** pick up purely server-driven flips to `invalid_target` or future auto-clears — none exist in NEO-23, but if combat/PvP (E5.M1 / E6.M1) adds them later, revisit (either add a narrow server push or reintroduce a throttled poll).

## Decisions

| # | Topic | Choice |
|---|-------|--------|
| 1 | **Input bindings** | `target_tab` → **Tab**; `target_clear` → **Escape**. Registered in `client/project.godot`. |
| 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_ack` (new signal — fires on every server-confirmed position, including `move-stream` 200) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). |
| 3 | **Signal for the refresh hook** | Review follow-up (see [2026-04-21-NEO-24 review](../reviews/2026-04-21-NEO-24.md)): the initial wiring hooked `authoritative_position_received`, which only fires on boot / rejection resync — normal `move-stream` 200s are intentionally silent to avoid RTT rubber-banding. A new `authoritative_ack(world)` signal was added to `PositionAuthorityClient` and emitted from both 200 paths so the movement-driven refresh is actually reachable during WASD. The snap signal kept its current semantics; the ack signal is pure heartbeat. |
| 4 | **Post-merge UX: anchor visibility** | Users reported "targeting feels inconsistent" because the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have no visible meshes — the only visible console in the scene is the NEO-9 `PrototypeTerminal` at origin, which is unrelated to targeting. Added `client/scripts/prototype_target_markers.gd` + `PrototypeTargetMarkers` node in `main.tscn` (colored mast + translucent flat ring per anchor) and extended `TargetLockLabel` with per-anchor `<id>: <d> m / <radius> (in|out)` distance lines. Anchors/radii are mirrored in `prototype_target_constants.gd` as **display-only** data — server remains authoritative. If `PrototypeTargetRegistry.cs` moves, update both files in the same commit. |
| 5 | **Post-merge UX: position-drift race** | Follow-up report: "when in range of beta, pressing Tab says denied." Root cause is a race between `move-stream` 20 Hz sampling and the target-select POST — a Tab press right after you stop moving is validated against the last sampled position, which can trail the visible capsule by several meters. Two changes: (a) `PlayerPositionLabel` now renders a `srv: (x,y,z) Δ=<d>m age=<ms>` line from `PositionAuthorityClient.authoritative_ack`, making the server/client divergence directly visible; (b) `TargetSelectionClient.set_freshness_kick(authority, player)` (wired in `main.gd`) has the target client call `authority.submit_stream_targets([player.global_position])` immediately before every `POST /target/select`, so the server's stored snapshot is as fresh as possible when the range check runs. The freshness kick does not *wait* on the stream 200 — both requests race on separate HTTP connections — but it shrinks the window enough in practice that "stop, Tab, denied" stops firing. Both pieces are pure client changes; the server contract (NEO-23) is untouched. |
| 6 | **Post-merge UX: range-aware Tab cycle** | Follow-up report: "tab here does not acquire target" while standing 2.41 m from beta (inside 4 m) with `Δ=0` — server and client agreed on position but Tab still denied. Root cause: with no lock held, `request_tab_next()` returned `ORDERED_IDS[0]` = alpha (17.90 m away), not the anchor the user was visibly standing next to. Added `PrototypeTargetConstants.next_in_range_id_after(current, world)` which walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; `TargetSelectionClient.request_tab_next()` prefers that pick when the player ref is wired (via the existing `set_freshness_kick` path) and falls back to `next_id_after(current)` when nothing is in range so the server still owns the denial reason. Pure client-side UX improvement; server contract untouched. The picker also **skips the currently-locked id** so Tab always expresses a swap intent — if the only in-range target is what you already have, the fallback picks the next cycle id and lets the server deny (preserves the NEO-23 soft-lock "Tab = visible denial when no swap possible" rule). |
| 7 | **Prototype target geometry: shared radius + overlap** | Follow-up request: make all targetable objects share a single radius and make the rings overlap in the test scene so Tab-flipping is observable without locomotion. Introduced `PrototypeTargetRegistry.SharedLockRadius = 6.0` and moved anchors to **alpha `(-3, 0.5, -3)`** and **beta `(3, 0, 3)`** (distance ~8.49 m ⇒ ~3.5 m-wide overlap centered at origin). Spawn `(-5, -5)` keeps alpha in range (~2.83 m) and beta out (~11.31 m) so the existing manual QA "Tab-to-out-of-range = soft-lock denial" path still holds; walking to the origin puts both rings in range and Tab cleanly flips alpha ↔ beta. `client/scripts/prototype_target_constants.gd` mirrors the new values as display-only data (+ a `SHARED_LOCK_RADIUS` constant to match the server). Keep the two in lock-step; NEO-23 reader tests updated to the new boundary (`(3, -3)` for alpha radius 6). Per-target radii will return with real combat design (E5.M1); this is a prototype stub only. |
| 8 | **Post-merge UX: `positionHint` on `target/select` (NEO-23 contract extension)** | Follow-up report: "targeted A, moved out of A range and into B range — Tab gets denied incorrectly" at `Δ=0`, `age≈8 s`. Root cause: the freshness kick from Decision 5 kicks a `move-stream` POST *and* the select POST on separate HTTP connections — nothing guarantees the server processes the stream first, so the select's range check can still run against the stale stored snap. Fix: extend `TargetSelectRequest` v1 with an **optional** `positionHint: {x,y,z}`; when present, the server uses it for the radius check and for the `validity` field in the response (both the soft-lock and applied-lock paths). The hint is purely advisory — it does **not** write to `IPositionStateStore` (`move-stream` stays the only write path) and remains trust-on-client for the prototype; E5.M1 combat will bound the hint against the stored snap / movement budget before shipping to players. Client wires the hint automatically when `set_freshness_kick(...)` is called (same player ref), so existing headless unit tests that skip that wiring continue to omit the hint. The freshness kick is kept in place to refresh the stored snap for subsequent validity GETs, but is no longer load-bearing for denial correctness. |