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
November 1, 2026 04:16
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
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: |
…9694 (an editor stopped matching its own document in long sessions) Reverts #9679 whole, on top of current `main` (keeps #9684 and #9685). With #9679, a downstream app's long-note sessions end with an editor that no longer matches its own Yjs document (15 of 133 sessions; 0 of 132 on the commit before #9679). Reverting only part of it does not work: the delete rule (D86) alone is worse on a recorded session (2810), and the divergence was traced to one of its hunks (the end a restated run gets at a range's end, which could be linked into another container; #9693 fixes that at the source). This PR is the fallback if #9693 is not taken. Closes #9694 if merged instead of #9693. What this brings back, as before #9679: - A delete collapses the run of deleted content and format items it leaves to a bounded number per format key (D24's bound): the cases it fixed (#9575, #9609, a #9620 cell, #9640's concurrent restore cells) fail again and are pinned with their issues. - No format items kept from garbage collection: with gc on, an undo can again put a value back past its end (#9623). The harness keeps its `LEXICAL_Y_GC` switch (added by #9679), and the cells that fail only with gc on are pinned as `test.fails` with gc on, naming #9623. - The restore rules for #9671 and the restore scan for #9599 (D78) are gone: those cases are open again. `DECISIONS.md` keeps D86 and D88 (now `REVERTED`, with the reason) and reopens D78, rather than rewriting them. `y.md` goes back to the behavior before #9679. The two changesets of #9679 are removed: it was not part of a release, so there is nothing to announce. `UndoSpreadSession2810.test.ts` (added by #9679) is kept: it passes without D86 and D88 as well. `main` 60022ae: the downstream app's long-note sessions diverge in 15 of 133. ``` $ LEXICAL_Y_GC=0 vitest --run --project unit Test Files 1 failed | 648 passed | 4 skipped (653) Tests 1 failed | 28071 passed | 481 expected fail | 135 skipped (28688) $ LEXICAL_Y_GC=1 vitest --run --project unit Test Files 2 failed | 647 passed | 4 skipped (653) Tests 2 failed | 28055 passed | 496 expected fail | 135 skipped (28688) $ pnpm run ci-check exit 0 ``` The failures are load flakes with 40 workers: `DisposeMemory.test.ts` (heap bound; gc off and on) and `MarkdownEscaping.test.ts` (5 s timeout, gc on). Each passes alone (two runs each).
…in D51 and #9671's reopening in D78 ## Description Review follow-ups to the revert: - `regressions/RunEndInTableCell.test.ts`: #9694's five steps (a style write in a table cell next to a run whose client wrote in another container). It checks each item as it is integrated, so a write that throws cannot hide a mismatch. - D51 notes that D86's revert brings back the collapse and the #9575 failures; D78 notes that the revert also reopens #9671 (its own-insert rules and their coverage go with it). ## Test plan ### Before ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RunEndInTableCell.test.ts # main 60022ae Tests 2 failed (2) ``` ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/RunEndInTableCell.test.ts Tests 2 passed (2) $ LEXICAL_Y_GC=1 (same file) Tests 2 passed (2) ```
…ome guards, D95 moot, gc-on #9623 cells pinned, D51 back to an accepted limit ## Description Rebased on `main` 064b18e (#9688, #9691). Without #9679's rules: - The #9690 cells (`FormatRuleTrade.test.ts`, `FormatRuleTradeSessions.test.ts`) pass: they become plain tests, guards against bringing those rules back as they were. D95 is marked moot (`REVERTED`). - `UndoRestoredRunOverDeletedText.test.ts`: with gc on, the sequential order of six shapes spreads again (the undo cannot find items gc collected): `it.fails` with gc on, titled "(#9623 with gc on)". The #9689 cells are unchanged. - `UndoInvisibleSteps.test.ts` (#9688): the cell that needed a delete to keep the comment removal's null passes again with the collapse; it becomes a plain test, with its comment updated. - D51 goes back to `ACCEPTED LIMIT` (its status before #9679), keeping the D86 sentence and the revert note. ## Test plan ### Before On 064b18e with the revert alone, `FormatRuleTrade*` and one `UndoInvisibleSteps` cell failed as unexpected passes, and twelve gc-on cells of `UndoRestoredRunOverDeletedText` failed. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{UndoInvisibleSteps,UndoRestoredRunOverDeletedText,FormatRuleTrade,FormatRuleTradeSessions}.test.ts Tests 35 passed | 46 expected fail (81) $ LEXICAL_Y_GC=1 (same files) Tests 35 passed | 46 expected fail (81) ```
![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: it's a clean, exact revert of #9679 on top of current main.
- Everything under
packages/lexical-y/src/internalmatches the tree before #9679 byte for byte, andcollaboration/y.mdequals #9679's diff reversed. - The new
RunEndInTableCell.test.tsfails onmainin both client orders, with gc off and on, and passes here. - The full
@lexical/yunit project passes withLEXICAL_Y_GC=0and=1. - Dropping the two changesets is right, since #9679 never shipped in a release.
A few optional, non-blocking suggestions:
- Pin the two reopened cases that lost their tests (
packages/lexical-y/src/__tests__/unit/regressions/OwnFormatItems.test.ts, deleted here). Its #9671 block (old line 340) and its "a range end that restates another client's opener" block (old line 498) both fail on this branch: 24 #9671 cells and the 12colorcells. Unlike #9575, #9609, #9620, #9623 and #9599, nothing pins them now. Keeping both blocks astest.failsnaming #9671 and #9622 would flag a later fix. The PR description doesn't mention the restated-opener case at all. - List the reopened issues in the docs.
packages/lexical-website/docs/collaboration/y.md's Open issues list (around line 1366) names none of #9599, #9609, #9620, #9623 or #9671, and Limits covers only the #9575 case (lines 1249-1253). #9623 is the one apps on Yjs's default garbage collection would most want to know about. - Decision register housekeeping (
packages/lexical-y/DECISIONS.md): - Test plan. AGENTS.md asks for the real Before/After output of the command that was run. The new test's failing output on
mainwould make a concrete Before.
This PR and #9693 both close #9694 and exclude each other, so which one lands is a maintainer's call. As the fallback, this one is ready.
| } | ||
| }); | ||
|
|
||
| describe('undoing text typed into a run and then formatted leaves the untouched rest of the run as it was (#9671)', () => { |
There was a problem hiding this comment.
Optional, non-blocking: this #9671 block, and the "a range end that restates another client's opener" block at line 498, both fail on this branch (24 and 12 cells). Nothing else pins those cases once this file is deleted. Keeping both blocks as test.fails naming #9671 and #9622, as the other reopened issues are pinned, would flag a later fix.
| ended it (a delete no longer does: it keeps every format item it leaves): | ||
| [#9564](https://github.com/meta/lexical/issues/9564). | ||
| text inside it offline while a collaborator's deletes there drop the item | ||
| that ended it: [#9564](https://github.com/meta/lexical/issues/9564). |
There was a problem hiding this comment.
Optional, non-blocking: the revert reopens #9599, #9609, #9620, #9623 and #9671, but this Open issues list names none of them. Limits only covers the #9575 case (lines 1249-1253). #9623 (with garbage collection on, an undo can bring a comment back over the following paragraphs) is the one apps would most want to see here.
| | 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; D86 removed the bound, and its revert on 11-01 put it back). 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] | | ||
| | D86 | **Reverted (11-01): an editor stops matching its own document in long sessions; see [#9694].** **A delete keeps every format item it leaves (no collapse).** The collapse of the run of deleted content and format items a delete leaves (`Cursor#collapseGap`, D24's bound per format key) is removed; D24's cleanup-off stays. | The collapse dropped ends that a concurrent writer's restore or an undo relied on, and the value ran on: #9575, #9609, a #9620 cell, the concurrent restore cells of #9640, a KeepEndsSessions seed. Every attempt to keep the right ends in the collapse (#9617) needed to know which write relies on which item. Measured against main, same seeds: a three-client view fuzz of 15,000 seeds: 0 seeds wrong where main is right, 631 where main spreads a value and this does not. Cost: each formatted run whose text is deleted keeps its two items, none per keystroke or plain delete (F24A-1: 40, 80, 120 after 20, 40, 60 words typed and deleted at one place); a comment added and its word deleted 3,000 times at one place: 6,002 live format items (main 6); a downstream document library's corpus of 189 documents x1.003 at 10 cycles, x1.26 at 150; Jot x1.01 on its default profile, a concentrated recolor 438 items (main 345). Accepted: the linear growth with deleted formatted text, over the bound and its spreads. | `REVERTED` | 10-31 | [#9575], [#9609] | | ||
| | D88 | **Reverted (11-01) with D86: an editor stops matching its own document in long sessions; see [#9694].** **A client keeps its own format items from garbage collection while they are undoable (memory only).** The binding sets Yjs's `Item.keep` on the format items its own transactions and its undo manager's insert, and on the end of each run they delete; it releases them, between transactions, when both undo stacks are empty (after a clear, an undo that leaves nothing, or a write they did not capture), and when it is disposed or its document destroyed. An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as Yjs's `UndoManager` collects nothing when it releases what its steps kept: any `UndoManager` on the document may still restore the item. | An undo reads its change's format items, and the end each gave a run it deleted, after a collaborator deleted them (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`). With Yjs's default garbage collection, which products use, those were collected, the undo could not tell them apart from other content, and the restored value ran on over the rest of the document ([#9623]; a two-client undo fuzz: 221 of 3,000 seeds to 0). Nothing new is written and other clients are unchanged. The cost is local: a format item a collaborator deletes while this client can undo the change that wrote it keeps its deleted content in this client's document, and in a state it encodes, until the document reloads or a relay that loads its state with gc on compacts it: about 25 bytes per such deletion, in this client only (+0.6% to 0.8% of the client's state in a realistic 4,500-edit session). Accepted over a stored change (a `gcFilter` keeping every format item: +1.9% to 7.6% bytes on long notes, in every peer and relay) and over collecting on release, which was tried and dropped: it destroyed what another `UndoManager` on the same document still needed. A `gcFilter` keeping every format item fixed the same cases, but changes what every peer and relay stores (+1.9% to 7.6% bytes on long notes). | `REVERTED` | 10-31 | [#9623], [y.md: Limits][limits] | | ||
| | D78 | **A format spreading after one client's triple undo:** a client formats a run, types plain text, formats again and undoes all three while a collaborator types plain text inside the first run. | **The fix was reverted (11-01) with D86 and D88 (#9679): an editor stops matching its own document in long sessions; see [#9694]; the case is open again ([#9599]). The revert also takes out #9679's rules for [#9671] (a format whose range starts at the writer's own insert null, or ends at its own insert restore, removes it) and their coverage (`DeleteNextToTyping.test.ts`): [#9671] is open again.** Fixed at the source: a restore's scan (`runOpener`) stops at content of another client, so the writer's earlier end and later opener with a collaborator's text between them are no longer read as one restored run, and the undo writes the run's end. A two-client run-sequence fuzz: 8 of 1,000 seeds to 0, gc on and off. | `OPEN` | 10-31 | [#9599], [#9671], [#9694] | |
zurfyx
force-pushed
the
zurfyx/y-revert-9679
branch
from
November 1, 2026 05:04
032cfb7 to
3945178
Compare
Member
Author
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
Reverts #9679 whole, on top of current
main(keeps #9684 and #9685).With #9679, a downstream app's long-note sessions end with an editor that
no longer matches its own Yjs document (15 of 133 sessions; 0 of 132 on
the commit before #9679). Reverting only part of it does not work: the
delete rule (D86) alone is worse on a recorded session (2810), and the
divergence was traced to one of its hunks (the end a restated run gets at a range's end, which could be linked into another container; #9693 fixes that at the source). This PR is the fallback if #9693 is not taken. Closes #9694 if merged instead of #9693.
What this brings back, as before #9679:
leaves to a bounded number per format key (D24's bound): the cases it
fixed (Bug: [lexical-y] A comment spreads onto later words when two collaborators undo text they typed inside it while a third deletes past its end #9575, Bug: [lexical-y] A comment's restore inside a word its own client commented runs on over the rest of the document when a collaborator's delete drops the comment's end #9609, a 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 cell, [lexical-y] Chore: Add exact restored-extent cases for undoing a value's removal, with the ones main fails as expected failures #9640's concurrent restore cells) fail
again and are pinned with their issues.
again put a value back past its end (Bug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid adding it #9623). The harness keeps its
LEXICAL_Y_GCswitch (added by [lexical-y] Bug Fix: A delete keeps every format item it leaves (D86), and a client keeps its own format items from garbage collection while they are undoable (D88) #9679), and the cells that fail only withgc on are pinned as
test.failswith gc on, naming Bug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid adding it #9623.gone: those cases are open again.
DECISIONS.mdkeeps D86 and D88 (nowREVERTED, with the reason) andreopens D78, rather than rewriting them.
y.mdgoes back to the behaviorbefore #9679. The two changesets of #9679 are removed: it was not part of
a release, so there is nothing to announce.
UndoSpreadSession2810.test.ts(added by #9679) is kept: it passes withoutD86 and D88 as well.
Test plan
Before
main60022ae: the downstream app's long-note sessions diverge in15 of 133.
After
The failures are load flakes with 40 workers:
DisposeMemory.test.ts(heapbound; gc off and on) and
MarkdownEscaping.test.ts(5 s timeout, gc on).Each passes alone (two runs each).
Follow-ups (rebased on main 064b18e, with #9688 and #9691)
regressions/RunEndInTableCell.test.tspins Bug: [lexical-y] A client's format write can link an item into another shared type in its own document, and its editor then stops matching that document for good #9694 (each item checked as it is integrated).ACCEPTED LIMITwith the revert note; D78 notes that Bug: [lexical-y] Undoing text you typed into a format run and then formatted takes the format off the untouched rest of the run when a collaborator typed after it, with no concurrent edits #9671 reopens.UndoRestoredRunOverDeletedText.test.ts: six sequential gc-on cells fail again (Bug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid adding it #9623):it.failswith gc on.UndoInvisibleSteps.test.ts: one cell passes again with the collapse: plain test.lexical-y units on the new head: gc off 18,816 passed / 526 expected fail; gc on 18,801 / 541; the one failure in each run is
DisposeMemory.test.ts's heap bound under machine load (passes alone).pnpm run ci-check: exit 0.