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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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
Instead of just filtering old events, actually remove them from the buffer. This prevents stale events from ever being matched by future listeners, even if timestamps are somehow compared incorrectly. Events older than the listener registration time are definitely stale and will never be needed by any listener.
- Add @spotlightjs/spotlight dependency to test-utils - Create spotlight.ts with startSpotlight() and event waiting helpers - Add Spotlight mode to getPlaywrightConfig() via useSpotlight option - Migrate react-router-7-lazy-routes test app as proof of concept - Use DSN workaround (http://spotlight@localhost:8969/0) - Remove custom event proxy in favor of Spotlight - Update tests to use waitForSpotlightTransaction This migration eliminates the custom event proxy server which had timing issues causing flaky tests. Spotlight provides a more robust solution for capturing and streaming Sentry events during E2E tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The buffer pruning condition (<=) was inconsistent with the live event sending condition (>=). This meant buffered events at exactly the same millisecond as listener registration would be incorrectly pruned, even though live events at the same timestamp would be sent. With millisecond timestamps, same-timestamp collisions are more likely, which could cause intermittent test failures. Changed to strict < to match the >= behavior for live events.
…y-server.ts - Add null check for portMatch[1] in spotlight.ts - Add null check for eventBuffer element in generator - Refactor buffer pruning loop to avoid 'possibly undefined' error
…cies Installing Spotlight in individual test apps causes version conflicts because Spotlight's transitive dependencies (e.g. @sentry/electron) require specific @sentry/* versions that conflict with the freshly built packages used by E2E tests. Moving to root devDependencies avoids this conflict - the Spotlight CLI is available globally without polluting test app dependency trees.
Reverting all timestamp/buffer filtering changes as they were breaking other tests. The original implementation will be kept until the Spotlight migration is complete.
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Internal planning file accidentally committed to repository
The .cursor/plans/ directory contains internal planning documentation (a Cursor AI migration plan) that was accidentally committed to the repository. This file includes implementation todos and internal planning notes that likely shouldn't be part of the codebase.
The Spotlight approach had a fundamental architecture issue: - Playwright's webServer spawns Spotlight as a separate process - Tests run in the Playwright test runner process - The event buffer in spotlight.ts only gets populated if startSpotlight() is called in the same process as the tests - Since they're different processes, the buffer is always empty Keeping spotlight.ts for future use but marking exports as experimental. The test app is reverted to use the original event proxy approach.
Instead of using Playwright's webServer to spawn Spotlight (which runs in a separate process and can't share the event buffer), tests now call startSpotlight() directly in beforeAll hooks. This way: - Test process spawns Spotlight via `spotlight run -f json` - Spotlight auto-runs the app and streams events to stdout - Test process parses stdout and populates the event buffer - waitForSpotlightTransaction() works because it's in the same process Changes: - playwright.config.mjs: Empty webServer (Spotlight handles app startup) - src/index.tsx: Use Spotlight DSN workaround instead of tunnel - tests/*.ts: Add beforeAll/afterAll for Spotlight lifecycle - Deleted start-event-proxy.mjs (no longer needed)
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Cursor AI planning file accidentally committed to repo
The .cursor/plans/ directory contains an internal Cursor AI development planning file that appears to have been accidentally committed. This file is not in .gitignore and contains internal development notes, TODO status tracking, and architecture diagrams that are typically local developer artifacts rather than production code.
Test apps are copied to a temp directory during CI, so they don't have access to the root's node_modules where @spotlightjs/spotlight is installed. Using npx will use the installed version if available or download it if needed, working regardless of the package manager.
| const spotlightEnvValue = | ||
| globalWithInjectedValues.NEXT_PUBLIC_SENTRY_SPOTLIGHT ?? | ||
| process.env._sentrySpotlight ?? | ||
| globalWithInjectedValues._sentrySpotlight; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Cursor planning file accidentally included in commit
A Cursor AI development planning file was committed to the repository. The .cursor/plans/ directory contains internal development notes and task tracking that shouldn't be in the codebase. This file should be removed and the .cursor directory should likely be added to .gitignore.
- Find monorepo root by looking for package.json with workspaces - Use spotlight binary from root's node_modules/.bin/ - Fix regex to match 'Spotlight listening on PORT' format - Resolve immediately when listening message detected Tested locally - startSpotlight correctly detects port and starts.
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Plan file accidentally committed to repository
A Cursor IDE plan file (.cursor/plans/migrate_e2e_to_spotlight_358ad839.plan.md) was accidentally committed to the repository. This is a development artifact containing internal planning notes and TODO items that should not be part of the production codebase. The file includes task status markers and implementation notes specific to a developer's local workflow.
Use import.meta.url in ESM environments and __dirname in CJS. This fixes the ReferenceError when tests run as ES modules.
- Remove unused eslint-disable directive - Type JSON.parse result properly - Add null check for stdout/stderr instead of non-null assertions
…ight - Pass PORT env var to Spotlight so app runs on expected port (3030) - Add appPort option to SpotlightOptions (defaults to 3030) - Poll app's HTTP endpoint to confirm it's ready before resolving - This fixes ERR_CONNECTION_REFUSED errors in tests
| const value = (globalThis as Record<string, unknown>)[key]; | ||
| if (typeof value === 'string') { | ||
| return value; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Silently ignore | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // Timeout after 30 seconds | ||
| setTimeout(() => { | ||
| reject(new Error('Timeout waiting for envelope item')); | ||
| }, 30000); |
There was a problem hiding this comment.
Bug: Race condition: listener not removed on timeout
The waitForEnvelopeItem function adds a listener to eventListeners but never removes it when the timeout fires. When a timeout occurs, the listener remains in the set and will continue to process events and potentially call checkEnvelope after the promise has already rejected. This can cause memory leaks and unexpected behavior. The timeout handler needs to remove the listener from eventListeners before rejecting.
Additional Locations (1)
| } | ||
| }; | ||
| eventListeners.add(listener); | ||
| })().catch(reject); |
There was a problem hiding this comment.
Bug: Race condition between buffer check and listener registration
In waitForEnvelopeItem, there's a race condition where new events could arrive between finishing the buffer loop (line 316) and adding the listener (line 325). If an event arrives during this gap, it will be added to the buffer but the current check won't see it (already finished iterating), and the listener hasn't been registered yet to catch it. The listener should be registered before checking the buffer, not after.
| eventListeners.delete(listener); | ||
| } | ||
| }; | ||
| eventListeners.add(listener); |
There was a problem hiding this comment.
Bug: Listener callback errors cause unhandled promise rejections
The async listener function at lines 319-325 can throw if the user's callback throws, but its returned Promise is never handled. When eventListeners.forEach(listener => listener(envelope)) at line 209 invokes the listener, the returned Promise is discarded. If checkEnvelope rejects (because the user callback throws), this becomes an unhandled promise rejection. Unlike the buffer iteration which has .catch(reject) to propagate errors, errors occurring during listener-based event processing are silently swallowed.
Additional Locations (1)
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Accidentally committed Cursor plan file
The .cursor/plans/migrate_e2e_to_spotlight_358ad839.plan.md file appears to be a personal development planning document from the Cursor IDE. This file contains internal implementation notes and a to-do list that shouldn't be committed to the repository. It should likely be added to .gitignore or removed from this PR.
The app's DSN is hardcoded to http://spotlight@localhost:8969/0 so Spotlight must use the same port to receive events.
| // Calculate spotlight config once for both webpack and turbopack | ||
| const spotlightConfig = | ||
| incomingUserNextConfigObject.env?.NEXT_PUBLIC_SENTRY_SPOTLIGHT ?? process.env.NEXT_PUBLIC_SENTRY_SPOTLIGHT; |
There was a problem hiding this comment.
Bug: In Next.js production builds, Spotlight silently fails to initialize if configured with framework-specific environment variables other than NEXT_PUBLIC_SENTRY_SPOTLIGHT.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The Next.js SDK's build configuration only checks for the NEXT_PUBLIC_SENTRY_SPOTLIGHT environment variable. The browser SDK's logic for detecting other framework-specific variables (e.g., VITE_SENTRY_SPOTLIGHT) is stripped from production builds because it is wrapped in /* rollup-include-development-only */ comments. This causes a silent failure where Spotlight does not initialize in Next.js production environments if developers use any environment variable other than NEXT_PUBLIC_SENTRY_SPOTLIGHT, despite the feature advertising broader support.
💡 Suggested Fix
Update the Next.js build-time configuration (withSentryConfig.ts) to recognize the full range of framework-specific environment variables for Spotlight. The configuration should check for these variables and inject the correct value into NEXT_PUBLIC_SENTRY_SPOTLIGHT during the build, ensuring consistent behavior between development and production.
🤖 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/config/withSentryConfig.ts#L354-L356
Potential issue: The Next.js SDK's build configuration only checks for the
`NEXT_PUBLIC_SENTRY_SPOTLIGHT` environment variable. The browser SDK's logic for
detecting other framework-specific variables (e.g., `VITE_SENTRY_SPOTLIGHT`) is stripped
from production builds because it is wrapped in `/* rollup-include-development-only */`
comments. This causes a silent failure where Spotlight does not initialize in Next.js
production environments if developers use any environment variable other than
`NEXT_PUBLIC_SENTRY_SPOTLIGHT`, despite the feature advertising broader support.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7471107
| - Detects the start script from package.json (`dev`, `develop`, `serve`, `start`) | ||
| - Starts the Spotlight sidecar on the specified port (or dynamic with `-p 0`) | ||
| - Streams events to stdout in JSON format (with `-f json`) | ||
| - Sets `SENTRY_SPOTLIGHT` env var for the child process |
There was a problem hiding this comment.
Bug: Development plan file accidentally committed to repository
A Cursor IDE development plan file was committed to the repository. This contains implementation notes and todos with statuses like "in_progress" and "pending" that are internal development artifacts. The .cursor/ directory should likely be in .gitignore to prevent IDE-generated files from being committed.
![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