Better crazygames support #2900
Conversation
WalkthroughAdds a CrazyGames SDK with readiness gating, user auth hooks, midgame ad and gameplay start/stop APIs, invite-link and instant-multiplayer support, and integrates these into UI flows to hide/adapt features and gate actions when running on CrazyGames. (38 words) Changes
Sequence DiagramsequenceDiagram
participant App as Client App
participant SDK as CrazyGames SDK
participant UI as UI Component
participant Ad as Ad Provider
App->>SDK: init()
SDK->>SDK: create readyPromise & resolveReady
SDK-->>App: resolveReady()
UI->>SDK: await ready()
UI->>SDK: getUsername()
SDK-->>UI: username | null
UI->>SDK: addAuthListener(listener)
SDK-->>UI: invoke listener on auth change
UI->>SDK: requestMidgameAd()
SDK->>Ad: request ad (adType, callbacks)
Ad-->>SDK: adStarted / adFinished / adError
SDK-->>UI: resolve ad promise
UI->>SDK: gameplayStop() / gameplayStart()
SDK-->>UI: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧠 Learnings (12)📚 Learning: 2025-06-09T02:20:43.637ZApplied to files:
📚 Learning: 2026-01-02T18:11:15.132ZApplied to files:
📚 Learning: 2025-10-21T20:06:04.823ZApplied to files:
📚 Learning: 2026-01-13T20:16:20.098ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
📚 Learning: 2026-01-08T13:52:00.939ZApplied to files:
📚 Learning: 2026-01-12T21:37:01.156ZApplied to files:
📚 Learning: 2025-07-23T12:36:35.354ZApplied to files:
📚 Learning: 2025-07-14T20:41:57.645ZApplied to files:
📚 Learning: 2025-12-13T14:58:29.645ZApplied to files:
📚 Learning: 2026-01-08T13:52:08.203ZApplied to files:
📚 Learning: 2025-06-07T13:15:55.439ZApplied to files:
🧬 Code graph analysis (5)src/client/Main.ts (2)
src/client/UsernameInput.ts (1)
src/client/PatternInput.ts (1)
src/client/Matchmaking.ts (5)
src/client/SinglePlayerModal.ts (1)
⏰ 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)
🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/UsernameInput.ts (1)
165-178: Several issues with async username fetch pattern.
- Dead code: Line 171
return null;inside.then()does nothing useful.- Missing error handling: If
crazyGamesSDK.getUsername()rejects, the error is unhandled. Add.catch().- Bypasses parsing: Setting
baseUsernamedirectly skipsparseAndSetUsername(), so SDK usernames with clan tags won't be parsed correctly.- Not persisted: SDK username is not stored to localStorage, so it won't survive page refresh.
Proposed fix
private getUsername(): string { - crazyGamesSDK.getUsername().then((username) => { - if (username) { - this.baseUsername = username; - this.requestUpdate(); - } - return null; - }); + crazyGamesSDK + .getUsername() + .then((username) => { + if (username) { + this.parseAndSetUsername(username); + this.storeUsername(this.constructFullUsername()); + this.requestUpdate(); + } + }) + .catch((error) => { + console.error("Failed to get CrazyGames username:", error); + }); const storedUsername = localStorage.getItem(usernameKey); if (storedUsername) { return storedUsername; } return this.generateNewUsername(); }
🤖 Fix all issues with AI agents
In `@src/client/CrazyGamesSDK.ts`:
- Around line 115-121: The getUsername method calls
window.CrazyGames!.SDK.user.getUser() without protecting against exceptions;
wrap the SDK call in a try-catch inside getUsername so any thrown error is
caught, log or handle the error consistently with other methods (e.g., use
existing logger or error handling pattern in this class), and return null on
failure; specifically update the async getUsername() implementation to await the
SDK call inside a try block and return the username or null, and return null
from the catch block after logging the error.
- Around line 123-129: The isInstantMultiplayer method currently accesses
window.CrazyGames!.SDK.game.isInstantMultiplayer without error handling; update
isInstantMultiplayer to keep the initial await this.ready() call but wrap the
SDK property access in a try-catch (similar to other SDK methods and the
getUsername pattern), returning false if an exception is thrown and optionally
logging the error; reference the isInstantMultiplayer function and this.ready()
when making the change.
In `@src/client/Main.ts`:
- Around line 645-652: The instant-multiplayer path calls
crazyGamesSDK.isInstantMultiplayer() and directly opens the host modal via
this.hostModal.open() without performing the same username validation used by
the hostLobbyButton handler; update the instant-multiplayer branch to run the
same validation logic (reuse the hostLobbyButton's validation routine or the
exact check it uses) and only call this.hostModal.open() when the username check
passes, otherwise abort and surface the same validation error/feedback as
hostLobbyButton.
In `@startup.sh`:
- Around line 88-92: The if/else in startup.sh around the DOMAIN/SUBDOMAIN check
is redundant because both branches call exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf; remove the entire if [...] fi block and
replace it with a single direct exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf line (or, if there was intended
different behavior, restore the original differing command in the branch that
should differ instead of duplicating the same exec).
🧹 Nitpick comments (2)
src/client/graphics/layers/UnitDisplay.ts (1)
133-133: Redundant breakpoint classes can be simplified.The CSS mixes arbitrary breakpoint
min-[1200px]:*with standard Tailwind breakpointsxl:*. Sincexlis 1280px andmin-[1200px]is 1200px, thexl:flex-colandxl:gap-2classes are redundant—they're already covered bymin-[1200px]:flex-colandmin-[1200px]:gap-2.Consider removing the
xl:*classes to keep the responsive logic clear and consistent:♻️ Suggested cleanup
- class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row xl:flex-col min-[1200px]:flex-col 2xl:gap-5 xl:gap-2 min-[1200px]:gap-2 justify-center items-center" + class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row min-[1200px]:flex-col 2xl:gap-5 min-[1200px]:gap-2 justify-center items-center"src/client/graphics/layers/SettingsModal.ts (1)
3-4: Consider using relative imports for consistency.Other files in the codebase use relative paths (e.g.,
../../CrazyGamesSDK) for these imports. Using absolute-style paths likesrc/client/...might work depending on build config, but mixing styles can cause confusion.🔧 Suggested change
-import { crazyGamesSDK } from "src/client/CrazyGamesSDK"; -import { PauseGameIntentEvent } from "src/client/Transport"; +import { crazyGamesSDK } from "../../CrazyGamesSDK"; +import { PauseGameIntentEvent } from "../../Transport";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/client/CrazyGamesSDK.tssrc/client/Main.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
🧰 Additional context used
🧠 Learnings (9)
📚 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/graphics/layers/GameRightSidebar.tssrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/client/SinglePlayerModal.tssrc/client/graphics/layers/WinModal.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/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/graphics/layers/GameRightSidebar.tssrc/client/Main.tssrc/client/UsernameInput.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/SettingsModal.tssrc/client/SinglePlayerModal.tssrc/client/CrazyGamesSDK.tssrc/client/graphics/layers/WinModal.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.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/client/Main.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/Main.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
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (6)
src/client/graphics/layers/GameRightSidebar.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/Main.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/SinglePlayerModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)
src/client/graphics/layers/WinModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(284-284)
🔇 Additional comments (10)
src/client/graphics/layers/GameRightSidebar.ts (2)
106-114: LGTM! Pause/resume gameplay tracking is correct.When the game pauses,
gameplayStop()signals the SDK; when resuming,gameplayStart()signals active play. The toggle logic is sound.
116-128: LGTM! Exit flow with midgame ad is well-structured.The async/await pattern ensures the ad completes before stopping gameplay and redirecting. Clean and readable.
src/client/graphics/layers/WinModal.ts (1)
252-257: LGTM! Gameplay stop on modal show is appropriate.Signaling the SDK that active gameplay has ended when showing the win/death modal is the right approach. The call is placed before async content loading, which is correct.
src/client/SinglePlayerModal.ts (1)
843-844: LGTM! Midgame ad placement before game start is correct.Requesting the ad after validation completes but before dispatching the join-lobby event ensures users see the ad without blocking invalid submissions.
src/client/graphics/layers/SettingsModal.ts (1)
108-117: LGTM! Pause logic with SDK integration is correct.The guard
shouldPause && !wasPausedWhenOpenedensures SDK calls and pause events only fire when needed - not when the game was already paused before opening settings. Clean logic.src/client/Main.ts (2)
217-217: LGTM! Field extraction for reuse is clean.Moving
hostModalto a class field enables access fromhandleUrl(). Good refactor.
526-529: LGTM! Type guard and assignment are correct.Consistent with other modal initialization patterns in this file.
src/client/CrazyGamesSDK.ts (3)
44-61: Clean deferred promise pattern with timeout.The ready mechanism is well designed: the 3-second timeout prevents indefinite waiting if SDK init never completes. Using
Promise.racehere is the right approach.
256-280: Good defensive implementation.The
requestMidgameAdmethod handles all error paths gracefully and always resolves the promise, preventing callers from hanging. The callback pattern matches the SDK API correctly.
6-21: Use literal union type foradTypeparameter inad.requestAd().The
user.getUser()signature is correct and matches the official CrazyGames SDK documentation. However,ad.requestAd()should constrainadTypeto"midgame" | "rewarded"instead ofstring. The documentation specifies these two values, and using a literal union type provides better type safety and editor autocomplete:ad: { requestAd: ( adType: "midgame" | "rewarded", callbacks: { adStarted: () => void; adFinished: () => void; adError: (error: any) => void; }, ) => void; };⛔ Skipped due to learnings
Learnt from: Dimitrije-V Repo: openfrontio/OpenFrontIO PR: 1814 File: src/client/graphics/layers/PlayerActionHandler.ts:0-0 Timestamp: 2025-08-14T14:05:00.867Z Learning: In TypeScript, when dealing with union types like NukeType (derived from UnitType), it's better to accept the broader type (UnitType) as a parameter and use runtime type checking rather than trying to narrow the parameter type at compile time. The approach of keeping flexible input parameters with runtime validation followed by properly typed state/storage is more practical than over-constraining function signatures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/UsernameInput.ts (1)
165-178: Async result is ignored - username fetch has no effect on return value.The
getUsername()method fires an async call tocrazyGamesSDK.getUsername()but does not await it. The method returns the localStorage value or generates a new username before the SDK call completes. The SDK username is only applied as a side effect later viathis.baseUsername = username.This creates a race: the initial render uses localStorage/generated username, then potentially flickers to the SDK username after the promise resolves.
Proposed fix - make connectedCallback async
- connectedCallback() { + async connectedCallback() { super.connectedCallback(); - const stored = this.getUsername(); - this.parseAndSetUsername(stored); + // Try CrazyGames username first + const cgUsername = await crazyGamesSDK.getUsername(); + if (cgUsername) { + this.baseUsername = cgUsername; + this.requestUpdate(); + return; + } + // Fall back to localStorage or generate new + const stored = this.getStoredUsername(); + this.parseAndSetUsername(stored); } - private getUsername(): string { - crazyGamesSDK.getUsername().then((username) => { - if (username) { - this.baseUsername = username; - this.requestUpdate(); - } - return null; - }); + private getStoredUsername(): string { const storedUsername = localStorage.getItem(usernameKey); if (storedUsername) { return storedUsername; } return this.generateNewUsername(); }src/client/CrazyGamesSDK.ts (1)
63-77: Debug code left in -return trueshort-circuits the real check.Line 64 has
return true;which makesisOnCrazyGames()always returntrue, regardless of the actual environment. The actual detection logic below (lines 65-76) is now unreachable dead code.This will cause CrazyGames-specific behavior to run everywhere, including hiding matchmaking buttons and triggering SDK calls that will fail outside CrazyGames.
Proposed fix - remove debug line
isOnCrazyGames(): boolean { - return true; try { // Check if we're in an iframe if (window.self !== window.top) {
🤖 Fix all issues with AI agents
In `@src/client/Matchmaking.ts`:
- Around line 252-259: The event listener for "userMeResponse" added in
connectedCallback causes a leak because it's never removed; in
disconnectedCallback remove the exact listener by defining the handler as a
named method or a bound property (e.g., this.onUserMeResponse) and use
document.removeEventListener("userMeResponse", this.onUserMeResponse). Update
Matchmaking.connectedCallback to attach the handler via that property and
implement Matchmaking.disconnectedCallback to call document.removeEventListener
with the same function reference so listeners don't accumulate when the
component is detached.
♻️ Duplicate comments (3)
startup.sh (1)
88-92: Redundant conditional - both branches do the same thing.After removing the timeout wrapper, both the
ifandelsebranches run the exact same command. The conditional no longer serves a purpose and can be simplified.🧹 Suggested fix: remove the dead conditional
# Start supervisord -if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -else - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -fi +exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.confsrc/client/Main.ts (1)
597-604: Add username validation before opening the host modal.The instant multiplayer path calls
this.hostModal.open()without validating the username first. ThehostLobbyButtonhandler (lines 486-501) validates username before proceeding. This path should do the same.src/client/CrazyGamesSDK.ts (1)
116-130: Missing try-catch around SDK calls.Both
getUsername()andisInstantMultiplayer()access SDK methods without error handling. Other methods in this class consistently wrap SDK calls in try-catch blocks. These should follow the same pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/client/CrazyGamesSDK.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
✅ Files skipped from review due to trivial changes (1)
- src/client/components/PatternButton.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/graphics/layers/SettingsModal.ts
- src/client/SinglePlayerModal.ts
- src/client/graphics/layers/WinModal.ts
- src/client/graphics/layers/UnitDisplay.ts
🧰 Additional context used
🧠 Learnings (13)
📚 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/components/PlayPage.tssrc/client/components/DesktopNavBar.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/components/PlayPage.tssrc/client/components/DesktopNavBar.tssrc/client/UsernameInput.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/PatternInput.tssrc/client/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-07-23T12:36:35.354Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1534
File: src/client/LangSelector.ts:97-106
Timestamp: 2025-07-23T12:36:35.354Z
Learning: In OpenFrontIO's LangSelector.ts, the getClosestSupportedLang method always joins language code parts with underscores ("_") because all keys in the languageMap use underscore format (e.g., pt_BR, sv_SE, zh_CN). This normalization ensures consistency regardless of whether the input language code uses hyphens or underscores as delimiters.
Applied to files:
src/client/LangSelector.ts
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
src/client/LangSelector.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/client/Main.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/Main.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
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.tssrc/client/CrazyGamesSDK.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/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.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/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/CrazyGamesSDK.ts
🧬 Code graph analysis (5)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(285-285)
src/client/Main.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(285-285)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(285-285)
src/client/Matchmaking.ts (3)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(285-285)
src/client/graphics/layers/GameRightSidebar.ts (4)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(285-285)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)src/core/game/GameView.ts (1)
isAlive(470-472)src/core/game/PlayerImpl.ts (1)
isAlive(347-349)
⏰ 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 (13)
src/client/LangSelector.ts (1)
231-231: LGTM!Adding
"matchmaking-button"to the components list ensures translation updates reach the new custom element. This aligns with the refactor inPlayPage.tsthat replaces explicit button markup with the<matchmaking-button>component.src/client/components/PlayPage.ts (1)
140-142: LGTM!Clean refactor replacing two explicit button variants with a single declarative
<matchmaking-button>component. This moves the login-state conditional logic into the component itself, keeping the page template simpler.src/client/components/DesktopNavBar.ts (2)
107-111: LGTM!Adding
no-crazygamesclass to the store button enables the globalhideCrazyGamesElementslogic to hide it when running inside the CrazyGames environment.
128-141: LGTM!The
no-crazygamesclass on the account button and avatar image follows the same pattern. This ensures account-related UI is hidden in the CrazyGames context where external authentication is not supported.src/client/graphics/layers/GameRightSidebar.ts (2)
106-114: LGTM!Correct integration of CrazyGames SDK gameplay state. Calling
gameplayStop()when pausing andgameplayStart()when resuming ensures the SDK knows when the game is interruptible (for ads, overlays, etc.).
116-128: Error handling is already in place — no changes needed.The
requestMidgameAd()method never rejects the promise. All code paths (SDK not ready, ad finished, ad error, caught exceptions) callresolve(), so the async flow is safe. The method cannot throw and block the redirect. Additionally,gameplayStop()is already awaited on line 125 and handles errors internally.Likely an incorrect or invalid review comment.
src/client/PatternInput.ts (2)
8-8: LGTM!Import added for the SDK singleton.
90-94: Pattern input is intentionally hidden on CrazyGames, consistent with codebase approach. Other UI components like Matchmaking also return empty templates on CrazyGames, confirming this is the standard pattern for the platform. No changes needed.src/client/Matchmaking.ts (2)
267-304: LGTM!The CrazyGames guard and login-based rendering split are clean. Both branches include the modal component, which is consistent.
307-329: LGTM!The handlers follow existing patterns in the codebase. Username validation before opening the modal and the navigation to account page are both correct.
src/client/Main.ts (1)
915-935: LGTM!The
hideCrazyGamesElementsfunction with delayed calls handles late-rendered Lit components. This is a practical workaround for components that render after initial DOM load.src/client/CrazyGamesSDK.ts (2)
44-61: LGTM!The readiness mechanism with
Promise.raceand a 3-second timeout is a clean pattern. It prevents indefinite waits while allowing async SDK initialization.
257-282: LGTM!The
requestMidgameAdmethod handles all cases well: SDK not ready, ad finished, ad error, and SDK call failure. The Promise always resolves, which is appropriate for optional ad functionality that should not block the game.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/CrazyGamesSDK.ts (1)
69-83: Debug code left in -return truemakes platform detection dead code.Line 70 unconditionally returns
true, making lines 71-82 unreachable. This looks like debug code that was not removed before commit.This will cause the SDK to always behave as if running on CrazyGames, which will break functionality on other deployments:
- SDK initialization will be attempted everywhere
- UI elements may be incorrectly hidden
- Username fetching and ad requests will fail unexpectedly
Proposed fix - remove debug line
isOnCrazyGames(): boolean { - return true; try { // Check if we're in an iframe if (window.self !== window.top) {
🤖 Fix all issues with AI agents
In `@src/client/CrazyGamesSDK.ts`:
- Around line 130-148: The addAuthListener method uses a synchronous isReady()
check whereas other APIs (getUsername, isInstantMultiplayer) use the async
ready() with timeout; change addAuthListener to the same async pattern: make
addAuthListener return a Promise<void> (or be async), call this.ready(timeout)
and await it before invoking
window.CrazyGames!.SDK.user.addAuthListener(listener), log and throw or handle
timeout errors consistently, and keep the same try/catch around
window.CrazyGames!.SDK.user.addAuthListener to surface any failures.
In `@src/client/graphics/layers/GameRightSidebar.ts`:
- Around line 116-128: The onExitButtonClick handler can throw from
crazyGamesSDK.requestMidgameAd() or crazyGamesSDK.gameplayStop(), preventing the
final redirect; wrap those SDK calls in a try-catch around requestMidgameAd()
and gameplayStop() inside onExitButtonClick, log or console.error any caught
error for diagnostics, and ensure window.location.href = "/" runs in a finally
block (or after the catch) so the redirect always occurs even if the SDK calls
fail.
In `@src/client/PatternInput.ts`:
- Around line 91-94: Remove the premature hard-coded return in the
isOnCrazyGames() method in CrazyGamesSDK: delete the `return true;` early return
so the function proceeds to the existing iframe/platform detection logic (the
code that checks window.top, document.referrer, or iframe identifiers) and
correctly returns true only when actually running on CrazyGames; ensure the
method name isOnCrazyGames() is left intact and its detection branch executes
normally.
♻️ Duplicate comments (3)
src/client/Matchmaking.ts (1)
252-259: Missing cleanup for event listener - memory leak.This was flagged in a previous review. The
userMeResponselistener is added inconnectedCallbackbut never removed. Extract the handler to a class property and adddisconnectedCallbackto remove it.src/client/CrazyGamesSDK.ts (2)
122-128: Missing try-catch around SDK call.The SDK call at line 127 can throw, but there's no error handling. Other methods in this class wrap SDK calls in try-catch.
Proposed fix
async getUsername(): Promise<string | null> { const isReady = await this.ready(); if (!isReady) { return null; } - return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + try { + return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + } catch (error) { + console.error("Failed to get user:", error); + return null; + } }
150-156: Missing try-catch around SDK access.The SDK property access at line 155 could throw, but there's no error handling. Other methods wrap SDK calls in try-catch.
Proposed fix
async isInstantMultiplayer(): Promise<boolean> { const isReady = await this.ready(); if (!isReady) { return false; } - return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + try { + return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + } catch (error) { + console.error("Failed to check instant multiplayer:", error); + return false; + } }
🧹 Nitpick comments (2)
src/client/components/DesktopNavBar.ts (1)
128-141: Redundantno-crazygamesclass on nested element.The avatar image (line 137) is inside the account button (line 130). Since the parent button already has
no-crazygames, the class on the child image is redundant—when the parent hides, so does everything inside it.Consider removing the class from the avatar:
Suggested change
<img id="nav-account-avatar" - class="no-crazygames hidden w-8 h-8 rounded-full object-cover" + class="hidden w-8 h-8 rounded-full object-cover" alt=""src/client/Matchmaking.ts (1)
307-325: Consider adding type safety for DOM queries.Casting
usernameInputtoanyloses type safety. Ifusername-inputexposesisValid()andvalidationError, define an interface for it.Suggested approach
// Define interface for the expected shape interface UsernameInputElement extends HTMLElement { isValid(): boolean; validationError?: string; } // Use in handler private handleLoggedInClick() { const usernameInput = document.querySelector("username-input") as UsernameInputElement | null; const publicLobby = document.querySelector("public-lobby") as { leaveLobby?: () => void } | null; if (usernameInput?.isValid()) { this.open(); publicLobby?.leaveLobby?.(); } else { // ... } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/client/CrazyGamesSDK.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- src/client/graphics/layers/UnitDisplay.ts
- src/client/UsernameInput.ts
- startup.sh
- src/client/Main.ts
- src/client/SinglePlayerModal.ts
- src/client/LangSelector.ts
🧰 Additional context used
🧠 Learnings (9)
📚 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/components/PatternButton.tssrc/client/graphics/layers/WinModal.tssrc/client/graphics/layers/SettingsModal.tssrc/client/Matchmaking.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/components/PatternButton.tssrc/client/components/DesktopNavBar.tssrc/client/graphics/layers/WinModal.tssrc/client/PatternInput.tssrc/client/graphics/layers/SettingsModal.tssrc/client/Matchmaking.tssrc/client/CrazyGamesSDK.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/WinModal.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Matchmaking.ts
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/CrazyGamesSDK.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/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
🧬 Code graph analysis (5)
src/client/graphics/layers/WinModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(311-311)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(311-311)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(311-311)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/Matchmaking.ts (5)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(311-311)src/client/LangSelector.ts (1)
translateText(277-301)src/client/Utils.ts (1)
translateText(165-224)
src/client/graphics/layers/GameRightSidebar.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(311-311)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
⏰ 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 (17)
src/client/components/PatternButton.ts (1)
74-79: LGTM!Adding
no-crazygamesto the wrapper div is consistent with how other UI components (likeDesktopNavBarandPatternInput) handle CrazyGames-specific visibility. This allows the hiding logic inMain.tsto target this element correctly.src/client/components/DesktopNavBar.ts (1)
107-111: LGTM!Hiding the store button on CrazyGames platform makes sense. The class follows the established pattern for conditional visibility.
src/client/graphics/layers/SettingsModal.ts (2)
3-4: Imports look correct.The
crazyGamesSDKsingleton andPauseGameIntentEventare imported from their correct modules, matching the exports shown in the relevant code snippets.
108-117: SDK methods already handle errors gracefully—code is correct as written.The
gameplayStop()andgameplayStart()methods inCrazyGamesSDK.tsalready include try-catch blocks that catch and log errors to console. Errors fail silently without propagating, so wrapping the calls in SettingsModal would be redundant. The pattern of not awaiting these methods is intentional and consistent throughout the codebase. The logic correctly gates both SDK calls and the pause event emission under the same condition, preventing unpausing a game that was already paused before the modal opened.src/client/graphics/layers/WinModal.ts (1)
252-261: LGTM! Fire-and-forget call is fine here.The
gameplayStop()call signals the CrazyGames SDK that gameplay has paused when the win/death modal appears. The fire-and-forget pattern (noawait) matches the existing usage ofhappytime()on lines 299 and 329, so this is consistent.src/client/graphics/layers/GameRightSidebar.ts (1)
106-114: LGTM! Pause toggle with SDK integration.The fire-and-forget calls to
gameplayStop()andgameplayStart()are appropriate here. The user expects immediate UI feedback when toggling pause, so blocking on SDK responses would degrade the experience.src/client/components/PlayPage.ts (1)
139-142: Clean refactor to a single component.Moving login-state awareness into
<matchmaking-button>simplifiesPlayPageand follows composition over inheritance. The parent no longer needs to track auth state for this UI.src/client/Matchmaking.ts (2)
267-270: LGTM! Consistent platform check.This matches the pattern used in
PatternInput.tsfor hiding features on CrazyGames.
272-305: Both render paths include<matchmaking-modal>- good.The modal component is rendered regardless of login state, which ensures it is available when needed. The button text and click handlers correctly differ based on
isLoggedIn.src/client/CrazyGamesSDK.ts (8)
1-45: Type declarations look good.The SDK interface declarations are clear and match the expected CrazyGames API surface.
47-57: Readiness promise pattern is clean.The deferred promise pattern with captured resolver is a good approach for coordinating initialization state.
59-67: Timeout-based readiness check is solid.The
Promise.racepattern with a 3-second timeout is a good safeguard against indefinite hangs.
85-87: Synchronous readiness check is correct.Simple guard combining platform check with initialization state.
89-120: Initialization logic is well structured.Good polling mechanism for SDK availability with proper error handling. The
resolveReady()call on success correctly signals the readiness promise.
158-281: Existing SDK methods are well structured.All existing methods follow a consistent pattern with
isReady()guards and try-catch error handling.
283-308: Ad request implementation is robust.Good handling of all completion paths (success, error, not-ready). The Promise always resolves rather than rejects, which is appropriate for optional ad functionality that shouldn't break the main flow.
311-311: Singleton export is appropriate.SDK wrapper with global state benefits from singleton pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private async onExitButtonClick() { | ||
| const isAlive = this.game.myPlayer()?.isAlive(); | ||
| if (isAlive) { | ||
| const isConfirmed = confirm( | ||
| translateText("help_modal.exit_confirmation"), | ||
| ); | ||
| if (!isConfirmed) return; | ||
| } | ||
| crazyGamesSDK.gameplayStop().then(() => { | ||
| // redirect to the home page | ||
| window.location.href = "/"; | ||
| }); | ||
| await crazyGamesSDK.requestMidgameAd(); | ||
| await crazyGamesSDK.gameplayStop(); | ||
| // redirect to the home page | ||
| window.location.href = "/"; | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent users getting stuck on exit.
If requestMidgameAd() or gameplayStop() throws an error, the redirect will never happen and the user will be stuck. Wrap in try-catch to ensure the redirect always occurs.
Proposed fix
private async onExitButtonClick() {
const isAlive = this.game.myPlayer()?.isAlive();
if (isAlive) {
const isConfirmed = confirm(
translateText("help_modal.exit_confirmation"),
);
if (!isConfirmed) return;
}
- await crazyGamesSDK.requestMidgameAd();
- await crazyGamesSDK.gameplayStop();
- // redirect to the home page
- window.location.href = "/";
+ try {
+ await crazyGamesSDK.requestMidgameAd();
+ await crazyGamesSDK.gameplayStop();
+ } catch (err) {
+ console.error("SDK error on exit:", err);
+ }
+ window.location.href = "/";
}🤖 Prompt for AI Agents
In `@src/client/graphics/layers/GameRightSidebar.ts` around lines 116 - 128, The
onExitButtonClick handler can throw from crazyGamesSDK.requestMidgameAd() or
crazyGamesSDK.gameplayStop(), preventing the final redirect; wrap those SDK
calls in a try-catch around requestMidgameAd() and gameplayStop() inside
onExitButtonClick, log or console.error any caught error for diagnostics, and
ensure window.location.href = "/" runs in a finally block (or after the catch)
so the redirect always occurs even if the SDK calls fail.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/CrazyGamesSDK.ts (1)
70-84: Critical: Hardcodedreturn truemakes all code unreachable.Line 71 returns
trueunconditionally, making the entire detection logic (lines 72-83) unreachable. This appears to be debug code left in accidentally.This will cause all environments to behave as if running on CrazyGames, potentially breaking non-CrazyGames deployments.
Proposed fix - remove debug return
isOnCrazyGames(): boolean { - return true; try { // Check if we're in an iframe if (window.self !== window.top) {
🤖 Fix all issues with AI agents
In `@src/client/Main.ts`:
- Line 589: The call to crazyGamesSDK.createInviteLink() is missing the required
gameId argument; replace it with crazyGamesSDK.getInviteGameId() if you intend
to retrieve an existing invite (e.g., const lobbyId =
crazyGamesSDK.getInviteGameId()), or if you truly want to create a new invite
call crazyGamesSDK.createInviteLink(gameId) and supply the appropriate gameId
value; update the code path using lobbyId accordingly.
In `@src/client/SinglePlayerModal.ts`:
- Around line 843-844: The call to crazyGamesSDK.requestMidgameAd() can throw or
hang and currently blocks game start; wrap that call in a try/catch and guard it
with a short timeout (e.g., Promise.race with a 3–5s timeout) so failures or
hangs are ignored and the flow continues to start the game; catch and log any
error (or timeout) and ensure the subsequent game-start logic (where
requestMidgameAd() is currently awaited) runs regardless.
♻️ Duplicate comments (6)
src/client/PatternInput.ts (1)
91-94: Verify thatisOnCrazyGames()returns the correct value.This early return hides the pattern input on CrazyGames. The logic here looks fine, but a past review flagged that
isOnCrazyGames()inCrazyGamesSDK.tshas a hard-codedreturn true;that makes it always return true. If that bug exists, this will hide the pattern selector for ALL users, not just CrazyGames users.Please confirm that
isOnCrazyGames()has proper platform detection before merging.#!/bin/bash # Check the implementation of isOnCrazyGames in CrazyGamesSDK.ts rg -n "isOnCrazyGames" -A 15 src/client/CrazyGamesSDK.tssrc/client/graphics/layers/GameRightSidebar.ts (1)
116-128: Add error handling to prevent users getting stuck on exit.If
requestMidgameAd()orgameplayStop()throws, the redirect never happens and user is stuck. Wrap in try-catch to ensure redirect always occurs.Proposed fix
private async onExitButtonClick() { const isAlive = this.game.myPlayer()?.isAlive(); if (isAlive) { const isConfirmed = confirm( translateText("help_modal.exit_confirmation"), ); if (!isConfirmed) return; } - await crazyGamesSDK.requestMidgameAd(); - await crazyGamesSDK.gameplayStop(); - // redirect to the home page - window.location.href = "/"; + try { + await crazyGamesSDK.requestMidgameAd(); + await crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("SDK error on exit:", err); + } + window.location.href = "/"; }src/client/CrazyGamesSDK.ts (3)
131-149: Inconsistent readiness check - uses syncisReady()while others use asyncready().Methods like
getUsername()andisInstantMultiplayer()use asyncready()with timeout. This method uses synchronousisReady(), which may fail if called before SDK initialization completes.Consider using the async pattern:
Proposed fix
-addAuthListener( +async addAuthListener( listener: ( user: { username: string; } | null, ) => void, -): void { - if (!this.isReady()) { +): Promise<void> { + const isReady = await this.ready(); + if (!isReady) { console.warn("CrazyGames SDK not ready, auth listener will not be added"); return; }
151-157: Add try-catch for SDK property access.For consistency with other methods, wrap the SDK access in try-catch:
Proposed fix
async isInstantMultiplayer(): Promise<boolean> { const isReady = await this.ready(); if (!isReady) { return false; } - return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + try { + return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + } catch (error) { + console.error("Failed to check instant multiplayer:", error); + return false; + } }
123-129: Add try-catch for SDK call consistency.Other SDK methods wrap calls in try-catch. This method should follow the same pattern to handle potential SDK errors gracefully.
Proposed fix
async getUsername(): Promise<string | null> { const isReady = await this.ready(); if (!isReady) { return null; } - return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + try { + return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + } catch (error) { + console.error("Failed to get user:", error); + return null; + } }src/client/Main.ts (1)
597-604: Add username validation before opening host modal.The
hostLobbyButtonhandler (lines 486-501) validates the username before proceeding. The instant multiplayer path bypasses this check. Add validation for consistency:Proposed fix
crazyGamesSDK.isInstantMultiplayer().then((isInstant) => { if (isInstant) { console.log( `CrazyGames: joining instant multiplayer lobby from CrazyGames`, ); - this.hostModal.open(); + if (this.usernameInput?.isValid()) { + this.hostModal.open(); + } else { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: this.usernameInput?.validationError || "Invalid username", + color: "red", + duration: 3000, + }, + }), + ); + } } });
🧹 Nitpick comments (6)
src/client/components/DesktopNavBar.ts (1)
128-141: Consider removing redundant class from child element.The
no-crazygamesclass onnav-account-avatar(line 137) seems redundant since its parentnav-account-button(line 130) already has this class. When the parent button is hidden, the avatar inside will be hidden too.Unless there is a use case where the avatar needs to be hidden independently while the button remains visible, you can simplify by removing the class from the avatar.
Suggested simplification
<img id="nav-account-avatar" - class="no-crazygames hidden w-8 h-8 rounded-full object-cover" + class="hidden w-8 h-8 rounded-full object-cover" alt=""src/client/graphics/layers/UnitDisplay.ts (1)
131-134: Looks good - responsive layout update is correct.The Tailwind classes are well-formed:
z-[1100]uses proper arbitrary value syntaxmin-[1200px]:flexis valid custom breakpoint syntaxSmall note:
min-[1200px]:flex-colandxl:flex-colboth set the same value (same forgap-2). Since xl (1280px) is higher than 1200px, thexl:variants are redundant when the values match. You could simplify by removingxl:flex-colandxl:gap-2if the intent is just flex-col from 1200px until 2xl kicks in.This is purely a minor readability thing - the code works correctly as written.
Optional: Remove redundant xl: variants
- class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row xl:flex-col min-[1200px]:flex-col 2xl:gap-5 xl:gap-2 min-[1200px]:gap-2 justify-center items-center" + class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row min-[1200px]:flex-col 2xl:gap-5 min-[1200px]:gap-2 justify-center items-center"src/client/graphics/layers/WinModal.ts (1)
253-253: Consider adding error handling forgameplayStop().If
gameplayStop()throws, the modal content won't load and the modal won't show properly. Since this is a fire-and-forget call, wrapping it in try-catch ensures the win modal always appears.Proposed fix
async show() { - crazyGamesSDK.gameplayStop(); + try { + crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("gameplayStop failed:", err); + } await this.loadPatternContent();src/client/HostLobbyModal.ts (1)
116-121: Add error handling forcreateInviteLink()to ensure fallback works.If
createInviteLink()throws instead of returning null, the method will fail and no URL will be built. Wrap in try-catch to guarantee fallback to standard URL construction.Proposed fix
private async buildLobbyUrl(): Promise<string> { if (crazyGamesSDK.isOnCrazyGames()) { - const link = crazyGamesSDK.createInviteLink(this.lobbyId); - if (link !== null) { - return link; + try { + const link = crazyGamesSDK.createInviteLink(this.lobbyId); + if (link !== null) { + return link; + } + } catch (err) { + console.error("CrazyGames invite link failed:", err); } } const config = await getServerConfigFromClient();src/client/UsernameInput.ts (2)
43-49: Debug log will show "[object Object]" - consider JSON.stringify.Line 44 logs the user object directly in a template literal, which will display as
[object Object]. For useful debug output:- console.log(`got user: ${user}`); + console.log(`got user:`, user);Also, the optional chaining on line 46 (
user?.username) is redundant sinceuseris already truthy inside theif (user)block.
172-185: Fire-and-forget async call creates confusing control flow.This method fires an async promise (lines 173-179) but does not await it. The method immediately continues to the synchronous fallback path (lines 180-184) and returns. The async callback later overwrites
baseUsername, causing the username to change after initial render.The
return nullon line 178 has no effect since nothing awaits this promise.If this "eventual update" behavior is intended, consider extracting it for clarity:
Suggested refactor
private getUsername(): string { - crazyGamesSDK.getUsername().then((username) => { - if (username) { - this.baseUsername = username; - this.requestUpdate(); - } - return null; - }); + // Async fetch from SDK - will update UI if successful + this.fetchUsernameFromSDK(); + const storedUsername = localStorage.getItem(usernameKey); if (storedUsername) { return storedUsername; } return this.generateNewUsername(); } +private fetchUsernameFromSDK(): void { + crazyGamesSDK.getUsername().then((username) => { + if (username) { + this.baseUsername = username; + this.requestUpdate(); + } + }); +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
✅ Files skipped from review due to trivial changes (1)
- src/client/components/PatternButton.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- startup.sh
- src/client/Matchmaking.ts
- src/client/LangSelector.ts
- src/client/components/PlayPage.ts
🧰 Additional context used
🧠 Learnings (13)
📚 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/graphics/layers/SettingsModal.tssrc/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/WinModal.tssrc/client/Main.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/graphics/layers/SettingsModal.tssrc/client/PatternInput.tssrc/client/HostLobbyModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/SinglePlayerModal.tssrc/client/components/DesktopNavBar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/WinModal.tssrc/client/CrazyGamesSDK.tssrc/client/Main.tssrc/client/UsernameInput.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/PatternInput.tssrc/client/CrazyGamesSDK.tssrc/client/Main.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/client/PatternInput.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/PatternInput.tssrc/client/CrazyGamesSDK.tssrc/client/Main.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-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.tssrc/client/Main.ts
📚 Learning: 2025-12-29T23:33:25.733Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:25.733Z
Learning: In Tailwind CSS v4, blur utilities were renamed: `backdrop-blur-sm` from v3 is now `backdrop-blur-xs` in v4, and `backdrop-blur` (bare) becomes `backdrop-blur-sm`. Similarly, other blur utilities like `blur-sm` are renamed to `blur-xs`.
Applied to files:
src/client/graphics/layers/UnitDisplay.ts
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/WinModal.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/client/Main.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 (7)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/HostLobbyModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/SinglePlayerModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/graphics/layers/GameRightSidebar.ts (4)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)src/core/game/GameView.ts (1)
isAlive(470-472)src/core/game/PlayerImpl.ts (1)
isAlive(347-349)
src/client/graphics/layers/WinModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/Main.ts (1)
src/core/Schemas.ts (1)
GAME_ID_REGEX(236-236)
🪛 GitHub Actions: 🧪 CI
src/client/Main.ts
[error] 589-589: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: 🚀 Deploy
src/client/Main.ts
[error] 589-589: TS2554: Expected 1 arguments, but got 0. (Command: npm run build-prod)
🔇 Additional comments (7)
src/client/components/DesktopNavBar.ts (1)
107-111: LGTM!Adding
no-crazygamesto the store button is a clean way to hide platform-specific UI. This follows the same pattern seen across other files in this PR.src/client/SinglePlayerModal.ts (1)
30-30: LGTM!Import of the singleton is correct and consistent with other files in this PR.
src/client/PatternInput.ts (1)
8-8: LGTM!Import is correct and consistent with other files.
src/client/graphics/layers/GameRightSidebar.ts (1)
106-114: LGTM!Good integration of CrazyGames gameplay lifecycle. Stopping gameplay when paused and starting when resumed is the correct pattern for the SDK.
src/client/graphics/layers/SettingsModal.ts (1)
108-117: Looks good - SDK pause/resume integration.The logic correctly calls
gameplayStop()when pausing andgameplayStart()when resuming. The conditional guard ensures we only affect gameplay state when the modal should control pause and the game was not already paused.src/client/Main.ts (1)
915-936: Acceptable workaround for late-rendered components.The multiple
setTimeoutcalls (100ms, 500ms) handle components that render after initial page load. This is a pragmatic approach.Consider adding a comment explaining why multiple delays are needed:
+ // Hide elements immediately hideCrazyGamesElements(); + // Also hide elements after a short delay to catch late-rendered components + // (some Lit components may render asynchronously) setTimeout(hideCrazyGamesElements, 100); setTimeout(hideCrazyGamesElements, 500);src/client/CrazyGamesSDK.ts (1)
300-325: Good callback-to-promise conversion with proper error handling.The method correctly wraps the callback-based SDK API in a Promise, handles all completion paths (adFinished, adError, catch), and always resolves to avoid breaking calling code when ads fail.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/client/Main.ts
Outdated
| // Check if CrazyGames SDK is enabled first (no hash needed in CrazyGames) | ||
| if (crazyGamesSDK.isOnCrazyGames()) { | ||
| const lobbyId = crazyGamesSDK.getInviteGameId(); | ||
| const lobbyId = crazyGamesSDK.createInviteLink(); |
There was a problem hiding this comment.
Build failure: createInviteLink() requires a gameId argument.
Pipeline fails with TS2554: Expected 1 arguments, but got 0. The method signature in CrazyGamesSDK.ts is createInviteLink(gameId: string): string | null.
Looking at the context, this code tries to get an existing invite link rather than create one. You likely want getInviteGameId() instead:
Proposed fix
- const lobbyId = crazyGamesSDK.createInviteLink();
+ const lobbyId = crazyGamesSDK.getInviteGameId();🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 589-589: TS2554: Expected 1 arguments, but got 0.
🪛 GitHub Actions: 🚀 Deploy
[error] 589-589: TS2554: Expected 1 arguments, but got 0. (Command: npm run build-prod)
🤖 Prompt for AI Agents
In `@src/client/Main.ts` at line 589, The call to crazyGamesSDK.createInviteLink()
is missing the required gameId argument; replace it with
crazyGamesSDK.getInviteGameId() if you intend to retrieve an existing invite
(e.g., const lobbyId = crazyGamesSDK.getInviteGameId()), or if you truly want to
create a new invite call crazyGamesSDK.createInviteLink(gameId) and supply the
appropriate gameId value; update the code path using lobbyId accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/CrazyGamesSDK.ts (1)
70-84: Critical:return true;makes all detection logic unreachable.Line 71 returns
trueunconditionally. The iframe detection logic (lines 72-83) is dead code and will never execute. This meansisOnCrazyGames()always returnstrueeven when running outside the CrazyGames platform.This causes:
- CrazyGames SDK initialization on all platforms
- Platform-specific UI hiding everywhere
- Potential runtime errors when SDK methods are called on non-CrazyGames hosts
Fix: Remove the hardcoded return
isOnCrazyGames(): boolean { - return true; try { // Check if we're in an iframe if (window.self !== window.top) { // Try to access parent URL return window?.top?.location?.hostname.includes("crazygames") ?? false; } return false; } catch (e) { // If we get a cross-origin error, we're definitely iframed // Check our own referrer as fallback return document.referrer.includes("crazygames"); } }
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 3-4: Replace the absolute-style imports in SettingsModal.ts for
crazyGamesSDK and PauseGameIntentEvent with relative paths; change import from
"src/client/CrazyGamesSDK" to "../../CrazyGamesSDK" and from
"src/client/Transport" to "../../Transport" so they match the file's existing
relative import style and project conventions, updating the import statements
that reference crazyGamesSDK and PauseGameIntentEvent accordingly.
♻️ Duplicate comments (7)
src/client/SinglePlayerModal.ts (1)
843-844: Add error handling to prevent blocking game start if ad fails.If
requestMidgameAd()throws or hangs, users cannot start their game. Wrap this in try-catch so the game starts regardless of ad outcome.Proposed fix
- await crazyGamesSDK.requestMidgameAd(); - + try { + await crazyGamesSDK.requestMidgameAd(); + } catch (err) { + console.error("Midgame ad request failed:", err); + }src/client/graphics/layers/GameRightSidebar.ts (1)
116-128: Add error handling to prevent users getting stuck on exit.If
requestMidgameAd()orgameplayStop()throws, the redirect never happens and users are stuck. Wrap SDK calls in try-catch so redirect always runs.Proposed fix
private async onExitButtonClick() { const isAlive = this.game.myPlayer()?.isAlive(); if (isAlive) { const isConfirmed = confirm( translateText("help_modal.exit_confirmation"), ); if (!isConfirmed) return; } - await crazyGamesSDK.requestMidgameAd(); - await crazyGamesSDK.gameplayStop(); - // redirect to the home page - window.location.href = "/"; + try { + await crazyGamesSDK.requestMidgameAd(); + await crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("SDK error on exit:", err); + } + window.location.href = "/"; }src/client/Matchmaking.ts (1)
252-259: Missing cleanup for event listener causes memory leak.The
userMeResponseevent listener is added inconnectedCallbackbut never removed indisconnectedCallback. If this component is removed and re-added to the DOM, listeners stack up.Proposed fix
+ private handleUserMeResponse = (event: Event) => { + const customEvent = event as CustomEvent; + if (customEvent.detail) { + const userMeResponse = customEvent.detail as UserMeResponse | false; + this.isLoggedIn = hasLinkedAccount(userMeResponse); + } + }; async connectedCallback() { super.connectedCallback(); - // Listen for user authentication changes - document.addEventListener("userMeResponse", (event: Event) => { - const customEvent = event as CustomEvent; - if (customEvent.detail) { - const userMeResponse = customEvent.detail as UserMeResponse | false; - this.isLoggedIn = hasLinkedAccount(userMeResponse); - } - }); + document.addEventListener("userMeResponse", this.handleUserMeResponse); } + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this.handleUserMeResponse); + }src/client/Main.ts (1)
597-604: Username validation still missing for instant multiplayer flow.The
hostLobbyButtonhandler (lines 486-501) validates the username before proceeding. This instant multiplayer path callsthis.hostModal.open()directly without the same check, allowing users to create a lobby with an invalid or empty username.Suggested fix
crazyGamesSDK.isInstantMultiplayer().then((isInstant) => { if (isInstant) { console.log( `CrazyGames: joining instant multiplayer lobby from CrazyGames`, ); - this.hostModal.open(); + if (this.usernameInput?.isValid()) { + this.hostModal.open(); + } else { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: this.usernameInput?.validationError, + color: "red", + duration: 3000, + }, + }), + ); + } } });src/client/CrazyGamesSDK.ts (3)
123-129: Missing try-catch around SDK call.If
window.CrazyGames!.SDK.user.getUser()throws, the error propagates unhandled. Other methods in this class wrap SDK calls in try-catch blocks for consistency.Suggested fix
async getUsername(): Promise<string | null> { const isReady = await this.ready(); if (!isReady) { return null; } - return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + try { + return (await window.CrazyGames!.SDK.user.getUser())?.username ?? null; + } catch (error) { + console.error("Failed to get user:", error); + return null; + } }
131-149: Inconsistent readiness check.This method uses synchronous
isReady()(line 138) whilegetUsername()andisInstantMultiplayer()use asyncready()with timeout. IfaddAuthListeneris called during initialization, it silently fails even though the SDK might become ready shortly after.Suggested fix for consistency
- addAuthListener( + async addAuthListener( listener: ( user: { username: string; } | null, ) => void, - ): void { - if (!this.isReady()) { + ): Promise<void> { + const isReady = await this.ready(); + if (!isReady) { console.warn("CrazyGames SDK not ready, auth listener will not be added"); return; }
151-157: Missing try-catch for consistency.Same pattern issue as
getUsername(). Wrap the SDK access in try-catch to match other methods.Suggested fix
async isInstantMultiplayer(): Promise<boolean> { const isReady = await this.ready(); if (!isReady) { return false; } - return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + try { + return window.CrazyGames!.SDK.game.isInstantMultiplayer ?? false; + } catch (error) { + console.error("Failed to check instant multiplayer:", error); + return false; + } }
🧹 Nitpick comments (2)
src/client/UsernameInput.ts (1)
172-185: Simplify async username retrieval pattern.The fire-and-forget
.then()returningnullis confusing. The return value is ignored anyway. Consider usingvoidoperator or just omitting the return.Also,
user?.usernameon line 46 is redundant sinceuseris already truthy from theif (user)check.Proposed fix
private getUsername(): string { - crazyGamesSDK.getUsername().then((username) => { + void crazyGamesSDK.getUsername().then((username) => { if (username) { this.baseUsername = username; this.requestUpdate(); } - return null; }); const storedUsername = localStorage.getItem(usernameKey); if (storedUsername) { return storedUsername; } return this.generateNewUsername(); }src/client/Matchmaking.ts (1)
307-325: Use proper types instead ofany.Using
anyforusernameInputandpublicLobbybypasses type safety. Use the actual types or create a minimal interface.Proposed fix
+import { UsernameInput } from "./UsernameInput"; + +// If PublicLobby type is not exported, create a minimal interface: +interface LobbyComponent { + leaveLobby(): void; +} private handleLoggedInClick() { - const usernameInput = document.querySelector("username-input") as any; - const publicLobby = document.querySelector("public-lobby") as any; + const usernameInput = document.querySelector("username-input") as UsernameInput | null; + const publicLobby = document.querySelector("public-lobby") as LobbyComponent | null; if (usernameInput?.isValid()) { this.open(); publicLobby?.leaveLobby(); } else { window.dispatchEvent( new CustomEvent("show-message", { detail: { message: usernameInput?.validationError, color: "red", duration: 3000, }, }), ); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- src/client/PatternInput.ts
- src/client/components/PatternButton.ts
- src/client/graphics/layers/WinModal.ts
- src/client/HostLobbyModal.ts
- src/client/graphics/layers/UnitDisplay.ts
- startup.sh
- src/client/LangSelector.ts
- src/client/components/DesktopNavBar.ts
- src/client/components/PlayPage.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/UsernameInput.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/client/Matchmaking.tssrc/client/CrazyGamesSDK.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/graphics/layers/GameRightSidebar.tssrc/client/Matchmaking.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/GameRightSidebar.tssrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/client/Matchmaking.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.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/client/Main.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/Main.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
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/Main.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Matchmaking.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/client/CrazyGamesSDK.ts
🧬 Code graph analysis (4)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/Matchmaking.ts (4)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/LangSelector.ts (1)
translateText(277-301)
src/client/SinglePlayerModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
⏰ 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/client/SinglePlayerModal.ts (1)
30-30: LGTM!Import of
crazyGamesSDKis clean and matches the pattern used across other files in this PR.src/client/graphics/layers/GameRightSidebar.ts (1)
108-112: LGTM!Clean toggle logic for gameplay start/stop tied to pause state.
src/client/graphics/layers/SettingsModal.ts (1)
109-116: LGTM!The conditional logic correctly avoids double-pausing when the game was already paused before the modal opened. Clean approach using composition of SDK calls with event emission.
src/client/Matchmaking.ts (2)
267-270: LGTM!Clean early return guard for CrazyGames platform.
327-329: LGTM!Clean navigation handler with defensive optional chaining on
window.showPage.src/client/Main.ts (2)
217-217: Field declaration looks correct now.The
hostModalfield is assigned at line 478 before use. TypeScript's strict mode should catch any issues if the DOM query fails. The check at line 481 logs a warning if the element is missing.
915-936: Reasonable approach for hiding platform-specific elements.The delayed calls at 100ms and 500ms help catch late-rendered components. This pattern works for most cases.
One edge case: components rendered after 500ms (lazy-loaded routes, dynamic modals) might slip through. If that becomes a problem later, a
MutationObserverwatching for new.no-crazygameselements would be more robust. For now, this is fine.src/client/CrazyGamesSDK.ts (3)
51-68: Clean readiness pattern.The promise-based readiness with timeout is a good approach. The 3-second timeout balances waiting for slow SDK loads against hanging forever. The
resolveReady!definite assignment works because the constructor always sets it.
269-283: Well-structured invite link creation.Good error handling with try-catch, proper null return on failure, and helpful logging. Follows the same pattern as other methods.
300-325: Clean ad request implementation.The promise-based approach properly resolves in all cases: success (
adFinished), failure (adError), not ready, and exceptions. This prevents hanging promises and lets callers continue regardless of ad outcome.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import { crazyGamesSDK } from "src/client/CrazyGamesSDK"; | ||
| import { PauseGameIntentEvent } from "src/client/Transport"; |
There was a problem hiding this comment.
Use relative import paths for consistency.
These imports use absolute-style paths (src/client/...) while the rest of the file uses relative paths (../../../core/...). Use relative paths for consistency.
Proposed fix
-import { crazyGamesSDK } from "src/client/CrazyGamesSDK";
-import { PauseGameIntentEvent } from "src/client/Transport";
+import { crazyGamesSDK } from "../../CrazyGamesSDK";
+import { PauseGameIntentEvent } from "../../Transport";📝 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.
| import { crazyGamesSDK } from "src/client/CrazyGamesSDK"; | |
| import { PauseGameIntentEvent } from "src/client/Transport"; | |
| import { crazyGamesSDK } from "../../CrazyGamesSDK"; | |
| import { PauseGameIntentEvent } from "../../Transport"; |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/SettingsModal.ts` around lines 3 - 4, Replace the
absolute-style imports in SettingsModal.ts for crazyGamesSDK and
PauseGameIntentEvent with relative paths; change import from
"src/client/CrazyGamesSDK" to "../../CrazyGamesSDK" and from
"src/client/Transport" to "../../Transport" so they match the file's existing
relative import style and project conventions, updating the import statements
that reference crazyGamesSDK and PauseGameIntentEvent accordingly.
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 (3)
src/client/components/DesktopNavBar.ts (1)
128-141: Removeno-crazygamesfrom the avatar image (line 137).The avatar already has the
hiddenclass, which always setsdisplay: none. Theno-crazygamesclass is for JavaScript hiding on CrazyGames platform, but it has no effect on an element that's already hidden. Remove it to keep the code clean:Suggested change
<img id="nav-account-avatar" - class="no-crazygames hidden w-8 h-8 rounded-full object-cover" + class="hidden w-8 h-8 rounded-full object-cover" alt="" data-i18n-alt="main.discord_avatar_alt" referrerpolicy="no-referrer" />src/client/CrazyGamesSDK.ts (1)
70-84: Critical:return truemakes rest of function unreachable.Line 71 unconditionally returns
true, so the iframe detection logic (lines 72-84) never runs. This looks like debug code that was accidentally left in.🐛 Proposed fix: remove the debug return statement
isOnCrazyGames(): boolean { - return true; try { // Check if we're in an iframe if (window.self !== window.top) {src/client/UsernameInput.ts (1)
172-185: Fire-and-forget async call creates race condition.The
getUsername()method starts an async fetch but immediately returns the localStorage value. The async result arrives later and updatesbaseUsername, potentially overwriting what the user already typed.Issues:
return null;inside.then()does nothing useful- No error handling for the promise
- Race condition between SDK response and user input
Proposed fix using proper async flow
- private getUsername(): string { - crazyGamesSDK.getUsername().then((username) => { - if (username) { - this.baseUsername = username; - this.requestUpdate(); - } - return null; - }); - const storedUsername = localStorage.getItem(usernameKey); - if (storedUsername) { - return storedUsername; - } - return this.generateNewUsername(); - } + private getUsername(): string { + // Async fetch from SDK - will update UI if successful + crazyGamesSDK.getUsername().then((username) => { + if (username && this.baseUsername === this.getStoredOrGeneratedUsername()) { + // Only override if user hasn't modified the username + this.parseAndSetUsername(username); + this.storeUsername(username); + } + }).catch((err) => { + console.warn("Failed to fetch CrazyGames username:", err); + }); + + return this.getStoredOrGeneratedUsername(); + } + + private getStoredOrGeneratedUsername(): string { + const storedUsername = localStorage.getItem(usernameKey); + if (storedUsername) { + return storedUsername; + } + return this.generateNewUsername(); + }This fix:
- Adds error handling
- Only updates if user hasn't changed the default username
- Properly stores the SDK username
♻️ Duplicate comments (9)
startup.sh (1)
88-92: Redundant conditional - both branches are identical.This conditional no longer serves a purpose since both branches execute the same command.
src/client/graphics/layers/SettingsModal.ts (1)
3-4: Use relative import paths for consistency.These imports use absolute-style paths while the rest of the file uses relative paths.
src/client/CrazyGamesSDK.ts (2)
123-129: Missing try-catch around SDK call.If
window.CrazyGames!.SDK.user.getUser()throws, the error propagates unhandled. Other methods in this class wrap SDK calls in try-catch blocks.
151-157: Same missing try-catch pattern.For consistency with other SDK methods, wrap the SDK access in try-catch.
src/client/Main.ts (2)
217-217: Use definite assignment assertion forhostModal.The field is assigned in
initialize()but TypeScript doesn't know this. Add!to indicate definite assignment.Proposed fix
- private hostModal: HostPrivateLobbyModal; + private hostModal!: HostPrivateLobbyModal;
597-604: Missing username validation before opening host modal.This issue was already flagged. The
hostLobbyButtonhandler (lines 486-501) validates the username before proceeding, but the instant multiplayer path skips this check. Add the same validation here.src/client/Matchmaking.ts (1)
252-259: Memory leak: Event listener never removed.This issue was already flagged. The
userMeResponselistener added here is never cleaned up indisconnectedCallback. Store the handler as a class property and remove it when the component disconnects.src/client/UsernameInput.ts (1)
43-49: Memory leak: Auth listener cannot be removed.This was already flagged. The CrazyGames SDK lacks a
removeAuthListenermethod, so this listener will stack if the component remounts.Also, line 46 has redundant optional chaining:
user?.usernameshould beuser.usernamesinceuseris already checked for truthiness on line 45.src/client/PatternInput.ts (1)
91-94: Remove the hardcodedreturn true;fromisOnCrazyGames()in CrazyGamesSDK.ts.The
isOnCrazyGames()method has a hardcodedreturn true;at the start that makes all platform detection code unreachable. This causes the pattern input to hide on ALL platforms, not just CrazyGames. Delete the hardcoded return statement and let the platform detection logic execute properly.Current buggy code:
isOnCrazyGames(): boolean { return true; try { // Check if we're in an iframe if (window.self !== window.top) { // Try to access parent URL return window?.top?.location?.hostname.includes("crazygames") ?? false; } return false; } catch (e) { // If we get a cross-origin error, we're definitely iframed
🧹 Nitpick comments (3)
src/client/graphics/layers/UnitDisplay.ts (1)
131-134: CSS changes look valid, but there is slight redundancy.The Tailwind v4 syntax is correct:
z-[1100]uses proper arbitrary value brackets.min-[1200px]:flexis valid arbitrary breakpoint syntax.However,
xl:flex-col(1280px) andmin-[1200px]:flex-coloverlap since 1280px > 1200px. Same goes forxl:gap-2andmin-[1200px]:gap-2. You can simplify by removing thexl:variants if the only goal is to show the component at 1200px+.♻️ Suggested simplification
- class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row xl:flex-col min-[1200px]:flex-col 2xl:gap-5 xl:gap-2 min-[1200px]:gap-2 justify-center items-center" + class="hidden min-[1200px]:flex fixed bottom-4 left-1/2 transform -translate-x-1/2 z-[1100] 2xl:flex-row min-[1200px]:flex-col 2xl:gap-5 min-[1200px]:gap-2 justify-center items-center"src/client/Matchmaking.ts (1)
307-325: Avoidas anycasts; use proper types instead.Casting DOM queries to
anybypasses TypeScript's type checking. Define an interface or use the actual component types for better safety.Proposed fix
private handleLoggedInClick() { - const usernameInput = document.querySelector("username-input") as any; - const publicLobby = document.querySelector("public-lobby") as any; + const usernameInput = document.querySelector("username-input") as UsernameInput | null; + const publicLobby = document.querySelector("public-lobby") as PublicLobby | null; - if (usernameInput?.isValid()) { + if (usernameInput && usernameInput.isValid()) { this.open(); publicLobby?.leaveLobby(); } else { window.dispatchEvent( new CustomEvent("show-message", { detail: { - message: usernameInput?.validationError, + message: usernameInput?.validationError ?? "", color: "red", duration: 3000, }, }), ); } }You'll need to import
UsernameInputandPublicLobbyat the top of the file.src/client/Main.ts (1)
929-935: MultiplesetTimeoutcalls are fragile; consider a single observer.Using hardcoded delays (100ms, 500ms) to catch "late-rendered components" is unreliable. Components might render at different times depending on network or device speed. A
MutationObserverwould be more robust.Alternative using MutationObserver
// In bootstrap or initialize if (crazyGamesSDK.isOnCrazyGames()) { hideCrazyGamesElements(); const observer = new MutationObserver(() => { hideCrazyGamesElements(); }); observer.observe(document.body, { childList: true, subtree: true }); // Stop observing after reasonable time setTimeout(() => observer.disconnect(), 5000); }The current approach works for now but may miss components that render after 500ms.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/components/PatternButton.ts
- src/client/graphics/layers/WinModal.ts
- src/client/SinglePlayerModal.ts
- src/client/LangSelector.ts
- src/client/graphics/layers/GameRightSidebar.ts
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/PatternInput.tssrc/client/Main.tssrc/client/CrazyGamesSDK.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/client/PatternInput.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/PatternInput.tssrc/client/UsernameInput.tssrc/client/Main.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/PatternInput.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/HostLobbyModal.tssrc/client/CrazyGamesSDK.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/SettingsModal.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/UsernameInput.tssrc/client/Matchmaking.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/components/DesktopNavBar.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/HostLobbyModal.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/SettingsModal.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/client/Main.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/Main.tssrc/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
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/Matchmaking.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-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/graphics/layers/SettingsModal.ts
🧬 Code graph analysis (6)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/Main.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/Matchmaking.ts (4)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/Utils.ts (1)
translateText(165-224)
src/client/HostLobbyModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(328-328)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
⏰ 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/components/DesktopNavBar.ts (1)
107-111: LGTM!Clean addition of the
no-crazygamesmarker class to hide the store button when running on CrazyGames. This follows the pattern seen in other components.src/client/graphics/layers/SettingsModal.ts (1)
108-117: Logic looks correct for conditional pause handling.The SDK calls (
gameplayStop/gameplayStart) are properly guarded by theshouldPause && !wasPausedWhenOpenedcondition. The event emission follows the SDK state change - good ordering.src/client/CrazyGamesSDK.ts (2)
48-68: Readiness pattern is clean and well-structured.The
Promise.raceapproach with a 3-second timeout is a solid pattern. The constructor initializes the deferred promise correctly, andready()provides a safe way for consumers to wait for SDK initialization.
300-325: requestMidgameAd handles all callback paths correctly.The promise resolves on
adFinished,adError, and synchronous exceptions - good defensive coding. No hanging promises.src/client/HostLobbyModal.ts (2)
115-124: CrazyGames invite link path looks good.The fallback logic is correct: try SDK invite link first, fall back to standard URL construction if null. Note this depends on the
isOnCrazyGames()fix in CrazyGamesSDK.ts - once that debugreturn trueis removed, this will correctly gate the SDK path.
131-135: Correct: skip history changes when embedded in CrazyGames.When running inside the CrazyGames iframe, the parent controls navigation. Skipping
history.replaceStateavoids conflicts with the host platform.src/client/components/PlayPage.ts (1)
139-142: Clean component extraction.Replacing the button blocks with a single
<matchmaking-button>component keeps the template lean and moves state management (login checks, modal handling) to the right place. The component is properly registered and handles both logged-in and logged-out states with appropriate callbacks.src/client/Matchmaking.ts (3)
6-14: Imports look good.The new imports for
hasLinkedAccountandcrazyGamesSDKare correctly added and used throughout the component.
267-270: Early return for CrazyGames is appropriate.This pattern matches the approach used in other components (e.g.,
PatternInput.ts). Returning an empty template when on CrazyGames keeps the UI clean.
327-329: Consider null check forwindow.showPage.
window.showPageis optional (showPage?: ...). The optional chaining is correct, but the function silently does nothing ifshowPageis undefined. This is fine if intended, just noting for awareness.src/client/Main.ts (2)
478-483: Initialization looks correct.The host modal is properly queried and assigned. The warning for missing element is helpful for debugging.
915-922: Simple and effective hide function.The function clearly hides elements marked with
.no-crazygamesclass when on the CrazyGames platform.src/client/PatternInput.ts (1)
8-8: Import is correctly added.The
crazyGamesSDKsingleton import follows the established pattern.src/client/UsernameInput.ts (1)
11-11: Import is correctly added.The SDK singleton import follows the project pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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/UsernameInput.ts (1)
172-185: Logic issue: SDK username bypasses parseAndSetUsername.The async SDK username update (line 175) sets
baseUsernamedirectly, but theconnectedCallbackusesparseAndSetUsername(line 51) which handles clan tags. This means if the SDK returns a username with a clan tag like[TAG] Player, it won't be parsed correctly.Also,
return nullon line 178 is dead code since the return value is never used.Proposed fix
private getUsername(): string { crazyGamesSDK.getUsername().then((username) => { if (username) { - this.baseUsername = username; + this.parseAndSetUsername(username); - this.requestUpdate(); } - return null; }); const storedUsername = localStorage.getItem(usernameKey); if (storedUsername) { return storedUsername; } return this.generateNewUsername(); }
🤖 Fix all issues with AI agents
In `@src/client/UsernameInput.ts`:
- Around line 41-52: The async SDK path, localStorage path and auth-listener
path set baseUsername inconsistently causing skipped clan-tag parsing and
validation; consolidate into a single setter method (e.g., create/rename to
setParsedUsername or normalizeUsername) and update all call sites
(connectedCallback’s stored flow, crazyGamesSDK.getCurrentUser()/async callback,
and the auth listener) to call that method instead of assigning baseUsername
directly or calling inconsistent helpers; ensure this central method runs
clan-tag extraction, validation, sets baseUsername, updates any derived fields,
and calls requestUpdate() to avoid multiple renders/flicker.
♻️ Duplicate comments (4)
src/client/graphics/layers/GameRightSidebar.ts (1)
116-128: Add error handling to prevent users getting stuck on exit.If
requestMidgameAd()orgameplayStop()throws, the redirect never happens and the user is stuck. Wrap in try-catch to ensure the redirect always runs.Proposed fix
private async onExitButtonClick() { const isAlive = this.game.myPlayer()?.isAlive(); if (isAlive) { const isConfirmed = confirm( translateText("help_modal.exit_confirmation"), ); if (!isConfirmed) return; } - await crazyGamesSDK.requestMidgameAd(); - await crazyGamesSDK.gameplayStop(); - // redirect to the home page - window.location.href = "/"; + try { + await crazyGamesSDK.requestMidgameAd(); + await crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("SDK error on exit:", err); + } + window.location.href = "/"; }src/client/Matchmaking.ts (1)
252-259: Missing cleanup for event listener causes memory leak.This event listener is added in
connectedCallbackbut never removed. If the component is removed and re-added to the DOM, listeners will stack up.Proposed fix
+ private boundUserMeHandler = (event: Event) => { + const customEvent = event as CustomEvent; + if (customEvent.detail) { + const userMeResponse = customEvent.detail as UserMeResponse | false; + this.isLoggedIn = hasLinkedAccount(userMeResponse); + } + }; async connectedCallback() { super.connectedCallback(); - // Listen for user authentication changes - document.addEventListener("userMeResponse", (event: Event) => { - const customEvent = event as CustomEvent; - if (customEvent.detail) { - const userMeResponse = customEvent.detail as UserMeResponse | false; - this.isLoggedIn = hasLinkedAccount(userMeResponse); - } - }); + document.addEventListener("userMeResponse", this.boundUserMeHandler); } + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this.boundUserMeHandler); + }src/client/Main.ts (1)
597-604: Add username validation before opening the host modal.The
hostLobbyButtonhandler (lines 486-501) validates the username before showing the host page. The instant multiplayer path callsthis.hostModal.open()without this check, which could allow creating a lobby with an invalid username.Proposed fix
crazyGamesSDK.isInstantMultiplayer().then((isInstant) => { if (isInstant) { console.log( `CrazyGames: joining instant multiplayer lobby from CrazyGames`, ); - this.hostModal.open(); + if (this.usernameInput?.isValid()) { + this.hostModal.open(); + } else { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: this.usernameInput?.validationError, + color: "red", + duration: 3000, + }, + }), + ); + } } });src/client/UsernameInput.ts (1)
43-49: Memory leak: Auth listener cannot be removed due to SDK limitations.The auth listener added here is never cleaned up. The CrazyGames SDK does not expose a
removeAuthListenermethod.Also, on line 46, the optional chaining
user?.usernameis redundant sinceuseris already checked for truthiness on line 45.Fix redundant optional chaining
crazyGamesSDK.addAuthListener((user) => { console.log(`got user: ${user}`); if (user) { - this.parseAndSetUsername(user?.username); + this.parseAndSetUsername(user.username); } this.requestUpdate(); });
🧹 Nitpick comments (3)
src/client/CrazyGamesSDK.ts (1)
277-291: Consider using asyncready()for consistency.
createInviteLink()uses syncisReady()whilegetUsername()andisInstantMultiplayer()use asyncready(). If called early during initialization, this method will returnnulleven though the SDK might become ready shortly after.If this is intentional (because invite links are only created after game starts), this is fine. Otherwise, consider making it async.
src/client/Matchmaking.ts (1)
307-325: Consider using proper types instead ofas anycasts.The
as anycasts work but lose type safety. IfUsernameInputandPublicLobbyare exported types, import them for better type checking.Suggested improvement
+import { UsernameInput } from "./UsernameInput"; +import { PublicLobby } from "./PublicLobby"; private handleLoggedInClick() { - const usernameInput = document.querySelector("username-input") as any; - const publicLobby = document.querySelector("public-lobby") as any; + const usernameInput = document.querySelector("username-input") as UsernameInput | null; + const publicLobby = document.querySelector("public-lobby") as PublicLobby | null; if (usernameInput?.isValid()) {src/client/Main.ts (1)
915-936: Timeout-based hiding is a reasonable workaround.The multiple
setTimeoutcalls handle late-rendered components. This works but is fragile if components render after 500ms. Consider using aMutationObserverfor a more robust solution if this becomes unreliable.Alternative with MutationObserver
const hideCrazyGamesElements = () => { if (!crazyGamesSDK.isOnCrazyGames()) return; document.querySelectorAll(".no-crazygames").forEach((el) => { (el as HTMLElement).style.display = "none"; }); }; // Run once immediately hideCrazyGamesElements(); // Watch for new elements if (crazyGamesSDK.isOnCrazyGames()) { const observer = new MutationObserver((mutations) => { for (const mutation of mutations) { mutation.addedNodes.forEach((node) => { if (node instanceof HTMLElement) { if (node.classList.contains("no-crazygames")) { node.style.display = "none"; } node.querySelectorAll(".no-crazygames").forEach((el) => { (el as HTMLElement).style.display = "none"; }); } }); } }); observer.observe(document.body, { childList: true, subtree: true }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.tsstartup.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- startup.sh
- src/client/graphics/layers/SettingsModal.ts
- src/client/SinglePlayerModal.ts
- src/client/components/PlayPage.ts
- src/client/HostLobbyModal.ts
- src/client/components/PatternButton.ts
- src/client/graphics/layers/WinModal.ts
- src/client/graphics/layers/UnitDisplay.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
📚 Learning: 2025-07-23T12:36:35.354Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1534
File: src/client/LangSelector.ts:97-106
Timestamp: 2025-07-23T12:36:35.354Z
Learning: In OpenFrontIO's LangSelector.ts, the getClosestSupportedLang method always joins language code parts with underscores ("_") because all keys in the languageMap use underscore format (e.g., pt_BR, sv_SE, zh_CN). This normalization ensures consistency regardless of whether the input language code uses hyphens or underscores as delimiters.
Applied to files:
src/client/LangSelector.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/LangSelector.tssrc/client/Main.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/components/DesktopNavBar.tssrc/client/Matchmaking.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/LangSelector.tssrc/client/PatternInput.tssrc/client/Main.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/components/DesktopNavBar.tssrc/client/UsernameInput.tssrc/client/Matchmaking.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/PatternInput.tssrc/client/Main.tssrc/client/CrazyGamesSDK.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/client/PatternInput.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/PatternInput.tssrc/client/Main.tssrc/client/UsernameInput.tssrc/client/CrazyGamesSDK.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/client/Main.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/Main.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
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/graphics/layers/GameRightSidebar.tssrc/client/Matchmaking.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/UsernameInput.tssrc/client/Matchmaking.ts
🧬 Code graph analysis (5)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/Main.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/graphics/layers/GameRightSidebar.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/Matchmaking.ts (4)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/LangSelector.ts (1)
translateText(278-302)
🔇 Additional comments (14)
src/client/components/DesktopNavBar.ts (1)
108-108: LGTM! Clean styling hooks for platform-specific UI.The
no-crazygamesclass additions provide a consistent way to hide store and account elements when running on the CrazyGames platform. This approach keeps the render logic simple and delegates visibility to CSS.Also applies to: 130-130, 137-137
src/client/LangSelector.ts (1)
231-231: LGTM! Correct addition for translation support.Adding
"matchmaking-button"to the components list ensures it receivesrequestUpdate()calls when the language changes. This keeps translation behavior consistent with other components.src/client/graphics/layers/GameRightSidebar.ts (1)
106-114: LGTM! SDK integration for pause/resume is correct.The gameplay state tracking via
crazyGamesSDK.gameplayStop()andgameplayStart()aligns with the pause toggle. Fire-and-forget calls are acceptable here since the pause event emission should not wait for SDK acknowledgment.src/client/CrazyGamesSDK.ts (3)
48-68: LGTM! Clean readiness mechanism with timeout.The
readyPromisepattern with a 3-second timeout inready()is a good approach. It prevents indefinite waits while giving the SDK time to initialize.
122-165: LGTM! Async methods with proper error handling.
getUsername(),addAuthListener(), andisInstantMultiplayer()all use the asyncready()pattern with try-catch blocks. This addresses the previous review feedback.
308-333: Good callback-to-Promise wrapper.The
requestMidgameAd()implementation correctly wraps the callback-based SDK API in a Promise. It resolves on bothadFinishedandadError, ensuring callers are never left waiting.One note: this uses sync
isReady()likecreateInviteLink(). Same optional consideration applies for consistency.src/client/PatternInput.ts (2)
91-94: LGTM! Clean platform check for conditional rendering.The early return when on CrazyGames hides the pattern customization UI on that platform. The
isOnCrazyGames()method properly checks iframe context and referrer.
8-8: Verify the import is used correctly.The import adds a dependency on the SDK singleton. This is fine since the component needs to check the platform.
src/client/Matchmaking.ts (3)
6-6: Imports look good.The
hasLinkedAccountutility andcrazyGamesSDKsingleton are correctly imported for the new authentication-aware rendering logic.Also applies to: 12-12
266-304: Render logic is clear and well-structured.The conditional rendering follows a clean pattern: first check for CrazyGames platform, then branch based on login state. Both paths include the
matchmaking-modalcomponent which is correct.
327-329: Handler is simple and correct.The
handleLoggedOutClickmethod correctly navigates to the account page using the globalshowPagefunction.src/client/Main.ts (2)
217-217: Field declaration looks good.The
hostModalfield type is correctly declared. Based on past review, this was addressed.
478-483: Host modal initialization is correct.The initialization properly queries the DOM and validates the element type. The warning helps with debugging if the element is missing.
src/client/UsernameInput.ts (1)
11-11: Import is correct.The
crazyGamesSDKsingleton is properly imported.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/Matchmaking.ts`:
- Around line 307-325: In handleLoggedInClick, guard against
document.querySelector("username-input") returning null by explicitly checking
usernameInput before calling isValid or reading validationError; if
usernameInput is null, dispatch the "show-message" event with a clear fallback
message like "Username input not found" (or a generic validation prompt) and
return; otherwise proceed with the existing logic (call usernameInput.isValid(),
this.open(), and publicLobby?.leaveLobby()) and use
usernameInput.validationError when showing the error.
♻️ Duplicate comments (4)
src/client/graphics/layers/GameRightSidebar.ts (1)
116-128: Add error handling to prevent users getting stuck on exit.If
requestMidgameAd()orgameplayStop()throws, the redirect never happens and the user is stuck. Wrap SDK calls in try-catch to ensure the redirect always runs.Proposed fix
private async onExitButtonClick() { const isAlive = this.game.myPlayer()?.isAlive(); if (isAlive) { const isConfirmed = confirm( translateText("help_modal.exit_confirmation"), ); if (!isConfirmed) return; } - await crazyGamesSDK.requestMidgameAd(); - await crazyGamesSDK.gameplayStop(); + try { + await crazyGamesSDK.requestMidgameAd(); + await crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("SDK error on exit:", err); + } // redirect to the home page window.location.href = "/"; }src/client/Matchmaking.ts (1)
252-259: Missing cleanup for event listener causes memory leak.The
userMeResponseevent listener is added inconnectedCallbackbut never removed indisconnectedCallback. If this component is removed and re-added to the DOM, listeners will stack up.Proposed fix
+ private boundUserMeHandler = (event: Event) => { + const customEvent = event as CustomEvent; + if (customEvent.detail) { + const userMeResponse = customEvent.detail as UserMeResponse | false; + this.isLoggedIn = hasLinkedAccount(userMeResponse); + } + }; async connectedCallback() { super.connectedCallback(); - // Listen for user authentication changes - document.addEventListener("userMeResponse", (event: Event) => { - const customEvent = event as CustomEvent; - if (customEvent.detail) { - const userMeResponse = customEvent.detail as UserMeResponse | false; - this.isLoggedIn = hasLinkedAccount(userMeResponse); - } - }); + document.addEventListener("userMeResponse", this.boundUserMeHandler); } + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this.boundUserMeHandler); + }src/client/Main.ts (2)
217-217: FieldhostModalmay be undefined at runtime.The field
private hostModal: HostPrivateLobbyModalis declared without initialization. If the DOM query at line 478 fails,this.hostModalwill beundefined, but TypeScript won't catch this.Use the definite assignment assertion
!if you're certain the element exists, or handle the null case.Proposed fix
- private hostModal: HostPrivateLobbyModal; + private hostModal!: HostPrivateLobbyModal;
597-604: Add username validation before opening the host modal.The
hostLobbyButtonhandler (lines 486-501) validates the username before opening. The instant multiplayer path callshostModal.open()without this check, which could create a lobby with an invalid username.Proposed fix
crazyGamesSDK.isInstantMultiplayer().then((isInstant) => { if (isInstant) { console.log( `CrazyGames: joining instant multiplayer lobby from CrazyGames`, ); - this.hostModal.open(); + if (this.usernameInput?.isValid()) { + this.hostModal.open(); + } else { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: this.usernameInput?.validationError || "Invalid username", + color: "red", + duration: 3000, + }, + }), + ); + } } });
🧹 Nitpick comments (3)
src/client/UsernameInput.ts (1)
51-56: Remove redundant optional chaining.On line 53,
user?.usernamehas unnecessary optional chaining sinceuseris already checked for truthiness on line 52.Proposed fix
crazyGamesSDK.addAuthListener((user) => { if (user) { - this.parseAndSetUsername(user?.username); + this.parseAndSetUsername(user.username); } this.requestUpdate(); });src/client/Main.ts (1)
916-936: Consider using MutationObserver instead of timeouts.The
setTimeoutdelays (100ms, 500ms) to catch late-rendered components is a bit fragile. A MutationObserver would be more reliable for detecting when new elements with.no-crazygamesclass appear.That said, this approach works for most cases and keeps the code simple.
Alternative with MutationObserver
const hideCrazyGamesElements = () => { if (crazyGamesSDK.isOnCrazyGames()) { document.querySelectorAll(".no-crazygames").forEach((el) => { (el as HTMLElement).style.display = "none"; }); } }; const observeCrazyGamesElements = () => { if (!crazyGamesSDK.isOnCrazyGames()) return; hideCrazyGamesElements(); const observer = new MutationObserver(() => { hideCrazyGamesElements(); }); observer.observe(document.body, { childList: true, subtree: true }); };src/client/CrazyGamesSDK.ts (1)
277-291: Inconsistent readiness check pattern.
createInviteLinkuses synchronousisReady()whilegetUsername,addAuthListener, andisInstantMultiplayeruse asyncready(). If called during initialization, this method returns null even though the SDK might become ready shortly after.Consider using the async pattern for consistency, or document why synchronous check is preferred here.
Proposed fix for consistency
- createInviteLink(gameId: string): string | null { - if (!this.isReady()) { + async createInviteLink(gameId: string): Promise<string | null> { + const isReady = await this.ready(); + if (!isReady) { console.warn("CrazyGames SDK not ready, cannot create invite link"); return null; } try { const link = window.CrazyGames!.SDK.game.inviteLink({ gameId }); console.log("CrazyGames: created invite link:", link); return link; } catch (error) { console.error("Failed to create invite link:", error); return null; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/client/SinglePlayerModal.ts
- src/client/LangSelector.ts
- src/client/graphics/layers/WinModal.ts
- src/client/components/PatternButton.ts
- src/client/HostLobbyModal.ts
- src/client/components/DesktopNavBar.ts
- src/client/PatternInput.ts
- src/client/components/PlayPage.ts
- src/client/graphics/layers/UnitDisplay.ts
- src/client/graphics/layers/SettingsModal.ts
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/UsernameInput.tssrc/client/Matchmaking.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/UsernameInput.tssrc/client/CrazyGamesSDK.tssrc/client/Main.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/UsernameInput.tssrc/client/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/CrazyGamesSDK.tssrc/client/Main.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/Main.ts
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/Matchmaking.tssrc/client/graphics/layers/GameRightSidebar.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.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/client/CrazyGamesSDK.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/CrazyGamesSDK.tssrc/client/Main.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/client/Main.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/Main.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/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/Matchmaking.ts (5)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/LangSelector.ts (1)
translateText(278-302)src/client/Utils.ts (1)
translateText(165-224)
src/client/graphics/layers/GameRightSidebar.ts (4)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)src/core/game/GameView.ts (1)
isAlive(470-472)src/core/game/PlayerImpl.ts (1)
isAlive(347-349)
src/client/Main.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/UsernameInput.ts (1)
genAnonUsername(204-210)
⏰ 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 (9)
src/client/graphics/layers/GameRightSidebar.ts (1)
106-114: SDK calls look good here.The pause/resume toggle correctly calls
gameplayStop()andgameplayStart(). These methods have internal try-catch blocks in the SDK, so failures won't break the pause flow.src/client/UsernameInput.ts (2)
203-210: Good anonymous username generator.The
genAnonUsernamefunction produces usernames like "Anon042" using a stable hash from UUID. The BigInt conversion from hex and modulo 1000 gives a well-distributed three-digit suffix.One small note:
padStart(3, "0")ensures consistent formatting which is nice.
41-57: AdddisconnectedCallbackwith SDK listener tracking.The CrazyGames SDK does not expose a method to remove auth listeners. To prevent listener accumulation, add a
disconnectedCallbackto clean up and track whether the listener was already registered at the SDK wrapper level. Consider adding a flag toCrazyGamesSDKto ensure the auth listener is registered only once globally, since the SDK cannot remove listeners—stacking them on repeated component mount/unmount will cause duplicate updates.disconnectedCallback() { super.disconnectedCallback(); // Remove listeners or track registration at SDK level }Similar cleanup patterns exist elsewhere in the codebase (see
PatternInput,LangSelector, etc.).src/client/Matchmaking.ts (1)
266-270: Good guard for CrazyGames platform.Returning empty html when on CrazyGames correctly hides the matchmaking UI as intended by the PR.
src/client/Main.ts (1)
727-728: Good fallback to anonymous username.Using
genAnonUsername()as fallback instead of an empty string ensures players always have a valid name when joining.src/client/CrazyGamesSDK.ts (4)
51-68: Good readiness mechanism with timeout.The
ready()method usingPromise.racewith a 3-second timeout is a clean pattern. This prevents indefinite waiting if the SDK never initializes.
122-133: Try-catch added for getUsername.The SDK call is now wrapped in try-catch, returning null on error. This matches the pattern used elsewhere in the class.
135-152: Auth listener now uses async ready pattern.This addresses the previous inconsistency -
addAuthListenernow awaitsready()before adding the listener, matchinggetUsernameandisInstantMultiplayer.
308-333: Good midgame ad implementation.The
requestMidgameAdmethod properly:
- Returns a Promise that always resolves (never rejects)
- Handles all callback paths (finished, error, started)
- Wraps SDK call in try-catch
- Resolves immediately if SDK not ready
This ensures callers can safely await without worrying about unhandled rejections.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private handleLoggedInClick() { | ||
| const usernameInput = document.querySelector("username-input") as any; | ||
| const publicLobby = document.querySelector("public-lobby") as any; | ||
|
|
||
| if (usernameInput?.isValid()) { | ||
| this.open(); | ||
| publicLobby?.leaveLobby(); | ||
| } else { | ||
| window.dispatchEvent( | ||
| new CustomEvent("show-message", { | ||
| detail: { | ||
| message: usernameInput?.validationError, | ||
| color: "red", | ||
| duration: 3000, | ||
| }, | ||
| }), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle null usernameInput gracefully.
If document.querySelector("username-input") returns null, calling usernameInput?.isValid() returns undefined (falsy), so the error message path runs. But usernameInput?.validationError will be undefined, showing no useful message to the user.
Proposed fix
private handleLoggedInClick() {
const usernameInput = document.querySelector("username-input") as any;
const publicLobby = document.querySelector("public-lobby") as any;
if (usernameInput?.isValid()) {
this.open();
publicLobby?.leaveLobby();
} else {
+ const errorMessage = usernameInput?.validationError || translateText("username.invalid");
window.dispatchEvent(
new CustomEvent("show-message", {
detail: {
- message: usernameInput?.validationError,
+ message: errorMessage,
color: "red",
duration: 3000,
},
}),
);
}
}🤖 Prompt for AI Agents
In `@src/client/Matchmaking.ts` around lines 307 - 325, In handleLoggedInClick,
guard against document.querySelector("username-input") returning null by
explicitly checking usernameInput before calling isValid or reading
validationError; if usernameInput is null, dispatch the "show-message" event
with a clear fallback message like "Username input not found" (or a generic
validation prompt) and return; otherwise proceed with the existing logic (call
usernameInput.isValid(), this.open(), and publicLobby?.leaveLobby()) and use
usernameInput.validationError when showing the error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 108-117: The pauseGame method calls async
crazyGamesSDK.gameplayStop() and gameplayStart() without awaiting them which can
lead to race conditions; make pauseGame async (or otherwise return a Promise)
and await the calls to crazyGamesSDK.gameplayStop() /
crazyGamesSDK.gameplayStart(), then emit the PauseGameIntentEvent only after the
awaited call completes, preserving the existing guard using this.shouldPause and
this.wasPausedWhenOpened to avoid changing control flow.
♻️ Duplicate comments (5)
src/client/graphics/layers/SettingsModal.ts (1)
3-4: Use relative import paths for consistency.These imports use absolute-style paths (
src/client/...) while the rest of the file uses relative paths (../../../core/...). Use relative paths for consistency.Suggested change
-import { crazyGamesSDK } from "src/client/CrazyGamesSDK"; -import { PauseGameIntentEvent } from "src/client/Transport"; +import { crazyGamesSDK } from "../../CrazyGamesSDK"; +import { PauseGameIntentEvent } from "../../Transport";src/client/graphics/layers/GameRightSidebar.ts (1)
116-128: Add error handling to prevent users getting stuck on exit.If
requestMidgameAd()orgameplayStop()throws an error, the redirect will never happen and the user will be stuck. Wrap in try-catch to ensure the redirect always occurs.Proposed fix
private async onExitButtonClick() { const isAlive = this.game.myPlayer()?.isAlive(); if (isAlive) { const isConfirmed = confirm( translateText("help_modal.exit_confirmation"), ); if (!isConfirmed) return; } - await crazyGamesSDK.requestMidgameAd(); - await crazyGamesSDK.gameplayStop(); - window.location.href = "/"; + try { + await crazyGamesSDK.requestMidgameAd(); + await crazyGamesSDK.gameplayStop(); + } catch (err) { + console.error("SDK error on exit:", err); + } + window.location.href = "/"; }src/client/Matchmaking.ts (2)
252-259: Missing cleanup for event listener causes memory leak.The
userMeResponseevent listener is added inconnectedCallbackbut never removed indisconnectedCallback. If this component is removed and re-added to the DOM, listeners will stack up.Proposed fix
+ private boundUserMeHandler = (event: Event) => { + const customEvent = event as CustomEvent; + if (customEvent.detail) { + const userMeResponse = customEvent.detail as UserMeResponse | false; + this.isLoggedIn = hasLinkedAccount(userMeResponse); + } + }; async connectedCallback() { super.connectedCallback(); - document.addEventListener("userMeResponse", (event: Event) => { - const customEvent = event as CustomEvent; - if (customEvent.detail) { - const userMeResponse = customEvent.detail as UserMeResponse | false; - this.isLoggedIn = hasLinkedAccount(userMeResponse); - } - }); + document.addEventListener("userMeResponse", this.boundUserMeHandler); } + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener("userMeResponse", this.boundUserMeHandler); + }
307-325: Handle null usernameInput gracefully.If
document.querySelector("username-input")returns null,usernameInput?.validationErrorwill be undefined, showing no useful message to the user.Proposed fix
private handleLoggedInClick() { const usernameInput = document.querySelector("username-input") as any; const publicLobby = document.querySelector("public-lobby") as any; if (usernameInput?.isValid()) { this.open(); publicLobby?.leaveLobby(); } else { + const errorMessage = usernameInput?.validationError || translateText("username.invalid"); window.dispatchEvent( new CustomEvent("show-message", { detail: { - message: usernameInput?.validationError, + message: errorMessage, color: "red", duration: 3000, }, }), ); } }src/client/Main.ts (1)
597-604: Add username validation before opening the host modal.The instant multiplayer path calls
hostModal.open()without the same username validation used by thehostLobbyButtonhandler. This could allow creating a lobby with an invalid username.Proposed fix
crazyGamesSDK.isInstantMultiplayer().then((isInstant) => { if (isInstant) { console.log( `CrazyGames: joining instant multiplayer lobby from CrazyGames`, ); - this.hostModal.open(); + if (this.usernameInput?.isValid()) { + this.hostModal.open(); + } else { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: this.usernameInput?.validationError || translateText("username.invalid"), + color: "red", + duration: 3000, + }, + }), + ); + } } });
🧹 Nitpick comments (4)
src/client/CrazyGamesSDK.ts (2)
277-291: Inconsistent readiness check pattern.
createInviteLinkuses synchronousisReady()whilegetUsername(),addAuthListener(), andisInstantMultiplayer()use asyncready()with timeout. If called during initialization, it may returnnulleven though the SDK would become ready shortly after.Consider using the same async pattern for consistency:
Suggested change
-createInviteLink(gameId: string): string | null { - if (!this.isReady()) { +async createInviteLink(gameId: string): Promise<string | null> { + const isReady = await this.ready(); + if (!isReady) { console.warn("CrazyGames SDK not ready, cannot create invite link"); return null; } try { const link = window.CrazyGames!.SDK.game.inviteLink({ gameId }); console.log("CrazyGames: created invite link:", link); return link; } catch (error) { console.error("Failed to create invite link:", error); return null; } }
308-333: Same inconsistent readiness check in requestMidgameAd.Uses synchronous
isReady()instead of asyncready(). This may cause the ad request to silently skip if called during SDK initialization.Suggested change
-requestMidgameAd(): Promise<void> { - return new Promise((resolve) => { - if (!this.isReady()) { - resolve(); - return; - } +async requestMidgameAd(): Promise<void> { + const isReady = await this.ready(); + if (!isReady) { + return; + } - try { - const callbacks = { - adFinished: () => { - console.log("End midgame ad"); - resolve(); - }, - adError: (error: any) => { - console.log("Error midgame ad", error); - resolve(); - }, - adStarted: () => console.log("Start midgame ad"), - }; - window.CrazyGames!.SDK.ad.requestAd("midgame", callbacks); - } catch (error) { - console.error("Failed to request midgame ad:", error); - resolve(); - } - }); + return new Promise((resolve) => { + try { + const callbacks = { + adFinished: () => { + console.log("End midgame ad"); + resolve(); + }, + adError: (error: any) => { + console.log("Error midgame ad", error); + resolve(); + }, + adStarted: () => console.log("Start midgame ad"), + }; + window.CrazyGames!.SDK.ad.requestAd("midgame", callbacks); + } catch (error) { + console.error("Failed to request midgame ad:", error); + resolve(); + } + }); }src/client/UsernameInput.ts (1)
41-57: Username initialization flow is well structured.The three-step approach (localStorage → SDK → auth listener) ensures username is always populated. Calling
parseAndSetUsernameconsistently in all paths handles clan tag extraction correctly.One small nit: on line 51,
user?.usernameuses optional chaining butuseris already checked for truthiness on line 50. This is harmless but redundant.Optional cleanup
crazyGamesSDK.addAuthListener((user) => { if (user) { - this.parseAndSetUsername(user?.username); + this.parseAndSetUsername(user.username); } else { this.parseAndSetUsername(genAnonUsername()); } this.requestUpdate(); });src/client/Main.ts (1)
916-937: Use a consistent method for hiding elements on CrazyGames and consider if multiple timeouts are necessary.The
hideCrazyGamesElements()function usesstyle.display = "none", which differs from the codebase pattern ofclassList.add("hidden")seen elsewhere in Main.ts. Update for consistency:const hideCrazyGamesElements = () => { if (crazyGamesSDK.isOnCrazyGames()) { document.querySelectorAll(".no-crazygames").forEach((el) => { (el as HTMLElement).classList.add("hidden"); }); } };The multiple
setTimeoutcalls work for components that render within 500ms, but consider if this is overly defensive. For reference,MatchmakingButtonalready handles this better by checkingisOnCrazyGames()in itsrender()method and returning empty content. If other components likePatternButtondo the same, the timeouts become unnecessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/client/CrazyGamesSDK.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/PatternInput.tssrc/client/SinglePlayerModal.tssrc/client/UsernameInput.tssrc/client/components/DesktopNavBar.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/WinModal.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/components/PlayPage.ts
- src/client/components/DesktopNavBar.ts
- src/client/graphics/layers/UnitDisplay.ts
- src/client/graphics/layers/WinModal.ts
- src/client/SinglePlayerModal.ts
🧰 Additional context used
🧠 Learnings (15)
📚 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/components/PatternButton.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/HostLobbyModal.tssrc/client/Matchmaking.tssrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/client/LangSelector.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/components/PatternButton.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/HostLobbyModal.tssrc/client/PatternInput.tssrc/client/Matchmaking.tssrc/client/UsernameInput.tssrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/client/CrazyGamesSDK.tssrc/client/LangSelector.ts
📚 Learning: 2026-01-08T13:52:08.203Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:08.203Z
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/graphics/layers/GameRightSidebar.tssrc/client/HostLobbyModal.tssrc/client/Matchmaking.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/layers/GameRightSidebar.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-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.tssrc/client/Main.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/client/PatternInput.tssrc/client/Matchmaking.tssrc/client/Main.tssrc/client/CrazyGamesSDK.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/client/PatternInput.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/PatternInput.tssrc/client/Matchmaking.tssrc/client/UsernameInput.tssrc/client/Main.tssrc/client/CrazyGamesSDK.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Matchmaking.tssrc/client/UsernameInput.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/client/Main.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
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/graphics/layers/SettingsModal.ts
📚 Learning: 2025-07-23T12:36:35.354Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1534
File: src/client/LangSelector.ts:97-106
Timestamp: 2025-07-23T12:36:35.354Z
Learning: In OpenFrontIO's LangSelector.ts, the getClosestSupportedLang method always joins language code parts with underscores ("_") because all keys in the languageMap use underscore format (e.g., pt_BR, sv_SE, zh_CN). This normalization ensures consistency regardless of whether the input language code uses hyphens or underscores as delimiters.
Applied to files:
src/client/LangSelector.ts
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
src/client/LangSelector.ts
🧬 Code graph analysis (6)
src/client/graphics/layers/GameRightSidebar.ts (3)
src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)src/core/game/GameView.ts (1)
isAlive(470-472)src/core/game/PlayerImpl.ts (1)
isAlive(347-349)
src/client/HostLobbyModal.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/PatternInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/Matchmaking.ts (5)
src/core/ApiSchemas.ts (1)
UserMeResponse(78-78)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/LangSelector.ts (1)
translateText(278-302)src/client/Utils.ts (1)
translateText(165-224)
src/client/UsernameInput.ts (1)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)
src/client/graphics/layers/SettingsModal.ts (2)
src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(336-336)src/client/Transport.ts (1)
PauseGameIntentEvent(33-35)
🔇 Additional comments (16)
src/client/CrazyGamesSDK.ts (4)
51-68: Good setup for async readiness gating.The promise-based readiness mechanism with a 3-second timeout is a clean pattern. This allows callers to wait for SDK initialization without blocking forever.
122-133: LGTM on getUsername.Try-catch added around SDK call as previously requested. Returns
nullon failure which is a safe fallback.
135-152: LGTM on addAuthListener.Now uses async
ready()pattern consistent with other methods. Good error handling with try-catch.
154-165: LGTM on isInstantMultiplayer.Try-catch added as previously requested. Returns
falseon failure.src/client/LangSelector.ts (1)
231-231: LGTM.Adding
"matchmaking-button"to the components list ensures it receives translation updates when the language changes. Consistent with how other components are handled.src/client/PatternInput.ts (2)
8-8: LGTM.Import added for CrazyGames SDK integration.
91-94: LGTM - Clean conditional rendering.Hiding the pattern input on CrazyGames aligns with the PR objective to hide features requiring login until CrazyGames accounts are implemented. Returning an empty template is a simple approach.
src/client/components/PatternButton.ts (1)
74-79: Theno-crazygamesclass is properly defined and functional.The class works through JavaScript in
src/client/Main.ts(lines 916-921), wherehideCrazyGamesElements()selects all elements with this class and hides them when on the CrazyGames platform. The class is consistently used across multiple components including PatternButton and DesktopNavBar, so no additional verification needed.src/client/HostLobbyModal.ts (2)
115-124: Good CrazyGames invite link integration.The logic correctly checks for the CrazyGames environment and uses the platform-specific invite link when available, falling back to the server-config-based URL otherwise. The null check on
createInviteLinkis properly handled.
131-135: Correct history update guard for CrazyGames.Avoiding browser history manipulation on CrazyGames makes sense since the platform manages its own navigation. This aligns with the broader pattern of bypassing server URLs in CrazyGames contexts.
src/client/graphics/layers/GameRightSidebar.ts (1)
106-113: LGTM for pause/resume SDK integration.Calling
gameplayStop()andgameplayStart()synchronously (fire-and-forget) is fine for pause/resume toggles. The SDK state stays in sync with the game pause state.src/client/Matchmaking.ts (2)
267-270: Correct guard for CrazyGames platform.Hiding matchmaking UI on CrazyGames makes sense since the feature is not supported there. The early return is clean and simple.
272-304: Logged-in and logged-out UI rendering looks good.The conditional rendering based on
isLoggedInis clean. Both paths include thematchmaking-modalcomponent, which is correct for ensuring the modal is always available.src/client/UsernameInput.ts (1)
203-210: Clean anonymous username generation.The
genAnonUsernamefunction is well implemented: converts UUID to a BigInt, takes modulo 1000, and pads to 3 digits. Exporting it allows reuse inMain.tsfor fallback scenarios.src/client/Main.ts (2)
217-217: Field initialization clarified.The
hostModalfield is now declared with definite assignment. The null check at line 481 ensures proper error handling if the DOM query fails.
727-728: Good fallback for anonymous username.Using
genAnonUsername()instead of an empty string ensures players always have a displayable name when joining a lobby.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private pauseGame(pause: boolean) { | ||
| if (this.shouldPause && !this.wasPausedWhenOpened) | ||
| if (this.shouldPause && !this.wasPausedWhenOpened) { | ||
| if (pause) { | ||
| crazyGamesSDK.gameplayStop(); | ||
| } else { | ||
| crazyGamesSDK.gameplayStart(); | ||
| } | ||
| this.eventBus.emit(new PauseGameIntentEvent(pause)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unawaited async calls may cause race conditions.
gameplayStop() and gameplayStart() are async methods but are not awaited here. If the user rapidly opens/closes the modal, the stop/start calls could complete out of order.
Suggested change
-private pauseGame(pause: boolean) {
+private async pauseGame(pause: boolean) {
if (this.shouldPause && !this.wasPausedWhenOpened) {
if (pause) {
- crazyGamesSDK.gameplayStop();
+ await crazyGamesSDK.gameplayStop();
} else {
- crazyGamesSDK.gameplayStart();
+ await crazyGamesSDK.gameplayStart();
}
this.eventBus.emit(new PauseGameIntentEvent(pause));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private pauseGame(pause: boolean) { | |
| if (this.shouldPause && !this.wasPausedWhenOpened) | |
| if (this.shouldPause && !this.wasPausedWhenOpened) { | |
| if (pause) { | |
| crazyGamesSDK.gameplayStop(); | |
| } else { | |
| crazyGamesSDK.gameplayStart(); | |
| } | |
| this.eventBus.emit(new PauseGameIntentEvent(pause)); | |
| } | |
| } | |
| private async pauseGame(pause: boolean) { | |
| if (this.shouldPause && !this.wasPausedWhenOpened) { | |
| if (pause) { | |
| await crazyGamesSDK.gameplayStop(); | |
| } else { | |
| await crazyGamesSDK.gameplayStart(); | |
| } | |
| this.eventBus.emit(new PauseGameIntentEvent(pause)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/SettingsModal.ts` around lines 108 - 117, The
pauseGame method calls async crazyGamesSDK.gameplayStop() and gameplayStart()
without awaiting them which can lead to race conditions; make pauseGame async
(or otherwise return a Promise) and await the calls to
crazyGamesSDK.gameplayStop() / crazyGamesSDK.gameplayStart(), then emit the
PauseGameIntentEvent only after the awaited call completes, preserving the
existing guard using this.shouldPause and this.wasPausedWhenOpened to avoid
changing control flow.
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan