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 #17848 - Ensure NFT collections toggle appropriately #17972

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Mar 2, 2023

Explanation

This weird effect of the collections toggling themselves was due to mutating the current state, as evidenced by this error:

Uncaught Error: Invariant failed: A state mutation was detected between dispatches, in the path 'metamask.nftsDropdownState.0x0836f5ed6b62baf60706fe3adc0ff0fd1df833da.0x1'.  This may cause incorrect behavior. (https://redux.js.org/style-guide/style-guide#do-not-mutate-state)

This PR prevents the mutation and thus fixes the issue.

Manual Testing Steps

  1. Start the extension with NFTS turned on: NFTS_V1 yarn start
  2. Toggle a collection chevron to hide the collection
  3. Wait 10 seconds, see the collection is still closed

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@darkwing darkwing requested a review from a team as a code owner March 2, 2023 23:29
@darkwing darkwing requested a review from adonesky1 March 2, 2023 23:29
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

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.

NidhiKJha
NidhiKJha previously approved these changes Mar 6, 2023
@darkwing
Copy link
Contributor Author

darkwing commented Mar 6, 2023

This PR relies on #17988 being merged.

@metamaskbot
Copy link
Collaborator

Builds ready [3c64642]
Page Load Metrics (1844 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint100155126147
domContentLoaded15412084179715574
load15962161184414168
domInteractive15412084179715574
Bundle size diffs
  • background: 0 bytes
  • ui: 11 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #17972 (2f918bf) into develop (abff495) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2f918bf differs from pull request most recent head c7de835. Consider uploading reports for the commit c7de835 to get more accurate results

@@           Coverage Diff            @@
##           develop   #17972   +/-   ##
========================================
  Coverage    63.44%   63.44%           
========================================
  Files          903      903           
  Lines        35267    35266    -1     
  Branches      8920     8920           
========================================
+ Hits         22373    22374    +1     
+ Misses       12894    12892    -2     
Impacted Files Coverage Δ
ui/components/app/nfts-items/nfts-items.js 93.55% <100.00%> (-0.10%) ⬇️
app/scripts/migrations/023.js 93.75% <0.00%> (-3.12%) ⬇️
app/scripts/metamask-controller.js 60.18% <0.00%> (+0.25%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [d2988a3]
Page Load Metrics (1684 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97150128157
domContentLoaded14721931165212761
load14822024168414771
domInteractive14721931165212761

@metamaskbot
Copy link
Collaborator

Builds ready [2f918bf]
Page Load Metrics (1635 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1012621283517
domContentLoaded1368174315928943
load13681844163511555
domInteractive1368174315928943
Bundle size diffs
  • background: 0 bytes
  • ui: 11 bytes
  • common: 0 bytes

@darkwing darkwing force-pushed the 17848-nft-dropdowns branch from c7de835 to 5f844a7 Compare March 7, 2023 15:40
@darkwing
Copy link
Contributor Author

darkwing commented Mar 8, 2023

These tests will pass after #18044 is merged.

@darkwing darkwing force-pushed the 17848-nft-dropdowns branch from a4f2274 to 288a9b7 Compare March 9, 2023 16:43
@metamaskbot
Copy link
Collaborator

Builds ready [288a9b7]
Page Load Metrics (1559 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9813111694
domContentLoaded1310168315348239
load1322168615598239
domInteractive1310168315348239
Bundle size diffs
  • background: 0 bytes
  • ui: 11 bytes
  • common: 0 bytes

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@darkwing darkwing merged commit 423e085 into develop Mar 9, 2023
@darkwing darkwing deleted the 17848-nft-dropdowns branch March 9, 2023 18:10
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2023
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.

[Bug]: NFT dropdown for owned and previously owned NFTs displays/hiddens NFTs after some seconds
5 participants