Repository navigation
fix(desktop): Chat toggle attaches to the chat a terminal id already runs (SUPER-2635) - #8625
superset-app[bot] wants to merge 1 commit into
Conversation
…runs (SUPER-2635) A chat-surface automation run starts a host chat session bound to a terminal id that has no PTY. When that id ended up in a terminal pane, the pane offered the CLI/Chat toggle from the chat's binding, and "Chat" replaced it with a chat pane whose sessionId was null. AcpChatPane then created a second session resuming the same agent conversation, so the user's message went to a new agent while the run's session kept running. The switch now reads the terminal's binding: a terminal that already runs a chat gets a chat pane on that chat session, and no terminal is killed, since there is none. The toggle is offered for a chat binding even before the agent has reported its own session id.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
 [Medium impact] Chat toggle logic for terminal panes. The PR appears safe to merge, with a non-blocking request to test the no-kill behavior. Findings
Fix with agent prompt### Issue 1
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/AgentTerminalPane/hooks/useAgentSurfaceSwitch/useAgentSurfaceSwitch.test.ts:7-14
These tests check only `chatPaneDataForTerminal`, so they would still pass if `switchSurface` killed a chat-bound terminal. Add a test that calls `switchSurface` with a chat binding and checks that it preserves the session without calling `killSession`. Also cover a binding without `agentSessionId`, since this PR now offers the toggle in that case.
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!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe Chat toggle now attaches to the chat already bound to a terminal ID instead of starting another chat.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User selects Chat] --> B{Existing chat binding?}
B -->|Yes| C[Keep existing chat session ID]
B -->|No| D[Stop terminal process]
D --> E[Create chat pane without a session ID]
C --> F[Attach to existing chat]
E --> G[Start chat and resume agent conversation]
Reviews (1) · Last reviewed commit: "fix(desktop): Chat toggle attaches to th..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("attaches to the chat already running on the terminal", () => { | ||
| expect( | ||
| chatPaneDataForTerminal({ | ||
| terminalId: "terminal-1", | ||
| agent, | ||
| binding: { chatSessionId: "chat-1" }, | ||
| }), | ||
| ).toEqual({ terminalId: "terminal-1", sessionId: "chat-1", agent }); |
There was a problem hiding this comment.
These tests check only chatPaneDataForTerminal, so they would still pass if switchSurface killed a chat-bound terminal. Add a test that calls switchSurface with a chat binding and checks that it preserves the session without calling killSession. Also cover a binding without agentSessionId, since this PR now offers the toggle in that case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/AgentTerminalPane/hooks/useAgentSurfaceSwitch/useAgentSurfaceSwitch.test.ts
Line: 7-14
Comment:
**No-kill behavior remains untested**
These tests check only `chatPaneDataForTerminal`, so they would still pass if `switchSurface` killed a chat-bound terminal. Add a test that calls `switchSurface` with a chat binding and checks that it preserves the session without calling `killSession`. Also cover a binding without `agentSessionId`, since this PR now offers the toggle in that case.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
Related to SUPER-2635, but does not fix what that issue reports (see "Verification"). It fixes a real duplicate-session path that comes from the SUPER-2825 bug.
What this fixes
With
acp-chaton, automation dispatch callsagents.runwithsurface: "chat". The host starts a chat session (launchChatAgent) and binds it to a fresh terminal id that has no PTY. The run row stores that terminal id.Several desktop paths treat that terminal id as a PTY and open a bare
terminalpane for it (#8626 fixes those paths). That pane looks up the id's agent binding, whichterminalAgents.listByWorkspacereturns for chats too. So it shows the CLI/Chat toggle. Clicking Chat ranswitchSurface("acp"), which:terminal.killSessionsucceeds for an unknown id, so the switch continues, andchat-v3pane withsessionId: null.AcpChatPanecreates a session wheneversessionIdis null (createSession({ terminalId, resume })), and the chat runtime does not deduplicate by terminal id. So the same agent conversation is resumed in a second chat session while the run's session keeps running.Change
useAgentSurfaceSwitch: when the terminal id already has a chat binding (binding.chatSessionId), the new chat pane carries that session id. Nothing is killed, because there is no PTY. Plain terminal agents keep the old behaviour (kill the PTY, then start a chat that resumes the agent session).useAgentSurface: offers the toggle for a chat binding even before the agent has reported its own session id.Verification
This path: reproduced on
main(0e0c87f), fixed on this branch. A hook-level repro renders the realuseAgentSurfaceSwitchagainst a real@superset/panesstore. Only the host is faked: terminal id T1 has a chat binding (chatSessionId: chat-session-1) and no PTY. It callsswitchSurface(ctx, "acp", agent)on a bare terminal pane for T1.main: ❌ the newchat-v3pane hassessionId: null, soAcpChatPanestarts a second session, andterminal.killSessionis called.sessionId: "chat-session-1", andkillSessionis not called.The repro is not committed (it has to
mock.module@superset/workspace-client).useAgentSurfaceSwitch.test.tsis the committed regression test.SUPER-2635 as reported: a different root cause, already fixed on
mainby #8242.surface: "chat"dispatch,continueChatAgent) was opened at 22:54 UTC that day and merged at 01:30 UTC on 10-07. So the path above cannot be what the reporter hit.continueTerminalAgent). Switching that agent to Chat kills its PTY and starts a chat on the same terminal id. A host-level repro on 66e170b (realTerminalAgentStoreandcontinuationTarget, with the chat binding recorded as the chat bridge does) shows the next run accept T1 as the target. The send then goes to a PTY that no longer exists, socontinueTerminalAgentreturns null andagents.runlaunches a new agent on every run. That matches the screenshot: 16 Claude agents in the "Email Triage" workspace. The send failing on a disposed PTY comes from reading the code, not from running it.maindelivers into the chat (chatContinuationTargetreturnschat-2), so this direction is fixed.main, not fixed by this PR: the reverse direction. A chat run switched to CLI gets its conversation resumed in a new terminal T2, but the next continue-run still names T1. Neither the chat nor the terminal continuation finds it, so a new agent launches. Shown by the same host repro onmain. Needs a separate change (for example, continuation following the resumed successor).Also run:
bun testoverv2-workspace(752 pass);bun run typecheckinapps/desktop; biome. This branch merges cleanly with #8626, and the repro passes on the merged tree.Not verified in the running desktop app. This cloud workspace has no signed-in desktop session, no
acp-chathost, and no Claude credentials.Tests
useAgentSurfaceSwitch.test.ts: a chat-bound terminal maps to its existing chat session. A terminal agent still getssessionId: null.Summary by cubic
Fixes the CLI/Chat toggle in a terminal pane creating a duplicate chat session for automation runs that already run a chat.
When a chat-surface automation run binds to a terminal id that has no PTY, switching that pane to Chat now opens the existing chat session instead of killing the (non-existent) terminal and starting a second session that resumes the same agent conversation. Plain terminal agents keep the old behavior: the PTY is killed and a new chat session is started.
Also makes the CLI/Chat toggle appear for chat bindings even before the agent has reported its own session id.
Written for commit e2e6c64. Summary will update on new commits.