diff --git a/.changeset/y-recovered-write-diff.md b/.changeset/y-recovered-write-diff.md new file mode 100644 index 00000000000..c2e0be79613 --- /dev/null +++ b/.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. diff --git a/.changeset/y-undo-run-end-links.md b/.changeset/y-undo-run-end-links.md new file mode 100644 index 00000000000..4fa3d582361 --- /dev/null +++ b/.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. diff --git a/packages/lexical-website/docs/collaboration/y.md b/packages/lexical-website/docs/collaboration/y.md index 180273ed710..7b45e4f3e84 100644 --- a/packages/lexical-website/docs/collaboration/y.md +++ b/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 diff --git a/packages/lexical-y/DECISIONS.md b/packages/lexical-y/DECISIONS.md index a3f456a3350..db3a7c2e0f9 100644 --- a/packages/lexical-y/DECISIONS.md +++ b/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 diff --git a/packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts b/packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts index 459bc111d96..ecd30541c1e 100644 --- a/packages/lexical-y/src/__tests__/unit/StructureFuzz.test.ts +++ b/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 { diff --git a/packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts index 9cd6f7fc40b..250919fa916 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/CrossContainerEnd.test.ts +++ b/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. diff --git a/packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/RecoveredWriteRepeats.test.ts new file mode 100644 index 00000000000..681508b9428 --- /dev/null +++ b/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); + }, + ); +} diff --git a/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts new file mode 100644 index 00000000000..f89f943952e --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndLinks.test.ts @@ -0,0 +1,190 @@ +/** + * 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 + +// 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 split items and merged them back, so the item to its +// left could be a piece no longer in the store. The null was linked to that +// piece: the item in the store kept its old right, so a walk of the list +// skipped the null (a broken link, on the undoing client only). Two clients, +// synced after every step, reduced from an undo fuzz: B deletes two +// characters; A comments the end of one line and part of another (one step) +// and bolds part of the first; B types " new " and deletes part of it, +// removes A's first comment and undoes that. + +import { + buildEditorFromExtensions, + getExtensionDependencyFromEditor, +} from '@lexical/extension'; +import {$addMarkID, $removeMarkID, MarkStateExtension} from '@lexical/mark'; +import {RichTextExtension} from '@lexical/rich-text'; +import * as Y from '@y/y'; +import { + $createParagraphNode, + $createRangeSelection, + $createTextNode, + $getRoot, + $isElementNode, + $isTextNode, + $setSelection, + configExtension, + defineExtension, + type LexicalNode, + type TextNode, + UNDO_COMMAND, +} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +import {createYDoc, YCollabExtension} from '../../..'; +import {seedYState} from '../../../headless'; +import {misLinkedItems} from '../../utils/structure'; + +const EXTENSIONS = [RichTextExtension, MarkStateExtension]; +const WORDS = ['alpha', 'beta', 'gamma', 'delta', 'note', 'run']; + +type Op = {e: number; end?: boolean} & ( + | {k: 'add'; t: number; a: number; b: number; id: string} + | {k: 'remove'; id: string} + | {k: 'format'; t: number; a: number; b: number; f: 'bold'} + | {k: 'type'; t: number; a: number; s: string} + | {k: 'del'; t: number; a: number; b: number} + | {k: 'undo'} +); + +// The undo fuzz's text node indexes (modulo the text nodes there are). +const OPS: Op[] = [ + {a: 18, b: 20, e: 1, k: 'del', t: 1}, + {a: 25, b: 34, e: 0, id: 'c2', k: 'add', t: 13}, + {a: 6, b: 8, e: 0, end: true, id: 'c4', k: 'add', t: 1}, + {a: 21, b: 25, e: 0, f: 'bold', k: 'format', t: 0}, + {a: 19, e: 1, k: 'type', s: ' new ', t: 6}, + {a: 3, b: 5, e: 1, k: 'del', t: 6}, + {e: 1, id: 'c2', k: 'remove'}, + {e: 1, k: 'undo'}, +]; + +function $texts(): TextNode[] { + const out: TextNode[] = []; + const walk = (n: LexicalNode) => { + if ($isTextNode(n) && n.getType() === 'text') { + out.push(n); + } + if ($isElementNode(n)) { + n.getChildren().forEach(walk); + } + }; + walk($getRoot()); + return out; +} + +function $apply(op: Op) { + if (op.k === 'undo') { + return; + } + if (op.k === 'remove') { + $removeMarkID(op.id); + return; + } + const texts = $texts(); + const t = texts[op.t % Math.max(1, texts.length)]; + const size = t.getTextContentSize(); + const s = $createRangeSelection(); + const b = op.k === 'type' ? op.a : op.b; + s.anchor.set(t.getKey(), Math.min(op.a, size), 'text'); + s.focus.set(t.getKey(), Math.min(b, size), 'text'); + $setSelection(s); + if (op.k === 'add') { + $addMarkID(op.id, s); + } else if (op.k === 'format') { + s.formatText(op.f); + } else if (op.k === 'type') { + s.format = 0; + s.insertText(op.s); + } else { + s.removeText(); + } + $setSelection(null); +} + +describe('an undo that ends a restored run', () => { + for (const gc of [false, true]) { + test(`keeps the list's links whole (gc ${gc ? 'on' : 'off'})`, () => { + const seeded = seedYState( + {extensions: EXTENSIONS, schemaVersion: 1}, + () => { + const root = $getRoot().clear(); + WORDS.forEach((word, i) => + root.append( + $createParagraphNode().append( + $createTextNode(`${word} one two three four five ${i}`), + ), + ), + ); + }, + ); + const peers = [0, 1].map(i => { + const doc = createYDoc(seeded, {gc}); + const editor = buildEditorFromExtensions( + defineExtension({ + dependencies: [ + ...EXTENSIONS, + configExtension(YCollabExtension, { + captureTimeout: 2 ** 30, + doc, + schemaVersion: 1, + synced: true, + }), + ], + name: `run-end-links/${i}`, + onError: error => { + throw error; + }, + }), + ); + const {binding} = getExtensionDependencyFromEditor( + editor, + YCollabExtension, + ).output; + return {binding, doc, editor}; + }); + const sync = () => { + const [a, b] = peers; + Y.applyUpdate( + b.doc, + Y.encodeStateAsUpdate(a.doc, Y.encodeStateVector(b.doc)), + ); + Y.applyUpdate( + a.doc, + Y.encodeStateAsUpdate(b.doc, Y.encodeStateVector(a.doc)), + ); + }; + try { + for (const [i, op] of OPS.entries()) { + const {binding, editor} = peers[op.e]; + if (op.k === 'undo') { + editor.dispatchCommand(UNDO_COMMAND, undefined); + } else { + editor.update(() => $apply(op), {discrete: true}); + } + if (op.end) { + binding.undoManager?.stopCapturing(); + } + sync(); + for (const peer of peers) { + expect(misLinkedItems(peer.doc), `after step ${i}`).toEqual([]); + } + } + } finally { + peers.forEach(p => p.editor.dispose()); + } + }); + } +}); diff --git a/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndSharedGap.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndSharedGap.test.ts new file mode 100644 index 00000000000..0eb85bbfabd --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/UndoRunEndSharedGap.test.ts @@ -0,0 +1,103 @@ +/** + * 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 + +// An undo that brings back two values ending at the same place writes an end +// of its own for each where a collaborator's end is gone (closeRestoredRuns). +// A bolds and italicizes "word"; B removes both in one step; A's bold end is +// deleted (by id, as the other restore-end tests do), its italic end stays; +// B undoes. The second key must not read the first key's new null as a +// change of the gap and skip its own: when A's italic end goes later, italic +// would run on over " plain tail". + +import * as Y from '@y/y'; +import { + $createParagraphNode, + $createTextNode, + $getRoot, + $setSelection, + type TextNode, +} from 'lexical'; +import {expect, test} from 'vitest'; + +import {$block, $selectIn, freshReader, setup} from '../../utils/harness'; + +for (const gc of [false, true]) { + test(`an undo restoring two values at one place ends both (gc ${gc ? 'on' : 'off'})`, async () => { + const t = await setup( + () => + $getRoot().append( + $createParagraphNode().append($createTextNode('word plain tail')), + ), + {ids: [1001, 1002, 1003]}, + ); + t.peers.forEach(p => (p.doc.gc = gc)); + const $formats = () => + ($block([0]).getAllTextNodes() as TextNode[]).map(n => [ + n.getTextContent(), + n.getFormat(), + ]); + try { + t.a.update(() => { + const s = $selectIn([0], 0, 4); + s.formatText('bold', 1); + s.formatText('italic', 2); + $setSelection(null); + }); + await t.sync(); + let boldEnd: Y.ID | null = null; + let italicEnd: Y.ID | null = null; + for (let it = t.a.doc.get('lexical-y')._start; it; it = it.right) { + if ( + it.content instanceof Y.ContentFormat && + it.content.value === null + ) { + if (it.content.key === 'f:bold') { + boldEnd = it.id; + } else if (it.content.key === 'f:italic') { + italicEnd = it.id; + } + } + } + expect(boldEnd).not.toBeNull(); + expect(italicEnd).not.toBeNull(); + t.b.update(() => { + const s = $selectIn([0], 0, 4); + s.formatText('bold', 0); + s.formatText('italic', 0); + $setSelection(null); + }); + await t.sync(); + t.a.doc.transact(tr => + (t.a.doc.store.getItem(boldEnd!) as Y.Item).delete(tr), + ); + await t.sync(); + t.b.undo(); + await t.sync(); + t.a.doc.transact(tr => + (t.a.doc.store.getItem(italicEnd!) as Y.Item).delete(tr), + ); + await t.sync(); + for (const peer of t.peers) { + expect(peer.read($formats)).toEqual([ + ['word', 3], + [' plain tail', 0], + ]); + } + const fresh = await freshReader(t.b); + expect(fresh.read($formats)).toEqual([ + ['word', 3], + [' plain tail', 0], + ]); + fresh.stop(); + } finally { + t.peers.forEach(p => p.stop()); + } + }); +} diff --git a/packages/lexical-y/src/__tests__/utils/harness.ts b/packages/lexical-y/src/__tests__/utils/harness.ts index 8ed55925269..e00376f244d 100644 --- a/packages/lexical-y/src/__tests__/utils/harness.ts +++ b/packages/lexical-y/src/__tests__/utils/harness.ts @@ -478,6 +478,12 @@ export async function expectConverged(peers: Peer[], expected?: string) { for (const peer of peers) { expect(peer.errors).toEqual([]); expect(peer.warnings.filter(w => w.includes('self-check'))).toEqual([]); + // A write the binding could not store is recovered from (rewritten from + // the editor) and reported: never expected in a test that converges, as + // a recovery could hide the failure it recovered from. + expect( + peer.warnings.filter(w => w.includes('could not be written')), + ).toEqual([]); expect(misLinkedItems(peer.doc)).toEqual([]); expect(peer.describe()).toBe(first.describe()); expect(shape(peer)).toEqual(shape(first)); diff --git a/packages/lexical-y/src/__tests__/utils/structure.ts b/packages/lexical-y/src/__tests__/utils/structure.ts index 5428371cede..c2737287b89 100644 --- a/packages/lexical-y/src/__tests__/utils/structure.ts +++ b/packages/lexical-y/src/__tests__/utils/structure.ts @@ -9,11 +9,13 @@ import * as Y from '@y/y'; /** - * The items of `doc` whose left or right neighbour belongs to another shared - * type (`client:clock` of each). A well-formed document has none: such an item - * is linked into one type's list while it names another as its parent, which - * only the document that wrote it holds (every other client integrates it - * from its origin), and that document's reads then go wrong. + * The items of `doc` whose list links are broken (`client:clock` of each): a + * left or right neighbour that belongs to another shared type (or another key + * of a map), or that does not link back to the item. A well-formed document + * has none. An item linked into one type's list while it names another as its + * parent, or one a forward walk of its list skips, is held only by the + * document that wrote it (every other client integrates it from its origin), + * and that document's reads then go wrong. */ export function misLinkedItems(doc: Y.Doc): string[] { const out: string[] = []; @@ -22,9 +24,16 @@ export function misLinkedItems(doc: Y.Doc): string[] { if (!(s instanceof Y.Item)) { continue; } + const {left, right} = s; if ( - (s.left !== null && s.left.parent !== s.parent) || - (s.right !== null && s.right.parent !== s.parent) + (left !== null && + (left.parent !== s.parent || + left.parentSub !== s.parentSub || + left.right !== s)) || + (right !== null && + (right.parent !== s.parent || + right.parentSub !== s.parentSub || + right.left !== s)) ) { out.push(`${s.id.client}:${s.id.clock}`); } diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 9eeb8ac86f8..84ca1f5d340 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -24,6 +24,9 @@ import { encodeStateAsUpdate, encodeStateVector, findIndexSS, + getItemCleanStart, + type ID, + type IdSet, Item, iterateStructsByIdSet, Node as YNode, @@ -831,8 +834,8 @@ export class YBinding { private restoring: Set | null = null; /** Stored nodes the running undo/redo deletes (it inserted them). */ private removing: Set | null = null; - /** The items the running undo/redo deletes (see isKeptByUndo). */ - private undoing: Set | null = null; + /** The ids of the items the running undo/redo deletes (see isKeptByUndo). */ + private undoing: IdSet | null = null; private wasEditable = true; private undoCache = new WeakMap>(); private readonly afterTransaction: (tr: Transaction) => void; @@ -5445,7 +5448,7 @@ export class YBinding { if (!(it.content instanceof ContentFormat) || it.content.key !== key) { continue; } - if (this.undoing.has(it)) { + if (this.undoing.hasId(it.id)) { if (it.deleted) { return true; } @@ -5650,8 +5653,14 @@ export class YBinding { const doc = this._doc; const store = doc.store; const client = doc.clientID; - // Where to write each null: after `left`, before `right`. - const ends: [left: Item | null, right: Item | null, key: string][] = []; + // Where to write each null: after the item that ends at `leftEnd`, + // before the item that starts at `right` (null: at that end of the + // list). Ids, not items: the scan's + // transaction splits items and its cleanup merges them back, so an item + // object it saw can be a piece no longer in the store by the time the + // null is written (an item linked to such a piece breaks the list's + // links). + const ends: [leftEnd: ID | null, right: ID | null, key: string][] = []; doc.transact(tr => { const copies: [original: Item, copy: Item][] = []; iterateStructsByIdSet(tr, step.deletes, s => { @@ -5681,7 +5690,7 @@ export class YBinding { for (let it = copy.right; it !== null; it = it.right) { if (it === gone && it.deleted) { if (shown) { - ends.push([it, it.right, key]); + ends.push([it.lastId, it.right && it.right.id, key]); } break; } @@ -5695,7 +5704,7 @@ export class YBinding { it.id.client !== client && it.content.value === null ) { - ends.push([it.left, it, key]); + ends.push([it.left && it.left.lastId, it.id, key]); } break; } @@ -5707,16 +5716,66 @@ export class YBinding { }, this.scanOrigin); if (ends.length > 0) { doc.transact(tr => { - for (const [left, right, key] of ends) { - // In the type of the item it goes before, never next to an item - // of another type (see checkLinkable in Cursor). - if (left !== null && right !== null && left.parent !== right.parent) { + // The nulls this loop writes: two keys can need an end in the same + // gap, and the second must not take the first's null for a change + // of the gap. + const written = new Set(); + for (const [leftEnd, rightStart, key] of ends) { + // The items as the store holds them now, split at those ids. + const right = + rightStart === null ? null : getItemCleanStart(tr, rightStart); + let left: Item | null = null; + if (leftEnd !== null) { + const at = store.getItem(leftEnd); + if ( + at instanceof Item && + at.id.clock + at.length - 1 > leftEnd.clock + ) { + getItemCleanStart( + tr, + createID(leftEnd.client, leftEnd.clock + 1), + ); + } + const found = store.getItem(leftEnd); + left = found instanceof Item ? found : null; + } + if ( + (rightStart !== null && !(right instanceof Item)) || + (leftEnd !== null && left === null) || + (left === null && right === null) || + (left !== null && right !== null && left.parent !== right.parent) + ) { continue; } + // Still next to each other but for the nulls written here, in one + // type (never next to an item of another type: see checkLinkable + // in Cursor). The null goes right before `right`, after those. + let before: Item | null; + if (right !== null) { + let l = right.left; + while (l !== null && written.has(l)) { + l = l.left; + } + if (l !== left) { + continue; + } + before = right.left; + } else { + let last = left as Item; + let r = last.right; + while (r !== null && written.has(r)) { + last = r; + r = r.right; + } + if (r !== null) { + continue; + } + before = last; + } const item = new Item( createID(client, store.getClock(client)), - left, - left && left.lastId, + before, + before && before.lastId, right, right && right.id, (left ?? right)!.parent, @@ -5724,6 +5783,7 @@ export class YBinding { new ContentFormat(key, null as unknown as object), ); item.integrate(tr, 0); + written.add(item); markFormatCloser(item); } }, this.runEndOrigin); @@ -5744,7 +5804,6 @@ export class YBinding { } const names = new Set(); const removing = new Set(); - const undoing = new Set(); const isStored = (s: unknown): s is Item => { if (!(s instanceof Item) || !(s.content instanceof ContentType)) { return false; @@ -5759,9 +5818,6 @@ export class YBinding { } }); iterateStructsByIdSet(tr, top.inserts, s => { - if (s instanceof Item) { - undoing.add(s); - } if (isStored(s)) { removing.add(s); } @@ -5769,7 +5825,9 @@ export class YBinding { }, this.scanOrigin); this.restoring = names; this.removing = removing; - this.undoing = undoing; + // 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. + this.undoing = top.inserts; } // ------------------------------------------------------------------------- diff --git a/packages/lexical-y/src/internal/Cursor.ts b/packages/lexical-y/src/internal/Cursor.ts index b480e69c465..9abb3ca5f08 100644 --- a/packages/lexical-y/src/internal/Cursor.ts +++ b/packages/lexical-y/src/internal/Cursor.ts @@ -148,8 +148,9 @@ export const markFormatCloser = (item: Item): void => { * Refuse to link a new item next to `left` and `right` unless both are in * `parent`, the type it is written to. An item whose neighbour sits in * another type is malformed: its own document links it into that type's list - * while every other client integrates it into `parent`, and later reads of - * that document go wrong. A write that hits this throws, and is recovered + * while naming `parent` (every other client takes its parent from its origin, + * so only this document holds it wrong), and later reads of that document go + * wrong. A write that hits this throws, and is recovered * from (RECOVERED_WRITE_WARNING) instead of storing the item. */ function checkLinkable( diff --git a/packages/lexical-y/src/internal/Write.ts b/packages/lexical-y/src/internal/Write.ts index 9dec2ec4a72..88e5be75cf8 100644 --- a/packages/lexical-y/src/internal/Write.ts +++ b/packages/lexical-y/src/internal/Write.ts @@ -2956,7 +2956,10 @@ export class Writer { /** Rewrite one line (its content and marker) on its own. */ $writeLineAlone(u: Unit, d: Desired): void { - if (this.$writeTextEdit(u, d)) { + // 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. + if (!this.rewriteAll && this.$writeTextEdit(u, d)) { return; } const old = u.cs.units;