NEO-29: apply code review follow-ups
Set E1.M4 documentation status to In Progress across decomposition tracking docs and add lower-bound slot validation coverage. Mark the corresponding review suggestions and nit as resolved in the saved review file.pull/54/head
parent
c4b71aac0d
commit
d25d7d0211
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E1.M4 |
|
||||
| **Epic** | [Epic 1 — Core Player Runtime](../epics/epic_01_core_player_runtime.md) |
|
||||
| **Stage target** | Prototype |
|
||||
| **Status** | Planned (see [dependency register](module_dependency_register.md)) |
|
||||
| **Status** | In Progress (see [dependency register](module_dependency_register.md)) |
|
||||
| **Linear** | Story slugs **E1M4-01** through **E1M4-05** in [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); label **`E1.M4`** per [decomposition README — Linear alignment](../README.md#linear-alignment) |
|
||||
|
||||
## Purpose
|
||||
|
|
|
|||
|
|
@ -49,6 +49,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not
|
|||
| E1.M1 | Ready | Prototype milestone **Done** ([E1.M1](https://linear.app/neon-sprawl/project/e1m1-inputandmovementruntime-20eb28dd3db4)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEO-8](../../plans/NEO-8-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEO-13](../../plans/NEO-13-implementation-plan.md)); Godot sync + path-follow ([NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEO-9](../../plans/NEO-9-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEO-6](../../plans/NEO-6-implementation-plan.md), [NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-8](../../plans/NEO-8-implementation-plan.md), [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-10](../../plans/NEO-10-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md), [NEO-13](../../plans/NEO-13-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) |
|
||||
| E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) |
|
||||
| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **NEO-27 landed:** Slice 3 telemetry hook sites documented — `selection_event` maps to product `target_changed` in `target_selection_client.gd`; server lock transition hook comments in `InMemoryPlayerTargetLockStore`; reserved `ability_cast_requested` / `ability_cast_denied` comments in `client/scripts/main.gd` (all TODO(E9.M1)); see [NEO-27](../../plans/NEO-27-implementation-plan.md) and [`docs/manual-qa/NEO-27.md`](../../manual-qa/NEO-27.md). **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`; production telemetry ingest/catalog after E9.M1. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) |
|
||||
| E1.M4 | In Progress | **NEO-29 landed:** prototype `HotbarLoadout` v1 server contract (`GET`/`POST /game/players/{id}/hotbar-loadout`) with stable deny reason codes, per-player scope, and persistence policy matching NEO-8/NS-17 (Postgres when configured, in-memory fallback otherwise). Client boot hydration is wired via `hotbar_loadout_client.gd` + `hotbar_state.gd` from `main.gd`; manual QA checklist created. **Still open:** cast request path, accept/deny integration UX, cooldown snapshot sync, and telemetry hooks from later E1.M4 stories. | [NEO-29](../../plans/NEO-29-implementation-plan.md); [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md); [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); `server/NeonSprawl.Server/Game/AbilityInput/`; [`client/scripts/hotbar_loadout_client.gd`](../../../client/scripts/hotbar_loadout_client.gd), [`client/scripts/hotbar_state.gd`](../../../client/scripts/hotbar_state.gd); [server README — Hotbar loadout](../../../server/README.md#hotbar-loadout-neo-29) |
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -15,13 +15,13 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
| E1.M1 | InputAndMovementRuntime | None | MoveCommand, PositionState, InteractionRequest | Prototype | Ready |
|
||||
| E1.M2 | IsometricCameraController | E1.M1 | CameraState, ZoomBandConfig, OcclusionPolicy | Prototype | Ready |
|
||||
| E1.M3 | InteractionAndTargetingLayer | E1.M1 | TargetState, InteractableDescriptor, SelectionEvent | Prototype | In Progress |
|
||||
| E1.M4 | AbilityInputScaffold | E1.M3, E5.M1 | AbilityCastRequest, HotbarLoadout, CooldownSnapshot | Prototype | Planned |
|
||||
| E1.M4 | AbilityInputScaffold | E1.M3, E5.M1 | AbilityCastRequest, HotbarLoadout, CooldownSnapshot | Prototype | In Progress |
|
||||
|
||||
**E1.M2 note:** Prototype slice **Ready**: `client/scripts/isometric_follow_camera.gd` + child `Camera3D` on **`World/IsometricFollowCamera`** in `client/scenes/main.tscn`; per-tick **`CameraState`** (`client/scripts/camera_state.gd`); **`ZoomBandConfig`** ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); **`OcclusionPolicy`** ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); pick-through **`"occluder"`** convention ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); consumer contract + occluder lifecycle hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). See [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md).
|
||||
|
||||
**E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **Partial (NEO-23):** versioned JSON v1 **`PlayerTargetStateResponse`** + **`POST …/target/select`** under `server/NeonSprawl.Server/Game/Targeting/` ([NEO-23](../../plans/NEO-23-implementation-plan.md); [server README — Targeting](../../../server/README.md#targeting-neo-23)) — throwaway HTTP spike per [contracts.md](contracts.md). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **NEO-26 (prototype `SelectionEvent`):** Godot **`TargetSelectionClient.selection_event`** on **`lockedTargetId`** changes ([NEO-26](../../plans/NEO-26-implementation-plan.md)). **Follow-on / in progress:** richer multi-consumer **`InteractableDescriptor`** targeting on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`).
|
||||
|
||||
**E1.M4 note:** Vertical-slice decomposition is defined in [E1M4-prototype-backlog.md](../../plans/E1M4-prototype-backlog.md) with story slugs **E1M4-01** through **E1M4-05** (loadout contract, cast path, accept/deny UX, cooldown sync, telemetry hooks). Module remains **Planned** until implementation stories begin; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md) for scope and backlog links.
|
||||
**E1.M4 note:** Vertical-slice decomposition is defined in [E1M4-prototype-backlog.md](../../plans/E1M4-prototype-backlog.md) with story slugs **E1M4-01** through **E1M4-05** (loadout contract, cast path, accept/deny UX, cooldown sync, telemetry hooks). **NEO-29** started implementation with `HotbarLoadout` v1 server APIs + persistence wiring and client hydration, so module status is **In Progress**; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md) and [NEO-29 plan](../../plans/NEO-29-implementation-plan.md).
|
||||
|
||||
### Epic 2 — Skills and Progression Framework
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,44 @@
|
|||
# Code review — NEO-29
|
||||
|
||||
- **Date:** 2026-04-25
|
||||
- **Scope:** Branch `NEO-29-hotbarloadout-v1-contract-baseline-persistence-path` (`origin/main...HEAD`)
|
||||
- **Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
Approve with nits.
|
||||
|
||||
## Summary
|
||||
|
||||
This branch delivers the planned `HotbarLoadout` v1 vertical slice end-to-end: server `GET`/`POST` APIs with stable deny reason codes, persistence selection (Postgres when configured, in-memory fallback otherwise), client hydration wiring, and targeted tests/manual QA artifacts. The API behavior and persistence wiring align with the NEO-29 plan decisions (per-player scope, fixed eight slots, server-owned validation). Contract safety is good for a prototype slice: deny responses preserve authoritative loadout snapshots, and request validation covers out-of-bounds, unknown abilities, and duplicate slots. Overall runtime risk is low; most residual risk is documentation-state drift rather than code behavior.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
- `docs/plans/NEO-29-implementation-plan.md` — **matches** (scope, API shape, persistence policy, deny reasons, tests/docs artifacts all present).
|
||||
- `docs/plans/E1M4-prototype-backlog.md` — **matches** (E1M4-01 slice framing and NEO-29 implementation note are consistent).
|
||||
- `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` — **partially matches** (implementation snapshot updated, but summary `Status` still `Planned` despite landed implementation).
|
||||
- `docs/decomposition/modules/module_dependency_register.md` — **partially matches** (E1.M4 row remains `Planned`; this story appears to satisfy “work started,” which should move to `In Progress` per alignment policy).
|
||||
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **conflicts** with current status labels (rule says move module status from `Planned` to `In Progress` when implementation stories merge).
|
||||
- `docs/decomposition/modules/contracts.md` — **matches** for prototype scope (JSON/HTTP spike is acceptable for early slice; versioned envelope and stable reason codes are documented).
|
||||
- `docs/decomposition/modules/client_server_authority.md` — **matches** (server remains authority for persistent gameplay-relevant loadout state; client acts as view/sync).
|
||||
|
||||
Register/tracking follow-up needed after merge: update E1.M4 status in `module_dependency_register.md`, `E1_M4_AbilityInputScaffold.md` summary table, and the implementation tracking table entry expectations.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. ~~Update decomposition status tracking for E1.M4 from `Planned` to `In Progress` in the register/module summary (and corresponding tracking inventory) so docs comply with `documentation_and_implementation_alignment.md` and do not under-report shipped progress.~~ Done. Updated `docs/decomposition/modules/module_dependency_register.md` (row + note), `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` (summary status), and `docs/decomposition/modules/documentation_and_implementation_alignment.md` (new E1.M4 tracking row).
|
||||
|
||||
## Nits
|
||||
|
||||
- ~~Nit: Consider adding one API test for `slotIndex < 0` deny parity with the existing `slotIndex == 8` test, to lock the lower-bound contract explicitly.~~ Done. Added `PostHotbarLoadout_ShouldDenyNegativeSlot_WithReasonCode` in `server/NeonSprawl.Server.Tests/Game/AbilityInput/HotbarLoadoutApiTests.cs`.
|
||||
|
||||
## Verification
|
||||
|
||||
- `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutApiTests"`
|
||||
- `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutPersistenceIntegrationTests"`
|
||||
- `godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_loadout_client_test.gd`
|
||||
- Manual checklist: `docs/manual-qa/NEO-29.md`
|
||||
|
|
@ -78,6 +78,27 @@ public sealed class HotbarLoadoutApiTests
|
|||
Assert.Equal(HotbarLoadoutApi.ReasonSlotOutOfBounds, body.ReasonCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task PostHotbarLoadout_ShouldDenyNegativeSlot_WithReasonCode()
|
||||
{
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var client = factory.CreateClient();
|
||||
|
||||
var post = await client.PostAsJsonAsync(
|
||||
"/game/players/dev-local-1/hotbar-loadout",
|
||||
new HotbarLoadoutUpdateRequest
|
||||
{
|
||||
SchemaVersion = HotbarLoadoutUpdateRequest.CurrentSchemaVersion,
|
||||
Slots = [new HotbarSlotBindingJson { SlotIndex = -1, AbilityId = PrototypeAbilityRegistry.PrototypePulse }],
|
||||
});
|
||||
Assert.Equal(HttpStatusCode.OK, post.StatusCode);
|
||||
|
||||
var body = await post.Content.ReadFromJsonAsync<HotbarLoadoutUpdateResponse>();
|
||||
Assert.NotNull(body);
|
||||
Assert.False(body!.Updated);
|
||||
Assert.Equal(HotbarLoadoutApi.ReasonSlotOutOfBounds, body.ReasonCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task PostHotbarLoadout_ShouldDenyUnknownAbility_WithReasonCode()
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in New Issue