Closed
Conversation
Contributor
WalkthroughThe configuration loader was updated to handle non-browser execution contexts. It now conditionally reads from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/configuration/ConfigLoader.test.ts (1)
28-34: Add one test that truly covers the worker branch (windowundefined).This case currently keeps
windowavailable, so it does not validate thetypeof window === "undefined"path inConfigLoader.Possible test shape
+ test("falls back to bundled env in worker context (no window)", async () => { + const originalWindow = (globalThis as { window?: Window }).window; + delete (globalThis as { window?: Window }).window; + process.env.GAME_ENV = "prod"; + + const config = await getServerConfigFromClient(); + expect(config.env()).toBe(GameEnv.Prod); + + (globalThis as { window?: Window }).window = originalWindow; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/configuration/ConfigLoader.test.ts` around lines 28 - 34, The current test doesn't exercise the worker branch where typeof window === "undefined"; add a test (or modify the "falls back to bundled env when bootstrap config is unavailable" case) that temporarily removes the global window reference before calling getServerConfigFromClient (e.g., save const originalWindow = (global as any).window, delete (global as any).window or set it to undefined), call getServerConfigFromClient and assert env() === GameEnv.Prod, then restore the originalWindow in a finally/after block to avoid side effects; target the ConfigLoader code path that checks typeof window === "undefined" so the worker branch is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/configuration/ConfigLoader.ts`:
- Around line 42-53: The code currently uses bootstrapGameEnv ?? bundledGameEnv
for gameEnv which allows the build-time bundledGameEnv to be used in browser
contexts when bootstrap is missing; change the logic so browsers (typeof window
!== "undefined") always use bootstrapGameEnv only, and only non-browser runtimes
(workers/Node) fall back to bundledGameEnv. Update the assignment for gameEnv to
prefer bootstrapGameEnv in browser contexts and to use bootstrapGameEnv ??
bundledGameEnv when window is undefined, then continue to call
getServerConfig(gameEnv) and assign cachedSC as before (references:
bootstrapGameEnv, bundledGameEnv, gameEnv, getServerConfig, cachedSC).
---
Nitpick comments:
In `@tests/core/configuration/ConfigLoader.test.ts`:
- Around line 28-34: The current test doesn't exercise the worker branch where
typeof window === "undefined"; add a test (or modify the "falls back to bundled
env when bootstrap config is unavailable" case) that temporarily removes the
global window reference before calling getServerConfigFromClient (e.g., save
const originalWindow = (global as any).window, delete (global as any).window or
set it to undefined), call getServerConfigFromClient and assert env() ===
GameEnv.Prod, then restore the originalWindow in a finally/after block to avoid
side effects; target the ConfigLoader code path that checks typeof window ===
"undefined" so the worker branch is executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdf34b96-c095-4db3-82ae-9c821273670f
📒 Files selected for processing (2)
src/core/configuration/ConfigLoader.tstests/core/configuration/ConfigLoader.test.ts
Comment on lines
+42
to
+53
| const bootstrapGameEnv = | ||
| typeof window !== "undefined" | ||
| ? window.BOOTSTRAP_CONFIG?.gameEnv | ||
| : undefined; | ||
| // Vite replaces this at build time for browser and worker bundles. | ||
| const bundledGameEnv = process.env.GAME_ENV; | ||
| const gameEnv = bootstrapGameEnv ?? bundledGameEnv; | ||
| if (!gameEnv) { | ||
| throw new Error("Missing client server config"); | ||
| } | ||
|
|
||
| cachedSC = getServerConfig(bootstrapGameEnv); | ||
| cachedSC = getServerConfig(gameEnv); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting client env resolution branch in ConfigLoader:"
rg -n -C3 'bootstrapGameEnv|bundledGameEnv|gameEnv =|typeof window' src/core/configuration/ConfigLoader.ts
echo
echo "Inspecting Vite define mapping for process.env.GAME_ENV:"
rg -n -C3 '"process.env.GAME_ENV"' vite.config.tsRepository: openfrontio/OpenFrontIO
Length of output: 1154
Restrict bundled environment fallback to worker runtimes only.
At line 48, bootstrapGameEnv ?? bundledGameEnv falls back to build-time config even in browser contexts when bootstrap is missing. This masks bootstrap failures and can route staging/test environments to the production build-time value.
Suggested fix
- const bootstrapGameEnv =
- typeof window !== "undefined"
- ? window.BOOTSTRAP_CONFIG?.gameEnv
- : undefined;
- // Vite replaces this at build time for browser and worker bundles.
- const bundledGameEnv = process.env.GAME_ENV;
- const gameEnv = bootstrapGameEnv ?? bundledGameEnv;
+ const gameEnv =
+ typeof window !== "undefined"
+ ? window.BOOTSTRAP_CONFIG?.gameEnv
+ : process.env.GAME_ENV; // worker / non-window runtime
if (!gameEnv) {
throw new Error("Missing client server config");
}This ensures browsers always use runtime bootstrap config and only workers fall back to the build-time environment variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/ConfigLoader.ts` around lines 42 - 53, The code
currently uses bootstrapGameEnv ?? bundledGameEnv for gameEnv which allows the
build-time bundledGameEnv to be used in browser contexts when bootstrap is
missing; change the logic so browsers (typeof window !== "undefined") always use
bootstrapGameEnv only, and only non-browser runtimes (workers/Node) fall back to
bundledGameEnv. Update the assignment for gameEnv to prefer bootstrapGameEnv in
browser contexts and to use bootstrapGameEnv ?? bundledGameEnv when window is
undefined, then continue to call getServerConfig(gameEnv) and assign cachedSC as
before (references: bootstrapGameEnv, bundledGameEnv, gameEnv, getServerConfig,
cachedSC).
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
Author
|
This fixes the regression, but the config model is still mixed between runtime bootstrap and bundled env fallback. The cleaner long-term fix would be to unify client config loading around a single runtime source of truth and pass that explicitly, instead of relying on two different env sources. |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a hotfix for the worker regression introduced by the
/api/envremoval / HTML-bootstrap rework.The main thread can read
window.BOOTSTRAP_CONFIG, but the game worker cannot.This PR keeps the current runtime behavior for the main thread and restores worker initialization by making the client config loader worker-safe.
What Changed
getServerConfigFromClient()now:window.BOOTSTRAP_CONFIG?.gameEnvwhenwindowexistsprocess.env.GAME_ENVwhen running in the workerWhy This Hotfix Uses This Approach
This branch intentionally does not switch the whole client config path to
process.env.GAME_ENV.A fully “clean” switch to
process.env.GAME_ENVeverywhere was tested https://github.com/openfrontio/OpenFrontIO/compare/fix-worker-config-env?expand=1 , but it changed behavior on test/staging deployments.The client-side
process.env.GAME_ENVvalue is derived from the Vite build mode. That means production builds resolve toprodeven when the runtime deployment should behave asstaging, which caused test-domain deployments to use the production API / Turnstile configuration.This hotfix avoids that behavior change by preserving the existing runtime bootstrap path on the main thread and only using the bundled env where it is actually needed: inside the worker.
This hotfix preserves the current environment-selection behavior on the main thread:
windowis unavailable thereThat keeps the deployed behavior aligned with the existing staging/preprod setup while fixing the worker crash.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME