From 49e675f118cdd71a73fd19edca1753c34720ef05 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Fri, 30 Oct 2026 17:08:41 -0700 Subject: [PATCH] [lexical-y] Bug Fix: End a value an undo brings back where it ended, also when the change that wrote its end is undone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description An undo (or redo) that brings back a format item opening a value (a comment id, a format, a link, a style, a NodeState value) already ends that run with a null of its own when the item that ended it belongs to another change (`closeRestoredRuns`). Two shapes were missed, and the value then covered everything after it, on every client and in a fresh read of the document: - #9623, sequential: B comments a word; A removes the comment (which deletes B's opener); B undoes its comment (which deletes B's closer); A undoes its removal. The undo brought B's opener back, but B's closer was gone, and with garbage collection on (the default) its content was collected, so the binding could no longer find it from B's change (`changeEnd`). - #9620, concurrent: the run's end was still live when the undo ran, but no content showed between the restored item and that end yet, so no null was written; at the same time the end's own change was undone. The binding now notes, for each format item opening a value that a change of this client's deletes, the null that ended its run right after that change (deleted items keep their place, so the end can be found again after collection). When an undo brings the item back and the change's own end cannot be found, the run ends right after that noted end. And a live null of another change that ends the run is now ended before also where no content shows between the restored item and it yet, when that null is the end its change gave a run it opened (`isChangeEnd`): a null by which a change made text inside the run plain (a hole that change restores the value after) is no such end, and ending before it would take the value off the text after the hole. Every such null is written inside the local undo, never while applying a remote change. Recorded as DECISIONS.md D83 (proposed; the number may shift at landing): it writes one null per restored run another change's end closed, also where nothing shows yet. Closes #9623 Closes #9620 ## Test plan - `regressions/UndoRestoredRunEnds.test.ts` (147 cells): the value is a comment id (MarkState), bold, a color style or a NodeState value; a collaborator removes it from the whole word, part of it, or (comments) the whole document; its author undoes its change first, or not; a third client types plain text right after the word, or not; then a redo and another undo; garbage collection on and off; two client id orders. Every client and a fresh read agree, and the value is on the word only or nowhere, never on the typed text or another block. Plus repeated undo and redo of the removal (the live format items stay bounded) and #9620's two client id orders. - `UndoSpreadFuzz.test.ts`: two bound editors synced after every op, random comment adds, removals, formats, typing, deletes and undos; a comment id on two blocks fails (60 seeds by default, `US_SEEDS` for more). - Mutation checks: without the noted end, 20 cells fail; without the end before a live change end where nothing shows yet, #9620's 2 fail. ### Before main (4421e5bd): ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts Tests 22 failed | 125 passed (147) × comment, removed from the word, its author undoes it first (gc on, ids 1,2,3) × a comment its author undid does not cover the rest of the document (ids 1,2) $ US_SEEDS=3000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts Tests 336 failed | 2664 passed (3000) ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts Tests 147 passed (147) $ US_SEEDS=3000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts Tests 3000 passed (3000) ``` Unit tests of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list and lexical-table: 22,273 passed, 190 expected fail, 0 failed. `pnpm run ci-check` passes. Against main with the same seeds: a three-client view property fuzz (5,000 seeds; every client's view after every round, a sentinel line no one edits): 4,993 identical, 7 differ, and in each of the 7 main has a format or comment on text this branch leaves without it: 5 are main's value running onto the untouched last line, and the 2 others, replayed step by step, are main's value spreading onto text that did not have it before the undo (this branch shows the text as it was). The run-sequence fuzz (1,000 seeds) fails the same 6 seeds as main (#9599), and the comment-run fuzz (500 seeds) none on either. --- .changeset/y-undo-restored-run-ends.md | 7 + packages/lexical-y/DECISIONS.md | 5 + .../src/__tests__/unit/UndoSpreadFuzz.test.ts | 251 +++++++++++ .../regressions/UndoRestoredRunEnds.test.ts | 415 ++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 79 +++- packages/lexical-y/src/internal/Cursor.ts | 2 +- 6 files changed, 750 insertions(+), 9 deletions(-) create mode 100644 .changeset/y-undo-restored-run-ends.md create mode 100644 packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts create mode 100644 packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts diff --git a/.changeset/y-undo-restored-run-ends.md b/.changeset/y-undo-restored-run-ends.md new file mode 100644 index 00000000000..916a920548f --- /dev/null +++ b/.changeset/y-undo-restored-run-ends.md @@ -0,0 +1,7 @@ +--- +'@lexical/y': patch +--- + +[fix] An undo that brings back a comment, format, link, style or NodeState value ends it where it ended before, also when the change that wrote that end was undone before it or is undone at the same time + +Verify: undo a collaborator's removal of a comment (or a format) after its author undid it, or while the author undoes it: the value comes back on its own text only, or not at all, instead of covering everything after it. Each such undo writes one format item of its own. diff --git a/packages/lexical-y/DECISIONS.md b/packages/lexical-y/DECISIONS.md index 5393d6811fd..e8240fb123c 100644 --- a/packages/lexical-y/DECISIONS.md +++ b/packages/lexical-y/DECISIONS.md @@ -58,6 +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. 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] | | 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] | +| D83 | **An undo that brings back a value brings back its end.** Where an undo or redo restores a format item that opens a value (a format, link, style, NodeState value or comment id), it ends the run with a null of its own where the run ended when the item was deleted (the end is noted at the delete, as Yjs may collect the deleted end's content), and before another change's live end of the run even where no content shows between them yet. A value an undo restores no longer runs past the place it ended at the delete when the change that wrote that end is undone, before or at the same time. | The restored item relied on another change's end, and the undo of that change (sequential: [#9623]; concurrent: [#9620]) left the value open over everything after it. Written inside the local undo, never on a remote change (D20). Cost: one null per restored run that another change's end closed, also where nothing shows yet. A null by which a change made text inside the run plain (a hole its client restores the value after) is not taken for the end. The undo of a write whose end a live run still relies on is [#9624] (D26's sequential case), and runs a collaborator's write relies on while their writer undoes them at the same time stay open ([#9599], [#9622]). | `DECIDED` | 10-30 | [#9623], [#9620] | | D27 | **Moves are a delete and a re-insert** at launch; the binding rewrites the cheaper side. Real moves within a container ([#9449], records after Yjs's unmerged `ContentMove` design) are paused and land last, backward compatible: a new layout version that older clients open read only. Move records leave tombstones, as every Yjs binding does. Moves across containers stay copies. | Every Yjs binding turns moves into delete plus insert; Yjs dropped its own move for complexity. Reliability of the rest came first. | `DECIDED` | 10-25 | [#9449], [#9455], [y.md: Limits][limits] | | D28 | **Text identity is fixed at the source.** While a binding is attached, core records text edits (`spliceText`, insert and remove, split and merge), and text entity plugins record a grow as a move and a remake as a retype. Text only `setTextContent` or the DOM changed is diffed. The binding adds no per-case heuristics. | No binding can recover intent an API dropped; special-case rules were fragile and regressed neighbouring cases. | `DECIDED` | 10-27 | [#9483], [y.md: Edits made by code][y-code] | | D29 | **Remade text keeps its common start and end by character**, whatever the node boundaries (y-prosemirror's `updateYText` rule), and never brings back a collaborator's delete. It replaces the word matching of [#9565]. | It is the industry-standard behaviour, and word matching glued words together and merged numbers oddly. | `DECIDED` | 10-30 | [#9565] | @@ -198,3 +199,7 @@ issues and docs that carry it. [#9608]: https://github.com/meta/lexical/issues/9608 [#9609]: https://github.com/meta/lexical/issues/9609 [#9610]: https://github.com/meta/lexical/issues/9610 +[#9620]: https://github.com/meta/lexical/issues/9620 +[#9622]: https://github.com/meta/lexical/issues/9622 +[#9623]: https://github.com/meta/lexical/issues/9623 +[#9624]: https://github.com/meta/lexical/issues/9624 diff --git a/packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts b/packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts new file mode 100644 index 00000000000..6b8465656de --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts @@ -0,0 +1,251 @@ +/** + * 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 + +// Fuzz: does an undo spread a comment onto other blocks? Two bound editors, +// synced after every op, run random comment adds (each inside one text +// node), comment removals ($removeMarkID over the whole document), formats, +// plain typing, deletes and undos. A comment id on two blocks is a spread +// (#9623: "X adds, Y removes, X undoes, Y undoes" put it on everything +// after it). Env: US_SEEDS (default 60), US_START, US_OPS (default 30), +// US_UNDO (default 0.15). + +import {buildEditorFromExtensions} from '@lexical/extension'; +import { + $addMarkID, + $getNodeMarkIDs, + $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 LexicalEditor, + type LexicalNode, + type TextNode, + UNDO_COMMAND, +} from 'lexical'; +import {expect, test} from 'vitest'; + +import {createYDoc, YCollabExtension} from '../..'; +import {seedYState} from '../../headless'; + +const SEEDS = Number(process.env.US_SEEDS ?? 60); +const START = Number(process.env.US_START ?? 1); +const OPS = Number(process.env.US_OPS ?? 30); +const UNDO = Number(process.env.US_UNDO ?? 0.15); +const BLOCKS = 6; +const CONFIG = { + extensions: [RichTextExtension, MarkStateExtension], + schemaVersion: 1, +}; + +function rng(seed: number) { + let s = seed >>> 0 || 1; + return () => { + s ^= s << 13; + s ^= s >>> 17; + s ^= s << 5; + return (s >>> 0) / 4294967296; + }; +} + +type Op = {e: number} & ( + | {k: 'add'; t: number; a: number; b: number; id: string} + | {k: 'remove'; id: string} + | {k: 'format'; t: number; a: number; b: number; f: 'bold' | 'italic'} + | {k: 'type'; t: number; a: number; s: string} + | {k: 'del'; t: number; a: number; b: number} + | {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 $select(t: TextNode, a: number, b: number) { + const s = $createRangeSelection(); + const n = t.getTextContentSize(); + s.anchor.set(t.getKey(), Math.min(a, n), 'text'); + s.focus.set(t.getKey(), Math.min(b, n), 'text'); + $setSelection(s); + return s; +} + +function $apply(op: Op) { + if (op.k === 'undo') { + return; + } + if (op.k === 'remove') { + $removeMarkID(op.id); + return; + } + const ts = $texts(); + const t = ts[op.t % Math.max(1, ts.length)]; + if (t === undefined) { + return; + } + const size = t.getTextContentSize(); + if (op.k === 'add') { + if (Math.min(op.b, size) > Math.min(op.a, size)) { + $addMarkID(op.id, $select(t, op.a, op.b)); + } + } else if (op.k === 'format') { + $select(t, op.a, op.b).formatText(op.f); + } else if (op.k === 'type') { + const s = $select(t, op.a, op.a); + s.format = 0; + s.insertText(op.s); + } else if (Math.min(op.b, size) > Math.min(op.a, size)) { + $select(t, op.a, op.b).removeText(); + } + $setSelection(null); +} + +/** The comment ids that are on more than one block. */ +function spread(editor: LexicalEditor): string[] { + return editor.read(() => { + const blocks = new Map>(); + for (const t of $texts()) { + const block = t.getParentOrThrow().getKey(); + for (const id of $getNodeMarkIDs(MarkStateExtension, t)) { + const set = blocks.get(id) ?? new Set(); + set.add(block); + blocks.set(id, set); + } + } + return [...blocks].filter(([, s]) => s.size > 1).map(([id]) => id); + }); +} + +function gen(seed: number): Op[] { + const r = rng(seed * 2654435761); + const ops: Op[] = []; + const live: string[] = []; + let n = 0; + const pos = () => Math.floor(r() * 30); + for (let i = 0; i < OPS; i++) { + const e = Math.floor(r() * 2); + const v = r() * (0.88 + UNDO); + const t = Math.floor(r() * BLOCKS * 3); + if (v < 0.25) { + const a = pos(); + const id = `c${++n}`; + live.push(id); + ops.push({a, b: a + 1 + Math.floor(r() * 12), e, id, k: 'add', t}); + } else if (v < 0.37 && live.length > 0) { + const [id] = live.splice(Math.floor(r() * live.length), 1); + ops.push({e, id, k: 'remove'}); + } else if (v < 0.5) { + const a = pos(); + const f = r() < 0.5 ? 'bold' : 'italic'; + ops.push({a, b: a + 1 + Math.floor(r() * 8), e, f, k: 'format', t}); + } else if (v < 0.68) { + const s = ['x', ' new ', 'yy'][Math.floor(r() * 3)]; + ops.push({a: pos(), e, k: 'type', s, t}); + } else if (v < 0.88) { + const a = pos(); + ops.push({a, b: a + 1 + Math.floor(r() * 6), e, k: 'del', t}); + } else { + ops.push({e, k: 'undo'}); + } + } + return ops; +} + +const WORDS = ['alpha', 'beta', 'gamma', 'delta', 'note', 'run']; + +test.each(Array.from({length: SEEDS}, (_, i) => START + i))( + 'an undo never spreads a comment onto another block (seed %i)', + seed => { + const seeded = seedYState(CONFIG, () => { + const root = $getRoot().clear(); + for (let i = 0; i < BLOCKS; i++) { + root.append( + $createParagraphNode().append( + $createTextNode(`${WORDS[i]} one two three four five ${i}`), + ), + ); + } + }); + const peers = ['A', 'B'].map(name => { + const doc = createYDoc(seeded); + const editor = buildEditorFromExtensions( + defineExtension({ + dependencies: [ + RichTextExtension, + MarkStateExtension, + configExtension(YCollabExtension, { + doc, + schemaVersion: 1, + synced: true, + }), + ], + name: `undo-spread-fuzz/${name}`, + onError: error => { + throw error; + }, + }), + ); + return {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)), + ); + }; + const ops = gen(seed); + try { + ops.forEach((op, i) => { + const {editor} = peers[op.e]; + if (op.k === 'undo') { + editor.dispatchCommand(UNDO_COMMAND, undefined); + } else { + editor.update(() => $apply(op), {discrete: true}); + } + sync(); + for (const peer of peers) { + expect( + spread(peer.editor), + `after op ${i}: ${JSON.stringify(ops.slice(0, i + 1))}`, + ).toEqual([]); + } + }); + } finally { + for (const peer of peers) { + peer.editor.dispose(); + } + } + }, +); diff --git a/packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts new file mode 100644 index 00000000000..ad1276dacc6 --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts @@ -0,0 +1,415 @@ +/** + * 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 + +import {buildEditorFromExtensions} from '@lexical/extension'; +import { + $addMarkID, + $getNodeMarkIDs, + $removeMarkID, + MarkStateExtension, +} from '@lexical/mark'; +import {RichTextExtension} from '@lexical/rich-text'; +import {$patchStyleText} from '@lexical/selection'; +import * as Y from '@y/y'; +import { + $createParagraphNode, + $createRangeSelection, + $createTextNode, + $getRoot, + $getState, + $isElementNode, + $isTextNode, + $setSelection, + $setState, + configExtension, + defineExtension, + type ElementNode, + type LexicalEditor, + type LexicalNode, + REDO_COMMAND, + type TextNode, + UNDO_COMMAND, +} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +import {createYDoc, YCollabExtension, type YDoc} from '../../..'; +import {readYState, seedYState} from '../../../headless'; +import {$block, $selectIn, expectConverged, setup} from '../../utils/harness'; +import {markState, prepareMarkState as prepare} from '../../utils/sizeOps'; + +// #9623, #9620: an undo that brings back an item opening a value (a comment, +// a format, a style, a NodeState value) whose run's end is gone, because +// another change's undo deleted it before (#9623) or at the same time +// (#9620), left the value open: it covered everything after it. The undo +// now ends the run it brings back where that run ended when the item was +// deleted, with a null of its own (see Binding#closeRestoredRuns and +// #recordRunEnds). +// +// Input space: the value is a comment id (MarkState), bold, a color style or +// a NodeState value; a collaborator removes it from the whole word, from part +// of it, or (comments) from the whole document; the value's author undoes +// its change before the removal is undone, or not; another client types +// plain text right after the word, or not; then a redo and another undo; +// garbage collection on and off; two client id orders. + +const PARAS = ['zero zero', 'one one', 'two two two', 'three three', 'four']; +const WORD: [block: number, start: number, end: number] = [2, 0, 5]; +const NODE_STATE = markState('n'); + +type Value = 'comment' | 'bold' | 'color' | 'NodeState'; +type Removal = 'the word' | 'part of the word' | 'the whole document'; + +function $wordSelection(start = WORD[1], end = WORD[2]) { + const block = $getRoot().getChildAtIndex(WORD[0]) as ElementNode; + let offset = 0; + const selection = $createRangeSelection(); + for (const node of block.getAllTextNodes()) { + const size = node.getTextContentSize(); + if (start >= offset && start <= offset + size) { + selection.anchor.set(node.getKey(), start - offset, 'text'); + break; + } + offset += size; + } + offset = 0; + for (const node of block.getAllTextNodes()) { + const size = node.getTextContentSize(); + if (end >= offset && end <= offset + size) { + selection.focus.set(node.getKey(), end - offset, 'text'); + break; + } + offset += size; + } + $setSelection(selection); + return selection; +} + +function $give(value: Value, on: boolean, start?: number, end?: number) { + const selection = $wordSelection(start, end); + if (value === 'comment') { + if (on) { + $addMarkID('c2', selection); + } else { + $removeMarkID('c2', selection); + } + } else if (value === 'bold') { + // 1 is bold's format flag: bold on, or off, whatever the text has. + selection.formatText('bold', on ? 1 : 0); + } else if (value === 'color') { + $patchStyleText(selection, {color: on ? 'red' : null}); + } else { + for (const node of selection.extract()) { + if ($isTextNode(node)) { + $setState(node, NODE_STATE, on); + } + } + } + $setSelection(null); +} + +function $has(value: Value, node: TextNode): boolean { + switch (value) { + case 'comment': + return $getNodeMarkIDs(MarkStateExtension, node).includes('c2'); + case 'bold': + return node.hasFormat('bold'); + case 'color': + return node.getStyle().includes('red'); + case 'NodeState': + return $getState(node, NODE_STATE); + } +} + +/** Each block's text with every character that has the value as `^`. */ +function $marked(value: Value): string[] { + return $getRoot() + .getChildren() + .map(block => + $isElementNode(block) + ? block + .getAllTextNodes() + .map(node => + $has(value, node) + ? '^'.repeat(node.getTextContentSize()) + : node.getTextContent(), + ) + .join('') + : '?', + ); +} + +const CONFIG = { + extensions: [RichTextExtension, MarkStateExtension], + schemaVersion: 1, +}; + +function clients(gc: boolean, ids: number[]) { + const seeded = seedYState(CONFIG, () => { + $getRoot() + .clear() + .append( + ...PARAS.map(text => + $createParagraphNode().append($createTextNode(text)), + ), + ); + }); + const peers = ids.map(id => { + const doc = createYDoc(seeded, {gc}); + doc.clientID = id; + const editor = buildEditorFromExtensions( + defineExtension({ + dependencies: [ + RichTextExtension, + MarkStateExtension, + configExtension(YCollabExtension, { + captureTimeout: 0, + doc, + schemaVersion: 1, + synced: true, + }), + ], + name: `undo-restored-run-ends/${id}`, + onError: error => { + throw error; + }, + }), + ); + return {doc, editor}; + }); + const sync = () => { + for (const from of peers) { + for (const to of peers) { + if (from !== to) { + Y.applyUpdate( + to.doc, + Y.encodeStateAsUpdate(from.doc, Y.encodeStateVector(to.doc)), + ); + } + } + } + }; + return {peers, sync}; +} + +function update(editor: LexicalEditor, fn: () => void) { + editor.update(fn, {discrete: true}); +} + +function liveFormatItems(doc: YDoc): number { + let n = 0; + const root = doc.get('lexical-y') as unknown as {_start: Y.Item | null}; + for (let it = root._start; it !== null; it = it.right) { + if (!it.deleted && it.content instanceof Y.ContentFormat) { + n++; + } + } + return n; +} + +/** + * Every client and a fresh read of the document agree, and the value is on + * nothing but what `allowed` gives for the word's block (a string of the + * word's block with `^` where the value may be); the other blocks have none. + */ +function check( + value: Value, + t: ReturnType, + allowed: (marked: string) => boolean, +) { + const views = t.peers.map(p => p.editor.read(() => $marked(value))); + const fresh = readYState(Y.encodeStateAsUpdate(t.peers[0].doc), CONFIG, () => + $marked(value), + ); + for (const view of views) { + expect(view).toEqual(views[0]); + } + expect(fresh).toEqual(views[0]); + views[0].forEach((text, i) => { + if (i !== WORD[0]) { + expect(text).not.toContain('^'); + } + }); + expect(allowed(views[0][WORD[0]])).toBe(true); +} + +/** The value on the word (or the part `[a, b)` of it) only, or nowhere. */ +const onWordOrNowhere = (marked: string, a = WORD[1], b = WORD[2]) => { + const text = PARAS[WORD[0]]; + const typed = marked.length - text.length; + const plain = marked.replace(/\^/g, ''); + if (!marked.includes('^')) { + return true; + } + // Text typed after the word ("ZZ") is never marked. + const first = marked.indexOf('^'); + const last = marked.lastIndexOf('^') + 1; + return ( + typed >= 0 && + first >= a && + last <= b && + plain.length <= marked.length && + marked.slice(b).replace(/\^/g, '') === marked.slice(b) + ); +}; + +const VALUES: Value[] = ['comment', 'bold', 'color', 'NodeState']; +const REMOVALS: Removal[] = [ + 'the word', + 'part of the word', + 'the whole document', +]; + +describe('an undo that brings back a value ends its run where it ended (#9623)', () => { + for (const gc of [false, true]) { + for (const ids of [ + [1, 2, 3], + [3, 2, 1], + ]) { + for (const value of VALUES) { + for (const removal of REMOVALS) { + if (removal === 'the whole document' && value !== 'comment') { + continue; + } + for (const authorUndoes of [true, false]) { + for (const typedAfter of [false, true]) { + test(`${value}, removed from ${removal}, ${authorUndoes ? 'its author undoes it first' : 'kept by its author'}${typedAfter ? ', text typed after the word' : ''} (gc ${gc ? 'on' : 'off'}, ids ${ids})`, () => { + const t = clients(gc, ids); + const [a, b, c] = t.peers; + update(b.editor, () => $give(value, true)); + t.sync(); + if (typedAfter) { + update(c.editor, () => { + const selection = $wordSelection(WORD[2], WORD[2]); + selection.format = 0; + selection.style = ''; + selection.insertNodes([$createTextNode('ZZ')]); + $setSelection(null); + }); + t.sync(); + } + update(a.editor, () => { + if (removal === 'the whole document') { + $removeMarkID('c2'); + } else if (removal === 'the word') { + $give(value, false); + } else { + $give(value, false, WORD[1], WORD[1] + 3); + } + }); + t.sync(); + if (authorUndoes) { + b.editor.dispatchCommand(UNDO_COMMAND, undefined); + t.sync(); + } + a.editor.dispatchCommand(UNDO_COMMAND, undefined); + t.sync(); + check(value, t, marked => onWordOrNowhere(marked)); + a.editor.dispatchCommand(REDO_COMMAND, undefined); + t.sync(); + check(value, t, marked => onWordOrNowhere(marked)); + a.editor.dispatchCommand(UNDO_COMMAND, undefined); + t.sync(); + check(value, t, marked => onWordOrNowhere(marked)); + for (const p of t.peers) { + p.editor.dispose(); + } + }); + } + } + } + } + } + } + + test('repeated undo and redo of the removal keep the items bounded', () => { + const t = clients(false, [1, 2, 3]); + const [a, b] = t.peers; + update(b.editor, () => $give('comment', true)); + t.sync(); + update(a.editor, () => $give('comment', false)); + t.sync(); + b.editor.dispatchCommand(UNDO_COMMAND, undefined); + t.sync(); + const counts: number[] = []; + for (let i = 1; i <= 40; i++) { + a.editor.dispatchCommand(UNDO_COMMAND, undefined); + t.sync(); + a.editor.dispatchCommand(REDO_COMMAND, undefined); + t.sync(); + if (i % 10 === 0) { + counts.push(liveFormatItems(a.doc)); + } + } + check('comment', t, marked => onWordOrNowhere(marked)); + // As many live format items after 40 cycles as after 10. + expect(counts[3]).toBe(counts[0]); + for (const p of t.peers) { + p.editor.dispose(); + } + }); +}); + +// #9620: the end goes at the same time as the undo brings the value back. +describe('an undo that brings back a value while its end goes at the same time (#9620)', () => { + function show(peer: {read: (fn: () => T) => T}): string { + return peer.read(() => + $getRoot() + .getChildren() + .map((_, i) => + ($block([i]).getAllTextNodes() as TextNode[]) + .map(n => { + const ids = $getNodeMarkIDs(n as LexicalNode); + return ( + JSON.stringify(n.getTextContent()) + + (ids.length > 0 ? `[${ids.join(',')}]` : '') + ); + }) + .join(''), + ) + .join(' / '), + ); + } + for (const ids of [ + [1, 2], + [2, 1], + ]) { + test(`a comment its author undid does not cover the rest of the document (ids ${ids})`, async () => { + const t = await setup( + () => + $getRoot().append( + $createParagraphNode().append($createTextNode('abcd Kilo bb cc')), + $createParagraphNode().append($createTextNode('tail line')), + ), + {ids, prepare}, + ); + t.a.update(() => void $addMarkID('c1', $selectIn([0], 5, 9))); + t.b.update(() => void $addMarkID('c2', $selectIn([0], 0, 3))); + t.b.update(() => { + const [, tail] = ( + $block([0]).getAllTextNodes() as TextNode[] + )[0].splitText(2); + tail.insertBefore($createTextNode('XY')); + }); + await t.sync(); + expect(show(t.a)).toBe( + '"ab"[c2]"XY""c"[c2]"d ""Kilo"[c1]" bb cc" / "tail line"', + ); + t.offline(); + t.a.update(() => void $selectIn([0], 2, 5).removeText()); + t.a.update(() => void $selectIn([0], 2, 7).removeText()); + t.a.undo(); + t.b.undo(); + t.b.undo(); + await t.sync(); + await expectConverged(t.peers); + expect(show(t.a)).toBe('"abd ""Kilo"[c1]" bb cc" / "tail line"'); + }); + } +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 671394ceee2..b496c4d8078 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -24,6 +24,7 @@ import { encodeStateAsUpdate, encodeStateVector, findIndexSS, + type ID, Item, iterateStructsByIdSet, Node as YNode, @@ -86,6 +87,7 @@ import { import { changeEnd, equalFormatValue, + isChangeEnd, isFormatCloser, markFormatCloser, } from './Cursor'; @@ -806,6 +808,12 @@ export class YBinding { * collaborator's change is, and not part of the undo stack. */ private readonly runEndOrigin = {binding: this, runEnd: true}; + /** + * Of each format item that opens a value and that a change of this + * client's (a write, an undo or a redo) deleted, the null that ended its + * run right after that change, where a null did (see recordRunEnds). + */ + private readonly runEndAtDelete = new WeakMap(); /** Stored nodes the running undo/redo restores (see isKeptByUndo). */ private restoring: Set | null = null; /** Stored nodes the running undo/redo deletes (it inserted them). */ @@ -1094,6 +1102,13 @@ export class YBinding { trackedOrigins: new Set([this]), }); this._undoManager = um; + const recordEnds = (tr: Transaction) => { + if (tr.origin === this || tr.origin === um) { + this.recordRunEnds(tr); + } + }; + this._doc.on('afterTransaction', recordEnds); + this.disposers.push(() => this._doc.off('afterTransaction', recordEnds)); // Each step keeps the selection from before it, which undoing it // restores (as @lexical/history does): a local edit's from before the // edit, and the step an undo or redo pushes to the other stack the @@ -5231,12 +5246,51 @@ export class YBinding { }, this.scanOrigin); } + /** + * Note, for each format item opening a value that `tr` (a change of this + * client's) deleted, the null that ended its run then: the first item of + * its key after it, live or deleted by `tr`, with another value, where + * that is a null (items of its own value continue the run). An undo that + * brings the item back ends its run there (see closeRestoredRuns), also + * when that null is gone by then: deleted by the undo of its own change + * (a comment added, removed by a collaborator, its add undone and then + * the removal undone put the comment on everything after it), and + * possibly collected (its place stays, as a deleted item). + */ + private recordRunEnds(tr: Transaction): void { + iterateStructsByIdSet(tr, tr.deleteSet, s => { + if ( + !(s instanceof Item) || + !(s.content instanceof ContentFormat) || + s.content.value === null + ) { + return; + } + const {key, value} = s.content; + for (let it = s.right; it !== null; it = it.right) { + if ( + it.content instanceof ContentFormat && + it.content.key === key && + (!it.deleted || tr.deleteSet.hasId(it.id)) && + !equalFormatValue(it.content.value, value) + ) { + if (it.content.value === null) { + this.runEndAtDelete.set(s, it.id); + } + return; + } + } + }); + } + /** * End each run that a format item the undo or redo of `step` brought back * opens with an item of this client's own, where the item that ended it is * another client's: right before a live null of another client's that ends - * it, or right after the end the original's own change gave its run (see - * changeEnd), where that is gone and nothing ends the run before it. An + * it, or right after the null that ended the run when this client's + * change deleted the original (see recordRunEnds; else the end the + * original's own change gave its run, see changeEnd), where that is gone + * and nothing ends the run before it. An * undo brings back the items its change deleted, and the closer that ended * such a run was not among them: a collaborator's concurrent undo of the * change that wrote it removed it, and the value ran on to the end of the @@ -5281,13 +5335,22 @@ export class YBinding { // The end of the run the original opened in its own write (see // changeEnd): where that is gone (the undo of that write), the // copy's run ends there. - const gone = opensRun(copy) ? changeEnd(doc, original) : null; + const recorded = this.runEndAtDelete.get(original); + const gone = opensRun(copy) + ? (changeEnd(doc, original) ?? + (recorded === undefined ? null : store.getItem(recorded))) + : null; + // A gone end is ended where it was, and a live null that ends a + // run its client opened in the same write (see isChangeEnd) also + // where no content shows between the copy and it yet: that end can + // go at the same time as this undo (a collaborator's undo of the + // change that wrote it), and the run then took everything after. A + // null by which a change made text inside the run plain (a hole its + // client restores the value after) is no such end. let shown = false; 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, it.right, key]); break; } if (it.deleted) { @@ -5296,9 +5359,9 @@ export class YBinding { if (it.content instanceof ContentFormat) { if (it.content.key === key) { if ( - shown && it.id.client !== client && - it.content.value === null + it.content.value === null && + (shown || isChangeEnd(doc, it)) ) { ends.push([it.left, it, key]); } diff --git a/packages/lexical-y/src/internal/Cursor.ts b/packages/lexical-y/src/internal/Cursor.ts index cdc638f1cfc..f8a180d5ea7 100644 --- a/packages/lexical-y/src/internal/Cursor.ts +++ b/packages/lexical-y/src/internal/Cursor.ts @@ -197,7 +197,7 @@ function lastOwnItemBefore(doc: Doc, item: Item, key: string): Item | null { * same write (see changeEnd): the item of its key its client made last * before it opens a run that ends at `item`. */ -function isChangeEnd(doc: Doc, item: Item): boolean { +export function isChangeEnd(doc: Doc, item: Item): boolean { const opener = lastOwnItemBefore(doc, item, keyOf(item)); return ( opener !== null &&