Navigate to the clicked expense in a multi-expense report preview (v2) - #97308
MonilBhavsar merged 51 commits into
Conversation
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 Expensify#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 (Expensify#97184 replaced the deleted thread with a duplicate report route, Expensify#97183 left a stale route after a split save, Expensify#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 (Expensify#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'.
…expense-preview-per-item-nav-v2
…f 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 (Expensify#97183), and it leaves the discarded thread as a focused full-screen report while its data is torn down (Expensify#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 (Expensify#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.
…expense-preview-per-item-nav-v2
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.
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 Expensify#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.
…ne seeding Two gaps the existing suite left open: - No test asserted that an offline delete-pending expense is still RENDERED. Issue Expensify#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.
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.
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.
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.
…expense-preview-per-item-nav-v2
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.
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.
…expense-preview-per-item-nav-v2 # Conflicts: # src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
…expense-preview-per-item-nav-v2
…expense-preview-per-item-nav-v2
|
@trjExpensify, @jayeshmangwani is it possible that regression tests are conducted before merging into main? I suspect there might still be a hidden error because this change is affecting many existing flow. |
|
@huult Sorry, that reply was analysis when you asked for a recording. Android, current branch — PR flows, rapid double-press, and the split after the update: 97308-1-card-back-view-arrows 97308-1-card-back-view-arrows.mp497308-2-rapid-double-press 97308-2-rapid-double-press.mp497308-3-split-after-update 97308-3-split-after-update.mp4 |
…expense-preview-per-item-nav-v2 # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx
…expense-preview-per-item-nav-v2 # Conflicts: # tests/ui/MoneyRequestReportPreview.test.tsx
|
@wildan-m Could you test this on the iOS simulator as well? There’s an issue mentioned here: #97308 (comment). Could you do one round of testing on the simulator to make sure everything works correctly? I just retested it, and it’s still happening. |
…expense-preview-per-item-nav-v2
|
@huult i can repro this in main #97308 (comment) Kapture.2026-09-05.at.09.26.11.mp4 |
…ort 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.
|
iOS simulator. Tap card → expense → Back → report → Back → chat 01-card-tap-expense-back-report-back-chat.mp4View → report 02-view-button-opens-report.mp4#97184 delete → Back 03-blocker-97184-delete-from-details-then-back.mp4#97183 split → Save → Back 04-blocker-97183-split-save-then-back.mp4#97158 review duplicates 05-blocker-97158-review-duplicates-keep-confirm.mp4Expense arrows, first card 06-expense-arrows-first-card-prev-disabled.mp4Split → Save, hands off branch-split-halves-7s-handsoff.mp4 |
Screen.Recording.2026-09-06.at.09.55.43.mov@MonilBhavsar The PR is ready. All yours for final review. |
…expense-preview-per-item-nav-v2
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.74-0 🚀
|
|
🤖 Yes — help site changes are required. Draft PR: #100634 One article covers report previews, and it never documented what pressing an expense card does — so this PR changed behavior the help site was silent about. Article updated: Its Report Previews section listed what a preview shows ("Up to 10 expense previews", "Carousel navigation", "+X more link if there are over 10 expenses") but nothing about pressing a card. Added:
Nothing needed correcting. The existing "Up to 10 expense previews" and "+X more" lines still match the code — UI verification and authoring notesI drove the web build to check every label before writing, rather than take them from the PR description:
Against the three authoring docs: both new headings are task-based One thing I couldn't do: @wildan-m, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #100669 was identified to be related to this PR |
|
Deploy Blocker #100673 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
v2 of #92546, which was reverted in #97198 after five deploy blockers.
Tapping a card in the multi-expense Report Preview carousel opens that expense instead of the parent report.
What caused the blockers, and what v2 does about it
v1 inserted the parent report beneath the expense by pushing the thread into the split navigator — a spliced stack entry on native (
Navigation.openExpenseOverParentReport) and an extra history entry on mobile web. Every native blocker traced back to that hand-built stack being mutated by another flow:goBackcouldn't param-match the spliced parent (itsbackToisn't inrouteParamsIgnore) → REPLACE inserted a duplicate report route → back showed the report twiceremoveScreenByKeyonly filters the root navigator, so the stale thread route nested in the split survived → back landed on the LHNv2 removes the splice entirely.
Navigation.tsis untouched by this PR — no custom stack manipulation, nothing for those flows to trip over. The back order is preserved by opening the report as an ordinary split screen and keeping only the expense in the RHP, so the thread never becomes a nested split-navigator screen. #97149 is additionally guarded explicitly: a pending-DELETE press opens the parent report, and offline-deleted expenses are kept out of the expense view's prev/next list.The expense view's prev/next arrows are also seeded with every expense on the report, in the order the carousel sorts them (RBR first, then date) rather than raw collection order, so on a report where those differ the arrows no longer walk a sequence that isn't on screen, and on a report with more than 11 expenses they still reach the ones the carousel does not render.
Note on #97155**.** It was closed with the revert, and its expected result was "only the specific expense should open". This PR opens the report underneath and cascades the expense on top, on both layouts — that is what preserves the pre-revert back order. Flagging it explicitly so it can be confirmed rather than assumed.
Fixed Issues
$ #91042
$ #97149
$ #97158
$ #97183
$ #97184
PROPOSAL: #91042 (comment)
The four deploy blockers above were closed by the revert in #97198 rather than fixed. They are linked here because they were caused by the reverted PR and would return on a re-land, so this PR has to address each one; the QA steps below cover them individually. #97155 is deliberately not listed — see the note in the explanation above.
Tests
Offline tests
QA Steps
Same as Tests, plus the regression checks from the reverted PR's blockers:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Kapture.2026-08-11.at.22.46.50.mp4
Android: mWeb Chrome
Kapture.2026-08-11.at.22.56.20.mp4
iOS: Native
Kapture.2026-08-11.at.21.53.09.mp4
iOS: mWeb Safari
Kapture.2026-08-11.at.22.36.16.mp4
MacOS: Chrome / Safari
Kapture.2026-08-11.at.21.39.06.mp4