Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 13 commits
977e122
d66c2e8
ce44bde
047f320
ffa8eb5
ccf5350
f28e5e8
2b4c5c5
dd42f18
f558989
8822800
477260e
f8834dc
bef018a
4aaef6c
a8b581a
0bf553b
e873c96
17b178e
e862eb9
05f60cb
38147dc
767db86
c17880b
6a669c6
d3fb666
4e3ec39
79a7140
80e5b83
3c822e5
e3b1afa
49635ed
49a421f
712a381
146fbc9
4bb2ece
42e2aa9
9504d74
d894a22
07e7d50
ec4c747
d421d17
60c7149
d246f11
ad4a170
fb19c55
7716743
1d4cabc
c55c40f
cb671ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
NAB, this comment can be moved inside the
createOptimisticPersonalDetailOption
where we are generating optimistic account ID or we can remove this 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.
Don't we need to use
searchInputValue
when e164 is not available?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.
Recently this PR caused a big performance regression: #38887 and it was reverted in #40019. Since the filtering was merged somewhere close before reverting, I adjusted function to match the correct behavior. Seems like it works the same way on production
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 togetSearchText
.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. OncegetSearchText
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.
That reverted PR is in production, doubt why it still shows the policy expense chats in the search filter for request/send money flow.
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 bygetSearchText
and found that for non-chatrooms, it also looks for matches in a participants list. Updated thefilterOptions
to match this behavior so now it should also display e.g. workspaces when you search by user name. Thanks for spotting this! 👍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.
Let's use the destructured prop
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