fix: use textContent instead of innerText for emoji span parsing#39059
fix: use textContent instead of innerText for emoji span parsing#39059dodaa08 wants to merge 3 commits into
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 |
|
WalkthroughThis change replaces Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/lib/utils/detectEmoji.ts (1)
13-13: Consider??over||for clarity, but no behavioral issues.The change from
innerTexttotextContentis sound. Since thedivis a detached (off-document) element,innerTextis unreliable — it attempts to compute CSS-based layout on elements not in the live DOM, which yields implementation-dependent results.textContentis the correct choice here and avoids unnecessary reflow.The
contentfield is rendered as JSX children in Fuselage emoji components (EmojiRenderer.tsx). Emoji content is typically a single Unicode character or short notation, so any whitespace differences between the two APIs are negligible in practice.One optional improvement: use
??instead of||for semantic precision —textContentreturnsnullonly for document/doctype nodes, never for element nodes, so the guard is redundant, but??is more semantically explicit about a null check:- content: span.textContent || '', + content: span.textContent ?? '',🤖 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` at line 13, Replace the fallback operator on the content field to use the nullish coalescing operator so the expression reads as a null check for span.textContent; update the line referencing span.textContent || '' to use ?? instead (the unique symbols to change are the content property assignment and the span.textContent expression in detectEmoji.ts) to make the intent explicit without changing behavior.
🤖 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/lib/utils/detectEmoji.ts`:
- Line 13: Replace the fallback operator on the content field to use the nullish
coalescing operator so the expression reads as a null check for
span.textContent; update the line referencing span.textContent || '' to use ??
instead (the unique symbols to change are the content property assignment and
the span.textContent expression in detectEmoji.ts) to make the intent explicit
without changing behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/lib/utils/detectEmoji.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/lib/utils/detectEmoji.ts
🧠 Learnings (1)
📚 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/lib/utils/detectEmoji.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39059 +/- ##
===========================================
- Coverage 70.68% 70.62% -0.07%
===========================================
Files 3190 3190
Lines 112732 112732
Branches 20448 20388 -60
===========================================
- Hits 79689 79615 -74
- Misses 30996 31065 +69
- Partials 2047 2052 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Proposed changes (including videos or screenshots)
Replaced innerText with textContent in detectEmoji when reading emoji span text. textContent avoids unnecessary layout reflow and better expresses the intent of just reading raw text content.
Steps to test or reproduce
Run the emoji autocomplete in the message input and verify emoji suggestions render correctly.
Further comments
The change is minimal and behavior remains identical in practice both innerText and textContent return the same value for emoji spans
Summary by CodeRabbit