[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

mike-atticus · 2024-09-26T04:10:14Z

Description

Adds fix for #6614 where using undo in a collaborative session results in the Lexical editor content getting out of sync with the Yjs doc. The undo operation deletes the YMap representing the text node, and the original content string, but it leaves another collaborator's content string dangling at the end. Even though the dangling text is cleaned up on page reload, before that any document edits can cause further document corruption / loss of data.

See #6670 (comment) for a more detailed explanation.

Test plan

Before

No tests to detect the collaboration bug that leads to Yjs and Lexical getting out of sync after one user clicks Undo.

After

Playwright tests added which use the side-by-side collaborative playground editors to reproduce the behaviour and confirm that dangling text is cleaned up.

vercel · 2024-09-26T04:10:18Z

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	Nov 5, 2024 7:40pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 5, 2024 7:40pm

github-actions · 2024-09-26T04:12:00Z

size-limit report 📦

Path	Size
lexical - cjs	29.92 KB (0%)
lexical - esm	29.78 KB (0%)
@lexical/rich-text - cjs	38.57 KB (0%)
@lexical/rich-text - esm	31.63 KB (0%)
@lexical/plain-text - cjs	37.22 KB (0%)
@lexical/plain-text - esm	28.94 KB (0%)
@lexical/react - cjs	40.33 KB (-0.16% 🔽)
@lexical/react - esm	33.06 KB (0%)

potatowagon · 2024-10-02T10:39:56Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

@@ -230,4 +230,92 @@ test.describe('Collaboration', () => {
      focusPath: [1, 1, 0],
    });
  });
