From 60fd4d9ade653ba359086f58fe22514669bf01b2 Mon Sep 17 00:00:00 2001 From: Test Date: Mon, 9 Mar 2026 10:18:36 +0100 Subject: [PATCH] fix: embed conversation history in AI agent chat messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AI chat panel (AiPanel → useAiAgent) was sending each message as a standalone request with no prior context. Root cause: useAiAgent.sendMessage called streamClaudeAgent with raw text, never embedding history. - Add agentMessagesToChatHistory() to convert AiAgentMessage[] to ChatMessage[] - Embed trimmed history in each agent request via formatMessageWithHistory - Use messagesRef/statusRef to avoid stale closures in async callbacks - Also fix useAIChat (dead code path) with same ref pattern - Update mock layers to detect history presence for testability - Add Playwright smoke tests verifying history accumulates and resets Co-Authored-By: Claude Opus 4.6 --- src/hooks/useAIChat.test.ts | 24 +++++ src/hooks/useAIChat.ts | 32 ++++--- src/hooks/useAiAgent.test.ts | 130 +++++++++++++++++++++++++++- src/hooks/useAiAgent.ts | 36 +++++++- src/utils/ai-agent.ts | 18 +++- src/utils/ai-chat.test.ts | 23 ++++- src/utils/ai-chat.ts | 19 +++- tests/smoke/ai-chat-history.spec.ts | 84 ++++++++++++++++++ 8 files changed, 347 insertions(+), 19 deletions(-) create mode 100644 tests/smoke/ai-chat-history.spec.ts diff --git a/src/hooks/useAIChat.test.ts b/src/hooks/useAIChat.test.ts index cf6ac74b..ee3b294f 100644 --- a/src/hooks/useAIChat.test.ts +++ b/src/hooks/useAIChat.test.ts @@ -165,4 +165,28 @@ describe('useAIChat', () => { expect(lastCall[0]).toBe('hello') expect(lastCall[2]).toBeUndefined() }) + + it('reads latest messages from ref (not stale closure)', async () => { + const { result } = renderHook(() => useAIChat([])) + + // Send first message + act(() => { result.current.sendMessage('msg1') }) + await act(async () => { vi.advanceTimersByTime(50) }) + + // Verify messages state is correct + expect(result.current.messages).toHaveLength(2) + expect(result.current.messages[0].content).toBe('msg1') + expect(result.current.messages[1].content).toBe('mock response') + + // Send second message — the ref should have the latest messages + // even without depending on messages in useCallback deps + act(() => { result.current.sendMessage('msg2') }) + + const secondCall = streamClaudeChatMock.mock.calls[1] + const sentMessage = secondCall[0] + // Must contain history from first exchange + expect(sentMessage).toContain('[user]: msg1') + expect(sentMessage).toContain('[assistant]: mock response') + expect(sentMessage).toContain('[user]: msg2') + }) }) diff --git a/src/hooks/useAIChat.ts b/src/hooks/useAIChat.ts index a0794203..329b72bf 100644 --- a/src/hooks/useAIChat.ts +++ b/src/hooks/useAIChat.ts @@ -5,8 +5,11 @@ * Conversation continuity embeds prior exchanges in each prompt * (each CLI invocation is a fresh subprocess with no memory). * History is trimmed to MAX_HISTORY_TOKENS, dropping oldest first. + * + * Uses a ref (messagesRef) to read the latest messages in callbacks, + * avoiding stale closure issues with React's useCallback memoization. */ -import { useState, useCallback, useRef } from 'react' +import { useState, useCallback, useRef, useEffect } from 'react' import type { VaultEntry } from '../types' import { type ChatMessage, type ChatStreamCallbacks, nextMessageId, @@ -57,10 +60,18 @@ export function useAIChat( const [isStreaming, setIsStreaming] = useState(false) const [streamingContent, setStreamingContent] = useState('') const abortRef = useRef(false) + const isStreamingRef = useRef(false) + const messagesRef = useRef([]) - /** Internal: send text with explicit history context. */ - const doSend = useCallback((text: string, history: ChatMessage[]) => { - if (!text.trim() || isStreaming) return + // Keep refs in sync with state — runs after render, before next user interaction. + useEffect(() => { messagesRef.current = messages }, [messages]) + useEffect(() => { isStreamingRef.current = isStreaming }, [isStreaming]) + + /** Internal: send text, reading history from the messages ref. */ + const doSend = useCallback((text: string, historyOverride?: ChatMessage[]) => { + if (!text.trim() || isStreamingRef.current) return + + const history = historyOverride ?? messagesRef.current setMessages(prev => [...prev, { role: 'user', content: text.trim(), id: nextMessageId() }]) setIsStreaming(true) @@ -80,11 +91,11 @@ export function useAIChat( streamClaudeChat(formattedMessage, systemPrompt, undefined, callbacks) .catch(() => { /* errors forwarded via onError */ }) - }, [isStreaming, contextNotes]) + }, [contextNotes]) const sendMessage = useCallback((text: string) => { - doSend(text, messages) - }, [doSend, messages]) + doSend(text) + }, [doSend]) const clearConversation = useCallback(() => { abortRef.current = true @@ -94,15 +105,16 @@ export function useAIChat( }, []) const retryMessage = useCallback((msgIndex: number) => { + const currentMessages = messagesRef.current const userMsgIndex = msgIndex - 1 if (userMsgIndex < 0) return - const userMsg = messages[userMsgIndex] + const userMsg = currentMessages[userMsgIndex] if (userMsg.role !== 'user') return - const historyForRetry = messages.slice(0, userMsgIndex) + const historyForRetry = currentMessages.slice(0, userMsgIndex) setMessages(prev => prev.slice(0, userMsgIndex)) doSend(userMsg.content, historyForRetry) - }, [messages, doSend]) + }, [doSend]) return { messages, isStreaming, streamingContent, sendMessage, clearConversation, retryMessage } } diff --git a/src/hooks/useAiAgent.test.ts b/src/hooks/useAiAgent.test.ts index 540ffc02..67af3207 100644 --- a/src/hooks/useAiAgent.test.ts +++ b/src/hooks/useAiAgent.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { renderHook, act } from '@testing-library/react' -import { detectFileOperation, parseBashFileCreation, useAiAgent } from './useAiAgent' -import type { AgentFileCallbacks } from './useAiAgent' +import { detectFileOperation, parseBashFileCreation, agentMessagesToChatHistory, useAiAgent } from './useAiAgent' +import type { AgentFileCallbacks, AiAgentMessage } from './useAiAgent' import { streamClaudeAgent } from '../utils/ai-agent' vi.mock('../utils/ai-agent', () => ({ @@ -237,4 +237,130 @@ describe('useAiAgent', () => { expect(mockStreamClaudeAgent).toHaveBeenCalledTimes(1) expect(mockStreamClaudeAgent.mock.calls[0][1]).toBe('default-system-prompt') }) + + it('sends first message without conversation history', async () => { + const { result } = renderHook(() => useAiAgent(VAULT, undefined)) + + await act(async () => { + await result.current.sendMessage('Hello') + }) + + expect(mockStreamClaudeAgent).toHaveBeenCalledTimes(1) + const sentMessage = mockStreamClaudeAgent.mock.calls[0][0] + expect(sentMessage).toBe('Hello') + expect(sentMessage).not.toContain('') + }) + + it('embeds conversation history in second message', async () => { + // Mock that simulates a response for the first message + mockStreamClaudeAgent.mockImplementation(async (_msg, _sys, _vault, callbacks) => { + callbacks.onText('The answer is 4') + callbacks.onDone() + }) + + const { result } = renderHook(() => useAiAgent(VAULT, undefined)) + + // First message + await act(async () => { + await result.current.sendMessage('What is 2+2?') + }) + + // Second message — should include history from first exchange + await act(async () => { + await result.current.sendMessage('What was my previous question?') + }) + + expect(mockStreamClaudeAgent).toHaveBeenCalledTimes(2) + + // First call: no history + expect(mockStreamClaudeAgent.mock.calls[0][0]).toBe('What is 2+2?') + + // Second call: includes history + const secondMsg = mockStreamClaudeAgent.mock.calls[1][0] + expect(secondMsg).toContain('') + expect(secondMsg).toContain('What is 2+2?') + expect(secondMsg).toContain('The answer is 4') + expect(secondMsg).toContain('What was my previous question?') + }) + + it('accumulates history across multiple exchanges', async () => { + let callCount = 0 + mockStreamClaudeAgent.mockImplementation(async (_msg, _sys, _vault, callbacks) => { + callCount++ + callbacks.onText(`Response ${callCount}`) + callbacks.onDone() + }) + + const { result } = renderHook(() => useAiAgent(VAULT, undefined)) + + await act(async () => { await result.current.sendMessage('Q1') }) + await act(async () => { await result.current.sendMessage('Q2') }) + await act(async () => { await result.current.sendMessage('Q3') }) + + expect(mockStreamClaudeAgent).toHaveBeenCalledTimes(3) + + // First: no history + expect(mockStreamClaudeAgent.mock.calls[0][0]).toBe('Q1') + + // Second: history from first exchange + const msg2 = mockStreamClaudeAgent.mock.calls[1][0] + expect(msg2).toContain('Q1') + expect(msg2).toContain('Response 1') + expect(msg2).toContain('Q2') + + // Third: history from both exchanges + const msg3 = mockStreamClaudeAgent.mock.calls[2][0] + expect(msg3).toContain('Q1') + expect(msg3).toContain('Response 1') + expect(msg3).toContain('Q2') + expect(msg3).toContain('Response 2') + expect(msg3).toContain('Q3') + }) + + it('resets history after clearConversation', async () => { + mockStreamClaudeAgent.mockImplementation(async (_msg, _sys, _vault, callbacks) => { + callbacks.onText('reply') + callbacks.onDone() + }) + + const { result } = renderHook(() => useAiAgent(VAULT, undefined)) + + await act(async () => { await result.current.sendMessage('hello') }) + + act(() => { result.current.clearConversation() }) + + await act(async () => { await result.current.sendMessage('fresh start') }) + + const lastCall = mockStreamClaudeAgent.mock.calls[mockStreamClaudeAgent.mock.calls.length - 1] + expect(lastCall[0]).toBe('fresh start') + expect(lastCall[0]).not.toContain('') + }) +}) + +describe('agentMessagesToChatHistory', () => { + it('returns empty array for no messages', () => { + expect(agentMessagesToChatHistory([])).toEqual([]) + }) + + it('converts agent messages to user/assistant pairs', () => { + const msgs: AiAgentMessage[] = [ + { userMessage: 'Q1', response: 'A1', actions: [], id: 'm1' }, + { userMessage: 'Q2', response: 'A2', actions: [], id: 'm2' }, + ] + const result = agentMessagesToChatHistory(msgs) + expect(result).toHaveLength(4) + expect(result[0]).toMatchObject({ role: 'user', content: 'Q1' }) + expect(result[1]).toMatchObject({ role: 'assistant', content: 'A1' }) + expect(result[2]).toMatchObject({ role: 'user', content: 'Q2' }) + expect(result[3]).toMatchObject({ role: 'assistant', content: 'A2' }) + }) + + it('skips assistant entry when response is undefined (still streaming)', () => { + const msgs: AiAgentMessage[] = [ + { userMessage: 'Q1', actions: [], id: 'm1', isStreaming: true }, + ] + const result = agentMessagesToChatHistory(msgs) + expect(result).toHaveLength(1) + expect(result[0]).toMatchObject({ role: 'user', content: 'Q1' }) + }) }) diff --git a/src/hooks/useAiAgent.ts b/src/hooks/useAiAgent.ts index 1261a4e5..db478d61 100644 --- a/src/hooks/useAiAgent.ts +++ b/src/hooks/useAiAgent.ts @@ -14,7 +14,10 @@ import { useState, useCallback, useRef, useEffect } from 'react' import type { AiAction } from '../components/AiMessage' import type { NoteReference } from '../utils/ai-context' import { streamClaudeAgent, buildAgentSystemPrompt } from '../utils/ai-agent' -import { nextMessageId } from '../utils/ai-chat' +import { + nextMessageId, trimHistory, formatMessageWithHistory, + type ChatMessage, MAX_HISTORY_TOKENS, +} from '../utils/ai-chat' export type AgentStatus = 'idle' | 'thinking' | 'tool-executing' | 'done' | 'error' @@ -36,6 +39,18 @@ export interface AgentFileCallbacks { onVaultChanged?: () => void } +/** Convert completed agent messages to ChatMessage pairs for history embedding. */ +export function agentMessagesToChatHistory(msgs: AiAgentMessage[]): ChatMessage[] { + const history: ChatMessage[] = [] + for (const msg of msgs) { + history.push({ role: 'user', content: msg.userMessage, id: msg.id ?? '' }) + if (msg.response) { + history.push({ role: 'assistant', content: msg.response, id: `${msg.id}-resp` }) + } + } + return history +} + export function useAiAgent( vaultPath: string, contextPrompt?: string, @@ -48,13 +63,20 @@ export function useAiAgent( const fileCallbacksRef = useRef(fileCallbacks) // Track tool inputs for file-operation detection on ToolDone const toolInputMapRef = useRef>(new Map()) + // Refs for latest state — avoids stale closures in callbacks. + // Synced via useEffect (runs after render, before next user interaction). + const messagesRef = useRef([]) + const statusRef = useRef('idle') + useEffect(() => { messagesRef.current = messages }, [messages]) + useEffect(() => { statusRef.current = status }, [status]) useEffect(() => { fileCallbacksRef.current = fileCallbacks }, [fileCallbacks]) const sendMessage = useCallback(async (text: string, references?: NoteReference[]) => { - if (!text.trim() || status === 'thinking' || status === 'tool-executing') return + const currentStatus = statusRef.current + if (!text.trim() || currentStatus === 'thinking' || currentStatus === 'tool-executing') return const refs = references && references.length > 0 ? references : undefined @@ -87,7 +109,13 @@ export function useAiAgent( const systemPrompt = contextPrompt ?? buildAgentSystemPrompt() - await streamClaudeAgent(text.trim(), systemPrompt, vaultPath, { + // Embed conversation history from previous exchanges. + // Uses messagesRef (not closure-captured messages) to avoid stale closures. + const chatHistory = agentMessagesToChatHistory(messagesRef.current.filter(m => !m.isStreaming)) + const trimmedHistory = trimHistory(chatHistory, MAX_HISTORY_TOKENS) + const formattedMessage = formatMessageWithHistory(trimmedHistory, text.trim()) + + await streamClaudeAgent(formattedMessage, systemPrompt, vaultPath, { onThinking: (chunk) => { if (abortRef.current.aborted) return update(m => ({ ...m, reasoning: (m.reasoning ?? '') + chunk })) @@ -174,7 +202,7 @@ export function useAiAgent( fileCallbacksRef.current?.onVaultChanged?.() }, }) - }, [status, vaultPath, contextPrompt]) + }, [vaultPath, contextPrompt]) const clearConversation = useCallback(() => { abortRef.current.aborted = true diff --git a/src/utils/ai-agent.ts b/src/utils/ai-agent.ts index b873b378..8301a8f2 100644 --- a/src/utils/ai-agent.ts +++ b/src/utils/ai-agent.ts @@ -46,6 +46,22 @@ export interface AgentStreamCallbacks { onDone: () => void } +/** + * Generate a mock response for browser/test mode. + * Inspects the message for conversation history so Playwright tests + * can verify that history is actually being sent. + */ +function mockAgentResponse(message: string): string { + if (message.includes('')) { + const allUserLines = message.match(/\[user\]: .+/g) ?? [] + const turnCount = allUserLines.length + const lastLine = allUserLines[allUserLines.length - 1] ?? '' + const lastUserMsg = lastLine.replace('[user]: ', '') + return `[mock-with-history turns=${turnCount}] You asked: "${lastUserMsg}" — This note is related to [[Build Laputa App]] and [[Matteo Cellini]].` + } + return `[mock-no-history] You said: "${message}" — This note is related to [[Build Laputa App]] and [[Matteo Cellini]].` +} + /** * Stream an agent task through the Claude CLI subprocess with full tool access. * The CLI handles the tool-use loop; we receive events for UI updates. @@ -58,7 +74,7 @@ export async function streamClaudeAgent( ): Promise { if (!isTauri()) { setTimeout(() => { - callbacks.onText('This note is related to [[Build Laputa App]] and [[Matteo Cellini]]. The AI Agent requires the Claude CLI for full functionality.') + callbacks.onText(mockAgentResponse(message)) callbacks.onDone() }, 300) return diff --git a/src/utils/ai-chat.test.ts b/src/utils/ai-chat.test.ts index 42dbd929..995fa903 100644 --- a/src/utils/ai-chat.test.ts +++ b/src/utils/ai-chat.test.ts @@ -200,8 +200,29 @@ describe('streamClaudeChat', () => { await new Promise(r => setTimeout(r, 400)) expect(sessionId).toBe('mock-session') - expect(onText).toHaveBeenCalledWith(expect.stringContaining('Claude CLI')) + expect(onText).toHaveBeenCalledWith(expect.stringContaining('[mock-no-history]')) expect(onDone).toHaveBeenCalled() expect(onError).not.toHaveBeenCalled() }) + + it('mock detects conversation history in message', async () => { + const onText = vi.fn() + const onDone = vi.fn() + const onError = vi.fn() + + const msgWithHistory = formatMessageWithHistory( + [{ role: 'user', content: 'What is 2+2?', id: 'm1' }, { role: 'assistant', content: '4', id: 'm2' }], + 'What was my previous question?', + ) + + await streamClaudeChat(msgWithHistory, undefined, undefined, { + onText, onError, onDone, + }) + + await new Promise(r => setTimeout(r, 400)) + + expect(onText).toHaveBeenCalledWith(expect.stringContaining('[mock-with-history')) + expect(onText).toHaveBeenCalledWith(expect.stringContaining('turns=2')) + expect(onText).toHaveBeenCalledWith(expect.stringContaining('What was my previous question?')) + }) }) diff --git a/src/utils/ai-chat.ts b/src/utils/ai-chat.ts index dd537bab..c20d110a 100644 --- a/src/utils/ai-chat.ts +++ b/src/utils/ai-chat.ts @@ -148,6 +148,23 @@ function handleChatStreamEvent( } } +/** + * Generate a mock response for browser/test mode. + * Inspects the message for conversation history so Playwright tests + * can verify that history is actually being sent. + */ +function mockChatResponse(message: string): string { + if (message.includes('')) { + const allUserLines = message.match(/\[user\]: .+/g) ?? [] + const turnCount = allUserLines.length + // The last [user] line is the actual new message + const lastLine = allUserLines[allUserLines.length - 1] ?? '' + const lastUserMsg = lastLine.replace('[user]: ', '') + return `[mock-with-history turns=${turnCount}] You asked: "${lastUserMsg}"` + } + return `[mock-no-history] You said: "${message}"` +} + /** * Stream a chat message through the Claude CLI subprocess. * Returns the session ID for conversation continuity via --resume. @@ -160,7 +177,7 @@ export async function streamClaudeChat( ): Promise { if (!isTauri()) { setTimeout(() => { - callbacks.onText('AI Chat requires the Claude CLI. Install it and run the native app.') + callbacks.onText(mockChatResponse(message)) callbacks.onDone() }, 300) return 'mock-session' diff --git a/tests/smoke/ai-chat-history.spec.ts b/tests/smoke/ai-chat-history.spec.ts new file mode 100644 index 00000000..5aec6a3c --- /dev/null +++ b/tests/smoke/ai-chat-history.spec.ts @@ -0,0 +1,84 @@ +import { test, expect } from '@playwright/test' +import { sendShortcut } from './helpers' + +test.describe('AI chat conversation history', () => { + test.beforeEach(async ({ page }) => { + // Block vault API so mock entries are used + await page.route('**/api/vault/ping', route => route.fulfill({ status: 503 })) + + await page.goto('/') + await page.waitForTimeout(500) + + // Select a note so the AI panel has context + const noteItem = page.locator('.app__note-list .cursor-pointer').first() + await noteItem.click() + await page.waitForTimeout(500) + + // Open AI Chat with Ctrl+I + await sendShortcut(page, 'i', ['Control']) + await expect(page.getByTestId('ai-panel')).toBeVisible({ timeout: 3000 }) + }) + + test('first message has no conversation history marker', async ({ page }) => { + // Find the input and send a message + const input = page.locator('input[placeholder*="Ask"]') + await input.fill('Hello') + await page.getByTestId('agent-send').click() + + // Wait for mock response to appear + const response = page.getByTestId('ai-message').last() + await expect(response).toBeVisible({ timeout: 5000 }) + + // First message should have [mock-no-history] since there's no prior conversation + await expect(response).toContainText('[mock-no-history]') + }) + + test('second message includes conversation history from first exchange', async ({ page }) => { + // Send first message + const input = page.locator('input[placeholder*="Ask"]') + await input.fill('What is 2+2?') + await page.getByTestId('agent-send').click() + + // Wait for first response to appear + const firstResponse = page.getByTestId('ai-message').last() + await expect(firstResponse).toBeVisible({ timeout: 5000 }) + await expect(firstResponse).toContainText('[mock-no-history]') + + // Send second message + await input.fill('What was my previous question?') + await page.getByTestId('agent-send').click() + + // Wait for second response — it should contain history marker + await page.waitForTimeout(1000) + const secondResponse = page.getByTestId('ai-message').last() + await expect(secondResponse).toContainText('[mock-with-history', { timeout: 5000 }) + // turns=2 means 2 [user] lines: original + new question + await expect(secondResponse).toContainText('turns=2') + }) + + test('history resets after clearing conversation', async ({ page }) => { + // Send first message + const input = page.locator('input[placeholder*="Ask"]') + await input.fill('Hello') + await page.getByTestId('agent-send').click() + + // Wait for response + const firstResponse = page.getByTestId('ai-message').last() + await expect(firstResponse).toBeVisible({ timeout: 5000 }) + + // Clear conversation (click the + button) + await page.locator('button[title="New conversation"]').click() + await page.waitForTimeout(300) + + // Messages should be cleared + await expect(page.getByTestId('ai-message')).toHaveCount(0) + + // Send new message — should have no history + await input.fill('Fresh start') + await page.getByTestId('agent-send').click() + + const freshResponse = page.getByTestId('ai-message').last() + await expect(freshResponse).toBeVisible({ timeout: 5000 }) + await expect(freshResponse).toContainText('[mock-no-history]') + }) +})