55 lines
5.5 KiB
Markdown
55 lines
5.5 KiB
Markdown
# Code review — NEO-142 (E7M3-10)
|
|
|
|
**Date:** 2026-06-17
|
|
**Scope:** Branch `NEO-142-client-faction-standing-gate-hud` — commits `fb9a8ab` … `e396228` vs `main`
|
|
**Base:** `main`
|
|
|
|
## Verdict
|
|
|
|
**Approve with nits**
|
|
|
|
## Summary
|
|
|
|
This branch lands **E7M3-10**: Godot client faction standing HUD and readable **`faction_gate_blocked`** accept feedback, wired through existing quest HUD patterns (NEO-122 / NEO-131). New **`faction_standing_client.gd`** mirrors **`skill_progression_client.gd`** (GET parse, pending-sync coalesce, 404 handling). **`quest_hud_controller.gd`** renders **`FactionStandingLabel`**, refreshes standing on boot + in-session completion transitions, formats **`reputationGrants`** reward lines, and builds readable gate deny copy from cached **`factionGateRules`** + last standing snapshot. **`quest_definitions_client.gd`** adds **`faction_gate_rules_for`**. GdUnit coverage spans client parse/sync, HUD render/deny/refresh, and operator-chain rep reward line. Docs: implementation plan, **`client/README.md`** faction subsection, **`docs/manual-qa/NEO-142.md`**. The branch also bundles **NEO-141 story-end cleanup** (remove **`docs/manual-qa/NEO-141.md`**, plan/README wording) and a small unrelated **`RewardRouterOperations`** collection-expression refactor. **24** targeted GdUnit tests pass locally. Low merge risk — read-only client projections; no server contract changes beyond doc/chore.
|
|
|
|
## Documentation checked
|
|
|
|
| Path | Result |
|
|
|------|--------|
|
|
| `docs/plans/NEO-142-implementation-plan.md` | **Matches** — kickoff decisions, file list, technical approach, AC checklist, and **Implementation reconciliation (shipped)** align with the diff; PR bundled-commit note added. |
|
|
| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-10 scope bullets match; E7M3-10 AC checked + landed note. |
|
|
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — module intent and contracts satisfied; Status line notes **E7M3-10 landed (NEO-142)**; **E7M3-11** capstone pending. |
|
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row includes NEO-142 client HUD landed note + README/manual QA links. |
|
|
| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc / alignment table. |
|
|
| `docs/manual-qa/NEO-142.md` | **Matches** — component checklist, honest gate-deny testing note, standing-label refresh lag note on step 4. |
|
|
| `client/README.md` | **Matches** — **Faction standing + gate feedback HUD (NEO-142)** subsection documents GET endpoints, label placement, refresh triggers, and error pattern. Does not yet include **End-to-end faction reputation loop** capstone (E7M3-11 / NEO-143 scope per backlog). |
|
|
| `docs/plans/NEO-141-implementation-plan.md` | **Matches** — bundled story-end manual QA removal consistent with NEO-140 precedent. |
|
|
|
|
## Blocking issues
|
|
|
|
None.
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**Update decomposition tracking when NEO-142 closes.** After manual QA, extend **`E7_M3_FactionReputationLedger.md`** Status and **`documentation_and_implementation_alignment.md`** E7.M3 row with **E7M3-10 landed (NEO-142)** — client faction standing GET + HUD + gate deny + rep reward lines; link **`client/README.md`** faction subsection and **`docs/manual-qa/NEO-142.md`**.~~ **Done.**
|
|
|
|
2. ~~**Reconcile plan AC at story end.** Check off **`docs/plans/NEO-142-implementation-plan.md`** acceptance criteria and E7M3-10 backlog AC after Godot manual verification; mark **Implementation reconciliation** complete (same pattern as NEO-140/141 reviews).~~ **Done.**
|
|
|
|
3. ~~**PR description should call bundled commits.** If opening one PR from this branch, mention NEO-142 HUD work plus NEO-141 manual-QA removal and the **`RewardRouterOperations`** collection-expression chore so reviewers are not surprised by server diff noise.~~ **Done.** Noted in plan **PR notes (bundled branch)** section.
|
|
|
|
## Nits
|
|
|
|
- ~~Nit: **`quest_hud_controller.gd`** **`_faction_display_name`** probes an instance **`display_name_for`** on **`FactionStandingClient`**, but the script exposes **`display_name_for`** as **`static`** only — the instance branch is likely dead; fallback static call is correct either way.~~ **Done.** **`_faction_display_name`** calls **`FactionStandingClient.display_name_for`** directly.
|
|
- Nit: **`_title_case_token`** is duplicated in **`faction_standing_client.gd`** and **`quest_hud_controller.gd`** — acceptable for prototype; consolidate only if a shared helper already exists elsewhere.
|
|
- ~~Nit: No dedicated GdUnit for **`quest_definitions_client.faction_gate_rules_for`** — gate rules are exercised indirectly via **`faction_standing_hud_test.gd`**; optional direct parse test if the helper grows.~~ **Done.** **`quest_definitions_client_test.gd`** adds **`faction_gate_rules_for`** coverage.
|
|
- ~~Nit: Brief standing-label staleness on the same frame as completion (reward label shows **`+15 rep`** before faction GET returns) is adopted plan behavior; manual QA step 4 should confirm label catches up within one round-trip.~~ **Done.** Step 4 notes one GET round-trip lag.
|
|
|
|
## Verification
|
|
|
|
```bash
|
|
cd /Users/don/neon-sprawl/client && godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
|
|
-a test/faction_standing_client_test.gd -a test/faction_standing_hud_test.gd -a test/quest_reward_hud_test.gd
|
|
```
|
|
|
|
Manual verification: **`docs/manual-qa/NEO-142.md`** — fresh server + Godot **F5**; operator chain → **`Grid Operators: 15`** + reward rep line; optional dev fixture for live **`faction_gate_blocked`** deny copy.
|