Conversation
Signed-off-by: Simo <simo@6529.io>
WalkthroughThe plugin now consults an emoji context and memoized custom IDs, validates emoji IDs via a callback, and only transforms valid matches into EmojiNodes while emitting plain text for invalid matches; effects and listeners were updated to use the new validation logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Editor
participant EmojiPlugin
participant EmojiContext
participant Transformer
User->>Editor: Type text (e.g. ":smile:")
Editor->>EmojiPlugin: onTextContentChange(text)
EmojiPlugin->>EmojiContext: read emojiMap, findNativeEmoji()
EmojiPlugin->>EmojiPlugin: compute customEmojiIds (useMemo)
EmojiPlugin->>EmojiPlugin: isEmojiIdValid(id) (useCallback)
EmojiPlugin->>Transformer: transformEmojiTextToNode(text, isEmojiIdValid)
Transformer->>Transformer: parse matches, build enriched match objects
alt emojiId valid
Transformer->>EmojiPlugin: emit EmojiNode
else emojiId invalid
Transformer->>EmojiPlugin: emit plain TextNode
end
EmojiPlugin-->>Editor: apply node changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (1)
118-124: Caret placed before the inserted space; set selection to end of trailing text.Placing the caret at offset 0 positions it before the space, so the next character appears before the space, creating a jarring UX. Set it to the end of the trailing text node.
Apply this diff:
- const cursorTextNodeKey = (cursorNode as TextNode).getKey(); - const newSelection = $createRangeSelection(); - newSelection.anchor.set(cursorTextNodeKey, 0, "text"); - newSelection.focus.set(cursorTextNodeKey, 0, "text"); + const cursorTextNodeKey = (cursorNode as TextNode).getKey(); + const newSelection = $createRangeSelection(); + const offset = (cursorNode as TextNode).getTextContent().length; + newSelection.anchor.set(cursorTextNodeKey, offset, "text"); + newSelection.focus.set(cursorTextNodeKey, offset, "text"); $setSelection(newSelection);
🧹 Nitpick comments (4)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (4)
86-96: Avoid inserting a trailing space unconditionally.Always adding " " changes document content (e.g., before punctuation). Only add it when the caret was inside the converted shortcode.
Apply this diff:
- const trailingTextNode = new TextNode(" "); - newNodes.push(trailingTextNode); + const addSpace = + anchorNodeKey === node.getKey() && + anchorOffset >= startIndex && + anchorOffset <= endIndex; + let trailingTextNode: TextNode | null = null; + if (addSpace) { + trailingTextNode = $createTextNode(" "); + newNodes.push(trailingTextNode); + }And keep the cursor assignment guarded:
- if ( + if ( anchorNodeKey === node.getKey() && anchorOffset >= startIndex && anchorOffset <= endIndex ) { - cursorNode = trailingTextNode; + if (trailingTextNode) cursorNode = trailingTextNode; }
44-51: Guard against RegExp lastIndex quirks by using a fresh regex instance.String.matchAll with a shared global regex can interact with lastIndex. Low risk, but easy to harden.
- const matches = Array.from(textContent.matchAll(EMOJI_MATCH_REGEX)).map( + const matches = Array.from( + textContent.matchAll(new RegExp(EMOJI_MATCH_REGEX, "g")) + ).map( (match) => ({ matchText: match[0], emojiId: match[1], startIndex: match.index!, endIndex: match.index! + match[0].length, }) );
17-19: Shortcode regex excludes common IDs with hyphens or plus signs.If your emoji IDs include dashes (e.g., :thumbs-up:) or plus signs, expand the character class.
-export const EMOJI_MATCH_REGEX = /:(\w+):/g; +export const EMOJI_MATCH_REGEX = /:([A-Za-z0-9_+-]+):/g; -const EMOJI_TEST_REGEX = /:(\w+)/; +const EMOJI_TEST_REGEX = /:([A-Za-z0-9_+-]+)/;
35-37: Reduce work by transforming only affected nodes.Scanning all text nodes on every content change is heavier than needed. Consider a TextNode transform or scoping to the current selection’s block.
- Option A: editor.registerNodeTransform(TextNode, ...), inspect node text once it changes.
- Option B: When triggered by typing, restrict search to the anchor’s text node instead of root.getAllTextNodes().
Also applies to: 154-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (2)
components/drops/create/lexical/nodes/EmojiNode.tsx (1)
EmojiNode(14-70)contexts/EmojiContext.tsx (1)
useEmoji(120-126)
⏰ 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: Analyze (javascript-typescript)
🔇 Additional comments (1)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (1)
129-152: Validation via EmojiContext looks solid.Memoized custom IDs and a simple native lookup keep the hot path cheap, and the callback deps are correct.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/components/drops/create/lexical/plugins/emoji/EmojiPlugin.extra.test.tsx(0 hunks)__tests__/components/drops/create/lexical/plugins/emoji/EmojiPlugin.test.tsx(1 hunks)components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts(6 hunks)
💤 Files with no reviewable changes (1)
- tests/components/drops/create/lexical/plugins/emoji/EmojiPlugin.extra.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (2)
components/drops/create/lexical/nodes/EmojiNode.tsx (1)
EmojiNode(14-70)contexts/EmojiContext.tsx (1)
useEmoji(120-126)
__tests__/components/drops/create/lexical/plugins/emoji/EmojiPlugin.test.tsx (2)
contexts/EmojiContext.tsx (1)
useEmoji(120-126)components/drops/create/lexical/plugins/emoji/EmojiPlugin.ts (1)
EMOJI_MATCH_REGEX(18-18)
| if (matches.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const hasValidEmoji = matches.some(({ emojiId }) => | ||
| isEmojiIdValid(emojiId) | ||
| ); | ||
|
|
||
| if (!hasValidEmoji) { | ||
| return; | ||
| } | ||
|
|
||
| let lastIndex = 0; | ||
| const newNodes: (TextNode | EmojiNode)[] = []; | ||
| let cursorNode: TextNode | null = null; | ||
|
|
||
| matches.forEach((match) => { | ||
| const emojiText = match[0]; | ||
| const emojiId = match[1]; | ||
| const startIndex = match.index!; | ||
| const endIndex = startIndex + emojiText.length; | ||
|
|
||
| for (const { matchText, emojiId, startIndex, endIndex } of matches) { | ||
| if (startIndex > lastIndex) { | ||
| const beforeStr = textContent.slice(lastIndex, startIndex); | ||
| if (beforeStr.length > 0) { | ||
| newNodes.push(new TextNode(beforeStr)); | ||
| } | ||
| } | ||
|
|
||
| if (!isEmojiIdValid(emojiId)) { | ||
| newNodes.push(new TextNode(matchText)); | ||
| lastIndex = endIndex; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't abort processing when encountering invalid emoji IDs
These new return statements exit transformEmojiTextToNode entirely as soon as the current text node lacks a valid emoji or the loop hits one invalid ID. In practice, a leading :foo: (unknown) completely prevents later :smile: matches—in the same node or subsequent nodes—from ever converting, so the colon-jump bug persists. We need to skip the offending segment/node but keep scanning the rest.
- if (matches.length === 0) {
- return;
- }
+ if (matches.length === 0) {
+ continue;
+ }
@@
- if (!hasValidEmoji) {
- return;
- }
+ if (!hasValidEmoji) {
+ continue;
+ }
@@
- if (!isEmojiIdValid(emojiId)) {
- newNodes.push(new TextNode(matchText));
- lastIndex = endIndex;
- return;
- }
+ if (!isEmojiIdValid(emojiId)) {
+ newNodes.push(new TextNode(matchText));
+ lastIndex = endIndex;
+ continue;
+ }Committable suggestion skipped: line range outside the PR's diff.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes