[Bug]: Usage of ReactNodeViewRender drastically reduces performance for large Text

[wasular]

Which packages did you experience the bug in?

core

What Tiptap version are you using?

2.1.6

What’s the bug you are facing?

If I use this simple React Component as Wrapper for a Custom Paragraph the performance drastically reduces for large text. Luckily in my use case I don't need to use React Render for the Paragraph but there seems to be some performance bug which could be critical for other users.

import { NodeViewWrapper, NodeViewContent } from '@tiptap/react';
import DragIndicatorIcon from '@mui/icons-material/DragIndicator';
import styles from './DraggableNodeView.module.scss';
import React from 'react';

const DraggableNodeView = ({ contentComponent: ContentComponent, ...props }) => {
  return (
    <NodeViewWrapper className={styles.node__wrapper}>
      <DragIndicatorIcon draggable="true" data-drag-handle className={styles.drag__icon} />
      {ContentComponent ? (
        <ContentComponent {...props} />
      ) : (
        <NodeViewContent className={styles.node__content} />
      )}
    </NodeViewWrapper>
  );
};

export default DraggableNodeView;

I now switched to vanilla html and js for this and it works

import { Paragraph } from '@tiptap/extension-paragraph';

const CustomParagraph = Paragraph.extend({
  draggable: true,
  addNodeView() {
    return () => {
      const dom = document.createElement('div');
      dom.className = 'node__wrapper';

      const dragIcon = document.createElement('div');
      dragIcon.draggable = true;
      dragIcon.setAttribute('data-drag-handle', '');
      dragIcon.className = 'drag__icon';
      dragIcon.innerHTML =
        'svg';
      dom.appendChild(dragIcon);

      const content = document.createElement('div');
      content.className = 'node__content';
      dom.appendChild(content);

      return {
        dom: dom,
        contentDOM: content,
      };
    };
  },
});

export default CustomParagraph;

What browser are you using?

Chrome

Code example

No response

What did you expect to happen?

Using a simple React render should not have this big of influence on the performance.

Anything to add? (optional)

No response

Did you update your dependencies?

• Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

• Yes, I’m a sponsor. 💖

[jackbb147]

I have the same issue. Terrible terrible performance with just a few react node views.

[Zloka]

I noticed the same. In my case, it seems to be related to when the selection changes. ReactNodeViewRenderer has the following code:

handleSelectionUpdate() {
    const { from, to } = this.editor.state.selection

    if (from <= this.getPos() && to >= this.getPos() + this.node.nodeSize) {
      this.selectNode()
    } else {
      this.deselectNode()
    }
}

The problem is that this.deselectNode() is called for all nodes, regardless of if they were selected or not, which causes a subsequent call to this.updateProps({ selected: false }), and that seems to cause a re-render of all nodes.

A better solution, assuming it is easy to track the currently selected node, would be to only deselect relevant nodes, i.e. something like:

handleSelectionUpdate() {
    const { from, to } = this.editor.state.selection

    if (from <= this.getPos() && to >= this.getPos() + this.node.nodeSize) {
      this.selectNode()
    } else if (this.node.attrs.selected) {
      this.deselectNode();
    }
}

From what I've tested, interestingly enough in my case, I could just remove the this.deselectNode(); call entirely to both see a drastic improvement in performance, and the behavior remained as one would expect 🤔

[Zloka]

I noticed the same. In my case, it seems to be related to when the selection changes. ReactNodeViewRenderer has the following code:

handleSelectionUpdate() {
    const { from, to } = this.editor.state.selection

    if (from <= this.getPos() && to >= this.getPos() + this.node.nodeSize) {
      this.selectNode()
    } else {
      this.deselectNode()
    }
}

The problem is that this.deselectNode() is called for all nodes, regardless of if they were selected or not, which causes a subsequent call to this.updateProps({ selected: false }), and that seems to cause a re-render of all nodes.

A better solution, assuming it is easy to track the currently selected node, would be to only deselect relevant nodes, i.e. something like:

handleSelectionUpdate() {
    const { from, to } = this.editor.state.selection

    if (from <= this.getPos() && to >= this.getPos() + this.node.nodeSize) {
      this.selectNode()
    } else if (this.node.attrs.selected) {
      this.deselectNode();
    }
}

