Repository navigation
[lexical-y] Chore: Add exact restored-extent cases for undoing a value's removal, with the ones main fails as expected failures - #9640
Conversation
…e's removal, with the ones main fails as expected failures ## Description Tests only, for #9623, #9620 and the D64 shape (#9560), so a fix for them is judged by exact extents instead of "on the word or nowhere": a looser oracle let a mutation that drops every restored value pass. Every case checks that all clients and a fresh read agree, and the exact characters the value is on. - `regressions/UndoRestoredRunEndExtents.test.ts` (244 cases): a collaborator removes a value (comment, bold, italic, underline, color, NodeState, link) from a word; its author undoes the value, maybe writing the same key on a later range before or after; a third client may delete text around the value's end; the collaborator undoes the removal: the value is back on exactly the word. Also 1 to 256 comment ids in one write, and the concurrent shape of #9620. Main fails every garbage-collection-on cell and the concurrent cells (`test.fails`). - `regressions/UndoRestoredRunEnds.test.ts` (151 cases): comment, bold, color and NodeState, removed from the word, part of it or the whole document, the author undoing first or not, text typed after the word, garbage collection on and off, two id orders; undo, redo and undo again, each with its exact extent. Main fails the author-undoes cells with garbage collection on (#9623) and the partial removals (the author's undo leaves the rest of the word with the value the removal restated, then the removal's undo gives the whole word back: D64), and #9620's two cells. - `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`, `US_START`, `US_OPS`, `US_UNDO`); the 9 seeds main fails are `test.fails`. - `regressions/UndoCommentRemovalSpread.test.ts`: #9623 with a second comment and typing in the same undo step (4 cases, all fail on main). - `regressions/UndoKeepsLaterFormat.test.ts`: an undo of a delete keeps the italic a collaborator gave the text afterwards (passes on main; a fix that ends the runs an undo brings back where they ended at the delete broke it). ## Test plan main (1853707) with these files: ``` $ vitest --run --project unit <the five files> Test Files 5 passed (5) Tests 262 passed | 194 expected fail (456) ``` `pnpm run ci-check` passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: |
![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: the five files pin exact restored extents for @lexical/y undo, compare every peer with a fresh read, and each test.fails cell names the issue whose fix will flip it, as packages/lexical-y/AGENTS.md asks. The suggestions below are optional and non-blocking.
Make undo grouping explicit in two files (optional). packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts (L207-211) and packages/lexical-y/src/__tests__/unit/regressions/UndoCommentRemovalSpread.test.ts (L90-94) bind with the default captureTimeout of 500 ms (DECISIONS.md D25) and never call stopCapturing(), so what an UNDO_COMMAND reverts depends on how fast the writes run. With captureTimeout: 0 in those two configs, none of the 9 FAILS_ON_MAIN seeds fail and 3 of the 4 UndoCommentRemovalSpread cells pass on main; at 5 ms seed 12 already passes. On a normal runner the pins hold, but a long stall could flip a pinned cell with no code change, and once the fix lands the fuzz would only cover undos that revert everything an editor wrote since its last undo. The harness's Peer.update and Peer.undo (src/__tests__/utils/harness.ts L279-301) show a deterministic pattern: a large captureTimeout plus binding.undoManager.stopCapturing() where a step should end, or a generated step-boundary op in the fuzz.
Smaller nits (optional).
UndoSpreadFuzz.test.tsL184:FAILS_ON_MAINis keyed by seed, butUS_OPSandUS_UNDOchange every seed's ops, so the pin only describes the defaults; applying it only at the defaults avoids noise when exploring.regressions/UndoRestoredRunEnds.test.tsL47-53:#recordRunEndsdoes not exist on main, and "the undo now ends the run" describes the parked fix rather than main; describing the expected behavior keeps it accurate whichever design lands.regressions/UndoRestoredRunEnds.test.tsL339-341 disposes the editors only when every check passes, the #9620 cells never stop their peers, andregressions/UndoCommentRemovalSpread.test.tsnever disposes its editors;try/finally, asUndoRestoredRunEndExtents.test.tsuses, keeps failing cells from leaving editors bound.- The PR body lists 151 cases for
UndoRestoredRunEnds.test.ts; the file defines 147 (the 456 total is right).
| configExtension(YCollabExtension, { | ||
| doc, | ||
| schemaVersion: 1, | ||
| synced: true, | ||
| }), |
There was a problem hiding this comment.
Optional: this binds with the default captureTimeout (500 ms) and nothing calls stopCapturing(), so which writes an undo op reverts depends on timing. With captureTimeout: 0, none of the FAILS_ON_MAIN seeds fail on main (at 5 ms seed 12 already passes), so a long stall could flip a pin, and after the fix the fuzz only exercises undos that revert everything an editor wrote since its last undo. A large captureTimeout plus stopCapturing() at generated step boundaries, as Peer.update and Peer.undo in utils/harness.ts do, would make the grouping part of the seed.
| const WORDS = ['alpha', 'beta', 'gamma', 'delta', 'note', 'run']; | ||
|
|
||
| // Seeds main fails (#9623), pinned as test.fails until the fix lands. | ||
| const FAILS_ON_MAIN = new Set([10, 12, 23, 26, 28, 35, 50, 51, 55]); |
There was a problem hiding this comment.
Nit: US_OPS and US_UNDO change every seed's op list, so this set only describes the defaults (30 ops, 0.15); applying it only while both are at their defaults would keep runs with other values free of pass/fail noise.
| configExtension(YCollabExtension, { | ||
| doc, | ||
| schemaVersion: 1, | ||
| synced: true, | ||
| }), |
There was a problem hiding this comment.
Optional: the header relies on A's writes landing "in one undo step", which here comes from the default 500 ms capture window. With captureTimeout: 0, 3 of the 4 cells pass on main, so the it.fails pins depend on timing. A large captureTimeout (with stopCapturing() where a step should end) makes the single step explicit.
| // #9623, #9620: an undo that brings back an item opening a value (a comment, | ||
| // a format, a style, a NodeState value) whose run's end is gone, because | ||
| // another change's undo deleted it before (#9623) or at the same time | ||
| // (#9620), left the value open: it covered everything after it. The undo | ||
| // now ends the run it brings back where that run ended when the item was | ||
| // deleted, with a null of its own (see Binding#closeRestoredRuns and | ||
| // #recordRunEnds). |
There was a problem hiding this comment.
Nit: #recordRunEnds does not exist on main (it is from the parked #9633), and main does not yet end the restored run for the gc-on cells; describing the expected behavior rather than a mechanism would keep this accurate whichever design lands.
…ent cases ## Description The cases with 1 to 256 comment ids in one write checked the extent of c0 only, so a decoder that kept one id per character passed them. They now check that the restored word has every id. ## Test plan main (1853707): UndoRestoredRunEndExtents 117 passed, 127 expected fail (244).
…d itself in the undo restore cases ## Description - UndoRestoredRunEnds checks every block's text too, so a change that loses a character is caught (it checked only where the value is). - UndoCommentRemovalSpread checks that c1 is back on its words, so an undo that drops the value no longer passes it. - UndoSpreadFuzz pins the seeds that spread on main only with its default profile (US_OPS and US_UNDO unchanged): another profile makes other ops from the same seeds. ## Test plan main (1853707), the five files: 262 passed, 194 expected fail. `US_OPS=29`: no seed pinned, the seeds that spread fail as tests.
There was a problem hiding this comment.
This looks good to me: the five files pin exact restored extents for the undo shapes in #9623, #9620 and #9560, the test.fails selections match exactly the cells that fail on main today, and each of those fails on the extent its comment describes. A few optional, non-blocking suggestions:
-
Make the undo grouping explicit (
packages/lexical-y/src/__tests__/unit/UndoSpreadFuzz.test.ts:211,packages/lexical-y/src/__tests__/unit/regressions/UndoCommentRemovalSpread.test.ts:90). These peers use the default 500 mscaptureTimeout, so which edits share an undo step is decided by the clock. WithcaptureTimeout: 0, none of the nineSPREADS_ON_MAINseeds spreads and three of the four UndoCommentRemovalSpread cases pass; with a very large value, the same nine seeds and four cases fail exactly as they do now. An explicit largecaptureTimeoutkeeps every pin, and a slow CI runner can then no longer split a step and turn atest.failsred on someone else's pull request.UndoRestoredRunEnds.test.tsalready setscaptureTimeout: 0for its own clients. -
Which outcome should a #9623 fix produce once the author has undone the comment? The author-undoes cells (
regressions/UndoRestoredRunEnds.test.ts:341-343, the non-concurrent cells ofregressions/UndoRestoredRunEndExtents.test.ts, and therestoredcheck inregressions/UndoCommentRemovalSpread.test.ts:171-182) require the value back on exactly the word. #9623's expected behavior also accepts no comment at all, and the alternative described on #9633 would give that. If "nowhere" is still an option, consider accepting either outcome for those cells: the exact extent on the kept-by-author cells already catches a fix that drops every restored value. If "back on the word" is the decision, a row inpackages/lexical-y/DECISIONS.mdwould record it, sincepackages/lexical-y/AGENTS.mdasks for one when a pull request makes a consequential decision. -
Header comment (
regressions/UndoRestoredRunEnds.test.ts:47-53). It says the undo "now" ends the restored run and points at#recordRunEnds, which is not onmain. Describing the pending fix instead would avoid confusion while these cells are pinned. -
Test plan. AGENTS.md ("Commits and Pull Requests") and the pull request template ask for
### Beforeand### Aftersubsections with pasted output.
| dependencies: [ | ||
| RichTextExtension, | ||
| MarkStateExtension, | ||
| configExtension(YCollabExtension, { |
There was a problem hiding this comment.
Optional: these peers use the default 500 ms captureTimeout, so how a peer's ops group into undo steps depends on wall-clock time. With captureTimeout: 0, none of the nine SPREADS_ON_MAIN seeds spreads; with a very large value, the same nine spread as today. Passing an explicit large captureTimeout here keeps the pins and stops a slow runner from splitting a step and flipping a test.fails.
| dependencies: [ | ||
| RichTextExtension, | ||
| MarkStateExtension, | ||
| configExtension(YCollabExtension, { |
There was a problem hiding this comment.
Optional: "in one undo step" (line 12) comes from the default 500 ms capture window here. With captureTimeout: 0, three of the four it.fails cases pass; an explicit large captureTimeout keeps all four failing on main without depending on timing.
| // (#9620), left the value open: it covered everything after it. The undo | ||
| // now ends the run it brings back where that run ended when the item was | ||
| // deleted, with a null of its own (see Binding#closeRestoredRuns and | ||
| // #recordRunEnds). |
There was a problem hiding this comment.
Nit: this describes the fix as landed ("now ends ... see ... #recordRunEnds"), but recordRunEnds is not on main, and the cells it describes are test.fails here. Consider saying what main does and that the pins wait on the fix.
| const undone = authorUndoes | ||
| ? exactly(from, to, typedAfter) | ||
| : exactly(WORD[1], WORD[2], typedAfter); |
There was a problem hiding this comment.
Question: when the author undid first, this requires the value back on exactly [from, to). #9623 also accepts no comment at all, and the alternative described on #9633 would give that. Should these cells accept either, or is "back on the word" the decision? If it is, a DECISIONS.md row would record it.
…ays, not by the runner's speed ## Description The fuzz grouped ops into undo steps by the UndoManager's 500 ms capture window, so a slow runner split them differently and a pinned seed could pass. Undo steps now end where a second sequence from the seed says (after an op with chance `US_STEP_END`, default 0.1), with the capture window out of reach, and the seeds that spread a comment on main with the default profile are pinned again: 10, 23, 26, 35 and 51. ## Test plan main (1853707), twice under load: UndoSpreadFuzz 55 passed, 5 expected fail (60); the five files: all pass or fail as expected.
There was a problem hiding this comment.
This looks good. Checking exact extents on every client and in a fresh read is a much stronger bar than "on the word or nowhere". Every pinned test.fails/it.fails case fails on main at its extent or spread assertion, not during setup, and the other 266 pass. A few optional, non-blocking suggestions:
-
packages/lexical-y/src/__tests__/unit/regressions/UndoCommentRemovalSpread.test.ts:90: undo grouping depends on timing.- The editors use
YCollabExtension's default 500 mscaptureTimeout, and the file relies on A's two or three writes (lines 148-154) landing in one undo step. - If a slow runner splits them, three of the four
it.failscases pass on main, and Vitest reports a passingit.failsas a failure. SettingcaptureTimeout: 0reproduces it. - The fix this PR already applied to
UndoSpreadFuzzworks here too: a very largecaptureTimeoutplusundoManager.stopCapturing()where A's step should end. Putting A's writes in oneeditor.updatealso works.
- The editors use
-
Restore wins vs. retraction wins. Wherever the value's author undoes its own write first, the expected outcome is the value back on the word:
UndoRestoredRunEnds.test.ts:341-342- the non-concurrent cases of
UndoRestoredRunEndExtents.test.ts UndoCommentRemovalSpread.test.ts:169-182
#9623 accepts either answer: back on
two t, or nowhere because B undid adding it. It also notes that raw Yjs leaves noc2, and the redesign described on #9633 would give nowhere too. Is restore-wins the intended semantics for@lexical/y?- If so, an entry in
packages/lexical-y/DECISIONS.mdwould make it the documented bar for the fix. - If it is still open, the author-undoes cases could accept "exactly what the removal took, or nothing, never past it". The kept-by-author cases keep the exact check, which already catches a change that drops every restored value.
UndoRestoredRunEndExtents.test.tswould need a few kept-by-author cases for that.
-
Nit,
UndoRestoredRunEnds.test.ts:50-53: the header says the undo "now ends the run … (see Binding#closeRestoredRuns and #recordRunEnds)".recordRunEndsdoes not exist on main, and these are the cases pinned as failing. Describing main's current behaviour and what the pinned cases expect would avoid confusing the next reader.
A tests-only change needs no changeset (CONTRIBUTING.md, "Changesets").
| dependencies: [ | ||
| RichTextExtension, | ||
| MarkStateExtension, | ||
| configExtension(YCollabExtension, { |
There was a problem hiding this comment.
Optional: these editors use the default 500 ms captureTimeout, and the cases rely on A's writes (lines 148-154) landing in one undo step. If a slow runner splits them, three of the four it.fails cases pass on main (captureTimeout: 0 reproduces it), and Vitest reports that as a failure. The approach UndoSpreadFuzz uses here would make this deterministic: a very large captureTimeout plus undoManager.stopCapturing() where A's step should end.
| const undone = authorUndoes | ||
| ? exactly(from, to, typedAfter) |
There was a problem hiding this comment.
Question: when the value's author undoes first, this expects the removal's undo to bring the value back (restore wins). #9623 accepts either this or nowhere, and raw Yjs gives nowhere. If restore-wins is the intended semantics, a packages/lexical-y/DECISIONS.md entry would make it explicit. If it is still open, these cases could accept "exactly what the removal took, or nothing, never past it".
| // (#9620), left the value open: it covered everything after it. The undo | ||
| // now ends the run it brings back where that run ended when the item was | ||
| // deleted, with a null of its own (see Binding#closeRestoredRuns and | ||
| // #recordRunEnds). |
There was a problem hiding this comment.
Nit: this describes a fix that is not on main (recordRunEnds does not exist here), while the cases it calls fixed are pinned as test.fails. Consider describing main's current behaviour and what the pinned cases expect instead.
…'s author undid it, and end UndoCommentRemovalSpread's undo steps explicitly ## Description - Whether a collaborator's undo of a removal brings back a value its author undid is undecided (#9623 accepts either). Where the author undid the value, the restored-extent cases now accept the exact original extent or nothing, and still fail on anything more; where the author keeps it, the value must come back exactly (a new set of many-id cases checks every id). - UndoCommentRemovalSpread ends its undo steps where the test says (a large capture window and stopCapturing after each client's step), not by the runner's speed, and accepts c1 on exactly its words or nowhere. ## Test plan main (1853707), the five files: 276 passed, 190 expected fail.
There was a problem hiding this comment.
Thanks, this looks good to merge. The five files give #9623 and #9620 a precise bar: an undo of a removal must give the value back on exactly the characters it covered (or, where the value's author undid it, the exact extent or nothing). Every client and a fresh reader must agree, and the cells main fails are pinned as test.fails. Everything below is optional and non-blocking.
Optional suggestions
- Check every id's extent in the many-id cells (
regressions/UndoRestoredRunEndExtents.test.ts:89).check()tracks onlyc0. Theconcurrent empty restorecells (lines 318-359) write up to 256 ids pluskeep, and assert neither the other ids nor thatkeepstays on exactlyKilo. Onmainevery id spreads with this shape, soc0is representative today, but a fix that ends only some ids' runs would pass. A per-character check of every id, pluskeepasserted onKiloas the #9620 cells do (regressions/UndoRestoredRunEnds.test.ts:462), would close that. Themany comment keysblock has the same shape: it checks that every id is onKilo, but not where the ids other thanc0end. - Assert the edited paragraph in
regressions/UndoCommentRemovalSpread.test.ts(lines 185-186). Paragraphs 0-1 and 3-4 are compared, but neither paragraph 2's text (newgone after A's undo) nor the absence ofc2(whensecondis true) is checked. - Reword the header of
regressions/UndoRestoredRunEnds.test.ts(lines 50-53). It says "The undo now ends the run it brings back … (see Binding#closeRestoredRuns and #recordRunEnds)", butrecordRunEndsdoes not exist on this branch, and the file pinsmain's failures of that behaviour. Describing the expected behaviour instead keeps the comment accurate until the fix lands. - PR description:
UndoRestoredRunEndExtents.test.tshas 254 cases, not 244 (254 is what adds up to the 466 total). "Main fails every garbage-collection-on cell" doesn't cover the fivemany comment keys … gc=true author keepscells, which pass onmainand aren't pinned.
A question on the partial-removal cells
The 32 part of the word, its author undoes it first cells in regressions/UndoRestoredRunEnds.test.ts (gc on and off) fail on main because the removal's undo gives back the whole word. That is the D64 leftover (packages/lexical-y/DECISIONS.md, still OPEN), not the #9623/#9620 run end. packages/lexical-y/AGENTS.md asks that a known failure be a test.fails naming the issue or pull request that fixes it, and these name D64/#9560, which fits while D64 stays open for a fix. If D64 is later recorded as an accepted limit, should these cells move to the documented limits instead of staying pinned?
| (block as ElementNode).getAllTextNodes().flatMap((n: TextNode) => { | ||
| const on = | ||
| kind === 'comment' | ||
| ? $getNodeMarkIDs(n).includes('c0') |
There was a problem hiding this comment.
Optional: check() only tracks c0, so the concurrent empty restore cells (318-359), which write up to 256 ids plus keep, assert neither the other ids nor that keep stays on exactly Kilo. On main every id spreads with this shape, so c0 is representative today, but a fix that ends only some ids' runs would pass. A per-character check of every id, plus keep on Kilo as the #9620 cells do in UndoRestoredRunEnds.test.ts:462, would close it.
| // (#9620), left the value open: it covered everything after it. The undo | ||
| // now ends the run it brings back where that run ended when the item was | ||
| // deleted, with a null of its own (see Binding#closeRestoredRuns and | ||
| // #recordRunEnds). |
There was a problem hiding this comment.
Nit: "The undo now ends the run it brings back …" describes a fix this file pins as failing on main, and #recordRunEnds doesn't exist on this branch. Describing the expected behaviour instead keeps the header accurate until the fix lands.
| expect(a.slice(0, 2)).toEqual(PARAS.slice(0, 2)); | ||
| expect(a.slice(3)).toEqual(PARAS.slice(3)); |
There was a problem hiding this comment.
Nit: paragraph 2 is skipped here, and withC1 checks only c1. Asserting paragraph 2's text (new gone after A's undo) and that c2 is gone when second is true would cover the rest of A's undone step.
…any-id restored-extent cases ## Description The many-id cases checked the extent of c0 and the set of ids on the word only, so an id other than c0 spreading over the rest of the line or the next one passed. They now check every character of every line: the word has the restored set (every id, or, where the author undid them, every id or none), and every other character has none. ## Test plan main (1853707): UndoRestoredRunEndExtents 127 passed, 127 expected fail (254), as before.
There was a problem hiding this comment.
Thanks, this looks good to me. The pins match the bugs: each test.fails cell fails on main because the restored value runs past its extent, the unpinned cells pass, and the pinned seeds don't depend on Yjs client order. A few optional, non-blocking suggestions:
- Check every id in the concurrent many-id cells.
packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunEndExtents.test.tsL337-378 variescountfrom 1 to 256, butcheck()reads the value throughchars(), which looks only atc0(L89).@lexical/ystores each mark id as its own format key, so a fix that clearsc0but leaves another id on the text would still pass these cells. Reusing the per-character every-id loop from themany comment keyscells (L310-328) would cover it, withkeepexpected onKiloand nothing elsewhere. - Rethrow and dispose in
UndoCommentRemovalSpread.test.ts. The editors (L88-103) set noonError, so an exception only reachesconsole.error. If B's undo threw,c1would stay removed, and the case accepts that as "nowhere" (L202). The sibling files already passonError: error => { throw error; }. Adding that, pluseditor.dispose()in afinally, would make the case fail loudly once the pins come off. - Header comment in
UndoRestoredRunEnds.test.ts(L50-53). It says the undo "now" ends the restored run and points to#recordRunEnds, which doesn't exist in the package yet. Phrasing it as what the fix must do would avoid confusion. - Description nits. The description gives 244 cases for
UndoRestoredRunEndExtents.test.ts, but the file has 254 (the 466 total is right). TheUndoCommentRemovalSpreadbullet says each case checks thatc1is back on its words, but the case also acceptsc1nowhere.
A question for the #9623 fix rather than for this PR: where the value's author undid it first, these tests accept either the exact extent or nothing. Which one should a collaborator's undo of the removal produce?
| (block as ElementNode).getAllTextNodes().flatMap((n: TextNode) => { | ||
| const on = | ||
| kind === 'comment' | ||
| ? $getNodeMarkIDs(n).includes('c0') |
There was a problem hiding this comment.
Optional: chars() looks only at c0, so the concurrent empty restore count=N cells (L337-378) never check c1…c{N-1}, and they never check A's keep comment on Kilo either. Each mark id is its own format key in @lexical/y, so a fix that clears c0 but leaves another id on the text would pass. The per-character every-id loop from the many comment keys cells (L310-328) would cover it.
| synced: true, | ||
| }), | ||
| ], | ||
| name: `repro/${name}`, |
There was a problem hiding this comment.
Optional: these editors set no onError, so an exception only reaches console.error. If B's undo threw, c1 would stay removed, and the case accepts that as "nowhere" (L202). Consider onError: error => { throw error; } as in the sibling files, plus editor.dispose() in a finally.
| // (#9620), left the value open: it covered everything after it. The undo | ||
| // now ends the run it brings back where that run ended when the item was | ||
| // deleted, with a null of its own (see Binding#closeRestoredRuns and | ||
| // #recordRunEnds). |
There was a problem hiding this comment.
Nit: this reads as if main already ends the restored run where it ended at the delete, and #recordRunEnds doesn't exist in the package yet. Since the pinned cells below show main doesn't do this, consider phrasing it as what the fix must do.
## Description Merges main at c0c7e42 (#9640, #9600, #9652, #9659, #9648 and #9657). The one conflict is in the link list of `packages/lexical-y/DECISIONS.md`, where both sides added a reference (#9597 here, #9606 on main); both are kept. ## Test plan ### Before The branch was on main f59f74d. ### After `pnpm run ci-check` and `pnpm exec vitest run --project unit --project scripts-unit` pass on the merged tree (outputs in the pull request).
…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).
…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).
Description
Tests only, for #9623, #9620 and the D64 shape (#9560), so a fix for them is
judged by exact extents: where the value's author keeps it, the removal's
undo must give it back exactly; where the author undid it, the exact extent
or nothing is accepted (which one is undecided), and anything more fails. Every case checks that
all clients and a fresh read agree, and the exact characters the value is on.
regressions/UndoRestoredRunEndExtents.test.ts(244 cases): a collaboratorremoves a value (comment, bold, italic, underline, color, NodeState, link)
from a word; its author undoes the value, maybe writing the same key on a
later range before or after; a third client may delete text around the
value's end; the collaborator undoes the removal: the value is back on
exactly the word. Also 1 to 256 comment ids in one write, and the
concurrent shape of 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 (the many-id cases check every id, not only the
first). Main fails every garbage-collection-on cell and
the concurrent cells (
test.fails).regressions/UndoRestoredRunEnds.test.ts(147 cases): comment, bold,color and NodeState, removed from the word, part of it or the whole
document, the author undoing first or not, text typed after the word,
garbage collection on and off, two id orders; undo, redo and undo again,
each with its exact extent and every block's text. Main fails the author-undoes cells with garbage
collection on (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) and the partial removals (the author's undo leaves
the rest of the word with the value the removal restated, then the
removal's undo gives the whole word back: D64), 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 cells.
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. Undo steps end where a second sequence from the seed says
(
US_STEP_END, default 0.1), not by the runner's speed. 60 seeds by default(
US_SEEDS,US_START,US_OPS,US_UNDO,US_STEP_END); the 5 seedsthat spread a comment on main are
test.failswith the default profileonly.
regressions/UndoCommentRemovalSpread.test.ts: 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 with a second commentand typing in the same undo step (4 cases, all fail on main); each also
checks that c1 is back on its words.
regressions/UndoKeepsLaterFormat.test.ts: an undo of a delete keeps theitalic a collaborator gave the text afterwards (passes on main; a fix that
ends the runs an undo brings back where they ended at the delete broke it).
Test plan
main (1853707) with these files:
pnpm run ci-checkpasses.