Pass env vars to client in index.html, remove /env/vars #2816
Conversation
WalkthroughThe PR transitions server configuration delivery from a runtime HTTP API fetch to build-time injection into the global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/core/configuration/Env.ts:
- Line 55: The comment in Env.ts that currently says "injected at build time" is
inaccurate; update the two occurrences (the comment above the check for
window.SERVER_CONFIG at the lines around 55 and 108) to state that
window.SERVER_CONFIG is injected at runtime (when the HTML is served) or
"injected into index.html at runtime", so the comment accurately reflects the
runtime injection behavior shown in index.html/vite.config.ts.
- Around line 55-62: The code returns window.SERVER_CONFIG.gameEnv even when
it's empty/falsy, preventing fallback to import.meta.env.MODE or
process.env.GAME_ENV; update the getter in Env (the block that checks
window.SERVER_CONFIG and accesses gameEnv) to verify that
window.SERVER_CONFIG.gameEnv is a non-empty/truthy string before returning it
(e.g., check typeof and truthiness), and only return it when valid so that
otherwise the function proceeds to the existing fallbacks.
- Around line 108-115: The current check returns
String(window.SERVER_CONFIG.numWorkers) unconditionally which yields "undefined"
when numWorkers is missing; instead read the value into a variable (e.g., const
numWorkers = window.SERVER_CONFIG.numWorkers), verify it is not null or
undefined (numWorkers !== undefined && numWorkers !== null) before returning
String(numWorkers), and only fall back to getEnv("NUM_WORKERS") when that
validation fails; update the logic around window.SERVER_CONFIG.numWorkers in
Env.ts accordingly.
In @vite.config.ts:
- Around line 59-62: Update the serverConfig JSON.stringify block in
vite.config.ts to validate and sanitize NUM_WORKERS and to include "staging" in
the gameEnv fallback: parse env.NUM_WORKERS to an integer, check for NaN and
clamp to a minimum of 1 (default to 2 if invalid), then use that sanitized value
for numWorkers; for gameEnv, prefer env.GAME_ENV if present, otherwise compute a
fallback that checks isProduction first, then an isStaging flag (compute
isStaging similar to isProduction or via env.NODE_ENV/other config), returning
"staging" if set, else "dev"; reference the existing serverConfig construction,
env.NUM_WORKERS, parseInt usage, and gameEnv/isProduction symbols when making
the changes.
🧹 Nitpick comments (2)
src/core/configuration/ConfigLoader.ts (1)
36-47: Consider renaming to reduce confusion.The code correctly reads from
window.SERVER_CONFIG, but there are naming overlaps that could confuse developers:
ServerConfiginterface in client/ServerConfig.ts (raw window data) vsServerConfigtype in Config.ts (configuration logic)getServerConfig()in client/ServerConfig.ts vsgetServerConfig(gameEnv)in this file (different signatures)While technically correct, consider renaming the raw interface to something like
InjectedServerConfigorServerConfigDatato clarify the distinction.src/client/ServerConfig.ts (1)
8-21: Implementation is clean and correct.The interface and getter function are straightforward. The error handling properly checks for missing
window.SERVER_CONFIG.As noted in ConfigLoader.ts, consider renaming this interface (e.g.,
InjectedServerConfig) to distinguish it from theServerConfigtype imported from Config.ts. This would make the codebase clearer for developers encountering both types.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/settings.jsonindex.htmlnginx.confsrc/client/ServerConfig.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/Env.tssrc/server/Master.tsvite.config.ts
💤 Files with no reviewable changes (2)
- nginx.conf
- src/server/Master.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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: 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.ts
🧬 Code graph analysis (2)
src/client/ServerConfig.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfig(53-67)
src/core/configuration/ConfigLoader.ts (1)
src/client/ServerConfig.ts (1)
getServerConfig(17-22)
⏰ 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 (3)
.vscode/settings.json (1)
1-3: Dev setting looks good.Disabling HTML script validation makes sense given the runtime-injected
window.SERVER_CONFIGscript in index.html. This prevents false positives during development.index.html (1)
136-139: Global config injection looks correct.The
window.SERVER_CONFIGassignment uses unescaped EJS (<%-), which is safe here becauseserverConfigis alreadyJSON.stringify'd in vite.config.ts. The placement before the CrazyGames SDK ensures the config is available when needed.src/core/configuration/ConfigLoader.ts (1)
9-10: Side-effect import for type declaration.This import ensures the global
Windowinterface extension (withSERVER_CONFIGproperty) is available. While this pattern works, it relies on implicit side effects.
| interface Window { | ||
| SERVER_CONFIG?: { | ||
| gameEnv: string; | ||
| numWorkers: number; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Type mismatch: numWorkers should be string or the getter should return number.
The interface defines numWorkers: number, but the NUM_WORKERS getter (line 111) converts it to a string with String(window.SERVER_CONFIG.numWorkers). This creates type confusion.
Either change the interface to numWorkers: string or have the getter return the number directly without conversion.
Proposed fix: align types
Option 1 - Make interface use string:
interface Window {
SERVER_CONFIG?: {
gameEnv: string;
- numWorkers: number;
+ numWorkers: string;
};
}Option 2 - Return number from getter:
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);
+ return window.SERVER_CONFIG.numWorkers;
}
} catch {
// Ignore errors accessing window
}
return getEnv("NUM_WORKERS");
},📝 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.
| interface Window { | |
| SERVER_CONFIG?: { | |
| gameEnv: string; | |
| numWorkers: number; | |
| }; | |
| } | |
| interface Window { | |
| SERVER_CONFIG?: { | |
| gameEnv: string; | |
| numWorkers: string; | |
| }; | |
| } |
|
|
||
| export const Env = { | ||
| get GAME_ENV(): string { | ||
| // Check window.SERVER_CONFIG first (injected at build time) |
There was a problem hiding this comment.
Comment says "build time" but injection appears to be at runtime.
The comments state "injected at build time", but based on the PR context (index.html and vite.config.ts), the window.SERVER_CONFIG object is injected at runtime when the HTML is served, not during the build process.
Update the comment for accuracy:
- // Check window.SERVER_CONFIG first (injected at build time)
+ // Check window.SERVER_CONFIG first (injected at runtime)Also applies to: 108-108
🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts at line 55, The comment in Env.ts that
currently says "injected at build time" is inaccurate; update the two
occurrences (the comment above the check for window.SERVER_CONFIG at the lines
around 55 and 108) to state that window.SERVER_CONFIG is injected at runtime
(when the HTML is served) or "injected into index.html at runtime", so the
comment accurately reflects the runtime injection behavior shown in
index.html/vite.config.ts.
| // Check window.SERVER_CONFIG first (injected at build time) | ||
| try { | ||
| if (typeof window !== "undefined" && window.SERVER_CONFIG) { | ||
| return window.SERVER_CONFIG.gameEnv; | ||
| } | ||
| } catch { | ||
| // Ignore errors accessing window | ||
| } |
There was a problem hiding this comment.
Fallback won't trigger if gameEnv is empty or falsy.
If window.SERVER_CONFIG exists but gameEnv is an empty string or other falsy value, the getter returns that falsy value instead of falling back to import.meta.env.MODE or process.env.GAME_ENV.
Consider checking if the value is truthy before returning it:
Proposed fix
// Check window.SERVER_CONFIG first (injected at build time)
try {
if (typeof window !== "undefined" && window.SERVER_CONFIG) {
- return window.SERVER_CONFIG.gameEnv;
+ const gameEnv = window.SERVER_CONFIG.gameEnv;
+ if (gameEnv) {
+ return gameEnv;
+ }
}
} catch {
// Ignore errors accessing window
}🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 55 - 62, The code returns
window.SERVER_CONFIG.gameEnv even when it's empty/falsy, preventing fallback to
import.meta.env.MODE or process.env.GAME_ENV; update the getter in Env (the
block that checks window.SERVER_CONFIG and accesses gameEnv) to verify that
window.SERVER_CONFIG.gameEnv is a non-empty/truthy string before returning it
(e.g., check typeof and truthiness), and only return it when valid so that
otherwise the function proceeds to the existing fallbacks.
| // 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 | ||
| } |
There was a problem hiding this comment.
Fallback won't trigger if numWorkers is missing or 0.
If window.SERVER_CONFIG exists but numWorkers is undefined or 0, the behavior is problematic:
String(undefined)returns"undefined"as a string literalString(0)returns"0", which might be valid but prevents fallback togetEnv("NUM_WORKERS")
Check if the value exists and is valid before returning it:
Proposed fix
// Check window.SERVER_CONFIG first (injected at build time)
try {
if (typeof window !== "undefined" && window.SERVER_CONFIG) {
- return String(window.SERVER_CONFIG.numWorkers);
+ const numWorkers = window.SERVER_CONFIG.numWorkers;
+ if (numWorkers !== undefined && numWorkers !== null) {
+ return String(numWorkers);
+ }
}
} catch {
// Ignore errors accessing window
}🤖 Prompt for AI Agents
In @src/core/configuration/Env.ts around lines 108 - 115, The current check
returns String(window.SERVER_CONFIG.numWorkers) unconditionally which yields
"undefined" when numWorkers is missing; instead read the value into a variable
(e.g., const numWorkers = window.SERVER_CONFIG.numWorkers), verify it is not
null or undefined (numWorkers !== undefined && numWorkers !== null) before
returning String(numWorkers), and only fall back to getEnv("NUM_WORKERS") when
that validation fails; update the logic around window.SERVER_CONFIG.numWorkers
in Env.ts accordingly.
| serverConfig: JSON.stringify({ | ||
| gameEnv: env.GAME_ENV || (isProduction ? "prod" : "dev"), | ||
| numWorkers: parseInt(env.NUM_WORKERS || "2", 10), | ||
| }), |
There was a problem hiding this comment.
Handle invalid NUM_WORKERS input.
The parseInt call will return NaN if NUM_WORKERS is set to a non-numeric value. This could cause runtime issues. Add validation or use a safer default.
Additionally, the gameEnv fallback logic doesn't account for "staging", which is a valid option in ConfigLoader.ts (line 58). Consider adding it to the fallback.
🛡️ Proposed fix with validation
data: {
serverConfig: JSON.stringify({
- gameEnv: env.GAME_ENV || (isProduction ? "prod" : "dev"),
- numWorkers: parseInt(env.NUM_WORKERS || "2", 10),
+ gameEnv: env.GAME_ENV || (isProduction ? "prod" : "dev"),
+ numWorkers: Math.max(1, parseInt(env.NUM_WORKERS || "2", 10) || 2),
}),
},This ensures:
- If
NUM_WORKERSis invalid, defaults to 2 - If it's valid but less than 1, clamps to 1
🤖 Prompt for AI Agents
In @vite.config.ts around lines 59 - 62, Update the serverConfig JSON.stringify
block in vite.config.ts to validate and sanitize NUM_WORKERS and to include
"staging" in the gameEnv fallback: parse env.NUM_WORKERS to an integer, check
for NaN and clamp to a minimum of 1 (default to 2 if invalid), then use that
sanitized value for numWorkers; for gameEnv, prefer env.GAME_ENV if present,
otherwise compute a fallback that checks isProduction first, then an isStaging
flag (compute isStaging similar to isProduction or via env.NODE_ENV/other
config), returning "staging" if set, else "dev"; reference the existing
serverConfig construction, env.NUM_WORKERS, parseInt usage, and
gameEnv/isProduction symbols when making the changes.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME