Repository navigation
Conversation
## Description An update tagged `SKIP_COLLAB_TAG` can leave the editor holding a stored node somewhere else than the document stores it. #9513: client A moves box `d` out of box `c` and `c` into `d`, unwritten; B inserts a paragraph after `d`, inside `c` as the document has it. The self-check reads `c` again, which places `d` inside `c`. `$setChildren` takes `c` out of `d` first so as not to make a cycle, expecting the read of the container that stores `c` to place it again, but no such read runs: A showed only the first paragraph until it reloaded. The reader now keeps one rule for every node a read takes out of where the editor held it (`$setChildren`, the removal of a region's old nodes): at the end of the read, a node still out of the tree whose Y.Node the document still holds was held elsewhere than the document stores it, and the read throws instead of dropping it, which reads the whole document again (`recoverRead`), with a warning. Concurrent moves are unaffected: they delete the old item, or the read places the node again before it ends. A stored inline element the read leaves out on purpose (emptied by collaborators, and unable to be empty, as Lexical would remove it) is no loss and is excluded. The same happened to a line: A moves paragraph t1 before t0, unwritten; B removes t1. The read of B's change took out every node between the first and the last node of the lines it read, assuming the editor holds them in the document's order; t0 was among them, and A lost it until it reloaded (without the self-check). A region read now takes out only each old unit's own nodes; a node an unwritten update moved between them is not theirs and stays. The insert-before variant of #9513 (a read that would place a node inside itself) already recovered this way since #9510. Closes #9513 ## Changeset `.changeset/lexical-y-read-keeps-stored-nodes.md` (`[fix]`). ## Test plan `EditorDrift.test.ts`: a line removed after a skipped update moved it before another (the other stays, without the self-check); a block inserted after a box that a skipped update made hold the box storing it, with the self-check on (A shows every block, read again with a warning) and off (A keeps its unwritten nesting, with the insert in it); then A's next edit reaches B with nothing missing. An emptied stored inline element that cannot be empty is left out without a recovery (a raw Yjs delete, and a peer deleting its text). ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/EditorDrift.test.ts -t 'nothing goes missing' × a block inserted after a box that a skipped update made hold the box storing it (self-check true): nothing goes missing 376ms ✓ a block inserted after a box that a skipped update made hold the box storing it (self-check false): nothing goes missing 16ms × a block inserted after a box that a skipped update made hold the box storing it (self-check true): nothing goes missing 84ms ✓ a block inserted after a box that a skipped update made hold the box storing it (self-check false): nothing goes missing 251ms ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯ Expected: "paragraph("top") / test_box[c](paragraph("c1") test_box[d](paragraph("d1")) paragraph("new") paragraph("c2"))" Received: "paragraph("top")" Tests 2 failed | 2 passed | 22 skipped (26) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/EditorDrift.test.ts -t 'nothing goes missing' ✓ |unit| packages/lexical-y/src/__tests__/unit/EditorDrift.test.ts (26 tests | 22 skipped) 351ms Tests 4 passed | 22 skipped (26) ``` - `@lexical/y` unit suite: 155 files, all pass (7244 passed, 42 expected fail). - Fuzz (a randomized test, not committed: random `SKIP_COLLAB_TAG` moves of boxes and paragraphs on A, random inserts and deletes on B; oracle: A shows every paragraph B shows), 1,500 sessions each: self-check off (the default), main 20 failures, this change 0; self-check on, main 242, this change 182. Every failure left with the self-check on is a paragraph that the unwritten update moved into a container the self-check then reads again in full (`$readFull`, which sets the container's children to what the document stores there); that path is opt-in diagnostics and is left for a follow-up. - In-repo fuzz (`Fuzz.test.ts`, seeds 1..3000, gc 1..600): same result as main (seed 2277 only, a documented limit).
zurfyx
requested review from
acywatson,
ivailop7 and
potatowagon
as code owners
October 30, 2026 00:10
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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: |
…other container stores ## Description Follow-up in the same PR (#9513 family). With the self-check on, a paragraph that an update tagged `SKIP_COLLAB_TAG` moved into a box was dropped when the self-check read that box again in full: `$readFullInner` sets the box's children to what the document stores there, and the paragraph's own container (the root) was not read. The full read now notes the lines it sets aside; at the end of the read, one still out of the tree whose unit another live container holds makes the read throw, which reads the whole document again (as for stored nodes). ## Test plan `EditorDrift.test.ts`: a container a skipped update moved a line into, read again in full: the line stays. ### Before ``` × a container a skipped update moved a line into, read again in full: the line stays Expected: "paragraph("top") / test_box[b](paragraph("b1") paragraph("new")) / paragraph("p2")" Received: "paragraph("top") / test_box[b](paragraph("b1") paragraph("new"))" ``` ### After ``` ✓ a container a skipped update moved a line into, read again in full: the line stays ``` - `@lexical/y` unit suite: all pass (7493 passed, 46 expected fail). - Skip-structure fuzz (local), 1,500 sessions: self-check on 182 failures before this commit, 0 after; self-check off 0.
… line another container holds
## Description
Follow-up in the same PR (soak finding S9-8, introduced by the previous commit). The full-read line guard looked up a line it set aside by its old node key in `unitByKey`. A read that makes a line's node anew (a line whose type changed: a heading made a paragraph again) keeps its unit for the new node and leaves the old key's entry behind, naming that live unit. The guard took that stale entry for a line another container still holds and threw. On a read-only client reading a change that brings a layout record (a whole-document read), the throw made the binding fail closed: it kept showing what it showed before the change, for good, and missed text a collaborator typed in the same change (with or without the self-check).
The guard now counts a line as held elsewhere only while its unit is still its own (`unit.key === key`).
## Test plan
`EditorDrift.test.ts`: a change that retypes a line and brings a layout record is read (a client read only for a node type it does not register).
### Before
```
$ pnpm exec vitest run --project unit --reporter=verbose packages/lexical-y/src/__tests__/unit/EditorDrift.test.ts -t 'retypes a line and brings'
a change that retypes a line and brings a layout record is read 33ms
a change that retypes a line and brings a layout record is read 13ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯
Expected: "paragraph("one") / quote(paragraph("inner typed"))"
Received: "heading[h2]("one") / quote(paragraph("inner"))"
Tests 2 failed | 32 skipped (34)
```
### After
```
$ pnpm exec vitest run --project unit --reporter=verbose packages/lexical-y/src/__tests__/unit/EditorDrift.test.ts -t 'retypes a line and brings'
a change that retypes a line and brings a layout record is read 33ms
a change that retypes a line and brings a layout record is read 12ms
Tests 2 passed | 32 skipped (34)
```
- The soak capture (a read-only client applying a remote batch with a retype and a layout record) passes, alone and together with #9552.
…nce per container
## Description
Follow-up in the same PR (integration review). The check that a read never drops a node the document still holds ran at the end of each outermost `$readFull`. A read that reads several containers in full one after the other (the re-read of what a refused local update changed, the self-check's re-read, a read of the whole document with its containers, and a remote read after the refused containers) is one logical read: a line one container sets aside, the read of another places again. Checked after each container, a read-only client's refused move of a line between containers (from the root into a cell, between cells) was taken for a dropped line: the client read the whole document again, with a recovered-read warning, for a refused edit. Content ended right.
`Reader#$reading(read)` runs a logical read made of several entry points and checks once at its end; the binding's multi-container reads use it.
## Test plan
New `regressions/RefusedMoveRead.test.ts`: a read-only client refuses a move of a line across containers (cell to root, root to cell, cell to cell), self-check on and off: no recovered-read warning, the editor shows what it showed, and a later remote change reads in.
### Before
```
$ pnpm exec vitest run --project unit --reporter=verbose packages/lexical-y/src/__tests__/unit/regressions/RefusedMoveRead.test.ts
a cell line to the root (selfCheck false)
a root line into a cell (selfCheck false)
a cell line to another cell (selfCheck false)
a cell line to the root (selfCheck true)
a root line into a cell (selfCheck true)
a cell line to another cell (selfCheck true)
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 4 ⎯⎯⎯⎯⎯⎯⎯
Tests 4 failed | 2 passed (6)
```
### After
```
$ pnpm exec vitest run --project unit --reporter=verbose packages/lexical-y/src/__tests__/unit/regressions/RefusedMoveRead.test.ts
a cell line to the root (selfCheck false)
a root line into a cell (selfCheck false)
a cell line to another cell (selfCheck false)
a cell line to the root (selfCheck true)
a root line into a cell (selfCheck true)
a cell line to another cell (selfCheck true)
Tests 6 passed (6)
```
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
An update tagged
SKIP_COLLAB_TAGcan leave the editor holding a stored node somewhere else than the document stores it. #9513: client A moves boxdout of boxcandcintod, unwritten; B inserts a paragraph afterd, insidecas the document has it. The self-check readscagain, which placesdinsidec.$setChildrentakescout ofdfirst so as not to make a cycle, expecting the read of the container that storescto place it again, but no such read runs: A showed only the first paragraph until it reloaded.The reader now keeps one rule for every node a read takes out of where the editor held it (
$setChildren, the removal of a region's old nodes): at the end of the read, a node still out of the tree whose Y.Node the document still holds was held elsewhere than the document stores it, and the read throws instead of dropping it, which reads the whole document again (recoverRead), with a warning. Concurrent moves are unaffected: they delete the old item, or the read places the node again before it ends. A stored inline element the read leaves out on purpose (emptied by collaborators, and unable to be empty, as Lexical would remove it) is no loss and is excluded.The same happened to a line: A moves paragraph t1 before t0, unwritten; B removes t1. The read of B's change took out every node between the first and the last node of the lines it read, assuming the editor holds them in the document's order; t0 was among them, and A lost it until it reloaded (without the self-check). A region read now takes out only each old unit's own nodes; a node an unwritten update moved between them is not theirs and stays.
The insert-before variant of #9513 (a read that would place a node inside itself) already recovered this way since #9510.
Closes #9513
Changeset
.changeset/lexical-y-read-keeps-stored-nodes.md([fix]).Test plan
EditorDrift.test.ts: a line removed after a skipped update moved it before another (the other stays, without the self-check); a block inserted after a box that a skipped update made hold the box storing it, with the self-check on (A shows every block, read again with a warning) and off (A keeps its unwritten nesting, with the insert in it); then A's next edit reaches B with nothing missing. An emptied stored inline element that cannot be empty is left out without a recovery (a raw Yjs delete, and a peer deleting its text).Before
After
@lexical/yunit suite: all pass (7491 passed, 46 expected fail).SKIP_COLLAB_TAGmoves of boxes and paragraphs on A, random inserts and deletes on B; oracle: A shows every paragraph B shows), 1,500 sessions each: self-check off (the default), main 20 failures, this change 0; self-check on, main 242, this change 0 (the second commit covers a line the self-check's full read of a container set aside: see below).Fuzz.test.ts, seeds 1..3000, gc 1..600): same result as main (seed 2277 only, a documented limit).Second commit: a full read of a container never drops a line another container stores
With the self-check on, a paragraph that an unwritten update moved into a box was dropped when the self-check read that box again in full (
$readFullInnersets the box's children to what the document stores there, and the paragraph's own container was not read). The full read now notes the lines it sets aside; at the end of the read, one still out of the tree whose unit another live container holds makes the read throw, which reads the whole document again. Test: "a container a skipped update moved a line into, read again in full: the line stays" (fails before, passes after).@lexical/ysuite: 7493 passed, 46 expected fail; in-repo fuzz 1..3000: same as main.