fix: breaking selector due to missing controller state #12375

Prithpal-Sooriya · 2024-11-21T16:15:45Z

Description

The selector is trying to access some engine state that is removed at build time through code-fences.
This fix ensures that the selectors are using the engine state or default to the controllers default state.

Long term - we need to investigate and ensure that the UI is not calling the selectors.

Related issues

Fixes: #11909

Manual testing steps

Can you start up the application without it breaking?

Screenshots/Recordings

Before / After Recording
https://www.loom.com/share/badc4b4cd3b0446486497973dcf337f6?sid=33bcfe23-c495-41bf-89f5-e57b4b72e3d2

I was not able to replicate the controller state being undefined/missing, so forced it to be undefined when testing.

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile 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.

the selector is trying to access some engine state that is removed at build time through code-fences. Long term - we need to investigate and ensure that the UI is not calling the selectors.

github-actions · 2024-11-21T16:15:58Z

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.

app/selectors/notifications/index.tsx

github-actions · 2024-11-21T16:53:56Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 4540de3
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/fa2865f3-b5ee-4385-9ece-7bc947326d7d

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

matteoscurati

LGTM

sethkfman · 2024-11-21T17:06:20Z

@Prithpal-Sooriya can you show manual confirmation this has fixed the issue?

…tamask-mobile into hotfix/fix-breaking-selector

Prithpal-Sooriya · 2024-11-22T10:31:45Z

app/selectors/notifications/index.tsx

+  state?.engine?.backgroundState?.UserStorageController ??
+  UserStorageController.defaultState;


This is the main fix for the missing controller state.

It is really odd that the controller state is missing sometimes. I have a hypothesis, but it is hard to prove.

Maybe we should have it as mobile best practice to "distrust controller state", or maybe at the type level make the controllers partial/optional (but this would be a large breaking change).

I think I prefer this selector default state approach instead of migrations, as the redux state should do a second re-hydration once the engine has been initialised. We can then keep migrations only for state patches/fixes.

The easiest way to proof this is creating a migration for testing purposes and delete the UserStorageController data if exists, wdyt?

Prithpal-Sooriya · 2024-11-22T10:34:11Z

app/util/notifications/hooks/useCreateSession.ts

+    if (!isNotificationsFeatureEnabled()) {
+      return;
+    }


All these other changes to hooks is to ensure we aren't unintentionally calling these hooks when the feature is off.

app/util/notifications/hooks/useSwitchNotifications.ts

Prithpal-Sooriya · 2024-11-22T11:16:07Z

app/components/Views/Settings/SecuritySettings/__snapshots__/SecuritySettings.test.tsx.snap

