Skip to content

Conversation

@arya-s
Copy link
Contributor

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

Sends reportdialog_closed via window.postMessage to the parent of the error page embed in preparation of supporting an onClose callback in @sentry/browser showReportDialog.

Fixes getsentry/sentry-javascript#9433

Please see the accompanying PRs:

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Nov 14, 2023
@github-actions
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@codecov
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #59885 (5fffdc0) into master (596e46b) will increase coverage by 0.00%.
Report is 2 commits behind head on master.
The diff coverage is 75.00%.

❗ Current head 5fffdc0 differs from pull request most recent head 201ceab. Consider uploading reports for the commit 201ceab to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #59885   +/-   ##
=======================================
  Coverage   80.86%   80.87%           
=======================================
  Files        5183     5183           
  Lines      227620   227618    -2     
  Branches    38300    38299    -1     
=======================================
+ Hits       184072   184085   +13     
+ Misses      37928    37918   -10     
+ Partials     5620     5615    -5     
Files Coverage Δ
src/sentry/testutils/pytest/selenium.py 74.65% <75.00%> (+1.73%) ⬆️

... and 8 files with indirect coverage changes

@arya-s arya-s force-pushed the feat/feedback-close-message-event branch 3 times, most recently from 459fe56 to ec6d03a Compare November 14, 2023 13:32
@AbhiPrasad AbhiPrasad added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Nov 21, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
window.parent.postMessage('reportdialog_closed', '*');
window.parent.postMessage('sentry_reportdialog_closed', '*');

Let's namespace to sentry to reduce possible collisions/confusions as much as possible

Copy link
Contributor Author

@arya-s arya-s Nov 22, 2023

Choose a reason for hiding this comment

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

How about even more of an indication that it comes from a library: __sentry_reportdialog_closed__?

Copy link
Member

Choose a reason for hiding this comment

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

Even better!

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Selenium tests and embed logic changes look good to me.

Sends `reportdialog_closed` via window.postMessage to the parent of the
error page embed in preparation of supporting an `onClose` callback in
@sentry/browser `showReportDialog`.

Fixes getsentry/sentry-javascript#9433
@arya-s arya-s force-pushed the feat/feedback-close-message-event branch from ec6d03a to 5fffdc0 Compare November 22, 2023 09:21
@github-actions github-actions bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Nov 22, 2023
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.

Thanks a lot for the PR - lets get this deployed and then merge in changes for SDK + docs :)

@AbhiPrasad AbhiPrasad added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Nov 22, 2023
@github-actions github-actions bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Nov 22, 2023
@AbhiPrasad
Copy link
Member

/gcbrun

@arya-s
Copy link
Contributor Author

arya-s commented Nov 22, 2023

Thanks a lot for the PR - lets get this deployed and then merge in changes for SDK + docs :)

My pleasure, please let me know if there's anything else needed.

@AbhiPrasad AbhiPrasad added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Nov 22, 2023
@AbhiPrasad
Copy link
Member

/gcbrun

@AbhiPrasad AbhiPrasad merged commit 635b182 into getsentry:master Nov 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add onClose callback (or equiv) to showReportDialog

3 participants