[WIKI-498] regression: table bugs#7631
Conversation
aaryan610
added
🐛bug
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
WalkthroughAdds table structure tracking (width/height and node position) to column/row drag-handle plugins to decide when to reuse or rebuild decorations. Updates drag-handle core to special-case table node selection by adjusting the resolved position and reorders checks to handle tables before blockquotes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Editor
participant ColumnPlugin as Column Drag-Handle Plugin
participant RowPlugin as Row Drag-Handle Plugin
User->>Editor: Interact with table drag handle
Editor->>ColumnPlugin: apply(tr, prevState)
Note right of ColumnPlugin: Compute tableMap, table.pos, width
ColumnPlugin->>ColumnPlugin: tableStructureChanged = (prev.width,pos) vs current
alt Structure unchanged
ColumnPlugin->>ColumnPlugin: Map decorations via tr.mapping
alt Decorations valid
ColumnPlugin-->>Editor: Reuse mapped decorations + update width/pos
else Decorations stale
ColumnPlugin-->>Editor: Rebuild decorations + update width/pos
end
else Structure changed
ColumnPlugin-->>Editor: Rebuild decorations + update width/pos
end
Editor->>RowPlugin: apply(tr, prevState)
Note right of RowPlugin: Compute tableMap, table.pos, height
RowPlugin->>RowPlugin: tableStructureChanged = (prev.height,pos) vs current
alt Structure unchanged
RowPlugin->>RowPlugin: Map decorations via tr.mapping
alt Decorations valid
RowPlugin-->>Editor: Reuse mapped decorations + update height/pos
else Decorations stale
RowPlugin-->>Editor: Rebuild decorations + update height/pos
end
else Structure changed
RowPlugin-->>Editor: Rebuild decorations + update height/pos
end
sequenceDiagram
autonumber
actor User
participant Editor
participant DragHandle as handleNodeSelection
User->>Editor: Drag handle on node
Editor->>DragHandle: handleNodeSelection(event)
alt Node is table
Note right of DragHandle: Adjust draggedNodePos (decrement by 2)
else Node is blockquote
DragHandle->>DragHandle: nodePosAtDOMForBlockQuotes(...)
else Other node
DragHandle->>DragHandle: Resolve parent position and adjust for lists/tasks
end
DragHandle-->>Editor: Final selection/position
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/extensions/table/plugins/drag-handles/column/plugin.ts (2)
37-61: Avoid double-mapping DecorationSet; compute once and reuse
prev.decorations?.map(tr.mapping, tr.doc)is called in both the staleness check and the non-stale return path. Compute it once to reduce work and ensure consistency.Apply this refactor:
- let isStale = tableStructureChanged; - - // Only do position-based stale check if structure hasn't changed - if (!isStale) { - const mapped = prev.decorations?.map(tr.mapping, tr.doc); + let isStale = tableStructureChanged; + let mapped: DecorationSet | undefined; + // Only do position-based stale check if structure hasn't changed + if (!isStale) { + mapped = prev.decorations?.map(tr.mapping, tr.doc); for (let col = 0; col < tableMap.width; col++) { const pos = getTableCellWidgetDecorationPos(table, tableMap, col); - if (mapped?.find(pos, pos + 1)?.length !== 1) { + if (mapped?.find(pos, pos + 1)?.length !== 1) { isStale = true; break; } } } if (!isStale) { - const mapped = prev.decorations?.map(tr.mapping, tr.doc); return { decorations: mapped, tableWidth: tableMap.width, tableNodePos: table.pos, }; }
63-84: Lifecycle of ReactRenderer instances for widgetsEach rebuild creates new
ReactRenderer(ColumnDragHandle, ...)instances. Unless those are explicitly destroyed when decorations are dropped, you risk retained references. Consider:
- Adding a stable widget
keyto help ProseMirror map/retain widgets when possible.- Ensuring any prior
ReactRendereris disposed when its decoration disappears (if yourColumnDragHandleencapsulates cleanup, note it).Optionally, add a stable key:
- decorations.push(Decoration.widget(pos, () => dragHandleComponent.element)); + decorations.push( + Decoration.widget(pos, () => dragHandleComponent.element, { + key: `table-col-handle:${table.pos}:${col}`, + }) +);And, if
ColumnDragHandleexposes a destroy/cleanup, wire it via your component or by tracking and disposing renderers when the plugin recalculates.Manually verify (DevTools Performance/Memory) that repeated column add/delete cycles do not grow retained
ReactRendererinstances.packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (2)
37-61: Same double-mapping here; compute once and reuseMirror the column plugin optimization to avoid mapping the DecorationSet twice.
Apply this refactor:
- let isStale = tableStructureChanged; - - // Only do position-based stale check if structure hasn't changed - if (!isStale) { - const mapped = prev.decorations?.map(tr.mapping, tr.doc); + let isStale = tableStructureChanged; + let mapped: DecorationSet | undefined; + // Only do position-based stale check if structure hasn't changed + if (!isStale) { + mapped = prev.decorations?.map(tr.mapping, tr.doc); for (let row = 0; row < tableMap.height; row++) { const pos = getTableCellWidgetDecorationPos(table, tableMap, row * tableMap.width); if (mapped?.find(pos, pos + 1)?.length !== 1) { isStale = true; break; } } } if (!isStale) { - const mapped = prev.decorations?.map(tr.mapping, tr.doc); return { decorations: mapped, tableHeight: tableMap.height, tableNodePos: table.pos, }; }
80-84: Consider widget keys and renderer cleanupAs with columns, rows create new
ReactRenderer(RowDragHandle, ...)instances on rebuild. Add stable keys and ensure cleanup to prevent leaks.Example:
- decorations.push(Decoration.widget(pos, () => dragHandleComponent.element)); + decorations.push( + Decoration.widget(pos, () => dragHandleComponent.element, { + key: `table-row-handle:${table.pos}:${row}`, + }) +);Profile memory while repeatedly adding/removing rows to ensure renderer instances are not retained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts(3 hunks)packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts(3 hunks)packages/editor/src/core/plugins/drag-handle.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
packages/editor/src/core/extensions/table/table/utilities/helpers.ts (1)
getTableCellWidgetDecorationPos(196-197)
packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (2)
packages/editor/src/core/extensions/table/table/utilities/helpers.ts (1)
getTableCellWidgetDecorationPos(196-197)packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
decorations(88-90)
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (3)
packages/editor/src/core/plugins/drag-handle.ts (1)
382-398: Replace magic table offset with structural resolutionUsing a hard-coded
-2to back up from the table cell into the table node is brittle—any change to the schema or additional wrappers (callouts, embeds, etc.) will break this offset. Instead, resolve the document position and walk up the tree to find the nearesttablenode.• File:
packages/editor/src/core/plugins/drag-handle.ts
• Context: lines 382–398Apply this diff within the existing
if (node.matches("table")) { … }branch:if (node.matches("table")) { - // Magic offset: brittle if structure changes - draggedNodePos = draggedNodePos - 2; + // Resolve to the true table node start instead of a fixed offset + const $pos = view.state.doc.resolve(draggedNodePos); + let tableDepth: number | null = null; + for (let i = $pos.depth; i >= 0; i--) { + if ($pos.node(i).type.name === "table") { + tableDepth = i; + break; + } + } + if (tableDepth == null) return; // No table ancestor found + draggedNodePos = $pos.before(tableDepth); } else if (node.matches("blockquote")) { draggedNodePos = nodePosAtDOMForBlockQuotes(node, view); if (draggedNodePos === null || draggedNodePos === undefined) return; } else { // …• This approach locates the table node structurally, making it resilient to nested wrappers or schema changes.
• Keep the table–blockquote ordering to maintain correct specificity.Please sanity-check the following scenarios after applying the refactor:
- Selecting a table as the very first node in the document.
- Selecting a table nested inside other blocks (e.g., callouts or custom wrappers).
- Drag-and-drop initiated from inside a cell versus from outside the table entirely.
packages/editor/src/core/extensions/table/plugins/drag-handles/column/plugin.ts (1)
15-20: Good call tracking table structure (width and node position)Adding
tableWidthandtableNodePosto plugin state is a clean way to detect structural changes and avoid unnecessary rebuilds. This should directly address post-delete drift.packages/editor/src/core/extensions/table/plugins/drag-handles/row/plugin.ts (1)
15-20: Row plugin state tracking looks goodMirrors the column plugin by tracking
tableHeightandtableNodePos. This symmetry simplifies reasoning and should stabilize decoration reuse across row mutations.
3 participants
Description
This PR fixes the following-
Type of Change
Summary by CodeRabbit
Bug Fixes
Performance
Refactor