refactor(emoji): centralize emoji rendering into app/components/Emoji… #6643
Conversation
|
|
WalkthroughAdds a reusable Emoji component at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant Picker as EmojiPickerEmoji
participant Shared as app/components/Emoji/Emoji
participant Shortname as useShortnameToUnicode
participant Custom as CustomEmoji
Note right of Picker: wrapper selects style then delegates
UI->>Picker: render({ emoji, size, style })
Picker->>Shared: render({ emoji, size, style })
alt emoji is string
Shared->>Shortname: formatShortnameToUnicode(trimmed or :shortname:)
Shortname-->>Shared: unicodeChar
Shared-->>UI: <Text style(fontSize=size)>{leading + unicodeChar + trailing}</Text>
else emoji is ICustomEmoji
Shared->>Custom: render({ emoji, width:size, height:size, style })
Custom-->>Shared: <Image />
Shared-->>UI: <CustomEmoji />
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing touches
🧪 Generate unit tests
Comment |
abhisek7154
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Hi 👋, this PR is ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/markdown/components/emoji/Emoji.tsx (1)
55-56: Fix potential undefined access when sanitizing custom emoji name
block.value?.value.replace(...)can throw whenblock.valueorblock.value.valueis undefined. Guard before callingreplace.-const emoji = getCustomEmoji?.(block.value?.value.replace(/\:/g, '')); +const raw = block.value?.value; +const emoji = raw ? getCustomEmoji?.(raw.replace(/:/g, '')) : undefined;
🧹 Nitpick comments (6)
app/containers/markdown/components/emoji/Emoji.tsx (2)
41-41: Minor: simplify space prefix logicMore explicit and avoids falsy truthiness quirks.
-const spaceLeft = index && index > 0 ? ' ' : ''; +const spaceLeft = (index ?? 0) > 0 ? ' ' : '';
80-90: Avoid redundant fontSize sourcesYou pass both
size={...}and text styles (styles.text/styles.textBig) that likely setfontSize. Prefer one source to prevent surprises.app/components/Emoji/Emoji.tsx (3)
5-5: Layering concern: components → containers dependency
components/Emojiimportscontainers/EmojiPicker/CustomEmoji, creating an upward dependency. Consider movingCustomEmojitoapp/componentsor a shared module to keep layers clean.
8-12: Doc the accepted string formsClarify via a short JSDoc that
emoji(string) may be Unicode,:shortname:or bareshortname, and that ASCII emoticons are rendered verbatim. Helps future call-sites.
1-30: Add focused tests for the centralized componentPlease add unit tests covering:
- '😄' (Unicode) → renders unchanged
- ':smile:' and 'smile' → render 😄
- ':)' (ASCII) → rendered verbatim
- Custom emoji object → uses CustomEmoji with size applied
I can scaffold Jest tests for these cases if helpful.
app/containers/EmojiPicker/Emoji.tsx (1)
3-11: Optional: memoize to reduce list re-rendersEmoji picker lists can be large; wrapping with
React.memo(ormemowith a shallow props check) can help.-import React from 'react'; +import React, { memo } from 'react'; ... -export const EmojiPickerEmoji = ({ emoji }: IEmojiProps): React.ReactElement => ( +export const EmojiPickerEmoji = memo(({ emoji }: IEmojiProps): React.ReactElement => ( <Emoji emoji={emoji} style={typeof emoji === 'string' ? styles.categoryEmoji : styles.customCategoryEmoji} /> -); +));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/Emoji/Emoji.tsx(1 hunks)app/containers/EmojiPicker/Emoji.tsx(1 hunks)app/containers/markdown/components/emoji/Emoji.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/EmojiPicker/Emoji.tsx (1)
app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
app/components/Emoji/Emoji.tsx (1)
app/definitions/IEmoji.ts (1)
ICustomEmoji(12-15)
🔇 Additional comments (1)
app/containers/EmojiPicker/Emoji.tsx (1)
7-12: LGTM: wrapper cleanly delegates to the shared Emoji componentKeeps styling concerns local and centralizes rendering. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/components/Emoji/Emoji.tsx (5)
5-5: Avoid component → container dependency; relocate CustomEmoji to a shared layerImporting from a container path in a shared component can create layering cycles. Consider moving
CustomEmojiunderapp/components/Emoji/CustomEmoji(or a shared module) and updating imports. Keep a re-export at the old path for compatibility if needed.
10-11: Clarify styling API to prevent misuse and casts (add textStyle/imageStyle)Today a single
styleprop is cast to bothTextStyleandImageStyle, which is easy to misuse. Keepstylefor BC, but add explicittextStyle/imageStyleprops and apply them in the respective branches.interface IEmojiProps { emoji: string | ICustomEmoji; - style?: StyleProp<TextStyle | ImageStyle>; + style?: StyleProp<TextStyle | ImageStyle>; // backward compatible + textStyle?: StyleProp<TextStyle>; + imageStyle?: StyleProp<ImageStyle>; size?: number; } -const Emoji = ({ emoji, style, size = 16 }: IEmojiProps) => { +const Emoji = ({ emoji, style, textStyle, imageStyle, size = 16 }: IEmojiProps) => { const { formatShortnameToUnicode } = useShortnameToUnicode(true); @@ - <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>]}> + <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>, textStyle]}> {`${leading}${converted}${trailing}`} </Text> @@ <CustomEmoji - style={[{ width: size, height: size }, style as StyleProp<ImageStyle>]} + style={[{ width: size, height: size }, style as StyleProp<ImageStyle>, imageStyle]} emoji={emoji} />Also applies to: 37-37, 45-45
1-1: Wrap with React.memo to reduce unnecessary re-renders in emoji-heavy listsThis component is frequently used in FlatLists. Memoization gives a cheap win.
-import React from 'react'; +import React, { memo } from 'react'; @@ -export default Emoji; +export default memo(Emoji);Also applies to: 51-51
37-37: Consider disabling font scaling for predictable emoji sizingIf you want consistent emoji sizes independent of system font scaling, set
allowFontScaling={false}(or expose it as a prop defaulting to false).- <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>]}> + <Text allowFontScaling={false} style={[{ fontSize: size }, style as StyleProp<TextStyle>]}>
17-33: Add minimal tests for the conversion matrixRecommend unit tests covering: Unicode (“😄”), colon shortname (:smile:), bare shortname (smile), ASCII (":)"), unknown shortname (":notfound:"), and custom emoji object. I can draft Jest + @testing-library/react-native tests if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/Emoji/Emoji.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/Emoji/Emoji.tsx (2)
app/definitions/IEmoji.ts (1)
ICustomEmoji(12-15)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)
🔇 Additional comments (1)
app/components/Emoji/Emoji.tsx (1)
17-33: Shortname handling looks correct and resolves the earlier colon-wrapping issue — LGTMColon-delimited and bare shortnames are converted; Unicode and ASCII are preserved; leading/trailing whitespace retained. This addresses the prior review concern.
Also applies to: 36-40
|
✅ All tests have passed locally and the lint-testunit job has completed successfully on CircleCI. |
|
@abhisek7154 we'll get here eventually. Just finishing internal priorities. Thanks for your contribution! |
… (#6535)
Proposed changes
Refactored emoji rendering by introducing a shared component at
app/components/Emoji/Emoji.tsx.app/containers/EmojiPicker/Emoji.tsxupdated to use the shared component.app/containers/markdown/components/emoji/Emoji.tsxupdated to use the shared component.This removes duplicate logic and ensures consistent rendering of Unicode, shortcode, and custom emojis.
Issue(s)
Closes #6535
How to test or reproduce
:smile::)Screenshots
Types of changes
Checklist
Further comments
This refactor consolidates emoji handling into a single reusable component.
Markdown and EmojiPicker previously duplicated similar logic, which made updates harder and risked inconsistent behavior.
Centralizing this logic ensures:
No new functionality is added — the focus is purely on improving structure and maintainability.
Summary by CodeRabbit