Discord(et al.) embedded URLs #2740
Conversation
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 24 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a game-preview endpoint and builder, centralizes game ID validation, shifts client join URLs from hash to path with history updates, refactors modal URL/leave behavior, and adjusts server static/mime/caching and nginx worker routing. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Route as GamePreviewRoute
participant GM as GameManager
participant Central as RemoteAPI
participant File as BaseHTML
Browser->>Route: GET /game/:id
Route->>Route: validate id (GAME_ID_REGEX)
Route->>GM: lookup lobby by id
alt lobby found
GM-->>Route: lobby data
else
Route->>Central: fetch public game info
Central-->>Route: public info / 404
end
Route->>Route: buildPreview(lobby, publicInfo)
Route->>File: read base HTML template
Route->>Route: inject OG/Twitter meta into HTML
Route-->>Browser: HTML response with preview meta
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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/GamePreviewBuilder.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/server/Master.ts
🧬 Code graph analysis (2)
src/server/GamePreviewBuilder.ts (4)
src/core/game/GameView.ts (1)
gameID(910-912)src/core/Schemas.ts (1)
GameInfo(130-136)src/core/game/GameImpl.ts (1)
map(199-201)src/core/configuration/DefaultConfig.ts (1)
bots(304-306)
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)src/server/GamePreviewBuilder.ts (2)
ExternalGameInfo(202-202)GamePreviewBuilder(37-200)
⏰ 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 (10)
src/server/Master.ts (4)
67-72: LGTM!Good use of
x-forwarded-protowith fallback chain. Thetrust proxysetting (line 189) limits spoofing risk.
74-114: LGTM!Timeout handling is correct with
finallyblock ensuring cleanup. Both fetch functions follow the same clean pattern.
116-152: LGTM!Clean separation of bot vs. human flows. Input validation with
ID.safeParseprevents invalid game IDs from reaching downstream logic. Parallel fetches are efficient.
191-195: Verify the 50x rate limit increase is intentional.The global rate limit jumped from 20 to 1000 requests per IP per second. While the commit message mentions increasing the rate limit, this is a significant change that could expose the server to abuse.
Consider if a lower value (e.g., 100-200) would suffice, or add a comment explaining why 1000 is needed.
src/server/GamePreviewBuilder.ts (6)
3-35: LGTM!Types are clean and use optional fields appropriately. Using
unknownforstatsis a safe choice when the shape isn't needed.
38-47: LGTM!Duration formatting logic is correct and handles undefined gracefully.
49-73: LGTM!Winner parsing handles both team and player formats cleanly. The fallback to
join(" ")is a reasonable catch-all.
79-86: LGTM!Standard HTML entity escaping covering all necessary characters.
88-151: LGTM!Build logic correctly handles finished games vs. active lobbies. Good use of
encodeURIComponentfor the map name in the image URL.
202-202: LGTM!Using
export typefor type-only exports is clean TypeScript practice.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/Master.ts (1)
309-326: Timeout not cleared in fetchLobbies.Unlike
fetchLobbyInfoandfetchPublicGameInfowhich usetry/finallyto clear timeouts, this function never clears the timeout. While not critical (aborting a completed fetch is harmless), it leaks timers and is inconsistent with the pattern used elsewhere.🔎 Proposed fix
for (const gameID of new Set(publicLobbyIDs)) { const controller = new AbortController(); - setTimeout(() => controller.abort(), 5000); // 5 second timeout + const timeout = setTimeout(() => controller.abort(), 5000); const port = config.workerPort(gameID); const promise = fetch(`http://localhost:${port}/api/game/${gameID}`, { headers: { [config.adminHeader()]: config.adminToken() }, signal: controller.signal, }) .then((resp) => resp.json()) .then((json) => { return json as GameInfo; }) .catch((error) => { log.error(`Error fetching game ${gameID}:`, error); - // Return null or a placeholder if fetch fails publicLobbyIDs.delete(gameID); return null; + }) + .finally(() => { + clearTimeout(timeout); });
♻️ Duplicate comments (1)
src/server/Master.ts (1)
18-36: Duplicate bot list still present.The bot user-agent list is defined twice: inline in
joinPreviewLimiter.skip(lines 23-34) and again inBOT_USER_AGENTS(lines 49-60). MoveBOT_USER_AGENTSabove the limiter and reuse it.🔎 Proposed fix
+const BOT_USER_AGENTS = [ + "discordbot", + "twitterbot", + "slackbot", + "facebookexternalhit", + "linkedinbot", + "telegrambot", + "applebot", + "snapchat", + "whatsapp", + "pinterestbot", +]; + const joinPreviewLimiter = rateLimit({ windowMs: 1000, // 1 second max: 100, // limit each IP to 100 requests per windowMs skip: (req) => { const ua = req.get("user-agent")?.toLowerCase() ?? ""; - return [ - "discordbot", - "twitterbot", - "slackbot", - "facebookexternalhit", - "linkedinbot", - "telegrambot", - "applebot", - "snapchat", - "whatsapp", - "pinterestbot", - ].some((bot) => ua.includes(bot)); + return BOT_USER_AGENTS.some((bot) => ua.includes(bot)); }, }); - -const BOT_USER_AGENTS = [ - "discordbot", - "twitterbot", - "slackbot", - "facebookexternalhit", - "linkedinbot", - "telegrambot", - "applebot", - "snapchat", - "whatsapp", - "pinterestbot", -];Also applies to: 49-60
🧹 Nitpick comments (1)
src/server/Master.ts (1)
154-159: Consider using an async wrapper or express-async-errors.The pattern works but mixing callback-style with
.catch()on async function is less clean than using an async wrapper or middleware likeexpress-async-errors. This is optional since the current approach handles errors correctly.🔎 Alternative pattern
-app.get("/join/:gameId", joinPreviewLimiter, (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.get("/join/:gameId", joinPreviewLimiter, async (req, res) => { + try { + await 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"); + } +});
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/GamePreviewBuilder.tssrc/server/Master.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/GamePreviewBuilder.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.ts
🧬 Code graph analysis (1)
src/server/Master.ts (3)
src/core/Schemas.ts (2)
GameInfo(130-136)ID(208-211)src/core/configuration/DefaultConfig.ts (1)
workerPort(207-209)src/server/GamePreviewBuilder.ts (2)
ExternalGameInfo(203-203)GamePreviewBuilder(37-201)
⏰ 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/Master.ts (7)
62-65: LGTM!Clean helper function with proper nullish coalescing.
67-72: LGTM!Good handling of
X-Forwarded-Protowith proper fallback chain. The comma split correctly handles multiple proxies.
74-94: LGTM!The
try/finallypattern correctly clears the timeout regardless of success or failure. Clean error handling.
96-114: LGTM!Consistent pattern with
fetchLobbyInfo. TheAPI_DOMAINenvironment variable provides good flexibility for different deployment environments.
116-152: LGTM!Good structure:
- Zod validation with early return on invalid ID
- Parallel fetching with
Promise.allis efficient- Clean separation between bot and human responses
no-storecache header is correct for dynamic previews
161-166: LGTM!Consistent with the caching strategy for other static assets.
197-202: Verify the rate limit increase is intentional.The global rate limit was increased from 20 to 1000 requests per second per IP. This is a 50x increase and quite permissive. Please confirm this is intentional and won't expose the server to abuse.
ryanbarlow97
force-pushed
the
discordembedded
branch
from
35a4190 to
3c62927
Compare
ryanbarlow97
force-pushed
the
discordembedded
branch
from
3c62927 to
d2d0e8d
Compare
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 from `openfront.io/game/RQDUy8nP?replay` to `api.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. ----------------------------------------------- Discord: <img width="556" height="487" alt="image" src="https://github.com/user-attachments/assets/efd4a06d-506c-4036-9403-ee7c9a669e21" /> WhatsApp: <img width="353" height="339" alt="image" src="https://github.com/user-attachments/assets/3b2d0c69-988c-424f-9dee-f4e6a6868f6b" /> x.com: <img width="588" height="325" alt="image" src="https://github.com/user-attachments/assets/d9e78169-20be-4a3e-8df4-8ad41d08a750" /> ------------------------- Discord: <img width="506" height="468" alt="image" src="https://github.com/user-attachments/assets/69947774-c943-4a50-b470-5634ed3bf3d7" /> WhatsApp: <img width="770" height="132" alt="image" src="https://github.com/user-attachments/assets/eec28bf8-bf64-4ab8-954e-03dfdd1aae40" /> x.com <img width="584" height="350" alt="image" src="https://github.com/user-attachments/assets/168063e2-b707-422b-b7a1-0025f3ebeb92" /> - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: w.o.n
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 from `openfront.io/game/RQDUy8nP?replay` to `api.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. ----------------------------------------------- Discord: <img width="556" height="487" alt="image" src="https://github.com/user-attachments/assets/efd4a06d-506c-4036-9403-ee7c9a669e21" /> WhatsApp: <img width="353" height="339" alt="image" src="https://github.com/user-attachments/assets/3b2d0c69-988c-424f-9dee-f4e6a6868f6b" /> x.com: <img width="588" height="325" alt="image" src="https://github.com/user-attachments/assets/d9e78169-20be-4a3e-8df4-8ad41d08a750" /> ------------------------- Discord: <img width="506" height="468" alt="image" src="https://github.com/user-attachments/assets/69947774-c943-4a50-b470-5634ed3bf3d7" /> WhatsApp: <img width="770" height="132" alt="image" src="https://github.com/user-attachments/assets/eec28bf8-bf64-4ab8-954e-03dfdd1aae40" /> x.com <img width="584" height="350" alt="image" src="https://github.com/user-attachments/assets/168063e2-b707-422b-b7a1-0025f3ebeb92" /> - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: w.o.n
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 from `openfront.io/game/RQDUy8nP?replay` to `api.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. ----------------------------------------------- Discord: <img width="556" height="487" alt="image" src="https://github.com/user-attachments/assets/efd4a06d-506c-4036-9403-ee7c9a669e21" /> WhatsApp: <img width="353" height="339" alt="image" src="https://github.com/user-attachments/assets/3b2d0c69-988c-424f-9dee-f4e6a6868f6b" /> x.com: <img width="588" height="325" alt="image" src="https://github.com/user-attachments/assets/d9e78169-20be-4a3e-8df4-8ad41d08a750" /> ------------------------- Discord: <img width="506" height="468" alt="image" src="https://github.com/user-attachments/assets/69947774-c943-4a50-b470-5634ed3bf3d7" /> WhatsApp: <img width="770" height="132" alt="image" src="https://github.com/user-attachments/assets/eec28bf8-bf64-4ab8-954e-03dfdd1aae40" /> x.com <img width="584" height="350" alt="image" src="https://github.com/user-attachments/assets/168063e2-b707-422b-b7a1-0025f3ebeb92" /> - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: w.o.n
…2777) Please merge it into v29, since in that version the back navigation out of a game is currently **broken** after switching from hash-based to path-based routing via #2740 ## Description: Protect against players accidentally leaving an active game by pressing the browser back button. Uses the same confirmation dialog as the game exit button. Partially handles issue #1877 (protects against back button, not closing tab or editing the URL directly). <img width="861" height="373" alt="image" src="https://github.com/user-attachments/assets/167cc137-6df3-44a7-a594-91ffd904857d" /> Partial credit to PR #2141 ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: deshack_82603
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