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

Fix reconciler recovery listeners #4654

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Fix reconciler recovery listeners #4654

merged 1 commit into from
Jun 14, 2023

Conversation

fantactuka
Copy link
Contributor

@fantactuka fantactuka commented Jun 13, 2023

When reconciler fails we try to recover editor by reseting it to empty state and then doing full reconciliation for pendingEditorState. If that goes through, we trigger listeners but the problem is that since we reset editor during recovery prevEditorState will be empty, while it's expected to be whatever state was before changes that caused reconciler failre. This PR adjusts commitPendingUpdates to ensure it uses proper prevEditorState for those listeners.

This issue affecting collab in its worst way since collab heavily relies on prevEditorState from update listener to calculate Yjs changes for dirty nodes. And since in reconciler recovery scenario prev state is empty, collab assumes that all mutated nodes didn't exist before and re-inserts them causing content duplication. Here's an example of MentionNode that has error thrown inside updateDOM method:

Screen.Recording.2023-06-13.at.2.33.55.PM.mov

@vercel
Copy link

vercel bot commented Jun 13, 2023

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

Name Status Preview Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview Jun 13, 2023 6:36pm
lexical-playground ✅ Ready (Inspect) Visit Preview Jun 13, 2023 6:36pm

@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 Jun 13, 2023
@github-actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 27.9 KB (+0.05% 🔺) 559 ms (+0.05% 🔺) 202 ms (-36.34% 🔽) 760 ms
packages/lexical-rich-text/dist/LexicalRichText.js 38.85 KB (+0.04% 🔺) 778 ms (+0.04% 🔺) 185 ms (-17.96% 🔽) 962 ms
packages/lexical-plain-text/dist/LexicalPlainText.js 38.83 KB (+0.04% 🔺) 777 ms (+0.04% 🔺) 208 ms (-35.58% 🔽) 985 ms

@@ -645,8 +650,6 @@ function triggerTextContentListeners(

function triggerMutationListeners(
editor: LexicalEditor,
currentEditorState: EditorState,
pendingEditorState: EditorState,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just cleanup, we didn't use these args

@fantactuka fantactuka merged commit 5162dd2 into main Jun 14, 2023
@fantactuka fantactuka deleted the reconciler-recovery branch June 14, 2023 15:02
@zurfyx zurfyx mentioned this pull request Jul 11, 2023
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.

3 participants