Skip to content

[lexical] Bug Fix: Correct children fast-path text size for cross-parent-moved elements#8569

Merged
etrepum merged 7 commits into
facebook:mainfrom
etrepum:claude/modest-turing-NbK2O
May 27, 2026
Merged

[lexical] Bug Fix: Correct children fast-path text size for cross-parent-moved elements#8569
etrepum merged 7 commits into
facebook:mainfrom
etrepum:claude/modest-turing-NbK2O

Conversation

@etrepum

@etrepum etrepum commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Description

The children fast path's $cachedTextSize read an ElementNode's previous text size from its keyed dom.__lexicalTextContent. That DOM node is shared between the previous and next keyToDOMMaps and is mutated in place during reconciliation. When an inline element moves to a different parent and is reconciled (growing) under its new parent before its old parent's suffix walk runs, the old parent read the new, larger size as if it were the previous size and sliced too much off its cached text. The DOM stayed correct, but RootNode.__cachedText (which backs getTextContent()) silently desynced.

Recover the previous-cycle size from the previous node map for an element whose next-state parent differs from its previous-state parent. This is in the same family as the full-reconcile hazard fixed in #8564.

Test plan

New unit tests

…ent-moved elements

The children fast path's `$cachedTextSize` read an ElementNode's previous text
size from its keyed `dom.__lexicalTextContent`. That DOM node is shared between
the previous and next `keyToDOMMap`s and is mutated in place during
reconciliation. When an inline element moves to a different parent and is
reconciled (growing) under its new parent before its old parent's suffix walk
runs, the old parent read the new, larger size as if it were the previous size
and sliced too much off its cached text. The DOM stayed correct, but
`RootNode.__cachedText` (which backs `getTextContent()`) silently desynced.

Recover the previous-cycle size from the previous node map for an element whose
next-state parent differs from its previous-state parent. This is in the same
family as the full-reconcile hazard fixed in facebook#8564.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01HYqH94pEhCRhymfZsY2J6e
@vercel

vercel Bot commented May 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment May 26, 2026 10:52pm
lexical-playground Ready Ready Preview, Comment May 26, 2026 10:52pm

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 26, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label May 26, 2026
…tate

`editor.read()` flushes pending updates before reading, so it can observe a
different state than the `getRootElement().innerHTML` / `getEditorState().toJSON()`
snapshots it is compared against. Read `getTextContent()` from the same
`getEditorState()` snapshot so all three observations are consistent.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01HYqH94pEhCRhymfZsY2J6e
…ion test

The deterministic regression test built a known structure, so guarding the
move with `if ($isElementNode(...))` would let the test pass vacuously (the
move silently skipped) if that structure ever changed. Use `invariant` so the
test fails loudly instead.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01HYqH94pEhCRhymfZsY2J6e
…tate

The de-optimized cross-parent path previously hand-walked the prev node map and
called `isInline()` on previous-state child instances. For custom nodes whose
`isInline()` routes through `getLatest()`, that resolves against the next state
and throws once a child has been detached — the same hazard fixed in facebook#8548. It
also leaned on the per-instance leaf size cache, which a child changed during
the move can invalidate.

Instead, read the previous size via `prevEditorState.read(() =>
node.getTextContentSize())`. Inside that read `getChildren()` / `isInline()`
resolve against the previous tree, so we neither touch the forward-mutated DOM
cache nor hit the getLatest -> next-state trap, and the size is recomputed from
the prev tree rather than from leaf caches. `ElementNode.getTextContentSize()`
uses the same double-line-break rule as the reconciler cache, so the value
matches `__lexicalTextContent.length`.

Bump the cross-parent differential fuzzer back to a seed range that includes a
case exercising this path.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01HYqH94pEhCRhymfZsY2J6e
The known-problematic cross-parent move is covered deterministically by
FastPathCrossParentTextCache; the differential fuzzer should exist to surface
NEW divergences, not be tuned to regenerate that specific case. Revert its
seed count to a general-coverage value.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01HYqH94pEhCRhymfZsY2J6e
Computing a cross-parent-moved element's previous text size inside
`prevEditorState.read(() => node.getTextContentSize())` fixes that element's
own size, but the surrounding `oldSuffixLength` walk has a second `isInline()`
call — the one deciding the double line break BETWEEN suffix siblings — that
still runs on a previous-state node reference outside any read. That call
cannot throw (a removed sibling is always the last of the suffix, where
`isInline()` is skipped), but when a non-last suffix element's `isInline()` is
state-dependent (routes through `getLatest()`) and flips between the previous
and next states, the walk reads the next-state value, computes the wrong
double-line-break length, and silently desyncs `RootNode.__cachedText`.

Move the whole walk into a single `activePrevEditorState.read(...)`
(`$prevSuffixTextSize`) so every node method — the moved-element
`getTextContentSize()` and the inter-sibling `isInline()` — resolves against
the previous node map. Non-moved elements and leaves still read their O(1)
caches, so an untouched suffix child is not re-walked.

Adds a regression test: flipping a non-last suffix element block<->inline in a
size-0 reconcile must keep `getTextContent()` (backed by the cache) equal to a
fresh tree walk.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01VkVms36ysWg3qxKeUeQPg8
The per-child size helper (`$cachedTextSize`) had a read-context precondition:
its moved-element branch resolves `getTextContentSize()` against the active
editor state, which is only correct while the caller has swapped in the
previous state via `activePrevEditorState.read`. Documented and single-caller
today, but a future caller added outside the read would silently resolve
against the next state. Inline the logic into `$prevSuffixTextSize` so it
physically cannot be called outside the read.

While inlining, replace the unreachable `prevNode === undefined` early `break`
(which would proceed with a partial suffix length) with an `invariant`: callers
validate every suffix key is present in the previous node map, so a miss is a
broken upstream invariant — fail loudly (the reconciler catch recovers via a
full reconcile) rather than splice a wrong length. Consistent with the other
cache-miss invariants in the same walk.

No behavior change on any reachable path; the cross-parent move and the
inter-sibling `isInline()` desync regression tests still pass.

Co-authored-by: Claude <noreply@anthropic.com>

https://claude.ai/code/session_01VkVms36ysWg3qxKeUeQPg8
@etrepum etrepum added this pull request to the merge queue May 27, 2026
Merged via the queue into facebook:main with commit b732601 May 27, 2026
42 checks passed
@etrepum etrepum deleted the claude/modest-turing-NbK2O branch May 27, 2026 14:14
etrepum pushed a commit to etrepum/lexical that referenced this pull request May 27, 2026
…acebook#8569) into claude/kind-johnson-RexEG

Resolved conflicts and reconciled new upstream surface with this branch's
shared -> @lexical/internal migration and per-module __DEV__ convention:
- clipboard.ts: kept @lexical/internal/invariant; dropped $generateNodesFromDOM
  (no longer used after facebook#8528 introduced the DOMImportExtension pipeline).
- code-core / rich-text package.json + pnpm-lock: took main's dependency set
  (adds @lexical/html) and regenerated via update-version + pnpm install, which
  re-applies the @lexical/internal deps and files allowlist.
- Converted new shared/invariant imports (facebook#8528 parseCss.ts/sel.ts, facebook#8569 test)
  to @lexical/internal/invariant.
- Added per-module `const __DEV__` to facebook#8528's compileImportRules.ts / runImport.ts
  (this branch removed the global __DEV__).

Verified: tsc, tsc-test, tsc-scripts, flow, lint, prettier clean; test-unit
3373 pass.
@etrepum etrepum mentioned this pull request May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants