Repository navigation
Conversation
…osed D86) ## Description Proposed for review: DECISIONS.md D86 is `OPEN` (the number may shift at landing). A delete collapsed the run of deleted content and format items it left (`Cursor#collapseGap`, D24's bound per format key): it kept a few items per key and deleted the rest. The ends it deleted were sometimes the ones a concurrent writer's restore, or an undo, relied on, and the value then ran on past its end: a comment over the following words and line (#9575), a comment's restore after a collaborator's plain text inside it (#9609), a comment its author undid (a #9620 case), the concurrent restore cases of #9640. Keeping the right ends inside the collapse (#9617) needed to know which write relies on which item, which the document does not store. This removes the collapse: a delete keeps every format item it leaves. Yjs's own format cleanup stays off (D24). `Cursor` loses `collapseGap` and the gap tracking it used (`gapStart`, `gapFormats`); comments that pointed at it now point at `Cursor#delete`. Cost: each formatted run (a format, a comment, a NodeState value) whose text is deleted keeps its two items, its opener and its closer. Plain typing and deleting add none, and neither does typing inside a formatted run. y.md gets a Limits entry (Deleted formatted text), and its Write rules paragraph and Undo paragraph lose the collapse's limits. Tests changed where they encoded the collapse, each with the reason in a comment: - `regressions/Formats.test.ts`, F24A-1, and `CursorFamily.test.ts`: words of a format, comment or set member typed and deleted again at one place leave exactly two items each (40, 80, 120 after 20, 40, 60 words) instead of a bound. - `Cursor.test.ts`: a format change then a delete on one cursor keeps all five items after the range. - `Races.test.ts`, #9575: the comment ends where it ended (was `test.fails`); the cell that pinned the spread "as documented" is gone. - `regressions/CollapseKeepsEnds.test.ts`, #9609 (12 cells), `regressions/KeepEndsSessions.test.ts` (4694), `regressions/UndoRestoredRunEnds.test.ts` (#9620, 2 cells) and `regressions/UndoRestoredRunEndExtents.test.ts` (the 10 concurrent empty restore cells): were `test.fails`, pass. - `CollapseKeepsEndsProperty.test.ts` is removed: it compared every session with a run of the same session with the collapse turned off, which is now the only run. ## Test plan Unit tests of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,546 passed, 395 expected fail, 0 failed (`DisposeMemory.test.ts`'s heap bound failed once under a 16-worker run and passes alone and in reruns, here and on main). `pnpm run ci-check` passes. `MATRIX_FULL=1` matrix: see the body. Measured (the coordinator's runs and mine, same seeds, against main): - A three-client view fuzz, 15,000 seeds: 0 seeds where this is wrong and main is right; 631 where main spreads a value and this does not. - Growth (measured on 1853707 with the collapse turned off, the same rule): F24A-1 above; a comment added and its word deleted 3,000 times at one place: 6,002 live format items (main 6), 41 ms per edit (main 27); adding and removing a comment 1,000 times: 1 live item (as main); 1,000 keystrokes inside a commented run: none added. - Emily: x1.003 at 10 cycles, x1.26 at 150. Jot: x1.01 on its default profile; a concentrated recolor 438 items (main 345).
…to the collapse ## Description KeepEndsSessions' 3645 cell spied on the removed `Cursor#collapseGap` inside `test.fails`, so it passed by throwing before its comparison. It now asserts what the session shows: c9 on every character of "wxq4xq56abcd". Test comments that pointed at `Cursor#collapseGap` now point at `Cursor#delete` or say the collapse is gone (D86); no test references `collapseGap`, `gapStart` or `gapFormats` any more. ## Test plan KeepEndsSessions passes (3645 as a plain test). Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,547 passed, 394 expected fail, 0 failed. `pnpm run ci-check` passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: |
There was a problem hiding this comment.
This looks good to me. Removing Cursor#collapseGap makes a delete delete only text. That is always a valid Yjs operation, and it removes the class of spreads where a comment or format lost the end a concurrent restore or an undo relied on. The regression tests that now pass (#9575, #9609, #9620, the KeepEndsSessions seeds and the concurrent restore cells) fail against main's implementation in both client id orders, and they assert converged peers and comment extents rather than only checking that nothing throws. The storage trade-off is recorded as D86 and documented in y.md's Limits.
Optional, non-blocking suggestions:
- Decision register.
packages/lexical-y/DECISIONS.md:58(D24) still says a delete keeps the ends of deleted runs "within a bound per format key", andpackages/lexical-y/src/internal/Cursor.ts:848cites D24 for the new rule. The register's preamble asks a pull request that changes a recorded decision to update its row ("a replaced decision says what replaced it"). Consider pointing D24 at D86, citing D86 in the JSDoc, and updating D65 (:131). D65's reasoning relies on the growth bound this removes, and its #9609 cells inOwnRunRestoreEnd.test.ts(:209,:390,:567) are stilltest.fails. - Test plan. AGENTS.md (Commits and Pull Requests) asks for
### Beforeand### Afterwith the command and its pasted output. The post-rebase gate results are still "to be added", and the 15,000-seed three-client comparison and the growth and timing figures are neither committed nor pasted, so nobody else can rerun them. - Stale wording.
regressions/Formats.test.ts:729is still titled "leave a bounded number of format items" but now asserts linear growth. Present-tense descriptions of the collapse remain inUndoRunEnds.test.ts:648and:677,OwnCommentGapCollapse.test.ts:60andregressions/TypedTextAfterDeletedSpace.test.ts:10(title at:54). - Coverage. The deleted
CollapseKeepsEndsProperty.test.tsalso checked that both peers and a fresh reader agree over random typing ‖ delete sessions. That check never depended on the collapse, and keeping it as a convergence property would preserve the coverage. - Changeset. One sentence saying that documents now grow with each deleted formatted run would help people upgrading (CONTRIBUTING.md: write the entry for someone who uses Lexical).
| * items without live content that the delete leaves (see collapseGap). | ||
| * Delete `n` content units. The format items around and inside them stay: | ||
| * a concurrent writer that has not seen the delete relies on them, and an | ||
| * undo of the delete brings the content back between them (D24). |
There was a problem hiding this comment.
Optional: this rule is D86. D24's row still says a delete keeps the ends "within a bound per format key", so citing D86 here (and adding "replaced by D86" to D24, as the register's preamble asks) would keep the two in step.
| | D76 | **A code-token-aware trim for remade text.** A stale replace of a word can take the first letter of the word a rewrite made from it, where a node boundary cut the old word. | Writing a changed word whole fixes it, but also rewrites highlighter tokens glued to a pasted-over token in a code line. A follow-up would apply the rule everywhere except at code highlight token boundaries (D29). | `OPEN` | 10-30 | [#9608] | | ||
| | D77 | **A stale comment on a word a collaborator rewrites** lands on the new word for one client id order only. | Likely the documented race where text typed at the start of a range a collaborator formats takes that format, with a comment instead of a format. Either the docs name comments and whole-word replacements, or a fix makes both orders agree. | `OPEN` | 10-30 | [#9610], [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. | Reproduced on main in both client id orders; no design chosen. | `OPEN` | 10-30 | [#9599] | | ||
| | D86 | **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); Emily 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. | `DECIDED` | 10-31 | [#9575], [#9609] | |
There was a problem hiding this comment.
Optional: the preamble asks a pull request that changes a recorded decision to update its row. D24 (line 58) still describes the bound this removes, and D65 (line 131) still reasons from "growth bounds" and points at #9609, part of which this fixes. Its OwnRunRestoreEnd.test.ts #9609 cells are still test.fails.
There was a problem hiding this comment.
D24 points at D86 (d318d9a). D65 is updated in 8ed68ce: it no longer argues from the growth bound, and says the #9609 cells where a collaborator's delete took the run's end pass while the OwnRunRestoreEnd.test.ts cells (a restore whose run has no end of its own) still fail, which is why those stay test.fails. D51 (the #9575 spread) is marked replaced by D86.
## Description D86 (a delete keeps every format item it leaves) moves from `OPEN` to `DECIDED` (10-31): the linear growth with deleted formatted text is accepted over the bound and its spreads. The row moves from Open to Storage model, next to D24, whose bound per format key it removes; D24 says so. ## Test plan Docs only: DECISIONS.md rows D24 and D86; `prettier --check` passes.
8c57ef8 to
d318d9a
Compare
There was a problem hiding this comment.
This looks good to me. Taking Cursor#collapseGap out of @lexical/y's delete path fixes the run-on cases at their source, and the cost is written down where people will look for it: D86, and the new "Deleted formatted text" entry in y.md's Limits. Every cell that moves from test.fails to test checks that the peers converge and then checks the exact words with their comment ids. Those cells fail against the previous cursor, so they really tell the two behaviors apart. Deferring #9623 (the gc-only class) to the stacked follow-up makes sense.
A few optional, non-blocking suggestions:
- Refresh DECISIONS.md D51 and D65 (
packages/lexical-y/DECISIONS.md:112and:132). D51 still lists the #9575 spread as an accepted limit "documented in Limits", but this PR removes that Limits paragraph and the #9575 cells now pass. D65 still argues that keeping the last nulls "breaks growth bounds", and D86 removes those bounds.packages/lexical-y/AGENTS.md(Decisions) asks a pull request that changes a recorded decision to update its row in the same pull request, and DECISIONS.md asks a replaced decision to say what replaced it. D65 can keep tracking the raw-delete #9609 cells inOwnRunRestoreEnd.test.ts. - Keep a randomized convergence check.
CollapseKeepsEndsProperty.test.tswas the only randomized session test where one peer types inside a commented word offline while another deletes around it. Its no-collapse comparison is moot now, but its checks still apply: no errors, and every peer and a fresh reader agree.packages/lexical-y/AGENTS.md(Testing) asks to test the input space; keepingrun()with just those assertions would preserve that coverage. - Stale wording in tests. Some test names and comments still describe the bound:
regressions/Formats.test.ts:729is still named "leave a bounded number of format items" but now asserts[40, 80, 120].regressions/CollapseKeepsEnds.test.ts:152-155still talks about "a later collapse of the same run".- Comments at
OwnRunRestoreEnd.test.ts:254and:293,OwnCommentGapCollapse.test.ts:60,UndoRunEnds.test.ts:648and:677, andregressions/TypedTextAfterDeletedSpace.test.ts:10and:54.
- D86 measurement names (
DECISIONS.md:59). The row cites one measurement corpus only by a name the register doesn't define. The PR body's wording ("a downstream document library's corpus") would make it readable. - Changeset. The
Verify:paragraph is good. One more sentence pointing to y.md's "Deleted formatted text" Limits entry would tell upgrading apps that stored documents now grow with deleted formatted text. - Test plan. AGENTS.md (Commits and Pull Requests) asks for the real command output under
### Beforeand### After. The rebased gate results still read "will be added".
A heads-up, not a request: the 3645 cell now asserts [c9] directly. The comment it replaces noted that #9630's undo-press change removes c9 in that session, so that draft will need to revisit the cell.
| // As above, and B then deletes "cc " too: the first collapse removed | ||
| // the comment's opener and kept its end, which the second collapse | ||
| // must keep although, for B, nothing opens the comment before it. | ||
| test(`#9609: the comment's end stays through a later collapse of the same run (ids ${ids})`, async () => { |
There was a problem hiding this comment.
Optional: this test name and the comment under it still describe "a later collapse of the same run", which no longer happens after D86. Something like "the comment's end stays through a later delete next to the same run" would match the new behavior.
There was a problem hiding this comment.
Fixed in 8ed68ce: renamed to "the comment's end stays through a later delete next to the same run", with the comment saying the collapse is gone (D86).
| | D23 | **Garbage collection stays on**, as Yjs defaults; headless results are compacted as clients store them. Only temporary documents turn it off. Turning it off for history is an app's choice, on every client and server. | A server that skips compaction while clients compact is a trap, and undo and insert attribution work with GC on. | `DECIDED` | 10-23 | [#9437], [y-server.md][y-server] | | ||
| | 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] | | ||
| | 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 until D86 removed the bound: a delete keeps every format item it leaves). 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 | **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); Emily 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. | `DECIDED` | 10-31 | [#9575], [#9609] | |
There was a problem hiding this comment.
Optional: one of the measurement corpora in this row is cited only by a name the register doesn't define. The PR body's description ("a downstream document library's corpus") would make the row readable for others.
There was a problem hiding this comment.
Fixed in 8ed68ce: the row now says "a downstream document library's corpus of 189 documents".
…keep the delete-next-to-typing convergence property ## Description Review follow-ups for D86: - DECISIONS.md: D51 (the #9575 spread) is replaced by D86, whose cells pass; D65 no longer argues from the growth bound D86 removed and says which #9609 cells still fail; D86 names its corpus measurement without an internal name. `Cursor#delete` cites D86. - `DeleteNextToTyping.test.ts` keeps the randomized sessions of the removed `CollapseKeepsEndsProperty.test.ts` (one peer types inside a commented word while the other deletes around it, taking turns and at the same time) with the checks that never depended on the collapse: no errors, every peer and a fresh reader agree. - Test names and comments that described the collapse in the present tense now say it is gone (D86). - The changeset says documents keep two format items per formatted run whose text is deleted. ## Test plan `DeleteNextToTyping.test.ts`: 4 passed (60 sessions typing ‖ deleting and 20 taking turns per client id order). The renamed tests pass. `prettier --check`, `eslint --no-cache` on the changed files and the changeset check pass.
There was a problem hiding this comment.
This looks good to me. Taking Cursor#collapseGap out of @lexical/y's delete path means a delete removes only text. That fixes the cases where a comment or format lost the end a concurrent restore or an undo relied on and ran on over the following words and lines (#9575, #9609, #9620). The cells that move from test.fails to test check that the peers converge and then check the exact words and comment ids, and they fail against the previous cursor. The storage cost is written down where people will look for it: D86, and the new "Deleted formatted text" entry in y.md's Limits.
A few optional, non-blocking suggestions:
- #9564 in y.md's Open issues (
packages/lexical-website/docs/collaboration/y.md:1349-1351). The bullet still says a comment spreads "while a collaborator's deletes there drop the item that ended it". After this PR, a delete never drops a format item. TheOwnRunRestoreEnd.test.tscells that staytest.failsreproduce the spread only by removing the comment's opener and end with raw Yjs deletes (dropCommentItems). That section says each listed issue "has a test that reproduces it", so rewording the bullet to the write that can still drop that end (or narrowing it if none remains) would keep it accurate. #9564 and #9609 describe the same mechanism. - Edit cost in the new Limits entry (
y.md:1099-1106). It explains the growth. One clause saying that edits at a place where formatted text was deleted many times also get slower would set expectations; the PR's own figure after 3,000 cycles is 41 ms against 27 ms per edit. - Test plan. AGENTS.md (Commits and Pull Requests) asks for the command and its real output under
### Beforeand### After. The rebased gate results still read "will be added here". - Nits. The describe title at
packages/lexical-y/src/__tests__/unit/regressions/CollapseKeepsEnds.test.ts:117still says "a delete collapsing a run". A few new comment lines run past 80 columns, which Prettier does not rewrap (packages/lexical-y/src/internal/Cursor.ts:50,packages/lexical-y/src/internal/Write.ts:2459).
| - **Deleted formatted text.** A delete keeps every format item it leaves, | ||
| as a collaborator's concurrent edit there and an undo of the delete rely | ||
| on them: each formatted run (a format, a comment, a NodeState value) | ||
| whose text is deleted leaves its two items (its opener and its closer). | ||
| Plain typing and deleting add none, and neither does typing | ||
| inside a formatted run. So a document grows with the formatted text | ||
| deleted from it, not only with its content: most in a place where | ||
| formatted text is typed and deleted over and over. |
There was a problem hiding this comment.
Optional: this explains the growth well. One clause saying that edits at a place where formatted text was deleted many times also get slower (the PR's 3,000-cycle figure is 41 ms against 27 ms per edit) would tell apps with such a spot what to expect.
| * Y.Node#applyDelta), but the items differ: a delete keeps every format item | ||
| * in the run of deleted content and format items it leaves, which Yjs removes | ||
| * as redundant and concurrent writers rely on (see delete), and a format | ||
| * change keeps the items that bound the content around its range, opens its value next to its content and closes it where |
There was a problem hiding this comment.
Nit: this comment line is 122 columns, and Prettier does not rewrap comments. The same applies to Write.ts:2459.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its transactions and its undo manager's insert, and on the end of each run they delete. It releases them 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, and then collects the ones a remote update deleted meanwhile (Yjs's `tryGc`, as a transaction collects its own deletes); one deleted locally is left as Yjs leaves it, since an `UndoManager` step on the same document may restore it. An item another binding or an `UndoManager` step also keeps stays kept. Nothing new is written; while an item is kept, this client's state still holds its deleted content, as for the items an `UndoManager` step deletes. `YjsContract` checks `Item#keep`, `tryGc` and the `Doc` members used. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo after this one releases, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 14 passed; 5 fail with the keep and without the release paths, and 4 more without the remote-only collection and the uncaptured-write release. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its transactions and its undo manager's insert, and on the end of each run they delete. It releases them 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. An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 16 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its 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. An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases (also from inside that `UndoManager`'s capture), repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 18 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on, as does a release inside that `UndoManager`'s capture. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its 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 (a queued release keeps the stronger request; a listener call under way at dispose keeps nothing). An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases (also from inside that `UndoManager`'s capture), dispose or destroy while a release waits, dispose during the first capture, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 22 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on, as does a release inside that `UndoManager`'s capture; the 4 dispose and destroy cells fail without the queued-release handling. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its 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 (a queued release keeps the stronger request; a listener call under way at dispose keeps nothing; ownership is per listening lifetime, so a binding started again keeps what its new steps need). An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases (also from inside that `UndoManager`'s capture), dispose or destroy while a release waits, dispose during the first capture, a restart before the old release ran, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 24 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on, as does a release inside that `UndoManager`'s capture; the 4 dispose and destroy cells fail without the queued-release handling, and the restart cell (gc on) with ownership per binding instead of per lifetime. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its 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 (a queued release keeps the stronger request; a listener call under way at dispose keeps nothing; ownership is per listening lifetime, so a binding started again keeps what its new steps need). An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases (also from inside that `UndoManager`'s capture), dispose or destroy while a release waits, dispose during the first capture, a restart before the old release ran, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 24 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on, as does a release inside that `UndoManager`'s capture; the 4 dispose and destroy cells fail without the queued-release handling, and the restart cell (gc on) with ownership per binding instead of per lifetime. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
…llection while they are undoable ## Description With Yjs's default garbage collection, which products use, an undo could not read the format items of its own change after a collaborator deleted them: they were collected, so the undo guards (`isKeptByUndo`, `changeEnd`, `closeRestoredRuns`) took them for other content, and the value an undo brought back ran on over the rest of the document (#9623). The unit harness built every peer with gc off, so most tests never saw it. - `Binding`: the binding sets Yjs's own `Item.keep` on the format items its 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 (a queued release keeps the stronger request; a listener call under way at dispose keeps nothing; ownership is per listening lifetime, so a binding started again keeps what its new steps need). An item another binding or an `UndoManager` step also keeps stays kept. Releasing collects nothing, as the `UndoManager` collects nothing when it releases what its steps kept: a format item a collaborator deleted while kept keeps its deleted content in this client's document until it reloads. Nothing new is written. `YjsContract` checks `Item#keep` and `Transaction#local`. - `Cursor#runOpener`: a restore's scan stops at content of another client, so a client's earlier end and later opener with a collaborator's text between them are not one restored run (#9599). - `Cursor` rule 3: a range end that writes its own copy of another client's opener also ends it where that opener's run ended, so undoing that client's change leaves the text after the range alone. - `Cursor`: a format write removes the null and the restore that this client's own earlier insert left inside its range, as Yjs's `formatText` does, so a collaborator's typing no longer relies on items the insert's undo deletes (#9671). - Tests: `regressions/OwnFormatItems.test.ts` runs the #9623, #9599 and #9671 repros and a restated-opener case, gc on and off, every client id order. `regressions/OwnFormatItemsRelease.test.ts` checks the release: clears, an undo that empties both stacks, writes the stacks do not capture, dispose, a new binding on the same document, another binding's undo and an app `UndoManager` tracking a provider's updates after this one releases (also from inside that `UndoManager`'s capture), dispose or destroy while a release waits, dispose during the first capture, a restart before the old release ran, repeated cycles, and what a relay stores. `regressions/UndoSpreadSession2810.test.ts` replays a downstream app's undo-fuzz session where, with gc on, the parent commit put a comment on four paragraphs and main did not. The harness takes a `gc` option and `LEXICAL_Y_GC=1` runs every harness test with gc on. Pins that now pass are plain tests: `UndoRestoredRunEndExtents` (117 cells), `UndoRestoredRunEnds` (20; the D64 cells stay pinned), `UndoSpreadFuzz` (5 seeds; its documents use gc on), `UndoCommentRemovalSpread` (4), `RunSequenceUndo` seed 87. - DECISIONS.md: D88 (own format items kept from gc while undoable), D78 decided (the #9599 class fixed). y.md's Limits states D88's local cost. - Docs for D86 (#9678 review follow-ups): y.md's #9564 entry no longer says a delete drops the item that ended the comment, and the Deleted formatted text limit says edits at such a place get slower; two comments rewrapped and a test title updated. ## Test plan `OwnFormatItems.test.ts`: 72 passed; on the parent commit 44 fail (the issue cells with gc on for #9623; #9599, #9671 and the restated-opener color cells with gc on and off). `OwnFormatItemsRelease.test.ts`: 24 passed; without the release paths 5 fail, and collecting on release fails the other binding's and the app `UndoManager`'s undo with gc on, as does a release inside that `UndoManager`'s capture; the 4 dispose and destroy cells fail without the queued-release handling, and the restart cell (gc on) with ownership per binding instead of per lifetime. `UndoSpreadSession2810.test.ts`: 4 passed; on the parent commit the 2 gc-on cells fail (main passes all 4). The pins above flip from expected-fail to pass and nothing else changes in the lexical-y unit suite.
Description
DECISIONS.md D86,
DECIDED(10-31). Lands together with #9679 (stacked on this one): on its own, this PR is worse than main on one gc-on downstream undo-fuzz session (below), which #9679 fixes.A delete collapsed the run of deleted content and format items it left
(
Cursor#collapseGap, D24's bound per format key): it kept a few items perkey and deleted the rest. The ends it deleted were sometimes the ones a
concurrent writer's restore, or an undo, relied on, and the value then ran
on past its end: a comment over the following words and line (#9575), a
comment's restore after a collaborator's plain text inside it (#9609), a
comment its author undid (a #9620 case), the concurrent restore cases of
#9640. Keeping the right ends inside the collapse (#9617) needed to know
which write relies on which item, which the document does not store.
This removes the collapse: a delete keeps every format item it leaves.
Yjs's own format cleanup stays off (D24, which now points at D86).
Cursorloses
collapseGapand the gap tracking it used (gapStart,gapFormats).Cost: each formatted run (a format, a comment, a NodeState value) whose
text is deleted keeps its two items, its opener and its closer. Plain
typing and deleting add none, and neither does typing inside a formatted
run. y.md gets a Limits entry (Deleted formatted text), and its Write rules
paragraph and Undo paragraph lose the collapse's limits. D51 (the #9575
spread) is marked replaced by D86; D65 is updated.
Tests changed where they encoded the collapse, each with the reason in a
comment:
regressions/Formats.test.ts, F24A-1, andCursorFamily.test.ts: wordsof a format, comment or set member typed and deleted again at one place
leave exactly two items each (40, 80, 120 after 20, 40, 60 words).
Cursor.test.ts: a format change then a delete on one cursor keeps allfive items after the range.
Races.test.ts, 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: the comment ends where it ended (wastest.fails).regressions/CollapseKeepsEnds.test.ts, 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 (12 cells),regressions/KeepEndsSessions.test.ts(4694; 3645 now asserts c9directly),
regressions/UndoRestoredRunEnds.test.ts(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, 2 cells)and
regressions/UndoRestoredRunEndExtents.test.ts(10 concurrent emptyrestore cells): were
test.fails, pass.CollapseKeepsEndsProperty.test.ts(which compared each session with arun without the collapse) becomes
DeleteNextToTyping.test.ts: the samerandom sessions, checking no errors and every peer and a fresh reader
agreeing.
Test plan
Landing tree: main cfcd3c4 + this PR (code identical from 8c57ef8 to
8ed68ce: later commits change docs, tests and one comment). Every
property, fuzz and soak lane ran with Yjs garbage collection on (what
products use) and off.
Before (main cfcd3c4)
with gc off, 511 with gc on.
pnpm run ci-checkpasses; units of every package: 0 failures with gcoff; with gc on 15 Formats tests fail (gc-only, on main too); MATRIX_FULL:
11 legacy-binding failures.
After (this PR)
main is right in either; 601 (gc off) and 608 (gc on) main-wrong and
this right.
seeds), comment-run fuzz 0 / 0.
pnpm run ci-checkpasses. Units, all packages, gc off and gc on: nofailure that main does not have (the same 15 gc-on Formats tests).
MATRIX_FULL: the same 11.
sessions, 0 failing only here, 4 / 4 failing only on main. L1C (IME):
10,988 sessions, 0 either way. L3 (hour-long sessions, reloads, offline
gaps, clients with gc on and off in each session): 269 sessions, 6
flagged only here and 13 only on main, in the same classes both ways.
probes) on preview builds: identical to main. Its corpus of 189 documents
after 10 / 40 / 150 comment cycles: format items x1.003 / x1.049 / x1.26,
bytes x1.000 / x1.012 / x1.051.
identical to main. Its undo-spread fuzz (gc on): plain 351 vs main 350:
the extra seed (2810) spreads here and not on main with gc on, passes with
gc off on both, and passes with [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, which adds it as a test
(
regressions/UndoSpreadSession2810.test.ts). Macro profile 433 = 433.