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: Prevent rerenders of the AccountListItem #30873

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Mar 7, 2025

Description

The AccountListMenu has been been flagged as slow in the past, so I've memoized some selectors and child elements to make the menu render much, much less.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Check out main branch
  2. Start the extension with the ENABLE_WHY_DID_YOU_RENDER flag on
  3. Open the Account List Menu
  4. See loads of rerendering of the AccountListItem due to 4 props changing (onClick, closeMenu, etc)
  5. Checkout this branch PR
  6. Start the extension with the ENABLE_WHY_DID_YOU_RENDER flag on
  7. Open the Account List Menu
  8. See 0 re-renders of the AccountListItem

Screenshots/Recordings

Before

After

Pre-merge author checklist

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 March 7, 2025 21:07
Copy link
Contributor

github-actions bot commented Mar 7, 2025

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 force-pushed the faster-account-list-menu branch from 90b6a1d to 0a679f6 Compare March 7, 2025 21:16
@darkwing darkwing changed the title Prevent rerenders of the AccountListItem fix: Prevent rerenders of the AccountListItem Mar 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [0a679f6]
Page Load Metrics (1772 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1638191417707838
domContentLoaded1601185517215929
load1639196417728440
domInteractive26121382110
backgroundConnect13201534924
firstReactRender1569322110
getState6100242613
initialActions01000
loadScripts1200144213065928
setupStore860262110
uiStartup181829292079262126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 125 Bytes (0.00%)
  • common: 169 Bytes (0.00%)

export function getInternalAccounts(state: AccountsState) {
return Object.values(state.metamask.internalAccounts.accounts);
}
export const getInternalAccounts = createSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice work with the createSelector here!

Copy link
Contributor

Choose a reason for hiding this comment

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

(future) - can we add an equality check to prove that this is not creating a new array, and causing extra rerenders?

it('returns same result with exact same state', () => {
  const result1 = getInternalAccounts(state);
  const result2 = getInternalAccounts(state);
  expect(result1 === result2).toBe(true)
})

vinnyhoward
vinnyhoward previously approved these changes Mar 7, 2025
@darkwing darkwing dismissed stale reviews from vinnyhoward and Prithpal-Sooriya via 72a4090 March 8, 2025 02:13
@metamaskbot
Copy link
Collaborator

Builds ready [214cea0]
Page Load Metrics (2051 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18062426205219192
domContentLoaded17532281197616780
load179725082051209100
domInteractive227936136
backgroundConnect13214765124
firstReactRender1695322110
getState5152443617
initialActions01000
loadScripts13441747151614067
setupStore9441595
uiStartup199837102463453218
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 178 Bytes (0.00%)
  • common: 169 Bytes (0.00%)

@darkwing darkwing force-pushed the faster-account-list-menu branch from 214cea0 to dd2efdb Compare March 8, 2025 03:43
@metamaskbot
Copy link
Collaborator

Builds ready [dd2efdb]
Page Load Metrics (2185 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91425952101329158
domContentLoaded18702595212019995
load19162601218520799
domInteractive28206604321
backgroundConnect10201645225
firstReactRender17102452512
getState7118463215
initialActions05011
loadScripts14262072163417584
setupStore96220168
uiStartup214140652654487234
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 178 Bytes (0.00%)
  • common: 169 Bytes (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants