Split runtime and game logic env loading#3505
Merged
evanpelle merged 1 commit intoopenfrontio:mainfrom Mar 24, 2026
Merged
Conversation
Contributor
WalkthroughThis pull request refactors configuration loading to separate build-time game logic configuration from runtime client-server configuration. New functions replace existing ones across the codebase: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/configuration/ConfigLoader.ts (2)
64-76: Consider removing or guarding the console.log.The
console.log("using default game logic config")on line 71 runs every timegetServerConfigForGameLogicEnv(GameLogicEnv.Default)is called. SincegetGameLogicConfig()calls this function, the log may appear frequently during normal operation.If this is purely for debugging during development, consider removing it or guarding it behind a debug flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/ConfigLoader.ts` around lines 64 - 76, The console.log inside getServerConfigForGameLogicEnv causes noisy output whenever GameLogicEnv.Default is used; remove the console.log("using default game logic config") or replace it with a gated debug/log call (e.g., use your application's logger or a DEBUG flag) so the message only emits when debugging is enabled; update the logic in getServerConfigForGameLogicEnv (and any callers like getGameLogicConfig) to use the logger/debug flag instead of unconditional console.log.
15-15: Avoid exporting mutable cache variable.The
cachedRuntimeClientServerConfigis exported as a mutablelet, which allows external code to overwrite it directly. This breaks encapsulation and could lead to subtle bugs.Consider keeping the cache private and exporting only the getter and clearer functions.
♻️ Suggested change
-export let cachedRuntimeClientServerConfig: ServerConfig | null = null; +let cachedRuntimeClientServerConfig: ServerConfig | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/ConfigLoader.ts` at line 15, The exported mutable variable cachedRuntimeClientServerConfig breaks encapsulation; make the variable private (remove export) and expose controlled accessors instead—add and export a getCachedRuntimeClientServerConfig() function to read the cache and a clearCachedRuntimeClientServerConfig() (and optionally a setCachedRuntimeClientServerConfig() used only internally) to mutate it; update any callers to use these functions and ensure the internal variable name (cachedRuntimeClientServerConfig) remains the single source of truth inside ConfigLoader.ts.src/client/GameModeSelector.ts (1)
61-63: Consider adding error handling for config load failure.If
getRuntimeClientServerConfig()rejects, the promise is unhandled anddefaultLobbyTimestays at 0. This might cause incorrect countdown displays.♻️ Suggested change
getRuntimeClientServerConfig().then((config) => { this.defaultLobbyTime = config.gameCreationRate() / 1000; - }); + }).catch((err) => { + console.warn("Failed to load runtime config for lobby time:", err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 61 - 63, The promise returned by getRuntimeClientServerConfig() can reject leaving this.defaultLobbyTime at 0; wrap the config load in proper error handling (either await inside an async init or add .catch) in GameModeSelector so rejections are handled, log the error, and set a sensible fallback for defaultLobbyTime (e.g., keep current default or a configured fallback) so the countdown still works; update the code that calls getRuntimeClientServerConfig() to handle errors and assign a fallback value to this.defaultLobbyTime and emit/log the failure for visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/GameModeSelector.ts`:
- Line 3: The import in GameModeSelector.ts uses an absolute path; change the
import of getRuntimeClientServerConfig to the same relative style used elsewhere
(e.g., import from ../core/configuration/ConfigLoader) so it matches other
client files and avoids bundler path issues—update the import statement that
references getRuntimeClientServerConfig accordingly.
---
Nitpick comments:
In `@src/client/GameModeSelector.ts`:
- Around line 61-63: The promise returned by getRuntimeClientServerConfig() can
reject leaving this.defaultLobbyTime at 0; wrap the config load in proper error
handling (either await inside an async init or add .catch) in GameModeSelector
so rejections are handled, log the error, and set a sensible fallback for
defaultLobbyTime (e.g., keep current default or a configured fallback) so the
countdown still works; update the code that calls getRuntimeClientServerConfig()
to handle errors and assign a fallback value to this.defaultLobbyTime and
emit/log the failure for visibility.
In `@src/core/configuration/ConfigLoader.ts`:
- Around line 64-76: The console.log inside getServerConfigForGameLogicEnv
causes noisy output whenever GameLogicEnv.Default is used; remove the
console.log("using default game logic config") or replace it with a gated
debug/log call (e.g., use your application's logger or a DEBUG flag) so the
message only emits when debugging is enabled; update the logic in
getServerConfigForGameLogicEnv (and any callers like getGameLogicConfig) to use
the logger/debug flag instead of unconditional console.log.
- Line 15: The exported mutable variable cachedRuntimeClientServerConfig breaks
encapsulation; make the variable private (remove export) and expose controlled
accessors instead—add and export a getCachedRuntimeClientServerConfig() function
to read the cache and a clearCachedRuntimeClientServerConfig() (and optionally a
setCachedRuntimeClientServerConfig() used only internally) to mutate it; update
any callers to use these functions and ensure the internal variable name
(cachedRuntimeClientServerConfig) remains the single source of truth inside
ConfigLoader.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a804c563-fcfa-468a-814c-bcb4358f90f2
📒 Files selected for processing (12)
src/client/AccountModal.tssrc/client/ClientGameRunner.tssrc/client/GameModeSelector.tssrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/LobbySocket.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/components/CopyButton.tssrc/core/GameRunner.tssrc/core/configuration/ConfigLoader.tstests/core/configuration/ConfigLoader.test.ts
| import { html, LitElement, nothing, type TemplateResult } from "lit"; | ||
| import { customElement, state } from "lit/decorators.js"; | ||
| import { getServerConfigFromClient } from "src/core/configuration/ConfigLoader"; | ||
| import { getRuntimeClientServerConfig } from "src/core/configuration/ConfigLoader"; |
Contributor
There was a problem hiding this comment.
Inconsistent import path style.
This file uses src/core/configuration/ConfigLoader (absolute-style path), while other client files in this PR use relative paths like ../core/configuration/ConfigLoader. This inconsistency could cause issues depending on bundler configuration.
🔧 Suggested fix
-import { getRuntimeClientServerConfig } from "src/core/configuration/ConfigLoader";
+import { getRuntimeClientServerConfig } from "../core/configuration/ConfigLoader";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/GameModeSelector.ts` at line 3, The import in GameModeSelector.ts
uses an absolute path; change the import of getRuntimeClientServerConfig to the
same relative style used elsewhere (e.g., import from
../core/configuration/ConfigLoader) so it matches other client files and avoids
bundler path issues—update the import statement that references
getRuntimeClientServerConfig accordingly.
Contributor
Author
There was a problem hiding this comment.
good spot, but out of scope for this
Contributor
There was a problem hiding this comment.
@scamiv, understood — happy to leave it for a later cleanup pass. Would you like me to open a follow-up issue to track the import path style inconsistency in src/client/GameModeSelector.ts?
🧠 Learnings used
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3349
File: src/client/HostLobbyModal.ts:649-660
Timestamp: 2026-03-04T20:14:53.404Z
Learning: In `src/client/HostLobbyModal.ts`, `putGameConfig()` dispatches the entire game config object as a full replacement via `CustomEvent("update-game-config")` on every call (not a partial merge). Fields set to `undefined` in the config (e.g. `startingGold`, `goldMultiplier`, `maxTimerValue`) correctly signal "not set / cleared" to the server. There is no stale-value risk when these fields are `undefined`.
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.
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()`.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 3298
File: src/server/Worker.ts:359-372
Timestamp: 2026-02-28T21:54:43.932Z
Learning: In src/server/Worker.ts, there are two distinct reconnect paths: (1) explicit "rejoin" messages (clientMsg.type === "rejoin") are sent during mid-game reconnects and don't contain a username field per the ClientRejoinMessage schema; (2) implicit reconnects via "join" messages (clientMsg.type === "join") from existing clients handle page refreshes in pre-game and do contain username. Only the implicit path should pass newUsername to gm.rejoinClient because the explicit rejoin message doesn't have that field.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2975
File: src/client/JoinPublicLobbyModal.ts:39-54
Timestamp: 2026-01-31T21:39:22.022Z
Learning: In src/client/JoinPublicLobbyModal.ts, the msUntilStart value received from lobby_info messages is a fixed timestamp (not a duration) set when the lobby is created on the server (this.createdAt + this.config.gameCreationRate()). It does not update on each broadcast. The client must subtract Date.now() to convert it to a relative duration before using it with countdown timers.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2975
File: src/client/JoinLobbyModal.ts:482-493
Timestamp: 2026-01-31T21:28:44.679Z
Learning: In src/client/JoinLobbyModal.ts, the msUntilStart value received from lobby_info messages is a duration in milliseconds (not a fixed timestamp). The client adds Date.now() to convert it to an absolute timestamp for storage in lobbyStartAt, then subtracts Date.now() when displaying the countdown to get the remaining seconds.
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.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
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.
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
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.
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.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
This refactors client configuration loading to make the environment split explicit.
The app currently has two different env concerns:
Before this change, both responsibilities were hidden behind the same loader, which made the intent unclear and caused confusion around the worker fallback behavior.
This PR separates those paths explicitly:
getRuntimeClientServerConfig()getGameLogicConfig()GameLogicEnvWhat Changed
GameLogicEnvto model the build-time game config choice explicitly.getRuntimeClientServerConfig()for live runtime browser config fromwindow.BOOTSTRAP_CONFIG.getBuildTimeGameLogicEnv()andgetServerConfigForGameLogicEnv()for build-time worker/game-logic config.getConfig()togetGameLogicConfig()to reflect what it actually does.Behavior
This keeps the current intended behavior, but makes it explicit:
Runtime client env:
window.BOOTSTRAP_CONFIGBuild-time game-logic env:
process.env.GAME_ENVdev-> dev game configstaging-> default/prod game configprod-> default/prod game configThat means preprod/staging deployments can continue using prod game logic while still using staging API/auth settings on the main thread.
Why
The previous setup worked, but the naming and loader boundaries were misleading:
This change makes that intent visible in code without changing the desired deployment behavior.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME