Conversation
📝 WalkthroughWalkthroughRefactors EmojiPicker from anchor-based to parent-controlled visibility (show/changeState/buttonRef) across multiple consumers; replaces ClickAwayListener with pointerdown handling. Poll maxChoices defaulting now uses logical OR to coerce falsy values to 1. GIF picker refetch moved to useEffect on filter changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Parent as Parent Component
participant EmojiPicker as EmojiPicker
participant DOM as Document/Viewport
rect rgb(220,240,200)
note right of Parent: NEW STATE-DRIVEN FLOW
User->>Parent: Click emoji button
Parent->>Parent: Toggle show state (show = true)
Parent->>EmojiPicker: Render with show=true, buttonRef, changeState
EmojiPicker->>DOM: Compute position from buttonRef, render at coords
User->>EmojiPicker: Select emoji
EmojiPicker->>Parent: onSelect(...) then changeState(false)
Parent->>Parent: Update show = false (unmount/hide)
alt Click outside
User->>DOM: pointerdown outside
EmojiPicker->>Parent: changeState(false)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)apps/web/src/features/ui/emoji-picker/index.tsx (1)
🔇 Additional comments (5)
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: 6
🧹 Nitpick comments (2)
apps/web/src/features/ui/emoji-picker/index.tsx (2)
99-105: Consider guarding render until position is calculated.When
showbecomestrue,pickerPositionis initiallynull, sotopandleftwill beundefinedfor one render frame. This may cause a brief visual flash before the picker moves to its calculated position.🔎 Optional improvement
+ // Don't render until position is calculated to avoid flash + if (!pickerPosition) { + return null; + } + const mergedStyle: CSSProperties = { position: "fixed", - top: pickerPosition?.top, - left: pickerPosition?.left, + top: pickerPosition.top, + left: pickerPosition.left, zIndex: 9999, ...style };
72-97: Addrefto the dependency array for completeness.The effect uses
ref.currentbut doesn't includerefin the dependency array. While refs are typically stable, including it satisfies the exhaustive-deps lint rule and makes the dependency explicit.🔎 Proposed fix
- }, [show, changeState]); + }, [show, changeState, ref, buttonRef]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/public/sw.jsapps/web/src/app/decks/_components/deck-settings/decks-settings.tsxapps/web/src/app/decks/_components/deck-threads-form/deck-threads-form-emoji-picker.tsxapps/web/src/app/publish/_components/publish-editor-toolbar.tsxapps/web/src/app/publish/_components/publish-gif-picker-dialog.tsxapps/web/src/features/chat/components/message-input.tsxapps/web/src/features/entry-management/entry-metadata-manager/entry-metadata-builder.tsapps/web/src/features/polls/components/poll-widget.tsxapps/web/src/features/polls/components/polls-creation.tsxapps/web/src/features/polls/hooks/use-entry-poll-extractor.tsapps/web/src/features/shared/editor-toolbar/index.tsxapps/web/src/features/ui/emoji-picker/index.tsxapps/web/src/features/ui/gif-picker/index.tsxapps/web/src/features/waves/components/wave-form/wave-form-emoji-picker.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
apps/web/src/features/waves/components/wave-form/wave-form-emoji-picker.tsx (2)
apps/web/src/features/ui/button/index.tsx (1)
Button(88-88)apps/web/src/features/ui/emoji-picker/index.tsx (1)
EmojiPicker(24-132)
apps/web/src/app/publish/_components/publish-editor-toolbar.tsx (2)
apps/web/src/features/ui/button/index.tsx (1)
Button(88-88)apps/web/src/features/ui/emoji-picker/index.tsx (1)
EmojiPicker(24-132)
apps/web/src/features/shared/editor-toolbar/index.tsx (2)
apps/web/src/features/ui/tooltip.tsx (1)
Tooltip(17-19)apps/web/src/features/ui/emoji-picker/index.tsx (1)
EmojiPicker(24-132)
apps/web/src/app/decks/_components/deck-threads-form/deck-threads-form-emoji-picker.tsx (2)
apps/web/src/features/ui/button/index.tsx (1)
Button(88-88)apps/web/src/features/ui/emoji-picker/index.tsx (1)
EmojiPicker(24-132)
apps/web/src/app/decks/_components/deck-settings/decks-settings.tsx (1)
apps/web/src/features/ui/emoji-picker/index.tsx (1)
EmojiPicker(24-132)
apps/web/src/features/ui/emoji-picker/index.tsx (1)
apps/web/src/core/global-store/index.ts (1)
useGlobalStore(13-22)
🔇 Additional comments (14)
apps/web/src/features/entry-management/entry-metadata-manager/entry-metadata-builder.ts (1)
175-175: LGTM! Behavioral change ensures minimum of 1 choice.The change from nullish coalescing (
??) to logical OR (||) now treats 0 as a falsy value, defaulting it to 1. This prevents polls with 0 maximum choices, which aligns with poll semantics and is consistent with validation changes in other files.apps/web/src/features/polls/components/polls-creation.tsx (2)
194-206: LGTM! Validation logic enforces minimum of 1 choice.The updated validation logic correctly enforces a minimum of 1 for
maxChoicesVoted, aligning with poll semantics and preventing invalid poll configurations. The min attribute and boundary checks are consistent.
304-304: LGTM! Consistent with other poll max choices changes.The change from
??to||ensures that falsy values (including 0) default to 1, consistent with the validation logic and changes inentry-metadata-builder.ts.apps/web/src/features/chat/components/message-input.tsx (3)
1-1: LGTM: State management setup.The addition of
useStateand theshowEmojiPickerstate properly support the refactored emoji picker pattern.Also applies to: 143-143
398-402: LGTM: Click handler properly toggles picker.The button click handler correctly prevents default behavior and propagation, then toggles the emoji picker visibility.
531-541: LGTM: EmojiPicker integration follows the new pattern.The conditional rendering and prop passing (show, changeState, onSelect, buttonRef, position) align with the state-driven EmojiPicker API. The
position="top"placement is appropriate for the bottom-anchored input.apps/web/src/app/decks/_components/deck-threads-form/deck-threads-form-emoji-picker.tsx (2)
1-1: LGTM: State and ref setup.The state management and ref declaration properly support the controlled emoji picker pattern.
Also applies to: 11-12
16-29: LGTM: Button and EmojiPicker refactor.The refactored implementation correctly:
- Forwards the ref to the Button
- Toggles visibility on click
- Conditionally renders EmojiPicker with the required props (show, changeState, onSelect, buttonRef)
apps/web/src/app/decks/_components/deck-settings/decks-settings.tsx (1)
128-137: LGTM: EmojiPicker integration updated correctly.The conditional rendering and prop passing follow the state-driven pattern. The
onSelecthandler correctly delegates visibility control tochangeStaterather than manually togglingshowEmoji.apps/web/src/app/publish/_components/publish-editor-toolbar.tsx (2)
124-124: LGTM: State and ref setup for emoji picker.The
emojiButtonRefandshowEmojiPickerstate declarations properly support the controlled emoji picker pattern.Also applies to: 140-140
339-357: LGTM: Emoji picker refactored to state-driven pattern.The implementation correctly:
- Wraps the button in a positioning container
- Attaches the ref to the button
- Toggles visibility on click
- Conditionally renders EmojiPicker with proper props (show, changeState, onSelect, buttonRef)
- Inserts selected emoji into the editor
apps/web/src/features/polls/components/poll-widget.tsx (1)
145-145: Remove this comment. The concern about||treating0as falsy does not apply here because the poll system explicitly validates thatmax_choices_votedmust be greater than 0 (see use-entry-poll-extractor.ts), and the UI input enforcesmin={1}. Zero is not a valid value in this system, making||the appropriate operator. There is no evidence of a recent change from??to||, and the current implementation is correct.Likely an incorrect or invalid review comment.
apps/web/src/features/shared/editor-toolbar/index.tsx (1)
86-94: LGTM on the state-driven emoji picker refactor.The transition from anchor-based to state-driven control aligns with the unified pattern being adopted across the codebase.
apps/web/src/features/ui/emoji-picker/index.tsx (1)
24-36: LGTM on the controlled visibility API.The refactor to a controlled
show/changeStatepattern is clean and aligns well with React's controlled component conventions. The internal ref fallback pattern on lines 34-35 is a nice touch for flexibility.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.