Uh oh!
There was an error while loading. Please reload this page.
test(react-router): Add e2e assertions for debug ID injection and source map upload - #22956
Merged
Merged
Conversation
…ugin The bundler plugin deletes these files in a `finally` block in `writeBundle` that runs regardless of `sourcemaps.disable`, so forwarding the option removed the maps before `sentryOnBuildEnd` could inject debug IDs and upload them - the same end symptom as the double-injection bug, reached from the other side. Deletion still happens in `sentryOnBuildEnd`, driven by the same user option. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rce map upload The existing react-router e2e suites build real apps but never check how many debug IDs each chunk carries, so they passed throughout both the double-injection bug in #22929 and the premature source map deletion found while fixing it. This app builds against a mock Sentry server and asserts on the build output: exactly one debug ID per client chunk, source maps with real mappings present in the uploaded artifact bundles, and every shipped debug ID backed by an upload. It deliberately routes `sourcemaps` through `unstable_sentryVitePluginOptions`, the config shape that triggered the original report. Verified to fail on both bugs - two debug IDs per chunk for the first, no uploaded chunks to cross-check for the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
size-limit report 📦
|
The shared E2E job runs its "Install Playwright" step for every test application, so a build-time-only app still needs the dependency present or the job exits 127 before reaching the assertions. Matches what nextjs-sourcemaps does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… ts-node The app is ESM (`type: module`, required by react-router), so `ts-node` needs `--esm` and still fails with ERR_UNKNOWN_FILE_EXTENSION on CI. nextjs-sourcemaps gets away with plain `ts-node` only because it is CommonJS. tsx handles ESM TypeScript natively and is already the standard for running .ts scripts elsewhere in dev-packages/e2e-tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chargome
commented
Aug 3, 2026
MemberAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 899454b. Configure here.
chargome
marked this pull request as ready for review
August 3, 2026 14:15
Uh oh!
There was an error while loading. Please reload this page.
…t one `ids.length <= 1` also passed for chunks with zero debug IDs, and those chunks were then skipped by the `ids.length > 0` guard so nothing downstream checked them. A regression where injection silently missed some client chunks would have gone undetected, even though unresolvable frames are the same user-visible outcome as injecting twice. Verified both directions against a real build: stripping a chunk's snippet now fails with "found 0", adding a second one fails with "found 2". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GHSA-qwww-vcr4-c8h2 (high, CSRF bypass in RSC mode) covers react-router >= 7.12.0 < 8.3.0 with no patched 7.x release, so dependency-review rejected the new app's manifest. Existing apps still pin ^7.13.0 but are not re-checked, since dependency-review only inspects the diff. Nothing in these assertions is version-specific - they check debug ID injection and upload, which work the same on 8.x - so the app moves to 8.3.0 and drops the version from its name. Verified on 8.3.0: all 6 chunks carry exactly one debug ID, and reintroducing the trailing spread still fails the assertion with "found 2". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nicohrubec
approved these changes
Aug 4, 2026
Comment on lines
19
to
23
Member
There was a problem hiding this comment.
l: I would just remove the comment, it's clear what this does from the function name imo
MemberAuthor
There was a problem hiding this comment.
This pr was targeting another branch that got merged, the diff here is misleading
Member
There was a problem hiding this comment.
m: the title is test(react-router) and the description also only talks about the e2e app, are these changes included in this pr on purpose? if yes we should probably update the title/description
Member
There was a problem hiding this comment.
q: did we land on removing these in the major?
nicohrubec
approved these changes
Aug 4, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This adds a build-time assertion app modelled on
nextjs-sourcemaps, for react router. It asserts three things about the real build output:ref #22945