refactor(ai): remove legacy GlobalChatContext message state + fix stream reload - #1308
Conversation
Replace unconditional refreshConversation() in GlobalChatContext.onStreamComplete with local synthesis from usePendingStreamsStore (matching the pattern already used by useAgentChannelMultiplayer and AiChatView). Own fresh streams skip entirely — GlobalAssistantView's sync effect keeps context messages current. Remote and bootstrapped streams synthesize the final message locally without triggering setIsInitialized(false) or a server round-trip, eliminating the visible reload/flash on every AI response in the sidebar and dashboard global mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTier GlobalChatContext into config/stream/conversation hooks, remove provider-owned messages, add numeric refreshSignal, consult usePendingStreamsStore in socket onStreamComplete(messageId) to synthesize or trigger refreshSignal for matching remote streams, and update GlobalAssistantView and SidebarChatTab to use refreshSignal and local message ownership. ChangesGlobal chat refactor and UI integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 6b01d27456
ℹ️ 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".
| setMessages((prev) => | ||
| prev.some((m) => m.id === messageId) | ||
| ? prev | ||
| : [...prev, synthesizeAssistantMessage(messageId, stream.parts)], | ||
| ); |
There was a problem hiding this comment.
This callback synthesizes the completed assistant message into GlobalChatContext state only, but the global UIs render from useChat state (GlobalAssistantView and SidebarChatTab use globalLocalMessages/messages, not context.messages). Because useChannelStreamSocket removes the pending stream immediately after this callback, non-origin tabs (and reload-mid-stream bootstrap cases) can lose the assistant reply at completion: the in-flight ghost disappears and no final message is added to the displayed useChat message list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the review! This comment was against the first intermediate commit (6b01d27) — the subsequent refactor commit (739e9b1) changed the approach entirely.
The final onStreamComplete does not synthesize into context state at all. Instead it just increments refreshSignal, which causes surfaces (GlobalAssistantView and SidebarChatTab) to call their own handlePullUpRefresh → fetch from the API → call useChat's setMessages directly. No context messages state involved.
The lookup-in-pending-store is safe: useChannelStreamSocket's fireComplete fires onStreamComplete(messageId) before removeStream(messageId) (which is in the finally block), so the stream is still in the store when we check it. For own fresh streams, handleStreamStart is filtered by the isOwnStream guard so they're never added to the store — those fall through to the 'own fresh stream' comment and do nothing (surface's useChat already has the message from the direct SSE connection).
Replace vestigial context messages/setMessages/refreshConversation with a refreshSignal counter. Surfaces (GlobalAssistantView, SidebarChatTab) now own their fetch-and-set cycles and watch the signal for remote events (reconnect, undo, cross-tab edits). Removes duplicate useGlobalChat() combined hook; all callers now use the tiered selective hooks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2witstudios
changed the title
|
Re: Codex P1 comment on commit 6b01d27 Addressed by commit 739e9b1. The synthesis-into-context-state approach from the first commit has been replaced entirely. In the new approach (
For own fresh streams, |
Add globalIsInitialized check before fetching messages on app resume, matching the guard already present in the refreshSignal effect. Prevents a fetch attempt against a stale conversationId while the global chat is still initializing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace useGlobalChat with tiered hooks, test refreshSignal instead of context messages state, wire mockStreams for onStreamComplete lookups. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eam reload (#1308) * fix(ai): stop reload after global chat stream ends Replace unconditional refreshConversation() in GlobalChatContext.onStreamComplete with local synthesis from usePendingStreamsStore (matching the pattern already used by useAgentChannelMultiplayer and AiChatView). Own fresh streams skip entirely — GlobalAssistantView's sync effect keeps context messages current. Remote and bootstrapped streams synthesize the final message locally without triggering setIsInitialized(false) or a server round-trip, eliminating the visible reload/flash on every AI response in the sidebar and dashboard global mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(ai): remove legacy GlobalChatContext message state Replace vestigial context messages/setMessages/refreshConversation with a refreshSignal counter. Surfaces (GlobalAssistantView, SidebarChatTab) now own their fetch-and-set cycles and watch the signal for remote events (reconnect, undo, cross-tab edits). Removes duplicate useGlobalChat() combined hook; all callers now use the tiered selective hooks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ai): guard handleAppResume against uninitialized global state Add globalIsInitialized check before fetching messages on app resume, matching the guard already present in the refreshSignal effect. Prevents a fetch attempt against a stale conversationId while the global chat is still initializing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(ai): update GlobalChatContext tests for new API Replace useGlobalChat with tiered hooks, test refreshSignal instead of context messages state, wire mockStreams for onStreamComplete lookups. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Root cause:
GlobalChatContext.onStreamCompleteunconditionally calledrefreshConversation()→loadConversation()→setIsInitialized(false)+ server re-fetch +setInitialMessages, producing a visible loading flash after every AI response.Fix: remove duplicate message state + signal-based refresh
messagesstate,setMessages, andrefreshConversationfromGlobalChatContext— surfaces (GlobalAssistantView,SidebarChatTab) own their message state viauseChatdirectlyrefreshSignal: numbercounter toGlobalChatConversationContext; all remote events (reconnect, undo, cross-tab edits, stream complete for remote/bootstrapped streams) increment itrefreshSignaland self-fetch when it changes — no context intermediary, no loading flashuseGlobalChat()combined hook; consumers now use the tiereduseGlobalChatConfig,useGlobalChatStream,useGlobalChatConversationhooksrefreshConversation()which updated context messages but notuseChat— nowrefreshSignalcauses surfaces to fetch and calluseChat.setMessagesdirectlyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit