Repository navigation
[lexical-website][lexical-y] Bug Fix: Keep every unchanged word between two changes of text set with setTextContent - #9667
Merged
Conversation
…en two changes of text set with setTextContent ## Description Text set whole with `setTextContent` (and the DOM's text, which the binding diffs the same way) kept only the start and end it shares with the old text (`setTextEnds`), so with two or more words changed everything between the first and the last change was written again: a word a collaborator deleted there at the same time came back, and showed twice after the collaborator's undo. #9621 had the same shape for text made again, which #9625 fixed with D29's word diff. Set text now keeps, between its common start and end, every run of unchanged words a shortest word-by-word edit script keeps, with the same function text made again uses (`withKeptWords`, split out of `keptStretches`; `setTextStretches` = `setTextEnds` + it), recorded (`TextReplay#align`) and diffed by the writer (`Write`'s fallback now writes one edit per gap between kept runs). The start and end still come from `setTextEnds`, so a caret still places a single insertion or deletion (#9641, #9657), and an IME composition keeps the exact diff. The composition of an edit and a set within one update (`TextReplay#compose`) is unchanged. y.md ("Edits made by code") and D29 say so. Closes #9666 ## Test plan `regressions/SetTextStretches.test.ts`: the issue's line with two words changed while a collaborator deletes `t5z`, then undoes (and three words changed with one removed; one word changed as control), recorded and diffed by the writer, both id orders; and a property over random lines set with 2 to 4 word edits (replaced, inserted, removed) while a collaborator deletes from, types into, or deletes and undoes in an unchanged word, unread and concurrent, recorded and diffed by the writer (60 seeds per cell; 300 also pass). ### Before On main (21bcf62): ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts × two words changed, recorded (ids 1,2) × three words changed, one removed, recorded (ids 1,2) × two words changed, diffed by the writer (ids 1,2) × three words changed, one removed, diffed by the writer (ids 1,2) ... (the same for ids 2,1) × unread, recorded: lines set with 2 to 4 word edits ‖ the peer deletes an unchanged word ... (all 12 property cells) Tests 20 failed | 4 passed (24) ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts Tests 24 passed (24) $ LEXICAL_Y_SEEDS=300 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts packages/lexical-y/src/__tests__/unit/regressions/SetTextWordEdges.test.ts Tests 97 passed (97) $ pnpm run ci-check EXIT 0 $ vitest --run --project unit --project scripts-unit Test Files 669 passed | 4 skipped (673) Tests 31543 passed | 470 expected fail | 135 skipped (32148) $ MATRIX_FULL=1 vitest --run --project unit packages/lexical-y/src/__tests__/unit/matrix Tests 11 failed | 16001 passed | 2 expected fail | 128 skipped (16142) (the 11 known cells, as on main) $ FUZZ_SEED_START=1 FUZZ_SEEDS=3000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/Fuzz.test.ts Tests 1 failed | 123 passed (124) (seed 2277: dup, as on main) $ FUZZ_SEED_START=50000 FUZZ_SEEDS=2000 vitest --run --project unit packages/lexical-y/src/__tests__/unit/Fuzz.test.ts Tests 7 failed | 77 passed (84) (seeds 50180, 50231, 50372, 51439, 51507, 51972, 51976, as on main) ``` A randomized session generator (lines set with `setTextContent` by an agent with concurrent comments, formats, typing, deletes and undo), 500 sessions, main vs this change: deletes brought back by a rewritten middle 25 vs 0, collaborator edits lost in a rewritten middle 27 vs 0, words twice 2 vs 0; the other classes (documented limits) equal.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 31, 2026 08:30
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: |
![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. Set text now uses the same word diff that #9625 gave remade lines. Single edits and IME composition keep their current placement. The new SetTextStretches.test.ts checks both peers and a fresh reader after the sync and after the collaborator's undo. The suggestions below are optional and non-blocking.
Optional follow-up
packages/lexical-y/src/internal/Write.ts:3147: the new per-gap write is in$writeTextEdit, which only handles a line where exactly one text node changed (Write.ts:3062-3076). A set can happen in the same update as another change to its line, and the recorded edits may not explain that line. Then it still goes through the general sequence writer, which writes the middle again as before. For example: with text-edit recording off, set the issue's line and append a bold text node in the same update while a collaborator deletest5z.t5zstill comes back, and shows twice after the undo. With recording on, as the binding sets it, that same update now keeps the delete. A case like that inSetTextStretches.test.tswould pin it. This is fine as a follow-up issue.
Nits
packages/lexical-website/docs/collaboration/y.md:839: the new text makes a 109-character line in a paragraph wrapped at 80 columns..prettierignoreexcludes Markdown, so it needs re-wrapping by hand.packages/lexical-y/DECISIONS.md:63: the D29 row now covers set text. DECISIONS.md asks a pull request that changes a recorded decision to update its row. Consider adding[#9666]to the Links column, and updating the date too, since dates record when a decision was made.
| const {prefix, suffix} = setTextEnds( | ||
| // What it keeps of the old text (its common start and end, and every | ||
| // run of unchanged words between them), and the hunksToWrite between. | ||
| const kept = setTextStretches( |
There was a problem hiding this comment.
Optional follow-up, non-blocking: this fast path only runs when exactly one text node of the line changed (lines 3062-3076). A set in the same update as another change to its line, which the recorded edits do not explain, still goes through the general sequence writer and rewrites the middle as before. For example, with text-edit recording off, setting the issue's line and appending a bold node in one update while a collaborator deletes t5z still brings t5z back. A follow-up issue is fine.
| was made without one. Without a caret that places it (none, or one at | ||
| was made without one, and between two changes the words the two texts | ||
| share, as text made again keeps them (a word a collaborator deletes there | ||
| at the same time stays deleted; not in an IME composition). Without a caret that places it (none, or one at |
There was a problem hiding this comment.
Nit (optional): this line runs to 109 characters in a paragraph wrapped at 80. .prettierignore excludes Markdown, so it needs re-wrapping by hand.
| | D27 | **Moves are a delete and a re-insert** at launch; the binding rewrites the cheaper side. Real moves within a container ([#9449], records after Yjs's unmerged `ContentMove` design) are paused and land last, backward compatible: a new layout version that older clients open read only. Move records leave tombstones, as every Yjs binding does. Moves across containers stay copies. | Every Yjs binding turns moves into delete plus insert; Yjs dropped its own move for complexity. Reliability of the rest came first. | `DECIDED` | 10-25 | [#9449], [#9455], [y.md: Limits][limits] | | ||
| | D28 | **Text identity is fixed at the source.** While a binding is attached, core records text edits (`spliceText`, insert and remove, split and merge), and text entity plugins record a grow as a move and a remake as a retype. Text only `setTextContent` or the DOM changed is diffed. The binding adds no per-case heuristics. | No binding can recover intent an API dropped; special-case rules were fragile and regressed neighbouring cases. | `DECIDED` | 10-27 | [#9483], [y.md: Edits made by code][y-code] | | ||
| | D29 | **Remade text keeps its common start and end by character**, whatever the node boundaries (y-prosemirror's `updateYText` rule), **and between two changes every run of unchanged whole words** (the longest common run of words, by a linear-space shortest edit script over the words both texts have, up to 2,048 edits, words only added or only removed between a shared start and end taking none, linear in the line's length; past it, only the start and end; where a word repeats next to a change, which occurrence is kept follows that edit script (`aa bb` made again as `aa aa bb` keeps the first `aa`); a word the update moved is written again; a word changed only in part is written whole, so a collaborator's undone delete of its unchanged part shows twice, [#9635]), and never brings back a collaborator's delete. It replaces the word matching of [#9565]. | It is the industry-standard behaviour, and word matching glued words together and merged numbers oddly. Keeping only the start and end wrote everything between two changed words again, so a collaborator's delete there came back, and showed twice after the collaborator's undo ([#9621]). | `DECIDED` | 10-30 | [#9565], [#9598], [#9621] | | ||
| | D29 | **Remade text keeps its common start and end by character**, whatever the node boundaries (y-prosemirror's `updateYText` rule), **and between two changes every run of unchanged whole words** (the longest common run of words, by a linear-space shortest edit script over the words both texts have, up to 2,048 edits, words only added or only removed between a shared start and end taking none, linear in the line's length; past it, only the start and end; where a word repeats next to a change, which occurrence is kept follows that edit script (`aa bb` made again as `aa aa bb` keeps the first `aa`); a word the update moved is written again; a word changed only in part is written whole, so a collaborator's undone delete of its unchanged part shows twice, [#9635]), and never brings back a collaborator's delete. Text set whole (`setTextContent`, the DOM's text) keeps its unchanged words between two changes the same way ([#9666]). It replaces the word matching of [#9565]. | It is the industry-standard behaviour, and word matching glued words together and merged numbers oddly. Keeping only the start and end wrote everything between two changed words again, so a collaborator's delete there came back, and showed twice after the collaborator's undo ([#9621]). | `DECIDED` | 10-30 | [#9565], [#9598], [#9621] | |
There was a problem hiding this comment.
Nit (optional): DECISIONS.md asks a pull request that changes a recorded decision to update its row. Consider adding [#9666] to D29's Links column, and the date too, since dates record when a decision was made.
…et at once in the writer, and pin the set that also changes a node's format ## Description Review follow-up. Where text edits are not recorded, the writer's fast path (`$writeTextEdit`) diffed a line only when exactly one text node changed; with several text nodes set in one update the line went to the general writer, which does not diff the text, so a word a collaborator deleted in one of them came back. The fast path now takes every changed text node of the line (each keeping its type, format, style, mode, detail and state), works out each node's edits first (the record's, or `setTextStretches`' diff anchored at the caret; nothing is written if one node holds another's characters), then writes them from the last node to the first, so each lands at its old offset. A set that also changes the node's format or style in the same update, where text edits are not recorded, still writes the node whole (its attributes and text in one write); that stays open in #9666, pinned with expected failures and listed in y.md and D29. Recorded, it keeps the unchanged words. ## Test plan New cells in `regressions/SetTextStretches.test.ts`: a line of 2, 4 and 16 differently formatted text nodes each set with its first and last words changed while a collaborator deletes a word of the first, recorded and diffed by the writer, both id orders; a set that also toggles bold (recorded passes; diffed by the writer is `test.fails` for #9666). ### Before On a3e662d, the review's partition repro: diffed by the writer, 6 of 12 fail (main: 12 of 12). ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts Tests 38 passed | 2 expected fail (40) review partition repro: 12/12 $ pnpm run ci-check EXIT 0 $ vitest --run --project unit --project scripts-unit Test Files 669 passed | 4 skipped (673) Tests 31557 passed | 472 expected fail | 135 skipped (32164) $ MATRIX_FULL=1 vitest --run --project unit packages/lexical-y/src/__tests__/unit/matrix Tests 11 failed | 16001 passed | 2 expected fail | 128 skipped (16142) (the 11 known) $ Fuzz 1-3000: seed 2277 dup; 50000-51999: seeds 50180, 50231, 50372, 51439, 51507, 51972, 51976 (all as on main) ``` The randomized `setTextContent` session generator (500 sessions) gives the same result as the first commit: no rewritten-middle losses, resurrections or words twice.
…one pass of one cursor ## Description Review follow-up. The writer's diff of several text nodes set at once wrote them from the last to the first, each with a cursor built at the line's start and advanced to the node's old offset, so the cursor walked the line once per node: for 1,024 changed nodes about a million cursor steps (a second, against 54 ms). The nodes are now written from the first to the last with one cursor: each starts at its old offset moved by the edits written before it, and `$writeHunks` returns where the cursor ends (`$writeEdits` takes the cursor at the node's start). ## Test plan A new cell in `regressions/SetTextStretches.test.ts` counts the cursor's steps for 256 and 1,024 differently formatted text nodes set at once, diffed by the writer: four times the nodes must take less than six times the steps. ### Before On e8671d2: ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts -t 'one pass' × #9666: many text nodes set at once, diffed by the writer, in one pass AssertionError: expected 15.815384615384616 to be less than 6 ``` ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts Tests 39 passed | 2 expected fail (41) review cost repro: 64 / 256 / 1,024 nodes: 446 / 1,790 / 7,166 cursor steps, 17 / 27 / 79 ms $ pnpm run ci-check EXIT 0 $ vitest --run --project unit --project scripts-unit Test Files 669 passed | 4 skipped (673) Tests 31558 passed | 472 expected fail | 135 skipped (32165) $ MATRIX_FULL=1: 11 failed (the 11 known) | 16001 passed | 2 expected fail $ Fuzz 1-3000 and 50000-51999: the same seeds as main ```
There was a problem hiding this comment.
This looks good to me. Keeping the unchanged words between two changes of set text closes the #9621-shaped hole for setTextContent. The new regression file covers both peers, a fresh reader, undo, both client-id orders, and both the recorded and the writer-diffed paths. The suggestions below are optional and non-blocking.
1. The documented exception is narrower than the code (packages/lexical-website/docs/collaboration/y.md lines 837-842, and D29 in packages/lexical-y/DECISIONS.md). When text edits are not recorded, the caveat names only an update that also changes the node's format or style (D29 says attributes). But $writeTextEdit in packages/lexical-y/src/internal/Write.ts also drops the per-node diff in other cases, and the full line write it falls back to rewrites the middle as before. Those cases are an update that adds, removes or splits a node of the same line, a changed node that runs with a neighbour, and a change to the node's mode, detail or NodeState. For example, with _recordTextEdits = false, take a line t1z t2z t3z t4z t5z t6z t7z set to t1z n9z t3z t4z t5z t6z n8z while a node is appended to the same line. If a collaborator deletes t5z at the same time, every client ends with t5z back (main does the same). packages/lexical-y/AGENTS.md asks for a known failure to be a test.fails naming its issue, so consider these three changes:
- widen the caveat, for example to "…changes the node's attributes or the line's nodes…";
- pin that case next to the format pin in
SetTextStretches.test.ts; - list #9666 under y.md's Limits → Open issues.
2. Refresh the Test plan output. The pasted Before and After counts (20 failed | 4 passed (24) and 24 passed (24)) come from the first commit. The file now has 41 cells, including the several-nodes, format-pin and counted cells added later. AGENTS.md (Commits and Pull Requests) asks for the real Before and After output of the command that was run.
A small note: the counted one-pass cell also passes on main, because main sends a line with several changed nodes to the full line write. It guards the new path's linear cost rather than showing a difference from main, which is fine for a guard.
Comment on lines
+839
to
+841
| at the same time stays deleted; not in an IME composition, and not where | ||
| the editor does not record text edits and the same update also changes | ||
| the node's format or style, which writes the node again: |
There was a problem hiding this comment.
Optional: when text edits are not recorded, other updates also take the full line write in $writeTextEdit and bring a collaborator's delete back. That happens when the update adds, removes or splits a node of this line, or changes the node's mode, detail or NodeState. So this caveat, and D29's "attributes", is narrower than the code. Consider "…changes the node's attributes or the line's nodes…", with a test.fails pin next to the format one.
…fast path only where text edits are not recorded ## Description Review follow-up. With text edits recorded, a line whose several text nodes changed in one update went through the line writer before this pull request; the second commit sent it through the fast path too, which changed what Yjs stores for those edits and so what an undo restores: one undo undid two updates (a join and a bold, with a collaborator's concurrent bold and delete; a randomized interleaved-undo session showed an undo popping two stack items). The fast path now takes several changed nodes only where text edits are not recorded (where it fixes the lost delete); recorded, those lines are written as before this pull request (the record keeps a collaborator's delete there already). ## Test plan A new cell in `regressions/SetTextStretches.test.ts`: a join, then a bold, while a collaborator bolds the same words and then deletes them; one undo keeps the join, both id orders. ### Before On e8671d2 the cell fails for ids 2,1 (one undo undoes the join too). On 6827742 it passes, but the 4,000-seed interleaved-undo fuzz still differs from main on one seed (3154: an undo restores a word without its bold). ### After ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextStretches.test.ts Tests 41 passed | 2 expected fail (43) interleaved-undo fuzz, seeds 1-4,000: the same flags, seed by seed, as main (278 lines, no difference) $ pnpm run ci-check EXIT 0 $ vitest --run --project unit --project scripts-unit Test Files 669 passed | 4 skipped (673) Tests 31558 passed | 472 expected fail | 135 skipped (32165) ```
There was a problem hiding this comment.
Thanks, this looks good. Set text now keeps the unchanged words between two changes on both the recorded path and the writer's diff. The new SetTextStretches.test.ts cells fail on main and pass on this branch, with both peers and a fresh reader agreeing. Both suggestions below are optional and non-blocking.
Optional suggestions
packages/lexical-website/docs/collaboration/y.md, lines 839–842: the caveat names a state readers can't reach. The binding sets_recordTextEdits = truewhile it is attached (packages/lexical-y/src/internal/Binding.ts, line 1032), and that includes the headless editor. So "where the editor does not record text edits" never applies to a bound editor. When recording is forced off, the gap is also wider than a format or style change: setting the text and removing a sibling text node in the same update brings the collaborator's delete back too, while the recorded path keeps it deleted. Consider dropping the clause from the public page, since D29 and thetest.failscells already track it. Alternatively, describe the general condition: the line is written again where the record does not explain it.- Test plan output. The Before (
20 failed | 4 passed (24)) and After (24 passed (24)) blocks come from the first commit, butSetTextStretches.test.tsnow has 43 tests. AGENTS.md asks a pull request's Test plan to carry the real Before and After output, so re-pasting both runs of the final file would keep it accurate.
Comment on lines
+839
to
+842
| at the same time stays deleted; not in an IME composition, and not where | ||
| the editor does not record text edits and the same update also changes | ||
| the node's format or style, which writes the node again: | ||
| [#9666](https://github.com/meta/lexical/issues/9666)). Without a caret that places it (none, or one at |
There was a problem hiding this comment.
Optional: every @lexical/y binding, headless included, turns text-edit recording on while it is attached (Binding.ts, line 1032), so a reader can never be in the "editor does not record text edits" state this clause names. And when recording is forced off, a format or style change is not the only trigger: setting the text and removing a sibling text node in the same update also brings the collaborator's delete back. Consider dropping the clause here, keeping it in D29 and the test.fails cells, or wording it as the general condition (the line is written again).
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
Text set whole with
setTextContent(and the DOM's text, which the binding diffs the same way) kept only the start and end it shares with the old text (setTextEnds), so with two or more words changed everything between the first and the last change was written again: a word a collaborator deleted there at the same time came back, and showed twice after the collaborator's undo. #9621 had the same shape for text made again, which #9625 fixed with D29's word diff.Set text now keeps, between its common start and end, every run of unchanged words a shortest word-by-word edit script keeps, with the same function text made again uses (
withKeptWords, split out ofkeptStretches;setTextStretches=setTextEnds+ it), recorded (TextReplay#align) and diffed by the writer (Write's fallback now writes one edit per gap between kept runs). The start and end still come fromsetTextEnds, so a caret still places a single insertion or deletion (#9641, #9657), and an IME composition keeps the exact diff. The composition of an edit and a set within one update (TextReplay#compose) is unchanged. y.md ("Edits made by code") and D29 say so.Refs #9666 (the unrecorded set that also changes a node's format stays open)
Review follow-up (second commit)
Where text edits are not recorded, the writer's fast path now diffs every changed text node of a line (not only one; recorded lines of several changed nodes keep the line writer), each at its old offset, so a line of several text nodes set at once keeps a collaborator's delete too. A set that also changes the node's format or style in the same update, unrecorded, still writes the node whole: open in #9666, pinned as expected failures, listed in y.md and D29.
Review follow-up (third commit)
The writer's diff of several text nodes now writes them in one pass of one cursor (first node to last, each at its old offset moved by the edits before it), so its cost grows with the line, not with the line times the nodes (1,024 nodes: 7,166 cursor steps, 79 ms; it was about a million, a second). A counted cell pins it.
Review follow-up (fourth commit)
The fast path takes several changed text nodes only where text edits are not recorded; recorded, such lines go through the line writer as before this pull request. Sending them through the fast path changed what an undo restores (one undo undid a join and a bold). With it, the 4,000-seed interleaved-undo fuzz gives the same flags, seed by seed, as main; a cell pins the join-then-bold case.
Test plan
regressions/SetTextStretches.test.ts: the issue's line with two words changed while a collaborator deletest5z, then undoes (and three words changed with one removed; one word changed as control), recorded and diffed by the writer, both id orders; and a property over random lines set with 2 to 4 word edits (replaced, inserted, removed) while a collaborator deletes from, types into, or deletes and undoes in an unchanged word, unread and concurrent, recorded and diffed by the writer (60 seeds per cell; 300 also pass).Before
On main (21bcf62):
After
A randomized session generator (lines set with
setTextContentby an agent with concurrent comments, formats, typing, deletes and undo), 500 sessions, main vs this change: deletes brought back by a rewritten middle 25 vs 0, collaborator edits lost in a rewritten middle 27 vs 0, words twice 2 vs 0; the other classes (documented limits) equal.