fix(copilot): read an artifact inside the transaction that revises it (#10647)

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

frontend/src/lib/components/copilot/chat/artifacts/artifactsDB.ts

@@ -1,6 +1,6 @@
 // Scoped by sessionId (fixed for the session's life), not chatId: a session follows its
 // active chat's rotation, so chatId-keying would drop artifacts on each new conversation.
-import { type DBSchema as IDBSchema, type IDBPObjectStore } from 'idb'
+import { type DBSchema as IDBSchema, type IDBPObjectStore, type IDBPTransaction } from 'idb'
 import { userScopedDb } from '$lib/userScopedDb'
 
 export type ArtifactKind = 'md' | 'html'
@@ -135,6 +135,11 @@ export async function listArtifactsForSession(sessionId: string): Promise<Persis
 	}
 }
 
+export interface ArtifactEdit {
+	artifact: PersistedArtifact
+	snapshots: ArtifactVersion[]
+}
+
 /**
  * Write an artifact and the snapshots that edit produced in one transaction.
  *
@@ -151,11 +156,7 @@ export async function putArtifactWithVersions(
 	if (!db) return
 	try {
 		const tx = db.transaction(['items', 'versions'], 'readwrite')
-		const versions = tx.objectStore('versions')
-		await tx.objectStore('items').put(artifact)
-		for (const entry of snapshots) await versions.put(entry)
-		const newest = snapshots.at(-1)
-		if (newest) await pruneVersionsIn(versions, artifact.id, newest.content.length)
+		await writeEdit(tx.objectStore('items'), tx.objectStore('versions'), { artifact, snapshots })
 		await tx.done
 	} catch (err) {
 		// A rejected write (most likely QuotaExceededError) leaves the artifact usable for the
@@ -164,6 +165,97 @@ export async function putArtifactWithVersions(
 	}
 }
 
+async function writeEdit(
+	items: ItemsStore,
+	versions: VersionsStore,
+	edit: ArtifactEdit
+): Promise<void> {
+	await items.put(edit.artifact)
+	for (const entry of edit.snapshots) await versions.put(entry)
+	const newest = edit.snapshots.at(-1)
+	if (newest) await pruneVersionsIn(versions, edit.artifact.id, newest.content.length)
+}
+
+/**
+ * Read an artifact and write it back in one transaction. `mutate` returns the edit to
+ * write, or undefined to leave the artifact alone and resolve to undefined. A store that
+ * fails is reported rather than thrown, so the edited row resolves either way — persisted
+ * where it could be, and usable for the session where it could not.
+ */
+export async function mutateArtifact(
+	id: string,
+	mutate: (existing: PersistedArtifact | undefined) => ArtifactEdit | undefined
+): Promise<PersistedArtifact | undefined> {
+	const db = await getDB()
+	// `transaction()` throws on a connection closed since `getDB()` answered — another tab
+	// upgrading the schema, or a user switch releasing the handle.
+	let opened: IDBPTransaction<ArtifactsSchema, ('items' | 'versions')[], 'readwrite'> | undefined
+	try {
+		opened = db?.transaction(['items', 'versions'], 'readwrite')
+	} catch (err) {
+		console.error('Could not open an artifact write transaction', err)
+	}
+	// Not `return undefined`: `create` can hand out an artifact the store never took, and it
+	// stays revisable only if the edit is computed anyway.
+	if (!opened) return mutate(undefined)?.artifact
+	const tx = opened
+	// Attached before the first await: idb builds `done` eagerly and rejects it on abort, so
+	// attaching later would leave an unhandled rejection. Cleared before each deliberate
+	// abort below, whose own site reports the failure when there was one.
+	let reportFailure = true
+	const settled = tx.done.catch((err) => {
+		if (reportFailure) console.error('Could not persist artifact', err)
+	})
+	const abort = () => {
+		try {
+			tx.abort()
+		} catch {}
+	}
+	const items = tx.objectStore('items')
+	const versions = tx.objectStore('versions')
+	let existing: PersistedArtifact | undefined
+	try {
+		// Read outside this transaction, two tabs both see version N, both stamp N+1, and the
+		// later write silently replaces the earlier one — content and snapshot alike.
+		existing = await items.get(id)
+	} catch (err) {
+		console.error('Could not read the artifact being written', err)
+		reportFailure = false
+		abort()
+		await settled
+		return mutate(undefined)?.artifact
+	}
+	// Kept out of the store's own error handling: a mutator that fails is not the store
+	// failing, so its error is neither reported as one nor swallowed.
+	let edit: ArtifactEdit | undefined
+	try {
+		edit = mutate(existing)
+	} catch (err) {
+		reportFailure = false
+		abort()
+		await settled
+		throw err
+	}
+	if (!edit) {
+		reportFailure = false
+		abort()
+		await settled
+		return undefined
+	}
+	try {
+		await writeEdit(items, versions, edit)
+	} catch (err) {
+		// A request that errors aborts the transaction itself; one that throws before creating
+		// a request (DataCloneError) would otherwise commit the row without its snapshot.
+		// Logged here because `settled` sees only a cause-less AbortError.
+		console.error('Could not persist artifact', err)
+		reportFailure = false
+		abort()
+	}
+	await settled
+	return edit.artifact
+}
+
 async function pruneVersionsIn(
 	store: VersionsStore,
 	artifactId: string,
@@ -242,6 +334,8 @@ export async function deleteArtifactsForSession(sessionId: string): Promise<void
 	}
 }
 
