Skip to content

Update Emoji Picker Search to use Trie Search#14212

Merged
roryabraham merged 20 commits into
mainfrom
stites-EmojiPickerTrieSearch
Jan 17, 2023
Merged

Update Emoji Picker Search to use Trie Search#14212
roryabraham merged 20 commits into
mainfrom
stites-EmojiPickerTrieSearch

Conversation

@stitesExpensify

@stitesExpensify stitesExpensify commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

cc @roryabraham since you were heavily involved in the initial creation of the trie search

Details

This PR changes the emoji picker search to use the Trie Search which is much more efficient.

Unfortunately, the results are not in the same order as the initial list, so they don't show up how you would expect. To fix this, I gave each emoji an index so that regardless of which order the search returns them in, we can put them back in the right order.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/239099
PROPOSAL: N/A

Tests

Web and Desktop Only:

  1. Open the emoji picker
  2. Search for an emoji like "smile"
  3. Make sure you see the emojis that you expect, in the order that you expect.

WRONG

2023-01-10_15-52-17

RIGHT
2023-01-10_15-52-38

All Platforms:

  1. Type a colon code emoji into a chat e.g. :smile:
  2. Make sure that the emoji is shown correctly
  • Verify that no errors appear in the JS console

Offline tests

This is not reliant on any API requests, so offline tests are not needed

QA Steps

Web and Desktop Only:

  1. Open the emoji picker
  2. Search for an emoji like "smile"
  3. Make sure you see the emojis that you expect, in the order that you expect.

WRONG

2023-01-10_15-52-17

RIGHT
2023-01-10_15-52-38

All Platforms:

  1. Type a colon code emoji into a chat e.g. :smile:
  2. Make sure that the emoji is shown correctly
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
2023-01-10_16-17-19.mp4
Mobile Web - Chrome
2023-01-10_16-10-37.mp4
Mobile Web - Safari
2023-01-10_15-57-07.mp4
Desktop
2023-01-10_16-18-13.mp4
iOS
2023-01-10_15-59-13.mp4
Android
2023-01-10_16-09-32.mp4

@stitesExpensify stitesExpensify self-assigned this Jan 10, 2023
@stitesExpensify stitesExpensify marked this pull request as ready for review January 10, 2023 23:26
@stitesExpensify stitesExpensify requested a review from a team as a code owner January 10, 2023 23:26
@melvin-bot melvin-bot Bot requested review from Luke9389 and mananjadhav and removed request for a team January 10, 2023 23:27
@melvin-bot

melvin-bot Bot commented Jan 10, 2023

Copy link
Copy Markdown

@mananjadhav @Luke9389 One of you needs to 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]

Luke9389
Luke9389 previously approved these changes Jan 11, 2023

@Luke9389 Luke9389 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@mananjadhav

Copy link
Copy Markdown
Collaborator

Starting to review this.

@mananjadhav

mananjadhav commented Jan 11, 2023

Copy link
Copy Markdown
Collaborator

@stitesExpensify How is the search supposed to work?

We have this emoji:

{
        name: 'stuck_out_tongue_closed_eyes',
        code: '😝',
        keywords: [
            'prank',
            'stuck_out_tongue_closed_eyes',
            'eye',
            'face',
            'horrible',
            'taste',
            'tongue',
        ],
        index: 27,
    }

and if I search by the name, it doesn't show up in the result. It works by stuck.

image

@stitesExpensify

stitesExpensify commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Hm playing with this even more, I think our trie might not be building correctly, or the search method itself is broken.

Prod (with expected results)

2023-01-11_11-17-58.mp4

This branch (st doesn't bring up stuck_out_tongue but stu does)

2023-01-11_11-18-17.mp4

CC @roryabraham @Karim-30

@Karim-30

Copy link
Copy Markdown
Contributor

@stitesExpensify Ops, this regex missing _,+,- and digits, we should use this instead :[a-zA-Z0-9_+-]{1,40}$

Screen.Recording.2023-01-12.at.6.47.53.PM.mov

I'm working on the order issue now.

@Karim-30

Copy link
Copy Markdown
Contributor

For the order issue, we should use push instead of unshift

+++ b/src/libs/EmojiUtils.js
@@ -228,7 +228,7 @@ function suggestEmojis(text, limit = 5) {
                 if (matching.length === limit) {
                     return matching;
                 }
-                matching.unshift({code: nodes[j].metaData.code, name: nodes[j].name});
+                matching.push({code: nodes[j].metaData.code, name: nodes[j].name});
             }
             const suggestions = nodes[j].metaData.suggestions;
             for (let i = 0; i < suggestions.length; i++) {
@@ -236,7 +236,7 @@ function suggestEmojis(text, limit = 5) {
                     return matching;
                 }
                 if (!_.find(matching, obj => obj.name === suggestions[i].name)) {
-                    matching.unshift(suggestions[i]);
+                    matching.push(suggestions[i]);
                 }
             }
         }
