From 1228b6e5b21ac4f5d4ef0581c31367d7e0f22c02 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Sun, 14 Jun 2026 13:38:46 +0800 Subject: [PATCH 1/2] fix(agent-sidebar): restore width+open on reload, add deleteConversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent cross-host agent sidebar defects, all against v0.5.6: 1. Width not restored on reload. SidebarProvider's mount fetch only set lastOpenWidth, never the rendered sidebarWidth, so a persisted width was stranded until the next open gesture (close-then-reopen restored it; reload did not). Reconcile the fetched width into rendered state via a guarded functional updater (cur <= 0) so a user gesture landing before the async fetch is never clobbered. 2. Open flag dropped on reload. AppShell derived visibility purely from width, with no controlled-open prop, so the shell always painted closed regardless of the persisted open flag both host session hooks already expose. Add open?/onOpenChange? to AppShellProps, derive isOpen = open ?? (width > 0), seed/collapse width via an effect when controlled open flips, and fire onOpenChange from the open (FAB) and close handlers. Uncontrolled (open undefined) behavior is unchanged. 3. Remove conversation had no transport. The trash-icon delete button already shipped but nothing backed it. Add deleteConversation to the AgentChatClient contract and useAgentChat (optimistic history removal + thread clear when the open conversation is deleted, rollback on failure), re-exported through useAgentSession. No version bump — rides a later rollup release. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../layout/app-shell/AppShell.test.tsx | 67 +++++++++++++++++-- src/components/layout/app-shell/AppShell.tsx | 56 ++++++++++++++-- src/context/sidebar/SidebarProvider.tsx | 11 +++ src/hooks/agent-chat/client.ts | 4 ++ src/hooks/agent-chat/useAgentChat.test.ts | 44 ++++++++++++ src/hooks/agent-chat/useAgentChat.ts | 35 ++++++++++ src/hooks/agent-chat/useAgentSession.test.ts | 1 + src/hooks/agent-chat/useAgentSession.ts | 2 + 8 files changed, 211 insertions(+), 9 deletions(-) diff --git a/src/components/layout/app-shell/AppShell.test.tsx b/src/components/layout/app-shell/AppShell.test.tsx index 38b87b711..27676f6fc 100644 --- a/src/components/layout/app-shell/AppShell.test.tsx +++ b/src/components/layout/app-shell/AppShell.test.tsx @@ -257,13 +257,71 @@ describe("AppShell agent sidebar pass-through", () => { }); }); +describe("AppShell controlled sidebar open", () => { + // jsdom reports clientWidth 0; give the strip a real width so the header + // renders (same fixup the pass-through suite uses). + beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, "clientWidth", { + configurable: true, + value: 400, + }); + }); + afterAll(() => { + Reflect.deleteProperty(HTMLElement.prototype, "clientWidth"); + }); + + it("paints the sidebar open (no FAB) when open is true, without a click", () => { + render( + {}}> + content + , + ); + // The controlled-open effect seeds the width, so the agent header shows and + // the open FAB is gone — the persisted open flag drove it open on mount. + expect(screen.getByLabelText("Chat history")).toBeInTheDocument(); + expect(screen.queryByLabelText("Open agent")).not.toBeInTheDocument(); + }); + + it("paints closed (FAB shown) when open is false", () => { + render( + {}}> + content + , + ); + expect(screen.getByLabelText("Open agent")).toBeInTheDocument(); + expect(screen.queryByLabelText("Chat history")).not.toBeInTheDocument(); + }); + + it("fires onOpenChange(true) when the FAB opens the sidebar", () => { + const onOpenChange = vi.fn(); + render( + + content + , + ); + fireEvent.click(screen.getByLabelText("Open agent")); + expect(onOpenChange).toHaveBeenCalledWith(true); + }); + + it("fires onOpenChange(false) when the sidebar is closed", () => { + const onOpenChange = vi.fn(); + render( + + content + , + ); + fireEvent.click(screen.getByLabelText("Close sidebar")); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); +}); + describe("AppShell sidebar width persistence", () => { // Width now lives in the INJECTED server persistence (docs 12.4 + 13b), ONE // shared per-user value across every host — NOT per-origin localStorage. The // host wires `sidebarWidthPersistence`; the rail reads it on mount and writes // it on resize-end. - it("reopens to the server-persisted width after reload (clamped to the viewport)", async () => { + it("restores the server-persisted width on reload (reconciled into rendered state)", async () => { // Simulate a prior session that drag-resized to 500px, persisted to the // user's shared sidebar-state row. jsdom's window.innerWidth is 1024, so // 500 is well within [350, 1004]. @@ -273,12 +331,13 @@ describe("AppShell sidebar width persistence", () => { content, ); // The mount read resolves asynchronously (get() may be a promise) — wait - // for lastOpenWidth to rehydrate before opening. + // for it to land. The fetch now reconciles into the RENDERED width (not + // just the reopen memory), so the sidebar repaints at the user's size on + // reload without a manual open gesture — the fix for defect #1 (width + // stranded in lastOpenWidth until the next open click). await act(async () => { await Promise.resolve(); }); - // Sidebar starts closed (toggle button shown), not auto-opened. - fireEvent.click(screen.getByLabelText("Open agent")); // The agent inner panel width reflects the rehydrated 500, not the 350 // floor — the persisted size survived the "reload". diff --git a/src/components/layout/app-shell/AppShell.tsx b/src/components/layout/app-shell/AppShell.tsx index da93e7ef1..3c8edc81f 100644 --- a/src/components/layout/app-shell/AppShell.tsx +++ b/src/components/layout/app-shell/AppShell.tsx @@ -59,6 +59,19 @@ export interface AppShellProps { * in-memory-only width — Storybook and pre-migration hosts keep working, * width just doesn't persist. */ sidebarWidthPersistence?: SidebarWidthPersistence; + /** Controlled sidebar open/closed. Wire it to the host's persisted session + * open flag (`useAgentSession().open`) so a reload repaints the sidebar in + * the state the user left it — without it the shell derives visibility from + * width alone and always paints closed on reload. When `true` while the + * width is still 0 (fresh open, or restored-open before the width fetch + * lands) the shell seeds the width from the remembered/half-viewport size; + * when `false` while open it collapses to 0. Omit it (undefined) to keep the + * uncontrolled width-derived behavior unchanged. */ + open?: boolean; + /** Called whenever the user opens or closes the sidebar (toggle, close, + * FAB). Pair with `open` to persist the flag — e.g. + * `useAgentSession().setOpen`. */ + onOpenChange?: (open: boolean) => void; /** Class for the outer shell */ className?: string; /** Class for the app switcher container */ @@ -248,6 +261,8 @@ function AppShellInner({ mobileNavigation, mobileNavigationVariant = "bottom", appSwitcher, + open, + onOpenChange, className, appSwitcherClassName = "max-w-7xl", contentClassName, @@ -299,8 +314,13 @@ function AppShellInner({ return () => window.removeEventListener("resize", update); }, []); + // Sidebar visibility: controlled by `open` when the host wires it (restores + // the persisted open flag on reload), else derived from width alone + // (uncontrolled — behavior unchanged when `open` is undefined). + const isOpen = open ?? sidebarWidth > 0; + const isOverlaying = - sidebarWidth > 0 && + isOpen && windowWidth > 0 && windowWidth < sidebarWidth + 800; @@ -355,6 +375,29 @@ function AppShellInner({ return Math.min(Math.max(remembered, MIN_WIDTH), maxWidthRef.current); }; + // When `open` is controlled, keep the rendered width in lockstep with it: + // open === true while the width is still 0 (fresh open, or a reload that + // restored open:true before the async width fetch landed) seeds the width; + // open === false while open collapses it. The width fetch's own reconcile + // (SidebarProvider) only touches a default-0 width, so the two don't fight: + // the first one to land wins and the other no-ops. Uncontrolled + // (open === undefined) skips this entirely. + useEffect(() => { + if (open === undefined) return; + if (open && sidebarWidth === 0) setSidebarWidth(reopenWidth()); + else if (!open && sidebarWidth > 0) setSidebarWidth(0); + // reopenWidth reads refs/state captured per render; depend on the inputs + // that actually change the decision. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open, sidebarWidth]); + + // Open/close the sidebar AND notify the host so it can persist the flag. + // When uncontrolled (`onOpenChange` omitted) only the width changes. + const openSidebar = () => { + setSidebarWidth(reopenWidth()); + onOpenChange?.(true); + }; + const handleToggleCollapse = () => { if (sidebarWidth <= MIN_WIDTH) { setSidebarWidth(reopenWidth()); @@ -363,7 +406,10 @@ function AppShellInner({ } }; - const handleClose = () => setSidebarWidth(0); + const handleClose = () => { + setSidebarWidth(0); + onOpenChange?.(false); + }; return (
@@ -427,7 +473,7 @@ function AppShellInner({ @starting-style block that holds width at 0 on mount, then transition-all interpolates up to the inline style — so open uses the same curve + duration as drag-end + collapse. */} - {sidebarWidth > 0 && ( + {isOpen && sidebarWidth > 0 && (
setSidebarWidth(reopenWidth())} + onClick={openSidebar} aria-label="Open agent" /> )} diff --git a/src/context/sidebar/SidebarProvider.tsx b/src/context/sidebar/SidebarProvider.tsx index a715b79ff..362ce8e4c 100644 --- a/src/context/sidebar/SidebarProvider.tsx +++ b/src/context/sidebar/SidebarProvider.tsx @@ -151,6 +151,17 @@ export function SidebarProvider({ if (stored === null) return; if (stored < min || stored > max) return; setLastOpenWidth(stored); + // Reconcile the fetched width into the RENDERED state, not just the + // reopen memory (`lastOpenWidth`). Without this the fetched width is + // stranded — it only surfaces on the NEXT open gesture (reopenWidth reads + // lastOpenWidth), which is why close-then-reopen restored it but a reload + // did not. The functional updater seeds only from the default floor + // (`cur <= 0`), so a user open gesture (or AppShell's controlled-open + // effect) that landed a real width before this async fetch resolved is + // never clobbered — first writer of a real width wins. No render gate: + // width intentionally paints default-then-hydrate to avoid an SSR + // mismatch. + setSidebarWidth((cur) => (cur <= 0 ? stored : cur)); }; const persistence = persistenceRef.current; diff --git a/src/hooks/agent-chat/client.ts b/src/hooks/agent-chat/client.ts index 2a26cab0b..29f578f4f 100644 --- a/src/hooks/agent-chat/client.ts +++ b/src/hooks/agent-chat/client.ts @@ -7,6 +7,7 @@ // listConversations: (o) => listConversations(agentApi, o), // createConversation: (o) => createConversation(agentApi, o), // renameConversation: (id, t) => renameConversation(agentApi, id, t), +// deleteConversation: (id) => deleteConversation(agentApi, id), // patchConversationModel: (id, m) => patchConversationModel(agentApi, id, m), // listConversationMessages: (id) => listConversationMessages(agentApi, id), // fetchAgentModels: () => fetchAgentModels(agentApi), @@ -137,6 +138,9 @@ export interface AgentChatClient { listConversations(opts?: ListConversationsOptions): Promise; createConversation(opts?: CreateConversationOptions): Promise; renameConversation(conversationId: string, title: string): Promise; + /** Permanently delete a conversation and all its messages (ms_agent.message + * FK is ON DELETE CASCADE, so one DELETE removes the whole thread). */ + deleteConversation(conversationId: string): Promise; /** Re-pin the conversation's model ("" clears to the platform default). */ patchConversationModel(conversationId: string, model: string): Promise; /** Full replay, oldest first — includes superseded rows (clients hide them). */ diff --git a/src/hooks/agent-chat/useAgentChat.test.ts b/src/hooks/agent-chat/useAgentChat.test.ts index b20e6d06a..6354c9122 100644 --- a/src/hooks/agent-chat/useAgentChat.test.ts +++ b/src/hooks/agent-chat/useAgentChat.test.ts @@ -28,6 +28,7 @@ function fakeClient(over: Partial = {}): AgentChatClient { renameConversation: vi.fn().mockImplementation(async (id: string, title: string) => conv({ id, title }), ), + deleteConversation: vi.fn().mockResolvedValue(undefined), patchConversationModel: vi.fn().mockImplementation(async (id: string, model: string) => conv({ id, model }), ), @@ -447,6 +448,49 @@ describe("useAgentChat", () => { expect(result.current.history?.[0]?.items[0]?.title).toBe("Old title"); }); + it("deleteConversation removes the history row optimistically and rolls back on failure", async () => { + const client = fakeClient({ + listConversations: vi.fn().mockResolvedValue({ + items: [ + conv({ id: "c-1", title: "Keep me", updatedAt: NOW }), + conv({ id: "c-2", title: "Delete me", updatedAt: NOW }), + ], + nextCursor: null, + }), + deleteConversation: vi.fn().mockRejectedValue(new Error("500")), + }); + const { result } = renderHook(() => useAgentChat(client, { appId: null })); + await waitFor(() => expect(result.current.history).toBeDefined()); + + act(() => result.current.deleteConversation("c-2")); + expect(result.current.history?.[0]?.items.map((i) => i.id)).toEqual(["c-1"]); + expect(client.deleteConversation).toHaveBeenCalledWith("c-2"); + + await flush(); + expect(result.current.history?.[0]?.items.map((i) => i.id)).toEqual(["c-1", "c-2"]); + }); + + it("deleteConversation clears the open thread when the deleted id is active", async () => { + const client = fakeClient({ + listConversations: vi.fn().mockResolvedValue({ + items: [conv({ id: "c-1", title: "Open one", updatedAt: NOW })], + nextCursor: null, + }), + listConversationMessages: vi + .fn() + .mockResolvedValue([{ id: "m-1", role: "user", content: "hi", createdAt: NOW }]), + }); + const { result } = renderHook(() => useAgentChat(client, { appId: null })); + await waitFor(() => expect(result.current.history).toBeDefined()); + + act(() => result.current.selectConversation("c-1")); + await waitFor(() => expect(result.current.messages.length).toBe(1)); + + act(() => result.current.deleteConversation("c-1")); + expect(result.current.messages).toEqual([]); + expect(client.deleteConversation).toHaveBeenCalledWith("c-1"); + }); + it("maps the model roster and falls back to the server default selection", async () => { const client = fakeClient({ fetchAgentModels: vi.fn().mockResolvedValue({ diff --git a/src/hooks/agent-chat/useAgentChat.ts b/src/hooks/agent-chat/useAgentChat.ts index 7cfe398a9..38d1cb965 100644 --- a/src/hooks/agent-chat/useAgentChat.ts +++ b/src/hooks/agent-chat/useAgentChat.ts @@ -96,6 +96,10 @@ export interface UseAgentChatResult { /** Retitle a past conversation — optimistic in history, rolls back on * failure. Empty titles are ignored. */ renameConversation: (id: string, title: string) => void; + /** Permanently delete a past conversation — optimistic (the history row + * vanishes now), rolls back on failure. Clears the open thread when the + * deleted id is the active conversation. */ + deleteConversation: (id: string) => void; /** Roster for the input's model picker — undefined hides the picker * (e.g. /v1/models not deployed yet). */ models: AgentSidebarInputModel[] | undefined; @@ -567,6 +571,36 @@ export function useAgentChat( [conversations], ); + const deleteConversation = useCallback( + (id: string) => { + // Optimistic: snapshot the prior list for rollback, then drop the row + // now so the history entry vanishes immediately. The snapshot is read + // from state (not inside the updater) so it can't depend on updater + // invocation count. + const prevConversations = conversations; + setConversations((prev) => prev?.filter((c) => c.id !== id)); + + // Deleting the OPEN conversation must clear the thread too — otherwise a + // freshly-deleted conversation keeps rendering its messages and the next + // send would resurrect it under the dead id. Abort any in-flight stream + // and null the ref so the next send lazy-creates a fresh conversation. + if (conversationIdRef.current === id) { + abortRef.current?.abort(); + conversationIdRef.current = null; + createConversationRef.current = null; + setMessages([]); + } + + clientRef.current.deleteConversation(id).catch((err) => { + console.error("agent: delete failed", err); + // Restore the history row on failure; the cleared thread is not + // re-loaded (the host can reselect the conversation to reopen it). + setConversations(() => prevConversations); + }); + }, + [conversations], + ); + const rateMessage = useCallback( (messageId: string, rating: AgentSidebarMessageFeedback) => { const convId = conversationIdRef.current; @@ -673,6 +707,7 @@ export function useAgentChat( history, selectConversation, renameConversation, + deleteConversation, models, // The pick when the user made one, else the server default — never // let the consumer guess (a first-enabled fallback can advertise a diff --git a/src/hooks/agent-chat/useAgentSession.test.ts b/src/hooks/agent-chat/useAgentSession.test.ts index 384e464bf..9b52f3d10 100644 --- a/src/hooks/agent-chat/useAgentSession.test.ts +++ b/src/hooks/agent-chat/useAgentSession.test.ts @@ -49,6 +49,7 @@ function fakeChatClient(over: Partial = {}): AgentChatClient { renameConversation: vi .fn() .mockImplementation(async (id: string, title: string) => conv({ id, title })), + deleteConversation: vi.fn().mockResolvedValue(undefined), patchConversationModel: vi .fn() .mockImplementation(async (id: string, model: string) => conv({ id, model })), diff --git a/src/hooks/agent-chat/useAgentSession.ts b/src/hooks/agent-chat/useAgentSession.ts index 85e0c8800..99b191d17 100644 --- a/src/hooks/agent-chat/useAgentSession.ts +++ b/src/hooks/agent-chat/useAgentSession.ts @@ -96,6 +96,7 @@ export interface UseAgentSessionResult | "rerunMessage" | "history" | "renameConversation" + | "deleteConversation" | "models" | "selectedModelId" | "selectModel" @@ -364,6 +365,7 @@ export function useAgentSession(deps: UseAgentSessionDeps): UseAgentSessionResul rerunMessage: chat.rerunMessage, history: chat.history, renameConversation: chat.renameConversation, + deleteConversation: chat.deleteConversation, models: chat.models, selectedModelId: chat.selectedModelId, selectModel: chat.selectModel, From 0a48665454a0a68cf9004cb6a32fe391487e18b7 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Sun, 14 Jun 2026 13:47:21 +0800 Subject: [PATCH 2/2] test(agent-sidebar): fix legacy-path SSR test + harden delete rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the agent-sidebar reload/delete fix: - SidebarProvider.test.tsx: the legacy localStorage `persistKey` unit test ("does not read localStorage at module load") still asserted width=0 after mount, the pre-fix behavior. Defect #1's reconcile (setSidebarWidth((cur) => cur <= 0 ? stored : cur)) now restores a persisted 500 into the rendered width over a defaultWidth of 0, so the assertion + comment are updated to expect 500. The equivalent server-persistence test was already updated; this legacy path was missed and was landing main red. - useAgentChat.deleteConversation: switch the rollback from a full-list overwrite to a MERGE updater (mirrors renameConversation), re-inserting the removed row at its original index — preserves any conversation created between the optimistic drop and the failed network call instead of clobbering it. Behavior-identical in the common case. - AppShell.tsx: clarify two comments (controlled-open effect dep rationale; the load-bearing `sidebarWidth > 0` panel guard) — no behavior change. No version bump — rides a later rollup release. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/components/layout/app-shell/AppShell.tsx | 8 +++++-- src/context/sidebar/SidebarProvider.test.tsx | 14 +++++++----- src/hooks/agent-chat/useAgentChat.ts | 24 ++++++++++++++------ 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/components/layout/app-shell/AppShell.tsx b/src/components/layout/app-shell/AppShell.tsx index 3c8edc81f..3a1a700d3 100644 --- a/src/components/layout/app-shell/AppShell.tsx +++ b/src/components/layout/app-shell/AppShell.tsx @@ -386,8 +386,8 @@ function AppShellInner({ if (open === undefined) return; if (open && sidebarWidth === 0) setSidebarWidth(reopenWidth()); else if (!open && sidebarWidth > 0) setSidebarWidth(0); - // reopenWidth reads refs/state captured per render; depend on the inputs - // that actually change the decision. + // reopenWidth is recreated each render; we intentionally depend only on + // [open, sidebarWidth], the inputs that gate this decision. // eslint-disable-next-line react-hooks/exhaustive-deps }, [open, sidebarWidth]); @@ -473,6 +473,10 @@ function AppShellInner({ @starting-style block that holds width at 0 on mount, then transition-all interpolates up to the inline style — so open uses the same curve + duration as drag-end + collapse. */} + {/* `sidebarWidth > 0` is load-bearing, not redundant with isOpen: in the + controlled-open path open can be true while the seed effect hasn't yet + landed a real width, so we wait for it before painting (avoids a 0-width + flash). Uncontrolled, the two halves are equivalent. */} {isOpen && sidebarWidth > 0 && (
{ it("does not read localStorage at module load (SSR-safe — value applied after mount)", () => { // A pre-existing stored value must NOT bleed into the first render: the - // default stands on the SSR/first pass, the stored value applies in an - // effect. Guards against hydration mismatch. + // default stands on the SSR/first pass, the stored value applies in a + // post-mount effect. Guards against hydration mismatch. localStorage.setItem(SIDEBAR_WIDTH_STORAGE_KEY, "500"); const getItem = vi.spyOn(Storage.prototype, "getItem"); @@ -85,10 +85,12 @@ describe("SidebarProvider persistence", () => { , ); - // The width state still reflects the default on the committed render — the - // sidebar doesn't auto-open from a persisted width. - expect(screen.getByTestId("width").textContent).toBe("0"); - // After mount, lastOpenWidth carries the rehydrated value for reopen. + // After mount, the persisted width is reconciled into the RENDERED state + // (defect #1 fix: a persisted width is restored on reload, not stranded in + // reopen memory). The functional updater seeds only from the default floor + // (cur <= 0), so a persisted 500 over a defaultWidth of 0 applies here. + expect(screen.getByTestId("width").textContent).toBe("500"); + // lastOpenWidth also carries the rehydrated value for reopen. expect(screen.getByTestId("last-open").textContent).toBe("500"); getItem.mockRestore(); }); diff --git a/src/hooks/agent-chat/useAgentChat.ts b/src/hooks/agent-chat/useAgentChat.ts index 38d1cb965..b6890130c 100644 --- a/src/hooks/agent-chat/useAgentChat.ts +++ b/src/hooks/agent-chat/useAgentChat.ts @@ -573,11 +573,12 @@ export function useAgentChat( const deleteConversation = useCallback( (id: string) => { - // Optimistic: snapshot the prior list for rollback, then drop the row - // now so the history entry vanishes immediately. The snapshot is read - // from state (not inside the updater) so it can't depend on updater + // Optimistic: snapshot the removed row AND its position for rollback, then + // drop it now so the history entry vanishes immediately. The snapshot is + // read from state (not inside the updater) so it can't depend on updater // invocation count. - const prevConversations = conversations; + const removedIndex = conversations?.findIndex((c) => c.id === id) ?? -1; + const removedRow = removedIndex >= 0 ? conversations?.[removedIndex] : undefined; setConversations((prev) => prev?.filter((c) => c.id !== id)); // Deleting the OPEN conversation must clear the thread too — otherwise a @@ -593,9 +594,18 @@ export function useAgentChat( clientRef.current.deleteConversation(id).catch((err) => { console.error("agent: delete failed", err); - // Restore the history row on failure; the cleared thread is not - // re-loaded (the host can reselect the conversation to reopen it). - setConversations(() => prevConversations); + // Restore the removed row on failure with a MERGE updater (mirrors + // renameConversation's rollback) so any conversation created between the + // optimistic drop and this failure is preserved rather than clobbered. + // Re-insert at the original index to keep history order stable. The + // cleared thread is not re-loaded (the host can reselect the + // conversation to reopen it). + setConversations((prev) => { + if (!removedRow || prev?.some((c) => c.id === id)) return prev; + const next = [...(prev ?? [])]; + next.splice(Math.min(removedIndex, next.length), 0, removedRow); + return next; + }); }); }, [conversations],