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