Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
23 changes: 23 additions & 0 deletions src/libs/actions/IOU/MoneyRequestBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,29 @@ function buildOnyxDataForMoneyRequest(moneyRequestParams: BuildOnyxDataForMoneyR
},
},
);

// Reset the newly-created chat/IOU report metadata to non-optimistic on failure, mirroring the success path
// (isOptimisticReport: false). Without this the new chat stays isOptimisticReport: true, so `fetchReport` never
// calls `openReport` and the chat is stuck on an infinite loading skeleton. The failed expense's red-brick-road
// error stays visible; dismissing it then fully removes the orphaned chat and IOU report shells. See #93542.
if (isNewChatReport) {
onyxData.failureData?.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${chat.report?.reportID}`,
value: {
isOptimisticReport: false,

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 created-row dismiss deleting failed new chats

For a failed brand-new chat, this flips the chat metadata to non-optimistic, but the created/welcome row close handler still calls clearCreateChatError, which deletes the report only when REPORT_METADATA.isOptimisticReport is true; once this failure merge runs, that handler only clears errorFields.createChat. If the user closes the red error on the created row first, the optimistic chat and IOU shell are left behind, and the expense close path will no longer see report.errorFields.createChat to clean them up.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ef00b98. The created-row dismiss (ReportActionItemCreated) now detects a failed brand-new chat (errorFields.createChat + non-optimistic) and deletes the chat with the same cascade + navigate-away used by the failed-expense dismiss path, instead of falling through to clearCreateChatError (which only clears the error once isOptimisticReport is false). So closing the red error on the created row first no longer orphans the chat/IOU shells.

},
});
}
if (shouldCreateNewMoneyRequestReport) {
onyxData.failureData?.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${iou.report.reportID}`,
value: {
isOptimisticReport: false,
},
});
}
}