From what I've tested, interestingly enough in my case, I could just remove the this.deselectNode(); call entirely to both see a drastic improvement in performance, and the behavior remained as one would expect 🤔

To follow up a bit on my own reply: I did notice a change in behavior in some cases, as one would expect with the naive solution I suggested.

A more proper solution that, from what I can tell, keeps the behavior identical but also improves the performance, is essentially any solution that prevents the unnecessary this.render(); calls in updateProps.

One temporary workaround could be to e.g. add a separate updateSelectedProp function:

updateSelectedProp(selected = false) {
    if (this.props.selected !== selected) {
      this.props.selected = selected;
      this.render();
    }
  }

...and change deselectNode to call it instead...

deselectNode() {
    this.renderer.updateSelectedProp(false);
    this.renderer.element.classList.remove('ProseMirror-selectednode');
  }

Now, it seems that only the relevant nodes re-render, which in my cases of multiple thousands of nodes is very noticeable. A lot of unnecessary calls to this.deselectNode and this.renderer.element.classList.remove('ProseMirror-selectednode'); still happen with the above approach, but those don't seem very costly.

[fdintino]

edit: There is a flaw with this patch that is addressed in a different post below.

I experienced a similar problem: text input in documents with many react node views was so slow as to make it unusable. There are probably a few performance optimizations to be had, but I found my issues to be related to #3580, which called out this method:

tiptap/packages/react/src/EditorContent.tsx

Lines 72 to 82 in 42039c0

	maybeFlushSync(fn: () => void) {
	// Avoid calling flushSync until the editor is initialized.
	// Initialization happens during the componentDidMount or componentDidUpdate
	// lifecycle methods, and React doesn't allow calling flushSync from inside
	// a lifecycle method.
	if (this.initialized) {
	flushSync(fn)
	} else {
	fn()
	}
	}

I fixed it by patching the function to just execute the function passed to it:

// EDIT: see updated patch below
// import { PureEditorContent } from "@tiptap/react";
//
// PureEditorContent.prototype.maybeFlushSync = (fn: () => void) => {
//     fn();
//   };

I'll keep tabs on #3580 to see if there is a functional reason that it needs to call flushSync there, but I haven't noticed any issues so far.

[jaekunchoi]

I experienced a similar problem: text input in documents with many react node views was so slow as to make it unusable. There are probably a few performance optimizations to be had, but I found my issues to be related to #3580, which called out this method:

tiptap/packages/react/src/EditorContent.tsx

Lines 72 to 82 in 42039c0

	maybeFlushSync(fn: () => void) {
	// Avoid calling flushSync until the editor is initialized.
	// Initialization happens during the componentDidMount or componentDidUpdate
	// lifecycle methods, and React doesn't allow calling flushSync from inside
	// a lifecycle method.
	if (this.initialized) {
	flushSync(fn)
	} else {
	fn()
	}
	}

I fixed it by patching the function to just execute the function passed to it:

import { PureEditorContent } from "@tiptap/react";

PureEditorContent.prototype.maybeFlushSync = (fn: () => void) => {
    fn();
  };

I'll keep taps on #3580 to see if there is a functional reason that it needs to call flushSync there, but I haven't noticed any issues so far.

this fixed for me aswell

[frankdilo]

I confirm this is an issue for my team too!

Removing the flushSync(fn) as the comments above suggest improves performance massively.

With unpatched TipTap I see my custom nodes re-render on every keystroke and it gets very problematic when the document has tens of them.

With the patch, I see only the custom node I am editing re-render.

EDIT: Digging around with the React Profilers, I see many more React render commits (hundreds vs a handful) with flushSync enabled.

EDIT 2: digging through the blames in ReactNodeViewRenderer it seems as if the flushSync behavior was introduced to fix this bug and judging from the commit message it was intended to render node views immediately when created, but what seems to be happening here is that they are rendered on every keystroke, which was not the intended behavior.

[fdintino]

Thanks for digging into that. I think you're right that flushSync is only meant to run once, after the component mounts. I've modified my patch and I believe it fixes some odd interactivity issues I had been experiencing, while also showing the same performance improvements. For some reason I didn't think to attribute my issues to this change.

