feature: add conversation history feature to AI assistant #74
Conversation
- Add ConversationHistory dropdown component with list of past conversations - Add useConversations hook for fetching and managing conversations via React Query - Implement conversation switching with proper state management - Fix bug where reopening panel showed new greeting instead of resuming conversation - Fix bug where selecting from history caused conversation ID to revert - Add server-side history context loading for resumed conversations - Add Playwright E2E tests for conversation history feature - Add logging for debugging conversation flow Key changes: - AssistantPanel: manages conversation state with localStorage persistence - AssistantChat: header with [+] New Chat and [History] buttons - Server: skips greeting for resumed conversations, loads history context on first message - Fixed race condition in onConversationCreated callback
📝 WalkthroughWalkthroughThis pull request adds conversation history support across backend and frontend: conditional greeting/resume logic and history-aware context on the server, new UI components and hooks for listing/selecting/deleting conversations, localStorage persistence of selected conversation, Playwright E2E tests, and Playwright config & scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Client UI
participant Server
participant DB as Database
participant Claude
rect rgba(200,150,255,0.5)
Note over User,Server: Select existing conversation
User->>UI: Choose conversation from History
UI->>Server: GET conversation messages
Server->>DB: Query conversation & messages
DB-->>Server: Return messages
Server-->>UI: Respond with initialMessages
UI->>UI: Display messages (no greeting)
end
rect rgba(150,200,255,0.5)
Note over User,Claude: First user message for resumed conversation
User->>UI: Send user message
UI->>Server: WebSocket start / send message
Server->>Server: Check _history_loaded flag
Server->>Claude: Send combined prior-history + new message as context
Claude-->>Server: Stream response chunks
Server-->>UI: Stream response chunks to client
Server->>Server: Set _history_loaded = true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/components/AssistantChat.tsx (1)
147-153: Block sending while a conversation is still loadingWhile
isLoadingConversationis true,start()is skipped, sosendMessagecan still target the previous conversation. Disable input/send and guardhandleSendto prevent misrouted messages.🔧 Proposed fix
const handleSend = () => { const content = inputValue.trim() - if (!content || isLoading) return + if (!content || isLoading || isLoadingConversation) return sendMessage(content) setInputValue('') } @@ placeholder="Ask about the codebase..." - disabled={isLoading || connectionStatus !== 'connected'} + disabled={isLoading || isLoadingConversation || connectionStatus !== 'connected'} @@ - disabled={!inputValue.trim() || isLoading || connectionStatus !== 'connected'} + disabled={ + !inputValue.trim() || + isLoading || + isLoadingConversation || + connectionStatus !== 'connected' + }Also applies to: 291-305
🤖 Fix all issues with AI agents
In `@ui/src/components/AssistantChat.tsx`:
- Around line 60-75: The effect is comparing and storing the prop conversationId
instead of the internal activeConversationId which causes duplicate
onConversationCreated calls when the parent updates conversationId; change the
ref to track activeConversationId (initialize previousConversationIdRef with
activeConversationId and set previousConversationIdRef.current =
activeConversationId inside the useEffect), remove conversationId from the
dependency array, and keep [activeConversationId, onConversationCreated] as the
effect dependencies so onConversationCreated only fires once when the internal
activeConversationId transitions from null/undefined to a value.
- Around line 162-170: displayMessages currently switches from initialMessages
to messages when the first live message arrives, causing initial history loss;
update the logic that computes displayMessages (referencing initialMessages,
messages, lastConversationIdRef, conversationId, isConversationSynced) to merge
both arrays instead of replacing, deduplicating by message ID so any message
present in either source appears once and maintaining order (e.g., keep
initialMessages first then append live messages whose IDs are not already
present) and only fall back to messages when no initialMessages exist or
conversation sync indicates full replacement.
🧹 Nitpick comments (11)
ui/src/hooks/useAssistantChat.ts (1)
122-124: Guard WebSocket debug logs behind a dev flag.These logs include full payloads; consider gating them to avoid leaking message content and reduce console noise in production.
♻️ Proposed tweak
- console.log('[useAssistantChat] Received WebSocket message:', data.type, data); + if (import.meta.env.DEV) { + console.debug('[useAssistantChat] Received WebSocket message:', data.type, data); + }- console.log('[useAssistantChat] Sending start message:', payload); + if (import.meta.env.DEV) { + console.debug('[useAssistantChat] Sending start message:', payload); + }Also applies to: 274-282
server/routers/assistant_chat.py (1)
272-288: Avoid info-level logging per streamed chunk.Per-chunk
infologs can be extremely noisy and expensive in production; consider downgrading todebugand gating on log level.♻️ Suggested adjustment
- logger.info(f"Sending chunk: {chunk.get('type')}") + if logger.isEnabledFor(logging.DEBUG): + logger.debug("Sending chunk: %s", chunk.get("type")).gitignore (1)
67-67: Normalize gitignore paths (remove leading./).The leading
./prefix in gitignore patterns is redundant. Git normalizes pathnames relative to the.gitignorefile without a./prefix, making it unnecessary and non-standard. Removing it follows gitignore conventions.♻️ Proposed changes
-./ui/playwright-report +ui/playwright-report/-./ui/test-results +ui/test-results/Also applies to: 146-146
ui/src/components/ConversationHistory.tsx (2)
79-82: Remove debug console.log statements before merging.These logging statements are useful during development but should be removed or converted to conditional debug logging for production code.
🔧 Remove debug logs
const handleSelectConversation = (conversationId: number) => { - console.log('[ConversationHistory] handleSelectConversation called with id:', conversationId) onSelectConversation(conversationId) onClose() }{conversations.map((conversation) => { const isCurrent = conversation.id === currentConversationId - console.log('[ConversationHistory] Rendering conversation:', { - id: conversation.id, - currentConversationId, - isCurrent - }) return (Also applies to: 132-136
62-72: Consider showing user feedback on delete failure.Currently, delete errors are only logged to console. Users won't know if deletion failed.
💡 Optional: Add error state or toast notification
const handleConfirmDelete = async () => { if (!conversationToDelete) return try { await deleteConversation.mutateAsync(conversationToDelete.id) setConversationToDelete(null) } catch (error) { console.error('Failed to delete conversation:', error) + // Consider adding a toast notification here setConversationToDelete(null) } }ui/src/hooks/useConversations.ts (1)
23-29: Consider adding staleTime for single conversation queries.Unlike
useConversationswhich has a 30-second staleTime, this hook has no staleTime. If the conversation is fetched multiple times in quick succession, it may cause unnecessary API calls.💡 Optional: Add staleTime for consistency
export function useConversation(projectName: string | null, conversationId: number | null) { return useQuery({ queryKey: ['conversation', projectName, conversationId], queryFn: () => api.getAssistantConversation(projectName!, conversationId!), enabled: !!projectName && !!conversationId, + staleTime: 30000, // Consistent with useConversations }) }server/services/assistant_chat_session.py (1)
341-361: History loading logic is sound, but consider edge cases.The implementation correctly:
- Only loads history once via
_history_loadedflag- Excludes the just-added message (
history[:-1])- Truncates long messages to 500 chars
However, there's no limit on the total number of history messages loaded. For very long conversations, this could result in a large context block.
💡 Consider limiting history messages
if not self._history_loaded: self._history_loaded = True history = get_messages(self.project_dir, self.conversation_id) # Exclude the message we just added (last one) history = history[:-1] if history else [] + # Limit to last N messages to avoid context overflow + MAX_HISTORY_MESSAGES = 20 + if len(history) > MAX_HISTORY_MESSAGES: + history = history[-MAX_HISTORY_MESSAGES:] if history:ui/e2e/conversation-history.spec.ts (2)
26-40: Consider extracting shared helper functions to reduce duplication.The
selectProject,waitForPanelOpen, andwaitForPanelClosedfunctions are duplicated between the two test.describe blocks.💡 Extract to shared helpers
// At the top of the file, after imports async function selectProject(page: import('@playwright/test').Page) { const projectSelector = page.locator('button:has-text("Select Project")') if (await projectSelector.isVisible()) { await projectSelector.click() const projectItem = page.locator('.neo-dropdown-item').first() const hasProject = await projectItem.isVisible().catch(() => false) if (!hasProject) return false await projectItem.click() await page.waitForTimeout(500) return true } return false } async function waitForPanelOpen(page: import('@playwright/test').Page) { await page.waitForFunction(() => { const panel = document.querySelector('[aria-label="Project Assistant"]') return panel && panel.getAttribute('aria-hidden') !== 'true' }, { timeout: 5000 }) } async function waitForPanelClosed(page: import('@playwright/test').Page) { await page.waitForFunction(() => { const panel = document.querySelector('[aria-label="Project Assistant"]') return !panel || panel.getAttribute('aria-hidden') === 'true' }, { timeout: 5000 }) }Also applies to: 316-328
36-37: FixedwaitForTimeoutcalls may cause flakiness.Multiple fixed waits (500ms, 1000ms, 2000ms) can make tests flaky in CI. Consider using explicit wait conditions where possible.
For example, instead of:
await page.waitForTimeout(500)Use condition-based waits:
await page.waitForSelector('.expected-element') // or await expect(someLocator).toBeVisible()Also applies to: 324-325, 402-403, 416-417, 444-444, 488-488
ui/src/components/AssistantPanel.tsx (2)
65-70: Remove debug console.log statement before merging.Similar to ConversationHistory.tsx, this debug logging should be removed for production.
🔧 Remove debug log
- console.log('[AssistantPanel] State:', { - conversationId, - isLoadingConversation, - conversationDetailId: conversationDetail?.id, - initialMessagesCount: initialMessages?.length ?? 0 - })
73-80: Consider consolidating the two effects to avoid potential timing issues.Having two separate effects that both depend on
projectNamecould lead to subtle race conditions during project switching. The second effect resetsconversationId, but the first effect would have already persisted the old value.💡 Consolidate effects
- // Persist conversation ID changes to localStorage - useEffect(() => { - setStoredConversationId(projectName, conversationId) - }, [projectName, conversationId]) - - // Reset conversation ID when project changes - useEffect(() => { - setConversationId(getStoredConversationId(projectName)) - }, [projectName]) + // Handle project changes and persist conversation ID + const prevProjectRef = useRef(projectName) + useEffect(() => { + if (prevProjectRef.current !== projectName) { + // Project changed - load stored ID for new project + prevProjectRef.current = projectName + setConversationId(getStoredConversationId(projectName)) + } else { + // Same project - persist current conversation ID + setStoredConversationId(projectName, conversationId) + } + }, [projectName, conversationId])
lirielgozi
changed the title
- 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
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/assistant_chat_session.py (1)
200-211: Validate resumed conversation IDs before skipping creation.If a client passes a stale/deleted
conversation_id,start()treats it as a resume and skips creation/greeting; subsequentadd_message/get_messagesthen no-op, so the chat runs without persistence or history. Consider verifying the conversation exists and either fall back to a new conversation or return an error. You’ll also need to importget_conversationfromassistant_database.🐛 Suggested fix (validate before treating as resume)
- # Track if this is a new conversation (for greeting decision) - is_new_conversation = self.conversation_id is None + # Track if this is a new conversation (for greeting decision) + is_new_conversation = self.conversation_id is None + if not is_new_conversation: + existing = get_conversation(self.project_dir, self.conversation_id) + if not existing: + logger.warning( + "Conversation %s not found; starting a new one", + self.conversation_id, + ) + self.conversation_id = None + is_new_conversation = True
🧹 Nitpick comments (5)
server/services/assistant_chat_session.py (1)
341-368: Consider DB-level limits when loading history.
get_messagesreturns the full conversation and then slices to the last 35. For long conversations this can be unnecessarily heavy. Consider adding a limit/offset in the query (or a helper likeget_recent_messages) to fetch only the last N messages while preserving chronological order.ui/e2e/conversation-history.spec.ts (3)
26-55: Consider extracting shared test helpers to reduce duplication.The helper functions
selectProject,waitForPanelOpen, andwaitForPanelClosedare duplicated verbatim between the twotest.describeblocks. This increases maintenance burden when behavior needs to change.♻️ Suggested refactor
Extract shared helpers to a common scope or a separate file:
+// Shared test helpers +async function selectProject(page: import('@playwright/test').Page) { + const projectSelector = page.locator('button:has-text("Select Project")') + if (await projectSelector.isVisible()) { + await projectSelector.click() + const projectItem = page.locator('.neo-dropdown-item').first() + const hasProject = await projectItem.isVisible().catch(() => false) + if (!hasProject) return false + await projectItem.click() + await expect(projectSelector).not.toBeVisible({ timeout: 5000 }).catch(() => {}) + return true + } + return false +} + +async function waitForPanelOpen(page: import('@playwright/test').Page) { + await page.waitForFunction(() => { + const panel = document.querySelector('[aria-label="Project Assistant"]') + return panel && panel.getAttribute('aria-hidden') !== 'true' + }, { timeout: 5000 }) +} + +async function waitForPanelClosed(page: import('@playwright/test').Page) { + await page.waitForFunction(() => { + const panel = document.querySelector('[aria-label="Project Assistant"]') + return !panel || panel.getAttribute('aria-hidden') === 'true' + }, { timeout: 5000 }) +} + test.describe('Assistant Panel UI', () => { - // ... duplicated helpers removed + // Use shared helpers above })Also applies to: 317-344
37-37: Swallowing assertion errors may mask test failures.The
.catch(() => {})silently ignores the assertion failure. If the dropdown genuinely fails to close, subsequent test steps may produce confusing failures. Consider logging or using a softer wait instead:- await expect(projectSelector).not.toBeVisible({ timeout: 5000 }).catch(() => {}) + // Wait briefly for dropdown animation, but don't fail if it's slow + await page.waitForTimeout(500)
484-487: Consider a more specific selector for the confirm dialog button.Using
.last()onbutton:has-text("Delete")assumes the confirm dialog's button is always last in the DOM. If another Delete button appears, this could click the wrong one.- const confirmButton = page.locator('button:has-text("Delete")').last() + // Target the confirmation dialog specifically + const confirmButton = page.locator('[role="alertdialog"] button:has-text("Delete"), .confirm-dialog button:has-text("Delete")').first()ui/src/components/AssistantChat.tsx (1)
81-86: Consider gating debug logs behind a development flag.The console.log statements aid debugging but will appear in production builds. The commit messages mention gating WebSocket debug logs behind a DEV flag—consider applying the same pattern here.
+const DEBUG = import.meta.env.DEV + // In the useEffect: - console.log('[AssistantChat] useEffect running:', { ... }) + if (DEBUG) console.log('[AssistantChat] useEffect running:', { ... })Also applies to: 90-91, 96-97, 104-108, 115-115, 120-120
…orks - Verified sidebar Settings link navigates to /settings page - Settings page displays all configuration options: - Model, Provider, Concurrency, YOLO Mode, Testing Agent Ratio - All steps verified with browser automation - Screenshots captured for documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Key changes:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.