Repository navigation
[lexical-website][lexical-y] Bug Fix: Place an edit of set text at a word edge when the caret is past it - #9657
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Thanks, this looks good. A caret at the end of the node or in a later node no longer pins the diff of text set with setTextContent, so setTextContent('t1z t3z'); selectEnd() deletes t2z and a collaborator's concurrent delete of t2z merges to t1z t3z on every client. The new cells fail against main's Inline.ts and pass here, and the narrowed glue allowance makes the word property stricter. A few optional, non-blocking suggestions:
- Docs and the decision row.
packages/lexical-website/docs/collaboration/y.md(lines 829-843) and D30 inpackages/lexical-y/DECISIONS.mdstill describe the word-edge placement, and the glue it accepts, for a set "without a caret". After this change it also applies with the caret at the end of the node, past the edit or in a later node.packages/lexical-y/AGENTS.md(Decisions) asks a pull request that changes a recorded decision to update its row and y.md in the same pull request; for example: "Without a caret, or with one at the end of the node, past the edit or in a later node, …". - Pin the case that changes the other way. With the caret at the end, a collaborator typing
q9zright beforet2zused to merge tot1z q9z t3zand now merges tot1zq9z t3z, D30's documented glue. The new describe covers deletingt2zand typing after it; atypes "q9z " before t2zrow expectingt1zq9z t3z(SetTextWordEdges.test.ts:229-232, with the splice carried in the row) would make that trade explicit. - Pin the branch that keeps a caret's placement. No
@lexical/yunit test fails ifreturn composing || caretPlaces(a, b, cursor)(Inline.ts:809) becomesreturn composing, or if<=incaretPlacesbecomes<: the cells cited for it (a space typed after two spaces,1..set to1.) land at word edges either way. Two unit cells tell them apart:setTextEnds('t1z t2z t3z', 't1z t3z', 5, false)is{prefix: 5, suffix: 2}({prefix: 3, suffix: 4}without the branch), andsetTextEnds('ab ab', 'ab abz ab', 9, false), text typed at the end with the caret after it, is{prefix: 5, suffix: 0}({prefix: 2, suffix: 3}without it).packages/lexical-y/AGENTS.md(Testing) asks every bug fix to land with a regression test. - Nit. The
caretPlacesJSDoc (Inline.ts:774-776) and the describe comment (SetTextWordEdges.test.ts:214-218) say a caret at the end of the node places nothing, but with the furthest place included it still places an edit at the very end:setTextEnds('abc abc ab', 'abc ab', 6, false)deletesc abrather thanabc. "A caret past the furthest place of the run places nothing" would match the code.
| const how = record ? 'recorded' : 'diffed by the writer'; | ||
| for (const [peer, want] of [ | ||
| ['deletes t2z', 't1z t3z'], | ||
| ['types " q9z" after t2z', 't1z q9z t3z'], |
There was a problem hiding this comment.
Optional: with the caret at the end, a collaborator typing q9z right before t2z now merges to t1zq9z t3z (it was t1z q9z t3z on main), D30's documented glue. A row for it here, with the splice carried in the row ($splice(4, 0, 'q9z ')), would pin that trade alongside the two cases this describe covers.
There was a problem hiding this comment.
Added in 9659452: the caret-at-end describe now carries the splice in each row and has types "q9z " before t2z expecting t1zq9z t3z (the glue y.md documents), in both id orders, recorded and through the writer's diff, with the caret at the end and in the next node.
| a.length !== b.length | ||
| ) { | ||
| return composing || caretAnchors(a, b, cursor) | ||
| return composing || caretPlaces(a, b, cursor) |
There was a problem hiding this comment.
Optional: no @lexical/y unit test fails if this becomes return composing (or if <= in caretPlaces becomes <). setTextEnds('t1z t2z t3z', 't1z t3z', 5, false) → {prefix: 5, suffix: 2} and setTextEnds('ab ab', 'ab abz ab', 9, false) → {prefix: 5, suffix: 0} would pin the branch that keeps a caret's placement ({prefix: 3, suffix: 4} and {prefix: 2, suffix: 3} without it).
There was a problem hiding this comment.
Added in 9659452: setTextEnds("t1z t2z t3z", "t1z t3z", 5) gives {prefix: 5, suffix: 2} and setTextEnds("ab ab", "ab abz ab", 9) gives {prefix: 5, suffix: 0}, so dropping the caret branch or turning <= into < fails.
| * A caret at the end of the node or past the edit (and the end of the node, | ||
| * which the writer passes for a caret in a later node) anchors every place | ||
| * of the run and places nothing. |
There was a problem hiding this comment.
Nit: a caret at the end of the node still places an edit the diff can put at the very end (the furthest place is included): setTextEnds('abc abc ab', 'abc ab', 6, false) deletes c ab, where no caret deletes abc . Maybe "a caret past the furthest place of the run places nothing".
There was a problem hiding this comment.
Reworded in 9659452: "A caret past the furthest place (at the end of the node or past the edit, and the end of the node, which the writer passes for a caret in a later node) anchors every place of the run and places nothing"; one at the furthest place still places the edit, as in your abc abc ab example. y.md and D30 now say the same (item 1 of the review).
…e caret is past it ## Description Follow-up to #9641 (#9636), from its review. `setTextEnds` kept the diff's placement whenever the caret anchored it, and a caret anchors the diff whenever the text after it is unchanged: always true for a caret at the end of the node, anywhere past the edit, or in a later node (for which the writer anchors the diff at the end of the node). So the usual `node.setTextContent('t1z t3z'); node.selectEnd();` was placed as on main: `2z t` deleted, and a collaborator's concurrent delete of `t2z` left `t1z 3z`. A caret now places the edit only where its bound lies in the run of equal text the edit could be in, the furthest place included (`caretPlaces`): a caret right after typed text still places it (a space typed after two spaces stays after them), and a caret past the edit places nothing. The property's allowance for glue also narrows to the side y.md documents (a word typed before the deleted word, glued to the word before it; typed after a line's first word, glued to the word after it), so it guards D30's choice. ## Test plan New cells in `SetTextWordEdges.test.ts`: `setTextEnds` with the caret at the end; a set with the caret at the end of the node or in the next node while a collaborator deletes `t2z` or types ` q9z` after it, recorded and diffed by the writer, both id orders; `1..` set to `1.` with the caret at 2 while a collaborator types between the dots. ### Before On #9641's head (862ecca): ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextWordEdges.test.ts -t 'caret past' × places nothing: `t1z t2z t3z` set to `t1z t3z` with the caret at the end × set with the caret at the end ‖ the peer deletes t2z, recorded (ids 1,2) × set with the caret at the end ‖ the peer types " q9z" after t2z, recorded (ids 1,2) × set with the caret at the end ‖ the peer deletes t2z, diffed by the writer (ids 1,2) × set with the caret in the next node ‖ the peer deletes t2z, diffed by the writer (ids 1,2) × set with the caret at the end ‖ the peer types " q9z" after t2z, diffed by the writer (ids 1,2) × set with the caret in the next node ‖ the peer types " q9z" after t2z, diffed by the writer (ids 1,2) ... (the same 6 for ids 2,1) Tests 13 failed | 6 passed | 44 skipped (63) ``` ### After ``` $ LEXICAL_Y_SEEDS=600 vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextWordEdges.test.ts Tests 63 passed (63) $ pnpm run ci-check EXIT 0 $ vitest --run --project unit --project scripts-unit Test Files 644 passed | 4 skipped (648) Tests 27748 passed | 204 expected fail | 135 skipped (28087) ```
…ast the edit places nothing, in y.md and D30, and pin both sides of the trade
Review follow-up on this pull request. y.md ("Edits made by code") and D30 said the word-edge placement applies to a set "without a caret"; it now also applies with a caret that places nothing (at the end of the node, past the edit, or in a later node), and both say so. `caretPlaces`' comment says a caret past the furthest place of the run places nothing (one at the furthest place still places the edit).
New cells: with the caret at the end, a collaborator typing `q9z ` right before `t2z` now gets `t1zq9z t3z` (y.md's documented glue; main gave `t1z q9z t3z` while the caret pinned the diff); and `setTextEnds` with a caret in the run keeps the caret's placement (`t1z t2z t3z` set to `t1z t3z` with the caret at 5 deletes `2z t`; `ab ab` set to `ab abz ab` with the caret at 9 inserts at the end), so a change that dropped the caret branch fails.
```
$ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/SetTextWordEdges.test.ts
Tests 73 passed (73)
```
d1c01e3 to
9659452
Compare
zurfyx
changed the title
There was a problem hiding this comment.
Thanks, this looks good. A caret past the edit (at the end of the node or in a later node, after an edit away from the end) no longer pins the diff of text set with setTextContent, so setTextContent('t1z t3z'); selectEnd() deletes t2z, and a collaborator's concurrent delete of t2z now merges to t1z t3z on every client. The follow-ups from the last round are in: the two a caret in the run places it unit cells pin the caret branch of caretPlaces, the types "q9z " before t2z cells pin the documented glue, and y.md and D30 state the new rule.
One optional, non-blocking suggestion:
- Which carets place nothing (
packages/lexical-website/docs/collaboration/y.mdlines 837-838, the D30 row inpackages/lexical-y/DECISIONS.md, and the describe comment atpackages/lexical-y/src/__tests__/unit/regressions/SetTextWordEdges.test.ts:214-218). All three list "one at the end of the node" among carets that place nothing, butcaretPlacesstill places an edit that could end at the caret:setTextEnds('ab ab', 'ab abz ab', 9, false)writesz abat the end (your own cell at line 229) where no caret writesabzat 2, and'abc abc ab'set to'abc ab'with the caret at 6 deletesc abwhere no caret deletesabc. ThecaretPlacesJSDoc already says it precisely ("a caret past the furthest place"); wording such as "none, or one past the furthest place the edit could be (at the end of the node or in a later node, unless the edit could end there)" would bring the other three in line.packages/lexical-y/AGENTS.md(Decisions) has a pull request that changes a recorded decision update its row, so the row is where readers look for the exact rule.
| was made without one. Without a caret that places it (none, or one at | ||
| the end of the node, past the edit or in a later node), an insertion or |
There was a problem hiding this comment.
Optional nit: a caret at the end of the node still places an edit that could end there. setTextEnds('ab ab', 'ab abz ab', 9, false) keeps z ab at the end (the cell at SetTextWordEdges.test.ts:229), where no caret writes abz at 2. Something like "(none, or one past the furthest place the edit could be: at the end of the node or in a later node, unless the edit could end there)" would match caretPlaces; the D30 row and the describe comment at SetTextWordEdges.test.ts:214-218 use the same list.
## 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).
Description
Follow-up to #9641 (#9636), from its review.
setTextEndskept the diff's placement whenever the caret anchored it, and a caret anchors the diff whenever the text after it is unchanged: always true for a caret at the end of the node, anywhere past the edit, or in a later node (for which the writer anchors the diff at the end of the node). So the usualnode.setTextContent('t1z t3z'); node.selectEnd();was placed as on main:2z tdeleted, and a collaborator's concurrent delete oft2zleftt1z 3z.A caret now places the edit only where its bound lies in the run of equal text the edit could be in, the furthest place included (
caretPlaces): a caret right after typed text still places it (a space typed after two spaces stays after them), and a caret past the edit places nothing.The property's allowance for glue also narrows to the side y.md documents (a word typed before the deleted word, glued to the word before it; typed after a line's first word, glued to the word after it), so it guards D30's choice.
Review follow-up (second commit)
y.md ("Edits made by code") and D30 now say the word-edge placement applies to a set with no caret that places it (none, or one at the end of the node, past the edit or in a later node);
caretPlaces' comment says a caret past the furthest place places nothing. New cells pin both sides of the trade: with the caret at the end,q9ztyped right beforet2znow getst1zq9z t3z(the documented glue; it wast1z q9z t3zwhile the caret pinned the diff), and a caret in the run still places the edit (setTextEndswith the caret at 5 and at 9). Rebased on main (c363dd9).Test plan
New cells in
SetTextWordEdges.test.ts:setTextEndswith the caret at the end; a set with the caret at the end of the node or in the next node while a collaborator deletest2zor typesq9zafter it, recorded and diffed by the writer, both id orders;1..set to1.with the caret at 2 while a collaborator types between the dots.Before
On #9641's head (862ecca):
After