[lexical-y] Bug Fix: A write recovered from after it failed part way does not repeat what it stored

.changeset/y-recovered-write-diff.md

@@ -0,0 +1,7 @@
+---
+'@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
+
+Verify: when a local write throws after storing part of a change (reported as `@lexical/y/recovered-write`), every client shows the change once, and the author's editor matches its document.

.changeset/y-undo-run-end-links.md

@@ -0,0 +1,7 @@
+---
+'@lexical/y': patch
+---
+
+[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.

packages/lexical-website/docs/collaboration/y.md

@@ -1324,7 +1324,11 @@ not merge as a user might expect:
   `Kilo` bolded by one client, another unbolds `il` or types plain text
   inside `Kilo` as the first or a third bolds ` bb` (or `lo bb`), and with
   some client id orders ` bb` stays plain. No text is lost, and when that
-  client undoes its edit, ` bb` is bold again.
+  client undoes its edit, ` bb` is bold again. A local change the binding
+  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).
 - **Large lists.** A structural edit in a list costs time proportional to
   the list on every client.
 - **Older app versions.** Clients of different `schemaVersion`s never

packages/lexical-y/DECISIONS.md

@@ -58,7 +58,7 @@ issues and docs that carry it.
 | 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] |
+| 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 took the item's parent from its origin and put it in the root's list, where it changes nothing, 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] |
 | D78 | **A format spreading after one client's triple undo:** a client formats a run, types plain text, formats again and undoes all three while a collaborator types plain text inside the first run. | Fixed at the source: a restore's scan (`runOpener`) stops at content of another client, so the writer's earlier end and later opener with a collaborator's text between them are no longer read as one restored run, and the undo writes the run's end. A two-client run-sequence fuzz: 8 of 1,000 seeds to 0, gc on and off. | `DECIDED` | 10-31 | [#9599] |
 | D25 | **Undo is Yjs's `UndoManager`**, scoped to the binding's root and this client's own edits (500 ms capture by default). It never deletes a container a collaborator has put content in (its `deleteFilter`). Do not combine it with `HistoryExtension`. | Collaborative undo must not revert other people's work; keeping a container whole once someone else wrote in it matches y-prosemirror's protected nodes and is cheap to maintain. The `UndoManager` was chosen on 10-23; keeping containers whole was confirmed on 10-30. | `DECIDED` | 10-30 | [#9400], [y.md: Limits][limits] |
 | D26 | **Undo favours people taking turns over concurrent races.** Where a fix makes undo correct for sequential edits at the cost of rare races between an undo and a simultaneous edit of the same text, the sequential case wins. | Taking turns is the common case; a race with a simultaneous edit of the same text is rare. Picked on 10-28, confirmed on 10-30. | `DECIDED` | 10-30 | [#9400] |
@@ -132,6 +132,7 @@ issues and docs that carry it.
 
 | ID  | Question | Where it stands | Status | Date | Links |
 | --- | -------- | --------------- | ------ | ---- | ----- |
+| D97 | **A recovered write can be two undo steps.** When a local change cannot be written part way and the binding rewrites the document from the editor (`RECOVERED_WRITE_WARNING`), what the failed write stored and the rewrite are two transactions; a rewrite that takes longer than the undo manager's `captureTimeout` (a line of about a million characters at 20 ms) lands as its own undo step. | Joining them by suspending the capture timeout 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 ([#9697] review). Accept as a limit (documented in [Limits][limits]), or associate the two at the transaction level. | `OPEN` | 11-01 | [#9697], [limits] |
 | D59 | **Block containers in the node structure.** A container that is not a shadow root counts as one block, so `$insertList` replaces it; a box inside a box loses the outer box. | The rule should live in `NodeStructureExtension`. Draft in progress. | `OPEN` | 10-30 | [#9570] |
 | D60 | **Code highlighting as display-only.** Today highlighting is stored token nodes, so code written without the highlighter opens plain until edited (D21). | Right long term, but intrusive; documented for now. | `OPEN` | 10-30 | [#9536], [#9558] |
 | D61 | **Two concurrent joins into one line** both write its type; Yjs keeps one by client id. | Inherent to the marker layout; a real trade-off. | `OPEN` | 10-30 | [#9511] |
@@ -236,3 +237,4 @@ issues and docs that carry it.
 [yjs#382]: https://github.com/yjs/yjs/issues/382
 [#9679]: https://github.com/meta/lexical/pull/9679
 [#9694]: https://github.com/meta/lexical/issues/9694
+[#9697]: https://github.com/meta/lexical/pull/9697

packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts

@@ -152,6 +152,9 @@ function gen(seed: number): Op[] {
 for (let seed = FIRST; seed < FIRST + SEEDS; seed++) {
   test(`seed ${seed}: no item has a neighbour in another container (gc ${GC ? 'on' : 'off'})`, () => {
     const errors: unknown[] = [];
+    // A write the binding refuses is recovered from and only warned about:
+    // record the warnings so a failing seed is named.
+    const warnings: string[] = [];
     const docs = [0, 1].map(i => {
       const doc = createYDoc(null, {gc: GC});
       doc.clientID = 1001 + i;
@@ -171,6 +174,7 @@ for (let seed = FIRST; seed < FIRST + SEEDS; seed++) {
           ],
           name: `structure/${i}`,
           onError: e => errors.push(e),
+          onWarn: e => warnings.push(String(e && (e as Error).message)),
         }),
       ),
     );
@@ -191,6 +195,7 @@ for (let seed = FIRST; seed < FIRST + SEEDS; seed++) {
         for (const doc of docs) {
           expect(misLinkedItems(doc), `after step ${i}`).toEqual([]);
         }
+        expect(warnings, `after step ${i}`).toEqual([]);
       }
       expect(errors).toEqual([]);
     } finally {

packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts

@@ -15,8 +15,9 @@
 // in the paragraph before the table, and B recolors part of the cell. B's
 // write linked its end after A's item in the root while naming the cell as
 // its parent: B's own document held an item in the wrong list (every other
-// client placed it in the cell), and later changes next to it made B's
-// editor stop reading part of the document, for good. A run cannot end in
+// client takes the parent from the item's origin and puts it in the root,
+// where it changes nothing), and later changes next to it made B's editor
+// stop reading part of the document, for good. A run cannot end in
 // another container: changeEnd only looks in the run's own, and no item is
 // written next to an item of another container.
 

packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts

@@ -0,0 +1,125 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ */
+
+// @vitest-environment node
+
+// A write that fails part way is recovered from: the document is rewritten
+// from the editor. The rewrite wrote a changed line through the keystroke
+// fast path, 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 diffs each line against the document instead. The failure is
+// injected (the second item a cursor writes throws).
+
+import {$getRoot} from 'lexical';
+import {expect, test} from 'vitest';
+
+import {Cursor} from '../../../internal/Cursor';
+import {$p, $selectIn, freshReader, setup} from '../../utils/harness';
+
+for (const ids of [
+  [1, 2],
+  [2, 1],
+]) {
+  test(`the rewrite after a failed write does not repeat what it stored (ids ${ids})`, async () => {
+    const t = await setup(
+      () =>
+        $getRoot().append(
+          $p('one alpha beta'),
+          $p('two gamma delta'),
+          $p('untouched words here'),
+        ),
+      {ids},
+    );
+    const proto = Cursor.prototype as unknown as {
+      newItem: (...args: unknown[]) => unknown;
+    };
+    const newItem = proto.newItem;
+    let calls = 0;
+    proto.newItem = function (this: unknown, ...args: unknown[]) {
+      if (++calls === 2) {
+        throw new Error('injected failure');
+      }
+      return newItem.apply(this, args);
+    };
+    try {
+      t.a.update(() => {
+        $selectIn([0], 3).insertText(' INSERT');
+        $selectIn([1], 3).insertText(' CHANGE');
+      });
+    } finally {
+      proto.newItem = newItem;
+    }
+    expect(
+      t.a.warnings.filter(w => w.includes('could not be written')),
+    ).toHaveLength(1);
+    await t.sync();
+    const expected =
+      'paragraph("one INSERT alpha beta") / paragraph("two CHANGE gamma delta") / paragraph("untouched words here")';
+    for (const peer of t.peers) {
+      expect(peer.describe()).toBe(expected);
+      const fresh = await freshReader(peer);
+      expect(fresh.describe()).toBe(expected);
+      fresh.stop();
+    }
+    t.a.undo();
+    await t.sync();
+    for (const peer of t.peers) {
+      expect(peer.describe()).toBe(
+        'paragraph("one alpha beta") / paragraph("two gamma delta") / paragraph("untouched words here")',
+      );
+    }
+  });
+}
+
+// The failed write and its rewrite are two transactions: a rewrite slower
+// than the undo manager's capture timeout (a very long line) makes one edit
+// two undo steps. An open question (D97): accept it as a limit, or join the
+// two at the transaction level. A capture timeout of 0 stands for a rewrite
+// slower than any timeout.
+for (const ids of [
+  [1, 2],
+  [2, 1],
+]) {
+  test.fails(
+    `D97: the rewrite after a failed write is in the same undo step (ids ${ids})`,
+    async () => {
+      const t = await setup(
+        () => $getRoot().append($p('one alpha beta'), $p('two gamma delta')),
+        {ids},
+      );
+      const um = t.a.binding.undoManager!;
+      um.captureTimeout = 0;
+      const steps = um.undoStack.length;
+      const proto = Cursor.prototype as unknown as {
+        newItem: (...args: unknown[]) => unknown;
+      };
+      const newItem = proto.newItem;
+      let calls = 0;
+      proto.newItem = function (this: unknown, ...args: unknown[]) {
+        if (++calls === 2) {
+          throw new Error('injected failure');
+        }
+        return newItem.apply(this, args);
+      };
+      try {
+        t.a.update(() => {
+          $selectIn([0], 3).insertText(' INSERT');
+          $selectIn([1], 3).insertText(' CHANGE');
+        });
+      } finally {
+        proto.newItem = newItem;
+      }
+      expect(
+        t.a.warnings.filter(w => w.includes('could not be written')),
+      ).toHaveLength(1);
+      expect(um.undoStack.length).toBe(steps + 1);
+    },
+  );
+}