Skip to content

Commit 68acab6

Browse files
committed
fix(copilot): address review — order merges, exclude update, gate throttle, unhide stream
Review round on #6108: - Greptile P1 (durable merges lose ordering): serialize ALL merges per file on one chain in mergeEditIntoLiveFileDoc (each chains after the current tail), so concurrent durable writes can't resume-and-fire out of order. notify now exposes isLiveDocMergeInFlight. - Cursor High (update stream blanks the doc): only append/patch stream — they build on the loaded base; update is a from-scratch rewrite whose partial snapshot would diff the full doc toward a fragment, so it applies atomically at the durable write. - Cursor Medium (throttle advances on a dropped merge): the adapter gates on !isLiveDocMergeInFlight, so the send throttle advances only on an actual dispatch — no lag, no backlog behind a slow relay. - Cursor Medium (placeholder hides a live stream): show the fast-render placeholder only when not streaming, so a stream that starts before the doc seeds shows through the editor. - Soften merge.ts/notify.ts comments per the lifecycle audit: only UNTOUCHED regions are preserved; a region the merge rewrites reconciles toward copilot's content. Tests updated: notify covers chain ordering + isLiveDocMergeInFlight; adapter covers append streaming, throttle, non-markdown/base-less/update skips, and the in-flight skip.
1 parent 4226987 commit 68acab6

6 files changed

Lines changed: 148 additions & 60 deletions

File tree

apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -935,6 +935,11 @@ export function LoadedRichMarkdownEditor({
935935
[]
936936
)
937937

938+
// Show the read-only placeholder only for a plain cold open — never during an agent stream. A stream
939+
// that begins before the doc has seeded fills the (hidden) editor via Yjs, so gating the placeholder
940+
// off while streaming lets that live content show through instead of hiding it behind stale markdown.
941+
const showPlaceholder = collaborationEnabled && !collabReady && !isStreaming
942+
938943
return (
939944
<div
940945
ref={containerRef}
@@ -959,7 +964,7 @@ export function LoadedRichMarkdownEditor({
959964
if (images.length > 0) void insertImagesRef.current(images, at)
960965
}}
961966
/>
962-
{collaborationEnabled && !collabReady && placeholderHtml && (
967+
{showPlaceholder && placeholderHtml && (
963968
// Instant read-only content while the collaborative doc seeds; the editor stays mounted-but-
964969
// hidden below so it renders the seeded doc before the swap. Same layout box → no reflow.
965970
<div
@@ -971,7 +976,7 @@ export function LoadedRichMarkdownEditor({
971976
editor={editor}
972977
className={cn(
973978
'mx-auto flex w-full max-w-[48rem] flex-1 flex-col px-8 py-6 selection:bg-[var(--selection-bg)] selection:text-[var(--text-primary)] dark:selection:bg-[var(--selection-dark)] dark:selection:text-white',
974-
collaborationEnabled && !collabReady && 'hidden'
979+
showPlaceholder && placeholderHtml && 'hidden'
975980
)}
976981
/>
977982
</div>

apps/sim/lib/collab-doc/merge.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@ import { applyMarkdownToYDoc } from './converter'
1010
* and applies the returned diff, which Yjs merges with any concurrent user edits before relaying it to
1111
* every connected editor.
1212
*
13-
* `applyMarkdownToYDoc` performs a real `updateYFragment` diff (not a replace), so unrelated
14-
* paragraphs the user is editing are preserved. The returned update is relative to the document's
15-
* state at call time (`Y.encodeStateAsUpdate(doc, before)`), so it is exactly the change to apply — and
16-
* is empty (a no-op update) when `markdown` already matches the document.
13+
* `applyMarkdownToYDoc` performs a real `updateYFragment` diff (not a replace), so paragraphs the diff
14+
* does not touch are preserved even while the user edits them. A region the incoming `markdown` DOES
15+
* change is reconciled toward that markdown — a concurrent user edit inside such a region is diffed
16+
* away, since `markdown` is built from a base snapshot, not the user's in-flight text. The returned
17+
* update is relative to the document's state at call time (`Y.encodeStateAsUpdate(doc, before)`), so it
18+
* is exactly the change to apply — and is empty (a no-op update) when `markdown` already matches.
1719
*/
1820
export function buildFileDocMergeUpdate(docState: Uint8Array, markdown: string): Uint8Array {
1921
const doc = new Y.Doc()

apps/sim/lib/copilot/request/go/file-preview-adapter.test.ts

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ import {
99
MothershipStreamV1ToolPhase,
1010
} from '@/lib/copilot/generated/mothership-stream-v1'
1111

12-
const { mergeEditIntoLiveFileDocMock } = vi.hoisted(() => ({
12+
const { mergeEditIntoLiveFileDocMock, isLiveDocMergeInFlightMock } = vi.hoisted(() => ({
1313
mergeEditIntoLiveFileDocMock:
1414
vi.fn<(fileId: string, markdown: string, version?: number) => Promise<void>>(),
15+
isLiveDocMergeInFlightMock: vi.fn<(fileId: string) => boolean>(),
1516
}))
1617

1718
const { peekFileIntentMock } = vi.hoisted(() => ({
@@ -20,6 +21,7 @@ const { peekFileIntentMock } = vi.hoisted(() => ({
2021

2122
vi.mock('@/lib/realtime/notify', () => ({
2223
mergeEditIntoLiveFileDoc: mergeEditIntoLiveFileDocMock,
24+
isLiveDocMergeInFlight: isLiveDocMergeInFlightMock,
2325
}))
2426

2527
vi.mock('@/lib/copilot/tools/server/files/file-intent-store', () => ({
@@ -95,7 +97,9 @@ describe('processFilePreviewStreamEvent — live-doc streaming merge', () => {
9597
beforeEach(() => {
9698
vi.clearAllMocks()
9799
mergeEditIntoLiveFileDocMock.mockResolvedValue(undefined)
98-
peekFileIntentMock.mockResolvedValue(undefined)
100+
isLiveDocMergeInFlightMock.mockReturnValue(false)
101+
// Default: an append/patch base is available (a non-empty file), so the base-present gate passes.
102+
peekFileIntentMock.mockResolvedValue({ existingContent: 'Base.' })
99103
state = createFilePreviewAdapterState()
100104
nowMs = 1_000_000
101105
vi.spyOn(Date, 'now').mockImplementation(() => nowMs)
@@ -120,7 +124,7 @@ describe('processFilePreviewStreamEvent — live-doc streaming merge', () => {
120124
}
121125

122126
it('merges the growing full content (no version arg) into the live doc as it streams', async () => {
123-
const intent = makeIntent({ operation: 'update', fileId: 'file-grow', fileName: 'notes.md' })
127+
const intent = makeIntent({ operation: 'append', fileId: 'file-grow', fileName: 'notes.md' })
124128

125129
await drive(editContentDelta('{"content":"Hello'), intent)
126130
await flushMicrotasks()
@@ -131,17 +135,21 @@ describe('processFilePreviewStreamEvent — live-doc streaming merge', () => {
131135
await flushMicrotasks()
132136

133137
expect(mergeEditIntoLiveFileDocMock).toHaveBeenCalledTimes(2)
134-
expect(mergeEditIntoLiveFileDocMock.mock.calls[0]).toEqual(['file-grow', 'Hello'])
135-
// The second merge carries the GROWN full text, never a diff.
136-
expect(mergeEditIntoLiveFileDocMock.mock.calls[1]).toEqual(['file-grow', 'Hello world'])
137-
// No version arg on either streaming merge — the durable version rides the final edit_content write.
138-
expect(mergeEditIntoLiveFileDocMock.mock.calls[0]).toHaveLength(2)
139-
expect(mergeEditIntoLiveFileDocMock.mock.calls[1]).toHaveLength(2)
138+
const [first, second] = mergeEditIntoLiveFileDocMock.mock.calls
139+
// A full-file snapshot (base + streamed), never a diff; it grows across deltas; no version arg on
140+
// either streaming merge — the durable version rides the final edit_content write.
141+
expect(first[0]).toBe('file-grow')
142+
expect(first).toHaveLength(2)
143+
expect(first[1]).toContain('Base.')
144+
expect(first[1]).toContain('Hello')
145+
expect(second).toHaveLength(2)
146+
expect(second[1]).toContain('Hello world')
147+
expect(second[1].length).toBeGreaterThan(first[1].length)
140148
})
141149

142150
it('throttles merges: two deltas within LIVE_DOC_MERGE_THROTTLE_MS yield one merge', async () => {
143151
const intent = makeIntent({
144-
operation: 'update',
152+
operation: 'append',
145153
fileId: 'file-throttle',
146154
fileName: 'notes.md',
147155
})
@@ -155,11 +163,10 @@ describe('processFilePreviewStreamEvent — live-doc streaming merge', () => {
155163
await flushMicrotasks()
156164

157165
expect(mergeEditIntoLiveFileDocMock).toHaveBeenCalledTimes(1)
158-
expect(mergeEditIntoLiveFileDocMock).toHaveBeenCalledWith('file-throttle', 'Hel')
159166
})
160167

161168
it('does not merge for a non-markdown file (no collaborative room)', async () => {
162-
const intent = makeIntent({ operation: 'update', fileId: 'file-txt', fileName: 'notes.txt' })
169+
const intent = makeIntent({ operation: 'append', fileId: 'file-txt', fileName: 'notes.txt' })
163170

164171
await drive(editContentDelta('{"content":"plain text body'), intent)
165172
await flushMicrotasks()
@@ -178,4 +185,25 @@ describe('processFilePreviewStreamEvent — live-doc streaming merge', () => {
178185
// A base-less snapshot would diff to a delete-everything wipe of the seeded doc, so it must be skipped.
179186
expect(mergeEditIntoLiveFileDocMock).not.toHaveBeenCalled()
180187
})
188+
189+
it('does not stream an update (from-scratch rewrite) — it would blank the doc mid-stream', async () => {
190+
const intent = makeIntent({ operation: 'update', fileId: 'file-update', fileName: 'notes.md' })
191+
192+
await drive(editContentDelta('{"content":"Rewritten intro'), intent)
193+
await flushMicrotasks()
194+
195+
// Update streams a partial rewrite; diffing the full doc toward it would delete most of the file
196+
// until it grows back, so update applies atomically at the final durable write instead.
197+
expect(mergeEditIntoLiveFileDocMock).not.toHaveBeenCalled()
198+
})
199+
200+
it('skips the merge while one is already in flight for the file (does not backlog / advance throttle)', async () => {
201+
isLiveDocMergeInFlightMock.mockReturnValue(true)
202+
const intent = makeIntent({ operation: 'append', fileId: 'file-busy', fileName: 'notes.md' })
203+
204+
await drive(editContentDelta('{"content":"Hello'), intent)
205+
await flushMicrotasks()
206+
207+
expect(mergeEditIntoLiveFileDocMock).not.toHaveBeenCalled()
208+
})
181209
})

apps/sim/lib/copilot/request/go/file-preview-adapter.ts

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {
2424
buildFilePreviewText,
2525
loadWorkspaceFileTextForPreview,
2626
} from '@/lib/copilot/tools/server/files/file-preview'
27-
import { mergeEditIntoLiveFileDoc } from '@/lib/realtime/notify'
27+
import { isLiveDocMergeInFlight, mergeEditIntoLiveFileDoc } from '@/lib/realtime/notify'
2828
import { resolveWorkspaceFileReference } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
2929
import { isMarkdownFile } from '@/lib/uploads/utils/file-utils'
3030

@@ -653,19 +653,25 @@ export async function processFilePreviewStreamEvent(input: {
653653

654654
// Stream the growing content into the file's LIVE collaborative Y.Doc (when a room is open)
655655
// so collaborators watching the file see the copilot write stream in via Yjs — the AI as a
656-
// CRDT peer, applied by the relay as a minimal `updateYFragment` diff. Throttled per file;
657-
// fire-and-forget so a slow relay never stalls the stream (`mergeEditIntoLiveFileDoc` never
658-
// throws, coordinates ordering per file, and treats a versionless call as a non-durable
659-
// preview merge). No-op for `create` (never streams here) and for a file with no open room
660-
// (the relay reports `applied: false`). Gates: markdown only — non-markdown has no
661-
// collaborative room; and for `append`/`patch`, only once the base file content has loaded —
662-
// a base-less snapshot would diff to a delete-everything wipe of the seeded doc. `update`
663-
// streams a full rewrite from scratch, so it needs no base.
656+
// CRDT peer, applied by the relay as a minimal `updateYFragment` diff. Fire-and-forget so a
657+
// slow relay never stalls the stream; `mergeEditIntoLiveFileDoc` serializes ordering per file
658+
// and treats a versionless call as a non-durable preview merge (the final `edit_content`
659+
// write carries the real version and reconciles the durable file). No-op for `create` (never
660+
// streams here) and for a file with no open room (the relay reports `applied: false`).
661+
//
662+
// Gates: markdown only (non-markdown has no collaborative room). Only `append`/`patch` stream
663+
// — they build on the existing content, so they need the base loaded (a base-less snapshot
664+
// would diff to a delete-everything wipe of the seeded doc). `update` is a from-scratch
665+
// rewrite: streaming its partial content would diff the full doc toward a fragment and blank
666+
// it mid-stream, so it applies atomically at the final durable write instead. Skip while a
667+
// merge is in flight for this file — one at a time, and don't advance the throttle on a
668+
// no-op — so a slow relay can't backlog stale snapshots or make the doc lag the stream.
664669
const dueForLiveMerge =
665670
nextSession.fileId !== undefined &&
666671
isMarkdownFile({ name: nextSession.fileName ?? '' }) &&
667-
(editIntent.operation === 'update' ||
668-
currentPreview.session.baseContent !== undefined) &&
672+
(editIntent.operation === 'append' || editIntent.operation === 'patch') &&
673+
currentPreview.session.baseContent !== undefined &&
674+
!isLiveDocMergeInFlight(nextSession.fileId) &&
669675
now - currentPreview.lastLiveMergeAt >= LIVE_DOC_MERGE_THROTTLE_MS
670676
const nextLiveMergeAt = dueForLiveMerge ? now : currentPreview.lastLiveMergeAt
671677
if (dueForLiveMerge && nextSession.fileId) {

apps/sim/lib/realtime/notify.test.ts

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'
66
vi.mock('@/lib/core/utils/urls', () => ({ getSocketServerUrl: () => 'http://realtime' }))
77
vi.mock('@/lib/core/config/env', () => ({ env: { INTERNAL_API_SECRET: 'secret' } }))
88

9-
import { mergeEditIntoLiveFileDoc } from './notify'
9+
import { isLiveDocMergeInFlight, mergeEditIntoLiveFileDoc } from './notify'
1010

1111
describe('mergeEditIntoLiveFileDoc', () => {
1212
afterEach(() => {
@@ -39,23 +39,23 @@ describe('mergeEditIntoLiveFileDoc', () => {
3939
await expect(mergeEditIntoLiveFileDoc('file-1', '# hello', 42)).resolves.toBeUndefined()
4040
})
4141

42-
it('drops a streaming (versionless) merge while one is already in flight for the same file', async () => {
43-
let resolveFirst: (value: { ok: boolean }) => void = () => {}
44-
const fetchMock = vi
45-
.fn()
46-
.mockImplementationOnce(() => new Promise((resolve) => (resolveFirst = resolve)))
47-
.mockResolvedValue({ ok: true })
48-
vi.stubGlobal('fetch', fetchMock)
42+
it('reports isLiveDocMergeInFlight while a merge runs and clears when it settles', async () => {
43+
let resolveFetch: (value: { ok: boolean }) => void = () => {}
44+
vi.stubGlobal(
45+
'fetch',
46+
vi.fn(() => new Promise((resolve) => (resolveFetch = resolve)))
47+
)
4948

50-
const first = mergeEditIntoLiveFileDoc('file-inflight', 'v1') // versionless → in flight (pending)
49+
expect(isLiveDocMergeInFlight('file-flight')).toBe(false)
50+
const run = mergeEditIntoLiveFileDoc('file-flight', 'v1')
5151
await Promise.resolve()
52-
// A second versionless merge while the first is pending is dropped, not queued — so a stale
53-
// snapshot can never land after a newer one and regress the doc.
54-
await mergeEditIntoLiveFileDoc('file-inflight', 'v2')
55-
expect(fetchMock).toHaveBeenCalledTimes(1)
52+
// The streaming caller checks this to skip a redundant merge (and not advance its throttle) while
53+
// one is in flight, so a slow relay can't backlog stale snapshots.
54+
expect(isLiveDocMergeInFlight('file-flight')).toBe(true)
5655

57-
resolveFirst({ ok: true })
58-
await first
56+
resolveFetch({ ok: true })
57+
await run
58+
expect(isLiveDocMergeInFlight('file-flight')).toBe(false)
5959
})
6060

6161
it('a durable (versioned) merge waits for an in-flight streaming merge, then applies last', async () => {
@@ -86,4 +86,41 @@ describe('mergeEditIntoLiveFileDoc', () => {
8686
JSON.stringify({ fileId: 'file-durable', markdown: 'final content', version: 100 })
8787
)
8888
})
89+
90+
it('serializes concurrent durable writes behind a streaming merge, strictly in order', async () => {
91+
const applied: Array<number | 'stream'> = []
92+
const resolvers: Array<() => void> = []
93+
vi.stubGlobal(
94+
'fetch',
95+
vi.fn((_url: string, init: { body: string }) => {
96+
applied.push(JSON.parse(init.body).version ?? 'stream')
97+
return new Promise<{ ok: boolean }>((resolve) =>
98+
resolvers.push(() => resolve({ ok: true }))
99+
)
100+
})
101+
)
102+
const flush = async () => {
103+
for (let i = 0; i < 6; i++) await Promise.resolve()
104+
}
105+
106+
const s = mergeEditIntoLiveFileDoc('file-order', 's') // streaming, in flight
107+
await flush()
108+
// Two durable writes arrive while the streaming merge is in flight — both must chain, not both
109+
// resume-and-fire concurrently.
110+
const a = mergeEditIntoLiveFileDoc('file-order', 'a', 1)
111+
const b = mergeEditIntoLiveFileDoc('file-order', 'b', 2)
112+
await flush()
113+
expect(applied).toEqual(['stream']) // A and B queued behind streaming
114+
115+
resolvers[0]() // finish streaming → A applies next (not B)
116+
await flush()
117+
expect(applied).toEqual(['stream', 1])
118+
119+
resolvers[1]() // finish A → B applies after A
120+
await flush()
121+
expect(applied).toEqual(['stream', 1, 2])
122+
123+
resolvers[2]()
124+
await Promise.all([s, a, b])
125+
})
89126
})

apps/sim/lib/realtime/notify.ts

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -121,41 +121,51 @@ export async function notifyFolderResourceChanged(
121121
* server-side), and the relay applies this merge THROUGH the shared Redis stream, so it reaches the
122122
* live doc on whichever task holds it and can't go stale relative to this direct write.
123123
*
124-
* Awaited (not fire-and-forget) so the fetch dispatches before the route handler returns; bounded to
125-
* {@link APPLY_EDIT_TIMEOUT_MS}, so it adds latency only when the socket pod is unreachable.
124+
* A durable caller awaits this (so the fetch dispatches before the route handler returns); the copilot
125+
* streaming caller fires and forgets it. Bounded to {@link APPLY_EDIT_TIMEOUT_MS}, so it adds latency
126+
* only when the socket pod is unreachable.
126127
*
127128
* `version` is the durable `contentUpdatedAt` (epoch ms) this markdown was written with, for a durable
128129
* write. Omit it for a STREAMING intermediate merge (the copilot stream mid-flight): intermediate
129130
* content advances the live doc for viewers but is not a durable checkpoint, so the relay leaves its
130131
* synced version pinned to the last durable write — which is exactly the copilot tool's final
131132
* `edit_content` write, carrying the real version, that reconciles the durable file.
132133
*
133-
* Concurrency is coordinated per file so that ordering can never regress the doc: a STREAMING
134-
* (versionless) merge is DROPPED while another is already in flight for the file — the next throttled
135-
* caller sends the latest snapshot, and a stale snapshot can never land after a newer one; a DURABLE
136-
* (versioned) write instead WAITS for the in-flight streaming merge, so the final content is always the
137-
* last merge applied and cannot be clobbered by a late straggler.
134+
* Merges for a file run on a single serialized chain: each is chained after the current tail and
135+
* applies strictly after it, so ordering can never regress the doc — a DURABLE (versioned) write
136+
* always applies after any in-flight streaming merge AND after every earlier durable write, never
137+
* concurrently. The final durable write is therefore always the last merge applied and cannot be
138+
* clobbered by a late straggler. The copilot streaming caller uses {@link isLiveDocMergeInFlight} to
139+
* skip redundant snapshots while one is in flight, so a slow relay can't backlog stale snapshots.
138140
*/
139141
export async function mergeEditIntoLiveFileDoc(
140142
fileId: string,
141143
markdown: string,
142144
version?: number
143145
): Promise<void> {
144-
const pending = liveDocMergeInFlight.get(fileId)
145-
if (version === undefined && pending) return
146-
if (pending) await pending
147-
148-
const run = applyLiveFileDocMerge(fileId, markdown, version)
149-
liveDocMergeInFlight.set(fileId, run)
146+
const tail = liveDocMergeChain.get(fileId) ?? Promise.resolve()
147+
const run = tail.then(() => applyLiveFileDocMerge(fileId, markdown, version))
148+
liveDocMergeChain.set(fileId, run)
150149
try {
151150
await run
152151
} finally {
153-
if (liveDocMergeInFlight.get(fileId) === run) liveDocMergeInFlight.delete(fileId)
152+
if (liveDocMergeChain.get(fileId) === run) liveDocMergeChain.delete(fileId)
154153
}
155154
}
156155

157-
/** Files with a live-doc merge in flight → the running merge promise (never rejects). */
158-
const liveDocMergeInFlight = new Map<string, Promise<void>>()
156+
/** Per file, the tail of the serialized merge chain (each merge applies after it); never rejects
157+
* because {@link applyLiveFileDocMerge} never throws. Absent when the file's chain is idle. */
158+
const liveDocMergeChain = new Map<string, Promise<void>>()
159+
160+
/**
161+
* Whether a live-doc merge is currently running or queued for the file. The copilot streaming caller
162+
* checks this to skip a redundant snapshot (and to not advance its send throttle) while a merge is in
163+
* flight — bounding the stream to one live merge per file at a time without backlogging stale
164+
* snapshots behind a slow relay.
165+
*/
166+
export function isLiveDocMergeInFlight(fileId: string): boolean {
167+
return liveDocMergeChain.has(fileId)
168+
}
159169

160170
/** POST the merge to the relay. Never throws (a live-doc merge is best-effort). */
161171
async function applyLiveFileDocMerge(

0 commit comments

Comments
 (0)