From 2afa92c74ff591dbd7b7b85decd9fb07df932d15 Mon Sep 17 00:00:00 2001 From: joaomarcos Date: Fri, 10 Jul 2026 12:11:43 -0300 Subject: [PATCH] fix(desktop): pin composer draft scope to the swap-effect owner, not the render ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #54527 — a message typed into one TUI session could be silently misrouted into (or overwritten by) another concurrently-open session. Root cause: activeQueueSessionKeyRef is written on every render, but the debounced draft-persist timer, the pagehide flush, and dispatchSubmit's reject-restore path all read it lazily at async-resolve time instead of capturing the scope that was active when the operation started. A session switch landing between capture and resolve relabels one session's text under the other session's key. A large paste widens the window (slower synchronous render), which matches the original report. Fix: introduce draftScopeRef, written only by the draft-swap effect (so it always reflects the session whose text is actually loaded in the editor) and read it instead of the render-time ref at both async write sites. dispatchSubmit's restore() now uses the submittedScope already captured at dispatch instead of re-reading the live ref. Also adds isPendingDraftPersistCurrent as defense-in-depth: before the debounce timer commits a write, it verifies its captured {scope, text} pair is still the one on file. This is a no-op under the fix above (a session swap or a newer keystroke already clears/replaces the pending entry via clearTimeout), but turns any future regression that reintroduces a stale/live-ref read at this call site into a dropped write instead of a silent cross-session misroute. Co-Authored-By: Claude Sonnet 5 --- .../app/chat/composer/composer-utils.test.ts | 42 ++++++++++++++++++- .../src/app/chat/composer/composer-utils.ts | 25 +++++++++++ .../chat/composer/hooks/use-composer-draft.ts | 38 +++++++++++++++-- .../composer/hooks/use-composer-submit.ts | 6 ++- 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/composer-utils.test.ts b/apps/desktop/src/app/chat/composer/composer-utils.test.ts index 9fc5f5b5730..4df8463ba2d 100644 --- a/apps/desktop/src/app/chat/composer/composer-utils.test.ts +++ b/apps/desktop/src/app/chat/composer/composer-utils.test.ts @@ -1,7 +1,14 @@ import type { Unstable_TriggerItem } from '@assistant-ui/core' import { describe, expect, it } from 'vitest' -import { pickPlaceholder, slashArgStage, slashChipKindForItem, slashCommandToken } from './composer-utils' +import { + isPendingDraftPersistCurrent, + type PendingDraftPersist, + pickPlaceholder, + slashArgStage, + slashChipKindForItem, + slashCommandToken +} from './composer-utils' const item = (group: string): Unstable_TriggerItem => ({ id: 'x', type: 'slash', label: 'x', metadata: { group } }) as unknown as Unstable_TriggerItem @@ -38,3 +45,36 @@ describe('pickPlaceholder', () => { expect(pool).toContain(pickPlaceholder(pool)) }) }) + +describe('isPendingDraftPersistCurrent (#54527 integrity guard)', () => { + it('accepts a write when the pending entry still matches what was captured', () => { + const entry: PendingDraftPersist = { scope: 'session-a', text: 'hello' } + + expect(isPendingDraftPersistCurrent(entry, entry)).toBe(true) + expect(isPendingDraftPersistCurrent({ scope: 'session-a', text: 'hello' }, entry)).toBe(true) + }) + + it('rejects when the pending slot was cleared (session swap / newer flush already committed)', () => { + const entry: PendingDraftPersist = { scope: 'session-a', text: 'hello' } + + expect(isPendingDraftPersistCurrent(null, entry)).toBe(false) + }) + + it('rejects when the pending slot now belongs to a different session (the #54527 misroute shape)', () => { + const captured: PendingDraftPersist = { scope: 'session-a', text: 'carefully composed prompt' } + const supersededBy: PendingDraftPersist = { scope: 'session-b', text: 'different draft' } + + expect(isPendingDraftPersistCurrent(supersededBy, captured)).toBe(false) + }) + + it('rejects when the pending slot was replaced by a newer keystroke in the same session', () => { + const captured: PendingDraftPersist = { scope: 'session-a', text: 'first draft' } + const supersededBy: PendingDraftPersist = { scope: 'session-a', text: 'first draft continued' } + + expect(isPendingDraftPersistCurrent(supersededBy, captured)).toBe(false) + }) + + it('rejects when nothing was ever captured', () => { + expect(isPendingDraftPersistCurrent(null, null)).toBe(false) + }) +}) diff --git a/apps/desktop/src/app/chat/composer/composer-utils.ts b/apps/desktop/src/app/chat/composer/composer-utils.ts index ad7b63787fd..66f438f6270 100644 --- a/apps/desktop/src/app/chat/composer/composer-utils.ts +++ b/apps/desktop/src/app/chat/composer/composer-utils.ts @@ -58,3 +58,28 @@ export interface QueueEditState { } export const cloneAttachments = (attachments: ComposerAttachment[]) => attachments.map(a => ({ ...a })) + +export interface PendingDraftPersist { + scope: string | null + text: string +} + +/** + * Defense-in-depth for #54527: the debounce timer and the `pagehide` flush + * both write a captured `{ scope, text }` pair some time after it was + * scheduled. Before either commits the write, this checks the pair is still + * the one currently on file — i.e. nothing cleared or replaced it in the + * meantime (a session swap, a newer keystroke). The scope-capture fix + * upstream (`draftScopeRef`) already makes every captured pair correct by + * construction; this guard exists so that if a future change reintroduces a + * stale/live-ref read at one of these call sites, the write is dropped + * instead of silently filing one session's text under another session's key. + */ +export function isPendingDraftPersistCurrent( + pending: PendingDraftPersist | null, + expected: PendingDraftPersist | null +): boolean { + return ( + pending !== null && expected !== null && pending.scope === expected.scope && pending.text === expected.text + ) +} diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts index 5f8bcf8e233..52cb605f541 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts @@ -5,7 +5,12 @@ import { SLASH_COMMAND_RE } from '@/lib/chat-runtime' import { $composerAttachments, type ComposerAttachment, stashSessionDraft, takeSessionDraft } from '@/store/composer' import { isBrowsingHistory } from '@/store/composer-input-history' -import { cloneAttachments, DRAFT_PERSIST_DEBOUNCE_MS, type QueueEditState } from '../composer-utils' +import { + cloneAttachments, + DRAFT_PERSIST_DEBOUNCE_MS, + isPendingDraftPersistCurrent, + type QueueEditState +} from '../composer-utils' import { type ComposerInsertMode, focusComposerInput, @@ -77,6 +82,13 @@ export function useComposerDraft({ const draftPersistTimerRef = useRef(undefined) const activeQueueSessionKeyRef = useRef(activeQueueSessionKey) activeQueueSessionKeyRef.current = activeQueueSessionKey + // Owned only by the swap effect below — unlike activeQueueSessionKeyRef this + // does NOT update on every render, so it always reflects the session whose + // text is actually loaded in the editor. Async work (debounce timers, + // pagehide flush) must persist against this, not the render-time ref, or a + // session switch mid-flight files one session's draft under another's key + // (#54527). + const draftScopeRef = useRef(activeQueueSessionKey) const sessionIdRef = useRef(sessionId) sessionIdRef.current = sessionId const queueEditStateRef = useRef(queueEditRef.current) @@ -222,10 +234,20 @@ export function useComposerDraft({ return } - const scope = activeQueueSessionKeyRef.current - pendingDraftPersistRef.current = { scope, text } + const scope = draftScopeRef.current + const entry = { scope, text } + pendingDraftPersistRef.current = entry window.clearTimeout(draftPersistTimerRef.current) draftPersistTimerRef.current = window.setTimeout(() => { + // Integrity guard (defense-in-depth, #54527): only commit if this is + // still the pending write on file. A session swap or a newer + // keystroke clears/replaces it before firing in the normal case; this + // catches any future call site that skips that bookkeeping instead of + // silently filing text under the wrong session. + if (!isPendingDraftPersistCurrent(pendingDraftPersistRef.current, entry)) { + return + } + pendingDraftPersistRef.current = null stashAt(scope, text) }, DRAFT_PERSIST_DEBOUNCE_MS) @@ -285,6 +307,14 @@ export function useComposerDraft({ // never clears composer state; this effect alone stashes on leave, restores // on enter. Keyed writes are idempotent, so no skip-sentinel. useEffect(() => { + // A pending debounce timer from the outgoing session is now stale — its + // scope was correct when scheduled, but the authoritative stash below + // (and the cleanup on the way out) already covers that text. Letting it + // fire later would just clobber with an older snapshot. + window.clearTimeout(draftPersistTimerRef.current) + pendingDraftPersistRef.current = null + draftScopeRef.current = activeQueueSessionKey + const { attachments, text } = takeSessionDraft(activeQueueSessionKey) loadIntoComposer(text, attachments) @@ -304,7 +334,7 @@ export function useComposerDraft({ // inside the debounce/rAF window would drop trailing keystrokes without this. useEffect(() => { const flushPendingDraftPersist = () => { - const scope = activeQueueSessionKeyRef.current + const scope = draftScopeRef.current const editing = queueEditStateRef.current if (editing?.sessionKey === scope || isBrowsingHistory(sessionIdRef.current)) { diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts index eab822d7cd8..2dc0ef8047f 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts @@ -79,7 +79,11 @@ export function useComposerSubmit({ const restore = () => { loadIntoComposer(text, submittedAttachments) - stashAt(activeQueueSessionKeyRef.current, text, submittedAttachments) + // Use the scope captured at dispatch, not whatever session is focused + // now — the gateway can reject well after the user has switched away, + // and re-stashing into the currently-focused session would overwrite + // its draft with the rejected text from a different session (#54527). + stashAt(submittedScope, text, submittedAttachments) } void Promise.resolve(attachments ? onSubmit(text, { attachments }) : onSubmit(text))