diff --git a/canvas/src/components/BatchActionBar.tsx b/canvas/src/components/BatchActionBar.tsx index 5421fbe2..f207e843 100644 --- a/canvas/src/components/BatchActionBar.tsx +++ b/canvas/src/components/BatchActionBar.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState } from "react"; +import { useEffect, useState } from "react"; import { createPortal } from "react-dom"; import { useCanvasStore } from "@/store/canvas"; import { ConfirmDialog } from "./ConfirmDialog"; @@ -17,14 +17,32 @@ export function BatchActionBar() { const [pending, setPending] = useState(null); const [busy, setBusy] = useState(false); + // Retry survivorship (QA pr-949 follow-up): when a batch action partial-fails + // and leaves a single survivor id, the default `count < 2` gate unmounts the + // bar and forces per-node context-menu retry. Track "active failure" so the + // bar stays mounted with a single item and the user can click the same action + // button to retry without re-selecting. Resets on success or Escape/clear. + const [hasFailedBatch, setHasFailedBatch] = useState(false); const count = selectedNodeIds.size; - if (count < 2) return null; + // Reset failure flag when the user clears selection (Escape / ✕ button). + useEffect(() => { + if (count === 0 && hasFailedBatch) setHasFailedBatch(false); + }, [count, hasFailedBatch]); + // Hide when nothing is selected. Hide for single-node selection UNLESS a + // partial-failure left a survivor awaiting retry. + if (count === 0) return null; + if (count < 2 && !hasFailedBatch) return null; + + // Message copy must handle both multi (count >= 2) and single-survivor retry + // (count === 1 && hasFailedBatch). Use a helper so we render singular form + // only when there is exactly one survivor to act on. + const plural = (n: number) => (n === 1 ? "workspace" : "workspaces"); const confirmMessages: Record, string> = { - restart: `Restart ${count} workspace${count !== 1 ? "s" : ""}? Each will briefly go offline while it restarts.`, - pause: `Pause ${count} workspace${count !== 1 ? "s" : ""}? Their containers will be stopped.`, - delete: `Permanently delete ${count} workspace${count !== 1 ? "s" : ""}? This cannot be undone.`, + restart: `Restart ${count} ${plural(count)}? Each will briefly go offline while it restarts.`, + pause: `Pause ${count} ${plural(count)}? Their containers will be stopped.`, + delete: `Permanently delete ${count} ${plural(count)}? This cannot be undone.`, }; const confirmLabels: Record, string> = { @@ -40,10 +58,18 @@ export function BatchActionBar() { if (pending === "restart") await batchRestart(); if (pending === "pause") await batchPause(); if (pending === "delete") await batchDelete(); - showToast(`${pending.charAt(0).toUpperCase() + pending.slice(1)} applied to ${count} workspace${count !== 1 ? "s" : ""}`, "success"); + // Reaching here means every store call fulfilled (the store throws on + // any partial failure), so `count` is the actual success count. + showToast(`${pending.charAt(0).toUpperCase() + pending.slice(1)} applied to ${count} ${plural(count)}`, "success"); + setHasFailedBatch(false); clearSelection(); - } catch { - showToast(`Batch ${pending} failed`, "error"); + } catch (e) { + const msg = e instanceof Error && e.message ? e.message : `Batch ${pending} failed`; + showToast(msg, "error"); + // Leave the failed IDs selected (the store preserved them) so the user + // can retry without re-selecting, and set hasFailedBatch so the bar + // stays mounted even if a single survivor remains. + setHasFailedBatch(true); } finally { setBusy(false); setPending(null); diff --git a/canvas/src/components/__tests__/BatchActionBar.test.tsx b/canvas/src/components/__tests__/BatchActionBar.test.tsx index 55eb786d..4dd9fef6 100644 --- a/canvas/src/components/__tests__/BatchActionBar.test.tsx +++ b/canvas/src/components/__tests__/BatchActionBar.test.tsx @@ -41,11 +41,16 @@ vi.mock("@/store/canvas", () => ({ ), })); -// Mock ConfirmDialog to just render buttons for testing +// Mock ConfirmDialog — renders title + message + buttons so tests can assert +// on dialog copy (singular/plural, retry prompts, etc.). Keeping the message +// accessible in the DOM keeps copy-regression tests cheap. (QA recommendation +// from pr-batch-bar-retry-survivor review, memo qa-batch-bar-retry-survivor- +// approve-2026-04-19.) vi.mock("@/components/ConfirmDialog", () => ({ ConfirmDialog: ({ open, title, + message, onConfirm, onCancel, }: { @@ -59,7 +64,8 @@ vi.mock("@/components/ConfirmDialog", () => ({ }) => open ? (
- {title} + {title} +

{message}

@@ -125,3 +131,137 @@ describe("BatchActionBar", () => { expect(toolbar.getAttribute("aria-label")).toBe("Batch workspace actions"); }); }); + +/** + * Retry-survivorship regression tests (QA pr-949 follow-up). + * + * When batchRestart / batchPause / batchDelete partial-fail, the store + * preserves the failed ids in selectedNodeIds and throws. BatchActionBar's + * catch handler now sets hasFailedBatch=true so the toolbar stays mounted + * even if only 1 survivor remains, letting the user click the same action + * button again to retry without re-selecting. + * + * Prior behavior: `if (count < 2) return null` unmounted the bar when a + * single survivor remained, forcing per-node context-menu retry. These + * tests pin the new behavior. + */ +describe("BatchActionBar — partial-failure retry survivorship", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockSelectedNodeIds = new Set(); + }); + + it("keeps bar mounted with '1 selected' when partial failure leaves one survivor", async () => { + // User starts with 2 selected — bar renders. + mockSelectedNodeIds = new Set(["ws-ok", "ws-fail"]); + // Simulate store's partial-failure behavior: throws after the fulfilled-branch mutations. + mockBatchDelete.mockImplementationOnce(() => + Promise.reject(new Error("1/2 delete(s) failed")) + ); + + const { rerender } = render(); + expect(screen.getByText("2 selected")).toBeTruthy(); + + // Open confirm dialog → click confirm → execute() runs, rejects, catch sets hasFailedBatch. + fireEvent.click(screen.getByText("Delete All")); + fireEvent.click(screen.getByText("confirm")); + // Let the microtask for the rejection and the subsequent setState run. + await new Promise((r) => setTimeout(r, 0)); + + // Store would have removed ws-ok and kept ws-fail — simulate the store's + // `selectedNodeIds` mutation by swapping the mock and re-rendering. + mockSelectedNodeIds = new Set(["ws-fail"]); + rerender(); + + // Bar MUST still render (hasFailedBatch=true from the catch), and the + // count badge MUST show the survivor count so the user can retry. + expect(screen.getByText("1 selected")).toBeTruthy(); + expect(screen.getByText("Delete All")).toBeTruthy(); + }); + + it("confirm dialog uses singular 'workspace' copy when only one survivor remains", async () => { + mockSelectedNodeIds = new Set(["ws-ok", "ws-fail"]); + mockBatchDelete.mockImplementationOnce(() => + Promise.reject(new Error("1/2 delete(s) failed")) + ); + const { rerender } = render(); + fireEvent.click(screen.getByText("Delete All")); + fireEvent.click(screen.getByText("confirm")); + await new Promise((r) => setTimeout(r, 0)); + + // After failure: 1 survivor remains. Open the confirm dialog again for retry. + mockSelectedNodeIds = new Set(["ws-fail"]); + rerender(); + // Dialog is closed after the prior execute() — re-open via click. + fireEvent.click(screen.getByText("Delete All")); + + // Count badge should show the survivor count. + expect(screen.getByText("1 selected")).toBeTruthy(); + // Dialog copy MUST be singular — plural(1) → "workspace" (not "workspaces"). + // This is the primary user-facing signal that the retry is scoped to one item. + const msg = screen.getByTestId("confirm-dialog-message"); + expect(msg.textContent).toBe( + "Permanently delete 1 workspace? This cannot be undone." + ); + }); + + it("bar unmounts once a single-survivor selection is cleared (hasFailedBatch resets)", async () => { + // Setup: simulate post-failure state with 1 survivor + hasFailedBatch=true. + mockSelectedNodeIds = new Set(["ws-ok", "ws-fail"]); + mockBatchDelete.mockImplementationOnce(() => + Promise.reject(new Error("1/2 delete(s) failed")) + ); + const { rerender, container } = render(); + fireEvent.click(screen.getByText("Delete All")); + fireEvent.click(screen.getByText("confirm")); + await new Promise((r) => setTimeout(r, 0)); + + mockSelectedNodeIds = new Set(["ws-fail"]); + rerender(); + // Bar mounted with survivor visible. + expect(screen.getByText("1 selected")).toBeTruthy(); + + // User clears selection (Escape / ✕ button) — selection empties. + mockSelectedNodeIds = new Set(); + rerender(); + + // Bar unmounts. The count===0 early return hides it; the useEffect then + // resets hasFailedBatch so a future single-node selection won't re-show + // the bar by mistake. + expect(container.innerHTML).toBe(""); + }); + + it("hasFailedBatch resets after a successful retry (success clears before clearSelection)", async () => { + // Setup: partial-fail with 1 survivor → hasFailedBatch=true. + mockSelectedNodeIds = new Set(["ws-ok", "ws-fail"]); + mockBatchDelete.mockImplementationOnce(() => + Promise.reject(new Error("1/2 delete(s) failed")) + ); + const { rerender, container } = render(); + fireEvent.click(screen.getByText("Delete All")); + fireEvent.click(screen.getByText("confirm")); + await new Promise((r) => setTimeout(r, 0)); + + // Survivor remains → bar still mounted, hasFailedBatch=true. + mockSelectedNodeIds = new Set(["ws-fail"]); + rerender(); + expect(screen.getByText("1 selected")).toBeTruthy(); + + // Successful retry: resolve without error → hasFailedBatch clears + // before clearSelection() is called. The survivor is then removed + // (deleted), leaving count=0. + mockBatchDelete.mockImplementationOnce(() => Promise.resolve()); + fireEvent.click(screen.getByText("Delete All")); + fireEvent.click(screen.getByText("confirm")); + await new Promise((r) => setTimeout(r, 0)); + + // After success + deletion: 0 remaining, hasFailedBatch=false. + // Clearing selection must unmount the bar. If hasFailedBatch had NOT + // been cleared, the bar would re-mount as a single-node toolbar + // (because it would still be in the survivor state from the prior + // catch block). + mockSelectedNodeIds = new Set(); + rerender(); + expect(container.innerHTML).toBe(""); + }); +}); diff --git a/canvas/src/store/__tests__/canvas-batch-partial-failure.test.ts b/canvas/src/store/__tests__/canvas-batch-partial-failure.test.ts new file mode 100644 index 00000000..ee3771ff --- /dev/null +++ b/canvas/src/store/__tests__/canvas-batch-partial-failure.test.ts @@ -0,0 +1,209 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; + +// Mock fetch BEFORE importing the store — api.ts uses the global. +// Individual tests replace this mock to drive ok/!ok per-URL. +global.fetch = vi.fn(); + +import { useCanvasStore } from "../canvas"; +import type { WorkspaceData } from "../socket"; + +function makeWS(overrides: Partial & { id: string }): WorkspaceData { + return { + name: "WS", + role: "agent", + tier: 1, + status: "online", + agent_card: null, + url: "http://localhost:9000", + parent_id: null, + active_tasks: 0, + last_error_rate: 0, + last_sample_error: "", + uptime_seconds: 60, + current_task: "", + x: 0, + y: 0, + collapsed: false, + runtime: "", + budget_limit: null, + ...overrides, + }; +} + +/** + * Partial-failure contract for batchRestart / batchPause / batchDelete. + * + * api.post / api.del throw on non-2xx (src/lib/api.ts:32-34). The store uses + * Promise.allSettled which swallows those rejections. Before the fix: + * - batchDelete removed every id unconditionally, producing ghost workspaces. + * - batchRestart cleared needsRestart on every id unconditionally. + * - All three resolved undefined, so BatchActionBar's catch was dead code. + * + * After the fix: successful ids mutate, failed ids stay selected for retry, + * and the method throws an Error summarising the failure count. + */ + +beforeEach(() => { + useCanvasStore.setState({ + nodes: [], + edges: [], + selectedNodeId: null, + selectedNodeIds: new Set(), + panelTab: "details", + dragOverNodeId: null, + contextMenu: null, + searchOpen: false, + viewport: { x: 0, y: 0, zoom: 1 }, + }); + vi.clearAllMocks(); +}); + +// Drives global.fetch so that a URL matching `failSubstring` returns a 500 +// and every other call returns ok:true with an empty JSON body. +function installPartialFetch(failSubstring: string) { + (global.fetch as unknown as ReturnType).mockImplementation( + (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : input.toString(); + if (url.includes(failSubstring)) { + return Promise.resolve({ + ok: false, + status: 500, + json: () => Promise.resolve({}), + text: () => Promise.resolve("boom"), + } as Response); + } + return Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({}), + text: () => Promise.resolve(""), + } as Response); + } + ); +} + +// ────────────────────────────────────────────────────────────────────────── +// batchDelete +// ────────────────────────────────────────────────────────────────────────── + +describe("batchDelete — partial failure", () => { + it("leaves the failed workspace in `nodes` (no ghost removal)", async () => { + useCanvasStore.setState({ + nodes: [ + { id: "ws-ok", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-ok" }) }, + { id: "ws-fail", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-fail" }) }, + ], + selectedNodeIds: new Set(["ws-ok", "ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await expect(useCanvasStore.getState().batchDelete()).rejects.toThrow(/1\/2 delete/); + + const ids = useCanvasStore.getState().nodes.map((n) => n.id); + expect(ids).toContain("ws-fail"); + expect(ids).not.toContain("ws-ok"); + }); + + it("keeps the failed id in selectedNodeIds so the user can retry", async () => { + useCanvasStore.setState({ + nodes: [ + { id: "ws-ok", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-ok" }) }, + { id: "ws-fail", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-fail" }) }, + ], + selectedNodeIds: new Set(["ws-ok", "ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await useCanvasStore.getState().batchDelete().catch(() => { + /* swallow — we're asserting state */ + }); + + const sel = useCanvasStore.getState().selectedNodeIds; + expect(sel.has("ws-fail")).toBe(true); + expect(sel.has("ws-ok")).toBe(false); + }); + + it("rejects so the BatchActionBar error-toast path runs", async () => { + useCanvasStore.setState({ + nodes: [ + { id: "ws-fail", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-fail" }) }, + ], + selectedNodeIds: new Set(["ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await expect(useCanvasStore.getState().batchDelete()).rejects.toBeInstanceOf(Error); + }); +}); + +// ────────────────────────────────────────────────────────────────────────── +// batchRestart +// ────────────────────────────────────────────────────────────────────────── + +describe("batchRestart — partial failure", () => { + it("keeps needsRestart=true on the workspace that failed to restart", async () => { + useCanvasStore.setState({ + nodes: [ + { + id: "ws-ok", + type: "workspace", + position: { x: 0, y: 0 }, + data: { ...makeWS({ id: "ws-ok" }), needsRestart: true } as WorkspaceData & { needsRestart: boolean }, + }, + { + id: "ws-fail", + type: "workspace", + position: { x: 0, y: 0 }, + data: { ...makeWS({ id: "ws-fail" }), needsRestart: true } as WorkspaceData & { needsRestart: boolean }, + }, + ], + selectedNodeIds: new Set(["ws-ok", "ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await useCanvasStore.getState().batchRestart().catch(() => { + /* swallow — we're asserting state */ + }); + + const byId = Object.fromEntries( + useCanvasStore.getState().nodes.map((n) => [n.id, n.data as WorkspaceData & { needsRestart?: boolean }]) + ); + expect(byId["ws-ok"].needsRestart).toBe(false); + expect(byId["ws-fail"].needsRestart).toBe(true); + }); + + it("rejects so the BatchActionBar error-toast path runs", async () => { + useCanvasStore.setState({ + nodes: [ + { + id: "ws-fail", + type: "workspace", + position: { x: 0, y: 0 }, + data: { ...makeWS({ id: "ws-fail" }), needsRestart: true } as WorkspaceData & { needsRestart: boolean }, + }, + ], + selectedNodeIds: new Set(["ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await expect(useCanvasStore.getState().batchRestart()).rejects.toBeInstanceOf(Error); + }); +}); + +// ────────────────────────────────────────────────────────────────────────── +// batchPause +// ────────────────────────────────────────────────────────────────────────── + +describe("batchPause — partial failure", () => { + it("rejects so the BatchActionBar error-toast path runs", async () => { + useCanvasStore.setState({ + nodes: [ + { id: "ws-fail", type: "workspace", position: { x: 0, y: 0 }, data: makeWS({ id: "ws-fail" }) }, + ], + selectedNodeIds: new Set(["ws-fail"]), + }); + installPartialFetch("ws-fail"); + + await expect(useCanvasStore.getState().batchPause()).rejects.toBeInstanceOf(Error); + }); +}); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index f0c73b6d..7c7cc314 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -126,22 +126,56 @@ export const useCanvasStore = create((set, get) => ({ clearSelection: () => set({ selectedNodeIds: new Set() }), batchRestart: async () => { const ids = Array.from(get().selectedNodeIds); - await Promise.allSettled(ids.map((id) => api.post(`/workspaces/${id}/restart`))); - for (const id of ids) { - get().updateNodeData(id, { needsRestart: false }); + const results = await Promise.allSettled( + ids.map((id) => api.post(`/workspaces/${id}/restart`)) + ); + const failed: string[] = []; + results.forEach((r, i) => { + if (r.status === "fulfilled") { + get().updateNodeData(ids[i], { needsRestart: false }); + } else { + failed.push(ids[i]); + } + }); + // Keep failed IDs selected so the user can retry; drop the successful ones. + set({ selectedNodeIds: new Set(failed) }); + if (failed.length > 0) { + throw new Error(`${failed.length}/${ids.length} restart(s) failed`); } }, batchPause: async () => { const ids = Array.from(get().selectedNodeIds); - await Promise.allSettled(ids.map((id) => api.post(`/workspaces/${id}/pause`))); + const results = await Promise.allSettled( + ids.map((id) => api.post(`/workspaces/${id}/pause`)) + ); + const failed: string[] = []; + results.forEach((r, i) => { + if (r.status !== "fulfilled") failed.push(ids[i]); + }); + set({ selectedNodeIds: new Set(failed) }); + if (failed.length > 0) { + throw new Error(`${failed.length}/${ids.length} pause(s) failed`); + } }, batchDelete: async () => { const ids = Array.from(get().selectedNodeIds); - await Promise.allSettled(ids.map((id) => api.del(`/workspaces/${id}`))); - for (const id of ids) { - get().removeNode(id); + const results = await Promise.allSettled( + ids.map((id) => api.del(`/workspaces/${id}`)) + ); + const failed: string[] = []; + results.forEach((r, i) => { + if (r.status === "fulfilled") { + get().removeNode(ids[i]); + } else { + failed.push(ids[i]); + } + }); + // Keep failed IDs selected so the user can retry; the successful ones + // were already removed from `nodes` above. + set({ selectedNodeIds: new Set(failed) }); + if (failed.length > 0) { + throw new Error(`${failed.length}/${ids.length} delete(s) failed`); } - set({ selectedNodeIds: new Set() }); }, wsStatus: "connecting", setWsStatus: (status) => set({ wsStatus: status }),