-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Handle the preExistingReportID for transaction threads, message threads, and single expense reports #75101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle the preExistingReportID for transaction threads, message threads, and single expense reports #75101
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
… to fetch report data
|
Asking about the test failure here: |
@s77rt - could you please check this Auth side fix for clearing the optimistic IOU report action: https://github.com/Expensify/Auth/pull/19610? What do you think about this BE solution? Can we rely on it instead of the logic added in the App PR #79356? |
|
@rayane-d I feel like the |
I've added these test cases. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9021057780
ℹ️ 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".
| Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${preexistingReportID}`, { | ||
| ...report, | ||
| reportID: preexistingReportID, | ||
| preexistingReportID: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve existing thread report fields when replacing optimistic
When a preexisting thread is detected, this branch replaces the existing report with the optimistic report object via Onyx.set. Optimistic thread reports are created with only a minimal field set (see setOptimisticTransactionThread in src/libs/actions/Report/index.ts), so overwriting here drops existing fields like participants, reportName, and other metadata on the preexisting thread. In the preexisting-thread scenario this can leave the UI missing avatars/header info after the switch. Consider merging into the existing report or preserving its fields (similar to the DM path) instead of replacing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rayane-d What do you think of this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny - I don't agree with the bot comment. When the backend returns preexistingReportID, we usually don't have Onyx data for the preexisting report yet, so there's nothing to merge into. In this case, we should create the report using Onyx.set with the optimistic report data. After that, OpenReport will be called again with the preexisting report ID, and the backend will return the full report data to update it.
|
@rayane-d I was not able to reproduce using last repro steps but since the logic to clear the optimistic iOU action is there then we should be fine. |
|
@s77rt Thanks for confirming! |
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex review
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Code Review ObservationsThanks for the detailed PR! I've reviewed the changes and have a few observations: 1. Potential Race Condition in SilentCommentUpdaterFile: The if (isTransitioningToPreExistingReport.current && reportID !== prevReportId) {
onTransitionToPreExistingReportComplete();
return;
}This condition only triggers when the
Suggestion: Consider adding a test case that verifies the timing of 2. Explicit Null Check for parentReportActionIDFile: The current code around line 108 checks const parentReportAction = parentReportID ? allReportActions?.[parentReportID]?.[parentReportActionID] : null;For improved clarity and safety, consider making both checks explicit: const parentReportAction = (parentReportID && parentReportActionID)
? allReportActions?.[parentReportID]?.[parentReportActionID]
: null;This makes the intent clearer and ensures both IDs are present before attempting the lookup. 3. Verify isOptimisticAction Flag Cleanup in Success HandlersFile: The new code adds ...optimisticIOUAction,
childReportID: reportID,
isOptimisticAction: true,I see that in other parts of the codebase, success handlers clear this flag: successReportActions[actionKey] = {pendingAction: null, isOptimisticAction: null};Question: Could you verify that the success handler for this specific action path (when OpenReport succeeds after creating the transaction thread) also clears the Overall the PR looks solid with great test coverage! These are just some observations for consideration. |
|
Some nice opus observations @rayane-d |
|
✋ 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/grgia in version: 9.3.11-19 🚀
|
Explanation of Change
Auth PR:
Fixed Issues
$ #74846
PROPOSAL:
Tests
Test 1 — Optimistically created transaction thread with a one-expense parent report
Goal:
Verify that when the parent IOU report contains only one transaction, any optimistically created transaction thread is discarded when the transaction thread already exists and the user is redirected back to the parent one-expense report.
Steps
Create an account and a workspace.
Open the workspace chat.
Create an expense.
Enable debug mode.
Open the expense.
Tap the header → open the debug menu.
Tap Actions.
Open the IOU action.
Copy the
reportActionID.Copy the
reportID.Copy the
childReportID(the existing transaction thread).Go offline.
Run the following snippet after replacing the IDs:
Open the workspace chat.
Open the expense report.
Open the debug menu again (header → debug).
Tap Actions.
Open the IOU action.
Copy the newly created optimistic
childReportID.Replace the URL's report ID with the ID from step 21.
Confirm that a new transaction thread opens (may be greyed out) that has a report ID different than the report ID from step 11.
Type some text in the composer without sending it (draft message).
Run the following snippet to simulate OpenReport response:
Confirm that you are automatically redirected to the parent one-expense report (reportID from step 10).
Confirm the draft message from step 24 is moved to the parent one-expense report.
Confirm that the Onyx data of the optimistic report from step 21 is deleted
Confirm that the
childReportIDof the IOU report action points to the correct report ID (reportID from step 10)Go back online
Confirm that OpenReport is called again for the parent report (the one expense report) (reportID from step 10).
Screen.Recording.2025-12-17.at.1.18.14.PM.mov
ios.mov
Test 2 — Optimistically created transaction thread in a multi-expense parent report, with existing transaction thread
Goal:
Verify that when the IOU report has multiple expenses, and the App try to create an optimistic transaction thread for an existing transaction thread, the user is redirected to the correct existing transaction thread, and the draft message is preserved.
Steps
Create an account and a workspace.
Open the workspace chat.
Create two expenses.
Enable debug mode.
Open the IOU report.
Copy the
reportIDOpen each of the two expenses so that a transaction thread report is created
Open the IOU report.
Tap the header → open debug menu.
Tap Actions.
Open one of the IOU actions (choose one expense).
Copy the
reportActionID.Copy the
childReportID(existing transaction thread).Go offline.
Open the browser console.
Run:
Open the workspace chat.
Open the IOU report.
Open the expense selected in step 11.
Confirm that a new optimistic transaction thread (may be greyed out) is created that has a report ID different than the report ID from step 13.
Copy the optimistic transaction thread's report ID.
Type some text in the composer without sending it (draft message).
Run the following snippet to simulate OpenReport response:
Confirm automatic redirection to the correct transaction thread (ID = step 13).
Confirm the draft message from step 24 appears in the correct thread (ID = step 13).
Confirm that the Onyx data of the optimistic report (report ID from step 23) is deleted
Confirm that the
childReportIDof the IOU report action points to the correct report ID (reportID from step 13)Go back online
Confirm that OpenReport is called again for
preExistingReportID(report ID from step 13).Screen.Recording.2025-11-20.at.12.32.46.AM.mov
Screen.Recording.2025-12-17.at.12.08.28.PM.mov
Test 3 — Optimistically created thread under a comment, but an existing thread exists
Goal:
Verify that when the App tries to create an optimistic thread report under a comment, and a thread report already exists, the App redirects to the correct thread and transfers the draft message.
Steps
Create an account and a workspace.
Open the workspace chat.
Send a message.
Right-click the message → Reply in thread.
Send a message in the thread.
Enable debug mode.
Return to the workspace chat.
Copy the workspace chat reportID
Tap header → debug menu.
Tap Actions.
Open the
ADDCOMMENTaction for the message from step 3.Copy the
reportActionID.Copy the
childReportID(existing thread).Go offline.
Run:
Open the workspace chat.
Click the original message (step 3) to open the thread.
Confirm that a new optimistic transaction thread (may be greyed out) is created that has a report ID different than the report ID from step 13.
Copy the optimistic thread's report ID.
Type a draft message in the composer without sending it.
Run the following snippet to simulate OpenReport response:
Confirm automatic redirection to the correct thread (reportID form step 13).
Confirm the draft message from step 20 is restored in the correct thread (reportID from step 13).
Confirm that the Onyx data of the optimistic report from step 19 is deleted
Confirm that the
childReportIDof the IOU report action from step 11 points to the correct report ID (reportID from step 13)Go back online
Confirm that OpenReport is called again for
preExistingReportID(reportID from step 13).Untitled1.mov
Screen.Recording.2025-12-17.at.11.58.04.AM.mov
Offline tests
QA Steps
Same as the tests, but QA should only test on the web platform. QA will only be able to properly test this on mobile platforms after the Auth PR has been merged and deployed.
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
Tested with backend changes:
Screen.Recording.2025-11-14.at.10.01.42.PM.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop