Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,16 @@ const mockCreateDeepResearchClient = vi.fn((options: { callbacks: Record<string,

const mockCancelJob = vi.fn()
const mockGetJobStatus = vi.fn<() => Promise<{ status: string }>>().mockResolvedValue({ status: 'running' })
const mockGetJobReport = vi.fn<() => Promise<{ has_report: boolean; report?: string }>>().mockResolvedValue({ has_report: false })
const mockGetJobReport = vi
.fn<(jobId: string, authToken?: string) => Promise<{ has_report: boolean; report?: string }>>()
.mockResolvedValue({ has_report: false })

vi.mock('@/adapters/api', () => ({
createDeepResearchClient: (options: { callbacks: Record<string, (...args: unknown[]) => void> }) =>
mockCreateDeepResearchClient(options),
cancelJob: (...args: unknown[]) => mockCancelJob(...args),
getJobStatus: () => mockGetJobStatus(),
getJobReport: () => mockGetJobReport(),
getJobReport: (jobId: string, authToken?: string) => mockGetJobReport(jobId, authToken),
}))

import { useChatStore } from '../store'
Expand Down Expand Up @@ -595,6 +597,47 @@ describe('useDeepResearch', () => {
)
})

test('onJobStatus success backfills missing report content before finalizing', async () => {
mockGetJobReport.mockResolvedValueOnce({
has_report: true,
report: 'Recovered final report',
})

await setupConnectedHook({
reportContent: '',
activeDeepResearchMessageId: 'msg-123',
})

vi.mocked(useChatStore).getState = vi.fn(() => ({
...mockStoreState,
reportContent: '',
deepResearchLLMSteps: [],
deepResearchToolCalls: [],
deepResearchCitations: [],
addErrorCard: mockAddErrorCard,
deepResearchOwnerConversationId: 'test-conv-123',
activeDeepResearchMessageId: 'msg-123',
})) as unknown as typeof useChatStore.getState

await act(async () => {
await mockClient?.callbacks.onJobStatus?.('success', undefined)
})

expect(mockGetJobReport).toHaveBeenCalledWith('job-456', 'mock-id-token')
expect(mockSetReportContent).toHaveBeenCalledWith('Recovered final report')
expect(mockPatchConversationMessage).toHaveBeenCalledWith(
'test-conv-123',
'msg-123',
expect.objectContaining({
deepResearchJobStatus: 'success',
isDeepResearchActive: false,
showViewReport: true,
})
)
expect(mockSetStreamLoaded).toHaveBeenCalledWith(true)
expect(mockCompleteDeepResearch).toHaveBeenCalled()
})

test('onJobStatus failure stops todos and shows error', async () => {
await setupConnectedHook()

Expand Down
20 changes: 17 additions & 3 deletions frontends/ui/src/features/chat/hooks/use-deep-research.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useEffect, useRef, useCallback, useState } from 'react'
import {
createDeepResearchClient,
cancelJob,
getJobReport,
type DeepResearchClient,
type DeepResearchJobStatus,
type TodoItem,
Expand Down Expand Up @@ -248,7 +249,7 @@ export const useDeepResearch = (): UseDeepResearchReturn => {
}
},

onJobStatus: (status, error) => {
onJobStatus: async (status, error) => {
if (buf.active) flushBuffer()
if (!isOwnerActive()) return
resetTimeout()
Expand All @@ -266,10 +267,23 @@ export const useDeepResearch = (): UseDeepResearchReturn => {

if (status === 'success') {
setCurrentStatus('complete')
const { reportContent: currentReport, deepResearchLLMSteps, deepResearchToolCalls } = state
let resolvedReport = state.reportContent
if (!resolvedReport?.trim()) {
try {
const response = await getJobReport(jobId, idToken || undefined)
if (response.has_report && response.report?.trim()) {
resolvedReport = response.report
setReportContent(response.report)
}
} catch (reportError) {
console.warn('Failed to backfill final report after deep research success:', reportError)
}
}
Comment on lines +271 to +281
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ownership not re-validated after await getJobReport

isOwnerActive() is called once at the top of the callback (line 254), but the code makes multiple store mutations after the await getJobReport(...) call. If the user switches conversations during the async fetch (a few hundred ms window), ownerConvId and messageId were captured from the pre-await state snapshot and are now stale. The subsequent patchConversationMessage, stopAllDeepResearchSpinners, completeDeepResearch, and setStreaming(false) calls will mutate state for the old session, potentially corrupting the current view.

Adding a guard after the await prevents this:

const response = await getJobReport(jobId, idToken || undefined)
if (!isOwnerActive()) return  // re-check after async gap
if (response.has_report && response.report?.trim()) {
  resolvedReport = response.report
  setReportContent(response.report)
}


const { deepResearchLLMSteps, deepResearchToolCalls } = state
const totalTokens = deepResearchLLMSteps.reduce((sum, step) => sum + (step.usage?.input_tokens || 0) + (step.usage?.output_tokens || 0), 0)
const toolCallCount = deepResearchToolCalls.length
Comment on lines +283 to 285
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State snapshot read before await is stale after

deepResearchLLMSteps and deepResearchToolCalls (used for totalTokens and toolCallCount in the success banner) are read from state – a snapshot captured before the await getJobReport. If the SSE stream delivers any final LLM or tool-call events while the fetch is in-flight, the token/call counts in the banner will be incorrect. Consider re-reading those values from useChatStore.getState() after the await:

const freshState = useChatStore.getState()
const { deepResearchLLMSteps, deepResearchToolCalls } = freshState

const hasReport = Boolean(currentReport?.trim())
const hasReport = Boolean(resolvedReport?.trim())

if (ownerConvId && messageId) {
patchConversationMessage(ownerConvId, messageId, {
Expand Down
18 changes: 18 additions & 0 deletions frontends/ui/src/features/chat/hooks/use-load-job-data.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,22 @@ describe('useLoadJobData', () => {
'Failed to get job status: 404'
)
})

test('preserves loaded stream data when fetching missing report for same completed job', async () => {
mockStoreState.deepResearchJobId = 'job-123'
mockStoreState.deepResearchStreamLoaded = true
mockGetJobStatus.mockResolvedValue({ status: 'success' })
mockGetJobReport.mockResolvedValue({ has_report: true, report: 'Recovered report' })
mockGetJobState.mockResolvedValue({ has_state: false, artifacts: undefined })

const { result } = renderHook(() => useLoadJobData())

await act(async () => {
await result.current.loadReport('job-123')
})

expect(mockClearDeepResearch).not.toHaveBeenCalled()
expect(mockSetReportContent).toHaveBeenCalledWith('Recovered report')
expect(mockSetLoadedJobId).toHaveBeenCalledWith('job-123')
})
})
8 changes: 7 additions & 1 deletion frontends/ui/src/features/chat/hooks/use-load-job-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,10 @@ export const useLoadJobData = (): UseLoadJobDataReturn => {
const hasStreamData =
currentState.deepResearchJobId === jobId &&
currentState.deepResearchStreamLoaded
const preserveExistingResearchState =
!shouldStreamFull &&
currentState.deepResearchJobId === jobId &&
currentState.deepResearchStreamLoaded

// If we have what we need, just open the panel
if (hasReportData && (!shouldStreamFull || hasStreamData)) {
Expand All @@ -514,7 +518,9 @@ export const useLoadJobData = (): UseLoadJobDataReturn => {
throw new Error(`Job is still ${jobStatus}. Cannot load data from incomplete job.`)
}

clearDeepResearch()
if (!preserveExistingResearchState) {
clearDeepResearch()
}

if (shouldStreamFull) {
await streamFullJob(jobId)
Expand Down
107 changes: 106 additions & 1 deletion frontends/ui/src/features/layout/components/ResearchPanel.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

import { render, screen } from '@/test-utils'
import { waitFor } from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import { vi, describe, test, expect, beforeEach } from 'vitest'
import { ResearchPanel } from './ResearchPanel'
Expand Down Expand Up @@ -34,7 +35,16 @@ vi.mock('@/adapters/api', () => ({
let mockIsDeepResearchStreaming = false
let mockDeepResearchJobId: string | null = null
let mockDeepResearchStreamLoaded = false
let mockCurrentStatus: string | null = null
let mockReportContent = ''
let mockDeepResearchTodosCount = 0
let mockDeepResearchCitationsCount = 0
let mockDeepResearchLLMStepsCount = 0
let mockDeepResearchAgentsCount = 0
let mockDeepResearchToolCallsCount = 0
let mockDeepResearchFilesCount = 0
const mockImportJobStream = vi.fn()
const mockLoadReport = vi.fn().mockResolvedValue(undefined)

const mockCancelCurrentJob = vi.fn()

Expand All @@ -43,13 +53,30 @@ vi.mock('@/features/chat', () => ({
isDeepResearchStreaming: boolean
deepResearchJobId: string | null
deepResearchStreamLoaded: boolean
currentStatus: string | null
reportContent: string
deepResearchTodos: Array<unknown>
deepResearchCitations: Array<unknown>
deepResearchLLMSteps: Array<unknown>
deepResearchAgents: Array<unknown>
deepResearchToolCalls: Array<unknown>
deepResearchFiles: Array<unknown>
}) => unknown) =>
selector({
isDeepResearchStreaming: mockIsDeepResearchStreaming,
deepResearchJobId: mockDeepResearchJobId,
deepResearchStreamLoaded: mockDeepResearchStreamLoaded,
currentStatus: mockCurrentStatus,
reportContent: mockReportContent,
deepResearchTodos: Array.from({ length: mockDeepResearchTodosCount }),
deepResearchCitations: Array.from({ length: mockDeepResearchCitationsCount }),
deepResearchLLMSteps: Array.from({ length: mockDeepResearchLLMStepsCount }),
deepResearchAgents: Array.from({ length: mockDeepResearchAgentsCount }),
deepResearchToolCalls: Array.from({ length: mockDeepResearchToolCallsCount }),
deepResearchFiles: Array.from({ length: mockDeepResearchFilesCount }),
}),
useLoadJobData: () => ({
loadReport: mockLoadReport,
importStreamOnly: mockImportJobStream,
isLoading: false,
}),
Expand Down Expand Up @@ -77,7 +104,9 @@ vi.mock('./CitationsTab', () => ({

vi.mock('./ReportTab', () => ({
ReportTab: ({ children }: { children?: React.ReactNode }) => (
<div data-testid="report-tab">Report Tab Content {children}</div>
<div data-testid="report-tab">
{mockReportContent || 'Report Tab Content'} {children}
</div>
),
}))

Expand All @@ -89,7 +118,16 @@ describe('ResearchPanel', () => {
mockIsDeepResearchStreaming = false
mockDeepResearchJobId = null
mockDeepResearchStreamLoaded = false
mockCurrentStatus = null
mockReportContent = ''
mockDeepResearchTodosCount = 0
mockDeepResearchCitationsCount = 0
mockDeepResearchLLMStepsCount = 0
mockDeepResearchAgentsCount = 0
mockDeepResearchToolCallsCount = 0
mockDeepResearchFilesCount = 0
mockImportJobStream.mockClear()
mockLoadReport.mockClear()
})

describe('panel visibility', () => {
Expand Down Expand Up @@ -140,6 +178,19 @@ describe('ResearchPanel', () => {
expect(mockSetResearchPanelTab).toHaveBeenCalledWith('citations')
})

test('loads report data when report tab is clicked for a completed job without report content', async () => {
const user = userEvent.setup()
mockDeepResearchJobId = 'job-123'
mockResearchPanelTab = 'tasks'

render(<ResearchPanel isAuthenticated={true} />)

await user.click(screen.getByText('Report'))

expect(mockSetResearchPanelTab).toHaveBeenCalledWith('report')
expect(mockLoadReport).toHaveBeenCalledWith('job-123')
})

test('displays correct tab content based on researchPanelTab', () => {
mockResearchPanelTab = 'tasks'
const { rerender } = render(<ResearchPanel isAuthenticated={true} />)
Expand Down Expand Up @@ -225,6 +276,16 @@ describe('ResearchPanel', () => {
// When not streaming, the generate icon is shown instead of spinner
expect(screen.queryByLabelText('Researching')).not.toBeInTheDocument()
})

test('shows a tab loading spinner while a new deep research job is waiting for first task data', () => {
mockIsDeepResearchStreaming = true
mockDeepResearchJobId = 'job-123'
mockResearchPanelTab = 'tasks'

render(<ResearchPanel isAuthenticated={true} />)

expect(screen.getByLabelText('Preparing research tasks...')).toBeInTheDocument()
})
})

describe('children rendering', () => {
Expand All @@ -239,6 +300,36 @@ describe('ResearchPanel', () => {

expect(screen.getByTestId('custom-content')).toBeInTheDocument()
})

test('shows report content automatically when it arrives while report tab is open', () => {
mockResearchPanelTab = 'report'
mockIsDeepResearchStreaming = true
mockDeepResearchJobId = 'job-123'
mockCurrentStatus = 'writing'
mockReportContent = ''

const { rerender } = render(<ResearchPanel isAuthenticated={true} />)

expect(screen.getByLabelText('Drafting report...')).toBeInTheDocument()
expect(screen.queryByText('Recovered final report')).not.toBeInTheDocument()

mockReportContent = 'Recovered final report'
rerender(<ResearchPanel isAuthenticated={true} />)

expect(screen.queryByLabelText('Drafting report...')).not.toBeInTheDocument()
expect(screen.getByText('Recovered final report')).toBeInTheDocument()
})

test('loads report data automatically when panel is already open on report tab', async () => {
mockResearchPanelTab = 'report'
mockDeepResearchJobId = 'job-123'

render(<ResearchPanel isAuthenticated={true} />)

await waitFor(() => {
expect(mockLoadReport).toHaveBeenCalledWith('job-123')
})
})
})

describe('segmented control groups', () => {
Expand Down Expand Up @@ -291,5 +382,19 @@ describe('ResearchPanel', () => {

expect(mockOpenRightPanel).not.toHaveBeenCalled()
})

test('loads report data when opening panel directly on report tab for a completed job', async () => {
const user = userEvent.setup()
mockRightPanel = null
mockResearchPanelTab = 'report'
mockDeepResearchJobId = 'job-123'

render(<ResearchPanel isAuthenticated={true} />)

await user.click(screen.getByTestId('research-panel-toggle'))

expect(mockOpenRightPanel).toHaveBeenCalledWith('research')
expect(mockLoadReport).toHaveBeenCalledWith('job-123')
})
})
})
Loading
Loading