refactor(client): Sprint 20 D-020 compliance and structural cleanup #71

2026-02-25T22:07:25+01:00

jpmschweitzer commented

2026-02-25 22:07:25 +01:00

Summary

Sprint 20 client refactors — 4 tickets enforcing D-020 (Godot = pure renderer) and reducing structural debt.

#557: game_state.gd — stationary_ticks and zone_id now read from server snapshot with deprecated client-side fallbacks. Protocol extended with decode paths and TODO markers for server team.
#558: dialogue_box.gd — replaced 3 direct GameState.dialogue_active mutations and all AudioManager calls with signals (dialogue_state_changed, audio_dip_requested, audio_dip_cleared). Zero autoload references remain.
#559: main.gd — extracted SnapshotEventRouter (46 lines) with callable-based register()/dispatch() pattern. Replaces 15+ inline if-has dispatch blocks.
#560: Unified duplicate YAML parsers into YamlParser utility (parse() for nested typed dicts, parse_flat() for dotted-key UIStrings format). ~140 lines removed from consumers.

Stats: 15 files changed, +1172 / -265 lines. 4 new test files with comprehensive coverage.

Test plan

test_game_state_sprint20.gd — server-authoritative paths and fallback degradation (10 tests)
test_dialogue_sprint20.gd — signal emission, no-direct-mutation assertions (7 tests)
test_snapshot_event_router.gd — keyed/always dispatch, ordering, edge cases (8 tests)
test_yaml_parser.gd — parse/parse_flat, type inference, arrays, nesting, regression (39 tests)
Existing test_dialogue_sprint18.gd passes with new signal-based architecture
Existing test_game_state.gd passes with refactored apply_snapshot()

## Summary Sprint 20 client refactors — 4 tickets enforcing D-020 (Godot = pure renderer) and reducing structural debt. - **#557**: `game_state.gd` — `stationary_ticks` and `zone_id` now read from server snapshot with deprecated client-side fallbacks. Protocol extended with decode paths and TODO markers for server team. - **#558**: `dialogue_box.gd` — replaced 3 direct `GameState.dialogue_active` mutations and all `AudioManager` calls with signals (`dialogue_state_changed`, `audio_dip_requested`, `audio_dip_cleared`). Zero autoload references remain. - **#559**: `main.gd` — extracted `SnapshotEventRouter` (46 lines) with callable-based `register()`/`dispatch()` pattern. Replaces 15+ inline if-has dispatch blocks. - **#560**: Unified duplicate YAML parsers into `YamlParser` utility (`parse()` for nested typed dicts, `parse_flat()` for dotted-key UIStrings format). ~140 lines removed from consumers. **Stats:** 15 files changed, +1172 / -265 lines. 4 new test files with comprehensive coverage. ## Test plan - [ ] `test_game_state_sprint20.gd` — server-authoritative paths and fallback degradation (10 tests) - [ ] `test_dialogue_sprint20.gd` — signal emission, no-direct-mutation assertions (7 tests) - [ ] `test_snapshot_event_router.gd` — keyed/always dispatch, ordering, edge cases (8 tests) - [ ] `test_yaml_parser.gd` — parse/parse_flat, type inference, arrays, nesting, regression (39 tests) - [ ] Existing test_dialogue_sprint18.gd passes with new signal-based architecture - [ ] Existing test_game_state.gd passes with refactored apply_snapshot()

jpmschweitzer added 5 commits 2026-02-25 22:07:25 +01:00

