refactor: have the chat persist through the item store, not beside it

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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())
 		}

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 },

frontend/src/lib/itemStore.svelte.ts

@@ -592,11 +592,22 @@ class Entry<V> {
 		})
 	}
 
-	/** 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<void> {
 			return find(workspace, kind, path)?.itemDeleted() ?? Promise.resolve()
 		},

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<ItemLoad<Res>>()
 		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<ItemLoad<Res>>()
-		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<ItemLoad<Res>>()
@@ -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

frontend/src/lib/userDraft.svelte.ts

@@ -135,13 +135,14 @@ const liveEditorDrafts = new Map<string, LiveEditorDraft>()
 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<void>
 	/** 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