feat(browser): Add environment variable support for Spotlight configuration #18198
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
…ration - Add support for multiple framework/bundler-specific environment variables with proper precedence - SENTRY_SPOTLIGHT (highest priority - base name, supported natively by many bundlers) - PUBLIC_SENTRY_SPOTLIGHT (SvelteKit, Astro, Qwik) - NEXT_PUBLIC_SENTRY_SPOTLIGHT (Next.js) - VITE_SENTRY_SPOTLIGHT (Vite) - NUXT_PUBLIC_SENTRY_SPOTLIGHT (Nuxt.js) - REACT_APP_SENTRY_SPOTLIGHT (Create React App) - VUE_APP_SENTRY_SPOTLIGHT (Vue CLI) - GATSBY_SENTRY_SPOTLIGHT (Gatsby) - Add defensive environment variable access via process.env (transformed by all major bundlers) - Move envToBool utility from node-core to core for shared usage - Add resolveSpotlightOptions utility for consistent precedence rules - Update node-core and aws-serverless to use shared utilities - Add comprehensive tests for all new utilities and SDK integration Note: import.meta.env is intentionally not checked because bundlers only replace static references (e.g., import.meta.env.VITE_VAR) at build time, not dynamic access. All major bundlers transform process.env references, making it the universal solution.
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
92ffd33 to
6cb5513
Compare
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
4f6dcd0 to
fda3d61
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). Fixes a test failure where `getSpotlightConfig` was returning empty or whitespace-only strings for `SENTRY_SPOTLIGHT` environment variables, instead of `undefined`. This change explicitly filters out such values, aligning the function's behavior with test expectations and preventing invalid Spotlight configurations. --- <a href="https://cursor.com/background-agent?bcId=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> --------- Co-authored-by: Cursor Agent <[email protected]>
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
First pass review. I still want to figure out why size bot reports a slight increase for the CDN bundles.
packages/browser/src/utils/env.ts
Outdated
| * Checks process.env which is transformed by most bundlers (Webpack, Vite, Rollup, Rspack, Parcel, etc.) | ||
| * at build time. | ||
| * | ||
| * Note: We don't check import.meta.env because: | ||
| * 1. Bundlers only replace static references like `import.meta.env.VITE_VAR`, not dynamic access | ||
| * 2. Dynamic access causes syntax errors in unsupported environments | ||
| * 3. Most bundlers transform process.env references anyway |
There was a problem hiding this comment.
m: My impression while doing some research for #18050 (comment) was what very few bundlers inject process.env into browser bundles. For example, this code will not work in Angular.
Vite instructs checking on import.meta.env, so not sure if it double-writes to process.env (my gut feeling is no).
Did you test this in the respective frameworks? Maybe I'm also misinformed and this works just fine 😅 Ideally we can add an e2e test for at least NextJS and some Vite-based framework app to be sure.
|
|
||
| // No Spotlight configuration found in environment | ||
| return undefined; |
There was a problem hiding this comment.
l: neat size trick: You can let the function return void and simply omit the return undefined here. Saves a couple of bytes and JS returns undefined anyway. 😅 (but feel free to ignore since this is shaken out for prod builds anyway)
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). --- This PR addresses critical feedback regarding environment variable detection, particularly for Vite-based frameworks. **Key Changes:** * **`packages/browser/src/utils/env.ts`**: The `getEnvValue` function now checks both `process.env` (for Webpack, Next.js, CRA) and `import.meta.env` (for Vite, Astro, SvelteKit). This ensures that environment variables (like those for Spotlight) are correctly detected across a wider range of bundlers and frameworks, fixing a significant compatibility issue. * **`packages/browser/test/utils/env.test.ts`**: Updated unit tests to focus on `process.env` scenarios. Added a note explaining that `import.meta.env` cannot be unit tested due to its read-only, compile-time nature and is covered by e2e tests. * **`packages/browser/src/utils/spotlightConfig.ts`**: Added a comment to clarify the explicit `return undefined` for readability, noting its optimization in production builds. --- <a href="https://cursor.com/background-agent?bcId=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> Co-authored-by: Cursor Agent <[email protected]>
| if (typeof import.meta !== 'undefined' && import.meta.env) { | ||
| // @ts-expect-error import.meta.env is typed differently in different environments | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| const value = import.meta.env[key]; |
There was a problem hiding this comment.
Bug: Dynamic Env Access Breaks Bundler Builds
Dynamic property access import.meta.env[key] won't work with build-time environment variable replacement in bundlers like Vite, Astro, and SvelteKit. These bundlers require static property access like import.meta.env.VARIABLE_NAME for compile-time replacement. The dynamic bracket notation will always return undefined in production builds because the object doesn't exist at runtime.
There was a problem hiding this comment.
@BYK I have no idea if this is true but seems relevant! Sounds like in these cases the bundler only replaces the full string/identifier?
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
Looks good but needs some e2e tests of some kind otherwise we could inadvertently break this in the future.
It should be quite easy to test Vite by adding some additional tests to dev-packages/bundler-tests with the specific variables set and then check they make it into the bundle.
Then as @Lms24 says, a Nextjs test would probably be a good idea too!
There was a problem hiding this comment.
Bug: Breaking change: `envToBool` export removed from `@sentry/node-core` (Bugbot Rules)
The envToBool function was previously exported from @sentry/node-core but has been removed in this change (now exported from @sentry/core instead). This is a breaking change for any external consumers who were importing envToBool directly from @sentry/node-core. The function is still available from @sentry/core, but existing code using import { envToBool } from '@sentry/node-core' will break. Flagging this per the review rules about "Removal of publicly exported functions, classes, or types."
packages/node-core/src/index.ts#L44-L45
sentry-javascript/packages/node-core/src/index.ts
Lines 44 to 45 in b6fda2b
There was a problem hiding this comment.
Bug: Removed `module` field may break older bundler resolution (Bugbot Rules)
The module field was removed from package.json while other packages in the monorepo still have it (e.g., @sentry/aws-serverless, @sentry/browser-utils, @sentry/angular). This field is used by older bundlers and tools that don't fully support the exports field. While modern bundlers should work correctly with the exports configuration, this removal could be a breaking change for users with legacy build setups. Flagging because this relates to potential breaking changes in public APIs as mentioned in the rules file.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in d556863
There was a problem hiding this comment.
Bug: Removal of `module` field from browser package.json (Bugbot Rules)
The module field was removed from package.json, which is a potential breaking change per the review rules. Older bundlers and environments that don't fully support the exports field rely on the module field to find the ESM entry point. Other Sentry packages like @sentry/aws-serverless, @sentry/browser-utils, and @sentry/bun retain their module fields. This removal may cause some bundlers to fail to resolve the ESM build, falling back to the CJS main entry point instead.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 64c4692
There was a problem hiding this comment.
Bug: Browser package `module` field accidentally removed
The module field pointing to "build/npm/esm/prod/index.js" was removed from packages/browser/package.json. While modern bundlers use the exports field, older bundlers (webpack 4, older rollup configs) rely on the module field to locate ESM entry points. Without it, these bundlers will fall back to the main field (CJS), potentially breaking ESM tree-shaking for users on legacy toolchains. Other similar packages like @sentry-internal/feedback, @sentry-internal/replay, and @sentry/wasm still have their module field intact.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 9f1e28f
In production builds, the automatic Spotlight enablement from env vars
is stripped (by design). The test now explicitly reads VITE_SENTRY_SPOTLIGHT
and passes it to Sentry.init({ spotlight: ... }) which is the recommended
approach for production builds.
Configure Vite's resolve.conditions to use 'development' exports from @sentry/* packages. This includes the Spotlight auto-enablement code that reads VITE_SENTRY_SPOTLIGHT from environment variables. The SDK has conditional exports: - 'development': includes Spotlight auto-enablement - 'production': strips Spotlight code (default for builds)
Switch from vite build+preview to vite dev server because: - Spotlight is designed for development mode - Dev server automatically uses 'development' exports from SDK - More realistic test of how users would use Spotlight Changes: - test:build now only runs install (no build needed for dev server) - playwright starts 'pnpm dev' instead of 'pnpm preview' - Removed resolve.conditions hack (dev server handles this)
New test application (nextjs-15-spotlight) that: - Uses 'next dev' to test development-mode behavior - Verifies Spotlight auto-enablement from NEXT_PUBLIC_SENTRY_SPOTLIGHT - Has increased timeout (90s) for dev server startup - Tests that no syntax errors occur during initialization This tests the realistic scenario where Spotlight is used during development with Next.js.
Next.js/webpack doesn't use the 'development' export condition by default, even in 'next dev' mode. Added webpack config to enable it, which allows the SDK's development-only features like Spotlight auto-enablement to work.
The SDK's webpack config now automatically adds 'development' to resolve.conditionNames when running 'next dev'. This enables: - Spotlight auto-enablement from NEXT_PUBLIC_SENTRY_SPOTLIGHT env var - Other development-only SDK features Users no longer need manual webpack configuration for Spotlight to work.
Reverted browser-webworker-vite to use 'vite build' + 'vite preview' because the errors.test.ts tests expect production-style filenames (bundled .js files). Added resolve.conditions to get SDK development exports during build, enabling Spotlight auto-enablement while keeping production-like behavior for worker tests.
Updated worker filename patterns to accept both: - Dev mode: worker.ts?worker_file&type=module - Prod mode: worker-[hash].js Also removed debug_meta assertions since they're not present in dev mode. Kept Vite using dev server for realistic Spotlight testing.
There was a problem hiding this comment.
Bug: Removal of `module` field may break older bundlers
The "module" field was removed from package.json. While the "exports" field provides ESM paths, older bundlers (pre-Node 12.7 resolution, older webpack configs, Rollup 1.x) that don't fully support the "exports" field will fall back to the "main" field (CJS) instead of ESM. This could cause unexpected behavior or bundle size increases for users with legacy build configurations. The removal appears unrelated to the Spotlight feature being implemented.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 433c0ac
dev-packages/e2e-tests/test-applications/browser-webworker-vite/tests/errors.test.ts
Outdated
Show resolved
Hide resolved
The automatic addition of 'development' to webpack conditionNames causes issues with Next.js client boundaries due to 'export *' statements in ESM builds. Reverted to manual config in test app only.
Only prepend 'development' to existing conditionNames if they exist, preserving Next.js's original ESM/CJS resolution behavior. This avoids the 'export *' client boundary error while still enabling SDK development features like Spotlight auto-enablement.
If conditionNames is not set, use webpack's default conditions with 'development' prepended. This ensures Spotlight works even when Next.js doesn't initialize conditionNames.
When setting default conditions, include 'browser' for client bundles and 'node' for server bundles. This is required for @sentry/browser to resolve correctly on the client side.
There was a problem hiding this comment.
Bug: Removed `module` field may break older bundlers
The module field was removed from package.json, but other packages with similar build structures (@sentry/feedback, @sentry-internal/replay, etc.) retain it. Bundlers that don't fully support the exports field or conditional exports may fall back to the main field (CJS) instead of using ESM, potentially causing duplicate module issues or breaking tree-shaking in older build environments.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 7cffb67
Instead of modifying conditionNames (which wasn't working reliably), use webpack's resolve.alias to explicitly point @sentry/browser to its CJS development build for client bundles in dev mode. This enables Spotlight auto-enablement from env vars in Next.js dev mode.
There was a problem hiding this comment.
Bug: Removing `module` field may break older bundlers
The module field was removed from package.json while other Sentry packages (like @sentry/core, @sentry/browser-utils) still have it. The module field is used by older bundlers (like Webpack 4 and some Rollup configurations) that don't fully support the exports field. While modern bundlers will use the exports field correctly, removing module could cause older bundlers to fall back to the CJS main entry point instead of ESM, potentially causing issues or larger bundle sizes for users on older toolchains. The removal appears intentional for the dev/prod conditional exports feature, but this inconsistency with other packages could lead to unexpected behavior.
packages/browser/package.json#L14-L15
sentry-javascript/packages/browser/package.json
Lines 14 to 15 in dde88ca
packages/nextjs/src/client/index.ts
Outdated
| opts.integrations = [ | ||
| ...(Array.isArray(opts.integrations) ? opts.integrations : []), | ||
| spotlightBrowserIntegration(spotlightArgs), | ||
| ]; |
There was a problem hiding this comment.
Bug: User integrations function silently lost when Spotlight enabled
When NEXT_PUBLIC_SENTRY_SPOTLIGHT is set and options.spotlight is undefined, the code converts opts.integrations to an array using Array.isArray(opts.integrations) ? opts.integrations : []. If the user provided integrations as a callback function (which is a supported configuration per the CoreOptions type), the function check fails (Array.isArray returns false), so it falls back to an empty array. This silently discards the user's custom integration logic and only adds the Spotlight integration. The SDK officially supports integrations as either an array OR a function, and this code only handles the array case.
| const spotlightEnvValue = process.env.NEXT_PUBLIC_SENTRY_SPOTLIGHT; | ||
| if (spotlightEnvValue !== undefined && options.spotlight === undefined) { | ||
| const boolValue = envToBool(spotlightEnvValue, { strict: true }); | ||
| const spotlightConfig = boolValue !== null ? boolValue : spotlightEnvValue; | ||
| const spotlightValue = resolveSpotlightOptions(undefined, spotlightConfig); | ||
|
|
||
| if (spotlightValue) { | ||
| const spotlightArgs = typeof spotlightValue === 'string' ? { sidecarUrl: spotlightValue } : undefined; | ||
| customDefaultIntegrations.push(spotlightBrowserIntegration(spotlightArgs)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Silent configuration mismatch occurs when PUBLIC_SENTRY_SPOTLIGHT and NEXT_PUBLIC_SENTRY_SPOTLIGHT are both set, leading to an unintended override.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
When both PUBLIC_SENTRY_SPOTLIGHT and NEXT_PUBLIC_SENTRY_SPOTLIGHT environment variables are set to different values, the Next.js SDK initializes a Spotlight integration using NEXT_PUBLIC_SENTRY_SPOTLIGHT. However, the Browser SDK subsequently initializes another Spotlight integration using PUBLIC_SENTRY_SPOTLIGHT due to its higher priority in environment variable resolution. The filterDuplicates mechanism then silently discards the Next.js-configured integration in favor of the Browser SDK's integration, leading to an unintended configuration override and violating the user's expectation for NEXT_PUBLIC_SENTRY_SPOTLIGHT prioritization.
💡 Suggested Fix
Ensure consistent environment variable resolution logic across SDKs or modify the integration deduplication mechanism to prioritize framework-specific integrations. Alternatively, prevent the Browser SDK from adding a duplicate Spotlight integration if one is already provided by the Next.js SDK.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/nextjs/src/client/index.ts#L149-L159
Potential issue: When both `PUBLIC_SENTRY_SPOTLIGHT` and `NEXT_PUBLIC_SENTRY_SPOTLIGHT`
environment variables are set to different values, the Next.js SDK initializes a
Spotlight integration using `NEXT_PUBLIC_SENTRY_SPOTLIGHT`. However, the Browser SDK
subsequently initializes another Spotlight integration using `PUBLIC_SENTRY_SPOTLIGHT`
due to its higher priority in environment variable resolution. The `filterDuplicates`
mechanism then silently discards the Next.js-configured integration in favor of the
Browser SDK's integration, leading to an unintended configuration override and violating
the user's expectation for `NEXT_PUBLIC_SENTRY_SPOTLIGHT` prioritization.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5849182
| const spotlightArgs = typeof spotlightValue === 'string' ? { sidecarUrl: spotlightValue } : undefined; | ||
| customDefaultIntegrations.push(spotlightBrowserIntegration(spotlightArgs)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Next.js Spotlight env handling lacks development-only guard
The Spotlight integration is added in the Next.js client based on NEXT_PUBLIC_SENTRY_SPOTLIGHT without any development-only guard. Unlike the browser SDK which wraps equivalent code in /* rollup-include-development-only */ comments, this code runs in both development and production builds. If a user sets NEXT_PUBLIC_SENTRY_SPOTLIGHT=true, Spotlight will be enabled in production, which contradicts the documented guidance that Spotlight should only be used in development.
![https://cursor.com/open-in-cursor-dark.svg"><source](https://cursor.com/open-in-cursor-dark.svg"><source){kind=link}
![https://cursor.com/open-in-cursor-light.svg"><img](https://cursor.com/open-in-cursor-light.svg"><img){kind=link}
![https://cursor.com/open-in-cursor.svg"></picture></a> <a](https://cursor.com/open-in-cursor.svg"></picture></a> <a){kind=link}
![https://cursor.com/open-in-web-dark.svg"><source](https://cursor.com/open-in-web-dark.svg"><source){kind=link}
![https://cursor.com/open-in-web-light.svg"><img](https://cursor.com/open-in-web-light.svg"><img){kind=link}
![https://cursor.com/open-in-web.svg"></picture></a](https://cursor.com/open-in-web.svg"></picture></a){kind=link}
Implements full Spotlight spec with support for multiple framework-specific
environment variable prefixes. Adds defensive environment variable access
for both process.env and import.meta.env to support various bundlers.
Supported environment variables (in priority order):
PUBLIC_SENTRY_SPOTLIGHT(SvelteKit, Astro, Qwik)NEXT_PUBLIC_SENTRY_SPOTLIGHT(Next.js)VITE_SENTRY_SPOTLIGHT(Vite)NUXT_PUBLIC_SENTRY_SPOTLIGHT(Nuxt)REACT_APP_SENTRY_SPOTLIGHT(Create React App)VUE_APP_SENTRY_SPOTLIGHT(Vue CLI)GATSBY_SENTRY_SPOTLIGHT(Gatsby)SENTRY_SPOTLIGHT(base/official)SENTRY_SPOTLIGHTis last as in environments like Docker Compose, we actually make the front-end env variable different than the baseSENTRY_SPOTLIGHTone -- the backends need to reachdocker.host.internalwhereas front-ends always needlocalhostas we assume the browser runs on the same host with Spotlight.Refactors envToBool utility from node-core to core package for shared usage.
Adds resolveSpotlightOptions utility to ensure consistent precedence rules
across Browser and Node SDKs.
Includes comprehensive test coverage for all new utilities and integration
tests for environment variable precedence behavior.
Closes #18404