Custom Node View Unexpectedly Destroyed and Recreated during Reconciliation

1

Summary

I’m experiencing an issue where a custom node view is destroyed and recreated when certain edits occur in preceding nodes. I don’t have a self-contained sample reproduction yet, but I have identified the underlying cause and wanted to first see if anyone has experienced something similar, has any workaround, or whether my thoughts below are worth opening a PR for (which I’m happy to do).

Related

Recent post about a similar issue: Node decorations before custom node view causes rerender

Recent changes to relevant part of prosemirror-view: Improve pre-matching in DOM updates · ProseMirror/prosemirror-view@31e7cdf · GitHub, Increase bounds check for children in reconcilliation back to 5 by seanchambo · Pull Request #113 · ProseMirror/prosemirror-view · GitHub

I don’t think either of these account for what I’m experiencing.

Details

I have a custom node view that renders a block element, which is a div containing an iframe. I don’t want this node to be destroyed and recreated on the DOM when other parts of the document change, since that would cause the iframe to reload.

If I have several blank paragraphs before the div/iframe, and at least one blank paragraph after, then if I add a new blank paragraph before the div/iframe and within a few lines of it, the div/iframe node is destroyed and recreated unexpectedly.

Diagnosis

I’ve traced this down to the behavior of prosemirror-view’s ViewDescription reconciliation logic, and in particular to ViewTreeUpdater.prototype.findNodeMatch (prosemirror-view/viewdesc.js at 49ecad1ad1e0e7cf750a1d22322010a007be1479 · ProseMirror/prosemirror-view · GitHub).

What’s happening is this: When the new blank paragraph is inserted shortly before the div/iframe, the reconciliation looks ahead up to 5 nodes to find a match. Since there’s a blank paragraph after the div/iframe, it treats that as a match, destroying everything in between.

To confirm that’s the case, I locally patched prosemirror-view/viewdesc.js at 49ecad1ad1e0e7cf750a1d22322010a007be1479 · ProseMirror/prosemirror-view · GitHub and replaced the lines I highlighted with the following:

      for (let i = this.index, e = Math.min(children.length, i + 5); i < e; i++) {
        let child = children[i]
        if (child.matchesNode(node, outerDeco, innerDeco) && !this.preMatch.matched.has(child)) {
          found = i
          break
        }

        if (child instanceof CustomNodeViewDesc) {
            break;
        }
      }

This treats custom node views as “expensive” and prevents them from being skipped over (and possibly destroyed) during the matching. Of course, I don’t think this is an appropriate patch as is, it’s just meant to illustrate my current understanding of the issue.

Solutions

What’s the appropriate way to address this?

One idea is making the above patch a bit more generic and opt-in. Block nodes could have a property indicating whether they are “expensive” (default false), and the reconciliation logic would try not to destroy expensive block nodes unnecessarily, as per the patch. (Of course, we’d need to think about whether “expensiveness” needs to cascade up the hierarchy.) I’d be happy to open a PR for this as long as the approach is agreed upon.

Other Observations

The related discussion I mentioned above (Node decorations before custom node view causes rerender) is for a cool looking open source project (Curvenote · GitHub). That project doesn’t seem to be experiencing this issue out-of-the-box. I’ve identified the reason why. It has a plugin to decorate the current paragraph with a placeholder class. When a new paragraph is inserted, it’s given that decoration, therefore during reconciliation it cannot match against the empty paragraphs after the iframe, which don’t have the decoration. Running that project and disabling this plugin results in the same behavior I experience above.

Versions

  • prosemirror-model: 1.15.0
  • prosemirror-state: 1.3.4
  • prosemirror-view: 1.23.1

