fix: defer selection validation when the DOM hasn't caught up to the model#2581
Open
christianhg wants to merge 1 commit into
Open
fix: defer selection validation when the DOM hasn't caught up to the model#2581christianhg wants to merge 1 commit into
christianhg wants to merge 1 commit into
Conversation
…model When a behavior runs an action set that synchronously mutates structure (insert + delete a sibling, replace a block), the editor's MutationObserver fires while React is still mid-commit. The selection validation machine then runs against a model that's ahead of the DOM, `toDOMRange` throws, and the catch path used to deselect and re-select the top of the document, clobbering any selection the action set placed. The validation machine now treats the throw as "DOM hasn't caught up yet" rather than "selection is invalid." It re-fires itself in a microtask, giving React time to commit. After three failed retries it stops, leaving the selection alone instead of forcing it to the top of the document. Consumer behaviors that previously had to pre-emptively `select(null)` and `queueMicrotask(re-select)` to work around this can drop the workaround.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9d52651 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (ba16137) |
|---|---|---|
| Internal (raw) | 722.7 KB | +172 B, +0.0% |
| Internal (gzip) | 138.5 KB | +80 B, +0.1% |
| Bundled (raw) | 1.32 MB | +172 B, +0.0% |
| Bundled (gzip) | 298.7 KB | +79 B, +0.0% |
| Import time | 95ms | -1ms, -0.5% |
@portabletext/editor/behaviors
| Metric | Value | vs main (ba16137) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | -0ms, -1.1% |
@portabletext/editor/plugins
| Metric | Value | vs main (ba16137) |
|---|---|---|
| Internal (raw) | 3.1 KB | - |
| Internal (gzip) | 967 B | - |
| Bundled (raw) | 2.9 KB | - |
| Bundled (gzip) | 899 B | - |
| Import time | 8ms | -0ms, -1.8% |
@portabletext/editor/selectors
| Metric | Value | vs main (ba16137) |
|---|---|---|
| Internal (raw) | 76.0 KB | - |
| Internal (gzip) | 13.8 KB | - |
| Bundled (raw) | 71.3 KB | - |
| Bundled (gzip) | 12.7 KB | - |
| Import time | 7ms | -0ms, -2.0% |
@portabletext/editor/utils
| Metric | Value | vs main (ba16137) |
|---|---|---|
| Internal (raw) | 27.8 KB | - |
| Internal (gzip) | 5.5 KB | - |
| Bundled (raw) | 25.4 KB | - |
| Bundled (gzip) | 5.1 KB | - |
| Import time | 6ms | -0ms, -0.1% |
🗺️ . · ./behaviors · ./plugins · ./selectors · ./utils · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
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
1 participant
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.
When Canvas opened sanity-io/canvas#1243 they hit the editor's
validate-selection-machinecatch path and worked around it on their side: pre-emptively clearing selection withselect({at: null}), then re-selecting the new block in aqueueMicrotaskafter their action set commits. The comment (discussion) calls it "a peculiar PTE behaviour where it will select the start of a document if it's unable to validate a selection." That's exactly what the editor was doing, and it's not the right shape.What was happening
validate-selection-machine.tsruns whenever the editable'sMutationObserverfires. It compares the editor's model selection against whattoDOMRangeproduces from the live DOM. When the model and DOM are in sync, the function syncs the DOM selection and returns. WhentoDOMRangethrows, the catch path used to deselect and callapplySelect(start(editor, []))— replacing whatever selection the consumer set with a forced cursor at the top of the document.Action sets that synchronously mutate structure (insert a new block, then delete the original) trip this. The model has both operations applied; the DOM still shows the pre-action structure because React hasn't committed.
toDOMRangewalks the model selection's path, can't find the matching DOM nodes, throws. The catch fires and the consumer's intended selection is gone.What this changes
The catch now treats a
toDOMRangethrow as "DOM hasn't caught up yet" instead of "selection is invalid."validateSelectionreturns'retry'; the machine action re-fires the validation event in aqueueMicrotask, giving React a tick to commit. On the next attempt the DOM matches the model, validation succeeds, and the selection survives intact.If validation throws three times in a row the machine stops retrying. It leaves the selection alone — no forced deselect, no top-of-document fallback. If the selection is genuinely broken at that point, the next user interaction will trigger fresh DOM events and a fresh validation cycle.
On testing
The catch only fires under real React render timing, where the model commits ahead of the DOM. In
vitest-browserthe operation queue,MutationObserver, and React commit interleave synchronously enough that the catch never fires for any scenario I could construct, including the exact action-set shape Canvas uses. The pinning test intests/validate-selection-action-set.test.tsxverifies the action-set contract (selection lands on the new block) and passes on both pre-fix and post-fix code; it documents the desired behavior but doesn't discriminate the bug.The static analysis is solid (the catch is unreachable in current tests, the retry path is straightforward), the existing
validate-selection-machine.test.tsstill passes, and the verification of the actual symptom can be done by removing Canvas's workaround and re-testing in their environment.