fix: let the newest incoming value supersede a parked pending value - #3281
Open
christianhg wants to merge 1 commit into
Open
christianhg wants to merge 1 commit into
christianhg wants to merge 1 commit into
Conversation
The sync machine's `is new value` guard judged an incoming `update value` event only against `context.previousValue`, the last value the machine had synced. While a value sits parked in `context.pendingValue` during a busy retry, a further `update value` that happened to equal `previousValue` was discarded by this guard even though it was newer than the parked one, so the older parked value survived and synced in later. A host clearing a field back to `undefined` right after sending a stale non-empty value hit this exactly: `previousValue` was still `undefined` (the machine never advanced it for an editor that started empty), so the clearing update was silently dropped and the stale value synced in and got recorded as `lastSyncedValue`. Downstream, the patch-generation subscriber's stale-echo suppressor compares `lastSyncedValue` against the pre-edit value to decide whether to rebuild an unset field before the next edit's patches; a wrongly recorded `lastSyncedValue` suppressed that rebuild, so the next edit's patches targeted a field the document no longer had. The guard now compares against `context.pendingValue` whenever one is parked, and against `previousValue` only when nothing is parked. `pendingValue`'s type (`Array<PortableTextBlock> | undefined`) can't distinguish "parked: undefined" from "nothing parked", so a new `hasPendingValue` flag carries that distinction; every action that sets or clears `pendingValue` keeps it in sync. Pinned in `placeholder-block.test.tsx` (home of the other parked/repaired-placeholder contracts): a local edit defers mutations, a stale value parks, a newer host-truth `undefined` arrives, a remote root unset lands, and once the deferred mutation flushes and the busy retry runs, the parked value must have been superseded. Confirmed red on the pre-fix guard (the stale parked block synced in as `k2`/`k3` instead of the freshly normalized `k6`/`k7`) before restoring the fix.
🦋 Changeset detectedLatest commit: 5e3a278 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 branch was successfully deployed
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.
A newer document value arriving while an older one is still waiting to sync no longer loses to the older one. Previously, once a value was parked awaiting sync, a further incoming value was compared against the last value the editor had synced, not the parked one. A value that happened to match that older baseline (for example, the host unsetting a field back to its prior state) was dropped as "nothing new", so the stale parked value synced in instead of it. The editor could then treat that stale sync as persisted, which suppressed the usual rebuild on the next edit, leaving the edit targeting content the document no longer had.
The
is new valueguard in the sync machine judged every incomingupdate valueagainstcontext.previousValue, even while a value sat parked incontext.pendingValueduring a busy window (mutations deferred by local edits). The failure needs an editor that started empty:previousValueis stillundefined, a stale non-empty snapshot parks, and the host's newerundefined(the field was unset) compares equal topreviousValueand is discarded. The stale snapshot then syncs in and is recorded aslastSyncedValue, and patch generation's stale-echo suppressor reads that record as "this block is persisted", so the first edit after emits a keyed patch with nosetIfMissing/insertrebuild. Against a store where the field is gone, that patch and every one after it is a silent no-op.The guard now takes the parked value as its baseline whenever one exists.
pendingValue's type cannot distinguish "parked:undefined" from "nothing parked", so ahasPendingValueflag carries that distinction, kept in sync by the same actions that set and clearpendingValue.Pinned in
placeholder-block.test.tsx: a local edit defers mutations, a stale value parks, the newerundefinedarrives, a remote root unset lands, and after the deferred mutation flushes the editor must settle on a fresh placeholder rather than the stale block, with the next edit emitting the full rebuild (setIfMissing,insert, then the text patch). The test fails on the pre-fix guard: the stale parked block syncs in instead.Note
Medium Risk
Changes core value-sync deduplication during busy states; wrong baseline logic could drop or double-apply host updates, though scope is narrow and covered by a new integration test.
Overview
Fixes a race in the sync machine where a newer host value could be ignored while an older snapshot was still parked during a busy (deferred-mutation) window.
The
is new valueguard always compared incomingupdate valueevents topreviousValue, so a later update that matched the last synced baseline (e.g. the host unsetting the field toundefinedwhen the editor started empty) was treated as a no-op and never replaced the stalependingValue. That stale sync could be recorded as persisted and break the next edit’s patch rebuild.comparisonValuenow uses the parkedpendingValuewhenhasPendingValueis set; a newhasPendingValueflag distinguishes “parkedundefined” from “nothing parked,” sincependingValuealone cannot. Assign/clear pending actions keep the flag in sync.Adds a
placeholder-blockscenario test: local type while busy, stale value parks, newerundefinedsupersedes, remote unset, then the next keystroke must emitsetIfMissing,insert, and the text patch.Reviewed by Cursor Bugbot for commit 5e3a278. Bugbot is set up for automated code reviews on this repo. Configure here.