jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(client): Sprint 21 — save/load game flow + debug_overlay move #73

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-27 11:08:54 +01:00
Owner
Copy Link
Copy Source

Summary

  • #257: Add Load Game screen to main menu with sorted save list, loading overlay during quickload, pending_load_path cross-scene flow
  • #561: Move debug_overlay.gd from scripts/ui/ to ui/ for consistency with all other UI components

Changes

  • New: client/ui/loading_screen.gd + .tscn — full-screen overlay during save/load round-trip
  • New: Load Game button + panel in main menu with saves sorted by date
  • New: GameState.pending_load_path for cross-scene load dispatch
  • Moved: debug_overlay.gd from scripts/ui/ to ui/
  • Updated: main.tscn, main_menu.tscn, main.gd, game_state.gd, ui-strings.yaml, test file

Test plan

  • F5 quicksave writes file to user://saves/<game-id>/quicksave.sav
  • F6 quickload shows loading overlay, restores state
  • Main menu Load Game lists saves sorted by date (newest first)
  • Empty saves list shows 'No saves found.' message
  • Loading overlay dismisses on save_result (success or failure)
  • debug_overlay.gd works at new path, test passes
  • make ci-client passes (pre-existing failures excluded)
## Summary - **#257**: Add Load Game screen to main menu with sorted save list, loading overlay during quickload, pending_load_path cross-scene flow - **#561**: Move debug_overlay.gd from scripts/ui/ to ui/ for consistency with all other UI components ## Changes - New: `client/ui/loading_screen.gd` + `.tscn` — full-screen overlay during save/load round-trip - New: Load Game button + panel in main menu with saves sorted by date - New: `GameState.pending_load_path` for cross-scene load dispatch - Moved: `debug_overlay.gd` from `scripts/ui/` to `ui/` - Updated: `main.tscn`, `main_menu.tscn`, `main.gd`, `game_state.gd`, `ui-strings.yaml`, test file ## Test plan - [ ] F5 quicksave writes file to `user://saves/<game-id>/quicksave.sav` - [ ] F6 quickload shows loading overlay, restores state - [ ] Main menu Load Game lists saves sorted by date (newest first) - [ ] Empty saves list shows 'No saves found.' message - [ ] Loading overlay dismisses on save_result (success or failure) - [ ] debug_overlay.gd works at new path, test passes - [ ] `make ci-client` passes (pre-existing failures excluded)
jpmschweitzer added 1 commit 2026-02-27 11:08:54 +01:00
feat(client): add save/load game flow and move debug_overlay (#257, #561) c797869503 
#257: Add Load Game screen to main menu with sorted save list, loading
overlay during quickload round-trip, and pending_load_path cross-scene
flow. F5/F6 quicksave/quickload were already wired.

#561: Move debug_overlay.gd from scripts/ui/ to ui/ for consistency
with all other UI components. Update scene and test references.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-27 17:44:39 +01:00
Author
Owner
Copy Link
Copy Source

Review: client -> main (PR #73, type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Save/load flow has a critical race condition in live mode.

# File Severity Issue
1 client/scripts/main.gd:51-64 critical LOAD_GAME sent in _ready() before SimBridge is connected — send_input() silently drops the command in live mode, loading screen stays up permanently
2 client/ui/main_menu.gd:97-102 warning Empty newest_save silently cancels load — no error shown, game starts in default state
3 client/ui/main_menu.gd:73-76 warning queue_free() race in _build_saves_list() — rapid reopen can double the save list for one frame
4 client/scripts/main.gd:178-181 suggestion F6 quickload show_loading() before send_input — stuck screen if send fails

Tyre (Architecture): APPROVE

Architecture is sound — correctly respects D-010/D-020 client-server split. pending_load_path is a pragmatic cross-scene handoff in existing GameState autoload pattern. File placement correct.

# File Severity Issue
1 client/ui/main_menu.gd warning pending_load_path not cleared in _on_new_game() / _on_continue() — stale path could trigger wrong load
2 client/ui/loading_screen.gd suggestion No failure state affordance — hide_loading(success: bool) would future-proof the API
3 client/tests/ suggestion No test_save_load_flow_sprint21.gd — pattern established, logical gap

Verdict: CHANGES REQUESTED

The critical issue is the LOAD_GAME race in main.gd._ready() — must defer dispatch until SimBridge connection is established. Also fix the stale pending_load_path defensive clears and the empty newest_save guard.

## Review: client -> main (PR #73, type: code) ### Hoshe (Code Quality): REQUEST_CHANGES Save/load flow has a critical race condition in live mode. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `client/scripts/main.gd:51-64` | **critical** | `LOAD_GAME` sent in `_ready()` before SimBridge is connected — `send_input()` silently drops the command in live mode, loading screen stays up permanently | | 2 | `client/ui/main_menu.gd:97-102` | warning | Empty `newest_save` silently cancels load — no error shown, game starts in default state | | 3 | `client/ui/main_menu.gd:73-76` | warning | `queue_free()` race in `_build_saves_list()` — rapid reopen can double the save list for one frame | | 4 | `client/scripts/main.gd:178-181` | suggestion | F6 quickload `show_loading()` before `send_input` — stuck screen if send fails | ### Tyre (Architecture): APPROVE Architecture is sound — correctly respects D-010/D-020 client-server split. `pending_load_path` is a pragmatic cross-scene handoff in existing GameState autoload pattern. File placement correct. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `client/ui/main_menu.gd` | warning | `pending_load_path` not cleared in `_on_new_game()` / `_on_continue()` — stale path could trigger wrong load | | 2 | `client/ui/loading_screen.gd` | suggestion | No failure state affordance — `hide_loading(success: bool)` would future-proof the API | | 3 | `client/tests/` | suggestion | No `test_save_load_flow_sprint21.gd` — pattern established, logical gap | ### Verdict: CHANGES REQUESTED The critical issue is the `LOAD_GAME` race in `main.gd._ready()` — must defer dispatch until SimBridge connection is established. Also fix the stale `pending_load_path` defensive clears and the empty `newest_save` guard.
jpmschweitzer added 1 commit 2026-02-27 18:06:20 +01:00
fix(client): address PR #73 review — race conditions and defensive guards (#257) 64bf4ec539 
- Defer LOAD_GAME dispatch until SimBridge reaches CONNECTED (critical)
- Guard _build_saves_list() against queue_free() race on rapid reopen
- Disable save entries with empty newest_save, guard in _on_save_selected
- Send before show_loading on F6 quickload, skip overlay on send failure
- Clear pending_load_path in _on_new_game()/_on_continue() (stale path)
- Add hide_loading(success: bool) API for future failure-state UI
- Add test_save_load_flow_sprint21.gd covering LoadingScreen + GameState

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-27 18:07:42 +01:00
Author
Owner
Copy Link
Copy Source

All review comments addressed in 64bf4ec:

Hoshe (Code Quality):

  1. ✅ Critical — LOAD_GAME race: deferred dispatch until SimBridge reaches CONNECTED (test mode dispatches synchronously, live mode uses signal handler)
  2. ✅ Warning — Empty newest_save: disabled button in save list + guard in _on_save_selected
  3. ✅ Warning — queue_free() race: _list_built flag guards _build_saves_list against rapid reopen
  4. ✅ Suggestion — F6 quickload: send_input first, show loading only on OK, skip overlay on failure

Tyre (Architecture):

  1. ✅ Warning — Stale pending_load_path: cleared at top of _on_new_game() and _on_continue()
  2. ✅ Suggestion — hide_loading(success: bool = true) API added
  3. ✅ Suggestion — test_save_load_flow_sprint21.gd added (LoadingScreen overlay + GameState.pending_load_path coverage)
All review comments addressed in 64bf4ec: **Hoshe (Code Quality):** 1. ✅ **Critical** — LOAD_GAME race: deferred dispatch until SimBridge reaches CONNECTED (test mode dispatches synchronously, live mode uses signal handler) 2. ✅ **Warning** — Empty newest_save: disabled button in save list + guard in _on_save_selected 3. ✅ **Warning** — queue_free() race: _list_built flag guards _build_saves_list against rapid reopen 4. ✅ **Suggestion** — F6 quickload: send_input first, show loading only on OK, skip overlay on failure **Tyre (Architecture):** 1. ✅ **Warning** — Stale pending_load_path: cleared at top of _on_new_game() and _on_continue() 2. ✅ **Suggestion** — hide_loading(success: bool = true) API added 3. ✅ **Suggestion** — test_save_load_flow_sprint21.gd added (LoadingScreen overlay + GameState.pending_load_path coverage)
jpmschweitzer commented 2026-02-27 18:46:09 +01:00
Author
Owner
Copy Link
Copy Source

Re-review: client -> main (PR #73, round 2)

Hoshe (Code Quality): APPROVE

All 6 issues from round 1 verified fixed:

  1. CRITICAL race condition — fixed via deferred dispatch (_on_sim_connected_for_load signal handler + _dispatch_pending_load()). Signal disconnection is safe.
  2. Empty newest_save — saves without files shown as disabled buttons + early return with push_error.
  3. queue_free() race — _list_built flag prevents rebuild on rapid reopen.
  4. Stale pending_load_path — cleared at top of _on_new_game() and _on_continue().
  5. hide_loading param — hide_loading(success: bool = true) added.
  6. Test file — 8 tests covering LoadingScreen API and pending_load_path field.

Remaining low-priority observations (non-blocking):

  • _dispatch_pending_load fails silently to player when send_input errors — no notification shown
  • No automated test for the deferred dispatch signal wiring itself (hard to test in GdUnit4)

Tyre (Architecture): APPROVE

Deferred dispatch pattern is architecturally sound and consistent with existing connection_state_changed usage. D-010 boundaries respected — pending_load_path stays client-side.

Verdict: APPROVED

All blockers resolved. Clean to merge.

## Re-review: client -> main (PR #73, round 2) ### Hoshe (Code Quality): APPROVE All 6 issues from round 1 verified fixed: 1. **CRITICAL race condition** — fixed via deferred dispatch (`_on_sim_connected_for_load` signal handler + `_dispatch_pending_load()`). Signal disconnection is safe. 2. **Empty `newest_save`** — saves without files shown as disabled buttons + early return with `push_error`. 3. **`queue_free()` race** — `_list_built` flag prevents rebuild on rapid reopen. 4. **Stale `pending_load_path`** — cleared at top of `_on_new_game()` and `_on_continue()`. 5. **`hide_loading` param** — `hide_loading(success: bool = true)` added. 6. **Test file** — 8 tests covering LoadingScreen API and `pending_load_path` field. Remaining low-priority observations (non-blocking): - `_dispatch_pending_load` fails silently to player when `send_input` errors — no notification shown - No automated test for the deferred dispatch signal wiring itself (hard to test in GdUnit4) ### Tyre (Architecture): APPROVE Deferred dispatch pattern is architecturally sound and consistent with existing `connection_state_changed` usage. D-010 boundaries respected — `pending_load_path` stays client-side. ### Verdict: APPROVED All blockers resolved. Clean to merge.
jpmschweitzer closed this pull request 2026-02-27 18:53:19 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#73