Skip to content

Commit

Permalink
chore: Remove notifications logic from wallet view (#12276) (#12291)
Browse files Browse the repository at this point in the history
Removing notifications logic from wallet view because of a performance
degredation. Investigations are still in progress.

Cherrypick for
7b76e08
(#12276)

Fixes:

1. Go to this page... 2.
3.

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] 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).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] 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.

- [ ] 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.

---------

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] 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).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] 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**

- [ ] 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.

Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
Co-authored-by: Cal-L <cleun007@gmail.com>
Co-authored-by: Cal Leung <cal.leung@consensys.net>
  • Loading branch information
4 people authored Nov 14, 2024
1 parent 478107e commit 750701e
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 87 deletions.
40 changes: 1 addition & 39 deletions app/components/Views/Wallet/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import React from 'react';
import Wallet from './';
import { renderScreen } from '../../../util/test/renderWithProvider';
import { act, screen } from '@testing-library/react-native';
import { screen } from '@testing-library/react-native';
import ScrollableTabView from 'react-native-scrollable-tab-view';
import Routes from '../../../constants/navigation/Routes';
import { backgroundState } from '../../../util/test/initial-root-state';
import { createMockAccountsControllerState } from '../../../util/test/accountsControllerTestUtils';
import { WalletViewSelectorsIDs } from '../../../../e2e/selectors/wallet/WalletView.selectors';
import { CommonSelectorsIDs } from '../../../../e2e/selectors/Common.selectors';
import { useAccountSyncing } from '../../../util/notifications/hooks/useAccountSyncing';
import { AppState } from 'react-native';

const MOCK_ADDRESS = '0xc4955c0d639d99699bfd7ec54d9fafee40e4d272';

Expand Down Expand Up @@ -153,40 +151,4 @@ describe('Wallet', () => {
const foxIcon = screen.getByTestId(CommonSelectorsIDs.FOX_ICON);
expect(foxIcon).toBeDefined();
});
it('dispatches account syncing on mount', () => {
jest.clearAllMocks();
//@ts-expect-error we are ignoring the navigation params on purpose because we do not want to mock setOptions to test the navbar
render(Wallet);
expect(useAccountSyncing().dispatchAccountSyncing).toHaveBeenCalledTimes(1);
});
it('dispatches account syncing when appState switches from inactive|background to active', () => {
jest.clearAllMocks();

const addEventListener = jest.spyOn(AppState, 'addEventListener');

//@ts-expect-error we are ignoring the navigation params on purpose because we do not want to mock setOptions to test the navbar
render(Wallet);

expect(addEventListener).toHaveBeenCalledWith(
'change',
expect.any(Function),
);
const handleAppStateChange = (
addEventListener as jest.Mock
).mock.calls.find(([event]) => event === 'change')[1];

act(() => {
handleAppStateChange('background');
handleAppStateChange('active');
});

expect(useAccountSyncing().dispatchAccountSyncing).toHaveBeenCalledTimes(2);

act(() => {
handleAppStateChange('inactive');
handleAppStateChange('active');
});

expect(useAccountSyncing().dispatchAccountSyncing).toHaveBeenCalledTimes(3);
});
});
50 changes: 4 additions & 46 deletions app/components/Views/Wallet/index.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
import React, {
useEffect,
useRef,
useCallback,
useContext,
useLayoutEffect,
} from 'react';
import React, { useEffect, useRef, useCallback, useContext } from 'react';
import {
ActivityIndicator,
StyleSheet,
View,
TextStyle,
InteractionManager,
Linking,
AppState,
AppStateStatus,
} from 'react-native';
import type { Theme } from '@metamask/design-tokens';
import { connect, useSelector } from 'react-redux';
Expand Down Expand Up @@ -93,8 +85,6 @@ import {
selectIsProfileSyncingEnabled,
} from '../../../selectors/notifications';
import { ButtonVariants } from '../../../component-library/components/Buttons/Button';
import { useListNotifications } from '../../../util/notifications/hooks/useNotifications';
import { useAccountSyncing } from '../../../util/notifications/hooks/useAccountSyncing';

import { PortfolioBalance } from '../../UI/Tokens/TokenList/PortfolioBalance';
import useCheckNftAutoDetectionModal from '../../hooks/useCheckNftAutoDetectionModal';
Expand Down Expand Up @@ -161,10 +151,7 @@ const Wallet = ({
showNftFetchingLoadingIndicator,
hideNftFetchingLoadingIndicator,
}: WalletProps) => {
const appState = useRef(AppState.currentState);
const { navigate } = useNavigation();
const { listNotifications } = useListNotifications();
const { dispatchAccountSyncing } = useAccountSyncing();
const walletRef = useRef(null);
const theme = useTheme();
const { toastRef } = useContext(ToastContext);
Expand Down Expand Up @@ -406,35 +393,6 @@ const Wallet = ({
[navigation, providerConfig.chainId],
);

// Layout effect when component/view is visible
// - fetches notifications
// - dispatches account syncing
useLayoutEffect(() => {
const handleAppStateChange = (nextAppState: AppStateStatus) => {
if (
appState.current?.match(/inactive|background/) &&
nextAppState === 'active'
) {
listNotifications();
dispatchAccountSyncing();
}

appState.current = nextAppState;
};

const subscription = AppState.addEventListener(
'change',
handleAppStateChange,
);

listNotifications();
dispatchAccountSyncing();

return () => {
subscription.remove();
};
}, [listNotifications, dispatchAccountSyncing]);

useEffect(() => {
navigation.setOptions(
getWalletNavbarOptions(
Expand Down Expand Up @@ -514,7 +472,8 @@ const Wallet = ({
// TODO: Replace "any" with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let balance: any = 0;
let assets = tokens;

let assets = [...(tokens || [])];

if (accountBalanceByChainId) {
balance = renderFromWei(accountBalanceByChainId.balance);
Expand All @@ -539,9 +498,8 @@ const Wallet = ({
} as any,
...(tokens || []),
];
} else {
assets = tokens;
}

return (
<View
style={styles.wrapper}
Expand Down
4 changes: 2 additions & 2 deletions bitrise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ stages:
- run_ios_api_specs: {}
- run_tag_smoke_accounts_ios: {}
- run_tag_smoke_accounts_android: {}
- run_tag_smoke_notifications_ios: {}
- run_tag_smoke_notifications_android: {}
# - run_tag_smoke_notifications_ios: {}
# - run_tag_smoke_notifications_android: {}
# - run_tag_smoke_assets_ios: {}
- run_tag_smoke_assets_android: {}
- run_tag_smoke_confirmations_ios: {}
Expand Down

0 comments on commit 750701e

Please sign in to comment.