From 7e8141fd7487a7ec9421184008bd3c30307beeef Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 16 Sep 2026 12:25:42 +0200 Subject: [PATCH] refactor: have the chat persist through the item store, not beside it Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 26 ++++++---- .../copilot/chat/global/userDraftAdapter.ts | 37 +++++++------- frontend/src/lib/itemStore.svelte.ts | 42 ++++++++------- frontend/src/lib/itemStore.test.ts | 51 ++++--------------- frontend/src/lib/userDraft.svelte.ts | 42 +++++++-------- 5 files changed, 85 insertions(+), 113 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 4721fba229..0ff3eca0bd 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2316,29 +2316,37 @@ describe('global AI tools', () => { function noLiveItems(): LiveItemBridge { return { seed: () => false, - rowFor: () => undefined, + persist: () => undefined, read: () => undefined, refresh: () => undefined, - noteRow: () => {}, itemDeleted: () => Promise.resolve(), discard: () => undefined, list: () => [] } } - // The chat can write a value that turns out to be the deployed one. A live editor takes it and - // stays clean, keeping no row — so this save has to clear the row rather than persist a draft - // its own editor does not have: nothing would show it, and nothing short of an edit would - // remove it, while it still lists and goes stale once the deployed item moves on. - it('clears the row when the chat writes the value the live editor already deploys', async () => { + // An open editor that takes the value owns the row for it, including deciding there should not + // be one when the value is the deployed one. Writing the row here as well would make this the + // second writer of one key, which is the whole class of bug this defers to avoid. + it('leaves the row to a live editor that took the value', async () => { const path = 'f/res/noop' const deployed = { path, value: { a: 1 } } seedBackendDraft('resource', path, { path, value: { a: 2 } }) - registerLiveItemBridge({ ...noLiveItems(), seed: () => true, rowFor: () => null }) + let persisted = 0 + registerLiveItemBridge({ + ...noLiveItems(), + seed: () => true, + persist: () => { + persisted++ + return Promise.resolve('saved' as const) + } + }) try { const res = await persistGlobalDraft(WORKSPACE, 'resource', path, deployed) expect(res.status).toBe('saved') - expect(getBackendDraft('resource', path)).toBeUndefined() + expect(persisted).toBe(1) + // Untouched by this caller: what the row should hold was the editor's to decide. + expect(getBackendDraft('resource', path)).toEqual({ path, value: { a: 2 } }) } finally { registerLiveItemBridge(noLiveItems()) } diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 91cdb23fd1..b1d71533ad 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -3,9 +3,8 @@ import { DraftService } from '$lib/gen' import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' import { DEFAULT_DATA as DEFAULT_RAW_APP_DATA } from '$lib/components/raw_apps/dataTableRefUtils' import { - liveItemRow, + persistLiveItem, markLiveItemDeleted, - noteLiveItemRow, refreshLiveItem, UserDraft, type UserDraftEntry, @@ -429,24 +428,22 @@ export async function persistGlobalDraft( const itemKind = itemKindFor(type, opts.triggerKind) if (!itemKind) throw new Error(`Unsupported draft type "${type}".`) const storagePath = resolveDraftStoragePath(workspace, itemKind, path) - // A live item that took the value decides what belongs in the row, and answers `null` when the - // value it now holds is the deployed one. Persisting `value` regardless would leave a row for a - // draft that item does not have, invisible to its banner and undeletable by its Save. Asked - // only of an item that took this value, so the answer is about it and not what it held before. - const took = UserDraft.seed(itemKind, storagePath, value, { workspace }) - const held = took ? liveItemRow(workspace, itemKind, storagePath) : undefined - const row = held === undefined ? value : held === null ? null : held.value - // An open editor took that value and may have written its own row for it; this save is a - // second row for the same value, so say so rather than leave it looking like someone else's. - noteLiveItemRow(workspace, itemKind, storagePath) - await UserDraftDbSyncer.save({ - workspace, - itemKind, - path: storagePath, - value: row, - immediate: true, - force: opts.force - }) + // An open editor that takes this value owns the row for it: it persists what its own rule says + // belongs there, which is nothing when the value turns out to be the deployed one. Writing the + // row here as well would make this the second writer of one key, and leave a draft that editor + // does not have. Only with nobody holding it is the row this caller's to write. + UserDraft.seed(itemKind, storagePath, value, { workspace }) + const landed = await persistLiveItem(workspace, itemKind, storagePath, opts.force) + if (landed === undefined) { + await UserDraftDbSyncer.save({ + workspace, + itemKind, + path: storagePath, + value, + immediate: true, + force: opts.force + }) + } const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath) const item = userDraftEntryToWorkspaceItem( { workspace, itemKind, path: storagePath, value }, diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index c021f78c26..c3b7f0594d 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -592,11 +592,22 @@ class Entry { }) } - /** A caller that handed a value in through `applyExternal` and wrote its own row for it, - * rather than leaving that to this entry: the row is accounted for, and counting it as - * unaccounted would read as someone else having written it. */ - noteRow(): void { - this.values++ + /** + * Put this entry's row on the server now, for a caller that handed a value in and needs to + * know it landed. The row itself is whatever the rule says — the value, or nothing when it + * matches the deployed one — so a caller cannot persist a draft this entry does not have. + */ + async persistRow(force = false): Promise<'saved' | 'conflict' | 'failed'> { + const key = this.key + if (this.stale) return 'failed' + if (force) { + await this.ports.overwrite(key, this.dirty ? snapshot(this.value) : null) + } else { + if (this.ports.conflicted(key)) return 'conflict' + await this.ports.flush(key) + } + if (this.ports.conflicted(key)) return 'conflict' + return this.ports.failure(key) === undefined ? 'saved' : 'failed' } /** An outside write (the AI chat, another editor): a real divergence, never settling. */ @@ -1270,18 +1281,18 @@ export function createItemStore(ports: ItemRowPort) { entry.applyExternal(value) return true }, - /** The row this entry keeps for the key, by the same rule `reconcile` writes by: `{ value }` - * when its value diverges from the deployed one, `null` when it does not and so nothing - * belongs in the row. `undefined` when nobody can answer — no entry, one that has not read - * yet, or one whose baseline is known to be behind the server. */ - rowFor( + /** Put the live item's own row on the server now and say how it went. `undefined` when + * nobody holds the key, or holds it without having read it: then the row is the caller's + * to write, because no entry here can say what belongs in it. */ + persist( workspace: string, kind: UserDraftItemKind, - path: string - ): { value: unknown } | null | undefined { + path: string, + force?: boolean + ): Promise<'saved' | 'conflict' | 'failed'> | undefined { const entry = find(workspace, kind, path) - if (!entry || !entry.loaded || entry.stale) return undefined - return entry.dirty ? { value: snapshot(entry.value) } : null + if (!entry || !entry.loaded) return undefined + return entry.persistRow(force) }, /** The draft as this tab knows it: the value when it diverges, nothing otherwise. */ read( @@ -1300,9 +1311,6 @@ export function createItemStore(ports: ItemRowPort) { ): Promise<'done' | 'absent' | 'failed'> | undefined { return find(workspace, kind, path)?.refreshed() }, - noteRow(workspace: string, kind: UserDraftItemKind, path: string): void { - find(workspace, kind, path)?.noteRow() - }, itemDeleted(workspace: string, kind: UserDraftItemKind, path: string): Promise { return find(workspace, kind, path)?.itemDeleted() ?? Promise.resolve() }, diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index a4db40c91b..7bdf80d875 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -781,7 +781,6 @@ describe('item store: origins', () => { const fromChat = { ...deployedRes, description: 'from the chat' } item.applyExternal(fromChat) // `persistGlobalDraft` hands the value here, says it is writing the row for it, and does. - store.bridge.noteRow('w', 'resource', 'u/me/r') rows.handExternally('u/me/r') read.resolve({ deployed: deployedRes }) @@ -913,7 +912,7 @@ describe('item store: origins', () => { expect(item.value?.description).toBe('typed during the deploy') }) - it('still autosaves after the chat wrote a row during its first read', async () => { + it('still autosaves after the chat handed it a value during its first read', async () => { const rows = fakeRows() const read = deferred>() const store = createItemStore(rows.port) @@ -924,13 +923,10 @@ describe('item store: origins', () => { adapter(() => read.promise) ) await settle() - // The chat writes while the GET is out. Its row is this entry's own doing, and its value - // is right here — so the read is behind the server, but not in a way that loses anything. + // The chat hands its value here while the GET is out and leaves the row to this entry, so + // the read is behind the server but not in a way that loses anything. const fromChat = { ...deployedRes, description: 'from the chat' } item.applyExternal(fromChat) - // `persistGlobalDraft` hands the value here and writes its own row through the syncer. - store.bridge.noteRow('w', 'resource', 'u/me/r') - rows.handExternally('u/me/r') read.resolve({ deployed: deployedRes }) await settle() @@ -957,17 +953,15 @@ describe('item store: origins', () => { const reloading = item.reload() await settle() - // `persistGlobalDraft` on a loaded item: the value goes in through the bridge, whose - // reconcile writes a row, and then it writes its own row for that same value. + // `persistGlobalDraft` on a loaded item: the value goes in through the bridge and this + // entry's own reconcile is what writes the row for it. const fromChat = { ...deployedRes, description: 'from the chat' } item.applyExternal(fromChat) - store.bridge.noteRow('w', 'resource', 'u/me/r') - rows.handExternally('u/me/r') read.resolve({ deployed: deployedRes }) await reloading - // Two rows, but both carry the value that is right here, so nothing is unaccounted for - // and the editor has to stay able to persist what is typed next. + // The row that appeared is this entry's own, so nothing is unaccounted for and the editor + // has to stay able to persist what is typed next. expect(item.value).toEqual(fromChat) expect(item.canSave).toBe(true) const typed = { ...deployedRes, description: 'typed after the chat wrote' } @@ -1030,33 +1024,6 @@ describe('item store: origins', () => { expect(item.canSave).toBe(false) }) - it('is not fooled by the chat crediting a row a loading item never wrote', async () => { - const rows = fakeRows() - const read = deferred>() - const store = createItemStore(rows.port) - const key: ItemKey = { workspace: 'w', kind: 'resource', path: 'u/me/r' } - const { handle: item } = store.acquire( - key, - { workspace: 'w', path: 'u/me/r' }, - adapter(() => read.promise) - ) - await settle() - - // The chat seeds and persists while the first GET is out. Loading, this entry writes no - // row of its own, so the chat's save is the only row that value produced. - item.applyExternal({ ...deployedRes, description: 'from the chat' }) - store.bridge.noteRow('w', 'resource', 'u/me/r') - rows.handExternally('u/me/r') - // The legacy agent editor then writes the same resource: a row nobody here has. - rows.handExternally('u/me/r') - read.resolve({ deployed: deployedRes }) - await settle() - - // Two rows, one of them unaccounted for. Crediting the chat twice would hide it and let - // this editor post over the agent editor's newer draft. - expect(item.canSave).toBe(false) - }) - it('does not credit a row to a seed that wrote none', async () => { const rows = fakeRows() const read = deferred>() @@ -1069,8 +1036,8 @@ describe('item store: origins', () => { ) await settle() - // The agent editor seeds its restored draft here while the GET is out. Loading, this - // writes no row, and the seeding editor writes none for it either. + // A value is handed in here while the GET is out. Loading, this entry writes no row for + // it, and whoever handed it in leaves the row to this entry. const seeded = { ...deployedRes, description: 'restored by the agent editor' } item.applyExternal(seeded) // It then autosaves something newer through the `UserDraft` handle it kept: a row whose diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 401dd002c8..20abebf4b1 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -135,13 +135,14 @@ const liveEditorDrafts = new Map() export type LiveItemBridge = { /** Apply `value` as an outside write. False when no live item holds the key. */ seed(workspace: string, itemKind: UserDraftItemKind, path: string, value: unknown): boolean - /** The row a live item keeps for the key: `{ value }` when its value diverges from the deployed - * one, `null` when it does not. `undefined` when no live item can answer. */ - rowFor( + /** Put the live item's own row on the server now and say how it went. `undefined` when no live + * item can answer, and the row is then the caller's to write. */ + persist( workspace: string, itemKind: UserDraftItemKind, - path: string - ): { value: unknown } | null | undefined + path: string, + force?: boolean + ): Promise<'saved' | 'conflict' | 'failed'> | undefined /** `undefined` when no live item holds the key; else the draft, if it has one. */ read( workspace: string, @@ -156,8 +157,6 @@ export type LiveItemBridge = { itemKind: UserDraftItemKind, path: string ): Promise<'done' | 'absent' | 'failed'> | undefined - /** A row the caller wrote itself for a value it just handed in through `seed`. */ - noteRow(workspace: string, itemKind: UserDraftItemKind, path: string): void /** 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. `undefined` when no live item holds the key and `absent` @@ -183,26 +182,19 @@ export function refreshLiveItem( return liveItems?.refresh(workspace, itemKind, path) } -/** What a live item keeps in the key's row, for a caller about to write that row itself: the - * store's rule is that a row exists exactly while the value diverges from the deployed one, so - * persisting the raw value instead would leave a row behind for a value that matches it. - * `undefined` when no live item can answer and the caller's own value is all there is. */ -export function liveItemRow( +/** + * Have the live item holding this key put its own row on the server now, for a caller that handed + * it a value and needs to know it landed. The row is whatever that item's own rule says belongs + * there, so a caller cannot leave behind a draft the item does not have. `undefined` when no live + * item can answer and the caller's own value is all there is. + */ +export function persistLiveItem( workspace: string, itemKind: UserDraftItemKind, - path: string -): { value: unknown } | null | undefined { - return liveItems?.rowFor(workspace, itemKind, path) -} - -/** Tell a live item that a row just written for it was written by whoever handed it that same - * value through `seed`, not by some other editor whose value it does not have. */ -export function noteLiveItemRow( - workspace: string, - itemKind: UserDraftItemKind, - path: string -): void { - liveItems?.noteRow(workspace, itemKind, path) + path: string, + force?: boolean +): Promise<'saved' | 'conflict' | 'failed'> | undefined { + return liveItems?.persist(workspace, itemKind, path, force) } /** Tell a live item that the deployed item under it has been deleted, so it stops showing a