-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[WIKI-823] fix: callout element emoji search #8201
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
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughPopover event propagation is blocked inside the emoji picker panel and the emoji search input now automatically receives focus when the picker mounts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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). (2)
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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
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.
Pull request overview
This PR aims to fix issues with the emoji search functionality in the callout element emoji picker by preventing unwanted event propagation and ensuring the search input is always enabled and focused when the picker opens.
Key Changes
- Added auto-focus logic to the emoji search input using
useEffectand DOM manipulation - Added event propagation prevention (
stopPropagation) for mouseDown, click, focus, and keyDown events on the emoji picker popover panel - Added a ref to the search wrapper div to enable DOM querying for the input element
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/propel/src/emoji-icon-picker/emoji/emoji.tsx | Adds useEffect hook with DOM manipulation to force-enable and focus the search input, includes leading whitespace formatting issue |
| packages/propel/src/emoji-icon-picker/emoji-picker.tsx | Adds broad event propagation prevention on the popover panel for multiple event types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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: 1
🧹 Nitpick comments (1)
packages/propel/src/emoji-icon-picker/emoji/emoji.tsx (1)
18-22: Consider using a more robust ref pattern for input access.The
querySelector("input")approach is fragile and tightly couples this component to the internal DOM structure ofEmojiPicker.Searchfrom the frimousse library. If the library updates its component structure, this selector could break.Recommended approach:
If the frimousse library exposes a ref forwarding mechanism for
EmojiPicker.Search, use it directly:const inputRef = useRef<HTMLInputElement>(null); useEffect(() => { if (inputRef.current && !searchDisabled) { inputRef.current.focus(); } }, [searchDisabled]); // In JSX <EmojiPicker.Search ref={inputRef} placeholder={searchPlaceholder} disabled={searchDisabled} className="..." />If the library doesn't support ref forwarding, the current querySelector approach is acceptable but document the dependency on the library's internal structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/propel/src/emoji-icon-picker/emoji-picker.tsx(1 hunks)packages/propel/src/emoji-icon-picker/emoji/emoji.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/propel/src/emoji-icon-picker/emoji-picker.tsxpackages/propel/src/emoji-icon-picker/emoji/emoji.tsx
🧬 Code graph analysis (1)
packages/propel/src/emoji-icon-picker/emoji/emoji.tsx (1)
packages/propel/src/emoji-icon-picker/emoji-picker.tsx (1)
EmojiPicker(11-142)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/propel/src/emoji-icon-picker/emoji-picker.tsx (1)
111-114: LGTM! Event propagation control implemented correctly.The event handlers effectively isolate the emoji picker panel by preventing mouse, click, focus, and keyboard events from bubbling to parent components. This ensures interactions within the panel remain contained.
* fix: emoji search input * fix: enhance keyboard navigation in EmojiPicker component
* fix: emoji search input * fix: enhance keyboard navigation in EmojiPicker component
* fix: emoji search input * fix: enhance keyboard navigation in EmojiPicker component
Description
this PR preventing unwanted event propagation and ensuring the search input is always enabled and focused.
Type of Change
Screenshots and Media (if applicable)
Screen.Recording.2025-11-30.at.5.24.57.PM.mov
Test Scenarios
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.