From 541363febdd8ede20cce8d1d324cd6dd57430bb0 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 19:44:58 -0400 Subject: [PATCH 01/12] [lexical-website][lexical-y] Bug Fix: Keep the fields an older client's classes do not declare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During a rollout, clients of two app versions edit the same room. A client whose class lacks a field that a newer version of the class added deleted that field for every client as soon as it wrote the node: a block container's or decorator's field when it changed the node, a list's field when it changed the list (group fields, stored on every item marker and in the outer levels of nested lists), a link's field when it called `setURL` or typed inside the link, and a TextNode subclass's field when it applied bold or typed in it. Only plain line attributes survived. A server agent with the older config (`editYState`) did the same. Each write path decided on its own which stored names it could delete. This replaces those checks with one rule, in `Schema.ts`: a client writes and deletes only the stored names it owns, and keeps every other name as stored. It owns: - the binding's own keys, and NodeState (Lexical keeps the NodeState keys it does not know, so a key a node no longer holds was removed); - the text formats its Lexical knows; - the fields its classes declare in `$config` for the location's type. Where the write changes the type, or leaves none (a line that is no group item), every field: no node there takes another type's fields. A class that writes its own `exportJSON` owns every field of its type, since which ones it writes cannot be known. The same predicates (`ownsNodeAttr`, `ownsMarkerAttr`, `ownsFormat`, `ownsMember`) serve the node, marker, text format and wrapper writes, the self-check, and the reader, which reads only what it owns, so an unknown name never reads as a change. A new item of a group takes the group fields it does not own from the item next to it; inserted text keeps the formats it does not own of the text it continues; a link's value keeps the members it does not own. A field a client knows and clears is still deleted. S7-11: a link (or another inline element stored as a format) of a type the client does not register was dropped from what it showed, and the client stayed editable, so wrapping that paragraph in a container deleted the link for everyone. The reader now records the type as unknown, as it does for every other stored kind: the client opens the room read only with `UNKNOWN_NODE_TYPES_WARNING` (`reason: 'unregistered'`). This covers an editor that does not register `LinkNode` at all, and `editYState` refuses such a document with a `YHeadlessError` that carries the typed warning. Stored data stays layout 1, and sessions without version skew store the same bytes as before: over the whole `@lexical/y` unit suite plus 800 fuzz seeds (5,362 deterministic sessions, 47,402 document states), every stored document is byte-identical to `main`'s. Limits, documented in y.md: a block an older client moves is written anew, so the moved block loses the fields that client does not know (a `test.fails` case); a class that writes its own `exportJSON` owns every field of its type. New `packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`. Each S7-3 case runs one edit made by a client with the older classes and the same edit made by a client with the newer ones, and requires the same stored document; three clients (old, new, a fresh reader) converge each time. On `main` with this test file, `./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: ``` ❯ packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts (38 tests | 26 failed) ❯ S7-3: an older client keeps the fields a newer class added (24) × a block container: its format, and a field the old class knows × a block decorator: a field the old class knows × an inline decorator: a field the old class knows ✓ a line: its format (as before) ✓ a line whose type changes takes no field of the old type × a group: its key fields × a group: an item added before its first item × a group: an item added after its last item × a group holding a nested group: the outer key fields × an inline element stored as a format: its URL × an inline element stored as a format: typing inside it × a text subclass: bold over part of it × a text subclass: typing inside it × NodeState the old app does not know, on a node, a line and text × a set-valued NodeState the old app does not know × a named slot the old client's class does not use ✓ a root attribute the old client does not know ❯ a field the client knows and clears is deleted (4) × a decorator × a group × an inline element stored as a format × a text subclass ✓ a group's field left on a line that is no item is deleted, as before ✓ a class that writes its JSON by hand owns every field of its type ✓ limit: a block an older client moves keeps the fields it does not know ❯ S7-3: which stored names a client owns (3) × a node or marker attribute × a text format and a member of a value × a client's fields at a location: its class's, or every field where the type changes ❯ S7-11: content of a type a client does not register opens the room read only (9) ✓ a line ✓ a group × an inline element stored as a format × a link, by an editor without LinkNode ✓ a text subclass ✓ a block node ✓ an inline node ✓ a named slot × the warning is typed ❯ S7-3, S7-11: a server agent with older classes (2) × S7-3: its edit keeps the fields its classes do not declare × S7-11: it refuses a document with a link type it does not register Tests 26 failed | 11 passed | 1 expected fail (38) ``` For example: ``` FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-3: an older client keeps the fields a newer class added > a block container: its format, and a field the old class knows AssertionError: expected { root: { children: [ …(2) ], …(5) } } to deeply equal { root: { children: [ …(2) ], …(5) } } - Expected + Received @@ -26,11 +26,11 @@ ], "direction": null, "format": "center", "indent": 0, "label": "Renamed", - "tone": "warm", + "tone": "", "type": "test_skew_card", "version": 1, }, FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-11: content of a type a client does not register opens the room read only > an inline element stored as a format FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-11: content of a type a client does not register opens the room read only > a link, by an editor without LinkNode AssertionError: expected 'active' to be 'readonly' // Object.is equality FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-3, S7-11: a server agent with older classes > S7-3: its edit keeps the fields its classes do not declare AssertionError: expected [ 'center', '' ] to deeply equal [ 'center', 'warm' ] ``` ``` $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts Tests 37 passed | 1 expected fail (38) $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/ Tests 4711 passed | 13 expected fail | 2 skipped (4726) ``` `pnpm run ci-check` passes. Not run: browser and e2e suites (the change is in the binding's storage logic, covered by the unit suites above). --- .../lexical-y-older-clients-keep-fields.md | 7 + .../lexical-website/docs/collaboration/y.md | 20 +- packages/lexical-y/README.md | 9 + .../unit/regressions/SchemaSkew.test.ts | 1189 +++++++++++++++++ packages/lexical-y/src/internal/Read.ts | 63 +- packages/lexical-y/src/internal/Schema.ts | 301 ++++- packages/lexical-y/src/internal/SelfCheck.ts | 21 +- packages/lexical-y/src/internal/Write.ts | 347 ++++- 8 files changed, 1872 insertions(+), 85 deletions(-) create mode 100644 .changeset/lexical-y-older-clients-keep-fields.md create mode 100644 packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts diff --git a/.changeset/lexical-y-older-clients-keep-fields.md b/.changeset/lexical-y-older-clients-keep-fields.md new file mode 100644 index 00000000000..9fdaabc4c92 --- /dev/null +++ b/.changeset/lexical-y-older-clients-keep-fields.md @@ -0,0 +1,7 @@ +--- +'@lexical/y': patch +--- + +[fix] Keep the fields a newer version of a node class added when a client with the older class edits the node in `@lexical/y` + +A client writes and deletes only the fields its classes declare in `$config`, with the text formats and NodeState it holds, and keeps every other stored name. A client that does not register a link class (or `LinkNode`) a document uses opens it read only, as it does for every other node type it does not register. diff --git a/packages/lexical-website/docs/collaboration/y.md b/packages/lexical-website/docs/collaboration/y.md index 570da7b3ff7..2b385e29a85 100644 --- a/packages/lexical-website/docs/collaboration/y.md +++ b/packages/lexical-website/docs/collaboration/y.md @@ -249,7 +249,7 @@ a short note for developers: render your own text from `code` and `data`. | Constant | Reported when | `data` (`reason`: other fields) | | -- | -- | -- | | `UNSUPPORTED_LAYOUT_WARNING` | The layout is one this version cannot read. Read only. | `'newer'`: `found`; `'malformed'`: `key`, `found`; `'missing'`, `'removed'`; `'legacy-roots'`: `roots`. All: `supported` | -| `UNKNOWN_NODE_TYPES_WARNING` | The document has node types the editor cannot build. Read only. | `'unregistered'`, `'malformed-slot'`: `types` | +| `UNKNOWN_NODE_TYPES_WARNING` | The document has node types the editor cannot build: types it does not register, however they are stored (a link class it lacks too). Read only. | `'unregistered'`, `'malformed-slot'`: `types` | | `STORAGE_MISMATCH_WARNING` | A node type is stored in a way its class cannot be (one warning per type). Read only. | `'declaration-changed'`: `type`, `stored`, `declared`; `'malformed'`: `type`, `key`, `found` | | `UNSUPPORTED_SLOTS_WARNING` | Named slots where the document cannot store them. Not written; read only. | `'stored-host'`, `'root'`: `types` | | `UNSUPPORTED_CONFIG_WARNING` | `BlockIdExtension` with `repairTransforms: true` (see [Set up an editor](#set-up-an-editor)). Nothing is written; read only. | `'repair-transforms'`: `options` | @@ -874,6 +874,14 @@ not merge as a user might expect: keeps the format: the collaborator's undo brings it back. - **Large lists.** A structural edit in a list costs time proportional to the list on every client. +- **Older app versions.** A client keeps the fields, text formats and + NodeState it does not know when it edits a node (see + [Changing your own node types](#changing-the-layout-and-migrating)), but a + block it moves is written anew from what it shows, so the moved block loses + them (see Moves). A class that writes its own `exportJSON` owns every + field of its type, since which ones it writes cannot be known: an older + version of it deletes the fields a newer one added. Declare fields in + `$config` to keep them. - **The caret when a collaborator rewrites its line.** A caret stays after the text it was after. When a collaborator replaces the line holding it, deleting all the line showed and putting new content at its place in one @@ -936,9 +944,13 @@ hold one stay readable and editable by layout 1 clients. field with a default, or dropping one, works as in [Lexical JSON](/docs/serialization/#versioning--breaking-changes): a field a room does not have reads as its default, and one your class no longer has - is ignored. A change to a class's declared node structure does not apply to - rooms that already hold that type (they keep the recorded kind); reseed - them to adopt it. + is ignored. Clients of both versions can edit a room at once (during a + rollout): a client writes only the fields its classes declare in + `$config`, so an older client's edit of a node keeps the fields a newer + version added, and a field a client knows and clears is deleted (see + "Older app versions" in the limits). A change to a class's declared node + structure does not apply to rooms that already hold that type (they keep + the recorded kind); reseed them to adopt it. ### Reseeding a room diff --git a/packages/lexical-y/README.md b/packages/lexical-y/README.md index d71459c4332..a8b201911bc 100644 --- a/packages/lexical-y/README.md +++ b/packages/lexical-y/README.md @@ -174,6 +174,15 @@ of set-valued NodeState; layout 3 (planned, #9449) adds move records. `\u0000` followed by the value's JSON with sorted keys. A string that itself starts with `\u0000` is stored the same way (as the JSON of that string), so every member reads back with its type and value. +- **Names a client does not own:** a writer sets and deletes only the names + it owns, the binding's own and the fields its classes declare for the + content's type (all of them where the write changes that type), its text + formats and NodeState, and keeps every other name as stored: a field a + newer version of a class added, a text format a newer Lexical added, a key + of a later layout. Inserted text takes the names it does not own from the + text it continues (a field only from text of its own type), and a new + group item the group fields another item of its group stores. A reader + reads only the names it owns. - **Defaults are left out:** a field or NodeState value equal to the one a new instance of the class has (its `$config` default) is not stored, so a reader takes it from its own class (an `h1` heading's marker is diff --git a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts new file mode 100644 index 00000000000..b8ec8bbe856 --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts @@ -0,0 +1,1189 @@ +/** + * 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 + +// Version skew: clients of two app versions edit one room (during a +// rollout). Each case is named after its finding. +// +// S7-3: a client whose class lacks a field a newer version of the class +// declares (a "v1" class next to the "v2" one) keeps that field when it +// writes the node: it writes and deletes only the stored names it owns (see +// ownsNodeAttr and the rest in Schema.ts). Each case runs one edit made by a +// v1 client and the same edit made by a v2 client, and requires the same +// document, as a v2 reader reads it; three clients (the v1 one, the v2 one +// that wrote the content, a fresh reader) converge each time. +// +// S7-11: content of a type a client does not register opens the room read +// only for it, whatever kind stores it: a line, a group, text, a node, or an +// inline element stored as a format (a link class it lacks). + +import type {ListItemNode} from '@lexical/list'; + +import {$createLinkNode, LinkNode} from '@lexical/link'; +import {$createListItemNode, ListNode} from '@lexical/list'; +import {HeadingNode} from '@lexical/rich-text'; +import * as Y from '@y/y'; +import { + $create, + $createParagraphNode, + $createTextNode, + $getRoot, + $getSlot, + $getState, + $isElementNode, + $isTextNode, + $setSlot, + $setState, + arraySetValue, + createState, + DecoratorNode, + ElementNode, + type Klass, + type LexicalNode, + type LexicalUpdateJSON, + nodeSchema, + ParagraphNode, + type SerializedElementNode, + stringValue, + TextNode, + withField, +} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +import { + editYState, + readYState, + seedYState, + YHeadlessError, +} from '../../../headless'; +import { + ownedFields, + ownsFormat, + ownsMarkerAttr, + ownsMember, + ownsNodeAttr, +} from '../../../internal/Schema'; +import {UNKNOWN_NODE_TYPES_WARNING} from '../../../internal/Warnings'; +import { + $block, + $createBoxNode, + $p, + $selectIn, + flush, + freshReader, + NODES, + Peer, + shape, +} from '../../utils/harness'; + +// --------------------------------------------------------------------------- +// v1 and v2 of app node types: v2 adds a field to each. + +const cardV1Schema = nodeSchema()({ + label: withField(stringValue(), {field: '__label'}), +}); +class CardV1 extends ElementNode { + __label: string = ''; + $config() { + return this.config('test_skew_card', { + extends: ElementNode, + json: cardV1Schema, + }); + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} +const cardV2Schema = nodeSchema()({ + label: withField(stringValue(), {field: '__label'}), + tone: withField(stringValue(), {field: '__tone'}), +}); +class CardV2 extends ElementNode { + __label: string = ''; + __tone: string = ''; + $config() { + return this.config('test_skew_card', { + extends: ElementNode, + json: cardV2Schema, + }); + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} + +const imageV1Schema = nodeSchema()({ + src: withField(stringValue(), {field: '__src'}), +}); +class ImageV1 extends DecoratorNode { + __src: string = ''; + $config() { + return this.config('test_skew_image', { + extends: DecoratorNode, + json: imageV1Schema, + }); + } + isInline(): boolean { + return false; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): false { + return false; + } + decorate(): null { + return null; + } +} +const imageV2Schema = nodeSchema()({ + alt: withField(stringValue(), {field: '__alt'}), + src: withField(stringValue(), {field: '__src'}), +}); +class ImageV2 extends DecoratorNode { + __src: string = ''; + __alt: string = ''; + $config() { + return this.config('test_skew_image', { + extends: DecoratorNode, + json: imageV2Schema, + }); + } + isInline(): boolean { + return false; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): false { + return false; + } + decorate(): null { + return null; + } +} + +const chipV1Schema = nodeSchema()({ + name: withField(stringValue(), {field: '__name'}), +}); +class ChipV1 extends DecoratorNode { + __name: string = ''; + $config() { + return this.config('test_skew_chip', { + extends: DecoratorNode, + json: chipV1Schema, + }); + } + isInline(): boolean { + return true; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): false { + return false; + } + decorate(): null { + return null; + } +} +const chipV2Schema = nodeSchema()({ + color: withField(stringValue(), {field: '__color'}), + name: withField(stringValue(), {field: '__name'}), +}); +class ChipV2 extends DecoratorNode { + __name: string = ''; + __color: string = ''; + $config() { + return this.config('test_skew_chip', { + extends: DecoratorNode, + json: chipV2Schema, + }); + } + isInline(): boolean { + return true; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): false { + return false; + } + decorate(): null { + return null; + } +} + +class NoteV1 extends ParagraphNode { + $config() { + return this.config('test_skew_note', {extends: ParagraphNode}); + } +} +const noteV2Schema = nodeSchema()({ + tone: withField(stringValue(), {field: '__tone'}), +}); +class NoteV2 extends ParagraphNode { + __tone: string = ''; + $config() { + return this.config('test_skew_note', { + extends: ParagraphNode, + json: noteV2Schema, + }); + } +} + +class ListV1 extends ListNode { + $config() { + return this.config('test_skew_list', {extends: ListNode}); + } +} +const listV2Schema = nodeSchema()({ + tone: withField(stringValue(), {field: '__tone'}), +}); +class ListV2 extends ListNode { + __tone: string = ''; + $config() { + return this.config('test_skew_list', { + extends: ListNode, + json: listV2Schema, + }); + } +} + +class LinkV1 extends LinkNode { + $config() { + return this.config('test_skew_link', {extends: LinkNode}); + } +} +const linkV2Schema = nodeSchema()({ + tone: withField(stringValue(), {field: '__tone'}), +}); +class LinkV2 extends LinkNode { + __tone: string = ''; + $config() { + return this.config('test_skew_link', { + extends: LinkNode, + json: linkV2Schema, + }); + } +} + +class TagV1 extends TextNode { + $config() { + return this.config('test_skew_tag', {extends: TextNode}); + } +} +const tagV2Schema = nodeSchema()({ + tag: withField(stringValue(), {field: '__tag'}), +}); +class TagV2 extends TextNode { + __tag: string = ''; + $config() { + return this.config('test_skew_tag', { + extends: TextNode, + json: tagV2Schema, + }); + } +} + +// A class that writes its JSON by hand (no `$config` schema). +type SerializedLegacy = SerializedElementNode & {label?: string; tone?: string}; +class LegacyV1 extends ElementNode { + __label = ''; + static getType(): string { + return 'test_skew_legacy'; + } + static clone(node: LegacyV1): LegacyV1 { + const copy = new LegacyV1(node.__key); + copy.__label = node.__label; + return copy; + } + static importJSON(json: SerializedLegacy): LegacyV1 { + return new LegacyV1().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + const self = super.updateFromJSON(json); + self.__label = json.label ?? ''; + return self; + } + exportJSON(): SerializedLegacy { + return {...super.exportJSON(), label: this.__label}; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} +class LegacyV2 extends ElementNode { + __label = ''; + __tone = ''; + static getType(): string { + return 'test_skew_legacy'; + } + static clone(node: LegacyV2): LegacyV2 { + const copy = new LegacyV2(node.__key); + copy.__label = node.__label; + copy.__tone = node.__tone; + return copy; + } + static importJSON(json: SerializedLegacy): LegacyV2 { + return new LegacyV2().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + const self = super.updateFromJSON(json); + self.__label = json.label ?? ''; + self.__tone = json.tone ?? ''; + return self; + } + exportJSON(): SerializedLegacy { + return {...super.exportJSON(), label: this.__label, tone: this.__tone}; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} + +const V1: Klass[] = [ + ...NODES, + CardV1, + ImageV1, + ChipV1, + NoteV1, + ListV1, + LinkV1, + TagV1, + LegacyV1, +]; +const V2: Klass[] = [ + ...NODES, + CardV2, + ImageV2, + ChipV2, + NoteV2, + ListV2, + LinkV2, + TagV2, + LegacyV2, +]; + +// A NodeState only the v2 app uses, and a set-valued one (stored one entry +// per member on text and nodes stored as their own Y.Node). +const moodState = createState('test_skew_mood', {parse: stringValue()}); +const tagsState = createState('test_skew_tags', { + parse: arraySetValue(stringValue()), +}); + +// --------------------------------------------------------------------------- +// Rooms + +type Net = { + peers: Peer[]; + delivery: 'causal'; + queue: [Peer, Uint8Array][]; + pumping: boolean; +}; +const newNet = (): Net => ({ + delivery: 'causal', + peers: [], + pumping: false, + queue: [], +}); + +type Room = {author: Peer; editor: Peer}; + +/** + * A room whose content a v2 client (`author`) writes with `$init`, joined by + * a client with `editorNodes` (`editor`). + */ +async function room( + editorNodes: Klass[], + $init: () => void, +): Promise { + const net = newNet(); + const author = new Peer(2, net as never, V2); + const editor = new Peer(1, net as never, editorNodes); + author.update(() => { + $getRoot().clear(); + $init(); + }); + author.binding.initialize(); + await flush(); + return {author, editor}; +} + +/** + * The editor of a new room makes `$edit` (with the v1 nodes or the v2 nodes): + * the clients converge, each version with a fresh reader of its own, and the + * room is returned with a fresh v2 reader's document. + */ +async function edited( + editorNodes: Klass[], + $init: () => void, + $edit: (r: Room) => void, +): Promise { + const r = await room(editorNodes, $init); + expect(r.editor.binding.status).toBe('active'); + r.editor.update(() => $edit(r)); + await flush(); + for (const peer of [r.author, r.editor]) { + expect(peer.errors).toEqual([]); + expect(peer.warnings).toEqual([]); + expect(peer.binding.status).toBe('active'); + } + const freshNew = await freshReader(r.author); + const freshOld = await freshReader(r.author, editorNodes); + expect(shape(freshNew)).toEqual(shape(r.author)); + expect(shape(freshOld)).toEqual(shape(r.editor)); + expect(freshNew.warnings).toEqual([]); + expect(freshOld.warnings).toEqual([]); + return {...r, stored: shape(freshNew)}; +} + +/** + * `$edit` made by a v1 client stores what the same edit made by a v2 client + * stores. Returns the v1 client's room. + */ +async function expectSameAsV2( + $init: () => void, + $edit: (r: Room) => void, +): Promise { + const byV1 = await edited(V1, $init, $edit); + const byV2 = await edited(V2, $init, $edit); + expect(byV1.stored).toEqual(byV2.stored); + return byV1; +} + +const docJSON = (peer: Peer): string => JSON.stringify(peer.json()); + +/** The marker of the first line of `type` in the root of `doc`. */ +function markerOf(doc: Y.Doc, type: string): Y.Node { + for (let it = doc.get('lexical-y')._start; it !== null; it = it.right) { + const content = it.content; + if ( + !it.deleted && + content instanceof Y.ContentType && + content.type.name === '\u00b6' && + content.type.getAttr('t') === type + ) { + return content.type as Y.Node; + } + } + throw new Error(`no ${type} marker`); +} + +// --------------------------------------------------------------------------- +// Content + +const $card = () => { + const card = $create(CardV2).append($p('in the card')); + card.__label = 'Card'; + card.__tone = 'warm'; + return card; +}; +const $image = () => { + const image = $create(ImageV2); + image.__src = 'a.png'; + image.__alt = 'A cat'; + return image; +}; +const $chip = () => { + const chip = $create(ChipV2); + chip.__name = 'Ada'; + chip.__color = 'teal'; + return chip; +}; +const $note = (text = 'a note') => { + const note = $create(NoteV2).append($createTextNode(text)); + note.__tone = 'warm'; + return note; +}; +const $list = (...items: string[]) => { + const list = $create(ListV2); + list.__listType = 'bullet'; + list.__tag = 'ul'; + list.__tone = 'warm'; + for (const text of items) { + list.append($createListItemNode().append($createTextNode(text))); + } + return list; +}; +const $link = () => { + const link = $create(LinkV2); + link.__url = 'https://a.dev'; + link.__tone = 'warm'; + return link.append($createTextNode('link text')); +}; +const $tag = () => { + const tag = $create(TagV2).setTextContent('tagged words'); + tag.__tag = 'x1'; + return tag; +}; + +// --------------------------------------------------------------------------- + +describe('S7-3: an older client keeps the fields a newer class added', () => { + test('a block container: its format, and a field the old class knows', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($card(), $p('after')), + () => { + const card = $block([0]) as CardV1; + card.setFormat('center'); + card.getWritable().__label = 'Renamed'; + }, + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + expect(docJSON(r.author)).toContain('"label":"Renamed"'); + }); + + test('a block decorator: a field the old class knows', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($image(), $p('after')), + () => { + ($block([0]) as unknown as ImageV1).getWritable().__src = 'b.png'; + }, + ); + expect(docJSON(r.author)).toContain('"alt":"A cat"'); + expect(docJSON(r.author)).toContain('"src":"b.png"'); + }); + + test('an inline decorator: a field the old class knows', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($p('hi ').append($chip())), + () => { + const chip = $block([0]).getChildAtIndex(1) as unknown as ChipV1; + chip.getWritable().__name = 'Grace'; + }, + ); + expect(docJSON(r.author)).toContain('"color":"teal"'); + expect(docJSON(r.author)).toContain('"name":"Grace"'); + }); + + test('a line: its format (as before)', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($note(), $p('after')), + () => $block([0]).setFormat('center'), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + }); + + test('a line whose type changes takes no field of the old type', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($note(), $p('after')), + () => { + const note = $block([0]); + const heading = $create(HeadingNode); + heading.append(...note.getChildren()); + note.replace(heading); + }, + ); + expect(docJSON(r.author)).not.toContain('"tone"'); + }); + + test('a group: its key fields', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($list('one', 'two')), + () => ($block([0]) as ListNode).setStart(3), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + expect(docJSON(r.author)).toContain('"start":3'); + }); + + test('a group: an item added before its first item', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($list('one')), + () => + ($block([0]).getFirstChildOrThrow() as ListItemNode).insertBefore( + $createListItemNode().append($createTextNode('zero')), + ), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + }); + + test('a group: an item added after its last item', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($list('one')), + () => + $block([0]).append( + $createListItemNode().append($createTextNode('two')), + ), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + }); + + test('a group holding a nested group: the outer key fields', async () => { + const r = await expectSameAsV2( + () => { + const outer = $list('one'); + const inner = $list('nested'); + inner.__tone = 'cool'; + outer.append($createListItemNode().append(inner)); + $getRoot().append(outer); + }, + () => ($block([0]) as ListNode).setStart(5), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + expect(docJSON(r.author)).toContain('"tone":"cool"'); + }); + + test('an inline element stored as a format: its URL', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($p('see ').append($link())), + () => + ($block([0]).getChildAtIndex(1) as LinkNode).setURL('https://b.dev'), + ); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + expect(docJSON(r.author)).toContain('https://b.dev'); + }); + + test('an inline element stored as a format: typing inside it', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($p('see ').append($link())), + () => $selectIn([0], 7).insertText('Q'), + ); + expect(r.author.read(() => $block([0]).getChildrenSize())).toBe(2); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + }); + + test('a text subclass: bold over part of it', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($p('before ').append($tag())), + () => $selectIn([0], 7, 13).formatText('bold'), + ); + expect(docJSON(r.author)).toContain('"tag":"x1"'); + }); + + test('a text subclass: typing inside it', async () => { + const r = await expectSameAsV2( + () => $getRoot().append($p('before ').append($tag())), + () => $selectIn([0], 10).insertText('Q'), + ); + const tags = r.author.read(() => + $block([0]) + .getChildren() + .filter($isTextNode) + .map(t => `${t.getTextContent()}:${(t as TagV2).__tag ?? '-'}`), + ); + expect(tags).toEqual(['before :-', 'tagQged words:x1']); + }); + + test('NodeState the old app does not know, on a node, a line and text', async () => { + const r = await expectSameAsV2( + () => { + const card = $card(); + $setState(card, moodState, 'calm'); + const text = $createTextNode('plain words'); + $setState(text, moodState, 'happy'); + const line = $createParagraphNode().append(text); + $setState(line, moodState, 'sad'); + $getRoot().append(card, line); + }, + () => { + ($block([0]) as CardV1).setFormat('right'); + $block([1]).setFormat('center'); + $selectIn([1], 0, 5).formatText('italic'); + }, + ); + for (const mood of ['calm', 'happy', 'sad']) { + expect(docJSON(r.author)).toContain(`"test_skew_mood":"${mood}"`); + } + }); + + test('a set-valued NodeState the old app does not know', async () => { + const r = await expectSameAsV2( + () => { + const text = $createTextNode('tagged text'); + $setState(text, tagsState, ['x', 'y']); + const image = $image(); + $setState(image, tagsState, ['z']); + $getRoot().append($createParagraphNode().append(text), image); + }, + () => { + $selectIn([0], 0, 6).formatText('bold'); + ($block([1]) as unknown as ImageV1).getWritable().__src = 'b.png'; + }, + ); + const sets = r.author.read(() => [ + ...$block([0]) + .getChildren() + .map(t => $getState(t, tagsState)), + $getState($getRoot().getChildAtIndex(1)!, tagsState), + ]); + expect(sets).toEqual([['x', 'y'], ['x', 'y'], ['z']]); + }); + + test("a named slot the old client's class does not use", async () => { + const r = await expectSameAsV2( + () => { + const card = $card(); + $setSlot(card, 'caption', $p('a caption')); + $getRoot().append(card); + }, + () => ($block([0]) as CardV1).setFormat('center'), + ); + expect( + r.author.read(() => $getSlot($block([0]), 'caption')?.getTextContent()), + ).toBe('a caption'); + }); + + test('a root attribute the old client does not know', async () => { + const r = await room(V1, () => $getRoot().append($card())); + const root = r.author.doc.get('lexical-y'); + r.author.doc.transact(() => root.setAttr('zz:future', 'kept')); + await flush(); + r.editor.update(() => $block([0]).setFormat('center')); + await flush(); + expect(root.getAttr('zz:future')).toBe('kept'); + expect(r.editor.doc.get('lexical-y').getAttr('zz:future')).toBe('kept'); + }); + + describe('a field the client knows and clears is deleted', () => { + test.each([ + [ + 'a decorator', + () => $getRoot().append($image(), $p('after')), + () => { + ($block([0]) as unknown as ImageV1).getWritable().__src = 'b.png'; + }, + () => { + ($block([0]) as unknown as ImageV2).getWritable().__alt = ''; + }, + '"alt":"A cat"', + ], + [ + 'a group', + () => $getRoot().append($list('one', 'two')), + () => ($block([0]) as ListNode).setStart(3), + () => { + ($block([0]) as ListV2).getWritable().__tone = ''; + }, + '"tone":"warm"', + ], + [ + 'an inline element stored as a format', + () => $getRoot().append($p('see ').append($link())), + () => + ($block([0]).getChildAtIndex(1) as LinkNode).setURL('https://b.dev'), + () => { + ($block([0]).getChildAtIndex(1) as LinkV2).getWritable().__tone = ''; + }, + '"tone":"warm"', + ], + [ + 'a text subclass', + () => $getRoot().append($p('before ').append($tag())), + () => $selectIn([0], 7, 13).formatText('bold'), + () => { + ($block([0]).getChildAtIndex(1) as TagV2).getWritable().__tag = ''; + }, + '"tag":"x1"', + ], + ] as const)('%s', async (_name, $init, $editV1, $clear, value) => { + // After a v1 client's edit kept it. + const r = await room(V1, $init); + r.editor.update($editV1); + await flush(); + expect(docJSON(r.author)).toContain(value); + r.author.update($clear); + await flush(); + const fresh = await freshReader(r.author); + expect(docJSON(r.author)).not.toContain(value); + expect(shape(fresh)).toEqual(shape(r.author)); + expect(shape(await freshReader(r.author, V1))).toEqual(shape(r.editor)); + }); + }); + + test("a group's field left on a line that is no item is deleted, as before", async () => { + // Concurrent edits can leave one (a list item made a heading while a + // peer changes the list's type): no group's, so every client owns it. + const r = await room(V2, () => + $getRoot().append($create(HeadingNode).append($createTextNode('title'))), + ); + const marker = markerOf(r.author.doc, 'heading'); + r.author.doc.transact(() => marker.setAttr('g:listType', 'bullet')); + await flush(); + r.editor.update(() => $block([0]).setFormat('center')); + await flush(); + expect(marker.getAttr('g:listType')).toBeUndefined(); + expect(marker.getAttr('p:format')).toBe('center'); + }); + + test('a class that writes its JSON by hand owns every field of its type', async () => { + // Which fields such a class writes cannot be known, so an older version + // of it deletes the ones a newer version added (documented in y.md). + const r = await room(V1, () => { + const node = $create(LegacyV2).append($p('legacy')); + node.__label = 'L'; + node.__tone = 'warm'; + $getRoot().append(node); + }); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + r.editor.update(() => { + ($block([0]) as LegacyV1).getWritable().__label = 'M'; + }); + await flush(); + expect(docJSON(r.author)).toContain('"label":"M"'); + expect(docJSON(r.author)).not.toContain('"tone":"warm"'); + }); + + test.fails( + 'limit: a block an older client moves keeps the fields it does not know', + async () => { + // A move writes the block anew from what the moving client shows + // (see "Moves" in y.md's limits): here into a container. + const r = await room(V1, () => $getRoot().append($note(), $p('after'))); + r.editor.update(() => { + const note = $getRoot().getFirstChildOrThrow(); + const box = $createBoxNode('t'); + note.insertBefore(box); + box.append(note); + }); + await flush(); + expect(docJSON(r.author)).toContain('"tone":"warm"'); + }, + ); +}); + +describe('S7-3: which stored names a client owns', () => { + test('a node or marker attribute', () => { + const fields = new Set(['src']); + expect(ownsNodeAttr('p:src', fields)).toBe(true); + expect(ownsNodeAttr('p:alt', fields)).toBe(false); + expect(ownsNodeAttr('s:anything', fields)).toBe(true); + expect(ownsNodeAttr('t', fields)).toBe(false); + const group = new Set(['listType']); + for (const key of ['t', 'g', 'l', 'o', 'g:$', 'g:listType', 's:x']) { + expect(ownsMarkerAttr(key, fields, group)).toBe(true); + } + for (const key of ['g:tone', 'p:alt', 'zz']) { + expect(ownsMarkerAttr(key, fields, group)).toBe(false); + } + }); + + test('a text format and a member of a value', () => { + const fields = new Set(['style']); + const wrapper = (type: string) => + type === 'link' ? new Set(['url']) : null; + for (const key of ['t', 'k', 'f:bold', 'p:style', 's:x', 'w:link']) { + expect(ownsFormat(key, fields, wrapper)).toBe(true); + } + for (const key of ['f:sparkle', 'p:tag', 'w:fancy', 'zz']) { + expect(ownsFormat(key, fields, wrapper)).toBe(false); + } + const url = new Set(['url']); + for (const member of ['url', '#', '@', '$', 's:x']) { + expect(ownsMember(member, url)).toBe(true); + } + expect(ownsMember('tone', url)).toBe(false); + }); + + test("a client's fields at a location: its class's, or every field where the type changes", () => { + const old = new Peer(7, newNet() as never, V1); + const fields = ownedFields( + old.editor, + 'test_skew_card', + 'test_skew_card', + false, + ); + expect(fields.has('label')).toBe(true); + expect(fields.has('format')).toBe(true); + expect(fields.has('tone')).toBe(false); + expect( + ownedFields(old.editor, 'test_skew_card', 'paragraph', false).has('tone'), + ).toBe(true); + expect( + ownedFields(old.editor, 'not_registered', 'not_registered', false).has( + 'format', + ), + ).toBe(false); + expect( + ownedFields(old.editor, 'test_skew_legacy', 'test_skew_legacy', true).has( + 'anything', + ), + ).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- + +class UnknownNote extends ParagraphNode { + $config() { + return this.config('test_unknown_note', {extends: ParagraphNode}); + } +} +class UnknownList extends ListNode { + $config() { + return this.config('test_unknown_list', {extends: ListNode}); + } +} +class UnknownLink extends LinkNode { + $config() { + return this.config('test_unknown_link', {extends: LinkNode}); + } +} +class UnknownTag extends TextNode { + $config() { + return this.config('test_unknown_tag', {extends: TextNode}); + } +} +class UnknownPanel extends ElementNode { + $config() { + return this.config('test_unknown_panel', {extends: ElementNode}); + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} +class UnknownChip extends DecoratorNode { + $config() { + return this.config('test_unknown_chip', {extends: DecoratorNode}); + } + isInline(): boolean { + return true; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): false { + return false; + } + decorate(): null { + return null; + } +} + +describe('S7-11: content of a type a client does not register opens the room read only', () => { + const withoutLink = NODES.filter(k => k !== LinkNode); + test.each([ + [ + 'a line', + [UnknownNote], + NODES, + () => + $getRoot().append( + $create(UnknownNote).append($createTextNode('note')), + $p('after'), + ), + 'test_unknown_note', + ], + [ + 'a group', + [UnknownList], + NODES, + () => { + const list = $create(UnknownList); + list.__listType = 'bullet'; + list.__tag = 'ul'; + $getRoot().append( + list.append($createListItemNode().append($createTextNode('item'))), + $p('after'), + ); + }, + 'test_unknown_list', + ], + [ + 'an inline element stored as a format', + [UnknownLink], + NODES, + () => { + const link = $create(UnknownLink); + link.__url = 'https://a.dev'; + $getRoot().append( + $p('see ').append(link.append($createTextNode('here'))), + $p('after'), + ); + }, + 'test_unknown_link', + ], + [ + 'a link, by an editor without LinkNode', + [], + withoutLink, + () => + $getRoot().append( + $p('see ').append( + $createLinkNode('https://a.dev').append($createTextNode('here')), + ), + $p('after'), + ), + 'link', + ], + [ + 'a text subclass', + [UnknownTag], + NODES, + () => + $getRoot().append( + $p('plain ').append($create(UnknownTag).setTextContent('tag')), + $p('after'), + ), + 'test_unknown_tag', + ], + [ + 'a block node', + [UnknownPanel], + NODES, + () => + $getRoot().append( + $create(UnknownPanel).append($p('in it')), + $p('after'), + ), + 'test_unknown_panel', + ], + [ + 'an inline node', + [UnknownChip], + NODES, + () => + $getRoot().append($p('hi ').append($create(UnknownChip)), $p('after')), + 'test_unknown_chip', + ], + [ + 'a named slot', + [UnknownPanel], + NODES, + () => { + const card = $createBoxNode('t').append($p('body')); + $setSlot(card, 'aside', $create(UnknownPanel).append($p('in a slot'))); + $getRoot().append(card, $p('after')); + }, + 'test_unknown_panel', + ], + ] as const)('%s', async (_name, extra, oldNodes, $init, type) => { + const net = newNet(); + const author = new Peer(2, net as never, [...NODES, ...extra]); + const old = new Peer(1, net as never, [...oldNodes]); + author.update(() => { + $getRoot().clear(); + $init(); + }); + author.binding.initialize(); + await flush(); + expect(old.binding.status).toBe('readonly'); + expect(old.warnings).toEqual([`unregistered node types: ${type}`]); + const before = docJSON(author); + const vector = Y.encodeStateVector(old.doc); + // An edit of the old client is not written. + old.update(() => { + const last = $getRoot().getLastChildOrThrow(); + if ($isElementNode(last)) { + last.append($createTextNode(' more')); + } + }); + await flush(); + expect(Y.encodeStateVector(old.doc)).toEqual(vector); + expect(docJSON(author)).toBe(before); + }); + + test('the warning is typed', async () => { + const net = newNet(); + const author = new Peer(2, net as never, [...NODES, UnknownLink]); + const warnings: unknown[] = []; + const old = new Peer(1, net as never, NODES, editor => { + editor._onWarn = error => { + warnings.push(error); + }; + }); + author.update(() => { + $getRoot().clear(); + const link = $create(UnknownLink); + link.__url = 'https://a.dev'; + $getRoot().append($p('see ').append(link.append($createTextNode('x')))); + }); + author.binding.initialize(); + await flush(); + expect(old.binding.status).toBe('readonly'); + expect(warnings).toMatchObject([ + { + code: UNKNOWN_NODE_TYPES_WARNING, + data: {reason: 'unregistered', types: ['test_unknown_link']}, + }, + ]); + }); +}); + +// --------------------------------------------------------------------------- + +// The same rules for a server agent whose config has the older classes: the +// headless helpers write through the same binding. +describe('S7-3, S7-11: a server agent with older classes', () => { + const quiet = () => {}; + const refusal = (edit: () => void) => { + try { + edit(); + } catch (error) { + return error; + } + throw new Error('the edit was not refused'); + }; + + test('S7-3: its edit keeps the fields its classes do not declare', () => { + const seeded = seedYState({nodes: V2, onWarn: quiet}, () => + $getRoot().append($card(), $p('after')), + ); + const {state} = editYState( + seeded, + {nodes: V1, onWarn: quiet}, + () => $block([0]).setFormat('center'), + {writer: null}, + ); + expect( + readYState(state, {nodes: V2, onWarn: quiet}, () => { + const card = $block([0]) as CardV2; + return [card.getFormatType(), card.__tone]; + }), + ).toEqual(['center', 'warm']); + }); + + test('S7-11: it refuses a document with a link type it does not register', () => { + const seeded = seedYState( + {nodes: [...NODES, UnknownLink], onWarn: quiet}, + () => { + const link = $create(UnknownLink); + link.__url = 'https://a.dev'; + $getRoot().append($p('see ').append(link.append($createTextNode('x')))); + }, + ); + const error = refusal(() => + editYState( + seeded, + {nodes: NODES, onWarn: quiet}, + () => { + const box = $createBoxNode('t'); + $block([0]).insertBefore(box); + box.append($block([1])); + }, + {writer: null}, + ), + ); + expect(error).toBeInstanceOf(YHeadlessError); + expect(error).toMatchObject({ + code: 'read-only', + warnings: [ + { + code: UNKNOWN_NODE_TYPES_WARNING, + data: {reason: 'unregistered', types: ['test_unknown_link']}, + }, + ], + }); + }); +}); diff --git a/packages/lexical-y/src/internal/Read.ts b/packages/lexical-y/src/internal/Read.ts index 605144846c5..d85ad06c08b 100644 --- a/packages/lexical-y/src/internal/Read.ts +++ b/packages/lexical-y/src/internal/Read.ts @@ -56,6 +56,7 @@ import { type Attrs, BLOCK_PREFIX, decodeFormatValue, + eachOuterLevel, FIELD_PREFIX, getContainerGroupSpec, getGroupSpec, @@ -63,6 +64,7 @@ import { getWrapperSpec, GROUP_FIELD_PREFIX, GROUP_KEY, + groupItemFields, type GroupSpec, INLINE_PREFIX, isUnsafeKey, @@ -76,14 +78,21 @@ import { MARKER, nodeJSONFromAttrs, OUTER_KEY, + type OwnedFields, + ownedFields, + ownedFieldsOf, + ownsMarkerAttr, + ownsNodeAttr, SEGMENT_KEY, stableStringify, STATE_PREFIX, storedTypeOf, textJSONFromAttrs, TYPE_KEY, + withoutUnownedMembers, WRAPPER_FIELDS_KEY, WRAPPER_PREFIX, + wrapperFieldsOf, wrappersFromAttrs, type WrapperValue, } from './Schema'; @@ -244,7 +253,12 @@ function parseRange( } export class Reader { - private readonly isKnownWrapper: (type: string) => boolean; + /** + * The fields this editor owns in the value of a format wrapper (see + * wrapperFieldsOf), or null where it does not store the type as one. A + * type it does not register is an unknown type (see State#unknownTypes). + */ + private readonly wrapperFields: (type: string) => OwnedFields | null; /** Item -> unit cache for one read. */ private located = new Map(); /** Formats in effect after the content last read by $pieces. */ @@ -274,10 +288,12 @@ export class Reader { readonly settler: BlockIdSettler; constructor(readonly state: State) { this.settler = new BlockIdSettler(state); - const nodes = state.editor._nodes; - this.isKnownWrapper = type => { - const info = nodes.get(type); - return info !== undefined && getWrapperSpec(info.klass) !== null; + const editor = state.editor; + this.wrapperFields = type => { + if (!editor._nodes.has(type)) { + state.unknownTypes.add(type); + } + return wrapperFieldsOf(editor, type); }; } @@ -1080,9 +1096,16 @@ export class Reader { } const spec = ginfo === undefined ? null : getGroupSpec(ginfo.klass); if (spec !== null && spec.items.includes(info.klass)) { + // What this editor's classes read: the group fields it owns (see + // ownsMarkerAttr), as its writes give them. + const lineFields = ownedFieldsOf(info.klass, false); + const groupFields = ownedFieldsOf(spec.klass, false); const gkey: Attrs = {}; for (const k in attrs) { - if (k.startsWith(GROUP_FIELD_PREFIX)) { + if ( + k.startsWith(GROUP_FIELD_PREFIX) && + ownsMarkerAttr(k, lineFields, groupFields) + ) { gkey[k] = attrs[k]; } } @@ -1104,7 +1127,13 @@ export class Reader { Array.isArray(o) && o.length === level && o.every(v => v !== null && typeof v === 'object' && !Array.isArray(v)) - ? (o as Attrs[]) + ? eachOuterLevel( + o as Attrs[], + null, + groupFields, + groupItemFields(spec), + (value, _stored, owns) => withoutUnownedMembers(value, owns), + ) : null; return {gkey, gtype: g as string, level, outer, spec}; } @@ -1809,14 +1838,20 @@ export class Reader { y.getAttrs() as Attrs, placed, ); - // Compare in the stored encoding (compact, textFormat always kept). A - // value holding an array hole is stored as null, which the class parses - // its own way (colWidths: 0): applied, so the node shows what every - // reader shows. + // Compare in the stored encoding (compact, textFormat always kept), what + // this editor owns of it (see ownsNodeAttr). A value holding an array + // hole is stored as null, which the class parses its own way (colWidths: + // 0): applied, so the node shows what every reader shows. + const fields = ownedFields( + this.state.editor, + node.getType(), + node.getType(), + true, + ); const owned = (x: Attrs) => { const out: Attrs = {}; for (const k of Object.keys(x).sort()) { - if (k.startsWith(FIELD_PREFIX) || k.startsWith(STATE_PREFIX)) { + if (ownsNodeAttr(k, fields)) { out[k] = x[k]; } } @@ -1953,7 +1988,7 @@ export class Reader { } }); tsig = stableStringify(tattrs); - wrappers = wrappersFromAttrs(all, this.isKnownWrapper); + wrappers = wrappersFromAttrs(all, this.wrapperFields); } }; for (let it = start; it !== null && it !== stop; it = it.right) { @@ -2341,7 +2376,7 @@ export class Reader { if (reused !== undefined) { const own: Attrs = {}; $wrapperAttrs(reused, own); - const current = wrappersFromAttrs(own, () => true)[0]; + const current = wrappersFromAttrs(own, this.wrapperFields)[0]; if ( current === undefined || stableStringify(current.json) !== stableStringify(w.json) diff --git a/packages/lexical-y/src/internal/Schema.ts b/packages/lexical-y/src/internal/Schema.ts index 19e3cb80b93..cba89bacb52 100644 --- a/packages/lexical-y/src/internal/Schema.ts +++ b/packages/lexical-y/src/internal/Schema.ts @@ -22,6 +22,7 @@ import { type Klass, type LexicalEditor, type LexicalNode, + ParagraphNode, TEXT_TYPE_TO_FORMAT, type TextNode, } from 'lexical'; @@ -616,7 +617,7 @@ export function getLineSpec(klass: Klass): LineSpec | null { } let spec = lineSpecs.get(klass); if (spec === undefined) { - spec = {fields: new Set(Object.keys(getComposedSchemaFields(klass)))}; + spec = {fields: schemaFieldsOf(klass)}; lineSpecs.set(klass, spec); } return spec; @@ -1629,23 +1630,289 @@ export function textJSONFromAttrs(text: string, encoded: Attrs): Attrs { return json; } -/** Text keys this binding owns (and so may change or remove). */ -export function isOwnedTextKey( +// --------------------------------------------------------------------------- +// Ownership +// +// Invariant: a write sets and deletes only the stored names its client owns +// (ownsNodeAttr, ownsMarkerAttr, ownsFormat, and ownsMember for the members +// of a value that holds a node's fields); every other name keeps its stored +// value. Why: a document outlives app versions, and clients of several +// versions edit it at once (during a rollout): a name a client does not +// know, such as a field a newer version of a class declares, is another +// client's data, which this one can neither show nor write back. + +/** + * The fields a client owns at one stored location: those its classes for + * the location's types declare (see ownedFields). + */ +export interface OwnedFields { + has(field: string): boolean; +} + +/** No fields: a type the editor does not register. */ +const NO_FIELDS: OwnedFields = {has: () => false}; + +/** Every field: a class that writes its JSON by hand (see writesOwnJSON). */ +const EVERY_FIELD: OwnedFields = {has: () => true}; + +/** The member of a value that holds a node's fields that holds its NodeState. */ +const STATE_MEMBER = '$'; + +const schemaFieldSets = new WeakMap, ReadonlySet>(); + +/** The fields `klass` declares in its `$config` schema, flat NodeState too. */ +function schemaFieldsOf(klass: Klass): ReadonlySet { + let fields = schemaFieldSets.get(klass); + if (fields === undefined) { + fields = new Set(Object.keys(getComposedSchemaFields(klass))); + schemaFieldSets.set(klass, fields); + } + return fields; +} + +/** + * Whether nodes of `klass` write their JSON by hand: the exportJSON they use + * is not one of `lexical`'s (LexicalNode's, which ElementNode does not + * override, writes the schema; ParagraphNode's only adds schema fields), so + * it may write fields the schema does not declare. + */ +function writesOwnJSON(klass: Klass): boolean { + const exportJSON = (klass.prototype as LexicalNode).exportJSON; + return ( + exportJSON !== ElementNode.prototype.exportJSON && + exportJSON !== ParagraphNode.prototype.exportJSON + ); +} + +/** + * The fields a client owns at a stored location whose type is `stored` and + * is `written` once its write is done. Where the write changes the type, or + * leaves none (a line that is not an item of a group holds no group's + * fields), every field: no node there takes the fields stored for another + * type. Otherwise those its class of the type declares in its `$config` + * schema, and none where it does not register the type. Where the location + * holds the class's own exportJSON (`json`: a node stored as its own Y.Node, + * a text run), a class that writes its JSON by hand (see writesOwnJSON) owns + * every field: which ones it writes cannot be known. + */ +export function ownedFields( + editor: LexicalEditor, + stored: unknown, + written: unknown, + json: boolean, +): OwnedFields { + if (stored !== written || written === undefined) { + return EVERY_FIELD; + } + const info = + typeof written === 'string' ? editor._nodes.get(written) : undefined; + return info === undefined ? NO_FIELDS : ownedFieldsOf(info.klass, json); +} + +/** {@link ownedFields} where the type is `klass`'s and stays so. */ +export function ownedFieldsOf( + klass: Klass, + json: boolean, +): OwnedFields { + return json && writesOwnJSON(klass) ? EVERY_FIELD : schemaFieldsOf(klass); +} + +const itemFieldSets = new WeakMap(); + +/** + * The fields a client owns in the stored fields of an item of a group of + * `spec` that holds a nested group (an outer level's wrapper item, see + * WRAPPER_FIELDS_KEY): those one of the group's item classes declares. + */ +export function groupItemFields(spec: GroupSpec): OwnedFields { + let fields = itemFieldSets.get(spec); + if (fields === undefined) { + const sets = spec.items.map(schemaFieldsOf); + fields = {has: field => sets.some(set => set.has(field))}; + itemFieldSets.set(spec, fields); + } + return fields; +} + +/** + * The fields a client owns in the value of format wrapper `type` (see + * ownsMember), or null where it does not store `type` as a format wrapper. + */ +export function wrapperFieldsOf( + editor: LexicalEditor, + type: string, +): OwnedFields | null { + const info = editor._nodes.get(type); + return info === undefined || getWrapperSpec(info.klass) === null + ? null + : schemaFieldsOf(info.klass); +} + +/** + * Whether a client owns attribute `key` of a node stored as its own Y.Node: + * a field it owns there (`fields`), or NodeState, which the node holds + * whole (NodeState keeps the keys its client does not know, see + * $stateAttrs), so a key the node no longer holds was removed. + */ +export function ownsNodeAttr(key: string, fields: OwnedFields): boolean { + return ( + key.startsWith(STATE_PREFIX) || + (key.startsWith(FIELD_PREFIX) && fields.has(key.slice(FIELD_PREFIX.length))) + ); +} + +/** + * Whether a client owns attribute `key` of a line marker: the binding's own + * keys (type, group, level, outer levels), a field of the line it owns + * (`fields`) or of the line's group (`groupFields`), and NodeState (see + * ownsNodeAttr), the group's too. + */ +export function ownsMarkerAttr( key: string, - isKnownWrapper: (type: string) => boolean, + fields: OwnedFields, + groupFields: OwnedFields, ): boolean { - if (key.startsWith(WRAPPER_PREFIX)) { - return isKnownWrapper(key.slice(WRAPPER_PREFIX.length)); + if (key.startsWith(GROUP_FIELD_PREFIX)) { + const field = key.slice(GROUP_FIELD_PREFIX.length); + return field === STATE_MEMBER || groupFields.has(field); } return ( key === TYPE_KEY || - key === SEGMENT_KEY || - key.startsWith(TEXT_FORMAT_PREFIX) || - key.startsWith(FIELD_PREFIX) || - key.startsWith(STATE_PREFIX) + key === GROUP_KEY || + key === LEVEL_KEY || + key === OUTER_KEY || + ownsNodeAttr(key, fields) + ); +} + +/** + * Whether a client owns text format `key`: the binding's own keys (the text + * node type, a segment key), a text format its Lexical knows, a field of the + * text's class it owns (`fields`), NodeState (see ownsNodeAttr), and a + * format wrapper of a type it stores as one (`wrapperFields`, see ownsMember + * for the members of its value). + */ +export function ownsFormat( + key: string, + fields: OwnedFields, + wrapperFields: (type: string) => OwnedFields | null, +): boolean { + if (key.startsWith(WRAPPER_PREFIX)) { + return wrapperFields(key.slice(WRAPPER_PREFIX.length)) !== null; + } + if (key.startsWith(TEXT_FORMAT_PREFIX)) { + return hasOwn(TEXT_TYPE_TO_FORMAT, key.slice(TEXT_FORMAT_PREFIX.length)); + } + return key === TYPE_KEY || key === SEGMENT_KEY || ownsNodeAttr(key, fields); +} + +/** + * Whether a client owns member `member` of a value that holds a node's + * fields (a format wrapper's value, an outer level's fields or its wrapper + * item's in OUTER_KEY): the binding's own members (a wrapper's instance, a + * level's wrapper item), NodeState, and a field it owns (`fields`). + */ +export function ownsMember(member: string, fields: OwnedFields): boolean { + return ( + member === INSTANCE_KEY || + member === WRAPPER_FIELDS_KEY || + member === STATE_MEMBER || + member.startsWith(STATE_PREFIX) || + fields.has(member) ); } +const isPlainObject = (v: unknown): v is Attrs => + v !== null && typeof v === 'object' && !Array.isArray(v); + +/** + * `value`, a value that holds a node's fields as its client writes it, with + * the members of `stored` (the value stored there) that the client does not + * own (`owns`) kept as stored: `value` itself when there are none. + */ +export function withUnownedMembers( + value: Attrs, + stored: unknown, + owns: (member: string) => boolean, +): Attrs { + if (!isPlainObject(stored)) { + return value; + } + let out: Attrs | null = null; + for (const member of Object.keys(stored)) { + if (!(member in value) && !isUnsafeKey(member) && !owns(member)) { + if (out === null) { + out = {...value}; + } + out[member] = stored[member]; + } + } + return out === null ? value : out; +} + +/** + * `value` without the members its reader does not own (`owns`): what the + * reader's classes read of it. `value` itself when it has none. + */ +export function withoutUnownedMembers( + value: Attrs, + owns: (member: string) => boolean, +): Attrs { + let out: Attrs | null = null; + for (const member of Object.keys(value)) { + if (!owns(member)) { + if (out === null) { + out = {...value}; + } + delete out[member]; + } + } + return out === null ? value : out; +} + +/** + * Apply `apply` (withUnownedMembers or withoutUnownedMembers) to each level + * of an OUTER_KEY value, `stored[i]` being what level `i` is stored as: a + * level's fields are its group's (`groupFields`), and its wrapper item's + * those of the group's items (`itemFields`). `outer` itself where that + * changes nothing. + */ +export function eachOuterLevel( + outer: Attrs[], + stored: unknown, + groupFields: OwnedFields, + itemFields: OwnedFields, + apply: ( + value: Attrs, + at: unknown, + owns: (member: string) => boolean, + ) => Attrs, +): Attrs[] { + let out: Attrs[] | null = null; + outer.forEach((level, i) => { + const at = Array.isArray(stored) ? stored[i] : undefined; + let next = apply(level, at, m => ownsMember(m, groupFields)); + const item = level[WRAPPER_FIELDS_KEY]; + if (isPlainObject(item)) { + const kept = apply( + item, + isPlainObject(at) ? at[WRAPPER_FIELDS_KEY] : undefined, + m => ownsMember(m, itemFields), + ); + if (kept !== item) { + next = {...next, [WRAPPER_FIELDS_KEY]: kept}; + } + } + if (next !== level) { + if (out === null) { + out = outer.slice(); + } + out[i] = next; + } + }); + return out === null ? outer : out; +} + // --------------------------------------------------------------------------- // Wrappers @@ -1704,10 +1971,14 @@ export type WrapperValue = { readonly instance: string | null; }; -/** Wrapper values in a format map, sorted by type. */ +/** + * Wrapper values in a format map, sorted by type: of the types `fieldsOf` + * gives the fields of (those its reader stores as format wrappers, see + * wrapperFieldsOf), each with the members its reader owns (see ownsMember). + */ export function wrappersFromAttrs( attrs: Attrs, - isKnownWrapper: (type: string) => boolean, + fieldsOf: (type: string) => OwnedFields | null, ): WrapperValue[] { const result: WrapperValue[] = []; for (const key in attrs) { @@ -1716,7 +1987,8 @@ export function wrappersFromAttrs( } const type = key.slice(WRAPPER_PREFIX.length); const value = attrs[key]; - if (!isKnownWrapper(type) || value === null || typeof value !== 'object') { + const fields = fieldsOf(type); + if (fields === null || value === null || typeof value !== 'object') { continue; } const json: Attrs = {type, version: 1}; @@ -1729,6 +2001,9 @@ export function wrappersFromAttrs( instance = typeof v === 'string' ? v : null; continue; } + if (!ownsMember(f, fields)) { + continue; + } if (typeof v === 'string' && v.startsWith('\u0000')) { try { v = JSON.parse(v.slice(1)); diff --git a/packages/lexical-y/src/internal/SelfCheck.ts b/packages/lexical-y/src/internal/SelfCheck.ts index 31b2cd689f1..284d5cd0be8 100644 --- a/packages/lexical-y/src/internal/SelfCheck.ts +++ b/packages/lexical-y/src/internal/SelfCheck.ts @@ -40,9 +40,9 @@ import { getGroupSpec, getWrapperSpec, INLINE_PREFIX, - isOwnedTextKey, isSetStateConfig, MARKER, + ownsFormat, RUN_END, SEGMENT_KEY, SET_MEMBER_SEPARATOR, @@ -123,7 +123,6 @@ function hasLiveContent(y: YNode): boolean { */ export function $checkWrite(writer: Writer): string[] { const problems: string[] = []; - const isKnownWrapper = (type: string) => writer.state.editor._nodes.has(type); // A line written twice in one write (its container rewritten after it) // is checked as last written. const last = new Map(); @@ -209,6 +208,7 @@ export function $checkWrite(writer: Writer): string[] { continue; } lastKey = key; + const fields = writer.textFields(a[TYPE_KEY], b[TYPE_KEY]); for (const k of new Set([...Object.keys(a), ...Object.keys(b)])) { // A segment key under text the editor holds in a node Lexical // merges (an opener an undo restored, left open over later lines): @@ -230,13 +230,18 @@ export function $checkWrite(writer: Writer): string[] { // reader shows none, whatever is stored. continue; } + // What the write stores of it, where the editor owns it (see + // ownsFormat, Writer#formatAsStored). if ( (!wrappersOnly || k.startsWith(WRAPPER_PREFIX) || (isBreak && k.startsWith(STATE_PREFIX))) && - isOwnedTextKey(k, isKnownWrapper) && + ownsFormat(k, fields, writer.wrapperFields) && !equalFormatValue( - withoutTextFields(a[k] ?? null, derivedText), + withoutTextFields( + writer.formatAsStored(k, a[k] ?? null, b[k]), + derivedText, + ), withoutTextFields(b[k] ?? null, derivedText), ) ) { @@ -247,11 +252,17 @@ export function $checkWrite(writer: Writer): string[] { } } if (d.kind === 'line') { - const marker = writer.$marker(d); const attrs = (end.content as ContentType).type.getAttrs() as Record< string, unknown >; + const desired = writer.$marker(d); + const marker = writer.$markerAsStored( + desired, + attrs, + writer.markerOwners(desired, attrs), + null, + ); for (const k in marker) { if ( !equalValue(attrs[k], marker[k]) && diff --git a/packages/lexical-y/src/internal/Write.ts b/packages/lexical-y/src/internal/Write.ts index dbe730d7257..5f9a425d33c 100644 --- a/packages/lexical-y/src/internal/Write.ts +++ b/packages/lexical-y/src/internal/Write.ts @@ -60,18 +60,18 @@ import { $textAttrs, type Attrs, BLOCK_PREFIX, + eachOuterLevel, equalValue, FIELD_PREFIX, getContainerGroupSpec, getGroupSpec, - getLineSpec, GROUP_FIELD_PREFIX, GROUP_KEY, groupAttrsOf, + groupItemFields, type GroupSpec, INLINE_PREFIX, isMergeableText, - isOwnedTextKey, isTextFieldKey, KIND_KEY, klassOf, @@ -79,13 +79,22 @@ import { MARKER, OUTER_KEY, outerWithoutTextFields, + type OwnedFields, + ownedFields, + ownsFormat, + ownsMarkerAttr, + ownsMember, + ownsNodeAttr, RUN_END, stableStringify, STATE_PREFIX, storedTypeOf, + TEXT_FORMAT_PREFIX, TYPE_KEY, + withUnownedMembers, WRAPPER_FIELDS_KEY, WRAPPER_PREFIX, + wrapperFieldsOf, } from './Schema'; import {$writeSlots} from './Slots'; import { @@ -132,6 +141,16 @@ const storedType = (y: YNode): string => const __DEV__ = process.env.NODE_ENV !== 'production'; +/** The fields a client owns on one marker (see Writer#markerOwners). */ +export type MarkerOwners = { + /** Of the line. */ + readonly fields: OwnedFields; + /** Of its group, stored as `g:` and in each outer level. */ + readonly groupFields: OwnedFields; + /** Of its group's items, in each outer level's wrapper item. */ + readonly itemFields: OwnedFields; +}; + type Desired = { kind: 'line' | 'node' | 'run'; key: NodeKey | null; @@ -393,7 +412,13 @@ export class Writer { * (see YBinding#$normalize). */ readonly holed = new Set(); - private readonly isKnownWrapper: (type: string) => boolean; + /** + * The fields this editor owns in the value of a format wrapper, or null + * where it does not store the type as one (see wrapperFieldsOf). + */ + readonly wrapperFields: (type: string) => OwnedFields | null; + /** Per item, see $storedItemAtOrBefore. */ + private readonly groupReferences = new Map(); constructor( readonly state: State, readonly roles: Roles, @@ -401,8 +426,8 @@ export class Writer { readonly prevState: EditorState, readonly plan: WritePlan, ) { - const nodes = state.editor._nodes; - this.isKnownWrapper = type => nodes.has(type); + const editor = state.editor; + this.wrapperFields = type => wrapperFieldsOf(editor, type); } // ------------------------------------------------------------------------- @@ -3607,15 +3632,96 @@ export class Writer { return true; } - private inheritUnknown(cursor: Cursor, attrs: Attrs): Attrs { + /** + * The fields this editor owns in the formats of text whose type keys + * (TYPE_KEY, absent for `text`) are `a` and `b` (see ownsFormat): its + * classes' of those text node types. + */ + textFields(a: unknown, b: unknown): OwnedFields { + return ownedFields(this.state.editor, a ?? 'text', b ?? 'text', true); + } + + /** + * Format `key`'s value `value`, as this editor writes it, as stored where + * `stored` is: a format wrapper's value keeps the members of `stored` + * this editor does not own (see ownsMember). With `same`, only where + * `stored` is the same wrapper as this editor shows it: its owned members + * (but the text fields Lexical derives, see stripGroupText) are `value`'s. + */ + formatAsStored( + key: string, + value: unknown, + stored: unknown, + same = false, + ): unknown { + const fields = key.startsWith(WRAPPER_PREFIX) + ? this.wrapperFields(key.slice(WRAPPER_PREFIX.length)) + : null; + if ( + fields === null || + value === null || + typeof value !== 'object' || + stored === null || + typeof stored !== 'object' + ) { + return value; + } + const owns = (m: string) => ownsMember(m, fields); + if (same) { + const a = value as Attrs; + const b = stored as Attrs; + for (const m of new Set([...Object.keys(a), ...Object.keys(b)])) { + if ( + m !== 'textFormat' && + m !== 'textStyle' && + owns(m) && + !equalValue(a[m], b[m]) + ) { + return value; + } + } + } + return withUnownedMembers(value as Attrs, stored, owns); + } + + /** + * `attrs`, the formats of content inserted at `cursor`, with the formats + * there that this editor does not own (see ownsFormat): the content + * continues what is before it, which its formats must not cut. A field + * goes only into text of the same type (it is that node's: of another + * type, every field is owned), and neither a field nor a text format into + * an inline node (`wrappersOnly`), which holds wrappers only; a wrapper's + * value keeps the members it does not own of the same wrapper there (see + * formatAsStored). + */ + private inheritUnknown( + cursor: Cursor, + attrs: Attrs, + wrappersOnly = false, + ): Attrs { let result = attrs; + const fields = this.textFields( + cursor.formats.get(TYPE_KEY), + attrs[TYPE_KEY], + ); cursor.formats.forEach((v, k) => { - if (!(k in attrs) && !isOwnedTextKey(k, this.isKnownWrapper)) { - if (result === attrs) { - result = {...attrs}; + let value = v; + if (k in attrs) { + value = this.formatAsStored(k, attrs[k], v, true); + if (value === attrs[k]) { + return; } - result[k] = v; + } else if ( + ownsFormat(k, fields, this.wrapperFields) || + (wrappersOnly && + (k.startsWith(FIELD_PREFIX) || k.startsWith(TEXT_FORMAT_PREFIX))) + ) { + return; + } + if (result === attrs) { + result = {...attrs}; } + result[k] = value; }); return result; } @@ -3645,7 +3751,11 @@ export class Writer { const y = new YNode(INLINE_PREFIX + special.node.getType()); cursor.insert( new ContentType(y), - this.inheritUnknown(cursor, wrapperOnly(run ? run.attrs : {})), + this.inheritUnknown( + cursor, + wrapperOnly(run ? run.attrs : {}), + true, + ), ); this.$fillNode(y, special.node); } @@ -3685,12 +3795,18 @@ export class Writer { ); const ops: {start: number; end: number; formats: Attrs; sig: string}[] = []; // A line break carries wrapper formats and its own NodeState; a child - // node carries wrapper formats only. - const owned = (k: string, wrappersOnly: boolean, isBreak: boolean) => + // node carries wrapper formats only. Of those, what this editor owns + // (see ownsFormat). + const owned = ( + k: string, + wrappersOnly: boolean, + isBreak: boolean, + fields: OwnedFields, + ) => (!wrappersOnly || k.startsWith(WRAPPER_PREFIX) || (isBreak && k.startsWith(STATE_PREFIX))) && - isOwnedTextKey(k, this.isKnownWrapper); + ownsFormat(k, fields, this.wrapperFields); // Walk segments between run boundaries, child nodes and line breaks // (where only wrapper formats apply), not characters: the cost is in // runs, so a keystroke in a long line stays cheap. @@ -3744,19 +3860,20 @@ export class Writer { : {}; const isBreak = special === undefined && text.charCodeAt(start) === 10; const wrappersOnly = special !== undefined || isBreak; + const fields = this.textFields(want[TYPE_KEY], have[TYPE_KEY]); let diff: Attrs | null = null; for (const k in want) { - if ( - owned(k, wrappersOnly, isBreak) && - !equalFormatValue(want[k] ?? null, have[k] ?? null) - ) { - (diff || (diff = {}))[k] = want[k] ?? null; + if (owned(k, wrappersOnly, isBreak, fields)) { + const value = this.formatAsStored(k, want[k] ?? null, have[k]); + if (!equalFormatValue(value, have[k] ?? null)) { + (diff || (diff = {}))[k] = value; + } } } for (const k in have) { if ( !(k in want) && - owned(k, wrappersOnly, isBreak) && + owned(k, wrappersOnly, isBreak, fields) && have[k] !== null ) { (diff || (diff = {}))[k] = null; @@ -3795,24 +3912,22 @@ export class Writer { const y = typeOf(item); const current = attrsOf(y) as Attrs; const storedText = $isElementNode(line) && !$holdsText(line); + const owners = this.markerOwners(desired, current); + const want = this.$markerAsStored(desired, current, owners, line); let changed = false; - for (const k in desired) { + for (const k in want) { if ( - !equalValue(current[k], desired[k]) && - !this.keepsStored(k, current[k], desired[k], storedText) + !equalValue(current[k], want[k]) && + !this.keepsStored(k, current[k], want[k], storedText) ) { - setAttr(y, k, desired[k], this.tr); + setAttr(y, k, want[k], this.tr); changed = true; } } - const types = [ - desired[TYPE_KEY] ?? 'paragraph', - current[TYPE_KEY] ?? 'paragraph', - ]; for (const k in current) { if ( - !(k in desired) && - this.isOwnedMarkerKey(k, types) && + !(k in want) && + ownsMarkerAttr(k, owners.fields, owners.groupFields) && !this.keepsStored(k, current[k], undefined, storedText) ) { deleteAttr(y, k, this.tr); @@ -3860,29 +3975,155 @@ export class Writer { ); } - isOwnedMarkerKey(key: string, types: unknown[]): boolean { + /** + * The fields this editor owns on a marker that is stored as `current` and + * written as `desired` (see ownsMarkerAttr): its classes' of the line's + * type, of its group's type and of that group's items (whose fields an + * outer level's wrapper item stores), each stored and written. + */ + markerOwners(desired: Attrs, current: Attrs): MarkerOwners { + const editor = this.state.editor; + const group = current[GROUP_KEY]; + const info = + group === desired[GROUP_KEY] && typeof group === 'string' + ? editor._nodes.get(group) + : undefined; + const spec = info === undefined ? null : getGroupSpec(info.klass); + const groupFields = ownedFields(editor, group, desired[GROUP_KEY], false); + return { + fields: ownedFields( + editor, + current[TYPE_KEY] ?? 'paragraph', + desired[TYPE_KEY] ?? 'paragraph', + false, + ), + groupFields, + itemFields: spec === null ? groupFields : groupItemFields(spec), + }; + } + + /** + * What a write of marker attributes `desired` (what this editor owns of + * them, see $marker) stores where `current` is stored (see ownsMarkerAttr, + * `owners`): the members of the outer levels (OUTER_KEY) it does not own + * kept as stored, and, for an item new to its group (`line`), the group's + * fields it does not own taken from another item of the group, as every + * item stores them (see $groupReference). + */ + $markerAsStored( + desired: Attrs, + current: Attrs, + owners: MarkerOwners, + line: LexicalNode | null, + ): Attrs { + let want = desired; + const keepOuter = (stored: unknown, of: MarkerOwners) => { + const outer = want[OUTER_KEY]; + if (Array.isArray(outer) && stored !== undefined) { + const kept = eachOuterLevel( + outer, + stored, + of.groupFields, + of.itemFields, + withUnownedMembers, + ); + if (kept !== outer) { + want = {...want, [OUTER_KEY]: kept}; + } + } + }; + keepOuter(current[OUTER_KEY], owners); if ( - key === TYPE_KEY || - key === GROUP_KEY || - key === LEVEL_KEY || - key === OUTER_KEY || - key.startsWith(GROUP_FIELD_PREFIX) || - key.startsWith(STATE_PREFIX) + line !== null && + GROUP_KEY in desired && + current[GROUP_KEY] === undefined ) { - return true; - } - if (key.startsWith(FIELD_PREFIX)) { - const f = key.slice(FIELD_PREFIX.length); - for (const t of types) { - const info = - typeof t === 'string' ? this.state.editor._nodes.get(t) : undefined; - const spec = info ? getLineSpec(info.klass) : null; - if (spec !== null && spec.fields.has(f)) { - return true; + const ref = this.$groupReference(line, desired[GROUP_KEY]); + if (ref !== null) { + // Owned as at the other item, whose group has the item's type. + const of = this.markerOwners(desired, ref); + for (const k in ref) { + if ( + k.startsWith(GROUP_FIELD_PREFIX) && + !(k in want) && + !ownsMarkerAttr(k, of.fields, of.groupFields) + ) { + want = {...want, [k]: ref[k]}; + } } + keepOuter(ref[OUTER_KEY], of); } } - return false; + return want; + } + + /** + * The stored marker attributes of the item of `line`'s group (of type + * `gtype`) nearest before it, else after it, that this write did not + * make, or null. Its neighbor, not any item: a client that does not own a + * group field shows one group where another client shows two that differ + * in it, and the item goes into the one it is next to. + */ + private $groupReference(line: LexicalNode, gtype: unknown): Attrs | null { + const before = line.getPreviousSibling(); + const ref = + before === null ? null : this.$storedItemAtOrBefore(before, gtype); + if (ref !== null) { + return ref; + } + for (let c = line.getNextSibling(); c !== null; c = c.getNextSibling()) { + const attrs = this.storedItemAttrs(c, gtype); + if (attrs !== null) { + return attrs; + } + } + return null; + } + + /** + * The stored marker attributes of `item` or of the nearest item before it + * that this write did not make (see $groupReference), or null. Noted for + * each item walked: new items written in order find theirs at once. + */ + private $storedItemAtOrBefore( + item: LexicalNode, + gtype: unknown, + ): Attrs | null { + const walked: NodeKey[] = []; + let ref: Attrs | null = null; + for (let c: LexicalNode | null = item; c !== null; ) { + const noted = this.groupReferences.get(c.getKey()); + if (noted !== undefined) { + ref = noted; + break; + } + walked.push(c.getKey()); + ref = this.storedItemAttrs(c, gtype); + if (ref !== null) { + break; + } + c = c.getPreviousSibling(); + } + for (const key of walked) { + this.groupReferences.set(key, ref); + } + return ref; + } + + /** + * The stored marker attributes of item `item` of a group of type `gtype`, + * or null where this write made its marker (or it has none). + */ + private storedItemAttrs(item: LexicalNode, gtype: unknown): Attrs | null { + const unit = this.state.unitByKey.get(item.getKey()); + const end = unit === undefined ? null : unit.end; + if (end === null || end.deleted || this.tr.insertSet.hasId(end.id)) { + return null; + } + const marker = typeOf(end); + return marker.getAttr(GROUP_KEY) === gtype + ? (marker.getAttrs() as Attrs) + : null; } $writeNodeAttrs(y: YNode, node: LexicalNode): void { @@ -3892,6 +4133,14 @@ export class Writer { this.holed.add(node.getKey()); } const current = attrsOf(y) as Attrs; + // The fields this editor owns here (see ownsNodeAttr): its classes' of + // the type stored and of the node's. + const fields = ownedFields( + this.state.editor, + storedTypeOf(y), + node.getType(), + true, + ); if (String(y.name).slice(2) !== node.getType()) { desired[TYPE_KEY] = node.getType(); } else if (TYPE_KEY in current) { @@ -3909,7 +4158,7 @@ export class Writer { for (const k in current) { if ( !(k in desired) && - (k.startsWith(FIELD_PREFIX) || k.startsWith(STATE_PREFIX)) && + ownsNodeAttr(k, fields) && !this.keepsStored(k, current[k], undefined, storedText) ) { deleteAttr(y, k, this.tr); From 4387fe3591d853f3c7980201dd42db9cde6f9cb5 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 19:44:58 -0400 Subject: [PATCH 02/12] [lexical-website][lexical-y] Bug Fix: Open a document read only when a node class throws on its stored content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description An older client's class can throw on a value only a newer version writes, for example an `updateFromJSON` that rejects it. The binding warned `RECOVERED_READ_WARNING`, threw again on the rebuild that followed, and then stayed active with an empty editor: the whole document was missing from its view. The error also escaped into the provider's `Y.applyUpdate`, and opening such a room made `initialize()` throw with no warning. Any failure to read stored content now lands in the path unknown types use: - Each place where stored values reach class code (`importJSON`, `updateFromJSON`, a NodeState parse) runs through `Reader#$readStored`. Where the class throws, the type is noted as unreadable and a fallback stands in (the node as it was, its defaults, plain text, or a paragraph). After the read the binding goes read only and warns `UNKNOWN_NODE_TYPES_WARNING` with the new reason `'unreadable'`, with `types` and `error`. The editor keeps showing the rest of the document. - A read that still throws on the fresh read of the document that follows (a reader bug, not a class) fails closed too: read only, the same warning, the editor shows what it showed before, and the binding reads nothing more. Nothing reaches Yjs or the provider. - A read-only client writes nothing. `editYState` and the other headless helpers refuse such a document with `YHeadlessError` (`'read-only'`) and its typed warnings. `YUnknownNodeTypesData` becomes a union of its two shapes, in TypeScript and Flow, so `data.reason === 'unreadable'` narrows to the one with `error`. ## Test plan New cases in `packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: a line and a node of its own, each live (a value a newer client sets) and on opening a room; a read that throws on every attempt, live and on opening; and `editYState`. Each checks that the client is read only, warns once, throws nothing, and writes nothing afterwards. ### Before On the previous commit with this test file (the describe blocks that pass are left out), `./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: ``` ❯ packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts (45 tests | 7 failed) ❯ S7-21: stored content an older class throws on opens the room read only (4) × a line: a value a newer client sets × a node of its own: a value a newer client sets × a line: opening a room that holds it × a node of its own: opening a room that holds it ❯ S7-21: a read that throws on every attempt fails closed (2) × a remote change: the editor keeps what it showed × opening the room: nothing is thrown, and nothing is shown ❯ S7-3, S7-11, S7-21: a server agent with older classes (3) ✓ S7-3: its edit keeps the fields its classes do not declare ✓ S7-11: it refuses a document with a link type it does not register × S7-21: it refuses a document its classes throw on Tests 7 failed | 37 passed | 1 expected fail (45) ``` For example: ``` FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: stored content an older class throws on opens the room read only > a line: a value a newer client sets FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: stored content an older class throws on opens the room read only > a node of its own: a value a newer client sets AssertionError: expected [ Error: unknown mode v2only ] to deeply equal [] FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: a read that throws on every attempt fails closed > opening the room: nothing is thrown, and nothing is shown AssertionError: expected [Function] to not throw an error but 'Error: simulated reader failure' was thrown FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-3, S7-11, S7-21: a server agent with older classes > S7-21: it refuses a document its classes throw on AssertionError: expected Error: unknown mode v2only to be an instance of YHeadlessError ``` ### After ``` $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts Tests 44 passed | 1 expected fail (45) $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/ Tests 4718 passed | 13 expected fail | 2 skipped (4733) ``` `pnpm run ci-check` passes. Not run: browser and e2e suites. --- .changeset/lexical-y-unreadable-content.md | 7 + .../docs/collaboration/y-server.md | 4 +- .../lexical-website/docs/collaboration/y.md | 15 +- packages/lexical-y/flow/LexicalY.js.flow | 14 +- .../unit/regressions/SchemaSkew.test.ts | 262 +++++++++++++++++- packages/lexical-y/src/internal/Binding.ts | 197 ++++++++++--- packages/lexical-y/src/internal/Read.ts | 206 +++++++++++--- packages/lexical-y/src/internal/SelfCheck.ts | 6 +- packages/lexical-y/src/internal/State.ts | 6 + packages/lexical-y/src/internal/Warnings.ts | 29 +- 10 files changed, 635 insertions(+), 111 deletions(-) create mode 100644 .changeset/lexical-y-unreadable-content.md diff --git a/.changeset/lexical-y-unreadable-content.md b/.changeset/lexical-y-unreadable-content.md new file mode 100644 index 00000000000..b44779cecd5 --- /dev/null +++ b/.changeset/lexical-y-unreadable-content.md @@ -0,0 +1,7 @@ +--- +'@lexical/y': patch +--- + +[fix] Open a document read only in `@lexical/y` when a node class throws on its stored content, instead of showing it empty + +The binding warns `UNKNOWN_NODE_TYPES_WARNING` with the new reason `'unreadable'` (with `types` and `error`), keeps showing the rest of the document, and throws nothing into the provider's `Y.applyUpdate` or from `initialize()`. diff --git a/packages/lexical-website/docs/collaboration/y-server.md b/packages/lexical-website/docs/collaboration/y-server.md index 98d46d5d77a..1a7af4ba0f5 100644 --- a/packages/lexical-website/docs/collaboration/y-server.md +++ b/packages/lexical-website/docs/collaboration/y-server.md @@ -143,8 +143,8 @@ synchronous loop over many documents holds one at a time. They throw a `YHeadlessError` with a `code`: `'not-initialized'` (no layout record yet: seed the document first), `'read-only'` (a newer layout, a -node type the config does not register, or a named slot the document cannot -store, which the binding refuses to write; the error's `warnings` are the +node type the config does not register, content its classes throw on, or a +named slot the document cannot store, which the binding refuses to write; the error's `warnings` are the binding's typed warnings that say which, so an app can narrow on each one's `code` and `data.reason` instead of reading the message), `'foreign-roots'` (the update writes to a root that is not allowed), diff --git a/packages/lexical-website/docs/collaboration/y.md b/packages/lexical-website/docs/collaboration/y.md index 2b385e29a85..ab02d976fbb 100644 --- a/packages/lexical-website/docs/collaboration/y.md +++ b/packages/lexical-website/docs/collaboration/y.md @@ -98,8 +98,8 @@ provider.once('sync', () => { - The editor stays **read only** until the document's layout record has been read. `status` is a signal: `'pending'`, then `'active'`, or `'readonly'` when the binding cannot write the document safely (a newer - layout, a node type this editor does not register; a - [warning](#warnings) says why). + layout, a node type this editor does not register, stored content its + classes throw on; a [warning](#warnings) says why). Register the same node classes on every client. - `binding.initialize()`, called after the provider's first sync, writes the editor's content into an empty document (an `editor.update()` still @@ -249,7 +249,7 @@ a short note for developers: render your own text from `code` and `data`. | Constant | Reported when | `data` (`reason`: other fields) | | -- | -- | -- | | `UNSUPPORTED_LAYOUT_WARNING` | The layout is one this version cannot read. Read only. | `'newer'`: `found`; `'malformed'`: `key`, `found`; `'missing'`, `'removed'`; `'legacy-roots'`: `roots`. All: `supported` | -| `UNKNOWN_NODE_TYPES_WARNING` | The document has node types the editor cannot build: types it does not register, however they are stored (a link class it lacks too). Read only. | `'unregistered'`, `'malformed-slot'`: `types` | +| `UNKNOWN_NODE_TYPES_WARNING` | The document has node types the editor cannot build: types it does not register, however they are stored (a link class it lacks too), or content its classes throw on. Read only. | `'unregistered'`, `'malformed-slot'`: `types`; `'unreadable'`: `types`, `error` | | `STORAGE_MISMATCH_WARNING` | A node type is stored in a way its class cannot be (one warning per type). Read only. | `'declaration-changed'`: `type`, `stored`, `declared`; `'malformed'`: `type`, `key`, `found` | | `UNSUPPORTED_SLOTS_WARNING` | Named slots where the document cannot store them. Not written; read only. | `'stored-host'`, `'root'`: `types` | | `UNSUPPORTED_CONFIG_WARNING` | `BlockIdExtension` with `repairTransforms: true` (see [Set up an editor](#set-up-an-editor)). Nothing is written; read only. | `'repair-transforms'`: `options` | @@ -948,9 +948,12 @@ hold one stay readable and editable by layout 1 clients. rollout): a client writes only the fields its classes declare in `$config`, so an older client's edit of a node keeps the fields a newer version added, and a field a client knows and clears is deleted (see - "Older app versions" in the limits). A change to a class's declared node - structure does not apply to rooms that already hold that type (they keep - the recorded kind); reseed them to adopt it. + "Older app versions" in the limits). A value an older class throws on (an + `updateFromJSON` that rejects a value the newer version writes) opens the + room read only for that client, as a type it does not register does. A + change to a class's declared node structure does not apply to rooms that + already hold that type (they keep the recorded kind); reseed them to adopt + it. ### Reseeding a room diff --git a/packages/lexical-y/flow/LexicalY.js.flow b/packages/lexical-y/flow/LexicalY.js.flow index def1d31dee5..84e5773c6e9 100644 --- a/packages/lexical-y/flow/LexicalY.js.flow +++ b/packages/lexical-y/flow/LexicalY.js.flow @@ -62,10 +62,16 @@ export type YStorageMismatchData = readonly stored: YStoredAs, readonly declared: YStoredAs, }; -export type YUnknownNodeTypesData = { - readonly reason: 'unregistered' | 'malformed-slot', - readonly types: ReadonlyArray, -}; +export type YUnknownNodeTypesData = + | { + readonly reason: 'unregistered' | 'malformed-slot', + readonly types: ReadonlyArray, + } + | { + readonly reason: 'unreadable', + readonly types: ReadonlyArray, + readonly error: unknown, + }; export type YUnsupportedSlotsData = { readonly reason: 'stored-host' | 'root', readonly types: ReadonlyArray, diff --git a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts index b8ec8bbe856..f59b436efa4 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts @@ -22,6 +22,12 @@ // S7-11: content of a type a client does not register opens the room read // only for it, whatever kind stores it: a line, a group, text, a node, or an // inline element stored as a format (a link class it lacks). +// +// S7-21: stored content a client's class throws on (an updateFromJSON that +// rejects a value only a newer version writes) is unreadable for it, as +// content of a type it does not register is: it opens the room read only, +// warns (`'unreadable'`), keeps showing the rest of the document, and throws +// nothing, into the provider or from initialize(). import type {ListItemNode} from '@lexical/list'; @@ -62,6 +68,7 @@ import { seedYState, YHeadlessError, } from '../../../headless'; +import {Reader} from '../../../internal/Read'; import { ownedFields, ownsFormat, @@ -1123,9 +1130,230 @@ describe('S7-11: content of a type a client does not register opens the room rea // --------------------------------------------------------------------------- +type SerializedMode = SerializedElementNode & {mode?: string}; +/** v1 of a class with a field `mode` whose updateFromJSON rejects a value v2 writes. */ +function modeClasses(type: string, base: typeof ElementNode) { + class ModeV2 extends base { + __mode = 'a'; + static getType(): string { + return type; + } + static clone(node: ModeV2): ModeV2 { + const copy = new ModeV2(node.__key); + copy.__mode = node.__mode; + return copy; + } + static importJSON(json: SerializedMode): ModeV2 { + return new ModeV2().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + const self = super.updateFromJSON(json); + self.__mode = json.mode ?? 'a'; + return self; + } + exportJSON(): SerializedMode { + return {...super.exportJSON(), mode: this.__mode, type}; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } + } + class ModeV1 extends ModeV2 { + static getType(): string { + return type; + } + static clone(node: ModeV1): ModeV1 { + const copy = new ModeV1(node.__key); + copy.__mode = node.__mode; + return copy; + } + static importJSON(json: SerializedMode): ModeV1 { + return new ModeV1().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + if (json.mode !== undefined && json.mode !== 'a' && json.mode !== 'b') { + throw new Error('unknown mode ' + json.mode); + } + return super.updateFromJSON(json); + } + } + return {ModeV1, ModeV2}; +} + +const modeLine = modeClasses( + 'test_mode_line', + ParagraphNode as unknown as typeof ElementNode, +); +const modeBox = modeClasses('test_mode_box', ElementNode); + +describe('S7-21: stored content an older class throws on opens the room read only', () => { + const shapes = [ + [ + 'a line', + modeLine, + 'test_mode_line', + () => $create(modeLine.ModeV2).append($createTextNode('mode line')), + ], + [ + 'a node of its own', + modeBox, + 'test_mode_box', + () => $create(modeBox.ModeV2).append($p('in the box')), + ], + ] as const; + + test.each(shapes)( + '%s: a value a newer client sets', + async (_name, {ModeV1, ModeV2}, type, $node) => { + const net = newNet(); + const author = new Peer(2, net as never, [...NODES, ModeV2]); + const old = new Peer(1, net as never, [...NODES, ModeV1]); + author.update(() => { + $getRoot().clear().append($p('first'), $node(), $p('last')); + }); + author.binding.initialize(); + await flush(); + const shown = old.describe(); + expect(old.binding.status).toBe('active'); + author.update(() => { + ( + $block([1]) as unknown as {getWritable(): {__mode: string}} + ).getWritable().__mode = 'v2only'; + }); + await flush(); + expectUnreadable(old, type, shown); + await expectWritesNothing(old, author); + }, + ); + + test.each(shapes)( + '%s: opening a room that holds it', + async (_name, {ModeV1, ModeV2}, type, $node) => { + const net = newNet(); + const author = new Peer(2, net as never, [...NODES, ModeV2]); + author.update(() => { + const node = $node(); + (node as unknown as {__mode: string}).__mode = 'v2only'; + $getRoot().clear().append($p('first'), node, $p('last')); + }); + author.binding.initialize(); + await flush(); + const old = new Peer(1, newNet() as never, [...NODES, ModeV1]); + Y.applyUpdate(old.doc, Y.encodeStateAsUpdate(author.doc)); + old.binding.initialize(); + await flush(); + expectUnreadable(old, type, null); + expect(old.describe()).toContain('"first"'); + expect(old.describe()).toContain('"last"'); + }, + ); +}); + +describe('S7-21: a read that throws on every attempt fails closed', () => { + // Not a class: the reader itself (a bug) throws, on a read and on the + // fresh read of the document that follows. + const failingReads = () => { + const proto = Reader.prototype as unknown as { + $apply: (...args: unknown[]) => void; + $readFull: (...args: unknown[]) => void; + }; + const {$apply, $readFull} = proto; + proto.$apply = proto.$readFull = () => { + throw new Error('simulated reader failure'); + }; + return () => { + proto.$apply = $apply; + proto.$readFull = $readFull; + }; + }; + const warned = (peer: Peer) => + peer.warnings.filter(w => !w.includes('stored as a nested')); + + test('a remote change: the editor keeps what it showed', async () => { + const net = newNet(); + const a = new Peer(2, net as never); + const b = new Peer(1, net as never); + a.update(() => $getRoot().clear().append($p('hello'))); + a.binding.initialize(); + await flush(); + const restore = failingReads(); + try { + a.update(() => $block([0]).append($createTextNode(' there'))); + await flush(); + } finally { + restore(); + } + expect(b.errors).toEqual([]); + expect(b.binding.status).toBe('readonly'); + expect(warned(b)).toEqual([ + 'the document cannot be read: simulated reader failure', + ]); + expect(b.describe()).toBe('paragraph("hello")'); + // It reads nothing more. + a.update(() => $block([0]).append($createTextNode('!'))); + await flush(); + expect(b.describe()).toBe('paragraph("hello")'); + expect(b.errors).toEqual([]); + }); + + test('opening the room: nothing is thrown, and nothing is shown', async () => { + const a = new Peer(2, newNet() as never); + a.update(() => $getRoot().clear().append($p('hello'))); + a.binding.initialize(); + await flush(); + const b = new Peer(1, newNet() as never); + const restore = failingReads(); + try { + expect(() => { + Y.applyUpdate(b.doc, Y.encodeStateAsUpdate(a.doc)); + b.binding.initialize(); + }).not.toThrow(); + } finally { + restore(); + } + await flush(); + expect(b.errors).toEqual([]); + expect(b.binding.status).toBe('readonly'); + expect(warned(b)).toEqual([ + 'the document cannot be read: simulated reader failure', + ]); + expect(b.describe()).toBe(''); + }); +}); + +/** + * `peer` opened the room read only for stored content of `type` its class + * throws on, warned once, threw nothing, and shows `shown` (when given). + */ +function expectUnreadable(peer: Peer, type: string, shown: string | null) { + expect(peer.errors).toEqual([]); + expect(peer.binding.status).toBe('readonly'); + expect( + peer.warnings.filter(w => !w.includes('stored as a nested node')), + ).toEqual([`stored content the editor's classes cannot read: ${type}`]); + if (shown !== null) { + expect(peer.describe()).toBe(shown); + } +} + +/** An edit of read-only `peer` changes nothing `other` shows or stores. */ +async function expectWritesNothing(peer: Peer, other: Peer) { + const vector = Y.encodeStateVector(peer.doc); + const before = docJSON(other); + peer.update(() => $getRoot().append($p('typed while read only'))); + await flush(); + expect(Y.encodeStateVector(peer.doc)).toEqual(vector); + expect(docJSON(other)).toBe(before); +} + +// --------------------------------------------------------------------------- + // The same rules for a server agent whose config has the older classes: the // headless helpers write through the same binding. -describe('S7-3, S7-11: a server agent with older classes', () => { +describe('S7-3, S7-11, S7-21: a server agent with older classes', () => { const quiet = () => {}; const refusal = (edit: () => void) => { try { @@ -1186,4 +1414,36 @@ describe('S7-3, S7-11: a server agent with older classes', () => { ], }); }); + + test('S7-21: it refuses a document its classes throw on', () => { + const seeded = seedYState( + {nodes: [...NODES, modeBox.ModeV2], onWarn: quiet}, + () => { + const box = $create(modeBox.ModeV2).append($p('in the box')); + box.__mode = 'v2only'; + $getRoot().append(box); + }, + ); + const error = refusal(() => + editYState( + seeded, + {nodes: [...NODES, modeBox.ModeV1], onWarn: quiet}, + () => $getRoot().append($p('more')), + {writer: null}, + ), + ); + expect(error).toBeInstanceOf(YHeadlessError); + expect(error).toMatchObject({code: 'read-only'}); + expect((error as YHeadlessError).warnings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + code: UNKNOWN_NODE_TYPES_WARNING, + data: expect.objectContaining({ + reason: 'unreadable', + types: ['test_mode_box'], + }), + }), + ]), + ); + }); }); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index f523994041d..854b62d6758 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -630,6 +630,11 @@ export class YBinding { private readonly pendingWarnings: LexicalWarning[] = []; /** Depth of the binding's own work in progress (see guard). */ private busy = 0; + /** + * Whether the editor's classes cannot read the document (see failClosed): + * the binding is read only, and reads nothing more. + */ + private unreadable = false; private flushingWarnings = false; /** Whether start() has completed, and dispose() not run since. */ private started = false; @@ -1431,7 +1436,11 @@ export class YBinding { } private rereadContainers(list: CS[], extraTags: string[] = []): void { + if (this.unreadable) { + return; + } this.commitPending(); + const shown = this._editor.getEditorState(); let error: unknown = null; this._editor.update( () => { @@ -1457,7 +1466,7 @@ export class YBinding { }, ); if (error !== null) { - this.recoverRead(error); + this.recoverRead(error, shown); } } @@ -1495,8 +1504,25 @@ export class YBinding { return true; } - /** Fail closed when a read found node types this editor lacks. */ + /** + * Fail closed when a read found node types this editor lacks, or stored + * content its classes throw on (see Reader#$readStored). + */ private checkUnknownTypes(): void { + const unreadable = this.state.unreadable; + if (unreadable.size > 0 && this._status === 'active') { + const types = [...unreadable.keys()].sort(); + this.warn( + UNKNOWN_NODE_TYPES_WARNING, + `stored content the editor's classes cannot read: ${types.join(', ')}`, + { + error: unreadable.get(types[0]), + reason: 'unreadable', + types, + }, + ); + this.setStatus('readonly'); + } const unknown = this.state.unknownTypes; if (unknown.size > 0 && this._status === 'active') { const slots = this.state.malformedSlots; @@ -1643,39 +1669,18 @@ export class YBinding { kept: SelectionState | null = null, deep = false, ): void { + if (this.unreadable) { + return; + } + const shown = this._editor.getEditorState(); + let error: unknown = null; this._editor.update( () => { - const reader = new Reader(this.state); - let cs = this.state.containers.get(this._root); - if (cs === undefined) { - cs = this.state.newCS(this._root, 'root', 'b'); - } - reader.$readFull(cs, $getRoot()); - if (deep) { - // The containers of the stored nodes the read kept (see - // initializeInner). - for (const inner of [...this.state.csByKey.values()]) { - const el = $getNodeByKey(inner.key); - if ( - inner !== cs && - this.state.csByKey.get(inner.key) === inner && - $isElementNode(el) && - el.isAttached() - ) { - reader.$readFull(inner, el); - } - } - } - $setSelection(null); - if (cursor !== null) { - this.$restoreSelection(cursor); - } - if (kept !== null) { - $restoreSelectionState(this.state, kept, json => - this.$resolvePosition(json), - ); + try { + this.$readAllInner(cursor, kept, deep); + } catch (e) { + error = e; } - this.$quietSelection(); }, { discrete: true, @@ -1683,10 +1688,52 @@ export class YBinding { tag: REMOTE_TAGS, }, ); + if (error !== null) { + this.failClosed(error, shown); + return; + } this.checkUnknownTypes(); this.flushRefresh(true); } + private $readAllInner( + cursor: YCursor | null, + kept: SelectionState | null, + deep: boolean, + ): void { + const reader = new Reader(this.state); + let cs = this.state.containers.get(this._root); + if (cs === undefined) { + cs = this.state.newCS(this._root, 'root', 'b'); + } + reader.$readFull(cs, $getRoot()); + if (deep) { + // The containers of the stored nodes the read kept (see + // initializeInner). + for (const inner of [...this.state.csByKey.values()]) { + const el = $getNodeByKey(inner.key); + if ( + inner !== cs && + this.state.csByKey.get(inner.key) === inner && + $isElementNode(el) && + el.isAttached() + ) { + reader.$readFull(inner, el); + } + } + } + $setSelection(null); + if (cursor !== null) { + this.$restoreSelection(cursor); + } + if (kept !== null) { + $restoreSelectionState(this.state, kept, json => + this.$resolvePosition(json), + ); + } + this.$quietSelection(); + } + // ------------------------------------------------------------------------- // Remote -> Lexical @@ -1747,6 +1794,9 @@ export class YBinding { return; } this.remoteSeq++; + if (this.unreadable) { + return; + } if (this._status === 'pending') { if (recordedLayout(this._root) !== null) { this.evaluate(); @@ -1847,12 +1897,14 @@ export class YBinding { } let rebuilt: ReadonlySet = new Set(); let readError: unknown = null; + // What the editor shows before the read (see failClosed). + const shown = this._editor.getEditorState(); // A re-read of a naming update's change is tagged as that update (see // rereadRefused). const naming = this.refused.size > 0 && this.refusedNaming; const after = () => { if (readError !== null) { - this.recoverRead(readError); + this.recoverRead(readError, shown); return; } this.checkUnknownTypes(); @@ -1982,8 +2034,16 @@ export class YBinding { * showing something else than the document for good: the editor is * rebuilt from the shared content (a fresh reader's result, which every * client converges on) in a fresh update, and the failure is reported. + * Where the rebuild throws too, the editor's classes cannot read the + * document: the binding fails closed, back to `shown` (see failClosed). */ - private recoverRead(error: unknown): void { + private recoverRead(error: unknown, shown: EditorState): void { + try { + this.rebuildFromDocument(); + } catch (e) { + this.failClosed(e, shown); + return; + } this.warn( RECOVERED_READ_WARNING, `a remote change could not be read: ${String( @@ -1991,10 +2051,43 @@ export class YBinding { )}`, {error, reason: 'remote-change'}, ); - try { - this.rebuildFromDocument(); - } catch (e) { - this._editor._onError(e instanceof Error ? e : new Error(String(e))); + } + + /** + * Fail closed where the document cannot be read (a read threw and so did + * a fresh read of the document after it, or a read of the whole document + * did; a class that throws on stored content does not, see + * Reader#$readStored): warn (UNKNOWN_NODE_TYPES_WARNING, reason + * `'unreadable'`), open read only, and show `shown` again, what the editor + * showed before. The binding reads nothing more from then on, so the + * editor keeps showing it, and no error reaches Yjs or a provider. + */ + private failClosed(error: unknown, shown: EditorState): void { + if (this.unreadable) { + return; + } + this.unreadable = true; + this.warn( + UNKNOWN_NODE_TYPES_WARNING, + `the document cannot be read: ${String( + error instanceof Error ? error.message : error, + )}`, + {error, reason: 'unreadable', types: []}, + ); + this.setStatus('readonly'); + const editor = this._editor; + if (editor.getEditorState() === shown) { + return; + } + if (shown.isEmpty()) { + // Before the first read: an editor that showed nothing yet. + editor.update(() => $getRoot().clear(), { + discrete: true, + skipTransforms: true, + tag: REMOTE_TAGS, + }); + } else { + editor.setEditorState(shown, {tag: COLLABORATION_TAG}); } } @@ -2661,6 +2754,7 @@ export class YBinding { this._status !== 'active' || state.hidden !== null || state.unknownTypes.size > 0 || + state.unreadable.size > 0 || this.untransformed.size > 0 || this.untransformedText.size > 0 || this.storedHoles.size > 0 @@ -3633,6 +3727,9 @@ export class YBinding { $addUpdateTag(BLOCK_ID_REPAIR_TAG); } const cursor = this.$cursorBeforeRead(); + // What the editor shows before the read (see failClosed). + const shown = this._editor.getEditorState(); + let unreadable: unknown = null; try { const reader = new Reader(this.state); for (const cs of list) { @@ -3644,14 +3741,22 @@ export class YBinding { reader.$settle(settle); this.$selectionAfterRead(cursor); } catch (e) { - this.warn( - RECOVERED_READ_WARNING, - `a change could not be read again: ${String( - e instanceof Error ? e.message : e, - )}`, - {error: e, reason: 'reread'}, - ); - this.$rebuild(); + try { + this.$rebuild(); + this.warn( + RECOVERED_READ_WARNING, + `a change could not be read again: ${String( + e instanceof Error ? e.message : e, + )}`, + {error: e, reason: 'reread'}, + ); + } catch (rebuildError) { + unreadable = rebuildError; + } + } + if (unreadable !== null) { + const error = unreadable; + return () => this.failClosed(error, shown); } this.$quietSelection(); if (last) { diff --git a/packages/lexical-y/src/internal/Read.ts b/packages/lexical-y/src/internal/Read.ts index d85ad06c08b..366c0c1e77e 100644 --- a/packages/lexical-y/src/internal/Read.ts +++ b/packages/lexical-y/src/internal/Read.ts @@ -17,6 +17,7 @@ import { import { $createLineBreakNode, $createParagraphNode, + $createTextNode, $getEditor, $getNodeByKey, $getSelection, @@ -297,6 +298,25 @@ export class Reader { }; } + /** + * Run `read`, code of the class of stored content of `type` that reads the + * stored values (its importJSON or updateFromJSON, a NodeState parse). + * Where it throws, this editor cannot read that content, as with content + * of a type it does not register: the type is noted (see + * State#unreadable), the binding opens read only after the read, and + * `fallback` stands in (what is shown keeps the rest of the document). + */ + private $readStored(type: string, read: () => T, fallback: () => T): T { + try { + return read(); + } catch (error) { + if (!this.state.unreadable.has(type)) { + this.state.unreadable.set(type, error); + } + return fallback(); + } + } + // ------------------------------------------------------------------------- // Entry points @@ -1631,12 +1651,18 @@ export class Reader { level, ); if (wrapper === undefined) { - wrapper = $parseSerializedNode({ - ...clean, - children: [], - type: spec.items[0].getType(), - version: 1, - } as unknown as SerializedLexicalNode) as ElementNode; + const type = spec.items[0].getType(); + wrapper = this.$readStored( + type, + () => + $parseSerializedNode({ + ...clean, + children: [], + type, + version: 1, + } as unknown as SerializedLexicalNode) as ElementNode, + () => $parseStoredDefaults(type, true) as ElementNode, + ); $setTextFields(wrapper, clean); } else if ( stableStringify($levelItemFields(wrapper, spec)) !== @@ -1649,8 +1675,14 @@ export class Reader { Object.assign(json, clean); json.$ = clean.$ || {}; json.type = wrapper.getType(); - wrapper = wrapper.updateFromJSON( - json as Parameters[0], + const node = wrapper; + wrapper = this.$readStored( + node.getType(), + () => + node.updateFromJSON( + json as Parameters[0], + ), + () => node, ); } $setTextFields(wrapper, clean); @@ -1711,20 +1743,31 @@ export class Reader { Object.assign(json, fields); json.$ = fields.$ || {}; json.type = reused.getType(); - reused.updateFromJSON( - json as Parameters[0], + const group = reused; + this.$readStored( + group.getType(), + () => + group.updateFromJSON( + json as Parameters[0], + ), + () => group, ); $setTextFields(reused, fields); } this.state.ids.noteStoredId(reused.getKey(), storedId); return reused.getLatest(); } - const created = $parseSerializedNode({ - children: [], - type: item.gtype, - version: 1, - ...fields, - } as unknown as SerializedLexicalNode) as ElementNode; + const created = this.$readStored( + item.gtype, + () => + $parseSerializedNode({ + children: [], + type: item.gtype, + version: 1, + ...fields, + } as unknown as SerializedLexicalNode) as ElementNode, + () => $parseStoredDefaults(item.gtype, true) as ElementNode, + ); $setTextFields(created, fields); this.state.ids.noteStoredId(created.getKey(), storedId); return created; @@ -1748,9 +1791,22 @@ export class Reader { return existing.getLatest(); } const json = lineJSON(attrs, derived, type as string); - return $parseSerializedNode( - json as unknown as SerializedLexicalNode, - ) as ElementNode; + return this.$readStored( + type as string, + () => + $parseSerializedNode( + json as unknown as SerializedLexicalNode, + ) as ElementNode, + // As a line of a type it does not register. + () => + $parseSerializedNode( + lineJSON( + {[TYPE_KEY]: 'paragraph'}, + derived, + 'paragraph', + ) as unknown as SerializedLexicalNode, + ) as ElementNode, + ); } /** @@ -1813,10 +1869,17 @@ export class Reader { json.textStyle = ''; } json.type = node.getType(); - const writable = node.updateFromJSON( - json as Parameters[0], + this.$readStored( + node.getType(), + () => + $setStateInOrder( + node.updateFromJSON( + json as Parameters[0], + ), + state, + ), + () => undefined, ); - $setStateInOrder(writable, state); } private $applyNodeAttrs(node: LexicalNode, y: YNode, placed = true): void { @@ -1862,7 +1925,11 @@ export class Reader { owned($nodeAttrs(node, holes, false)) !== owned(attrs) || holes.length > 0; if (differs) { - $applyStoredAttrs(node, attrs); + this.$readStored( + node.getType(), + () => $applyStoredAttrs(node, attrs), + () => undefined, + ); if ( $isElementNode(node) && getGroupSpec(klassOf(node.getLatest())) !== null @@ -1890,7 +1957,14 @@ export class Reader { if ($isElementNode(node)) { json.children = []; } - const next = $parseSerializedNode(json as unknown as SerializedLexicalNode); + const next = this.$readStored( + type, + () => $parseSerializedNode(json as unknown as SerializedLexicalNode), + () => null, + ); + if (next === null) { + return; + } if ($isElementNode(node) && $isElementNode(next)) { $appendAll(next, node.getChildren()); } @@ -1948,7 +2022,11 @@ export class Reader { if (isElement) { json.children = []; } - const node = $parseSerializedNode(json as unknown as SerializedLexicalNode); + const node = this.$readStored( + type, + () => $parseSerializedNode(json as unknown as SerializedLexicalNode), + () => $parseStoredDefaults(type, isElement), + ); this.state.mapNode(node.getKey(), y); // It shows the id it stores: in the index of who stores what. this.state.ids.noteStoredId( @@ -2274,16 +2352,27 @@ export class Reader { node.getWritable().__state = undefined; } if (foreign || currentSig !== piece.tsig) { - node = $applyJSON(node, json); + const text = node; + node = this.$readStored( + text.getType(), + () => $applyJSON(text, json), + () => text, + ); } if (node.getTextContent() !== piece.text) { node = node.setTextContent(piece.text); } $clampPointsIn(node); } else { - node = $parseSerializedNode( - json as unknown as SerializedLexicalNode, - ) as TextNode; + node = this.$readStored( + json.type as string, + () => + $parseSerializedNode( + json as unknown as SerializedLexicalNode, + ) as TextNode, + // As text of a type it does not register. + () => $createTextNode(piece.text), + ); if (segment !== null) { state.setSegment(node.getKey(), segment); } @@ -2299,12 +2388,18 @@ export class Reader { ) { br = br.getWritable(); const info = state.editor._nodes.get(br.getType()); - $getWritableNodeState(br).updateFromJSON( - joinSetMembers( - brState, - info && info.sharedNodeState.sharedConfigMap, - info && learnedSetKeys(info.sharedNodeState), - ), + const writable = br; + this.$readStored( + br.getType(), + () => + $getWritableNodeState(writable).updateFromJSON( + joinSetMembers( + brState, + info && info.sharedNodeState.sharedConfigMap, + info && learnedSetKeys(info.sharedNodeState), + ), + ), + () => undefined, ); } push(br); @@ -2389,19 +2484,30 @@ export class Reader { delete json.textFormat; delete json.textStyle; Object.assign(json, w.json); - const writable = reused.updateFromJSON( - json as Parameters[0], + return this.$readStored( + w.type, + () => { + const writable = reused.updateFromJSON( + json as Parameters[0], + ); + $setTextFields(writable, w.json); + $setStateInOrder(writable, (w.json.$ as Attrs) || {}); + return writable; + }, + () => reused, ); - $setTextFields(writable, w.json); - $setStateInOrder(writable, (w.json.$ as Attrs) || {}); - return writable; } return reused; } - return $parseSerializedNode({ - ...w.json, - children: [], - } as unknown as SerializedLexicalNode) as ElementNode; + return this.$readStored( + w.type, + () => + $parseSerializedNode({ + ...w.json, + children: [], + } as unknown as SerializedLexicalNode) as ElementNode, + () => $parseStoredDefaults(w.type, true) as ElementNode, + ); } /** Make `el`'s children exactly `children`, keeping unchanged ends. */ @@ -2486,6 +2592,18 @@ type GroupItemInfo = { spec: GroupSpec; }; +/** + * A node of `type` with no stored values: what a read shows for stored + * content of `type` that its class throws on (see Reader#$readStored). + */ +function $parseStoredDefaults(type: string, element: boolean): LexicalNode { + return $parseSerializedNode({ + type, + version: 1, + ...(element ? {children: []} : {}), + } as unknown as SerializedLexicalNode); +} + /** * Give a stored node the attributes `attrs` (see nodeJSONFromAttrs), as a * reader shows them. diff --git a/packages/lexical-y/src/internal/SelfCheck.ts b/packages/lexical-y/src/internal/SelfCheck.ts index 284d5cd0be8..c885f3cd2bc 100644 --- a/packages/lexical-y/src/internal/SelfCheck.ts +++ b/packages/lexical-y/src/internal/SelfCheck.ts @@ -510,7 +510,11 @@ export function checkContainer( cs: CS, editorState: EditorState, ): string[] { - if (state.hidden !== null || state.unknownTypes.size > 0) { + if ( + state.hidden !== null || + state.unknownTypes.size > 0 || + state.unreadable.size > 0 + ) { return []; } const live = editorState.read(() => { diff --git a/packages/lexical-y/src/internal/State.ts b/packages/lexical-y/src/internal/State.ts index 5355476f2da..f0d6be0fdd3 100644 --- a/packages/lexical-y/src/internal/State.ts +++ b/packages/lexical-y/src/internal/State.ts @@ -170,6 +170,12 @@ export class State { * only rather than hide it and then delete it on the next edit. */ readonly unknownTypes = new Set(); + /** + * Node types whose stored content this editor's classes throw on when they + * read it (see Reader#$readStored), each with the first error: as with + * unknownTypes, the binding goes read only. + */ + readonly unreadable = new Map(); /** * Of unknownTypes, the slots that are not a block node: a slot's key, or * the type of the inline node in it (see Slots). diff --git a/packages/lexical-y/src/internal/Warnings.ts b/packages/lexical-y/src/internal/Warnings.ts index db2d7e23427..0d587757140 100644 --- a/packages/lexical-y/src/internal/Warnings.ts +++ b/packages/lexical-y/src/internal/Warnings.ts @@ -140,14 +140,29 @@ export type YStorageMismatchData = /** * @experimental `data` of an `UNKNOWN_NODE_TYPES_WARNING` (the binding is read - * only): types the editor does not register (`'unregistered'`), or slots the - * document holds that are not a block node (`'malformed-slot'`: the slot's - * key, or the type of the inline node in it). + * only). */ -export interface YUnknownNodeTypesData { - readonly reason: 'unregistered' | 'malformed-slot'; - readonly types: readonly string[]; -} +export type YUnknownNodeTypesData = + /** + * Types the editor does not register (`'unregistered'`), or slots the + * document holds that are not a block node (`'malformed-slot'`: the slot's + * key, or the type of the inline node in it). + */ + | { + readonly reason: 'unregistered' | 'malformed-slot'; + readonly types: readonly string[]; + } + /** + * The editor's classes throw `error` on stored content (of `types`, where + * the error came from a class reading content of a type), on a read and on + * a fresh read of the document after it. The editor keeps what it showed + * before, and reads nothing more. + */ + | { + readonly reason: 'unreadable'; + readonly types: readonly string[]; + readonly error: unknown; + }; /** * @experimental `data` of an `UNSUPPORTED_SLOTS_WARNING` (the change is not From a9aadff64b7bb22f8f5459b4237e7b1c0ed23c4b Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 20:24:46 -0400 Subject: [PATCH 03/12] [lexical-website][lexical-y] Bug Fix: Match outer list levels by group, and undo what a class changed before it threw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Follow-up to the two version-skew fixes, from an independent review of them. - **A nested group's outer levels are matched by group.** A line's marker keeps the members of its outer levels (`o`) that the client does not own. It matched them by depth, so an edit that removed an outer group (lifting the middle of three nested lists out of the outer one) gave the remaining level the removed group's fields. Each level now takes the members stored for the same group, found from the groups that held the line before the update; a level whose group cannot be matched takes none. - **A run of new group items finds its reference item in linear time.** Prepending many items to a list scanned forward to the same stored item from each new one (quadratic); the forward walk is now noted like the backward one. - **What a class changed before it threw is undone.** A class's `updateFromJSON` may change a node and then throw; the read kept those changes, and where Lexical could not commit them the error still escaped into the provider. The node now gets back what it held before. A fallback node is built by the class's constructor instead of its `importJSON`, which may reject the minimal JSON the fallback gave it, and a read that throws while committing fails closed too. - **Any thrown value fails closed.** A thrown `null` was taken for "no error", and a thrown value that cannot be turned into a string made the warning itself throw. - **`YUnknownNodeTypesData` stays an interface.** The new `'unreadable'` reason has its own `YUnreadableContentData` (TypeScript and Flow), so an app's `interface ... extends YUnknownNodeTypesData` still compiles. - y.md names one more limit, with a `test.fails` case: a link an older client replaces with another link of the same type over the same text keeps the fields it does not know of the replaced link. ## Test plan New cases in `packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`, one per review finding, plus indent and outdent across levels of distinct fields, and a type-level check that the warning data can be extended. ### Before On the previous commit with this test file (the describe blocks that pass are left out), `./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: ``` ❯ packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts (55 tests | 7 failed) ❯ S7-3: an older client restructuring a nested group (5) × an outer group removed: each level keeps its own group's fields ✓ outdent, levels of distinct fields ✓ indent, levels of distinct fields × a run of new items finds the item their group fields come from in linear time ✓ limit: a link an older client replaces with another keeps the fields it does not know ❯ S7-21: what an older class does before it throws (5) × a fallback node does not go through the importJSON that rejected it × what it throws need not be printable × a node it updates: what it changed is undone × a node it updates: a change it made that Lexical cannot commit is undone × a reader that throws null fails closed too Tests 7 failed | 46 passed | 2 expected fail (55) ``` For example: ``` FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-3: an older client restructuring a nested group > an outer group removed: each level keeps its own group's fields AssertionError: expected { root: { …(6) } } to deeply equal { root: { …(6) } } - Expected + Received @@ -50,11 +50,11 @@ "format": "", "indent": 0, "listType": "bullet", "start": 1, "tag": "ul", - "tone": "middle", + "tone": "outer", "type": "test_skew_list", "version": 1, }, ], FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-3: an older client restructuring a nested group > a run of new items finds the item their group fields come from in linear time AssertionError: expected 33151 to be less than 8944 FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: what an older class does before it throws > what it throws need not be printable AssertionError: expected [Function] to not throw an error but 'TypeError: Cannot convert object to p…' was thrown FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: what an older class does before it throws > a node it updates: what it changed is undone AssertionError: expected 'right' to be '' // Object.is equality FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: what an older class does before it throws > a node it updates: a change it made that Lexical cannot commit is undone AssertionError: expected 'active' to be 'readonly' // Object.is equality FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21: what an older class does before it throws > a reader that throws null fails closed too AssertionError: expected 'active' to be 'readonly' // Object.is equality ``` `tsc -p tsconfig.test.json` on the previous commit with this test file: ``` packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts(1737,50): error TS2312: An interface can only extend an object type or intersection of object types with statically known members. ``` ### After ``` $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts Tests 53 passed | 2 expected fail (55) $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/ Tests 4727 passed | 14 expected fail | 2 skipped (4743) ``` `pnpm run ci-check` passes. Sessions without version skew still store the same bytes as `main`: over the whole `@lexical/y` unit suite plus 800 fuzz seeds, every deterministic session is byte-identical (the only test that differs draws random client ids and differs between two runs of `main` too). Not run: browser and e2e suites. --- .../lexical-website/docs/collaboration/y.md | 10 +- packages/lexical-y/flow/LexicalY.js.flow | 24 +- .../unit/regressions/SchemaSkew.test.ts | 292 +++++++++++++++++- packages/lexical-y/src/index.ts | 2 + packages/lexical-y/src/internal/Binding.ts | 211 +++++++------ packages/lexical-y/src/internal/Read.ts | 183 ++++++----- packages/lexical-y/src/internal/Schema.ts | 3 +- packages/lexical-y/src/internal/Warnings.ts | 51 +-- packages/lexical-y/src/internal/Write.ts | 174 +++++++++-- 9 files changed, 703 insertions(+), 247 deletions(-) diff --git a/packages/lexical-website/docs/collaboration/y.md b/packages/lexical-website/docs/collaboration/y.md index ab02d976fbb..c7671a157ef 100644 --- a/packages/lexical-website/docs/collaboration/y.md +++ b/packages/lexical-website/docs/collaboration/y.md @@ -878,10 +878,12 @@ not merge as a user might expect: NodeState it does not know when it edits a node (see [Changing your own node types](#changing-the-layout-and-migrating)), but a block it moves is written anew from what it shows, so the moved block loses - them (see Moves). A class that writes its own `exportJSON` owns every - field of its type, since which ones it writes cannot be known: an older - version of it deletes the fields a newer one added. Declare fields in - `$config` to keep them. + them (see Moves). A link it replaces with another link of the same type over + the same text keeps the ones of the link it replaced: it cannot tell the + new link from the old one edited. A class that writes its own `exportJSON` + owns every field of its type, since which ones it writes cannot be known: + an older version of it deletes the fields a newer one added. Declare fields + in `$config` to keep them. - **The caret when a collaborator rewrites its line.** A caret stays after the text it was after. When a collaborator replaces the line holding it, deleting all the line showed and putting new content at its place in one diff --git a/packages/lexical-y/flow/LexicalY.js.flow b/packages/lexical-y/flow/LexicalY.js.flow index 84e5773c6e9..162e72668bc 100644 --- a/packages/lexical-y/flow/LexicalY.js.flow +++ b/packages/lexical-y/flow/LexicalY.js.flow @@ -62,16 +62,15 @@ export type YStorageMismatchData = readonly stored: YStoredAs, readonly declared: YStoredAs, }; -export type YUnknownNodeTypesData = - | { - readonly reason: 'unregistered' | 'malformed-slot', - readonly types: ReadonlyArray, - } - | { - readonly reason: 'unreadable', - readonly types: ReadonlyArray, - readonly error: unknown, - }; +export type YUnknownNodeTypesData = { + readonly reason: 'unregistered' | 'malformed-slot', + readonly types: ReadonlyArray, +}; +export type YUnreadableContentData = { + readonly reason: 'unreadable', + readonly types: ReadonlyArray, + readonly error: unknown, +}; export type YUnsupportedSlotsData = { readonly reason: 'stored-host' | 'root', readonly types: ReadonlyArray, @@ -129,7 +128,10 @@ declare export class YWarningOf } export type YWarning = | YWarningOf - | YWarningOf + | YWarningOf< + typeof UNKNOWN_NODE_TYPES_WARNING, + YUnknownNodeTypesData | YUnreadableContentData, + > | YWarningOf | YWarningOf | YWarningOf diff --git a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts index f59b436efa4..135bfb340fd 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts @@ -76,7 +76,11 @@ import { ownsMember, ownsNodeAttr, } from '../../../internal/Schema'; -import {UNKNOWN_NODE_TYPES_WARNING} from '../../../internal/Warnings'; +import { + UNKNOWN_NODE_TYPES_WARNING, + type YUnknownNodeTypesData, +} from '../../../internal/Warnings'; +import {Writer} from '../../../internal/Write'; import { $block, $createBoxNode, @@ -867,6 +871,112 @@ describe('S7-3: an older client keeps the fields a newer class added', () => { ); }); +describe('S7-3: an older client restructuring a nested group', () => { + const $tl = (tone: string, ...items: (string | ListNode)[]) => { + const list = $create(ListV2); + list.__listType = 'bullet'; + list.__tag = 'ul'; + list.__tone = tone; + for (const item of items) { + list.append( + $createListItemNode().append( + typeof item === 'string' ? $createTextNode(item) : item, + ), + ); + } + return list; + }; + + test("an outer group removed: each level keeps its own group's fields", async () => { + // The line's marker stays; its outer levels shift by one. + const r = await expectSameAsV2( + () => + $getRoot().append($tl('outer', $tl('middle', $tl('inner', 'leaf')))), + () => { + const outer = $block([0]); + const item = outer.getFirstChildOrThrow() as ElementNode; + const middle = item.getFirstChildOrThrow(); + outer.insertBefore(middle); + outer.remove(); + }, + ); + expect(docJSON(r.author)).not.toContain('"tone":"outer"'); + expect(docJSON(r.author)).toContain('"tone":"middle"'); + expect(docJSON(r.author)).toContain('"tone":"inner"'); + }); + + test.each([ + ['outdent', [0, 1, 0, 0], 0], + ['indent', [0, 2], 1], + ] as const)('%s, levels of distinct fields', async (_name, path, indent) => { + await expectSameAsV2( + () => $getRoot().append($tl('out', 'a', $tl('in', 'x', 'y'), 'b')), + () => { + let node: LexicalNode = $getRoot(); + for (const i of path) { + node = (node as ElementNode).getChildAtIndex(i)!; + } + (node as ListItemNode).setIndent(indent); + }, + ); + }); + + test('a run of new items finds the item their group fields come from in linear time', async () => { + // Prepended: each finds the first stored item after the run. + const proto = Writer.prototype as unknown as { + storedItemAttrs: (...args: unknown[]) => unknown; + }; + const {storedItemAttrs} = proto; + const calls: number[] = []; + try { + for (const size of [32, 256]) { + const r = await room(V1, () => $getRoot().append($list('existing'))); + let n = 0; + proto.storedItemAttrs = function (this: unknown, ...args: unknown[]) { + n++; + return storedItemAttrs.apply(this, args); + }; + r.editor.update(() => + $block([0]).splice( + 0, + 0, + Array.from({length: size}, (_, i) => + $createListItemNode().append($createTextNode('new' + i)), + ), + ), + ); + await flush(); + proto.storedItemAttrs = storedItemAttrs; + expect(r.editor.errors).toEqual([]); + expect(docJSON(r.author).match(/"tone":"warm"/g)).toHaveLength(1); + calls.push(n); + } + } finally { + proto.storedItemAttrs = storedItemAttrs; + } + expect(calls[1]).toBeLessThan(calls[0] * 8 * 2); + }); + + test.fails( + 'limit: a link an older client replaces with another keeps the fields it does not know', + async () => { + // It cannot tell the new link from the old one edited, and keeps what + // it does not own (see "Older app versions" in y.md's limits). + await expectSameAsV2( + () => $getRoot().append($p('see ').append($link())), + () => { + const old = $block([0]).getChildAtIndex(1) as LinkNode; + // Of the client's own class for the type. + const next = $create(old.constructor as Klass) + .setURL('https://b.dev') + .append(...old.getChildren()); + old.replace(next); + }, + ); + }, + ); +}); + describe('S7-3: which stored names a client owns', () => { test('a node or marker attribute', () => { const fields = new Set(['src']); @@ -1324,6 +1434,181 @@ describe('S7-21: a read that throws on every attempt fails closed', () => { }); }); +type SerializedRequired = SerializedElementNode & {mode: string}; +/** + * A class whose `mode` its JSON always holds (its importJSON requires it), + * and older versions of it that reject a value a newer one writes, each in + * its own way. + */ +class RequiredV2 extends ElementNode { + __mode = 'a'; + static getType(): string { + return 'test_required'; + } + static clone(node: RequiredV2): RequiredV2 { + const copy = new RequiredV2(node.__key); + copy.__mode = node.__mode; + return copy; + } + static importJSON(json: SerializedRequired): RequiredV2 { + return new RequiredV2().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + const self = super.updateFromJSON(json); + self.__mode = json.mode; + return self; + } + exportJSON(): SerializedRequired { + return {...super.exportJSON(), mode: this.__mode, type: 'test_required'}; + } + createDOM(): HTMLElement { + throw new Error('headless'); + } + updateDOM(): boolean { + return false; + } +} +function requiredV1( + reject: (node: RequiredV2, json: SerializedRequired) => void, +): typeof RequiredV2 { + return class RequiredV1 extends RequiredV2 { + static getType(): string { + return 'test_required'; + } + static clone(node: RequiredV1): RequiredV1 { + const copy = new RequiredV1(node.__key); + copy.__mode = node.__mode; + return copy; + } + static importJSON(json: SerializedRequired): RequiredV1 { + return new RequiredV1().updateFromJSON(json); + } + updateFromJSON(json: LexicalUpdateJSON): this { + if (json.mode === 'a' || json.mode === 'b') { + return super.updateFromJSON(json); + } + reject(this, json as SerializedRequired); + throw new Error('unknown mode ' + json.mode); + } + }; +} + +describe('S7-21: what an older class does before it throws', () => { + const $content = (mode: string) => () => { + const box = $create(RequiredV2).append($p('inside')); + box.__mode = mode; + $getRoot().clear().append($p('FIRST'), box, $p('LAST')); + }; + const opened = async (v1: typeof RequiredV2, mode: string) => { + const a = new Peer(2, newNet() as never, [...NODES, RequiredV2]); + a.update($content(mode)); + a.binding.initialize(); + await flush(); + const b = new Peer(1, newNet() as never, [...NODES, v1]); + expect(() => { + Y.applyUpdate(b.doc, Y.encodeStateAsUpdate(a.doc)); + b.binding.initialize(); + }).not.toThrow(); + await flush(); + return b; + }; + + test('a fallback node does not go through the importJSON that rejected it', async () => { + const b = await opened( + requiredV1(() => {}), + 'c', + ); + expectUnreadable(b, 'test_required', null); + expect(b.describe()).toContain('"FIRST"'); + expect(b.describe()).toContain('"LAST"'); + }); + + test('what it throws need not be printable', async () => { + const v1 = class extends requiredV1(() => {}) { + static getType(): string { + return 'test_required'; + } + updateFromJSON(json: LexicalUpdateJSON): this { + if (json.mode !== 'a' && json.mode !== 'b') { + throw Object.create(null); + } + return super.updateFromJSON(json); + } + }; + const b = await opened(v1, 'c'); + expect(b.errors).toEqual([]); + expect(b.binding.status).toBe('readonly'); + }); + + test.each([ + [ + 'what it changed is undone', + (node: RequiredV2) => { + node.setFormat('right'); + }, + ], + [ + 'a change it made that Lexical cannot commit is undone', + (node: RequiredV2) => { + node.getWritable().__mode = 'c'; + }, + ], + ])('a node it updates: %s', async (_name, reject) => { + const v1 = class extends requiredV1(reject) { + static getType(): string { + return 'test_required'; + } + getTextContent(): string { + if (this.getLatest().__mode === 'c') { + throw new Error('cannot show mode c'); + } + return super.getTextContent(); + } + }; + const net = newNet(); + const a = new Peer(2, net as never, [...NODES, RequiredV2]); + const b = new Peer(1, net as never, [...NODES, v1]); + a.update($content('a')); + a.binding.initialize(); + await flush(); + const shown = b.describe(); + a.update(() => { + ($block([1]) as RequiredV2).getWritable().__mode = 'c'; + }); + await flush(); + expectUnreadable(b, 'test_required', shown); + expect(b.read(() => $block([1]).getFormatType())).toBe(''); + }); + + test('a reader that throws null fails closed too', async () => { + const a = new Peer(2, newNet() as never); + a.update(() => $getRoot().clear().append($p('hello'))); + a.binding.initialize(); + await flush(); + const b = new Peer(1, newNet() as never); + const proto = Reader.prototype as unknown as { + $readFull: (...args: unknown[]) => void; + }; + const {$readFull} = proto; + proto.$readFull = () => { + // eslint-disable-next-line no-throw-literal + throw null; + }; + try { + Y.applyUpdate(b.doc, Y.encodeStateAsUpdate(a.doc)); + b.binding.initialize(); + } finally { + proto.$readFull = $readFull; + } + await flush(); + expect(b.binding.status).toBe('readonly'); + const vector = Y.encodeStateVector(b.doc); + b.update(() => $getRoot().append($p('typed while read only'))); + await flush(); + expect(Y.encodeStateVector(b.doc)).toEqual(vector); + }); +}); + /** * `peer` opened the room read only for stored content of `type` its class * throws on, warned once, threw nothing, and shows `shown` (when given). @@ -1447,3 +1732,8 @@ describe('S7-3, S7-11, S7-21: a server agent with older classes', () => { ); }); }); + +// The data of the warning stays an interface an app can extend. +export interface AppUnknownNodeTypesData extends YUnknownNodeTypesData { + readonly requestId: string; +} diff --git a/packages/lexical-y/src/index.ts b/packages/lexical-y/src/index.ts index 51665fc30cc..97f791ea3d7 100644 --- a/packages/lexical-y/src/index.ts +++ b/packages/lexical-y/src/index.ts @@ -39,6 +39,7 @@ import { type YStorageMismatchData, type YStoredAs, type YUnknownNodeTypesData, + type YUnreadableContentData, type YUnsupportedConfigData, type YUnsupportedLayoutData, type YUnsupportedSlotsData, @@ -64,6 +65,7 @@ export type { YStorageMismatchData, YStoredAs, YUnknownNodeTypesData, + YUnreadableContentData, YUnsupportedConfigData, YUnsupportedLayoutData, YUnsupportedSlotsData, diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 854b62d6758..513701878e9 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -167,6 +167,18 @@ const REMOTE_TAGS = [ HISTORY_MERGE_TAG, DEFER_COMPOSITION_MERGE_TAG, ]; +/** A failure caught where any value may be thrown, `null` and `undefined` too. */ +type Failure = {readonly error: unknown} | null; + +/** `error`'s text for a warning; never throws, whatever value was thrown. */ +function errorText(error: unknown): string { + try { + return String(error instanceof Error ? error.message : error); + } catch { + return Object.prototype.toString.call(error); + } +} + /** The binding's refresh of derived group fields (see flushRefresh). */ const REFRESH_TAG = 'lexical-y-refresh'; /** The author's normalization of its own update (see normalizeUntransformed). */ @@ -1441,32 +1453,38 @@ export class YBinding { } this.commitPending(); const shown = this._editor.getEditorState(); - let error: unknown = null; - this._editor.update( - () => { - try { - const cursor = this.$cursorBeforeRead(); - const reader = new Reader(this.state); - for (const cs of list) { - const el = cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); - if ($isElementNode(el) && el.isAttached()) { - reader.$readFull(cs, el); + let failure = null as Failure; + try { + this._editor.update( + () => { + try { + const cursor = this.$cursorBeforeRead(); + const reader = new Reader(this.state); + for (const cs of list) { + const el = + cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); + if ($isElementNode(el) && el.isAttached()) { + reader.$readFull(cs, el); + } } + this.$selectionAfterRead(cursor); + this.$quietSelection(); + } catch (error) { + failure = {error}; } - this.$selectionAfterRead(cursor); - this.$quietSelection(); - } catch (e) { - error = e; - } - }, - { - discrete: true, - skipTransforms: true, - tag: [...REMOTE_TAGS, ...extraTags], - }, - ); - if (error !== null) { - this.recoverRead(error, shown); + }, + { + discrete: true, + skipTransforms: true, + tag: [...REMOTE_TAGS, ...extraTags], + }, + ); + } catch (error) { + // Thrown committing what was read. + failure = {error}; + } + if (failure !== null) { + this.recoverRead(failure.error, shown); } } @@ -1673,23 +1691,28 @@ export class YBinding { return; } const shown = this._editor.getEditorState(); - let error: unknown = null; - this._editor.update( - () => { - try { - this.$readAllInner(cursor, kept, deep); - } catch (e) { - error = e; - } - }, - { - discrete: true, - skipTransforms: true, - tag: REMOTE_TAGS, - }, - ); - if (error !== null) { - this.failClosed(error, shown); + let failure = null as Failure; + try { + this._editor.update( + () => { + try { + this.$readAllInner(cursor, kept, deep); + } catch (error) { + failure = {error}; + } + }, + { + discrete: true, + skipTransforms: true, + tag: REMOTE_TAGS, + }, + ); + } catch (error) { + // Thrown committing what was read. + failure = {error}; + } + if (failure !== null) { + this.failClosed(failure.error, shown); return; } this.checkUnknownTypes(); @@ -1896,7 +1919,7 @@ export class YBinding { cursor = this.stepCursor ?? cursor; } let rebuilt: ReadonlySet = new Set(); - let readError: unknown = null; + let readError = null as Failure; // What the editor shows before the read (see failClosed). const shown = this._editor.getEditorState(); // A re-read of a naming update's change is tagged as that update (see @@ -1904,7 +1927,7 @@ export class YBinding { const naming = this.refused.size > 0 && this.refusedNaming; const after = () => { if (readError !== null) { - this.recoverRead(readError, shown); + this.recoverRead(readError.error, shown); return; } this.checkUnknownTypes(); @@ -1919,38 +1942,46 @@ export class YBinding { // update of its own, see undo): the read is part of that update, and // what follows it waits for its commit. const nested = this._editor._updating; - this._editor.update( - () => { - try { - this.$readRemote( - state, - childItems, - attrKeys, - cursor, - !undoing, - r => { - rebuilt = r; - }, - tr, - ); - } catch (e) { - readError = e; - // The editor is rebuilt from the document right after (see - // recoverRead): a selection the failed read left on a removed - // node must not fail this update's commit too, which would drop - // an undo or redo the read was part of. - $setSelection(null); - } - if (nested) { - $onUpdate(() => this.guard(() => withPins(this._pins, after))); - } - }, - { - discrete: true, - skipTransforms: true, - tag: naming ? [...REMOTE_TAGS, BLOCK_ID_REPAIR_TAG] : REMOTE_TAGS, - }, - ); + try { + this._editor.update( + () => { + try { + this.$readRemote( + state, + childItems, + attrKeys, + cursor, + !undoing, + r => { + rebuilt = r; + }, + tr, + ); + } catch (error) { + readError = {error}; + // The editor is rebuilt from the document right after (see + // recoverRead): a selection the failed read left on a removed + // node must not fail this update's commit too, which would drop + // an undo or redo the read was part of. + $setSelection(null); + } + if (nested) { + $onUpdate(() => this.guard(() => withPins(this._pins, after))); + } + }, + { + discrete: true, + skipTransforms: true, + tag: naming ? [...REMOTE_TAGS, BLOCK_ID_REPAIR_TAG] : REMOTE_TAGS, + }, + ); + } catch (error) { + if (nested) { + throw error; + } + // Thrown committing what was read. + readError = {error}; + } if (!nested) { after(); } @@ -2046,9 +2077,7 @@ export class YBinding { } this.warn( RECOVERED_READ_WARNING, - `a remote change could not be read: ${String( - error instanceof Error ? error.message : error, - )}`, + `a remote change could not be read: ${errorText(error)}`, {error, reason: 'remote-change'}, ); } @@ -2069,9 +2098,7 @@ export class YBinding { this.unreadable = true; this.warn( UNKNOWN_NODE_TYPES_WARNING, - `the document cannot be read: ${String( - error instanceof Error ? error.message : error, - )}`, + `the document cannot be read: ${errorText(error)}`, {error, reason: 'unreadable', types: []}, ); this.setStatus('readonly'); @@ -2110,13 +2137,13 @@ export class YBinding { /** Replace the editor's content with a fresh read of the shared root. */ private rebuildFromDocument(): void { this.commitPending(); - let error: unknown = null; + let failure = null as Failure; this._editor.update( () => { try { this.$rebuild(); - } catch (e) { - error = e; + } catch (error) { + failure = {error}; } }, { @@ -2125,8 +2152,8 @@ export class YBinding { tag: REMOTE_TAGS, }, ); - if (error !== null) { - throw error; + if (failure !== null) { + throw failure.error; } this.checkUnknownTypes(); this.flushRefresh(true); @@ -3729,7 +3756,7 @@ export class YBinding { const cursor = this.$cursorBeforeRead(); // What the editor shows before the read (see failClosed). const shown = this._editor.getEditorState(); - let unreadable: unknown = null; + let unreadable = null as Failure; try { const reader = new Reader(this.state); for (const cs of list) { @@ -3745,17 +3772,15 @@ export class YBinding { this.$rebuild(); this.warn( RECOVERED_READ_WARNING, - `a change could not be read again: ${String( - e instanceof Error ? e.message : e, - )}`, + `a change could not be read again: ${errorText(e)}`, {error: e, reason: 'reread'}, ); - } catch (rebuildError) { - unreadable = rebuildError; + } catch (error) { + unreadable = {error}; } } if (unreadable !== null) { - const error = unreadable; + const {error} = unreadable; return () => this.failClosed(error, shown); } this.$quietSelection(); diff --git a/packages/lexical-y/src/internal/Read.ts b/packages/lexical-y/src/internal/Read.ts index 366c0c1e77e..397d046f735 100644 --- a/packages/lexical-y/src/internal/Read.ts +++ b/packages/lexical-y/src/internal/Read.ts @@ -15,6 +15,7 @@ import { type Node as YNode, } from '@y/y'; import { + $create, $createLineBreakNode, $createParagraphNode, $createTextNode, @@ -67,6 +68,7 @@ import { GROUP_KEY, groupItemFields, type GroupSpec, + hasOwn, INLINE_PREFIX, isUnsafeKey, joinSetMembers, @@ -299,24 +301,44 @@ export class Reader { } /** - * Run `read`, code of the class of stored content of `type` that reads the - * stored values (its importJSON or updateFromJSON, a NodeState parse). - * Where it throws, this editor cannot read that content, as with content - * of a type it does not register: the type is noted (see - * State#unreadable), the binding opens read only after the read, and - * `fallback` stands in (what is shown keeps the rest of the document). + * Run `read`, code of the class of stored content of `type` that builds a + * node from the stored values (its importJSON). Where it throws, this + * editor cannot read that content, as with content of a type it does not + * register: the type is noted (see State#unreadable), the binding opens + * read only after the read, and `fallback` stands in, so that what is + * shown keeps the rest of the document. */ private $readStored(type: string, read: () => T, fallback: () => T): T { try { return read(); } catch (error) { - if (!this.state.unreadable.has(type)) { - this.state.unreadable.set(type, error); - } + this.noteUnreadable(type, error); return fallback(); } } + /** + * Run `update`, code of `node`'s class that gives it stored values (its + * updateFromJSON, a NodeState parse). Where it throws, as in $readStored, + * and `node` keeps what it held before: what the class changed before it + * threw is undone. + */ + private $updateStored(node: LexicalNode, update: () => void): void { + const was = {...node.getLatest()} as Record; + try { + update(); + } catch (error) { + this.noteUnreadable(node.getType(), error); + $restoreNode(node, was); + } + } + + private noteUnreadable(type: string, error: unknown): void { + if (!this.state.unreadable.has(type)) { + this.state.unreadable.set(type, error); + } + } + // ------------------------------------------------------------------------- // Entry points @@ -1661,7 +1683,7 @@ export class Reader { type, version: 1, } as unknown as SerializedLexicalNode) as ElementNode, - () => $parseStoredDefaults(type, true) as ElementNode, + () => $storedDefault(type) as ElementNode, ); $setTextFields(wrapper, clean); } else if ( @@ -1676,14 +1698,12 @@ export class Reader { json.$ = clean.$ || {}; json.type = wrapper.getType(); const node = wrapper; - wrapper = this.$readStored( - node.getType(), - () => - node.updateFromJSON( - json as Parameters[0], - ), - () => node, - ); + this.$updateStored(node, () => { + node.updateFromJSON( + json as Parameters[0], + ); + }); + wrapper = node.getLatest(); } $setTextFields(wrapper, clean); $detachFromKids(wrapper, [g]); @@ -1744,14 +1764,11 @@ export class Reader { json.$ = fields.$ || {}; json.type = reused.getType(); const group = reused; - this.$readStored( - group.getType(), - () => - group.updateFromJSON( - json as Parameters[0], - ), - () => group, - ); + this.$updateStored(group, () => { + group.updateFromJSON( + json as Parameters[0], + ); + }); $setTextFields(reused, fields); } this.state.ids.noteStoredId(reused.getKey(), storedId); @@ -1766,7 +1783,7 @@ export class Reader { version: 1, ...fields, } as unknown as SerializedLexicalNode) as ElementNode, - () => $parseStoredDefaults(item.gtype, true) as ElementNode, + () => $storedDefault(item.gtype) as ElementNode, ); $setTextFields(created, fields); this.state.ids.noteStoredId(created.getKey(), storedId); @@ -1869,16 +1886,13 @@ export class Reader { json.textStyle = ''; } json.type = node.getType(); - this.$readStored( - node.getType(), - () => - $setStateInOrder( - node.updateFromJSON( - json as Parameters[0], - ), - state, + this.$updateStored(node, () => + $setStateInOrder( + node.updateFromJSON( + json as Parameters[0], ), - () => undefined, + state, + ), ); } @@ -1925,11 +1939,7 @@ export class Reader { owned($nodeAttrs(node, holes, false)) !== owned(attrs) || holes.length > 0; if (differs) { - this.$readStored( - node.getType(), - () => $applyStoredAttrs(node, attrs), - () => undefined, - ); + this.$updateStored(node, () => $applyStoredAttrs(node, attrs)); if ( $isElementNode(node) && getGroupSpec(klassOf(node.getLatest())) !== null @@ -2025,7 +2035,7 @@ export class Reader { const node = this.$readStored( type, () => $parseSerializedNode(json as unknown as SerializedLexicalNode), - () => $parseStoredDefaults(type, isElement), + () => $storedDefault(type), ); this.state.mapNode(node.getKey(), y); // It shows the id it stores: in the index of who stores what. @@ -2353,11 +2363,10 @@ export class Reader { } if (foreign || currentSig !== piece.tsig) { const text = node; - node = this.$readStored( - text.getType(), - () => $applyJSON(text, json), - () => text, - ); + this.$updateStored(text, () => { + $applyJSON(text, json); + }); + node = text.getLatest(); } if (node.getTextContent() !== piece.text) { node = node.setTextContent(piece.text); @@ -2389,17 +2398,14 @@ export class Reader { br = br.getWritable(); const info = state.editor._nodes.get(br.getType()); const writable = br; - this.$readStored( - br.getType(), - () => - $getWritableNodeState(writable).updateFromJSON( - joinSetMembers( - brState, - info && info.sharedNodeState.sharedConfigMap, - info && learnedSetKeys(info.sharedNodeState), - ), + this.$updateStored(writable, () => + $getWritableNodeState(writable).updateFromJSON( + joinSetMembers( + brState, + info && info.sharedNodeState.sharedConfigMap, + info && learnedSetKeys(info.sharedNodeState), ), - () => undefined, + ), ); } push(br); @@ -2484,18 +2490,14 @@ export class Reader { delete json.textFormat; delete json.textStyle; Object.assign(json, w.json); - return this.$readStored( - w.type, - () => { - const writable = reused.updateFromJSON( - json as Parameters[0], - ); - $setTextFields(writable, w.json); - $setStateInOrder(writable, (w.json.$ as Attrs) || {}); - return writable; - }, - () => reused, - ); + this.$updateStored(reused, () => { + const writable = reused.updateFromJSON( + json as Parameters[0], + ); + $setTextFields(writable, w.json); + $setStateInOrder(writable, (w.json.$ as Attrs) || {}); + }); + return reused.getLatest(); } return reused; } @@ -2506,7 +2508,7 @@ export class Reader { ...w.json, children: [], } as unknown as SerializedLexicalNode) as ElementNode, - () => $parseStoredDefaults(w.type, true) as ElementNode, + () => $storedDefault(w.type) as ElementNode, ); } @@ -2593,15 +2595,40 @@ type GroupItemInfo = { }; /** - * A node of `type` with no stored values: what a read shows for stored - * content of `type` that its class throws on (see Reader#$readStored). + * A new node of `type`'s class, as its constructor makes it: what a read + * shows for stored content of `type` that its class throws on (see + * Reader#$readStored). Not through the class's importJSON, which may reject + * the values a node it exports always has. + */ +function $storedDefault(type: string): LexicalNode { + const info = $getEditor()._nodes.get(type); + if (info === undefined) { + throw new Error(`@lexical/y: no node class of type ${type}`); + } + return $create(info.klass); +} + +/** + * Give `node` back the own properties `was` held (see Reader#$updateStored): + * what its class changed before it threw is undone. Nothing is written where + * it changed nothing. */ -function $parseStoredDefaults(type: string, element: boolean): LexicalNode { - return $parseSerializedNode({ - type, - version: 1, - ...(element ? {children: []} : {}), - } as unknown as SerializedLexicalNode); +function $restoreNode(node: LexicalNode, was: Record): void { + const latest = node.getLatest() as unknown as Record; + const keys = Object.keys(latest); + if ( + keys.length === Object.keys(was).length && + keys.every(k => hasOwn(was, k) && latest[k] === was[k]) + ) { + return; + } + const writable = node.getWritable() as unknown as Record; + for (const k of Object.keys(writable)) { + if (!hasOwn(was, k)) { + delete writable[k]; + } + } + Object.assign(writable, was); } /** diff --git a/packages/lexical-y/src/internal/Schema.ts b/packages/lexical-y/src/internal/Schema.ts index cba89bacb52..cb325fe9a60 100644 --- a/packages/lexical-y/src/internal/Schema.ts +++ b/packages/lexical-y/src/internal/Schema.ts @@ -112,7 +112,8 @@ export function formatFamily(key: string): string { return key; } -const hasOwn = (obj: object, key: string) => +/** Whether `obj` has an own property `key`. */ +export const hasOwn = (obj: object, key: string): boolean => Object.prototype.hasOwnProperty.call(obj, key); /** `T` without `K`, per member of a union. */ diff --git a/packages/lexical-y/src/internal/Warnings.ts b/packages/lexical-y/src/internal/Warnings.ts index 0d587757140..100a4d681dd 100644 --- a/packages/lexical-y/src/internal/Warnings.ts +++ b/packages/lexical-y/src/internal/Warnings.ts @@ -17,8 +17,9 @@ import {LexicalWarning} from 'lexical'; */ export const UNSUPPORTED_LAYOUT_WARNING = '@lexical/y/unsupported-layout'; /** - * @experimental The document has node types the editor cannot build: read - * only. `data`: YUnknownNodeTypesData. + * @experimental The document has node types the editor cannot build, or + * stored content its classes throw on: read only. `data`: + * YUnknownNodeTypesData, or YUnreadableContentData. */ export const UNKNOWN_NODE_TYPES_WARNING = '@lexical/y/unknown-node-types'; /** @@ -140,29 +141,29 @@ export type YStorageMismatchData = /** * @experimental `data` of an `UNKNOWN_NODE_TYPES_WARNING` (the binding is read - * only). + * only): types the editor does not register (`'unregistered'`), or slots the + * document holds that are not a block node (`'malformed-slot'`: the slot's + * key, or the type of the inline node in it). */ -export type YUnknownNodeTypesData = - /** - * Types the editor does not register (`'unregistered'`), or slots the - * document holds that are not a block node (`'malformed-slot'`: the slot's - * key, or the type of the inline node in it). - */ - | { - readonly reason: 'unregistered' | 'malformed-slot'; - readonly types: readonly string[]; - } - /** - * The editor's classes throw `error` on stored content (of `types`, where - * the error came from a class reading content of a type), on a read and on - * a fresh read of the document after it. The editor keeps what it showed - * before, and reads nothing more. - */ - | { - readonly reason: 'unreadable'; - readonly types: readonly string[]; - readonly error: unknown; - }; +export interface YUnknownNodeTypesData { + readonly reason: 'unregistered' | 'malformed-slot'; + readonly types: readonly string[]; +} + +/** + * @experimental `data` of an `UNKNOWN_NODE_TYPES_WARNING` whose `reason` is + * `'unreadable'` (the binding is read only): stored content the editor's + * classes throw on when they read it (its importJSON or updateFromJSON, a + * NodeState parse), of `types`, with the first `error` thrown. The editor + * shows the rest of the document. `types` is empty, and the editor keeps + * what it showed before and reads nothing more, where a read failed without + * a class to name (it threw again on a fresh read of the document). + */ +export interface YUnreadableContentData { + readonly reason: 'unreadable'; + readonly types: readonly string[]; + readonly error: unknown; +} /** * @experimental `data` of an `UNSUPPORTED_SLOTS_WARNING` (the change is not @@ -273,7 +274,7 @@ export interface YUnsyncedValueData { /** The `data` of each code. */ interface YWarningDataByCode { [UNSUPPORTED_LAYOUT_WARNING]: YUnsupportedLayoutData; - [UNKNOWN_NODE_TYPES_WARNING]: YUnknownNodeTypesData; + [UNKNOWN_NODE_TYPES_WARNING]: YUnknownNodeTypesData | YUnreadableContentData; [STORAGE_MISMATCH_WARNING]: YStorageMismatchData; [SELF_CHECK_WARNING]: YSelfCheckData; [RECOVERED_READ_WARNING]: YRecoveredReadData; diff --git a/packages/lexical-y/src/internal/Write.ts b/packages/lexical-y/src/internal/Write.ts index 5f9a425d33c..59fd73761b9 100644 --- a/packages/lexical-y/src/internal/Write.ts +++ b/packages/lexical-y/src/internal/Write.ts @@ -417,8 +417,9 @@ export class Writer { * where it does not store the type as one (see wrapperFieldsOf). */ readonly wrapperFields: (type: string) => OwnedFields | null; - /** Per item, see $storedItemAtOrBefore. */ - private readonly groupReferences = new Map(); + /** Per item, the stored item found from it (see $storedItemFrom). */ + private readonly storedBefore = new Map(); + private readonly storedAfter = new Map(); constructor( readonly state: State, readonly roles: Roles, @@ -4032,14 +4033,21 @@ export class Writer { } } }; - keepOuter(current[OUTER_KEY], owners); + keepOuter( + line === null + ? current[OUTER_KEY] + : this.$alignedOuter(line, line, current[OUTER_KEY]), + owners, + ); if ( line !== null && GROUP_KEY in desired && current[GROUP_KEY] === undefined ) { - const ref = this.$groupReference(line, desired[GROUP_KEY]); - if (ref !== null) { + const item = this.$groupReference(line, desired[GROUP_KEY]); + const ref = + item === null ? null : this.storedItemAttrs(item, desired[GROUP_KEY]); + if (item !== null && ref !== null) { // Owned as at the other item, whose group has the item's type. const of = this.markerOwners(desired, ref); for (const k in ref) { @@ -4051,61 +4059,159 @@ export class Writer { want = {...want, [k]: ref[k]}; } } - keepOuter(ref[OUTER_KEY], of); + keepOuter(this.$alignedOuter(line, item, ref[OUTER_KEY]), of); } } return want; } /** - * The stored marker attributes of the item of `line`'s group (of type - * `gtype`) nearest before it, else after it, that this write did not - * make, or null. Its neighbor, not any item: a client that does not own a - * group field shows one group where another client shows two that differ - * in it, and the item goes into the one it is next to. + * `stored`, the outer levels (OUTER_KEY) the marker of `item` stores, + * aligned with the outer levels `line` is written with: at each, the + * level stored for the same group (see $outerGroupKeys), or none. Matched + * by group, not by depth: an edit that removes or adds an outer group + * shifts the depths, and a level keeps the members of its own group only. + * Undefined where the stored levels cannot be matched to groups. + */ + private $alignedOuter( + line: LexicalNode, + item: LexicalNode, + stored: unknown, + ): Attrs[] | undefined { + if (!Array.isArray(stored)) { + return undefined; + } + // The groups `item`'s marker was written for: those before this update, + // unless this write already wrote its outer levels. + const unit = this.state.unitByKey.get(item.getKey()); + const attr = + unit === undefined || unit.end === null + ? undefined + : typeOf(unit.end)._map.get(OUTER_KEY); + const was = + attr !== undefined && this.tr.insertSet.hasId(attr.id) + ? this.$outerGroupKeys(item) + : this.outerGroupKeysBefore(item.getKey()); + if (was === null || was.length !== stored.length) { + return undefined; + } + return this.$outerGroupKeys(line).map(key => { + const at = was.indexOf(key); + return at === -1 ? undefined : stored[at]; + }); + } + + /** + * The keys of the groups whose levels the marker of `line` stores as its + * outer levels (OUTER_KEY), outermost first: the groups that hold its + * group through level items, as $lineDesired finds them. + */ + private $outerGroupKeys(line: LexicalNode): NodeKey[] { + const keys: NodeKey[] = []; + for (let g = line.getParent(); g !== null; ) { + const levelItem = g.getParent(); + if (levelItem === null || this.roles.$role(levelItem) !== 'level') { + break; + } + g = levelItem.getParent(); + if (g !== null) { + keys.push(g.getKey()); + } + } + return keys.reverse(); + } + + /** + * $outerGroupKeys for the node of `key` in the editor state before this + * update (a level item there holds only the nested group, see + * $nestedGroupOf), or null where it had no such node. */ - private $groupReference(line: LexicalNode, gtype: unknown): Attrs | null { + private outerGroupKeysBefore(key: NodeKey): NodeKey[] | null { + const map = this.prevState._nodeMap; + const node = map.get(key); + if (node === undefined) { + return null; + } + const parentOf = (n: LexicalNode) => + n.__parent === null ? undefined : map.get(n.__parent); + const keys: NodeKey[] = []; + for (let g = parentOf(node); g !== undefined; ) { + const levelItem = parentOf(g) as ElementNode | undefined; + const outer = levelItem === undefined ? undefined : parentOf(levelItem); + const spec = outer === undefined ? null : getGroupSpec(klassOf(outer)); + if ( + outer === undefined || + levelItem === undefined || + spec === null || + !spec.nested || + klassOf(g) !== spec.klass || + levelItem.__first !== g.__key || + levelItem.__last !== g.__key + ) { + break; + } + keys.push(outer.__key); + g = outer; + } + return keys.reverse(); + } + + /** + * The item of `line`'s group (of type `gtype`) nearest before it, else + * after it, whose stored marker this write did not make (see + * storedItemAttrs), or null. Its neighbor, not any item: a client that + * does not own a group field shows one group where another client shows + * two that differ in it, and the item goes into the one it is next to. + */ + private $groupReference( + line: LexicalNode, + gtype: unknown, + ): LexicalNode | null { const before = line.getPreviousSibling(); const ref = - before === null ? null : this.$storedItemAtOrBefore(before, gtype); + before === null + ? null + : this.$storedItemFrom(before, gtype, this.storedBefore, n => + n.getPreviousSibling(), + ); if (ref !== null) { return ref; } - for (let c = line.getNextSibling(); c !== null; c = c.getNextSibling()) { - const attrs = this.storedItemAttrs(c, gtype); - if (attrs !== null) { - return attrs; - } - } - return null; + const after = line.getNextSibling(); + return after === null + ? null + : this.$storedItemFrom(after, gtype, this.storedAfter, n => + n.getNextSibling(), + ); } /** - * The stored marker attributes of `item` or of the nearest item before it - * that this write did not make (see $groupReference), or null. Noted for - * each item walked: new items written in order find theirs at once. + * `item`, or the nearest item from it on (`step`), whose stored marker + * this write did not make, or null. Noted (`noted`) for each item walked: + * a run of new items finds it at once. */ - private $storedItemAtOrBefore( + private $storedItemFrom( item: LexicalNode, gtype: unknown, - ): Attrs | null { + noted: Map, + step: (node: LexicalNode) => LexicalNode | null, + ): LexicalNode | null { const walked: NodeKey[] = []; - let ref: Attrs | null = null; - for (let c: LexicalNode | null = item; c !== null; ) { - const noted = this.groupReferences.get(c.getKey()); - if (noted !== undefined) { - ref = noted; + let ref: LexicalNode | null = null; + for (let c: LexicalNode | null = item; c !== null; c = step(c)) { + const known = noted.get(c.getKey()); + if (known !== undefined) { + ref = known; break; } walked.push(c.getKey()); - ref = this.storedItemAttrs(c, gtype); - if (ref !== null) { + if (this.storedItemAttrs(c, gtype) !== null) { + ref = c; break; } - c = c.getPreviousSibling(); } for (const key of walked) { - this.groupReferences.set(key, ref); + noted.set(key, ref); } return ref; } From 5bad5066c3f8fddd5a2cfc86d37a6b03a91a1ab9 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 22:21:11 -0400 Subject: [PATCH 04/12] [lexical-y] Bug Fix: Route every read through one fail-closed entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Follow-up from a cross-review of the open `@lexical/y` pull requests. - **One guarded read.** Each read of the binding wrapped its update in the same try/update/try/catch so that it could recover or fail closed, in about five copies. Releasing what a composition held back (`flushHidden`) had none: a reader throw reached the editor's `onError` and left the held text missing from an active editor. The re-read of what a follow-up changed (`rereadRefused`) had no catch for a throw while committing. `YBinding#guardedRead` is now the one entry point every read goes through: - it reads nothing once the binding failed closed; - a throw, of the read or of committing it, goes to the read's recovery (`recoverRead`, or `failClosed` for a read of the whole document) with what the editor showed before, and never reaches Yjs, a provider or `initialize()`; - what follows the read runs after its commit, also where the read is nested in another update. A release of a hold still clears the hold before its read, so the read shows what it held; a recovery reads the whole document again. - **`errorText` never throws.** Its fallback, `Object.prototype.toString`, can throw too (a throwing `Symbol.toStringTag`, a revoked proxy); it is a fixed text now. - **The ownership rule in two clauses.** A write changes the value a stored name gives content only where its client owns the name; format items that keep every name's value around a write (negations and restores around inserted content, run ends, the copies an undo brings back) carry any name. The README's wire format says the same. ## Test plan New cases in `packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: a reader throw while releasing what a composition held back, a re-read of what a follow-up changed that throws once and on every read, and a thrown value whose text cannot be read. ### Before On the previous commit with this test file (the describe blocks that pass are left out), `./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts`: ``` ❯ packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts (59 tests | 2 failed) ❯ S7-21 (MF-2): every read goes through the one fail-closed path (4) × releasing what a composition held back ✓ a re-read of what a follow-up changed that throws once: read again ✓ a re-read of what a follow-up changed that throws on every read: kept as shown, read only × a thrown value whose text cannot be read Tests 2 failed | 55 passed | 2 expected fail (59) ``` ``` FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21 (MF-2): every read goes through the one fail-closed path > releasing what a composition held back AssertionError: expected [ Error: simulated reader failure ] to deeply equal [] FAIL |unit| packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts > S7-21 (MF-2): every read goes through the one fail-closed path > a thrown value whose text cannot be read AssertionError: expected [Function] to not throw an error but 'Error: no tag' was thrown ``` The two re-read cases pass there too: they keep what the earlier code did. ### After ``` $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts Tests 57 passed | 2 expected fail (59) $ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/ Tests 4731 passed | 14 expected fail | 2 skipped (4747) ``` `pnpm run ci-check` passes. Sessions without version skew still store the same bytes as `main`: over the whole `@lexical/y` unit suite plus 800 fuzz seeds, every deterministic session is byte-identical. Not run: browser and e2e suites. --- packages/lexical-y/README.md | 18 +- .../unit/regressions/SchemaSkew.test.ts | 159 ++++++++ packages/lexical-y/src/internal/Binding.ts | 386 +++++++++--------- packages/lexical-y/src/internal/Schema.ts | 21 +- 4 files changed, 368 insertions(+), 216 deletions(-) diff --git a/packages/lexical-y/README.md b/packages/lexical-y/README.md index a8b201911bc..46efc81e4fe 100644 --- a/packages/lexical-y/README.md +++ b/packages/lexical-y/README.md @@ -174,14 +174,16 @@ of set-valued NodeState; layout 3 (planned, #9449) adds move records. `\u0000` followed by the value's JSON with sorted keys. A string that itself starts with `\u0000` is stored the same way (as the JSON of that string), so every member reads back with its type and value. -- **Names a client does not own:** a writer sets and deletes only the names - it owns, the binding's own and the fields its classes declare for the - content's type (all of them where the write changes that type), its text - formats and NodeState, and keeps every other name as stored: a field a - newer version of a class added, a text format a newer Lexical added, a key - of a later layout. Inserted text takes the names it does not own from the - text it continues (a field only from text of its own type), and a new - group item the group fields another item of its group stores. A reader +- **Names a client does not own:** a writer changes the value a name gives + content only for the names it owns, the binding's own and the fields its + classes declare for the content's type (all of them where the write + changes that type), its text formats and NodeState, and keeps every other + name's value as stored: a field a newer version of a class added, a text + format a newer Lexical added, a key of a later layout. Format items that + only keep every name's value where it is (around inserted content, at the + end of a run) carry any name. Inserted text takes the names it does not + own from the text it continues (a field only from text of its own type), + and a new group item the group fields of the item next to it. A reader reads only the names it owns. - **Defaults are left out:** a field or NodeState value equal to the one a new instance of the class has (its `$config` default) is not stored, so a diff --git a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts index 135bfb340fd..69d577de4bf 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts @@ -39,11 +39,13 @@ import { $create, $createParagraphNode, $createTextNode, + $getNodeByKey, $getRoot, $getSlot, $getState, $isElementNode, $isTextNode, + $setCompositionKey, $setSlot, $setState, arraySetValue, @@ -84,12 +86,14 @@ import {Writer} from '../../../internal/Write'; import { $block, $createBoxNode, + $locateIn, $p, $selectIn, flush, freshReader, NODES, Peer, + setup, shape, } from '../../utils/harness'; @@ -1609,6 +1613,161 @@ describe('S7-21: what an older class does before it throws', () => { }); }); +describe('S7-21 (MF-2): every read goes through the one fail-closed path', () => { + /** Make `method` of the reader throw `error` once. */ + const throwOnce = (method: string, error: unknown) => { + const proto = Reader.prototype as unknown as Record< + string, + (...args: unknown[]) => unknown + >; + const original = proto[method]; + proto[method] = function (this: unknown, ...args: unknown[]) { + proto[method] = original; + throw error; + }; + return () => { + proto[method] = original; + }; + }; + + test('releasing what a composition held back', async () => { + const t = await setup(() => $getRoot().append($p('alpha beta gamma'))); + let key = ''; + t.a.update(() => { + const [node, offset] = $locateIn($block([0]), 16); + node.select(offset, offset); + $setCompositionKey(node.getKey()); + key = node.getKey(); + }); + t.a.update(() => { + const node = $getNodeByKey(key) as TextNode; + node.setTextContent('alpha beta gammaか'); + node.select(17, 17); + }); + await t.sync(); + // Typed into the composed line: held back on the composing client. + t.b.update(() => { + const [node] = $locateIn($block([0]), 0); + (node as TextNode).spliceText(0, 0, 'XY'); + }); + await t.sync(); + const restore = throwOnce( + '$refreshComposed', + new Error('simulated reader failure'), + ); + try { + expect(() => + t.a.update(() => { + const node = $getNodeByKey(key) as TextNode; + node.setTextContent('alpha beta gamma漢'); + node.select(17, 17); + $setCompositionKey(null); + }), + ).not.toThrow(); + await flush(); + await t.sync(); + } finally { + restore(); + } + expect(t.a.errors).toEqual([]); + expect(t.a.warnings).toEqual([ + 'a remote change could not be read: simulated reader failure', + ]); + expect(t.a.binding.status).toBe('active'); + expect(t.a.describe()).toBe(t.b.describe()); + expect(t.a.describe()).toContain('XY'); + }); + + test.each([ + ['once: read again', 1], + ['on every read: kept as shown, read only', Infinity], + ])( + 'a re-read of what a follow-up changed that throws %s', + async (_name, times) => { + const net = newNet(); + const a = new Peer(2, net as never); + const b = new Peer(1, net as never); + a.update(() => $getRoot().clear().append($p('hello'))); + a.binding.initialize(); + await flush(); + const shown = b.describe(); + // As a follow-up the binding does not write leaves it (see + // rereadRefused): the root's container is read again, as the + // binding's own work. + const binding = b.binding as unknown as { + _root: unknown; + guard: (fn: () => void) => void; + refused: Set; + rereadRefused: () => void; + state: {containers: Map}; + }; + binding.refused.add(binding.state.containers.get(binding._root)); + const proto = Reader.prototype as unknown as { + $readFull: (...args: unknown[]) => void; + }; + const {$readFull} = proto; + let left = times; + proto.$readFull = function (this: unknown, ...args: unknown[]) { + if (left-- > 0) { + throw new Error('simulated reader failure'); + } + return $readFull.apply(this, args); + }; + try { + expect(() => + binding.guard(() => binding.rereadRefused()), + ).not.toThrow(); + await flush(); + } finally { + proto.$readFull = $readFull; + } + expect(b.errors).toEqual([]); + expect(b.describe()).toBe(shown); + if (times === 1) { + expect(b.warnings).toEqual([ + 'a change could not be read again: simulated reader failure', + ]); + expect(b.binding.status).toBe('active'); + } else { + expect(b.warnings).toEqual([ + 'the document cannot be read: simulated reader failure', + ]); + expect(b.binding.status).toBe('readonly'); + } + }, + ); + + test('a thrown value whose text cannot be read', async () => { + // Neither String() nor Object.prototype.toString can read it. + const unprintable = Object.create(null, { + [Symbol.toStringTag]: { + get() { + throw new Error('no tag'); + }, + }, + }); + const a = new Peer(2, newNet() as never); + a.update(() => $getRoot().clear().append($p('hello'))); + a.binding.initialize(); + await flush(); + const b = new Peer(1, newNet() as never); + const restore = throwOnce('$readFull', unprintable); + try { + expect(() => { + Y.applyUpdate(b.doc, Y.encodeStateAsUpdate(a.doc)); + b.binding.initialize(); + }).not.toThrow(); + } finally { + restore(); + } + await flush(); + expect(b.binding.status).toBe('readonly'); + expect(b.warnings).toEqual([ + 'the document cannot be read: a value that cannot be shown as text', + ]); + }); +}); + /** * `peer` opened the room read only for stored content of `type` its class * throws on, warned once, threw nothing, and shows `shown` (when given). diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 513701878e9..853faa1202d 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -130,6 +130,7 @@ import { UNSUPPORTED_CONFIG_WARNING, UNSUPPORTED_LAYOUT_WARNING, UNSUPPORTED_SLOTS_WARNING, + type YRecoveredReadData, type YStorageMismatchData, type YWarningCode, type YWarningData, @@ -170,12 +171,28 @@ const REMOTE_TAGS = [ /** A failure caught where any value may be thrown, `null` and `undefined` too. */ type Failure = {readonly error: unknown} | null; +/** A read of the shared document into the editor (see YBinding#guardedRead). */ +type GuardedRead = { + /** The read, inside the update. */ + readonly $read: () => void; + /** + * Where a throw goes, of the read or of committing it, with what the + * editor showed before the read: recoverRead, which reads the whole + * document again, or failClosed for a read of the whole document. + */ + readonly recover: (error: unknown, shown: EditorState) => void; + /** Runs once the read has committed and nothing threw. */ + readonly after?: () => void; + /** The update's tags. */ + readonly tag?: string | string[]; +}; + /** `error`'s text for a warning; never throws, whatever value was thrown. */ function errorText(error: unknown): string { try { return String(error instanceof Error ? error.message : error); } catch { - return Object.prototype.toString.call(error); + return 'a value that cannot be shown as text'; } } @@ -1448,44 +1465,23 @@ export class YBinding { } private rereadContainers(list: CS[], extraTags: string[] = []): void { - if (this.unreadable) { - return; - } this.commitPending(); - const shown = this._editor.getEditorState(); - let failure = null as Failure; - try { - this._editor.update( - () => { - try { - const cursor = this.$cursorBeforeRead(); - const reader = new Reader(this.state); - for (const cs of list) { - const el = - cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); - if ($isElementNode(el) && el.isAttached()) { - reader.$readFull(cs, el); - } - } - this.$selectionAfterRead(cursor); - this.$quietSelection(); - } catch (error) { - failure = {error}; + this.guardedRead({ + $read: () => { + const cursor = this.$cursorBeforeRead(); + const reader = new Reader(this.state); + for (const cs of list) { + const el = cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); + if ($isElementNode(el) && el.isAttached()) { + reader.$readFull(cs, el); } - }, - { - discrete: true, - skipTransforms: true, - tag: [...REMOTE_TAGS, ...extraTags], - }, - ); - } catch (error) { - // Thrown committing what was read. - failure = {error}; - } - if (failure !== null) { - this.recoverRead(failure.error, shown); - } + } + this.$selectionAfterRead(cursor); + this.$quietSelection(); + }, + recover: (error, shown) => this.recoverRead(error, shown), + tag: [...REMOTE_TAGS, ...extraTags], + }); } /** @@ -1687,36 +1683,16 @@ export class YBinding { kept: SelectionState | null = null, deep = false, ): void { - if (this.unreadable) { - return; - } - const shown = this._editor.getEditorState(); - let failure = null as Failure; - try { - this._editor.update( - () => { - try { - this.$readAllInner(cursor, kept, deep); - } catch (error) { - failure = {error}; - } - }, - { - discrete: true, - skipTransforms: true, - tag: REMOTE_TAGS, - }, - ); - } catch (error) { - // Thrown committing what was read. - failure = {error}; - } - if (failure !== null) { - this.failClosed(failure.error, shown); - return; - } - this.checkUnknownTypes(); - this.flushRefresh(true); + this.guardedRead({ + $read: () => this.$readAllInner(cursor, kept, deep), + after: () => { + this.checkUnknownTypes(); + this.flushRefresh(true); + }, + // The whole document: read again, it would throw again. + recover: (error, shown) => this.failClosed(error, shown), + tag: REMOTE_TAGS, + }); } private $readAllInner( @@ -1919,72 +1895,34 @@ export class YBinding { cursor = this.stepCursor ?? cursor; } let rebuilt: ReadonlySet = new Set(); - let readError = null as Failure; - // What the editor shows before the read (see failClosed). - const shown = this._editor.getEditorState(); // A re-read of a naming update's change is tagged as that update (see // rereadRefused). const naming = this.refused.size > 0 && this.refusedNaming; - const after = () => { - if (readError !== null) { - this.recoverRead(readError.error, shown); - return; - } - this.checkUnknownTypes(); - this.checkRead(rebuilt); - this.flushRefresh(true); - // Invariant: the binding's own pending updates run once the editor - // shows the remote change, never before. Why: a write before that would - // take what the editor does not show yet for deleted. - this.runPending(); - }; - // Applied while the editor is updating (an undo the binding runs in an - // update of its own, see undo): the read is part of that update, and - // what follows it waits for its commit. - const nested = this._editor._updating; - try { - this._editor.update( - () => { - try { - this.$readRemote( - state, - childItems, - attrKeys, - cursor, - !undoing, - r => { - rebuilt = r; - }, - tr, - ); - } catch (error) { - readError = {error}; - // The editor is rebuilt from the document right after (see - // recoverRead): a selection the failed read left on a removed - // node must not fail this update's commit too, which would drop - // an undo or redo the read was part of. - $setSelection(null); - } - if (nested) { - $onUpdate(() => this.guard(() => withPins(this._pins, after))); - } - }, - { - discrete: true, - skipTransforms: true, - tag: naming ? [...REMOTE_TAGS, BLOCK_ID_REPAIR_TAG] : REMOTE_TAGS, - }, - ); - } catch (error) { - if (nested) { - throw error; - } - // Thrown committing what was read. - readError = {error}; - } - if (!nested) { - after(); - } + this.guardedRead({ + $read: () => + this.$readRemote( + state, + childItems, + attrKeys, + cursor, + !undoing, + r => { + rebuilt = r; + }, + tr, + ), + after: () => { + this.checkUnknownTypes(); + this.checkRead(rebuilt); + this.flushRefresh(true); + // Invariant: the binding's own pending updates run once the editor + // shows the remote change, never before. Why: a write before that + // would take what the editor does not show yet for deleted. + this.runPending(); + }, + recover: (error, shown) => this.recoverRead(error, shown), + tag: naming ? [...REMOTE_TAGS, BLOCK_ID_REPAIR_TAG] : REMOTE_TAGS, + }); } private $readRemote( @@ -2068,7 +2006,11 @@ export class YBinding { * Where the rebuild throws too, the editor's classes cannot read the * document: the binding fails closed, back to `shown` (see failClosed). */ - private recoverRead(error: unknown, shown: EditorState): void { + private recoverRead( + error: unknown, + shown: EditorState, + reason: YRecoveredReadData['reason'] = 'remote-change', + ): void { try { this.rebuildFromDocument(); } catch (e) { @@ -2077,11 +2019,79 @@ export class YBinding { } this.warn( RECOVERED_READ_WARNING, - `a remote change could not be read: ${errorText(error)}`, - {error, reason: 'remote-change'}, + `${ + reason === 'reread' + ? 'a change could not be read again' + : 'a remote change could not be read' + }: ${errorText(error)}`, + {error, reason}, ); } + /** + * Read the shared document into the editor: `read.$read`, in an update. + * The one entry point for every read of the binding: a read that does not + * go through it can neither recover nor fail closed. + * + * - Nothing is read once the binding failed closed (see failClosed). + * - A throw, of the read or of committing it, never reaches Yjs, a + * provider or `initialize()`: it goes to `read.recover`, with what the + * editor showed before the read. + * - `read.after` runs once the read has committed and nothing threw: after + * the update the read is part of, where it is nested in one (an undo the + * binding runs in an update of its own, see undo). + * + * What the read shows is its caller's to set up: a release of a hold + * clears the hold first (see flushHidden), so that the read shows what it + * held, and a recovery reads the whole document again. + */ + private guardedRead(read: GuardedRead): void { + if (this.unreadable) { + return; + } + const editor = this._editor; + const nested = editor._updating; + const shown = editor.getEditorState(); + let failure = null as Failure; + const settle = () => { + if (failure !== null) { + read.recover(failure.error, shown); + } else if (read.after !== undefined) { + read.after(); + } + }; + try { + editor.update( + () => { + try { + read.$read(); + } catch (error) { + failure = {error}; + // It is recovered from right after (see read.recover): a + // selection the failed read left on a removed node must not fail + // this update's commit too, which would drop an undo or redo the + // read is part of. + $setSelection(null); + } + if (nested) { + $onUpdate(() => this.guard(() => withPins(this._pins, settle))); + } + }, + {discrete: true, skipTransforms: true, tag: read.tag}, + ); + } catch (error) { + if (nested) { + // The update the read is part of threw: not the read's to recover. + throw error; + } + // Thrown committing what was read. + failure = {error}; + } + if (!nested) { + settle(); + } + } + /** * Fail closed where the document cannot be read (a read threw and so did * a fresh read of the document after it, or a read of the whole document @@ -2134,29 +2144,23 @@ export class YBinding { this.$quietSelection(); } - /** Replace the editor's content with a fresh read of the shared root. */ + /** + * Replace the editor's content with a fresh read of the shared root; what + * it throws is passed on (see recoverRead). + */ private rebuildFromDocument(): void { this.commitPending(); - let failure = null as Failure; - this._editor.update( - () => { - try { - this.$rebuild(); - } catch (error) { - failure = {error}; - } + this.guardedRead({ + $read: () => this.$rebuild(), + after: () => { + this.checkUnknownTypes(); + this.flushRefresh(true); }, - { - discrete: true, - skipTransforms: true, - tag: REMOTE_TAGS, + recover: error => { + throw error; }, - ); - if (failure !== null) { - throw failure.error; - } - this.checkUnknownTypes(); - this.flushRefresh(true); + tag: REMOTE_TAGS, + }); } /** @@ -2189,9 +2193,11 @@ export class YBinding { return; } const cursor = this.getCursor(); + // Cleared before the read, which shows what it held; a read that throws + // reads the whole document again (see recoverRead). this.state.hidden = null; - this._editor.update( - () => { + this.guardedRead({ + $read: () => { const reader = new Reader(this.state); if (!reader.$refreshComposed(hidden.ownerKey)) { // A local edit removed the composed line: what was hidden in it is @@ -2204,12 +2210,9 @@ export class YBinding { this.$caretOnText(); this.$quietSelection(); }, - { - discrete: true, - skipTransforms: true, - tag: REMOTE_TAGS, - }, - ); + recover: (error, shown) => this.recoverRead(error, shown), + tag: REMOTE_TAGS, + }); } /** The items of the binding's containers that `hidden` holds. */ @@ -3659,15 +3662,17 @@ export class YBinding { */ private rereadRefused(): void { let after: (() => void) | null = null; - this._editor.update( - () => { + this.guardedRead({ + $read: () => { after = this.$rereadRefused(); }, - {discrete: true, skipTransforms: true}, - ); - if (after !== null) { - (after as () => void)(); - } + after: () => { + if (after !== null) { + (after as () => void)(); + } + }, + recover: (error, shown) => this.recoverRead(error, shown, 'reread'), + }); } /** @@ -3703,17 +3708,16 @@ export class YBinding { try { this.guard(() => withPins(this._pins, () => { - const after = this.$rereadRefused(); - if (after !== null || later.length > 0) { + if (later.length > 0) { $onUpdate(() => { for (const [fn, options] of later) { editor.update(fn, options); } - if (after !== null) { - this.guard(() => withPins(this._pins, after)); - } }); } + // Nested in this update (see guardedRead): what follows the read + // runs after its commit, after the updates queued behind it. + this.rereadRefused(); }), ); done = true; @@ -3727,8 +3731,8 @@ export class YBinding { }; /** - * The re-read itself (see rereadRefused), inside an update; returns what - * runs once it has committed, if anything. + * The re-read itself (see rereadRefused), inside an update (see + * guardedRead); returns what runs once it has committed, if anything. */ private $rereadRefused(): (() => void) | null { const list = [...this.refused].filter( @@ -3754,35 +3758,15 @@ export class YBinding { $addUpdateTag(BLOCK_ID_REPAIR_TAG); } const cursor = this.$cursorBeforeRead(); - // What the editor shows before the read (see failClosed). - const shown = this._editor.getEditorState(); - let unreadable = null as Failure; - try { - const reader = new Reader(this.state); - for (const cs of list) { - const el = cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); - if ($isElementNode(el) && el.isAttached()) { - reader.$readFull(cs, el); - } - } - reader.$settle(settle); - this.$selectionAfterRead(cursor); - } catch (e) { - try { - this.$rebuild(); - this.warn( - RECOVERED_READ_WARNING, - `a change could not be read again: ${errorText(e)}`, - {error: e, reason: 'reread'}, - ); - } catch (error) { - unreadable = {error}; + const reader = new Reader(this.state); + for (const cs of list) { + const el = cs.y === this._root ? $getRoot() : $getNodeByKey(cs.key); + if ($isElementNode(el) && el.isAttached()) { + reader.$readFull(cs, el); } } - if (unreadable !== null) { - const {error} = unreadable; - return () => this.failClosed(error, shown); - } + reader.$settle(settle); + this.$selectionAfterRead(cursor); this.$quietSelection(); if (last) { // A transform that changes the same content each time it runs (a diff --git a/packages/lexical-y/src/internal/Schema.ts b/packages/lexical-y/src/internal/Schema.ts index cb325fe9a60..a934b75124c 100644 --- a/packages/lexical-y/src/internal/Schema.ts +++ b/packages/lexical-y/src/internal/Schema.ts @@ -1634,13 +1634,20 @@ export function textJSONFromAttrs(text: string, encoded: Attrs): Attrs { // --------------------------------------------------------------------------- // Ownership // -// Invariant: a write sets and deletes only the stored names its client owns -// (ownsNodeAttr, ownsMarkerAttr, ownsFormat, and ownsMember for the members -// of a value that holds a node's fields); every other name keeps its stored -// value. Why: a document outlives app versions, and clients of several -// versions edit it at once (during a rollout): a name a client does not -// know, such as a field a newer version of a class declares, is another -// client's data, which this one can neither show nor write back. +// Invariant, in two clauses: +// - A write changes the value a stored name gives content only where its +// client owns the name (ownsNodeAttr, ownsMarkerAttr, ownsFormat, and +// ownsMember for the members of a value that holds a node's fields); +// every other name keeps the value it gives content. +// - Format items that keep every name's value on the content around a write +// (the negations and restores around inserted content, run ends, gap +// cleanup, the copies an undo brings back) are written for any name: they +// change no value, and filtering them by owner would let a value run on +// past its content. +// Why: a document outlives app versions, and clients of several versions +// edit it at once (during a rollout): a name a client does not know, such +// as a field a newer version of a class declares, is another client's data, +// which this one can neither show nor write back. /** * The fields a client owns at one stored location: those its classes for From db9fcc4ffd821c2bcf5446b771c4e083d95bf3b2 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Thu, 29 Oct 2026 01:13:30 -0400 Subject: [PATCH 05/12] [lexical-y] Bug Fix: Read a node's stored type and its group's stored fields in the document the editor shows when a remote change waits to be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description A local update made before a remote change the binding has not read yet is written on the document as its editor shows it (see `Unread`). Two reads this PR adds to the writer still read raw Yjs: - A new item of a group takes the group fields its client does not own from the item next to it (`Writer#storedItemAttrs`). When the update waited for an unread remote change, the new item copied the change's value, so the room ended differently from the same edits written before the change arrived. - `$writeNodeAttrs` decides which fields the client owns from the node's stored type (`ownedFields`). Read raw, a remote retype the binding had not read looked like a type change of this write, which owns every field, so the write deleted the fields of the new type the client does not declare. Both now read through the unread view: `storedItemAttrs` with `isGone`, `attrOf` and `attrsOf`, and `$writeNodeAttrs` with `storedType`, as the rest of the writer does. ## Test plan ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts -t N2 × a card a remote change retypes, centered meanwhile ✓ an item appended to it, in a list a remote change retones × an item added before its first item, in a list a remote change retones (the waiting run deletes the card's "p:tone"; the waiting list item stores "tone":"cool", the concurrent run "tone":"warm") ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts Test Files 1 passed (1) ``` --- .../unit/regressions/SchemaSkew.test.ts | 109 ++++++++++++++++++ packages/lexical-y/src/internal/Write.ts | 11 +- 2 files changed, 115 insertions(+), 5 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts index 69d577de4bf..f686d4b744c 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/SchemaSkew.test.ts @@ -1892,6 +1892,115 @@ describe('S7-3, S7-11, S7-21: a server agent with older classes', () => { }); }); +// N2: a local update made before a remote change the binding has not read +// yet is written on the document its editor shows (see Unread). The group +// fields a client does not own, which a new item takes from the item next to +// it, are read in that document too, so the run where the update waits for +// the change ends as the run where it is written before the change arrives. +describe('N2: a write before an unread remote change reads the document its editor shows', () => { + // The editor's update `$local`, made while the remote change waits to be + // read (`waiting`), or written before it arrives. + const edit = (r: Room, waiting: boolean, $local: () => void) => { + if (waiting) { + // Not committed yet when the remote change arrives in this task: it is + // committed and written first, and the change is read after it. + r.editor.editor.update($local); + } else { + r.editor.update($local); + } + }; + // Delivers what the two clients sent while offline, until they converge. + const deliver = async (r: Room) => { + r.author.online = true; + r.editor.online = true; + for (let i = 0; i < 3; i++) { + for (const [to, from] of [ + [r.editor, r.author], + [r.author, r.editor], + ] as const) { + const list = to.pending.get(from) || []; + to.pending.delete(from); + if (list.length > 0) { + Y.applyUpdate( + to.doc, + Y.mergeUpdates(list as Uint8Array[]), + to.net, + ); + } + } + await flush(); + } + expect([...r.editor.errors, ...r.author.errors]).toEqual([]); + }; + const run = async ( + nodes: Klass[], + waiting: boolean, + $init: () => void, + $remote: () => void, + $local: () => void, + ): Promise => { + const r = await room(nodes, $init); + r.author.online = false; + r.editor.online = false; + r.author.update($remote); + edit(r, waiting, $local); + await deliver(r); + return docJSON(await freshReader(r.author)); + }; + const $retoneList = () => { + ($getRoot().getFirstChildOrThrow() as ListV2).getWritable().__tone = 'cool'; + }; + const $item = () => $createListItemNode().append($createTextNode('z')); + // The stored type is read there too: where a remote change retypes a node, + // this client owns the fields of the type its editor shows (see + // ownedFields), so its write deletes no field it does not own. + test('a card a remote change retypes, centered meanwhile', async () => { + const stored: Record[] = []; + for (const waiting of [false, true]) { + const r = await room(V1, () => $getRoot().append($card(), $p('para'))); + let card: Y.Node | null = null; + for (let it = r.author.doc.get('lexical-y')._start; it; it = it.right) { + const content = it.content; + if ( + content instanceof Y.ContentType && + String((content.type as Y.Node).name).slice(2) === 'test_skew_card' + ) { + card = content.type as Y.Node; + } + } + r.author.online = false; + r.editor.online = false; + // To a type both clients register, whose class declares the tone. + r.author.doc.transact(() => card!.setAttr('t', 'test_box')); + edit(r, waiting, () => $block([0]).setFormat('center')); + await deliver(r); + stored.push(card!.getAttrs()); + } + expect(stored[0]).toMatchObject({'p:tone': 'warm', t: 'test_box'}); + expect(stored[1]).toEqual(stored[0]); + }); + test.each([ + [ + 'appended to it', + () => ($getRoot().getFirstChildOrThrow() as ListNode).append($item()), + ], + [ + 'added before its first item', + () => + ($getRoot().getFirstChildOrThrow() as ListNode) + .getFirstChildOrThrow() + .insertBefore($item()), + ], + ])('an item %s, in a list a remote change retones', async (_what, $local) => { + const $init = () => $getRoot().append($list('x', 'y'), $p('para')); + for (const nodes of [V1, V2]) { + const concurrent = await run(nodes, false, $init, $retoneList, $local); + const waiting = await run(nodes, true, $init, $retoneList, $local); + expect(waiting).toBe(concurrent); + } + }); +}); + // The data of the warning stays an interface an app can extend. export interface AppUnknownNodeTypesData extends YUnknownNodeTypesData { readonly requestId: string; diff --git a/packages/lexical-y/src/internal/Write.ts b/packages/lexical-y/src/internal/Write.ts index 59fd73761b9..c4253302947 100644 --- a/packages/lexical-y/src/internal/Write.ts +++ b/packages/lexical-y/src/internal/Write.ts @@ -4218,17 +4218,18 @@ export class Writer { /** * The stored marker attributes of item `item` of a group of type `gtype`, - * or null where this write made its marker (or it has none). + * in the document the write sees (see Unread), or null where this write + * made its marker (or it has none). */ private storedItemAttrs(item: LexicalNode, gtype: unknown): Attrs | null { const unit = this.state.unitByKey.get(item.getKey()); const end = unit === undefined ? null : unit.end; - if (end === null || end.deleted || this.tr.insertSet.hasId(end.id)) { + if (end === null || isGone(end) || this.tr.insertSet.hasId(end.id)) { return null; } const marker = typeOf(end); - return marker.getAttr(GROUP_KEY) === gtype - ? (marker.getAttrs() as Attrs) + return attrOf(marker, GROUP_KEY) === gtype + ? (attrsOf(marker) as Attrs) : null; } @@ -4243,7 +4244,7 @@ export class Writer { // the type stored and of the node's. const fields = ownedFields( this.state.editor, - storedTypeOf(y), + storedType(y), node.getType(), true, ); From 7702f6608f9976656abed8cbfbddfea84b696597 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 22:20:57 -0700 Subject: [PATCH 06/12] [lexical-y] Bug Fix: Read a remote change, and throw nothing into Y.applyUpdate, when a pending local update throws as it commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Before a remote read the binding commits the local updates still pending, so that the read's update commits alone. That commit ran outside the read's fail-closed entry point (`YBinding#guardedRead`): when an update or an update listener of the app's threw on it, the error escaped into the provider's `Y.applyUpdate`, the remote change was never read, and the editor kept showing the old text while the binding stayed active. The next local edit was then written at offsets computed without the remote change, so the clients diverged for good. Now `guardedRead` commits the pending updates itself, for every read that is not nested in an update. A throw from that commit is the app's error: it is rethrown in a microtask (an uncaught error, as a throwing `onWarn` handler's is), the remaining pending updates still commit, and the read goes on. The three call sites that committed before calling `guardedRead` no longer do; what the remote read takes from the editor once those updates committed (the selection to keep, whether it re-reads a naming update) moves into the read's new `before` step. A read of the whole document still takes over a pending update instead of committing it (`replacesPending`), as before: that is how an initial state set before opening a document with content is replaced. ## Test plan New `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`, `N4` cases: A deletes a word while B has a formatting update pending; B's commit of it throws (an app update listener, with `onError` rethrowing and reporting, and an update an app listener queues that throws). Each requires that nothing is thrown into the sync, B reads A's change, the app's error is rethrown in a microtask, and B's next edit lands where B made it on every client. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ❯ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (3 tests | 3 failed) 132ms × N4: an update listener of the app throws on that commit 98ms × N4: an update listener of the app throws on that commit 17ms × N4: an update an app listener queues throws, and onError rethrows 15ms FAIL |unit| ... > onError rethrows > N4: an update listener of the app throws on that commit FAIL |unit| ... > onError reports > N4: an update listener of the app throws on that commit AssertionError: promise rejected "Error: app failure" instead of resolving FAIL |unit| ... > N4: an update an app listener queues throws, and onError rethrows AssertionError: promise rejected "Error: app failure" instead of resolving Tests 3 failed (3) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (3 tests) 194ms Tests 3 passed (3) ``` The whole `@lexical/y` unit suite passes with this commit (`pnpm exec vitest run --project unit packages/lexical-y/`); the full suite and `pnpm run ci-check` results are on the last commit of this series. --- .../unit/regressions/GuardedRead.test.ts | 125 ++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 105 ++++++++++----- 2 files changed, 197 insertions(+), 33 deletions(-) create mode 100644 packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts new file mode 100644 index 00000000000..6206c81215b --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.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 + +// Every read of the binding goes through one fail-closed entry point +// (YBinding#guardedRead). These cases pin what it owns beyond the read +// itself: the local updates it commits before the read, a recovery that +// runs after the update the read is nested in, and a read that runs after +// the binding failed closed. Each case is named after its finding. + +import {$getRoot} from 'lexical'; +import {afterEach, describe, expect, test, vi} from 'vitest'; + +import { + $block, + $p, + $selectIn, + freshReader, + type Peer, + setup, +} from '../../utils/harness'; + +/** + * Run every microtask as usual, and collect what those that throw threw + * (an error the binding rethrows in a microtask, which would otherwise be + * an uncaught error). + */ +function catchMicrotasks(): unknown[] { + const caught: unknown[] = []; + const original = globalThis.queueMicrotask.bind(globalThis); + vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(fn => + original(() => { + try { + fn(); + } catch (error) { + caught.push(error); + } + }), + ); + return caught; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +const LINE = 't1z t2z t3z t4z t5z t6z t7z t8z t9z'; + +function $selectWord(word: string) { + const at = $block([0]).getTextContent().indexOf(word); + return $selectIn([0], at, at + word.length); +} + +describe('N4: a throw committing the updates pending before a read reaches no provider', () => { + /** + * A deletes `t2z` while B, offline, bolds `t7z` in an update still + * pending when A's change arrives; `arm` makes the commit of B's update + * throw. Requires: nothing thrown into the sync, B reads A's change, the + * app's error is rethrown in a microtask, and B's next edit lands where B + * made it, on every client. + */ + async function run( + arm: (b: Peer, error: Error) => void, + onError: 'rethrows' | 'reports', + ) { + const t = await setup(() => $getRoot().append($p(LINE), $p('tail'))); + if (onError === 'reports') { + t.b.editor._onError = e => void t.b.errors.push(e); + } + t.offline(); + t.a.update(() => void $selectWord('t2z').removeText()); + t.b.editor.update(() => void $selectWord('t7z').formatText('bold')); + const error = new Error('app failure'); + arm(t.b, error); + const caught = catchMicrotasks(); + await expect(t.sync()).resolves.toBeUndefined(); + expect(caught).toContain(error); + expect(t.b.binding.status).toBe('active'); + expect(t.b.describe()).toBe((await freshReader(t.a)).describe()); + expect(t.b.describe()).not.toContain('t2z'); + t.b.update(() => void $selectWord('t5z').removeText()); + await t.sync(); + const fresh = await freshReader(t.a); + expect(t.a.describe()).toBe(fresh.describe()); + expect(t.b.describe()).toBe(fresh.describe()); + expect(fresh.read(() => $getRoot().getTextContent())).toBe( + 't1z t3z t4z t6z t7z t8z t9z\n\ntail', + ); + } + + describe.each(['rethrows', 'reports'] as const)('onError %s', onError => { + test('N4: an update listener of the app throws on that commit', async () => { + await run((b, error) => { + let armed = true; + b.editor.registerUpdateListener(() => { + if (armed) { + armed = false; + throw error; + } + }); + }, onError); + }); + }); + + test('N4: an update an app listener queues throws, and onError rethrows', async () => { + await run((b, error) => { + let armed = true; + b.editor.registerUpdateListener(() => { + if (armed) { + armed = false; + b.editor.update(() => { + $selectWord('t9z').removeText(); + throw error; + }); + } + }); + }, 'rethrows'); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 853faa1202d..4dd6681bb1f 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -173,8 +173,21 @@ type Failure = {readonly error: unknown} | null; /** A read of the shared document into the editor (see YBinding#guardedRead). */ type GuardedRead = { + /** + * Runs right before the read's update, once the local updates still + * pending have committed (see commitPending): what the read takes from + * the editor as those updates left it. + */ + readonly before?: () => void; /** The read, inside the update. */ readonly $read: () => void; + /** + * Whether the read's update takes over the local updates still pending + * instead of committing them first (see commitPending): a read of the + * whole document, which replaces what they show (the initial state an + * app sets before the binding opens a document that has content). + */ + readonly replacesPending?: true; /** * Where a throw goes, of the read or of committing it, with what the * editor showed before the read: recoverRead, which reads the whole @@ -1465,7 +1478,6 @@ export class YBinding { } private rereadContainers(list: CS[], extraTags: string[] = []): void { - this.commitPending(); this.guardedRead({ $read: () => { const cursor = this.$cursorBeforeRead(); @@ -1691,6 +1703,7 @@ export class YBinding { }, // The whole document: read again, it would throw again. recover: (error, shown) => this.failClosed(error, shown), + replacesPending: true, tag: REMOTE_TAGS, }); } @@ -1861,25 +1874,15 @@ export class YBinding { return; } // Invariant: a local update still pending (an editor.update() made in - // the same task) is committed, and written, before a remote read. Why: - // committed with the read, it would carry the read's tags and never be - // written. It is written on the document as it was before this change, - // which it was made on (see Unread). + // the same task) is committed, and written, before a remote read (see + // guardedRead). Why: committed with the read, it would carry the read's + // tags and never be written. It is written on the document as it was + // before this change, which it was made on (see Unread). if (this.unread === null || !this.unread.changes.includes(tr)) { this.noteChange(tr); } const committed = this._editor.getEditorState(); - this.commitPending(); - // The selection the read keeps is the one those updates left, on the - // document they were made on. - if (this._editor.getEditorState() !== committed) { - this.pendingCursor = this.getCursor(); - } - // The read sees the document as it is: this change, and those applied - // since, which are read after it (see Reader). - this.reading = true; - let cursor = this.pendingCursor; - this.pendingCursor = null; + let cursor: YCursor | null = null; // An undo puts back the selection of its step (a range, after typing // over one); any other read keeps the user's caret a caret. The nulls an // undo writes after itself (see closeRestoredRuns) are read as part of @@ -1888,18 +1891,13 @@ export class YBinding { const undoing = (tr.origin === this._undoManager && this._undoManager !== null) || tr.origin === this.runEndOrigin; - if (undoing) { - if (tr.origin !== this.runEndOrigin) { - this.undoneCursor = cursor; - } - cursor = this.stepCursor ?? cursor; - } let rebuilt: ReadonlySet = new Set(); - // A re-read of a naming update's change is tagged as that update (see - // rereadRefused). - const naming = this.refused.size > 0 && this.refusedNaming; + let naming = false; this.guardedRead({ - $read: () => + $read: () => { + if (naming) { + $addUpdateTag(BLOCK_ID_REPAIR_TAG); + } this.$readRemote( state, childItems, @@ -1910,7 +1908,8 @@ export class YBinding { rebuilt = r; }, tr, - ), + ); + }, after: () => { this.checkUnknownTypes(); this.checkRead(rebuilt); @@ -1920,8 +1919,29 @@ export class YBinding { // would take what the editor does not show yet for deleted. this.runPending(); }, + before: () => { + // The selection the read keeps is the one those updates left, on the + // document they were made on. + if (this._editor.getEditorState() !== committed) { + this.pendingCursor = this.getCursor(); + } + // The read sees the document as it is: this change, and those + // applied since, which are read after it (see Reader). + this.reading = true; + cursor = this.pendingCursor; + this.pendingCursor = null; + if (undoing) { + if (tr.origin !== this.runEndOrigin) { + this.undoneCursor = cursor; + } + cursor = this.stepCursor ?? cursor; + } + // A re-read of a naming update's change is tagged as that update + // (see rereadRefused). + naming = this.refused.size > 0 && this.refusedNaming; + }, recover: (error, shown) => this.recoverRead(error, shown), - tag: naming ? [...REMOTE_TAGS, BLOCK_ID_REPAIR_TAG] : REMOTE_TAGS, + tag: REMOTE_TAGS, }); } @@ -2034,9 +2054,14 @@ export class YBinding { * go through it can neither recover nor fail closed. * * - Nothing is read once the binding failed closed (see failClosed). + * - The local updates still pending commit first, on their own (see + * commitPending), where the read is not nested in an update and does + * not replace them (`read.replacesPending`). * - A throw, of the read or of committing it, never reaches Yjs, a * provider or `initialize()`: it goes to `read.recover`, with what the - * editor showed before the read. + * editor showed before the read. A throw committing the pending + * updates is the app's (an update or a listener of its own): it is + * rethrown in a microtask, and the read goes on. * - `read.after` runs once the read has committed and nothing threw: after * the update the read is part of, where it is nested in one (an undo the * binding runs in an update of its own, see undo). @@ -2051,6 +2076,12 @@ export class YBinding { } const editor = this._editor; const nested = editor._updating; + if (read.replacesPending !== true) { + this.commitPending(); + } + if (read.before !== undefined) { + read.before(); + } const shown = editor.getEditorState(); let failure = null as Failure; const settle = () => { @@ -2149,7 +2180,6 @@ export class YBinding { * it throws is passed on (see recoverRead). */ private rebuildFromDocument(): void { - this.commitPending(); this.guardedRead({ $read: () => this.$rebuild(), after: () => { @@ -3509,9 +3539,18 @@ export class YBinding { i++ ) { const pending = editor._pendingEditorState; - editor.update(() => {}, {discrete: true}); - if (editor._pendingEditorState === pending) { - editor.read(() => {}); + try { + editor.update(() => {}, {discrete: true}); + if (editor._pendingEditorState === pending) { + editor.read(() => {}); + } + } catch (error) { + // An update or a listener of the app's threw: its error is rethrown + // in a microtask (an uncaught error), never into Yjs or a provider, + // and the updates still pending commit. + queueMicrotask(() => { + throw error; + }); } } } From 90641125acea5eb6d0d4102779fe2e36bcca0088 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 22:25:54 -0700 Subject: [PATCH 07/12] [lexical-y] Bug Fix: Report a read recovered only once the rebuild ran, and fail closed when it fails, for a read nested in an update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description A remote read that throws is recovered from by rebuilding the editor from the document (`recoverRead`), and where that rebuild throws too, the binding fails closed (`failClosed`). Where the read was nested in an update (`Y.applyUpdate` or `binding.undo()` called inside `editor.update()`), the rebuild could only be queued, so `recoverRead` took the queuing for success: it warned `RECOVERED_READ_WARNING` before anything was rebuilt. When the queued rebuild then threw, its error escaped into the app's update and the provider, and the editor ended empty and active, with no read-only status. Now the recovery settles on the rebuild itself. `recoverRead` hands the rebuild to `guardedRead` with the warning as what follows a committed rebuild and `failClosed` (with what the editor showed before the failed read) as where its throw goes; `guardedRead` runs either once the rebuild has run, nested or not. The separate `rebuildFromDocument`, whose only job was to rethrow, is gone. `failClosed`, which can now run from a commit's `$onUpdate` callbacks, commits the state it restores there too: `setEditorState` alone leaves it pending in that phase. ## Test plan New `N5` cases in `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`: a remote change and an undo read inside an app update while every read throws (nothing thrown, read only, the editor shows what it showed, one `'unreadable'` warning), and a remote change read inside an app update whose first read throws (the recovered warning arrives once the editor shows the rebuilt document). ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts × N5: a remote change read inside an app update, whose recovery fails too, fails closed 17ms × N5: an undo read inside an app update, whose recovery fails too, fails closed 11ms × N5: a remote change read inside an app update, recovered: warned once the rebuild committed 12ms FAIL |unit| ... > N5: a remote change read inside an app update, whose recovery fails too, fails closed FAIL |unit| ... > N5: an undo read inside an app update, whose recovery fails too, fails closed AssertionError: expected [Function] to not throw an error but 'Error: reader failure' was thrown FAIL |unit| ... > N5: a remote change read inside an app update, recovered: warned once the rebuild committed AssertionError: expected [ Array(1) ] to deeply equal [ Array(1) ] [ - "a remote change could not be read: reader failure: paragraph(\"hello remote\")", + "a remote change could not be read: reader failure: paragraph(\"hello\")", ] Tests 3 failed | 3 passed (6) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (6 tests) 236ms Tests 6 passed (6) $ pnpm exec vitest run --project unit packages/lexical-y/ Test Files 143 passed | 2 skipped (145) Tests 5020 passed | 40 expected fail | 2 skipped (5062) ``` --- .../unit/regressions/GuardedRead.test.ts | 93 ++++++++++++++++++- packages/lexical-y/src/internal/Binding.ts | 60 ++++++------ 2 files changed, 119 insertions(+), 34 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index 6206c81215b..cb4e8028879 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -14,13 +14,16 @@ // runs after the update the read is nested in, and a read that runs after // the binding failed closed. Each case is named after its finding. -import {$getRoot} from 'lexical'; +import * as Y from '@y/y'; +import {$createTextNode, $getRoot} from 'lexical'; import {afterEach, describe, expect, test, vi} from 'vitest'; +import {Reader} from '../../../internal/Read'; import { $block, $p, $selectIn, + flush, freshReader, type Peer, setup, @@ -123,3 +126,91 @@ describe('N4: a throw committing the updates pending before a read reaches no pr }, 'rethrows'); }); }); + +describe('N5: a recovery nested in an update settles on the rebuild it runs', () => { + /** Make every read of the reader throw, until restored. */ + function failEveryRead(): () => void { + const proto = Reader.prototype; + const {$apply, $readFull} = proto; + proto.$apply = () => { + throw new Error('reader failure'); + }; + proto.$readFull = () => { + throw new Error('reader failure'); + }; + return () => { + proto.$apply = $apply; + proto.$readFull = $readFull; + }; + } + + test.each(['a remote change', 'an undo'] as const)( + 'N5: %s read inside an app update, whose recovery fails too, fails closed', + async entry => { + const t = await setup(() => $getRoot().append($p('hello'))); + let update = new Uint8Array(); + if (entry === 'a remote change') { + t.offline(); + t.b.update(() => void $block([0]).append($createTextNode(' remote'))); + update = Y.encodeStateAsUpdate(t.b.doc, Y.encodeStateVector(t.a.doc)); + } else { + t.a.update(() => void $block([0]).append($createTextNode(' local'))); + expect(t.a.binding.undoManager!.canUndo()).toBe(true); + } + const shown = t.a.describe(); + const restore = failEveryRead(); + try { + expect(() => + t.a.editor.update( + () => { + if (entry === 'a remote change') { + Y.applyUpdate(t.a.doc, update); + } else { + t.a.binding.undo(); + } + }, + {discrete: true}, + ), + ).not.toThrow(); + await flush(); + } finally { + restore(); + } + expect(t.a.binding.status).toBe('readonly'); + expect(t.a.describe()).toBe(shown); + expect(t.a.errors).toEqual([]); + expect(t.a.warnings).toEqual([ + 'the document cannot be read: reader failure', + ]); + }, + ); + + test('N5: a remote change read inside an app update, recovered: warned once the rebuild committed', async () => { + const t = await setup(() => $getRoot().append($p('hello'))); + t.offline(); + t.b.update(() => void $block([0]).append($createTextNode(' remote'))); + const update = Y.encodeStateAsUpdate(t.b.doc, Y.encodeStateVector(t.a.doc)); + const proto = Reader.prototype; + const {$apply} = proto; + proto.$apply = () => { + throw new Error('reader failure'); + }; + const shownAtWarning: string[] = []; + t.a.editor._onWarn = w => { + shownAtWarning.push(`${(w as Error).message}: ${t.a.describe()}`); + }; + try { + t.a.editor.update(() => Y.applyUpdate(t.a.doc, update), { + discrete: true, + }); + await flush(); + } finally { + proto.$apply = $apply; + } + expect(t.a.binding.status).toBe('active'); + expect(t.a.describe()).toBe('paragraph("hello remote")'); + expect(shownAtWarning).toEqual([ + 'a remote change could not be read: reader failure: paragraph("hello remote")', + ]); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 4dd6681bb1f..3d337d37811 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -2025,27 +2025,33 @@ export class YBinding { * client converges on) in a fresh update, and the failure is reported. * Where the rebuild throws too, the editor's classes cannot read the * document: the binding fails closed, back to `shown` (see failClosed). + * Either settles once the rebuild has run, which is after the update the + * failed read is part of where it is nested in one (see guardedRead): the + * warning follows a rebuild that committed, never one only queued. */ private recoverRead( error: unknown, shown: EditorState, reason: YRecoveredReadData['reason'] = 'remote-change', ): void { - try { - this.rebuildFromDocument(); - } catch (e) { - this.failClosed(e, shown); - return; - } - this.warn( - RECOVERED_READ_WARNING, - `${ - reason === 'reread' - ? 'a change could not be read again' - : 'a remote change could not be read' - }: ${errorText(error)}`, - {error, reason}, - ); + this.guardedRead({ + $read: () => this.$rebuild(), + after: () => { + this.checkUnknownTypes(); + this.flushRefresh(true); + this.warn( + RECOVERED_READ_WARNING, + `${ + reason === 'reread' + ? 'a change could not be read again' + : 'a remote change could not be read' + }: ${errorText(error)}`, + {error, reason}, + ); + }, + recover: rebuildError => this.failClosed(rebuildError, shown), + tag: REMOTE_TAGS, + }); } /** @@ -2156,6 +2162,12 @@ export class YBinding { }); } else { editor.setEditorState(shown, {tag: COLLABORATION_TAG}); + if (editor._updating) { + // Set from a commit's $onUpdate callbacks (the recovery of a read + // nested in an update, see guardedRead), where nothing commits it: + // the update queued here, which runs right after them, does. + editor.update(() => {}, {discrete: true}); + } } } @@ -2175,24 +2187,6 @@ export class YBinding { this.$quietSelection(); } - /** - * Replace the editor's content with a fresh read of the shared root; what - * it throws is passed on (see recoverRead). - */ - private rebuildFromDocument(): void { - this.guardedRead({ - $read: () => this.$rebuild(), - after: () => { - this.checkUnknownTypes(); - this.flushRefresh(true); - }, - recover: error => { - throw error; - }, - tag: REMOTE_TAGS, - }); - } - /** * The line or inline container holding the node being composed (IME), if * the binding stores it as one. From 2c3e8f590f68d34cb229278ebf5014f9a3fc77aa Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 22:30:38 -0700 Subject: [PATCH 08/12] [lexical-y] Bug Fix: Read nothing after the binding failed closed, also for a read asked for before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `YBinding#guardedRead` reads nothing once the binding failed closed, but it checked that only when a read was asked for. A read nested in an update is only queued then, and runs later: when the binding failed closed in between (a read of the whole document that throws settles to `failClosed`, and an update listener applies a remote change before that), the queued read still ran, and changed what the frozen editor showed. Now `guardedRead` checks where the read actually runs, inside its update, and again before what follows it (its recovery or its follow-up): a read that runs after the binding failed closed reads, recovers and follows up nothing. That covers updates queued behind the one that failed and those drained while the editor's earlier state is restored. ## Test plan New `N6` case in `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`: a read-only reader (a type it does not register) reads a layout record inside an app update; that read of the whole document throws, and an update listener applies a content change before it settles. Requires the binding to fail closed and keep showing what it showed, without the content change. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts × N6: a remote change an update listener applies while a failing read of the whole document waits 28ms FAIL |unit| ... > N6: a remote change an update listener applies while a failing read of the whole document waits AssertionError: expected 'paragraph("hello remote")' to be 'paragraph("hello")' // Object.is equality Tests 1 failed | 6 passed (7) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (7 tests) 256ms Tests 7 passed (7) $ pnpm exec vitest run --project unit packages/lexical-y/ Test Files 143 passed | 2 skipped (145) Tests 5021 passed | 40 expected fail | 2 skipped (5063) ``` --- .../unit/regressions/GuardedRead.test.ts | 85 ++++++++++++++++++- packages/lexical-y/src/internal/Binding.ts | 13 ++- 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index cb4e8028879..cdd0e55707c 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -21,14 +21,25 @@ import {afterEach, describe, expect, test, vi} from 'vitest'; import {Reader} from '../../../internal/Read'; import { $block, + $createBoxNode, $p, $selectIn, + BoxNode, flush, freshReader, - type Peer, + NODES, + Peer, setup, } from '../../utils/harness'; +/** A network of its own, for a peer the case connects by hand. */ +const network = (): ConstructorParameters[1] => ({ + delivery: 'causal', + peers: [], + pumping: false, + queue: [], +}); + /** * Run every microtask as usual, and collect what those that throw threw * (an error the binding rethrows in a microtask, which would otherwise be @@ -214,3 +225,75 @@ describe('N5: a recovery nested in an update settles on the rebuild it runs', () ]); }); }); + +describe('N6: a read admitted before the binding failed closed reads nothing after it', () => { + test('N6: a remote change an update listener applies while a failing read of the whole document waits', async () => { + const author = new Peer(2, network()); + author.update(() => + $getRoot() + .clear() + .append($p('hello'), $createBoxNode('t').append($p('inside'))), + ); + author.binding.initialize(); + await flush(); + // A reader without the box class: read only, still reading what changes. + const reader = new Peer( + 1, + network(), + NODES.filter(k => k !== BoxNode), + ); + Y.applyUpdate(reader.doc, Y.encodeStateAsUpdate(author.doc)); + reader.binding.initialize(); + await flush(); + expect(reader.binding.status).toBe('readonly'); + // A layout record this reader reads, which makes it read the whole + // document again, and a content change after it. + const seen = Y.encodeStateVector(reader.doc); + author.doc.transact(() => + (author.doc.get('lexical-y') as Y.Node).setAttr('lexical-y:2', true), + ); + const layout = Y.encodeStateAsUpdate(author.doc, seen); + const afterLayout = Y.encodeStateVector(author.doc); + author.update(() => void $block([0]).append($createTextNode(' remote'))); + await flush(); + const content = Y.encodeStateAsUpdate(author.doc, afterLayout); + const shown = reader.describe(); + // The whole read fails once (the binding fails closed); before that + // settles, an update listener applies the content change. + const proto = Reader.prototype; + const {$readFull} = proto; + let failed = false; + proto.$readFull = function (this: Reader, ...args) { + if (!failed && this.state.doc === reader.doc) { + failed = true; + throw new Error('reader failure'); + } + return $readFull.apply(this, args); + }; + let applied = false; + const off = reader.editor.registerUpdateListener(() => { + if (failed && !applied) { + applied = true; + Y.applyUpdate(reader.doc, content); + } + }); + try { + expect(() => + reader.editor.update(() => Y.applyUpdate(reader.doc, layout), { + discrete: true, + }), + ).not.toThrow(); + await flush(); + } finally { + off(); + proto.$readFull = $readFull; + } + expect(applied).toBe(true); + expect(reader.warnings).toContain( + 'the document cannot be read: reader failure', + ); + expect(reader.binding.status).toBe('readonly'); + expect(reader.describe()).toBe(shown); + expect(shown).not.toContain('remote'); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 3d337d37811..5571f5a51f8 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -2059,7 +2059,9 @@ export class YBinding { * The one entry point for every read of the binding: a read that does not * go through it can neither recover nor fail closed. * - * - Nothing is read once the binding failed closed (see failClosed). + * - Nothing is read, recovered or followed up once the binding failed + * closed (see failClosed), whenever the read was asked for: where it + * runs, not only where it is admitted. * - The local updates still pending commit first, on their own (see * commitPending), where the read is not nested in an update and does * not replace them (`read.replacesPending`). @@ -2091,6 +2093,9 @@ export class YBinding { const shown = editor.getEditorState(); let failure = null as Failure; const settle = () => { + if (this.unreadable) { + return; + } if (failure !== null) { read.recover(failure.error, shown); } else if (read.after !== undefined) { @@ -2100,6 +2105,12 @@ export class YBinding { try { editor.update( () => { + // Admitted before the binding failed closed and run after it (an + // update queued behind the one that failed, or drained by the + // restore of what the editor showed): nothing is read. + if (this.unreadable) { + return; + } try { read.$read(); } catch (error) { From 42825918db3e0d906316d00e4393044e858d8892 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 23:04:42 -0700 Subject: [PATCH 09/12] [lexical-y] Bug Fix: Write a local update an app listener queues before a remote read, also when that listener throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Before a remote read the binding commits the local updates still pending (`commitPending`), so that the read's update commits alone. It looked only at the editor's pending state. When an app update listener queued an update and then threw, Lexical never ran the queued update (it runs queued updates at the end of the commit the listener aborted), so there was no pending state, only a queued update: the binding went on to the read, Lexical ran the queued update inside the read's update, and it committed under the read's `COLLABORATION_TAG` and was never written. The editor showed it while no other client stored it, with the binding active and no warning. Now `commitPending` commits until neither a pending state nor a queued update is left, and says whether it got there. Where listeners keep making local updates past the commits a read makes first (`MAX_PENDING_COMMITS`, 16 as before), `guardedRead` does not run the read, which would take them over unwritten: it goes to the read's recovery, as a read that throws does. A remote read is then rebuilt from the document once those updates have committed (warning `RECOVERED_READ_WARNING`), or fails closed if they still do not stop. ## Test plan New `R2-1` cases in `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`: A deletes the first character while B has a bold edit pending, and B's update listener queues a paragraph and throws, once, or on each of 20 commits in a row (past the 16 a read commits first). Each requires B to match A and a fresh reader, with every queued paragraph stored, the binding active, and the recovery warned in the second case. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts × R2-1: an update listener queues a local update, then throws 30ms × R2-1: listeners that queue local updates past the commits a read makes first: recovered, every update written 24ms FAIL |unit| ... > R2-1: an update listener queues a local update, then throws Expected: "paragraph("lpha " "beta"{1} " gamma") / paragraph("tail")" Received: "paragraph("lpha " "beta"{1} " gamma") / paragraph("tail") / paragraph("queued edit")" FAIL |unit| ... > R2-1: listeners that queue local updates past the commits a read makes first: recovered, every update written Expected: "paragraph("lpha " "beta"{1} " gamma") / paragraph("tail")" Received: "paragraph("lpha " "beta"{1} " gamma") / paragraph("tail") / paragraph("queued 20")" Tests 2 failed | 7 passed (9) ``` (Expected is what A and a fresh reader store; Received is what B shows.) ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (9 tests) Tests 9 passed (9) $ pnpm exec vitest run --project unit --maxWorkers=24 packages/lexical-y/ packages/lexical/src Test Files 245 passed | 2 skipped (247) Tests 7408 passed | 40 expected fail | 2 skipped (7450) ``` `pnpm run ci-check` passes. --- .../unit/regressions/GuardedRead.test.ts | 64 +++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 63 ++++++++++++------ 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index cdd0e55707c..6033609a21f 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -297,3 +297,67 @@ describe('N6: a read admitted before the binding failed closed reads nothing aft expect(shown).not.toContain('remote'); }); }); + +describe('R2-1: a read runs only once no local update is pending or queued', () => { + /** + * A deletes the first character while B, offline, has a bold edit + * pending; `arm` makes B's listeners queue local updates on that commit. + */ + async function run(arm: (b: Peer) => void) { + const t = await setup(() => + $getRoot().append($p('alpha beta gamma'), $p('tail')), + ); + t.offline(); + t.a.update(() => void $selectIn([0], 0, 1).removeText()); + t.b.editor.update(() => void $selectWord('beta').formatText('bold')); + arm(t.b); + const caught = catchMicrotasks(); + await expect(t.sync()).resolves.toBeUndefined(); + await t.sync(); + const fresh = await freshReader(t.a); + expect(t.b.binding.status).toBe('active'); + expect(t.a.describe()).toBe(fresh.describe()); + expect(t.b.describe()).toBe(fresh.describe()); + return {b: t.b, caught, fresh}; + } + + test('R2-1: an update listener queues a local update, then throws', async () => { + const error = new Error('app failure'); + const {caught, fresh} = await run(b => { + let armed = true; + b.editor.registerUpdateListener(() => { + if (armed) { + armed = false; + b.editor.update(() => void $getRoot().append($p('queued edit'))); + throw error; + } + }); + }); + expect(caught).toEqual([error]); + expect(fresh.describe()).toContain('lpha'); + expect(fresh.describe()).not.toContain('alpha'); + expect(fresh.describe()).toContain('paragraph("queued edit")'); + }); + + test('R2-1: listeners that queue local updates past the commits a read makes first: recovered, every update written', async () => { + const {b, caught, fresh} = await run(peer => { + // Each commit's listener queues the next update and throws, so that + // each update waits for a commit of its own. + let left = 20; + peer.editor.registerUpdateListener(({tags}) => { + if (left > 0 && !tags.has('collaboration')) { + const n = left--; + peer.editor.update(() => void $getRoot().append($p(`queued ${n}`))); + throw new Error(`app failure ${n}`); + } + }); + }); + expect(caught).toHaveLength(20); + for (let n = 1; n <= 20; n++) { + expect(fresh.describe()).toContain(`paragraph("queued ${n}")`); + } + expect(b.warnings).toEqual([ + 'a remote change could not be read: local updates were still pending after 16 commits', + ]); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 5571f5a51f8..dd08a4041a0 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -242,6 +242,15 @@ const PENDING_ORDER: readonly PendingWork[] = [ */ const MAX_REREADS = 3; +/** + * The commits of local updates a read makes before it runs (see + * commitPending). Invariant: a read never runs with local updates still + * pending, which it would commit unwritten under its own tags; where + * listeners keep making them past this many commits, the read does not run + * and is recovered from instead (see guardedRead). + */ +const MAX_PENDING_COMMITS = 16; + /** * How many updates for registered transforms the binding runs before the * event loop turns (see YBinding#runRegistrationPasses), as Lexical caps @@ -2064,7 +2073,9 @@ export class YBinding { * runs, not only where it is admitted. * - The local updates still pending commit first, on their own (see * commitPending), where the read is not nested in an update and does - * not replace them (`read.replacesPending`). + * not replace them (`read.replacesPending`). Where listeners keep making + * them past MAX_PENDING_COMMITS, the read does not run: that goes to + * `read.recover` too. * - A throw, of the read or of committing it, never reaches Yjs, a * provider or `initialize()`: it goes to `read.recover`, with what the * editor showed before the read. A throw committing the pending @@ -2084,14 +2095,20 @@ export class YBinding { } const editor = this._editor; const nested = editor._updating; - if (read.replacesPending !== true) { - this.commitPending(); + let failure = null as Failure; + if (read.replacesPending !== true && !this.commitPending()) { + // The app's listeners keep making local updates: the read, which would + // commit them unwritten under its tags, does not run. + failure = { + error: new Error( + `local updates were still pending after ${MAX_PENDING_COMMITS} commits`, + ), + }; } if (read.before !== undefined) { read.before(); } const shown = editor.getEditorState(); - let failure = null as Failure; const settle = () => { if (this.unreadable) { return; @@ -2102,6 +2119,11 @@ export class YBinding { read.after(); } }; + if (failure !== null) { + // Not nested (in an update, commitPending leaves the updates to it). + settle(); + return; + } try { editor.update( () => { @@ -3527,26 +3549,28 @@ export class YBinding { /** * Commit the local updates still pending (an editor.update() made in the - * same task, or one a listener of the last commit queued). Invariant: the - * update the binding makes next (a read) commits alone. Why: an update - * still pending would be committed with it, and take its tags. Committing - * one can leave another pending (its listeners queue one): up to 16 are - * committed. A pending state that holds nothing to commit is committed as - * it is: an update reuses such a state without committing it, so a read - * that changes nothing would leave its tags on it, and the next update - * that changes something would commit with them and not be written. + * same task, or one a listener of the last commit queued, also where that + * listener threw before Lexical ran it). Invariant: the update the binding + * makes next (a read) commits alone. Why: an update still pending would be + * committed with it, and take its tags. Committing one can leave another + * pending (its listeners queue one): up to MAX_PENDING_COMMITS are + * committed. Returns whether none is left (where the editor is not in an + * update; in one, the updates are its own). A pending state that holds + * nothing to commit is committed as it is: an update reuses such a state + * without committing it, so a read that changes nothing would leave its + * tags on it, and the next update that changes something would commit + * with them and not be written. */ - private commitPending(): void { + private commitPending(): boolean { const editor = this._editor; - for ( - let i = 0; - i < 16 && editor._pendingEditorState !== null && !editor._updating; - i++ - ) { + const left = () => + !editor._updating && + (editor._pendingEditorState !== null || editor._updates.length > 0); + for (let i = 0; i < MAX_PENDING_COMMITS && left(); i++) { const pending = editor._pendingEditorState; try { editor.update(() => {}, {discrete: true}); - if (editor._pendingEditorState === pending) { + if (pending !== null && editor._pendingEditorState === pending) { editor.read(() => {}); } } catch (error) { @@ -3558,6 +3582,7 @@ export class YBinding { }); } } + return !left(); } /** From 913e6adeee6a6bd0425662bdda1e8be06f96c1f3 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Wed, 28 Oct 2026 23:30:41 -0700 Subject: [PATCH 10/12] [lexical-y] Bug Fix: Write the local updates a recovery commits on the document the editor shows, not on a remote change it has not read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description When listeners keep making local updates past the commits a read makes first, `YBinding#guardedRead` skips the read and recovers from it instead. It still ran the read's `before` step, though, and a remote read's `before` switches the binding to reading the document as it is (the remote change included). The recovery then committed the remaining local updates in that mode, while the editor did not show the remote change yet: their offsets were taken on the wrong text. With a remote `!` inserted at the start and 20 local deletions of the first character, every client stored `tuvwxyz` instead of `!uvwxyz`: the remote insert was deleted and a character meant to be deleted survived. Now `before` runs only where the read runs. The switch to reading the document as it is has one owner, `startReading`, which a remote read's `before` and the recovery's rebuild both call once the local updates still pending are written, on the document the editor shows (see `Unread`). It changes nothing outside a transaction being read. ## Test plan New `R3-1` case in `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`: A inserts `!` at the start while B deletes its first character 20 times (one pending deletion, then a listener that queues each next one and throws, past the 16 commits a read makes first). Requires every client to store what the edits give when they meet concurrently, `!uvwxyz`, with B active and the recovery warned once. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts × R3-1: listeners that queue local deletions past the commits a read makes first, while a remote insert waits 48ms FAIL |unit| ... > R3-1: listeners that queue local deletions past the commits a read makes first, while a remote insert waits AssertionError: expected 'paragraph("tuvwxyz")' to be 'paragraph("!uvwxyz")' // Object.is equality Expected: "paragraph("!uvwxyz")" Received: "paragraph("tuvwxyz")" Tests 1 failed | 9 passed (10) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts (10 tests) Tests 10 passed (10) $ pnpm exec vitest run --project unit --maxWorkers=24 packages/lexical-y/ packages/lexical/src Test Files 245 passed | 2 skipped (247) Tests 7409 passed | 40 expected fail | 2 skipped (7451) ``` `pnpm run ci-check` passes. --- .../unit/regressions/GuardedRead.test.ts | 38 +++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 24 ++++++++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index 6033609a21f..70911d43b42 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -361,3 +361,41 @@ describe('R2-1: a read runs only once no local update is pending or queued', () ]); }); }); + +describe('R3-1: local updates a recovery commits are written on the document the editor shows', () => { + test('R3-1: listeners that queue local deletions past the commits a read makes first, while a remote insert waits', async () => { + const ALPHABET = 'abcdefghijklmnopqrstuvwxyz'; + const t = await setup(() => $getRoot().append($p(ALPHABET))); + t.offline(); + t.a.update(() => void $selectIn([0], 0).insertText('!')); + // B deletes its first character 20 times: once pending when A's insert + // arrives, then from a listener of each commit that queues the next + // deletion and throws, so that each waits for a commit of its own (past + // the 16 a read commits first). + let left = 19; + t.b.editor.registerUpdateListener(({tags}) => { + if (left > 0 && !tags.has('collaboration')) { + left--; + t.b.editor.update(() => void $selectIn([0], 0, 1).removeText()); + throw new Error('app failure'); + } + }); + t.b.editor.update(() => void $selectIn([0], 0, 1).removeText()); + const caught = catchMicrotasks(); + await expect(t.sync()).resolves.toBeUndefined(); + await t.sync(); + expect(caught).toHaveLength(19); + expect(left).toBe(0); + expect(t.b.warnings).toEqual([ + 'a remote change could not be read: local updates were still pending after 16 commits', + ]); + const fresh = await freshReader(t.a); + // As if the two edits had met concurrently: A's insert kept, B's 20 + // deletions applied to B's own text. + const expected = `paragraph("!${ALPHABET.slice(20)}")`; + expect(fresh.describe()).toBe(expected); + expect(t.a.describe()).toBe(expected); + expect(t.b.describe()).toBe(expected); + expect(t.b.binding.status).toBe('active'); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index dd08a4041a0..0c340b5cae4 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -176,7 +176,7 @@ type GuardedRead = { /** * Runs right before the read's update, once the local updates still * pending have committed (see commitPending): what the read takes from - * the editor as those updates left it. + * the editor as those updates left it. Not where the read does not run. */ readonly before?: () => void; /** The read, inside the update. */ @@ -1805,6 +1805,21 @@ export class YBinding { } } + /** + * An import starts (see guardedRead, once the local updates pending before + * it were committed): the binding sees the document as it is, the changes + * Yjs applied and it has not read included, until the transaction being + * read ends (see onAfterTransaction). Invariant: never before those + * updates are written. Why: they were made on the document the editor + * shows, without those changes (see Unread). Outside a transaction being + * read, nothing is unread and nothing changes. + */ + private startReading(): void { + if (this.unread !== null) { + this.reading = true; + } + } + private readChange(tr: Transaction): void { if ( tr.origin === this || @@ -1936,7 +1951,7 @@ export class YBinding { } // The read sees the document as it is: this change, and those // applied since, which are read after it (see Reader). - this.reading = true; + this.startReading(); cursor = this.pendingCursor; this.pendingCursor = null; if (undoing) { @@ -2058,6 +2073,9 @@ export class YBinding { {error, reason}, ); }, + // The rebuild reads the document as it is, once the local updates still + // pending were written on it as the editor showed it. + before: () => this.startReading(), recover: rebuildError => this.failClosed(rebuildError, shown), tag: REMOTE_TAGS, }); @@ -2105,7 +2123,7 @@ export class YBinding { ), }; } - if (read.before !== undefined) { + if (failure === null && read.before !== undefined) { read.before(); } const shown = editor.getEditorState(); From f759ee626a60b4b12b551bf4dff9bd1ade6fb2b7 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Thu, 29 Oct 2026 02:31:19 -0700 Subject: [PATCH 11/12] [lexical-y] Bug Fix: Keep a remote read when an app update listener throws on its commit, instead of rebuilding or opening read only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `YBinding#guardedRead` sent any error thrown by the read's `editor.update` to the read's recovery, as one thrown committing what was read. That included an error thrown by an update listener of the app's, which Lexical runs once the read is committed and the editor shows it. The read was then rebuilt from the document (warning that a remote change could not be read), and when the listener threw on the rebuild's commit too, the binding failed closed: "the document cannot be read", read only, showing the editor as it was before the collaborator's change. Now an error thrown once the editor's state is the read's (Lexical committed it and was running listeners) is the app's: it is rethrown in a microtask, as one from committing the updates pending before a read already is, and the read stands, with what follows it. Only an error from the read itself, or from its commit before the editor shows it, goes to recovery. The recovery's rebuild goes through the same entry point, so a listener that throws on the rebuild's commit no longer fails it either. The microtask rethrow has one helper, `rethrowLater`. ## Test plan New `R-1` cases in `packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts`: B's update listener throws on the first 1, 2, 3 or every commit while B reads A's insert. Each requires B to show A's insert with no warning and stay active, and the next edits of A and B to converge with A's word kept on every client. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts × R-1: it throws on 1 commits while B reads A's insert 35ms × R-1: it throws on 2 commits while B reads A's insert 21ms × R-1: it throws on 3 commits while B reads A's insert 24ms × R-1: it throws on Infinity commits while B reads A's insert 20ms FAIL |unit| ... > R-1: it throws on 1 commits while B reads A's insert AssertionError: expected [ Array(1) ] to deeply equal [] + "a remote change could not be read: app failure", FAIL |unit| ... > R-1: it throws on Infinity commits while B reads A's insert AssertionError: expected [ Array(1) ] to deeply equal [] + "the document cannot be read: app failure", Tests 4 failed | 10 passed (14) ``` (The 2 and 3 cases fail with the same "the document cannot be read" warning.) ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts Tests 14 passed (14) $ pnpm exec vitest run --project unit --maxWorkers=8 packages/lexical-y/ packages/lexical/src Test Files 245 passed | 2 skipped (247) Tests 7413 passed | 40 expected fail | 2 skipped (7455) ``` `pnpm run ci-check` passes. --- .../unit/regressions/GuardedRead.test.ts | 36 +++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 35 +++++++++++++----- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index 70911d43b42..89a137dd786 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -399,3 +399,39 @@ describe('R3-1: local updates a recovery commits are written on the document the expect(t.b.binding.status).toBe('active'); }); }); + +describe('R-1: an update listener of the app that throws on the commit of a read leaves the read standing', () => { + test.each([[1], [2], [3], [Infinity]])( + "R-1: it throws on %d commits while B reads A's insert", + async throws => { + const t = await setup(() => + $getRoot().append($p('t1z t2z t3z'), $p('tail')), + ); + let left = throws; + t.b.editor.registerUpdateListener(() => { + if (left > 0) { + left--; + throw new Error('app failure'); + } + }); + const caught = catchMicrotasks(); + t.a.update(() => void $selectIn([0], 3).insertText(' one')); + await t.sync(); + expect(t.b.warnings).toEqual([]); + expect(t.b.binding.status).toBe('active'); + expect(t.b.describe()).toBe( + 'paragraph("t1z one t2z t3z") / paragraph("tail")', + ); + expect(caught.length).toBeGreaterThan(0); + left = 0; + t.a.update(() => void $selectIn([1], 4).insertText(' two')); + t.b.update(() => void $selectIn([0], 15).insertText(' three')); + await t.sync(); + const expected = + 'paragraph("t1z one t2z t3z three") / paragraph("tail two")'; + expect(t.a.describe()).toBe(expected); + expect(t.b.describe()).toBe(expected); + expect((await freshReader(t.a)).describe()).toBe(expected); + }, + ); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 0c340b5cae4..f2a7b9dd0a0 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -200,6 +200,17 @@ type GuardedRead = { readonly tag?: string | string[]; }; +/** + * Rethrow an app's error (an update or a listener of its own that threw on a + * commit the binding made) in a microtask: an uncaught error, never thrown + * into Yjs or a provider. + */ +function rethrowLater(error: unknown): void { + queueMicrotask(() => { + throw error; + }); +} + /** `error`'s text for a warning; never throws, whatever value was thrown. */ function errorText(error: unknown): string { try { @@ -2098,7 +2109,9 @@ export class YBinding { * provider or `initialize()`: it goes to `read.recover`, with what the * editor showed before the read. A throw committing the pending * updates is the app's (an update or a listener of its own): it is - * rethrown in a microtask, and the read goes on. + * rethrown in a microtask, and the read goes on. So is a throw once the + * read committed (the editor shows what it read, and an update listener + * of the app's threw): the read stands. * - `read.after` runs once the read has committed and nothing threw: after * the update the read is part of, where it is nested in one (an undo the * binding runs in an update of its own, see undo). @@ -2142,6 +2155,8 @@ export class YBinding { settle(); return; } + // The state the read makes, which its commit gives the editor. + let readState: EditorState | null = null; try { editor.update( () => { @@ -2151,6 +2166,7 @@ export class YBinding { if (this.unreadable) { return; } + readState = editor._pendingEditorState; try { read.$read(); } catch (error) { @@ -2172,8 +2188,14 @@ export class YBinding { // The update the read is part of threw: not the read's to recover. throw error; } - // Thrown committing what was read. - failure = {error}; + if (readState !== null && editor.getEditorState() === readState) { + // Thrown once the read had committed, by a listener of the app's: + // the read stands. + rethrowLater(error); + } else { + // Thrown committing what was read. + failure = {error}; + } } if (!nested) { settle(); @@ -3593,11 +3615,8 @@ export class YBinding { } } catch (error) { // An update or a listener of the app's threw: its error is rethrown - // in a microtask (an uncaught error), never into Yjs or a provider, - // and the updates still pending commit. - queueMicrotask(() => { - throw error; - }); + // in a microtask, and the updates still pending commit. + rethrowLater(error); } } return !left(); From 81f87e85a187a4750e78fda2a0a9dd3aa9e09f57 Mon Sep 17 00:00:00 2001 From: Gerard Rovira Date: Thu, 29 Oct 2026 03:23:29 -0700 Subject: [PATCH 12/12] [lexical-y] Bug Fix: Fail closed again when the editor cannot render what a remote read brings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `YBinding#guardedRead` keeps a read that an app update listener threw on, and recovers from one whose commit threw otherwise. It told the two apart by the editor's state being the read's. Lexical gives the editor the new state before it reconciles the DOM, though, so a class that throws rendering content the read brought (`createDOM`) also left the editor's state the read's: the binding kept the read and stayed active with no warning, on a document this editor cannot render. Before the app listener case was told apart, it failed closed ("the document cannot be read"). Now the phase decides: Lexical marks the read's state read only once it has reconciled it, before it runs the commit's listeners. A throw once the state is marked is a listener's (the app's): rethrown in a microtask, and the read stands. A throw before it, rendering what was read, goes to recovery as before: the document is rebuilt, and where that throws too, the binding fails closed. So does a throw on the commit of an update queued behind the read (the editor's state is no longer the read's), where the throw can have skipped writing that update. This does not depend on the order the app's listeners and the binding's were registered in. ## Test plan - New `packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts` (jsdom, the reader's editor attached to a DOM root): A appends a block whose class throws in `createDOM`; B must open read only with one "the document cannot be read" warning. - `R-1` in `regressions/GuardedRead.test.ts` now runs each throw count with the throwing listener registered after the binding's and before it (an app that registers its listeners before the binding starts); all keep the read. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts × R-1: a class that throws rendering stored content still fails closed 160ms AssertionError: expected 'active' to be 'readonly' // Object.is equality Tests 1 failed | 18 passed (19) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts Tests 19 passed (19) $ pnpm exec vitest run --project unit --maxWorkers=24 packages/lexical-y/ packages/lexical/src Test Files 246 passed | 2 skipped (248) Tests 7426 passed | 40 expected fail | 2 skipped (7468) ``` `pnpm run ci-check` passes. --- .../unit/regressions/GuardedRead.test.ts | 39 ++++++++----- .../regressions/GuardedReadRender.test.ts | 55 +++++++++++++++++++ packages/lexical-y/src/internal/Binding.ts | 18 ++++-- 3 files changed, 94 insertions(+), 18 deletions(-) create mode 100644 packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts index 89a137dd786..5818404ab2a 100644 --- a/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedRead.test.ts @@ -15,7 +15,7 @@ // the binding failed closed. Each case is named after its finding. import * as Y from '@y/y'; -import {$createTextNode, $getRoot} from 'lexical'; +import {$createTextNode, $getRoot, type LexicalEditor} from 'lexical'; import {afterEach, describe, expect, test, vi} from 'vitest'; import {Reader} from '../../../internal/Read'; @@ -401,19 +401,32 @@ describe('R3-1: local updates a recovery commits are written on the document the }); describe('R-1: an update listener of the app that throws on the commit of a read leaves the read standing', () => { - test.each([[1], [2], [3], [Infinity]])( - "R-1: it throws on %d commits while B reads A's insert", - async throws => { - const t = await setup(() => - $getRoot().append($p('t1z t2z t3z'), $p('tail')), + test.each( + [1, 2, 3, Infinity].flatMap(n => [ + [n, 'after'], + [n, 'before'], + ]), + )( + "R-1: it throws on %d commits while B reads A's insert (registered %s the binding's)", + async (throws, order) => { + let left = 0; + let armed: LexicalEditor | null = null; + const listener = (editor: LexicalEditor) => + editor.registerUpdateListener(() => { + if (editor === armed && left > 0) { + left--; + throw new Error('app failure'); + } + }); + const t = await setup( + () => $getRoot().append($p('t1z t2z t3z'), $p('tail')), + order === 'before' ? {prepare: listener} : {}, ); - let left = throws; - t.b.editor.registerUpdateListener(() => { - if (left > 0) { - left--; - throw new Error('app failure'); - } - }); + if (order === 'after') { + listener(t.b.editor); + } + armed = t.b.editor; + left = throws as number; const caught = catchMicrotasks(); t.a.update(() => void $selectIn([0], 3).insertText(' one')); await t.sync(); diff --git a/packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts new file mode 100644 index 00000000000..8b95f380b67 --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/GuardedReadRender.test.ts @@ -0,0 +1,55 @@ +/** + * 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. + * + */ + +// A read that the editor cannot render (jsdom: the reader's editor is +// attached to a DOM root, so Lexical reconciles what it reads). Lexical +// gives the editor the read's state before it reconciles it, so a throw +// rendering it comes after the editor shows the read's state, and before +// its update listeners run (see YBinding#guardedRead). + +import {$create, $getRoot, ElementNode} from 'lexical'; +import {describe, expect, test} from 'vitest'; + +import {$p, NODES, type Peer, setup} from '../../utils/harness'; + +/** A block whose class throws rendering it, as on a client that cannot. */ +class UnrenderableNode extends ElementNode { + $config() { + return this.config('test_unrenderable', {extends: ElementNode}); + } + createDOM(): HTMLElement { + throw new Error('cannot render'); + } + updateDOM(): boolean { + return false; + } +} + +function attach(peer: Peer) { + const element = document.createElement('div'); + element.contentEditable = 'true'; + document.body.appendChild(element); + peer.editor.setRootElement(element); +} + +describe('R-1: a read the editor cannot render', () => { + test('R-1: a class that throws rendering stored content still fails closed', async () => { + const t = await setup(() => $getRoot().append($p('hello')), { + nodes: [...NODES, UnrenderableNode], + }); + attach(t.b); + t.a.update(() => + $getRoot().append($create(UnrenderableNode).append($p('inside'))), + ); + await expect(t.sync()).resolves.toBeUndefined(); + expect(t.b.binding.status).toBe('readonly'); + expect(t.b.warnings).toEqual([ + expect.stringMatching(/^the document cannot be read: /), + ]); + }); +}); diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index f2a7b9dd0a0..c420a8f7797 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -2110,8 +2110,8 @@ export class YBinding { * editor showed before the read. A throw committing the pending * updates is the app's (an update or a listener of its own): it is * rethrown in a microtask, and the read goes on. So is a throw once the - * read committed (the editor shows what it read, and an update listener - * of the app's threw): the read stands. + * read is committed and rendered (a listener of the app's threw): the + * read stands. * - `read.after` runs once the read has committed and nothing threw: after * the update the read is part of, where it is nested in one (an undo the * binding runs in an update of its own, see undo). @@ -2188,9 +2188,17 @@ export class YBinding { // The update the read is part of threw: not the read's to recover. throw error; } - if (readState !== null && editor.getEditorState() === readState) { - // Thrown once the read had committed, by a listener of the app's: - // the read stands. + if ( + readState !== null && + (readState as EditorState)._readOnly && + editor.getEditorState() === readState + ) { + // Thrown by a listener of the app's on the read's own commit, once + // Lexical had committed and rendered the read (it marks the state + // read only after reconciling it, before its listeners run): the + // read stands. Thrown before (rendering what was read), or on the + // commit of an update queued behind the read (where the throw can + // have skipped writing that update), it is recovered from. rethrowLater(error); } else { // Thrown committing what was read.