Improving logic to avoid multiple iterations#62928
Conversation
f16b77f to
6f27676
Compare
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppCleanShot.2025-06-02.at.09.51.10.mp4Android: mWeb ChromeCleanShot.2025-06-02.at.09.10.11.mp4iOS: HybridAppCleanShot.2025-06-02.at.09.02.17.mp4iOS: mWeb SafariCleanShot.2025-06-02.at.09.04.48.mp4MacOS: Chrome / SafariCleanShot.2025-06-02.at.08.43.57.mp4MacOS: DesktopCleanShot.2025-06-02.at.09.53.24.mp4 |
There was a problem hiding this comment.
There's a bit of redundancy here, I think it can be simplified to something like this:
for (const action of reportActions) {
if (allReportActionIDsSet.has(action.reportActionID)) {
// belongs to current report
if (!currentReportNewestAction) {
currentReportNewestAction = action;
}
currentReportOldestAction = action;
} else if (transactionThreadReport) {
// belongs to transaction thread (anything not in the set)
if (!transactionThreadNewestAction) {
transactionThreadNewestAction = action;
}
transactionThreadOldestAction = action;
}
}With the current logic there would be possible problem if transactionThreadReport is null or undefined - then targetReportID would be undefined too, and the else if (targetReportID === transactionThreadReport?.reportID block would always execute (cause undefined === undefined)
There was a problem hiding this comment.
Yeah, makes sense, thanks for pointing this out.
I'm thinking about a potential issue: what if transactionThreadReport has data and targetReportID is assigned transactionThreadReport.reportID? With your current code, I might miss the case where transactionThreadReport.reportID === reportID.
Maybe it would be better to add a check to the second if condition:
if (transactionThreadReport && targetReportID === transactionThreadReport?.reportID)
That way I can avoid the edge case you mentioned undefined === undefined
6f27676 to
3b05dec
Compare
|
@mjasikowski can you take a second look? |
|
✋ 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/mjasikowski in version: 9.1.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.59-7 🚀
|
Explanation of Change
Replace the current O(n²) Array.map + Array.includes pattern with O(1)
Eliminate redundant iterations by processing all report actions in a single pass.
Fixed Issues
$ #63026
PROPOSAL: -
Tests
Offline tests
QA Steps
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
improve_useLoadReportAction_android.mp4
Android: mWeb Chrome
improve_useLoadReportAction_androidWeb.mp4
iOS: Native
improve_useLoadReportAction_ios.mp4
iOS: mWeb Safari
improve_useLoadReportAction_iosSafari.mp4
MacOS: Chrome / Safari
improve_useLoadReportAction_chrome.mp4
improve_useLoadReportAction_safari.mp4
MacOS: Desktop
improve_useLoadReportAction_desktop.mp4