From 47ff61d9047faa257d8723df74484133b1002d3a Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Fri, 19 Jul 2024 12:13:23 +0100 Subject: [PATCH] fix: notification slowness and crashes (#25946) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This is a series of fixes added to core libraries. Adding to extension, and soon we will migrate extension to use shared libraries. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25946?quickstart=1) ## **Related issues** https://github.com/MetaMask/core/pull/4530 https://github.com/MetaMask/core/pull/4531 https://github.com/MetaMask/core/pull/4532 https://github.com/MetaMask/core/pull/4533 https://github.com/MetaMask/core/pull/4536 https://github.com/MetaMask/metamask-extension/issues/25749 ## **Manual testing steps** 1. Create multiple accounts 2. Go to notification settings page 3. Wait for settings to load, and try toggling notifications Before: some settings would error or not load After: you should be able to toggle accounts and not see errors. ## **Screenshots/Recordings** ### **Before** [](https://github.com/MetaMask/metamask-extension/issues/25749) ### **After** https://www.loom.com/share/49b582e8c33b4199bdafc994a3f6087f?sid=9f94a885-351b-4fee-84d8-f72c97506e7d ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --- .../authentication-controller.test.ts | 6 +- .../authentication-controller.ts | 39 ++++--- .../metamask-notifications.ts | 36 ++++--- .../services/feature-announcements.ts | 19 ++-- .../metamask-notifications/utils/utils.ts | 2 +- .../user-storage/user-storage-controller.ts | 16 ++- .../useSwitchNotifications.ts | 77 +++++++++++++- .../notifications-settings-per-account.tsx | 100 ++++++++++-------- .../notifications-settings.tsx | 55 +++++----- 9 files changed, 233 insertions(+), 117 deletions(-) diff --git a/app/scripts/controllers/authentication/authentication-controller.test.ts b/app/scripts/controllers/authentication/authentication-controller.test.ts index 1a86cee38c82..b9b272c51a62 100644 --- a/app/scripts/controllers/authentication/authentication-controller.test.ts +++ b/app/scripts/controllers/authentication/authentication-controller.test.ts @@ -280,11 +280,7 @@ function createMockAuthenticationMessenger() { ); } - function exhaustedMessengerMocks(action: never) { - throw new Error(`MOCK_FAIL - unsupported messenger call: ${action}`); - } - - return exhaustedMessengerMocks(actionType); + throw new Error(`MOCK_FAIL - unsupported messenger call: ${actionType}`); }); return { messenger, mockSnapGetPublicKey, mockSnapSignMessage }; diff --git a/app/scripts/controllers/authentication/authentication-controller.ts b/app/scripts/controllers/authentication/authentication-controller.ts index 24da63bd58bf..d3586e05419b 100644 --- a/app/scripts/controllers/authentication/authentication-controller.ts +++ b/app/scripts/controllers/authentication/authentication-controller.ts @@ -4,7 +4,6 @@ import { StateMetadata, } from '@metamask/base-controller'; import { HandleSnapRequest } from '@metamask/snaps-controllers'; -import { UserStorageControllerDisableProfileSyncing } from '../user-storage/user-storage-controller'; import { createSnapPublicKeyRequest, createSnapSignMessageRequest, @@ -84,9 +83,7 @@ export type AuthenticationControllerGetSessionProfile = export type AuthenticationControllerIsSignedIn = ActionsObj['isSignedIn']; // Allowed Actions -export type AllowedActions = - | HandleSnapRequest - | UserStorageControllerDisableProfileSyncing; +export type AllowedActions = HandleSnapRequest; // Messenger export type AuthenticationControllerMessenger = RestrictedControllerMessenger< @@ -271,8 +268,6 @@ export default class AuthenticationController extends BaseController< }; } catch (e) { console.error('Failed to authenticate', e); - // Disable Profile Syncing - this.messagingSystem.call('UserStorageController:disableProfileSyncing'); const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); throw new Error( @@ -299,28 +294,48 @@ export default class AuthenticationController extends BaseController< return THIRTY_MIN_MS > diffMs; } + #_snapPublicKeyCache: string | undefined; + /** * Returns the auth snap public key. * * @returns The snap public key. */ - #snapGetPublicKey(): Promise { - return this.messagingSystem.call( + async #snapGetPublicKey(): Promise { + if (this.#_snapPublicKeyCache) { + return this.#_snapPublicKeyCache; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapPublicKeyRequest(), - ) as Promise; + )) as string; + + this.#_snapPublicKeyCache = result; + + return result; } + #_snapSignMessageCache: Record<`metamask:${string}`, string> = {}; + /** * Signs a specific message using an underlying auth snap. * * @param message - A specific tagged message to sign. * @returns A Signature created by the snap. */ - #snapSignMessage(message: `metamask:${string}`): Promise { - return this.messagingSystem.call( + async #snapSignMessage(message: `metamask:${string}`): Promise { + if (this.#_snapSignMessageCache[message]) { + return this.#_snapSignMessageCache[message]; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), - ) as Promise; + )) as string; + + this.#_snapSignMessageCache[message] = result; + + return result; } } diff --git a/app/scripts/controllers/metamask-notifications/metamask-notifications.ts b/app/scripts/controllers/metamask-notifications/metamask-notifications.ts index f9929a049b54..4af08474769c 100644 --- a/app/scripts/controllers/metamask-notifications/metamask-notifications.ts +++ b/app/scripts/controllers/metamask-notifications/metamask-notifications.ts @@ -266,22 +266,34 @@ export class MetamaskNotificationsController extends BaseController< #pushNotifications = { enablePushNotifications: async (UUIDs: string[]) => { - return await this.messagingSystem.call( - 'PushPlatformNotificationsController:enablePushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'PushPlatformNotificationsController:enablePushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to enable push notifications', e); + } }, disablePushNotifications: async (UUIDs: string[]) => { - return await this.messagingSystem.call( - 'PushPlatformNotificationsController:disablePushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'PushPlatformNotificationsController:disablePushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to disable push notifications', e); + } }, updatePushNotifications: async (UUIDs: string[]) => { - return await this.messagingSystem.call( - 'PushPlatformNotificationsController:updateTriggerPushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'PushPlatformNotificationsController:updateTriggerPushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to update push notifications', e); + } }, subscribe: () => { this.messagingSystem.subscribe( diff --git a/app/scripts/controllers/metamask-notifications/services/feature-announcements.ts b/app/scripts/controllers/metamask-notifications/services/feature-announcements.ts index 0fb911497cd7..8b3c3870627e 100644 --- a/app/scripts/controllers/metamask-notifications/services/feature-announcements.ts +++ b/app/scripts/controllers/metamask-notifications/services/feature-announcements.ts @@ -26,7 +26,7 @@ export type ContentfulResult = { async function fetchFromContentful( url: string, - retries = 3, + retries = 1, retryDelay = 1000, ): Promise { let lastError: Error | null = null; @@ -113,10 +113,17 @@ async function fetchFeatureAnnouncementNotifications(): Promise< export async function getFeatureAnnouncementNotifications(): Promise< Notification[] > { - const rawNotifications = await fetchFeatureAnnouncementNotifications(); - const notifications = rawNotifications.map((notification) => - processFeatureAnnouncement(notification), - ); + if ( + process.env.CONTENTFUL_ACCESS_SPACE_ID && + process.env.CONTENTFUL_ACCESS_TOKEN + ) { + const rawNotifications = await fetchFeatureAnnouncementNotifications(); + const notifications = rawNotifications.map((notification) => + processFeatureAnnouncement(notification), + ); + + return notifications; + } - return notifications; + return []; } diff --git a/app/scripts/controllers/metamask-notifications/utils/utils.ts b/app/scripts/controllers/metamask-notifications/utils/utils.ts index f71f9e568dcd..547a38b217d5 100644 --- a/app/scripts/controllers/metamask-notifications/utils/utils.ts +++ b/app/scripts/controllers/metamask-notifications/utils/utils.ts @@ -603,7 +603,7 @@ export async function makeApiCall( endpoint: string, method: 'POST' | 'DELETE', body: T, - retries = 3, + retries = 1, retryDelay = 1000, ): Promise { const options: RequestInit = { diff --git a/app/scripts/controllers/user-storage/user-storage-controller.ts b/app/scripts/controllers/user-storage/user-storage-controller.ts index bcbc81618da8..bc5c0b77c2bb 100644 --- a/app/scripts/controllers/user-storage/user-storage-controller.ts +++ b/app/scripts/controllers/user-storage/user-storage-controller.ts @@ -365,17 +365,27 @@ export default class UserStorageController extends BaseController< return storageKey; } + #_snapSignMessageCache: Record<`metamask:${string}`, string> = {}; + /** * Signs a specific message using an underlying auth snap. * * @param message - A specific tagged message to sign. * @returns A Signature created by the snap. */ - #snapSignMessage(message: `metamask:${string}`): Promise { - return this.messagingSystem.call( + async #snapSignMessage(message: `metamask:${string}`): Promise { + if (this.#_snapSignMessageCache[message]) { + return this.#_snapSignMessageCache[message]; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), - ) as Promise; + )) as string; + + this.#_snapSignMessageCache[message] = result; + + return result; } async #setIsProfileSyncingUpdateLoading( diff --git a/ui/hooks/metamask-notifications/useSwitchNotifications.ts b/ui/hooks/metamask-notifications/useSwitchNotifications.ts index 0d60b16714a2..53e121473bac 100644 --- a/ui/hooks/metamask-notifications/useSwitchNotifications.ts +++ b/ui/hooks/metamask-notifications/useSwitchNotifications.ts @@ -1,5 +1,5 @@ -import { useState, useCallback } from 'react'; -import { useDispatch } from 'react-redux'; +import { useState, useCallback, useMemo, useEffect } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; import log from 'loglevel'; import { setFeatureAnnouncementsEnabled, @@ -8,6 +8,7 @@ import { updateOnChainTriggersByAccount, hideLoadingIndication, } from '../../store/actions'; +import { getIsUpdatingMetamaskNotificationsAccount } from '../../selectors/metamask-notifications/metamask-notifications'; export function useSwitchFeatureAnnouncementsChange(): { onChange: (state: boolean) => Promise; @@ -114,3 +115,75 @@ export function useSwitchAccountNotificationsChange(): { error, }; } + +function useRefetchAccountSettings() { + const dispatch = useDispatch(); + + const getAccountSettings = useCallback(async (accounts: string[]) => { + try { + const result = (await dispatch( + checkAccountsPresence(accounts), + )) as unknown as UseSwitchAccountNotificationsData; + + return result; + } catch { + return {}; + } + }, []); + + return getAccountSettings; +} + +/** + * Account Settings Hook. + * Gets initial loading states, and returns enable/disable account states. + * Also exposes an update() method so each switch can be manually updated. + * + * @param accounts - the accounts we are checking to see if notifications are enabled/disabled + * @returns props for settings page + */ +export function useAccountSettingsProps(accounts: string[]) { + const accountsBeingUpdated = useSelector( + getIsUpdatingMetamaskNotificationsAccount, + ); + const fetchAccountSettings = useRefetchAccountSettings(); + const [data, setData] = useState({}); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); + + // Memoize the accounts array to avoid unnecessary re-fetching + const jsonAccounts = useMemo(() => JSON.stringify(accounts), [accounts]); + + const update = useCallback(async (addresses: string[]) => { + try { + setLoading(true); + setError(null); + const res = await fetchAccountSettings(addresses); + setData(res); + } catch { + setError('Failed to get account settings'); + } finally { + setLoading(false); + } + }, []); + + // Effect - async get if accounts are enabled/disabled + useEffect(() => { + try { + const memoAccounts: string[] = JSON.parse(jsonAccounts); + update(memoAccounts); + } catch { + setError('Failed to get account settings'); + } finally { + setLoading(false); + } + }, [jsonAccounts, fetchAccountSettings]); + + return { + data, + initialLoading: loading, + error, + accountsBeingUpdated, + update, + }; +} diff --git a/ui/pages/notifications-settings/notifications-settings-per-account.tsx b/ui/pages/notifications-settings/notifications-settings-per-account.tsx index a591a4f81ea9..6ebfcdad5f0f 100644 --- a/ui/pages/notifications-settings/notifications-settings-per-account.tsx +++ b/ui/pages/notifications-settings/notifications-settings-per-account.tsx @@ -1,89 +1,95 @@ -import React, { useEffect, useState, useCallback, useContext } from 'react'; +import React, { useState, useCallback, useContext } from 'react'; import { MetaMetricsContext } from '../../contexts/metametrics'; import { MetaMetricsEventCategory, MetaMetricsEventName, } from '../../../shared/constants/metametrics'; -import { useMetamaskNotificationsContext } from '../../contexts/metamask-notifications/metamask-notifications'; -import { - useSwitchAccountNotifications, - useSwitchAccountNotificationsChange, - type UseSwitchAccountNotificationsData, -} from '../../hooks/metamask-notifications/useSwitchNotifications'; +import { useSwitchAccountNotificationsChange } from '../../hooks/metamask-notifications/useSwitchNotifications'; import { NotificationsSettingsBox, NotificationsSettingsAccount, } from '../../components/multichain'; +import { useListNotifications } from '../../hooks/metamask-notifications/useNotifications'; type NotificationsSettingsPerAccountProps = { address: string; name: string; - disabled: boolean; - loading: boolean; + + isEnabled: boolean; + isLoading?: boolean; + disabledSwitch?: boolean; + refetchAccountSettings: () => Promise; }; +function useUpdateAccountSetting( + address: string, + refetchAccountSettings: () => Promise, +) { + const { onChange: switchAccountNotifications, error } = + useSwitchAccountNotificationsChange(); + const { listNotifications: refetch } = useListNotifications(); + + // Local states + const [loading, setLoading] = useState(false); + + const toggleAccount = useCallback( + async (state: boolean) => { + setLoading(true); + try { + await switchAccountNotifications([address], state); + await refetchAccountSettings(); + refetch(); + } catch { + // Do nothing (we don't need to propagate this) + } + setLoading(false); + }, + [address, refetch, refetchAccountSettings, switchAccountNotifications], + ); + + return { toggleAccount, loading, error }; +} + export const NotificationsSettingsPerAccount = ({ address, name, - disabled, - loading, + isEnabled, + isLoading, + disabledSwitch, + refetchAccountSettings, }: NotificationsSettingsPerAccountProps) => { - const { listNotifications } = useMetamaskNotificationsContext(); const trackEvent = useContext(MetaMetricsContext); - // Hooks const { - onChange: onChangeAccountNotifications, - error: errorAccountNotificationsChange, - } = useSwitchAccountNotificationsChange(); - const { switchAccountNotifications, error: errorSwitchAccountNotifications } = - useSwitchAccountNotifications(); - - const [data, setData] = useState< - UseSwitchAccountNotificationsData | undefined - >(undefined); - const [isLoading, setIsLoading] = useState(false); - - useEffect(() => { - const fetchData = async () => { - const fetchedData = await switchAccountNotifications([address]); - setData(fetchedData || {}); - }; - fetchData(); - }, [address, switchAccountNotifications]); - - useEffect(() => { - setIsLoading(loading); - }, [loading]); + toggleAccount, + loading: isUpdatingAccount, + error: accountError, + } = useUpdateAccountSetting(address, refetchAccountSettings); - const error = - errorAccountNotificationsChange || errorSwitchAccountNotifications; + const loading = isLoading || isUpdatingAccount; + const error = accountError; const handleToggleAccountNotifications = useCallback(async () => { - const originalValue = data?.[address]; - await onChangeAccountNotifications([address], !originalValue); trackEvent({ category: MetaMetricsEventCategory.NotificationSettings, - event: originalValue + event: isEnabled ? MetaMetricsEventName.DisablingAccountNotifications : MetaMetricsEventName.EnablingAccountNotifications, properties: { address, }, }); - const fetchedData = await switchAccountNotifications([address]); - setData(fetchedData || {}); - listNotifications(); - }, [address, data, onChangeAccountNotifications]); + await toggleAccount(!isEnabled); + }, [address, isEnabled, toggleAccount, trackEvent]); return ( <> diff --git a/ui/pages/notifications-settings/notifications-settings.tsx b/ui/pages/notifications-settings/notifications-settings.tsx index c4e71e97f273..7261daeed02f 100644 --- a/ui/pages/notifications-settings/notifications-settings.tsx +++ b/ui/pages/notifications-settings/notifications-settings.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import React, { useMemo, useState } from 'react'; import { useSelector } from 'react-redux'; import { useHistory } from 'react-router-dom'; import type { InternalAccount } from '@metamask/keyring-api'; @@ -26,9 +26,9 @@ import { Content, Header } from '../../components/multichain/pages/page'; import { selectIsMetamaskNotificationsEnabled, getIsUpdatingMetamaskNotifications, - getIsUpdatingMetamaskNotificationsAccount, } from '../../selectors/metamask-notifications/metamask-notifications'; import { getInternalAccounts } from '../../selectors'; +import { useAccountSettingsProps } from '../../hooks/metamask-notifications/useSwitchNotifications'; import { NotificationsSettingsAllowNotifications } from './notifications-settings-allow-notifications'; import { NotificationsSettingsTypes } from './notifications-settings-types'; import { NotificationsSettingsPerAccount } from './notifications-settings-per-account'; @@ -56,36 +56,23 @@ export default function NotificationsSettings() { const isUpdatingMetamaskNotifications = useSelector( getIsUpdatingMetamaskNotifications, ); - const isUpdatingMetamaskNotificationsAccount = useSelector( - getIsUpdatingMetamaskNotificationsAccount, - ); const accounts: AccountType[] = useSelector(getInternalAccounts); // States const [loadingAllowNotifications, setLoadingAllowNotifications] = useState(isUpdatingMetamaskNotifications); - const [updatingAccountList, setUpdatingAccountList] = useState([]); - const [updatingAccount, setUpdatingAccount] = useState(false); - - useEffect(() => { - if (updatingAccountList.length > 0) { - setUpdatingAccount(true); - } else { - setUpdatingAccount(false); - } - }, [updatingAccountList]); - useEffect(() => { - if (isUpdatingMetamaskNotifications) { - setLoadingAllowNotifications(isUpdatingMetamaskNotifications); - } - }, [isUpdatingMetamaskNotifications]); + const accountAddresses = useMemo( + () => accounts.map((a) => a.address), + [accounts], + ); - useEffect(() => { - if (isUpdatingMetamaskNotificationsAccount) { - setUpdatingAccountList(isUpdatingMetamaskNotificationsAccount); - } - }, [isUpdatingMetamaskNotificationsAccount]); + // Account Settings + const accountSettingsProps = useAccountSettingsProps(accountAddresses); + const updatingAccounts = accountSettingsProps.accountsBeingUpdated.length > 0; + const refetchAccountSettings = async () => { + await accountSettingsProps.update(accountAddresses); + }; return ( @@ -108,7 +95,7 @@ export default function NotificationsSettings() { loading={loadingAllowNotifications} setLoading={setLoadingAllowNotifications} data-testid="notifications-settings-allow-notifications" - disabled={updatingAccount} + disabled={updatingAccounts} /> {/* Notifications settings per types */} {/* Notifications settings per account */} @@ -160,8 +147,18 @@ export default function NotificationsSettings() { key={account.id} address={account.address} name={account.metadata.name} - disabled={updatingAccountList.length > 0} - loading={updatingAccountList.includes(account.address)} + disabledSwitch={ + accountSettingsProps.initialLoading || updatingAccounts + } + isLoading={accountSettingsProps.accountsBeingUpdated.includes( + account.address, + )} + isEnabled={ + accountSettingsProps.data?.[ + account.address.toLowerCase() + ] ?? false + } + refetchAccountSettings={refetchAccountSettings} /> ))}