@@ -3583,188 +3583,8 @@ exports[`SecuritySettings should render correctly 1`] = `
      swipeDirection="down"
      swipeThreshold={100}
      transparent={true}
-      visible={true}
-    >
-      <View


Snapshot change as we are now not calling any notification hooks during a side effect.

TODO: I want us to clean up/remove this effect:

metamask-mobile/app/components/Views/Settings/SecuritySettings/SecuritySettings.tsx

Lines 209 to 216 in 35c51df

useEffect(() => {

const triggerCascadeBasicFunctionalityDisable = async () => {

if (!isBasicFunctionalityEnabled) {

isNotificationEnabled && (await disableNotifications());

isProfileSyncingEnabled && (await disableProfileSyncing());

}

};

triggerCascadeBasicFunctionalityDisable();

I don't want side effects to cascade the updates for basic functionality (especially since this can be re-invoked from dependency changes). Instead I want this to be done during the basic functionality switch-off.

github-actions · 2024-11-22T11:17:27Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 344002f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/27765754-fa23-4f5a-9405-4be22391be53

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Prithpal-Sooriya · 2024-11-22T11:23:56Z

@sethkfman I've attached a recording for a before and after the changes.

I've also sneaked in some other changes to ensure notification hooks are not called when the feature is disabled.
If this is too many changes, I'm happy to make them separate PRs.

sonarcloud · 2024-11-22T17:15:01Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

fix: breaking selector

df8e3b9

the selector is trying to access some engine state that is removed at build time through code-fences. Long term - we need to investigate and ensure that the UI is not calling the selectors.

Prithpal-Sooriya requested a review from a team as a code owner November 21, 2024 16:15

metamaskbot added the team-notifications Notifications team label Nov 21, 2024

Prithpal-Sooriya commented Nov 21, 2024

View reviewed changes

app/selectors/notifications/index.tsx Show resolved Hide resolved

Merge branch 'main' into hotfix/fix-breaking-selector

4540de3

Prithpal-Sooriya added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 21, 2024

matteoscurati previously approved these changes Nov 21, 2024

View reviewed changes

mathieuartu previously approved these changes Nov 21, 2024

View reviewed changes

Prithpal-Sooriya added 2 commits November 22, 2024 10:28

fix: ensure notification hooks are not called when flag is disabled

01a1859

Merge branch 'hotfix/fix-breaking-selector' of github.com:MetaMask/me…

825d2a4

…tamask-mobile into hotfix/fix-breaking-selector

Prithpal-Sooriya dismissed stale reviews from mathieuartu and matteoscurati via 825d2a4 November 22, 2024 10:29

Prithpal-Sooriya requested review from a team as code owners November 22, 2024 10:29

Prithpal-Sooriya commented Nov 22, 2024

View reviewed changes

app/util/notifications/hooks/useSwitchNotifications.ts Outdated Show resolved Hide resolved

fix: tests and linting

344002f

Prithpal-Sooriya requested a review from a team as a code owner November 22, 2024 11:13

Prithpal-Sooriya changed the title ~~fix: breaking selector~~ fix: breaking selector due to missing controller state Nov 22, 2024

Prithpal-Sooriya commented Nov 22, 2024

View reviewed changes

Prithpal-Sooriya added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 22, 2024

mathieuartu approved these changes Nov 22, 2024

View reviewed changes

Merge branch 'main' into hotfix/fix-breaking-selector

3f63b51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: breaking selector due to missing controller state #12375

fix: breaking selector due to missing controller state #12375

Prithpal-Sooriya commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited by metamaskbot

Loading

matteoscurati left a comment

sethkfman commented Nov 21, 2024

Prithpal-Sooriya Nov 22, 2024 •

edited

Loading

Prithpal-Sooriya Nov 22, 2024

tommasini Nov 22, 2024

Prithpal-Sooriya Nov 22, 2024

Prithpal-Sooriya Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited by metamaskbot

Loading

Prithpal-Sooriya commented Nov 22, 2024

sonarcloud bot commented Nov 22, 2024

		state?.engine?.backgroundState?.UserStorageController ??
		UserStorageController.defaultState;

	useEffect(() => {
	const triggerCascadeBasicFunctionalityDisable = async () => {
	if (!isBasicFunctionalityEnabled) {
	isNotificationEnabled && (await disableNotifications());
	isProfileSyncingEnabled && (await disableProfileSyncing());
	}
	};
	triggerCascadeBasicFunctionalityDisable();

fix: breaking selector due to missing controller state #12375

Are you sure you want to change the base?

fix: breaking selector due to missing controller state #12375

Conversation

Prithpal-Sooriya commented Nov 21, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 • edited by metamaskbot Loading

Bitrise

matteoscurati left a comment

Choose a reason for hiding this comment

sethkfman commented Nov 21, 2024

Prithpal-Sooriya Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Prithpal-Sooriya Nov 22, 2024

Choose a reason for hiding this comment

tommasini Nov 22, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Nov 22, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2024 • edited by metamaskbot Loading

Bitrise

Prithpal-Sooriya commented Nov 22, 2024

sonarcloud bot commented Nov 22, 2024

Quality Gate passed

Prithpal-Sooriya commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited by metamaskbot

Loading

Prithpal-Sooriya Nov 22, 2024 •

edited

Loading

Prithpal-Sooriya Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited by metamaskbot

Loading