From 705b93507f582f9dbf79bc10b3af115c713c232a Mon Sep 17 00:00:00 2001 From: Blake Gentry Date: Fri, 10 Apr 2026 13:50:27 -0500 Subject: [PATCH 1/2] fix vitest hook mocks --- src/components/JobList.test.tsx | 51 +++++++++++++++++++--------- src/components/SettingsPage.test.tsx | 40 ++++++++++++++++------ src/hooks/use-settings.test.tsx | 35 +++++++++++++++---- 3 files changed, 92 insertions(+), 34 deletions(-) diff --git a/src/components/JobList.test.tsx b/src/components/JobList.test.tsx index fda42b0..6ba19f4 100644 --- a/src/components/JobList.test.tsx +++ b/src/components/JobList.test.tsx @@ -1,13 +1,36 @@ 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 { type ReactNode } from "react"; +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 +38,7 @@ vi.mock("@tanstack/react-router", () => { className, to, }: { - children: React.ReactNode; + children: ReactNode; className: string; to: string; }) => ( @@ -28,10 +51,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 +66,7 @@ describe("JobList", () => { }); // Mock settings with no override - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: {}, - }); + mockUseSettings.mockReturnValue(settingsMock({})); render( @@ -71,9 +96,7 @@ describe("JobList", () => { }); // Mock settings with no override - (useSettings as unknown as ReturnType).mockReturnValue({ - settings: {}, - }); + mockUseSettings.mockReturnValue(settingsMock({})); render( @@ -105,9 +128,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 +159,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( diff --git a/src/components/SettingsPage.test.tsx b/src/components/SettingsPage.test.tsx index 0b078bc..9dd8a57 100644 --- a/src/components/SettingsPage.test.tsx +++ b/src/components/SettingsPage.test.tsx @@ -1,27 +1,45 @@ -import { useFeatures } from "@contexts/Features.hook"; -import { useSettings } from "@hooks/use-settings"; import { createFeatures } from "@test/utils/features"; import { fireEvent, render, screen } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { + beforeEach, + describe, + expect, + it, + type MockedFunction, + vi, +} from "vitest"; import SettingsPage from "./SettingsPage"; +type UseFeatures = typeof import("@contexts/Features.hook").useFeatures; +type UseSettings = typeof import("@hooks/use-settings").useSettings; + +const { mockUseFeatures, mockUseSettings } = vi.hoisted(() => ({ + 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, }), From d7b0de56fe61d3b7810512459ac0294c170e545e Mon Sep 17 00:00:00 2001 From: Blake Gentry Date: Fri, 10 Apr 2026 13:08:17 -0500 Subject: [PATCH 2/2] add delete confirmations for jobs Deleting jobs is irreversible, but the UI previously dispatched both the single-job and bulk delete actions immediately. That made it too easy to remove the wrong jobs with one stray click, especially from the bulk selection toolbar. Add a shared confirmation dialog built on the existing Headless UI `Dialog` primitives and route only delete actions through it. The job detail page now confirms before deleting one job, and the job list now confirms before deleting the current selection while leaving retry and cancel unchanged. Raise modal z-index so the backdrop and panel consistently layer over the left navigation rail. This uses the app's existing dialog stack instead of adding a new UI library, keeps styling consistent, and adds focused test coverage for the single-job and bulk delete flows. Fixes #545. --- CHANGELOG.md | 1 + src/components/ConfirmationDialog.tsx | 90 ++++++++++++++++++++++++++ src/components/JobDetail.test.tsx | 79 +++++++++++++++++++--- src/components/JobDetail.tsx | 64 ++++++++++++------ src/components/JobList.test.tsx | 65 ++++++++++++++++++- src/components/JobList.tsx | 84 +++++++++++++++++------- src/components/RetryWorkflowDialog.tsx | 2 +- 7 files changed, 328 insertions(+), 57 deletions(-) create mode 100644 src/components/ConfirmationDialog.tsx 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 6ba19f4..d8e202c 100644 --- a/src/components/JobList.test.tsx +++ b/src/components/JobList.test.tsx @@ -2,8 +2,9 @@ import { FeaturesContext } from "@contexts/Features"; 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 { act, render, screen, waitFor, within } from "@testing-library/react"; import { type ReactNode } from "react"; +import { userEvent } from "storybook/test"; import { beforeEach, describe, @@ -184,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 ( - +