Skip to content

Commit 225c1cc

Browse files
mustafa-yilmazclaude
mustafa-yilmaz
and
claude
committed
fix(core): guard the whole table drag decoration build against stale state
Addresses review on #2920 (r3798069357): the try/catch only covered `state.doc.resolve(tablePos + 1)`, so everything after it was still unprotected. `posAtIndex` calls `node.child()` internally and throws a RangeError when the index is out of range, which is reachable two ways - `tablePos` still resolving but no longer pointing at the table, and the row/column counts coming from `state.block`, a snapshot taken on hover that can exceed what's in the document by the time it's used. The decoration build moves into `getTableDragDecorations`, which the plugin prop calls inside a single try/catch, so a stale position or index anywhere in it - including the drop-cursor branches, which were never covered - skips the decorations instead of throwing out of the plugin. It also checks the resolved node really is a table before indexing into it. This still isn't a fix for the underlying staleness (#2921), which needs `tablePos` remapped through `tr.mapping`; it keeps the editor alive until then. Also clears the drag image in `TableHandlesView.destroy()`. Cleanup otherwise only runs from `dragEnd`, which never arrives if the editor is torn down mid-drag, leaving the copy in the DOM and the module-scope reference set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 48bbace commit 225c1cc

1 file changed

Lines changed: 174 additions & 158 deletions

File tree

‎packages/core/src/extensions/TableHandles/TableHandles.ts‎

Lines changed: 174 additions & 158 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,10 @@ export class TableHandlesView implements PluginView {
694694
}
695695

696696
destroy() {
697+
// The drag image is normally cleaned up on `dragEnd`, which never arrives
698+
// if the editor is torn down mid-drag.
699+
unsetTableDragImage();
700+
697701
this.pmView.dom.removeEventListener("mousemove", this.mouseMoveHandler);
698702
window.removeEventListener("mouseup", this.mouseUpHandler);
699703
this.pmView.dom.removeEventListener("mousedown", this.viewMousedownHandler);
@@ -708,6 +712,170 @@ export class TableHandlesView implements PluginView {
708712
}
709713
}
710714

