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: Perf: Prevent rerenders on the NetworkListMenu's NetworkListItems #30287

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

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Feb 13, 2025

Description

The NetworkListItem instances in NetworkListMenu were rerendering because the callbacks weren't using useCallback.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Start the extension with ENABLE_WHY_DID_YOU_RENDER=1 yarn start
  2. Don't see loads of rerenders for NetworkListItem in the console

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 February 13, 2025 03:51
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.

@darkwing
Copy link
Contributor Author

This is presently failing because Rendered fewer hooks than expected. This may be caused by an accidental early return statement. error throws when clicking on/off the "Show test networks" toggle. This is because the generateNetworkListItem function isn't called as many times when the toggle is off.

@darkwing
Copy link
Contributor Author

New error: clicking the three-dot menu of a menu and clicking "onEdit" bricks the extension due to not as many hooks being called, likely because the "Edit" mode renders a different set of UI.

onConfirm: () => undefined,
}),
);
}, [canDeleteNetwork, dispatch, toggleNetworkMenu, showModal, network]);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can further refine this callback by only including network.chainId instead of the full network.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll push to get ESLint support for TS React files, that way we can "auto-fix"/auto-complete the dependency arrays

}),
);
setActionMode(ACTION_MODES.ADD_EDIT);
}, [dispatch, network, setEditedNetwork, setActionMode, ACTION_MODES]);
Copy link
Contributor

Choose a reason for hiding this comment

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

ACTION_MODES is an enum/constant created once outside of the component? If so then we don't necessarily need it in the dep array.

) : null}
<Box
className="multichain-network-list-menu"
hidden={!(showTestNetworks || currentlyOnTestNetwork)}
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 use of hidden here. This way we stabilise the child component, but this parent component can hide/show.

Just want to be potentially wary of doing this pattern everywhere, especially since this means that children are "mounted" and useFootGun useEffects could still be ran.

salimtb
salimtb previously approved these changes Feb 13, 2025
@salimtb
Copy link
Contributor

salimtb commented Feb 13, 2025

the build need to be fixed on this one

@salimtb salimtb dismissed stale reviews from Prithpal-Sooriya and themself via b9e2aa6 February 13, 2025 13:03
@salimtb
Copy link
Contributor

salimtb commented Feb 13, 2025

just pushed a fix for the search feature , build should be green now

Screen.Recording.2025-02-13.at.14.02.44.mov

@metamaskbot
Copy link
Collaborator

Builds ready [d5f30da]
Page Load Metrics (1613 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint38418511550292140
domContentLoaded14211827159211555
load14331848161311857
domInteractive228035157
backgroundConnect974282010
firstReactRender1599412814
getState472152010
initialActions01000
loadScripts10041373113810148
setupStore65716178
uiStartup16282406188419192
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 2.65 KiB (0.03%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [3d7200f]
Page Load Metrics (1820 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint160525281819210101
domContentLoaded158025121788212102
load159225271820209100
domInteractive27181433316
backgroundConnect972312211
firstReactRender1676412512
getState56221199
initialActions0452105
loadScripts11131891130817584
setupStore75814147
uiStartup185828582096221106
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 2.67 KiB (0.03%)
  • common: 0 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