Repository navigation
[lexical-y] Chore: Pin the open undo-restore cases (#9689, #9690) and record the #9671/#9599 rules' trade (D95) - #9691
Merged
Merged
Conversation
… record the #9671/#9599 rules' trade (D95) ## Description Tests and decision-register entries only; no behavior change. - `regressions/UndoRestoredRunOverDeletedText.test.ts` (64 cells): a value whose text and closer its author undid while a collaborator removed it, and the collaborator undoes the removal. 28 cells spread the value over the rest of the document on `main` (gc on and off): `it.fails` naming #9689. The other 36 pass and guard the shapes around them (a value typed at the end of the document, the same comment id of the other client after it, another comment id after it). - `regressions/UndoRestoredRunSharedEnd.test.ts` (16 cells, all pass on `main`): a run that reused an end the document already held, with a later separate write by the same author that is undone. A fix for #9689 that trusted the end the author's write appears to give the run was parked (#9687) because it broke this case; these cells keep a future fix honest. - `regressions/FormatRuleTrade.test.ts` and `regressions/FormatRuleTradeSessions.test.ts` (10 cells, `test.fails` naming #9690): sequences that the rules added for #9671 and #9599 (D78) made wrong, including a single client undoing its own three steps and losing bold on a word none of them touched. Each passed on the commit before those rules. - `DECISIONS.md`: D95 (`OPEN`) records that trade with its numbers; D65 notes #9689 and the parked #9687. ## Test plan ### Before The new files do not exist on `main`. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{FormatRuleTrade,FormatRuleTradeSessions,UndoRestoredRunSharedEnd,UndoRestoredRunOverDeletedText}.test.ts Tests 52 passed | 38 expected fail (90) $ LEXICAL_Y_GC=1 vitest --run --project unit (same files) Tests 52 passed | 38 expected fail (90) $ pnpm run ci-check exit 0 ``` The `test.fails` cells were checked against the commit before #9679: the #9690 cells pass there (the three-client sessions with gc off), and fail on `main` with gc off and on.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
November 1, 2026 02:59
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
## Description Adds the fully sequential shape of #9689 to its pins: A and B comment the same word (c1, c2); A types at its start; B undoes its comment and removes c2; A undoes; B undoes the removal. c2 comes back over the rest of the paragraph and the paragraphs after it. `it.fails` naming #9689, gc off and on, both client id orders. Tests only. ## Test plan ### Before The cells do not exist. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunOverDeletedText.test.ts Tests 36 passed | 32 expected fail (68) $ LEXICAL_Y_GC=1 (same file) Tests 36 passed | 32 expected fail (68) ``` Without `it.fails` the new cells fail with `"two |two two[c2]", "three three three[c2]"`.
… and costs ## Description Replaces D95's figures with per-rule numbers: each of the three rules disabled alone on the same fresh seeds (a raw-Yjs-oracle op-list generator, its original set, a three-client view property, a sequential undo fuzz, a run-sequence fuzz, unit cells, a long-session soak), and keeps the single-client case first. Documentation only. ## Test plan ### Before D95 gave totals only. ### After ``` $ prettier --check packages/lexical-y/DECISIONS.md All matched files use Prettier code style! ```
…scope D95 ## Description Review follow-ups, tests and docs only: - The `it.fails` / `test.fails` cells name their open issue in the test title (#9689, #9690); the #9671/#9599 rule references stay as context. - D95 says none of the narrower variants tried kept the gains, and lists stored provenance and dropping the restore rule as options under consideration rather than the only ones. - `FormatRuleTradeSessions.test.ts`: the cells pass before the rules with gc off and on (the header said gc on failed there too; with the binding sources from before #9679 and today's harness all ten trade cells pass under both settings). ## Test plan ### Before Titles without the issue numbers. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{FormatRuleTrade,FormatRuleTradeSessions,UndoRestoredRunSharedEnd,UndoRestoredRunOverDeletedText}.test.ts Tests 52 passed | 42 expected fail (94) $ LEXICAL_Y_GC=1 (same files) Tests 52 passed | 42 expected fail (94) ```
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good to me. The 42 pinned cells fail on the content assertions they describe, after both peers converge. The 52 guard cells pass. D95 and D65 record the trade with the evidence behind it. A few optional, non-blocking suggestions:
- Pin gc on two #9690 sessions (
packages/lexical-y/src/__tests__/unit/regressions/FormatRuleTradeSessions.test.ts:103and:135). Sessions 107991 and 111874 pass nogc, so they take it fromLEXICAL_Y_GC(the harness'sWITH_GC). With the three rules short-circuited inCursor.ts(the twoisOwnInsertFormatbranches informatand the other-client check inrunOpener), both sessions pass with gc off but still fail withLEXICAL_Y_GC=1. So for these two, the header's "They passed before those rules, gc off and on" (line 14) does not hold with gc on, and #9690 itself notes this for the 111874 script.packages/lexical-y/AGENTS.mdasks for a control on the baseline a failure claims. Passinggc: falseto both sessions, as session 110073 passesgc: true, and narrowing the header would keep the pin's claim exact. - Comments in
FormatRuleTrade.test.tsthat don't match the ops:- Line 178 says italic over "ef", but
{i: 4, j: 9}coversefghi. - Line 198 says Z is typed into the struck text, but it lands after the plain
ghi. - The header (line 17) says A's character stays plain, but
runfilters out characters that start with a digit (line 168), so that is never asserted.
- Line 178 says italic over "ef", but
DECISIONS.mdlinks. D95 (line 140) describes the rules #9679 added but doesn't link #9679. D65's Links column (line 139) leaves out #9687, though its text cites it. The register's preamble says "Links go to the pull requests, issues and docs that carry it." A "see D95" on D78 (line 61) would also help readers find the cost of the scan rule.UndoRestoredRunSharedEnd.test.ts:112: the editor namereview9687/provenance/${id}looks like a leftover.
No changeset is needed, since only tests and the package's decision register change. If the Changesets check asks for one, the skip-changeset label fits, as CONTRIBUTING.md describes.
| [0, 0, 0, 8, 2, 1, 0, 0], | ||
| [0, 0, 0, 0, 0, 8, 8, 0], | ||
| ], | ||
| {ids: [2, 1, 3]}, |
There was a problem hiding this comment.
Optional: this session (and 111874 at line 135) takes gc from LEXICAL_Y_GC. With the three #9679 rules short-circuited, it passes with gc off but still fails with LEXICAL_Y_GC=1, so the header's "gc off and on" (line 14) doesn't hold here. Passing gc: false, as 110073 passes gc: true, would keep the pin exact.
| ops: Op[]; | ||
| }[] = [ | ||
| { | ||
| // italic over "ef", Z typed inside, italic over "Z" and the rest; A types 1 |
There was a problem hiding this comment.
Nit: {i: 4, j: 9} italicizes efghi, not "ef". Line 198's Z also lands after the plain ghi rather than inside the struck text.
| | D64 | **Undoing a format while a collaborator edits inside its range.** | Yjs format items cannot be conditional on another item; the remaining option is a core or storage change. | `OPEN` | 10-30 | [#9560], [#9521], [#9682] | | ||
| | D65 | **A comment spreading after an offline client types plain text into it.** | Since D86 a delete keeps every format item it leaves, so the [#9609] cells where a collaborator's delete took the run's end pass; the cells where the restore's run has no end of its own (`OwnRunRestoreEnd.test.ts`) still fail. [#9562], which extended the restore's end rule to runs the writer opened itself, is parked: each attempt regressed a case main handles. The case is tracked in [#9609], with the tests moving to main in [#9605]. A restore in a run whose format change wrote more than 64 items gets no end, because the end scan stops there ([#9603]). | `OPEN` | 10-30 | [#9564], [#9609], [#9562], [#9605], [#9603] | | ||
| | D65 | **A comment spreading after an offline client types plain text into it.** | Since D86 a delete keeps every format item it leaves, so the [#9609] cells where a collaborator's delete took the run's end pass; the cells where the restore's run has no end of its own (`OwnRunRestoreEnd.test.ts`) still fail. [#9562], which extended the restore's end rule to runs the writer opened itself, is parked: each attempt regressed a case main handles. The case is tracked in [#9609], with the tests moving to main in [#9605]. A restore in a run whose format change wrote more than 64 items gets no end, because the end scan stops there ([#9603]). An undo of a removal can bring back a run whose text and closer its author undid at the same time, and that run then has no end ([#9689]); ending it at the end the author's write gave the run (`changeEnd`) was tried and parked ([#9687]): the restoring client cannot tell that end from the end of a later, separate write by the same author. | `OPEN` | 10-31 | [#9564], [#9609], [#9562], [#9605], [#9603], [#9689] | | ||
| | D95 | **The rules that fixed #9671 and #9599 make rare other cases wrong, one of them for a single client.** A format over a range that starts at the writer's own earlier insert null removes that null (the null rule), one whose range ends at the writer's own earlier insert restore removes that restore and joins the runs (the restore rule; both [#9671]), and a restore's scan for the run it restates stops at another client's text (the scan rule, D78, [#9599]). | The restore rule also changes a single client's own undo, with no collaborator acting: type plain text into a bold run, bold a range ending at it, type plain text later on, undo the three steps, and a word none of them touched loses its bold (main before these rules was right) ([#9690]). Each rule disabled alone, same fresh seeds, gc on and off (counts of runs main gets right and the rule's absence wrong, then the reverse): a raw-Yjs-oracle op-list generator, 36,000 runs: null rule 18 / 4, restore rule 28 / 0, scan rule 2 / 1 (gc on and off identical; all three off equals main before them); its 18,000-run original set: 20 / 0, 6 / 0, 0 / 0; a three-client view property, 15,000 seeds: null rule 0 / 2 (gc off), restore rule 0 / 1 (gc on), scan rule 2 / 0; a sequential undo fuzz, 3,000 seeds: restore rule 0 / 2 (one single-client); a two-client run-sequence fuzz, 1,000: scan rule 7 / 0; unit cells: null rule 8, restore rule 16, scan rule 5; a long-session soak, about 20,600 sessions: 2 sessions wrong (null rule 1, scan rule 1) against 25 the other way. None of the narrower variants tried kept the gains: the writer cannot tell from the document whether a collaborator's lone null ends a format range or starts a removal (a variant of the null rule that writes the range's own opener fixed its cases and lost all 8 of its unit cells). Kept for now. Options under consideration include stored provenance and dropping the restore rule because of the single-client case. | `OPEN` | 10-31 | [#9690], [#9671], [#9599] | |
…ents; link #9679 in D95 ## Description Review follow-ups, tests and docs only: sessions 107991 and 111874 pass `gc: false` (the setting they passed with before the rules; 110073 already pins `gc: true`), and the header says so; three comments in `FormatRuleTrade.test.ts` now match their ops; D95 links #9679. ## Test plan ### Before The two sessions took gc from `LEXICAL_Y_GC`. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{FormatRuleTrade,FormatRuleTradeSessions,UndoRestoredRunSharedEnd,UndoRestoredRunOverDeletedText}.test.ts Tests 52 passed | 42 expected fail (94) $ LEXICAL_Y_GC=1 (same files) Tests 52 passed | 42 expected fail (94) ```
There was a problem hiding this comment.
This looks good. It is tests and decision-register text only, and the pinned cells match the issues and rules they name. A few optional, non-blocking suggestions:
packages/lexical-y/src/__tests__/unit/regressions/UndoRestoredRunSharedEnd.test.ts:173-174and:205-206assert whatchangeEndreturns today. They read as scenario checks, but a future #9689 fix that teacheschangeEndwhich write an end belongs to would fail them even when the document comes out right (tailkeepsc1). Consider a short comment saying a fix may change them, or asserting the stored items instead.- Same file: the extension name
review9687/provenance/${id}(:112) looks left over. "No Item is inserted or deleted by this test" (:77) reads as if the editors wrote nothing; "by hand" would make it accurate. packages/lexical-y/src/__tests__/unit/regressions/FormatRuleTrade.test.ts:11: the header says each sequence leaves a format on text nobody formatted, but the scan-rule case loses one (mnoloses its strikethrough). "A format spreads or is lost" covers both, as #9690's title does.packages/lexical-y/DECISIONS.md:139: D65's text cites [#9687], but its Links column adds only [#9689]. The register's preamble says links go to the pull requests and issues that carry a row, and the earlier parked [#9562] is listed there.- The description's test plan says the #9690 cells pass before #9679 "(the three-client sessions with gc off)". Since the last commit, session 110073 is pinned to
gc: true. AGENTS.md asks the Test plan to show what was actually run, so the parenthetical is worth updating.
No changeset is needed: CONTRIBUTING.md exempts tests, and DECISIONS.md is not part of the published package.
Comment on lines
+204
to
+207
| expect( | ||
| changeEnd(a.doc, opener), | ||
| 'clock scan crosses a separate write', | ||
| ).toBe(unrelated); |
There was a problem hiding this comment.
Optional: this precondition, like the one at :173-174, pins what changeEnd returns today. A #9689 fix that teaches changeEnd which write an end belongs to would fail here even with the right document (tail keeping c1). Consider a comment saying a fix may change it, or asserting the stored items instead.
Member
Author
There was a problem hiding this comment.
Agreed; the change is ready (468d417) but not pushed: the branch now refuses pushes (protected), so it waits for the maintainer.
|
|
||
| // @vitest-environment node | ||
|
|
||
| // Rare sequences in which a format ends up on text nobody formatted, made |
There was a problem hiding this comment.
Optional wording: the scan-rule case loses a format instead (mno loses its strikethrough), so "a format spreads or is lost" would cover both cases, as #9690's title does.
Member
Author
There was a problem hiding this comment.
Agreed; the change is ready (468d417) but not pushed: the branch now refuses pushes (protected), so it waits for the maintainer.
| | D63 | **A line without a marker joins a paragraph a collaborator appends.** | Every reader rule considered changes how stored documents read. | `OPEN` | 10-30 | [#9524] | | ||
| | D64 | **Undoing a format while a collaborator edits inside its range.** | Yjs format items cannot be conditional on another item; the remaining option is a core or storage change. | `OPEN` | 10-30 | [#9560], [#9521], [#9682] | | ||
| | D65 | **A comment spreading after an offline client types plain text into it.** | Since D86 a delete keeps every format item it leaves, so the [#9609] cells where a collaborator's delete took the run's end pass; the cells where the restore's run has no end of its own (`OwnRunRestoreEnd.test.ts`) still fail. [#9562], which extended the restore's end rule to runs the writer opened itself, is parked: each attempt regressed a case main handles. The case is tracked in [#9609], with the tests moving to main in [#9605]. A restore in a run whose format change wrote more than 64 items gets no end, because the end scan stops there ([#9603]). | `OPEN` | 10-30 | [#9564], [#9609], [#9562], [#9605], [#9603] | | ||
| | D65 | **A comment spreading after an offline client types plain text into it.** | Since D86 a delete keeps every format item it leaves, so the [#9609] cells where a collaborator's delete took the run's end pass; the cells where the restore's run has no end of its own (`OwnRunRestoreEnd.test.ts`) still fail. [#9562], which extended the restore's end rule to runs the writer opened itself, is parked: each attempt regressed a case main handles. The case is tracked in [#9609], with the tests moving to main in [#9605]. A restore in a run whose format change wrote more than 64 items gets no end, because the end scan stops there ([#9603]). An undo of a removal can bring back a run whose text and closer its author undid at the same time, and that run then has no end ([#9689]); ending it at the end the author's write gave the run (`changeEnd`) was tried and parked ([#9687]): the restoring client cannot tell that end from the end of a later, separate write by the same author. | `OPEN` | 10-31 | [#9564], [#9609], [#9562], [#9605], [#9603], [#9689] | |
Member
Author
There was a problem hiding this comment.
Agreed; the change is ready (468d417) but not pushed: the branch now refuses pushes (protected), so it waits for the maintainer.
zurfyx
added a commit
that referenced
this pull request
Nov 1, 2026
…ome guards, D95 moot, gc-on #9623 cells pinned, D51 back to an accepted limit ## Description Rebased on `main` 064b18e (#9688, #9691). Without #9679's rules: - The #9690 cells (`FormatRuleTrade.test.ts`, `FormatRuleTradeSessions.test.ts`) pass: they become plain tests, guards against bringing those rules back as they were. D95 is marked moot (`REVERTED`). - `UndoRestoredRunOverDeletedText.test.ts`: with gc on, the sequential order of six shapes spreads again (the undo cannot find items gc collected): `it.fails` with gc on, titled "(#9623 with gc on)". The #9689 cells are unchanged. - `UndoInvisibleSteps.test.ts` (#9688): the cell that needed a delete to keep the comment removal's null passes again with the collapse; it becomes a plain test, with its comment updated. - D51 goes back to `ACCEPTED LIMIT` (its status before #9679), keeping the D86 sentence and the revert note. ## Test plan ### Before On 064b18e with the revert alone, `FormatRuleTrade*` and one `UndoInvisibleSteps` cell failed as unexpected passes, and twelve gc-on cells of `UndoRestoredRunOverDeletedText` failed. ### After ``` $ LEXICAL_Y_GC=0 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/{UndoInvisibleSteps,UndoRestoredRunOverDeletedText,FormatRuleTrade,FormatRuleTradeSessions}.test.ts Tests 35 passed | 46 expected fail (81) $ LEXICAL_Y_GC=1 (same files) Tests 35 passed | 46 expected fail (81) ```
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Tests and decision-register entries only; no behavior change.
regressions/UndoRestoredRunOverDeletedText.test.ts(64 cells): a valuewhose text and closer its author undid while a collaborator removed it,
and the collaborator undoes the removal. 28 cells spread the value over
the rest of the document on
main(gc on and off):it.failsnamingBug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid the typing and comment together #9689. The other 36 pass and guard the shapes around them (a value typed
at the end of the document, the same comment id of the other client after
it, another comment id after it).
regressions/UndoRestoredRunSharedEnd.test.ts(16 cells, all pass onmain): a run that reused an end the document already held, with a laterseparate write by the same author that is undone. A fix for Bug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid the typing and comment together #9689 that
trusted the end the author's write appears to give the run was parked
([lexical-y] Bug Fix: Keep a comment brought back by undoing its removal off the rest of the document when its author undid the text it was on #9687) because it broke this case; these cells keep a future fix honest.
regressions/FormatRuleTrade.test.tsandregressions/FormatRuleTradeSessions.test.ts(10 cells,test.failsnaming Bug: [lexical-y] Undoing a collaborator's format steps around text you typed can spread or drop a format, in rare cases the #9671 and #9599 rules introduced #9690): sequences that the rules added for Bug: [lexical-y] Undoing text you typed into a format run and then formatted takes the format off the untouched rest of the run when a collaborator typed after it, with no concurrent edits #9671 and Bug: [lexical-y] Undoing your own formats spreads a format onto the next word after a collaborator typed plain text inside your first run #9599 (D78)
made wrong, including a single client undoing its own three steps and
losing bold on a word none of them touched. Each passed on the commit
before those rules.
DECISIONS.md: D95 (OPEN) records that trade with its numbers; D65notes Bug: [lexical-y] A comment comes back over the rest of the document when a collaborator undoes removing it after its author undid the typing and comment together #9689 and the parked [lexical-y] Bug Fix: Keep a comment brought back by undoing its removal off the rest of the document when its author undid the text it was on #9687.
Test plan
Before
The new files do not exist on
main.After
The
test.failscells were checked against the commit before #9679:the #9690 cells pass there (the three-client sessions with gc off), and
fail on
mainwith gc off and on.Follow-up commits
it.fails(4 more cells).Totals now:
Tests 52 passed | 42 expected fail (94), gc off and on.