From 2183d637a22519d64b0a4dd861b83b07bbfbcff9 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 11 Aug 2026 07:51:18 -0700 Subject: [PATCH] fix(ui): keep discussions closed until toggled (#122072) --- ...chat-session-discussion-toggle.e2e.test.ts | 14 +++++-- ui/src/pages/chat/chat-pane-header.ts | 31 +-------------- .../chat/chat-pane.session-discussion.test.ts | 39 +++++++------------ 3 files changed, 26 insertions(+), 58 deletions(-) diff --git a/ui/src/e2e/chat-session-discussion-toggle.e2e.test.ts b/ui/src/e2e/chat-session-discussion-toggle.e2e.test.ts index 8ebef506626d..d45e4b8b52d6 100644 --- a/ui/src/e2e/chat-session-discussion-toggle.e2e.test.ts +++ b/ui/src/e2e/chat-session-discussion-toggle.e2e.test.ts @@ -53,7 +53,7 @@ describeControlUiE2e("session discussion toggle", () => { await server?.close(); }); - it("opens and closes the sidebar from the same header action", async () => { + it("keeps an existing discussion closed until the header action opens it", async () => { const context = await browser.newContext({ ...(captureUiProof ? { recordVideo: { dir: proofDir, size: { height: 720, width: 1280 } } } @@ -73,7 +73,10 @@ describeControlUiE2e("session discussion toggle", () => { }, ], methodResponses: { - "session.discussion.info": { state: "available" }, + "session.discussion.info": { + openUrl: "https://discussion.example/session", + state: "open", + }, "session.discussion.open": { openUrl: "https://discussion.example/session", state: "open", @@ -88,6 +91,9 @@ describeControlUiE2e("session discussion toggle", () => { const showDiscussion = page.getByRole("button", { name: "Show discussion" }); await expect.poll(() => showDiscussion.isVisible()).toBe(true); await expect.poll(() => showDiscussion.getAttribute("aria-pressed")).toBe("false"); + if (captureUiProof) { + await page.screenshot({ path: path.join(proofDir, "discussion-initial-closed.png") }); + } await showDiscussion.click(); @@ -95,7 +101,7 @@ describeControlUiE2e("session discussion toggle", () => { const closeDiscussion = page.getByRole("button", { name: "Close Discussion" }); await expect.poll(() => hideDiscussion.getAttribute("aria-pressed")).toBe("true"); await expect.poll(() => closeDiscussion.isVisible()).toBe(true); - expect(await gateway.getRequests("session.discussion.open")).toHaveLength(1); + expect(await gateway.getRequests("session.discussion.open")).toHaveLength(0); if (captureUiProof) { await page.screenshot({ path: path.join(proofDir, "discussion-open.png") }); } @@ -104,7 +110,7 @@ describeControlUiE2e("session discussion toggle", () => { await expect.poll(() => closeDiscussion.isVisible()).toBe(false); await expect.poll(() => showDiscussion.getAttribute("aria-pressed")).toBe("false"); - expect(await gateway.getRequests("session.discussion.open")).toHaveLength(1); + expect(await gateway.getRequests("session.discussion.open")).toHaveLength(0); if (captureUiProof) { await page.screenshot({ path: path.join(proofDir, "discussion-closed.png") }); } diff --git a/ui/src/pages/chat/chat-pane-header.ts b/ui/src/pages/chat/chat-pane-header.ts index 8ec5705ebc32..513ce849c021 100644 --- a/ui/src/pages/chat/chat-pane-header.ts +++ b/ui/src/pages/chat/chat-pane-header.ts @@ -1,8 +1,5 @@ import { html, nothing } from "lit"; -import type { - SessionDiscussionInfo, - SessionDiscussionState, -} from "../../../../packages/gateway-protocol/src/index.js"; +import type { SessionDiscussionInfo } from "../../../../packages/gateway-protocol/src/index.js"; import type { GatewaySessionRow } from "../../api/types.ts"; import { isDesktopPanelAvailable } from "../../app/app-shell-chrome.ts"; import { resolveControlUiAuthCandidates } from "../../app/control-ui-auth.ts"; @@ -498,7 +495,6 @@ export abstract class ChatPaneHeader extends ChatPaneSessionMenu { return; } this.sessionDiscussionStates.set(sessionKey, info.state); - this.maybeAutoShowSessionDiscussion(sessionKey, info.state); this.requestUpdate(); } catch { // Leave unprobed: the action stays hidden and a later switch retries. @@ -516,31 +512,6 @@ export abstract class ChatPaneHeader extends ChatPaneSessionMenu { } } - // An "open" probe result means this session already has a bound discussion; - // surface it immediately instead of hiding live chat behind the toggle. - // Probe resolution is the only hook needed: willUpdate deletes the target - // key's cached state on every session switch (and reconnect clears all), so - // each activation resolves a fresh probe and reaches this. Within one - // activation the cache dedupes — closing the sidebar sticks, and an - // already-open discussion column is never duplicated. - protected maybeAutoShowSessionDiscussion( - sessionKey: string, - discussionState: SessionDiscussionState, - ) { - const state = this.state; - if ( - discussionState !== "open" || - !state || - state.sessionKey.trim() !== sessionKey || - state.sidebarLayout.columns.some((column) => - column.panels.some((panel) => panel.slot === "discussion"), - ) - ) { - return; - } - this.openSessionDiscussionSlot(); - } - protected buildSessionDiscussionPanel( state: NonNullable, sessionKey: string, diff --git a/ui/src/pages/chat/chat-pane.session-discussion.test.ts b/ui/src/pages/chat/chat-pane.session-discussion.test.ts index 1aed47070b46..edbea5f464af 100644 --- a/ui/src/pages/chat/chat-pane.session-discussion.test.ts +++ b/ui/src/pages/chat/chat-pane.session-discussion.test.ts @@ -47,21 +47,18 @@ function createDiscussionPane(params: { return { pane, state, updateSidebarLayout, request }; } -describe("chat pane session discussion auto-show", () => { - it("auto-shows the discussion slot when the probe reports an open discussion", async () => { - const { pane, state, updateSidebarLayout } = createDiscussionPane({ +describe("chat pane session discussion", () => { + it("does not auto-show an open discussion", async () => { + const { pane, updateSidebarLayout } = createDiscussionPane({ info: { state: "open", embedUrl: "https://clack.example/embed/c1" }, }); await pane.probeSessionDiscussion(SESSION_KEY); - expect(updateSidebarLayout).toHaveBeenCalledTimes(1); - expect( - state.sidebarLayout.columns.flatMap((column) => column.panels.map((panel) => panel.slot)), - ).toEqual(["discussion"]); + expect(updateSidebarLayout).not.toHaveBeenCalled(); }); - it("keeps the reported external URL with the promoted discussion panel", async () => { + it("keeps the reported external URL with the discussion panel", async () => { const openUrl = "https://clack.example/channels/c1"; const { pane, state } = createDiscussionPane({ info: { state: "open", embedUrl: "https://clack.example/embed/c1", openUrl }, @@ -148,12 +145,17 @@ describe("chat pane session discussion auto-show", () => { info: { state: "open", embedUrl: "https://clack.example/embed/c1" }, detailOpen: true, }); + const container = document.createElement("div"); + document.body.append(container); await pane.probeSessionDiscussion(SESSION_KEY); + render(pane.renderSessionDiscussionAction(), container); + container.querySelector(".chat-session-discussion-toggle")?.click(); expect( state.sidebarLayout.columns.flatMap((column) => column.panels.map((panel) => panel.slot)), ).toEqual(["detail", "discussion"]); + container.remove(); }); it("opens as a collapsed tab when two columns cannot fit side by side", async () => { @@ -162,12 +164,17 @@ describe("chat pane session discussion auto-show", () => { detailOpen: true, }); pane.paneWidth = 700; + const container = document.createElement("div"); + document.body.append(container); await pane.probeSessionDiscussion(SESSION_KEY); + render(pane.renderSessionDiscussionAction(), container); + container.querySelector(".chat-session-discussion-toggle")?.click(); expect( state.sidebarLayout.columns.flatMap((column) => column.panels.map((panel) => panel.slot)), ).toEqual(["detail", "discussion"]); + container.remove(); }); it("ignores a stale none callback after switching sessions", async () => { @@ -196,20 +203,4 @@ describe("chat pane session discussion auto-show", () => { expect(state.sidebarLayout.columns[0]?.panels[0]?.slot).toBe("discussion"); }); - - it("does not auto-show when the pane switched sessions before the probe resolved", async () => { - let resolveInfo!: (value: SessionDiscussionInfo) => void; - const { pane, state, updateSidebarLayout } = createDiscussionPane({ - info: new Promise((resolve) => { - resolveInfo = resolve; - }), - }); - - const probe = pane.probeSessionDiscussion(SESSION_KEY); - state.sessionKey = "agent:main:other"; - resolveInfo({ state: "open", embedUrl: "https://clack.example/embed/c1" }); - await probe; - - expect(updateSidebarLayout).not.toHaveBeenCalled(); - }); });