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

Remove useDeepCompareRef from MoneyRequestReportTransactionList

Author: mollfpr

src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx

@@ -14,7 +14,6 @@ import Text from '@components/Text';
 import {useWideRHPActions} from '@components/WideRHPContextProvider';
 import useCopySelectionHelper from '@hooks/useCopySelectionHelper';
 import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
-import useDeepCompareRef from '@hooks/useDeepCompareRef';
 import useHandleSelectionMode from '@hooks/useHandleSelectionMode';
 import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset';
 import useLocalize from '@hooks/useLocalize';
@@ -284,17 +283,19 @@ function MoneyRequestReportTransactionList({
         return groupedTransactions.flatMap((group) => group.transactions.filter((transaction) => !isTransactionPendingDelete(transaction)).map((transaction) => transaction.transactionID));
     }, [groupedTransactions, sortedTransactions, shouldShowGroupedTransactions]);
 
-    const visualOrderTransactionIDsDeepCompare = useDeepCompareRef(visualOrderTransactionIDs);
     useEffect(() => {
         const focusedRoute = findFocusedRoute(navigationRef.getRootState());
         if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) {
             return;
         }
-        setActiveTransactionIDs(visualOrderTransactionIDsDeepCompare ?? []);
+        setActiveTransactionIDs(visualOrderTransactionIDs);
+    }, [visualOrderTransactionIDs]);
+
+    useEffect(() => {
         return () => {
             clearActiveTransactionIDs();
         };
-    }, [visualOrderTransactionIDsDeepCompare]);
+    }, []);
 
     const groupSelectionState = useMemo(() => {
         const state = new Map<string, {isSelected: boolean; isIndeterminate: boolean; isDisabled: boolean; pendingAction?: PendingAction}>();

src/libs/actions/TransactionThreadNavigation.ts

@@ -9,11 +9,23 @@ import ONYXKEYS from '@src/ONYXKEYS';
  * We save this value in onyx, so that we can correctly display navigation UI in transaction thread RHP.
  */
 
+let lastSetIDs: string[] | null = null;
+
+/**
+ * Idempotent: skips the Onyx write when the IDs haven't changed.
+ * This lets callers (e.g. useEffect in MoneyRequestReportTransactionList) fire
+ * freely without worrying about referential equality of the input array.
+ */
 function setActiveTransactionIDs(ids: string[]) {
+    if (lastSetIDs?.length === ids.length && lastSetIDs.every((id, i) => id === ids.at(i))) {
+        return Promise.resolve();
+    }
+    lastSetIDs = ids;
     return Onyx.set(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, ids);
 }
 
 function clearActiveTransactionIDs() {
+    lastSetIDs = null;
     return Onyx.set(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, null);
 }
 

tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx

@@ -1,7 +1,6 @@
 import {findFocusedRoute} from '@react-navigation/native';
 import {renderHook} from '@testing-library/react-native';
 import {useEffect} from 'react';
-import useDeepCompareRef from '@hooks/useDeepCompareRef';
 import {clearActiveTransactionIDs, setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation';
 import {navigationRef} from '@libs/Navigation/Navigation';
 import SCREENS from '@src/SCREENS';
@@ -30,20 +29,19 @@ jest.mock('@react-navigation/native', () => ({
  * to allow isolated testing of the useEffect behavior.
  */
 function useActiveTransactionIDsEffect(visualOrderTransactionIDs: string[]) {
-    const visualOrderTransactionIDsDeepCompare = useDeepCompareRef(visualOrderTransactionIDs);
-
     useEffect(() => {
         const focusedRoute = findFocusedRoute(navigationRef.getRootState());
         if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) {
             return;
         }
-        setActiveTransactionIDs(visualOrderTransactionIDsDeepCompare ?? []);
+        setActiveTransactionIDs(visualOrderTransactionIDs);
+    }, [visualOrderTransactionIDs]);
+
+    useEffect(() => {
         return () => {
             clearActiveTransactionIDs();
         };
-    }, [visualOrderTransactionIDsDeepCompare]);
-
-    return {visualOrderTransactionIDsDeepCompare};
+    }, []);
 }
 
 describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () => {
@@ -112,7 +110,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', ()
         expect(mockClearActiveTransactionIDs).toHaveBeenCalledTimes(1);
     });
 
-    it('should NOT call clearActiveTransactionIDs on unmount when route was NOT SEARCH_REPORT', () => {
+    it('should call clearActiveTransactionIDs on unmount even when route was NOT SEARCH_REPORT', () => {
         // Given the focused route is NOT SEARCH_REPORT
         mockFindFocusedRoute.mockReturnValue({name: 'SomeOtherRoute', key: 'test-key'});
 
@@ -123,11 +121,11 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', ()
 
         unmount();
 
-        // Then clearActiveTransactionIDs should NOT be called (since the effect returned early)
-        expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled();
+        // Then clearActiveTransactionIDs should still be called (cleanup runs on unmount regardless of route)
+        expect(mockClearActiveTransactionIDs).toHaveBeenCalledTimes(1);
     });
 
-    it('should update active transaction IDs when the list changes (deep comparison)', () => {
+    it('should update active transaction IDs when the list changes', () => {
         // Given the focused route is SEARCH_REPORT
         mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'});
 
@@ -150,7 +148,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', ()
         expect(mockSetActiveTransactionIDs).toHaveBeenLastCalledWith(newTransactionIDs);
     });
 
-    it('should NOT update when transaction IDs array has same content (deep comparison)', () => {
+    it('should call setActiveTransactionIDs on reference change without clearing first (idempotent guard is in the action layer)', () => {
         // Given the focused route is SEARCH_REPORT
         mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'});
 
@@ -167,8 +165,12 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', ()
         const sameContentNewArray = ['trans1', 'trans2'];
         rerender({ids: sameContentNewArray});
 
-        // Then setActiveTransactionIDs should NOT be called again (deep comparison prevents it)
-        expect(mockSetActiveTransactionIDs).toHaveBeenCalledTimes(1);
+        // 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);
+        expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled();
     });
 
     it('should handle empty transaction IDs array', () => {