fix(frontend): Keep the agent session streaming when you navigate away - #5862
fix(frontend): Keep the agent session streaming when you navigate away#5862ashrafchowdury wants to merge 8 commits into
Conversation
…c to maintain chat instances across navigation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a session-scoped chat registry. ChangesSession chat persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change keeps active agent sessions streaming across navigation while preserving shutdown when a session is closed, deleted, or archived; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant AgentChatSession
participant SessionChatRegistry
participant SessionState
participant Chat
AgentChatSession->>SessionChatRegistry: acquireSessionChat(sessionId, hooks)
SessionChatRegistry->>Chat: create or reuse session chat
Chat-->>AgentChatSession: provide shared chat instance
AgentChatSession->>SessionChatRegistry: releaseSessionChat(sessionId, stillOpen)
SessionState->>SessionChatRegistry: dropSessionChat(sessionId) when session closes
SessionChatRegistry->>Chat: stop and remove closed chat
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dda9c5a8-2ea1-43be-ba15-ba583abd5e42
📒 Files selected for processing (3)
web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tsweb/oss/src/components/AgentChatSlice/state/chatRegistry.test.tsweb/oss/src/components/AgentChatSlice/state/chatRegistry.ts
Railway Preview Environment
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/oss/src/components/AgentChatSlice/state/chatRegistry.ts (1)
47-65: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not publish a new registry entry during render.
useAgentChatSessioncallsacquireSessionChatduring render (the suppliedweb/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tssnippet, Lines 65-224). Lines 49-65 create and register aChatbefore any commit. If React abandons that first render, no effect cleanup runs. A later committed mount reuses the entry at Line 47, ignores its owninitialMessages, and can retain the abandoned chat indefinitely.Make registry ownership commit-aware. Keep a new entry provisional until a committed mount claims it, or discard uncommitted entries with a tokenized protocol. Add a regression test for an abandoned first acquisition with different
initialMessages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23cb2404-9619-4daa-89e6-4d3d055919e5
📒 Files selected for processing (4)
web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.tsweb/oss/src/components/AgentChatSlice/state/chatRegistry.test.tsweb/oss/src/components/AgentChatSlice/state/chatRegistry.tsweb/oss/src/components/AgentChatSlice/state/sessions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts
…sposal logic and auto-resume prevention
|
Thanks for the PR @ashrafchowdury. Please resolve the merge conflicts here |
Resolves three conflicts, keeping both sides' behaviour: - useAgentChatSession: this branch moved sendAutomaticallyWhen/onFinish/onError out of `useChat` onto the registry-owned chat's per-mount hooks; release grew those same callbacks in place. onFinish now carries both the preserved-run dot retirement and release's session-liveness + session-list invalidation (#5844). - chatRegistry: onError joins the SessionChatHooks contract (gated on isLive like onFinish) so release's "clear the pending gate marker on a failed stream" fix has somewhere to land; the F-033 console.warn stays unconditional. - AgentConversation: comment-only. Dropped release's now-stale "reset to idle on unmount" sentence (that reset is conditional on hasSessionChat here) and kept its hitlPending paragraph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Fixed @bekossy |
One conflict in useAgentChatSession.ts. Release added the bounded-request feature (#6042): buildRequestWithinDeadline retries while the invocation URL is still loading and rejects if the build hangs, so a stalled send surfaces as an error bubble instead of an eternal spinner. Release wired it into the old useMemo AgentChatTransport; this branch moved request-building into the registry-owned chat's hooks.prepareRequest, so the bounded build now lives there (reading entityId/captureTurnRequest directly, not via refs) and the helper owns the not-ready / timed-out errors. Dropped the stale AgentChatTransport import (the registry owns the transport here). Also normalized formatting on sessionRouteScope.ts (pre-commit prettier). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai please resolve merge conflict here |
|
✅ Merge conflicts resolved successfully! Resolved 1 conflict file(s). Commit: The resolved commit is ready for your repository's normal checks and review. 6 file operation(s)
View agent analysis |
Resolved conflicts in: - web/oss/src/components/AgentChatSlice/hooks/useAgentChatSession.ts (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
bekossy
left a comment
bekossy
left a comment
There was a problem hiding this comment.
Thanks for the work here. The registry direction makes sense for preserving a live stream across route changes, but I don’t think this is ready yet.
Two things still need addressing:
-
acquireSessionChat()still creates and stores a newChatin the module registry during render. Moving hook rebinding to an effect helps with stale callbacks, but it doesn’t cover the first-acquire case: if React abandons that first render, the registry can retain a chat seeded from an uncommitted render, and the next committed mount will reuse it with the wronginitialMessages. Can we make the first registry claim commit-aware too, and add a regression test for an abandoned first acquisition with differentinitialMessages? -
Remote archive reconciliation drops the chat and marks the session archived, but it doesn’t remove that id from the open-tab list or update the active-session atom. Since some child hooks read the raw active atom, the visible fallback tab may not be treated as active for pending runs, shortcuts, or record watches. Can we mirror the local archive behavior here: remove the archived id from
openIdsByAppAtom, repointactiveByAppAtomto the next available open tab, and add coverage for that case?
Once those are fixed, I think the approach should be in good shape.
2 participants
Context
Start an agent run in the playground, switch to another page while it is still producing, and come back: the answer had stopped streaming. The turn was still alive on the runner, but the browser was no longer following it. The tab fell back to the 15s durable-log catch-up, so the rest of the answer arrived in jumps instead of live.
The cause was ownership.
useChatcreated itsChatinside the conversation component, so the SSE read lived and died with the mount, and the D9 teardown effect calledstop()on every unmount. A route change is an unmount, so it looked exactly like closing the tab. Fixes #5724.Changes
The
Chatinstance now lives in a small module-scoped registry keyed by session id (state/chatRegistry.ts), and the component borrows it instead of owning it.Before: unmount always aborted the stream.
After: unmount asks whether the session itself is gone. A route change leaves the tab open, so the chat stays and the stream keeps running; re-entering the route re-binds to the same instance mid-turn. Closing, deleting, or archiving the session removes it from the open-tab set first, so that path still stops the stream and drops the instance.
Because the chat now outlives the mount, its callbacks (
prepareRequest,sendAutomaticallyWhen,onFinish) are rebound on every acquire. That is what keeps a long-lived chat from running stale closures, and it is why a run still follows a revision switch or a self-commit rather than sticking to the revision the session first mounted on.One subtlety is load-bearing and worth knowing while reading the diff: the registry must never hand
useChata fresh instance under a session id it already rendered.useChatswaps its internal ref on identity change but keys its message subscription on the chat id, which does not change, so it would keep listening to the dropped instance and the transcript would freeze. Keeping the entry alive for as long as the tab is open is what guarantees that. The trade-off is one idleChatper open tab until that tab is closed or the page reloads.Tests
chatRegistry.test.tscovers the acquire/release policy in 6 cases: re-bind on remount, preserve astreamingand asubmittedchat across a navigation, keep an idle chat while its tab is open, tear down when the session is no longer open, and forward a settled turn to the current mount'sonFinish.vitest run src/components/AgentChatSlice, 18 files, 132 tests).tsc --noEmitand eslint clean on@agenta/oss.state/sessions.tsremoves the id fromopenIdsByAppAtombefore React runs the cleanup. If a new teardown path is ever added, it has to follow the same order.What to QA