diff --git a/app/src/providers/ChatRuntimeProvider.tsx b/app/src/providers/ChatRuntimeProvider.tsx index 18e32b330..8da525d33 100644 --- a/app/src/providers/ChatRuntimeProvider.tsx +++ b/app/src/providers/ChatRuntimeProvider.tsx @@ -65,6 +65,34 @@ const MAX_SEGMENT_DELIVERIES = 100; type SegmentDelivery = { segments: Map; createdAt: number; lastSeenAt: number }; +type ThreadSliceState = ReturnType['thread']; + +/** + * Whether a thread should be treated as occupied for proactive delivery — a + * thread is only a valid target when it is provably "fresh" (holds no + * messages). We check the in-memory message cache and the persisted + * `messageCount` snapshot, so a thread that already has a conversation + * server-side (but whose messages aren't loaded into the cache yet) is still + * treated as occupied and never reused. + * + * Crucially, *unknown* thread metadata counts as occupied: when only a + * rehydrated `selectedThreadId` is present (e.g. before `loadThreads` + * resolves, or after caches were reset while selection was preserved), + * `state.threads.find` returns `undefined`. Treating that as fresh would let + * a proactive event append into a conversation we simply haven't loaded yet. + * We fail closed and open a new thread instead. See #3713. + */ +function threadHasMessages(state: ThreadSliceState, threadId: string): boolean { + const cached = state.messagesByThreadId[threadId]; + if (cached && cached.length > 0) return true; + if (threadId === state.selectedThreadId && state.messages.length > 0) return true; + const thread = state.threads.find(t => t.id === threadId); + // Unknown metadata → fail closed (occupied) rather than risk interrupting an + // unloaded conversation. + if (!thread) return true; + return thread.messageCount > 0; +} + function rtLog(message: string, fields?: Record) { if (IS_PROD) return; if (fields && Object.keys(fields).length > 0) { @@ -262,14 +290,18 @@ const ChatRuntimeProvider = ({ children }: { children: React.ReactNode }) => { } const state = store.getState().thread; - // Resolution priority: selected > any in-flight inference thread > first - // thread. With parallel inference there may be several active threads; - // any one is an acceptable proactive target when nothing is selected. - const firstActiveThreadId = Object.keys(state.activeThreadIds)[0] ?? null; - const targetFromState = - state.selectedThreadId ?? firstActiveThreadId ?? state.threads[0]?.id ?? null; - if (targetFromState) { - return targetFromState; + // Reuse an existing thread for proactive delivery ONLY when it is + // fresh (no messages). Injecting a morning brief / subconscious + // update into a thread that already holds a conversation interrupts + // the active chat flow (#3713). Candidate priority is selected > + // first thread; if the candidate already has messages we fall + // through and open a dedicated new thread instead. An in-flight + // inference thread always has at least the user's message, so it is + // never considered fresh — that is why `activeThreadIds` is no + // longer used as a target here. + const candidateThreadId = state.selectedThreadId ?? state.threads[0]?.id ?? null; + if (candidateThreadId && !threadHasMessages(state, candidateThreadId)) { + return candidateThreadId; } if (proactiveThreadCreationPromiseRef.current) { diff --git a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx index 1f687b7fa..87a973f10 100644 --- a/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx +++ b/app/src/providers/__tests__/ChatRuntimeProvider.test.tsx @@ -421,10 +421,10 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria }); describe('proactive thread resolution', () => { - it('reuses the selected thread when resolving a proactive: sender', async () => { + it('reuses the selected thread when it is fresh (no messages)', async () => { store.dispatch( loadThreads.fulfilled( - { threads: [{ id: 'visible-thread', title: 'x' }] as never, count: 1 }, + { threads: [{ id: 'visible-thread', title: 'x', messageCount: 0 }] as never, count: 1 }, 'req-id', undefined ) @@ -440,7 +440,7 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria }); }); - // createNewThread must NOT be invoked when a visible thread already exists. + // createNewThread must NOT be invoked when a fresh visible thread exists. expect(threadApi.createNewThread).not.toHaveBeenCalled(); await waitFor(() => expect(threadApi.appendMessage).toHaveBeenCalledWith( @@ -450,6 +450,82 @@ describe('ChatRuntimeProvider — dedupe, proactive resolution, mid-turn invaria ); }); + it('opens a new thread instead of interrupting a selected thread that has messages (#3713)', async () => { + vi.mocked(threadApi.createNewThread).mockResolvedValue({ + id: 'fresh-thread', + title: 'new', + } as never); + vi.mocked(threadApi.getThreads).mockResolvedValue({ + threads: [{ id: 'fresh-thread', title: 'new' }] as never, + count: 1, + }); + + // The user is mid-conversation: the selected thread already holds + // messages, so a proactive morning brief / subconscious update must + // NOT be injected into it. + store.dispatch( + loadThreads.fulfilled( + { threads: [{ id: 'busy-thread', title: 'chat', messageCount: 4 }] as never, count: 1 }, + 'req-id', + undefined + ) + ); + store.dispatch(setSelectedThread('busy-thread')); + const listeners = renderProvider(); + + await act(async () => { + listeners.onProactiveMessage?.({ + thread_id: 'proactive:morning_briefing', + request_id: 'req-mb', + full_response: "good morning! here's your briefing", + }); + }); + + // The active conversation must be left untouched; delivery goes to a + // dedicated new thread. + await waitFor(() => expect(threadApi.createNewThread).toHaveBeenCalledTimes(1)); + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 'fresh-thread', + expect.objectContaining({ content: "good morning! here's your briefing" }) + ); + expect(threadApi.appendMessage).not.toHaveBeenCalledWith('busy-thread', expect.anything()); + }); + + it('treats a selected thread with unknown metadata as occupied and opens a new thread', async () => { + vi.mocked(threadApi.createNewThread).mockResolvedValue({ + id: 'fresh-thread', + title: 'new', + } as never); + vi.mocked(threadApi.getThreads).mockResolvedValue({ + threads: [{ id: 'fresh-thread', title: 'new' }] as never, + count: 1, + }); + + // Only a rehydrated selection is present — the thread list hasn't loaded, + // so its message metadata is unknown. We must NOT assume it is fresh + // (it could already hold a server-side conversation). See #3713. + store.dispatch(setSelectedThread('rehydrated-thread')); + const listeners = renderProvider(); + + await act(async () => { + listeners.onProactiveMessage?.({ + thread_id: 'proactive:morning_briefing', + request_id: 'req-unknown', + full_response: 'briefing', + }); + }); + + await waitFor(() => expect(threadApi.createNewThread).toHaveBeenCalledTimes(1)); + expect(threadApi.appendMessage).toHaveBeenCalledWith( + 'fresh-thread', + expect.objectContaining({ content: 'briefing' }) + ); + expect(threadApi.appendMessage).not.toHaveBeenCalledWith( + 'rehydrated-thread', + expect.anything() + ); + }); + it('creates a new thread when no visible thread exists for proactive handoff', async () => { vi.mocked(threadApi.createNewThread).mockResolvedValue({ id: 'created-thread',