lobby websocket instead of polling #2727
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces periodic HTTP polling with a WebSocket push path for public lobbies. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as PublicLobby (UI)
participant Socket as PublicLobbySocket
participant Server as Master (WS)
participant HTTP as /api/public_lobbies
UI->>Socket: start()
Socket->>Server: WS CONNECT /lobbies
note right of Server `#D6EAF8`: add client to connectedClients\nsend current lobbies immediately
Server-->>Socket: {"type":"lobbies_update","lobbies":[...]}
Socket-->>UI: onLobbiesUpdate(lobbies)
UI->>UI: handleLobbiesUpdate()\n(update state, compute times, load maps)
rect rgb(230,255,230)
note over Server: On lobby data change
Server-->>Socket: broadcast lobbies_update
Socket-->>UI: onLobbiesUpdate(lobbies)
UI->>UI: handleLobbiesUpdate()
end
rect rgb(255,245,230)
note over Socket: If WS closes repeatedly\nand max attempts reached
Socket->>HTTP: GET /api/public_lobbies (fallback poll)
HTTP-->>Socket: JSON lobbies
Socket-->>UI: onLobbiesUpdate(lobbies)
end
UI->>Socket: stop()
Socket->>Server: WS CLOSE
Socket->>Socket: clear timers & intervals
UI->>UI: reset UI state (currLobby, highlights)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (2)
src/server/Master.ts (1)
68-82: Avoid double JSON parse/stringify overhead.
publicLobbiesJsonStris already a JSON string. Parsing it just to stringify it again in the message wastes CPU cycles. You can build the message directly using string concatenation or store the lobbies object separately.Proposed fix
function broadcastLobbies() { - const message = JSON.stringify({ - type: "lobbies_update", - data: JSON.parse(publicLobbiesJsonStr || '{"lobbies":[]}'), - }); + const lobbiesData = publicLobbiesJsonStr || '{"lobbies":[]}'; + const message = `{"type":"lobbies_update","data":${lobbiesData}}`; connectedClients.forEach((client) => { if (client.readyState === WebSocket.OPEN) { client.send(message); } else { connectedClients.delete(client); } }); }src/client/PublicLobby.ts (1)
126-132: Fetch lobbies immediately when starting fallback polling.When WebSocket fails and you fall back to HTTP, there's a 1-second delay before the first fetch. For a better user experience, fetch immediately on fallback start.
Proposed fix
private startFallbackPolling() { if (this.fallbackPollInterval !== null) return; console.log("Starting HTTP fallback polling"); + this.fetchLobbiesHTTP(); // Fetch immediately this.fallbackPollInterval = window.setInterval(() => { this.fetchLobbiesHTTP(); }, 1000); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/PublicLobby.tssrc/server/Master.ts
🧰 Additional context used
🧠 Learnings (2)
📓 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-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/PublicLobby.ts
🧬 Code graph analysis (1)
src/client/PublicLobby.ts (3)
src/core/Schemas.ts (2)
GameInfo(130-136)GameID(25-25)src/core/game/GameView.ts (1)
gameID(910-912)src/client/TerrainMapFileLoader.ts (1)
terrainMapFileLoader(4-4)
🪛 GitHub Actions: 🧪 CI
src/client/PublicLobby.ts
[error] 1-1: Prettier formatting check failed. Code style issues found in src/client/PublicLobby.ts. Run 'prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (5)
src/server/Master.ts (2)
95-119: WebSocket setup is clean. Consider adding a heartbeat mechanism for production.The connection, close, and error handlers work well. Two things to note:
Lines 103-108 duplicate the JSON parse/stringify pattern from
broadcastLobbies. Extract this into a shared helper or use the string concatenation fix from above.For production, WebSocket connections can silently fail without triggering close events. A ping/pong heartbeat helps detect stale connections. This is optional for a draft PR but worth adding before release.
273-281: Good optimization: broadcast only when lobbies change.Comparing JSON strings to detect changes and broadcasting only on update reduces unnecessary network traffic. This is a solid approach.
src/client/PublicLobby.ts (3)
96-104: Same double-counting issue in the catch block.When the constructor throws, you increment attempts and then potentially call
scheduleReconnect, which will create a new connection that may also fail. Thecloseevent won't fire here since no connection was established. This path is fine, but the logic should be consistent with the event handlers above.Consider whether you want the catch block to also trigger fallback if attempts are exhausted:
} catch (error) { console.error("Error connecting WebSocket:", error); this.wsConnectionAttempts++; if (this.wsConnectionAttempts >= this.maxWsAttempts) { this.startFallbackPolling(); } else { this.scheduleReconnect(); } }This part looks correct. Just verify the interaction with event-based retry logic once you remove the increment from the error handler.
152-165: Clean centralized handler for lobby updates.Good refactor. The
handleLobbiesUpdatemethod handles both WebSocket and HTTP sources uniformly. The logic for tracking lobby start times and loading map images is straightforward.
282-288: Good cleanup in stop() method.Properly disconnects WebSocket, stops fallback polling, and resets UI state. This avoids resource leaks when the component is stopped.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/PublicLobby.ts (1)
81-97: The double-counting issue may have been reintroduced.The error handler (line 96) still increments
wsConnectionAttempts, causing each failed connection to count twice (error fires before close in browsers). This was previously flagged and marked as addressed in commit 423f4d4, but the increment remains in the current code.Proposed fix: Remove increment from error handler
this.ws.addEventListener("error", (error) => { console.error("WebSocket error:", error); - this.wsConnectionAttempts++; + // Attempt counting happens in close handler });
🧹 Nitpick comments (4)
src/client/PublicLobby.ts (4)
109-118: Consider explicitly removing WebSocket event listeners.While setting
this.ws = nulltypically allows garbage collection, explicitly removing the event listeners ensures cleanup, especially since the handlers referencethis(component instance). Store handler references or useAbortControllerfor cleaner removal.Proposed pattern using AbortController
+ private wsAbortController: AbortController | null = null; private connectWebSocket() { try { const protocol = window.location.protocol === "https:" ? "wss:" : "ws:"; const wsUrl = `${protocol}//${window.location.host}/socket`; this.ws = new WebSocket(wsUrl); + this.wsAbortController = new AbortController(); + const signal = this.wsAbortController.signal; - this.ws.addEventListener("open", () => { + this.ws.addEventListener("open", () => { // ... existing code - }); + }, { signal }); - this.ws.addEventListener("message", (event) => { + this.ws.addEventListener("message", (event) => { // ... existing code - }); + }, { signal }); - this.ws.addEventListener("close", () => { + this.ws.addEventListener("close", () => { // ... existing code - }); + }, { signal }); - this.ws.addEventListener("error", (error) => { + this.ws.addEventListener("error", (error) => { // ... existing code - }); + }, { signal }); } catch (error) { // ... existing catch block } } private disconnectWebSocket() { + if (this.wsAbortController) { + this.wsAbortController.abort(); + this.wsAbortController = null; + } if (this.ws) { this.ws.close(); this.ws = null; } if (this.wsReconnectTimeout !== null) { clearTimeout(this.wsReconnectTimeout); this.wsReconnectTimeout = null; } }
128-134: Extract hardcoded polling interval to a configurable property.The fallback polling interval (1000ms) is hardcoded, while other timing values (
wsReconnectDelay,debounceDelay) are class properties. For consistency and easier tuning, extract this to a property.Proposed refactor
private wsReconnectDelay: number = 3000; + private fallbackPollDelay: number = 1000; private fallbackPollInterval: number | null = null; // ... private startFallbackPolling() { if (this.fallbackPollInterval !== null) return; console.log("Starting HTTP fallback polling"); this.fallbackPollInterval = window.setInterval(() => { this.fetchLobbiesHTTP(); - }, 1000); + }, this.fallbackPollDelay); }
51-107: Consider adding a connection timeout for the WebSocket.If the server doesn't respond, the WebSocket connection will hang until the browser's default timeout. Adding an explicit timeout provides better control over failure detection and faster fallback to HTTP polling.
Example timeout pattern
private wsConnectionTimeout: number | null = null; private wsConnectionTimeoutDelay: number = 5000; // 5 seconds private connectWebSocket() { try { const protocol = window.location.protocol === "https:" ? "wss:" : "ws:"; const wsUrl = `${protocol}//${window.location.host}/socket`; this.ws = new WebSocket(wsUrl); // Set connection timeout this.wsConnectionTimeout = window.setTimeout(() => { if (this.ws && this.ws.readyState !== WebSocket.OPEN) { console.warn("WebSocket connection timeout"); this.ws.close(); // The close handler will handle reconnect/fallback logic } }, this.wsConnectionTimeoutDelay); this.ws.addEventListener("open", () => { console.log("WebSocket connected to lobby updates"); // Clear connection timeout on successful open if (this.wsConnectionTimeout !== null) { clearTimeout(this.wsConnectionTimeout); this.wsConnectionTimeout = null; } this.wsConnectionAttempts = 0; // ... rest of open handler }); // ... rest of event handlers } catch (error) { // ... existing catch block } } private disconnectWebSocket() { if (this.wsConnectionTimeout !== null) { clearTimeout(this.wsConnectionTimeout); this.wsConnectionTimeout = null; } // ... rest of disconnectWebSocket }
109-118: Optionally check WebSocket readyState before calling close().Calling
close()on an already-closed WebSocket is safe (no-op), but checkingreadyStateis defensive and can provide clearer logging.Optional defensive check
private disconnectWebSocket() { if (this.ws) { - this.ws.close(); + if (this.ws.readyState === WebSocket.OPEN || this.ws.readyState === WebSocket.CONNECTING) { + this.ws.close(); + } this.ws = null; } if (this.wsReconnectTimeout !== null) { clearTimeout(this.wsReconnectTimeout); this.wsReconnectTimeout = null; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/PublicLobby.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-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/PublicLobby.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/PublicLobby.ts
🧬 Code graph analysis (1)
src/client/PublicLobby.ts (3)
src/core/Schemas.ts (2)
GameInfo(130-136)GameID(25-25)src/core/game/GameView.ts (1)
gameID(910-912)src/client/TerrainMapFileLoader.ts (1)
terrainMapFileLoader(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/PublicLobby.ts (1)
160-173: Memory leak: stale lobby entries not cleaned up.When lobbies are removed from updates, their entries persist in
lobbyIDToStartandmapImages. Over long sessions, stale data accumulates. ThemapImagesentries may hold blob URLs that won't be garbage-collected without explicit revocation.Proposed fix: Prune stale entries
private handleLobbiesUpdate(lobbies: GameInfo[]) { + // Build set of current lobby IDs + const currentIDs = new Set(lobbies.map(l => l.gameID)); + + // Remove stale entries from lobbyIDToStart + for (const gameID of this.lobbyIDToStart.keys()) { + if (!currentIDs.has(gameID)) { + this.lobbyIDToStart.delete(gameID); + } + } + + // Remove stale entries from mapImages and revoke blob URLs + for (const gameID of this.mapImages.keys()) { + if (!currentIDs.has(gameID)) { + const imageUrl = this.mapImages.get(gameID); + if (imageUrl && imageUrl.startsWith('blob:')) { + URL.revokeObjectURL(imageUrl); + } + this.mapImages.delete(gameID); + } + } + this.lobbies = lobbies; this.lobbies.forEach((l) => { if (!this.lobbyIDToStart.has(l.gameID)) { const msUntilStart = l.msUntilStart ?? 0; this.lobbyIDToStart.set(l.gameID, msUntilStart + Date.now()); } if (l.gameConfig && !this.mapImages.has(l.gameID)) { this.loadMapImage(l.gameID, l.gameConfig.gameMap); } }); this.requestUpdate(); }
🧹 Nitpick comments (1)
src/client/PublicLobby.ts (1)
52-112: Double-counting fix looks good; minor edge case with flag.The
wsAttemptCountedflag properly prevents double counting between error and close events for a single connection attempt, addressing the prior review concern.However, there's a subtle edge case: the flag is reset to false each time
connectWebSocketis called (line 58), but old WebSocket instances retain their event listeners. If a previous WebSocket's close event fires after you've already started a new connection attempt, it could incrementwsConnectionAttemptsunexpectedly. In practice, this is unlikely because the close event should complete before the reconnect delay (3 seconds) expires.Consider storing a connection ID or generation counter if you observe reconnection issues in production.
Optional: More robust approach using per-connection flag
private connectWebSocket() { try { const protocol = window.location.protocol === "https:" ? "wss:" : "ws:"; const wsUrl = `${protocol}//${window.location.host}/socket`; this.ws = new WebSocket(wsUrl); let attemptCounted = false; // Per-connection flag instead of instance variable this.ws.addEventListener("close", () => { console.log("WebSocket disconnected, attempting to reconnect..."); if (!attemptCounted) { attemptCounted = true; this.wsConnectionAttempts++; } // ... rest of close handler }); // ... rest of setup } catch (error) { // ... error handling } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/PublicLobby.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-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/PublicLobby.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/PublicLobby.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/PublicLobby.ts
🧬 Code graph analysis (1)
src/client/PublicLobby.ts (3)
src/core/Schemas.ts (2)
GameInfo(130-136)GameID(25-25)src/core/game/GameView.ts (1)
gameID(910-912)src/client/TerrainMapFileLoader.ts (1)
terrainMapFileLoader(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (4)
src/client/PublicLobby.ts (4)
114-131: LGTM!Reconnect scheduling properly guards against multiple pending timeouts, and cleanup is thorough.
133-158: LGTM!HTTP fallback is straightforward with proper guards and error handling.
175-184: LGTM!Async map loading is clean with proper error handling.
290-296: LGTM!Public
stop()method provides comprehensive cleanup of WebSocket, polling, state, and animations.
ryanbarlow97
changed the title
There was a problem hiding this comment.
Pull request overview
This PR replaces HTTP polling with WebSocket connections for real-time game lobby updates. The implementation adds a WebSocket server on the master process and updates the client to use WebSocket connections with automatic fallback to HTTP polling if WebSocket connections fail after 3 attempts.
Key Changes:
- WebSocket server setup on
/socketendpoint with automatic client broadcast on lobby changes - Client WebSocket connection with reconnection logic and HTTP polling fallback mechanism
- Removal of interval-based polling in favor of push-based updates
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/server/Master.ts | Adds WebSocket server setup, client connection management, and broadcasts lobby updates to connected clients when lobbies change |
| src/client/PublicLobby.ts | Replaces HTTP polling with WebSocket connection, implements reconnection logic with fallback to HTTP polling after max attempts, and consolidates lobby update handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
store the json as an object always broadcast rename the path to /lobbies
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/LobbySocket.ts (1)
3-9: Export types for better developer experience.The
LobbyUpdateHandlerandLobbySocketOptionstypes are part of the public API but aren't exported. Consumers need these when creating instances or typing their callbacks.🔎 Proposed fix
-type LobbyUpdateHandler = (lobbies: GameInfo[]) => void; +export type LobbyUpdateHandler = (lobbies: GameInfo[]) => void; -interface LobbySocketOptions { +export interface LobbySocketOptions { reconnectDelay?: number; maxWsAttempts?: number; pollIntervalMs?: number; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/LobbySocket.tssrc/server/Master.tstests/client/LobbySocket.test.tsvite.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/Master.ts
- tests/client/LobbySocket.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 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-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/LobbySocket.ts
🧬 Code graph analysis (1)
src/client/LobbySocket.ts (1)
src/core/Schemas.ts (1)
GameInfo(130-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (7)
vite.config.ts (1)
103-107: LGTM! Proxy path aligns with WebSocket endpoint.The updated path
/lobbiesmatches the WebSocket URL constructed inLobbySocket.tsline 46, ensuring proper routing for the new WebSocket-based lobby updates.src/client/LobbySocket.ts (6)
23-31: Clean constructor with sensible defaults.The default configuration values are reasonable for production use: 3-second reconnect delay, 3 WebSocket attempts before HTTP fallback, and 1-second polling interval.
70-89: Good error handling with safe fallback.The message parsing uses optional chaining and provides an empty array fallback, preventing crashes from malformed messages. The decision to close the socket on parse errors is reasonable to avoid processing corrupt data streams.
91-105: Reconnection logic correctly prevents double-counting.The
wsAttemptCountedflag ensures connection attempts are only incremented once per connection try, even if both error and close events fire.
124-130: Good guard against concurrent reconnection attempts.The null check on line 125 prevents multiple reconnect timers from running simultaneously, avoiding connection storms.
143-150: LGTM! Fallback polling with immediate first fetch.The immediate call to
fetchLobbiesHTTP()on line 146 ensures the UI updates quickly when falling back from WebSocket, before starting the interval.
159-170: Add runtime validation before casting toGameInfo[]at line 166.Line 166 uses an unsafe type assertion:
data.lobbies as GameInfo[]. If the API returns unexpected data structure, this will cause runtime errors without any type checking. Validate the response structure before the cast—for example, check thatdata.lobbiesis an array and contains objects with the expected properties.The WebSocket path at line 74 also lacks validation but avoids the type assertion by using optional chaining (
?.). Both paths should validate the data before passing it toonLobbiesUpdate().⛔ Skipped due to learnings
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.
src/server/Master.ts
Outdated
| // Broadcast lobbies to all connected clients | ||
| function broadcastLobbies() { | ||
| const dataStr = JSON.stringify(publicLobbiesData); | ||
| const message = `{"type":"lobbies_update","data":${dataStr}}`; |
There was a problem hiding this comment.
instead, do:
const message = JSON.stringify({type: "lobbies_update", data: publicLobbiesData}))
src/server/Master.ts
Outdated
| const lobbiesJson = JSON.stringify(publicLobbiesData); | ||
| ws.send(`{"type":"lobbies_update","data":${lobbiesJson}}`); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/server/Master.ts (2)
63-88: Good implementation of broadcast with proper cleanup.The deferred deletion pattern avoids modifying the Set during iteration, and the message is stringified once before sending to all clients.
One optional improvement: the inline type
{ lobbies: GameInfo[] }could be extracted to a named type (e.g.,PublicLobbiesData) for reuse and clarity, especially if the client also defines a matching type.
101-130: Consider adding an error handler on the WebSocketServer.The connection-level error handling looks good. However, the
wssinstance itself can emit errors (though rare when attached to an existing server). Adding a handler prevents unhandled exceptions from crashing the master process.🔎 Suggested addition after line 102
const wss = new WebSocketServer({ server, path: "/lobbies" }); + wss.on("error", (error) => { + log.error("WebSocketServer error:", error); + }); + wss.on("connection", (ws: WebSocket) => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/Master.ts
🧰 Additional context used
🧠 Learnings (2)
📓 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-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/Master.ts
🧬 Code graph analysis (1)
src/server/Master.ts (1)
src/core/Schemas.ts (1)
GameInfo(130-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/server/Master.ts (3)
8-8: LGTM!Clean import of WebSocket types from the
wspackage.
215-217: LGTM!Using
res.json()sets the correctContent-Typeheader and serializes the object cleanly. This maintains compatibility with clients that fall back to HTTP polling.
283-288: LGTM!Updates the lobbies data and broadcasts to all connected clients in a single call. Clean and straightforward.
Description:
Changes game lobbies into websockets instead of polling
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n