diff --git a/frontend/src/components/sources/SourceDetailContent.test.tsx b/frontend/src/components/sources/SourceDetailContent.test.tsx index b80f3b78..85e6df9c 100644 --- a/frontend/src/components/sources/SourceDetailContent.test.tsx +++ b/frontend/src/components/sources/SourceDetailContent.test.tsx @@ -1,10 +1,13 @@ -import { render, screen, waitFor } from '@testing-library/react' +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { describe, it, expect, vi, beforeEach } from 'vitest' import { SourceDetailContent } from './SourceDetailContent' import { sourcesApi } from '@/lib/api/sources' +import { insightsApi } from '@/lib/api/insights' +import { transformationsApi } from '@/lib/api/transformations' import { QUERY_KEYS } from '@/lib/api/query-client' import { SourceDetailResponse } from '@/lib/types/api' +import { toast } from 'sonner' // useTranslation is mocked globally in setup.ts (t returns the key string) @@ -17,6 +20,8 @@ vi.mock('@/lib/api/sources', () => ({ vi.mock('@/lib/api/insights', () => ({ insightsApi: { listForSource: vi.fn().mockResolvedValue([]), + create: vi.fn(), + waitForCommand: vi.fn(), }, })) @@ -40,7 +45,41 @@ vi.mock('@/components/sources/NotebookAssociations', () => ({ NotebookAssociations: () => null, })) +vi.mock('@/components/ui/select', () => ({ + Select: ({ children, onValueChange, ...props }: React.PropsWithChildren<{ + onValueChange: (value: string) => void + }>) => ( + + ), + SelectTrigger: ({ children }: React.PropsWithChildren) => <>{children}, + SelectValue: () => null, + SelectContent: ({ children }: React.PropsWithChildren) => <>{children}, + SelectItem: ({ children, value }: React.PropsWithChildren<{ value: string }>) => ( + + ), +})) + +vi.mock('@/components/ui/tabs', () => ({ + Tabs: ({ children }: React.PropsWithChildren) =>
{children}
, + TabsList: ({ children }: React.PropsWithChildren) =>
{children}
, + TabsTrigger: ({ children }: React.PropsWithChildren) => , + TabsContent: ({ children }: React.PropsWithChildren) =>
{children}
, +})) + +vi.mock('sonner', () => ({ + toast: { + success: vi.fn(), + error: vi.fn(), + }, +})) + const mockSourcesGet = vi.mocked(sourcesApi.get) +const mockListInsights = vi.mocked(insightsApi.listForSource) +const mockCreateInsight = vi.mocked(insightsApi.create) +const mockWaitForCommand = vi.mocked(insightsApi.waitForCommand) +const mockListTransformations = vi.mocked(transformationsApi.list) const notFoundError = Object.assign(new Error('Request failed with status code 404'), { isAxiosError: true, @@ -52,18 +91,50 @@ const networkError = Object.assign(new Error('Network Error'), { response: undefined, }) -function renderContent(onClose?: () => void) { +function renderContent(onClose?: () => void, sourceId = 'source:missing') { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) return render( - + ) } +const loadedSource: SourceDetailResponse = { + id: 'source:loaded', + title: 'Loaded source', + asset: null, + embedded: false, + embedded_chunks: 0, + insights_count: 0, + created: '2026-01-01T00:00:00Z', + updated: '2026-01-01T00:00:00Z', + full_text: 'Source content', +} + +async function startInsightGeneration() { + await screen.findByText('Loaded source') + fireEvent.change(screen.getByRole('combobox', { name: 'transformation' }), { + target: { value: 'transformation:summary' }, + }) + fireEvent.click(screen.getByRole('button', { name: 'common.create' })) +} + describe('SourceDetailContent', () => { beforeEach(() => { vi.clearAllMocks() + mockListInsights.mockResolvedValue([]) + mockListTransformations.mockResolvedValue([{ + id: 'transformation:summary', + title: 'Summary', + name: 'summary', + description: '', + prompt: 'Summarize', + apply_default: false, + model_id: null, + created: '2026-01-01T00:00:00Z', + updated: '2026-01-01T00:00:00Z', + }]) }) it('shows the shared not-found state when the source returns 404', async () => { @@ -142,4 +213,83 @@ describe('SourceDetailContent', () => { screen.getByText('common.close').click() expect(onClose).toHaveBeenCalled() }) + + it('does not start polling when creation finishes after unmount', async () => { + mockSourcesGet.mockResolvedValue(loadedSource) + let resolveCreate!: (value: Awaited>) => void + mockCreateInsight.mockReturnValue(new Promise(resolve => { + resolveCreate = resolve + })) + + const view = renderContent(undefined, loadedSource.id) + await startInsightGeneration() + await waitFor(() => expect(mockCreateInsight).toHaveBeenCalled()) + + view.unmount() + await act(async () => { + resolveCreate({ + status: 'pending', + message: 'started', + source_id: loadedSource.id, + transformation_id: 'transformation:summary', + command_id: 'job-1', + }) + }) + + expect(mockWaitForCommand).not.toHaveBeenCalled() + }) + + it('shows the backend error when insight generation fails', async () => { + mockSourcesGet.mockResolvedValue(loadedSource) + mockCreateInsight.mockResolvedValue({ + status: 'pending', + message: 'started', + source_id: loadedSource.id, + transformation_id: 'transformation:summary', + command_id: 'job-1', + }) + mockWaitForCommand.mockResolvedValue({ + job_id: 'job-1', + status: 'failed', + error_message: 'Model ran out of memory', + }) + + renderContent(undefined, loadedSource.id) + await startInsightGeneration() + + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith('Model ran out of memory') + }) + }) + + it('does not apply terminal effects when unmounted during the final refresh', async () => { + mockSourcesGet.mockResolvedValue(loadedSource) + let resolveRefresh!: (value: Awaited>) => void + mockListInsights + .mockResolvedValueOnce([]) + .mockReturnValueOnce(new Promise(resolve => { + resolveRefresh = resolve + })) + mockCreateInsight.mockResolvedValue({ + status: 'pending', + message: 'started', + source_id: loadedSource.id, + transformation_id: 'transformation:summary', + command_id: 'job-1', + }) + mockWaitForCommand.mockResolvedValue({ + job_id: 'job-1', + status: 'failed', + error_message: 'Stale failure', + }) + + const view = renderContent(undefined, loadedSource.id) + await startInsightGeneration() + await waitFor(() => expect(mockListInsights).toHaveBeenCalledTimes(2)) + + view.unmount() + await act(async () => resolveRefresh([])) + + expect(toast.error).not.toHaveBeenCalledWith('Stale failure') + }) }) diff --git a/frontend/src/components/sources/SourceDetailContent.tsx b/frontend/src/components/sources/SourceDetailContent.tsx index 5a498399..964f37b1 100644 --- a/frontend/src/components/sources/SourceDetailContent.tsx +++ b/frontend/src/components/sources/SourceDetailContent.tsx @@ -1,6 +1,6 @@ 'use client' -import { useState, useEffect, useCallback, useMemo } from 'react' +import { useState, useEffect, useCallback, useMemo, useRef } from 'react' import { useQueryClient } from '@tanstack/react-query' import { MarkdownRenderer } from '@/components/ui/markdown-renderer' import { sourcesApi } from '@/lib/api/sources' @@ -114,6 +114,7 @@ function SourceDetailContentInner({ const [selectedInsight, setSelectedInsight] = useState(null) const [insightToDelete, setInsightToDelete] = useState(null) const [deletingInsight, setDeletingInsight] = useState(false) + const insightPollingRef = useRef(null) // A 404 means the source was deleted (e.g. a dangling chat/ask reference) — // handled by the shared "content no longer exists" state. The global query @@ -157,17 +158,25 @@ function SourceDetailContentInner({ } }, [fetchInsights, fetchTransformations, sourceId]) + useEffect(() => () => insightPollingRef.current?.abort(), []) + const createInsight = async () => { if (!selectedTransformation) { toast.error(t('sources.selectTransformation')) return } + insightPollingRef.current?.abort() + const controller = new AbortController() + insightPollingRef.current = controller + try { setCreatingInsight(true) const response = await insightsApi.create(sourceId, { transformation_id: selectedTransformation }) + if (controller.signal.aborted) return + // Show toast for async operation toast.success(t('sources.insightGenerationStarted')) setSelectedTransformation('') @@ -176,30 +185,50 @@ function SourceDetailContentInner({ if (response.command_id) { // Poll in background (don't block UI) insightsApi.waitForCommand(response.command_id, { - maxAttempts: 120, // Up to 4 minutes (120 * 2s) - intervalMs: 2000 - }).then(success => { - if (success) { - void fetchInsights() - // Invalidate sources queries so notebook page refreshes with updated insights_count - queryClient.invalidateQueries({ queryKey: ['sources'] }) + intervalMs: 2000, + signal: controller.signal, + }).then(async status => { + if (controller.signal.aborted) return + + await fetchInsights() + if (controller.signal.aborted) return + // Invalidate sources queries so notebook page refreshes with updated insights_count + queryClient.invalidateQueries({ queryKey: ['sources'] }) + if (status?.status !== 'completed') { + toast.error(status?.error_message || t('common.error')) } }).catch(err => { + if (controller.signal.aborted) return console.error('Error waiting for insight command:', err) + toast.error(t('common.error')) + }).finally(() => { + if (insightPollingRef.current === controller) { + insightPollingRef.current = null + } }) } else { // Fallback: refresh after delay if no command_id setTimeout(() => { + if (controller.signal.aborted) return void fetchInsights() // Also invalidate sources queries queryClient.invalidateQueries({ queryKey: ['sources'] }) + if (insightPollingRef.current === controller) { + insightPollingRef.current = null + } }, 5000) } } catch (err) { + if (controller.signal.aborted) return console.error('Failed to create insight:', err) toast.error(t('common.error')) + if (insightPollingRef.current === controller) { + insightPollingRef.current = null + } } finally { - setCreatingInsight(false) + if (!controller.signal.aborted) { + setCreatingInsight(false) + } } } diff --git a/frontend/src/lib/api/insights.test.ts b/frontend/src/lib/api/insights.test.ts new file mode 100644 index 00000000..0ef0ed2e --- /dev/null +++ b/frontend/src/lib/api/insights.test.ts @@ -0,0 +1,91 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import apiClient from './client' +import { insightsApi } from './insights' + +vi.mock('./client', () => ({ + default: { + get: vi.fn(), + }, +})) + +const mockGet = vi.mocked(apiClient.get) + +describe('insightsApi.waitForCommand', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('keeps polling while a command remains pending', async () => { + mockGet.mockImplementation(async () => ({ + data: { + job_id: 'job-1', + status: mockGet.mock.calls.length > 120 ? 'completed' : 'running', + }, + })) + + await expect( + insightsApi.waitForCommand('job-1', { intervalMs: 0 }) + ).resolves.toMatchObject({ status: 'completed' }) + expect(mockGet).toHaveBeenCalledTimes(121) + }) + + it('stops polling when aborted', async () => { + const controller = new AbortController() + mockGet.mockResolvedValue({ data: { job_id: 'job-1', status: 'running' } }) + + const polling = insightsApi.waitForCommand('job-1', { + intervalMs: 60_000, + signal: controller.signal, + }) + await vi.waitFor(() => expect(mockGet).toHaveBeenCalledTimes(1)) + + controller.abort() + + await expect(polling).resolves.toBeNull() + expect(mockGet).toHaveBeenCalledTimes(1) + }) + + it('stops polling when the job is unknown', async () => { + mockGet.mockResolvedValue({ data: { job_id: 'job-1', status: 'unknown' } }) + + await expect( + insightsApi.waitForCommand('job-1', { intervalMs: 0 }) + ).resolves.toMatchObject({ status: 'unknown' }) + expect(mockGet).toHaveBeenCalledTimes(1) + }) + + it('stops polling when the job reports an error', async () => { + const controller = new AbortController() + mockGet.mockResolvedValue({ data: { job_id: 'job-1', status: 'error' } }) + const abort = setTimeout(() => controller.abort(), 20) + + const status = await insightsApi.waitForCommand('job-1', { + intervalMs: 1, + signal: controller.signal, + }) + clearTimeout(abort) + + expect(status).toMatchObject({ status: 'error' }) + expect(mockGet).toHaveBeenCalledTimes(1) + }) + + it('stops after three consecutive status errors', async () => { + const statusError = Object.assign(new Error('status unavailable'), { + config: { headers: { Authorization: 'Bearer secret' } }, + }) + const errorLog = vi.spyOn(console, 'error').mockImplementation(() => undefined) + mockGet.mockRejectedValue(statusError) + + await expect( + insightsApi.waitForCommand('job-1', { intervalMs: 0 }) + ).resolves.toBeNull() + expect(mockGet).toHaveBeenCalledTimes(3) + expect(errorLog).toHaveBeenNthCalledWith(1, 'Error checking command status') + expect(errorLog).toHaveBeenNthCalledWith(2, 'Error checking command status') + expect(errorLog).toHaveBeenNthCalledWith(3, 'Error checking command status') + }) +}) diff --git a/frontend/src/lib/api/insights.ts b/frontend/src/lib/api/insights.ts index a2fbcde7..1bfdbc14 100644 --- a/frontend/src/lib/api/insights.ts +++ b/frontend/src/lib/api/insights.ts @@ -52,44 +52,63 @@ export const insightsApi = { await apiClient.delete(`/insights/${insightId}`) }, - getCommandStatus: async (commandId: string) => { + getCommandStatus: async (commandId: string, signal?: AbortSignal) => { const response = await apiClient.get( - `/commands/jobs/${commandId}` + `/commands/jobs/${commandId}`, + { signal } ) return response.data }, /** - * Poll command status until completed or failed. - * Returns true if completed successfully, false if failed. + * Poll command status until it reaches a terminal state. + * Returns the terminal status, or null if polling is aborted or repeatedly errors. */ waitForCommand: async ( commandId: string, - options?: { maxAttempts?: number; intervalMs?: number } - ): Promise => { - const maxAttempts = options?.maxAttempts ?? 60 // Default 60 attempts + options?: { intervalMs?: number; signal?: AbortSignal } + ): Promise => { const intervalMs = options?.intervalMs ?? 2000 // Default 2 seconds + const signal = options?.signal + let consecutiveErrors = 0 - for (let i = 0; i < maxAttempts; i++) { + while (!signal?.aborted) { + let status: CommandJobStatusResponse | undefined try { - const status = await insightsApi.getCommandStatus(commandId) - if (status.status === 'completed') { - return true - } + status = await insightsApi.getCommandStatus(commandId, signal) + consecutiveErrors = 0 + } catch { + if (signal?.aborted) return null + console.error('Error checking command status') + consecutiveErrors += 1 + if (consecutiveErrors >= 3) return null + } + + if (status && ['completed', 'failed', 'canceled', 'error', 'unknown'].includes(status.status)) { if (status.status === 'failed' || status.status === 'canceled') { console.error('Command failed:', status.error_message) - return false } - // Still running, wait and retry - await new Promise(resolve => setTimeout(resolve, intervalMs)) - } catch (error) { - console.error('Error checking command status:', error) - // Continue polling on error - await new Promise(resolve => setTimeout(resolve, intervalMs)) + return status } + + await new Promise(resolve => { + if (signal?.aborted) { + resolve() + return + } + + const onAbort = () => { + clearTimeout(timeout) + resolve() + } + const timeout = setTimeout(() => { + signal?.removeEventListener('abort', onAbort) + resolve() + }, intervalMs) + signal?.addEventListener('abort', onAbort, { once: true }) + }) } - // Timeout - console.warn('Command polling timed out') - return false + + return null } -} \ No newline at end of file +}