[WIKI-740] refactor: editor table performance #8411
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 listener-tracking and cleanup to column and row drag-handle components and introduces renderer lifecycle management (track, destroy, reuse) plus a plugin-level destroy hook for column and row drag-handle plugins. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial 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). (4)
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 improves performance and memory management in the table editor's drag handle plugins by implementing proper cleanup of React renderers and event listeners. The changes ensure that old renderers are destroyed before creating new ones and that event listeners attached to the window object are properly tracked and removed.
Key Changes:
- Added renderer tracking in plugin state to enable cleanup of old renderers before creating new ones
- Implemented a destroy hook in both row and column plugins to clean up renderers when plugins are destroyed
- Added useRef-based tracking of window event listeners in drag handle components for cleanup on unmount
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts | Adds renderer tracking to plugin state, implements cleanup before creating new renderers, and adds destroy hook for plugin teardown |
| packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx | Adds ref-based tracking of window event listeners and cleanup effect on component unmount |
| packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts | Adds renderer tracking to plugin state, implements cleanup before creating new renderers, and adds destroy hook for plugin teardown |
| packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx | Adds ref-based tracking of window event listeners and cleanup effect on component unmount |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx
Show resolved
Hide resolved
![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 (1)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
109-121: Consider extracting shared cleanup logic.Both column and row plugins have identical
destroy()implementations. Since this is a refactor focused on performance and maintainability, consider extracting this cleanup logic into a shared utility function.🔎 Example shared utility
// In a shared utils file export function destroyRenderers(renderers: ReactRenderer[] | undefined): void { renderers?.forEach((renderer) => { try { renderer.destroy(); } catch (error) { console.error("Error destroying renderer:", error); } }); }Then in both plugins:
destroy() { const state = editor.state && (TABLE_COLUMN_DRAG_HANDLE_PLUGIN_KEY.getState(editor.state) as TableColumnDragHandlePluginState | undefined); - state?.renderers?.forEach((renderer: ReactRenderer) => { - try { - renderer.destroy(); - } catch (error) { - console.error("Error destroying renderer:", error); - } - }); + destroyRenderers(state?.renderers); },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.tspackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.tspackages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.tspackages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.tspackages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.tspackages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts
🧠 Learnings (3)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
🧬 Code graph analysis (2)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (1)
decorations(104-107)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (1)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
decorations(104-107)
⏰ 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). (5)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx (2)
52-70: Good cleanup pattern for window event listeners.The implementation correctly captures
activeListenersRef.currentat effect setup time (line 60), which is the right approach for cleanup functions. This ensures the cleanup removes the exact handlers that were registered, even ifactiveListenersRef.currentis modified before unmount.
192-201: Solid defensive implementation.Storing handler references before attaching listeners (lines 194-196), combined with the try-catch wrapper, provides robust error handling. If an exception occurs during listener setup,
handleFinish()is called to clean up markers and state.packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx (2)
52-70: Consistent cleanup implementation across drag handles.The cleanup pattern correctly mirrors the row drag handle implementation. Capturing the ref value at setup time ensures reliable cleanup regardless of subsequent state changes.
192-201: LGTM!The try-catch with fallback to
handleFinish()ensures markers are cleaned up even if listener attachment fails unexpectedly.packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (2)
67-74: LGTM!Properly destroys old renderers before creating new ones, preventing memory leaks. The try-catch ensures a single failing renderer doesn't prevent others from being cleaned up.
109-121: Remove incorrect claim aboutgetState()throwing.The first concern about
editor.statebeing undefined is valid and properly handled by the code. However, ProseMirror's PluginKey.getState() returns PluginState | undefined and does not throw exceptions—it safely returns undefined if the plugin key has no state. The code's defensive pattern with optional chaining and try-catch wrapping renderer cleanup is appropriate and safe.packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (2)
21-22: Renderer preservation looks correct.When decorations aren't stale, the existing renderers are preserved (line 63), avoiding unnecessary destruction and recreation. This is a good optimization for performance.
Also applies to: 57-65
67-99: Proper renderer lifecycle management.The implementation correctly:
- Destroys old renderers before creating new ones (lines 68-74)
- Creates and tracks new renderers (lines 78, 91)
- Returns the new renderers array in state (line 99)
This ensures no orphaned ReactRenderer instances are left behind.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| tableWidth?: number; | ||
| tableNodePos?: number; | ||
| // track renderers for cleanup | ||
| renderers?: ReactRenderer[]; |
There was a problem hiding this comment.
Renderers not destroyed when navigating away from table
When a user navigates away from a table (so findTable returns undefined), the apply function returns {} without destroying the renderers tracked in prev.renderers. This causes a memory leak because the ReactRenderer instances and their associated event listeners are never cleaned up. The PR adds renderer tracking for cleanup but misses this code path where prev has renderers that need destruction before returning an empty state.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx (1)
58-70: Simplify cleanup by directly accessing the ref.The local variable capture of
activeListenersRef.currentis unnecessary. Since refs maintain the same object reference and the properties are mutated rather than replaced, you can directly accessactiveListenersRef.currentin the cleanup function.🔎 Proposed simplification
// Cleanup window event listeners on unmount useEffect(() => { - const listenersRef = activeListenersRef.current; return () => { // Remove any lingering window event listeners when component unmounts - if (listenersRef.mouseup) { - window.removeEventListener("mouseup", listenersRef.mouseup); + if (activeListenersRef.current.mouseup) { + window.removeEventListener("mouseup", activeListenersRef.current.mouseup); } - if (listenersRef.mousemove) { - window.removeEventListener("mousemove", listenersRef.mousemove); + if (activeListenersRef.current.mousemove) { + window.removeEventListener("mousemove", activeListenersRef.current.mousemove); } }; }, []);packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx (1)
58-70: Simplify cleanup by directly accessing the ref.The local variable capture is unnecessary—you can directly access
activeListenersRef.currentin the cleanup function.🔎 Proposed simplification
// Cleanup window event listeners on unmount useEffect(() => { - const listenersRef = activeListenersRef.current; return () => { // Remove any lingering window event listeners when component unmounts - if (listenersRef.mouseup) { - window.removeEventListener("mouseup", listenersRef.mouseup); + if (activeListenersRef.current.mouseup) { + window.removeEventListener("mouseup", activeListenersRef.current.mouseup); } - if (listenersRef.mousemove) { - window.removeEventListener("mousemove", listenersRef.mousemove); + if (activeListenersRef.current.mousemove) { + window.removeEventListener("mousemove", activeListenersRef.current.mousemove); } }; }, []);
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx (1)
48-258: Consider extracting shared drag logic.The column and row drag-handle components share nearly identical listener tracking and cleanup logic. Consider refactoring the common patterns into a shared custom hook (e.g.,
useDragListeners) or utility to reduce duplication and improve maintainability.💡 Example approach
Create a shared hook:
function useDragListeners() { const activeListenersRef = useRef<{ mouseup?: (e: MouseEvent) => void; mousemove?: (e: MouseEvent) => void; }>({}); useEffect(() => { return () => { if (activeListenersRef.current.mouseup) { window.removeEventListener("mouseup", activeListenersRef.current.mouseup); } if (activeListenersRef.current.mousemove) { window.removeEventListener("mousemove", activeListenersRef.current.mousemove); } }; }, []); const attachListeners = (mouseup: (e: MouseEvent) => void, mousemove: (e: MouseEvent) => void) => { // Remove old listeners if (activeListenersRef.current.mouseup) { window.removeEventListener("mouseup", activeListenersRef.current.mouseup); } if (activeListenersRef.current.mousemove) { window.removeEventListener("mousemove", activeListenersRef.current.mousemove); } // Store and attach new listeners activeListenersRef.current.mouseup = mouseup; activeListenersRef.current.mousemove = mousemove; window.addEventListener("mouseup", mouseup); window.addEventListener("mousemove", mousemove); }; const clearListeners = () => { if (activeListenersRef.current.mouseup) { window.removeEventListener("mouseup", activeListenersRef.current.mouseup); } if (activeListenersRef.current.mousemove) { window.removeEventListener("mousemove", activeListenersRef.current.mousemove); } activeListenersRef.current.mouseup = undefined; activeListenersRef.current.mousemove = undefined; }; return { attachListeners, clearListeners }; }Then use it in both components to eliminate duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsxpackages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.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: Build packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/drag-handle.tsx (4)
52-56: LGTM!The
activeListenersRefproperly tracks window event listeners for cleanup, addressing the memory leak concern from previous reviews.
116-125: Memory leak properly addressed!The pre-cleanup logic correctly removes existing listeners before adding new ones, preventing multiple simultaneous drag operations and addressing the memory leak concern from previous reviews.
166-168: LGTM!Clearing the ref after removing listeners ensures no stale references remain for the next drag operation.
204-206: LGTM!Storing listener references before attaching them enables proper cleanup in all scenarios (rapid clicks, errors, unmount).
packages/editor/src/core/extensions/table/plugins/drag-handles/column/drag-handle.tsx (2)
52-56: LGTM!The
activeListenersRefproperly tracks window event listeners, mirroring the cleanup strategy in the row drag-handle.
116-125: Memory leak properly addressed!The cleanup strategy is correct:
- Remove old listeners before starting a new drag (lines 116-125)
- Clear refs after finishing a drag (lines 166-168)
- Store refs before attaching listeners (lines 204-206)
This prevents multiple simultaneous drags and ensures proper cleanup in all scenarios.
Also applies to: 166-168, 204-206
…actor/editor-table-performance
Description
This PR refactors table extension's column and row drag handle plugins-
Type of Change
Note
Refactors table row/column drag handles and plugins to reliably clean up window event listeners and React renderers for better performance and stability.
packages/editor/src/core/extensions/table/plugins/drag-handles/*/drag-handle.tsx)mouseup/mousemovehandlers via refs; remove on finish/unmount; prevent concurrent drags; add defensive error handling.*/drag-handles/*/plugin.ts)ReactRendererinstances; destroy old renderers before recreating and on plugindestroy.decorationswhen structure unchanged; add null-safedecorationsaccess inprops.Written by Cursor Bugbot for commit e285ccc. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Performance
Stability
✏️ Tip: You can customize this high-level summary in your review settings.