Repository navigation
Conversation
… run still open before it ## Description Closes #9624. A client comments (or formats) some words and types plain text inside them: the typing writes a null before its text and restores the value after it, relying on the run's own end further on. A collaborator then deletes the text after the typed text, past the run's end: the delete's collapse of format items (D24) can take both the restore and the run's end. When the client undoes its typing, the undo deleted its null too, and the value was left open over everything after it, on every client and in a fresh read. An undo or redo now keeps a null of its change that is the last end of a run still open before it: the nearest live item of the key before it (leaving out what the undo deletes) gives a value, content shows after the null, and the next live item of the key gives another value or there is none. A null after which the run's own value is written again is a hole and is still deleted, so undoing typing inside a run joins the run as before. The nulls are found in one pass over each container that holds a null the undo deletes, before the undo runs (`lastRunEnds`), and `isKeptByUndo` keeps them. Nothing is written. Recorded as DECISIONS.md D84 (the number may shift at landing). With the "retraction wins / restore wins" choice for undoing a removal (#9640, #9633): this is independent of it. Here the value's author is the client that undoes, and the end is lost to a delete's collapse, not to an undo of another change; without the collapse of format items (a measurement build with collapse off) the case does not arise and this change is not needed. ## Test plan `regressions/UndoKeepsLastRunEnd.test.ts` (310 cases): a comment, bold, italic, underline, color or NodeState value given by the typist or a collaborator; plain text typed inside the run; the rest of the run deleted in five ways (to the run's end, past it, to the end of the line, from one character later, in two steps); the typing undone; garbage collection on and off; two id orders: exact extent and text on every client and a fresh read. Controls: no delete (the hole closes), and the run's own value written again over the run after the typing. Plus #9624 as reported (both id orders). Mutation: keeping nothing fails the 2 #9624 cells. Treating a restate of the run's own value as an end passes these cells but changes 102 of 15,000 seeds of a three-client view fuzz against main (taking a value off text that had it); the code here changes none. ### Before main (1853707): ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoKeepsLastRunEnd.test.ts Tests 2 failed | 308 passed (310) × a comment does not run on after a collaborator deleted its end and the typing inside it is undone (#9624, ids 1,2) Expected: ""tail line"" Received: ""tail line"[c2]" ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoKeepsLastRunEnd.test.ts Tests 310 passed (310) ``` Unit tests of lexical-y, lexical-mark, lexical and lexical-selection: 22,377 passed, 190 expected fail, 0 failed. `pnpm run ci-check` passes. Against main with the same seeds: the three-client view fuzz (15,000 seeds; every client's view after every round, with a last line no one edits): 14,944 identical, 53 where main spreads a value onto the last line and this branch does not, 3 bad on both, 0 other differences; against main and a build with collapse off together, 0 seeds where both are right and this branch is not. The restored-extent suites of #9640 fail exactly the same cases as on main. Run-sequence fuzz (1,000 seeds): the same 6 seeds as main; comment-run fuzz (500): none; undo spread fuzz (3,000): the same 336 as main (#9623).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 31, 2026 01:27
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.
The #9624 cells fail on main and pass on this branch, as the Test plan says, and the matrix asserts every client and a fresh read. One regression needs fixing before this can merge.
Blocking: the undo now drops a style (or NodeState) value from text it never touched
lastRunEnds keeps the change's null when the next live item of its key carries a different non-null value (packages/lexical-y/src/internal/Binding.ts, L5143-5147), while the change's own restore after the typed text is skipped because it is in undoing (L5164-5174). p:style holds the whole style string, so restyling part of a run writes exactly such a value, and the null is then kept even with no delete and the restore still live. On a single client:
- Colour
Kilo bb ccred. - Give
cca font size (its style becomescolor: red;font-size: 20px;). - Insert plain
XYafterKilo(the test's$plainAt). - Undo.
main restores Kilo bb to red; this branch leaves bb plain on every client and in a fresh read, in both client id orders. The same happens when a collaborator colours cc blue after the typing. Comments and boolean formats are not affected (their only value is true), which is why the matrix does not catch it.
One direction that passed all 310 cells here plus these cases (I did not run the fuzz suites from the Test plan): treat an undone item of the key whose value equals a pending null's open, which is the change's own restore and still live, as closing the hole.
} else if (value === null) {
// ...as now
} else {
// The change's own restore of the run's value is still live: the null is a hole.
const list = pending.get(key);
if (list !== undefined) {
pending.set(key, list.filter(([, , open]) => !equalFormatValue(value, open)));
}
}Please add the one-client case above to UndoKeepsLastRunEnd.test.ts; packages/lexical-y/AGENTS.md asks that every bug fix land with a regression test in src/__tests__/unit/regressions/.
Optional, non-blocking
DECISIONS.md: D84 decides part of the OPEN D74 (keeping "the run ends of text an undo removes from inside a format run", #9561 and #9593).packages/lexical-y/AGENTS.mdasks a pull request that changes a recorded decision to update its row, so a cross-link between D84 and D74 would help whoever picks up D74 next.UndoKeepsLastRunEnd.test.tslines 18-24 describe another suite (#9623, #9620, cells "pinned as test.fails"); this file has notest.failscell. Thelinkkind,$give'scountparameter and itson: falsebranches are never exercised.
Comment on lines
+5143
to
+5147
| if ( | ||
| content > at && | ||
| (next === undefined || | ||
| (next !== null && !equalFormatValue(next, open))) | ||
| ) { |
There was a problem hiding this comment.
Blocking. A following live item of the key with a different non-null value counts as the run's end here, while the change's own restore after the typed text is skipped (it is in undoing). For p:style (the whole style string) that happens whenever part of the run is restyled, with no delete at all: one client colours Kilo bb cc red, gives cc a font size, inserts plain XY after Kilo and undoes. main restores Kilo bb to red; here bb stays plain on every client and in a fresh read.
One direction that passed all 310 cells plus that case: when the scan meets an undone item of the key whose value equals a pending null's open (the change's own restore, still live), drop that null from pending.
} else {
const list = pending.get(key);
if (list !== undefined) {
pending.set(key, list.filter(([, , open]) => !equalFormatValue(value, open)));
}
}
Comment on lines
+18
to
+24
| // The value an undo brings back covers exactly what it covered before: | ||
| // a collaborator removes a value, its author undoes it (and may write the | ||
| // same key later), a third client may delete text around the value's end, | ||
| // and the collaborator then undoes the removal. On main, with garbage | ||
| // collection on, the restored value runs on past its end (#9623), and with | ||
| // many comment ids or a concurrent undo of the end's change too (#9620): | ||
| // those cells are pinned as test.fails until the fix lands. |
| | 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] | | ||
| | D84 | **An undo keeps a null that is the last end of a run still open before it.** Where an undo or redo would delete a null of its change (the null by which typed text was made plain inside a run of a value) and that null is the run's only end left (no later live item of the key ends the run, and content shows after the null), the null is kept, and the run ends there. | A collaborator's delete can take the run's own end and the restore the typing wrote after its text; deleting the null then left the value open over everything after it, on every client ([#9624]). It follows D26 (taking turns) and writes nothing: it keeps an item. Cost: one pass over each container that holds a null the undo would delete. Without the delete's collapse of format items (D24) the run's end stays and this does not arise. | `DECIDED` | 10-30 | [#9624] | |
There was a problem hiding this comment.
Optional: D84 settles part of the OPEN D74 (keeping the run ends of text an undo removes from inside a format run). packages/lexical-y/AGENTS.md asks a PR that changes a recorded decision to update its row, so a cross-link between D84 and D74 would help.
zurfyx
marked this pull request as draft
October 31, 2026 01:50
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.
Parked (10/30)
Parked as a draft: keeping a run's end at undo time needs to know which change wrote what, the same wall #9633 (ending a restored run) and #9617 (keeping ends at collapse) met. Review found three shapes:
Without the delete's collapse of format items the run's end is never taken and #9624 does not arise (its cells pass with collapse off), so this waits for that decision.
Description
Closes #9624.
A client comments (or formats) some words and types plain text inside them:
the typing writes a null before its text and restores the value after it,
relying on the run's own end further on. A collaborator then deletes the text
after the typed text, past the run's end: the delete's collapse of format
items (D24) can take both the restore and the run's end. When the client
undoes its typing, the undo deleted its null too, and the value was left open
over everything after it, on every client and in a fresh read.
An undo or redo now keeps a null of its change that is the last end of a run
still open before it: the nearest live item of the key before it (leaving out
what the undo deletes) gives a value, content shows after the null, and the
next live item of the key gives another value or there is none. A null after
which the run's own value is written again is a hole and is still deleted, so
undoing typing inside a run joins the run as before. The nulls are found in
one pass over each container that holds a null the undo deletes, before the
undo runs (
lastRunEnds), andisKeptByUndokeeps them. Nothing is written.Recorded as DECISIONS.md D84 (the number may shift at landing).
With the "retraction wins / restore wins" choice for undoing a removal
(#9640, #9633): this is independent of it. Here the value's author is the
client that undoes, and the end is lost to a delete's collapse, not to an
undo of another change; without the collapse of format items (a measurement
build with collapse off) the case does not arise and this change is not
needed.
Changeset
.changeset/y-undo-keeps-last-run-end.md(@lexical/ypatch,[fix]with aVerify:paragraph).Test plan
regressions/UndoKeepsLastRunEnd.test.ts(310 cases): a comment, bold,italic, underline, color or NodeState value given by the typist or a
collaborator; plain text typed inside the run; the rest of the run deleted in
five ways (to the run's end, past it, to the end of the line, from one
character later, in two steps); the typing undone; garbage collection on and
off; two id orders: exact extent and text on every client and a fresh read.
Controls: no delete (the hole closes), and the run's own value written again
over the run after the typing. Plus #9624 as reported (both id orders).
Mutation: keeping nothing fails the 2 #9624 cells. Treating a restate of the
run's own value as an end passes these cells but changes 102 of 15,000
seeds of a three-client view fuzz against main (taking a value off text that
had it); the code here changes none.
Before
main (1853707):
After
Unit tests of lexical-y, lexical-mark, lexical and lexical-selection: 22,377
passed, 190 expected fail, 0 failed.
pnpm run ci-checkpasses.Against main with the same seeds: the three-client view fuzz (15,000 seeds;
every client's view after every round, with a last line no one edits):
14,944 identical, 53 where main spreads a value onto the last line and this
branch does not, 3 bad on both, 0 other differences; against main and a
build with collapse off together, 0 seeds where both are right and this
branch is not. The restored-extent suites of #9640 fail exactly the same
cases as on main. Run-sequence fuzz (1,000 seeds): the same 6 seeds as main;
comment-run fuzz (500): none; undo spread fuzz (3,000): the same 336 as main
(#9623).