Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent automatic rejection of confirmations #13194

Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 3, 2022

Confirmations are now only automatically rejected if a user explicitly closes the notification window. If we close the window programmatically because there are no notifications left to show, nothing gets rejected.

This partially avoids a race condition where a confirmation gets rejected automatically without the user having seen the confirmation first. This could happen if the confirmation was processed just as the notification window was being closed.

It's still possible for a confirmation that the user has never seen to get rejected as a result of the user closing the window. But at least now it's no longer possible for a confirmation to get rejected in this manner after the user resolves the last confirmation in the queue.

If there is a queued confirmation when the popup is automatically closed, the popup is triggered again so that the user is presented with it.

@Gudahtt Gudahtt force-pushed the prevent-confirmation-rejection-from-automatic-closes branch 3 times, most recently from b5a30c2 to 654b174 Compare January 4, 2022 15:59
@Gudahtt Gudahtt changed the base branch from develop to simplify-home-routing-logic January 4, 2022 15:59
@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 4, 2022

This depends upon #13202

@Gudahtt Gudahtt force-pushed the prevent-confirmation-rejection-from-automatic-closes branch 2 times, most recently from 431047f to 0701bc5 Compare January 4, 2022 17:02
@metamaskbot
Copy link
Collaborator

Builds ready [0701bc5]
Page Load Metrics (1285 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint631379517555267
domContentLoaded10771489128410048
load10771489128510048
domInteractive10771489128410048

highlights:

storybook

Base automatically changed from simplify-home-routing-logic to develop January 4, 2022 22:49
Confirmations are now only automatically rejected if a user explicitly
closes the notification window. If we close the window programmatically
because there are no notifications left to show, nothing gets rejected.

This partially avoids a race condition where a confirmation gets
rejected automatically without the user having seen the confirmation
first. This could happen if the confirmation was processed just as the
notification window was being closed.

It's still possible for a confirmation that the user has never seen to
get rejected as a result of the user closing the window. But at least
now it's no longer possible for a confirmation to get rejected in this
manner after the user resolves the last confirmation in the queue.
@Gudahtt Gudahtt force-pushed the prevent-confirmation-rejection-from-automatic-closes branch from 0701bc5 to 83fe26d Compare January 4, 2022 22:49
@Gudahtt Gudahtt marked this pull request as ready for review January 4, 2022 22:49
@Gudahtt Gudahtt requested a review from a team as a code owner January 4, 2022 22:49
@Gudahtt Gudahtt requested a review from darkwing January 4, 2022 22:49
@metamaskbot
Copy link
Collaborator

Builds ready [83fe26d]
Page Load Metrics (1186 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint661299225346166
domContentLoaded10761477118510149
load10761478118610148
domInteractive10761477118510149

highlights:

storybook

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Looks good overall!

ui/pages/home/home.component.js Show resolved Hide resolved
All windows were being detected as explicit window closures,
essentially just as they were previously, because this variable was
cleared too soon.
After the window is automatically closed, a confirmation may have been
queued up while the window was closing. If so, the popup is now re-
opened.
@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 5, 2022

When testing this with the snaps branch, it appeared that the problem was fixed but there was a new problem now: the popup never opened. I realized that it's because if the confirmation was queued while the window was closing, it would never be shown.

I've updated this branch to automatically re-trigger the UI in this circumstance as well. The PR description has been updated accordingly:

If there is a queued confirmation when the popup is automatically closed, the popup is triggered again so that the user is presented with it.

@metamaskbot
Copy link
Collaborator

Builds ready [3198d31]
Page Load Metrics (1121 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint611295274408196
domContentLoaded9881359112110852
load9881359112110852
domInteractive9881359112110852

highlights:

storybook

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit 22f931e into develop Jan 5, 2022
@Gudahtt Gudahtt deleted the prevent-confirmation-rejection-from-automatic-closes branch January 5, 2022 17:09
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants