Repository navigation
Fix client config env guard for local builds - #7386
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe Vercel non-preview deployment check in ChangesVercel Deployment Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Bun test case
participant Helper as importEnv
participant Node as Spawned Node process
participant EnvModule as ./app/env
Test->>Helper: importEnv(env)
Helper->>Node: spawn node import
Node->>EnvModule: load module with env vars
EnvModule-->>Node: validate config
Node-->>Helper: exitCode, stderr
Helper-->>Test: result object
Test->>Test: assert exit code and stderr text
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummaryThis PR tightens the
Confidence Score: 5/5Safe to merge — the production validation path is preserved and the relaxed guard only affects builds where VERCEL_ENV is absent. The guard change is narrowly scoped: it adds an explicit string-presence check for VERCEL_ENV so that local Vercel-like builds (VERCEL=1, no VERCEL_ENV) no longer incorrectly fail. Real production deployments always have VERCEL_ENV set to production or development, so the limiter-id requirement remains intact there. The one finding is a test-helper robustness issue that does not affect production behavior. web/tests/client-config-env.test.ts — the spawnSync helper lacks an explicit cwd, making it sensitive to the caller's working directory. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Module load: env.ts] --> B{VERCEL === '1'?}
B -- No --> C[isVercelNonPreviewDeployment = false\nvalidation skipped]
B -- Yes --> D{typeof VERCEL_ENV === 'string'?}
D -- No\n(local / Vercel CLI build) --> C
D -- Yes --> E{VERCEL_ENV !== 'preview'?}
E -- No\n(preview deploy) --> C
E -- Yes\n(production / development) --> F[isVercelNonPreviewDeployment = true]
F --> G{CMUX_CLIENT_CONFIG_RATE_LIMIT_ID set?}
G -- Yes --> H[Validation passes]
G -- No --> I[Zod error: required for non-preview runtimes]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Module load: env.ts] --> B{VERCEL === '1'?}
B -- No --> C[isVercelNonPreviewDeployment = false\nvalidation skipped]
B -- Yes --> D{typeof VERCEL_ENV === 'string'?}
D -- No\n(local / Vercel CLI build) --> C
D -- Yes --> E{VERCEL_ENV !== 'preview'?}
E -- No\n(preview deploy) --> C
E -- Yes\n(production / development) --> F[isVercelNonPreviewDeployment = true]
F --> G{CMUX_CLIENT_CONFIG_RATE_LIMIT_ID set?}
G -- Yes --> H[Validation passes]
G -- No --> I[Zod error: required for non-preview runtimes]
Reviews (2): Last reviewed commit: "Fix client config env test typecheck" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+51
to
+58
| const result = spawnSync( | ||
| process.execPath, | ||
| ["-e", "await import('./app/env')"], | ||
| { | ||
| env: env as NodeJS.ProcessEnv, | ||
| encoding: "utf8", | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
spawnSync call does not specify a cwd, so the subprocess resolves ./app/env relative to whatever directory the test runner was invoked from. If CI or a developer runs bun test from the repo root instead of web/, the dynamic import will fail to find the module and all three tests will produce misleading failures (exit code ≠ 0 but for the wrong reason). Pinning cwd to the directory containing the test file makes the helper robust regardless of invocation path.
Suggested change
| const result = spawnSync( | |
| process.execPath, | |
| ["-e", "await import('./app/env')"], | |
| { | |
| env: env as NodeJS.ProcessEnv, | |
| encoding: "utf8", | |
| }, | |
| ); | |
| const result = spawnSync( | |
| process.execPath, | |
| ["-e", "await import('./app/env')"], | |
| { | |
| cwd: new URL("..", import.meta.url).pathname, | |
| env: env as NodeJS.ProcessEnv, | |
| encoding: "utf8", | |
| }, | |
| ); |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/tests/client-config-env.test.ts`:
- Around line 50-58: The importEnv helper in client-config-env.test.ts uses
spawnSync without any execution bound, so add a timeout option to the spawnSync
call to prevent the child process from hanging the test run. Update the
importEnv function to include a reasonable timeout while preserving the existing
env and encoding behavior, and ensure the test still returns exitCode and stderr
from the spawn result.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 926e4b5b-c1f4-4d15-8277-fb00e6e98abb
📒 Files selected for processing (1)
web/tests/client-config-env.test.ts
Comment on lines
+50
to
+58
| function importEnv(env: Record<string, string>): { exitCode: number; stderr: string } { | ||
| const result = spawnSync( | ||
| process.execPath, | ||
| ["-e", "await import('./app/env')"], | ||
| { | ||
| env: env as NodeJS.ProcessEnv, | ||
| encoding: "utf8", | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a timeout to spawnSync to prevent CI hangs.
The subprocess call has no bound; if the child ever fails to exit (e.g. an unresolved promise introduced later in env.ts), the whole test run blocks indefinitely.
🔒 Proposed fix
const result = spawnSync(
process.execPath,
["-e", "await import('./app/env')"],
{
env: env as NodeJS.ProcessEnv,
encoding: "utf8",
+ timeout: 10_000,
},
);📝 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.
Suggested change
| function importEnv(env: Record<string, string>): { exitCode: number; stderr: string } { | |
| const result = spawnSync( | |
| process.execPath, | |
| ["-e", "await import('./app/env')"], | |
| { | |
| env: env as NodeJS.ProcessEnv, | |
| encoding: "utf8", | |
| }, | |
| ); | |
| function importEnv(env: Record<string, string>): { exitCode: number; stderr: string } { | |
| const result = spawnSync( | |
| process.execPath, | |
| ["-e", "await import('./app/env')"], | |
| { | |
| env: env as NodeJS.ProcessEnv, | |
| encoding: "utf8", | |
| timeout: 10_000, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/tests/client-config-env.test.ts` around lines 50 - 58, The importEnv
helper in client-config-env.test.ts uses spawnSync without any execution bound,
so add a timeout option to the spawnSync call to prevent the child process from
hanging the test run. Update the importEnv function to include a reasonable
timeout while preserving the existing env and encoding behavior, and ensure the
test still returns exitCode and stderr from the spawn result.
This was referenced Jul 10, 2026
This branch was successfully deployed
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.
Summary\n- require CMUX_CLIENT_CONFIG_RATE_LIMIT_ID only when VERCEL_ENV is explicitly non-preview\n- keep production deployment validation for missing limiter IDs\n- add subprocess coverage for local Vercel-like builds and production validation\n\n## Tests\n- bun test tests/client-config-env.test.ts tests/client-config-route.test.ts\n- env VERCEL=1 VERCEL_PREVIEW_COMMENTS_ENABLED=0 RESEND_API_KEY=test-resend CMUX_FEEDBACK_FROM_EMAIL=hello@example.com CMUX_FEEDBACK_RATE_LIMIT_ID=feedback-rule STACK_SECRET_SERVER_KEY=stack-secret NEXT_PUBLIC_STACK_PROJECT_ID=00000000-0000-4000-8000-000000000000 NEXT_PUBLIC_STACK_PUBLISHABLE_CLIENT_KEY=stack-public ./node_modules/.bin/next build && bun tools/build-docs-search.mjs
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Narrower env validation for one optional server var; deployed production still requires the limiter ID, with new tests locking in the behavior.
Overview
Fixes false env validation failures when building locally with
VERCEL=1but noVERCEL_ENV(e.g. Vercel CLI–like setups).CMUX_CLIENT_CONFIG_RATE_LIMIT_IDis now required only whenisVercelNonPreviewDeploymentis true:VERCEL=1,VERCEL_ENVis a defined string, and it is notpreview. Previously, an undefinedVERCEL_ENVstill satisfied!== "preview", so the limiter ID was incorrectly enforced.Production behavior is unchanged: explicit
VERCEL_ENV=production(or other non-preview values) still fails validation without the limiter ID.Adds
client-config-env.test.tswith subprocess imports of./app/envcovering local Vercel-like builds, failing production without the ID, and passing production with it.Reviewed by Cursor Bugbot for commit 0c59f4e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Relaxed env validation for client config limiter. We now require
CMUX_CLIENT_CONFIG_RATE_LIMIT_IDonly for explicit Vercel non-preview deployments, while allowing local and Vercel-like builds to run without it.isVercelNonPreviewDeploymentguard to limit checks toVERCEL=1with non-previewVERCEL_ENV.Written for commit 3af8cfb. Summary will update on new commits.
Summary by CodeRabbit