fix: Remove multiple overlapping spinners #28301

pedronfigueiredo · 2024-11-05T17:17:34Z

Description

We were previously using a UI blocking, whole page spinner while loading the redesigned confirmations. This overlapped with the individual component spinners we use on some components that decode transaction data. That second pattern is preferable because the spinner is contained and doesn't block the user from taking action.

The global spinner comes from routes.component. The first part of this fix is to bypass it in that file for redesigned confirmations.

The second aspect of this fix is to not condense two component spinners into one. Since the transaction flow section always loads before the send heading component, we can omit the spinner for that first component.

Finally, this PR fixes the loading behaviour on the send heading component so that if the decimals amount coming from useAssetDetails hasn't been received yet, the heading is not shown, preventing undesireable flickering in the hero value.

Related issues

Fixes: #27972

Manual testing steps

Go to test dapp
Click on malicious erc20 transfer to see the 2 spinners on top and the different one in the middle
Click on malicious erc20 approve to see the different spinner in the middle

Screenshots/Recordings

Before

After

Screen.Recording.2024-11-05.at.16.45.16.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

github-actions · 2024-11-05T17:17:47Z

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.

metamaskbot · 2024-11-05T18:04:40Z

Builds ready [e48a72a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2100 ± 81 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	264	2519	1925	576	277
		domContentLoaded	1810	2509	2064	164	79
		load	1850	2521	2100	168	81
		domInteractive	31	285	61	53	26
		backgroundConnect	9	75	28	19	9
		firstReactRender	63	290	120	58	28
		getState	9	71	25	21	10
		initialActions	0	1	0	0	0
		loadScripts	1331	1928	1537	143	69
		setupStore	12	98	34	25	12
		uiStartup	2069	3017	2366	236	113

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 909 Bytes (0.01%)
common: 0 Bytes (0.00%)

HowardBraham · 2024-11-06T02:34:37Z

@pedronfigueiredo are we going to have a direct merge conflict, or maybe even a functional merge conflict?
#28172

pedronfigueiredo · 2024-11-06T11:37:28Z

Thanks for flagging this @HowardBraham, excited for your PR!

However, I checked out your branch and we still see the three issues my PR aims to solve:

The global spinner that blocks the UI
The multiple loading spinners for each component
The flickering amount in the send heading component

Screen.Recording.2024-11-06.at.11.27.16.mov

So I think we need both PRs.

digiwand · 2024-11-06T12:45:18Z

ui/pages/confirmations/components/confirm/info/hooks/use-token-values.ts

+        decodedTransferValue: new BigNumber(
+          value.data[0].params[paramIndex].value.toString(),
+        )
+          .dividedBy(new BigNumber(10).pow(Number(decimals)))
+          .toNumber(),


[nit] could use calcTokenAmount here. Then, we could return toFixed to ensure precision is preserved. Then there would be no need for toNonScientificString below

digiwand

It appears toNonScientificString is a newly added method in our repo as of a week ago. I think we can remove the need fo toNonScientificString by avoiding the scientific notation conversion that can be applied when JavaScript handles lengthy digits. This could be done by passing values as strings to BigNumber directly and using toFixed or toString instead of toNumber to display numbers

unblock PR to be merged

metamaskbot · 2024-11-07T17:25:03Z

Builds ready [63168a9]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1772 ± 88 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1518	2258	1777	191	92
		domContentLoaded	1506	2232	1752	183	88
		load	1515	2246	1772	183	88
		domInteractive	18	80	44	17	8
		backgroundConnect	8	61	23	17	8
		firstReactRender	48	211	100	32	16
		getState	4	66	13	16	8
		initialActions	0	0	0	0	0
		loadScripts	1089	1647	1289	144	69
		setupStore	11	69	23	18	8
		uiStartup	1796	2458	1985	187	90

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 56 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-11-08T10:45:31Z

Builds ready [f56df68]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2227 ± 131 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	312	2763	1951	689	331
		domContentLoaded	1739	2739	2184	264	127
		load	1754	2816	2227	272	131
		domInteractive	18	199	62	41	20
		backgroundConnect	9	91	45	25	12
		firstReactRender	63	170	113	20	10
		getState	5	83	26	21	10
		initialActions	0	1	0	0	0
		loadScripts	1270	2004	1616	199	96
		setupStore	7	59	14	14	7
		uiStartup	1961	3054	2466	278	133

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 56 Bytes (0.00%)
common: 0 Bytes (0.00%)

pedronfigueiredo · 2024-11-08T11:18:23Z

I addressed removing the toNonScientificNotation function as well as the precision issue mentioned #28212 on this PR

metamaskbot · 2024-11-08T11:54:28Z

Builds ready [3de0647]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2015 ± 150 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1782	3357	2018	330	159
		domContentLoaded	1771	3186	1991	299	144
		load	1780	3258	2015	312	150
		domInteractive	19	83	47	18	8
		backgroundConnect	9	78	23	19	9
		firstReactRender	51	162	112	25	12
		getState	5	50	11	11	5
		initialActions	0	1	0	0	0
		loadScripts	1288	2447	1465	248	119
		setupStore	6	57	17	18	9
		uiStartup	1979	3695	2238	360	173

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 56 Bytes (0.00%)
common: 0 Bytes (0.00%)

digiwand · 2024-11-11T17:13:02Z

ui/pages/confirmations/components/confirm/info/approve/hooks/use-approve-token-simulation.ts

-      : new Intl.NumberFormat(locale).format(decodedSpendingCap);
+    return isNFT || parseInt(decodedSpendingCap, 10) < 1
+      ? decodedSpendingCap
+      : new Intl.NumberFormat(locale).format(parseInt(decodedSpendingCap, 10));


This might cause some precision issues with parseInt here. Examples of supporting precision passing BN to Intl.NumberFormat can be found here
ui/pages/confirmations/components/simulation-details/formatAmount.ts

digiwand

nice! left one minor comment above

metamaskbot · 2024-11-13T12:13:48Z

Builds ready [17b7015]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1920 ± 98 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1627	2435	1924	202	97
		domContentLoaded	1616	2415	1893	207	99
		load	1625	2438	1920	204	98
		domInteractive	15	85	49	24	11
		backgroundConnect	8	71	28	22	10
		firstReactRender	84	218	111	34	16
		getState	5	62	21	22	11
		initialActions	0	1	0	0	0
		loadScripts	1158	1772	1394	172	83
		setupStore	6	79	13	18	9
		uiStartup	1820	2756	2165	261	125

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 56 Bytes (0.00%)
common: 0 Bytes (0.00%)

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Nov 5, 2024

pedronfigueiredo self-assigned this Nov 5, 2024

pedronfigueiredo requested review from a team as code owners November 5, 2024 17:17

pedronfigueiredo force-pushed the pnf/27972 branch from ac33280 to e48a72a Compare November 5, 2024 17:28

jpuri previously approved these changes Nov 5, 2024

View reviewed changes

digiwand reviewed Nov 6, 2024

View reviewed changes

digiwand previously requested changes Nov 6, 2024

View reviewed changes

pedronfigueiredo dismissed jpuri’s stale review via 63168a9 November 7, 2024 16:51

pedronfigueiredo force-pushed the pnf/27972 branch from 63168a9 to f56df68 Compare November 8, 2024 10:11

pedronfigueiredo force-pushed the pnf/27972 branch from f56df68 to 3de0647 Compare November 8, 2024 11:22

digiwand reviewed Nov 11, 2024

View reviewed changes

digiwand previously approved these changes Nov 11, 2024

View reviewed changes

vinistevam previously approved these changes Nov 12, 2024

View reviewed changes

pedronfigueiredo added this pull request to the merge queue Nov 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024

pedronfigueiredo added this pull request to the merge queue Nov 13, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 13, 2024