From b384c0e83e87c18b8232b7061c72b882d6f365e6 Mon Sep 17 00:00:00 2001 From: mustafa-yilmaz Date: Thu, 20 Aug 2026 11:06:55 +0300 Subject: [PATCH] fix(core): resolve the table drag position against the current document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2921. `TableHandlesView.tablePos` was captured on `mousemove` and then used directly. `mousemove` doesn't fire while a native drag is in progress, so any transaction that changed the document mid-drag - a concurrent local edit, or another collaborator's change over Yjs - left it stale. The next `decorations()` call resolved it into whatever node now sat at that position and threw: RangeError: Index 1 out of range for That escapes `viewDecorations` -> `updateStateInner` -> `dispatchTransaction`, so it breaks the dispatch of the *other* edit, not just the drag. `getTablePos(doc)` now resolves the position against the document it's about to be used with: it keeps the cached value when that still points at the table, and re-resolves from the table's block ID when it doesn't. All three consumers go through it - the drop-cursor decorations, the drag-start transaction meta, and `setCellSelection`. Re-resolving by ID rather than remapping through `tr.mapping` was the deliberate choice here. A mapping follows the position when content shifts around the table, but not when the table node is replaced outright, which is what Yjs does when e.g. a cell colour changes from the side menu. The ID survives both. The cached position is checked first, so the common case is one `resolve` rather than a document walk. The regression test drives a real drag, dispatches an insert into the paragraph above the table mid-drag, and asserts the drop cursor still renders afterwards - so the position is re-resolved rather than merely swallowed - and that the concurrent edit itself landed. It reproduces the RangeError above without the fix. Co-Authored-By: Claude Opus 5 --- .../extensions/TableHandles/TableHandles.ts | 63 +++++++++- tests/src/end-to-end/tables/tables.test.tsx | 109 ++++++++++++++++++ 2 files changed, 166 insertions(+), 6 deletions(-) diff --git a/packages/core/src/extensions/TableHandles/TableHandles.ts b/packages/core/src/extensions/TableHandles/TableHandles.ts index 4616d76b70..1a4946e2e9 100644 --- a/packages/core/src/extensions/TableHandles/TableHandles.ts +++ b/packages/core/src/extensions/TableHandles/TableHandles.ts @@ -1,3 +1,4 @@ +import type { Node } from "prosemirror-model"; import { EditorState, Plugin, PluginKey, PluginView } from "prosemirror-state"; import { CellSelection, @@ -526,6 +527,44 @@ export class TableHandlesView implements PluginView { return true; }; + /** + * The position just before the table node, resolved against `doc`. + * + * `tablePos` is only refreshed on `mousemove`, which doesn't fire while a + * native drag is in progress, so any transaction that changes the document + * mid-drag leaves it stale - resolving it then lands in the wrong node, or + * past the end of the document, and throws (#2921). + * + * Rather than mapping the stored position through every transaction, it's + * checked against the document it's about to be used with and re-resolved + * from the table's block ID when it no longer points at that table. That + * also covers the table node being replaced outright rather than moved, + * which a mapping wouldn't follow - it happens when collaborating. + */ + getTablePos(doc: Node): number | undefined { + if (this.tableId === undefined) { + return undefined; + } + + // `tablePos` sits just inside the block container, so the node it resolves + // into is the container, which carries the ID to check against. A false + // negative here only costs the lookup below, so the cheap check is enough. + if (this.tablePos !== undefined && this.tablePos <= doc.content.size) { + try { + if (doc.resolve(this.tablePos).parent.attrs.id === this.tableId) { + return this.tablePos; + } + } catch { + // Out of range for this document - re-resolve below. + } + } + + const posInfo = getNodeById(this.tableId, doc); + this.tablePos = posInfo && posInfo.posBeforeNode + 1; + + return this.tablePos; + } + // Updates drag handles when the table is modified or removed. update() { if (!this.state || !this.state.show) { @@ -645,12 +684,19 @@ export const TableHandlesExtension = createExtension(({ editor }) => { if ( view === undefined || view.state === undefined || - view.state.draggingState === undefined || - view.tablePos === undefined + view.state.draggingState === undefined ) { return; } + // Resolved against the state being rendered, rather than read from + // the last `mousemove`, so a document change mid-drag can't leave + // the decorations pointing at a position that no longer exists. + const tablePos = view.getTablePos(state.doc); + if (tablePos === undefined) { + return; + } + const newIndex = view.state.draggingState.draggedCellOrientation === "row" ? view.state.rowIndex @@ -681,7 +727,7 @@ export const TableHandlesExtension = createExtension(({ editor }) => { } // Gets the table to show the drop cursor in. - const tableResolvedPos = state.doc.resolve(view.tablePos + 1); + const tableResolvedPos = state.doc.resolve(tablePos + 1); if (view.state.draggingState.draggedCellOrientation === "row") { const cellsInRow = getCellsAtRowHandle( @@ -816,7 +862,7 @@ export const TableHandlesExtension = createExtension(({ editor }) => { view!.state!.draggingState!.draggedCellOrientation, originalIndex: view!.state!.colIndex, newIndex: view!.state!.colIndex, - tablePos: view!.tablePos, + tablePos: view!.getTablePos(tr.doc), }), ); @@ -856,7 +902,7 @@ export const TableHandlesExtension = createExtension(({ editor }) => { view!.state!.draggingState!.draggedCellOrientation, originalIndex: view!.state!.rowIndex, newIndex: view!.state!.rowIndex, - tablePos: view!.tablePos, + tablePos: view!.getTablePos(tr.doc), }), ); @@ -952,7 +998,12 @@ export const TableHandlesExtension = createExtension(({ editor }) => { throw new Error("Table handles view not initialized"); } - const tableResolvedPos = state.doc.resolve(view.tablePos! + 1); + const tablePos = view.getTablePos(state.doc); + if (tablePos === undefined) { + throw new Error("Table handles view is not attached to a table"); + } + + const tableResolvedPos = state.doc.resolve(tablePos + 1); const startRowResolvedPos = state.doc.resolve( tableResolvedPos.posAtIndex(relativeStartCell.row) + 1, ); diff --git a/tests/src/end-to-end/tables/tables.test.tsx b/tests/src/end-to-end/tables/tables.test.tsx index f3b6bf7ce4..7694bf924e 100644 --- a/tests/src/end-to-end/tables/tables.test.tsx +++ b/tests/src/end-to-end/tables/tables.test.tsx @@ -1,6 +1,7 @@ import App from "@examples/01-basic/testing/src/App"; import { beforeEach, describe, expect, test, vi } from "vite-plus/test"; import { render } from "vitest-browser-react"; +import type { EditorView } from "prosemirror-view"; import { browserName, userEvent } from "../../utils/context.js"; import { EDITOR_SELECTOR, TABLE_SELECTOR } from "../../utils/const.js"; import { @@ -301,4 +302,112 @@ describe("Check Table interactions", () => { await compareDocToSnapshot("addColumnThenRow"); }, ); + + // Regression test for https://github.com/TypeCellOS/BlockNote/issues/2921. + // `TableHandlesView.tablePos` is only refreshed on `mousemove`, which does + // not fire while a native drag is in progress. A transaction that changes + // the document elsewhere mid-drag - a concurrent local edit, or another + // collaborator's change over Yjs - therefore used to leave it stale, and the + // next `dragover` resolved it into the wrong node and threw a RangeError out + // of `decorations()`. That breaks `dispatchTransaction` for the *other* + // edit, not just the drag. Playwright doesn't correctly simulate drag events + // in Firefox. + test.skipIf(browserName === "firefox")( + "Document change mid-drag should not break the drag", + async () => { + await focusOnEditor(); + await userEvent.keyboard("Paragraph above the table"); + await userEvent.keyboard("{Enter}"); + await executeSlashCommand("table"); + await waitForSelector(TABLE_SELECTOR); + + const rows = document.querySelectorAll(`${TABLE_SELECTOR} tbody tr`); + const handle = await getTableHandle( + rows[0].querySelector("td") as HTMLElement, + "row", + ); + const handleBox = handle.getBoundingClientRect(); + const secondRowBox = ( + rows[1].querySelector("td") as HTMLElement + ).getBoundingClientRect(); + + await mouseSequence([ + { + type: "move", + x: handleBox.x + handleBox.width / 2, + y: handleBox.y + handleBox.height / 2, + steps: 5, + }, + { type: "down" }, + { + type: "move", + x: secondRowBox.x + secondRowBox.width / 2, + y: secondRowBox.y + secondRowBox.height / 2, + steps: 10, + }, + ]); + + // Inserts into the paragraph above the table, shifting every position + // after it - including the table's - while the drag is still in + // progress. + const view = ( + window as unknown as { + ProseMirror: { view: EditorView }; + } + ).ProseMirror.view; + let firstTextblockPos: number | undefined; + view.state.doc.descendants((node, pos) => { + if (firstTextblockPos !== undefined) { + return false; + } + if (node.isTextblock) { + firstTextblockPos = pos + 1; + return false; + } + return true; + }); + expect(firstTextblockPos).toBeDefined(); + // Long enough that the stale table position lands inside the paragraph + // rather than a little short of the table, which is what turns a wrong + // position into a thrown RangeError. + const insertedText = "X".repeat(80); + view.dispatch( + view.state.tr.insertText( + insertedText, + firstTextblockPos!, + firstTextblockPos!, + ), + ); + + // The next dragover recomputes the decorations against the shifted + // document. The drop cursor still rendering proves the table position + // was re-resolved rather than merely swallowed by a guard. + const lastRowBox = ( + document.querySelectorAll( + `${TABLE_SELECTOR} tbody tr`, + )[1] as HTMLElement + ).getBoundingClientRect(); + await mouseSequence([ + { + type: "move", + x: lastRowBox.x + lastRowBox.width / 2, + y: lastRowBox.y + lastRowBox.height / 2 + 2, + steps: 10, + }, + ]); + + await vi.waitFor(() => { + expect( + document.querySelectorAll(".bn-table-drop-cursor").length, + ).toBeGreaterThan(0); + }); + + await mouseSequence([{ type: "up" }]); + + // The concurrent edit landed, i.e. its transaction dispatched normally. + expect(document.querySelector(EDITOR_SELECTOR)!.textContent).toContain( + `${insertedText}Paragraph above the table`, + ); + }, + ); });