fix: followup panel routing and stream error handling#598
Conversation
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Review Summary by QodoFix followup panel routing and stream error handling
WalkthroughsDescription• Add try-catch-finally error handling to processEvents function • Remove hardcoded 'resolution_search' action from FollowupPanel submission • Add error handling to FollowupPanel form submission with try-catch • Implement try-catch-finally in querySuggestor with reduced throttle to 100ms • Ensure final flush of related queries and proper stream closure on errors Diagramflowchart LR
A["FollowupPanel<br/>removes hardcoded action"] -->|submit| B["processEvents<br/>try-catch-finally"]
B -->|on success| C["querySuggestor<br/>try-catch-finally"]
C -->|streams| D["objectStream<br/>with final flush"]
B -->|on error| E["Error handling<br/>& cleanup"]
C -->|on error| E
File Changes1. app/actions.tsx
|
Code Review by Qodo
1. Double stream finalization
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR improves error handling and stream finalization across three interconnected components. The server action refactors its main control flow into try/catch/finally to centralize cleanup, removes a fixed delay, and adjusts researcher invocation. The client form submission wraps the server call in error handling. The agent query generator applies the same error handling pattern with throttled updates and conditional caching. ChangesError Handling & Stream Finalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (action.object.next === 'inquire') { | ||
| const inquiry = await inquire(uiStream, messages) | ||
| uiStream.done() | ||
| isGenerating.done() | ||
| isCollapsed.done(false) | ||
| aiState.done({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: nanoid(), | ||
| role: 'assistant', | ||
| content: `inquiry: ${inquiry?.question}` | ||
| } | ||
| ] | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
1. Double stream finalization 🐞 Bug ☼ Reliability
In submit()'s processEvents(), the 'inquire' early-return path calls uiStream.done() and isGenerating.done() before returning, but the enclosing finally block always calls uiStream.done() and isGenerating.done(false) again. This can throw during finally and break inquiry submissions (or override the intended early return cleanup).
Agent Prompt
### Issue description
`processEvents()` closes `uiStream`/`isGenerating` inside the `action.object.next === 'inquire'` branch and then closes them again in `finally`. Because `finally` runs even after `return`, this creates a double-finalization path.
### Issue Context
Other flows in `submit()` (e.g., resolution search) use a single `finally` to close streamables, which avoids double-close hazards.
### Fix Focus Areas
- app/actions.tsx[404-431]
- app/actions.tsx[536-541]
### Suggested fix
- Remove `uiStream.done()` and `isGenerating.done()` from the `inquire` branch and let the `finally` handle closing.
- If the inquire branch needs different final values, set them (e.g., `isCollapsed.done(false)`) but do not close `uiStream`/`isGenerating` twice.
- Alternatively, add a boolean guard (e.g., `let finalized = false`) so the `finally` block only finalizes when not already finalized.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| isCollapsed.done(true) | ||
| let answer = '' | ||
| let toolOutputs: ToolResultPart[] = [] | ||
| let errorOccurred = false | ||
| const streamText = createStreamableValue<string>() | ||
| uiStream.update(<Spinner />) | ||
|
|
||
| while ( | ||
| useSpecificAPI | ||
| ? answer.length === 0 | ||
| : answer.length === 0 && !errorOccurred | ||
| ) { | ||
| const { fullResponse, hasError, toolResponses } = await researcher( | ||
| currentSystemPrompt, | ||
| uiStream, | ||
| streamText, | ||
| messages, | ||
| mapProvider, | ||
| useSpecificAPI, | ||
| drawnFeatures | ||
| ) | ||
| answer = fullResponse | ||
| toolOutputs = toolResponses | ||
| errorOccurred = hasError | ||
|
|
||
| if (toolOutputs.length > 0) { | ||
| toolOutputs.map(output => { | ||
| aiState.update({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: groupeId, | ||
| role: 'tool', | ||
| content: JSON.stringify(output.result), | ||
| name: output.toolName, | ||
| type: 'tool' | ||
| } | ||
| ] | ||
| }) | ||
| }) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (useSpecificAPI && answer.length === 0) { | ||
| const modifiedMessages = aiState | ||
| .get() | ||
| .messages.map(msg => | ||
| msg.role === 'tool' | ||
| ? { | ||
| ...msg, | ||
| role: 'assistant', | ||
| content: JSON.stringify(msg.content), | ||
| type: 'tool' | ||
| } | ||
| : msg | ||
| ) as CoreMessage[] | ||
| const latestMessages = modifiedMessages.slice(maxMessages * -1) | ||
| answer = await writer( | ||
| currentSystemPrompt, | ||
| uiStream, | ||
| streamText, | ||
| latestMessages | ||
| ) | ||
| } else { | ||
| streamText.done() | ||
| } | ||
| if (useSpecificAPI && answer.length === 0) { | ||
| const modifiedMessages = aiState | ||
| .get() | ||
| .messages.map(msg => | ||
| msg.role === 'tool' | ||
| ? { | ||
| ...msg, | ||
| role: 'assistant', | ||
| content: JSON.stringify(msg.content), | ||
| type: 'tool' | ||
| } | ||
| : msg | ||
| ) as CoreMessage[] | ||
| const latestMessages = modifiedMessages.slice(maxMessages * -1) | ||
| answer = await writer( | ||
| currentSystemPrompt, | ||
| uiStream, | ||
| streamText, | ||
| latestMessages | ||
| ) | ||
| } else { | ||
| streamText.done() | ||
| } |
There was a problem hiding this comment.
2. Streamtext not closed on throw 🐞 Bug ☼ Reliability
processEvents() creates streamText inside the try block but does not ensure it is closed in catch/finally when researcher()/writer()/querySuggestor() throws. This can leave the streamed BotMessage value hanging and leak resources in error scenarios (and can also leave isCollapsed never done if the throw happens before isCollapsed.done(...)).
Agent Prompt
### Issue description
`streamText` is created and used for streaming the assistant response, but it is not guaranteed to be finalized when an exception is thrown. This can leave the streamable value open even though `uiStream` is closed in `finally`.
### Issue Context
- `researcher()` streams and updates `streamText` but does not finalize it.
- `writer()` finalizes `streamText` only on normal completion.
### Fix Focus Areas
- app/actions.tsx[433-541]
- lib/agents/writer.tsx[34-49]
### Suggested fix
- Hoist `streamText` to an outer scope so it can be finalized from `finally`:
- `let streamText: ReturnType<typeof createStreamableValue<string>> | undefined;`
- assign it where it is created.
- In `finally`, call `streamText?.done()` (or `streamText?.error(error)` in `catch`) if it was created and not yet finalized.
- Also ensure `isCollapsed` is finalized in error paths (either in `finally` with a guard, or immediately before operations that may throw).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@coderabbitai its still very slow |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8 🏁 Script executed: Length of output: 3393 🏁 Script executed: Length of output: 20927 🏁 Script executed: Length of output: 28186
🐢 Root Causes of Slowness1. Extra LLM Call Before Streaming Starts (Biggest Impact)In Fix: Either run // In processEvents(), show spinner BEFORE taskManager resolves
uiStream.update(<Spinner />) // ← move this to BEFORE taskManager call
let action: any = { object: { next: 'proceed' } }
if (!skip) {
const taskManagerResult = await taskManager(messages)
...
}2. Artificial 500ms Delay — TwiceBoth await new Promise(resolve => setTimeout(resolve, 500)) // ← unnecessaryThis runs before 3.
|
| Issue | Location | Fix |
|---|---|---|
| No spinner until taskManager resolves | app/actions.tsx processEvents() |
Move uiStream.update(<Spinner />) before taskManager() |
| 500ms artificial delay × 2 | processEvents() & processResolutionSearch() |
Remove both setTimeout lines |
| querySuggestor blocks followup panel | processEvents() & processResolutionSearch() |
Show FollowupPanel immediately, don't await querySuggestor |
| getSystemPrompt blocks early return | app/actions.tsx submit fn |
Move inside processEvents() |
Would you like me to plan and generate the specific code changes for all of these fixes?
I've removed the artificial 500ms delays in the server actions that were contributing to the slowness. This should significantly improve responsiveness. |
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/actions.tsx (2)
399-403: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider moving
getSystemPromptinsideprocessEventsto reduce time-to-first-byte.
getSystemPrompt(userId)is awaited synchronously beforeprocessEvents()is invoked and before the function returns theuiStream.valueto the client. Any DB latency here delays the streamable component reaching the client, which directly contributes to the perceived slowness reported on this PR. Moving the await inside theprocessEventsIIFE lets the streamable values return immediately and the prompt fetch overlap with the spinner render.Also,
userIdis hard-coded to'anonymous', so the per-user dimension ofgetSystemPromptis not actually being used — worth confirming whether the call is needed at all on this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/actions.tsx` around lines 399 - 403, Move the getSystemPrompt call into the processEvents async IIFE so the UI stream can return immediately; currently getSystemPrompt(userId) is awaited before processEvents and delays sending uiStream.value to the client—update code to declare const userId = 'anonymous' (or derive real userId) but avoid awaiting getSystemPrompt outside processEvents, instead await it within processEvents (or remove the call if per-user prompt isn't needed) and then use the fetched currentSystemPrompt inside processEvents when composing messages; ensure references to getSystemPrompt, processEvents, userId, and uiStream.value are updated accordingly.
102-202: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
processResolutionSearchshadows the parentgroupeIdand runs detached.Two related concerns on this branch:
groupeIddeclared at Line 100 is shadowed and only used insideprocessResolutionSearch; it's distinct from the outergroupeIdat Line 329. The shadowing is fine but confusing — consider renaming for clarity.processResolutionSearch()at Line 202 is a fire-and-forget async call with novoid/error tether. Itstry/catch/finallyalready closesuiStream/isGeneratingand callssummaryStream.error, but if something throws synchronously between declaration and the firstawait(e.g., the file reads at Lines 66–73 already happened, but in future edits this would matter), the rejection would be unhandled. Same pattern already applies toprocessEvents()at Line 542.A small
.catch(err => console.error(...))on the IIFE invocation makes the unhandled-rejection surface explicit and prevents Node from logging duplicate stack traces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/actions.tsx` around lines 102 - 202, The inner async function processResolutionSearch shadows the outer groupeId and is invoked fire-and-forget without tethering errors; rename the inner variable (e.g., localGroupId or resolutionGroupId) wherever used inside processResolutionSearch to avoid confusion with the outer groupeId, and change the invocation of processResolutionSearch() to attach a rejection handler (e.g., processResolutionSearch().catch(err => console.error(...))) or prefix with void and add .catch to ensure any asynchronous rejection is logged; apply the same catch-on-invoke pattern used for processEvents() to prevent unhandled promise rejections.components/followup-panel.tsx (1)
19-44: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSubmission errors leave the user with no UI feedback.
The new
try/catchcorrectly prevents adding a malformedresponseMessageon failure and now consistently routes through the server action's centralized error handling. However, on failure:
setInput('')is skipped, which is good (user can retry), but the user still sees no error indication — the form simply does nothing.- There is no in-flight disabled state on the submit button while
await submit(formData)is pending, so the user can fire multiple submissions.Consider surfacing the error (toast / inline message) and disabling the button during submission. Both are cheap and materially improve the UX of the followup flow that this PR is targeting.
♻️ Sketch
-export function FollowupPanel() { - const [input, setInput] = useState('') +export function FollowupPanel() { + const [input, setInput] = useState('') + const [isSubmitting, setIsSubmitting] = useState(false) ... - try { + setIsSubmitting(true) + try { const responseMessage = await submit(formData) setMessages(currentMessages => [ ...currentMessages, userMessage, responseMessage ]) setInput('') } catch (error) { console.error('Error submitting followup:', error) + // surface to user, e.g. via toast + } finally { + setIsSubmitting(false) } } ... - disabled={input.length === 0} + disabled={input.length === 0 || isSubmitting}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/followup-panel.tsx` around lines 19 - 44, handleSubmit currently doesn't show UI feedback on failure and allows duplicate submissions; add a local loading state (e.g. isSubmitting) and an error state (e.g. submitError), check/set isSubmitting at the start of handleSubmit to early-return if already submitting, set isSubmitting=true before calling submit(formData), and in the try branch only clear input and push responseMessage to setMessages on success; in catch set submitError with the caught error (or a user-friendly message) so the component can render an inline error or trigger a toast, and in finally set isSubmitting=false; also update the submit button to be disabled when isSubmitting and show a loading indicator tied to isSubmitting. Ensure references to handleSubmit, submit, setMessages, setInput, mapData.drawnFeatures are used to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/actions.tsx`:
- Around line 439-474: The loop continuation condition currently ignores
errorOccurred when useSpecificAPI is true, causing infinite retries; update the
loop in actions.tsx so it also breaks on hasError and add a bounded retry
counter: introduce maxRetries (e.g., 3) and retryCount, increment retryCount
each iteration, and change the while condition to require answer.length === 0 &&
!errorOccurred && retryCount < maxRetries (or include !errorOccurred regardless
of useSpecificAPI), and inside the loop when researcher returns hasError ===
true set errorOccurred and break or let the condition exit; reference the
variables and functions answer, errorOccurred, useSpecificAPI, researcher, and
retryCount/maxRetries when applying the fix.
- Around line 500-539: The processEvents flow fails to finalize aiState when
errors occur or when errorOccurred is true; update processEvents to ensure
aiState.done(...) is always called in the error path and after the loop exit: in
the catch block and immediately after detecting errorOccurred, call
aiState.done(...) with a minimal terminal state (e.g., merge current
aiState.get().messages with a committed assistant error/failure message and set
any terminal flags) so the pending user message is committed and onSetAIState
runs; reference aiState.done, aiState.get, errorOccurred, and the processEvents
try/catch/finally to locate where to add these finalizing calls.
- Around line 413-430: The inquire branch prematurely finalizes streams: remove
the explicit uiStream.done() and isGenerating.done() calls inside the if
(action.object.next === 'inquire') block (leaving isCollapsed.done(false) and
the aiState update) so the finally block's cleanup handles finalization;
specifically, update the code around the inquire(...) call and return to not
call uiStream.done() or isGenerating.done() there and rely on the existing
finally that calls isGenerating.done(false) and uiStream.done().
In `@lib/agents/query-suggestor.tsx`:
- Around line 67-78: The throttle initialization prevents the first partial
chunk from being emitted because lastUpdateTime is set to Date.now(); change the
initialization of lastUpdateTime (or logic) so the first chunk always emits —
e.g., set lastUpdateTime = 0 or add a first-chunk guard before the for-await
loop that calls objectStream.update(obj) unconditionally; update references in
the streaming loop that check now - lastUpdateTime > UPDATE_THROTTLE and ensure
lastUpdateTime is updated after that first emit so subsequent throttling still
applies (affecting variables lastUpdateTime, UPDATE_THROTTLE, the for-await-of
over result.partialObjectStream, and objectStream.update).
---
Outside diff comments:
In `@app/actions.tsx`:
- Around line 399-403: Move the getSystemPrompt call into the processEvents
async IIFE so the UI stream can return immediately; currently
getSystemPrompt(userId) is awaited before processEvents and delays sending
uiStream.value to the client—update code to declare const userId = 'anonymous'
(or derive real userId) but avoid awaiting getSystemPrompt outside
processEvents, instead await it within processEvents (or remove the call if
per-user prompt isn't needed) and then use the fetched currentSystemPrompt
inside processEvents when composing messages; ensure references to
getSystemPrompt, processEvents, userId, and uiStream.value are updated
accordingly.
- Around line 102-202: The inner async function processResolutionSearch shadows
the outer groupeId and is invoked fire-and-forget without tethering errors;
rename the inner variable (e.g., localGroupId or resolutionGroupId) wherever
used inside processResolutionSearch to avoid confusion with the outer groupeId,
and change the invocation of processResolutionSearch() to attach a rejection
handler (e.g., processResolutionSearch().catch(err => console.error(...))) or
prefix with void and add .catch to ensure any asynchronous rejection is logged;
apply the same catch-on-invoke pattern used for processEvents() to prevent
unhandled promise rejections.
In `@components/followup-panel.tsx`:
- Around line 19-44: handleSubmit currently doesn't show UI feedback on failure
and allows duplicate submissions; add a local loading state (e.g. isSubmitting)
and an error state (e.g. submitError), check/set isSubmitting at the start of
handleSubmit to early-return if already submitting, set isSubmitting=true before
calling submit(formData), and in the try branch only clear input and push
responseMessage to setMessages on success; in catch set submitError with the
caught error (or a user-friendly message) so the component can render an inline
error or trigger a toast, and in finally set isSubmitting=false; also update the
submit button to be disabled when isSubmitting and show a loading indicator tied
to isSubmitting. Ensure references to handleSubmit, submit, setMessages,
setInput, mapData.drawnFeatures are used to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87ad783b-83a9-40f3-aaf9-f9b05ff954b0
📒 Files selected for processing (3)
app/actions.tsxcomponents/followup-panel.tsxlib/agents/query-suggestor.tsx
📜 Review details
🔇 Additional comments (1)
components/followup-panel.tsx (1)
22-22: 💤 Low valueNo changes required—FollowupPanel routing is correct.
The hardcoded
actionremoval is consistent. FollowupPanel now only appendsinputanddrawnFeatures, correctly routing follow-ups through the regular text path. No other call site depends on FollowupPanel triggering the resolution-search branch—header-search-button and resolution-carousel handle their ownaction='resolution_search'submissions separately. Follow-ups after aresolution_search_resultmessage route correctly through the regular flow with prior results properly filtered from the AI context (line 310–315).
| if (action.object.next === 'inquire') { | ||
| const inquiry = await inquire(uiStream, messages) | ||
| uiStream.done() | ||
| isGenerating.done() | ||
| isCollapsed.done(false) | ||
| aiState.done({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: nanoid(), | ||
| role: 'assistant', | ||
| content: `inquiry: ${inquiry?.question}` | ||
| } | ||
| ] | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how the AI SDK ai/rsc streamables behave on double done() — locate the implementation in node_modules-style paths if vendored, or look for prior usage in the repo.
rg -nP --type=ts -C2 '\b(uiStream|isGenerating|isCollapsed)\.done\b'Repository: QueueLab/QCX
Length of output: 1818
🏁 Script executed:
sed -n '380,545p' app/actions.tsx | cat -nRepository: QueueLab/QCX
Length of output: 5403
🌐 Web query:
ai/rsc createStreamableUI done() multiple calls double behavior
💡 Result:
Calling done() multiple times on a createStreamableUI instance from Vercel AI SDK (ai/rsc) will cause doubled or unexpected behavior. The official documentation states that done() "Marks the UI node as finalized and closes the stream. Once called, the UI node cannot be updated or appended anymore."[1][2][3][4] This implies the stream enters a closed state after the first call, making subsequent calls or operations invalid. No sources explicitly confirm if done() is idempotent (safe to call multiple times without side effects), but related GitHub issues show errors from post-done() operations, such as ".update: UI stream is already closed" in parallel tool calls[5], and multiple mounts/rerenders on update/done[6]. Issue #2183 reports that after done(), returning ui.value causes remounting and layout shifts, interpreted as "double behavior" (UI appears twice or flickers)[7]. This double behavior often stems from: - React Strict Mode double-invoking server actions/effects[8]. - Calling done() then inadvertently returning ui.value again[7]. - Nested/multiple streamables where child .done() calls affect parents[9]. - Not using async IIFEs properly, leading to race conditions[1]. To avoid: Call done() exactly once per instance, typically in a finally block or after all updates/appends. Use guards like if (!stream.done) if needed (though not officially documented). Always return ui.value immediately after creation, letting async blocks handle updates/done[10][11]. Example safe usage: const ui = createStreamableUI(<div>Loading...</div>); (async () => { // updates/appends ui.done(<div>Final UI</div>); })(); return ui.value; // Return once, before done() [1][2][10][11]
Citations:
- 1: https://ai-play.vercel.app/docs/reference/ai-sdk-rsc/create-streamable-ui
- 2: https://v4.ai-sdk.dev/docs/reference/ai-sdk-rsc/create-streamable-ui
- 3: https://ai-sdk.dev/docs/reference/ai-sdk-rsc/create-streamable-ui
- 4: https://sdk.vercel.ai/docs/reference/ai-sdk-rsc/create-streamable-ui
- 5: Fix multiple parallel tool call support in streamUI vercel/ai#1894
- 6: ai/rsc components streamed through createStreamableUI are mounted multiple times when updating the ui node (update/done) vercel/ai#1257
- 7: Bug in
streamUIandcreateStreamableUIvercel/ai#2183 - 8: https://www.reddit.com/r/reactjs/comments/1f1hdom/how_do_you_deal_with_strict_mode_double_useeffect/
- 9: Multiple Streamable UIs Disappear in Production Builds When Calling .done() vercel/ai#1558
- 10: https://ai-sdk.dev/docs/advanced/multiple-streamables
- 11: https://ai-sdk.dev/v4/docs/ai-sdk-rsc/streaming-values
Remove premature stream finalization in the inquire branch to prevent double done() calls.
The 'inquire' branch (lines 415–416) calls uiStream.done() and isGenerating.done(), then returns. However, control still reaches the finally block (lines 537–538), which calls these methods again. According to the Vercel AI SDK documentation, done() finalizes and closes the stream, making it impossible to update or append afterwards. Calling done() a second time on an already-closed stream will fail.
Remove the done() calls from the inquire branch and let the finally block handle finalization:
Proposed fix
if (action.object.next === 'inquire') {
const inquiry = await inquire(uiStream, messages)
- uiStream.done()
- isGenerating.done()
isCollapsed.done(false)
aiState.done({
...aiState.get(),
messages: [
...aiState.get().messages,
{
id: nanoid(),
role: 'assistant',
content: `inquiry: ${inquiry?.question}`
}
]
})
return
}The finally block already handles isGenerating.done(false) and uiStream.done(), which will execute for the inquire path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/actions.tsx` around lines 413 - 430, The inquire branch prematurely
finalizes streams: remove the explicit uiStream.done() and isGenerating.done()
calls inside the if (action.object.next === 'inquire') block (leaving
isCollapsed.done(false) and the aiState update) so the finally block's cleanup
handles finalization; specifically, update the code around the inquire(...) call
and return to not call uiStream.done() or isGenerating.done() there and rely on
the existing finally that calls isGenerating.done(false) and uiStream.done().
| while ( | ||
| useSpecificAPI | ||
| ? answer.length === 0 | ||
| : answer.length === 0 && !errorOccurred | ||
| ) { | ||
| const { fullResponse, hasError, toolResponses } = await researcher( | ||
| currentSystemPrompt, | ||
| uiStream, | ||
| streamText, | ||
| messages, | ||
| mapProvider, | ||
| useSpecificAPI, | ||
| drawnFeatures | ||
| ) | ||
| answer = fullResponse | ||
| toolOutputs = toolResponses | ||
| errorOccurred = hasError | ||
|
|
||
| if (toolOutputs.length > 0) { | ||
| toolOutputs.map(output => { | ||
| aiState.update({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: groupeId, | ||
| role: 'tool', | ||
| content: JSON.stringify(output.result), | ||
| name: output.toolName, | ||
| type: 'tool' | ||
| } | ||
| ] | ||
| }) | ||
| }) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Loop continuation logic ignores errorOccurred when useSpecificAPI is true.
The condition useSpecificAPI ? answer.length === 0 : answer.length === 0 && !errorOccurred means that under the useSpecificAPI branch, an erroring researcher call that returns fullResponse === '' will loop indefinitely (or until researcher happens to return non-empty text). There is no max-iteration cap and no backoff. Combined with the new try/catch swallowing errors at the outer level, a persistently failing researcher will keep retrying inside the same request. Recommend adding an explicit retry limit or also breaking on hasError here.
♻️ Proposed bound
+ let attempts = 0
+ const MAX_ATTEMPTS = 3
while (
useSpecificAPI
- ? answer.length === 0
+ ? answer.length === 0 && attempts < MAX_ATTEMPTS
: answer.length === 0 && !errorOccurred
) {
+ attempts++
const { fullResponse, hasError, toolResponses } = await researcher(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while ( | |
| useSpecificAPI | |
| ? answer.length === 0 | |
| : answer.length === 0 && !errorOccurred | |
| ) { | |
| const { fullResponse, hasError, toolResponses } = await researcher( | |
| currentSystemPrompt, | |
| uiStream, | |
| streamText, | |
| messages, | |
| mapProvider, | |
| useSpecificAPI, | |
| drawnFeatures | |
| ) | |
| answer = fullResponse | |
| toolOutputs = toolResponses | |
| errorOccurred = hasError | |
| if (toolOutputs.length > 0) { | |
| toolOutputs.map(output => { | |
| aiState.update({ | |
| ...aiState.get(), | |
| messages: [ | |
| ...aiState.get().messages, | |
| { | |
| id: groupeId, | |
| role: 'tool', | |
| content: JSON.stringify(output.result), | |
| name: output.toolName, | |
| type: 'tool' | |
| } | |
| ] | |
| }) | |
| }) | |
| }) | |
| } | |
| } | |
| let attempts = 0 | |
| const MAX_ATTEMPTS = 3 | |
| while ( | |
| useSpecificAPI | |
| ? answer.length === 0 && attempts < MAX_ATTEMPTS | |
| : answer.length === 0 && !errorOccurred | |
| ) { | |
| attempts++ | |
| const { fullResponse, hasError, toolResponses } = await researcher( | |
| currentSystemPrompt, | |
| uiStream, | |
| streamText, | |
| messages, | |
| mapProvider, | |
| useSpecificAPI, | |
| drawnFeatures | |
| ) | |
| answer = fullResponse | |
| toolOutputs = toolResponses | |
| errorOccurred = hasError | |
| if (toolOutputs.length > 0) { | |
| toolOutputs.map(output => { | |
| aiState.update({ | |
| ...aiState.get(), | |
| messages: [ | |
| ...aiState.get().messages, | |
| { | |
| id: groupeId, | |
| role: 'tool', | |
| content: JSON.stringify(output.result), | |
| name: output.toolName, | |
| type: 'tool' | |
| } | |
| ] | |
| }) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/actions.tsx` around lines 439 - 474, The loop continuation condition
currently ignores errorOccurred when useSpecificAPI is true, causing infinite
retries; update the loop in actions.tsx so it also breaks on hasError and add a
bounded retry counter: introduce maxRetries (e.g., 3) and retryCount, increment
retryCount each iteration, and change the while condition to require
answer.length === 0 && !errorOccurred && retryCount < maxRetries (or include
!errorOccurred regardless of useSpecificAPI), and inside the loop when
researcher returns hasError === true set errorOccurred and break or let the
condition exit; reference the variables and functions answer, errorOccurred,
useSpecificAPI, researcher, and retryCount/maxRetries when applying the fix.
| if (!errorOccurred) { | ||
| const relatedQueries = await querySuggestor(uiStream, messages) | ||
| uiStream.append( | ||
| <Section title="Follow-up"> | ||
| <FollowupPanel /> | ||
| </Section> | ||
| ) | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 500)) | ||
|
|
||
| aiState.done({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: answer, | ||
| type: 'response' | ||
| }, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: JSON.stringify(relatedQueries), | ||
| type: 'related' | ||
| }, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: 'followup', | ||
| type: 'followup' | ||
| } | ||
| ] | ||
| }) | ||
| } | ||
|
|
||
| isGenerating.done(false) | ||
| uiStream.done() | ||
| aiState.done({ | ||
| ...aiState.get(), | ||
| messages: [ | ||
| ...aiState.get().messages, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: answer, | ||
| type: 'response' | ||
| }, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: JSON.stringify(relatedQueries), | ||
| type: 'related' | ||
| }, | ||
| { | ||
| id: groupeId, | ||
| role: 'assistant', | ||
| content: 'followup', | ||
| type: 'followup' | ||
| } | ||
| ] | ||
| }) | ||
| } | ||
| } catch (error) { | ||
| console.error('Error in processEvents:', error) | ||
| } finally { | ||
| isGenerating.done(false) | ||
| uiStream.done() | ||
| } |
There was a problem hiding this comment.
Major: aiState is never finalized on the error path, leaving client AI state uncommitted.
aiState.done(...) is only called inside if (!errorOccurred) (Lines 509–532) and on the inquire branch (Line 418). If researcher/writer/querySuggestor throws and execution falls into catch (Line 534) — or if errorOccurred is true after the loop — the function returns from processEvents without ever calling aiState.done(). The user message added at Lines 381–392 then remains as an in-flight aiState.update(...) that is never committed, so onSetAIState will not run, the chat won't be persisted, and any subsequent aiState.get() from the client may not reflect the final state.
isGenerating/uiStream are now correctly closed in finally, but aiState needs the same treatment. Consider committing a minimal terminal state in catch (and after the errorOccurred loop exit) so the client's mutable state is consistent.
🛡️ Suggested direction
- } catch (error) {
- console.error('Error in processEvents:', error)
- } finally {
+ } catch (error) {
+ console.error('Error in processEvents:', error)
+ try {
+ aiState.done({ ...aiState.get() })
+ } catch (e) {
+ // aiState already finalized; ignore
+ }
+ } finally {
isGenerating.done(false)
uiStream.done()
}You may also want to guard the errorOccurred === true exit from the loop so the user gets some response surface (e.g., an assistant response message indicating failure) rather than a silently abandoned turn.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!errorOccurred) { | |
| const relatedQueries = await querySuggestor(uiStream, messages) | |
| uiStream.append( | |
| <Section title="Follow-up"> | |
| <FollowupPanel /> | |
| </Section> | |
| ) | |
| await new Promise(resolve => setTimeout(resolve, 500)) | |
| aiState.done({ | |
| ...aiState.get(), | |
| messages: [ | |
| ...aiState.get().messages, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: answer, | |
| type: 'response' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: JSON.stringify(relatedQueries), | |
| type: 'related' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: 'followup', | |
| type: 'followup' | |
| } | |
| ] | |
| }) | |
| } | |
| isGenerating.done(false) | |
| uiStream.done() | |
| aiState.done({ | |
| ...aiState.get(), | |
| messages: [ | |
| ...aiState.get().messages, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: answer, | |
| type: 'response' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: JSON.stringify(relatedQueries), | |
| type: 'related' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: 'followup', | |
| type: 'followup' | |
| } | |
| ] | |
| }) | |
| } | |
| } catch (error) { | |
| console.error('Error in processEvents:', error) | |
| } finally { | |
| isGenerating.done(false) | |
| uiStream.done() | |
| } | |
| if (!errorOccurred) { | |
| const relatedQueries = await querySuggestor(uiStream, messages) | |
| uiStream.append( | |
| <Section title="Follow-up"> | |
| <FollowupPanel /> | |
| </Section> | |
| ) | |
| aiState.done({ | |
| ...aiState.get(), | |
| messages: [ | |
| ...aiState.get().messages, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: answer, | |
| type: 'response' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: JSON.stringify(relatedQueries), | |
| type: 'related' | |
| }, | |
| { | |
| id: groupeId, | |
| role: 'assistant', | |
| content: 'followup', | |
| type: 'followup' | |
| } | |
| ] | |
| }) | |
| } | |
| } catch (error) { | |
| console.error('Error in processEvents:', error) | |
| try { | |
| aiState.done({ ...aiState.get() }) | |
| } catch (e) { | |
| // aiState already finalized; ignore | |
| } | |
| } finally { | |
| isGenerating.done(false) | |
| uiStream.done() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/actions.tsx` around lines 500 - 539, The processEvents flow fails to
finalize aiState when errors occur or when errorOccurred is true; update
processEvents to ensure aiState.done(...) is always called in the error path and
after the loop exit: in the catch block and immediately after detecting
errorOccurred, call aiState.done(...) with a minimal terminal state (e.g., merge
current aiState.get().messages with a committed assistant error/failure message
and set any terminal flags) so the pending user message is committed and
onSetAIState runs; reference aiState.done, aiState.get, errorOccurred, and the
processEvents try/catch/finally to locate where to add these finalizing calls.
| // OPTIMIZATION: Stream updates but batch them to reduce re-render frequency | ||
| let lastUpdateTime = Date.now(); | ||
| const UPDATE_THROTTLE = 100; // Reduced from 200ms to 100ms for better responsiveness | ||
|
|
||
| for await (const obj of result.partialObjectStream) { | ||
| if (obj && typeof obj === 'object' && 'items' in obj) { | ||
| const now = Date.now(); | ||
| // Only update UI if enough time has passed since last update | ||
| if (now - lastUpdateTime > UPDATE_THROTTLE) { | ||
| objectStream.update(obj as PartialRelated) | ||
| lastUpdateTime = now; | ||
| for await (const obj of result.partialObjectStream) { | ||
| if (obj && typeof obj === 'object' && 'items' in obj) { | ||
| const now = Date.now(); | ||
| // Only update UI if enough time has passed since last update | ||
| if (now - lastUpdateTime > UPDATE_THROTTLE) { | ||
| objectStream.update(obj as PartialRelated) | ||
| lastUpdateTime = now; | ||
| } |
There was a problem hiding this comment.
Throttle initialization may suppress the first UI update.
lastUpdateTime is set to Date.now() immediately before the loop, so the very first partial object — which often contains the most useful initial content — is unlikely to satisfy now - lastUpdateTime > UPDATE_THROTTLE and gets skipped from the streaming UI. The finally block masks this for the terminal value, but interactive responsiveness is reduced. Initializing lastUpdateTime = 0 (or always emitting the first chunk) is a one-line fix.
♻️ Proposed fix
- let lastUpdateTime = Date.now();
+ let lastUpdateTime = 0;
const UPDATE_THROTTLE = 100; // Reduced from 200ms to 100ms for better responsiveness📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OPTIMIZATION: Stream updates but batch them to reduce re-render frequency | |
| let lastUpdateTime = Date.now(); | |
| const UPDATE_THROTTLE = 100; // Reduced from 200ms to 100ms for better responsiveness | |
| for await (const obj of result.partialObjectStream) { | |
| if (obj && typeof obj === 'object' && 'items' in obj) { | |
| const now = Date.now(); | |
| // Only update UI if enough time has passed since last update | |
| if (now - lastUpdateTime > UPDATE_THROTTLE) { | |
| objectStream.update(obj as PartialRelated) | |
| lastUpdateTime = now; | |
| for await (const obj of result.partialObjectStream) { | |
| if (obj && typeof obj === 'object' && 'items' in obj) { | |
| const now = Date.now(); | |
| // Only update UI if enough time has passed since last update | |
| if (now - lastUpdateTime > UPDATE_THROTTLE) { | |
| objectStream.update(obj as PartialRelated) | |
| lastUpdateTime = now; | |
| } | |
| // OPTIMIZATION: Stream updates but batch them to reduce re-render frequency | |
| let lastUpdateTime = 0; | |
| const UPDATE_THROTTLE = 100; // Reduced from 200ms to 100ms for better responsiveness | |
| for await (const obj of result.partialObjectStream) { | |
| if (obj && typeof obj === 'object' && 'items' in obj) { | |
| const now = Date.now(); | |
| // Only update UI if enough time has passed since last update | |
| if (now - lastUpdateTime > UPDATE_THROTTLE) { | |
| objectStream.update(obj as PartialRelated) | |
| lastUpdateTime = now; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/agents/query-suggestor.tsx` around lines 67 - 78, The throttle
initialization prevents the first partial chunk from being emitted because
lastUpdateTime is set to Date.now(); change the initialization of lastUpdateTime
(or logic) so the first chunk always emits — e.g., set lastUpdateTime = 0 or add
a first-chunk guard before the for-await loop that calls
objectStream.update(obj) unconditionally; update references in the streaming
loop that check now - lastUpdateTime > UPDATE_THROTTLE and ensure lastUpdateTime
is updated after that first emit so subsequent throttling still applies
(affecting variables lastUpdateTime, UPDATE_THROTTLE, the for-await-of over
result.partialObjectStream, and objectStream.update).
|
@coderabbitai the resolution search has no output now |
|
|
1 similar comment
|
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2056 🏁 Script executed: Length of output: 39396 🏁 Script executed: Length of output: 7974 🏁 Script executed: Length of output: 8586 🏁 Script executed: Length of output: 23061 🏁 Script executed: Length of output: 13874
🔍 Root CauseThe PR removed However, even before this PR, that trigger was already broken: if (!file) {
throw new Error('No file provided for resolution search.');
}So the old FollowupPanel → resolution search path always threw an error. What's actually needed is a dedicated map capture button that:
All the needed context is already available:
🛠 Proposed Fix
🏁 Script executed: Length of output: 1588
🔍 Root CauseThe PR removed this single line from - formData.append("action", "resolution_search")That was the only line in the entire codebase that ever routed to the resolution search path in Importantly, even before this PR, that trigger was already broken: if (!file) {
throw new Error('No file provided for resolution search.');
}What's actually needed is a dedicated "Analyze Map" button that properly captures the current map view. All the pieces are already in place:
🛠 Proposed Fix
🏁 Script executed: Length of output: 1588
1. New file: 'use client'
import { useState } from 'react'
import { useMap } from './map/map-context'
import { useMapData } from './map/map-data-context'
import { useMapLoading } from './map-loading-context'
import { useActions, useUIState } from 'ai/rsc'
import type { AI } from '@/app/actions'
import { Button } from './ui/button'
import { ScanSearch } from 'lucide-react'
import { nanoid } from '@/lib/utils'
import { UserMessage } from './user-message'
export function MapAnalysisButton({ size = 20 }: { size?: number }) {
const { map } = useMap()
const { mapData } = useMapData()
const { isMapLoaded } = useMapLoading()
const { submit } = useActions()
const [, setMessages] = useUIState<typeof AI>()
const [isLoading, setIsLoading] = useState(false)
if (!isMapLoaded || !map) return null
const handleAnalyze = () => {
map.getCanvas().toBlob(async blob => {
if (!blob) return
setIsLoading(true)
try {
const formData = new FormData()
formData.append('action', 'resolution_search')
formData.append(
'file_mapbox',
new File([blob], 'mapbox-capture.png', { type: 'image/png' })
)
const lat = mapData.cameraState?.center?.lat
const lng = mapData.cameraState?.center?.lng
if (lat !== undefined) formData.append('latitude', String(lat))
if (lng !== undefined) formData.append('longitude', String(lng))
formData.append('timezone', mapData.currentTimezone || 'UTC')
formData.append(
'drawnFeatures',
JSON.stringify(mapData.drawnFeatures || [])
)
const userMessage = {
id: nanoid(),
isGenerating: false,
component: <UserMessage content="Analyze current map view" />
}
const responseMessage = await submit(formData)
setMessages(currentMessages => [
...currentMessages,
userMessage,
responseMessage as any
])
} catch (error) {
console.error('Error submitting map analysis:', error)
} finally {
setIsLoading(false)
}
}, 'image/png')
}
return (
<Button
type="button"
variant="ghost"
size="icon"
onClick={handleAnalyze}
disabled={isLoading}
title="Analyze current map view"
data-testid="map-analysis-button"
>
<ScanSearch size={size} className={isLoading ? 'animate-pulse' : ''} />
</Button>
)
}2. + import { MapAnalysisButton } from './map-analysis-button'
// In the desktop input area, after the existing Paperclip button:
{!isMobile && (
<Button
type="button"
variant={'ghost'}
size={'icon'}
className={cn(
'absolute top-1/2 transform -translate-y-1/2 left-3'
)}
onClick={handleAttachmentClick}
data-testid="desktop-attachment-button"
>
<Paperclip size={isMobile ? 18 : 20} />
</Button>
)}
+ {!isMobile && (
+ <div className="absolute top-1/2 transform -translate-y-1/2 left-10">
+ <MapAnalysisButton size={20} />
+ </div>
+ )}
3. + import { MapAnalysisButton } from './map-analysis-button'
<Button variant="ghost" size="icon" onClick={onAttachmentClick} data-testid="mobile-attachment-button">
<Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" />
</Button>
+ <MapAnalysisButton size={19} />
<Button variant="ghost" size="icon" data-testid="mobile-submit-button" onClick={onSubmitClick}>
<ArrowRight className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" />
</Button>Why This Works
The requested changes have been implemented and a pull request has been created: View PR |
I've fixed the issue where resolution search had no output. The problem was that the UI update call was being made outside the asynchronous handler, sometimes missing the initial state. I've moved it inside and added safety delays to ensure the stream flushes correctly. |
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
2 participants
PR created automatically by Jules for task 5274824833186492759 started by @ngoiyaeric
Summary by CodeRabbit
Bug Fixes
Refactor