if (shouldGenerateTransactionThreadReport) {
Expand Down
2 changes: 2 additions & 0 deletions src/libs/actions/Report/DeleteReport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ function deleteReport(reportID: string | undefined, shouldDeleteChildReports = f
const onyxData: Record<string, null> = {
[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]: null,
[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`]: null,
// Also clear the report metadata (e.g. isOptimisticReport) so a deleted report leaves nothing orphaned behind.
[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`]: null,
};

// Delete linked transactions
Expand Down
15 changes: 15 additions & 0 deletions src/pages/inbox/report/ReportActionItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {canUseTouchScreen, hasHoverSupport} from '@libs/DeviceCapabilities';
import type {OnyxDataWithErrors} from '@libs/ErrorUtils';
import {getLatestErrorMessageField, isReceiptError} from '@libs/ErrorUtils';
import {isReportMessageAttachment} from '@libs/isReportMessageAttachment';
import Navigation from '@libs/Navigation/Navigation';
import type {PlatformStackNavigationProp} from '@libs/Navigation/PlatformStackNavigation/types';
import type {ReportsSplitNavigatorParamList} from '@libs/Navigation/types';
import Permissions from '@libs/Permissions';
Expand Down Expand Up @@ -65,6 +66,7 @@ import AttachmentModalContext from '@pages/media/AttachmentModalScreen/Attachmen
import {clearAllRelatedReportActionErrors} from '@userActions/ClearReportActionErrors';
import {hideEmojiPicker, isActive} from '@userActions/EmojiPickerAction';
import {expandURLPreview} from '@userActions/Report';
import deleteReport from '@userActions/Report/DeleteReport';
import {clearError} from '@userActions/Transaction';

import CONST from '@src/CONST';
Expand Down Expand Up @@ -259,6 +261,19 @@ function ReportActionItem({
cleanUpMoneyRequest(transactionIDToDismiss, action, reportID, transactionThreadReport, report, chatReport, undefined, originalReportID, true, iouPolicy);
return;
}

// When the expense created a brand-new chat that failed to be created on the server, the chat, IOU report and
// everything created solely for this request are orphaned optimistic shells. Dismissing the error should remove
// them entirely (see #93542) rather than only clearing the error, so navigate the user out of the now-deleted
// chat and delete it. We delete the parent chat report — deleteReport cascades from the chat down to the linked
// IOU report and the transaction thread(s). Deleting the current report here would only remove the IOU report
// (this error is dismissed from the expense report, so reportID is the IOU report ID) and leave the chat orphaned.
if (reportID && report?.errorFields?.createChat) {
const chatReportIDToDelete = report.chatReportID ?? reportID;
Navigation.goBack(undefined, {afterTransition: () => deleteReport(chatReportIDToDelete, true)});
Comment on lines +271 to +273

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Do not delete existing chats on IOU-report failures

When an expense is added to an existing chat that does not already have an IOU report, the failure builder still writes errorFields.createChat onto the newly-created IOU report while isNewChatReport is false. In that case report.chatReportID points to the existing server-backed chat, so dismissing the failed expense hits this branch and calls deleteReport(chatReportIDToDelete, true), removing the existing chat and its actions locally instead of only cleaning up the failed IOU shell. Gate this path on the parent chat also being a failed new optimistic chat, or delete only the new IOU report for existing-chat failures.

Useful? React with 👍 / 👎.

return;
}

if (action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && isReportActionLinked) {
navigation.setParams({reportActionID: ''});
}
Expand Down
19 changes: 16 additions & 3 deletions src/pages/inbox/report/ReportActionItemCreated.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';

import {hasDeferredWriteForReport} from '@libs/deferredLayoutWrite';
import Navigation from '@libs/Navigation/Navigation';
import {isChatReport, isCurrentUserInvoiceReceiver, isInvoiceRoom, navigateToDetailsPage, shouldDisableDetailPage as shouldDisableDetailPageReportUtils} from '@libs/ReportUtils';

import {clearCreateChatError} from '@userActions/Report';
import deleteReport from '@userActions/Report/DeleteReport';

import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -38,6 +40,7 @@ function ReportActionItemCreated({reportID, policyID}: ReportActionItemCreatedPr
const {translate} = useLocalize();
const {shouldUseNarrowLayout} = useResponsiveLayout();
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`);
const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID);
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
Expand Down Expand Up @@ -66,7 +69,17 @@ function ReportActionItemCreated({reportID, policyID}: ReportActionItemCreatedPr
pendingAction={report?.pendingFields?.addWorkspaceRoom ?? report?.pendingFields?.createChat}
errors={report?.errorFields?.addWorkspaceRoom ?? report?.errorFields?.createChat}
errorRowStyles={[styles.ml10, styles.mr2, styles.mb2]}
onClose={() =>
onClose={() => {
// A brand-new chat whose server creation failed (e.g. via a failed new-chat expense, see #93542) is
// reset to non-optimistic so it can load instead of spinning on an infinite skeleton. But that means
// clearCreateChatError would only clear the createChat error and leave the chat + its linked IOU report
// orphaned. Dismissing the created row on such a chat should remove it entirely — mirroring the
// failed-expense dismiss path — so delete the chat (deleteReport cascades to the IOU report and
// transaction thread(s)) and navigate the user out of the now-deleted chat.
if (report?.errorFields?.createChat && !reportMetadata?.isOptimisticReport) {
Navigation.goBack(undefined, {afterTransition: () => deleteReport(report.reportID, true)});
return;
}
clearCreateChatError(
report,
conciergeReportID,
Expand All @@ -77,8 +90,8 @@ function ReportActionItemCreated({reportID, policyID}: ReportActionItemCreatedPr
reportOwnerPersonalDetail,
currentUserPersonalDetail,
conciergePersonalDetail,
)
}
);
}}
>
<View style={[styles.pRelative]}>
{/* hasDeferredWriteForReport is non-reactive (reads a module-level Map, not tracked by React).
Expand Down
97 changes: 97 additions & 0 deletions tests/actions/IOU/BuildOnyxDataForMoneyRequestTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -475,4 +475,101 @@ describe('buildOnyxDataForMoneyRequest', () => {
expect((chatReportEntry?.value as Partial<Report>)?.iouReportID).toBeUndefined();
});
});

describe('failure handling for a brand-new chat', () => {
function buildNewChatParams(overrides?: Partial<BuildOnyxDataParams>): BuildOnyxDataParams {
return {
isNewChatReport: true,
shouldCreateNewMoneyRequestReport: true,
shouldGenerateTransactionThreadReport: true,
isASAPSubmitBetaEnabled: false,
currentUserAccountIDParam: CURRENT_USER_ACCOUNT_ID,
currentUserEmailParam: CURRENT_USER_EMAIL,
hasViolations: false,
quickAction: undefined,
isSelfDMSplit: false,
optimisticParams: buildBaseOptimisticParams(IOU_REPORT_ID),
delegateAccountID: undefined,
isTrackIntentUser: false,
...overrides,
};
}

function findFailureEntry(failureData: ReturnType<typeof buildOnyxDataForMoneyRequest>['failureData'], key: string) {
return failureData?.find((entry) => entry.key === key);
}

it('keeps the failed expense visible by MERGE-ing createChat errors onto the new chat and IOU reports instead of deleting them', () => {
const {failureData} = buildOnyxDataForMoneyRequest(buildNewChatParams());

const chatReportEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT}${CHAT_REPORT_ID}`);
const iouReportEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT}${IOU_REPORT_ID}`);

expect(chatReportEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect((chatReportEntry?.value as Partial<Report>)?.errorFields?.createChat).toBeDefined();
expect(iouReportEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect((iouReportEntry?.value as Partial<Report>)?.errorFields?.createChat).toBeDefined();
});

it('resets the new chat and IOU report metadata to non-optimistic so the chat is not stuck on an infinite loading skeleton', () => {
const {failureData} = buildOnyxDataForMoneyRequest(buildNewChatParams());

const chatMetadataEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT_METADATA}${CHAT_REPORT_ID}`);
const iouMetadataEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT_METADATA}${IOU_REPORT_ID}`);

expect(chatMetadataEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect((chatMetadataEntry?.value as {isOptimisticReport?: boolean})?.isOptimisticReport).toBe(false);
expect(iouMetadataEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect((iouMetadataEntry?.value as {isOptimisticReport?: boolean})?.isOptimisticReport).toBe(false);
});

it('does not SET any of the new-chat entities to null on failure (rollback happens on dismiss, not on failure)', () => {
const {failureData} = buildOnyxDataForMoneyRequest(buildNewChatParams());

const keys = [
`${ONYXKEYS.COLLECTION.REPORT}${CHAT_REPORT_ID}`,
`${ONYXKEYS.COLLECTION.REPORT}${IOU_REPORT_ID}`,
`${ONYXKEYS.COLLECTION.TRANSACTION}${TRANSACTION_ID}`,
`${ONYXKEYS.COLLECTION.REPORT}${THREAD_REPORT_ID}`,
];

for (const key of keys) {
const entry = findFailureEntry(failureData, key);
expect(entry).toBeDefined();
expect(entry?.value).not.toBeNull();
}
});

it('does not reset report metadata when the expense is added to an existing chat and IOU report', () => {
const {failureData} = buildOnyxDataForMoneyRequest(
buildNewChatParams({
isNewChatReport: false,
shouldCreateNewMoneyRequestReport: false,
}),
);

const chatMetadataEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT_METADATA}${CHAT_REPORT_ID}`);
const iouMetadataEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT_METADATA}${IOU_REPORT_ID}`);

expect(chatMetadataEntry).toBeUndefined();
expect(iouMetadataEntry).toBeUndefined();
});

it('keeps the standard error-merge behavior when the expense is added to an existing chat and IOU report (retryable)', () => {
const {failureData} = buildOnyxDataForMoneyRequest(
buildNewChatParams({
isNewChatReport: false,
shouldCreateNewMoneyRequestReport: false,
}),
);

const chatReportEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT}${CHAT_REPORT_ID}`);
const iouReportEntry = findFailureEntry(failureData, `${ONYXKEYS.COLLECTION.REPORT}${IOU_REPORT_ID}`);

