Repository navigation
[lexical-y] Bug Fix: A write recovered from after it failed part way does not repeat what it stored - #9697
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:43
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
zurfyx
force-pushed
the
zurfyx/y-recovered-write-diff
branch
from
November 1, 2026 04:45
0fb5a41 to
ceb23e7
Compare
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
force-pushed
the
zurfyx/y-recovered-write-diff
branch
from
November 1, 2026 04:56
ceb23e7 to
38c6cd9
Compare
![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. Skipping the keystroke fast path while rewriteAll is set (packages/lexical-y/src/internal/Write.ts:2962) makes the recovery rewrite diff each changed line against what the document already holds, so edits the failed write stored are kept instead of written again. The new test fails without the change and passes with it, for both client-id orders and with garbage collection on and off.
Two optional, non-blocking suggestions for packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts:
- Add a stored-delete case. The test stores only an insertion before the throw (lines 52-55). Replaying a stored delete is the more damaging half of the same bug: with the same injection, deleting
alphain the first line, typingCHANGEin the second and failing on an insert in the third leavesparagraph("one CHANGE wo CHANGE gamma delta") / …without this fix (two paragraphs merged, text lost) and the expected three paragraphs with it.packages/lexical-y/AGENTS.mdasks every bug fix to land with a regression test insrc/__tests__/unit/regressions/; a deletion case would keep that consequence covered as well. - Assert what
expectConvergedchecks. The recovery warning rules outexpectConverged(it rejectscould not be written), so the test also skips that helper's checks ofpeer.errors, self-check warnings andmisLinkedItems(peer.doc). Adding those three assertions would stop a recovery that reads right but raises an editor error, a self-check mismatch or a mis-linked item from passing.
Comment on lines
+52
to
+55
| t.a.update(() => { | ||
| $selectIn([0], 3).insertText(' INSERT'); | ||
| $selectIn([1], 3).insertText(' CHANGE'); | ||
| }); |
There was a problem hiding this comment.
Optional: a sibling case that stores a delete before the throw (for example $selectIn([0], 3, 9).removeText(), then an insert in the second line, with the throw on an insert in the third) fails without the fix with two paragraphs merged and text lost (paragraph("one CHANGE wo CHANGE gamma delta") / …) and passes with it. It would cover the data-loss half of the same bug.
…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.
…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
force-pushed
the
zurfyx/y-undo-run-end-links
branch
from
November 1, 2026 05:15
896ff92 to
d7773fc
Compare
zurfyx
force-pushed
the
zurfyx/y-recovered-write-diff
branch
from
November 1, 2026 05:20
38c6cd9 to
f0a8dc5
Compare
There was a problem hiding this comment.
This looks good to me. With rewriteAll set, the recovery rewrite now skips $writeTextEdit, the one path that writes Lexical's recorded edits at previous-state offsets without reading the stored line. The line diff it falls back to keeps an old character only where it matches the stored one, so text the failed write already stored can't be written twice. The undo grouping is careful too: the rewrite joins the failed write's step only when that write was actually captured (a new step, or the last step's id sets replaced), A failed write that stored nothing leaves captureTimeout alone, so that rewrite is grouped like any other write.
A few optional, non-blocking suggestions:
- Changeset (
.changeset/y-recovered-write-diff.md): it describes only the repeat fix. AGENTS.md and.changeset/README.mdask a fix that changes observable behavior to say inVerify:what to test. The undo-step change from the second commit could go there too, for example: "…and one undo reverts the whole change, even withcaptureTimeout: 0." - Test plan: the pasted output shows 2 tests, but
RecoveredWriteRepeats.test.tsnow has 4, and the description doesn't mention the undo-step fix. AGENTS.md asks the Test plan to carry the real Before/After output of the command that was run, so refreshing both would help readers. - Extended-step branch (
packages/lexical-y/src/internal/Binding.ts:4284-4288): the tests reach only the new-step branch ofcaptured(), becausePeer.updatecallsstopCapturing()and the second test setscaptureTimeout = 0. The other branch,last.inserts !== lastInserts || last.deletes !== lastDeletes, relies onUndoManagerreplacing a step's id sets when it merges a transaction into the step. @y/y 14.0.0-rc.29 does that throughmergeIdSets, but nothing pins it. One more case would: an edit, then a failed write within the capture timeout througheditor.updatewithoutstopCapturing(), then a rewrite that outlasts the timeout. That fitspackages/lexical-y/AGENTS.md's testing guidance and would catch a future @y/y that merges in place.
| const last = stack === null || depth === 0 ? null : stack[depth - 1]; | ||
| const lastInserts = last === null ? null : last.inserts; | ||
| const lastDeletes = last === null ? null : last.deletes; | ||
| const captured = () => |
There was a problem hiding this comment.
Optional: the new tests reach only the stack.length !== depth branch. The last.inserts !== lastInserts || last.deletes !== lastDeletes branch relies on UndoManager replacing the id sets when it merges into the last step (mergeIdSets in @y/y 14.0.0-rc.29). A case with an edit followed, within the capture timeout, by a failed write through editor.update without stopCapturing() would pin it.
…vered write can be two undo steps (D97, open)
## Description
The failed write and its rewrite are two transactions, so a rewrite that
takes longer than the undo manager's `captureTimeout` (a very long line)
lands as its own undo step. Suspending the capture timeout during the
rewrite was tried and dropped in review: it merged the rewrite into an
app's own step where the app wrote between the two, and did nothing for an
app's `UndoManager` tracking the same origin. Recorded as D97 (`OPEN`:
accept as a limit, or associate the two at the transaction level), with one
sentence in y.md's Limits (Undo), and pinned as `test.fails` naming D97.
## Test plan
### Before
Not recorded.
### After
```
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts
Tests 2 passed | 2 expected fail (4)
$ LEXICAL_Y_GC=1 (same file)
Tests 2 passed | 2 expected fail (4)
```
The pinned cell uses a capture timeout of 0, which stands for a rewrite
slower than any timeout (the same window as a million-character line at
20 ms), so it does not depend on the machine's speed.
zurfyx
force-pushed
the
zurfyx/y-recovered-write-diff
branch
from
November 1, 2026 05:46
f0a8dc5 to
195d750
Compare
There was a problem hiding this comment.
This looks good to me. Skipping $writeTextEdit when rewriteAll is set makes the recovery rewrite diff each line against the stored document, and the new regression test fails before the change and passes after it. A few optional, non-blocking suggestions:
- Pin the stored-delete case too (
packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts:30). Before this change a stored delete was the more damaging variant: deletingalphainone alpha beta, with the failure injected on the next line's insert, left the other client and fresh readers withone CHANGE wo gamma delta(two lines merged,betalost). It converges with this change; a loop for it in this file would keep it that way (packages/lexical-y/AGENTS.md: every bug fix lands with a regression test insrc/__tests__/unit/regressions/). - The D97 trigger (
packages/lexical-website/docs/collaboration/y.md:1331and the D97 row inpackages/lexical-y/DECISIONS.md). The recovery rewrites every line of the document, so its duration grows with the document rather than with one line: at the defaultcaptureTimeoutof 500 ms, a document of 16,000 ordinary 100-character lines becomes two undo steps, while a single 4,000,000-character line stays one. "(a large document, or a very long line)" would point readers at the case they will meet. - Changeset wording (
.changeset/y-recovered-write-diff.md:5). The old recovery could also lose text after a stored delete or replacement, not only repeat it. "…without repeating or losing what it already stored", with aVerify:that ends "with nothing around it lost", would tell people upgrading (.changeset/README.md: the entry is written for someone who uses Lexical). - Comment wording (
packages/lexical-y/src/internal/Write.ts:2959-2961). In the rewrite the plan carries notextEdits, so the fast path wrote a diff of the node's previous and current text rather than Lexical's recorded edits; "edits computed against the previous editor state" names the actual cause. - Test plan output. The After block shows
Tests 2 passed (2)for the new file, which now reportsTests 2 passed | 2 expected fail (4)with the D97test.fails; the rootAGENTS.mdasks for the real output of the command that was run.
| could not write and recovered from (reported as | ||
| `@lexical/y/recovered-write`) can land as two undo steps: what the failed | ||
| write stored, and its rewrite, when the rewrite takes longer than the | ||
| undo manager's `captureTimeout` (a very long line). |
There was a problem hiding this comment.
Optional: the recovery rewrites every line of the document, so its duration grows with the document rather than with one line. At the default captureTimeout of 500 ms, a document of 16,000 ordinary 100-character lines becomes two undo steps, while a single 4,000,000-character line stays one. "(a large document, or a very long line)" would point readers at the case they will meet; the D97 row in packages/lexical-y/DECISIONS.md could say the same.
Comment on lines
+2959
to
+2961
| // A rewrite after a failed write diffs the line against the document: | ||
| // the edits Lexical recorded may be stored already (the failed write | ||
| // stored part of its change), and writing them again would repeat them. |
There was a problem hiding this comment.
Nit (optional): in the rewrite the plan carries no textEdits, so exactEdits returns null and $writeTextEdit wrote a diff of the node's previous and current text, not Lexical's recorded edits. Something like "the edits computed against the previous editor state may be stored already" names the actual cause.
| [1, 2], | ||
| [2, 1], | ||
| ]) { | ||
| test(`the rewrite after a failed write does not repeat what it stored (ids ${ids})`, async () => { |
There was a problem hiding this comment.
Optional: a stored delete was the more damaging case before this fix. With $selectIn([0], 4, 10).removeText() in place of the line 1 insert and the failure injected on the first item, the other client and fresh readers read one CHANGE wo gamma delta (two lines merged, beta lost). It converges with this change; a loop for it here would keep it that way (packages/lexical-y/AGENTS.md: every bug fix lands with a regression test in src/__tests__/unit/regressions/).
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] A write that fails part way is recovered from without repeating what it already stored: the rewrite diffs each line against the document instead of writing the recorded keystrokes again |
There was a problem hiding this comment.
Optional: before this fix the recovery could also lose text, not only repeat it (a stored delete merged two lines on the other clients). "…without repeating or losing what it already stored", with a Verify: that ends "with nothing around it lost", would tell people upgrading (.changeset/README.md: the entry is written for someone who uses Lexical).
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 #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 achanged line through the keystroke fast path (
$writeTextEdit), whichwrites 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
After
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.