+type ItemsStore = IDBPObjectStore<ArtifactsSchema, ('items' | 'versions')[], 'items', 'readwrite'>
+
 type VersionsStore = IDBPObjectStore<
 	ArtifactsSchema,
 	('items' | 'versions')[],

frontend/src/lib/components/copilot/chat/artifacts/artifactsState.svelte.ts

@@ -6,6 +6,7 @@ import {
 	getArtifactVersion,
 	listArtifactVersions,
 	listArtifactsForSession,
+	mutateArtifact,
 	putArtifactWithVersions,
 	versionKey,
 	type ArtifactKind,
@@ -113,31 +114,38 @@ export class SessionArtifactsStore {
 		input: UpdateArtifactInput,
 		opts?: { sessionId?: string }
 	): Promise<PersistedArtifact | undefined> {
-		const existing = this.artifacts.find((a) => a.id === id) ?? (await getArtifact(id))
-		if (!existing) return undefined
-		if (opts?.sessionId !== undefined && existing.sessionId !== opts.sessionId) return undefined
-		// Only a content change earns a version: a rename or an identical rewrite would
-		// otherwise fill the picker with entries the user cannot tell apart.
-		const contentChanged = input.content !== undefined && input.content !== existing.content
-		const version = currentVersion(existing) + (contentChanged ? 1 : 0)
-		const updated: PersistedArtifact = {
-			...existing,
-			name: input.name ?? existing.name,
-			content: input.content ?? existing.content,
-			updatedAt: Date.now(),
-			version
-		}
-		const snapshots: ArtifactVersion[] = []
-		// An artifact written before history existed has no snapshot of its current content,
-		// so capture one on *any* update, not just a content change: this write stamps
-		// `version`, and nothing afterwards would recognise it as pre-history.
-		if (existing.version === undefined) {
-			snapshots.push(snapshotOf(existing, currentVersion(existing)))
-		}
-		if (contentChanged) {
-			snapshots.push(snapshotOf(updated, version, input.note))
-		}
-		await putArtifactWithVersions(updated, snapshots)
+		const updated = await mutateArtifact(id, (stored) => {
+			// Read inside the mutator: hoisted out, it would weigh a stale copy against a fresh one.
+			const existing = furtherAlong(
+				stored,
+				this.artifacts.find((a) => a.id === id)
+			)
+			if (!existing) return undefined
+			if (opts?.sessionId !== undefined && existing.sessionId !== opts.sessionId) return undefined
+			// Only a content change earns a version: a rename or an identical rewrite would
+			// otherwise fill the picker with entries the user cannot tell apart.
+			const contentChanged = input.content !== undefined && input.content !== existing.content
+			const version = currentVersion(existing) + (contentChanged ? 1 : 0)
+			const artifact: PersistedArtifact = {
+				...existing,
+				name: input.name ?? existing.name,
+				content: input.content ?? existing.content,
+				updatedAt: Date.now(),
+				version
+			}
+			const snapshots: ArtifactVersion[] = []
+			// An artifact written before history existed has no snapshot of its current content,
+			// so capture one on *any* update, not just a content change: this write stamps
+			// `version`, and nothing afterwards would recognise it as pre-history.
+			if (existing.version === undefined) {
+				snapshots.push(snapshotOf(existing, currentVersion(existing)))
+			}
+			if (contentChanged) {
+				snapshots.push(snapshotOf(artifact, version, input.note))
+			}
+			return { artifact, snapshots }
+		})
+		if (!updated) return undefined
 		if (updated.sessionId === this.#sessionId) {
 			this.#applyWrite(sortByUpdatedDesc(this.artifacts.map((a) => (a.id === id ? updated : a))))
 		}
@@ -187,6 +195,24 @@ export class SessionArtifactsStore {
 	}
 }
 
+/**
+ * Neither copy is authoritative: only the store carries an edit another tab made, and only
+ * memory carries one the store refused (quota) and `update` handed back unpersisted.
+ * Always preferring one side reverts the other's text on the next edit, so the later wins.
+ */
+function furtherAlong(
+	stored: PersistedArtifact | undefined,
+	held: PersistedArtifact | undefined
+): PersistedArtifact | undefined {
+	if (!stored || !held) return stored ?? held
+	const heldVersion = currentVersion(held)
+	const storedVersion = currentVersion(stored)
+	// A rename earns no version, so at equal versions only the clock separates the two.
+	// Both are written by the same browser, which is what makes the stamps comparable.
+	if (heldVersion === storedVersion) return held.updatedAt > stored.updatedAt ? held : stored
+	return heldVersion > storedVersion ? held : stored
+}
+
 function snapshotOf(a: PersistedArtifact, version: number, note?: string): ArtifactVersion {
 	return {
 		key: versionKey(a.id, version),

frontend/src/lib/components/copilot/chat/artifacts/artifactsState.test.ts

@@ -143,6 +143,55 @@ describe('SessionArtifactsStore', () => {
 		expect(await store.update('nope', { content: 'x' })).toBeUndefined()
 	})
 
+	it('gives two tabs editing at once distinct versions, keeping both snapshots', async () => {
+		// Without the transactional read both tabs stamp the same next version, and one edit
+		// and its snapshot vanish under the other.
+		const other = new (await import('./artifactsState.svelte')).SessionArtifactsStore()
+		await store.setSession('s1')
+		const created = await store.create('s1', { name: 'Doc', content: 'v1' })
+		await other.setSession('s1')
+
+		await Promise.all([
+			store.update(created.id, { content: 'from A', note: 'A' }),
+			other.update(created.id, { content: 'from B', note: 'B' })
+		])
+
+		expect((await dbMod.listArtifactVersions(created.id)).map((v) => v.version)).toEqual([3, 2, 1])
+		expect((await dbMod.getArtifact(created.id))?.version).toBe(3)
+	})
+
+	it('keeps an edit the store refused when the next update lands', async () => {
+		await store.setSession('s1')
+		const created = await store.create('s1', { name: 'Doc', content: 'v1' })
+
+		// Refused, so v2 lives only in memory while the stored row stays behind at v1.
+		const quiet = vi.spyOn(console, 'error').mockImplementation(() => {})
+		const put = vi.spyOn(IDBObjectStore.prototype, 'put').mockImplementationOnce(() => {
+			throw new DOMException('quota', 'QuotaExceededError')
+		})
+		await store.update(created.id, { content: 'v2' })
+		put.mockRestore()
+		quiet.mockRestore()
+		expect((await dbMod.getArtifact(created.id))?.content).toBe('v1')
+
+		// Computed from the stored row, this rename would put v1's text back under the new name.
+		expect(await store.update(created.id, { name: 'Renamed' })).toMatchObject({
+			name: 'Renamed',
+			content: 'v2'
+		})
+	})
+
+	it('creates and then revises an artifact when there is no store to write to', async () => {
+		// No scoped user, so the database never opens. `create` hands the id out either way, so
+		// the document it named has to stay revisable rather than come back as an unknown one.
+		;(await import('$lib/stores')).userStore.set(undefined as never)
+		await store.setSession('s1')
+		const created = await store.create('s1', { name: 'A', content: 'x' })
+
+		expect((await store.update(created.id, { content: 'y' }))?.content).toBe('y')
+		expect(store.artifacts.map((a) => a.content)).toEqual(['y'])
+	})
+
 	it('get resolves from the in-memory list even when the DB lacks the record', async () => {
 		await store.setSession('s1')
 		const created = await store.create('s1', { name: 'A', content: 'x' })
@@ -213,7 +262,7 @@ describe('SessionArtifactsStore', () => {
 		await store.setSession('s1')
 		const created = await store.create('s1', { name: 'Doc', content: 'c1' })
 
-		const spy = vi.spyOn(dbMod, 'putArtifactWithVersions')
+		const spy = vi.spyOn(dbMod, 'mutateArtifact')
 		await store.update(created.id, { content: 'c2', note: 'second' })
 
 		// Split into two writes, a stamped version can outlive the snapshot that failed to
@@ -221,8 +270,8 @@ describe('SessionArtifactsStore', () => {
 		// synthesizes the current version from the row — right up until the next edit
 		// overwrites the row, which is the only copy of that content left.
 		expect(spy).toHaveBeenCalledTimes(1)
-		expect(spy.mock.calls[0][1].map((v) => v.version)).toEqual([2])
 		spy.mockRestore()
+		expect((await store.listVersions(created.id)).map((v) => v.version)).toEqual([2, 1])
 
 		const row = await dbMod.getArtifact(created.id)
 		const stored = await dbMod.listArtifactVersions(created.id)