diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx b/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx new file mode 100644 index 000000000000..30146d30493c --- /dev/null +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx @@ -0,0 +1,128 @@ +import { act, cleanup, render } from '@testing-library/react' +import { useLayoutEffect } from 'react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { mainComposerScope, stashSessionDraft, type ComposerAttachment } from '@/store/composer' + +import type { QueueEditState } from '../composer-utils' +import { useComposerDraft } from './use-composer-draft' + +const mockComposerApi = { setText: vi.fn() } + +vi.mock('@assistant-ui/react', () => ({ + useAui: () => ({ composer: () => mockComposerApi }), + useAuiState: (selector: (state: { composer: { text: string } }) => unknown) => + selector({ composer: { text: '' } }), + useComposerRuntime: () => ({ + getState: () => ({ text: '' }), + subscribe: () => () => undefined + }) +})) + +interface ProbeHarnessProps { + activeQueueSessionKey: string | null + onLayoutSnapshot: (attachments: ComposerAttachment[]) => void + sessionId: string +} + +function ProbeHarness({ activeQueueSessionKey, onLayoutSnapshot, sessionId }: ProbeHarnessProps) { + useComposerDraft({ + activeQueueSessionKey, + focusKey: null, + inputDisabled: false, + queueEditRef: { current: null as QueueEditState | null }, + sessionId + }) + + // useLayoutEffect fires synchronously right after the DOM commit, BEFORE + // the hook's per-thread scope-swap useEffect (a passive effect) has a + // chance to swap attachmentScope.$attachments over to the new session. A + // synchronous read here — the same read ChatBar's `attachments` prop + // performs at render time — observes the OUTGOING session's attachments. + useLayoutEffect(() => { + onLayoutSnapshot(mainComposerScope.$attachments.get()) + }) + + return null +} + +describe('useComposerDraft — attachment scope stays coherent with the committed session on switch (#59305)', () => { + afterEach(() => { + cleanup() + mainComposerScope.clear() + }) + + it('clears the outgoing session attachments by the layout phase right after switching sessions', () => { + const attachmentA: ComposerAttachment = { id: 'url-A', kind: 'url', label: 'A' } + stashSessionDraft('session-A', 'hi from A', [attachmentA]) + + const snapshots: ComposerAttachment[][] = [] + + const { rerender } = render( + snapshots.push(s)} + sessionId="session-A" + /> + ) + + // Mount loads session A's stashed attachment into the (module-level) main + // scope — confirms the fixture actually seeded the leak precondition. + expect(mainComposerScope.$attachments.get()).toEqual([attachmentA]) + + snapshots.length = 0 // drop the initial-mount snapshot; only the switch matters + + act(() => { + rerender( + snapshots.push(s)} + sessionId="session-B" + /> + ) + }) + + // By the layout phase the scope must already be B's (empty) — a submit + // fired the instant B renders must never ship session A's attachment. + expect(snapshots[0]).toEqual([]) + }) +}) + +describe('useComposerDraft — rehydrate diagnostic log stays redacted', () => { + afterEach(() => { + cleanup() + mainComposerScope.clear() + vi.restoreAllMocks() + }) + + it('logs counts/kinds/scope on restore but never the raw url, refText, or label', () => { + const secretUrl = 'https://secret.example.com/private-workspace-path' + const attachment: ComposerAttachment = { + id: 'url-secret', + kind: 'url', + label: 'do-not-leak-label', + refText: `@url:${secretUrl}` + } + stashSessionDraft('session-secret', '', [attachment]) + + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => undefined) + + render( + undefined} sessionId="session-secret" /> + ) + + const rehydrateCalls = debugSpy.mock.calls.filter(call => call[0] === '[composer-rehydrate]') + expect(rehydrateCalls.length).toBeGreaterThan(0) + + const serialized = JSON.stringify(rehydrateCalls) + expect(serialized).not.toContain(secretUrl) + expect(serialized).not.toContain(attachment.label) + expect(serialized).not.toContain(attachment.refText) + + expect(rehydrateCalls[0]?.[1]).toMatchObject({ + attachmentCount: 1, + attachmentKinds: ['url'], + scope: 'session-secret' + }) + }) +}) 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 11dc9534df0f..ab3281b8a99c 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 @@ -1,5 +1,5 @@ import { useAui, useAuiState, useComposerRuntime } from '@assistant-ui/react' -import { type RefObject, useCallback, useEffect, useRef, useState } from 'react' +import { type RefObject, useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react' import { SLASH_COMMAND_RE } from '@/lib/chat-runtime' import { type ComposerAttachment, stashSessionDraft, takeSessionDraft } from '@/store/composer' @@ -20,7 +20,7 @@ import { onComposerInsertRequest } from '../focus' import { type InlineRefInput, insertInlineRefsIntoEditor } from '../inline-refs' -import { composerPlainText, placeCaretEnd, renderComposerContents } from '../rich-editor' +import { composerPlainText, placeCaretEnd, REF_RE, renderComposerContents } from '../rich-editor' import { useComposerScope } from '../scope' import type { ChatBarProps } from '../types' @@ -189,6 +189,21 @@ export function useComposerDraft({ stashSessionDraft(scope, text, attachments) const loadIntoComposer = (text: string, attachments: ComposerAttachment[]) => { + // Diagnostic breadcrumb for #59305-class reports: identifies WHAT kind of + // state got restored into the composer (session switch, queue-edit + // restore, history browse) without logging any raw content. REF_RE has the + // global flag — testing against a throwaway clone avoids mutating the + // shared instance's lastIndex, which would otherwise corrupt this check on + // the next call. + if (attachments.length > 0 || new RegExp(REF_RE.source, REF_RE.flags).test(text)) { + console.debug('[composer-rehydrate]', { + attachmentCount: attachments.length, + attachmentKinds: attachments.map(a => a.kind), + hasTextRefs: new RegExp(REF_RE.source, REF_RE.flags).test(text), + scope: activeQueueSessionKeyRef.current + }) + } + attachmentScope.$attachments.set(cloneAttachments(attachments)) paintDraft(text, false) } @@ -318,7 +333,16 @@ export function useComposerDraft({ // Per-thread draft swap — the composer's only session coupling. Lifecycle // never clears composer state; this effect alone stashes on leave, restores // on enter. Keyed writes are idempotent, so no skip-sentinel. - useEffect(() => { + // + // MUST be a layout effect, not a passive one: it swaps attachmentScope's + // module-level $attachments atom, and a passive effect fires only after the + // browser paints the new session's view — leaving a window where the DOM + // already shows session B while $attachments (and therefore ChatBar's + // `attachments` prop) still holds session A's chips. A submit fired in that + // window (e.g. a fast session switch immediately followed by Enter) would + // ship A's attachments into B's turn (#59305). useLayoutEffect closes the + // window by running before paint. + useLayoutEffect(() => { // 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 diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx index a09cd10ef29d..b3bcaaa463e3 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx @@ -113,7 +113,9 @@ describe('useComposerSubmit busy-turn routing', () => { hook.result.current.submitDraft() }) - await waitFor(() => expect(onSubmit).toHaveBeenCalledWith('/compress preserve context')) + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith('/compress preserve context', { composerScope: 'stored-session' }) + ) expect(clearDraft).toHaveBeenCalledTimes(1) expect(onSteer).not.toHaveBeenCalled() expect(queueCurrentDraft).not.toHaveBeenCalled() @@ -159,9 +161,26 @@ describe('useComposerSubmit busy-turn routing', () => { hook.result.current.submitDraft() }) - await waitFor(() => expect(onSubmit).toHaveBeenCalledWith('ordinary question', { attachments: [] })) + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith('ordinary question', { + attachments: [], + composerScope: 'stored-session' + }) + ) expect(onSteer).not.toHaveBeenCalled() expect(queueCurrentDraft).not.toHaveBeenCalled() expect(onCancel).not.toHaveBeenCalled() }) + + it('threads the loaded composer scope through onSubmit for the #59305 submit-time guard', async () => { + const { hook, onSubmit } = renderSubmitHook({ text: 'hello' }) + + act(() => { + hook.result.current.submitDraft() + }) + + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith('hello', expect.objectContaining({ composerScope: 'stored-session' })) + ) + }) }) 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 fb1cec2cf04a..315157a96e55 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 @@ -89,7 +89,11 @@ export function useComposerSubmit({ stashAt(submittedScope, text, submittedAttachments) } - void Promise.resolve(attachments ? onSubmit(text, { attachments }) : onSubmit(text)) + void Promise.resolve( + attachments + ? onSubmit(text, { attachments, composerScope: submittedScope }) + : onSubmit(text, { composerScope: submittedScope }) + ) .then(accepted => void (accepted === false ? restore() : clearSessionDraft(submittedScope))) .catch(restore) } diff --git a/apps/desktop/src/app/chat/index.tsx b/apps/desktop/src/app/chat/index.tsx index bd18be0c4834..8f7a2670a97c 100644 --- a/apps/desktop/src/app/chat/index.tsx +++ b/apps/desktop/src/app/chat/index.tsx @@ -42,7 +42,7 @@ import { import { isSecondaryWindow, isWatchWindow } from '@/store/windows' import type { ModelOptionsResponse } from '@/types/hermes' -import { routeSessionId } from '../routes' +import { primaryRouteSelectedSessionId, routeSessionId } from '../routes' import { titlebarHeaderBaseClass, titlebarHeaderShadowClass, titlebarHeaderTitleClass } from '../shell/titlebar' import { ChatDropOverlay } from './chat-drop-overlay' @@ -285,11 +285,18 @@ export function ChatView({ const resumeExhaustedSessionId = useStore($resumeExhaustedSessionId) // Durable composer/queue scope (lineage root) so auto-compression tip rotation - // does not wipe an in-progress draft or orphan /queue entries. - const queueSessionKey = useMemo( - () => resolveComposerSessionKey(selectedSessionId, sessions), - [selectedSessionId, sessions] - ) + // does not wipe an in-progress draft or orphan /queue entries. For the + // primary view, the route is authoritative over the store selection — the + // latter can be momentarily null/stale mid-switch, which used to leak into + // the composer's scope key (#59305). A tile has no route, so it always uses + // its own selection directly. + const queueSessionKey = useMemo(() => { + const effectiveSelectedSessionId = isPrimary + ? primaryRouteSelectedSessionId(location.pathname, selectedSessionId) + : selectedSessionId + + return resolveComposerSessionKey(effectiveSelectedSessionId, sessions) + }, [isPrimary, location.pathname, selectedSessionId, sessions]) // When the tip row arrives after compression, migrate any tip-keyed stash onto // the durable lineage key before the composer remounts onto that key. diff --git a/apps/desktop/src/app/routes.test.ts b/apps/desktop/src/app/routes.test.ts new file mode 100644 index 000000000000..3c8933d93563 --- /dev/null +++ b/apps/desktop/src/app/routes.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest' + +import { NEW_CHAT_ROUTE, primaryRouteSelectedSessionId, sessionRoute, SETTINGS_ROUTE } from './routes' + +const SESS_A = 'sess-a' +const SESS_B = 'sess-b' + +describe('primaryRouteSelectedSessionId', () => { + it('prefers the routed session id over a stale/different store selection (#59305)', () => { + // The route already committed to B while the store selection hasn't + // caught up yet (still reads A) — the route wins. + expect(primaryRouteSelectedSessionId(sessionRoute(SESS_B), SESS_A)).toBe(SESS_B) + }) + + it('returns null on the new-chat route even with a leftover selection from the previous chat', () => { + expect(primaryRouteSelectedSessionId(NEW_CHAT_ROUTE, SESS_A)).toBeNull() + }) + + it('falls back to the store selection on a non-chat route (settings, overlays)', () => { + expect(primaryRouteSelectedSessionId(SETTINGS_ROUTE, SESS_A)).toBe(SESS_A) + }) + + it('falls back to the store selection when the route matches the same session', () => { + expect(primaryRouteSelectedSessionId(sessionRoute(SESS_A), SESS_A)).toBe(SESS_A) + }) + + it('returns null on a non-chat route with no store selection', () => { + expect(primaryRouteSelectedSessionId(SETTINGS_ROUTE, null)).toBeNull() + }) +}) diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index 8965cc6a27a0..b3c28a0ae017 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -147,6 +147,23 @@ export function routeSessionId(pathname: string): string | null { return id && !id.includes('/') ? decodeURIComponent(id) : null } +/** + * The primary composer's durable scope key candidate: the route is the source + * of truth for which chat is on screen, so prefer its (stable) stored session + * id over a store selection that can be momentarily null/stale mid-switch + * (#59305). A genuine new-chat route always wins with `null`, never falling + * back to a leftover selection from the chat just left. A non-chat route + * (settings, an overlay) has no session opinion, so the store selection passes + * through unchanged. + */ +export function primaryRouteSelectedSessionId(pathname: string, storeSelectedSessionId: string | null): string | null { + if (isNewChatRoute(pathname)) { + return null + } + + return routeSessionId(pathname) ?? storeSelectedSessionId +} + export function sessionRoute(sessionId: string): string { return `${SESSION_ROUTE_PREFIX}${encodeURIComponent(sessionId)}` } diff --git a/apps/desktop/src/app/session/hooks/session-context-drift.test.ts b/apps/desktop/src/app/session/hooks/session-context-drift.test.ts index 7846ecfe221b..009d719e5e57 100644 --- a/apps/desktop/src/app/session/hooks/session-context-drift.test.ts +++ b/apps/desktop/src/app/session/hooks/session-context-drift.test.ts @@ -122,4 +122,77 @@ describe('sessionContextDrift', () => { expect(reason).toBe('route:__new__->sess-b') }) + + it('does not drift when composerScope is omitted (non-composer submits: queue drain, steer)', () => { + const reason = sessionContextDrift({ + startRouteToken: routeToken(sessionRoute(SESS_A)), + nowRouteToken: routeToken(sessionRoute(SESS_A)), + startSelectedStoredId: SESS_A, + nowSelectedStoredId: SESS_A, + submitTargetStoredId: SESS_A + }) + + expect(reason).toBeNull() + }) + + it('does not drift when composerScope matches the resolved (lineage) submit target', () => { + const reason = sessionContextDrift({ + startRouteToken: routeToken(sessionRoute(SESS_A)), + nowRouteToken: routeToken(sessionRoute(SESS_A)), + startSelectedStoredId: SESS_A, + nowSelectedStoredId: SESS_A, + submitTargetStoredId: SESS_A, + composerScope: SESS_A, + submitTargetComposerScope: SESS_A + }) + + expect(reason).toBeNull() + }) + + it('drifts (composer prong) when the loaded composer scope disagrees with the resolved submit target (#59305)', () => { + const reason = sessionContextDrift({ + startRouteToken: routeToken(sessionRoute(SESS_A)), + nowRouteToken: routeToken(sessionRoute(SESS_A)), + startSelectedStoredId: SESS_A, + nowSelectedStoredId: SESS_A, + submitTargetStoredId: SESS_A, + composerScope: SESS_B, + submitTargetComposerScope: SESS_A + }) + + expect(reason).toBe('composer:sess-b->sess-a') + }) + + it('checks the composer prong before the route/selection prongs', () => { + const reason = sessionContextDrift({ + startRouteToken: routeToken(sessionRoute(SESS_A)), + nowRouteToken: routeToken(sessionRoute(SESS_B)), + startSelectedStoredId: SESS_A, + nowSelectedStoredId: SESS_B, + submitTargetStoredId: SESS_A, + composerScope: SESS_B, + submitTargetComposerScope: SESS_A + }) + + expect(reason).toBe('composer:sess-b->sess-a') + }) + + it('does not drift when the session has rotated via compression (composerScope is the lineage root, submitTargetStoredId is the live tip)', () => { + const ROOT_ID = 'stored-root' + const TIP_ID = 'stored-tip-after-compression' + + const reason = sessionContextDrift({ + startRouteToken: routeToken(sessionRoute(TIP_ID)), + nowRouteToken: routeToken(sessionRoute(TIP_ID)), + startSelectedStoredId: TIP_ID, + nowSelectedStoredId: TIP_ID, + submitTargetStoredId: TIP_ID, + composerScope: ROOT_ID, + // What submit.ts actually passes: resolveComposerSessionKey(TIP_ID, sessions), + // which resolves to the lineage root for a session that has compressed. + submitTargetComposerScope: ROOT_ID + }) + + expect(reason).toBeNull() + }) }) diff --git a/apps/desktop/src/app/session/hooks/session-context-drift.ts b/apps/desktop/src/app/session/hooks/session-context-drift.ts index 5f7e8a54b91d..7f264a581f9f 100644 --- a/apps/desktop/src/app/session/hooks/session-context-drift.ts +++ b/apps/desktop/src/app/session/hooks/session-context-drift.ts @@ -37,6 +37,25 @@ interface SessionContextDriftArgs { * a real chat as drift. */ submitTargetStoredId?: string | null + /** + * The composer scope that was actually loaded when the text was submitted + * (SubmitTextOptions.composerScope). The composer and the session-side refs + * live in separate React subtrees and can each be internally consistent yet + * still disagree with each other at the instant of send — this prong catches + * that cross-component drift (#59305). Omit for non-composer submits. + */ + composerScope?: string | null + /** + * resolveComposerSessionKey(submitTargetStoredId, sessions) — the durable + * lineage-root form of the submit target, in the SAME domain as + * composerScope. Compared against composerScope instead of the raw + * submitTargetStoredId: the composer keys drafts/attachments on the lineage + * root (stable across auto-compression tip rotation) while + * submitTargetStoredId tracks the live tip — comparing composerScope + * directly against the tip would false-positive-abort every submit into any + * session that has ever compressed. + */ + submitTargetComposerScope?: string | null } /** @@ -56,8 +75,21 @@ export function sessionContextDrift({ nowRouteToken, startSelectedStoredId, nowSelectedStoredId, - submitTargetStoredId + submitTargetStoredId, + composerScope, + submitTargetComposerScope }: SessionContextDriftArgs): string | null { + // Composer prong: the composer's loaded scope disagrees with the resolved + // submit target. Not a start/now comparison like the two prongs below — the + // composer only hands us one snapshot per submit — but it belongs in the + // same fail-closed gate since it's exactly the same "wrong session" failure + // mode. Compared against submitTargetComposerScope (lineage-pinned), NOT + // submitTargetStoredId (live tip) — see the field doc on + // SessionContextDriftArgs for why those two must not be conflated. + if (composerScope !== undefined && composerScope !== null && composerScope !== submitTargetComposerScope) { + return `composer:${composerScope}->${submitTargetComposerScope}` + } + const targetStart = routeTargetFromToken(startRouteToken) const targetNow = routeTargetFromToken(nowRouteToken) diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx index 99f00a25a624..a73797b060e2 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx @@ -2458,6 +2458,7 @@ describe('usePromptActions submit session-context isolation (#54527)', () => { afterEach(() => { cleanup() vi.restoreAllMocks() + setSessions(() => []) }) it('aborts submit when the user switches sessions during session.resume (no misroute)', async () => { @@ -2515,6 +2516,100 @@ describe('usePromptActions submit session-context isolation (#54527)', () => { }) }) + it('does not false-positive-abort when the session has rotated via compression (lineage root vs tip)', async () => { + // The composer keys drafts/attachments on the DURABLE lineage root + // (resolveComposerSessionKey / sessionPinId — survives auto-compression + // tip rotation), but selectedStoredSessionIdRef tracks the CURRENT TIP. + // For any session that has compressed at least once, root !== tip — if + // composerScope is compared against the raw tip, every legitimate submit + // into that session would look like drift. + const ROOT_ID = 'stored-root-original' + const TIP_ID = 'stored-tip-after-compression' + + setSessions(() => [sessionInfo({ id: TIP_ID, _lineage_root_id: ROOT_ID })]) + + const calls: { method: string; params?: Record }[] = [] + + const requestGateway = vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params }) + + return {} as never + }) + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + storedSessionId={TIP_ID} + /> + ) + + // The composer's scope is the lineage root (what resolveComposerSessionKey + // actually returns for this session) — a legitimate, non-drifted submit. + const ok = await handle!.submitText('message into the rotated session', { composerScope: ROOT_ID }) + + expect(ok).toBe(true) + expect(calls.some(c => c.method === 'prompt.submit')).toBe(true) + }) + + it('aborts submit when the composer scope disagrees with the resolved target (#59305)', async () => { + // The composer (ChatBar) and the session-side refs live in separate React + // subtrees; each can be internally consistent yet still disagree with each + // other at the instant of send if the two updated on different commits. + // composerScope carries the composer's own snapshot of "what session was + // loaded" into submit.ts, which must refuse to send when it disagrees with + // the session the submit is actually about to target. + const calls: { method: string; params?: Record }[] = [] + + const requestGateway = vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params }) + + return {} as never + }) + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + storedSessionId={STORED_SESSION_A} + /> + ) + + const ok = await handle!.submitText('typed while B was on screen', { composerScope: STORED_SESSION_B }) + + expect(ok).toBe(false) + expect(calls.some(c => c.method === 'prompt.submit')).toBe(false) + }) + + it('submits normally when the composer scope agrees with the resolved target', async () => { + const calls: { method: string; params?: Record }[] = [] + + const requestGateway = vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params }) + + return {} as never + }) + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + storedSessionId={STORED_SESSION_A} + /> + ) + + const ok = await handle!.submitText('typed while A was on screen', { composerScope: STORED_SESSION_A }) + + expect(ok).toBe(true) + expect(calls.some(c => c.method === 'prompt.submit')).toBe(true) + }) + it('aborts recovery submit when the user switches sessions during timeout resume', async () => { const calls: { method: string; params?: Record }[] = [] let submitAttempts = 0 diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts index 398a72f3be4a..dc7abd08bb73 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts @@ -20,7 +20,7 @@ import { } from '@/store/composer' import { clearNotifications, notify, notifyError } from '@/store/notifications' import { requestDesktopOnboarding } from '@/store/onboarding' -import { setAwaitingResponse, setBusy, setMessages } from '@/store/session' +import { $sessions, resolveComposerSessionKey, setAwaitingResponse, setBusy, setMessages } from '@/store/session' import type { ClientSessionState } from '../../../types' import { sessionContextDrift } from '../session-context-drift' @@ -210,7 +210,15 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) { nowRouteToken: getRouteToken(), startSelectedStoredId: startingStoredSessionId, nowSelectedStoredId: selectedStoredSessionIdRef.current, - submitTargetStoredId: startingStoredSessionId + submitTargetStoredId: startingStoredSessionId, + composerScope: options?.composerScope, + // The composer keys drafts/attachments on the durable lineage + // root (survives auto-compression tip rotation), while + // startingStoredSessionId is the live tip — resolve the target + // into the same lineage-root domain before comparing, or every + // submit into a session that has ever compressed would + // false-positive-abort. + submitTargetComposerScope: resolveComposerSessionKey(startingStoredSessionId, $sessions.get()) }) : null diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts index 42598fb54627..ad9167ebb050 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts @@ -359,6 +359,14 @@ export function visibleUserIndexAtOrdinal(messages: readonly ChatMessage[], targ export interface SubmitTextOptions { attachments?: ComposerAttachment[] + /** The composer scope key that was actually loaded when this text was + * submitted (see use-composer-draft's activeQueueSessionKeyRef). Compared + * against the resolved submit target in sessionContextDrift — a mismatch + * means the composer and the session-side refs disagreed about which + * session this send belongs to (#59305). Omit for non-composer submits + * (queue drain, steer, external submit requests): the check is a no-op + * without it. */ + composerScope?: string | null fromQueue?: boolean /** Runtime session id to submit into. Queue drains pass this so a * backgrounded/source session cannot be replaced by the current foreground diff --git a/apps/desktop/src/app/session/hooks/use-session-state-cache.test.tsx b/apps/desktop/src/app/session/hooks/use-session-state-cache.test.tsx index c2d5a0d0376b..f96307f361cf 100644 --- a/apps/desktop/src/app/session/hooks/use-session-state-cache.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-state-cache.test.tsx @@ -1,5 +1,5 @@ import { act, cleanup, render } from '@testing-library/react' -import type { MutableRefObject } from 'react' +import { type MutableRefObject, useLayoutEffect } from 'react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { ChatMessage } from '@/lib/chat-messages' @@ -260,6 +260,111 @@ describe('useSessionStateCache — per-session turn timer', () => { }) }) +interface LayoutProbeHarnessProps { + activeSessionId: string | null + onLayoutSnapshot: (snapshot: { active: string | null; selected: string | null }) => void + onReady: (cache: Cache) => void + selectedStoredSessionId: string | null +} + +function LayoutProbeHarness({ + activeSessionId, + onLayoutSnapshot, + onReady, + selectedStoredSessionId +}: LayoutProbeHarnessProps) { + const busyRef: MutableRefObject = { current: false } + + const cache = useSessionStateCache({ + activeSessionId, + busyRef, + selectedStoredSessionId, + setAwaitingResponse: () => undefined, + setBusy: () => undefined, + setMessages: () => undefined + }) + + onReady(cache) + + // useLayoutEffect fires synchronously right after the DOM commit, BEFORE + // the hook's own useEffect (a passive effect) has a chance to mirror the + // new props into activeSessionIdRef/selectedStoredSessionIdRef. Anything + // that reads the refs in this window — including a synchronous DOM event + // handler firing against the just-committed view — observes the outgoing + // session's ids. + useLayoutEffect(() => { + onLayoutSnapshot({ + active: cache.activeSessionIdRef.current, + selected: cache.selectedStoredSessionIdRef.current + }) + }) + + return null +} + +describe('useSessionStateCache — refs stay coherent with the committed session on switch (#59305)', () => { + afterEach(() => cleanup()) + + it('reflects the new session ids from the layout phase right after switching to a new session', () => { + let cache!: Cache + const snapshots: Array<{ active: string | null; selected: string | null }> = [] + + const { rerender } = render( + snapshots.push(s)} + onReady={c => (cache = c)} + selectedStoredSessionId="stored-A" + /> + ) + + void cache + snapshots.length = 0 // drop the initial-mount snapshot; only the switch matters + + rerender( + snapshots.push(s)} + onReady={c => (cache = c)} + selectedStoredSessionId="stored-B" + /> + ) + + // The refs must already reflect B by the layout phase — a callback firing + // in this window must never observe the outgoing session's ids. + expect(snapshots[0]).toEqual({ active: 'runtime-B', selected: 'stored-B' }) + }) + + it('does not clobber an imperative ref pin on a re-render that leaves the props unchanged (#54527-class)', () => { + // submit.ts pins activeSessionIdRef.current to a freshly resumed runtime id + // WITHOUT updating the source atom that feeds the activeSessionId prop (by + // design — see submit.ts's "pin the foreground session context" comment). + // The prop-mirroring here must only fire when the prop itself changes; an + // unconditional resync would silently undo that pin on the next incidental + // render (wiring.tsx re-renders constantly during an active turn). + let cache!: Cache + + const { rerender } = render( + (cache = c)} selectedStoredSessionId="stored-A" /> + ) + + // Simulate submit.ts's imperative pin: a resume swapped in a new runtime + // id without touching the prop. + cache.activeSessionIdRef.current = 'runtime-resumed' + + // A re-render with the SAME props (e.g. an unrelated $busy/$messages + // change elsewhere in the tree) must not touch the pinned ref. + rerender( (cache = c)} selectedStoredSessionId="stored-A" />) + + expect(cache.activeSessionIdRef.current).toBe('runtime-resumed') + + // A genuine prop change (a real navigation/selection move) still wins. + rerender( (cache = c)} selectedStoredSessionId="stored-B" />) + + expect(cache.activeSessionIdRef.current).toBe('runtime-B') + }) +}) + function userMessage(id: string, text: string): ChatMessage { return { id, role: 'user', parts: [{ type: 'text', text }] } } diff --git a/apps/desktop/src/app/session/hooks/use-session-state-cache.ts b/apps/desktop/src/app/session/hooks/use-session-state-cache.ts index 7e0d7278ed77..0f93d02a7999 100644 --- a/apps/desktop/src/app/session/hooks/use-session-state-cache.ts +++ b/apps/desktop/src/app/session/hooks/use-session-state-cache.ts @@ -51,8 +51,32 @@ export function useSessionStateCache({ setMessages }: SessionStateCacheOptions) { const busy = useStore($busy) - const activeSessionIdRef = useRef(null) - const selectedStoredSessionIdRef = useRef(null) + const activeSessionIdRef = useRef(activeSessionId) + const selectedStoredSessionIdRef = useRef(selectedStoredSessionId) + + // Mirror the latest prop into its ref synchronously during render — not via + // a passive useEffect, which only fires a frame after paint and left the + // ref pointing at the outgoing session for one commit (#59305). Guarded to + // fire only when the PROP itself changed since the last render (the same + // condition a `useEffect(..., [activeSessionId])` dependency array already + // enforced) rather than unconditionally: submit.ts and use-session-actions + // pin these refs imperatively mid-flight (e.g. to a just-resumed runtime id) + // without updating the source atom in lockstep, and wiring.tsx re-renders + // constantly during an active turn — an unconditional resync would silently + // clobber that pin on the next incidental render (#54527-class regression). + const activeSessionIdPropRef = useRef(activeSessionId) + + if (activeSessionIdPropRef.current !== activeSessionId) { + activeSessionIdPropRef.current = activeSessionId + activeSessionIdRef.current = activeSessionId + } + + const selectedStoredSessionIdPropRef = useRef(selectedStoredSessionId) + + if (selectedStoredSessionIdPropRef.current !== selectedStoredSessionId) { + selectedStoredSessionIdPropRef.current = selectedStoredSessionId + selectedStoredSessionIdRef.current = selectedStoredSessionId + } const sessionStateByRuntimeIdRef = useRef(new Map()) const runtimeIdByStoredSessionIdRef = useRef(new Map()) const pendingViewStateRef = useRef<{ sessionId: string; state: ClientSessionState } | null>(null) @@ -61,18 +85,10 @@ export function useSessionStateCache({ // flush below tell a same-session refresh from a thread switch. const viewSessionIdRef = useRef(null) - useEffect(() => { - activeSessionIdRef.current = activeSessionId - }, [activeSessionId]) - useEffect(() => { setMutableRef(busyRef, busy) }, [busy, busyRef]) - useEffect(() => { - selectedStoredSessionIdRef.current = selectedStoredSessionId - }, [selectedStoredSessionId]) - const ensureSessionState = useCallback((sessionId: string, storedSessionId?: string | null) => { const existing = sessionStateByRuntimeIdRef.current.get(sessionId) diff --git a/apps/desktop/src/lib/chat-runtime.test.ts b/apps/desktop/src/lib/chat-runtime.test.ts index b3d4e6385372..46ce1da92198 100644 --- a/apps/desktop/src/lib/chat-runtime.test.ts +++ b/apps/desktop/src/lib/chat-runtime.test.ts @@ -4,6 +4,7 @@ import type { ComposerAttachment } from '@/store/composer' import { attachmentDisplayText, + attachmentId, coerceThinkingText, optimisticAttachmentRef, parseCommandDispatch, @@ -150,3 +151,30 @@ describe('parseSlashCommand', () => { expect(parseSlashCommand('/ some words')).toEqual({ arg: '', name: '' }) }) }) + +describe('attachmentId', () => { + it('normalizes a trailing slash on a url so a re-attach dedupes (#59305 P2)', () => { + expect(attachmentId('url', 'https://example.com/a')).toBe(attachmentId('url', 'https://example.com/a/')) + }) + + it('falls back to the trimmed raw value for a malformed url instead of throwing', () => { + expect(() => attachmentId('url', 'not a url')).not.toThrow() + expect(attachmentId('url', ' not a url ')).toBe(attachmentId('url', 'not a url')) + }) + + it('normalizes backslash path separators so a Windows and posix path dedupe', () => { + expect(attachmentId('file', 'a\\b.ts')).toBe(attachmentId('file', 'a/b.ts')) + }) + + it('normalizes a trailing slash on a folder path', () => { + expect(attachmentId('folder', 'src/app/')).toBe(attachmentId('folder', 'src/app')) + }) + + it('does not collapse a bare root path to an empty id', () => { + expect(attachmentId('folder', '/')).toBe('folder:/') + }) + + it('keeps distinct urls distinct', () => { + expect(attachmentId('url', 'https://example.com/a')).not.toBe(attachmentId('url', 'https://example.com/b')) + }) +}) diff --git a/apps/desktop/src/lib/chat-runtime.ts b/apps/desktop/src/lib/chat-runtime.ts index d2cacf876006..b392d2692a3d 100644 --- a/apps/desktop/src/lib/chat-runtime.ts +++ b/apps/desktop/src/lib/chat-runtime.ts @@ -149,8 +149,37 @@ export function contextPath(path: string, cwd: string): string { return path.startsWith(normalizedCwd) ? path.slice(normalizedCwd.length) : path } +// IDs are content-derived (`kind:value`), not uuids, so upsertAttachment's +// exact-match dedupe only catches a re-attach when the raw value matches +// byte-for-byte. Normalize the value first so a trailing slash, a `\` path +// separator, etc. don't slip past dedupe as a "different" attachment. +function normalizeAttachmentValue(kind: ComposerAttachment['kind'], value: string): string { + const trimmed = value.trim() + + if (kind === 'url') { + try { + // The WHATWG URL parser only collapses an EMPTY path to '/' (bare + // origin) — it does not treat '/a' and '/a/' as equivalent, so strip a + // trailing slash ourselves once the URL is otherwise canonicalized + // (scheme/host case, default ports, etc.). + return new URL(trimmed).toString().replace(/\/+$/, '') + } catch { + return trimmed + } + } + + if (kind === 'file' || kind === 'folder' || kind === 'image') { + const posix = trimmed.replace(/\\/g, '/') + + // Don't collapse a bare root ('/' or 'C:/') down to an empty string. + return posix.length > 1 ? posix.replace(/\/+$/, '') : posix + } + + return trimmed +} + export function attachmentId(kind: ComposerAttachment['kind'], value: string): string { - return `${kind}:${value}` + return `${kind}:${normalizeAttachmentValue(kind, value)}` } export function pathLabel(path: string): string {