fix: prevent Concierge redirect and LHN disappearance when vacation delegate splits expense#86869
Conversation
…elegate splits expense
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/pages/inbox/ReportScreen.tsx
Outdated
| // Track whether the current route is an own workspace chat (isOwnPolicyExpenseChat). | ||
| // Must be a ref set synchronously during render — by the time the navigation effects fire | ||
| // after a delegate split, the server SET has wiped report/prevReport in Onyx so we can't | ||
| // rely on live state or usePrevious. See issue #84248. |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The isCurrentRouteOwnWorkspaceChatRef ref-tracking pattern (declare ref, update synchronously during render based on report/reportID, preserve last-known value on wipe) is duplicated almost identically in three files:
src/pages/inbox/ReportScreen.tsx(lines 361-370)src/pages/inbox/ReportFetchHandler.tsx(lines 73-78, 107-113)src/hooks/useSidebarOrderedReports.tsx(lines 91-103)
The ReportScreen.tsx and ReportFetchHandler.tsx versions are nearly character-for-character identical. Extract this into a shared custom hook (e.g., useIsOwnWorkspaceChatRef(report, reportIDFromRoute)) that encapsulates the ref, the synchronous render-time update, and the wipe-preservation logic. Each consumer would then call the hook and read .current as needed.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Extracted into useIsOwnWorkspaceChatRef hook, duplication removed. Updated
|
|
||
| const isTransactionThreadView = isReportTransactionThread(report); | ||
|
|
||
| // Update the ref synchronously each render so the re-fetch effect below can read it |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This ref-tracking block is nearly identical to the one in ReportScreen.tsx (lines 361-370). Both check report?.reportID, compare against reportIDFromRoute, set isOwnPolicyExpenseChat, and intentionally preserve the last-known value when the report is wiped. Extract a shared hook such as useIsOwnWorkspaceChatRef(report, reportIDFromRoute) that returns the ref, so both files can consume it without duplicating the logic.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| useEffect(() => { | ||
| const wasJustWiped = !!prevReportID && prevReportID === reportIDFromRoute && !report?.reportID; | ||
| if (!wasJustWiped || !isCurrentRouteOwnWorkspaceChatRef.current) { | ||
| return; |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react-hooks/exhaustive-deps on line 183 lacks a justification comment explaining why certain dependencies (likely fetchReport) are omitted. Even though fetchReport is created via useEffectEvent (and is therefore stable), this should be documented for future readers.
Add a comment above or on the same line, for example:
// fetchReport is a stable useEffectEvent callback and does not need to be listed as a dependency.
// eslint-disable-next-line react-hooks/exhaustive-depsPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Added justification comment above the eslint-disable line. Updated
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc08632d60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/pages/inbox/ReportScreen.tsx
Outdated
| if (isCurrentRouteOwnWorkspaceChatRef.current) { | ||
| return; |
There was a problem hiding this comment.
This early return blocks all reportWasDeleted handling whenever the current route was previously marked isOwnPolicyExpenseChat, so if an own workspace chat is genuinely removed (for example, policy/workspace access is revoked), we now skip both parent-report and Concierge fallback navigation and can leave the user on a dead report route. Since this commit also excluded own policy chats from the other removal effect, there is no remaining redirect path for real deletions of that chat type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern, replaced the blunt early return with a 500ms deferred navigation. Temporary delegate wipe: re-fetch restores the report before the timer fires so reportWasDeleted resets to false and navigation is cancelled. Genuine deletion: report stays gone, timer fires, navigation proceeds correctly. Updated
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Checklist Notes
|
2 participants
Explanation of Change
When a vacation delegate (Account C) splits a submitted expense, the server sends an Onyx
SETupdate that temporarily wipes the workspace chat report on the submitter's (Account D) client. This triggered two navigation effects inReportScreen.tsxbefore the data recovered:isRemovalExpectedForReportType) incorrectly flagged the workspace chat as removed and initiated navigation awayreportWasDeletedeffect) navigated to Concierge, and caused an infinite loop if the screen remained in the navigation stack (pressing Back re-triggered the effect)On mobile (narrow layout), the LHN didn't recover because
useSidebarOrderedReportsskips force-inclusion of the focused report on narrow layouts. On web (wide layout), the sidebar force-includes it so it reappeared after ~2 seconds.Three targeted fixes across three files:
ReportScreen.tsxisCurrentRouteOwnWorkspaceChatRef— auseRefset synchronously during render that survives the Onyx wipe window (by the time navigation effects fire,prevReportis alreadyundefinedsousePreviouscannot be used)isPolicyExpenseChat(prevReport)→(isPolicyExpenseChat(prevReport) && !prevReport?.isOwnPolicyExpenseChat)to exclude own workspace chats from triggering the Concierge redirectNavigation.dismissModal()+Navigation.popToSidebar()stack cleanup before genuine Concierge redirects to prevent the infinite loopReportFetchHandler.tsxisCurrentRouteOwnWorkspaceChatRefref pattern to survive the Onyx wipe windowuseEffectthat detects the wipe (prevReportID === reportIDFromRoute && !report?.reportID) and callsfetchReport()to restore data — without this, the screen stays as a blank loading skeleton after the wipeuseSidebarOrderedReports.tsxisCurrentReportOwnWorkspaceChatRefref with synchronous render update|| isCurrentReportOwnWorkspaceChatRef.currentto the narrow-layout force-inclusion condition so the workspace chat stays visible in the LHN on mobile during the wipe windowKnown limitation: A ~2 second visual flicker on web LHN still occurs as an artifact of the Onyx
SETtiming. A full fix requires the server to switch fromSETtoMERGEfor this update (backend change outside scope of this PR).Fixed Issues
$ #84248
PROPOSAL: #84248 (comment)
Tests
Preconditions — set up 4 accounts:
Test steps:
Offline tests
QA Steps
Same as Tests section above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
WhatsApp.Video.2026-03-25.at.23.54.53.mp4
Android: mWeb Chrome
Video shows Account D on Android staying in workspace chat after Account C splits the expense. Same behavior verified on
Android mWeb Chrome shown in attached video above
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari