Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/y-undo-restored-run-ends.md
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 5 additions & 0 deletions packages/lexical-y/DECISIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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] |
Expand Down Expand Up @@ -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
251 changes: 251 additions & 0 deletions packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts
Original file line number Diff line number Diff line change
@@ -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<string, Set<string>>();
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();
}
}
},
);
Loading
Loading