Add spectator role backend plumbing #2800
Conversation
WalkthroughAdds spectator mode feature enabling clients to join games as observers rather than active players. Introduces JoinSpectatorIntent, spectator state tracking in Game, and lobby roster broadcasting to keep all clients synchronized on player and spectator status. Changes
Sequence DiagramssequenceDiagram
actor Client
participant Worker as Server Worker
participant GM as GameServer
participant Game
participant Exec as JoinSpectatorExecution
Client->>Worker: ClientJoinMessage with isSpectator=true
Worker->>Worker: Parse isSpectator flag
activate Worker
Worker->>GM: addClient(client, isSpectator: true)
deactivate Worker
activate GM
GM->>Game: createExecution(JoinSpectatorIntent)
activate Game
Game->>Exec: new JoinSpectatorExecution(clientID)
Game-->>GM: exec
deactivate Game
GM->>Exec: exec.init(game, ticks)
activate Exec
Exec->>Game: addSpectator(clientID)
Game->>Game: _spectators.add(clientID)
deactivate Exec
GM->>GM: sendLobbyRoster()
activate GM
GM->>Game: getPlayers(), getSpectators()
GM->>Client: ServerLobbyRosterMessage
deactivate GM
deactivate GM
sequenceDiagram
participant Game
participant Client1 as Client (Player)
participant Client2 as Client (Spectator)
participant Server as GameServer
Game->>Server: onClientDisconnect(clientID)
activate Server
Server->>Game: removeSpectator(clientID)
Server->>Server: sendLobbyRoster()
Server->>Client1: ServerLobbyRosterMessage
Server->>Client2: ServerLobbyRosterMessage
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 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 (3)
src/core/Schemas.ts (1)
521-524: Use stricter validation for consistency.LobbyClientInfoSchema uses
z.string()for bothclientIDandusername, but elsewhere in the codebase (PlayerSchema, ClientJoinMessageSchema) these fields use stricter validators:IDandUsernameSchema. Consider using the same validators here for consistency and to ensure lobby roster data meets the same requirements as player data.🔎 Suggested refactor
export const LobbyClientInfoSchema = z.object({ - clientID: z.string(), - username: z.string(), + clientID: ID, + username: UsernameSchema, });src/server/GameServer.ts (1)
233-236: Clarify intent: sendLobbyRoster() after late-join scenario.The
sendLobbyRoster()call appears after the late-join handling block (lines 230-232), butsendLobbyRoster()itself returns early if the game has started (line 240-242). This means roster updates are only sent for pre-start joins, which is correct.However, this placement could confuse future maintainers who might expect roster broadcasts for late joins. Consider adding a comment to clarify that roster updates only apply before game start:
🔎 Suggested clarification
// In case a client joined the game late and missed the start message. if (this._hasStarted) { this.sendStartGameMsg(client.ws, 0); } - // Send updated lobby roster to all clients + // Send updated lobby roster to all clients (only if game hasn't started) this.sendLobbyRoster();src/core/game/GameView.ts (1)
723-737: Consider readonly return types for lobby accessor arrays.The
lobbySpectators()andlobbyPlayers()methods return mutable arrays directly. Callers could accidentally modify the internal state. Consider returning readonly arrays to prevent unintended mutations:🔎 Suggested improvement
- lobbySpectators(): LobbyClientInfo[] { + lobbySpectators(): readonly LobbyClientInfo[] { return this._lobbySpectators; } - lobbyPlayers(): LobbyClientInfo[] { + lobbyPlayers(): readonly LobbyClientInfo[] { return this._lobbyPlayers; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/core/Schemas.tssrc/core/execution/ExecutionManager.tssrc/core/execution/JoinSpectatorExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/GameView.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.ts
🧰 Additional context used
🧠 Learnings (13)
📚 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/core/game/GameView.tssrc/core/game/GameUpdates.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/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/server/GameServer.tssrc/core/game/Game.tssrc/core/game/GameUpdates.tssrc/core/game/GameImpl.tssrc/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: 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.tssrc/core/game/GameImpl.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
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.tssrc/core/game/GameView.tssrc/core/game/GameUpdates.tssrc/core/game/GameImpl.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/core/game/GameImpl.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/core/execution/ExecutionManager.tssrc/core/execution/JoinSpectatorExecution.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/ExecutionManager.tssrc/core/execution/JoinSpectatorExecution.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-11-06T00:56:21.251Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 2396
File: src/core/execution/TransportShipExecution.ts:180-189
Timestamp: 2025-11-06T00:56:21.251Z
Learning: In OpenFrontIO, when a player conquers a disconnected player, transport ships and warships are only transferred to the conqueror if they are on the same team. If an enemy (non-teammate) conquers a disconnected player, the ships are deleted (described as "boated into a sea mine"), not captured.
Applied to files:
src/core/game/GameImpl.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
🧬 Code graph analysis (7)
src/server/GameServer.ts (1)
src/core/Schemas.ts (1)
ClientID(26-26)
src/core/game/Game.ts (4)
src/core/game/GameView.ts (1)
clientID(460-462)src/core/game/PlayerImpl.ts (1)
clientID(197-199)src/core/game/TerraNulliusImpl.ts (1)
clientID(9-11)src/core/Schemas.ts (1)
ClientID(26-26)
src/core/execution/ExecutionManager.ts (2)
src/core/execution/JoinSpectatorExecution.ts (1)
JoinSpectatorExecution(8-34)src/core/execution/NoOpExecution.ts (1)
NoOpExecution(3-12)
src/core/game/GameView.ts (1)
src/core/game/GameUpdates.ts (2)
LobbyClientInfo(285-288)LobbyRosterUpdate(292-298)
src/core/execution/JoinSpectatorExecution.ts (2)
src/core/game/Game.ts (2)
Execution(367-372)Game(685-791)src/core/Schemas.ts (1)
ClientID(26-26)
src/server/Client.ts (1)
src/core/game/GameImpl.ts (1)
isSpectator(354-356)
src/core/game/GameImpl.ts (4)
src/core/Schemas.ts (1)
ClientID(26-26)src/core/game/GameView.ts (1)
clientID(460-462)src/core/game/PlayerImpl.ts (1)
clientID(197-199)src/core/game/TerraNulliusImpl.ts (1)
clientID(9-11)
🔇 Additional comments (31)
src/core/Schemas.ts (7)
52-53: LGTM! Spectator intent properly added to union.The JoinSpectatorIntent is correctly included in the Intent union type, maintaining consistency with existing patterns.
89-89: LGTM! Type export follows convention.The JoinSpectatorIntent type is correctly inferred from the schema using the standard pattern.
378-380: LGTM! Schema definition is clean and minimal.The JoinSpectatorIntentSchema correctly extends BaseIntentSchema and includes only the necessary type discriminator. No additional fields are needed since the clientID comes from BaseIntentSchema.
407-407: LGTM! Schema registered in discriminated union.The JoinSpectatorIntentSchema is properly added to the IntentSchema discriminated union.
526-533: LGTM! Lobby roster message schema is well-structured.The ServerLobbyRosterMessageSchema correctly defines the message structure with separate arrays for players and spectators, and the type is properly exported.
542-542: LGTM! Server message schema updated.ServerLobbyRosterMessageSchema is correctly added to the ServerMessageSchema discriminated union.
585-586: LGTM! Spectator flag properly added.The optional
isSpectatorboolean flag is correctly added to ClientJoinMessageSchema with clear documentation explaining its purpose.src/server/Client.ts (1)
24-24: LGTM! Spectator flag added to Client.The
isSpectatorparameter is correctly added as a public readonly boolean with a sensible default offalse, following the existing pattern for constructor parameters.src/core/execution/ExecutionManager.ts (4)
19-19: LGTM! Import added correctly.JoinSpectatorExecution is properly imported and placed in alphabetical order.
51-54: LGTM! Join spectator intent handled early.Correctly handles the
join_spectatorintent before player lookup, since spectators don't have Player objects. The early return prevents false "player not found" warnings.
56-60: LGTM! Critical security boundary for spectators.This guard correctly prevents spectators from sending game intents by returning NoOpExecution. The warning log aids debugging. This is a secure-by-default approach, blocking all game actions from spectators.
146-147: LGTM! Placeholder for future implementation.The
update_game_configcase returns NoOpExecution, indicating the intent schema is defined but not yet implemented. This is appropriate for backend plumbing work.src/server/Worker.ts (1)
404-405: LGTM! Client instantiation updated correctly.The two new constructor parameters are properly passed:
isRejoinis correctly hardcoded asfalsefor join messagesisSpectatorcorrectly uses nullish coalescing to default tofalsewhen not providedsrc/core/execution/JoinSpectatorExecution.ts (5)
1-11: LGTM! Class structure is clean.Imports are correct, and the class properly implements the Execution interface with appropriate private state. The nullable
mgfield is set duringinit().
13-15: LGTM! Correct activity status.Returning
falseis appropriate since adding a spectator is an instantaneous operation that doesn't require ongoing tracking.
17-19: LGTM! Correctly active during spawn phase.Returning
trueallows clients to join as spectators before the game starts, which is the intended behavior.
33-33: LGTM! No-op tick is appropriate.Empty
tick()implementation is correct since this execution performs a one-time operation duringinit()and has no ongoing behavior.
21-31: Check if active players can become spectators without being removed from player state first.The
addSpectator()method has no validation—it simply adds a client to the spectators set without checking if they're still an active player. This allows creating an inconsistent state where a client is both player and spectator.While dead players are automatically converted through game logic (they're removed from players before
addSpectator()is called), there's no handling here for active players trying to join as spectators. Either add a check likemg.playerByClientID(this.clientID)and handle that case (throw an error or remove them from players first), or add validation inaddSpectator()itself.⛔ 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/GameServer.ts (4)
156-158: LGTM: Spectator bypass logic is correct.Spectators correctly bypass the
maxPlayerslimit, allowing observers to join full lobbies without affecting active player slots.
515-516: LGTM: Disconnect handling is correct.The roster is correctly updated when clients disconnect, ensuring all remaining clients receive the updated player and spectator lists.
831-832: LGTM: Kick handling is correct.The roster is correctly updated when clients are kicked, ensuring all remaining clients receive the updated player and spectator lists.
238-265: Schema alignment is correct—no changes needed.The
sendLobbyRoster()method properly constructs a message matchingServerLobbyRosterMessageSchema. Theplayersandspectatorsarrays contain objects withclientIDandusernamefields as expected byLobbyClientInfoSchema, and thetypefield is the correct literal"lobby_roster". No runtime validation errors will occur.src/core/game/Game.ts (1)
785-790: LGTM: Clean spectator management API.The API is well-designed:
- Method names are clear and follow existing conventions.
getSpectators()returnsReadonlySet<ClientID>to prevent external mutation.- The interface is minimal and focused on essential spectator operations.
src/core/game/GameView.ts (2)
603-606: LGTM: Lobby state fields are well-defined.Private fields follow the class naming convention and are initialized with appropriate default values.
700-716: LGTM: Update handling supports partial updates.The implementation correctly handles partial updates by checking each field for
undefinedbefore updating. This allows the server to send incremental roster changes efficiently.src/core/game/GameImpl.ts (3)
90-90: LGTM: Spectators field is appropriately defined.Using a
Set<ClientID>ensures unique spectator tracking and provides efficient add/remove/has operations.
353-372: LGTM: Spectator management implementation is clean.The implementation is straightforward and leverages
Setoperations effectively:
getSpectators()correctly returnsReadonlySetto prevent external modifications.- Duplicate additions are automatically handled by
Setsemantics.- All methods match the interface signatures.
1019-1023: The dual state of dead players who are spectators is already safely handled in the codebase.Your concern is well-founded, but verification shows the game logic already handles this correctly:
Intent blocking at boundary: ExecutionManager.ts (lines 56-60) checks spectator status BEFORE allowing any game intents, preventing spectators from taking actions regardless of their
isAlive()state.isAlive() filtering throughout: Dead players are consistently filtered via
isAlive()checks across the codebase (PlayerExecution, AllianceExtensionExecution, PlayerImpl, and others), preventing dead player actions independently.Spectator checks happen first: The execution pipeline validates spectator status before processing player-specific logic, so the dual state poses no risk.
No changes needed—the architecture handles this case safely.
src/core/game/GameUpdates.ts (3)
51-51: LGTM: Enum value addition is correct.The
LobbyRosterenum value is appropriately named and added at the end to avoid breaking existing code that might rely on enum ordering.
74-75: LGTM: Union type correctly extended.The
GameUpdateunion type is properly extended to include the newLobbyRosterUpdatetype.
284-298: LGTM: Interfaces are well-designed.The interfaces follow good TypeScript practices:
LobbyClientInfois a simple data container with clear fields.LobbyRosterUpdateuses the discriminated union pattern withtype: GameUpdateType.LobbyRoster.- Optional fields enable efficient partial updates.
- Comments clearly document the purpose.
1 participant
Description:
Adds backend infrastructure for spectator mode without UI changes.
How it works
isSpectator: truein join messagePlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
abodcraft1