Repository navigation
[lexical-yjs] Bug Fix: Map an equal subtree whole when collaboration v2 writes a local change - #9584
Conversation
…v2 writes a local change ## Description When the v2 binding writes a local change (`$updateYFragment`), it pairs the editor's children with the Y children from both ends. A child that is deep-equal to a Y child it is not mapped to (`$equalYTypePNode`), such as a table row equal to another row, was mapped to that Y type, but only at the top: its descendants kept their previous mappings, which can point at another subtree's nodes (the cells of an equal row). A later remote change to that row's children then read those cells and moved them into the wrong row. In #9581 one client's editor ends with a row that has no cells while its own document and the other clients have two; with other interleavings, a later read throws "Lexical node does not exist in active editor state", or the clients diverge. The same happens on main when one update removes a row's last cell and appends a new one (as padding a row does). Now an equality match maps the whole subtree, child by child, as the comparison paired them (slots are equal only when already mapped to each other, so they need nothing more). The v1 binding does not use this code. Closes #9581 ## Changeset `.changeset/lexical-yjs-v2-equal-subtree-mapping.md` (`@lexical/yjs`, `[fix]`). ## Test plan `V2TableEqualRows.test.ts`: the issue's three-peer script, and 60 seeded runs of three v2 peers editing a table whose rows are equal (row and column inserts, a one-update remove-and-append of a row's last cell, partial delivery, each channel in order), each checked against a fresh reader of the document. ``` pnpm exec vitest run --project unit packages/lexical-yjs/src/__tests__/unit/V2TableEqualRows.test.ts ``` ### Before ``` × v2 peers: every editor shows what its own document holds after concurrent table edits 96ms × seed 1: every editor shows its own document, and the peers agree 21ms × seed 2: every editor shows its own document, and the peers agree 26ms × seed 3: every editor shows its own document, and the peers agree 11ms Error: Lexical node does not exist in active editor state (key 419, type tablecell). The reference was probably kept from another read or update, for example in a closure, event handler or component Tests 58 failed | 3 passed (61) ``` ### After ``` Tests 61 passed (61) ``` `pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit packages/lexical-table packages/lexical-y`: ``` Test Files 230 passed | 2 skipped (232) Tests 8053 passed | 46 expected fail | 4 skipped (8103) ``` `pnpm run ci-check` (each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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: |
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the focused fix and the clear reproduction. $mapEqualSubtree pairs nodes exactly as $equalYTypePNode compared them, CollabV2Mapping.set then drops the stale cross-row entries, and the new test fails on main and passes here.
A few suggestions, none blocking:
-
The same stale mapping remains in the compare loop (
packages/lexical-yjs/src/SyncV2.ts:1137-1141). When the loop meets a text run equal to its Y text, nothing updates the mapping. To reproduce with two v2 peers:- Write a paragraph
link("a"), "middle", link("c"). - In one update, change both links' URLs and replace "middle" with a new text node holding the same text.
- Have the other peer type into "middle".
The first client throws
Lexical node does not exist in active editor stateand keeps showing "middle" while its document holds "middleX". This predates the PR, but it is the same class of bug. Addingbinding.mapping.set(leftY, leftL)when the text is equal fixes it, and the package's unit tests still pass. It could go in this PR or in a follow-up. - Write a paragraph
-
Assert the whole editor state in the test (
packages/lexical-yjs/src/__tests__/unit/V2TableEqualRows.test.ts:185-193).rows()compares only the cell count of each row, so content that lands in the wrong row with the same counts would still pass. Comparingeditor.getEditorState().toJSON()with the fresh reader's also passes with this fix. -
Add
// @vitest-environment nodeafter the copyright header of the new test. AGENTS.md (Testing Strategy) asks for it in unit tests that only exercise editor state or headless behavior, and this test needs no DOM. -
Broaden the changeset wording (
.changeset/lexical-yjs-v2-equal-subtree-mapping.md, lines 5 and 7). The fix covers any block that equals another: list items with equal text hit the same error onmain. The entry mentions only table rows, and.changeset/README.mdasks for entries written for people who use Lexical, so naming the general case (for example "table rows or list items") would help them find it.
… tests, add swapped equal blocks of each kind, and dispose the test editors ## Description The tests for #9581 compared only the number of cells per table row, which would not show a wrong payload, format or list state. Every check now compares each editor's whole state (its JSON) with a fresh reader of the document and with the other peers. A third group swaps two equal blocks of each kind (paragraph, text runs with formats, a tab, a hashtag and a code token, quote, list, nested list, table, link, mark and line break, a slot host, a decorator), with the lines around them changed or not so that the matching reaches them from the left, from the right or in the middle, in both client id orders, with and without gc, then has a peer edit the first of them. Each test disposes its editors and documents when it finishes. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-yjs/src/__tests__/unit/V2TableEqualRows.test.ts ``` ### Before With `SyncV2.ts` as on main: ``` Error: Lexical node does not exist in active editor state (key 9869, type tablecell). The reference was probably kept from another read or update, for example in a closure, event handler or component AssertionError: expected { root: { children: [ …(5) ], …(7) } } to deeply equal { root: { children: [ …(5) ], …(7) } } Tests 130 failed | 39 passed (169) ``` (The scripted case, 57 of the 60 seeded runs, and 72 of the 108 swapped-block cases: every kind but the slot host and the decorator.) ### After ``` Tests 169 passed (169) ``` `pnpm exec vitest run --project unit packages/lexical-yjs`: ``` Test Files 13 passed (13) Tests 408 passed | 4 expected fail (412) ``` `pnpm run tsc-test`, `pnpm run lint` and `pnpm run prettier` pass.
… middle of a block's children
## Description
When collaboration v2 writes a local change, the loop that compares the children left between the matched start and end (`$updateYFragment`) left a text run that equals its Y text unmapped. A text node made again with the same text there (between two links whose URLs changed in the same update) kept the mapping of the node it replaced, and a collaborator's edit of that text then failed with "Lexical node does not exist in active editor state" while the editor kept showing the old text. That run is now mapped as the start and end loops map an equal child.
Also: the test file runs in the node environment, and the changeset names the general case (table rows or list items with equal content, a text node made again with the same text).
## Test plan
```
pnpm exec vitest run --project unit packages/lexical-yjs/src/__tests__/unit/V2TableEqualRows.test.ts
```
### Before
Without the `SyncV2.ts` change in this commit:
```
× v2 peers: a text run made again with the same text, between two changed links, keeps its mapping 14ms
Error: Lexical node does not exist in active editor state (key 39364, type text). The reference was probably kept from another read or update, for example in a closure, event handler or component
Tests 1 failed | 169 passed (170)
```
### After
```
Tests 170 passed (170)
```
`pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit`:
```
Test Files 54 passed (54)
Tests 689 passed | 4 expected fail (693)
```
`pnpm run tsc`, `tsc-test`, `flow`, `check-changesets`, `lint` and `prettier` pass.
|
Review follow-ups:
|
There was a problem hiding this comment.
This looks good to me. $mapEqualSubtree maps each descendant exactly as $equalYTypePNode paired it, CollabV2Mapping.set then drops the stale entries that pointed into an equal sibling, and the new tests fail on main and pass here while comparing every editor's whole state with a fresh reader and with the other peers. Thanks for also covering the equal text run in the compare loop.
Two optional, non-blocking notes:
-
Changeset wording (
.changeset/lexical-yjs-v2-equal-subtree-mapping.md, line 5). The summary ends withafter a collaborator's concurrent edit, but the edit does not have to be concurrent: in the swapped-block tests, A's swap reaches B before B edits, and those cases still diverge onmain..changeset/README.mdasks for an entry that describes the change from the user's side, soafter a collaborator's later editwould match what people see. -
A separate follow-up, not for this PR. With randomized three-peer edits over equal table rows, list items and paragraphs, this branch converges where
mainusually does not, and the one remaining divergence I found has another cause: when a remote update removes a list's last item, the receiving editor can drop theListNodewhile the Y document keeps an empty list element. A later remote change then throwsLexical node does not exist in active editor statefor the list, or the editors end without the empty list their documents hold. It may deserve its own issue.
…ubtree changeset ## Description The changeset said the wrong content or the error appeared after a collaborator's concurrent edit, but the edit does not have to be concurrent: in the swapped-block tests, the swap reaches the other client before it edits, and those cases fail on main too. It now says "a collaborator's later edit". ## Test plan ``` pnpm run check-changesets ``` ### Before A wording change; the check passed before it too. ### After ``` Changeset check passed: 32 pending changeset(s) valid. ```
|
Thanks. (1) Changeset now says "a collaborator's later edit" (ad08474, wording only). (2) The list case (a remote update removing a list's last item drops the |
|
The list case from the review is now #9592 (a v2 client with a caret drops a list that remote removals empty, while the document keeps it; a later change to the list throws). It reproduces on main and on this branch, so it stays out of this PR. |
There was a problem hiding this comment.
This looks good to me, thanks for the thorough fix and tests.
- The mapping walk matches the comparison.
$mapEqualSubtree(packages/lexical-yjs/src/SyncV2.ts:804-825) pairs the i-th Y child with the i-th normalized Lexical child, which is exactly how$equalYTypePNode(:768-802) compared them, andCollabV2Mapping.setdrops each node's earlier association. That removes the stale descendant entries that let a later remote change splice another row's cells, or a removed node, into a block. Slots need no walk because$equalSlotsonly accepts slots that are already mapped to each other, and the compare-loop branch (:1137-1142) now maps an equal text run the same way$updateYTextmaps an unequal one. - No protocol impact. None of the three changed branches writes to the Y document, so existing documents and peers on older versions are unaffected.
- The tests separate the base from the fix. The new tests fail against the base
SyncV2.ts(131 of 170: the issue's script, 57 of the 60 seeds, 72 of the 108 swapped-block cases and the text-run case) and pass on this branch, matching the Before and After in the description. Every check compares whole editor states with a fresh reader of the document and with the other peers, rather than only cell counts. - Changeset.
.changeset/lexical-yjs-v2-equal-subtree-mapping.mdfollows.changeset/README.mdand CONTRIBUTING.md: apatchbump, a one-line[fix]summary and aVerify:paragraph.
Thanks also for splitting the empty-list case out into #9592; it is a separate bug and does not need to block this one. No changes needed from my side.
…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 ```
…and count each id's holders in the HTML import check ## Description #9540 and #9584 merged, so the cells marked `test.fails` for them pass on main and report as passing unexpectedly: letters composed past the end of a marked run (26 cells in `MarksIME.test.ts`) and the `@lexical/yjs` v2 table row cell of #9581 (`C6Settles.test.ts`, `yjs-v2, table edits, follow-ups none, repair none, seed 11`). They are plain tests again. The marks for #9553, #9568, #9578 (and #9561) and #9606 stay. The import check of `C7Html.test.ts` collected the imported blocks' ids in a map, so two blocks holding the same id counted as one and "exactly one imported block" was never asserted. It now keeps every holder and requires exactly one, of the same type. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/matrix pnpm run ci-check ``` ### Before On main `04e899d` with the marks: ``` ❯ packages/lexical-y/src/__tests__/unit/matrix/MarksIME.test.ts (56 tests | 26 failed) ❯ packages/lexical-y/src/__tests__/unit/matrix/C6Settles.test.ts (360 tests | 1 failed) ``` ### After On main `04e899d`: ``` Test Files 13 passed (13) Tests 2395 passed | 13 expected fail (2408) $ pnpm run ci-check Found 0 errors ```
Description
When the v2 binding writes a local change (
$updateYFragment), it pairs the editor's children with the Y children from both ends. A child that is deep-equal to a Y child it is not mapped to ($equalYTypePNode), such as a table row equal to another row, was mapped to that Y type, but only at the top: its descendants kept their previous mappings, which can point at another subtree's nodes (the cells of an equal row). A later remote change to that row's children then read those cells and moved them into the wrong row. In #9581 one client's editor ends with a row that has no cells while its own document and the other clients have two; with other interleavings, a later read throws "Lexical node does not exist in active editor state", or the clients diverge. The same happens on main when one update removes a row's last cell and appends a new one (as padding a row does).Now an equality match maps the whole subtree, child by child, as the comparison paired them (slots are equal only when already mapped to each other, so they need nothing more). The v1 binding does not use this code. The loop that compares the children left in the middle had the same gap for a text run equal to its Y text (a text node made again with the same text, between two links changed in the same update): it is mapped too.
Closes #9581
Changeset
.changeset/lexical-yjs-v2-equal-subtree-mapping.md(@lexical/yjs,[fix]).Test plan
V2TableEqualRows.test.ts: the issue's three-peer script; 60 seeded runs of three v2 peers editing a table whose rows are equal (row and column inserts, a one-update remove-and-append of a row's last cell, partial delivery, each channel in order); and 108 cases that swap two equal blocks of each kind (paragraph, text runs, quote, list, nested list, table, inline mix of link, mark and line break, slot host, decorator), reached by the matching from the left, the right or the middle, in both client id orders, with gc on and off, then edit one of them. Each editor's whole state (its JSON) is compared with a fresh reader of the document and with the other peers.Before
With
SyncV2.tsas on main:(The script, 57 of the 60 seeded runs, and 72 of the 108 swapped-block cases: every kind but the slot host and the decorator.)
After
After the third commit (the text run in the middle loop; its new test fails without it with
Lexical node does not exist in active editor state (key 39364, type text)):pnpm exec vitest run --project unit packages/lexical-yjs packages/lexical-react/src/__tests__/unit packages/lexical-table packages/lexical-y:pnpm exec vitest run --project unit packages/lexical-yjsafter the second commit (whole-state checks, swapped blocks):pnpm run ci-check(each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.