Repository navigation
Conversation
|
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 00:09
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: |
…also when the change that wrote its end is undone ## Description An undo (or redo) that brings back a format item opening a value (a comment id, a format, a link, a style, a NodeState value) already ends that run with a null of its own when the item that ended it belongs to another change (`closeRestoredRuns`). Two shapes were missed, and the value then covered everything after it, on every client and in a fresh read of the document: - #9623, sequential: B comments a word; A removes the comment (which deletes B's opener); B undoes its comment (which deletes B's closer); A undoes its removal. The undo brought B's opener back, but B's closer was gone, and with garbage collection on (the default) its content was collected, so the binding could no longer find it from B's change (`changeEnd`). - #9620, concurrent: the run's end was still live when the undo ran, but no content showed between the restored item and that end yet, so no null was written; at the same time the end's own change was undone. The binding now notes, for each format item opening a value that a change of this client's deletes, the null that ended its run right after that change (deleted items keep their place, so the end can be found again after collection). When an undo brings the item back and the change's own end cannot be found, the run ends right after that noted end. And a live null of another change that ends the run is now ended before also where no content shows between the restored item and it yet, when that null is the end its change gave a run it opened (`isChangeEnd`): a null by which a change made text inside the run plain (a hole that change restores the value after) is no such end, and ending before it would take the value off the text after the hole. Every such null is written inside the local undo, never while applying a remote change. Recorded as DECISIONS.md D83 (proposed; the number may shift at landing): it writes one null per restored run another change's end closed, also where nothing shows yet. Closes #9623 Closes #9620 ## Test plan - `regressions/UndoRestoredRunEnds.test.ts` (147 cells): the value is a comment id (MarkState), bold, a color style or a NodeState value; a collaborator removes it from the whole word, part of it, or (comments) the whole document; its author undoes its change first, or not; a third client types plain text right after the word, or not; then a redo and another undo; garbage collection on and off; two client id orders. Every client and a fresh read agree, and the value is on the word only or nowhere, never on the typed text or another block. Plus repeated undo and redo of the removal (the live format items stay bounded) and #9620's two client id orders. - `UndoSpreadFuzz.test.ts`: two bound editors synced after every op, random comment adds, removals, formats, typing, deletes and undos; a comment id on two blocks fails (60 seeds by default, `US_SEEDS` for more). - Mutation checks: without the noted end, 20 cells fail; without the end before a live change end where nothing shows yet, #9620's 2 fail. ### Before main (4421e5b): ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts Tests 22 failed | 125 passed (147) × comment, removed from the word, its author undoes it first (gc on, ids 1,2,3) × a comment its author undid does not cover the rest of the document (ids 1,2) $ US_SEEDS=3000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts Tests 336 failed | 2664 passed (3000) ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEnds.test.ts Tests 147 passed (147) $ US_SEEDS=3000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts Tests 3000 passed (3000) ``` Unit tests of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list and lexical-table: 22,273 passed, 190 expected fail, 0 failed. `pnpm run ci-check` passes. Against main with the same seeds: a three-client view property fuzz (5,000 seeds; every client's view after every round, a sentinel line no one edits): 4,993 identical, 7 differ, and in each of the 7 main has a format or comment on text this branch leaves without it: 5 are main's value running onto the untouched last line, and the 2 others, replayed step by step, are main's value spreading onto text that did not have it before the undo (this branch shows the text as it was). The run-sequence fuzz (1,000 seeds) fails the same 6 seeds as main (#9599), and the comment-run fuzz (500 seeds) none on either.
zurfyx
force-pushed
the
zurfyx/y-undo-restored-run-ends
branch
from
October 31, 2026 00:14
2657b20 to
49e675f
Compare
zurfyx
marked this pull request as draft
October 31, 2026 00:29
![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 to me. The noted run end and the isChangeEnd check fix both reported shapes (#9623, #9620), and the new regression grid and fuzz fail on main and pass on this branch. A few optional, non-blocking suggestions:
- A restored run can still spread when the value's author wrote the same key again before the undo (
packages/lexical-y/src/internal/Binding.ts, lines 5339-5342). Steps, with gc on: B boldstwo t; A unbolds it; B undoes; B boldszeroin the first paragraph; A undoes. Both clients and a fresh read then show bold fromtwo tto the end of the document; a color style or a NodeState value does the same. The cause:changeEndis tried first, and once the author's closer has been collected it steps over it and returns the end of the author's next write of that key, so the noted end is never used.mainbehaves the same, so this is not a regression. But it is the class this PR closes, and the method's JSDoc (lines 5290-5292), D83 and the changeset all describe the noted end as the one used.
Putting the noted end first is not enough: it breaks the hole case. Take B boldstwo t, C unboldswo, A unbolds the word, C undoes, A undoes:mainand this branch correctly bring bold back on all oftwo t, while noted-end-first leaveswoplain. What passed the PR's tests, both of these cases and the rest of the@lexical/yunit suite for me: keepchangeEndfirst, but drop its answer when one of the author's structs between the original and that answer has been collected, then fall back to the noted end. If that check importsContentDeleted,YjsContract.test.tsneeds it listed (D14). Perpackages/lexical-y/AGENTS.md, the repro would go insrc/__tests__/unit/regressions/. - Changeset and D83 wording. If (1) is left for a follow-up, narrow
.changeset/y-undo-restored-run-ends.md(line 11) and D83 so they do not promise the case where the author rewrote the key. - D83 status. The row is
DECIDED(10-30), while the description calls it proposed.DECISIONS.md's legend definesDECIDEDas ratified, so it is worth confirming the status and number at landing.
Comment on lines
+5339
to
+5342
| const gone = opensRun(copy) | ||
| ? (changeEnd(doc, original) ?? | ||
| (recorded === undefined ? null : store.getItem(recorded))) | ||
| : null; |
There was a problem hiding this comment.
Optional, non-blocking: changeEnd runs first here, and once the author's closer has been collected (gc on) it steps over it and returns the end of the author's next write of the same key. That item is live and elsewhere, so the noted end below is never read. Repro: B bolds two t; A unbolds it; B undoes; B bolds zero; A undoes. Bold then runs from two t to the end of the document (same on main). The JSDoc above (lines 5290-5292) describes the noted end as the one used. Putting recorded first breaks the hole case (C unbolds wo, then undoes it before A's undo), but keeping changeEnd first and dropping its answer when one of the author's structs between original and that answer has been collected passed both cases and the @lexical/y unit suite for me.
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: the approach (end a run an undo brings back at the end its run had when the item was deleted) cannot tell, without recording provenance, two cases that need opposite answers:
Review also found that the head here (49e675f) can take the value from a later write of the same client where the end's content was collected, and misses the end of a write with more than 64 comment ids (
changeEnd's bound). The tests move to main in a test-only pull request (test.failswhere main fails). A different design is being prototyped: a removal never deletes another change's opener and writes its own null (and restore) over its range, as Yjs's ownformatdoes, so undoing it never brings back an opener whose end is gone.Description
An undo (or redo) that brings back a format item opening a value (a comment
id, a format, a link, a style, a NodeState value) already ends that run with
a null of its own when the item that ended it belongs to another change
(
closeRestoredRuns). Two shapes were missed, and the value then coveredeverything after it, on every client and in a fresh read of the document:
B's opener); B undoes its comment (which deletes B's closer); A undoes its
removal. The undo brought B's opener back, but B's closer was gone, and
with garbage collection on (the default) its content was collected, so the
binding could no longer find it from B's change (
changeEnd).content showed between the restored item and that end yet, so no null was
written; at the same time the end's own change was undone.
The binding now notes, for each format item opening a value that a change
of this client's deletes, the null that ended its run right after that
change (deleted items keep their place, so the end can be found again after
collection). When an undo brings the item back and the change's own end
cannot be found, the run ends right after that noted end. And a live null of
another change that ends the run is now ended before also where no content
shows between the restored item and it yet, when that null is the end its
change gave a run it opened (
isChangeEnd): a null by which a change madetext inside the run plain (a hole that change restores the value after) is
no such end, and ending before it would take the value off the text after
the hole.
Every such null is written inside the local undo, never while applying a
remote change. Recorded as DECISIONS.md D83 (proposed; the number may shift at landing): it writes one null
per restored run another change's end closed, also where nothing shows yet.
Closes #9623
Closes #9620
Changeset
.changeset/y-undo-restored-run-ends.md(@lexical/ypatch,[fix]with aVerify:paragraph).Decision for review
DECISIONS.md D83 is proposed here (the number may shift at landing): an undo that brings back a value brings back its end, at the cost of one null per such run, also where no content shows yet. The concurrent case where a collaborator's write relies on a run end while its writer undoes it ([#9599], [#9622]) stays open; the sequential undo of a write whose end a live run relies on is #9624.
With #9617
#9617 (keep-ends) changes how a delete's collapse treats run ends; this PR adds nulls only inside a local undo and deletes nothing, so the two touch different code paths (
Cursor#collapseGapthere,YBinding#closeRestoredRunshere). Tested together on main + #9617 (6b89fd6) + this change: the same regression tests and fuzz pass (147/147, 3,000/3,000), and against main + #9617 the view property fuzz (5,000 seeds) gives 4,996 identical, 2 last-line spreads of #9617's fixed, and 2 differences where #9617 alone spreads a value onto text that did not have it; run-sequence and comment-run fuzz give the same results as main + #9617.Test plan
regressions/UndoRestoredRunEnds.test.ts(147 cells): the value is acomment id (MarkState), bold, a color style or a NodeState value; a
collaborator removes it from the whole word, part of it, or (comments)
the whole document; its author undoes its change first, or not; a third
client types plain text right after the word, or not; then a redo and
another undo; garbage collection on and off; two client id orders. Every
client and a fresh read agree, and the value is on the word only or
nowhere, never on the typed text or another block. Plus repeated undo and
redo of the removal (the live format items stay bounded) and Bug: [lexical-y] A comment its author undid comes back over the rest of the document when a collaborator deletes across it and undoes one of the deletes at the same time #9620's two
client id orders.
UndoSpreadFuzz.test.ts: two bound editors synced after every op, randomcomment adds, removals, formats, typing, deletes and undos; a comment id
on two blocks fails (60 seeds by default,
US_SEEDSfor more).before a live change end where nothing shows yet, Bug: [lexical-y] A comment its author undid comes back over the rest of the document when a collaborator deletes across it and undoes one of the deletes at the same time #9620's 2 fail.
Before
main (4421e5b):
After
Unit tests of lexical-y, lexical-mark, lexical, lexical-selection,
lexical-list and lexical-table: 22,273 passed, 190 expected fail, 0 failed.
pnpm run ci-checkpasses.Against main with the same seeds: a three-client view property fuzz
(5,000 seeds; every client's view after every round, a sentinel line no one
edits): 4,993 identical, 7 differ, and in each of the 7 main has a format
or comment on text this branch leaves without it: 5 are main's value
running onto the untouched last line, and the 2 others, replayed step by
step, are main's value spreading onto text that did not have it before the
undo (this branch shows the text as it was). The run-sequence fuzz (1,000
seeds) fails the same 6 seeds as main (#9599), and the comment-run fuzz
(500 seeds) none on either.