mirror of
https://github.com/leonvanzyl/autocoder.git
synced 2026-01-30 14:22:04 +00:00
fix: address performance and code quality issues in conversation history
Performance improvements: - Fix N+1 query in get_conversations() using COUNT subquery instead of len(c.messages) which triggered lazy loading for each conversation - Add SQLAlchemy engine caching to avoid creating new database connections on every request - Add React.memo to ChatMessage component to prevent unnecessary re-renders during message streaming - Move BOLD_REGEX to module scope to avoid recreating on each render Code quality improvements: - Remove 10+ console.log debug statements from AssistantChat.tsx and AssistantPanel.tsx that were left from development - Add user feedback for delete errors in ConversationHistory - dialog now stays open and shows error message instead of silently failing - Update ConfirmDialog to accept ReactNode for message prop to support rich error content These changes address issues identified in the code review of PR #74 (conversation history feature). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -78,22 +78,13 @@ export function AssistantChat({
|
||||
|
||||
// Start or resume the chat session when component mounts or conversationId changes
|
||||
useEffect(() => {
|
||||
console.log('[AssistantChat] useEffect running:', {
|
||||
conversationId,
|
||||
isLoadingConversation,
|
||||
lastRef: lastConversationIdRef.current,
|
||||
hasStarted: hasStartedRef.current
|
||||
})
|
||||
|
||||
// Skip if we're loading conversation details
|
||||
if (isLoadingConversation) {
|
||||
console.log('[AssistantChat] Skipping - loading conversation')
|
||||
return
|
||||
}
|
||||
|
||||
// Only start if conversationId has actually changed
|
||||
if (lastConversationIdRef.current === conversationId && hasStartedRef.current) {
|
||||
console.log('[AssistantChat] Skipping - same conversationId')
|
||||
return
|
||||
}
|
||||
|
||||
@@ -101,23 +92,15 @@ export function AssistantChat({
|
||||
const isSwitching = lastConversationIdRef.current !== undefined &&
|
||||
lastConversationIdRef.current !== conversationId
|
||||
|
||||
console.log('[AssistantChat] Processing conversation change:', {
|
||||
from: lastConversationIdRef.current,
|
||||
to: conversationId,
|
||||
isSwitching
|
||||
})
|
||||
|
||||
lastConversationIdRef.current = conversationId
|
||||
hasStartedRef.current = true
|
||||
|
||||
// Clear existing messages when switching conversations
|
||||
if (isSwitching) {
|
||||
console.log('[AssistantChat] Clearing messages for conversation switch')
|
||||
clearMessages()
|
||||
}
|
||||
|
||||
// Start the session with the conversation ID (or null for new)
|
||||
console.log('[AssistantChat] Starting session with conversationId:', conversationId)
|
||||
start(conversationId)
|
||||
}, [conversationId, isLoadingConversation, start, clearMessages])
|
||||
|
||||
@@ -129,7 +112,6 @@ export function AssistantChat({
|
||||
|
||||
// Handle selecting a conversation from history
|
||||
const handleSelectConversation = useCallback((id: number) => {
|
||||
console.log('[AssistantChat] handleSelectConversation called with id:', id)
|
||||
setShowHistory(false)
|
||||
onSelectConversation?.(id)
|
||||
}, [onSelectConversation])
|
||||
|
||||
@@ -62,13 +62,6 @@ export function AssistantPanel({ projectName, isOpen, onClose }: AssistantPanelP
|
||||
timestamp: msg.timestamp ? new Date(msg.timestamp) : new Date(),
|
||||
}))
|
||||
|
||||
console.log('[AssistantPanel] State:', {
|
||||
conversationId,
|
||||
isLoadingConversation,
|
||||
conversationDetailId: conversationDetail?.id,
|
||||
initialMessagesCount: initialMessages?.length ?? 0
|
||||
})
|
||||
|
||||
// Persist conversation ID changes to localStorage
|
||||
useEffect(() => {
|
||||
setStoredConversationId(projectName, conversationId)
|
||||
@@ -86,7 +79,6 @@ export function AssistantPanel({ projectName, isOpen, onClose }: AssistantPanelP
|
||||
|
||||
// Handle selecting a conversation from history
|
||||
const handleSelectConversation = useCallback((id: number) => {
|
||||
console.log('[AssistantPanel] handleSelectConversation called with id:', id)
|
||||
setConversationId(id)
|
||||
}, [])
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
* Supports user, assistant, and system messages with neobrutalism styling.
|
||||
*/
|
||||
|
||||
import { memo } from 'react'
|
||||
import { Bot, User, Info } from 'lucide-react'
|
||||
import type { ChatMessage as ChatMessageType } from '../lib/types'
|
||||
|
||||
@@ -12,7 +13,10 @@ interface ChatMessageProps {
|
||||
message: ChatMessageType
|
||||
}
|
||||
|
||||
export function ChatMessage({ message }: ChatMessageProps) {
|
||||
// Module-level regex to avoid recreating on each render
|
||||
const BOLD_REGEX = /\*\*(.*?)\*\*/g
|
||||
|
||||
export const ChatMessage = memo(function ChatMessage({ message }: ChatMessageProps) {
|
||||
const { role, content, attachments, timestamp, isStreaming } = message
|
||||
|
||||
// Format timestamp
|
||||
@@ -112,13 +116,13 @@ export function ChatMessage({ message }: ChatMessageProps) {
|
||||
{content && (
|
||||
<div className={`whitespace-pre-wrap text-sm leading-relaxed ${config.textColor}`}>
|
||||
{content.split('\n').map((line, i) => {
|
||||
// Bold text
|
||||
const boldRegex = /\*\*(.*?)\*\*/g
|
||||
// Bold text - use module-level regex, reset lastIndex for each line
|
||||
BOLD_REGEX.lastIndex = 0
|
||||
const parts = []
|
||||
let lastIndex = 0
|
||||
let match
|
||||
|
||||
while ((match = boldRegex.exec(line)) !== null) {
|
||||
while ((match = BOLD_REGEX.exec(line)) !== null) {
|
||||
if (match.index > lastIndex) {
|
||||
parts.push(line.slice(lastIndex, match.index))
|
||||
}
|
||||
@@ -196,4 +200,4 @@ export function ChatMessage({ message }: ChatMessageProps) {
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
})
|
||||
|
||||
@@ -5,12 +5,13 @@
|
||||
* Used to confirm destructive actions like deleting projects.
|
||||
*/
|
||||
|
||||
import type { ReactNode } from 'react'
|
||||
import { AlertTriangle, X } from 'lucide-react'
|
||||
|
||||
interface ConfirmDialogProps {
|
||||
isOpen: boolean
|
||||
title: string
|
||||
message: string
|
||||
message: ReactNode
|
||||
confirmLabel?: string
|
||||
cancelLabel?: string
|
||||
variant?: 'danger' | 'warning'
|
||||
@@ -75,9 +76,9 @@ export function ConfirmDialog({
|
||||
|
||||
{/* Content */}
|
||||
<div className="p-6">
|
||||
<p className="text-[var(--color-neo-text-secondary)] mb-6">
|
||||
<div className="text-[var(--color-neo-text-secondary)] mb-6">
|
||||
{message}
|
||||
</p>
|
||||
</div>
|
||||
|
||||
{/* Actions */}
|
||||
<div className="flex justify-end gap-3">
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
*/
|
||||
|
||||
import { useState, useEffect } from 'react'
|
||||
import { MessageSquare, Trash2, Loader2 } from 'lucide-react'
|
||||
import { MessageSquare, Trash2, Loader2, AlertCircle } from 'lucide-react'
|
||||
import { useConversations, useDeleteConversation } from '../hooks/useConversations'
|
||||
import { ConfirmDialog } from './ConfirmDialog'
|
||||
import type { AssistantConversation } from '../lib/types'
|
||||
@@ -50,10 +50,18 @@ export function ConversationHistory({
|
||||
onSelectConversation,
|
||||
}: ConversationHistoryProps) {
|
||||
const [conversationToDelete, setConversationToDelete] = useState<AssistantConversation | null>(null)
|
||||
const [deleteError, setDeleteError] = useState<string | null>(null)
|
||||
|
||||
const { data: conversations, isLoading } = useConversations(projectName)
|
||||
const deleteConversation = useDeleteConversation(projectName)
|
||||
|
||||
// Clear error when dropdown closes
|
||||
useEffect(() => {
|
||||
if (!isOpen) {
|
||||
setDeleteError(null)
|
||||
}
|
||||
}, [isOpen])
|
||||
|
||||
const handleDeleteClick = (e: React.MouseEvent, conversation: AssistantConversation) => {
|
||||
e.stopPropagation()
|
||||
setConversationToDelete(conversation)
|
||||
@@ -63,16 +71,18 @@ export function ConversationHistory({
|
||||
if (!conversationToDelete) return
|
||||
|
||||
try {
|
||||
setDeleteError(null)
|
||||
await deleteConversation.mutateAsync(conversationToDelete.id)
|
||||
setConversationToDelete(null)
|
||||
} catch (error) {
|
||||
console.error('Failed to delete conversation:', error)
|
||||
setConversationToDelete(null)
|
||||
} catch {
|
||||
// Keep dialog open and show error to user
|
||||
setDeleteError('Failed to delete conversation. Please try again.')
|
||||
}
|
||||
}
|
||||
|
||||
const handleCancelDelete = () => {
|
||||
setConversationToDelete(null)
|
||||
setDeleteError(null)
|
||||
}
|
||||
|
||||
const handleSelectConversation = (conversationId: number) => {
|
||||
@@ -183,7 +193,19 @@ export function ConversationHistory({
|
||||
<ConfirmDialog
|
||||
isOpen={conversationToDelete !== null}
|
||||
title="Delete Conversation"
|
||||
message={`Are you sure you want to delete "${conversationToDelete?.title || 'this conversation'}"? This action cannot be undone.`}
|
||||
message={
|
||||
deleteError ? (
|
||||
<div className="space-y-3">
|
||||
<p>{`Are you sure you want to delete "${conversationToDelete?.title || 'this conversation'}"? This action cannot be undone.`}</p>
|
||||
<div className="flex items-center gap-2 p-2 bg-[var(--color-neo-danger)]/10 border border-[var(--color-neo-danger)] rounded text-sm text-[var(--color-neo-danger)]">
|
||||
<AlertCircle size={16} className="flex-shrink-0" />
|
||||
<span>{deleteError}</span>
|
||||
</div>
|
||||
</div>
|
||||
) : (
|
||||
`Are you sure you want to delete "${conversationToDelete?.title || 'this conversation'}"? This action cannot be undone.`
|
||||
)
|
||||
}
|
||||
confirmLabel="Delete"
|
||||
cancelLabel="Cancel"
|
||||
variant="danger"
|
||||
|
||||
Reference in New Issue
Block a user