refactor: buildOptimisticChildReport to handle optimistic report creation correctly #79845
Conversation
samranahm
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }); | ||
|
|
||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${childReportID ?? newChat.reportID}`, newChat); | ||
| if (!childReportID) { |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs) The condition \!childReportID does not properly handle the scenario where childReportID is provided but the report does not exist in Onyx yet. Before: Callers checked \!existingChildReport to determine if openReport() was needed After: Checks \!childReportID which is a different condition Breaking scenario: When childReportID is provided but report is not in Onyx, the code will call Onyx.merge() instead of openReport(), leading to missing data and potential failures. Fix: Pass existingChildReport parameter or check report existence inside the function: typescript const existingReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${childReportID ?? newChat.reportID}`]; if (\!existingReport) { const participantLogins = PersonalDetailsUtils.getLoginsByAccountIDs(Object.keys(newChat.participants ?? {}).map(Number)); openReport(newChat.reportID, "", participantLogins, newChat, parentReportAction.reportActionID, undefined, undefined, undefined, true); } else { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${childReportID}`, newChat); }
There was a problem hiding this comment.
Pull request overview
This PR refactors the buildOptimisticChildReport function to consolidate duplicate code by moving the openReport call logic from navigateToAndOpenChildReport and explain functions into buildOptimisticChildReport itself.
Changes:
- Modified
buildOptimisticChildReportto conditionally callopenReportorOnyx.mergebased on whetherchildReportIDis provided - Removed duplicate
openReportlogic fromnavigateToAndOpenChildReportfunction - Removed duplicate
openReportlogic fromexplainfunction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!childReportID) { | ||
| const participantLogins = PersonalDetailsUtils.getLoginsByAccountIDs(Object.keys(newChat.participants ?? {}).map(Number)); | ||
| openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID, undefined, undefined, undefined, true); | ||
| } else { | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${childReportID}`, newChat); | ||
| } |
There was a problem hiding this comment.
This conditional logic introduces a behavioral regression. When a childReportID exists but the child report hasn't been loaded into Onyx yet (e.g., when clicking on a thread that was created previously but not yet opened), the original code would call openReport to fetch the thread from the server. The new code only calls Onyx.merge, which creates an optimistic report locally without fetching the actual thread data from the server. This means existing threads won't load their messages properly. The condition should check whether existingChildReport is falsy (not whether childReportID is falsy) to match the original behavior and ensure threads are fetched from the server when they already exist.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@marcaaron You left a couple of comments on the previous PR, please let me know if we can address them in this one. |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@samranahm Thanks for preparing this PR but I think this can be closed as the original PR has been closed. |
|
Agree, we can close this one. I will add both these tests in follow-up PR. |
3 participants
Explanation of Change
Fixed Issues
$ #79830
$ #79831
$ #78464
PROPOSAL: #78464 (comment)
Tests
Precondition:
[Member]
[Approver]
Offline tests
QA Steps
Same as test
// 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
macOS.Chrome.one.mp4
macOS.Chrome.two.mp4