+++ b/src/libs/Trie/index.js
@@ -102,7 +102,7 @@ class Trie {
             return matching;
         }
         if (node.isEndOfWord) {
-            matching.unshift({name: prefix, metaData: node.metaData});
+            matching.push({name: prefix, metaData: node.metaData});
         }
         const children = _.keys(node.children);
         for (let i = 0; i < children.length; i++) {
Screen.Recording.2023-01-12.at.7.47.23.PM.mov

@stitesExpensify

Copy link
Copy Markdown
Contributor Author

@Karim-30 I made those changes and did not get the expected results.

The regex change allowed searching stuck_ to find the correct emojis, but that did not fix the fact that searching st does not show 😛 as an option.

Additionally, using push did not properly order the emojis unfortunately.

@Karim-30

Copy link
Copy Markdown
Contributor

@stitesExpensify getAllMatchingWords missing the limit arg.

const nodes = emojisTrie.getAllMatchingWords(emojiData[0].toLowerCase().slice(1), limit);
webb-2023-01-13_09.11.04.mp4

Additionally, using push did not properly order the emojis unfortunately.

It work without this sorting

webb-2023-01-13_09.11.57.mp4

@mananjadhav

Copy link
Copy Markdown
Collaborator

@stitesExpensify Is it ready for the next round of review and testing?

@stitesExpensify

Copy link
Copy Markdown
Contributor Author

@mananjadhav not quite.

@Karim-30 I just pushed my changes, am I missing something? I'm still getting the wrong order.
2023-01-13_11-52-53

@stitesExpensify

Copy link
Copy Markdown
Contributor Author

In my understanding of the data structure, we won't always find emojis in the same order as they appear in the main list right? That would only happen if they were alphabetically listed?

@stitesExpensify

Copy link
Copy Markdown
Contributor Author

Swapping only one to push, and not the other fixed it for :smile: specifically. I chatted with @roryabraham about this and we decided it doesn't really matter if the search results come up in the normal order. Slack, for instance, sorts by most used first which is something we probably want to do in the future, but we can discuss that when we create the design doc for the potential replacement of trie search.

@stitesExpensify

Copy link
Copy Markdown
Contributor Author

Updated and ready for review!

Comment thread src/components/EmojiPicker/EmojiPickerMenu/index.js Outdated

@roryabraham roryabraham left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tests well

@Karim-30

Copy link
Copy Markdown
Contributor

@stitesExpensify The Trie searching order is FIFO for nodes.

If you insert Robert then Rock then Robertson into the Trie and search for Ro, because the b node in (Robert) inserted before the c node in (Rock), the b node and all of its children will appear before the c node and all of its children, the result will be :

  1. Robert.
  2. Robertson
  3. Rock.

@mananjadhav

Copy link
Copy Markdown
Collaborator

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web-emoji-trie-search.mov
Mobile Web - Chrome
mweb-chrome-emoji-trie-search.mov
Mobile Web - Safari
mweb-safari-emoji-trie-search.mov
Desktop
desktop-emoji-trie-search.mov
iOS
ios-emoji-trie-search.mov
Android
android-emoji-trie-search.mov

Thanks @stitesExpensify and @Karim-30 for the changes here. These test well now and faster too. I've completed the checklist and attached the screencasts for all platforms (while for search only Web and Desktop are relevant).

@Luke9389 @roryabraham All yours to merge.

@roryabraham roryabraham merged commit e6399c2 into main Jan 17, 2023
@roryabraham roryabraham deleted the stites-EmojiPickerTrieSearch branch January 17, 2023 01:47
@github-actions

Copy link
Copy Markdown
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 610.850 ms → 615.672 ms (+4.822 ms, +0.8%)
App start TTI 674.473 ms → 678.389 ms (+3.917 ms, +0.6%)
App start runJsBundle 185.097 ms → 188.688 ms (+3.591 ms, +1.9%)
App start regularAppStart 0.014 ms → 0.016 ms (+0.002 ms, +13.4%)
App start nativeLaunch 20.033 ms → 19.563 ms (-0.471 ms, -2.4%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 610.850 ms
Stdev: 23.414 ms (3.8%)
Runs: 571.1521000005305 571.8365070000291 574.3629160001874 579.7312419973314 580.8206389993429 584.6306969970465 584.656576000154 586.7873129993677 591.547770999372 596.2577320002019 597.7984619997442 601.7308759987354 605.9725350029767 606.5265709981322 610.6684169992805 616.4342040009797 616.9967860020697 617.0303139984608 617.8346759974957 620.8471269980073 623.3936769999564 624.4007569998503 624.4883619993925 626.2875580005348 626.8161629997194 628.0201830007136 630.5838220007718 632.0658369995654 643.8365070000291 647.7911380007863 651.9080410003662 653.9775400012732

Current
Mean: 615.672 ms
Stdev: 29.048 ms (4.7%)
Runs: 572.6645509973168 581.2941490001976 586.5736900009215 589.5323079973459 590.454549998045 590.9750569984317 592.443848002702 593.4807539992034 594.2597660012543 594.3044020012021 595.2216800004244 596.0098470002413 598.709188003093 598.7399500012398 600.0985920019448 603.4867359995842 607.1359050013125 610.2741289995611 614.0307619981468 614.6159269995987 615.0828050002456 625.4730640016496 628.9700930006802 630.1238609999418 631.162883002311 632.7990319989622 638.6859540008008 648.4474689997733 653.9206140004098 656.291300997138 659.186971001327 669.1289470009506 703.6021320000291
App start TTI Baseline
Mean: 674.473 ms
Stdev: 27.607 ms (4.1%)
Runs: 627.6662760004401 633.4746459983289 637.4507259987295 637.503176998347 645.1308449991047 646.9741999991238 648.116590000689 648.4539510011673 651.2298699989915 652.808928001672 655.9615139998496 661.7984270006418 661.9857209995389 666.4252140000463 667.3956580013037 672.2131970003247 679.0755940005183 680.5794040001929 681.4120669998229 687.2797509990633 689.9086959995329 695.3008010014892 697.9947110004723 700.2350570000708 702.2559299990535 703.4145249985158 709.2678570002317 711.099024001509 713.9693019986153 719.8048610016704 722.4615989997983

Current
Mean: 678.389 ms
Stdev: 28.984 ms (4.3%)
Runs: 631.1884539984167 631.770799998194 634.016465999186 639.3041259981692 641.7668769992888 642.0412329994142 647.4376569986343 660.1540010012686 663.7005769982934 664.20266199857 667.7747850008309 669.5670650005341 669.8740639984608 669.9823900014162 675.1422180011868 675.9733100011945 676.402366001159 677.3676399998367 682.0497029982507 682.7329440005124 685.9385050013661 686.2850859984756 693.9689100012183 694.3456990011036 696.0312509983778 714.1915430016816 715.5184440016747 717.2142979986966 717.6532960012555 719.5038370005786 730.9247330017388 734.4292559996247
App start runJsBundle Baseline
Mean: 185.097 ms
Stdev: 18.454 ms (10.0%)
Runs: 163 164 164 164 165 166 169 171 171 172 172 173 174 175 181 182 182 185 186 188 189 194 195 198 201 205 214 214 214 217 230

Current
Mean: 188.688 ms
Stdev: 22.851 ms (12.1%)
Runs: 157 160 161 164 165 165 165 166 167 167 170 174 175 178 183 185 193 193 193 194 200 201 201 201 204 205 213 219 226 228 232 233
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.3%)
Runs: 0.01245100051164627 0.01326499879360199 0.013305999338626862 0.01354999840259552 0.013590998947620392 0.013753999024629593 0.013794001191854477 0.013875000178813934 0.01387600228190422 0.013957001268863678 0.014038000255823135 0.014120001345872879 0.014200996607542038 0.014201000332832336 0.014281999319791794 0.014444999396800995 0.01464800164103508 0.014649000018835068 0.014649000018835068 0.014729999005794525 0.014812000095844269 0.014851998537778854 0.014892000705003738 0.014974001795053482 0.015014000236988068 0.015015002340078354 0.01505500078201294 0.015137001872062683 0.01550300046801567 0.015870001167058945 0.015990998595952988

Current
Mean: 0.016 ms
Stdev: 0.001 ms (4.4%)
Runs: 0.015177998691797256 0.01534000039100647 0.01534000039100647 0.015420999377965927 0.015422001481056213 0.015625 0.015625 0.015787001699209213 0.015828996896743774 0.01590999960899353 0.015991002321243286 0.016032002866268158 0.016071997582912445 0.016113001853227615 0.016154002398252487 0.016234997659921646 0.016235999763011932 0.016276001930236816 0.016479000449180603 0.016682997345924377 0.016885999590158463 0.016927000135183334 0.016967996954917908 0.01704999804496765 0.017170999199151993 0.017211999744176865 0.017333999276161194 0.017374999821186066 0.017537999898195267 0.017740998417139053
App start nativeLaunch Baseline
Mean: 20.033 ms
Stdev: 2.287 ms (11.4%)
Runs: 17 17 17 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 22 22 23 23 24 25 26

Current
Mean: 19.563 ms
Stdev: 1.560 ms (8.0%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 22 23

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.2.56-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants