From 34f87f6c92ce3d7ec30d41436618e4aff2e36ad7 Mon Sep 17 00:00:00 2001 From: Ryker_Feng <90562015+18062706139fcz@users.noreply.github.com> Date: Mon, 6 Jul 2026 22:14:44 +0800 Subject: [PATCH] fix(sidecar): panel button deletes the side chat instead of hiding it (#3961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sidecar): make panel button delete the side chat instead of hiding it The side chat panel's top-right button called `sidecar.close()`, which only hid the panel — duplicating the header `SidecarTrigger` toggle. Repurpose the button so the panel owns deletion and the header toggle owns hide/show. - When a conversation exists, the button shows a trash icon and opens a confirmation dialog before deleting via `useDeleteThread` (backend cascade). - In the draft state (references only, no thread yet) the header trigger is not rendered, so the button falls back to a plain close (X) that discards the draft without a confirm — there is nothing persisted to delete. Also fix a latent double-delete bug surfaced by the new dialog: `useDeleteThread` deletes through the LangGraph route (which drops the thread_meta row) and then calls `deleteLocalThreadData`, which hits the same gateway handler whose `require_existing=True` guard now 404s. Treat that 404 as idempotent success. The recent-chat delete used fire-and-forget `mutate`, so it swallowed this error; the sidecar's `mutateAsync` surfaced it as a toast. The e2e mock now mirrors the gateway ownership guard (second DELETE 404s once the thread is gone) so the regression stays covered. Adds tests for delete, draft-state close, and header-owned hide/show. * fix(sidecar): lock delete dialog while deletion is in flight The delete confirmation dialog disabled Cancel during an in-flight delete but still allowed dismissal via the overlay, Esc, and the built-in close (X). Those paths could imply the delete was cancelled when it was actually still running. Ignore dismissals and drop the close button while `isDeleting` so only the (disabled) Cancel path controls the dialog. --- .../workspace/sidecar/sidecar-panel.tsx | 134 ++++++++- frontend/src/core/i18n/locales/en-US.ts | 5 + frontend/src/core/i18n/locales/types.ts | 4 + frontend/src/core/i18n/locales/zh-CN.ts | 5 + frontend/src/core/threads/hooks.ts | 7 +- frontend/tests/e2e/sidecar-chat.spec.ts | 283 +++++++++++++++++- frontend/tests/e2e/utils/mock-api.ts | 16 + 7 files changed, 435 insertions(+), 19 deletions(-) diff --git a/frontend/src/components/workspace/sidecar/sidecar-panel.tsx b/frontend/src/components/workspace/sidecar/sidecar-panel.tsx index 009dd15b8..4ebf7948e 100644 --- a/frontend/src/components/workspace/sidecar/sidecar-panel.tsx +++ b/frontend/src/components/workspace/sidecar/sidecar-panel.tsx @@ -8,6 +8,7 @@ import { MessageSquareTextIcon, PaperclipIcon, RocketIcon, + Trash2Icon, XIcon, ZapIcon, } from "lucide-react"; @@ -35,6 +36,14 @@ import { type PromptInputMessage, } from "@/components/ai-elements/prompt-input"; import { Button } from "@/components/ui/button"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; import { DropdownMenuGroup, DropdownMenuLabel, @@ -50,6 +59,7 @@ import { } from "@/core/sidecar"; import { createSidecarThread } from "@/core/sidecar/api"; import { + useDeleteThread, useThreadStream, type ThreadStreamOptions, } from "@/core/threads/hooks"; @@ -137,6 +147,9 @@ export function SidecarPanel({ className }: { className?: string }) { const { models, tokenUsageEnabled } = useModels(); const [modelDialogOpen, setModelDialogOpen] = useState(false); const [creatingThread, setCreatingThread] = useState(false); + const [deleteDialogOpen, setDeleteDialogOpen] = useState(false); + const { mutateAsync: deleteThread, isPending: isDeleting } = + useDeleteThread(); const [queuedSubmit, setQueuedSubmit] = useState<{ message: PromptInputMessage; references: SidecarReference[]; @@ -437,6 +450,39 @@ export function SidecarPanel({ className }: { className?: string }) { ], ); + const discardDraftAndClose = useCallback(() => { + sidecar.clearActiveReferences(); + sidecar.setSidecarThreadId(null); + sidecar.close(); + }, [sidecar]); + + const handleDelete = useCallback(async () => { + const threadId = sidecar.sidecarThreadId; + // Guard: the trash button only opens this dialog once a thread exists, so a + // missing id here means the draft was cleared underneath us — just close. + if (!threadId) { + discardDraftAndClose(); + setDeleteDialogOpen(false); + return; + } + try { + await deleteThread({ threadId }); + discardDraftAndClose(); + setDeleteDialogOpen(false); + toast.success(t.sidecar.deleteSuccess); + } catch (error) { + toast.error( + error instanceof Error ? error.message : t.sidecar.deleteFailed, + ); + } + }, [ + deleteThread, + discardDraftAndClose, + sidecar.sidecarThreadId, + t.sidecar.deleteFailed, + t.sidecar.deleteSuccess, + ]); + return (
- - - + {hasSidecarThread ? ( + + + + ) : ( + // No conversation yet — nothing to delete, so this just discards the + // draft and closes the panel. A plain X (no confirm) keeps it light. + + + + )}
@@ -558,6 +623,55 @@ export function SidecarPanel({ className }: { className?: string }) {
+ + { + // While the delete is in flight the only way out is the (disabled) + // Cancel button, so ignore overlay/Esc/close-button dismissals that + // would otherwise hide the dialog and imply the delete was cancelled. + if (!open && isDeleting) { + return; + } + setDeleteDialogOpen(open); + }} + > + { + if (isDeleting) { + event.preventDefault(); + } + }} + onInteractOutside={(event) => { + if (isDeleting) { + event.preventDefault(); + } + }} + > + + {t.sidecar.delete} + {t.sidecar.deleteConfirm} + + + + + + + ); } diff --git a/frontend/src/core/i18n/locales/en-US.ts b/frontend/src/core/i18n/locales/en-US.ts index 37d995c84..28053ddee 100644 --- a/frontend/src/core/i18n/locales/en-US.ts +++ b/frontend/src/core/i18n/locales/en-US.ts @@ -452,6 +452,11 @@ export const enUS: Translations = { title: "Side chat", open: "Open side chat", close: "Close side chat", + delete: "Delete side chat", + deleteConfirm: + "Are you sure you want to delete this side chat? This action cannot be undone. To simply hide it, use the side chat toggle in the header instead.", + deleteSuccess: "Side chat deleted", + deleteFailed: "Failed to delete side chat.", addToConversation: "Add to conversation", askInSideChat: "Ask in side chat", reference: "Reference", diff --git a/frontend/src/core/i18n/locales/types.ts b/frontend/src/core/i18n/locales/types.ts index 7a272fa58..1940c13ab 100644 --- a/frontend/src/core/i18n/locales/types.ts +++ b/frontend/src/core/i18n/locales/types.ts @@ -362,6 +362,10 @@ export interface Translations { title: string; open: string; close: string; + delete: string; + deleteConfirm: string; + deleteSuccess: string; + deleteFailed: string; addToConversation: string; askInSideChat: string; reference: string; diff --git a/frontend/src/core/i18n/locales/zh-CN.ts b/frontend/src/core/i18n/locales/zh-CN.ts index d234f5c03..bdfdd8e9f 100644 --- a/frontend/src/core/i18n/locales/zh-CN.ts +++ b/frontend/src/core/i18n/locales/zh-CN.ts @@ -436,6 +436,11 @@ export const zhCN: Translations = { title: "侧边对话", open: "打开侧边对话", close: "关闭侧边对话", + delete: "删除侧边对话", + deleteConfirm: + "确定要删除该侧边对话吗？此操作不可撤销。如果只是想隐藏，请使用顶部的侧边对话开关。", + deleteSuccess: "侧边对话已删除", + deleteFailed: "删除侧边对话失败。", addToConversation: "添加到对话", askInSideChat: "在侧边聊天中提问", reference: "引用", diff --git a/frontend/src/core/threads/hooks.ts b/frontend/src/core/threads/hooks.ts index 2ac389564..b35aa1ad1 100644 --- a/frontend/src/core/threads/hooks.ts +++ b/frontend/src/core/threads/hooks.ts @@ -2044,7 +2044,12 @@ async function deleteLocalThreadData(threadId: string) { }, ); - if (!response.ok) { + // A 404 means the thread is already gone — the desired end state. The prior + // `apiClient.threads.delete` call hits the same gateway handler (nginx + // rewrites /api/langgraph/threads/* to /api/threads/*) and removes the + // thread_meta row, so this second delete's ownership guard 404s. Treat it as + // success to keep the delete idempotent. + if (!response.ok && response.status !== 404) { const error = await response .json() .catch(() => ({ detail: "Failed to delete local thread data." })); diff --git a/frontend/tests/e2e/sidecar-chat.spec.ts b/frontend/tests/e2e/sidecar-chat.spec.ts index cc8e7823a..b7c44e943 100644 --- a/frontend/tests/e2e/sidecar-chat.spec.ts +++ b/frontend/tests/e2e/sidecar-chat.spec.ts @@ -781,6 +781,10 @@ test.describe("Side chat", () => { await expect( page.getByRole("heading", { name: "Ask a follow-up" }), ).toBeVisible(); + // Draft state (no thread created yet): the header shows a plain close (X), + // not the destructive delete — there is nothing persisted to delete. + await expect(page.getByTestId("sidecar-close-button")).toBeVisible(); + await expect(page.getByTestId("sidecar-delete-button")).toBeHidden(); const sidecarReference = page.getByTestId("sidecar-reference-attachment"); const sidecarInputForm = page.locator("form").filter({ has: page.getByPlaceholder(/deeper follow-up/i), @@ -943,14 +947,8 @@ test.describe("Side chat", () => { await expectComposerHeightsEqual(page); await expect(page.getByTestId("sidecar-header-trigger")).toBeVisible(); - await page - .getByTestId("sidecar-panel") - .getByRole("button", { name: /close/i }) - .click(); - await expect(page.getByTestId("sidecar-panel")).toBeHidden(); - await expect(page.getByTestId("sidecar-header-trigger")).toBeVisible(); - await page.getByTestId("sidecar-header-trigger").click(); - await expect(page.getByTestId("sidecar-panel")).toBeVisible(); + // Hiding the side chat is owned by the header trigger; the panel's own + // button deletes the side chat instead of hiding it. await expect( page.getByTestId("sidecar-header-trigger"), ).toHaveAccessibleName("Close side chat"); @@ -1279,4 +1277,273 @@ test.describe("Side chat", () => { timeout: 10_000, }); }); + + test("deletes the side chat from the panel's own button", async ({ + page, + }) => { + mockLangGraphAPI(page, { + threads: [ + { + thread_id: MOCK_THREAD_ID, + title: "Main conversation", + messages: [ + { + type: "human", + id: "parent-human-1", + content: [{ type: "text", text: "Plan the feature." }], + }, + { + type: "ai", + id: "parent-ai-1", + content: "Build it as a side conversation.", + }, + ], + }, + { + thread_id: MOCK_SIDECAR_THREAD_ID, + title: "Restored side chat", + updated_at: "2025-01-01T00:00:01Z", + metadata: { + deerflow_sidecar: true, + parent_thread_id: MOCK_THREAD_ID, + sidecar_context_type: "referenced_message", + sidecar_context_label: "Selected assistant text #2", + sidecar_context_count: 1, + referenced_message_id: "parent-ai-1", + referenced_message_ids: ["parent-ai-1"], + referenced_message_role: "assistant", + referenced_message_roles: ["assistant"], + }, + messages: [ + { + type: "ai", + id: "side-ai-1", + content: "Restored side answer.", + }, + ], + }, + ], + }); + + await page.goto(`/workspace/chats/${MOCK_THREAD_ID}`); + await expect( + page.getByText("Build it as a side conversation."), + ).toBeVisible({ timeout: 15_000 }); + await expect(page.getByTestId("sidecar-header-trigger")).toBeVisible({ + timeout: 10_000, + }); + + // Open the side chat panel via the header trigger. + await page.getByTestId("sidecar-header-trigger").click(); + await expect(page.getByTestId("sidecar-panel")).toBeVisible(); + + // The panel's own button deletes the side chat (it does not merely hide it). + await page.getByTestId("sidecar-delete-button").click(); + await expect( + page.getByText("This action cannot be undone", { exact: false }), + ).toBeVisible(); + + const deleteRequestPromise = page.waitForRequest( + (request) => + request.method() === "DELETE" && + request.url().includes(`/threads/${MOCK_SIDECAR_THREAD_ID}`), + ); + await page.getByTestId("sidecar-delete-confirm-button").click(); + await deleteRequestPromise; + + // The panel closes and, because the sidecar thread is gone, the header + // trigger unmounts too — hiding is owned by the trigger, deleting by this. + await expect(page.getByTestId("sidecar-panel")).toBeHidden(); + await expect(page.getByTestId("sidecar-header-trigger")).toBeHidden({ + timeout: 10_000, + }); + }); + + test("keeps the delete dialog open while the delete is in flight", async ({ + page, + }) => { + mockLangGraphAPI(page, { + threads: [ + { + thread_id: MOCK_THREAD_ID, + title: "Main conversation", + messages: [ + { + type: "human", + id: "parent-human-1", + content: [{ type: "text", text: "Plan the feature." }], + }, + { + type: "ai", + id: "parent-ai-1", + content: "Build it as a side conversation.", + }, + ], + }, + { + thread_id: MOCK_SIDECAR_THREAD_ID, + title: "Restored side chat", + updated_at: "2025-01-01T00:00:01Z", + metadata: { + deerflow_sidecar: true, + parent_thread_id: MOCK_THREAD_ID, + sidecar_context_type: "referenced_message", + sidecar_context_label: "Selected assistant text #2", + sidecar_context_count: 1, + referenced_message_id: "parent-ai-1", + referenced_message_ids: ["parent-ai-1"], + referenced_message_role: "assistant", + referenced_message_roles: ["assistant"], + }, + messages: [ + { + type: "ai", + id: "side-ai-1", + content: "Restored side answer.", + }, + ], + }, + ], + }); + + // Hold the local-delete step open so the mutation stays pending while we + // probe every dismissal path Radix would otherwise honor. + let releaseDelete: (() => void) | undefined; + const deleteGate = new Promise((resolve) => { + releaseDelete = resolve; + }); + await page.route(/\/api\/threads\/[^/]+$/, async (route) => { + if (route.request().method() !== "DELETE") { + return route.fallback(); + } + await deleteGate; + return route.fallback(); + }); + + await page.goto(`/workspace/chats/${MOCK_THREAD_ID}`); + await expect( + page.getByText("Build it as a side conversation."), + ).toBeVisible({ timeout: 15_000 }); + await expect(page.getByTestId("sidecar-header-trigger")).toBeVisible({ + timeout: 10_000, + }); + + await page.getByTestId("sidecar-header-trigger").click(); + await expect(page.getByTestId("sidecar-panel")).toBeVisible(); + + await page.getByTestId("sidecar-delete-button").click(); + const dialogTitle = page.getByRole("heading", { name: "Delete side chat" }); + await expect(dialogTitle).toBeVisible(); + // The built-in Radix close (X) is present before the delete starts. + await expect( + page.locator('[data-slot="dialog-content"] [data-slot="dialog-close"]'), + ).toHaveCount(1); + + await page.getByTestId("sidecar-delete-confirm-button").click(); + + // Delete is in flight: confirm shows the loading label and Cancel disables. + await expect( + page.getByTestId("sidecar-delete-confirm-button"), + ).toBeDisabled(); + await expect(page.getByRole("button", { name: "Cancel" })).toBeDisabled(); + // The built-in close (X) is removed so it can't imply a cancel. + await expect( + page.locator('[data-slot="dialog-content"] [data-slot="dialog-close"]'), + ).toHaveCount(0); + + // Esc and overlay clicks must not dismiss the dialog mid-delete. + await page.keyboard.press("Escape"); + await expect(dialogTitle).toBeVisible(); + await page + .locator('[data-slot="dialog-overlay"]') + .click({ position: { x: 5, y: 5 } }); + await expect(dialogTitle).toBeVisible(); + + // Once the delete resolves the dialog closes and the panel goes away. + releaseDelete?.(); + await expect(dialogTitle).toBeHidden({ timeout: 10_000 }); + await expect(page.getByTestId("sidecar-panel")).toBeHidden(); + }); + + test("closes the draft side chat without deleting when no conversation exists", async ({ + page, + }) => { + mockLangGraphAPI(page, { + threads: [ + { + thread_id: MOCK_THREAD_ID, + title: "Main conversation", + messages: [ + { + type: "human", + id: "parent-human-1", + content: [{ type: "text", text: "Plan the feature." }], + }, + { + type: "ai", + id: "parent-ai-1", + content: "Build it as a side conversation.", + }, + ], + }, + ], + }); + await page.route("**/api/models", (route) => { + if (route.request().method() !== "GET") { + return route.fallback(); + } + return route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + models: [ + { + id: "deepseek-v4-pro", + name: "deepseek-v4-pro", + model: "deepseek-v4-pro", + display_name: "DeepSeek V4 Pro", + supports_thinking: true, + supports_reasoning_effort: true, + }, + ], + token_usage: { enabled: false }, + }), + }); + }); + + let deleteRequestFired = false; + page.on("request", (request) => { + if ( + request.method() === "DELETE" && + request.url().includes("/threads/") + ) { + deleteRequestFired = true; + } + }); + + await page.goto(`/workspace/chats/${MOCK_THREAD_ID}`); + await expect( + page.getByText("Build it as a side conversation."), + ).toBeVisible({ timeout: 15_000 }); + + // Open the side chat as a draft (references only, no thread created yet). + await selectTextAndClickToolbarButton( + page, + "Build it as a side conversation.", + "Ask in side chat", + ); + await expect(page.getByTestId("sidecar-panel")).toBeVisible(); + await expect( + page.getByTestId("sidecar-reference-attachment"), + ).toBeVisible(); + + // The draft has no persisted thread, so the header offers a plain close (X) + // instead of the destructive delete button. + await expect(page.getByTestId("sidecar-delete-button")).toBeHidden(); + await page.getByTestId("sidecar-close-button").click(); + + await expect(page.getByTestId("sidecar-panel")).toBeHidden(); + // No thread was ever created, so closing must not issue a DELETE request. + expect(deleteRequestFired).toBe(false); + }); }); diff --git a/frontend/tests/e2e/utils/mock-api.ts b/frontend/tests/e2e/utils/mock-api.ts index 8ae002850..ed1795aed 100644 --- a/frontend/tests/e2e/utils/mock-api.ts +++ b/frontend/tests/e2e/utils/mock-api.ts @@ -691,6 +691,22 @@ export function mockLangGraphAPI(page: Page, options?: MockAPIOptions) { void page.route(/\/api\/threads\/[^/]+$/, (route) => { if (route.request().method() === "DELETE") { + const threadId = decodeURIComponent( + new URL(route.request().url()).pathname.split("/").at(-1) ?? "", + ); + // Mirror the gateway's `require_existing=True` ownership guard: deleting + // an already-removed thread 404s. `useDeleteThread` first deletes via the + // LangGraph route (which drops the thread_meta row) and then hits this + // route, so this reproduces the real double-delete 404 the frontend must + // treat as idempotent success. + if (!threads.some((thread) => thread.thread_id === threadId)) { + return route.fulfill({ + status: 404, + contentType: "application/json", + body: JSON.stringify({ detail: `Thread ${threadId} not found` }), + }); + } + threads = threads.filter((thread) => thread.thread_id !== threadId); return route.fulfill({ status: 204, });