From be95b7dc4cec54db72d63eab4d0a2056dfb4f162 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 29 Jul 2026 13:27:03 +0700 Subject: [PATCH 01/31] Open the pressed expense from a multi-expense report preview Tapping a card in the multi-expense report preview carousel opens that expense instead of the parent report. Wide layouts keep the report in the super-wide RHP and cascade the pressed expense on top; narrow layouts open the expense with a single forward navigation, so back returns to the chat. This is v2 of #92546, which was reverted for five deploy blockers. v1 inserted the parent report beneath the expense so back stopped on the report first -- a spliced stack entry on native, an extra history entry on mobile web. That hand-built stack is gone: it broke whenever another flow mutated it (#97184 replaced the deleted thread with a duplicate report route, #97183 left a stale route after a split save, #97158 crashed resolving duplicates) and on mobile web the extra entry forced a state rebuild that flashed the chat on back. Navigation.ts is untouched by this version. Also guards an expense deleted while offline (#97149): those rows stay in the carousel but their thread is gone, so pressing one now opens the parent report instead of landing on 'It's not here'. --- .../ReportPreviewActionButton.tsx | 33 +- .../MoneyRequestReportPreview/index.tsx | 222 ++++++++++-- tests/ui/MoneyRequestReportPreview.test.tsx | 327 ++++++++++++++++++ .../ReportPreviewActionButtonTest.tsx | 4 +- 4 files changed, 554 insertions(+), 32 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx index f2fc0616078e..2589406ec148 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx @@ -24,7 +24,16 @@ function ReportPreviewActionButton() { const {buttonMaxWidth} = useReportPreviewUIState(); const {openReportFromPreview} = useReportPreviewActions(); - const renderButton = () => { + const viewButton = ( + + ); + + const renderPrimaryButton = () => { if (reportPreviewAction === CONST.REPORT.REPORT_PREVIEW_ACTIONS.SUBMIT) { return ; } @@ -45,17 +54,21 @@ function ReportPreviewActionButton() { return ; } - return ( - - ); + return null; }; - return {renderButton()}; + const primaryButton = renderPrimaryButton(); + + if (!primaryButton) { + return {viewButton}; + } + + return ( + + {primaryButton} + {viewButton} + + ); } export default ReportPreviewActionButton; diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 587105679366..e29e0da01475 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -1,6 +1,8 @@ import {usePersonalDetails} from '@components/OnyxListItemProvider'; import TransactionPreview from '@components/ReportActionItem/TransactionPreview'; +import {useWideRHPActions} from '@components/WideRHPContextProvider'; +import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; import useNetwork from '@hooks/useNetwork'; import useNewTransactions from '@hooks/useNewTransactions'; import useOnyx from '@hooks/useOnyx'; @@ -11,8 +13,16 @@ import useStyleUtils from '@hooks/useStyleUtils'; import useThemeStyles from '@hooks/useThemeStyles'; import useTransactionViolations from '@hooks/useTransactionViolations'; +import {createTransactionThreadReport, openReport, setOptimisticTransactionThread} from '@libs/actions/Report'; +import {setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; -import {getIOUActionForReportID, isSplitBillAction as isSplitBillActionReportActionsUtils, isTrackExpenseAction as isTrackExpenseActionReportActionsUtils} from '@libs/ReportActionsUtils'; +import { + getIOUActionForReportID, + getOriginalMessage, + isMoneyRequestAction, + isSplitBillAction as isSplitBillActionReportActionsUtils, + isTrackExpenseAction as isTrackExpenseActionReportActionsUtils, +} from '@libs/ReportActionsUtils'; import {isIOUReport} from '@libs/ReportUtils'; import {startSpan} from '@libs/telemetry/activeSpans'; @@ -30,12 +40,16 @@ import type {ListRenderItem} from '@shopify/flash-list'; import type {LayoutChangeEvent} from 'react-native'; import {useIsFocused} from '@react-navigation/core'; -import React, {useCallback, useMemo, useRef, useState} from 'react'; +import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import type {MoneyRequestReportPreviewProps} from './types'; import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent'; +// Delay (ms) before the pressed expense opens on top of the report's wide RHP. Letting the report settle +// into the wide RHP first makes the two panels open as a cascade rather than appearing at once. +const PRESSED_EXPENSE_CASCADE_DELAY = 180; + function MoneyRequestReportPreview({ iouReportID, iouReport, @@ -53,7 +67,11 @@ function MoneyRequestReportPreview({ const StyleUtils = useStyleUtils(); // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth const {shouldUseNarrowLayout, isSmallScreenWidth} = useResponsiveLayout(); + const {markReportRHPWidth} = useWideRHPActions(); const personalDetailsList = usePersonalDetails(); + const {email: currentUserEmail, accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); + const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); + const [betas] = useOnyx(ONYXKEYS.BETAS); const invoiceReceiverPolicyID = chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : undefined; const [invoiceReceiverPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(invoiceReceiverPolicyID)}`); const invoiceReceiverPersonalDetail = chatReport?.invoiceReceiver && 'accountID' in chatReport.invoiceReceiver ? personalDetailsList?.[chatReport.invoiceReceiver.accountID] : null; @@ -63,6 +81,19 @@ function MoneyRequestReportPreview({ // reimbursable derivations so they include optimistically-deleted rows, exactly as before the decomposition. const allReportTransactions = Object.values(reportTransactionsCollection ?? {}).filter((transaction): transaction is Transaction => !!transaction); const transactions = allReportTransactions.filter((transaction) => isOffline || transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE); + // Tracks how many actions the IOU report has loaded so a deferred expense press can be retried once + // the actions arrive (they may be missing right after a cache clear). + const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { + selector: (reportActions) => Object.keys(reportActions ?? {}).length, + }); + // Whether the deferred press's openReport fetch is still in flight. The true -> false flip re-runs the drain + // effect below, so a deferred press settles even when the fetch returns the actions we already had. + const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { + selector: (loadingState) => !!loadingState?.isLoadingInitialReportActions, + }); + // Holds a pressed transaction whose thread report could not be resolved yet, so the expense can be + // opened once the IOU report's actions have loaded instead of falling back to the parent report. + const pendingExpenseTransactionRef = useRef(null); const policy = usePolicy(policyID); const lastTransaction = transactions?.at(0); const lastTransactionViolations = useTransactionViolations(lastTransaction?.transactionID); @@ -144,27 +175,176 @@ function MoneyRequestReportPreview({ const transactionPreviewContainerStyles = [styles.h100, reportPreviewStyles.transactionPreviewCarouselStyle]; - const renderItem: ListRenderItem = ({item}) => ( - + // Resolve the target transaction thread report. Prefer the IOU action's childReportID, then the + // transaction's own thread id, and finally create the thread inline so the press never lands on a dead route. + const resolveChildReportID = useCallback( + (transaction: Transaction) => { + const transactionIOUAction = getIOUActionForReportID(transaction.reportID, transaction.transactionID); + let childReportID = transactionIOUAction?.childReportID ?? transaction.transactionThreadReportID; + if (childReportID) { + // The thread already exists, but it may not be present in OnyxDB. Seed its + // optimistic report shell + parent linkage so navigating to it renders the expense instead of a + // blank/not-found screen. + setOptimisticTransactionThread(childReportID, iouReport?.reportID ?? transaction.reportID, transactionIOUAction?.reportActionID, iouReport?.policyID ?? policyID); + } else if (transactionIOUAction?.reportActionID) { + const transactionID = isMoneyRequestAction(transactionIOUAction) ? getOriginalMessage(transactionIOUAction)?.IOUTransactionID : undefined; + if (transactionID) { + childReportID = createTransactionThreadReport({ + introSelected, + currentUserLogin: currentUserEmail ?? '', + currentUserAccountID, + betas, + iouReport, + iouReportAction: transactionIOUAction, + })?.reportID; + } + } + return childReportID; + }, + [betas, currentUserAccountID, currentUserEmail, introSelected, iouReport, policyID], + ); + + const navigateToExpense = useCallback( + (childReportID: string) => { + startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${childReportID}`, { + name: 'MoneyRequestReportPreview.Transaction', + op: CONST.TELEMETRY.SPAN_OPEN_REPORT, + }); + + if (isSmallScreenWidth) { + // Narrow layouts open the pressed expense as a plain forward navigation, so back returns to the chat + // it was opened from. v1 instead inserted the parent report beneath the expense (a spliced stack entry + // on native, an extra history entry on mobile web) so that back stopped on the report first. That + // hand-built stack broke whenever another flow mutated it -- deleting an expense replaced the thread + // with a duplicate report route, the split-expense save left a stale route behind, and on mobile web + // the extra entry forced a state rebuild that flashed the chat on back. Keeping the stack untouched + // costs one intermediate stop and removes that whole class of failure. + setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)); + Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(childReportID, undefined, undefined, Navigation.getActiveRoute())); + return; + } + + // On wide layouts open the expense report itself in the wide RHP (super wide for multi-expense + // reports) and show the pressed expense on top of it — mirroring how an expense opens from the + // report view — rather than navigating to the report in the Inbox. Back returns to the report, + // and back again to the chat. + if (iouReportID) { + const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); + markReportRHPWidth(iouReportID, 'super-wide'); + Navigation.navigate(reportRoute); + setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)).then(() => { + markReportRHPWidth(childReportID, 'wide'); + // Let the report's wide RHP settle before opening the pressed expense on top, so the two + // panels open as a cascade rather than at once. + setTimeout(() => { + // The user may have closed the report's wide RHP or navigated away during the cascade delay; + // don't reopen the expense over whatever screen is now active. + if (!Navigation.isActiveRoute(reportRoute)) { + return; + } + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); + }, PRESSED_EXPENSE_CASCADE_DELAY); + }); + return; + } + + // Fallback when the parent report is unknown: open the pressed expense alone in the wide RHP. + setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)).then(() => { + markReportRHPWidth(childReportID, 'wide'); + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); + }); + }, + [isSmallScreenWidth, iouReportID, markReportRHPWidth, transactions], + ); + + const openTransactionFromPreview = useCallback( + (transaction: Transaction) => { + if (contextMenuRef.current?.isContextMenuOpening) { + return; + } + + // A report with a single expense opens the report itself, not the lone expense — opening the + // expense directly would skip the report the user expects to land on. + if (transactions.length <= 1) { + openReportFromPreview(); + return; + } + + // An expense deleted while offline stays in the carousel (see the `transactions` filter above) but its + // thread is already gone, so opening it lands on "It's not here". Open the parent report instead. + if (transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { + openReportFromPreview(); + return; + } + + const childReportID = resolveChildReportID(transaction); + if (childReportID) { + navigateToExpense(childReportID); + return; + } + + // The thread could not be resolved because this expense's IOU action isn't present in OnyxDB. Fetch the report's actions and + // open the expense once the fetch settles, instead of falling back to the parent + // report and losing the pressed expense. Skip this while offline: openReport can't fetch, so the + // deferred press would never fire (dead tap) — fall through to opening the cached parent report + // instead, matching the "View" button. + const isIOUActionLoaded = !!getIOUActionForReportID(transaction.reportID, transaction.transactionID); + if (!isIOUActionLoaded && iouReportID && !isOffline) { + pendingExpenseTransactionRef.current = transaction; + openReport({reportID: iouReportID, introSelected, betas}); + return; + } + + openReportFromPreview(); + }, + [betas, introSelected, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], ); + // Completes a deferred expense press once the IOU report's actions have loaded. + useEffect(() => { + const pendingTransaction = pendingExpenseTransactionRef.current; + // Hold the press while the fetch is in flight — the loading flip back to false re-runs this effect, so the + // press settles even when the fetched actions match the cache and the action count never changes. + if (!pendingTransaction || isLoadingInitialIOUReportActions) { + return; + } + const childReportID = resolveChildReportID(pendingTransaction); + if (childReportID) { + pendingExpenseTransactionRef.current = null; + navigateToExpense(childReportID); + return; + } + // The actions finished loading but the expense still has no resolvable thread — open the parent report. + if (iouReportActionCount) { + pendingExpenseTransactionRef.current = null; + openReportFromPreview(); + } + }, [iouReportActionCount, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); + + const renderItem: ListRenderItem = ({item}) => { + const transactionIOUAction = getIOUActionForReportID(item.reportID, item.transactionID); + return ( + openTransactionFromPreview(item)} + shouldShowPayerAndReceiver={shouldShowPayerAndReceiver} + shouldHighlight={!!newTransactionIDs?.has(item.transactionID)} + /> + ); + }; + return ( { jest.mock('@react-navigation/native'); +jest.mock('@hooks/useNetwork'); +const mockUseNetwork = jest.mocked(useNetwork); + jest.mock('@rnmapbox/maps', () => { return { default: jest.fn(), @@ -74,6 +83,45 @@ jest.mock('@src/hooks/useReportWithTransactionsAndViolations', () => ({ default: (...args: Parameters) => mockUseReportWithTransactionsAndViolations(...args), })); +// Lets a single test force the narrow (mobile) layout. When left undefined every other test +// runs the real hook unchanged, so the existing wide-layout tests keep their behavior. +let mockResponsiveLayoutOverride: ResponsiveLayoutResult | undefined; +jest.mock('@hooks/useResponsiveLayout', () => { + const actual = jest.requireActual<{default: () => ResponsiveLayoutResult}>('@hooks/useResponsiveLayout'); + return { + __esModule: true, + default: () => mockResponsiveLayoutOverride ?? actual.default(), + }; +}); + +const narrowResponsiveLayout: ResponsiveLayoutResult = { + shouldUseNarrowLayout: true, + isSmallScreenWidth: true, + isInNarrowPaneModal: false, + isExtraSmallScreenHeight: false, + isMediumScreenWidth: false, + isLargeScreenWidth: false, + isExtraLargeScreenWidth: false, + isExtraSmallScreenWidth: false, + isSmallScreen: true, + onboardingIsMediumOrLargerScreenWidth: false, + isInLandscapeMode: false, +}; + +const wideResponsiveLayout: ResponsiveLayoutResult = { + shouldUseNarrowLayout: false, + isSmallScreenWidth: false, + isInNarrowPaneModal: false, + isExtraSmallScreenHeight: false, + isMediumScreenWidth: false, + isLargeScreenWidth: true, + isExtraLargeScreenWidth: false, + isExtraSmallScreenWidth: false, + isSmallScreen: false, + onboardingIsMediumOrLargerScreenWidth: true, + isInLandscapeMode: false, +}; + // The preview reads `iouReport` from a prop (provided stable by the parent) and its transactions from the // scoped `useReportTransactionsCollection` hook, so the test drives those two sources directly. let mockIOUReportProp: OnyxEntry = mockIOUReport; @@ -250,6 +298,7 @@ describe('MoneyRequestReportPreview', () => { beforeEach(() => { jest.clearAllMocks(); + mockUseNetwork.mockReturnValue({isOffline: false}); mockDeferredValueOverride = undefined; mockOnHoldMenuOpenHolder.current = undefined; mockHoldMenuPropsHolder.current = undefined; @@ -414,4 +463,282 @@ describe('MoneyRequestReportPreview', () => { expect(screen.queryByText(TestHelper.translateLocal('search.moneyRequestReport.emptyStateTitle'))).not.toBeOnTheScreen(); }); + + describe('pressing a transaction in the carousel', () => { + const navigateSpy = jest.spyOn(Navigation, 'navigate'); + + // Give every transaction its own thread report so the assertion proves the *pressed* card + // drives navigation, instead of every card sharing one parent-report handler. + const buildActionWithThread = (reportID: string | undefined, transactionID: string | undefined) => { + if (!reportID || !transactionID) { + return undefined; + } + return {...mockAction, childReportID: `thread_${transactionID}`, originalMessage: {...mockAction, IOUTransactionID: transactionID}}; + }; + + const renderAndPopulateCarousel = async () => { + renderPage({}); + await waitForBatchedUpdatesWithAct(); + setCurrentWidth(); + await act(async () => { + await Onyx.mergeCollection(ONYXKEYS.COLLECTION.TRANSACTION, mockOnyxTransactions); + await waitForBatchedUpdatesWithAct(); + }); + await waitForBatchedUpdatesWithAct(); + }; + + const pressSecondTransaction = async () => { + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockSecondTransaction); + fireEvent.press(screen.getByText(transactionDisplayAmount)); + await waitForBatchedUpdatesWithAct(); + }; + + beforeEach(() => { + navigateSpy.mockImplementation(() => {}); + jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(''); + // The wide-layout cascade guards its delayed expense navigation on isActiveRoute(reportRoute); default to + // "still on the report" so the happy-path cascade fires. + jest.spyOn(Navigation, 'isActiveRoute').mockReturnValue(true); + }); + + afterEach(() => { + mockResponsiveLayoutOverride = undefined; + // Restore the globally-enabled fake timers in case a test opted into real timers. + jest.useFakeTimers(); + }); + + it('opens the report in the wide RHP and then the pressed expense on top (after a short delay) on wide layouts', async () => { + // The pressed expense opens on a short setTimeout so the report's wide RHP settles first. Use real + // timers so that delayed navigation actually fires + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + // The report opens in the wide RHP first so it sits below, then the pressed expense opens on top + // of it (back returns to the report, not the Inbox). + const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''}); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy).toHaveBeenNthCalledWith(1, reportRoute); + expect(navigateSpy).toHaveBeenNthCalledWith(2, ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); + }); + + it('does not reopen the pressed expense if the user leaves the report during the wide-layout cascade delay', async () => { + // Regression: the report opens, but if the user dismisses its wide RHP (or navigates away) before the + // cascade timer fires, the delayed callback must not reopen the expense over whatever screen is now active. + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + jest.spyOn(Navigation, 'isActiveRoute').mockReturnValue(false); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''}); + expect(navigateSpy).toHaveBeenCalledWith(reportRoute); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); + }); + + it('opens the pressed expense as a plain forward navigation on narrow layouts', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // The pressed expense opens with a single navigate and nothing is inserted beneath it, so back returns to + // the chat and no other flow can trip over a hand-built stack. + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + }); + + it('opens the parent report instead of an expense deleted while offline', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + mockUseNetwork.mockReturnValue({isOffline: true}); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + // Offline deletes stay in the carousel, but the thread is already gone — pressing it must not land on + // "It's not here" (deploy blocker #97149). + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet( + ONYXKEYS.COLLECTION.TRANSACTION, + [mockTransaction, {...mockTransaction, transactionID: mockSecondTransactionID, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}], + (transaction) => transaction.transactionID, + ), + ); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + }); + + it('seeds the optimistic transaction thread before opening an existing (possibly uncached) expense', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + const seedSpy = jest.spyOn(ReportActions, 'setOptimisticTransactionThread').mockImplementation(() => {}); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // The thread already exists but may not be cached (offline / after a cache clear), so its optimistic + // report shell is seeded before navigating — otherwise the tap can land on a blank expense. + expect(seedSpy).toHaveBeenCalledWith(`thread_${mockSecondTransactionID}`, mockIOUReport.reportID, expect.anything(), expect.anything()); + }); + + it('opens the parent report (like View) instead of a dead tap when offline and the thread is unresolved', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + mockUseNetwork.mockReturnValue({isOffline: true}); + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // Offline, and the IOU action isn't loaded, so the thread can't be resolved at press time. + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // openReport can't fetch offline, so rather than leaving the tap dead we open the cached parent report. + expect(openReportSpy).not.toHaveBeenCalled(); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + + it('fetches the report actions and opens the pressed expense once they load, instead of the parent report, after a cache clear', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // Simulate a cache clear: the IOU report's actions are not loaded yet, so the pressed expense's + // thread cannot be resolved at press time. + const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // The press fetches the IOU report's actions and waits, rather than falling back to the parent report. + expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); + expect(navigateSpy).not.toHaveBeenCalled(); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + + // Once the actions arrive the thread resolves and the pressed expense opens (report placed underneath). + getIOUActionSpy.mockImplementation(buildActionWithThread); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_loaded`]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + }); + + it('falls back to the parent report once the re-fetch settles when the expense has no IOU action at all', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // A legacy expense: the IOU report's actions are loaded, but none of them is this expense's IOU + // action, and re-fetching surfaces nothing new. + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[mockAction.reportActionID]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + await pressSecondTransaction(); + + // The press defers and re-fetches the report's actions (the missing action may simply not be cached). + expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); + expect(navigateSpy).not.toHaveBeenCalled(); + + // The fetch settles without changing the cached actions. The loading flip alone must drain the press to + // the parent report — regression: it used to wait for an action-count change that never came, leaving + // the tap permanently dead. + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${mockIOUReport.reportID}`, {isLoadingInitialReportActions: true}); + await waitForBatchedUpdatesWithAct(); + }); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${mockIOUReport.reportID}`, {isLoadingInitialReportActions: false}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + + it('opens the pressed expense after re-fetching when only part of the report actions were cached', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + // Partially seeded cache: some of the report's actions are present (e.g. from the app-wide bootstrap), + // but not the pressed expense's IOU action. + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[mockAction.reportActionID]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + await pressSecondTransaction(); + + // Regression: the press used to give up immediately (parent report) because some actions were cached; + // it must re-fetch instead — the missing IOU action may just not have been seeded. + expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalled(); + + // The fetch lands the missing IOU action — the pressed expense opens (report beneath), not the parent report. + getIOUActionSpy.mockImplementation(buildActionWithThread); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_loaded`]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + }); + + it('falls back to opening the parent report when the pressed expense has no thread', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActions, 'createTransactionThreadReport').mockReturnValue(undefined); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation((reportID, transactionID) => { + if (!reportID || !transactionID) { + return undefined; + } + return {...mockAction, childReportID: undefined, originalMessage: {...mockAction, IOUTransactionID: transactionID}}; + }); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + + it('opens the report instead of the lone expense for a single-expense report', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + setReportPreviewData({transactions: [mockTransaction]}); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + renderPage({}); + await waitForBatchedUpdatesWithAct(); + setCurrentWidth(); + await act(async () => { + await Onyx.mergeCollection(ONYXKEYS.COLLECTION.TRANSACTION, mockOnyxTransactions); + await waitForBatchedUpdatesWithAct(); + }); + await waitForBatchedUpdatesWithAct(); + + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + fireEvent.press(screen.getByText(transactionDisplayAmount)); + await waitForBatchedUpdatesWithAct(); + + // A single-expense report opens the report itself, never the lone expense thread. + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockTransaction.transactionID}`, backTo: ''})); + }); + }); }); diff --git a/tests/ui/components/ReportPreviewActionButtonTest.tsx b/tests/ui/components/ReportPreviewActionButtonTest.tsx index 346d80edccbe..714b10745217 100644 --- a/tests/ui/components/ReportPreviewActionButtonTest.tsx +++ b/tests/ui/components/ReportPreviewActionButtonTest.tsx @@ -106,7 +106,9 @@ describe('ReportPreviewActionButton', () => { mockActionState.connectedIntegration = CONST.POLICY.CONNECTIONS.NAME.QBO; render(); expect(mockExport).toHaveBeenCalled(); - expect(mockView).not.toHaveBeenCalled(); + // The View button now renders alongside the primary action button (here ExportActionButton) rather than + // instead of it, so it is expected to render too. + expect(mockView).toHaveBeenCalled(); }); it('falls back to the View button for EXPORT_TO_ACCOUNTING when no integration is connected', () => { From 00c442952da0ef2f1895de5e23a93788f1fc0a72 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 4 Aug 2026 23:16:43 +0700 Subject: [PATCH 02/31] Open the narrow-layout expense in the RHP and keep deleted rows out of the carousel Narrow layouts now open the pressed expense via ROUTES.SEARCH_REPORT, the same RHP route every other narrow entry point uses, instead of pushing it as a split-navigator screen. Putting the thread in the split stack is what the cleanup flows do not expect: the split-expense save relies on removeScreenByKey, which only filters the ROOT navigator's routes and can never remove a nested split screen (#97183), and it leaves the discarded thread as a focused full-screen report while its data is torn down (#97158). The RHP leaves the split stack exactly as those flows assume. Also stops seeding the expense view's prev/next carousel with offline-deleted rows. The press handler already opened the parent report for them (#97149), but the sibling arrows were seeded from the unfiltered list, so the deleted expense stayed reachable and still landed on 'It's not here'. Every other seeder in the tree filters with isTransactionPendingDelete; this one now does too. Unmarks the expense's RHP width hint when the wide cascade aborts, so a hint can't leak and force an unrelated later open to be wide. --- .../MoneyRequestReportPreview/index.tsx | 36 +++++++++------ tests/ui/MoneyRequestReportPreview.test.tsx | 46 +++++++++++++++---- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index e29e0da01475..549359f752ab 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -25,6 +25,7 @@ import { } from '@libs/ReportActionsUtils'; import {isIOUReport} from '@libs/ReportUtils'; import {startSpan} from '@libs/telemetry/activeSpans'; +import {isTransactionPendingDelete} from '@libs/TransactionUtils'; import Navigation from '@navigation/Navigation'; @@ -67,7 +68,7 @@ function MoneyRequestReportPreview({ const StyleUtils = useStyleUtils(); // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth const {shouldUseNarrowLayout, isSmallScreenWidth} = useResponsiveLayout(); - const {markReportRHPWidth} = useWideRHPActions(); + const {markReportRHPWidth, unmarkReportRHPWidth} = useWideRHPActions(); const personalDetailsList = usePersonalDetails(); const {email: currentUserEmail, accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); @@ -81,6 +82,10 @@ function MoneyRequestReportPreview({ // reimbursable derivations so they include optimistically-deleted rows, exactly as before the decomposition. const allReportTransactions = Object.values(reportTransactionsCollection ?? {}).filter((transaction): transaction is Transaction => !!transaction); const transactions = allReportTransactions.filter((transaction) => isOffline || transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE); + // IDs seeded into the expense view's prev/next carousel. Offline-deleted rows stay visible in the preview (above) + // but their threads are gone, so they must not be reachable through the arrows either — same filter every other + // seeder in the tree applies. + const openableTransactionIDs = transactions.filter((transaction) => !isTransactionPendingDelete(transaction)).map((transaction) => transaction.transactionID); // Tracks how many actions the IOU report has loaded so a deferred expense press can be retried once // the actions arrive (they may be missing right after a cache clear). const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { @@ -212,15 +217,17 @@ function MoneyRequestReportPreview({ }); if (isSmallScreenWidth) { - // Narrow layouts open the pressed expense as a plain forward navigation, so back returns to the chat - // it was opened from. v1 instead inserted the parent report beneath the expense (a spliced stack entry - // on native, an extra history entry on mobile web) so that back stopped on the report first. That - // hand-built stack broke whenever another flow mutated it -- deleting an expense replaced the thread - // with a duplicate report route, the split-expense save left a stale route behind, and on mobile web - // the extra entry forced a state rebuild that flashed the chat on back. Keeping the stack untouched - // costs one intermediate stop and removes that whole class of failure. - setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)); - Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(childReportID, undefined, undefined, Navigation.getActiveRoute())); + // Narrow layouts open the pressed expense in the RHP over the chat — the same route every other + // narrow entry point uses (see useNavigateToTransactionThread). Back returns to the chat. + // + // Deliberately NOT as a split-navigator screen with the parent report placed beneath it. Doing that + // leaves the thread as a full-screen SCREENS.REPORT route inside the split stack, and the flows that + // clean up after a thread assume it is not there: the split-expense save path relies on + // removeScreenByKey, which only filters the ROOT navigator's routes and so can never remove a nested + // split screen, and the delete path's goBack can land on a second copy of the parent report. Keeping + // the expense in the RHP keeps the split stack exactly as those flows expect it. + setActiveTransactionIDs(openableTransactionIDs); + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); return; } @@ -232,7 +239,7 @@ function MoneyRequestReportPreview({ const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); markReportRHPWidth(iouReportID, 'super-wide'); Navigation.navigate(reportRoute); - setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)).then(() => { + setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); // Let the report's wide RHP settle before opening the pressed expense on top, so the two // panels open as a cascade rather than at once. @@ -240,6 +247,9 @@ function MoneyRequestReportPreview({ // The user may have closed the report's wide RHP or navigated away during the cascade delay; // don't reopen the expense over whatever screen is now active. if (!Navigation.isActiveRoute(reportRoute)) { + // Drop the width hint we set for the expense, otherwise it would force that thread to + // open wide later from an unrelated entry point. + unmarkReportRHPWidth(childReportID); return; } Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); @@ -249,12 +259,12 @@ function MoneyRequestReportPreview({ } // Fallback when the parent report is unknown: open the pressed expense alone in the wide RHP. - setActiveTransactionIDs(transactions.map((transaction) => transaction.transactionID)).then(() => { + setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); }); }, - [isSmallScreenWidth, iouReportID, markReportRHPWidth, transactions], + [isSmallScreenWidth, iouReportID, markReportRHPWidth, unmarkReportRHPWidth, openableTransactionIDs], ); const openTransactionFromPreview = useCallback( diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 044bb6eb9e2e..ed586d7c401d 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -15,6 +15,7 @@ import useNetwork from '@hooks/useNetwork'; import type ResponsiveLayoutResult from '@hooks/useResponsiveLayout/types'; import * as ReportActions from '@libs/actions/Report'; +import * as TransactionThreadNavigation from '@libs/actions/TransactionThreadNavigation'; import DateUtils from '@libs/DateUtils'; import Navigation from '@libs/Navigation/Navigation'; import {getFormattedCreated, isManagedCardTransaction} from '@libs/TransactionUtils'; @@ -551,17 +552,44 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); }); - it('opens the pressed expense as a plain forward navigation on narrow layouts', async () => { + it('opens the pressed expense in the RHP over the chat on narrow layouts', async () => { mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); await renderAndPopulateCarousel(); await pressSecondTransaction(); - // The pressed expense opens with a single navigate and nothing is inserted beneath it, so back returns to - // the chat and no other flow can trip over a hand-built stack. - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); - expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + // The expense opens in the RHP, leaving the split stack untouched — the flows that clean up after a + // thread (split-expense save, delete) assume the thread is not a split-navigator screen. + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + }); + + it('keeps an offline-deleted sibling out of the expense view prev/next carousel', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + mockUseNetwork.mockReturnValue({isOffline: true}); + const setActiveTransactionIDsSpy = jest.spyOn(TransactionThreadNavigation, 'setActiveTransactionIDs'); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + // First row is live, second is delete-pending. Offline keeps the deleted row visible in the carousel. + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet( + ONYXKEYS.COLLECTION.TRANSACTION, + [mockTransaction, {...mockTransaction, transactionID: mockSecondTransactionID, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}], + (transaction) => transaction.transactionID, + ), + ); + + await renderAndPopulateCarousel(); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const [liveRow] = screen.getAllByText(transactionDisplayAmount); + fireEvent.press(liveRow); + await waitForBatchedUpdatesWithAct(); + + // Pressing the LIVE row must not seed the deleted sibling, otherwise the RHP's next arrow opens a thread + // that no longer exists and lands on "It's not here" (deploy blocker #97149, arrow path). + expect(setActiveTransactionIDsSpy).toHaveBeenCalled(); + const seededIDs = setActiveTransactionIDsSpy.mock.calls.at(-1)?.at(0); + expect(seededIDs).not.toContain(mockSecondTransactionID); }); it('opens the parent report instead of an expense deleted while offline', async () => { @@ -626,7 +654,7 @@ describe('MoneyRequestReportPreview', () => { // The press fetches the IOU report's actions and waits, rather than falling back to the parent report. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); expect(navigateSpy).not.toHaveBeenCalled(); - expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); // Once the actions arrive the thread resolves and the pressed expense opens (report placed underneath). getIOUActionSpy.mockImplementation(buildActionWithThread); @@ -635,7 +663,7 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }); - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); }); it('falls back to the parent report once the re-fetch settles when the expense has no IOU action at all', async () => { @@ -688,7 +716,7 @@ describe('MoneyRequestReportPreview', () => { // Regression: the press used to give up immediately (parent report) because some actions were cached; // it must re-fetch instead — the missing IOU action may just not have been seeded. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); - expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); expect(navigateSpy).not.toHaveBeenCalled(); // The fetch lands the missing IOU action — the pressed expense opens (report beneath), not the parent report. @@ -698,7 +726,7 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }); - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); }); From d7be269947a588a43b95713871fa7fbcb617ae51 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 05:12:54 +0700 Subject: [PATCH 03/31] Hydrate the report's actions when a thread resolves without them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveChildReportID falls back to the transaction's own transactionThreadReportID when the IOU action isn't in OnyxDB (cache clear, or a chat whose expense report was never opened). v1 got away with that because the narrow path pushed the parent report first, which mounted the report screen and fetched its actions. v2 opens the expense directly, so nothing hydrated them — and the expense view's prev/next carousel resolves each sibling through those actions. With them missing the carousel could not find the sibling's existing thread and would mint a parentless duplicate thread instead, landing on an empty expense view. Fetch the actions in the background when we take that fallback; the press still opens immediately. Also clears the seeded sibling IDs when the wide cascade aborts. The screen that clears them on unmount is never reached in that case, so they would outlive the interaction. --- .../MoneyRequestReportPreview/index.tsx | 17 ++++++++--- tests/ui/MoneyRequestReportPreview.test.tsx | 28 +++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 549359f752ab..ef205dcf3c6b 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -14,7 +14,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; import useTransactionViolations from '@hooks/useTransactionViolations'; import {createTransactionThreadReport, openReport, setOptimisticTransactionThread} from '@libs/actions/Report'; -import {setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; +import {clearActiveTransactionIDs, setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import { getIOUActionForReportID, @@ -247,9 +247,11 @@ function MoneyRequestReportPreview({ // The user may have closed the report's wide RHP or navigated away during the cascade delay; // don't reopen the expense over whatever screen is now active. if (!Navigation.isActiveRoute(reportRoute)) { - // Drop the width hint we set for the expense, otherwise it would force that thread to - // open wide later from an unrelated entry point. + // The expense never opened, so drop what we staged for it: the width hint would force + // that thread to open wide later from an unrelated entry point, and the seeded sibling + // IDs would never reach the screen that clears them on unmount. unmarkReportRHPWidth(childReportID); + clearActiveTransactionIDs(); return; } Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); @@ -287,8 +289,16 @@ function MoneyRequestReportPreview({ return; } + const isIOUActionLoaded = !!getIOUActionForReportID(transaction.reportID, transaction.transactionID); const childReportID = resolveChildReportID(transaction); if (childReportID) { + // The thread resolved from the transaction's own transactionThreadReportID rather than from a loaded + // IOU action, which means the report's actions aren't in OnyxDB yet. The expense view's prev/next + // carousel resolves each sibling through those actions, so fetch them in the background — without + // them an arrow press can't resolve the sibling's existing thread and mints a parentless duplicate. + if (!isIOUActionLoaded && iouReportID && !isOffline) { + openReport({reportID: iouReportID, introSelected, betas}); + } navigateToExpense(childReportID); return; } @@ -298,7 +308,6 @@ function MoneyRequestReportPreview({ // report and losing the pressed expense. Skip this while offline: openReport can't fetch, so the // deferred press would never fire (dead tap) — fall through to opening the cached parent report // instead, matching the "View" button. - const isIOUActionLoaded = !!getIOUActionForReportID(transaction.reportID, transaction.transactionID); if (!isIOUActionLoaded && iouReportID && !isOffline) { pendingExpenseTransactionRef.current = transaction; openReport({reportID: iouReportID, introSelected, betas}); diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index ed586d7c401d..442259bc34be 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -565,6 +565,34 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); }); + it('fetches the report actions when the thread resolved only from the transaction, so the carousel can resolve siblings', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // Cache-clear shape: the IOU report's actions are absent, but each transaction still carries its own + // transactionThreadReportID, so the press resolves a thread WITHOUT loading the report's actions. + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet( + ONYXKEYS.COLLECTION.TRANSACTION, + [ + {...mockTransaction, transactionThreadReportID: `thread_${mockTransaction.transactionID}`}, + {...mockTransaction, transactionID: mockSecondTransactionID, transactionThreadReportID: `thread_${mockSecondTransactionID}`}, + ], + (transaction) => transaction.transactionID, + ), + ); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // The expense opens straight away from the transaction's own thread id... + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + // ...but the report's actions must still be fetched. The prev/next carousel resolves each sibling through + // those actions; without them an arrow press cannot find the sibling's existing thread and mints a + // parentless duplicate instead. + expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); + }); + it('keeps an offline-deleted sibling out of the expense view prev/next carousel', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; mockUseNetwork.mockReturnValue({isOffline: true}); From f0a606be346072b1f4beed569e60e90cd8a1ce8b Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 05:17:00 +0700 Subject: [PATCH 04/31] Make the report-preview action row layout testable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite mocked useThemeStyles as {flex1: {}}, so styles.flexRow and styles.gap2 resolved to undefined and were dropped from the style array — the two-button row that issue #91042 asks for was completely untested and a typo'd style key would have passed. Give the mock real sentinel styles and assert the row is applied when a primary action is present and NOT applied when View stands alone. --- .../ReportPreviewActionButtonTest.tsx | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/ui/components/ReportPreviewActionButtonTest.tsx b/tests/ui/components/ReportPreviewActionButtonTest.tsx index 714b10745217..2f3cad6c6319 100644 --- a/tests/ui/components/ReportPreviewActionButtonTest.tsx +++ b/tests/ui/components/ReportPreviewActionButtonTest.tsx @@ -5,9 +5,11 @@ import ReportPreviewActionButton from '@components/ReportActionItem/MoneyRequest import CONST from '@src/CONST'; import type {ConnectionName} from '@src/types/onyx/Policy'; +import type {StyleProp, ViewStyle} from 'react-native'; import type {ValueOf} from 'type-fest'; import React from 'react'; +import {View} from 'react-native'; // The dispatcher owns no props and reads its decision from context, so drive the decision through the mocked context // slice and stub each branch component with a spy so we can assert which one gets rendered for a given action. @@ -79,7 +81,11 @@ jest.mock('@components/ButtonComposed', () => { }; }); -jest.mock('@hooks/useThemeStyles', () => ({__esModule: true, default: () => ({flex1: {}})})); +// Sentinel style objects so the row layout is actually observable. With `{flex1: {}}` alone, `styles.flexRow` and +// `styles.gap2` resolved to undefined and were silently dropped from the style array, leaving the two-button row +// completely untested — a typo'd style key would have passed. +const mockStyles = {flex1: {flex: 1}, flexRow: {flexDirection: 'row'}, gap2: {gap: 8}}; +jest.mock('@hooks/useThemeStyles', () => ({__esModule: true, default: () => mockStyles})); jest.mock('@hooks/useLocalize', () => ({__esModule: true, default: () => ({translate: (key: string) => key})})); describe('ReportPreviewActionButton', () => { @@ -118,4 +124,20 @@ describe('ReportPreviewActionButton', () => { expect(mockView).toHaveBeenCalled(); expect(mockExport).not.toHaveBeenCalled(); }); + it('lays the primary action and View out in a row, and keeps a lone View full-width', () => { + // Issue #91042 adds the grey View button beside the primary action. The row is width-capped, so the layout + // styles are load-bearing: without flexRow/gap2 the two buttons stack instead of sitting side by side. + mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.PAY; + const withPrimary = render(); + const rowStyle = withPrimary.UNSAFE_getAllByType(View).at(0)?.props.style as StyleProp; + expect(rowStyle).toEqual(expect.arrayContaining([mockStyles.flexRow, mockStyles.gap2])); + withPrimary.unmount(); + + // With no primary action the View button stands alone and must NOT be laid out as a row. + jest.clearAllMocks(); + mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.VIEW; + const viewOnly = render(); + const soloStyle = viewOnly.UNSAFE_getAllByType(View).at(0)?.props.style as StyleProp; + expect(soloStyle).not.toEqual(expect.arrayContaining([mockStyles.flexRow])); + }); }); From 95697ada42e3597fa5be273e1fd9cced383aac8d Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 05:20:09 +0700 Subject: [PATCH 05/31] Read the action row style without an unsafe cast --- .../components/ReportPreviewActionButtonTest.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/ui/components/ReportPreviewActionButtonTest.tsx b/tests/ui/components/ReportPreviewActionButtonTest.tsx index 2f3cad6c6319..caffa0e1e432 100644 --- a/tests/ui/components/ReportPreviewActionButtonTest.tsx +++ b/tests/ui/components/ReportPreviewActionButtonTest.tsx @@ -5,7 +5,6 @@ import ReportPreviewActionButton from '@components/ReportActionItem/MoneyRequest import CONST from '@src/CONST'; import type {ConnectionName} from '@src/types/onyx/Policy'; -import type {StyleProp, ViewStyle} from 'react-native'; import type {ValueOf} from 'type-fest'; import React from 'react'; @@ -88,6 +87,13 @@ const mockStyles = {flex1: {flex: 1}, flexRow: {flexDirection: 'row'}, gap2: {ga jest.mock('@hooks/useThemeStyles', () => ({__esModule: true, default: () => mockStyles})); jest.mock('@hooks/useLocalize', () => ({__esModule: true, default: () => ({translate: (key: string) => key})})); +// Reads the style array off the component's outermost View without an unsafe cast. +function flattenContainerStyle(rendered: ReturnType): unknown[] { + const container = rendered.UNSAFE_getAllByType(View).at(0); + const style: unknown = container?.props.style; + return Array.isArray(style) ? style : [style]; +} + describe('ReportPreviewActionButton', () => { beforeEach(() => { jest.clearAllMocks(); @@ -129,15 +135,13 @@ describe('ReportPreviewActionButton', () => { // styles are load-bearing: without flexRow/gap2 the two buttons stack instead of sitting side by side. mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.PAY; const withPrimary = render(); - const rowStyle = withPrimary.UNSAFE_getAllByType(View).at(0)?.props.style as StyleProp; - expect(rowStyle).toEqual(expect.arrayContaining([mockStyles.flexRow, mockStyles.gap2])); + expect(flattenContainerStyle(withPrimary)).toEqual(expect.arrayContaining([mockStyles.flexRow, mockStyles.gap2])); withPrimary.unmount(); // With no primary action the View button stands alone and must NOT be laid out as a row. jest.clearAllMocks(); mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.VIEW; const viewOnly = render(); - const soloStyle = viewOnly.UNSAFE_getAllByType(View).at(0)?.props.style as StyleProp; - expect(soloStyle).not.toEqual(expect.arrayContaining([mockStyles.flexRow])); + expect(flattenContainerStyle(viewOnly)).not.toEqual(expect.arrayContaining([mockStyles.flexRow])); }); }); From cd9a6da1f17fb9c412738262df658132fd6f2779 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 05:34:38 +0700 Subject: [PATCH 06/31] Add UI regression tests for offline delete-pending rendering and online seeding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps the existing suite left open: - No test asserted that an offline delete-pending expense is still RENDERED. Issue #26939 (and OfflineWithFeedback's own rule, which hides a pending-delete item only when ONLINE) require the card to stay visible and greyed while offline; v2 only makes it non-navigable. Verified the test fails if the display filter stops honouring isOffline. - No test pinned the new openableTransactionIDs list as an offline-only refinement. Online, a delete-pending row is already filtered upstream, so the seeded array must still equal the full visible list — this catches anyone widening the predicate and silently dropping live siblings from the prev/next carousel. --- tests/ui/MoneyRequestReportPreview.test.tsx | 35 +++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 442259bc34be..e8c2e8bdb8b7 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -593,6 +593,41 @@ describe('MoneyRequestReportPreview', () => { expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); }); + it('seeds every transaction when online, so the new filter changes nothing there', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + mockUseNetwork.mockReturnValue({isOffline: false}); + const setActiveTransactionIDsSpy = jest.spyOn(TransactionThreadNavigation, 'setActiveTransactionIDs'); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // Online, a delete-pending row is already filtered out of `transactions` upstream, so openableTransactionIDs + // must equal the full visible list. This pins the filter as an offline-only refinement — it would fail if + // someone widened the predicate (e.g. to one that also matches pendingFields) and started dropping live rows. + expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith(defaultPreviewTransactions.map((transaction) => transaction.transactionID)); + }); + + it('still renders an offline-deleted expense card in the carousel', async () => { + mockResponsiveLayoutOverride = wideResponsiveLayout; + mockUseNetwork.mockReturnValue({isOffline: true}); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet( + ONYXKEYS.COLLECTION.TRANSACTION, + [mockTransaction, {...mockTransaction, transactionID: mockSecondTransactionID, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}], + (transaction) => transaction.transactionID, + ), + ); + + await renderAndPopulateCarousel(); + + // Issue #26939: deleting an expense offline must leave the preview VISIBLE (greyed out) rather than + // collapsing it. v2 only makes that row non-navigable — it must not disappear, so both cards still render. + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + expect(screen.getAllByText(transactionDisplayAmount).length).toBeGreaterThanOrEqual(2); + }); + it('keeps an offline-deleted sibling out of the expense view prev/next carousel', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; mockUseNetwork.mockReturnValue({isOffline: true}); From 1bc259aea4da1aaa7992d42858e8c3bfcc2b6484 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 05:47:56 +0700 Subject: [PATCH 07/31] Stop a deferred expense press from hijacking a later one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pendingExpenseTransactionRef was only ever cleared by the drain effect, so a press that deferred waiting on the report's actions stayed armed. Pressing a second card that resolved immediately opened that expense, but when the first press's fetch landed the drain fired and navigated away to the first expense instead — the user ends up on a card they pressed several seconds earlier. Clear the pending press at the top of every press so the latest one always wins. --- .../MoneyRequestReportPreview/index.tsx | 5 +++ tests/ui/MoneyRequestReportPreview.test.tsx | 38 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index ef205dcf3c6b..6772acd3f6c9 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -275,6 +275,11 @@ function MoneyRequestReportPreview({ return; } + // Any new press supersedes one still waiting on a fetch. Without this an earlier deferred press stays + // armed and hijacks the navigation the moment its fetch lands — sending the user to the expense they + // pressed first rather than the one they pressed last. + pendingExpenseTransactionRef.current = null; + // A report with a single expense opens the report itself, not the lone expense — opening the // expense directly would skip the report the user expects to land on. if (transactions.length <= 1) { diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index e8c2e8bdb8b7..f517a20121db 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -593,6 +593,44 @@ describe('MoneyRequestReportPreview', () => { expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); }); + it('does not let a deferred press hijack a later press once its fetch lands', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // First card has no resolvable thread (its press defers); second card resolves immediately. + const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet( + ONYXKEYS.COLLECTION.TRANSACTION, + [mockTransaction, {...mockTransaction, transactionID: mockSecondTransactionID, transactionThreadReportID: `thread_${mockSecondTransactionID}`}], + (transaction) => transaction.transactionID, + ), + ); + + await renderAndPopulateCarousel(); + + // Press the first card — it defers, waiting on the report's actions. + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const [firstCard] = screen.getAllByText(transactionDisplayAmount); + fireEvent.press(firstCard); + await waitForBatchedUpdatesWithAct(); + expect(navigateSpy).not.toHaveBeenCalled(); + + // Now press the second card, which opens straight away. That is the expense the user is waiting on. + navigateSpy.mockClear(); + await pressSecondTransaction(); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + + // The first press's fetch finally lands. It must NOT navigate — the user has moved on. + navigateSpy.mockClear(); + getIOUActionSpy.mockImplementation(buildActionWithThread); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_late`]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).not.toHaveBeenCalled(); + }); + it('seeds every transaction when online, so the new filter changes nothing there', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; mockUseNetwork.mockReturnValue({isOffline: false}); From 87ee423e3a9f3e0808f3b456fa562c470daae4bf Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 08:14:00 +0700 Subject: [PATCH 08/31] Reword a comment so it passes spellcheck --- .../ReportActionItem/MoneyRequestReportPreview/index.tsx | 2 +- tests/ui/MoneyRequestReportPreview.test.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 6772acd3f6c9..0697debe85a7 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -300,7 +300,7 @@ function MoneyRequestReportPreview({ // The thread resolved from the transaction's own transactionThreadReportID rather than from a loaded // IOU action, which means the report's actions aren't in OnyxDB yet. The expense view's prev/next // carousel resolves each sibling through those actions, so fetch them in the background — without - // them an arrow press can't resolve the sibling's existing thread and mints a parentless duplicate. + // them an arrow press can't resolve the sibling's existing thread and mints a duplicate with no parent. if (!isIOUActionLoaded && iouReportID && !isOffline) { openReport({reportID: iouReportID, introSelected, betas}); } diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index f517a20121db..9a330a40e595 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -589,7 +589,7 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); // ...but the report's actions must still be fetched. The prev/next carousel resolves each sibling through // those actions; without them an arrow press cannot find the sibling's existing thread and mints a - // parentless duplicate instead. + // duplicate thread with no parent instead. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); }); From e00a24f3cf34e329e747d950705f792003c1dc44 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 12:14:14 +0700 Subject: [PATCH 09/31] Cancel staged navigation when the user chooses something else MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three confirmed races, all in the report preview's press handling: - A deferred press was armed until another CARD press cleared it. Opening the report instead ('View', or the preview itself) left it armed, so when the report-actions fetch landed the app navigated to that expense from wherever the user had gone. The View button is wired to a separate openReportFromPreview in the provider, so clearing it in this file's handler is not enough: record the route the press was made on and drop the press if the active route has moved by the time the fetch settles. - The wide cascade's setTimeout was never cancelled. A second press within the 180ms delay let the first press's timer open the wrong expense and then run its cleanup over the expense actually on screen. Keep a handle and clear it on any later press, on opening the report, and on unmount. - The cascade's abort path cleared the sibling-transaction IDs unconditionally. That key is global, so an abort could wipe a carousel another flow had seeded during the delay. Only clear it if it still holds what this press wrote. Also stops opening a thread shell that has no parent action while offline: with the IOU action absent and no way to fetch it, open the parent report instead — the same fallback the deferred path already uses. --- .../MoneyRequestReportPreview/index.tsx | 88 +++++++++++++++---- tests/ui/MoneyRequestReportPreview.test.tsx | 31 +++++++ 2 files changed, 100 insertions(+), 19 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 0697debe85a7..c7bbb0567ff9 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -14,7 +14,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; import useTransactionViolations from '@hooks/useTransactionViolations'; import {createTransactionThreadReport, openReport, setOptimisticTransactionThread} from '@libs/actions/Report'; -import {clearActiveTransactionIDs, setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; +import {clearActiveTransactionIDs, getActiveTransactionIDs, setActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import { getIOUActionForReportID, @@ -98,7 +98,15 @@ function MoneyRequestReportPreview({ }); // Holds a pressed transaction whose thread report could not be resolved yet, so the expense can be // opened once the IOU report's actions have loaded instead of falling back to the parent report. - const pendingExpenseTransactionRef = useRef(null); + // The deferred press, plus the route the user was on when they made it. The route is what tells us the press is + // still wanted: the preview stays mounted (and focused) behind an RHP, and the "View" button is wired to a + // different openReportFromPreview in the provider, so neither unmounting nor our own handler can be relied on to + // cancel it. If the active route has moved on by the time the fetch lands, the user has chosen something else. + const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); + // Handle for the wide-layout cascade's pending expense navigation, so a later press (or unmount) can cancel it. + // Without this a second press within the delay lets the first press's timer open the wrong expense and then run + // its cleanup over the expense that is actually on screen. + const cascadeTimerRef = useRef | null>(null); const policy = usePolicy(policyID); const lastTransaction = transactions?.at(0); const lastTransactionViolations = useTransactionViolations(lastTransaction?.transactionID); @@ -154,6 +162,15 @@ function MoneyRequestReportPreview({ return; } + // Opening the report is an explicit choice, so it supersedes anything an earlier press staged: a deferred + // press would otherwise navigate away when its fetch lands, and a pending cascade would cover this report + // with an expense the user did not just ask for. + pendingExpenseTransactionRef.current = null; + if (cascadeTimerRef.current) { + clearTimeout(cascadeTimerRef.current); + cascadeTimerRef.current = null; + } + startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreview', op: CONST.TELEMETRY.SPAN_OPEN_REPORT, @@ -243,15 +260,19 @@ function MoneyRequestReportPreview({ markReportRHPWidth(childReportID, 'wide'); // Let the report's wide RHP settle before opening the pressed expense on top, so the two // panels open as a cascade rather than at once. - setTimeout(() => { + cascadeTimerRef.current = setTimeout(() => { + cascadeTimerRef.current = null; // The user may have closed the report's wide RHP or navigated away during the cascade delay; // don't reopen the expense over whatever screen is now active. if (!Navigation.isActiveRoute(reportRoute)) { - // The expense never opened, so drop what we staged for it: the width hint would force - // that thread to open wide later from an unrelated entry point, and the seeded sibling - // IDs would never reach the screen that clears them on unmount. + // The expense never opened, so drop the width hint we staged for it — otherwise it would + // force that thread to open wide later from an unrelated entry point. unmarkReportRHPWidth(childReportID); - clearActiveTransactionIDs(); + // Only drop the seeded sibling IDs if they are still the ones this press wrote. Another + // flow may have seeded its own carousel during the delay, and clearing is global. + if (getActiveTransactionIDs().ids === openableTransactionIDs) { + clearActiveTransactionIDs(); + } return; } Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); @@ -275,10 +296,14 @@ function MoneyRequestReportPreview({ return; } - // Any new press supersedes one still waiting on a fetch. Without this an earlier deferred press stays - // armed and hijacks the navigation the moment its fetch lands — sending the user to the expense they - // pressed first rather than the one they pressed last. + // Any new press supersedes what an earlier one staged. Without this an earlier deferred press stays + // armed and hijacks the navigation the moment its fetch lands, and an in-flight cascade timer opens the + // previously pressed expense over the one the user just chose. pendingExpenseTransactionRef.current = null; + if (cascadeTimerRef.current) { + clearTimeout(cascadeTimerRef.current); + cascadeTimerRef.current = null; + } // A report with a single expense opens the report itself, not the lone expense — opening the // expense directly would skip the report the user expects to land on. @@ -297,11 +322,17 @@ function MoneyRequestReportPreview({ const isIOUActionLoaded = !!getIOUActionForReportID(transaction.reportID, transaction.transactionID); const childReportID = resolveChildReportID(transaction); if (childReportID) { - // The thread resolved from the transaction's own transactionThreadReportID rather than from a loaded - // IOU action, which means the report's actions aren't in OnyxDB yet. The expense view's prev/next - // carousel resolves each sibling through those actions, so fetch them in the background — without - // them an arrow press can't resolve the sibling's existing thread and mints a duplicate with no parent. - if (!isIOUActionLoaded && iouReportID && !isOffline) { + if (!isIOUActionLoaded && iouReportID) { + // The thread resolved from the transaction's own transactionThreadReportID rather than from a + // loaded IOU action, so the report's actions aren't in OnyxDB. Offline we cannot fetch them, and + // the thread shell we would open has no parent action to render from — open the parent report + // instead, the same fallback the deferred path uses. Online, fetch them in the background: the + // expense view's prev/next carousel resolves each sibling through those actions, and without + // them an arrow press mints a duplicate thread with no parent. + if (isOffline) { + openReportFromPreview(); + return; + } openReport({reportID: iouReportID, introSelected, betas}); } navigateToExpense(childReportID); @@ -314,7 +345,7 @@ function MoneyRequestReportPreview({ // deferred press would never fire (dead tap) — fall through to opening the cached parent report // instead, matching the "View" button. if (!isIOUActionLoaded && iouReportID && !isOffline) { - pendingExpenseTransactionRef.current = transaction; + pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; openReport({reportID: iouReportID, introSelected, betas}); return; } @@ -326,12 +357,19 @@ function MoneyRequestReportPreview({ // Completes a deferred expense press once the IOU report's actions have loaded. useEffect(() => { - const pendingTransaction = pendingExpenseTransactionRef.current; + const pendingPress = pendingExpenseTransactionRef.current; // Hold the press while the fetch is in flight — the loading flip back to false re-runs this effect, so the // press settles even when the fetched actions match the cache and the action count never changes. - if (!pendingTransaction || isLoadingInitialIOUReportActions) { + if (!pendingPress || isLoadingInitialIOUReportActions) { + return; + } + // The user went somewhere else while the fetch was in flight (opened the report with "View", followed another + // link, changed tab). Replaying now would yank them out of the screen they chose, so drop the press. + if (!isFocused || Navigation.getActiveRoute() !== pendingPress.originRoute) { + pendingExpenseTransactionRef.current = null; return; } + const pendingTransaction = pendingPress.transaction; const childReportID = resolveChildReportID(pendingTransaction); if (childReportID) { pendingExpenseTransactionRef.current = null; @@ -343,7 +381,19 @@ function MoneyRequestReportPreview({ pendingExpenseTransactionRef.current = null; openReportFromPreview(); } - }, [iouReportActionCount, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); + }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); + + // Drop a pending cascade if this preview goes away, so its timer cannot navigate or clean up after unmount. + useEffect( + () => () => { + if (!cascadeTimerRef.current) { + return; + } + clearTimeout(cascadeTimerRef.current); + cascadeTimerRef.current = null; + }, + [], + ); const renderItem: ListRenderItem = ({item}) => { const transactionIOUAction = getIOUActionForReportID(item.reportID, item.transactionID); diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 9a330a40e595..7fc515dee3e3 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -25,6 +25,7 @@ import * as ReportActionUtils from '@src/libs/ReportActionsUtils'; import {getReportName} from '@src/libs/ReportNameUtils'; import * as ReportUtils from '@src/libs/ReportUtils'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {Route} from '@src/ROUTES'; import ROUTES from '@src/ROUTES'; import type {Report, Transaction, TransactionViolation, TransactionViolations} from '@src/types/onyx'; import type {PaymentMethodType} from '@src/types/onyx/OriginalMessage'; @@ -593,6 +594,36 @@ describe('MoneyRequestReportPreview', () => { expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); }); + it('does not let a deferred press hijack an explicit "View" of the report', async () => { + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + // Cold cache: the press cannot resolve a thread, so it defers and nothing opens. + const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + expect(navigateSpy).not.toHaveBeenCalled(); + + // The tap looked dead, so the user opens the report the other way. That is an explicit choice and must + // supersede the deferred press — otherwise the fetch landing yanks them into the expense. + navigateSpy.mockClear(); + fireEvent.press(screen.getByText(TestHelper.translateLocal('common.view'))); + await waitForBatchedUpdatesWithAct(); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + // The app is now on the report, not the chat the press was made from. The suite pins getActiveRoute to a + // constant, so model the real navigation for the assertion below to mean anything. + jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID) as Route); + + navigateSpy.mockClear(); + getIOUActionSpy.mockImplementation(buildActionWithThread); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_late`]: mockAction}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).not.toHaveBeenCalled(); + }); + it('does not let a deferred press hijack a later press once its fetch lands', async () => { mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); From b34fdbeb440a42eb10cac9ba535a43937c25af13 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 5 Aug 2026 13:19:20 +0700 Subject: [PATCH 10/31] Cover the narrow fallback route and the RHP width mechanism MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two behaviors the suite could not see: - Every fallback assertion ran on a wide layout, so the narrow branch of openReportFromPreview had no coverage at all — the whole branch could be deleted and the suite stayed green. Narrow has no super-wide RHP, and this is the route a deleted-expense tap and an offline dead tap both land on. - The reserved RHP widths never reach the rendered output, so every mark/unmark call could be removed without failing anything. Assert the report is widened, the pressed expense is widened, and that the expense's width is released when the press is abandoned. Both were checked by deleting the code under test and confirming the new tests fail. --- tests/ui/MoneyRequestReportPreview.test.tsx | 57 +++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 7fc515dee3e3..591d55bf35da 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -10,6 +10,7 @@ import type ReportPreviewActionButton from '@components/ReportActionItem/MoneyRe import type {MoneyRequestReportPreviewProps} from '@components/ReportActionItem/MoneyRequestReportPreview/types'; import ScreenWrapper from '@components/ScreenWrapper'; import {ShowContextMenuActionsContext, ShowContextMenuStateContext} from '@components/ShowContextMenuContext'; +import type * as WideRHPContextProvider from '@components/WideRHPContextProvider'; import useNetwork from '@hooks/useNetwork'; import type ResponsiveLayoutResult from '@hooks/useResponsiveLayout/types'; @@ -156,6 +157,18 @@ jest.mock('@components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewA }; }); +// The preview widens the RHP for the report it opens and narrows it back when a press is abandoned. Nothing in the +// rendered output reflects that, so capture the calls to assert the widths are actually requested and released. +const mockMarkReportRHPWidth = jest.fn(); +const mockUnmarkReportRHPWidth = jest.fn(); +jest.mock('@components/WideRHPContextProvider', () => ({ + ...jest.requireActual('@components/WideRHPContextProvider'), + useWideRHPActions: () => ({ + markReportRHPWidth: mockMarkReportRHPWidth, + unmarkReportRHPWidth: mockUnmarkReportRHPWidth, + }), +})); + // Capture the props the preview forwards to the hold menu so the selected bank account that reaches it can be asserted. const mockHoldMenuPropsHolder: {current?: {isVisible?: boolean; paymentType?: PaymentMethodType; methodID?: number}} = {current: undefined}; jest.mock('@components/ProcessMoneyReportHoldMenu', () => ({ @@ -878,6 +891,50 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); }); + it('falls back to the full report view, not the super-wide RHP, when the pressed expense has no thread on narrow layouts', async () => { + // Every other fallback assertion here runs wide. Narrow has no super-wide RHP, so the fallback has to + // land on the report screen itself — the route the deleted-expense and offline dead-tap paths rely on. + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActions, 'createTransactionThreadReport').mockReturnValue(undefined); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation((reportID, transactionID) => { + if (!reportID || !transactionID) { + return undefined; + } + return {...mockAction, childReportID: undefined, originalMessage: {...mockAction, IOUTransactionID: transactionID}}; + }); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + + it('widens the RHP for the report and the pressed expense, and narrows the expense back when the press is abandoned', async () => { + // The widths are invisible in the rendered output, so without this the whole widen/release mechanism + // could be deleted and every other test here would still pass. + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + // The user leaves the report before the cascade fires, so the expense's reserved width must be released. + jest.spyOn(Navigation, 'isActiveRoute').mockReturnValue(false); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + expect(mockMarkReportRHPWidth).toHaveBeenCalledWith(mockIOUReport.reportID, 'super-wide'); + expect(mockMarkReportRHPWidth).toHaveBeenCalledWith(`thread_${mockSecondTransactionID}`, 'wide'); + expect(mockUnmarkReportRHPWidth).not.toHaveBeenCalled(); + + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + expect(mockUnmarkReportRHPWidth).toHaveBeenCalledWith(`thread_${mockSecondTransactionID}`); + }); + it('opens the report instead of the lone expense for a single-expense report', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; setReportPreviewData({transactions: [mockTransaction]}); From b7d4a81cb676f02cb65dcc602f1844e480fbbe83 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Sat, 8 Aug 2026 22:02:02 +0700 Subject: [PATCH 11/31] Open the report beneath the pressed expense on narrow layouts Restores the original back order on narrow: back from the expense returns to the report, and back again to the chat, matching how the wide layout already cascades. The expense still opens in the RHP rather than as a screen inside the split navigator. That distinction is what the deploy blockers came from, not the back order. A thread pushed into the split stack is a full-screen report route, and the flows that clean up after a thread cannot reach it there: the split-expense save path calls removeScreenByKey, which only filters the root navigator's routes and so can never remove a nested split screen, and the delete path's goBack can land on a second copy of the report. Opening the report as an ordinary split screen and keeping only the expense in the RHP gives the same back order while leaving the split stack exactly as those flows expect it. The cascade reuses the wide layout's timer, including the guard that drops the delayed navigation when the user has moved on and only clears the seeded sibling IDs when they still belong to this press. --- .../MoneyRequestReportPreview/index.tsx | 42 +++++++-- tests/ui/MoneyRequestReportPreview.test.tsx | 91 +++++++++++++++++-- 2 files changed, 115 insertions(+), 18 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index c7bbb0567ff9..2cc62e2c44f8 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -233,16 +233,42 @@ function MoneyRequestReportPreview({ op: CONST.TELEMETRY.SPAN_OPEN_REPORT, }); - if (isSmallScreenWidth) { - // Narrow layouts open the pressed expense in the RHP over the chat — the same route every other - // narrow entry point uses (see useNavigateToTransactionThread). Back returns to the chat. + if (isSmallScreenWidth && iouReportID) { + // Narrow layouts open the report first and then the pressed expense on top of it, so back returns to + // the report and back again to the chat — the same order the wide layout cascades in. // - // Deliberately NOT as a split-navigator screen with the parent report placed beneath it. Doing that - // leaves the thread as a full-screen SCREENS.REPORT route inside the split stack, and the flows that - // clean up after a thread assume it is not there: the split-expense save path relies on + // The expense opens in the RHP, deliberately NOT as a split-navigator screen with the report placed + // beneath it. A thread pushed into the split stack is a full-screen SCREENS.REPORT route, and the + // flows that clean up after a thread assume it is not there: the split-expense save path relies on // removeScreenByKey, which only filters the ROOT navigator's routes and so can never remove a nested - // split screen, and the delete path's goBack can land on a second copy of the parent report. Keeping - // the expense in the RHP keeps the split stack exactly as those flows expect it. + // split screen, and the delete path's goBack can land on a second copy of the report. Opening the + // report as an ordinary split screen and keeping only the expense in the RHP gives the same back + // order without putting the thread anywhere those flows cannot reach it. + const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); + Navigation.navigate(reportRoute); + setActiveTransactionIDs(openableTransactionIDs).then(() => { + // Let the report settle before opening the expense over it, so the two open as a cascade. On + // narrow the report takes the whole screen, so without the delay it is never seen at all. + cascadeTimerRef.current = setTimeout(() => { + cascadeTimerRef.current = null; + // The user may have navigated away during the cascade delay; don't open the expense over + // whatever screen is now active. + if (!Navigation.isActiveRoute(reportRoute)) { + // Only drop the seeded sibling IDs if they are still the ones this press wrote. Another + // flow may have seeded its own carousel during the delay, and clearing is global. + if (getActiveTransactionIDs().ids === openableTransactionIDs) { + clearActiveTransactionIDs(); + } + return; + } + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); + }, PRESSED_EXPENSE_CASCADE_DELAY); + }); + return; + } + + if (isSmallScreenWidth) { + // Fallback when the report is unknown: open the pressed expense over whatever is showing. setActiveTransactionIDs(openableTransactionIDs); Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); return; diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 591d55bf35da..de25e1f1ef34 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -508,6 +508,19 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }; + // Both layouts open the report first and the pressed expense on a short timer on top of it. Let that timer + // run so assertions see the expense, not just the report underneath it. + const settleCascade = async () => { + await act(async () => { + jest.advanceTimersByTime(400); + await Promise.resolve(); + }); + await waitForBatchedUpdatesWithAct(); + }; + + // Route the narrow cascade opens beneath the pressed expense. + const narrowReportRoute = () => ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, ''); + beforeEach(() => { navigateSpy.mockImplementation(() => {}); jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(''); @@ -566,17 +579,71 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); }); - it('opens the pressed expense in the RHP over the chat on narrow layouts', async () => { + it('opens the report and then the pressed expense on top of it (after a short delay) on narrow layouts', async () => { + // The pressed expense opens on a short setTimeout so the report settles first. Use real timers so that + // delayed navigation actually fires. + jest.useRealTimers(); + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + // Back returns to the report and back again to the chat, matching the wide layout's order. + const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, ''); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy).toHaveBeenNthCalledWith(1, reportRoute); + expect(navigateSpy).toHaveBeenNthCalledWith(2, ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); + }); + + it('keeps the pressed expense out of the split stack on narrow layouts', async () => { + // The expense must open in the RHP, never as a split-navigator screen: the flows that clean up after a + // thread (split-expense save, delete) assume it is not there. removeScreenByKey only filters the root + // navigator's routes, so a nested split screen can never be removed by it. + jest.useRealTimers(); mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); await renderAndPopulateCarousel(); await pressSecondTransaction(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); - // The expense opens in the RHP, leaving the split stack untouched — the flows that clean up after a - // thread (split-expense save, delete) assume the thread is not a split-navigator screen. - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); - expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(`thread_${mockSecondTransactionID}`, undefined, undefined, '')); + // The thread must never be navigated to as a report screen, whatever backTo it would carry. + const threadID = `thread_${mockSecondTransactionID}`; + const threadAsReportScreen = navigateSpy.mock.calls.map(([route]) => String(route)).filter((route) => route.startsWith(`r/${threadID}`)); + expect(threadAsReportScreen).toEqual([]); + // ...and it did open, as the RHP route, so the assertion above is not passing merely because nothing opened. + expect(navigateSpy).toHaveBeenLastCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: threadID, backTo: narrowReportRoute()})); + }); + + it('does not open the pressed expense if the user leaves the report during the narrow cascade delay', async () => { + // Same guard the wide cascade has: the delayed navigation must not land on top of whatever screen the + // user moved to while the timer was pending. + jest.useRealTimers(); + mockResponsiveLayoutOverride = narrowResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + jest.spyOn(Navigation, 'isActiveRoute').mockReturnValue(false); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, ''); + expect(navigateSpy).toHaveBeenCalledWith(reportRoute); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); }); it('fetches the report actions when the thread resolved only from the transaction, so the carousel can resolve siblings', async () => { @@ -598,9 +665,10 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); await pressSecondTransaction(); + await settleCascade(); // The expense opens straight away from the transaction's own thread id... - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); // ...but the report's actions must still be fetched. The prev/next carousel resolves each sibling through // those actions; without them an arrow press cannot find the sibling's existing thread and mints a // duplicate thread with no parent instead. @@ -662,7 +730,8 @@ describe('MoneyRequestReportPreview', () => { // Now press the second card, which opens straight away. That is the expense the user is waiting on. navigateSpy.mockClear(); await pressSecondTransaction(); - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + await settleCascade(); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); // The first press's fetch finally lands. It must NOT navigate — the user has moved on. navigateSpy.mockClear(); @@ -807,8 +876,9 @@ describe('MoneyRequestReportPreview', () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_loaded`]: mockAction}); await waitForBatchedUpdatesWithAct(); }); + await settleCascade(); - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); }); it('falls back to the parent report once the re-fetch settles when the expense has no IOU action at all', async () => { @@ -871,8 +941,9 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }); - expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); - expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); + // The expense must end up on top. The report opening underneath it is the cascade's base, but stopping + // there would mean the press fell back to the parent report instead of reaching the pressed expense. + expect(navigateSpy).toHaveBeenLastCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); }); it('falls back to opening the parent report when the pressed expense has no thread', async () => { From c28fc9c7abf028da7d0978455143926d16478a00 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Sat, 8 Aug 2026 23:34:05 +0700 Subject: [PATCH 12/31] Seed the expense view arrows in the order the cards are shown The carousel sorts before rendering, putting transactions with a red brick road first and tiebreaking by date, but the prev/next arrows were seeded from the raw transaction collection. On a report where those two orders differ, the arrows walked a sequence the user could not see: pressing next on the visually last card jumped to the visually first one, and next on the first card was disabled. The sorted list is owned by useReportPreviewCarousel, while the press handler that seeds lives above it, so the carousel now reports its rendered order upward and the seeding uses that. Sorting a second time next to the press handler was the alternative, but it would have meant duplicating the comparator and re-reading violations and the report owner's login, leaving two orderings to keep in step. The seeded list is read once per press, since the cascade's abort compares it by identity against what is currently seeded. --- .../MoneyRequestReportPreviewContent.tsx | 2 ++ .../MoneyRequestReportPreviewProvider.tsx | 3 ++ .../MoneyRequestReportPreview/index.tsx | 28 +++++++++++++--- .../MoneyRequestReportPreview/types.ts | 3 ++ .../useReportPreviewCarousel.tsx | 11 ++++++- tests/ui/MoneyRequestReportPreview.test.tsx | 33 +++++++++++++++++++ 6 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx index fba47afcd12d..040654d7d537 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx @@ -29,6 +29,7 @@ function MoneyRequestReportPreviewContent({ invoiceReceiverPersonalDetail, lastTransactionViolations, renderTransactionItem, + onOrderedTransactionsChange, onCarouselLayout, onWrapperLayout, currentWidth, @@ -53,6 +54,7 @@ function MoneyRequestReportPreviewContent({ onPaymentOptionsShow={onPaymentOptionsShow} onPaymentOptionsHide={onPaymentOptionsHide} renderTransactionItem={renderTransactionItem} + onOrderedTransactionsChange={onOrderedTransactionsChange} currentWidth={currentWidth} reportPreviewStyles={reportPreviewStyles} newTransactionIDs={newTransactionIDs} diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx index 5df2d070e252..cfea0c6a640b 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx @@ -70,6 +70,7 @@ type MoneyRequestReportPreviewProviderProps = ChildrenProps & { onPaymentOptionsShow?: () => void; onPaymentOptionsHide?: () => void; renderTransactionItem: ListRenderItem; + onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; currentWidth: number; reportPreviewStyles: MoneyRequestReportPreviewStyleType; newTransactionIDs?: Set; @@ -96,6 +97,7 @@ function MoneyRequestReportPreviewProvider({ onPaymentOptionsShow, onPaymentOptionsHide, renderTransactionItem, + onOrderedTransactionsChange, currentWidth, reportPreviewStyles, newTransactionIDs, @@ -221,6 +223,7 @@ function MoneyRequestReportPreviewProvider({ currentWidth, newTransactionIDs, renderTransactionItem, + onOrderedTransactionsChange, }); const openReportFromPreview = useCallback(() => { diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 2cc62e2c44f8..5e9ff0898aff 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -82,10 +82,23 @@ function MoneyRequestReportPreview({ // reimbursable derivations so they include optimistically-deleted rows, exactly as before the decomposition. const allReportTransactions = Object.values(reportTransactionsCollection ?? {}).filter((transaction): transaction is Transaction => !!transaction); const transactions = allReportTransactions.filter((transaction) => isOffline || transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE); - // IDs seeded into the expense view's prev/next carousel. Offline-deleted rows stay visible in the preview (above) - // but their threads are gone, so they must not be reachable through the arrows either — same filter every other - // seeder in the tree applies. - const openableTransactionIDs = transactions.filter((transaction) => !isTransactionPendingDelete(transaction)).map((transaction) => transaction.transactionID); + // The transactions in the order the carousel renders them, reported back by it as that order changes. + const orderedTransactionsRef = useRef([]); + const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { + orderedTransactionsRef.current = orderedTransactions; + }, []); + // IDs seeded into the expense view's prev/next carousel, in the order the cards are shown. The carousel sorts + // RBR transactions first and then by date, so seeding the raw collection order would leave the arrows walking a + // different sequence from the one on screen — pressing next on the last card could jump to the first. + // Offline-deleted rows stay visible in the preview (above) but their threads are gone, so they must not be + // reachable through the arrows either — same filter every other seeder in the tree applies. + const getOpenableTransactionIDs = useCallback( + () => + (orderedTransactionsRef.current.length > 0 ? orderedTransactionsRef.current : transactions) + .filter((transaction) => !isTransactionPendingDelete(transaction)) + .map((transaction) => transaction.transactionID), + [transactions], + ); // Tracks how many actions the IOU report has loaded so a deferred expense press can be retried once // the actions arrive (they may be missing right after a cache clear). const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { @@ -233,6 +246,10 @@ function MoneyRequestReportPreview({ op: CONST.TELEMETRY.SPAN_OPEN_REPORT, }); + // Read once per press. The cascade's abort compares this exact array against what is seeded, so it has to + // be the same reference throughout this call. + const openableTransactionIDs = getOpenableTransactionIDs(); + if (isSmallScreenWidth && iouReportID) { // Narrow layouts open the report first and then the pressed expense on top of it, so back returns to // the report and back again to the chat — the same order the wide layout cascades in. @@ -313,7 +330,7 @@ function MoneyRequestReportPreview({ Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); }); }, - [isSmallScreenWidth, iouReportID, markReportRHPWidth, unmarkReportRHPWidth, openableTransactionIDs], + [isSmallScreenWidth, iouReportID, markReportRHPWidth, unmarkReportRHPWidth, getOpenableTransactionIDs], ); const openTransactionFromPreview = useCallback( @@ -465,6 +482,7 @@ function MoneyRequestReportPreview({ invoiceReceiverPolicy={invoiceReceiverPolicy} lastTransactionViolations={lastTransactionViolations} renderTransactionItem={renderItem} + onOrderedTransactionsChange={handleOrderedTransactionsChange} onCarouselLayout={onCarouselLayout} onWrapperLayout={onWrapperLayout} currentWidth={widths.currentWidth} diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts b/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts index c769b651f699..5ffd34f81e9d 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts @@ -93,6 +93,9 @@ type MoneyRequestReportPreviewContentProps = MoneyRequestReportPreviewContentOny /** Callback to render a transaction preview item */ renderTransactionItem: ListRenderItem; + /** Called with the transactions in the order the carousel renders them */ + onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; + /** Callback called when the whole preview is pressed */ onPress: () => void; diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx index de5ca7cf685d..4ab84ae97410 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx @@ -35,6 +35,13 @@ type UseReportPreviewCarouselParams = { /** Transactions that belong to the previewed report */ transactions: Transaction[]; + /** + * Called with the transactions in the order the carousel renders them. The expense view's prev/next arrows are + * seeded from this, and they have to walk the cards in the order the user sees, not the order the collection + * happens to be in. + */ + onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; + /** Violations for the previewed transactions, used to sort RBR transactions first */ transactionViolations: Parameters[2]; @@ -66,6 +73,7 @@ type UseReportPreviewCarouselParams = { */ function useReportPreviewCarousel({ transactions, + onOrderedTransactionsChange, transactionViolations, iouReport, policy, @@ -156,7 +164,8 @@ function useReportPreviewCarousel({ useEffect(() => { carouselTransactionsRef.current = carouselTransactions; - }, [carouselTransactions]); + onOrderedTransactionsChange?.(carouselTransactions); + }, [carouselTransactions, onOrderedTransactionsChange]); useEffect(() => { const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.has(transaction.transactionID)); diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index de25e1f1ef34..78266f48f4ad 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -1006,6 +1006,39 @@ describe('MoneyRequestReportPreview', () => { expect(mockUnmarkReportRHPWidth).toHaveBeenCalledWith(`thread_${mockSecondTransactionID}`); }); + it('seeds the expense view carousel in the order the cards are rendered, not collection order', async () => { + // The carousel sorts before rendering, so the collection order and the on-screen order can differ. The + // arrows walk the seeded list, so seeding collection order makes "next" on the last card jump to the + // first one. These two are supplied newest-first and render oldest-first. + mockResponsiveLayoutOverride = wideResponsiveLayout; + const olderTransaction = {...mockTransaction, transactionID: 'ordering_older', created: '2026-08-01 00:00:00', amount: mockTransaction.amount * 3}; + const newerTransaction = {...mockTransaction, transactionID: 'ordering_newer', created: '2026-08-20 00:00:00', amount: mockTransaction.amount * 5}; + mockUseReportWithTransactionsAndViolations.mockImplementation(() => [mockIOUReport, [newerTransaction, olderTransaction], {}]); + mockUseReportTransactionsCollection.mockImplementation(() => + toCollectionDataSet(ONYXKEYS.COLLECTION.TRANSACTION, [newerTransaction, olderTransaction], (transaction) => transaction.transactionID), + ); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + const setActiveTransactionIDsSpy = jest.spyOn(TransactionThreadNavigation, 'setActiveTransactionIDs'); + + renderPage({}); + await waitForBatchedUpdatesWithAct(); + setCurrentWidth(); + await act(async () => { + await Onyx.mergeCollection(ONYXKEYS.COLLECTION.TRANSACTION, { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${olderTransaction.transactionID}`]: olderTransaction, + [`${ONYXKEYS.COLLECTION.TRANSACTION}${newerTransaction.transactionID}`]: newerTransaction, + }); + await waitForBatchedUpdatesWithAct(); + }); + await waitForBatchedUpdatesWithAct(); + + fireEvent.press(screen.getByText(getTransactionDisplayAmountAndHeaderText(olderTransaction).transactionDisplayAmount)); + await waitForBatchedUpdatesWithAct(); + + expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith([olderTransaction.transactionID, newerTransaction.transactionID]); + expect(setActiveTransactionIDsSpy).not.toHaveBeenCalledWith([newerTransaction.transactionID, olderTransaction.transactionID]); + }); + it('opens the report instead of the lone expense for a single-expense report', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; setReportPreviewData({transactions: [mockTransaction]}); From 5942e243db215b07fa041652da4663e1021ceaf7 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 11 Aug 2026 23:22:54 +0700 Subject: [PATCH 13/31] Trim the report preview's comments to the non-obvious parts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several comments had grown into paragraphs that restated the code around them. Kept the reasons a reader could not infer — why the expense must stay out of the split navigator, why the seeded order is the rendered one, why clearing the sibling IDs is conditional — and dropped the rest. --- .../MoneyRequestReportPreview/index.tsx | 103 ++++++------------ .../useReportPreviewCarousel.tsx | 6 +- 2 files changed, 33 insertions(+), 76 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 228e34187995..19a029b84413 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -51,8 +51,7 @@ import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent // How many actions the IOU report has loaded. Only the count matters — a deferred press retries when it changes. const reportActionCountSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length; -// Delay (ms) before the pressed expense opens on top of the report's wide RHP. Letting the report settle -// into the wide RHP first makes the two panels open as a cascade rather than appearing at once. +// Lets the report settle first so the two open as a cascade rather than at once. const PRESSED_EXPENSE_CASCADE_DELAY = 180; function MoneyRequestReportPreview({ @@ -92,31 +91,21 @@ function MoneyRequestReportPreview({ const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { orderedTransactionsRef.current = orderedTransactions; }, []); - // IDs seeded into the expense view's prev/next carousel, in the order the cards are shown. The carousel sorts - // RBR transactions first and then by date, so seeding the raw collection order would leave the arrows walking a - // different sequence from the one on screen — pressing next on the last card could jump to the first. - // Offline-deleted rows stay visible in the preview (above) but their threads are gone, so they must not be - // reachable through the arrows either — same filter every other seeder in the tree applies. - // Tracks how many actions the IOU report has loaded so a deferred expense press can be retried once - // the actions arrive (they may be missing right after a cache clear). + // Seeds the expense view's prev/next arrows. Uses the rendered order, not collection order, or the arrows walk a + // sequence that isn't on screen. Offline-deleted rows stay visible here but their threads are gone, so they are excluded. + // A deferred press retries when this changes; the actions can be missing right after a cache clear. const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: reportActionCountSelector, }); - // Whether the deferred press's openReport fetch is still in flight. The true -> false flip re-runs the drain - // effect below, so a deferred press settles even when the fetch returns the actions we already had. + // The true -> false flip re-runs the drain effect, so a press settles even when the fetch returns nothing new. const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: isLoadingInitialReportActionsSelector, }); - // Holds a pressed transaction whose thread report could not be resolved yet, so the expense can be - // opened once the IOU report's actions have loaded instead of falling back to the parent report. - // The deferred press, plus the route the user was on when they made it. The route is what tells us the press is - // still wanted: the preview stays mounted (and focused) behind an RHP, and the "View" button is wired to a - // different openReportFromPreview in the provider, so neither unmounting nor our own handler can be relied on to - // cancel it. If the active route has moved on by the time the fetch lands, the user has chosen something else. + // A press whose thread could not be resolved yet, replayed once the report's actions load. The route it was made + // on is how we know it is still wanted: the preview stays mounted behind an RHP, and "View" goes through a + // different handler, so neither unmount nor this handler can be relied on to cancel it. const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); - // Handle for the wide-layout cascade's pending expense navigation, so a later press (or unmount) can cancel it. - // Without this a second press within the delay lets the first press's timer open the wrong expense and then run - // its cleanup over the expense that is actually on screen. + // Lets a later press or unmount cancel the cascade, so a stale timer cannot open the wrong expense. const cascadeTimerRef = useRef | null>(null); const transactionsWithReceipts = getTransactionsWithReceipts(iouReportID, allReportTransactions); const hasNonReimbursableTransactions = hasNonReimbursableTransactionsTransactionUtils(allReportTransactions); @@ -176,9 +165,7 @@ function MoneyRequestReportPreview({ return; } - // Opening the report is an explicit choice, so it supersedes anything an earlier press staged: a deferred - // press would otherwise navigate away when its fetch lands, and a pending cascade would cover this report - // with an expense the user did not just ask for. + // An explicit choice supersedes anything an earlier press staged. pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { clearTimeout(cascadeTimerRef.current); @@ -254,28 +241,20 @@ function MoneyRequestReportPreview({ .map((pressedTransaction) => pressedTransaction.transactionID); if (isSmallScreenWidth && iouReportID) { - // Narrow layouts open the report first and then the pressed expense on top of it, so back returns to - // the report and back again to the chat — the same order the wide layout cascades in. + // Report first, expense on top, so back returns to the report and then the chat. // - // The expense opens in the RHP, deliberately NOT as a split-navigator screen with the report placed - // beneath it. A thread pushed into the split stack is a full-screen SCREENS.REPORT route, and the - // flows that clean up after a thread assume it is not there: the split-expense save path relies on - // removeScreenByKey, which only filters the ROOT navigator's routes and so can never remove a nested - // split screen, and the delete path's goBack can land on a second copy of the report. Opening the - // report as an ordinary split screen and keeping only the expense in the RHP gives the same back - // order without putting the thread anywhere those flows cannot reach it. + // The expense must stay in the RHP and never become a split-navigator screen: removeScreenByKey only + // filters the root navigator, so the split-save flow could not remove a nested thread, and the delete + // flow's goBack could land on a second copy of the report. const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { - // Let the report settle before opening the expense over it, so the two open as a cascade. On - // narrow the report takes the whole screen, so without the delay it is never seen at all. + // Without the delay the report is never seen — on narrow it takes the whole screen. cascadeTimerRef.current = setTimeout(() => { cascadeTimerRef.current = null; - // The user may have navigated away during the cascade delay; don't open the expense over - // whatever screen is now active. + // The user may have navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { - // Only drop the seeded sibling IDs if they are still the ones this press wrote. Another - // flow may have seeded its own carousel during the delay, and clearing is global. + // Clearing is global, so only drop the IDs if they are still the ones this press wrote. if (getActiveTransactionIDs().ids === openableTransactionIDs) { clearActiveTransactionIDs(); } @@ -294,25 +273,19 @@ function MoneyRequestReportPreview({ return; } - // On wide layouts open the expense report itself in the wide RHP (super wide for multi-expense - // reports) and show the pressed expense on top of it — mirroring how an expense opens from the - // report view — rather than navigating to the report in the Inbox. Back returns to the report, - // and back again to the chat. + // Same cascade as narrow, but the report opens in the wide RHP rather than as a full screen. if (iouReportID) { const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); markReportRHPWidth(iouReportID, 'super-wide'); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); - // Let the report's wide RHP settle before opening the pressed expense on top, so the two - // panels open as a cascade rather than at once. + // Let the report's RHP settle so the two panels open as a cascade. cascadeTimerRef.current = setTimeout(() => { cascadeTimerRef.current = null; - // The user may have closed the report's wide RHP or navigated away during the cascade delay; - // don't reopen the expense over whatever screen is now active. + // The user may have dismissed the report or navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { - // The expense never opened, so drop the width hint we staged for it — otherwise it would - // force that thread to open wide later from an unrelated entry point. + // Drop the staged width hint, or the thread would open wide from an unrelated entry point. unmarkReportRHPWidth(childReportID); // Only drop the seeded sibling IDs if they are still the ones this press wrote. Another // flow may have seeded its own carousel during the delay, and clearing is global. @@ -342,24 +315,20 @@ function MoneyRequestReportPreview({ return; } - // Any new press supersedes what an earlier one staged. Without this an earlier deferred press stays - // armed and hijacks the navigation the moment its fetch lands, and an in-flight cascade timer opens the - // previously pressed expense over the one the user just chose. + // A new press supersedes what an earlier one staged, or the older press hijacks this navigation. pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { clearTimeout(cascadeTimerRef.current); cascadeTimerRef.current = null; } - // A report with a single expense opens the report itself, not the lone expense — opening the - // expense directly would skip the report the user expects to land on. + // A single-expense report opens the report itself, not the lone expense. if (transactions.length <= 1) { openReportFromPreview(); return; } - // An expense deleted while offline stays in the carousel (see the `transactions` filter above) but its - // thread is already gone, so opening it lands on "It's not here". Open the parent report instead. + // An offline-deleted expense stays in the carousel but its thread is gone, so it would land on "It's not here". if (transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { openReportFromPreview(); return; @@ -369,12 +338,9 @@ function MoneyRequestReportPreview({ const childReportID = resolveChildReportID(transaction); if (childReportID) { if (!isIOUActionLoaded && iouReportID) { - // The thread resolved from the transaction's own transactionThreadReportID rather than from a - // loaded IOU action, so the report's actions aren't in OnyxDB. Offline we cannot fetch them, and - // the thread shell we would open has no parent action to render from — open the parent report - // instead, the same fallback the deferred path uses. Online, fetch them in the background: the - // expense view's prev/next carousel resolves each sibling through those actions, and without - // them an arrow press mints a duplicate thread with no parent. + // The thread came from the transaction rather than a loaded IOU action, so the report's actions are + // absent. Offline the shell would have no parent action to render, so open the report instead. Online, + // fetch them: the arrows resolve siblings through those actions and would otherwise mint dead threads. if (isOffline) { openReportFromPreview(); return; @@ -385,11 +351,8 @@ function MoneyRequestReportPreview({ return; } - // The thread could not be resolved because this expense's IOU action isn't present in OnyxDB. Fetch the report's actions and - // open the expense once the fetch settles, instead of falling back to the parent - // report and losing the pressed expense. Skip this while offline: openReport can't fetch, so the - // deferred press would never fire (dead tap) — fall through to opening the cached parent report - // instead, matching the "View" button. + // The IOU action isn't loaded, so fetch and open the expense once it settles rather than losing the press. + // Offline the fetch can never land, so fall through to the cached parent report instead of a dead tap. if (!isIOUActionLoaded && iouReportID && !isOffline) { pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; openReport({reportID: iouReportID, introSelected, betas}); @@ -404,13 +367,11 @@ function MoneyRequestReportPreview({ // Completes a deferred expense press once the IOU report's actions have loaded. useEffect(() => { const pendingPress = pendingExpenseTransactionRef.current; - // Hold the press while the fetch is in flight — the loading flip back to false re-runs this effect, so the - // press settles even when the fetched actions match the cache and the action count never changes. + // Hold while the fetch is in flight; the loading flip re-runs this effect even if the action count never changes. if (!pendingPress || isLoadingInitialIOUReportActions) { return; } - // The user went somewhere else while the fetch was in flight (opened the report with "View", followed another - // link, changed tab). Replaying now would yank them out of the screen they chose, so drop the press. + // The user chose something else while the fetch was in flight, so replaying would yank them out of it. if (!isFocused || Navigation.getActiveRoute() !== pendingPress.originRoute) { pendingExpenseTransactionRef.current = null; return; @@ -429,7 +390,7 @@ function MoneyRequestReportPreview({ } }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); - // Drop a pending cascade if this preview goes away, so its timer cannot navigate or clean up after unmount. + // A pending cascade must not navigate after unmount. useEffect( () => () => { if (!cascadeTimerRef.current) { diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx index 4ab84ae97410..516e655cf40d 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx @@ -35,11 +35,7 @@ type UseReportPreviewCarouselParams = { /** Transactions that belong to the previewed report */ transactions: Transaction[]; - /** - * Called with the transactions in the order the carousel renders them. The expense view's prev/next arrows are - * seeded from this, and they have to walk the cards in the order the user sees, not the order the collection - * happens to be in. - */ + /** Called with the transactions in the order the carousel renders them, used to seed the expense view's arrows */ onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; /** Violations for the previewed transactions, used to sort RBR transactions first */ From df54f878316820c57d8b4942b3e4ea32ec5c4b59 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 11 Aug 2026 23:42:42 +0700 Subject: [PATCH 14/31] Address review: release staged cascade state and fix the empty-refetch dead tap Two issues raised on review, both real: - Unmounting during the cascade cleared the timer but not what the press had staged. The timer's callback was the only thing that would hand back the reserved RHP width and the sibling IDs, so a preview that was virtualized away mid-cascade left both dangling and affected the next expense opened from elsewhere. The pending cascade now carries its own release, and every cancel path runs it. - A deferred press only fell back to the parent report when the report had at least one action. A refetch that legitimately returns none left the press armed forever and the tap dead. The effect already waits for the fetch to settle, so the count check was both redundant and harmful. Also moves the report preview action row's height into the shared styles instead of an inline object, and drops an em dash and a banned word from comments. --- .../ReportPreviewActionButton.tsx | 6 +- .../MoneyRequestReportPreview/index.tsx | 74 +++++++++++-------- src/styles/index.ts | 4 + tests/ui/MoneyRequestReportPreview.test.tsx | 23 ++++++ .../ReportPreviewActionButtonTest.tsx | 2 +- 5 files changed, 74 insertions(+), 35 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx index 2589406ec148..e4275d45e25b 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton.tsx @@ -3,8 +3,6 @@ import Button from '@components/ButtonComposed'; import useLocalize from '@hooks/useLocalize'; import useThemeStyles from '@hooks/useThemeStyles'; -import variables from '@styles/variables'; - import CONST from '@src/CONST'; import React from 'react'; @@ -60,11 +58,11 @@ function ReportPreviewActionButton() { const primaryButton = renderPrimaryButton(); if (!primaryButton) { - return {viewButton}; + return {viewButton}; } return ( - + {primaryButton} {viewButton} diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 19a029b84413..966cc21c7b54 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -48,7 +48,7 @@ import type {MoneyRequestReportPreviewProps} from './types'; import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent'; -// How many actions the IOU report has loaded. Only the count matters — a deferred press retries when it changes. +// How many actions the IOU report has loaded. Only the count matters. A deferred press retries when it changes. const reportActionCountSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length; // Lets the report settle first so the two open as a cascade rather than at once. @@ -92,12 +92,12 @@ function MoneyRequestReportPreview({ orderedTransactionsRef.current = orderedTransactions; }, []); // Seeds the expense view's prev/next arrows. Uses the rendered order, not collection order, or the arrows walk a - // sequence that isn't on screen. Offline-deleted rows stay visible here but their threads are gone, so they are excluded. + // sequence that is not on screen. Offline-deleted rows stay visible here but their threads are gone, so they are excluded. // A deferred press retries when this changes; the actions can be missing right after a cache clear. const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: reportActionCountSelector, }); - // The true -> false flip re-runs the drain effect, so a press settles even when the fetch returns nothing new. + // When this flips back to false it re-runs the drain effect, so a press settles even when the fetch returns nothing new. const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: isLoadingInitialReportActionsSelector, }); @@ -105,8 +105,9 @@ function MoneyRequestReportPreview({ // on is how we know it is still wanted: the preview stays mounted behind an RHP, and "View" goes through a // different handler, so neither unmount nor this handler can be relied on to cancel it. const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); - // Lets a later press or unmount cancel the cascade, so a stale timer cannot open the wrong expense. - const cascadeTimerRef = useRef | null>(null); + // Lets a later press or unmount cancel the cascade. Carries the release for what the press staged, because the + // timer is otherwise the only thing that would hand those globals back. + const cascadeTimerRef = useRef<{timer: ReturnType; release: () => void} | null>(null); const transactionsWithReceipts = getTransactionsWithReceipts(iouReportID, allReportTransactions); const hasNonReimbursableTransactions = hasNonReimbursableTransactionsTransactionUtils(allReportTransactions); const areAllRequestsBeingSmartScanned = areAllRequestsBeingSmartScannedReportUtils(iouReportID, action, allReportTransactions); @@ -168,7 +169,8 @@ function MoneyRequestReportPreview({ // An explicit choice supersedes anything an earlier press staged. pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { - clearTimeout(cascadeTimerRef.current); + clearTimeout(cascadeTimerRef.current.timer); + cascadeTimerRef.current.release(); cascadeTimerRef.current = null; } @@ -192,7 +194,7 @@ function MoneyRequestReportPreview({ }); const isFocused = useIsFocused(); const newTransactions = useNewTransactions(hasOnceLoadedReportActions, transactions, pendingNewTransactionIDs, chatReportID, isFocused); - // Don't surface the highlight while the preview is covered — it'd animate the one-shot off-screen and be missed. + // Don't surface the highlight while the preview is covered. it'd animate the one-shot off-screen and be missed. const isReportVisible = shouldUseNarrowLayout ? isFocused : true; const newTransactionIDs = new Set(isReportVisible ? newTransactions.map((transaction) => transaction.transactionID) : []); @@ -243,25 +245,30 @@ function MoneyRequestReportPreview({ if (isSmallScreenWidth && iouReportID) { // Report first, expense on top, so back returns to the report and then the chat. // - // The expense must stay in the RHP and never become a split-navigator screen: removeScreenByKey only + // The expense must stay in the RHP and never become a split-navigator screen. removeScreenByKey only // filters the root navigator, so the split-save flow could not remove a nested thread, and the delete // flow's goBack could land on a second copy of the report. const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { - // Without the delay the report is never seen — on narrow it takes the whole screen. - cascadeTimerRef.current = setTimeout(() => { + // Without the delay the report is never seen, because on narrow it takes the whole screen. + // Clearing is global, so only drop the IDs if they are still the ones this press wrote. + const release = () => { + if (getActiveTransactionIDs().ids !== openableTransactionIDs) { + return; + } + clearActiveTransactionIDs(); + }; + const timer = setTimeout(() => { cascadeTimerRef.current = null; // The user may have navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { - // Clearing is global, so only drop the IDs if they are still the ones this press wrote. - if (getActiveTransactionIDs().ids === openableTransactionIDs) { - clearActiveTransactionIDs(); - } + release(); return; } Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); }, PRESSED_EXPENSE_CASCADE_DELAY); + cascadeTimerRef.current = {timer, release}; }); return; } @@ -281,21 +288,25 @@ function MoneyRequestReportPreview({ setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); // Let the report's RHP settle so the two panels open as a cascade. - cascadeTimerRef.current = setTimeout(() => { + // Drop the staged width hint, or the thread would open wide from an unrelated entry point. + // Clearing the IDs is global, so only drop them if they are still the ones this press wrote. + const release = () => { + unmarkReportRHPWidth(childReportID); + if (getActiveTransactionIDs().ids !== openableTransactionIDs) { + return; + } + clearActiveTransactionIDs(); + }; + const timer = setTimeout(() => { cascadeTimerRef.current = null; // The user may have dismissed the report or navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { - // Drop the staged width hint, or the thread would open wide from an unrelated entry point. - unmarkReportRHPWidth(childReportID); - // Only drop the seeded sibling IDs if they are still the ones this press wrote. Another - // flow may have seeded its own carousel during the delay, and clearing is global. - if (getActiveTransactionIDs().ids === openableTransactionIDs) { - clearActiveTransactionIDs(); - } + release(); return; } Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); }, PRESSED_EXPENSE_CASCADE_DELAY); + cascadeTimerRef.current = {timer, release}; }); return; } @@ -318,7 +329,8 @@ function MoneyRequestReportPreview({ // A new press supersedes what an earlier one staged, or the older press hijacks this navigation. pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { - clearTimeout(cascadeTimerRef.current); + clearTimeout(cascadeTimerRef.current.timer); + cascadeTimerRef.current.release(); cascadeTimerRef.current = null; } @@ -383,20 +395,22 @@ function MoneyRequestReportPreview({ navigateToExpense(childReportID); return; } - // The actions finished loading but the expense still has no resolvable thread — open the parent report. - if (iouReportActionCount) { - pendingExpenseTransactionRef.current = null; - openReportFromPreview(); - } + // The fetch has settled (guarded above) and the expense still has no resolvable thread, so open the parent + // report. This must not depend on the action count: a refetch that legitimately returns none would otherwise + // leave the press armed forever and the tap dead. + pendingExpenseTransactionRef.current = null; + openReportFromPreview(); }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); - // A pending cascade must not navigate after unmount. + // A pending cascade must not navigate after unmount, and its staged width hint and sibling IDs are global, so + // they have to be handed back here too. useEffect( () => () => { if (!cascadeTimerRef.current) { return; } - clearTimeout(cascadeTimerRef.current); + clearTimeout(cascadeTimerRef.current.timer); + cascadeTimerRef.current.release(); cascadeTimerRef.current = null; }, [], diff --git a/src/styles/index.ts b/src/styles/index.ts index d364a6465f57..99f484a09f45 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -4890,6 +4890,10 @@ const staticStyles = (theme: ThemeColors) => borderColor: theme.border, }, + reportPreviewActionRow: { + height: variables.h40, + }, + reportPreviewBox: { backgroundColor: theme.cardBG, borderRadius: variables.componentBorderRadiusLarge, diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 66ca7dc3c295..777ff2e63a57 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -883,6 +883,29 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); }); + it('falls back to the parent report when the re-fetch settles with no report actions at all', async () => { + // Same shape as the test below, but nothing is ever cached for the report, so the action count stays 0. + // The fallback must key off the fetch settling, not off there being actions, or the tap stays dead. + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + expect(navigateSpy).not.toHaveBeenCalled(); + + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${mockIOUReport.reportID}`, {isLoadingInitialReportActions: true}); + await waitForBatchedUpdatesWithAct(); + }); + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${mockIOUReport.reportID}`, {isLoadingInitialReportActions: false}); + await waitForBatchedUpdatesWithAct(); + }); + + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + it('falls back to the parent report once the re-fetch settles when the expense has no IOU action at all', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); diff --git a/tests/ui/components/ReportPreviewActionButtonTest.tsx b/tests/ui/components/ReportPreviewActionButtonTest.tsx index caffa0e1e432..16978159e785 100644 --- a/tests/ui/components/ReportPreviewActionButtonTest.tsx +++ b/tests/ui/components/ReportPreviewActionButtonTest.tsx @@ -80,7 +80,7 @@ jest.mock('@components/ButtonComposed', () => { }; }); -// Sentinel style objects so the row layout is actually observable. With `{flex1: {}}` alone, `styles.flexRow` and +// Marker style objects so the row layout is actually observable. With `{flex1: {}}` alone, `styles.flexRow` and // `styles.gap2` resolved to undefined and were silently dropped from the style array, leaving the two-button row // completely untested — a typo'd style key would have passed. const mockStyles = {flex1: {flex: 1}, flexRow: {flexDirection: 'row'}, gap2: {gap: 8}}; From 275de6a154dfc2ed5ed2782110a0b5e7f87cf016 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 18 Aug 2026 17:06:37 +0700 Subject: [PATCH 15/31] Shorten the report preview's comments and follow upstream API changes Cuts the remaining multi-line comments down to the single non-obvious point each, as asked on review. The merge also brought three upstream changes this file had to follow: createTransactionThreadReport now takes personalDetails, openReport now takes currentUserAccountID and hasReportActions, and a test helper was renamed back. --- .../MoneyRequestReportPreview/index.tsx | 53 +++++++------------ tests/ui/MoneyRequestReportPreview.test.tsx | 12 ++--- 2 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 966cc21c7b54..dfbee247ddde 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -91,8 +91,7 @@ function MoneyRequestReportPreview({ const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { orderedTransactionsRef.current = orderedTransactions; }, []); - // Seeds the expense view's prev/next arrows. Uses the rendered order, not collection order, or the arrows walk a - // sequence that is not on screen. Offline-deleted rows stay visible here but their threads are gone, so they are excluded. + // Rendered order, not collection order, or the arrows walk a sequence that is not on screen. Deleted rows have no thread. // A deferred press retries when this changes; the actions can be missing right after a cache clear. const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: reportActionCountSelector, @@ -101,12 +100,9 @@ function MoneyRequestReportPreview({ const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: isLoadingInitialReportActionsSelector, }); - // A press whose thread could not be resolved yet, replayed once the report's actions load. The route it was made - // on is how we know it is still wanted: the preview stays mounted behind an RHP, and "View" goes through a - // different handler, so neither unmount nor this handler can be relied on to cancel it. + // Replayed once the report's actions load. The route it was made on is how we know it is still wanted. const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); - // Lets a later press or unmount cancel the cascade. Carries the release for what the press staged, because the - // timer is otherwise the only thing that would hand those globals back. + // Carries its own release: the timer is otherwise the only thing that hands the staged globals back. const cascadeTimerRef = useRef<{timer: ReturnType; release: () => void} | null>(null); const transactionsWithReceipts = getTransactionsWithReceipts(iouReportID, allReportTransactions); const hasNonReimbursableTransactions = hasNonReimbursableTransactionsTransactionUtils(allReportTransactions); @@ -200,16 +196,13 @@ function MoneyRequestReportPreview({ const transactionPreviewContainerStyles = [styles.h100, reportPreviewStyles.transactionPreviewCarouselStyle]; - // Resolve the target transaction thread report. Prefer the IOU action's childReportID, then the - // transaction's own thread id, and finally create the thread inline so the press never lands on a dead route. + // Falls back through the IOU action, the transaction's own thread, then creating one, so a press never lands on a dead route. const resolveChildReportID = useCallback( (transaction: Transaction) => { const transactionIOUAction = getIOUActionForReportID(transaction.reportID, transaction.transactionID); let childReportID = transactionIOUAction?.childReportID ?? transaction.transactionThreadReportID; if (childReportID) { - // The thread already exists, but it may not be present in OnyxDB. Seed its - // optimistic report shell + parent linkage so navigating to it renders the expense instead of a - // blank/not-found screen. + // The thread exists but may not be cached, so seed a shell or it renders as not-found. setOptimisticTransactionThread(childReportID, iouReport?.reportID ?? transaction.reportID, transactionIOUAction?.reportActionID, iouReport?.policyID ?? policyID); } else if (transactionIOUAction?.reportActionID) { const transactionID = isMoneyRequestAction(transactionIOUAction) ? getOriginalMessage(transactionIOUAction)?.IOUTransactionID : undefined; @@ -221,12 +214,13 @@ function MoneyRequestReportPreview({ betas, iouReport, iouReportAction: transactionIOUAction, + personalDetails: personalDetailsList, })?.reportID; } } return childReportID; }, - [betas, currentUserAccountID, currentUserEmail, introSelected, iouReport, policyID], + [betas, currentUserAccountID, currentUserEmail, introSelected, iouReport, personalDetailsList, policyID], ); const navigateToExpense = useCallback( @@ -236,18 +230,14 @@ function MoneyRequestReportPreview({ op: CONST.TELEMETRY.SPAN_OPEN_REPORT, }); - // Read once per press. The cascade's abort compares this exact array against what is seeded, so it has to - // be the same reference throughout this call. + // Read once per press: the abort compares this exact array by reference. const openableTransactionIDs = (orderedTransactionsRef.current.length > 0 ? orderedTransactionsRef.current : transactions) .filter((pressedTransaction) => !isTransactionPendingDelete(pressedTransaction)) .map((pressedTransaction) => pressedTransaction.transactionID); if (isSmallScreenWidth && iouReportID) { - // Report first, expense on top, so back returns to the report and then the chat. - // - // The expense must stay in the RHP and never become a split-navigator screen. removeScreenByKey only - // filters the root navigator, so the split-save flow could not remove a nested thread, and the delete - // flow's goBack could land on a second copy of the report. + // Report first, expense on top, so back returns to the report and then the chat. The expense must stay + // in the RHP: removeScreenByKey only filters the root navigator, so a nested thread could never be removed. const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { @@ -350,36 +340,34 @@ function MoneyRequestReportPreview({ const childReportID = resolveChildReportID(transaction); if (childReportID) { if (!isIOUActionLoaded && iouReportID) { - // The thread came from the transaction rather than a loaded IOU action, so the report's actions are - // absent. Offline the shell would have no parent action to render, so open the report instead. Online, - // fetch them: the arrows resolve siblings through those actions and would otherwise mint dead threads. + // The report's actions are absent. Offline the shell has no parent action to render; online, fetch them + // or the arrows mint dead threads. if (isOffline) { openReportFromPreview(); return; } - openReport({reportID: iouReportID, introSelected, betas}); + openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!iouReportActionCount}); } navigateToExpense(childReportID); return; } - // The IOU action isn't loaded, so fetch and open the expense once it settles rather than losing the press. - // Offline the fetch can never land, so fall through to the cached parent report instead of a dead tap. + // Fetch and open once it settles. Offline the fetch never lands, so fall through to the cached parent report. if (!isIOUActionLoaded && iouReportID && !isOffline) { pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; - openReport({reportID: iouReportID, introSelected, betas}); + openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!iouReportActionCount}); return; } openReportFromPreview(); }, - [betas, introSelected, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], + [betas, currentUserAccountID, introSelected, iouReportActionCount, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], ); // Completes a deferred expense press once the IOU report's actions have loaded. useEffect(() => { const pendingPress = pendingExpenseTransactionRef.current; - // Hold while the fetch is in flight; the loading flip re-runs this effect even if the action count never changes. + // Hold while the fetch is in flight; the loading flip re-runs this effect. if (!pendingPress || isLoadingInitialIOUReportActions) { return; } @@ -395,15 +383,12 @@ function MoneyRequestReportPreview({ navigateToExpense(childReportID); return; } - // The fetch has settled (guarded above) and the expense still has no resolvable thread, so open the parent - // report. This must not depend on the action count: a refetch that legitimately returns none would otherwise - // leave the press armed forever and the tap dead. + // Settled with no thread, so open the parent. Must not depend on the action count, or an empty result leaves the tap dead. pendingExpenseTransactionRef.current = null; openReportFromPreview(); }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); - // A pending cascade must not navigate after unmount, and its staged width hint and sibling IDs are global, so - // they have to be handed back here too. + // A pending cascade must not navigate after unmount, and its staged globals must be handed back. useEffect( () => () => { if (!cascadeTimerRef.current) { diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 788080edab0e..040b26fcef4d 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -503,7 +503,7 @@ describe('MoneyRequestReportPreview', () => { }; const pressSecondTransaction = async () => { - const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockSecondTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockSecondTransaction); fireEvent.press(screen.getByText(transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); }; @@ -721,7 +721,7 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); // Press the first card — it defers, waiting on the report's actions. - const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); const [firstCard] = screen.getAllByText(transactionDisplayAmount); fireEvent.press(firstCard); await waitForBatchedUpdatesWithAct(); @@ -775,7 +775,7 @@ describe('MoneyRequestReportPreview', () => { // Issue #26939: deleting an expense offline must leave the preview VISIBLE (greyed out) rather than // collapsing it. v2 only makes that row non-navigable — it must not disappear, so both cards still render. - const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); expect(screen.getAllByText(transactionDisplayAmount).length).toBeGreaterThanOrEqual(2); }); @@ -794,7 +794,7 @@ describe('MoneyRequestReportPreview', () => { ); await renderAndPopulateCarousel(); - const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); const [liveRow] = screen.getAllByText(transactionDisplayAmount); fireEvent.press(liveRow); await waitForBatchedUpdatesWithAct(); @@ -1055,7 +1055,7 @@ describe('MoneyRequestReportPreview', () => { }); await waitForBatchedUpdatesWithAct(); - fireEvent.press(screen.getByText(getTransactionDisplayAmountAndMetadataText(olderTransaction).transactionDisplayAmount)); + fireEvent.press(screen.getByText(getTransactionDisplayAmountAndHeaderText(olderTransaction).transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith([olderTransaction.transactionID, newerTransaction.transactionID]); @@ -1076,7 +1076,7 @@ describe('MoneyRequestReportPreview', () => { }); await waitForBatchedUpdatesWithAct(); - const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); fireEvent.press(screen.getByText(transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); From bfbc7459f66c8e61d5543df8d68c6df248e5963a Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 19 Aug 2026 08:59:08 +0700 Subject: [PATCH 16/31] Fix two carousel navigation bugs found in review Opening the report from "View" while a carousel press was still staged left the expense to land on top of it, so going back showed the details twice. The press's own guard only checks whether the user navigated away, and "View" opens the same report route, so it could not catch this. The cancel is now shared and runs from that path too. The expense view's arrows stopped at the last card the carousel draws. Seeding was changed to follow the rendered order, but the carousel only renders the first few, so everything after that became unreachable. The full sorted order is now reported for seeding while the carousel keeps rendering its capped slice. Also removes the comments that only restated the code they sat above. --- .../MoneyRequestReportPreviewContent.tsx | 2 + .../MoneyRequestReportPreviewProvider.tsx | 6 +- .../MoneyRequestReportPreview/index.tsx | 35 +++++------ .../MoneyRequestReportPreview/types.ts | 3 + .../useReportPreviewCarousel.tsx | 11 ++-- tests/ui/MoneyRequestReportPreview.test.tsx | 59 +++++++++++++++++++ 6 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx index 86ff681d0a74..0365bc237ab3 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx @@ -32,6 +32,7 @@ function MoneyRequestReportPreviewContent({ lastTransactionViolations, renderTransactionItem, onOrderedTransactionsChange, + onCancelPendingPress, onCarouselLayout, onWrapperLayout, currentWidth, @@ -59,6 +60,7 @@ function MoneyRequestReportPreviewContent({ onPaymentOptionsHide={onPaymentOptionsHide} renderTransactionItem={renderTransactionItem} onOrderedTransactionsChange={onOrderedTransactionsChange} + onCancelPendingPress={onCancelPendingPress} currentWidth={currentWidth} reportPreviewStyles={reportPreviewStyles} newTransactionIDs={newTransactionIDs} diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx index 5b763dc4dd9a..325496d9e05a 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx @@ -69,6 +69,7 @@ type MoneyRequestReportPreviewProviderProps = ChildrenProps & { onPaymentOptionsHide?: () => void; renderTransactionItem: ListRenderItem; onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; + onCancelPendingPress?: () => void; currentWidth: number; reportPreviewStyles: MoneyRequestReportPreviewStyleType; newTransactionIDs?: Set; @@ -98,6 +99,7 @@ function MoneyRequestReportPreviewProvider({ onPaymentOptionsHide, renderTransactionItem, onOrderedTransactionsChange, + onCancelPendingPress, currentWidth, reportPreviewStyles, newTransactionIDs, @@ -211,6 +213,8 @@ function MoneyRequestReportPreviewProvider({ if (!iouReportID) { return; } + // A carousel press may still have a delayed expense navigation staged, and it opens over this same report. + onCancelPendingPress?.(); startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreviewContent', op: CONST.TELEMETRY.SPAN_OPEN_REPORT, @@ -227,7 +231,7 @@ function MoneyRequestReportPreviewProvider({ }), ); } - }, [iouReportID, isSmallScreenWidth]); + }, [iouReportID, isSmallScreenWidth, onCancelPendingPress]); const onHoldMenuOpen = useCallback((requestType: string, paymentType?: PaymentMethodType, canPay?: boolean, methodID?: number) => { if (requestType !== CONST.IOU.REPORT_ACTION_TYPE.PAY && requestType !== CONST.IOU.REPORT_ACTION_TYPE.APPROVE) { diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index dfbee247ddde..19d0e684f279 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -86,13 +86,11 @@ function MoneyRequestReportPreview({ // Kept local to this component rather than passed down, so children only receive the derived values they need. const allReportTransactions = Object.values(reportTransactionsCollection ?? {}).filter((transaction): transaction is Transaction => !!transaction); const transactions = allReportTransactions.filter((transaction) => isOffline || transaction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE); - // The transactions in the order the carousel renders them, reported back by it as that order changes. const orderedTransactionsRef = useRef([]); const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { orderedTransactionsRef.current = orderedTransactions; }, []); // Rendered order, not collection order, or the arrows walk a sequence that is not on screen. Deleted rows have no thread. - // A deferred press retries when this changes; the actions can be missing right after a cache clear. const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: reportActionCountSelector, }); @@ -157,18 +155,24 @@ function MoneyRequestReportPreview({ return transactions.some((transaction) => (Number(transaction?.modifiedAmount) || transaction?.amount) < 0); }, [transactions, action.childType, iouReport]); + // An explicit choice supersedes anything an earlier press staged. Also reachable from the "View" button, which + // opens the same report route, so the cascade's own "did the user navigate away" guard would not catch it. + const cancelPendingPress = useCallback(() => { + pendingExpenseTransactionRef.current = null; + if (!cascadeTimerRef.current) { + return; + } + clearTimeout(cascadeTimerRef.current.timer); + cascadeTimerRef.current.release(); + cascadeTimerRef.current = null; + }, []); + const openReportFromPreview = useCallback(() => { if (!iouReportID || contextMenuRef.current?.isContextMenuOpening) { return; } - // An explicit choice supersedes anything an earlier press staged. - pendingExpenseTransactionRef.current = null; - if (cascadeTimerRef.current) { - clearTimeout(cascadeTimerRef.current.timer); - cascadeTimerRef.current.release(); - cascadeTimerRef.current = null; - } + cancelPendingPress(); startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreview', @@ -181,7 +185,7 @@ function MoneyRequestReportPreview({ } else { Navigation.navigate(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()})); } - }, [iouReportID, isSmallScreenWidth]); + }, [cancelPendingPress, iouReportID, isSmallScreenWidth]); const [hasOnceLoadedReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${chatReportID}`, { selector: hasOnceLoadedReportActionsSelector, }); @@ -196,7 +200,6 @@ function MoneyRequestReportPreview({ const transactionPreviewContainerStyles = [styles.h100, reportPreviewStyles.transactionPreviewCarouselStyle]; - // Falls back through the IOU action, the transaction's own thread, then creating one, so a press never lands on a dead route. const resolveChildReportID = useCallback( (transaction: Transaction) => { const transactionIOUAction = getIOUActionForReportID(transaction.reportID, transaction.transactionID); @@ -241,7 +244,6 @@ function MoneyRequestReportPreview({ const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { - // Without the delay the report is never seen, because on narrow it takes the whole screen. // Clearing is global, so only drop the IDs if they are still the ones this press wrote. const release = () => { if (getActiveTransactionIDs().ids !== openableTransactionIDs) { @@ -251,7 +253,6 @@ function MoneyRequestReportPreview({ }; const timer = setTimeout(() => { cascadeTimerRef.current = null; - // The user may have navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { release(); return; @@ -264,20 +265,17 @@ function MoneyRequestReportPreview({ } if (isSmallScreenWidth) { - // Fallback when the report is unknown: open the pressed expense over whatever is showing. setActiveTransactionIDs(openableTransactionIDs); Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); return; } - // Same cascade as narrow, but the report opens in the wide RHP rather than as a full screen. if (iouReportID) { const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); markReportRHPWidth(iouReportID, 'super-wide'); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); - // Let the report's RHP settle so the two panels open as a cascade. // Drop the staged width hint, or the thread would open wide from an unrelated entry point. // Clearing the IDs is global, so only drop them if they are still the ones this press wrote. const release = () => { @@ -289,7 +287,6 @@ function MoneyRequestReportPreview({ }; const timer = setTimeout(() => { cascadeTimerRef.current = null; - // The user may have dismissed the report or navigated away during the delay. if (!Navigation.isActiveRoute(reportRoute)) { release(); return; @@ -301,7 +298,6 @@ function MoneyRequestReportPreview({ return; } - // Fallback when the parent report is unknown: open the pressed expense alone in the wide RHP. setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); @@ -364,10 +360,8 @@ function MoneyRequestReportPreview({ [betas, currentUserAccountID, introSelected, iouReportActionCount, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], ); - // Completes a deferred expense press once the IOU report's actions have loaded. useEffect(() => { const pendingPress = pendingExpenseTransactionRef.current; - // Hold while the fetch is in flight; the loading flip re-runs this effect. if (!pendingPress || isLoadingInitialIOUReportActions) { return; } @@ -448,6 +442,7 @@ function MoneyRequestReportPreview({ lastTransactionViolations={lastTransactionViolations} renderTransactionItem={renderItem} onOrderedTransactionsChange={handleOrderedTransactionsChange} + onCancelPendingPress={cancelPendingPress} onCarouselLayout={onCarouselLayout} onWrapperLayout={onWrapperLayout} currentWidth={widths.currentWidth} diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts b/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts index 684ab20bcb48..4b46b7b262d6 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/types.ts @@ -100,6 +100,9 @@ type MoneyRequestReportPreviewContentProps = MoneyRequestReportPreviewContentOny /** Called with the transactions in the order the carousel renders them */ onOrderedTransactionsChange?: (orderedTransactions: Transaction[]) => void; + /** Cancels anything a carousel press staged, so opening the report cannot be overtaken by it */ + onCancelPendingPress?: () => void; + /** Callback called when the whole preview is pressed */ onPress: () => void; diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx index 516e655cf40d..057eddb808d0 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx @@ -85,7 +85,7 @@ function useReportPreviewCarousel({ const [ownerLogin] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {selector: personalDetailsLoginSelector(iouReport?.ownerAccountID)}); const isFocusedRef = useIsFocusedRef(); - const carouselTransactions = useMemo(() => { + const sortedTransactions = useMemo(() => { if (shouldShowAccessPlaceHolder) { return []; } @@ -106,8 +106,11 @@ function useReportPreviewCarousel({ // Tiebreak by date (ascending — oldest first) so position is stable across RBR state changes return localeCompare(getCreated(a), getCreated(b)); }); - return sorted.slice(0, MAX_PREVIEWS_NUMBER + 1); + return sorted; }, [shouldShowAccessPlaceHolder, transactions, transactionViolations, currentUserDetails?.login, currentUserDetails?.accountID, iouReport, ownerLogin, policy, localeCompare]); + + // The carousel only renders the first few cards, but the expense view's arrows must still reach every expense. + const carouselTransactions = useMemo(() => sortedTransactions.slice(0, MAX_PREVIEWS_NUMBER + 1), [sortedTransactions]); const prevCarouselTransactionLength = useRef(0); useEffect(() => { @@ -160,8 +163,8 @@ function useReportPreviewCarousel({ useEffect(() => { carouselTransactionsRef.current = carouselTransactions; - onOrderedTransactionsChange?.(carouselTransactions); - }, [carouselTransactions, onOrderedTransactionsChange]); + onOrderedTransactionsChange?.(sortedTransactions); + }, [carouselTransactions, onOrderedTransactionsChange, sortedTransactions]); useEffect(() => { const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.has(transaction.transactionID)); diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 040b26fcef4d..5ef1be7e4320 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -1062,6 +1062,65 @@ describe('MoneyRequestReportPreview', () => { expect(setActiveTransactionIDsSpy).not.toHaveBeenCalledWith([newerTransaction.transactionID, olderTransaction.transactionID]); }); + it('does not open the pressed expense over the report when "View" is tapped during the cascade delay', async () => { + // Regression: "View" opens the same report route, so the cascade's own "did the user navigate away" guard + // does not catch it and the expense used to land on top, showing the details twice after going back. + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + + // Tap "View" while the cascade timer is still pending. + fireEvent.press(screen.getByText(TestHelper.translateLocal('common.view'))); + await waitForBatchedUpdatesWithAct(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''}); + expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); + }); + + it('seeds every expense into the arrows, not just the ones the carousel renders', async () => { + // Regression: the carousel caps how many cards it draws, and seeding that capped list left the next arrow + // disabled on the last drawn card even though more expenses existed after it. + mockResponsiveLayoutOverride = wideResponsiveLayout; + const many = Array.from({length: 14}, (_, index) => ({ + ...mockTransaction, + transactionID: `bulk_${index}`, + created: `2026-08-${String(index + 1).padStart(2, '0')} 00:00:00`, + amount: mockTransaction.amount - (index + 1) * 1300, + })); + mockUseReportWithTransactionsAndViolations.mockImplementation(() => [mockIOUReport, many, {}]); + mockUseReportTransactionsCollection.mockImplementation(() => toCollectionDataSet(ONYXKEYS.COLLECTION.TRANSACTION, many, (transaction) => transaction.transactionID)); + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + const setActiveTransactionIDsSpy = jest.spyOn(TransactionThreadNavigation, 'setActiveTransactionIDs'); + + renderPage({}); + await waitForBatchedUpdatesWithAct(); + setCurrentWidth(); + await act(async () => { + await Onyx.mergeCollection( + ONYXKEYS.COLLECTION.TRANSACTION, + Object.fromEntries(many.map((transaction) => [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction])) as Record< + `${typeof ONYXKEYS.COLLECTION.TRANSACTION}${string}`, + Transaction + >, + ); + await waitForBatchedUpdatesWithAct(); + }); + await waitForBatchedUpdatesWithAct(); + + fireEvent.press(screen.getByText(getTransactionDisplayAmountAndHeaderText(many.at(0) ?? mockTransaction).transactionDisplayAmount)); + await waitForBatchedUpdatesWithAct(); + + expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith(many.map((transaction) => transaction.transactionID)); + }); + it('opens the report instead of the lone expense for a single-expense report', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; setReportPreviewData({transactions: [mockTransaction]}); From f49b296482eeff04b5b3e4b6dd1d7549a921710c Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 20 Aug 2026 09:53:23 +0700 Subject: [PATCH 17/31] Remove the report preview's explanatory comments Requested on review. The reasoning is preserved in the commit history and the PR description, so the source no longer carries comments that could drift out of step with the code. Also follows an upstream rename of a test helper that came in with the last main merge. --- .../MoneyRequestReportPreviewProvider.tsx | 1 - .../MoneyRequestReportPreview/index.tsx | 25 ------------------- .../useReportPreviewCarousel.tsx | 1 - tests/ui/MoneyRequestReportPreview.test.tsx | 14 +++++------ 4 files changed, 7 insertions(+), 34 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx index 325496d9e05a..1647e99644e6 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx @@ -213,7 +213,6 @@ function MoneyRequestReportPreviewProvider({ if (!iouReportID) { return; } - // A carousel press may still have a delayed expense navigation staged, and it opens over this same report. onCancelPendingPress?.(); startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreviewContent', diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 19d0e684f279..af74d4ff3eb0 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -48,10 +48,8 @@ import type {MoneyRequestReportPreviewProps} from './types'; import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent'; -// How many actions the IOU report has loaded. Only the count matters. A deferred press retries when it changes. const reportActionCountSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length; -// Lets the report settle first so the two open as a cascade rather than at once. const PRESSED_EXPENSE_CASCADE_DELAY = 180; function MoneyRequestReportPreview({ @@ -90,17 +88,13 @@ function MoneyRequestReportPreview({ const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { orderedTransactionsRef.current = orderedTransactions; }, []); - // Rendered order, not collection order, or the arrows walk a sequence that is not on screen. Deleted rows have no thread. const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: reportActionCountSelector, }); - // When this flips back to false it re-runs the drain effect, so a press settles even when the fetch returns nothing new. const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: isLoadingInitialReportActionsSelector, }); - // Replayed once the report's actions load. The route it was made on is how we know it is still wanted. const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); - // Carries its own release: the timer is otherwise the only thing that hands the staged globals back. const cascadeTimerRef = useRef<{timer: ReturnType; release: () => void} | null>(null); const transactionsWithReceipts = getTransactionsWithReceipts(iouReportID, allReportTransactions); const hasNonReimbursableTransactions = hasNonReimbursableTransactionsTransactionUtils(allReportTransactions); @@ -155,8 +149,6 @@ function MoneyRequestReportPreview({ return transactions.some((transaction) => (Number(transaction?.modifiedAmount) || transaction?.amount) < 0); }, [transactions, action.childType, iouReport]); - // An explicit choice supersedes anything an earlier press staged. Also reachable from the "View" button, which - // opens the same report route, so the cascade's own "did the user navigate away" guard would not catch it. const cancelPendingPress = useCallback(() => { pendingExpenseTransactionRef.current = null; if (!cascadeTimerRef.current) { @@ -194,7 +186,6 @@ function MoneyRequestReportPreview({ }); const isFocused = useIsFocused(); const newTransactions = useNewTransactions(hasOnceLoadedReportActions, transactions, pendingNewTransactionIDs, chatReportID, isFocused); - // Don't surface the highlight while the preview is covered. it'd animate the one-shot off-screen and be missed. const isReportVisible = shouldUseNarrowLayout ? isFocused : true; const newTransactionIDs = new Set(isReportVisible ? newTransactions.map((transaction) => transaction.transactionID) : []); @@ -205,7 +196,6 @@ function MoneyRequestReportPreview({ const transactionIOUAction = getIOUActionForReportID(transaction.reportID, transaction.transactionID); let childReportID = transactionIOUAction?.childReportID ?? transaction.transactionThreadReportID; if (childReportID) { - // The thread exists but may not be cached, so seed a shell or it renders as not-found. setOptimisticTransactionThread(childReportID, iouReport?.reportID ?? transaction.reportID, transactionIOUAction?.reportActionID, iouReport?.policyID ?? policyID); } else if (transactionIOUAction?.reportActionID) { const transactionID = isMoneyRequestAction(transactionIOUAction) ? getOriginalMessage(transactionIOUAction)?.IOUTransactionID : undefined; @@ -233,18 +223,14 @@ function MoneyRequestReportPreview({ op: CONST.TELEMETRY.SPAN_OPEN_REPORT, }); - // Read once per press: the abort compares this exact array by reference. const openableTransactionIDs = (orderedTransactionsRef.current.length > 0 ? orderedTransactionsRef.current : transactions) .filter((pressedTransaction) => !isTransactionPendingDelete(pressedTransaction)) .map((pressedTransaction) => pressedTransaction.transactionID); if (isSmallScreenWidth && iouReportID) { - // Report first, expense on top, so back returns to the report and then the chat. The expense must stay - // in the RHP: removeScreenByKey only filters the root navigator, so a nested thread could never be removed. const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { - // Clearing is global, so only drop the IDs if they are still the ones this press wrote. const release = () => { if (getActiveTransactionIDs().ids !== openableTransactionIDs) { return; @@ -276,8 +262,6 @@ function MoneyRequestReportPreview({ Navigation.navigate(reportRoute); setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); - // Drop the staged width hint, or the thread would open wide from an unrelated entry point. - // Clearing the IDs is global, so only drop them if they are still the ones this press wrote. const release = () => { unmarkReportRHPWidth(childReportID); if (getActiveTransactionIDs().ids !== openableTransactionIDs) { @@ -312,7 +296,6 @@ function MoneyRequestReportPreview({ return; } - // A new press supersedes what an earlier one staged, or the older press hijacks this navigation. pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { clearTimeout(cascadeTimerRef.current.timer); @@ -320,13 +303,11 @@ function MoneyRequestReportPreview({ cascadeTimerRef.current = null; } - // A single-expense report opens the report itself, not the lone expense. if (transactions.length <= 1) { openReportFromPreview(); return; } - // An offline-deleted expense stays in the carousel but its thread is gone, so it would land on "It's not here". if (transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { openReportFromPreview(); return; @@ -336,8 +317,6 @@ function MoneyRequestReportPreview({ const childReportID = resolveChildReportID(transaction); if (childReportID) { if (!isIOUActionLoaded && iouReportID) { - // The report's actions are absent. Offline the shell has no parent action to render; online, fetch them - // or the arrows mint dead threads. if (isOffline) { openReportFromPreview(); return; @@ -348,7 +327,6 @@ function MoneyRequestReportPreview({ return; } - // Fetch and open once it settles. Offline the fetch never lands, so fall through to the cached parent report. if (!isIOUActionLoaded && iouReportID && !isOffline) { pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!iouReportActionCount}); @@ -365,7 +343,6 @@ function MoneyRequestReportPreview({ if (!pendingPress || isLoadingInitialIOUReportActions) { return; } - // The user chose something else while the fetch was in flight, so replaying would yank them out of it. if (!isFocused || Navigation.getActiveRoute() !== pendingPress.originRoute) { pendingExpenseTransactionRef.current = null; return; @@ -377,12 +354,10 @@ function MoneyRequestReportPreview({ navigateToExpense(childReportID); return; } - // Settled with no thread, so open the parent. Must not depend on the action count, or an empty result leaves the tap dead. pendingExpenseTransactionRef.current = null; openReportFromPreview(); }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); - // A pending cascade must not navigate after unmount, and its staged globals must be handed back. useEffect( () => () => { if (!cascadeTimerRef.current) { diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx index 057eddb808d0..ede408e2a1b1 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/useReportPreviewCarousel.tsx @@ -109,7 +109,6 @@ function useReportPreviewCarousel({ return sorted; }, [shouldShowAccessPlaceHolder, transactions, transactionViolations, currentUserDetails?.login, currentUserDetails?.accountID, iouReport, ownerLogin, policy, localeCompare]); - // The carousel only renders the first few cards, but the expense view's arrows must still reach every expense. const carouselTransactions = useMemo(() => sortedTransactions.slice(0, MAX_PREVIEWS_NUMBER + 1), [sortedTransactions]); const prevCarouselTransactionLength = useRef(0); diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index 487e84d21b0e..a42383e9fbd3 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -505,7 +505,7 @@ describe('MoneyRequestReportPreview', () => { }; const pressSecondTransaction = async () => { - const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockSecondTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockSecondTransaction); fireEvent.press(screen.getByText(transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); }; @@ -723,7 +723,7 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); // Press the first card — it defers, waiting on the report's actions. - const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); const [firstCard] = screen.getAllByText(transactionDisplayAmount); fireEvent.press(firstCard); await waitForBatchedUpdatesWithAct(); @@ -777,7 +777,7 @@ describe('MoneyRequestReportPreview', () => { // Issue #26939: deleting an expense offline must leave the preview VISIBLE (greyed out) rather than // collapsing it. v2 only makes that row non-navigable — it must not disappear, so both cards still render. - const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); expect(screen.getAllByText(transactionDisplayAmount).length).toBeGreaterThanOrEqual(2); }); @@ -796,7 +796,7 @@ describe('MoneyRequestReportPreview', () => { ); await renderAndPopulateCarousel(); - const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); const [liveRow] = screen.getAllByText(transactionDisplayAmount); fireEvent.press(liveRow); await waitForBatchedUpdatesWithAct(); @@ -1057,7 +1057,7 @@ describe('MoneyRequestReportPreview', () => { }); await waitForBatchedUpdatesWithAct(); - fireEvent.press(screen.getByText(getTransactionDisplayAmountAndHeaderText(olderTransaction).transactionDisplayAmount)); + fireEvent.press(screen.getByText(getTransactionDisplayAmountAndMetadataText(olderTransaction).transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith([olderTransaction.transactionID, newerTransaction.transactionID]); @@ -1117,7 +1117,7 @@ describe('MoneyRequestReportPreview', () => { }); await waitForBatchedUpdatesWithAct(); - fireEvent.press(screen.getByText(getTransactionDisplayAmountAndHeaderText(many.at(0) ?? mockTransaction).transactionDisplayAmount)); + fireEvent.press(screen.getByText(getTransactionDisplayAmountAndMetadataText(many.at(0) ?? mockTransaction).transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith(many.map((transaction) => transaction.transactionID)); @@ -1137,7 +1137,7 @@ describe('MoneyRequestReportPreview', () => { }); await waitForBatchedUpdatesWithAct(); - const {transactionDisplayAmount} = getTransactionDisplayAmountAndHeaderText(mockTransaction); + const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); fireEvent.press(screen.getByText(transactionDisplayAmount)); await waitForBatchedUpdatesWithAct(); From 8876f0c909fe4405bc44749d1189fb7088ad86b5 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 20 Aug 2026 13:54:02 +0700 Subject: [PATCH 18/31] Trim test comments and take the transaction seed off the navigation path The pressed-expense cascade awaited the setActiveTransactionIDs Onyx write before scheduling the expense navigation, which left the report screen interactive for the duration of that write. Fire the write and schedule the navigation immediately instead, deferring only the release check onto it. --- .../MoneyRequestReportPreview/index.tsx | 58 ++++----- tests/ui/MoneyRequestReportPreview.test.tsx | 118 +++++------------- .../ReportPreviewActionButtonTest.tsx | 13 +- 3 files changed, 61 insertions(+), 128 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index af74d4ff3eb0..8b7e240c85a5 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -230,23 +230,24 @@ function MoneyRequestReportPreview({ if (isSmallScreenWidth && iouReportID) { const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); Navigation.navigate(reportRoute); - setActiveTransactionIDs(openableTransactionIDs).then(() => { - const release = () => { + const seeded = setActiveTransactionIDs(openableTransactionIDs); + const release = () => { + seeded.then(() => { if (getActiveTransactionIDs().ids !== openableTransactionIDs) { return; } clearActiveTransactionIDs(); - }; - const timer = setTimeout(() => { - cascadeTimerRef.current = null; - if (!Navigation.isActiveRoute(reportRoute)) { - release(); - return; - } - Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); - }, PRESSED_EXPENSE_CASCADE_DELAY); - cascadeTimerRef.current = {timer, release}; - }); + }); + }; + const timer = setTimeout(() => { + cascadeTimerRef.current = null; + if (!Navigation.isActiveRoute(reportRoute)) { + release(); + return; + } + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); + }, PRESSED_EXPENSE_CASCADE_DELAY); + cascadeTimerRef.current = {timer, release}; return; } @@ -260,25 +261,26 @@ function MoneyRequestReportPreview({ const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); markReportRHPWidth(iouReportID, 'super-wide'); Navigation.navigate(reportRoute); - setActiveTransactionIDs(openableTransactionIDs).then(() => { - markReportRHPWidth(childReportID, 'wide'); - const release = () => { - unmarkReportRHPWidth(childReportID); + const seeded = setActiveTransactionIDs(openableTransactionIDs); + markReportRHPWidth(childReportID, 'wide'); + const release = () => { + unmarkReportRHPWidth(childReportID); + seeded.then(() => { if (getActiveTransactionIDs().ids !== openableTransactionIDs) { return; } clearActiveTransactionIDs(); - }; - const timer = setTimeout(() => { - cascadeTimerRef.current = null; - if (!Navigation.isActiveRoute(reportRoute)) { - release(); - return; - } - Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); - }, PRESSED_EXPENSE_CASCADE_DELAY); - cascadeTimerRef.current = {timer, release}; - }); + }); + }; + const timer = setTimeout(() => { + cascadeTimerRef.current = null; + if (!Navigation.isActiveRoute(reportRoute)) { + release(); + return; + } + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: reportRoute})); + }, PRESSED_EXPENSE_CASCADE_DELAY); + cascadeTimerRef.current = {timer, release}; return; } diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index a42383e9fbd3..6f92a967b478 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -86,8 +86,7 @@ jest.mock('@src/hooks/useReportWithTransactionsAndViolations', () => ({ default: (...args: Parameters) => mockUseReportWithTransactionsAndViolations(...args), })); -// Lets a single test force the narrow (mobile) layout. When left undefined every other test -// runs the real hook unchanged, so the existing wide-layout tests keep their behavior. +// Left undefined, every other test runs the real hook, so the wide-layout tests keep their behavior. let mockResponsiveLayoutOverride: ResponsiveLayoutResult | undefined; jest.mock('@hooks/useResponsiveLayout', () => { const actual = jest.requireActual<{default: () => ResponsiveLayoutResult}>('@hooks/useResponsiveLayout'); @@ -125,8 +124,6 @@ const wideResponsiveLayout: ResponsiveLayoutResult = { isInLandscapeMode: false, }; -// The preview reads `iouReport` from a prop (provided stable by the parent) and its transactions from the -// scoped `useReportTransactionsCollection` hook, so the test drives those two sources directly. let mockIOUReportProp: OnyxEntry = mockIOUReport; const mockUseReportTransactionsCollection = jest.fn(() => toCollectionDataSet(ONYXKEYS.COLLECTION.TRANSACTION, defaultPreviewTransactions, (transaction) => transaction.transactionID)); @@ -138,9 +135,7 @@ jest.mock('@hooks/useReportTransactionsCollection', () => ({ type OnHoldMenuOpen = (requestType: string, paymentType?: PaymentMethodType, canPay?: boolean, methodID?: number) => void; -// Capture the onHoldMenuOpen callback the preview passes to the pay button so a held-expense payment can be triggered -// directly with a selected bank account, mirroring a user picking an account in the dropdown for a held report. -// The wrapper still renders the real component so these tests keep exercising it. +// Capture onHoldMenuOpen so a held-expense payment can be triggered with a chosen bank account. const mockOnHoldMenuOpenHolder: {current?: OnHoldMenuOpen} = {current: undefined}; jest.mock('@components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewActionButton', () => { const actualReact = jest.requireActual('react'); @@ -149,7 +144,6 @@ jest.mock('@components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewA return { __esModule: true, default: function MockReportPreviewActionButton() { - // ReportPreviewActionButton now reads from context instead of props; capture onHoldMenuOpen from the context. const {onHoldMenuOpen} = useReportPreviewActions(); mockOnHoldMenuOpenHolder.current = onHoldMenuOpen; return actualReact.createElement(actualModule.default); @@ -157,8 +151,7 @@ jest.mock('@components/ReportActionItem/MoneyRequestReportPreview/ReportPreviewA }; }); -// The preview widens the RHP for the report it opens and narrows it back when a press is abandoned. Nothing in the -// rendered output reflects that, so capture the calls to assert the widths are actually requested and released. +// The RHP widths never reach the rendered output, so capture the calls to assert they are requested and released. const mockMarkReportRHPWidth = jest.fn(); const mockUnmarkReportRHPWidth = jest.fn(); jest.mock('@components/WideRHPContextProvider', () => ({ @@ -169,7 +162,6 @@ jest.mock('@components/WideRHPContextProvider', () => ({ }), })); -// Capture the props the preview forwards to the hold menu so the selected bank account that reaches it can be asserted. const mockHoldMenuPropsHolder: {current?: {isVisible?: boolean; paymentType?: PaymentMethodType; methodID?: number}} = {current: undefined}; jest.mock('@components/ProcessMoneyReportHoldMenu', () => ({ __esModule: true, @@ -243,7 +235,6 @@ const getTransactionDisplayAmountAndMetadataText = (transaction: Transaction) => const created = getFormattedCreated(transaction); const date = DateUtils.formatWithUTCTimeZone(created, DateUtils.doesDateBelongToAPastYear(created) ? CONST.DATE.MONTH_DAY_YEAR_ABBR_FORMAT : CONST.DATE.MONTH_DAY_ABBR_FORMAT, undefined); const isTransactionMadeWithCard = isManagedCardTransaction(transaction); - // The date leads the supporting line, which can also carry the category and the report status. const transactionSupportingText = new RegExp(`^${date}`); const transactionTypeText = isTransactionMadeWithCard ? TestHelper.translateLocal('iou.card') : TestHelper.translateLocal('iou.cash'); const transactionDisplayAmount = TestHelper.convertToDisplayString(-transaction.amount, transaction.currency); @@ -484,8 +475,7 @@ describe('MoneyRequestReportPreview', () => { describe('pressing a transaction in the carousel', () => { const navigateSpy = jest.spyOn(Navigation, 'navigate'); - // Give every transaction its own thread report so the assertion proves the *pressed* card - // drives navigation, instead of every card sharing one parent-report handler. + // A distinct thread per transaction, so the assertions prove the *pressed* card drives navigation. const buildActionWithThread = (reportID: string | undefined, transactionID: string | undefined) => { if (!reportID || !transactionID) { return undefined; @@ -510,8 +500,7 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }; - // Both layouts open the report first and the pressed expense on a short timer on top of it. Let that timer - // run so assertions see the expense, not just the report underneath it. + // Both layouts open the report first and the pressed expense on a short timer; let that timer run. const settleCascade = async () => { await act(async () => { jest.advanceTimersByTime(400); @@ -520,26 +509,21 @@ describe('MoneyRequestReportPreview', () => { await waitForBatchedUpdatesWithAct(); }; - // Route the narrow cascade opens beneath the pressed expense. const narrowReportRoute = () => ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, ''); beforeEach(() => { navigateSpy.mockImplementation(() => {}); jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(''); - // The wide-layout cascade guards its delayed expense navigation on isActiveRoute(reportRoute); default to - // "still on the report" so the happy-path cascade fires. + // The cascade guards its delayed navigation on isActiveRoute; default to "still on the report". jest.spyOn(Navigation, 'isActiveRoute').mockReturnValue(true); }); afterEach(() => { mockResponsiveLayoutOverride = undefined; - // Restore the globally-enabled fake timers in case a test opted into real timers. jest.useFakeTimers(); }); it('opens the report in the wide RHP and then the pressed expense on top (after a short delay) on wide layouts', async () => { - // The pressed expense opens on a short setTimeout so the report's wide RHP settles first. Use real - // timers so that delayed navigation actually fires jest.useRealTimers(); mockResponsiveLayoutOverride = wideResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -552,8 +536,7 @@ describe('MoneyRequestReportPreview', () => { }); }); - // The report opens in the wide RHP first so it sits below, then the pressed expense opens on top - // of it (back returns to the report, not the Inbox). + // The report opens first and sits below, so back returns to it rather than the Inbox. const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''}); expect(navigateSpy).toHaveBeenCalledTimes(2); expect(navigateSpy).toHaveBeenNthCalledWith(1, reportRoute); @@ -561,8 +544,7 @@ describe('MoneyRequestReportPreview', () => { }); it('does not reopen the pressed expense if the user leaves the report during the wide-layout cascade delay', async () => { - // Regression: the report opens, but if the user dismisses its wide RHP (or navigates away) before the - // cascade timer fires, the delayed callback must not reopen the expense over whatever screen is now active. + // Regression: navigating away before the timer fires must not reopen the expense over the new screen. jest.useRealTimers(); mockResponsiveLayoutOverride = wideResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -582,8 +564,6 @@ describe('MoneyRequestReportPreview', () => { }); it('opens the report and then the pressed expense on top of it (after a short delay) on narrow layouts', async () => { - // The pressed expense opens on a short setTimeout so the report settles first. Use real timers so that - // delayed navigation actually fires. jest.useRealTimers(); mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -604,9 +584,7 @@ describe('MoneyRequestReportPreview', () => { }); it('keeps the pressed expense out of the split stack on narrow layouts', async () => { - // The expense must open in the RHP, never as a split-navigator screen: the flows that clean up after a - // thread (split-expense save, delete) assume it is not there. removeScreenByKey only filters the root - // navigator's routes, so a nested split screen can never be removed by it. + // Deploy blocker #97183: removeScreenByKey only filters the root navigator, so a nested split screen can never be removed. jest.useRealTimers(); mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -619,17 +597,13 @@ describe('MoneyRequestReportPreview', () => { }); }); - // The thread must never be navigated to as a report screen, whatever backTo it would carry. const threadID = `thread_${mockSecondTransactionID}`; const threadAsReportScreen = navigateSpy.mock.calls.map(([route]) => String(route)).filter((route) => route.startsWith(`r/${threadID}`)); expect(threadAsReportScreen).toEqual([]); - // ...and it did open, as the RHP route, so the assertion above is not passing merely because nothing opened. expect(navigateSpy).toHaveBeenLastCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: threadID, backTo: narrowReportRoute()})); }); it('does not open the pressed expense if the user leaves the report during the narrow cascade delay', async () => { - // Same guard the wide cascade has: the delayed navigation must not land on top of whatever screen the - // user moved to while the timer was pending. jest.useRealTimers(); mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -651,8 +625,7 @@ describe('MoneyRequestReportPreview', () => { it('fetches the report actions when the thread resolved only from the transaction, so the carousel can resolve siblings', async () => { mockResponsiveLayoutOverride = narrowResponsiveLayout; const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); - // Cache-clear shape: the IOU report's actions are absent, but each transaction still carries its own - // transactionThreadReportID, so the press resolves a thread WITHOUT loading the report's actions. + // Cache-clear shape: no report actions, but each transaction still carries its own thread id. jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); mockUseReportTransactionsCollection.mockImplementation(() => toCollectionDataSet( @@ -669,11 +642,8 @@ describe('MoneyRequestReportPreview', () => { await pressSecondTransaction(); await settleCascade(); - // The expense opens straight away from the transaction's own thread id... expect(navigateSpy).toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); - // ...but the report's actions must still be fetched. The prev/next carousel resolves each sibling through - // those actions; without them an arrow press cannot find the sibling's existing thread and mints a - // duplicate thread with no parent instead. + // The actions must still be fetched, or an arrow press mints a duplicate thread with no parent. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); }); @@ -687,14 +657,12 @@ describe('MoneyRequestReportPreview', () => { await pressSecondTransaction(); expect(navigateSpy).not.toHaveBeenCalled(); - // The tap looked dead, so the user opens the report the other way. That is an explicit choice and must - // supersede the deferred press — otherwise the fetch landing yanks them into the expense. + // Opening the report explicitly must supersede the deferred press. navigateSpy.mockClear(); fireEvent.press(screen.getByText(TestHelper.translateLocal('common.view'))); await waitForBatchedUpdatesWithAct(); expect(navigateSpy).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID, undefined, undefined, '')); - // The app is now on the report, not the chat the press was made from. The suite pins getActiveRoute to a - // constant, so model the real navigation for the assertion below to mean anything. + // The suite pins getActiveRoute, so model the real navigation for the assertion below to mean anything. jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(ROUTES.REPORT_WITH_ID.getRoute(mockIOUReport.reportID) as Route); navigateSpy.mockClear(); @@ -722,14 +690,12 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); - // Press the first card — it defers, waiting on the report's actions. const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); const [firstCard] = screen.getAllByText(transactionDisplayAmount); fireEvent.press(firstCard); await waitForBatchedUpdatesWithAct(); expect(navigateSpy).not.toHaveBeenCalled(); - // Now press the second card, which opens straight away. That is the expense the user is waiting on. navigateSpy.mockClear(); await pressSecondTransaction(); await settleCascade(); @@ -755,9 +721,7 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); await pressSecondTransaction(); - // Online, a delete-pending row is already filtered out of `transactions` upstream, so openableTransactionIDs - // must equal the full visible list. This pins the filter as an offline-only refinement — it would fail if - // someone widened the predicate (e.g. to one that also matches pendingFields) and started dropping live rows. + // Online, delete-pending rows are already filtered upstream, so the seed must equal the full visible list. expect(setActiveTransactionIDsSpy).toHaveBeenCalledWith(defaultPreviewTransactions.map((transaction) => transaction.transactionID)); }); @@ -775,8 +739,7 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); - // Issue #26939: deleting an expense offline must leave the preview VISIBLE (greyed out) rather than - // collapsing it. v2 only makes that row non-navigable — it must not disappear, so both cards still render. + // Issue #26939: an offline-deleted expense stays visible but non-navigable, so both cards still render. const {transactionDisplayAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); expect(screen.getAllByText(transactionDisplayAmount).length).toBeGreaterThanOrEqual(2); }); @@ -786,7 +749,6 @@ describe('MoneyRequestReportPreview', () => { mockUseNetwork.mockReturnValue({isOffline: true}); const setActiveTransactionIDsSpy = jest.spyOn(TransactionThreadNavigation, 'setActiveTransactionIDs'); jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); - // First row is live, second is delete-pending. Offline keeps the deleted row visible in the carousel. mockUseReportTransactionsCollection.mockImplementation(() => toCollectionDataSet( ONYXKEYS.COLLECTION.TRANSACTION, @@ -801,8 +763,7 @@ describe('MoneyRequestReportPreview', () => { fireEvent.press(liveRow); await waitForBatchedUpdatesWithAct(); - // Pressing the LIVE row must not seed the deleted sibling, otherwise the RHP's next arrow opens a thread - // that no longer exists and lands on "It's not here" (deploy blocker #97149, arrow path). + // Deploy blocker #97149: seeding a deleted sibling makes the next arrow land on "It's not here". expect(setActiveTransactionIDsSpy).toHaveBeenCalled(); const seededIDs = setActiveTransactionIDsSpy.mock.calls.at(-1)?.at(0); expect(seededIDs).not.toContain(mockSecondTransactionID); @@ -812,8 +773,7 @@ describe('MoneyRequestReportPreview', () => { mockResponsiveLayoutOverride = wideResponsiveLayout; mockUseNetwork.mockReturnValue({isOffline: true}); jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); - // Offline deletes stay in the carousel, but the thread is already gone — pressing it must not land on - // "It's not here" (deploy blocker #97149). + // Deploy blocker #97149: the thread is already gone, so pressing the row must not land on "It's not here". mockUseReportTransactionsCollection.mockImplementation(() => toCollectionDataSet( ONYXKEYS.COLLECTION.TRANSACTION, @@ -837,8 +797,7 @@ describe('MoneyRequestReportPreview', () => { await renderAndPopulateCarousel(); await pressSecondTransaction(); - // The thread already exists but may not be cached (offline / after a cache clear), so its optimistic - // report shell is seeded before navigating — otherwise the tap can land on a blank expense. + // The thread may not be cached, so its optimistic shell is seeded before navigating. expect(seedSpy).toHaveBeenCalledWith(`thread_${mockSecondTransactionID}`, mockIOUReport.reportID, expect.anything(), expect.anything()); }); @@ -860,19 +819,16 @@ describe('MoneyRequestReportPreview', () => { it('fetches the report actions and opens the pressed expense once they load, instead of the parent report, after a cache clear', async () => { mockResponsiveLayoutOverride = narrowResponsiveLayout; const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); - // Simulate a cache clear: the IOU report's actions are not loaded yet, so the pressed expense's - // thread cannot be resolved at press time. + // Cache clear: the report's actions are not loaded, so the thread cannot resolve at press time. const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); await renderAndPopulateCarousel(); await pressSecondTransaction(); - // The press fetches the IOU report's actions and waits, rather than falling back to the parent report. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); expect(navigateSpy).not.toHaveBeenCalled(); expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); - // Once the actions arrive the thread resolves and the pressed expense opens (report placed underneath). getIOUActionSpy.mockImplementation(buildActionWithThread); await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_loaded`]: mockAction}); @@ -884,8 +840,7 @@ describe('MoneyRequestReportPreview', () => { }); it('falls back to the parent report when the re-fetch settles with no report actions at all', async () => { - // Same shape as the test below, but nothing is ever cached for the report, so the action count stays 0. - // The fallback must key off the fetch settling, not off there being actions, or the tap stays dead. + // Nothing is ever cached here, so the fallback must key off the fetch settling, not off there being actions. mockResponsiveLayoutOverride = wideResponsiveLayout; jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); @@ -909,8 +864,7 @@ describe('MoneyRequestReportPreview', () => { it('falls back to the parent report once the re-fetch settles when the expense has no IOU action at all', async () => { mockResponsiveLayoutOverride = wideResponsiveLayout; const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); - // A legacy expense: the IOU report's actions are loaded, but none of them is this expense's IOU - // action, and re-fetching surfaces nothing new. + // A legacy expense: the actions are loaded but this expense's IOU action is missing, and refetching finds nothing. jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); await renderAndPopulateCarousel(); @@ -920,13 +874,10 @@ describe('MoneyRequestReportPreview', () => { }); await pressSecondTransaction(); - // The press defers and re-fetches the report's actions (the missing action may simply not be cached). expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); expect(navigateSpy).not.toHaveBeenCalled(); - // The fetch settles without changing the cached actions. The loading flip alone must drain the press to - // the parent report — regression: it used to wait for an action-count change that never came, leaving - // the tap permanently dead. + // Regression: the drain used to wait for an action-count change that never came, leaving the tap dead. await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${mockIOUReport.reportID}`, {isLoadingInitialReportActions: true}); await waitForBatchedUpdatesWithAct(); @@ -945,29 +896,25 @@ describe('MoneyRequestReportPreview', () => { const getIOUActionSpy = jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockReturnValue(undefined); await renderAndPopulateCarousel(); - // Partially seeded cache: some of the report's actions are present (e.g. from the app-wide bootstrap), - // but not the pressed expense's IOU action. + // Partially seeded cache: some actions are present, but not the pressed expense's IOU action. await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[mockAction.reportActionID]: mockAction}); await waitForBatchedUpdatesWithAct(); }); await pressSecondTransaction(); - // Regression: the press used to give up immediately (parent report) because some actions were cached; - // it must re-fetch instead — the missing IOU action may just not have been seeded. + // Regression: the press used to give up immediately because some actions were cached. expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: mockIOUReport.reportID})); expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: ''})); expect(navigateSpy).not.toHaveBeenCalled(); - // The fetch lands the missing IOU action — the pressed expense opens (report beneath), not the parent report. getIOUActionSpy.mockImplementation(buildActionWithThread); await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${mockIOUReport.reportID}`, {[`${mockAction.reportActionID}_loaded`]: mockAction}); await waitForBatchedUpdatesWithAct(); }); - // The expense must end up on top. The report opening underneath it is the cascade's base, but stopping - // there would mean the press fell back to the parent report instead of reaching the pressed expense. + // The expense must end up on top; stopping at the report would mean the press fell back to it. expect(navigateSpy).toHaveBeenLastCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: narrowReportRoute()})); }); @@ -988,8 +935,7 @@ describe('MoneyRequestReportPreview', () => { }); it('falls back to the full report view, not the super-wide RHP, when the pressed expense has no thread on narrow layouts', async () => { - // Every other fallback assertion here runs wide. Narrow has no super-wide RHP, so the fallback has to - // land on the report screen itself — the route the deleted-expense and offline dead-tap paths rely on. + // Narrow has no super-wide RHP, so the fallback lands on the report screen itself. mockResponsiveLayoutOverride = narrowResponsiveLayout; jest.spyOn(ReportActions, 'createTransactionThreadReport').mockReturnValue(undefined); jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation((reportID, transactionID) => { @@ -1007,8 +953,6 @@ describe('MoneyRequestReportPreview', () => { }); it('widens the RHP for the report and the pressed expense, and narrows the expense back when the press is abandoned', async () => { - // The widths are invisible in the rendered output, so without this the whole widen/release mechanism - // could be deleted and every other test here would still pass. jest.useRealTimers(); mockResponsiveLayoutOverride = wideResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -1032,9 +976,7 @@ describe('MoneyRequestReportPreview', () => { }); it('seeds the expense view carousel in the order the cards are rendered, not collection order', async () => { - // The carousel sorts before rendering, so the collection order and the on-screen order can differ. The - // arrows walk the seeded list, so seeding collection order makes "next" on the last card jump to the - // first one. These two are supplied newest-first and render oldest-first. + // Supplied newest-first and rendered oldest-first, so the arrows must walk render order, not collection order. mockResponsiveLayoutOverride = wideResponsiveLayout; const olderTransaction = {...mockTransaction, transactionID: 'ordering_older', created: '2026-08-01 00:00:00', amount: mockTransaction.amount * 3}; const newerTransaction = {...mockTransaction, transactionID: 'ordering_newer', created: '2026-08-20 00:00:00', amount: mockTransaction.amount * 5}; @@ -1065,8 +1007,7 @@ describe('MoneyRequestReportPreview', () => { }); it('does not open the pressed expense over the report when "View" is tapped during the cascade delay', async () => { - // Regression: "View" opens the same report route, so the cascade's own "did the user navigate away" guard - // does not catch it and the expense used to land on top, showing the details twice after going back. + // Regression: "View" opens the same report route, so the cascade's navigate-away guard does not catch it. jest.useRealTimers(); mockResponsiveLayoutOverride = wideResponsiveLayout; jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); @@ -1088,8 +1029,7 @@ describe('MoneyRequestReportPreview', () => { }); it('seeds every expense into the arrows, not just the ones the carousel renders', async () => { - // Regression: the carousel caps how many cards it draws, and seeding that capped list left the next arrow - // disabled on the last drawn card even though more expenses existed after it. + // Regression: seeding the capped list left the next arrow disabled on the last drawn card. mockResponsiveLayoutOverride = wideResponsiveLayout; const many = Array.from({length: 14}, (_, index) => ({ ...mockTransaction, diff --git a/tests/ui/components/ReportPreviewActionButtonTest.tsx b/tests/ui/components/ReportPreviewActionButtonTest.tsx index 16978159e785..944b723decde 100644 --- a/tests/ui/components/ReportPreviewActionButtonTest.tsx +++ b/tests/ui/components/ReportPreviewActionButtonTest.tsx @@ -10,8 +10,6 @@ import type {ValueOf} from 'type-fest'; import React from 'react'; import {View} from 'react-native'; -// The dispatcher owns no props and reads its decision from context, so drive the decision through the mocked context -// slice and stub each branch component with a spy so we can assert which one gets rendered for a given action. const mockActionState: {reportPreviewAction: ValueOf; connectedIntegration: ConnectionName | undefined} = { reportPreviewAction: CONST.REPORT.REPORT_PREVIEW_ACTIONS.VIEW, connectedIntegration: undefined, @@ -80,14 +78,11 @@ jest.mock('@components/ButtonComposed', () => { }; }); -// Marker style objects so the row layout is actually observable. With `{flex1: {}}` alone, `styles.flexRow` and -// `styles.gap2` resolved to undefined and were silently dropped from the style array, leaving the two-button row -// completely untested — a typo'd style key would have passed. +// These must stay distinct objects: with `{}` values the style assertions below pass even when the styles are missing. const mockStyles = {flex1: {flex: 1}, flexRow: {flexDirection: 'row'}, gap2: {gap: 8}}; jest.mock('@hooks/useThemeStyles', () => ({__esModule: true, default: () => mockStyles})); jest.mock('@hooks/useLocalize', () => ({__esModule: true, default: () => ({translate: (key: string) => key})})); -// Reads the style array off the component's outermost View without an unsafe cast. function flattenContainerStyle(rendered: ReturnType): unknown[] { const container = rendered.UNSAFE_getAllByType(View).at(0); const style: unknown = container?.props.style; @@ -118,8 +113,7 @@ describe('ReportPreviewActionButton', () => { mockActionState.connectedIntegration = CONST.POLICY.CONNECTIONS.NAME.QBO; render(); expect(mockExport).toHaveBeenCalled(); - // The View button now renders alongside the primary action button (here ExportActionButton) rather than - // instead of it, so it is expected to render too. + // View renders alongside the primary action, not instead of it. expect(mockView).toHaveBeenCalled(); }); @@ -131,14 +125,11 @@ describe('ReportPreviewActionButton', () => { expect(mockExport).not.toHaveBeenCalled(); }); it('lays the primary action and View out in a row, and keeps a lone View full-width', () => { - // Issue #91042 adds the grey View button beside the primary action. The row is width-capped, so the layout - // styles are load-bearing: without flexRow/gap2 the two buttons stack instead of sitting side by side. mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.PAY; const withPrimary = render(); expect(flattenContainerStyle(withPrimary)).toEqual(expect.arrayContaining([mockStyles.flexRow, mockStyles.gap2])); withPrimary.unmount(); - // With no primary action the View button stands alone and must NOT be laid out as a row. jest.clearAllMocks(); mockActionState.reportPreviewAction = CONST.REPORT.REPORT_PREVIEW_ACTIONS.VIEW; const viewOnly = render(); From 1d8b6e0da1de44e240e8f2c891bb8d6a1298fbf0 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Sun, 23 Aug 2026 21:32:52 +0700 Subject: [PATCH 19/31] Record why the pressed-expense cascade waits 180ms --- .../ReportActionItem/MoneyRequestReportPreview/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 8b7e240c85a5..0209fd6dcb8f 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -50,6 +50,7 @@ import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent const reportActionCountSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length; +// The stagger between the report and the expense that design asked for: https://github.com/Expensify/App/pull/92546#issuecomment-4687440972 const PRESSED_EXPENSE_CASCADE_DELAY = 180; function MoneyRequestReportPreview({ From 588ec9e6398b2e2a2037217a50fa16d03f776d92 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 27 Aug 2026 18:55:43 +0700 Subject: [PATCH 20/31] Wait for the parent action before minting a transaction thread from the arrows Pressing prev or next while the report's actions are still loading resolved no child report ID, so the fallback created a thread with no parent report and no parent action. Skip the press in that window instead. --- ...neyRequestReportTransactionsNavigation.tsx | 8 ++ ...equestReportTransactionsNavigationTest.tsx | 74 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 42731e90c0d7..6aa980b52da7 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -188,6 +188,10 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR } // The transaction thread doesn't exist yet, so we should create it if (!nextThreadReportID) { + // Without the parent action we would mint a thread with no parent, so wait for it to load instead. + if (!nextParentReportAction) { + return; + } const transactionThreadReport = createTransactionThreadReport({ introSelected, currentUserLogin: currentUserEmail ?? '', @@ -257,6 +261,10 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR } // The transaction thread doesn't exist yet, so we should create it if (!prevThreadReportID) { + // Without the parent action we would mint a thread with no parent, so wait for it to load instead. + if (!prevParentReportAction) { + return; + } const transactionThreadReport = createTransactionThreadReport({ introSelected, currentUserLogin: currentUserEmail ?? '', diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx new file mode 100644 index 000000000000..b57550052a36 --- /dev/null +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -0,0 +1,74 @@ +import {fireEvent, render, screen, waitFor} from '@testing-library/react-native'; + +import MoneyRequestReportTransactionsNavigation from '@components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation'; + +import * as ReportActions from '@libs/actions/Report'; + +import Navigation from '@navigation/Navigation'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {Transaction} from '@src/types/onyx'; + +import React from 'react'; +import Onyx from 'react-native-onyx'; + +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +jest.mock('@components/WideRHPContextProvider', () => ({ + useWideRHPActions: () => ({markReportRHPWidth: jest.fn(), unmarkReportRHPWidth: jest.fn()}), +})); + +jest.mock('@components/OnyxListItemProvider', () => ({ + usePersonalDetails: () => ({}), +})); + +jest.mock('@hooks/useCurrentUserPersonalDetails', () => ({ + __esModule: true, + default: () => ({email: 'a@b.com', accountID: 1}), +})); + +const IOU_REPORT_ID = 'iou1'; +const FIRST_TRANSACTION_ID = 't1'; +const SECOND_TRANSACTION_ID = 't2'; + +function buildTransaction(transactionID: string): Transaction { + return {transactionID, reportID: IOU_REPORT_ID, amount: 100, created: '2026-08-01', currency: 'USD'} as Transaction; +} + +describe('MoneyRequestReportTransactionsNavigation', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + await Onyx.clear(); + // The report's own actions are deliberately absent: this is the cache-cleared shape, where the + // seeded sibling IDs are known but the IOU actions that resolve them have not been fetched yet. + await Onyx.multiSet({ + [ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS]: [FIRST_TRANSACTION_ID, SECOND_TRANSACTION_ID], + [`${ONYXKEYS.COLLECTION.TRANSACTION}${FIRST_TRANSACTION_ID}`]: buildTransaction(FIRST_TRANSACTION_ID), + [`${ONYXKEYS.COLLECTION.TRANSACTION}${SECOND_TRANSACTION_ID}`]: buildTransaction(SECOND_TRANSACTION_ID), + }); + await waitForBatchedUpdates(); + }); + + it('does not mint a thread or navigate when next is pressed before the report actions load', async () => { + const createThreadSpy = jest.spyOn(ReportActions, 'createTransactionThreadReport'); + const setParamsSpy = jest.spyOn(Navigation, 'setParams').mockImplementation(() => {}); + + render(); + await waitForBatchedUpdates(); + + // Both arrows render with the generic button role; the second one is next. + const buttons = screen.getAllByLabelText(CONST.ROLE.BUTTON); + expect(buttons).toHaveLength(2); + fireEvent.press(buttons.at(1)); + + await waitFor(() => { + expect(createThreadSpy).not.toHaveBeenCalled(); + }); + expect(setParamsSpy).not.toHaveBeenCalled(); + }); +}); From d12bdc3155e91324d0ce78153fb2666f546c24b1 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 27 Aug 2026 19:03:50 +0700 Subject: [PATCH 21/31] Wait for the parent action before minting a transaction thread from the arrows Pressing prev or next while the report's actions are still loading resolves no child report ID, so the fallback created a thread with no parent report and no parent action. Skip the press in that window instead, and pass the concierge chat the thread helper now requires. --- ...neyRequestReportTransactionsNavigation.tsx | 12 ++++++++++++ .../MoneyRequestReportPreview/index.tsx | 5 ++++- ...equestReportTransactionsNavigationTest.tsx | 19 +++++++++++-------- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 3afdfd942a9e..9819ff267be5 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -251,6 +251,12 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } + // Until the report's actions load there is no parent action to hang a thread off, so a press here + // would mint one with no parent. Do nothing and let the in-flight fetch settle instead. + if (!nextParentReportAction) { + return; + } + const nextThreadReportID = nextParentReportAction?.childReportID; const navigationParams = {reportID: nextThreadReportID, reportActionID: undefined, anchorTransactionID: nextTransactionID, backTo}; @@ -313,6 +319,12 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } + // Until the report's actions load there is no parent action to hang a thread off, so a press here + // would mint one with no parent. Do nothing and let the in-flight fetch settle instead. + if (!prevParentReportAction) { + return; + } + const prevThreadReportID = prevParentReportAction?.childReportID; const navigationParams = {reportID: prevThreadReportID, reportActionID: undefined, anchorTransactionID: prevTransactionID, backTo}; diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 0209fd6dcb8f..7ee5727b48e9 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -75,6 +75,8 @@ function MoneyRequestReportPreview({ const {email: currentUserEmail, accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); const [betas] = useOnyx(ONYXKEYS.BETAS); + const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID); + const [conciergeChat] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${conciergeReportID}`); const invoiceReceiverPolicyID = chatReport?.invoiceReceiver && 'policyID' in chatReport.invoiceReceiver ? chatReport.invoiceReceiver.policyID : undefined; const [invoiceReceiverPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(invoiceReceiverPolicyID)}`); const invoiceReceiverPersonalDetail = chatReport?.invoiceReceiver && 'accountID' in chatReport.invoiceReceiver ? personalDetailsList?.[chatReport.invoiceReceiver.accountID] : null; @@ -203,6 +205,7 @@ function MoneyRequestReportPreview({ if (transactionID) { childReportID = createTransactionThreadReport({ introSelected, + conciergeChat, currentUserLogin: currentUserEmail ?? '', currentUserAccountID, betas, @@ -214,7 +217,7 @@ function MoneyRequestReportPreview({ } return childReportID; }, - [betas, currentUserAccountID, currentUserEmail, introSelected, iouReport, personalDetailsList, policyID], + [betas, conciergeChat, currentUserAccountID, currentUserEmail, introSelected, iouReport, personalDetailsList, policyID], ); const navigateToExpense = useCallback( diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index b57550052a36..adfade82b45b 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -13,6 +13,7 @@ import type {Transaction} from '@src/types/onyx'; import React from 'react'; import Onyx from 'react-native-onyx'; +import createRandomTransaction from '../utils/collections/transaction'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; jest.mock('@components/WideRHPContextProvider', () => ({ @@ -32,8 +33,8 @@ const IOU_REPORT_ID = 'iou1'; const FIRST_TRANSACTION_ID = 't1'; const SECOND_TRANSACTION_ID = 't2'; -function buildTransaction(transactionID: string): Transaction { - return {transactionID, reportID: IOU_REPORT_ID, amount: 100, created: '2026-08-01', currency: 'USD'} as Transaction; +function buildTransaction(transactionID: string, index: number): Transaction { + return {...createRandomTransaction(index), transactionID, reportID: IOU_REPORT_ID}; } describe('MoneyRequestReportTransactionsNavigation', () => { @@ -46,11 +47,9 @@ describe('MoneyRequestReportTransactionsNavigation', () => { await Onyx.clear(); // The report's own actions are deliberately absent: this is the cache-cleared shape, where the // seeded sibling IDs are known but the IOU actions that resolve them have not been fetched yet. - await Onyx.multiSet({ - [ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS]: [FIRST_TRANSACTION_ID, SECOND_TRANSACTION_ID], - [`${ONYXKEYS.COLLECTION.TRANSACTION}${FIRST_TRANSACTION_ID}`]: buildTransaction(FIRST_TRANSACTION_ID), - [`${ONYXKEYS.COLLECTION.TRANSACTION}${SECOND_TRANSACTION_ID}`]: buildTransaction(SECOND_TRANSACTION_ID), - }); + await Onyx.set(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, [FIRST_TRANSACTION_ID, SECOND_TRANSACTION_ID]); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${FIRST_TRANSACTION_ID}`, buildTransaction(FIRST_TRANSACTION_ID, 0)); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${SECOND_TRANSACTION_ID}`, buildTransaction(SECOND_TRANSACTION_ID, 1)); await waitForBatchedUpdates(); }); @@ -64,7 +63,11 @@ describe('MoneyRequestReportTransactionsNavigation', () => { // Both arrows render with the generic button role; the second one is next. const buttons = screen.getAllByLabelText(CONST.ROLE.BUTTON); expect(buttons).toHaveLength(2); - fireEvent.press(buttons.at(1)); + const nextButton = buttons.at(1); + if (!nextButton) { + throw new Error('next arrow did not render'); + } + fireEvent.press(nextButton); await waitFor(() => { expect(createThreadSpy).not.toHaveBeenCalled(); From e6eb279c03e52a3caabe6a9d3ec374be40e9333c Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Mon, 31 Aug 2026 16:20:10 +0700 Subject: [PATCH 22/31] Shorten the arrow guard comments --- .../MoneyRequestReportTransactionsNavigation.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 9819ff267be5..3d793661f1d0 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -251,8 +251,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - // Until the report's actions load there is no parent action to hang a thread off, so a press here - // would mint one with no parent. Do nothing and let the in-flight fetch settle instead. + // A thread created before the parent action loads would have no parent. if (!nextParentReportAction) { return; } @@ -319,8 +318,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - // Until the report's actions load there is no parent action to hang a thread off, so a press here - // would mint one with no parent. Do nothing and let the in-flight fetch settle instead. + // A thread created before the parent action loads would have no parent. if (!prevParentReportAction) { return; } From 09cd83372a587d28e76fd156f64cde0f20afa7bc Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Mon, 31 Aug 2026 23:18:45 +0700 Subject: [PATCH 23/31] Keep the newest seeded transaction IDs when the write is skipped setActiveTransactionIDs returned before updating lastSetIDs on its idempotent path, so a second press with identical contents left the module holding the first array. Callers compare that reference by identity to tell their own seed from a newer one, so a cancelled press could clear what a later press seeded. --- .../MoneyRequestReportTransactionsNavigation.tsx | 4 ++-- src/libs/actions/TransactionThreadNavigation.ts | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 5f460ba83b30..7fbeb9148d10 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -171,7 +171,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - const nextThreadReportID = nextParentReportAction?.childReportID; + const nextThreadReportID = nextParentReportAction.childReportID; const navigationParams = { reportID: nextThreadReportID, reportActionID: undefined, @@ -247,7 +247,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - const prevThreadReportID = prevParentReportAction?.childReportID; + const prevThreadReportID = prevParentReportAction.childReportID; const navigationParams = { reportID: prevThreadReportID, reportActionID: undefined, diff --git a/src/libs/actions/TransactionThreadNavigation.ts b/src/libs/actions/TransactionThreadNavigation.ts index 4e87112d3579..e84373b19b47 100644 --- a/src/libs/actions/TransactionThreadNavigation.ts +++ b/src/libs/actions/TransactionThreadNavigation.ts @@ -52,11 +52,14 @@ function areDescriptorMapsEqual(a: Record) { const nextDescriptors = siblingDescriptorsByTransactionID ?? null; const sameIDs = lastSetIDs?.length === ids.length && lastSetIDs.every((id, i) => id === ids.at(i)); - if (sameIDs && areDescriptorMapsEqual(lastSetDescriptors, nextDescriptors)) { - return Promise.resolve(); - } + const isUnchanged = sameIDs && areDescriptorMapsEqual(lastSetDescriptors, nextDescriptors); + // Track the newest array even when the write is skipped: callers compare this reference by identity to tell + // "my seed is still active" from "someone re-seeded after me", and a stale one makes a newer seed look older. lastSetIDs = ids; lastSetDescriptors = nextDescriptors; + if (isUnchanged) { + return Promise.resolve(); + } return Promise.all([Onyx.set(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, ids), Onyx.set(ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_THREAD_REPORT_IDS, nextDescriptors)]); } From 40d90e1126dec1eeab5d64f4ae5de2be72a285fb Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 1 Sep 2026 00:42:41 +0700 Subject: [PATCH 24/31] Fetch the sibling's parent report instead of dropping an arrow press MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The arrows are enabled on ID presence but the handlers bailed when the sibling's IOU action was not cached, so an enabled arrow could be a silent no-op — most reachable for expenses past the rendered cards, whose actions are least likely to be paginated in. Stage the press, fetch the parent report, and replay it when the action lands, but only while the user is still on the route they pressed from: this screen stays mounted under a pushed RHP, and resuming from there would pull them out of that screen with a stale backTo. --- ...neyRequestReportTransactionsNavigation.tsx | 52 +++++++++++-- ...equestReportTransactionsNavigationTest.tsx | 73 ++++++++++++++++++- 2 files changed, 118 insertions(+), 7 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 7fbeb9148d10..f9a381df8d9e 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -3,9 +3,10 @@ import PrevNextButtons from '@components/PrevNextButtons'; import {useWideRHPActions} from '@components/WideRHPContextProvider'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; +import useNetwork from '@hooks/useNetwork'; import useOnyx from '@hooks/useOnyx'; -import {createTransactionThreadReport, setOptimisticTransactionThread} from '@libs/actions/Report'; +import {createTransactionThreadReport, openReport, setOptimisticTransactionThread} from '@libs/actions/Report'; import {clearActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import type {RightModalNavigatorParamList} from '@libs/Navigation/types'; import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils'; @@ -22,8 +23,8 @@ import getEmptyArray from '@src/types/utils/getEmptyArray'; import type {GestureResponderEvent} from 'react-native'; import type {OnyxCollection} from 'react-native-onyx'; -import {findFocusedRoute} from '@react-navigation/native'; -import React, {startTransition, useCallback, useEffect, useMemo} from 'react'; +import {findFocusedRoute, useIsFocused} from '@react-navigation/native'; +import React, {startTransition, useCallback, useEffect, useMemo, useRef} from 'react'; type MoneyRequestReportRHPNavigationButtonsProps = { currentTransactionID: string; @@ -41,6 +42,11 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR const {email: currentUserEmail, accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const {markReportRHPWidth} = useWideRHPActions(); + const {isOffline} = useNetwork(); + const isFocused = useIsFocused(); + + // The sibling an arrow press is waiting on, with the route it was made from so a stale replay can be dropped. + const pendingSiblingRef = useRef<{transactionID: string; originRoute: string} | null>(null); const {prevTransactionID, nextTransactionID} = useMemo(() => { if (!transactionIDsList || transactionIDsList.length < 2) { @@ -123,9 +129,15 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR }; }, []); - if (transactionIDsList.length < 2) { - return; - } + // Holds the press rather than dropping it: fetching the sibling's parent report brings in the IOU action the + // thread hangs off, and the effect below replays the press once it lands. + const stageSiblingPress = (transactionID: string | undefined, parentReportID: string | undefined) => { + if (!transactionID || !parentReportID || isOffline) { + return; + } + pendingSiblingRef.current = {transactionID, originRoute: Navigation.getActiveRoute()}; + openReport({reportID: parentReportID, introSelected, conciergeChat, betas, currentUserAccountID, hasReportActions: true}); + }; const onNext = (e: GestureResponderEvent | KeyboardEvent | undefined) => { e?.preventDefault(); @@ -168,6 +180,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR // A thread created before the parent action loads would have no parent. if (!nextParentReportAction) { + stageSiblingPress(nextTransactionID, nextTransaction?.reportID); return; } @@ -244,6 +257,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR // A thread created before the parent action loads would have no parent. if (!prevParentReportAction) { + stageSiblingPress(prevTransactionID, prevTransaction?.reportID); return; } @@ -282,6 +296,32 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR }); }; + // Replays a staged press once its parent action arrives, but only if the user is still where they pressed — + // this screen stays mounted under a pushed RHP, and resuming from there would yank them out with a stale backTo. + useEffect(() => { + const pending = pendingSiblingRef.current; + if (!pending) { + return; + } + if (!isFocused || Navigation.getActiveRoute() !== pending.originRoute) { + pendingSiblingRef.current = null; + return; + } + if (pending.transactionID === nextTransactionID && nextParentReportAction) { + pendingSiblingRef.current = null; + onNext(undefined); + return; + } + if (pending.transactionID === prevTransactionID && prevParentReportAction) { + pendingSiblingRef.current = null; + onPrevious(undefined); + } + }); + + if (transactionIDsList.length < 2) { + return; + } + return ( { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- jest.requireActual() returns the real module for partial mocking + const actualNavigation = jest.requireActual('@react-navigation/native'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-return -- returning the real module plus one overridden hook is the standard Jest partial-mock pattern + return { + ...actualNavigation, + useIsFocused: () => true, + }; +}); + jest.mock('@components/WideRHPContextProvider', () => ({ useWideRHPActions: () => ({markReportRHPWidth: jest.fn(), unmarkReportRHPWidth: jest.fn()}), })); @@ -33,6 +45,17 @@ const IOU_REPORT_ID = 'iou1'; const FIRST_TRANSACTION_ID = 't1'; const SECOND_TRANSACTION_ID = 't2'; +function buildIOUActions(): OnyxReportActions { + const action = { + ...createRandomReportAction(2), + reportActionID: 'action2', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + childReportID: 'thread2', + originalMessage: {IOUTransactionID: SECOND_TRANSACTION_ID, IOUReportID: IOU_REPORT_ID, type: CONST.IOU.REPORT_ACTION_TYPE.CREATE, amount: 100, currency: 'USD'}, + }; + return {action2: action}; +} + function buildTransaction(transactionID: string, index: number): Transaction { return {...createRandomTransaction(index), transactionID, reportID: IOU_REPORT_ID}; } @@ -74,4 +97,52 @@ describe('MoneyRequestReportTransactionsNavigation', () => { }); expect(setParamsSpy).not.toHaveBeenCalled(); }); + + it('fetches the sibling parent report instead of dropping the press, then replays it when the action lands', async () => { + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + const setParamsSpy = jest.spyOn(Navigation, 'setParams').mockImplementation(() => {}); + + render(); + await waitForBatchedUpdates(); + + const buttons = screen.getAllByLabelText(CONST.ROLE.BUTTON); + const nextButton = buttons.at(1); + if (!nextButton) { + throw new Error('next arrow did not render'); + } + fireEvent.press(nextButton); + + // The press is staged rather than dropped: the sibling's parent report is fetched. + await waitFor(() => { + expect(openReportSpy).toHaveBeenCalledWith(expect.objectContaining({reportID: IOU_REPORT_ID})); + }); + expect(setParamsSpy).not.toHaveBeenCalled(); + }); + + it('abandons a staged press when the user has navigated elsewhere', async () => { + jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + const createThreadSpy = jest.spyOn(ReportActions, 'createTransactionThreadReport'); + const setParamsSpy = jest.spyOn(Navigation, 'setParams').mockImplementation(() => {}); + const getActiveRouteSpy = jest.spyOn(Navigation, 'getActiveRoute'); + getActiveRouteSpy.mockReturnValue(ROUTES.REPORT_WITH_ID.getRoute('origin')); + + render(); + await waitForBatchedUpdates(); + + const buttons = screen.getAllByLabelText(CONST.ROLE.BUTTON); + const nextButton = buttons.at(1); + if (!nextButton) { + throw new Error('next arrow did not render'); + } + fireEvent.press(nextButton); + await waitForBatchedUpdates(); + + // The user moved on before the fetch settled, so the staged press must not navigate. + getActiveRouteSpy.mockReturnValue(ROUTES.REPORT_WITH_ID.getRoute('elsewhere')); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${IOU_REPORT_ID}`, buildIOUActions()); + await waitForBatchedUpdates(); + + expect(setParamsSpy).not.toHaveBeenCalled(); + expect(createThreadSpy).not.toHaveBeenCalled(); + }); }); From c3a3ef3cb4b72d61c42f65294575cd117a99f820 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Tue, 1 Sep 2026 07:49:30 +0700 Subject: [PATCH 25/31] Open only one expense when two rows are pressed in quick succession Navigation from this hook is deferred behind the seed write while backTo is captured synchronously, so two presses made inside that window both navigated and the second expense was pushed on top of the first. Let only the press the user is still sitting on open. --- src/hooks/useNavigateToTransactionThread.ts | 7 +- .../useNavigateToTransactionThreadTest.tsx | 83 +++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 tests/unit/useNavigateToTransactionThreadTest.tsx diff --git a/src/hooks/useNavigateToTransactionThread.ts b/src/hooks/useNavigateToTransactionThread.ts index f3f59199b9be..0dcb9bfd4c94 100644 --- a/src/hooks/useNavigateToTransactionThread.ts +++ b/src/hooks/useNavigateToTransactionThread.ts @@ -55,7 +55,8 @@ function useNavigateToTransactionThread() { return ({transactionID, reportActions, report, transaction, siblingTransactionIDs, backTo}: NavigateToTransactionThreadParams) => { const iouAction = getIOUActionForTransactionID(reportActions, transactionID); - const resolvedBackTo = backTo ?? Navigation.getActiveRoute(); + const routeAtPress = Navigation.getActiveRoute(); + const resolvedBackTo = backTo ?? routeAtPress; let reportIDToNavigate = iouAction?.childReportID; const routeParams: {reportID: string | undefined; reportActionID?: string; backTo?: string} = { @@ -86,6 +87,10 @@ function useNavigateToTransactionThread() { // Single transaction report opens in RHP. We seed every sibling transaction ID so the RHP can // display prev/next arrows for navigation between expenses. setActiveTransactionIDs(siblingTransactionIDs).then(() => { + // A second press made before this resolves would stack its expense on top of the first. + if (Navigation.getActiveRoute() !== routeAtPress) { + return; + } if (reportIDToNavigate) { markReportRHPWidth(reportIDToNavigate, 'wide'); } diff --git a/tests/unit/useNavigateToTransactionThreadTest.tsx b/tests/unit/useNavigateToTransactionThreadTest.tsx new file mode 100644 index 000000000000..3fd53cf9126e --- /dev/null +++ b/tests/unit/useNavigateToTransactionThreadTest.tsx @@ -0,0 +1,83 @@ +import {renderHook} from '@testing-library/react-native'; + +import useNavigateToTransactionThread from '@hooks/useNavigateToTransactionThread'; + +import Navigation from '@navigation/Navigation'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import ROUTES from '@src/ROUTES'; +import type {Report, ReportAction} from '@src/types/onyx'; + +import Onyx from 'react-native-onyx'; + +import createRandomReportAction from '../utils/collections/reportActions'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +jest.mock('@components/OnyxListItemProvider', () => ({ + usePersonalDetails: () => ({}), +})); + +jest.mock('@components/WideRHPContextProvider', () => ({ + useWideRHPActions: () => ({markReportRHPWidth: jest.fn(), unmarkReportRHPWidth: jest.fn()}), +})); + +jest.mock('@hooks/useCurrentUserPersonalDetails', () => ({ + __esModule: true, + default: () => ({email: 'a@b.com', accountID: 1}), +})); + +const IOU_REPORT_ID = 'iou1'; +const REPORT_ROUTE = ROUTES.REPORT_WITH_ID.getRoute(IOU_REPORT_ID); + +function buildIOUAction(index: number, transactionID: string, childReportID: string) { + return { + ...createRandomReportAction(index), + reportActionID: `action${index}`, + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + childReportID, + originalMessage: {IOUTransactionID: transactionID, IOUReportID: IOU_REPORT_ID, type: CONST.IOU.REPORT_ACTION_TYPE.CREATE, amount: 100, currency: 'USD'}, + }; +} + +function buildReportActions(): ReportAction[] { + return [buildIOUAction(1, 't1', 'threadA'), buildIOUAction(2, 't2', 'threadB')]; +} + +describe('useNavigateToTransactionThread', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + await Onyx.clear(); + await waitForBatchedUpdates(); + }); + + it('opens only one expense when two rows are pressed inside the deferred window', async () => { + const navigateSpy = jest.spyOn(Navigation, 'navigate').mockImplementation(() => {}); + // Both presses are made while the report is still the active route, and the first navigation has not + // been dispatched yet — the shape that used to push two expense screens. + const getActiveRouteSpy = jest.spyOn(Navigation, 'getActiveRoute').mockReturnValue(REPORT_ROUTE); + + const {result} = renderHook(() => useNavigateToTransactionThread()); + const navigateToTransaction = result.current; + + const params = { + reportActions: buildReportActions(), + report: {reportID: IOU_REPORT_ID} as Report, + transaction: undefined, + siblingTransactionIDs: ['t1', 't2'], + backTo: undefined, + }; + navigateToTransaction({...params, transactionID: 't1'}); + navigateToTransaction({...params, transactionID: 't2'}); + + // The first continuation navigates; by the time the second runs the route has moved off the report. + getActiveRouteSpy.mockImplementation(() => (navigateSpy.mock.calls.length === 0 ? REPORT_ROUTE : ROUTES.SEARCH_REPORT.getRoute({reportID: 'threadA'}))); + await waitForBatchedUpdates(); + + expect(navigateSpy).toHaveBeenCalledTimes(1); + }); +}); From cd75767e1e17ba5498fdf0080623e6137d668b2b Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 2 Sep 2026 03:01:19 +0700 Subject: [PATCH 26/31] Stop re-rendering the preview on every IOU report action Subscribing to the report's action count re-rendered the preview, its provider and every card on each write, and a split writes a burst of actions, so the new expenses took far longer to show. Only a deferred press needs that trigger, so select the count only while one is pending and read a plain boolean for hasReportActions, which can flip once at most. --- .../MoneyRequestReportPreview/index.tsx | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 710685e7e59b..8d638388350d 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -48,7 +48,7 @@ import type {MoneyRequestReportPreviewProps} from './types'; import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent'; -const reportActionCountSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length; +const hasReportActionsSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length > 0; // The stagger between the report and the expense that design asked for: https://github.com/Expensify/App/pull/92546#issuecomment-4687440972 const PRESSED_EXPENSE_CASCADE_DELAY = 180; @@ -91,13 +91,24 @@ function MoneyRequestReportPreview({ const handleOrderedTransactionsChange = useCallback((orderedTransactions: Transaction[]) => { orderedTransactionsRef.current = orderedTransactions; }, []); - const [iouReportActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { - selector: reportActionCountSelector, + const [hasIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { + selector: hasReportActionsSelector, + }); + const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); + // Subscribing to the whole action list re-rendered every card on each write, and a split writes a burst of + // them. Narrow it to the one action a deferred press is waiting for, so nothing pending means nothing changes. + const pendingPressActionCountSelector = useCallback((reportActions: OnyxEntry) => { + if (!pendingExpenseTransactionRef.current) { + return undefined; + } + return Object.keys(reportActions ?? {}).length; + }, []); + const [pendingPressActionCount] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(iouReportID)}`, { + selector: pendingPressActionCountSelector, }); const [isLoadingInitialIOUReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${getNonEmptyStringOnyxID(iouReportID)}`, { selector: isLoadingInitialReportActionsSelector, }); - const pendingExpenseTransactionRef = useRef<{transaction: Transaction; originRoute: string} | null>(null); const cascadeTimerRef = useRef<{timer: ReturnType; release: () => void} | null>(null); const transactionsWithReceipts = getTransactionsWithReceipts(iouReportID, allReportTransactions); const hasNonReimbursableTransactions = hasNonReimbursableTransactionsTransactionUtils(allReportTransactions); @@ -340,7 +351,7 @@ function MoneyRequestReportPreview({ openReportFromPreview(); return; } - openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!iouReportActionCount}); + openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!hasIOUReportActions}); } navigateToExpense(childReportID); return; @@ -348,13 +359,13 @@ function MoneyRequestReportPreview({ if (!isIOUActionLoaded && iouReportID && !isOffline) { pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; - openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!iouReportActionCount}); + openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!hasIOUReportActions}); return; } openReportFromPreview(); }, - [betas, currentUserAccountID, introSelected, iouReportActionCount, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], + [betas, currentUserAccountID, hasIOUReportActions, introSelected, iouReportID, isOffline, navigateToExpense, openReportFromPreview, resolveChildReportID, transactions.length], ); useEffect(() => { @@ -375,7 +386,7 @@ function MoneyRequestReportPreview({ } pendingExpenseTransactionRef.current = null; openReportFromPreview(); - }, [iouReportActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); + }, [pendingPressActionCount, isFocused, isLoadingInitialIOUReportActions, navigateToExpense, openReportFromPreview, resolveChildReportID]); useEffect( () => () => { From 4a515eff15d7900da806c161f42ff40de6522f5e Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Wed, 2 Sep 2026 03:21:16 +0700 Subject: [PATCH 27/31] Keep the offline arrow working and release the report's super-wide hint Offline the arrow staged nothing and returned, so it did nothing at all; fall through to the optimistic thread instead, which is what happened before. Also unmark the parent report's super-wide hint when a cascade is cancelled, since that costs nothing even where the report clears it itself. --- ...neyRequestReportTransactionsNavigation.tsx | 18 +++++------ .../MoneyRequestReportPreview/index.tsx | 2 ++ ...equestReportTransactionsNavigationTest.tsx | 31 +++++++++++++++++++ 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index f9a381df8d9e..bbc8b92f8c45 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -132,11 +132,13 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR // Holds the press rather than dropping it: fetching the sibling's parent report brings in the IOU action the // thread hangs off, and the effect below replays the press once it lands. const stageSiblingPress = (transactionID: string | undefined, parentReportID: string | undefined) => { + // Offline there is no fetch to wait for, so the caller builds the thread optimistically instead. if (!transactionID || !parentReportID || isOffline) { - return; + return false; } pendingSiblingRef.current = {transactionID, originRoute: Navigation.getActiveRoute()}; openReport({reportID: parentReportID, introSelected, conciergeChat, betas, currentUserAccountID, hasReportActions: true}); + return true; }; const onNext = (e: GestureResponderEvent | KeyboardEvent | undefined) => { @@ -178,13 +180,12 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - // A thread created before the parent action loads would have no parent. - if (!nextParentReportAction) { - stageSiblingPress(nextTransactionID, nextTransaction?.reportID); + // A thread created before the parent action loads would have no parent, so wait for the fetch. + if (!nextParentReportAction && stageSiblingPress(nextTransactionID, nextTransaction?.reportID)) { return; } - const nextThreadReportID = nextParentReportAction.childReportID; + const nextThreadReportID = nextParentReportAction?.childReportID; const navigationParams = { reportID: nextThreadReportID, reportActionID: undefined, @@ -255,13 +256,12 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return; } - // A thread created before the parent action loads would have no parent. - if (!prevParentReportAction) { - stageSiblingPress(prevTransactionID, prevTransaction?.reportID); + // A thread created before the parent action loads would have no parent, so wait for the fetch. + if (!prevParentReportAction && stageSiblingPress(prevTransactionID, prevTransaction?.reportID)) { return; } - const prevThreadReportID = prevParentReportAction.childReportID; + const prevThreadReportID = prevParentReportAction?.childReportID; const navigationParams = { reportID: prevThreadReportID, reportActionID: undefined, diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 8d638388350d..782c0651b3c2 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -293,6 +293,8 @@ function MoneyRequestReportPreview({ markReportRHPWidth(childReportID, 'wide'); const release = () => { unmarkReportRHPWidth(childReportID); + // Only clears if the report still carries our hint, so it can't undo one set by the report itself. + unmarkReportRHPWidth(iouReportID, 'super-wide'); seeded.then(() => { if (getActiveTransactionIDs().ids !== openableTransactionIDs) { return; diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index 1528420fa2e2..453ecba4ef86 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -36,6 +36,13 @@ jest.mock('@components/OnyxListItemProvider', () => ({ usePersonalDetails: () => ({}), })); +const mockIsOffline = {value: false}; + +jest.mock('@hooks/useNetwork', () => ({ + __esModule: true, + default: () => ({isOffline: mockIsOffline.value}), +})); + jest.mock('@hooks/useCurrentUserPersonalDetails', () => ({ __esModule: true, default: () => ({email: 'a@b.com', accountID: 1}), @@ -67,6 +74,7 @@ describe('MoneyRequestReportTransactionsNavigation', () => { beforeEach(async () => { jest.clearAllMocks(); + mockIsOffline.value = false; await Onyx.clear(); // The report's own actions are deliberately absent: this is the cache-cleared shape, where the // seeded sibling IDs are known but the IOU actions that resolve them have not been fetched yet. @@ -145,4 +153,27 @@ describe('MoneyRequestReportTransactionsNavigation', () => { expect(setParamsSpy).not.toHaveBeenCalled(); expect(createThreadSpy).not.toHaveBeenCalled(); }); + + it('still builds the thread optimistically when offline, rather than leaving the arrow dead', async () => { + mockIsOffline.value = true; + const openReportSpy = jest.spyOn(ReportActions, 'openReport').mockImplementation(() => {}); + const createThreadSpy = jest.spyOn(ReportActions, 'createTransactionThreadReport'); + jest.spyOn(Navigation, 'setParams').mockImplementation(() => {}); + + render(); + await waitForBatchedUpdates(); + + const buttons = screen.getAllByLabelText(CONST.ROLE.BUTTON); + const nextButton = buttons.at(1); + if (!nextButton) { + throw new Error('next arrow did not render'); + } + fireEvent.press(nextButton); + + // Offline there is nothing to fetch, so the press must fall through instead of being staged. + await waitFor(() => { + expect(createThreadSpy).toHaveBeenCalled(); + }); + expect(openReportSpy).not.toHaveBeenCalled(); + }); }); From cbfe93c9bb0cf14067103e21f232e457ef718a5e Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 3 Sep 2026 04:36:02 +0700 Subject: [PATCH 28/31] Give the replay effect an explicit dependency list Also record why the arrow's openReport passes hasReportActions unconditionally: it is fetching that report's actions, so it must not overwrite the cached name. --- .../MoneyRequestReportTransactionsNavigation.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index bbc8b92f8c45..68980ff49b4a 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -137,6 +137,7 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR return false; } pendingSiblingRef.current = {transactionID, originRoute: Navigation.getActiveRoute()}; + // Always true here: we are fetching this report's actions, so it must not overwrite its cached name. openReport({reportID: parentReportID, introSelected, conciergeChat, betas, currentUserAccountID, hasReportActions: true}); return true; }; @@ -316,7 +317,8 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR pendingSiblingRef.current = null; onPrevious(undefined); } - }); + // eslint-disable-next-line react-hooks/exhaustive-deps -- onNext/onPrevious are rebuilt every render, so listing them would defeat the dependency list + }, [isFocused, nextTransactionID, nextParentReportAction, prevTransactionID, prevParentReportAction]); if (transactionIDsList.length < 2) { return; From 7588a5daabaf6d385010d5dd5e2aed33838af23d Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Thu, 3 Sep 2026 14:54:19 +0700 Subject: [PATCH 29/31] Drop two comments the field names already state --- .../MoneyRequestReportTransactionsNavigation.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx index 68980ff49b4a..54a46ef6e741 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx @@ -45,7 +45,6 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR const {isOffline} = useNetwork(); const isFocused = useIsFocused(); - // The sibling an arrow press is waiting on, with the route it was made from so a stale replay can be dropped. const pendingSiblingRef = useRef<{transactionID: string; originRoute: string} | null>(null); const {prevTransactionID, nextTransactionID} = useMemo(() => { @@ -129,8 +128,6 @@ function MoneyRequestReportTransactionsNavigation({currentTransactionID, isFromR }; }, []); - // Holds the press rather than dropping it: fetching the sibling's parent report brings in the IOU action the - // thread hangs off, and the effect below replays the press once it lands. const stageSiblingPress = (transactionID: string | undefined, parentReportID: string | undefined) => { // Offline there is no fetch to wait for, so the caller builds the thread optimistically instead. if (!transactionID || !parentReportID || isOffline) { From 145b823e83342b6a3e0d24dae6b9ca8528a47cc8 Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Fri, 4 Sep 2026 05:27:34 +0700 Subject: [PATCH 30/31] Build preview navigation from the route captured at press time A second card press, or a "View" press, inside the cascade window ran after the first press had already navigated, so backTo was read from the report route itself and the report was pushed again nested under its own route. Capture the route once per press and build every backTo from it. When the press was made from the report, reuse that report's own backTo and skip the push, since the report is already open. The helper strips the leading slash Navigation.getActiveRoute() returns, which the route builders never emit. Two tests press a second card, and "View", inside the window against a route mock shaped like the real one, and check the report is pushed exactly once with the chat as backTo. --- .../MoneyRequestReportPreviewProvider.tsx | 19 ++++-- .../MoneyRequestReportPreview/index.tsx | 44 ++++++++---- .../resolvePressOrigin.ts | 18 +++++ tests/ui/MoneyRequestReportPreview.test.tsx | 67 +++++++++++++++++++ 4 files changed, 129 insertions(+), 19 deletions(-) create mode 100644 src/components/ReportActionItem/MoneyRequestReportPreview/resolvePressOrigin.ts diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx index 1647e99644e6..2b979e811261 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewProvider.tsx @@ -47,6 +47,7 @@ import { ReportPreviewTransactionViolationsContext, ReportPreviewUIStateContext, } from './MoneyRequestReportPreviewContext'; +import resolvePressOrigin from './resolvePressOrigin'; import usePreviewMessageAnimation from './usePreviewMessageAnimation'; import useReportPreviewActionDecision from './useReportPreviewActionDecision'; import useReportPreviewCarousel from './useReportPreviewCarousel'; @@ -213,7 +214,16 @@ function MoneyRequestReportPreviewProvider({ if (!iouReportID) { return; } + const routeAtPress = Navigation.getActiveRoute(); onCancelPendingPress?.(); + + // "View" pressed inside the cascade window lands on a report a card press already opened, so there is + // nothing left to push; pushing it again would nest the report under itself. + const {wasPressedFromReport, backTo} = resolvePressOrigin(routeAtPress, isSmallScreenWidth ? `r/${iouReportID}` : `e/${iouReportID}`); + if (wasPressedFromReport) { + return; + } + startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreviewContent', op: CONST.TELEMETRY.SPAN_OPEN_REPORT, @@ -221,14 +231,9 @@ function MoneyRequestReportPreviewProvider({ // Small screens navigate to full report view since super wide RHP // is not available on narrow layouts and would break the navigation logic. if (isSmallScreenWidth) { - Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute())); + Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, backTo)); } else { - Navigation.navigate( - ROUTES.EXPENSE_REPORT_RHP.getRoute({ - reportID: iouReportID, - backTo: Navigation.getActiveRoute(), - }), - ); + Navigation.navigate(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo})); } }, [iouReportID, isSmallScreenWidth, onCancelPendingPress]); diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx index 782c0651b3c2..4792af911304 100644 --- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx @@ -47,6 +47,7 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import type {MoneyRequestReportPreviewProps} from './types'; import MoneyRequestReportPreviewContent from './MoneyRequestReportPreviewContent'; +import resolvePressOrigin from './resolvePressOrigin'; const hasReportActionsSelector = (reportActions: OnyxEntry) => Object.keys(reportActions ?? {}).length > 0; @@ -178,8 +179,16 @@ function MoneyRequestReportPreview({ return; } + const routeAtPress = Navigation.getActiveRoute(); cancelPendingPress(); + // "View" pressed inside the cascade window lands on a report a card press already opened, so there is + // nothing left to push; pushing it again would nest the report under itself. + const {wasPressedFromReport, backTo} = resolvePressOrigin(routeAtPress, isSmallScreenWidth ? `r/${iouReportID}` : `e/${iouReportID}`); + if (wasPressedFromReport) { + return; + } + startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${iouReportID}`, { name: 'MoneyRequestReportPreview', op: CONST.TELEMETRY.SPAN_OPEN_REPORT, @@ -187,9 +196,9 @@ function MoneyRequestReportPreview({ // Small screens navigate to full report view since super wide RHP // is not available on narrow layouts and would break the navigation logic. if (isSmallScreenWidth) { - Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute())); + Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, backTo)); } else { - Navigation.navigate(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()})); + Navigation.navigate(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo})); } }, [cancelPendingPress, iouReportID, isSmallScreenWidth]); const [hasOnceLoadedReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${chatReportID}`, { @@ -244,8 +253,11 @@ function MoneyRequestReportPreview({ [betas, conciergeChat, currentUserAccountID, currentUserEmail, introSelected, iouReport, personalDetailsList, policyID], ); + // `routeAtPress` is captured when the user pressed, not read live: a second press inside the cascade window + // runs after the first press already navigated, so the live route is the report we opened and `backTo` would + // point at itself. const navigateToExpense = useCallback( - (childReportID: string) => { + (childReportID: string, routeAtPress: string) => { startSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${childReportID}`, { name: 'MoneyRequestReportPreview.Transaction', op: CONST.TELEMETRY.SPAN_OPEN_REPORT, @@ -256,8 +268,11 @@ function MoneyRequestReportPreview({ .map((pressedTransaction) => pressedTransaction.transactionID); if (isSmallScreenWidth && iouReportID) { - const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, Navigation.getActiveRoute()); - Navigation.navigate(reportRoute); + const {wasPressedFromReport, backTo} = resolvePressOrigin(routeAtPress, `r/${iouReportID}`); + const reportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReportID, undefined, undefined, backTo); + if (!wasPressedFromReport) { + Navigation.navigate(reportRoute); + } const seeded = setActiveTransactionIDs(openableTransactionIDs); const release = () => { seeded.then(() => { @@ -281,14 +296,17 @@ function MoneyRequestReportPreview({ if (isSmallScreenWidth) { setActiveTransactionIDs(openableTransactionIDs); - Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: routeAtPress})); return; } if (iouReportID) { - const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo: Navigation.getActiveRoute()}); + const {wasPressedFromReport, backTo} = resolvePressOrigin(routeAtPress, `e/${iouReportID}`); + const reportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: iouReportID, backTo}); markReportRHPWidth(iouReportID, 'super-wide'); - Navigation.navigate(reportRoute); + if (!wasPressedFromReport) { + Navigation.navigate(reportRoute); + } const seeded = setActiveTransactionIDs(openableTransactionIDs); markReportRHPWidth(childReportID, 'wide'); const release = () => { @@ -316,7 +334,7 @@ function MoneyRequestReportPreview({ setActiveTransactionIDs(openableTransactionIDs).then(() => { markReportRHPWidth(childReportID, 'wide'); - Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: Navigation.getActiveRoute()})); + Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute({reportID: childReportID, backTo: routeAtPress})); }); }, [isSmallScreenWidth, iouReportID, markReportRHPWidth, unmarkReportRHPWidth, transactions], @@ -328,6 +346,8 @@ function MoneyRequestReportPreview({ return; } + const routeAtPress = Navigation.getActiveRoute(); + pendingExpenseTransactionRef.current = null; if (cascadeTimerRef.current) { clearTimeout(cascadeTimerRef.current.timer); @@ -355,12 +375,12 @@ function MoneyRequestReportPreview({ } openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!hasIOUReportActions}); } - navigateToExpense(childReportID); + navigateToExpense(childReportID, routeAtPress); return; } if (!isIOUActionLoaded && iouReportID && !isOffline) { - pendingExpenseTransactionRef.current = {transaction, originRoute: Navigation.getActiveRoute()}; + pendingExpenseTransactionRef.current = {transaction, originRoute: routeAtPress}; openReport({reportID: iouReportID, introSelected, betas, currentUserAccountID, hasReportActions: !!hasIOUReportActions}); return; } @@ -383,7 +403,7 @@ function MoneyRequestReportPreview({ const childReportID = resolveChildReportID(pendingTransaction); if (childReportID) { pendingExpenseTransactionRef.current = null; - navigateToExpense(childReportID); + navigateToExpense(childReportID, pendingPress.originRoute); return; } pendingExpenseTransactionRef.current = null; diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/resolvePressOrigin.ts b/src/components/ReportActionItem/MoneyRequestReportPreview/resolvePressOrigin.ts new file mode 100644 index 000000000000..753a9394e794 --- /dev/null +++ b/src/components/ReportActionItem/MoneyRequestReportPreview/resolvePressOrigin.ts @@ -0,0 +1,18 @@ +/** + * Works out where a preview press was made from, so the report route it opens can be built from the press-time route. + * + * A press made while the report is already open (a second card, or "View", inside the cascade window) carries that + * report's own route, `backTo` included. Reusing that `backTo` rebuilds the identical route instead of nesting the + * report inside itself, and tells the caller there is nothing left to push. + */ +function resolvePressOrigin(routeAtPress: string, reportPath: string) { + const [path, query] = routeAtPress.split('?'); + // `Navigation.getActiveRoute()` returns a leading slash that the route builders never emit. + const pressedPath = path?.startsWith('/') ? path.substring(1) : path; + if (pressedPath !== reportPath) { + return {wasPressedFromReport: false, backTo: routeAtPress}; + } + return {wasPressedFromReport: true, backTo: new URLSearchParams(query).get('backTo') ?? ''}; +} + +export default resolvePressOrigin; diff --git a/tests/ui/MoneyRequestReportPreview.test.tsx b/tests/ui/MoneyRequestReportPreview.test.tsx index c9fbe23808a6..c5a236ff7a54 100644 --- a/tests/ui/MoneyRequestReportPreview.test.tsx +++ b/tests/ui/MoneyRequestReportPreview.test.tsx @@ -633,6 +633,73 @@ describe('MoneyRequestReportPreview', () => { expect(navigateSpy).not.toHaveBeenCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: reportRoute})); }); + it('keeps backTo pointing at the chat when a second card is pressed inside the cascade window', async () => { + // Regression: the second press read backTo from the active route, which press 1 had already + // moved to the report, so the report was pushed with a backTo pointing at itself. + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + + // The constant getActiveRoute mock in beforeEach hides this bug: let the route follow navigation. + // The real getActiveRoute returns a leading slash that the route builders never emit, so model that too. + let activeRoute = ''; + jest.spyOn(Navigation, 'getActiveRoute').mockImplementation(() => activeRoute); + navigateSpy.mockImplementation((route) => { + activeRoute = `/${String(route)}`; + }); + + await renderAndPopulateCarousel(); + + const {transactionDisplayAmount: firstAmount} = getTransactionDisplayAmountAndMetadataText(mockTransaction); + const {transactionDisplayAmount: secondAmount} = getTransactionDisplayAmountAndMetadataText(mockSecondTransaction); + + // The first amount also appears in the report total, so take the card. + const [firstCard] = screen.getAllByText(firstAmount); + fireEvent.press(firstCard); + await waitForBatchedUpdatesWithAct(); + // Second press lands before the 180ms cascade timer fires, while the carousel is still mounted. + fireEvent.press(screen.getByText(secondAmount)); + await waitForBatchedUpdatesWithAct(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + // The report is pushed exactly once, with the chat as backTo, and the second expense lands on top of it. + const chatBackedReportRoute = ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''}); + const reportRoutes = navigateSpy.mock.calls.map(([route]) => String(route)).filter((route) => route.startsWith(`e/${mockIOUReport.reportID}`)); + expect(reportRoutes).toEqual([chatBackedReportRoute]); + expect(navigateSpy).toHaveBeenCalledTimes(2); + expect(navigateSpy).toHaveBeenLastCalledWith(ROUTES.SEARCH_REPORT.getRoute({reportID: `thread_${mockSecondTransactionID}`, backTo: chatBackedReportRoute})); + }); + + it('pushes the report once, with the chat as backTo, when "View" is pressed inside the cascade window', async () => { + // Regression: "View" read backTo from the live route, which the card press had already moved to the + // report, so the report was pushed a second time with a backTo pointing at itself. + jest.useRealTimers(); + mockResponsiveLayoutOverride = wideResponsiveLayout; + jest.spyOn(ReportActionUtils, 'getIOUActionForReportID').mockImplementation(buildActionWithThread); + let activeRoute = ''; + jest.spyOn(Navigation, 'getActiveRoute').mockImplementation(() => activeRoute); + navigateSpy.mockImplementation((route) => { + activeRoute = `/${String(route)}`; + }); + + await renderAndPopulateCarousel(); + await pressSecondTransaction(); + fireEvent.press(screen.getByText(TestHelper.translateLocal('common.view'))); + await waitForBatchedUpdatesWithAct(); + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 350); + }); + }); + + expect(navigateSpy).toHaveBeenCalledTimes(1); + expect(navigateSpy).toHaveBeenCalledWith(ROUTES.EXPENSE_REPORT_RHP.getRoute({reportID: mockIOUReport.reportID, backTo: ''})); + }); + it('opens the report and then the pressed expense on top of it (after a short delay) on narrow layouts', async () => { jest.useRealTimers(); mockResponsiveLayoutOverride = narrowResponsiveLayout; From 0e29d3de60cf717d1ef9d65b81374cc2a7a9eeed Mon Sep 17 00:00:00 2001 From: Wildan Muhlis Date: Sat, 5 Sep 2026 10:19:08 +0700 Subject: [PATCH 31/31] Keep the preview's carousel order for the expense arrows when the report list re-seeds The report list re-seeds the prev/next arrows whenever its visual order changes while an expense is focused on top. A press in the report preview seeds the same rows in the carousel's order (violations first), so the refresh that follows the press replaced that order with the list's own and the first card no longer had a disabled prev arrow. The list now keeps a seed that covers exactly its rows and only re-seeds when the rows themselves change. --- .../MoneyRequestReportTransactionList.tsx | 11 ++++- ...ransactionListActiveTransactionIDsTest.tsx | 41 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 4e7f31a04cf2..1d41ba464ca0 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -602,9 +602,18 @@ function MoneyRequestReportTransactionList({ // "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) { + const {ids: activeIDs, descriptors: activeDescriptors} = getActiveTransactionIDs(); + if (activeDescriptors) { return; } + // A report preview press seeds these arrows in the carousel's order, which can differ from this list's order. + // Keep that seed while it still covers exactly these rows, and re-seed only when the rows themselves change. + if (activeIDs && activeIDs.length === visualOrderTransactionIDs.length) { + const activeIDSet = new Set(activeIDs); + if (visualOrderTransactionIDs.every((transactionID) => activeIDSet.has(transactionID))) { + return; + } + } setActiveTransactionIDs(visualOrderTransactionIDs); return () => { clearActiveTransactionIDs(); diff --git a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx index 5ca692bb657c..3e77e1a8826c 100644 --- a/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx +++ b/tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx @@ -45,9 +45,16 @@ function useActiveTransactionIDsEffect(visualOrderTransactionIDs: string[]) { if (focusedRoute?.name !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) { return; } - if (getActiveTransactionIDs().descriptors) { + const {ids: activeIDs, descriptors: activeDescriptors} = getActiveTransactionIDs(); + if (activeDescriptors) { return; } + if (activeIDs && activeIDs.length === visualOrderTransactionIDs.length) { + const activeIDSet = new Set(activeIDs); + if (visualOrderTransactionIDs.every((transactionID) => activeIDSet.has(transactionID))) { + return; + } + } setActiveTransactionIDs(visualOrderTransactionIDs); return () => { clearActiveTransactionIDs(); @@ -209,6 +216,38 @@ describe('MoneyRequestReportTransactionList - Active Transaction IDs Effect', () expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); }); + it('should keep an active seed that covers the same rows in a different order', () => { + // Given the focused route is SEARCH_REPORT and a report preview press seeded the same rows in carousel order + mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'}); + mockGetActiveTransactionIDs.mockReturnValue({ids: ['trans3', 'trans1', 'trans2'], 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 the carousel order nor clear it + expect(mockSetActiveTransactionIDs).not.toHaveBeenCalled(); + + unmount(); + + expect(mockClearActiveTransactionIDs).not.toHaveBeenCalled(); + }); + + it('should re-seed when the active seed covers different rows', () => { + // Given the focused route is SEARCH_REPORT and the active seed is missing one of the rows + mockFindFocusedRoute.mockReturnValue({name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'test-key'}); + mockGetActiveTransactionIDs.mockReturnValue({ids: ['trans2', 'trans1'], descriptors: null}); + + const transactionIDs = ['trans1', 'trans2', 'trans3']; + + // When the hook is rendered + renderHook(() => useActiveTransactionIDsEffect(transactionIDs)); + + // Then setActiveTransactionIDs should be called with the visual order + expect(mockSetActiveTransactionIDs).toHaveBeenCalledWith(transactionIDs); + }); + 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'});