Better crazygames support #2900
Conversation
WalkthroughAdds a CrazyGames SDK (readiness gating, user/auth hooks, invite links, ad APIs), integrates it into UI flows (username, modals, pause/resume, midgame ads), refactors boat/transport intents/executions to (dst, troops), converts conquests into per-type arrays and updates rankings, plus large localization updates. (41 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client App
participant SDK as CrazyGames SDK
participant Ad as Ad Provider
participant UI as UI Component
App->>SDK: initialize()
SDK->>SDK: create readyPromise / resolveReady()
SDK-->>App: ready resolved
UI->>SDK: await ready()
UI->>SDK: getUsername()
SDK-->>UI: username | null
UI->>SDK: addAuthListener(fn)
SDK-->>UI: invoke listener on auth change
UI->>SDK: requestMidgameAd()
SDK->>Ad: requestAd(adType, callbacks)
Ad-->>SDK: adStarted / adFinished / adError
SDK-->>UI: resolve/reject ad Promise
UI->>SDK: gameplayStop()/gameplayStart()
SDK-->>UI: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
## Description: As reported on Discord, warship could get stuck. This PR fixes the issue. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: moleole
## Description: "You can pick your spawn in random spawn games in v29. You need to open the menu and click on the attack button. That's it." Thats the fix for this problem. Radial menu no longer allows to attack (pick a spawn) while random spawn is enabled. And SpawnExecution got a check so you cannot send malicious intents. ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: FloPinguin
## Description: Quickfix: Disable nations in ranked and change map selection (Lewis wanted these, Australia three times so it occurs more often) Just a quickfix, we will probably have to improve the map selection later on, and maybe play on non-compact maps too? ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: FloPinguin
…timeout, add duration logging to some longer operations
## Description: ### Short path for multi-source HPA* Math was not mathing, increased the bounds to 260x260, it is a bit slower but should work better. The short path was breaking when player owned a lot of shores. This is because the bounding box of tiles with less than 120 distance + 10 padding could be as big as 260x260 and the optimized array was set to 140x140. I made mistake of calculating it as `2 * (60 + 10)` instead of `2 * (120 + 10)`. ### LoS path refinement Previously, we ran 2 passes of LoS smoothing on the path. However, since we are effectively tracing the same path, the line of sight is essentially the same. This PR makes second line of sight stop on water tiles with magnitude `n + 1` compared to first path. Practically, this means it'll attempt LoS exactly 1 tile after previous corner. See screenshot. <img width="1299" height="1151" alt="image" src="https://github.com/user-attachments/assets/726be236-1ff8-406c-896a-02902a762ab0" /> ### SendBoatAttackIntentEvent The flow of sending transport ships is currently strange. This PR makes the flow more sane. **Old flow** ``` - Player clicks TARGET tile, it can be deep inland - Client asks Worker for the best START tile to TARGET tile - Worker answers `false`, since the tile is inland - Client sends BoatAttackIntent with START=false and TARGET tiles set - Worker accepts BoatAttackIntent, computes DESTINATION as closest shore to TARGET - Worker re-computes best START to DESTINATION - Worker sends boat from START to DESTINATION ``` **New flow** ``` - Player clicks TARGET tile, it can be deep inland - Client sends BoatAttackIntent with TARGET - Worker accepts BoatAttackIntent, computes DESTINATION as closest shore to TARGET - Worker computes START as the best tile to DESTINATION - Worker sends boat from START to DESTINATION ``` ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: moleole
## Description: I analyzed the avg fill time of team games (past 30 days) and was able to confirm what people in the main discord said: Duos / Trios / Quads fill slower. Might be something for v29. | Game Mode | Games | Avg Fill Time | |-----------|-------|---------------| | **FFA** (Excluding ranked) | 53,654 | **29s** | | Team: 2 teams | 3,379 | 33s | | Team: 3 teams | 3,291 | 32s | | Team: 4 teams | 3,242 | 31s | | Team: 5 teams | 3,364 | 32s | | Team: 6 teams | 3,381 | 31s | | Team: 7 teams | 3,227 | 31s | | Team: Duos | 3,295 | **43s** | | Team: Trios | 3,300 | 39s | | Team: Quads | 3,299 | 37s | | Team: Humans Vs Nations | 101 | **24s** | Therefore I propose to decrease the chance of Duos, Trios and Quads (especially Duos). Also, increase the chance of HumansVsNations because its special and unlike all the other team modes. | Team Config | Previous | New | |-------------|----------|-----| | 2 teams | 10% | 10% | | 3 teams | 10% | 10% | | 4 teams | 10% | 10% | | 5 teams | 10% | 10% | | 6 teams | 10% | 10% | | 7 teams | 10% | 10% | | **Duos** | 10% | **5%** ↓ | | **Trios** | 10% | **7.5%** ↓ | | **Quads** | 10% | **7.5%** ↓ | | **HumansVsNations** | 10% | **20%** ↑ | ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: FloPinguin
…2971) ## Description: For v29 HvN winrate is between 10 and 15%, but should be around 50%. 1. Change HvN difficulty to Medium 2. Little balance change in `NationAllianceBehavior` ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: FloPinguin
## Description: Adds back the nuke overlay for teammates ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: Mr. Box
|
|
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 (4)
src/core/pathfinding/transformers/SmoothingWaterTransformer.ts (1)
89-97: PropagateminMagnitudeintotracePathto avoid shallow tiles.
canSeenow filters by magnitude, buttracePathstill allows shallow tiles. That can reintroduce tiles the LOS pass was meant to avoid.✅ Proposed fix
- const trace = this.tracePath(path[current], path[farthest]); + const trace = this.tracePath(path[current], path[farthest], minMagnitude);- private tracePath(from: TileRef, to: TileRef): TileRef[] | null { + private tracePath( + from: TileRef, + to: TileRef, + minMagnitude: number, + ): TileRef[] | null { const x0 = from % this.mapWidth; const y0 = (from / this.mapWidth) | 0; const x1 = to % this.mapWidth; const y1 = (to / this.mapWidth) | 0; const tiles: TileRef[] = []; @@ const tile = (y * this.mapWidth + x) as TileRef; if (!this.isTraversable(tile)) return null; + const magnitude = this.terrain[tile] & MAGNITUDE_MASK; + if (magnitude < minMagnitude) return null; tiles.push(tile); @@ x += sx; err -= dy; const intermediateTile = (y * this.mapWidth + x) as TileRef; - if (!this.isTraversable(intermediateTile)) { + const intMag = this.terrain[intermediateTile] & MAGNITUDE_MASK; + if (!this.isTraversable(intermediateTile) || intMag < minMagnitude) { x -= sx; err += dy; y += sy; err += dx; const altTile = (y * this.mapWidth + x) as TileRef; - if (!this.isTraversable(altTile)) return null; + const altMag = this.terrain[altTile] & MAGNITUDE_MASK; + if (!this.isTraversable(altTile) || altMag < minMagnitude) + return null; tiles.push(altTile);Also applies to: 269-316
resources/lang/ja.json (1)
1-953: Non-English locale file should not be updated in this PR.Based on project guidelines, translation files in
resources/lang/*.json(excepten.json) should only be updated in dedicated translation PRs titled "mls" made by Aotumori. This ensures translations are reviewed by native speakers.Please remove changes to
ja.jsonfrom this PR. New translation keys can be added toen.json, and the Japanese translations will be handled in a separate translation PR.resources/lang/tr.json (1)
1-953: Non-English locale file should not be updated in this PR.Same concern as
ja.json: Turkish translation updates should be handled in dedicated translation PRs, not in feature PRs like this CrazyGames support PR.Please remove changes to
tr.jsonfrom this PR and coordinate with the translation team to update these locales separately.resources/lang/ru.json (1)
1-952: Avoid updating non‑English locale files in this PR.Project policy is to update only
resources/lang/en.jsonin regular PRs and let translation PRs handle other locales. Please revertru.jsonchanges here (keep keys in en.json only), or move these translations to the dedicated translation PR flow.Based on learnings, non‑English locale updates are handled separately.
🤖 Fix all issues with AI agents
In `@resources/lang/nl.json`:
- Around line 488-492: Replace the incorrect Dutch imperative "Houdt" with
"Houd" in the translation strings; update the values for the keys
"build_menu_modifier_desc" and "emoji_menu_modifier_desc" so they read "Houd
deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen." and "Houd
deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen."
respectively to fix the grammar.
In `@resources/lang/zh-CN.json`:
- Around line 488-492: The translations for "build_menu_modifier" and
"emoji_menu_modifier" mistakenly use "编辑器" (editor); change their values to use
"修饰键" (modifier key) instead, e.g., update "build_menu_modifier" from "建造菜单编辑器"
to "建造菜单修饰键" and "emoji_menu_modifier" from "Emoji 表情菜单编辑器" to "Emoji 表情菜单修饰键",
leaving the corresponding "..._desc" entries (press-and-click instructions)
unchanged.
In `@src/core/execution/TransportShipExecution.ts`:
- Around line 80-83: The property/parameter troops in TransportShipExecution is
treated as possibly undefined (using the ??= operator) but is currently typed as
number; update the constructor/signature for TransportShipExecution to accept an
optional troops (e.g., troops?: number or troops: number | undefined) so the
nullish assignment is type-safe, then keep the existing logic that sets
this.troops = this.troops ?? this.mg.config().boatAttackAmount(this.attacker,
this.target) and clamps it with Math.min(this.troops, this.attacker.troops()) to
ensure a valid numeric value.
In `@src/core/Schemas.ts`:
- Around line 248-252: The UsernameSchema regex now accepts dots (.) but the
user-facing validation message still only lists letters, numbers, spaces and
underscores; update the English locale string used for username validation to
mention dots (and mirror the change in other locale files as needed) so the
message matches UsernameSchema's allowed characters; locate the username
validation message in your app's localization files (English first) and edit the
string to include "dots" (or ".") and any punctuation/translation variants
accordingly.
♻️ Duplicate comments (4)
src/client/Matchmaking.ts (2)
252-259: Missing cleanup for event listener causes memory leak.The
userMeResponseevent listener is added inconnectedCallbackbut never removed. If this component is removed and re-added to the DOM, listeners will stack up.
307-325: Handle null usernameInput gracefully.If
document.querySelector("username-input")returns null,usernameInput?.validationErrorwill be undefined, showing no useful message to the user.src/client/PatternInput.ts (1)
76-79: VerifyisOnCrazyGames()is accurate before hiding the UI.This now hides the pattern input entirely when
isOnCrazyGames()is true. Please confirm the detection is tight so non‑CrazyGames users don’t lose this feature.#!/bin/bash # Inspect isOnCrazyGames implementation rg -n "isOnCrazyGames\\s*\\(" src/client/CrazyGamesSDK.ts sed -n '/isOnCrazyGames/,/^[[:space:]]*}/p' src/client/CrazyGamesSDK.ts | head -80src/client/Main.ts (1)
619-626: Missing username validation before opening host modal in instant multiplayer flow.The
hostLobbyButtonhandler (lines 488-502) validates the username before proceeding. This instant multiplayer path callsthis.hostModal.open()directly without the same validation check.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 || "Invalid username", + color: "red", + duration: 3000, + }, + }), + ); + } } });
🧹 Nitpick comments (9)
src/core/pathfinding/algorithms/AStar.WaterHierarchical.ts (1)
45-46: Keep short-path sizing in one place to avoid drift.
shortPathSizeis tied toSHORT_PATH_THRESHOLDandPADDING, but the values are duplicated. If one changes, the other can go out of sync. Consider shared constants so the size always matches the threshold + padding.♻️ Proposed refactor
export class AStarWaterHierarchical implements PathFinder<number> { + private static readonly SHORT_PATH_THRESHOLD = 120; + private static readonly SHORT_PATH_PADDING = 10; + private static readonly SHORT_PATH_SIZE = + 2 * (AStarWaterHierarchical.SHORT_PATH_THRESHOLD + + AStarWaterHierarchical.SHORT_PATH_PADDING); + @@ - const shortPathSize = 260; // 2 * (120 + padding 10) + const shortPathSize = AStarWaterHierarchical.SHORT_PATH_SIZE; const maxShortPathNodes = shortPathSize * shortPathSize; @@ - const SHORT_PATH_THRESHOLD = 120; - const PADDING = 10; + const SHORT_PATH_THRESHOLD = AStarWaterHierarchical.SHORT_PATH_THRESHOLD; + const PADDING = AStarWaterHierarchical.SHORT_PATH_PADDING;Also applies to: 99-100
resources/lang/nl.json (1)
29-31: Non-English locale file updated in regular PR.Based on learnings, translation files in
resources/lang/*.json(excepten.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. This allows technical changes to proceed while ensuring accurate translations from native speakers.Consider removing the Dutch translations from this PR and letting the dedicated translation workflow handle them.
src/server/MapPlaylist.ts (1)
147-173: Make 1v1 map weighting explicit (optional).The duplicate entries work, but a weighted list reads cleaner and avoids accidental edits.
💡 One way to make weights explicit
- const maps = [ - GameMapType.Iceland, - GameMapType.Australia, - GameMapType.Australia, - GameMapType.Australia, - GameMapType.Pangaea, - GameMapType.Italia, - GameMapType.FalklandIslands, - GameMapType.Sierpinski, - ]; + const maps: Array<{ map: GameMapType; weight: number }> = [ + { map: GameMapType.Iceland, weight: 1 }, + { map: GameMapType.Australia, weight: 3 }, + { map: GameMapType.Pangaea, weight: 1 }, + { map: GameMapType.Italia, weight: 1 }, + { map: GameMapType.FalklandIslands, weight: 1 }, + { map: GameMapType.Sierpinski, weight: 1 }, + ]; + const totalWeight = maps.reduce((sum, m) => sum + m.weight, 0); + let roll = Math.random() * totalWeight; + const pickedMap = + maps.find(({ weight }) => (roll -= weight) < 0)?.map ?? maps[0].map; return { @@ - gameMap: maps[Math.floor(Math.random() * maps.length)], + gameMap: pickedMap,src/core/StatsSchemas.ts (1)
105-112: Consider enforcing a 3-slot conquests array.If code indexes by
PLAYER_INDEX_HUMAN/NATION/BOT, a short array will yieldundefined. If the intent is always 3 slots, a dedicated schema would be safer.♻️ Possible schema tightening
-const AtLeastOneNumberSchema = BigIntStringSchema.array().min(1); +const AtLeastOneNumberSchema = BigIntStringSchema.array().min(1); +const ConquestsByTypeSchema = BigIntStringSchema.array().length(3); ... - conquests: AtLeastOneNumberSchema.optional(), + conquests: ConquestsByTypeSchema.optional(),src/core/game/StatsImpl.ts (1)
257-263: Undefined check is redundant but harmless.Since
conquest_by_typeis typed asRecord<PlayerType, number>and covers allPlayerTypevalues,conquestTypecan never be undefined. The check at line 260 is defensive but unnecessary.That said, keeping it provides safety if
PlayerTypeexpands in the future, so this is fine to leave as-is.src/client/Cosmetics.ts (1)
32-56: Reset the cache on failure to allow retry.If the first fetch fails, the cached
nullblocks all future retries. Consider clearing__cosmeticson error so a later call can re‑try.♻️ Proposed change
const response = await fetch(`${getApiBase()}/cosmetics.json`); if (!response.ok) { console.error(`HTTP error! status: ${response.status}`); + __cosmetics = null; return null; } const result = CosmeticsSchema.safeParse(await response.json()); if (!result.success) { console.error(`Invalid cosmetics: ${result.error.message}`); + __cosmetics = null; return null; } return result.data; } catch (error) { console.error("Error getting cosmetics:", error); + __cosmetics = null; return null; } })();src/client/UsernameInput.ts (1)
49-54: Redundant optional chaining onuser?.username.On line 51,
useris already truthy from the check on line 50. The optional chaininguser?.usernameis unnecessary.Suggested fix
crazyGamesSDK.addAuthListener((user) => { if (user) { - this.parseAndSetUsername(user?.username); + this.parseAndSetUsername(user.username); } this.requestUpdate(); });src/client/Main.ts (1)
942-962: Consider a single delayed call instead of multiple timeouts.Calling
hideCrazyGamesElements()three times (immediately, 100ms, 500ms) is a bit fragile. If components render later than 500ms, they won't be hidden. Consider using a MutationObserver or a single longer delay.Alternative approach with MutationObserver
const hideCrazyGamesElements = () => { if (!crazyGamesSDK.isOnCrazyGames()) return; document.querySelectorAll(".no-crazygames").forEach((el) => { (el as HTMLElement).style.display = "none"; }); }; // In bootstrap: hideCrazyGamesElements(); if (crazyGamesSDK.isOnCrazyGames()) { const observer = new MutationObserver(() => hideCrazyGamesElements()); observer.observe(document.body, { childList: true, subtree: true }); // Disconnect after 2 seconds to avoid permanent overhead setTimeout(() => observer.disconnect(), 2000); }src/client/CrazyGamesSDK.ts (1)
277-291: Consider using asyncready()for consistency.This method uses synchronous
isReady()while the other new methods (getUsername,isInstantMultiplayer) that return values use asyncready()with timeout. IfcreateInviteLinkis called during the initialization window, it fails immediately rather than waiting up to 3 seconds.If this is intentional (fail-fast behavior for UI), the current approach is fine. Otherwise, consider aligning with the async pattern:
Proposed change 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; }
| "menu_shortcuts": "Menu sneltoetsen", | ||
| "build_menu_modifier": "Bouwmenu", | ||
| "build_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen.", | ||
| "emoji_menu_modifier": "Emoji-menu", | ||
| "emoji_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen.", |
There was a problem hiding this comment.
Minor Dutch grammar issue: "Houdt" should be "Houd".
In Dutch, the imperative singular form drops the 't'. The correct phrasing is "Houd deze toets ingedrukt" rather than "Houdt deze toets ingedrukt".
Proposed fix
"menu_shortcuts": "Menu sneltoetsen",
"build_menu_modifier": "Bouwmenu",
- "build_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen.",
+ "build_menu_modifier_desc": "Houd deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen.",
"emoji_menu_modifier": "Emoji-menu",
- "emoji_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen.",
+ "emoji_menu_modifier_desc": "Houd deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen.",📝 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.
| "menu_shortcuts": "Menu sneltoetsen", | |
| "build_menu_modifier": "Bouwmenu", | |
| "build_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen.", | |
| "emoji_menu_modifier": "Emoji-menu", | |
| "emoji_menu_modifier_desc": "Houdt deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen.", | |
| "menu_shortcuts": "Menu sneltoetsen", | |
| "build_menu_modifier": "Bouwmenu", | |
| "build_menu_modifier_desc": "Houd deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen.", | |
| "emoji_menu_modifier": "Emoji-menu", | |
| "emoji_menu_modifier_desc": "Houd deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen.", |
🤖 Prompt for AI Agents
In `@resources/lang/nl.json` around lines 488 - 492, Replace the incorrect Dutch
imperative "Houdt" with "Houd" in the translation strings; update the values for
the keys "build_menu_modifier_desc" and "emoji_menu_modifier_desc" so they read
"Houd deze toets ingedrukt terwijl je klikt, om het bouwmenu te openen." and
"Houd deze toets ingedrukt terwijl je klikt, om het emoji-menu te openen."
respectively to fix the grammar.
| "menu_shortcuts": "菜单快捷键", | ||
| "build_menu_modifier": "建造菜单编辑器", | ||
| "build_menu_modifier_desc": "按住此键并点击以打开建造菜单。", | ||
| "emoji_menu_modifier": "Emoji 表情菜单编辑器", | ||
| "emoji_menu_modifier_desc": "按住此键并点击以打开 Emoji 表情菜单。", |
There was a problem hiding this comment.
Translation uses "编辑器" (editor) instead of "修饰键" (modifier key).
In keyboard shortcut contexts, "modifier" refers to keys like Shift/Ctrl/Alt that modify other key presses. The current translation "编辑器" means "editor", which is misleading.
Suggested fix
- "menu_shortcuts": "菜单快捷键",
- "build_menu_modifier": "建造菜单编辑器",
- "build_menu_modifier_desc": "按住此键并点击以打开建造菜单。",
- "emoji_menu_modifier": "Emoji 表情菜单编辑器",
- "emoji_menu_modifier_desc": "按住此键并点击以打开 Emoji 表情菜单。",
+ "menu_shortcuts": "菜单快捷键",
+ "build_menu_modifier": "建造菜单修饰键",
+ "build_menu_modifier_desc": "按住此键并点击以打开建造菜单。",
+ "emoji_menu_modifier": "Emoji 表情菜单修饰键",
+ "emoji_menu_modifier_desc": "按住此键并点击以打开 Emoji 表情菜单。",🤖 Prompt for AI Agents
In `@resources/lang/zh-CN.json` around lines 488 - 492, The translations for
"build_menu_modifier" and "emoji_menu_modifier" mistakenly use "编辑器" (editor);
change their values to use "修饰键" (modifier key) instead, e.g., update
"build_menu_modifier" from "建造菜单编辑器" to "建造菜单修饰键" and "emoji_menu_modifier" from
"Emoji 表情菜单编辑器" to "Emoji 表情菜单修饰键", leaving the corresponding "..._desc" entries
(press-and-click instructions) unchanged.
| this.troops ??= this.mg | ||
| .config() | ||
| .boatAttackAmount(this.attacker, this.target); | ||
|
|
||
| this.startTroops = Math.min(this.startTroops, this.attacker.troops()); | ||
| this.troops = Math.min(this.troops, this.attacker.troops()); |
There was a problem hiding this comment.
Type mismatch: troops is typed as number but handled as potentially nullish.
The ??= operator implies troops could be undefined, yet the constructor parameter is typed as number. If you want to support optional troops, update the signature to troops?: number or troops: number | undefined.
Proposed fix
constructor(
private attacker: Player,
private ref: TileRef,
- private troops: number,
+ private troops?: number,
) {
this.originalOwner = this.attacker;
}🤖 Prompt for AI Agents
In `@src/core/execution/TransportShipExecution.ts` around lines 80 - 83, The
property/parameter troops in TransportShipExecution is treated as possibly
undefined (using the ??= operator) but is currently typed as number; update the
constructor/signature for TransportShipExecution to accept an optional troops
(e.g., troops?: number or troops: number | undefined) so the nullish assignment
is type-safe, then keep the existing logic that sets this.troops = this.troops
?? this.mg.config().boatAttackAmount(this.attacker, this.target) and clamps it
with Math.min(this.troops, this.attacker.troops()) to ensure a valid numeric
value.
| export const UsernameSchema = z | ||
| .string() | ||
| .regex(/^[a-zA-Z0-9_ [\]üÜ]+$/u) | ||
| .regex(/^[a-zA-Z0-9_ [\]üÜ.]+$/u) | ||
| .min(3) | ||
| .max(27); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the UsernameSchema in src/core/Schemas.ts around lines 248-252
if [ -f "src/core/Schemas.ts" ]; then
echo "=== UsernameSchema snippet (lines 240-260) ==="
sed -n '240,260p' src/core/Schemas.ts
else
echo "File not found at src/core/Schemas.ts"
# Try to find the file
find . -name "Schemas.ts" -type f | head -5
fiRepository: openfrontio/OpenFrontIO
Length of output: 704
🏁 Script executed:
# Search for the error message mentioned in the suggestion
echo "=== Searching for 'invalid_chars' in locale files ==="
find . -name "*.json" -path "*/lang/*" -o -name "*.json" -path "*/i18n/*" | xargs grep -l "invalid_chars" 2>/dev/null || echo "No matches found with xargs, trying different approach"
rg "invalid_chars" --type jsonRepository: openfrontio/OpenFrontIO
Length of output: 6242
🏁 Script executed:
# Also search for any username-related error messages
echo "=== Searching for username-related error messages ==="
rg "Username can only" --type jsonRepository: openfrontio/OpenFrontIO
Length of output: 236
Update username error text to mention dots.
Regex now allows "." but the username error message still lists only letters, numbers, spaces, and underscores. This will confuse users. Update the English string (and other locales if you prefer) to include dots.
🔧 Suggested update (resources/lang/en.json)
- "invalid_chars": "Username can only contain letters, numbers, spaces, and underscores.",
+ "invalid_chars": "Username can only contain letters, numbers, spaces, underscores, and dots.",🤖 Prompt for AI Agents
In `@src/core/Schemas.ts` around lines 248 - 252, The UsernameSchema regex now
accepts dots (.) but the user-facing validation message still only lists
letters, numbers, spaces and underscores; update the English locale string used
for username validation to mention dots (and mirror the change in other locale
files as needed) so the message matches UsernameSchema's allowed characters;
locate the username validation message in your app's localization files (English
first) and edit the string to include "dots" (or ".") and any
punctuation/translation variants accordingly.
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)
11-48: Do not overwrite stored username when CrazyGames SDK is unavailable.When
crazyGamesSDK.getUsername()returnsnull, the code generates a fresh random anon value and overwrites the stored username. This causes the username to change unexpectedly on each visit when the SDK is not ready. Preserve the username set in line 44 (stored or generated on first visit) unless the SDK returns a real username.Suggested fix
crazyGamesSDK.getUsername().then((username) => { - this.parseAndSetUsername(username ?? genAnonUsername()); - this.requestUpdate(); + if (username) { + this.parseAndSetUsername(username); + this.requestUpdate(); + } });
🤖 Fix all issues with AI agents
In `@src/client/CrazyGamesSDK.ts`:
- Around line 308-332: The requestMidgameAd promise can hang if the CrazyGames
SDK never calls adFinished or adError; modify requestMidgameAd to implement an
8‑second timeout and a local done flag so the promise always resolves once:
create a boolean done = false, wrap adFinished/adError handlers (and the
try/catch resolve) to check/set done before resolving, start a setTimeout for
8000ms that resolves if not done and sets done, and clearTimeout when the SDK
callback runs so you never resolve twice; update the callbacks object referenced
in window.CrazyGames!.SDK.ad.requestAd and ensure the catch path also follows
the done flag logic.
♻️ Duplicate comments (6)
src/client/Matchmaking.ts (2)
252-259: Clean up the auth listener on disconnect.The inline listener can’t be removed, so if the element is reattached you’ll stack listeners. Please bind the handler and remove it in
disconnectedCallback.🧹 Suggested 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); + }
307-325: Guard whenusername-inputis missing.If the element is absent, the error message becomes
undefined. Add a clear fallback and return early.🛡️ Suggested fix
private handleLoggedInClick() { const usernameInput = document.querySelector("username-input") as any; const publicLobby = document.querySelector("public-lobby") as any; - if (usernameInput?.isValid()) { + if (!usernameInput) { + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: translateText("username.invalid"), + color: "red", + duration: 3000, + }, + }), + ); + return; + } + + if (usernameInput.isValid()) { this.open(); publicLobby?.leaveLobby(); } else { window.dispatchEvent( new CustomEvent("show-message", { detail: { - message: usernameInput?.validationError, + message: usernameInput.validationError, color: "red", duration: 3000, }, }), ); } }src/client/graphics/layers/GameRightSidebar.ts (1)
116-127: Ensure redirect happens even if SDK calls fail.If the SDK throws or hangs, the redirect never runs. Wrap SDK calls in try/finally (or catch) so exit always completes.
🧯 Suggested 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); + } finally { + // redirect to the home page + window.location.href = "/"; + } }src/client/UsernameInput.ts (1)
49-54: Auth listener cleanup is still missing.Listeners can stack if the component is reattached. If the SDK doesn’t support removal, consider a lifecycle guard or avoiding re‑creation.
src/client/Main.ts (2)
219-219: Field declaration still lacks definite assignment assertion.The past review noted this field may be undefined if DOM query fails at line 480. The fix using
!assertion was reportedly addressed but isn't visible in this diff. If the modal element is missing,this.hostModal.open()at line 624 will throw.
619-626: Missing username validation before opening host modal.The
hostLobbyButtonhandler at lines 488-502 validates the username before showing the host page. This instant multiplayer path callsthis.hostModal.open()without the same check, which could create a lobby with invalid username.
🧹 Nitpick comments (4)
src/client/CrazyGamesSDK.ts (1)
51-68: Skip the 3s wait when not on CrazyGames.Right now
ready()can delay non-CrazyGames callers by up to 3 seconds. Short‑circuiting avoids UI waits on normal web hosts.♻️ Suggested tweak
async ready(): Promise<boolean> { + if (!this.isOnCrazyGames()) { + return false; + } const timeout = new Promise<boolean>((resolve) => { setTimeout(() => resolve(false), 3000); });src/client/Main.ts (3)
918-921: Consider updating the log message.The condition now also triggers when on CrazyGames (even if a prefetched token exists). The log message "No prefetched turnstile token" doesn't cover the CrazyGames case.
Suggested improvement
if (this.turnstileTokenPromise === null || crazyGamesSDK.isOnCrazyGames()) { - console.log("No prefetched turnstile token, getting new token"); + console.log("Getting fresh turnstile token"); return (await getTurnstileToken())?.token ?? null; }
943-950: Consider usingclassList.add("hidden")for consistency.Based on learnings, this codebase uses
classList.add("hidden")for hiding elements. Usingstyle.display = "none"works but differs from the established pattern.Suggested change for consistency
const hideCrazyGamesElements = () => { if (crazyGamesSDK.isOnCrazyGames()) { document.querySelectorAll(".no-crazygames").forEach((el) => { - (el as HTMLElement).style.display = "none"; + el.classList.add("hidden"); }); } };
957-963: Pragmatic but fragile timing approach.Using fixed delays (100ms, 500ms) to catch late-rendered components works but may miss components that render later. If this becomes unreliable, consider a
MutationObserverto detect when new.no-crazygameselements appear in the DOM.For now, this approach is acceptable given its simplicity.
| requestMidgameAd(): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| if (!this.isReady()) { | ||
| resolve(); | ||
| 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(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specific lines
cat -n src/client/CrazyGamesSDK.ts | sed -n '300,340p'Repository: openfrontio/OpenFrontIO
Length of output: 1257
🏁 Script executed:
# Search for other ad request methods to see if there are existing timeout patterns
rg "requestAd|adFinished|adError" src/client/CrazyGamesSDK.ts -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 775
🏁 Script executed:
# Search for timeout patterns in the codebase to understand conventions
rg "setTimeout|clearTimeout" src/ --type ts -B 2 -A 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 2309
🏁 Script executed:
# Check if there are other ad request methods that might need the same treatment
rg "requestAd|rewardedAd|interstitial" src/client/CrazyGamesSDK.ts -B 2 -A 15Repository: openfrontio/OpenFrontIO
Length of output: 914
Add a timeout fail‑safe for midgame ads.
If the SDK never calls adFinished or adError, the promise hangs indefinitely and blocks the game flow. Add an 8-second timeout that resolves the promise even if callbacks never fire. Use a done flag to prevent multiple resolutions.
⏱️ Suggested fix
requestMidgameAd(): Promise<void> {
return new Promise((resolve) => {
if (!this.isReady()) {
resolve();
return;
}
+ let done = false;
+ const finish = () => {
+ if (done) return;
+ done = true;
+ clearTimeout(timeoutId);
+ resolve();
+ };
+
+ const timeoutId = window.setTimeout(() => {
+ console.warn("Midgame ad timeout, continuing");
+ finish();
+ }, 8000);
+
try {
const callbacks = {
adFinished: () => {
console.log("End midgame ad");
- resolve();
+ finish();
},
adError: (error: any) => {
console.log("Error midgame ad", error);
- resolve();
+ finish();
},
adStarted: () => console.log("Start midgame ad"),
};
window.CrazyGames!.SDK.ad.requestAd("midgame", callbacks);
} catch (error) {
console.error("Failed to request midgame ad:", error);
- resolve();
+ finish();
}
});
}📝 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.
| requestMidgameAd(): Promise<void> { | |
| return new Promise((resolve) => { | |
| if (!this.isReady()) { | |
| resolve(); | |
| 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(); | |
| } | |
| }); | |
| requestMidgameAd(): Promise<void> { | |
| return new Promise((resolve) => { | |
| if (!this.isReady()) { | |
| resolve(); | |
| return; | |
| } | |
| let done = false; | |
| const finish = () => { | |
| if (done) return; | |
| done = true; | |
| clearTimeout(timeoutId); | |
| resolve(); | |
| }; | |
| const timeoutId = window.setTimeout(() => { | |
| console.warn("Midgame ad timeout, continuing"); | |
| finish(); | |
| }, 8000); | |
| try { | |
| const callbacks = { | |
| adFinished: () => { | |
| console.log("End midgame ad"); | |
| finish(); | |
| }, | |
| adError: (error: any) => { | |
| console.log("Error midgame ad", error); | |
| finish(); | |
| }, | |
| adStarted: () => console.log("Start midgame ad"), | |
| }; | |
| window.CrazyGames!.SDK.ad.requestAd("midgame", callbacks); | |
| } catch (error) { | |
| console.error("Failed to request midgame ad:", error); | |
| finish(); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/CrazyGamesSDK.ts` around lines 308 - 332, The requestMidgameAd
promise can hang if the CrazyGames SDK never calls adFinished or adError; modify
requestMidgameAd to implement an 8‑second timeout and a local done flag so the
promise always resolves once: create a boolean done = false, wrap
adFinished/adError handlers (and the try/catch resolve) to check/set done before
resolving, start a setTimeout for 8000ms that resolves if not done and sets
done, and clearTimeout when the SDK callback runs so you never resolve twice;
update the callbacks object referenced in window.CrazyGames!.SDK.ad.requestAd
and ensure the catch path also follows the done flag logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/GameRightSidebar.ts`:
- Around line 106-113: The onPauseButtonClick handler flips isPaused and calls
crazyGamesSDK.gameplayStop()/gameplayStart() without awaiting them, so change
onPauseButtonClick to await the async SDK calls (await
crazyGamesSDK.gameplayStop() / await crazyGamesSDK.gameplayStart()) before
emitting the PauseGameIntentEvent to keep isPaused in sync; ensure the method is
declared async and still toggles this.isPaused, and optionally wrap the await in
try/catch to handle SDK errors while preserving the correct event emission.
In `@src/client/UsernameInput.ts`:
- Around line 41-48: The current connectedCallback calls
crazyGamesSDK.getUsername() and unconditionally uses "username ??
genAnonUsername()" which overwrites a previously stored username when the SDK
times out or returns null; change the logic in connectedCallback so that after
calling crazyGamesSDK.getUsername() you only call parseAndSetUsername(username)
(and persist/update) when username is non-null/defined (i.e., an actual
CrazyGames username); if the SDK returns null, do not call parseAndSetUsername
with genAnonUsername() — keep the already-loaded stored value from
getUsername(); ensure you still call this.requestUpdate() after handling the SDK
result. Use the existing symbols connectedCallback, crazyGamesSDK.getUsername,
parseAndSetUsername, getUsername, and genAnonUsername to find and update the
code.
♻️ Duplicate comments (8)
src/client/Matchmaking.ts (2)
250-259: Remove the userMeResponse listener on disconnect.This listener is never removed, so it can stack if the component is re-mounted.
307-324: Guard null usernameInput and provide a fallback error message.If the element is missing, the message can be empty.
💡 Suggested fix
- if (usernameInput?.isValid()) { + 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/graphics/layers/GameRightSidebar.ts (1)
116-127: Ensure exit redirect still happens if SDK calls fail.Wrap the SDK calls so errors do not block the redirect.
src/client/graphics/layers/SettingsModal.ts (2)
3-4: Use relative import paths for consistency.These absolute-style paths are inconsistent with the rest of this file.
108-116: If gameplayStop/start are async, await them.This avoids pause/resume calls completing out of order.
🔎 Verify SDK method signatures
#!/bin/bash # Inspect CrazyGamesSDK method declarations rg -n "gameplayStop|gameplayStart" -C 2 src/client/CrazyGamesSDK.ts♻️ Possible change if methods are async
- 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)); } }src/client/UsernameInput.ts (1)
49-54: Auth listener cleanup still missing (already flagged).Listener is added but never removed, so reconnecting the element can stack callbacks. This matches the earlier review note.
src/client/Main.ts (1)
619-626: Still missing username validation before opening host modal.Same issue as previously flagged: instant multiplayer calls
hostModal.open()without the validation used in the host button flow.src/client/CrazyGamesSDK.ts (1)
316-340: Midgame ad promise can still hang (already flagged).If the SDK never fires
adFinishedoradError, the promise never resolves.
🧹 Nitpick comments (3)
src/client/graphics/layers/UnitDisplay.ts (1)
131-134: Redundant responsive classes can be removed.The
xl:flex-colandxl:gap-2classes are redundant. Sincemin-[1200px](1200px) is smaller thanxl(1280px), themin-[1200px]:flex-colandmin-[1200px]:gap-2already cover thexlbreakpoint with identical values.♻️ Suggested cleanup
<div - 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] min-[1200px]:flex-col 2xl:flex-row min-[1200px]:gap-2 2xl:gap-5 justify-center items-center" >src/client/PatternInput.ts (1)
49-59: Skip cosmetics fetch on CrazyGames to avoid wasted network.You already render nothing there, so this work is unused.
♻️ Suggested change
async connectedCallback() { super.connectedCallback(); this._abortController = new AbortController(); this.isLoading = true; + if (crazyGamesSDK.isOnCrazyGames()) { + return; + } const cosmetics = await fetchCosmetics();src/client/Main.ts (1)
918-920: Replace the TODO with a real note or remove it.Right now it doesn’t explain intent.
If you want, I can draft a short comment that explains the condition.
| private onPauseButtonClick() { | ||
| this.isPaused = !this.isPaused; | ||
| if (this.isPaused) { | ||
| crazyGamesSDK.gameplayStop(); | ||
| } else { | ||
| crazyGamesSDK.gameplayStart(); | ||
| } | ||
| this.eventBus.emit(new PauseGameIntentEvent(this.isPaused)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and inspect the CrazyGamesSDK file
find . -name "*CrazyGames*" -type fRepository: openfrontio/OpenFrontIO
Length of output: 96
🏁 Script executed:
# Check the gameplayStop and gameplayStart method signatures
cat ./src/client/CrazyGamesSDK.tsRepository: openfrontio/OpenFrontIO
Length of output: 8705
🏁 Script executed:
# Search for onPauseButtonClick to see if it's called in a way that requires synchronous execution
rg -n "onPauseButtonClick" src/client/graphics/layers/GameRightSidebar.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 402
Await gameplayStop/Start to keep pause state in sync with SDK.
The gameplayStop() and gameplayStart() methods are async and return Promise<void>. Without awaiting them, the pause state can change before the SDK operations finish, causing them to complete out of order.
Suggested change
- private onPauseButtonClick() {
+ private async onPauseButtonClick() {
this.isPaused = !this.isPaused;
if (this.isPaused) {
- crazyGamesSDK.gameplayStop();
+ await crazyGamesSDK.gameplayStop();
} else {
- crazyGamesSDK.gameplayStart();
+ await crazyGamesSDK.gameplayStart();
}
this.eventBus.emit(new PauseGameIntentEvent(this.isPaused));
}📝 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 onPauseButtonClick() { | |
| this.isPaused = !this.isPaused; | |
| if (this.isPaused) { | |
| crazyGamesSDK.gameplayStop(); | |
| } else { | |
| crazyGamesSDK.gameplayStart(); | |
| } | |
| this.eventBus.emit(new PauseGameIntentEvent(this.isPaused)); | |
| private async onPauseButtonClick() { | |
| this.isPaused = !this.isPaused; | |
| if (this.isPaused) { | |
| await crazyGamesSDK.gameplayStop(); | |
| } else { | |
| await crazyGamesSDK.gameplayStart(); | |
| } | |
| this.eventBus.emit(new PauseGameIntentEvent(this.isPaused)); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/GameRightSidebar.ts` around lines 106 - 113, The
onPauseButtonClick handler flips isPaused and calls
crazyGamesSDK.gameplayStop()/gameplayStart() without awaiting them, so change
onPauseButtonClick to await the async SDK calls (await
crazyGamesSDK.gameplayStop() / await crazyGamesSDK.gameplayStart()) before
emitting the PauseGameIntentEvent to keep isPaused in sync; ensure the method is
declared async and still toggles this.isPaused, and optionally wrap the await in
try/catch to handle SDK errors while preserving the correct event emission.
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| const stored = this.getStoredUsername(); | ||
| const stored = this.getUsername(); | ||
| this.parseAndSetUsername(stored); | ||
| crazyGamesSDK.getUsername().then((username) => { | ||
| this.parseAndSetUsername(username ?? genAnonUsername()); | ||
| this.requestUpdate(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the UsernameInput.ts file
find . -name "UsernameInput.ts" -type f 2>/dev/null | head -5Repository: openfrontio/OpenFrontIO
Length of output: 96
🏁 Script executed:
# Also look for crazyGamesSDK related files
fd -e ts -e js | grep -i "crazy\|sdk" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 94
🏁 Script executed:
# Search for getUsername implementation
rg "getUsername" --type ts --type js -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 1088
🏁 Script executed:
# Read the complete UsernameInput.ts file to see context around lines 41-48
cat -n src/client/UsernameInput.ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 4073
🏁 Script executed:
# Check parseAndSetUsername implementation
rg "parseAndSetUsername" --type ts -A 5 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 1265
🏁 Script executed:
# Check for isOnCrazyGames method
rg "isOnCrazyGames" --type ts -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 3306
🏁 Script executed:
# Check the full CrazyGamesSDK implementation
cat -n src/client/CrazyGamesSDK.ts | head -150Repository: openfrontio/OpenFrontIO
Length of output: 5068
🏁 Script executed:
# Check genAnonUsername implementation
rg "genAnonUsername" --type ts -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 1591
🏁 Script executed:
# Check validateAndStore implementation to confirm it writes to localStorage
rg "validateAndStore" --type ts -A 8 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 1837
🏁 Script executed:
# Get more context on validateAndStore to see if it stores to localStorage
cat -n src/client/UsernameInput.ts | sed -n '100,150p'Repository: openfrontio/OpenFrontIO
Length of output: 2000
🏁 Script executed:
# Also check storeUsername method if it exists
rg "storeUsername" --type ts -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 883
Only update stored username if SDK provides a valid CrazyGames username.
On non-CrazyGames pages, getUsername() returns null after timing out. When this happens, the ?? genAnonUsername() fallback generates a new anonymous name and overwrites the user's previously stored username. This wastes the stored value.
Only call getUsername() and update the stored username when actually on CrazyGames. If the SDK returns null even on CrazyGames (user not logged in), keep the already-loaded stored username.
🔧 Suggested fix
- crazyGamesSDK.getUsername().then((username) => {
- this.parseAndSetUsername(username ?? genAnonUsername());
- this.requestUpdate();
- });
+ if (crazyGamesSDK.isOnCrazyGames()) {
+ crazyGamesSDK.getUsername().then((username) => {
+ if (username) {
+ this.parseAndSetUsername(username);
+ this.requestUpdate();
+ }
+ });
+ }🤖 Prompt for AI Agents
In `@src/client/UsernameInput.ts` around lines 41 - 48, The current
connectedCallback calls crazyGamesSDK.getUsername() and unconditionally uses
"username ?? genAnonUsername()" which overwrites a previously stored username
when the SDK times out or returns null; change the logic in connectedCallback so
that after calling crazyGamesSDK.getUsername() you only call
parseAndSetUsername(username) (and persist/update) when username is
non-null/defined (i.e., an actual CrazyGames username); if the SDK returns null,
do not call parseAndSetUsername with genAnonUsername() — keep the already-loaded
stored value from getUsername(); ensure you still call this.requestUpdate()
after handling the SDK result. Use the existing symbols connectedCallback,
crazyGamesSDK.getUsername, parseAndSetUsername, getUsername, and genAnonUsername
to find and update the code.
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan