From bf00b63215b4db062caafc1fb66e9e666d868d75 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 23 Sep 2026 21:22:25 -0400 Subject: [PATCH] Preserve inactive assignees on edit and manage edit-sheet focus NQ-198-001: a PATCH omits assignee_child_ids while the active selection is unchanged, so inactive assignees survive unrelated edits. Changing the selection when inactive children would be dropped asks for confirmation first. Inactive assignees are shown locked in the picker with a hint. NQ-198-002: the edit sheet moves focus in on open, wraps Tab/Shift+Tab, restores focus to the trigger on close, and cancels on Escape when not saving. Co-Authored-By: Claude Opus 5.5 --- admin/src/definitions/DefinitionsTab.css | 28 ++++ admin/src/definitions/DefinitionsTab.test.tsx | 138 +++++++++++++++- admin/src/definitions/DefinitionsTab.tsx | 154 ++++++++++++++++-- 3 files changed, 302 insertions(+), 18 deletions(-) diff --git a/admin/src/definitions/DefinitionsTab.css b/admin/src/definitions/DefinitionsTab.css index 0c115ba..1b9f425 100644 --- a/admin/src/definitions/DefinitionsTab.css +++ b/admin/src/definitions/DefinitionsTab.css @@ -353,6 +353,16 @@ color: var(--nq-brand-blue); } +.defs-picker-option--locked { + color: var(--nq-a-ink-secondary); + cursor: default; +} + +.defs-picker-note { + font-size: 12px; + font-weight: 600; +} + .defs-repeats { display: flex; gap: 8px; @@ -493,6 +503,24 @@ margin-top: 16px; } +.defs-confirm { + margin-top: 12px; + padding: 10px 12px; + border-radius: var(--nq-a-density-radius); + background: var(--nq-a-danger-bg); + color: var(--nq-a-danger-strong); + font-size: 12.5px; + font-weight: 700; +} + +.defs-confirm p { + margin: 0; +} + +.defs-confirm .defs-sheet-footer { + margin-top: 10px; +} + .defs-cancel, .defs-save { padding: 13px 0; diff --git a/admin/src/definitions/DefinitionsTab.test.tsx b/admin/src/definitions/DefinitionsTab.test.tsx index 96c0654..1ae0d64 100644 --- a/admin/src/definitions/DefinitionsTab.test.tsx +++ b/admin/src/definitions/DefinitionsTab.test.tsx @@ -152,6 +152,14 @@ function fakeApi(initial: QuestDefinition[]) { title: body.title ?? d.title, skip_on_away: body.skip_on_away ?? d.skip_on_away, rule: body.rule ?? d.rule, + // Like the API: a sent list replaces the whole set; an absent one keeps it. + assignees: + "assignee_child_ids" in body + ? CHILDREN.filter((c) => body.assignee_child_ids.includes(c.id)).map((c) => ({ + id: c.id, + display_name: c.display_name, + })) + : d.assignees, } : d, ); @@ -360,9 +368,13 @@ describe("DefinitionsTab", () => { expect((within(s).getByLabelText("Afternoon due time") as HTMLInputElement).value).toBe( "16:00", ); - // The inactive child "Old" is not offered and not submitted. + // The inactive child "Old" is shown locked, not offered as a checkbox. fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); expect(within(s).queryByRole("checkbox", { name: "Old" })).toBeNull(); + const locked = within(s).getByTestId("inactive-assignee-4"); + expect(locked.textContent).toContain("Old"); + expect(locked.textContent).toContain("Inactive"); + expect(locked.getAttribute("aria-disabled")).toBe("true"); fireEvent.click(within(s).getByRole("checkbox", { name: "Maeve" })); fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { @@ -382,8 +394,11 @@ describe("DefinitionsTab", () => { }); fireEvent.click(within(s).getByRole("switch", { name: "Skip on away days" })); + // Changing the selection would drop "Old": confirm before sending. + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + expect(api.writes()).toHaveLength(0); await act(async () => { - fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + fireEvent.click(within(s).getByRole("button", { name: "Unassign and save" })); }); expect(api.writes()).toEqual([ @@ -417,6 +432,125 @@ describe("DefinitionsTab", () => { expect(screen.queryByRole("dialog")).toBeNull(); }); + it("editing only the title omits assignee_child_ids and keeps an inactive assignee", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + expect(within(s).getByText(/Also assigned \(inactive\): Old\./)).toBeTruthy(); + fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { + target: { value: "Bins out" }, + }); + + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + + expect(screen.queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(1); + const body = api.writes()[0].body as Record; + expect(api.writes()[0].method).toBe("PATCH"); + expect(body.title).toBe("Bins out"); + expect(body).not.toHaveProperty("assignee_child_ids"); + + await screen.findByText("Bins out"); + expect(meta(screen.getByTestId("definition-11"))).toBe( + "Declan, Old · Every 2 weeks on Mon, Thu · Afternoon", + ); + }); + + it("changing the active selection with an inactive assignee asks first", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + const confirm = within(s).getByTestId("unassign-confirm"); + expect(confirm.textContent).toContain("Saving will unassign Old."); + expect(document.activeElement).toBe(confirm); + expect(api.writes()).toHaveLength(0); + + // Going back sends nothing and restores the normal footer. + fireEvent.click(within(confirm).getByRole("button", { name: "Go back" })); + expect(within(s).queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(0); + + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Unassign and save" })); + }); + expect(api.writes()).toHaveLength(1); + expect((api.writes()[0].body as { assignee_child_ids: number[] }).assignee_child_ids).toEqual([ + 1, 3, + ]); + }); + + it("reverting the selection to the original omits assignee_child_ids without asking", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + expect(screen.queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(1); + expect(api.writes()[0].body).not.toHaveProperty("assignee_child_ids"); + }); + + it("focus moves into the sheet on open and returns to the row on Escape", async () => { + await renderReady(); + const row = screen.getByTestId("definition-10"); + row.focus(); + fireEvent.click(row); + const s = sheet(); + expect(document.activeElement).toBe(within(s).getByRole("heading", { name: "Edit task" })); + + fireEvent.keyDown(s, { key: "Escape" }); + expect(screen.queryByRole("dialog")).toBeNull(); + expect(document.activeElement).toBe(row); + expect(api.writes()).toHaveLength(0); + }); + + it("Tab and Shift+Tab wrap within the sheet", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + const s = sheet(); + const title = within(s).getByRole("textbox", { name: "Title" }); + const save = within(s).getByRole("button", { name: "Save changes" }); + + save.focus(); + fireEvent.keyDown(s, { key: "Tab" }); + expect(document.activeElement).toBe(title); + + fireEvent.keyDown(s, { key: "Tab", shiftKey: true }); + expect(document.activeElement).toBe(save); + }); + + it("Escape does not close the sheet while saving", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + let release: (response: Response) => void = () => {}; + api.fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (release = resolve)), + ); + const s = sheet(); + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + fireEvent.keyDown(s, { key: "Escape" }); + expect(screen.getByRole("dialog")).toBeTruthy(); + + await act(async () => { + release(jsonResponse(BRUSH)); + }); + await vi.waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + }); + it("the skip-on-away toggle reflects and persists a false value", async () => { await renderReady(); fireEvent.click(screen.getByTestId("definition-11")); diff --git a/admin/src/definitions/DefinitionsTab.tsx b/admin/src/definitions/DefinitionsTab.tsx index b136446..2bb80e6 100644 --- a/admin/src/definitions/DefinitionsTab.tsx +++ b/admin/src/definitions/DefinitionsTab.tsx @@ -4,7 +4,13 @@ * The icon picker (§3.3) and the occurrence preview are separate tasks: an * existing definition's icon is carried through unchanged by never sending it. */ -import { useEffect, useRef, useState, type ReactNode } from "react"; +import { + useEffect, + useRef, + useState, + type KeyboardEvent as ReactKeyboardEvent, + type ReactNode, +} from "react"; import { ApiForbiddenError } from "../api/client"; import { ApiRequestError, @@ -14,6 +20,7 @@ import { updateDefinition, type AdminChild, type DefinitionCreateBody, + type DefinitionEditBody, type DefinitionRule, type QuestDefinition, type WindowEntry, @@ -202,10 +209,14 @@ function buildRule(draft: Draft): DefinitionRule | string { } } -function buildBody(draft: Draft): DefinitionCreateBody | string { +/** + * `keepsAssignees`: an edit whose active selection is unchanged sends no + * assignee list, so an empty selection is fine when inactive children remain. + */ +function buildBody(draft: Draft, keepsAssignees = false): DefinitionCreateBody | string { const title = draft.title.trim(); if (!title) return "Give the task a title."; - if (draft.assigneeIds.length === 0) return "Assign at least one child."; + if (draft.assigneeIds.length === 0 && !keepsAssignees) return "Assign at least one child."; if (draft.windows.length === 0) return "Pick at least one window."; const rule = buildRule(draft); if (typeof rule === "string") return rule; @@ -231,6 +242,13 @@ function toggle(list: T[], value: T): T[] { return list.includes(value) ? list.filter((item) => item !== value) : [...list, value]; } +function sameIds(a: number[], b: number[]): boolean { + return a.length === b.length && a.every((id) => b.includes(id)); +} + +const FOCUSABLE = + 'button:not([disabled]), input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [href], [tabindex]:not([tabindex="-1"])'; + /* ── List ───────────────────────────────────────────────────────────── */ function FilterChips({ @@ -334,11 +352,58 @@ function EditSheet({ const [draft, setDraft] = useState(() => target === "new" ? newDraft(activeChildren) : draftFrom(target, activeChildren), ); + // The active selection as loaded: while it is unchanged, the PATCH omits + // assignee_child_ids, because the API replaces the whole set and inactive + // children cannot be sent back. + const [originalActiveIds] = useState(() => draft.assigneeIds); + const inactiveAssignees = + target === "new" + ? [] + : target.assignees.filter((a) => !activeChildren.some((child) => child.id === a.id)); const [pickerOpen, setPickerOpen] = useState(false); const [saving, setSaving] = useState(false); + const [confirming, setConfirming] = useState(false); const [error, setError] = useState(null); // A ref, not state: two taps in one frame must not both see `saving === false`. const inFlight = useRef(false); + const sheetRef = useRef(null); + const titleRef = useRef(null); + const confirmRef = useRef(null); + + // Modal focus: move in on open, hand back to the trigger on close. + useEffect(() => { + const trigger = document.activeElement as HTMLElement | null; + titleRef.current?.focus(); + return () => { + if (trigger && trigger.isConnected) trigger.focus(); + }; + }, []); + + useEffect(() => { + if (confirming) confirmRef.current?.focus(); + }, [confirming]); + + function onKeyDown(event: ReactKeyboardEvent) { + if (event.key === "Escape") { + event.preventDefault(); + if (!inFlight.current) onCancel(); + return; + } + if (event.key !== "Tab" || !sheetRef.current) return; + const focusable = Array.from(sheetRef.current.querySelectorAll(FOCUSABLE)); + if (focusable.length === 0) return; + const first = focusable[0]; + const last = focusable[focusable.length - 1]; + const current = document.activeElement; + const outside = !focusable.includes(current as HTMLElement); + if (event.shiftKey && (current === first || outside)) { + event.preventDefault(); + last.focus(); + } else if (!event.shiftKey && (current === last || outside)) { + event.preventDefault(); + first.focus(); + } + } const update = (patch: Partial) => setDraft((d) => ({ ...d, ...patch })); @@ -347,19 +412,32 @@ function EditSheet({ .map((child) => ({ id: child.id, display_name: child.display_name })); const orderedWindows = WINDOW_ORDER.filter((w) => draft.windows.includes(w)); - async function save() { + async function save(confirmed = false) { if (inFlight.current) return; - const body = buildBody(draft); + const keepsAssignees = target !== "new" && sameIds(draft.assigneeIds, originalActiveIds); + const body = buildBody(draft, keepsAssignees); if (typeof body === "string") { setError(body); + setConfirming(false); return; } + if (target !== "new" && !keepsAssignees && inactiveAssignees.length > 0 && !confirmed) { + setError(null); + setConfirming(true); + return; + } + setConfirming(false); inFlight.current = true; setSaving(true); setError(null); try { - if (target === "new") await createDefinition(body); - else await updateDefinition(target.id, body); + if (target === "new") { + await createDefinition(body); + } else { + const edit: DefinitionEditBody = { ...body }; + if (keepsAssignees) delete edit.assignee_child_ids; + await updateDefinition(target.id, edit); + } onSaved(); } catch (err) { setError(saveErrorMessage(err)); @@ -378,13 +456,15 @@ function EditSheet({ }} />
);