yjs/y-prosemirror

[Major bug] data loss in collaborative scenario

lijie1129 opened this issue · 7 comments

lijie1129 commented

Describe the bug
Using the demo in y-prosemirror, data is lost in the collaborative scenario.

To Reproduce
Steps to reproduce the behavior:

  1. git clone https://github.com/yjs/y-prosemirror.git
  2. cd y-prosemirror && npm install && npm run build && npm run start
  3. Open Chrome & Edge open the http://127.0.0.1:8080/demo/prosemirror.html
  4. Enter some characters at either peer
  5. Click the "disconnect" button to take both peers offline
  6. Input some new characters at one peer; The other peer deletes all characters
  7. Click the "connect" button to go online and find that all contents are lost

Expected behavior
Keep the newly added characters at one peer instead of losing them all.

Recording screen

480p.mov

Environment Information

  • NodeJS v16.13.1
  • Chrome 101.0.4951.64
  • Microsoft Edge 101.0.1210.39
  • y-prosemirror master branch
cobbcheng commented

This issue has led to the frequent loss of data when cooperating in the table.

lijie1129 commented

I think the scenario handled by this line of code may contain cases of lost data.

y-prosemirror/src/plugins/sync-plugin.js

Line 823 in e3755b1

yDomFragment.delete(left, yDelLen)

Suppose that optimizing the logic here will help? Similar to the following code:

      if (left === 0 && yDelLen === 1 && yDomFragment.firstChild) {
        yDomFragment.firstChild.delete(left, yDomFragment.firstChild.length)
      } else {
        yDomFragment.delete(left, yDelLen)
      }
flaviouk commented

This is the behaviour as far as I know, if you add more than one paragraph while offline, that new paragraph that no one else has will be visible, but if it's the same paragraph the delete keeps the online user intent.
There are also similar issues with lists, when indenting forward/backwards, nodes are marked as deleted and re-added which is something I wasn't expecting at all, but currently, yjs doesn't have move operations, definitely no move operation in this prosemirror binding, but there are ways around it.
Same thing when joining backwards (which deletes the node)

  • 👍1
lijie1129 commented

The code I provided above may introduce the problem of missing styles (injection: bold, italic), because when deleting the last character of the node, the current yDomFragment is a paragraph, while its first child is a text node (yXmlText), while the strong tag representing bold does not create the corresponding yXmlElement, so when deleting yDomFragment After the text content of FirstChild, the strong tag is removed, and the bold style is lost.

The following is the screen recording of the lost style：

720-3.mp4

I think the scenario handled by this line of code may contain cases of lost data.

y-prosemirror/src/plugins/sync-plugin.js

Line 823 in e3755b1

yDomFragment.delete(left, yDelLen)

Suppose that optimizing the logic here will help? Similar to the following code:

      if (left === 0 && yDelLen === 1 && yDomFragment.firstChild) {
        yDomFragment.firstChild.delete(left, yDomFragment.firstChild.length)
      } else {
        yDomFragment.delete(left, yDelLen)
      }
  • 👍1
lijie1129 commented

This is the behaviour as far as I know, if you add more than one paragraph while offline, that new paragraph that no one else has will be visible, but if it's the same paragraph the delete keeps the online user intent. There are also similar issues with lists, when indenting forward/backwards, nodes are marked as deleted and re-added which is something I wasn't expecting at all, but currently, yjs doesn't have move operations, definitely no move operation in this prosemirror binding, but there are ways around it. Same thing when joining backwards (which deletes the node)

Hi，@flaviouk

Thank you for your supplement and reminder. I don't have the energy to explore the problem of the list, but I believe that the loss of data must be the highest priority problem to be solved. At present, I don't think the solution I provided above is elegant enough and straight to the key of the problem. You and other friends are welcome to provide better solutions.

  • 👍1
lijie1129 commented

This is the behaviour as far as I know, if you add more than one paragraph while offline, that new paragraph that no one else has will be visible, but if it's the same paragraph the delete keeps the online user intent. There are also similar issues with lists, when indenting forward/backwards, nodes are marked as deleted and re-added which is something I wasn't expecting at all, but currently, yjs doesn't have move operations, definitely no move operation in this prosemirror binding, but there are ways around it. Same thing when joining backwards (which deletes the node)

Thank you for your supplement and reminder. I don't have the energy to explore the problem of the list, but I believe that the loss of data must be the highest priority problem to be solved. At present, I don't think the solution I provided above is elegant enough and straight to the key of the problem. You and other friends are welcome to provide better solutions.

Hi, @dmonad

We also look forward to any suggestions from you. :)

dmonad commented

Thanks for the suggestions. ProseMirror Text nodes are mapped to Y.Text objects. However, when ProseMirror deletes a Text object, we do the same, which results in deleting the Y.Text objects and all future changes that are applied to it.

In this simple case, we can simply retain the Y.Text object. I think this might even fix the table and list issue.

I implemented a fix which I will release today.

  • 🎉2