From 994b8dcae16fc5f2daad98cd16b8561dbb73ee27 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 14:31:52 +0200 Subject: [PATCH] fix: keep an unsent row on its own baseline, and let one editor own one delete Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 41 ++++++++++++++++++- .../copilot/chat/global/userDraftAdapter.ts | 30 ++++++++------ frontend/src/lib/itemStore.svelte.ts | 28 +++++++++---- frontend/src/lib/itemStore.test.ts | 24 +++++++++++ frontend/src/lib/userDraft.svelte.ts | 35 +++++++++++----- 5 files changed, 128 insertions(+), 30 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 90ff8a8f77..043ca9d794 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -342,7 +342,11 @@ import { setListAppRunsHandler, setOpenPreviewHandler } from './core' -import { UserDraft, __resetUserDraftForTesting } from '$lib/userDraft.svelte' +import { + UserDraft, + __resetUserDraftForTesting, + registerLiveItemBridge +} from '$lib/userDraft.svelte' import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' import { clearGlobalDrafts, @@ -2306,6 +2310,41 @@ describe('global AI tools', () => { expect(res.status).toBe('conflict') }) + // An open editor owns its own row: the chat must defer to that editor's discard rather than + // send a delete of its own, which would be a second unconditional request. + it('leaves the delete to a live editor that owns the draft', async () => { + const path = 'f/scripts/livedel' + seedBackendDraft('script', path, { + path, + summary: 's', + content: 'export function main() {}', + language: 'bun' + }) + let discards = 0 + registerLiveItemBridge({ + seed: () => false, + read: () => undefined, + // Claims the key and removes nothing, so anything still gone was deleted by the chat. + discard: () => { + discards++ + return Promise.resolve({ removed: true }) + }, + list: () => [] + }) + try { + await deleteGlobalDraft(WORKSPACE, 'script', path) + expect(discards).toBe(1) + expect(getBackendDraft('script', path)).toBeDefined() + } finally { + registerLiveItemBridge({ + seed: () => false, + read: () => undefined, + discard: () => undefined, + list: () => [] + }) + } + }) + // A failed server delete must surface (throw), not silently report removed — // the same guard the write path got, applied to the delete path. it('deleteGlobalDraft throws when the server delete fails', async () => { diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index f2c3f02431..980a5d7fca 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -587,20 +587,26 @@ export async function deleteGlobalDraft( if (!itemKind) return const storagePath = resolveDraftStoragePath(workspace, itemKind, path) const liveDraft = UserDraft.getLiveEditorDraft(itemKind, { workspace }) - if (options.preserveLiveDraft && liveDraft?.storagePath === storagePath) { - UserDraft.remove(itemKind, storagePath, { workspace }) + const live = + options.preserveLiveDraft && liveDraft?.storagePath === storagePath + ? UserDraft.remove(itemKind, storagePath, { workspace }) + : UserDraft.clear(itemKind, storagePath, { workspace }) + if (live) { + // An open editor owns this row and its own discard deletes it, resolving once that has + // landed. A delete of ours alongside it would be a second request with nothing to check + // against, free to remove a draft saved from elsewhere in between. + await live } else { - UserDraft.clear(itemKind, storagePath, { workspace }) + // `remove`/`clear` only debounce the delete; persist it now so a deploy/discard + // that the caller awaits has actually cleared the server draft on return. + await UserDraftDbSyncer.save({ + workspace, + itemKind, + path: storagePath, + value: null, + immediate: true + }) } - // `remove`/`clear` only debounce the delete; persist it now so a deploy/discard - // that the caller awaits has actually cleared the server draft on return. - await UserDraftDbSyncer.save({ - workspace, - itemKind, - path: storagePath, - value: null, - immediate: true - }) // A failed (network/5xx) or conflicted delete is recorded in the syncer state, // not thrown — surface it so callers don't report the draft as removed while // the DB-backed source of truth still has it (same guard as the write path). diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index ac82dd5904..77498a6121 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -286,7 +286,13 @@ class Entry { * `last_sync`: one fact, so taken together and against one `asOf`, the rows sent when the * read was issued. Seeding behind a row sent since puts `last_sync` before what the syncer * has already sent, which the server refuses from then on. */ - private adoptRow(key: ItemKey, row: unknown, draftSavedAt: string | undefined, asOf: number) { + private adoptRow( + key: ItemKey, + row: unknown, + draftSavedAt: string | undefined, + asOf: number, + waiting: boolean + ) { if (this.ports.rowMark(key) !== asOf) return // A payload the server *refused* predates this read and the baseline taken below would // make it acceptable, so it goes; the reconcile after the read re-queues whatever the @@ -294,7 +300,10 @@ class Entry { // only record of an edit that never reached anyone, so it stays. if (this.ports.conflicted(key)) this.ports.dropPending(key) this.row = row === null || row === undefined ? null : (serialize(row) ?? null) - this.ports.seedSync(key, draftSavedAt, asOf) + // A payload still waiting to be sent was written against the baseline it had then. Taking + // this read's would make it acceptable over whatever has been written since, so it would + // overwrite that without ever conflicting. Left alone, its own baseline decides. + if (!waiting) this.ports.seedSync(key, draftSavedAt, asOf) } private replaceValue(value: V | undefined): void { @@ -414,7 +423,7 @@ class Entry { const draft = (unsent === null ? undefined : row) as V | undefined // The deployed side above always advances: the item did change, and a discard after // the user keeps theirs has to land on what the server holds now. - if (!standOff) this.adoptRow(key, row, res.draftSavedAt, rowsAtRead) + if (!standOff) this.adoptRow(key, row, res.draftSavedAt, rowsAtRead, unsent !== undefined) this.loaded = true if (!keepValue) { this.pristine = this.settles && this.origin === 'deployed' && draft === undefined @@ -1080,11 +1089,16 @@ export function createItemStore(ports: ItemRowPort) { if (!entry || !entry.loaded) return undefined return { value: entry.dirty && entry.origin !== 'new' ? snapshot(entry.value) : undefined } }, - discard(workspace: string, kind: UserDraftItemKind, path: string): boolean { + discard( + workspace: string, + kind: UserDraftItemKind, + path: string + ): Promise | undefined { const entry = find(workspace, kind, path) - if (!entry) return false - void entry.discard() - return true + if (!entry) return undefined + // Returned rather than left running: the caller would otherwise send its own delete + // alongside this one, and the second goes out with no baseline to check. + return entry.discard() }, list(workspace: string, kinds: readonly UserDraftItemKind[]): UserDraftEntry[] { const out: UserDraftEntry[] = [] diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 653aa71ec4..f13984fbba 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -1251,6 +1251,30 @@ describe('item store: conflicts', () => { }) }) + it('does not rebase an unsent edit onto a draft written since', async () => { + const rows = fakeRows() + const store = createItemStore(rows.port) + const key: ItemKey = { workspace: 'w', kind: 'resource', path: 'u/me/r' } + // Another tab saved B while our A sat parked, so the server's row is theirs now. + const theirs = { ...deployedRes, description: 'saved by another tab' } + const a = adapter(async () => ({ deployed: deployedRes, draft: theirs, draftSavedAt: 'T9' })) + const first = store.acquire(key, { workspace: 'w', path: 'u/me/r' }, a) + await settle() + rows.failing.add('u/me/r') + first.handle.value = { ...deployedRes, description: 'mine, never sent' } + await rows.port.flush(key) + first.release() + + const seedsBefore = rows.seeds.length + const second = store.acquire(key, { workspace: 'w', path: 'u/me/r' }, a) + await settle() + + // Ours is shown, because it is the only copy of it. But taking their timestamp would let + // it be accepted over their draft with no conflict ever raised, so it is not taken. + expect(second.handle.value?.description).toBe('mine, never sent') + expect(rows.seeds.slice(seedsBefore)).toEqual([]) + }) + it('re-issues a draft delete the server never received, on reopening', async () => { const rows = fakeRows() const store = createItemStore(rows.port) diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index aa55b2907d..99dcf0922e 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -141,8 +141,13 @@ export type LiveItemBridge = { itemKind: UserDraftItemKind, path: string ): { value: unknown | undefined } | undefined - /** Discard the live item's draft. False when no live item holds the key. */ - discard(workspace: string, itemKind: UserDraftItemKind, path: string): boolean + /** Discard the live item's draft, resolving once its delete has landed. `undefined` when no + * live item holds the key, so the caller issues the delete itself. */ + discard( + workspace: string, + itemKind: UserDraftItemKind, + path: string + ): Promise | undefined list(workspace: string, itemKinds: readonly UserDraftItemKind[]): UserDraftEntry[] } @@ -370,15 +375,20 @@ export const UserDraft = { return writtenCache.get(mk)?.val !== undefined }, - remove(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { + remove( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions + ): Promise | undefined { const ws = resolveWorkspace(opts) - if (liveItems?.discard(ws, itemKind, path)) { + const live = liveItems?.discard(ws, itemKind, path) + if (live) { // The live item owns the row, and its own discard deletes it. Falling through to the // POST below would race that delete with an unconditional one (see `forgetLocal`), so // only this caller's mirror is dropped — otherwise its reads keep answering with a // draft that is gone. UserDraft.forgetLocal(itemKind, path, opts) - return + return live } const mk = mapKey(ws, itemKind, path) const entry = entries.get(mk) @@ -418,8 +428,12 @@ export const UserDraft = { writtenCache.delete(mk) }, - clear(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { - UserDraft.discard(itemKind, path, undefined, opts) + clear( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions + ): Promise | undefined { + return UserDraft.discard(itemKind, path, undefined, opts) }, /** @@ -568,12 +582,13 @@ export const UserDraft = { * leave it unset. */ auto?: boolean } - ): void { + ): Promise | undefined { const ws = resolveWorkspace(opts) const mk = mapKey(ws, itemKind, path) const entry = entries.get(mk) const safeFallback = snapshotDraftValue(fallback) - if (liveItems?.discard(ws, itemKind, path)) { + const live = liveItems?.discard(ws, itemKind, path) + if (live) { // No POST, for the reason `remove` gives. The cell still has to reach `fallback`: // a caller holding one resets it to what it just saved, and its apply-effect would // otherwise copy the stale draft straight back over the form. @@ -582,7 +597,7 @@ export const UserDraft = { entry.state.val = safeFallback } writtenCache.delete(mk) - return + return live } if (entry) { entry.skipNextSync = true