Full disclosure: I’m using ProseMirror through tiptap (https://tiptap.dev/), so it’s certainly possible that something in my custom node implementation via tiptap, or in tiptap itself, is fully or partially to blame.

3 Likes
2

When the change is in front of the node view only, the preMatch.matched.has check should prevent the node view from being discarded here – unless pre-matching is somehow failing, I just patched a problem like that in 609880b. If that patch doesn’t help for you, is it possible to set up a minimal script that shows the problem occurring, using raw ProseMirror? That’d allow me to debug what’s going on, and why the intended mechanism to prevent this is failing.

3

Thanks for the reply.

I ended up going down a different path which obviated my immediate need to have iframe nodes in the document. I may still see if I can create a minimal test case, but unfortunately it’s no longer a priority for me at the moment because I’m not blocked on it anymore.

That said, I did have some observations about prematching when I was first debugging this which may be helpful. The issue I described occurs when the document is first rendered and after the first few edits, but after a while (exact steps unclear), the issue would stop. I was able to see in debugging logs that it did have to do with prematching. When the document was first rendered, the prematch path didn’t kick in, but later, it did. Perhaps this has to do with the prematch data structure not being populated initially (either by a bug in ProseMirror or something the wrapper library I’m using, tiptap, has failed to do – I haven’t looked closely).

For example, I remember a case with a document more or less like this:

<p>Begin</p>
<p></p>
<p></p>
<iframe />
<p></p>
<p></p>
<p>End</p>

If I loaded this document, then inserted a new paragraph between the first and second blank ones, the iframe node would be destroyed and recreated, and I could see in the debugging log it was because my newly inserted empty paragraph was being matched against the empty paragraph after the the iframe. Repeating this several times, and each time it would be destroyed and recreated. But surprisingly, after a few more new empty paragraphs (6 or 7, IIRC), the iframe would no longer be destroyed and recreated. And again, I could see in the debugging that it had to do with the prematch logic.

4

Which version of prosemirror-view are you using? If I set up a scenario like this with the current 1.23.1, the node between the empty paragraphs gets reused.

5

Interesting. I’m using prosemirror-view 1.23.1 as well.

Perhaps this weekend I’ll see if I can create a minimal test case.

6

I’m having exactly the same situation. Custom node gets destroyed and immediatly created, after new line is passed in above node, whith emplty lines above and below of custom node. It is a bit random as not every new lines causes this issue.

Version: prosemirror-view: 1.33.8

7

I get the destroy/recreate behavior specifically when I drag a NodeView down, regardless of which or how many nodes surround it above or below. It doesn’t happen when dragging up :thinking:

"prosemirror-commands": "1.6.0",
"prosemirror-dropcursor": "1.8.1",
"prosemirror-gapcursor": "1.3.2",
"prosemirror-history": "1.4.1",
"prosemirror-inputrules": "1.4.0",
"prosemirror-keymap": "1.2.2",
"prosemirror-model": "1.22.3",
"prosemirror-state": "1.4.3",
"prosemirror-transform": "1.10.0",
"prosemirror-view": "1.34.2",
8

That’s expected—there no guarantee that a node won’t be redrawn when it changes its position in the document. The update code happens to preserve it in the dragging up situation here, but even that isn’t something you can count on.

9

Ah okay. If I have a custom NodeView that is rendering something expensive and I can’t make guarantees of when it’ll be re-rendered, do you suggest I implement some cache layer then? I’m specifically building a view of panels a user can interact with that render graphs. Though IDK how frequently someone would be moving this nodeview around.

I thought that manually managing the dom state of a NodeView would allow me to avoid this constraint. Is there a possible improvement that can be made so that NodeViews can opt out of being destroy and re-created when only their position changes?

For reference, it looks like this at the moment. I’m using the nodeview such that I can allow GridStack to handle the dom updates within the nodeview

image
image1284×1210 41.7 KB

10

Not really. As far as ProseMirror is concerned nodes do not have identity, and what’s happening is one node being deleted and another one being created. You could work around this by caching DOM elements, if you’re careful (to not, say, try and return the same element twice when a given node occurs multiple times in the document). But for the panels in the screenshot, I’m not sure why rendering those would be all that expensive.

11

Hmm okay that makes sense but I’m still confused as to why the drag up doesn’t trigger it. Can you point me in the direction of where I can read about this behavior or the code paths I could review?

I don’t have the graphs rendering yet as it’s a WIP. But you could imagine a potential tens of thousands of points per graph. Those would be created after a potentially expensive network request, then some data manipulation to shape the network data into something usable by the graphing package, then rendering. So destroying that work isn’t ideal.

I have a few potential optimizations outside of prosemirror such as caching the network request, which would likely suffice for most cases, but was really hoping to optimize at the source.

12

Hi there !

I’m digging up this topic as I’m noticing similar behaviors in our editor implementation. I tried to pinpoint our issue but I think I’m missing some key-points to fully understand our issue.

Context:

  • We are using Tiptap (And its ability to use React components in NodeViews) and some prosemirror plugin.
  • We are using a lot of custom React Nodes to render complex components, backed up by Prosemirror node view system.

Test case:

  • I have an instance hosting 511 nodes (10 paragraphs, 500 custom nodes using React, 1 paragraph)
  • I’m deleting the 10 first paragraphs
  • We are getting a freeze (Linked to the 500 custom nodes re-rendering 500 times).

What I identified so far

While debugging, I noticed that:

  • The latency is due to the re-creation of the 500 prosemirror NodeView (The ones with React implementation)
  • The matching between the existing nodes and the updated version is failing, leading to the re-creation of the associated node view.
    • Prematch stops early due to this equality check (Even of the node content looks identical)
    • The next matching step is using a 5 items window which is failing as well as the deletion of 10 paragraphs is over this limit.
  • If I undo and perform the same operation again, there’s no issue somehow.

Do you have any idea what could lead to such issues ? What could recreate the prosemirror “Node” which triggers the failing prematch ? Why match is using node.eq(this.node) while prematch is using simple equality checks node != frag.child(fI - 1) ?

Thank you for your inputs :slight_smile:

13

Can you inspect the transactions generated by Yjs and see which part of your document they actually replace? I’ve seen y-prosemirror generate changes that were much bigger than the content actually touched by the original edit.

14

Thanks for the quick feedback !

I’m not very familiar with the internals of YJS, nor the bindings with prosemirror.

With a listener on the YDoc, I can see that:

  • there is a small DeleteSet ({ "clock": 40,"len": 160}) only linked to the content that I selected, not the entire document.
  • the YXMLEvent contains
    • 0 added
    • 8 deleted elements
    • a delta aligned with that [ { "retain": 2 }, { "delete": 8 }]

When comparing the YDoc before and after, the store contains the same count of entries. When checking the YDoc after, the share contains the correct elements: The YXmlElement linked to the NodeView have the same clock as before the deletion.

In which way is it syncing the changes ? The transaction is applied to prosemirror internal state, then synced to the YDoc / the other way / both at the same time ?

15

I’m not actually sure. But the fact that removing Yjs from the setup makes the problem go away suggests that it is affecting the way the local editor processes updates that originate in that same editor, not just passively recording/sending them.

If you can reproduce an issue like this in plain ProseMirror without TipTap or Yjs, I’ll gladly debug it. But if those libraries are necessary to get the problem, I’d suggest you talk with their authors.

1 Like
16

The way that this is implemented in Y.js is that every remote change to the document causes a replace of the entire prosemirror document. This is because y-prosemirror does not perform a diff, which would only change the part of the document that changed, rather it generates the entire document tree from scratch. This means that to ProseMirror, the whole document was destroyed & happened to be recreated again.

This is a known limitation of the y-prosemirror binding, and will not likely be fixed anytime soon. Someone has to do the diff of the documents, and both y-prosemirror, and ProseMirror do not want to be responsible for it for their own reasons. In my opinion, this should be the responsibility of y-prosemirror.

@donovan-fournier my recommendation would be to not use React for nodeviews, React is pure overhead for what can likely be done with plain JS nodeviews. Each nodeview has to render with React separately, and React is not known for being fast.

1 Like