mirror of
https://github.com/leonvanzyl/autocoder.git
synced 2026-01-30 06:12:06 +00:00
fix: address CodeRabbitAI review comments for conversation history
- Fix duplicate onConversationCreated callbacks by tracking activeConversationId - Fix history loss when switching conversations with Map-based deduplication - Disable input while conversation is loading to prevent message routing issues - Gate WebSocket debug logs behind DEV flag (import.meta.env.DEV) - Downgrade server logging from info to debug level for reduced noise - Fix .gitignore prefixes for playwright paths (ui/playwright-report/, ui/test-results/) - Remove debug console.log from ConversationHistory.tsx - Add staleTime (30s) to single conversation query for better caching - Increase history message cap from 20 to 35 for better context - Replace fixed timeouts with condition-based waits in e2e tests
This commit is contained in:
4
.gitignore
vendored
4
.gitignore
vendored
@@ -64,7 +64,7 @@ coverage.xml
|
|||||||
.hypothesis/
|
.hypothesis/
|
||||||
.pytest_cache/
|
.pytest_cache/
|
||||||
nosetests.xml
|
nosetests.xml
|
||||||
./ui/playwright-report
|
ui/playwright-report/
|
||||||
|
|
||||||
# mypy
|
# mypy
|
||||||
.mypy_cache/
|
.mypy_cache/
|
||||||
@@ -143,4 +143,4 @@ Pipfile.lock
|
|||||||
.tmp/
|
.tmp/
|
||||||
.temp/
|
.temp/
|
||||||
tmpclaude-*-cwd
|
tmpclaude-*-cwd
|
||||||
./ui/test-results
|
ui/test-results/
|
||||||
|
|||||||
@@ -260,7 +260,7 @@ async def assistant_chat_websocket(websocket: WebSocket, project_name: str):
|
|||||||
data = await websocket.receive_text()
|
data = await websocket.receive_text()
|
||||||
message = json.loads(data)
|
message = json.loads(data)
|
||||||
msg_type = message.get("type")
|
msg_type = message.get("type")
|
||||||
logger.info(f"Assistant received message type: {msg_type}")
|
logger.debug(f"Assistant received message type: {msg_type}")
|
||||||
|
|
||||||
if msg_type == "ping":
|
if msg_type == "ping":
|
||||||
await websocket.send_json({"type": "pong"})
|
await websocket.send_json({"type": "pong"})
|
||||||
@@ -269,23 +269,24 @@ async def assistant_chat_websocket(websocket: WebSocket, project_name: str):
|
|||||||
elif msg_type == "start":
|
elif msg_type == "start":
|
||||||
# Get optional conversation_id to resume
|
# Get optional conversation_id to resume
|
||||||
conversation_id = message.get("conversation_id")
|
conversation_id = message.get("conversation_id")
|
||||||
logger.info(f"Processing start message with conversation_id={conversation_id}")
|
logger.debug(f"Processing start message with conversation_id={conversation_id}")
|
||||||
|
|
||||||
try:
|
try:
|
||||||
# Create a new session
|
# Create a new session
|
||||||
logger.info(f"Creating session for {project_name}")
|
logger.debug(f"Creating session for {project_name}")
|
||||||
session = await create_session(
|
session = await create_session(
|
||||||
project_name,
|
project_name,
|
||||||
project_dir,
|
project_dir,
|
||||||
conversation_id=conversation_id,
|
conversation_id=conversation_id,
|
||||||
)
|
)
|
||||||
logger.info(f"Session created, starting...")
|
logger.debug("Session created, starting...")
|
||||||
|
|
||||||
# Stream the initial greeting
|
# Stream the initial greeting
|
||||||
async for chunk in session.start():
|
async for chunk in session.start():
|
||||||
logger.info(f"Sending chunk: {chunk.get('type')}")
|
if logger.isEnabledFor(logging.DEBUG):
|
||||||
|
logger.debug(f"Sending chunk: {chunk.get('type')}")
|
||||||
await websocket.send_json(chunk)
|
await websocket.send_json(chunk)
|
||||||
logger.info("Session start complete")
|
logger.debug("Session start complete")
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.exception(f"Error starting assistant session for {project_name}")
|
logger.exception(f"Error starting assistant session for {project_name}")
|
||||||
await websocket.send_json({
|
await websocket.send_json({
|
||||||
|
|||||||
@@ -345,6 +345,8 @@ class AssistantChatSession:
|
|||||||
history = get_messages(self.project_dir, self.conversation_id)
|
history = get_messages(self.project_dir, self.conversation_id)
|
||||||
# Exclude the message we just added (last one)
|
# Exclude the message we just added (last one)
|
||||||
history = history[:-1] if history else []
|
history = history[:-1] if history else []
|
||||||
|
# Cap history to last 35 messages to prevent context overload
|
||||||
|
history = history[-35:] if len(history) > 35 else history
|
||||||
if history:
|
if history:
|
||||||
# Format history as context for Claude
|
# Format history as context for Claude
|
||||||
history_lines = ["[Previous conversation history for context:]"]
|
history_lines = ["[Previous conversation history for context:]"]
|
||||||
|
|||||||
@@ -33,7 +33,8 @@ test.describe('Assistant Panel UI', () => {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
await projectItem.click()
|
await projectItem.click()
|
||||||
await page.waitForTimeout(500)
|
// Wait for dropdown to close (project selected)
|
||||||
|
await expect(projectSelector).not.toBeVisible({ timeout: 5000 }).catch(() => {})
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
return false
|
return false
|
||||||
@@ -321,7 +322,8 @@ test.describe('Conversation History Integration', () => {
|
|||||||
const hasProject = await projectItem.isVisible().catch(() => false)
|
const hasProject = await projectItem.isVisible().catch(() => false)
|
||||||
if (!hasProject) return false
|
if (!hasProject) return false
|
||||||
await projectItem.click()
|
await projectItem.click()
|
||||||
await page.waitForTimeout(500)
|
// Wait for dropdown to close (project selected)
|
||||||
|
await expect(projectSelector).not.toBeVisible({ timeout: 5000 }).catch(() => {})
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
return false
|
return false
|
||||||
@@ -360,7 +362,7 @@ test.describe('Conversation History Integration', () => {
|
|||||||
await expect(page.locator(`text=${message}`).first()).toBeVisible({ timeout: 5000 })
|
await expect(page.locator(`text=${message}`).first()).toBeVisible({ timeout: 5000 })
|
||||||
await page.waitForSelector('text=Thinking...', { timeout: 10000 }).catch(() => {})
|
await page.waitForSelector('text=Thinking...', { timeout: 10000 }).catch(() => {})
|
||||||
await expect(inputArea).toBeEnabled({ timeout: 60000 })
|
await expect(inputArea).toBeEnabled({ timeout: 60000 })
|
||||||
await page.waitForTimeout(500)
|
// Wait for any streaming to complete (input enabled means response done)
|
||||||
}
|
}
|
||||||
|
|
||||||
// --------------------------------------------------------------------------
|
// --------------------------------------------------------------------------
|
||||||
@@ -399,7 +401,6 @@ test.describe('Conversation History Integration', () => {
|
|||||||
|
|
||||||
await page.keyboard.press('a')
|
await page.keyboard.press('a')
|
||||||
await waitForPanelOpen(page)
|
await waitForPanelOpen(page)
|
||||||
await page.waitForTimeout(2000)
|
|
||||||
|
|
||||||
// Verify our question is still visible (conversation resumed)
|
// Verify our question is still visible (conversation resumed)
|
||||||
await expect(page.locator('text=how much is 1+1').first()).toBeVisible({ timeout: 10000 })
|
await expect(page.locator('text=how much is 1+1').first()).toBeVisible({ timeout: 10000 })
|
||||||
@@ -413,7 +414,6 @@ test.describe('Conversation History Integration', () => {
|
|||||||
console.log('STEP 3: New chat')
|
console.log('STEP 3: New chat')
|
||||||
const newChatButton = page.locator('button[title="New conversation"]')
|
const newChatButton = page.locator('button[title="New conversation"]')
|
||||||
await newChatButton.click()
|
await newChatButton.click()
|
||||||
await page.waitForTimeout(500)
|
|
||||||
|
|
||||||
if (!await waitForAssistantReady(page)) {
|
if (!await waitForAssistantReady(page)) {
|
||||||
test.skip(true, 'Assistant API not available')
|
test.skip(true, 'Assistant API not available')
|
||||||
@@ -441,7 +441,7 @@ test.describe('Conversation History Integration', () => {
|
|||||||
// STEP 6: Switch to first conversation
|
// STEP 6: Switch to first conversation
|
||||||
console.log('STEP 6: Switch conversation')
|
console.log('STEP 6: Switch conversation')
|
||||||
await conversationItems.nth(1).click()
|
await conversationItems.nth(1).click()
|
||||||
await page.waitForTimeout(2000)
|
// Wait for conversation to load by checking for the expected message
|
||||||
await expect(page.locator('text=how much is 1+1').first()).toBeVisible({ timeout: 10000 })
|
await expect(page.locator('text=how much is 1+1').first()).toBeVisible({ timeout: 10000 })
|
||||||
await expect(page.locator('text=how much is 2+2')).not.toBeVisible()
|
await expect(page.locator('text=how much is 2+2')).not.toBeVisible()
|
||||||
|
|
||||||
@@ -485,7 +485,9 @@ test.describe('Conversation History Integration', () => {
|
|||||||
const confirmButton = page.locator('button:has-text("Delete")').last()
|
const confirmButton = page.locator('button:has-text("Delete")').last()
|
||||||
await expect(confirmButton).toBeVisible()
|
await expect(confirmButton).toBeVisible()
|
||||||
await confirmButton.click()
|
await confirmButton.click()
|
||||||
await page.waitForTimeout(1000)
|
|
||||||
|
// Wait for confirmation dialog to close
|
||||||
|
await expect(confirmButton).not.toBeVisible({ timeout: 5000 })
|
||||||
|
|
||||||
// Verify count decreased
|
// Verify count decreased
|
||||||
await historyButton.click()
|
await historyButton.click()
|
||||||
|
|||||||
@@ -6,7 +6,7 @@
|
|||||||
* Supports conversation history with resume functionality.
|
* Supports conversation history with resume functionality.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { useState, useRef, useEffect, useCallback } from 'react'
|
import { useState, useRef, useEffect, useCallback, useMemo } from 'react'
|
||||||
import { Send, Loader2, Wifi, WifiOff, Plus, History } from 'lucide-react'
|
import { Send, Loader2, Wifi, WifiOff, Plus, History } from 'lucide-react'
|
||||||
import { useAssistantChat } from '../hooks/useAssistantChat'
|
import { useAssistantChat } from '../hooks/useAssistantChat'
|
||||||
import { ChatMessage as ChatMessageComponent } from './ChatMessage'
|
import { ChatMessage as ChatMessageComponent } from './ChatMessage'
|
||||||
@@ -58,21 +58,18 @@ export function AssistantChat({
|
|||||||
})
|
})
|
||||||
|
|
||||||
// Notify parent when a NEW conversation is created (not when switching to existing)
|
// Notify parent when a NEW conversation is created (not when switching to existing)
|
||||||
// This should only fire when conversationId was null/undefined and a new one was created
|
// Track activeConversationId to fire callback only once when it transitions from null to a value
|
||||||
const previousConversationIdRef = useRef<number | null | undefined>(conversationId)
|
const previousActiveConversationIdRef = useRef<number | null>(activeConversationId)
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
// Only notify if we had NO conversation (null/undefined) and now we have one
|
const hadNoConversation = previousActiveConversationIdRef.current === null
|
||||||
// This prevents the bug where switching conversations would trigger this
|
const nowHasConversation = activeConversationId !== null
|
||||||
const hadNoConversation = previousConversationIdRef.current === null || previousConversationIdRef.current === undefined
|
|
||||||
const nowHasConversation = activeConversationId !== null && activeConversationId !== undefined
|
|
||||||
|
|
||||||
if (hadNoConversation && nowHasConversation && onConversationCreated) {
|
if (hadNoConversation && nowHasConversation && onConversationCreated) {
|
||||||
console.log('[AssistantChat] New conversation created:', activeConversationId)
|
|
||||||
onConversationCreated(activeConversationId)
|
onConversationCreated(activeConversationId)
|
||||||
}
|
}
|
||||||
|
|
||||||
previousConversationIdRef.current = conversationId
|
previousActiveConversationIdRef.current = activeConversationId
|
||||||
}, [activeConversationId, conversationId, onConversationCreated])
|
}, [activeConversationId, onConversationCreated])
|
||||||
|
|
||||||
// Auto-scroll to bottom on new messages
|
// Auto-scroll to bottom on new messages
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@@ -146,7 +143,7 @@ export function AssistantChat({
|
|||||||
|
|
||||||
const handleSend = () => {
|
const handleSend = () => {
|
||||||
const content = inputValue.trim()
|
const content = inputValue.trim()
|
||||||
if (!content || isLoading) return
|
if (!content || isLoading || isLoadingConversation) return
|
||||||
|
|
||||||
sendMessage(content)
|
sendMessage(content)
|
||||||
setInputValue('')
|
setInputValue('')
|
||||||
@@ -160,23 +157,30 @@ export function AssistantChat({
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Combine initial messages (from resumed conversation) with live messages
|
// Combine initial messages (from resumed conversation) with live messages
|
||||||
// Show initialMessages when:
|
// Merge both arrays with deduplication by message ID to prevent history loss
|
||||||
// 1. We have initialMessages from the API
|
const displayMessages = useMemo(() => {
|
||||||
// 2. AND either messages is empty OR we haven't processed this conversation yet
|
const isConversationSynced = lastConversationIdRef.current === conversationId && !isLoadingConversation
|
||||||
// This prevents showing old conversation messages while switching
|
|
||||||
const isConversationSynced = lastConversationIdRef.current === conversationId && !isLoadingConversation
|
// If not synced yet, show only initialMessages (or empty)
|
||||||
const displayMessages = initialMessages && (messages.length === 0 || !isConversationSynced)
|
if (!isConversationSynced) {
|
||||||
? initialMessages
|
return initialMessages ?? []
|
||||||
: messages
|
}
|
||||||
console.log('[AssistantChat] displayMessages decision:', {
|
|
||||||
conversationId,
|
// If no initial messages, just show live messages
|
||||||
lastRef: lastConversationIdRef.current,
|
if (!initialMessages || initialMessages.length === 0) {
|
||||||
isConversationSynced,
|
return messages
|
||||||
initialMessagesCount: initialMessages?.length ?? 0,
|
}
|
||||||
messagesCount: messages.length,
|
|
||||||
displayMessagesCount: displayMessages.length,
|
// Merge both arrays, deduplicating by ID (live messages take precedence)
|
||||||
showingInitial: displayMessages === initialMessages
|
const messageMap = new Map<string, ChatMessage>()
|
||||||
})
|
for (const msg of initialMessages) {
|
||||||
|
messageMap.set(msg.id, msg)
|
||||||
|
}
|
||||||
|
for (const msg of messages) {
|
||||||
|
messageMap.set(msg.id, msg)
|
||||||
|
}
|
||||||
|
return Array.from(messageMap.values())
|
||||||
|
}, [initialMessages, messages, conversationId, isLoadingConversation])
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="flex flex-col h-full">
|
<div className="flex flex-col h-full">
|
||||||
@@ -288,7 +292,7 @@ export function AssistantChat({
|
|||||||
onChange={(e) => setInputValue(e.target.value)}
|
onChange={(e) => setInputValue(e.target.value)}
|
||||||
onKeyDown={handleKeyDown}
|
onKeyDown={handleKeyDown}
|
||||||
placeholder="Ask about the codebase..."
|
placeholder="Ask about the codebase..."
|
||||||
disabled={isLoading || connectionStatus !== 'connected'}
|
disabled={isLoading || isLoadingConversation || connectionStatus !== 'connected'}
|
||||||
className="
|
className="
|
||||||
flex-1
|
flex-1
|
||||||
neo-input
|
neo-input
|
||||||
@@ -301,7 +305,7 @@ export function AssistantChat({
|
|||||||
/>
|
/>
|
||||||
<button
|
<button
|
||||||
onClick={handleSend}
|
onClick={handleSend}
|
||||||
disabled={!inputValue.trim() || isLoading || connectionStatus !== 'connected'}
|
disabled={!inputValue.trim() || isLoading || isLoadingConversation || connectionStatus !== 'connected'}
|
||||||
className="
|
className="
|
||||||
neo-btn neo-btn-primary
|
neo-btn neo-btn-primary
|
||||||
px-4
|
px-4
|
||||||
|
|||||||
@@ -76,7 +76,6 @@ export function ConversationHistory({
|
|||||||
}
|
}
|
||||||
|
|
||||||
const handleSelectConversation = (conversationId: number) => {
|
const handleSelectConversation = (conversationId: number) => {
|
||||||
console.log('[ConversationHistory] handleSelectConversation called with id:', conversationId)
|
|
||||||
onSelectConversation(conversationId)
|
onSelectConversation(conversationId)
|
||||||
onClose()
|
onClose()
|
||||||
}
|
}
|
||||||
@@ -129,11 +128,6 @@ export function ConversationHistory({
|
|||||||
<div className="max-h-[300px] overflow-auto">
|
<div className="max-h-[300px] overflow-auto">
|
||||||
{conversations.map((conversation) => {
|
{conversations.map((conversation) => {
|
||||||
const isCurrent = conversation.id === currentConversationId
|
const isCurrent = conversation.id === currentConversationId
|
||||||
console.log('[ConversationHistory] Rendering conversation:', {
|
|
||||||
id: conversation.id,
|
|
||||||
currentConversationId,
|
|
||||||
isCurrent
|
|
||||||
})
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div
|
<div
|
||||||
|
|||||||
@@ -120,7 +120,9 @@ export function useAssistantChat({
|
|||||||
ws.onmessage = (event) => {
|
ws.onmessage = (event) => {
|
||||||
try {
|
try {
|
||||||
const data = JSON.parse(event.data) as AssistantChatServerMessage;
|
const data = JSON.parse(event.data) as AssistantChatServerMessage;
|
||||||
console.log('[useAssistantChat] Received WebSocket message:', data.type, data);
|
if (import.meta.env.DEV) {
|
||||||
|
console.debug('[useAssistantChat] Received WebSocket message:', data.type, data);
|
||||||
|
}
|
||||||
|
|
||||||
switch (data.type) {
|
switch (data.type) {
|
||||||
case "text": {
|
case "text": {
|
||||||
@@ -278,7 +280,9 @@ export function useAssistantChat({
|
|||||||
payload.conversation_id = existingConversationId;
|
payload.conversation_id = existingConversationId;
|
||||||
setConversationId(existingConversationId);
|
setConversationId(existingConversationId);
|
||||||
}
|
}
|
||||||
console.log('[useAssistantChat] Sending start message:', payload);
|
if (import.meta.env.DEV) {
|
||||||
|
console.debug('[useAssistantChat] Sending start message:', payload);
|
||||||
|
}
|
||||||
wsRef.current.send(JSON.stringify(payload));
|
wsRef.current.send(JSON.stringify(payload));
|
||||||
} else if (wsRef.current?.readyState === WebSocket.CONNECTING) {
|
} else if (wsRef.current?.readyState === WebSocket.CONNECTING) {
|
||||||
checkAndSendTimeoutRef.current = window.setTimeout(checkAndSend, 100);
|
checkAndSendTimeoutRef.current = window.setTimeout(checkAndSend, 100);
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ export function useConversation(projectName: string | null, conversationId: numb
|
|||||||
queryKey: ['conversation', projectName, conversationId],
|
queryKey: ['conversation', projectName, conversationId],
|
||||||
queryFn: () => api.getAssistantConversation(projectName!, conversationId!),
|
queryFn: () => api.getAssistantConversation(projectName!, conversationId!),
|
||||||
enabled: !!projectName && !!conversationId,
|
enabled: !!projectName && !!conversationId,
|
||||||
|
staleTime: 30_000, // Cache for 30 seconds
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user