fix: remove the local placeholder on a remote root insert after a self-emitted unset - #3257
Merged
Merged
Conversation
…emitted `unset` After this editor's stream ends with a became-empty `unset([])`, a lagging mirrored `update value` echo can record the editor's own placeholder as `lastSyncedValue` (the same settled poisoned state the character-by-character retype scenario builds). A collaborator's block then arriving through `patches` (`setIfMissing([], [])` plus a root `insert`) must leave the engine holding exactly that block: keeping the placeholder would show a block the document does not have, and text typed into it would emit patches against a span no store holds. The scenario asserts the removal even though the recorded value equals the placeholder: while the editor's own stream has destroyed the field, a value-equal placeholder is deliberately treated as unpersisted. That pins the accepted trade; narrowing the predicate back to the `lastSyncedValue` equality alone turns this red. Red before the fix, three-for-three: the engine kept two blocks (the collaborator's block plus the phantom placeholder) against the expected one.
…elf-emitted `unset` `insertPatch` in `applyPatch.ts` decided whether the editor was empty before a remote root insert (and hence whether to remove the local placeholder after applying it) from the `lastSyncedValue` equality alone. After a character-by-character clear under a value-mirroring host, a lagging echo records the placeholder as `lastSyncedValue`, the equality reads as proof of persistence, and the placeholder survives next to the inserted block: the editor shows a block the document does not have, and edits into it emit patches no store can apply. The predicate now mirrors `editorWasEmpty` in `subscriber.patch-generation.ts`: while `valueUnsetEmitted` is set, this editor's own stream has destroyed the field, so a value-equal placeholder is treated as unpersisted and removed. The flag is deliberately not cleared by the remote write; only the editor's own emitted rebuild clears it. One accepted trade, disclosed in the changeset: a lone empty block that a host genuinely persisted inside this window is indistinguishable by value from the placeholder and is now removed when a remote root insert arrives, until the next value sync restores it. Distinguishing the two takes provenance metadata, tracked as follow-up work. Behavior is unchanged whenever `valueUnsetEmitted` is unset: pristine blocks synced in without a preceding local clear keep suppressing the removal via the existing `lastSyncedValue` equality.
🦋 Changeset detectedLatest commit: 4591bb6 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✅ No significant changes. All scenario measurements (7)🗺️
Significant means at least 1.0 KB and 1% gzip, or at least 5 ms and 10% import time. |
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.
After clearing a Portable Text field under a host that mirrors
mutation.valueback intoupdate value, a collaborator's block arriving throughpatchesleft a phantom block behind: the editor showed its local placeholder next to the inserted block while the document held only the block, and text typed into the phantom emitted patches against a span no store has, so it never persisted.The cause is the same poisoned recording #3253's sibling fix guards against in patch generation. A lagging mirror echo of the cleared state records the editor's own placeholder as the last synced value, and
insertPatch's root-insert handling decided whether to remove the placeholder from that equality alone, reading the poison as proof of persistence. The predicate now mirrors patch generation's: while this editor's own stream has unset the field (valueUnsetEmitted), a value-equal placeholder is treated as unpersisted and removed. The remote write deliberately does not clear the flag; only the editor's own emitted rebuild does.One additional change, deliberate and pinned by the test: a lone empty block that a host genuinely persisted inside this window is indistinguishable by value from the placeholder, so it is now also removed when a remote root insert arrives, until the next value sync restores it. Telling the two apart takes provenance metadata, which is follow-up work. Behavior is unchanged whenever the editor has not unset the field itself: pristine blocks synced in without a preceding local clear keep suppressing the removal through the existing equality, pinned by the existing scenarios.
Note
Medium Risk
Changes collaborative patch application and empty-editor placeholder logic; incorrect handling could drop blocks or show duplicates, but scope is narrow and covered by new tests.
Overview
Fixes a collaboration bug where, after the local editor clears the field (
unset) under a host that mirrorsmutation.valueback, a remote rootinsertcould leave a phantom empty block beside the collaborator’s content.insertPatchnow uses the sameeditorWasEmptyBeforerule as patch generation: whenvalueUnsetEmittedis set, a placeholder that still matcheslastSyncedValue(including a stale mirror echo) is treated as unpersisted, so the empty block is removed when applying the remote insert. Docs onvalueUnsetEmittedare updated to note this applies to remote root inserts too.Trade-off: a genuinely persisted empty block after a local unset may also be dropped until the next value sync; separating that from the editor placeholder needs provenance metadata (called out as follow-up). Behavior when the editor has not locally unset is unchanged.
Adds a regression test for the mirror-echo + remote insert scenario and a changeset for
@portabletext/editor.Reviewed by Cursor Bugbot for commit 4591bb6. Bugbot is set up for automated code reviews on this repo. Configure here.