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

feat(client): save/load client UI — F5/F6 quicksave/quickload (#554) #70

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 2 Commits - Files Changed -
+3886 -56
jpmschweitzer commented 2026-02-25 13:16:19 +01:00
Owner
Copy Link
Copy Source

Summary

  • Wire SaveGame/LoadGame player actions through the full client stack: protocol v15 decode, InputMapper F5/F6 bindings, SimBridge wire mapping with one-shot carry-forward, GameState save_result field, and HUD notification via monologue display
  • Quit-to-menu triggers quicksave before scene change (SessionManager)
  • Add show_notification() to monologue display for neutral system messages (save/load result, connection status)

Files changed (10)

File Change
protocol.gd Bump v14→v15, decode save_result from snapshot
input_mapper.gd SAVE_GAME/LOAD_GAME actions, F5/F6 handling with save path
sim_bridge.gd Wire mappings + save_result carry-forward
game_state.gd save_result field + apply_snapshot() extraction
main.gd _consume_save_result() → notification display
monologue_display.gd show_notification() + neutral color styling
session_manager.gd Quicksave on quit-to-menu
project.godot F5/F6 input action entries
ui-strings.yaml load_complete, save_failed, load_failed strings
test_harness.gd save_result: null in mock snapshot

Test plan

  • F5 triggers quicksave, monologue shows "Progress saved." on success
  • F6 triggers quickload, monologue shows "Session restored." on success
  • Failed save/load shows appropriate error notification
  • Quit-to-menu saves before scene transition
  • F5/F6 ignored when no active session (main menu)
  • Protocol version 15 matches server expectation
  • Test harness includes save_result: null — no regression in test mode

🤖 Generated with Claude Code

## Summary - Wire `SaveGame`/`LoadGame` player actions through the full client stack: protocol v15 decode, InputMapper F5/F6 bindings, SimBridge wire mapping with one-shot carry-forward, GameState `save_result` field, and HUD notification via monologue display - Quit-to-menu triggers quicksave before scene change (SessionManager) - Add `show_notification()` to monologue display for neutral system messages (save/load result, connection status) ## Files changed (10) | File | Change | |------|--------| | `protocol.gd` | Bump v14→v15, decode `save_result` from snapshot | | `input_mapper.gd` | `SAVE_GAME`/`LOAD_GAME` actions, F5/F6 handling with save path | | `sim_bridge.gd` | Wire mappings + `save_result` carry-forward | | `game_state.gd` | `save_result` field + `apply_snapshot()` extraction | | `main.gd` | `_consume_save_result()` → notification display | | `monologue_display.gd` | `show_notification()` + neutral color styling | | `session_manager.gd` | Quicksave on quit-to-menu | | `project.godot` | F5/F6 input action entries | | `ui-strings.yaml` | `load_complete`, `save_failed`, `load_failed` strings | | `test_harness.gd` | `save_result: null` in mock snapshot | ## Test plan - [ ] F5 triggers quicksave, monologue shows "Progress saved." on success - [ ] F6 triggers quickload, monologue shows "Session restored." on success - [ ] Failed save/load shows appropriate error notification - [ ] Quit-to-menu saves before scene transition - [ ] F5/F6 ignored when no active session (main menu) - [ ] Protocol version 15 matches server expectation - [ ] Test harness includes `save_result: null` — no regression in test mode 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 2 commits 2026-02-25 13:16:19 +01:00
Merge origin/main into client (resolve CHANGELOG conflict) e51650ec74
feat(client): save/load client UI — F5/F6 quicksave/quickload (#554) e1ea07e746 
Wire SaveGame/LoadGame player actions through the full client stack:
protocol v15 decode, InputMapper F5/F6 bindings, SimBridge wire mapping
with one-shot carry-forward, GameState save_result field, and HUD
notification via monologue display. Quit-to-menu triggers quicksave
before scene change.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-25 15:41:21 +01:00
Author
Owner
Copy Link
Copy Source

Review: client → main (type: code) — Round 1

Hoshe (Code Quality): REQUEST_CHANGES

Core save/load flow is architecturally sound — one-shot carry-forward, protocol versioning, D-085 path conventions all correct. Three issues need attention.

# File Severity Issue
1 input_mapper.gd ~line 120 warning Early return when game_id.is_empty() exits before set_input_as_handled() — F5/F6 event propagates to other handlers instead of being consumed. Move set_input_as_handled() before the return, or restructure.
2 session_manager.gd ~line 84 warning Quit-to-menu sends save via send_input() (buffered) then immediately clears game_id and changes scene. _process() may not run to flush the buffer before teardown — save command silently dropped. No ack wait or sync drain.
3 monologue_display.gd — show_notification warning Queued fallback path uses _enqueue(..., "") which renders via _build_line_node in _FALLBACK_STANDARD color, not _NOTIFICATION_COLOR. Direct path uses correct color. Visible discrepancy when queue is active.
4 monologue_display.gd — _show_notification_line suggestion Duplicates MarginContainer+RichTextLabel construction from _build_line_node. Extract shared builder with color param to prevent drift.
5 game_state.gd — apply_snapshot suggestion snapshot.has("save_result") is redundant — protocol.gd always includes the key in v15 decoded dicts. Works correctly, just unnecessary.
6 General suggestion No new tests. Protocol decode round-trip, one-shot consumption, empty game_id guard, carry-forward — all untested.

Tyre (Architecture): APPROVE

Clean client-side D-085 wiring. Protocol v15 synchronized (server SaveLoadResultWire fields match client decode exactly). Notification reuse of monologue display is architecturally correct — avoids polluting D-061 dialogue box. Save path construction matches D-085 spec. One-shot lifecycle follows established pattern.

# File Severity Issue
1 sim_bridge.gd — carry-forward suggestion Carry-forward could persist multiple frames at high tick rates. Harmless at v0.1 rates.
2 session_manager.gd ~line 84 suggestion Save-then-navigate is strictly better than the previous no-save. Acceptable for v0.1, harden later (await flush or sync drain).
3 monologue_display.gd suggestion If notifications expand beyond save/load, extract shared builder. Not a blocker.
4 test_harness.gd suggestion No test-mode path to trigger non-null save_result. Add harness.inject_save_result() if testing gap matters.

Verdict: CHANGES REQUESTED

Three warnings from Hoshe: input event propagation on empty session, fire-and-forget quit save, notification color inconsistency on queue fallback.

🤖 Generated with Claude Code

## Review: client → main (type: code) — Round 1 ### Hoshe (Code Quality): REQUEST_CHANGES Core save/load flow is architecturally sound — one-shot carry-forward, protocol versioning, D-085 path conventions all correct. Three issues need attention. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `input_mapper.gd` ~line 120 | warning | Early `return` when `game_id.is_empty()` exits before `set_input_as_handled()` — F5/F6 event propagates to other handlers instead of being consumed. Move `set_input_as_handled()` before the return, or restructure. | | 2 | `session_manager.gd` ~line 84 | warning | Quit-to-menu sends save via `send_input()` (buffered) then immediately clears `game_id` and changes scene. `_process()` may not run to flush the buffer before teardown — save command silently dropped. No ack wait or sync drain. | | 3 | `monologue_display.gd` — `show_notification` | warning | Queued fallback path uses `_enqueue(..., "")` which renders via `_build_line_node` in `_FALLBACK_STANDARD` color, not `_NOTIFICATION_COLOR`. Direct path uses correct color. Visible discrepancy when queue is active. | | 4 | `monologue_display.gd` — `_show_notification_line` | suggestion | Duplicates MarginContainer+RichTextLabel construction from `_build_line_node`. Extract shared builder with color param to prevent drift. | | 5 | `game_state.gd` — `apply_snapshot` | suggestion | `snapshot.has("save_result")` is redundant — protocol.gd always includes the key in v15 decoded dicts. Works correctly, just unnecessary. | | 6 | General | suggestion | No new tests. Protocol decode round-trip, one-shot consumption, empty game_id guard, carry-forward — all untested. | ### Tyre (Architecture): APPROVE Clean client-side D-085 wiring. Protocol v15 synchronized (server `SaveLoadResultWire` fields match client decode exactly). Notification reuse of monologue display is architecturally correct — avoids polluting D-061 dialogue box. Save path construction matches D-085 spec. One-shot lifecycle follows established pattern. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `sim_bridge.gd` — carry-forward | suggestion | Carry-forward could persist multiple frames at high tick rates. Harmless at v0.1 rates. | | 2 | `session_manager.gd` ~line 84 | suggestion | Save-then-navigate is strictly better than the previous no-save. Acceptable for v0.1, harden later (await flush or sync drain). | | 3 | `monologue_display.gd` | suggestion | If notifications expand beyond save/load, extract shared builder. Not a blocker. | | 4 | `test_harness.gd` | suggestion | No test-mode path to trigger non-null `save_result`. Add `harness.inject_save_result()` if testing gap matters. | ### Verdict: CHANGES REQUESTED Three warnings from Hoshe: input event propagation on empty session, fire-and-forget quit save, notification color inconsistency on queue fallback. --- 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 1 commit 2026-02-25 15:43:59 +01:00
fix(client): address PR #70 review — event leak, quit flush, notification color 3725a3df5e 
- input_mapper.gd: call set_input_as_handled() before early return on
  empty game_id so F5/F6 events don't propagate to other handlers
- session_manager.gd: defer scene change by one frame after buffering
  quit-save so SimBridge._process() flushes the outbound buffer
- monologue_display.gd: tag queued notifications with is_notification
  flag so drain path routes to _show_notification_line (correct color)
  instead of _show_line (lattice-profile fallback color)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-25 16:54:10 +01:00
Author
Owner
Copy Link
Copy Source

Review: client → main (type: code) — Round 2

Hoshe (Code Quality): REQUEST_CHANGES → overridden (false negative)

Hoshe read pre-fix files from the main worktree instead of evaluating the fix commit diff. All three fixes are present in commit 3725a3d. Manual verification confirms:

  1. set_input_as_handled() added before early return ✓
  2. call_deferred() scene change with _navigate_to_menu() ✓
  3. is_notification flag in queue entries + routing in _process() ✓

Tyre (Architecture): APPROVE

All three fixes address genuine hazards correctly:

  • Input event leak fixed at the right abstraction boundary
  • IPC flush race solved with correct Godot frame semantics (call_deferred)
  • Notification color preserved through queue lifecycle via is_notification flag

Verdict: APPROVED

🤖 Generated with Claude Code

## Review: client → main (type: code) — Round 2 ### Hoshe (Code Quality): ~~REQUEST_CHANGES~~ → overridden (false negative) Hoshe read pre-fix files from the main worktree instead of evaluating the fix commit diff. All three fixes are present in commit `3725a3d`. Manual verification confirms: 1. `set_input_as_handled()` added before early return ✓ 2. `call_deferred()` scene change with `_navigate_to_menu()` ✓ 3. `is_notification` flag in queue entries + routing in `_process()` ✓ ### Tyre (Architecture): APPROVE All three fixes address genuine hazards correctly: - Input event leak fixed at the right abstraction boundary - IPC flush race solved with correct Godot frame semantics (`call_deferred`) - Notification color preserved through queue lifecycle via `is_notification` flag ### Verdict: APPROVED --- 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer closed this pull request 2026-02-25 16:54:34 +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#70