perf: improve native apps startup #44068
Conversation
…citly checking whether derivedCurrentReportID is not -1
hurali97
changed the title
melvin-bot
bot
requested review from
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 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] |
|
Would be good to fix e2e tests (to make a new release of the app) and then run e2e against this PR. I'm just curious whether we'll see decreased CPU/RAM usage and higher FPS and whether some of our metrics will go down. And if not, then we will need to investigate why e2e tests are not reporting huge improvements (maybe we will need to use a heavier account for them or generate more data for account that we currently use). |
|
I think that's a very good idea |
|
Tests are fixed 😎 But would be good to merge #43482 to capture even more metrics 😊 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
@mountiny can we get going with this PR? 👀 |
…y-App into hur/improve-native-app-startup
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
…y-App into hur/improve-native-app-startup
|
@hurali97 conflicts again 🙈 |
…y-App into hur/improve-native-app-startup
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.5-2 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.5-3 🚀
|
| .filter((accountID) => accountID !== currentUserAccountID); | ||
| return participantAccountIDs.length === 1 && participantAccountIDs[0] === CONST.ACCOUNT_ID.CONCIERGE && !isChatThread(report); | ||
| const participantAccountIDs = Object.keys(report?.participants ?? {}); | ||
| return participantAccountIDs.length === 1 && Number(participantAccountIDs[0]) === CONST.ACCOUNT_ID.CONCIERGE && !isChatThread(report); |
There was a problem hiding this comment.
Hi 👋 @hurali97 @hungvu193 , regarding this issue #45036, in this PR we have removed the accountID !== currentUserAccountID filter for participants, which caused the regression where isConciergeChatReport returns a wrong value. A contributor suggested a fix that restores the old code. Before moving forward with the PR, can you please explain why we removed the filter and if adding back the old code will cause any issues?"
There was a problem hiding this comment.
Hey @jayeshmangwani 👋
Oh snap, I removed this filter option as I thought it wasn't adding any value because later we are checking for conciergeID in return.
I tested the linked issue and I see that with these changes isConciergeReport return false, so it's better to bring back the filter only, we can ditch the map. Below is how the change will look like:
const participantAccountIDs = Object.keys(report?.participants ?? {}).filter((accountID) => Number(accountID) !== currentUserAccountID);With this, we get true from isConciergeReport BUT I still don't see any "Book A Call" option. This also happened on Prod where I can't see this option.
Prod:
Screen.Recording.2024-07-10.at.12.02.08.PM.mov
I spent some time and this happens because guideCalendarLink is always undefined in ProfilePage for Concierge chat. So the filter change is not relevant to this bug BUT we should make this change either way.
Code to show the option is below, where we need both isConcierge and guideCalendarLink to be true, but even after the filter change, we get guideCalendarLink as undefined:
{isConcierge && guideCalendarLink && (
<MenuItem
title={translate('videoChatButtonAndMenu.tooltip')}
icon={Expensicons.Phone}
isAnonymousAction={false}
onPress={SessionActions.checkIfActionIsAllowed(() => {
LinkActions.openExternalLink(guideCalendarLink);
})}
/>
)}
There was a problem hiding this comment.
Thanks for the insight, and yea even though we revert the isConciergeChatReport logic we still cannot see the Book a call option because of the guideCalendarLink undefined value
There was a problem hiding this comment.
I spent some time and this happens because guideCalendarLink is always undefined in ProfilePage for Concierge chat
@hurali97 The guideCalendarLink value may be undefined because we might be using an email that contains a '+'. With a normal email, we are able to get the guide link.
Context: https://expensify.slack.com/archives/C01GTK53T8Q/p1720464122617119?thread_ts=1720457309.992509&cid=C01GTK53T8Q
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
Details
The changes in the PR are part of improving the app startup which was profiled for the native Apps.
The main change is related to Report connections scattered through the util files. Each connection does heavy operations under the hood which are visible on a large data. One such example is `fastMerge` which calls `mergeObject` recursively. Now, 15k reports against 14 connections doesn’t scale. In the observations, this as a whole took around 18 seconds.
The solution can be to use only ONE report connection in a new util file say,
ReportConnectionand export a function from it calledgetAllReports. Then wherever we want to access updated reports, we can just use this function. Also, in some cases where we want to invoke a function in connection callback we can export it and invoke it from within theReportConnection.The other important change is for
useReportIDhook, which is called 3 times during the startup. This hook callsSidebarUtils.getOrderedReportIDfrom within which is a heavy function and is not supposed to be called unless required.We can fix the two additional calls by memoizing
policyMemberAccountIDs, which reduces the call by 1. Then we can add a condition to verify ifcurrentReportID===-1don't callgetOrderedReportIDbecause we use-1as default ID if nothing is passed and we do it in the initial iteration, so there seems no point in calling it again with the same ID. This as a whole saves us around 3 seconds.Some other changes are:
getLastVisibleActioncallsfastMergeeach time even if there are noactionsToMerge.isActionableJoinRequestPendingcan be simplified to avoid sorting and directly callArray.findBefore
android-before.mov
After
android-after.mov
Fixed Issues
$ #43746
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov