Repository navigation
[lexical-y] Bug Fix: An undo that ends a restored run links its null to the items the store holds - #9696
Merged
Conversation
|
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:30
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: |
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…does not repeat what it stored ## Description Stacked on #9696. When a local write throws part way, the binding rewrites the document from the editor and reports `@lexical/y/recovered-write`. That rewrite wrote a changed line through the keystroke fast path (`$writeTextEdit`), which writes the text edits Lexical recorded against the previous editor state. Where the failed write had already stored such an edit, the rewrite stored it again: the other client showed the inserted text twice, and the author's editor no longer matched its own document. The rewrite now skips the fast path and diffs each line against the document. The failure is not reachable through a known path; it is measured by injecting a throw (the second item a cursor writes) and affects every tree, including the commit before #9679. ## Test plan ### Before ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts AssertionError: expected 'paragraph("one INSERT INSERT alpha be…' to be 'paragraph("one INSERT alpha beta") / …' // Object.is equality Tests 2 failed (2) ``` ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts Tests 2 passed (2) $ LEXICAL_Y_GC=1 (same file) Tests 2 passed (2) $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19232 passed | 304 expected fail | 130 skipped (19667) $ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19229 passed | 307 expected fail | 130 skipped (19667) $ pnpm run ci-check exit 0 ``` The one failure in each full run is a memory-loop test (Headless gc off, DisposeMemory gc on) that exceeded its heap bound under load from other runs on the machine; both pass alone.
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…does not repeat what it stored ## Description Stacked on #9696. When a local write throws part way, the binding rewrites the document from the editor and reports `@lexical/y/recovered-write`. That rewrite wrote a changed line through the keystroke fast path (`$writeTextEdit`), which writes the text edits Lexical recorded against the previous editor state. Where the failed write had already stored such an edit, the rewrite stored it again: the other client showed the inserted text twice, and the author's editor no longer matched its own document. The rewrite now skips the fast path and diffs each line against the document. The failure is not reachable through a known path; it is measured by injecting a throw (the second item a cursor writes) and affects every tree, including the commit before #9679. ## Test plan ### Before ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts AssertionError: expected 'paragraph("one INSERT INSERT alpha be…' to be 'paragraph("one INSERT alpha beta") / …' // Object.is equality Tests 2 failed (2) ``` ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts Tests 2 passed (2) $ LEXICAL_Y_GC=1 (same file) Tests 2 passed (2) $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19232 passed | 304 expected fail | 130 skipped (19667) $ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19229 passed | 307 expected fail | 130 skipped (19667) $ pnpm run ci-check exit 0 ``` The one failure in each full run is a memory-loop test (Headless gc off, DisposeMemory gc on) that exceeded its heap bound under load from other runs on the machine; both pass alone.
…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 ```
…o recover from
## Description
The harness's `expectConverged` accepted a recovered-write warning (a local change that could not be written, rewritten from the editor). A fix that only refused a bad write and let the recovery repair it would pass every converging test. It now fails on that warning, as it does on a self-check warning; no existing test converges after a recovery. Tests only.
## Test plan
### Before
A recovered write passed `expectConverged`.
### After
```
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y
Test Files 1 failed | 240 passed | 2 skipped (243)
Tests 1 failed | 19230 passed | 304 expected fail | 130 skipped (19665)
$ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y
Test Files 1 failed | 240 passed | 2 skipped (243)
Tests 1 failed | 19227 passed | 307 expected fail | 130 skipped (19665)
$ vitest --run --project unit packages/lexical-y/src/__tests__/unit/Headless.test.ts packages/lexical-y/src/__tests__/unit/DisposeMemory.test.ts # gc off, then on
Tests 33 passed (33)
Tests 33 passed (33)
```
The one failure in each full run is a memory-loop test (Headless gc off, DisposeMemory gc on) that exceeded its heap bound with 30 workers and other load on the machine; both pass alone.
…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) ```
…ach of them
## Description
Review follow-up to this PR's run-end change: `closeRestoredRuns` resolves
each saved gap again before writing a null, and skipped the gap when its
two items were no longer next to each other. Where two keys need an end in
the same gap, the second key found the first key's new null between them
and skipped its own: A bolds and italicizes "word"; B removes both in one
step; A's bold end is gone while its italic end stays; B undoes; once A's
italic end goes too, italic ran on over " plain tail". The check now passes
over the nulls this loop wrote, and each null goes right before the gap's
right item, after those. No end is deduplicated by place, and none is
assigned to a write.
## Test plan
### Before
```
$ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndSharedGap.test.ts
Tests 2 failed (2)
```
### After
```
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{UndoRunEndSharedGap,UndoRunEndLinks}.test.ts
Tests 4 passed (4)
$ LEXICAL_Y_GC=1 (same files)
Tests 4 passed (4)
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y
Test Files 1 failed | 241 passed | 2 skipped (244)
Tests 1 failed | 19232 passed | 304 expected fail | 130 skipped (19667)
$ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y
Test Files 242 passed | 2 skipped (244)
Tests 19230 passed | 307 expected fail | 130 skipped (19667)
$ tsc --noEmit
exit 0
```
The gc-off failure is `DisposeMemory.test.ts`'s heap bound under load from
other runs on the machine; it passes alone.
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change looks good. In closeRestoredRuns (packages/lexical-y/src/internal/Binding.ts, lines 5663–5789), the saved ids are now resolved in the store at write time, so the run-end null links to the item the store holds rather than to a split piece that the scan transaction's cleanup merged away. UndoRunEndLinks.test.ts fails at the undo step on the base implementation and passes here, with gc off and on. For a single end, the item written has the same origin, right origin and content as before, so other clients receive the same update.
Two optional, non-blocking suggestions:
- Pin the visible symptom in the regression test (
packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts,OPSat lines 63–72 and the assertion at line 182). The test checks the broken link only. Add two steps after the undo,{e: 0, k: 'undo'}twice (A undoes its bold, then its two comments). On the base, B's editor then shows commentc2over the four paragraphs after the restored run, while A's editor, and a fresh read of B's own document, show it only onXe 1. Asserting after those steps that the two editors' JSON match, or that each matches a fresh reader of its own document, keeps the test tied to what users see even ifmisLinkedItemschanges.packages/lexical-y/AGENTS.mdasks every bug fix to land with a regression test insrc/__tests__/unit/regressions/; this would make that test cover the visible symptom too. - The comment on
undoing(Binding.ts, lines 5828–5829).isKeptByUndoonly looks up format items, and Yjs never splits or merges those, as the PR description's audit notes. Sotop.inserts.hasId(it.id)gives the same answer as the previousSet<Item>. Using theIdSetis a fine simplification, but the reason the comment gives does not apply to these items. You could say the two are equivalent for format items instead.
Comment on lines
+5828
to
+5829
| // By id: this scan's transaction splits items and merges them back, so | ||
| // the item objects it saw need not be those the undo meets. |
There was a problem hiding this comment.
Nit (optional): isKeptByUndo only looks up format items here, and Yjs never splits or merges those, so hasId(it.id) gives the same answer as the previous Set<Item>. The switch is fine, but this reason does not apply to those items. Maybe say the two are equivalent for format items instead.
| } | ||
| sync(); | ||
| for (const peer of peers) { | ||
| expect(misLinkedItems(peer.doc), `after step ${i}`).toEqual([]); |
There was a problem hiding this comment.
Optional: this checks the broken link only. If you append {e: 0, k: 'undo'} twice to OPS, then on the base B's editor shows comment c2 over the four paragraphs after the restored run, while A's editor shows it only on Xe 1. Asserting the two editors' JSON match after those steps would pin the user-visible symptom as well.
zurfyx
force-pushed
the
zurfyx/y-undo-run-end-links
branch
from
November 1, 2026 05:15
896ff92 to
d7773fc
Compare
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…does not repeat what it stored ## Description Stacked on #9696. When a local write throws part way, the binding rewrites the document from the editor and reports `@lexical/y/recovered-write`. That rewrite wrote a changed line through the keystroke fast path (`$writeTextEdit`), which writes the text edits Lexical recorded against the previous editor state. Where the failed write had already stored such an edit, the rewrite stored it again: the other client showed the inserted text twice, and the author's editor no longer matched its own document. The rewrite now skips the fast path and diffs each line against the document. The failure is not reachable through a known path; it is measured by injecting a throw (the second item a cursor writes) and affects every tree, including the commit before #9679. ## Test plan ### Before ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts AssertionError: expected 'paragraph("one INSERT INSERT alpha be…' to be 'paragraph("one INSERT alpha beta") / …' // Object.is equality Tests 2 failed (2) ``` ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts Tests 2 passed (2) $ LEXICAL_Y_GC=1 (same file) Tests 2 passed (2) $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19232 passed | 304 expected fail | 130 skipped (19667) $ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y Test Files 1 failed | 241 passed | 2 skipped (244) Tests 1 failed | 19229 passed | 307 expected fail | 130 skipped (19667) $ pnpm run ci-check exit 0 ``` The one failure in each full run is a memory-loop test (Headless gc off, DisposeMemory gc on) that exceeded its heap bound under load from other runs on the machine; both pass alone.
There was a problem hiding this comment.
Thanks, this looks good. The id-based write in closeRestoredRuns fixes the broken link that UndoRunEndLinks.test.ts reproduces: the test fails on main at the undo step, with gc on and off, and passes here. UndoRunEndSharedGap.test.ts covers the new handling of two ends in one gap.
Two optional, non-blocking suggestions:
- Make the skipped gaps visible (
packages/lexical-y/src/internal/Binding.ts, lines 5742-5772). Thecontinuebranches that skip a gap when its two items are no longer adjacent cannot fire today: the scan and the write run back to back, and the only changes between them are merges the recorded ids survive and the nulls this loop writes itself. If a later change does make one fire, the undo writes no end and the restored value runs on over the rest of the document with no signal. A dev-only warning on those branches would make that show up in the fuzzes. Verify:wording (.changeset/y-undo-run-end-links.md, line 7). CONTRIBUTING.md asks this paragraph to say what to test, but its last sentence is a structural check that people using@lexical/ycannot observe. A user-side check fits better, for example: after the undo, the undoing client's editor shows what a fresh load of its document shows.
Comment on lines
+5759
to
+5761
| if (l !== left) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Optional, non-blocking: this skip (and the ones at 5742-5749 and 5770-5772) cannot fire today: the scan and the write run back to back, and the only changes between them are merges the recorded ids survive and the nulls this loop writes itself. If a later change does make one fire, the undo writes no end and the restored value runs on with no signal. A dev-only warning here would make that show up in the fuzzes.
|
|
||
| [fix] An undo that ends a restored run keeps the document's item list linked: the client that undid no longer holds an item a walk of its list skips, which could make its editor stop matching its document | ||
|
|
||
| Verify: two clients synced after every step; one deletes two characters of a line, the other comments the end of one line and part of another and bolds part of a third, the first types " new " and deletes part of it, removes the first comment and undoes that. Every item's neighbours link back to it on both clients. |
There was a problem hiding this comment.
Optional nit: CONTRIBUTING.md asks Verify: to say what to test, but the last sentence is a structural check users cannot observe. Something like "after the undo, the undoing client's editor shows what a fresh load of its document shows" fits better.
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
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 andwrote 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:
closeRestoredRunskeeps ids, not items, across the two transactions, andresolves 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.
isKeptByUndo) are looked up by id in thestep's insertions instead of by the item objects an earlier transaction
saw.
misLinkedItems, run byexpectConverged, the undofuzz 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
After