feat(browser): Add onError callback to showReportDialog - #24780
Conversation
Closes: #24765 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
|
batman begin |
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 1c0b723. Configure here.
|
|
||
| if (onError) { | ||
| script.onerror = () => { | ||
| onError(new Error('Failed to load the report dialog script')); |
There was a problem hiding this comment.
It may be prudent to propagate the original error via { cause: ev.error } depending on the compatibility you are targeting.
There was a problem hiding this comment.
Just tested this and browsers just give back a plain Event here, there's no .error field. That being said, there's one case where the report dialog errors with a 400 that would be lumped in with this hardcoded error message.
| } | ||
|
|
||
| if (onError) { | ||
| script.onerror = () => { |
There was a problem hiding this comment.
Doesn't the error handler need to be registered before src is set? Same for onLoad really.
There was a problem hiding this comment.
To the best of my knowledge, the script starts loading only once its attached to the DOM, so when we set these properties makes no difference.
This is different from when you write a script tag directly to the DOM.
There was a problem hiding this comment.
Yep, scripts load only when added to the DOM, unlike images.
| DEBUG_BUILD && debug.error('[showReportDialog] No event ID'); | ||
| onError?.(new Error('No event ID to show the report dialog for')); | ||
| return; |
There was a problem hiding this comment.
q: The docs say, onError only reports load errors. Wondering if we should adjust docs (probably) or remove the invocation here, wdyt?
| if (!mergedOptions.eventId) { | ||
| DEBUG_BUILD && debug.error('[showReportDialog] No event ID'); | ||
| onError?.(new Error('No event ID to show the report dialog for')); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think rejecting on missing eventId is a bit of a behavior change, probably breaking.
But it never worked in the first place 🤔
EDIT: Never mind, if they don't have the callback then nothing happens. Disregard.
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Thought it was breaking change for a second but seems good to me.
…ry-javascript into ab/report-dialog-on-error
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
## DESCRIBE YOUR PR
Document the `showReportDialog({ onError })` callback added in
getsentry/sentry-javascript#24780. Explain the
`Error` argument for a missing event ID or a script-loading failure, and
show how to notify users when the report dialog cannot open. Clarify
that this callback does not handle report-submission errors.
Keep this draft until the SDK change is released, then add the confirmed
minimum JavaScript SDK version before merging. The v7 documentation is
unchanged.
## IS YOUR CHANGE URGENT?
- [ ] Urgent deadline (GA date, etc.): YYYY-MM-DD
- [ ] Other deadline: YYYY-MM-DD
- [x] No deadline: Not urgent, can wait up to 1 week+
<!-- junior-request-attribution:start -->
via Junior system actor `event`.
<!-- junior-request-attribution:end -->
<!-- junior-session-footer:start -->
<!-- junior-conversation-id:slack%3AC0BKK9QLCUA%3A1790585017.857999 -->
--
[View Junior
Session](https://junior-prod.sentry.dev/conversations/slack%3AC0BKK9QLCUA%3A1790585017.857999)
[[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AC0BKK9QLCUA%3A1790585017.857999/?project=4510944073809921)
<!-- junior-session-footer:end -->
---------
Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com>
Co-authored-by: Andrei Borza <andrei.borza@sentry.io>
5 participants
What
showReportDialogaccepts a newonErrorcallback. The SDK calls it when the dialog cannot be shown: the dialog script fails to load, or there is no event ID.Why
Ad blockers or network problems can block the dialog script, and there was no way to detect it. Apps can now tell users why the dialog did not open.
Closes: #24765