Improve loading spinner response #153
Improve loading spinner response #153exactlyallan wants to merge 4 commits intoNVIDIA-AI-Blueprints:developfrom
Conversation
Show an immediate spinner in research tabs while deep research is active but the first task, thinking, citation, or report data has not arrived yet. Made-with: Cursor
Backfill the final report on deep research success when live report content has not reached the store yet, and cover the completion flow with hook and panel regressions. Made-with: Cursor
Load archived report content when users open the research panel directly to the report tab for an already completed job, and cover the side-panel flow with regressions. Made-with: Cursor
Greptile SummaryThis PR improves the deep research panel's loading UX by adding per-tab pending spinners, auto-loading the report when the panel is opened directly to the Report tab, and backfilling the final report content when the SSE stream ends before the report has been written to the store. Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ResearchPanel
participant useLoadJobData
participant useDeepResearch
participant API
Note over ResearchPanel: Panel open, Report tab, no content
ResearchPanel->>ResearchPanel: shouldLoadReport = true (useEffect)
ResearchPanel->>useLoadJobData: requestReportLoad() → loadReport(jobId)
useLoadJobData->>API: getJobStatus(jobId)
API-->>useLoadJobData: { status: 'success' }
useLoadJobData->>API: getJobReport(jobId) + getJobState(jobId)
User->>ResearchPanel: Closes panel (closeRightPanel)
API-->>useLoadJobData: { has_report: true, report: '...' }
useLoadJobData->>useLoadJobData: setReportContent(report)
useLoadJobData->>ResearchPanel: setResearchPanelTab('report') ⚠️
useLoadJobData->>ResearchPanel: openRightPanel('research') ⚠️ re-opens panel
Note over useDeepResearch: Concurrent: SSE job completion
useDeepResearch->>useDeepResearch: onJobStatus('success') — isOwnerActive() ✅
useDeepResearch->>API: await getJobReport(jobId)
User->>ResearchPanel: Navigates to different conversation
API-->>useDeepResearch: report content
useDeepResearch->>useDeepResearch: patchConversationMessage(stale ownerConvId) ⚠️
useDeepResearch->>useDeepResearch: completeDeepResearch() / setStreaming(false)
Last reviewed commit: 825cfdd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const shouldLoadReport = | ||
| researchPanelTab === 'report' && | ||
| Boolean(deepResearchJobId) && | ||
| !hasReportContent && | ||
| !isDeepResearchStreaming && | ||
| !isStreamLoading |
There was a problem hiding this comment.
shouldLoadReport shows spinner but never triggers load
shouldLoadReport is used only in the JSX (line 307) to conditionally render a spinner, but there is no useEffect or other mechanism that actually calls loadReport() when this condition becomes true. This means if the component renders (e.g. on mount or a re-render from state change) while the panel is already open on the 'report' tab with a completed job and no report content, the user will see the "Drafting report..." spinner indefinitely — the report data will never be fetched.
Consider adding a useEffect to trigger the load:
useEffect(() => {
if (shouldLoadReport && deepResearchJobId) {
void loadReport(deepResearchJobId)
}
}, [shouldLoadReport, deepResearchJobId, loadReport])
Additionally, when shouldLoadReport is true the job is complete (!isDeepResearchStreaming), so the message "Drafting report..." is misleading — something like "Loading report..." would be more accurate for the archived-report case.
Trigger archived report loads for already-open report tabs, preserve loaded research data when backfilling a missing report, and tighten test coverage so type-check and CI stay clean. Made-with: Cursor
| const requestReportLoad = useCallback(() => { | ||
| if (!deepResearchJobId || pendingReportLoadJobIdRef.current === deepResearchJobId) return | ||
|
|
||
| pendingReportLoadJobIdRef.current = deepResearchJobId | ||
| void Promise.resolve(loadReport(deepResearchJobId)).finally(() => { | ||
| if (pendingReportLoadJobIdRef.current === deepResearchJobId) { | ||
| pendingReportLoadJobIdRef.current = null | ||
| } | ||
| }) |
There was a problem hiding this comment.
Auto-load triggers openRightPanel when panel is closed
requestReportLoad delegates to loadReport, which ultimately calls loadJobData. On success, loadJobData unconditionally calls setResearchPanelTab('report') and openRightPanel('research') (lines 541–542 in use-load-job-data.ts). This means if the user closes the panel while an auto-triggered report fetch is in-flight (e.g., the 200–500 ms API round-trip), the completion will forcefully re-open the panel the user just closed.
The navigation side effects are correct for the explicit "View Report" button flow, but are a regression here where loadReport is being used as a pure data-fetch. Consider either adding a noNavigate option to loadJobData / loadReport, or having requestReportLoad check panel state before invoking the underlying hook:
const requestReportLoad = useCallback(() => {
if (!deepResearchJobId || pendingReportLoadJobIdRef.current === deepResearchJobId) return
pendingReportLoadJobIdRef.current = deepResearchJobId
void Promise.resolve(loadReport(deepResearchJobId)).finally(() => {
if (pendingReportLoadJobIdRef.current === deepResearchJobId) {
pendingReportLoadJobIdRef.current = null
}
})
}, [deepResearchJobId, loadReport])The .finally cleanup exists but there is no guard inside loadJobData against the navigation after the panel has been closed while the fetch was in-flight.
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 showPendingTabSpinner = | ||
| Boolean(deepResearchJobId) && | ||
| isDeepResearchStreaming && | ||
| !isStreamLoading && | ||
| ((researchPanelTab === 'tasks' && deepResearchTodosCount === 0) || | ||
| (researchPanelTab === 'thinking' && !hasThinkingContent) || | ||
| (researchPanelTab === 'citations' && deepResearchCitationsCount === 0) || | ||
| (researchPanelTab === 'report' && !hasReportContent)) |
There was a problem hiding this comment.
shouldLoadReport spinner shown even when panel is closed
The shouldLoadReport condition correctly guards on isOpen, but the JSX at line 328 uses showPendingTabSpinner || shouldLoadReport as the condition for the spinner block. showPendingTabSpinner does not require isOpen, so the spinner markup is rendered even when the panel is hidden (aria-hidden="true"). While it does not affect visual output (the panel is invisible), it creates unnecessary DOM nodes and could cause unexpected aria-label matches in tests. Consider gating showPendingTabSpinner on isOpen as well:
const showPendingTabSpinner =
isOpen &&
Boolean(deepResearchJobId) &&
isDeepResearchStreaming &&
...1 participant
Summary