fix: transform points through text-preserving children replacements#2933
Draft
christianhg wants to merge 2 commits into
Draft
fix: transform points through text-preserving children replacements#2933christianhg wants to merge 2 commits into
christianhg wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 01e5f87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 (e94e676) |
|---|---|---|
| Internal (raw) | 803.4 KB | +2.7 KB, +0.3% |
| Internal (gzip) | 153.8 KB | +668 B, +0.4% |
| Bundled (raw) | 1.41 MB | +2.6 KB, +0.2% |
| Bundled (gzip) | 318.0 KB | +682 B, +0.2% |
| Import time | 96ms | +1ms, +1.1% |
@portabletext/editor/behaviors
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | +0ms, +1.7% |
@portabletext/editor/plugins
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | -0ms, -0.1% |
@portabletext/editor/selectors
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 82.1 KB | +903 B, +1.1% |
| Internal (gzip) | 15.1 KB | +228 B, +1.5% |
| Bundled (raw) | 77.5 KB | +861 B, +1.1% |
| Bundled (gzip) | 14.0 KB | +225 B, +1.6% |
| Import time | 8ms | +0ms, +0.6% |
@portabletext/editor/traversal
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 28.0 KB | - |
| Internal (gzip) | 5.6 KB | - |
| Bundled (raw) | 27.9 KB | - |
| Bundled (gzip) | 5.5 KB | - |
| Import time | 6ms | +0ms, +2.6% |
@portabletext/editor/utils
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 30.6 KB | +903 B, +3.0% |
| Internal (gzip) | 6.4 KB | +244 B, +3.8% |
| Bundled (raw) | 28.0 KB | +861 B, +3.1% |
| Bundled (gzip) | 6.1 KB | +233 B, +3.9% |
| Import time | 6ms | -0ms, -0.3% |
🗺️ . · ./behaviors · ./plugins · ./selectors · ./traversal · ./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.
📦 Bundle Stats — @portabletext/markdown
Compared against main (e94e6766)
| Metric | Value | vs main (e94e676) |
|---|---|---|
| Internal (raw) | 53.8 KB | - |
| Internal (gzip) | 9.8 KB | - |
| Bundled (raw) | 348.9 KB | - |
| Bundled (gzip) | 96.3 KB | - |
| Import time | 39ms | +2ms, +5.6% |
🗺️ View treemap · 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.
`util.block-offset.ts` resolved offsets with two inline walks over a block's children (sum the spans before a key; place an offset among spans). The point transform needs the same arithmetic but is schema-free and snapshot-free, so rather than hand-rolling semantic siblings, the walks move to `util.child-text-offset.ts` as pure functions over a children array with a span-text accessor. `spanSelectionPointToBlockOffset` and the forward branch of `blockOffsetToSpanSelectionPoint` now delegate to them; the backward branch keeps its inline walk (its boundary and inline-object handling is direction-specific and has no second consumer). Net behavior unchanged: the block-offset suite passes unmodified, and the forward-boundary convention (a boundary offset stays at the end of the earlier span) now lives in one place.
When the sync machine receives a block whose children share no keys with the local ones (a remote mark toggle that split spans without preserving keys), it replaces the children array wholesale with one `set` op. `transformPoint`'s `set` case only handled `_key` renames and `text` clamps, so points inside the replaced children kept referencing dead keys and the selection clamped to the block start. The reported symptom is the local caret jumping, but every key-addressed position flowing through the op stream was equally stranded: `rangeRefs`, `pointRefs`, remote presence selections, range decorations. `transformPoint` now remaps such points through their text offset within the node: the offset into the old children (the op's `inverse`, which the `_key` branch already reads) locates the same textual position in the new children, via the shared walks in `util.child-text-offset.ts`. The mapping is gated on the old and new children carrying identical concatenated span text, the condition that makes it lossless; a remote mark toggle preserves text by definition. Points whose span key survives the replacement keep their identity untouched; raw block-offset points and points deeper than a direct child pass through untransformed; text-changing replacements keep the previous behavior, offset mapping would be guesswork there. Spans are detected structurally (`text` being a string) because `transformPoint` is deliberately schema-free; a container's array field may also be named `children` and hold blocks, which the text requirement turns into a no-op by construction. Fixing at the transform layer rather than around the sync machine's `set` call (the approach explored in the earlier draft) means all point consumers are covered by the same code and the correctness precondition is enforced where the mapping happens. The sync machine is untouched. The browser scenario (ported from the earlier draft, verified failing before the fix) pins the end-to-end behavior along with a guard scenario for text-changing replacements; unit tests pin the mechanics, mid-span remaps, boundary offsets, inline objects, identity over offset, container-depth remaps, container blocks in `children`-named fields, raw block-offset points, and the no-inverse and foreign-block no-ops.
christianhg
force-pushed
the
transform-point-children-replacement
branch
from
941e61a to
01e5f87
Compare
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 a collaborator toggles a mark in a way that splits spans without preserving keys, the local cursor jumps to the start of the block. The scenario is #2480's (its test reproduces on today's main: caret after "foobarbaz" ends on the first new span at offset 0 after a remote bold of "bar"), and the lossy branch it identified survived the rearchitecture verbatim:
updateBlock's!hasSharedKeyscase replaces the block's children wholesale with onesetop.The general problem is that
set [block, 'children']erases node identity, andtransformPoint'ssetcase only handled_keyrenames andtextclamps, so points inside the replaced children kept referencing dead keys until downstream resolution clamped them. The visible casualty is the local caret, but everything key-addressed that flows through the op stream was equally stranded:rangeRefs,pointRefs, remote presence selections, range decorations.#2480 had the right insight, map the point through its block text offset, at the wrong layer: a save/restore around one sync-machine call site rescues only the local selection at only that call site, and applies the offset mapping unconditionally, which is only correct when the replacement preserves the block's text. This PR moves the same arithmetic into
transformPoint'ssetcase with the validity condition enforced where the mapping happens: the point remaps through its text offset only when the old children (available on the op'sinverse, which the_keybranch already reads) and the new children carry identical concatenated span text, exactly the mark-toggle case. Points whose span key survives the replacement keep their identity untouched, text-changing replacements keep the previous behavior, and spans are detected structurally (textbeing a string) becausetransformPointis deliberately schema-free. The sync machine is untouched, and every point consumer is covered by the same code.The ported browser scenario pins the end-to-end behavior (verified failing before the fix) alongside a guard scenario pinning that text-changing replacements do not get offset-mapped; seven unit tests pin the mechanics (mid-span remap, boundary offsets landing at the earlier span's end, inline objects occupying no offset, identity over offset, no-inverse and foreign-block no-ops). The eight pre-existing remote-selection scenarios pass unmodified, and the full suite is green (859 unit, 1703 chromium).
Supersedes #2480. Not addressed here, deliberately: why the producing peer emits all-new keys for a mark toggle in the first place, a well-behaved producer preserving the text-identical fragment's key would hit the fine-grained sync path and never reach the lossy branch. That is an upstream question; the editor must tolerate arbitrary valid
update valueinput regardless.Two review questions shaped the final form. "Does it use the existing utils?" surfaced that the hand-rolled offset walks were semantic siblings of
util.block-offset.ts, so a preparatoryrefactor:commit extracts the walks intoutil.child-text-offset.ts(pure functions over a children array with a span-text accessor) and both the snapshot-aware utils and the schema-free transform consume them, the boundary convention is now shared by construction instead of by discipline. "What about containers with blocks in fields calledchildren?" is live (arrayFieldis unconstrained andresolve-container-field.tsspecial-cases the name): the text requirement makes those replacements a no-op by construction, blocks carry notextof their own, and dedicated unit tests pin that, along with container-depth remaps and raw block-offset points (which EDEX-1611 documents as living in real selections) passing through untouched.