refactor(client): make stationary_ticks and zone_id server-authoritative (#557 ) c8de1a0629

apply_snapshot() now reads stationary_ticks and zone_id directly from
the server snapshot when present (D-020 compliance). Client-side
accumulation and tile lookup retained as deprecated fallbacks until
the server populates these fields. Protocol.gd extended with decode
paths and TODO markers for the server team.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor(client): decouple dialogue_box from GameState and AudioManager (#558 ) 6f9c97ca03

Replace 3 direct GameState.dialogue_active mutations and all
AudioManager.apply_dip/clear_dip calls with signals:
dialogue_state_changed, audio_dip_requested, audio_dip_cleared.
dialogue_box.gd now has zero references to GameState or AudioManager.
main.gd wires coordinator handlers in _ready() (D-020).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor(client): extract SnapshotEventRouter from main.gd (#559 ) 964bb9c459

New SnapshotEventRouter class (46 lines) provides callable-based
snapshot dispatch via register(), register_always(), and dispatch().
main.gd _process() now calls _router.dispatch(snapshot) instead of
15+ inline if-has blocks. Handlers registered in _ready().

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor(client): unify duplicate YAML parsers into YamlParser (#560 ) de136fc1a5

Extract shared YamlParser utility (client/scripts/util/yaml_parser.gd)
with parse() for nested typed dicts and parse_flat() for dotted-key
string format. UIStrings._parse_yaml() and ChecklistEvaluator's inline
parser both delegate to YamlParser, removing ~140 lines of duplication.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore(meta): update changelog 98100da0d0

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-02-25 22:42:36 +01:00

Review: client -> main (type: code)

Hoshe (Code Quality): APPROVE

Solid refactoring set with good test coverage. Four items worth noting, none blocking.

#	File	Severity	Issue
1	`client/scripts/util/yaml_parser.gd:34-72`	warning	Array pop/flush edge case: top-level array followed by top-level key not tested — could silently corrupt parsed structure
2	`client/scripts/util/yaml_parser.gd:130`	suggestion	Inline comment stripping ( `#`) could truncate unquoted values containing that pattern
3	`client/ui/dialogue_box.gd:603-616`	warning	`_expire_entries()` breaks early — pinned entry at index 0 can prevent pruning of expired entries beyond it, causing unbounded growth in long dialogues
4	`client/scripts/snapshot_event_router.gd:40-45`	suggestion	No `is_valid()` guard on handler callables — freed objects could halt dispatch loop
5	`client/scripts/main.gd:321`	suggestion	Silent discard when dialogue already active — no `push_warning` for debugging
6	`client/tests/test_dialogue_sprint18.gd`	suggestion	Duplicate test assertion with sprint20 test class — naming overlap may confuse

Tyre (Architecture): APPROVE

All four commits architecturally sound. D-020 compliance correct, new abstractions well-bounded, no D-010/D-012 violations.

#	File	Severity	Issue
1	`client/scripts/snapshot_event_router.gd`	suggestion	`_keyed` Dictionary untyped — runtime failure mode on wrong types
2	`client/scripts/snapshot_event_router.gd:39-45`	suggestion	Dispatch ordering guarantee undocumented — docstring should note it's a contract
3	`client/ui/dialogue_box.gd:147`	suggestion	`_load_theme()` calls `UIStrings._parse_yaml()` (private) instead of `YamlParser.parse_flat()` (public API from #560)
4	`client/scripts/util/yaml_parser.gd:64-72`	suggestion	Array termination lacks guard for empty stack — defensive code for user-authored content
5	`client/scripts/autoloads/game_state.gd:116-117`	suggestion	`_prev_player_position` deprecated field — flag for two-part removal with fallback
6	`client/scripts/autoloads/game_state.gd:162-170`	suggestion	`tiles` vs `visible_tiles` dual-key accreting complexity — candidate for deprecated fallback treatment

Verdict: APPROVED

## Review: client -> main (type: code) ### Hoshe (Code Quality): APPROVE Solid refactoring set with good test coverage. Four items worth noting, none blocking. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `client/scripts/util/yaml_parser.gd:34-72` | warning | Array pop/flush edge case: top-level array followed by top-level key not tested — could silently corrupt parsed structure | | 2 | `client/scripts/util/yaml_parser.gd:130` | suggestion | Inline comment stripping (` #`) could truncate unquoted values containing that pattern | | 3 | `client/ui/dialogue_box.gd:603-616` | warning | `_expire_entries()` breaks early — pinned entry at index 0 can prevent pruning of expired entries beyond it, causing unbounded growth in long dialogues | | 4 | `client/scripts/snapshot_event_router.gd:40-45` | suggestion | No `is_valid()` guard on handler callables — freed objects could halt dispatch loop | | 5 | `client/scripts/main.gd:321` | suggestion | Silent discard when dialogue already active — no `push_warning` for debugging | | 6 | `client/tests/test_dialogue_sprint18.gd` | suggestion | Duplicate test assertion with sprint20 test class — naming overlap may confuse | ### Tyre (Architecture): APPROVE All four commits architecturally sound. D-020 compliance correct, new abstractions well-bounded, no D-010/D-012 violations. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `client/scripts/snapshot_event_router.gd` | suggestion | `_keyed` Dictionary untyped — runtime failure mode on wrong types | | 2 | `client/scripts/snapshot_event_router.gd:39-45` | suggestion | Dispatch ordering guarantee undocumented — docstring should note it's a contract | | 3 | `client/ui/dialogue_box.gd:147` | suggestion | `_load_theme()` calls `UIStrings._parse_yaml()` (private) instead of `YamlParser.parse_flat()` (public API from #560) | | 4 | `client/scripts/util/yaml_parser.gd:64-72` | suggestion | Array termination lacks guard for empty stack — defensive code for user-authored content | | 5 | `client/scripts/autoloads/game_state.gd:116-117` | suggestion | `_prev_player_position` deprecated field — flag for two-part removal with fallback | | 6 | `client/scripts/autoloads/game_state.gd:162-170` | suggestion | `tiles` vs `visible_tiles` dual-key accreting complexity — candidate for deprecated fallback treatment | ### Verdict: APPROVED

jpmschweitzer closed this pull request

2026-02-25 23:51:00 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#71