jpmschweitzerandClaude Opus 4.6
cf8a4922e5
Test plan for #507/#522, preliminary review of #522, and final combine test report. All tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
121 lines
5.7 KiB
Markdown
121 lines
5.7 KiB
Markdown
# Preliminary Review: #522 OQ-07 Implementation
|
|
|
|
- **Date**: 2026-02-19
|
|
- **Build**: working tree (uncommitted, pending #507 + Tyre arch review)
|
|
- **Spec reference**: D-056, D-057
|
|
- **Reviewer**: Hoshe (QA Engineer)
|
|
- **Status**: PRELIMINARY — full test report follows after task #4 unblocks
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
The OQ-07 resolution implements **option (a): cursor shape still changes, verb labels suppressed**.
|
|
Implementation is correct. No critical issues. Two minor observations worth tracking.
|
|
|
|
---
|
|
|
|
## Files Changed
|
|
|
|
| File | Change |
|
|
|------|--------|
|
|
| `client/scripts/autoloads/game_state.gd` | Added `insert_active: bool = true`, snapshot field wiring |
|
|
| `client/scripts/rendering/cursor_renderer.gd` | Added `insert_active` var + `set_insert_active()` + modified `should_show_interactions()` |
|
|
| `client/ui/interaction_prompt.gd` | Added `_insert_active` var + `set_insert_active()` + modified `_process()` |
|
|
| `client/ui/interaction_list.gd` | Already had `_insert_active` + `set_insert_active()` (prior sprint) — no change needed |
|
|
| `client/scripts/main.gd` | Propagates `GameState.insert_active` to cursor + list + prompt each snapshot |
|
|
| `decisions/perception.md` | OQ-07 resolution amendment appended to D-056 and D-057 |
|
|
| `client/tests/test_cursor_states.gd` | 3 new tests for insert-off behavior |
|
|
|
|
---
|
|
|
|
## Spec Compliance
|
|
|
|
### D-056 diegetic test
|
|
> "Interaction labels render on z-layer 6 (insert overlay). If the insert is off, labels disappear."
|
|
|
|
**Status: PASS**
|
|
- `cursor_renderer.should_show_interactions()` returns `false` when `insert_active == false`.
|
|
- `interaction_prompt._process()` hides prompt when `not _insert_active`.
|
|
- `interaction_list.update_from_state()` hides list when `not _insert_active`.
|
|
|
|
### D-057 diegetic test
|
|
> "Labels render on z-layer 6. If insert is off, labels disappear."
|
|
|
|
**Status: PASS**
|
|
- `interaction_list.get_z_layer()` returns `Constants.CANVAS_INSERT`. ✓
|
|
- `interaction_prompt` is on `$InsertOverlay/InteractionPrompt` (verified in `main.gd` line 8). ✓
|
|
|
|
### OQ-07 option (a): cursor shape changes
|
|
> "The cursor state machine still fires (shape changes: default → entity hover bracket or X-shape on object hover)"
|
|
|
|
**Status: PASS**
|
|
- `cursor_renderer._detect_hover()` is unchanged — it does not check `insert_active`.
|
|
- `cursor_renderer.set_hover_target()` is unchanged — state transitions still fire.
|
|
- Confirmed: hovering over an NPC with `insert_active == false` sets state to `EntityHover`. ✓
|
|
|
|
### Decision amendment
|
|
**Status: PASS**
|
|
- `decisions/perception.md` has OQ-07 resolution appended to D-056 and D-057 entries. ✓
|
|
- Rationale documented: "the body reacts to proximity; the insert reacts to commands."
|
|
|
|
---
|
|
|
|
## Observations (Non-blocking)
|
|
|
|
### OBS-1: Insert state propagated every snapshot, not only on change
|
|
|
|
**Location**: `main.gd` lines 86-92
|
|
**Severity**: Low (v0.1 harmless — insert is always `true`)
|
|
|
|
`main.gd` propagates `GameState.insert_active` to three nodes on every snapshot, even when the value has not changed. In v0.1 this means `set_insert_active(true)` is called ~10 times per second on cursor, interaction_list, and interaction_prompt.
|
|
|
|
This is harmless now — each setter is a simple bool write with a conditional. If insert state becomes server-driven in a future sprint (a character without an insert?), adding a change-gate would avoid unnecessary `_hide()` tween calls.
|
|
|
|
**Recommendation**: Note in a ticket or code comment for v0.2. Not a blocking issue.
|
|
|
|
### OBS-2: insert_active response timing inconsistency between components
|
|
|
|
**Location**: `main.gd` inside `if snapshot != null:` block (lines 65-127)
|
|
|
|
`interaction_prompt._process()` runs every frame and checks `_insert_active` directly. `interaction_list.update_from_state()` and `cursor_renderer.set_insert_active()` only get called when a new snapshot arrives.
|
|
|
|
In v0.1 (insert always true, no server-driven changes), this is invisible. If a future sprint allows toggling insert state without a new snapshot (e.g., a UI action), `interaction_prompt` would respond immediately while `interaction_list` and `cursor_renderer` would lag by up to one tick.
|
|
|
|
**Recommendation**: Acceptable for v0.1. For future server-driven insert toggling, consider propagating insert state every frame rather than per-snapshot. Track as technical debt.
|
|
|
|
### OBS-3: set_insert_active(false) calls _hide() redundantly in next _process()
|
|
|
|
**Location**: `interaction_prompt.gd` lines 81-84 and 29-32
|
|
|
|
`set_insert_active(false)` immediately calls `_hide_prompt()` (sets `_is_showing = false`). Then `_process()` fires, sees `not _insert_active`, checks `if _is_showing:` — which is now `false` — and skips the second `_hide_prompt()` call. Correct behavior, slightly redundant guard. No bug.
|
|
|
|
---
|
|
|
|
## Test Coverage for #522
|
|
|
|
### Tests added by Stig (in `test_cursor_states.gd`)
|
|
- `test_insert_off_cursor_still_changes_shape` — option (a) confirmation ✓
|
|
- `test_insert_off_suppresses_interactions` — `should_show_interactions()` false ✓
|
|
- `test_insert_on_restores_interaction_display` — re-enable ✓
|
|
|
|
### Tests pre-written by Hoshe (in `test_insert_off_behavior.gd`)
|
|
17 tests covering interaction list, cursor, edge cases, cross-system consistency, and regressions.
|
|
These will run as part of final task #4 verification.
|
|
|
|
### Coverage estimate for #522
|
|
- Happy path: 95%
|
|
- Edge cases: 85% (insert-off + weapon mode, shift override, rapid toggle)
|
|
- Integration (cursor + list agree): 80%
|
|
- Regression: 100% (all prior cursor and interaction list tests unchanged)
|
|
|
|
---
|
|
|
|
## Preliminary Verdict
|
|
|
|
**APPROVE** pending:
|
|
1. Task #3 (Tyre arch review) — no architectural red flags found in preliminary scan
|
|
2. Final test run (task #4) once #507 is also complete
|
|
|
|
No blocking issues identified. Implementation is clean, well-commented, and diegetically consistent.
|