chore: fix unsafe type assertions in useEmojiOne#39155
Closed
NAME-ASHWANIYADAV wants to merge 1 commit intoRocketChat:developfrom
Closed
chore: fix unsafe type assertions in useEmojiOne#39155NAME-ASHWANIYADAV wants to merge 1 commit intoRocketChat:developfrom
NAME-ASHWANIYADAV wants to merge 1 commit intoRocketChat:developfrom
Conversation
Contributor
|
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 |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2026-02-25T20:10:16.987ZApplied to files:
📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
📚 Learning: 2026-02-24T19:09:09.561ZApplied to files:
📚 Learning: 2026-02-10T16:32:42.586ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
🔇 Additional comments (3)
WalkthroughAdded a compile-time type constraint to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/useEmojiOne.ts (1)
23-23: Consider removing inline comment.As per coding guidelines, TypeScript files should avoid code comments in the implementation. The comment's intent is clear from the code context.
🔧 Suggested change
- // RocketChat.emoji.list is the collection of emojis from all emoji packages for (const [key, currentEmoji] of Object.entries(config.emojione.emojioneList)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/root/hooks/useEmojiOne.ts` at line 23, Remove the inline comment "// RocketChat.emoji.list is the collection of emojis from all emoji packages" from the TypeScript implementation in useEmojiOne.ts; locate the comment near references to RocketChat.emoji.list (e.g., within the useEmojiOne hook or where RocketChat.emoji.list is accessed) and delete that line so the code complies with the guideline to avoid implementation comments in TS files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/root/hooks/useEmojiOne.ts`:
- Line 23: Remove the inline comment "// RocketChat.emoji.list is the collection
of emojis from all emoji packages" from the TypeScript implementation in
useEmojiOne.ts; locate the comment near references to RocketChat.emoji.list
(e.g., within the useEmojiOne hook or where RocketChat.emoji.list is accessed)
and delete that line so the code complies with the guideline to avoid
implementation comments in TS files.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/emoji-emojione/lib/emojiPicker.tsapps/meteor/client/views/root/hooks/useEmojiOne.ts
📜 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/app/emoji-emojione/lib/emojiPicker.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 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.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/client/views/root/hooks/useEmojiOne.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.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/app/emoji-emojione/lib/emojiPicker.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/app/emoji-emojione/lib/emojiPicker.ts
🔇 Additional comments (2)
apps/meteor/app/emoji-emojione/lib/emojiPicker.ts (1)
1-19: LGTM!Good use of
satisfies EmojiCategory[]to validate thei18nvalues asTranslationKeyat compile time while preserving the literal types. This is the correct TypeScript approach for improving type safety without requiring runtime changes.apps/meteor/client/views/root/hooks/useEmojiOne.ts (1)
14-21: LGTM!Clean refactor consolidating the emoji package initialization into a single typed object literal. This properly eliminates the need for
as anyassertions by constructing the object inline with the expected properties.
NAME-ASHWANIYADAV
force-pushed
the
chore/fix-unsafe-type-assertion-useEmojiOne
branch
from
530e12c to
36865be
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/useEmojiOne.ts (1)
23-31: Code looks good; minor type annotation redundancy.The emoji list population logic is correct. The explicit type annotation on line 28
(shortname: string)may be redundant ifcurrentEmoji.shortnamesis already typed asstring[], as TypeScript would infer the type.Optional: Remove redundant type annotation
if (currentEmoji.shortnames) { - currentEmoji.shortnames.forEach((shortname: string) => { + currentEmoji.shortnames.forEach((shortname) => { emoji.list[shortname] = currentEmoji; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/root/hooks/useEmojiOne.ts` around lines 23 - 31, Remove the redundant explicit type annotation on the forEach callback parameter: in useEmojiOne.ts inside the loop that iterates Object.entries(config.emojione.emojioneList) (where currentEmoji is used and currentEmoji.shortnames.forEach is called), drop the “: string” from the shortname parameter so TypeScript will infer the type from currentEmoji.shortnames; keep the rest of the population logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/root/hooks/useEmojiOne.ts`:
- Around line 23-31: Remove the redundant explicit type annotation on the
forEach callback parameter: in useEmojiOne.ts inside the loop that iterates
Object.entries(config.emojione.emojioneList) (where currentEmoji is used and
currentEmoji.shortnames.forEach is called), drop the “: string” from the
shortname parameter so TypeScript will infer the type from
currentEmoji.shortnames; keep the rest of the population logic unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/emoji-emojione/lib/emojiPicker.tsapps/meteor/client/views/root/hooks/useEmojiOne.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/emoji-emojione/lib/emojiPicker.ts
📜 Review details
🧰 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.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 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.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/client/views/root/hooks/useEmojiOne.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.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.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.ts
🔇 Additional comments (1)
apps/meteor/client/views/root/hooks/useEmojiOne.ts (1)
14-21: LGTM! Clean type-safe object literal initialization.This properly replaces the
as anyassertion with a well-structured object literal, maintaining type safety while achieving the PR objective.
NAME-ASHWANIYADAV
force-pushed
the
chore/fix-unsafe-type-assertion-useEmojiOne
branch
from
36865be to
870b337
Compare
Member
|
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 🙌 |
Contributor
Author
|
@engelgabriel Thanks for reviewing :) |
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
2 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.
Closes #39154
Replaced two
as anytype assertions in useEmojiOne.ts with proper type-safe alternatives.Changes
useEmojiOne.ts
config.emojione as anyassignment with a properly typed EmojiPackage object literalconfig.emojiCategories as any(no longer needed after source fix)if (emoji.packages.emojione)guard since the object is always constructedemojiPicker.ts
satisfies EmojiCategory[]annotation toemojiCategoriesarray to validatei18nvalues asTranslationKeyat the sourceScreenshots
No UI changes — internal type safety improvement only.
Summary by CodeRabbit