[WIKI-623] fix: add block menu to rich text editor #7813
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. WalkthroughIntegrates a new BlockMenu into the RichText editor render path and refactors BlockMenu to use Floating UI with a virtual anchor from the drag handle. Adds open/close/animation state, outside/escape/scroll dismiss, updates Delete and Duplicate flows, and extends BlockMenu props with flaggedExtensions and disabledExtensions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant DragHandle as Drag Handle (DOM)
participant BlockMenu as BlockMenu (Floating UI)
participant Editor as Editor (TipTap)
User->>DragHandle: Click drag handle
DragHandle-->>BlockMenu: supply bounding rect (virtual ref)
BlockMenu->>BlockMenu: set open, compute position via Floating UI
BlockMenu->>User: render floating menu
alt User selects "Delete"
User->>BlockMenu: Click Delete
BlockMenu->>Editor: chain().deleteSelection().focus().run()
Editor-->>BlockMenu: operation complete
BlockMenu->>BlockMenu: close menu
else User selects "Duplicate"
User->>BlockMenu: Click Duplicate
BlockMenu->>Editor: insert content at selection end (with checks)
Editor-->>BlockMenu: operation complete
BlockMenu->>BlockMenu: close menu
end
rect rgba(230,240,255,0.25)
note over BlockMenu,User: Menu dismissed on Escape, outside click, or scroll
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Pull Request Overview
This PR updates the block menu functionality in the rich text editor by migrating from Tippy.js to Floating UI for better positioning and interactions. It also adds the block menu component to the rich text editor.
- Replaces Tippy.js implementation with Floating UI for improved positioning and animations
- Adds block menu functionality to the rich text editor component
- Implements smooth animations and better event handling for the menu
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/editor/src/core/components/menus/block-menu.tsx | Complete rewrite to use Floating UI instead of Tippy.js with improved animations and event handling |
| packages/editor/src/core/components/editors/rich-text/editor.tsx | Adds BlockMenu component to the rich text editor |
Comments suppressed due to low confidence (1)
packages/editor/src/core/components/menus/block-menu.tsx:1
- The animation logic sets
isAnimatedInto false and then immediately to true within a single useEffect. This creates unnecessary state updates. Consider using a single boolean state or restructuring the animation logic to avoid the redundant state change.
import {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const timeout = setTimeout(() => { | ||
| requestAnimationFrame(() => { | ||
| setIsAnimatedIn(true); | ||
| }); | ||
| }, 50); |
There was a problem hiding this comment.
The magic number 50 for the animation delay should be extracted to a named constant to improve code maintainability and make the timing configurable.
Copilot uses AI. Check for mistakes.
packages/editor/src/core/components/editors/rich-text/editor.tsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/editor/src/core/components/menus/block-menu.tsx (1)
141-144: Fix crash: selection.content() can throw on empty selectionsComputing
isDisabledby callingselection.content()during render can throw when the selection is empty, crashing the menu. Guard for empty selections and errors; compute lazily.Apply:
- isDisabled: - editor.state.selection.content().content.firstChild?.type.name === CORE_EXTENSIONS.IMAGE || - editor.isActive(CORE_EXTENSIONS.CUSTOM_IMAGE), + isDisabled: (() => { + const { state } = editor; + const sel = state.selection; + if (sel.empty) return true; // nothing to duplicate + try { + const slice = sel.content(); + const node = slice.content.firstChild as any; + const typeName = node?.type?.name; + return typeName === CORE_EXTENSIONS.IMAGE || editor.isActive(CORE_EXTENSIONS.CUSTOM_IMAGE); + } catch { + return true; + } + })(),
♻️ Duplicate comments (2)
packages/editor/src/core/components/menus/block-menu.tsx (1)
99-110: Extract magic number 50ms to a named constantMove the animation delay to a constant for clarity and reuse.
Apply:
-import { cn } from "@plane/utils"; +import { cn } from "@plane/utils"; + +const MENU_ANIMATION_DELAY_MS = 50;and
- }, 50); + }, MENU_ANIMATION_DELAY_MS);packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
44-47: Guard BlockMenu until editor is ready (prevents null access)
BlockMenureceiveseditoreven when it can be null during init, causing runtime errors inside the menu.Apply:
- <> - {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />} - <BlockMenu editor={editor} flaggedExtensions={flaggedExtensions} disabledExtensions={disabledExtensions} /> - </> + <> + {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />} + {editor && ( + <BlockMenu editor={editor} flaggedExtensions={flaggedExtensions} disabledExtensions={disabledExtensions} /> + )} + </>
🧹 Nitpick comments (7)
packages/editor/src/core/components/menus/block-menu.tsx (6)
148-176: Harden duplicate logic: fragile position/size math and empty selection
- Using
selection.toanddoc.content.sizeis brittle; preferstate.doc.nodeSize - 2for upper bound or rely on TipTap to resolve.- Add an explicit guard for empty selections to avoid runtime errors.
- Focusing to
insertPos + 1may land mid-node; let TipTap set selection or compute from inserted slice size.Apply:
- try { + try { const { state } = editor; const { selection } = state; - const firstChild = selection.content().content.firstChild; - const docSize = state.doc.content.size; + if (selection.empty) { + throw new Error("Nothing selected to duplicate."); + } + const slice = selection.content(); + const firstChild = slice.content.firstChild; + const docMaxPos = state.doc.nodeSize - 2; // valid range: [0, docMaxPos] if (!firstChild) { throw new Error("No content selected or content is not duplicable."); } // Directly use selection.to as the insertion position const insertPos = selection.to; // Ensure the insertion position is within the document's bounds - if (insertPos < 0 || insertPos > docSize) { + if (insertPos < 0 || insertPos > docMaxPos) { throw new Error("The insertion position is invalid or outside the document."); } - const contentToInsert = firstChild.toJSON(); + const contentToInsert = firstChild.toJSON(); // keep duplicating first node of the slice // Insert the content at the calculated position editor .chain() .insertContentAt(insertPos, contentToInsert, { updateSelection: true, }) - .focus(Math.min(insertPos + 1, docSize), { scrollIntoView: false }) + .focus(undefined, { scrollIntoView: false }) .run();Please verify with node selections, text selections, and table/cell selections.
47-66: Avoid non-unique #id selectors for multiple drag handles
closest("#drag-handle")assumes a unique id and can conflict when multiple blocks exist. Prefer a data attribute or class.Apply:
- const dragHandle = target.closest("#drag-handle"); + const dragHandle = target.closest('[data-drag-handle="true"]');Remember to update the handle element markup.
68-72: Redundant outside-click handling (double-dismiss risk)You already use
useDismiss(context). The manual outside click close duplicates behavior and can cause flicker/races.Apply:
- // If clicking outside and not on a menu item, hide the menu - if (menuRef.current && !menuRef.current.contains(target)) { - setIsOpen(false); - }If needed, configure
useDismiss({ outsidePress: true })and rely on it.Also applies to: 43-45
197-206: Conflicting z-index: inline style vs utility class
style={{ zIndex: 99 }}andclassName="z-20 ..."conflict. Keep one source of truth.Apply:
- className={cn( - "z-20 max-h-60 min-w-[7rem] overflow-y-scroll rounded-lg border border-custom-border-200 bg-custom-background-100 p-1.5 shadow-custom-shadow-rg", + className={cn( + "max-h-60 min-w-[7rem] overflow-y-scroll rounded-lg border border-custom-border-200 bg-custom-background-100 p-1.5 shadow-custom-shadow-rg",Also applies to: 202-206
191-209: Add minimal ARIA roles for accessibilityAdvertise semantics for assistive tech.
Apply:
- <div + <div + role="menu" + aria-label="Block actions" ... > - {MENU_ITEMS.map((item) => { + {MENU_ITEMS.map((item) => { ... - <button + <button + role="menuitem" + aria-disabled={!!item.isDisabled} ... >Also applies to: 215-224
25-33: Props not used (flaggedExtensions, disabledExtensions)These props are accepted but unused here. Remove or wire them up to item visibility.
Do you plan to gate items by these props? If not, drop them from
Propsand call sites.packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
4-4: Optional: drop unused props to BlockMenu if not consumedIf
flaggedExtensions/disabledExtensionsaren’t used, omit them here too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/components/editors/rich-text/editor.tsx(2 hunks)packages/editor/src/core/components/menus/block-menu.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/editor/src/core/components/editors/rich-text/editor.tsx (2)
packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
EditorBubbleMenu(52-223)packages/editor/src/core/components/menus/block-menu.tsx (1)
BlockMenu(25-230)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/editor/src/core/components/menus/block-menu.tsx (1)
99-115: Extract animation delay constant (repeat from prior review)The 50ms delay is a magic number. Promote to a named constant.
- }, 50); + }, ANIMATION_DELAY_MS);Outside this hunk, define once near the top:
const ANIMATION_DELAY_MS = 50;packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
43-48: Guard BlockMenu until editor is ready (prevents null deref at runtime)Same issue noted previously: BlockMenu renders even when editor can be null during init. Guard it like BubbleMenu.
- {(editor) => ( - <> - {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />} - <BlockMenu editor={editor} flaggedExtensions={flaggedExtensions} disabledExtensions={disabledExtensions} /> - </> - )} + {(editor) => ( + <> + {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />} + {editor && <BlockMenu editor={editor} />} + </> + )}
🧹 Nitpick comments (4)
packages/editor/src/core/components/menus/block-menu.tsx (3)
35-41: Prefer fixed positioning to avoid transform/scroll jitterAdd strategy: "fixed" to stabilize the floating panel within transformed/scrollable containers.
const { refs, floatingStyles, context } = useFloating({ open: isOpen, onOpenChange: setIsOpen, middleware: [offset({ crossAxis: -10 }), flip(), shift()], whileElementsMounted: autoUpdate, placement: "left-start", + strategy: "fixed", });
76-97: Use passive scroll listenerMinor perf win and prevents scroll handler from blocking.
- document.addEventListener("scroll", handleScroll, true); // Using capture phase + document.addEventListener("scroll", handleScroll, { capture: true, passive: true }); // capture + passive
191-201: Tidy z-index and scrolling; add ARIA roles
- Drop class z-20 since inline zIndex: 99 already applies.
- Prefer overflow-y-auto.
- Add role="menu" and role="menuitem" for a11y.
- <div + <div ref={(node) => { refs.setFloating(node); menuRef.current = node; }} style={{ ...floatingStyles, zIndex: 99, animationFillMode: "forwards", transitionTimingFunction: "cubic-bezier(0.16, 1, 0.3, 1)", // Expo ease out }} - className={cn( - "z-20 max-h-60 min-w-[7rem] overflow-y-scroll rounded-lg border border-custom-border-200 bg-custom-background-100 p-1.5 shadow-custom-shadow-rg", + className={cn( + "max-h-60 min-w-[7rem] overflow-y-auto rounded-lg border border-custom-border-200 bg-custom-background-100 p-1.5 shadow-custom-shadow-rg", "transition-all duration-300 transform origin-top-right", isAnimatedIn ? "opacity-100 scale-100" : "opacity-0 scale-75" )} + role="menu" data-prevent-outside-click {...getFloatingProps()} > - {MENU_ITEMS.map((item) => { + {MENU_ITEMS.map((item) => { if (item.isDisabled) { return null; } return ( <button key={item.key} type="button" className="flex w-full items-center gap-1.5 truncate rounded px-1 py-1.5 text-xs text-custom-text-200 hover:bg-custom-background-90" onClick={item.onClick} disabled={item.isDisabled} + role="menuitem" >Also applies to: 203-206
packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
46-46: Avoid passing unused props to BlockMenuflaggedExtensions/disabledExtensions aren’t used inside BlockMenu. Drop them to reduce API surface and churn.
- {editor && <BlockMenu editor={editor} flaggedExtensions={flaggedExtensions} disabledExtensions={disabledExtensions} />} + {editor && <BlockMenu editor={editor} />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/components/editors/rich-text/editor.tsx(2 hunks)packages/editor/src/core/components/menus/block-menu.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/editor/src/core/components/editors/rich-text/editor.tsx (2)
packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
EditorBubbleMenu(52-223)packages/editor/src/core/components/menus/block-menu.tsx (1)
BlockMenu(25-230)
⏰ 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: Build and lint web apps
🔇 Additional comments (1)
packages/editor/src/core/components/menus/block-menu.tsx (1)
116-136: Delete command should be robust to empty TextSelectionIf selection is empty, deleteSelection no-ops. After setting NodeSelection on handle click (see above), this will be fine. Please verify behavior for both NodeSelection and TextSelection.
VipinDevelops
force-pushed
the
fix-block_menu_options_issue_view
branch
from
bb18f82 to
d76bfdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/editor/src/core/components/menus/block-menu.tsx (2)
219-235: Consider accessibility improvements for the menu items.While the current implementation is functional, consider adding ARIA attributes and keyboard navigation for better accessibility.
Consider these accessibility enhancements:
<button key={item.key} type="button" + role="menuitem" + tabIndex={-1} className="flex w-full items-center gap-1.5 truncate rounded px-1 py-1.5 text-xs text-custom-text-200 hover:bg-custom-background-90" onClick={item.onClick} disabled={item.isDisabled} >And add keyboard navigation handling to the menu container:
<div + role="menu" + tabIndex={0} ref={(node) => { refs.setFloating(node); menuRef.current = node; }}
47-83: Approve — block selection logic is correct; add missing callback dependencyVirtual reference + posAtCoords → $pos.before($pos.depth) → setNodeSelection correctly targets the block. Update handleClickDragHandle's useCallback deps to include editor (e.g., useCallback(..., [refs, editor])) to avoid a stale closure. Location: packages/editor/src/core/components/menus/block-menu.tsx — handleClickDragHandle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/components/editors/rich-text/editor.tsx(2 hunks)packages/editor/src/core/components/menus/block-menu.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/components/editors/rich-text/editor.tsx
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (10)
packages/editor/src/core/components/menus/block-menu.tsx (10)
1-15: LGTM! Well-organized imports for the Floating UI migration.The imports are properly organized and include all necessary dependencies for the new Floating UI-based implementation. The addition of
cnutility for class name management is appropriate.
19-23: LGTM! Props interface correctly extended.The Props interface properly extends the component's capabilities with the new optional
flaggedExtensionsanddisabledExtensionsfields, maintaining backward compatibility while enabling new functionality.
27-33: LGTM! State management properly implemented.The state variables are well-structured for managing menu visibility and animations. The virtual reference implementation provides a clean way to position the floating menu relative to the drag handle.
35-44: LGTM! Floating UI configuration is well-structured.The floating UI setup uses appropriate middleware for positioning (offset, flip, shift) and the interaction handlers are properly configured. The placement and offset values provide good UX positioning.
85-106: LGTM! Event handling is comprehensive and well-implemented.The event listeners properly handle keyboard (Escape), scroll, and click events. The use of capture phase for scroll events ensures proper handling across nested elements.
113-117: Extract magic number to a named constant.The magic number
50for the animation delay should be extracted to improve maintainability and make the timing configurable.Apply this diff to extract the magic number:
+const ANIMATION_DELAY_MS = 50; + // Animation effect useEffect(() => { if (isOpen) { setIsAnimatedIn(false); // Add a small delay before starting the animation - const timeout = setTimeout(() => { + const timeout = setTimeout(() => { requestAnimationFrame(() => { setIsAnimatedIn(true); }); - }, 50); + }, ANIMATION_DELAY_MS);
140-144: LGTM! Delete action is correctly implemented.The delete action properly uses the editor's chain API and includes proper event handling and menu closure.
157-190: LGTM! Duplicate action has robust error handling.The duplicate functionality includes comprehensive bounds checking, error handling, and proper content insertion logic. The implementation correctly handles edge cases and provides informative error messages.
195-197: LGTM! Clean conditional rendering.The early return pattern when the menu is closed is clean and efficient, preventing unnecessary DOM rendering.
199-237: LGTM! FloatingPortal implementation is well-structured.The FloatingPortal implementation properly integrates with Floating UI, includes appropriate styling and animations, and handles disabled items correctly by filtering them out of the render.
aaryan610
changed the title
Description
This PR adds block menu options to the rich-text editor and updates the old block menu logic with a new floating UI.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Refactor