fix(edit-sync): retain buffer on force-post so a stale settlement-ack post survives#226
Merged
Merged
Conversation
… post survives flush() force-posts pending bytes with the current docVersion, which is stale the instant the host has settled the in-flight Edit (write lock released, ack still in transit). Such a force-post misses the host's lock-held stash path and is stale-rejected, so the authoritative Document reposts over the typed bytes. Nulling the buffer on force-post left nothing to replay, so on an alive hide (tab switch during fast typing) the trailing keystroke was visibly erased. Retain the buffer on a successful force-post instead of nulling it: the next ack replays it at the fresh docVersion via replayIfNeeded (which nulls the buffer on its own post, so it is exactly one replay, never a loop). Double delivery is idempotent at the host via the no-op verdict on content equality. Correct the flush()/flushIfIdle comments that overstated the host-stash guarantee and only acknowledged this window for the handoff path.
…ent; pin loop invariant Review-cycle findings (Codex + error-handler CRITICAL, test-analyzer HIGH, comment-analyzer CRITICAL/MEDIUM): - Gate the force-post buffer retention on there having been an Edit in flight. Unconditional retention also fired on the accept path (no prior lock held → base matches → host accepts and durably applies): the retained already-applied bytes could later replay over a racing external edit and silently clobber it, since the webview has no client-side conflict guard for a post-settlement replay. Retention now mirrors trySend (retain only under single-flight), which is the sole path to the stale settlement→ack window the fix targets. - Correct the stale flushIfIdle comment that still described flush as force-post-then-null. - Pin the 'exactly one replay, never a loop' invariant with a second reducer commit assertion, and add a test that the not-in-flight force-post nulls the buffer. Both revert-checked non-vacuous. - Consolidate the triplicated retain rationale into the flush JSDoc.
…railing-keystroke
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.
Summary
Fix a data-loss race where the trailing keystroke typed during a tab switch (fast typing) is visibly erased.
flush()force-posts the pending bytes with the currentdocVersion, which is stale the instant the host has settled the in-flight Edit (write lock released, ack still in transit). Such a force-post misses the host's lock-held stash path and isstale-rejected, so the authoritative Document reposts over the typed bytes — and nulling the buffer on force-post left nothing to replay.Changes
src/webview/cm/edit-sync.ts—flush()now RETAINS the buffer on a successful force-post instead of nulling it. The next ack replays it at the freshdocVersionviareplayIfNeeded(which nulls the buffer on its own post → exactly one replay, never a loop). Double delivery is idempotent at the host via theno-opverdict on content equality.flush()/flushIfIdle()doc + impl comments that overstated the host-stash guarantee and only acknowledged the settlement→ack window for the handoff path.test/webview/cm-edit-sync.test.ts— new red-first test pinning flush-during-the-settlement→ack window (bytes must survive via replay); updated the existing force-post test whose assertion pinned the buggy null-the-buffer contract.Related
flush()loses the trailing keystroke in the host-settlement→ack-in-transit window (Fable full review 2026-07-17)Test Plan
pnpm compilecleanpnpm test:unitgreen (3575 passed)