diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index cbfde54c85..be0fbb27aa 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -1896,6 +1896,28 @@ export class AIChatManager { void this.sendRequest({ instructions: text }) } + /** Hand a refused send's text back to the user. The composer takes the text + * and clears itself before the send is attempted, so a refusal that keeps no + * copy loses what they typed. An auto-sent queued message is excluded: the + * caller reports the failure instead, which puts that one back on the queue. + */ + private restoreRefusedSend( + options: NonNullable[0]> + ): void { + if (options.queued) return + const restored = this.aiChatInput?.restoreInstructions( + options.instructions ?? '', + options.pastes ?? [], + options.images ?? [], + options.files ?? [] + ) + // No composer mounted (a programmatic send): park it on the queue so it is + // still the user's to send rather than silently gone. + if (restored !== true && options.instructions) { + this.restoreToInput(options.instructions, options.images, options.files) + } + } + /** Send the queued message, if there is one, as its own turn. Also the path a * tab takes when the turn it was watching ends in another tab: the queue was * filled here while that run held the session, and it is owed the same @@ -2845,6 +2867,15 @@ export class AIChatManager { sendUserToast('This action needs the AI chat. Start an AI session to continue.', true) return } + // Still holding a mirrored transcript against the pre-turn history: the + // other tab's run has ended but this one has not finished reading what it + // left behind. Sending now would put that stale history to the model and + // persist it over the driver's completed turn. + if (this.mirroringRemoteRun) { + this.restoreRefusedSend(options) + sendUserToast('Catching up on the turn that just finished. Try again in a moment.', true) + return false + } this.#sendsInFlight++ try { // A guarded turn re-enters this method for the sends it spawns itself @@ -2864,23 +2895,7 @@ export class AIChatManager { } }) if (outcome !== 'busy') return outcome - // Refused: the body never ran. The composer already took the text on - // submit, so hand it back rather than let the refusal eat it — an - // auto-sent queued message is excluded, since reporting the failure - // below is what puts that one back on the queue. - if (!options.queued) { - const restored = this.aiChatInput?.restoreInstructions( - options.instructions ?? '', - options.pastes ?? [], - options.images ?? [], - options.files ?? [] - ) - // No composer mounted (a programmatic send): park it on the queue so - // it is still the user's to send rather than silently gone. - if (restored !== true && options.instructions) { - this.restoreToInput(options.instructions, options.images, options.files) - } - } + this.restoreRefusedSend(options) return false } finally { this.#sendsInFlight-- diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index a4b7f3e73a..1fc4dc697c 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -304,6 +304,24 @@ describe('AIChatManager cross-tab run guard', () => { expect(saveChat).not.toHaveBeenCalled() }) + // The other tab's run has ended but this one is still reading what it left + // behind, so its transcript is mirrored while `messages` is the pre-turn + // history. A send in that window would put the stale history to the model and + // persist it over the completed turn. + it('refuses to send while still catching up on a finished remote turn', async () => { + const manager = new AIChatManager() + manager.isSessionChat = true + manager.mirroringRemoteRun = true + const restoreInstructions = vi.fn(() => true) + manager.setAiChatInput({ restoreInstructions } as any) + + const accepted = await manager.sendRequest({ instructions: 'too soon' }) + + expect(mocks.runChatLoop).not.toHaveBeenCalled() + expect(accepted).toBe(false) + expect(restoreInstructions).toHaveBeenCalledWith('too soon', [], [], []) + }) + // The composer hands its text over and clears itself before the send is even // attempted, so a refusal that keeps no copy loses what the user typed. it('hands a refused message back to the composer', async () => { diff --git a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts index ef310230d7..c3e4c53728 100644 --- a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts @@ -601,18 +601,23 @@ export default class HistoryManager { /** Re-read one chat from the store into the in-memory mirror, for a record * another tab wrote after this manager last read it. `init()` is the wrong - * tool: it re-reads the user's entire history to pick up a single chat. */ - async reloadChat(id: string): Promise { + * tool: it re-reads the user's entire history to pick up a single chat. + * + * 'missing' is a fact about the conversation (it holds nothing yet); + * 'unavailable' is a fact about this browser. Callers act on the first and + * must not act on the second — treating a closed database as an empty chat + * would throw away a transcript that is merely unreadable right now. */ + async reloadChat(id: string): Promise<'loaded' | 'missing' | 'unavailable'> { const db = await this.dbh.whenReady() - if (!db) return false + if (!db) return 'unavailable' try { const chat = await db.get('chats', id) - if (!chat) return false + if (!chat) return 'missing' this.savedChats = { ...this.savedChats, [id]: chat } - return true + return 'loaded' } catch (err) { console.error('Could not reload chat', err) - return false + return 'unavailable' } } diff --git a/frontend/src/lib/components/sessions/sessionMirror.test.ts b/frontend/src/lib/components/sessions/sessionMirror.test.ts index c06f031377..af16552b65 100644 --- a/frontend/src/lib/components/sessions/sessionMirror.test.ts +++ b/frontend/src/lib/components/sessions/sessionMirror.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { withRestoredPayloads, withoutHeavyPayloads } from './sessionMirrorPayload' +import { withoutHeavyPayloads } from './sessionMirrorPayload' import type { DisplayMessage } from '$lib/components/copilot/chat/shared' // Frames go out several times a second for the whole turn, so anything left in @@ -41,29 +41,3 @@ describe('withoutHeavyPayloads', () => { expect(withoutHeavyPayloads(messages)[0]).toBe(messages[0]) }) }) - -// A frame carries the last several messages, but only the newest are new to the -// watcher; the rest it already holds complete from IndexedDB. -describe('withRestoredPayloads', () => { - it('keeps attachments the watcher already had', () => { - const local = { - role: 'user', - content: 'look at this', - images: [{ dataUrl: 'data:image/png;base64,AAAA', mediaType: 'image/png' }], - files: [{ name: 'notes.md', content: 'the real content', id: 'f1' }], - pastes: [{ id: 1, lines: 400, content: 'the real paste' }] - } as unknown as DisplayMessage - - const [merged] = withRestoredPayloads(withoutHeavyPayloads([local]), () => local) - - expect(merged).toEqual(local) - }) - - it('leaves a message the watcher does not have yet stripped', () => { - const incoming = [ - { role: 'user', content: 'brand new', files: [{ name: 'a.md', content: '', id: 'f9' }] } - ] as unknown as DisplayMessage[] - - expect(withRestoredPayloads(incoming, () => undefined)).toEqual(incoming) - }) -}) diff --git a/frontend/src/lib/components/sessions/sessionMirrorPayload.ts b/frontend/src/lib/components/sessions/sessionMirrorPayload.ts index 4a52cbeb3f..8276223aed 100644 --- a/frontend/src/lib/components/sessions/sessionMirrorPayload.ts +++ b/frontend/src/lib/components/sessions/sessionMirrorPayload.ts @@ -49,46 +49,3 @@ export function withoutHeavyPayloads(messages: DisplayMessage[]): DisplayMessage return stripped as DisplayMessage }) } - -/** - * Put back the payloads a watcher already holds. - * - * A frame carries the last several messages, but only the newest one or two are - * ever new to the watcher — the rest it loaded from IndexedDB with attachments - * intact. Overwriting those with the stripped copies makes a screenshot from an - * earlier turn vanish the moment another tab starts a turn, and stay gone until - * the turn ends. Positional, which is what the frame's own indexing already - * assumes: a transcript only grows within a turn, and a rewrite (compaction) - * forces a full frame instead of a tail. - */ -export function withRestoredPayloads( - incoming: DisplayMessage[], - localAt: (offset: number) => DisplayMessage | undefined -): DisplayMessage[] { - return incoming.map((message, i) => { - const local = localAt(i) as Record | undefined - if (!local || local.role !== (message as any).role) return message - const merged: Record = { ...message } - let changed = false - for (const f of DROPPED_LIST_FIELDS) { - if (merged[f] === undefined && local[f]?.length) { - merged[f] = local[f] - changed = true - } - } - for (const [f, item] of BLANKED_ITEM_FIELDS) { - if (!merged[f]?.length || !local[f]?.length) continue - merged[f] = merged[f].map((v: any, j: number) => - v[item] === '' && local[f][j]?.[item] ? { ...v, [item]: local[f][j][item] } : v - ) - changed = true - } - for (const f of BLANKED_FIELDS) { - if (merged[f] === '' && local[f]) { - merged[f] = local[f] - changed = true - } - } - return (changed ? merged : message) as DisplayMessage - }) -} diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 731376a6d6..2264ac9368 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -93,7 +93,7 @@ import { getNonStreamingMetadataCompletion } from '$lib/components/copilot/lib' import { sendUserToast } from '$lib/toast' import { pendingUserAction, type DisplayMessage } from '$lib/components/copilot/chat/shared' import type { ChatCompletionMessageParam } from 'openai/resources/index.mjs' -import { withRestoredPayloads, withoutHeavyPayloads } from './sessionMirrorPayload' +import { withoutHeavyPayloads } from './sessionMirrorPayload' import { broadcastMirror, broadcastTurnEnd, @@ -1017,6 +1017,13 @@ async function initRuntime(runtime: SessionRuntime, session: Session) { // that reactive tracking of the array root would miss. const mirrorTimers = new Map>() +// Where the running turn's own messages start, per session. A frame never +// reaches below it, which is what makes overwriting safe: everything at or after +// it either is new this turn or is a stripped copy the watcher got from an +// earlier frame of this same turn, so a frame can never replace a message the +// watcher holds complete from the store. Reset to 0 by a rewrite (compaction), +// after which no index is stable and whole transcripts travel instead. +const turnStarts = new Map() // The driver's transcript length at its last frame, so a shrink is detectable. const lastSentTotals = new Map() @@ -1031,10 +1038,14 @@ function mirrorSnapshotOf(sessionId: string, full: boolean): MirrorSnapshot | un // lengths still line up. Send the whole thing instead. const rewritten = total < (lastSentTotals.get(sessionId) ?? 0) lastSentTotals.set(sessionId, total) + if (rewritten) turnStarts.set(sessionId, 0) // Slice before cloning: `$state.snapshot` walks whatever it is handed, so // snapshotting the whole transcript to send ten messages would traverse the // entire conversation on every tick. - const baseIndex = mirrorBaseIndex(total, full || rewritten) + const baseIndex = Math.max( + turnStarts.get(sessionId) ?? 0, + mirrorBaseIndex(total, full || rewritten) + ) return { sessionId, chatId: m.historyManager.getCurrentChatId(), @@ -1060,6 +1071,9 @@ function postMirror(sessionId: string, { full = false } = {}): void { function startMirroring(sessionId: string): void { if (!RUN_OWNERSHIP_AVAILABLE) return stopMirroring(sessionId) + // Captured before the turn pushes its first message, so it is the index of + // the oldest message this turn owns. + turnStarts.set(sessionId, runtimes.get(sessionId)?.manager.displayMessages.length ?? 0) postMirror(sessionId) mirrorTimers.set( sessionId, @@ -1073,6 +1087,7 @@ function stopMirroring(sessionId: string): void { clearInterval(timer) mirrorTimers.delete(sessionId) lastSentTotals.delete(sessionId) + turnStarts.delete(sessionId) // One last frame: the closing tokens of a turn usually land between ticks, // and this is what the passive tabs render until their re-read completes. postMirror(sessionId) @@ -1102,12 +1117,12 @@ function applyMirror(msg: MirrorMsg): void { m.historyManager.setCurrentChatId(msg.chatId) setSessionChatId(msg.sessionId, msg.chatId) } - // The frame's tail arrives stripped of attachment bytes; anything this tab - // already holds complete stays complete, so an earlier turn's screenshot does - // not blink out for the length of someone else's turn. - const tail = withRestoredPayloads(msg.tail, (i) => m.displayMessages[msg.baseIndex + i]) + // Safe to overwrite wholesale: a frame reaches no further back than the + // running turn's first message, so everything it replaces is either new or a + // stripped copy of its own from an earlier frame — never a message this tab + // holds complete from the store. m.displayMessages = - msg.baseIndex === 0 ? tail : [...m.displayMessages.slice(0, msg.baseIndex), ...tail] + msg.baseIndex === 0 ? msg.tail : [...m.displayMessages.slice(0, msg.baseIndex), ...msg.tail] // The frame carries the rendered transcript but not the API-format history, // so this manager is now holding a mismatched pair. Flag it: the save paths // that run outside a turn would otherwise write that pair over the record the @@ -1136,34 +1151,43 @@ async function applyTurnEnd(sessionId: string, chatId: string, committed: boolea const runtime = runtimes.get(sessionId) if (!runtime) return const m = runtime.manager - // Cleared before the re-read, not after: loadPastChat refuses to run while - // the manager looks busy, and `loading` here is the mirrored driver's. + // `loading` has to go first: loadPastChat refuses to run while the manager + // looks busy, and this one is the mirrored driver's, not a turn of our own. m.loading = false - // The re-read below restores a matched transcript/history pair. - m.mirroringRemoteRun = false m.currentReply = '' m.currentReasoning = '' m.currentReasoningActive = false m.loadingLabel = undefined m.compacting = false const id = chatId || m.historyManager.getCurrentChatId() - if (id && (await m.historyManager.reloadChat(id))) { - // `refresh`: this is the same conversation caught up from the store, so a - // message queued here while the other tab held the session is still meant - // for it — a plain load would drop it on the floor instead of sending it. - await m.loadPastChat(id, { refresh: true }) - } else if (id && id !== m.historyManager.getCurrentChatId()) { - // The driver rotated to a chat with no record yet: it ran "/clear", or its - // turn rolled back to nothing. Either way this tab's transcript and model - // history belong to the conversation just left, and keeping them would - // send that history under the new id on the next turn. - m.adoptEmptyChat(id) - setSessionChatId(sessionId, id) + try { + if (!id) return + const found = await m.historyManager.reloadChat(id) + if (found === 'loaded') { + // `refresh`: the same conversation caught up from the store, so a message + // queued here while the other tab held the session is still meant for it. + // A plain load would drop it instead of sending it below. + await m.loadPastChat(id, { refresh: true }) + } else if (found === 'missing') { + // The driver rotated to a chat that holds nothing: it ran "/clear", or its + // turn rolled back to empty. This tab's transcript and model history + // belong to the conversation just left, and keeping them would send that + // history to the model under the new id on the next turn. + m.adoptEmptyChat(id) + setSessionChatId(sessionId, id) + } + // 'unavailable' leaves everything as it is: the store is unreadable right + // now, which says nothing about the conversation. + } finally { + // Cleared only once the catch-up is done. Until then this manager still + // pairs a mirrored transcript with the pre-turn history, and the send and + // save paths gate on this flag to stay off that pair. + m.mirroringRemoteRun = false } // Anything typed here while the other tab held the session was queued rather // than sent. Send it only after a turn that landed, which is the rule a turn - // follows locally: firing it into a failed turn, or into the gap left by a - // tab that vanished, is how a follow-up ends up answering nothing. + // follows locally: firing it into a failed turn, or into the gap left by a tab + // that vanished, is how a follow-up ends up answering nothing. if (committed) await m.flushQueuedMessage() }