fix: keep normalization fallout of remote changes local (normalization echo corruption)#2999
Closed
ryanbonial wants to merge 1 commit into
Closed
fix: keep normalization fallout of remote changes local (normalization echo corruption)#2999ryanbonial wants to merge 1 commit into
ryanbonial wants to merge 1 commit into
Conversation
When a collaborator's formatting toggle splits a span, every receiving editor's normalizer merges the resulting adjacent same-mark spans back together, and those merge operations were emitted as local patches and pushed to the server. Each receiving client pushed its own competing cleanup of the same structure (different fragment keys, overlapping dmp inserts, unsets of each other's fragments), and the interleaved merges corrupted the shared document. Field signature: bold/italic appears to work briefly, then the formatted region's text is duplicated at the end of the block with the marks gone; repeated pastes while another user types multiply. Mechanism: the suppression scopes were nested with withoutNormalizing outermost, so its exit normalize pass ran after patching was restored, and the explicit normalize after remote application was likewise unsuppressed. This draft makes withoutPatching outermost at every remote application site (remote-patches.ts and the five sync-machine sites), so remote-triggered normalization stays local. The originating client still pushes its own merge as a genuine local edit, so the server converges to the normalized form. DRAFT: this blanket suppression conflicts with the self-solving documents contract (pinned normalization-emission tests fail; see the PR description for the enumeration). The likely correct mechanism is extending the existing pendingEvents hold-and-discard machinery to remote-triggered normalization during dirty sessions instead of suppressing emission entirely; needs a design call. The new regression test pins the leak: applying remote patches that create adjacent same-mark spans must produce zero locally-emitted patches or mutations. It fails on main in all three browsers with exactly the whipsaw's two-write signature.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 806.6 KB | +9 B, +0.0% |
| Internal (gzip) | 154.9 KB | +6 B, +0.0% |
| Bundled (raw) | 1.42 MB | +9 B, +0.0% |
| Bundled (gzip) | 319.2 KB | +6 B, +0.0% |
| Import time | 95ms | +0ms, +0.1% |
@portabletext/editor/behaviors
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | +0ms, +4.8% |
@portabletext/editor/plugins
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | +0ms, +0.4% |
@portabletext/editor/selectors
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 82.7 KB | - |
| Internal (gzip) | 15.4 KB | - |
| Bundled (raw) | 78.4 KB | - |
| Bundled (gzip) | 14.3 KB | - |
| Import time | 8ms | +0ms, +0.5% |
@portabletext/editor/traversal
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 28.1 KB | - |
| Internal (gzip) | 5.6 KB | - |
| Bundled (raw) | 28.1 KB | - |
| Bundled (gzip) | 5.5 KB | - |
| Import time | 6ms | -0ms, -2.6% |
@portabletext/editor/utils
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 30.6 KB | - |
| Internal (gzip) | 6.4 KB | - |
| Bundled (raw) | 28.2 KB | - |
| Bundled (gzip) | 6.1 KB | - |
| Import time | 6ms | -0ms, -0.8% |
🗺️ . · ./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 (11d72d02)
| Metric | Value | vs main (11d72d0) |
|---|---|---|
| Internal (raw) | 53.8 KB | - |
| Internal (gzip) | 9.8 KB | - |
| Bundled (raw) | 348.9 KB | - |
| Bundled (gzip) | 96.3 KB | - |
| Import time | 36ms | -3ms, -7.1% |
🗺️ 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.
Contributor
Author
|
closing in favor of a SDK plugin fix |
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.
The bug (SDK-path concurrent corruption; not observed in Studio)
Reported on
@portabletext/plugin-sdk-value@7.1.2+@portabletext/editor@7.10.10after the two typing fixes landed: bolding/italicizing selected text "works briefly, but then the formatting is lost and all of the content from the selected text onward is duplicated and appended to the end"; repeated pastes while another user types multiply.Reproduced on demand with a two-client human-gesture driver (
duo-typing.mjs formatin sanity-labs/pte-sdk-concurrent-repro): user A keyboard-selects a word and toggles bold while user B types in another block.Head-to-head on the same editor version (
@portabletext/editor@7.10.10):sanity@6.5.0, Structure form, no SDK)plugin-sdk-value@7.1.2)…in the r,…rrant and it wants a taco estau)So this is not a latent Studio PTE bug that somehow went unreported. Studio's FormBuilder +
@sanity/mutatorpath does not reproduce the Axios format-corruption class under the same workload. The SDK value-plugin sync path does.That also fits "why after 7.1.0": the operational patch channel made fine-grained normalize echoes into real competing writes. Pre-7.1.0 whole-value LWW mostly hid them.
Harness:
studio-native.mjs format(Studio) vsduo-typing.mjs format(SDK), same seed/oracle.Root cause (from SDK-store-level traces of a corrupted trial)
Every receiving editor on the SDK path pushes its own normalization of a collaborator's formatting. The smoking gun, client B (typing only in block B) pushing edits to block A:
A's bold toggle splits a span into keyed fragments. Those arrive at B as remote patches; B's normalizer merges the adjacent same-mark spans back together (correct local hygiene), but the merge operations are emitted as local patches and the plugin pushes them. A performs the same merge with different fragment keys. The interleaved competing merges (overlapping dmp inserts, unsets of each other's fragments) corrupt the server: fragments unset after their text already moved (truncation) or text applied twice (the duplication users see).
Mechanically, the leak is scope nesting: remote application runs under
withoutPatching, butwithoutNormalizingwas outermost, and its exit normalize pass (plus the explicitnormalizeinremote-patches.ts) ran after patching was restored.Studio likely survives the same editor behavior because its BufferedDocument / mutator path buffers, squashes, and rebases pending local mutations over arriving remotes before commit, so a receiver's normalize fallout often never lands as a competing write. The SDK plugin, by design after the 7.1.1 typing fix, pushes every mutation flush immediately and has no equivalent "hold normalize echo until rebase proves it is still needed" layer.
What this draft does
Makes
withoutPatchingoutermost at every remote-application site (remote-patches.tsand the fivesync-machine.tssites), so normalization fallout of remote changes stays local. The originator still pushes its own merge as a genuine local edit, so the server converges to the normalized form.This stops the echo at the editor. It is one valid fix site, but given the Studio comparison it should be framed as protecting the SDK sync path (and any other consumer that eagerly commits every mutation), not as "Studio has this bug too."
Verification (external harness, real Content Lake, local builds)
The new regression test (
remote-patches-normalization.test.tsx) pins the leak: remote patches creating adjacent same-mark spans must produce zero locally emitted patches/mutations. It fails on main in all three browsers with exactly the two-write merge signature.Why it's a draft: the self-solving collision
Against a clean-main baseline (10 pre-existing browser-suite failures on my machine), this suppression newly breaks ~21 pinned tests, and they're not collateral: they're the self-solving documents contract (
normalization.test.tsx,self-solving.test.tsx,container-normalization.test.tsx,event.patches.test.tsxnormalization emissions, and thecollaborative-editing.test.tsxdeferred-normalization scenarios). Those tests deliberately assert that normalization fix-ups ARE emitted so broken documents heal.The existing architecture already has the right idea for the pristine case: normalization patches are held in
pendingEventsanddiscard conflicting pending patchesdrops them when a conflicting remote patch arrives. The failure mode fixed here is specifically remote-triggered normalization during an already-dirty session, which bypasses the hold and emits with the next flush.So the likely correct mechanism, rather than this blanket suppression: extend the hold-and-discard treatment to normalization patches generated while
isProcessingRemoteChanges(regardless of dirty state), so the originator's own merge ops discard them on arrival, and they still emit eventually if no one else fixes the document (self-solving preserved). That reaches into the deferral design ineditor-machine.ts, which is why I'd rather hand it over than guess.Alternative framing (plugin / SDK, not core)
Because Studio does not reproduce this, another valid fix site is
plugin-sdk-value: do not push mutations that are only the normalize fallout of a remote apply (or hold them until a mutator-style rebase shows they are still needed). That would mirror Studio's "buffer and rebase away" behavior without teaching the core editor to suppress self-solving emissions. Happy to pursue either site.The driver reproduces the corruption in about 30 seconds per trial.