Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edits by peers produce a prosemirror transaction that spans the entire document #113

Open
2 tasks done
ascott18 opened this issue May 19, 2022 · 3 comments
Open
2 tasks done
Assignees
Labels
bug Something isn't working

Comments

@ascott18
Copy link

ascott18 commented May 19, 2022

Checklist

Describe the bug
When a remote peer makes a change (types a single letter, for example), other peers receive this update as a prosemirror transaction that replaces the entire document content. This happens at

let tr = this._tr.replace(0, this.prosemirrorView.state.doc.content.size, new PModel.Slice(new PModel.Fragment(fragmentContent), 0, 0))
.

This makes it very hard for prosemirror plugins to manage their state in state apply functions. For example, when such an update occurs, tr.mapping.map(oldPos) will always point at the end of the entire document (except 0, which maps to 0).

y-prosemirror manually fixes the main consequence of this (that the current text selection/cursor location would otherwise be lost) with restoreRelativeSelection, but any prosemirror plugins that need to deal with positions in the document cannot use standard prosemirror mechanisms for tracking shifts in position.

One of the major consequences of this is that any plugins that compute decorations will have to recompute all that state any time a peer makes a single letter change. We have a custom spellcheck plugin that suffers from this, and other plugins like TipTap's syntax highlighting plugin also suffer - any time a peer makes a change, all code blocks most recompute their entire syntax highlighting because the prosemirror transaction from y-prosemirror spanned and replaced the entire document, preventing correlation of the old nodes to the new ones. For large documents and on devices with limited CPU (e.g. mobile), this causes very significant performance issues.

To Reproduce

  1. Open two editors using y-prosemirror. E.g. https://tiptap.dev/ - refresh until both browsers are in the same room.
  2. Open devtools in one browser, put a breakpoint anywhere that receives prosemirror transactions. E.g. in some plugin's state apply function.
  3. Type a letter in the other browser. You may have to continue past peer cursor position change transactions.
  4. On the transaction that represents the remote peer's minor change, observe that tr.mapping.map() produces useless results for any input value, and that the single step in the transaction replaces the entire document content rather than surgically updating the single text node that actually changed.

Expected behavior
y-prosemirror commits updates received from the Yjs doc as precise, surgical updates rather than replacing the entire document.

Screenshots
image

Environment Information

  • Chrome
  • Yjs version:
    • yjs@13.5.38
    • y-prosemirror@1.1.0
    • y-webrtc@10.2.3
@ascott18
Copy link
Author

I am able to tentatively resolve this by integrating https://github.com/sueddeutsche/prosemirror-recreate-transform to calculate the specific diffs between the incoming yjs update and the current prosemirror doc. Of course, it would be better if this could be computed directly from the yjs transaction in the same manner that it is computed when going from prosemirror => yjs:

diff --git a/node_modules/y-prosemirror/src/plugins/sync-plugin.js b/node_modules/y-prosemirror/src/plugins/sync-plugin.js
index 2a83da1..c5f0f78 100644
--- a/node_modules/y-prosemirror/src/plugins/sync-plugin.js
+++ b/node_modules/y-prosemirror/src/plugins/sync-plugin.js
@@ -382,8 +382,38 @@ export class ProsemirrorBinding {
       transaction.changedParentTypes.forEach(delType)
       const fragmentContent = this.type.toArray().map(t => createNodeIfNotExists(/** @type {Y.XmlElement | Y.XmlHook} */ (t), this.prosemirrorView.state.schema, this.mapping)).filter(n => n !== null)
       // @ts-ignore
-      let tr = this._tr.replace(0, this.prosemirrorView.state.doc.content.size, new PModel.Slice(new PModel.Fragment(fragmentContent), 0, 0))
-      restoreRelativeSelection(tr, this.beforeTransactionSelection, this)
+ 
+      // BEGIN https://github.com/yjs/y-prosemirror/issues/113
+      const { recreateTransform } = require("@technik-sde/prosemirror-recreate-transform");
+      let tr = this._tr;
+      const newContent = new PModel.Fragment(fragmentContent)
+      const newDoc = this.prosemirrorView.state.schema.node("doc", {}, newContent); 
+
+      let transform = recreateTransform(
+        this.prosemirrorView.state.doc, 
+        newDoc, 
+        {
+            complexSteps: true, // Whether step types other than ReplaceStep are allowed.
+            wordDiffs: false, // Whether diffs in text nodes should cover entire words.
+            simplifyDiffs: true // Whether steps should be merged, where possible
+        }
+      );
+      for (const step of transform.steps) {
+        tr = tr.step(step)
+      } 
+
+      // Safety check: Make sure that `recreateTransform` did the right thing.
+      // If there's a diff, fall back on the old behavior of y-prosemirror and replace the whole content.
+      const diffStart = newContent.findDiffStart(tr.doc.content)
+      if (diffStart !== null) {
+        console.error("recreateTransform produced a possibly incorrect transform. Falling back on old behavior.");
+
+        tr = this._tr.replace(0, this.prosemirrorView.state.doc.content.size, new PModel.Slice(new PModel.Fragment(fragmentContent), 0, 0))
+        restoreRelativeSelection(tr, this.beforeTransactionSelection, this)
+      }
+      // END https://github.com/yjs/y-prosemirror/issues/113
+
+      
       tr = tr.setMeta(ySyncPluginKey, { isChangeOrigin: true })
       if (this.beforeTransactionSelection !== null && this._isLocalCursorInView()) {
         tr.scrollIntoView()

@jjgmckenzie
Copy link

jjgmckenzie commented May 25, 2023

Any update on this? The prosemirror-recreate-transform package seems like the only current solution. Is there a better approach?

edit: for anyone else looking; the current version of prosemirror-recreate-transform is hosted at a different fork than the one listed above, fixing several security vulnerabilities and an issue with webpack

https://github.com/fellowapp/prosemirror-recreate-transform
https://www.npmjs.com/package/@fellow/prosemirror-recreate-transform

additionally, if you only have one, custom plugin that is affected, you can use something along these lines:

apply(tr, decorationSet) {
    const mapping = recreateTransform(
        tr.before,
        tr.doc,
        {
            complexSteps: true,
            wordDiffs:false,
            simplifyDiff:true,
        }
    ).mapping
    return decorationSet.map(mapping,tr.doc)
}

@tommoor
Copy link
Contributor

tommoor commented Aug 16, 2023

Previously discussed in the community here too, https://discuss.yjs.dev/t/decorationsets-and-remapping-broken-with-y-sync-plugin/845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants