perf(6402): clear set-state-in-effect in platform UI and assets - #45423
Draft
DDDDDanica wants to merge 1 commit into
Draft
perf(6402): clear set-state-in-effect in platform UI and assets#45423DDDDDanica wants to merge 1 commit into
DDDDDanica wants to merge 1 commit into
Conversation
Part of MetaMask-planning#6402 subtask 1 (set-state-in-effect). CODEOWNERS: @MetaMask/extension-platform leftovers, @MetaMask/metamask-assets
Contributor
✨ Files requiring CODEOWNER review ✨💎 @MetaMask/metamask-assets (1 files, +5 -1)
|
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. |
Contributor
Builds ready [501f609]
⚡ Performance Benchmarks (Total: 🟢 10 pass · 🟡 13 warn · 🔴 1 fail)
Bundle size diffs
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a broad UI/hooks cleanup aimed at eliminating react-hooks/set-state-in-effect warnings by refactoring prop→state synchronization, derived values, and one-time tracking logic across platform UI pages, shared hooks, and a few components (including NFT full-image fade-in).
Changes:
- Refactors many effects that synchronously updated state into derived render-time values, refs, and/or deferred work (e.g.,
queueMicrotask,requestAnimationFrame). - Updates several pages/components to de-dupe “consume once” behaviors (toasts, route transitions, QR flows, etc.) without effect-driven state syncing.
- Improves some derived-state patterns (e.g., memoizing checksum validity in SRP import) and adjusts a few UI timing behaviors (spin delay, NFT fade-in).
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/pages/token-management/token-management.tsx | Refactors route-toast consumption logic to avoid effect-driven state syncing. |
| ui/pages/routes/global-menu-route-transition.tsx | Reworks transition-direction updates tied to route changes. |
| ui/pages/networks/networks-page.tsx | Changes edit-complete toast triggering and edited-network reset behavior. |
| ui/pages/networks/chainlist-network-picker.tsx | Resets pagination on search changes without an effect. |
| ui/pages/keychains/reveal-seed.tsx | Adjusts phishing scan reset logic and switches SRP view tracking to a ref. |
| ui/pages/defi/hooks/useDeFiPositionsV2.ts | Resets failure state when account group changes without effect-driven syncing. |
| ui/pages/custom-token-import/custom-token-import.tsx | Updates network/form reset behavior keyed off selected network changes. |
| ui/pages/contacts/components/add-contact-form.tsx | Refactors QR-driven address fill/validation and QR clearing logic. |
| ui/pages/batch-sell/pages/review/batch-sell-review-page.tsx | Closes modal when quote refresh expires without effect-based syncing. |
| ui/hooks/useTokenTracker.js | Changes tracker initialization/loading reset behavior and defers tracker construction. |
| ui/hooks/useTokenSecurityData.ts | Refactors request keying and defers fetch to avoid sync state updates in effects. |
| ui/hooks/useTheme.ts | Removes derived theme state and returns resolved theme directly. |
| ui/hooks/useSpinDelay.ts | Reworks spinner delay/min-duration state machine and timeout management. |
| ui/hooks/useShouldShowSpeedUp.js | Refactors speed-up enabling/timer scheduling logic. |
| ui/hooks/useMultichainAccountsIntroModal.ts | Replaces effect-based modal visibility syncing with derived conditions + override. |
| ui/hooks/useIntersectionObserver.ts | Alters ref reset behavior and intersection state reset handling. |
| ui/hooks/useEqualityCheck.js | Changes equality memoization logic used to stabilize references. |
| ui/hooks/useDebouncedValue.ts | Returns raw value immediately when delay is disabled. |
| ui/hooks/useCarouselManagement/useCarouselManagement.ts | Refactors download eligibility readiness initialization/reset behavior. |
| ui/components/ui/tabs/tabs.tsx | Updates active tab index syncing behavior to avoid effect-based updates. |
| ui/components/ui/metafox-logo/horizontal-logo.js | Removes theme state/effect and derives theme directly. |
| ui/components/ui/button-group/button-group.component.js | Updates active button index syncing without effect-based updates. |
| ui/components/app/wallet-overview/coin-overview.tsx | Refactors zero-fiat-balance delay reset logic. |
| ui/components/app/toast-master/toast-master.js | Switches view-tracking state to a ref for storage error toast. |
| ui/components/app/srp-input-import/srp-input-import.tsx | Memoizes checksum validity instead of storing it in state. |
| ui/components/app/qr-hardware-popover/qr-hardware-sign-request/qr-hardware-sign-request/qr-hardware-sign-request.tsx | Resets QR signing flow status on request changes without an effect. |
| ui/components/app/permission-page-container/permission-page-container.component.js | Refactors Snaps privacy warning handling logic/state. |
| ui/components/app/name/name-details/name-details.tsx | Refactors saved-name/source syncing and tracks open metric via ref. |
| ui/components/app/currency-input/currency-input.js | Moves upstream-value alignment logic and conversion-dependent syncing out of effects. |
| ui/components/app/confirm/info/row/expandable-row.tsx | Changes expandable height measurement approach to avoid forced re-render effect. |
| ui/components/app/assets/nfts/nft-details/nft-full-image.tsx | Defers visibility change via rAF to preserve fade-in behavior. |
| ui/components/app/alert-system/multiple-alert-modal/multiple-alert-modal.tsx | Refactors alert-key reconciliation logic and pending/current selection handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
18
to
+22
| const [computedValue, setComputedValue] = useState(value); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!equalityFn(value, computedValue)) { | ||
| setComputedValue(value); | ||
| } | ||
| }, [value, equalityFn, computedValue]); | ||
| if (!equalityFn(value, computedValue)) { | ||
| setComputedValue(value); | ||
| } |
Comment on lines
+88
to
92
| const [prevRef, setPrevRef] = useState(ref); | ||
| if (ref !== prevRef) { | ||
| setPrevRef(ref); | ||
| if (!ref && state.entry?.target) { | ||
| setState({ isIntersecting: initialIsIntersecting, entry: undefined }); |
Comment on lines
152
to
+155
| if (!addresses.includes(scannedAddress)) { | ||
| setInput(scannedAddress); | ||
| validate(scannedAddress); | ||
| } |
Comment on lines
+147
to
+151
| if ( | ||
| hexValue !== prevHexValue || | ||
| asset?.address !== prevAssetAddress || | ||
| isTokenPrimary !== prevIsTokenPrimary || | ||
| assetDecimals !== prevAssetDecimals || |
Comment on lines
+29
to
33
| const setContentRef = useCallback((node: HTMLSpanElement | null) => { | ||
| if (node) { | ||
| setContentHeight(node.scrollHeight); | ||
| } | ||
| }, []); |
Comment on lines
+204
to
+208
| if ( | ||
| alertKey !== prevAlertKey || | ||
| alertsKeyFingerprint !== prevAlertsKeyFingerprint || | ||
| navigableAlertsKeyFingerprint !== prevNavigableAlertsKeyFingerprint | ||
| ) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Part of MetaMask-planning#6402 subtask 1: clear
react-hooks/set-state-in-effectwarnings.What this cleanup does: React warns when
setStateruns synchronously inside auseEffect, because that forces an extra render pass (You Might Not Need an Effect). These changes replace prop→state syncing in effects with render-time updates or derived values, and keep effects for real external sync only.Scope: shared platform UI hooks/components/pages, plus NFT full-image.
Changelog
CHANGELOG entry: null
Related issues
Part of: MetaMask/MetaMask-planning#6402
Manual testing steps
Pre-merge author checklist
Pre-merge reviewer checklist