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..3a1a700d3 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 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]); + + // 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,11 @@ 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 && ( + {/* `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 && (
setSidebarWidth(reopenWidth())} + onClick={openSidebar} aria-label="Open agent" /> )} diff --git a/src/context/sidebar/SidebarProvider.test.tsx b/src/context/sidebar/SidebarProvider.test.tsx index 4a6b0e3a2..87b3f4b94 100644 --- a/src/context/sidebar/SidebarProvider.test.tsx +++ b/src/context/sidebar/SidebarProvider.test.tsx @@ -70,8 +70,8 @@ describe("SidebarProvider persistence", () => { 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/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..b6890130c 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,46 @@ export function useAgentChat( [conversations], ); + const deleteConversation = useCallback( + (id: string) => { + // 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 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 + // 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 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], + ); + const rateMessage = useCallback( (messageId: string, rating: AgentSidebarMessageFeedback) => { const convId = conversationIdRef.current; @@ -673,6 +717,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,