[lexical-yjs] Bug Fix: Fix text duplication bug in collaborative editors

[amanharwara]

Description

When using collaboration with shouldBootstrap set to false, you start with an empty root. A paragraph is added when the user actually starts typing.

However, it is possible to undo the creation of this initial paragraph. A new empty paragraph is indeed created, but that is not correctly reflected in the Yjs tree. This causes a de-sync of the Lexical and Yjs trees, which then depending on what the user does can lead to either broken sync (some of the changes going missing) or text duplication (seen in the video below)

This issue has also been reported by other users in the Discord: https://discord.com/channels/953974421008293909/1240637271594762270/1240637271594762270

We noticed in our testing that even though the syncYjsChangesToLexical function creates a new paragraph if the root gets empty, the timing of the creation seems to be incorrect in a way where the Yjs tree won't be updated properly. Moving this logic to another editor.update() call in the onUpdate callback fixes this.

Steps to replicate

1. Have collaborative editor with shouldBootstrap set to false, so that you start with an empty root. There needs to be persistence of the Ydoc for this to happen
2. Make sure this is a new doc
3. Type a word and then undo it
4. Type something else, and press Enter to add a new paragraph
5. Select all and then type something else again
6. Reload the page

Test plan

Before

text-duplicaton-bug.mp4

After

text-duplication-fix.mp4

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 9, 2024 0:26am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 9, 2024 0:26am

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	28.52 KB (0%)
lexical - esm	28.32 KB (0%)
@lexical/rich-text - cjs	36.94 KB (0%)
@lexical/rich-text - esm	28.14 KB (0%)
@lexical/plain-text - cjs	35.53 KB (+0.06% 🔺)
@lexical/plain-text - esm	25.34 KB (0%)
@lexical/react - cjs	38.81 KB (0%)
@lexical/react - esm	29.31 KB (0%)

[amanharwara]

btw, it possible to replicate this with https://stackblitz.com/github/facebook/lexical/tree/fix/collab_example/examples/react-rich-collab?file=src%2FApp.tsx

simplescreenrecorder-2024-07-05_16.50.24.mp4

this makes the desync more obvious. also you'll notice at the end that the text also does get duplicated, just not as severely as in our reproduction as it seems to get cut off at 3 characters

[moughxyz]

I've put together a very minimal repro project here using Lexical 0.16.1: https://github.com/moughxyz/lexical-yjs-bug

Steps

Bootstrap

1. git clone git@github.com:moughxyz/lexical-yjs-bug.git
2. yarn install
3. yarn dev
4. Open browser to localhost:5173 (or whatever port Vite used)

Bug

1. Type test in the editor
2. Cmd/Ctrl + Z to undo
3. Type new, press enter, type text
4. Cmd/Ctrl + A to select all text, then type anything
5. Refresh the page

Please see video of the result:

repro-video.mov

[ivailop7]

@fantactuka or @StyleT could probably comment more

[amanharwara]

We did some more testing on our end and found out that this patch only fixes part of the problem.
In the "Select all" step for repro, if you select-all and then paste something instead of select-all and manually typing, you will come across this error:

This won't cause duplication, but if you reload the page, you will be greeted with an empty document. Because of the error, anything that was pasted doesn't get stored correctly in the Ydoc, and hence when you replay the updates on reload you see:

With the last update, the ydoc root is empty.

[amanharwara]

With some more testing, we've managed to figure out that to fix this issue in both of these scenarios, these lines (https://github.com/facebook/lexical/pull/6374/files#diff-faeb2a44b10d1affd1808827e738984026ed388ce861f20f18d498fa64a8bfe2R129-R133) need to be removed completely, and not just moved below like we do in this PR.

@etrepum Since these changes were introduced in this PR, could you add some insight as to why these changes were necessary?

When we remove that code so that no new paragraph is added, everything still seems to function correctly. Having that code there seems to cause the Yjs tree to get out of sync for whatever reason. Maybe we can add a didBootstrap flag to the syncYjsChangesToLexical function and only run this is code if didBootstrap is true? That way users of the CollabPlugin that don't have shouldBootstrap=false don't end up with these issues.

[amanharwara]

I've updated the fix according to my comment above, and also added a unit test in f3ccd44 (#6374) that covers this.

Is there a way in the e2e tests to persist data after reload? That way I can also add an e2e test that simulates both manual typing and copy-pasting scenarios.

[etrepum]

I couldn’t tell you why the code is there, I just moved it from somewhere else because it seemed less likely to cause test failures in that spot. You’d have to follow the git blame further back to figure out who introduced it and why.

[moughxyz]

I've created a followup PR that should be a little simpler, and hopefully more safely mergeable as it doesn't prevent the insertion of a paragraph altogether, but moves it to the appropriate location:

#6523

I hope we can move forward with this, as it is a high priority item that causes permanent data loss in a very easy to replicate manner.

[ivailop7]

#6523 covers this PR, closing this one.