Repository navigation
[lexical-y] Bug Fix: Keep the text before the node being composed in a code token split by comment ids - #9580
Merged
Merged
Conversation
…a code token split by comment ids ## Description While an editor composes with an IME, a read of the line being composed keeps the composed node and its key, so the browser goes on composing in its DOM text node. The read reuses the line's other text nodes for the other pieces: in order, or by segment for code highlight nodes (the nodes of one highlighted token share a segment). A code token that NodeState splits into several highlight nodes (comment ids on part of it) shares one segment among them, and the reader's segment lookup kept the last of them: in the reported line, the node being composed. When a collaborator's change to the line made the editor read it during the composition, an earlier piece of that token took the composed node by segment and set its text, and the composed piece then took the same node again. The earlier piece's text (`// t46z note t47z`, which nobody edited) disappeared from the editor, and the composing client wrote the line without it for every client when the composition ended. The guard against reusing the composed node covered only the in-order reuse. Now, once the composed node is reserved for its piece, it counts as used: no other piece reuses it, in order or by segment. The same reuse also lost text in a second line shape found by the soak (a remote insert in a code line before the composed token), which this fixes too. Closes #9577 ## Changeset `.changeset/lexical-y-composition-code-segments.md` (`@lexical/y`, `[fix]`). ## Test plan `regressions/CompositionCodeSegments.test.ts` replays the issue's captured document and the two remote updates, composing and not composing. ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/CompositionCodeSegments.test.ts ``` ### Before ``` × remote comment id changes to the line keep its text (composing: true) 126ms ✓ remote comment id changes to the line keep its text (composing: false) 49ms Expected: "const t44z = fetch(t45z); // t46z note t47zt48z t49z t50z t t51z" Received: "const t44z = fetch(t45z); t48z t49z t50z t t51z" Tests 1 failed | 1 passed (2) ``` ### After ``` ✓ unit packages/lexical-y/src/__tests__/unit/regressions/CompositionCodeSegments.test.ts (2 tests) 235ms Tests 2 passed (2) ``` `pnpm exec vitest run --project unit packages/lexical-y packages/lexical-code-prism packages/lexical-mark`: ``` Test Files 188 passed | 2 skipped (190) Tests 8765 passed | 46 expected fail | 4 skipped (8815) ``` `pnpm run ci-check` (each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.
zurfyx
requested review from
acywatson,
ivailop7 and
potatowagon
as code owners
October 30, 2026 12:19
|
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 for tracking this down with a captured replay. Marking the composed node as used once it is placed (packages/lexical-y/src/internal/Read.ts, the new block after the two placement loops) closes the path the old in-order-only guard missed. bySegment keeps only the last node of each segment, so an earlier piece of a code token that NodeState split could take the composed node; the composed piece then took the same node again and overwrote that text. Without the Read.ts change, the new test's composing case fails with Received: "const t44z = fetch(t45z); t48z t49z t50z t t51z". With it, both cases pass, and the composed node keeps its key and caret through both remote updates. The changeset follows CONTRIBUTING.md: a patch bump, a one-line [fix] summary and a Verify: paragraph.
Two suggestions:
- Back the second line shape with a test, or drop the claim. The description says this also fixes "a remote insert in a code line before the composed token", but
CompositionCodeSegments.test.tsreplays only #9577's document and its two comment-id updates.AGENTS.mdasks the Test plan to carry the real Before and After output of what was run. Please add that captured shape as a second case, or remove the sentence. - Nit: assert that the composition stays in its node (
CompositionCodeSegments.test.ts:154). Without the fix, the second remote update also detaches the composed node and moves the composition key to a new node, which in a browser replaces the DOM text node the IME is composing in. The test compares line text only. Checking that$getNodeByKey(key)is still attached after each update would pin that behavior as well.
Optional: the extended workflow, which includes the Collab Y e2e specs, was skipped on this head because its gate (the extended-tests label or an approving review) was closed. A maintainer can add the label to get browser coverage of the collab path.
| for (const u of REMOTE) { | ||
| Y.applyUpdate(doc, b64(u), 'remote'); | ||
| await idle(); | ||
| expect(codeLine(editor)).toBe(expected); |
There was a problem hiding this comment.
Nit: this compares line text only. Without the Read.ts change, the second update also detaches the composed node and moves the composition key to a new node, which in a browser replaces the DOM text node the IME is composing in. An assertion like this would pin that the composition stays in its node:
expect(
editor.getEditorState().read(() => $getNodeByKey(key)?.isAttached() ?? false),
).toBe(composing);
Member
Author
There was a problem hiding this comment.
Done in c00aea6: after each remote update the test now checks that the composed node is still attached (toBe(composing), as suggested).
…#9577 test ## Description The #9577 regression test compared only the code line's text. It now also checks, after each remote update, that the node being composed is still attached: before the fix, the second update also detached it and moved the composition to a new node, which in a browser replaces the DOM text node the IME is composing in. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/CompositionCodeSegments.test.ts ``` ### Before Without the `Read.ts` change: ``` AssertionError: expected 'const t44z = fetch(t45z); t48z t49z t…' to be 'const t44z = fetch(t45z); // t46z not…' // Object.is equality Expected: "const t44z = fetch(t45z); // t46z note t47zt48z t49z t50z t t51z" Received: "const t44z = fetch(t45z); t48z t49z t50z t t51z" ``` ### After ``` Tests 2 passed (2) ```
Member
Author
|
Review follow-ups: (1) the second line shape needs the randomized harness it was captured with, so I removed that sentence from the description rather than add a test that depends on the harness; (2) the composition check is in c00aea6. |
zurfyx
added a commit
that referenced
this pull request
Oct 30, 2026
## Description Merges `main` (cad9015) into this branch to resolve the conflict in `packages/lexical-y/src/internal/Read.ts` with #9580 ("Keep the composed node for its own piece"). Resolution: this branch's removal of the fallback loop that marked a caret piece as composed stays removed, and #9580's reservation of the composed node (`usedTexts.add(composed)` when it was placed) is kept after this branch's placement loop. No other change. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/Composition.test.ts packages/lexical-y/src/__tests__/unit/CompositionHoldBack.test.ts packages/lexical-y/src/__tests__/unit/CompositionHoldBackBlockIds.test.ts packages/lexical-y/src/__tests__/unit/CompositionInterleave.test.ts packages/lexical-y/src/__tests__/unit/regressions/Composition.test.ts packages/lexical-y/src/__tests__/unit/regressions/CompositionCodeSegments.test.ts ``` ### Before `git merge main` stops with a conflict in `Read.ts`. ### After ``` Test Files 6 passed (6) Tests 2066 passed | 2 skipped (2068) ``` ``` pnpm exec vitest run --project unit packages/lexical-y packages/lexical-hashtag Test Files 169 passed | 2 skipped (171) Tests 7510 passed | 46 expected fail | 4 skipped (7560) ``` `pnpm run ci-check` passes.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
While an editor composes with an IME, a read of the line being composed keeps the composed node and its key, so the browser goes on composing in its DOM text node. The read reuses the line's other text nodes for the other pieces: in order, or by segment for code highlight nodes (the nodes of one highlighted token share a segment).
A code token that NodeState splits into several highlight nodes (comment ids on part of it) shares one segment among them, and the reader's segment lookup kept the last of them: in the reported line, the node being composed. When a collaborator's change to the line made the editor read it during the composition, an earlier piece of that token took the composed node by segment and set its text, and the composed piece then took the same node again. The earlier piece's text (
// t46z note t47z, which nobody edited) disappeared from the editor, and the composing client wrote the line without it for every client when the composition ended. The guard against reusing the composed node covered only the in-order reuse.Now, once the composed node is reserved for its piece, it counts as used: no other piece reuses it, in order or by segment.
Closes #9577
Changeset
.changeset/lexical-y-composition-code-segments.md(@lexical/y,[fix]).Test plan
regressions/CompositionCodeSegments.test.tsreplays the issue's captured document and the two remote updates, composing and not composing.Before
After
The second commit also checks that the node being composed stays attached after each remote update (
Tests 2 passed (2)).pnpm exec vitest run --project unit packages/lexical-y packages/lexical-code-prism packages/lexical-mark:pnpm run ci-check(each script): tsc, tsc-test, tsc-scripts, tsc-extension, tsc-website, flow, prettier, lint, check-changesets pass.