diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index c99ced50a449f..f6eac599e1b28 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -280,19 +280,24 @@ function MoneyRequestReportTransactionList({ return groupedTransactions.flatMap((group) => group.transactions.filter((transaction) => !isTransactionPendingDelete(transaction)).map((transaction) => transaction.transactionID)); }, [groupedTransactions, sortedTransactions, shouldShowGroupedTransactions]); + // Primitive proxy for visualOrderTransactionIDs used as the effect dependency below. + // Other callers (e.g. TransactionDuplicateReview.onPreviewPressed) can write to the same + // Onyx key with a different ordering. Using the raw array reference would cause the effect + // to re-fire on every referential change and overwrite those IDs. The joined string ensures + // the effect only re-fires when the actual content changes. + const visualOrderTransactionIDsKey = useMemo(() => visualOrderTransactionIDs.join(','), [visualOrderTransactionIDs]); + useEffect(() => { const focusedRoute = findFocusedRoute(navigationRef.getRootState()); if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) { return; } setActiveTransactionIDs(visualOrderTransactionIDs); - }, [visualOrderTransactionIDs]); - - useEffect(() => { return () => { clearActiveTransactionIDs(); }; - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps -- visualOrderTransactionIDsKey is a primitive proxy for the array to avoid re-firing on referential-only changes + }, [visualOrderTransactionIDsKey]); const sortedTransactionsMap = useMemo(() => { const map = new Map(); diff --git a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx index cc41316738195..442a9da5c5728 100644 --- a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx +++ b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx @@ -1,6 +1,6 @@ import {findFocusedRoute} from '@react-navigation/native'; import {renderHook} from '@testing-library/react-native'; -import {useEffect} from 'react'; +import {useEffect, useMemo} from 'react'; import {clearActiveTransactionIDs, setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import {navigationRef} from '@libs/Navigation/Navigation'; import SCREENS from '@src/SCREENS'; @@ -29,19 +29,21 @@ jest.mock('@react-navigation/native', () => ({ * to allow isolated testing of the useEffect behavior. */ function useActiveTransactionIDsEffect(visualOrderTransactionIDs: string[]) { + const visualOrderTransactionIDsKey = useMemo(() => visualOrderTransactionIDs.join(','), [visualOrderTransactionIDs]); + useEffect(() => { const focusedRoute = findFocusedRoute(navigationRef.getRootState()); if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) { return; } setActiveTransactionIDs(visualOrderTransactionIDs); - }, [visualOrderTransactionIDs]); - - useEffect(() => { return () => { clearActiveTransactionIDs(); }; - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps -- visualOrderTransactionIDsKey is a primitive proxy for the array + }, [visualOrderTransactionIDsKey]); + + return {visualOrderTransactionIDsKey}; } describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () => { @@ -110,7 +112,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () expect(mockClearActiveTransactionIDs).toHaveBeenCalledTimes(1); }); - it('should call clearActiveTransactionIDs on unmount even when route was NOT SEARCH_REPORT', () => { + it('should NOT call clearActiveTransactionIDs on unmount when route was NOT SEARCH_REPORT', () => { // Given the focused route is NOT SEARCH_REPORT mockFindFocusedRoute.mockReturnValue({name: 'SomeOtherRoute', key: 'test-key'}); @@ -121,8 +123,8 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () unmount(); - // Then clearActiveTransactionIDs should still be called (cleanup runs on unmount regardless of route) - expect(mockClearActiveTransactionIDs).toHaveBeenCalledTimes(1); + // Then clearActiveTransactionIDs should NOT be called (since the effect returned early, no cleanup was registered) + expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); }); it('should update active transaction IDs when the list changes', () => { @@ -148,7 +150,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () expect(mockSetActiveTransactionIDs).toHaveBeenLastCalledWith(newTransactionIDs); }); - it('should call setActiveTransactionIDs on reference change without clearing first (idempotent guard is in the action layer)', () => { + it('should NOT re-fire when array reference changes but content is the same', () => { // Given the focused route is SEARCH_REPORT mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'}); @@ -165,11 +167,9 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () const sameContentNewArray = ['trans1', 'trans2']; rerender({ids: sameContentNewArray}); - // Then the effect fires (new reference), but setActiveTransactionIDs internally skips the Onyx write. - // Crucially, clearActiveTransactionIDs is NOT called on re-render (only on unmount), - // preventing the null→same-IDs flash. - expect(mockSetActiveTransactionIDs).toHaveBeenCalledTimes(2); - expect(mockSetActiveTransactionIDs).toHaveBeenLastCalledWith(sameContentNewArray); + // Then the effect should NOT re-fire because the join(',') key hasn't changed. + // This prevents overwriting IDs set by other callers (e.g. TransactionDuplicateReview.onPreviewPressed). + expect(mockSetActiveTransactionIDs).toHaveBeenCalledTimes(1); expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); });