-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
perf: Filter options in Request Money and Send Money #40235
perf: Filter options in Request Money and Send Money #40235
Conversation
} else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) { | ||
if (item.subtitle) { | ||
values.push(item.subtitle); | ||
} | ||
} else { | ||
values = values.concat(getParticipantsLoginsArray(item)); |
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.
Why do we move this to the last else block, with that for the policy expense chat it does not consider the participants but only subtitle for the filter values. Due to that policy expense chats are missing in the Recent section while filtering.
Screen.Recording.2024-04-17.at.23.16.23.mov
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.
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.
Ah, Ok. On prod I am not seeing those policy expense chat for the global search but it is shown for the request/send money participants filtration. I think that reverted PR tried manipulating the getSearchText
but here for request money participants search I don't think we are making a call to getSearchText
.
Screen.Recording.2024-04-18.at.23.40.09.mov
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.
Yeah the reverted PR was adding all the participants of the chat to the searchText
, so it became super inefficient, as some of the chats had thousands of participants.
getSearchText
is called when options are created (first opening of any search page - then they are cached), then, when searching, we are not calling it anymore because we are not recreating the options. Once getSearchText
is removed, the initial load of the list will speed up as well.
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.
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.
I re-reviewed the isSearchStringMatch
function, responsible for searching a match in a string generated by getSearchText
and found that for non-chatrooms, it also looks for matches in a participants list. Updated the filterOptions
to match this behavior so now it should also display e.g. workspaces when you search by user name. Thanks for spotting this! 👍
…equest-participants
@TMisiukiewicz If I understand correctly in this PR we are saving calls of Log with main branchScreen.Recording.2024-04-17.at.22.54.25.movLog with this PR changesScreen.Recording.2024-04-17.at.23.01.59.movProd recording where doesn't feel lagScreen.Recording.2024-04-19.at.00.04.48.mov |
@Pujan92 the problem is more visible on accounts with a lot of reports and personal details, especially on Android devices. We have an access to account with ~15k reports and on Android it took around 3 seconds to get results based on
It still calls |
Thanks @TMisiukiewicz for the explantion! |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeanh1.webmAndroid: mWeb Chromeanh2.webmiOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-23.at.14.40.12.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-23.at.14.28.15.mp4MacOS: Chrome / SafariScreen.Recording.2024-04-22.at.22.38.51.movMacOS: DesktopScreen.Recording.2024-04-23.at.15.01.38.mov |
src/libs/OptionsListUtils.ts
Outdated
function filterOptions(options: Options, searchInputValue: string, config?: FilterOptionsConfig): Options { | ||
const {sortByReportTypeInSearch = false, canInviteUser = true, betas = [], selectedOptions = [], excludeUnknownUsers = false, excludeLogins = []} = config ?? {}; | ||
const parsedPhoneNumber = PhoneNumber.parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue))); | ||
const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number?.e164 ?? '' : searchInputValue.toLowerCase(); |
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.
const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number?.e164 ?? '' : searchInputValue.toLowerCase(); | |
const searchValue = (parsedPhoneNumber.possible && parsedPhoneNumber.number?.e164) ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase(); |
Don't we need to use searchInputValue
when e164 is not available?
src/libs/OptionsListUtils.ts
Outdated
let {recentReports, personalDetails} = matchResults; | ||
|
||
if (sortByReportTypeInSearch) { | ||
recentReports = recentReports.concat(matchResults.personalDetails); |
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.
recentReports = recentReports.concat(matchResults.personalDetails); | |
recentReports = recentReports.concat(personalDetails); |
Let's use the destructured prop
} else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) { | ||
if (item.subtitle) { | ||
values.push(item.subtitle); | ||
} | ||
} else { | ||
values = values.concat(getParticipantsLoginsArray(item)); |
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.
src/libs/OptionsListUtils.ts
Outdated
searchValue && | ||
(noOptions || noOptionsMatchExactly) && | ||
!isCurrentUser({login: searchValue} as PersonalDetails) && | ||
selectedOptions.every((option) => 'login' in option && option.login !== searchValue) && | ||
((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue) && !Str.endsWith(searchValue, CONST.SMS.DOMAIN)) || | ||
(parsedPhoneNumber.possible && Str.isValidE164Phone(LoginUtils.getPhoneNumberWithoutSpecialChars(parsedPhoneNumber.number?.input ?? '')))) && | ||
!excludeLogins.find((optionToExclude) => optionToExclude === PhoneNumber.addSMSDomainIfPhoneNumber(searchValue).toLowerCase()) && | ||
(searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas)) && | ||
!excludeUnknownUsers |
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.
How about creating a function for these if conditions part as the same set of conditions is used twice in the file
385947d
to
4aaef6c
Compare
Hello @Pujan92, @TMisiukiewicz is out today, I just resolved the conflicts |
Thanks @rinej, I am seeing an issue of missed entries while searching for the Workspace. Screen.Recording.2024-06-13.at.11.16.44.mov |
Hello @Pujan92 Could you check if this behavior is similar on the main branch? I tested it on the main branch, and it also missed the workspace: search-main.mp4If this is unexpected (which it seems to be), we might have an issue on the main branch. |
@rinej For me the behavior is different on the main branch. It is kind of similar to I raised earlier here The difference is the generating entries for the recent reports. In our branch, while generating the data we are restricting the recent reports to a max 5, so on searching for any term it will just filter out from those 5 entries instead of considering all recent reports list. @TMisiukiewicz I believe it raised while resolving the conflict here, I think it needs to be 0 only. |
Thanks Pujan92 for noticing it! I added the suggested fix which reverts the change that was introduced after resolving the conflicts. Please let me know if that helps. If not I will dig deeper into the logic of that PR :) |
Thanks @rinej, Now it looks good to me. |
Thanks! Conflicts resolved 💪 |
@roryabraham Can you plz review again? |
✋ 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/roryabraham in version: 9.0.1-0 🚀
|
|
||
let userToInvite = null; | ||
if (canInviteUser) { | ||
if (recentReports.length === 0) { |
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.
@TMisiukiewicz @rinej This causes a regression #44200 where due to the default sortByReportTypeInSearch
is false which won't concat recentReports
& personalDetails
. With that even though we have entries in personalDetails
it will add that entry to userToInvite
as well because of this truthy condition.
I think we also need to check the personalDetails.length
here.
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.
opened a PR #44253 with a fix
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.
Thanks! I will check it.
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.1-19 🚀
|
options.reports, | ||
options.personalDetails, | ||
betas, | ||
debouncedSearchTerm, | ||
'', |
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.
This change caused a regression #48114, see the explanations #48114 (comment)
Details
This is the continuation of moving from
getSearchText
to filtering the lists. Here we are migrating the logic in Request Money and Send Money pagesFixed Issues
$ #37619
PROPOSAL:
Tests
Request money:
main
branchadmin@expensify.com
Send money:
main
branchadmin@expensify.com
Offline tests
n/a
QA Steps
admin@expensify.com
Send money:
admin@expensify.com
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
ANDROID.mov
Android: mWeb Chrome
ANDROID.WEB.mov
iOS: Native
IOS.mp4
iOS: mWeb Safari
IOS.WEB.mp4
MacOS: Chrome / Safari
WEB.mov
MacOS: Desktop
DESKTOP.mov