715+
/**
716+
* Builds the decorations shown while a table row or column is being dragged:
717+
* a highlight on the cells being dragged, and the drop cursor marking where
718+
* they'll end up.
719+
*
720+
* Both `tablePos` and the row/column counts in `viewState.block` are captured
721+
* when a cell is hovered and aren't remapped afterwards, so resolving them can
722+
* throw once they've gone stale - see the caller, which treats that as "no
723+
* decorations".
724+
*/
725+
function getTableDragDecorations(
726+
state: EditorState,
727+
tablePos: number,
728+
viewState: TableHandlesState,
729+
): DecorationSet | undefined {
730+
const { block, draggingState } = viewState;
731+
732+
if (!block || !draggingState) {
733+
return undefined;
734+
}
735+
736+
const { originalIndex, draggedCellOrientation } = draggingState;
737+
const decorations: Decoration[] = [];
738+
739+
// Gets the table to show the decorations in.
740+
const tableResolvedPos = state.doc.resolve(tablePos + 1);
741+
if (tableResolvedPos.node().type.name !== "table") {
742+
return undefined;
743+
}
744+
745+
// Highlights the cells of the row/column being dragged, so it stays clear
746+
// what is being moved while the drop cursor shows where it will be moved to.
747+
const draggedCells =
748+
draggedCellOrientation === "row"
749+
? getCellsAtRowHandle(block, originalIndex)
750+
: getCellsAtColumnHandle(block, originalIndex);
751+
752+
draggedCells.forEach(({ row, col }) => {
753+
// Gets the row in the table, then the cell within that row.
754+
const rowResolvedPos = state.doc.resolve(
755+
tableResolvedPos.posAtIndex(row) + 1,
756+
);
757+
const cellPos = rowResolvedPos.posAtIndex(col);
758+
const cellNode = state.doc.resolve(cellPos + 1).node();
759+
760+
decorations.push(
761+
Decoration.node(cellPos, cellPos + cellNode.nodeSize, {
762+
class: "bn-table-drag-source",
763+
}),
764+
);
765+
});
766+
767+
const newIndex =
768+
draggedCellOrientation === "row" ? viewState.rowIndex : viewState.colIndex;
769+
770+
// Only the highlight is shown, without a drop cursor, if:
771+
// - The cursor isn't over a cell
772+
// - Dragging to same position
773+
// - Row drag not allowed
774+
// - Column drag not allowed
775+
if (
776+
newIndex === undefined ||
777+
newIndex === originalIndex ||
778+
(draggedCellOrientation === "row" &&
779+
!canRowBeDraggedInto(block, originalIndex, newIndex)) ||
780+
(draggedCellOrientation === "col" &&
781+
!canColumnBeDraggedInto(block, originalIndex, newIndex))
782+
) {
783+
return DecorationSet.create(state.doc, decorations);
784+
}
785+
786+
if (draggedCellOrientation === "row") {
787+
const cellsInRow = getCellsAtRowHandle(block, newIndex);
788+
789+
cellsInRow.forEach(({ row, col }) => {
790+
// Gets each row in the table.
791+
const rowResolvedPos = state.doc.resolve(
792+
tableResolvedPos.posAtIndex(row) + 1,
793+
);
794+
795+
// Gets the cell within the row.
796+
const cellResolvedPos = state.doc.resolve(
797+
rowResolvedPos.posAtIndex(col) + 1,
798+
);
799+
const cellNode = cellResolvedPos.node();
800+
// Creates a decoration at the start or end of each cell,
801+
// depending on whether the new index is before or after the
802+
// original index.
803+
const decorationPos =
804+
cellResolvedPos.pos +
805+
(newIndex > originalIndex ? cellNode.nodeSize - 2 : 0);
806+
decorations.push(
807+
// The widget is a small bar which spans the width of the cell.
808+
Decoration.widget(decorationPos, () => {
809+
const widget = document.createElement("div");
810+
widget.className = "bn-table-drop-cursor";
811+
widget.style.left = "0";
812+
widget.style.right = "0";
813+
// This is only necessary because the drop indicator's height
814+
// is an even number of pixels, whereas the border between
815+
// table cells is an odd number of pixels. So this makes the
816+
// positioning slightly more consistent regardless of where
817+
// the row is being dropped.
818+
if (newIndex > originalIndex) {
819+
widget.style.bottom = "-2px";
820+
} else {
821+
widget.style.top = "-3px";
822+
}
823+
widget.style.height = "4px";
824+
825+
return widget;
826+
}),
827+
);
828+
});
829+
} else {
830+
const cellsInColumn = getCellsAtColumnHandle(block, newIndex);
831+
832+
cellsInColumn.forEach(({ row, col }) => {
833+
// Gets each row in the table.
834+
const rowResolvedPos = state.doc.resolve(
835+
tableResolvedPos.posAtIndex(row) + 1,
836+
);
837+
838+
// Gets the cell within the row.
839+
const cellResolvedPos = state.doc.resolve(
840+
rowResolvedPos.posAtIndex(col) + 1,
841+
);
842+
const cellNode = cellResolvedPos.node();
843+
844+
// Creates a decoration at the start or end of each cell,
845+
// depending on whether the new index is before or after the
846+
// original index.
847+
const decorationPos =
848+
cellResolvedPos.pos +
849+
(newIndex > originalIndex ? cellNode.nodeSize - 2 : 0);
850+
851+
decorations.push(
852+
// The widget is a small bar which spans the height of the cell.
853+
Decoration.widget(decorationPos, () => {
854+
const widget = document.createElement("div");
855+
widget.className = "bn-table-drop-cursor";
856+
widget.style.top = "0";
857+
widget.style.bottom = "0";
858+
// This is only necessary because the drop indicator's width
859+
// is an even number of pixels, whereas the border between
860+
// table cells is an odd number of pixels. So this makes the
861+
// positioning slightly more consistent regardless of where
862+
// the column is being dropped.
863+
if (newIndex > originalIndex) {
864+
widget.style.right = "-2px";
865+
} else {
866+
widget.style.left = "-3px";
867+
}
868+
widget.style.width = "4px";
869+
870+
return widget;
871+
}),
872+
);
873+
});
874+
}
875+
876+
return DecorationSet.create(state.doc, decorations);
877+
}
878+
711879
export const tableHandlesPluginKey = new PluginKey("TableHandlesPlugin");
712880

