fix: room list rendering wrong built-in emoji when a custom emoji shares its shortcode#7462
fix: room list rendering wrong built-in emoji when a custom emoji shares its shortcode#7462Rohit3523 wants to merge 4 commits into
Conversation
Rohit3523
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,jsx,ts,tsx,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/containers/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-04-30T17:07:51.020ZApplied to files:
📚 Learning: 2026-06-25T18:37:25.526ZApplied to files:
📚 Learning: 2026-06-25T18:37:44.793ZApplied to files:
🔇 Additional comments (2)
WalkthroughThe shortcode-to-emoji resolver now checks custom emojis from Redux state before built-in emoji mappings, so custom emoji names can override colliding shortcodes. Test mocks now pass mocked state through selectors, and coverage was added for the collision cases. ChangesEmoji shortcode precedence fix
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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.
🧹 Nitpick comments (1)
app/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts (1)
42-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup runs after the assertion, risking state leakage on failure.
If the
expecton Line 46 fails,setCustomEmojis({})on Line 47 never runs, leaving the custom emoji polluting later tests in this file. PreferafterEach/afterAll, as done inusePreviewFormatText.test.ts.🧹 Proposed fix using afterEach
-test('do NOT resolve a shortcode that collides with a custom emoji name', () => { - mockedStore.dispatch(setCustomEmojis({ no: { name: 'no', extension: 'png' } })); - const { formatShortnameToUnicode } = useShortnameToUnicode(); - const unicodeEmoji = formatShortnameToUnicode(':no:'); - expect(unicodeEmoji).toBe(':no:'); - mockedStore.dispatch(setCustomEmojis({})); -}); +describe('shortcode collides with a custom emoji name', () => { + beforeAll(() => { + mockedStore.dispatch(setCustomEmojis({ no: { name: 'no', extension: 'png' } })); + }); + + afterAll(() => { + mockedStore.dispatch(setCustomEmojis({})); + }); + + test('do NOT resolve a shortcode that collides with a custom emoji name', () => { + const { formatShortnameToUnicode } = useShortnameToUnicode(); + const unicodeEmoji = formatShortnameToUnicode(':no:'); + expect(unicodeEmoji).toBe(':no:'); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts` around lines 42 - 48, The test in useShortnameToUnicode.test.ts leaves cleanup after the assertion, so a failing expect can leak custom emoji state into later tests. Move the reset logic for mockedStore.dispatch(setCustomEmojis({})) into a shared afterEach (or afterAll if appropriate), matching the cleanup pattern used in usePreviewFormatText.test.ts, and keep the test body focused on the formatShortnameToUnicode behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts`:
- Around line 42-48: The test in useShortnameToUnicode.test.ts leaves cleanup
after the assertion, so a failing expect can leak custom emoji state into later
tests. Move the reset logic for mockedStore.dispatch(setCustomEmojis({})) into a
shared afterEach (or afterAll if appropriate), matching the cleanup pattern used
in usePreviewFormatText.test.ts, and keep the test body focused on the
formatShortnameToUnicode behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45835e7a-e205-49e5-8ecf-7c7886a21945
📒 Files selected for processing (3)
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsxapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsx
🧠 Learnings (3)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.tsapp/lib/hooks/useShortnameToUnicode/index.tsx
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.tsapp/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/lib/hooks/useShortnameToUnicode/index.tsx
🔇 Additional comments (4)
app/lib/hooks/useShortnameToUnicode/index.tsx (1)
31-63: LGTM!app/lib/hooks/useShortnameToUnicode/useShortnameToUnicode.test.ts (2)
3-7: LGTM!
36-40: LGTM!app/lib/hooks/usePreviewFormatText/usePreviewFormatText.test.ts (1)
3-7: LGTM!Also applies to: 130-144
|
|
||
| const useShortnameToUnicode = (isEmojiPicker?: boolean) => { | ||
| const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji); | ||
| const customEmojis = useAppSelector(state => state.customEmojis); |
There was a problem hiding this comment.
We had customers with 20k custom emojis. You don't want to import them all here.
| const useShortnameToUnicode = (isEmojiPicker?: boolean) => { | ||
| const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji); | ||
| const customEmojis = useAppSelector(state => state.customEmojis); | ||
| const replaceShortNameWithUnicode = (shortname: string) => { |
There was a problem hiding this comment.
Missing consistency with the other places calling the domain
| const replaceShortNameWithUnicode = (shortname: string) => { | |
| const replaceShortnameWithUnicode = (shortname: string) => { |
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
2 participants
Proposed changes
Fixes the room list's last-message preview showing the wrong built-in emoji when a workspace has a custom emoji whose name collides with a built-in shortcode alias (e.g. a custom emoji named
nocolliding with:no:, an alias of the Norway flag). In-channel messages already prioritize custom emoji correctly; the room list preview did not check for custom emoji before resolving shortcodes, so it always fell back to the built-in table.useShortnameToUnicodenow checksstate.customEmojisfirst and leaves the shortcode as literal text when a custom emoji shares that name.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1374
How to test or reproduce
no(or any name colliding with a built-in shortcode alias).:no:in any channel.:no:as literal text, matching the in-channel view.Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
:no:) so they no longer resolve to an unintended default emoji when a matching custom emoji exists.Tests