Repository navigation
[lexical-y] Bug Fix: Keep the characters next to a paste in a line a transform makes again - #9585
Conversation
…transform makes again
## Description
A paste over part of a highlighted code line is recorded exactly (its splice), but the code highlighter then makes the line's token nodes again, with the old text and the pasted text in one node. Text made by a transform is aligned with the old text that left by the diff a write makes without the record, and that alignment keeps only what the diff that takes the common end first keeps too. That second reading did not see the pasted text, which arrived and left again in the same commit (the highlighter replaced the pasted nodes): when the old text before the range ended with the character the pasted text ends with (`t55z` before the range, `t87z` at the end of what was pasted), it aligned the two, and the write wrote that character again. A collaborator typing right after the word at the same time landed inside it, on every client: `t55 t83zzt84z` instead of `t55z t83zt84z`.
Now the reading that takes the common end first sees the text that arrived and left again where it was, between the old characters around it, so both readings keep the old text before the paste, and the rest of the line is written exactly, as the collaboration docs say.
## Changeset
`.changeset/lexical-y-paste-remade-line.md` (`@lexical/y`, `[fix]`).
## Test plan
`regressions/PasteRemadeLine.test.ts`: two clients, one pasting rich HTML over the end of a line while the other types next to it, over: a code line (the range in a comment), a line with a hashtag, and a plain line; which character of the pasted text coincides with the text around the range (last, first, none); a one-line paste and one with line breaks; typing before, inside and right after the range. The case captured from the randomized soak that found this also passes with the fix, but it needs the soak harness, so this matrix is the regression test.
```
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts
```
### Before
```
× code, last character coincides, with line breaks, typing after the range
× code, last character coincides, with line breaks, typing before the range
× code, last character coincides, with line breaks, typing inside the range
AssertionError: expected 't1z\n\nconst t44z = f(); // t54z t55 …' to contain 't54z t55z'
Tests 3 failed | 51 passed (54)
```
### After
```
Tests 54 passed (54)
```
`pnpm exec vitest run --project unit packages/lexical-y packages/lexical`:
```
Test Files 563 passed | 3 skipped (566)
Tests 14665 passed | 46 expected fail | 6 skipped (14717)
```
`pnpm run ci-check` (each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.
|
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.
Thanks for tracking this down. The fix works for the case it targets: on a highlighted code line, a paste that ends like the word before the range (t87z after t55z, and also t65z or t55z) no longer rewrites the end of that word. The three code, last character coincides, with line breaks rows fail on main and pass on this branch, and the rest of the @lexical/y unit suite still passes.
Must fix: the new reading can lose characters the old one kept (packages/lexical-y/src/internal/TextEdits.ts:1500)
align used to get the old text with the deleted-whole runs in place ({at, text: d}) as its second reading. It now gets {at: sureAt, text: was} instead of that reading, not as well as it. was (lines 1459-1473) splices in the passed runs but not the line breaks between them, because those line breaks stay live in the arrived text. When the first pasted line is a single word, the word before the range runs into the passed text, and snapToWords cuts the kept start back past that word.
To reproduce, set up two peers on const t44z = f(); // t54z t55zt78z t57zt80z in a JavaScript code block, as PasteRemadeLine.test.ts does:
- B pastes
<p><b>t84z</b></p><ul><li>t80z</li></ul>overt78z t57z. - At the same time, A types
t83zright aftert55z.
main gives // t54z t55z t83zt84z. This branch gives // t54z t55 t83zzt84z, the same symptom the PR fixes, in a neighbouring case.
Keeping both readings appears to fix it: pass [{at, text: d}, {at: sureAt, text: was}] to align and run the was !== w block once per reading, so a character is kept when either reading agrees with the main diff. With that change, the 54 new cases still pass, this case gives the same result as main, and the @lexical/y unit suite passes. A row with a one-word first pasted line would cover it.
Consider
- The test checks less than its comment says. The comment at
PasteRemadeLine.test.ts:207says nothing outside the pasted range is written again, but the assertions only check visible text. On the code rows where the first character coincides, the oldt8oft80zis still deleted and written again, onmainas well. Those rows pass because no typing position is insidet80z. An item-identity assertion likelineIdsinExactTextEdits.test.tswould make the matrix catch this and the regression above. - The mirror case is still open. If A types after the
toft80z, their text lands aftert55z, before the whole paste. A collapsed paste that starts like the word after it does the same. The collaboration docs say otherwise (packages/lexical-website/docs/collaboration/y.md: "wherever the line's text is unchanged … keep their characters" and "The rest of the line is written exactly"). It would help to pin this with an expected-fail row or a follow-up issue, and to narrow "the rest of the line is written exactly" in the description.
| cursor, | ||
| undefined, | ||
| {at, text: d}, | ||
| {at: sureAt, text: was}, |
There was a problem hiding this comment.
Must fix. This replaces the base's second reading ({at, text: d}) instead of adding to it. was has the passed runs spliced in but not the live line breaks between them. So when the first pasted line is a single word, the word before the range runs into the passed text and snapToWords drops the kept start of that word.
To reproduce: on the code line // t54z t55zt78z t57zt80z, paste <p><b>t84z</b></p><ul><li>t80z</li></ul> over t78z t57z while a peer types t83z right after t55z. main keeps t55z t83z; this branch writes t55 t83zz.
Passing both readings ([{at, text: d}, {at: sureAt, text: was}]) and running the was !== w block once per reading keeps every character either reading agrees on. With that change, the 54 new cases and the @lexical/y unit suite still pass.
There was a problem hiding this comment.
Fixed in 680a4a5 as suggested: align gets both readings ({at, text: d} and {at: sureAt, text: was}) and runs the end-first block once per reading, keeping a character when either agrees with the diff. Your case is now a test (code, a one-word first pasted line that ends like the word after the range): it failed on d848b4c and passes now, giving the same // t54z t55z t83zt84z as main; the matrix also has a one-word first pasted line. All 82 cases pass, and the @lexical/y + lexical unit suites pass.
| ); | ||
| expect(text(a)).toBe(stored); | ||
| expect(text(b)).toBe(stored); | ||
| // Nothing outside the pasted range is written again: the words |
There was a problem hiding this comment.
Consider. The assertions below only check visible text. So the code rows where the first character coincides pass, even though the old t8 of t80z is deleted and written again (on main too); none of the typing positions is inside t80z. An item-identity check around the range, like lineIds in ExactTextEdits.test.ts, plus a PASTES entry with a one-word first line would catch both that and the regression noted on TextEdits.ts:1500.
There was a problem hiding this comment.
Done in 680a4a5: every case now checks that the characters of the word before the range are the same Y items after the paste, not only the same text. The mirror case (the word after the range, when the paste starts like it) is pinned as a test.fails row with a comment saying it is open, and the description now says the fix covers the side before the range and names the mirror case as open (it is so on main too).
…d check that the word before a paste keeps its characters ## Description The first commit gave the alignment's end-first reading the text that left with the text that arrived and left again in place, instead of the reading it had (the text deleted whole in place). That new reading has the passed text but not the line breaks that stay between its parts, so where the first pasted line is a single word, the word before the range ran into it, and a collaborator typing after that word landed inside it again (`// t54z t55 t83zzt84z` for a paste of `<p><b>t84z</b></p><ul><li>t80z</li></ul>` over `t78z t57z` in `// t54z t55zt78z t57zt80z`, where main keeps `t55z t83z`). Now both readings are kept, and a character is kept when either agrees with the diff. The test now checks that the characters of the word before the range are the same items after the paste (not only the same text), adds a paste whose first line is one word, and the case above. The mirror case is open and pinned as an expected failure: where the pasted text starts like the word right after the range, that word's first character is written again, and what a collaborator types right after it lands before the paste. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts ``` ### Before On the first commit (d848b4c): ``` × code, a one-word first pasted line that ends like the word after the range AssertionError: expected -1 to be greater than or equal to 0 Tests 1 failed | 81 passed | 1 expected fail (83) ``` On main: ``` × code, last character coincides, one word, then a list, typing after the range × code, last character coincides, one word, then a list, typing before the range × code, last character coincides, one word, then a list, typing inside the range × code, last character coincides, with line breaks, typing after the range × code, last character coincides, with line breaks, typing before the range × code, last character coincides, with line breaks, typing inside the range Tests 6 failed | 76 passed | 1 expected fail (83) ``` ### After ``` Tests 82 passed | 1 expected fail (83) ``` `pnpm exec vitest run --project unit packages/lexical-y packages/lexical`: ``` Test Files 563 passed | 3 skipped (566) Tests 14693 passed | 47 expected fail | 6 skipped (14746) ``` `pnpm run tsc`, `tsc-test`, `flow`, `lint`, `prettier` and `check-changesets` pass.
## Description The expected failure for the mirror case (a collaborator typing inside the word right after a pasted range, when the pasted text starts like that word) now names its issue, #9587. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts ``` ### Before ``` Tests 82 passed | 1 expected fail (83) ``` ### After ``` Tests 82 passed | 1 expected fail (83) ```
There was a problem hiding this comment.
This looks good to me. The fix does what the description says for the side before the pasted range: in PasteRemadeLine.test.ts, the six code, last character coincides cases (with line breaks and one word, then a list, at every typing position) fail on main and pass on this branch; the one-word-first-line case raised earlier is now a test and keeps the word; and the rest of the @lexical/y unit suite still passes. Because align only gains another reading that can confirm what its main diff already keeps, this branch keeps every character main keeps. The changeset is in the shape CONTRIBUTING.md describes, Verify: paragraph included.
One optional, non-blocking suggestion:
- Order passed text by line order against text that has no old position (
packages/lexical-y/src/internal/TextEdits.ts, lines 1227, 1238 and 1462-1473). Text typed in the same commit whose node a transform then removed has no old position, soresolvegives itat: lastOld, while passed text getslastOld + 0.5; the merge that buildswasthen puts the passed text after the typed text even where the line had it first. As a result, a paste followed by typing in the same update still writes the end of the word before the range again, as onmain. For example, on the code lineconst x = 1; // t55 zzz t 5tz5, pasting<p><b>z55z</b></p><ul><li>ttz</li></ul>overt 5tz5and typingxafter it in one update, while a collaborator deletes thezright before the range, gives// t55 zzzz55zon every client, so the delete is lost. Recording each run's index in the line's items on both lists, and taking passed text first whenpassed.at - 0.5 === left.atand it came earlier, keeps the word there (// t55 zzz55z), and the@lexical/yunit suite,PasteRemadeLine.test.tsincluded, still passes with it. The collaboration docs (packages/lexical-website/docs/collaboration/y.md) say characters keep their identity wherever the line's text is unchanged, so this would bring one more case in line with them.mainhas the same gap, so a follow-up works just as well.
Thanks for pinning the mirror case with test.fails and tracking it in #9587; that reads well as a separate follow-up.
| } else if (x.node !== null || x.run.dead === true) { | ||
| stretches[s].live.push(x); | ||
| } else if (x.run.gone === true && x.run.kind === LOOSE) { | ||
| stretches[s].passed.push({at: lastOld + 0.5, text: textOf(x.run)}); |
There was a problem hiding this comment.
Optional, non-blocking. Passed text gets lastOld + 0.5 here, but a left run with no old position (such as text typed in this commit whose node a transform removed) gets at: lastOld (line 1238), so the merge at lines 1462-1473 reads the passed text after such a run even where the line had it first. A paste followed by typing in the same update then still writes the end of the word before the range again, as on main. Recording each run's index in the line's items on both lists, and taking passed text first when passed.at - 0.5 === left.at and it came earlier, keeps the word in those cases, and the @lexical/y unit suite still passes with it.
There was a problem hiding this comment.
Done in 65d0a3f: passed text and left runs now carry their index in the line's items, and passed text goes first when passed.at - 0.5 === left.at and it came earlier. Your example is a test (code, a paste and typing after it in one update, ...): it gave // t55 zzzz55z before and gives // t55 zzz55z now; the @lexical/y and lexical unit suites pass.
… commit by the line's order
## Description
In the end-first reading added by this PR, text that arrived and left again was placed after the old position before it, so it came after text typed in the same commit that has no old position (and is placed at that same old position), even where the line had the passed text first. A paste followed by typing in the same update, while a collaborator deleted the character right before the pasted range, then still wrote that character again, and the delete was lost (`// t55 zzzz55z` instead of `// t55 zzz55z`). The two are now ordered by where they were in the line's items.
## Test plan
```
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts
```
### Before
```
× code, a paste and typing after it in one update, while a collaborator deletes the character before the range 50ms
AssertionError: expected 't1z\n\nconst x = 1; // t55 zzzz55z\n\…' to contain '// t55 zzz55z'
Tests 1 failed | 82 passed | 1 expected fail (84)
```
### After
```
Tests 83 passed | 1 expected fail (84)
```
`pnpm exec vitest run --project unit packages/lexical-y packages/lexical`:
```
Test Files 563 passed | 3 skipped (566)
Tests 14694 passed | 47 expected fail | 6 skipped (14747)
```
`pnpm run tsc`, `tsc-test`, `flow`, `prettier`, `check-changesets` and `eslint --no-cache` on the changed files pass.
There was a problem hiding this comment.
This looks good to me. Reading the old text with the text that arrived and left again in the same commit put back in place fixes the paste case it targets. In PasteRemadeLine.test.ts, the six code, last character coincides cases (with line breaks and one word, then a list) and the paste-then-type case fail on main and pass on this branch, and the rest of the @lexical/y unit suite still passes. align only gains a reading that can confirm what its main diff already keeps, so nothing main keeps is lost. The same change also stops a collaborator's concurrent delete of that last character from being undone, and keeps the whole word when the paste ends with that word.
Two optional, non-blocking suggestions:
- Refresh the pasted test output (PR description, Test plan). AGENTS.md (Commits and Pull Requests) asks the Test plan to carry the real Before and After output of the command. The current
### Beforeis the first commit's 54-case run, and### Aftershows82 passed | 1 expected fail (83)from the second commit. At this head the file has 84 cases:mainfails 7 of them, and this branch gives83 passed | 1 expected fail. - Mention the undone delete in the changeset (
.changeset/lexical-y-paste-remade-line.md, line 5). The summary describes concurrent typing moving inside the word. The same rewrite also undid a collaborator's concurrent delete of that character:// t54z t55zt84z…onmain,// t54z t55t84z…on this branch. Something like "…which moved a collaborator's concurrent typing there inside the word, or undid their concurrent delete of that character" would cover both.
Thanks for pinning the mirror case as test.fails and tracking it in #9587; that works well as a separate follow-up.
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] A paste over part of a code line (or another line a transform makes again with the pasted text) no longer writes again a character next to the pasted range that equals the last character pasted, which moved a collaborator's concurrent typing there inside the word |
There was a problem hiding this comment.
Optional, non-blocking: the same rewrite also undid a collaborator's concurrent delete of that character (// t54z t55zt84z… on main, // t54z t55t84z… with this change). Something like "…which moved a collaborator's concurrent typing there inside the word, or undid their concurrent delete of that character" would tell changelog readers about both symptoms.
…oncurrent delete of the character was undone too ## Description The changeset said the rewritten character moved a collaborator's concurrent typing inside the word. The same rewrite also undid a collaborator's concurrent delete of that character; the summary now says so. ## Test plan ``` pnpm run check-changesets ``` ### Before A wording change; the check passed before it too. ### After ``` Changeset check passed: 32 pending changeset(s) valid. ```
|
Thanks. Both done: the description's Test plan now has the real output for this head (main fails 7 of the 84 cases, this branch passes 83 with the mirror case as the expected failure), and the changeset also mentions the undone concurrent delete (a4872ac, wording only). |
…in a highlighted line, and pin the stale-replace case main shares ## Description The previous commit also stopped the made text from keeping main's pieces of an old word between its common ends. That writes again the whole code token after a paste over part of a highlighted line, where the old token is glued to the pasted-over one (`t57zt80z`). #9585's paste cases keep that token, and they failed with it on top of #9585. Main's keeps are taken as before (identical to 1a232a6). Only the insertion or deletion inside a word stays replaced whole, which fixes soak L2 8345429. Soak L2 8138978 has the same shape on main: - the stale `t4z` → `t98z` deletes the `t` of `t4z`; - main and this change both keep that `t` as the first character of `t96z`, because a comment split `t4z` and main keeps the removed node `t` whole; - main shows `t98z96z`, whose `t` is the one t98z brought (checked by item ids in the soak's captures). It is now an expected-fail case that names this. ## Test plan ### Before ``` $ pnpm vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts # #9585 + 4b1dbcd Tests 11 failed | 148 passed | 1 expected fail (160) ``` ### After ``` $ pnpm vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/PasteRemadeLine.test.ts # #9585 + this head Tests 83 passed | 1 expected fail (84) $ pnpm vitest run --project unit packages/lexical-y/src/__tests__/unit/RemadeTextEnds.test.ts -t "stale replacement" Tests 2 passed | 2 expected fail | 72 skipped (76) $ pnpm vitest run --project unit packages/lexical-y Test Files 167 passed | 2 skipped (169) Tests 7785 passed | 48 expected fail | 4 skipped (7837) ```
Description
A paste over part of a highlighted code line is recorded exactly (its splice), but the code highlighter then makes the line's token nodes again, with the old text and the pasted text in one node. Text made by a transform is aligned with the old text that left by the diff a write makes without the record, and that alignment keeps only what the diff that takes the common end first keeps too. That second reading did not see the pasted text, which arrived and left again in the same commit (the highlighter replaced the pasted nodes): when the old text before the range ended with the character the pasted text ends with (
t55zbefore the range,t87zat the end of what was pasted), it aligned the two, and the write wrote that character again. A collaborator typing right after the word at the same time landed inside it, on every client:t55 t83zzt84zinstead oft55z t83zt84z.Now the reading that takes the common end first also reads the text that left with the text that arrived and left again where it was, between the old characters around it, alongside the reading it had (the text deleted whole in place), and a character is kept when either reading agrees with the diff. So the word before the paste keeps its characters, as the collaboration docs say ("the rest of the line is written exactly"), on the side before the range.
Text that arrived and left again is ordered against text typed in the same commit (which has no old position) by the line's order, so a paste followed by typing in one update keeps the word before the range too (fourth commit).
Still open, #9587 (pinned as an expected failure in the test): the mirror case. Where the pasted text starts like the word right after the range (
t84zbeforet80z), that word's first character is written again, and what a collaborator types right after it lands before the paste. This is so on main too.Changeset
.changeset/lexical-y-paste-remade-line.md(@lexical/y,[fix]).Test plan
regressions/PasteRemadeLine.test.ts: two clients, one pasting rich HTML over the end of a line while the other types next to it, over: a code line (the range in a comment), a line with a hashtag, and a plain line; which character of the pasted text coincides with the text around the range (last, first, none); a one-line paste and one with line breaks; typing before, inside and right after the range. The case captured from the randomized soak that found this also passes with the fix, but it needs the soak harness, so this matrix is the regression test.Before
On main, with this branch's test file (84 cases):
After
(The expected failure is the mirror case, #9587.)
pnpm exec vitest run --project unit packages/lexical-y packages/lexical:pnpm run ci-check(each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.