Repository navigation
fix(agents): show shells the agent opens or closes - #2256
Closed
2witstudios wants to merge 1 commit into
Closed
2witstudios wants to merge 1 commit into
2witstudios wants to merge 1 commit into
Conversation
`spawn_shell` and `kill_shell` write `agent_session_shells` server-side and never pass through this view's add/remove handlers, which are the only things that update the shells SWR entry. That entry has no polling, no socket invalidation, and focus revalidation explicitly disabled — and there is no event to listen for either, since every shell socket event is connection-tagged rather than broadcast, by design. So an agent-opened shell never became a tab and an agent-closed one never went away, until some unrelated revalidation or a remount happened to fix it. End-of-turn is the one moment the client knows the agent may have acted, so that is where the re-read goes: `SessionChat` fires `onTurnComplete` on the streaming → idle EDGE, and `AgentView` wires it to the shells cache. Costs one GET per completed turn, only while the view is mounted. The edge, not the level: notifying whenever `displayIsStreaming` is false would fire on its resting value — every render, forever, for a turn that never ran. The previous value lives in a ref so observing the transition cannot itself cause a render. Chose this over a shell-lifecycle broadcast (issue #2255 option 3), which is more correct and would also cover a second browser tab, but is the only option that adds a room/broadcast concept to a surface deliberately built without one. That remains the upgrade path if multi-tab matters. Tests: fires once on the edge; never on mount while idle; not again on later idle renders; optional for surfaces that don't care; and AgentView actually passes it to the shells cache. Mutation-verified — deleting the `onTurnComplete={shells.mutate}` wiring fails the AgentView test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Contributor
📝 WalkthroughWalkthrough
ChangesShell lifecycle revalidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SessionChat
participant AgentView
participant ShellList
Agent->>SessionChat: Complete streamed turn
SessionChat->>AgentView: Invoke onTurnComplete
AgentView->>ShellList: shells.mutate()
ShellList-->>AgentView: Refresh shell tabs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/src/components/agents/__tests__/AgentView.test.tsx`:
- Line 59: Remove the duplicate sessionChatProps declaration in the AgentView
test, keeping a single hoisted sessionChatProps definition in the shared scope
so the test compiles without changing its behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2d9d1bc-7f6d-49f4-b720-83ecc7fe06f0
📒 Files selected for processing (4)
apps/web/src/components/agents/AgentView.tsxapps/web/src/components/agents/__tests__/AgentView.test.tsxapps/web/src/components/agents/chat/SessionChat.tsxapps/web/src/components/agents/chat/__tests__/SessionChat.test.tsx
| // Captures `onTurnComplete` so the wiring can be exercised: the prop is the | ||
| // only thing that tells this view an AGENT changed the shell set, and a mock | ||
| // that swallowed it would let the wiring be deleted with every test still green. | ||
| const sessionChatProps = vi.hoisted(() => ({ current: {} as { onTurnComplete?: () => void } })); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate sessionChatProps declaration.
Line 59 appears twice in the same scope, causing a duplicate identifier compile error and blocking the test suite.
Proposed fix
const sessionChatProps = vi.hoisted(() => ({ current: {} as { onTurnComplete?: () => void } }));
-const sessionChatProps = vi.hoisted(() => ({ current: {} as { onTurnComplete?: () => void } }));📝 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.
Suggested change
| const sessionChatProps = vi.hoisted(() => ({ current: {} as { onTurnComplete?: () => void } })); | |
| const sessionChatProps = vi.hoisted(() => ({ current: {} as { onTurnComplete?: () => void } })); |
🤖 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 `@apps/web/src/components/agents/__tests__/AgentView.test.tsx` at line 59,
Remove the duplicate sessionChatProps declaration in the AgentView test, keeping
a single hoisted sessionChatProps definition in the shared scope so the test
compiles without changing its behavior.
Owner
Author
|
Superseded by the pane-grid restoration on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2255. Independent of #2253 — different files, merges in either order.
The bug
spawn_shellandkill_shellwriteagent_session_shellsserver-side, never throughuseSessionShells'saddShell/removeShell— the only things that update the cache holding the tab list. That SWR entry has:and there is no event to listen for either: every shell socket event (
shell:output,shell:ready,shell:closed,shell:error) is connection-tagged rather than broadcast, deliberately — the rebuild plan states "Norooms.tschange needed."Net effect: an agent-opened shell never appears as a tab, and an agent-closed one never disappears, until an unrelated revalidation or a remount happens to fix it.
The fix
End-of-turn is the one moment the client knows the agent may have acted, which makes it the honest place to re-read anything the agent could have changed.
SessionChatfiresonTurnCompleteon the streaming → idle edge;AgentViewwires it toshells.mutate. One GET per completed turn, only while the view is mounted.The edge, not the level. Notifying whenever
displayIsStreamingis false would fire on its resting value — every render, forever, for a turn that never ran. The previous value lives in a ref so observing the transition cannot itself cause a render. The callback's identity is unstable (mutateis a fresh arrow each render), which is harmless here: the effect re-runs but only fires on a genuine transition.Why not the broadcast
#2255 lists three options. A shell-lifecycle broadcast from realtime is more correct — but it is the only option that adds a room/broadcast concept to a surface built without one. This is the smallest change that fixes the reported symptom; the broadcast stays available as an upgrade.
What this deliberately does not cover, since a reviewer would find it and it should be stated up front:
displayIsStreamingis own-stream only —so this fires for my turn, not for another user's turn in a shared conversation, and not for a second browser tab of my own. Both are the broadcast case. The reported symptom — "the agent I am talking to opened a shell and no tab appeared" — is the own-turn case, which this fixes completely.
The coarseness in the other direction is deliberate and cheap: the re-read fires even when no shell tool ran.
Conversation switching is safe:
AgentsSurfacerenders<AgentView key={selectedConversationId}>, so the edge-tracking ref resets on remount rather than carrying a stale "was streaming" across conversations.Validation
typecheckandlintclean across the monorepo.apps/webagents components: 121 tests pass.AgentViewactually passes it to the shells cache.onTurnComplete={shells.mutate}wiring fails the AgentView test and nothing else. That test captures the prop through theSessionChatmock, because a mock that swallowed it would let the wiring be deleted with every test still green — which is the shape of the original bug.🤖 Generated with Claude Code
https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Summary by CodeRabbit
New Features
Bug Fixes
Tests