Repository navigation
[lexical-y] Bug Fix: A run ends in its own container, and no item is linked next to an item of another container - #9693
Merged
Merged
Conversation
…linked next to an item of another container ## Description A format write that restates another client's run at the end of its range gives that run an end of its own, after the end that client's change gave it (#9679, rule 3). That end came from `changeEnd`, which walks the client's items in clock order and could return an item in another container (a write that colored text in a table cell and then in the root). The writer then linked its new item after that item, into the root's list, while naming the cell as its parent. Every other client integrated the item into the cell, but the writer's own document held a malformed item: later changes next to it made the writer's editor stop reading part of the document, for good and without a warning (a downstream app's long notes: 15 of 133 sessions on `main`, 0 of 132 before #9679). Now: - `changeEnd` and `lastOwnItemBefore` only look at items in the run's own shared type: a run cannot end in another container. - The cursor refuses to link a new item next to an item of another type (`checkLinkable` in `newItem` and `insertAfter`, in every build): the write throws, and the existing recovery rewrites the document from the editor and reports `@lexical/y/recovered-write`, instead of storing a malformed item. `closeRestoredRuns` skips such a pair. - Tests check the structure: `misLinkedItems` (no item with a neighbour in another type) runs in the harness's `expectConverged`, after every op of `UndoSpreadFuzz`, and in a new structure fuzz. Recorded as D96 (its issue link is a placeholder until the issue is filed). ## Test plan ### Before ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts Tests 11 failed | 191 passed (202) AssertionError: expected [ '6414644327309859:22', '1:5', '2:2' ] to deeply equal [] $ LEXICAL_Y_GC=1 (same files) Tests 11 failed | 191 passed (202) ``` ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts Tests 202 passed (202) $ LEXICAL_Y_GC=1 (same files) Tests 202 passed (202) $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y Test Files 240 passed | 2 skipped (242) Tests 19229 passed | 304 expected fail | 130 skipped (19663) $ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y Tests 19226 passed | 307 expected fail | 130 skipped (19663) $ pnpm run ci-check exit 0 ``` With gc on, `DisposeMemory.test.ts` exceeded its heap bound once with 40 workers in parallel; it passes alone (three runs). The 1,000-seed version of the structure fuzz: 48 (gc off) and 49 (gc on) seeds failing before, 0 after.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
November 1, 2026 04:09
|
AI review status for this pull request.
|
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: |
## Description D96 links its issue, #9694 (it named a placeholder). Documentation only. ## Test plan ### Before D96 linked a placeholder issue. ### After ``` $ prettier --check packages/lexical-y/DECISIONS.md All matched files use Prettier code style! ```
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good to me. A format run in one Yjs shared type can never be ended by an item in another, so keeping changeEnd and lastOwnItemBefore inside the run's own type (packages/lexical-y/src/internal/Cursor.ts:189, :215) is the right rule. checkLinkable then turns any future cross-container link into a recovered write instead of a malformed item. CrossContainerEnd and StructureFuzz fail without the Cursor.ts changes and pass with them, with garbage collection on and off. The changeset follows CONTRIBUTING.md, including the Verify: paragraph a behaviour fix needs.
Two optional, non-blocking suggestions:
-
Correct D96's account of the other clients (
packages/lexical-y/DECISIONS.md:61, alsoCursor.ts:150-151andCrossContainerEnd.test.ts:17-18). These say every other client integrated the stray end item into the table cell. In@y/y, an item that has an origin is encoded without its parent, and a receiving client takes the parent from the origin item (Item#write, andstruct.parent = struct.left.parentinutils/encoding.js). Peers therefore put it in the root's list, right after the other author's own null, where it changes nothing; only the writer's own copy names the cell.src/__tests__/utils/structure.ts:15-16and #9694 already describe it that way ("integrates it from its origin").packages/lexical-y/AGENTS.mdmakes DECISIONS.md the record later changes are checked against, so the row's reasoning is worth fixing. -
Let
StructureFuzzname a failing seed (StructureFuzz.test.ts:195). WithcheckLinkablein place, a regression of thechangeEndfilter no longer stores a malformed item, somisLinkedItemsstays empty. The recovered-write warning goes to Lexical's defaultonWarn, which throws it in a microtask, so the run reports all 200 tests passed plus unhandled errors and names no seed. Recording warnings in the fuzz's editors and expecting no@lexical/y/recovered-writeafter each step would point such a failure at its seed. Harness tests have the same gap:expectConvergedonly rejectsself-checkwarnings (harness.ts:480).
| | D24 | **Yjs's format cleanup is off** in `@lexical/y` containers and on every relay that loads a state (`createYDoc`). A delete keeps the ends of each deleted formatted run (within a bound per format key until D86 removed the bound: a delete keeps every format item it leaves). Not reported upstream. | The cleanup deletes format items a concurrent writer may still rely on. Upstream already knows that skipping it is the app's trade-off. | `DECIDED` | 10-28 | [#9400], [y.md: Servers][y-servers] | | ||
| | D86 | **A delete keeps every format item it leaves (no collapse).** The collapse of the run of deleted content and format items a delete leaves (`Cursor#collapseGap`, D24's bound per format key) is removed; D24's cleanup-off stays. | The collapse dropped ends that a concurrent writer's restore or an undo relied on, and the value ran on: #9575, #9609, a #9620 cell, the concurrent restore cells of #9640, a KeepEndsSessions seed. Every attempt to keep the right ends in the collapse (#9617) needed to know which write relies on which item. Measured against main, same seeds: a three-client view fuzz of 15,000 seeds: 0 seeds wrong where main is right, 631 where main spreads a value and this does not. Cost: each formatted run whose text is deleted keeps its two items, none per keystroke or plain delete (F24A-1: 40, 80, 120 after 20, 40, 60 words typed and deleted at one place); a comment added and its word deleted 3,000 times at one place: 6,002 live format items (main 6); a downstream document library's corpus of 189 documents x1.003 at 10 cycles, x1.26 at 150; Jot x1.01 on its default profile, a concentrated recolor 438 items (main 345). Accepted: the linear growth with deleted formatted text, over the bound and its spreads. | `DECIDED` | 10-31 | [#9575], [#9609] | | ||
| | D88 | **A client keeps its own format items from garbage collection while they are undoable (memory only).** The binding sets Yjs's `Item.keep` on the format items its own transactions and its undo manager's insert, and on the end of each run they delete; it releases them, between transactions, when both undo stacks are empty (after a clear, an undo that leaves nothing, or a write they did not capture), and when it is disposed or its document destroyed. An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as Yjs's `UndoManager` collects nothing when it releases what its steps kept: any `UndoManager` on the document may still restore the item. | An undo reads its change's format items, and the end each gave a run it deleted, after a collaborator deleted them (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`). With Yjs's default garbage collection, which products use, those were collected, the undo could not tell them apart from other content, and the restored value ran on over the rest of the document ([#9623]; a two-client undo fuzz: 221 of 3,000 seeds to 0). Nothing new is written and other clients are unchanged. The cost is local: a format item a collaborator deletes while this client can undo the change that wrote it keeps its deleted content in this client's document, and in a state it encodes, until the document reloads or a relay that loads its state with gc on compacts it: about 25 bytes per such deletion, in this client only (+0.6% to 0.8% of the client's state in a realistic 4,500-edit session). Accepted over a stored change (a `gcFilter` keeping every format item: +1.9% to 7.6% bytes on long notes, in every peer and relay) and over collecting on release, which was tried and dropped: it destroyed what another `UndoManager` on the same document still needed. A `gcFilter` keeping every format item fixed the same cases, but changes what every peer and relay stores (+1.9% to 7.6% bytes on long notes). | `DECIDED` | 10-31 | [#9623], [y.md: Limits][limits] | | ||
| | D96 | **A run ends in its own container, and no item is linked next to an item of another container.** `changeEnd` and `lastOwnItemBefore` look only at items in the run's shared type; the cursor refuses to link a new item next to an item of another type (the write throws and is recovered from, `RECOVERED_WRITE_WARNING`). | The end a write gives a restated run at its range's end (D78's rule 3, [#9679]) came from `changeEnd`, which walks the client's items in clock order across containers and could return an item in another type; the writer linked its end into that type's list while naming the run's container as its parent. Every other client integrated the item into the right list, but the writer's own document held a malformed item, and its editor stopped reading part of the document for good (a downstream app's long notes: 15 of 133 sessions). A structure check (no item's neighbour in another container) now runs in every harness convergence check, the undo fuzz and a structure fuzz. | `DECIDED` | 11-01 | [#9694] | |
There was a problem hiding this comment.
Optional: peers don't integrate the stray end into the cell. An item with an origin is encoded without its parent, and @y/y takes the parent from the origin item on receipt, so every other client puts it in the root's list, after the author's own null, where it changes nothing. Only the writer's copy names the cell. utils/structure.ts:15-16 and #9694 already say this; the same sentence is in Cursor.ts:150-151 and CrossContainerEnd.test.ts:17-18.
Member
Author
| expect(misLinkedItems(doc), `after step ${i}`).toEqual([]); | ||
| } | ||
| } | ||
| expect(errors).toEqual([]); |
There was a problem hiding this comment.
Optional: with checkLinkable in place, a regression of the changeEnd filter is refused and recovered, so misLinkedItems stays empty. The recovered-write warning reaches the default onWarn and surfaces only as an unhandled error, with every seed reported passed. Collecting warnings here and expecting no @lexical/y/recovered-write would make the failing seed show up.
Member
Author
There was a problem hiding this comment.
Done in #9696 (096f779): the fuzz records the editors' warnings and expects none after each step. With the same-type filter in changeEnd removed it now names 9 of 200 failing seeds (it reported 200 passed plus unhandled errors before). The harness gap is closed there too: expectConverged fails on a recovered write.
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…9694 (an editor stopped matching its own document in long sessions) Reverts #9679 whole, on top of current `main` (keeps #9684 and #9685). With #9679, a downstream app's long-note sessions end with an editor that no longer matches its own Yjs document (15 of 133 sessions; 0 of 132 on the commit before #9679). Reverting only part of it does not work: the delete rule (D86) alone is worse on a recorded session (2810), and the divergence was traced to one of its hunks (the end a restated run gets at a range's end, which could be linked into another container; #9693 fixes that at the source). This PR is the fallback if #9693 is not taken. Closes #9694 if merged instead of #9693. What this brings back, as before #9679: - A delete collapses the run of deleted content and format items it leaves to a bounded number per format key (D24's bound): the cases it fixed (#9575, #9609, a #9620 cell, #9640's concurrent restore cells) fail again and are pinned with their issues. - No format items kept from garbage collection: with gc on, an undo can again put a value back past its end (#9623). The harness keeps its `LEXICAL_Y_GC` switch (added by #9679), and the cells that fail only with gc on are pinned as `test.fails` with gc on, naming #9623. - The restore rules for #9671 and the restore scan for #9599 (D78) are gone: those cases are open again. `DECISIONS.md` keeps D86 and D88 (now `REVERTED`, with the reason) and reopens D78, rather than rewriting them. `y.md` goes back to the behavior before #9679. The two changesets of #9679 are removed: it was not part of a release, so there is nothing to announce. `UndoSpreadSession2810.test.ts` (added by #9679) is kept: it passes without D86 and D88 as well. `main` 60022ae: the downstream app's long-note sessions diverge in 15 of 133. ``` $ LEXICAL_Y_GC=0 vitest --run --project unit Test Files 1 failed | 648 passed | 4 skipped (653) Tests 1 failed | 28071 passed | 481 expected fail | 135 skipped (28688) $ LEXICAL_Y_GC=1 vitest --run --project unit Test Files 2 failed | 647 passed | 4 skipped (653) Tests 2 failed | 28055 passed | 496 expected fail | 135 skipped (28688) $ pnpm run ci-check exit 0 ``` The failures are load flakes with 40 workers: `DisposeMemory.test.ts` (heap bound; gc off and on) and `MarkdownEscaping.test.ts` (5 s timeout, gc on). Each passes alone (two runs each).
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…to the items the store holds ## Description Stacked on #9693 (it extends that PR's structure check). An undo that brings back a run gives it an end of this client's own where a closer ended it (`closeRestoredRuns`). It found where in one transaction and wrote the null in a second one, keeping the item objects it had found. The first transaction's scan splits items and its cleanup merges them back, so the item to the null's left could be a piece no longer in the store. The null was linked to that piece: the merged item in the store kept its old right, so a walk of the list skipped the null. Only the undoing client's document is affected (others integrate the null from its origin), and its editor can stop matching its document: every pre-existing editor-vs-document divergence a downstream long-session fuzz found starts with such a link. Now: - `closeRestoredRuns` keeps ids, not items, across the two transactions, and resolves them in the store when it writes (splitting there as Yjs does); it writes nothing where the two are no longer next to each other. - The running undo's own items (`isKeptByUndo`) are looked up by id in the step's insertions instead of by the item objects an earlier transaction saw. - The structure check (`misLinkedItems`, run by `expectConverged`, the undo fuzz and the structure fuzz) also requires each neighbour to link back and to have the same map key. Audit of other item references kept across a transaction: the format-closer marks and the kept format items are format items, which Yjs never splits or merges; the stored nodes an undo removes are nested types, likewise. Nothing else outlives its transaction. ## Test plan ### Before ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts AssertionError: after step 7: expected [ '795599280632996:0' ] to deeply equal [] AssertionError: after step 7: expected [ '2159088886642643:0' ] to deeply equal [] Tests 2 failed (2) ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts Tests 2 passed (2) $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y Test Files 241 passed | 2 skipped (243) Tests 19231 passed | 304 expected fail | 130 skipped (19665) $ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y Test Files 241 passed | 2 skipped (243) Tests 19228 passed | 307 expected fail | 130 skipped (19665) $ pnpm run ci-check exit 0 ```
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…D96 on what other clients hold ## Description Review follow-ups to #9693, tests and docs only: - `StructureFuzz.test.ts` records the editors' warnings and expects none after each step. With `checkLinkable`, a regression of the run-end rule is refused and recovered from instead of stored, so the structure check alone stays empty and the warning only surfaced as an unhandled error; now the failing seeds are named. - D96, `Cursor.ts` (checkLinkable's comment) and `CrossContainerEnd.test.ts`: other clients take the stray item's parent from its origin and put it in the root's list, where it changes nothing; only the writer's document holds it wrong. ## Test plan ### Before With the run-end rule's same-type filter removed, the fuzz reported 200 passed plus unhandled errors. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts Tests 200 passed (200) $ LEXICAL_Y_GC=1 (same file) Tests 200 passed (200) $ (same-type filter in changeEnd removed) LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts Tests 9 failed | 191 passed (200) ```
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
A format write that restates another client's run at the end of its range
gives that run an end of its own, after the end that client's change gave it
(#9679, rule 3). That end came from
changeEnd, which walks the client'sitems in clock order and could return an item in another container (a write
that colored text in a table cell and then in the root). The writer then
linked its new item after that item, into the root's list, while naming the
cell as its parent. Every other client integrated the item into the cell, but
the writer's own document held a malformed item: later changes next to it
made the writer's editor stop reading part of the document, for good and
without a warning (a downstream app's long notes: 15 of 133 sessions on
main, 0 of 132 before #9679).Now:
changeEndandlastOwnItemBeforeonly look at items in the run's ownshared type: a run cannot end in another container.
(
checkLinkableinnewItemandinsertAfter, in every build): the writethrows, and the existing recovery rewrites the document from the editor
and reports
@lexical/y/recovered-write, instead of storing a malformeditem.
closeRestoredRunsskips such a pair.misLinkedItems(no item with a neighbour inanother type) runs in the harness's
expectConverged, after every op ofUndoSpreadFuzz, and in a new structure fuzz.Recorded as D96. Closes #9694.
Test plan
Before
After
With gc on,
DisposeMemory.test.tsexceeded its heap bound once with 40workers in parallel; it passes alone (three runs). The 1,000-seed version
of the structure fuzz: 48 (gc off) and 49 (gc on) seeds failing before,
0 after.