-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[WIKI-621] refactor: editor config #7850
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
base: preview
Are you sure you want to change the base?
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. WalkthroughAdds an extended editor file-handlers surface (types + CE hook) and merges it into the core editor config; refactors block menu from Tippy to Floating UI; extends drag-handle selectors; exposes an integrations-loading flag to gate editor rendering. One service file change is formatting-only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Editor UI
participant CoreHook as useEditorConfig
participant ExtHook as useExtendedEditorConfig
participant Handlers as Editor file handlers
UI->>CoreHook: request editor file handlers
CoreHook->>ExtHook: getExtendedEditorFileHandlers({ workspaceSlug, projectId })
ExtHook-->>CoreHook: extendedHandlers ({...})
CoreHook-->>UI: { ...coreHandlers, ...extendedHandlers }
note right of Handlers: TFileHandler is intersected with TExtendedFileHandler
sequenceDiagram
autonumber
participant User as User
participant Drag as Drag-handle (virtual ref)
participant Menu as BlockMenu (Floating UI)
User->>Drag: click drag-handle
Drag->>Menu: set virtual reference
Menu->>Menu: open (FloatingPortal render)
User->>Menu: select action / outside click / Escape / scroll
alt action chosen
Menu->>Menu: perform action (delete/duplicate)
Menu->>Menu: close
else dismiss
Menu->>Menu: close
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 with no reviewable 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). (3)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the editor configuration by introducing extension points for file handling, updating the block menu component to use Floating UI instead of Tippy.js, and adding support for draw.io components in the editor.
- Adds
TExtendedFileHandler
type extension point for custom file handling configurations - Replaces Tippy.js with Floating UI in the block menu component for better positioning and animations
- Introduces support for draw.io components and adds reupload methods for workspace and project assets
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/editor/src/core/types/config.ts | Adds TExtendedFileHandler extension to TFileHandler type |
packages/editor/src/core/plugins/drag-handle.ts | Includes editor-drawio-component in drag handle selectors |
packages/editor/src/core/components/menus/block-menu.tsx | Refactors from Tippy.js to Floating UI with improved animations |
packages/editor/src/core/components/editors/rich-text/editor.tsx | Adds BlockMenu component to RichTextEditor |
packages/editor/src/ce/types/config.ts | Defines TExtendedFileHandler as null for CE version |
packages/editor/src/ce/types/index.ts | Exports config types |
apps/web/core/services/file.service.ts | Adds reupload methods for workspace and project assets |
apps/web/core/hooks/editor/use-editor-config.ts | Integrates extended editor config |
apps/web/ce/hooks/editor/use-editor-config.ts | Provides CE implementation of extended editor config |
Comments suppressed due to low confidence (1)
packages/editor/src/core/components/menus/block-menu.tsx:1
- Remove the trailing space character after the closing JSX tag. This adds unnecessary whitespace to the rendered output.
import {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/web/ce/hooks/editor/use-editor-config.ts (1)
6-7
: Make the extension type-safe (avoidobject
).Returning
object
hides unsafe overrides; use a typed surface so TS validates handler shapes across CE/EE.Apply this diff:
-export type TExtendedEditorConfig = object; +export type TExtendedEditorConfig = Partial<TFileHandler>;Add this type-only import at the top of the file:
import type { TFileHandler } from "@plane/editor";Please confirm this type-only import doesn’t introduce layering or circular-dependency issues in your build.
apps/web/core/hooks/editor/use-editor-config.ts (1)
90-93
: Type-narrow the extended spread and confirm override intent.
- Cast to Partial so TS checks shapes of any overrides.
- Because the spread is last, it will override base handlers; confirm this is intentional.
Apply this diff:
- ...extendedEditorConfig({ - projectId, - workspaceSlug, - }), + ...(extendedEditorConfig({ + projectId, + workspaceSlug, + }) as Partial<TFileHandler>),If overriding core handlers is not desired, move this spread before the core properties instead.
packages/editor/src/core/plugins/drag-handle.ts (1)
108-116
: Prevent matching nested Draw.io internals (parity with embed).If Draw.io contains nested DOM, clicks inside it may pick inner nodes before the block root. Mirror the existing embed skip so we only match the Draw.io root.
Apply this change near the existing embed skip:
// Skip elements inside .editor-embed-component if (elem.closest(".editor-embed-component") && !elem.matches(".editor-embed-component")) { continue; } + + // Skip elements inside .editor-drawio-component; allow only the root + if (elem.closest(".editor-drawio-component") && !elem.matches(".editor-drawio-component")) { + continue; + }packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
43-48
: Avoid always mounting BlockMenu; gate by dragDropEnabled.BlockMenu attaches global listeners on mount. If drag-drop is disabled, skip mounting to reduce overhead.
Apply this diff:
- <> - {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />}{" "} - <BlockMenu editor={editor} flaggedExtensions={flaggedExtensions} disabledExtensions={disabledExtensions} /> - </> + <> + {editor && bubbleMenuEnabled && <EditorBubbleMenu editor={editor} />} + {editor && dragDropEnabled && ( + <BlockMenu + editor={editor} + flaggedExtensions={flaggedExtensions} + disabledExtensions={disabledExtensions} + /> + )} + </>packages/editor/src/core/types/config.ts (1)
3-4
: Guard the intersection to be safe if CE leavesTExtendedFileHandler
asnull
.Even after fixing CE to
{}
, harden the core type sovalidation
doesn’t collapse tonever
if an edition accidentally setsTExtendedFileHandler
to a non-object.Apply this diff:
-import { TExtendedFileHandler } from "@/plane-editor/types/config"; +import { TExtendedFileHandler } from "@/plane-editor/types/config"; @@ - } & TExtendedFileHandler; + } & (TExtendedFileHandler extends object ? TExtendedFileHandler : {});Also applies to: 20-21
packages/editor/src/core/components/menus/block-menu.tsx (4)
46-75
: Allow toggle on drag‑handle click (open/close).Currently clicking the handle when open does not close the menu. Toggle to improve UX.
Apply this diff:
- // Show the menu - setIsOpen(true); + // Toggle the menu + setIsOpen((prev) => !prev);
86-97
: Mark scroll listener as passive for perf.Use passive capture to avoid main-thread blocking during scroll.
Apply this diff:
- document.addEventListener("scroll", handleScroll, true); // Using capture phase + const scrollOpts: AddEventListenerOptions = { capture: true, passive: true }; + document.addEventListener("scroll", handleScroll, scrollOpts); // Using capture phase @@ - document.removeEventListener("scroll", handleScroll, true); + document.removeEventListener("scroll", handleScroll, scrollOpts);
190-209
: A11y: add roles; remove inert attribute.
- Add role semantics for menu and items.
data-prevent-outside-click
is not used by Floating UI; remove to avoid confusion.Apply this diff:
- <div + <div + role="menu" 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", "transition-all duration-300 transform origin-top-right", isAnimatedIn ? "opacity-100 scale-100" : "opacity-0 scale-75" )} - data-prevent-outside-click {...getFloatingProps()} > {MENU_ITEMS.map((item) => { if (item.isDisabled) { return null; } return ( <button key={item.key} type="button" + role="menuitem" 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} >
196-206
: Minor: avoid conflicting z-index sources.Inline style zIndex: 99 and class "z-20" conflict. Prefer one source of truth (class or style). Consider removing one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/ce/hooks/editor/use-editor-config.ts
(1 hunks)apps/web/core/hooks/editor/use-editor-config.ts
(2 hunks)apps/web/core/services/file.service.ts
(1 hunks)packages/editor/src/ce/types/config.ts
(1 hunks)packages/editor/src/ce/types/index.ts
(1 hunks)packages/editor/src/core/components/editors/rich-text/editor.tsx
(2 hunks)packages/editor/src/core/components/menus/block-menu.tsx
(5 hunks)packages/editor/src/core/plugins/drag-handle.ts
(1 hunks)packages/editor/src/core/types/config.ts
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
packages/editor/src/core/components/menus/block-menu.tsx (1)
BlockMenu
(25-230)
packages/editor/src/core/types/config.ts (1)
packages/editor/src/ce/types/config.ts (1)
TExtendedFileHandler
(1-1)
apps/web/core/hooks/editor/use-editor-config.ts (1)
apps/web/ce/hooks/editor/use-editor-config.ts (1)
extendedEditorConfig
(7-7)
⏰ 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: Cursor Bugbot
🔇 Additional comments (3)
apps/web/core/hooks/editor/use-editor-config.ts (1)
11-11
: Verify CE/EE path alias configuration
apps/web/tsconfig.json and apps/space/tsconfig.json map@/plane-web/*
→ce/*
. Confirm EE builds override this toee/*
and that your bundler (Next/Webpack/Vite) respects the same alias mapping.packages/editor/src/core/plugins/drag-handle.ts (1)
12-26
: Selector addition looks good; confirm class name presence.Adding ".editor-drawio-component" to the drag-targets is fine. Please confirm this class is applied to the Draw.io block root so the handle targets the correct container.
packages/editor/src/ce/types/index.ts (1)
3-3
: Barrel export addition is fine.Re-exporting config from the types index is OK. Watch for circulars if any downstream also re-exports from this index.
@@ -0,0 +1 @@ | |||
export type TExtendedFileHandler = object; |
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.
the type of the variable is object because we are destructuring it to use.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/ce/hooks/editor/use-extended-editor-config.ts (1)
15-18
: Expose the args parameter for clarityThe typed signature expects
(args: TExtendedEditorFileHandlersArgs)
, but the implementation currently relies on TypeScript’s parameter bivariance by omitting the argument altogether. Explicitly accepting_args
keeps the intent obvious and prevents subtle regressions if the annotation ever moves or strict function types are enabled.- const getExtendedEditorFileHandlers: TExtendedEditorConfig["getExtendedEditorFileHandlers"] = useCallback( - () => ({}), - [] - ); + const getExtendedEditorFileHandlers: TExtendedEditorConfig["getExtendedEditorFileHandlers"] = useCallback( + (_args) => ({}), + [] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/ce/hooks/editor/use-extended-editor-config.ts
(1 hunks)apps/web/ce/hooks/use-editor-flagging.ts
(2 hunks)apps/web/core/components/pages/editor/editor-body.tsx
(2 hunks)apps/web/core/hooks/editor/use-editor-config.ts
(3 hunks)packages/editor/src/ce/types/config.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/ce/types/config.ts
- apps/web/core/hooks/editor/use-editor-config.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/ce/hooks/editor/use-extended-editor-config.ts (1)
packages/editor/src/ce/types/config.ts (1)
TExtendedFileHandler
(1-1)
apps/web/core/components/pages/editor/editor-body.tsx (1)
apps/web/ce/hooks/use-editor-flagging.ts (1)
useEditorFlagging
(29-43)
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/web/ce/hooks/use-editor-flagging.ts (1)
18-43
: CE fallback stays consistent with new contractThe added
isLoadingIntegrations: false
keeps the CE hook aligned with the expanded return type without changing runtime behavior. Looks good.apps/web/core/components/pages/editor/editor-body.tsx (1)
100-104
: Loader guard cleanly incorporates integration stateWiring
isLoadingIntegrations
into the existing loader gate is tidy and keeps the UI from flashing incomplete content. Nicely done.Also applies to: 182-184
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: Drag Handle Selection Bug
Clicking a block's drag handle no longer selects the corresponding block. This can cause block menu actions, such as delete or duplicate, to operate on the wrong content or fail, as they depend on the editor's current selection.
packages/editor/src/core/components/menus/block-menu.tsx#L62-L72
plane/packages/editor/src/core/components/menus/block-menu.tsx
Lines 62 to 72 in 17d8cb7
// Show the menu | |
setIsOpen(true); | |
return; | |
} | |
// If clicking outside and not on a menu item, hide the menu | |
if (menuRef.current && !menuRef.current.contains(target)) { | |
setIsOpen(false); | |
} | |
}, |
Description
Update the editor config and file config
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Refactor