Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0a1308e12
ℹ️ 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".
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.
|
| importEmojiLocale(normalizedLocale).then(() => { | ||
| endSpan(CONST.TELEMETRY.SPAN_LOCALE.EMOJI_IMPORT); |
There was a problem hiding this comment.
@elirangoshen Why are we importing emoji files when we are not even using them immediately?
There was a problem hiding this comment.
Lets defer this import if possible
src/libs/EmojiTrie.ts
Outdated
| } | ||
| // Don't build and cache the trie if locale emoji data hasn't been loaded yet, | ||
| // otherwise we'd permanently cache a trie with only English fallback names. | ||
| if (locale !== CONST.LOCALES.DEFAULT && !localeEmojis[locale]) { |
There was a problem hiding this comment.
@elirangoshen Maybe we should import the emoji file if this condition is met.
| startSpan(CONST.TELEMETRY.SPAN_LOCALE.EMOJI_TRIE_BUILD, { | ||
| name: CONST.TELEMETRY.SPAN_LOCALE.EMOJI_TRIE_BUILD, | ||
| op: CONST.TELEMETRY.SPAN_LOCALE.EMOJI_TRIE_BUILD, | ||
| }); |
There was a problem hiding this comment.
Just thinking if this can be parented dynamically or must be orphaned. I think no parent makes sense 👍
|
No product review needed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 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.55-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a performance-only change that defers the internal emoji Trie data structure build from app startup to first use. There are no user-facing behavior changes, no new or removed features, and no changes to UI text, settings, or labels — nothing that would need to be reflected in |
|
Deploy Blocker #87453 was identified to be related to this PR. |
|
This PR failing because of the issue #87453 |
Explanation of Change
Defers the emoji Trie build from app startup to first use. Previously,
LocaleContextProvidereagerly calledbuildEmojisTrieright after importing the emoji locale data, adding to startup time.Now, the trie is built lazily via
getEmojiTrie()on the first call toreplaceEmojisorsuggestEmojisinEmojiUtils—i.e., when the user actually interacts with emoji shortcodes.Fixed Issues
Tests
:smileto trigger emoji suggestions:thumbsup:and verify it gets replaced with the emojiOffline Tests
N/A — This change only affects when the emoji trie is built (deferred from startup to first use). No network dependency.
QA Steps
[No QA] — Performance-only change that defers emoji trie construction. No user-facing behavior change.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari