Refactored Emoji Trie build logic to improve perf#86422
Refactored Emoji Trie build logic to improve perf#86422roryabraham merged 9 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
joekaufmanexpensify and
rojiphil
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fe571d1cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
No product review needed |
|
Awaiting C+ checklist from @rojiphil |
| } | ||
| if (normalizedName !== localeName) { | ||
| const {node: normNode} = trie.getOrCreate(normalizedName); | ||
| normNode.metaData = node.metaData; |
There was a problem hiding this comment.
Could we shallow copy here so mutations don't affect the other. Same for addKeywordsToTrie
normNode.metaData = {...node.metaData, suggestions: [...(node.metaData.suggestions ?? [])]};
There was a problem hiding this comment.
hmmm I'm not sure I understand why we'd to a shallow copy. Avoiding such copies is how we achieve a performance gain here.
There was a problem hiding this comment.
I didn't explain it well, but this test shows the problem.
It's an edge case but these two emojis are normalised to the same word campana in Spanish:
campana🔔campaña⛺
There was a problem hiding this comment.
I can't see any other conflicting emoji's
|
Reviewing now... |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp86422-android-hybrid-001.mp4Android: mWeb Chrome86422-mweb-chrome-001.mp4iOS: HybridApp86422-ios-hybrid-001.mp4iOS: mWeb Safari86422-mweb-safari-001.mp4MacOS: Chrome / Safari86422-web-chrome-001.mp4 |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@roryabraham @Julesssss Changes LGTM. Works well too.
All yours now. Thanks.
|
@shubham1206agra it seems like at some point you might have force-pushed to this branch. Please try to avoid force-pushing when possible because it breaks the "changes since your last review" diff filter in GitHub |
|
@roryabraham I did not force push this branch at all. |
| @@ -56,8 +50,13 @@ function addKeywordsToTrie(trie: Trie<EmojiMetaData>, keywords: string[], item: | |||
| * @returns An array containing the name parts | |||
| */ | |||
| function getNameParts(name: string): string[] { | |||
There was a problem hiding this comment.
NAB, slightly cleaner alternative, removes one arithmetic operation per iteration:
function getNameParts(name: string): string[] {
const parts: string[] = [name];
let idx = 0;
while ((idx = name.indexOf('_', idx) + 1) !== 0) {
parts.push(name.slice(idx));
}
return parts;
}
|
weird, must be a github bug or something then |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.54-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes required. This PR refactors the internal Emoji Trie build logic ( |
7 participants
Explanation of Change
Fixed Issues
$ #86759
Tests
:operator and verify that the search result is correct.Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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
Screen.Recording.2026-04-06.at.7.42.50.PM.mov