fix(emoji): migrate emoji rendering from emojione to emoji-toolkit (JoyPixels)#39501
fix(emoji): migrate emoji rendering from emojione to emoji-toolkit (JoyPixels)#39501adityasingh6533 wants to merge 3 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughSwap the emoji implementation from emojione to emoji-toolkit v10, update assets and generated emoji index, adjust sprite import ordering, remove per-emoji sprite positioning rules from many CSS files, add TypeScript declarations for emoji-toolkit, and update code that used emojione to use emoji-toolkit equivalents. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts">
<violation number="1" location="apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts:124">
P2: `render` now uses `shortnameToImage`, which does not convert native Unicode emoji to images. This regresses behavior compared to `toImage`, so Unicode emoji input (e.g., from mobile keyboards) will remain unconverted in this render path.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| render: emojione.toImage, | ||
| renderPicker: emojione.shortnameToImage, | ||
| sprites: true, | ||
| render: shortnameToImage, |
There was a problem hiding this comment.
P2: render now uses shortnameToImage, which does not convert native Unicode emoji to images. This regresses behavior compared to toImage, so Unicode emoji input (e.g., from mobile keyboards) will remain unconverted in this render path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts, line 124:
<comment>`render` now uses `shortnameToImage`, which does not convert native Unicode emoji to images. This regresses behavior compared to `toImage`, so Unicode emoji input (e.g., from mobile keyboards) will remain unconverted in this render path.</comment>
<file context>
@@ -270,12 +117,12 @@ const isEmojiSupported = (str: string) => {
- render: emojione.toImage,
- renderPicker: emojione.shortnameToImage,
- sprites: true,
+ render: shortnameToImage,
+ renderPicker: shortnameToImage,
+ sprites: false,
</file context>
| render: shortnameToImage, | |
| render: emojiToolkit.toImage, |
adityasingh6533
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
194-200:⚠️ Potential issue | 🟠 MajorGuard the Enter check against IME composition to prevent submitting partially composed text.
Line 199 uses
event.key === 'Enter'to detect submit, which will match during IME composition when the user is confirming a candidate. The previous keyCode checks (10, 13) avoided this because browsers report keyCode229during IME events, but the newevent.keycheck fires regardless, creating a regression for CJK input users.Add an
event.isComposingguard before theisSubmitKeycheck to block submission during composition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx` around lines 194 - 200, The Enter-submit detection in keyboardEventHandler (inside the useEffectEvent callback) uses event.key === 'Enter' which fires during IME composition; update the logic to guard against composition by checking event.isComposing before treating Enter as a submit key. Concretely, modify the isSubmitKey computation or add an early return so that if event.isComposing is true you do not consider Enter/CR/NewLine as submit (refers to keyboardEventHandler, useEffectEvent, and the isSubmitKey variable).apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts (1)
106-117:⚠️ Potential issue | 🟡 Minor
isEmojiSupportedhas identical logic toshortnameToImageand misleading naming.This function should return a
booleanindicating emoji support, but instead implements the same string transformation asshortnameToImage, returning HTML. Additionally, the function is exported but never used in the codebase. Either rename it to reflect its actual behavior, fix its implementation to return a boolean comparison, or remove it as dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts` around lines 106 - 117, The function isEmojiSupported currently performs the same string transformations as shortnameToImage and returns transformed HTML instead of a boolean; locate isEmojiSupported (and related symbols convertShortName, convertUnicode, emojiToolkit, shortnameToImage) and either: 1) change isEmojiSupported to return a boolean (e.g., compute transformed = str.replace(emojiToolkit.regShortNames, convertShortName) and, if emojiToolkit.ascii, transformed = transformed.replace(...), then return transformed !== str or use regex test to detect any emoji matches), or 2) if the intent was to perform conversion, rename isEmojiSupported to shortnameToImage (and remove the duplicate shortnameToImage implementation) to reflect behavior, or 3) if the function is unused, delete the exported isEmojiSupported to remove dead code; ensure exports/imports are updated accordingly.
🧹 Nitpick comments (1)
apps/meteor/emojiPicker.js (1)
1-6: Make the generated header deterministic.Line 3 bakes a machine-local timestamp into the committed artifact, so regenerating identical emoji data on two machines still produces a diff. Prefer omitting it entirely or emitting a stable UTC/ISO value from the generator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/emojiPicker.js` around lines 1 - 6, The generated file header currently includes a machine-local timestamp (line with "Last generated ..."), which makes commits non-deterministic; update the generator (generateEmojiIndex.mjs) to stop embedding a local timestamp into apps/meteor/emojiPicker.js and instead emit either no timestamp or a stable deterministic value (e.g., a fixed UTC/ISO string derived from a reproducible source or a build ID) in the header comment; modify the header template in generateEmojiIndex.mjs (the section that writes the "Last generated" line) to produce the chosen deterministic output and regenerate emojiPicker.js so the committed file no longer changes between identical runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs`:
- Around line 110-114: The generated file is written with
fs.writeFileSync('emojiPicker.ts', ...) in generateEmojiIndex.mjs which can
place the file in the wrong cwd relative to getEmojiConfig.ts's import; change
the write to use an absolute path built from the generator module directory (use
path.join(__dirname, 'emojiPicker.ts') or equivalent) so emojiPicker.ts is
created in the same lib directory as getEmojiConfig.ts, updating imports if
needed and ensuring you require/import path at the top of
generateEmojiIndex.mjs.
In `@apps/meteor/client/lib/utils/detectEmoji.ts`:
- Around line 13-16: The returned object from detectEmoji (in the function
returning name/className/image) incorrectly clears className for IMG elements,
which breaks downstream consumers that rely on renderer CSS classes; update the
return so className always preserves element.className (do not set it to '' for
element.tagName === 'IMG')—i.e., remove the IMG-specific blanking and return
element.className as-is in the object produced by the function (refer to the
properties name, className, image in the return structure).
In `@apps/meteor/client/lib/utils/renderEmoji.ts`:
- Around line 52-56: getEmojiClassNameAndDataTitle currently strips class names
for IMG-backed emoji by returning '' when isImageTag is true; restore the class
attribute so renderers keep their styling hook. Update the returned 'className'
to always use emojiElement.getAttribute('class') || '' (remove the isImageTag
conditional) so components that spread className receive the original class for
<img> elements; verify the same function/return object ('className',
'data-title', 'name', 'children') is left otherwise unchanged.
In `@apps/meteor/emojiPicker.js`:
- Around line 452-456: The category arrays currently include placeholder entries
with shortname 'no_name' that must be removed before serialization; update
generateEmojiIndex.mjs (where emoji category arrays are assembled/serialized) to
filter out any emoji objects whose shortname equals 'no_name' (or equivalent
placeholder) prior to writing the category arrays or calling the
serializer/export function, ensuring the output arrays contain only resolved
emoji items and preventing broken picker/search entries.
---
Outside diff comments:
In `@apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts`:
- Around line 106-117: The function isEmojiSupported currently performs the same
string transformations as shortnameToImage and returns transformed HTML instead
of a boolean; locate isEmojiSupported (and related symbols convertShortName,
convertUnicode, emojiToolkit, shortnameToImage) and either: 1) change
isEmojiSupported to return a boolean (e.g., compute transformed =
str.replace(emojiToolkit.regShortNames, convertShortName) and, if
emojiToolkit.ascii, transformed = transformed.replace(...), then return
transformed !== str or use regex test to detect any emoji matches), or 2) if the
intent was to perform conversion, rename isEmojiSupported to shortnameToImage
(and remove the duplicate shortnameToImage implementation) to reflect behavior,
or 3) if the function is unused, delete the exported isEmojiSupported to remove
dead code; ensure exports/imports are updated accordingly.
In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx`:
- Around line 194-200: The Enter-submit detection in keyboardEventHandler
(inside the useEffectEvent callback) uses event.key === 'Enter' which fires
during IME composition; update the logic to guard against composition by
checking event.isComposing before treating Enter as a submit key. Concretely,
modify the isSubmitKey computation or add an early return so that if
event.isComposing is true you do not consider Enter/CR/NewLine as submit (refers
to keyboardEventHandler, useEffectEvent, and the isSubmitKey variable).
---
Nitpick comments:
In `@apps/meteor/emojiPicker.js`:
- Around line 1-6: The generated file header currently includes a machine-local
timestamp (line with "Last generated ..."), which makes commits
non-deterministic; update the generator (generateEmojiIndex.mjs) to stop
embedding a local timestamp into apps/meteor/emojiPicker.js and instead emit
either no timestamp or a stable deterministic value (e.g., a fixed UTC/ISO
string derived from a reproducible source or a build ID) in the header comment;
modify the header template in generateEmojiIndex.mjs (the section that writes
the "Last generated" line) to produce the chosen deterministic output and
regenerate emojiPicker.js so the committed file no longer changes between
identical runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 194d4a5b-67a7-472b-bfa8-1647f7b6d6aa
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
apps/meteor/app/emoji-emojione/client/activity-sprites.cssapps/meteor/app/emoji-emojione/client/emojione-sprites.cssapps/meteor/app/emoji-emojione/client/flags-sprites.cssapps/meteor/app/emoji-emojione/client/food-sprites.cssapps/meteor/app/emoji-emojione/client/modifier-sprites.cssapps/meteor/app/emoji-emojione/client/nature-sprites.cssapps/meteor/app/emoji-emojione/client/objects-sprites.cssapps/meteor/app/emoji-emojione/client/people-sprites.cssapps/meteor/app/emoji-emojione/client/regional-sprites.cssapps/meteor/app/emoji-emojione/client/symbols-sprites.cssapps/meteor/app/emoji-emojione/client/travel-sprites.cssapps/meteor/app/emoji-emojione/lib/emoji-toolkit.d.tsapps/meteor/app/emoji-emojione/lib/emojiPicker.tsapps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjsapps/meteor/app/emoji-emojione/lib/getEmojiConfig.tsapps/meteor/app/emoji-emojione/server/callbacks.tsapps/meteor/app/emoji-emojione/server/lib.tsapps/meteor/app/emoji/client/lib.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/client/lib/utils/detectEmoji.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/client/lib/utils/renderEmoji.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/definition/externals/emojione.d.tsapps/meteor/emojiPicker.jsapps/meteor/package.jsonpackage.json
💤 Files with no reviewable changes (9)
- apps/meteor/app/emoji-emojione/client/regional-sprites.css
- apps/meteor/app/emoji-emojione/client/objects-sprites.css
- apps/meteor/app/emoji-emojione/client/nature-sprites.css
- apps/meteor/app/emoji-emojione/client/food-sprites.css
- apps/meteor/app/emoji-emojione/client/flags-sprites.css
- apps/meteor/app/emoji-emojione/client/travel-sprites.css
- apps/meteor/app/emoji-emojione/client/modifier-sprites.css
- apps/meteor/app/emoji-emojione/client/symbols-sprites.css
- apps/meteor/app/emoji-emojione/client/activity-sprites.css
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/emojiPicker.jsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/app/emoji-emojione/server/callbacks.tsapps/meteor/app/emoji/client/lib.tsapps/meteor/client/lib/utils/detectEmoji.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/emoji-emojione/server/lib.tsapps/meteor/app/emoji-emojione/lib/getEmojiConfig.tsapps/meteor/client/lib/utils/renderEmoji.tsapps/meteor/definition/externals/emojione.d.tsapps/meteor/app/emoji-emojione/lib/emoji-toolkit.d.ts
🧠 Learnings (8)
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/app/emoji/client/lib.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/client/lib/utils/detectEmoji.tsapps/meteor/client/lib/utils/renderEmoji.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/app/emoji-emojione/server/callbacks.tsapps/meteor/app/emoji/client/lib.tsapps/meteor/client/lib/utils/detectEmoji.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/emoji-emojione/server/lib.tsapps/meteor/app/emoji-emojione/lib/getEmojiConfig.tsapps/meteor/client/lib/utils/renderEmoji.tsapps/meteor/definition/externals/emojione.d.tsapps/meteor/app/emoji-emojione/lib/emoji-toolkit.d.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/views/root/hooks/useEmojiOne.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/app/emoji-emojione/server/callbacks.tsapps/meteor/app/emoji/client/lib.tsapps/meteor/client/lib/utils/detectEmoji.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/emoji-emojione/server/lib.tsapps/meteor/app/emoji-emojione/lib/getEmojiConfig.tsapps/meteor/client/lib/utils/renderEmoji.tsapps/meteor/definition/externals/emojione.d.tsapps/meteor/app/emoji-emojione/lib/emoji-toolkit.d.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-01-19T18:17:46.433Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
Applied to files:
apps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
🪛 Biome (2.4.6)
apps/meteor/emojiPicker.js
[error] 8-17: Illegal use of an export declaration outside of a module
(parse)
[error] 18-319: Illegal use of an export declaration outside of a module
(parse)
[error] 320-2242: Illegal use of an export declaration outside of a module
(parse)
🔇 Additional comments (13)
apps/meteor/app/emoji-emojione/client/emojione-sprites.css (1)
1-10: Sprite import coverage looks good.Including
regional-sprites.cssin the aggregate bundle fits the new sprite split and keeps the regional asset wired in with the other sprite groups.apps/meteor/client/views/root/hooks/useEmojiOne.ts (1)
25-34: LGTM!The property rename from
emojioneListtoemojiListcorrectly aligns with the emoji-toolkit library's API and is consistent with the server-side implementation atapps/meteor/app/emoji-emojione/server/lib.ts:21which uses the sameconfig.emojione.emojiListpattern. The use ofObject.entries()for iteration is a clean, modern approach.apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts (2)
11-11: LGTM!Import correctly updated from
emojionetoemoji-toolkitto use the new library directly.
194-200: LGTM!Emoji shortname-to-unicode conversions for email notifications correctly migrated to use
emojiToolkit.shortnameToUnicode(). The null/undefined guards with type checks remain intact.apps/meteor/app/emoji-emojione/server/lib.ts (1)
21-33: LGTM!Server-side emoji list population correctly updated to use
config.emojione.emojiList(matching emoji-toolkit's API), consistent with the client-side pattern inuseEmojiOne.ts(lines 25-33).apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs (1)
11-18: LGTM!Proper ES module path handling using
import.meta.urlto derive__dirname, and asset paths correctly updated to reference theemoji-toolkitpackage.apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts (5)
1-9: LGTM!Correct initialization of emoji-toolkit with appropriate image path and size configuration. The path
/packages/emojione/png/32/aligns with the asset structure.
11-20: LGTM!The
getEmojiImagePathhelper correctly handles both explicit size folders in the configured path and dynamically appends the emoji size when needed.
22-61: LGTM!
convertShortNamecorrectly migrated to useemojiToolkitAPIs. The memoization and fallback logic are preserved.
63-90: LGTM!
convertUnicodecorrectly migrated to useemojiToolkitAPIs for ASCII emoji handling.
119-127: LGTM!Export configuration correctly updated to expose
emojiToolkitunder theemojionekey for backward compatibility, with sprites disabled for individual image rendering.apps/meteor/definition/externals/emojione.d.ts (1)
1-6: This module declaration syntax is valid; both export styles are allowed and serve different purposes.The combination of
export * from 'emoji-toolkit'andexport = emojiToolkitin the same module declaration is not an anti-pattern. TypeScript supports both patterns simultaneously:export *re-exports named exports from emoji-toolkit, whileexport =handles the default export. This enables consumers to useimport emojione from 'emojione'(which is the pattern used throughout the codebase) and optionally import named exports if needed. The code compiles without errors.apps/meteor/app/emoji-emojione/lib/emoji-toolkit.d.ts (1)
1-56: Type declarations accurately reflect emoji-toolkit 10.0.0 API.The EmojiToolkit interface correctly declares all properties and methods used in the codebase. Line 25 correctly types
shortnames: string— emoji-toolkit uses a concatenated regex alternation pattern for pattern matching (e.g.,"smile|grinning|..."), distinct from theEmojiData.shortnames: string[]array of alternate names per emoji.
| fs.writeFileSync('emojiPicker.ts', output, { | ||
| encoding: 'utf8', | ||
| flag: 'w', | ||
| }); | ||
| console.log('Generated emojiPicker.js!'); | ||
| console.log('Generated emojiPicker.ts!'); |
There was a problem hiding this comment.
Output file path may not match import expectations.
The script writes emojiPicker.ts to the current working directory (line 110), but getEmojiConfig.ts imports from './emojiPicker' expecting the file to be in the same lib directory. Depending on where the script is executed from, this could cause the generated file to be placed in the wrong location.
Consider using path.join(__dirname, 'emojiPicker.ts') to ensure the file is generated in the correct directory:
Proposed fix
- fs.writeFileSync('emojiPicker.ts', output, {
+ fs.writeFileSync(path.join(__dirname, 'emojiPicker.ts'), output, {
encoding: 'utf8',
flag: 'w',
});📝 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.
| fs.writeFileSync('emojiPicker.ts', output, { | |
| encoding: 'utf8', | |
| flag: 'w', | |
| }); | |
| console.log('Generated emojiPicker.js!'); | |
| console.log('Generated emojiPicker.ts!'); | |
| fs.writeFileSync(path.join(__dirname, 'emojiPicker.ts'), output, { | |
| encoding: 'utf8', | |
| flag: 'w', | |
| }); | |
| console.log('Generated emojiPicker.ts!'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs` around lines 110 -
114, The generated file is written with fs.writeFileSync('emojiPicker.ts', ...)
in generateEmojiIndex.mjs which can place the file in the wrong cwd relative to
getEmojiConfig.ts's import; change the write to use an absolute path built from
the generator module directory (use path.join(__dirname, 'emojiPicker.ts') or
equivalent) so emojiPicker.ts is created in the same lib directory as
getEmojiConfig.ts, updating imports if needed and ensuring you require/import
path at the top of generateEmojiIndex.mjs.
| return { | ||
| name: element.getAttribute('title') || '', | ||
| className: element.tagName === 'IMG' ? '' : element.className, | ||
| image: backgroundImage || (sourceImage ? `url("${sourceImage}")` : undefined), |
There was a problem hiding this comment.
Preserve the renderer classes for <img> emojis.
This now blanks className for image-based emojis, but downstream consumers still pass that field through directly. Once render() returns <img> output, dropping the original classes will break any path that still relies on the renderer’s CSS classes for emoji styling/detection.
Suggested fix
return {
name: element.getAttribute('title') || '',
- className: element.tagName === 'IMG' ? '' : element.className,
+ className: element.getAttribute('class') || '',
image: backgroundImage || (sourceImage ? `url("${sourceImage}")` : undefined),
content: element.textContent || element.getAttribute('alt') || '',
};📝 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 { | |
| name: element.getAttribute('title') || '', | |
| className: element.tagName === 'IMG' ? '' : element.className, | |
| image: backgroundImage || (sourceImage ? `url("${sourceImage}")` : undefined), | |
| return { | |
| name: element.getAttribute('title') || '', | |
| className: element.getAttribute('class') || '', | |
| image: backgroundImage || (sourceImage ? `url("${sourceImage}")` : undefined), | |
| content: element.textContent || element.getAttribute('alt') || '', | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/utils/detectEmoji.ts` around lines 13 - 16, The
returned object from detectEmoji (in the function returning
name/className/image) incorrectly clears className for IMG elements, which
breaks downstream consumers that rely on renderer CSS classes; update the return
so className always preserves element.className (do not set it to '' for
element.tagName === 'IMG')—i.e., remove the IMG-specific blanking and return
element.className as-is in the object produced by the function (refer to the
properties name, className, image in the return structure).
| return { | ||
| 'className': emojiElement.getAttribute('class') || '', | ||
| 'data-title': emojiElement.getAttribute('data-title') || '', | ||
| 'name': emojiElement.getAttribute('name') || '', | ||
| 'children': emojiElement.innerHTML, | ||
| 'className': isImageTag ? '' : emojiElement.getAttribute('class') || '', | ||
| 'data-title': emojiElement.getAttribute('data-title') || emojiElement.getAttribute('title') || '', | ||
| 'name': emojiElement.getAttribute('name') || emojiElement.getAttribute('title') || '', | ||
| 'children': emojiElement.innerHTML || emojiElement.getAttribute('alt') || '', |
There was a problem hiding this comment.
Don't strip the class from IMG-backed emoji markup.
getEmojiClassNameAndDataTitle() still feeds components that spread className directly into their emoji element. Returning '' here for <img> output removes the renderer’s styling hook exactly in the new path this PR introduces.
Suggested fix
return {
- 'className': isImageTag ? '' : emojiElement.getAttribute('class') || '',
+ 'className': emojiElement.getAttribute('class') || '',
'data-title': emojiElement.getAttribute('data-title') || emojiElement.getAttribute('title') || '',
'name': emojiElement.getAttribute('name') || emojiElement.getAttribute('title') || '',
'children': emojiElement.innerHTML || emojiElement.getAttribute('alt') || '',📝 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 { | |
| 'className': emojiElement.getAttribute('class') || '', | |
| 'data-title': emojiElement.getAttribute('data-title') || '', | |
| 'name': emojiElement.getAttribute('name') || '', | |
| 'children': emojiElement.innerHTML, | |
| 'className': isImageTag ? '' : emojiElement.getAttribute('class') || '', | |
| 'data-title': emojiElement.getAttribute('data-title') || emojiElement.getAttribute('title') || '', | |
| 'name': emojiElement.getAttribute('name') || emojiElement.getAttribute('title') || '', | |
| 'children': emojiElement.innerHTML || emojiElement.getAttribute('alt') || '', | |
| return { | |
| 'className': emojiElement.getAttribute('class') || '', | |
| 'data-title': emojiElement.getAttribute('data-title') || emojiElement.getAttribute('title') || '', | |
| 'name': emojiElement.getAttribute('name') || emojiElement.getAttribute('title') || '', | |
| 'children': emojiElement.innerHTML || emojiElement.getAttribute('alt') || '', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/utils/renderEmoji.ts` around lines 52 - 56,
getEmojiClassNameAndDataTitle currently strips class names for IMG-backed emoji
by returning '' when isImageTag is true; restore the class attribute so
renderers keep their styling hook. Update the returned 'className' to always use
emojiElement.getAttribute('class') || '' (remove the isImageTag conditional) so
components that spread className receive the original class for <img> elements;
verify the same function/return object ('className', 'data-title', 'name',
'children') is left otherwise unchanged.
| 'no_name', | ||
| 'no_name', | ||
| 'no_name', | ||
| 'no_name', | ||
| 'no_name', |
There was a problem hiding this comment.
Filter out unresolved placeholder shortnames before publishing picker data.
These no_name entries will become broken picker/search items unless every consumer filters them out. Since this file is generated, the fix should happen in generateEmojiIndex.mjs before the category arrays are serialized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/emojiPicker.js` around lines 452 - 456, The category arrays
currently include placeholder entries with shortname 'no_name' that must be
removed before serialization; update generateEmojiIndex.mjs (where emoji
category arrays are assembled/serialized) to filter out any emoji objects whose
shortname equals 'no_name' (or equivalent placeholder) prior to writing the
category arrays or calling the serializer/export function, ensuring the output
arrays contain only resolved emoji items and preventing broken picker/search
entries.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/package.json`:
- Line 198: The app is configured by getEmojiConfig.ts to use individual PNGs
(imagePathPNG = '/packages/emojione/png/32/' and sprites = false) but those
files aren’t present; update the build/deploy pipeline or code so emoji assets
from the emoji-assets package are actually served: either add a build step that
copies emoji-assets' PNGs into public/packages/emojione/png/32/ and png/64/
(preserving filenames expected by getEmojiConfig.ts) or change getEmojiConfig.ts
to enable sprites = true and update sprite CSS paths to point to the bundled
sprites; reference getEmojiConfig.ts, imagePathPNG and sprites when making the
change so runtime emoji <img> src or sprite CSS paths resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d00ee405-b693-488e-9a3f-fb0d11329ec4
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
apps/meteor/package.json
| "emailreplyparser": "^0.0.5", | ||
| "emoji-toolkit": "^7.0.1", | ||
| "emojione": "^4.5.0", | ||
| "emoji-toolkit": "^10.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Pinned versions in apps/meteor/package.json:"
python - <<'PY'
import json, pathlib
pkg = json.loads(pathlib.Path("apps/meteor/package.json").read_text())
print("emoji-toolkit =", pkg["dependencies"].get("emoji-toolkit"))
print("emoji-assets =", pkg["devDependencies"].get("emoji-assets"))
PY
echo
echo "Current repo references that still assume the old layout/names:"
rg -n -C2 '/packages/emojione/|node_modules/emoji-toolkit/(png|sprites)|emojione-|joypixels-' apps/meteor
echo
echo "Published package layouts from UNPKG:"
python - <<'PY'
import json, urllib.request
for pkg, ver in [("emoji-toolkit", "10.0.0"), ("emoji-assets", "9.0.0")]:
meta = json.load(urllib.request.urlopen(f"https://unpkg.com/{pkg}@{ver}/?meta"))
paths = [item["path"] for item in meta.get("files", [])]
print(f"{pkg}@{ver}")
for needle in ("/png/32/", "/png/64/", "/sprites/", "/emoji.json", "/extras/css/joypixels.css"):
print(f" has {needle}: {any(p.startswith(needle) or p == needle.rstrip('/') for p in paths)}")
print()
PYRepository: RocketChat/Rocket.Chat
Length of output: 13941
🏁 Script executed:
# Check if emoji-assets is actually used anywhere
echo "=== Searching for emoji-assets usage ==="
rg -i "emoji-assets" apps/meteor --type js --type ts --type json -C 2
echo
echo "=== Check if public/packages/emojione directory exists and has PNG files ==="
find apps/meteor/public/packages/emojione -type f -name "*.png" 2>/dev/null | head -20 || echo "Directory or PNG files not found"
echo
echo "=== Check build or asset generation references ==="
rg -i "node_modules.*emoji|generateEmojiIndex|emoji.*public" apps/meteor --type js --type mjs -C 2
echo
echo "=== Check package.json for any build scripts related to emoji ==="
grep -A 5 -B 5 "emoji" apps/meteor/package.json || trueRepository: RocketChat/Rocket.Chat
Length of output: 1824
🏁 Script executed:
# Check if public/packages/emojione has a png subdirectory
echo "=== Check public/packages/emojione structure ==="
find apps/meteor/public/packages/emojione -type d
find apps/meteor/public/packages/emojione -type f | head -20
echo
echo "=== Search for where generateEmojiIndex is executed ==="
rg -i "generateEmojiIndex|emoji.*index" apps/meteor --type json --type js --type ts --type json5
echo
echo "=== Check what generateEmojiIndex.mjs actually does ==="
head -40 apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs
echo
echo "=== Look for build scripts that might run emoji index generation ==="
rg "buildEmoji|emoji" apps/meteor --type json | grep -i script || echo "No build scripts found for emoji"Repository: RocketChat/Rocket.Chat
Length of output: 2357
🏁 Script executed:
# Check if png subdirectories exist
echo "=== Check for png subdirectories in public/packages/emojione ==="
ls -la apps/meteor/public/packages/emojione/ 2>/dev/null | head -20
find apps/meteor/public/packages/emojione -name "*.png" -type d 2>/dev/null || echo "No PNG directories found"
echo
echo "=== Look for png/32 or png/64 directories anywhere ==="
find apps/meteor -path "*/packages/emojione/png/*" -type f 2>/dev/null | head -20 || echo "Not found"
echo
echo "=== Get the middle and end of generateEmojiIndex.mjs to see what PNG paths it expects ==="
wc -l apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs
sed -n '60,100p' apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs
echo "..."
tail -50 apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs
echo
echo "=== Check if there's any code that copies from emoji-toolkit to public ==="
rg "emoji-toolkit|emoji-assets" apps/meteor/app --type js --type mjs --type ts -C 3 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 3557
🏁 Script executed:
# Look at getEmojiConfig.ts more carefully to understand the rendering paths
echo "=== Full getEmojiConfig.ts content ==="
cat -n apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts
echo
echo "=== Check if there's code that uses imagePathPNG ==="
rg "imagePathPNG|png/32|png/64" apps/meteor --type ts --type js -C 3
echo
echo "=== Check what emoji-toolkit actually exports ==="
ls -la apps/meteor/node_modules/emoji-toolkit 2>/dev/null | head -30 || echo "node_modules not available in sandbox"
echo
echo "=== Look for any recent changes or migration comments ==="
rg "TODO|FIXME|migration|emoji-toolkit|emoji-assets" apps/meteor/app/emoji-emojione --type ts --type js --type mjsRepository: RocketChat/Rocket.Chat
Length of output: 7662
🏁 Script executed:
# Check if there's a way emoji-assets is supposed to be used
echo "=== Search for emoji-assets or where png/32 might come from ==="
rg "emoji-assets|png/32|png/64" apps/meteor --type ts --type js --type json -C 2
echo
echo "=== Check if sprites mode is ever enabled ==="
rg "sprites.*=.*true|\.sprites\s*=" apps/meteor/app/emoji-emojione --type ts --type js -C 2
echo
echo "=== Check for any static serving or asset copying logic ==="
rg "public.*emoji|emoji.*public|copy.*emoji|emoji.*copy" apps/meteor --type ts --type js | head -20
echo
echo "=== Look at how emoji-toolkit and emoji-assets are documented or installed ==="
find apps/meteor -name "*.md" -o -name "README*" | xargs grep -l emoji 2>/dev/null | head -5
echo
echo "=== Check if there are any comments about the migration in package.json or related files ==="
git log --oneline --all --grep="emoji\|emojione" -- apps/meteor/package.json 2>/dev/null | head -10 || echo "Git not available"Repository: RocketChat/Rocket.Chat
Length of output: 1888
🏁 Script executed:
# Check the README for emoji-emojione
echo "=== emoji-emojione README ==="
cat apps/meteor/app/emoji-emojione/README.md
echo
echo "=== Check CHANGELOG for emoji-related entries ==="
head -100 apps/meteor/CHANGELOG.md | grep -A 5 -B 5 -i emoji
echo
echo "=== Look at the commit that changed emoji ==="
git show 7527ee39dc --stat 2>/dev/null | head -30 || echo "Git show not available"
echo
echo "=== Check if there's actual image files in node_modules simulation or check if png/32 assets are supposed to exist ==="
find apps/meteor -type d -name "png" 2>/dev/null
echo
echo "=== Check server/lib.ts for how sprites config is used ==="
cat -n apps/meteor/app/emoji-emojione/server/lib.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 4663
Update emoji asset infrastructure before shipping emoji-toolkit 10.
The dependency bump to emoji-toolkit@^10.0.0 is incomplete. At runtime, apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts sets imagePathPNG = '/packages/emojione/png/32/' and disables sprites (sprites = false), which causes the app to render individual emoji as <img src="/packages/emojione/png/32/{code}.png"/>. However, these individual PNG files do not exist in that location. The emoji-assets@^9.0.0 package was added to devDependencies but is never referenced in code—per JoyPixels' migration documentation, this package contains the individual emoji assets that should be served from that path. The build or deployment process must either:
- Copy individual PNG assets from
emoji-assetstopublic/packages/emojione/png/32/andpng/64/, or - Enable sprite mode and ensure the sprite CSS paths are correctly configured.
As-is, all emoji rendering will fail at runtime.
Also applies to: 414-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/package.json` at line 198, The app is configured by
getEmojiConfig.ts to use individual PNGs (imagePathPNG =
'/packages/emojione/png/32/' and sprites = false) but those files aren’t
present; update the build/deploy pipeline or code so emoji assets from the
emoji-assets package are actually served: either add a build step that copies
emoji-assets' PNGs into public/packages/emojione/png/32/ and png/64/ (preserving
filenames expected by getEmojiConfig.ts) or change getEmojiConfig.ts to enable
sprites = true and update sprite CSS paths to point to the bundled sprites;
reference getEmojiConfig.ts, imagePathPNG and sprites when making the change so
runtime emoji <img> src or sprite CSS paths resolve correctly.
|
Hello @adityasingh6533 , I had actually already made a PR #39207, hope you knew |
|
Hi everyone, I was looking through this issue about migrating emoji rendering from Emojione to JoyPixels, and it seems like an interesting improvement for the project. I’d be happy to work on this if it’s still open. From what I understand, the goal is to replace the current emoji rendering with the JoyPixels toolkit while keeping the existing message rendering behavior consistent. My plan would be to first identify where Emojione is currently used in the codebase, then carefully migrate those parts to JoyPixels and test to make sure emoji rendering continues to work properly across messages and reactions. Please let me know if this approach sounds good, or if there are specific things I should keep in mind before starting. I’d really appreciate any guidance. |
|
Closing this PR in favor of #39411, where we’re replacing Emojione with native Unicode emojis. It’s still a draft, and we’d love help from the community to get it across the finish line 🙌 |
5 participants
This PR migrates emoji rendering from the deprecated
emojioneruntime toemoji-toolkit(JoyPixels).The changes update emoji rendering logic, emoji picker integration, and detection utilities to ensure compatibility with the new library.
It also fixes rendering inconsistencies caused by legacy
emojioneassumptions while keeping the existing behavior of the emoji picker and message rendering intact.Closes #31069
Summary by CodeRabbit