NEO-30: address code review follow-ups
parent
deec789af3
commit
8a9a9bcf78
|
|
@ -21,6 +21,7 @@ body:json {
|
|||
|
||||
tests {
|
||||
test("status 200", function () {
|
||||
// NEO-30 review follow-up: cast JSON contract unchanged (server comments only).
|
||||
expect(res.getStatus()).to.equal(200);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -21,7 +21,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
|
||||
**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). **NEO-29** started implementation with `HotbarLoadout` v1 server APIs + persistence wiring and client hydration; **NEO-31** added the prototype cast POST + client digit-key path and resolver tests — module status remains **In Progress**; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md), [NEO-29 plan](../../plans/NEO-29-implementation-plan.md), and [NEO-31 plan](../../plans/NEO-31-implementation-plan.md).
|
||||
**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; **NEO-31** added the prototype cast POST + client digit-key path and resolver tests; **NEO-28** added server target lock + range gates and client cast feedback UX; **NEO-30** documented Epic 1 Slice 3 cast-funnel telemetry hook sites (comment-only, `AbilityCastApi`) — module status remains **In Progress**; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md), [NEO-29 plan](../../plans/NEO-29-implementation-plan.md), [NEO-31 plan](../../plans/NEO-31-implementation-plan.md), [NEO-28 plan](../../plans/NEO-28-implementation-plan.md), and [NEO-30 plan](../../plans/NEO-30-implementation-plan.md).
|
||||
|
||||
### Epic 2 — Skills and Progression Framework
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,8 @@
|
|||
|
||||
**Base:** `origin/main`
|
||||
|
||||
**Follow-up:** Suggestions and nits below are **done** (strikethrough + **Done.**); register row and XML doc tweaks landed in the same change-set.
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve**
|
||||
|
|
@ -21,12 +23,12 @@ The branch delivers NEO-30 as agreed: **comment-only** telemetry hook sites on t
|
|||
| `docs/plans/NEO-30-implementation-plan.md` | **Matches** — kickoff decisions (comment-only server hooks, no new server logging), file list, acceptance criteria, and technical approach align with the diff. |
|
||||
| `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` | **Matches** — snapshot and E1M4-05 row cite NEO-30, hook anchors, and E9.M1 deferral. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E1.M4 row documents NEO-30 telemetry hook sites. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E1.M4 remains **In Progress** (correct); the long **E1.M4 note** still summarizes NEO-29/31/28 but does not mention NEO-30 (optional discoverability; not a plan requirement). |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** (post-review) — E1.M4 note now includes NEO-28 + NEO-30 with plan links. |
|
||||
| `docs/decomposition/modules/E9_M1_TelemetryEventSchema.md` | **N/A** — deferred per plan; `TODO(E9.M1)` in code matches. |
|
||||
| `docs/decomposition/modules/client_server_authority.md` / `contracts.md` | **Matches** — no change to authority or wire contracts; comments reinforce server as authoritative deny/reason source. |
|
||||
| `docs/manual-qa/NEO-30.md` | **Matches** — concrete checks for comments, 400/404 exclusion, and shared NEO-28/31 vocabulary. |
|
||||
|
||||
**Register / tracking table:** No mandatory row edit beyond what landed in `documentation_and_implementation_alignment.md`; optional follow-up is a one-line NEO-30 mention in the register’s E1.M4 narrative note for consistency.
|
||||
**Register / tracking table:** E1.M4 narrative note updated to include NEO-30 (and NEO-28) with plan links — see **Suggestions** resolution above.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
|
|
@ -34,13 +36,13 @@ The branch delivers NEO-30 as agreed: **comment-only** telemetry hook sites on t
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Module dependency register (optional):** Add **NEO-30** to the **E1.M4 note** paragraph in `docs/decomposition/modules/module_dependency_register.md` so readers who start from the register see hook-site documentation alongside NEO-29/31/28 — mirrors the alignment table and avoids “only in alignment doc” drift.
|
||||
1. ~~**Module dependency register (optional):** Add **NEO-30** to the **E1.M4 note** paragraph in `docs/decomposition/modules/module_dependency_register.md` so readers who start from the register see hook-site documentation alongside NEO-29/31/28 — mirrors the alignment table and avoids “only in alignment doc” drift.~~ **Done.** E1.M4 note now includes **NEO-28** and **NEO-30** with plan links (`module_dependency_register.md`).
|
||||
|
||||
## Nits
|
||||
|
||||
1. **Nit:** `AbilityCastApi` class `<remarks>` uses `<c>AbilityCastResponse.reasonCode</c>` (JSON casing). C# readers may expect `ReasonCode`; harmless for intent.
|
||||
1. ~~**Nit:** `AbilityCastApi` class `<remarks>` uses `<c>AbilityCastResponse.reasonCode</c>` (JSON casing). C# readers may expect `ReasonCode`; harmless for intent.~~ **Done.** Remarks now use `<see cref="AbilityCastResponse.ReasonCode"/>` and note wire JSON `reasonCode` (`AbilityCastApi.cs`).
|
||||
|
||||
2. **Nit:** `AbilityCastApiTests` `<see cref="AbilityCastApi.MapAbilityCastApi"/>` targets an extension method; some doc generators are picky. If XML doc warnings appear in CI later, switch to a fully qualified `cref` or drop the `see` to the containing type only.
|
||||
2. ~~**Nit:** `AbilityCastApiTests` `<see cref="AbilityCastApi.MapAbilityCastApi"/>` targets an extension method; some doc generators are picky. If XML doc warnings appear in CI later, switch to a fully qualified `cref` or drop the `see` to the containing type only.~~ **Done.** `<see>` targets `AbilityCastApi` only; extension name in plain `<c>MapAbilityCastApi</c>` (`AbilityCastApiTests.cs`).
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ namespace NeonSprawl.Server.Tests.Game.AbilityInput;
|
|||
/// <remarks>
|
||||
/// NEO-30: every JSON deny (<see cref="AbilityCastResponse.Accepted"/> <c>false</c>) asserts a non-whitespace
|
||||
/// <see cref="AbilityCastResponse.ReasonCode"/> — matches Slice 3 <c>ability_cast_denied</c> payload expectations;
|
||||
/// authoritative hook-site comments live on <see cref="AbilityCastApi.MapAbilityCastApi"/>.
|
||||
/// authoritative hook-site comments live on <see cref="AbilityCastApi"/> (cast route: <c>MapAbilityCastApi</c> extension in the same source file).
|
||||
/// </remarks>
|
||||
public sealed class AbilityCastApiTests
|
||||
{
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ namespace NeonSprawl.Server.Game.AbilityInput;
|
|||
/// <summary>Maps prototype ability cast POST (NEO-31 + NEO-28 target authority).</summary>
|
||||
/// <remarks>
|
||||
/// Epic 1 Slice 3 product telemetry names <c>ability_cast_requested</c> and <c>ability_cast_denied</c>
|
||||
/// (see <c>AbilityCastResponse.reasonCode</c> on denies) — hook sites are marked inline below.
|
||||
/// (on JSON denies, <see cref="AbilityCastResponse.ReasonCode"/> — wire JSON <c>reasonCode</c>) — hook sites are marked inline below.
|
||||
/// TODO(E9.M1): emit cataloged events (payload: route player id, slotIndex, abilityId, targetId; on deny include reasonCode).
|
||||
/// HTTP 400/404 here are outside the v1 cast deny contract (no JSON <c>AbilityCastResponse</c>).
|
||||
/// </remarks>
|
||||
|
|
|
|||
Loading…
Reference in New Issue