fix: Global create emoji is overlapping the message with markdown#77790
Merged
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 removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, [tnode.attributes, styles, styleProp]); | ||
| }; | ||
|
|
||
| const style = getStyle(); |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
Removing useMemo here causes the style array to be recreated on every render, triggering unnecessary re-renders of EmojiWithTooltip even when the actual style values haven't changed.
Fix: Restore the useMemo hook to preserve referential stability:
const style = useMemo(() => {
if ('islarge' in tnode.attributes) {
return [styleProp as TextStyle, styles.onlyEmojisText];
}
if ('ismedium' in tnode.attributes) {
return [styleProp as TextStyle, styles.emojisWithTextFontSize, styles.verticalAlignTopText];
}
return null;
}, [tnode.attributes, styles, styleProp]);
| @@ -62,7 +62,7 @@ function TextCommentFragment({fragment, styleAsDeleted, reportActionID, styleAsM | |||
|
|
|||
| const message = isEmpty(iouMessage) ? text : iouMessage; | |||
|
|
|||
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
Removing useMemo causes processedTextArray to be recalculated on every render, even when message hasn't changed. This creates unnecessary computation overhead.
Fix: Restore the useMemo hook:
const processedTextArray = useMemo(() => splitTextWithEmojis(message), [message]);
| // on other device, only render it as text if the only difference is <br /> tag | ||
| const containsOnlyEmojis = containsOnlyEmojisUtil(text ?? ''); | ||
| const containsOnlyCustomEmoji = useMemo(() => containsOnlyCustomEmojiUtil(text), [text]); | ||
| const containsOnlyCustomEmoji = containsOnlyCustomEmojiUtil(text); |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
Removing useMemo causes containsOnlyCustomEmoji to be recalculated on every render, even when text hasn't changed. The containsOnlyCustomEmojiUtil function performs string processing that should be memoized.
Fix: Restore the useMemo hook:
const containsOnlyCustomEmoji = useMemo(() => containsOnlyCustomEmojiUtil(text), [text]);
| } | ||
|
|
||
| export default memo(TextCommentFragment); | ||
| export default TextCommentFragment; |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
Removing memo wrapper from TextCommentFragment component can cause unnecessary re-renders when parent components update, even if this component's props haven't changed. This is particularly important for components used in lists or frequently re-rendered contexts.
Fix: Restore the memo wrapper:
export default memo(TextCommentFragment);
joekaufmanexpensify
previously approved these changes
Dec 17, 2025
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
Contributor
There was a problem hiding this comment.
Fine from a product perspective
truph01
reviewed
Dec 26, 2025
Contributor
Author
|
@truph01 Updated. Please help to check again. Thanks |
truph01
reviewed
Dec 26, 2025
Comment on lines
+10
to
+16
| if ('islarge' in tnode.attributes) { | ||
| style = [styleProp as TextStyle, styles.onlyEmojisText]; | ||
| } | ||
|
|
||
| if ('ismedium' in tnode.attributes) { | ||
| style = [styleProp as TextStyle, styles.emojisWithTextFontSize, styles.verticalAlignTopText]; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if ('islarge' in tnode.attributes) { | |
| style = [styleProp as TextStyle, styles.onlyEmojisText]; | |
| } | |
| if ('ismedium' in tnode.attributes) { | |
| style = [styleProp as TextStyle, styles.emojisWithTextFontSize, styles.verticalAlignTopText]; | |
| } | |
| if ('islarge' in tnode.attributes) { | |
| style = [styleProp as TextStyle, styles.onlyEmojisText]; | |
| } else if ('ismedium' in tnode.attributes) { | |
| style = [styleProp as TextStyle, styles.emojisWithTextFontSize, styles.verticalAlignTopText]; | |
| } else { | |
| style = null; | |
| } |
truph01
reviewed
Dec 29, 2025
| @@ -1,24 +1,23 @@ | |||
| import React, {useMemo} from 'react'; | |||
Contributor
There was a problem hiding this comment.
@paulnjs You shouldn't remove import React here. It will cause a crash app
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb SafariScreen.Recording.2025-12-29.at.16.00.39.movMacOS: Chrome / Safari
|
truph01
reviewed
Dec 29, 2025
truph01
approved these changes
Dec 29, 2025
MariaHCD
reviewed
Dec 29, 2025
MariaHCD
left a comment
MariaHCD
left a comment
Contributor
There was a problem hiding this comment.
Looks good overall, I just have some suggestions around naming!
| import type EmojiWithTooltipProps from './types'; | ||
|
|
||
| function EmojiWithTooltip({emojiCode, style = {}, isMedium = false}: EmojiWithTooltipProps) { | ||
| function EmojiWithTooltip({emojiCode, style = {}, isMedium = false, oneLine = false}: EmojiWithTooltipProps) { |
Contributor
There was a problem hiding this comment.
Suggested change
| function EmojiWithTooltip({emojiCode, style = {}, isMedium = false, oneLine = false}: EmojiWithTooltipProps) { | |
| function EmojiWithTooltip({emojiCode, style = {}, isMedium = false, isOnSeparateLine = false}: EmojiWithTooltipProps) { |
I think this name would be a bit more clear
| return privateUseAreaRegex.test(text); | ||
| } | ||
|
|
||
| function isSingleEmojiLine(line: string) { |
Contributor
There was a problem hiding this comment.
Suggested change
| function isSingleEmojiLine(line: string) { | |
| function isEmojiOnSeparateLine(line: string) { |
Here as well - I think this name would be a bit more clearer
Contributor
Author
|
@MariaHCD I updated. Please help to check again. Thank you! |
MariaHCD
approved these changes
Dec 30, 2025
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
56 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.2.89-1 🚀
|
7 tasks
53 tasks
Contributor
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.2.89-6 🚀
|
16 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$#76899
PROPOSAL:#76899 (comment)
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