Revert "Refactored toggleEmojiReaction to pure function" #73996
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| onPressOpenPicker={onPressOpenPicker} | ||
| onWillShowPicker={onWillShowPicker} | ||
| onSelectEmoji={onEmojiSelectedWithReactions} | ||
| onSelectEmoji={(emoji) => onEmojiSelected(emoji, emojiReactions)} |
There was a problem hiding this comment.
❌ PERF-4 (docs)
This inline arrow function creates a new function instance on every render because emojiReactions changes reference. When emojiReactions updates, this creates a new function, causing AddReactionBubble to re-render unnecessarily.
Suggested fix: Memoize the callback with useCallback:
const handleSelectEmoji = useCallback(
(emoji: Emoji) => {
onEmojiSelected(emoji, emojiReactions);
},
[onEmojiSelected, emojiReactions]
);
// Then use it in the JSX:
<AddReactionBubble
...
onSelectEmoji={handleSelectEmoji}
...
/>
| onModalHide: onEmojiPickerClosed, | ||
| onEmojiSelected: (_emojiCode, emojiObject, skinTone) => { | ||
| onEmojiSelectedWithReactions(emojiObject, skinTone); | ||
| onEmojiSelected: (_emojiCode, emojiObject) => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
This inline arrow function creates a new function instance on every render because it closes over emojiReactions, which changes reference frequently. The openEmojiPicker function itself will be recreated on every render, causing unnecessary re-renders when passed as a callback.
Suggested fix: Memoize the callback with useCallback:
const handleEmojiSelected = useCallback(
(_emojiCode: string, emojiObject: Emoji) => {
onEmojiSelected(emojiObject, emojiReactions);
},
[onEmojiSelected, emojiReactions]
);
const openEmojiPicker = useCallback(() => {
onPressOpenPicker();
showEmojiPicker({
onModalHide: onEmojiPickerClosed,
onEmojiSelected: handleEmojiSelected,
emojiPopoverAnchor: ref,
id: reportAction.reportActionID,
});
}, [onPressOpenPicker, onEmojiPickerClosed, handleEmojiSelected, reportAction.reportActionID]);
melvin-bot
bot
requested review from
allroundexperts
and removed request for
a team
|
@allroundexperts Please 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] |
|
I asked about the full story console error here https://expensify.slack.com/archives/C01GTK53T8Q/p1761955541563179 |
|
LGTM 👍 Thank you for your hard work! |
|
Does this need my review @cead22? |
|
@shubham1206agra Do you think you can fix this bug instead of reverting? |
|
@allroundexperts Not needed. Thanks. It's simple revert. |
|
@cead22 Let's not revert here before finding the actual cause. As I am not able to repro this either and this might be a BE issue for some accounts maybe. |
Explanation of Change
Fixed Issues
$ #73991 (comment)
PROPOSAL: N/A - this reverts this PR #73668
Tests
Offline tests
QA Steps
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop