[WIKI-696] fix: peek view close on click block menu options#7861
[WIKI-696] fix: peek view close on click block menu options#7861
Conversation
iam-vipin
requested review from
aaryan610 and
Copilot
and removed request for
Copilot
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
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. WalkthroughIntroduces explicit openBlockMenu and closeBlockMenu callbacks that register/unregister the SIDE_MENU via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant DragHandle as Drag Handle
participant BlockMenu as Block Menu
participant Editor as Editor.commands
participant Dropbar as ActiveDropbarExtensions
User->>DragHandle: Click
DragHandle->>BlockMenu: call openBlockMenu
BlockMenu->>Editor: addActiveDropbarExtension(SIDE_MENU)
Editor->>Dropbar: SIDE_MENU active
alt Dismiss (Outside click / Esc / Scroll)
User->>BlockMenu: Dismiss interaction
BlockMenu->>Editor: removeActiveDropbarExtension(SIDE_MENU)
BlockMenu-->>User: Close menu (isOpen false)
end
rect rgba(230,245,255,0.6)
note right of BlockMenu: Menu item click (wrapped)
User->>BlockMenu: Click menu item
BlockMenu->>BlockMenu: invoke item.onClick (wrapped -> prevents propagation)
BlockMenu->>Editor: removeActiveDropbarExtension(SIDE_MENU)
BlockMenu-->>User: Close menu
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 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 |
![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
🧹 Nitpick comments (4)
packages/editor/src/core/components/menus/block-menu.tsx (4)
73-85: Stabilize hook deps to prevent unnecessary re-binding.Using editor.commands in deps can be unstable and cause listener churn. Prefer editor (which is stable) in the deps for handleClickDragHandle.
Apply:
- [editor.commands, refs] + [editor, refs]
87-99: Ensure SIDE_MENU is always cleared on close/unmount.You clear the extension on several events, but if the menu closes via other paths or the component unmounts while open, SIDE_MENU may remain active. Add explicit cleanup in the effect and a small watcher on isOpen.
Add cleanup on unmount:
return () => { document.removeEventListener("click", handleClickDragHandle); document.removeEventListener("contextmenu", handleClickDragHandle); document.removeEventListener("keydown", handleKeyDown); document.removeEventListener("scroll", handleScroll, true); + // Ensure SIDE_MENU is not left active on unmount + editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); };Also add a watcher so any close path clears it:
+ // Ensure SIDE_MENU is cleared whenever the menu closes via any path + useEffect(() => { + if (!isOpen) { + editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); + } + }, [isOpen, editor]);Also applies to: 104-111
110-110: Use editor (not editor.commands) in effect deps.Prevents unnecessary add/remove of global listeners if commands proxy is non-stable.
- }, [editor.commands, handleClickDragHandle]); + }, [editor, handleClickDragHandle]);
223-229: Call preventDefault/stopPropagation before executing item action.Prevents any default/bubbling side-effects from firing before the action runs.
- onClick={(e) => { - item.onClick(e); - e.preventDefault(); - e.stopPropagation(); - setIsOpen(false); - editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); - }} + onClick={(e) => { + e.preventDefault(); + e.stopPropagation(); + item.onClick(e); + setIsOpen(false); + editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/components/menus/block-menu.tsx(3 hunks)packages/editor/src/core/extensions/utility.ts(1 hunks)
⏰ 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)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/editor/src/core/extensions/utility.ts (1)
14-20: Approve SIDE_MENU addition
TAdditionalActiveDropbarExtensions is defined asnever, so addingCORE_EXTENSIONS.SIDE_MENUintroduces no duplicate union.
iam-vipin
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where peek view closes unexpectedly when clicking block menu options by implementing proper active modal utility management. The fix prevents unintended closing behavior by properly tracking the side menu state through the active dropbar extension system.
- Adds
SIDE_MENUto the active dropbar extensions type - Implements
toggleBlockMenufunction to manage side menu state consistently - Refactors menu item click handlers to prevent event propagation and ensure proper cleanup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/editor/src/core/extensions/utility.ts | Adds SIDE_MENU to active dropbar extensions type |
| packages/editor/src/core/components/menus/block-menu.tsx | Implements proper state management with toggleBlockMenu and refactors event handlers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const toggleBlockMenu = useCallback(() => { | ||
| if (isOpen) { | ||
| setIsOpen(false); | ||
| editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); | ||
| } else { | ||
| setIsOpen(true); | ||
| editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.SIDE_MENU); | ||
| } | ||
| }, [editor, isOpen]); |
There was a problem hiding this comment.
The function name toggleBlockMenu is misleading since it's being called in contexts where the menu should always be closed (like on Escape key or scroll). This could lead to unintended opening of the menu when it should only close.
| // Show the menu | ||
| setIsOpen(true); | ||
| toggleBlockMenu(); |
There was a problem hiding this comment.
Using toggleBlockMenu() here will close the menu if it's open, but open it if it's closed. Since this is in the drag handle click handler, it should only open the menu, not toggle it.
| // If clicking outside and not on a menu item, hide the menu | ||
| if (menuRef.current && !menuRef.current.contains(target)) { | ||
| setIsOpen(false); | ||
| toggleBlockMenu(); |
There was a problem hiding this comment.
Using toggleBlockMenu() when clicking outside should only close the menu, not toggle it. This could accidentally open the menu when clicking outside.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/components/menus/block-menu.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/editor/src/core/components/menus/block-menu.tsx (1)
packages/editor/src/index.ts (1)
CORE_EXTENSIONS(16-16)
⏰ 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)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
…e#7861) * fix: block close * chore : add toggle option in block menu * fix: separate methods
Description
This PR adds logic to use the active modal utility, preventing the issue peek view from closing unexpectedly.
Type of Change
Screenshots and Media (if applicable)
Screen.Recording.2025-09-26.at.6.40.59.PM.mov
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes