Threads 18: Show parallel response lifecycle states - #247
Threads 18: Show parallel response lifecycle states#247FranciscoMoretti wants to merge 3 commits into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @FranciscoMoretti, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67a87b78b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| message: ParallelResponseStatusMessage | null | ||
| ): ChatStatus { | ||
| const activeStreamId = message?.metadata.activeStreamId; | ||
| if (!activeStreamId || status === "submitted" || status === "streaming") { |
There was a problem hiding this comment.
When a parallel request fails after its pending or real activeStreamId has been attached, the selected run reports error, but this branch converts that status back to submitted or streaming. The primary error path only shows a toast and does not clear or refresh the local placeholder, so the composer remains a stop button, submissions stay disabled, and the response continues to appear active until the page is reloaded. Return error unchanged, as is already done for the two active statuses.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces a 3-state parallel response lifecycle (
Confidence Score: 4/5Safe to merge with one fix: the pending-response guard in handleStop reads stream state from the throttled selector while the target message ID comes from the raw store, creating a narrow window where the check evaluates the wrong message. The core lifecycle logic is sound and the new helpers are well-tested. The one concrete defect is in handleStop: useLastMessageMetadata pulls from getThrottledMessages() while useLastMessageId uses raw state.messages, so between a new message landing and the throttle firing, isPendingResponseStream checks the previous message's activeStreamId rather than the newly-added pending one. A stop action during that window skips the guard and incorrectly sends a server stop request for a message that has no server-side stream yet. Files Needing Attention: apps/chat/components/multimodal-input.tsx — the handleStop callback's pending-response guard reads from a throttled metadata hook while the message ID comes from the raw store. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User submits message] --> B[setStatus submitted]
B --> C[runParallelThreadRequestSpecs]
C --> D{For each model slot}
D --> E[activeStreamId = pending:X]
E --> F[getParallelResponseLifecycle to queued]
F --> G[Card: spinner + Generating...]
G --> H{Server stream starts?}
H -- Yes --> I[activeStreamId = stream-X]
I --> J[getParallelResponseLifecycle to generating]
J --> K[Card: spinner + Generating...]
K --> L[Stream ends: activeStreamId = null]
L --> M[getParallelResponseLifecycle to complete]
M --> N[Card: Task completed or Selected]
H -- User clicks Stop --> O{isPendingResponseStream?}
O -- Yes pending --> P[Skip server stop and clearResponseActiveStream locally]
O -- No real stream --> Q[stopStreamMutation.mutate and clearResponseActiveStream locally]
P --> M
Q --> M
N --> R[shouldHideCompletionActions = false and Actions visible]
Reviews (31): Last reviewed commit: "fix(chat): settle stopped response state..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| message: ParallelResponseStatusMessage | null | ||
| ): ChatStatus { | ||
| const activeStreamId = message?.metadata.activeStreamId; | ||
| if (!activeStreamId || status === "submitted" || status === "streaming") { |
There was a problem hiding this comment.
"error" not in the short-circuit guard — error state is overridden by a lingering stream ID
When status === "error" (e.g., the primary request fails) and a parallel response's activeStreamId is still set, the early-return guard only catches "submitted" and "streaming", so the function falls through and returns "submitted" or "streaming" instead of "error". In multimodal-input.tsx, the send-button check treats both those values as "still busy", so the composer stays disabled with the misleading "Please wait for the model to finish" message even though the request actually errored and the user should be allowed to retry or submit a new message.
| export const useLastMessageMetadata = () => | ||
| useBaseChatStore((state) => state.getThrottledMessages().at(-1)?.metadata); |
There was a problem hiding this comment.
useLastMessageMetadata is the only metadata hook in this file that doesn't use shallow equality. Without it the selector returns a new object reference on every store tick, causing PureMultimodalInput to re-render even when metadata content is unchanged — opposite to the throttling pattern used by every other hook here.
| export const useLastMessageMetadata = () => | |
| useBaseChatStore((state) => state.getThrottledMessages().at(-1)?.metadata); | |
| export const useLastMessageMetadata = () => | |
| useBaseChatStore( | |
| (state) => state.getThrottledMessages().at(-1)?.metadata, | |
| shallow | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| it("keeps selected pending responses stoppable", () => { | ||
| assert.equal( | ||
| getResponseAwareStatus( | ||
| "ready", | ||
| createAssistantMessage("pending:assistant-1") | ||
| ), | ||
| "submitted" | ||
| ); | ||
| assert.equal( | ||
| getResponseAwareStatus("ready", createAssistantMessage("stream-1")), | ||
| "streaming" | ||
| ); | ||
| assert.equal( | ||
| getResponseAwareStatus("ready", createAssistantMessage(null)), | ||
| "ready" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
getResponseAwareStatus when status is "error"
The test suite exercises "ready" → "submitted" / "streaming" / "ready" transitions, but not what happens when the AI SDK sets status === "error" while an activeStreamId is still present. That is precisely the boundary where the short-circuit guard in getResponseAwareStatus matters most; adding a case like getResponseAwareStatus("error", createAssistantMessage("stream-1")) === "error" would pin the intended behaviour and prevent a silent regression.
67a87b7 to
8586679
Compare
b604d91 to
88e95e9
Compare
| useEffect(() => { | ||
| if (pendingParallelIndex === null) { | ||
| return; | ||
| } | ||
|
|
||
| const response = cardSlots.find( | ||
| (slot) => slot.parallelIndex === pendingParallelIndex | ||
| )?.message; | ||
| if (!response) { | ||
| return; | ||
| } | ||
|
|
||
| setPendingParallelIndex(null); | ||
| navigateToMessage(response.id); | ||
| }, [cardSlots, navigateToMessage, pendingParallelIndex]); |
There was a problem hiding this comment.
pendingParallelIndex never cleared on failed response
When a user clicks a queued card (no message yet), pendingParallelIndex is set to that slot's index. The useEffect only clears it when cardSlots.find(…)?.message is truthy — i.e., when an assistant message with that parallelIndex actually arrives. If the request fails before any message is created for that slot, cardSlots will always show slot.message === null for that index, the if (!response) return short-circuits on every tick, and pendingParallelIndex is never reset.
The visible result: selectedParallelIndex is stuck returning pendingParallelIndex (line 107-109 priority check), so the failed card stays permanently shown as "Selected" with a spinning loader and "Generating…" label. The only escape is clicking a different card whose slot.message is non-null, which calls setPendingParallelIndex(null) via the if (slot.message) branch. If all other slots also fail to produce messages, there is no way to clear the state.
88e95e9 to
bc22d66
Compare
bc22d66 to
40ba9b4
Compare
40b4619 to
a764034
Compare
a764034 to
5c0d228
Compare
| stopHelper?.(); | ||
| }, [chatId, lastMessageId, session?.user, stopHelper, stopStreamMutation]); | ||
| if (lastMessageId) { | ||
| setMessages( | ||
| clearResponseActiveStream(storeApi.getState().messages, lastMessageId) | ||
| ); | ||
| } |
There was a problem hiding this comment.
clearResponseActiveStream is called unconditionally even when the active response is still in pending state. clearResponseActiveStream sets activeStreamId to null on the last message in the store, which causes getParallelResponseLifecycle to return "complete" for that message. The parallel card then displays "Task completed" for an assistant message that has no content yet, and shouldHideCompletionActions in assistant-message.tsx lets copy/retry/feedback actions appear for the empty message. Guarding the call with !isPendingResponse preserves the "queued" lifecycle until the server either delivers content or the cancellation propagates.
| stopHelper?.(); | |
| }, [chatId, lastMessageId, session?.user, stopHelper, stopStreamMutation]); | |
| if (lastMessageId) { | |
| setMessages( | |
| clearResponseActiveStream(storeApi.getState().messages, lastMessageId) | |
| ); | |
| } | |
| stopHelper?.(); | |
| if (lastMessageId && !isPendingResponse) { | |
| setMessages( | |
| clearResponseActiveStream(storeApi.getState().messages, lastMessageId) | |
| ); | |
| } |
1 participant
Summary
submittedat the app submission boundary.Behavior
Parallel cards no longer flash
Task completed, show premature actions, or expose a send icon while work is pending.Verification
Screenshot
Review focus
The single response-lifecycle rule used by cards, actions, loading, and the composer.
Summary by cubic
Shows accurate lifecycle states for parallel responses (queued, generating, complete) in cards and loading UI. Disables inputs and actions until the active response settles; Stop is instant for the selected response and skips server calls for
pending:*streams.New Features
getParallelResponseLifecycle,getStatusLabel,getResponseAwareStatus,clearResponseActiveStream,isPendingResponseStream.status("submitted")at provisional chat start.Bug Fixes and Refactors
activeStreamId; skip server stop/reconnect forpending:*streams.useChatActionsand a redundant thread hook.Written for commit 72a3612. Summary will update on new commits.
Stack