-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: fix token detection modal stuck on firefox #25279
Conversation
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. |
@@ -55,7 +54,6 @@ function AutoDetectTokenModal({ | |||
category: MetaMetricsEventCategory.Navigation, | |||
properties: { | |||
chain_id: chainId, | |||
locale: getCurrentLocale, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @salimtb 👋 any idea why this was needed in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
locale: this.locale, |
nice find 😎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix works for me.
Should this also have some failing e2e test associated with it?
I expect this would need to be cherry picked to 12.0.0 |
Builds ready [43e4a1a]
Page Load Metrics (47 ± 3 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Hey @legobeat , i started creating the e2e test for this case, then realized that the modal can't be shown in the test because In the code, the and
I wasn't sure what was the context of having the |
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
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
Pre-merge reviewer checklist