Inject config in index.html, remove /env endpoint #2875
Conversation
WalkthroughRefactors configuration management to unify server config retrieval through Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
evanpelle
force-pushed
the
evan-fix-num-workers
branch
from
c4a6aea to
37c5410
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/core/configuration/ConfigLoader.ts:
- Around line 23-25: The default branch throws an error referencing the global
Env.GAME_ENV instead of the function parameter gameEnv; update the thrown Error
in the default case to interpolate the local parameter (gameEnv) so the message
correctly reflects the input value — look for the switch's default branch in
ConfigLoader.ts and replace Env.GAME_ENV with gameEnv in the Error construction.
In @src/core/configuration/Env.ts:
- Around line 105-114: Env.GIT_COMMIT can be undefined, so in
JoinPrivateLobbyModal where myGitCommit is compared to "DEV" you must normalize
the value before comparing (e.g., set myGitCommit = Env.GIT_COMMIT ?? "DEV" or
otherwise handle undefined explicitly) to avoid treating undefined as a non-DEV
commit; mirror the approach used in DefaultConfig (Env.GIT_COMMIT ?? "") and
update the comparison logic that currently checks myGitCommit !== "DEV" to use
the normalized value.
In @vite.config.ts:
- Around line 59-63: serverConfig currently uses parseInt(env.NUM_WORKERS, 10)
(which can produce NaN) and directly uses env.GIT_COMMIT (bypassing the earlier
fallback); update the serverConfig construction so numWorkers uses a safe parse
with a fallback (e.g., Number.isFinite/Number and default to 2 or the non-prod
default when parseInt yields NaN) and reuse the already-computed gitCommit
fallback value instead of env.GIT_COMMIT, referencing the serverConfig object
and the numWorkers and gitCommit keys to locate where to change.
🧹 Nitpick comments (10)
.vscode/settings.json (1)
1-3: Verify this workspace setting is necessary and document why.Disabling HTML script validation affects all HTML files in the workspace. If this was added to suppress false positives from the new
window.SERVER_CONFIGinjection approach, consider:
- Documenting the reason in a comment or README
- Whether a more targeted approach exists (e.g., inline directives for specific files)
- The trade-off: legitimate script errors in HTML won't be caught by VSCode
Can you confirm this setting is needed for the config injection changes? If so, please add a code comment or documentation explaining why, so future maintainers understand the reasoning.
src/server/Server.ts (1)
12-12: Consider storing or documenting the discarded return value.The call to
getServerConfig()returns aServerConfigobject that is discarded. If this is intentional for early validation and logging (to fail fast on misconfiguration), add a brief comment. Otherwise, consider storing it if needed downstream.- getServerConfig(); + // Validate environment and log active configuration before forking workers + getServerConfig();tests/core/pathfinding/_fixtures.ts (1)
3-3: Mixed import styles in the file need consistency.This file uses both
"src/core/...imports (line 3) and relative paths like"../../../src/core/..."(lines 5-10). Both work due tobaseUrl: "."in tsconfig, but the inconsistency within the same file reduces readability. Consider standardizing on one style—either use relative paths for all imports or use the"src/..."pattern consistently.src/client/JoinPrivateLobbyModal.ts (2)
515-516:getServerConfig()is synchronous - no need forawait.Looking at
ConfigLoader.ts,getServerConfig()returnsServerConfigdirectly, not aPromise<ServerConfig>. Theawaitis not needed here.♻️ Suggested fix
private async checkActiveLobby(lobbyId: string): Promise<boolean> { - const config = await getServerConfig(); + const config = getServerConfig(); const url = `/${config.workerPath(lobbyId)}/api/game/${lobbyId}/exists`;
598-603: Same here -getServerConfig()is synchronous.This method can be simplified since
getServerConfig()does not return a Promise.♻️ Suggested fix
private async pollPlayers() { const lobbyId = this.currentLobbyId; if (!lobbyId) return; - const config = await getServerConfig(); + const config = getServerConfig(); fetch(`/${config.workerPath(lobbyId)}/api/game/${lobbyId}`, {src/client/Main.ts (3)
738-738:getServerConfig()returns synchronously -awaitis not needed.The new
getServerConfig()function returnsServerConfigdirectly. No Promise here.♻️ Suggested fix
- const config = await getServerConfig(); + const config = getServerConfig();
895-904: Same pattern - unnecessaryawaiton synchronous call.♻️ Suggested fix
- const config = await getServerConfig(); + const config = getServerConfig();
958-958: Third occurrence of unnecessaryawait.♻️ Suggested fix
- const config = await getServerConfig(); + const config = getServerConfig();src/core/configuration/Env.ts (1)
59-85: Console logs on every access will spam the logs.The
GAME_ENVgetter logs every time it is accessed. Since this is a getter, each call triggers a log. In a long session, this creates noise.Consider caching the result or removing the logs after initial debugging.
♻️ Suggested fix - cache the value
+let cachedGameEnv: GameEnv | null = null; + export const Env = { get GAME_ENV(): GameEnv { + if (cachedGameEnv !== null) { + return cachedGameEnv; + } + // Check window.SERVER_CONFIG first (injected at build time) try { if (typeof window !== "undefined" && window.SERVER_CONFIG) { console.log("using server config from window"); - return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; + cachedGameEnv = GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; + return cachedGameEnv; } } catch { // Ignore errors accessing window } // Check MODE for Vite, GAME_ENV for Node try { if ( typeof import.meta !== "undefined" && import.meta.env && import.meta.env.MODE ) { console.log("using server config from import.meta.env"); - return GameEnv[import.meta.env.MODE as keyof typeof GameEnv]; + cachedGameEnv = GameEnv[import.meta.env.MODE as keyof typeof GameEnv]; + return cachedGameEnv; } } catch { // Ignore errors accessing import.meta } console.log("using server config from environment variable"); - return GameEnv[getEnv("GAME_ENV") as keyof typeof GameEnv]; + cachedGameEnv = GameEnv[getEnv("GAME_ENV") as keyof typeof GameEnv]; + return cachedGameEnv; },src/core/configuration/ConfigLoader.ts (1)
31-39: Logging on every call togetServerConfig()will create noise.Similar to
Env.GAME_ENV, these console.log statements fire on every call. Consider caching the server config or removing the logs.♻️ Optional: cache the server config
+let cachedServerConfig: ServerConfig | null = null; + export function getServerConfig(): ServerConfig { + if (cachedServerConfig !== null) { + return cachedServerConfig; + } + const gameEnv = Env.GAME_ENV; switch (gameEnv) { case GameEnv.Dev: console.log("using dev server config"); - return new DevServerConfig(); + cachedServerConfig = new DevServerConfig(); + return cachedServerConfig; case GameEnv.Preprod: console.log("using preprod server config"); - return preprodConfig; + cachedServerConfig = preprodConfig; + return cachedServerConfig; case GameEnv.Prod: console.log("using prod server config"); - return prodConfig; + cachedServerConfig = prodConfig; + return cachedServerConfig; default: throw Error(`unsupported server configuration: ${gameEnv}`); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.vscode/launch.json.vscode/settings.jsonindex.htmlnginx.confpackage.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/OtelResource.tssrc/server/Server.tssrc/server/Worker.tssrc/server/WorkerMetrics.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/util/Setup.tsvite.config.ts
💤 Files with no reviewable changes (1)
- nginx.conf
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
index.html
📚 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/core/worker/Worker.worker.tssrc/server/Logger.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Matchmaking.tssrc/client/graphics/layers/MultiTabModal.tssrc/server/Archive.tssrc/server/WorkerMetrics.tssrc/core/configuration/Config.tssrc/server/Server.tssrc/core/GameRunner.tssrc/core/configuration/Env.tstests/core/pathfinding/_fixtures.tssrc/server/Worker.tssrc/core/worker/WorkerMessages.tssrc/server/MapPlaylist.tssrc/core/worker/WorkerClient.tssrc/server/OtelResource.tsvite.config.tssrc/server/Master.tssrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DevConfig.tssrc/core/configuration/DefaultConfig.tssrc/client/Main.tstests/util/Setup.tstests/pathfinding/utils.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/JoinPrivateLobbyModal.tssrc/client/graphics/layers/MultiTabModal.tssrc/client/HostLobbyModal.tssrc/client/Main.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-12-26T22:21:21.904Z
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").
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/HostLobbyModal.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/Matchmaking.tssrc/core/configuration/DevConfig.tssrc/client/Main.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/server/Archive.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:
tests/core/pathfinding/_fixtures.tssrc/client/HostLobbyModal.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/ClientGameRunner.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-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.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 (17)
src/server/Logger.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/client/Matchmaking.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Archive.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/WorkerMetrics.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Server.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/core/GameRunner.ts (4)
src/core/Schemas.ts (1)
ClientID(26-26)src/core/game/GameMapLoader.ts (1)
GameMapLoader(4-6)src/core/game/GameUpdates.ts (2)
GameUpdateViewData(17-23)ErrorUpdate(25-28)src/core/configuration/ConfigLoader.ts (1)
getConfig(10-26)
src/server/Worker.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/MapPlaylist.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/core/worker/WorkerClient.ts (2)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)
src/server/OtelResource.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
vite.config.ts (2)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)
src/server/Master.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/client/ClientGameRunner.ts (2)
src/core/configuration/ConfigLoader.ts (1)
getConfig(10-26)src/core/configuration/Env.ts (1)
Env(58-133)
src/client/HostLobbyModal.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/core/configuration/ConfigLoader.ts (4)
src/core/configuration/DefaultConfig.ts (4)
gameConfig(278-280)userSettings(282-287)isReplay(261-263)DefaultConfig(243-1000)src/core/configuration/DevConfig.ts (1)
DevConfig(50-59)src/core/configuration/Env.ts (1)
Env(58-133)src/core/configuration/PreprodConfig.ts (1)
preprodConfig(4-26)
src/core/configuration/DevConfig.ts (1)
src/core/configuration/DefaultConfig.ts (1)
isReplay(261-263)
src/client/Main.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
⏰ 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 (33)
package.json (1)
8-11: LGTM!The environment value capitalization (
Dev) now matches theGameEnvenum naming convention. All dev-related scripts are consistently updated.src/core/configuration/Config.ts (2)
23-27: Simple typed enum for environment - clean approach.Using a typed enum (
GameEnv) with explicit values instead of string literals makes the environment handling type-safe and self-documenting.
70-71: LGTM!Adding
env(): GameEnvto theConfiginterface provides a clean way to access the runtime environment from game logic.src/core/worker/WorkerMessages.ts (2)
1-1: LGTM!Clean import of
GameEnvtype for use in the message interface.
40-45: Clean extension of InitMessage.Adding
env: GameEnvto theInitMessageinterface allows the worker to receive environment context during initialization. Using the typed enum keeps the message contract type-safe.index.html (1)
59-62: Config injection is safe: all values are server-controlled and JSON-stringified.The
serverConfigobject contains only environment variables (GAME_ENV,NUM_WORKERS,GIT_COMMIT) set at build time, never user input. UsingJSON.stringify()properly escapes dangerous characters, and the unescaped<%-syntax is correct for JSON output—escaping would break the JSON structure. No XSS risk.src/core/configuration/DefaultConfig.ts (1)
246-255: Good simplification using typed value instead of complex object.The change from a
ServerConfigparameter to aGameEnvparameter makes the dependency simpler and cleaner. Using a typed enum value instead of a full configuration object is a better composition pattern. All call sites have been updated correctly to pass theGameEnvvalue..vscode/launch.json (1)
25-25: LGTM!The environment value now matches the
GameEnv.Devenum key used in the new configuration system. This alignment is required for theConfigLoaderswitch statement to work correctly.src/client/graphics/layers/MultiTabModal.ts (1)
34-34: LGTM!The environment check now uses the direct
env()accessor on the game config, consistent with the unified configuration API. TheGameEnv.Devcomparison is correct.src/server/WorkerMetrics.ts (1)
7-7: LGTM!Import and usage updated to the consolidated
getServerConfig()API. The function behavior remains the same—it reads fromEnv.GAME_ENVand returns the appropriateServerConfig.Also applies to: 15-15
src/client/ClientGameRunner.ts (3)
1-1: LGTM!The
Envimport provides access toGAME_ENV, which reads fromwindow.SERVER_CONFIG(injected at build time) in the browser context. This is the correct pattern for the new configuration flow.
186-191: LGTM!The
getConfigcall now passesEnv.GAME_ENVas the second argument, matching the updated signaturegetConfig(gameConfig, gameEnv, userSettings, isReplay).
203-207: LGTM!The
WorkerClientconstructor receivesEnv.GAME_ENVto propagate the environment to the worker, consistent with the updated worker initialization pattern.src/server/Worker.ts (1)
10-10: LGTM!The import rename from
getServerConfigFromServertogetServerConfigis consistent with the broader refactor. The server-side config initialization at module level is appropriate here since workers run as separate processes.Also applies to: 33-33
src/core/worker/WorkerClient.ts (2)
72-78: LGTM!The
envproperty is correctly included in the init message, matching theInitMessagetype defined inWorkerMessages.ts.
23-27: LGTM! Constructor updated to acceptGameEnvparameter.The new
envparameter makes environment passing explicit, avoiding runtime config lookups. The single call site inClientGameRunner.ts(line 203) correctly passes the env parameter. Change is complete and consistent.tests/core/pathfinding/_fixtures.ts (1)
149-154: LGTM!The
TestConfigconstructor now receivesGameEnv.Devdirectly instead of aTestServerConfiginstance. This simplifies test setup by removing the server config dependency.src/server/MapPlaylist.ts (1)
1-1: LGTM!The import and usage update from
getServerConfigFromServertogetServerConfigis consistent with the broader refactor pattern seen across the codebase.Also applies to: 20-20
src/server/Master.ts (1)
9-9: LGTM!The import and call site updates align with the new
getServerConfig()API. Calling it at module load time on the server is fine sinceEnv.GAME_ENVis available at startup.Also applies to: 15-15
src/core/GameRunner.ts (1)
36-43: LGTM!The new
env: GameEnvparameter is correctly passed togetConfig(). The signature now explicitly requires the environment, making configuration resolution clear and testable.src/server/Archive.ts (1)
2-2: LGTM!Import and usage updates are consistent with the new config API pattern used across the server codebase.
Also applies to: 13-13
src/server/OtelResource.ts (1)
6-8: LGTM!Import and call site updates follow the same pattern as other server modules.
src/core/worker/Worker.worker.ts (1)
44-56: Code change is correct. Themessage.envparameter matches the updatedcreateGameRunnersignature andInitMessagetype includes theenv: GameEnvproperty as expected.tests/util/Setup.ts (1)
3-3: LGTM!Clean switch from
ServerConfigtoGameEnv.Dev. The test setup now aligns with the new environment-based configuration pattern.Also applies to: 73-78
src/server/Logger.ts (1)
10-14: LGTM!Import and call site updated to use the renamed
getServerConfig()API. Server-side module initialization is fine here.src/core/configuration/DevConfig.ts (1)
50-58: LGTM!Good refactoring to accept
GameEnvinstead ofServerConfig. Using a typed enum keeps the constructor simple and avoids passing object dependencies when a plain value suffices.src/client/HostLobbyModal.ts (2)
1159-1159: Same concern: verify ifawaitis necessary forgetServerConfig().Per the earlier comment on
Matchmaking.ts, ifgetServerConfig()is synchronous, theseawaitcalls are unnecessary. The verification script in the other file will cover this.Also applies to: 1181-1181, 1234-1234
1248-1252: Nice improvement to error handling.Good addition of
response.okcheck with error text logging before throwing. This makes debugging failed lobby creation much easier.src/client/Matchmaking.ts (1)
146-146: Remove unnecessaryawaitfrom synchronousgetServerConfig()calls.
getServerConfig()returnsServerConfigsynchronously, not a Promise. Removeawaitat lines 146 and 225.Also applies to: 225-225
⛔ Skipped due to learnings
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()`.src/client/JoinPrivateLobbyModal.ts (1)
572-575: Good simplification usingEnv.GIT_COMMIT.Replacing the fetch-based git commit retrieval with
Env.GIT_COMMITis cleaner. The version check logic is preserved correctly.src/core/configuration/Env.ts (1)
9-13: Good use of a simple interface for config vars.Clean, typed interface. This is the right approach over class hierarchies.
tests/pathfinding/utils.ts (1)
251-271: Test config updated correctly for new constructor signature.The change to pass
GameEnv.Devas the second argument aligns with the refactoredTestConfigandDefaultConfigconstructors shown in the relevant code snippets.src/core/configuration/ConfigLoader.ts (1)
28-43:getServerConfig()is now synchronous - good simplification.The function reads
Env.GAME_ENVdirectly and returns the appropriate config. This is cleaner than the previous async approach. The switch cases cover allGameEnvvalues correctly.
| get GIT_COMMIT(): string | undefined { | ||
| try { | ||
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | ||
| return window.SERVER_CONFIG.gitCommit; | ||
| } | ||
| } catch { | ||
| // Ignore errors accessing window | ||
| } | ||
| return getEnv("GIT_COMMIT"); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "Env\.GIT_COMMIT" --type=ts -C2Repository: openfrontio/OpenFrontIO
Length of output: 682
🏁 Script executed:
# Also check the exact usage at JoinPrivateLobbyModal.ts:572
fd "JoinPrivateLobbyModal.ts" --type=f -x cat -n {} | sed -n '565,580p'Repository: openfrontio/OpenFrontIO
Length of output: 728
🏁 Script executed:
# Check the Env.ts file to see the exact return type and implementation
fd "Env.ts" --type=f -path "*/core/configuration/*" -x cat -n {}Repository: openfrontio/OpenFrontIO
Length of output: 235
🏁 Script executed:
cat -n src/core/configuration/Env.ts | head -120Repository: openfrontio/OpenFrontIO
Length of output: 3955
🏁 Script executed:
# Search for all GIT_COMMIT usages without limiting to ts type
rg -n "GIT_COMMIT" -C2Repository: openfrontio/OpenFrontIO
Length of output: 2824
🏁 Script executed:
# Check if there are other problematic usages of GIT_COMMIT without null handling
rg -n "myGitCommit|Env\.GIT_COMMIT" --type-list all -B3 -A3Repository: openfrontio/OpenFrontIO
Length of output: 456
🏁 Script executed:
# Check all usages of myGitCommit and Env.GIT_COMMIT
rg -n "myGitCommit|Env\.GIT_COMMIT" -B3 -A3Repository: openfrontio/OpenFrontIO
Length of output: 1273
Handle undefined git commit in version comparison
Env.GIT_COMMIT returns string | undefined. In JoinPrivateLobbyModal.ts at line 575, the comparison myGitCommit !== "DEV" will evaluate to true when myGitCommit is undefined, causing the code to enter the version mismatch check even when the git commit is missing. This can trigger false version mismatch errors.
Use a fallback value instead, similar to DefaultConfig.ts line 153 which uses Env.GIT_COMMIT ?? "", or add explicit undefined handling:
const myGitCommit = Env.GIT_COMMIT ?? "DEV";
This ensures undefined cases are treated as development builds and won't cause unexpected version errors.
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 105 - 114, Env.GIT_COMMIT can be
undefined, so in JoinPrivateLobbyModal where myGitCommit is compared to "DEV"
you must normalize the value before comparing (e.g., set myGitCommit =
Env.GIT_COMMIT ?? "DEV" or otherwise handle undefined explicitly) to avoid
treating undefined as a non-DEV commit; mirror the approach used in
DefaultConfig (Env.GIT_COMMIT ?? "") and update the comparison logic that
currently checks myGitCommit !== "DEV" to use the normalized value.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/core/configuration/ConfigLoader.ts:
- Around line 23-25: The default case in the switch that throws an error uses
the global Env.GAME_ENV instead of the switch parameter gameEnv; update the
thrown Error in ConfigLoader (the switch handling gameEnv) to reference the
function parameter (gameEnv) so the message accurately reports the unsupported
value (e.g., throw Error(`unsupported server configuration: ${gameEnv}`)).
In @src/core/configuration/Env.ts:
- Around line 121-132: The NUM_WORKERS getter currently returns a string by
calling String(window.SERVER_CONFIG.numWorkers) but ServerConfigVars.numWorkers
is typed as number, causing a type mismatch; update the getter to return a
number (or number | undefined) by removing the String() conversion and
coercing/parsing as needed (e.g., return window.SERVER_CONFIG.numWorkers
directly when present and otherwise parse or convert the value from
getEnv("NUM_WORKERS") to a number), ensuring the getter's TypeScript signature
matches the chosen return type; adjust any callers or the getter's declared type
accordingly so NUM_WORKERS and ServerConfigVars.numWorkers are consistent.
- Line 64: The enum lookup GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof
GameEnv] is unsafe and can return undefined; update the getter(s) to validate
the raw string from window.SERVER_CONFIG.gameEnv (and the similar lookups at the
other two sites) before casting—check whether the string is a valid key/value of
GameEnv (e.g., via Object.keys/values or a membership check), and if invalid
throw a clear error including the invalid value; ensure the function(s) that use
GameEnv (the lines referencing GameEnv[...] at line 64 and the similar lookups
at lines 78 and 84) return a valid GameEnv or raise an error instead of
returning undefined.
- Around line 59-85: The GAME_ENV getter on Env currently logs every access
which floods logs; change it to compute the environment once and cache the
result in a private static field (e.g. add a private static _cachedGameEnv:
GameEnv | null) and have GAME_ENV return that cached value after the first
resolution; remove or replace the console.log calls inside GAME_ENV and ensure
the final fallback still uses getEnv("GAME_ENV") and assigns the resolved enum
value to _cachedGameEnv before returning.
🧹 Nitpick comments (2)
vite.config.ts (1)
59-63: Consider adding a fallback forparseIntto avoidNaN.If
env.NUM_WORKERSis undefined or not a valid number in production,parseIntwill returnNaN. This could cause unexpected behavior downstream.🔧 Suggested fix with fallback
serverConfig: JSON.stringify({ gameEnv: isProduction ? env.GAME_ENV : "Dev", - numWorkers: isProduction ? parseInt(env.NUM_WORKERS, 10) : 2, + numWorkers: isProduction ? (parseInt(env.NUM_WORKERS, 10) || 1) : 2, gitCommit: isProduction ? env.GIT_COMMIT : "DEV", }),src/core/configuration/ConfigLoader.ts (1)
31-39: Inconsistent instantiation: Dev creates new instance, others use singletons.
preprodConfigandprodConfigare reused singleton objects, butDevServerConfigis instantiated fresh on every call. The parentDefaultServerConfigcaches thepublicKeyafter fetching JWKS. Creating new instances could cause repeated JWKS fetches.Consider making
DevServerConfiga singleton like the others for consistency.Proposed fix
+const devServerConfig = new DevServerConfig(); + export function getServerConfig(): ServerConfig { const gameEnv = Env.GAME_ENV; switch (gameEnv) { case GameEnv.Dev: console.log("using dev server config"); - return new DevServerConfig(); + return devServerConfig; case GameEnv.Preprod:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.vscode/launch.json.vscode/settings.jsonindex.htmlnginx.confpackage.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/OtelResource.tssrc/server/Server.tssrc/server/Worker.tssrc/server/WorkerMetrics.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/util/Setup.tsvite.config.ts
💤 Files with no reviewable changes (1)
- nginx.conf
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (11)
- src/server/Server.ts
- src/core/worker/WorkerClient.ts
- src/core/GameRunner.ts
- src/client/Matchmaking.ts
- tests/core/pathfinding/_fixtures.ts
- index.html
- src/client/Main.ts
- .vscode/launch.json
- src/server/MapPlaylist.ts
- tests/util/Setup.ts
- tests/pathfinding/utils.ts
🧰 Additional context used
🧠 Learnings (12)
📚 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/server/Logger.tssrc/server/Master.tssrc/server/Worker.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/WorkerMetrics.tsvite.config.tssrc/server/OtelResource.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/HostLobbyModal.tssrc/core/configuration/DevConfig.tssrc/core/configuration/ConfigLoader.tssrc/client/JoinPrivateLobbyModal.tssrc/client/graphics/layers/MultiTabModal.tssrc/client/ClientGameRunner.tssrc/core/configuration/Env.tssrc/core/worker/Worker.worker.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
vite.config.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/ConfigLoader.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/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.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.ts
📚 Learning: 2025-12-26T22:21:21.904Z
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").
Applied to files:
src/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.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/HostLobbyModal.tssrc/core/configuration/DevConfig.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/JoinPrivateLobbyModal.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/ClientGameRunner.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/core/configuration/Env.ts
🧬 Code graph analysis (11)
src/server/Logger.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Master.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Worker.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Archive.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/WorkerMetrics.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
vite.config.ts (2)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)
src/server/OtelResource.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/client/HostLobbyModal.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/core/configuration/ConfigLoader.ts (5)
src/core/configuration/DefaultConfig.ts (4)
gameConfig(278-280)userSettings(282-287)isReplay(261-263)DefaultConfig(243-1000)src/core/configuration/Config.ts (2)
Config(70-181)ServerConfig(29-63)src/core/configuration/DevConfig.ts (2)
DevConfig(50-59)DevServerConfig(6-48)src/core/configuration/Env.ts (1)
Env(58-133)src/core/configuration/PreprodConfig.ts (1)
preprodConfig(4-26)
src/client/JoinPrivateLobbyModal.ts (2)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)src/core/configuration/Env.ts (1)
Env(58-133)
src/client/ClientGameRunner.ts (4)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getConfig(10-26)src/core/configuration/Env.ts (1)
Env(58-133)
🔇 Additional comments (27)
package.json (1)
8-11: LGTM!The
GAME_ENV=Devchange aligns with theGameEnv.Devenum value, keeping naming consistent across the codebase.src/core/configuration/Config.ts (2)
23-27: LGTM! Clean enum definition.Simple typed union via enum is a good pattern here.
70-71: AllConfigimplementations properly updated with theenv()method.Verification confirms:
DefaultConfig(implementsConfig) includesenv(): GameEnvDefaultServerConfig(abstract, implementsServerConfig) definesabstract env(): GameEnv- All concrete subclasses (
DevConfig,ProdConfig,PreprodConfig) implementenv()TestServerConfig(test implementation) includesenv()The breaking change is complete across the codebase. No implementations are missing the method.
src/core/worker/WorkerMessages.ts (2)
1-1: LGTM!Import is correctly scoped to just the
GameEnvenum.
40-45: LGTM! Clean interface extension.Adding
env: GameEnvtoInitMessageproperly threads the environment through worker initialization.src/client/graphics/layers/MultiTabModal.ts (1)
30-37: LGTM! Cleaner access pattern.The change from
config().serverConfig().env()toconfig().env()simplifies the call chain while preserving the same behavior.src/server/WorkerMetrics.ts (1)
7-7: LGTM!The import and usage are updated correctly. Good practice to retrieve config inside the function rather than at module level—this ensures
Env.GAME_ENVis set when the function runs.Also applies to: 15-15
src/server/Worker.ts (1)
10-10: LGTM!The migration to
getServerConfig()is correct. Module-level initialization works fine for server-side code since environment variables are set before the process starts.Also applies to: 33-33
src/server/Archive.ts (1)
2-2: LGTM!Clean migration to the new
getServerConfig()API. Consistent with the pattern used in other server modules.Also applies to: 13-13
src/server/OtelResource.ts (1)
6-6: LGTM!Straightforward API migration. The module-level config pattern is consistent with other server files.
Also applies to: 8-8
src/server/Master.ts (1)
9-9: LGTM!The API migration is correct. The removal of the
/api/envendpoint (as noted in the summary) aligns with the new approach of injecting server config viawindow.SERVER_CONFIGon the client side.Also applies to: 15-15
src/client/ClientGameRunner.ts (3)
1-1: LGTM!Clean import of
Envfor accessing the game environment. This aligns well with the refactor to use a single source of truth for configuration.
186-191: LGTM!Passing
Env.GAME_ENVas the second parameter togetConfigis consistent with the updated API. The environment is now sourced fromwindow.SERVER_CONFIG(injected at build time) or falls back appropriately.
203-207: LGTM!The
WorkerClientnow receivesEnv.GAME_ENVas the second parameter, threading the environment through to the worker initialization. This keeps the configuration flow consistent.src/core/worker/Worker.worker.ts (1)
44-50: LGTM!The worker now receives
envfrom the init message and passes it tocreateGameRunner. This keeps the environment configuration consistent between main thread and worker.src/server/Logger.ts (1)
10-14: LGTM!The rename from
getServerConfigFromServertogetServerConfigsimplifies the API surface. On the server side,Env.GAME_ENVwill read from environment variables sincewindowis not available.src/client/JoinPrivateLobbyModal.ts (5)
3-3: LGTM!Clean import of
Envfor accessing configuration values likeGIT_COMMIT.
12-12: LGTM!The rename from
getServerConfigFromClienttogetServerConfigconsolidates the API. On the client side,Env.GAME_ENVreads fromwindow.SERVER_CONFIGwhich is injected at build time.
515-516: LGTM!Using
getServerConfig()here is consistent with the refactored API.
549-582: Nice simplification!Replacing the parallel git commit fetch with
Env.GIT_COMMITremoves async complexity. The git commit is now synchronously available fromwindow.SERVER_CONFIG.gitCommit, which is injected at build time.
601-603: LGTM!Consistent use of
getServerConfig()for the worker path lookup.src/client/HostLobbyModal.ts (4)
4-4: LGTM!The rename from
getServerConfigFromClienttogetServerConfigkeeps the API surface clean and consistent.
1159-1161: LGTM!Consistent use of
getServerConfig()for the worker path in game start.
1181-1182: LGTM!Consistent use of
getServerConfig()for the worker path in player polling.
1233-1258: Good improvement on error handling!The
createLobbyfunction now checksresponse.okand logs the server error text before throwing. This makes debugging failed lobby creation much easier.src/core/configuration/DefaultConfig.ts (1)
246-256: Clean simplification - LGTM!Replacing the full
ServerConfigdependency with just theGameEnvenum is a solid refactor. The class now holds only what it needs. Good use of composition.src/core/configuration/DevConfig.ts (1)
50-58: LGTM!Clean pass-through of
gameEnvto the parent class. This aligns well with the DefaultConfig constructor changes.
src/core/configuration/Env.ts
Outdated
| try { | ||
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | ||
| console.log("using server config from window"); | ||
| return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; |
There was a problem hiding this comment.
Unsafe enum lookup may return undefined.
If window.SERVER_CONFIG.gameEnv contains an unexpected string (typo, misconfiguration), this lookup returns undefined while the return type says GameEnv. This could cause hard-to-debug issues downstream.
Proposed fix: validate and throw on invalid value
- return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv];
+ const env = GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv];
+ if (env === undefined) {
+ throw new Error(`Invalid GAME_ENV value: ${window.SERVER_CONFIG.gameEnv}`);
+ }
+ return env;Apply similar validation at lines 78 and 84.
📝 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.
| return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; | |
| const env = GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; | |
| if (env === undefined) { | |
| throw new Error(`Invalid GAME_ENV value: ${window.SERVER_CONFIG.gameEnv}`); | |
| } | |
| return env; |
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts at line 64, The enum lookup
GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv] is unsafe and can
return undefined; update the getter(s) to validate the raw string from
window.SERVER_CONFIG.gameEnv (and the similar lookups at the other two sites)
before casting—check whether the string is a valid key/value of GameEnv (e.g.,
via Object.keys/values or a membership check), and if invalid throw a clear
error including the invalid value; ensure the function(s) that use GameEnv (the
lines referencing GameEnv[...] at line 64 and the similar lookups at lines 78
and 84) return a valid GameEnv or raise an error instead of returning undefined.
| get NUM_WORKERS() { | ||
| // Check window.SERVER_CONFIG first (injected at build time) | ||
| try { | ||
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | ||
| return String(window.SERVER_CONFIG.numWorkers); | ||
| } | ||
| } catch { | ||
| // Ignore errors accessing window | ||
| } | ||
|
|
||
| return getEnv("NUM_WORKERS"); | ||
| }, |
There was a problem hiding this comment.
NUM_WORKERS returns string but interface defines number.
ServerConfigVars.numWorkers is typed as number (line 11), but this getter converts it to String() at line 125. This inconsistency may confuse callers. Either:
- Return
number | undefinedand skip theString()conversion, or - Document/type the getter return as
string | undefinedif string is intentional
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 121 - 132, The NUM_WORKERS getter
currently returns a string by calling String(window.SERVER_CONFIG.numWorkers)
but ServerConfigVars.numWorkers is typed as number, causing a type mismatch;
update the getter to return a number (or number | undefined) by removing the
String() conversion and coercing/parsing as needed (e.g., return
window.SERVER_CONFIG.numWorkers directly when present and otherwise parse or
convert the value from getEnv("NUM_WORKERS") to a number), ensuring the getter's
TypeScript signature matches the chosen return type; adjust any callers or the
getter's declared type accordingly so NUM_WORKERS and
ServerConfigVars.numWorkers are consistent.
37c5410 to
d56fc5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/deploy.yml:
- Line 113: The ENV value in the workflow currently emits capitalized
'Prod'/'Staging', which will be rejected by the downstream scripts; change the
GitHub Actions expression that sets ENV so it outputs lowercase 'prod' and
'staging' instead of 'Prod'/'Staging' (i.e., update the ENV assignment
expression that uses inputs.target_domain to return 'prod' or 'staging'),
ensuring consistency with the strict lowercase checks in build-deploy.sh,
build.sh, and deploy.sh.
In @src/core/configuration/ConfigLoader.ts:
- Around line 23-25: The default case in the switch inside ConfigLoader.ts
throws an error referencing Env.GAME_ENV instead of the actual function
parameter; update the throw in that default branch to include the incoming
gameEnv variable (the parameter named gameEnv) so the error message reflects the
actual argument that triggered the unsupported configuration, e.g. replace
Env.GAME_ENV with gameEnv in the thrown Error in the default case of the switch.
In @src/core/configuration/Env.ts:
- Around line 59-85: The GAME_ENV getter can return undefined when mapping
strings to the GameEnv enum; validate each lookup (for
window.SERVER_CONFIG.gameEnv, import.meta.env.MODE, and getEnv("GAME_ENV")) by
checking that the mapped value is a valid GameEnv member before returning it,
and otherwise fall back to a safe default or throw a clear error; update the get
GAME_ENV implementation to compute the candidate key, verify via something like
Object.hasOwnProperty or a reverse-lookup on GameEnv, and only return
GameEnv[candidate] when valid, otherwise continue to the next source or return a
validated default.
In @vite.config.ts:
- Around line 59-63: The serverConfig construction sets numWorkers using
parseInt(env.NUM_WORKERS, 10) which yields NaN if NUM_WORKERS is missing in
production; update the numWorkers logic in the serverConfig block to use a safe
fallback (e.g., parseInt(env.NUM_WORKERS || "2", 10) or
parseInt(env.NUM_WORKERS, 10) with a Number.isNaN check) so production falls
back to 2 when env.NUM_WORKERS is undefined or invalid.
🧹 Nitpick comments (6)
src/core/configuration/PreprodConfig.ts (1)
4-7: Consider renaming file and class to match the new Staging enum value.The file is named
PreprodConfig.tsand the class ispreprodConfig, but it now returnsGameEnv.Staging. This mismatch might confuse future maintainers. Consider renaming toStagingConfig.tsandstagingConfigfor clarity.src/client/JoinPrivateLobbyModal.ts (1)
514-516: RepeatedgetServerConfig()calls may cause console noise.Looking at
getServerConfig()in ConfigLoader.ts (lines 27-42), it logs "using X server config" on every call. SincepollPlayersruns every second and also callsgetServerConfig(), you will see this log message repeatedly in the console.Consider caching the config at the class level or module level to avoid repeated logging.
🔧 Suggested fix: cache config at class level
export class JoinPrivateLobbyModal extends BaseModal { + private serverConfig = getServerConfig(); @query("#lobbyIdInput") private lobbyIdInput!: HTMLInputElement;Then use
this.serverConfiginstead of callinggetServerConfig()incheckActiveLobbyandpollPlayers.src/client/HostLobbyModal.ts (2)
1180-1182: Same console noise issue inpollPlayers.This method runs every second (via interval at line 867), and
getServerConfig()logs to console on each call. This will produce repeated log messages.Consider caching the server config at class or module level.
🔧 Suggested fix: cache at class level
export class HostLobbyModal extends BaseModal { + private serverConfig = getServerConfig(); @state() private selectedMap: GameMapType = GameMapType.World;Then use
this.serverConfiginstartGame,pollPlayers, and pass tocreateLobby.
1233-1234: Consider accepting config as parameter.The
createLobbyfunction could receive the config as a parameter instead of callinggetServerConfig()internally. This would make the function more testable and avoid the logging side effect.🔧 Suggested change
-async function createLobby(creatorClientID: string): Promise<GameInfo> { - const config = getServerConfig(); +async function createLobby(creatorClientID: string, config: ServerConfig): Promise<GameInfo> {Then call it with the cached config from the class.
src/core/configuration/Env.ts (2)
63-63: Remove or guard debug logging in frequently accessed getter.The
console.logstatements on lines 63, 77, and 83 run every timeEnv.GAME_ENVis accessed. This creates log noise in production. Consider removing these or logging only once (e.g., cache the result after first access).Also applies to: 77-77, 83-83
121-132: Consider returningnumber | undefinedinstead ofstring | undefined.
ServerConfigVars.numWorkersis typed asnumber(line 11), butNUM_WORKERSgetter converts it to a string (line 125). Callers expecting a number will need to parse it back. Returningnumber | undefinedwould be more consistent with the interface definition and avoid extra parsing downstream.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/deploy.yml.vscode/launch.json.vscode/settings.jsonindex.htmlnginx.confpackage.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PreprodConfig.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/OtelResource.tssrc/server/Server.tssrc/server/Worker.tssrc/server/WorkerMetrics.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/util/Setup.tsvite.config.ts
💤 Files with no reviewable changes (1)
- nginx.conf
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (13)
- src/client/Matchmaking.ts
- src/server/MapPlaylist.ts
- src/server/Logger.ts
- src/server/Worker.ts
- src/core/GameRunner.ts
- src/core/worker/WorkerClient.ts
- src/server/Master.ts
- src/client/ClientGameRunner.ts
- index.html
- src/server/Server.ts
- tests/core/pathfinding/_fixtures.ts
- tests/util/Setup.ts
- src/client/Main.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/configuration/DefaultConfig.ts:326-328
Timestamp: 2025-08-12T00:30:20.374Z
Learning: In the OpenFrontIO codebase, game configurations are generated at runtime rather than loaded from saved files, so configuration properties will always be present and don't require fallback defaults for backward compatibility.
📚 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/core/worker/WorkerMessages.tssrc/core/configuration/Config.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/DefaultConfig.tssrc/client/HostLobbyModal.tssrc/core/worker/Worker.worker.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/configuration/Env.tssrc/server/Archive.tssrc/server/WorkerMetrics.tsvite.config.tssrc/server/OtelResource.tstests/pathfinding/utils.tssrc/core/configuration/DevConfig.tssrc/client/JoinPrivateLobbyModal.tssrc/core/configuration/ConfigLoader.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/ConfigLoader.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/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.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.ts
📚 Learning: 2025-12-26T22:21:21.904Z
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").
Applied to files:
src/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.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/HostLobbyModal.tssrc/core/configuration/DevConfig.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/core/configuration/Env.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/core/configuration/Env.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
vite.config.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/JoinPrivateLobbyModal.ts
🧬 Code graph analysis (8)
src/client/HostLobbyModal.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Archive.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/WorkerMetrics.ts (2)
src/server/GameManager.ts (1)
GameManager(15-136)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
vite.config.ts (3)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)src/core/configuration/PreprodConfig.ts (1)
env(5-7)
src/server/OtelResource.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/core/configuration/DevConfig.ts (1)
src/core/configuration/DefaultConfig.ts (1)
isReplay(261-263)
src/client/JoinPrivateLobbyModal.ts (2)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)src/core/configuration/Env.ts (1)
Env(58-133)
src/core/configuration/ConfigLoader.ts (6)
src/core/configuration/DefaultConfig.ts (4)
gameConfig(278-280)userSettings(282-287)isReplay(261-263)DefaultConfig(243-1000)src/core/Schemas.ts (1)
GameConfig(90-90)src/core/configuration/Config.ts (2)
Config(70-181)ServerConfig(29-63)src/core/configuration/DevConfig.ts (2)
DevConfig(50-59)DevServerConfig(6-48)src/core/configuration/Env.ts (1)
Env(58-133)src/core/configuration/PreprodConfig.ts (1)
preprodConfig(4-26)
🔇 Additional comments (17)
.vscode/launch.json (1)
24-26: LGTM!Environment value now matches the new
GameEnv.Devenum convention used throughout the codebase.src/client/graphics/layers/MultiTabModal.ts (1)
30-37: LGTM!Clean migration from server config to game config for environment access. The early return for dev mode, spawn phase, and singleplayer games is clear and correct.
package.json (1)
8-11: LGTM!Scripts consistently use
GAME_ENV=Devfor local development. Thedev:staginganddev:prodvariants correctly differentiate only byAPI_DOMAIN- running locally in dev mode but hitting remote APIs.src/core/worker/WorkerMessages.ts (1)
1-1: LGTM!Clean addition of
GameEnvimport and theenvproperty toInitMessage. This properly types the environment information passed to workers during initialization.Also applies to: 40-45
src/server/WorkerMetrics.ts (1)
7-7: LGTM!Straightforward migration from the old
getServerConfigFromServer()to the newgetServerConfig()API. Consistent with the broader config refactor.Also applies to: 15-15
src/server/OtelResource.ts (1)
8-8: Module-level config initialization runs at import time.
getServerConfig()is called when this module is first imported. This means the console log ("using dev/preprod/prod server config") fires immediately at module load. This should be fine for server-side code, but verify thatEnv.GAME_ENVis always set before this module is imported.src/core/worker/Worker.worker.ts (1)
44-56: LGTM! The env parameter is properly threaded through.The
message.envfrom theInitMessageis correctly passed as the second parameter tocreateGameRunner. This completes the environment propagation from main thread to worker.src/core/configuration/Config.ts (2)
23-27: Clear naming improvement.Renaming
PreprodtoStagingis more standard terminology. The enum is clean and simple.
70-71: Good addition to the interface.Adding
env(): GameEnvtoConfigallows access to the environment from game configuration, matching the pattern already inServerConfig. This keeps the API consistent.src/server/Archive.ts (1)
2-2: Import and usage updated correctly.The migration from
getServerConfigFromServertogetServerConfigis applied consistently. Module-level initialization is appropriate for server code.Also applies to: 13-13
src/client/JoinPrivateLobbyModal.ts (1)
572-572: Clean simplification usingEnv.GIT_COMMIT.Direct access to
Env.GIT_COMMITis simpler than the previous async fetch approach. This removes the need for Promise.all handling.src/client/HostLobbyModal.ts (1)
4-4: Import and usage look correct.The migration to
getServerConfig()is applied correctly across all call sites.Also applies to: 1159-1159
tests/pathfinding/utils.ts (2)
3-3: Import added correctly.Direct import of
GameEnvfrom the configuration module is clean and explicit.
251-271: Test config setup simplified.Passing
GameEnv.Devdirectly toTestConfigis cleaner than the previous approach withTestServerConfig. The signature(configObject, GameEnv.Dev, UserSettings, boolean)is clear and easy to understand.src/core/configuration/DefaultConfig.ts (1)
246-255: Clean refactor using composition.The constructor now takes
GameEnvdirectly rather than depending on aServerConfigobject. This is simpler and clearer. Theenv()accessor provides direct access to the injected value.src/core/configuration/DevConfig.ts (1)
50-58: LGTM!The
DevConfigconstructor update aligns with the parent class signature change. Simple pass-through tosuper().src/core/configuration/ConfigLoader.ts (1)
28-43: LGTM!The new
getServerConfig()function provides a clean factory forServerConfigbased onEnv.GAME_ENV. The console logs here are reasonable since this runs once during initialization.
.github/workflows/deploy.yml
Outdated
| GHCR_REPO: ${{ vars.GHCR_REPO }} | ||
| GHCR_USERNAME: ${{ vars.GHCR_USERNAME }} | ||
| ENV: ${{ inputs.target_domain == 'openfront.io' && 'prod' || 'staging' }} | ||
| ENV: ${{ inputs.target_domain == 'openfront.io' && 'Prod' || 'Staging' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the build-deploy.sh script
fd -type f -name "build-deploy.sh"Repository: openfrontio/OpenFrontIO
Length of output: 237
🏁 Script executed:
# Find build-deploy.sh anywhere in the repository
find . -name "build-deploy.sh" 2>/dev/nullRepository: openfrontio/OpenFrontIO
Length of output: 84
🏁 Script executed:
# Read the build-deploy.sh script
cat -n ./build-deploy.shRepository: openfrontio/OpenFrontIO
Length of output: 3069
🏁 Script executed:
# Check build.sh for ENV validation
head -50 ./build.sh | grep -n -A 5 -B 5 "prod\|staging" || echo "Searching full file..."
cat -n ./build.sh | grep -n "prod\|staging" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 1119
🏁 Script executed:
# Check deploy.sh for ENV validation
cat -n ./deploy.sh | grep -n "prod\|staging" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 765
Fix capitalization mismatch: use lowercase 'prod' and 'staging' in ENV value.
The deploy.yml change at line 113 sends capitalized 'Prod' and 'Staging' values, but build-deploy.sh (line 29), build.sh (line 30), and deploy.sh (line 24) all validate the ENV argument with strict lowercase checks (prod and staging). The scripts will reject the capitalized values and fail. Change ENV to use lowercase to match script expectations.
🤖 Prompt for AI Agents
In @.github/workflows/deploy.yml at line 113, The ENV value in the workflow
currently emits capitalized 'Prod'/'Staging', which will be rejected by the
downstream scripts; change the GitHub Actions expression that sets ENV so it
outputs lowercase 'prod' and 'staging' instead of 'Prod'/'Staging' (i.e., update
the ENV assignment expression that uses inputs.target_domain to return 'prod' or
'staging'), ensuring consistency with the strict lowercase checks in
build-deploy.sh, build.sh, and deploy.sh.
| default: | ||
| throw Error(`unsupported server configuration: ${Env.GAME_ENV}`); | ||
| } |
There was a problem hiding this comment.
Error message references wrong variable.
The switch operates on the gameEnv parameter, but the error message references Env.GAME_ENV. If the default case is reached, the logged value may differ from the actual argument that caused the error.
Proposed fix
default:
- throw Error(`unsupported server configuration: ${Env.GAME_ENV}`);
+ throw Error(`unsupported server configuration: ${gameEnv}`);
}🤖 Prompt for AI Agents
In @src/core/configuration/ConfigLoader.ts around lines 23 - 25, The default
case in the switch inside ConfigLoader.ts throws an error referencing
Env.GAME_ENV instead of the actual function parameter; update the throw in that
default branch to include the incoming gameEnv variable (the parameter named
gameEnv) so the error message reflects the actual argument that triggered the
unsupported configuration, e.g. replace Env.GAME_ENV with gameEnv in the thrown
Error in the default case of the switch.
| get GAME_ENV(): GameEnv { | ||
| // Check window.SERVER_CONFIG first (injected at build time) | ||
| try { | ||
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | ||
| console.log("using server config from window"); | ||
| return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; | ||
| } | ||
| } catch { | ||
| // Ignore errors accessing window | ||
| } | ||
|
|
||
| // Check MODE for Vite, GAME_ENV for Node | ||
| try { | ||
| if ( | ||
| typeof import.meta !== "undefined" && | ||
| import.meta.env && | ||
| import.meta.env.MODE | ||
| ) { | ||
| return import.meta.env.MODE; | ||
| console.log("using server config from import.meta.env"); | ||
| return GameEnv[import.meta.env.MODE as keyof typeof GameEnv]; | ||
| } | ||
| } catch { | ||
| // Ignore errors accessing import.meta | ||
| } | ||
|
|
||
| return getEnv("GAME_ENV") ?? "dev"; | ||
| console.log("using server config from environment variable"); | ||
| return GameEnv[getEnv("GAME_ENV") as keyof typeof GameEnv]; | ||
| }, |
There was a problem hiding this comment.
Missing validation for GameEnv lookup can return undefined.
When the string from window.SERVER_CONFIG.gameEnv, import.meta.env.MODE, or getEnv("GAME_ENV") does not match a valid GameEnv key, the expression GameEnv[... as keyof typeof GameEnv] returns undefined. The return type says GameEnv, but callers may get undefined at runtime.
Consider adding validation:
Proposed fix
get GAME_ENV(): GameEnv {
+ const toGameEnv = (value: string | undefined): GameEnv => {
+ if (value && value in GameEnv) {
+ return GameEnv[value as keyof typeof GameEnv];
+ }
+ throw new Error(`Invalid GAME_ENV value: ${value}`);
+ };
+
// Check window.SERVER_CONFIG first (injected at build time)
try {
if (typeof window !== "undefined" && window.SERVER_CONFIG) {
console.log("using server config from window");
- return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv];
+ return toGameEnv(window.SERVER_CONFIG.gameEnv);
}
} catch {
// Ignore errors accessing window
}
// Check MODE for Vite, GAME_ENV for Node
try {
if (
typeof import.meta !== "undefined" &&
import.meta.env &&
import.meta.env.MODE
) {
console.log("using server config from import.meta.env");
- return GameEnv[import.meta.env.MODE as keyof typeof GameEnv];
+ return toGameEnv(import.meta.env.MODE);
}
} catch {
// Ignore errors accessing import.meta
}
console.log("using server config from environment variable");
- return GameEnv[getEnv("GAME_ENV") as keyof typeof GameEnv];
+ return toGameEnv(getEnv("GAME_ENV"));
},📝 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.
| get GAME_ENV(): GameEnv { | |
| // Check window.SERVER_CONFIG first (injected at build time) | |
| try { | |
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | |
| console.log("using server config from window"); | |
| return GameEnv[window.SERVER_CONFIG.gameEnv as keyof typeof GameEnv]; | |
| } | |
| } catch { | |
| // Ignore errors accessing window | |
| } | |
| // Check MODE for Vite, GAME_ENV for Node | |
| try { | |
| if ( | |
| typeof import.meta !== "undefined" && | |
| import.meta.env && | |
| import.meta.env.MODE | |
| ) { | |
| return import.meta.env.MODE; | |
| console.log("using server config from import.meta.env"); | |
| return GameEnv[import.meta.env.MODE as keyof typeof GameEnv]; | |
| } | |
| } catch { | |
| // Ignore errors accessing import.meta | |
| } | |
| return getEnv("GAME_ENV") ?? "dev"; | |
| console.log("using server config from environment variable"); | |
| return GameEnv[getEnv("GAME_ENV") as keyof typeof GameEnv]; | |
| }, | |
| get GAME_ENV(): GameEnv { | |
| const toGameEnv = (value: string | undefined): GameEnv => { | |
| if (value && value in GameEnv) { | |
| return GameEnv[value as keyof typeof GameEnv]; | |
| } | |
| throw new Error(`Invalid GAME_ENV value: ${value}`); | |
| }; | |
| // Check window.SERVER_CONFIG first (injected at build time) | |
| try { | |
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | |
| console.log("using server config from window"); | |
| return toGameEnv(window.SERVER_CONFIG.gameEnv); | |
| } | |
| } catch { | |
| // Ignore errors accessing window | |
| } | |
| // Check MODE for Vite, GAME_ENV for Node | |
| try { | |
| if ( | |
| typeof import.meta !== "undefined" && | |
| import.meta.env && | |
| import.meta.env.MODE | |
| ) { | |
| console.log("using server config from import.meta.env"); | |
| return toGameEnv(import.meta.env.MODE); | |
| } | |
| } catch { | |
| // Ignore errors accessing import.meta | |
| } | |
| console.log("using server config from environment variable"); | |
| return toGameEnv(getEnv("GAME_ENV")); | |
| }, |
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 59 - 85, The GAME_ENV getter can
return undefined when mapping strings to the GameEnv enum; validate each lookup
(for window.SERVER_CONFIG.gameEnv, import.meta.env.MODE, and getEnv("GAME_ENV"))
by checking that the mapped value is a valid GameEnv member before returning it,
and otherwise fall back to a safe default or throw a clear error; update the get
GAME_ENV implementation to compute the candidate key, verify via something like
Object.hasOwnProperty or a reverse-lookup on GameEnv, and only return
GameEnv[candidate] when valid, otherwise continue to the next source or return a
validated default.
evanpelle
force-pushed
the
evan-fix-num-workers
branch
from
d56fc5b to
5766d71
Compare
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/core/configuration/ConfigLoader.ts (1)
28-43: LGTM!The
getServerConfigfunction is clean and straightforward. Good use of a switch on the enum. The console logs are helpful for debugging which config is active.One small thing: the log message says "preprod server config" for
GameEnv.Staging(line 35). Update it to "using staging server config" to match the enum name for consistency.Verified: all old function references (
getServerConfigFromClient/getServerConfigFromServer) have been removed, and the newgetServerConfigis being used correctly across the codebase.
🤖 Fix all issues with AI agents
In @src/core/configuration/ConfigLoader.ts:
- Around line 10-26: The thrown error in getConfig incorrectly references
Env.GAME_ENV instead of the function parameter; update the error to include the
provided gameEnv parameter (e.g., throw Error(`unsupported server configuration:
${gameEnv}`)) so the message reflects the actual value passed into getConfig and
aids debugging of GameEnv handling.
In @src/core/configuration/Env.ts:
- Around line 58-70: The error handling in getGameEnvValue is inconsistent: it
throws when envString is undefined but silently returns GameEnv.Dev for
unrecognized strings; make it strict by throwing for both cases — update
getGameEnvValue to throw a clear Error when envString is undefined or when the
normalized value does not match any GameEnv key (include the provided envString
in the error message for debugging) instead of defaulting to GameEnv.Dev so
callers must supply a valid GAME_ENV.
In @vite.config.ts:
- Around line 59-63: The numWorkers value inside serverConfig can become NaN
because parseInt(env.NUM_WORKERS, 10) returns NaN for missing/invalid values;
change the expression used for serverConfig.numWorkers to parse and validate the
env var and fall back to a safe default (e.g., 2) in production—e.g., compute a
parsed value (parseInt(..., 10)), check Number.isNaN(parsed) (or isFinite) and
use the fallback if invalid, leaving the non-production branch as 2; update the
numWorkers expression inside the serverConfig JSON.stringify accordingly.
🧹 Nitpick comments (4)
vite.config.ts (1)
14-25: UnusedgitCommitvariable is now dead code.The
gitCommitvariable computed here is no longer referenced after the refactor. The newserverConfigusesenv.GIT_COMMITdirectly for production and"DEV"for development.Consider removing this dead code block.
🧹 Suggested cleanup
-let gitCommit = process.env.GIT_COMMIT; - -if (!gitCommit) { - try { - gitCommit = execSync("git rev-parse HEAD").toString().trim(); - } catch (error) { - if (process.env.NODE_ENV !== "production") { - console.warn("Unable to determine git commit:", error.message); - } - gitCommit = "unknown"; - } -}tests/pathfinding/utils.ts (1)
3-3: Import path style is inconsistent with the rest of the file.Other imports in this file use relative paths (e.g.,
"../../src/core/game/Game"), but this import uses an absolute-style path"src/core/configuration/Config". Consider using a relative path for consistency.Suggested fix
-import { GameEnv } from "src/core/configuration/Config"; +import { GameEnv } from "../../src/core/configuration/Config";src/core/configuration/Env.ts (2)
73-98: Remove or reduceconsole.logstatements for production.These log statements (lines 77, 91, 97) will run on every access to
GAME_ENV. In production, this adds noise. Consider usingconsole.debugor removing them once the refactor is stable.Use console.debug instead
if (typeof window !== "undefined" && window.SERVER_CONFIG) { - console.log("using server config from window"); + console.debug("using server config from window"); return getGameEnvValue(window.SERVER_CONFIG.gameEnv); }Apply similar changes to lines 91 and 97.
135-146:NUM_WORKERSreturn type could be more precise.The getter returns
string | undefinedimplicitly (viagetEnv), but when reading fromwindow.SERVER_CONFIG, it converts the number to string. This works, but the return type annotation is missing. Consider adding explicit return type for clarity.Also, if callers expect a number, they'll need to parse this. Might be cleaner to return
number | undefinedand parse once here.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.vscode/launch.json.vscode/settings.jsonindex.htmlnginx.confpackage.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/Main.tssrc/client/Matchmaking.tssrc/client/graphics/layers/MultiTabModal.tssrc/core/GameRunner.tssrc/core/configuration/Config.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PreprodConfig.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/core/worker/WorkerMessages.tssrc/server/Archive.tssrc/server/Logger.tssrc/server/MapPlaylist.tssrc/server/Master.tssrc/server/OtelResource.tssrc/server/Server.tssrc/server/Worker.tssrc/server/WorkerMetrics.tstests/core/pathfinding/_fixtures.tstests/pathfinding/utils.tstests/util/Setup.tsvite.config.ts
💤 Files with no reviewable changes (1)
- nginx.conf
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (15)
- src/core/GameRunner.ts
- src/core/worker/WorkerMessages.ts
- index.html
- src/server/MapPlaylist.ts
- src/server/Server.ts
- src/client/ClientGameRunner.ts
- src/core/configuration/DefaultConfig.ts
- src/server/OtelResource.ts
- .vscode/launch.json
- src/client/Main.ts
- src/server/Logger.ts
- tests/util/Setup.ts
- package.json
- src/client/graphics/layers/MultiTabModal.ts
- src/client/HostLobbyModal.ts
🧰 Additional context used
🧠 Learnings (10)
📚 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/core/configuration/PreprodConfig.tssrc/client/Matchmaking.tssrc/server/WorkerMetrics.tstests/core/pathfinding/_fixtures.tssrc/server/Archive.tstests/pathfinding/utils.tssrc/server/Worker.tssrc/core/configuration/Env.tsvite.config.tssrc/core/configuration/Config.tssrc/core/worker/WorkerClient.tssrc/server/Master.tssrc/core/worker/Worker.worker.tssrc/core/configuration/DevConfig.tssrc/core/configuration/ConfigLoader.tssrc/client/JoinPrivateLobbyModal.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:
tests/core/pathfinding/_fixtures.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/server/Archive.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/core/configuration/Env.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/core/configuration/Env.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DevConfig.tssrc/core/configuration/ConfigLoader.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/core/configuration/DevConfig.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/JoinPrivateLobbyModal.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/JoinPrivateLobbyModal.ts
📚 Learning: 2025-12-26T22:21:21.904Z
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").
Applied to files:
src/client/JoinPrivateLobbyModal.ts
🧬 Code graph analysis (7)
src/client/Matchmaking.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/WorkerMetrics.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Archive.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
src/server/Worker.ts (3)
src/core/game/GameImpl.ts (1)
config(357-359)src/core/game/GameView.ts (1)
config(783-785)src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
vite.config.ts (3)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)src/core/configuration/PreprodConfig.ts (1)
env(5-7)
src/core/worker/WorkerClient.ts (3)
src/core/configuration/DefaultConfig.ts (1)
env(253-255)src/core/configuration/DevConfig.ts (1)
env(23-25)src/core/configuration/PreprodConfig.ts (1)
env(5-7)
src/server/Master.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(28-43)
⏰ 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 (20)
src/server/WorkerMetrics.ts (1)
7-15: LGTM!Clean update to use the new unified
getServerConfig()function. The import and call site are correctly updated.src/core/worker/WorkerClient.ts (2)
23-27: LGTM!Good use of composition - passing
GameEnvexplicitly to the worker rather than relying on global state or inheritance. The constructor signature change is clean and typed.
72-78: LGTM!The
envfield is correctly included in the init message payload, matching the updatedInitMessageschema inWorkerMessages.ts.src/server/Worker.ts (1)
10-33: LGTM!Clean migration to
getServerConfig(). The import and call site are correctly updated.One thing to keep in mind:
getServerConfig()is called at module load time (line 33), so ensureEnv.GAME_ENVis set before this module is imported. Based on the codebase pattern wheredotenv.config()is called early in the startup sequence, this should be fine.src/core/configuration/PreprodConfig.ts (1)
4-7: LGTM!Correctly updated to use
GameEnv.Stagingmatching the renamed enum value inConfig.ts.src/client/Matchmaking.ts (1)
4-4: LGTM on config retrieval refactor.The switch from
getServerConfigFromClient()to the synchronousgetServerConfig()aligns with the broader refactor. Removing theawaitis correct sincegetServerConfig()now returnsServerConfigdirectly.Also applies to: 146-146, 225-225
src/core/worker/Worker.worker.ts (1)
42-56: LGTM on env parameter propagation.The updated call to
createGameRunnercorrectly passesmessage.envas the second argument, matching the new signature. TheInitMessagetype (per the AI summary) now includes theenv: GameEnvfield.src/core/configuration/Config.ts (2)
70-71: Good addition ofenv()to the Config interface.Adding
env(): GameEnvto theConfiginterface provides a clean way for components to access the current environment without depending onServerConfig.
23-27: Enum rename fromPreprodtoStagingis complete.All references to
GameEnv.Preprodhave been updated toGameEnv.Staging. The enum definition is correct, and the ConfigLoader and PreprodConfig classes both use the new enum value properly. No leftover references to the old name exist.src/server/Archive.ts (1)
2-2: LGTM on config retrieval update.The switch to
getServerConfig()is correct. Top-level initialization is appropriate for server-side module code.Also applies to: 13-13
src/server/Master.ts (1)
9-9: LGTM on config retrieval update.The switch to
getServerConfig()is correct. The removal of the/api/envendpoint (per the AI summary) aligns with the new approach of injecting server config into the client viawindow.SERVER_CONFIG.Also applies to: 15-15
src/client/JoinPrivateLobbyModal.ts (3)
3-3: LGTM on import updates.Adding
Envimport and switching togetServerConfig()aligns with the refactor.Also applies to: 12-12
549-582: Good simplification ofcheckArchivedGame.Replacing the parallel
gitCommitPromisefetch with directEnv.GIT_COMMITaccess removes unnecessary complexity. The version check logic remains intact.
514-516: LGTM on synchronous config access.Both
checkActiveLobbyandpollPlayersnow use the synchronousgetServerConfig()correctly.Also applies to: 598-603
tests/core/pathfinding/_fixtures.ts (1)
3-3: Clean refactoring of test config initialization.The switch from
TestServerConfigtoGameEnv.Devaligns with the newDefaultConfigconstructor that takesGameEnvas the second parameter. UsingGameEnv.Devfor tests is the right choice.tests/pathfinding/utils.ts (1)
251-271: LGTM!The
TestConfigconstructor call correctly passesGameEnv.Devas the second argument, aligning with the new config API that takes the environment enum directly.src/core/configuration/DevConfig.ts (2)
3-3: LGTM!Clean import change - now only imports what's needed (
GameEnv) instead of the fullServerConfig.
50-58: LGTM!The constructor signature change is a good simplification. Passing
GameEnvdirectly instead of the fullServerConfigmakes the dependency clearer and the API simpler. Good use of composition - the config only receives what it actually needs.src/core/configuration/Env.ts (2)
9-13: LGTM!Clean interface definition. Using a simple interface for the config shape is good - typed and easy to understand.
22-24: LGTM!Good global type augmentation for the injected server config.
| export function getConfig( | ||
| gameConfig: GameConfig, | ||
| gameEnv: GameEnv, | ||
| userSettings: UserSettings | null, | ||
| isReplay: boolean = false, | ||
| ): Promise<Config> { | ||
| const sc = await getServerConfigFromClient(); | ||
| switch (sc.env()) { | ||
| ): Config { | ||
| switch (gameEnv) { | ||
| case GameEnv.Dev: | ||
| return new DevConfig(sc, gameConfig, userSettings, isReplay); | ||
| case GameEnv.Preprod: | ||
| return new DevConfig(gameConfig, gameEnv, userSettings, isReplay); | ||
| case GameEnv.Staging: | ||
| case GameEnv.Prod: | ||
| console.log("using prod config"); | ||
| return new DefaultConfig(sc, gameConfig, userSettings, isReplay); | ||
| return new DefaultConfig(gameConfig, gameEnv, userSettings, isReplay); | ||
| default: | ||
| throw Error(`unsupported server configuration: ${Env.GAME_ENV}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error message uses wrong variable.
Line 24 references Env.GAME_ENV but the function receives gameEnv as a parameter. The error message should use the parameter value for accurate debugging.
Suggested fix
default:
- throw Error(`unsupported server configuration: ${Env.GAME_ENV}`);
+ throw Error(`unsupported server configuration: ${gameEnv}`);
}📝 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.
| export function getConfig( | |
| gameConfig: GameConfig, | |
| gameEnv: GameEnv, | |
| userSettings: UserSettings | null, | |
| isReplay: boolean = false, | |
| ): Promise<Config> { | |
| const sc = await getServerConfigFromClient(); | |
| switch (sc.env()) { | |
| ): Config { | |
| switch (gameEnv) { | |
| case GameEnv.Dev: | |
| return new DevConfig(sc, gameConfig, userSettings, isReplay); | |
| case GameEnv.Preprod: | |
| return new DevConfig(gameConfig, gameEnv, userSettings, isReplay); | |
| case GameEnv.Staging: | |
| case GameEnv.Prod: | |
| console.log("using prod config"); | |
| return new DefaultConfig(sc, gameConfig, userSettings, isReplay); | |
| return new DefaultConfig(gameConfig, gameEnv, userSettings, isReplay); | |
| default: | |
| throw Error(`unsupported server configuration: ${Env.GAME_ENV}`); | |
| } | |
| } | |
| export function getConfig( | |
| gameConfig: GameConfig, | |
| gameEnv: GameEnv, | |
| userSettings: UserSettings | null, | |
| isReplay: boolean = false, | |
| ): Config { | |
| switch (gameEnv) { | |
| case GameEnv.Dev: | |
| return new DevConfig(gameConfig, gameEnv, userSettings, isReplay); | |
| case GameEnv.Staging: | |
| case GameEnv.Prod: | |
| console.log("using prod config"); | |
| return new DefaultConfig(gameConfig, gameEnv, userSettings, isReplay); | |
| default: | |
| throw Error(`unsupported server configuration: ${gameEnv}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/core/configuration/ConfigLoader.ts around lines 10 - 26, The thrown
error in getConfig incorrectly references Env.GAME_ENV instead of the function
parameter; update the error to include the provided gameEnv parameter (e.g.,
throw Error(`unsupported server configuration: ${gameEnv}`)) so the message
reflects the actual value passed into getConfig and aids debugging of GameEnv
handling.
| // Helper function to get GameEnv value case-insensitively | ||
| function getGameEnvValue(envString: string | undefined): GameEnv { | ||
| if (!envString) { | ||
| throw new Error("GAME_ENV is not defined"); | ||
| } | ||
|
|
||
| const normalizedEnv = envString.toLowerCase(); | ||
| const enumKey = Object.keys(GameEnv).find( | ||
| (key) => key.toLowerCase() === normalizedEnv, | ||
| ); | ||
|
|
||
| return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling in getGameEnvValue.
The function throws if envString is undefined (line 61), but silently defaults to GameEnv.Dev if the string doesn't match any enum key (line 69). This is inconsistent - either both cases should throw, or both should have a default.
If defaulting to Dev is intentional, consider removing the throw and defaulting consistently. If strict validation is needed, throw on unrecognized values too.
Option A: Always default to Dev
function getGameEnvValue(envString: string | undefined): GameEnv {
- if (!envString) {
- throw new Error("GAME_ENV is not defined");
- }
+ if (!envString) {
+ return GameEnv.Dev;
+ }
const normalizedEnv = envString.toLowerCase();
const enumKey = Object.keys(GameEnv).find(
(key) => key.toLowerCase() === normalizedEnv,
);
return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev;
}Option B: Strict validation
function getGameEnvValue(envString: string | undefined): GameEnv {
if (!envString) {
throw new Error("GAME_ENV is not defined");
}
const normalizedEnv = envString.toLowerCase();
const enumKey = Object.keys(GameEnv).find(
(key) => key.toLowerCase() === normalizedEnv,
);
- return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev;
+ if (!enumKey) {
+ throw new Error(`Unknown GAME_ENV value: ${envString}`);
+ }
+ return GameEnv[enumKey as keyof typeof GameEnv];
}📝 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.
| // Helper function to get GameEnv value case-insensitively | |
| function getGameEnvValue(envString: string | undefined): GameEnv { | |
| if (!envString) { | |
| throw new Error("GAME_ENV is not defined"); | |
| } | |
| const normalizedEnv = envString.toLowerCase(); | |
| const enumKey = Object.keys(GameEnv).find( | |
| (key) => key.toLowerCase() === normalizedEnv, | |
| ); | |
| return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev; | |
| } | |
| // Helper function to get GameEnv value case-insensitively | |
| function getGameEnvValue(envString: string | undefined): GameEnv { | |
| if (!envString) { | |
| return GameEnv.Dev; | |
| } | |
| const normalizedEnv = envString.toLowerCase(); | |
| const enumKey = Object.keys(GameEnv).find( | |
| (key) => key.toLowerCase() === normalizedEnv, | |
| ); | |
| return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev; | |
| } |
| // Helper function to get GameEnv value case-insensitively | |
| function getGameEnvValue(envString: string | undefined): GameEnv { | |
| if (!envString) { | |
| throw new Error("GAME_ENV is not defined"); | |
| } | |
| const normalizedEnv = envString.toLowerCase(); | |
| const enumKey = Object.keys(GameEnv).find( | |
| (key) => key.toLowerCase() === normalizedEnv, | |
| ); | |
| return enumKey ? GameEnv[enumKey as keyof typeof GameEnv] : GameEnv.Dev; | |
| } | |
| // Helper function to get GameEnv value case-insensitively | |
| function getGameEnvValue(envString: string | undefined): GameEnv { | |
| if (!envString) { | |
| throw new Error("GAME_ENV is not defined"); | |
| } | |
| const normalizedEnv = envString.toLowerCase(); | |
| const enumKey = Object.keys(GameEnv).find( | |
| (key) => key.toLowerCase() === normalizedEnv, | |
| ); | |
| if (!enumKey) { | |
| throw new Error(`Unknown GAME_ENV value: ${envString}`); | |
| } | |
| return GameEnv[enumKey as keyof typeof GameEnv]; | |
| } |
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 58 - 70, The error handling in
getGameEnvValue is inconsistent: it throws when envString is undefined but
silently returns GameEnv.Dev for unrecognized strings; make it strict by
throwing for both cases — update getGameEnvValue to throw a clear Error when
envString is undefined or when the normalized value does not match any GameEnv
key (include the provided envString in the error message for debugging) instead
of defaulting to GameEnv.Dev so callers must supply a valid GAME_ENV.
| serverConfig: JSON.stringify({ | ||
| gameEnv: isProduction ? env.GAME_ENV : "Dev", | ||
| numWorkers: isProduction ? parseInt(env.NUM_WORKERS, 10) : 2, | ||
| gitCommit: isProduction ? env.GIT_COMMIT : "DEV", | ||
| }), |
There was a problem hiding this comment.
Potential NaN when NUM_WORKERS env var is missing or invalid in production.
parseInt(env.NUM_WORKERS, 10) returns NaN if env.NUM_WORKERS is undefined or not a valid number. This would inject NaN into the client config.
Consider adding a fallback:
🛠️ Proposed fix
serverConfig: JSON.stringify({
gameEnv: isProduction ? env.GAME_ENV : "Dev",
- numWorkers: isProduction ? parseInt(env.NUM_WORKERS, 10) : 2,
+ numWorkers: isProduction ? (parseInt(env.NUM_WORKERS, 10) || 2) : 2,
gitCommit: isProduction ? env.GIT_COMMIT : "DEV",
}),📝 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.
| serverConfig: JSON.stringify({ | |
| gameEnv: isProduction ? env.GAME_ENV : "Dev", | |
| numWorkers: isProduction ? parseInt(env.NUM_WORKERS, 10) : 2, | |
| gitCommit: isProduction ? env.GIT_COMMIT : "DEV", | |
| }), | |
| serverConfig: JSON.stringify({ | |
| gameEnv: isProduction ? env.GAME_ENV : "Dev", | |
| numWorkers: isProduction ? (parseInt(env.NUM_WORKERS, 10) || 2) : 2, | |
| gitCommit: isProduction ? env.GIT_COMMIT : "DEV", | |
| }), |
🤖 Prompt for AI Agents
In @vite.config.ts around lines 59 - 63, The numWorkers value inside
serverConfig can become NaN because parseInt(env.NUM_WORKERS, 10) returns NaN
for missing/invalid values; change the expression used for
serverConfig.numWorkers to parse and validate the env var and fall back to a
safe default (e.g., 2) in production—e.g., compute a parsed value (parseInt(...,
10)), check Number.isNaN(parsed) (or isFinite) and use the fallback if invalid,
leaving the non-production branch as 2; update the numWorkers expression inside
the serverConfig JSON.stringify accordingly.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Before, certain vars from the server were either hardcoded or retrieved from the /env endpoint or /commit.txt. Instead we can simplify this by just inject env variables into index.html so it's available to the client immediately.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan