diff --git a/CHANGELOG.md b/CHANGELOG.md index c7b1958..12d7652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Workflow detail: truncate long workflow names in the header to prevent overflow and add a copy button for the full name. [PR #524](https://github.com/riverqueue/riverui/pull/524). - JSON viewer: sort keys alphabetically in rendered and copied output for object payloads. [PR #525](https://github.com/riverqueue/riverui/pull/525). - Job state sidebar: only highlight `Running` when the selected jobs state is actually running, even with retained search filters in the URL. [Fixes #526](https://github.com/riverqueue/riverui/issues/526). [PR #527](https://github.com/riverqueue/riverui/pull/527). +- Job delete actions: require confirmation before deleting a single job or selected jobs in bulk. [Fixes #545](https://github.com/riverqueue/riverui/issues/545). [PR #546](https://github.com/riverqueue/riverui/pull/546). ## [v0.15.0] - 2026-02-26 diff --git a/src/components/ConfirmationDialog.tsx b/src/components/ConfirmationDialog.tsx new file mode 100644 index 0000000..077cd25 --- /dev/null +++ b/src/components/ConfirmationDialog.tsx @@ -0,0 +1,90 @@ +import { + Description, + Dialog, + DialogBackdrop, + DialogPanel, + DialogTitle, +} from "@headlessui/react"; +import { type ReactNode } from "react"; + +export type ConfirmationDialogProps = { + cancelText?: string; + confirmText: string; + description: ReactNode; + onClose: () => void; + onConfirm: () => void; + open: boolean; + pending?: boolean; + title: string; +}; + +export default function ConfirmationDialog({ + cancelText = "Cancel", + confirmText, + description, + onClose, + onConfirm, + open, + pending = false, + title, +}: ConfirmationDialogProps) { + if (!open) return null; + + const handleClose = () => { + if (!pending) { + onClose(); + } + }; + + return ( + + + +
+
+ +
+ + {title} + + + {description} + +
+
+ + +
+
+
+
+
+ ); +} diff --git a/src/components/JobDetail.test.tsx b/src/components/JobDetail.test.tsx index 1bffa5e..9e1aa95 100644 --- a/src/components/JobDetail.test.tsx +++ b/src/components/JobDetail.test.tsx @@ -1,16 +1,75 @@ import { jobFactory } from "@test/factories/job"; -import { render } from "@testing-library/react"; -import { expect, test } from "vitest"; +import { act, render, screen, waitFor, within } from "@testing-library/react"; +import { userEvent } from "storybook/test"; +import { expect, test, vi } from "vitest"; import JobDetail from "./JobDetail"; -test("adds 1 + 2 to equal 3", () => { - const job = jobFactory.build(); - const cancel = () => {}; - const deleteFn = () => {}; - const retry = () => {}; - const { getByTestId: _getTestById } = render( - , +test("requires confirmation before deleting a job", async () => { + const job = jobFactory.completed().build({ id: 123n }); + const deleteFn = vi.fn(); + const user = userEvent.setup(); + + render( + , ); - expect(3).toBe(3); + + await act(async () => { + await user.click(screen.getByRole("button", { name: /^delete$/i })); + }); + + expect(deleteFn).not.toHaveBeenCalled(); + const dialog = await screen.findByRole("dialog", { name: "Delete job?" }); + expect( + within(dialog).getByText(/This permanently deletes job/i), + ).toBeInTheDocument(); + expect(within(dialog).getByText("123")).toBeInTheDocument(); + + await act(async () => { + await user.click( + within(dialog).getByRole("button", { name: /delete job/i }), + ); + }); + + await waitFor(() => { + expect(deleteFn).toHaveBeenCalledTimes(1); + expect( + screen.queryByRole("dialog", { name: "Delete job?" }), + ).not.toBeInTheDocument(); + }); +}); + +test("cancels job delete confirmation", async () => { + const job = jobFactory.completed().build(); + const deleteFn = vi.fn(); + const user = userEvent.setup(); + + render( + , + ); + + await act(async () => { + await user.click(screen.getByRole("button", { name: /^delete$/i })); + }); + const dialog = await screen.findByRole("dialog", { name: "Delete job?" }); + await act(async () => { + await user.click(within(dialog).getByRole("button", { name: /cancel/i })); + }); + + await waitFor(() => { + expect(deleteFn).not.toHaveBeenCalled(); + expect( + screen.queryByRole("dialog", { name: "Delete job?" }), + ).not.toBeInTheDocument(); + }); }); diff --git a/src/components/JobDetail.tsx b/src/components/JobDetail.tsx index 7109518..693abc4 100644 --- a/src/components/JobDetail.tsx +++ b/src/components/JobDetail.tsx @@ -1,5 +1,6 @@ import { Badge } from "@components/Badge"; import ButtonForGroup from "@components/ButtonForGroup"; +import ConfirmationDialog from "@components/ConfirmationDialog"; import JobAttempts from "@components/JobAttempts"; import JobTimeline from "@components/JobTimeline"; import JSONView from "@components/JSONView"; @@ -15,7 +16,7 @@ import { Job, JobWithKnownMetadata } from "@services/jobs"; import { JobState } from "@services/types"; import { Link } from "@tanstack/react-router"; import { capitalize } from "@utils/string"; -import { FormEvent } from "react"; +import { FormEvent, useState } from "react"; type JobDetailProps = { cancel: () => void; @@ -185,12 +186,19 @@ export default function JobDetail({ } function ActionButtons({ cancel, deleteFn, job, retry }: JobDetailProps) { + const [deleteConfirmationOpen, setDeleteConfirmationOpen] = useState(false); + // Can only delete jobs that aren't running: const deleteDisabled = job.state === JobState.Running; const deleteJob = (event: FormEvent) => { event.preventDefault(); + setDeleteConfirmationOpen(true); + }; + + const confirmDelete = () => { deleteFn(); + setDeleteConfirmationOpen(false); }; // Can only cancel jobs that aren't already finalized (completed, discarded, cancelled): @@ -215,26 +223,42 @@ function ActionButtons({ cancel, deleteFn, job, retry }: JobDetailProps) { }; return ( - - - - + + + + + + + This permanently deletes job{" "} + {job.id.toString()}. This action + cannot be undone. + + } + onClose={() => setDeleteConfirmationOpen(false)} + onConfirm={confirmDelete} + open={deleteConfirmationOpen} + title="Delete job?" /> - + ); } diff --git a/src/components/JobList.test.tsx b/src/components/JobList.test.tsx index fda42b0..d8e202c 100644 --- a/src/components/JobList.test.tsx +++ b/src/components/JobList.test.tsx @@ -1,13 +1,37 @@ import { FeaturesContext } from "@contexts/Features"; -import { useSettings } from "@hooks/use-settings"; import { JobState } from "@services/types"; import { jobMinimalFactory } from "@test/factories/job"; import { createFeatures } from "@test/utils/features"; -import { render, screen } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { act, render, screen, waitFor, within } from "@testing-library/react"; +import { type ReactNode } from "react"; +import { userEvent } from "storybook/test"; +import { + beforeEach, + describe, + expect, + it, + type MockedFunction, + vi, +} from "vitest"; import JobList from "./JobList"; +type UseSettings = typeof import("@hooks/use-settings").useSettings; +type UseSettingsReturn = ReturnType; + +const { mockUseSettings } = vi.hoisted(() => ({ + mockUseSettings: vi.fn() as MockedFunction, +})); + +const settingsMock = ( + settings: UseSettingsReturn["settings"], +): UseSettingsReturn => ({ + clearShowJobArgs: vi.fn(), + setShowJobArgs: vi.fn(), + settings, + shouldShowJobArgs: true, +}); + vi.mock("@tanstack/react-router", () => { return { Link: ({ @@ -15,7 +39,7 @@ vi.mock("@tanstack/react-router", () => { className, to, }: { - children: React.ReactNode; + children: ReactNode; className: string; to: string; }) => ( @@ -28,10 +52,14 @@ vi.mock("@tanstack/react-router", () => { // Mock the useSettings hook vi.mock("@hooks/use-settings", () => ({ - useSettings: vi.fn(), + useSettings: mockUseSettings, })); describe("JobList", () => { + beforeEach(() => { + mockUseSettings.mockReset(); + }); + it("shows job args by default", () => { const job = jobMinimalFactory.build(); const features = createFeatures({ @@ -39,9 +67,7 @@ describe("JobList", () => { }); // Mock settings with no override - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: {}, - }); + mockUseSettings.mockReturnValue(settingsMock({})); render( @@ -71,9 +97,7 @@ describe("JobList", () => { }); // Mock settings with no override - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: {}, - }); + mockUseSettings.mockReturnValue(settingsMock({})); render( @@ -105,9 +129,7 @@ describe("JobList", () => { }); // Mock settings with override to show args - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: { showJobArgs: true }, - }); + mockUseSettings.mockReturnValue(settingsMock({ showJobArgs: true })); render( @@ -138,9 +160,7 @@ describe("JobList", () => { }); // Mock settings with override to hide args - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: { showJobArgs: false }, - }); + mockUseSettings.mockReturnValue(settingsMock({ showJobArgs: false })); render( @@ -165,4 +185,66 @@ describe("JobList", () => { screen.queryByText(JSON.stringify(job.args)), ).not.toBeInTheDocument(); }); + + it("requires confirmation before deleting selected jobs", async () => { + const jobs = [ + jobMinimalFactory.completed().build({ id: 1n }), + jobMinimalFactory.completed().build({ id: 2n }), + ]; + const deleteJobs = vi.fn(); + const user = userEvent.setup(); + const features = createFeatures({ + jobListHideArgsByDefault: false, + }); + + mockUseSettings.mockReturnValue(settingsMock({})); + + render( + + + , + ); + + await act(async () => { + await user.click( + screen.getByRole("checkbox", { name: /select all jobs/i }), + ); + }); + await act(async () => { + await user.click(screen.getByRole("button", { name: /^delete$/i })); + }); + + expect(deleteJobs).not.toHaveBeenCalled(); + const dialog = await screen.findByRole("dialog", { + name: "Delete selected jobs?", + }); + expect( + within(dialog).getByText(/This permanently deletes 2 selected jobs/i), + ).toBeInTheDocument(); + + await act(async () => { + await user.click( + within(dialog).getByRole("button", { name: /delete jobs/i }), + ); + }); + + await waitFor(() => { + expect(deleteJobs).toHaveBeenCalledWith(jobs.map((job) => job.id)); + expect( + screen.queryByRole("dialog", { name: "Delete selected jobs?" }), + ).not.toBeInTheDocument(); + }); + }); }); diff --git a/src/components/JobList.tsx b/src/components/JobList.tsx index f212e5f..820004f 100644 --- a/src/components/JobList.tsx +++ b/src/components/JobList.tsx @@ -1,6 +1,7 @@ import { Badge } from "@components/Badge"; import { Button } from "@components/Button"; import ButtonForGroup from "@components/ButtonForGroup"; +import ConfirmationDialog from "@components/ConfirmationDialog"; import { CustomCheckbox } from "@components/CustomCheckbox"; import { Dropdown, DropdownItem, DropdownMenu } from "@components/Dropdown"; import { Filter, JobSearch } from "@components/job-search/JobSearch"; @@ -29,7 +30,13 @@ import { jobStateFilterItems, } from "@utils/jobStateFilterItems"; import { classNames } from "@utils/style"; -import React, { FormEvent, useCallback, useEffect, useMemo } from "react"; +import React, { + FormEvent, + useCallback, + useEffect, + useMemo, + useState, +} from "react"; const states: { [key in JobState]: string } = { [JobState.Available]: "text-sky-500 bg-sky-100/10", @@ -216,12 +223,19 @@ function JobListActionButtons({ retry: (jobIDs: bigint[]) => void; state: JobState; }) { + const [deleteConfirmationOpen, setDeleteConfirmationOpen] = useState(false); + // Can only delete jobs that aren't running: const deleteDisabled = state === JobState.Running; const deleteJob = (event: FormEvent) => { event.preventDefault(); + setDeleteConfirmationOpen(true); + }; + + const confirmDelete = () => { deleteFn(jobIDs); + setDeleteConfirmationOpen(false); }; // Can only cancel jobs that aren't already finalized (completed, discarded, cancelled): @@ -243,32 +257,52 @@ function JobListActionButtons({ retry(jobIDs); }; + const selectedJobCount = jobIDs.length; + return ( - - - - + + + + + + setDeleteConfirmationOpen(false)} + onConfirm={confirmDelete} + open={deleteConfirmationOpen} + title={ + selectedJobCount === 1 + ? "Delete selected job?" + : "Delete selected jobs?" + } /> - + ); } diff --git a/src/components/RetryWorkflowDialog.tsx b/src/components/RetryWorkflowDialog.tsx index 2da3772..2fad381 100644 --- a/src/components/RetryWorkflowDialog.tsx +++ b/src/components/RetryWorkflowDialog.tsx @@ -58,7 +58,7 @@ function RetryWorkflowDialogInner({ }; return ( - + ({ + mockUseFeatures: vi.fn() as MockedFunction, + mockUseSettings: vi.fn() as MockedFunction, +})); + // Mock useSettings hook vi.mock("@hooks/use-settings", () => ({ - useSettings: vi.fn(), + useSettings: mockUseSettings, })); // Mock useFeatures hook vi.mock("@contexts/Features.hook", () => ({ - useFeatures: vi.fn(), + useFeatures: mockUseFeatures, })); describe("SettingsPage", () => { + beforeEach(() => { + mockUseFeatures.mockReset(); + mockUseSettings.mockReset(); + }); + it("renders correctly with default settings", () => { // Mock settings hook const mockSetShowJobArgs = vi.fn(); const mockClearShowJobArgs = vi.fn(); - (useSettings as unknown as ReturnType).mockReturnValue({ + mockUseSettings.mockReturnValue({ clearShowJobArgs: mockClearShowJobArgs, setShowJobArgs: mockSetShowJobArgs, settings: {}, @@ -29,7 +47,7 @@ describe("SettingsPage", () => { }); // Mock features - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: false, }), @@ -61,7 +79,7 @@ describe("SettingsPage", () => { // Mock settings hook with override const mockSetShowJobArgs = vi.fn(); const mockClearShowJobArgs = vi.fn(); - (useSettings as unknown as ReturnType).mockReturnValue({ + mockUseSettings.mockReturnValue({ clearShowJobArgs: mockClearShowJobArgs, setShowJobArgs: mockSetShowJobArgs, settings: { showJobArgs: true }, @@ -69,7 +87,7 @@ describe("SettingsPage", () => { }); // Mock features - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: true, }), @@ -96,7 +114,7 @@ describe("SettingsPage", () => { // Mock settings hook const mockSetShowJobArgs = vi.fn(); const mockClearShowJobArgs = vi.fn(); - (useSettings as unknown as ReturnType).mockReturnValue({ + mockUseSettings.mockReturnValue({ clearShowJobArgs: mockClearShowJobArgs, setShowJobArgs: mockSetShowJobArgs, settings: {}, @@ -104,7 +122,7 @@ describe("SettingsPage", () => { }); // Mock features - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: true, }), diff --git a/src/hooks/use-settings.test.tsx b/src/hooks/use-settings.test.tsx index 92c8648..86f5d93 100644 --- a/src/hooks/use-settings.test.tsx +++ b/src/hooks/use-settings.test.tsx @@ -1,25 +1,46 @@ -import { useFeatures } from "@contexts/Features.hook"; import { $userSettings } from "@stores/settings"; import { createFeatures } from "@test/utils/features"; import { renderHook } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { + beforeEach, + describe, + expect, + it, + type MockedFunction, + vi, +} from "vitest"; import { useSettings } from "./use-settings"; +type UseFeatures = typeof import("@contexts/Features.hook").useFeatures; +type UseStore = typeof import("@nanostores/react").useStore; + +const { mockUseFeatures, mockUseStore } = vi.hoisted(() => ({ + mockUseFeatures: vi.fn() as MockedFunction, + mockUseStore: vi.fn() as MockedFunction, +})); + // Mock nanostores/react vi.mock("@nanostores/react", () => ({ - useStore: vi.fn().mockImplementation((store) => store.get()), + useStore: mockUseStore, })); // Mock Features context vi.mock("@contexts/Features.hook", () => ({ - useFeatures: vi.fn(), + useFeatures: mockUseFeatures, })); describe("useSettings", () => { + beforeEach(() => { + vi.restoreAllMocks(); + mockUseFeatures.mockReset(); + mockUseStore.mockReset(); + mockUseStore.mockImplementation((store) => store.get()); + }); + it("should return default show job args value when no override", () => { // Mock Features hook - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: true, }), @@ -34,7 +55,7 @@ describe("useSettings", () => { it("should return override when set to true", () => { // Mock Features hook with hide args by default - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: true, }), @@ -49,7 +70,7 @@ describe("useSettings", () => { it("should return override when set to false", () => { // Mock Features hook with show args by default - (useFeatures as unknown as ReturnType).mockReturnValue({ + mockUseFeatures.mockReturnValue({ features: createFeatures({ jobListHideArgsByDefault: false, }),