From 02b40fa34233e239f7b28b4f220d7d57f7912eae Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 22:18:15 +0100 Subject: [PATCH] fix: address all PR review findings for bulk delete/archive functionality Comprehensive code quality improvements addressing all review comments: **Critical Fixes:** - Fixed double-toggle bug where checkbox called onToggleSelect twice - Added accessibility attributes (role, tabIndex, onKeyDown) to interactive div **Error Handling:** - Removed error re-throws in async event handlers to prevent unhandled rejections - Added error logging to empty catch blocks in session-storage.ts - Added error handling with .catch() for dropdown menu promise rejections - Added logging for partial failures in bulk operations **Code Quality:** - Replaced non-null assertion (!) with explicit null check in session-manager.ts - Removed redundant async/await wrappers in archive/unarchive handlers - Removed duplicate loadInsightsSession calls from store functions - Added skip-first-run guard to prevent double load on component mount - Added clarifying comment for showArchived useEffect dependency **Performance:** - Eliminated redundant IPC calls by removing duplicate session reloads - Fixed flicker issue caused by double reload with different filters All changes maintain backward compatibility and improve user experience. Co-Authored-By: Claude Opus 4.6 --- .../src/main/insights/session-manager.ts | 5 ++-- .../src/main/insights/session-storage.ts | 6 +++-- .../components/ChatHistorySidebar.tsx | 27 +++++++++---------- .../src/renderer/components/Insights.tsx | 21 ++++++++++++--- .../src/renderer/stores/insights-store.ts | 14 ++-------- 5 files changed, 40 insertions(+), 33 deletions(-) diff --git a/apps/frontend/src/main/insights/session-manager.ts b/apps/frontend/src/main/insights/session-manager.ts index 32f03c11..83ad5429 100644 --- a/apps/frontend/src/main/insights/session-manager.ts +++ b/apps/frontend/src/main/insights/session-manager.ts @@ -20,8 +20,9 @@ export class SessionManager { */ loadSession(projectId: string, projectPath: string): InsightsSession | null { // Check in-memory cache first - if (this.sessions.has(projectId)) { - return this.sessions.get(projectId)!; + const cachedSession = this.sessions.get(projectId); + if (cachedSession) { + return cachedSession; } // Migrate old format if needed diff --git a/apps/frontend/src/main/insights/session-storage.ts b/apps/frontend/src/main/insights/session-storage.ts index 45eadfad..e5e01722 100644 --- a/apps/frontend/src/main/insights/session-storage.ts +++ b/apps/frontend/src/main/insights/session-storage.ts @@ -75,7 +75,8 @@ export class SessionStorage { session.archivedAt = new Date(); this.saveSession(projectPath, session); return true; - } catch { + } catch (error) { + console.error(`[SessionStorage] Failed to archive session ${sessionId}:`, error); return false; } } @@ -91,7 +92,8 @@ export class SessionStorage { delete session.archivedAt; this.saveSession(projectPath, session); return true; - } catch { + } catch (error) { + console.error(`[SessionStorage] Failed to unarchive session ${sessionId}:`, error); return false; } } diff --git a/apps/frontend/src/renderer/components/ChatHistorySidebar.tsx b/apps/frontend/src/renderer/components/ChatHistorySidebar.tsx index 6db464c7..fc57342e 100644 --- a/apps/frontend/src/renderer/components/ChatHistorySidebar.tsx +++ b/apps/frontend/src/renderer/components/ChatHistorySidebar.tsx @@ -86,7 +86,8 @@ export function ChatHistorySidebar({ }); }, []); - // Clear selection when showArchived toggles + // Clear selection when showArchived toggles - prevents selecting archived sessions when filter changes + // biome-ignore lint/correctness/useExhaustiveDependencies: showArchived is intentionally a dependency to reset selection on filter change useEffect(() => { setSelectedIds(new Set()); }, [showArchived]); @@ -144,8 +145,6 @@ export function ChatHistorySidebar({ setBulkDeleteOpen(false); } catch (error) { console.error('Failed to delete sessions:', error); - // Re-throw to allow parent components to handle - throw error; } } }; @@ -157,8 +156,6 @@ export function ChatHistorySidebar({ setSelectedIds(new Set()); } catch (error) { console.error('Failed to archive sessions:', error); - // Re-throw to allow parent components to handle - throw error; } } }; @@ -304,8 +301,8 @@ export function ChatHistorySidebar({ onCancelEdit={handleCancelEdit} onEditTitleChange={setEditTitle} onDelete={() => setDeleteSessionId(session.id)} - onArchive={onArchiveSession ? async () => await onArchiveSession(session.id) : undefined} - onUnarchive={onUnarchiveSession ? async () => await onUnarchiveSession(session.id) : undefined} + onArchive={onArchiveSession ? () => onArchiveSession(session.id).catch((e) => console.error('Archive failed:', e)) : undefined} + onUnarchive={onUnarchiveSession ? () => onUnarchiveSession(session.id).catch((e) => console.error('Unarchive failed:', e)) : undefined} isArchived={!!session.archivedAt} isSelectionMode={isSelectionMode} isSelected={selectedIds.has(session.id)} @@ -476,23 +473,25 @@ function SessionItem({ return (
{ + if (!isSelectionMode && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + onSelect(); + } + }} > {/* Content with reserved space for the menu button */}
{isSelectionMode ? ( -
{ - e.stopPropagation(); - onToggleSelect(); - }} - className="shrink-0" - > +
e.stopPropagation()}> onToggleSelect()} diff --git a/apps/frontend/src/renderer/components/Insights.tsx b/apps/frontend/src/renderer/components/Insights.tsx index ca3ef015..23b9c00f 100644 --- a/apps/frontend/src/renderer/components/Insights.tsx +++ b/apps/frontend/src/renderer/components/Insights.tsx @@ -149,8 +149,13 @@ export function Insights({ projectId }: InsightsProps) { return cleanup; }, [projectId]); - // Reload sessions when showArchived changes + // Reload sessions when showArchived changes (skip first run to avoid duplicate load) + const isFirstRun = useRef(true); useEffect(() => { + if (isFirstRun.current) { + isFirstRun.current = false; + return; + } loadInsightsSessions(projectId, showArchived); }, [projectId, showArchived]); @@ -221,13 +226,23 @@ export function Insights({ projectId }: InsightsProps) { }; const handleDeleteSessions = async (sessionIds: string[]) => { - await deleteSessions(projectId, sessionIds); + const result = await deleteSessions(projectId, sessionIds); await loadInsightsSessions(projectId, showArchived); + + // Log partial failures for debugging + if (result.failedIds && result.failedIds.length > 0) { + console.warn(`Failed to delete ${result.failedIds.length} session(s):`, result.failedIds); + } }; const handleArchiveSessions = async (sessionIds: string[]) => { - await archiveSessions(projectId, sessionIds); + const result = await archiveSessions(projectId, sessionIds); await loadInsightsSessions(projectId, showArchived); + + // Log partial failures for debugging + if (result.failedIds && result.failedIds.length > 0) { + console.warn(`Failed to archive ${result.failedIds.length} session(s):`, result.failedIds); + } }; const handleToggleShowArchived = () => { diff --git a/apps/frontend/src/renderer/stores/insights-store.ts b/apps/frontend/src/renderer/stores/insights-store.ts index 8e56bc83..f397a82f 100644 --- a/apps/frontend/src/renderer/stores/insights-store.ts +++ b/apps/frontend/src/renderer/stores/insights-store.ts @@ -316,7 +316,6 @@ export async function renameSession(projectId: string, sessionId: string, newTit export async function deleteSessions(projectId: string, sessionIds: string[]): Promise<{ success: boolean; failedIds?: string[] }> { const result = await window.electronAPI.deleteInsightsSessions(projectId, sessionIds); if (result.success) { - await loadInsightsSession(projectId); return { success: true }; } return { success: false, failedIds: result.data?.failedIds }; @@ -324,17 +323,12 @@ export async function deleteSessions(projectId: string, sessionIds: string[]): P export async function archiveSession(projectId: string, sessionId: string): Promise { const result = await window.electronAPI.archiveInsightsSession(projectId, sessionId); - if (result.success) { - await loadInsightsSession(projectId); - return true; - } - return false; + return result.success; } export async function archiveSessions(projectId: string, sessionIds: string[]): Promise<{ success: boolean; failedIds?: string[] }> { const result = await window.electronAPI.archiveInsightsSessions(projectId, sessionIds); if (result.success) { - await loadInsightsSession(projectId); return { success: true }; } return { success: false, failedIds: result.data?.failedIds }; @@ -342,11 +336,7 @@ export async function archiveSessions(projectId: string, sessionIds: string[]): export async function unarchiveSession(projectId: string, sessionId: string): Promise { const result = await window.electronAPI.unarchiveInsightsSession(projectId, sessionId); - if (result.success) { - await loadInsightsSession(projectId); - return true; - } - return false; + return result.success; } export async function updateModelConfig(projectId: string, sessionId: string, modelConfig: InsightsModelConfig): Promise {