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

Fix #20930 - Prevent Firefox start failure by awaiting startUISync event #20992

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

darkwing
Copy link
Contributor

Description

This PR further highlighted the problem that sometimes the UI loads before the background (most notably in Firefox). This leaves MetaMask in a state where the user sees the MetaMask fox logo and spinner but nothing actually happens. I've seen this quite a bit specifically in E2E tests.

Manual testing steps

_1. Complete usual wallet creation and subsequent login and logout, refreshing the page, etc. Everything should work as normal.

  1. Checkout UX: Load the extension HTML pages and background with async JavaScript #20843
  2. Run yarn build test
  3. Run yarn test:e2e:single test/e2e/metamask-ui.spec.js --browser firefox --leave-running --debug
  4. All tests should pass and MetaMask should properly function

Related issues

_Fixes #20930

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes #???
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@darkwing darkwing requested a review from a team as a code owner September 21, 2023 17:40
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@darkwing darkwing added the team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead label Sep 21, 2023
@Gudahtt
Copy link
Member

Gudahtt commented Sep 21, 2023

Could you update the comment on this line to remove the "in MV3" part?

// Message startUISync is used in MV3 to start syncing state with UI

As of this change, this will be relied on in all builds. That was the only comment I could find that needs updating.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally per instructions

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a0349a6) 68.40% compared to head (8432d88) 68.40%.
Report is 3 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #20992   +/-   ##
========================================
  Coverage    68.40%   68.40%           
========================================
  Files         1006     1006           
  Lines        40217    40217           
  Branches     10758    10758           
========================================
  Hits         27510    27510           
  Misses       12707    12707           
Files Changed Coverage Δ
app/scripts/metamask-controller.js 54.04% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@darkwing darkwing merged commit 48184fe into develop Sep 21, 2023
9 checks passed
@darkwing darkwing deleted the 20930-restart-error branch September 21, 2023 18:29
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 21, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [8432d88]
Page Load Metrics (1584 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint120192145199
domContentLoaded14401792158410550
load14401793158410550
domInteractive14401792158410550
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 158 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.3.0 Issue or pull request that will be included in release 11.3.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants