chore: update emoji picker to use emoji-toolkit #37716
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 |
|
|
WalkthroughReplaces hard-coded emoji.json path with Node's createRequire + require.resolve for dynamic resolution, adds top-level try/catch that logs and calls process.exit(1) on resolution failure in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-10T19:06:20.146ZApplied to files:
📚 Learning: 2025-11-19T12:32:29.696ZApplied to files:
🪛 Biome (2.1.2)apps/meteor/package.json[error] 60-60: unexpected character (parse) [error] 60-60: expected Remove update (parse) [error] 60-60: expected Remove -e (parse) [error] 60-60: Invalid exponent part (parse) [error] 60-60: expected Remove moji (parse) [error] 60-60: Minus must be followed by a digit (parse) [error] 60-60: expected Remove library (parse) [error] 60-60: expected Remove -24917 (parse) [error] 61-61: expected Remove "test" (parse) [error] 68-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 69-69: unexpected character (parse) [error] 70-70: End of file expected Use an array for a sequence of values: (parse) [error] 70-70: End of file expected Use an array for a sequence of values: (parse) [error] 70-70: End of file expected Use an array for a sequence of values: (parse) [error] 70-70: End of file expected Use an array for a sequence of values: (parse) [error] 71-71: End of file expected Use an array for a sequence of values: (parse) [error] 71-71: End of file expected Use an array for a sequence of values: (parse) [error] 71-71: End of file expected Use an array for a sequence of values: (parse) [error] 72-72: End of file expected Use an array for a sequence of values: (parse) [error] 73-73: End of file expected Use an array for a sequence of values: (parse) [error] 73-73: End of file expected Use an array for a sequence of values: (parse) [error] 73-76: End of file expected Use an array for a sequence of values: (parse) [error] 76-77: End of file expected Use an array for a sequence of values: (parse) [error] 499-500: End of file expected Use an array for a sequence of values: (parse) 🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (2)
apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs (1)
27-29: Consider removing redundant existence check.The
fs.existsSynccheck at line 27 is now redundant sincerequire.resolve()at line 17 only succeeds if the file exists. If resolution fails, the script exits before reaching this check.That said, this defensive check doesn't cause harm and might catch edge cases, so removing it is purely optional.
If you'd like to remove the redundancy:
-if (!fs.existsSync(emojiJsonFile)) { - console.error(`${emojiJsonFile} doesn't exist.`); -} else { - const emojiJson = fs.readFileSync(emojiJsonFile); - generateEmojiPicker(emojiJson); -} +const emojiJson = fs.readFileSync(emojiJsonFile); +generateEmojiPicker(emojiJson);apps/meteor/package.json (1)
172-172: Consider upgrading emoji-toolkit to the latest version.Version 7.0.1 is valid and has no known security vulnerabilities, but it is significantly outdated. The latest version is 9.0.1 (released on npm). Consider upgrading to benefit from bug fixes, performance improvements, and staying current with the package ecosystem. If upgrading is not feasible due to breaking changes, ensure the dependency is periodically reviewed.
📜 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 selected for processing (2)
apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs(1 hunks)apps/meteor/package.json(2 hunks)
🔇 Additional comments (4)
apps/meteor/app/emoji-emojione/lib/generateEmojiIndex.mjs (3)
10-13: LGTM!The use of
createRequirewithimport.meta.urlis the correct approach for resolving CommonJS modules in an ESM context. This enables proper resolution ofemoji-toolkitfrom the workspace's node_modules.
15-22: LGTM!The error handling is robust with clear, actionable error messages. Exiting early with a non-zero status code when the dependency cannot be resolved is appropriate for a build script.
24-24: LGTM!This comment is helpful for maintainability. It clearly explains why
emojione-assetsis still referenced for sprites, aligning with the PR's scope to update only the metadata source.apps/meteor/package.json (1)
172-172: Remove emoji-toolkit from dependencies if not used at runtime.The package
emoji-toolkitappears in bothdevDependencies(line 172) anddependencies(line 351). SincegenerateEmojiIndex.mjsis a build-time script that generates static output files,emoji-toolkitlikely belongs only indevDependencies. Confirm that no runtime code directly importsemoji-toolkit; if not, remove it fromdependencies.
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 (1)
apps/meteor/package.json (1)
598-598: Remove emojione-assets from devDependencies.Per the PR objectives, emojione-assets should be fully removed. However, it still remains at line 598 in devDependencies. This contradicts the stated goal of replacing the deprecated library. Remove this entry to complete the migration.
- "emojione-assets": "^4.5.0",
📜 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 selected for processing (1)
apps/meteor/package.json(2 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
apps/meteor/package.json
[error] 60-60: unexpected character /
(parse)
[error] 60-60: expected , but instead found update
Remove update
(parse)
[error] 60-60: expected : but instead found -e
Remove -e
(parse)
[error] 60-60: Invalid exponent part
(parse)
[error] 60-60: expected , but instead found moji
Remove moji
(parse)
[error] 60-60: Minus must be followed by a digit
(parse)
[error] 60-60: expected , but instead found library
Remove library
(parse)
[error] 60-60: expected : but instead found -24917
Remove -24917
(parse)
[error] 61-61: expected , but instead found "test"
Remove "test"
(parse)
[error] 79-79: The key devDependencies was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (1)
apps/meteor/package.json (1)
373-373: Approve: emoji-toolkit added to dependencies.The addition of emoji-toolkit to production dependencies is correct per the PR objectives.
FizaSiddique123
force-pushed
the
chore/update-emoji-library-24917
branch
from
ac75209 to
5e8fafe
Compare
2 participants
Proposed changes (including videos or screenshots)
This PR updates the emoji picker to use the newer emoji-toolkit dataset instead of the deprecated emojione-assets JSON metadata.
What was changed
generateEmojiIndex.mjsso it resolves emoji metadata fromemoji-toolkit/emoji.jsonusingrequire.resolve().emojione-assetsfor emoji data."emoji-toolkit": "^7.0.1"toapps/meteor/package.jsonand removed"emojione-assets".emojione-assetsto avoid breaking the existing UI.(A follow-up PR can migrate sprites to JoyPixels if needed.)
Why this change is needed
emojione-assetsis outdated and no longer maintained.emoji-toolkitprovides an updated, actively maintained emoji dataset that aligns Rocket.Chat with current emoji standards and prepares the codebase for future improvements.This PR completes the step requested in #24917 regarding replacing the old emoji metadata source.
Issue(s)
Closes #24917
Steps to test or reproduce
yarn dev).(No new sprites yet — only the metadata source has been updated.)
generateEmojiIndex.mjs), it will now load data fromemoji-toolkitinstead ofemojione-assets.Further comments
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.