feat: show next map and mode info in public lobby #2629
Conversation
|
|
WalkthroughAdds optional next-game metadata through playlist, server, and worker layers; exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (PublicLobby)
participant Master as Server (Master)
participant Playlist as MapPlaylist
participant Worker as Worker / GameManager
participant GameSrv as GameServer
Client->>Master: request lobbies / subscribe
Master->>Playlist: request current & next config (ensurePlaylistPopulated)
Playlist-->>Master: { gameConfig, nextGameConfig? }
Master-->>Client: Lobby state (includes gameConfig, nextGameConfig)
Client->>Client: Render current game details
alt nextGameConfig present
Client->>Client: Render "Up next" block (mode/team/map)
end
Note right of Worker: Game creation path
Worker->>Playlist: obtain { gameConfig, nextGameConfig }
Worker->>Worker: validate/authorize gameConfig (+ nextGameConfig)
Worker->>GameSrv: createGame(id, gameConfig, nextGameConfig, creatorClientID)
GameSrv-->>Master: serialized GameInfo includes nextGameConfig
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 0
🧹 Nitpick comments (1)
src/server/MapPlaylist.ts (1)
83-96: Confusing variable naming: "nextItem" vs "upNextItem".The naming here is backwards from the user's perspective:
nextItem(line 84) = the current game being configured (shifted from playlist)upNextItem(line 88) = the following game to display in UIConsider renaming for clarity:
public gameConfig(): GameConfig { - const nextItem = this.getNextMap(); - const { map, mode } = nextItem; + const currentItem = this.getNextMap(); + const { map, mode } = currentItem; this.ensurePlaylistPopulated(); - const upNextItem = this.mapsPlaylist[0]; + const followingItem = this.mapsPlaylist[0]; let nextPlayerTeams: TeamCountConfig | undefined; - if (upNextItem) { - if (upNextItem.mode === GameMode.Team && !upNextItem.playerTeams) { - upNextItem.playerTeams = this.getTeamCount(); + if (followingItem) { + if (followingItem.mode === GameMode.Team && !followingItem.playerTeams) { + followingItem.playerTeams = this.getTeamCount(); } - nextPlayerTeams = upNextItem.playerTeams; + nextPlayerTeams = followingItem.playerTeams; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/PublicLobby.ts(2 hunks)src/core/Schemas.ts(1 hunks)src/server/MapPlaylist.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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.tssrc/server/MapPlaylist.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/PublicLobby.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/core/Schemas.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/Schemas.tssrc/server/MapPlaylist.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-08-16T16:16:00.414Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1758
File: src/core/ApiSchemas.ts:63-72
Timestamp: 2025-08-16T16:16:00.414Z
Learning: In Zod v4, z.enum() can accept TypeScript native enums directly (e.g., z.enum(MyEnum)). The z.nativeEnum() function was deprecated in v4 and replaced by the overloaded z.enum(). This means z.enum(GameType), z.enum(GameMode), etc. are valid in Zod v4.
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-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
🧬 Code graph analysis (2)
src/client/PublicLobby.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/server/MapPlaylist.ts (4)
src/core/Schemas.ts (2)
TeamCountConfig(157-157)GameConfig(84-84)src/core/configuration/DefaultConfig.ts (1)
playerTeams(331-333)src/core/game/GameImpl.ts (1)
config(336-338)src/core/game/GameView.ts (1)
config(665-667)
🔇 Additional comments (9)
src/core/Schemas.ts (1)
177-180: Clean schema extension for next-game metadata.The new optional fields follow the same patterns as existing fields and reuse
TeamCountConfigSchemaproperly. Good use of composition with the existing schema types.resources/lang/en.json (1)
252-253: Translation key added correctly.The new
following_gamekey follows existing naming patterns underpublic_lobbynamespace. The comma fix ensures valid JSON.src/client/PublicLobby.ts (3)
147-176: Good reuse of existing helper methods for next-game details.The code correctly reuses
getTeamSize,getTeamTotal,getModeLabel, andgetTeamDetailLabelmethods for computing next-game labels. This keeps behavior consistent between current and following game displays.Note:
nextMaxPlayersis computed at line 159 but not displayed in the UI. This is fine if planned for future use, otherwise it can be removed.
231-255: Clean conditional UI block for following game info.The "Following Game" section is well-structured:
- Correct guard on
nextMapbefore rendering- Consistent badge styling with current game display
- Clear visual hierarchy with smaller text for secondary info
182-182: Height increase to h-60 accommodates new section.The button container grows from h-40 to h-60 to fit the new "Following Game" block. This is a reasonable increase.
src/server/MapPlaylist.ts (4)
98-101: Logic is correct for pre-assigning team counts.The current game's
playerTeamsusesnextItem.playerTeams(set during previous call's look-ahead) or falls back to a new random count. This ensures the team count shown in "Following Game" matches what actually gets used.
139-156: Clean refactor: extracting ensurePlaylistPopulated.The playlist population logic is now reusable. The method is called:
- In
getNextMap()before shifting- In
gameConfig()after shifting to ensure a following item existsThis separation is clear and correct.
122-132: Next-game fields added correctly to config.Good use of optional chaining for safety. The
nextMaxPlayerscomputation correctly uses the following item's parameters withconfig.lobbyMaxPlayers().
60-64: Interface extended to support pre-assigned team counts.Adding optional
playerTeamstoMapWithModeenables the look-ahead logic for displaying following game details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/MapPlaylist.ts (1)
84-96: Document the mutation strategy for team count consistency.The logic mutates
upNextItem.playerTeams(line 93) to ensure the team count displayed as "next" in the current lobby matches the actual config in the subsequent lobby. This is clever but not immediately obvious.Add a clarifying comment:
const upNextItem = this.mapsPlaylist[0]; let nextPlayerTeams: TeamCountConfig | undefined; if (upNextItem) { + // Pre-assign team count for upcoming game to ensure consistency + // between what's displayed as "next" and what actually happens if (upNextItem.mode === GameMode.Team && !upNextItem.playerTeams) {Minor: Remove redundant
ensurePlaylistPopulated()call.Line 87 calls
ensurePlaylistPopulated()butgetNextMap()(line 84) already calls it internally (line 153). The second call is harmless but unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/PublicLobby.ts(2 hunks)src/core/Schemas.ts(1 hunks)src/server/MapPlaylist.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/PublicLobby.ts
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (5)
📚 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.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-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/Schemas.tssrc/server/MapPlaylist.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-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/MapPlaylist.ts
🔇 Additional comments (5)
src/core/Schemas.ts (1)
177-180: LGTM! Clean schema extension for next-game metadata.The four optional fields properly support displaying upcoming map rotation info in the public lobby. Types are correct and backward compatible.
src/server/MapPlaylist.ts (4)
63-63: Good addition to support team count consistency.Adding
playerTeamstoMapWithModeenables the playlist to remember team counts assigned when an item is previewed as "next", ensuring the displayed info matches the actual game config later.
98-101: LGTM! Correct fallback for team count assignment.Using
nextItem.playerTeams ?? this.getTeamCount()correctly handles both the first game (where playerTeams is undefined) and subsequent games (where it was pre-assigned when the item was "next").
122-131: LGTM! Proper population of next-game metadata.The next* fields are correctly populated from
upNextItemwith appropriate undefined handling. ThenextMaxPlayerscomputation properly uses the upcoming map/mode/teams data.
139-156: LGTM! Clean refactor for playlist population.The idempotent
ensurePlaylistPopulated()check and early return pattern are clear. ThegetNextMap()method properly ensures the playlist has items before shifting.
There was a problem hiding this comment.
Would be better if it said something like "Next Game"
There was a problem hiding this comment.
That was what I put initially (well it was "Up next", hence the branch name). Then I realized that the button to join the game was actually "Join next Game". This would have result into two usage of "next" and they were not referring to the same game, that might have been confusing, that's why I used another name.
I even thought of changing "Join next Game" by just "Join Game" to get rid of the "next", but I didn't had the courage to change all the translations !
Or maybe I am just overthinking this, I am happy to get it back to to "Up next" or "Next Game" if you find it better.
There was a problem hiding this comment.
I think I prefer "up next"
There was a problem hiding this comment.
I think I prefer "up next"
how about "Starting Soon" and then "Up Next" because I do agree having two "next"s is odd
There was a problem hiding this comment.
Would it make sense to change "Join next game" to "Join this lobby" or simply "Join this game"?
There was a problem hiding this comment.
I changed it to "Join Game" and "Up next": fix displayed text
There was a problem hiding this comment.
I think I prefer "up next"
|
I had to rewrite commits with rebase on main in order to fix conflicts |
- Add next map/mode fields to GameConfig schema - Update MapPlaylist to pre-calculate next map details - Show 'Up Next' section in PublicLobby UI
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/PublicLobby.ts (1)
1-1: Fix Prettier formatting before merge.The CI pipeline detected formatting issues. Please run:
npx prettier --write src/client/PublicLobby.tsor
npx prettier --write .
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
resources/lang/en.jsonsrc/client/PublicLobby.tssrc/core/Schemas.tssrc/core/WorkerSchemas.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/Worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/Master.ts
- src/server/GameManager.ts
- src/server/GameServer.ts
- src/server/Worker.ts
- src/core/WorkerSchemas.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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/PublicLobby.tssrc/server/MapPlaylist.ts
📚 Learning: 2025-08-12T00:30:20.374Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/configuration/DefaultConfig.ts:326-328
Timestamp: 2025-08-12T00:30:20.374Z
Learning: In the OpenFrontIO codebase, game configurations are generated at runtime rather than loaded from saved files, so configuration properties will always be present and don't require fallback defaults for backward compatibility.
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/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
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/client/PublicLobby.tssrc/server/MapPlaylist.ts
🧬 Code graph analysis (2)
src/client/PublicLobby.ts (4)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)src/core/configuration/DefaultConfig.ts (1)
playerTeams(310-312)src/core/Schemas.ts (1)
TeamCountConfig(158-158)
src/server/MapPlaylist.ts (3)
src/core/Schemas.ts (2)
TeamCountConfig(158-158)GameConfig(84-84)src/core/configuration/DefaultConfig.ts (2)
playerTeams(310-312)gameConfig(267-269)src/core/game/GameImpl.ts (1)
map(197-199)
🪛 GitHub Actions: 🧪 CI
src/client/PublicLobby.ts
[error] 1-1: Prettier formatting issue detected in src/client/PublicLobby.ts. Run 'npx prettier --write src/client/PublicLobby.ts' or 'npx prettier --write .' to fix code style issues.
🔇 Additional comments (11)
src/core/Schemas.ts (1)
133-133: LGTM!The optional field addition is clean and follows the existing pattern in the GameInfo interface. The type is correctly defined and available.
resources/lang/en.json (1)
269-277: LGTM! Translation changes look good.The simplified "Join Game" text and new "Up next" label clearly distinguish between the current lobby game and the following game in rotation. The comma addition on line 276 is syntactically correct for JSON.
src/client/PublicLobby.ts (5)
12-12: LGTM!The TeamCountConfig import is necessary for the new helper method and is properly sourced from core/Schemas.
127-136: Good refactoring!Extracting display logic into the
getGameDisplayDetailshelper eliminates duplication and makes the code easier to maintain. ThecurrentMapNamealias clearly distinguishes it from the next game's map.
145-159: LGTM!The next game details calculation correctly reuses the
getGameDisplayDetailshelper and properly handles the optionalnextGameConfig. The logic is consistent with how current game details are computed.
164-221: LGTM! UI changes look good.The increased card height accommodates the new next game section nicely. The conditional rendering properly checks for
nextGameConfigexistence, and the styling is consistent with the rest of the lobby card.
294-323: Excellent helper method!The
getGameDisplayDetailshelper cleanly encapsulates the display logic for both current and next game configurations. It properly handles optional parameters and returns sensible defaults for undefined inputs.src/server/MapPlaylist.ts (4)
63-63: LGTM!Adding the optional
playerTeamsfield toMapWithModeallows the playlist to remember team configurations, ensuring consistency when items move from "next" to "current."
83-122: Logic looks correct but complex.The refactored
gameConfig()properly computes both current and next configurations. The mutation ofupNextItem.playerTeamsat line 96 ensures consistency—when that item becomes current, it uses the same team count.The flow is:
- Get current game (shifts from playlist)
- Ensure playlist populated
- Peek at next game (index 0)
- Assign team counts if needed
- Return both configs
This addresses the past review comment—calling
gameConfig()twice would skip a map due to the shift, so computing both in one call is the right approach.
124-148: Excellent refactoring!Extracting
createConfigas a helper eliminates duplication between current and next game config creation. The method is clean, properly parameterized, and maintains all the original configuration logic.
154-165: Good extraction for readability.Moving the playlist population check into its own helper improves code organization. The early return avoids unnecessary regeneration attempts.
| private getNextMap(): MapWithMode { | ||
| this.ensurePlaylistPopulated(); | ||
| // Even if it failed, playlist will be partially populated. | ||
| return this.mapsPlaylist.shift()!; | ||
| } |
There was a problem hiding this comment.
Non-null assertion may be unsafe.
Line 170 uses the non-null assertion operator (!) on shift(), which could return undefined if the playlist is empty. While the comment says the playlist will be "partially populated" even on failure, there's no explicit guarantee it's never empty.
Consider either:
- Removing the
!and handling theundefinedcase explicitly - Adding a runtime check that throws a descriptive error if the playlist is empty
- Ensuring
shuffleMapsPlaylist()always populates at least one item (with proof/assertion)
🔎 Safer alternative
private getNextMap(): MapWithMode {
this.ensurePlaylistPopulated();
- // Even if it failed, playlist will be partially populated.
- return this.mapsPlaylist.shift()!;
+ const next = this.mapsPlaylist.shift();
+ if (!next) {
+ throw new Error("Map playlist is empty - failed to generate valid playlist");
+ }
+ return next;
}📝 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 getNextMap(): MapWithMode { | |
| this.ensurePlaylistPopulated(); | |
| // Even if it failed, playlist will be partially populated. | |
| return this.mapsPlaylist.shift()!; | |
| } | |
| private getNextMap(): MapWithMode { | |
| this.ensurePlaylistPopulated(); | |
| const next = this.mapsPlaylist.shift(); | |
| if (!next) { | |
| throw new Error("Map playlist is empty - failed to generate valid playlist"); | |
| } | |
| return next; | |
| } |
🤖 Prompt for AI Agents
In src/server/MapPlaylist.ts around lines 167-171 the code uses a non-null
assertion on this.mapsPlaylist.shift() which can return undefined if the
playlist is empty; replace the unsafe assertion with an explicit runtime check:
after ensurePlaylistPopulated() verify this.mapsPlaylist.length > 0 and if not
throw a descriptive Error (e.g. "Maps playlist is empty after population"), then
call shift() (no !) and return the result; alternatively adjust the method
signature to return MapWithMode | undefined and propagate that change — prefer
the throw approach for a clear fail-fast behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/WorkerSchemas.ts (1)
4-15: Schema looks correct, but consider type narrowing complexity.The union schema properly supports three input shapes while maintaining backward compatibility. The
.strict()on the empty object ensures only exact{}matches.One consideration: the inferred type becomes
{ gameConfig: GameConfig; nextGameConfig?: GameConfig } | GameConfig | undefined, which requires careful narrowing in consumer code (as seen inWorker.tslines 132-141). This is acceptable given the backward compatibility requirement.If backward compatibility with raw
GameConfiginput is no longer needed, you could simplify to just the wrapper format:export const CreateGameInputSchema = z .object({ gameConfig: GameConfigSchema.optional(), nextGameConfig: GameConfigSchema.optional(), });This would make consumer code cleaner. But if legacy callers exist, keep the current union.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
resources/lang/en.jsonsrc/client/PublicLobby.tssrc/core/Schemas.tssrc/core/WorkerSchemas.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/Worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/MapPlaylist.ts
- src/server/Master.ts
- src/client/PublicLobby.ts
- src/core/Schemas.ts
- src/server/GameManager.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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/WorkerSchemas.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/WorkerSchemas.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: The PlayerStats type from ArchiveSchemas already includes undefined in its definition, making explicit union types with undefined (PlayerStats | undefined) redundant.
Applied to files:
src/core/WorkerSchemas.ts
📚 Learning: 2025-05-21T04:10:59.706Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:44-53
Timestamp: 2025-05-21T04:10:59.706Z
Learning: In Zod, when a schema has `.optional()` applied at the object level, the TypeScript type inferred using `z.infer` already includes `undefined` in the union type. Therefore, when using such a type as a return type annotation, there's no need to explicitly add `| undefined`.
Applied to files:
src/core/WorkerSchemas.ts
📚 Learning: 2025-08-16T16:16:00.414Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1758
File: src/core/ApiSchemas.ts:63-72
Timestamp: 2025-08-16T16:16:00.414Z
Learning: In Zod v4, z.enum() can accept TypeScript native enums directly (e.g., z.enum(MyEnum)). The z.nativeEnum() function was deprecated in v4 and replaced by the overloaded z.enum(). This means z.enum(GameType), z.enum(GameMode), etc. are valid in Zod v4.
Applied to files:
src/core/WorkerSchemas.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
🧬 Code graph analysis (3)
src/server/Worker.ts (4)
src/server/MapPlaylist.ts (1)
gameConfig(83-122)src/core/configuration/DefaultConfig.ts (1)
gameConfig(267-269)src/core/Schemas.ts (1)
GameConfig(84-84)src/server/GameManager.ts (1)
game(25-27)
src/core/WorkerSchemas.ts (1)
src/core/Schemas.ts (1)
GameConfigSchema(160-178)
src/server/GameServer.ts (1)
src/core/Schemas.ts (1)
GameConfig(84-84)
🔇 Additional comments (6)
resources/lang/en.json (1)
269-269: LGTM! Clean resolution to the naming discussion.The changes correctly implement the consensus from past review comments: simplifying the join button text to "Join Game" and using "Up next" for the new section heading eliminates the confusion about having two different references to "next."
Both values are clear, concise, and idiomatic English.
Also applies to: 277-277
src/server/GameServer.ts (2)
72-82: LGTM!Clean addition of the optional
nextGameConfigparameter. The public property is well-typed and correctly positioned in the constructor signature.
647-660: LGTM!The
nextGameConfigis properly included ingameInfo(). WhennextGameConfigisundefined, it will be omitted from the JSON response (or included asundefined), which is the expected behavior for optional fields.src/server/Worker.ts (3)
163-168: LGTM!The
createGamecall correctly passes bothgameConfigandnextGameConfigto theGameManager, and the logging message properly referencesgameConfig?.gameMode.
570-571: LGTM!Clean destructuring from
playlist.gameConfig()and proper forwarding of both config values tocreateGame. This aligns well with theMapPlaylist.gameConfig()return type shown in the relevant snippets.
132-141: The code is safe and handles all cases correctly. When the request body is an empty object{}, it transforms toundefined, and the conditioninput && "gameConfig" in inputshort-circuits safely without throwing a TypeError. Theinoperator is never evaluated wheninputisundefined, allowing the code to proceed to the else branch and correctly assignundefinedtogameConfig.
| modeLabel: nextModeLabel, | ||
| teamDetailLabel: nextTeamDetailLabel, | ||
| mapName: nextMapName, | ||
| } = this.getGameDisplayDetails( |
There was a problem hiding this comment.
would be a bit cleaner to pass the entire game config into getGameDisplayDetails, instead of passing in 4 arguments.
| </div> | ||
| <div class="text-md font-medium text-white-400">${timeDisplay}</div> | ||
| </div> | ||
| ${lobby.nextGameConfig |
There was a problem hiding this comment.
this makes the PublicLobby component too large. would it be possible to keep it the same size, and have the "up next" be a small chip on the bottom left.
| .or(GameConfigSchema) | ||
| .or( | ||
| z | ||
| .object({}) | ||
| .strict() | ||
| .transform((val) => undefined), | ||
| ); |
There was a problem hiding this comment.
would it be possible to remove these two .or, instead just have:
.object({
gameConfig: GameConfigSchema,
nextGameConfig: GameConfigSchema.optional(),
})
There was a problem hiding this comment.
do we need to modify this file at all? why not just call "gameConfig()" twice?
There was a problem hiding this comment.
From delowan 3 days ago
If we do that, we will consume a map from the playlist (with the shift()), so the next map would be actually skipped.
|
@DeLoWaN how's it going? |
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
|
@DeLoWaN this has been set to draft due to conflicts, please reopen it when ready |
6 participants
Description:
Show a "Following Game" section in the lobby to see the next map in the current rotation. Useful for example in teams to plan when to join a game (some players leave the game after joining-in if their team mates did not join):
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
delovan