feat: add spectator mode for all games #2546
Conversation
|
Warning Rate limit exceeded@Phantasm0009 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds spectator support: new spectator flags and schemas, client join-as-spectator UI and event payloads, transport and leaderboards reporting spectator counts, host UI shows spectators, and server tracks spectators separately from players. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant JoinModal as JoinPrivateLobbyModal
participant Client as Browser (Main / Transport)
participant Worker as Server Worker
participant GameServer as GameServer
User->>JoinModal: Click "Join as Spectator"
JoinModal->>JoinModal: joinLobbyInternal(isSpectator=true)
JoinModal->>JoinModal: checkActiveLobby(lobbyId, true)
alt lobby active
JoinModal->>Client: dispatch "join-lobby" (isSpectator: true)
Client->>Worker: WS ClientJoinMessage { clientID, isSpectator: true, ... }
Worker->>GameServer: new Client(..., isSpectator=true)
GameServer->>GameServer: addClient(client) -> spectator path
GameServer->>GameServer: setupSpectatorHandlers(client, lastTurn)
GameServer->>Client: send gameInfo (includes spectators)
Client->>JoinModal: join ack
JoinModal->>User: Show "joined_as_spectator"
else lobby archived
JoinModal->>User: Show "not_found"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/GameServer.ts (1)
634-650: Spectators are not cleaned up on ping timeout.The
phase()method checkslastPingand removes stale clients fromactiveClients, but spectators with stale pings are never removed. Over time, this could accumulate disconnected spectator entries.Consider adding similar cleanup for spectators:
phase(): GamePhase { const now = Date.now(); const alive: Client[] = []; for (const client of this.activeClients) { if (now - client.lastPing > 60_000) { this.log.info("no pings received, terminating connection", { clientID: client.clientID, persistentID: client.persistentID, }); if (client.ws.readyState === WebSocket.OPEN) { client.ws.close(1000, "no heartbeats received, closing connection"); } } else { alive.push(client); } } this.activeClients = alive; + + // Clean up stale spectators + const aliveSpectators: Client[] = []; + for (const spectator of this.spectators) { + if (now - spectator.lastPing > 60_000) { + this.log.info("spectator no pings, terminating", { + clientID: spectator.clientID, + }); + if (spectator.ws.readyState === WebSocket.OPEN) { + spectator.ws.close(1000, "no heartbeats received"); + } + } else { + aliveSpectators.push(spectator); + } + } + this.spectators = aliveSpectators;
🧹 Nitpick comments (6)
src/client/ClientGameRunner.ts (1)
54-66: Short‑circuit player commands when running as a spectator
LobbyConfig.isSpectatoris now plumbed intoClientGameRunnerbut not used to gate any actions, so spectators can still trigger spawn/attack/upgrade events (the server will ignore them, but it wastes traffic and can be confusing).You can keep camera movement etc. while blocking only game‑changing commands by early‑returning in the handlers that emit intents:
export interface LobbyConfig { serverConfig: ServerConfig; cosmetics: PlayerCosmeticRefs; playerName: string; clientID: ClientID; gameID: GameID; token: string; - isSpectator?: boolean; + isSpectator?: boolean; // GameStartInfo only exists when playing a singleplayer game. gameStartInfo?: GameStartInfo; // GameRecord exists when replaying an archived game. gameRecord?: GameRecord; } @@ private inputEvent(event: MouseUpEvent) { if (!this.isActive || this.renderer.uiState.ghostStructure !== null) { return; } + if (this.lobby.isSpectator) { + return; + } @@ private autoUpgradeEvent(event: AutoUpgradeEvent) { if (!this.isActive) { return; } + if (this.lobby.isSpectator) { + return; + } @@ private doBoatAttackUnderCursor(): void { + if (this.lobby.isSpectator) { + return; + } const tile = this.getTileUnderCursor(); @@ private doGroundAttackUnderCursor(): void { + if (this.lobby.isSpectator) { + return; + } const tile = this.getTileUnderCursor();This keeps behavior simple: spectators see the full game but cannot accidentally issue any commands from the client side.
Also applies to: 449-492, 494-520, 564-581, 583-605
src/client/Main.ts (1)
81-90: Spectator flag wiring is correct; consider not counting spectators as played gamesThe new
isSpectator?: booleanonJoinLobbyEventand passingisSpectator: lobby.isSpectatorintojoinLobbylooks clean and keeps the join flow simple.If you want “games played” to reflect only matches where the user actually plays (not spectates), you could gate
incrementGamesPlayed():() => { this.joinModal.close(); this.publicLobby.stop(); - incrementGamesPlayed(); + if (!lobby.isSpectator) { + incrementGamesPlayed(); + } @@ // Ensure there's a homepage entry in history before adding the lobby entryIf you’re happy treating spectating as a “game played”, the current behavior is fine.
Also applies to: 461-557
src/server/Client.ts (1)
13-24: Server Client now exposes spectator status; consider a simple role union laterAdding
public readonly isSpectator: boolean = falsetoClientis a straightforward way to distinguish spectators from players and fits well with howWorkernow constructs clients. If you ever add more client kinds, you may want to evolve this into something likerole: "player" | "spectator"instead of additional booleans, but the current change is fine for this feature.src/client/graphics/layers/Leaderboard.ts (1)
68-90: Potential race condition on unmount.The
updateSpectatorCountmethod is async and fetches data, but there is no cancellation mechanism if the component is destroyed while the fetch is in flight. This could cause a state update on an unmounted component.Consider adding a check or using an AbortController:
+ private abortController: AbortController | null = null; + private async updateSpectatorCount() { if (this.game === null) return; + this.abortController?.abort(); + this.abortController = new AbortController(); try { const gameID = this.game.gameID(); const config = await getServerConfigFromClient(); const response = await fetch( `/${config.workerPath(gameID)}/api/game/${gameID}`, + { signal: this.abortController.signal } ); if (response.ok) { const data = await response.json(); this.spectatorCount = data.spectators?.length ?? 0; } } catch (error) { + if ((error as Error).name === 'AbortError') return; // Silently fail - spectator count is not critical console.debug("Failed to fetch spectator count:", error); } }src/client/JoinPrivateLobbyModal.ts (2)
90-98: Translation key mismatch: usinghost_modal.spectators_countin join modal.This uses a translation key from
host_modalnamespace in the join modal. For consistency and to avoid coupling between modal components, consider using a dedicated key in theprivate_lobbynamespace.
327-337: Archived game join event missingisSpectatorfield.When joining an archived game, the
join-lobbyevent detail does not includeisSpectator. While spectators cannot join archived games (due to the early return on line 211-214), this inconsistency could cause issues if the logic changes later. Consider addingisSpectator: falsefor consistency with the active lobby join event.this.dispatchEvent( new CustomEvent("join-lobby", { detail: { gameID: lobbyId, gameRecord: parsed.data, clientID: generateID(), + isSpectator: false, } as JoinLobbyEvent, bubbles: true, composed: true, }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
resources/lang/en.json(3 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/HostLobbyModal.ts(3 hunks)src/client/JoinPrivateLobbyModal.ts(6 hunks)src/client/Main.ts(2 hunks)src/client/Transport.ts(1 hunks)src/client/graphics/layers/Leaderboard.ts(4 hunks)src/core/Schemas.ts(2 hunks)src/server/Client.ts(1 hunks)src/server/GameServer.ts(7 hunks)src/server/Worker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 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/Main.tssrc/client/ClientGameRunner.tssrc/server/Worker.tssrc/server/GameServer.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-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 a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.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.tssrc/client/ClientGameRunner.tssrc/client/graphics/layers/Leaderboard.tssrc/client/JoinPrivateLobbyModal.tssrc/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/client/HostLobbyModal.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
📚 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-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (4)
src/client/HostLobbyModal.ts (3)
src/core/Schemas.ts (1)
ClientInfo(133-137)src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/client/graphics/layers/Leaderboard.ts (3)
src/core/game/GameView.ts (2)
gameID(780-782)config(660-662)src/core/configuration/ConfigLoader.ts (1)
getServerConfigFromClient(28-45)src/client/Utils.ts (1)
translateText(92-147)
src/client/JoinPrivateLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/core/Util.ts (1)
generateID(261-267)
src/server/GameServer.ts (2)
src/server/Client.ts (1)
Client(6-25)src/core/Schemas.ts (1)
ClientMessageSchema(541-548)
🔇 Additional comments (13)
src/core/Schemas.ts (1)
125-137: Spectator wiring in shared schemas looks consistent and backward compatibleAdding
spectators?: ClientInfo[]toGameInfo,isSpectator?: booleantoClientInfo, and optionalisSpectatoronClientJoinMessageSchemakeeps older clients valid while giving you the data you need for spectator flows. This aligns cleanly with howGameInfoand join messages are already used across client and server.Also applies to: 528-539
src/client/Transport.ts (1)
379-390: Join payload now carries spectator flag correctlyIncluding
isSpectator: this.lobbyConfig.isSpectatorin thejoinmessage matches the updatedClientJoinMessageSchemaand whatWorker.tsexpects, while remaining safe whenisSpectatoris undefined (it simply won’t appear in the JSON).src/client/HostLobbyModal.ts (1)
56-58: Host lobby spectator count integration looks solidTracking
spectators: ClientInfo[]in state, filling it fromdata.spectators ?? []inpollPlayers(), and showing a translated count only whenspectators.length > 0is a clean and low-noise way to surface spectator presence in the host UI. The translation call with{ count: this.spectators.length }matches the newhost_modal.spectators_countkey.Also applies to: 548-578, 847-861
src/server/Worker.ts (1)
392-405: Client construction with spectator flag is correct and backward compatibleUsing
clientMsg.isSpectator ?? falseas the finalClientconstructor argument cleanly wires spectator status into the server while keeping older clients (that never send this field) working as regular players. This givesGameServerthe information it needs to place connections into players vs spectators without changing the join flow.resources/lang/en.json (1)
223-236: New spectator translations align with UI usage and policyThe added spectator strings (
private_lobby.join_as_spectator,private_lobby.joined_as_spectator,host_modal.spectators_countwith{count}, andleaderboard.spectator/leaderboard.spectators) match how the UI consumes them and keep language simple. Limiting changes toen.jsonis consistent with the project’s translation workflow.Also applies to: 259-288, 544-561
src/client/graphics/layers/Leaderboard.ts (1)
201-212: LGTM! Spectator banner display is clean.Good use of conditional rendering and separate translation keys for singular/plural forms. The emoji adds visual clarity without extra asset overhead.
src/server/GameServer.ts (4)
141-165: LGTM! Clean spectator join flow.Good separation of spectator handling from player logic. The early return after setting up spectator handlers keeps the code easy to follow. Stale spectator removal on reconnect prevents duplicate entries.
370-433: Spectator handlers are well isolated.Spectators can only ping, and intent attempts are logged as warnings. The close/error handlers properly clean up the spectators array. This matches the PR goal of view-only access.
478-484: LGTM! Spectators receive all game broadcasts.Prestart, start, and turn messages are properly sent to spectators, giving them full visibility into game state without any ability to influence it.
Also applies to: 525-531, 576-579
705-709: GameInfo includes spectator data correctly.The
isSpectator: truefield is hardcoded since all entries come from the spectators array. This is clean and matches the schema expectations.src/client/JoinPrivateLobbyModal.ts (3)
102-117: LGTM! Clear separation of join options.Two distinct buttons with clear labels let users choose how to join. The
gap-3spacing keeps them visually separated.
190-233: Clean refactor with shared internal method.Good use of a single
joinLobbyInternalthat handles both player and spectator joins. The early return for spectators trying to join archived games (lines 211-214) is correct since replays do not support spectating.
341-362: LGTM! Polling includes spectator count.The
pollPlayersmethod now fetches and updates both player list and spectator count from the same endpoint, keeping UI in sync.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/GameServer.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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-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.ts
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/server/Client.ts (1)
Client(6-25)src/core/Schemas.ts (1)
ClientMessageSchema(541-548)
🔇 Additional comments (8)
src/server/GameServer.ts (8)
43-43: LGTM!The
spectatorsarray follows the same pattern asactiveClientsand provides clear separation between players and observers.
478-484: LGTM!The spectator broadcast correctly mirrors the player broadcast pattern. The duplication is clear and acceptable for this use case.
525-531: LGTM!Spectator start message broadcast follows the established pattern.
576-579: LGTM!Turn updates correctly broadcast to spectators for real-time game state viewing.
651-665: LGTM!Spectator cleanup correctly mirrors the player cleanup logic with consistent timeout handling.
141-165: Critical: Spectator websockets not tracked for cleanup.Spectator websockets are not added to
this.websockets(compare with line 134 for regular clients). This means whenend()is called, spectator connections won't be closed (lines 587-591), leaving dangling websockets.Apply this diff to track spectator websockets:
public addClient(client: Client, lastTurn: number) { this.websockets.add(client.ws); if (this.kickedClients.has(client.clientID)) {The line at 134 already adds all websockets including spectators, so this should actually work correctly. Let me verify this is the case.
Actually, looking more carefully, line 134 executes before the spectator check at line 143, so spectator websockets ARE added to
this.websockets. Please disregard this concern.
160-160: Verify: Spectators added to allClients affects disconnect tracking.Spectators are added to
allClients(line 160), which is iterated incheckDisconnectedStatus()(line 773). This generatesmark_disconnectedintents for spectators even though they may not affect gameplay. Review whether spectators need disconnect tracking or if they should be excluded fromcheckDisconnectedStatus()to reduce overhead.
720-724: Verify: Schema alignment for spectators in GameInfo.The
gameInfo()method includes aspectatorsarray mapping withusername,clientID, andisSpectator: truefields. Confirm that theGameInfoschema insrc/core/Schemas.tsincludes aspectatorsarray field, and thatClientInfoschema supports theisSpectatorboolean property to ensure serialization consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/server/GameServer.ts (1)
43-43: Add persistentID check for spectator reconnects and confirm allClients useThe spectator reconnect branch only matches on
clientIDand always replaces any existing spectator, without checkingpersistentID. Regular players validate bothclientIDandpersistentIDbefore replacing an existing client.To avoid one account impersonating a reconnecting spectator with the same
clientID, mirror the player logic here:// Handle spectators separately if (client.isSpectator) { this.log.info("spectator joining game", { clientID: client.clientID, persistentID: client.persistentID, clientIP: ipAnonymize(client.ip), }); // Remove stale spectator if this is a reconnect const existing = this.spectators.find( (c) => c.clientID === client.clientID, ); if (existing !== undefined) { + if (client.persistentID !== existing.persistentID) { + this.log.error("spectator persistent ids do not match", { + clientID: client.clientID, + clientIP: ipAnonymize(client.ip), + clientPersistentID: client.persistentID, + existingIP: ipAnonymize(existing.ip), + existingPersistentID: existing.persistentID, + }); + return; + } this.spectators = this.spectators.filter((c) => c !== existing); } this.spectators.push(client); client.lastPing = Date.now(); this.allClients.set(client.clientID, client); // Set up spectator message handlers (receive only, no intents) this.setupSpectatorHandlers(client, lastTurn); return; }Also note that spectators are added to
allClients. This means they will participate incheckDisconnectedStatus()and emitmark_disconnectedintents. If the game core should only ever see disconnect intents for real players, consider keeping spectators out ofallClientsor gatingmarkClientDisconnectedto player client IDs only.Also applies to: 141-165
🧹 Nitpick comments (1)
src/server/GameServer.ts (1)
738-742: Spectators in gameInfo shape look good; consider shared type if this growsAdding a
spectatorsarray with{ username, clientID, isSpectator: true }cleanly separates spectators from players and keeps the payload small.If you later add more spectator fields, consider introducing a shared “client summary” type (for both players and spectators) rather than parallel shapes to keep the TypeScript types simple and rely on discriminated unions instead of more classes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/GameServer.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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-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.ts
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/server/Client.ts (1)
Client(6-25)src/core/Schemas.ts (1)
ClientMessageSchema(541-548)
🔇 Additional comments (5)
src/server/GameServer.ts (5)
370-451: Spectator message handler is strict and keeps spectators read‑onlyThe new
setupSpectatorHandlersonly allowsping, closes on schema errors or handler exceptions, and logs and ignores intents and other message types. This is clean, uses the existing discriminated union, and keeps spectators strictly view‑only without extra libraries.
496-502: Prestart broadcast correctly extended to spectatorsForwarding the validated prestart message to
this.spectatorswith simple logging mirrors the player path and keeps the protocol simple. This looks good.
543-549: Start broadcast for spectators is consistent with player flowSending the same
startpayload to spectators (usingsendStartGameMsg) keeps one code path for game state bootstrap and avoids divergence between players and spectators. Implementation is straightforward and idiomatic.
594-597: Turn updates to spectators keep them in sync with the gameReusing the existing
ServerTurnMessagefor spectators keeps protocol shape simple and means spectators replay the same turn stream as players. This is a good composition‑first design.
669-683: Spectator heartbeat cleanup matches active client logicThe spectator cleanup loop mirrors the active client heartbeat handling and uses the same timeout and close semantics. This prevents stale spectator connections from lingering and matches the rest of the server’s lifecycle behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/server/GameServer.ts (2)
381-462: Spectator message handler is robust and aligned with “view‑only” designThe new
setupSpectatorHandlerslooks solid:
- Reuses
ClientMessageSchemawith Zod and logs detailed validation errors.- Closes the connection and removes the spectator on invalid payloads or handler exceptions.
- Allows only
ping, and logs + ignoresintentor other message types, which matches the requirement “block and log spectator commands”.- Mirrors the player path for
WS_ERR_UNEXPECTED_RSV_1.- Sends
startto late‑joining spectators when_hasStartedis already true.One small TypeScript style nit: you could avoid
(clientMsg as any).typein the default case and just useclientMsg.typefrom the discriminated union, but this is minor and consistent with existing code.
796-812: Spectators inallClientswill triggermark_disconnectedintents
checkDisconnectedStatus()iterates overthis.allClients, and in the spectator join branch you now add spectators intoallClients. That means spectators will also getmark_disconnectedintents emitted when they crossdisconnectedTimeout.If the game core ignores
mark_disconnectedfor non‑players this is harmless, but it does add noise to the intent stream and mixes two different concepts (“player connectivity” and “spectator connectivity”).If you want a cleaner separation, consider skipping spectators here, for example:
- for (const [clientID, client] of this.allClients) { + for (const [clientID, client] of this.allClients) { + if (client.isSpectator) { + continue; + } const isDisconnected = this.isClientDisconnected(clientID); ...This keeps
mark_disconnectedreserved for actual players.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/GameServer.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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-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/server/GameServer.ts
📚 Learning: 2025-07-15T17:20:17.831Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/GameServer.ts
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/server/Client.ts (1)
Client(6-25)src/core/Schemas.ts (1)
ClientMessageSchema(541-548)
🔇 Additional comments (7)
src/server/GameServer.ts (7)
43-45: Spectator list on GameServer is clear and well‑scopedHaving
public spectators: Client[] = [];separate fromactiveClientskeeps read‑only clients clearly separated from players and avoids confusing game logic that relies onactiveClients. No issues here.
133-176: Spectator join / reconnect flow looks consistent and safeThe spectator branch in
addClientis clean:
- It logs joins with anonymized IP and persistent ID.
- It handles reconnects by clientID and rejects on
persistentIDmismatch without replacing the existing spectator.- It refreshes
lastPing, updatesallClients, and then delegates tosetupSpectatorHandlers.This matches the player reconnect checks while keeping spectators out of
activeClients. I do not see correctness or security problems in this block.
507-513: Prestart broadcast to spectators is straightforwardForwarding the same prestart payload to
this.spectatorsas toactiveClientsis simple and matches expectations for lobby observers. The logging is helpful and low‑noise.
554-560: Start broadcast to spectators matches player flowSending the same
startpayload to spectators keeps them in sync with the game start without touching player state. UsingsendStartGameMsghere also keeps the logic shared and easier to maintain.
605-608: Turn updates to spectators are correct and minimalReusing the same serialized
turnmessage for spectators is the right choice and keeps bandwidth and code small. Since spectators never send intents, this remains strictly one‑way and safe.
742-753:gameInfo()spectator exposure matches the new schemaAdding a separate
spectatorsarray with{ username, clientID, isSpectator: true }is clear and easy to consume on the client side. It keeps the oldclientslist unchanged, so existing code that only cares about players should not break.
663-695: Spectator heartbeat cleanup is good, but they now affect game lifetime vialastPingUpdateCleaning up stale spectators in
phase()using the same 60s heartbeat rule as players is good; it stops dead spectator sockets from lingering.However, if spectator
pingupdates bothclient.lastPingand the globallastPingUpdate, spectators now keepnoRecentPingsfalse even when there are zeroactiveClients. That changes phase behavior:
- For private games, a game with no players but one spectator who keeps pinging will stay in
ACTIVEinstead of moving toFINISHEDuntilmaxGameDurationis hit.- For public games, similar logic applies to the
noRecentPingschecks.Verify whether this is intentional (game stays alive while anyone is watching) or if spectators should not extend game lifetime once all players are gone. If the latter, consider not updating
lastPingUpdatefrom spectator pings:- case "ping": { - this.lastPingUpdate = Date.now(); - client.lastPing = Date.now(); - break; - } + case "ping": { + client.lastPing = Date.now(); + break; + }Then keep
lastPingUpdateupdates only in the player path.
|
I think if players join via link, they should also be able to be a spectator. |
|
I understand what you mean by link but could you elaborate on what you mean by having a toggle in the lobby overview? |
I mean this UI. Having a toggle when you are already joined |
ohhh okay I get what you mean now. I'll add those 2 fixes when Evan aproves this pull so I atleast know its valid. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
54-66: Spectator flag is added but not used to block client commands
LobbyConfig.isSpectatoris defined but never read in this file. Right now, a spectator client still:
- Registers all input handlers.
- Can still send a spawn intent in
inputEventbecause that path does not rely onmyPlayer.- Will try to run other action flows until they early-return when
playerByClientIDreturnsnull.Server-side blocking helps, but the PR goal says spectators “cannot send commands/intents”. From the client side, it is cleaner and cheaper to never emit them at all.
I suggest gating all intent-sending handlers on the spectator flag:
@@ private inputEvent(event: MouseUpEvent) { - if (!this.isActive || this.renderer.uiState.ghostStructure !== null) { + if (this.lobby.isSpectator) { + return; + } + if (!this.isActive || this.renderer.uiState.ghostStructure !== null) { return; } @@ private autoUpgradeEvent(event: AutoUpgradeEvent) { - if (!this.isActive) { + if (this.lobby.isSpectator) { + return; + } + if (!this.isActive) { return; } @@ private doBoatAttackUnderCursor(): void { + if (this.lobby.isSpectator) { + return; + } @@ private doGroundAttackUnderCursor(): void { + if (this.lobby.isSpectator) { + return; + } @@ private sendBoatAttackIntent(tile: TileRef) { - if (!this.myPlayer) return; + if (this.lobby.isSpectator) return; + if (!this.myPlayer) return;This keeps spectator behavior simple to reason about: “view-only, no outgoing intents”.
Optionally, for clearer types you could later move away from a single
LobbyConfigbag toward a discriminated union, for example:type LobbyConfig = | ({ isSpectator?: false } & PlayerLobbyConfig) | ({ isSpectator: true } & SpectatorLobbyConfig);so that player-only fields and flows are not even available on the spectator side at the type level.
Also applies to: 459-502, 504-530, 574-616, 646-660
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(3 hunks)src/client/ClientGameRunner.ts(1 hunks)src/server/GameServer.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (6)
📚 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.tssrc/server/GameServer.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/ClientGameRunner.tssrc/server/GameServer.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/ClientGameRunner.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/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: 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/server/GameServer.ts
📚 Learning: 2025-07-15T17:20:17.831Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1441
File: tests/LangCode.test.ts:19-32
Timestamp: 2025-07-15T17:20:17.831Z
Learning: In the OpenFrontIO codebase, for unit tests it's acceptable to let exceptions (like JSON parsing errors) propagate and fail the test naturally rather than adding explicit error handling with try-catch blocks, as the test framework will catch them and fail the test appropriately.
Applied to files:
src/server/GameServer.ts
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/server/Client.ts (1)
Client(6-25)src/core/Schemas.ts (1)
ClientMessageSchema(541-548)
🔇 Additional comments (8)
src/server/GameServer.ts (8)
43-43: LGTM: Clean spectator tracking.The separate
spectatorsarray correctly keeps spectators isolated fromactiveClients, preventing them from counting toward player limits or affecting gameplay.
142-176: LGTM: Spectator join flow is secure.The spectator join path correctly:
- Validates persistentID on reconnect to prevent account impersonation
- Removes stale spectator entries on valid reconnection
- Bypasses player join logic (slots, team assignment, etc.)
- Delegates to spectator-specific handlers
398-479: LGTM: Spectator handlers enforce receive-only behavior.The implementation correctly:
- Validates all messages and closes connection on schema violations
- Allows only ping messages (lines 424-427)
- Blocks intent commands with clear logging (lines 429-434)
- Cleans up spectator on disconnect and error
- Sends catch-up start message for late joins
524-530: LGTM: Spectators receive prestart correctly.
571-577: LGTM: Spectators receive start message.
622-625: LGTM: Spectators receive turn updates.
697-711: LGTM: Spectator cleanup mirrors player cleanup.The 60-second timeout and WebSocket closure match the player cleanup logic, ensuring stale spectators don't accumulate.
766-770: LGTM: gameInfo includes spectator metadata.The
isSpectator: trueflag allows UI to distinguish spectators from players.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
This seems a bit overengineered, i think the game server can be basically agnostic to spectators, just need to pass the "isSpectator" (similar to how cosmetics are passed). I think we should add another intent like "JoinSpectatorIntent", which spectators do instead of spawning
Got it. I have fixed it and made it simplified. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/ClientGameRunner.ts(2 hunks)src/client/Transport.ts(2 hunks)src/core/Schemas.ts(6 hunks)src/server/GameServer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/ClientGameRunner.ts
- src/core/Schemas.ts
- src/server/GameServer.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/Transport.ts (1)
src/core/Schemas.ts (1)
Intent(28-51)
🔇 Additional comments (1)
src/client/Transport.ts (1)
388-388: LGTM! Spectator flag correctly propagated.The
isSpectatorfield properly extends the join payload to carry spectator status from the lobby configuration to the server.
3 participants
Resolves #2488
Description:
This PR has a Spectator Mode feature that allows users to join matches as non-playing observers without taking up player slots or affecting gameplay.
Key Features:
Join as Spectator: New button in the Join Private Lobby modal allows users to join games as spectators
Separate Tracking: Spectators are tracked in a separate
spectators[]array on the server, distinct from active playersView-Only Access: Spectators receive all game state broadcasts (prestart, start, turns) but cannot send any commands or intents
Real-Time Display: Spectator count is displayed in:
No Player Slot Usage: Spectators don't count toward player limits or team assignments
Server-Side Validation: All spectator intent commands are blocked and logged as warnings
Technical Implementation:
GameInfo,ClientInfo, andClientJoinMessageschemas with spectator fieldsisSpectatorflag toClientclass and throughout the join flowsetupSpectatorHandlers()in GameServer for receive-only message handlingPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
.fruitymctooty