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

[lexical-yjs] Bug Fix: handle text node being split by Yjs redo #7098

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

james-atticus
Copy link
Contributor

@james-atticus james-atticus commented Jan 26, 2025

Description

Fixes a bug in applyChildrenYjsDelta which caused Lexical and Yjs to get out of sync. Once in this state, some changes would not sync to other clients; the changes would be lost on refresh.

This can be reproduced in the Playground with the following steps:

  1. In the left editor, type "Hello World"
  2. Select "World" and change the formatting to bold
  3. In the right editor, type a character in the middle of "World", eg "Wor_ld"
  4. In the left editor, click undo then redo
  5. Any text added at the end in the left editor won't be synced to the right editor

When applying change deltas from Yjs, an insertDelta can be a string or a sharedType. If currIndex is pointing to some position within a text node and insertDelta is a string, then the new text is spliced in correctly. If insertDelta represents a shared type, then the newly created collabNode is inserted in the wrong position (before said text node).

This PR updates the logic in applyChildrenYjsDelta to handle inserting a sharedType node in the middle of a text node.

Test plan

The repro steps above are covered in an end-to-end test. I added a second E2E test to cover the case where the first insertDelta corresponds to an ElementNode, then the second delta is the TextNode.

Before

Lexical.applyChildrenYjsDelta.before.mov

After

Lexical.applyChildrenYjsDelta.after.mov

Copy link

vercel bot commented Jan 26, 2025

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 Jan 26, 2025 10:28am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 26, 2025 10:28am

@facebook-github-bot 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 Jan 26, 2025
Copy link

github-actions bot commented Jan 26, 2025

size-limit report 📦

Path Size
lexical - cjs 29.07 KB (0%)
lexical - esm 28.86 KB (0%)
@lexical/rich-text - cjs 38.04 KB (0%)
@lexical/rich-text - esm 30.92 KB (0%)
@lexical/plain-text - cjs 36.55 KB (0%)
@lexical/plain-text - esm 28.22 KB (0%)
@lexical/react - cjs 39.85 KB (0%)
@lexical/react - esm 32.28 KB (0%)

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done an audit of exactly how this works but it seems unlikely to be problematic given that it passes the existing e2e suite and the new test. Locally the change looks like it makes sense.

@etrepum etrepum added this pull request to the merge queue Jan 26, 2025
Merged via the queue into facebook:main with commit 212b70f Jan 26, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants