feat(browser): Add environment variable support for Spotlight configuration #18198
+2,857
−379
Conversation
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
Contributor
size-limit report 📦
|
Contributor
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
reviewed
Nov 17, 2025
Member
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
Comment on lines
3
to
9
| * 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 |
Member
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.
Comment on lines
62
to
64
|
|
||
| // No Spotlight configuration found in environment | ||
| return undefined; |
Member
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
reviewed
Nov 18, 2025
Collaborator
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
The timestamp filtering used process.hrtime() which returns time relative to an arbitrary process-local reference point. Since the event proxy server and Playwright test runner are separate Node.js processes, their hrtime values were not comparable. This caused waitForTransaction() to sometimes match transactions from previous tests (stale events leaking from the buffer), leading to flaky test failures like the react-router-7-lazy-routes duration assertion. Fix: Use Date.now() (epoch milliseconds) plus a sub-millisecond component from performance.now() to get high-resolution timestamps that are comparable across processes.
The previous fix used performance.now() for sub-millisecond precision, but this is also process-local and can cause incorrect comparisons. If an event is buffered and a listener starts in the same millisecond but with different performance.now() fractional parts, the stale event can incorrectly pass the timestamp filter. Fix: Use only Date.now() (milliseconds since epoch). This is the only reliable cross-process timestamp. Millisecond precision is sufficient since tests have gaps of 100ms+ between transactions. Also renamed getNanosecondTimestamp to getTimestamp for accuracy.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Change from >= to > when filtering buffered events. This prevents stale events from previous tests leaking through when they were buffered at the same millisecond as the new listener started. New events arriving after the listener is registered go through the callback directly (not the buffer), so they're unaffected by this filter.
The timestamp filter only applied to BUFFERED events. Live events arriving after a listener registered were sent to ALL listeners without any timestamp check. This caused stale events from previous tests to leak through if the event was still in-flight when the next test started listening. Fix: Track each listener's registration timestamp and filter live events the same way as buffered events - only send events that occurred AFTER the listener was registered.
…t time The listener was using the client's timestamp from the query param, but due to network latency, events could be buffered between when the client generated its timestamp and when the HTTP request reached the server. Timeline of the bug: 1. Client: T1 = 1000ms (generates timestamp) 2. Client: Sends HTTP request with ?timestamp=1000 3. [NETWORK DELAY] 4. Previous test's event arrives at proxy at T2 = 1002ms (buffered!) 5. Server receives HTTP request at T3 = 1003ms 7. Buffer check: 1002 > 1000 = TRUE → stale event passes! Fix: Use the server's current time when the listener is actually registered. This ensures we only receive events that arrived AFTER the listener was registered on the server.
Live events arriving at the same millisecond as listener registration were being filtered out, causing tests to timeout waiting for events that never arrived. - Buffered events: Use > (strict) - they might be from before registration - Live events: Use >= - they are guaranteed to be new, not stale This fixes the test timeouts while still preventing stale buffered events from leaking through.
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.
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.
Comment on lines
+158
to
+161
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
![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}
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.
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