Add new comments to transaction thread for single transaction expense#68692
Add new comments to transaction thread for single transaction expense#68692
Conversation
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@jayeshmangwani ready for another round? 😓 |
|
@aldo-expensify thanks for the new PR. I will start reviewing it and will try to cover many cases around report deletion, navigation, and offline behavior, and compare them with the prod version to make sure we can reach prod with this PR this time. |
|
Thank you @jayeshmangwani 🙇 |
|
@aldo-expensify buttons don’t work after deleting the second expense while offline.
buttons-doent-works.movEdit: It also doesn’t work when we delete it while online. |
|
Thanks @jayeshmangwani , I'll investigate |
|
I'm suspecting the bug is coming from the Update: or maybe not a mutation, but a bug caused from reading data loading by Onyx.connect |
| function useOriginalReportID(reportID: string | undefined, reportAction: OnyxInputOrEntry<ReportAction>): string | undefined { | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID ?? CONST.DEFAULT_NUMBER_ID}`, {canBeMissing: true}); | ||
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID ?? CONST.DEFAULT_NUMBER_ID}`, {canBeMissing: true}); | ||
| const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.chatReportID}`, {canBeMissing: true}); | ||
|
|
||
| if (!reportID) { | ||
| return undefined; | ||
| } | ||
| const currentReportAction = reportAction?.reportActionID ? reportActions?.[reportAction.reportActionID] : undefined; | ||
|
|
||
| if (Object.keys(currentReportAction ?? {}).length === 0) { | ||
| const isThreadReportParentAction = reportAction?.childReportID?.toString() === reportID; | ||
| if (isThreadReportParentAction) { | ||
| return report?.parentReportID; | ||
| } | ||
| const transactionThreadReportID = getOneTransactionThreadReportID(report, chatReport, reportActions ?? ([] as ReportAction[])); | ||
| const retorno = transactionThreadReportID ?? reportID; | ||
| return retorno; | ||
| } | ||
| return reportID; | ||
| } |
There was a problem hiding this comment.
Added this hook analog to the function ReportUtils::getOriginalReportID which was reading data from Onyx.connect and causing this: #68692 (comment)
|
Thanks @jayeshmangwani for catching data, it should be fixed now. I added the reproduction step as a testing case in the PR description. |
|
Testing the PR now. |
|
Not a big issue, but running into a minor bug in the offline case. Steps to reproduce:
offline-bug.mov |
|
hmm, it is because when you do that offline, we find two IOU reportActions here:
App/src/libs/ReportActionsUtils.ts Line 1329 in 2948a86 so we consider it to not be a single-transaction expense report, and then we end up with the wrong Having said that, there must be some inconsistency because the report is rendered as a single-transaction expense report, so we may have two different logics that don't find the same result. |
|
@jayeshmangwani this should be working now, and while fixing that I also caught another bug I fixed: Test editing comment while offline:
Screen.Recording.2025-08-21.at.4.41.54.PM.mov |
|
While testing these steps #68692 (comment), I noticed that the second edited message is missing the |
Yes, I noticed too, but I was wondering if it could be a feature and not a bug. That message was edited before it was actually sent. |
|
Please continue with the review/testing so we don't run into more conflicts |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb-2.movMacOS: Desktopdesktop.mov |
|
🎯 @jayeshmangwani, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #69135. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ 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/nkuoch in version: 9.1.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.99-11 🚀
|



Explanation of Change
Re-implementation of this #68415, fixing bugs that it caused.
Changes: When a report has a single transactions and we merge reportActions for this report with the transaction thread, new comments should be added to the transaction thread instead of the main report. (more context: https://expensify.slack.com/archives/C090U774ZH7/p1754067856509739?thread_ts=1752726050.956949&cid=C090U774ZH7)
Parent components of
ReportFooterare already obtaining the transaction thread reportID for reports with single transactions, so I'm just passing it down here.Then,
ReportFooterwill prefer using the single transactiontransactionThreadReportIDinstead ofreport.reportIDif available.The previous PR broke whispers because the code resolving whispers was using the
reportIDwhich pointed to the expense report when the whisper was in an expense report with a single transaction, even if the whisper reportAction belonged to the transaction thread.This solves the issue by first trying to use
originalReportID.originalReportIDholds the correctreportIDcorresponding to theactionbeing handled byPureReportActionItem.Another issue handled here is auto-scrolling when a new comment is added:
notifyNewActionhandles scrolling to the bottom when we add a new comment. In single transaction expense reports we have to scroll to the bottom of the expense report, not the transaction thread.And, another consideration is that we handle reports created in the "send money" flow differently, for those we don't want to use the transaction thread by default for new comments because we don't combine reportActions there.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/526433
PROPOSAL:
Tests
You need to disable the beta
BETAS.NO_OPTIMISTIC_TRANSACTION_THREADSto test this properlyMessages are stored in the correct report:
Set category to "Travel"Attachments are stored in the correct report:
Screen.Recording.2025-08-18.at.12.44.09.PM.mov
Test editing comment while offline:
Screen.Recording.2025-08-21.at.4.41.54.PM.mov
Mention whisper:
Screen.Recording.2025-08-18.at.1.01.41.PM.mov
Mention whisper - deleting second expense:
Screen.Recording.2025-08-20.at.4.23.31.PM.mov
Suggested category actionable whisper:
dsgdgrewerfwefaegand don't select any categoryset the category to "Other"Screen.Recording.2025-08-18.at.1.05.12.PM.mov
Report mention whisper:
#1232534543634522 hello#asdasda214234 asdsasfScreen.Recording.2025-08-18.at.1.06.39.PM.mov
Pay user flow (plus auto-scrolling)
Screen.Recording.2025-08-15.at.12.50.26.PM.mov
Request money flow (plus auto-scrolling)
Screen.Recording.2025-08-15.at.12.51.35.PM.mov
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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))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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop