From 1deacf6caff53437ea15ad0c0ce8d0ebccd45c42 Mon Sep 17 00:00:00 2001 From: "thelullabyy (via MelvinBot)" Date: Tue, 21 Jul 2026 03:26:10 +0000 Subject: [PATCH] Fix: preserve prev/next carousel when opening expense from expense report The MoneyRequestReportView mount effect guarded against taking over a descriptor-backed carousel (Home Recently added) but not a snapshot-backed one (the Spend page seeds the carousel with a snapshot hash and no descriptors). It overwrote the Spend-page carousel and its unmount cleanup then wiped it, so the prev/next arrows disappeared after navigating back. Extend getActiveTransactionIDs to expose the snapshot hash and bail out of the report-view effect when a snapshot-backed carousel is already active, mirroring the existing descriptor guard. Co-authored-by: thelullabyy --- .../MoneyRequestReportTransactionList.tsx | 12 ++++--- .../actions/TransactionThreadNavigation.ts | 11 ++++--- ...ransactionListActiveTransactionIDsTest.tsx | 31 +++++++++++++++++-- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 18cb1750b5bd..68449739aaa5 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -500,11 +500,13 @@ function MoneyRequestReportTransactionList({ if (anchorTransactionID && latestActiveTransactionIDs?.includes(anchorTransactionID)) { return; } - // Don't take over a snapshot-backed carousel (identified by its sibling descriptors, e.g. the Home - // "Recently added" flow) that belongs to the transaction thread sitting underneath this report. - // Overwriting and then clearing it would drop that carousel when the user navigates back. Row presses - // still seed the correct siblings lazily via useNavigateToTransactionThread. - if (getActiveTransactionIDs().descriptors) { + // Don't take over a snapshot-backed carousel (identified by its snapshot hash, e.g. the Spend page + // search, or its sibling descriptors, e.g. the Home "Recently added" flow) that belongs to the + // transaction thread sitting underneath this report. Overwriting and then clearing it would drop that + // carousel when the user navigates back. Row presses still seed the correct siblings lazily via + // useNavigateToTransactionThread. + const {snapshotHash: activeSnapshotHash, descriptors: activeDescriptors} = getActiveTransactionIDs(); + if (activeSnapshotHash != null || activeDescriptors) { return; } diff --git a/src/libs/actions/TransactionThreadNavigation.ts b/src/libs/actions/TransactionThreadNavigation.ts index ab0f1131fbb4..573e6ec88043 100644 --- a/src/libs/actions/TransactionThreadNavigation.ts +++ b/src/libs/actions/TransactionThreadNavigation.ts @@ -78,12 +78,13 @@ function setActiveTransactionIDs(ids: string[], snapshotHash?: number, siblingDe } /** - * Returns the currently active transaction IDs and sibling descriptors. Used by screens that would otherwise - * take over the carousel context (e.g. a money request report opened on top of an existing transaction thread) - * so they can detect a snapshot-backed carousel (one with descriptors) and avoid clobbering it. + * Returns the currently active transaction IDs, snapshot hash, and sibling descriptors. Used by screens that + * would otherwise take over the carousel context (e.g. a money request report opened on top of an existing + * transaction thread) so they can detect a snapshot-backed carousel (one with a snapshot hash and/or + * descriptors) and avoid clobbering it. */ -function getActiveTransactionIDs(): {ids: string[] | null; descriptors: Record | null} { - return {ids: lastSetIDs, descriptors: lastSetDescriptors}; +function getActiveTransactionIDs(): {ids: string[] | null; snapshotHash: number | null; descriptors: Record | null} { + return {ids: lastSetIDs, snapshotHash: lastSetSnapshotHash, descriptors: lastSetDescriptors}; } function clearActiveTransactionIDs() { diff --git a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx index 9fe9dc19f4e6..40c184d7c328 100644 --- a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx +++ b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx @@ -14,7 +14,7 @@ import createRandomTransaction from '../utils/collections/transaction'; jest.mock('@libs/actions/TransactionThreadNavigation', () => ({ setActiveTransactionIDs: jest.fn(() => Promise.resolve()), clearActiveTransactionIDs: jest.fn(() => Promise.resolve()), - getActiveTransactionIDs: jest.fn(() => ({ids: null, descriptors: null})), + getActiveTransactionIDs: jest.fn(() => ({ids: null, snapshotHash: null, descriptors: null})), })); // Mock the navigation module @@ -44,7 +44,8 @@ function useActiveTransactionIDsEffect(visualOrderTransactionIDs: string[]) { if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) { return; } - if (getActiveTransactionIDs().descriptors) { + const {snapshotHash: activeSnapshotHash, descriptors: activeDescriptors} = getActiveTransactionIDs(); + if (activeSnapshotHash != null || activeDescriptors) { return; } setActiveTransactionIDs(visualOrderTransactionIDs); @@ -65,7 +66,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () beforeEach(() => { jest.clearAllMocks(); - mockGetActiveTransactionIDs.mockReturnValue({ids: null, descriptors: null}); + mockGetActiveTransactionIDs.mockReturnValue({ids: null, snapshotHash: null, descriptors: null}); (navigationRef.getRootState as jest.Mock).mockReturnValue({} as ReturnType); }); @@ -191,6 +192,7 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'}); mockGetActiveTransactionIDs.mockReturnValue({ ids: ['recentlyAdded1', 'recentlyAdded2'], + snapshotHash: null, descriptors: {recentlyAdded1: {reportID: 'r1', transaction: {...createRandomTransaction(1), transactionID: 'recentlyAdded1'}}}, }); @@ -207,6 +209,29 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); }); + it('should NOT take over a snapshot-backed carousel that already has a snapshot hash (e.g. the Spend page)', () => { + // Given the focused route is SEARCH_REPORT and a snapshot-backed carousel (e.g. the Spend page search) is active + mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'}); + mockGetActiveTransactionIDs.mockReturnValue({ + ids: ['spend1', 'spend2'], + snapshotHash: 1234, + descriptors: null, + }); + + const transactionIDs = ['trans1', 'trans2', 'trans3']; + + // When the hook is rendered and then unmounted + const {unmount} = renderHook(() => useActiveTransactionIDsEffect(transactionIDs)); + + // Then it should neither overwrite nor clear the existing carousel context, so the underlying + // Spend-page carousel (and its prev/next arrows) survives navigating back out of the report. + expect(mockSetActiveTransactionIDs).not.toHaveBeenCalled(); + + unmount(); + + expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); + }); + it('should handle empty transaction IDs array', () => { // Given the focused route is SEARCH_REPORT mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'});