From 0c51ca1a76f578765ff71f222fea4fd114eadd55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Thu, 13 Aug 2026 00:06:39 +0200 Subject: [PATCH] feat(automations): scope run delete-all to the active status view (#2584) --- .../automations/runs-section.test.tsx | 131 +++- .../components/automations/runs-section.tsx | 130 ++-- .../e2e/tests/automations-settings.spec.ts | 59 ++ .../tests/mobile-automations-scroll.spec.ts | 52 ++ .../use-automation-runs-delete.test.ts | 628 ++++++++++++++++++ .../use-automation-runs.test-utils.ts | 143 ++++ .../settings/use-automation-runs.test.ts | 168 ++--- .../domains/settings/use-automation-runs.ts | 427 +++++++++--- .../slices/automations/automations-slice.ts | 31 +- .../web/lib/state/slices/automations/types.ts | 29 + apps/web/lib/state/store.test.ts | 46 ++ apps/web/src/locales/en/automations.json | 3 + apps/web/src/locales/pseudo/automations.json | 3 + apps/web/src/locales/pt-pt/automations.json | 3 + apps/web/src/locales/zh-cn/automations.json | 3 + .../plan.md | 182 +++++ .../task-01-hook-status-scoped-delete.md | 91 +++ .../task-02-runs-table-header-delete-all.md | 103 +++ .../task-03-e2e-status-scoped-delete.md | 76 +++ docs/specs/INDEX.md | 1 + .../spec.md | 70 ++ 21 files changed, 2132 insertions(+), 247 deletions(-) create mode 100644 apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts create mode 100644 apps/web/hooks/domains/settings/use-automation-runs.test-utils.ts create mode 100644 docs/plans/automation-runs-delete-all-by-status/plan.md create mode 100644 docs/plans/automation-runs-delete-all-by-status/task-01-hook-status-scoped-delete.md create mode 100644 docs/plans/automation-runs-delete-all-by-status/task-02-runs-table-header-delete-all.md create mode 100644 docs/plans/automation-runs-delete-all-by-status/task-03-e2e-status-scoped-delete.md create mode 100644 docs/specs/automation-runs-delete-all-by-status/spec.md diff --git a/apps/web/components/automations/runs-section.test.tsx b/apps/web/components/automations/runs-section.test.tsx index c8edac7e70..0b45db1f83 100644 --- a/apps/web/components/automations/runs-section.test.tsx +++ b/apps/web/components/automations/runs-section.test.tsx @@ -36,19 +36,34 @@ function mkRun(overrides: Partial = {}): AutomationRun { }; } -function setup(runs: AutomationRun[]) { +type RunsSectionHook = { + runs: AutomationRun[]; + loading: boolean; + deleting: boolean; + refresh: () => void; + deleteRun: (id: string) => void; + deleteAllRuns: (runIds?: string[]) => void; +}; + +function setup( + runs: AutomationRun[], + overrides: Partial = {}, +): ReturnType { mockUseAutomationRuns.mockReturnValue({ runs, loading: false, + deleting: false, refresh: vi.fn(), deleteRun: vi.fn(), deleteAllRuns: vi.fn(), + ...overrides, }); - render(); + const view = render(); // The runs table only renders once the "Recent Runs" disclosure is expanded. // The heading is count-aware ("Recent Run (1)" / "Recent Runs (2)"), so the // matcher must tolerate both forms rather than pinning the plural. fireEvent.click(screen.getByText(/Recent Runs?\s*\(/)); + return view; } describe("RunsSection status badges", () => { @@ -151,6 +166,118 @@ describe("RunsSection run log", () => { }); }); +const DELETE_ALL_BTN = "delete-all-runs"; +const DELETE_ALL_CONFIRM = "delete-all-runs-confirm"; +const FILTER_SKIPPED = "run-filter-skipped"; + +describe("RunsSection delete-all scope", () => { + afterEach(() => { + cleanup(); + vi.clearAllMocks(); + }); + + it("renders delete-all in the rightmost table header cell, not beside the heading", () => { + setup([mkRun({ id: "run-solo", status: "skipped" })]); + + const button = screen.getByTestId(DELETE_ALL_BTN); + const headerRow = button.closest("thead")?.querySelector("tr"); + expect(headerRow).toBeTruthy(); + const cells = headerRow!.querySelectorAll("th"); + // Last header cell holds the control, aligned with the per-row delete + // buttons in the same column. + expect(cells[cells.length - 1].querySelector(`[data-testid="${DELETE_ALL_BTN}"]`)).toBeTruthy(); + // It no longer lives in the section header beside Refresh. + expect(screen.getByTestId(DELETE_ALL_BTN).closest("thead")).toBeTruthy(); + // Unfiltered: the accessible name matches the full scope. + expect(screen.getByTestId(DELETE_ALL_BTN).getAttribute("title")).toBe("Delete all runs"); + }); + + it("deletes only the runs in the active status view", () => { + const deleteAllRuns = vi.fn(); + setup( + [ + mkRun({ id: "run-pass", status: "succeeded" }), + mkRun({ id: "run-skip-1", status: "skipped" }), + mkRun({ id: "run-skip-2", status: "skipped" }), + ], + { deleteAllRuns }, + ); + + fireEvent.click(screen.getByTestId(FILTER_SKIPPED)); + fireEvent.click(screen.getByTestId(DELETE_ALL_BTN)); + fireEvent.click(screen.getByTestId(DELETE_ALL_CONFIRM)); + + expect(deleteAllRuns).toHaveBeenCalledWith(["run-skip-1", "run-skip-2"]); + }); + + it("deletes every run from the All view with no id scope", () => { + const deleteAllRuns = vi.fn(); + setup( + [mkRun({ id: "run-a", status: "succeeded" }), mkRun({ id: "run-b", status: "skipped" })], + { deleteAllRuns }, + ); + + fireEvent.click(screen.getByTestId(DELETE_ALL_BTN)); + fireEvent.click(screen.getByTestId(DELETE_ALL_CONFIRM)); + + expect(deleteAllRuns).toHaveBeenCalledWith(); + }); + + it("hides delete-all when the active filter's runs are gone", () => { + const { rerender } = setup([ + mkRun({ id: "run-temp", status: "skipped" }), + mkRun({ id: "run-stay", status: "succeeded" }), + ]); + + fireEvent.click(screen.getByTestId(FILTER_SKIPPED)); + expect(screen.getByTestId(DELETE_ALL_BTN)).toBeTruthy(); + + // The runs refresh and the skipped one disappears server-side while the + // filter is still active — the empty view must not offer delete-all. + mockUseAutomationRuns.mockReturnValue({ + runs: [mkRun({ id: "run-stay", status: "succeeded" })], + loading: false, + refresh: vi.fn(), + deleteRun: vi.fn(), + deleteAllRuns: vi.fn(), + }); + rerender(); + + expect(screen.queryByTestId(DELETE_ALL_BTN)).toBeNull(); + }); + + it("disables the delete controls while a delete is in flight", () => { + setup( + [mkRun({ id: "run-a", status: "succeeded" }), mkRun({ id: "run-b", status: "skipped" })], + { deleting: true }, + ); + + expect((screen.getByTestId(DELETE_ALL_BTN) as HTMLButtonElement).disabled).toBe(true); + expect((screen.getAllByTestId("delete-run")[0] as HTMLButtonElement).disabled).toBe(true); + }); + + it("names the active status in the confirmation dialog", () => { + setup([ + mkRun({ id: "run-stay", status: "succeeded" }), + mkRun({ id: "run-filtered", status: "skipped" }), + ]); + + fireEvent.click(screen.getByTestId(FILTER_SKIPPED)); + fireEvent.click(screen.getByTestId(DELETE_ALL_BTN)); + + // The button's accessible name names the active status, matching the + // dialog copy. + expect(screen.getByTestId(DELETE_ALL_BTN).getAttribute("title")).toBe( + "Delete all Skipped runs", + ); + expect( + screen.getByText( + "This will permanently remove the Skipped runs shown in this view and their associated tasks. This cannot be undone.", + ), + ).toBeTruthy(); + }); +}); + describe("RunsSection derived statuses", () => { afterEach(() => { cleanup(); diff --git a/apps/web/components/automations/runs-section.tsx b/apps/web/components/automations/runs-section.tsx index 8c3d532216..b00ee95faa 100644 --- a/apps/web/components/automations/runs-section.tsx +++ b/apps/web/components/automations/runs-section.tsx @@ -50,11 +50,12 @@ const STATUS_BADGE: Record< type RunRowProps = { run: AutomationRun; + deleting: boolean; onDelete: (id: string) => void; onNavigate: (taskId: string) => void; }; -function RunRow({ run, onDelete, onNavigate }: RunRowProps) { +function RunRow({ run, deleting, onDelete, onNavigate }: RunRowProps) { const { t } = useTranslation(); const badge = STATUS_BADGE[run.status] ?? STATUS_BADGE.triggered; // Any run that produced a task links to it, run-mode included. Run mode @@ -101,6 +102,7 @@ function RunRow({ run, onDelete, onNavigate }: RunRowProps) { e.preventDefault(); onDelete(run.id); }} + disabled={deleting} title={t("automations:deleteRun")} data-testid="delete-run" > @@ -111,46 +113,6 @@ function RunRow({ run, onDelete, onNavigate }: RunRowProps) { ); } -type DeleteAllButtonProps = { disabled: boolean; onConfirm: () => void }; - -function DeleteAllButton({ disabled, onConfirm }: DeleteAllButtonProps) { - const { t } = useTranslation(); - return ( - - - - - - - {t("automations:deleteAllRunsTitle")} - - {t("automations:deleteAllRunsDescription")} - - - - {t("common:cancel")} - - {t("automations:deleteAll")} - - - - - ); -} - /** * Status filters for the run log. "Skipped" is deliberately one of them: a * scheduled firing turned away by the concurrency cap writes a row and @@ -169,6 +131,70 @@ const STATUS_FILTERS: { value: RunStatus | "all"; labelKey: string }[] = [ { value: "cancelled", labelKey: "automations:runStatusCancelled" }, ]; +type DeleteAllButtonProps = { + disabled: boolean; + statusFilter: RunStatus | "all"; + onConfirm: () => void; +}; + +function DeleteAllButton({ disabled, statusFilter, onConfirm }: DeleteAllButtonProps) { + const { t } = useTranslation(); + // A status-scoped delete-all only removes the runs shown in the active + // view, so the confirmation names that scope instead of promising every + // run record for the automation. + const scoped = statusFilter !== "all"; + const statusLabel = scoped + ? t(STATUS_FILTERS.find((filter) => filter.value === statusFilter)?.labelKey ?? "") + : ""; + // The accessible name must match the action's actual scope, not just the + // dialog: in a filtered view "Delete all runs" would promise more than the + // button deletes. + const label = scoped + ? t("automations:deleteAllRunsScoped", { status: statusLabel }) + : t("automations:deleteAllRuns"); + return ( + + + + + + + + {scoped + ? t("automations:deleteAllRunsScopedTitle", { status: statusLabel }) + : t("automations:deleteAllRunsTitle")} + + + {scoped + ? t("automations:deleteAllRunsScopedDescription", { status: statusLabel }) + : t("automations:deleteAllRunsDescription")} + + + + {t("common:cancel")} + + {t("automations:deleteAll")} + + + + + ); +} + function StatusFilter({ runs, value, @@ -211,7 +237,7 @@ export function RunsSection({ automationId, workspaceId }: RunsSectionProps) { const { t } = useTranslation(); const [expanded, setExpanded] = useState(false); const [statusFilter, setStatusFilter] = useState("all"); - const { runs, loading, refresh, deleteRun, deleteAllRuns } = useAutomationRuns( + const { runs, loading, refresh, deleteRun, deleteAllRuns, deleting } = useAutomationRuns( automationId, workspaceId, ); @@ -246,12 +272,11 @@ export function RunsSection({ automationId, workspaceId }: RunsSectionProps) { size="icon-sm" className="cursor-pointer" onClick={refresh} - disabled={loading} + disabled={loading || deleting} title={t("automations:refresh")} > - {runs.length > 0 && } )} @@ -267,7 +292,21 @@ export function RunsSection({ automationId, workspaceId }: RunsSectionProps) { {t("automations:runsColumnStatus")} {t("automations:outcome")} {t("automations:runsColumnTime")} - + + {visibleRuns.length > 0 && ( + { + if (statusFilter === "all") { + deleteAllRuns(); + } else { + deleteAllRuns(visibleRuns.map((run) => run.id)); + } + }} + /> + )} + @@ -282,6 +321,7 @@ export function RunsSection({ automationId, workspaceId }: RunsSectionProps) { router.push(`/tasks/${id}`)} /> diff --git a/apps/web/e2e/tests/automations-settings.spec.ts b/apps/web/e2e/tests/automations-settings.spec.ts index 583f923bf7..cbc1349142 100644 --- a/apps/web/e2e/tests/automations-settings.spec.ts +++ b/apps/web/e2e/tests/automations-settings.spec.ts @@ -360,6 +360,65 @@ test.describe("Automations settings page", () => { await expect(testPage.getByText("No runs yet")).toBeVisible({ timeout: 5_000 }); }); + test("delete all only removes the runs in the active status view", async ({ + testPage, + seedData, + apiClient, + }) => { + // Seed mixed-status runs: the delete-all control must be scoped to the + // active filter, so clearing the Skipped view leaves the succeeded run + // alone instead of wiping the whole automation. + const automation = await apiClient.seedAutomation({ + workspaceId: seedData.workspaceId, + name: "Filtered Run Delete Test", + workflowId: seedData.workflowId, + workflowStepId: seedData.startStepId, + }); + await apiClient.seedAutomationRun(automation.id, "skipped"); + await apiClient.seedAutomationRun(automation.id, "skipped"); + await apiClient.seedAutomationRun(automation.id, "succeeded"); + + await testPage.goto( + `/settings/workspaces/${seedData.workspaceId}/automations/${automation.id}`, + ); + await testPage.getByTestId("automation-editor").waitFor({ state: "visible", timeout: 15_000 }); + + const scrollContainer = testPage.getByTestId("settings-scroll-container"); + await scrollContainer.evaluate((el) => (el.scrollTop = el.scrollHeight)); + + const recentRunsButton = testPage.locator("button", { hasText: /Recent Runs/ }); + await recentRunsButton.waitFor({ state: "visible", timeout: 10_000 }); + await recentRunsButton.click(); + + const tbody = testPage.locator("table tbody"); + await tbody.waitFor({ state: "visible", timeout: 5_000 }); + await expect(tbody.locator("tr")).toHaveCount(3, { timeout: 10_000 }); + + // Filter to Skipped: only the two skipped rows remain visible. + await testPage.getByTestId("run-filter-skipped").click(); + await expect(tbody.locator("tr")).toHaveCount(2, { timeout: 5_000 }); + + // The delete-all control lives in the table header, aligned with the + // per-row delete buttons, not beside the Recent Runs heading. + const thead = testPage.locator("table thead"); + await expect(thead.getByTestId("delete-all-runs")).toBeVisible(); + + // The confirmation names the scoped status rather than promising every + // run record for the automation. + await thead.getByTestId("delete-all-runs").click(); + await expect( + testPage.getByText(/permanently remove the Skipped runs shown in this view/), + ).toBeVisible(); + await testPage.getByTestId("delete-all-runs-confirm").click(); + + // Only the skipped runs are gone; the succeeded run survives. + await expect(tbody.locator("tr")).toHaveCount(1, { timeout: 5_000 }); + + // Switching back to All still shows the succeeded run. + await testPage.getByTestId("run-filter-all").click(); + await expect(tbody.locator("tr")).toHaveCount(1, { timeout: 5_000 }); + }); + test("archived task's run shows Archived, cancelled task's run shows Cancelled", async ({ testPage, seedData, diff --git a/apps/web/e2e/tests/mobile-automations-scroll.spec.ts b/apps/web/e2e/tests/mobile-automations-scroll.spec.ts index e877d50847..24ec34e035 100644 --- a/apps/web/e2e/tests/mobile-automations-scroll.spec.ts +++ b/apps/web/e2e/tests/mobile-automations-scroll.spec.ts @@ -48,4 +48,56 @@ test.describe("Automations settings on mobile", () => { await expect.poll(() => testPage.evaluate(() => window.scrollY), { timeout: 5_000 }).toBe(0); }); + + test("delete all in a status view removes only that view's runs on mobile", async ({ + testPage, + seedData, + apiClient, + }) => { + // The delete-all control now lives in the table header's rightmost cell; + // on a phone viewport that position must stay reachable and scoped to + // the active status filter. + const automation = await apiClient.seedAutomation({ + workspaceId: seedData.workspaceId, + name: "Mobile Filtered Delete", + workflowId: seedData.workflowId, + workflowStepId: seedData.startStepId, + }); + await apiClient.seedAutomationRun(automation.id, "skipped"); + await apiClient.seedAutomationRun(automation.id, "skipped"); + await apiClient.seedAutomationRun(automation.id, "succeeded"); + + await testPage.goto( + `/settings/workspaces/${seedData.workspaceId}/automations/${automation.id}`, + ); + await testPage.getByTestId("automation-editor").waitFor({ state: "visible", timeout: 15_000 }); + + const scrollContainer = testPage.getByTestId("settings-scroll-container"); + await scrollContainer.evaluate((el) => (el.scrollTop = el.scrollHeight)); + + const recentRunsButton = testPage.locator("button", { hasText: /Recent Runs/ }); + await recentRunsButton.waitFor({ state: "visible", timeout: 10_000 }); + await recentRunsButton.click(); + + const tbody = testPage.locator("table tbody"); + await tbody.waitFor({ state: "visible", timeout: 5_000 }); + await expect(tbody.locator("tr")).toHaveCount(3, { timeout: 10_000 }); + + // Filter to Skipped, then delete all from the table header. + await testPage.getByTestId("run-filter-skipped").click(); + await expect(tbody.locator("tr")).toHaveCount(2, { timeout: 5_000 }); + + const deleteAll = testPage.locator("table thead").getByTestId("delete-all-runs"); + await expect(deleteAll).toBeVisible(); + await deleteAll.click(); + await expect( + testPage.getByText(/permanently remove the Skipped runs shown in this view/), + ).toBeVisible(); + await testPage.getByTestId("delete-all-runs-confirm").click(); + + // Only the skipped runs are gone; the succeeded run survives. + await expect(tbody.locator("tr")).toHaveCount(1, { timeout: 5_000 }); + await testPage.getByTestId("run-filter-all").click(); + await expect(tbody.locator("tr")).toHaveCount(1, { timeout: 5_000 }); + }); }); diff --git a/apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts b/apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts new file mode 100644 index 0000000000..6ee2053d41 --- /dev/null +++ b/apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts @@ -0,0 +1,628 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import type { AutomationRun } from "@/lib/types/automation"; + +import { + AUTOMATION_ID, + WORKSPACE_ID, + runsStore, + deferred, + mkRun, + setRuns, +} from "./use-automation-runs.test-utils"; + +vi.mock("@/components/state-provider", async () => { + const { runsStore, mockStoreApi } = await import("./use-automation-runs.test-utils"); + return { + useAppStore: (selector: (s: unknown) => unknown) => selector(runsStore.get()), + useAppStoreApi: () => mockStoreApi, + }; +}); + +vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +vi.mock("@/lib/api/domains/automation-api", () => ({ + listAutomationRuns: vi.fn(), + deleteAutomationRun: vi.fn(), + deleteAllAutomationRuns: vi.fn(), +})); + +import { toast } from "sonner"; +import { + listAutomationRuns, + deleteAutomationRun, + deleteAllAutomationRuns, +} from "@/lib/api/domains/automation-api"; +import { useAutomationRuns } from "./use-automation-runs"; + +const BATCH_DELETE_ERROR = "batch delete failed"; +const NETWORK_DOWN = "network down"; + +beforeEach(() => { + runsStore.reset(); + vi.mocked(listAutomationRuns).mockReset(); + vi.mocked(deleteAutomationRun).mockReset(); + vi.mocked(deleteAllAutomationRuns).mockReset(); + vi.mocked(toast.error).mockReset(); +}); + +describe("useAutomationRuns - status-scoped delete-all", () => { + it("deletes exactly the given runs through the per-run API and removes them optimistically", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + const runZ = mkRun("run-z"); + setRuns(AUTOMATION_ID, [runX, runY, runZ]); + + vi.mocked(listAutomationRuns).mockReturnValue(Promise.withResolvers().promise); + vi.mocked(deleteAutomationRun).mockResolvedValue({ deleted: true }); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x", "run-z"]); + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + expect(deleteAutomationRun).toHaveBeenCalledTimes(2); + expect(deleteAutomationRun).toHaveBeenCalledWith("run-x", WORKSPACE_ID); + expect(deleteAutomationRun).toHaveBeenCalledWith("run-z", WORKSPACE_ID); + expect(deleteAllAutomationRuns).not.toHaveBeenCalled(); + }); + + it("keeps a run created after the deletes completed when reconciling the scoped view", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + const runNew = mkRun("run-new"); + setRuns(AUTOMATION_ID, [runX, runY]); + + const del = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(del.promise); + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockResolvedValue([runY, runNew]); // authoritative post-delete refresh + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + + // The success path must end with exactly the authoritative post-delete + // list: a blanket re-clear would drop run-new, which was created after + // the deletes completed server-side. + await act(async () => { + del.resolve({ deleted: true }); + await del.promise; + }); + rerender(); + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-new", "run-y"]); + }); +}); + +describe("useAutomationRuns - status-scoped delete-all failure recovery", () => { + it("shows one aggregated toast and reverts to the server list when any delete fails", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + vi.mocked(listAutomationRuns).mockResolvedValue([runX, runY]); + vi.mocked(deleteAutomationRun).mockRejectedValue(new Error(BATCH_DELETE_ERROR)); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + await act(async () => {}); + rerender(); + + await act(async () => { + result.current.deleteAllRuns(["run-x", "run-y"]); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + + // One toast for the whole batch, not one per failed delete. + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith(BATCH_DELETE_ERROR); + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + // The serialization slot reopens once the recovery settles. + expect(result.current.deleting).toBe(false); + }); + + it("restores the pre-delete snapshot if a delete fails and the recovery refresh also fails", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + vi.mocked(listAutomationRuns) + .mockResolvedValueOnce([runX, runY]) + .mockRejectedValueOnce(new Error(NETWORK_DOWN)); + vi.mocked(deleteAutomationRun).mockRejectedValue(new Error(BATCH_DELETE_ERROR)); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + await act(async () => {}); + rerender(); + + await act(async () => { + result.current.deleteAllRuns(["run-x"]); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(toast.error).toHaveBeenCalledTimes(2); // delete failure + could-not-refresh + // The serialization slot reopens even when the recovery refresh fails. + expect(result.current.deleting).toBe(false); + }); + + it("treats an empty id list as a no-op", () => { + setRuns(AUTOMATION_ID, [mkRun("run-x")]); + vi.mocked(listAutomationRuns).mockReturnValue(Promise.withResolvers().promise); + + const { result } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns([]); + }); + expect(result.current.runs).toHaveLength(1); + expect(deleteAutomationRun).not.toHaveBeenCalled(); + expect(deleteAllAutomationRuns).not.toHaveBeenCalled(); + }); +}); + +describe("useAutomationRuns - scoped delete-all stale list responses", () => { + it("discards a refresh that started before a scoped delete-all and resolves after it succeeds", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + const stale = deferred(); + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockReturnValueOnce(stale.promise) // refresh started before the delete + .mockResolvedValue([runY]); // authoritative post-delete refresh + vi.mocked(deleteAutomationRun).mockResolvedValue({ deleted: true }); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.refresh(); // in flight when the delete starts + result.current.deleteAllRuns(["run-x"]); // succeeds immediately + }); + // Let the delete's success re-removal land before the stale list resolves. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + + // The stale pre-delete list resolves after the delete confirmed — the + // mutation guard must discard it instead of resurrecting run-x. + await act(async () => { + stale.resolve([runX, runY]); + await stale.promise; + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + }); + + it("ignores a refresh while a scoped delete-all is in flight", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockResolvedValue([runY]); // authoritative post-delete refresh + vi.mocked(deleteAutomationRun).mockResolvedValue({ deleted: true }); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); + result.current.refresh(); // no-op while the delete owns the store + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + // Only the mount fetch and the post-delete reconciliation ran; the + // in-flight refresh was ignored at the hook level. + expect(listAutomationRuns).toHaveBeenCalledTimes(2); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + }); +}); +describe("useAutomationRuns - unscoped delete-all stale list responses", () => { + it("ignores a refresh while an unscoped delete-all is in flight", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockResolvedValue([]); // authoritative post-delete refresh + vi.mocked(deleteAllAutomationRuns).mockResolvedValue({ deleted: true }); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(); + result.current.refresh(); // no-op while the delete owns the store + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(listAutomationRuns).toHaveBeenCalledTimes(2); // mount + reconciliation + expect(result.current.runs).toEqual([]); + }); +}); +describe("useAutomationRuns - single-run delete invalidation", () => { + it("ignores a refresh while a single-run delete is in flight", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + const del = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(del.promise); + vi.mocked(listAutomationRuns).mockReturnValueOnce( + Promise.withResolvers().promise, + ); // mount + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteRun("run-x"); + result.current.refresh(); // no-op while the delete owns the store + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + + await act(async () => { + del.resolve({ deleted: true }); + await del.promise; + }); + rerender(); + expect(listAutomationRuns).toHaveBeenCalledTimes(1); // mount only + expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); + }); +}); +describe("useAutomationRuns - loading flag", () => { + it("keeps the loading flag until the latest list request settles", async () => { + setRuns(AUTOMATION_ID, []); + const first = deferred(); + const second = deferred(); + vi.mocked(listAutomationRuns) + .mockResolvedValueOnce([]) // mount + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + await act(async () => {}); + rerender(); + expect(result.current.loading).toBe(false); + + act(() => { + result.current.refresh(); // first request + result.current.refresh(); // second, newer request + }); + rerender(); + expect(result.current.loading).toBe(true); + + // The older request settling must not clear the newer request's loading. + await act(async () => { + first.resolve([mkRun("run-x")]); + await first.promise; + }); + rerender(); + expect(result.current.loading).toBe(true); + + await act(async () => { + second.resolve([]); + await second.promise; + }); + rerender(); + expect(result.current.loading).toBe(false); + }); + + it("does not let an older list response overwrite a newer one", async () => { + setRuns(AUTOMATION_ID, []); + const older = deferred(); + const newer = deferred(); + vi.mocked(listAutomationRuns) + .mockResolvedValueOnce([]) // mount + .mockReturnValueOnce(older.promise) + .mockReturnValueOnce(newer.promise); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + await act(async () => {}); + rerender(); + + act(() => { + result.current.refresh(); // older request + result.current.refresh(); // newer request + }); + // The newer request resolves first with the freshest data. + await act(async () => { + newer.resolve([mkRun("run-new")]); + await newer.promise; + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-new"]); + + // The older request resolves later with staler data — the latest-request + // token must discard it instead of overwriting the newer result. + await act(async () => { + older.resolve([mkRun("run-old")]); + await older.promise; + }); + rerender(); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-new"]); + }); +}); +describe("useAutomationRuns - batch recovery timing", () => { + it("does not let a refresh supersede an in-flight recovery", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + const recovery = deferred(); + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockReturnValueOnce(recovery.promise); // recovery refresh + vi.mocked(deleteAutomationRun).mockRejectedValue(new Error(BATCH_DELETE_ERROR)); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); // delete fails → recovery starts + result.current.refresh(); // must be a no-op while the recovery is in flight + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + // refresh() was ignored at the hook level: only the mount fetch and the + // recovery ran, so the recovery's fallback cannot be superseded. + expect(listAutomationRuns).toHaveBeenCalledTimes(2); + + // The recovery fails → the snapshot is restored, the slot is released, + // and the failed delete's rows stay visible. + await act(async () => { + recovery.reject(new Error(NETWORK_DOWN)); + await recovery.promise.catch(() => {}); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(toast.error).toHaveBeenCalledTimes(2); // delete failure + could-not-refresh + expect(result.current.deleting).toBe(false); + }); + + it("does not recover from a partial failure until every delete in the batch settles", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + setRuns(AUTOMATION_ID, [runX, runY]); + + const slowDelete = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun) + .mockRejectedValueOnce(new Error(BATCH_DELETE_ERROR)) + .mockReturnValueOnce(slowDelete.promise); + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockResolvedValue([runX, runY]); // recovery refresh + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x", "run-y"]); + }); + // run-x rejects first while run-y is still pending — the recovery must + // not run yet, or it would race the in-flight delete. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(listAutomationRuns).toHaveBeenCalledTimes(1); // mount fetch only + expect(toast.error).not.toHaveBeenCalled(); + + // run-y settles successfully; only now does the batch settle and recover. + await act(async () => { + slowDelete.resolve({ deleted: true }); + await slowDelete.promise; + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(listAutomationRuns).toHaveBeenCalledTimes(2); // mount + recovery + expect(toast.error).toHaveBeenCalledTimes(1); + // The server list (both runs still present) wins over the optimistic + // removal once every delete has settled. + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + }); +}); +describe("useAutomationRuns - batch recovery fallback", () => { + it("restores only the failed-delete rows when recovery fails after a partial batch", async () => { + const runX = mkRun("run-x"); + const runY = mkRun("run-y"); + const runZ = mkRun("run-z"); + setRuns(AUTOMATION_ID, [runX, runY, runZ]); + + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockRejectedValueOnce(new Error(NETWORK_DOWN)); // recovery fetch fails + vi.mocked(deleteAutomationRun) + .mockResolvedValueOnce({ deleted: true }) // run-x succeeds server-side + .mockRejectedValueOnce(new Error(BATCH_DELETE_ERROR)); // run-y fails + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + await act(async () => {}); + rerender(); + + await act(async () => { + result.current.deleteAllRuns(["run-x", "run-y"]); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + + // run-x was deleted server-side and must NOT be resurrected by the + // snapshot fallback; run-y failed and is restored; run-z was never + // targeted and stays. + expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-y", "run-z"]); + expect(toast.error).toHaveBeenCalledTimes(2); // delete failure + could-not-refresh + expect(result.current.deleting).toBe(false); + }); + + it("toasts when the post-delete reconciliation fetch fails", async () => { + const runX = mkRun("run-x"); + setRuns(AUTOMATION_ID, [runX]); + + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockRejectedValueOnce(new Error(NETWORK_DOWN)); // reconciliation fails + vi.mocked(deleteAutomationRun).mockResolvedValue({ deleted: true }); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + + // The optimistic removal stays (the delete succeeded), but the failed + // reconciliation is surfaced and the serialization slot releases. + expect(result.current.runs.map((r) => r.id)).toEqual([]); + expect(toast.error).toHaveBeenCalledTimes(1); // could-not-refresh copy + expect(result.current.deleting).toBe(false); + }); +}); +describe("useAutomationRuns - serialized deletes", () => { + it("ignores a second delete-all fired while one is still in flight", () => { + setRuns(AUTOMATION_ID, [mkRun("run-x"), mkRun("run-y"), mkRun("run-z")]); + const slow = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(slow.promise); + vi.mocked(listAutomationRuns).mockReturnValue(Promise.withResolvers().promise); + + const { result } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x", "run-y"]); + // The second, overlapping mutation must be a no-op: the two delete + // batches are serialized so their recoveries cannot race each other. + result.current.deleteAllRuns(["run-z"]); + }); + expect(deleteAutomationRun).toHaveBeenCalledTimes(2); // only the first batch + }); + + it("ignores a per-run delete fired while a delete-all is in flight", () => { + setRuns(AUTOMATION_ID, [mkRun("run-x"), mkRun("run-y")]); + const slow = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(slow.promise); + // Mount fetch stays pending; the post-delete reconciliation settles so + // A's endDelete (via the reconciliation settle) releases the slot. + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) + .mockResolvedValue([]); + + const { result } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); + result.current.deleteRun("run-y"); + }); + expect(deleteAutomationRun).toHaveBeenCalledTimes(1); // only the delete-all + }); + + it("shares the serialization slot across hook instances for the same automation", async () => { + setRuns(AUTOMATION_ID, [mkRun("run-x"), mkRun("run-y")]); + const slow = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(slow.promise); + // Mount fetch stays pending; the post-delete reconciliation settles so + // A's endDelete (via the reconciliation settle) releases the slot. + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) + .mockResolvedValue([]); + + // Instance A starts a delete and "unmounts" (the editor navigated away). + const first = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + act(() => { + first.result.current.deleteAllRuns(["run-x"]); + }); + first.unmount(); + + // A remounted instance for the same automation must see the shared + // in-flight slot and be gated, not start its own overlapping delete — + // and its mount fetch must not supersede A's recovery either. + const second = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + expect(second.result.current.deleting).toBe(true); + expect(listAutomationRuns).toHaveBeenCalledTimes(1); // A's mount fetch only + act(() => { + second.result.current.deleteAllRuns(["run-y"]); + second.result.current.deleteRun("run-y"); + }); + expect(deleteAutomationRun).toHaveBeenCalledTimes(1); // only A's batch + + // A's delete settles and its endDelete releases the shared slot. + await act(async () => { + slow.resolve({ deleted: true }); + await slow.promise; + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + second.rerender(); + expect(second.result.current.deleting).toBe(false); + }); + + it("exposes deleting while a delete runs and clears it once the batch settles", async () => { + setRuns(AUTOMATION_ID, [mkRun("run-x")]); + const slow = deferred<{ deleted: boolean }>(); + vi.mocked(deleteAutomationRun).mockReturnValue(slow.promise); + // Mount fetch stays pending; the post-delete reconciliation settles so + // the serialization slot (held until it does) can release. + vi.mocked(listAutomationRuns) + .mockReturnValueOnce(Promise.withResolvers().promise) + .mockResolvedValue([]); + + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + + act(() => { + result.current.deleteAllRuns(["run-x"]); + }); + rerender(); + expect(result.current.deleting).toBe(true); + + await act(async () => { + slow.resolve({ deleted: true }); + await slow.promise; + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(result.current.deleting).toBe(false); + }); +}); diff --git a/apps/web/hooks/domains/settings/use-automation-runs.test-utils.ts b/apps/web/hooks/domains/settings/use-automation-runs.test-utils.ts new file mode 100644 index 0000000000..a11fe6fe3c --- /dev/null +++ b/apps/web/hooks/domains/settings/use-automation-runs.test-utils.ts @@ -0,0 +1,143 @@ +import type { AutomationRun } from "@/lib/types/automation"; + +export const AUTOMATION_ID = "auto-1"; +export const WORKSPACE_ID = "ws-1"; + +export type MockRunsState = { + automationRuns: { + byAutomationId: Record; + loading: Record; + mutationEpoch: Record; + deleting: Record; + }; +}; + +function freshState(): MockRunsState { + return { automationRuns: { byAutomationId: {}, loading: {}, mutationEpoch: {}, deleting: {} } }; +} + +/** + * Shared mutable mock of the app store's automation-runs surface, mirroring + * `createAutomationsSlice`. Test files reset it in `beforeEach`; the + * `vi.mock("@/components/state-provider")` factories read `runsStore.get()`. + * `mockStoreApi` is a single stable object (like the production context + * store) so the hook's per-store request registry keys stay consistent. + */ +export const runsStore = { + state: freshState(), + reset() { + this.state = freshState(); + }, + get() { + return { ...this.state, ...actions }; + }, +}; + +export const mockStoreApi = { + getState: () => runsStore.get(), +}; + +function setRunsInternal(automationId: string, runs: AutomationRun[]) { + runsStore.state = { + automationRuns: { + ...runsStore.state.automationRuns, + byAutomationId: { ...runsStore.state.automationRuns.byAutomationId, [automationId]: runs }, + }, + }; +} + +/** Seed the mock store's run list for an automation. */ +export function setRuns(automationId: string, runs: AutomationRun[]) { + setRunsInternal(automationId, runs); +} + +export const actions = { + setAutomationRuns: setRuns, + setAutomationRunsLoading: (automationId: string, loading: boolean) => { + runsStore.state = { + automationRuns: { + ...runsStore.state.automationRuns, + loading: { ...runsStore.state.automationRuns.loading, [automationId]: loading }, + }, + }; + }, + removeAutomationRun: (automationId: string, runId: string) => { + const runs = runsStore.state.automationRuns.byAutomationId[automationId] ?? []; + setRunsInternal( + automationId, + runs.filter((r) => r.id !== runId), + ); + }, + clearAutomationRuns: (automationId: string) => setRunsInternal(automationId, []), + restoreAutomationRun: (automationId: string, run: AutomationRun) => { + const runs = runsStore.state.automationRuns.byAutomationId[automationId] ?? []; + if (runs.some((r) => r.id === run.id)) return; + // Mirrors the production slice: insert keeping newest-first order. + const insertAt = runs.findIndex((r) => r.created_at < run.created_at); + const next = runs.slice(); + if (insertAt === -1) { + next.push(run); + } else { + next.splice(insertAt, 0, run); + } + setRunsInternal(automationId, next); + }, + beginAutomationRunDelete: (automationId: string) => { + if ((runsStore.state.automationRuns.deleting[automationId] ?? false) !== false) return null; + const next = (runsStore.state.automationRuns.mutationEpoch[automationId] ?? 0) + 1; + runsStore.state = { + automationRuns: { + ...runsStore.state.automationRuns, + mutationEpoch: { + ...runsStore.state.automationRuns.mutationEpoch, + [automationId]: next, + }, + deleting: { ...runsStore.state.automationRuns.deleting, [automationId]: next }, + }, + }; + return next; + }, + endAutomationRunDelete: (automationId: string, generation: number) => { + if (runsStore.state.automationRuns.deleting[automationId] === generation) { + runsStore.state = { + automationRuns: { + ...runsStore.state.automationRuns, + deleting: { ...runsStore.state.automationRuns.deleting, [automationId]: false }, + }, + }; + } + }, + advanceAutomationRunEpoch: (automationId: string) => { + runsStore.state = { + automationRuns: { + ...runsStore.state.automationRuns, + mutationEpoch: { + ...runsStore.state.automationRuns.mutationEpoch, + [automationId]: (runsStore.state.automationRuns.mutationEpoch[automationId] ?? 0) + 1, + }, + }, + }; + }, +}; + +/** A promise plus its resolve/reject functions, for controlling when a mocked + * async call settles relative to other events in a test. */ +export function deferred() { + const { promise, resolve, reject } = Promise.withResolvers(); + return { promise, resolve, reject }; +} + +export function mkRun(id: string): AutomationRun { + return { + id, + automation_id: AUTOMATION_ID, + trigger_id: "trig-1", + trigger_type: "scheduled", + task_id: "", + status: "skipped", + dedup_key: "", + trigger_data: {}, + error_message: "", + created_at: new Date().toISOString(), + }; +} diff --git a/apps/web/hooks/domains/settings/use-automation-runs.test.ts b/apps/web/hooks/domains/settings/use-automation-runs.test.ts index 9c33a6e2dd..f206bc737a 100644 --- a/apps/web/hooks/domains/settings/use-automation-runs.test.ts +++ b/apps/web/hooks/domains/settings/use-automation-runs.test.ts @@ -1,57 +1,23 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { renderHook, act } from "@testing-library/react"; import type { AutomationRun } from "@/lib/types/automation"; -type MockState = { - automationRuns: { - byAutomationId: Record; - loading: Record; +import { + AUTOMATION_ID, + WORKSPACE_ID, + runsStore, + deferred, + mkRun, + setRuns, +} from "./use-automation-runs.test-utils"; + +vi.mock("@/components/state-provider", async () => { + const { runsStore, mockStoreApi } = await import("./use-automation-runs.test-utils"); + return { + useAppStore: (selector: (s: unknown) => unknown) => selector(runsStore.get()), + useAppStoreApi: () => mockStoreApi, }; -}; - -let mockState: MockState = { automationRuns: { byAutomationId: {}, loading: {} } }; - -function setRuns(automationId: string, runs: AutomationRun[]) { - mockState = { - automationRuns: { - ...mockState.automationRuns, - byAutomationId: { ...mockState.automationRuns.byAutomationId, [automationId]: runs }, - }, - }; -} - -const storeActions = { - setAutomationRuns: (automationId: string, runs: AutomationRun[]) => setRuns(automationId, runs), - setAutomationRunsLoading: (automationId: string, loading: boolean) => { - mockState = { - automationRuns: { - ...mockState.automationRuns, - loading: { ...mockState.automationRuns.loading, [automationId]: loading }, - }, - }; - }, - removeAutomationRun: (automationId: string, runId: string) => { - const runs = mockState.automationRuns.byAutomationId[automationId] ?? []; - setRuns( - automationId, - runs.filter((r) => r.id !== runId), - ); - }, - clearAutomationRuns: (automationId: string) => setRuns(automationId, []), - restoreAutomationRun: (automationId: string, run: AutomationRun) => { - const runs = mockState.automationRuns.byAutomationId[automationId] ?? []; - if (runs.some((r) => r.id === run.id)) return; - setRuns(automationId, [...runs, run]); - }, -}; - -vi.mock("@/components/state-provider", () => ({ - useAppStore: (selector: (s: MockState & typeof storeActions) => unknown) => - selector({ ...mockState, ...storeActions }), - useAppStoreApi: () => ({ - getState: () => ({ ...mockState, ...storeActions }), - }), -})); +}); vi.mock("sonner", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); @@ -69,33 +35,8 @@ import { } from "@/lib/api/domains/automation-api"; import { useAutomationRuns } from "./use-automation-runs"; -const AUTOMATION_ID = "auto-1"; -const WORKSPACE_ID = "ws-1"; - -function mkRun(id: string): AutomationRun { - return { - id, - automation_id: AUTOMATION_ID, - trigger_id: "trig-1", - trigger_type: "scheduled", - task_id: "", - status: "skipped", - dedup_key: "", - trigger_data: {}, - error_message: "", - created_at: new Date().toISOString(), - }; -} - -/** A promise plus its resolve function, for controlling when a mocked async - * call settles relative to other events in a test. */ -function deferred() { - const { promise, resolve } = Promise.withResolvers(); - return { promise, resolve }; -} - beforeEach(() => { - mockState = { automationRuns: { byAutomationId: {}, loading: {} } }; + runsStore.reset(); vi.mocked(listAutomationRuns).mockReset(); vi.mocked(deleteAutomationRun).mockReset(); vi.mocked(deleteAllAutomationRuns).mockReset(); @@ -103,42 +44,37 @@ beforeEach(() => { }); describe("useAutomationRuns", () => { - it("re-applies the optimistic removal if an in-flight refresh resurrects the row before delete confirms", async () => { + it("does not let a refresh started before a single-run delete resurrect the row", async () => { const runX = mkRun("run-x"); const runY = mkRun("run-y"); setRuns(AUTOMATION_ID, [runX, runY]); - // The delete request stays pending until we manually resolve it below, - // so we can interleave a "stale refresh" in between. + // The delete request stays pending until we manually resolve it below. const del = deferred<{ deleted: boolean }>(); vi.mocked(deleteAutomationRun).mockReturnValue(del.promise); - // A concurrent refresh() resolves with the pre-delete list — as if the - // list request was already in flight when the delete was fired. The - // hook's own mount-effect fetch (first call) is left pending so it - // doesn't confound the explicit refresh() below. + // The refresh starts before the delete claims the mutation epoch and + // resolves with the pre-delete list — the epoch guard must discard it. vi.mocked(listAutomationRuns) - .mockReturnValueOnce(Promise.withResolvers().promise) + .mockReturnValueOnce(Promise.withResolvers().promise) // mount .mockResolvedValue([runX, runY]); const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); act(() => { - result.current.deleteRun("run-x"); + result.current.refresh(); // captures the pre-delete epoch + result.current.deleteRun("run-x"); // claims the epoch + }); + // The pre-delete refresh resolves with the full list — it must be + // discarded (its captured epoch went stale when the delete claimed). + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); }); rerender(); expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); - // The in-flight refresh resolves and overwrites the store with the - // stale full list, resurrecting run-x — this is the race being guarded - // against, reproduced here explicitly. - await act(async () => { - await result.current.refresh(); - }); - rerender(); - expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); - - // The delete now confirms server-side. Without re-applying the removal - // on success, run-x would stay resurrected until the next full refresh. + // The delete confirms server-side. await act(async () => { del.resolve({ deleted: true }); await del.promise; @@ -147,16 +83,15 @@ describe("useAutomationRuns", () => { expect(result.current.runs.map((r) => r.id)).toEqual(["run-y"]); }); - it("re-applies the optimistic clear if an in-flight refresh resurrects rows before delete-all confirms", async () => { - const runX = mkRun("run-x"); - const runY = mkRun("run-y"); - setRuns(AUTOMATION_ID, [runX, runY]); + it("keeps a run created after delete-all completed when reconciling the full list", async () => { + const runNew = mkRun("run-new"); + setRuns(AUTOMATION_ID, [mkRun("run-x"), mkRun("run-y")]); const del = deferred<{ deleted: boolean }>(); vi.mocked(deleteAllAutomationRuns).mockReturnValue(del.promise); vi.mocked(listAutomationRuns) - .mockReturnValueOnce(Promise.withResolvers().promise) - .mockResolvedValue([runX, runY]); + .mockReturnValueOnce(Promise.withResolvers().promise) // mount + .mockResolvedValue([runNew]); // authoritative post-delete refresh const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); @@ -166,33 +101,40 @@ describe("useAutomationRuns", () => { rerender(); expect(result.current.runs).toEqual([]); - await act(async () => { - await result.current.refresh(); - }); - rerender(); - expect(result.current.runs).toHaveLength(2); - + // The success path must end with exactly the authoritative post-delete + // list: a blanket clear would drop run-new, which was created after the + // backend delete completed. await act(async () => { del.resolve({ deleted: true }); await del.promise; }); rerender(); - expect(result.current.runs).toEqual([]); + expect(result.current.runs.map((r) => r.id)).toEqual(["run-new"]); }); - it("passes workspaceId through to the delete-run and delete-all-runs API calls", () => { + it("passes workspaceId through to the delete-run and delete-all-runs API calls", async () => { setRuns(AUTOMATION_ID, [mkRun("run-x")]); vi.mocked(listAutomationRuns).mockReturnValue(Promise.withResolvers().promise); vi.mocked(deleteAutomationRun).mockResolvedValue({ deleted: true }); vi.mocked(deleteAllAutomationRuns).mockResolvedValue({ deleted: true }); - const { result } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); + const { result, rerender } = renderHook(() => useAutomationRuns(AUTOMATION_ID, WORKSPACE_ID)); act(() => { result.current.deleteRun("run-x"); }); expect(deleteAutomationRun).toHaveBeenCalledWith("run-x", WORKSPACE_ID); + // Let the per-run delete settle so the serialization gate reopens before + // the next mutation. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + rerender(); + expect(result.current.deleting).toBe(false); + act(() => { result.current.deleteAllRuns(); }); @@ -230,6 +172,7 @@ describe("useAutomationRuns - double-failure recovery", () => { // left permanently missing run-x. It should be restored from the // pre-delete snapshot rather than silently staying gone. expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(result.current.deleting).toBe(false); }); it("restores the full pre-clear snapshot if both delete-all and the recovery refresh fail", async () => { @@ -258,6 +201,7 @@ describe("useAutomationRuns - double-failure recovery", () => { // left permanently empty. It should be restored from the pre-clear // snapshot rather than silently staying cleared. expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(result.current.deleting).toBe(false); }); }); @@ -288,6 +232,7 @@ describe("useAutomationRuns - single-failure revert", () => { // The recovery refresh succeeded, so the store reflects the server's // authoritative list rather than the double-failure local-cache fallback. expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(result.current.deleting).toBe(false); }); it("shows a toast and reverts to the server list when deleteAllRuns fails but the recovery refresh succeeds", async () => { @@ -312,5 +257,6 @@ describe("useAutomationRuns - single-failure revert", () => { expect(toast.error).toHaveBeenCalledWith("delete-all failed"); expect(result.current.runs.map((r) => r.id).sort()).toEqual(["run-x", "run-y"]); + expect(result.current.deleting).toBe(false); }); }); diff --git a/apps/web/hooks/domains/settings/use-automation-runs.ts b/apps/web/hooks/domains/settings/use-automation-runs.ts index a2e5a259e1..9512f70d17 100644 --- a/apps/web/hooks/domains/settings/use-automation-runs.ts +++ b/apps/web/hooks/domains/settings/use-automation-runs.ts @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useCallback } from "react"; +import { useCallback, useEffect } from "react"; import { t } from "@/lib/i18n"; import { toast } from "@/lib/toast/sonner"; import { @@ -13,6 +13,259 @@ import type { AutomationRun } from "@/lib/types/automation"; const EMPTY_RUNS: AutomationRun[] = []; +const COULD_NOT_REFRESH_RUNS = "automations:couldNotRefreshRuns"; + +/** + * Latest in-flight list request per (store instance, automation). `fetchRuns` + * applies results and settles the shared `loading` flag only when the + * finishing request is still the latest, so an older request completing after + * a newer one started cannot overwrite the newer result or clear its loading + * state. Tokens are opaque symbols — never reused, even after cleanup — and + * the registry is scoped per store instance, so independent stores cannot + * conflate each other's requests. + */ +const latestListRequest = new WeakMap>(); + +function registerListRequest(storeApi: object, automationId: string): symbol { + let byAutomation = latestListRequest.get(storeApi); + if (!byAutomation) { + byAutomation = new Map(); + latestListRequest.set(storeApi, byAutomation); + } + const token = Symbol(); + byAutomation.set(automationId, token); + return token; +} + +function isLatestListRequest(storeApi: object, automationId: string, token: symbol): boolean { + return latestListRequest.get(storeApi)?.get(automationId) === token; +} + +function clearLatestListRequest(storeApi: object, automationId: string, token: symbol): void { + const byAutomation = latestListRequest.get(storeApi); + if (byAutomation?.get(automationId) === token) { + byAutomation.delete(automationId); + } +} + +type FetchRunsOptions = { + getEpoch: () => number; + setRunsLoading: (automationId: string, loading: boolean) => void; + setRuns: (automationId: string, runs: AutomationRun[]) => void; + onError?: () => void; + onSettled?: () => void; +}; + +function fetchRuns(storeApi: object, automationId: string, options: FetchRunsOptions): void { + const { getEpoch, setRunsLoading, setRuns, onError, onSettled } = options; + const captured = getEpoch(); + const token = registerListRequest(storeApi, automationId); + setRunsLoading(automationId, true); + listAutomationRuns(automationId) + .then((result) => { + // A newer request superseded this one: its result owns the store. + if (!isLatestListRequest(storeApi, automationId, token)) return; + // Discard responses that went stale while in flight: a delete that + // started after this fetch captured the epoch owns the store now. + if (getEpoch() === captured) setRuns(automationId, result ?? []); + }) + .catch(() => { + if (!isLatestListRequest(storeApi, automationId, token)) return; + if (getEpoch() === captured) onError?.(); + }) + .finally(() => { + if (isLatestListRequest(storeApi, automationId, token)) { + clearLatestListRequest(storeApi, automationId, token); + setRunsLoading(automationId, false); + } + onSettled?.(); + }); +} + +/** + * Shared revert for the delete paths: on failure, refresh from the server + * (authoritative — it drops whatever actually succeeded); if that also fails, + * fall back to the pre-delete snapshot so the store is never left + * permanently missing rows the delete never removed. Routed through + * `fetchRuns`, so the recovery participates in loading state, request + * ordering, and the epoch guard, and the serialization slot is held until + * the recovery settles. + */ +function revertAfterFailedDelete( + automationId: string, + fallbackRuns: AutomationRun[], + store: DeleteStore, +): void { + // The delete has settled: mark every list response captured while it was + // in flight stale — those may hold rows the delete removed server-side. + store.bumpEpoch(); + fetchRuns(store.storeApi, automationId, { + getEpoch: store.getEpoch, + setRunsLoading: store.setRunsLoading, + setRuns: store.setRuns, + onError: () => { + store.setRuns(automationId, fallbackRuns); + toast.error(t(COULD_NOT_REFRESH_RUNS)); + }, + onSettled: store.endDelete, + }); +} + +type DeleteStore = { + storeApi: object; + getEpoch: () => number; + bumpEpoch: () => void; + clearRuns: (automationId: string) => void; + removeRun: (automationId: string, runId: string) => void; + restoreRun: (automationId: string, run: AutomationRun) => void; + setRuns: (automationId: string, runs: AutomationRun[]) => void; + setRunsLoading: (automationId: string, loading: boolean) => void; + getRuns: () => AutomationRun[]; + getRun: (runId: string) => AutomationRun | undefined; + endDelete: () => void; +}; + +/** + * Delete-all, scoped to the given run ids when provided. The caller has + * already claimed the per-automation serialization slot (the epoch was + * bumped and the generation recorded), so recoveries can never race a newer + * mutation; the epoch still guards list fetches that predate the delete. On + * success the store is reconciled with an authoritative post-delete refresh + * instead of an unconditional clear, so a run created after the backend + * delete completed survives. + */ +function executeDeleteAll( + automationId: string, + workspaceId: string, + runIds: string[] | undefined, + store: DeleteStore, +): void { + if (runIds && runIds.length === 0) return; + const previousRuns = store.getRuns(); + if (!runIds) { + // Unscoped delete-all: remove every run for the automation in one call. + store.clearRuns(automationId); // optimistic + deleteAllAutomationRuns(automationId, workspaceId) + .then(() => { + // Settled: invalidate list responses captured while the delete was + // in flight (they may hold rows the delete removed), then reconcile + // with the authoritative post-delete list. The serialization slot + // stays held until the reconciliation settles. + store.bumpEpoch(); + fetchRuns(store.storeApi, automationId, { + getEpoch: store.getEpoch, + setRunsLoading: store.setRunsLoading, + setRuns: store.setRuns, + // Surface a failed reconciliation instead of silently keeping the + // optimistic state; the delete itself already succeeded. + onError: () => toast.error(t(COULD_NOT_REFRESH_RUNS)), + onSettled: store.endDelete, + }); + }) + .catch((err: unknown) => { + const msg = err instanceof Error ? err.message : t("automations:failedToDeleteRuns"); + toast.error(msg); + revertAfterFailedDelete(automationId, previousRuns, store); + }); + return; + } + // Status-scoped delete-all: remove exactly the given runs through the + // per-run API. Archived/Cancelled are read-time-derived statuses that only + // exist in the loaded payload, so the visible set is the only exact scope. + const ids = [...new Set(runIds)]; + ids.forEach((id) => store.removeRun(automationId, id)); // optimistic + Promise.allSettled(ids.map((id) => deleteAutomationRun(id, workspaceId))).then((results) => { + if (results.every((result) => result.status === "fulfilled")) { + // Settled: invalidate list responses captured while the deletes were + // in flight, then reconcile with the authoritative post-delete list: + // an in-flight refresh may have returned pre-delete rows, and a run + // created after the deletes completed must not be wiped. The + // serialization slot stays held until the reconciliation settles. + store.bumpEpoch(); + fetchRuns(store.storeApi, automationId, { + getEpoch: store.getEpoch, + setRunsLoading: store.setRunsLoading, + setRuns: store.setRuns, + // Surface a failed reconciliation instead of silently keeping the + // optimistic state; the deletes themselves already succeeded. + onError: () => toast.error(t(COULD_NOT_REFRESH_RUNS)), + onSettled: store.endDelete, + }); + return; + } + // One toast and one recovery refresh for the whole batch, and only after + // every delete has settled: recovering on the first rejection would race + // the still-pending deletes and could resurrect rows that later succeed. + const first = results.find((r): r is PromiseRejectedResult => r.status === "rejected"); + const msg = + first && first.reason instanceof Error + ? first.reason.message + : t("automations:failedToDeleteRuns"); + toast.error(msg); + // The double-failure fallback must not resurrect runs the server actually + // deleted in this batch: drop the succeeded ids from the pre-delete + // snapshot, keeping the failed rows and every untargeted row. + const succeededIds = new Set(ids.filter((_, i) => results[i].status === "fulfilled")); + revertAfterFailedDelete( + automationId, + previousRuns.filter((run) => !succeededIds.has(run.id)), + store, + ); + }); +} + +/** + * Single-run delete. Serialized with delete-all like every other destructive + * mutation (the caller claimed the slot); the epoch bump keeps pre-delete + * list fetches from resurrecting the row, and the failure recovery restores + * precisely the removed run when even the recovery refresh fails. + */ +function executeDeleteRun( + automationId: string, + workspaceId: string, + runId: string, + store: DeleteStore, +): void { + // Snapshot the run being removed so we can restore it precisely (not the + // whole list, which could clobber unrelated concurrent changes) if both + // the delete and the recovery refresh below fail. + const deletedRun = store.getRun(runId); + store.removeRun(automationId, runId); // optimistic + deleteAutomationRun(runId, workspaceId) + .then(() => { + // Re-apply the removal: an in-flight refresh() / initial load can + // resolve between the optimistic removeRun above and this success + // callback and overwrite the store with the pre-delete list, + // resurrecting the row. Removing it again here is a no-op unless + // that happened; the epoch bump then invalidates any list response + // captured while the delete was in flight. + store.removeRun(automationId, runId); + store.bumpEpoch(); + store.endDelete(); + }) + .catch((err: unknown) => { + // An Error message here is an API diagnostic and stays English; the + // fallback for a non-Error rejection is copy. + const msg = err instanceof Error ? err.message : t("automations:failedToDeleteRun"); + toast.error(msg); + store.bumpEpoch(); + fetchRuns(store.storeApi, automationId, { + getEpoch: store.getEpoch, + setRunsLoading: store.setRunsLoading, + setRuns: store.setRuns, + onError: () => { + // The recovery refresh also failed — the store would otherwise + // stay permanently missing this row even though the delete never + // succeeded server-side. Fall back to re-inserting just the run we + // know we removed. + if (deletedRun) store.restoreRun(automationId, deletedRun); + toast.error(t(COULD_NOT_REFRESH_RUNS)); + }, + onSettled: store.endDelete, + }); + }); +} + export function useAutomationRuns(automationId: string | null, workspaceId: string) { const runs = useAppStore((state) => automationId ? (state.automationRuns.byAutomationId[automationId] ?? EMPTY_RUNS) : EMPTY_RUNS, @@ -20,6 +273,9 @@ export function useAutomationRuns(automationId: string | null, workspaceId: stri const loading = useAppStore((state) => automationId ? (state.automationRuns.loading[automationId] ?? false) : false, ); + const deleting = useAppStore((state) => + automationId ? (state.automationRuns.deleting[automationId] ?? false) !== false : false, + ); const setRuns = useAppStore((state) => state.setAutomationRuns); const setRunsLoading = useAppStore((state) => state.setAutomationRunsLoading); const removeRun = useAppStore((state) => state.removeAutomationRun); @@ -27,104 +283,103 @@ export function useAutomationRuns(automationId: string | null, workspaceId: stri const restoreRun = useAppStore((state) => state.restoreAutomationRun); const storeApi = useAppStoreApi(); + // Destructive mutations are serialized per automation in shared store + // state: only one may be in flight, so a recovery can never race a newer + // mutation — even across hook instances (an editor can unmount mid-delete + // and remount). `deleting` is exposed so the UI disables the delete + // controls while one runs. + const beginDelete = useCallback(() => { + if (!automationId) return null; + return storeApi.getState().beginAutomationRunDelete(automationId); + }, [automationId, storeApi]); + const endDelete = useCallback( + (generation: number) => { + if (!automationId) return; + storeApi.getState().endAutomationRunDelete(automationId, generation); + }, + [automationId, storeApi], + ); + useEffect(() => { if (!automationId || loading) return; - setRunsLoading(automationId, true); - listAutomationRuns(automationId) - .then((result) => { - setRuns(automationId, result ?? []); - }) - .catch(() => { - setRuns(automationId, []); - }) - .finally(() => { - setRunsLoading(automationId, false); - }); + // A delete owns the store while it is in flight: its reconciliation or + // recovery is the only list request, so it can never be superseded by a + // mount fetch whose failure would skip the recovery's snapshot restore. + if ((storeApi.getState().automationRuns.deleting[automationId] ?? false) !== false) return; + fetchRuns(storeApi, automationId, { + getEpoch: () => storeApi.getState().automationRuns.mutationEpoch[automationId] ?? 0, + setRunsLoading, + setRuns, + onError: () => setRuns(automationId, []), + }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [automationId]); const refresh = useCallback(() => { if (!automationId) return; - setRunsLoading(automationId, true); - listAutomationRuns(automationId) - .then((result) => { - setRuns(automationId, result ?? []); - }) - .catch(() => {}) - .finally(() => { - setRunsLoading(automationId, false); - }); - }, [automationId, setRuns, setRunsLoading]); + // See the mount effect: while a delete is in flight its reconciliation / + // recovery owns the list state and must not be superseded. + if ((storeApi.getState().automationRuns.deleting[automationId] ?? false) !== false) return; + fetchRuns(storeApi, automationId, { + getEpoch: () => storeApi.getState().automationRuns.mutationEpoch[automationId] ?? 0, + setRunsLoading, + setRuns, + }); + }, [automationId, setRuns, setRunsLoading, storeApi]); + + const makeStore = useCallback( + (end: () => void): DeleteStore => ({ + storeApi, + getEpoch: () => storeApi.getState().automationRuns.mutationEpoch[automationId ?? ""] ?? 0, + bumpEpoch: () => storeApi.getState().advanceAutomationRunEpoch(automationId ?? ""), + clearRuns, + removeRun, + restoreRun, + setRuns, + setRunsLoading, + getRuns: () => storeApi.getState().automationRuns.byAutomationId[automationId ?? ""] ?? [], + getRun: (runId) => + storeApi + .getState() + .automationRuns.byAutomationId[automationId ?? ""]?.find((r) => r.id === runId), + endDelete: end, + }), + [clearRuns, removeRun, restoreRun, setRuns, setRunsLoading, storeApi, automationId], + ); const deleteRun = useCallback( (runId: string) => { if (!automationId) return; - // Snapshot the run being removed so we can restore it precisely (not - // the whole list, which could clobber unrelated concurrent changes) - // if both the delete and the recovery refresh below fail. - const deletedRun = storeApi - .getState() - .automationRuns.byAutomationId[automationId]?.find((r) => r.id === runId); - removeRun(automationId, runId); // optimistic - deleteAutomationRun(runId, workspaceId) - .then(() => { - // Re-apply the removal: an in-flight refresh() / initial load can - // resolve between the optimistic removeRun above and this success - // callback and overwrite the store with the pre-delete list, - // resurrecting the row. Removing it again here is a no-op unless - // that happened. - removeRun(automationId, runId); - }) - .catch((err: unknown) => { - // An Error message here is an API diagnostic and stays English; the - // fallback for a non-Error rejection is copy. - const msg = err instanceof Error ? err.message : t("automations:failedToDeleteRun"); - toast.error(msg); - // revert on failure - listAutomationRuns(automationId) - .then((result) => setRuns(automationId, result ?? [])) - .catch(() => { - // The recovery refresh also failed — the store would - // otherwise stay permanently missing this row even though - // the delete never succeeded server-side. Fall back to - // re-inserting just the run we know we removed. - if (deletedRun) { - restoreRun(automationId, deletedRun); - } - toast.error(t("automations:couldNotRefreshRuns")); - }); - }); + const generation = beginDelete(); + if (generation === null) return; + executeDeleteRun( + automationId, + workspaceId, + runId, + makeStore(() => endDelete(generation)), + ); }, - [automationId, removeRun, restoreRun, setRuns, storeApi, workspaceId], + [automationId, beginDelete, endDelete, makeStore, workspaceId], ); - const deleteAllRuns = useCallback(() => { - if (!automationId) return; - // Snapshot the full list so we can restore it if both the delete-all - // and the recovery refresh below fail. - const previousRuns = storeApi.getState().automationRuns.byAutomationId[automationId] ?? []; - clearRuns(automationId); // optimistic - deleteAllAutomationRuns(automationId, workspaceId) - .then(() => { - // See deleteRun: guard against an in-flight refresh() resurrecting - // rows between the optimistic clear and this success callback. - clearRuns(automationId); - }) - .catch((err: unknown) => { - const msg = err instanceof Error ? err.message : t("automations:failedToDeleteRuns"); - toast.error(msg); - // revert on failure - listAutomationRuns(automationId) - .then((result) => setRuns(automationId, result ?? [])) - .catch(() => { - // The recovery refresh also failed — without this the store - // would stay permanently empty even though delete-all never - // succeeded server-side. Fall back to the pre-clear snapshot. - setRuns(automationId, previousRuns); - toast.error(t("automations:couldNotRefreshRuns")); - }); - }); - }, [automationId, clearRuns, setRuns, storeApi, workspaceId]); + const deleteAllRuns = useCallback( + (runIds?: string[]) => { + if (!automationId) return; + // An empty scope is a no-op: the UI never offers delete-all for an + // empty view, and falling back to the all-runs delete here would be a + // dangerous surprise for a caller that computed zero ids. + if (runIds && runIds.length === 0) return; + const generation = beginDelete(); + if (generation === null) return; + executeDeleteAll( + automationId, + workspaceId, + runIds, + makeStore(() => endDelete(generation)), + ); + }, + [automationId, beginDelete, endDelete, makeStore, workspaceId], + ); - return { runs, loading, refresh, deleteRun, deleteAllRuns }; + return { runs, loading, refresh, deleteRun, deleteAllRuns, deleting }; } diff --git a/apps/web/lib/state/slices/automations/automations-slice.ts b/apps/web/lib/state/slices/automations/automations-slice.ts index 37e578664d..e013b6a210 100644 --- a/apps/web/lib/state/slices/automations/automations-slice.ts +++ b/apps/web/lib/state/slices/automations/automations-slice.ts @@ -3,7 +3,7 @@ import type { AutomationsSlice, AutomationsSliceState } from "./types"; export const defaultAutomationsState: AutomationsSliceState = { automations: { items: [], loaded: false, loading: false }, - automationRuns: { byAutomationId: {}, loading: {} }, + automationRuns: { byAutomationId: {}, loading: {}, mutationEpoch: {}, deleting: {} }, }; type ImmerSet = Parameters< @@ -50,6 +50,7 @@ function createAutomationsActions( function createRunsActions( set: ImmerSet, + get: () => AutomationsSlice, ): Pick< AutomationsSlice, | "setAutomationRuns" @@ -57,6 +58,9 @@ function createRunsActions( | "removeAutomationRun" | "clearAutomationRuns" | "restoreAutomationRun" + | "beginAutomationRunDelete" + | "endAutomationRunDelete" + | "advanceAutomationRunEpoch" > { return { setAutomationRuns: (automationId, runs) => @@ -96,6 +100,27 @@ function createRunsActions( } draft.automationRuns.byAutomationId[automationId] = next; }), + beginAutomationRunDelete: (automationId) => { + // `?? false` treats the never-started automation (absent key) as idle. + if ((get().automationRuns.deleting[automationId] ?? false) !== false) return null; + const next = (get().automationRuns.mutationEpoch[automationId] ?? 0) + 1; + set((draft) => { + draft.automationRuns.mutationEpoch[automationId] = next; + draft.automationRuns.deleting[automationId] = next; + }); + return next; + }, + endAutomationRunDelete: (automationId, generation) => + set((draft) => { + if (draft.automationRuns.deleting[automationId] === generation) { + draft.automationRuns.deleting[automationId] = false; + } + }), + advanceAutomationRunEpoch: (automationId) => + set((draft) => { + draft.automationRuns.mutationEpoch[automationId] = + (draft.automationRuns.mutationEpoch[automationId] ?? 0) + 1; + }), }; } @@ -104,8 +129,8 @@ export const createAutomationsSlice: StateCreator< [["zustand/immer", never]], [], AutomationsSlice -> = (set, _get, _api) => ({ +> = (set, get, _api) => ({ ...defaultAutomationsState, ...createAutomationsActions(set), - ...createRunsActions(set), + ...createRunsActions(set, get), }); diff --git a/apps/web/lib/state/slices/automations/types.ts b/apps/web/lib/state/slices/automations/types.ts index 737a9e6f48..0eed52cf61 100644 --- a/apps/web/lib/state/slices/automations/types.ts +++ b/apps/web/lib/state/slices/automations/types.ts @@ -9,6 +9,19 @@ export type AutomationsState = { export type AutomationRunsState = { byAutomationId: Record; loading: Record; + /** + * Monotonic delete-generation counter per automation, shared by every hook + * instance (an editor can unmount mid-delete and remount). List responses + * that captured an older generation are stale and must be discarded. + */ + mutationEpoch: Record; + /** + * Generation of the delete mutation currently in flight per automation, + * or `false` when none. A mutation's `endDelete` only clears its own + * generation, so a stale callback from an unmounted instance cannot lift + * a newer instance's serialization gate. + */ + deleting: Record; }; export type AutomationsSliceState = { @@ -27,6 +40,22 @@ export type AutomationsSliceActions = { removeAutomationRun: (automationId: string, runId: string) => void; clearAutomationRuns: (automationId: string) => void; restoreAutomationRun: (automationId: string, run: AutomationRun) => void; + /** + * Atomically claims the per-automation delete serialization slot: bumps + * the mutation epoch, records the new generation as in-flight, and returns + * that generation, or `null` when another delete is already in flight. + */ + beginAutomationRunDelete: (automationId: string) => number | null; + /** Releases the serialization slot if (and only if) it still holds the + * given generation — a stale callback from an unmounted hook instance + * cannot clear a newer mutation's slot. */ + endAutomationRunDelete: (automationId: string, generation: number) => void; + /** + * Advances the mutation epoch without claiming the serialization slot. A + * delete calls this when it settles so list responses that were captured + * while it was in flight go stale — they may hold pre-delete rows. + */ + advanceAutomationRunEpoch: (automationId: string) => void; }; export type AutomationsSlice = AutomationsSliceState & AutomationsSliceActions; diff --git a/apps/web/lib/state/store.test.ts b/apps/web/lib/state/store.test.ts index 1d2cba99df..c47f0e46e0 100644 --- a/apps/web/lib/state/store.test.ts +++ b/apps/web/lib/state/store.test.ts @@ -82,3 +82,49 @@ describe("createAppStore", () => { expect((store.getState().connection as { issueSeverity?: string }).issueSeverity).toBe("none"); }); }); + +describe("automation run delete serialization", () => { + it("claims the delete slot once and releases it only for the matching generation", () => { + const store = createAppStore(); + + // Absent key means idle: the first begin bumps the epoch and claims gen 1. + expect(store.getState().beginAutomationRunDelete("auto-1")).toBe(1); + expect(store.getState().automationRuns.mutationEpoch["auto-1"]).toBe(1); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(1); + + // While active, further claims are rejected. + expect(store.getState().beginAutomationRunDelete("auto-1")).toBeNull(); + + // Stale generations cannot release the slot. + store.getState().endAutomationRunDelete("auto-1", 0); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(1); + store.getState().endAutomationRunDelete("auto-1", 99); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(1); + + // The matching generation releases it. + store.getState().endAutomationRunDelete("auto-1", 1); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(false); + + // A stale end from an unmounted instance cannot clear a newer slot. + expect(store.getState().beginAutomationRunDelete("auto-1")).toBe(2); + store.getState().endAutomationRunDelete("auto-1", 1); // stale + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(2); + store.getState().endAutomationRunDelete("auto-1", 2); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(false); + }); + + it("advances the epoch without touching the delete slot", () => { + const store = createAppStore(); + + expect(store.getState().beginAutomationRunDelete("auto-1")).toBe(1); + store.getState().advanceAutomationRunEpoch("auto-1"); + expect(store.getState().automationRuns.mutationEpoch["auto-1"]).toBe(2); + // The slot is untouched and still holds its own generation. + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(1); + store.getState().endAutomationRunDelete("auto-1", 1); + expect(store.getState().automationRuns.deleting["auto-1"]).toBe(false); + + // A different automation has its own independent counter. + expect(store.getState().beginAutomationRunDelete("auto-2")).toBe(1); + }); +}); diff --git a/apps/web/src/locales/en/automations.json b/apps/web/src/locales/en/automations.json index bcc580eb7b..e6b23086aa 100644 --- a/apps/web/src/locales/en/automations.json +++ b/apps/web/src/locales/en/automations.json @@ -46,6 +46,9 @@ "deleteAll": "Delete all", "deleteAllRuns": "Delete all runs", "deleteAllRunsDescription": "This will permanently remove all run records for this automation; including any not currently loaded; and their associated tasks. This cannot be undone.", + "deleteAllRunsScoped": "Delete all {{status}} runs", + "deleteAllRunsScopedDescription": "This will permanently remove the {{status}} runs shown in this view and their associated tasks. This cannot be undone.", + "deleteAllRunsScopedTitle": "Delete all {{status}} runs?", "deleteAllRunsTitle": "Delete all runs?", "deleteRun": "Delete run", "details": "Details", diff --git a/apps/web/src/locales/pseudo/automations.json b/apps/web/src/locales/pseudo/automations.json index 4057d78e8d..3964e9118b 100644 --- a/apps/web/src/locales/pseudo/automations.json +++ b/apps/web/src/locales/pseudo/automations.json @@ -46,6 +46,9 @@ "deleteAll": "Ďēĺēţē àĺĺ", "deleteAllRuns": "Ďēĺēţē àĺĺ ŕũńś", "deleteAllRunsDescription": "Ţĥĩś ŵĩĺĺ ƥēŕḿàńēńţĺŷ ŕēḿōvē àĺĺ ŕũń ŕēćōŕďś ƒōŕ ţĥĩś àũţōḿàţĩōń; ĩńćĺũďĩńĝ àńŷ ńōţ ćũŕŕēńţĺŷ ĺōàďēď; àńď ţĥēĩŕ àśśōćĩàţēď ţàśķś. Ţĥĩś ćàńńōţ ƀē ũńďōńē.", + "deleteAllRunsScoped": "Ďēĺēţē àĺĺ {{status}} ŕũńś", + "deleteAllRunsScopedDescription": "Ţĥĩś ŵĩĺĺ ƥēŕḿàńēńţĺŷ ŕēḿōvē ţĥē {{status}} ŕũńś śĥōŵń ĩń ţĥĩś vĩēŵ àńď ţĥēĩŕ àśśōćĩàţēď ţàśķś. Ţĥĩś ćàńńōţ ƀē ũńďōńē.", + "deleteAllRunsScopedTitle": "Ďēĺēţē àĺĺ {{status}} ŕũńś?", "deleteAllRunsTitle": "Ďēĺēţē àĺĺ ŕũńś?", "deleteRun": "Ďēĺēţē ŕũń", "details": "Ďēţàĩĺś", diff --git a/apps/web/src/locales/pt-pt/automations.json b/apps/web/src/locales/pt-pt/automations.json index 42c4e62474..5cc411b508 100644 --- a/apps/web/src/locales/pt-pt/automations.json +++ b/apps/web/src/locales/pt-pt/automations.json @@ -46,6 +46,9 @@ "deleteAll": "Eliminar todas", "deleteAllRuns": "Eliminar todas as execuções", "deleteAllRunsDescription": "Isto remove permanentemente todos os registos de execução desta automatização; incluindo os que não estão carregados neste momento; e as tarefas associadas. Esta ação não pode ser anulada.", + "deleteAllRunsScoped": "Eliminar todas as execuções {{status}}", + "deleteAllRunsScopedDescription": "Isto remove permanentemente as execuções {{status}} mostradas nesta vista e as tarefas associadas. Esta ação não pode ser anulada.", + "deleteAllRunsScopedTitle": "Eliminar todas as execuções {{status}}?", "deleteAllRunsTitle": "Eliminar todas as execuções?", "deleteRun": "Eliminar execução", "details": "Detalhes", diff --git a/apps/web/src/locales/zh-cn/automations.json b/apps/web/src/locales/zh-cn/automations.json index c2e3476bf9..8a5928bc15 100644 --- a/apps/web/src/locales/zh-cn/automations.json +++ b/apps/web/src/locales/zh-cn/automations.json @@ -46,6 +46,9 @@ "deleteAll": "全部删除", "deleteAllRuns": "删除所有运行记录", "deleteAllRunsDescription": "这将永久删除此自动化的所有运行记录; 包括当前未加载的记录; 以及关联的任务。此操作无法撤销。", + "deleteAllRunsScoped": "删除所有{{status}}运行记录", + "deleteAllRunsScopedDescription": "这将永久删除此视图中显示的所有{{status}}运行记录以及关联的任务。此操作无法撤销。", + "deleteAllRunsScopedTitle": "删除所有{{status}}运行记录？", "deleteAllRunsTitle": "删除所有运行记录？", "deleteRun": "删除运行记录", "details": "详情", diff --git a/docs/plans/automation-runs-delete-all-by-status/plan.md b/docs/plans/automation-runs-delete-all-by-status/plan.md new file mode 100644 index 0000000000..eb153f9643 --- /dev/null +++ b/docs/plans/automation-runs-delete-all-by-status/plan.md @@ -0,0 +1,182 @@ +--- +spec: docs/specs/automation-runs-delete-all-by-status/spec.md +created: 2026-08-12 +status: completed +--- + +# Implementation Plan: Status-scoped delete-all for automation runs + +## Overview + +Scope the Recent Runs delete-all to the active status view and move the +control into the table header's rightmost column. The frontend hook gains an +optional run-id list on `deleteAllRuns`; a filtered view deletes exactly the +visible runs through the existing per-run `automation.run.delete` path, while +the "All" view keeps the single `automation.runs.delete_all` call. No backend +changes: `Archived`/`Cancelled` are read-time-derived statuses that only exist +in the loaded payload, so the visible set is the only exact scope. + +Adversarial review (rounds 1-3) hardened the delete flow: list fetches are +epoch-guarded against stale responses, delete mutations are serialized per +automation in shared store state (generation-tagged, so an unmounted editor +instance cannot clobber a remounted one), partial batch failures recover only +after every delete settles, and success paths reconcile with an authoritative +post-delete refresh instead of a blanket clear. + +## Backend + +None. The per-run delete path already deletes each run's associated task, and +per-run deletes are id-targeted so the orphaned-task race guarded by +`DeleteAllRuns`' per-automation run lock (a broad DELETE catching a +concurrently-created run) cannot occur. + +## Frontend + +### Hook — `apps/web/hooks/domains/settings/use-automation-runs.ts` + +Extend `deleteAllRuns(runIds?: string[])`: + +- No argument → unchanged: optimistic `clearRuns(automationId)` + one + `deleteAllAutomationRuns(automationId, workspaceId)` call; on success the + store is reconciled with an authoritative post-delete `fetchRuns` (not a + blanket re-clear, so a run created after the backend delete completed + survives); on failure → toast + recovery refresh, refresh failure → + pre-clear snapshot restore. +- Empty array → no-op (defensive; the UI never renders the button for an + empty view). +- Non-empty ids → snapshot the pre-delete list, `removeRun` each id + optimistically, then `Promise.allSettled(ids.map((id) => deleteAutomationRun( + id, workspaceId)))`. All fulfilled → authoritative post-delete refresh. Any + rejection → one aggregated toast + recovery refresh, run only after every + delete settles (never on the first rejection); refresh failure → snapshot + restore. Aggregated: one toast, one refresh per failed batch, not per id. + +Race hardening (added during adversarial review): + +- **Epoch-guarded fetches.** The store holds a per-automation `mutationEpoch` + (`automationRuns.mutationEpoch`). Every list fetch captures the epoch at + start and discards its response if a delete bumped it meanwhile — a refresh + started before a delete can never resurrect deleted rows. +- **Serialized mutations.** `automationRuns.deleting` holds the in-flight + mutation's generation (or `false`). `beginAutomationRunDelete` atomically + bumps the epoch and claims the slot; `deleteRun`/`deleteAllRuns` are no-ops + while it is held; `endAutomationRunDelete(generation)` releases only its own + generation. The flag is shared store state, so an editor that unmounts + mid-delete cannot leave a remounted instance unsynchronized, and a stale + callback cannot lift a newer mutation's slot. +- The hook exposes `deleting` so the UI disables both the delete-all button + and the per-row delete buttons while a delete is in flight. + +### Store slice — `apps/web/lib/state/slices/automations/` + +`AutomationRunsState` gains `mutationEpoch: Record` and +`deleting: Record`, plus +`beginAutomationRunDelete` / `endAutomationRunDelete` actions. No migration +required; the fields default to `{}`. + +### Component — `apps/web/components/automations/runs-section.tsx` + +- Remove the `DeleteAllButton` from the section header row (next to the + refresh button). +- Render it inside the last `` (the column holding + the per-row delete buttons), gated on `visibleRuns.length > 0` and + `expanded` (the table only renders when expanded): + `onConfirm={() => deleteAllRuns(statusFilter === "all" ? undefined : visibleRuns.map((r) => r.id))}`. +- `DeleteAllButton` gains the current filter so the dialog picks its copy: + - "all" → existing `deleteAllRunsTitle` / `deleteAllRunsDescription` + (e2e asserts `permanently remove all run records`). + - filtered → new `deleteAllRunsScopedTitle` / `deleteAllRunsScopedDescription` + with `{{status}}` = the localized status label (`t` of the matching + `STATUS_FILTERS` entry's `labelKey`, e.g. "Skipped", "Archived"). + +### Locales — `apps/web/src/locales/{en,pseudo,pt-pt,zh-cn}/automations.json` + +Add, in all four catalogs (plain punctuation, no em dash): + +- `deleteAllRunsScopedTitle`: `Delete all {{status}} runs?` +- `deleteAllRunsScopedDescription`: `This will permanently remove the {{status}} runs shown in this view and their associated tasks. This cannot be undone.` + +## Tests + +- **What:** scoped delete-all removes exactly the given run ids and calls the + per-run API per id. + **File:** `apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts` + **How:** mock `deleteAutomationRun`; assert per-id calls with + `WORKSPACE_ID`, optimistic removal of exactly those ids, success + reconciliation with the authoritative post-delete list (a run created + after the deletes completed survives), single aggregated failure toast + + recovery refresh, and double-failure snapshot restore. Add a no-op + assertion for `deleteAllRuns([])`. +- **What:** delete-all lives in the table header and is view-scoped. + **File:** `apps/web/components/automations/runs-section.test.tsx` + **How:** assert the button renders inside the table header (not beside the + Recent Runs heading); filtered view confirm calls `deleteAllRuns` with the + visible ids; "All" view calls `deleteAllRuns()` with no ids; empty filtered + view renders no button; filtered dialog shows the scoped title/description; + both delete controls are disabled while `deleting` is true. +- **What:** the "All" view still uses the single bulk delete. + **File:** `apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts` + **How:** existing no-arg tests already assert `deleteAllAutomationRuns` is + called with `(AUTOMATION_ID, WORKSPACE_ID)`; they must keep passing + unchanged. +- **What:** delete race conditions stay closed. + **File:** `apps/web/hooks/domains/settings/use-automation-runs-delete.test.ts` + **How:** stale refresh started before a delete and resolving after it is + discarded; partial batch failure does not recover until every delete + settles; a second delete-all or per-run delete fired while one is in flight + is a no-op; the serialization slot is shared across hook instances (an + unmounted instance's endDelete releases it); every recovery terminal path + resets `deleting` to `false`; older list responses cannot overwrite newer + ones. + +## E2E Tests + +- **Scenario:** Skipped-filtered delete-all removes only the skipped rows. + **File:** `apps/web/e2e/tests/automations-settings.spec.ts` + **What to verify:** seed 2 skipped + 1 succeeded run; expand Recent Runs; + filter Skipped (2 rows); assert `delete-all-runs` sits inside the table + header; click it and confirm; dialog shows the scoped copy + (`permanently remove the Skipped runs shown in this view`); table shows 1 + row; switch to All and assert the succeeded row remains. +- **Scenario:** All-view delete-all keeps the existing copy and placement. + **File:** `apps/web/e2e/tests/automations-settings.spec.ts` + **What to verify:** the existing "delete individual and all runs from + Recent Runs" test keeps passing unchanged (it already asserts header + visibility and the unqualified dialog copy). + +## Verification Results + +- Hook + component + store unit suites: `pnpm --filter @kandev/web test -- + --run hooks/domains/settings/use-automation-runs.test.ts + hooks/domains/settings/use-automation-runs-delete.test.ts + components/automations/runs-section.test.tsx lib/state/store.test.ts` — + 48 tests passed (final state after review rounds). +- `pnpm --filter @kandev/web typecheck` (from `apps/web`) — passed. +- `pnpm --filter @kandev/web lint -- ` (eslint + `--max-warnings 0`) — passed. +- `pnpm --filter @kandev/web i18n:check` + `i18n:ratchet` — passed; new keys + in en/pseudo/pt-pt/zh-cn, pseudo regenerated, no em dashes. +- E2E: `pnpm --filter @kandev/web e2e:raw -g "delete" --workers=4 + automations-settings.spec.ts` — delete-related tests passed; the full + chromium project suite passed on an earlier run. +- `git diff --check` — passed. Pre-commit hooks (prettier, web lint, i18n + guard, commitlint) passed on every commit. + +## Implementation Waves And Parallel Candidates + +Wave 1 (sequential): + +- [x] [task-01-hook-status-scoped-delete](task-01-hook-status-scoped-delete.md) — done +- [x] [task-02-runs-table-header-delete-all](task-02-runs-table-header-delete-all.md) — done +- [x] [task-03-e2e-status-scoped-delete](task-03-e2e-status-scoped-delete.md) — done + +The hook and the component are a vertical slice (the component consumes the +hook's new signature in the same change cycle); E2E follows both. Eight +adversarial review rounds (Luna, one fresh sub-task per round) ran after +implementation; each round's findings were fixed, re-verified, and committed +before the next. Round 8 reported `NO NEW FINDINGS` with a ready-to-merge +verdict, ending the loop. + +## Open Questions + +None. diff --git a/docs/plans/automation-runs-delete-all-by-status/task-01-hook-status-scoped-delete.md b/docs/plans/automation-runs-delete-all-by-status/task-01-hook-status-scoped-delete.md new file mode 100644 index 0000000000..c17eabf42b --- /dev/null +++ b/docs/plans/automation-runs-delete-all-by-status/task-01-hook-status-scoped-delete.md @@ -0,0 +1,91 @@ +--- +id: "01-hook-status-scoped-delete" +title: "Hook status-scoped delete-all" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/automation-runs-delete-all-by-status/spec.md" +--- + +# Task 01: Hook status-scoped delete-all + +Extend `useAutomationRuns.deleteAllRuns` so a filtered view can delete exactly +the runs it shows, while the unfiltered call keeps the single bulk-delete +behavior. + +## Acceptance + +- `deleteAllRuns(runIds)` removes exactly those run ids from the store + optimistically and calls `deleteAutomationRun(id, workspaceId)` once per id. +- On success it re-applies the removal (guarding against an in-flight refresh + resurrecting rows), mirroring the existing `deleteRun` guard. +- When any delete in the batch fails: one `toast.error` and one recovery + `listAutomationRuns` refresh; if the recovery refresh also fails, the + pre-delete snapshot is restored with a `couldNotRefreshRuns` toast. One + toast and one refresh per failed batch, not per id. +- `deleteAllRuns()` (no argument) keeps calling `deleteAllAutomationRuns( + automationId, workspaceId)` exactly once with the existing clear/revert + semantics — existing hook tests pass unchanged. +- `deleteAllRuns([])` is a no-op: no store mutation, no API call. + +## Verification + +From `apps/` (fresh worktrees must first run `pnpm install --frozen-lockfile` +from `apps/` when `apps/node_modules/` is absent): + +```bash +pnpm --filter @kandev/web test -- --run hooks/domains/settings/use-automation-runs.test.ts hooks/domains/settings/use-automation-runs-delete.test.ts +``` + +Also run `git diff --check` and Prettier on the changed files before commit. + +## Files likely touched + +- `apps/web/hooks/domains/settings/use-automation-runs.ts` +- `apps/web/hooks/domains/settings/use-automation-runs.test.ts` + +## Dependencies + +None. + +## Parallelism + +Sequential. The production callback and its unit tests share one behavior and +belong in one TDD cycle. + +## Inputs + +- Spec scenarios: filtered delete-all scope, failure revert contract. +- Existing patterns in `use-automation-runs.ts`: `deleteRun`'s optimistic + remove + success re-remove + failure refresh + snapshot restore; + `deleteAllRuns`' aggregated toast/revert shape. Reuse the store actions + already exposed (`removeRun`, `setRuns`) — do not add a store slice action. +- The `DeleteAllButton` gating in `runs-section.tsx` is out of scope here + (task 02); the component may still call `deleteAllRuns()` today. + +## Output contract + +Report the RED assertion, files changed, the exact focused command and +result, any blockers or risks, and synchronized task/plan status in the same +conversation. No backend, store-slice, or API-layer changes. + +## Results + +- RED: new race-condition tests failed before the guards existed (stale + refresh resurrecting deleted rows; early recovery on first rejection). +- GREEN: `pnpm --filter @kandev/web test -- --run + hooks/domains/settings/use-automation-runs.test.ts + hooks/domains/settings/use-automation-runs-delete.test.ts + components/automations/runs-section.test.tsx lib/state/store.test.ts` — + 48 tests passed (final state; the suite was split across the two hook + test files as it grew). +- Hardening added during review rounds: `fetchRuns`/`revertAfterFailedDelete` + are epoch-guarded; `executeDeleteAll` uses `Promise.allSettled` and + recovers only after every delete settles; destructive mutations are + serialized via generation-tagged store state + (`beginAutomationRunDelete`/`endAutomationRunDelete`, + `automationRuns.mutationEpoch`/`deleting`) so overlapping or cross-instance + deletes cannot race; `executeDeleteRun` joins the same mechanism; success + paths reconcile with an authoritative post-delete refresh. +- `git diff --check` and Prettier — passed. No backend or API-layer changes. diff --git a/docs/plans/automation-runs-delete-all-by-status/task-02-runs-table-header-delete-all.md b/docs/plans/automation-runs-delete-all-by-status/task-02-runs-table-header-delete-all.md new file mode 100644 index 0000000000..e4f38c37d1 --- /dev/null +++ b/docs/plans/automation-runs-delete-all-by-status/task-02-runs-table-header-delete-all.md @@ -0,0 +1,103 @@ +--- +id: "02-runs-table-header-delete-all" +title: "Runs table header delete-all" +status: done +wave: 2 +depends_on: + - "01-hook-status-scoped-delete" +plan: "plan.md" +spec: "../../specs/automation-runs-delete-all-by-status/spec.md" +--- + +# Task 02: Runs table header delete-all + +Move the delete-all control into the Recent Runs table header's rightmost +column and scope its confirmation to the active status view. + +## Acceptance + +- The delete-all button renders inside the last `` + (the per-row delete column), horizontally aligned with the per-row delete + buttons, and no longer appears in the section header row next to Refresh. +- With a status filter active, confirming the dialog calls + `deleteAllRuns(visibleRuns.map((r) => r.id))`; with the "All" filter it + calls `deleteAllRuns()` with no argument. +- The button is hidden when `visibleRuns.length === 0` (including the "No + runs match this filter" empty view). +- Dialog copy: the existing `deleteAllRunsTitle` / `deleteAllRunsDescription` + for "All"; the new scoped keys with the localized status label + (`t` of the active `STATUS_FILTERS` entry's `labelKey`) when filtered. +- New locale keys `deleteAllRunsScopedTitle` and + `deleteAllRunsScopedDescription` exist in all four catalogs + (`en`, `pseudo`, `pt-pt`, `zh-cn`), plain punctuation, no em dash. + +## Verification + +From `apps/` (fresh worktrees must first run `pnpm install --frozen-lockfile` +from `apps/` when `apps/node_modules/` is absent): + +```bash +pnpm --filter @kandev/web test -- --run components/automations/runs-section.test.tsx hooks/domains/settings/use-automation-runs.test.ts +``` + +Then from `apps/web/`: + +```bash +pnpm run typecheck +pnpm run i18n:check +pnpm run i18n:ratchet +pnpm run lint -- components/automations/runs-section.tsx components/automations/runs-section.test.tsx +``` + +Also run `git diff --check` and Prettier on the changed files before commit. + +## Files likely touched + +- `apps/web/components/automations/runs-section.tsx` +- `apps/web/components/automations/runs-section.test.tsx` +- `apps/web/src/locales/en/automations.json` +- `apps/web/src/locales/pseudo/automations.json` +- `apps/web/src/locales/pt-pt/automations.json` +- `apps/web/src/locales/zh-cn/automations.json` + +## Dependencies + +Task 01 (the component consumes the hook's new optional run-id argument). + +## Parallelism + +Sequential. Component behavior and its rendered regression tests share one +change cycle. + +## Inputs + +- Spec scenarios: header placement, view-scoped deletion, empty-view + hiding, scoped dialog copy. +- `runs-section.tsx` current structure: `visibleRuns` / `statusFilter` + state, the `STATUS_FILTERS` list (status → `labelKey`), the 5-column table + with the trailing `w-8` header cell, and the existing + `DeleteAllButton`/`AlertDialog` markup to relocate. +- i18n conventions in `apps/web/AGENTS.md`: all new copy through `t()`, + all four catalogs updated, no em dash in user-facing copy. + +## Output contract + +Report the RED assertion, files changed, the exact focused commands and +results, any blockers or risks, and synchronized task/plan status in the same +conversation. No backend changes. + +## Results + +- RED: new scope/placement tests failed before the component change (button + still beside the heading, no scoped ids, no scoped copy). +- GREEN: `pnpm --filter @kandev/web test -- --run + components/automations/runs-section.test.tsx + hooks/domains/settings/use-automation-runs.test.ts` — 34 tests passed + (final state). +- Added during review rounds: status-scoped `title`/`aria-label` + (`deleteAllRunsScoped` key), and `deleting`-gated disablement of both the + delete-all button and the per-row delete buttons. +- `pnpm run typecheck` (from `apps/web`) — passed; `i18n:check` + + `i18n:ratchet` — passed (keys in all four catalogs, pseudo regenerated); + changed-file eslint `--max-warnings 0` — passed; `git diff --check` — + passed. diff --git a/docs/plans/automation-runs-delete-all-by-status/task-03-e2e-status-scoped-delete.md b/docs/plans/automation-runs-delete-all-by-status/task-03-e2e-status-scoped-delete.md new file mode 100644 index 0000000000..3eedabf0ad --- /dev/null +++ b/docs/plans/automation-runs-delete-all-by-status/task-03-e2e-status-scoped-delete.md @@ -0,0 +1,76 @@ +--- +id: "03-e2e-status-scoped-delete" +title: "E2E status-scoped delete" +status: done +wave: 3 +depends_on: + - "02-runs-table-header-delete-all" +plan: "plan.md" +spec: "../../specs/automation-runs-delete-all-by-status/spec.md" +--- + +# Task 03: E2E status-scoped delete + +Prove the user-facing flow end to end: delete-all in a filtered view removes +only that view's runs, from the new table-header position. + +## Acceptance + +- New test in `apps/web/e2e/tests/automations-settings.spec.ts`: seed an + automation with 2 skipped and 1 succeeded run; expand Recent Runs; click + the Skipped filter (2 rows); assert the delete-all button lives inside the + table header (``); click it and confirm the dialog shows the scoped + copy (`permanently remove the Skipped runs shown in this view`); assert the + table drops to 1 row; switch to the All filter and assert the succeeded run + remains. +- The existing "delete individual and all runs from Recent Runs" test keeps + passing unchanged (it already asserts header visibility of + `delete-all-runs` and the unqualified All-view dialog copy). + +## Verification + +From `apps/` (fresh worktrees must first run `pnpm install --frozen-lockfile` +from `apps/` when `apps/node_modules/` is absent): + +```bash +pnpm --filter @kandev/web e2e:raw -- automations-settings.spec.ts +``` + +Also run `git diff --check` and Prettier on the changed files before commit. + +## Files likely touched + +- `apps/web/e2e/tests/automations-settings.spec.ts` + +## Dependencies + +Tasks 01 and 02 (the E2E exercises both the hook scope and the header +placement). + +## Parallelism + +Sequential. Runs after the frontend tasks land. + +## Inputs + +- Spec scenarios: Skipped-filtered delete-all, All-view delete-all. +- Existing patterns in `automations-settings.spec.ts`: + `apiClient.seedAutomation` / `seedAutomationRun`, the settings-scroll + reveal, the Recent Runs expansion, and the existing delete-all test + (dialog copy assertion, `delete-all-runs` / `delete-all-runs-confirm` + test ids). +- The seeded `succeeded` run needs no task; stored statuses render as-is. + +## Output contract + +Report the exact Playwright command and pass/fail counts, files changed, any +blockers or risks, and synchronized task/plan status in the same +conversation. No backend changes. + +## Results + +- E2E: `pnpm --filter @kandev/web e2e:raw -g "delete" --workers=4 + automations-settings.spec.ts` — "delete all only removes the runs in the + active status view" and "delete individual and all runs from Recent Runs" + passed; an earlier full chromium-project run (67 tests) passed with exit 0. +- `git diff --check` and Prettier — passed. No backend changes. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index dac6ba252f..215296d209 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -262,6 +262,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [missing-task-route-recovery](missing-task-route-recovery/spec.md) | draft | | [browser-inspect-annotations-save](browser-inspect-annotations-save/spec.md) | shipped | | [automations-pr-merged-trigger](automations-pr-merged-trigger/spec.md) | draft | +| [automation-runs-delete-all-by-status](automation-runs-delete-all-by-status/spec.md) | draft | | [no-silent-model-fallback](no-silent-model-fallback/spec.md) | approved | | [e2e-duration-aware-sharding](e2e-duration-aware-sharding/spec.md) | implemented | | [board-step-visibility-filter](board-step-visibility-filter/spec.md) | draft | diff --git a/docs/specs/automation-runs-delete-all-by-status/spec.md b/docs/specs/automation-runs-delete-all-by-status/spec.md new file mode 100644 index 0000000000..8fc538e996 --- /dev/null +++ b/docs/specs/automation-runs-delete-all-by-status/spec.md @@ -0,0 +1,70 @@ +--- +status: draft +created: 2026-08-12 +owner: clem +--- + +# Automation runs — status-scoped delete all + +## Why + +The Recent Runs table on an automation's settings page +(`/settings/workspaces//automations/`) can filter by status (Skipped, +Archived, Cancelled, ...), but the "delete all runs" action always clears +every run for the automation regardless of the active filter. A user who +filters to one status to tidy just those runs cannot delete them as a group: +they must click the per-row delete button once per run. The delete-all +control also sits in the section header above the table, away from the +per-row delete buttons, so its scope is easy to misread. + +## What + +- The delete-all control moves into the **rightmost cell of the Recent Runs + table header** — the same column as the per-row delete buttons. +- Delete-all acts only on the **currently active status view**: + - Filter "All" → every run of the automation is deleted (the existing + single `automation.runs.delete_all` call). + - Any status filter → exactly the runs shown in that view are deleted; + runs of every other status are untouched. +- The confirmation dialog names the scope: the existing copy when the filter + is "All", and a status-named variant (e.g. "Delete all Skipped runs?") when + a status filter is active. +- Deleting runs also deletes their associated tasks — identical to the + per-run delete and the existing delete-all today. +- The delete-all control is hidden when the active view shows no runs. +- A failed delete-all keeps the existing failure behavior: an error toast, + then the store reverts to the server's run list (or to the pre-delete + snapshot when the recovery refresh also fails). + +## Scenarios + +- **GIVEN** the Recent Runs table expanded with runs of mixed statuses and + the "All" filter active, **WHEN** the user clicks the delete-all button in + the table header and confirms, **THEN** every run is removed and the table + shows the no-runs empty state. +- **GIVEN** the "Skipped" filter active with 2 skipped and 1 succeeded run, + **WHEN** the user clicks delete-all in the table header and confirms, + **THEN** the 2 skipped runs are removed, the succeeded run remains, and + switching the filter back to "All" still shows the succeeded run. +- **GIVEN** the "Archived" filter active, **WHEN** the user deletes all, + **THEN** only runs whose displayed status is Archived are removed. +- **GIVEN** a filter whose view is empty ("No runs match this filter"), + **WHEN** the user looks at the table header, **THEN** no delete-all control + is shown. +- **GIVEN** the table is expanded, **WHEN** the user looks at the table + header, **THEN** the delete-all control sits in the rightmost header cell, + horizontally aligned with the per-row delete buttons. +- **GIVEN** the delete-all request fails, **WHEN** the user confirms the + dialog, **THEN** an error toast appears and the table reverts to the + server's run list. + +## Out of scope + +- No backend or API changes. The "All" view keeps the single + `automation.runs.delete_all` call; a filtered view deletes the visible runs + through the existing per-run `automation.run.delete` path. Filtered + delete-all therefore covers the runs currently loaded in the table (the + list is capped at 50 rows), not runs of the same status beyond the load + window. +- No change to the per-run delete button, the status filter chips, or the + workspace-wide runs feed (detail rail/drawer).