From 327cf31ca74ce0f444ecd10bebc0608cd1946d13 Mon Sep 17 00:00:00 2001 From: Pedro Pablo Aste Kompen Date: Fri, 7 Nov 2025 09:14:30 -0300 Subject: [PATCH 1/9] feat(ds): accept listItemProps in ListItemSelect (#22277) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This pull request enhances the flexibility of the `ListItemSelect` component by allowing custom props to be passed directly to its nested `ListItem` component. This makes it easier to customize accessibility and other properties when using `ListItemSelect`. **Component extensibility improvements:** * Added a new optional `listItemProps` prop to the `ListItemSelectProps` interface, allowing consumers to pass custom props to the nested `ListItem` component. * Updated the `ListItemSelect` component implementation to spread `listItemProps` onto the nested `ListItem` element. [[1]](diffhunk://#diff-b1ce9235b1fa82fff3758d3a8cefedb7decb33dd7445bf3411fb53925c9b06baR25) [[2]](diffhunk://#diff-b1ce9235b1fa82fff3758d3a8cefedb7decb33dd7445bf3411fb53925c9b06baL37-R38) **Testing enhancements:** * Added a test to verify that custom `listItemProps` are correctly passed through to the nested `ListItem`, ensuring that properties like `accessibilityHint` are set as expected. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** N/A ## **Screenshots/Recordings** ### **Before** ### **After** ## **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** - [ ] 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. --- > [!NOTE] > Adds `listItemProps` to `ListItemSelect` to forward props to its nested `ListItem`, with a test verifying passthrough (e.g., accessibility hints). > > - **Component Library** > - **`ListItemSelect`**: > - Adds optional `listItemProps` to `ListItemSelectProps` and spreads onto nested `ListItem` in `ListItemSelect.tsx`. > - **Tests**: > - Adds test ensuring `listItemProps` (e.g., `accessibilityHint`, `testID`) are passed through to the nested `ListItem`. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5e7b8bf73dd4996df6b7ebe0512ef8493b0f6f63. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../List/ListItemSelect/ListItemSelect.test.tsx | 17 +++++++++++++++++ .../List/ListItemSelect/ListItemSelect.tsx | 3 ++- .../List/ListItemSelect/ListItemSelect.types.ts | 4 ++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/component-library/components/List/ListItemSelect/ListItemSelect.test.tsx b/app/component-library/components/List/ListItemSelect/ListItemSelect.test.tsx index 3e85d37fcd56..6e82f3ba848f 100644 --- a/app/component-library/components/List/ListItemSelect/ListItemSelect.test.tsx +++ b/app/component-library/components/List/ListItemSelect/ListItemSelect.test.tsx @@ -174,4 +174,21 @@ describe('ListItemSelect', () => { const component = getByTestId('list-item-select'); expect(component).toBeOnTheScreen(); }); + + it('passes through custom listItemProps', () => { + const { getByTestId } = render( + null} + listItemProps={{ + accessibilityHint: 'Custom Hint', + testID: 'nested-list-item', + }} + > + Test Content + , + ); + + const component = getByTestId('nested-list-item'); + expect(component.props.accessibilityHint).toBe('Custom Hint'); + }); }); diff --git a/app/component-library/components/List/ListItemSelect/ListItemSelect.tsx b/app/component-library/components/List/ListItemSelect/ListItemSelect.tsx index 319ca6a678e0..c8dd8de20e97 100644 --- a/app/component-library/components/List/ListItemSelect/ListItemSelect.tsx +++ b/app/component-library/components/List/ListItemSelect/ListItemSelect.tsx @@ -22,6 +22,7 @@ const ListItemSelect: React.FC = ({ onLongPress, gap = DEFAULT_SELECTITEM_GAP, verticalAlignment, + listItemProps, ...props }) => { const { styles } = useStyles(styleSheet, { style, isDisabled }); @@ -34,7 +35,7 @@ const ListItemSelect: React.FC = ({ onLongPress={onLongPress} {...props} > - + {children} {isSelected && ( diff --git a/app/component-library/components/List/ListItemSelect/ListItemSelect.types.ts b/app/component-library/components/List/ListItemSelect/ListItemSelect.types.ts index 8ec21c11754a..06c3d4181e91 100644 --- a/app/component-library/components/List/ListItemSelect/ListItemSelect.types.ts +++ b/app/component-library/components/List/ListItemSelect/ListItemSelect.types.ts @@ -18,6 +18,10 @@ export interface ListItemSelectProps * Optional prop to determine if the item is disabled. */ isDisabled?: boolean; + /** + * Optional prop to configure the prop of nested ListItem + */ + listItemProps?: Partial; } /** From 2a5d2de5dce5d5250eb46426ea5a0dee6d427673 Mon Sep 17 00:00:00 2001 From: tommasini <46944231+tommasini@users.noreply.github.com> Date: Fri, 7 Nov 2025 12:21:37 +0000 Subject: [PATCH 2/9] fix: re order migrations 105, 106, 107 cp-7.59.0 (#22276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Re-order migrations 105, 106, 107 Removal of migration 104 redux-persist slicing. No longer needed ## **Changelog** CHANGELOG entry: ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** ## **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. --- > [!NOTE] > Reorders and repurposes migrations (104–106), removes 107, updates inflation/deflation thresholds, and aligns tests with the new behaviors. > > - **Migrations**: > - **104**: Simplified to only reset `PhishingController.urlScanCache`; adds error reporting for invalid state. > - **105**: Now removes `RatesController` from `engine.backgroundState`; no-ops if absent; standardized error handling. > - **106**: Now cleans PPOM MMKV storage (`PPOMDB`); logic moved from former `107`; returns unchanged state; error captured. > - **Removed**: `107` migration and its tests. > - **Index** (`migrations/index.ts`): Dropped `107` from `migrationList`; changed inflate trigger to `> 106` and deflate on last version `>= 106`. > - **Tests**: > - Updated/added tests for 104–106 reflecting new responsibilities and error messages. > - Adjusted async migration flow tests to new version thresholds; removed old 107 tests; renamed for concision. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit a52e1231a65bc13ee90ce267df59910316798a32. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --------- Co-authored-by: Aslau Mario-Daniel --- app/store/migrations/104.test.ts | 287 ++++++++++++----------------- app/store/migrations/104.ts | 101 +++------- app/store/migrations/105.test.ts | 147 ++++++++------- app/store/migrations/105.ts | 31 ++-- app/store/migrations/106.test.ts | 181 +++++------------- app/store/migrations/106.ts | 45 +++-- app/store/migrations/107.test.ts | 82 --------- app/store/migrations/107.ts | 40 ---- app/store/migrations/index.test.ts | 66 +++---- app/store/migrations/index.ts | 6 +- 10 files changed, 335 insertions(+), 651 deletions(-) delete mode 100644 app/store/migrations/107.test.ts delete mode 100644 app/store/migrations/107.ts diff --git a/app/store/migrations/104.test.ts b/app/store/migrations/104.test.ts index df88ebadd861..1018004d78c4 100644 --- a/app/store/migrations/104.test.ts +++ b/app/store/migrations/104.test.ts @@ -1,213 +1,162 @@ import migrate from './104'; -import FilesystemStorage from 'redux-persist-filesystem-storage'; -import Device from '../../util/device'; +import { ensureValidState } from './util'; +import { captureException } from '@sentry/react-native'; -jest.mock('redux-persist-filesystem-storage'); -const mockFilesystemStorage = FilesystemStorage as jest.Mocked< - typeof FilesystemStorage ->; +jest.mock('@sentry/react-native', () => ({ + captureException: jest.fn(), +})); -jest.mock('../../util/device'); -const mockDevice = Device as jest.Mocked; +jest.mock('./util', () => ({ + ensureValidState: jest.fn(), +})); -describe('Migration 104', () => { +const mockedCaptureException = jest.mocked(captureException); +const mockedEnsureValidState = jest.mocked(ensureValidState); + +describe('Migration 104: Reset PhishingController urlScanCache', () => { beforeEach(() => { - jest.clearAllMocks(); - mockDevice.isIos.mockReturnValue(true); - mockFilesystemStorage.setItem.mockResolvedValue(); + jest.resetAllMocks(); }); - it('should migrate existing engine data to individual controller storage', async () => { - const mockState = { - engine: { - backgroundState: { - KeyringController: { - vault: 'encrypted-vault-data', - isUnlocked: false, - }, - NetworkController: { - network: 'mainnet', - chainId: '1', - }, - TransactionController: { - transactions: [{ id: '1', status: 'pending' }], - methodData: { '0x123': { method: 'transfer' } }, - }, - }, - }, - }; - - const result = await migrate(mockState); - - expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(3); - - expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith( - 'persist:KeyringController', - JSON.stringify({ - vault: 'encrypted-vault-data', - isUnlocked: false, - }), - true, - ); + it('returns state unchanged if ensureValidState fails', () => { + const state = { some: 'state' }; - expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith( - 'persist:NetworkController', - JSON.stringify({ - network: 'mainnet', - chainId: '1', - }), - true, - ); + mockedEnsureValidState.mockReturnValue(false); - expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith( - 'persist:TransactionController', - JSON.stringify({ - transactions: [{ id: '1', status: 'pending' }], - methodData: { '0x123': { method: 'transfer' } }, - }), - true, - ); + const migratedState = migrate(state); - expect(result).toEqual({ - engine: { - backgroundState: {}, - }, - }); + expect(migratedState).toBe(state); + expect(mockedCaptureException).not.toHaveBeenCalled(); }); - it('should completely clear backgroundState when all controllers migrate successfully', async () => { - const mockState = { + it('captures exception if PhishingController state is invalid', () => { + const state = { engine: { backgroundState: { - KeyringController: { - vault: 'encrypted-vault-data', - }, - NetworkController: { - network: 'mainnet', - }, + // PhishingController is missing }, }, }; - // All migrations succeed - mockFilesystemStorage.setItem.mockResolvedValue(); + mockedEnsureValidState.mockReturnValue(true); - const result = await migrate(mockState); + const migratedState = migrate(state); - expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2); - - // Should completely clear backgroundState when all controllers migrate successfully - expect(result).toEqual({ - engine: { - backgroundState: {}, - }, - }); + expect(migratedState).toEqual(state); + expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); + expect(mockedCaptureException.mock.calls[0][0].message).toContain( + 'Migration 104: Invalid PhishingController state', + ); }); - it('should handle empty engine data gracefully', async () => { - const mockState = { + it('resets PhishingController urlScanCache to empty object while preserving other fields', () => { + interface TestState { engine: { - backgroundState: {}, - }, - }; - - const result = await migrate(mockState); - - expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled(); - - expect(result).toEqual(mockState); - }); - - it('should handle missing engine data gracefully', async () => { - const mockState = { - engine: {}, - }; - - const result = await migrate(mockState); - - expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled(); - // Should return state unchanged - expect(result).toEqual(mockState); - }); - - it('should handle partial controller data', async () => { - const mockState = { + backgroundState: { + PhishingController: { + c2DomainBlocklistLastFetched: number; + phishingLists: string[]; + whitelist: string[]; + hotlistLastFetched: number; + stalelistLastFetched: number; + urlScanCache: Record; + extraProperty?: string; + }; + OtherController: { + shouldStayUntouched: boolean; + }; + }; + }; + } + + const state: TestState = { engine: { backgroundState: { - KeyringController: { - vault: 'encrypted-vault-data', + PhishingController: { + c2DomainBlocklistLastFetched: 123456789, + phishingLists: ['list1', 'list2'], + whitelist: ['site1', 'site2'], + hotlistLastFetched: 987654321, + stalelistLastFetched: 123123123, + urlScanCache: { + 'example.com': { result: 'safe', timestamp: 1234567890 }, + 'phishing.com': { result: 'malicious', timestamp: 9876543210 }, + }, + extraProperty: 'should remain', }, - TransactionController: { - transactions: [], + OtherController: { + shouldStayUntouched: true, }, }, }, }; - const result = await migrate(mockState); - - expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2); - - expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith( - 'persist:KeyringController', - JSON.stringify({ vault: 'encrypted-vault-data' }), - true, - ); - - expect(mockFilesystemStorage.setItem).toHaveBeenCalledWith( - 'persist:TransactionController', - JSON.stringify({ transactions: [] }), - true, - ); - - expect(result).toEqual({ - engine: { - backgroundState: {}, - }, + mockedEnsureValidState.mockReturnValue(true); + + const migratedState = migrate(state) as typeof state; + + // urlScanCache should be reset to empty object + expect( + migratedState.engine.backgroundState.PhishingController.urlScanCache, + ).toEqual({}); + + // Other fields should remain unchanged + expect( + migratedState.engine.backgroundState.PhishingController + .c2DomainBlocklistLastFetched, + ).toBe(123456789); + expect( + migratedState.engine.backgroundState.PhishingController.phishingLists, + ).toEqual(['list1', 'list2']); + expect( + migratedState.engine.backgroundState.PhishingController.whitelist, + ).toEqual(['site1', 'site2']); + expect( + migratedState.engine.backgroundState.PhishingController + .hotlistLastFetched, + ).toBe(987654321); + expect( + migratedState.engine.backgroundState.PhishingController + .stalelistLastFetched, + ).toBe(123123123); + expect( + migratedState.engine.backgroundState.PhishingController.extraProperty, + ).toBe('should remain'); + + expect(migratedState.engine.backgroundState.OtherController).toEqual({ + shouldStayUntouched: true, }); + + expect(mockedCaptureException).not.toHaveBeenCalled(); }); - it('should handle storage errors gracefully and preserve failed controller state', async () => { - const mockState = { + it('handles error during migration', () => { + // Create state with a PhishingController that throws when urlScanCache is accessed + const state = { engine: { backgroundState: { - KeyringController: { - vault: 'encrypted-vault-data', - }, - NetworkController: { - network: 'mainnet', - }, + PhishingController: Object.defineProperty({}, 'urlScanCache', { + get: () => { + throw new Error('Test error'); + }, + set: () => { + throw new Error('Test error'); + }, + configurable: true, + enumerable: true, + }), }, }, }; - mockFilesystemStorage.setItem - .mockRejectedValueOnce(new Error('Storage error')) - .mockResolvedValueOnce(); - - const result = await migrate(mockState); + mockedEnsureValidState.mockReturnValue(true); - expect(mockFilesystemStorage.setItem).toHaveBeenCalledTimes(2); + const migratedState = migrate(state); - // Should preserve failed controller state to prevent data loss - expect(result).toEqual({ - engine: { - backgroundState: { - KeyringController: { - vault: 'encrypted-vault-data', - }, - // NetworkController should be migrated successfully and removed from backgroundState - }, - }, - }); - }); - - it('should handle invalid state gracefully', async () => { - const invalidState = null; - - const result = await migrate(invalidState); - - expect(result).toBe(invalidState); - expect(mockFilesystemStorage.setItem).not.toHaveBeenCalled(); + expect(migratedState).toEqual(state); + expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); + expect(mockedCaptureException.mock.calls[0][0].message).toContain( + 'Migration 104: cleaning PhishingController state failed with error', + ); }); }); diff --git a/app/store/migrations/104.ts b/app/store/migrations/104.ts index d86c699f8935..f741eea6693d 100644 --- a/app/store/migrations/104.ts +++ b/app/store/migrations/104.ts @@ -1,101 +1,46 @@ import { hasProperty, isObject } from '@metamask/utils'; import { ensureValidState } from './util'; import { captureException } from '@sentry/react-native'; -import FilesystemStorage from 'redux-persist-filesystem-storage'; -import Device from '../../util/device'; -// Note: Do NOT rely on a static controller list. Iterate discovered -// controllers from the legacy engine.backgroundState to avoid missing any. /** - * Migration to transition from old redux-persist engine data to new individual controller storage system. + * Migration 104: Reset PhishingController urlScanCache * - * This migration: - * 1. Checks if old engine data exists in redux-persist - * 2. Splits the engine data into individual controller files - * 3. Saves each controller to its own file using the new storage system - * 4. Clears the old engine data from redux-persist - * - * @param state - The current MetaMask extension state. - * @returns The updated state with engine data cleared from redux-persist. + * This migration resets only the urlScanCache object in the PhishingController state */ -export default async function migrate(state: unknown) { - if (!ensureValidState(state, 104)) { +const migration = (state: unknown): unknown => { + const migrationVersion = 104; + + if (!ensureValidState(state, migrationVersion)) { return state; } try { - const { engine } = state; - if ( - hasProperty(engine, 'backgroundState') && - isObject(engine.backgroundState) && - Object.keys(engine.backgroundState).length > 0 + !hasProperty(state.engine.backgroundState, 'PhishingController') || + !isObject(state.engine.backgroundState.PhishingController) ) { - const controllers = engine.backgroundState; - let failedControllers = 0; - const failedControllerStates: Record = {}; - - // Migrate every controller present in the legacy backgroundState - for (const controllerName of Object.keys(controllers)) { - try { - if ( - hasProperty(controllers, controllerName) && - isObject(controllers[controllerName]) - ) { - const controllerState = controllers[controllerName]; - - const key = `persist:${controllerName}`; - const value = JSON.stringify(controllerState); - - await FilesystemStorage.setItem(key, value, Device.isIos()); - } - } catch (error) { - failedControllers++; - captureException( - new Error( - `Migration 104: Failed to migrate ${controllerName} to individual storage: ${String( - error, - )}`, - ), - ); - - // Preserve failed controller state to prevent data loss - if (hasProperty(controllers, controllerName)) { - failedControllerStates[controllerName] = - controllers[controllerName]; - } - } - } - - // Only clear successfully migrated controllers, preserve failed ones to prevent data loss - // Create new state object to maintain immutability - const newState = { ...state }; - newState.engine = { - ...engine, - backgroundState: failedControllers > 0 ? failedControllerStates : {}, - }; - - if (failedControllers > 0) { - captureException( - new Error( - `Migration 104: ${failedControllers} controllers failed to migrate, preserving their state in redux-persist`, - ), - ); - } - - return newState; + captureException( + new Error( + `Migration 104: Invalid PhishingController state: '${JSON.stringify( + state.engine.backgroundState.PhishingController, + )}'`, + ), + ); + return state; } + // Only reset the urlScanCache field to an empty object + state.engine.backgroundState.PhishingController.urlScanCache = {}; + return state; } catch (error) { captureException( new Error( - `Migration 104: Failed to migrate from redux-persist to individual controller storage: ${String( - error, - )}`, + `Migration 104: cleaning PhishingController state failed with error: ${error}`, ), ); - return state; } -} +}; + +export default migration; diff --git a/app/store/migrations/105.test.ts b/app/store/migrations/105.test.ts index 8325e06d16e6..53a0a5f7f714 100644 --- a/app/store/migrations/105.test.ts +++ b/app/store/migrations/105.test.ts @@ -13,7 +13,7 @@ jest.mock('./util', () => ({ const mockedCaptureException = jest.mocked(captureException); const mockedEnsureValidState = jest.mocked(ensureValidState); -describe('Migration 105: Reset PhishingController urlScanCache', () => { +describe('Migration 105: Remove RatesController state', () => { beforeEach(() => { jest.resetAllMocks(); }); @@ -29,13 +29,9 @@ describe('Migration 105: Reset PhishingController urlScanCache', () => { expect(mockedCaptureException).not.toHaveBeenCalled(); }); - it('captures exception if PhishingController state is invalid', () => { + it('returns state unchanged if backgroundState is missing', () => { const state = { - engine: { - backgroundState: { - // PhishingController is missing - }, - }, + engine: {}, }; mockedEnsureValidState.mockReturnValue(true); @@ -43,24 +39,20 @@ describe('Migration 105: Reset PhishingController urlScanCache', () => { const migratedState = migrate(state); expect(migratedState).toEqual(state); - expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); - expect(mockedCaptureException.mock.calls[0][0].message).toContain( - 'Migration 105: Invalid PhishingController state', - ); + expect(mockedCaptureException).not.toHaveBeenCalled(); }); - it('resets PhishingController urlScanCache to empty object while preserving other fields', () => { + it('removes RatesController from backgroundState', () => { interface TestState { engine: { backgroundState: { - PhishingController: { - c2DomainBlocklistLastFetched: number; - phishingLists: string[]; - whitelist: string[]; - hotlistLastFetched: number; - stalelistLastFetched: number; - urlScanCache: Record; - extraProperty?: string; + RatesController?: { + cryptocurrencies: string[]; + fiatCurrency: string; + rates: Record; + }; + MultichainAssetsRatesController: { + someProperty: string; }; OtherController: { shouldStayUntouched: boolean; @@ -72,17 +64,24 @@ describe('Migration 105: Reset PhishingController urlScanCache', () => { const state: TestState = { engine: { backgroundState: { - PhishingController: { - c2DomainBlocklistLastFetched: 123456789, - phishingLists: ['list1', 'list2'], - whitelist: ['site1', 'site2'], - hotlistLastFetched: 987654321, - stalelistLastFetched: 123123123, - urlScanCache: { - 'example.com': { result: 'safe', timestamp: 1234567890 }, - 'phishing.com': { result: 'malicious', timestamp: 9876543210 }, + RatesController: { + cryptocurrencies: ['btc', 'sol'], + fiatCurrency: 'usd', + rates: { + btc: { + conversionRate: 45000, + conversionDate: 1234567890, + usdConversionRate: 45000, + }, + sol: { + conversionRate: 100, + conversionDate: 1234567890, + usdConversionRate: 100, + }, }, - extraProperty: 'should remain', + }, + MultichainAssetsRatesController: { + someProperty: 'should remain', }, OtherController: { shouldStayUntouched: true, @@ -95,34 +94,15 @@ describe('Migration 105: Reset PhishingController urlScanCache', () => { const migratedState = migrate(state) as typeof state; - // urlScanCache should be reset to empty object expect( - migratedState.engine.backgroundState.PhishingController.urlScanCache, - ).toEqual({}); + migratedState.engine.backgroundState.RatesController, + ).toBeUndefined(); - // Other fields should remain unchanged - expect( - migratedState.engine.backgroundState.PhishingController - .c2DomainBlocklistLastFetched, - ).toBe(123456789); - expect( - migratedState.engine.backgroundState.PhishingController.phishingLists, - ).toEqual(['list1', 'list2']); - expect( - migratedState.engine.backgroundState.PhishingController.whitelist, - ).toEqual(['site1', 'site2']); expect( - migratedState.engine.backgroundState.PhishingController - .hotlistLastFetched, - ).toBe(987654321); - expect( - migratedState.engine.backgroundState.PhishingController - .stalelistLastFetched, - ).toBe(123123123); - expect( - migratedState.engine.backgroundState.PhishingController.extraProperty, - ).toBe('should remain'); - + migratedState.engine.backgroundState.MultichainAssetsRatesController, + ).toEqual({ + someProperty: 'should remain', + }); expect(migratedState.engine.backgroundState.OtherController).toEqual({ shouldStayUntouched: true, }); @@ -130,33 +110,62 @@ describe('Migration 105: Reset PhishingController urlScanCache', () => { expect(mockedCaptureException).not.toHaveBeenCalled(); }); - it('handles error during migration', () => { - // Create state with a PhishingController that throws when urlScanCache is accessed - const state = { + it('leaves state unchanged if RatesController does not exist', () => { + interface TestState { engine: { backgroundState: { - PhishingController: Object.defineProperty({}, 'urlScanCache', { - get: () => { - throw new Error('Test error'); - }, - set: () => { - throw new Error('Test error'); - }, - configurable: true, - enumerable: true, - }), + MultichainAssetsRatesController: { + someProperty: string; + }; + OtherController: { + shouldStayUntouched: boolean; + }; + }; + }; + } + + const state: TestState = { + engine: { + backgroundState: { + MultichainAssetsRatesController: { + someProperty: 'should remain', + }, + OtherController: { + shouldStayUntouched: true, + }, }, }, }; mockedEnsureValidState.mockReturnValue(true); + const migratedState = migrate(state) as typeof state; + + expect(migratedState).toEqual(state); + expect(mockedCaptureException).not.toHaveBeenCalled(); + }); + + it('handles error during migration', () => { + const state = { + engine: { + backgroundState: Object.defineProperty({}, 'RatesController', { + get: () => { + throw new Error('Test error'); + }, + configurable: true, + enumerable: true, + }), + }, + }; + + mockedEnsureValidState.mockReturnValue(true); + const migratedState = migrate(state); expect(migratedState).toEqual(state); expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); expect(mockedCaptureException.mock.calls[0][0].message).toContain( - 'Migration 105: cleaning PhishingController state failed with error', + 'Migration 105 failed', ); }); }); diff --git a/app/store/migrations/105.ts b/app/store/migrations/105.ts index d16233a4262a..66af1713c8a0 100644 --- a/app/store/migrations/105.ts +++ b/app/store/migrations/105.ts @@ -3,9 +3,10 @@ import { ensureValidState } from './util'; import { captureException } from '@sentry/react-native'; /** - * Migration 105: Reset PhishingController urlScanCache + * Migration 105: Remove RatesController state * - * This migration resets only the urlScanCache object in the PhishingController state + * This migration removes the entire RatesController from backgroundState + * as it's no longer used (functionality moved to MultichainAssetsRatesController) */ const migration = (state: unknown): unknown => { const migrationVersion = 105; @@ -15,29 +16,23 @@ const migration = (state: unknown): unknown => { } try { - if ( - !hasProperty(state.engine.backgroundState, 'PhishingController') || - !isObject(state.engine.backgroundState.PhishingController) - ) { - captureException( - new Error( - `Migration 105: Invalid PhishingController state: '${JSON.stringify( - state.engine.backgroundState.PhishingController, - )}'`, - ), - ); + const backgroundState = state?.engine?.backgroundState; + + if (!backgroundState) { return state; } - // Only reset the urlScanCache field to an empty object - state.engine.backgroundState.PhishingController.urlScanCache = {}; + if ( + hasProperty(backgroundState, 'RatesController') && + isObject(backgroundState.RatesController) + ) { + delete backgroundState.RatesController; + } return state; } catch (error) { captureException( - new Error( - `Migration 105: cleaning PhishingController state failed with error: ${error}`, - ), + new Error(`Migration ${migrationVersion} failed: ${error}`), ); return state; } diff --git a/app/store/migrations/106.test.ts b/app/store/migrations/106.test.ts index d5338dbc9763..f1cdd62f44e5 100644 --- a/app/store/migrations/106.test.ts +++ b/app/store/migrations/106.test.ts @@ -1,171 +1,82 @@ +import { MMKV } from 'react-native-mmkv'; import migrate from './106'; -import { ensureValidState } from './util'; -import { captureException } from '@sentry/react-native'; -jest.mock('@sentry/react-native', () => ({ - captureException: jest.fn(), +jest.mock('react-native-mmkv', () => ({ + MMKV: jest.fn(), })); -jest.mock('./util', () => ({ - ensureValidState: jest.fn(), -})); +describe('Migration #106', () => { + const mockMMKVInstance = { + getAllKeys: jest.fn(), + clearAll: jest.fn(), + }; -const mockedCaptureException = jest.mocked(captureException); -const mockedEnsureValidState = jest.mocked(ensureValidState); + (MMKV as jest.Mock).mockImplementation(() => mockMMKVInstance); -describe('Migration 106: Remove RatesController state', () => { beforeEach(() => { - jest.resetAllMocks(); + jest.clearAllMocks(); }); - it('returns state unchanged if ensureValidState fails', () => { - const state = { some: 'state' }; - - mockedEnsureValidState.mockReturnValue(false); - - const migratedState = migrate(state); - - expect(migratedState).toBe(state); - expect(mockedCaptureException).not.toHaveBeenCalled(); + afterAll(() => { + jest.restoreAllMocks(); }); - it('returns state unchanged if backgroundState is missing', () => { - const state = { - engine: {}, + it('should clear PPOM storage when keys exist', () => { + const oldState = { + engine: { + backgroundState: {}, + }, }; - mockedEnsureValidState.mockReturnValue(true); + mockMMKVInstance.getAllKeys.mockReturnValue(['key1-0x1', 'key2-0x1']); - const migratedState = migrate(state); + const newState = migrate(oldState); - expect(migratedState).toEqual(state); - expect(mockedCaptureException).not.toHaveBeenCalled(); + expect(MMKV).toHaveBeenCalledWith({ id: 'PPOMDB' }); + expect(mockMMKVInstance.getAllKeys).toHaveBeenCalled(); + expect(mockMMKVInstance.clearAll).toHaveBeenCalled(); + expect(newState).toEqual(oldState); }); - it('removes RatesController from backgroundState', () => { - interface TestState { - engine: { - backgroundState: { - RatesController?: { - cryptocurrencies: string[]; - fiatCurrency: string; - rates: Record; - }; - MultichainAssetsRatesController: { - someProperty: string; - }; - OtherController: { - shouldStayUntouched: boolean; - }; - }; - }; - } - - const state: TestState = { + it('should not call clearAll when no keys exist', () => { + const oldState = { engine: { - backgroundState: { - RatesController: { - cryptocurrencies: ['btc', 'sol'], - fiatCurrency: 'usd', - rates: { - btc: { - conversionRate: 45000, - conversionDate: 1234567890, - usdConversionRate: 45000, - }, - sol: { - conversionRate: 100, - conversionDate: 1234567890, - usdConversionRate: 100, - }, - }, - }, - MultichainAssetsRatesController: { - someProperty: 'should remain', - }, - OtherController: { - shouldStayUntouched: true, - }, - }, + backgroundState: {}, }, }; - mockedEnsureValidState.mockReturnValue(true); - - const migratedState = migrate(state) as typeof state; + mockMMKVInstance.getAllKeys.mockReturnValue([]); - expect( - migratedState.engine.backgroundState.RatesController, - ).toBeUndefined(); + const newState = migrate(oldState); - expect( - migratedState.engine.backgroundState.MultichainAssetsRatesController, - ).toEqual({ - someProperty: 'should remain', - }); - expect(migratedState.engine.backgroundState.OtherController).toEqual({ - shouldStayUntouched: true, - }); - - expect(mockedCaptureException).not.toHaveBeenCalled(); + expect(MMKV).toHaveBeenCalledWith({ id: 'PPOMDB' }); + expect(mockMMKVInstance.getAllKeys).toHaveBeenCalled(); + expect(mockMMKVInstance.clearAll).not.toHaveBeenCalled(); + expect(newState).toEqual(oldState); }); - it('leaves state unchanged if RatesController does not exist', () => { - interface TestState { - engine: { - backgroundState: { - MultichainAssetsRatesController: { - someProperty: string; - }; - OtherController: { - shouldStayUntouched: boolean; - }; - }; - }; - } - - const state: TestState = { + it('should handle errors gracefully', () => { + const oldState = { engine: { - backgroundState: { - MultichainAssetsRatesController: { - someProperty: 'should remain', - }, - OtherController: { - shouldStayUntouched: true, - }, - }, + backgroundState: {}, }, }; - mockedEnsureValidState.mockReturnValue(true); + mockMMKVInstance.getAllKeys.mockImplementation(() => { + throw new Error('Storage error'); + }); - const migratedState = migrate(state) as typeof state; + const newState = migrate(oldState); - expect(migratedState).toEqual(state); - expect(mockedCaptureException).not.toHaveBeenCalled(); + expect(newState).toEqual(oldState); }); - it('handles error during migration', () => { - const state = { - engine: { - backgroundState: Object.defineProperty({}, 'RatesController', { - get: () => { - throw new Error('Test error'); - }, - configurable: true, - enumerable: true, - }), - }, - }; - - mockedEnsureValidState.mockReturnValue(true); + it('should return state unchanged if state is invalid', () => { + const invalidStates = [null, undefined, {}, { engine: null }]; - const migratedState = migrate(state); - - expect(migratedState).toEqual(state); - expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); - expect(mockedCaptureException.mock.calls[0][0].message).toContain( - 'Migration 106 failed', - ); + invalidStates.forEach((invalidState) => { + const newState = migrate(invalidState); + expect(newState).toEqual(invalidState); + }); }); }); diff --git a/app/store/migrations/106.ts b/app/store/migrations/106.ts index 1427f2398537..a738c80032de 100644 --- a/app/store/migrations/106.ts +++ b/app/store/migrations/106.ts @@ -1,41 +1,40 @@ -import { hasProperty, isObject } from '@metamask/utils'; -import { ensureValidState } from './util'; import { captureException } from '@sentry/react-native'; +import { MMKV } from 'react-native-mmkv'; +import { ensureValidState } from './util'; + +const migrationVersion = 106; /** - * Migration 106: Remove RatesController state + * Migration 106: Clean up PPOM MMKV storage after removing PPOM local execution * - * This migration removes the entire RatesController from backgroundState - * as it's no longer used (functionality moved to MultichainAssetsRatesController) + * This migration removes any lingering PPOM data stored in MMKV storage + * when the PPOM controller is removed from the codebase. */ -const migration = (state: unknown): unknown => { - const migrationVersion = 106; - +export default function migrate(state: unknown) { if (!ensureValidState(state, migrationVersion)) { return state; } try { - const backgroundState = state?.engine?.backgroundState; + const ppomStorageId = 'PPOMDB'; - if (!backgroundState) { - return state; - } + // Create MMKV instance with the same ID that was used by PPOM + const ppomStorage = new MMKV({ id: ppomStorageId }); - if ( - hasProperty(backgroundState, 'RatesController') && - isObject(backgroundState.RatesController) - ) { - delete backgroundState.RatesController; - } + // Get all keys from the PPOM storage + const allKeys = ppomStorage.getAllKeys(); - return state; + if (allKeys.length > 0) { + // Clear all data from PPOM storage + ppomStorage.clearAll(); + } } catch (error) { captureException( - new Error(`Migration ${migrationVersion} failed: ${error}`), + new Error( + `Migration ${migrationVersion}: Failed to clean up PPOM storage: ${error}`, + ), ); - return state; } -}; -export default migration; + return state; +} diff --git a/app/store/migrations/107.test.ts b/app/store/migrations/107.test.ts deleted file mode 100644 index 55a64d56d53e..000000000000 --- a/app/store/migrations/107.test.ts +++ /dev/null @@ -1,82 +0,0 @@ -import { MMKV } from 'react-native-mmkv'; -import migrate from './107'; - -jest.mock('react-native-mmkv', () => ({ - MMKV: jest.fn(), -})); - -describe('Migration #107', () => { - const mockMMKVInstance = { - getAllKeys: jest.fn(), - clearAll: jest.fn(), - }; - - (MMKV as jest.Mock).mockImplementation(() => mockMMKVInstance); - - beforeEach(() => { - jest.clearAllMocks(); - }); - - afterAll(() => { - jest.restoreAllMocks(); - }); - - it('should clear PPOM storage when keys exist', () => { - const oldState = { - engine: { - backgroundState: {}, - }, - }; - - mockMMKVInstance.getAllKeys.mockReturnValue(['key1-0x1', 'key2-0x1']); - - const newState = migrate(oldState); - - expect(MMKV).toHaveBeenCalledWith({ id: 'PPOMDB' }); - expect(mockMMKVInstance.getAllKeys).toHaveBeenCalled(); - expect(mockMMKVInstance.clearAll).toHaveBeenCalled(); - expect(newState).toEqual(oldState); - }); - - it('should not call clearAll when no keys exist', () => { - const oldState = { - engine: { - backgroundState: {}, - }, - }; - - mockMMKVInstance.getAllKeys.mockReturnValue([]); - - const newState = migrate(oldState); - - expect(MMKV).toHaveBeenCalledWith({ id: 'PPOMDB' }); - expect(mockMMKVInstance.getAllKeys).toHaveBeenCalled(); - expect(mockMMKVInstance.clearAll).not.toHaveBeenCalled(); - expect(newState).toEqual(oldState); - }); - - it('should handle errors gracefully', () => { - const oldState = { - engine: { - backgroundState: {}, - }, - }; - - mockMMKVInstance.getAllKeys.mockImplementation(() => { - throw new Error('Storage error'); - }); - - const newState = migrate(oldState); - - expect(newState).toEqual(oldState); - }); - - it('should return state unchanged if state is invalid', () => { - const invalidStates = [null, undefined, {}, { engine: null }]; - - invalidStates.forEach((invalidState) => { - const newState = migrate(invalidState); - expect(newState).toEqual(invalidState); - }); - }); -}); diff --git a/app/store/migrations/107.ts b/app/store/migrations/107.ts deleted file mode 100644 index 00f8ebdbb9d7..000000000000 --- a/app/store/migrations/107.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { captureException } from '@sentry/react-native'; -import { MMKV } from 'react-native-mmkv'; -import { ensureValidState } from './util'; - -const migrationVersion = 107; - -/** - * Migration 106: Clean up PPOM MMKV storage after removing PPOM local execution - * - * This migration removes any lingering PPOM data stored in MMKV storage - * when the PPOM controller is removed from the codebase. - */ -export default function migrate(state: unknown) { - if (!ensureValidState(state, migrationVersion)) { - return state; - } - - try { - const ppomStorageId = 'PPOMDB'; - - // Create MMKV instance with the same ID that was used by PPOM - const ppomStorage = new MMKV({ id: ppomStorageId }); - - // Get all keys from the PPOM storage - const allKeys = ppomStorage.getAllKeys(); - - if (allKeys.length > 0) { - // Clear all data from PPOM storage - ppomStorage.clearAll(); - } - } catch (error) { - captureException( - new Error( - `Migration ${migrationVersion}: Failed to clean up PPOM storage: ${error}`, - ), - ); - } - - return state; -} diff --git a/app/store/migrations/index.test.ts b/app/store/migrations/index.test.ts index 735f12e88224..66944858111e 100644 --- a/app/store/migrations/index.test.ts +++ b/app/store/migrations/index.test.ts @@ -65,7 +65,7 @@ const asyncMigration = async (state: any) => { }; describe('asyncifyMigrations', () => { - it('should convert synchronous migrations to asynchronous', async () => { + it('converts synchronous migrations to asynchronous', async () => { const testMigrationList = { ...recentMigrations, [numberOfMigrations]: asyncMigration, @@ -109,7 +109,7 @@ describe('migrations', () => { }); }); - it('should migrate successfully when latest migration is synchronous', async () => { + it('migrates successfully when latest migration is synchronous', async () => { const testMigrationList = { ...recentMigrations, [numberOfMigrations]: synchronousMigration, @@ -132,7 +132,7 @@ describe('migrations', () => { expect((migratedState as Record).test).toEqual('sync'); }); - it('should migrate successfully when latest migration is asynchronous', async () => { + it('migrates successfully when latest migration is asynchronous', async () => { const testMigrationList = { ...recentMigrations, [numberOfMigrations]: asyncMigration, @@ -155,7 +155,7 @@ describe('migrations', () => { expect((migratedState as Record).test).toEqual('async'); }); - it('should migrate successfully when using both synchronous and asynchronous migrations', async () => { + it('migrates successfully when using both synchronous and asynchronous migrations', async () => { const testMigrationList = { ...recentMigrations, [numberOfMigrations]: asyncMigration, @@ -194,7 +194,7 @@ describe('Critical Error Handling', () => { }); describe('inflateFromControllers error handling', () => { - it('should crash when ControllerStorage.getAllPersistedState fails', async () => { + it('crashes when ControllerStorage.getAllPersistedState fails', async () => { // Arrange const storageError = new Error('Storage access failed'); mockedControllerStorage.getAllPersistedState.mockRejectedValue( @@ -202,13 +202,13 @@ describe('Critical Error Handling', () => { ); const testMigrationList = { - 105: (state: unknown) => state, // Migration > 104 triggers inflation logic + 107: (state: unknown) => state, // Migration > 106 triggers inflation logic }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act & Assert - await expect(asyncMigrations['105'](initialState)).rejects.toThrow( + await expect(asyncMigrations['107'](initialState)).rejects.toThrow( 'Critical: Failed to load controller data for migration. Cannot continue safely as migrations may corrupt data without complete state. App will restart to attempt recovery. Error: Error: Storage access failed', ); @@ -220,18 +220,18 @@ describe('Critical Error Handling', () => { ); }); - it('should not crash when no controllers are found (empty state)', async () => { + it('does not crash when no controllers are found (empty state)', async () => { // Arrange mockedControllerStorage.getAllPersistedState.mockResolvedValue({}); const testMigrationList = { - 105: (state: unknown) => ({ ...(state as object), test: 'passed' }), + 107: (state: unknown) => ({ ...(state as object), test: 'passed' }), }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act - const result = await asyncMigrations['105'](initialState); + const result = await asyncMigrations['107'](initialState); // Assert expect((result as Record).test).toEqual('passed'); @@ -240,7 +240,7 @@ describe('Critical Error Handling', () => { }); describe('deflateToControllersAndStrip error handling', () => { - it('should crash when any controller fails to save during deflation', async () => { + it('crashes when any controller fails to save during deflation', async () => { // Arrange const stateWithControllers = { ...initialState, @@ -270,14 +270,14 @@ describe('Critical Error Handling', () => { }); const testMigrationList = { - 105: (state: unknown) => state, // This will trigger deflation after migration + 107: (state: unknown) => state, // This will trigger deflation after migration (lastVersion >= 106) }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act & Assert await expect( - asyncMigrations['105'](stateWithControllers), + asyncMigrations['107'](stateWithControllers), ).rejects.toThrow( "Critical: Migration failed for controller 'TestController'. Cannot continue with partial migration as this would corrupt user data. App will restart to attempt recovery. Error: Error: Disk full", ); @@ -290,7 +290,7 @@ describe('Critical Error Handling', () => { ); }); - it('should successfully deflate when all controllers save successfully', async () => { + it('deflates successfully when all controllers save successfully', async () => { // Arrange const stateWithControllers = { ...initialState, @@ -314,13 +314,13 @@ describe('Critical Error Handling', () => { mockedControllerStorage.setItem.mockResolvedValue(); const testMigrationList = { - 105: (state: unknown) => ({ ...(state as object), migrated: true }), + 107: (state: unknown) => ({ ...(state as object), migrated: true }), }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act - const result = (await asyncMigrations['105']( + const result = (await asyncMigrations['107']( stateWithControllers, )) as Record; @@ -331,7 +331,7 @@ describe('Critical Error Handling', () => { expect(mockedCaptureException).not.toHaveBeenCalled(); // No errors captured }); - it('should crash when deflation fails completely', async () => { + it('crashes when deflation fails completely', async () => { // Arrange const stateWithControllers = { ...initialState, @@ -354,14 +354,14 @@ describe('Critical Error Handling', () => { mockedControllerStorage.setItem.mockRejectedValue(catastrophicError); const testMigrationList = { - 105: (state: unknown) => state, + 107: (state: unknown) => state, }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act & Assert await expect( - asyncMigrations['105'](stateWithControllers), + asyncMigrations['107'](stateWithControllers), ).rejects.toThrow( "Critical: Migration failed for controller 'TestController'. Cannot continue with partial migration as this would corrupt user data. App will restart to attempt recovery. Error: Error: File system corrupted", ); @@ -376,29 +376,29 @@ describe('Critical Error Handling', () => { }); describe('Migration flow integration', () => { - it('should not trigger inflation/deflation for migrations <= 104', async () => { + it('does not trigger inflation/deflation for migrations < 106', async () => { // Arrange const testMigrationList = { - 104: (state: unknown) => ({ + 105: (state: unknown) => ({ ...(state as object), - test: 'migration104', + test: 'migration105', }), }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act - const result = await asyncMigrations['104'](initialState); + const result = await asyncMigrations['105'](initialState); // Assert - expect((result as Record).test).toEqual('migration104'); + expect((result as Record).test).toEqual('migration105'); expect( mockedControllerStorage.getAllPersistedState, ).not.toHaveBeenCalled(); expect(mockedControllerStorage.setItem).not.toHaveBeenCalled(); }); - it('should handle mixed migration versions correctly', async () => { + it('handles mixed migration versions correctly', async () => { // Arrange - Reset all mocks to clean state jest.clearAllMocks(); mockedControllerStorage.getAllPersistedState.mockResolvedValue({}); @@ -410,25 +410,25 @@ describe('Critical Error Handling', () => { } as PersistedState; const testMigrationList = { - 103: (state: unknown) => ({ ...(state as object), step103: true }), - 104: (state: unknown) => ({ ...(state as object), step104: true }), 105: (state: unknown) => ({ ...(state as object), step105: true }), + 106: (state: unknown) => ({ ...(state as object), step106: true }), + 107: (state: unknown) => ({ ...(state as object), step107: true }), }; const asyncMigrations = asyncifyMigrations(testMigrationList); // Act - Run migrations in sequence let state = stateWithoutControllers; - state = (await asyncMigrations['103'](state)) as PersistedState; - state = (await asyncMigrations['104'](state)) as PersistedState; - const finalState = await asyncMigrations['105'](state); + state = (await asyncMigrations['105'](state)) as PersistedState; + state = (await asyncMigrations['106'](state)) as PersistedState; + const finalState = await asyncMigrations['107'](state); // Assert - expect((finalState as Record).step103).toBe(true); - expect((finalState as Record).step104).toBe(true); expect((finalState as Record).step105).toBe(true); + expect((finalState as Record).step106).toBe(true); + expect((finalState as Record).step107).toBe(true); - // Inflation should only be called once for migration 105 + // Inflation should only be called once for migration 107 (> 106) expect( mockedControllerStorage.getAllPersistedState, ).toHaveBeenCalledTimes(1); diff --git a/app/store/migrations/index.ts b/app/store/migrations/index.ts index 545910796863..e63084cb88c3 100644 --- a/app/store/migrations/index.ts +++ b/app/store/migrations/index.ts @@ -107,7 +107,6 @@ import migration103 from './103'; import migration104 from './104'; import migration105 from './105'; import migration106 from './106'; -import migration107 from './107'; // Add migrations above this line import { ControllerStorage } from '../persistConfig'; @@ -231,7 +230,6 @@ export const migrationList: MigrationsList = { 104: migration104, 105: migration105, 106: migration106, - 107: migration107, }; // Enable both synchronous and asynchronous migrations @@ -378,13 +376,13 @@ export const asyncifyMigrations = (inputMigrations: MigrationsList) => { ) => { let state = await incomingState; - if (!didInflate && Number(migrationNumber) > 104) { + if (!didInflate && Number(migrationNumber) > 106) { state = await inflateFromControllers(state); didInflate = true; } const migratedState = await migrationFunction(state); - if (Number(migrationNumber) === lastVersion && lastVersion > 104) { + if (Number(migrationNumber) === lastVersion && lastVersion >= 106) { const s2 = migratedState as StateWithEngine; const hasControllers = Boolean( s2.engine?.backgroundState && From 77ef41d7ec405f6b53dba1e32b8a20a03a07d1a6 Mon Sep 17 00:00:00 2001 From: VGR Date: Fri, 7 Nov 2025 15:03:35 +0100 Subject: [PATCH 3/9] chore: rewards bip 44 tracked indicators (#22298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** - Account group rows in settings view show tracked/total supported addresses labels show the appropriate CTA icon depending on tracked addresses - Account group modal spawnable via settings view show unsupported header/addresses remove unsupported account type banner/alert aways show headers (tracked/untracked/unsupported) ## **Changelog** CHANGELOG entry: show tracked/untracked in reward settings view ## **Related Issue** https://consensyssoftware.atlassian.net/browse/RWDS-769 ## **Screenshots/Recordings** image image ## **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. --- > [!NOTE] > Adds tracked/total labels to account group rows, always shows tracked/untracked/unsupported sections in the opt-in modal, refines address processing and button logic, and updates tests/i18n accordingly. > > - **Rewards Settings UI**: > - Show tracked/total label via `strings('rewards.link_account_group.tracked_count')` in `RewardSettingsAccountGroup`. > - Contextual CTA icon (`check` when all tracked, `add` otherwise); disable link when no opted-out accounts. > - Minor layout tweaks; remove balance/Privacy Mode logic. > - **Opt-in Modal (`RewardOptInAccountGroupModal`)**: > - Refactor address flattening with wildcard/invalid-scope handling; treat `isSupported: undefined` as supported and filter EVM testnets. > - Always render section headers: `tracked`, `untracked`, and new `unsupported`; remove unsupported banner. > - Link button shows only when supported, untracked addresses exist; `onClose` triggers `navigation.goBack`. > - Update item testIDs to `flat-list-item-
-` and use `MultichainAddressRow` fields. > - **Account Group List**: > - Optimize `allAddresses` via `getAddressesForGroup`; provide `getItemType` to `FlashList`; header spacing tweaks; stable skeleton keys. > - **i18n**: > - Add `rewards.link_account_group.unsupported` and `rewards.link_account_group.tracked_count`. > - **Tests**: > - Large test updates to reflect new testIDs, sections, button states, loading, key extraction, item typing, and edge cases (wildcards, invalid scopes, missing data). > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ff4d9e93211525a492827b0b1e4267f2276706ef. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../RewardOptInAccountGroupModal.test.tsx | 636 ++++++++++++++---- .../Settings/RewardOptInAccountGroupModal.tsx | 260 ++++--- .../RewardSettingsAccountGroup.test.tsx | 205 +++--- .../Settings/RewardSettingsAccountGroup.tsx | 82 +-- .../RewardSettingsAccountGroupList.test.tsx | 116 +++- .../RewardSettingsAccountGroupList.tsx | 36 +- locales/languages/en.json | 2 + 7 files changed, 880 insertions(+), 457 deletions(-) diff --git a/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.test.tsx b/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.test.tsx index 59c062a71e81..a189c19a972f 100644 --- a/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.test.tsx +++ b/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.test.tsx @@ -162,6 +162,7 @@ jest.mock( { testID: 'bottom-sheet', ref, + onClose, ...props, }, children, @@ -227,42 +228,17 @@ jest.mock( ReactActual.createElement( View, { testID, ...props }, - ReactActual.createElement(View, { testID: `address-${address}` }), ReactActual.createElement(View, { - testID: `network-${networkName}`, + testID: `multichain-address-row-address`, + }), + ReactActual.createElement(View, { + testID: `multichain-address-row-network-name`, }), ), }; }, ); -// Mock RewardsInfoBanner -jest.mock('../RewardsInfoBanner', () => { - const ReactActual = jest.requireActual('react'); - const { View, Text } = jest.requireActual('react-native'); - - return { - __esModule: true, - default: ({ - title, - description, - testID, - ...props - }: { - title: string; - description: string; - testID?: string; - [key: string]: unknown; - }) => - ReactActual.createElement( - View, - { testID, ...props }, - ReactActual.createElement(Text, {}, title), - ReactActual.createElement(Text, {}, description), - ), - }; -}); - const mockUseNavigation = useNavigation as jest.MockedFunction< typeof useNavigation >; @@ -390,80 +366,98 @@ describe('RewardOptInAccountGroupModal', () => { }); describe('Basic Rendering', () => { - it('should render bottom sheet container', () => { + it('renders bottom sheet container', () => { const { getByTestId } = render(); expect(getByTestId('bottom-sheet')).toBeOnTheScreen(); }); - it('should render header with account group name', () => { + it('renders header with account group name', () => { const { getByTestId } = render(); expect(getByTestId('bottom-sheet-header')).toBeOnTheScreen(); }); - it('should render address list', () => { + it('renders address list', () => { const { getByTestId } = render(); expect(getByTestId('reward-opt-in-address-list')).toBeOnTheScreen(); }); - it('should render address items for each address', () => { + it('renders address items for each address', () => { const { getByTestId } = render(); - // Check that the MultichainAddressRow components are rendered expect( - getByTestId('address-0x1234567890123456789012345678901234567890'), + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), ).toBeOnTheScreen(); expect( - getByTestId('address-0x0987654321098765432109876543210987654321'), + getByTestId( + 'flat-list-item-0x0987654321098765432109876543210987654321-eip155:1', + ), ).toBeOnTheScreen(); }); - }); - describe('Unsupported Accounts Banner', () => { - it('should show banner when there are unsupported accounts', () => { - // Arrange + it('does not render address list when addressData is empty', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, - addressData: [ - ...defaultRouteParams.addressData, - { - address: '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', - hasOptedIn: false, - scopes: ['eip155:1' as CaipChainId], - isSupported: false, - }, - ], + addressData: [], }, key: 'test-route', name: 'RewardOptInAccountGroupModal', } as never); - // Act - const { getByTestId } = render(); + const { queryByTestId } = render(); - // Assert - expect(getByTestId('unsupported-accounts-banner')).toBeOnTheScreen(); + expect(queryByTestId('reward-opt-in-address-list')).toBeNull(); }); - it('should not show banner when all accounts are supported', () => { - const { queryByTestId } = render(); + it('calls navigation.goBack when BottomSheet onClose is triggered', () => { + const { getByTestId } = render(); + + const bottomSheet = getByTestId('bottom-sheet'); + const onClose = bottomSheet.props.onClose; - expect(queryByTestId('unsupported-accounts-banner')).toBeNull(); + if (onClose) { + onClose(); + } + + expect(mockGoBack).toHaveBeenCalledTimes(1); }); }); describe('Link Account Group Button', () => { - it('should render link button when there are accounts that can opt in', () => { + it('renders link button when there are accounts that can opt in', () => { const { getByTestId } = render(); expect(getByTestId('link-account-group-button')).toBeOnTheScreen(); }); - it('should not render link button when all accounts are opted in', () => { - // Arrange + it('does not render link button when all accounts are opted in', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: true, + scopes: ['eip155:1' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('link-account-group-button')).toBeNull(); + }); + + it('does not render link button when all supported addresses are opted in but unsupported addresses exist', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -474,21 +468,24 @@ describe('RewardOptInAccountGroupModal', () => { scopes: ['eip155:1' as CaipChainId], isSupported: true, }, + { + address: '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + hasOptedIn: false, + scopes: ['eip155:1' as CaipChainId], + isSupported: false, + }, ], }, key: 'test-route', name: 'RewardOptInAccountGroupModal', } as never); - // Act const { queryByTestId } = render(); - // Assert expect(queryByTestId('link-account-group-button')).toBeNull(); }); - it('should call linkAccountGroup when button is pressed', async () => { - // Arrange + it('calls linkAccountGroup when button is pressed', async () => { mockLinkAccountGroup.mockResolvedValue({ success: true, byAddress: { @@ -498,11 +495,9 @@ describe('RewardOptInAccountGroupModal', () => { const { getByTestId } = render(); - // Act const linkButton = getByTestId('link-account-group-button'); fireEvent.press(linkButton); - // Assert await waitFor(() => { expect(mockLinkAccountGroup).toHaveBeenCalledWith( 'keyring:wallet-1/ethereum', @@ -510,24 +505,20 @@ describe('RewardOptInAccountGroupModal', () => { }); }); - it('should show loading state when linking', () => { - // Arrange + it('shows loading state when linking', () => { mockUseLinkAccountGroup.mockReturnValue({ linkAccountGroup: mockLinkAccountGroup, isLoading: true, isError: false, }); - // Act const { getByTestId } = render(); - // Assert const linkButton = getByTestId('link-account-group-button'); expect(linkButton).toHaveProp('disabled', true); }); - it('should update local state after successful link', async () => { - // Arrange + it('updates local state after successful link', async () => { mockLinkAccountGroup.mockResolvedValue({ success: true, byAddress: { @@ -537,18 +528,24 @@ describe('RewardOptInAccountGroupModal', () => { const { getByTestId } = render(); - // Act const linkButton = getByTestId('link-account-group-button'); fireEvent.press(linkButton); - // Assert await waitFor(() => { expect(mockLinkAccountGroup).toHaveBeenCalled(); }); + + // Wait for the async state update to complete + await waitFor(() => { + expect( + getByTestId( + 'flat-list-item-0x0987654321098765432109876543210987654321-eip155:1', + ), + ).toBeOnTheScreen(); + }); }); - it('should handle link failure gracefully', async () => { - // Arrange + it('handles link failure gracefully', async () => { const consoleErrorSpy = jest .spyOn(console, 'error') .mockImplementation(() => { @@ -558,11 +555,9 @@ describe('RewardOptInAccountGroupModal', () => { const { getByTestId } = render(); - // Act const linkButton = getByTestId('link-account-group-button'); fireEvent.press(linkButton); - // Assert await waitFor(() => { expect(consoleErrorSpy).toHaveBeenCalledWith( 'Failed to link account group:', @@ -575,8 +570,7 @@ describe('RewardOptInAccountGroupModal', () => { }); describe('Network Resolution', () => { - it('should resolve non-EVM network names correctly', () => { - // Arrange + it('resolves non-EVM network names correctly', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -595,15 +589,16 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act const { getByTestId } = render(); - // Assert - expect(getByTestId('network-Bitcoin')).toBeOnTheScreen(); + expect( + getByTestId( + 'flat-list-item-bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh-bip122:000000000019d6689c085ae165831e93', + ), + ).toBeOnTheScreen(); }); - it('should handle unknown network scopes', () => { - // Arrange + it('handles unknown network scopes', () => { const consoleWarnSpy = jest .spyOn(console, 'warn') .mockImplementation(() => { @@ -626,10 +621,8 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act render(); - // Assert expect(consoleWarnSpy).toHaveBeenCalledWith( 'Unknown network for scope:', 'unknown:network', @@ -637,11 +630,37 @@ describe('RewardOptInAccountGroupModal', () => { consoleWarnSpy.mockRestore(); }); + + it('treats addresses with undefined isSupported as supported', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: ['eip155:1' as CaipChainId], + isSupported: undefined, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { getByTestId } = render(); + + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); + expect(getByTestId('link-account-group-button')).toBeOnTheScreen(); + }); }); describe('Wildcard Scope Handling', () => { - it('should expand eip155:* wildcard to all EVM networks', () => { - // Arrange + it('expands eip155:* wildcard to all EVM networks', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -658,15 +677,16 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act const { getByTestId } = render(); - // Assert - Should render Ethereum Mainnet, but not Goerli testnet - expect(getByTestId('network-Ethereum Mainnet')).toBeOnTheScreen(); + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); }); - it('should expand bip122:0 wildcard to all Bitcoin networks', () => { - // Arrange + it('expands bip122:0 wildcard to all Bitcoin networks', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -683,15 +703,16 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act const { getByTestId } = render(); - // Assert - expect(getByTestId('network-Bitcoin')).toBeOnTheScreen(); + expect( + getByTestId( + 'flat-list-item-bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh-bip122:000000000019d6689c085ae165831e93', + ), + ).toBeOnTheScreen(); }); - it('should filter out testnets when expanding EVM wildcards', () => { - // Arrange + it('filters out testnets when expanding EVM wildcards', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -708,15 +729,17 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act const { queryByTestId } = render(); - // Assert - Goerli testnet should not be rendered - expect(queryByTestId('network-Goerli Testnet')).toBeNull(); + // Goerli testnet should be filtered out, so no items with that network should exist + expect( + queryByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:5', + ), + ).toBeNull(); }); - it('should handle invalid CAIP scope format', () => { - // Arrange + it('handles invalid CAIP scope format', () => { const consoleWarnSpy = jest .spyOn(console, 'warn') .mockImplementation(() => { @@ -739,10 +762,8 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act render(); - // Assert expect(consoleWarnSpy).toHaveBeenCalledWith( 'Unknown network for scope:', 'invalid', @@ -750,22 +771,64 @@ describe('RewardOptInAccountGroupModal', () => { consoleWarnSpy.mockRestore(); }); + + it('handles wildcard scope with missing namespace', () => { + const consoleWarnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => { + // Suppress warning output in test + }); + + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: [':*' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + render(); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + 'Invalid CAIP scope format:', + ':*', + ); + + consoleWarnSpy.mockRestore(); + }); }); describe('Edge Cases', () => { - it('should handle missing account group context', () => { - // Arrange + it('handles missing account group context', () => { mockSelectAccountGroupById.mockReturnValue(undefined); - // Act const { queryByTestId } = render(); - // Assert - Header should not render without account group name expect(queryByTestId('bottom-sheet-header')).toBeNull(); }); - it('should handle EVM chain ID conversion errors', () => { - // Arrange + it('handles account group context without metadata name', () => { + mockSelectAccountGroupById.mockReturnValue({ + id: 'keyring:wallet-1/ethereum', + scopes: [], + keyringType: 'HD Key Tree', + metadata: {}, + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('bottom-sheet-header')).toBeNull(); + }); + + it('handles EVM chain ID conversion errors', () => { const consoleWarnSpy = jest .spyOn(console, 'warn') .mockImplementation(() => { @@ -786,10 +849,8 @@ describe('RewardOptInAccountGroupModal', () => { throw new Error('Invalid chain ID'); }); - // Act render(); - // Assert expect(consoleWarnSpy).toHaveBeenCalledWith( 'Invalid EVM chain ID:', 'invalid-chain-id', @@ -799,8 +860,7 @@ describe('RewardOptInAccountGroupModal', () => { consoleWarnSpy.mockRestore(); }); - it('should handle address items with valid scope', () => { - // Arrange + it('handles address items with valid scope', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -817,46 +877,247 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act const { getByTestId } = render(); - // Assert - Should render the address expect( - getByTestId('address-0x1234567890123456789012345678901234567890'), + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); + }); + + it('skips address items with missing address', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '', + hasOptedIn: false, + scopes: ['eip155:1' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('reward-opt-in-address-list')).toBeNull(); + }); + + it('skips address items with null address', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: null as unknown as string, + hasOptedIn: false, + scopes: ['eip155:1' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('reward-opt-in-address-list')).toBeNull(); + }); + + it('skips address items with empty scopes array', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: [], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('reward-opt-in-address-list')).toBeNull(); + }); + + it('skips address items with null scopes', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: null as unknown as string[], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { queryByTestId } = render(); + + expect(queryByTestId('reward-opt-in-address-list')).toBeNull(); + }); + + it('skips empty scope strings', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: ['', 'eip155:1' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { getByTestId } = render(); + + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); + }); + + it('skips whitespace-only scope strings', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: [' ', 'eip155:1' as CaipChainId], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { getByTestId } = render(); + + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); + }); + + it('handles address items with multiple scopes', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: false, + scopes: [ + 'eip155:1' as CaipChainId, + 'bip122:000000000019d6689c085ae165831e93' as CaipChainId, + ], + isSupported: true, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { getByTestId } = render(); + + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-eip155:1', + ), + ).toBeOnTheScreen(); + expect( + getByTestId( + 'flat-list-item-0x1234567890123456789012345678901234567890-bip122:000000000019d6689c085ae165831e93', + ), ).toBeOnTheScreen(); }); }); describe('FlatList Configuration', () => { - it('should render FlatList with correct testID', () => { + it('renders FlatList with correct testID', () => { const { getByTestId } = render(); - // Verify the FlatList is rendered expect(getByTestId('reward-opt-in-address-list')).toBeOnTheScreen(); }); - it('should have correct FlatList props for scrolling', () => { + it('has correct FlatList props for scrolling', () => { const { getByTestId } = render(); const flatList = getByTestId('reward-opt-in-address-list'); expect(flatList).toHaveProp('showsVerticalScrollIndicator', true); }); + + it('generates correct keys for header items', () => { + const { getByTestId } = render(); + + const flatList = getByTestId('reward-opt-in-address-list'); + const keyExtractor = flatList.props.keyExtractor; + + const headerItem = { type: 'header' as const, title: 'Test Header' }; + const key = keyExtractor(headerItem, 0); + + expect(key).toBe('header-Test Header-0'); + }); + + it('generates correct keys for address items', () => { + const { getByTestId } = render(); + + const flatList = getByTestId('reward-opt-in-address-list'); + const keyExtractor = flatList.props.keyExtractor; + + const addressItem = { + type: 'item' as const, + address: '0x123', + scope: 'eip155:1' as CaipChainId, + hasOptedIn: false, + networkName: 'Ethereum', + isSupported: true, + }; + const key = keyExtractor(addressItem, 1); + + expect(key).toBe('0x123-eip155:1-1'); + }); }); describe('Section Headers (Tracked/Untracked)', () => { - it('should show section headers when there are both tracked and untracked addresses', () => { - // Arrange - default route params has both tracked and untracked addresses + it('shows section headers when there are both tracked and untracked addresses', () => { const { getByText } = render(); - // Assert expect(getByText('rewards.link_account_group.tracked')).toBeOnTheScreen(); expect( getByText('rewards.link_account_group.untracked'), ).toBeOnTheScreen(); }); - it('should not show section headers when all addresses are tracked', () => { - // Arrange + it('shows tracked section header when all addresses are tracked', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -873,16 +1134,15 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act - const { queryByText } = render(); + const { getByText, queryByText } = render( + , + ); - // Assert - Headers should not be rendered when there's only one type - expect(queryByText('rewards.link_account_group.tracked')).toBeNull(); + expect(getByText('rewards.link_account_group.tracked')).toBeOnTheScreen(); expect(queryByText('rewards.link_account_group.untracked')).toBeNull(); }); - it('should not show section headers when all addresses are untracked', () => { - // Arrange + it('shows untracked section header when all addresses are untracked', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -899,16 +1159,17 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act - const { queryByText } = render(); + const { getByText, queryByText } = render( + , + ); - // Assert - Headers should not be rendered when there's only one type expect(queryByText('rewards.link_account_group.tracked')).toBeNull(); - expect(queryByText('rewards.link_account_group.untracked')).toBeNull(); + expect( + getByText('rewards.link_account_group.untracked'), + ).toBeOnTheScreen(); }); - it('should not show section headers for unsupported addresses', () => { - // Arrange + it('shows unsupported section header for unsupported addresses', () => { mockUseRoute.mockReturnValue({ params: { ...defaultRouteParams, @@ -925,17 +1186,46 @@ describe('RewardOptInAccountGroupModal', () => { name: 'RewardOptInAccountGroupModal', } as never); - // Act - const { queryByText } = render(); + const { getByText } = render(); - // Assert - Unsupported addresses should not be shown in the list - expect(queryByText('rewards.link_account_group.tracked')).toBeNull(); - expect(queryByText('rewards.link_account_group.untracked')).toBeNull(); + expect( + getByText('rewards.link_account_group.unsupported'), + ).toBeOnTheScreen(); + }); + + it('shows unsupported section header when unsupported addresses exist alongside supported ones', () => { + mockUseRoute.mockReturnValue({ + params: { + ...defaultRouteParams, + addressData: [ + { + address: '0x1234567890123456789012345678901234567890', + hasOptedIn: true, + scopes: ['eip155:1' as CaipChainId], + isSupported: true, + }, + { + address: '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + hasOptedIn: false, + scopes: ['eip155:1' as CaipChainId], + isSupported: false, + }, + ], + }, + key: 'test-route', + name: 'RewardOptInAccountGroupModal', + } as never); + + const { getByText } = render(); + + expect( + getByText('rewards.link_account_group.unsupported'), + ).toBeOnTheScreen(); }); }); describe('Accessibility', () => { - it('should have proper testIDs for all interactive elements', () => { + it('has proper testIDs for all interactive elements', () => { const { getByTestId } = render(); expect(getByTestId('bottom-sheet')).toBeOnTheScreen(); @@ -943,4 +1233,62 @@ describe('RewardOptInAccountGroupModal', () => { expect(getByTestId('link-account-group-button')).toBeOnTheScreen(); }); }); + + describe('RenderItem Function', () => { + it('renders header items correctly', () => { + const { getByTestId } = render(); + + const flatList = getByTestId('reward-opt-in-address-list'); + const renderItem = flatList.props.renderItem; + + const headerItem = { + item: { type: 'header' as const, title: 'Test Header' }, + }; + const headerComponent = renderItem(headerItem); + + expect(headerComponent).toBeTruthy(); + }); + + it('returns null for items without address', () => { + const { getByTestId } = render(); + + const flatList = getByTestId('reward-opt-in-address-list'); + const renderItem = flatList.props.renderItem; + + const invalidItem = { + item: { + type: 'item' as const, + address: '', + scope: 'eip155:1' as CaipChainId, + hasOptedIn: false, + networkName: 'Ethereum', + isSupported: true, + }, + }; + const result = renderItem(invalidItem); + + expect(result).toBeNull(); + }); + + it('returns null for items without scope', () => { + const { getByTestId } = render(); + + const flatList = getByTestId('reward-opt-in-address-list'); + const renderItem = flatList.props.renderItem; + + const invalidItem = { + item: { + type: 'item' as const, + address: '0x123', + scope: '' as CaipChainId, + hasOptedIn: false, + networkName: 'Ethereum', + isSupported: true, + }, + }; + const result = renderItem(invalidItem); + + expect(result).toBeNull(); + }); + }); }); diff --git a/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.tsx b/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.tsx index 6c7c808ca873..d38adebff441 100644 --- a/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.tsx +++ b/app/components/UI/Rewards/components/Settings/RewardOptInAccountGroupModal.tsx @@ -21,7 +21,6 @@ import { useSelector } from 'react-redux'; import { RootState } from '../../../../../reducers'; import { strings } from '../../../../../../locales/i18n'; import { useLinkAccountGroup } from '../../hooks/useLinkAccountGroup'; -import RewardsInfoBanner from '../RewardsInfoBanner'; import { selectEvmNetworkConfigurationsByChainId } from '../../../../../selectors/networkController'; import { selectNonEvmNetworkConfigurationsByChainId } from '../../../../../selectors/multichainNetworkController'; import { CaipChainId } from '@metamask/utils'; @@ -116,75 +115,139 @@ const RewardOptInAccountGroupModal: React.FC = () => { const flattenedAddressData = useMemo(() => { const flattened: FlattenedAddressItem[] = []; - addressData.forEach((item) => { - if (!item?.address || !Array.isArray(item.scopes)) { + const addFlattenedItem = ( + address: string, + hasOptedIn: boolean, + scope: CaipChainId, + networkName: string, + isSupported?: boolean, + ) => { + flattened.push({ + address, + hasOptedIn, + scope, + networkName, + isSupported, + }); + }; + + const processMatchingNetwork = ( + chainId: string, + network: { name: string; hexChainId?: string }, + namespace: string, + address: string, + hasOptedIn: boolean, + isSupported: boolean | undefined, + ) => { + const chainIdParts = chainId.split(':'); + if (chainIdParts.length < 2) { return; } - const updatedHasOptedIn = - localAccountStatuses[item.address] ?? item.hasOptedIn; + const chainIdNamespace = chainIdParts[0]; + if (chainIdNamespace !== namespace) { + return; + } - item.scopes.forEach((scope: string) => { - if (typeof scope !== 'string' || !scope.trim()) { - return; - } + // Skip testnets for EVM networks + if (network.hexChainId && isTestNet(network.hexChainId)) { + return; + } - const caipScope = scope as CaipChainId; + addFlattenedItem( + address, + hasOptedIn, + chainId as CaipChainId, + network.name, + isSupported, + ); + }; + + const processWildcardScope = ( + caipScope: CaipChainId, + address: string, + hasOptedIn: boolean, + isSupported: boolean | undefined, + ) => { + const scopeParts = caipScope.split(':'); + if (scopeParts.length < 2) { + console.warn('Invalid CAIP scope format:', caipScope); + return; + } - // Handle wildcard patterns (e.g., "eip155:*" or "bip122:0") - if (caipScope.includes(':*') || caipScope.endsWith(':0')) { - const scopeParts = caipScope.split(':'); - if (scopeParts.length < 2) { - console.warn('Invalid CAIP scope format:', caipScope); - return; - } + const namespace = scopeParts[0]; + if (!namespace) { + console.warn('Invalid CAIP scope format:', caipScope); + return; + } - const namespace = scopeParts[0]; - if (!namespace) { - return; - } + // Add all networks matching this namespace (excluding testnets) + Object.entries(allNetworks).forEach(([chainId, network]) => { + processMatchingNetwork( + chainId, + network, + namespace, + address, + hasOptedIn, + isSupported, + ); + }); + }; + + const processSpecificScope = ( + caipScope: CaipChainId, + address: string, + hasOptedIn: boolean, + isSupported: boolean | undefined, + ) => { + const network = allNetworks[caipScope]; + if (network) { + addFlattenedItem( + address, + hasOptedIn, + caipScope, + network.name, + isSupported, + ); + } else { + console.warn('Unknown network for scope:', caipScope); + } + }; + + const processScope = ( + scope: string, + address: string, + hasOptedIn: boolean, + isSupported: boolean | undefined, + ) => { + if (typeof scope !== 'string' || !scope.trim()) { + return; + } - // Add all networks matching this namespace (excluding testnets) - Object.entries(allNetworks).forEach(([chainId, network]) => { - const chainIdParts = chainId.split(':'); - if (chainIdParts.length < 2) { - return; - } - - const chainIdNamespace = chainIdParts[0]; - - if (chainIdNamespace === namespace) { - // Skip testnets for EVM networks - if (network.hexChainId && isTestNet(network.hexChainId)) { - return; - } - - flattened.push({ - address: item.address, - hasOptedIn: updatedHasOptedIn, - scope: chainId as CaipChainId, - networkName: network.name, - isSupported: item.isSupported, - }); - } - }); - } else { - // Specific network scope - const network = allNetworks[caipScope]; - if (network) { - flattened.push({ - address: item.address, - hasOptedIn: updatedHasOptedIn, - scope: caipScope, - networkName: network.name, - isSupported: item.isSupported, - }); - } else { - console.warn('Unknown network for scope:', caipScope); - } - } + const caipScope = scope as CaipChainId; + + // Handle wildcard patterns (e.g., "eip155:*" or "bip122:0") + if (caipScope.includes(':*') || caipScope.endsWith(':0')) { + processWildcardScope(caipScope, address, hasOptedIn, isSupported); + } else { + processSpecificScope(caipScope, address, hasOptedIn, isSupported); + } + }; + + const processAddressItem = (item: AddressItem) => { + if (!item?.address || !Array.isArray(item.scopes)) { + return; + } + + const updatedHasOptedIn = + localAccountStatuses[item.address] ?? item.hasOptedIn; + + item.scopes.forEach((scope: string) => { + processScope(scope, item.address, updatedHasOptedIn, item.isSupported); }); - }); + }; + + addressData.forEach(processAddressItem); return flattened; }, [addressData, localAccountStatuses, allNetworks]); @@ -205,13 +268,13 @@ const RewardOptInAccountGroupModal: React.FC = () => { const renderItem = useCallback( ({ - item, + item: itemCtx, }: { item: | { type: 'header'; title: string } | ({ type: 'item' } & FlattenedAddressItem); }) => { - if (item.type === 'header') { + if (itemCtx.type === 'header') { return ( { fontWeight={FontWeight.Medium} twClassName="text-alternative" > - {item.title} + {itemCtx.title} ); } - if (!item.address || !item.scope) { + if (!itemCtx.address || !itemCtx.scope) { return null; } return ( ); }, [], ); - const unsupportedAddresses = useMemo( - () => - flattenedAddressData?.filter((item) => item.isSupported === false) ?? [], - [flattenedAddressData], - ); - const canOptInAddresses = useMemo( () => flattenedAddressData?.filter( - (item) => item.isSupported === true && !item.hasOptedIn, + (item) => item.isSupported !== false && !item.hasOptedIn, ) ?? [], [flattenedAddressData], ); @@ -260,6 +317,9 @@ const RewardOptInAccountGroupModal: React.FC = () => { const supportedAddresses = flattenedAddressData.filter( (item) => item.isSupported !== false, ); + const unsupportedAddresses = flattenedAddressData.filter( + (item) => item.isSupported === false, + ); const trackedAddresses = supportedAddresses.filter( (item) => item.hasOptedIn, @@ -273,34 +333,36 @@ const RewardOptInAccountGroupModal: React.FC = () => { | ({ type: 'item' } & FlattenedAddressItem) )[] = []; - // Only show headers if there are both tracked and untracked addresses - const showHeaders = - trackedAddresses.length > 0 && untrackedAddresses.length > 0; - if (trackedAddresses.length > 0) { - if (showHeaders) { - data.push({ - type: 'header', - title: strings('rewards.link_account_group.tracked'), - }); - } + data.push({ + type: 'header', + title: strings('rewards.link_account_group.tracked'), + }); trackedAddresses.forEach((item) => { data.push({ type: 'item', ...item }); }); } if (untrackedAddresses.length > 0) { - if (showHeaders) { - data.push({ - type: 'header', - title: strings('rewards.link_account_group.untracked'), - }); - } + data.push({ + type: 'header', + title: strings('rewards.link_account_group.untracked'), + }); untrackedAddresses.forEach((item) => { data.push({ type: 'item', ...item }); }); } + if (unsupportedAddresses.length > 0) { + data.push({ + type: 'header', + title: strings('rewards.link_account_group.unsupported'), + }); + unsupportedAddresses.forEach((item) => { + data.push({ type: 'item', ...item }); + }); + } + return data; }, [flattenedAddressData]); @@ -318,20 +380,6 @@ const RewardOptInAccountGroupModal: React.FC = () => { )} - {unsupportedAddresses.length > 0 && ( - - - - )} - {flatListData.length > 0 && ( { Details: 'details', Check: 'check', Add: 'add', + Minus: 'minus', + }, + TextColor: { + TextAlternative: 'textAlternative', + TextDefault: 'textDefault', + TextMuted: 'textMuted', }, }; }); @@ -325,21 +331,10 @@ jest.mock('../../../../../component-library/components/Texts/Text', () => ({ })); // Mock selectors +const mockSelectIconSeedAddressByAccountGroupId = jest.fn(); jest.mock('../../../../../selectors/multichainAccounts/accounts', () => ({ - selectIconSeedAddressByAccountGroupId: jest.fn(), -})); - -jest.mock('../../../../../selectors/assets/balances', () => ({ - selectBalanceByAccountGroup: jest.fn(), -})); - -jest.mock('../../../../../selectors/preferencesController', () => ({ - selectPrivacyMode: jest.fn(), -})); - -// Mock utility functions -jest.mock('../../../../../util/assets', () => ({ - formatWithThreshold: jest.fn((value: number) => `$${value.toFixed(2)}`), + selectIconSeedAddressByAccountGroupId: (groupId: string) => + mockSelectIconSeedAddressByAccountGroupId(groupId), })); const mockUseSelector = useSelector as jest.MockedFunction; @@ -353,6 +348,7 @@ const mockUseLinkAccountGroup = useLinkAccountGroup as jest.MockedFunction< describe('RewardSettingsAccountGroup', () => { const mockNavigate = jest.fn(); const mockLinkAccountGroup = jest.fn(); + const mockEvmAddress = '0x1234567890123456789012345678901234567890'; const mockAccountGroup: AccountGroupWithOptInStatus = { id: 'keyring:wallet-1/ethereum', @@ -410,6 +406,11 @@ describe('RewardSettingsAccountGroup', () => { beforeEach(() => { jest.clearAllMocks(); + // Mock selectIconSeedAddressByAccountGroupId to return a selector function + mockSelectIconSeedAddressByAccountGroupId.mockReturnValue( + () => mockEvmAddress, + ); + // Mock useNavigation mockUseNavigation.mockReturnValue({ navigate: mockNavigate, @@ -430,28 +431,14 @@ describe('RewardSettingsAccountGroup', () => { isError: false, }); - // Mock useSelector calls + // Mock useSelector to execute the selector function mockUseSelector.mockImplementation((selector) => { - // Mock selectIconSeedAddressByAccountGroupId - if ( - selector.toString().includes('selectIconSeedAddressByAccountGroupId') - ) { - return '0x1234567890123456789012345678901234567890'; - } - - // Mock selectBalanceByAccountGroup - if (selector.toString().includes('selectBalanceByAccountGroup')) { - return { - totalBalanceInUserCurrency: 100.5, - userCurrency: 'usd', - }; + // The selector is a function that takes state and returns a value + // In the component, it calls selectIconSeedAddressByAccountGroupId(accountGroup.id) + // which returns a selector function, then calls that with state + if (typeof selector === 'function') { + return selector({} as never); } - - // Mock selectPrivacyMode - if (selector.toString().includes('selectPrivacyMode')) { - return false; - } - return null; }); }); @@ -496,7 +483,7 @@ describe('RewardSettingsAccountGroup', () => { ).toBeOnTheScreen(); }); - it('should render account group balance when available', () => { + it('should render tracked accounts count', () => { const { getByTestId } = render( { ); expect( - getByTestId(`rewards-account-group-balance-${mockAccountGroup.id}`), + getByTestId(`rewards-account-group-tracked-${mockAccountGroup.id}`), ).toBeOnTheScreen(); }); @@ -546,7 +533,27 @@ describe('RewardSettingsAccountGroup', () => { expect(getByTestId('icon-check')).toBeOnTheScreen(); }); - it('should show add icon when there are opted out accounts', () => { + it('should show add icon when there are only opted out accounts', () => { + const itemWithOnlyOptedOut: RewardSettingsAccountGroupListFlatListItem = { + type: 'accountGroup', + accountGroup: { + ...mockAccountGroup, + optedInAccounts: [], + optedOutAccounts: mockAccountGroup.optedOutAccounts, + }, + }; + + const { getByTestId } = render( + , + ); + + expect(getByTestId('icon-add')).toBeOnTheScreen(); + }); + + it('should show add icon when there are both opted in and opted out accounts', () => { const { getByTestId } = render( { ).toBeNull(); }); - it('should not render balance when balance data is unavailable', () => { - mockUseSelector.mockImplementation((selector) => { - if (selector.toString().includes('selectBalanceByAccountGroup')) { - return null; - } - if ( - selector.toString().includes('selectIconSeedAddressByAccountGroupId') - ) { - return '0x1234567890123456789012345678901234567890'; - } - if (selector.toString().includes('selectPrivacyMode')) { - return false; - } - return null; + it('should use fallback address when evmAddress is undefined', () => { + // Mock selector to throw an error (simulating no accounts found) + mockSelectIconSeedAddressByAccountGroupId.mockReturnValue(() => { + throw new Error('No accounts found'); }); - const { queryByTestId } = render( + const { getByTestId } = render( , ); + // Component should still render with fallback address expect( - queryByTestId(`rewards-account-group-balance-${mockAccountGroup.id}`), - ).toBeNull(); + getByTestId(`rewards-account-group-avatar-${mockAccountGroup.id}`), + ).toBeOnTheScreen(); }); - it('should not render balance when totalBalanceInUserCurrency is zero', () => { - mockUseSelector.mockImplementation((selector) => { - if (selector.toString().includes('selectBalanceByAccountGroup')) { - return { - totalBalanceInUserCurrency: 0, - userCurrency: 'usd', - }; - } - if ( - selector.toString().includes('selectIconSeedAddressByAccountGroupId') - ) { - return '0x1234567890123456789012345678901234567890'; - } - if (selector.toString().includes('selectPrivacyMode')) { - return false; - } - return null; - }); + it('should use fallback address when selector returns undefined', () => { + // Mock selector to return undefined + mockSelectIconSeedAddressByAccountGroupId.mockReturnValue( + () => undefined, + ); - const { queryByTestId } = render( + const { getByTestId } = render( , ); + // Component should still render with fallback address expect( - queryByTestId(`rewards-account-group-balance-${mockAccountGroup.id}`), - ).toBeNull(); + getByTestId(`rewards-account-group-avatar-${mockAccountGroup.id}`), + ).toBeOnTheScreen(); }); - it('should use fallback address when evmAddress is undefined', () => { - mockUseSelector.mockImplementation((selector) => { - if ( - selector.toString().includes('selectIconSeedAddressByAccountGroupId') - ) { - return undefined; - } - if (selector.toString().includes('selectBalanceByAccountGroup')) { - return { - totalBalanceInUserCurrency: 100.5, - userCurrency: 'usd', - }; - } - if (selector.toString().includes('selectPrivacyMode')) { - return false; - } - return null; - }); - - const { getByTestId } = render( + it('should call selectIconSeedAddressByAccountGroupId with correct account group ID', () => { + render( , ); - // Component should still render with fallback address - expect( - getByTestId(`rewards-account-group-avatar-${mockAccountGroup.id}`), - ).toBeOnTheScreen(); + // Verify the selector factory was called with the correct account group ID + expect(mockSelectIconSeedAddressByAccountGroupId).toHaveBeenCalledWith( + mockAccountGroup.id, + ); }); - it('should hide balance when privacy mode is enabled', () => { - mockUseSelector.mockImplementation((selector) => { - if (selector.toString().includes('selectBalanceByAccountGroup')) { - return { - totalBalanceInUserCurrency: 100.5, - userCurrency: 'usd', - }; - } - if ( - selector.toString().includes('selectIconSeedAddressByAccountGroupId') - ) { - return '0x1234567890123456789012345678901234567890'; - } - if (selector.toString().includes('selectPrivacyMode')) { - return true; - } - return null; - }); + it('should not call selector when accountGroup.id is undefined', () => { + const itemWithoutId: RewardSettingsAccountGroupListFlatListItem = { + type: 'accountGroup', + accountGroup: { + ...mockAccountGroup, + id: undefined as never, + }, + }; - const { getByTestId } = render( + render( , ); - // Balance should render but be hidden - const balance = getByTestId( - `rewards-account-group-balance-${mockAccountGroup.id}`, - ); - expect(balance).toBeOnTheScreen(); + // Verify the selector was not called when accountGroup.id is undefined + expect(mockSelectIconSeedAddressByAccountGroupId).not.toHaveBeenCalled(); }); }); @@ -926,7 +881,7 @@ describe('RewardSettingsAccountGroup', () => { getByTestId(`rewards-account-group-name-${mockAccountGroup.id}`), ).toBeOnTheScreen(); expect( - getByTestId(`rewards-account-group-balance-${mockAccountGroup.id}`), + getByTestId(`rewards-account-group-tracked-${mockAccountGroup.id}`), ).toBeOnTheScreen(); expect( getByTestId(`rewards-account-addresses-${mockAccountGroup.id}`), diff --git a/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroup.tsx b/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroup.tsx index a36792f63a7f..195086a49b7a 100644 --- a/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroup.tsx +++ b/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroup.tsx @@ -1,4 +1,4 @@ -import React, { memo, useCallback, useMemo } from 'react'; +import React, { memo, useCallback } from 'react'; import { Box, Text, @@ -9,10 +9,11 @@ import { ButtonIconSize, TextVariant as DsTextVariant, IconName as IconNameDS, + TextColor as DsTextColor, } from '@metamask/design-system-react-native'; import { useTailwind } from '@metamask/design-system-twrnc-preset'; -import I18n from '../../../../../../locales/i18n'; import { isEmpty } from 'lodash'; +import { strings } from '../../../../../../locales/i18n'; import AvatarAccount, { AvatarAccountType, } from '../../../../../component-library/components/Avatars/Avatar/variants/AvatarAccount'; @@ -24,16 +25,6 @@ import { RewardSettingsAccountGroupListFlatListItem } from './types'; import { useSelector } from 'react-redux'; import { RootState } from '../../../../../reducers'; import { selectIconSeedAddressByAccountGroupId } from '../../../../../selectors/multichainAccounts/accounts'; -import { selectBalanceByAccountGroup } from '../../../../../selectors/assets/balances'; -import { selectPrivacyMode } from '../../../../../selectors/preferencesController'; -import SensitiveText, { - SensitiveTextLength, -} from '../../../../../component-library/components/Texts/SensitiveText'; -import { - TextColor, - TextVariant, -} from '../../../../../component-library/components/Texts/Text'; -import { formatWithThreshold } from '../../../../../util/assets'; import { View, ActivityIndicator } from 'react-native'; import { useTheme } from '../../../../../util/theme'; @@ -69,29 +60,6 @@ const RewardSettingsAccountGroup: React.FC = ({ const { linkAccountGroup, isLoading } = useLinkAccountGroup(); const navigation = useNavigation(); - const privacyMode = useSelector(selectPrivacyMode); - - // Get account group balance - const groupBalance = useSelector((state: RootState) => { - if (!accountGroup?.id) return null; - const selector = selectBalanceByAccountGroup(accountGroup.id); - return selector(state); - }); - - const displayBalance = useMemo(() => { - if (!groupBalance?.userCurrency) { - return undefined; - } - return formatWithThreshold( - groupBalance.totalBalanceInUserCurrency, - 0.01, - I18n.locale, - { - style: 'currency', - currency: groupBalance.userCurrency.toUpperCase(), - }, - ); - }, [groupBalance]); // Inline handlers for each account group const handleLinkAccountGroup = useCallback(async () => { @@ -149,10 +117,21 @@ const RewardSettingsAccountGroup: React.FC = ({ return null; } + // Calculate tracked accounts count + const optedInCount = accountGroup.optedInAccounts.length; + const totalTrackableCount = + accountGroup.optedInAccounts.length + accountGroup.optedOutAccounts.length; + + // Determine icon name for link button + const linkButtonIconName = + accountGroup.optedOutAccounts.length === 0 + ? IconNameDS.Check + : IconNameDS.Add; + return ( = ({ {accountGroup.name} - {/* Account Balance */} - {displayBalance && - groupBalance?.totalBalanceInUserCurrency !== undefined && - groupBalance.totalBalanceInUserCurrency > 0 && ( - - {displayBalance} - - )} + {/* Tracked Accounts Count */} + + {strings('rewards.link_account_group.tracked_count', { + optedIn: optedInCount.toString(), + total: totalTrackableCount.toString(), + })} + = ({ ) : ( { data, renderItem, keyExtractor, + getItemType, ListHeaderComponent, ListEmptyComponent, ListFooterComponent, @@ -67,6 +68,7 @@ jest.mock('@shopify/flash-list', () => { index: number; }) => React.ReactElement; keyExtractor: (item: unknown, index: number) => string; + getItemType?: (item: unknown) => string; ListHeaderComponent?: () => React.ReactElement; ListEmptyComponent?: () => React.ReactElement; ListFooterComponent?: () => React.ReactElement; @@ -84,9 +86,15 @@ jest.mock('@shopify/flash-list', () => { data && Array.isArray(data) && data.length > 0 ? data.map((item: unknown, index: number) => { const key = keyExtractor ? keyExtractor(item, index) : index; + const itemType = getItemType ? getItemType(item) : undefined; return ReactActual.createElement( View, - { key, testID: `flash-list-item-${key}` }, + { + key, + testID: `flash-list-item-${key}`, + // Store item type as accessibility label for testing + accessibilityLabel: itemType, + }, renderItem({ item, index, @@ -140,31 +148,37 @@ jest.mock('@metamask/design-system-react-native', () => { onPress, testID, disabled, + isDisabled, ...props }: { children?: React.ReactNode; onPress?: () => void; testID?: string; disabled?: boolean; + isDisabled?: boolean; [key: string]: unknown; - }) => - ReactActual.createElement( + }) => { + const isButtonDisabled = disabled || isDisabled; + return ReactActual.createElement( TouchableOpacity, { onPress, testID, - disabled, + disabled: isButtonDisabled, ...props, }, ReactActual.createElement( RNText, { - disabled, - accessibilityState: disabled ? { disabled: true } : undefined, + disabled: isButtonDisabled, + accessibilityState: isButtonDisabled + ? { disabled: true } + : undefined, }, children, ), - ), + ); + }, TextVariant: { BodyMd: 'BodyMd', BodySm: 'BodySm', @@ -366,6 +380,7 @@ describe('RewardSettingsAccountGroupList', () => { jest.clearAllMocks(); // Mock selectInternalAccountsByGroupId to return accounts for each group + // This selector returns a function that takes state and returns accounts mockSelectInternalAccountsByGroupId.mockImplementation( (groupId: string) => { const mockAccounts: Record = { @@ -379,20 +394,33 @@ describe('RewardSettingsAccountGroupList', () => { { address: '0x1111111111111111111111111111111111111111' }, ], }; - return mockAccounts[groupId] || []; + // Return a selector function that returns the accounts for this group + return () => mockAccounts[groupId] || []; }, ); // Mock useSelector calls - mockUseSelector.mockImplementation((selector) => { + mockUseSelector.mockImplementation((selector: unknown) => { // Mock selectAvatarAccountType selector if (selector === selectAvatarAccountType) { return 'default'; } - // For the allAddresses selector, let it execute normally since we've mocked selectInternalAccountsByGroupId - // The selector will now work correctly with our mocked data - return selector({}); + // For the allAddresses selector, it will call selectInternalAccountsByGroupId + // for each group and build the addresses object + // We need to handle this by executing the selector with a mock state + if (typeof selector === 'function') { + try { + const mockState = {} as never; + return (selector as (state: never) => unknown)(mockState); + } catch { + // If selector fails, return empty object + return {}; + } + } + + // Fallback for unknown selector types + return undefined; }); // Mock useRewardOptinSummary hook @@ -739,6 +767,70 @@ describe('RewardSettingsAccountGroupList', () => { }); }); + describe('getItemType', () => { + it('should return correct item type for wallet items', () => { + const { getByTestId } = render(); + + // The FlashList mock stores item type in accessibilityLabel when getItemType is provided + const walletItem1 = getByTestId('flash-list-item-wallet-wallet-1'); + const walletItem2 = getByTestId('flash-list-item-wallet-wallet-2'); + + expect(walletItem1).toBeOnTheScreen(); + expect(walletItem1.props.accessibilityLabel).toBe('wallet'); + expect(walletItem2).toBeOnTheScreen(); + expect(walletItem2.props.accessibilityLabel).toBe('wallet'); + }); + + it('should return correct item type for account group items', () => { + const { getByTestId } = render(); + + const accountGroup1 = getByTestId('flash-list-item-accountGroup-group-1'); + const accountGroup2 = getByTestId('flash-list-item-accountGroup-group-2'); + const accountGroup3 = getByTestId('flash-list-item-accountGroup-group-3'); + + expect(accountGroup1).toBeOnTheScreen(); + expect(accountGroup1.props.accessibilityLabel).toBe('accountGroup'); + expect(accountGroup2).toBeOnTheScreen(); + expect(accountGroup2.props.accessibilityLabel).toBe('accountGroup'); + expect(accountGroup3).toBeOnTheScreen(); + expect(accountGroup3.props.accessibilityLabel).toBe('accountGroup'); + }); + }); + + describe('allAddresses Data', () => { + it('should pass allAddresses to account group items', () => { + const { getByTestId } = render(); + + // Verify account groups are rendered (they receive allAddresses as prop) + expect(getByTestId('account-group-group-1')).toBeOnTheScreen(); + expect(getByTestId('account-group-group-2')).toBeOnTheScreen(); + expect(getByTestId('account-group-group-3')).toBeOnTheScreen(); + }); + + it('should handle empty addresses for account groups', () => { + // Mock selector to return empty array for a group + mockSelectInternalAccountsByGroupId.mockImplementation( + (groupId: string) => { + const mockAccounts: Record = { + 'group-1': [], + 'group-2': [ + { address: '0x0987654321098765432109876543210987654321' }, + ], + 'group-3': [ + { address: '0x1111111111111111111111111111111111111111' }, + ], + }; + return () => mockAccounts[groupId] || []; + }, + ); + + const { getByTestId } = render(); + + // Should still render even with empty addresses + expect(getByTestId('account-group-group-1')).toBeOnTheScreen(); + }); + }); + describe('Memoization', () => { it('should memoize the component to prevent unnecessary re-renders', () => { const { rerender } = render(); diff --git a/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroupList.tsx b/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroupList.tsx index 2902f225dd4a..715fa41292b7 100644 --- a/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroupList.tsx +++ b/app/components/UI/Rewards/components/Settings/RewardSettingsAccountGroupList.tsx @@ -17,6 +17,7 @@ import { Skeleton } from '../../../../../component-library/components/Skeleton'; import { useRewardOptinSummary } from '../../hooks/useRewardOptinSummary'; import { selectAvatarAccountType } from '../../../../../selectors/settings'; import { selectInternalAccountsByGroupId } from '../../../../../selectors/multichainAccounts/accounts'; +import { AccountGroupId } from '@metamask/account-api'; import Button, { ButtonVariants, } from '../../../../../component-library/components/Buttons/Button'; @@ -43,23 +44,28 @@ const RewardSettingsAccountGroupList: React.FC = () => { refresh: fetchOptInStatus, } = useRewardOptinSummary(); + // Helper function to extract addresses for a single account group + const getAddressesForGroup = useCallback( + (state: RootState, groupId: AccountGroupId): string[] => { + try { + const accounts = selectInternalAccountsByGroupId(state)(groupId); + return accounts.map((account) => account.address).filter(Boolean); + } catch { + return []; + } + }, + [], + ); + // Memoize expensive selector operations to prevent unnecessary re-renders const allAddresses = useSelector((state: RootState) => { const addresses: Record = {}; - byWallet.forEach((walletItem) => { - walletItem.groups.forEach((accountGroup) => { - try { - const accounts = selectInternalAccountsByGroupId(state)( - accountGroup.id, - ); - addresses[accountGroup.id] = accounts - .map((account) => account.address) - .filter(Boolean); - } catch (error) { - addresses[accountGroup.id] = []; - } - }); + const allGroups = byWallet.flatMap((walletItem) => walletItem.groups); + + allGroups.forEach((accountGroup) => { + addresses[accountGroup.id] = getAddressesForGroup(state, accountGroup.id); }); + return addresses; }); @@ -128,7 +134,7 @@ const RewardSettingsAccountGroupList: React.FC = () => { const ListHeaderComponent = useCallback( () => ( - + {strings('rewards.settings.subtitle')} @@ -214,7 +220,7 @@ const RewardSettingsAccountGroupList: React.FC = () => { {[...Array(3)].map((_, index) => ( diff --git a/locales/languages/en.json b/locales/languages/en.json index af5d3310a46d..e1c192593999 100644 --- a/locales/languages/en.json +++ b/locales/languages/en.json @@ -6707,6 +6707,8 @@ "link_account": "Add account", "tracked": "Tracked", "untracked": "Untracked", + "unsupported": "Unsupported", + "tracked_count": "{{optedIn}}/{{total}} tracked", "link_account_success": "{{accountName}} added successfully", "link_account_error": "Failed to add one or more addresses for {{accountName}}" }, From 87bfd924b29c03861b3140993a47c1d31e2f860f Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Sat, 8 Nov 2025 00:30:03 +0800 Subject: [PATCH 4/9] fix: btc account selection during btc network change cp-7.59.0 (#22328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the issue when the user selects a BTC network, it would switch the user to the first multichain account. ## **Changelog** CHANGELOG entry: Changing networks to btc does not switch the selected account. ## **Related issues** Fixes: https://github.com/MetaMask/metamask-mobile/issues/22236#event-20790225755 ## **Manual testing steps** ```gherkin Feature: network switching Scenario: user wants to change the network filter Given the user is using a multichain account When user selected the btc network Then the account does not change. ``` ## **Screenshots/Recordings** ### **Before** ### **After** https://github.com/user-attachments/assets/ee43b688-8f64-4d16-bde9-b7efecef6cfe ## **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** - [ ] 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. --- > [!NOTE] > Stops auto-switching to a BTC account when toggling a popular BTC network; BTC address selection now only occurs during custom BTC network selection. > > - **Hooks (`useNetworkSelection.ts`)**: > - Remove Bitcoin-specific `Engine.setSelectedAddress` logic from `selectPopularNetwork`; keep BTC address handling only in `selectCustomNetwork`. > - Simplify `selectPopularNetwork` dependencies to exclude Bitcoin internals. > - **Tests (`useNetworkSelection.test.ts`)**: > - Delete test asserting BTC address is set on `selectPopularNetwork` for Bitcoin networks. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit b4746e4f5f39cc537bd66f8dd8a6e853b63a0cbc. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../useNetworkSelection.test.ts | 15 -------------- .../useNetworkSelection.ts | 20 +------------------ 2 files changed, 1 insertion(+), 34 deletions(-) diff --git a/app/components/hooks/useNetworkSelection/useNetworkSelection.test.ts b/app/components/hooks/useNetworkSelection/useNetworkSelection.test.ts index 7a5a599fa278..ffbe93eea7c6 100644 --- a/app/components/hooks/useNetworkSelection/useNetworkSelection.test.ts +++ b/app/components/hooks/useNetworkSelection/useNetworkSelection.test.ts @@ -1308,21 +1308,6 @@ describe('useNetworkSelection', () => { ); expect(mockEnableNetwork).toHaveBeenCalledWith(bitcoinMainnet); }); - - it('selectPopularNetwork sets selected address for Bitcoin networks', async () => { - const setSelectedAddressSpy = jest.spyOn(Engine, 'setSelectedAddress'); - - const { result } = renderHook(() => - useNetworkSelection({ networks: mockNetworks }), - ); - - await result.current.selectPopularNetwork(bitcoinMainnet); - - expect(setSelectedAddressSpy).toHaveBeenCalledWith( - mockBitcoinAccount.address, - ); - expect(mockEnableNetwork).toHaveBeenCalledWith(bitcoinMainnet); - }); }); describe('error handling and edge cases', () => { diff --git a/app/components/hooks/useNetworkSelection/useNetworkSelection.ts b/app/components/hooks/useNetworkSelection/useNetworkSelection.ts index 104e3318c7f7..ab466ab01a09 100644 --- a/app/components/hooks/useNetworkSelection/useNetworkSelection.ts +++ b/app/components/hooks/useNetworkSelection/useNetworkSelection.ts @@ -205,18 +205,6 @@ export const useNetworkSelection = ({ /** Toggles a popular network and resets all custom networks */ const selectPopularNetwork = useCallback( async (chainId: CaipChainId, onComplete?: () => void) => { - ///: BEGIN:ONLY_INCLUDE_IF(bitcoin) - if (chainId.includes(KnownCaipNamespace.Bip122)) { - const bitcoAccountInScope = bitcoinInternalAccounts.find((account) => - account.scopes.includes(chainId), - ); - - if (bitcoAccountInScope) { - Engine.setSelectedAddress(bitcoAccountInScope.address); - } - } - ///: END:ONLY_INCLUDE_IF - // Enable the network in NetworkEnablementController await enableNetwork(chainId); @@ -225,13 +213,7 @@ export const useNetworkSelection = ({ onComplete?.(); }, - [ - enableNetwork, - switchActiveNetwork, - ///: BEGIN:ONLY_INCLUDE_IF(bitcoin) - bitcoinInternalAccounts, - ///: END:ONLY_INCLUDE_IF(bitcoin) - ], + [enableNetwork, switchActiveNetwork], ); /** Selects a network, automatically handling popular vs custom logic */ From 9186674c9559d239148160a18a554da722def5af Mon Sep 17 00:00:00 2001 From: Nicholas Smith Date: Fri, 7 Nov 2025 10:32:07 -0600 Subject: [PATCH 5/9] fix: cp-7.59.0 hotfix-7.58.2 update close position calculation with funding fees and live data (#22229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fixes bug where close position view showed incorrect receive amounts, sometimes displaying negative values when position value had dropped. Root causes: 1. Used stale position data from route params instead of live WebSocket updates 2. HyperLiquid's marginUsed already includes PnL, but code was double-counting 3. Recalculated PnL from prices, which missed accumulated funding fees 4. Timing mismatch between price updates and position updates Changes: - Add usePerpsLivePositions to subscribe to real-time position data - Use livePosition.marginUsed which already includes accumulated PnL and funding fees - Use livePosition.positionValue for fee calculations to keep margin and fees synchronized - Remove effectivePnL recalculation that missed funding fees - Round margin and fees separately before subtraction for transparent calculation - Update tests to reflect that marginUsed includes PnL from HyperLiquid Result: - Position card and close position view now show matching margin values - Funding fees correctly included in all calculations - No more incorrect negative receive amounts - Calculation transparency: displayed margin - displayed fees = displayed receive amount ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/TAT-1956 ## **Manual testing steps** ```gherkin Feature: Close Position Calculation with Funding Fees Scenario: Close position with accumulated funding fees shows correct receive amount Given I have an open position with the following details: | Coin | ETH | | Margin Used | $100 | | Unrealized PnL| -$5 | | Entry Price | $2000 | | Current Price | $2000 | When I navigate to the close position view And I view the close position summary Then the margin displayed should be "$100" And the receive amount should match "margin - fees" And the receive amount should be positive And the receive amount should not be negative or zero Scenario: Receive amount calculation matches visual breakdown Given I have an open position When I view the close position summary Then the displayed margin should be rounded to 2 decimals And the displayed fees should be rounded to 2 decimals And the receive amount should equal "rounded margin minus rounded fees" And the calculation should be transparent to the user ``` ## **Screenshots/Recordings** ### **Before** ### **After** https://github.com/user-attachments/assets/29289663-7b77-45f7-8ff6-c48917e1e21a ## **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** - [ ] 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. --- > [!NOTE] > Close Position view now uses live position data (incl. funding fees) and computes receive amount as rounded margin minus rounded fees, with updated PnL/effective price logic and tests. > > - **Perps Close Position (UI)**: > - Subscribe to `usePerpsLivePositions` and derive `isLong`, `absSize`, `marginUsed`, and `unrealizedPnl` from live data. > - Rework P&L logic: `effectivePnL` uses live `pnl` for `market` orders; for `limit`, uses `limitPrice` (fallback `currentPrice`). > - Compute `positionValue` and `closingValue` with current/limit price in deps; remove ref-based price shortcuts. > - Recalculate `receiveAmount` as `(close% of marginUsed) - fees` with per-component rounding; update `Summary` `totalMargin` to `(close% of marginUsed)`. > - Pass `livePosition` to `handleClosePosition`; include updated `receivedAmount` and `realizedPnl` in params. > - Derive `unrealizedPnlPercent` by back-calculating initial margin from `marginUsed - pnl`. > - **Tests**: > - Mock `usePerpsLivePositions` and update expectations: receive = `marginUsed - fees`; PnL display uses live `unrealizedPnl`. > - Add price update sync test ensuring P&L/margin react to live price/position changes. > - Adjust confirmation assertions (e.g., `receivedAmount: 1405`) and various limit/market calculation cases. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5c3d3be8a0d7ca13363dd74552105b497612471f. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --------- Co-authored-by: abretonc7s <107169956+abretonc7s@users.noreply.github.com> --- .../PerpsClosePositionView.test.tsx | 143 +++++++++++++++--- .../PerpsClosePositionView.tsx | 86 ++++++----- 2 files changed, 171 insertions(+), 58 deletions(-) diff --git a/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.test.tsx b/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.test.tsx index 7ce0f0a7a2e9..6aa1fd71287d 100644 --- a/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.test.tsx +++ b/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.test.tsx @@ -51,6 +51,7 @@ jest.mock('../../hooks', () => ({ })); jest.mock('../../hooks/stream', () => ({ + usePerpsLivePositions: jest.fn(), usePerpsLivePrices: jest.fn(), usePerpsTopOfBook: jest.fn(), })); @@ -119,6 +120,9 @@ describe('PerpsClosePositionView', () => { const useRouteMock = jest.mocked( jest.requireMock('@react-navigation/native').useRoute, ); + const usePerpsLivePositionsMock = jest.mocked( + jest.requireMock('../../hooks/stream').usePerpsLivePositions, + ); const usePerpsLivePricesMock = jest.mocked( jest.requireMock('../../hooks/stream').usePerpsLivePrices, ); @@ -166,6 +170,10 @@ describe('PerpsClosePositionView', () => { }); // Setup hook mocks with default values + usePerpsLivePositionsMock.mockReturnValue({ + positions: [defaultPerpsPositionMock], + isInitialLoading: false, + }); usePerpsLivePricesMock.mockReturnValue(defaultPerpsLivePricesMock); usePerpsTopOfBookMock.mockReturnValue(defaultPerpsTopOfBookMock); usePerpsOrderFeesMock.mockReturnValue(defaultPerpsOrderFeesMock); @@ -446,14 +454,14 @@ describe('PerpsClosePositionView', () => { expect(usePerpsOrderFeesMock).toHaveBeenCalled(); }); - // Test that receiveAmount = (initial margin + effective P&L) - fees + // Test that receiveAmount uses marginUsed directly (which already includes PnL) it('calculates receive amount including P&L at effective price', () => { // Arrange const mockPosition = { ...defaultPerpsPositionMock, entryPrice: '100', // Entry at $100 - marginUsed: '1000', // $1000 initial margin - unrealizedPnl: '200', // Current unrealized P&L (not used in new calc) + marginUsed: '1200', // $1200 (includes $1000 initial + $200 unrealized PnL) + unrealizedPnl: '200', // Current unrealized P&L from HyperLiquid size: '1', // 1 token long position }; const mockFees = { @@ -462,7 +470,7 @@ describe('PerpsClosePositionView', () => { protocolFeeRate: 0.5, }; - // Set current price to $150 for clear P&L calculation + // Set current price to $150 for reference usePerpsLivePricesMock.mockReturnValue({ ETH: { price: '150' }, // Current price $150 }); @@ -471,6 +479,10 @@ describe('PerpsClosePositionView', () => { params: { position: mockPosition }, }); usePerpsOrderFeesMock.mockReturnValue(mockFees); + usePerpsLivePositionsMock.mockReturnValue({ + positions: [mockPosition], + isInitialLoading: false, + }); // Act const { getByText } = renderWithProvider( @@ -481,16 +493,15 @@ describe('PerpsClosePositionView', () => { true, ); - // Assert - receiveAmount = initialMargin + P&L - fees (P&L now included in calculation) - // P&L = (150 - 100) * 1 = 50 - // receiveAmount = 1000 + 50 - 50 = 1000 + // Assert - receiveAmount = marginUsed - fees + // HyperLiquid's marginUsed already includes PnL + // receiveAmount = 1200 - 50 = 1150 const receiveText = getByText( strings('perps.close_position.you_receive'), ); expect(receiveText).toBeDefined(); - // Look for 1000 in the display (margin + P&L - fees) // PRICE_RANGES_MINIMAL_VIEW: Fixed 2 decimals, trailing zeros removed - expect(getByText('$1,000')).toBeDefined(); + expect(getByText('$1,150')).toBeDefined(); }); it('calculates receive amount correctly for partial close percentages', () => { @@ -498,8 +509,8 @@ describe('PerpsClosePositionView', () => { const mockPosition = { ...defaultPerpsPositionMock, entryPrice: '100', // Entry at $100 - marginUsed: '2000', // $2000 initial margin - unrealizedPnl: '-300', // Current unrealized (not used in new calc) + marginUsed: '1700', // $1700 (includes $2000 initial - $300 unrealized loss) + unrealizedPnl: '-300', // Current unrealized loss from HyperLiquid size: '2', // 2 tokens long }; const mockFees = { @@ -517,6 +528,10 @@ describe('PerpsClosePositionView', () => { params: { position: mockPosition }, }); usePerpsOrderFeesMock.mockReturnValue(mockFees); + usePerpsLivePositionsMock.mockReturnValue({ + positions: [mockPosition], + isInitialLoading: false, + }); // Act const { getByText } = renderWithProvider( @@ -527,15 +542,15 @@ describe('PerpsClosePositionView', () => { true, ); - // For 100% close (default) with new calculation: - // P&L = (75 - 100) * 2 = -50 (loss) - // receiveAmount = 2000 + (-50) - 25 = 1925 + // For 100% close (default): + // HyperLiquid's marginUsed already includes PnL + // receiveAmount = 1700 - 25 = 1675 const receiveText = getByText( strings('perps.close_position.you_receive'), ); expect(receiveText).toBeDefined(); - // Look for 1925 in the display (margin + P&L - fees) - expect(getByText(/1,925/)).toBeDefined(); + // Look for 1675 in the display + expect(getByText(/1,675/)).toBeDefined(); }); }); @@ -632,8 +647,9 @@ describe('PerpsClosePositionView', () => { const positionWithLoss = { ...defaultPerpsPositionMock, entryPrice: '150', // Entry at $150 + marginUsed: '1350', // $1500 initial - $150 unrealized loss (includes funding) size: '1', // 1 token long - unrealizedPnl: '-100', // Current unrealized (not used for display) + unrealizedPnl: '-150', // Current unrealized loss including funding fees }; // Set current price lower than entry for loss @@ -644,6 +660,10 @@ describe('PerpsClosePositionView', () => { useRouteMock.mockReturnValue({ params: { position: positionWithLoss }, }); + usePerpsLivePositionsMock.mockReturnValue({ + positions: [positionWithLoss], + isInitialLoading: false, + }); // Act const { getByText } = renderWithProvider( @@ -654,9 +674,9 @@ describe('PerpsClosePositionView', () => { true, ); - // Assert - effectivePnL = (100 - 150) * 1 = -50 - // Look for negative P&L display (with - sign) - should show 50 (absolute value) - const pnlElement = getByText(/-.*50/); + // Assert - Now uses actual unrealizedPnl from position + // Look for negative P&L display (with - sign) - should show 150 (absolute value) + const pnlElement = getByText(/-.*150/); expect(pnlElement).toBeDefined(); }); }); @@ -2049,9 +2069,9 @@ describe('PerpsClosePositionView', () => { expect(track).toHaveBeenCalled(); // Assert - Should call with expected parameters structure for full close - // Calculation: effectivePnL = (3000 - 2900) * 1.5 = 150 - // effectiveMargin = 1450 + 150 = 1600 - // receivedAmount = 1600 - 45 = 1555 + // HyperLiquid's marginUsed already includes PnL + // receivedAmount = marginUsed - fees = 1450 - 45 = 1405 + // realizedPnl = unrealizedPnl = 150 (from defaultPerpsPositionMock) expect(handleClosePosition).toHaveBeenCalledWith( defaultPerpsPositionMock, '', @@ -2060,7 +2080,7 @@ describe('PerpsClosePositionView', () => { { totalFee: 45, marketPrice: 3000, - receivedAmount: 1555, + receivedAmount: 1405, realizedPnl: 150, metamaskFeeRate: 0, metamaskFee: 0, @@ -2369,6 +2389,81 @@ describe('PerpsClosePositionView', () => { }); describe('Price Update Synchronization', () => { + it('recalculates effectivePnL when current price changes', async () => { + // Arrange - Position with entry price + const mockPosition = { + ...defaultPerpsPositionMock, + size: '1', // 1 token long position + entryPrice: '100', // Entry at $100 + marginUsed: '100', + unrealizedPnl: '0', + }; + + useRouteMock.mockReturnValue({ + params: { position: mockPosition }, + }); + + // Mock usePerpsLivePositions to return the test's mock position + usePerpsLivePositionsMock.mockReturnValue({ + positions: [mockPosition], + isInitialLoading: false, + }); + + // Initially price equals entry price (no P&L) + usePerpsLivePricesMock.mockReturnValue({ + ETH: { price: '100' }, // Current price = entry price + }); + + const { rerender, getByText, queryByText } = renderWithProvider( + , + { + state: STATE_MOCK, + }, + true, + ); + + // Assert initial state - effectivePnL should be close to 0 when price = entry + // (100 - 100) * 1 = 0 + // The margin displayed should be just the margin used ($100) + // P&L displayed should be $0 (or very close to it) + await waitFor(() => { + const marginLabel = getByText(strings('perps.close_position.margin')); + expect(marginLabel).toBeDefined(); + // P&L should be ~$0 when price equals entry price + expect(queryByText(/\+.*\$0/)).toBeDefined(); + }); + + // Act - Simulate live price increasing to $150 + // Update both price and position to reflect the P&L change + const updatedPosition = { + ...mockPosition, + unrealizedPnl: '50', // (150 - 100) * 1 = 50 + marginUsed: '150', // 100 initial + 50 P&L + }; + + usePerpsLivePositionsMock.mockReturnValue({ + positions: [updatedPosition], + isInitialLoading: false, + }); + + usePerpsLivePricesMock.mockReturnValue({ + ETH: { price: '150' }, // Live price is $150 (50% profit) + }); + + // Force re-render with new price to trigger dependency recalculation + rerender(); + + // Assert - effectivePnL should update to positive value + // (150 - 100) * 1 = 50 profit + // Margin should now include the P&L: 100 + 50 = 150 + await waitFor(() => { + // Look for the positive P&L display in the "includes P&L" row + expect(queryByText(/\+.*\$50/)).toBeDefined(); + // Look for the margin label to ensure we're checking the right section + expect(getByText(strings('perps.close_position.margin'))).toBeDefined(); + }); + }); + it('syncs input amount when price updates from entry to live price', async () => { // Arrange - Position with entry price const mockPosition = { diff --git a/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.tsx b/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.tsx index f75d539c0594..9ec875ace2a7 100644 --- a/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.tsx +++ b/app/components/UI/Perps/Views/PerpsClosePositionView/PerpsClosePositionView.tsx @@ -46,7 +46,11 @@ import { usePerpsToasts, usePerpsMarketData, } from '../../hooks'; -import { usePerpsLivePrices, usePerpsTopOfBook } from '../../hooks/stream'; +import { + usePerpsLivePositions, + usePerpsLivePrices, + usePerpsTopOfBook, +} from '../../hooks/stream'; import { usePerpsEventTracking } from '../../hooks/usePerpsEventTracking'; import { usePerpsMeasurement } from '../../hooks/usePerpsMeasurement'; import { @@ -125,9 +129,19 @@ const PerpsClosePositionView: React.FC = () => { symbol: position.coin, }); - // Determine position direction - const isLong = parseFloat(position.size) > 0; - const absSize = Math.abs(parseFloat(position.size)); + // Subscribe to live position updates for this coin + // This ensures margin and PnL values include real-time funding fees + const { positions: livePositions } = usePerpsLivePositions({ + throttleMs: 1000, + }); + const livePosition = useMemo( + () => livePositions.find((p) => p.coin === position.coin) || position, + [livePositions, position], + ); + + // Determine position direction using live position data + const isLong = parseFloat(livePosition.size) > 0; + const absSize = Math.abs(parseFloat(livePosition.size)); // Calculate effective price for calculations // For limit orders, use limit price when available; otherwise use current market price const effectivePrice = useMemo(() => { @@ -160,16 +174,25 @@ const PerpsClosePositionView: React.FC = () => { ? closeAmountUSDString : calculatedUSDString; + // Use live position data which includes real-time funding fees + // HyperLiquid's marginUsed already includes accumulated PnL + const marginUsed = parseFloat(livePosition.marginUsed); + + // Use unrealizedPnl from live position (includes funding fees) + const unrealizedPnl = parseFloat(livePosition.unrealizedPnl); + + // Keep pnl reference for backwards compatibility with event tracking + const pnl = unrealizedPnl; + // Calculate position value and effective margin // For limit orders, use limit price for display calculations - // Use ref for market price to prevent recalculation on every WebSocket update const positionValue = useMemo(() => { const priceToUse = orderType === 'limit' && limitPrice ? parseFloat(limitPrice) - : currentPriceRef.current; + : currentPrice; return absSize * priceToUse; - }, [absSize, orderType, limitPrice]); // Exclude currentPrice from deps to prevent recalculation + }, [absSize, orderType, limitPrice, currentPrice]); // Calculate P&L based on effective price (limit price for limit orders) // Use ref for market price to prevent recalculation on every WebSocket update @@ -178,25 +201,19 @@ const PerpsClosePositionView: React.FC = () => { // Calculate P&L based on the effective price (limit price for limit orders) // For long positions: (effectivePrice - entryPrice) * absSize // For short positions: (entryPrice - effectivePrice) * absSize - const priceToUse = - orderType === 'limit' && limitPrice - ? parseFloat(limitPrice) - : currentPriceRef.current; + if (orderType === 'market') { + return pnl; + } + const priceToUse = limitPrice ? parseFloat(limitPrice) : currentPrice; const priceDiff = isLong ? priceToUse - entryPrice : entryPrice - priceToUse; return priceDiff * absSize; - }, [entryPrice, absSize, isLong, orderType, limitPrice]); // Exclude effectivePrice from deps - - // Use the actual initial margin from the position - const initialMargin = parseFloat(position.marginUsed); - - // Use unrealized PnL from position for current market price (for reference/tracking) - const pnl = parseFloat(position.unrealizedPnl); + }, [entryPrice, absSize, isLong, orderType, limitPrice, currentPrice, pnl]); // Exclude effectivePrice from deps // Calculate fees using the unified fee hook const closingValue = useMemo( - () => positionValue * (closePercentage / 100), // Round to 2 decimal places + () => positionValue * (closePercentage / 100), [positionValue, closePercentage], ); const closingValueString = useMemo( @@ -233,13 +250,17 @@ const PerpsClosePositionView: React.FC = () => { orderAmount: closingValueString, }); - // Calculate what user will receive (initial margin + P&L - fees) - // P&L is already shown separately in the margin section as "includes P&L" + // Calculate what user will receive (margin - fees) + // Round each component separately to match what user sees in UI + // This ensures: displayed margin - displayed fees = displayed receive amount const receiveAmount = useMemo(() => { - const marginPortion = (closePercentage / 100) * initialMargin; - const pnlPortion = effectivePnL * (closePercentage / 100); - return marginPortion + pnlPortion - feeResults.totalFee; - }, [closePercentage, initialMargin, effectivePnL, feeResults.totalFee]); + const marginPortion = (closePercentage / 100) * marginUsed; + // Round margin and fees to 2 decimals (what user sees) + const roundedMargin = Math.round(marginPortion * 100) / 100; + const roundedFees = Math.round(feeResults.totalFee * 100) / 100; + // Subtract rounded values for transparent calculation + return roundedMargin - roundedFees; + }, [closePercentage, marginUsed, feeResults.totalFee]); // Get minimum order amount for this asset const { minimumOrderAmount } = useMinimumOrderAmount({ @@ -269,10 +290,10 @@ const PerpsClosePositionView: React.FC = () => { const { handleClosePosition, isClosing } = usePerpsClosePosition(); - const unrealizedPnlPercent = useMemo( - () => (initialMargin > 0 ? (pnl / initialMargin) * 100 : 0), - [initialMargin, pnl], - ); + const unrealizedPnlPercent = useMemo(() => { + const initialMargin = marginUsed - pnl; // Back-calculate initial margin + return initialMargin > 0 ? (pnl / initialMargin) * 100 : 0; + }, [marginUsed, pnl]); usePerpsEventTracking({ eventName: MetaMetricsEvents.PERPS_SCREEN_VIEWED, @@ -328,7 +349,7 @@ const PerpsClosePositionView: React.FC = () => { navigation.goBack(); await handleClosePosition( - position, + livePosition, sizeToClose || '', orderType, orderType === 'limit' ? limitPrice : undefined, @@ -475,10 +496,7 @@ const PerpsClosePositionView: React.FC = () => { const Summary = ( Date: Fri, 7 Nov 2025 08:33:34 -0800 Subject: [PATCH 6/9] fix: Updated tabsbar to update when font size pref changes (#22208) ## **Description** Users were reporting that the TabsBar component in the Wallet view was not horizontally scrollable when tabs overflowed, particularly when device font size settings were increased. **Root causes identified:** 1. **Overflow detection didn't account for container padding** - The component has `px-4` (32px total) padding, but the overflow calculation compared total content width against the full container width, not the available space minus padding. 2. **Underline positioning race condition** - With font scaling, layout measurements would race with font rendering, causing the underline to have inconsistent width/position on initial load. 3. **No dynamic recalculation** - When font size changed after initial load, the overflow detection would not recalculate, leaving tabs non-scrollable even when they overflowed. **Solution:** - Fixed overflow detection to account for container's 32px horizontal padding in both calculation points - Added layout change detection to re-animate underline when tabs resize due to font scaling - Implemented dynamic recalculation of scroll state when any tab's layout changes significantly (>1px) - Used refs to avoid circular dependencies and performance issues ## **Changelog** CHANGELOG entry: Fixed TabsBar not being horizontally scrollable when tabs overflow, especially with increased font size settings ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: TabsBar horizontal scrolling with increased font size Scenario: user increases device font size with many tabs Given user has multiple tabs visible in the Wallet view (Tokens, Perps, Predictions, NFTs) And tabs fit within the screen width at default font size When user increases device font size in Settings > Display & Brightness > Text Size Then tabs should become horizontally scrollable And the underline indicator should correctly match the width of active tab And user should be able to swipe horizontally to see all tabs Scenario: user views tabs with default font size Given user has multiple tabs in the Wallet view And device font size is at default setting And tabs do not overflow the screen width When user views the Wallet screen Then tabs should not be scrollable And all tabs should be visible without scrolling And the underline should correctly align with active tab Scenario: user switches between tabs after font size increase Given user has increased device font size And tabs are horizontally scrollable due to overflow When user taps on a different tab or swipes to navigate Then the underline should animate smoothly to the new tab And the ScrollView should automatically scroll to show the active tab And the underline width should match the tab width accurately ``` ## **Screenshots/Recordings** ### **Before** https://github.com/user-attachments/assets/3359232b-c54a-4d6b-9124-c6e194de7fc1 ### **After** https://github.com/user-attachments/assets/b83a5cc9-9232-4fb1-bbde-3e73d6c75332 ## **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** - [ ] 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. --- > [!NOTE] > Improves TabsBar by accounting for container padding in overflow detection and re-animating the underline when tab layouts change (e.g., font scaling). > > - **TabsBar (`app/component-library/components-temp/Tabs/TabsBar/TabsBar.tsx`)** > - **Overflow detection**: Accounts for container padding (`-32px`) when computing `shouldScroll`. > - **Dynamic layout handling**: > - Detects significant tab layout changes (>1px) and recalculates scroll state. > - Re-animates active tab underline via `requestAnimationFrame`; added `activeIndexRef` to sync with `activeIndex`. > - **Stability/cleanup**: Cancels in-flight animations and pending RAF callbacks on updates/unmount. > - **Deps**: Updates effect/callback deps to include `animateToTab`. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f60d9618cd7c29b3dbe6514067702df98aa72576. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../components-temp/Tabs/TabsBar/TabsBar.tsx | 66 +++++++++++++++---- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/app/component-library/components-temp/Tabs/TabsBar/TabsBar.tsx b/app/component-library/components-temp/Tabs/TabsBar/TabsBar.tsx index 908fcf4398c5..dcc34ad9e7b6 100644 --- a/app/component-library/components-temp/Tabs/TabsBar/TabsBar.tsx +++ b/app/component-library/components-temp/Tabs/TabsBar/TabsBar.tsx @@ -38,13 +38,20 @@ const TabsBar: React.FC = ({ const underlineWidthAnimated = useRef(new Animated.Value(0)).current; const tabLayouts = useRef<{ x: number; width: number }[]>([]); const currentAnimation = useRef(null); + const rafCallbackId = useRef(null); const [isInitialized, setIsInitialized] = useState(false); const [layoutsReady, setLayoutsReady] = useState(false); + const activeIndexRef = useRef(activeIndex); // State for automatic overflow detection const [scrollEnabled, setScrollEnabled] = useState(false); const [containerWidth, setContainerWidth] = useState(0); + // Keep activeIndexRef in sync with activeIndex + useEffect(() => { + activeIndexRef.current = activeIndex; + }, [activeIndex]); + // Reset layout data when tabs change structurally (count or content) const tabKeys = useMemo(() => tabs.map((tab) => tab.key).join(','), [tabs]); const prevTabKeys = useRef(''); @@ -176,7 +183,8 @@ const TabsBar: React.FC = ({ const gapsWidth = (tabs.length - 1) * 24; // Account for gaps between tabs const calculatedContentWidth = totalTabsWidth + gapsWidth; - const shouldScroll = calculatedContentWidth > containerWidth; + // Account for container's px-4 padding (16px * 2 = 32px) + const shouldScroll = calculatedContentWidth > containerWidth - 32; setScrollEnabled(shouldScroll); } } @@ -197,6 +205,13 @@ const TabsBar: React.FC = ({ return; } + // Check if this is a significant change (more than 1px difference) + const previousLayout = tabLayouts.current[index]; + const hasSignificantChange = + !previousLayout || + Math.abs(previousLayout.width - width) > 1 || + Math.abs(previousLayout.x - x) > 1; + // Store layout data tabLayouts.current[index] = { x, width }; @@ -205,22 +220,41 @@ const TabsBar: React.FC = ({ (layout, i) => i >= tabs.length || (layout && layout.width > 0), ); - if (allLayoutsReady && !layoutsReady) { - setLayoutsReady(true); - - // Update scroll detection - if (containerWidth > 0) { - const totalWidth = tabLayouts.current.reduce( - (sum, layout) => sum + (layout?.width || 0), - 0, - ); - const gapsWidth = (tabs.length - 1) * 24; - const shouldScroll = totalWidth + gapsWidth > containerWidth; - setScrollEnabled(shouldScroll); + if (allLayoutsReady) { + // Recalculate scroll detection on initial load OR when any layout changes significantly + if (!layoutsReady || hasSignificantChange) { + if (!layoutsReady) { + setLayoutsReady(true); + } + + // If layouts were already ready and any tab changed, re-animate the active tab + // This ensures re-animation triggers regardless of which tab's callback fires last + if (layoutsReady && hasSignificantChange) { + // Cancel any pending RAF to avoid multiple callbacks + if (rafCallbackId.current !== null) { + cancelAnimationFrame(rafCallbackId.current); + } + rafCallbackId.current = requestAnimationFrame(() => { + rafCallbackId.current = null; + animateToTab(activeIndexRef.current); + }); + } + + // Update scroll detection + if (containerWidth > 0) { + const totalWidth = tabLayouts.current.reduce( + (sum, layout) => sum + (layout?.width || 0), + 0, + ); + const gapsWidth = (tabs.length - 1) * 24; + // Account for container's px-4 padding (16px * 2 = 32px) + const shouldScroll = totalWidth + gapsWidth > containerWidth - 32; + setScrollEnabled(shouldScroll); + } } } }, - [tabs.length, layoutsReady, containerWidth], + [tabs.length, layoutsReady, containerWidth, animateToTab], ); // Cleanup effect @@ -230,6 +264,10 @@ const TabsBar: React.FC = ({ currentAnimation.current.stop(); currentAnimation.current = null; } + if (rafCallbackId.current !== null) { + cancelAnimationFrame(rafCallbackId.current); + rafCallbackId.current = null; + } }, [], ); From a6597394604ba81ef1600f24292ad2b5a3cb1878 Mon Sep 17 00:00:00 2001 From: George Marshall Date: Fri, 7 Nov 2025 08:58:13 -0800 Subject: [PATCH 7/9] fix: Toast component theme reactivity in production builds cp-7.59.0 (#22291) ## **Description** Fixed Toast component theme not updating when switching between light and dark mode in production builds (TestFlight). The issue only affected production builds while working correctly in development/simulator. ### Root Cause: The Toast component had an empty dependency array `[]` in its `useMemo` hook (line 69), which cached the initial theme styles and never updated when the theme changed. This was a remnant from when Toast used inverse/hardcoded theming. ### Solution: Updated the `useMemo` dependency array to properly track `[styles.base, animatedStyle]`, ensuring the component re-renders with correct theme colors when theme changes occur. ## **Changelog** CHANGELOG entry: Fixed Toast component not updating theme colors when switching between light and dark mode ## **Related issues** Fixes: Theme change issue in Toast component ## **Manual testing steps** ```gherkin Feature: Toast theme updates correctly Scenario: User switches device theme while Toast is visible Given the app is running with a visible Toast notification When user switches from light mode to dark mode (or vice versa) in device settings Then the Toast should update to match the new theme colors immediately Scenario: Toast appears with correct theme in production build Given the app is running in a TestFlight production build And user has dark mode enabled When a Toast notification appears Then the Toast displays with correct dark theme colors (not stuck in light theme) ``` ## **Screenshots/Recordings** ### **Before** Toast remained in previous theme colors when device theme was changed in production builds. https://github.com/user-attachments/assets/d96faa41-10c3-4777-baf0-cb33ec137309 ### **After** Toast correctly updates to match current theme in both development and production builds. https://github.com/user-attachments/assets/aea3a725-0aa1-4412-bd56-5bde79fbfbbd ## **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 - [ ] 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** - [ ] 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. --- > [!NOTE] > Update `Toast` `baseStyle` `useMemo` to depend on `styles.base` and `animatedStyle` (and simplify its type) so styling updates correctly. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1a4dca6c844a0fc139bb98abb3dd55e436763fe9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- app/component-library/components/Toast/Toast.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/component-library/components/Toast/Toast.tsx b/app/component-library/components/Toast/Toast.tsx index 73b2b45aea5e..6cfa74c0e7b9 100644 --- a/app/component-library/components/Toast/Toast.tsx +++ b/app/component-library/components/Toast/Toast.tsx @@ -62,12 +62,10 @@ const Toast = forwardRef((_, ref: React.ForwardedRef) => { { translateY: translateYProgress.value - TAB_BAR_HEIGHT - customOffset }, ], })); - const baseStyle: StyleProp>> = - useMemo( - () => [styles.base, animatedStyle], - /* eslint-disable-next-line */ - [], - ); + const baseStyle: StyleProp = useMemo( + () => [styles.base, animatedStyle], + [styles.base, animatedStyle], + ); const resetState = () => setToastOptions(undefined); From c05199e8514e6f15bb96938efad7f64f49a47c44 Mon Sep 17 00:00:00 2001 From: Kylan Hurt <6249205+smilingkylan@users.noreply.github.com> Date: Fri, 7 Nov 2025 10:12:22 -0700 Subject: [PATCH 8/9] feat: Improve link-handling for internally-sourced links (carousel / in-app browser) (#22012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fixes: https://github.com/MetaMask/mobile-planning/issues/2341 This PR fixes an issue where MetaMask deeplinks clicked within the in-app browser or carousel banners were redirecting users to the app store instead of navigating internally within the app. ### Problem When users clicked MetaMask deeplinks (e.g., `https://link.metamask.io/swap`, `metamask://dapp/uniswap.org`) from within the app: - **In-App Browser**: Links would attempt to open the app externally, causing an app store redirect - **Carousel Banners**: Action links would trigger OS-level handling instead of internal navigation This occurred because these URLs were being passed to the OS linking system (`Linking.openURL()`), which would try to open MetaMask again, resulting in an app store redirect since the app was already running. ### Solution The fix intercepts MetaMask deeplinks and routes them through `SharedDeeplinkManager` for internal handling: 1. **Created utility function** (`isInternalDeepLink`) to identify MetaMask URLs that should be handled internally 2. **Updated BrowserTab** to intercept deeplinks in `onShouldStartLoadWithRequest` and use `browserCallBack` for in-browser navigation 3. **Updated Carousel** to differentiate between internal deeplinks and external URLs 4. **Added comprehensive unit tests** covering all deeplink types and edge cases The `browserCallBack` mechanism allows deeplinks that should stay in the browser (e.g., `dapp://` URLs) to navigate the current WebView tab, while other deeplinks (e.g., `swap`, `buy-crypto`) navigate to their respective app screens. ## **Changelog** CHANGELOG entry: Improve handling of links originating from in-app browser and carousel ## **Related issues** Fixes: #[ISSUE_NUMBER] ## **Manual testing steps** ```gherkin Feature: Internal deeplink handling in browser Scenario: user clicks MetaMask dapp deeplink in browser Given user has opened a website in the in-app browser And the website contains a link to "https://link.metamask.io/dapp/uniswap.org" When user taps the deeplink Then the current browser tab navigates to "https://uniswap.org" And the user remains in the browser (no app store redirect) Scenario: user clicks MetaMask feature deeplink in browser Given user has opened a website in the in-app browser And the website contains a link to "https://link.metamask.io/swap" When user taps the deeplink Then the app navigates to the Swap screen And the browser is exited Scenario: user clicks MetaMask deeplink in carousel Given user is viewing the wallet home screen And a carousel banner contains a MetaMask deeplink When user taps the carousel banner Then the app navigates to the appropriate feature screen And no app store redirect occurs Scenario: user clicks external link in browser Given user has opened a website in the in-app browser And the website contains a regular external link When user taps the external link Then the link behaves as normal (opens in browser or externally) Feature: Deeplink type detection Scenario: utility correctly identifies internal deeplinks Given various URL types are tested When "https://link.metamask.io/swap" is checked Then it is identified as internal deeplink When "metamask://dapp/uniswap.org" is checked Then it is identified as internal deeplink When "https://google.com" is checked Then it is identified as external URL ``` ### Additional Testing Steps: 1. **Browser Test Page**: Create an HTML page with test links covering: - Universal links: `https://link.metamask.io/dapp/uniswap.org` - Custom schemes: `metamask://dapp/app.aave.com`, `dapp://curve.fi` - Feature links: `https://link.metamask.io/swap`, `/buy-crypto`, `/home` - External links: `https://ethereum.org` (control group) 2. **Unit Tests**: Run `yarn jest app/util/deeplinks/index.test.ts` 3. **TypeScript**: Run `yarn lint:tsc` to verify type safety ## **Screenshots/Recordings** ### **Before** ### **After** ## **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). - [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** - [ ] 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. --- > [!NOTE] > Intercept MetaMask deeplinks in the in-app browser and carousel to handle them via SharedDeeplinkManager while opening only external URLs with the OS; add isInternalDeepLink utility and tests. > > - **Deeplink handling**: > - Add `util/deeplinks/isInternalDeepLink` to detect MetaMask internal links (universal and custom schemes). > - In `BrowserTab`, intercept internal links in `onShouldStartLoadWithRequest` and route via `SharedDeeplinkManager.parse` with `origin: AppConstants.DEEPLINKS.ORIGIN_IN_APP_BROWSER` and `browserCallBack` support; block WebView load for these. > - In `Carousel`, differentiate links: internal via `SharedDeeplinkManager.parse` (`origin: ORIGIN_CAROUSEL`), external via `Linking.openURL`. > - Extend `AppConstants.DEEPLINKS` with `ORIGIN_CAROUSEL` and `ORIGIN_IN_APP_BROWSER`. > - **Tests**: > - Add `app/util/deeplinks/index.test.ts` covering schemes/hosts and edge cases. > - Update `Carousel` tests to assert external vs internal link behavior and correct origins; mock `Linking.openURL`. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 924a4bce368b8ce8775f7a500f84c874710e6a26. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- app/components/UI/Carousel/index.test.tsx | 37 ++++++++- app/components/UI/Carousel/index.tsx | 32 +++++-- .../Views/BrowserTab/BrowserTab.tsx | 25 ++++++ app/core/AppConstants.ts | 2 + app/util/deeplinks/index.test.ts | 83 +++++++++++++++++++ app/util/deeplinks/index.ts | 55 ++++++++++++ 6 files changed, 221 insertions(+), 13 deletions(-) create mode 100644 app/util/deeplinks/index.test.ts create mode 100644 app/util/deeplinks/index.ts diff --git a/app/components/UI/Carousel/index.test.tsx b/app/components/UI/Carousel/index.test.tsx index ede360145c16..cce8a06e8e60 100644 --- a/app/components/UI/Carousel/index.test.tsx +++ b/app/components/UI/Carousel/index.test.tsx @@ -6,6 +6,7 @@ import { renderHook, } from '@testing-library/react-native'; import { useSelector, useDispatch } from 'react-redux'; +import { Linking } from 'react-native'; import SharedDeeplinkManager from '../../../core/DeeplinkManager/SharedDeeplinkManager'; import AppConstants from '../../../core/AppConstants'; import Carousel, { useFetchCarouselSlides } from './'; @@ -70,6 +71,10 @@ jest.mock('../../../core/DeeplinkManager/SharedDeeplinkManager', () => ({ parse: jest.fn(() => Promise.resolve()), })); +jest.mock('react-native/Libraries/Linking/Linking', () => ({ + openURL: jest.fn(() => Promise.resolve()), +})); + jest.mock('./fetchCarouselSlidesFromContentful', () => ({ ...jest.requireActual('./fetchCarouselSlidesFromContentful'), fetchCarouselSlidesFromContentful: jest.fn(), @@ -200,8 +205,10 @@ describe('Carousel Slide Filtering', () => { describe('Carousel Navigation', () => { it('opens external URLs when slide is clicked', async () => { + const slideID = 'deeplink-slide'; + const slideTestID = `carousel-slide-${slideID}`; const urlSlide = createMockSlide({ - id: 'url-slide', + id: slideID, navigation: { type: 'url', href: 'https://metamask.io' }, }); mockFetchCarouselSlides.mockResolvedValue({ @@ -210,14 +217,36 @@ describe('Carousel Navigation', () => { }); const { findByTestId } = render(); - const slide = await findByTestId('carousel-slide-url-slide'); + const slide = await findByTestId(slideTestID); fireEvent.press(slide); + + expect(Linking.openURL).toHaveBeenCalledWith('https://metamask.io'); + expect(SharedDeeplinkManager.parse).not.toHaveBeenCalled(); + }); + + it('handles internal deeplinks through SharedDeeplinkManager', async () => { + const slideID = 'deeplink-slide'; + const slideTestID = `carousel-slide-${slideID}`; + const deeplinkSlide = createMockSlide({ + id: slideID, + navigation: { type: 'url', href: 'https://link.metamask.io/swap' }, + }); + mockFetchCarouselSlides.mockResolvedValue({ + prioritySlides: [], + regularSlides: [deeplinkSlide], + }); + + const { findByTestId } = render(); + const slide = await findByTestId(slideTestID); + fireEvent.press(slide); + expect(SharedDeeplinkManager.parse).toHaveBeenCalledWith( - 'https://metamask.io', + 'https://link.metamask.io/swap', { - origin: AppConstants.DEEPLINKS.ORIGIN_DEEPLINK, + origin: AppConstants.DEEPLINKS.ORIGIN_CAROUSEL, }, ); + expect(Linking.openURL).not.toHaveBeenCalled(); }); it('navigates to buy flow for fund slides', async () => { diff --git a/app/components/UI/Carousel/index.tsx b/app/components/UI/Carousel/index.tsx index b0f08f2b1faf..b69ad2e83d34 100644 --- a/app/components/UI/Carousel/index.tsx +++ b/app/components/UI/Carousel/index.tsx @@ -6,7 +6,7 @@ import React, { useEffect, useRef, } from 'react'; -import { Dimensions, Animated } from 'react-native'; +import { Dimensions, Animated, Linking } from 'react-native'; import { useDispatch, useSelector } from 'react-redux'; import { useNavigation } from '@react-navigation/native'; import { CarouselProps, CarouselSlide, NavigationAction } from './types'; @@ -42,8 +42,9 @@ import { selectContentfulCarouselEnabledFlag } from './selectors/featureFlags'; import { createBuyNavigationDetails } from '../Ramp/Aggregator/routes/utils'; import Routes from '../../../constants/navigation/Routes'; import { subscribeToContentPreviewToken } from '../../../actions/notification/helpers'; -import AppConstants from '../../../core/AppConstants'; import SharedDeeplinkManager from '../../../core/DeeplinkManager/SharedDeeplinkManager'; +import { isInternalDeepLink } from '../../../util/deeplinks'; +import AppConstants from '../../../core/AppConstants'; const MAX_CAROUSEL_SLIDES = 8; @@ -362,13 +363,26 @@ const CarouselComponent: FC = ({ style, onEmptyState }) => { const openUrl = (href: string): (() => Promise) => - () => - SharedDeeplinkManager.parse(href, { - origin: AppConstants.DEEPLINKS.ORIGIN_DEEPLINK, - }).catch((error) => { - console.error('Failed to open URL:', error); - return false; - }); + () => { + // Check if this is an internal MetaMask deeplink + if (isInternalDeepLink(href)) { + // Handle internal deeplinks through SharedDeeplinkManager + return SharedDeeplinkManager.parse(href, { + origin: AppConstants.DEEPLINKS.ORIGIN_CAROUSEL, + }).catch((error) => { + console.error('Failed to handle internal deeplink:', error); + return false; + }); + } + + // For external URLs, use the OS linking system + return Linking.openURL(href) + .then(() => true) + .catch((error) => { + console.error('Failed to open external URL:', error); + return false; + }); + }; const handleSlideClick = useCallback( (slideId: string, navigation: NavigationAction) => { diff --git a/app/components/Views/BrowserTab/BrowserTab.tsx b/app/components/Views/BrowserTab/BrowserTab.tsx index 73ce163c509f..049c1c54781b 100644 --- a/app/components/Views/BrowserTab/BrowserTab.tsx +++ b/app/components/Views/BrowserTab/BrowserTab.tsx @@ -54,6 +54,8 @@ import { sortMultichainAccountsByLastSelected, } from '../../../core/Permissions'; import Routes from '../../../constants/navigation/Routes'; +import { isInternalDeepLink } from '../../../util/deeplinks'; +import SharedDeeplinkManager from '../../../core/DeeplinkManager/SharedDeeplinkManager'; import { selectIpfsGateway, selectIsIpfsGatewayEnabled, @@ -826,6 +828,29 @@ export const BrowserTab: React.FC = React.memo( } } + // Check if this is an internal MetaMask deeplink that should be handled within the app + if (isInternalDeepLink(urlToLoad)) { + // Handle the deeplink internally instead of passing to OS + SharedDeeplinkManager.parse(urlToLoad, { + origin: AppConstants.DEEPLINKS.ORIGIN_IN_APP_BROWSER, + browserCallBack: (url: string) => { + // If the deeplink handler wants to navigate to a different URL in the browser + if (url && webviewRef.current) { + webviewRef.current.injectJavaScript(` + window.location.href = '${sanitizeUrlInput(url)}'; + true; // Required for iOS + `); + } + }, + }).catch((error) => { + Logger.error( + error, + 'BrowserTab: Failed to handle internal deeplink in browser', + ); + }); + return false; // Stop the webview from loading this URL + } + const { protocol } = new URLParse(urlToLoad); if (trustedProtocolToDeeplink.includes(protocol)) { diff --git a/app/core/AppConstants.ts b/app/core/AppConstants.ts index 27ffd1de3279..de310f814581 100644 --- a/app/core/AppConstants.ts +++ b/app/core/AppConstants.ts @@ -73,9 +73,11 @@ export default { POLLING_FREQUENCY: 10000, }, DEEPLINKS: { + ORIGIN_CAROUSEL: 'carousel', ORIGIN_DEEPLINK: 'deeplink', ORIGIN_QR_CODE: 'qr-code', ORIGIN_NOTIFICATION: 'notifications', + ORIGIN_IN_APP_BROWSER: 'in-app-browser', }, WALLET_CONNECT: { //One day in hours diff --git a/app/util/deeplinks/index.test.ts b/app/util/deeplinks/index.test.ts new file mode 100644 index 000000000000..fe46622c6f58 --- /dev/null +++ b/app/util/deeplinks/index.test.ts @@ -0,0 +1,83 @@ +import { isInternalDeepLink } from './index'; + +describe('deeplinks utils', () => { + describe('isInternalDeepLink', () => { + it('identifies MetaMask custom scheme deeplinks', () => { + expect(isInternalDeepLink('metamask://connect')).toBe(true); + expect(isInternalDeepLink('metamask://wc?uri=...')).toBe(true); + expect(isInternalDeepLink('metamask://dapp/uniswap.org')).toBe(true); + }); + + it('identifies Ethereum scheme deeplinks', () => { + expect(isInternalDeepLink('ethereum://pay-0x1234')).toBe(true); + expect(isInternalDeepLink('ethereum://0x1234?value=1e18')).toBe(true); + }); + + it('identifies dapp scheme deeplinks', () => { + expect(isInternalDeepLink('dapp://app.uniswap.org')).toBe(true); + expect(isInternalDeepLink('dapp://portfolio.metamask.io')).toBe(true); + }); + + it('identifies MetaMask universal links', () => { + expect(isInternalDeepLink('https://link.metamask.io/swap')).toBe(true); + expect(isInternalDeepLink('https://link.metamask.io/buy-crypto')).toBe( + true, + ); + expect( + isInternalDeepLink('https://link.metamask.io/dapp/uniswap.org'), + ).toBe(true); + }); + + it('identifies MetaMask test universal links', () => { + expect(isInternalDeepLink('https://link-test.metamask.io/swap')).toBe( + true, + ); + expect(isInternalDeepLink('https://link-test.metamask.io/send')).toBe( + true, + ); + }); + + it('identifies MetaMask branch links', () => { + expect(isInternalDeepLink('https://metamask.app.link/swap')).toBe(true); + expect(isInternalDeepLink('https://metamask.test-app.link/home')).toBe( + true, + ); + }); + + it('does not identify external URLs as internal', () => { + expect(isInternalDeepLink('https://google.com')).toBe(false); + expect(isInternalDeepLink('https://uniswap.org')).toBe(false); + expect(isInternalDeepLink('https://portfolio.metamask.io')).toBe(false); + expect(isInternalDeepLink('http://example.com')).toBe(false); + }); + + it('does not identify other protocols as internal', () => { + expect(isInternalDeepLink('mailto:test@example.com')).toBe(false); + expect(isInternalDeepLink('tel:+1234567890')).toBe(false); + expect(isInternalDeepLink('wc://session')).toBe(false); + expect(isInternalDeepLink('https://wc.example.com')).toBe(false); + }); + + it('handles edge cases gracefully', () => { + expect(isInternalDeepLink('')).toBe(false); + expect(isInternalDeepLink(null)).toBe(false); + expect(isInternalDeepLink(undefined)).toBe(false); + expect(isInternalDeepLink('not-a-valid-url')).toBe(false); + expect(isInternalDeepLink('metamask://')).toBe(true); // Still a valid MetaMask scheme + }); + + it('handlesURLs with query parameters and fragments', () => { + expect( + isInternalDeepLink( + 'https://link.metamask.io/swap?chainId=1&token=0x...', + ), + ).toBe(true); + expect( + isInternalDeepLink('metamask://connect?channelId=123#fragment'), + ).toBe(true); + expect(isInternalDeepLink('https://google.com?metamask=true')).toBe( + false, + ); + }); + }); +}); diff --git a/app/util/deeplinks/index.ts b/app/util/deeplinks/index.ts new file mode 100644 index 000000000000..c491d253b99a --- /dev/null +++ b/app/util/deeplinks/index.ts @@ -0,0 +1,55 @@ +import AppConstants from '../../core/AppConstants'; + +const { + MM_UNIVERSAL_LINK_HOST, + MM_IO_UNIVERSAL_LINK_HOST, + MM_IO_UNIVERSAL_LINK_TEST_HOST, +} = AppConstants; + +/** + * Checks if a URL is an internal MetaMask deeplink that should be handled + * within the app rather than passed to the OS + * + * @param url - The URL to check + * @returns true if the URL is a MetaMask internal deeplink + */ +export const isInternalDeepLink = (url: string | null | undefined): boolean => { + if (!url) return false; + + const metamaskHosts = [ + MM_UNIVERSAL_LINK_HOST || 'link.metamask.io', + MM_IO_UNIVERSAL_LINK_HOST || 'link.metamask.io', + MM_IO_UNIVERSAL_LINK_TEST_HOST || 'link-test.metamask.io', + 'metamask.app.link', // todo double-check if we can remove + 'metamask.test-app.link', // todo double-check if we can remove + ].filter(Boolean); + + try { + // Check custom schemes first (more efficient for these cases) + const internalSchemes = ['metamask:', 'ethereum:', 'dapp:']; + if (internalSchemes.some((scheme) => url.startsWith(scheme))) { + return true; + } + + // Parse URL for host checking + const urlObj = new URL(url); + + // Check if it's a MetaMask universal link + return metamaskHosts.includes(urlObj.hostname); + } catch { + // If URL parsing fails, check if it's a simple scheme match + return ['metamask:', 'ethereum:', 'dapp:'].some((scheme) => + url.startsWith(scheme), + ); + } +}; + +/** + * Determines if a URL should be opened externally (Linking.openURL()) + * This is the inverse of isInternalDeepLink but kept separate for clarity + * + * @param url - The URL to check + * @returns true if the URL should be opened externally + */ +export const shouldOpenExternally = (url: string): boolean => + !isInternalDeepLink(url); From e30bced11bdb185245877af8e4757ace0608f631 Mon Sep 17 00:00:00 2001 From: Nick Gambino <35090461+gambinish@users.noreply.github.com> Date: Fri, 7 Nov 2025 09:24:58 -0800 Subject: [PATCH 9/9] fix: back arrow perps home should always navigate to wallet home cp-7.59.0 (#22288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The back arrow in `PerpsHomeScreen` was using `navigation.goBack()`, which would work correctly when navigating from Main wallet view → Perps Home → Main wallet view But when entering `PerpsHomeScreen` from another entry point, such as PerpsOnboarding, it would create a loop when navigating from Perps Tutorial → Perps Home (back arrow would return to tutorial) This PR fixes this by changing this to `navigateToWallet` which explicitly navigates to the wallet home screen as expected ## **Changelog** CHANGELOG entry: Fix navigation loop from PerpsHomeScreen ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/TAT-1984 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** ## **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** - [ ] 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. --- > [!NOTE] > PerpsHomeView back arrow now always navigates to wallet home; tests updated to assert wallet navigation. > > - **UI/Perps**: > - `PerpsHomeView.tsx`: Change back handler from `navigateBack` to `navigateToWallet` to avoid navigation loops; keep search navigation via `navigateToMarketList`. > - **Tests**: > - `PerpsHomeView.test.tsx`: Update mocks and expectations to use `navigateToWallet` for back action; clear relevant mocks; retain existing behavior checks for sections and actions. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 3695aa8b74e131615a63c91960ba2ca30e766e59. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../UI/Perps/Views/PerpsHomeView/PerpsHomeView.test.tsx | 9 ++++++--- .../UI/Perps/Views/PerpsHomeView/PerpsHomeView.tsx | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.test.tsx b/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.test.tsx index ccca11341d68..8d225a367082 100644 --- a/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.test.tsx +++ b/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.test.tsx @@ -41,6 +41,7 @@ jest.mock( // Mock hooks (consolidated to avoid conflicts) const mockNavigateBack = jest.fn(); +const mockNavigateToWallet = jest.fn(); const mockNavigateToMarketList = jest.fn(); jest.mock('../../hooks', () => ({ usePerpsHomeData: jest.fn(), @@ -49,6 +50,7 @@ jest.mock('../../hooks', () => ({ navigateTo: jest.fn(), navigateToMarketDetails: jest.fn(), navigateToMarketList: mockNavigateToMarketList, + navigateToWallet: mockNavigateToWallet, navigateBack: mockNavigateBack, goBack: jest.fn(), })), @@ -392,6 +394,7 @@ describe('PerpsHomeView', () => { beforeEach(() => { jest.clearAllMocks(); mockNavigateBack.mockClear(); + mockNavigateToWallet.mockClear(); mockNavigateToMarketList.mockClear(); mockUsePerpsHomeData.mockReturnValue(mockDefaultData); }); @@ -537,15 +540,15 @@ describe('PerpsHomeView', () => { expect(queryByText('perps.home.orders')).toBeNull(); }); - it('handles back button press', () => { + it('navigates to wallet home when back button is pressed', () => { // Arrange const { getByTestId } = render(); // Act fireEvent.press(getByTestId('back-button')); - // Assert - expect(mockNavigateBack).toHaveBeenCalled(); + // Assert - Always navigates to wallet home to avoid loops (e.g., from tutorial) + expect(mockNavigateToWallet).toHaveBeenCalled(); }); it('navigates to close all modal when close all is pressed', () => { diff --git a/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.tsx b/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.tsx index 315373c30773..9167894f8eb3 100644 --- a/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.tsx +++ b/app/components/UI/Perps/Views/PerpsHomeView/PerpsHomeView.tsx @@ -174,8 +174,9 @@ const PerpsHomeView = () => { setShowCancelAllSheet(false); }, []); - // Back button handler - now uses navigation hook - const handleBackPress = perpsNavigation.navigateBack; + // Back button handler - always navigate to wallet home to avoid loops + // (e.g., when coming from tutorial/onboarding flow) + const handleBackPress = perpsNavigation.navigateToWallet; return (