diff --git a/docs/reviews/2026-05-24-NEO-59.md b/docs/reviews/2026-05-24-NEO-59.md new file mode 100644 index 0000000..3c2f2f5 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-59.md @@ -0,0 +1,56 @@ +# Code review — NEO-59 resource-node definition registry + DI + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-59-e3m1-resource-node-definition-registry-di` · commits `4921f2c`–`01dec2b` vs `main` +**Base:** `main` + +## Verdict + +**Approve** + +## Summary + +NEO-59 adds `IResourceNodeDefinitionRegistry` backed by the startup-loaded `ResourceNodeCatalog` (NEO-58), following the NEO-52 `IItemDefinitionRegistry` pattern exactly. The registry exposes `TryGetDefinition`, `TryGetYield`, and `GetDefinitionsInIdOrder`; DI is wired inside the existing `AddResourceNodeCatalog` extension so no `Program.cs` change is needed. Nine focused AAA tests (null id, unknown id, known id with metadata, ordinal enumeration, loader-backed fixture, host DI resolution) all pass. Docs (implementation plan, alignment table, server README, E3M1 backlog) match the landed behavior. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-59-implementation-plan.md`](../plans/NEO-59-implementation-plan.md) | **Matches** — scope, acceptance checklist, technical approach, and file list align with the diff; all four checklist items marked complete. | +| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-03 acceptance items flipped with NEO-59 landed note. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-59 registry + plan link; "Still planned" updated to drop registry. | +| [`server/README.md`](../../server/README.md) | **Matches** — resource-node catalog section updated: registry guidance and "do not inject `ResourceNodeCatalog` in new game code" advisory added. | + +Register/tracking: alignment table updated; no further register change required. + +## Blocking issues + +None. + +## Suggestions + +None. + +## Nits + +- `GetDefinitionsInIdOrder()` iterates `.Keys` to build a sorted array then does a second dictionary lookup per key. Iterating key-value pairs directly is slightly more idiomatic and avoids the double lookup: + + ```csharp + return catalog.NodesById + .OrderBy(kv => kv.Key, StringComparer.Ordinal) + .Select(kv => kv.Value) + .ToList(); + ``` + + The current form matches `ItemDefinitionRegistry` exactly, so consistency may outweigh the change. No action required. + +- The null-id and unknown-id tests each build a fully populated `nodes`/`yields` dictionary even though the exercised code paths exit before those entries are touched. Consistent with `ItemDefinitionRegistryTests` — no action required. + +- No `docs/manual-qa/NEO-59.md` — consistent with plan (no user-facing HTTP endpoint in this ticket; NEO-60 covers that). + +## Verification + +```bash +dotnet test server/NeonSprawl.Server.Tests --filter "FullyQualifiedName~ResourceNodeDefinitionRegistry" +# Passed! Failed: 0, Passed: 9, Skipped: 0 +```