refactor(packages/app): refactor env #1407
Conversation
suddenlyGiovanni
temporarily deployed
to
fly-preview-1407
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThe changes refactor environment variable handling from standalone functions to a singleton Config class, updating how environment configuration is initialized and accessed in the application. IDE configuration for the Biome plugin was also updated to use a manual mode and a specific executable path. No changes to exported or public APIs were made. Changes
Sequence Diagram(s)sequenceDiagram
participant Process
participant ConfigClass
participant GlobalENV
Process->>ConfigClass: Access Config.instance (singleton)
ConfigClass->>ConfigClass: #init() (validate and freeze env)
ConfigClass->>Process: Provide serverEnv (full validated env)
ConfigClass->>Process: Provide clientEnv (safe subset)
Process->>GlobalENV: Assign ENV = Config.instance.clientEnv
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/app/src/root.tsxOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/app/src/entry.server.tsxOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/app/src/utils/env.server.tsOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the environment configuration system by introducing a centralized Config class to replace the previous env functions and updates IDE settings for Biome and TailwindCSS.
- Centralizes and validates environment variables using a new Config class.
- Introduces an optional APP_ENV variable with a default of "development".
- Updates import references in client files and adjusts global ENV assignment in server entry.
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/app/client/utils/env.server.ts | Introduces the Config class, adds APP_ENV to the schema, and refactors how environment variables are accessed. |
| packages/app/client/root.tsx | Replaces getEnv() with Config.instance.clientEnv to align with the new configuration system. |
| packages/app/client/entry.server.tsx | Removes init() and updates global.ENV assignment to use Config.instance.serverEnv. |
Files not reviewed (2)
- .idea/biome.xml: Language not supported
- .idea/tailwindcss.xml: Language not supported
Comments suppressed due to low confidence (1)
packages/app/client/utils/env.server.ts:95
- [nitpick] Consider renaming 'serverEnv' to a name that more clearly conveys it contains the full set of server-side environment variables. Similarly, ensure that the distinction between 'serverEnv' and 'clientEnv' is clearly documented for future maintainability.
get serverEnv() {
|
🔄 Preview deployment started... Please wait while we deploy your changes! |
|
❌ Preview deployment cancelled. Deployment failed. Please check the workflow logs for more details. |
|
🔄 Preview deployment started... Please wait while we deploy your changes! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/app/client/utils/env.server.ts (1)
34-45: 💡 Verification agent🧩 Analysis chain
Defaulting
GITHUB_TOKENto a mock value can silently break prod flowsProviding a fallback of
'MOCK_GITHUB_TOKEN'means the application will continue to boot in production even when the real secret is missing. Down-stream GitHub calls will then fail at runtime, making the failure harder to trace and potentially triggering rate-limits.- default(): string { - return 'MOCK_GITHUB_TOKEN' - }, + // In non-test environments we want the process to crash if the + // token is not provided, so we deliberately omit a default.If you still need a default for local development, gate it behind
NODE_ENV === 'development'.
🏁 Script executed:
#!/bin/bash # Locate the env.server.ts file ENV_FILE=$(fd env.server.ts -t f) # Display the file around the GITHUB_TOKEN definition echo "---- $ENV_FILE (lines 1-200) ----" sed -n '1,200p' "$ENV_FILE" # Search for all references to GITHUB_TOKEN in the repo echo "---- References to GITHUB_TOKEN ----" rg -n "GITHUB_TOKEN" .Length of output: 3942
Enforce explicit
GITHUB_TOKENin non-development environmentsThe unconditional fallback to
'MOCK_GITHUB_TOKEN'means your app will boot in production (or CI) without a real token and only fail later when calling GitHub, making the root cause hard to trace and potentially exhausting rate limits.Please update packages/app/client/utils/env.server.ts to remove the default for
GITHUB_TOKENand only provide a mock value whenNODE_ENV === 'development'(ortest, if you need it for tests):--- a/packages/app/client/utils/env.server.ts +++ b/packages/app/client/utils/env.server.ts @@ 31,13 +31,15 @@ const envSchema = Schema.Struct({ GITHUB_TOKEN: Schema.optionalWith( Schema.NonEmptyString.annotations({ description: 'GitHub token', examples: ['github_pat_xxxxxxx_xxxxxx'], }), { - default(): string { - return 'MOCK_GITHUB_TOKEN' - }, + // Only mock locally; in CI/production we want to fail fast if missing. + default(): string { + return process.env.NODE_ENV === 'development' + ? 'MOCK_GITHUB_TOKEN' + : (() => { throw new Error('GITHUB_TOKEN must be provided'); })() + }, exact: true, }, ),• This change will cause the schema validation to error out immediately when
GITHUB_TOKENis absent in non-development runs, making missing-secret failures obvious.
• If your tests rely on a mock token, consider allowing the default fortestas well.
• No other code paths referencedefault('MOCK_GITHUB_TOKEN'), so this is a safe, targeted change.
🧹 Nitpick comments (3)
packages/app/client/utils/env.server.ts (2)
70-93: Surface the exact validation errors instead of a generic message
#initlogs the formatted tree but then throws a new genericError('Invalid environment variables'), discarding the rich parse information that would be extremely helpful when the process crashes in CI.- console.error( - '❌ Invalid environment variables:', - ParseResult.TreeFormatter.formatErrorSync(parseError), - ) - throw new Error('Invalid environment variables') + const formatted = ParseResult.TreeFormatter.formatErrorSync(parseError) + console.error('❌ Invalid environment variables:\n', formatted) + // re-throw with the original context for better stack-traces + throw new Error(`Invalid environment variables:\n${formatted}`)This keeps the uncaught exception informative while preserving the nice console output.
106-111: Freeze the client env subset for immutability parityYou freeze
#envfor the server side, but the subset returned to the client remains mutable.
Freezing prevents accidental mutation during server-side rendering and aligns both getters.return { NODE_ENV: serverEnv.NODE_ENV, -} +} as const satisfies ClientEnvVarsOr simply wrap it with
Object.freeze.packages/app/client/root.tsx (1)
70-71: Minor: inline destructuring avoids the extraENVwrapperIf you adopt the lazy import above you can return the client env directly:
- return { ENV: Config.instance.clientEnv, requestInfo: { … } } + return { ENV: clientEnv, requestInfo: { … } }Less nesting simplifies
JSON.stringify(data?.ENV)later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.idea/biome.xml(1 hunks).idea/tailwindcss.xml(0 hunks)packages/app/client/entry.server.tsx(1 hunks)packages/app/client/root.tsx(2 hunks)packages/app/client/utils/env.server.ts(3 hunks)
💤 Files with no reviewable changes (1)
- .idea/tailwindcss.xml
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/app/client/entry.server.tsx (1)
packages/app/client/utils/env.server.ts (1)
Config(50-112)
packages/app/client/root.tsx (1)
packages/app/client/utils/env.server.ts (1)
Config(50-112)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Deploy Preview to Fly.io
🔇 Additional comments (2)
.idea/biome.xml (1)
6-9: IDE-only change looks goodPointing
executablePathto the project-local binary and switching to manual configuration will help avoid path issues across machines. No action needed.packages/app/client/entry.server.tsx (1)
9-9: Clean refactoring to singleton pattern!The changes properly implement the environment configuration refactoring, replacing the separate function calls with the new singleton
Configclass. The explicit initialization is now encapsulated within the class, resulting in cleaner and more maintainable code.Also applies to: 13-13
packages/app/client/root.tsx
Outdated
| import { Config } from '#root/client/utils/env.server.ts' | ||
| import { getDomainUrl } from '#root/client/utils/misc.ts' |
There was a problem hiding this comment.
Importing a .server.ts module at the top level leaks server-only code into the browser bundle
Because this file is compiled for both targets, the unconditional top-level import forces esbuild to parse–and often bundle–env.server.ts, which itself imports the Node-only 'node:process' module.
This will bloat the client bundle and can even break the build if the bundler cannot shim the Node core module.
Move the import inside the loader (server-only) or, even better, extract the loader to root.server.tsx.
-import { Config } from '#root/client/utils/env.server.ts'
+// defer server-only import to avoid polluting the client bundle
+Example:
export async function loader({ request }: Route.LoaderArgs) {
const { Config } = await import('#root/client/utils/env.server.ts')
return { ENV: Config.instance.clientEnv, /* … */ }
}🤖 Prompt for AI Agents (early access)
In packages/app/client/root.tsx around lines 22 to 23, the import of the
server-only module env.server.ts at the top level causes server code to leak
into the client bundle, increasing bundle size and potentially breaking the
build. To fix this, remove the top-level import of env.server.ts and instead
dynamically import it inside the server-only loader function or move the loader
to a separate root.server.tsx file. This ensures the server-only code is only
loaded on the server and not included in the client bundle.
|
✅ Preview deployment success. 🌐 Preview URL: https://suddenlygiovanni-dev-1407.fly.dev This preview environment will be automatically cleaned up when the PR is closed. |
Updated biome settings to use manual configuration mode and set executable path correctly. Removed the Tailwind CSS LSP server package reference for cleanup.
Replaced `init` and `getEnv` functions with a singleton `Config` class for environment variable management. Centralizes initialization, improves encapsulation, and ensures separation of server and client environment variables. Updated related imports and usage throughout the app.
suddenlyGiovanni
force-pushed
the
packages/app/refactor-env
branch
from
d1abfe2 to
4d9d928
Compare
suddenlyGiovanni
temporarily deployed
to
fly-preview-1407
GitHub Actions
Inactive
|
🔄 Preview deployment started... Please wait while we deploy your changes! |
|
❌ Preview deployment failure. Deployment failed. Please check the workflow logs for more details. |
|
🔄 Preview deployment started... Please wait while we deploy your changes! |
|
❌ Preview deployment failure. Deployment failed. Please check the workflow logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/app/src/root.tsx (1)
68-71: Eager singleton creation inside loader costs per-request
Config.instanceis dereferenced on every request even though the class is a global singleton.
That’s fine for correctness, but you could avoid the tiny cost and the try/catch noise by hoisting:const ENV = Config.instance.clientEnv export function loader({ request }: Route.LoaderArgs) { return { ENV, /* … */ } }Purely optional, but keeps the loader 100 % side-effect free.
packages/app/src/utils/env.server.ts (3)
30-33: Consider including all runtime stages inAPP_ENVThe schema now allows only
development | staging | production.
Common additional stages (test,preview,ci,local) will now abort the process.
If those modes are used anywhere in your pipelines you’ll get a hard failure.-APP_ENV: Schema.optionalWith(Schema.Literal('development', 'staging', 'production'), { +APP_ENV: Schema.optionalWith( + Schema.Literal('development', 'staging', 'production', 'test', 'preview', 'ci', 'local'), + { default: () => 'development', }, ),
70-93: Improve error surfacing & exit strategy
#initlogs the formatted error and then throws a genericError. In a server context this can lead to an unhandled-promise rejection or express/fastify swallowing the stack.Recommend:
- Throw a custom
ConfigErrorso upstream can discriminate.- Call
process.exit(1)after logging to guarantee termination, or re-throw inside the top-level startup script.- throw new Error('Invalid environment variables') + class ConfigError extends Error { name = 'ConfigError' } + throw new ConfigError('Invalid environment variables')
114-125: Type queries reference a value – safe but surprising
type ServerEnvVars = typeof Config.instance.serverEnvrelies on a value expression.
The query is erased at runtime, soConfig.instanceis not executed, but this pattern is non-obvious and eslint-rules often flag it.Alternative:
type ServerEnvVars = Schema.Schema.Type<typeof envSchema>Same resulting type, zero cognitive overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.idea/biome.xml(1 hunks)packages/app/src/entry.server.tsx(1 hunks)packages/app/src/root.tsx(2 hunks)packages/app/src/utils/env.server.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/entry.server.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .idea/biome.xml
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/app/src/root.tsx (1)
packages/app/src/utils/env.server.ts (1)
Config(50-112)
🔇 Additional comments (1)
packages/app/src/utils/env.server.ts (1)
106-111: ExposeAPP_ENVto the client?Only
NODE_ENVis forwarded. If the intent is to let the client know whether it isstagingvsproduction, addAPP_ENVas well; otherwise delete it from the public docs to avoid confusion.
| import tailwindStyleSheetUrl from '#root/src/styles/tailwind.css?url' | ||
| import { ClientHintCheck, getHints } from '#root/src/utils/client-hints.tsx' | ||
| import { getEnv } from '#root/src/utils/env.server.ts' | ||
| import { Config } from '#root/client/utils/env.server.ts' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent alias may break module resolution
All other intra-package imports use the #root/src/* alias, while this line points to #root/client/utils/env.server.ts.
If the file truly lives under packages/app/src/utils, this will fail at build time (or silently fall back to a second copy of Config, causing two singleton instances).
Run once to verify which path actually exists:
If only src/utils/env.server.ts is returned, fix the import:
-import { Config } from '#root/client/utils/env.server.ts'
+import { Config } from '#root/src/utils/env.server.ts'🏁 Script executed:
#!/bin/bash
fd --max-depth 4 env.server.ts packages/app | sortLength of output: 89
Fix inconsistent alias in env.server import
The import in packages/app/src/root.tsx line 22 uses the wrong alias and will fail to resolve:
- import { Config } from '#root/client/utils/env.server.ts'
+ import { Config } from '#root/src/utils/env.server.ts'– packages/app/src/root.tsx:22
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Config } from '#root/client/utils/env.server.ts' | |
| import { Config } from '#root/src/utils/env.server.ts' |
🤖 Prompt for AI Agents (early access)
In packages/app/src/root.tsx at line 22, the import statement for Config uses
the incorrect alias '#root/client/utils/env.server.ts'. To fix this, update the
import path to use the consistent alias '#root/src/utils/env.server.ts' that
matches the project's aliasing convention and points to the correct file
location.
This pull request introduces significant changes to the environment configuration system, updates IDE settings for Biome and TailwindCSS, and refactors imports in client files. The most notable change is the introduction of a
Configclass to centralize and encapsulate environment variable handling, replacing the previousinitandgetEnvfunctions.Environment Configuration Refactor:
Configclass inpackages/app/client/utils/env.server.tsto manage environment variables. This class validates variables during initialization and provides separate accessors for server and client environments. ([packages/app/client/utils/env.server.tsL46-R124](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-6f7f5b77afd0dcf116b54fc32fdb95af395c28264c52f2a8a7f1258535bdb9b4L46-R124))APP_ENVvariable to the environment schema with default valuedevelopment. ([packages/app/client/utils/env.server.tsR29-R33](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-6f7f5b77afd0dcf116b54fc32fdb95af395c28264c52f2a8a7f1258535bdb9b4R29-R33))getEnvandinitto use theConfigclass instead, ensuring a consistent approach to accessing environment variables. ([[1]](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-199b196bba25d94e44b07f446aeff2b4dbf6438eea947e5f5b9880ef56dfa83fL9-R13),[[2]](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-ebbd0569b6ff90476d880554face29eb50a19da9f701e48fa0d9f283b3dd5e91L22-R22),[[3]](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-ebbd0569b6ff90476d880554face29eb50a19da9f701e48fa0d9f283b3dd5e91L70-R70))IDE Configuration Updates:
.idea/biome.xmlto setconfigurationModetoMANUALand updated theexecutablePathto point tonode_modules/.bin/biome. ([.idea/biome.xmlR6-R8](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-37d6bb47dee1f744c785f033c57e242b5dea5f2aceb4083c43628a611a9ddee6R6-R8))lspServerPackageNameoption from.idea/tailwindcss.xml, simplifying the TailwindCSS configuration. ([.idea/tailwindcss.xmlL5](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-0fc5fe522dc1a8adbe278d909993548ea982e63c2de53b1ebcc448f96f4cd605L5))Miscellaneous:
packages/app/client/root.tsxandentry.server.tsxto align with the newConfigclass implementation. ([[1]](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-ebbd0569b6ff90476d880554face29eb50a19da9f701e48fa0d9f283b3dd5e91L22-R22),[[2]](https://github.com/suddenlyGiovanni/suddenlygiovanni.dev/pull/1407/files#diff-ebbd0569b6ff90476d880554face29eb50a19da9f701e48fa0d9f283b3dd5e91L70-R70))Summary by CodeRabbit
Refactor
Chores