perf: Remove dead "What's New" modal code and fix Home re-render cascade#39314
perf: Remove dead "What's New" modal code and fix Home re-render cascade#39314
Conversation
getSortedAnnouncementsToShow selector memoization to prevent cascade re-renders
|
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. |
Builds ready [61782ee]
UI Startup Metrics (1292 ± 116 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
61782ee to
394c13f
Compare
394c13f to
c298f9b
Compare
getSortedAnnouncementsToShow selector memoization to prevent cascade re-rendersgetSortedAnnouncementsToShow selector memoization to prevent cascade re-renders
This comment was marked as resolved.
This comment was marked as resolved.
getSortedAnnouncementsToShow selector memoization to prevent cascade re-rendersgetSortedAnnouncementsToShow broken selector memoization causes global cascading re-renders
This comment was marked as resolved.
This comment was marked as resolved.
getSortedAnnouncementsToShow broken selector memoization causes global cascading re-rendersgetSortedAnnouncementsToShow selector memoization to prevent cascade re-renders
|
@ameliejyc yes the selector it's not being used, neither the |
|
@ameliejyc @zone-live I did a first pass on removing dead code and it's a fair amount: 9 files deleted or with lines removed. Are we good with proceeding with this or would the team maybe prefer to handle this in a separate ticket? If this looks ok I can push as a commit here and update the pr title and description. diff --git a/ui/components/app/whats-new-modal/index.ts b/ui/components/app/whats-new-modal/index.ts
deleted file mode 100644
index 2e981b5a09..0000000000
--- a/ui/components/app/whats-new-modal/index.ts
+++ /dev/null
@@ -1 +0,0 @@
-export { default } from './whats-new-modal';
diff --git a/ui/components/app/whats-new-modal/notifications.ts b/ui/components/app/whats-new-modal/notifications.ts
deleted file mode 100644
index 8c469783c5..0000000000
--- a/ui/components/app/whats-new-modal/notifications.ts
+++ /dev/null
@@ -1,10 +0,0 @@
-import {
- TranslatedUINotifications,
- TranslationFunction,
-} from '../../../../shared/notifications';
-
-export const getTranslatedUINotifications = (
- _t: TranslationFunction,
-): TranslatedUINotifications => {
- return {};
-};
diff --git a/ui/components/app/whats-new-modal/whats-new-modal.tsx b/ui/components/app/whats-new-modal/whats-new-modal.tsx
deleted file mode 100644
index 5669c57652..0000000000
--- a/ui/components/app/whats-new-modal/whats-new-modal.tsx
+++ /dev/null
@@ -1,139 +0,0 @@
-import React, { useContext } from 'react';
-import { useSelector } from 'react-redux';
-
-import {
- MetaMetricsEventCategory,
- MetaMetricsEventName,
-} from '../../../../shared/constants/metametrics';
-import {
- ModalBodyProps,
- ModalComponent,
- ModalFooterProps,
- ModalHeaderProps,
-} from '../../../../shared/notifications';
-import { I18nContext } from '../../../contexts/i18n';
-import { MetaMetricsContext } from '../../../contexts/metametrics';
-import {
- Display,
- FlexDirection,
-} from '../../../helpers/constants/design-system';
-import { getSortedAnnouncementsToShow } from '../../../selectors';
-import { updateViewedNotifications } from '../../../store/actions';
-import { Modal, ModalContent, ModalOverlay } from '../../component-library';
-import { getTranslatedUINotifications } from './notifications';
-
-type WhatsNewModalProps = {
- onClose: () => void;
-};
-
-type NotificationType = {
- id: number;
- date?: string | null;
- title: string;
- description?: string | string[];
- image?: {
- src: string;
- width?: string;
- height?: string;
- };
- modal?: {
- header?: ModalComponent<ModalHeaderProps>;
- body?: ModalComponent<ModalBodyProps>;
- footer?: ModalComponent<ModalFooterProps>;
- };
-};
-
-type RenderNotificationProps = {
- notification: NotificationType;
- onClose: () => void;
- onNotificationViewed: (id: number) => Promise<void>;
-};
-
-const renderNotification = ({
- notification,
- onClose,
- onNotificationViewed,
-}: RenderNotificationProps) => {
- const { id, title, image, modal } = notification;
-
- const handleNotificationClose = async () => {
- await onNotificationViewed(id);
- onClose();
- };
-
- return (
- <ModalContent
- modalDialogProps={{
- display: Display.Flex,
- flexDirection: FlexDirection.Column,
- padding: 4,
- }}
- >
- {modal?.header && (
- <modal.header.component onClose={onClose} image={image} />
- )}
- {modal?.body && <modal.body.component title={title} />}
- {modal?.footer && (
- <modal.footer.component
- onAction={() => {
- // No action needed for whats-new notifications
- // This is required by the ModalFooterProps type
- console.log('No action needed for now');
- }}
- onCancel={handleNotificationClose}
- />
- )}
- </ModalContent>
- );
-};
-
-// TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
-// eslint-disable-next-line @typescript-eslint/naming-convention
-export default function WhatsNewModal({ onClose }: WhatsNewModalProps) {
- const t = useContext(I18nContext);
- const trackEvent = useContext(MetaMetricsContext);
-
- const notifications = useSelector(getSortedAnnouncementsToShow);
-
- const handleNotificationViewed = async (id: number) => {
- await updateViewedNotifications({ [id]: true });
- };
-
- const handleModalClose = async () => {
- await Promise.all(
- notifications.map(({ id }) => handleNotificationViewed(id)),
- );
- trackEvent({
- category: MetaMetricsEventCategory.Home,
- event: MetaMetricsEventName.WhatsNewViewed,
- });
- onClose();
- };
-
- return (
- <>
- <Modal
- // TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31879
- // eslint-disable-next-line @typescript-eslint/no-misused-promises
- onClose={handleModalClose}
- data-testid="whats-new-modal"
- isOpen={notifications.length > 0}
- isClosedOnOutsideClick
- isClosedOnEscapeKey
- autoFocus={false}
- >
- <ModalOverlay />
-
- {notifications.map(({ id }) => {
- const notification = getTranslatedUINotifications(t)[id];
-
- return renderNotification({
- notification,
- onClose,
- onNotificationViewed: handleNotificationViewed,
- });
- })}
- </Modal>
- </>
- );
-}
diff --git a/ui/ducks/app/app.ts b/ui/ducks/app/app.ts
index e874fa7c84..a4a8483cbf 100644
--- a/ui/ducks/app/app.ts
+++ b/ui/ducks/app/app.ts
@@ -90,7 +90,6 @@ type AppState = {
// TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31973
// eslint-disable-next-line @typescript-eslint/no-explicit-any
currentWindowTab: Record<string, any>; // tabs.tab https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/Tab
- showWhatsNewPopup: boolean;
showTermsOfUsePopup: boolean;
singleExceptions: {
testKey: string | null;
@@ -209,7 +208,6 @@ const initialState: AppState = {
requestAccountTabs: {},
openMetaMaskTabs: {},
currentWindowTab: {},
- showWhatsNewPopup: true,
showTermsOfUsePopup: true,
singleExceptions: {
testKey: null,
@@ -651,12 +649,6 @@ export default function reduceApp(
openMetaMaskTabs: action.payload,
};
- case actionConstants.HIDE_WHATS_NEW_POPUP:
- return {
- ...appState,
- showWhatsNewPopup: false,
- };
-
case actionConstants.CAPTURE_SINGLE_EXCEPTION:
return {
...appState,
@@ -806,12 +798,6 @@ export default function reduceApp(
}
// Action Creators
-export function hideWhatsNewPopup(): Action {
- return {
- type: actionConstants.HIDE_WHATS_NEW_POPUP,
- };
-}
-
export function openBasicFunctionalityModal(): Action {
return {
type: actionConstants.SHOW_BASIC_FUNCTIONALITY_MODAL_OPEN,
diff --git a/ui/pages/home/home.component.js b/ui/pages/home/home.component.js
index 57b71b4476..429138b024 100644
--- a/ui/pages/home/home.component.js
+++ b/ui/pages/home/home.component.js
@@ -12,7 +12,6 @@ import {
} from '../../../shared/constants/metametrics';
import TermsOfUsePopup from '../../components/app/terms-of-use-popup';
import RecoveryPhraseReminder from '../../components/app/recovery-phrase-reminder';
-import WhatsNewModal from '../../components/app/whats-new-modal';
import { FirstTimeFlowType } from '../../../shared/constants/onboarding';
import HomeNotification from '../../components/app/home-notification';
import MultipleNotifications from '../../components/app/multiple-notifications';
@@ -106,9 +105,6 @@ export default class Home extends PureComponent {
showTermsOfUsePopup: PropTypes.bool.isRequired,
firstTimeFlowType: PropTypes.string,
completedOnboarding: PropTypes.bool,
- showWhatsNewPopup: PropTypes.bool.isRequired,
- hideWhatsNewPopup: PropTypes.func.isRequired,
- announcementsToShow: PropTypes.bool.isRequired,
onboardedInThisUISession: PropTypes.bool,
showMultiRpcModal: PropTypes.bool.isRequired,
showUpdateModal: PropTypes.bool.isRequired,
@@ -772,11 +768,8 @@ export default class Home extends PureComponent {
isPopup,
showRecoveryPhraseReminder,
showTermsOfUsePopup,
- showWhatsNewPopup,
- hideWhatsNewPopup,
completedOnboarding,
onboardedInThisUISession,
- announcementsToShow,
firstTimeFlowType,
newNetworkAddedConfigurationId,
showMultiRpcModal,
@@ -803,23 +796,11 @@ export default class Home extends PureComponent {
firstTimeFlowType === FirstTimeFlowType.import) &&
!newNetworkAddedConfigurationId;
- const showWhatsNew =
- canSeeModals &&
- announcementsToShow &&
- showWhatsNewPopup &&
- !process.env.IN_TEST;
-
const showMultiRpcEditModal =
- canSeeModals &&
- showMultiRpcModal &&
- !showWhatsNew &&
- !process.env.IN_TEST;
+ canSeeModals && showMultiRpcModal && !process.env.IN_TEST;
const displayUpdateModal =
- canSeeModals &&
- showUpdateModal &&
- !showWhatsNew &&
- !showMultiRpcEditModal;
+ canSeeModals && showUpdateModal && !showMultiRpcEditModal;
const showTermsOfUse =
completedOnboarding &&
@@ -828,16 +809,13 @@ export default class Home extends PureComponent {
!isSocialLoginFlow;
const showRecoveryPhrase =
- !showWhatsNew &&
- showRecoveryPhraseReminder &&
- !isPrimarySeedPhraseBackedUp;
+ showRecoveryPhraseReminder && !isPrimarySeedPhraseBackedUp;
const showRewardsModal =
rewardsEnabled &&
rewardsOnboardingEnabled &&
canSeeModals &&
!showTermsOfUse &&
- !showWhatsNew &&
!showMultiRpcEditModal &&
!displayUpdateModal &&
!isSeedlessPasswordOutdated &&
@@ -886,7 +864,6 @@ export default class Home extends PureComponent {
{isSeedlessPasswordOutdated && <PasswordOutdatedModal />}
{showMultiRpcEditModal && <MultiRpcEditModal />}
{displayUpdateModal && <UpdateModal />}
- {showWhatsNew ? <WhatsNewModal onClose={hideWhatsNewPopup} /> : null}
{showRecoveryPhrase ? (
<RecoveryPhraseReminder
onConfirm={this.onRecoveryPhraseReminderClose}
diff --git a/ui/pages/home/home.container.js b/ui/pages/home/home.container.js
index f613e54938..3bfb6dab9b 100644
--- a/ui/pages/home/home.container.js
+++ b/ui/pages/home/home.container.js
@@ -10,8 +10,6 @@ import {
getOriginOfCurrentTab,
getTotalUnapprovedCount,
getWeb3ShimUsageStateForOrigin,
- getShowWhatsNewPopup,
- getSortedAnnouncementsToShow,
getShowRecoveryPhraseReminder,
getShowTermsOfUse,
getShowOutdatedBrowserWarning,
@@ -51,10 +49,7 @@ import {
lookupSelectedNetworks,
setPendingShieldCohort,
} from '../../store/actions';
-import {
- hideWhatsNewPopup,
- openBasicFunctionalityModal,
-} from '../../ducks/app/app';
+import { openBasicFunctionalityModal } from '../../ducks/app/app';
import {
getIsPrimarySeedPhraseBackedUp,
getIsSeedlessPasswordOutdated,
@@ -128,12 +123,6 @@ const mapStateToProps = (state) => {
///: END:ONLY_INCLUDE_IF
]);
- const TEMPORARY_DISABLE_WHATS_NEW = true;
-
- const showWhatsNewPopup = TEMPORARY_DISABLE_WHATS_NEW
- ? false
- : getShowWhatsNewPopup(state);
-
const shouldShowSeedPhraseReminder =
selectedAccount && getShouldShowSeedPhraseReminder(state, selectedAccount);
@@ -157,8 +146,6 @@ const mapStateToProps = (state) => {
originOfCurrentTab,
shouldShowWeb3ShimUsageNotification,
infuraBlocked: getInfuraBlocked(state),
- announcementsToShow: getSortedAnnouncementsToShow(state).length > 0,
- showWhatsNewPopup,
showRecoveryPhraseReminder: getShowRecoveryPhraseReminder(state),
showTermsOfUsePopup: getShowTermsOfUse(state),
showOutdatedBrowserWarning:
@@ -201,7 +188,6 @@ const mapDispatchToProps = (dispatch) => {
setWeb3ShimUsageAlertDismissed(origin),
disableWeb3ShimUsageAlert: () =>
setAlertEnabledness(AlertTypes.web3ShimUsage, false),
- hideWhatsNewPopup: () => dispatch(hideWhatsNewPopup()),
setRecoveryPhraseReminderHasBeenShown: () =>
dispatch(setRecoveryPhraseReminderHasBeenShown()),
setRecoveryPhraseReminderLastShown: (lastShown) =>
diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js
index fe31c68f6a..2404b5532e 100644
--- a/ui/selectors/selectors.js
+++ b/ui/selectors/selectors.js
@@ -216,10 +216,6 @@ export function getNextSuggestedNonce(state) {
return Number(state.appState.nextNonce);
}
-export function getShowWhatsNewPopup(state) {
- return state.appState.showWhatsNewPopup;
-}
-
export function getShowPermittedNetworkToastOpen(state) {
return state.appState.showPermittedNetworkToastOpen;
}
@@ -2429,48 +2425,6 @@ export const getSnapInsights = createDeepEqualSelector(
(insights, id) => insights?.[id],
);
-/**
- * Get an object of announcement IDs and if they are allowed or not.
- *
- * @returns {object}
- */
-const getAllowedAnnouncementIds = () => EMPTY_OBJECT;
-
-/**
- * Get the announcements object from state
- *
- * @param {object} state - the redux state object
- * @returns {object} The announcements object
- */
-const getAnnouncementsObject = (state) => state.metamask.announcements;
-
-/**
- * @typedef {object} Announcement
- * @property {number} id - A unique identifier for the announcement
- * @property {string} date - A date in YYYY-MM-DD format, identifying when the notification was first committed
- */
-
-/**
- * Announcements are managed by the announcement controller and referenced by
- * `state.metamask.announcements`. This function returns a list of announcements
- * the can be shown to the user. This list includes all announcements that do not
- * have a truthy `isShown` property.
- *
- * The returned announcements are sorted by date.
- *
- * @param {object} state - the redux state object
- * @returns {Announcement[]} An array of announcements that can be shown to the user
- */
-
-export const getSortedAnnouncementsToShow = createSelector(
- getAnnouncementsObject,
- getAllowedAnnouncementIds,
- (announcements, allowedIds) =>
- Object.values(announcements)
- .filter((a) => !a.isShown && allowedIds[a.id])
- .sort((a, b) => new Date(b.date) - new Date(a.date)),
-);
-
/**
* @param state
* @returns {{networkId: string}[]}
diff --git a/ui/selectors/selectors.test.js b/ui/selectors/selectors.test.js
index 2af103857d..05bb1fcf51 100644
--- a/ui/selectors/selectors.test.js
+++ b/ui/selectors/selectors.test.js
@@ -4170,111 +4170,3 @@ describe('getPermissionsForActiveTab', () => {
expect(result).toStrictEqual([]);
});
});
-
-describe('getSortedAnnouncementsToShow', () => {
- it('memoizes and returns the same reference when state unchanged', () => {
- const state = {
- metamask: {
- announcements: {
- 1: { id: 1, date: '2021-01-01', isShown: false },
- 2: { id: 2, date: '2021-01-02', isShown: true },
- 3: { id: 3, date: '2021-01-03', isShown: false },
- },
- },
- };
-
- const result1 = selectors.getSortedAnnouncementsToShow(state);
- const result2 = selectors.getSortedAnnouncementsToShow(state);
-
- // Should return the same reference when state unchanged (memoization)
- expect(result1).toBe(result2);
- });
-
- it('returns different reference when announcements object changes', () => {
- const state1 = {
- metamask: {
- announcements: {
- 1: { id: 1, date: '2021-01-01', isShown: false },
- },
- },
- };
-
- const result1 = selectors.getSortedAnnouncementsToShow(state1);
-
- const state2 = {
- metamask: {
- announcements: {
- 1: { id: 1, date: '2021-01-01', isShown: false },
- 2: { id: 2, date: '2021-01-02', isShown: false },
- },
- },
- };
-
- const result2 = selectors.getSortedAnnouncementsToShow(state2);
-
- // Should return different reference when announcements change
- expect(result1).not.toBe(result2);
- });
-
- it('filters out shown announcements and sorts by date descending', () => {
- const state = {
- metamask: {
- announcements: {
- 1: { id: 1, date: '2021-01-01', isShown: false },
- 2: { id: 2, date: '2021-01-03', isShown: true },
- 3: { id: 3, date: '2021-01-02', isShown: false },
- },
- },
- };
-
- const result = selectors.getSortedAnnouncementsToShow(state);
-
- // Currently getAllowedAnnouncementIds returns {}, so all announcements are filtered out
- expect(result).toStrictEqual([]);
- });
-
- it('returns empty array when no announcements exist', () => {
- const state = {
- metamask: {
- announcements: {},
- },
- };
-
- const result = selectors.getSortedAnnouncementsToShow(state);
- expect(result).toStrictEqual([]);
- });
-
- it('returns empty array when all announcements are shown', () => {
- const state = {
- metamask: {
- announcements: {
- 1: { id: 1, date: '2021-01-01', isShown: true },
- 2: { id: 2, date: '2021-01-02', isShown: true },
- },
- },
- };
-
- const result = selectors.getSortedAnnouncementsToShow(state);
- expect(result).toStrictEqual([]);
- });
-
- it('maintains memoization with same announcements object reference', () => {
- const announcements = {
- 1: { id: 1, date: '2021-01-01', isShown: false },
- };
-
- const state1 = {
- metamask: { announcements },
- };
-
- const state2 = {
- metamask: { announcements },
- };
-
- const result1 = selectors.getSortedAnnouncementsToShow(state1);
- const result2 = selectors.getSortedAnnouncementsToShow(state2);
-
- // Should be memoized since announcements object is the same reference
- expect(result1).toBe(result2);
- });
-});
diff --git a/ui/store/actionConstants.ts b/ui/store/actionConstants.ts
index 176cf780a0..7775f40e36 100644
--- a/ui/store/actionConstants.ts
+++ b/ui/store/actionConstants.ts
@@ -149,7 +149,6 @@ export const SET_REQUEST_ACCOUNT_TABS = 'SET_REQUEST_ACCOUNT_TABS';
export const SET_OPEN_METAMASK_TAB_IDS = 'SET_OPEN_METAMASK_TAB_IDS';
// Home Screen
-export const HIDE_WHATS_NEW_POPUP = 'HIDE_WHATS_NEW_POPUP';
export const TOGGLE_GAS_LOADING_ANIMATION = 'TOGGLE_GAS_LOADING_ANIMATION';
// Smart Transactions
|
✨ Files requiring CODEOWNER review ✨👨🔧 @MetaMask/core-extension-ux (3 files, +4 -43)
🕵️ @MetaMask/extension-privacy-reviewers (1 files, +0 -1)
🧪 @MetaMask/qa (1 files, +0 -1)
|
getSortedAnnouncementsToShow selector memoization to prevent cascade re-renders
Builds ready [431ea92]
UI Startup Metrics (1265 ± 119 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [04208e8]
UI Startup Metrics (1283 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ameliejyc
left a comment
There was a problem hiding this comment.
Thanks for doing this!
Description
This PR removes the dead "What's New" modal feature entirely, which also resolves the performance issue that was causing unnecessary re-renders of the Home component.
What is the reason for the change?
Investigation revealed that the
getSortedAnnouncementsToShowselector:getAllowedAnnouncementIds()returns{}UI_NOTIFICATIONSinshared/notifications/index.tsis emptyTEMPORARY_DISABLE_WHATS_NEW = truein home.container.jsRather than memoizing dead code, this PR removes it entirely.
What is the improvement/solution?
Removed all UI-side code related to the "What's New" modal:
ui/components/app/whats-new-modal/directorygetSortedAnnouncementsToShowandgetShowWhatsNewPopupselectorsshowWhatsNewPopupfrom Redux state and thehideWhatsNewPopupactionPerformance improvement: The previous
getSortedAnnouncementsToShowselector created new array references on every call, breaking React's memoization and causing the Home component tree (~50-60 children) to re-render unnecessarily. Removing this code path eliminates this re-render cascade entirely.Changelog
CHANGELOG entry: Removed unused "What's New" modal feature, improving Home page performance
Related issues
Manual testing steps
Screenshots/Recordings
N/A - Removes unused feature
Pre-merge author checklist
Pre-merge reviewer checklist
Technical Details
Files Removed
ui/components/app/whats-new-modal/whats-new-modal.tsxui/components/app/whats-new-modal/notifications.tsui/components/app/whats-new-modal/index.tsCode Removed
getSortedAnnouncementsToShowselector and helpers (getAnnouncementsObject,getAllowedAnnouncementIds)getShowWhatsNewPopupselectorshowWhatsNewPopupstate property andHIDE_WHATS_NEW_POPUPactionhideWhatsNewPopupaction creatorshowWhatsNewPopup,hideWhatsNewPopup,announcementsToShow)getSortedAnnouncementsToShowWhy This Is Better Than Memoization
The original issue asked to memoize
getSortedAnnouncementsToShow. However, investigation showed:getAllowedAnnouncementIds()returns{}(empty object)allowedIds[a.id]is alwaysundefined(falsy)[]regardless of announcements in stateTEMPORARY_DISABLE_WHATS_NEW = trueMemoizing code that always returns
[]is wasteful. Removing it:What's Preserved
The
AnnouncementControllerinfrastructure in the background scripts is preserved, as it's deeply integrated and may be needed for future announcements.Note
Removes the dormant announcements UI and simplifies Home modal gating to prevent unnecessary re-renders.
ui/components/app/whats-new-modal/*getSortedAnnouncementsToShowandgetShowWhatsNewPopupshowWhatsNewPopupandHIDE_WHATS_NEW_POPUP(andhideWhatsNewPopupcreator)home.component.jsto remove WhatsNew logic/props and simplifyshowMultiRpcEditModal,displayUpdateModal, and recovery/rewards gatinghome.container.jsto stop selecting announcements/WhatsNew, and adjusts mapState/mapDispatchWritten by Cursor Bugbot for commit 04208e8. This will update automatically on new commits. Configure here.