Merge pull request Expensify#83772 from Expensify/vit-removeDeepCompareRef-transactionList

Fix Expensify#83751: prevent overwriting active transaction IDs on referential re-renders

Author: Gonals

src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx

@@ -312,19 +312,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 groupSelectionState = useMemo(() => {
         const state = new Map<string, {isSelected: boolean; isIndeterminate: boolean; isDisabled: boolean; pendingAction?: PendingAction}>();

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();
     });