PureEditorContent.prototype.maybeFlushSync = function maybeFlushSync(fn) {
  if (this.initialized) {
    flushSync(fn);
    this.initialized = false;
  } else {
    fn();
  }
};

[andrewlu0]

We're experiencing similar issues as well, even after the patch above. I tried wrapping the NodeViewWrapper with a React Profiler https://react.dev/reference/react/Profiler and it looks like all the nodes are updating with every keystroke. Is this expected?

[fdintino]

@andrewlu0 I can only speculate without seeing your code, but I would try to rule out something in your application. The flushSync bug is a problem in the framework, but it's fairly common to cause excessive renders in the course of regular development. I'd recommend profiling with the react chrome extension and judiciously applying debounced renders and memoization.

[andrewlu0]

@fdintino I've tried a few attempts at memoization and debounced renders, but still experience significant lag when the number of React nodes reaches a few hundred, even with a very basic React node that is just a wrapper around a header. Is your application able to handle this well? Even trying the sample on their website here https://tiptap.dev/guide/node-views/react, if you copy/paste so there's around 300 copies of that yellow button node, the editor becomes very slow.

[fdintino]

I do experience lag and performance issues, and with hundreds of nodes it becomes unusable. But those performance issues are fixed (for me) with the overridden definition of PureEditorContent.prototype.maybeFlushSync I posted earlier in the thread.

[dbousamra]

Just adding another data point to the discussion. We have long documents (50+ A4 pages), with only ~ 100 React nodes in them. Performance is not yet at a standstill, but it's no longer "buttery" as we would like.

Adding the monkeypatch fix above fixes it for me. Thanks @fdintino

[kwarc-star]

I am deploying to firebase and I cannot monkey patch because firebase rebuilds node_modules directory. Any way around this?

[zolero]

Please can someone fix this at TipTap, because this makes TipTap with react components unusable... This should be fixed.

[AdventureBeard]

Just to add another data point, the monkey-patch provided above does not improve performance for me, and changes the behavior of cursor placements after inserting nodes with React Node Views.

[fdintino]

@AdventureBeard are you seeing the cursor behavior changes with the first or second patch that I posted?

[frankdilo]

We had to revert the patch as well because of some weird cursor placement bugs that were introduced after applying it.

[fdintino]

I wonder if there are other places where state dependencies aren't fully accounted for, but they hadn't caused issues before because this bug was rerendering all components on every keystroke.

[fdintino]

I am assuming you're both referring to my patch and not @Zloka's, but it's probably worth clarifying that.

[AdventureBeard]

@fdintino, this one:

PureEditorContent.prototype.maybeFlushSync = function maybeFlushSync(fn) {
  if (this.initialized) {
    flushSync(fn);
    this.initialized = false;
  } else {
    fn();
  }
};

Specifically, whenever I inserted a node with a NodeView with a content hole, the cursor would be placed after the entire node, instead of within it. WIthout the monkey-patch, the cursor is placed inside the node view after insert as expected. You can modify the insert code to account for this, but it would require code changes to all my nodes and I started to feel like I was working against Tiptap, so I put it on pause to look for other perf improvements I could make to the React node renderer.

[fdintino]

I'm not seeing that behavior. I was thinking of putting together a PR with this change, so if you could provide a minimal test case I would appreciate it. I would want to include the fix for that alongside this change.

[Nantris]

Can anyone tell me how exactly they're testing versus non-React rendering? So far I can't find any significant differences when rendering a codeblock in React versus pure JS.

[fdintino]

Sure. Here's a codesandbox demonstrating the problem, with a toggle between tiptap-react, a vanilla js nodeview, and a version with my patch and @Zloka's above. The number in the left margin is a count of the number of renders.

[Nantris]

Thanks @fdintino! That definitely makes React a lot more responsive in that example.

Unfortunately it doesn't seem to bring Android speed up to vanilla JS speed as it seems to on PC, but that's understandable.

[fdintino]

That can be achieved by wrapping the component in React.memo, since for this particular component it's not using any of the props. For me, this gives comparable performance:

