Conversation
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughAdds a computed SENTRY_UPLOAD flag and fail-fast validation + errorHandler to the Sentry Vite plugin and enables sourcemap generation in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
|
@cursoragent our project is open source so I don't think we need to delete source maps |
|
Suggested GitHub reply: Makes sense — since the repo is OSS, I’ve removed the Summary
Testing
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vite.config.ts (1)
68-73:⚠️ Potential issue | 🟠 Major
build.sourcemapis not set — Sentry upload will be a no-op.By default, Vite's
build.sourcemapisfalseand produces no.mapfiles. The@sentry/vite-plugindoes not auto-enable sourcemap generation; its README explicitly requiresbuild: { sourcemap: true }(or'hidden') as a prerequisite. Without this setting, the plugin has nothing to upload.For an open-source project,
sourcemap: trueis appropriate. For closed-source projects, usesourcemap: 'hidden'to upload maps to Sentry without exposing them viasourceMappingURLcomments.🛠️ Proposed fix
build: { cssMinify: MODE === 'production', + sourcemap: true, rollupOptions: { external: [/node:.*/, 'stream', 'crypto'], }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 68 - 73, The Vite config's build block (build: { ... }) does not set sourcemap, so `@sentry/vite-plugin` has no maps to upload; update the build object in vite.config.ts to include build.sourcemap and set it to true for open-source or 'hidden' for closed-source (keeping existing cssMinify and rollupOptions intact), i.e., add the sourcemap key alongside cssMinify and rollupOptions so sourcemaps are generated for Sentry uploads.
🧹 Nitpick comments (2)
vite.config.ts (2)
14-32: Solid fail-fast guard; minor note on thetokenImpliesOrgheuristic.The
sntrys_prefix check correctly handles new-style Sentry org-scoped tokens that embed org context, makingSENTRY_ORGoptional. This is a good defensive pattern. Just be aware that if Sentry ever changes this prefix convention the guard silently stops requiringSENTRY_ORG, so it's worth a comment documenting the assumption.✏️ Suggested clarifying comment
- const tokenImpliesOrg = Boolean(authToken?.startsWith('sntrys_')) + // New-style Sentry auth tokens (prefix "sntrys_") embed the org, so + // SENTRY_ORG is not required when using one. + const tokenImpliesOrg = Boolean(authToken?.startsWith('sntrys_'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 14 - 32, Add a short clarifying comment next to the tokenImpliesOrg heuristic in the SENTRY_UPLOAD/MODE production guard explaining the assumption that Sentry org-scoped tokens start with "sntrys_" and that this check makes SENTRY_ORG optional; mention that if Sentry changes the prefix convention this logic will no longer imply org context and SENTRY_ORG must be provided. Reference the tokenImpliesOrg const and the surrounding checks for SENTRY_AUTH_TOKEN, SENTRY_PROJECT and SENTRY_ORG so reviewers understand why the heuristic exists and when it may need updating.
56-58: Operational trade-off: all Sentry API errors (including transient ones) now block deploys.Re-throwing unconditionally means a Sentry 503, DNS failure, or rate-limit during upload will halt a production deploy entirely. Consider whether you want to distinguish between configuration/auth errors (should be fatal) and transient network errors (could be retried or warned). The Sentry GitHub discussion shows this is a real-world scenario where teams hit 503s and get their builds blocked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 56 - 58, The current errorHandler in vite.config.ts unconditionally re-throws all Sentry upload errors (errorHandler: (err) => { throw err }), which blocks deploys on transient network or rate-limit failures; change errorHandler to classify errors (e.g., by HTTP status on err.status or by error.name/message) and only throw for fatal configuration/auth errors (401/403, invalid DSN), while for transient errors (5xx, ENOTFOUND, ETIMEDOUT, rate-limit headers) either perform a limited retry (e.g., 2-3 attempts with backoff) or log a warning via the logger and continue the build. Add a small helper like isTransientError(err) and use it inside errorHandler to decide between retry/log-and-continue vs throw so deploys are not blocked by temporary Sentry outages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vite.config.ts`:
- Around line 54-58: The existing comment about the bundler plugin's default
behavior is incorrect; update the comment near the errorHandler option in
vite.config.ts (the errorHandler: (err) => { throw err } block) to state that
currently the `@sentry/vite-plugin` throws by default and that this explicit
errorHandler preserves that throwing behavior as a defensive measure in case the
plugin later changes to log-and-continue; modify the comment text only (leave
the errorHandler implementation unchanged) to accurately describe current
behavior and intent.
---
Outside diff comments:
In `@vite.config.ts`:
- Around line 68-73: The Vite config's build block (build: { ... }) does not set
sourcemap, so `@sentry/vite-plugin` has no maps to upload; update the build object
in vite.config.ts to include build.sourcemap and set it to true for open-source
or 'hidden' for closed-source (keeping existing cssMinify and rollupOptions
intact), i.e., add the sourcemap key alongside cssMinify and rollupOptions so
sourcemaps are generated for Sentry uploads.
---
Nitpick comments:
In `@vite.config.ts`:
- Around line 14-32: Add a short clarifying comment next to the tokenImpliesOrg
heuristic in the SENTRY_UPLOAD/MODE production guard explaining the assumption
that Sentry org-scoped tokens start with "sntrys_" and that this check makes
SENTRY_ORG optional; mention that if Sentry changes the prefix convention this
logic will no longer imply org context and SENTRY_ORG must be provided.
Reference the tokenImpliesOrg const and the surrounding checks for
SENTRY_AUTH_TOKEN, SENTRY_PROJECT and SENTRY_ORG so reviewers understand why the
heuristic exists and when it may need updating.
- Around line 56-58: The current errorHandler in vite.config.ts unconditionally
re-throws all Sentry upload errors (errorHandler: (err) => { throw err }), which
blocks deploys on transient network or rate-limit failures; change errorHandler
to classify errors (e.g., by HTTP status on err.status or by error.name/message)
and only throw for fatal configuration/auth errors (401/403, invalid DSN), while
for transient errors (5xx, ENOTFOUND, ETIMEDOUT, rate-limit headers) either
perform a limited retry (e.g., 2-3 attempts with backoff) or log a warning via
the logger and continue the build. Add a small helper like isTransientError(err)
and use it inside errorHandler to decide between retry/log-and-continue vs throw
so deploys are not blocked by temporary Sentry outages.
| // By default the bundler plugin logs and continues on upload/release | ||
| // errors. Fail the build so we don't deploy with broken symbolication. | ||
| errorHandler: (err) => { | ||
| throw err | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@sentry/vite-plugin errorHandler default behavior documentation latest
💡 Result:
In @sentry/vite-plugin, the errorHandler option is a callback invoked when the plugin hits an error during release creation or sourcemaps upload. By default, no handler is set and the plugin throws the error, which fails/stops the Vite build. If you provide an errorHandler, the build will continue unless your handler itself throws. [1]
Fix inaccurate plugin behavior comment.
The code comment stating "by default the bundler plugin logs and continues on upload/release errors" contradicts the actual documented behaviour of @sentry/vite-plugin. By default (without an errorHandler), the plugin throws an error and stops the build. The explicit errorHandler here is appropriate as a defensive measure to preserve throwing semantics when Sentry switches to log-and-continue defaults in a future SDK version.
✏️ Suggested comment fix
- // By default the bundler plugin logs and continues on upload/release
- // errors. Fail the build so we don't deploy with broken symbolication.
+ // Override the default throw behaviour explicitly so this keeps
+ // failing the build even after Sentry switches to log-and-continue
+ // defaults in a future SDK version.
errorHandler: (err) => {
throw err
},📝 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.
| // By default the bundler plugin logs and continues on upload/release | |
| // errors. Fail the build so we don't deploy with broken symbolication. | |
| errorHandler: (err) => { | |
| throw err | |
| }, | |
| // Override the default throw behaviour explicitly so this keeps | |
| // failing the build even after Sentry switches to log-and-continue | |
| // defaults in a future SDK version. | |
| errorHandler: (err) => { | |
| throw err | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.ts` around lines 54 - 58, The existing comment about the bundler
plugin's default behavior is incorrect; update the comment near the errorHandler
option in vite.config.ts (the errorHandler: (err) => { throw err } block) to
state that currently the `@sentry/vite-plugin` throws by default and that this
explicit errorHandler preserves that throwing behavior as a defensive measure in
case the plugin later changes to log-and-continue; modify the comment text only
(leave the errorHandler implementation unchanged) to accurately describe current
behavior and intent.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/sentry-sourcemap-404-78bf
branch
from
562a06d to
e806864
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@vite.config.ts`:
- Around line 56-60: Update the inaccurate comment above the errorHandler
option: mention that `@sentry/vite-plugin` currently throws by default when no
errorHandler is provided, and that the explicit errorHandler: (err) => { throw
err } in the configuration is a defensive measure to preserve the throwing
behavior if the plugin's default changes; reference the errorHandler setting to
make intent clear.
2 participants
Prevent Sentry sourcemap upload failures from silently deleting map files and returning 404 HTML, by making failures fatal and fixing
filesToDeleteAfterUploadglobs.The Sentry Vite plugin was configured to delete sourcemap files even if the upload failed, leading to Sentry receiving the application's 404 HTML page when it tried to fetch the maps. This change ensures that upload failures halt the build and that sourcemaps are only deleted after a successful upload.
Note
Medium Risk
Build/deploy behavior changes: production builds can now fail on missing/invalid Sentry configuration and sourcemap handling changes may affect artifact size and CI/release pipelines.
Overview
Tightens Sentry behavior across server and build: Sentry initialization is now gated behind a single
SHOULD_INIT_SENTRYflag (production + DSN present + notMOCKS=true), preventing Sentry setup during mocked CI/e2e runs.On the Vite build side,
SENTRY_UPLOADis normalized to a strict boolean and production builds now fail fast when upload is enabled but required Sentry env vars are missing; the Sentry Vite plugin is configured to throw on upload/release errors and the build always generates sourcemaps (build.sourcemap: true) rather than managing deletion viafilesToDeleteAfterUploadglobs.Written by Cursor Bugbot for commit e806864. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Chores