fix: cp-12.17.3 fix repeated re-rendering of approve row in simulations section #32658
+34
−26
Conversation
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
|
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. |
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations
|
Builds ready [2a46325]
UI Startup Metrics (1226 ± 65 ms)
Benchmark value 29 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 65 exceeds gate value 49 for firefox webpack home p95 backgroundConnect Sum of mean exceeds: 3ms | Sum of p95 exceeds: 18ms Sum of all benchmark exceeds: 21ms Bundle size diffs
|
Builds ready [e5862cf]
UI Startup Metrics (1233 ± 60 ms)
Benchmark value 1077 exceeds gate value 1070 for chrome browserify home mean load Benchmark value 1070 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 19 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 2233 exceeds gate value 2192 for chrome webpack home mean uiStartup Benchmark value 1739 exceeds gate value 1711 for chrome webpack home mean load Benchmark value 1733 exceeds gate value 1704 for chrome webpack home mean domContentLoaded Benchmark value 1728 exceeds gate value 1699 for chrome webpack home mean loadScripts Benchmark value 2472 exceeds gate value 2454 for chrome webpack home p95 uiStartup Sum of mean exceeds: 143ms | Sum of p95 exceeds: 19ms Sum of all benchmark exceeds: 162ms Bundle size diffs [🚀 Bundle size reduced!]
|
matthewwalsh0
requested changes
May 8, 2025
| @@ -66,7 +70,7 @@ function useBatchApproveSimulationBalanceChanges({ | |||
| }) { | |||
| return useAsyncResult( | |||
| async () => buildSimulationTokenBalanceChanges({ nestedTransactions }), | |||
| [nestedTransactions], | |||
| [nestedTransactions?.length], | |||
There was a problem hiding this comment.
If we're using the full nestedTransactions in the callback, then we have no choice but to use it as a dependency also, as otherwise we won't re-query with the latest data?
For example, if it was entirely new, but the same length.
There was a problem hiding this comment.
nestedTransactions are not likely to change thus even nestedTransactions?.length is ok to depend on and this does seem to fix re-rendering issue.
There was a problem hiding this comment.
Unfortunately we can't couple that decision here though as this component shouldn't know how often nestedTransactions will update, only that we do an async request based on the latest nested transaction data.
If this is the fix, that suggests the deeper issue is that the nestedTransactions reference is changing despite the data itself not changing.
So ideally we'd debug that or as a temporary fix only, use a "deep equal" dependency such as JSON.stringify(nestedTransactions).
Or if something actually is changing inside nestedTransactions, change the dependency to require only what is required and not the dynamic portion.
| ? () => handleEdit(change) | ||
| : undefined, | ||
| })); | ||
| const approveRow: StaticRow[] = useMemo(() => { |
There was a problem hiding this comment.
Minor, should we call this approveRows now?
Builds ready [248c93f]
UI Startup Metrics (1197 ± 63 ms)
Benchmark value 19 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 156 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 12 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 1633 exceeds gate value 1615 for firefox webpack home mean uiStartup Benchmark value 1399 exceeds gate value 1380 for firefox webpack home mean load Benchmark value 1399 exceeds gate value 1380 for firefox webpack home mean domContentLoaded Benchmark value 28 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 1375 exceeds gate value 1360 for firefox webpack home mean loadScripts Benchmark value 2040 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 1744 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1744 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 1688 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 74ms | Sum of p95 exceeds: 423ms Sum of all benchmark exceeds: 497ms Bundle size diffs
|
Builds ready [7f05376]
UI Startup Metrics (1226 ± 74 ms)
Benchmark value 1072 exceeds gate value 1070 for chrome browserify home mean load Benchmark value 1064 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 1398 exceeds gate value 1365 for chrome browserify home p95 uiStartup Benchmark value 1209 exceeds gate value 1190 for chrome browserify home p95 load Benchmark value 1199 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded Benchmark value 1196 exceeds gate value 1180 for chrome browserify home p95 firstPaint Benchmark value 26 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 959 exceeds gate value 940 for chrome browserify home p95 loadScripts Benchmark value 2244 exceeds gate value 2192 for chrome webpack home mean uiStartup Benchmark value 1740 exceeds gate value 1711 for chrome webpack home mean load Benchmark value 1733 exceeds gate value 1704 for chrome webpack home mean domContentLoaded Benchmark value 1728 exceeds gate value 1699 for chrome webpack home mean loadScripts Benchmark value 362 exceeds gate value 334 for chrome webpack home p95 firstPaint Sum of mean exceeds: 144ms | Sum of p95 exceeds: 142ms Sum of all benchmark exceeds: 286ms Bundle size diffs
|
matthewwalsh0
approved these changes
May 8, 2025
jpuri
changed the title
jpuri
changed the title
jpuri
changed the title
Builds ready [795a6b2]
UI Startup Metrics (1228 ± 60 ms)
Benchmark value 1071 exceeds gate value 1070 for chrome browserify home mean load Benchmark value 1064 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 16 exceeds gate value 15 for chrome browserify home mean getState Benchmark value 22 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 170 exceeds gate value 90 for chrome webpack home p95 backgroundConnect Benchmark value 1245 exceeds gate value 1239 for firefox browserify home mean domContentLoaded Benchmark value 112 exceeds gate value 110 for firefox browserify home mean domInteractive Benchmark value 1633 exceeds gate value 1615 for firefox webpack home mean uiStartup Benchmark value 1395 exceeds gate value 1380 for firefox webpack home mean load Benchmark value 1395 exceeds gate value 1380 for firefox webpack home mean domContentLoaded Benchmark value 1374 exceeds gate value 1360 for firefox webpack home mean loadScripts Benchmark value 1979 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 1771 exceeds gate value 1660 for firefox webpack home p95 load Benchmark value 1771 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded Benchmark value 1747 exceeds gate value 1630 for firefox webpack home p95 loadScripts Sum of mean exceeds: 75ms | Sum of p95 exceeds: 467ms Sum of all benchmark exceeds: 542ms Bundle size diffs
|
cryptotavares
approved these changes
May 9, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Fix repeated re-rendering of approve row in simulations section
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4836
Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist