feat(reply): 表情回应支持 Emoji#130
Conversation
📝 WalkthroughWalkthroughThis PR enhances the reply feature to accept multiple emoji and face ID targets in a single command. It introduces two emoji conversion utilities— ChangesEmoji Reply Feature Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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.
Inline comments:
In `@src/features/reply/feature.ts`:
- Around line 66-68: The loop that calls addReplyFace per item can leave partial
state on failure; change to perform the work atomically by adding a batch API
and using a transaction: implement a new bulkAddReplyFaces(userId, faceIds) (and
bulkRemoveReplyFaces for the removal case) that performs the inserts/deletes
inside a single DB transaction/atomic operation, then replace the per-item
awaited loops in feature.ts (the for...await loops that call addReplyFace) with
a single call to the new bulk function; ensure the bulk functions return a clear
error on failure and roll back the transaction so no partial writes remain.
- Around line 58-60: The emoji extraction using
message.content.match(/\p{Emoji_Presentation}/gu) is missing sequences with
variation selectors and ZWJ joins; update the regex used where emojis and
emojiIds are built (the emojis variable and its mapping to emojiIds that call
emojiToFaceId) to match full emoji sequences instead, e.g. use a regex based on
\p{Extended_Pictographic} that captures optional U+FE0F and
ZWJ+Extended_Pictographic runs (conceptually:
/\p{Extended_Pictographic}(?:\uFE0F|\u200D\p{Extended_Pictographic})*/gu), and
apply the same replacement at the other site that builds emojis/emojiIds so that
emojiToFaceId receives whole emoji graphemes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6c0a524-cb69-4199-8833-f327a71f4b4e
📒 Files selected for processing (3)
.narrafork/plan-flickering-foraging-matsumoto.mdsrc/features/reply/feature.tssrc/utils/index.ts
| const emojis = | ||
| message.content.match(/\p{Emoji_Presentation}/gu) || []; | ||
| const emojiIds = emojis.map((emoji) => `e:${emojiToFaceId(emoji)}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript Unicode property escapes, what does \p{Emoji_Presentation} exclude compared with \p{Extended_Pictographic}, and does it match emojis like "❤️" and ZWJ sequences such as "👨👩👧👦"?
💡 Result:
The \p{Emoji_Presentation} property and \p{Extended_Pictographic} property serve different purposes within the Unicode Standard, with \p{Extended_Pictographic} being a significantly broader superset [1]. \p{Emoji_Presentation} matches only those characters that are rendered as colorful emoji by default, without needing a variation selector (like U+FE0F) [1][2]. In contrast, \p{Extended_Pictographic} includes \p{Emoji_Presentation} and adds several other categories [1]: 1. Characters that are pictographic in nature but default to text presentation (and require a variation selector to render as emoji, such as many symbol characters) [3][1][4]. 2. Reserved code points within emoji blocks, intended for future emoji, which allows regex patterns using this property to be more future-proof [1]. 3. Other miscellaneous pictographic symbols that do not have the Emoji_Presentation property [3][1]. Regarding your specific examples: "❤️" (U+2764 U+FE0F) The sequence "❤️" generally matches \p{Emoji_Presentation} only if the engine considers the base character "❤" (U+2764) to have the property. However, U+2764 actually has Emoji_Presentation=No, meaning it defaults to text presentation [5][4]. When combined with the variation selector U+FE0F, it becomes an emoji presentation sequence [2][4]. Because \p{Emoji_Presentation} is a binary property of individual code points, it does not match this sequence as a whole; it matches the individual base character only if that character is marked as Emoji_Presentation=Yes. Consequently, \p{Emoji_Presentation} is typically insufficient for matching full emoji sequences [6][7]. ZWJ sequences (e.g., "👨👩👧👦") These sequences are composed of multiple code points joined by the Zero Width Joiner (U+200D) [7][8]. \p{Emoji_Presentation} is a property of individual code points, not sequences [1][6]. Therefore, it will not match a ZWJ sequence as a single match; it will only match the individual code points within the sequence that happen to have the Emoji_Presentation property [7]. To match complex ZWJ sequences or emoji presentation sequences correctly, standard regex property escapes are generally inadequate, and dedicated libraries like emoji-regex are recommended [6][7][9].
Citations:
- 1: https://emojifyi.com/stories/unicode-emoji-properties/
- 2: https://www.unicode.org/L2/L2025/25107-uts51-28-update-pri518.pdf
- 3: https://stackoverflow.com/questions/70401560/what-is-the-difference-between-emoji-presentation-and-extended-pictographic
- 4: https://emojifyi.com/stories/text-vs-emoji-presentation/
- 5: https://unicode.org/Public/UCD/latest/ucd/emoji/emoji-data.txt
- 6: https://mathiasbynens.be/notes/es-unicode-property-escapes
- 7: https://www.stefanjudis.com/snippets/how-to-detect-emojis-in-javascript-strings/
- 8: https://www.unicode.org/L2/L2022/22176r-uts51-22-draft-pri454.pdf
- 9: https://github.com/mathiasbynens/emoji-regex
Fix emoji extraction to handle variation-selector and ZWJ emoji sequences
The current /\p{Emoji_Presentation}/gu match is limited to code points with default emoji presentation, so it can miss emojis like "❤️" (❤ + FE0F) and won’t treat ZWJ emojis (e.g., "👨👩👧👦") as single units—causing missed/fragmented emoji IDs in the on/off logic (lines 58-60; also 106-108).
💡 Suggested fix
-const emojis = message.content.match(/\p{Emoji_Presentation}/gu) || [];
+const emojis =
+ message.content.match(
+ /\p{Extended_Pictographic}(?:\uFE0F|\uFE0E)?(?:\u200D\p{Extended_Pictographic}(?:\uFE0F|\uFE0E)?)*/gu
+ ) ?? [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const emojis = | |
| message.content.match(/\p{Emoji_Presentation}/gu) || []; | |
| const emojiIds = emojis.map((emoji) => `e:${emojiToFaceId(emoji)}`); | |
| const emojis = | |
| message.content.match( | |
| /\p{Extended_Pictographic}(?:\uFE0F|\uFE0E)?(?:\u200D\p{Extended_Pictographic}(?:\uFE0F|\uFE0E)?)*/gu | |
| ) ?? []; | |
| const emojiIds = emojis.map((emoji) => `e:${emojiToFaceId(emoji)}`); |
🤖 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 `@src/features/reply/feature.ts` around lines 58 - 60, The emoji extraction
using message.content.match(/\p{Emoji_Presentation}/gu) is missing sequences
with variation selectors and ZWJ joins; update the regex used where emojis and
emojiIds are built (the emojis variable and its mapping to emojiIds that call
emojiToFaceId) to match full emoji sequences instead, e.g. use a regex based on
\p{Extended_Pictographic} that captures optional U+FE0F and
ZWJ+Extended_Pictographic runs (conceptually:
/\p{Extended_Pictographic}(?:\uFE0F|\u200D\p{Extended_Pictographic})*/gu), and
apply the same replacement at the other site that builds emojis/emojiIds so that
emojiToFaceId receives whole emoji graphemes.
| for (const faceId of finalFaceIds) { | ||
| await addReplyFace(message.userId, faceId); | ||
| } |
There was a problem hiding this comment.
Batch on/off can leave partial persisted state on mid-loop failure.
These per-item awaited writes are non-atomic; if one write fails, earlier inserts/deletes stay committed, producing inconsistent final state versus the failure response.
💡 Suggested direction
- for (const faceId of finalFaceIds) {
- await addReplyFace(message.userId, faceId);
- }
+ await addReplyFacesBatch(message.userId, finalFaceIds);
- for (const faceId of finalFaceIds) {
- await removeReplyFace(message.userId, faceId);
- }
+ await removeReplyFacesBatch(message.userId, finalFaceIds);Also applies to: 114-116
🤖 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 `@src/features/reply/feature.ts` around lines 66 - 68, The loop that calls
addReplyFace per item can leave partial state on failure; change to perform the
work atomically by adding a batch API and using a transaction: implement a new
bulkAddReplyFaces(userId, faceIds) (and bulkRemoveReplyFaces for the removal
case) that performs the inserts/deletes inside a single DB transaction/atomic
operation, then replace the per-item awaited loops in feature.ts (the
for...await loops that call addReplyFace) with a single call to the new bulk
function; ensure the bulk functions return a clear error on failure and roll
back the transaction so no partial writes remain.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements