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: ensure create-password.js doesn't attempt to change routes twice #24874

Merged
merged 1 commit into from
May 29, 2024

Conversation

danjm
Copy link
Contributor

@danjm danjm commented May 29, 2024

Description

Multiple onboarding tests are flaky, failing with the error TimeoutError: Waiting for element to be located By(css selector, [data-testid="recovery-phrase-reveal"])

The problem is that the test can get to the page where that selector is present, but then be re-routed back to the previous screen. This is due to a race condition.

in create-password.js there is this code:

useEffect(() => {
   if (currentKeyring) {
     if (firstTimeFlowType === FirstTimeFlowType.import) {
       ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
       history.replace(ONBOARDING_COMPLETION_ROUTE);
       ///: END:ONLY_INCLUDE_IF

       ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
       history.replace(ONBOARDING_PIN_EXTENSION_ROUTE);
       ///: END:ONLY_INCLUDE_IF
     } else {
       ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
       history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
       ///: END:ONLY_INCLUDE_IF

       ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
       history.replace(SRP_REMINDER);
       ///: END:ONLY_INCLUDE_IF
     }
   }
 }, [currentKeyring, history, firstTimeFlowType]);

and there is this also this code inside handleCreate():

} else {
      // Otherwise we are in create new wallet flow
      try {
        if (createNewAccount) {
          await createNewAccount(password);
        }
        ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask)
        history.push(ONBOARDING_SECURE_YOUR_WALLET_ROUTE);
        ///: END:ONLY_INCLUDE_IF

        ///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
        history.replace(SRP_REMINDER);
        ///: END:ONLY_INCLUDE_IF
      } catch (error) {
        setPasswordError(error.message);
      }

What is happening is:

  1. user clicks submit/confirm on the create password screen
  2. await createNewAccount(password); occurs
  3. before that fully resolves, at least a new keyring is created, so the useEffect fires and history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE); so the user/test is taken to the next route and continues with the clicks on the "Secure your wallet" and then "Write down your Secret Recovery Phrase" screen
  4. then await createNewAccount(password); resolves, and the user/test is rerouted to the "Secure Your Wallet" screen again

The fix is to prevent the history.replace(ONBOARDING_SECURE_YOUR_WALLET_ROUTE) call in the useEffect if handleCreate has already started to create a new account

Open in GitHub Codespaces

Related issues

Fixes: #24608

Manual testing steps

e2e tests should pass

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@danjm danjm requested a review from a team as a code owner May 29, 2024 15:59
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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 29, 2024
@seaona
Copy link
Contributor

seaona commented May 29, 2024

amazing finding and fix 🔥 It looks good from QA.
ℹ️ To test locally (from Dan): add a small delay after getSeedPhrase from action.ts file

 await createNewVault(password);
 const seedPhrase = await getSeedPhrase(password);
  await new Promise(resolve => setTimeout(resolve, 500));

Before

redirect-onboarding.mp4

After

redirect-fix.mp4

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.78%. Comparing base (0df1d57) to head (89b8597).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #24874   +/-   ##
========================================
  Coverage    65.77%   65.78%           
========================================
  Files         1366     1366           
  Lines        54254    54256    +2     
  Branches     14101    14101           
========================================
+ Hits         35685    35687    +2     
  Misses       18569    18569           

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

Copy link
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

Great catch! Thanks!

@danjm danjm merged commit 40195d9 into develop May 29, 2024
77 of 79 checks passed
@danjm danjm deleted the test-7890 branch May 29, 2024 17:15
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
@metamaskbot metamaskbot added release-11.18.0 release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels May 29, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-11.16.6 Issue or pull request that will be included in release 11.16.6 team-extension-platform
Projects
None yet
4 participants