fix: emit item-keyed markDefs patches instead of whole-array sets#2982
Draft
christianhg wants to merge 3 commits into
Draft
fix: emit item-keyed markDefs patches instead of whole-array sets#2982christianhg wants to merge 3 commits into
markDefs patches instead of whole-array sets#2982christianhg wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 6301a18 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 806.6 KB | +2.6 KB, +0.3% |
| Internal (gzip) | 154.8 KB | +525 B, +0.3% |
| Bundled (raw) | 1.42 MB | +2.6 KB, +0.2% |
| Bundled (gzip) | 319.1 KB | +520 B, +0.2% |
| Import time | 95ms | -3ms, -2.9% |
@portabletext/editor/behaviors
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | -0ms, -0.5% |
@portabletext/editor/plugins
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | -0ms, -1.0% |
@portabletext/editor/selectors
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 81.8 KB | - |
| Internal (gzip) | 15.2 KB | - |
| Bundled (raw) | 77.6 KB | - |
| Bundled (gzip) | 14.1 KB | - |
| Import time | 8ms | -0ms, -0.9% |
@portabletext/editor/traversal
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 28.1 KB | - |
| Internal (gzip) | 5.6 KB | - |
| Bundled (raw) | 28.1 KB | - |
| Bundled (gzip) | 5.5 KB | - |
| Import time | 6ms | -0ms, -0.1% |
@portabletext/editor/utils
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 29.7 KB | - |
| Internal (gzip) | 6.2 KB | - |
| Bundled (raw) | 27.4 KB | - |
| Bundled (gzip) | 5.9 KB | - |
| Import time | 6ms | +0ms, +0.2% |
🗺️ . · ./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 (04b79737)
| Metric | Value | vs main (04b7973) |
|---|---|---|
| Internal (raw) | 53.8 KB | - |
| Internal (gzip) | 9.8 KB | - |
| Bundled (raw) | 348.9 KB | - |
| Bundled (gzip) | 96.3 KB | - |
| Import time | 38ms | +1ms, +1.5% |
🗺️ 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.
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 20, 2026 15:18
52e7eba to
e956c3b
Compare
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 20, 2026 15:25
e956c3b to
0495e57
Compare
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 20, 2026 16:01
0495e57 to
0009730
Compare
This was referenced Jul 20, 2026
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 20, 2026 16:15
0009730 to
1d51829
Compare
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 20, 2026 16:26
1d51829 to
248c9cd
Compare
The history subscriber's `isNoOp` early-return advanced `previousUndoStepId` before bailing. A no-op (an operation without an inverse, or a zero-length text operation) leading a new undo step then consumed the step boundary: the next history-affecting operation saw `currentUndoStepId === previousUndoStepId` and merged into the previous step, so a single undo reverted more than the last step's worth of changes. `previousUndoStepId` now only advances for operations that reach `createUndoSteps`, since it tracks the last operation that affected history. Relatedly, a step's pre-state selection was captured from the first history-affecting operation's `beforeSelection`. Operation implementations may park the selection on transient nodes (created and removed within the same step) before that operation applies, and undo then restored a selection that no longer resolves, dropping the cursor. The engine now snapshots its selection as `undoStepSelection` when `editor.undoStepId` is minted, and the history subscriber uses that snapshot as the step's pre-state selection when an operation opens a new step. Both defects are masked on `main` in the annotation and block-merge flows by an unconditionally applied whole-array `markDefs` set operation that reached `createUndoSteps` first, creating the boundary and capturing the selection before it moved. Emitting item-level `markDefs` operations removes that accidental mask, which is what surfaced both.
…ferenced definitions An upcoming editor change emits normalization's unused-definition prunes as item-keyed `unset` patches instead of whole-array sets. `toMergeableMarkDefsPatches` only decomposes whole-array sets, so those unsets would pass through without the referenced-keys guard: a diverged client (its span `marks` overwritten by the other client's flush, its own definition now locally unused) would prune a definition the store's spans still reference, orphaning the other client's annotation at the server. Keyed `markDefs` unsets now go through the same store-side referenced-keys check as decomposed sets and are dropped while the store's spans reference the definition. A dropped prune leaves an unused definition behind at worst; normalization in a later converged session removes it for real. Against the current editor's emission (whole-array sets only) the guard is inert; it ships ahead so a consumer upgrading the editor without upgrading this plugin is never exposed.
Every `markDefs` mutation funneled through `setNodeProperties`, which applied one `set` operation carrying the whole array, and patch generation echoed it verbatim. Under concurrency this is last writer wins at the server: client A adds an annotation, client B formats the same block, both flush `set(block.markDefs, [...])`, the loser's definition vanishes while both clients' span `marks` string references survive, stranding marks without definitions. The operation sites now carry their intent item-level instead of the patch layer reconstructing it. `annotation.add` and the block-merge path in `insert.block` apply keyed `insert` operations; the normalizer's unused-definition prune applies keyed `unset`s. The engine's sidecar routing (shipped for remote patches) applies these locally unchanged; it now also computes item-keyed inverses (a keyed `unset` for a keyed insert, a keyed re-insert for a keyed unset) so undo emits mergeable patches too instead of whole-array sets. Patch generation shrinks to mechanical representation changes: `insertNodePatch` already emits `setIfMissing` plus `insert` for any array-field insert, keyed `unset`s pass through verbatim, and `setNodePatch` keeps one translation with no operation vocabulary: normalization's ensure-`markDefs`-exists `set([], ...)` becomes `setIfMissing`, creating the property on the stored document without clobbering concurrent writers. Inserts anchor on index 0 rather than on an existing definition's key: a keyed anchor diffed against this engine's state may no longer exist at the server by application time, and `markDefs` order carries no meaning. Behavioral deltas: new definitions prepend instead of append (order tests re-pinned); whole-array `markDefs` writes through `block.set` still emit a whole-array `set` (explicit whole-property intent, and duplicate-key arrays are inexpressible item-level); dedupe normalization likewise keeps the whole-array set. Emitted patch shapes change for annotation add/remove, block merges and undo; consumers of `patch`/`mutation` events see `setIfMissing`/`insert`/keyed `unset` where they previously saw one whole-array `set`.
christianhg
force-pushed
the
fix-markdefs-item-patches
branch
from
July 21, 2026 08:30
248c9cd to
6301a18
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.
Stacks on #2983 (undo-step boundaries) and #2984 (plugin guard). The first two commits here are those PRs verbatim and disappear on rebase once they merge; review only the last commit in this PR. Both are extracted because they merge independently: #2983 fixes latent undo bugs reachable on
maintoday, and #2984 must release ahead of this PR's editor change (the plugin's published^7.xpeer range accepts the new emission, so a consumer upgrading the editor without the plugin would otherwise run the orphan race unguarded).Two clients annotating the same block concurrently silently destroy each other's work: client A adds a comment annotation, client B links a word in the same block, and whoever flushes last wins. The loser's definition vanishes from the stored document while both clients' span
marksreferences survive, leaving marks pointing at definitions that no longer exist. The concrete driver is the operational patch channel in #2977, which compensates for this outgoing shape channel-side (toMergeableMarkDefsPatches); fixing the emission in the engine pays for every multi-writer consumer at once, including two Studio users on the same block.The granularity was lost at the operation level: every
markDefsmutation funneled throughsetNodeProperties, which applies onesetoperation carrying the whole array, and patch generation echoed it verbatim. An earlier iteration of this PR fixed the symptom in the operation-to-patch layer by diffing the pre-apply value, but that reconstructs intent the operation sites had precisely and then erased, a translation layer compensating for the layer above it. The shipped shape moves the fix to the sites:annotation.addand the block-merge path ininsert.blockapply keyedinsertoperations, and the normalizer's unused-definition prune applies keyedunsets. The engine's sidecar routing (shipped in #2974 for remote patches) applies these locally unchanged, and now also computes item-keyed inverses so undo emits mergeable patches too instead of restoring whole arrays. Patch generation shrinks to mechanical representation changes, with the invariant pinned in a header comment: never diff sibling state to reconstruct intent here; if a patch needs more than the operation expresses, the operation site is emitting the wrong operation. One translation remains by necessity: normalization's ensure-markDefs-existsset([], ...)becomessetIfMissing, because no operation vocabulary carries ensure-exists intent, and a whole-array set of[]would clobber concurrent writers on documents that lack the property.Inserts anchor on index 0 rather than on an existing definition's key: a keyed anchor diffed against this engine's state may no longer exist at the server by application time (the same divergence lesson as the repair-path discussion on #2977), while index 0 always resolves and
markDefsorder carries no meaning. New definitions therefore prepend instead of append, a value-visible order change pinned by re-pinned tests inoverlapping-annotationsandunique-sibling-keys. Explicit whole-array writes throughblock.setstill emit a whole-arrayset(whole-property intent from a public API, and duplicate-key arrays, which the dedupe rule exists for, are inexpressible item-level).Removing the always-applied whole-array set unmasked two latent history bugs, fixed in their own commit. The
isNoOpearly-return in the history subscriber advancedpreviousUndoStepId, so a no-op leading a new undo step consumed the step boundary and a single undo reverted more than one step's worth of changes. And a step's pre-state selection was captured from the first history-affecting operation, by which time operation implementations may have parked the selection on transient nodes their own step removes again; the engine now snapshotsundoStepSelectionwhen the undo step ID is minted. Both were masked onmainbecause the redundantmarkDefsset reached the history subscriber first, creating the boundary and capturing the selection while it was still resolvable. Theevent.history.undo/event.history.redosuites pin the corrected behavior.The third commit extends
plugin-sdk-value's referenced-keys guard to the new patch shape. The editor's keyed prune unsets passed throughtoMergeableMarkDefsPatchesuntouched, bypassing the guard that previously protected decomposed whole-array sets, and a diverged client could prune a definition the store's spans still reference, exactly the orphaned-mark failure the collision-matrix link scenarios caught (orphan mark (no markDef)). KeyedmarkDefsunsets now go through the same store-side check; a dropped prune leaves an unused definition behind at worst, which a later converged session's normalization removes. The full collision matrix and the two-client rig pass on the new emission.The emit-to-apply round trip is pinned by the browser scenario "Emitted patches converge another editor on the same value" in
event.patch.markdefs.test.tsx, and the ensure-exists translation by thesetNodePatchunit matrix. The 12 existing tests pinning whole-array shapes are re-pinned; the remaining editor suites (857 unit, 1733 browser) and allplugin-sdk-valuesuites (192 across chromium, firefox and webkit) pass.The one deliberately unsolved residual: the prune-vs-concurrent-reference race is inherent to
marksbeing string references into a separate array, and fully closing it engine-side needs store knowledge the engine lacks. The channel-side guard covers the SDK path; deferred pruning as a general mechanism is tracked separately as a follow-up.