Repository navigation
[lexical-y] Bug Fix: Keep a comment brought back by undoing its removal off the rest of the document when its author undid the text it was on - #9687
Draft
zurfyx wants to merge 1 commit into
Conversation
…al off the rest of the document when its author undid the text it was on
## Description
A types "yy " and comments it (or bolds it) in one undo step. After a sync,
B removes the comment while A undoes its step, and after the next sync B
undoes the removal: the comment covers the rest of that paragraph and every
paragraph after it, on every client, with garbage collection on or off.
Stored items: A's step writes an opener `{c1=true}`, the text and a closer
`{c1=null}`. B's removal deletes A's opener and relies on A's closer. A's
concurrent undo deletes the text and its own closer (it keeps the closer
only when it has seen the removal, so the sequential version was fine). B's
undo brings the opener back as a copy, and `closeRestoredRuns` finds the end
the opener's own write gave its run (`changeEnd`), deleted, but wrote an end
there only when content between the copy and that end was shown. Here that
content is the text A's undo deleted, so no end was written and the copy's
run went on to the end of the document.
`closeRestoredRuns` now writes that end also when content follows it before
the next live item of that key. It is the end the rule already uses; nothing
else changes (the 15,000-seed multi-value view property is identical on
every seed, gc on and off).
## Test plan
New `regressions/UndoRestoredRunOverDeletedText.test.ts`: 8 shapes (a comment
on typed text; two comments and more typing before them; typed and commented
in two undo steps; bold on typed text; text typed at the end of the document;
at the end of a paragraph; the same comment id of the other client live after
it; another comment id live after it), A undoing while B removes and after B
removed, gc off and on, both client id orders (64 cells).
### Before
```
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunOverDeletedText.test.ts
× comment on typed text; A undoes while B removes (gc off, ids 1,2)
× two comments on typed text, then more typing before them; A undoes after B removed (gc off, ids 1,2)
× comment on typed text, typed and commented in two steps; A undoes while B removes (gc off, ids 1,2)
...
- "two two two",
- "three three three",
+ "two two two[c1]",
+ "three three three[c1]",
Tests 28 failed | 36 passed (64)
```
The same 28 fail with `LEXICAL_Y_GC=1`.
### After
```
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunOverDeletedText.test.ts
Tests 64 passed (64)
$ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunOverDeletedText.test.ts
Tests 64 passed (64)
$ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y
Tests 19153 passed | 304 expected fail | 130 skipped
$ LEXICAL_Y_GC=1 vitest --run --project unit packages/lexical-y
Tests 19152 passed | 307 expected fail | 130 skipped
$ pnpm run ci-check
exit 0
```
Randomized, seeds not run before, gc off and on, main vs this change: the
multi-value view property (15,000 seeds) identical on every seed; the comment
undo-spread fuzz (3,000 seeds), the run-sequence fuzz (1,000) and the
comment-run fuzz (500) pass on both.
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
November 1, 2026 02:29
|
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: |
zurfyx
marked this pull request as draft
November 1, 2026 02:46
![meta-code-review[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good, thanks! The new condition in closeRestoredRuns is narrow. It fires only when the restored opener's own end is deleted, nothing between the two is shown, and content follows. It ends the run where the existing rule already ends it. The new regression test fails on main and passes here, and it checks both peers and a fresh read of the encoded state.
A few optional, non-blocking suggestions:
- Changeset
Verify:paragraph (.changeset/y-restored-run-end-past-deleted-text.md). CONTRIBUTING.md (Changesets) and AGENTS.md ask that a fix that changes observable behavior carry aVerify:paragraph;.changeset/lexical-y-undo-run-end.mdshows the shape. For example:Verify: type text and comment or bold it in one undo step; undo that step while a collaborator removes the comment or bold; when the collaborator undoes the removal, no client shows it on the rest of the paragraph or the paragraphs after it. - JSDoc placement (
packages/lexical-y/src/internal/Binding.ts:535-559).showsPastwas inserted betweenopensRun's doc comment andopensRunitself, so that comment now sits aboveshowsPast's andopensRunhas none. MovingshowsPastabove line 535, or belowopensRun, keeps both documented. - A redo cell (
packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunOverDeletedText.test.ts).closeRestoredRunsalso runs after a redo (Binding.ts:5578-5581). A cell where B redoes the removal and A then redoes its step would cover the new branch on that path too. This is about coverage only.
| * Whether live format item `item` changes its key's value: the nearest live | ||
| * item of its key before it gives another value (none gives null). | ||
| */ | ||
| /** |
There was a problem hiding this comment.
Optional: showsPast landed between opensRun's doc comment (lines 535-538) and opensRun, so that comment no longer documents anything and opensRun has none. Moving showsPast and its comment above line 535, or below opensRun, keeps both documented.
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] Keep a comment or format that undoing its removal brings back off the rest of the document when its author undid the text it was on at the same time |
There was a problem hiding this comment.
Optional: CONTRIBUTING.md (Changesets) asks that a fix that changes observable behavior carry a Verify: paragraph. For example:
Verify: type text and comment or bold it in one undo step; undo that step while a collaborator removes the comment or bold; when the collaborator undoes the removal, no client shows it on the rest of the paragraph or the paragraphs after it.
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 (draft). The fix below guesses which end belongs to which write, and an independent review found a case it gets wrong that
maingets right. Kept as a record; the failing cells move to a test-only pull request astest.failspins.Why it is parked
closeRestoredRunstakes the end the restored opener's write gave its run fromchangeEnd: the next item of the key with another value among the author's next 64 structs, in clock order. That scan cannot tell the original write's own end from the end of a later, separate write by the same author: both are the same client's items of the same key, contiguous in clocks, with no content between.mainonly writes an end there when content is shown between the copy and that item, which happened to hide the guess in the case below; this change also writes it when content follows, and so acts on the guess.Counterexample (gc off, both client id orders, sequential and concurrent): A types
YYbeforetailand commentsYYtailin one step, its run ending at an end the document already held. A then removes the comment fromtailin a separate step and undoes that. B removes the comment while A deletesYY; B undoes its removal.mainrestores the comment ontail; this change writes a null at the separate (undone) write's boundary andtailloses it, on every client and a fresh read (8 of 16 cells; with gc on the unrelated deleted item is collected on B and the scan finds nothing).The restoring client cannot prove ownership locally: it does not hold the author's undo stack, and nothing stored marks where one write ends. Fixing this needs provenance (which write an end belongs to), not another proximity rule.
The bug it addressed (still open)
A types text and comments (or bolds) it in one undo step; after a sync, B removes the value while A undoes its step; after the next sync, B undoes its removal. B's undo brings A's opener back as a copy; the closer that ended it was A's own and A's concurrent undo deleted it, together with all the text the run was on, so the copy's run covers the rest of the document. 28 of the 64 cells in
regressions/UndoRestoredRunOverDeletedText.test.tsfail onmain(gc on and off); this change passed all 64, and an independent review's matrices showed 532 improvements against 8 regressions (the case above).Original description:
Description
A types "yy " and comments it (or bolds it) in one undo step. After a sync,
B removes the comment while A undoes its step, and after the next sync B
undoes the removal: the comment covers the rest of that paragraph and every
paragraph after it, on every client, with garbage collection on or off.
Stored items: A's step writes an opener
{c1=true}, the text and a closer{c1=null}. B's removal deletes A's opener and relies on A's closer. A'sconcurrent undo deletes the text and its own closer (it keeps the closer
only when it has seen the removal, so the sequential version was fine). B's
undo brings the opener back as a copy, and
closeRestoredRunsfinds the endthe opener's own write gave its run (
changeEnd), deleted, but wrote an endthere only when content between the copy and that end was shown. Here that
content is the text A's undo deleted, so no end was written and the copy's
run went on to the end of the document.
closeRestoredRunsnow writes that end also when content follows it beforethe next live item of that key. It is the end the rule already uses; nothing
else changes (the 15,000-seed multi-value view property is identical on
every seed, gc on and off).
Test plan
New
regressions/UndoRestoredRunOverDeletedText.test.ts: 8 shapes (a commenton typed text; two comments and more typing before them; typed and commented
in two undo steps; bold on typed text; text typed at the end of the document;
at the end of a paragraph; the same comment id of the other client live after
it; another comment id live after it), A undoing while B removes and after B
removed, gc off and on, both client id orders (64 cells).
Before
The same 28 fail with
LEXICAL_Y_GC=1.After
Randomized, seeds not run before, gc off and on, main vs this change: the
multi-value view property (15,000 seeds) identical on every seed; the comment
undo-spread fuzz (3,000 seeds), the run-sequence fuzz (1,000) and the
comment-run fuzz (500) pass on both.