Repository navigation
Conversation
…ed no longer loses a collaborator's held-back text ## Description While the editor composes (IME) in a line, a collaborator's insert into that line is applied to Yjs but held back from the editor (`Hidden`); a change applied during a local update is likewise not read until after it (`Unread`, #9784). A commit that moved the list item holding that content (down its list, to another list, nested), made locally or by an app's code in the update of a remote change's read (#9788), wrote the move as a delete and a write of the editor's view, which lacks it: - held or unread content inside a shared node that moved with the line (an inline container, a block node holding the line) was deleted with it, on every client; - held or unread text in a moved line stayed at the old place and, once read, showed glued to the next line (`Rtail`). Now: - A container write keeps the items of the units (lines, block nodes) and inline containers that hold content the editor does not show where they match one in the container (`Writer#$heldKeys`): the moved unit keeps its items, and the units it passed are written again instead. Exact: nothing hidden is copied, so held formats, held inline nodes and held line markers stay as stored, with no duplicate or resurrected delete. - Units written at a unit boundary next to a kept unit go before a held or unread format that opens a run over it (`Cursor#heldBoundary`), not inside that run. - A commit that moves held content to another container (a line into or out of a block node, a block node into another, an inline container to another line) cannot keep its items: it is not written, is read again, and reports `UNSUPPORTED_EDIT_WARNING` with a new `data.reason`, `'composing-move'`, once per refused commit (`YBinding#refusesHeldMove`, on the local commit path and in `$writeReadEdits`). The move is undone locally and nothing held is lost. Unchanged and out of scope: what a change not read yet inserted into a unit moved to another container stays under the concurrent move limits (D48, D49); text composed in a node moved into a new line elsewhere, its old line removed, leaves the held content at the old place, where it shows once the hold ends, as after Enter. Recorded as D112 in DECISIONS.md; y.md's Moves limit, Input method composition note and warnings table are updated, and so are `YUnsupportedEditData` (TypeScript and Flow) and the `UNSUPPORTED_EDIT_WARNING` JSDoc. ## Changeset `.changeset/lexical-y-composed-move-keeps-held-text.md` (`[fix]`, with a `Verify:` paragraph naming the new warning reason). ## Test plan New regression file `packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts` (104 cases, three peers, checked against a fresh reader, gc on and off, two client id orders): held text, held bolded text and a held inline node in a list item moved down its list, to another list and nested; a composed inline container in a moved line; a block node holding the line moved in its container; four moves to another container that are refused, with exactly one `'composing-move'` warning; the #9784 unread case in a line and in an inline container; and an app's selection transform that moves the composed list item in the update of a remote change's read (the three moves, and one into a block node that is refused with the warning). ### Before `pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts` on `main` (dfe058a): ``` FAIL |unit| .../ComposedMovePendingText.test.ts > held back in an inline container, the line holding it moved > moved to another list, text (gc true, ids 1,2,3) Expected: "list[bullet](listitem("tail")) / paragraph("sep") / list[number](listitem("other") listitem(test_composed_span("aRbc")))" Received: "list[bullet](listitem("tail")) / paragraph("sep") / list[number](listitem("other") listitem(test_composed_span("abc")))" FAIL |unit| .../ComposedMovePendingText.test.ts > held back while composing, the list item holding it moved > moved down its list, text (gc false, ids 1,2,3) AssertionError: expected 'list[bullet](listitem("Rtail") listit…' to be 'list[bullet](listitem("tail") listite…' FAIL |unit| .../ComposedMovePendingText.test.ts > held back while composing, moved by an app in a remote read > into a block node (gc true, ids 1,2,3) Expected: "aRbc" Received: "list[bullet](listitem("R"{1} "tail")) / paragraph("sep!") / list[number](listitem("other")) / test_box(paragraph("in box") list[number](listitem("abc")))" FAIL |unit| .../ComposedMovePendingText.test.ts > inserted by a change not read yet, the list item holding it moved > in an inline container (gc true, ids 1,2,3) Expected: "list[bullet](listitem("tail")) / paragraph("sep") / list[number](listitem("other") listitem(test_composed_span("aRbc"{1})))" Received: "list[bullet](listitem("tail")) / paragraph("sep") / list[number](listitem("other") listitem(test_composed_span("abc")))" Test Files 1 failed (1) Tests 104 failed (104) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts Test Files 1 passed (1) Tests 104 passed (104) $ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts Test Files 1 passed (1) Tests 104 passed (104) $ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/ Test Files 382 passed | 2 skipped (384) Tests 22595 passed | 379 expected fail | 131 skipped (23105) $ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y Test Files 270 passed | 2 skipped (272) Tests 19947 passed | 382 expected fail | 131 skipped (20460) $ pnpm exec tsc -p tsconfig.test.json --noEmit (no output) ``` ESLint and Prettier are clean on the changed files; `pnpm run check-changesets` passes. Browser and E2E suites were not run.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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: |
There was a problem hiding this comment.
Thanks for the careful fix and the 104-case matrix. Keeping the held unit for moves within a container and refusing cross-container moves reads well, and the new cases fail on main and pass on this branch.
One issue to fix before merge
- Composing inside a named slot now loses keystrokes (
packages/lexical-y/src/internal/Binding.ts, theinsidehelper in$movesHeld, around line 4310).node.getParents()stops at a slot boundary: a slot value has no__parent, and onlyisAttached()follows__slotHost. So for a line inside a slot (for example the playground's pull quotequoteslot, or a component's shadow-root body),inside(line, host)returns false and the commit is refused as a'composing-move'although nothing moved. To reproduce: A composes inabcinside a slot, B insertsR(held back), A typesx. Onmainevery client ends withaxRbc; on this branchxis dropped from A's editor and the document (aRbc), and a'composing-move'warning fires. A plain paragraph and a table cell behave correctly. Walkingn.getParent() ?? $getSlotHost(n)ininsidefixes it, and the 104 new cases still pass with that change. Writer#$heldKeys'add(packages/lexical-y/src/internal/Write.ts, around line 2231) walksgetParent()the same way, so a slot host is never treated as held. The same walk there would let a host moved within its container keep the held content in its slot.
Suggestions (non-blocking)
packages/lexical-y/AGENTS.md(Testing) asks@lexical/ytests to draw their inputs fromsrc/__tests__/catalog/, which includes named-slot hosts. A no-move case (a held collaborator change plus one composed keystroke, asserting the keystroke is stored on every peer) over the catalog's container shapes would have caught the issue above.- A refused move re-reads the composed line while the browser's composition is still open. One collab-Y E2E check (
pnpm run test-e2e-collab-y-chromium), or a recorded manual check, would show what the input method does there.
| return ( | ||
| node === null || | ||
| !node.isAttached() || | ||
| node.getParents().some(p => p.getKey() === above) |
There was a problem hiding this comment.
getParents() stops at a slot boundary (a slot value has no __parent; only isAttached() follows __slotHost), so for a line inside a named slot this returns false against the host's unit and the commit is refused as a 'composing-move' although nothing moved. Composing in a slot line while a collaborator's change is held then drops each keystroke (axRbc on main, aRbc here). Walking n.getParent() ?? $getSlotHost(n) fixes it, and the 104 new cases still pass.
| for ( | ||
| let n = key === null ? null : $getNodeByKey(key); | ||
| n !== null && !keys.has(n.getKey()); | ||
| n = n.getParent() |
There was a problem hiding this comment.
Same slot boundary here: getParent() stops at the slot value, so a slot host is never added to the held keys and a move of it within its container still rewrites the held content in its slot. The getParent() ?? $getSlotHost(n) walk would cover it.
…olding pending text and a replacement of a shared node holding held text; find a slot's host through the slot
## Description
Review of the previous commit:
- Two lines in one container that each hold content the editor does not show (held back by a composition, or inserted by a change not read yet), reordered by a commit: a write keeps only one of them, and the other's pending text was lost or misplaced. Such a commit is now refused (`YBinding#$reordersHeld`, in `refusesHeldMove`): not written, read again, `UNSUPPORTED_EDIT_WARNING` with `'composing-move'`. One such line alone is kept, as before.
- A node stored as a shared node of its own (an inline container, a block node, a slot's value) holding held content, removed by a commit while the text being composed stays (its children moved into a new node, a slot's value replaced): the held content was deleted with it. Such a commit is now refused the same way. A line's block type change (a paragraph made a heading) is not: a line's content is its container's, and stays.
- Composing in a line in a named slot: the check for a move to another container walked `getParents()`, which stops at a slot, so any commit (a keystroke in the composed text, an edit elsewhere) was refused and dropped. It now walks a node's parent or the host of the slot it fills; so does `Writer#$heldKeys`, so a slot's host moved in its container keeps the held content in its slot.
D112, y.md, the warning's doc comments and the changeset say so.
## Test plan
### Before
On the previous commit, the new cells fail, gc off and on: `inserted by a change not read yet in two lines, reordered` (2 lines: pending text lost or misplaced), `held back, the node holding the composed text replaced > an inline container` (R deleted), and `held back in a slot, an edit that moves nothing out` (all three: the edit refused and dropped).
### After
```
✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts (126 tests)
```
`vitest run --project unit packages/lexical-y packages/lexical/` (gc off) and `packages/lexical-y` (gc on):
```
Test Files 382 passed | 2 skipped (384)
Tests 22617 passed | 379 expected fail | 131 skipped (23127)
Test Files 270 passed | 2 skipped (272)
Tests 19969 passed | 382 expected fail | 131 skipped (20482)
```
|
Thanks. In e891a0d: |
…two hosts are reordered
## Description
Review of the previous commit: the check for a reorder of two units holding content the editor does not show looked through a shared node's children only; a slot's value is stored in its host's attributes, so two hosts whose slots each held a change not read yet counted as holding nothing, the reorder was written, and one slot's pending text was deleted on every client. The check now also looks through a shared node's attributes.
## Test plan
### Before
On the previous commit, the new cell `inserted by a change not read yet in two slots, their hosts reordered` fails, gc off and on.
### After
```
✓ |unit| packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts (128 tests)
```
`vitest run --project unit packages/lexical-y packages/lexical/` (gc off) and `packages/lexical-y` (gc on); a scratch session-lane test file was in the tree during this run, and accounts for one more file:
```
Test Files 383 passed | 2 skipped (385)
Tests 22620 passed | 379 expected fail | 131 skipped (23130)
Test Files 271 passed | 2 skipped (273)
Tests 19972 passed | 382 expected fail | 131 skipped (20485)
```
There was a problem hiding this comment.
Following __slotHost in inside and in Writer#$heldKeys fixes the named-slot cases, and the new reorder and replacement refusals come with cells that fail on main. One shape still trips the refusal.
One issue to fix before merge
- Composing in a block element that holds its text directly loses keystrokes (
packages/lexical-y/src/internal/Binding.ts,insidein$movesHeld, lines 4400-4417, checked at line 4468). A block element stored as a shared node of its own with inline children (the playground'sCollapsibleTitleNode, or an app'sElementNodeblock that is not a line class) is both the inline container one level down and the unit holding it one level up, under the same key.inside(key, key)starts at the node's parent and never compares the node itself, so it returnsfalseand every commit is refused as'composing-move'although nothing moved. To reproduce: A composes in a collapsible titleDetails, B inserts¶(held back), A types§. Onmainevery client ends withD§¶etails; on this branch§is refused, taken out of A's editor and never stored (D¶etailson every client and a fresh reader), and the warning fires. Moving the collapsible within the root is refused the same way. Returningtruewhenkey === abovefixes both (the move then keeps¶), the 128 cases inComposedMovePendingText.test.tsstill pass with it, and a title moved out of its collapsible is still refused bymoves(u).
Suggestions (non-blocking)
packages/lexical-y/AGENTS.md(Testing) asks@lexical/ytests to draw their inputs fromsrc/__tests__/catalog/. A cell built from the catalog'scollapsible/openshape would pin the fix above.$reordersHeld(Binding.ts, line 4351) counts the items of a whole paragraph that a change not read yet inserted toward the next existing line. WithY.applyUpdateinsideeditor.updateinserting a paragraph beforebband typing intocc, movingbbto the end in that update is refused with'composing-move', whilemainstores the move with nothing lost. No text is lost, but an app sees a valid move refused, with a reason that names a composition.- While anything is held,
$reordersHeldwalks every block container on each commit: a composed keystroke in a 4000-paragraph document goes from about 0.95 ms to 1.5 ms. Skipping the walk when only a composition hold is active (it normally covers one line) would avoid it. - The Test plan's Before/After output is for the first commit's 104 cases; the file now has 128. AGENTS.md ("Commits and Pull Requests") asks for the real output of the command at the current head.
- y.md's Moves limit (
packages/lexical-website/docs/collaboration/y.md, line 1092 on) says a block holding a change the editor does not show yet "is never the one rewritten"; a reorder of two such blocks is refused instead, which the Input method composition note and the warnings table say but the Moves limit does not. - A refused move is read back while the input method is still composing. One collab-Y E2E (
pnpm run test-e2e-collab-y-chromium) or a recorded manual check would show what the browser does there.
| ? state.containers.get(this._root) | ||
| : state.csByKey.get(key); | ||
| // Whether `key`, where the editor still has it, is below `above`. | ||
| const inside = (key: NodeKey, above: NodeKey): boolean => { |
There was a problem hiding this comment.
inside(key, key) is false: the walk starts at the node's parent and never compares the node itself. A block element stored as a shared node of its own with inline children (the playground's CollapsibleTitleNode, or an app's ElementNode block that is not a line class) is the inline container one level down and the unit one level up under the same key, so the check at line 4468 refuses every commit while a collaborator's change to it is held: each composed keystroke is dropped (main stores it), and a move of the block within its container is refused. With this change the 128 cases in ComposedMovePendingText.test.ts still pass, and a block moved to another container is still refused by moves(u).
| const inside = (key: NodeKey, above: NodeKey): boolean => { | |
| const inside = (key: NodeKey, above: NodeKey): boolean => { | |
| if (key === above) { | |
| return true; | |
| } |
| if (u === undefined) { | ||
| break; | ||
| } | ||
| if (held[held.length - 1] !== u && holds(it)) { |
There was a problem hiding this comment.
Non-blocking: items of a whole paragraph that the unread change inserted (its end is not in cs.units) are counted toward the next existing unit here. A change applied inside the update that inserts a paragraph before bb and types into cc makes a move of bb to the end look like a reorder of two held lines, so it is refused with 'composing-move', although main stores that move with nothing lost. Counting only items inside units the binding already has would avoid it.
…k node holding its text, nor a move past a paragraph not read yet
## Description
Review of the previous commit, two false refusals (no text lost, but a valid edit was dropped with a `'composing-move'` warning):
- Composing in a block node that holds its text itself (a collapsible's title): the same node is the inline container one level down and the unit holding it one level up, and the check for a move to another container did not count a node as inside itself, so every commit (a keystroke, a move of the collapsible in its container) was refused. It now does.
- A paragraph a change not read yet inserted, applied in the update that moves another line: its content was counted toward the next line the document had, so the move looked like a reorder of two lines holding pending content and was refused. The end of a unit the editor does not show yet now closes what came before it.
While only a composition holds content back, the reorder check now looks only at the containers it holds content in. y.md's Moves limit says a reorder of two blocks that each hold such a change is not stored.
## Test plan
### Before
On the previous commit, the new cells `held back in a collapsible title, an edit that moves nothing out` (both) and `a paragraph inserted by a change not read yet, another line moved` fail, gc off and on.
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 134 passed (134)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 134 passed (134)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22625 passed | 379 expected fail | 131 skipped (23135)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19977 passed | 382 expected fail | 131 skipped (20490)
```
|
Thanks. In 4ad4f2e: |
…ger makes the next line look like it holds pending content
## Description
Review of the previous commit: a change not read yet that types into a line the document had (in an inline container) and inserts a paragraph after it, applied in the update that moves the next line before the first: the writer counted the new paragraph's content toward the next line the document had, kept both, and rewrote the first line's inline container from the editor, deleting the change's text in it on every client. When it looks for the unit holding such content, the writer (`Writer#$heldKeys`), like the reorder check, now stops at the end of a unit that change inserted. The reorder check stops only at those, not at a line marker a composition holds back, whose content shows in the line composed in.
## Test plan
### Before
On the previous commit, `a paragraph and text inserted by a change not read yet, a line moved past them > in an inline container` fails, gc off and on (`main` passes it):
```
× in an inline container (gc false)
× in an inline container (gc true)
Tests 2 failed | 2 passed | 134 skipped (138)
```
### After
```
Tests 4 passed | 134 skipped (138)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22629 passed | 379 expected fail | 131 skipped (23139)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19981 passed | 382 expected fail | 131 skipped (20494)
```
There was a problem hiding this comment.
This looks good to me. Following inside for a block that holds its own text (the collapsible title) and closing a paragraph that a not-yet-read change inserted both come with regression cells, and the Test plan now shows the output at the head. Everything below is optional and non-blocking.
Optional suggestions
- A line that the not-yet-read change splits is rewritten again (
packages/lexical-y/src/internal/Write.ts,isNewEnd, lines 2264-2268, and the matching reset in$reordersHeld,packages/lexical-y/src/internal/Binding.ts, lines 4374-4386). A split of an existing line also inserts a new end item, after that line's own text, so the text before it is no longer attributed to the line. Repro: B typesXintoaabband splits it afteraXa; A applies that update inside aneditor.updatethat moves the line to the end. With the commit before this one every client ends withmid / tail / aXa / bb; at this head, as onmain, it ends withX / mid / tail / aabb, while y.md's Moves limit says a block holding a change the editor does not show yet "is never the one rewritten". Closing the run at an inserted end only when nothing between the previous known end and it is content the document already had (a wholly inserted paragraph) keeps both behaviours: with that condition the 138 cases still pass and the split keepsaXa / bb.packages/lexical-y/AGENTS.md(Testing) asks for a regression cell per fix, so one for this shape would pin it. - y.md's Moves limit and one refusal disagree (
packages/lexical-y/src/internal/Binding.ts, lines 4401-4411).$reordersHeldcompares the held units' order withisBefore, so moving one of two lines that both hold a not-yet-read change into a box counts as a reorder and is refused with'composing-move'; nothing is lost.packages/lexical-website/docs/collaboration/y.md(lines 1100-1103) says such a move is stored with the change lost or at the old position. One clause in y.md, or skipping units whose container changed, would align them. - Catalog-drawn cells.
packages/lexical-y/AGENTS.md(Testing) asks@lexical/ytests to draw their inputs fromsrc/__tests__/catalog/. Over the catalog's shapes (a composed keystroke, and a move of each shape's block in the root) nothing is refused and every held insert is kept at this head, so a few catalog-driven cells would cheaply pin that. - Cost while a composition holds a change.
$reordersHeld(Binding.ts, line 4305) runs on every local commit during a hold; for a line in the root its container list includes the root, andholdsrecurses into every block node. A composed keystroke in a 4000-paragraph document goes from about 0.8 ms to 1.2 ms while a change is held, and is unchanged otherwise. Deriving the held units from the held items upward, as$movesHeldandWriter#$heldKeysalready do, would make it proportional to what is held. - Real-browser check of a refused move. A refused move is read back while the input method is still composing. One collab-Y E2E run (
pnpm run test-e2e-collab-y-chromium, listed in AGENTS.md) or a recorded manual check would show what the browser does there.
| // paragraph) closes the content before it: that unit is none the | ||
| // document had, and holds no key to keep. | ||
| const unseen = viewOf(parent); | ||
| const isNewEnd = (end: Item) => |
There was a problem hiding this comment.
Optional: a split of an existing line also inserts an end item the not-yet-read change owns, after the line's own text, so this stops the walk before the line's original end and the line is no longer kept. B types X in aabb and splits after aXa, applied inside the update that moves the line: every client ends with X / mid / tail / aabb, as on main, where the commit before this one kept aXa / bb. Closing here only when nothing between the previous known end and this one is content the document already had would keep both cases (the same condition fits the reset in $reordersHeld).
| if (node === null || !node.isAttached()) { | ||
| continue; | ||
| } | ||
| if (prev !== null && !prev.isBefore(node)) { |
There was a problem hiding this comment.
Optional: isBefore compares document order, so a held unit moved into another container after the other held unit counts as a reorder and the update is refused with 'composing-move'. y.md's Moves limit says such a cross-container move is stored (the change lost or at the old position). Either document this case or skip units whose container changed.
…lits still keeps that change when it moves
## Description
Review of the previous commit: a change not read yet that types into a line and splits it inserts a unit end after the line's own text; the previous commit took every such end as the end of a new paragraph, so the line's text before it no longer counted as the line's, and a move of the line rewrote it from the editor (`X` landed on its own, `aabb` was written again), as on `main`. An inserted end now closes the content before it only when nothing between it and the end before it is content the editor shows (`endsNewUnit`): a wholly inserted paragraph, not a split. y.md's Moves limit says the reorder refusal also covers one of the two blocks moving to another container.
## Test plan
### Before
On the previous commit, `a line a change not read yet types into and splits, moved` fails, gc off and on:
```
× gc false
× gc true
Tests 2 failed | 138 passed (140)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 140 passed (140)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 140 passed (140)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22631 passed | 379 expected fail | 131 skipped (23141)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19983 passed | 382 expected fail | 131 skipped (20496)
```
|
Thanks. In 4b450dd: an inserted end closes the content before it only when nothing between it and the previous known end is content the editor shows ( |
…nserts once
## Description
Review of the previous commit: `endsNewUnit` walked from each inserted unit end back to the end before it that the binding knows, so a change not read yet that inserts N paragraphs in a row, applied in an update that moves a line, walked the run N times (a move past 3,200 such paragraphs took about 1.25 s, against about 0.1 s before). The answer for every inserted end a walk passes is the same as for the end it started from, so it is cached for the rest of the write (`Writer#$heldKeys`) or the check (`$reordersHeld`), and each run is walked once. Same results, same bytes.
## Test plan
### Before
`ComposedMovePendingText` passes on the previous commit (the change is in cost, not result).
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 140 passed (140)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 140 passed (140)
```
Full suites:
```
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22631 passed | 379 expected fail | 131 skipped (23141)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19983 passed | 382 expected fail | 131 skipped (20496)
```
There was a problem hiding this comment.
This looks good to me. The split-line case from the last round now keeps aXa / bb, and a regression cell covers it. Caching endsNewUnit keeps the results the same and makes a long run of inserted paragraphs cheap again. Everything below is optional and non-blocking.
Optional suggestions
-
An unread paragraph between the lines a held line passes ends up next to the moved line (
packages/lexical-y/src/internal/Write.ts, lines 2059-2066). Keeping the held line's items rewrites the lines it passes. A paragraph that the same not-yet-read change inserted between two of those lines is not rewritten, so it stays beside the kept line. Repro: B typesXintoaaand insertsnewbetweenccanddd; A applies that update inside aneditor.updatethat movesaato the end. Every client ends withbb / cc / dd / aXa / new, wheremaingivesXbb / cc / new / dd / aa. Nothing is lost, but neither y.md's Moves limit nor D112 mentions it.packages/lexical-y/AGENTS.md(Decisions) asks for accepted limits to be recorded in both, so a clause plus a cell would pin it. Alternatively,$reordersHeldcould refuse this shape, as it already does for two held lines. -
Two walks are quadratic in a large paste into one line. They are
unitOfin$movesHeld(packages/lexical-y/src/internal/Binding.ts, lines 4476-4484) and the walk in$heldKeys(packages/lexical-y/src/internal/Write.ts, lines 2273-2279). Each held or unread item walks right to its unit's end, so items that all sit in one line repeat the same walk.- A collaborator's 1600-line paste into the middle of the line being composed is held, per y.md's Input method composition note. While it waits, each composed keystroke takes about 40 ms, against under 2 ms on
main. - A not-yet-read 1600-paragraph paste into a line, applied in an update that inserts a paragraph, takes about 350 ms, against about 45 ms on
main.
Remembering the unit for every item a walk passes (a
Map<Item, Unit | undefined>filled along the walk) brings both back tomain's cost with the same results. A cell likeregressions/Scaling.test.tswould keep it there. - A collaborator's 1600-line paste into the middle of the line being composed is held, per y.md's Input method composition note. While it waits, each composed keystroke takes about 40 ms, against under 2 ms on
-
Evidence for the cost-only commit.
packages/lexical-y/AGENTS.md(Testing) asks a refactor to show that it stores the same bytes. The commit "Walk a run of paragraphs a change not read yet inserts once" says "Same results, same bytes", but its Test plan shows only the regression run; a short byte comparison would document it. The catalog-driven cells from the same section are still open, as you noted. -
Real-browser check of a refused move. The collab-Y E2E specs pass at this head, but none moves the composed line while a peer's change waits. One spec (
pnpm run test-e2e-collab-y-chromium, listed in AGENTS.md) or a recorded manual check would show what the input method does when the refused move is read back.
| }; | ||
| const unitOf = (it: Item, cs: CS): Unit | undefined => { | ||
| let end: Item | null = it; | ||
| while (end !== null && !state.unitByEnd.has(end)) { |
There was a problem hiding this comment.
Optional: every held item walks from here to its unit's end, so a collaborator's large paste into the composed line (held whole) makes each composed keystroke quadratic. With 1600 held lines that is about 40 ms per keystroke, against under 2 ms on main. Caching the unit for each item passed (a Map<Item, Unit | undefined> filled along the walk) keeps the results and restores main's cost.
| inserted(unseen, end) && | ||
| endsNewUnit(unseen, end, e => state.unitByEnd.has(e), newEnds); | ||
| let end: Item | null = it; | ||
| while (end !== null && !state.unitByEnd.has(end) && !isNewEnd(end)) { |
There was a problem hiding this comment.
Optional: same pattern as unitOf in $movesHeld. Since an unread split's ends are no longer new ends, every item of a not-yet-read paste into a line walks to the line's original end. A 1600-paragraph paste applied in an update that inserts a paragraph takes about 350 ms, against about 45 ms on main. A per-item memo of the unit found fixes it.
| // A unit holding what a composition holds back (see Hidden, $heldKeys) | ||
| // keeps its items: the units it passed are written again instead. | ||
| const held = this.$heldKeys(cs.y); | ||
| const heldWeight = 4 * big * (matches.length + 1); |
There was a problem hiding this comment.
Optional: when the kept unit passes other lines, they are rewritten, but a paragraph that the same not-yet-read change inserted between two of them is not. It ends up beside the kept line: aXa / bb / cc / new / dd with aa moved to the end gives bb / cc / dd / aXa / new on every client. Worth a clause in y.md's Moves limit and D112, plus a cell, or a refusal like the one for two held lines.
…a change not read yet appends, not at a format inside it; walk a line's items once
## Description
Review of the previous commit:
- A local update that appends a paragraph, while a change not read yet appends a paragraph with formatted text at the same place: the cursor stops before a held or unread format that opens a run where it writes whole units at a boundary, and it stopped at such a format inside the change's paragraph, so the update's paragraph landed in the middle of it (`[k19][k20` / `[k21]` / `]`), on every client; `main` writes it after. A format past content the editor does not show is inside that content, not at the boundary: the cursor no longer stops there (`Cursor#passesUnseen`).
- Finding the unit that holds each held or unread item walked to the end of its line once per item, so a collaborator's large paste into the line being composed cost each composed keystroke about 40 ms while it was held. The unit found is remembered for every item a walk passes (`Writer#$heldKeys`, `$movesHeld`).
- A paragraph a change not read yet inserted between two lines a kept line passes is not written again with them, and ends up next to the kept line; nothing is lost. D112 and y.md's Moves limit say so, and a cell pins it.
## Test plan
### Before
On the previous commit, `a formatted paragraph a change not read yet appends, and one the update appends` fails, gc off and on (`main` passes it):
```
× gc false
× gc true
Tests 2 failed | 140 skipped (142)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 144 passed (144)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 144 passed (144)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22635 passed | 379 expected fail | 131 skipped (23145)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19987 passed | 382 expected fail | 131 skipped (20500)
```
|
Thanks. In 09c886d: the unit found for each held or unread item is remembered for every item a walk passes ( |
There was a problem hiding this comment.
This looks good to me. Moving the line being composed now keeps a collaborator's held-back text on every client. Moves that cannot keep it are refused and read back with a clear 'composing-move' reason. Since the last round, a large held paste no longer slows composed keystrokes, and the unread-paragraph placement is documented. Everything below is optional and non-blocking, though the first item is worth fixing before merge.
Optional suggestions
- After an unread paragraph, written lines land inside a collaborator's format run again (
packages/lexical-y/src/internal/Cursor.ts, lines 549-555 and 744-750).passedis set once the cursor steps over unseen content and is never cleared. So when a paragraph that a change not read yet inserted comes right before a format that change opens over the next line, the cursor no longer stops at that format. Repro: B insertsnewbeforeaaand boldsaa, and A applies that update inside aneditor.updatethat movesaato the end. Every client ends withbbandccbold (new / bb / cc / aa), while the previous commit gave new / bb / cc / aa. That contradicts D112's "not inside it". Resettingpassedafter the end item of a unit the change inserted fixes it, and the whole@lexical/yunit run still passes with it:passed = isEndItem(r) && isUnreadInsert(r) ? false : passed || this.passesUnseen(r);
packages/lexical-y/AGENTS.md(Testing) asks every bug fix to land with a regression cell, so a cell for this shape would pin it. - The new memo never hits for items of an unread new paragraph (
packages/lexical-y/src/internal/Write.ts, lines 2280-2283;packages/lexical-y/src/internal/Binding.ts, lines 4484-4487).undefinedmeans both "not walked yet" and "no unit", which is what an unread new paragraph resolves to. So each of its items walks to its end again. A change not read yet that appends one paragraph of 2000 formatted runs, applied inside an update that inserts a paragraph, takes about 400 ms, against about 20 ms onmain.unitAt.has(end)restoresmain's cost, and the cells still pass. - Real-browser check of a refused move. The collab-Y E2E jobs pass, but none moves the composed line while a peer's change waits. One spec (
pnpm run test-e2e-collab-y-chromium, listed in AGENTS.md) or a recorded manual check would show how the input method copes when a refused move is read back mid-composition. You have already said this is out of scope here.
| if (this.opensAhead(r, passed)) { | ||
| break; | ||
| } | ||
| passed = passed || this.passesUnseen(r); |
There was a problem hiding this comment.
Optional, but worth fixing before merge: passed is never cleared, so after a paragraph that a change not read yet inserted, a format that change opens over the next line is no longer a boundary. Example: B inserts new before aa and bolds aa, and A applies it inside an update that moves aa to the end. Every client ends with bb and cc bold. Clearing it after an unread unit end fixes that and keeps the new cell passing:
passed =
isEndItem(r) && isUnreadInsert(r) ? false : passed || this.passesUnseen(r);The same applies at line 750.
| let end: Item | null = it; | ||
| let unit: Unit | undefined | null = null; | ||
| while (end !== null && !state.unitByEnd.has(end) && !isNewEnd(end)) { | ||
| const at = unitAt.get(end); |
There was a problem hiding this comment.
Optional: undefined is both "not walked yet" and the stored "no unit" (the walk stopped at an unread new paragraph's end), so items of such a paragraph never hit the memo. One unread paragraph of 2000 formatted runs applied inside an update takes about 400 ms here, against about 20 ms on main. if (unitAt.has(end)) { unit = unitAt.get(end); break; } fixes it; unitOf in Binding.ts (line 4484) has the same pattern.
…boundary again; remember items that are in no unit
## Description
Review of the previous commit:
- Once the cursor stepped over content the editor does not show, it never stopped at a format again, so a paragraph a change not read yet inserted right before a line that change formats put the lines written again inside that format run (B inserts `new` before `aa` and bolds `aa`, A moves `aa` to the end: `bb` and `cc` came out bold). The end of a unit such a change inserted now closes that content: a format after it is at a boundary again.
- The memo of the unit holding each item could not remember "no unit" (the items of a paragraph a change not read yet inserted), so each of them walked to its end again; it now does.
## Test plan
### Before
On the previous commit, `a paragraph inserted before a line a change not read yet formats, that line moved` fails, gc off and on:
```
× gc false
× gc true
Tests 2 failed | 144 passed (146)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 146 passed (146)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 146 passed (146)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22637 passed | 379 expected fail | 131 skipped (23147)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 1 failed | 269 passed | 2 skipped (272)
Tests 1 failed | 19988 passed | 382 expected fail | 131 skipped (20502)
```
The one failure is the timing test `Scaling.test.ts > R21-B` under the full run's load; alone:
```
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/Scaling.test.ts
Tests 1 passed (1)
```
|
Thanks. In 0333d0b: the end of a unit a change not read yet inserted resets |
There was a problem hiding this comment.
Thanks for the careful work here. The new regression file clearly separates the fix (its cells fail on main and pass at this head), and keeping a unit within its container while refusing a move to another one reads well. One issue should be fixed before merge; the rest are optional.
Must fix: a passed line's unread deletion or attribute change is undone
packages/lexical-y/src/internal/Binding.ts:4319: $reordersHeld's holds counts only content that a change not read yet inserted. When the same change deletes text, or sets marker attributes, in a line that the kept line passes, the reorder is not refused. Writer#$heldKeys then keeps the moved line, and the passed lines are rewritten from the editor's view, which still shows what the change deleted. Repro, in the shape of the inserted by a change not read yet in two lines, reordered cells: B types X into aa and changes the next line; A applies B's update inside its own editor.update() and moves aa to the end.
| B's change to the passed line | main |
this PR |
|---|---|---|
deletes one b of bbb |
Xbb / cc / aa |
bbb / cc / aXa |
removes paragraph bb |
X / cc / aa |
bb / cc / aXa |
makes bb a heading |
heading("Xbb") / ... |
paragraph("bb") / ... |
checks list item bb |
listitem[x]("Xbb") ... |
listitem[ ]("bb") ... |
Every peer and a fresh reader agree, with gc on or off, and nothing is warned. Preventing a rewrite from bringing back what such a change deleted is what D108 in packages/lexical-y/DECISIONS.md is for, and D112 calls the keep exact, with no resurrected delete, yet the rewritten passed lines bring one back. Suggested fix: also count a unit as holding when an unread change deleted one of its items or set an attribute on its end item, so that the reorder is refused as it is for two units holding inserts. Then add these shapes as cells in ComposedMovePendingText.test.ts (packages/lexical-y/AGENTS.md: every bug fix lands with a regression test).
Optional suggestions
packages/lexical-y/src/internal/Write.ts:2062:heldWeightkeeps the held unit whatever it passes. Moving the composed line from the top to the bottom of 200 paragraphs rewrites all 199 passed lines (7,431 bytes, against 38 onmain), and a third peer's concurrent typing in any of them ends up as a line of its own after the kept one. D112 accepts rewriting the passed side; consider bounding it (refuse once the passed units outweigh the kept one), or saying in y.md's Moves limit what happens to concurrent typing in passed lines.- The changeset's
Verify:paragraph could mention that same effect on passed lines. - The catalog-driven cells (
packages/lexical-y/AGENTS.md: "Test the input space") and a collab-Y E2E run with a real composition are already noted as follow-ups.
The TypeScript and Flow types for the new 'composing-move' reason, the changeset and the docs updates look good.
| !(hidden !== null && hidden.hides(it)) && | ||
| !(unread !== null && insertedBy(unread, it)); | ||
| // Whether `it` or anything below it is content the editor does not show. | ||
| const holds = (it: Item): boolean => { |
There was a problem hiding this comment.
Must fix: holds counts only content a change not read yet inserted. A passed line whose unread change deleted text (or removed the paragraph, or set a marker attribute such as a heading type or a check) is not counted, so the reorder is not refused, and the write rewrites that line from the editor's view: B's deletion of a b in bbb comes back as bbb / cc / aXa on every peer, where main gives Xbb / cc / aa. Counting a unit as holding when an unread change deleted one of its items, or set an attribute on its end item, would refuse it like two units holding inserts.
| // A unit holding what a composition holds back (see Hidden, $heldKeys) | ||
| // keeps its items: the units it passed are written again instead. | ||
| const held = this.$heldKeys(cs.y); | ||
| const heldWeight = 4 * big * (matches.length + 1); |
There was a problem hiding this comment.
Optional: this weight keeps the held unit whatever it passes. Moving the composed line from the top to the bottom of 200 paragraphs rewrites all 199 passed lines (7,431 bytes, against 38 on main), and a third peer's concurrent typing in a passed line ends up as a line of its own after the kept one. Consider refusing once the passed units outweigh the kept one, or documenting it in y.md's Moves limit.
… characters
## Description
The refusal (`'composing-move'`) read the whole commit again, so a commit that also typed (an input method's composed text moved or merged in the same update, an app's reaction to a keystroke) dropped what was typed. A commit that adds characters is now never refused (`addsCharacters`: a text node it changed whose text is not a subsequence, by code point, of the text it had, or a new non-empty text node): it is written as any other, and the held content is lost where the write cannot keep it, as before this pull request. What is typed is never dropped. D112, y.md, the warning's doc comments and the changeset say so.
## Test plan
### Before
On the previous commit, `held back, moved to another container with typing: written` fails, gc off and on (the commit is refused and `x` dropped):
```
× the line, into a block node (gc false)
× the line, into a block node (gc true)
Tests 2 failed | 146 skipped (148)
```
Over the 48 typing-plus-move or merge-into-emptied sessions of an external review harness, the previous commit kept the collaborator's text in all 48 and dropped the local typing in all 48.
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 148 passed (148)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 148 passed (148)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22639 passed | 379 expected fail | 131 skipped (23149)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 19991 passed | 382 expected fail | 131 skipped (20504)
```
The 48 sessions now keep the local typing in all 48, as `main` does (and lose the collaborator's text, as `main` does).
… not read yet deleted in a line a kept line passes
## Description
Review of the previous commits: the check for a reorder of units holding pending content counted only what a change not read yet inserted. When the same change also deleted text in a line the kept line passes, removed that line, or set one of its attributes (a block type, a checked list item), the reorder was not refused: the kept line stayed and the passed line was written again from the editor, which still showed what the change deleted, bringing it back on every client (`bbb / cc / aXa` where the change deleted a `b`). A unit now also holds pending content where a change not read yet deleted one of its items or set or removed one of its attributes, so such a reorder is refused like any other. A commit that is not refused because it adds characters is now written without keeping any such unit (`Writer#pins`), exactly as without the hold, so it cannot bring anything back either.
y.md and the changeset also say that what a collaborator types at the same time in a line a kept line passes lands next to the kept line.
## Test plan
### Before
On the previous commit, `a change not read yet types into a line and changes a line it passes, the line moved` fails, gc off and on (12 of 16; the 4 typing cells that change attributes pass):
```
× deletes text in it (gc false)
× deletes text in it, the update typing (gc false)
× removes it (gc false)
× removes it, the update typing (gc false)
× makes it a heading (gc false)
× checks it (gc false)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 164 passed (164)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 164 passed (164)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 382 passed | 2 skipped (384)
Tests 22655 passed | 379 expected fail | 131 skipped (23165)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 20007 passed | 382 expected fail | 131 skipped (20520)
```
|
Thanks. In 2ca6a5f: |
…read yet deleted
## Description
Review of the previous commit: the editor can still show a character that a change not read yet deleted (D108, applied during the update). A commit that types that character again (an input method replacing the whole composed text with it) and moves held content looked like it added nothing (its text a subsequence of what the editor showed), so it was refused, and reading the document again dropped what was typed on every client. While a change not read yet has deleted something, any change to a text node now counts as adding characters, so such a commit is written. D112 says so.
## Test plan
### Before
On the previous commit, `held back, moved to another container retyping a character a change not read yet deleted` fails, gc off and on (refused, with the warning):
```
× gc false
× gc true
AssertionError: expected [ Array(1) ] to deeply equal []
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 166 passed (166)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 166 passed (166)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 1 failed | 381 passed | 2 skipped (384)
Tests 1 failed | 22656 passed | 379 expected fail | 131 skipped (23167)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 270 passed | 2 skipped (272)
Tests 20009 passed | 382 expected fail | 131 skipped (20522)
```
The one failure is the timing test `Scaling.test.ts > R21-B` under the full run's load; alone:
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/Scaling.test.ts
Tests 1 passed (1)
```
… predicate shared with the identity binding
## Description
Whether a commit that moves held content adds characters (and so is never refused) is now decided by `insertionFree`, the same pure module the identity binding uses (`internal/InsertionFree.ts`, with its unit tests): an update that only deletes, moves or re-keys text adds none, where each run of removed text nodes can be given to the kept text node before or after it, or to one node the update made, and every kept or made node's text is what deleting characters from its sources gives. The previous check took any text a kept node gained as typed, so an unwrap of the inline container composed in, whose text Lexical merges into the text before it, was written as without the hold and lost the held text; it is now refused like any other move of held content out of its node. Typing, a composition's text, paste and text from nothing still add characters, and a commit that adds characters is still never refused. The texts passed are what the editor showed and shows: a composition holds back only insertions, and a change not read yet that deleted something is still admitted separately (D108). D112 says so.
## Test plan
### Before
On the previous commit, `held back in an inline container, unwrapped into the text before it` fails, gc off and on (written, R lost):
```
× gc false
× gc true
AssertionError: expected [] to have a length of 1 but got +0
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/InsertionFree.test.ts
Tests 11 passed (11)
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 168 passed (168)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 168 passed (168)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 383 passed | 2 skipped (385)
Tests 22670 passed | 379 expected fail | 131 skipped (23180)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 271 passed | 2 skipped (273)
Tests 20022 passed | 382 expected fail | 131 skipped (20535)
```
…hange is not read yet; bound the insertion-free search
## Description
Review of the previous commit:
- A commit refused while a change applied during it was not read yet (D108) was read again with that change in it, and the change was then read a second time: a collaborator's `S` showed and was stored twice (`aSRSbc`), where `main` stores it once. While any change is not read yet at decision time, a commit is now never refused: it is written as without the hold, keeping no unit (as the identity binding does, and as `main` writes it). This also covers retyping a character that change deleted.
- `insertionFree`'s search tried every assignment of up to 8 removed runs, about 5 s for 8 runs where none fits (2.3 s for a small update while composing). A made node now only takes a run whose text holds it, made nodes are tried first and a branch stops when too few runs remain for them, and the search stops after `MAX_STEPS` (20,000) assignments, counting the update as inserting (so never refused). The same change goes to the identity binding's copy of the module.
D112, y.md, the warning's doc comments and the changeset say which commits are not refused. The cells for a change not read yet that changes two lines' order, or a line a kept line passes, now check that the commit is written with nothing brought back and no warning.
## Test plan
### Before
On the previous commit, an external review case (a composition holding `R`, a collaborator's `S` applied during the update that moves the inline container to another line) ends `aSRSbc` on every client; `insertionFree` over 8 removed runs `ab` and 8 made `ba` takes about 4.8 s.
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/InsertionFree.test.ts packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts
Tests 182 passed (182)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical/
Test Files 383 passed | 2 skipped (385)
Tests 22673 passed | 379 expected fail | 131 skipped (23183)
$ LEXICAL_Y_GC=1 pnpm exec vitest run --project unit packages/lexical-y
Test Files 271 passed | 2 skipped (273)
Tests 20025 passed | 382 expected fail | 131 skipped (20538)
```
The external case now ends `dest|abc` (16 variants: no warning, no duplicate, the editor equal to a fresh reader); the 8-run case returns at once.
There was a problem hiding this comment.
Thanks, this looks good. The regression file tells the fix apart from main (it fails there and passes here with gc on and off), and the refusal path, D112 and the y.md limits fit together. Everything below is optional and non-blocking.
1. Concurrent edits in a passed block node (packages/lexical-y/src/internal/Write.ts:2069). When a unit holding held or unread content is kept, every unit it passes is written again. For a passed line, a collaborator's concurrent typing lands next to the kept line, as y.md says. A passed block node (a box, a table), though, is copied whole, and a collaborator's concurrent edit inside it is lost. Repro with three peers:
- A composes in
abcwhile B'sRis held back. - C, offline, types
Zinto a boxed paragraphinbox. - A moves its line to the end, past the box.
On main every client ends with paragraph("R") / test_box(paragraph("inZbox")) / …. Here they end with test_box(paragraph("inbox")) / paragraph("tail") / paragraph("aRbc"). The D108 path does the same with no composition. packages/lexical-y/AGENTS.md asks for accepted limits to be recorded in DECISIONS.md and in y.md's Limits. Right now y.md Moves (lines 1096-1099), D112 and the changeset's last Verify: sentence say concurrent typing in a passed block lands next to the kept one. Please either document the block-node case, or skip the keep when the passed run contains a block node.
2. A refused commit drops non-text content (packages/lexical-y/src/internal/Binding.ts:8819). addsCharacters compares only text nodes. An update that inserts an inline equation and moves the composed line into a box is therefore refused whole, and the equation disappears. Counting any node the update made as an insertion would keep to the rule's intent. Otherwise, mention it next to the "adds characters" rule in y.md.
3. Docs: unwraps. The cell held back in an inline container, unwrapped into the text before it expects a refusal. So removing an inline element around the composed text (a comment mark, say) is undone while a change waits. y.md's Input method composition note, the warnings table, the UNSUPPORTED_EDIT_WARNING / YUnsupportedEditData JSDoc and D112 cover a replacement and a move to another line, but not an unwrap.
4. Tests and cost.
- The two
insertionFree costcells (InsertionFree.test.ts:156and:169) never reachMAX_STEPS. The first returns at the made-node precheck, and the second atMAX_RUNS(the trailingzrun makes 9 runs). 8 one-character runs with 8 made nodes that each fit every run does reach the budget. - The budget limits assignments, not work: each full assignment re-checks every changed text, so a 20,000-character changed text takes about 2.7 s before the search gives up. Checking each changed node once for its four run combinations (none, left, right, both) before the search would bound it.
packages/lexical-y/AGENTS.mdasks for inputs drawn fromsrc/__tests__/catalog/. Table cells (named iny.mdand the changeset), links and comment marks as holders are not covered yet.- AGENTS.md's Testing Strategy keeps behaviour that needs a real browser out of the node tier. A collab Y E2E run (
pnpm run start:y, thenpnpm run test-e2e-collab-y-chromium) of a refused move during a real IME composition would show how the immediate re-read behaves.
5. Small wording.
$reordersHeld's JSDoc still says such a commit "is not written" (Binding.ts:4337).- D112 and
Binding.ts:4305/:8810describeinsertionFreeas shared with "the identity binding", but no other binding in this repository importsInsertionFree.ts. - The comment at
ComposedMovePendingText.test.ts:380puts the "with typing: written" explanation above the unwrap describe.
| // A unit holding what a composition holds back (see Hidden, $heldKeys) | ||
| // keeps its items: the units it passed are written again instead. | ||
| const held = this.$heldKeys(cs.y); | ||
| const heldWeight = 4 * big * (matches.length + 1); |
There was a problem hiding this comment.
Optional: this weight keeps a held unit over every passed one, so a passed block node (box, table) is copied whole and a collaborator's concurrent edit inside it is lost. With three peers (A composing in abc with B's R held, C typing Z into a boxed inbox, A moving its line past the box), main keeps inZbox and this branch ends with test_box(paragraph("inbox")) on every client. Consider skipping the keep when the passed run holds a block node, or documenting it in D112 and y.md's Moves limit.
| keeps that change, and the blocks it passes are written again (a block | ||
| the same change inserted between two of those ends up next to the kept | ||
| block, and so does what a collaborator types at the same time in a block | ||
| it passes, as with any rewritten block). Of two |
There was a problem hiding this comment.
Optional: for a passed block node (a box or a table) the whole node is written again, so what a collaborator types in it at the same time is lost, not placed next to the kept block. The changeset's last Verify: sentence says the same thing and could be corrected too.
| * insertions only) and for what a change not read yet deleted, which the | ||
| * caller admits separately. | ||
| */ | ||
| function addsCharacters( |
There was a problem hiding this comment.
Optional: only TextNode text is compared, so a refused commit also drops non-text content it made. An inline equation inserted in the same update as a move into a box disappears. Counting any node the update made as an insertion would keep the never-drop intent.
| expect(check(before, after)).toBe(false); | ||
| expect(performance.now() - started).toBeLessThan(1000); | ||
| }); | ||
| test('eight runs whose made nodes fit but a kept node never does stop at the budget', () => { |
There was a problem hiding this comment.
Nit: the trailing ['z', 'z'] makes 9 runs, so this returns on MAX_RUNS ("9 runs of removed text") without searching, and the previous cell returns at the made-node precheck. Neither reaches MAX_STEPS. 8 one-character runs with 8 made nodes that each fit every run would.
| * and written again from the editor, which does not show that content. | ||
| * Units moved to another container are left to $movesHeld (held) or the | ||
| * concurrent move limits (not read yet). | ||
| * Invariant: such a commit is not written (see refusesHeldMove). |
There was a problem hiding this comment.
Nit: since pinsOff, a commit that adds characters or is made while a change is unread is written (with no unit kept), so this invariant no longer holds as stated.
Description
While the editor composes (IME) in a line, a collaborator's insert into that
line is applied to Yjs but held back from the editor (
Hidden); a changeapplied during a local update is likewise not read until after it (
Unread,#9784). A commit that moved the list item holding that content (down its
list, to another list, nested), made locally or by an app's code in the
update of a remote change's read (#9788), wrote the move as a delete and a
write of the editor's view, which lacks it:
inline container, a block node holding the line) was deleted with it, on
every client;
showed glued to the next line (
Rtail).Now:
inline containers that hold content the editor does not show where they
match one in the container (
Writer#$heldKeys): the moved unit keeps itsitems, and the units it passed are written again instead. Exact: nothing
hidden is copied, so held formats, held inline nodes and held line markers
stay as stored, with no duplicate or resurrected delete.
unread format that opens a run over it (
Cursor#heldBoundary), not insidethat run.
of a block node, a block node into another, an inline container to another
line) cannot keep its items: it is not written, is read again, and reports
UNSUPPORTED_EDIT_WARNINGwith a newdata.reason,'composing-move',once per refused commit (
YBinding#refusesHeldMove, on the local commitpath and in
$writeReadEdits). The move is undone locally and nothing heldis lost.
Unchanged and out of scope: what a change not read yet inserted into a unit
moved to another container stays under the concurrent move limits (D48, D49);
text composed in a node moved into a new line elsewhere, its old line removed,
leaves the held content at the old place, where it shows once the hold ends,
as after Enter. Recorded as D112 in DECISIONS.md; y.md's Moves limit, Input
method composition note and warnings table are updated, and so are
YUnsupportedEditData(TypeScript and Flow) and theUNSUPPORTED_EDIT_WARNINGJSDoc.Changeset
.changeset/lexical-y-composed-move-keeps-held-text.md([fix], with aVerify:paragraph naming the new warning reason).A commit that adds characters (decided by
insertionFree, a pure module shared with the identity binding), or one made while a change applied during it is not read yet (D108), is never refused (what is typed is never dropped, and a deferred change is never read twice): it is written as any other, and held content is lost where the write cannot keep it, as onmain.Review rounds added (each with cells that fail on the commit before it):
Test plan
New regression file
packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.ts(168 cases, three peers, checked against a fresh reader, gc on and off).Before
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/ComposedMovePendingText.test.tsonmain(dfe058a), first commit's cells (later cells fail on the commit before them, see each commit):After
At the head of this pull request:
tsc, Flow, ESLint, Prettier andcheck-changesetsare clean. Browser and E2E suites were not run.