Use WebSocket intent for lobby game configuration updates #2761
Conversation
WalkthroughMigrates lobby config updates from an HTTP PUT endpoint to a WebSocket intent flow: the UI dispatches a CustomEvent -> Main converts it to a typed event -> Transport sends an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as HostLobbyModal
participant Main as Main (Client)
participant Transport as Transport
participant Server as GameServer
User->>Modal: Change lobby settings
activate Modal
Modal->>Modal: Dispatch "update-game-config" CustomEvent (config)
deactivate Modal
Main->>Main: Listen -> handleUpdateGameConfig(event)
activate Main
Main->>Main: Create SendUpdateGameConfigIntentEvent(config)
Main->>Transport: Emit on event bus
deactivate Main
Transport->>Transport: onSendUpdateGameConfigIntent
activate Transport
Transport->>Server: Send intent { type: "update_game_config", clientID, config } over WS
deactivate Transport
Server->>Server: Receive update_game_config
activate Server
rect rgb(220,240,220)
note right of Server: Validation checks
Server->>Server: Is requester the creator?
Server->>Server: Is game public or started?
Server->>Server: Is gameType set to Public via WS?
end
alt Valid
Server->>Server: updateGameConfig -> persist & broadcast
else Invalid
Server->>Server: Log and ignore/reject
end
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 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/client/HostLobbyModal.ts (1)
811-812: Consider removingawaitbeforeputGameConfig().
putGameConfig()no longer returns a Promise (it just dispatches a synchronous event). Theawaiton line 812 is now a no-op.Suggested fix
private async startGame() { - await this.putGameConfig(); + this.putGameConfig(); console.log(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/HostLobbyModal.tssrc/client/Main.tssrc/client/Transport.tssrc/core/Schemas.tssrc/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/client/HostLobbyModal.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/HostLobbyModal.tssrc/client/Main.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/HostLobbyModal.tssrc/core/Schemas.tssrc/client/Main.ts
🧬 Code graph analysis (3)
src/client/HostLobbyModal.ts (2)
src/core/game/Game.ts (1)
HumansVsNations(56-56)src/core/Schemas.ts (1)
GameConfig(90-90)
src/client/Main.ts (3)
src/client/Transport.ts (1)
SendUpdateGameConfigIntentEvent(179-181)src/core/game/GameImpl.ts (1)
config(338-340)src/core/game/GameView.ts (1)
config(783-785)
src/client/Transport.ts (2)
src/core/EventBus.ts (1)
GameEvent(1-1)src/core/Schemas.ts (1)
GameConfig(90-90)
🔇 Additional comments (8)
src/core/Schemas.ts (2)
370-373: LGTM!Schema follows existing patterns well. Uses
BaseIntentSchema.extendandGameConfigSchema.partial()for partial updates, which matches the PR objective to preserve partial-update semantics.
51-52: LGTM!
UpdateGameConfigIntentis properly added to both theIntenttype union and theIntentSchemadiscriminated union.Also applies to: 399-399
src/server/GameServer.ts (1)
351-404: LGTM!Handler follows the existing pattern from
kick_player. Validation checks are correct:
- Creator-only access control
- No updates to public games
- No updates after game starts
- No changing gameType to Public
Structured logging with
configKeysis helpful for debugging.src/client/HostLobbyModal.ts (1)
755-793: LGTM!Clean migration from HTTP PUT to CustomEvent dispatch. The pattern matches the existing
kickPlayerevent dispatch. Usingbubbles: trueandcomposed: trueensures proper propagation through shadow DOM boundaries.src/client/Main.ts (2)
39-42: LGTM!Import grouped with related
SendKickPlayerIntentEvent.
195-198: LGTM!Event listener registration and handler follow the same pattern as
kick-player. The guard checkif (this.eventBus)is consistent withhandleKickPlayer.Also applies to: 642-649
src/client/Transport.ts (2)
179-181: LGTM!Event class follows existing patterns. Uses
Partial<GameConfig>which matches the schema definition.
268-271: LGTM!Handler registration and implementation follow the same pattern as
SendKickPlayerIntentEvent. The intent structure matchesUpdateGameConfigIntentSchema.Also applies to: 671-677
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
please also remove the endpoint: PUT /api/game/:id on Worker.ts:179 since it is no longer used.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
809-809: Remove unnecessaryawaiton synchronous method.The
putGameConfig()method is now synchronous (dispatches an event) and no longer returns a Promise. Theawaiton line 809 is unnecessary and misleading.🔎 Proposed fix
- await this.putGameConfig(); + this.putGameConfig();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.tssrc/server/GameServer.tssrc/server/Worker.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/Worker.tssrc/client/HostLobbyModal.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/client/HostLobbyModal.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/HostLobbyModal.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/core/game/Game.ts (1)
HumansVsNations(56-56)src/core/Schemas.ts (1)
GameConfig(90-90)
🔇 Additional comments (3)
src/server/GameServer.ts (1)
351-399: LGTM! Access controls and validation are solid.The WebSocket intent handler correctly enforces all necessary constraints:
- Only lobby creator can update config (line 353)
- Rejects updates for public games (line 362)
- Prevents updates after game start (line 370)
- Blocks changing to public gameType via WebSocket (line 381)
All checks properly return early with warning logs, and the config is applied via the existing
updateGameConfigmethod that handles partial updates.src/server/Worker.ts (1)
20-20: LGTM! Import cleanup matches endpoint removal.The removal of
GameInputSchemafrom imports correctly reflects that the PUT/api/game/:idendpoint has been removed in favor of WebSocket-based updates.src/client/HostLobbyModal.ts (1)
755-791: LGTM! Event dispatch correctly replaces HTTP PUT.The refactored method properly dispatches a CustomEvent with:
- Correct event name "update-game-config" (line 757)
- Complete config payload matching
Partial<GameConfig>(lines 759-785)- Proper event propagation flags (bubbles, composed) for shadow DOM (lines 787-788)
- Preserved conditional logic for
disableNations(HumansVsNations mode) andmaxTimerValue
|
Thanks for the suggestions @evanpelle . |
|
@VahantSharma can you make a new PR to make it so that private lobbies also use WebSockets to update instead of polling? 
(same approach to what i did for public lobbies) |
3 participants
Fixes #2758
Description:
This PR migrates lobby configuration updates from the HTTP PUT
/game/:idendpoint to a WebSocket-based intent flow.The lobby creator is already authenticated via the game WebSocket, so updating configuration through intents removes redundant authentication and aligns with existing real-time lobby actions such as
kick_playerandtoggle_pause.Changes Made
update_game_configWebSocket intent schemaputGameConfig()to emit WebSocket intent instead of HTTP fetchTesting
PUT /api/game/:idrequests are made from the lobby UIChecklist: