From 2aee24e0b084b443999dce68deb8caf94db0441c Mon Sep 17 00:00:00 2001 From: Matthew Robert Wesney <157447210+dovvnloading@users.noreply.github.com> Date: Tue, 29 Sep 2026 04:27:34 -0400 Subject: [PATCH 1/6] feat(frontend): keep error toasts until dismissed and show why a download failed Error toasts vanished after 4.5 seconds, could not be dismissed, and were sometimes the only place a failure reason appeared: a failed GGUF download told the form to "see the notification" after the notification was gone. - Errors now stay until dismissed and have a Dismiss button. Success and info toasts still expire and now pause while hovered or focused. - Errors are announced as alerts and everything else as status; the region no longer announces on top of its children. An identical error that is already showing is not stacked, and the stack is capped at five. - A toast can carry an action button and an onClose callback that runs when it goes away unused, which the next change builds on. - runModelJob hands the specific failure reason to its caller, and the GGUF download form shows it inline. Plan item: FE-21 Co-Authored-By: Claude Sonnet 5.5 --- .../src/app/App.modelDownloadFailure.test.tsx | 101 ++++++++++++++ frontend/src/app/App.tsx | 39 ++++-- frontend/src/app/ToastProvider.test.tsx | 97 +++++++++++++ frontend/src/app/ToastProvider.tsx | 63 +++++++-- .../src/features/models/ModelsPanel.test.tsx | 78 +++++++++++ frontend/src/features/models/ModelsPanel.tsx | 7 +- frontend/src/stores/useUiStore.test.ts | 99 +++++++++++++ frontend/src/stores/useUiStore.ts | 132 +++++++++++++++--- frontend/src/styles/tokens.css | 8 +- 9 files changed, 576 insertions(+), 48 deletions(-) create mode 100644 frontend/src/app/App.modelDownloadFailure.test.tsx create mode 100644 frontend/src/app/ToastProvider.test.tsx create mode 100644 frontend/src/features/models/ModelsPanel.test.tsx diff --git a/frontend/src/app/App.modelDownloadFailure.test.tsx b/frontend/src/app/App.modelDownloadFailure.test.tsx new file mode 100644 index 00000000..8521b404 --- /dev/null +++ b/frontend/src/app/App.modelDownloadFailure.test.tsx @@ -0,0 +1,101 @@ +import { render, screen, waitFor, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { App } from "./App"; +import { CortexApi } from "../api/client"; +import { ToastProvider } from "./ToastProvider"; + +const json = (body: unknown, status = 200) => new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, +}); + +describe("App model download failures", () => { + // The two lazy routes are loaded once up front so the first test's time + // covers what it does rather than a cold module graph (see App.test.tsx). + beforeAll(async () => { + await Promise.all([ + import("../features/chat/ChatPage"), + import("../features/settings/SettingsPanel"), + ]); + }, 120_000); + + afterEach(() => { + window.sessionStorage.clear(); + window.history.replaceState({}, "", "/"); + }); + + const workspace = (onDownload: (url: string, init?: RequestInit) => Response | null) => vi.fn(async (input, init) => { + const url = String(input); + const handled = onDownload(url, init); + if (handled) return handled; + if (url.endsWith("/system")) return json({ status: "ok", preview: true, session_required: true, started_at: "2026-07-21T18:00:00Z" }); + if (url.endsWith("/chat-groups")) return json([]); + if (url.endsWith("/chats")) return json([]); + if (url.endsWith("/memories")) return json({ memos: [] }); + if (url.endsWith("/settings")) return json({ settings: { models: { chat: null, title: null }, appearance: { theme: "dark" } } }); + if (url.endsWith("/models")) return json({ required_models: [], optional_models: [], installed_models: [], connection: { success: true, status: "connected", message: "Ready" } }); + return json({ detail: "Unexpected test route." }, 404); + }); + + const startDownload = async () => { + const user = userEvent.setup(); + await user.click(screen.getByRole("link", { name: "Settings" })); + await user.click(await screen.findByRole("button", { name: "System" }, { timeout: 10_000 })); + await user.type(screen.getByLabelText(/Repo id/), "vendor/demo-GGUF"); + await user.type(screen.getByLabelText(/File name/), "demo.Q4_K_M.gguf"); + await user.click(screen.getByRole("button", { name: /Download model/ })); + }; + + it("keeps the failure reason on the form after the job reports it", async () => { + // The reason used to appear only in a toast, while the form told the + // person to look at that toast. + window.sessionStorage.setItem("cortex.session.token", "local-session"); + const reason = "Checksum mismatch: the downloaded file is not the published one."; + const fetcher = workspace((url, init) => { + if (url.endsWith("/models/gguf/downloads") && init?.method === "POST") { + return json({ job_id: "gguf-job", kind: "gguf_download", status: "queued" }, 202); + } + if (url.endsWith("/jobs/gguf-job/events")) { + const event = { id: 1, job_id: "gguf-job", kind: "error", status: "failed", phase: null, data: { message: reason } }; + return new Response(`data: ${JSON.stringify(event)}\n\n`, { headers: { "Content-Type": "text/event-stream" } }); + } + return null; + }); + + render(); + expect(await screen.findByRole("heading", { name: "New thread" })).toBeVisible(); + await startDownload(); + + await waitFor(() => expect(screen.getAllByRole("alert").filter((node) => node.textContent?.includes(reason))).toHaveLength(2)); + const inline = screen.getAllByRole("alert").find((node) => node.classList.contains("field-error") && node.textContent?.includes(reason)); + expect(inline).toBeDefined(); + expect(inline).toHaveTextContent(`${reason} Check the details above and try again.`); + expect(inline).not.toHaveTextContent("notification"); + // The toast is dismissible rather than timed. + const toast = screen.getAllByRole("alert").find((node) => node.classList.contains("toast")); + expect(toast).toBeDefined(); + expect(within(toast as HTMLElement).getByRole("button", { name: "Dismiss notification" })).toBeVisible(); + }); + + it("keeps the reason when the download cannot be started", async () => { + window.sessionStorage.setItem("cortex.session.token", "local-session"); + const fetcher = workspace((url, init) => { + if (url.endsWith("/models/gguf/downloads") && init?.method === "POST") { + return json({ detail: "That host is not on the download allow-list." }, 422); + } + return null; + }); + + render(); + expect(await screen.findByRole("heading", { name: "New thread" })).toBeVisible(); + await startDownload(); + + const inline = await waitFor(() => { + const node = screen.getAllByRole("alert").find((candidate) => candidate.classList.contains("field-error") && candidate.textContent?.includes("allow-list")); + expect(node).toBeDefined(); + return node as HTMLElement; + }); + expect(inline).toHaveTextContent("That host is not on the download allow-list. Check the details above and try again."); + }); +}); diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx index 8069eecf..36d0a59e 100644 --- a/frontend/src/app/App.tsx +++ b/frontend/src/app/App.tsx @@ -693,12 +693,15 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS // `notifyOnSuccess`: callers that want their own, more specific success // message (e.g. "modelname downloaded and selected") suppress the generic // one here instead of showing both. + // `onFailure`: receives the specific reason a failed job reports, so a + // caller can show it next to the control that started the job as well as in + // the toast. const runModelJob = async ( accepted: JobAccepted, model = "local model inventory", - options: { checkOllamaConnection?: boolean; notifyOnSuccess?: boolean } = {}, + options: { checkOllamaConnection?: boolean; notifyOnSuccess?: boolean; onFailure?: (message: string) => void } = {}, ): Promise | null> => { - const { checkOllamaConnection = true, notifyOnSuccess = true } = options; + const { checkOllamaConnection = true, notifyOnSuccess = true, onFailure } = options; const generation = ++modelGenerationRef.current; const isCurrentModelJob = () => mountedRef.current && modelGenerationRef.current === generation; setModelBusy(true); @@ -729,13 +732,10 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS } if (terminalStatus === "failed" || terminalStatus === "cancelled") { - if (isCurrentModelJob()) { - notify( - failureMessage - ?? (terminalStatus === "cancelled" ? "Model operation was cancelled." : "Model operation failed."), - "error", - ); - } + const message = failureMessage + ?? (terminalStatus === "cancelled" ? "Model operation was cancelled." : "Model operation failed."); + onFailure?.(message); + if (isCurrentModelJob()) notify(message, "error"); return null; } if (terminalStatus !== "succeeded") { @@ -760,7 +760,9 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS } return completedData; } catch (error) { - if (isCurrentModelJob()) notify(apiMessage(error, "Model operation failed."), "error"); + const message = apiMessage(error, "Model operation failed."); + onFailure?.(message); + if (isCurrentModelJob()) notify(message, "error"); return null; } finally { if (isCurrentModelJob()) setModelBusy(false); @@ -783,14 +785,21 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS try { accepted = await api.downloadGGUFModel(request); } catch (error) { - notify(apiMessage(error, "Could not start the model download."), "error"); - throw error; + const message = apiMessage(error, "Could not start the model download."); + notify(message, "error"); + throw new Error(message, { cause: error }); } - const result = await runModelJob(accepted, label, { checkOllamaConnection: false, notifyOnSuccess: false }); + let failure: string | null = null; + const result = await runModelJob(accepted, label, { + checkOllamaConnection: false, + notifyOnSuccess: false, + onFailure: (message) => { failure = message; }, + }); const filename = result && typeof result.filename === "string" ? result.filename : null; if (!filename) { - // runModelJob already showed the specific failure reason as a toast. - throw new Error("Model download failed."); + // The toast already carries the specific reason, but it is the only + // place it would otherwise appear. The form shows it inline as well. + throw new Error(failure ?? "Model download failed."); } const selected = await chooseLocalModel(`gguf:${filename}`); if (!selected) { diff --git a/frontend/src/app/ToastProvider.test.tsx b/frontend/src/app/ToastProvider.test.tsx new file mode 100644 index 00000000..9036b6ee --- /dev/null +++ b/frontend/src/app/ToastProvider.test.tsx @@ -0,0 +1,97 @@ +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useUiStore } from "../stores/useUiStore"; +import { ToastProvider } from "./ToastProvider"; + +const notify = (...args: Parameters["notify"]>) => { + let id = 0; + act(() => { id = useUiStore.getState().notify(...args); }); + return id; +}; + +const advance = (ms: number) => act(() => { vi.advanceTimersByTime(ms); }); + +describe("ToastProvider", () => { + beforeEach(() => { + vi.useFakeTimers(); + useUiStore.setState({ toasts: [] }); + }); + + afterEach(() => { + act(() => { useUiStore.setState({ toasts: [] }); }); + vi.useRealTimers(); + }); + + it("keeps an error toast past ten seconds and removes it with its Dismiss button", () => { + render(

app

); + + notify("The download failed: the checksum did not match.", "error"); + advance(10_000); + + expect(screen.getByRole("alert")).toHaveTextContent("The download failed: the checksum did not match."); + fireEvent.click(screen.getByRole("button", { name: "Dismiss notification" })); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + + it("expires a success toast and announces it as a status without a dismiss control", () => { + render(

app

); + + notify("Chat renamed.", "success"); + expect(screen.getByRole("status")).toHaveTextContent("Chat renamed."); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Dismiss notification" })).not.toBeInTheDocument(); + + advance(4500); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("does not make the region itself a live region, so nothing is read twice", () => { + const { container } = render(

app

); + notify("Saved.", "success"); + + const region = container.ownerDocument.querySelector(".toast-region"); + expect(region).not.toBeNull(); + expect(region).not.toHaveAttribute("aria-live"); + expect(region).not.toHaveAttribute("aria-atomic"); + }); + + it("waits while a toast is hovered, then finishes its countdown", () => { + render(

app

); + + notify("Model ready.", "info"); + advance(4000); + fireEvent.mouseEnter(screen.getByRole("status")); + advance(30_000); + expect(screen.getByRole("status")).toBeVisible(); + + fireEvent.mouseLeave(screen.getByRole("status")); + advance(499); + expect(screen.getByRole("status")).toBeVisible(); + advance(1); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("pauses while keyboard focus is on the toast's action and resumes when it leaves", () => { + render(); + + notify("Chat deleted.", "success", { action: { label: "Undo", onAction: vi.fn() }, durationMs: 6000 }); + act(() => { screen.getByRole("button", { name: "Undo" }).focus(); }); + advance(60_000); + expect(screen.getByRole("status")).toBeVisible(); + + act(() => { screen.getByRole("button", { name: "elsewhere" }).focus(); }); + advance(6000); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("runs a toast's action from its button", () => { + const onAction = vi.fn(); + render(

app

); + + notify("Chat deleted.", "success", { action: { label: "Undo", onAction } }); + fireEvent.click(screen.getByRole("button", { name: "Undo" })); + + expect(onAction).toHaveBeenCalledOnce(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/app/ToastProvider.tsx b/frontend/src/app/ToastProvider.tsx index f3f0654b..0d80baf9 100644 --- a/frontend/src/app/ToastProvider.tsx +++ b/frontend/src/app/ToastProvider.tsx @@ -1,7 +1,7 @@ /* eslint-disable react-refresh/only-export-components */ -import { Check, Info, X } from "lucide-react"; -import type { ReactNode } from "react"; -import { useUiStore } from "../stores/useUiStore"; +import { AlertTriangle, Check, Info, X } from "lucide-react"; +import type { FocusEvent, ReactNode } from "react"; +import { useUiStore, type Toast } from "../stores/useUiStore"; export { useToast } from "../stores/useUiStore"; @@ -11,16 +11,55 @@ export function ToastProvider({ children }: { children: ReactNode }) { return ( <> {children} -
- {toasts.map((toast) => ( -
- {toast.kind === "success" &&
- ))} + {/* + Each toast is its own live region (an alert for a failure, a status for + the rest), so the region itself announces nothing and a toast is never + read twice. + */} +
+ {toasts.map((toast) => )}
); } + +function ToastItem({ toast }: { toast: Toast }) { + const dismissToast = useUiStore((state) => state.dismissToast); + const pauseToast = useUiStore((state) => state.pauseToast); + const resumeToast = useUiStore((state) => state.resumeToast); + const runToastAction = useUiStore((state) => state.runToastAction); + const { id } = toast; + const isError = toast.kind === "error"; + + // A toast that is being hovered or has keyboard focus inside it is being + // read or used, so its countdown waits. + const resumeUnlessFocusStaysInside = (event: FocusEvent) => { + if (!event.currentTarget.contains(event.relatedTarget)) resumeToast(id); + }; + + return ( +
pauseToast(id)} + onMouseLeave={() => resumeToast(id)} + onFocus={() => pauseToast(id)} + onBlur={resumeUnlessFocusStaysInside} + > + {toast.kind === "success" &&
+ ); +} diff --git a/frontend/src/features/models/ModelsPanel.test.tsx b/frontend/src/features/models/ModelsPanel.test.tsx new file mode 100644 index 00000000..94ece4f1 --- /dev/null +++ b/frontend/src/features/models/ModelsPanel.test.tsx @@ -0,0 +1,78 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi } from "vitest"; +import type { ModelDownloadRequest, ModelResponse } from "../../../../contracts/cortex-api"; +import { ModelsPanel } from "./ModelsPanel"; + +const models: ModelResponse = { + required_models: [], + optional_models: [], + installed_models: [], + models: [], + connection: { success: true, status: "connected", message: "Ready" }, +}; + +function renderPanel(onDownload: (request: ModelDownloadRequest) => Promise) { + return render( + Promise>().mockResolvedValue()} + llamacppStatus={{ state: "idle", binary_present: false, models_directory: "C:/models", models_directory_exists: true }} + gguf={{ directory: "", directoryDirty: false, onDirectoryChange: vi.fn(), onDownload, busy: false }} + />, + ); +} + +async function submitHuggingFaceDownload(user: ReturnType) { + await user.type(screen.getByLabelText(/Repo id/), "vendor/demo-GGUF"); + await user.type(screen.getByLabelText(/File name/), "demo.Q4_K_M.gguf"); + await user.click(screen.getByRole("button", { name: /Download model/ })); +} + +describe("ModelsPanel GGUF download form", () => { + it("shows the reason a download failed next to the form, not only in a notification", async () => { + // The reason used to reach the person only as a toast, and the form said + // "See the notification" after that toast had already gone. + const user = userEvent.setup(); + const onDownload = vi.fn<(request: ModelDownloadRequest) => Promise>() + .mockRejectedValue(new Error("The downloaded file did not match the published SHA-256 checksum")); + renderPanel(onDownload); + + await submitHuggingFaceDownload(user); + + const alert = await screen.findByText(/did not match the published SHA-256 checksum/); + expect(alert).toHaveTextContent("The downloaded file did not match the published SHA-256 checksum. Check the details above and try again."); + expect(alert).toHaveAttribute("role", "alert"); + expect(alert).not.toHaveTextContent("notification"); + expect(onDownload).toHaveBeenCalledWith({ source: "huggingface", repo_id: "vendor/demo-GGUF", filename: "demo.Q4_K_M.gguf" }); + expect(screen.getByLabelText(/Repo id/)).toHaveValue("vendor/demo-GGUF"); + }); + + it("falls back to a generic sentence when the rejection carries no message", async () => { + const user = userEvent.setup(); + renderPanel(vi.fn<(request: ModelDownloadRequest) => Promise>().mockRejectedValue(undefined)); + + await submitHuggingFaceDownload(user); + + expect(await screen.findByText("The download did not complete. Check the details above and try again.")).toHaveAttribute("role", "alert"); + }); + + it("clears the reason when the next download starts and empties the form when it succeeds", async () => { + const user = userEvent.setup(); + const onDownload = vi.fn<(request: ModelDownloadRequest) => Promise>() + .mockRejectedValueOnce(new Error("Connection reset")) + .mockResolvedValueOnce(); + renderPanel(onDownload); + + await submitHuggingFaceDownload(user); + expect(await screen.findByText(/Connection reset\./)).toBeVisible(); + + await user.click(screen.getByRole("button", { name: /Download model/ })); + + expect(screen.queryByText(/Connection reset/)).not.toBeInTheDocument(); + expect(screen.getByLabelText(/Repo id/)).toHaveValue(""); + }); +}); diff --git a/frontend/src/features/models/ModelsPanel.tsx b/frontend/src/features/models/ModelsPanel.tsx index d99124fd..1958072f 100644 --- a/frontend/src/features/models/ModelsPanel.tsx +++ b/frontend/src/features/models/ModelsPanel.tsx @@ -171,8 +171,11 @@ function GGUFDownloadForm({ onDownload, busy, directoryDirty }: { onDownload: (r setRepoId(""); setFilename(""); setUrl(""); - } catch { - setError("The download did not complete. See the notification for details, or check the details above and try again."); + } catch (failure) { + // App reports why the download failed (a checksum mismatch, a refused + // URL) in the rejection, so the reason stays here after any toast is gone. + const reason = failure instanceof Error && failure.message.trim() ? failure.message.trim() : "The download did not complete."; + setError(`${/[.!?]$/.test(reason) ? reason : `${reason}.`} Check the details above and try again.`); } }; diff --git a/frontend/src/stores/useUiStore.test.ts b/frontend/src/stores/useUiStore.test.ts index f9212c55..62fe5ddc 100644 --- a/frontend/src/stores/useUiStore.test.ts +++ b/frontend/src/stores/useUiStore.test.ts @@ -28,6 +28,105 @@ describe("useUiStore", () => { expect(useUiStore.getState().toasts).toEqual([]); }); + it("keeps an error toast until it is dismissed", () => { + // A failure reason is sometimes the only place it appears (a failed model + // download points the form at the notification), so it must outlive the + // time it takes to read a 240-character validation message. + const id = useUiStore.getState().notify("Checksum did not match the published file.", "error"); + + vi.advanceTimersByTime(10 * 60 * 1000); + expect(useUiStore.getState().toasts).toMatchObject([{ id, kind: "error" }]); + + useUiStore.getState().dismissToast(id); + expect(useUiStore.getState().toasts).toEqual([]); + }); + + it("lets a caller give an error a lifetime, and gives info toasts the default one", () => { + useUiStore.getState().notify("Transient failure.", "error", { durationMs: 1000 }); + useUiStore.getState().notify("Working on it.", "info"); + + vi.advanceTimersByTime(1000); + expect(useUiStore.getState().toasts.map((toast) => toast.message)).toEqual(["Working on it."]); + vi.advanceTimersByTime(3500); + expect(useUiStore.getState().toasts).toEqual([]); + }); + + it("does not stack an identical error while it is still showing", () => { + const first = useUiStore.getState().notify("Could not rename chat.", "error"); + const again = useUiStore.getState().notify("Could not rename chat.", "error"); + const other = useUiStore.getState().notify("Could not delete chat.", "error"); + + expect(again).toBe(first); + expect(other).not.toBe(first); + expect(useUiStore.getState().toasts.map((toast) => toast.message)).toEqual(["Could not rename chat.", "Could not delete chat."]); + + // Once dismissed, the same failure is worth showing again. + useUiStore.getState().dismissToast(first); + useUiStore.getState().notify("Could not rename chat.", "error"); + expect(useUiStore.getState().toasts.map((toast) => toast.message)).toEqual(["Could not delete chat.", "Could not rename chat."]); + }); + + it("holds a toast's countdown while it is paused and finishes the remainder on resume", () => { + const id = useUiStore.getState().notify("Saved.", "success"); + + vi.advanceTimersByTime(3000); + useUiStore.getState().pauseToast(id); + vi.advanceTimersByTime(60_000); + expect(useUiStore.getState().toasts).toHaveLength(1); + + useUiStore.getState().resumeToast(id); + vi.advanceTimersByTime(1499); + expect(useUiStore.getState().toasts).toHaveLength(1); + vi.advanceTimersByTime(1); + expect(useUiStore.getState().toasts).toEqual([]); + }); + + it("runs a toast's action, removes the toast, and does not report it as closed", () => { + const onAction = vi.fn(); + const onClose = vi.fn(); + const id = useUiStore.getState().notify("Chat deleted.", "success", { action: { label: "Undo", onAction }, onClose }); + + useUiStore.getState().runToastAction(id); + expect(onAction).toHaveBeenCalledOnce(); + expect(useUiStore.getState().toasts).toEqual([]); + + // Its countdown was cleared with it, so nothing fires later. + vi.advanceTimersByTime(60_000); + expect(onClose).not.toHaveBeenCalled(); + useUiStore.getState().runToastAction(id); + expect(onAction).toHaveBeenCalledOnce(); + }); + + it("reports a toast as closed exactly once when it times out or is dismissed", () => { + const onExpire = vi.fn(); + useUiStore.getState().notify("Chat deleted.", "success", { durationMs: 6000, onClose: onExpire }); + vi.advanceTimersByTime(5999); + expect(onExpire).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(onExpire).toHaveBeenCalledOnce(); + vi.advanceTimersByTime(60_000); + expect(onExpire).toHaveBeenCalledOnce(); + + const onDismiss = vi.fn(); + const id = useUiStore.getState().notify("Chat deleted.", "success", { durationMs: 6000, onClose: onDismiss }); + useUiStore.getState().dismissToast(id); + useUiStore.getState().dismissToast(id); + vi.advanceTimersByTime(60_000); + expect(onDismiss).toHaveBeenCalledOnce(); + }); + + it("caps the stack, closing the oldest toast to make room", () => { + const onClose = vi.fn(); + useUiStore.getState().notify("First", "error", { onClose }); + for (const message of ["Second", "Third", "Fourth", "Fifth"]) useUiStore.getState().notify(message, "error"); + expect(useUiStore.getState().toasts).toHaveLength(5); + expect(onClose).not.toHaveBeenCalled(); + + useUiStore.getState().notify("Sixth", "error"); + expect(useUiStore.getState().toasts.map((toast) => toast.message)).toEqual(["Second", "Third", "Fourth", "Fifth", "Sixth"]); + expect(onClose).toHaveBeenCalledOnce(); + }); + it("command palette and shortcuts dialog visibility toggle independently", () => { useUiStore.getState().setCommandPaletteOpen(true); expect(useUiStore.getState().commandPaletteOpen).toBe(true); diff --git a/frontend/src/stores/useUiStore.ts b/frontend/src/stores/useUiStore.ts index 3cd6e64c..e074f6d4 100644 --- a/frontend/src/stores/useUiStore.ts +++ b/frontend/src/stores/useUiStore.ts @@ -1,38 +1,134 @@ import { create } from "zustand"; export type ToastKind = "success" | "error" | "info"; -export type Toast = { id: number; kind: ToastKind; message: string }; +export type ToastAction = { label: string; onAction: () => void }; +export type Toast = { id: number; kind: ToastKind; message: string; action?: ToastAction }; +/** How long a success or info toast stays up. Errors stay until dismissed. */ const TOAST_LIFETIME_MS = 4500; +/** Errors accumulate now that they persist, so the stack is capped. */ +const MAX_TOASTS = 5; + +export type NotifyOptions = { + /** A button on the toast, for an outcome the person can still reverse. */ + action?: ToastAction; + /** + * How long the toast stays up. Defaults to a few seconds for success and + * info toasts, and to "until dismissed" for errors: a failure reason is + * sometimes the only place the reason appears, so it must not vanish while + * it is still being read. + */ + durationMs?: number; + /** + * Called once when the toast goes away without its action having been used + * (it timed out, was dismissed, or was pushed off the stack). This is what + * lets a deferred change, such as a delete that can be undone, commit at + * exactly the moment the person can no longer take it back. + */ + onClose?: () => void; +}; interface UiStoreState { toasts: Toast[]; commandPaletteOpen: boolean; shortcutsDialogOpen: boolean; - notify: (message: string, kind?: ToastKind) => void; + /** Returns the toast's id. An identical error that is already showing is not stacked again. */ + notify: (message: string, kind?: ToastKind, options?: NotifyOptions) => number; dismissToast: (id: number) => void; + /** Stop a toast's countdown while it is being read or used. No effect on toasts that do not expire. */ + pauseToast: (id: number) => void; + resumeToast: (id: number) => void; + /** Run a toast's action and remove it. The action replaces `onClose`, which is not called. */ + runToastAction: (id: number) => void; setCommandPaletteOpen: (open: boolean) => void; setShortcutsDialogOpen: (open: boolean) => void; } -export const useUiStore = create((set) => ({ - toasts: [], - commandPaletteOpen: false, - shortcutsDialogOpen: false, - notify: (message, kind = "info") => { - const id = Date.now() + Math.random(); - set((state) => ({ toasts: [...state.toasts, { id, kind, message }] })); - window.setTimeout(() => { - set((state) => ({ toasts: state.toasts.filter((toast) => toast.id !== id) })); - }, TOAST_LIFETIME_MS); - }, - dismissToast: (id) => set((state) => ({ toasts: state.toasts.filter((toast) => toast.id !== id) })), - setCommandPaletteOpen: (commandPaletteOpen) => set({ commandPaletteOpen }), - setShortcutsDialogOpen: (shortcutsDialogOpen) => set({ shortcutsDialogOpen }), -})); +type ToastTimer = { + handle: number | null; + remainingMs: number; + startedAt: number; +}; + +// Timers live beside the store rather than in it: they are not state anything +// renders, and each one must be cleared when its toast leaves. +const toastTimers = new Map(); +const toastCloseHandlers = new Map void>(); +let nextToastId = 1; + +function clearToastTimer(id: number): void { + const timer = toastTimers.get(id); + if (timer?.handle != null) window.clearTimeout(timer.handle); + toastTimers.delete(id); +} + +function takeCloseHandler(id: number): (() => void) | undefined { + const handler = toastCloseHandlers.get(id); + toastCloseHandlers.delete(id); + return handler; +} + +export const useUiStore = create((set, get) => { + /** Remove a toast and, unless its action ran, tell its owner it is gone. */ + const closeToast = (id: number, runCloseHandler: boolean): void => { + clearToastTimer(id); + const onClose = takeCloseHandler(id); + if (!get().toasts.some((toast) => toast.id === id)) return; + set((state) => ({ toasts: state.toasts.filter((toast) => toast.id !== id) })); + if (runCloseHandler) onClose?.(); + }; + + const armToastTimer = (id: number, remainingMs: number): void => { + const handle = window.setTimeout(() => closeToast(id, true), remainingMs); + toastTimers.set(id, { handle, remainingMs, startedAt: Date.now() }); + }; + + return { + toasts: [], + commandPaletteOpen: false, + shortcutsDialogOpen: false, + notify: (message, kind = "info", options = {}) => { + const { action, onClose } = options; + if (kind === "error" && !action) { + const duplicate = get().toasts.find((toast) => toast.kind === "error" && !toast.action && toast.message === message); + if (duplicate) return duplicate.id; + } + const id = nextToastId++; + // Past the cap the oldest toast makes room. It counts as closed, so a + // pending undo commits rather than being left with no way to be used. + const overflow = get().toasts.length + 1 - MAX_TOASTS; + for (const evicted of get().toasts.slice(0, Math.max(0, overflow))) closeToast(evicted.id, true); + if (onClose) toastCloseHandlers.set(id, onClose); + set((state) => ({ toasts: [...state.toasts, action ? { id, kind, message, action } : { id, kind, message }] })); + const lifetime = options.durationMs ?? (kind === "error" ? null : TOAST_LIFETIME_MS); + if (lifetime !== null) armToastTimer(id, lifetime); + return id; + }, + dismissToast: (id) => closeToast(id, true), + pauseToast: (id) => { + const timer = toastTimers.get(id); + if (!timer || timer.handle === null) return; + window.clearTimeout(timer.handle); + timer.remainingMs = Math.max(0, timer.remainingMs - (Date.now() - timer.startedAt)); + timer.handle = null; + }, + resumeToast: (id) => { + const timer = toastTimers.get(id); + if (!timer || timer.handle !== null) return; + armToastTimer(id, timer.remainingMs); + }, + runToastAction: (id) => { + const action = get().toasts.find((toast) => toast.id === id)?.action; + closeToast(id, false); + action?.onAction(); + }, + setCommandPaletteOpen: (commandPaletteOpen) => set({ commandPaletteOpen }), + setShortcutsDialogOpen: (shortcutsDialogOpen) => set({ shortcutsDialogOpen }), + }; +}); /** Thin wrapper preserving the pre-Zustand useToast() API so call sites don't change. */ -export function useToast(): { notify: (message: string, kind?: ToastKind) => void } { +export function useToast(): { notify: (message: string, kind?: ToastKind, options?: NotifyOptions) => number } { const notify = useUiStore((state) => state.notify); return { notify }; } diff --git a/frontend/src/styles/tokens.css b/frontend/src/styles/tokens.css index 252b5e30..a6f491b4 100644 --- a/frontend/src/styles/tokens.css +++ b/frontend/src/styles/tokens.css @@ -1002,10 +1002,16 @@ input[type="range"] { width: 100%; accent-color: var(--accent); } @keyframes caret-pulse { 0%, 100% { opacity: 0.62; } 50% { opacity: 0.12; } } @keyframes thinking-dot { 0%, 80%, 100% { opacity: 0.35; transform: scale(0.75); } 40% { opacity: 1; transform: scale(1); } } .toast-region { position: fixed; z-index: 100; right: 20px; bottom: 20px; display: grid; gap: 9px; width: min(360px, calc(100vw - 40px)); } -.toast { display: flex; align-items: center; gap: 9px; padding: 13px 14px; border: 1px solid var(--line-strong); border-left-width: 3px; border-radius: 6px; background: var(--surface-elevated); box-shadow: var(--shadow-md); color: var(--text); font-size: 0.82rem; } +.toast { display: flex; align-items: flex-start; gap: 9px; padding: 13px 14px; border: 1px solid var(--line-strong); border-left-width: 3px; border-radius: 6px; background: var(--surface-elevated); box-shadow: var(--shadow-md); color: var(--text); font-size: 0.82rem; } .toast-success { color: var(--success); } .toast-error { color: var(--danger); } .toast-info { color: var(--accent); } +.toast > svg { flex: 0 0 auto; margin-top: 2px; } +.toast-message { flex: 1 1 auto; min-width: 0; line-height: 1.4; overflow-wrap: anywhere; } +.toast-action, .toast-dismiss { flex: 0 0 auto; border: 0; border-radius: 5px; background: transparent; color: var(--text); font: inherit; cursor: pointer; } +.toast-action { padding: 2px 8px; font-weight: 700; text-decoration: underline; text-underline-offset: 3px; } +.toast-dismiss { display: grid; place-items: center; width: 22px; height: 22px; padding: 0; color: var(--text-muted); } +.toast-action:hover, .toast-dismiss:hover { background: var(--surface-hover); } /* * .dialog self-centers via fixed positioning rather than relying on * .dialog-backdrop as a centering flex/grid parent, because Base UI's From eda60fecb04819e4a212d3ba3db89c169cac01e4 Mon Sep 17 00:00:00 2001 From: Matthew Robert Wesney <157447210+dovvnloading@users.noreply.github.com> Date: Tue, 29 Sep 2026 04:28:22 -0400 Subject: [PATCH 2/6] feat(frontend): delete a chat with one confirmation and an Undo Deleting a chat meant typing its exact title (quotes and all, for an "Untitled chat") and then had no way back. That is production-database friction for something a person does to tidy a sidebar. - The dialog is now a plain confirmation naming the chat. - Confirming hides the chat at once (it is filtered out of the sidebar, the command palette and the fallback choice) and shows "Chat deleted" with an Undo for six seconds. The delete request is sent only when that window closes. Undo puts the chat back exactly as it was and returns to it if the person is still where deleting it left them. - A delete that fails when the window closes brings the chat back and reports the reason in a toast that stays until dismissed. - The request is not sent if the app closes inside the window, so the chat is still there on the next launch rather than lost. Plan item: FE-24 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/app/App.chatDelete.test.tsx | 185 ++++++++++++++++++ frontend/src/app/App.tsx | 79 ++++++-- frontend/src/features/shell/AppShell.test.tsx | 56 ++++-- frontend/src/features/shell/AppShell.tsx | 34 ++-- frontend/src/styles/tokens.css | 4 +- 5 files changed, 302 insertions(+), 56 deletions(-) create mode 100644 frontend/src/app/App.chatDelete.test.tsx diff --git a/frontend/src/app/App.chatDelete.test.tsx b/frontend/src/app/App.chatDelete.test.tsx new file mode 100644 index 00000000..14338deb --- /dev/null +++ b/frontend/src/app/App.chatDelete.test.tsx @@ -0,0 +1,185 @@ +import { act, fireEvent, render, screen, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { App } from "./App"; +import { CortexApi } from "../api/client"; +import { useChatStore } from "../stores/useChatStore"; +import { useUiStore } from "../stores/useUiStore"; +import { ToastProvider } from "./ToastProvider"; + +const json = (body: unknown, status = 200) => new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, +}); + +const CHATS = [ + { id: "chat-a", title: "Alpha plan", timestamp: "2026-01-02T00:00:00Z" }, + { id: "chat-b", title: "Beta notes", timestamp: "2026-01-01T00:00:00Z" }, +]; + +/** The undo window in App.tsx. */ +const UNDO_WINDOW_MS = 6000; + +describe("App chat deletion with Undo", () => { + beforeAll(async () => { + await Promise.all([ + import("../features/chat/ChatPage"), + import("../features/settings/SettingsPanel"), + ]); + }, 120_000); + + afterEach(() => { + vi.useRealTimers(); + act(() => { useUiStore.setState({ toasts: [] }); }); + window.sessionStorage.clear(); + window.history.replaceState({}, "", "/"); + }); + + function workspace(options: { deleteStatus?: number } = {}) { + const deletes: string[] = []; + const fetcher = vi.fn(async (input, init) => { + const url = String(input); + const method = init?.method ?? "GET"; + const chatMatch = /\/chats\/([^/?]+)$/.exec(url); + if (chatMatch && method === "DELETE") { + deletes.push(decodeURIComponent(chatMatch[1])); + return options.deleteStatus && options.deleteStatus >= 400 + ? json({ detail: "The chat could not be deleted." }, options.deleteStatus) + : new Response(null, { status: 204 }); + } + if (chatMatch && method === "GET") { + const chat = CHATS.find((candidate) => candidate.id === chatMatch[1]); + return chat ? json({ ...chat, revision: 1, messages: [] }) : json({ detail: "Not found." }, 404); + } + if (url.endsWith("/system")) return json({ status: "ok", preview: true, session_required: true, started_at: "2026-07-21T18:00:00Z" }); + if (url.endsWith("/chat-groups")) return json([]); + if (url.endsWith("/chats")) return json(CHATS); + if (url.endsWith("/settings")) return json({ settings: { models: { chat: null, title: null }, appearance: { theme: "dark" } } }); + if (url.endsWith("/models")) return json({ required_models: [], optional_models: [], installed_models: [], connection: { success: true, status: "connected", message: "Ready" } }); + return json({ detail: "Unexpected test route." }, 404); + }); + return { fetcher, deletes }; + } + + async function openWorkspace(fetcher: typeof fetch, path = "/chat/new") { + window.sessionStorage.setItem("cortex.session.token", "local-session"); + window.history.replaceState({}, "", path); + render(); + expect(await screen.findByRole("button", { name: "Delete Alpha plan" }, { timeout: 10_000 })).toBeInTheDocument(); + } + + /** Confirm the dialog, then switch to fake timers so the undo window can be stepped through. */ + async function deleteChatAndControlTime(title: string) { + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: `Delete ${title}` })); + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }); + fireEvent.click(within(screen.getByRole("alertdialog")).getByRole("button", { name: "Delete chat" })); + await act(async () => { await vi.advanceTimersByTimeAsync(0); }); + } + + const passTime = (ms: number) => act(async () => { await vi.advanceTimersByTimeAsync(ms); }); + /** Let a request the last step started finish. Testing Library's waitFor cannot poll under fake timers. */ + const settle = () => passTime(50); + const toastRegion = () => within(document.querySelector(".toast-region") as HTMLElement); + + it("hides the chat at once, sends nothing while Undo is offered, and restores it on Undo", async () => { + const { fetcher, deletes } = workspace(); + await openWorkspace(fetcher); + + + await deleteChatAndControlTime("Alpha plan"); + + expect(screen.queryByRole("button", { name: "Delete Alpha plan" })).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Delete Beta notes" })).toBeInTheDocument(); + expect(toastRegion().getByText("Chat deleted.")).toBeVisible(); + expect(deletes).toEqual([]); + + fireEvent.click(toastRegion().getByRole("button", { name: "Undo" })); + await passTime(0); + + expect(screen.getByRole("button", { name: "Delete Alpha plan" })).toBeInTheDocument(); + expect(toastRegion().getByText("Chat restored.")).toBeVisible(); + expect(toastRegion().queryByText("Chat deleted.")).not.toBeInTheDocument(); + + // Long after the window would have closed, nothing was ever sent. + await passTime(60_000); + await settle(); + expect(deletes).toEqual([]); + expect(useChatStore.getState().chats.map((chat) => chat.id)).toEqual(["chat-a", "chat-b"]); + }); + + it("sends exactly one delete when the Undo window closes, and the chat stays gone", async () => { + const { fetcher, deletes } = workspace(); + await openWorkspace(fetcher); + + await deleteChatAndControlTime("Alpha plan"); + await passTime(UNDO_WINDOW_MS - 1); + expect(deletes).toEqual([]); + expect(toastRegion().getByRole("button", { name: "Undo" })).toBeInTheDocument(); + + await passTime(1); + await settle(); + expect(deletes).toEqual(["chat-a"]); + await passTime(60_000); + + expect(deletes).toEqual(["chat-a"]); + expect(screen.queryByRole("button", { name: "Undo" })).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Delete Alpha plan" })).not.toBeInTheDocument(); + expect(useChatStore.getState().chats.map((chat) => chat.id)).toEqual(["chat-b"]); + }); + + it("brings the chat back and says so when the delete fails at the end of the window", async () => { + const { fetcher, deletes } = workspace({ deleteStatus: 500 }); + await openWorkspace(fetcher); + + await deleteChatAndControlTime("Alpha plan"); + await passTime(UNDO_WINDOW_MS); + await settle(); + + expect(deletes).toEqual(["chat-a"]); + expect(screen.getByRole("button", { name: "Delete Alpha plan" })).toBeInTheDocument(); + expect(toastRegion().getByRole("alert")).toHaveTextContent("The chat could not be deleted."); + expect(useChatStore.getState().chats.map((chat) => chat.id)).toEqual(["chat-a", "chat-b"]); + }); + + it("keeps a chat hidden while its Undo window is open even if the list is refilled from the server", async () => { + const { fetcher, deletes } = workspace(); + await openWorkspace(fetcher); + + await deleteChatAndControlTime("Alpha plan"); + act(() => { useChatStore.getState().setChats([...CHATS]); }); + + expect(screen.queryByRole("button", { name: "Delete Alpha plan" })).not.toBeInTheDocument(); + expect(deletes).toEqual([]); + }); + + it("moves off an open chat when it is deleted and returns to it on Undo", async () => { + const { fetcher, deletes } = workspace(); + await openWorkspace(fetcher, "/chat/chat-a"); + expect(window.location.pathname).toBe("/chat/chat-a"); + + await deleteChatAndControlTime("Alpha plan"); + expect(window.location.pathname).toBe("/chat/chat-b"); + + fireEvent.click(toastRegion().getByRole("button", { name: "Undo" })); + await passTime(0); + + expect(window.location.pathname).toBe("/chat/chat-a"); + expect(deletes).toEqual([]); + }); + + it("does not pull the person back to a restored chat after they moved on", async () => { + const { fetcher } = workspace(); + await openWorkspace(fetcher, "/chat/chat-a"); + + await deleteChatAndControlTime("Alpha plan"); + expect(window.location.pathname).toBe("/chat/chat-b"); + act(() => { window.history.pushState({}, "", "/chat/new"); window.dispatchEvent(new PopStateEvent("popstate")); }); + + fireEvent.click(toastRegion().getByRole("button", { name: "Undo" })); + await passTime(0); + + expect(window.location.pathname).toBe("/chat/new"); + expect(screen.getByRole("button", { name: "Delete Alpha plan" })).toBeInTheDocument(); + }); +}); diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx index 36d0a59e..92843829 100644 --- a/frontend/src/app/App.tsx +++ b/frontend/src/app/App.tsx @@ -1,4 +1,4 @@ -import { Suspense, useCallback, useEffect, useLayoutEffect, useRef, useState } from "react"; +import { Suspense, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"; import type { ChatResponse, CortexSettings, ExecutionApprovalDecisionRequest, ExecutionTaskSummary, JobAccepted, JobStatusResponse, LlamaCppRuntimeStatus, MemoryResponse, ModelDownloadRequest, ModelResponse, SSEEvent, SystemResponse } from "../../../contracts/cortex-api"; import { CortexApi, @@ -34,6 +34,12 @@ import { resolveRuntimeAvailability } from "./runtimeAvailability"; type Props = { api?: CortexApi }; +/** + * How long "Chat deleted" offers Undo. The request is sent when this window + * closes, so it is also how long a deleted chat still exists on the backend. + */ +const CHAT_DELETE_UNDO_MS = 6000; + /** Statuses an execution task never leaves, so nothing more will change. */ const EXECUTION_TERMINAL_STATUSES = new Set(["succeeded", "failed", "cancelled"]); @@ -286,7 +292,23 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS // derived from it (and painted by useAppliedTheme), never copied into state. const theme = settings?.appearance?.theme ?? DEFAULT_THEME_PREFERENCE; useAppliedTheme(settings ? theme : null); - const chatsRef = useRef(chats); + // A chat that was just deleted is hidden here at once but only removed from + // the backend when its Undo window closes (see `deleteChat`). Everything that + // lists or picks a chat reads `visibleChats`, so a hidden chat stays hidden + // even if the store's list is refilled from the server or a finishing + // generation upserts it, and Undo puts it back exactly as it was. + const pendingChatDeletesRef = useRef(new Set()); + const [pendingChatDeleteIds, setPendingChatDeleteIds] = useState>(() => new Set()); + const visibleChats = useMemo( + () => chats.filter((chat) => !pendingChatDeleteIds.has(chat.id)), + [chats, pendingChatDeleteIds], + ); + const setChatDeletePending = useCallback((id: string, pending: boolean) => { + if (pending) pendingChatDeletesRef.current.add(id); + else pendingChatDeletesRef.current.delete(id); + setPendingChatDeleteIds(new Set(pendingChatDeletesRef.current)); + }, []); + const chatsRef = useRef(visibleChats); const executionTaskRefreshRef = useRef | null>(null); // These guards cover requests whose results are deliberately loaded out of // band. A response can outlive both the load that started it and this @@ -316,8 +338,8 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS }, [setModelBusy, setModelProgress]); useEffect(() => { - chatsRef.current = chats; - }, [chats]); + chatsRef.current = visibleChats; + }, [visibleChats]); const loadWorkspace = useCallback(async () => { const loadGeneration = ++workspaceLoadGenerationRef.current; @@ -527,19 +549,42 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS } }; + // Deleting hides the chat at once and asks the backend only when the Undo + // window closes. Closing the app inside that window loses nothing: the + // request was never sent, so the chat is simply still there next launch. const deleteChat = async (id: string): Promise => { - try { - await api.deleteChat(id); - const fallbackChatId = chatsRef.current.find((chat) => chat.id !== id)?.id ?? null; - setChats((current) => current.filter((chat) => chat.id !== id)); - setSettingsReturnChatId((current) => current === id ? fallbackChatId : current); - const currentRoute = parseAppRoute(window.location.pathname); - if (currentRoute.kind === "chat" && currentRoute.threadId === id) { - navigate(fallbackChatId ? chatPath(fallbackChatId) : "/chat/new", { replace: true }); + if (pendingChatDeletesRef.current.has(id)) return true; + const fallbackChatId = chatsRef.current.find((chat) => chat.id !== id)?.id ?? null; + const fallbackPath = fallbackChatId ? chatPath(fallbackChatId) : "/chat/new"; + const currentRoute = parseAppRoute(window.location.pathname); + const wasOpen = currentRoute.kind === "chat" && currentRoute.threadId === id; + setChatDeletePending(id, true); + setSettingsReturnChatId((current) => current === id ? fallbackChatId : current); + if (wasOpen) navigate(fallbackPath, { replace: true }); + + const commit = async () => { + try { + await api.deleteChat(id); + // Take the row out of the list before it is un-hidden, so it cannot flash back. + setChats((current) => current.filter((chat) => chat.id !== id)); + setChatDeletePending(id, false); + } catch (error) { + setChatDeletePending(id, false); + notify(apiMessage(error, "Could not delete chat."), "error"); } - notify("Chat deleted.", "success"); - return true; - } catch (error) { notify(apiMessage(error, "Could not delete chat."), "error"); return false; } + }; + const undo = () => { + setChatDeletePending(id, false); + // Only follow the chat back if the person is still where deleting it left them. + if (wasOpen && window.location.pathname === fallbackPath) navigate(chatPath(id), { replace: true }); + notify("Chat restored.", "success"); + }; + notify("Chat deleted.", "success", { + action: { label: "Undo", onAction: undo }, + durationMs: CHAT_DELETE_UNDO_MS, + onClose: () => void commit(), + }); + return true; }; const createGroup = async (name: string): Promise => { @@ -875,7 +920,7 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS <> {/* Above the route switch, so a generation keeps streaming into the store while Settings is open. */} - + Loading workspace...
}> {route.kind === "settings" ? @@ -883,7 +928,7 @@ function AuthenticatedWorkspace({ api, onSessionExpired }: { api: CortexApi; onS navigate("/chat/new")} diff --git a/frontend/src/features/shell/AppShell.test.tsx b/frontend/src/features/shell/AppShell.test.tsx index 5717a689..53fc1d65 100644 --- a/frontend/src/features/shell/AppShell.test.tsx +++ b/frontend/src/features/shell/AppShell.test.tsx @@ -1,4 +1,4 @@ -import { render, screen } from "@testing-library/react"; +import { render, screen, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, describe, expect, it, vi } from "vitest"; import type { ChatGroup, ChatSummary, ModelResponse } from "../../../../contracts/cortex-api"; @@ -87,7 +87,7 @@ describe("AppShell", () => { expect(window.location.pathname).toBe("/settings"); }); - it("requires the exact chat title before permanent deletion", async () => { + it("confirms a chat delete in one step, without typing the title", async () => { const user = userEvent.setup(); const chat: ChatSummary = { id: "chat-1", title: "Quarterly planning", timestamp: "2026-01-01T00:00:00Z" }; const onDeleteChat = vi.fn<(id: string) => Promise>().mockResolvedValue(); @@ -96,18 +96,29 @@ describe("AppShell", () => { await user.click(screen.getByRole("button", { name: "Delete Quarterly planning" })); - expect(screen.getByRole("alertdialog")).toHaveTextContent("Deleted chats cannot be recovered."); - const confirm = screen.getByRole("button", { name: "Delete permanently" }); - const verifier = screen.getByRole("textbox", { name: /Quarterly planning/ }); - expect(confirm).toBeDisabled(); + const dialog = screen.getByRole("alertdialog"); + expect(dialog).toHaveTextContent("You can undo it for a few seconds afterwards."); + expect(dialog).toHaveTextContent("Quarterly planning"); + expect(dialog).not.toHaveTextContent("cannot be recovered"); + expect(within(dialog).queryByRole("textbox")).not.toBeInTheDocument(); + expect(onDeleteChat).not.toHaveBeenCalled(); - await user.type(verifier, "Quarterly plan"); - expect(confirm).toBeDisabled(); - await user.type(verifier, "ning"); - expect(confirm).toBeEnabled(); - await user.click(confirm); + await user.click(within(dialog).getByRole("button", { name: "Delete chat" })); - expect(onDeleteChat).toHaveBeenCalledWith(chat.id); + expect(onDeleteChat).toHaveBeenCalledExactlyOnceWith(chat.id); + expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument(); + }); + + it("leaves the chat alone when the delete is cancelled", async () => { + const user = userEvent.setup(); + const chat: ChatSummary = { id: "chat-1", title: "Quarterly planning", timestamp: "2026-01-01T00:00:00Z" }; + const onDeleteChat = vi.fn<(id: string) => Promise>().mockResolvedValue(); + + renderShell({ chats: [chat], onDeleteChat, onOpenSettings: vi.fn() }); + await user.click(screen.getByRole("button", { name: "Delete Quarterly planning" })); + await user.click(screen.getByRole("button", { name: "Keep chat" })); + + expect(onDeleteChat).not.toHaveBeenCalled(); expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument(); }); @@ -128,20 +139,31 @@ describe("AppShell", () => { expect(field).toHaveValue("Updated planning"); }); - it("keeps the delete dialog and confirmation when deletion fails", async () => { + it("keeps the delete dialog open when deletion is refused", async () => { const user = userEvent.setup(); const chat: ChatSummary = { id: "chat-1", title: "Quarterly planning", timestamp: "2026-01-01T00:00:00Z" }; const onDeleteChat = vi.fn<(id: string) => Promise>().mockResolvedValue(false); renderShell({ chats: [chat], onDeleteChat, onOpenSettings: vi.fn() }); await user.click(screen.getByRole("button", { name: "Delete Quarterly planning" })); - const verifier = screen.getByRole("textbox", { name: /Quarterly planning/ }); - await user.type(verifier, "Quarterly planning"); - await user.click(screen.getByRole("button", { name: "Delete permanently" })); + await user.click(screen.getByRole("button", { name: "Delete chat" })); expect(onDeleteChat).toHaveBeenCalledWith(chat.id); expect(screen.getByRole("alertdialog")).toBeVisible(); - expect(verifier).toHaveValue("Quarterly planning"); + expect(screen.getByRole("button", { name: "Delete chat" })).toBeEnabled(); + }); + + it("keeps the delete dialog open when the delete callback rejects", async () => { + const user = userEvent.setup(); + const chat: ChatSummary = { id: "chat-1", title: "Quarterly planning", timestamp: "2026-01-01T00:00:00Z" }; + const onDeleteChat = vi.fn<(id: string) => Promise>().mockRejectedValue(new Error("offline")); + + renderShell({ chats: [chat], onDeleteChat, onOpenSettings: vi.fn() }); + await user.click(screen.getByRole("button", { name: "Delete Quarterly planning" })); + await user.click(screen.getByRole("button", { name: "Delete chat" })); + + expect(screen.getByRole("alertdialog")).toBeVisible(); + expect(screen.getByRole("button", { name: "Delete chat" })).toBeEnabled(); }); it("filters the thread list by title as the user types a search query", async () => { diff --git a/frontend/src/features/shell/AppShell.tsx b/frontend/src/features/shell/AppShell.tsx index 9105e9de..49405506 100644 --- a/frontend/src/features/shell/AppShell.tsx +++ b/frontend/src/features/shell/AppShell.tsx @@ -233,18 +233,22 @@ function RenameDialog({ chat, onClose, onSave }: { chat: ChatSummary; onClose: ( ); } +/** + * A plain confirmation, not a type-the-title gauntlet: the delete can be undone + * for a few seconds afterwards (see `deleteChat` in App), so the friction a + * production database deserves would be out of proportion to a chat. + */ function DeleteChatDialog({ chat, onClose, onConfirm }: { chat: ChatSummary; onClose: () => void; onConfirm: () => Promise }) { - const [confirmation, setConfirmation] = useState(""); const [busy, setBusy] = useState(false); const title = displayChatTitle(chat.title); - const confirmed = confirmation.trim() === title; - const submit = async (event: FormEvent) => { - event.preventDefault(); - if (!confirmed || busy) return; + const confirm = async () => { + if (busy) return; setBusy(true); try { await onConfirm(); + } catch { + // Preserve the dialog on failure; the workspace callback reports it. } finally { setBusy(false); } @@ -255,22 +259,14 @@ function DeleteChatDialog({ chat, onClose, onConfirm }: { chat: ChatSummary; onC
-
-

PERMANENT ACTION

- Delete this chat? -
+ Delete this chat?
- This permanently removes the conversation and all of its messages. Deleted chats cannot be recovered. + This removes the conversation and all of its messages. You can undo it for a few seconds afterwards.
Chat to delete{title}
-
- -
- - -
-
+
+ + +
); diff --git a/frontend/src/styles/tokens.css b/frontend/src/styles/tokens.css index a6f491b4..705e6c3f 100644 --- a/frontend/src/styles/tokens.css +++ b/frontend/src/styles/tokens.css @@ -1041,15 +1041,13 @@ kbd { display: inline-block; border: 1px solid var(--line-strong); border-radius .command-palette-item svg { flex: 0 0 auto; color: var(--text-faint); } .command-palette-current { margin-left: auto; color: var(--text-faint); font-size: var(--text-2xs); } .delete-dialog { width: min(100%, 480px); } -.delete-dialog-heading { display: flex; align-items: flex-start; gap: 12px; } -.delete-dialog-heading .eyebrow { margin-bottom: 4px; color: var(--danger); } +.delete-dialog-heading { display: flex; align-items: center; gap: 12px; margin-bottom: 14px; } .delete-dialog-heading h2 { margin-bottom: 0; } .delete-dialog-icon { display: grid; flex: 0 0 36px; place-items: center; width: 36px; height: 36px; border: 1px solid color-mix(in srgb, var(--danger) 35%, transparent); border-radius: 5px; background: var(--danger-soft); color: var(--danger); } .delete-dialog-description { margin: 0 0 17px; color: var(--text-muted); font-size: 0.86rem; line-height: 1.55; } .delete-dialog-target { display: grid; gap: 4px; margin-bottom: 18px; padding: 12px; border: 1px solid color-mix(in srgb, var(--danger) 25%, var(--line)); border-radius: 5px; background: var(--danger-soft); } .delete-dialog-target span { color: var(--text-faint); font-size: 0.72rem; font-weight: 700; letter-spacing: 0.08em; text-transform: uppercase; } .delete-dialog-target strong { overflow: hidden; color: var(--text); font-size: 0.86rem; text-overflow: ellipsis; white-space: nowrap; } -.delete-confirm-title { color: var(--text); font-family: "Cascadia Code", "SFMono-Regular", Consolas, monospace; font-size: 0.78rem; } @media (max-width: 920px) { .settings-dialog { width: min(100%, 860px); } From 9b27c516eb2f6735b68160700af7c351efd085c1 Mon Sep 17 00:00:00 2001 From: Matthew Robert Wesney <157447210+dovvnloading@users.noreply.github.com> Date: Tue, 29 Sep 2026 04:28:33 -0400 Subject: [PATCH 3/6] fix(frontend): show when a code-task approval expires A code task waiting for approval showed only "Approval needed", with no hint that the approval window was running out. A person who read the source and came back after it closed clicked "Allow once" and got a failure with no explanation, and an expired approval showed as a plain "Cancelled". - A pending approval shows "expires in N min" (or "under a minute", or "expiring now"), recomputed every 30 seconds and once more at the deadline. - An expired approval shows an Expired chip and a note that the window closed before the task was allowed and that it did not run, with no approval actions and no source review to open (nothing is left to decide). Other task kinds say "Expired" instead of "Cancelled" too, and the tray announcement says so. - The backend remains the authority on expiry: an approval that has run out locally but not yet been marked expired keeps its actions, and the server refuses them. Plan item: FE-29 Co-Authored-By: Claude Sonnet 5.5 --- .../features/shell/ExecutionTaskTray.test.tsx | 108 +++++++++++++++++- .../src/features/shell/ExecutionTaskTray.tsx | 72 ++++++++++-- frontend/src/styles/tokens.css | 1 + 3 files changed, 170 insertions(+), 11 deletions(-) diff --git a/frontend/src/features/shell/ExecutionTaskTray.test.tsx b/frontend/src/features/shell/ExecutionTaskTray.test.tsx index fc01a8bc..b80596e3 100644 --- a/frontend/src/features/shell/ExecutionTaskTray.test.tsx +++ b/frontend/src/features/shell/ExecutionTaskTray.test.tsx @@ -1,6 +1,6 @@ -import { render, screen, waitFor } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import type { ExecutionTaskSummary } from "../../../../contracts/cortex-api"; import { ExecutionTaskTray } from "./ExecutionTaskTray"; @@ -313,4 +313,108 @@ describe("ExecutionTaskTray", () => { expect(screen.getByRole("button", { name: "Allow background task code-1 once" })).toBeDisabled(); expect(screen.getByRole("button", { name: "Deny background task code-1" })).toBeEnabled(); }); + + describe("approval window", () => { + const NOW = new Date("2026-07-21T18:00:00Z"); + const pendingCode = (expiresAt: string | null): ExecutionTaskSummary => ({ + ...codeTask, + status: "queued", + approval_state: "pending", + approval_expires_at: expiresAt, + result: null, + }); + const advance = (ms: number) => act(() => { vi.advanceTimersByTime(ms); }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("shows how long a pending code approval has left and recomputes it every 30 seconds", () => { + vi.useFakeTimers(); + vi.setSystemTime(NOW); + render(); + + expect(screen.getByText("Approval needed · expires in 30 min")).toBeVisible(); + + // Nothing changes between ticks, and the first tick (29.5 minutes left) still rounds up to 30. + advance(30_000); + expect(screen.getByText("Approval needed · expires in 30 min")).toBeVisible(); + advance(4 * 60_000 + 30_000); + expect(screen.getByText("Approval needed · expires in 25 min")).toBeVisible(); + }); + + it("says under a minute in the last minute and expiring now at the deadline", () => { + vi.useFakeTimers(); + vi.setSystemTime(NOW); + // 105 s out: the deadline falls between the 90 s and 120 s ticks. + render(); + expect(screen.getByText("Approval needed · expires in 2 min")).toBeVisible(); + + advance(91_000); + expect(screen.getByText("Approval needed · expires in under a minute")).toBeVisible(); + + // The deadline itself wakes the display; it does not wait for the next tick. + advance(14_000 + 60); + expect(screen.getByText("Approval needed · expiring now")).toBeVisible(); + }); + + it("keeps the approval actions while the window is open and leaves the status plain without an expiry", () => { + render(); + + expect(screen.getByText("Approval needed")).toBeVisible(); + expect(screen.queryByText(/expires|expiring/)).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Deny background task code-1" })).toBeEnabled(); + }); + + it("ignores an expiry it cannot read", () => { + render(); + + expect(screen.getByText("Approval needed")).toBeVisible(); + expect(screen.queryByText(/expires|expiring/)).not.toBeInTheDocument(); + }); + + it("marks a code approval that expired, explains it, and offers no way to decide it", () => { + render( + , + ); + + expect(screen.getByText("Expired", { selector: ".execution-task-state" })).toBeVisible(); + expect(screen.getByText("Expired", { selector: ".execution-task-code-title span" })).toBeVisible(); + expect(screen.getByRole("note")).toHaveTextContent("The approval window closed before this task was allowed, so it did not run. Ask for it again to run it."); + expect(screen.queryByText("Cancelled")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /Allow background task|Deny background task/ })).not.toBeInTheDocument(); + expect(screen.queryByText("Review generated source")).not.toBeInTheDocument(); + expect(screen.getByRole("status")).toHaveTextContent("The latest task's approval expired before it was decided."); + }); + + it("calls an expired approval expired, not cancelled, for other task kinds too", () => { + render( + , + ); + + expect(screen.getByText("Expired")).toBeVisible(); + expect(screen.queryByText("Cancelled")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /Allow background task|Deny background task/ })).not.toBeInTheDocument(); + }); + + it("still calls a task the person denied cancelled", () => { + render( + , + ); + + expect(screen.getAllByText("Cancelled").length).toBeGreaterThan(0); + expect(screen.queryByText("Expired")).not.toBeInTheDocument(); + expect(screen.queryByRole("note")).not.toBeInTheDocument(); + }); + }); }); diff --git a/frontend/src/features/shell/ExecutionTaskTray.tsx b/frontend/src/features/shell/ExecutionTaskTray.tsx index 95ac0a7c..732b26e8 100644 --- a/frontend/src/features/shell/ExecutionTaskTray.tsx +++ b/frontend/src/features/shell/ExecutionTaskTray.tsx @@ -1,5 +1,5 @@ import { AlertTriangle, Check, ChevronDown, Code2, Download, X } from "lucide-react"; -import { useState } from "react"; +import { useEffect, useState } from "react"; import type { CodeExecutionSourceResponse, ExecutionApprovalDecisionRequest, ExecutionTaskSummary } from "../../../../contracts/cortex-api"; type ExecutionApprovalDecision = ExecutionApprovalDecisionRequest["decision"]; @@ -57,11 +57,13 @@ export function ExecutionTaskTray({ tasks, onCancel, onDecideApproval, onLoadCod ? `${pendingApprovals.length} task${pendingApprovals.length === 1 ? " requires" : "s require"} your approval.` : activeTasks.length ? `${activeTasks.length} local task${activeTasks.length === 1 ? " is" : "s are"} running.` - : latestTerminalTask?.status === "failed" - ? "The latest local task failed." - : latestTerminalTask?.status === "cancelled" - ? "The latest local task was cancelled." - : "The latest local task is complete."; + : latestTerminalTask?.approval_state === "expired" + ? "The latest task's approval expired before it was decided." + : latestTerminalTask?.status === "failed" + ? "The latest local task failed." + : latestTerminalTask?.status === "cancelled" + ? "The latest local task was cancelled." + : "The latest local task is complete."; const stop = async (jobId: string) => { if (!onCancel || cancelling.has(jobId)) return; @@ -191,7 +193,7 @@ export function ExecutionTaskTray({ tasks, onCancel, onDecideApproval, onLoadCod {showsWorking &&