Feat/skeleton loading #6814
Feat/skeleton loading #6814
Conversation
… textAlignVertical
…manth2377/Rocket.Chat.ReactNative into feat/reusable-emoji-component
WalkthroughAdds shared Emoji and Skeleton components; refactors emoji rendering to use the shared Emoji across EmojiPicker and Markdown; adds skeleton loading placeholders and loading state to image renders in Markdown, Message attachments, and ServerAvatar. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Consumer
participant SharedEmoji as SharedEmoji (component)
participant CustomEmoji as CustomEmoji
participant Theme as Theme
participant Text as Text
Consumer->>SharedEmoji: provide `literal`, `customEmoji`, `isBigEmoji`
alt `customEmoji` present
SharedEmoji->>CustomEmoji: Render custom asset with size
CustomEmoji-->>SharedEmoji: Return element
else `literal` present
SharedEmoji->>Theme: read color
SharedEmoji->>Text: render unicode text with sizing/color
Text-->>SharedEmoji: Return element
else neither
SharedEmoji-->>Consumer: Return null
end
sequenceDiagram
autonumber
participant Component
participant Skeleton
participant ExpoImage as Image
participant onLoadEnd
Component->>Component: set loading = true
Component->>Skeleton: render placeholder
Skeleton-->>Component: visible
Component->>ExpoImage: render with 0 width/height while loading
ExpoImage->>onLoadEnd: image loads
onLoadEnd-->>Component: set loading = false
Component->>ExpoImage: render with normal dimensions
Component->>Skeleton: hide placeholder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
app/views/WorkspaceView/ServerAvatar.tsx (1)
33-47: Update stale TODO and consider resettingloadingwhen avatar changes
- Line 33:
// TODO: missing skeletonis now outdated; the Skeleton is implemented, so this comment should be removed or updated.- Optionally, if
url/imagecan change without remounting, consider resettingloadingwhen they change so the skeleton appears again for new avatars.For example:
-// TODO: missing skeleton const ServerAvatar = React.memo(({ url, image }: IServerAvatar) => { const { colors } = useTheme(); const [loading, setLoading] = React.useState(true); + + React.useEffect(() => { + setLoading(true); + }, [url, image]);app/containers/markdown/components/Image.tsx (1)
16-27: Markdown image skeleton behavior is sound; consider matching final dimensionsThe loading state + Skeleton pattern is correct and should prevent flashes. If inline images can vary significantly in size, you might later want the Skeleton to match
styles.inlineImagedimensions (or known width/height) to reduce layout shifts when the image appears, but it’s not required for correctness.app/containers/message/Components/Attachments/Image/Image.tsx (1)
86-92: Revisit Skeleton width source andiconNametyping
width={styles.image.width}assumesstyles.imageis a plain object with awidthfield. Ifstylescomes fromStyleSheet.create,styles.imageis a registered style ID andstyles.image.widthwill beundefined, so the Skeleton may render without a meaningful width. In that case, consider using an explicit width (e.g.'100%') or passing dimensions via props rather than reading from the style object:- <Skeleton width={styles.image.width} height={200} borderRadius={4} /> + <Skeleton width='100%' height={200} borderRadius={4} />
- The cast
('loading' as any)works but weakens type safety. If convenient, you might extendOverlayComponent’siconNametype to include'loading'and drop theas any.app/containers/Emoji/Emoji.tsx (2)
26-34: Consider memoizing emoji size calculations.The size objects are recalculated on every render. Memoizing them would improve performance, especially for frequently rendered emojis.
+ const customEmojiSize = React.useMemo(() => ({ - const customEmojiSize = { width: 15 * fontScale, height: 15 * fontScale - }; + }), [fontScale]); + const customEmojiBigSize = React.useMemo(() => ({ - const customEmojiBigSize = { width: 30 * fontScale, height: 30 * fontScale - }; + }), [fontScale]);
36-50: Consider consolidating duplicate CustomEmoji rendering logic.Lines 37 and 49 contain nearly identical
CustomEmojirendering code. While this duplication is minor, consolidating it would improve maintainability.+ const renderCustomEmoji = (emoji: any) => ( + <CustomEmoji + style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style as StyleProp<ImageStyle>]} + emoji={emoji} + /> + ); + if (customEmoji) { - return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style as StyleProp<ImageStyle>]} emoji={customEmoji} />; + return renderCustomEmoji(customEmoji); } if (!literal) { return null; } const emojiUnicode = formatShortnameToUnicode(literal); const emojiName = literal.replace(/:/g, ''); const foundCustomEmoji = getCustomEmoji?.(emojiName); if (foundCustomEmoji) { - return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style as StyleProp<ImageStyle>]} emoji={foundCustomEmoji} />; + return renderCustomEmoji(foundCustomEmoji); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
app/containers/Emoji/Emoji.tsx(1 hunks)app/containers/Emoji/__tests__/Emoji.test.tsx(1 hunks)app/containers/Emoji/index.ts(1 hunks)app/containers/EmojiPicker/Emoji.tsx(1 hunks)app/containers/Skeleton/Skeleton.tsx(1 hunks)app/containers/Skeleton/__tests__/Skeleton.test.tsx(1 hunks)app/containers/Skeleton/index.ts(1 hunks)app/containers/markdown/components/Image.tsx(2 hunks)app/containers/markdown/components/emoji/Emoji.tsx(2 hunks)app/containers/message/Components/Attachments/Image/Image.tsx(2 hunks)app/views/WorkspaceView/ServerAvatar.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/views/WorkspaceView/ServerAvatar.tsx (1)
app/containers/LoginServices/styles.ts (1)
BORDER_RADIUS(6-6)
app/containers/Skeleton/Skeleton.tsx (3)
jest.setup.js (1)
React(174-174)app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
colors(280-302)
app/containers/EmojiPicker/Emoji.tsx (2)
app/containers/markdown/Markdown.stories.tsx (1)
Emoji(94-101)app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
app/containers/markdown/components/emoji/Emoji.tsx (4)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/containers/markdown/Markdown.stories.tsx (1)
Emoji(94-101)app/containers/markdown/components/emoji/index.ts (1)
Emoji(4-4)app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
🪛 ESLint
app/containers/Skeleton/Skeleton.tsx
[error] 2-2: There should be no empty line within import group
(import/order)
app/containers/markdown/components/emoji/Emoji.tsx
[error] 25-25: 'index' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (6)
app/containers/Emoji/index.ts (1)
1-1: Emoji barrel export is correctRe-exporting the default Emoji as a named export from the index keeps the API clean and consistent.
app/containers/Skeleton/index.ts (1)
1-1: Skeleton barrel export looks goodDefault re-export matches the Skeleton implementation and simplifies imports.
app/containers/Skeleton/__tests__/Skeleton.test.tsx (1)
6-27: Tests are good; double‑check theme mock path actually hits the component importThe snapshot coverage for base + styled Skeleton is useful and the skeleton-placeholder mock is straightforward. One thing to verify:
Skeleton.tsximportsuseThemefrom../../theme, while this test also mocks'../../theme'but from a different directory depth. Depending on your Jest resolver and moduleNameMapper, that relative path may resolve to a different file than the one used bySkeleton, meaning the mock might not apply.If you rely on this mock, please confirm it’s wired to the same module as the component’s import (or switch to an absolute/module alias for both).
app/containers/Emoji/__tests__/Emoji.test.tsx (1)
7-37: Emoji tests cleanly cover core behaviorsThe mocks are scoped well and the tests validate both unicode rendering and custom emoji rendering via the mocked CustomEmoji component. This gives good confidence in the new shared Emoji implementation.
app/containers/EmojiPicker/Emoji.tsx (1)
7-12: LGTM! Clean refactor to shared emoji component.The delegation to
SharedEmojiis well-structured and maintains the original component API while consolidating rendering logic.app/containers/markdown/components/emoji/Emoji.tsx (1)
15-23: LGTM! Helper function correctly handles emoji token formats.The conversion to an arrow function and the token extraction logic are appropriate.
| return ( | ||
| <Text | ||
| style={[ | ||
| { color: colors.fontDefault }, | ||
| isBigEmoji ? { fontSize: 30, lineHeight: 43 } : { fontSize: 16, lineHeight: 22 }, | ||
| style, | ||
| isAvatar && avatarStyle | ||
| ]}> | ||
| {emojiUnicode} | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
Consider accessibility: hard-coded font sizes don't respect user preferences.
Line 62 uses fixed fontSize values (16 and 30) for non-avatar emojis, which don't scale with the user's font size preferences. In contrast, avatar emojis (lines 53-54) use fontScaleLimited for better accessibility.
Consider applying consistent scaling:
return (
<Text
style={[
{ color: colors.fontDefault },
- isBigEmoji ? { fontSize: 30, lineHeight: 43 } : { fontSize: 16, lineHeight: 22 },
+ isBigEmoji ? { fontSize: 30 * fontScaleLimited, lineHeight: 43 * fontScaleLimited } : { fontSize: 16 * fontScaleLimited, lineHeight: 22 * fontScaleLimited },
style,
isAvatar && avatarStyle
]}>
{emojiUnicode}
</Text>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Text | |
| style={[ | |
| { color: colors.fontDefault }, | |
| isBigEmoji ? { fontSize: 30, lineHeight: 43 } : { fontSize: 16, lineHeight: 22 }, | |
| style, | |
| isAvatar && avatarStyle | |
| ]}> | |
| {emojiUnicode} | |
| </Text> | |
| ); | |
| return ( | |
| <Text | |
| style={[ | |
| { color: colors.fontDefault }, | |
| isBigEmoji ? { fontSize: 30 * fontScaleLimited, lineHeight: 43 * fontScaleLimited } : { fontSize: 16 * fontScaleLimited, lineHeight: 22 * fontScaleLimited }, | |
| style, | |
| isAvatar && avatarStyle | |
| ]}> | |
| {emojiUnicode} | |
| </Text> | |
| ); |
🤖 Prompt for AI Agents
In app/containers/Emoji/Emoji.tsx around lines 58 to 68, the Text component uses
hard-coded fontSize values (16 and 30) that don't respect user font-size
preferences; replace those fixed sizes with the same scaling utility used for
avatars (fontScaleLimited) so both big and normal emoji sizes scale with
accessibility settings, and adjust corresponding lineHeight to maintain visual
proportions (e.g., compute lineHeight from the scaled font size or apply a
multiplier) while keeping the rest of the style array and avatar handling
unchanged.
| const Emoji = ({ block, isBigEmoji, style, index, isAvatar }: IEmojiProps) => { | ||
| const { getCustomEmoji } = useContext(MarkdownContext); | ||
| const { fontScale } = useWindowDimensions(); | ||
| const { fontScaleLimited } = useResponsiveLayout(); | ||
| const { formatShortnameToUnicode } = useShortnameToUnicode(); | ||
| const spaceLeft = index && index > 0 ? ' ' : ''; | ||
| const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji); | ||
|
|
||
| if ('unicode' in block) { | ||
| return <Text style={[{ color: colors.fontDefault }, isBigEmoji ? styles.textBig : styles.text]}>{block.unicode}</Text>; | ||
| } | ||
|
|
||
| const emojiToken = getEmojiToken(block, isAvatar); | ||
| const emojiUnicode = formatShortnameToUnicode(emojiToken); | ||
| const emoji = getCustomEmoji?.(block.value?.value.replace(/\:/g, '')); | ||
| const isAsciiEmoji = !!block?.shortCode && block.value?.value !== block?.shortCode; | ||
| const displayAsciiEmoji = !convertAsciiEmoji && isAsciiEmoji && !!block.value; | ||
| const customEmojiSize = { | ||
| width: 15 * fontScale, | ||
| height: 15 * fontScale | ||
| }; | ||
|
|
||
| const customEmojiBigSize = { | ||
| width: 30 * fontScale, | ||
| height: 30 * fontScale | ||
| }; | ||
|
|
||
| const avatarStyle = { | ||
| fontSize: 30 * fontScaleLimited, | ||
| lineHeight: 30 * fontScaleLimited, | ||
| textAlign: 'center', | ||
| textAlignVertical: 'center' | ||
| }; | ||
|
|
||
| if (emoji) { | ||
| return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={emoji} />; | ||
| } | ||
| const literal = getEmojiToken(block, !!isAvatar); | ||
|
|
||
| return ( | ||
| <Text | ||
| style={[ | ||
| { color: colors.fontDefault }, | ||
| isBigEmoji && emojiToken !== emojiUnicode ? styles.textBig : styles.text, | ||
| style, | ||
| isAvatar && avatarStyle | ||
| ]}> | ||
| {spaceLeft} | ||
| {displayAsciiEmoji ? <Plain value={block.value!.value} /> : emojiUnicode} | ||
| </Text> | ||
| ); | ||
| return <SharedEmoji literal={literal} isBigEmoji={isBigEmoji} style={style} isAvatar={isAvatar} getCustomEmoji={getCustomEmoji} />; | ||
| }; |
There was a problem hiding this comment.
Remove unused index parameter.
ESLint correctly flags that index is defined but never used. Either remove it or prefix with underscore if it's part of a required interface.
Additionally, line 27's !!isAvatar double-negation is unnecessary since isAvatar is already boolean | undefined and that type is accepted by SharedEmoji.
Apply this diff:
-const Emoji = ({ block, isBigEmoji, style, index, isAvatar }: IEmojiProps) => {
+const Emoji = ({ block, isBigEmoji, style, isAvatar }: IEmojiProps) => {
const { getCustomEmoji } = useContext(MarkdownContext);
- const literal = getEmojiToken(block, !!isAvatar);
+ const literal = getEmojiToken(block, isAvatar ?? false);
return <SharedEmoji literal={literal} isBigEmoji={isBigEmoji} style={style} isAvatar={isAvatar} getCustomEmoji={getCustomEmoji} />;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const Emoji = ({ block, isBigEmoji, style, index, isAvatar }: IEmojiProps) => { | |
| const { getCustomEmoji } = useContext(MarkdownContext); | |
| const { fontScale } = useWindowDimensions(); | |
| const { fontScaleLimited } = useResponsiveLayout(); | |
| const { formatShortnameToUnicode } = useShortnameToUnicode(); | |
| const spaceLeft = index && index > 0 ? ' ' : ''; | |
| const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji); | |
| if ('unicode' in block) { | |
| return <Text style={[{ color: colors.fontDefault }, isBigEmoji ? styles.textBig : styles.text]}>{block.unicode}</Text>; | |
| } | |
| const emojiToken = getEmojiToken(block, isAvatar); | |
| const emojiUnicode = formatShortnameToUnicode(emojiToken); | |
| const emoji = getCustomEmoji?.(block.value?.value.replace(/\:/g, '')); | |
| const isAsciiEmoji = !!block?.shortCode && block.value?.value !== block?.shortCode; | |
| const displayAsciiEmoji = !convertAsciiEmoji && isAsciiEmoji && !!block.value; | |
| const customEmojiSize = { | |
| width: 15 * fontScale, | |
| height: 15 * fontScale | |
| }; | |
| const customEmojiBigSize = { | |
| width: 30 * fontScale, | |
| height: 30 * fontScale | |
| }; | |
| const avatarStyle = { | |
| fontSize: 30 * fontScaleLimited, | |
| lineHeight: 30 * fontScaleLimited, | |
| textAlign: 'center', | |
| textAlignVertical: 'center' | |
| }; | |
| if (emoji) { | |
| return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={emoji} />; | |
| } | |
| const literal = getEmojiToken(block, !!isAvatar); | |
| return ( | |
| <Text | |
| style={[ | |
| { color: colors.fontDefault }, | |
| isBigEmoji && emojiToken !== emojiUnicode ? styles.textBig : styles.text, | |
| style, | |
| isAvatar && avatarStyle | |
| ]}> | |
| {spaceLeft} | |
| {displayAsciiEmoji ? <Plain value={block.value!.value} /> : emojiUnicode} | |
| </Text> | |
| ); | |
| return <SharedEmoji literal={literal} isBigEmoji={isBigEmoji} style={style} isAvatar={isAvatar} getCustomEmoji={getCustomEmoji} />; | |
| }; | |
| const Emoji = ({ block, isBigEmoji, style, isAvatar }: IEmojiProps) => { | |
| const { getCustomEmoji } = useContext(MarkdownContext); | |
| const literal = getEmojiToken(block, isAvatar ?? false); | |
| return <SharedEmoji literal={literal} isBigEmoji={isBigEmoji} style={style} isAvatar={isAvatar} getCustomEmoji={getCustomEmoji} />; | |
| }; |
🧰 Tools
🪛 ESLint
[error] 25-25: 'index' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In app/containers/markdown/components/emoji/Emoji.tsx around lines 25 to 30,
remove the unused index parameter from the Emoji function signature (or rename
it to _index if the IEmojiProps interface requires it) and stop using the
unnecessary double-negation (!!isAvatar) when calling getEmojiToken — pass
isAvatar directly; update the function signature and call to SharedEmoji
accordingly so ESLint no longer reports an unused variable and the boolean is
passed without redundant coercion.
1 participant
Description
This PR implements skeleton loading for message images and workspace avatars, unifying the loading experience with the rest of the app.
Closes #4810
Changes
Skeletoncomponent inapp/containers/Skeleton.OverlayComponentloading spinner withSkeletoninMessageImage.Skeletonloading toMarkdownimages.Skeletonloading toServerAvatarinWorkspaceView.react-native-image-progressandreact-native-progress.Verification
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.