Repository navigation
Conversation
… as the document keeps it ## Description When remote changes leave an element with no children (two collaborators each remove one item of a two-item list at the same time), `syncYjsChangesToLexicalV2__EXPERIMENTAL` removed the old children with `ElementNode#splice`. With a range selection, `splice` also removes an element that cannot be empty (`ListNode#canBeEmpty` is false), so an editor with a caret dropped the list, while the Y document keeps the empty list element and the binding kept it mapped to the removed node. That editor then differed from a fresh reader of its document, and a later change to the list (an item a collaborator adds to it) failed with "Lexical node does not exist in active editor state". Editors without a selection kept the empty list. Now, when the Y element holds no children any more, the old children are removed one by one with the element kept, as the document keeps it, whatever the selection. Closes #9592 ## Changeset `.changeset/lexical-yjs-v2-keep-emptied-element.md` (`@lexical/yjs`, `[fix]`). ## Test plan `V2EmptiedList.test.ts`: the issue's three-client case, with the receiving editor's selection after the list, in the first item, in the second item, across the list, or none. Each checks the receiver against a fresh reader of its document, then has a collaborator add an item to the empty list and checks every client against it. ``` pnpm exec vitest run --project unit packages/lexical-yjs/src/__tests__/unit/V2EmptiedList.test.ts ``` ### Before ``` × v2: a list emptied by two clients' removals, the receiver with a caret after the list × v2: a list emptied by two clients' removals, the receiver with a caret in the first item × v2: a list emptied by two clients' removals, the receiver with a caret in the second item × v2: a list emptied by two clients' removals, the receiver with a selection across the list ✓ v2: a list emptied by two clients' removals, the receiver with no selection AssertionError: expected { root: { children: [ …(2) ], …(7) } } to deeply equal { root: { children: [ …(3) ], …(7) } } Tests 4 failed | 1 passed (5) ``` ### After ``` Tests 5 passed (5) ``` `pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit`: ``` Test Files 54 passed (54) Tests 524 passed | 4 expected fail (528) ``` `pnpm run ci-check` (each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass; `eslint --no-cache packages/lexical-yjs` passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 30, 2026 15:08
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
|
AI review status for this pull request.
|
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good to me. Keeping the emptied element matches what the Y document holds, so the receiving editor now agrees with a fresh reader of the document, which is also how collab v1 behaves (CollabElementNode.syncChildrenFromYjs detaches removed children with $removeFromParent and keeps the parent). The new test fails 4 of its 5 cases on main (cad90155) and passes all 5 here. The new branch also runs whenever a collaborator deletes all of a paragraph's text, and there the receiver's document and caret come out the same as on main; the same holds when remote removals empty the root.
Optional, non-blocking suggestions:
packages/lexical-yjs/src/__tests__/unit/V2EmptiedList.test.ts:229: the cases with a caret in the list don't assert where B's caret ends up. With this change it lands at offset 0 inside the now-emptyListNode(anelementpoint), and typing there adds a list item that reaches every peer. Asserting that point afterb.sync(c), or adding aninsertTextstep, would pin the selection repair this fix relies on.packages/lexical-yjs/src/SyncV2.ts:365:child.remove(true)goes through each node'sremove()override, which thesplicepath never did. The only override in the repository,ListItemNode.remove, merges nested lists only when both siblings exist, so it cannot fire when children are removed first to last; a few words in the comment would save the next reader that check.
| // B's editor shows what its document holds: an empty list. | ||
| const fresh = new Peer(99); | ||
| fresh.sync(b); | ||
| expect(b.json()).toEqual(fresh.json()); |
There was a problem hiding this comment.
Optional, non-blocking: the cases with a caret in the list don't assert where B's caret ends up. With this change it lands at offset 0 inside the now-empty ListNode (an element point), and typing there adds a list item that reaches every peer. Asserting that point here (or adding an insertText step on B) would pin the selection repair the fix relies on.
Member
Author
There was a problem hiding this comment.
Thanks, this found a real gap. The caret did land at offset 0 in the empty ListNode, but typing there put a text node straight into the list on that client, while the other clients read the stored text as a list item, so they diverged. Fixed in bf3cbaa: a selection point left in an emptied element that cannot be empty moves beside it, where Lexical moves a point out of a node it removes (as main did when it dropped the list). The test now asserts the caret ends at the end of the paragraph before the list and has B type there, checking every client and a fresh reader; each case runs in both client id orders.
| // remove an element that cannot be empty, such as a list, and the Y | ||
| // element would stay mapped to a removed node.) | ||
| for (const child of node.getChildren()) { | ||
| child.remove(true); |
There was a problem hiding this comment.
Optional, non-blocking: remove(true) dispatches to each child's remove() override, which the splice path never did. The only override in the repository, ListItemNode.remove, merges nested lists only when both siblings exist, so it cannot fire when children are removed first to last. A few words in the comment above would save the next reader that check.
Member
Author
There was a problem hiding this comment.
Done in bf3cbaa: the comment now says that remove() runs each child's override and why ListItemNode's nested-list merge cannot fire when children are removed first to last.
…2 empties, where removing it would have moved it ## Description With the first commit, an editor whose caret was in a list item that remote removals deleted kept the empty list, as the document does, but its caret stayed in the empty list (an element point at offset 0). Typing there put a text node straight into the `ListNode` on that client, while the other clients read the stored text as a list item, so they diverged. On main this did not arise because the list was dropped and Lexical moved the caret out of it. Now, when the emptied element cannot be empty, a selection point left in it moves beside it, exactly where Lexical moves a point out of a node it removes: to the end of its previous sibling, else the start of its next sibling, else its parent at its index. The comment also notes why running each child's `remove()` override is safe here (`ListItemNode`'s merges nested lists only when an item has siblings on both sides, which never holds when children are removed first to last). ## Test plan `V2EmptiedList.test.ts` now runs each receiver selection in both client id orders, checks that a caret that was in a removed item ends at the end of the paragraph before the list, and then has the receiver type where its selection is and checks every client and a fresh reader against it. ``` pnpm exec vitest run --project unit packages/lexical-yjs/src/__tests__/unit/V2EmptiedList.test.ts ``` ### Before On the first commit (7b00d5d): ``` × v2: a list emptied by two clients' removals, the receiver with a caret in the first item (client ids 1,2,3) × v2: a list emptied by two clients' removals, the receiver with a caret in the first item (client ids 3,2,1) × v2: a list emptied by two clients' removals, the receiver with a caret in the second item (client ids 1,2,3) × v2: a list emptied by two clients' removals, the receiver with a caret in the second item (client ids 3,2,1) AssertionError: expected '' to be 'before' // Object.is equality Tests 4 failed | 6 passed (10) ``` On main: `Tests 8 failed | 2 passed (10)`. ### After ``` Tests 10 passed (10) ``` `pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit`: ``` Test Files 54 passed (54) Tests 529 passed | 4 expected fail (533) ``` `pnpm run ci-check` (each script, run one at a time): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow (`Found 0 errors`), prettier (no files listed), lint, check-changesets pass; `eslint --no-cache packages/lexical-yjs` passes.
Member
Author
|
Closing without merging: |
zurfyx
added a commit
that referenced
this pull request
Oct 30, 2026
…ote the v1/v2 fixes merged and closed ## Description Two rows of the `@lexical/y` decision register did not match the code or the pull request history: - D72 said the binding's first read is not deferred until every extension registers. The binding starts in `YCollabExtension`'s `afterRegistration`, once every extension has registered (the `TableOnOpen` test comment #9537 corrects says the same). The row now says that, and that the decision is no further deferral or special-casing: an initialization-order bug is fixed in the extension that has it, as #9501 did. - D12 (no more fixes to `@lexical/yjs` v1 and v2) now notes that one v2 table fix, #9584, merged on 10-30 before the row was checked, and that later v1 and v2 fix pull requests, #9595 and #9596, were closed under it. ## Changeset None: documentation only. ## Test plan ### Before D72 contradicted `packages/lexical-y/src/internal/YExtension.ts`, which starts the binding in `afterRegistration`. ### After ``` $ pnpm run ci-check ... Changeset check passed: 31 pending changeset(s) valid. ... Found 0 errors ```
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When remote changes leave an element with no children (two collaborators each remove one item of a two-item list at the same time),
syncYjsChangesToLexicalV2__EXPERIMENTALremoved the old children withElementNode#splice. With a range selection,splicealso removes an element that cannot be empty (ListNode#canBeEmptyis false), so an editor with a caret dropped the list, while the Y document keeps the empty list element and the binding kept it mapped to the removed node. That editor then differed from a fresh reader of its document, and a later change to the list (an item a collaborator adds to it) failed with "Lexical node does not exist in active editor state". Editors without a selection kept the empty list.Now, when the Y element holds no children any more, the old children are removed one by one with the element kept, as the document keeps it, whatever the selection. When that element cannot be empty, a selection point left in it moves beside it, where Lexical moves a point out of a node it removes (as on main, where the list was removed): otherwise typing there would put a text node straight into the list on that client, which the other clients read as a list item.
Closes #9592
Changeset
.changeset/lexical-yjs-v2-keep-emptied-element.md(@lexical/yjs,[fix]).Test plan
V2EmptiedList.test.ts: the issue's three-client case, with the receiving editor's selection after the list, in the first item, in the second item, across the list, or none. Each checks the receiver against a fresh reader of its document, then has a collaborator add an item to the empty list and checks every client against it.Before
After
After the second commit (caret moved beside the emptied element; each case in both client id orders; the receiver types after the removals and every client and a fresh reader agree):
Tests 10 passed (10)(the first commit fails the 4 caret-in-item cases; main fails 8).pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit:pnpm run ci-check(each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass;eslint --no-cache packages/lexical-yjspasses.