Repository navigation
[lexical-y] Bug Fix: Read an inline element around blocks only, in a shadow root, as those blocks - #9654
[lexical-y] Bug Fix: Read an inline element around blocks only, in a shadow root, as those blocks#9654zurfyx wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this. It fixes #9571 as reported: with an outside link run over both shadow-root quotes, deleting the first quote now writes only that quote's text, and a collaborator's concurrent typing in the second quote survives on every client and on a fresh reader. The new S4-8 cases fail on main and pass here.
Decision needed before this lands. packages/lexical-y/DECISIONS.md records D89 as OPEN against D70 (DECIDED: no virtual structure beyond one existing case). packages/lexical-y/AGENTS.md lets a pull request bring new evidence against a decided row, and #9571 is that evidence. Can a maintainer confirm D89, so it lands as DECIDED? One fact for that call: the stored link is not shown inside the quote, while the same run still shows as a link on ordinary lines (a run from the title to the end paragraph shows on "tle" and "end" and disappears across the quotes).
Suggestions:
- D89's text (
DECISIONS.md:139) and the description say no local edit removes the stored link, but one does. A delete or typing inside the paragraph leaves it. Enter at the end of the second quote's paragraph (or inserting a paragraph after it) writesw:link=nullbefore that paragraph, so the stored run no longer covers it. Nothing is lost and readers agree, so either describe which edits clear it or keep the writer from clearing it if permanence is the intent. Read.ts:3558lifts the blocks only when every child of the inline element is a block. A link run that also covers inline text beside the paragraph in the same quote line (text that$inShadowRootalready shows as a paragraph of its own) still reads asparagraph(link(paragraph("q2 two") "zz")). Deleting the quote before it then still rewrites that paragraph and loses concurrent typing, as onmain. Lifting each block out of any wrapper that holds one, and keeping the inline runs in a copy of the wrapper, would cover it; the self-check rule already skips only block positions.- Optional: in
regressions/QuotesAndSlots.test.ts, add one run over a block decorator (the branch atRead.ts:3549-3550andSelfCheck.ts:230has no test), and countContentFormatitems inwrote, so the test pins what D89 settles on for the stored link.packages/lexical-y/AGENTS.mdasks tests to cover the input space. - Optional nit: the self-check exemption at
SelfCheck.ts:226-233applies to a block in any line, while the reader rule only runs for shadow roots. Also checking that the touched line is a shadow root would keep the self-check reporting a lost link elsewhere. - Optional nit: the changeset's
Verify:starts the run "from inside one shadow-root quote's paragraph". A root-sequence run can only start at that paragraph's item, so "from just before" matches what the test does.
| | D76 | **A code-token-aware trim for remade text.** A stale replace of a word can take the first letter of the word a rewrite made from it, where a node boundary cut the old word. | Writing a changed word whole fixes it, but also rewrites highlighter tokens glued to a pasted-over token in a code line. A follow-up would apply the rule everywhere except at code highlight token boundaries (D29). | `OPEN` | 10-30 | [#9608] | | ||
| | D77 | **A stale comment on a word a collaborator rewrites** lands on the new word for one client id order only. | Likely the documented race where text typed at the start of a range a collaborator formats takes that format, with a comment instead of a format. Either the docs name comments and whole-word replacements, or a fix makes both orders agree. | `OPEN` | 10-30 | [#9610], [limits] | | ||
| | D78 | **A format spreading after one client's triple undo:** a client formats a run, types plain text, formats again and undoes all three while a collaborator types plain text inside the first run. | Reproduced on main in both client id orders; no design chosen. | `OPEN` | 10-30 | [#9599] | | ||
| | D89 | **An inline element around blocks only, in a shadow root, reads as those blocks (proposed).** Another writer's link run left open over a shadow-root quote's paragraph read as `link(paragraph)` inside a paragraph of its own, a shape no editor keeps; it now reads as the paragraph, and the self-check does not count the stored link there. | A delete of the quote before it rewrote the next quote's paragraph (deleted and inserted it again), losing a collaborator's concurrent typing there ([#9571]). It extends how a shadow root already shows its content (inline runs in paragraphs of their own, `$inShadowRoot`), so it is a second case of D70's "text left without a line marker shows as a paragraph": the stored link stays and the next local edit there does not remove it. `@lexical/y`'s own writers leave no such runs. For Gerard against D70. | `OPEN` | 10-30 | [#9571] | |
There was a problem hiding this comment.
"the next local edit there does not remove it": a delete or typing inside the paragraph leaves the stored link, but Enter at the end of the second quote's paragraph (or inserting a paragraph after it) writes w:link=null before that paragraph, so the stored run no longer covers it. Worth describing which edits clear it, or keeping the writer from clearing it if permanence is the intent.
| $isElementNode(c) && c.isInline() && c.getChildrenSize() > 0 | ||
| ? c.getChildren() | ||
| : null; | ||
| for (const node of held !== null && held.every(isBlock) ? held : [c]) { |
There was a problem hiding this comment.
held.every(isBlock) leaves a wrapper over a block plus inline text (an inline run beside the paragraph in the same quote line) as before: it still reads paragraph(link(paragraph("q2 two") "zz")), and deleting the quote before it still rewrites the paragraph and loses concurrent typing. Lifting each block out of any wrapper that holds one, with the inline runs kept in a copy of the wrapper, would cover that case.
| if ( | ||
| k.startsWith(WRAPPER_PREFIX) && | ||
| special !== undefined && | ||
| special.kind === 'inline' && | ||
| ($isElementNode(special.node) || $isDecoratorNode(special.node)) && | ||
| !special.node.isInline() && | ||
| a[k] === undefined | ||
| ) { |
There was a problem hiding this comment.
Nit: this skips a wrapper key at a block in any line, while the reader drops the wrapper only in $inShadowRoot. Also requiring the touched line to be a shadow root would keep the self-check reporting a lost link elsewhere.
bc36e8a to
a484d8b
Compare
There was a problem hiding this comment.
Thanks for this. It fixes #9571 as reported: with another writer's link run over both shadow-root quotes, deleting the first quote now writes only that quote's text, and a collaborator's concurrent typing in the second quote survives on every client and on a fresh reader. The S4-8 cases fail on main and pass here, and the new cells cover nested links, inline text beside the paragraph, every catalog shape, and local edits with undo and redo.
Decision needed before this lands. packages/lexical-y/DECISIONS.md records D89 as OPEN against D70, which is DECIDED (no virtual structure beyond one existing case). packages/lexical-y/AGENTS.md lets a pull request bring new evidence against a decided row, and #9571 is that evidence. Can a maintainer confirm D89, so it lands as DECIDED? One fact for that call: the stored link is never shown over a shadow root's blocks, while the same run still shows as a link on ordinary lines (a run from the title to the end paragraph shows on "tle" and "end" and disappears across the quotes).
Suggestions:
- D89's text (
packages/lexical-y/DECISIONS.md:139) and the description say no local edit removes the stored link, but one does. A delete next to the quote, or typing inside its paragraph, leaves it. Enter in the second quote's paragraph, or appending a paragraph to that quote, writesw:link=nullbefore that paragraph, so the stored run no longer covers it. Nothing is lost and every reader agrees, so either say which edits close the run, or keep the writer from closing it if permanence is the intent. Review9654OpenChainCost.test.ts:108cannot fail on this head. It counts reads ofsrcon theopenframes, and$splitAroundBlocksno longer reads that field, so the count is 0 at every depth and the bound always holds. CountinggetChildrenvisits as the other cost tests do (or$copyNodecalls), and asserting the first count is above 0, would keep it a real guard.- Optional: count
ContentFormatitems in the S4-8wrotetally, or add one Enter cell that asserts the stored format items, so the tests pin what D89 settles on for the stored link. - Optional nit: the changeset's
Verify:paragraph starts the run "from inside one shadow-root quote's paragraph". A root-sequence run can only start at that paragraph's item, so "from just before" matches what the tests do (CONTRIBUTING.md asksVerify:to say what to test).
| | D76 | **A code-token-aware trim for remade text.** A stale replace of a word can take the first letter of the word a rewrite made from it, where a node boundary cut the old word. | Writing a changed word whole fixes it, but also rewrites highlighter tokens glued to a pasted-over token in a code line. A follow-up would apply the rule everywhere except at code highlight token boundaries (D29). | `OPEN` | 10-30 | [#9608] | | ||
| | D77 | **A stale comment on a word a collaborator rewrites** lands on the new word for one client id order only. | Likely the documented race where text typed at the start of a range a collaborator formats takes that format, with a comment instead of a format. Either the docs name comments and whole-word replacements, or a fix makes both orders agree. | `OPEN` | 10-30 | [#9610], [limits] | | ||
| | D78 | **A format spreading after one client's triple undo:** a client formats a run, types plain text, formats again and undoes all three while a collaborator types plain text inside the first run. | Reproduced on main in both client id orders; no design chosen. | `OPEN` | 10-30 | [#9599] | | ||
| | D89 | **Inline elements around blocks, in a shadow root, read as the blocks out of them (proposed).** Another writer's link run left open over a shadow-root quote's paragraph read as `link(paragraph)` inside a paragraph of its own, a shape no editor keeps; now the paragraph shows on its own, at any depth of such elements, and inline content they also hold stays in them, in a paragraph of its own. The write self-check does not count a stored wrapper over a block in a shadow root (only there). | A delete of the quote before it rewrote the next quote's paragraph (deleted and inserted it again), losing a collaborator's concurrent typing there ([#9571]). It extends how a shadow root already shows its content (inline runs in paragraphs of their own, `$inShadowRoot`), so it is a second case of D70's "text left without a line marker shows as a paragraph": the stored link stays and the next local edit there does not remove it. `@lexical/y`'s own writers leave no such runs. For Gerard against D70. | `OPEN` | 10-30 | [#9571] | |
There was a problem hiding this comment.
This row says "the next local edit there does not remove" the stored link, but Enter in the second quote's paragraph (or appending a paragraph to that quote) writes w:link=null before that paragraph, so the stored run no longer covers it. A delete next to the quote and typing inside its paragraph do leave it. Worth saying which edits close the run, or keeping the writer from closing it if permanence is the intent.
| } | ||
| counts.push(reads); | ||
| } | ||
| expect(counts[3]).toBeLessThan(counts[2] * 2.5 + 128); |
There was a problem hiding this comment.
On this head reads is 0 at every depth: $splitAroundBlocks never reads src from an open frame after pushing it, so this bound always holds. Counting getChildren visits as the other cost tests do (or $copyNode calls), plus expect(counts[0]).toBeGreaterThan(0), would keep it a real guard.
|
|
||
| [fix] A link run another writer left open over a shadow-root quote's paragraph no longer makes a delete of the quote before it rewrite that paragraph | ||
|
|
||
| Verify: with a plain Yjs client, format the root sequence with a link from inside one shadow-root quote's paragraph to past the next quote's paragraph; delete the first quote while a collaborator types in the second: the collaborator's text stays, and every client shows the quotes' paragraphs as paragraphs. |
There was a problem hiding this comment.
Optional nit: a root-sequence run can only start at the quote paragraph's item, not inside the paragraph, so "from just before one shadow-root quote's paragraph" matches what the tests do.
There was a problem hiding this comment.
Thanks for the thorough fix for #9571. The reader change reads correct to me: with another Yjs writer's link run left open over two shadow-root quotes, every client now shows quote(paragraph("q2 two")), and deleting the first quote writes only that quote's text. The S4-8 cells that were expected failures on main are plain passing tests here, and the new cases assert both peers, a fresh read and what the delete wrote.
Open decision for maintainers
packages/lexical-y/DECISIONS.md D89 is OPEN and adds a second shown-but-not-stored case next to D70 (DECIDED: "No virtual structure … The one existing case stays"). packages/lexical-y/AGENTS.md asks that a DECIDED row be reopened only with new evidence, which #9571's repro provides. Maintainers: do you accept D89 as a D70 exception, so the row can be marked DECIDED before merge? Under it, the stored link is not shown inside the quote (it still shows on the ordinary lines the same run covers), the read writes nothing, and the next local write of that quote's content stores w:link=null before what it writes and restores the link after it.
Suggestions (non-blocking)
packages/lexical-y/src/internal/Read.ts:3635: the$copyNodecopy made for a wrapper's second inline run (link text both before and after the quote's paragraph) is reached only by the read-onlyReview9654OpenChainCosttest.packages/lexical-y/AGENTS.mdasks to test the input space; amixed-bothvariant inReview9654Fixtures.tsrun throughReview9654ReadSafetyandReview9654Lifetimewould pin edits and races next to the copy.Review9654Fixtures.ts:33-34,:46-47,:163-164: thep,deepandq2aliases are therules-of-lexicalautofix's compatibility exports for renamed functions. Nothing depends on them in a new file, so the tests can import$p,$deepand$q2directly and the aliases can go.- The nine
Review9654*test files are named after this pull request, while the other files inregressions/are named by behaviour (QuotesAndSlots,Joins,Moves); a behaviour name such asShadowRootFormatWrappers*will read better after merge. - The self-check report for text inside the run next to the paragraph is pinned and tracked in #9656, which is fine as a follow-up.
| const target = (): LexicalNode[] => { | ||
| for (let d = open.length; d < chain.length; d++) { | ||
| const src = chain[d]; | ||
| const node = used.has(src.getKey()) ? $copyNode(src) : src; |
There was a problem hiding this comment.
Optional: this copy (a wrapper's second inline run, e.g. link text both before and after the quote's paragraph) is reached only by the read-only Review9654OpenChainCost test. A mixed-both variant in Review9654Fixtures.ts, run through Review9654ReadSafety and Review9654Lifetime, would pin typing, Enter and races next to the copy (packages/lexical-y/AGENTS.md: test the input space).
| /** @deprecated renamed to {@link $p} by @lexical/eslint-plugin rules-of-lexical */ | ||
| export const p = $p; |
There was a problem hiding this comment.
Nit: these @deprecated aliases are what the rules-of-lexical autofix adds to keep a renamed export working; in a new file nothing depends on p, deep or q2, so the tests can import $p, $deep and $q2 and the three aliases can go.
…shadow root, as those blocks Closes #9571. A writer other than `@lexical/y` (a plain Yjs client that does not clean up formats) can leave a link run open over the root sequence, from before a shadow-root quote's paragraph to after the next quote's paragraph. The reader put that link around each quote's paragraph, and since a shadow root holds blocks only, wrapped the link in a paragraph of its own: `quote(paragraph(link(paragraph("q2 two"))))`. A client that then deleted the first quote rewrote the second quote's paragraph (deleted it and inserted it again), so a collaborator's concurrent typing there was lost and its delete undone. A shadow root's content now reads an inline element that holds blocks only as those blocks (`$inShadowRoot`): the quotes show `quote(paragraph("q2 two"))`, and the delete deletes the first quote's text only. The write self-check does not count a stored wrapper over a block in a shadow root, which the reader does not show. The stored link stays; nothing is written to remove it. This extends how a shadow root already shows its content (inline runs in paragraphs of their own), so it is a second case of D70's exception: recorded as DECISIONS.md D88, `OPEN` for review (the number may shift at landing). `@lexical/y`'s own writers leave no such runs. `regressions/QuotesAndSlots.test.ts`, S4-8: an outside link run from the title or the first quote's paragraph to the end of the second quote's, or inside the first quote only (control); then a client deletes the first quote while a collaborator does nothing, empties the second quote or types in it. Each checks that the delete writes only the first quote's text, that every reader shows the quotes' paragraphs as paragraphs, and the text of the second quote on every client and a fresh read. The 6 runs that reach the second quote were pinned as failing; they pass now. Mutations: without the reader rule, all 9 fail (the new "shown as paragraphs" check included); without the self-check rule, the 6 fail on a self-check warning. main (2e2acb7), the S4-8 cells as plain tests: ``` Tests 6 failed | 3 passed (9) × S4-8 run: title to second quote end; peer: nothing: only the first quote's text is deleted expected { del: 'q1 oneq2 two', ins: 'q2 two' } to deeply equal { del: 'q1 one', ins: '' } ``` ``` $ vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/QuotesAndSlots.test.ts Tests 9 passed (9) ``` Unit tests of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,125 passed, 182 expected fail, 0 failed. `pnpm run ci-check` passes. Soak against main with the same seeds: L1B (mix profile) 300 sessions and L2 (mix and code profiles) 150 each: every session on the same op path, identical flag classes, none only on this branch.
## Description D88 is taken by another open draft. Number may shift at landing. ## Test plan Docs only.
… and only out of format wrappers ## Description Review of the first commit found three gaps: - The reader rule handled one wrapper around blocks only: two nested link runs, or a link that also holds text before or after the quote's paragraph, still lost a collaborator's typing. A format wrapper that holds a block, at any depth of such wrappers, is now taken apart: the blocks show on their own, in order, and inline content the wrapper also holds stays in it (itself around the first run, a copy around each later one), in a paragraph of its own. - The rule also unwrapped an inline element stored as a node of its own (with a slot): the node and its slot were dropped on read. It now applies only to wrappers the read makes from format items. - The self-check's exemption applied outside shadow roots too, and its per-run de-duplication skipped an inline node after a block: a writer that failed to remove a visible link there passed. The exemption now applies to a block whose parent is a shadow root only, and such a block is checked apart from what follows it. With text inside the link, the write self-check still reports the quote's line, as it does not model the paragraph a shadow root shows around inline content; what clients show and store is right. Pinned as that report, #9656. ## Test plan - `Review9654ReadSafety.test.ts` (none, one link, nested links, link with text before or after the paragraph; gc on/off; id orders; causal and sender delivery; collaborator does nothing, types, empties): 120 pass; on the previous head 72 failed. - `Review9654OwnInline.test.ts`: an inline node with a paragraph and a slot keeps both through the read and an unrelated delete, 4/4 (the previous head dropped it in all 4). - `Review9654SelfCheck.test.ts`: a missing visible link is reported for inline-only, block-only, block-then-inline and inline-then-block content outside a shadow root, 4/4 (previous head: 2 missed). Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,253 passed, 182 expected fail, 0 failed. `pnpm run ci-check` passes.
…s a block ## Description Taking blocks out of nested format wrappers asked, at each level, whether each wrapper below held a block, so the cost grew with the square of the nesting depth (128 nested link types: 8,393 child visits). Whether a wrapper holds a block is now kept per read, so each wrapper's children are looked at once. Also adds the review's coverage for #9571: local edits and undo after the paragraph shows out of the link (and the outside writer adding linked text next to it, which no longer brings the link back), every catalog shape as a quote's block, the unchanged reading outside shadow roots, and the cost of one wide wrapper. ## Test plan - `Review9654DepthCost.test.ts`: child visits for 8 to 128 nested wrappers grow linearly (fails on the previous head: 8,393 at 128). - `Review9654Lifetime.test.ts` 136, `Review9654Catalog.test.ts` 244, `Review9654Outside.test.ts` 4, `Review9654Cost.test.ts` 1: pass. Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,639 passed, 182 expected fail, 0 failed. `pnpm run ci-check` passes.
## Description Taking blocks out of nested format wrappers copied and rechecked the blocks found below at every level: n wrappers around n paragraphs cost n squared isInline calls (64: 4,163). One walk now pushes each block once to the result and each run of inline content to the wrappers open at its depth (opening, or copying, the chain only after a block closed it). ## Test plan `Review9654DepthWidthCost.test.ts`: 8 to 64 wrappers around as many paragraphs: linear (fails on the previous head: 4,163 at 64). All Review9654 suites and QuotesAndSlots pass. Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 24,640 passed, 182 expected fail, 0 failed. `pnpm run ci-check` passes.
## Description Each descent into a format wrapper compared the open wrappers with the walked ones from the outermost again: inline text at every depth cost the square of the depth. The open wrappers are now kept a prefix of the walked ones (a block empties them, leaving a wrapper shortens them), so only the wrappers past them are opened. ## Test plan `Review9654OpenChainCost.test.ts`: depth 8 to 64 with inline text at every depth and one innermost block: linear (fails on the previous head: 2,079 at 64). All Review9654 suites and QuotesAndSlots pass. Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 25,047 passed, 418 expected fail, 0 failed. `pnpm run ci-check` passes.
…and count a real cost in the open-chain test ## Description - D89: the read writes nothing; the stored link stays until a local edit writes that quote's content again (Enter in its paragraph, a paragraph appended to it), which stores a null of the link before what it writes and closes the run there. Nothing is lost and every reader shows the same. No behaviour change. - `Review9654OpenChainCost.test.ts` counted reads of a field the walk no longer has, so it could not fail; it now counts children enumerated during the read (and checks that count is not zero). - `Review9654EnterClosesRun.test.ts`: Enter in the second quote's paragraph stores a null of the link from the local client before it; every client shows the text with no link, as a fresh read does (gc on and off). - Changeset: the link run starts just before the paragraph. ## Test plan Units of lexical-y, lexical-mark, lexical, lexical-selection, lexical-list, lexical-table, lexical-html, lexical-extension and lexical-playground: 25,049 passed, 418 expected fail, 0 failed. `pnpm run ci-check` passes.
4539967 to
1cdc66b
Compare
There was a problem hiding this comment.
Thanks for the fix for #9571. The reader change does what the description says: with another Yjs writer's link run left open over two shadow-root quotes, every client shows quote(paragraph("q2 two")), and deleting the first quote writes only that quote's text. The S4-8 cells that were expected failures on main now pass as plain tests.
Suggestions
- Duplicate block id when the run holds text on both sides of the paragraph (
packages/lexical-y/src/internal/Read.ts:3566-3568). With block ids on, a link run holdingAAAbefore andBBBafter the second quote's paragraph showsparagraph(link("AAA ")),paragraph("q2 two")andparagraph(link(" BBB")), and both shown paragraphs get the same id, on every client and in a fresh read. Every run$splitAroundBlocksmakes from one top child takes that child's item asfirst, soBlockIdSettlerderives one id for both. That breaks its own invariant that no two blocks show one id (packages/lexical-y/src/internal/BlockIds.ts:780-785). Giving each run the item its first node was read from would fix it. Onmainthis shape kept unique ids, though it had the #9571 problems. - Coverage for that path (
Review9654Fixtures.ts:146-155).packages/lexical-y/AGENTS.mdasks tests to cover the input space, and no fixture variant holds link text on both sides of the paragraph, which is the path that makes a$copyNodecopy (Read.ts:3635). Amixed-bothvariant run throughReview9654ReadSafetyandReview9654Lifetime, with block ids on (import '../utils/blockIdsEnv', asShadowRootQuoteIds.test.tsdoes) and an assertion that ids are unique on both peers, would pin it. - Nits: the
Review9654*files are named after this pull request, while the rest ofregressions/is named by behaviour (QuotesAndSlots,Joins,Moves).Review9654Fixtures.ts:33-34,:46-47and:163-164keep@deprecatedp,deepandq2aliases that the new tests import; importing$p,$deepand$q2lets the aliases go. D89's row could also say that the local write stores the link again after the quote's paragraphs, soendkeeps it. - The self-check report for text inside the run next to the paragraph is pinned and tracked in #9656, which is fine as a follow-up.
Open decision for maintainers
packages/lexical-y/DECISIONS.md D89 is OPEN and adds a second shown-but-not-stored case next to D70 (DECIDED: "No virtual structure … The one existing case stays"). packages/lexical-y/AGENTS.md says a DECIDED row is reopened only with new evidence, which #9571's repro provides. Maintainers: do you accept D89 as an exception to D70, so the row can be marked DECIDED before merge? Under D89, the stored link is not shown inside the quote (it still shows on the ordinary lines the run covers), the read writes nothing, and the next local write of that quote's content stores w:link=null before what it writes and the link again after it.
| out.push(node); | ||
| } else { | ||
| if (run.length === 0) { | ||
| first = items[i]; |
There was a problem hiding this comment.
Every run that $splitAroundBlocks makes from one top child gets that child's item here, so with block ids on, a link holding text both before and after the quote's paragraph shows two paragraphs (paragraph(link("AAA ")), paragraph(link(" BBB"))) with the same derived id on every client. That breaks BlockIdSettler's invariant that no two blocks show one id (BlockIds.ts:780-785). Could each run take the item its first node was read from instead?
| }; | ||
| if (variant === 'nested') attrs['w:auditlink'] = {url: 'https://n.example'}; | ||
| if (variant !== 'none') root.format(a.at, b.at + 1 - a.at, attrs); | ||
| if (variant.startsWith('mixed')) |
There was a problem hiding this comment.
Optional: no variant puts link text on both sides of the paragraph, which is the path that makes a $copyNode copy in $splitAroundBlocks. A mixed-both variant (text before and after), run with block ids on (import '../utils/blockIdsEnv') and an assertion that ids are unique on both peers, would cover it.
Description
Closes #9571.
A writer other than
@lexical/y(a plain Yjs client that does not clean upformats) can leave a link run open over the root sequence, from before a
shadow-root quote's paragraph to after the next quote's paragraph. The reader
put that link around each quote's paragraph, and since a shadow root holds
blocks only, wrapped the link in a paragraph of its own:
quote(paragraph(link(paragraph("q2 two")))). A client that then deleted thefirst quote rewrote the second quote's paragraph (deleted it and inserted it
again), so a collaborator's concurrent typing there was lost and its delete
undone.
A shadow root's content now takes blocks out of the format wrappers (link
runs the read makes from format items, at any depth; never an inline element
stored as a node of its own) that hold them (
$inShadowRoot); inline contentsuch a wrapper also holds stays in it, in a paragraph of its own: the quotes show
quote(paragraph("q2 two")), and the delete deletes the first quote's textonly. The write self-check does not count a stored wrapper over a block whose
parent is a shadow root (only there), which the reader does not show. With
text inside the link, the self-check still reports the quote's line (it does
not model the paragraph a shadow root shows around inline content, #9656);
what clients show and store is right. The read writes nothing: the stored link stays until a local edit writes
that quote's content again (Enter in its paragraph, a paragraph appended to
it), which stores a null of the link before what it writes and closes the
run there; nothing is lost and every reader shows the same
(
Review9654EnterClosesRun.test.ts). This extends how a shadow root already shows itscontent (inline runs in paragraphs of their own), so it is a second case of
D70's exception: recorded as DECISIONS.md D89,
OPENfor review (the numbermay shift at landing).
@lexical/y's own writers leave no such runs.Changeset
.changeset/y-shadow-root-link-over-blocks.md(@lexical/ypatch,[fix]with aVerify:paragraph).Decision for review
DECISIONS.md D89 (
OPEN, number may shift at landing): the reader shows a shadow root's blocks without an inline element another writer left around them, a second case of D70's exception (shown, not stored).Test plan
regressions/QuotesAndSlots.test.ts, S4-8: an outside link run from thetitle or the first quote's paragraph to the end of the second quote's, or
inside the first quote only (control); then a client deletes the first
quote while a collaborator does nothing, empties the second quote or types
in it. Each checks that the delete writes only the first quote's text, that
every reader shows the quotes' paragraphs as paragraphs, and the text of the
second quote on every client and a fresh read. The 6 runs that reach the
second quote were pinned as failing; they pass now.
Mutations: without the reader rule, all 9 fail (the new "shown as
paragraphs" check included); without the self-check rule, the 6 fail on a
self-check warning.
Review9654ReadSafety.test.ts(one link, nested links, link with text before or after the paragraph; gc, id orders, delivery, collaborator edit): 120 pass.Review9654OwnInline.test.ts: an inline node with a paragraph and a slot keeps both, 4/4.Review9654SelfCheck.test.ts: a missing visible link outside a shadow root is still reported, 4/4.Review9654Lifetime.test.ts(local edits, moves, undo/redo, and the outside writer adding linked text next to the paragraph) 136,Review9654Catalog.test.ts(every catalog shape as the quote's block) 244,Review9654Outside.test.ts4;Review9654DepthCost.test.ts,Review9654DepthWidthCost.test.ts,Review9654OpenChainCost.test.tsandReview9654Cost.test.ts: linear cost in nesting depth, in depth and width together, with inline text at every depth, and in wrapper width (one walk takes the wrappers apart).Before
main (2e2acb7), the S4-8 cells as plain tests:
After
Unit tests of lexical-y, lexical-mark, lexical, lexical-selection,
lexical-list, lexical-table, lexical-html, lexical-extension and
lexical-playground: 24,125 passed, 182 expected fail, 0 failed.
pnpm run ci-checkpasses.Soak against main with the same seeds: L1B (mix profile) 300 sessions and
L2 (mix and code profiles) 150 each: every session on the same op path,
identical flag classes, none only on this branch.