Discord(et al.) embedded URLs #2740
Conversation
WalkthroughReplaces hash-based join/navigation with clean /game/{id} URLs; adds a server-side preview builder and GET /game/:id handler that injects social meta tags into HTML. Introduces GAME_ID_REGEX and validation helpers, updates client history handling, integrates HTML parsing, adjusts static asset headers, and updates nginx worker routing and caching. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant Nginx
participant Worker
participant PublicAPI
User->>Client: Open /game/{id} or click share link
Client->>Nginx: Request GET /game/{id}
Nginx->>Worker: Proxy /game/{id}
Worker->>Worker: Check local lobby cache
alt local lobby present
Worker->>Worker: buildPreview(gameID, origin, lobby, publicInfo?)
else not present
Worker->>PublicAPI: fetchRemoteLobbyInfo(gameID)
PublicAPI-->>Worker: remote lobby data
Worker->>PublicAPI: fetchPublicGameInfo(gameID)
PublicAPI-->>Worker: public info
Worker->>Worker: buildPreview(gameID, origin, null, publicInfo)
end
Worker->>Worker: Inject og/twitter meta tags into HTML
Worker-->>Nginx: HTML with meta tags
Nginx-->>Client: Respond with rendered HTML
Client->>Client: Show preview / allow join
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/JoinPrivateLobbyModal.ts (1)
139-161: Nice refactor using URL API for robust parsing.The structured URL parsing handles multiple URL shapes gracefully. The try-catch ensures malformed URLs don't crash the flow.
One small observation: the regex
/join\/([A-Za-z0-9]{8})/at line 153 would also match longer strings like/join/ABCD1234extra. Consider anchoring it with$or word boundary if you want stricter matching:- const match = url.pathname.match(/join\/([A-Za-z0-9]{8})/); + const match = url.pathname.match(/\/join\/([A-Za-z0-9]{8})$/);That said, since callers likely validate with
ID.safeParse()anyway, this is a minor nitpick.src/server/Master.ts (1)
173-179: Consider escaping URLs in HTML attributes.The
meta.redirectUrlandjoinIdare inserted into thecontentattribute and script without escaping. WhileredirectUrlis built fromorigin(which comes from request headers), a malformed host header could potentially break the HTML or introduce injection.Since
joinIdis already validated byID.safeParse(), it's safe. ForredirectUrl, you might want to validate or escape the origin:+const escapeAttr = (value: string): string => + value.replace(/"/g, """).replace(/</g, "<").replace(/>/g, ">"); const refreshTag = botRequest ? "" - : `<meta http-equiv="refresh" content="0; url=${meta.redirectUrl}#join=${joinId}">`; + : `<meta http-equiv="refresh" content="0; url=${escapeAttr(meta.redirectUrl)}#join=${joinId}">`;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/AccountModal.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/server/Master.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/AccountModal.tssrc/client/JoinPrivateLobbyModal.tssrc/server/Master.tssrc/client/Main.tssrc/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
🧬 Code graph analysis (2)
src/server/Master.ts (2)
src/core/Schemas.ts (2)
GameInfo(130-136)ID(208-211)src/core/configuration/DefaultConfig.ts (1)
workerPort(207-209)
src/client/Main.ts (1)
src/core/Schemas.ts (1)
ID(208-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (11)
src/client/HostLobbyModal.ts (1)
831-834: LGTM!The clipboard URL now uses the path-based format
/join/{lobbyId}, which is consistent with the PR's goal of transitioning to path-based routing. SincelobbyIdis an 8-character alphanumeric string (per theIDschema), no URL encoding is needed for the path segment.src/server/Master.ts (5)
52-57: Verify proxy trust configuration aligns with deployment.
requestOriginreadsx-forwarded-protodirectly from headers. Whileapp.set("trust proxy", 3)is configured at line 282, ensure your deployment actually has exactly 3 trusted proxy hops. Misconfiguration could allow clients to spoof the protocol/host.
126-127: Redirect URL uses query param format.The
redirectUrlis built as${origin}/?join=${gameID}, using query param style. The rest of the PR moves to path-based/join/{id}format. Is this intentional for backwards compatibility with the client'shandleUrlwhich checks query params first?If intentional, a brief comment explaining the choice would help future readers.
59-78: Good use of AbortController for fetch timeout.The 1.5 second timeout prevents the preview page from hanging if the worker is unresponsive. Clean pattern with proper cleanup via
clearTimeout.
220-245: Preview serves 404 status when lobby not found, but still returns HTML.Returning a 404 status with a preview page is unconventional. Bots might not index the page, but users still see a functional redirect. If that's the intent (graceful degradation), consider adding a comment. Otherwise, you might want to return a different response for non-existent lobbies.
247-252: Route handler looks good.The
/join/:gameIdroute delegates toserveJoinPreviewand properly catches errors with a 500 response. Clean error logging.src/client/AccountModal.ts (1)
145-152: LGTM!The navigation now uses path-based routing with
encodeURIComponentfor safety. The hash fragment is kept for the client-side join handler. DispatchinghashchangeensuresMain.tsprocesses the navigation.src/client/Main.ts (4)
612-634: Clean extraction logic with proper validation.The function checks multiple URL sources (query, path, hash) in a sensible priority order. Using
ID.safeParsefrom zod ensures the extracted value matches the expected 8-character alphanumeric format. Good defensive programming.
636-643: Good URL normalization helper.
updateJoinUrlForShareensures the URL is in the canonical format before sharing. UsingreplaceStateavoids polluting browser history with duplicate entries.
479-485: LGTM on the refactored join handling.The flow now extracts the join code, updates the URL to canonical format, opens the modal, and returns early. Clear and readable.
603-607: URL format now matches the canonical path-based pattern.The
pushStatecall uses/join/${gameID}#join=${gameID}, aligning with the rest of the PR. The hash fragment provides backwards compatibility for client-side routing.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/server/Master.ts (1)
284-289: Add rate limiting to prevent abuse.This route performs file system access but is registered before the rate limit middleware (lines 320-325), making it vulnerable to abuse. Move this route after the rate limiting setup.
🔎 Proposed fix
Move the route registration to after the rate limit middleware:
app.use(express.json()); - -app.get("/join/:gameId", (req, res) => { - serveJoinPreview(req, res, req.params.gameId).catch((error) => { - log.error("failed to render join preview", { error }); - res.status(500).send("Unable to render lobby preview"); - }); -}); - app.use( express.static(path.join(__dirname, "../../static"), { maxAge: "1y", // Set max-age to 1 year for all static assetsThen add after line 325:
max: 20, // 20 requests per IP per second }), ); + +app.get("/join/:gameId", (req, res) => { + serveJoinPreview(req, res, req.params.gameId).catch((error) => { + log.error("failed to render join preview", { error }); + res.status(500).send("Unable to render lobby preview"); + }); +});
🧹 Nitpick comments (1)
src/client/Main.ts (1)
608-623: Consider avoiding hardcoded length in the regex.The regex
/^\/join\/([A-Za-z0-9]{8})/hardcodes the 8-character length. If theIDschema's length changes in the future, this regex could become inconsistent. SinceID.safeParse()validates afterward, this provides a safety net, but the regex could become misleading.🔎 Option: Make the regex more flexible
private extractJoinCodeFromUrl(): string | null { const searchParams = new URLSearchParams(window.location.search); const joinFromQuery = searchParams.get("join"); if (joinFromQuery && ID.safeParse(joinFromQuery).success) { return joinFromQuery; } const pathMatch = window.location.pathname.match( - /^\/join\/([A-Za-z0-9]{8})/, + /^\/join\/([A-Za-z0-9]+)/, ); if (pathMatch && ID.safeParse(pathMatch[1]).success) { return pathMatch[1]; } return null; }This makes the regex more resilient to schema changes while still relying on
ID.safeParse()for validation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/AccountModal.tssrc/client/Main.tssrc/server/Master.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/AccountModal.tssrc/server/Master.tssrc/client/Main.ts
🧬 Code graph analysis (2)
src/server/Master.ts (2)
src/core/Schemas.ts (2)
GameInfo(130-136)ID(208-211)src/core/configuration/DefaultConfig.ts (1)
workerPort(207-209)
src/client/Main.ts (1)
src/core/Schemas.ts (1)
ID(208-211)
🪛 GitHub Check: 🔍 ESLint
src/server/Master.ts
[failure] 191-191:
Prefer using nullish coalescing operator (??) instead of a logical or (||), as it is a safer operator.
🪛 GitHub Check: CodeQL
src/server/Master.ts
[failure] 284-289: Missing rate limiting
This route handler performs a file system access, but is not rate-limited.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (11)
src/client/Main.ts (3)
479-485: LGTM! Clean URL extraction and early return.The flow correctly extracts the lobby ID, normalizes the URL for sharing, opens the modal, and returns early to prevent further processing. The logic is clear and handles both query parameter and path-based joins.
625-632: LGTM! Proper URL normalization without history pollution.The method correctly uses
replaceStateto normalize the URL to/join/{lobbyId}format without adding unnecessary entries to the browser history. The guard condition prevents redundant operations.
603-603: The server is already configured correctly for path-based URLs. The/join/:gameIdroute is defined atsrc/server/Master.tsline 284, and a catch-all SPA fallback route at line 532 servesindex.htmlfor all unmatched paths. This means direct navigation and page refresh to/join/*paths will work as expected.src/client/AccountModal.ts (1)
147-148: Clean URL construction looks good.The URL is properly constructed using
encodeURIComponentfor the path segment, creating a clean/join/{gameId}format that aligns with the PR's path-based URL standardization.src/server/Master.ts (7)
39-45: LGTM!The HTML escaping correctly handles the five key characters needed to prevent XSS in HTML contexts.
47-50: LGTM!Case-insensitive substring matching with proper null handling is the right approach for bot detection.
52-57: LGTM!The origin resolution correctly handles x-forwarded-proto with comma-separated values and provides sensible fallbacks for reverse proxy scenarios.
80-97: LGTM!The type definition appropriately uses optional fields for an external API response where the shape may vary.
117-123: LGTM!The type definition clearly captures the preview metadata structure.
199-249: LGTM!The HTML rendering correctly escapes user content, conditionally redirects humans vs bots, and follows Open Graph/Twitter Card specifications.
251-282: LGTM!The function correctly validates input, fetches data in parallel, and branches appropriately for bot vs human requests with proper cache headers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/AccountModal.tssrc/client/Main.tssrc/server/Master.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/AccountModal.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/Master.tssrc/client/Main.ts
🧬 Code graph analysis (2)
src/server/Master.ts (4)
src/core/configuration/ConfigLoader.ts (1)
getServerConfigFromServer(47-50)src/core/Schemas.ts (2)
GameInfo(130-136)ID(208-211)src/core/configuration/DefaultConfig.ts (1)
workerPort(207-209)tests/util/TestServerConfig.ts (1)
workerPort(67-69)
src/client/Main.ts (1)
src/core/Schemas.ts (1)
ID(208-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (19)
src/client/Main.ts (5)
379-379: LGTM!The custom event listener allows programmatic navigation to trigger the URL handler, consistent with the existing
popstateandhashchangelisteners.
480-486: LGTM!The join code extraction and URL normalization flow is clean, with an early return that correctly prevents further processing after handling the join link.
604-604: LGTM!The path-based URL is consistent with the new server route and the overall join link strategy.
609-624: LGTM!The extraction logic correctly prioritizes query parameters over path patterns and uses schema validation for both sources.
626-633: LGTM!The URL normalization correctly uses
replaceStateto avoid polluting history and includes a conditional check to prevent unnecessary updates.src/server/Master.ts (14)
47-58: LGTM!The bot list appropriately covers social media and messaging platform crawlers that consume Open Graph tags for link previews.
60-66: LGTM!The HTML escape utility correctly handles the five essential characters and processes
&first to prevent double-escaping.
68-71: LGTM!The bot detection logic is simple and effective, with case-insensitive matching and a safe default for missing user-agents.
73-78: LGTM!The origin extraction correctly handles proxy scenarios with
x-forwarded-protoand provides sensible fallbacks for both protocol and host.
80-100: LGTM!The lobby info fetch correctly uses
try/finallyto prevent timeout resource leaks and includes appropriate error handling.
102-119: LGTM!The type definition appropriately reflects the external API shape with optional fields for defensive parsing.
121-138: LGTM!The public game info fetch correctly uses
try/finallyto prevent timeout resource leaks and includes appropriate error handling.
140-146: LGTM!The preview metadata type is straightforward with clear, descriptive field names.
148-221: LGTM!The preview builder correctly prioritizes live lobby data over archived public data, uses nullish coalescing consistently, and handles all game states (finished, active, unknown).
223-273: LGTM!The HTML renderer correctly differentiates bot vs. human behavior, includes comprehensive Open Graph and Twitter Card tags, and uses HTML escaping to prevent XSS.
275-306: LGTM!The preview handler correctly validates IDs, fetches data in parallel, and differentiates between bot (static preview) and human (SPA) responses with appropriate cache headers.
308-313: LGTM!The route handler correctly applies rate limiting middleware and includes appropriate error handling with logging.
347-347: Verify the rate limit increase is intentional and appropriate.The global rate limit has been increased to 1000 requests per IP per second. Ensure this aligns with your system's capacity and DDoS protection strategy, as this is a significant threshold.
356-438: LGTM!The
startMasterfunction is now part of the public API. The implementation correctly handles worker lifecycle, scheduling coordination, and server startup.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/server/Worker.ts:
- Around line 91-116: fetchRemoteLobbyInfo currently casts the fetch response to
GameInfo without validation; parse the JSON from response (the await
response.json() call) into a local variable and validate it before casting using
a Zod schema (either create a new GameInfo Zod schema or reuse an existing
schema like ExternalGameInfoSchema/GameStartInfoSchema if it matches), and if
validation fails log a warning (including validation errors and gameID) and
return null instead of returning the raw object; keep the abort/timeout and
finally clearTimeout as-is and only return the parsed, validated value when
schema.safeParse succeeds.
🧹 Nitpick comments (2)
src/server/Worker.ts (2)
276-276: Add gameID validation at the start of the endpoint.The gameID parameter should be validated before using it to prevent invalid IDs from being processed.
Suggested validation
app.get("/game/:id", async (req, res) => { const gameID = req.params.id; + if (!gameIDSchema.safeParse(gameID).success) { + return res.redirect(302, "/"); + } const game = gm.game(gameID);
318-330: Consider caching the HTML template at startup.Since the base HTML template is static and only the meta tags change per request, you could parse it once during server initialization and clone/modify it for each request. This would eliminate the repeated file I/O and parsing overhead.
Example caching approach
At server startup:
let htmlTemplate: ReturnType<typeof parse> | null = null; async function loadHtmlTemplate() { const staticHtml = path.join(__dirname, "../../static/index.html"); const rootHtml = path.join(__dirname, "../../index.html"); // ... check existence and read file htmlTemplate = parse(html); }In the request handler:
if (htmlTemplate) { const root = htmlTemplate.clone(); // if library supports cloning // ... modify and return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/client/graphics/layers/WinModal.tssrc/core/Schemas.tssrc/server/Worker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/WinModal.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-08-14T10:07:44.588Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
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-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-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/Worker.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/server/Worker.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/server/Worker.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/server/Worker.ts
📚 Learning: 2025-06-13T20:57:49.599Z
Learnt from: El-Magico777
Repo: openfrontio/OpenFrontIO PR: 1166
File: src/core/Schemas.ts:50-56
Timestamp: 2025-06-13T20:57:49.599Z
Learning: In the OpenFrontIO codebase, player identification primarily uses numeric smallIDs throughout the system (e.g., playerBySmallID). When adding new intents or schemas, maintain consistency with this existing pattern rather than introducing mixed ID types, even if string IDs might be more standard elsewhere.
Applied to files:
src/server/Worker.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/Worker.ts
🧬 Code graph analysis (1)
src/server/Worker.ts (2)
src/core/Schemas.ts (2)
GAME_ID_REGEX(222-222)GameInfo(134-140)src/server/GamePreviewBuilder.ts (4)
ExternalGameInfo(40-56)ExternalGameInfoSchema(4-32)buildPreview(152-286)escapeHtml(143-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (5)
src/core/Schemas.ts (1)
221-227: LGTM! Good centralization of game ID validation.The extracted constant and helper function improve consistency across the codebase. The regex pattern correctly validates 8-character alphanumeric IDs, and the implementation is clean and reusable.
src/server/Worker.ts (4)
52-57: LGTM! Clean origin extraction with proper header handling.Correctly handles proxy headers and provides sensible fallbacks for protocol and host.
59-89: LGTM! Robust error handling and timeout management.The function correctly validates input, handles timeouts with AbortController, validates the response schema, and provides useful error logging.
177-182: LGTM! Appropriate static file serving with long-term caching.The 1-year cache is suitable for map assets that are unlikely to change, and
express.staticprovides built-in security for path handling.
7-7: No action required—node-html-parseris already declared in package.json dependencies (version ^7.0.2). The import at line 7 is valid.
| const fetchRemoteLobbyInfo = async ( | ||
| gameID: string, | ||
| ): Promise<GameInfo | null> => { | ||
| if (!gameIDSchema.safeParse(gameID).success) return null; | ||
|
|
||
| const targetWorkerIndex = config.workerIndex(gameID); | ||
| if (targetWorkerIndex === workerId) return null; | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = setTimeout(() => controller.abort(), 1500); | ||
| try { | ||
| const workerPort = config.workerPort(gameID); | ||
| const encodedID = encodeURIComponent(gameID); | ||
| const response = await fetch( | ||
| `http://127.0.0.1:${workerPort}/api/game/${encodedID}`, | ||
| { signal: controller.signal }, | ||
| ); | ||
| if (!response.ok) return null; | ||
| return (await response.json()) as GameInfo; | ||
| } catch (error) { | ||
| log.warn("failed to fetch remote lobby info", { gameID, error }); | ||
| return null; | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add response validation before casting to GameInfo.
Line 109 casts the response directly to GameInfo without validation. If the remote worker returns malformed or unexpected data, this could cause runtime errors downstream.
Suggested validation approach
Since GameInfo is defined as an interface (not a Zod schema), you have a few options:
- Create a Zod schema for GameInfo and validate the response
- Use the existing GameStartInfoSchema if it matches the expected structure
- Add basic structural checks before the cast
Example with a validation helper:
if (!response.ok) return null;
- return (await response.json()) as GameInfo;
+ const data = await response.json();
+ // Add validation here based on your approach
+ if (!data || typeof data !== 'object') return null;
+ return data as GameInfo;
} catch (error) {The safest approach would be to define a Zod schema for GameInfo similar to how ExternalGameInfoSchema is used in fetchPublicGameInfo.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchRemoteLobbyInfo = async ( | |
| gameID: string, | |
| ): Promise<GameInfo | null> => { | |
| if (!gameIDSchema.safeParse(gameID).success) return null; | |
| const targetWorkerIndex = config.workerIndex(gameID); | |
| if (targetWorkerIndex === workerId) return null; | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 1500); | |
| try { | |
| const workerPort = config.workerPort(gameID); | |
| const encodedID = encodeURIComponent(gameID); | |
| const response = await fetch( | |
| `http://127.0.0.1:${workerPort}/api/game/${encodedID}`, | |
| { signal: controller.signal }, | |
| ); | |
| if (!response.ok) return null; | |
| return (await response.json()) as GameInfo; | |
| } catch (error) { | |
| log.warn("failed to fetch remote lobby info", { gameID, error }); | |
| return null; | |
| } finally { | |
| clearTimeout(timeout); | |
| } | |
| }; | |
| const fetchRemoteLobbyInfo = async ( | |
| gameID: string, | |
| ): Promise<GameInfo | null> => { | |
| if (!gameIDSchema.safeParse(gameID).success) return null; | |
| const targetWorkerIndex = config.workerIndex(gameID); | |
| if (targetWorkerIndex === workerId) return null; | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 1500); | |
| try { | |
| const workerPort = config.workerPort(gameID); | |
| const encodedID = encodeURIComponent(gameID); | |
| const response = await fetch( | |
| `http://127.0.0.1:${workerPort}/api/game/${encodedID}`, | |
| { signal: controller.signal }, | |
| ); | |
| if (!response.ok) return null; | |
| const data = await response.json(); | |
| // Add validation here based on your approach | |
| if (!data || typeof data !== 'object') return null; | |
| return data as GameInfo; | |
| } catch (error) { | |
| log.warn("failed to fetch remote lobby info", { gameID, error }); | |
| return null; | |
| } finally { | |
| clearTimeout(timeout); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In @src/server/Worker.ts around lines 91 - 116, fetchRemoteLobbyInfo currently
casts the fetch response to GameInfo without validation; parse the JSON from
response (the await response.json() call) into a local variable and validate it
before casting using a Zod schema (either create a new GameInfo Zod schema or
reuse an existing schema like ExternalGameInfoSchema/GameStartInfoSchema if it
matches), and if validation fails log a warning (including validation errors and
gameID) and return null instead of returning the raw object; keep the
abort/timeout and finally clearTimeout as-is and only return the parsed,
validated value when schema.safeParse succeeds.
| if (fs.existsSync(staticHtml)) { | ||
| filePath = staticHtml; | ||
| } else if (fs.existsSync(rootHtml)) { | ||
| filePath = rootHtml; | ||
| } | ||
|
|
||
| if (filePath) { | ||
| const html = fs.readFileSync(filePath, "utf-8"); | ||
| const root = parse(html); | ||
| const head = root.querySelector("head"); | ||
| if (head) { | ||
| head | ||
| .querySelectorAll('meta[property^="og:"], meta[name^="twitter:"]') | ||
| .forEach((el) => el.remove()); | ||
| // Inject new meta tags | ||
| const tagsToInject = [ | ||
| `<meta property="og:title" content="${escapeHtml(meta.title)}" />`, | ||
| `<meta property="og:description" content="${escapeHtml(meta.description || meta.title)}" />`, | ||
| `<meta property="og:url" content="${escapeHtml(meta.joinUrl)}" />`, | ||
| `<meta property="og:image" content="${escapeHtml(meta.image)}" />`, | ||
| `<meta name="twitter:card" content="summary_large_image" />`, | ||
| `<meta name="twitter:title" content="${escapeHtml(meta.title)}" />`, | ||
| `<meta name="twitter:description" content="${escapeHtml(meta.description || meta.title)}" />`, | ||
| `<meta name="twitter:image" content="${escapeHtml(meta.image)}" />`, | ||
| ]; | ||
| tagsToInject.forEach((tag) => | ||
| head.insertAdjacentHTML("beforeend", tag), | ||
| ); | ||
| } | ||
| return res.status(200).type("html").send(root.toString()); |
There was a problem hiding this comment.
Replace synchronous file I/O with async operations.
Lines 303-310 use fs.existsSync and fs.readFileSync, which block the event loop and can degrade server performance under load. This is especially problematic in a request handler that may be called frequently.
Recommended async refactor
- if (fs.existsSync(staticHtml)) {
- filePath = staticHtml;
- } else if (fs.existsSync(rootHtml)) {
- filePath = rootHtml;
- }
+ const staticExists = await fs.promises.access(staticHtml, fs.constants.F_OK).then(() => true).catch(() => false);
+ const rootExists = await fs.promises.access(rootHtml, fs.constants.F_OK).then(() => true).catch(() => false);
+
+ if (staticExists) {
+ filePath = staticHtml;
+ } else if (rootExists) {
+ filePath = rootHtml;
+ }
if (filePath) {
- const html = fs.readFileSync(filePath, "utf-8");
+ const html = await fs.promises.readFile(filePath, "utf-8");
const root = parse(html);Alternatively, consider caching the parsed HTML template at startup since it's static content.
Committable suggestion skipped: line range outside the PR's diff.
|
looks like it has merge conflicts from the new ui update |
Yeah 😭 will fix today |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/client/styles/components/setting.css (1)
265-267: Don’t remove focus outline without adding a replacement focus style.Keyboard users will lose focus visibility on sliders.
Proposed focus-visible styling
.setting-input.slider:focus { outline: none; } + +.setting-input.slider:focus-visible { + box-shadow: 0 0 0 2px rgba(59, 130, 246, 0.9); +}src/client/styles/components/modal.css (1)
7-13: Remove redundant modal button styling rule.The
o-modal o-buttondescendant selector duplicates styling already defined in button.css (both applydisplay: blockandmargin: 0 autoat the1024pxbreakpoint). Since o-button elements inside o-modal inherit button.css styles, this global rule is redundant.Delete lines 7-13 entirely, or verify in component tests that button centering still works as expected without it.
src/core/execution/TransportShipExecution.ts (1)
257-302: AvoiddstShore!assertions in tick; make the invariant explicit with a guard.
Today it “should be set”, but a future change could makedstShorenull (or unset whendstis set) and this will crash hard in the hottest path.Proposed guard
tick(ticks: number) { if (this.dst === null) { this.active = false; return; } + if (this.dstShore === null) { + this.active = false; + return; + } if (!this.active) { return; }src/client/GameStartingModal.ts (1)
1-12: Extend GameStartingModal from BaseModal to align with modal patterns in this codebase.All other modals extend BaseModal, which provides Escape key handling and consistent modal lifecycle management. GameStartingModal uses
isVisibleand manualshow()/hide()methods instead. Extend BaseModal to gain automatic Escape key handling, listener cleanup, and consistent naming (isModalOpen).src/client/components/baseComponents/setting/SettingToggle.ts (1)
16-26: Fix duplicate"change"events (native input + custom event share the same name).Right now the input’s native
changewill bubble, andhandleChange()emits another bubbling"change"—listeners can see two events and/or the “wrong” event shape.Proposed fix (keep public event name, suppress native bubble)
private handleChange(e: Event) { - const input = e.target as HTMLInputElement; + // Prevent the native input "change" event from bubbling out and causing duplicates. + e.stopPropagation(); + const input = e.currentTarget as HTMLInputElement; this.checked = input.checked; this.dispatchEvent( new CustomEvent("change", { detail: { checked: this.checked }, bubbles: true, composed: true, }), ); }Optional: add a visible keyboard focus state (e.g.,
peer-focus-visible:ring-*on the<span>) since the checkbox is visually hidden.Also applies to: 28-61
tests/client/components/FluentSlider.test.ts (1)
55-79: Avoid null-deref in tests: assert the range input exists everywhere you use it.Several tests do
const rangeInput = slider.querySelector(...) as HTMLInputElement;and then immediately writerangeInput.valueAsNumber = ...without a guard—if the selector breaks, you’ll get a runtime error instead of a clean test failure.Example pattern to apply to each test
- const rangeInput = slider.querySelector( + const rangeInput = slider.querySelector( 'input[type="range"]', ) as HTMLInputElement; + expect(rangeInput).toBeTruthy();Also applies to: 83-124, 126-176, 222-248, 251-279
src/client/components/baseComponents/setting/SettingNumber.ts (1)
13-15: Shadow DOM disabled - potential ID conflicts ahead.Returning
thisfromcreateRenderRoot()disables shadow DOM encapsulation. This means multiple<setting-number>instances on the same page will have duplicateid="setting-number-input"(lines 43, 52), which breaks HTML semantics and accessibility.🔧 Use dynamic IDs or re-enable shadow DOM
Option 1: Generate unique IDs per instance
+ private inputId = `setting-number-input-${Math.random().toString(36).substr(2, 9)}`; + render() { const rainbowClass = this.easter ? "bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)] bg-[length:1400%_1400%] animate-rainbow-bg text-white hover:bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)]" : ""; return html` <div class="flex flex-row items-center justify-between w-full p-4 bg-white/5 border border-white/10 rounded-xl hover:bg-white/10 transition-all gap-4 ${rainbowClass}" > <div class="flex flex-col flex-1 min-w-0 mr-4"> <label class="text-white font-bold text-base block mb-1" - for="setting-number-input" + for=${this.inputId} >${this.label}</label > <div class="text-white/50 text-sm leading-snug"> ${this.description} </div> </div> <input type="number" - id="setting-number-input" + id=${this.inputId}Option 2: Remove
createRenderRoot()to restore shadow DOM (preferred if no global styles are needed)- createRenderRoot() { - return this; - }src/client/HostLobbyModal.ts (1)
1181-1191: Critical: Second unresolved merge conflict.Another merge conflict marker at line 1181-1182. This must also be resolved.
🔧 Suggested resolution
private async copyToClipboard() { -<<<<<<< HEAD try { await navigator.clipboard.writeText(this.buildLobbyUrl()); this.copySuccess = true; setTimeout(() => { this.copySuccess = false; }, 2000); } catch (err) { console.error(`Failed to copy text: ${err}`); } }Note: Remove the
<<<<<<< HEADmarker and keep the implementation. Check if there's a matching=======and>>>>>>>marker further down that was cut off.
🤖 Fix all issues with AI agents
In @src/client/GameStartingModal.ts:
- Around line 17-43: Add proper accessibility attributes and focus/keyboard
handling to the modal elements and its show/hide logic: give the visible modal
container a role="dialog" (or "alertdialog"), aria-modal="true", and either
aria-labelledby pointing to the title element or an aria-label; mark the
backdrop as aria-hidden when visible; update the GameStartingModal.show() and
GameStartingModal.hide() (or equivalent) to move focus into the modal (focus the
primary actionable element or the title), restore focus on close, implement a
focus trap to keep tab navigation inside the modal while isVisible is true, and
add Escape key handling to call hide(); ensure all referenced IDs (e.g., title
element ID used by aria-labelledby) and focus management calls match the modal
DOM nodes in GameStartingModal.ts.
In @src/client/graphics/layers/HeadsUpMessage.ts:
- Line 65: The ternary in HeadsUpMessage that sets the timeout uses a redundant
nullish coalescing: change the expression in the ternary from checking (duration
?? 2000) to just duration when typeof duration === "number"; specifically update
the expression in the code handling duration (the ternary that currently reads
typeof duration === "number" ? (duration ?? 2000) : 2000) to typeof duration ===
"number" ? duration : 2000 so the fallback 2000 remains for non-number values
but the unnecessary ?? is removed.
In @src/client/HostLobbyModal.ts:
- Around line 78-101: In renderOptionToggle, the template opens a <button> but
never closes it; update the TemplateResult returned by renderOptionToggle to
include the missing closing </button> tag (after the inner <div>), ensuring the
HTML template string is properly closed and the function still returns a valid
TemplateResult.
- Around line 934-964: Remove the Git conflict markers and merge both changes:
in HostLobbyModal (the modal close/reset logic), replace the conflict region
with a single block that first calls history.replaceState(null, "",
window.location.origin + "/"); and then resets all transient form state fields
(selectedMap, selectedDifficulty, disableNations, gameMode, teamCount, bots,
spawnImmunity, spawnImmunityDurationMinutes, infiniteGold, donateGold,
infiniteTroops, donateTroops, maxTimer, maxTimerValue, instantBuild,
randomSpawn, compactMap, useRandomMap, disabledUnits, lobbyId, copySuccess,
clients, lobbyIdVisible, nationCount) ensuring the method braces remain correct
and no conflict markers remain.
In @src/client/JoinPrivateLobbyModal.ts:
- Around line 352-367: Remove the unresolved git conflict markers and keep the
BaseModal approach in JoinPrivateLobbyModal.open(): replace the conflicting
block with a call to super.open(), initialize lobbyIdVisible from
userSettings.get("settings.lobbyIdVisibility", true), and if an id argument is
present call setLobbyId(id) and joinLobby(); remove the modalEl?.open() and
modalEl.onClose assignment from the old code path so no conflict markers remain
and the method uses the BaseModal behavior.
- Around line 395-401: In copyToClipboard
(JoinPrivateLobbyModal.copyToClipboard) replace the hash-style URL string
`${location.origin}/#join=${this.currentLobbyId}` with the path-based format
used elsewhere, e.g.
`${location.origin}/game/${encodeURIComponent(this.currentLobbyId)}` (or the
app's canonical /game/{id} path), so the copied URL matches the PR's migration;
keep the same success/failure callbacks when calling the copyToClipboard helper.
In @src/client/styles/components/setting.css:
- Around line 1-8: Reduce the grid item minimum and center the grid to avoid
horizontal overflow and ragged alignment: change the grid-template-columns in
.settings-list from repeat(auto-fit, minmax(360px, 1fr)) to a safer min (e.g.,
repeat(auto-fit, minmax(240px, 1fr))) and add centering rules (e.g.,
justify-content: center; and/or justify-items: center) so cards don’t force
horizontal scroll in narrow containers and remain centered on wide screens; keep
existing gap/padding rules and preserve card max-width elsewhere.
In @src/client/Utils.ts:
- Around line 87-97: The Digit regex only matches single digits and the simple
fallback capitalization produces "Arrowup" instead of "ArrowUp"; update the
Digit pattern from /^Digit\d$/ to accept one-or-more digits (e.g., /^Digit\d+$/)
so multi-digit codes are handled, keep or adjust the Key pattern (/^Key[A-Z]$/)
as needed, and replace the naive fallback (the line using
value.charAt(0).toUpperCase() + value.slice(1)) with a routine that preserves
camelCase by splitting on camel-word boundaries and capitalizing each segment’s
first letter (so "arrowup" → "ArrowUp" and existing "ArrowUp" stays correct).
Ensure you change the two regex literals (/^Digit\d$/ and the fallback
capitalization logic) in Utils.ts where these transformations occur.
In @src/client/vite-env.d.ts:
- Around line 13-17: The module declarations for "*.md?url" and "*.txt?raw" are
unsupported unless those extensions are added to Vite's assetsInclude; either
remove the "*.md?url" and "*.txt?raw" declarations from vite-env.d.ts, or add
the file globs to the vite config by updating the assetsInclude setting (e.g.,
include "**/*.md" and "**/*.txt"); keep "*.svg?url" as-is since .svg is an asset
by default.
In @src/core/validations/username.ts:
- Line 83: The comment examples are inconsistent with the actual behavior of the
username restoration which always uppercases the clan tag and inserts a space
(see the return expression `[${clanTag.toUpperCase()}]
${censoredNameWithoutClan}` in src/core/validations/username.ts); update the
example on line 61 to show the space after the bracket so it reads `"[CLAN]
GoodName" -> "[CLAN] GoodName"` (so examples match the logic that uppercases
clan tags and adds a space).
In @src/server/Worker.ts:
- Around line 275-277: Validate the route parameter gameID at the entry of
app.get("/game/:id") by checking req.params.id against GAME_ID_REGEX; if it
fails validation, respond immediately with a 400 (Bad Request) and an
explanatory message and do not call gm.game or log the invalid value; only
proceed to call gm.game(gameID) and any logging after the regex check passes.
- Around line 176-182: The app.use(express.static(path.join(__dirname,
"../../static"))) call references a non-existent static directory and will fail
to serve files; either create the repository root "static" folder and add the
required assets, or change the static middleware to point to an existing
directory (for example replace "../../static" with an existing path like
"../../resources" or another build output) so app.use(express.static(...)) in
Worker.ts and its surrounding static-serving setup uses a real folder; keep the
existing "/maps" route unchanged as it correctly points to
"../../resources/maps".
🧹 Nitpick comments (40)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
492-509: Consider aligning unit info opacity with the rest of the PR.You removed opacity styling from the health line, but
unit.type()still usesopacity-80(Line 498). If the intent is “no opacity in this overlay”, consider removing that too for consistency.src/client/ClientGameRunner.ts (1)
124-143: Good addition of error handling for game creation failures.The catch block appropriately logs errors, hides the loading modal, and displays error details to the user. The direct DOM manipulation to hide the modal follows the codebase pattern.
However, consider typing the error parameter to ensure
messageandstackproperties exist:♻️ Type the error parameter
) - .then((r) => r.start()) - .catch((e) => { - console.error("error creating client game", e); + .then((r) => r.start()) + .catch((e: unknown) => { + const error = e instanceof Error ? e : new Error(String(e)); + console.error("error creating client game", error); const startingModal = document.querySelector( "game-starting-modal", ) as HTMLElement; if (startingModal) { startingModal.classList.add("hidden"); } showErrorModal( - e.message, - e.stack, + error.message, + error.stack, lobbyConfig.gameID, lobbyConfig.clientID, true, false, "error_modal.connection_error", ); });src/client/utilities/RenderUnitTypeOptions.ts (1)
23-46: Consider clarifying thetoggleUnitAPI for better readability.The function works correctly, but the
toggleUnitinterface could be more intuitive. Currently, it's named "toggle" but requires passing the current state (isEnabled), meaning the handler must flip the boolean internally. This indirection adds cognitive load.Two clearer alternatives:
- State-less toggle:
toggleUnit(type: UnitType) => void— handler reads current state internally- Explicit setter: Rename to
setUnitEnabled(type: UnitType, enabled: boolean) => void— makes the set operation clearEither approach would make the intent more obvious at the call site.
♻️ Example refactor (state-less toggle)
Update the interface:
export interface UnitTypeRenderContext { disabledUnits: UnitType[]; - toggleUnit: (unit: UnitType, checked: boolean) => void; + toggleUnit: (unit: UnitType) => void; }Update the call site:
- @click=${() => toggleUnit(type, isEnabled)} + @click=${() => toggleUnit(type)}The handler would then check
disabledUnits.includes(type)internally to determine the new state.src/client/components/baseComponents/stats/DiscordUserHeader.ts (1)
43-56: Consider using Tailwind's spacing scale instead of arbitrary padding.Line 46 uses
p-[3px], which is outside Tailwind's standard spacing scale (0.5rem increments). Unless 3px is a strict design requirement, usep-0.5(2px) orp-1(4px) for consistency with the design system.♻️ Proposed refactor using spacing scale
- <div class="p-[3px] rounded-full bg-gray-500"> + <div class="p-1 rounded-full bg-gray-500">src/client/graphics/ui/NavalTarget.ts (2)
108-131: Optional: Consider composition over inheritance.
NavalTargetextendsTargetin a classical inheritance pattern. While this works, composition with typed unions can offer clearer contracts and easier testing. For example, a union type like{ kind: 'static', ... } | { kind: 'naval', unit: UnitView, ... }with a shared render function would avoid the base-class pattern.This is existing architecture and outside the scope of the current change—just noting for future refactors.
21-21: Remove deadlifeTimefield.The
lifeTimefield is incremented on line 35 but never used. The fade animation usesanimationElapsedTimeinstead. This field can be safely removed from the Target class to clean up the code.src/client/styles/components/setting.css (1)
220-240: Firefox slider track heights should match WebKit for visual consistency.The
--fillCSS variable is properly updated bySettingSlider.tson input changes (line 34:slider.style.setProperty("--fill", "${clamped}%")), so the gradient works correctly in WebKit. However, the Firefox pseudo-elements have different heights than the WebKit track, creating a visual mismatch.Consistency fix for Firefox track heights
.setting-input.slider::-moz-range-track { background-color: #444; - height: 10px; - border-radius: 5px; + height: 12px; + border-radius: 6px; } .setting-input.slider::-moz-range-progress { background-color: #2196f3; - height: 10px; - border-radius: 5px; + height: 12px; + border-radius: 6px; }Also applies to: 253-263
src/client/graphics/layers/HeadsUpMessage.ts (3)
19-20: Consider importingTemplateResultat the top.The inline
import("lit").TemplateResulttype reference works but is inconsistent with other imports. For cleaner code, importTemplateResultalongside other lit imports on line 1.📦 Proposed refactor
-import { LitElement, html } from "lit"; +import { LitElement, html, type TemplateResult } from "lit";Then update line 20:
- private toastMessage: string | import("lit").TemplateResult | null = null; + private toastMessage: string | TemplateResult | null = null;
48-68: Consider defining an interface for the event detail.The event handling relies on runtime type checks for
messageand loosely typedevent.detail. Defining a typed interface would improve maintainability and catch errors earlier.💡 Example interface
interface ShowMessageEventDetail { message: string | TemplateResult; duration?: number; color?: "green" | "red"; } // Then in the handler: private handleShowMessage = (event: CustomEvent<ShowMessageEventDetail>) => { const { message, duration, color } = event.detail ?? {}; // Type-safe access to properties this.toastMessage = message; this.toastColor = color === "red" ? "red" : "green"; // ... };
107-117: Extract repeated color logic to reduce duplication.The color conditionals for background, border, and box-shadow are repeated 5 times in the inline styles. Extracting this to a computed property or helper function would make the template cleaner and easier to maintain.
♻️ Example refactor
Add a method:
private getToastStyles() { const isRed = this.toastColor === "red"; const colorRgb = isRed ? "239,68,68" : "34,197,94"; return { background: `rgba(${colorRgb},0.1)`, border: `1px solid rgba(${colorRgb},0.5)`, boxShadow: `0 0 30px 0 rgba(${colorRgb},0.3)`, }; }Then in the template:
style="max-width: 90vw; min-width: 200px; text-align: center; background: ${styles.background}; border: ${styles.border}; color: white; box-shadow: ${styles.boxShadow}; backdrop-filter: blur(12px);"src/client/styles/components/modal.css (1)
1-4: Clear deprecation notice, but consider adding a removal timeline.The comment clearly explains the architectural shift to component-scoped styles. However, it would help future maintainers to include a target date or milestone for removing this file entirely.
📝 Optional: Add removal timeline
/* Deprecated global modal styles. The component-scoped styles in src/client/components/baseComponents/Modal.ts are the single source of truth now. Removing global overrides so the - component can control layout and internal scrolling behavior. */ + component can control layout and internal scrolling behavior. + TODO: Remove this entire file after all legacy modals are migrated (target: vX.Y.Z). */src/core/execution/TransportShipExecution.ts (3)
151-171: Spawn-to-water move is correct, but thedstShore !== nullbranch is dead code.
At this point init would have returned ifdstShorewas null; keeping the branch adds noise and can hide future regressions.Proposed tidy-up
- if (this.dstShore !== null) { - this.boat.setTargetTile(this.dstShore); - } else { - this.boat.setTargetTile(undefined); - } + this.boat.setTargetTile(this.dstShore);
218-255: Retreat: storingdstShore = srcis a clean invariant.
Also good: you recomputedstas adjacent water to keep the water-pathfinder consistent. One small safety win: avoidthis.dstShore!insetTargetTile()since you’ve just assigned it.
325-336:adjacentWater()is simple and readable; good helper extraction.
Minor thought: ifmg.neighbors()order is not stable, you may get non-deterministic “first water neighbor” selection (only matters if multiple water neighbors behave differently).src/client/GameStartingModal.ts (2)
15-15: Remove redundant local variable.The
const isVisible = this.isVisible;declaration is unnecessary. You can referencethis.isVisibledirectly in the template since Lit'shtmltagged template handles property access correctly.♻️ Proposed refactor
render() { - const isVisible = this.isVisible; return html` <div - class="fixed inset-0 bg-black/30 backdrop-blur-[4px] z-[9998] transition-all duration-300 ${isVisible + class="fixed inset-0 bg-black/30 backdrop-blur-[4px] z-[9998] transition-all duration-300 ${this.isVisible ? "opacity-100 visible" : "opacity-0 invisible"}" ></div> <div - class="fixed top-1/2 left-1/2 bg-zinc-800/70 p-6 rounded-xl z-[9999] shadow-[0_0_20px_rgba(0,0,0,0.5)] backdrop-blur-[5px] text-white w-[300px] text-center transition-all duration-300 -translate-x-1/2 ${isVisible + class="fixed top-1/2 left-1/2 bg-zinc-800/70 p-6 rounded-xl z-[9999] shadow-[0_0_20px_rgba(0,0,0,0.5)] backdrop-blur-[5px] text-white w-[300px] text-center transition-all duration-300 -translate-x-1/2 ${this.isVisible ? "opacity-100 visible -translate-y-1/2" : "opacity-0 invisible -translate-y-[48%]"}" >
47-55: ManualrequestUpdate()calls may be unnecessary.Since
isVisibleis decorated with@state(), LitElement should automatically trigger a re-render when it changes. The manualrequestUpdate()calls inshow()andhide()are likely redundant unless there's a specific timing requirement.You can simplify to:
♻️ Simplified show/hide methods
show() { this.isVisible = true; - this.requestUpdate(); } hide() { this.isVisible = false; - this.requestUpdate(); }If manual updates are required for timing reasons specific to game-start transitions, consider adding a comment explaining why.
src/server/Worker.ts (3)
52-57: Consider dev environment handling in protocol fallbackThe hardcoded
"https"fallback may cause issues in local development wherehttpis typically used. Consider checkingconfig.env()to provide appropriate defaults for dev vs production.💡 Optional enhancement for dev environment support
const requestOrigin = (req: Request): string => { const protoHeader = (req.headers["x-forwarded-proto"] as string) ?? ""; - const proto = protoHeader.split(",")[0]?.trim() || req.protocol || "https"; + const proto = protoHeader.split(",")[0]?.trim() || req.protocol || (config.env() === GameEnv.Dev ? "http" : "https"); const host = req.get("host") ?? `${config.subdomain()}.${config.domain()}`; return `${proto}://${host}`; };
299-311: Synchronous file operations may block under load
fs.existsSync()andfs.readFileSync()are synchronous and will block the event loop on each HTML request. For high-traffic scenarios, consider:
- Reading the HTML template once at startup and storing in memory
- Using async
fs.promises.readFile()- Caching the parsed HTML structure
However, since meta tags differ per game, you'd need to cache the template and inject tags dynamically.
💡 Example: Cache HTML template at startup
At the top of
startWorker():// Read HTML template once at startup let htmlTemplate: string | null = null; const staticHtml = path.join(__dirname, "../../static/index.html"); const rootHtml = path.join(__dirname, "../../index.html"); try { if (fs.existsSync(staticHtml)) { htmlTemplate = fs.readFileSync(staticHtml, "utf-8"); } else if (fs.existsSync(rootHtml)) { htmlTemplate = fs.readFileSync(rootHtml, "utf-8"); } } catch (error) { log.error("Failed to load HTML template", error); }Then in the route:
- if (filePath) { - const html = fs.readFileSync(filePath, "utf-8"); + if (htmlTemplate) { + const html = htmlTemplate; const root = parse(html);
290-292: Consider 404 response instead of redirect for missing gamesRedirecting to
/when no game info is found (line 291) loses the game ID context. A 404 status with a user-friendly message might provide better UX and allow proper error tracking.🎯 Alternative: Return 404 with message
if (!lobby && !publicInfo) { - return res.redirect(302, "/"); + return res.status(404).send("Game not found"); }src/client/components/baseComponents/Button.ts (1)
28-31: Simplify responsive class logic and verify fill+block interaction.The condition on lines 29-30 could be simplified using a single negative check. Also verify that
fillandblockwon't both be true simultaneously, as they apply conflicting width/height classes (h-full w-fullvsw-full block).♻️ Simplified logic
"h-full w-full flex items-center justify-center": this.fill, - "lg:w-auto lg:inline-block": - !this.block && !this.blockDesktop && !this.fill, + "lg:w-auto lg:inline-block": !this.block && !this.blockDesktop && !this.fill, "lg:w-1/2 lg:mx-auto lg:block": this.blockDesktop,Consider documenting that
fillshould not be used withblockorblockDesktop, or add runtime validation if they're mutually exclusive.src/client/styles.css (1)
42-45: Fallback pattern looks good, but consider adding a comment for clarity.The duplicate
heightproperty (line 43 as fallback, line 44 with100dvh) is a valid progressive enhancement pattern for browsers withoutdvhsupport. Static analysis flags this as a duplicate, but it is intentional.Consider adding a brief comment to make the intent clear and silence future warnings:
html { - height: 100%; /* Fallback */ + height: 100%; /* Fallback for browsers without dvh support */ height: 100dvh; }src/client/UsernameInput.ts (1)
134-162: Edge case: emptybaseUsernamecheck usestrim()but the stored value uses untrimmed input.At line 136, you check
!this.baseUsername.trim(), butthis.baseUsernameitself is assigned fromvalat line 130 which is not trimmed. The final stored username at line 157 usestrimmedFull. Consider whether leading/trailing spaces inbaseUsernameshould be allowed or stripped on input.This is a minor UX consideration - users might accidentally add spaces that remain in the input field but get trimmed when stored.
src/server/MapPlaylist.ts (1)
24-66: Sanity-checkGameMapTypekey iteration + consider avoidingPartialhere.Adding
DidierFrance: 2is fine, but the currentPartial<Record<GameMapName, number>>+Object.keys(GameMapType)pattern can hide mistakes (missing keys become 0, and enum key iteration can be tricky ifGameMapTypeis a numeric enum). Consider tightening this so missing/invalid map keys fail loudly (even if you keep a separate “default 0” fallback).tests/client/components/FluentSlider.test.ts (1)
281-294: Edge-case coverage is good; consider making the “0%” style assertion less brittle.The “no NaN” assertion is great. The
"0%"check may vary if the style string format changes; if this becomes flaky, consider matching the specific CSS var/property you set (regex) rather than any"0%"substring.src/client/Layout.ts (1)
7-7: Extract magic number 768 to a named constant.The breakpoint
768appears three times. Extract it to a constant likeMOBILE_BREAKPOINTfor maintainability.♻️ Extract breakpoint constant
+const MOBILE_BREAKPOINT = 768; + export function initLayout() { const hb = document.getElementById("hamburger-btn"); const sidebar = document.getElementById("sidebar-menu"); const backdrop = document.getElementById("mobile-menu-backdrop"); // Force sidebar visibility style to ensure it's not hidden by other CSS - if (sidebar && window.innerWidth < 768) { + if (sidebar && window.innerWidth < MOBILE_BREAKPOINT) { sidebar.style.display = "flex"; }And apply the same constant on lines 60 and 76:
- if (window.innerWidth >= 768) return; + if (window.innerWidth >= MOBILE_BREAKPOINT) return;Also applies to: 60-60, 76-76
src/client/Navigation.ts (1)
14-20: Redundant direct manipulation ofpage-play?Line 19 directly adds the
hiddenclass topage-playafter already iterating over all.page-contentelements (lines 15-18). Ifpage-playhas the.page-contentclass, this is redundant.Verify whether
page-playis already included in.page-contentand remove line 19 if redundant:#!/bin/bash # Check if page-play element has page-content class in HTML templates rg -i 'id="page-play"' -A2 -B2src/client/components/baseComponents/setting/SettingNumber.ts (1)
32-34: Long inline class string hurts readability.The rainbow gradient class is 255+ characters. Extract it to a constant at the module level for easier maintenance.
♻️ Extract to constant
+const RAINBOW_GRADIENT_CLASS = + "bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)] " + + "bg-[length:1400%_1400%] animate-rainbow-bg text-white " + + "hover:bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)]"; + @customElement("setting-number") export class SettingNumber extends LitElement { // ... render() { - const rainbowClass = this.easter - ? "bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)] bg-[length:1400%_1400%] animate-rainbow-bg text-white hover:bg-[linear-gradient(270deg,#990033,#996600,#336600,#008080,#1c3f99,#5e0099,#990033)]" - : ""; + const rainbowClass = this.easter ? RAINBOW_GRADIENT_CLASS : "";src/client/components/baseComponents/stats/PlayerStatsTree.ts (1)
130-141: Long inline class strings reduce readability.The type button classes span 4 lines with ternary logic embedded. Extract active and inactive class sets to constants.
♻️ Extract button class constants
+ private readonly activeTypeClass = + "text-xs px-3 py-1.5 rounded-md border font-bold uppercase tracking-wider " + + "transition-all duration-200 bg-blue-600 border-blue-500 text-white " + + "shadow-lg shadow-blue-900/40"; + + private readonly inactiveTypeClass = + "text-xs px-3 py-1.5 rounded-md border font-bold uppercase tracking-wider " + + "transition-all duration-200 bg-white/5 border-white/10 text-gray-400 " + + "hover:bg-white/10 hover:text-white"; + render() { // ... ${types.map( (t) => html` <button - class="text-xs px-3 py-1.5 rounded-md border font-bold uppercase tracking-wider transition-all duration-200 ${this - .selectedType === t - ? "bg-blue-600 border-blue-500 text-white shadow-lg shadow-blue-900/40" - : "bg-white/5 border-white/10 text-gray-400 hover:bg-white/10 hover:text-white"}" + class="${this.selectedType === t ? this.activeTypeClass : this.inactiveTypeClass}" @click=${() => this.setGameType(t)} >src/client/components/Difficulties.ts (1)
61-65: Long class constants could be split for readability.The
activeClassconstant is 240+ characters. While functional, consider splitting into semantic parts likeactiveColorClassandactiveAnimationClassfor easier maintenance.♻️ Split class constants
- const activeClass = - "opacity-100 text-[#ff3838] drop-shadow-[0_0_4px_rgba(255,56,56,0.4)] transform group-hover:drop-shadow-[0_0_6px_rgba(255,56,56,0.6)] group-hover:-translate-y-[2px] -translate-y-[1px] transition-all duration-200"; + const baseActiveClass = "opacity-100 text-[#ff3838] transition-all duration-200"; + const activeShadowClass = "drop-shadow-[0_0_4px_rgba(255,56,56,0.4)] group-hover:drop-shadow-[0_0_6px_rgba(255,56,56,0.6)]"; + const activeTransformClass = "transform -translate-y-[1px] group-hover:-translate-y-[2px]"; + const activeClass = `${baseActiveClass} ${activeShadowClass} ${activeTransformClass}`; + const inactiveClass = "opacity-30 w-4 h-4 transition-all duration-200";src/client/PublicLobby.ts (1)
376-393: Consider adding a typed interface for username-input.The
as anycast works but loses type safety. If theusername-inputcomponent exports its class, import and use it for better type checking.// Example improvement: import { UsernameInput } from "./UsernameInput"; // if exported const usernameInput = document.querySelector("username-input") as UsernameInput | null;src/client/components/BaseModal.ts (2)
115-119: Hardcoded fallback page"page-play"reduces flexibility.When closing an inline modal, the code always navigates to
"page-play". This may not be correct if the modal was opened from a different page. Consider passing the return target as a parameter or storing the previous page.
14-17: Prefer composition over inheritance for modal behavior.This abstract class requires subclasses to extend it. A composition-based approach (e.g., a helper object or mixin that modals can use) would be more flexible and easier to test. That said, the current approach works and aligns with other modals in this PR.
src/client/FlagInputModal.ts (2)
9-9: UnusedmodalRefquery.The
modalRefis declared but never used in the component. Consider removing it to reduce confusion.Proposed fix
@customElement("flag-input-modal") export class FlagInputModal extends BaseModal { - @query("#flag-input-modal") private modalRef!: HTMLElement; - @state() private search = "";
14-16: Emptyupdated()override can be removed.This override only calls
super.updated()with no additional logic. It can be safely deleted.src/client/KeybindsModal.ts (1)
35-38: Consider using a typed union instead ofstring | string[].The
valuefield accepts bothstringandstring[], but arrays are normalized to their first element everywhere. If arrays are not truly needed, simplify to juststring. If they are needed, document why.Proposed simplification if arrays are not needed
@state() private keybinds: Record< string, - { value: string | string[]; key: string } + { value: string; key: string } > = {};src/client/Main.ts (1)
237-256: The 100ms timeout is fragile for component readiness.Using
setTimeout(() => {...}, 100)to wait for custom element rendering is brittle. Different devices or load conditions may need more or less time. A better approach is to use a mutation observer or check for element existence in a loop with requestAnimationFrame.Suggested improvement using polling
- // Wait for the flag-input component to be fully ready - customElements.whenDefined("flag-input").then(() => { - // Use a small delay to ensure the component has rendered - setTimeout(() => { - const flagButton = document.querySelector( - "#flag-input-component #flag-input_", - ); - if (!flagButton) { - console.warn("Flag button not found inside component"); - return; - } - flagButton.addEventListener("click", (e) => { - e.preventDefault(); - e.stopPropagation(); - if (flagInputModal && flagInputModal instanceof FlagInputModal) { - flagInputModal.open(); - } - }); - }, 100); - }); + customElements.whenDefined("flag-input").then(() => { + const tryAttachListener = () => { + const flagButton = document.querySelector( + "#flag-input-component #flag-input_", + ); + if (!flagButton) { + requestAnimationFrame(tryAttachListener); + return; + } + flagButton.addEventListener("click", (e) => { + e.preventDefault(); + e.stopPropagation(); + if (flagInputModal && flagInputModal instanceof FlagInputModal) { + flagInputModal.open(); + } + }); + }; + tryAttachListener(); + });src/client/TerritoryPatternsModal.ts (1)
312-329: Options handling could use a type definition.The
open()method acceptsstring | { affiliateCode?: string; showOnlyOwned?: boolean }. Defining a named type makes the API clearer and helps with future maintenance.Define options type
type OpenOptions = { affiliateCode?: string; showOnlyOwned?: boolean; }; public async open(options?: string | OpenOptions) { // ... }src/client/components/baseComponents/Modal.ts (1)
75-78: Backdrop click handler should check for null.When
inlineis true, you assignnullto@click, but Lit handles this correctly. However, the ternary makes the intent less clear.Use conditional binding for clarity
- @click=${this.inline ? null : (e: Event) => this.close()} + @click=${this.inline ? undefined : () => this.close()}src/client/JoinPrivateLobbyModal.ts (1)
311-325: Extract hardcoded unit translation keys to a shared constant.The
unitKeysmapping duplicates translations that exist insrc/client/utilities/RenderUnitTypeOptions.ts. This mapping appears in multiple places (JoinPrivateLobbyModal, BuildMenu, tests) and will become brittle if translation keys change. Consider creating a shared constant for this mapping to maintain a single source of truth.Location
src/client/JoinPrivateLobbyModal.ts, lines 311-325
src/client/Matchmaking.ts (1)
176-179: WebSocket onclose handler is minimal.The handler only logs a message. This is fine since cleanup happens in
onClose(), but if the server closes the connection unexpectedly while the modal is still open, the user sees no feedback.Consider updating the UI state or showing a message when the connection closes unexpectedly:
💡 Optional: Add user feedback on unexpected disconnect
this.socket.onclose = () => { console.log("Matchmaking server closed connection"); + if (this.isModalOpen && this.connected && this.gameID === null) { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: translateText("matchmaking_modal.disconnected"), + color: "yellow", + duration: 3000, + }, + }), + ); + } };
| app.get("/game/:id", async (req, res) => { | ||
| const gameID = req.params.id; | ||
| const game = gm.game(gameID); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate gameID parameter at route entry point
The gameID parameter should be validated against GAME_ID_REGEX before use to prevent passing invalid input to downstream functions and logs.
🔒 Add gameID validation at entry
app.get("/game/:id", async (req, res) => {
const gameID = req.params.id;
+ if (!gameIDSchema.safeParse(gameID).success) {
+ return res.redirect(302, "/");
+ }
const game = gm.game(gameID);Based on learnings, input validation at API boundaries is critical for security.
🤖 Prompt for AI Agents
In @src/server/Worker.ts around lines 275 - 277, Validate the route parameter
gameID at the entry of app.get("/game/:id") by checking req.params.id against
GAME_ID_REGEX; if it fails validation, respond immediately with a 400 (Bad
Request) and an explanatory message and do not call gm.game or log the invalid
value; only proceed to call gm.game(gameID) and any logging after the regex
check passes.
ryanbarlow97
force-pushed
the
discordembedded
branch
from
8f28cc4 to
4c94f48
Compare
ryanbarlow97
force-pushed
the
discordembedded
branch
from
4c94f48 to
98cce38
Compare
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/JoinPrivateLobbyModal.ts (1)
389-395: Bug: Copy URL uses old hash-based format instead of new path-based format.The
copyToClipboardmethod still generates/#join=URLs, but this PR migrates to path-based/game/{id}URLs. This creates an inconsistency where users share broken links.Proposed fix
private async copyToClipboard() { await copyToClipboard( - `${location.origin}/#join=${this.currentLobbyId}`, + `${location.origin}/game/${this.currentLobbyId}`, () => (this.copySuccess = true), () => (this.copySuccess = false), ); }
🤖 Fix all issues with AI agents
In @src/client/Main.ts:
- Around line 639-647: The path-based lobby detection in Main.ts only handles
/game/{id} and returns early, breaking legacy hash links; add a fallback hash
parse before returning by matching window.location.hash for `#join=...`,
validate with GAME_ID_REGEX and set lobbyId accordingly if present; ensure this
fallback runs when the path check yields no lobbyId so old shared links still
open the join modal; also update JoinPrivateLobbyModal (the URL generation
logic) to emit new `/game/{id}` links instead of `#join=` so future links use
the canonical format.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/server/GamePreviewBuilder.ts
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/server/GamePreviewBuilder.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/server/GamePreviewBuilder.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/GamePreviewBuilder.tssrc/client/JoinPrivateLobbyModal.tssrc/client/HostLobbyModal.tssrc/client/Main.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/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/server/GamePreviewBuilder.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/GamePreviewBuilder.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 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/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/server/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/server/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
📚 Learning: 2026-01-08T13:52:00.939Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:00.939Z
Learning: In src/client/Main.ts, when the browser back button is pressed, the `popstate` event fires before the `hashchange` event. The `preventHashUpdate` flag is used to prevent the `hashchange` listener (`onHashUpdate`) from executing after a navigation rollback in the `popstate` listener (`onPopState`), specifically when the user cancels leaving an active game.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
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.tssrc/client/Main.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
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2026-01-08T13:52:00.939Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:00.939Z
Learning: In src/client/Main.ts, ensure the browser back/forward navigation ordering is correctly handled: when popstate fires before hashchange during a navigation rollback (e.g., user cancels leaving an active game), use a preventHashUpdate-like flag to suppress the hashchange listener (onHashUpdate) from running in that rollback scenario. This avoids applying an unintended hash update after a rollback and keeps the UI/game state consistent. Document the flag’s usage and the exact conditions under which it is set/reset to prevent subtle regressions.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (4)
src/server/GamePreviewBuilder.ts (4)
src/core/game/GameView.ts (1)
gameID(910-912)src/core/Schemas.ts (1)
GameInfo(134-140)src/core/game/GameImpl.ts (1)
map(206-208)src/core/configuration/DefaultConfig.ts (1)
playerTeams(317-319)
src/client/JoinPrivateLobbyModal.ts (1)
src/core/Schemas.ts (1)
GAME_ID_REGEX(222-222)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (1)
isValidGameID(224-225)
src/client/Main.ts (1)
src/core/Schemas.ts (1)
GAME_ID_REGEX(222-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (14)
src/client/JoinPrivateLobbyModal.ts (2)
413-429: Clean URL parsing with proper fallback.Good use of
URLconstructor with try-catch, optional chaining on regex match, and validation against the centralizedGAME_ID_REGEX. The fallback to return the original input on failure is a safe approach.
372-374: Good cleanup: URL reset on modal close.Resetting the URL to base path on close prevents stale lobby URLs from persisting after the user leaves. This is consistent with the
HostLobbyModalbehavior.src/client/HostLobbyModal.ts (3)
78-97: Clean URL helpers for cache-busting embed previews.Good separation of concerns:
getRandomString()for suffix generationbuildLobbyUrl()for consistent URL constructionconstructUrl()combines generation with buildingupdateHistory()for browser stateBased on learnings, the
?s=xxxxxsuffix is purely for cache-busting platform previews (Discord, WhatsApp, etc.) and is ignored by join logic.
849-858: Good defensive validation after lobby creation.Validating the lobby ID with
isValidGameIDbefore exposing it in URLs is a good safety check. If the server returns an unexpected format, the error is thrown early rather than propagating a malformed URL.
927-968: Thorough cleanup on modal close.Comprehensive state reset including URL, timers, and all form fields ensures a clean slate for the next lobby creation. Good defensive practice.
src/server/GamePreviewBuilder.ts (5)
143-150: Good HTML escaping utility.Covers the essential HTML entities:
&,<,>,",'. This is used by the Worker to safely inject preview metadata into HTML.
65-74: Clean duration formatting with edge case handling.Good defensive check for non-finite and negative values. The hierarchical formatting (hours → minutes → seconds) is readable.
102-128: Robust winner parsing with format detection.Good handling of both team and individual winner formats. The fallback to return
undefinedfor unknown formats avoids displaying confusing output in previews.
4-32: Flexible schema for external game info.Good use of optional fields and
z.unknown()for the stats field, which can vary in shape. Thez.union([z.number(), z.string()])forplayerTeamshandles both numeric team counts and named team configurations (e.g., "Quads").
220-293: User-generated content in preview metadata is properly escaped.All output from
buildPreviewis correctly escaped usingescapeHtmlin Worker.ts (lines 319–326) before injecting into HTML meta tags. This includes winner names, host username, and disabled units that appear in thetitleanddescriptionfields. No XSS vulnerability exists.src/client/Main.ts (4)
769-776: Good URL sync helper.Simple check to avoid redundant
replaceStatecalls. This keeps the browser URL in sync with the current lobby for sharing without polluting browser history.
537-537: Good event-driven join trigger.Adding
join-changedlistener allows other components (likeAccountModal) to trigger the join flow without direct coupling. Reuses the existingonHashUpdatehandler for consistency.
761-765: Live game URL with state marker.Using
?livequery param as a state indicator for embed previews is consistent with the PR's cache-busting strategy. Thehistory.pushStatecreates a proper history entry for back-button navigation.
704-731: Extended modal close list.Adding
game-top-bar,account-button, andstats-buttonto the batch close ensures all UI elements are properly hidden during game transitions. Based on learnings, this follows the codebase pattern of direct DOM manipulation for UI hiding.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/server/GamePreviewBuilder.ts:
- Around line 130-141: In countActivePlayers, the predicate wrongly returns true
for non-object stats (boolean/number/string), causing invalid stats to be
counted; update the predicate inside the filter in function countActivePlayers
to return false for any stats that are not an object with at least one property
(i.e., replace the final fallback return true with return false so only objects
with Object.keys(...).length > 0 are counted).
🧹 Nitpick comments (4)
src/server/GamePreviewBuilder.ts (4)
4-56: Derive ExternalGameInfo from schema to prevent type drift.The
ExternalGameInfotype (lines 40-56) manually mirrorsExternalGameInfoSchema(lines 4-32). Usez.infer<typeof ExternalGameInfoSchema>instead to eliminate duplication and ensure the type stays synchronized with the schema.♻️ Refactor to use inferred type
Replace the manual type definition with:
-export type ExternalGameInfo = { - info?: { - config?: { - gameMap?: string; - gameMode?: string; - gameType?: string; - maxPlayers?: number; - playerTeams?: number | string; - }; - players?: PlayerInfo[]; - winner?: string[]; - duration?: number; - start?: number; - end?: number; - lobbyCreatedAt?: number; - }; -}; +export type ExternalGameInfo = z.infer<typeof ExternalGameInfoSchema>;Note: You'll need to keep the
PlayerInfotype separate since it's referenced in the schema, or inline it within the schema definition.
76-78: Document the timestamp threshold.The magic number
1e12distinguishes seconds from milliseconds. Add a brief comment explaining this threshold to improve readability.📝 Add clarifying comment
function normalizeTimestamp(timestamp: number): number { + // Timestamps < 1 trillion are assumed to be in seconds (Unix epoch), otherwise milliseconds return timestamp < 1e12 ? timestamp * 1000 : timestamp; }
167-174: Player count fallback chain is complex but functional.The logic correctly handles different data sources (finished vs active games), but the nested fallback on lines 171-173 makes it harder to trace. Consider extracting this to a named helper if similar logic appears elsewhere.
152-286: Consider extracting smaller helpers from buildPreview.The function handles multiple responsibilities (config extraction, player counting, title/description generation) in 134 lines. While it's currently readable, extracting helpers like
buildFinishedGameDescription,buildPrivateLobbyDescription, andbuildTitlewould improve testability and make future changes easier.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/HostLobbyModal.tssrc/server/GamePreviewBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/HostLobbyModal.ts
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
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-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/server/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/server/GamePreviewBuilder.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/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/server/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/server/GamePreviewBuilder.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/server/GamePreviewBuilder.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/server/GamePreviewBuilder.ts
🧬 Code graph analysis (1)
src/server/GamePreviewBuilder.ts (4)
src/core/game/GameView.ts (1)
gameID(910-912)src/core/Schemas.ts (1)
GameInfo(134-140)src/core/game/GameImpl.ts (1)
map(206-208)src/core/configuration/DefaultConfig.ts (1)
playerTeams(317-319)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (7)
src/server/GamePreviewBuilder.ts (7)
65-74: LGTM!The duration formatting logic correctly handles edge cases and produces clean output.
80-98: LGTM!Clean use of
Intl.DateTimeFormatwith explicit UTC timezone handling.
102-128: LGTM!Winner parsing logic correctly handles team and individual formats with appropriate fallbacks.
143-150: LGTM!Standard HTML entity escaping correctly covers the necessary characters.
184-191: LGTM!Team formatting logic correctly handles finished vs lobby states, and the
Math.max(1, ...)guard prevents division edge cases.
202-217: LGTM!Image and title generation logic is clear and handles all expected states.
219-283: LGTM!Description generation correctly handles all game states (finished, private lobby, public lobby) with appropriate detail levels. Username inclusion is safe since HTML escaping occurs at injection time in Worker.ts.
| function countActivePlayers(players: PlayerInfo[] | undefined): number { | ||
| return (players ?? []).filter((p) => { | ||
| if (!p || p.stats === null || p.stats === undefined) return false; | ||
| // Exclude NPCs/bots (they don't have clientID) | ||
| if (!p.clientID) return false; | ||
| // Count only when `stats` has at least one property. | ||
| if (typeof p.stats === "object") { | ||
| return Object.keys(p.stats as Record<string, unknown>).length > 0; | ||
| } | ||
| return true; | ||
| }).length; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the PlayerInfo type definition
rg -t ts "type PlayerInfo\b|interface PlayerInfo" --max-count 5Repository: openfrontio/OpenFrontIO
Length of output: 213
🏁 Script executed:
# Find the stats field type in PlayerInfo or related types
rg -t ts "stats.*:" src/ -A 2 | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1242
🏁 Script executed:
# Check the schema or type definition for stats
rg -t ts "PlayerStatsSchema|stats.*Zod" --max-count 10Repository: openfrontio/OpenFrontIO
Length of output: 735
🏁 Script executed:
# See how countActivePlayers is called and used
rg -t ts "countActivePlayers" -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 1179
🏁 Script executed:
# Check the actual PlayerInfo type definition in GamePreviewBuilder.ts
rg -t ts "type PlayerInfo" src/server/GamePreviewBuilder.ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 259
🏁 Script executed:
# Check the PlayerStatsSchema definition
rg -t ts "export const PlayerStatsSchema" -A 20Repository: openfrontio/OpenFrontIO
Length of output: 987
🏁 Script executed:
# Check if there are any tests for countActivePlayers
rg -t ts "countActivePlayers" tests/ -B 2 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check how stats is populated/set in the codebase
rg -t ts "stats\s*:" src/server/GamePreviewBuilder.ts -B 5 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 277
🏁 Script executed:
# Search for other usages of stats in GamePreviewBuilder context
rg -t ts "\.stats" src/server/GamePreviewBuilder.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 531
🏁 Script executed:
# Check if there are any transformations or mappings of stats before calling countActivePlayers
rg -t ts "countActivePlayers|PlayerInfo" src/server/GamePreviewBuilder.ts -B 10 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1265
🏁 Script executed:
# Check what types of values stats can actually be in the actual game data
rg -t ts "stats\s*:" src/ -B 3 | grep -A 3 "stats" | head -40Repository: openfrontio/OpenFrontIO
Length of output: 1845
🏁 Script executed:
# Look for how stats is created/assigned in the actual game logic
rg -t ts "stats\s*=\s*" src/core -B 2 -A 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 336
🏁 Script executed:
# Check the exact context of where countActivePlayers is called and what data it receives
rg -t ts "buildGamePreview|ExternalGameInfo" src/server/GamePreviewBuilder.ts -B 5 -A 10 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 1311
🏁 Script executed:
# Check if stats is validated before being passed to countActivePlayers
rg -t ts "ExternalGameInfo\|players" src/server/GamePreviewBuilder.ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check git blame for context on this function
git log -p --all -- src/server/GamePreviewBuilder.ts | grep -A 20 "countActivePlayers" | head -40Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there are any related issues or PRs mentioning active players
rg -t ts "active.*player|player.*active" src/server/GamePreviewBuilder.ts -i -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 1289
🏁 Script executed:
# Final check: look at the full context around countActivePlayers to understand data flow
rg -t ts "function buildPreview" src/server/GamePreviewBuilder.ts -A 50 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 1739
The fallback return true on line 139 counts players with invalid stats values.
After filtering out null/undefined stats and players without clientID, the code checks if stats is an object with properties. However, the fallback return true counts players whose stats is a boolean, number, string, or other non-object value as active players. This contradicts the comment "Count only when stats has at least one property."
Fix by returning false for non-object stats:
Suggested fix
function countActivePlayers(players: PlayerInfo[] | undefined): number {
return (players ?? []).filter((p) => {
if (!p || p.stats === null || p.stats === undefined) return false;
// Exclude NPCs/bots (they don't have clientID)
if (!p.clientID) return false;
- // Count only when `stats` has at least one property.
- if (typeof p.stats === "object") {
- return Object.keys(p.stats as Record<string, unknown>).length > 0;
- }
- return true;
+ // Count only when `stats` is an object with at least one property
+ return typeof p.stats === "object" && Object.keys(p.stats as Record<string, unknown>).length > 0;
}).length;
}📝 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.
| function countActivePlayers(players: PlayerInfo[] | undefined): number { | |
| return (players ?? []).filter((p) => { | |
| if (!p || p.stats === null || p.stats === undefined) return false; | |
| // Exclude NPCs/bots (they don't have clientID) | |
| if (!p.clientID) return false; | |
| // Count only when `stats` has at least one property. | |
| if (typeof p.stats === "object") { | |
| return Object.keys(p.stats as Record<string, unknown>).length > 0; | |
| } | |
| return true; | |
| }).length; | |
| } | |
| function countActivePlayers(players: PlayerInfo[] | undefined): number { | |
| return (players ?? []).filter((p) => { | |
| if (!p || p.stats === null || p.stats === undefined) return false; | |
| // Exclude NPCs/bots (they don't have clientID) | |
| if (!p.clientID) return false; | |
| // Count only when `stats` is an object with at least one property | |
| return typeof p.stats === "object" && Object.keys(p.stats as Record<string, unknown>).length > 0; | |
| }).length; | |
| } |
🤖 Prompt for AI Agents
In @src/server/GamePreviewBuilder.ts around lines 130 - 141, In
countActivePlayers, the predicate wrongly returns true for non-object stats
(boolean/number/string), causing invalid stats to be counted; update the
predicate inside the filter in function countActivePlayers to return false for
any stats that are not an object with at least one property (i.e., replace the
final fallback return true with return false so only objects with
Object.keys(...).length > 0 are counted).
Description:
Changes URL embeds within other platforms, e.g. Discord, WhatsApp & X.
Updates game URLs to
/game/<code>instead of/#join=<code>(required for embedded URLs). An added benefit of this is that you would be able to change a url fromopenfront.io/game/RQDUy8nP?replaytoapi.openfront.io/game/RQDUy8nP?replay(add api. In front) and be in the right place for the API data.Updates URLs when joining/leaving private lobbies
Appends a random string to the end of the URL when inside a private lobby and options change - this is to force discord to update the embedded details.
Updates URL in different game states to ?lobby / ?live and ?replay. These do nothing other than being used as a cache-busting solution.
Lobby Info
Discord:
WhatsApp:
x.com:
Game Win Details
Discord:
WhatsApp:
x.com
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n