Skip to content

Commit

Permalink
perf: no multichain list calculations are made when feature flag is o…
Browse files Browse the repository at this point in the history
…ff (#12766)

## **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.
  • Loading branch information
vinnyhoward authored Dec 18, 2024
1 parent 25375aa commit 82fb18c
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 5 deletions.
2 changes: 1 addition & 1 deletion app/components/UI/Tokens/__snapshots__/index.test.tsx.snap
Original file line number Diff line number Diff line change
@@ -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`] = `
<View
style={
{
Expand Down
25 changes: 24 additions & 1 deletion app/components/UI/Tokens/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { createTokensBottomSheetNavDetails } from './TokensBottomSheet';
import useStakingEligibility from '../Stake/hooks/useStakingEligibility';
// eslint-disable-next-line import/no-namespace
import * as networks from '../../../util/networks';
// eslint-disable-next-line import/no-namespace
import * as multichain from '../../../selectors/multichain';

jest.mock('../../../core/NotificationManager', () => ({
showSimpleNotification: jest.fn(() => Promise.resolve()),
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions app/components/UI/Tokens/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ const Tokens: React.FC<TokensI> = ({ tokens }) => {
),
),
];
const selectedAccountTokensChains = useSelector(
selectAccountTokensAcrossChains,

const selectedAccountTokensChains = useSelector((state: RootState) =>
isPortfolioViewEnabled() ? selectAccountTokensAcrossChains(state) : {},
);

const actionSheet = useRef<typeof ActionSheet>();
Expand Down Expand Up @@ -153,7 +154,6 @@ const Tokens: React.FC<TokensI> = ({ 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
Expand Down

0 comments on commit 82fb18c

Please sign in to comment.