Added pause functionality for private multiplayer games #2657
Conversation
- Only lobby creator can pause/unpause the game - Show pause overlay when game is paused - Pause state communicated via worker messages - Server validates lobby creator before accepting pause intent
WalkthroughAdds a server-controlled pause flow: a new toggle_pause intent, execution handler, game paused state and update, client UI emitting pause-intent events, lobby-creator metadata propagation, and localized pause messages. Changes
Sequence Diagram(s)sequenceDiagram
actor LobbyCreator as Lobby Creator
participant UI as GameRightSidebar / SettingsModal
participant Transport as Client.Transport
participant Server as GameServer
participant Executor as ExecutionManager
participant Game as GameImpl
participant UpdateBus as Update Stream / Clients
rect rgb(220,240,255)
note over LobbyCreator,UI: User initiates pause
LobbyCreator->>UI: Click pause button
UI->>Transport: emit PauseGameIntentEvent(paused: true)
Transport->>Server: sendIntent(type: "toggle_pause", paused: true)
end
rect rgb(240,220,255)
note over Server: Server validates and applies intent
Server->>Server: isLobbyCreator(clientID)?
alt is lobby creator
Server->>Server: Add intent, end current turn, set isPaused = true
Server->>Executor: Deliver toggle_pause intent
Executor->>Executor: Create PauseExecution(player, paused:true)
Executor->>Game: PauseExecution.init -> setPaused(true)
Game->>UpdateBus: Emit GamePausedUpdate(paused: true)
else not lobby creator
Server->>Server: Log and ignore intent
end
end
rect rgb(240,255,220)
note over UpdateBus,UI: Clients receive pause update
UpdateBus->>UI: GamePausedUpdate(paused: true)
UI->>UI: HeadsUpMessage shows localized pause text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/GameServer.ts (1)
351-381: Authorization check is correct, but consider adding clarity comment.The authorization check properly restricts pause toggling to the lobby creator. The pause/unpause logic is correct but subtle: when pausing,
endTurn()executes before settingisPausedso the current turn completes; when unpausing,isPausedis cleared beforeendTurn()so the next turn can execute.📝 Optional: Add comment to explain the order
case "toggle_pause": { // Only lobby creator can pause/resume if (client.clientID !== this.lobbyCreatorID) { this.log.warn(`Only lobby creator can toggle pause`, { clientID: client.clientID, creatorID: this.lobbyCreatorID, gameID: this.id, }); return; } const paused = !!clientMsg.intent.paused; if (paused) { - // Pausing: send intent while game is still running, then pause + // Pausing: send intent and complete current turn before pause takes effect this.addIntent(clientMsg.intent); this.endTurn(); this.isPaused = true; } else { - // Unpausing: unpause first, then send intent + // Unpausing: clear pause flag before sending intent so next turn can execute this.isPaused = false; this.addIntent(clientMsg.intent); this.endTurn(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
resources/lang/en.json(1 hunks)src/client/ClientGameRunner.ts(3 hunks)src/client/Transport.ts(4 hunks)src/client/graphics/GameRenderer.ts(5 hunks)src/client/graphics/layers/GameRightSidebar.ts(4 hunks)src/client/graphics/layers/PauseOverlay.ts(1 hunks)src/client/graphics/layers/SettingsModal.ts(2 hunks)src/client/index.html(1 hunks)src/core/Schemas.ts(6 hunks)src/core/execution/ExecutionManager.ts(1 hunks)src/core/worker/Worker.worker.ts(1 hunks)src/core/worker/WorkerClient.ts(3 hunks)src/core/worker/WorkerMessages.ts(3 hunks)src/server/GameServer.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/GameServer.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/PauseOverlay.tssrc/client/ClientGameRunner.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/PauseOverlay.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/Schemas.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (6)
src/client/graphics/layers/PauseOverlay.ts (3)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
GamePausedEvent(36-38)
src/core/execution/ExecutionManager.ts (1)
src/core/execution/NoOpExecution.ts (1)
NoOpExecution(3-12)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/Transport.ts (1)
PauseGameIntentEvent(32-34)src/client/LocalServer.ts (1)
pause(100-102)
src/client/ClientGameRunner.ts (2)
src/client/graphics/GameRenderer.ts (1)
createRenderer(46-302)src/client/Transport.ts (1)
GamePausedEvent(36-38)
src/client/graphics/layers/GameRightSidebar.ts (1)
src/client/Transport.ts (1)
PauseGameIntentEvent(32-34)
src/client/Transport.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
🔇 Additional comments (19)
src/core/execution/ExecutionManager.ts (1)
126-128: LGTM - Clean server-side pause handling.The toggle_pause case correctly returns a NoOpExecution since pause state is managed server-side. The comment clearly explains the behavior.
resources/lang/en.json (1)
743-745: LGTM - Clear pause message translation.The new pause section with the "game_paused" key is well-structured and the message clearly indicates who paused the game.
src/core/Schemas.ts (2)
360-363: Clarify the default value for paused field.The
pausedfield has.default(false), which means if a toggle_pause intent is sent without the paused field, it will default tofalse(unpause). Is this the intended behavior? Typically, toggle intents would require the field to be explicitly set.Consider whether the paused field should be required instead:
export const TogglePauseIntentSchema = BaseIntentSchema.extend({ type: z.literal("toggle_pause"), - paused: z.boolean().default(false), + paused: z.boolean(), });This would ensure clients always explicitly specify whether they're pausing or unpausing.
449-449: LGTM - Optional isLobbyCreator field is appropriate.Making
isLobbyCreatoroptional maintains backward compatibility and only the lobby creator client needs this flag set.src/client/index.html (1)
356-356: LGTM - Pause overlay element added.The pause-overlay element is appropriately positioned in the DOM structure alongside other game overlay components.
src/core/worker/Worker.worker.ts (1)
71-80: LGTM - Clean pause state propagation.The worker correctly detects toggle_pause intents in processed turns and notifies the main thread of the pause state change. The type guard (
"paused" in pauseIntent) is good defensive programming.src/client/Transport.ts (2)
32-38: LGTM - Clean event separation.Good design to separate
PauseGameIntentEvent(user action) fromGamePausedEvent(state notification). This makes the event flow clear and prevents confusion.
583-601: Verify multiplayer pause feedback flow.In multiplayer mode (lines 594-600), the client sends a toggle_pause intent but doesn't emit
GamePausedEventlocally. This means the UI won't update until the server processes the intent and broadcasts it back through the turn system.Verify that:
- The server broadcasts the pause state change to all clients (including the sender) via the turn intents
- The worker detects this and triggers
GamePausedEvent(as seen in Worker.worker.ts lines 71-80)- There's acceptable UI feedback delay for the lobby creator who clicked pause
If the delay is noticeable, consider emitting an optimistic
GamePausedEventimmediately for the sender, then reconciling with server state.src/client/ClientGameRunner.ts (2)
198-203: LGTM - isLobbyCreator flag properly propagated.The isLobbyCreator flag is correctly extracted from gameStartInfo with a sensible default of
false, and passed to the renderer for UI control decisions.
338-340: LGTM - Pause state change handler wired correctly.The worker's pause state changes are properly bridged to the event bus via
GamePausedEvent, allowing UI components to react to pause state changes.src/client/graphics/layers/SettingsModal.ts (1)
17-17: LGTM - Updated to use PauseGameIntentEvent.The modal correctly uses
PauseGameIntentEventto signal pause intent, aligning with the new event model introduced in Transport.ts.Also applies to: 110-110
src/core/worker/WorkerMessages.ts (1)
17-17: LGTM! Clean pause state message definition.The new
PauseStateMessageinterface is simple and correctly typed. Including it inWorkerMessage(worker-to-main direction) aligns with the expected message flow where the worker notifies the main thread of pause state changes.Also applies to: 61-64, 136-136
src/client/graphics/GameRenderer.ts (1)
46-51: LGTM! Pause overlay integration follows established patterns.The
isLobbyCreatorparameter propagates correctly fromcreateRendererto the right sidebar. ThePauseOverlaysetup follows the same DOM query and initialization pattern used for other layers, and placing it near the end of the layers array (line 287) ensures it renders on top as overlays should.Also applies to: 159-159, 240-244, 287-287
src/core/worker/WorkerClient.ts (1)
21-21: LGTM! Callback pattern is consistent and simple.The
pauseStateCallbackfollows the same pattern as the existinggameUpdateCallback, maintaining consistency. The message handler correctly invokes the callback whenpause_statemessages arrive, and the registration method provides a clean API for consumers.Also applies to: 47-51, 99-101
src/server/GameServer.ts (3)
65-65: LGTM! Simple pause state flag.The
isPausedfield is initialized tofalseas expected for a new game.
523-558: LGTM! Defensive client lookup and correct isLobbyCreator propagation.The websocket-to-client lookup includes a defensive guard that logs a warning and returns early if the client isn't found. All call sites provide valid websockets, so this guard handles unexpected edge cases gracefully. The
isLobbyCreatorflag is correctly determined and included in the start message for each client.
560-564: LGTM! Efficient pause guard.The early return when
isPausedis true prevents turn execution and saves unnecessary work. Placing the check before creating the turn object avoids allocations while paused.src/client/graphics/layers/GameRightSidebar.ts (1)
12-12: LGTM! Clearer naming and improved conditional logic.The refactoring makes the button rendering logic more readable. Using
isReplayOrSingleplayerandshowPauseButtonas intermediate variables clarifies the conditions. Renaming toPauseGameIntentEventbetter expresses that this is an intent to pause (user action) rather than a notification that pause occurred.Also applies to: 22-22, 99-99, 153-184
src/client/graphics/layers/PauseOverlay.ts (1)
1-47: LGTM! Simple, focused pause overlay component.The
PauseOverlaycomponent is straightforward and does one thing well. It listens forGamePausedEvent, updates state, and renders a full-screen overlay with a centered message. The use oftranslateText("pause.game_paused")assumes the translation key exists (mentioned in the PR description). Avoiding shadow DOM by overridingcreateRenderRoot()is consistent with other UI components in this codebase.
|
Really great PR and good description! But blurring the screen on pause is probably not good? You want to be able to check out the state of the game while waiting for it to continue. Also I think we don't need the PauseOverlay in singleplayer. The player knows that he just stopped the game, we don't need to inform about that. |
|
@FloPinguin Thank you, really appreciate it!
Yes, makes sense to me, I'll remove it
I was worrying about game consistency and wanted to make sure the same behaviour shows in all game modes. That said, I do see your point as well. I’m not entirely sure what the best approach is here. |
|
Have had a discussion with Evan regarding this, I think the consistency between single player and the multiplayer private lobby is best so have the execution be the same across both |
|
@Sgt-lewis But we should use a text like "Game paused" instead of "Game paused by Lobby Creator" for singleplayer. Currently only "Game paused by Lobby Creator" is added to en.json |
Yep I agree sorry if I miss understood your point but I agree, would be good to have the "Game Paused" for single player and "Game Paused by Lobby Host" for the multiplayer games |
- removed background blur - added text for singleplayer game type - removed completely the overlay in replays
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(5 hunks)src/client/graphics/layers/PauseOverlay.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/GameRenderer.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PauseOverlay.ts (3)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
GamePausedEvent(36-38)
🔇 Additional comments (4)
resources/lang/en.json (1)
743-746: Translation strings look good.The pause messages appropriately differentiate between singleplayer and multiplayer contexts as discussed in the PR comments.
src/client/graphics/layers/PauseOverlay.ts (3)
1-7: Imports are clean and focused.All imports are used and necessary for the overlay implementation.
15-17: Correct pattern for global styles.Returning
thisfromcreateRenderRootbypasses the shadow DOM, allowing the overlay to use global Tailwind classes.
26-30: Correct replay handling.The early return correctly hides the overlay in replay mode, as discussed in the PR objectives.
github-project-automation
bot
moved this from Peer Review
to Development
in OpenFront Release Management
Done! I've also completely removed the pause overlay in replays. Figured it would make sense to keep the map navigable when reviewing gameplay. |
|
Great looks good. I'll pass this over to @DevelopingTom and @evanpelle to review |
- Store isLobbyCreator on PlayerView via PlayerSchema data pipeline - Replace PauseStateMessage with GamePausedUpdate in game update stream - Create PauseExecution with Game.setPaused()/isPaused() methods - Unify pause handling: LocalServer detects toggle_pause intents - Remove unused pause()/resume() methods from LocalServer - Simplify Transport to always send pause intent - Optimize Client constructor parameter order
|
@evanpelle All feedback has been addressed! Let me know if that works out. |
- Remove GamePausedEvent, use GameUpdateType.GamePaused directly in UI layers - Remove isLobbyCreator variable from Client - Fix pause functionality in replays by handling pause intents before replay check
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/graphics/layers/PauseOverlay.ts (1)
12-13: Add definite assignment assertions for externally-set properties.The
gameandeventBusproperties are set byGameRendererafter construction, but TypeScript strict mode will complain. Add the!assertion to signal these will be assigned before use:- public game: GameView; - public eventBus: EventBus; + public game!: GameView; + public eventBus!: EventBus;Also,
eventBusis imported and declared but never used in this file. If it's not needed, remove it to keep the code clean.
🧹 Nitpick comments (1)
src/client/graphics/layers/GameRightSidebar.ts (1)
105-108: LocalisPausedstate may drift from server state.The sidebar toggles its local
isPausedon button click, but the actual pause state is server-controlled. If the server rejects a pause request (e.g., non-lobby-creator tries to pause), the UI will show the wrong icon until the nextGamePausedupdate arrives.Consider syncing
isPausedfromGamePausedupdates intick()(likePauseOverlaydoes) rather than toggling locally:tick() { const updates = this.game.updatesSinceLastTick(); if (updates) { this.hasWinner = this.hasWinner || updates[GameUpdateType.Win].length > 0; + if (updates[GameUpdateType.GamePaused].length > 0) { + this.isPaused = updates[GameUpdateType.GamePaused][0].paused; + } }This keeps the button icon in sync with actual game state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/ClientGameRunner.tssrc/client/LocalServer.tssrc/client/Transport.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/PauseOverlay.tssrc/server/GameServer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/ClientGameRunner.ts
- src/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PauseOverlay.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/PauseOverlay.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/PauseOverlay.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/graphics/layers/PauseOverlay.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
🧬 Code graph analysis (3)
src/client/Transport.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/client/graphics/layers/PauseOverlay.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(584-910)
src/client/graphics/layers/GameRightSidebar.ts (1)
src/client/Transport.ts (1)
PauseGameIntentEvent(32-34)
🔇 Additional comments (9)
src/client/Transport.ts (3)
32-34: Good rename to clarify intent-based pause.The name
PauseGameIntentEventbetter describes what this event does - it signals user intent to toggle pause, which gets sent as an intent to the server. Clear naming.
241-241: Event binding looks good.The event handler registration follows the same pattern as other intent events in this file.
579-585: Clean intent-based pause handling.This handler sends
toggle_pauseintent through the standardsendIntentpath - same as all other game actions. This means pause works the same way for local and remote servers. Matches the past review feedback from evanpelle about having transport always send the pause intent.src/client/LocalServer.ts (2)
110-125: Pause/unpause order is correct and well-commented.The logic here is subtle but correct:
- Pause: Push intent →
endTurn()(delivers the pause intent) → setpaused = true(blocks future turns)- Unpause: Clear
paused = falsefirst (soendTurn()won't bail at line 182) → push intent →endTurn()The inline comments explain the reasoning. This ensures the pause/unpause intent reaches clients via the turn system.
127-130: Replay intent blocking is correct.Non-pause intents are ignored during replays. The
toggle_pauseintent is handled above and returns early, so pause still works during replays. Simple and clear.src/client/graphics/layers/GameRightSidebar.ts (2)
39-39: Lobby creator detection works correctly.The one-way transition (
false→trueonly) is a good pattern here since lobby creator status never changes during a game. TherequestUpdate()call triggers a re-render to show the pause button.Also applies to: 62-66
161-192: Clear conditional rendering logic.The separation of
isReplayOrSingleplayerandshowPauseButtonmakes the intent easy to follow:
- Fast-forward button: only for replays and singleplayer
- Pause button: replays, singleplayer, OR multiplayer lobby creators
Good use of template literals with conditional rendering.
src/client/graphics/layers/PauseOverlay.ts (2)
20-27: Tick-based pause detection is straightforward.Reading
GamePausedupdates intick()keeps the overlay in sync with actual game state. The check forlength > 0before accessing[0]is correct.
29-55: Render logic is clean and handles replay edge case.Good decisions here:
- Replays skip the overlay so players can inspect the map while paused
- Singleplayer vs multiplayer text selection uses simple conditional
- The overlay blocks context menu to prevent accidental interactions
One small thing: the double-space in the class string at line 38 (
bg-opacity-50 z-50) is harmless but looks like a typo.
|
@evanpelle Done! While implementing your feedback, I also noticed and fixed an issue where pause intents where not handled properly during replays. |
thanks! |
|
@YoussfeCantCode can you check the resolve comments when you have done them just so when it's skimmed over we know what's been actioned and what's outstanding? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/LocalServer.ts (1)
110-127: Consider blocking toggle_pause intents during replays.The toggle_pause intent is processed even when a game record exists (replay mode). While the pause state might not cause harm, allowing pause toggling during replays could lead to unexpected behavior since replay turns are pre-recorded.
Proposed fix
if (clientMsg.type === "intent") { + // Don't process any intents during replays + if (this.lobbyConfig.gameRecord) { + return; + } + if (clientMsg.intent.type === "toggle_pause") { if (clientMsg.intent.paused) { // Pausing: add intent and end turn before pause takes effect this.intents.push(clientMsg.intent); this.endTurn(); this.paused = true; } else { // Unpausing: clear pause flag before adding intent so next turn can execute this.paused = false; this.intents.push(clientMsg.intent); this.endTurn(); } return; } - // Don't process non-pause intents during replays - if (this.lobbyConfig.gameRecord) { - return; - }src/server/GameServer.ts (1)
522-534: Consider removing verbose logging in production.The added logging in
sendStartGameMsgis helpful for debugging, but logging on every start message (including rejoins) may be excessive in production. Consider using debug-level logging or removing the detailed logs once the feature is stable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/LocalServer.tssrc/client/graphics/layers/HeadsUpMessage.tssrc/core/execution/ExecutionManager.tssrc/core/execution/PauseExecution.tssrc/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/server/GameServer.tssrc/core/execution/PauseExecution.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/GameServer.tssrc/client/graphics/layers/HeadsUpMessage.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/PauseExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/PauseExecution.ts
🧬 Code graph analysis (2)
src/core/execution/PauseExecution.ts (1)
src/core/game/Game.ts (3)
Execution(356-361)Player(531-669)Game(671-767)
src/core/execution/ExecutionManager.ts (1)
src/core/execution/PauseExecution.ts (1)
PauseExecution(3-27)
🔇 Additional comments (6)
src/client/graphics/layers/HeadsUpMessage.ts (2)
28-36: LGTM!The pause state handling via
GamePausedupdates is clean. The visibility logic combining spawn phase and pause state is correct, and thegetMessage()helper keeps the render method simple.
39-50: LGTM!The
getMessage()method is straightforward and correctly selects the appropriate translation key based on game type when paused. Good separation of concerns.src/server/GameServer.ts (2)
351-378: LGTM!The toggle_pause handler correctly validates that only the lobby creator can pause/resume the game. The pause sequence (add intent → end turn → set flag) and unpause sequence (clear flag → add intent → end turn) ensure proper turn synchronization across clients.
555-559: LGTM!The early return when
isPausedis true correctly prevents turn advancement while the game is paused. Simple and effective.src/core/execution/ExecutionManager.ts (1)
127-128: LGTM!The
toggle_pausecase follows the established pattern for intent handling. Clean integration withPauseExecution.src/core/execution/PauseExecution.ts (1)
1-27: LGTM!Clean implementation following the
Executioninterface. The defense-in-depth check ininit()(verifying lobby creator or singleplayer) provides a good safety net even though the server already validates this. TheactiveDuringSpawnPhase() = truecorrectly allows pausing during the spawn phase.
|
I noticed a problem with using Pause Intent in replays that I didn't catch before (since replays weren't showing pause messages previously), when watching a replay, the viewer can see the original game's pauses flash briefly on screen because it's processing original gameplay intents. Additionally, when the viewer presses pause during a replay, it actually pauses, but the HeadsUpMessage doesn't get the update to show the pause message because the viewer's pause intent gets discarded when endTurn() replaces this.intents with the archived replay intents. OpenFrontIO/src/client/LocalServer.ts Lines 182 to 188 in c90ad49 Should I maybe filter out pause intents upon replay creation? Or maybe change the approach to this altogether without using intents? |
could we have GameRunner ignore pause intents on replays?
I think this is okay. I don't think we really need a heads up message when pause on replay (or even singpleplayer for that matter) since the user knows they paused it. |
…ng pause - Filter out pause intents from archived replays in ClientGameRunner - Prevent processing intents while game is paused in LocalServer and GameServer
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/client/graphics/layers/GameRightSidebar.ts (1)
40-40: Consider resettingisLobbyCreatorininit()to avoid stale state.The
isLobbyCreatorflag uses a one-way transition (false → true), but it's never reset wheninit()is called. If the same component instance is reused between games, a player who was lobby creator in one game could retain that status incorrectly in a subsequent game.Proposed fix
init() { + this.isLobbyCreator = false; this._isSinglePlayer = this.game?.config()?.gameConfig()?.gameType === GameType.Singleplayer || this.game.config().isReplay(); this._isVisible = true; this.game.inSpawnPhase(); this.requestUpdate(); }Also applies to: 46-54
src/server/GameServer.ts (1)
525-531: Early return when client not found could silently fail for late joiners.If
sendStartGameMsgis called for a websocket that isn't inactiveClients(e.g., during a race betweenjoinClientandsendStartGameMsg), the function returns early with only a warning. This could leave a late joiner without the start message.The current code path in
joinClient(line 224) andrejoinClient(line 271) should always have the client inactiveClientsbefore calling this, but the defensive return could mask issues.Consider throwing an error instead of silently returning, or verify that all callers guarantee the client is in
activeClients:#!/bin/bash # Find all call sites of sendStartGameMsg rg -n "sendStartGameMsg" --type=ts -C2src/client/LocalServer.ts (1)
111-121: Add comment explaining pause flag ordering.The ordering of when
this.pausedis set is subtle but important:
- When pausing: intent is pushed and
endTurn()called before settingpaused = true, so the pause intent is sent before the pause takes effect.- When unpausing:
pausedis cleared before callingendTurn(), so the unpause intent can be sent.Consider adding a brief comment explaining this sequencing to help future maintainers.
📝 Suggested clarifying comments
if (clientMsg.intent.type === "toggle_pause") { if (clientMsg.intent.paused) { - // Pausing: add intent and end turn before pause takes effect + // Pausing: send the pause intent in a turn, then set paused flag + // so subsequent turns are blocked by endTurn() gate at line 179 this.intents.push(clientMsg.intent); this.endTurn(); this.paused = true; } else { - // Unpausing: clear pause flag before adding intent so next turn can execute + // Unpausing: clear pause flag so unpause intent can be sent, + // then normal turn execution resumes this.paused = false; this.intents.push(clientMsg.intent); this.endTurn(); } return; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/ClientGameRunner.tssrc/client/LocalServer.tssrc/client/graphics/layers/GameRightSidebar.tssrc/server/GameServer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/graphics/layers/ControlPanel.ts:160-183
Timestamp: 2025-06-07T20:01:52.720Z
Learning: The user its-sii prefers simpler, more readable code over complex type-safe approaches when the benefits don't justify the added complexity. They value maintainability and clarity in code implementation.
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GameServer.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/server/GameServer.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/server/GameServer.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/GameServer.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/server/GameServer.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/GameRightSidebar.ts (1)
src/client/Transport.ts (1)
PauseGameIntentEvent(32-34)
🔇 Additional comments (8)
src/client/graphics/layers/GameRightSidebar.ts (2)
106-109: LocalisPausedtoggle is optimistic but acceptable here.The local state is toggled before the server confirms the pause. This works because the pause button is only visible to lobby creators (checked in
maybeRenderReplayButtons), so the server should always accept the request. For singleplayer/replay mode, there's no server validation anyway.
164-195: Clean conditional rendering logic.The computed flags
isReplayOrSingleplayerandshowPauseButtonmake the render logic easy to follow. The separation of replay controls (fast-forward) and pause controls is clear.src/server/GameServer.ts (4)
351-379: Pause toggle logic is correct and well-ordered.The sequencing handles the state transitions properly:
- Pause: broadcast intent → end turn → then freeze (clients see the pause intent)
- Unpause: unfreeze → broadcast intent → end turn (turn can execute normally)
Auth check ensures only the lobby creator can toggle. Good work.
380-386: Intents silently dropped while paused.When the game is paused, player intents are discarded without acknowledgment. This is the expected behavior for a frozen game, but players won't receive feedback that their actions aren't being queued. Consider if this could cause confusion during long pauses.
Confirm this matches the intended UX - are players expected to know their inputs won't queue while paused, or should the client disable input controls?
558-562: Pause guard inendTurncorrectly freezes turn execution.When paused, no turns are broadcast to clients, effectively freezing game state for all players. This pairs well with the intent-dropping logic above.
498-498: Good:isLobbyCreatorpropagated to all clients via player records.This aligns with the approach discussed in past reviews - storing lobby creator status in PlayerSchema so all clients know who the creator is. This enables future features like in-game kicks without additional round trips.
src/client/LocalServer.ts (2)
124-127: LGTM!The guard correctly blocks non-pause intents during replay or while paused, and the comment clearly explains the intent.
178-181: LGTM!The pause gate in
endTurn()correctly prevents turn execution while paused, and works correctly with the pause/unpause flows in lines 111-121.
| if (clientMsg.intent.type === "toggle_pause") { | ||
| if (clientMsg.intent.paused) { | ||
| // Pausing: add intent and end turn before pause takes effect | ||
| this.intents.push(clientMsg.intent); | ||
| this.endTurn(); | ||
| this.paused = true; | ||
| } else { | ||
| // Unpausing: clear pause flag before adding intent so next turn can execute | ||
| this.paused = false; | ||
| this.intents.push(clientMsg.intent); | ||
| this.endTurn(); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure
git ls-files | head -20Repository: openfrontio/OpenFrontIO
Length of output: 632
🏁 Script executed:
# Search for GameRunner or ExecutionManager classes
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "class GameRunner\|class ExecutionManager" 2>/dev/null | head -10Repository: openfrontio/OpenFrontIO
Length of output: 91
🏁 Script executed:
# Search for how pause intents are handled during replay
rg -n "toggle_pause|TogglePauseIntent|isReplay" --type ts -A 3 -B 3 | head -100Repository: openfrontio/OpenFrontIO
Length of output: 5847
🏁 Script executed:
# Check LocalServer.ts for the toggle_pause handling and endTurn method
cat -n src/client/LocalServer.ts | sed -n '105,190p'Repository: openfrontio/OpenFrontIO
Length of output: 3568
🏁 Script executed:
# Search for pause overlay or UI components related to replay
rg -n "PauseOverlay|pause.*replay|replay.*pause" --type ts --type tsx -C 2Repository: openfrontio/OpenFrontIO
Length of output: 94
🏁 Script executed:
# Check GameRunner for pause intent handling and replay mode
cat -n src/core/GameRunner.ts | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1920
🏁 Script executed:
# Search for PauseExecution class
rg -n "class PauseExecution" --type ts -A 20Repository: openfrontio/OpenFrontIO
Length of output: 1343
🏁 Script executed:
# Search for UI components or conditions preventing pause during replay
rg -n "pause.*button\|PauseButton\|canPause\|allowPause" --type ts --type jsx -C 3Repository: openfrontio/OpenFrontIO
Length of output: 94
🏁 Script executed:
# Check how archived replay intents are used and if they include pause intents
rg -n "replayTurns\|gameRecord" --type ts -C 2 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there's any code preventing toggle_pause during replay in UI or configuration
rg -n "gameRecord" --type ts -B 3 -A 3 | grep -A 5 -B 5 "pause\|toggle" | head -40Repository: openfrontio/OpenFrontIO
Length of output: 685
🏁 Script executed:
# Check Game config and replay related code
rg -n "isReplay\|gameRecord" --type ts | grep -v node_modules | head -30Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Look at how the game handles pause state during replay more completely
cat -n src/core/execution/PauseExecution.tsRepository: openfrontio/OpenFrontIO
Length of output: 804
🏁 Script executed:
# Search for any UI code that might prevent pause button during replay
find . -type f \( -name "*.ts" -o -name "*.js" \) | xargs grep -l "pause.*disable\|disabled.*pause" 2>/dev/null | head -5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check game.setPaused to see if it has any replay safeguards
rg -n "setPaused" --type ts -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 1935
🏁 Script executed:
# Check if UI prevents sending toggle_pause during replay
rg -n "gameRecord\|isReplay" --type ts | xargs -I {} sh -c 'grep -l "pause\|Pause" $(echo {} | cut -d: -f1)' | sort -u | head -10Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Look for client-side code that might disable pause during replay
find . -type f -name "*.ts" | xargs grep -l "clientMsg.intent.type.*pause\|toggle_pause" 2>/dev/nullRepository: openfrontio/OpenFrontIO
Length of output: 243
🏁 Script executed:
# Check if there's any filtering of intents based on replay status before they reach LocalServer
rg -n "sendIntent\|clientMsg" --type ts -B 3 -A 3 | grep -B 5 -A 5 "gameRecord\|isReplay" | head -50Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# More complete view of how pause intents are handled - check if archived intents can contain pause intents
rg -n "turn.*intents\|intents.*turn" --type ts | grep -i "replay\|gamerecord\|archive" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check ClientGameRunner for pause handling and replay mode
rg -n "pause\|toggle_pause\|gameRecord\|isReplay" src/client/ClientGameRunner.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check the full context of how intents are sent from client
cat -n src/client/ClientGameRunner.ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 3839
🏁 Script executed:
# Verify: does PauseExecution even execute during replay? Check if replayed turns include pause intents
rg -n "archiv\|replay.*intent\|intent.*replay" --type ts -C 3 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if pause intents can exist in archived turns
rg -n "TogglePauseIntent" --type ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 1570
🏁 Script executed:
# Check if singleplayer has special handling for pause during replay
rg -n "gameType.*Singleplayer\|Singleplayer.*replay" --type ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Verify: are singleplayer replays even possible? How is replay initiated?
rg -n "gameRecord\|replayTurns" src/client/LocalServer.ts -B 5 -A 5 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check the Turn schema to see if pause intents should be in archived turns
rg -n "type Turn\|TurnSchema" src/core/Schemas.ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check how replay is initiated and what gameRecord contains
rg -n "lobbyConfig.gameRecord" --type ts -B 3 -A 3 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 2901
🏁 Script executed:
# Verify the complete flow: does toggle_pause work during singleplayer replay?
cat -n src/client/LocalServer.ts | sed -n '30,50p'Repository: openfrontio/OpenFrontIO
Length of output: 793
🏁 Script executed:
# Look for any UI code that might disable pause button during replay
find . -type f -name "*.ts" | xargs grep -l "paused\|PauseOverlay" 2>/dev/null | head -10Repository: openfrontio/OpenFrontIO
Length of output: 379
🏁 Script executed:
# Check if PauseOverlay or pause UI is disabled during replay
cat src/client/graphics/layers/HeadsUpMessage.ts | grep -n "pause\|Pause" -i -C 3Repository: openfrontio/OpenFrontIO
Length of output: 946
🏁 Script executed:
# Search for pause button/control in Graphics or UI layers
find . -type f -name "*.ts" | xargs grep -l "pause.*button\|button.*pause\|disable.*pause" 2>/dev/null | head -5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check Transport to see if it blocks pause intent sending during replay
rg -n "SendPause\|toggle_pause" src/client/Transport.ts -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Verify the complete picture: How does replay speed work? Can user interact during replay?
rg -n "replaySpeed\|isReplay" src/client/LocalServer.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there's any code that prevents sending pause intent during replay from UI
rg -n "SendIntent\|sendIntent" src/client/Transport.ts -B 5 -A 5 | head -40Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Final check: is there any mention of pause being blocked during replay in comments or code?
rg -n "replay.*pause\|pause.*replay" src/client/ --type ts -i -C 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if singleplayer replays use the same PauseExecution logic
rg -n "Singleplayer" src/core/execution/PauseExecution.ts -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 315
🏁 Script executed:
# Verify: does LocalServer.isReplay have different behavior than checking lobbyConfig.gameRecord?
cat -n src/client/LocalServer.ts | sed -n '47,90p'Repository: openfrontio/OpenFrontIO
Length of output: 1660
🏁 Script executed:
# Final verification: can pause work during singleplayer replay?
rg -n "isReplay.*paused\|paused.*isReplay" src/client/LocalServer.ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 49
Guard toggle_pause against replay or document the behavior.
The toggle_pause handler doesn't check for replay mode (lines 110-122), so if sent during replay, the intent is pushed to this.intents but then discarded when endTurn() replaces intents with archived replay intents (line 187). In singleplayer replay, pause should work since the replayer is the player, but the discarded intent means PauseExecution never processes it.
Either:
- Guard toggle_pause against replay if pause should be blocked during replay, OR
- Preserve pause intents during replay if pause should work, OR
- Add a comment explaining why pause intents are silently discarded
🤖 Prompt for AI Agents
In src/client/LocalServer.ts around lines 110 to 123, the toggle_pause handler
currently pushes pause intents even during replay which causes them to be
overwritten/ignored later; update the handler to first check replay mode and
bail out (or explicitly document the behavior) — i.e., if this.replaying (or
equivalent flag) is true, do not push the pause intent or call endTurn(),
instead return early (optionally log/debug message), otherwise proceed with the
existing push/endTurn/pause logic so pause only takes effect outside replay.
Makes sense! I filtered in ClientGameRunner before sending turns to the worker since it already has access to lobbyConfig.gameRecord and avoids modifying worker messages. Thoughts? |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
4 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #2491
Description:
Adds pause/unpause functionality for private multiplayer games. Only the lobby creator can pause the game, and all players see a pause overlay when the game is paused.
Key features:
Implementation details:
isPaused) prevents turn execution during pauseisLobbyCreatorflag inGameStartInfoto show/hide pause buttonTogglePauseIntentthat broadcasts to all clients viaNoOpExecutionPauseOverlaycomponent (shows in single player also)Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
furo18