From 0fc532da18128bd4dd457d69c1c5bd2f2d44f965 Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Sun, 14 Jun 2026 15:53:36 +0800 Subject: [PATCH] feat(agent): confirm before deleting a conversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent sidebar history row's trash icon deleted a conversation immediately on click, so a misclick permanently removed it. Clicking delete now opens a destructive confirmation dialog first; onDeleteConversation fires ONLY on confirm — Cancel, Escape, and backdrop click do nothing. Reuses the kit's existing Dialog primitive (error color + filled confirm + text cancel), matching the TypeToConfirmDialog destructive precedent. Focus trap, Escape, backdrop dismiss, and focus return are handled by Dialog per kit conventions. Adds four OPTIONAL, localizable label overrides to AgentSidebarHeaderLabels (deleteConfirmTitle / deleteConfirmMessage / deleteConfirmConfirmLabel / deleteConfirmCancelLabel) with EN defaults. They thread through the existing AppShell agentHeaderLabels -> header labels mechanism, so existing consumers keep working unchanged. No version bump — rides the next kit rollup release. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../layout/app-shell/AppShell.test.tsx | 4 ++ src/components/layout/app-shell/AppShell.tsx | 9 +-- .../ui/agent/sidebar/AgentSidebar.stories.tsx | 4 +- .../ui/agent/sidebar/AgentSidebar.test.tsx | 63 ++++++++++++++++++- .../ui/agent/sidebar/AgentSidebarHeader.tsx | 43 ++++++++++++- src/components/ui/agent/sidebar/types.ts | 9 +++ 6 files changed, 122 insertions(+), 10 deletions(-) diff --git a/src/components/layout/app-shell/AppShell.test.tsx b/src/components/layout/app-shell/AppShell.test.tsx index 27676f6fc..3d5fc1f47 100644 --- a/src/components/layout/app-shell/AppShell.test.tsx +++ b/src/components/layout/app-shell/AppShell.test.tsx @@ -143,7 +143,11 @@ describe("AppShell agent sidebar pass-through", () => { fireEvent.keyDown(input, { key: "Enter" }); expect(onRename).toHaveBeenCalledWith("h1", "New title"); + // Delete opens a confirmation dialog first; the forwarded handler fires + // only after the user clicks the destructive confirm button. fireEvent.click(screen.getByLabelText("Delete conversation")); + expect(onDelete).not.toHaveBeenCalled(); + fireEvent.click(screen.getByRole("button", { name: "Delete" })); expect(onDelete).toHaveBeenCalledWith("h1"); }); diff --git a/src/components/layout/app-shell/AppShell.tsx b/src/components/layout/app-shell/AppShell.tsx index 3a1a700d3..6d58af848 100644 --- a/src/components/layout/app-shell/AppShell.tsx +++ b/src/components/layout/app-shell/AppShell.tsx @@ -118,11 +118,12 @@ export interface AppShellProps { /** Enables the hover rename affordance on agent history rows. * Called with the trimmed title (1-200 chars) when the user commits an inline rename. */ onRenameAgentConversation?: AgentSidebarHeaderProps["onRenameConversation"]; - /** Enables the hover delete affordance on agent history rows. Confirmation - * (if any) is the consumer's responsibility; the kit fires immediately. */ + /** Enables the hover delete affordance on agent history rows. Clicking it + * opens a destructive confirmation dialog first; this fires only once the + * user confirms (cancel/dismiss does nothing). */ onDeleteAgentConversation?: AgentSidebarHeaderProps["onDeleteConversation"]; - /** Label overrides for the agent header (history, rename, new-chat strings). - * All have EN defaults. */ + /** Label overrides for the agent header (history, rename, new-chat, and the + * delete-confirmation dialog strings). All have EN defaults. */ agentHeaderLabels?: AgentSidebarHeaderProps["labels"]; /** Messages waiting to send once the current reply finishes — rendered as * cancellable rows above the agent input, in send order. Display-only: diff --git a/src/components/ui/agent/sidebar/AgentSidebar.stories.tsx b/src/components/ui/agent/sidebar/AgentSidebar.stories.tsx index 9b5e82f23..27c3f7d3b 100644 --- a/src/components/ui/agent/sidebar/AgentSidebar.stories.tsx +++ b/src/components/ui/agent/sidebar/AgentSidebar.stories.tsx @@ -125,7 +125,9 @@ export const ActiveLastTab: StoryObj = { }, }; -/** History row actions: hover edit (inline rename) + hover delete. */ +/** History row actions: hover edit (inline rename) + hover delete. Delete + * opens a destructive confirmation dialog; onDeleteConversation fires only + * on confirm. */ export const History: StoryObj = { render: () => { const [history, setHistory] = useState(mockAgentHistory); diff --git a/src/components/ui/agent/sidebar/AgentSidebar.test.tsx b/src/components/ui/agent/sidebar/AgentSidebar.test.tsx index e41023f7f..51f8e119f 100644 --- a/src/components/ui/agent/sidebar/AgentSidebar.test.tsx +++ b/src/components/ui/agent/sidebar/AgentSidebar.test.tsx @@ -1,6 +1,7 @@ import { cleanup, render, screen, fireEvent } from "@testing-library/react"; import { describe, expect, it, vi, afterEach, beforeAll, afterAll } from "vitest"; import { AgentSidebarHeader } from "./AgentSidebarHeader"; +import type { AgentSidebarHeaderProps } from "./AgentSidebarHeader"; import { AgentSidebarInput } from "./AgentSidebarInput"; import type { AgentSidebarHistoryGroup, ChatTab } from "./types"; @@ -250,6 +251,7 @@ describe("AgentSidebarHeader history delete", () => { const renderHistory = (props?: { onDelete?: (id: string) => void; onSelect?: (id: string) => void; + labels?: AgentSidebarHeaderProps["labels"]; }) => render( { history={historyData} onSelectHistoryItem={props?.onSelect ?? (() => {})} onDeleteConversation={props?.onDelete} + labels={props?.labels} />, ); const openHistory = () => fireEvent.click(screen.getByLabelText("Chat history")); + const clickDelete = () => fireEvent.click(screen.getByLabelText("Delete conversation")); it("shows the delete button when onDeleteConversation is provided", () => { renderHistory({ onDelete: () => {} }); @@ -276,14 +280,69 @@ describe("AgentSidebarHeader history delete", () => { expect(screen.queryByLabelText("Delete conversation")).not.toBeInTheDocument(); }); - it("calls onDeleteConversation with the row id, without opening the conversation", () => { + it("opens a confirmation dialog instead of deleting immediately", () => { const onDelete = vi.fn(); const onSelect = vi.fn(); renderHistory({ onDelete, onSelect }); openHistory(); - fireEvent.click(screen.getByLabelText("Delete conversation")); + clickDelete(); + // Dialog is shown; nothing deleted or opened yet. + expect(screen.getByRole("dialog")).toBeInTheDocument(); + expect(screen.getByText("Delete conversation?")).toBeInTheDocument(); + expect(onDelete).not.toHaveBeenCalled(); + expect(onSelect).not.toHaveBeenCalled(); + }); + + it("calls onDeleteConversation with the row id only after confirm", () => { + const onDelete = vi.fn(); + const onSelect = vi.fn(); + renderHistory({ onDelete, onSelect }); + openHistory(); + clickDelete(); + fireEvent.click(screen.getByRole("button", { name: "Delete" })); expect(onDelete).toHaveBeenCalledWith("h-1"); + expect(onDelete).toHaveBeenCalledTimes(1); expect(onSelect).not.toHaveBeenCalled(); + // Dialog closes after confirm. + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("does not delete when the dialog is cancelled", () => { + const onDelete = vi.fn(); + renderHistory({ onDelete }); + openHistory(); + clickDelete(); + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect(onDelete).not.toHaveBeenCalled(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("does not delete when the dialog is dismissed via Escape", () => { + const onDelete = vi.fn(); + renderHistory({ onDelete }); + openHistory(); + clickDelete(); + fireEvent.keyDown(document, { key: "Escape" }); + expect(onDelete).not.toHaveBeenCalled(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("uses overridable labels for the confirm dialog", () => { + renderHistory({ + onDelete: () => {}, + labels: { + deleteConfirmTitle: "Eliminar conversación?", + deleteConfirmMessage: "Se eliminará de forma permanente.", + deleteConfirmConfirmLabel: "Eliminar", + deleteConfirmCancelLabel: "Cancelar", + }, + }); + openHistory(); + clickDelete(); + expect(screen.getByText("Eliminar conversación?")).toBeInTheDocument(); + expect(screen.getByText("Se eliminará de forma permanente.")).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Eliminar" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Cancelar" })).toBeInTheDocument(); }); }); diff --git a/src/components/ui/agent/sidebar/AgentSidebarHeader.tsx b/src/components/ui/agent/sidebar/AgentSidebarHeader.tsx index a6add0ee0..1e26c42cb 100644 --- a/src/components/ui/agent/sidebar/AgentSidebarHeader.tsx +++ b/src/components/ui/agent/sidebar/AgentSidebarHeader.tsx @@ -5,6 +5,7 @@ import { isDev } from "@/utils/env"; import { IconButton } from "@/components/ui/actions/icon-button/IconButton"; import { Icon } from "@/components/ui/media/icon/Icon"; import { Notch } from "@/components/ui/surfaces/notch/Notch"; +import { Dialog } from "@/components/ui/surfaces/dialog/Dialog"; import { useClickOutside } from "@/hooks/useClickOutside"; import type { AgentSidebarHeaderLabels, AgentSidebarHistoryGroup, ChatTab } from "./types"; @@ -29,8 +30,9 @@ export interface AgentSidebarHeaderProps { /** Enables the hover rename affordance on history rows. * Called with the trimmed title (1-200 chars) when the user commits an inline rename. */ onRenameConversation?: (id: string, title: string) => void; - /** Enables the hover delete affordance on history rows. Confirmation (if any) - * is the consumer's responsibility; the kit fires immediately. */ + /** Enables the hover delete affordance on history rows. Clicking it opens a + * destructive confirmation dialog first; this fires only once the user + * confirms (cancel/dismiss does nothing). */ onDeleteConversation?: (id: string) => void; /** Label overrides. All have EN defaults. */ labels?: AgentSidebarHeaderLabels; @@ -125,6 +127,11 @@ export function AgentSidebarHeader({ const [editValue, setEditValue] = useState(""); const editInputRef = useRef(null); + // Id of the conversation pending a delete confirmation; null = dialog closed. + // Deletion only fires once the user confirms, so a misclick on the row's + // trash icon can't silently remove a conversation. + const [deletingId, setDeletingId] = useState(null); + const calculateVisible = useCallback(() => { if (!tabsContainerRef.current) return; // clientWidth includes the strip's own pl-10 + pr-3 padding — subtract @@ -416,7 +423,7 @@ export function AgentSidebarHeader({