[lexical] Refactor: Cache RangeSelection.isBackward() result on the instance#8474
Merged
etrepum merged 2 commits intofacebook:mainfrom May 7, 2026
Merged
Conversation
…nstance RangeSelection.isBackward() calls Point.isBefore which, for the different-key case, builds two normalized carets and walks the tree via $comparePointCaretNext. The same answer is reached repeatedly during a single update cycle and the underlying carets stay valid for the lifetime of that anchor/focus pair. Cache the result on the RangeSelection instance, mirroring the existing _cachedNodes pattern: cache fills inside an update (gated on !isCurrentlyReadOnlyMode()) and invalidates when anchor or focus mutates via Point.set. The four direct-offset mutation sites that bypass Point.set (RangeSelection.insertText × 3 and $setTextContentWithSelection × 1) now null both _cachedNodes and _cachedIsBackward inline, which also closes a pre-existing latent gap in _cachedNodes invalidation on those paths. Closes facebook#5825
mayrang
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon,
takuyakanbr and
zurfyx
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
etrepum
reviewed
May 7, 2026
Collaborator
etrepum
left a comment
etrepum
left a comment
There was a problem hiding this comment.
I asked claude to do an audit and it found one more in-place mutation in https://github.com/mayrang/lexical/blob/b35c557a59995e880bb35fd19c970322962f3aaa/packages/lexical/src/LexicalEvents.ts#L1144
…osition workaround Per etrepum's audit, $handleInput's Firefox-only composition rewind (LexicalEvents.ts:1144) directly mutates anchor.offset, bypassing Point.set and leaving _cachedNodes / _cachedIsBackward stale. Null both caches at the mutation site to preserve the PR's invariant.
Contributor
Author
|
Yeah, my audit was too narrow — only RangeSelection.ts and LexicalUtils.ts. 8e8e7a5 nulls both caches at LexicalEvents.ts:1144. |
etrepum
approved these changes
May 7, 2026
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
2 participants
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
RangeSelection.isBackward()callsPoint.isBefore, which for the different-key case builds two normalized carets and walks the tree via$comparePointCaretNext. The same answer is reached repeatedly during a single update cycle (multiple call sites in the core and extensions readisBackwardagainst the same selection) and the underlying carets stay valid for the lifetime of that anchor/focus pair.This PR caches the result on the
RangeSelectioninstance, mirroring the existing_cachedNodespattern. The cache fills inside an update (gated by!isCurrentlyReadOnlyMode()to avoid writing to frozen state during reads) and invalidates when anchor or focus mutates viaPoint.set.What changed
RangeSelectiongains a_cachedIsBackward: boolean | nullfield, initialized tonullin the constructor and read/written inisBackward().Point.set's existing invalidation block (which already nulls_cachedNodes) also nulls_cachedIsBackwardwhen the parent is aRangeSelection.Point.setand write directly topoint.offsetto avoid settingselection.dirty = truemid-IME (RangeSelection.insertText× 3 and$setTextContentWithSelection× 1). Each now nulls_cachedNodesand_cachedIsBackwardinline so both caches stay in sync with the offset change. This also closes a pre-existing latent gap where_cachedNodescould go stale on those paths — currently masked because the IME paths typically operate on a collapsed selection, so the cached node list happens to remain correct.Backwards compatibility
isBackward()returns the same value it did before for any input. The only observable change is that a second call against the sameRangeSelectioninstance no longer re-runsPoint.isBefore. No interface or class signature changes;BaseSelectionis unchanged.Closes #5825
Test plan
pnpm vitest run --project unit— 2451 pass + 1 skipped, no regressions. New regression test inLexicalSelection.test.tsbuilds a forward selection, exercises the cache hit, mutates anchor/focus throughsetTextNodeRange(which routes throughPoint.set), and verifies invalidation + recomputation (anchor=5, focus=0 → backward). The test fails as expected when the cache logic is reverted.pnpm tscclean.pnpm lint-flow— no new warnings (_cachedIsBackwardis an internalRangeSelectionfield, not inBaseSelectionorLexical.js.flow).