+
+  test('Undo with two collaborators editing same paragraph', async ({


this is failing on the ci :(

the failure is reproducible across different envs so i dont think its a flaky failure

That's the point of this PR, to demonstrate that bug

Yea I created this test in case it helps others with reproducing the bug and confirming a fix, but it's not something that can be merged on its own. Let me know if I should switch this to a draft PR for now, if that's clearer for PR queue management.

i see, thanks for filing a detailed issue. if its not meant to be merged, converting to draft or adding a [do not merge] to title would work

Title updated 👍

james-atticus · 2024-10-29T05:03:07Z

@etrepum @ivailop7: @mike-atticus and I spent some more time on this and we think we have a fix...

Our understanding of the issue

When creating a new paragraph, it starts off with no children.

When a user types 'hello', we add two things to the paragraph's CollabElementNode _xmlText: a YMap which represents the TextNode and the string 'hello'. These are added to YJS's undo stack together.

The second user then types 'world', which adds only the string 'world' to _xmlText.

When the first user then clicks Undo, both the YMap and 'hello' are deleted, however 'world' remains in _xmlText. @trueadm added handling for this case in here, cleaning up this dangling text when the document is reloaded. However, until that point, Lexical and YJS are out of sync.

Proposed fix

The same PR above added some logic for detecting when a text node was deleted leaving dangling text, and tried to merge said text into the previous text node. However, the checks were too strict: the YMap can be deleted when delCount > 1, as is the case with undo above.

The first change in this PR is fixing the check to properly handle cases when the YMap has been deleted from YJS (if (offset === 0 && length === nodeSize) ...). The other part is updating the dangling text handling:

Get the dangling text with spliceString
If there's a preceding text node, move the text to that node
If there's no preceding text, remove the text from _xmlText; i.e.: the same thing as @trueadm's original fix but immediately

Note: this slightly differs from the test case that @mike-atticus originally submitted. At the time, we didn't really understand how Lexical and YJS synced with each other. Once we found @trueadm's PR, we realised the text node is fully deleted from YJS with the undo operation, so it was basically the same issue. Given this TODO about a "proper" fix, we opted to keep it simple and delete the dangling text.

Next steps

If Lexical you're are happy with this approach, we'll move the tests over to Collaboration.test.ts so that we can add checks on the YJS state and speed up the tests (no need for sleep).

etrepum · 2024-10-30T00:47:14Z

The analysis and proposed fix makes sense to me, although I have never done an audit of the collab code so I could be missing something. I think the important things to consider are:

Is the fix largely backwards compatible (at least such that coherent documents work the same way)? If not, is there another approach that would be more compatible?
Does it add new edge cases?

The fix sounds simple enough that I think it would be backwards compatible and not cause new trouble, but it's hard to see without at least observing a full e2e collab test suite since there really is a lot going on in here.

Thank you for investigating further!

james-atticus

Is the fix largely backwards compatible?

I believe so, yes. We're only changing logic for processing delete deltas from YJS, which shouldn't impact loading existing documents (pretty sure YJS just gives you insert events on load). For existing documents that have dangling text at the start of a paragraph, that text will still be deleted from YJS on load by the logic here.

Does it add new edge cases?

Not that we could think of. The change simplifies the overall deletion-handling logic; previously there were two branches that called children.splice(nodeIndex, 1), each in very specific circumstances (either delCount === 1 or delCount === nodeSize). Now it's "if YMap is deleted, always splice the CollabTextNode out", regardless of delCount, with some logic to either move dangling text to another TextNode or clean it up from YJS (delCount is used to determine what part of the text is left over).

We've pushed the code for the proposed fix as well as the two E2E tests that were failing before the change, covering the two paths for handling of dangling text. Are these what you meant by "full e2e collab test suite"? In case it's helpful for understanding how we got to these tests, I've added some comments inline highlighting where they're failing on main.

james-atticus · 2024-10-30T03:10:39Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

+    await assertHTML(
+      page,
+      html`
+        <p
+          class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
+          dir="ltr">
+          <span data-lexical-text="true">Line 1</span>
+        </p>
+        <p class="PlaygroundEditorTheme__paragraph"><br /></p>
+      `,
+    );


Fails on main with:

expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 5 Line 1 - + + ord + 

The text is ord because neither of the two if checks pass, so Lexical ends up in node._text = spliceString(node._text, offset, delCount, '');. delCount is the length of the string from the left user, plus 1 for the deleted YMap, resulting in the W also being removed.

james-atticus · 2024-10-30T03:12:39Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

+    await assertHTML(
+      page,
+      html`
+        <p
+          class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
+          dir="ltr">
+          <span data-lexical-text="true">normalBOLD</span>
+        </p>
+      `,
+    );


Fails on main with:

expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 4 - normalBOLD + normal + + OLD + 

The B character is dropped for the same reason as above.

james-atticus · 2024-10-30T03:12:55Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

+    await assertHTML(
+      page,
+      html`
+        <p
+          class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
+          dir="ltr">
+          <span data-lexical-text="true">normalBOLD</span>
+        </p>
+      `,
+    );


On main, this assert passes for the left editor (because the refresh got it back in sync) but not the right editor (which will only get back in sync when it refreshes).

james-atticus · 2024-10-30T03:20:43Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

+    await assertHTML(
+      page,
+      html`
+        <p
+          class="PlaygroundEditorTheme__paragraph PlaygroundEditorTheme__ltr"
+          dir="ltr">
+          <span data-lexical-text="true">Line 1</span>
+        </p>
+        <p class="PlaygroundEditorTheme__paragraph"><br /></p>
+      `,
+    );


If you comment out the assert above, this passes on main because of this handling. The left editor's change is synced over to the right editor.

james-atticus · 2024-10-30T03:26:27Z

packages/lexical-yjs/src/CollabElementNode.ts

-              children.splice(nodeIndex, 1);
-            } else if (offset === 0 && delCount === nodeSize) {
-              // The entire thing needs removing
+            if (offset === 0 && length === nodeSize) {


Our observation is that length < nodeSize when you're just deleting some text from the node, and length === nodeSize if the YMap has been deleted. This matches up with the old code's first if branch (which also had Array.from(node._map.keys()).length === 0 but we couldn't see why that was necessary). It also aligns with the second branch delCount === nodeSize, where delCount = min(length, deletionSize).

james-atticus · 2024-10-30T03:35:33Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

+    // test.skip(!isCollab || IS_MAC);
+    test.skip(!isCollab);


Ditto below

Suggested change

// test.skip(!isCollab || IS_MAC);

test.skip(!isCollab);

test.skip(!isCollab || IS_MAC);

Is there a reason this should be skipped on Mac? I am not seeing any platform specific code here, I think what this will do is make it harder for contributors to test locally on a Mac because some of the tests just won't run.

No particular reason, just pattern matching existing tests. Works fine on Mac so happy to leave as is.

ivailop7 · 2024-10-31T08:52:44Z

Pretty confident this is backwards-compatible too. It doesn't modify the stored Ydoc and how to interpret it. I'm happy with the change and tests.

etrepum

Given that all the test suites seem to be passing now I don't see any reason not to approve and merge this fix with additional tests. Thank you very much for doing all the hard work on this one!

mike-atticus requested review from zurfyx, fantactuka, acywatson, Fetz, ivailop7, Sahejkm and potatowagon as code owners September 26, 2024 04:10

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 26, 2024

vercel bot deployed to Preview – lexical September 26, 2024 04:11 View deployment

vercel bot deployed to Preview – lexical-playground September 26, 2024 04:11 View deployment

mike-atticus mentioned this pull request Sep 26, 2024

Bug: undo causes unexpected paragraph deletion in collaborative editor #6614

Open

potatowagon added the extended-tests Run extended e2e tests on a PR label Oct 2, 2024

potatowagon reviewed Oct 2, 2024

View reviewed changes

mike-atticus changed the title ~~[lexical-yjs][lexical-playground] Chore: Add test for collaboration undo bug~~ [do not merge][lexical-yjs][lexical-playground] Chore: Add test for collaboration undo bug Oct 3, 2024

Add test for collaboration undo bug

0d062f6

james-atticus force-pushed the collaboration-undo-bug-test branch from 9df14f7 to 2c811ef Compare October 29, 2024 04:58

vercel bot deployed to Preview – lexical-playground October 29, 2024 05:00 View deployment

vercel bot deployed to Preview – lexical October 29, 2024 05:00 View deployment

Fix: clean up dangling text in YJS

4fbe7fe

james-atticus force-pushed the collaboration-undo-bug-test branch from 2c811ef to 4fbe7fe Compare October 29, 2024 05:01

vercel bot deployed to Preview – lexical October 29, 2024 05:02 View deployment

vercel bot deployed to Preview – lexical-playground October 29, 2024 05:02 View deployment

james-atticus reviewed Oct 30, 2024

View reviewed changes

mike-atticus changed the title ~~[do not merge][lexical-yjs][lexical-playground] Chore: Add test for collaboration undo bug~~ [lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration Oct 30, 2024

remove unused comments

a40b5b9

vercel bot deployed to Preview – lexical-playground November 4, 2024 05:09 View deployment

vercel bot deployed to Preview – lexical November 4, 2024 05:09 View deployment

Merge branch 'main' into collaboration-undo-bug-test

1d43245

vercel bot deployed to Preview – lexical November 5, 2024 04:22 View deployment

vercel bot deployed to Preview – lexical-playground November 5, 2024 04:22 View deployment

Merge branch 'main' into collaboration-undo-bug-test

8bc45e6

vercel bot deployed to Preview – lexical-playground November 5, 2024 19:40 View deployment

vercel bot deployed to Preview – lexical November 5, 2024 19:40 View deployment

etrepum approved these changes Nov 5, 2024

View reviewed changes

etrepum added this pull request to the merge queue Nov 5, 2024

Merged via the queue into facebook:main with commit dd65f59 Nov 5, 2024
40 checks passed

mike-atticus deleted the collaboration-undo-bug-test branch November 5, 2024 23:15

etrepum mentioned this pull request Nov 7, 2024

v0.20.0 #6809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

mike-atticus commented Sep 26, 2024 •

edited

Loading

vercel bot commented Sep 26, 2024 •

edited

Loading

github-actions bot commented Sep 26, 2024 •

edited

Loading

potatowagon Oct 2, 2024

etrepum Oct 2, 2024

mike-atticus Oct 2, 2024

potatowagon Oct 3, 2024

mike-atticus Oct 3, 2024

james-atticus commented Oct 29, 2024 •

edited

Loading

etrepum commented Oct 30, 2024

james-atticus left a comment

james-atticus Oct 30, 2024 •

edited

Loading

james-atticus Oct 30, 2024 •

edited

Loading

james-atticus Oct 30, 2024

james-atticus Oct 30, 2024

james-atticus Oct 30, 2024

james-atticus Oct 30, 2024

etrepum Nov 2, 2024

james-atticus Nov 4, 2024

ivailop7 commented Oct 31, 2024

etrepum left a comment

	// test.skip(!isCollab \|\| IS_MAC);
	test.skip(!isCollab);
	test.skip(!isCollab \|\| IS_MAC);

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

[lexical-yjs] Bug Fix: clean up dangling text after undo in collaboration #6670

Conversation

mike-atticus commented Sep 26, 2024 • edited Loading

Description

Test plan

Before

After

vercel bot commented Sep 26, 2024 • edited Loading

github-actions bot commented Sep 26, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-atticus commented Oct 29, 2024 • edited Loading

etrepum commented Oct 30, 2024

james-atticus left a comment

Choose a reason for hiding this comment

james-atticus Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

james-atticus Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivailop7 commented Oct 31, 2024

etrepum left a comment

Choose a reason for hiding this comment

mike-atticus commented Sep 26, 2024 •

edited

Loading

vercel bot commented Sep 26, 2024 •

edited

Loading

github-actions bot commented Sep 26, 2024 •

edited

Loading

james-atticus commented Oct 29, 2024 •

edited

Loading

james-atticus Oct 30, 2024 •

edited

Loading

james-atticus Oct 30, 2024 •

edited

Loading