expect(chatReportEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect(chatReportEntry?.value).not.toBeNull();
expect(iouReportEntry?.onyxMethod).toBe(Onyx.METHOD.MERGE);
expect(iouReportEntry?.value).not.toBeNull();
});
});
});
76 changes: 76 additions & 0 deletions tests/unit/DeleteReportTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,80 @@ describe('actions/Report/DeleteReport', () => {
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`)).toBeUndefined();
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${iouReportID}`)).toBeUndefined();
});

it('should clear the report metadata when a report is deleted', async () => {
// Given a report that has metadata (e.g. an optimistic report)
const reportID = '1';
const report = createRandomReport(Number(reportID), undefined);

await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, report);
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`, {isOptimisticReport: true});

// When we delete the report
deleteReport(reportID);

await waitForBatchedUpdates();

// Then the report metadata should be cleared so nothing (e.g. isOptimisticReport) is left orphaned behind
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`)).toBeUndefined();
});

it('should cascade from a chat report to its linked IOU report, transaction thread, and transaction', async () => {
// Given a brand-new chat report linked to an IOU report (via iouReportID and a report preview child action),
// an IOU report whose money-request action opens a transaction thread, and that transaction. This mirrors the
// failed new-chat expense rollback: dismissing deletes the parent chat, which must cascade to everything below.
const chatReportID = '1';
const iouReportID = '2';
const transactionThreadReportID = '3';
const transactionID = '123';

const chatReport = {
...createRandomReport(Number(chatReportID), undefined),
iouReportID,
};
const iouReport = createRandomReport(Number(iouReportID), undefined);
const transactionThreadReport = createRandomReport(Number(transactionThreadReportID), undefined);
const transaction = createRandomTransaction(Number(transactionID));

// Report preview action in the chat linking to the IOU report
const reportPreviewAction = {
...createRandomReportAction(100),
childReportID: iouReportID,
};
// Money-request action in the IOU report linking to the transaction thread and transaction
const iouAction: ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU> = {
...createRandomReportAction(200),
actionName: CONST.REPORT.ACTIONS.TYPE.IOU,
childReportID: transactionThreadReportID,
previousMessage: [],
message: [{text: '', html: 'iou', type: 'COMMENT'}],
originalMessage: {
type: CONST.IOU.REPORT_ACTION_TYPE.CREATE,
IOUTransactionID: transactionID,
IOUDetails: {
amount: transaction.amount,
currency: transaction.currency,
comment: '',
},
},
};

await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`, chatReport);
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${iouReportID}`, iouReport);
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`, transactionThreadReport);
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReportID}`, {[reportPreviewAction.reportActionID]: reportPreviewAction});
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReportID}`, {[iouAction.reportActionID]: iouAction});
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, transaction);

// When we delete the parent chat report with cascade
deleteReport(chatReportID, true);

await waitForBatchedUpdates();

// Then the chat, the linked IOU report, the transaction thread, and the transaction are all removed
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`)).toBeUndefined();
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${iouReportID}`)).toBeUndefined();
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`)).toBeUndefined();
expect(await getOnyxValue(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`)).toBeUndefined();
});
});
Loading