[NoQA] fix: display fallback x button to dismiss the transaction report in createChat error#72606
[NoQA] fix: display fallback x button to dismiss the transaction report in createChat error#72606mountiny merged 11 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if (hasOnlyReportCreationError) { | ||
| navigateToConciergeChatAndDeleteReport(report.reportID, true, true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I don't understand why we should only run this only on hasOnlyReportCreationError condition I expect to execute it anytime there is a createReport error whehter it is the only one or not. Moreover, the exact line this code is added might work in our case of fake data simulation but on a real world scenario the above conditions might run which would cause the function to early return before it reaches here.
There was a problem hiding this comment.
I don't understand why we should only run this only on hasOnlyReportCreationError condition
Moreover, the exact line this code is added might work in our case of fake data simulation but on a real world scenario the above conditions might run which would cause the function to early return before it reaches here.
As discussed in the Slack thread, we’ve agreed to treat the mocked data used in the PR author's test steps as the target case we want to address. Based on that case, the current implementation behaves correctly: it calls navigateToConciergeChatAndDeleteReport when createChat is the only error.
In scenarios where createChat isn’t the only error, this same logic is already invoked here:
There was a problem hiding this comment.
@mountiny what do you think about the hasOnlyReportCreationError condition? Do we have to be selective to this extent? I expect to navigateToConciergeChatAndDeleteReport whenever there is a createChat error not only when it is the only error.
There was a problem hiding this comment.
I agree with @FitseTLT this should not really be about a specific error in the errorFields - any time there is some error with the report the RBR should show
There was a problem hiding this comment.
To make it clear @mountiny we are already showing the error, but when the user click the x button it is implemented to only navigateToConciergeChatAndDeleteReport when the createChat error is the only error which I disagree.
There was a problem hiding this comment.
There was a problem hiding this comment.
No we display one dismiss button for all errors and similarly one onDismiss callback so my suggestion is whenever we have a createReport error we always need to delete the report and navigate to concierge. Why would the existence of other type of errors change it?
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-10-15.00-35-33.mp4Android: mWeb Chrome2025-10-14.23-58-47.mp4iOS: HybridAppiOS: mWeb Safari2025-10-14.23-57-43.mp4MacOS: Chrome / Safari2025-10-14.23-56-16.mp4MacOS: Desktop2025-10-14.23-56-52.mp4 |
mountiny
changed the title
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ...(transaction?.errorFields?.route ?? transaction?.errorFields?.waypoints ?? transaction?.errors), | ||
| ...parentReportAction?.errors, | ||
| }), | ||
| [transaction, parentReportAction], |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects (transaction, parentReportAction) as dependencies causes this useMemo to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking and reduces unnecessary hook executions.
Suggested fix:
const errorsWithoutReportCreation = useMemo(
() => ({
...(transaction?.errorFields?.route ?? transaction?.errorFields?.waypoints ?? transaction?.errors),
...parentReportAction?.errors,
}),
[
transaction?.errorFields?.route,
transaction?.errorFields?.waypoints,
transaction?.errors,
parentReportAction?.errors,
],
);
| }), | ||
| [transaction, parentReportAction], | ||
| ); | ||
| const reportCreationError = useMemo(() => (getCreationReportErrors(report) ? getMicroSecondOnyxErrorWithTranslationKey('report.genericCreateReportFailureMessage') : {}), [report]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire report object as a dependency causes this useMemo to re-execute whenever any property of report changes. The function only uses getCreationReportErrors(report), so you should depend on the specific properties that getCreationReportErrors actually checks.
Suggested fix:
If getCreationReportErrors checks specific properties like report.errorFields or similar, use those:
const reportCreationError = useMemo(
() => (getCreationReportErrors(report) ? getMicroSecondOnyxErrorWithTranslationKey('report.genericCreateReportFailureMessage') : {}),
[report?.errorFields, report?.errors], // Adjust based on what getCreationReportErrors actually uses
);There was a problem hiding this comment.
Not much we can do about this one as the method expects full report cc @kacper-mikolajczak
| report?.reportID, | ||
| iouReport, | ||
| chatIOUReport, | ||
| isChatIOUReportArchived, |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The dismissReceiptError callback depends on entire objects (transaction, chatReport, parentReportAction, iouReport, chatIOUReport), causing it to be recreated whenever any property of these objects changes. This can lead to unnecessary re-renders of child components that receive this callback as a prop.
Suggested fix:
Extract only the specific properties used within the callback:
const dismissReceiptError = useCallback(() => {
// ... function body
}, [
transaction?.transactionID,
transaction?.pendingAction,
chatReport?.reportID,
parentReportAction, // May need to break down further if only specific properties are used
linkedTransactionID,
report?.reportID,
iouReport, // May need to break down further
chatIOUReport, // May need to break down further
isChatIOUReportArchived,
errorsWithoutReportCreation,
reportCreationError,
isInNarrowPaneModal,
]);
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
It's kinda' weird to call this a "chat" isn't it? Why not "expense"?
@trjExpensify That message appears when the expense is created successfully, but the transaction thread report fails to be created due to an error. In that case, the message should say "Unexpected error when creating this chat" rather than referring to it as an "expense." |
|
Gotcha, but to a customer, seeing this error message on an expense is unexpected I think. Going back to this of @mountiny's in the OP. Did we cover point 4 in the PR? So 99% of the time now for customers that end up in this weird spot, we'll navigate them out of the mess to the
|
I believe we need to investigate that part a bit more still, but we dont have to hold on that. eitherway these situations are very rare and result of BE bug often.
That is fair, though this is generic error for any report, I think we should be ok here for now as mentioned above, it is actually a rare case that they should not run into (we are separately fixing the BE issues that causing these) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.46-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.46-3 🚀
|
5 participants
Explanation of Change
Problem
Previously, the logic for displaying the error message and "X" button in the transaction thread relied on this
errors:App/src/components/ReportActionItem/MoneyRequestReceiptView.tsx
Lines 190 to 193 in cf5453a
However, this
errorsis empty when there's exactly one error related to the transaction thread report creation. In such cases, theerrorsobject is empty, resulting in no error message or "X" button being shown to the user.Solution
This change ensures that if there is a single error during transaction thread report creation, we display a fallback error message — "Unexpected error creating this chat. Please try again later." — along with the "X" button. This gives users clear feedback about the failure. Clicking the "X" button will now clear the invalid transaction report data and navigate the user to the Concierge report, allowing them to recover from the error gracefully.
Fixed Issues
$ #72036 (comment)
PROPOSAL:
Tests
12345for example./r/12345and enter.Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2025-10-16.at.16.40.32.mov
Android: mWeb Chrome
Screen.Recording.2025-10-16.at.16.35.46.mov
iOS: Native
Screen.Recording.2025-10-16.at.16.31.39.mov
iOS: mWeb Safari
Screen.Recording.2025-10-16.at.16.33.14.mov
MacOS: Chrome / Safari
Screen.Recording.2025-10-14.at.23.57.40.mov
MacOS: Desktop
Screen.Recording.2025-10-16.at.16.30.23.mov