713881
export const TableHandlesExtension = createExtension(({ editor }) => {
@@ -782,168 +950,16 @@ export const TableHandlesExtension = createExtension(({ editor }) => {
782950
return;
783951
}
784952

785-
const decorations: Decoration[] = [];
786-
const { block, draggingState } = view.state;
787-
const { originalIndex, draggedCellOrientation } = draggingState;
788-
789-
if (!block) {
790-
return DecorationSet.create(state.doc, decorations);
791-
}
792-
793-
// Gets the table to show the decorations in. `tablePos` is
794-
// captured when a cell is hovered and isn't remapped afterwards,
795-
// so a transaction that changes the document elsewhere while a
796-
// drag is in progress - a concurrent local or collaborative edit -
797-
// can leave it pointing outside the document. Skip the decorations
798-
// in that case rather than throwing out of the plugin.
799-
let tableResolvedPos;
800953
try {
801-
tableResolvedPos = state.doc.resolve(view.tablePos + 1);
954+
return getTableDragDecorations(state, view.tablePos, view.state);
802955
} catch {
956+
// A transaction that changes the document while a drag is in
957+
// progress - a concurrent local or collaborative edit - can
958+
// leave the captured table position and block snapshot pointing
959+
// past the end of the document. Skip the decorations for this
960+
// state rather than throwing out of the plugin.
803961
return;
804962
}
805-
806-
// Highlights the cells of the row/column being dragged, so it stays
807-
// clear what is being moved while the drop cursor shows where it
808-
// will be moved to.
809-
const draggedCells =
810-
draggedCellOrientation === "row"
811-
? getCellsAtRowHandle(block, originalIndex)
812-
: getCellsAtColumnHandle(block, originalIndex);
813-
814-
draggedCells.forEach(({ row, col }) => {
815-
// Gets the row in the table, then the cell within that row.
816-
const rowResolvedPos = state.doc.resolve(
817-
tableResolvedPos.posAtIndex(row) + 1,
818-
);
819-
const cellPos = rowResolvedPos.posAtIndex(col);
820-
const cellNode = state.doc.resolve(cellPos + 1).node();
821-
822-
decorations.push(
823-
Decoration.node(cellPos, cellPos + cellNode.nodeSize, {
824-
class: "bn-table-drag-source",
825-
}),
826-
);
827-
});
828-
829-
const newIndex =
830-
draggedCellOrientation === "row"
831-
? view.state.rowIndex
832-
: view.state.colIndex;
833-
834-
// Only the highlight is shown, without a drop cursor, if:
835-
// - The cursor isn't over a cell
836-
// - Dragging to same position
837-
// - Row drag not allowed
838-
// - Column drag not allowed
839-
if (
840-
newIndex === undefined ||
841-
newIndex === originalIndex ||
842-
(draggedCellOrientation === "row" &&
843-
!canRowBeDraggedInto(block, originalIndex, newIndex)) ||
844-
(draggedCellOrientation === "col" &&
845-
!canColumnBeDraggedInto(block, originalIndex, newIndex))
846-
) {
847-
return DecorationSet.create(state.doc, decorations);
848-
}
849-
850-
if (draggedCellOrientation === "row") {
851-
const cellsInRow = getCellsAtRowHandle(
852-
view.state.block,
853-
newIndex,
854-
);
855-
856-
cellsInRow.forEach(({ row, col }) => {
857-
// Gets each row in the table.
858-
const rowResolvedPos = state.doc.resolve(
859-
tableResolvedPos.posAtIndex(row) + 1,
860-
);
861-
862-
// Gets the cell within the row.
863-
const cellResolvedPos = state.doc.resolve(
864-
rowResolvedPos.posAtIndex(col) + 1,
865-
);
866-
const cellNode = cellResolvedPos.node();
867-
// Creates a decoration at the start or end of each cell,
868-
// depending on whether the new index is before or after the
869-
// original index.
870-
const decorationPos =
871-
cellResolvedPos.pos +
872-
(newIndex > originalIndex ? cellNode.nodeSize - 2 : 0);
873-
decorations.push(
874-
// The widget is a small bar which spans the width of the cell.
875-
Decoration.widget(decorationPos, () => {
876-
const widget = document.createElement("div");
877-
widget.className = "bn-table-drop-cursor";
878-
widget.style.left = "0";
879-
widget.style.right = "0";
880-
// This is only necessary because the drop indicator's height
881-
// is an even number of pixels, whereas the border between
882-
// table cells is an odd number of pixels. So this makes the
883-
// positioning slightly more consistent regardless of where
884-
// the row is being dropped.
885-
if (newIndex > originalIndex) {
886-
widget.style.bottom = "-2px";
887-
} else {
888-
widget.style.top = "-3px";
889-
}
890-
widget.style.height = "4px";
891-
892-
return widget;
893-
}),
894-
);
895-
});
896-
} else {
897-
const cellsInColumn = getCellsAtColumnHandle(
898-
view.state.block,
899-
newIndex,
900-
);
901-
902-
cellsInColumn.forEach(({ row, col }) => {
903-
// Gets each row in the table.
904-
const rowResolvedPos = state.doc.resolve(
905-
tableResolvedPos.posAtIndex(row) + 1,
906-
);
907-
908-
// Gets the cell within the row.
909-
const cellResolvedPos = state.doc.resolve(
910-
rowResolvedPos.posAtIndex(col) + 1,
911-
);
912-
const cellNode = cellResolvedPos.node();
913-
914-
// Creates a decoration at the start or end of each cell,
915-
// depending on whether the new index is before or after the
916-
// original index.
917-
const decorationPos =
918-
cellResolvedPos.pos +
919-
(newIndex > originalIndex ? cellNode.nodeSize - 2 : 0);
920-
921-
decorations.push(
922-
// The widget is a small bar which spans the height of the cell.
923-
Decoration.widget(decorationPos, () => {
924-
const widget = document.createElement("div");
925-
widget.className = "bn-table-drop-cursor";
926-
widget.style.top = "0";
927-
widget.style.bottom = "0";
928-
// This is only necessary because the drop indicator's width
929-
// is an even number of pixels, whereas the border between
930-
// table cells is an odd number of pixels. So this makes the
931-
// positioning slightly more consistent regardless of where
932-
// the column is being dropped.
933-
if (newIndex > originalIndex) {
934-
widget.style.right = "-2px";
935-
} else {
936-
widget.style.left = "-3px";
937-
}
938-
widget.style.width = "4px";
939-
940-
return widget;
941-
}),
942-
);
943-
});
944-
}
945-
946-
return DecorationSet.create(state.doc, decorations);
947963
},
948964
},
949965
}),

0 commit comments

Comments
 (0)