From 82fb18c3c3de0ae2c7c2b3d7a113f4ee5e3ca262 Mon Sep 17 00:00:00 2001 From: Vince Howard Date: Wed, 18 Dec 2024 12:23:55 -0700 Subject: [PATCH] perf: no multichain list calculations are made when feature flag is off (#12766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** ### Problem The `selectAccountTokensAcrossChains` selector is being executed even when the `PORTFOLIO_VIEW` feature flag is disabled. This causes unnecessary state computations and potential performance overhead since the selector's results aren't used when the feature is off. ### Solution Modified the selector usage to conditionally return an empty object when the feature flag is disabled: ### Testing Added test cases within the Portfolio View test suite to verify: - Selector is called when the feature flag is enabled - Selector is not called when the feature flag is disabled This change optimizes performance by preventing unnecessary selector computations ## **Related issues** Fixes: ## **Manual testing steps** 1. Run `yarn watch` with no feature flag. The app should be the exactly the same 2. Run `PORTFOLIO_VIEW=true yarn watch`. The app should be have the multi chain list 3. You can log the `selectedAccountTokensChains` selector with the feature flag on and off to confirm it works ## **Screenshots/Recordings** NA ### **Before** NA ### **After** NA ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. --- .../Tokens/__snapshots__/index.test.tsx.snap | 2 +- app/components/UI/Tokens/index.test.tsx | 25 ++++++++++++++++++- app/components/UI/Tokens/index.tsx | 6 ++--- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/components/UI/Tokens/__snapshots__/index.test.tsx.snap b/app/components/UI/Tokens/__snapshots__/index.test.tsx.snap index 3d53ad137db..4b1a0b3e3d0 100644 --- a/app/components/UI/Tokens/__snapshots__/index.test.tsx.snap +++ b/app/components/UI/Tokens/__snapshots__/index.test.tsx.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Tokens Portfolio View should match the snapshot when portfolio view is enabled 1`] = ` +exports[`Tokens Portfolio View should match the snapshot when portfolio view is enabled 1`] = ` ({ showSimpleNotification: jest.fn(() => Promise.resolve()), @@ -597,15 +599,36 @@ describe('Tokens', () => { }); describe('Portfolio View', () => { + let selectAccountTokensAcrossChainsSpy: jest.SpyInstance; + beforeEach(() => { + selectAccountTokensAcrossChainsSpy = jest.spyOn( + multichain, + 'selectAccountTokensAcrossChains', + ); jest.spyOn(networks, 'isPortfolioViewEnabled').mockReturnValue(true); }); - it('should match the snapshot when portfolio view is enabled ', () => { + afterEach(() => { + selectAccountTokensAcrossChainsSpy.mockRestore(); + }); + + it('should match the snapshot when portfolio view is enabled', () => { const { toJSON } = renderComponent(initialState); expect(toJSON()).toMatchSnapshot(); }); + it('should call selectAccountTokensAcrossChains when enabled', () => { + renderComponent(initialState); + expect(selectAccountTokensAcrossChainsSpy).toHaveBeenCalled(); + }); + + it('should not call selectAccountTokensAcrossChains when disabled', () => { + jest.spyOn(networks, 'isPortfolioViewEnabled').mockReturnValue(false); + renderComponent(initialState); + expect(selectAccountTokensAcrossChainsSpy).not.toHaveBeenCalled(); + }); + it('should handle network filtering correctly', () => { const multiNetworkState = { ...initialState, diff --git a/app/components/UI/Tokens/index.tsx b/app/components/UI/Tokens/index.tsx index 794a6ea0585..3f7bf864395 100644 --- a/app/components/UI/Tokens/index.tsx +++ b/app/components/UI/Tokens/index.tsx @@ -122,8 +122,9 @@ const Tokens: React.FC = ({ tokens }) => { ), ), ]; - const selectedAccountTokensChains = useSelector( - selectAccountTokensAcrossChains, + + const selectedAccountTokensChains = useSelector((state: RootState) => + isPortfolioViewEnabled() ? selectAccountTokensAcrossChains(state) : {}, ); const actionSheet = useRef(); @@ -153,7 +154,6 @@ const Tokens: React.FC = ({ tokens }) => { const allTokens = Object.values( selectedAccountTokensChains, ).flat() as TokenI[]; - /* If hideZeroBalanceTokens is ON and user is on "all Networks" we respect the setting and filter native and ERC20 tokens when zero If user is on "current Network" we want to show native tokens, even with zero balance