Repository navigation
[lexical-website][lexical-y] Documentation Update: Record that a Yjs UndoManager destroyed without a clear keeps what its steps protected (D98) - #9715
Conversation
…UndoManager destroyed without a clear keeps what its steps protected (D98)
## Description
When the binding stops (`binding.dispose()`, or the cleanup its start returns: an editor unmounted or mounted again), it destroys its Yjs `UndoManager` without clearing it, and Yjs destroys an `UndoManager` the same way when its document is destroyed. Yjs's `UndoManager#destroy` does not clear, so the items its steps kept stay kept until the document reloads: a character deleted in a table cell keeps the cell, its row and its table, and a table a collaborator deletes afterwards stays in that client's document (and in any state it encodes). Measured over 50 cycles per table size with one character deleted and one word bolded in one cell: 108 to 175 bytes per table from a 1x1 to a 4x4 table, the same on the release before D88's own-format keep, with that keep turned off and on the current release, and none when the history was cleared before stopping; more cells edited keep more (about 1.3 KB with one character deleted in every cell of a 4x4 table). Any editor that destroys a Yjs `UndoManager` without clearing it first keeps the same items.
This was weighed as an accepted limit rather than having the binding clear at stop: Yjs's clear releases with one shared keep flag, so it would also release what another `UndoManager` on the document (an app's, or a second binding's) still keeps for its own undo, which a later collection could then drop.
- `DECISIONS.md`: new row D98 (`ACCEPTED LIMIT`); D88's "one deleted after the binding stops is collected" now says it holds when the history was cleared before the stop (the same sentence in y.md points to the new item).
- `y.md` Limits: what happens, that binding the same document again does not free it, and how to avoid or free it (`binding.undoManager?.clear()` while the binding is started, before stopping and before the delete, where no other undo manager on the document still needs what it kept; a table deleted while the history held such a step stays after a later clear, as releasing collects nothing; a reload, or a server or relay that loads the state with garbage collection on, compacts it).
- New `regressions/UndoManagerStopWithoutClear.test.ts` pins the documented outcome with Yjs garbage collection on: stopped without a clear, the deleted table, row and cell stay in the client's document (and a new document loaded from its state collects them); cleared before stopping, none stay; cleared only after the table was deleted, they stay.
## Test plan
### Before
No test pinned the outcome. The new test's oracle rejects the alternative the row weighs: with the binding's stop changed to clear its undo manager before destroying it (local experiment, not part of this change), the "without a clear" cell fails:
```
$ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoManagerStopWithoutClear.test.ts
× a table a collaborator deletes stays in B’s document when B stopped without a clear (gc on) 80ms
Tests 1 failed | 2 passed (3)
```
### After
```
$ ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoManagerStopWithoutClear.test.ts
Tests 3 passed (3)
$ LEXICAL_Y_GC=1 ./node_modules/.bin/vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoManagerStopWithoutClear.test.ts
Tests 3 passed (3)
```
`tsc -p tsconfig.test.json` and `eslint` on the new file pass. No source change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
52c0538 to
85f4c8f
Compare
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good. D98 matches what the binding does: it destroys its UndoManager without clearing it when it stops. The new regression test tells the documented outcome apart from the alternative: it fails if the stop clears first and passes on this branch. A few optional, non-blocking suggestions:
- Keep D88 and the previous Limits bullet consistent with D98. The D88 row in
packages/lexical-y/DECISIONS.mdandpackages/lexical-website/docs/collaboration/y.mdlines 1186–1187 still say a table deleted after the binding stops "is collected". With D98 that holds only once the undo history was cleared, so the page now reads "collected" and then "stays" in consecutive bullets. Consider qualifying both sentences or pointing them to D98.packages/lexical-y/AGENTS.mdasks a pull request that changes a recorded limit to update its row. binding.undoManagercan be null (y.mdline 1196). The getter returnsUndoManager | null: it is null withcaptureTimeout: nulland after the binding stops. Sobinding.undoManager.clear()fails strict null checks, andbinding.undoManager?.clear(), called while the binding is still started, is the safer form to document.- Say when the remedy is safe. The bullet explains that the binding does not clear at stop because a clear releases what another undo manager on the document keeps. Adding that the app's own call is safe when no other undo manager (the app's, or a second binding's) needs its history would make the advice self-contained.
- Test nit:
UndoManagerStopWithoutClear.test.tsline 105 checks only that three deleted containers remain. Asserting they are the table, row and cell items, by id, would pin the documented claim exactly.
Docs and tests need no changeset (CONTRIBUTING.md, "Changesets").
| @@ -1187,6 +1187,17 @@ not merge as a user might expect: | |||
| the binding stops is collected. When the binding stops it lets them go, | |||
There was a problem hiding this comment.
Optional: with D98 below, "one deleted after the binding stops is collected" holds only once the undo history was cleared; a step left on the stack keeps the table, as the next bullet says. Qualifying this sentence (and the same one in D88 in packages/lexical-y/DECISIONS.md), or pointing it to D98, keeps the two bullets from reading "collected" then "stays".
There was a problem hiding this comment.
Fixed in 3e57d1e: the y.md sentence now says it is collected when the undo history was cleared before the stop (pointing to the next item), and D88's same sentence says so too and points to D98.
| a character deleted in a table cell keeps the cell, its row and its | ||
| table. A table a collaborator deletes later then stays in that client's | ||
| document, and in any state it encodes, until it reloads: about 110 to | ||
| 175 bytes per table. Call `binding.undoManager.clear()` before stopping, |
There was a problem hiding this comment.
Optional: binding.undoManager is typed UndoManager | null (null with captureTimeout: null, and after the binding stops), so binding.undoManager?.clear(), called while the binding is still started, is the form that type-checks. Saying the call is safe when no other undo manager on the document needs its history would also make the remedy self-contained.
There was a problem hiding this comment.
Fixed in 3e57d1e: D98 and y.md say binding.undoManager?.clear(), called while the binding is started, before stopping and before the delete, where no other undo manager on the document still needs what it kept (a clear releases that too). They also say a clear after the table was deleted does not free it (releasing collects nothing), with a test cell for that case.
| // The table, its row and its cell (the cell's paragraph is a line | ||
| // of the cell, not a container of its own). A binding that cleared | ||
| // at stop would leave none (checked: 0), at the cost D98 names. | ||
| expect(left).toHaveLength(3); |
There was a problem hiding this comment.
Nit: this passes for any three deleted containers. Capturing the table, row and cell items' ids from B's document before A's delete, and asserting left equals them, would pin the documented "the cell, its row and its table" exactly.
There was a problem hiding this comment.
Fixed in 3e57d1e: the test records the Y types A's delete removes from B's document (the table, row, cell and the cell's line marker) and asserts that the three left behind are among them, and that exactly three stay.
zurfyx
changed the title
85f4c8f to
3e57d1e
Compare
3e57d1e to
cd69741
Compare
There was a problem hiding this comment.
Looks good to me. D98 matches what the binding does today: it destroys its UndoManager without a clear when it stops (packages/lexical-y/src/internal/Binding.ts:1362). The new regression test also tells the documented outcome apart from a binding that clears at stop. Two optional, non-blocking suggestions:
- The retention depends on edit order (
packages/lexical-y/DECISIONS.md, row D98;packages/lexical-website/docs/collaboration/y.md, lines 1194-1196). Yjs'skeepItemstops at the first ancestor that is already kept. Suppose the binding already kept the cell for one of its own format items (D88) before the character was deleted. Then the undo step keeps only the character, and the binding's release at stop unkeeps the cell, row and table, so a table deleted later is collected. With the test's table, deleting a character and then bolding a word leaves 3 deleted containers in B; bolding and then deleting leaves 0. Saying "can keep", or naming the order ("where the delete came before any format the binding still keeps in that cell"), would make the row match what a reader measures. The advice to clear before stopping is right in either order.packages/lexical-y/AGENTS.mdasks for accepted limits to be documented in y.md's Limits, so it is worth keeping the two precise together. - Make the cleared test cell non-vacuous (
packages/lexical-y/src/__tests__/unit/regressions/UndoManagerStopWithoutClear.test.ts, line 142). Thebeforemode asserts only thatleftis empty. If A's delete ever stopped reaching B, that cell would still pass. Movingexpect(removed).toHaveLength(3)above theifmakes every cell prove the table was deleted first.
No changeset is needed: AGENTS.md says tests and website changes need none, and the decision register is not part of the published package.
| clearing it, as Yjs does when a document is destroyed, and Yjs keeps what | ||
| that manager's steps protected: a character deleted in a table cell keeps | ||
| the cell, its row and its table. A table a collaborator deletes afterwards |
There was a problem hiding this comment.
Optional: this holds when the delete is the first thing to keep the cell. Yjs's keepItem stops at the first ancestor that is already kept, so if the binding kept the cell for one of its own format items (D88) before the character was deleted, the undo step keeps only the character, and the binding's release at stop unkeeps the cell, row and table. With the test's table, bold then delete leaves 0 deleted containers in B, and delete then bold leaves 3. Writing "can keep", or naming the order, would match what a reader measures (the same applies to the D98 row).
There was a problem hiding this comment.
Good catch on keepItem stopping at an already kept ancestor. Deferred to a follow-up to keep this head stable for landing: D98 and y.md will say the table can stay (when the delete was the first thing to keep the cell, before any format the binding still keeps there), with a cell for the bold-then-delete order.
| } | ||
| const left = deletedContainers(t.b.doc); | ||
| if (clear === 'before') { | ||
| expect(left).toEqual([]); |
There was a problem hiding this comment.
Nit: this cell passes even if A's delete never reached B. Moving expect(removed).toHaveLength(3) above the if makes every mode prove the table was deleted first.
There was a problem hiding this comment.
Agreed; deferred to the same follow-up (assert the delete reached B in every mode).
Description
When the binding stops (
binding.dispose(), or the cleanup its start returns: an editor unmounted or mounted again), it destroys its YjsUndoManagerwithout clearing it, and Yjs destroys anUndoManagerthe same way when its document is destroyed. Yjs'sUndoManager#destroydoes not clear, so the items its steps kept stay kept until the document reloads: a character deleted in a table cell keeps the cell, its row and its table, and a table a collaborator deletes afterwards stays in that client's document (and in any state it encodes). Measured over 50 cycles per table size with one character deleted and one word bolded in one cell: 108 to 175 bytes per table from a 1x1 to a 4x4 table, the same on the release before D88's own-format keep, with that keep turned off and on the current release, and none when the history was cleared before stopping; more cells edited keep more (about 1.3 KB with one character deleted in every cell of a 4x4 table). Any editor that destroys a YjsUndoManagerwithout clearing it first keeps the same items.This was weighed as an accepted limit rather than having the binding clear at stop: Yjs's clear releases with one shared keep flag, so it would also release what another
UndoManageron the document (an app's, or a second binding's) still keeps for its own undo, which a later collection could then drop.DECISIONS.md: new row D98 (ACCEPTED LIMIT); D88's "one deleted after the binding stops is collected" now says it holds when the history was cleared before the stop (the same sentence in y.md points to the new item).y.mdLimits: what happens, that binding the same document again does not free it, and how to avoid or free it (binding.undoManager?.clear()while the binding is started, before stopping and before the delete, where no other undo manager on the document still needs what it kept; a table deleted while the history held such a step stays after a later clear, as releasing collects nothing; a reload, or a server or relay that loads the state with garbage collection on, compacts it).regressions/UndoManagerStopWithoutClear.test.tspins the documented outcome with Yjs garbage collection on: stopped without a clear, the deleted table, row and cell stay in the client's document (and a new document loaded from its state collects them); cleared before stopping, none stay; cleared only after the table was deleted, they stay.Test plan
Before
No test pinned the outcome. The new test's oracle rejects the alternative the row weighs: with the binding's stop changed to clear its undo manager before destroying it (local experiment, not part of this change), the "without a clear" cell fails:
After
tsc -p tsconfig.test.jsonandeslinton the new file pass. No source change.