Repository navigation
Conversation
…do or redo press changes what the document shows, or empties its stack ## Description Yjs's `UndoManager` skips an undo step only when it changes no item. A step that restores or removes only format items no reader shows is taken alone, and the press shows nothing; the next press then undoes what the first was meant to. On main this happens when a client formats text a collaborator then deletes: the format's items stay around the deleted text, and undoing the format removes only them. Keeping run ends in the collapse (#9617, parked) made such presses more common, and in the randomized sessions of its gate the press that did nothing was sometimes the one meant to remove text. Now `YBinding#applyUndo` compares a digest of what any reader can show (the live content in order with its format values, the attributes and the nested types) before and after each step and, while it is unchanged and steps remain, takes the next step in the same press. A press a stack listener starts while an undo runs takes one step, as before (the running step is not done yet). This is a behaviour change to undo: a press now takes more than one step wherever a step showed nothing. DECISIONS.md D81 records it as `OPEN`, for a maintainer's call; y.md's undo paragraph says it. ## Test plan ### Before `pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoInvisibleSteps.test.ts` on main (1853707): ``` × undoing a format on text a collaborator deleted takes the step before it too (1,2) × undoing a format on text a collaborator deleted takes the step before it too (2,1) ✓ undoing a comment removal on deleted text takes the format change before it too ``` `UNDO_PRESS_SEEDS=200 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/UndoPressProperty.test.ts` on main: ``` AssertionError: expected [ 'seed 2: undo press 0', …(9) ] to deeply equal [] ``` ### After ``` ✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/UndoInvisibleSteps.test.ts (3 tests) ✓ |unit| packages/lexical-y/src/__tests__/unit/UndoPressProperty.test.ts (2 tests) ``` with 200 sessions per client id order, each press checked against a fresh read of the document.
|
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: |
… what the document shows once per undo press
## Description
The review of the undo-press change noted that `visibleDigest` and `contentDigest` sat between `YBinding`'s `@experimental` comment and the class, so the exported class lost its doc comment; they now sit above it. A press also reads what the document shows once before its first step instead of before each step: a step that changed nothing visible leaves it as it was.
## Test plan
### Before
The class declaration had no doc comment; tests as before.
### After
`pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoInvisibleSteps.test.ts packages/lexical-y/src/__tests__/unit/UndoPressProperty.test.ts packages/lexical-y/src/__tests__/unit/UndoRunEnds.test.ts packages/lexical-y/src/__tests__/unit/YjsContract.test.ts`:
```
Tests 144 passed | 81 expected fail (225)
```
![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 the detailed description and the real before/after output. The loop in YBinding#applyUndo does what D81 describes: a step that only restores or removes format items around deleted text is now taken together with the next one, and the regression scenario fails on main and passes here. A few suggestions, then the one decision this needs from a maintainer.
Suggestions
- Per-press cost (
packages/lexical-y/src/internal/Binding.ts:5229and:5239).visibleDigestwalks the whole root before the first step and again after each step, so the common one-step press pays two full walks. In a quick benchmark undoing one typed character (self-check off), a press went from 0.42 ms to 11.74 ms on a 131,998-character document and from 0.33 ms to 24.11 ms at 273,898 characters, in line with the numbers in the description. The changed-items check the description proposes is the right fix;scanStackItemalready iterates the top stack item's inserts and deletes before the pop, which is a natural place for it. Until then,contentDigest(Binding.ts:674) could readc.strfor aContentStringinstead ofJSON.stringify(c.getContent()), which in@y/y14.0.0-rc.29 splits the string into one array entry per character. - Minimality in the property test (
packages/lexical-y/src/__tests__/unit/UndoPressProperty.test.ts:153). A press fails only when nothing changed and the stack is not empty, so an implementation that emptied the stack on every press would pass. SnapshottingY.encodeStateAsUpdateafter each step and requiring every non-final snapshot to read likebeforewould pin the other half of D81 (a press takes the next step only while nothing shows). That check passes on this head over 200 seeds per client-id order. - The command path. The new tests call
binding.undo()outside an update. A case that dispatchesUNDO_COMMAND/REDO_COMMANDon the regression scenario would cover the queued path (queueFirst) that a keyboard undo takes; it behaves correctly on this head. - Nit: where D81 is filed (
packages/lexical-y/DECISIONS.md:61). The row sits in the Storage model table betweenDECIDEDrows; the register's legend and its## Opensection are whereOPENrows live until they are decided (packages/lexical-y/AGENTS.mdasks for the row, which is here).
Question for maintainers (D81)
Should an undo or redo press in @lexical/y keep taking UndoManager steps until a fresh read of the document changes, rather than Yjs's one step per press where a press can show nothing? If yes, can the whole-document digest ship until the changed-items check replaces it, or should that check land in this pull request?
| try { | ||
| // What the document shows before the press: a step that changed | ||
| // nothing visible leaves it as it was. | ||
| const before = visibleDigest(this._root); |
There was a problem hiding this comment.
This digest and the one after each step walk the whole root, so the usual one-step press pays two full walks. Undoing one typed character (self-check off) went from 0.42 ms to 11.74 ms at 131,998 characters and from 0.33 ms to 24.11 ms at 273,898. The changed-items check from the description fixes it; until then, reading c.str for a ContentString in contentDigest (line 674) avoids getContent()'s per-character array.
| t.a.binding.redo(); | ||
| } | ||
| const after = await freshView(t.a); | ||
| if (after === before && stack.length > 0) { |
There was a problem hiding this comment.
This catches a press that shows nothing, but not one that takes a step too many: an applyUndo that emptied the stack on every press would pass. Snapshotting Y.encodeStateAsUpdate(t.a.doc) after each step and requiring every non-final snapshot to read like before would cover that half of D81.
| | D24 | **Yjs's format cleanup is off** in `@lexical/y` containers and on every relay that loads a state (`createYDoc`). A delete keeps the ends of each deleted formatted run, within a bound per format key. Not reported upstream. | The cleanup deletes format items a concurrent writer may still rely on. Upstream already knows that skipping it is the app's trade-off. | `DECIDED` | 10-28 | [#9400], [y.md: Servers][y-servers] | | ||
| | D25 | **Undo is Yjs's `UndoManager`**, scoped to the binding's root and this client's own edits (500 ms capture by default). It never deletes a container a collaborator has put content in (its `deleteFilter`). Do not combine it with `HistoryExtension`. | Collaborative undo must not revert other people's work; keeping a container whole once someone else wrote in it matches y-prosemirror's protected nodes and is cheap to maintain. The `UndoManager` was chosen on 10-23; keeping containers whole was confirmed on 10-30. | `DECIDED` | 10-30 | [#9400], [y.md: Limits][limits] | | ||
| | D26 | **Undo favours people taking turns over concurrent races.** Where a fix makes undo correct for sequential edits at the cost of rare races between an undo and a simultaneous edit of the same text, the sequential case wins. | Taking turns is the common case; a race with a simultaneous edit of the same text is rare. Picked on 10-28, confirmed on 10-30. | `DECIDED` | 10-30 | [#9400] | | ||
| | D81 | **An undo or redo press changes what a fresh read of the document shows, or takes every step there is.** After a step whose whole effect no reader shows, the same press takes the next step. A press that a listener starts while an undo runs takes one step, as before. | Yjs skips only a step that changes no item. A step that restores or removes only format items no reader shows (a format change on text a collaborator then deleted: the format's items stay around the deleted text, and undoing the format removes only them) changed items but nothing anyone sees, so the press did nothing visible and the next press undid what the first was meant to. What is visible is decided by a digest of what any reader can show (the live content in order with its format values, the attributes and the nested types), compared before and after each step. It changes how many steps a press takes wherever a step showed nothing, which is a behaviour change to undo: for a maintainer's call. The alternative is Yjs's rule as it is, a press that can show nothing. Keeping run ends in the collapse (#9617, parked) made such presses more common. | `OPEN` | 10-30 | [#9617] | |
There was a problem hiding this comment.
Nit: this OPEN row sits in the Storage model table among DECIDED rows; the register keeps OPEN rows under ## Open (Question / Where it stands) until they are decided.
zurfyx
added a commit
that referenced
this pull request
Oct 31, 2026
… exact ## Description Review of the test-only change found two expected failures that a real behaviour change could slip past: - The concurrent property test was one `test.fails` over 60 seeds. Since seed 53 fails on main, a change that broke a seed passing on main kept the whole test failing, so nothing noticed. Now one regular test runs every seed and checks everything (no errors, no more writes, the same characters, peers and a fresh reader agreeing); it skips the own-text comparison only on the seeds where main lets a comment spread (53). A `test.fails` per such seed checks only that comparison (#9609). - KeepEndsSessions 3645 shows main's drop of c9 only after the session's last undo. It also flips with #9630 (every undo press changes what the document shows), with no collapse change. Its name and comment now say so. ## Test plan ### Before `CollapseKeepsEndsProperty.test.ts` on main: 2 passed, 2 expected fail; one aggregate `test.fails` per id order. ### After `pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/CollapseKeepsEndsProperty.test.ts packages/lexical-y/src/__tests__/unit/regressions/KeepEndsSessions.test.ts` on main (1853707): ``` ✓ taking turns: every character shows as with no collapse (ids 1,2) ✓ typing ‖ deletes: the document's own text shows as with no collapse (ids 1,2) ✓ taking turns: every character shows as with no collapse (ids 2,1) ✓ typing ‖ deletes: the document's own text shows as with no collapse (ids 2,1) Tests 6 passed | 4 expected fail (10) ``` Seed 53 is the only one of 60 that fails on main, in both id orders, and only on the own text (its other checks pass). `npx tsc -p tsconfig.test.json --noEmit` and eslint pass.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Yjs's
UndoManagerskips an undo step only when it changes no item. A step that restores or removes only format items no reader shows is taken alone, and the press shows nothing; the next press then undoes what the first was meant to. On main this happens when a client formats text a collaborator then deletes: the format's items stay around the deleted text, and undoing the format removes only them. Keeping run ends in the collapse (#9617, parked) made such presses more common, and in the randomized sessions of its gate the press that did nothing was sometimes the one meant to remove text.Now
YBinding#applyUndocompares a digest of what any reader can show (the live content in order with its format values, the attributes and the nested types) before and after each step and, while it is unchanged and steps remain, takes the next step in the same press. A press a stack listener starts while an undo runs takes one step, as before (the running step is not done yet). This is a behaviour change to undo: a press now takes more than one step wherever a step showed nothing. DECISIONS.md D81 records it asOPEN, for a maintainer's call; y.md's undo paragraph says it.Test plan
Before
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoInvisibleSteps.test.tson main (1853707):UNDO_PRESS_SEEDS=200 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/UndoPressProperty.test.tson main:After
with 200 sessions per client id order, each press checked against a fresh read of the document.
Open before this can leave draft
OPEN: a maintainer decides whether undo should take more than one step where a step shows nothing.KeepEndsSessions3645 flips with this change (the session shows main's dropped comment only after an undo that this change makes take more steps); remove itstest.failshere then.