don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-94: verify code review follow-ups and bump verdict to Approve

pull/132/head
VinPropane 2026-05-28 23:24:12 -04:00
parent 798c601bc5
commit 55964da4ac
1 changed files with 6 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
docs/reviews/2026-05-28-NEO-94.md
View File

@ -4,9 +4,11 @@
**Scope:** Branch `NEO-94-npc-runtime-snapshot-get` vs `origin/main` — full rewrite in `a68d8aa` (+ plan `5442910`) **Scope:** Branch `NEO-94-npc-runtime-snapshot-get` vs `origin/main` — full rewrite in `a68d8aa` (+ plan `5442910`)
**Base:** `origin/main` **Base:** `origin/main`
**Follow-up:** Suggestions addressed in `798c601`; re-verified 2026-05-28 (NEO-94 tests 4/4 pass).
## Verdict ## Verdict
**Approve with nits** **Approve**
## Summary ## Summary
@ -29,7 +31,7 @@ None.
## Suggestions ## Suggestions
1. ~~**Update `documentation_and_implementation_alignment.md`** — Append **NEO-94 landed** to the E5.M2 tracking row (snapshot GET, Bruno folder, plan/README links), matching NEO-90–NEO-93 entries. Listed in the plan “Files to modify” but not yet in the diff.~~ **Done.** 1. ~~**Update `documentation_and_implementation_alignment.md`** — Append **NEO-94 landed** to the E5.M2 tracking row (snapshot GET, Bruno folder, plan/README links), matching NEO-90–NEO-93 entries. Listed in the plan “Files to modify” but not yet in the diff.~~ **Done.** (`798c601`)
2. ~~**AAA layout in two integration tests** — `GetNpcRuntimeSnapshot_ShouldShowAggro_OnMeleeAfterDamagingCast` and `GetNpcRuntimeSnapshot_ShouldShowTelegraph_AfterAttackCooldownElapses` call **`ReadFromJsonAsync`** in **Act**. Move status/body reads to **Assert** to match **`CombatTargetsWorldApiTests`** and [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert): 2. ~~**AAA layout in two integration tests** — `GetNpcRuntimeSnapshot_ShouldShowAggro_OnMeleeAfterDamagingCast` and `GetNpcRuntimeSnapshot_ShouldShowTelegraph_AfterAttackCooldownElapses` call **`ReadFromJsonAsync`** in **Act**. Move status/body reads to **Assert** to match **`CombatTargetsWorldApiTests`** and [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert):
@ -39,12 +41,12 @@ None.
// Assert // Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.Equal(HttpStatusCode.OK, response.StatusCode);
var body = await response.Content.ReadFromJsonAsync<NpcRuntimeSnapshotResponse>(); var body = await response.Content.ReadFromJsonAsync<NpcRuntimeSnapshotResponse>();
```~~ **Done.** Aggro + telegraph tests fixed; decreasing-windup test also moves all **`ReadFromJsonAsync`** calls to **Assert**. ```~~ **Done.** Aggro + telegraph tests fixed; decreasing-windup test also moves all **`ReadFromJsonAsync`** calls to **Assert** (`798c601`).
## Nits ## Nits
- Nit: **`ActiveTelegraph`** / **`AggroHolderPlayerId`** could use **`[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]`** for leaner JSON (optional; explicit `null` is fine for client parsers). - Nit: **`ActiveTelegraph`** / **`AggroHolderPlayerId`** could use **`[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]`** for leaner JSON (optional; explicit `null` is fine for client parsers).
- Nit: **`GetNpcRuntimeSnapshot_ShouldDecreaseWindupRemaining_AcrossPolls`** captures **`firstBody`** during **Arrange** (baseline for comparison). Acceptable for a two-poll delta test; alternatively split into two facts for stricter AAA. - ~~Nit: **`GetNpcRuntimeSnapshot_ShouldDecreaseWindupRemaining_AcrossPolls`** captures **`firstBody`** during **Arrange** (baseline for comparison). Acceptable for a two-poll delta test; alternatively split into two facts for stricter AAA.~~ **Done.** **`ReadFromJsonAsync`** for both polls now in **Assert**; first poll GET remains in **Arrange** as setup (`798c601`).
## Verification ## Verification