perf: Implement filtering in search page #37909
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gedu
reviewed
Mar 7, 2024
|
|
||
| // The regex below is used to remove dots only from the local part of the user email (local-part@domain) | ||
| // so that we can match emails that have dots without explicitly writing the dots (e.g: fistlast@domain will match first.last@domain) | ||
| const emailRegex = /\.(?=[^\s@]*@)/g; |
There was a problem hiding this comment.
probably you can initialize it outside of the function
There was a problem hiding this comment.
as it is not used anywhere else, I'd leave it here so it's clear where and why this regex belongs to
There was a problem hiding this comment.
rather than using a regex to replace dots in emails, can we just have filterArrayByMatch ignore dots, semicolons, dashes, etc... by default?
There was a problem hiding this comment.
hmm, I think it might negatively affect the performance, because we would have to call it for each value under a specified keys. With current approach, we can limit those checks only to the keys we need.
jbroma
reviewed
Mar 7, 2024
There was a problem hiding this comment.
would be nice to leave a comment that this is a slim version of what's available in match-sorter
gedu
reviewed
Mar 7, 2024
src/libs/filterArrayByMatch.ts
Outdated
| } else if (keyCopy.includes('.')) { | ||
| return getNestedValues<T>(keyCopy, item); | ||
| } else { | ||
| value = null; |
There was a problem hiding this comment.
probably you can do return []; here, instead of assign null and then check for null and return
TMisiukiewicz
force-pushed
the
perf/search-options-filtering
branch
from
47911af
to
790a191
Compare
melvin-bot
bot
requested review from
DylanDylann
and removed request for
a team
|
@roryabraham I've addressed all of your comments. I have reduced the filtering implementation by another ~50 LOC. Looking forward to get your opinion on current state of the PR |
|
I think there is some issue with reassure job, could anyone re-run it? |
roryabraham
requested changes
Apr 9, 2024
There was a problem hiding this comment.
As we iterate on this, more simplifications are becoming apparent:
- we aren't actually using different match thresholds at all
- we certainly aren't using different match thresholds on a per-key basis
- all "keys" are currently functions, but typed to be keys or functions that return either a string or array of string
Here's a diff that accomplishes the same thing we have here, more simply:
diff --git a/src/libs/OptionsListUtils.ts b/src/libs/OptionsListUtils.ts
index 33820da834..530e7e2b6a 100644
--- a/src/libs/OptionsListUtils.ts
+++ b/src/libs/OptionsListUtils.ts
@@ -2246,39 +2246,35 @@ function filterOptions(options: Options, searchInputValue: string): Options {
return keys;
};
const matchResults = searchTerms.reduceRight((items, term) => {
- const recentReports = filterArrayByMatch(items.recentReports, term, {
- keys: [
- (item) => {
- let keys: string[] = [];
- if (item.text) {
- keys.push(item.text);
- }
+ const recentReports = filterArrayByMatch(items.recentReports, term, (item) => {
+ let values: string[] = [];
+ if (item.text) {
+ values.push(item.text);
+ }
- if (item.login) {
- keys.push(item.login);
- keys.push(item.login.replace(emailRegex, ''));
- }
+ if (item.login) {
+ values.push(item.login);
+ values.push(item.login.replace(emailRegex, ''));
+ }
- if (item.isThread) {
- if (item.alternateText) {
- keys.push(item.alternateText);
- }
- keys = keys.concat(getParticipantsLoginsArray(item));
- } else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) {
- if (item.subtitle) {
- keys.push(item.subtitle);
- }
- } else {
- keys = keys.concat(getParticipantsLoginsArray(item));
- }
+ if (item.isThread) {
+ if (item.alternateText) {
+ values.push(item.alternateText);
+ }
+ values = values.concat(getParticipantsLoginsArray(item));
+ } else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) {
+ if (item.subtitle) {
+ values.push(item.subtitle);
+ }
+ } else {
+ values = values.concat(getParticipantsLoginsArray(item));
+ }
- return uniqFast(keys);
- },
- ],
- });
- const personalDetails = filterArrayByMatch(items.personalDetails, term, {
- keys: [(item) => item.participantsList?.[0]?.displayName ?? '', 'login', (item) => item.login?.replace(emailRegex, '') ?? ''],
+ return uniqFast(values);
});
+ const personalDetails = filterArrayByMatch(items.personalDetails, term, (item) =>
+ uniqFast([item.participantsList?.[0]?.displayName ?? '', item.login?.replace(emailRegex, '') ?? '']),
+ );
return {
recentReports: recentReports ?? [],
diff --git a/src/libs/filterArrayByMatch.ts b/src/libs/filterArrayByMatch.ts
index f481a08fe0..c87ad8fac1 100644
--- a/src/libs/filterArrayByMatch.ts
+++ b/src/libs/filterArrayByMatch.ts
@@ -18,59 +18,6 @@ const MATCH_RANK = {
type Ranking = ValueOf<typeof MATCH_RANK>;
-type RankingInfo = {
- rankedValue: string;
- rank: Ranking;
- keyIndex: number;
- keyThreshold?: Ranking;
-};
-
-type ValueGetterKey<T> = (item: T) => string | string[];
-
-type KeyAttributesOptions<T> = {
- key: string | ValueGetterKey<T>;
- threshold?: Ranking;
-};
-
-type KeyOption<T> = KeyAttributesOptions<T> | ValueGetterKey<T> | string;
-
-type Options<T = unknown> = {
- keys: ReadonlyArray<KeyOption<T>>;
- threshold?: Ranking;
-};
-type IndexableByString = Record<string, unknown>;
-
-/**
- * Gets value for key in item at arbitrarily nested keypath
- * @param item - the item
- * @param key - the potentially nested keypath or property callback
- * @returns an array containing the value(s) at the nested keypath
- */
-function getItemValues<T>(item: T, key: KeyOption<T>): string[] {
- if (!item) {
- return [];
- }
-
- const resolvedKey = typeof key === 'object' ? key.key : key;
- const value = typeof resolvedKey === 'function' ? resolvedKey(item) : (item as IndexableByString)[resolvedKey];
-
- if (!value) {
- return [];
- }
-
- return Array.isArray(value) ? value.map(String) : [String(value)];
-}
-
-/**
- * Gets all the values for the given keys in the given item and returns an array of those values
- * @param item - the item from which the values will be retrieved
- * @param keys - the keys to use to retrieve the values
- * @return objects with {itemValue}
- */
-function getAllValuesToRank<T>(item: T, keys: ReadonlyArray<KeyOption<T>>): string[] {
- return keys.flatMap((key) => getItemValues(item, key));
-}
-
/**
* Gives a rankings score based on how well the two strings match.
* @param testString - the string to test against
@@ -140,56 +87,31 @@ function getMatchRanking(testString: string, stringToRank: string): Ranking {
return ranking as Ranking;
}
-/**
- * Gets the highest ranking for value for the given item based on its values for the given keys
- * @param item - the item to rank
- * @param keys - the keys to get values from the item for the ranking
- * @param value - the value to rank against
- * @param options - options to control the ranking
- * @returns the highest ranking
- */
-function getHighestRanking<T>(item: T, keys: ReadonlyArray<KeyOption<T>>, value: string, options: Options<T>): RankingInfo {
- const valuesToRank = getAllValuesToRank(item, keys);
- return valuesToRank.reduce(
- (acc, itemValue, index) => {
- const ranking = acc;
- const newRank = getMatchRanking(itemValue, value);
- let newRankedValue = ranking.rankedValue;
-
- if (newRank > ranking.rank) {
- ranking.rank = newRank;
- ranking.keyIndex = index;
- ranking.keyThreshold = options.threshold;
- newRankedValue = itemValue;
- }
- return {rankedValue: newRankedValue, rank: ranking.rank, keyIndex: ranking.keyIndex, keyThreshold: ranking.keyThreshold};
- },
- {
- rankedValue: item as unknown as string,
- rank: MATCH_RANK.NO_MATCH as Ranking,
- keyIndex: -1,
- keyThreshold: options.threshold,
- },
- );
-}
-
/**
* Takes an array of items and a value and returns a new array with the items that match the given value
* @param items - the items to filter
* @param searchValue - the value to use for ranking
- * @param options - options to configure
+ * @param extractRankableValuesFromItem - an array of functions
* @returns the new filtered array
*/
-function filterArrayByMatch<T = string>(items: readonly T[], searchValue: string, options: Options<T>): T[] {
- const {keys, threshold = MATCH_RANK.MATCHES} = options;
+function filterArrayByMatch<T = string>(items: readonly T[], searchValue: string, extractRankableValuesFromItem: (item: T) => string[]): T[] {
+ const filteredItems = [];
+ for (const item of items) {
+ const valuesToRank = extractRankableValuesFromItem(item);
+ let itemRank: Ranking = MATCH_RANK.NO_MATCH;
+ for (const value of valuesToRank) {
+ const rank = getMatchRanking(value, searchValue);
+ if (rank > itemRank) {
+ itemRank = rank;
+ }
+ }
- return items
- .map((item) => ({...getHighestRanking(item, keys, searchValue, options), item}))
- .filter(({rank, keyThreshold = threshold}) => rank >= Math.max(keyThreshold, threshold + 1))
- .map(({item}) => item);
+ if (itemRank >= MATCH_RANK.MATCHES) {
+ filteredItems.push(item);
+ }
+ }
+ return filteredItems;
}
export default filterArrayByMatch;
export {MATCH_RANK};
-
-export type {Options, KeyAttributesOptions, KeyOption, RankingInfo, ValueGetterKey};
src/libs/filterArrayByMatch.ts
Outdated
| type IndexableByString = Record<string, unknown>; | ||
|
|
||
| /** | ||
| * Gets value for key in item at arbitrarily nested keypath |
There was a problem hiding this comment.
This comment description doesn't seem correct anymore?
roryabraham
approved these changes
Apr 10, 2024
|
✋ 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: 1.4.63-0 🚀
|
6 tasks
50 tasks
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
6 tasks
48 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Details
Currently when using search input, the option list is generated from scratch. With this change, list is created only once and it is filtered when value of search input change. Once we implement this in all search pages, we will be able to remove
searchTextproperty from options, which will have positive impact on execution time ofgetOptions(around 25% improvement). From our measurements, filtering is ~12x faster than creating an option list from scratch.Fixed Issues
$ #37619
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 so 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