Validate private lobby creation and modification with host's persistentID #2320
Conversation
WalkthroughAdds optional hostPersistentID end-to-end: GameServer stores it; Worker parses and enforces it for private-game create/start/update flows; client HostLobbyModal sends it on create/start/update and surfaces an error on create failure; schemas and localization updated to accept and surface the field. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant UI as HostLobbyModal (client)
participant W as Worker (HTTP)
participant G as GameServer.Instance
rect rgba(235,245,255,0.9)
Note over C,UI: Create lobby — include hostPersistentID when available
C->>UI: createLobby()
UI->>W: POST /create_game { ..., hostPersistentID? }
W->>G: create game instance
alt hostPersistentID provided
G->>G: setHostPersistentID(hostPersistentID)
else not provided and game is private
W-->>UI: 200 OK + warning logged
end
W-->>UI: 200 OK { gameInfo }
end
rect rgba(235,255,235,0.9)
Note over C,W: Start/update private game — host check
C->>UI: startGame()/updateGame()
UI->>W: POST /start_game or PUT /api/game { hostPersistentID? }
W->>G: getHostPersistentID()
alt game has hostPersistentID
alt provided && matches
W-->>UI: 200 OK (proceed)
else
W-->>UI: 403 Forbidden
end
else
W-->>UI: proceed (no host check)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/Worker.ts (1)
161-185: Authorization is broken and early returns hang requests—both server and client need fixes.The issues are confirmed:
- Route
/api/start_game/:idhas no:hostTokenparameter, soreq.params.hostTokenis always undefined.- Early
returnstatements at lines 165 and 172 send no response, hanging the request.- Client code at
src/client/HostLobbyModal.ts:798-810sends nohostTokenin the body at all.Apply server changes in
src/server/Worker.ts(161–185):+import { timingSafeEqual } from "node:crypto"; app.post("/api/start_game/:id", async (req, res) => { log.info(`starting private lobby with id ${req.params.id}`); const game = gm.game(req.params.id); if (!game) { - return; + return res.status(404).json({ error: "Game not found" }); } if (game.isPublic()) { const clientIP = req.ip || req.socket.remoteAddress || "unknown"; log.info( `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, ); - return; + return res.status(400).json({ error: "Cannot start public game" }); } - if ( - !("hostToken" in req.params) || - req.params.hostToken !== game.getHostToken() - ) { - log.info("cannot start private game, requestor is not host"); - res.status(403); - return; - } + const tokenSchema = z.object({ + hostToken: z.string().regex(/^[A-F0-9]{16}$/), + }); + const parsed = tokenSchema.safeParse(req.body); + if (!parsed.success) { + const error = z.prettifyError(parsed.error); + return res.status(400).json({ error }); + } + const expected = game.getHostToken(); + if (!expected) { + return res.status(409).json({ error: "Host token not set for this game" }); + } + const suppliedBuf = Buffer.from(parsed.data.hostToken, "utf8"); + const expectedBuf = Buffer.from(expected, "utf8"); + if ( + suppliedBuf.length !== expectedBuf.length || + !timingSafeEqual(suppliedBuf, expectedBuf) + ) { + log.info("cannot start private game, requester is not host"); + return res.status(403).json({ error: "Forbidden" }); + } game.start(); res.status(200).json({ success: true }); });Update client at
src/client/HostLobbyModal.ts:798-810to send the token in body. The fetch call needs a body withhostToken. Where doesthis.lobbyIdor similar contain or reference the host token? Please confirm the token source so the client code can be corrected.
🧹 Nitpick comments (2)
src/server/Worker.ts (1)
151-153: Only include hostToken for private games; avoid spreading an object wrapper.Return the token only for private lobbies and keep the response shape simple.
Apply:
- const token = game.createHostToken(); - const hostToken = { hostToken: token }; + const hostToken = game.isPublic() ? undefined : game.createHostToken(); ... - res.json({ ...game.gameInfo(), ...hostToken }); + res.json( + hostToken ? { ...game.gameInfo(), hostToken } : game.gameInfo(), + );This avoids leaking an unnecessary secret for public games and keeps the JSON compact.
Also applies to: 157-157
src/server/GameServer.ts (1)
145-148: Add an explicit return type and consider null‑clearing semantics.Small polish and safety:
- Add return type for clarity.
- Consider clearing the token after it’s used (e.g., right after
start()).Apply:
- public getHostToken() { - return this.hostToken ?? null; - } + public getHostToken(): string | null { + return this.hostToken ?? null; + }If you decide to clear the token on start:
// At the end of start(): this.hostToken = null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/GameServer.ts(2 hunks)src/server/Worker.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/Worker.ts (1)
src/server/GameManager.ts (1)
game(24-26)
🪛 GitHub Check: 🔍 ESLint
src/server/GameServer.ts
[failure] 134-134:
'tokenLength' is assigned a value but never used.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/client/HostLobbyModal.ts (3)
801-809: Cookie parsing: simplify and harden.Consider a small helper to read cookies; reduces bugs and improves readability.
Example:
+function getCookie(name: string): string | null { + const m = document.cookie.match(new RegExp(`(?:^|; )${name}=([^;]*)`)); + return m ? decodeURIComponent(m[1]) : null; +} ... - const cookies = document.cookie.split(";"); - let hostToken = ""; - for (let i = 0; i < cookies.length; i++) { - const cookie = cookies[i]; - if (cookie.trim().startsWith("hostToken=")) { - hostToken = cookie.split("=")[1]; - break; - } - } + const hostToken = getCookie("hostToken") ?? "";
870-873: Type shape consistency for createLobby response.You re-wrap server data into
{ gameInfo, hostToken }. Good. If TypeScript ever complains at call sites (e.g., “property hostToken missing”), ensure this function’s return type is used by callers and consider validating the server payload.Optional hardening:
+type CreateLobbyResponse = { gameInfo: GameInfo; hostToken: string }; +const CreateLobbySchema = z.object({ + gameInfo: z.any(), // or stricter GameInfo schema if available + hostToken: z.string().min(1), +}); ... -): Promise<{ gameInfo: GameInfo; hostToken: string }> { +): Promise<CreateLobbyResponse> { ... - const data = await response.json(); + const data = await response.json(); + // If server returns flat { ...gameInfo, hostToken }, normalize here: + const normalized = { gameInfo: data, hostToken: data.hostToken }; + // Optionally validate: + // CreateLobbySchema.parse(normalized); - return { gameInfo: data, hostToken: data.hostToken }; + return normalized;Also applies to: 896-896
60-63: Timer typings cross‑env.For browser code, prefer
ReturnType<typeof setInterval/setTimeout>to avoid NodeJS.Timeout vs number mismatches.- private playersInterval: NodeJS.Timeout | null = null; + private playersInterval: ReturnType<typeof setInterval> | null = null; - private botsUpdateTimer: number | null = null; + private botsUpdateTimer: ReturnType<typeof setTimeout> | null = null;src/server/Worker.ts (2)
174-181: Validate request body and tighten responses.Add schema validation for
hostToken; keep responses explicit.- const hostToken = req.body.hostToken ?? ""; + const body = z.object({ hostToken: z.string().min(1) }).safeParse(req.body); + if (!body.success) { + return res.status(400).json({ error: "Invalid body" }); + } + const { hostToken } = body.data; ... - log.info(`cannot start private game ${game.id}, requestor is not host`); - res.status(403).json({ success: false }); - return; + log.info(`cannot start private game ${game.id}, requester is not host`); + return res.status(403).json({ success: false });Also consider responding 404 when game is missing:
- if (!game) { - return; - } + if (!game) { + return res.status(404).json({ error: "Game not found" }); + }
151-157: Response shape: return{ gameInfo, hostToken }instead of flattening.This avoids mixing concerns and matches the client’s natural usage.
- const hostToken = { hostToken: game.createHostToken() }; ... - res.json({ ...game.gameInfo(), ...hostToken }); + const hostToken = game.createHostToken(); + res.json({ gameInfo: game.gameInfo(), hostToken });Please confirm no other clients depend on the flat shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.ts(4 hunks)src/server/GameServer.ts(3 hunks)src/server/Worker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/GameServer.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/Worker.ts (1)
src/server/GameManager.ts (1)
game(24-26)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🪛 Biome (2.1.2)
src/server/Worker.ts
[error] 178-178: expected , but instead found ;
Remove ;
(parse)
🪛 GitHub Actions: 🧪 CI
src/server/Worker.ts
[error] 178-178: SyntaxError: ')' expected while running 'npx prettier --check .' (likely missing closing parenthesis or semicolon in response. Add ); to properly close res.status(403).json({ success: false }).
🪛 GitHub Check: 🔍 ESLint
src/server/Worker.ts
[failure] 178-178:
Parsing error: ')' expected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/HostLobbyModal.ts (2)
595-600: Fix cookie typo and add security attributes.The cookie setting still has "Max-Axe" instead of "Max-Age", and is missing URL encoding and security flags (SameSite, Secure).
This issue was already flagged in a previous review at lines 597-600.
821-821: Cookie deletion needs matching attributes.The cookie deletion is missing the Path, SameSite, and Secure attributes used when setting the cookie. This can cause the deletion to fail.
This issue was already flagged in a previous review at line 821.
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
818-818: Use object shorthand.Simplify
{ hostToken: hostToken }to{ hostToken }.- body: JSON.stringify({ hostToken: hostToken }), + body: JSON.stringify({ hostToken }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🔇 Additional comments (1)
src/client/HostLobbyModal.ts (1)
868-899: API returns hostToken—no validation needed.The backend endpoint (Worker.ts line 156) spreads
hostTokeninto every successful response:res.json({ ...game.gameInfo(), ...hostToken }). The assumption in the client code is valid and safe.
Lavodan
changed the title
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/server/Worker.ts (1)
160-173: Bug: early returns without HTTP response.Both “game not found” and “game is public” paths return without sending a response; callers will hang. Return proper status codes.
- if (!game) { - return; - } + if (!game) { + return res.status(404).json({ error: "Game not found" }); + } if (game.isPublic()) { // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing const clientIP = req.ip || req.socket.remoteAddress || "unknown"; - log.info( + log.warn( `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, ); - return; + return res.status(400).json({ error: "Cannot start a public game" }); }
♻️ Duplicate comments (4)
src/server/Worker.ts (1)
178-179: Syntax fix acknowledged.The missing parenthesis previously flagged here is now fixed; CI should pass this spot.
src/client/HostLobbyModal.ts (3)
801-810: Make cookie parsing robust and URL‑decode the value.Handle values with “=” and decode.
- const cookies = document.cookie.split(";"); - let hostToken = ""; - for (let i = 0; i < cookies.length; i++) { - const cookie = cookies[i]; - if (cookie.trim().startsWith("hostToken=")) { - hostToken = cookie.split("=")[1]; - break; - } - } + const cookies = document.cookie.split(";"); + let hostToken = ""; + for (const cookie of cookies) { + const trimmed = cookie.trim(); + if (trimmed.startsWith("hostToken=")) { + hostToken = decodeURIComponent(trimmed.substring("hostToken=".length)); + break; + } + } + if (!hostToken) { + console.warn("startGame aborted: missing hostToken cookie"); + return; + }
595-600: Harden cookie: URL‑encode and add security attributes; remove stray comment.Encode the token and set SameSite and Secure (when HTTPS). The inline “getting error…” comment should be removed.
- const cookieDurationSec = 60 * 60 * 6; //Store cookie for max 6 hours - // getting error here from lobby.hostToken - document.cookie = `hostToken=${lobby.hostToken}; Max-Age=${cookieDurationSec}; Path=/`; + const cookieDurationSec = 60 * 60 * 6; // store cookie for max 6 hours + const secure = location.protocol === "https:" ? "; Secure" : ""; + document.cookie = `hostToken=${encodeURIComponent(lobby.hostToken)}; Max-Age=${cookieDurationSec}; Path=/; SameSite=Strict${secure}`;
892-896: Do not log tokens (response includes hostToken).
console.log("Success:", data)prints the token. Remove or log only non‑sensitive fields.- const data = await response.json(); - console.log("Success:", data); - - return { gameInfo: data, hostToken: data.hostToken }; + const data = await response.json(); + // Optional: debug minimal info + // console.debug("Lobby created", { gameID: data.gameID }); + return { gameInfo: data, hostToken: data.hostToken };
🧹 Nitpick comments (3)
src/server/Worker.ts (1)
174-183: Validate request body and align error shape.Parse
hostTokenwith Zod, reject missing/empty values, and keep error shape consistent across APIs. Optionally invalidate the host token after a successful start to prevent reuse.+ const StartGameSchema = z.object({ hostToken: z.string().min(1) }); + const parsed = StartGameSchema.safeParse(req.body); + if (!parsed.success) { + const error = z.prettifyError ? z.prettifyError(parsed.error) : "Invalid body"; + return res.status(400).json({ error }); + } - const hostToken = req.body.hostToken ?? ""; + const { hostToken } = parsed.data; if (hostToken !== game.getHostToken()) { - log.info(`cannot start private game ${game.id}, requestor is not host`); - res.status(403).json({ success: false }); - return; + log.warn(`cannot start private game ${game.id}, requestor is not host`); + return res.status(403).json({ error: "Forbidden" }); } game.start(); - res.status(200).json({ success: true }); + // Optional: make token one-time. If supported: + // game.invalidateHostToken?.(); + return res.status(200).json({ success: true });src/client/HostLobbyModal.ts (2)
818-819: Tiny cleanup: shorthand property.Use
{ hostToken }for brevity.- body: JSON.stringify({ hostToken: hostToken }), + body: JSON.stringify({ hostToken }),
851-854: Noisy console log in production path.Logging full
GameInfoeach second is noisy and may include usernames. Consider removing or guard behind a debug flag.- console.log(`got game info response: ${JSON.stringify(data)}`); + // console.debug("got game info response");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/HostLobbyModal.ts(4 hunks)src/server/Worker.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/Worker.ts (1)
src/server/GameManager.ts (1)
game(24-26)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🔇 Additional comments (1)
src/client/HostLobbyModal.ts (1)
821-823: Good: delete cookie with matching attributes.Deletion mirrors Path and SameSite and adds Secure when applicable.
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 (2)
src/client/HostLobbyModal.ts (2)
789-824: Validate hostToken presence before starting game.If the cookie expired or was deleted,
hostTokenwill be an empty string (line 803). The request will proceed and likely fail server-side without clear user feedback. Consider checking that the token exists before calling the API.Add validation before the fetch:
} + if (!hostToken) { + console.error("Host token missing - cannot start private lobby"); + alert("Session expired. Please create a new lobby."); + return; + } const response = await fetch(
869-895: Validate hostToken at response time and update caller to handle null case.Your review identifies a real bug. The code has a type-caller mismatch:
- createLobby returns
hostToken: string | nullwith no validation (line 895)- Caller at line 599 uses
encodeURIComponent(lobby.hostToken)directly without checking for null- If the server omits
hostToken, the code silently sets the cookie tohostToken=null(the string), which breaks authenticationFix with validation + type change (preferred):
async function createLobby( creatorClientID: string, -): Promise<{ gameInfo: GameInfo; hostToken: string | null }> { +): Promise<{ gameInfo: GameInfo; hostToken: string }> { // ... fetch logic ... const data = await response.json(); console.log("Success:", data); + if (!data.hostToken || typeof data.hostToken !== "string") { + throw new Error("Server returned invalid hostToken"); + } return { gameInfo: data, hostToken: data.hostToken };This approach is cleaner because:
- You validate once at the source, not at every call site
- Type system now prevents null
- Errors are explicit, not silent cookie corruption
- Caller at line 599 works safely without changes
Add tests to verify the server always returns valid
hostTokenin the response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/HostLobbyModal.ts(4 hunks)src/server/GameServer.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
PR: openfrontio/OpenFrontIO#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-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🪛 GitHub Actions: 🧪 CI
src/client/HostLobbyModal.ts
[error] 599-599: TS2345: Argument of type 'string | null' is not assignable to parameter of type 'string | number | boolean'. Type 'null' is not assignable to type 'string | number | boolean'.
🔇 Additional comments (1)
src/client/HostLobbyModal.ts (1)
801-810: Cookie parsing looks good.The manual parsing correctly handles URL-encoded tokens and
=characters within the value usingslice(1).join("=")anddecodeURIComponent.
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/client/HostLobbyModal.ts (1)
869-895: Silent failure: validate server response includes hostToken.Line 895 returns an empty string if
data.hostTokenis missing (??operator), but the function signature promiseshostToken: string. An empty string satisfies the type contract but isn't a valid token. This silent failure means the call site at line 593 will store an invalid cookie without realizing the server didn't provide a token.Apply this diff to fail fast when the server doesn't return a token:
const data = await response.json(); console.log("Success:", data); - return { gameInfo: data, hostToken: data.hostToken ?? "" }; + if (!data.hostToken || typeof data.hostToken !== "string") { + throw new Error("Server did not return a valid hostToken"); + } + return { gameInfo: data, hostToken: data.hostToken };
♻️ Duplicate comments (2)
src/client/HostLobbyModal.ts (2)
800-810: Cookie parsing is fragile and missing URL decoding.The manual parsing at line 807 uses
cookie.split("=")[1], which breaks if the token contains=characters. It also lacksdecodeURIComponentto match the encoding that should be applied when the cookie is set.Apply this diff:
// Parse cookies for hostToken const cookies = document.cookie.split(";"); let hostToken = ""; - for (let i = 0; i < cookies.length; i++) { - const cookie = cookies[i]; - if (cookie.trim().startsWith("hostToken=")) { - hostToken = cookie.split("=")[1]; + for (const cookie of cookies) { + const trimmed = cookie.trim(); + if (trimmed.startsWith("hostToken=")) { + hostToken = decodeURIComponent(trimmed.split("=").slice(1).join("=")); break; } }
593-600: Guard against null hostToken and fix cookie attributes.The code has two critical issues:
Type error:
lobby.hostTokenis typed asstring | null(per the return type at line 871), but line 599 doesn't check for null before using it. If the server fails to return a token, this will cause a runtime error.Cookie security mismatch: Line 599 sets the cookie without URL encoding,
SameSite, or conditionalSecureflag, but line 822 includes these when deleting. This mismatch will cause the deletion to fail, leaving the token in the browser.Apply this diff to fix both issues:
createLobby(this.lobbyCreatorClientID) .then((lobby) => { + if (!lobby.hostToken) { + throw new Error("Server did not return hostToken for private lobby"); + } this.lobbyId = lobby.gameInfo.gameID; // join lobby const cookieDurationSec = 60 * 60 * 6; //Store cookie for max 6 hours - // getting error here from lobby.hostToken - document.cookie = `hostToken=${lobby.hostToken}; Max-Age=${cookieDurationSec}; Path=/`; + const secure = location.protocol === "https:" ? "; Secure" : ""; + document.cookie = `hostToken=${encodeURIComponent(lobby.hostToken)}; Max-Age=${cookieDurationSec}; Path=/; SameSite=Strict${secure}`; })
🧹 Nitpick comments (2)
src/client/HostLobbyModal.ts (2)
818-818: Use object shorthand.Simplify
{ hostToken: hostToken }to{ hostToken }.- body: JSON.stringify({ hostToken: hostToken }), + body: JSON.stringify({ hostToken }),
869-895: Add tests for server response contract.The function assumes the server will include
hostTokenin the response, but there's no test coverage verifying this contract. Add unit or integration tests that validate:
- Server response includes a non-empty
hostTokenstring- Function throws when
hostTokenis missing or invalid
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
PR: openfrontio/OpenFrontIO#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
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🔇 Additional comments (1)
src/client/HostLobbyModal.ts (1)
821-822: Cookie deletion looks correct.The deletion properly includes matching attributes (
Path,SameSite, conditionalSecure) and usesMax-Age=0.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/HostLobbyModal.ts (1)
1-1: Fix code formatting.The pipeline detected code style issues. Run Prettier to fix:
npx prettier --write src/client/HostLobbyModal.ts
🧹 Nitpick comments (3)
src/client/HostLobbyModal.ts (3)
804-813: Validate hostToken before sending request.If the cookie is missing or expired,
hostTokenwill be an empty string and still sent to the server. The server will likely reject it, but making this explicit on the client improves UX.Add a check before the fetch:
// Parse cookies for hostToken const cookies = document.cookie.split(";"); let hostToken = ""; for (const cookie of cookies) { const trimmed = cookie.trim(); if (trimmed.startsWith("hostToken=")) { hostToken = decodeURIComponent(trimmed.split("=").slice(1).join("=")); break; } } + + if (!hostToken) { + console.error("No hostToken found - cannot start game"); + alert(translateText("errors.host_token_missing")); + return; + } + const response = await fetch(
821-821: Use shorthand property syntax.- body: JSON.stringify({ hostToken: hostToken }), + body: JSON.stringify({ hostToken }),
895-898: Consider validating server response shape.The return statement assumes
dataincludeshostToken, but if the server omits it,nullis returned silently. While the null check at line 595 catches this later, validating here would fail faster and provide clearer error context.Optional improvement:
const data = await response.json(); console.log("Success:", data); + if (typeof data.hostToken !== "string") { + throw new Error("Server response missing or invalid hostToken field"); + } + - return { gameInfo: data, hostToken: data.hostToken ?? null }; + return { gameInfo: data, hostToken: data.hostToken };This makes the function's contract stricter (always returns string) and clarifies where validation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
PR: openfrontio/OpenFrontIO#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
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
GameInfo(125-131)
🪛 GitHub Actions: 🧪 CI
src/client/HostLobbyModal.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🔇 Additional comments (3)
src/client/HostLobbyModal.ts (3)
598-598: LGTM!Correctly accesses
gameIDfrom the nestedgameInfoobject after the return type change.
600-602: Cookie settings look good.The cookie attributes (Max-Age, Path, SameSite, conditional Secure) are properly set. SameSite=Strict is appropriate for preventing CSRF, though if the app needs to work in cross-site contexts (e.g., embeds), consider SameSite=Lax instead.
872-874: Typed return contract is clear.The explicit return type
{ gameInfo: GameInfo; hostToken: string | null }makes the contract clear. However, the past review comment about adding tests to validate the server response is still relevant.Consider adding tests that:
- Verify the server API endpoint includes
hostTokenin responses- Test the null check at line 595 triggers when server returns null
- Mock various server response shapes to ensure type safety
This would prevent runtime issues if the server response format changes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/HostLobbyModal.ts (1)
1-1: Run prettier to fix formatting.The pipeline detected code style issues. Please run:
npx prettier --write src/client/HostLobbyModal.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
PR: openfrontio/OpenFrontIO#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
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
PR: openfrontio/OpenFrontIO#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-08-17T20:48:49.411Z
Learnt from: woodydrn
PR: openfrontio/OpenFrontIO#1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/core/Schemas.ts (1)
GameInfo(125-131)
🪛 GitHub Actions: 🧪 CI
src/client/HostLobbyModal.ts
[warning] 1-1: Code style issues found in the file. Run 'npx prettier --write' to fix.
🔇 Additional comments (5)
resources/lang/en.json (1)
219-220: LGTM! Translation key added correctly.The new
creation_errorkey provides user feedback when lobby creation fails, and the comma fix maintains valid JSON syntax.src/client/HostLobbyModal.ts (4)
595-602: Good: null check and secure cookie attributes.The guard prevents
nullfrom reachingencodeURIComponent, and the cookie now includes properMax-Age,Path,SameSite, and conditionalSecureflags.
812-821: Cookie parsing handles edge cases correctly.The implementation properly URL-decodes the token and handles
=characters within the value usingslice(1).join("=").
832-833: Cookie deletion uses matching attributes.The deletion correctly uses
Max-Age=0with the samePath,SameSite, andSecureflags as creation.
880-882: Clear type contract with null handling.The function signature explicitly returns
string | nullforhostToken, and the implementation uses??to coalesceundefinedtonull, matching the contract.Also applies to: 906-906
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/HostLobbyModal.ts (1)
616-623: Remove popup from DOM to prevent memory leak.Each failed lobby creation appends a new
divtodocument.bodythat is never removed. On repeated failures, orphan elements accumulate.Add cleanup logic:
.catch((err) => { console.error(`Failed to create lobby: ${err}`); const popup = document.createElement("div"); - popup.className = "setting-popup"; // TODO: Change to general popup class? + popup.className = "error-popup"; popup.textContent = translateText("private_lobby.creation_error"); document.body.appendChild(popup); + setTimeout(() => { + popup.remove(); + }, 3000); this.close(); });Alternatively, use an existing toast/notification system if available in the codebase.
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
829-829: Validate hostToken before sending.If the cookie is missing,
hostTokenwill be an empty string. The server will reject it, but the user sees no feedback until after the network round-trip.Add validation and use shorthand syntax:
+ if (!hostToken) { + console.error("No hostToken found in cookies"); + alert(translateText("errors.missing_host_token")); + return; + } const response = await fetch( `${window.location.origin}/${config.workerPath(this.lobbyId)}/api/start_game/${this.lobbyId}`, { method: "POST", headers: { "Content-Type": "application/json", }, - body: JSON.stringify({ hostToken: hostToken }), + body: JSON.stringify({ hostToken }), }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
PR: openfrontio/OpenFrontIO#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-08-17T20:48:49.411Z
Learnt from: woodydrn
PR: openfrontio/OpenFrontIO#1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/core/Schemas.ts (1)
GameInfo(125-131)
🔇 Additional comments (4)
src/client/HostLobbyModal.ts (4)
595-602: Nice: null check prevents type error.The guard correctly narrows the type and ensures
encodeURIComponentreceives a string. Cookie attributes (Max-Age, Path, SameSite, Secure) are properly set.
812-821: LGTM!Cookie parsing correctly handles tokens containing "=" characters and applies URL decoding.
832-833: LGTM!Cookie deletion uses matching attributes (Path, SameSite, Secure) to ensure removal.
880-882: LGTM!The function signature clearly promises both
gameInfoandhostToken(nullable). The return statement correctly coalesces missinghostTokentonull.Also applies to: 906-906
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/server/Worker.ts (3)
157-165: Critical: Fix logic error in hostPersistentID validation.Line 161 checks
!hostPersistentID, buthostPersistentIDis the object{ hostPersistentID: ... }, which is always truthy. The warning never logs even when the ID is null, and the response incorrectly sendshostPersistentID: null.Apply this fix:
// Pass creatorClientID to createGame const game = gm.createGame(id, gc, creatorClientID); log.info( `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, ); - const hostPersistentID = { - hostPersistentID: game.setHostPersistentID(req.body.hostPersistentID), - }; + const hostPersistentID = game.setHostPersistentID(req.body.hostPersistentID); if (!hostPersistentID) { log.warn( `Worker ${workerId}: IP ${ipAnonymize(clientIP)} id ${id} did not receive hostPersistentID, game may be started by anyone`, ); } - res.json({ ...game.gameInfo(), ...hostPersistentID }); + res.json({ ...game.gameInfo(), hostPersistentID });
186-194: Fix indentation: use spaces instead of tabs.The authorization block uses tabs for indentation while the rest of the file uses spaces. This breaks Prettier formatting.
Apply this diff:
- - const hostPersistentID = req.body.hostPersistentID ?? ""; - const gameHostPersistentID = game.getHostPersistentID(); - - // If gameHostPersistentID is null, no auth is done - if (gameHostPersistentID && hostPersistentID !== gameHostPersistentID) { - log.info(`cannot start private game ${game.id}, requestor is not host`); - res.status(403).json({ success: false }); - return; - } + + const hostPersistentID = req.body.hostPersistentID ?? ""; + const gameHostPersistentID = game.getHostPersistentID(); + + // If gameHostPersistentID is null, no auth is done + if (gameHostPersistentID && hostPersistentID !== gameHostPersistentID) { + log.info(`cannot start private game ${game.id}, requestor is not host`); + res.status(403).json({ success: false }); + return; + }
123-123: Clarify or resolve the TODO comment.The TODO states "this destroys hostPersistentID requests, but also I cannot put hostPersistentID in configschema". This is unclear. If
CreateGameInputSchemastripshostPersistentID, extract it before validation:const clientIP = req.ip || req.socket.remoteAddress || "unknown"; + // Extract hostPersistentID before schema validation + const hostPersistentIDFromClient = req.body.hostPersistentID; - // TODO: this destroys hostPersistentID requests, but also I cannot put hostPersistentID in configschema const result = CreateGameInputSchema.safeParse(req.body);Then use
hostPersistentIDFromClientat line 157. Remove or update the TODO once resolved.src/server/GameServer.ts (1)
132-142: Fix indentation and consider throwing on duplicate set.Two issues:
- Indentation: Uses tabs instead of spaces (inconsistent with file)
- Silent failure: Returning null when already set hides bugs. Worker.ts line 157 doesn't check the return value.
Apply this diff:
- public setHostPersistentID(id: string): string | null { - if (this.getHostPersistentID()) { - return null; - } - this._hostPersistentID = id; - return this.getHostPersistentID(); - } - - public getHostPersistentID(): string | null { - return this._hostPersistentID ?? null; - } + public setHostPersistentID(id: string): string | null { + if (this.getHostPersistentID()) { + this.log.warn(`hostPersistentID already set for game ${this.id}`); + return null; + } + this._hostPersistentID = id; + return this.getHostPersistentID(); + } + + public getHostPersistentID(): string | null { + return this._hostPersistentID ?? null; + }Alternatively, throw an error instead of returning null to make failures explicit. If keeping null return, update Worker.ts to check and log the return value.
src/client/HostLobbyModal.ts (1)
818-818: Validate getPersistentID() returns non-empty string before sending.Based on the code in
src/client/Main.tslines 628-632,getPersistentID()delegates togetPersistentIDFromCookie()which can return an empty string if a cookie exists with an empty value. Sending an emptyhostPersistentIDto the server bypasses authorization silently.Add validation before both requests:
private async startGame() { if (this.useRandomMap) { this.selectedMap = this.getRandomMap(); } await this.putGameConfig(); console.log( `Starting private game with map: ${GameMapType[this.selectedMap as keyof typeof GameMapType]} ${this.useRandomMap ? " (Randomly selected)" : ""}`, ); this.close(); const config = await getServerConfigFromClient(); + + const hostPersistentID = getPersistentID(); + if (!hostPersistentID) { + console.error("Cannot start game without persistent ID"); + alert(translateText("errors.missing_persistent_id")); + return; + } const response = await fetch( `${window.location.origin}/${config.workerPath(this.lobbyId)}/api/start_game/${this.lobbyId}`, { method: "POST", headers: { "Content-Type": "application/json", }, - body: JSON.stringify({ hostPersistentID: getPersistentID() }), + body: JSON.stringify({ hostPersistentID }), }, ); return response; }Apply the same pattern at line 880 in
createLobby:const id = generateID(); + const hostPersistentID = getPersistentID(); + if (!hostPersistentID) { + throw new Error("Cannot create lobby without persistent ID"); + } const response = await fetch( `/${config.workerPath(id)}/api/create_game/${id}?creatorClientID=${encodeURIComponent(creatorClientID)}`, { method: "POST", headers: { "Content-Type": "application/json", }, - body: JSON.stringify({ hostPersistentID: getPersistentID() }), + body: JSON.stringify({ hostPersistentID }), }, );
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
867-893: Consider narrowing return type after validation.The return type allows
hostPersistentID: string | null, but the caller at line 595 immediately validates and throws if null. Consider either:
- Having
createLobbythrow if the server returns null (fail-fast), or- Narrow the return type to
stringand document that it throws on nullThis makes the type contract clearer:
async function createLobby( creatorClientID: string, ): Promise<{ gameInfo: GameInfo; hostPersistentID: string }> { // ... existing code ... const data = await response.json(); console.log("Success:", data); const hostPersistentID = data.hostPersistentID ?? null; if (!hostPersistentID) { throw new Error("Server did not return hostPersistentID"); } return { gameInfo: data, hostPersistentID }; }This eliminates the null check at line 595 since the type guarantees a string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.ts(7 hunks)src/server/GameServer.ts(2 hunks)src/server/Worker.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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/client/HostLobbyModal.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.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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.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/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: 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/client/HostLobbyModal.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (3)
src/client/Utils.ts (1)
translateText(92-147)src/client/Main.ts (1)
getPersistentID(628-632)src/core/Schemas.ts (1)
GameInfo(125-131)
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 (4)
src/client/HostLobbyModal.ts(7 hunks)src/core/WorkerSchemas.ts(1 hunks)src/server/GameServer.ts(2 hunks)src/server/Worker.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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/client/HostLobbyModal.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: 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-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/WorkerSchemas.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.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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/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: 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/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/HostLobbyModal.tssrc/server/GameServer.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (4)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(254-274)src/client/Main.ts (1)
getPersistentID(628-632)src/core/Schemas.ts (1)
GameInfo(125-131)
🪛 GitHub Check: 🔍 ESLint
src/server/Worker.ts
[failure] 156-156:
'hostPersistentID' is assigned a value but never used.
🔇 Additional comments (9)
src/server/GameServer.ts (1)
35-35: LGTM: Clean field declaration.The private field follows TypeScript conventions with the underscore prefix and nullable typing.
src/server/Worker.ts (2)
160-164: Good: Helpful warning log for missing host ID.This warning helps identify when private lobbies are created without authorization tokens, which is useful for debugging and security monitoring.
185-193: Authorization logic is sound.The check correctly:
- Defaults missing
hostPersistentIDto empty string- Only enforces authorization when
gameHostPersistentIDis set- Returns 403 on mismatch
The empty string handling is consistent with the fallback behavior, though it relies on the earlier GameServer validation (see GameServer.ts review comment).
src/core/WorkerSchemas.ts (1)
4-9: Good: UUID validation rejects invalid formats.The
.uuid()validator ensureshostPersistentIDis a valid UUID when present, rejecting empty strings and malformed values at the schema level. The.transform(() => undefined)pattern allows the schema to accept requests containing onlyhostPersistentID(without full game config), which the Worker extracts separately fromreq.body.This addresses empty string concerns in the GameServer/Worker flow.
src/client/HostLobbyModal.ts (5)
29-29: LGTM: Import added for host authentication.The
getPersistentIDimport supports the new host-based authorization flow.
593-617: Error handling follows codebase patterns.The updated code:
- Correctly accesses
lobby.gameInfo.gameIDper the new return type- Displays user feedback on failure using the established popup pattern
- Follows precedent from UserSettingModal.ts for popup behavior (CSS animation handles disappearance)
Clean error flow.
813-813: Server-side validation handles edge cases.While
getPersistentID()could theoretically return an empty string for anonymous users, the server's UUID validation inWorkerSchemas.tswill reject invalid formats with a 400 error. This is acceptable—server-side validation provides the security boundary.Optional improvement: add client-side validation to provide earlier feedback, but not required.
862-888: Return type change is consistent with persistentID approach.The function now returns
{ gameInfo: GameInfo }instead ofGameInfodirectly. Since the flow switched from cookie-based tokens to usinggetPersistentID()(per the conversation with evanpelle), the client doesn't need the server to returnhostPersistentID—it reads it from the session when callingstart_game.The wrapping provides a clear API shape, though it adds nesting at the call site (line 595).
875-875: Consistent with start_game flow.Sending
hostPersistentIDduring lobby creation mirrors the pattern at line 813. Server-side validation ensures format correctness.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/client/HostLobbyModal.ts (1)
813-813: Validate persistentID before sending to prevent empty string bypass.
getPersistentID()can return an empty string (whengetPersistentIDFromCookie()returns empty). If the client sends an emptyhostPersistentID, and the server also stored empty/null during lobby creation, the authorization check at Worker.ts line 185 will pass, allowing anyone to start the game.Add validation before the fetch:
await this.putGameConfig(); console.log( `Starting private game with map: ${GameMapType[this.selectedMap as keyof typeof GameMapType]} ${this.useRandomMap ? " (Randomly selected)" : ""}`, ); this.close(); const config = await getServerConfigFromClient(); + + const hostPersistentID = getPersistentID(); + if (!hostPersistentID) { + console.error("Cannot start game: missing persistent ID"); + alert(translateText("errors.missing_persistent_id")); + return; + } const response = await fetch( `${window.location.origin}/${config.workerPath(this.lobbyId)}/api/start_game/${this.lobbyId}`, { method: "POST", headers: { "Content-Type": "application/json", }, - body: JSON.stringify({ hostPersistentID: getPersistentID() }), + body: JSON.stringify({ hostPersistentID }), }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/HostLobbyModal.ts(4 hunks)src/server/Worker.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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/Worker.tssrc/client/HostLobbyModal.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.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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.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/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: 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/client/HostLobbyModal.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/Main.ts (1)
getPersistentID(628-632)
🔇 Additional comments (1)
src/client/HostLobbyModal.ts (1)
610-617: Error handling looks good.The catch block properly logs the error, shows user feedback via popup, and closes the modal. This follows the established codebase pattern from UserSettingModal.ts.
Lavodan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
771-799: Type safety looks good; consider extracting for readability.The
satisfies Partial<GameConfig>clause ensures type safety for the game config portion while allowinghostPersistentIDin the outer object. The nested parentheses are required syntax.For slightly better readability, you could extract the game config:
private async putGameConfig() { const config = await getServerConfigFromClient(); + const gameConfig: Partial<GameConfig> = { + gameMap: this.selectedMap, + gameMapSize: this.compactMap ? GameMapSize.Compact : GameMapSize.Normal, + difficulty: this.selectedDifficulty, + bots: this.bots, + infiniteGold: this.infiniteGold, + donateGold: this.donateGold, + infiniteTroops: this.infiniteTroops, + donateTroops: this.donateTroops, + instantBuild: this.instantBuild, + randomSpawn: this.randomSpawn, + gameMode: this.gameMode, + disabledUnits: this.disabledUnits, + playerTeams: this.teamCount, + ...(this.gameMode === GameMode.Team && this.teamCount === HumansVsNations + ? { disableNPCs: false } + : { disableNPCs: this.disableNPCs }), + maxTimerValue: this.maxTimer === true ? this.maxTimerValue : undefined, + }; const response = await fetch( `${window.location.origin}/${config.workerPath(this.lobbyId)}/api/game/${this.lobbyId}`, { method: "PUT", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ hostPersistentID: getPersistentID(), - ...({ - gameMap: this.selectedMap, - gameMapSize: this.compactMap ? GameMapSize.Compact : GameMapSize.Normal, - difficulty: this.selectedDifficulty, - bots: this.bots, - infiniteGold: this.infiniteGold, - donateGold: this.donateGold, - infiniteTroops: this.infiniteTroops, - donateTroops: this.donateTroops, - instantBuild: this.instantBuild, - randomSpawn: this.randomSpawn, - gameMode: this.gameMode, - disabledUnits: this.disabledUnits, - playerTeams: this.teamCount, - ...(this.gameMode === GameMode.Team && this.teamCount === HumansVsNations - ? { disableNPCs: false } - : { disableNPCs: this.disableNPCs }), - maxTimerValue: this.maxTimer === true ? this.maxTimerValue : undefined, - } satisfies Partial<GameConfig>), + ...gameConfig, }), }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(5 hunks)src/server/GameServer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- src/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.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/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: 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/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/HostLobbyModal.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (3)
src/client/Utils.ts (1)
translateText(92-147)src/client/Main.ts (1)
getPersistentID(628-632)src/core/game/Game.ts (1)
HumansVsNations(56-56)
🔇 Additional comments (4)
src/client/HostLobbyModal.ts (4)
29-29: Clean import addition.The
getPersistentIDimport is necessary and well-placed alongside the existingJoinLobbyEventimport.
627-634: Good error handling for lobby creation.The catch block properly surfaces creation failures to the user with a localized message, following the established pattern from
UserSettingModal.ts.
839-839: Consistent persistentID usage.The
startGamerequest properly includeshostPersistentID, consistent with the validation approach used inputGameConfigandcreateLobby. Server-side UUID validation will handle empty or invalid IDs.
899-899: Lobby creation properly includes host validation.The
createLobbyfunction correctly sendshostPersistentIDto establish the host identity at creation time. This is consistent with the validation approach used throughout the file.
| maxTimerValue: | ||
| this.maxTimer === true ? this.maxTimerValue : undefined, | ||
| } satisfies Partial<GameConfig>), | ||
| hostPersistentID: getPersistentID(), |
There was a problem hiding this comment.
instead of the body, let's put it as x-persistent-id header.
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ hostPersistentID: getPersistentID() }), |
There was a problem hiding this comment.
same here, put in the header
| export const StartGameSchema = z.object({ | ||
| hostPersistentID: z.string().uuid().optional(), | ||
| }); |
There was a problem hiding this comment.
this can be removed now that we are using headers.
|
|
||
| export const GameInputSchema = GameConfigSchema.partial(); | ||
| export const GameInputSchema = | ||
| GameConfigSchema.partial().merge(StartGameSchema); |
There was a problem hiding this comment.
merging the persistent id in to the game config could make us accidently leak persistent id if we're not careful.
There was a problem hiding this comment.
Instead lets do
{
persistentID: ...
config: GameConfig
}
|
If someone else wants to take this I'm down |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/GameServer.ts (1)
133-142: Consider adding input validation for defense in depth.The
setHostPersistentIDmethod accepts any string without validation. While server-side Zod validation should prevent empty or malformed IDs at the API boundary, adding a defensive check here would catch programming errors earlier and make the intent clearer.Apply this diff to add validation:
public setHostPersistentID(id: string): void { if (this.getHostPersistentID()) { throw new Error("hostPersistentID is already set"); } + if (!id || !id.trim()) { + throw new Error("hostPersistentID cannot be empty"); + } this._hostPersistentID = id; }Based on learnings, server-side validation is the authoritative check, so this is optional defensive coding rather than a critical fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(5 hunks)src/server/GameServer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
📚 Learning: 2025-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/client/HostLobbyModal.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.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-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/HostLobbyModal.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/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: 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/client/HostLobbyModal.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (6)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)src/client/Main.ts (1)
getPersistentID(614-618)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/core/Schemas.ts (1)
GameConfig(84-84)src/core/WorkerSchemas.ts (1)
GameConfig(15-15)
🔇 Additional comments (6)
src/server/GameServer.ts (1)
35-35: Clean addition of the host persistent ID field.The nullable string field properly represents that a hostPersistentID may not be set initially. The initialization to
nullis explicit and clear.src/client/HostLobbyModal.ts (5)
30-30: Good import of the persistent ID helper.Importing
getPersistentIDfrom Main enables the persistent ID validation flow. The import is correctly placed with other Main imports.
614-621: Error handling follows codebase precedent.The
.catch()block properly logs the error, displays a user-facing popup usingsetting-popupclass (consistent with UserSettingModal.ts:221), and closes the modal. The popup disappears via CSS animation as designed.
758-786: Consider movinghostPersistentIDto request header.The implementation correctly includes
hostPersistentIDin the request body before spreading thePartial<GameConfig>. However, a past comment from evanpelle suggested using anx-persistent-idheader instead of the body for better separation of authorization data from game configuration.If the header approach is preferred, coordinate with server-side changes to parse the header. Otherwise, the current body approach works correctly.
Based on past review comments, this design choice may have been discussed elsewhere.
826-826: Persistent ID correctly included in start game request.The
hostPersistentIDis properly sent in the request body to authorize the game start. The same header vs. body consideration fromputGameConfigapplies here.
886-886: Persistent ID correctly included in lobby creation.The
hostPersistentIDis properly sent when creating the lobby, establishing the authorization token at creation time. The structure is clean and straightforward.
3 participants
Description:
Fixes #344.
Uses the host's persistentID upon lobby creation, saves it as a GameServer attribute. Afterwards, when the host starts the game, it sends its persistentID in the request's body. If the ID does not match, the lobby does not start.
The requests also aren't secure exclusive. I figured this isn't a very sensitive piece of information so it doesn't have to be protected too hard.
I couldn't figure out how to do cookies through Express (I think we lack a middleware, and it seems silly to get it for just a single purpose), so technically, the hostToken is sent twice in the request (in cookie and in body), but I feel like this is so minor it doesn't matter too much.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lavodan