Skip to content

Conversation

@arya-s
Copy link
Contributor

@arya-s arya-s commented Nov 14, 2023

Adds an onClose callback to showReportDialog.

The callback is invoked when a __sentry_reportdialog_closed__ MessageEvent is received. This is sent from the error page embed via window.postMessage and listened to in the sdk.

Please have a look at the accompanying PRs

And here is a sample project that uses onClose to color the background green again after closing the widget:
https://github.com/arya-s/sentry-onclose-report-dialog

Caveats

  • Any __sentry_reportdialog_closed__ message will call onClose, so if users use more than one widget per page it could wrongly trigger onClose. I figured the widget takes over the entire page and having more than one wouldn't make much sense, but if this is an issue I could generate a key in the sdk and pass it along to the backend and specifically listen for it back, please advise.
  • The jest test for when onClose throws, prevents jest from failing when encountering the uncaught exception when firing off the MessageEvent. This seems a bit hackish, and I had to dig into jest-environment-jsdom's source to figure out how to prevent the test from failing.
    Curiously, I couldn't reproduce this on codesandbox: https://codesandbox.io/s/distracted-dubinsky-7x59dy?file=/src/index.test.js

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@arya-s arya-s force-pushed the feat/feedback-reportdialog-onClose-callback branch from 3d011c6 to 1459a9a Compare November 22, 2023 09:45
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any sentry_reportdialog_closed message will call onClose, so if users use more than one widget per page it could wrongly trigger onClose. I figured the widget takes over the entire page and having more than one wouldn't make much sense

There should only be one widget on the page at a time, this implementation is fine.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) November 22, 2023 15:50
@AbhiPrasad AbhiPrasad merged commit c99b2ae into getsentry:develop Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants