Repository navigation
[lexical-website][lexical-y] Documentation Update: Record that an undo or redo restoring content inside a container keeps it from garbage collection until reload (D99) - #9723
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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: |
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for writing this up — the change looks good. D99 and the new Limits bullet describe the redo case accurately (the 151-byte figure reproduces), and the bound-editor tests in UndoRedoContainerKeeps.test.ts reproduce it with a useful control (the type and type-then-undo cases are collected). The suggestions below are optional and non-blocking.
Optional suggestions
-
Cover an undo that restores a delete, not only a redo (
packages/lexical-website/docs/collaboration/y.mdline 1212,packages/lexical-y/DECISIONS.mdD99). With the same harness and table, deleting one character in the cell, undoing, thenbinding.undoManager?.clear()andstop()also leaves the table, row and cell after a collaborator deletes the table (139 bytes over the reloaded state); the same delete without the undo leaves nothing. That path is more common than a redo, and neither D98 nor D99 describes it today. Retitling to something like "An undo or redo that restores content inside a container…" and adding a delete-then-undo case (expected 3) toCASESwould cover it;packages/lexical-y/AGENTS.mdasks that accepted limits be documented in y.md's Limits. -
Name the step that stays kept (D99, and y.md lines 1214–1218). In the redo case the cell, row and table are already kept after the undo (Yjs keeps everything an
UndoManagertransaction deletes, walking up the parents), sokeepItem(copy, true)stops at the copy; setting only the copy'skeepback tofalseafter the redo still leaves all three containers. What is never released is the keep for the delete recorded in the step the redo pops:popStackItemremoves it from the stack, andclear()only walks steps still on a stack. SinceDECISIONS.mdrecords the reason for later work, and an upstream ask aboutredoItemalone would not change this case, it is worth saying so. -
Make the plain-Yjs test reproduce the retention (
UndoRedoContainerKeeps.test.tslines 156–181). It callsum.undo()with no redo, C1 is already kept byc1.deletebefore that undo, and it asserts that the row and table are released; deleting the table after these steps leaves nothing behind, so it does not back D99's "reproduced on plain Yjs" claim. A one-cell insert →um.undo()→um.redo()→um.clear()→um.destroy()→root.delete(0, 1)leaves the table, row and cell, and a fresh gc document loaded from that state collects them; asserting that would. The line 173 comment creditingkeepItem(copy, true)could go too. -
Test plan output (nit): the
### Aftersection ends at "pass:"; the repository's AGENTS.md (Commits and Pull Requests) asks for the real output of the command that was run.
…o or redo restoring content inside a container keeps it from garbage collection until reload (D99) ## Description When content inside a container (such as a table cell) is deleted—either directly by an edit, or when undoing an insertion—Yjs's `UndoManager` records that deletion on a stack item, keeping the deleted item and walking up the parent chain setting `keep = true` (cell, row, table). When an undo restores that deleted content, or a redo re-creates the inserted text, `popStackItem` removes that deletion step from the stack. Clearing every `UndoManager` and stopping the binding does not release the parent containers: Yjs's `clearUndoManagerStackItem` only calls `keepItem(item, false)` for items in deletions of steps still on the stacks. Because the popped deletion step is no longer on any stack, its parent keep walk is never released (and in the redo case, setting only the re-created copy's keep back to false still leaves all three containers kept). If a collaborator later deletes that container, this client's document and encoded state retain the container's structs until reload (about 139 to 151 bytes for a one-cell table with nine characters). This records this behavior as an accepted limit (D99) in `DECISIONS.md` and `y.md`, alongside the existing residue limits, and adds regression tests reproducing both the bound editors flow (with delete-then-undo and redo) and plain Yjs calls. Closes meta#9721 ## Changeset None needed (documentation and tests only). ## Test plan ### Before No documentation in `y.md` or `DECISIONS.md` recording that an undo or redo restoring content inside a container keeps its parent containers from collection until reload, and no unit test asserting this behavior. ### After Tests in `UndoRedoContainerKeeps.test.ts` pass: ``` $ pnpm vitest run packages/lexical-y/src/__tests__/unit/regressions/UndoRedoContainerKeeps.test.ts RUN v5.0.1 C:/lexical ✓ unit packages/lexical-y/src/__tests__/unit/regressions/UndoRedoContainerKeeps.test.ts (7 tests) 775ms Test Files 1 passed (1) Tests 7 passed (7) Start at 15:19:15 Duration 3.98s ```
a8e01b5 to
49509ea
Compare
Monier-Ayman
changed the title
There was a problem hiding this comment.
Thanks, this looks good. The D99 row, the y.md bullet and the regression test match how Yjs 14's UndoManager behaves, and the byte figures (139 to 151 bytes for the one-cell table) reproduce. Everything below is optional and non-blocking.
- Scope D99 the way D98 is scoped (
packages/lexical-y/DECISIONS.md, D99 row;packages/lexical-website/docs/collaboration/y.md, line 1212). The residue appears only when the undo step's deletion is the first thing to keep the cell, because Yjs'skeepItemstops at an ancestor that is already kept. Suppose the binding has already kept the cell for one of its own format items (D88), for example because a word in the cell was bolded. Then the same type → undo → redo (or delete → undo) → clear → stop → table delete leaves no containers and 0 extra bytes. The D98 row and bullet already say this ("it does when nothing kept the cell before the delete; a cell the binding already kept for a format of its own is let go when it stops"). The same clause here would stop readers expecting residue that isn't there. - Point the neighbouring limits at D99. The D88 bullet (y.md lines 1186-1188, and the D88 row) says a table deleted after the binding stops is collected if the undo history was cleared first. The D98 bullet (lines 1203-1205) says a clear before stopping prevents the residue. The new test's undo/redo cases clear before stopping and still keep the table.
packages/lexical-y/AGENTS.mdasks a pull request that changes a recorded limit to update its row, so adding "(except after an undo or redo inside it; D99)" to those sentences would keep the register consistent. - Add the boundary case to the test. A case that bolds a word in the cell, then types, undoes and redoes, expecting
[], would pin where D99 stops applying.UndoManagerStopWithoutClear.test.tsdoes the same with its "bolded before deleting" case. - PR description. The description still blames
redoItem'skeepItem(copy, true). In these flows that walk stops at the cell the undo step already kept, so the docs' popped-step explanation is the accurate one. The squash commit carries the description, so it is worth updating. AGENTS.md ("Commits and Pull Requests") also asks for the real test output under### After. - Nit: the other Limits bullets open with a short bold label ("An undo manager destroyed without a clear.") and leave out Yjs function names. A short label here, without
popStackItemand the parenthetical about the copy's keep, would match them.
| destroys a Yjs `UndoManager` without clearing it first behaves the same; | ||
| the binding does not clear at stop because a clear also releases what | ||
| another undo manager on the document still keeps for its own undo. | ||
| - **An undo or redo that restores content inside a container keeps that container from collection until reload.** |
There was a problem hiding this comment.
Optional: this only happens when the undo step's deletion is the first keep on the cell (Yjs's keepItem stops at an already-kept ancestor). If the binding already kept the cell for its own format (D88), for example after bolding a word in it, the same flow and the same clear, stop and table delete leave no containers and 0 extra bytes. The D98 bullet above already has this qualifier ("it does when nothing kept the cell before the delete…"); the same clause would fit here and in the D99 row.
| 'stays in document when client deleted, undid, and cleared before stopping', | ||
| 'delete, then undo', | ||
| 3, | ||
| ], |
There was a problem hiding this comment.
Optional: a control that bolds a word in the cell before typing, undoing and redoing, expecting [], would pin where D99 stops applying, as UndoManagerStopWithoutClear.test.ts does with "is collected when B bolded before deleting".
Description
When content inside a container (such as a table cell) is deleted—either directly by an edit, or when undoing an insertion—Yjs's
UndoManagerrecords that deletion on a stack item, keeping the deleted item and walking up the parent chain settingkeep = true(cell, row, table). When an undo restores that deleted content, or a redo re-creates the inserted text,popStackItemremoves that deletion step from the stack.Clearing every
UndoManagerand stopping the binding does not release the parent containers: Yjs'sclearUndoManagerStackItemonly callskeepItem(item, false)for items in deletions of steps still on the stacks. Because the popped deletion step is no longer on any stack, its parent keep walk is never released (and in the redo case, setting only the re-created copy's keep back to false still leaves all three containers kept). If a collaborator later deletes that container, this client's document and encoded state retain the container's structs until reload (about 139 to 151 bytes for a one-cell table with nine characters).This records this behavior as an accepted limit (D99) in
DECISIONS.mdandy.md, alongside the existing residue limits, and adds regression tests reproducing both the bound editors flow (with delete-then-undo and redo) and plain Yjs calls.Closes #9721
Changeset
None needed (documentation and tests only).
Test plan
Before
No documentation in
y.mdorDECISIONS.mdrecording that an undo or redo restoring content inside a container keeps its parent containers from collection until reload, and no unit test asserting this behavior.After
Tests in
UndoRedoContainerKeeps.test.tspass: