-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(ui): correct emoji extraction logic using regex #12749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
fix(ui): correct emoji extraction logic using regex #12749
Conversation
…dentify and return emoji from the input string test(extract-emoji-icon): update test case to reflect the correct emoji extraction behavior and expected output
…con function to ensure correct emoji extraction and handling of plain text
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
…to-be-used-as-document-icons
Warning Rate limit exceeded@NorkzYT has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes expand the test coverage for the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant extractEmojiIcon
Caller->>extractEmojiIcon: Call with input string
extractEmojiIcon->>extractEmojiIcon: Reset regex index
extractEmojiIcon->>extractEmojiIcon: Execute emoji regex on input
alt Match at start of string
extractEmojiIcon->>extractEmojiIcon: Find emoji cluster boundary
extractEmojiIcon->>Caller: Return {emoji, rest}
else No match at start
extractEmojiIcon->>Caller: Return {emoji: null, rest: input}
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/frontend/core/src/utils/__tests__/extract-emoji-icon.spec.ts (1)
5-30
: Consider adding edge case tests for the broader regex pattern.While the current test coverage is good, consider adding tests for Unicode characters that might be falsely matched by the broad regex pattern to ensure the function behaves correctly.
Add these additional test cases to verify edge cases:
+ expect(extractEmojiIcon('©️test')).toEqual({ + emoji: '©️', + rest: 'test', + }); + + expect(extractEmojiIcon('®️test')).toEqual({ + emoji: '®️', + rest: 'test', + }); + + // Test potential false positives from broad Unicode range + expect(extractEmojiIcon('†test')).toEqual({ + emoji: null, + rest: '†test', + }); + + expect(extractEmojiIcon('√test')).toEqual({ + emoji: null, + rest: '√test', + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/frontend/core/src/utils/__tests__/extract-emoji-icon.spec.ts
(1 hunks)packages/frontend/core/src/utils/extract-emoji-icon.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/frontend/core/src/utils/__tests__/extract-emoji-icon.spec.ts (1)
packages/frontend/core/src/utils/extract-emoji-icon.ts (1)
extractEmojiIcon
(4-19)
🔇 Additional comments (3)
packages/frontend/core/src/utils/extract-emoji-icon.ts (1)
11-12
: Good choice usingslice()
oversubstring()
.Using
slice()
is the modern preferred approach oversubstring()
as it has more predictable behavior with negative indices and is generally more intuitive.packages/frontend/core/src/utils/__tests__/extract-emoji-icon.spec.ts (2)
11-24
: Excellent test coverage for emoji extraction scenarios.The new test cases effectively cover:
- Variation selector emojis (❤️, ➡️,
✈️ ) with trailing text- Different emoji categories (heart, arrow, transport)
- Proper separation of emoji from following content
26-28
: Good negative test case update.The updated test case with 'plain text' clearly validates that non-emoji strings are handled correctly, ensuring no false positive extraction occurs.
…to-be-used-as-document-icons
…or better readability
…to-be-used-as-document-icons
fix.ui.-correct.emoji-extraction-logic-using-regex.mp4
This PR solves the issue where a majority of emoji's are unable to become the document or folders icon.
The regex used is below with the test string of a variety of emoji's:
https://regex101.com/r/0anB6Z/1
Summary by CodeRabbit
Bug Fixes
Tests