[WIKI-547] fix: update find suggestion logic for emoji extension#7411
[WIKI-547] fix: update find suggestion logic for emoji extension#7411
Conversation
WalkthroughThe changes introduce a custom suggestion matching function for emoji triggers in the editor, update the emoji suggestion plugin to use this function, and modify the emoji list component to render only when a query is present. The query string is now passed to the emoji list during suggestion events. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant EmojiExtension
participant EmojiList
User->>Editor: Types text with emoji trigger
Editor->>EmojiExtension: Detects trigger, invokes customFindSuggestionMatch
EmojiExtension->>Editor: Returns match details (if any)
Editor->>EmojiList: Renders EmojiList with query (if query is non-empty)
EmojiList-->>Editor: Renders suggestion UI or returns null if query is empty
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/emoji/emoji.ts(2 hunks)packages/editor/src/core/extensions/emoji/find-suggestion-match.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/editor/src/core/extensions/emoji/emoji.ts (1)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts (1)
customFindSuggestionMatch(19-107)
🔇 Additional comments (3)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts (2)
1-18: Well-structured type definitions and imports.The
Triggerinterface andSuggestionMatchtype provide clear contracts for the function's configuration and return value.
34-107: Excellent implementation addressing the core issue.The approach of processing the entire text block as a single piece effectively solves the problem where text decorations (bold/italic) would break emoji detection. The comprehensive validation checks and edge case handling ensure robust functionality.
packages/editor/src/core/extensions/emoji/emoji.ts (1)
18-18: Clean integration of the custom suggestion matcher.The integration is minimal and correctly replaces the default suggestion matching behavior with the custom implementation that handles text blocks holistically.
Also applies to: 347-347
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts (1)
58-64: Critical security issue: Regex injection vulnerability persists.The regex construction for prefix validation still contains the unescaped special characters issue flagged in the previous review. This could cause incorrect matching or runtime errors if
allowedPrefixescontains regex metacharacters.Apply this fix to properly escape the prefix characters:
// Check if the trigger character has an allowed prefix const prefixChar = lastTriggerIndex > 0 ? textBeforeCursor[lastTriggerIndex - 1] : "\0"; - const matchPrefixIsAllowed = new RegExp(`^[${allowedPrefixes?.join("")}\0]?$`).test(prefixChar); + const escapedPrefixes = allowedPrefixes?.map(p => escapeForRegEx(p)).join("") || ""; + const matchPrefixIsAllowed = new RegExp(`^[${escapedPrefixes}\0]?$`).test(prefixChar);
🧹 Nitpick comments (4)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts (4)
28-30: Complex regex construction needs validation.The regex patterns are quite complex and handle multiple scenarios. The construction logic could benefit from clearer documentation or refactoring for maintainability.
Consider breaking this into separate functions for better readability:
- const regexp = allowSpaces - ? new RegExp(`${prefix}${escapedChar}.*?(?=\\s${finalEscapedChar}|$)`, "gm") - : new RegExp(`${prefix}(?:^)?${escapedChar}[^\\s${finalEscapedChar}]*`, "gm"); + const regexp = allowSpaces + ? createSpacesAllowedRegex(prefix, escapedChar, finalEscapedChar) + : createNoSpacesRegex(prefix, escapedChar, finalEscapedChar);Then implement helper functions with clear documentation about what each pattern matches.
82-85: Edge case handling modifies match array directly.The code directly modifies the
match[0]array element, which could be confusing and potentially cause issues if the match object is used elsewhere.Consider creating a new variable instead of modifying the match array:
// Edge case handling; if spaces are allowed and we're directly in between // two triggers + let matchText = match[0]; + let matchLength = match[0].length; if (allowSpaces && suffix.test(blockText.slice(to - 1, to + 1))) { - match[0] += " "; + matchText += " "; + matchLength += 1; to += 1; }Then update the return statement to use
matchTextinstead ofmatch[0].
19-106: Function is quite long and handles multiple responsibilities.The
customFindSuggestionMatchfunction is doing a lot of work in a single function. Consider breaking it down into smaller, focused functions for better maintainability and testability.Consider refactoring into smaller functions:
function validatePosition($position: ResolvedPos): { blockText: string; relativePos: number; blockStart: number } | null { // Position validation logic } function findTriggerIndex(text: string, char: string): number { // Trigger finding logic } function validatePrefix(textBeforeCursor: string, triggerIndex: number, allowedPrefixes: string[] | null): boolean { // Prefix validation logic } function createMatch(/* parameters */): SuggestionMatch { // Match creation logic }This would make the main function more readable and each piece more testable.
91-93: Code block detection is appropriate but could be more robust.The code block detection only checks the parent type's spec.code property. Consider if additional context validation is needed for other restricted contexts.
You might want to add documentation about what other contexts should be excluded or consider making this configurable:
// Additional check: make sure we're not inside a code block or other restricted context const $from = $position.doc.resolve(from); - if ($from.parent.type.spec.code) { + if ($from.parent.type.spec.code || /* other restricted contexts */) { return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts(1 hunks)
🔇 Additional comments (3)
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts (3)
1-3: LGTM!Clean imports with necessary utilities from the tiptap library.
4-11: LGTM!Well-structured interface definition with clear property types and naming.
13-17: LGTM!Appropriate type alias for the function's return value with clear null handling.
packages/editor/src/core/extensions/emoji/find-suggestion-match.ts
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/emoji/components/emojis-list.tsx
Outdated
Show resolved
Hide resolved
aaryan610
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/editor/src/core/helpers/find-suggestion-match.ts (1)
40-42: Update outdated comment about JavaScript lookbehinds.The comment states that JavaScript doesn't have lookbehinds, but modern JavaScript (ES2018+) does support them. While the current implementation is fine and more compatible, the comment should be updated for accuracy.
Apply this diff to update the comment:
- // JavaScript doesn't have lookbehinds. This hacks a check that first character - // is a space or the start of the line + // Check that the first character before the match is a space or the start of the line + // This approach is more compatible than using lookbehinds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/emoji/emoji.ts(2 hunks)packages/editor/src/core/helpers/find-suggestion-match.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/emoji/emoji.ts
⏰ 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)
🔇 Additional comments (2)
packages/editor/src/core/helpers/find-suggestion-match.ts (2)
17-31: Excellent implementation of paragraph-level text extraction!This approach correctly addresses the core issue by extracting text from the entire paragraph using
textBetween, which handles text across different nodes/marks. This ensures that formatted text (bold, italic, etc.) doesn't break the emoji suggestion detection.
1-73: ✅ Integration Verified: customFindSuggestionMatch properly integrated in the emoji extension
In packages/editor/src/core/extensions/emoji/emoji.ts:
•customFindSuggestionMatchis imported from@/helpers/find-suggestion-match.
• It’s assigned tofindSuggestionMatchin the Suggestion configuration.In packages/editor/src/core/extensions/emoji/suggestion.ts:
• Thequeryprop is correctly passed through to theEmojiListitems renderer.All checks confirm the function is imported and used as intended.
* fix: update find suggestion logic * refactor: remove logs * refactor : make logic simpler * feat: check for one char to show suggestion * refactor : import types from extension * refactor: add early return * refactor : put custom suggestion in helper * fix : char * fix: types
3 participants
Description
Previously the logic looked at tiny pieces of text (one node at a time) which was breaking text on different decorator like bold and italic the updated logic looks at the whole text block as one piece now no matter how the text is decorated (bold, italic, etc.), the emoji suggestion will always find the colon and work correctly.
Type of Change
Screenshots and Media (if applicable)
Screen.Recording.2025-07-15.at.2.12.02.PM.mov
Screen.Recording.2025-07-15.at.8.44.27.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes