Fix issue with manually added networks #22832

adonesky1 · 2024-02-06T20:23:05Z

Fixes issue where after manually adding a network via the network form the network switches to the newly added one but the user is also prompted to switch anyways. As is the case with adding one of the "supported" networks, the expected UX is that the network does not automatically switch but the user sees a modal after adding the network with the option to switch.

Manual testing steps

Go to Add Network in Network Selector
Scroll to the bottom and click Add Network Manually
Add any network via the network form
See that the globally selected network does not automatically switch but that a modal pops with an option to switch
Click the switch option and see that the globally selected network switches as expected.

Screenshots/Recordings

Before

Screen.Recording.2024-02-06.at.2.24.46.PM.mov

(Before) Adding a network in the onboarding flow:

Screen.Recording.2024-02-06.at.8.08.56.PM.mov

After

Screen.Recording.2024-02-06.at.2.26.41.PM.mov

(After) adding a network in the onboarding flow:

Screen.Recording.2024-02-06.at.6.07.13.PM.mov

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.

…the network switches to the newly added one but the user is also prompted to switch anyways

github-actions · 2024-02-06T20:23:27Z

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.

adonesky1 · 2024-02-06T20:32:02Z

ui/pages/settings/networks-tab/networks-form/networks-form.js

            },
            {
-              setActive: true,
+              setActive: false,


This line could also just be removed since the default value for this argument is false.

It seems no other caller of upsertNetworkConfiguration tries to override setActive. Let's just remove this and rely on the default

metamaskbot · 2024-02-06T21:06:22Z

Builds ready [2c0adcb]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (812 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	82	151	107	17	8
		domContentLoaded	10	52	20	10	5
		load	712	907	812	51	24
		domInteractive	10	52	20	10	5

Bundle size diffs

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

test/e2e/tests/onboarding.spec.js

jiexi · 2024-02-06T21:14:06Z

Tested. Great fix!

davidmurdoch

I've always expected the save button to take me back to the settings page, not the main page. This PR definitely improves the current flow, and is likely what was originally intended, so it's a win even if its not what I expect the behavior to be.

codecov · 2024-02-06T23:34:25Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (dc312cd) 68.47% compared to head (72b5268) 68.47%.
Report is 1 commits behind head on develop.

Files	Patch %	Lines
...ttings/networks-tab/networks-form/networks-form.js	25.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #22832   +/-   ##
========================================
  Coverage    68.47%   68.47%           
========================================
  Files         1088     1088           
  Lines        42956    42957    +1     
  Branches     11437    11438    +1     
========================================
+ Hits         29410    29411    +1     
  Misses       13546    13546

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-02-06T23:44:34Z

Builds ready [8921af5]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (793 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	82	137	102	14	7
		domContentLoaded	10	33	17	5	3
		load	705	976	793	71	34
		domInteractive	10	33	17	5	3

Bundle size diffs

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

ui/pages/settings/networks-tab/networks-form/networks-form.js

metamaskbot · 2024-02-07T22:34:40Z

Builds ready [3c770ff]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (810 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	80	161	108	21	10
		domContentLoaded	9	55	18	9	4
		load	738	1074	810	84	40
		domInteractive	9	55	18	9	4

Bundle size diffs

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

ui/pages/onboarding-flow/add-network-modal/index.js

test/e2e/tests/custom-rpc-history.spec.js

darkwing · 2024-02-09T03:24:45Z

Works as expected but will await response to Mark's comments.

metamaskbot · 2024-02-13T19:01:20Z

Builds ready [72b5268]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1055 ± 20 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	132	269	201	32	15
		domContentLoaded	12	68	42	22	10
		load	971	1114	1055	41	20
		domInteractive	11	68	42	22	10

Bundle size diffs

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

adonesky1 · 2024-02-13T19:21:00Z

ui/pages/settings/networks-tab/networks-form/networks-form.js

+      }

-        submitCallback?.();
+      if (addNewNetwork && !setActiveOnSubmit) {


If setActiveOnSubmit is true we switch to the network immediately so we should not show a modal prompting the user to switch to the newly added network (as happens when setNewNetworkAdded action is called.

adonesky1 · 2024-02-13T19:23:24Z

ui/pages/settings/networks-tab/networks-form/networks-form.js

-        );
-
        trackEvent({
          event: MetaMetricsEventName.CustomNetworkAdded,


This event is only called now when a network is added, not when an existing network is edited.

Gudahtt

LGTM!

fix issue where after manually adding a network via the network form …

4246a17

…the network switches to the newly added one but the user is also prompted to switch anyways

adonesky1 requested a review from a team as a code owner February 6, 2024 20:23

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 6, 2024

adonesky1 added team-extension-ux team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Feb 6, 2024

adonesky1 commented Feb 6, 2024

View reviewed changes

fix tests

2c0adcb

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 6, 2024

adonesky1 mentioned this pull request Feb 6, 2024

Subscribe multi #22202

Closed

13 tasks

adonesky1 requested review from Gudahtt, jiexi, shanejonas and tmashuang February 6, 2024 20:44

jiexi reviewed Feb 6, 2024

View reviewed changes

test/e2e/tests/onboarding.spec.js Outdated Show resolved Hide resolved

davidmurdoch previously approved these changes Feb 6, 2024

View reviewed changes

fix onboarding flow

d07717c

adonesky1 dismissed davidmurdoch’s stale review via d07717c February 6, 2024 21:30

adonesky1 added 3 commits February 6, 2024 15:49

remove notifications when adding a network in onboarding flow

6d5b587

fix issues with network form in onboarding

9e62e01

update snapshot

8921af5

adonesky1 requested review from davidmurdoch and jiexi February 6, 2024 23:28

tmashuang previously approved these changes Feb 6, 2024

View reviewed changes

Gudahtt reviewed Feb 7, 2024

View reviewed changes

ui/pages/settings/networks-tab/networks-form/networks-form.js Outdated Show resolved Hide resolved

adonesky1 requested a review from Gudahtt February 7, 2024 16:01

darkwing added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Feb 7, 2024

davidmurdoch previously approved these changes Feb 7, 2024

View reviewed changes

move trackEvent so that it is only called when networks are added

3c770ff

adonesky1 dismissed stale reviews from davidmurdoch and tmashuang via 3c770ff February 7, 2024 22:08

adonesky1 requested review from davidmurdoch and tmashuang February 7, 2024 22:08

Gudahtt reviewed Feb 8, 2024

View reviewed changes

ui/pages/onboarding-flow/add-network-modal/index.js Show resolved Hide resolved

Gudahtt reviewed Feb 8, 2024

View reviewed changes

test/e2e/tests/custom-rpc-history.spec.js Show resolved Hide resolved

tmashuang previously approved these changes Feb 8, 2024

View reviewed changes

address feedback

534f10f

adonesky1 dismissed tmashuang’s stale review via 534f10f February 13, 2024 17:52

update snapshot

bc198f5

adonesky1 requested review from Gudahtt and tmashuang February 13, 2024 18:27

Merge branch 'develop' into ad/fix-manual-network-add

72b5268

adonesky1 commented Feb 13, 2024

View reviewed changes

Gudahtt approved these changes Feb 13, 2024

View reviewed changes

tmashuang approved these changes Feb 13, 2024

View reviewed changes

adonesky1 merged commit 14d1974 into develop Feb 13, 2024

adonesky1 deleted the ad/fix-manual-network-add branch February 13, 2024 21:03

github-actions bot locked and limited conversation to collaborators Feb 13, 2024

metamaskbot added the release-11.12.0 Issue or pull request that will be included in release 11.12.0 label Feb 13, 2024

Fix issue with manually added networks #22832

Fix issue with manually added networks #22832

Uh oh!

Conversation

adonesky1 commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual testing steps

Screenshots/Recordings

Before

(Before) Adding a network in the onboarding flow:

After

(After) adding a network in the onboarding flow:

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Feb 6, 2024

Uh oh!

adonesky1 Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

jiexi Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

Uh oh!

jiexi commented Feb 6, 2024

Uh oh!

davidmurdoch left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

Uh oh!

metamaskbot commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

darkwing commented Feb 9, 2024

Uh oh!

metamaskbot commented Feb 13, 2024

Uh oh!

adonesky1 Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adonesky1 commented Feb 6, 2024 •

edited

Loading

codecov bot commented Feb 6, 2024 •

edited

Loading