const ParagraphComponentPatched = React.memo(
  () => (
    <NodeViewWrapperPatched className="wrapper">
      <RenderCount />
      <NodeViewContentPatched as="p" />
    </NodeViewWrapperPatched>
  ),
  (prevProps, nextProps) => true
);

[bdbch]

We have the performance issues in our pipeline and we'll probably start working on all performance related issues with React after the holidays - just as a heads up.

[fdintino]

The fix incorporated one of the fixes, but it didn't include my flushSync fix. I'll open that in a pull request.

[ArihantBapna]

@fdintino was your flushSync fix added yet?

[Nantris]

@bdbch is there any update on plans for this? Or has anything already been implemented?

I think it would be good to get this issue re-opened since it certainly does remain a problem (in 2.1.14 at least.)

We use a React renderer for only one node type, but it slows the editor down 10 fold in circumstances.

[Nantris]

Could we please get this re-opened since the issue has been noted to not have really been addressed?

We have the performance issues in our pipeline and we'll probably start working on all performance related issues with React after the holidays - just as a heads up.

[Nantris]

Will there ever be follow-up on this? It's been nearly half a year.

The issue persists and it should be opened, at the very least.

[Nantris]

Bump. The last time we heard from maintainers on this was five months ago and it's not even been re-opened, much less a single word of communication.

[Reza-tm]

Same here on version 2.4.0

[svenadlung]

Sorry for the silence. We will definitely take care of the issue.

[nperez0111]

I have some ideas on how to improve this that I'll be implementing as a follow up to: #5161

Hopefully will get to it this week or beginning of next week

[bibschan]

following the thread here as I am in urgent need of this fix, thanks for looking into it @svenadlung!

[ianrtracey]

Running into performance issues as well

[bartlomiej-korpus]

We've prepared a hacky patch ourselves and run it in production for several months now. It's goal is to avoid portal rerenders as much as possible in ReactNodeViews https://github.com/bartlomiej-korpus/tiptap/pull/1/files

@ianrtracey @bibschan let me know if you'd like to try this, I can help you prepare a yarn patch

the problem with current solution is that all the ReactRenderer portals are rendered flat as part of same component(Portals), so each time any of them changes all of them rerender, and even memoization doesn't help if there's a lot of them. This happens inside flushSync so it quickly becomes catastrophic :D

In our version portals are rendered in a nested structure and at each level there is a memo which is an opportunity to stop further/deeper renders from happening at all.

[nperez0111]

This is fantastic @bartlomiej-korpus, I think your approach of avoiding re-rendering portals, and my approach of avoiding flushSyncs (instead using useSyncExternalStore) will definitely help performance of the integration. Sorry that I haven't gotten the time to get this completely done yet.

[bartlomiej-korpus]

This is fantastic @bartlomiej-korpus, I think your approach of avoiding re-rendering portals, and my approach of avoiding flushSyncs (instead using useSyncExternalStore) will definitely help performance of the integration. Sorry that I haven't gotten the time to get this completely done yet.

Nice @nperez0111 ! I definitely wanted to avoid flushSync, but I didn't feel confident enough in understanding its relationship to prosemirror-view rendering process to do it and opted for keeping current setRenderer behaviour 100% compatible instead. If it works and doesn't break anything then my fix becomes redundant, react batching will just do its job(there would still be some rerenders, but not nearly as many/as costly).

[Nantris]

Closeable now with the advent of shouldRerenderOnTransaction?

[nperez0111]

There definitely still are some optimizations that I want to make for react node views specifically so happy to keep it open

[nperez0111]

So I looked into the changes made with: #4492 (comment)

and it suffers from the same problem that I've been seeing (where on first render focus cannot be moved into the element). This issue happens because React renders in two passes and not synchronously. My patch here eeks out a bit more performance (by not using flushSync so often).

So I think for now we can probably just apply this, see if there are drawbacks I haven't accounted for and go from there.

[frankdilo]

Just saw the news for 2.5. Does it effectively close this issue?

[Nantris]

@frankdilo

There definitely still are some optimizations that I want to make for react node views specifically so happy to keep it open

and the PR linked above is not yet merged.

[nperez0111]

This is now resolved since 2.6