fix: fix token detection modal stuck on firefox

[sahar-fehri]

Description

Token detection modal seems to stay stuck on firefox

Related issues

Fixes: #25256

Manual testing steps

To run locally use:
rm -rf dist && ENABLE_MV3=false yarn && ENABLE_MV3=false yarn build:dev dev --build-type main --lockdown false --snow false --apply-lavamoat false

1. Import wallet on firefox
2. Got to settings => security and privacy and disable token auto detection
3. Go back to home page and you should see the token detection modal
4. Clicking on allow should dismiss the modal and enable the token detection

Screenshots/Recordings

Before

Screen.Recording.2024-06-13.at.00.14.55.mov

After

Screen.Recording.2024-06-13.at.00.05.54.mov

Pre-merge author checklist

• I've followed MetaMask Contributor Docs and MetaMask Extension 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.

[github-actions]

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.

[sahar-fehri]

Hey @salimtb 👋 any idea why this was needed in the first place?

[darkwing]

Ohhhh, we're trying to set a value of a function instead of the locale itself. So assuming we need the locale, we could do:

const locale = useSelector(getCurrentLocale)

Is this correct?

[sahar-fehri]

Right that fixes it, i haven't seen any trackEvent passing the locale, so i removed, i guess i can put it back just in case and merge 🙏 thanks!

[bergeron]

I think selecting the locale here is fine, although it may not be necessary since it looks like events automatically get locale added on this line.

metamask-extension/app/scripts/controllers/metametrics.js

Line 761 in 92e6b94

locale: this.locale,

[bergeron]

nice find 😎

[legobeat]

Fix works for me.

Should this also have some failing e2e test associated with it?

[bergeron]

I expect this would need to be cherry picked to 12.0.0

[metamaskbot]

Builds ready [43e4a1a]

• builds: chrome, firefox
• builds (beta): chrome
• builds (flask): chrome, firefox
• builds (MMI): chrome, firefox
• builds (test): chrome, firefox
• builds (test-flask): chrome, firefox
• build viz: Build System
• mv3: Background Module Init Stats
• mv3: UI Init Stats
• mv3: Module Load Stats
• mv3: Bundle Size Stats
• mv2: E2e Actions Stats
• code coverage: Report
• storybook: Storybook
• typescript migration: Dashboard
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5
  • common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
  • content-script: 0
  • offscreen: 0
  • ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (47 ± 3 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	107	78	10	5
		domContentLoaded	9	12	10	1	0
		load	40	65	47	7	3
		domInteractive	9	12	10	1	0

Bundle size diffs

• background: 0 Bytes (0.00%)
• ui: 23 Bytes (0.00%)
• common: 0 Bytes (0.00%)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.61%. Comparing base (92e6b94) to head (43e4a1a).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #25279   +/-   ##
========================================
  Coverage    65.61%   65.61%           
========================================
  Files         1372     1372           
  Lines        54519    54520    +1     
  Branches     14282    14282           
========================================
+ Hits         35770    35771    +1     
  Misses       18749    18749           

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

[sahar-fehri]

Fix works for me.

Should this also have some failing e2e test associated with it?

Hey @legobeat , i started creating the e2e test for this case, then realized that the modal can't be shown in the test because process.env.IN_TEST is equal true in the tests.

In the code, the showAutoDetectionModal depends on canSeeModals which also depends on process.env.IN_TEST
const canSeeModals = completedOnboarding && (!onboardedInThisUISession || firstTimeFlowType === FirstTimeFlowType.import) && !process.env.IN_TEST && !newNetworkAddedConfigurationId;

and

const showAutoDetectionModal =
      canSeeModals &&
      isShowTokenAutodetectModal &&
      !showSmartTransactionsOptInModal &&
      !showWhatsNew;

I wasn't sure what was the context of having the !process.env.IN_TEST check in the canSeeModals, and mocking it in the test seemed to be tricky ..