have lobby schedule ffa, teams, & special game types #3196
Conversation
WalkthroughThe pull request introduces a Changes
Sequence DiagramsequenceDiagram
participant Master as Master<br/>LobbyService
participant Playlist as MapPlaylist
participant Manager as GameManager
participant Worker as Worker
participant Server as GameServer
Master->>Master: getAllLobbies()<br/>grouped by PublicGameType
Master->>Master: maybeScheduleLobby()<br/>iterate each type
loop For Each PublicGameType (ffa, team, special)
Master->>Playlist: gameConfig(type)
alt Special Type
Playlist->>Playlist: getSpecialConfig()
Playlist-->>Master: tailored GameConfig
else FFA or Team Type
Playlist->>Playlist: getNextMap(type)<br/>fetch from type playlist
Playlist-->>Master: type-specific GameConfig
end
Master->>Manager: createGame(id, config,<br/>publicGameType)
Manager->>Server: new GameServer(...,<br/>publicGameType)
Server-->>Manager: server created
Manager-->>Master: game ready
Master->>Worker: MasterCreateGame IPC<br/>includes publicGameType
Worker->>Manager: createGame(publicGameType)
Manager->>Server: configure with type
Server->>Server: expose publicGameType<br/>in gameInfo payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/WorkerLobbyService.ts (1)
67-80:⚠️ Potential issue | 🟠 MajorNon-null assertion
gi.publicGameType!is unsafe — matchmaking games lackpublicGameType.In
src/server/Worker.ts(line 568-569), the matchmaking polling path callsgm.createGame(gameId, gameConfig)without apublicGameType. These games are public (GameType.Publicfromget1v1Config), so they appear inpublicLobbies(). WhensendMyLobbiesToMasterruns,gi.publicGameTypeisundefined, and the!assertion just suppresses the compiler — at runtime,undefinedis serialized and the master will fail to parse this message (sincePublicGameInfoSchemarequirespublicGameType).This would silently break the lobby list broadcast for any worker running matchmaking games.
Proposed fix: filter out lobbies without publicGameType, or assign one for matchmaking
private sendMyLobbiesToMaster() { const lobbies = this.gm .publicLobbies() .map((g) => g.gameInfo()) + .filter((gi) => gi.publicGameType !== undefined) .map((gi) => { return { gameID: gi.gameID, numClients: gi.clients?.length ?? 0, startsAt: gi.startsAt!, gameConfig: gi.gameConfig, - publicGameType: gi.publicGameType!, + publicGameType: gi.publicGameType!, } satisfies PublicGameInfo; });
🤖 Fix all issues with AI agents
In `@src/client/PublicLobby.ts`:
- Line 52: The access this.publicGames.games["ffa"] can be undefined because
PublicGamesSchema is a z.record; update the PublicLobby code to guard against a
missing "ffa" key before calling forEach (e.g., retrieve a local variable like
ffaGames from this.publicGames.games["ffa"] and if it's undefined use an empty
array or skip iteration), and then call forEach on that safe value so you don't
attempt to read forEach of undefined.
- Line 80: The code accesses this.publicGames.games["ffa"][0] without guarding
for missing or empty arrays; update the logic where const lobby =
this.publicGames.games["ffa"][0] is set (in PublicLobby) to first verify that
this.publicGames, this.publicGames.games, and this.publicGames.games["ffa"]
exist and have length > 0, and handle the empty/missing case (e.g., return
early, set lobby to null/undefined, or create a fallback) so you never index
into an undefined array.
In `@src/core/Schemas.ts`:
- Around line 169-172: The PublicGamesSchema currently uses
z.record(PublicGameTypeSchema, z.array(PublicGameInfoSchema)) which allows
missing game-type keys; replace that record with an explicit z.object listing
the three keys (e.g., ffa, team, special) each typed as
z.array(PublicGameInfoSchema) so the schema guarantees all game-type keys are
present (matching MasterLobbyService.getAllLobbies() behavior and client code in
PublicLobby.ts that accesses games["ffa"] without null checks); update
PublicGamesSchema to use z.object({ serverTime: z.number(), games: z.object({
ffa: z.array(PublicGameInfoSchema), team: z.array(PublicGameInfoSchema),
special: z.array(PublicGameInfoSchema) }) }) to enforce the invariant.
In `@src/server/MapPlaylist.ts`:
- Around line 87-95: Remove the three unused private fields ffaPlaylist,
teamPlaylist, and specialPlaylist which are dead code; the logic uses the
playlists object and getNextMap, so delete those unused declarations and ensure
no remaining references exist elsewhere (search for ffaPlaylist, teamPlaylist,
specialPlaylist) and rely solely on the existing playlists:
Record<PublicGameType, GameMapType[]> field and getNextMap for map selection.
- Around line 241-267: generateNewPlaylist currently returns an empty playlist
if all 10,000 attempts fail, causing getNextMap to call playlist.shift() and
produce undefined; modify generateNewPlaylist (which uses buildMapsList,
PseudoRandom, shuffledSource and addNextMapNonConsecutive) to detect failure
after the attempts loop and as a fallback return a simple shuffled copy of the
maps (e.g., use the same PseudoRandom.shuffleArray on [...maps] or
shuffledSource) so the function never returns an empty array.
🧹 Nitpick comments (2)
src/server/MapPlaylist.ts (1)
233-239: All three types share the same generated playlist — is that intended?
getNextMapgenerates the same playlist (same frequency map, samebuildMapsList) regardless of type. Team and FFA games may have different map suitability. If this is a known simplification for now, a short comment would help future readers.src/server/MasterLobbyService.ts (1)
112-153: Per-type scheduling loop is straightforward — one minor note ongameConfigfetch before worker check.The logic is clean: iterate types, skip if 2+ lobbies exist, compute timing, fetch config, send to worker. Easy to read.
Lines 129–133:
gameConfigis fetched viaawait this.playlist.gameConfig(type)before checking if the target worker exists. If the worker is missing, the config fetch was wasted work. Consider swapping the order — check for the worker first, then fetch config.Swap worker lookup before async config fetch
const gameID = generateID(); const workerId = this.config.workerIndex(gameID); - const gameConfig = await this.playlist.gameConfig(type); const worker = this.workers.get(workerId); if (!worker) { this.log.error(`Worker ${workerId} not found`); continue; } + const gameConfig = await this.playlist.gameConfig(type); + worker.send(
| this.serverTimeOffset = this.publicGames.serverTime - Date.now(); | ||
| } | ||
| this.publicGames.games.forEach((l) => { | ||
| this.publicGames.games["ffa"].forEach((l) => { |
There was a problem hiding this comment.
Missing guard: games["ffa"] could be undefined if the key is absent.
Since PublicGamesSchema uses z.record, the "ffa" key is not guaranteed to exist. This line will throw TypeError: Cannot read properties of undefined (reading 'forEach').
Add a fallback:
Proposed fix
- this.publicGames.games["ffa"].forEach((l) => {
+ (this.publicGames.games["ffa"] ?? []).forEach((l) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.publicGames.games["ffa"].forEach((l) => { | |
| (this.publicGames.games["ffa"] ?? []).forEach((l) => { |
🤖 Prompt for AI Agents
In `@src/client/PublicLobby.ts` at line 52, The access
this.publicGames.games["ffa"] can be undefined because PublicGamesSchema is a
z.record; update the PublicLobby code to guard against a missing "ffa" key
before calling forEach (e.g., retrieve a local variable like ffaGames from
this.publicGames.games["ffa"] and if it's undefined use an empty array or skip
iteration), and then call forEach on that safe value so you don't attempt to
read forEach of undefined.
| if (!this.publicGames) return html``; | ||
|
|
||
| const lobby = this.publicGames.games[0]; | ||
| const lobby = this.publicGames.games["ffa"][0]; |
There was a problem hiding this comment.
Same issue: access games["ffa"][0] without checking if the array exists.
Proposed fix
- const lobby = this.publicGames.games["ffa"][0];
+ const ffaGames = this.publicGames.games["ffa"] ?? [];
+ const lobby = ffaGames[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lobby = this.publicGames.games["ffa"][0]; | |
| const ffaGames = this.publicGames.games["ffa"] ?? []; | |
| const lobby = ffaGames[0]; |
🤖 Prompt for AI Agents
In `@src/client/PublicLobby.ts` at line 80, The code accesses
this.publicGames.games["ffa"][0] without guarding for missing or empty arrays;
update the logic where const lobby = this.publicGames.games["ffa"][0] is set (in
PublicLobby) to first verify that this.publicGames, this.publicGames.games, and
this.publicGames.games["ffa"] exist and have length > 0, and handle the
empty/missing case (e.g., return early, set lobby to null/undefined, or create a
fallback) so you never index into an undefined array.
| export const PublicGamesSchema = z.object({ | ||
| serverTime: z.number(), | ||
| games: PublicGameInfoSchema.array(), | ||
| games: z.record(PublicGameTypeSchema, z.array(PublicGameInfoSchema)), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Find the relevant files
git ls-files | grep -E "(Schemas|PublicLobby|PublicGame)" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 214
🏁 Script executed:
# Check the Schemas.ts file to see PublicGameTypeSchema definition and context
fd "Schemas.ts" --type f -x cat {} \; | head -250Repository: openfrontio/OpenFrontIO
Length of output: 8417
🏁 Script executed:
# Look for PublicLobby.ts and the specific usage at line 52
fd "PublicLobby.ts" --type f -x cat -n {} \;Repository: openfrontio/OpenFrontIO
Length of output: 10309
🏁 Script executed:
# Search for where PublicGamesSchema is parsed or used
rg "PublicGamesSchema" --type ts --context 5Repository: openfrontio/OpenFrontIO
Length of output: 3751
🏁 Script executed:
# Search for server-side code that constructs the games object
rg "games.*ffa\|games.*team\|games.*special" --type tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there's server code that ensures all three keys are present
fd "PublicLobby" --type f -o -name "*Lobby*" | grep -v ".js" | grep -v node_modulesRepository: openfrontio/OpenFrontIO
Length of output: 297
🏁 Script executed:
# Search for where games object is constructed - look for server-side code
rg "PublicGames|games.*ffa|games.*team" --type ts -A 3 | head -100Repository: openfrontio/OpenFrontIO
Length of output: 5630
🏁 Script executed:
# Search for server files that might handle public lobbies/games
fd ".*[Ll]obby.*" --type f --extension ts | grep -i serverRepository: openfrontio/OpenFrontIO
Length of output: 132
🏁 Script executed:
# Look for where the games object is built - search for object literals with ffa/team/special
rg "ffa.*team.*special|team.*special|{.*ffa" --type ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 214
🏁 Script executed:
# Examine the MasterLobbyService.ts file to see how games object is built
wc -l src/server/MasterLobbyService.tsRepository: openfrontio/OpenFrontIO
Length of output: 103
🏁 Script executed:
# Examine the WorkerLobbyService.ts file
wc -l src/server/WorkerLobbyService.tsRepository: openfrontio/OpenFrontIO
Length of output: 103
🏁 Script executed:
# Read MasterLobbyService.ts
cat -n src/server/MasterLobbyService.tsRepository: openfrontio/OpenFrontIO
Length of output: 5529
🏁 Script executed:
# Read WorkerLobbyService.ts
cat -n src/server/WorkerLobbyService.tsRepository: openfrontio/OpenFrontIO
Length of output: 5126
Use z.object with explicit keys to express that all game types are always present.
The schema currently uses z.record(PublicGameTypeSchema, ...), which permits objects like { games: { ffa: [...] } } without team or special keys. However, MasterLobbyService.getAllLobbies() always initializes all three keys and the server sends them every time (lines 78–82, 100 in MasterLobbyService.ts). The client code in PublicLobby.ts (lines 52, 80) accesses games["ffa"] without null checks, assuming the key always exists.
To correctly express this semantic invariant and prevent client crashes, use an explicit z.object:
Proposed fix
export const PublicGamesSchema = z.object({
serverTime: z.number(),
- games: z.record(PublicGameTypeSchema, z.array(PublicGameInfoSchema)),
+ games: z.object({
+ ffa: z.array(PublicGameInfoSchema),
+ team: z.array(PublicGameInfoSchema),
+ special: z.array(PublicGameInfoSchema),
+ }),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PublicGamesSchema = z.object({ | |
| serverTime: z.number(), | |
| games: PublicGameInfoSchema.array(), | |
| games: z.record(PublicGameTypeSchema, z.array(PublicGameInfoSchema)), | |
| }); | |
| export const PublicGamesSchema = z.object({ | |
| serverTime: z.number(), | |
| games: z.object({ | |
| ffa: z.array(PublicGameInfoSchema), | |
| team: z.array(PublicGameInfoSchema), | |
| special: z.array(PublicGameInfoSchema), | |
| }), | |
| }); |
🤖 Prompt for AI Agents
In `@src/core/Schemas.ts` around lines 169 - 172, The PublicGamesSchema currently
uses z.record(PublicGameTypeSchema, z.array(PublicGameInfoSchema)) which allows
missing game-type keys; replace that record with an explicit z.object listing
the three keys (e.g., ffa, team, special) each typed as
z.array(PublicGameInfoSchema) so the schema guarantees all game-type keys are
present (matching MasterLobbyService.getAllLobbies() behavior and client code in
PublicLobby.ts that accesses games["ffa"] without null checks); update
PublicGamesSchema to use z.object({ serverTime: z.number(), games: z.object({
ffa: z.array(PublicGameInfoSchema), team: z.array(PublicGameInfoSchema),
special: z.array(PublicGameInfoSchema) }) }) to enforce the invariant.
| private ffaPlaylist: GameMapType[] = []; | ||
| private teamPlaylist: GameMapType[] = []; | ||
| private specialPlaylist: GameMapType[] = []; | ||
|
|
||
| private playlists: Record<PublicGameType, GameMapType[]> = { | ||
| ffa: [], | ||
| special: [], | ||
| team: [], | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ffaPlaylist, teamPlaylist, specialPlaylist are dead code.
Lines 87-89 declare three private arrays that are never read or written. Only this.playlists (line 91-95) is used by getNextMap. Remove the unused fields.
Proposed fix
export class MapPlaylist {
- private ffaPlaylist: GameMapType[] = [];
- private teamPlaylist: GameMapType[] = [];
- private specialPlaylist: GameMapType[] = [];
-
private playlists: Record<PublicGameType, GameMapType[]> = {
ffa: [],
special: [],
team: [],
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private ffaPlaylist: GameMapType[] = []; | |
| private teamPlaylist: GameMapType[] = []; | |
| private specialPlaylist: GameMapType[] = []; | |
| private playlists: Record<PublicGameType, GameMapType[]> = { | |
| ffa: [], | |
| special: [], | |
| team: [], | |
| }; | |
| private playlists: Record<PublicGameType, GameMapType[]> = { | |
| ffa: [], | |
| special: [], | |
| team: [], | |
| }; |
🤖 Prompt for AI Agents
In `@src/server/MapPlaylist.ts` around lines 87 - 95, Remove the three unused
private fields ffaPlaylist, teamPlaylist, and specialPlaylist which are dead
code; the logic uses the playlists object and getNextMap, so delete those unused
declarations and ensure no remaining references exist elsewhere (search for
ffaPlaylist, teamPlaylist, specialPlaylist) and rely solely on the existing
playlists: Record<PublicGameType, GameMapType[]> field and getNextMap for map
selection.
| private generateNewPlaylist(): GameMapType[] { | ||
| const maps = this.buildMapsList(); | ||
| const rand = new PseudoRandom(Date.now()); | ||
| const shuffledSource = rand.shuffleArray([...maps]); | ||
| const playlist: GameMapType[] = []; | ||
|
|
||
| const numAttempts = 10000; | ||
| for (let attempt = 0; attempt < numAttempts; attempt++) { | ||
| playlist.length = 0; | ||
| const source = [...shuffledSource]; | ||
|
|
||
| let success = true; | ||
| while (source.length > 0) { | ||
| if (!this.addNextMapNonConsecutive(playlist, source)) { | ||
| success = false; | ||
| break; | ||
| } | ||
| } | ||
| log.error("Failed to generate a valid map playlist"); | ||
|
|
||
| if (success) { | ||
| log.info(`Generated map playlist in ${attempt} attempts`); | ||
| break; | ||
| } | ||
| } | ||
| // Even if it failed, playlist will be partially populated. | ||
| return this.mapsPlaylist.shift()!; | ||
|
|
||
| return playlist; | ||
| } |
There was a problem hiding this comment.
generateNewPlaylist silently returns an empty array if all 10,000 attempts fail.
Each attempt resets playlist.length = 0. If no attempt succeeds, the method returns []. Then getNextMap calls playlist.shift()! on that empty array and gets undefined, which is passed as gameMap into the config — a runtime bug.
Add a fallback: if no valid non-consecutive sequence is found, return a simple shuffle.
Proposed fix
if (success) {
log.info(`Generated map playlist in ${attempt} attempts`);
break;
}
}
+ if (playlist.length === 0) {
+ log.warn("Failed to generate non-consecutive playlist, falling back to shuffle");
+ return rand.shuffleArray([...maps]);
+ }
+
return playlist;
}🤖 Prompt for AI Agents
In `@src/server/MapPlaylist.ts` around lines 241 - 267, generateNewPlaylist
currently returns an empty playlist if all 10,000 attempts fail, causing
getNextMap to call playlist.shift() and produce undefined; modify
generateNewPlaylist (which uses buildMapsList, PseudoRandom, shuffledSource and
addNextMapNonConsecutive) to detect failure after the attempts loop and as a
fallback return a simple shuffled copy of the maps (e.g., use the same
PseudoRandom.shuffleArray on [...maps] or shuffledSource) so the function never
returns an empty array.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
ryanbarlow97
added
Backend
2 participants
Description:
in preparation for #3191
Have the Master scheduler schedule ffa, teams, & special games.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan