From 09b9c4566561bb6a698c263de8aae8c74a4e1434 Mon Sep 17 00:00:00 2001 From: Developer Date: Fri, 26 Jun 2026 15:31:30 +0000 Subject: [PATCH] SheetForm dirty-state confirm + wire isDirty into all form consumers (R4.5) SheetForm gains an isDirty prop. When true, any close attempt (Cancel button, header X, Radix overlay click, Escape) opens a 'Discard changes?' ConfirmDialog instead of discarding unsaved edits. Radix dismiss callbacks (onEscapeKeyDown, onPointerDownOutside) are intercepted when dirty so the guard applies uniformly. All four form consumers now compute and pass isDirty: - ServicePage: name/enabled/config differ from the persisted instance. - Settings machine editor: field-by-field draft vs editingMachine (create mode is always dirty; secret write-only fields excluded). - Message compose: subject non-empty, body differs from default, or attachments present. - WidgetConfigDialog: draft !== null (only draft mode is guarded; list mode has nothing to discard). Tests: 3 new SheetForm dirty-guard cases (prompt on cancel, abort discard, clean close when not dirty) + one focused dirty-guard test per consumer. 122 tests pass; lint/build green. Refs openspec/changes/mobile-responsive-parity/verify-report.md residual risk #1. --- .../src/components/WidgetConfigDialog.tsx | 7 +- .../__tests__/WidgetConfigDialog.test.tsx | 14 ++++ .../ui/__tests__/sheet-form.test.tsx | 72 +++++++++++++++++++ frontend/src/components/ui/sheet-form.tsx | 63 +++++++++++++++- frontend/src/pages/ServicePage.tsx | 13 +++- frontend/src/pages/Settings.tsx | 25 +++++++ frontend/src/pages/UsersPage.impl.tsx | 13 ++-- .../src/pages/__tests__/Settings.test.tsx | 23 ++++++ .../src/pages/__tests__/UsersPage.test.tsx | 31 ++++++++ 9 files changed, 248 insertions(+), 13 deletions(-) diff --git a/frontend/src/components/WidgetConfigDialog.tsx b/frontend/src/components/WidgetConfigDialog.tsx index 8634442..6b6eaca 100644 --- a/frontend/src/components/WidgetConfigDialog.tsx +++ b/frontend/src/components/WidgetConfigDialog.tsx @@ -315,7 +315,7 @@ export function WidgetConfigDialog({ open, onClose }: Props) {
setDraft({ ...draft, enabled: checked }) @@ -394,8 +394,8 @@ export function WidgetConfigDialog({ open, onClose }: Props) { toggleEnabled(instance)} aria-label={`Toggle ${instance.title}`} /> @@ -479,6 +479,7 @@ export function WidgetConfigDialog({ open, onClose }: Props) { onCancel={draft ? reset : () => handleClose(false)} saveLabel={draft ? "Save widget" : "Done"} isPending={draft ? saveWidget.isPending : false} + isDirty={draft !== null} >
{draftBody}
diff --git a/frontend/src/components/__tests__/WidgetConfigDialog.test.tsx b/frontend/src/components/__tests__/WidgetConfigDialog.test.tsx index cb87696..baab0ed 100644 --- a/frontend/src/components/__tests__/WidgetConfigDialog.test.tsx +++ b/frontend/src/components/__tests__/WidgetConfigDialog.test.tsx @@ -50,4 +50,18 @@ describe("WidgetConfigDialog (mobile SheetForm — slice 8)", () => { // List mode footer: "Done" button closes. expect(screen.getByRole("button", { name: "Done" })).toBeInTheDocument(); }); + + it("prompts before discarding a widget draft (R4.5)", async () => { + const { userEvent } = await import("@testing-library/user-event"); + render( {}} />); + + // Enter draft mode by clicking an "Add widget" button. + await userEvent.click(screen.getByRole("button", { name: /Backups/i })); + + // Now in draft mode — Cancel should prompt before resetting. + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect( + screen.getByRole("heading", { name: "Discard changes?" }), + ).toBeInTheDocument(); + }); }); diff --git a/frontend/src/components/ui/__tests__/sheet-form.test.tsx b/frontend/src/components/ui/__tests__/sheet-form.test.tsx index e9d4871..11e9ed1 100644 --- a/frontend/src/components/ui/__tests__/sheet-form.test.tsx +++ b/frontend/src/components/ui/__tests__/sheet-form.test.tsx @@ -93,4 +93,76 @@ describe("SheetForm", () => { await userEvent.click(screen.getByRole("button", { name: "Close" })); expect(onCancel).toHaveBeenCalledTimes(1); }); + + describe("dirty-state confirm (R4.5)", () => { + it("prompts before discarding via Cancel when isDirty", async () => { + const onCancel = vi.fn(); + render( + {}} + title="Edit" + onSave={() => {}} + onCancel={onCancel} + isDirty + > +
+ , + ); + + // Cancel does not immediately close; a confirm opens. + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect(onCancel).not.toHaveBeenCalled(); + expect( + screen.getByRole("heading", { name: "Discard changes?" }), + ).toBeInTheDocument(); + + // Confirm discard -> actually closes. + await userEvent.click(screen.getByRole("button", { name: "Discard" })); + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + it("closing the confirm without discarding keeps the form open", async () => { + const onCancel = vi.fn(); + render( + {}} + title="Edit" + onSave={() => {}} + onCancel={onCancel} + isDirty + > +
+ , + ); + + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + // Two Cancel buttons now exist: the SheetForm footer and the confirm dialog. + const cancelButtons = screen.getAllByRole("button", { name: "Cancel" }); + await userEvent.click(cancelButtons[cancelButtons.length - 1]); + expect(onCancel).not.toHaveBeenCalled(); + }); + + it("closes immediately when not dirty", async () => { + const onCancel = vi.fn(); + render( + {}} + title="Edit" + onSave={() => {}} + onCancel={onCancel} + > +
+ , + ); + + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect(onCancel).toHaveBeenCalledTimes(1); + expect( + screen.queryByRole("heading", { name: "Discard changes?" }), + ).not.toBeInTheDocument(); + }); + }); }); diff --git a/frontend/src/components/ui/sheet-form.tsx b/frontend/src/components/ui/sheet-form.tsx index c19e1db..452158a 100644 --- a/frontend/src/components/ui/sheet-form.tsx +++ b/frontend/src/components/ui/sheet-form.tsx @@ -4,6 +4,7 @@ import { Loader2, XIcon } from "lucide-react"; import { cn } from "@/lib/utils"; import { Button } from "@/components/ui/button"; import { Sheet, SheetContent, SheetTitle } from "@/components/ui/sheet"; +import { ConfirmDialog } from "@/components/ConfirmDialog"; export interface SheetFormProps { open: boolean; @@ -17,6 +18,12 @@ export interface SheetFormProps { saveLabel?: string; /** Disable the Save button (e.g. when required fields are empty). */ saveDisabled?: boolean; + /** + * When true, any close attempt (Cancel button, header X, overlay click, + * Escape) prompts a discard-confirmation instead of immediately closing. + * Spec R4.5. + */ + isDirty?: boolean; children: React.ReactNode; /** Optional className applied to the scrolling body. */ bodyClassName?: string; @@ -43,15 +50,53 @@ export function SheetForm({ isPending = false, saveDisabled = false, saveLabel = "Save", + isDirty = false, children, bodyClassName, }: SheetFormProps) { + const [confirmDiscardOpen, setConfirmDiscardOpen] = React.useState(false); + + // Route every close path (Cancel, header X, Radix overlay/Escape) through one + // guard so the dirty-confirm is applied uniformly (spec R4.5). + const attemptClose = React.useCallback(() => { + if (isDirty) { + setConfirmDiscardOpen(true); + } else { + onCancel(); + } + }, [isDirty, onCancel]); + + const handleOpenChange = React.useCallback( + (next: boolean) => { + if (!next) { + attemptClose(); + } else { + onOpenChange(next); + } + }, + [attemptClose, onOpenChange], + ); + return ( - + { + // Prevent Radix's default Escape close so our guard runs instead. + if (isDirty) { + e.preventDefault(); + attemptClose(); + } + }} + onPointerDownOutside={(e) => { + // Prevent overlay-click close so our guard runs instead. + if (isDirty) { + e.preventDefault(); + attemptClose(); + } + }} > {/* Header — fixed at top */}
@@ -62,7 +107,7 @@ export function SheetForm({ variant="ghost" size="icon-sm" aria-label="Close" - onClick={onCancel} + onClick={attemptClose} > @@ -75,7 +120,7 @@ export function SheetForm({ {/* Footer — fixed at bottom */}
-
+ + setConfirmDiscardOpen(false)} + onConfirm={() => { + setConfirmDiscardOpen(false); + onCancel(); + }} + /> ); } diff --git a/frontend/src/pages/ServicePage.tsx b/frontend/src/pages/ServicePage.tsx index 7f2603a..448d6ae 100644 --- a/frontend/src/pages/ServicePage.tsx +++ b/frontend/src/pages/ServicePage.tsx @@ -174,6 +174,12 @@ export function ServicePage() { /> ); + // Dirty when any editable field diverges from the persisted instance (mobile SheetForm R4.5 guard). + const isDirty = + name !== instance.name || + enabled !== instance.enabled || + JSON.stringify(draftConfig) !== JSON.stringify(instance.config); + if (isMobile) { return (
@@ -183,10 +189,11 @@ export function ServicePage() { title={name || instance.name} onSave={save} onCancel={() => { - setSheetOpen(false); - navigate("/services"); - }} + setSheetOpen(false); + navigate("/services"); + }} isPending={saveService.isPending} + isDirty={isDirty} >
diff --git a/frontend/src/pages/Settings.tsx b/frontend/src/pages/Settings.tsx index 71811fe..4a99293 100644 --- a/frontend/src/pages/Settings.tsx +++ b/frontend/src/pages/Settings.tsx @@ -128,6 +128,30 @@ function emptyMachine( }; } +/** + * Dirty check for the machine editor SheetForm guard (spec R4.5). + * Pragmatic field-by-field comparison of the user-editable fields. In create + * mode (editingMachine is null) the form is always dirty. + */ +function isMachineDraftDirty( + draft: MonitoringMachineInput, + editingMachine: MonitoringMachine | null, +): boolean { + if (!editingMachine) return true; + return ( + draft.name !== editingMachine.name || + draft.host !== editingMachine.host || + draft.mode !== editingMachine.mode || + draft.port !== editingMachine.port || + draft.username !== editingMachine.username || + draft.ssh_key_id !== editingMachine.ssh_key_id || + draft.enabled !== editingMachine.enabled || + draft.notes !== editingMachine.notes || + JSON.stringify([...draft.services].sort()) !== + JSON.stringify([...editingMachine.services].sort()) + ); +} + function MachineEditor({ title, hint, @@ -1159,6 +1183,7 @@ export function Settings() { (machineDraft.mode === "ssh" && !machineDraft.host.trim()) } saveLabel={machineDraft.id ? "Save machine" : "Create machine"} + isDirty={isMachineDraftDirty(machineDraft, editingMachine)} > - - - - 0 + } >
{composeBody}
diff --git a/frontend/src/pages/__tests__/Settings.test.tsx b/frontend/src/pages/__tests__/Settings.test.tsx index 7530a7a..174a3ab 100644 --- a/frontend/src/pages/__tests__/Settings.test.tsx +++ b/frontend/src/pages/__tests__/Settings.test.tsx @@ -187,4 +187,27 @@ describe("Settings (mobile SheetForm — slice 7)", () => { // (The page content itself is still rendered; only the sheet unmounts.) expect(screen.queryByText("Edit machine")).not.toBeInTheDocument(); }); + + it("prompts before discarding unsaved machine edits (R4.5)", async () => { + machines = [localMachine()]; + render(); + + const detailEdit = screen + .getAllByRole("button", { name: "Edit" }) + .find((button) => button.textContent === "Edit") as HTMLButtonElement; + await userEvent.click(detailEdit); + + // Edit the name to make the form dirty. + const nameInput = screen.getByLabelText("Name"); + await userEvent.clear(nameInput); + await userEvent.type(nameInput, "Dirty name"); + + // Cancel should NOT immediately close — the discard confirm appears. + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect( + screen.getByRole("heading", { name: "Discard changes?" }), + ).toBeInTheDocument(); + // The editor is still open. + expect(screen.getByText("Edit machine")).toBeInTheDocument(); + }); }); diff --git a/frontend/src/pages/__tests__/UsersPage.test.tsx b/frontend/src/pages/__tests__/UsersPage.test.tsx index d465976..7bc8677 100644 --- a/frontend/src/pages/__tests__/UsersPage.test.tsx +++ b/frontend/src/pages/__tests__/UsersPage.test.tsx @@ -373,4 +373,35 @@ describe("UsersPage (mobile card layout — slice 5)", () => { ).toBeInTheDocument(); expect(screen.getByLabelText("Subject")).toBeInTheDocument(); }); + + it("prompts before discarding unsaved compose edits (R4.5)", async () => { + users = [ + userFixture({ + jellyfin_id: "u1", + display_name: "Alice", + email: "alice@example.com", + }), + ]; + render( + + + , + ); + + await userEvent.click( + screen.getByRole("checkbox", { name: /Select Alice/i }), + ); + await userEvent.click( + screen.getByRole("button", { name: "Message selected" }), + ); + + // Type a subject to make the compose form dirty. + await userEvent.type(screen.getByLabelText("Subject"), "Urgent update"); + + // Cancel should NOT immediately close — the discard confirm appears. + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect( + screen.getByRole("heading", { name: "Discard changes?" }), + ).toBeInTheDocument(); + }); });