From 94f018caad57dde2bbd9e0108a9268683a8c338a Mon Sep 17 00:00:00 2001 From: wuqxuan Date: Mon, 13 Jul 2026 21:13:58 +0800 Subject: [PATCH] fix(ui): preserve sidebar collapsed state when group rename/delete fails (#106152) * fix(ui): preserve sidebar collapsed state when group rename/delete fails * test(ui): cover group rename/delete rejection error publish * fix(ui): harden session group mutations * fix(ui): guard group mutations across reconnects * fix(ui): keep session group result internal * style(ui): format sidebar tests * fix(ui): preserve confirmed group mutations * chore: keep release notes out of contributor PR * style(ui): refresh session test formatting --------- Co-authored-by: Peter Steinberger --- ui/src/components/app-sidebar.test.ts | 136 +++++++++++++++++++++- ui/src/components/app-sidebar.ts | 63 +++++++--- ui/src/e2e/session-management.e2e.test.ts | 80 +++++++++++++ ui/src/lib/sessions/index.test.ts | 93 +++++++++++++++ ui/src/lib/sessions/index.ts | 60 +++++++--- 5 files changed, 394 insertions(+), 38 deletions(-) diff --git a/ui/src/components/app-sidebar.test.ts b/ui/src/components/app-sidebar.test.ts index 1d65d7bd2021..03b279672b23 100644 --- a/ui/src/components/app-sidebar.test.ts +++ b/ui/src/components/app-sidebar.test.ts @@ -26,6 +26,8 @@ import { type LobsterLogoVisitDetail, } from "./lobster-pet.ts"; +type SessionGroupMutationResult = Awaited>; + // Keep the attention widget inert: it fires its own health RPCs (cron.list, // models.authStatus) on connect, which would interleave with the nth-call // assertions on the shared mocked client below. It has its own test file. @@ -140,6 +142,8 @@ function createSessionsHarness(agentId: string, keys: string[]) { let canonicalListRevision = 1; const listeners = new Set<(next: SessionState) => void>(); const groupsPut = vi.fn(() => Promise.resolve()); + const groupsRename = vi.fn(() => Promise.resolve("completed")); + const groupsDelete = vi.fn(() => Promise.resolve("completed")); const patch = vi.fn(() => Promise.resolve(null)); const deleteMany = vi.fn(() => Promise.resolve({ deleted: [] as string[], errors: [], preservedWorktrees: [] }), @@ -158,6 +162,8 @@ function createSessionsHarness(agentId: string, keys: string[]) { subscribeCreated: () => () => undefined, groupsLoad: () => Promise.resolve(), groupsPut, + groupsRename, + groupsDelete, patch, deleteMany, refresh: () => Promise.resolve(), @@ -171,6 +177,8 @@ function createSessionsHarness(agentId: string, keys: string[]) { return { sessions, groupsPut, + groupsRename, + groupsDelete, patch, deleteMany, publish, @@ -1794,7 +1802,6 @@ describe("AppSidebar custom group reordering", () => { expect(harness.groupsPut).toHaveBeenCalledWith(["Gamma", "Alpha", "Beta"]); }); }); - describe("AppSidebar catalog session rows", () => { const catalogList = ( sessions: Array>, @@ -1979,3 +1986,130 @@ describe("AppSidebar catalog session rows", () => { } }); }); +describe("AppSidebar group mutation collapsed state", () => { + const COLLAPSED_STORAGE_KEY = "openclaw:sidebar:sessions:collapsed-sections"; + + async function mountCollapsedGroup(options: { + groupsRename?: () => Promise; + groupsDelete?: () => Promise; + }) { + localStorage.setItem(COLLAPSED_STORAGE_KEY, JSON.stringify(["category:Alpha"])); + const gatewayHarness = createGatewayHarness({} as GatewayBrowserClient); + const harness = createSessionsHarness("main", ["agent:main:main"]); + if (options.groupsRename) { + harness.groupsRename.mockImplementation(options.groupsRename); + } + if (options.groupsDelete) { + harness.groupsDelete.mockImplementation(options.groupsDelete); + } + const { sidebar } = await mountSidebar(gatewayHarness.gateway, harness.sessions); + sidebar.connected = true; + harness.publish({ groups: ["Alpha"] }); + await sidebar.updateComplete; + return { sidebar, harness, gatewayHarness }; + } + + async function openGroupMenu(sidebar: SidebarLifecycleState) { + const actions = sidebar.querySelector( + '[data-session-section="category:Alpha"] .sidebar-session-group-actions', + ); + if (!actions) { + throw new Error("expected group actions trigger"); + } + actions.click(); + await sidebar.updateComplete; + const menu = sidebar.querySelector(".sidebar-session-group-menu"); + if (!menu) { + throw new Error("expected group menu"); + } + return menu; + } + + it("keeps collapsed keys when group rename is rejected", async () => { + const { sidebar, harness } = await mountCollapsedGroup({ + groupsRename: () => Promise.reject(new Error("rename failed")), + }); + const promptSpy = vi.spyOn(window, "prompt").mockReturnValue("Beta"); + const menu = await openGroupMenu(sidebar); + const rename = menu.querySelectorAll(".session-menu__item")[0]; + rename?.click(); + await vi.waitFor(() => expect(harness.groupsRename).toHaveBeenCalledWith("Alpha", "Beta")); + await Promise.resolve(); + await Promise.resolve(); + + expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe(JSON.stringify(["category:Alpha"])); + promptSpy.mockRestore(); + }); + + it("rewrites collapsed keys only after group rename succeeds", async () => { + const { sidebar, harness } = await mountCollapsedGroup({ + groupsRename: () => Promise.resolve("completed"), + }); + const promptSpy = vi.spyOn(window, "prompt").mockReturnValue("Beta"); + const menu = await openGroupMenu(sidebar); + menu.querySelectorAll(".session-menu__item")[0]?.click(); + await vi.waitFor(() => expect(harness.groupsRename).toHaveBeenCalledWith("Alpha", "Beta")); + await Promise.resolve(); + await Promise.resolve(); + + expect(JSON.parse(localStorage.getItem(COLLAPSED_STORAGE_KEY) ?? "[]")).toEqual([ + "category:Beta", + ]); + promptSpy.mockRestore(); + }); + + it("ignores a stale group rename after its Gateway reconnects with the same client", async () => { + let resolveRename!: (result: SessionGroupMutationResult) => void; + const rename = new Promise((resolve) => { + resolveRename = resolve; + }); + const { sidebar, harness, gatewayHarness } = await mountCollapsedGroup({ + groupsRename: () => rename, + }); + const promptSpy = vi.spyOn(window, "prompt").mockReturnValue("Beta"); + const menu = await openGroupMenu(sidebar); + menu.querySelectorAll(".session-menu__item")[0]?.click(); + await vi.waitFor(() => expect(harness.groupsRename).toHaveBeenCalledWith("Alpha", "Beta")); + + gatewayHarness.publish({ connected: false }); + gatewayHarness.publish({ connected: true }); + resolveRename("stale"); + await Promise.resolve(); + await Promise.resolve(); + + expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe(JSON.stringify(["category:Alpha"])); + promptSpy.mockRestore(); + }); + + it("keeps collapsed keys when group delete is rejected", async () => { + const { sidebar, harness } = await mountCollapsedGroup({ + groupsDelete: () => Promise.reject(new Error("delete failed")), + }); + const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true); + const menu = await openGroupMenu(sidebar); + const items = menu.querySelectorAll(".session-menu__item"); + items[items.length - 1]?.click(); + await vi.waitFor(() => expect(harness.groupsDelete).toHaveBeenCalledWith("Alpha")); + await Promise.resolve(); + await Promise.resolve(); + + expect(localStorage.getItem(COLLAPSED_STORAGE_KEY)).toBe(JSON.stringify(["category:Alpha"])); + confirmSpy.mockRestore(); + }); + + it("drops collapsed keys only after group delete succeeds", async () => { + const { sidebar, harness } = await mountCollapsedGroup({ + groupsDelete: () => Promise.resolve("completed"), + }); + const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true); + const menu = await openGroupMenu(sidebar); + const items = menu.querySelectorAll(".session-menu__item"); + items[items.length - 1]?.click(); + await vi.waitFor(() => expect(harness.groupsDelete).toHaveBeenCalledWith("Alpha")); + await Promise.resolve(); + await Promise.resolve(); + + expect(JSON.parse(localStorage.getItem(COLLAPSED_STORAGE_KEY) ?? "[]")).toEqual([]); + confirmSpy.mockRestore(); + }); +}); diff --git a/ui/src/components/app-sidebar.ts b/ui/src/components/app-sidebar.ts index 702d73bd9064..1ef828c40769 100644 --- a/ui/src/components/app-sidebar.ts +++ b/ui/src/components/app-sidebar.ts @@ -1616,40 +1616,65 @@ class AppSidebar extends OpenClawLightDomContentsElement { private renameSessionGroupFromMenu(group: string) { const context = this.context; - if (!context || !this.connected) { + const client = context?.gateway.snapshot.client; + if (!context || !client || !this.connected || !context.gateway.snapshot.connected) { return; } const next = window.prompt(t("sessionsView.renameGroupPrompt"), group)?.trim(); if (!next || next === group) { return; } - // The gateway renames the catalog entry and repoints member sessions. - void context.sessions.groupsRename(group, next).finally(() => { - const from = `category:${group}`; - if (this.collapsedSessionSections.has(from)) { - const collapsed = new Set(this.collapsedSessionSections); - collapsed.delete(from); - collapsed.add(`category:${next}`); - this.saveCollapsedSessionSections(collapsed); - } - this.requestUpdate(); - }); + // Collapse keys follow the confirmed Gateway rename only and stay scoped + // to the connection that issued it; stale completions must not rewrite storage. + void context.sessions.groupsRename(group, next).then( + (outcome) => { + if (outcome !== "completed" || !this.isCurrentSessionGroupMutation(context, client)) { + return; + } + const from = `category:${group}`; + if (this.collapsedSessionSections.has(from)) { + const collapsed = new Set(this.collapsedSessionSections); + collapsed.delete(from); + collapsed.add(`category:${next}`); + this.saveCollapsedSessionSections(collapsed); + } + this.requestUpdate(); + }, + () => undefined, + ); } private deleteSessionGroupFromMenu(group: string) { const context = this.context; - if (!context || !this.connected) { + const client = context?.gateway.snapshot.client; + if (!context || !client || !this.connected || !context.gateway.snapshot.connected) { return; } if (!window.confirm(t("sessionsView.deleteGroupConfirm", { group }))) { return; } - void context.sessions.groupsDelete(group).finally(() => { - const collapsed = new Set(this.collapsedSessionSections); - collapsed.delete(`category:${group}`); - this.saveCollapsedSessionSections(collapsed); - this.requestUpdate(); - }); + void context.sessions.groupsDelete(group).then( + (outcome) => { + if (outcome !== "completed" || !this.isCurrentSessionGroupMutation(context, client)) { + return; + } + const collapsed = new Set(this.collapsedSessionSections); + collapsed.delete(`category:${group}`); + this.saveCollapsedSessionSections(collapsed); + this.requestUpdate(); + }, + () => undefined, + ); + } + + private isCurrentSessionGroupMutation( + context: ApplicationContext, + client: GatewayBrowserClient, + ) { + const snapshot = context.gateway.snapshot; + return ( + this.context === context && this.connected && snapshot.connected && snapshot.client === client + ); } private saveCollapsedSessionSections(sections: ReadonlySet) { diff --git a/ui/src/e2e/session-management.e2e.test.ts b/ui/src/e2e/session-management.e2e.test.ts index ee370bfa07dc..d89a1b632cd5 100644 --- a/ui/src/e2e/session-management.e2e.test.ts +++ b/ui/src/e2e/session-management.e2e.test.ts @@ -17,6 +17,7 @@ const chromiumExecutablePath = resolvePlaywrightChromiumExecutablePath(chromium. const chromiumAvailable = canRunPlaywrightChromium(chromiumExecutablePath); const allowMissingChromium = process.env.OPENCLAW_UI_E2E_ALLOW_MISSING_CHROMIUM === "1"; const describeControlUiE2e = chromiumAvailable || !allowMissingChromium ? describe : describe.skip; +const collapsedSessionSectionsStorageKey = "openclaw:sidebar:sessions:collapsed-sections"; let browser: Browser; let server: ControlUiE2eServer; @@ -823,6 +824,85 @@ describeControlUiE2e("Control UI session management mocked Gateway E2E", () => { } }); + it("preserves a collapsed sidebar group when its rename is rejected", async () => { + const baseTime = Date.parse("2026-07-01T16:00:00.000Z"); + const context = await browser.newContext({ + locale: "en-US", + serviceWorkers: "block", + viewport: { height: 900, width: 1280 }, + }); + const page = await context.newPage(); + await page.addInitScript( + ({ key, value }) => { + try { + if (localStorage.getItem(key) === null) { + localStorage.setItem(key, value); + } + } catch { + // The opaque initial document has no storage; the app origin does. + } + }, + { + key: collapsedSessionSectionsStorageKey, + value: JSON.stringify(["category:Research"]), + }, + ); + const gateway = await installMockGateway(page, { + deferredMethods: ["sessions.groups.rename"], + featureMethods: ["chat.metadata", "chat.startup", "sessions.groups.list"], + methodResponses: { + "sessions.list": sessionsListResponse([ + sessionRow("agent:main:main", "Main", baseTime), + sessionRow("agent:main:paper", "Paper", baseTime - 60_000, { + category: "Research", + }), + ]), + }, + sessionGroups: ["Research"], + sessionKey: "agent:main:main", + }); + const pageErrors: string[] = []; + page.on("pageerror", (error) => pageErrors.push(error.message)); + + try { + await page.goto(`${server.baseUrl}chat`); + const researchGroup = page.locator('[data-session-section="category:Research"]'); + await researchGroup.waitFor({ state: "visible", timeout: 10_000 }); + await expect.poll(() => researchGroup.locator(".sidebar-recent-session").count()).toBe(0); + await researchGroup.locator(".sidebar-recent-sessions__head").hover(); + await researchGroup.getByRole("button", { name: "Group options for Research" }).click(); + page.once("dialog", (dialog) => void dialog.accept("Projects")); + await page.getByRole("menuitem", { name: "Rename group…" }).click(); + await gateway.waitForRequest("sessions.groups.rename"); + await gateway.rejectDeferred("sessions.groups.rename", { + code: "INVALID_REQUEST", + message: "rejected group rename", + }); + await page.evaluate( + () => + new Promise((resolve) => { + requestAnimationFrame(() => requestAnimationFrame(() => resolve())); + }), + ); + + expect( + await page.evaluate((key) => localStorage.getItem(key), collapsedSessionSectionsStorageKey), + ).toBe(JSON.stringify(["category:Research"])); + await researchGroup.waitFor({ state: "visible" }); + expect(await page.locator('[data-session-section="category:Projects"]').count()).toBe(0); + expect(pageErrors).toEqual([]); + + await page.reload(); + await researchGroup.waitFor({ state: "visible", timeout: 10_000 }); + await expect.poll(() => researchGroup.locator(".sidebar-recent-session").count()).toBe(0); + expect( + await page.evaluate((key) => localStorage.getItem(key), collapsedSessionSectionsStorageKey), + ).toBe(JSON.stringify(["category:Research"])); + } finally { + await context.close(); + } + }); + it("pages sidebar sessions and supports complete drag-managed groups", async () => { const baseTime = Date.parse("2026-07-01T16:00:00.000Z"); const context = await browser.newContext({ diff --git a/ui/src/lib/sessions/index.test.ts b/ui/src/lib/sessions/index.test.ts index 5cefd31a6df9..b1f89d853942 100644 --- a/ui/src/lib/sessions/index.test.ts +++ b/ui/src/lib/sessions/index.test.ts @@ -160,6 +160,99 @@ describe("createSessionCapability", () => { sessions.dispose(); }); + it("publishes state.error when group rename is rejected", async () => { + const request = vi.fn(async (method: string) => { + if (method === "sessions.groups.rename") { + throw new Error("rename failed"); + } + throw new Error(`Unexpected request: ${method}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const { gateway } = createGatewayHarness(client, ["sessions.groups.rename"]); + const sessions = createSessionCapability(gateway); + + await expect(sessions.groupsRename("Alpha", "Beta")).rejects.toThrow("rename failed"); + expect(sessions.state.error).toBe("Error: rename failed"); + sessions.dispose(); + }); + + it("publishes state.error when group delete is rejected", async () => { + const request = vi.fn(async (method: string) => { + if (method === "sessions.groups.delete") { + throw new Error("delete failed"); + } + throw new Error(`Unexpected request: ${method}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const { gateway } = createGatewayHarness(client, ["sessions.groups.delete"]); + const sessions = createSessionCapability(gateway); + + await expect(sessions.groupsDelete("Alpha")).rejects.toThrow("delete failed"); + expect(sessions.state.error).toBe("Error: delete failed"); + sessions.dispose(); + }); + + it("reports a group rename as stale after a same-client reconnect", async () => { + const renamed = deferred<{ groups: Array<{ name: string }> }>(); + const request = vi.fn(async (method: string) => { + if (method === "sessions.groups.rename") { + return await renamed.promise; + } + if (method === "sessions.subscribe") { + return {}; + } + if (method === "sessions.list") { + return sessionsResult([], 2); + } + throw new Error(`Unexpected request: ${method}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const { gateway, publish } = createGatewayHarness(client, ["sessions.groups.rename"]); + const sessions = createSessionCapability(gateway); + + const operation = sessions.groupsRename("Alpha", "Beta"); + publish(false); + publish(true); + renamed.resolve({ groups: [{ name: "Beta" }] }); + + await expect(operation).resolves.toBe("stale"); + expect(sessions.state.groups).toEqual([]); + sessions.dispose(); + }); + + it.each(["rename", "delete"] as const)( + "keeps a confirmed group %s completed when its row refresh outlives the connection", + async (operation) => { + const refreshed = deferred(); + const method = operation === "rename" ? "sessions.groups.rename" : "sessions.groups.delete"; + const request = vi.fn(async (requestedMethod: string) => { + if (requestedMethod === method) { + return { groups: [{ name: operation === "rename" ? "Beta" : "Other" }] }; + } + if (requestedMethod === "sessions.list") { + return await refreshed.promise; + } + throw new Error(`Unexpected request: ${requestedMethod}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const { gateway, publish } = createGatewayHarness(client, [method]); + const sessions = createSessionCapability(gateway); + + const mutation = + operation === "rename" + ? sessions.groupsRename("Alpha", "Beta") + : sessions.groupsDelete("Alpha"); + await vi.waitFor(() => + expect(request).toHaveBeenCalledWith("sessions.list", expect.any(Object)), + ); + publish(false); + refreshed.resolve(sessionsResult([], 2)); + + await expect(mutation).resolves.toBe("completed"); + sessions.dispose(); + }, + ); + it("does not probe for a group catalog when the method is explicitly absent", async () => { const request = vi.fn(); const client = { request } as unknown as GatewayBrowserClient; diff --git a/ui/src/lib/sessions/index.ts b/ui/src/lib/sessions/index.ts index 98a59a472539..00bf9b7f697b 100644 --- a/ui/src/lib/sessions/index.ts +++ b/ui/src/lib/sessions/index.ts @@ -62,6 +62,8 @@ export type SessionState = { groups: readonly string[]; }; +type SessionGroupMutationResult = "completed" | "stale"; + export type SessionListOptions = { agentId?: string; activeMinutes?: number; @@ -240,10 +242,10 @@ export type SessionCapability = { groupsLoad: () => Promise; /** Replaces the gateway-owned group catalog (order included). */ groupsPut: (names: readonly string[]) => Promise; - /** Renames a group; the gateway repoints member sessions server-side. */ - groupsRename: (from: string, to: string) => Promise; - /** Deletes a group; the gateway clears member categories server-side. */ - groupsDelete: (name: string) => Promise; + /** Renames a group; stale means the initiating connection retired before reconciliation. */ + groupsRename: (from: string, to: string) => Promise; + /** Deletes a group; stale means the initiating connection retired before reconciliation. */ + groupsDelete: (name: string) => Promise; subscribeCreated: (listener: (key: string) => void) => () => void; subscribe: (listener: (state: SessionState) => void) => () => void; dispose: () => void; @@ -994,30 +996,52 @@ export function createSessionCapability(gateway: SessionGateway): SessionCapabil } }; - const groupsRename = async (from: string, to: string) => { + const groupsRename = async (from: string, to: string): Promise => { const scope = captureConnection(); if (!scope) { - return; + return "stale"; } - const result = await scope.client.request("sessions.groups.rename", { name: from, to }); - if (!isCurrentConnection(scope)) { - return; + try { + const result = await scope.client.request("sessions.groups.rename", { name: from, to }); + if (!isCurrentConnection(scope)) { + return "stale"; + } + publishGroups(readGroupNames(result)); + // The mutation response is the commit point. Reconcile member rows in + // the background so a later disconnect cannot downgrade confirmed work. + void refresh({ ...lastListOptions, force: true }); + return "completed"; + } catch (error) { + if (!isCurrentConnection(scope)) { + return "stale"; + } + publish({ ...state, error: String(error) }); + throw error; } - publishGroups(readGroupNames(result)); - await refresh({ ...lastListOptions, force: true }); }; - const groupsDelete = async (name: string) => { + const groupsDelete = async (name: string): Promise => { const scope = captureConnection(); if (!scope) { - return; + return "stale"; } - const result = await scope.client.request("sessions.groups.delete", { name }); - if (!isCurrentConnection(scope)) { - return; + try { + const result = await scope.client.request("sessions.groups.delete", { name }); + if (!isCurrentConnection(scope)) { + return "stale"; + } + publishGroups(readGroupNames(result)); + // See groupsRename: collapsed-state consumers must observe confirmed + // completion before an unrelated refresh can outlive the connection. + void refresh({ ...lastListOptions, force: true }); + return "completed"; + } catch (error) { + if (!isCurrentConnection(scope)) { + return "stale"; + } + publish({ ...state, error: String(error) }); + throw error; } - publishGroups(readGroupNames(result)); - await refresh({ ...lastListOptions, force: true }); }; const patch = async (