From da588bf8edc9b7f2a2f8870da77a25e124a5d930 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 16:18:53 +0200 Subject: [PATCH] fix: tell a failed refresh from an absent editor, and a patch's value from a newer one Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 8 +-- .../copilot/chat/global/userDraftAdapter.ts | 13 +++-- frontend/src/lib/itemStore.svelte.ts | 25 ++++++--- frontend/src/lib/itemStore.test.ts | 52 ++++++++++++++++++- frontend/src/lib/userDraft.svelte.ts | 7 +-- 5 files changed, 86 insertions(+), 19 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 90016c209c..67c4cb7706 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2361,7 +2361,7 @@ describe('global AI tools', () => { read: () => undefined, refresh: () => { calls.push('refresh') - return Promise.resolve(true) + return Promise.resolve('done' as const) }, itemDeleted: () => { calls.push('itemDeleted') @@ -2401,8 +2401,8 @@ describe('global AI tools', () => { registerLiveItemBridge({ seed: () => false, read: () => undefined, - // Registered, but not loaded: it answers, and says it has not dealt with the row. - refresh: () => Promise.resolve(false), + // Registered, but never had the item: it has not dealt with the row. + refresh: () => Promise.resolve('absent' as const), itemDeleted: () => Promise.resolve(), discard: () => Promise.resolve(false), list: () => [] @@ -2434,7 +2434,7 @@ describe('global AI tools', () => { registerLiveItemBridge({ seed: () => false, read: () => undefined, - refresh: () => Promise.resolve(true), + refresh: () => Promise.resolve('done' as const), itemDeleted: () => { calls.push('itemDeleted') return Promise.resolve() diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 319cc7e1dd..9bc5e63b7d 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -620,15 +620,22 @@ export async function deleteGlobalDraft( // divergence from what was deployed and belongs in the row. The editor's own rule drops // the row when the two match and keeps it when they do not. const refreshed = await refreshLiveItem(workspace, itemKind, storagePath) - if (refreshed) { + if (refreshed === 'done') { // Its reconcile only queued that; send it, so a caller awaiting this sees the server // settled — with no draft, or with the newer edit. await UserDraftDbSyncer.flush({ workspace, itemKind, path: storagePath }) await assertDraftCleanupLanded(workspace, itemKind, path, storagePath) return } - // It could not read after all (still loading, or the read failed), so it has not dealt - // with the row and this caller still has to. + if (refreshed === 'failed') { + // It has the item and a baseline from before the deploy. Discarding its row would + // reset it to that and throw away anything typed during the deploy, so the deploy is + // reported as done and its draft cleanup as not, which is what happened. + throw new Error( + `Deployed "${path}", but its open editor could not be refreshed, so its draft was left in place. Reload the editor.` + ) + } + // `absent`: never had the item, so it has not dealt with the row and this caller must. } const liveDraft = UserDraft.getLiveEditorDraft(itemKind, { workspace }) const live = diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index b79b09519d..f70b30e777 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -451,13 +451,16 @@ class Entry { /** * Re-read for a caller that has just written the deployed item, taking a turn in the queue * first: asked for during the first load, `reread` would find nothing loaded and return - * without doing anything. Resolves false when it could not read after all, so the caller - * knows this editor has not dealt with the row and it must. + * without doing anything. The three answers are not interchangeable — `absent` means this + * editor never had the item and the row is the caller's to deal with, while `failed` means it + * has the item and an out-of-date baseline, so anything resetting to that baseline now would + * undo what was just deployed. */ - async refreshed(): Promise { + async refreshed(): Promise<'done' | 'absent' | 'failed'> { await this.run(async () => {}) - if (!this.loaded || this.retired || !this.adapter) return false - return (await this.reread()).ok + if (this.retired || !this.adapter) return 'absent' + if (!this.loaded) return 'absent' + return (await this.reread()).ok ? 'done' : 'failed' } /** @@ -683,6 +686,10 @@ class Entry { } if (this.value !== undefined) Object.assign(this.value as object, fields) this.touch() + // Taken after this patch's own touch, so it counts only what happens next. The value it + // put on screen is the user's copy, and a field carrying the value this patch sent is not + // proof this patch put it there: the chat can write a draft holding the same value. + const appliedAt = this.edits /** Release each field this patch still owns; one a later patch has taken stays with it. */ const release = () => { for (const k of Object.keys(sent)) { @@ -704,12 +711,14 @@ class Entry { } release() // The value is the user's copy, so it is given back by hand — to the baseline as it - // stands now, which is the server's value or a later patch's, never this one's. + // stands now, which is the server's value or a later patch's, never this one's. Not + // given back at all if something has written the value since: that is newer than this + // patch and rolling it back would be this patch overwriting it. if (failed) { const base = (onTemplate ? this.template : this.deployed) as | Record | undefined - if (base !== undefined && this.value !== undefined) { + if (base !== undefined && this.value !== undefined && this.edits === appliedAt) { const out = snapshot(this.value) as Record let changed = false for (const [k, v] of Object.entries(sent)) { @@ -1137,7 +1146,7 @@ export function createItemStore(ports: ItemRowPort) { workspace: string, kind: UserDraftItemKind, path: string - ): Promise | undefined { + ): Promise<'done' | 'absent' | 'failed'> | undefined { return find(workspace, kind, path)?.refreshed() }, itemDeleted(workspace: string, kind: UserDraftItemKind, path: string): Promise { diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 43085a4d15..db5dd96e9f 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -785,6 +785,56 @@ describe('item store: origins', () => { }) }) + it('tells a caller its post-deploy re-read failed, apart from never having the item', async () => { + const rows = fakeRows() + const store = createItemStore(rows.port) + let reads = 0 + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'resource', path: 'u/me/r' }, + { workspace: 'w', path: 'u/me/r' }, + adapter(async () => { + // The first read works, so this editor has the item; the one after the deploy does not. + if (reads++ > 0) throw new Error('the GET failed') + return { deployed: deployedRes } + }) + ) + await settle() + item.value = { ...deployedRes, description: 'typed during the deploy' } + + // `absent` would send the caller off to delete this row and discard against a baseline + // from before the deploy; the editor still has the item, so it says so. + expect(await store.bridge.refresh('w', 'resource', 'u/me/r')).toBe('failed') + expect(item.value?.description).toBe('typed during the deploy') + expect(item.dirty).toBe(true) + }) + + it('does not roll a refused toggle back over a draft written while it was out', async () => { + type Sched = { path: string; enabled: boolean; summary: string } + const rows = fakeRows() + const store = createItemStore(rows.port) + const refusal = deferred() + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'trigger_schedule', path: 's' }, + { workspace: 'w', path: 's' }, + { + load: async () => ({ deployed: { path: 's', enabled: false, summary: 'a' } }) + } as ItemAdapter + ) + await settle() + const toggling = item.patch({ enabled: true }, () => refusal.promise) + // The chat writes a draft that happens to carry the same `enabled` the toggle sent. + item.applyExternal({ path: 's', enabled: true, summary: 'from the chat' }) + refusal.reject(new Error('refused')) + + expect(await toggling).toMatchObject({ ok: false }) + // Equality with what the toggle sent is not proof the toggle put it there. + expect(item.value).toEqual({ path: 's', enabled: true, summary: 'from the chat' }) + expect(rows.writes.at(-1)).toEqual({ + path: 's', + value: { path: 's', enabled: true, summary: 'from the chat' } + }) + }) + it('says it did not deal with the row when its own first read never arrived', async () => { const rows = fakeRows() const store = createItemStore(rows.port) @@ -801,7 +851,7 @@ describe('item store: origins', () => { // It is registered, so the chat would otherwise take its answer as the cleanup being done // — while an entry with nothing loaded writes no delete at all and the row survives. expect(await store.bridge.discard('w', 'resource', 'u/me/r')).toBe(false) - expect(await store.bridge.refresh('w', 'resource', 'u/me/r')).toBe(false) + expect(await store.bridge.refresh('w', 'resource', 'u/me/r')).toBe('absent') expect(rows.writes).toEqual([]) }) diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 1d58dffcdc..9d9c8e4aa8 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -142,12 +142,13 @@ export type LiveItemBridge = { path: string ): { value: unknown | undefined } | undefined /** Re-read the live item, for a caller that has just written the deployed item under it. - * `undefined` when no live item holds the key. */ + * `undefined` when no live item holds the key; `absent` when one is registered but never had + * the item; `failed` when it has the item but could not re-read, so its baseline is stale. */ refresh( workspace: string, itemKind: UserDraftItemKind, path: string - ): Promise | undefined + ): Promise<'done' | 'absent' | 'failed'> | undefined /** The deployed item was deleted: the live item reports itself gone. */ itemDeleted(workspace: string, itemKind: UserDraftItemKind, path: string): Promise /** Discard the live item's draft, resolving to whether it dealt with the row. `undefined` when @@ -169,7 +170,7 @@ export function refreshLiveItem( workspace: string, itemKind: UserDraftItemKind, path: string -): Promise | undefined { +): Promise<'done' | 'absent' | 'failed'> | undefined { return liveItems?.refresh(workspace, itemKind, path) }