refactor: PureReportActionItem, ReportActionItemEmojiReactions#88324
refactor: PureReportActionItem, ReportActionItemEmojiReactions#88324cristipaval merged 11 commits intoExpensify:mainfrom
Conversation
Swap withCurrentUserPersonalDetails HOC for the useCurrentUserPersonalDetails hook. No behavior change — both read the same Context — but removes the HOC wrapper and aligns the component with the current codebase convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the unused preferredLocale prop and read it from useLocalize() inside the component. The prop was never threaded through PureReportActionItem, so the component was silently falling back to CONST.LOCALES.DEFAULT — this change fixes that latent bug and removes one prop from the interface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tions
Replace the emojiReactions prop with a direct useOnyx subscription on
REPORT_ACTIONS_REACTIONS. Use the shared getEmptyObject<ReportActionReactions>()
helper for the stable empty default (matches BaseQuickEmojiReactions /
MiniQuickEmojiReactions).
Clean up the full prop chain:
- PureReportActionItem no longer takes/forwards emojiReactions; its memo loses
the deepEqual(emojiReactions) entry.
- The isOnSearch ? {} override is replaced with an !isOnSearch mount gate in the
parent, so on search we skip the mount (and the subscription) entirely.
- ReportActionItem, ReportActionsListItemRenderer, DuplicateTransactionItem and
ReportActionItemParentAction drop the prop and the now-unused useOnyx calls /
selectors that only existed to source it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LukasMod
force-pushed
the
perf-send-message-phase-12
branch
from
7d4b99e to
c5135ac
Compare
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.
|
LukasMod
changed the title
melvin-bot
Bot
requested review from
dmkt9 and
trjExpensify
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: ef1754d19a
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5d9de15a
ℹ️ 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".
LukasMod
force-pushed
the
perf-send-message-phase-12
branch
from
ad5d9de to
7dee6a8
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@LukasMod, could you please retest this on iOS Safari and mobile Chrome? I noticed that ios.safari.mp4 |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
…to perf-send-message-phase-12
|
@dmkt9 I added another video for iOS web and Android web. I don’t see this issue on my side. I’ve pulled the latest from main, could you check again? Also, these changes shouldn’t cause this behavior 🤔 |
@LukasMod yes. I have reviewed your changes; they do not alter any behavior and look good to me. However, the issue occurred when I tested on those platforms, so let me test it again. |
…to perf-send-message-phase-12
|
Just resolved newest conflicts |
|
I can now reproduce the issue on staging. It appears to be a BE bug where the I will complete the checklist shortly. |
|
On videos (ios mobile safari) I also checked on staging and it was fine. Also checking this right now on https://staging.new.expensify.com, refreshing, adding new reactions in workspaces, DMs works well (iOS mobile safari) |
Steps to reproduce: Navigate to "Troubleshoot > Clear cache and restart" in Safari on iOS, then return to a comment with at least one reaction. Note that reactions are no longer displayed. |
|
Can't repro on staging even with clear cache |
|
I can easily reproduce this on my end: |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.hybrid.mp4Android: mWeb Chromeandroid.chrome.mp4iOS: HybridAppios.native.mp4iOS: mWeb Safariios.safari.mp4MacOS: Chrome / Safarimac.safari.mp4 |
|
🚧 @cristipaval 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! 🧪🧪
|
5 participants
Explanation of Change
Emoji reactions decomposition
REPORT_ACTIONS_REACTIONS(viauseOnyx),useCurrentUserPersonalDetails, anduseLocalize. Drops theemojiReactions,currentUserAccountID,toggleEmojiReaction, andpreferredLocaleprops.emojiReactionsprop chain throughPureReportActionItem→ReportActionItem→ReportActionsListItemRenderer/DuplicateTransactionItem/ReportActionItemParentAction. Removes 3 upstreamuseOnyxcalls and onedeepEqualentry fromPureReportActionItem's memo comparator.emojiReactions={isOnSearch ? {} : …}"mute" with a!isOnSearchmount gate on the reactions block — matches how the sibling thread-replies block already handles Search.ReactionTooltipContent. Replaces the legacywithCurrentUserPersonalDetailsHOC withuseCurrentUserPersonalDetails, and moves emoji-name localization into the tooltip itself (it already usesuseLocalize), so callers can pass the raw English name and stop threadingpreferredLocale/currentUserPersonalDetailsthrough the list-item render path.Language fix
emoji-name tooltips stayed blank for non-EN users until they typed in the composer, the localized emoji table is lazy-loaded inside
getEmojiTrie, which is only called from the composer path (recently introduced).Fixed by firing
importEmojiLocale(preferredLocale)insideReportActionItemEmojiReactionswhen a reaction row first renders with reactions. Matches the composer's on-demand pattern (no boot cost, nouseEffect, no bundle regression), fires strictly on-demand (only when a user is actually in a chat with emoji reactions), and resolves well before the user has time to hover a bubble.Fixed Issues
$ #88377
PROPOSAL:
Tests
Test 1: Basic reactions flow:
Test 2: Search surface:
Test 3: Localization:
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
android.native.mov
Android: mWeb Chrome
android.web.mov
new video:
https://github.com/user-attachments/assets/4ee2ea1d-1429-4f78-bd83-81fe8c2cca1f
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.web.mov
new video:
https://github.com/user-attachments/assets/8d4af4c2-75a9-4e8f-b2ca-8301eac0e0d9
MacOS: Chrome / Safari
web.mov