Table drag: view.tablePos is never remapped through tr.mapping, throws on a concurrent edit mid-drag

[mustafa-yilmaz]

Summary

While building a table-drag visual-feedback example (#2920), a reviewer asked about behavior when two users edit a table at the same time. Investigating turned up a crash bug in TableHandlesExtension itself, unrelated to that PR's own code.

Root cause

In packages/core/src/extensions/TableHandles/TableHandles.ts, TableHandlesView.tablePos is set once per mousemove:

this.tablePos = pmNodeInfo.posBeforeNode + 1;

It's then used directly, without ever being remapped through tr.mapping, in several places - notably the drop-cursor decorations():

const tableResolvedPos = state.doc.resolve(view.tablePos + 1);

mousemove doesn't fire on the dragged-over element while a native HTML5 drag is in progress, so tablePos (and the state.block content snapshot used later by dropHandler) can't refresh during a drag. If any transaction changes the document elsewhere while a drag is active - a concurrent local edit, or another collaborator's change over Yjs - tablePos goes stale.

Reproduction

1. Start dragging a table row (mousedown on the row handle, then move over another row without releasing).
2. While the drag is still in progress, dispatch any transaction that changes the document before the table (e.g. editor._tiptapEditor.view.dispatch(editor._tiptapEditor.view.state.tr.insertText("X", 1, 1)) to insert into a heading above the table).
3. The next dragover recomputes the drop-cursor decoration from the now-stale tablePos, which resolves into the wrong node and throws:

RangeError: Index 1 out of range for <"...heading text...">
  at posAtIndex (prosemirror-model)
  at TableHandles.ts:695 (decorations)

This throws out of viewDecorations → updateStateInner → dispatchTransaction, i.e. it breaks the transaction dispatch for the other user's edit, not just the drag.

Suggested fix

Same pattern already used correctly elsewhere for stored positions across transactions: remap tablePos (and any other stored position/snapshot used mid-drag) through tr.mapping in an appendTransaction/plugin apply, rather than only refreshing it on mousemove. Happy to open a PR for this if useful, but wanted to report it as its own issue since it's unrelated to the table-drag-visualization example in #2920 where it was found.

Environment

Found against main while working from a local build of packages/core/packages/react/packages/mantine.

---

Update — after #2920 (225c1ccf4)

Recording where this stands, since #2920 changes the symptom but not the cause.

The crash path is now guarded. #2920 moves the drag decoration build into getTableDragDecorations and calls it inside a single try/catch in the plugin's decorations prop, covering every resolve/posAtIndex in both the highlight and drop-cursor branches. It also verifies the resolved node is actually a table before indexing into it. With that in place the reproduction above no longer throws — the decorations are skipped for that state, so the other user's transaction dispatches normally instead of dispatchTransaction blowing up.

What this issue still covers: view.tablePos is captured on mousemove and never refreshed against the current document. That is the whole of it.

Correction. An earlier version of this section also claimed view.state.block goes stale and that dropHandler therefore works from a stale snapshot, so a concurrent edit could land a wrong drop. That was wrong, and I withdraw it. TableHandlesView.update() already re-resolves the block on every view update:

this.state.block = this.editor.getBlock(this.state.block.id)!;

tablePos is the only piece that isn't refreshed, which is what the title of this issue says. Apologies for the noise.

So the impact splits cleanly by version. Before #2920: the RangeError above, which escapes dispatchTransaction. After #2920: the guard catches it, so the drop cursor silently stops rendering for the rest of the drag while the drop itself still lands correctly. A visual bug rather than a correctness one — but still worth fixing, since the guard is a backstop, not a fix.

Note the stack trace above cites TableHandles.ts:695; that line number predates #2920 and has since moved.

Per @nperez0111's comment below, this will come as its own PR.

[nperez0111]

Happy to take a separate PR for this @mustafa-yilmaz

[mustafa-yilmaz]

Some context on how this relates to #2920, since the two are easy to conflate, plus a correction I have just made to the body above.

How the two are connected

This was found while working on #2920, not caused by it. #2920 adds a drag preview and a drag-source highlight to TableHandlesExtension; while a reviewer was asking about concurrent editing during a drag, the RangeError in the report above turned up in BlockNote's own drop-cursor code, which #2920 does not otherwise touch.

How they differ in what they fix

#2920 wraps the whole drag decoration build in a try/catch, so a stale position skips the decorations instead of throwing out of the plugin. That is a backstop: it stops the crash from escaping into dispatchTransaction and breaking the other user's edit. It leaves the position itself wrong.

This issue is the actual fix — making the position correct, so the backstop never has to fire.

Timing

They are independent and can land in either order. The fix here is written against main as it stands today, without #2920, so it does not depend on that PR being merged. If both land, they touch the same file and will need a trivial conflict resolution, but neither blocks the other.

Correction to the body above

I had appended an update claiming view.state.block also goes stale, and that dropHandler consequently works from a stale snapshot and can land a wrong drop. That was wrong and I have struck it. TableHandlesView.update() already re-resolves the block from its ID on every view update, so tablePos really is the only piece that is not refreshed — exactly as originally reported. The impact after #2920 is therefore visual (the drop cursor stops rendering for the rest of the drag) rather than a wrong drop.

On the suggested fix

The original report suggested remapping through tr.mapping. Having implemented it, re-resolving from the table's block ID turned out to be the better fit: a mapping follows the position when surrounding content shifts, but not when the table node is replaced outright, which is what Yjs does when e.g. a cell colour is changed from the side menu. The ID survives both. The cached position is validated first, so the usual path is a single resolve rather than a document walk.

I have a branch with the fix and a regression test that reproduces the exact RangeError above (it drives a real drag, dispatches an insert into the paragraph above the table mid-drag, and asserts the drop cursor still renders afterwards). Opening the PR (#2986) shortly, @nperez0111.

[mustafa-yilmaz]

This is fixed on main by #2972 (b9cd9c4f, "fix(core): only attach table handles to actual table blocks"), so closing.

That commit added TableHandlesView.getTablePos(doc), which resolves the table by ID against the document being used rather than reading the position cached on mousemove, and decorations() calls it. Its comment names the ordering detail that made this bite — ProseMirror computes decorations for a transaction before it calls the plugin view's update, so a cached position is still the pre-transaction one at that point.

Verified, not assumed. I wrote an end-to-end test that reproduces the exact RangeError in the report: it drives a real drag via Playwright's mouse, dispatches an insert into the paragraph above the table while the drag is in progress, then asserts the drop cursor still renders and the concurrent edit landed. Against b9cd9c4f with no other changes it passes on chromium and webkit. On v0.52.1 it fails with:

RangeError: Index 1 out of range for <paragraph("XXXX…Paragraph above the table")>

Correcting my earlier comments here. I said I had a branch with a fix and was about to open a PR — that became #2986, which I have now closed as redundant. I had branched from v0.52.1 rather than current main and did not re-check that the bug still reproduced before writing the fix. Apologies for the churn.

Worth noting the fix that landed matches what I had independently arrived at, including the choice to re-resolve by block ID rather than remap through tr.mapping — a mapping follows the position when surrounding content shifts, 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. Good call in #2972.