Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/hooks/useSidebarOrderedReports.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,23 @@ function SidebarOrderedReportsContextProvider({
const derivedCurrentReportID = currentReportIDForTests ?? currentReportIDValue;
const prevDerivedCurrentReportID = usePrevious(derivedCurrentReportID);

// Track whether the currently-open report is an own workspace chat. We use a ref set
// synchronously during render because chatReports[currentReportID] gets wiped to undefined
// when a delegate splits an expense — the live value can't be trusted at effect time.
// The ref persists through the wipe so the LHN force-inclusion still fires. See issue #84248.
const isCurrentReportOwnWorkspaceChatRef = useRef(false);
const prevCurrentReportIDForRef = useRef<string | undefined>(undefined);
if (derivedCurrentReportID !== prevCurrentReportIDForRef.current) {
// Report changed — reset
isCurrentReportOwnWorkspaceChatRef.current = false;
prevCurrentReportIDForRef.current = derivedCurrentReportID;
}
const currentChatReportEntry = derivedCurrentReportID ? chatReports?.[`${ONYXKEYS.COLLECTION.REPORT}${derivedCurrentReportID}`] : undefined;
if (currentChatReportEntry?.reportID) {
isCurrentReportOwnWorkspaceChatRef.current = !!currentChatReportEntry.isOwnPolicyExpenseChat;
}
// else: report was wiped — intentionally keep the last known value in the ref

// we need to force reportsToDisplayInLHN to re-compute when we clear currentReportsToDisplay, but the way it currently works relies on not having currentReportsToDisplay as a memo dependency, so we just need something we can change to trigger it
// I don't like it either, but clearing the cache is only a hack for the debug modal and I will endeavor to make it better as I work to improve the cache correctness of the LHN more broadly
const [clearCacheDummyCounter, setClearCacheDummyCounter] = useState(0);
Expand Down Expand Up @@ -284,12 +301,13 @@ function SidebarOrderedReportsContextProvider({
// the current report is missing from the list, which should very rarely happen. In this
// case we re-generate the list a 2nd time with the current report included.

// We also execute the following logic if `shouldUseNarrowLayout` is false because this is
// requirement for web. Consider a case, where we have report with expenses and we click on
// any expense, a new LHN item is added in the list and is visible on web. But on mobile, we
// just navigate to the screen with expense details, so there seems no point to execute this logic on mobile.
// On narrow layouts (iOS/mobile) the force-inclusion block is normally skipped because
// navigating to a new report simply replaces the screen. However, when a vacation delegate
// splits an expense, a temporary server SET wipes the own workspace chat from chatReports.
// We bypass the narrow-layout skip for that specific case so the LHN stays correct.
// See issue #84248.
if (
(!shouldUseNarrowLayout || orderedReportIDs.length === 0) &&
(!shouldUseNarrowLayout || orderedReportIDs.length === 0 || isCurrentReportOwnWorkspaceChatRef.current) &&
derivedCurrentReportID &&
derivedCurrentReportID !== '-1' &&
orderedReportIDs.indexOf(derivedCurrentReportID) === -1
Expand Down
30 changes: 30 additions & 0 deletions src/pages/inbox/ReportFetchHandler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ function ReportFetchHandler() {
const hasCreatedLegacyThreadRef = useRef(false);
const didSubscribeToReportLeavingEvents = useRef(false);

// Track whether the current route is an own workspace chat synchronously during render.
// After a delegate split the server sends an Onyx SET that wipes the report; by the time a
// useEffect fires, report is undefined and we can no longer detect the chat type. See #84248.
const isCurrentRouteOwnWorkspaceChatRef = useRef(false);
// (Updated below after reportOnyx is read.)

const [reportOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}`);
const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportOnyx?.chatReportID}`);
const [reportMetadata = defaultReportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`);
Expand Down Expand Up @@ -98,6 +104,16 @@ function ReportFetchHandler() {

const isTransactionThreadView = isReportTransactionThread(report);

// Update the ref synchronously each render so the re-fetch effect below can read it
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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.

// even after the Onyx wipe has cleared `report`.
if (report?.reportID && report.reportID === reportIDFromRoute) {
isCurrentRouteOwnWorkspaceChatRef.current = !!report.isOwnPolicyExpenseChat;
} else if (!report?.reportID) {
// Report wiped — intentionally keep the last known value.
} else {
isCurrentRouteOwnWorkspaceChatRef.current = false;
}

const indexOfLinkedMessage = reportActionIDFromRoute ? reportActions.findIndex((obj) => String(obj.reportActionID) === String(reportActionIDFromRoute)) : -1;
const doesCreatedActionExists = !!reportActions?.findLast((action) => isCreatedAction(action));
const isLinkedMessageAvailable = indexOfLinkedMessage > -1;
Expand Down Expand Up @@ -156,6 +172,20 @@ function ReportFetchHandler() {

// Effect order below matches the original declaration order in ReportScreen.tsx.

// When a delegate splits an expense the server sends a temporary Onyx SET that wipes the
// workspace chat. The navigation guards in ReportScreen block any redirect, but the report
// stays blank until something re-fetches it. This effect detects the wipe and re-fetches.
// See issue #84248.
const prevReportID = usePrevious(report?.reportID);
useEffect(() => {
const wasJustWiped = !!prevReportID && prevReportID === reportIDFromRoute && !report?.reportID;
if (!wasJustWiped || !isCurrentRouteOwnWorkspaceChatRef.current) {
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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-deps

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added justification comment above the eslint-disable line. Updated

}
fetchReport();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [report?.reportID, prevReportID, reportIDFromRoute]);

useEffect(() => {
if (!transactionThreadReportID || !route?.params?.reportActionID || !isOneTransactionThread(childReport, report, linkedAction)) {
return;
Expand Down
39 changes: 36 additions & 3 deletions src/pages/inbox/ReportScreen.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {PortalHost} from '@gorhom/portal';
import {useIsFocused} from '@react-navigation/native';
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import type {ViewStyle} from 'react-native';
// We use Animated for all functionality related to wide RHP to make it easier
// to interact with react-navigation components (e.g., CardContainer, interpolator), which also use Animated.
Expand Down Expand Up @@ -354,6 +354,20 @@ function ReportScreen({route, navigation}: ReportScreenProps) {
});
const isDeletedTransactionThread = isReportTransactionThread(report) && (isParentActionDeleted || isParentActionMissingAfterLoad);
const [deleteTransactionNavigateBackUrl] = useOnyx(ONYXKEYS.NVP_DELETE_TRANSACTION_NAVIGATE_BACK_URL);

// 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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted into useIsOwnWorkspaceChatRef hook, duplication removed. Updated

const isCurrentRouteOwnWorkspaceChatRef = useRef(false);
if (report?.reportID && report.reportID === reportIDFromRoute) {
isCurrentRouteOwnWorkspaceChatRef.current = !!report.isOwnPolicyExpenseChat;
} else if (!report?.reportID) {
// Report wiped by Onyx SET — intentionally keep the last known value.
} else {
isCurrentRouteOwnWorkspaceChatRef.current = false;
}

// eslint-disable-next-line rulesdir/no-negated-variables
const shouldShowNotFoundLinkedAction =
(!isLinkedActionInaccessibleWhisper && isLinkedActionDeleted && isNavigatingToDeletedAction) ||
Expand Down Expand Up @@ -473,7 +487,10 @@ function ReportScreen({route, navigation}: ReportScreenProps) {
isEmpty(report) &&
(isMoneyRequest(prevReport) ||
isMoneyRequestReport(prevReport) ||
isPolicyExpenseChat(prevReport) ||
// Own policy expense chats (workspace chats) are excluded: a vacation delegate
// splitting an expense sends a temporary server SET that wipes the report, but
// the chat was never intentionally removed. See issue #84248.
(isPolicyExpenseChat(prevReport) && !prevReport?.isOwnPolicyExpenseChat) ||
isGroupChat(prevReport) ||
isAdminRoom(prevReport) ||
isAnnounceRoom(prevReport));
Expand Down Expand Up @@ -566,6 +583,22 @@ function ReportScreen({route, navigation}: ReportScreenProps) {
return;
}

// Do not navigate away for own workspace chats — a delegate split causes a temporary
// Onyx wipe that looks like a deletion but the chat was never actually removed.
// See issue #84248.
if (isCurrentRouteOwnWorkspaceChatRef.current) {
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep deletion redirect for truly removed own workspace chats

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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Clean up the navigation stack before redirecting to prevent an infinite loop where
// pressing back returns to the wiped report URL and re-triggers this effect.
Navigation.dismissModal();
if (Navigation.getTopmostReportId() === reportIDFromRoute) {
Navigation.isNavigationReady().then(() => {
Navigation.popToSidebar();
});
}

// Try to navigate to parent report if available
if (deletedReportParentID && !isMoneyRequestReportPendingDeletion(deletedReportParentID)) {
Navigation.isNavigationReady().then(() => {
Expand All @@ -578,7 +611,7 @@ function ReportScreen({route, navigation}: ReportScreenProps) {
Navigation.isNavigationReady().then(() => {
navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas);
});
}, [reportWasDeleted, isFocused, deletedReportParentID, conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas]);
}, [reportWasDeleted, isFocused, deletedReportParentID, conciergeReportID, introSelected, currentUserAccountID, isSelfTourViewed, betas, reportIDFromRoute]);

const actionListValue = useActionListContextValue();

Expand Down
Loading