-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[WIKI-638] fix: peek overview closing while dropdowns are open #7841
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
Changes from all commits
334ea49
a86183b
ceec33b
ecebd9a
712030a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ import { Ellipsis } from "lucide-react"; | |
| import { useCallback, useState } from "react"; | ||
| // plane imports | ||
| import { cn } from "@plane/utils"; | ||
| // constants | ||
| import { CORE_EXTENSIONS } from "@/constants/extension"; | ||
| // extensions | ||
| import { | ||
| findTable, | ||
|
|
@@ -59,7 +61,16 @@ export const ColumnDragHandle: React.FC<ColumnDragHandleProps> = (props) => { | |
| }), | ||
| ], | ||
| open: isDropdownOpen, | ||
| onOpenChange: setIsDropdownOpen, | ||
| onOpenChange: (open) => { | ||
| setIsDropdownOpen(open); | ||
| if (open) { | ||
| editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.TABLE); | ||
| } else { | ||
| setTimeout(() => { | ||
| editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE); | ||
| }, 0); | ||
| } | ||
| }, | ||
|
Comment on lines
+64
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion 🧩 Analysis chainFix race when switching between table dropbars (shared key + deferred removal). Problem:
Recommendations:
Proposed change in this component (requires extending commands to accept instanceId): - onOpenChange: (open) => {
- setIsDropdownOpen(open);
- if (open) {
- editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.TABLE);
- } else {
- setTimeout(() => {
- editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE);
- }, 0);
- }
- },
+ onOpenChange: (open) => {
+ setIsDropdownOpen(open);
+ if (open) {
+ editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current);
+ } else {
+ queueMicrotask(() => {
+ editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current);
+ });
+ }
+ },Additionally (outside this hunk), create a stable instance id and clean up on unmount: // imports: add useRef, useEffect
const instanceIdRef = useRef(Symbol("table-col-dropbar"));
useEffect(() => {
return () => {
// defensive cleanup
try {
editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current);
} catch {}
};
}, [editor]);Run to verify whether ref-counting already exists (if so, just pass instance ids everywhere): Expected:
🏁 Script executed: #!/bin/bash
rg -n -C2 "activeDropbarExtensions" packages/editor
rg -n -C3 "addActiveDropbarExtension|removeActiveDropbarExtension" packages/editorLength of output: 11357 Fix race when switching dropbars: implement per-instance tracking for activeDropbarExtensions Utility currently stores activeDropbarExtensions as a simple array and add/remove only accept the extension key; the deferred removal via setTimeout can remove a key added by another instance (race). Implement per-instance ref-counting and update call sites.
Call sites to update:
Component-level change (example; requires command API change): - onOpenChange: (open) => {
- setIsDropdownOpen(open);
- if (open) {
- editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.TABLE);
- } else {
- setTimeout(() => {
- editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE);
- }, 0);
- }
- },
+ onOpenChange: (open) => {
+ setIsDropdownOpen(open);
+ if (open) {
+ editor.commands.addActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current);
+ } else {
+ queueMicrotask(() => {
+ editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current);
+ });
+ }
+ },Also add in the component: const instanceIdRef = useRef(Symbol("table-col-dropbar"));
useEffect(() => () => {
try { editor.commands.removeActiveDropbarExtension(CORE_EXTENSIONS.TABLE, instanceIdRef.current); } catch {}
}, [editor]);🤖 Prompt for AI Agents |
||
| whileElementsMounted: autoUpdate, | ||
| }); | ||
| const click = useClick(context); | ||
|
|
@@ -185,7 +196,6 @@ export const ColumnDragHandle: React.FC<ColumnDragHandleProps> = (props) => { | |
| }} | ||
| lockScroll | ||
| /> | ||
|
|
||
| <div | ||
| className="max-h-[90vh] w-[12rem] overflow-y-auto rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 shadow-custom-shadow-rg" | ||
| ref={refs.setFloating} | ||
|
|
@@ -195,7 +205,7 @@ export const ColumnDragHandle: React.FC<ColumnDragHandleProps> = (props) => { | |
| zIndex: 100, | ||
| }} | ||
| > | ||
| <ColumnOptionsDropdown editor={editor} onClose={() => setIsDropdownOpen(false)} /> | ||
| <ColumnOptionsDropdown editor={editor} onClose={() => context.onOpenChange(false)} /> | ||
| </div> | ||
| </FloatingPortal> | ||
| )} | ||
|
|
||
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.
Bug: TipTap Suggestion Plugin Callback Parameter Issue
The
onExitcallback for the TipTapSuggestionplugin now destructures{ editor }, but the callback typically receives no parameters. This makeseditorundefined, preventingremoveActiveDropbarExtensionfrom running and leaving the slash command in the active dropdowns list, which breaks dropdown tracking.