From 34aa20387b969383e18faf0f0ba8abcc30a71379 Mon Sep 17 00:00:00 2001 From: Nickk'z Date: Fri, 30 Oct 2026 03:55:14 +0530 Subject: [PATCH] Let people delete skills and playbooks from the library Skills and playbooks could be created and imported but never removed in the app, even though the backend already exposes a delete endpoint. This adds a Delete action to the skill detail dialog and to the playbook detail. Both surfaces share one small component, SkillDeleteAction, which reuses the existing InlineDeleteConfirm strip: a quiet Delete button opens a confirm with Cancel and a red Delete, calls the delete endpoint, refreshes the list and closes the dialog, and shows a plain error line if the call fails. Deleting the last variant of a grouped skill closes the dialog; deleting one of several leaves the rest open. A test covers the confirm and list refresh, Cancel leaving the item untouched, and the failure path. Closes #351. --- .../(library)/playbooks/playbook-detail.tsx | 11 ++ .../(library)/playbooks/playbooks-view.tsx | 4 + .../skill-delete-action.interaction.test.tsx | 148 ++++++++++++++++++ .../(library)/skills/skill-delete-action.tsx | 77 +++++++++ .../(library)/skills/skill-detail-dialog.tsx | 12 ++ 5 files changed, 252 insertions(+) create mode 100644 frontend/app/(library)/skills/skill-delete-action.interaction.test.tsx create mode 100644 frontend/app/(library)/skills/skill-delete-action.tsx diff --git a/frontend/app/(library)/playbooks/playbook-detail.tsx b/frontend/app/(library)/playbooks/playbook-detail.tsx index ba3062c47..9f9831b5f 100644 --- a/frontend/app/(library)/playbooks/playbook-detail.tsx +++ b/frontend/app/(library)/playbooks/playbook-detail.tsx @@ -10,6 +10,7 @@ import { import { Chip, type ChipProps } from "@/components/base/badges/chip"; import { Button } from "@/components/base/buttons/button"; import * as Modal from "@/components/base/modal/modal"; +import { SkillDeleteAction } from "@/app/(library)/skills/skill-delete-action"; import { tagChipColor, usageCaption, type Skill } from "@/app/(library)/skills/skills-data"; import type { ChipColor } from "@/app/(library)/knowledge/knowledge-data"; import { CHIP_COLOR } from "@/components/foundations/form-recipes"; @@ -92,12 +93,15 @@ export function PlaybookDetail({ playbook, onEdit, onRun, + onDeleted, }: { open: boolean; onOpenChange: (open: boolean) => void; playbook: Skill | null; onEdit: (playbook: Skill) => void; onRun: (playbook: Skill) => void; + /** Deleted for good: the parent closes this dialog and refetches the list. */ + onDeleted: () => void | Promise; }) { return ( @@ -178,6 +182,13 @@ export function PlaybookDetail({ + + )} diff --git a/frontend/app/(library)/playbooks/playbooks-view.tsx b/frontend/app/(library)/playbooks/playbooks-view.tsx index 2cee62265..72d5c51e9 100644 --- a/frontend/app/(library)/playbooks/playbooks-view.tsx +++ b/frontend/app/(library)/playbooks/playbooks-view.tsx @@ -260,6 +260,10 @@ export function PlaybooksView({ playbook={detail} onEdit={openEdit} onRun={runPlaybook} + onDeleted={() => { + setDetailOpen(false); + void refetch(); + }} /> Promise }; +const hookHolder = globalThis as unknown as { __skillsApiHook?: SkillsApiHook }; + +mock.module("@/app/(library)/skills/skills-api", () => ({ + deleteSkill: (id: string) => hookHolder.__skillsApiHook?.deleteSkill(id), +})); + +const { act } = await import("react"); +const { createRoot } = await import("react-dom/client"); +const { SkillDeleteAction } = await import("./skill-delete-action"); + +let root: ReturnType | null = null; +let container: HTMLElement | null = null; + +async function flush() { + // Let the delete promise and the state updates it schedules settle. + await act(async () => { + await Promise.resolve(); + }); +} + +async function mount(onDeleted: () => void | Promise) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + await act(async () => { + root?.render( + , + ); + }); +} + +/** First button whose label contains `label`, scoped to the confirm strip when + * asked so the trigger Delete and the strip's Delete don't collide. */ +function button(label: string, inConfirm = false): HTMLButtonElement { + const scope = inConfirm + ? (container?.querySelector('[data-testid="inline-delete-confirm"]') as HTMLElement | null) + : container; + const match = Array.from(scope?.querySelectorAll("button") ?? []).find((el) => + (el.textContent ?? "").includes(label), + ); + if (!match) throw new Error(`no button labelled ${label}`); + return match as HTMLButtonElement; +} + +function confirmStrip(): HTMLElement | null { + return container?.querySelector('[data-testid="inline-delete-confirm"]') ?? null; +} + +async function click(el: HTMLButtonElement) { + await act(async () => { + el.dispatchEvent(new window.MouseEvent("click", { bubbles: true })); + }); +} + +beforeEach(() => { + deleteCalls = []; + deleteShouldFail = false; + hookHolder.__skillsApiHook = { + deleteSkill: async (id: string) => { + deleteCalls.push(id); + if (deleteShouldFail) throw new Error("delete skill 500"); + }, + }; +}); + +afterEach(() => { + if (root) act(() => root?.unmount()); + container?.remove(); + root = null; + container = null; +}); + +describe("SkillDeleteAction", () => { + test("confirming deletes the skill and asks the caller to refresh", async () => { + let refreshed = 0; + await mount(() => { + refreshed += 1; + }); + + // The confirm strip only appears after the Delete trigger is pressed. + expect(confirmStrip()).toBeNull(); + await click(button("Delete")); + expect(confirmStrip()).not.toBeNull(); + + await click(button("Delete", true)); + await flush(); + + expect(deleteCalls).toEqual(["k1"]); + expect(refreshed).toBe(1); + // The flow finished, so the confirm strip is gone again. + expect(confirmStrip()).toBeNull(); + }); + + test("Cancel leaves the skill untouched", async () => { + let refreshed = 0; + await mount(() => { + refreshed += 1; + }); + + await click(button("Delete")); + await click(button("Cancel", true)); + + expect(deleteCalls).toEqual([]); + expect(refreshed).toBe(0); + expect(confirmStrip()).toBeNull(); + }); + + test("a failed delete shows a plain error line and does not refresh", async () => { + deleteShouldFail = true; + let refreshed = 0; + await mount(() => { + refreshed += 1; + }); + + await click(button("Delete")); + await click(button("Delete", true)); + await flush(); + + expect(deleteCalls).toEqual(["k1"]); + expect(refreshed).toBe(0); + const alert = container?.querySelector('[role="alert"]'); + expect(alert?.textContent ?? "").toContain("Delete failed"); + // Still confirming, so the user can retry after the error. + expect(confirmStrip()).not.toBeNull(); + }); +}); diff --git a/frontend/app/(library)/skills/skill-delete-action.tsx b/frontend/app/(library)/skills/skill-delete-action.tsx new file mode 100644 index 000000000..cb7c749f2 --- /dev/null +++ b/frontend/app/(library)/skills/skill-delete-action.tsx @@ -0,0 +1,77 @@ +"use client"; + +import { useState } from "react"; +import { RiDeleteBinLine } from "@remixicon/react"; + +import { Button } from "@/components/base/buttons/button"; +import { InlineDeleteConfirm } from "@/components/shared/inline-delete-confirm"; +import { deleteSkill } from "./skills-api"; + +/** + * The whole delete flow for one skill or playbook (both are skills, one DELETE + * endpoint): a Delete button that opens the shared confirm strip, the backend + * call, and a plain error line if it fails. On success the caller refreshes its + * list (and closes the dialog) via `onDeleted`. Reused by the skill detail + * dialog and the playbook detail so delete asks and behaves the same in both. + */ +export function SkillDeleteAction({ + skillId, + question, + consequence, + onDeleted, +}: { + skillId: string; + question: string; + consequence: string; + /** Deleted for good - the caller refetches its list and closes if needed. */ + onDeleted: () => void | Promise; +}) { + const [confirming, setConfirming] = useState(false); + const [deleting, setDeleting] = useState(false); + const [error, setError] = useState(null); + + const onConfirm = async () => { + setDeleting(true); + setError(null); + try { + await deleteSkill(skillId); + setConfirming(false); + await onDeleted(); + } catch { + setError("Delete failed. Check the backend and try again."); + } finally { + setDeleting(false); + } + }; + + return ( +
+ + {confirming && ( + <> + setConfirming(false)} + onConfirm={onConfirm} + /> + {error && ( +

+ {error} +

+ )} + + )} +
+ ); +} diff --git a/frontend/app/(library)/skills/skill-detail-dialog.tsx b/frontend/app/(library)/skills/skill-detail-dialog.tsx index 0d9ea7329..fa0824234 100644 --- a/frontend/app/(library)/skills/skill-detail-dialog.tsx +++ b/frontend/app/(library)/skills/skill-detail-dialog.tsx @@ -6,6 +6,7 @@ import { RiPlayMiniLine, RiRefreshLine } from "@remixicon/react"; import { Chip, type ChipProps } from "@/components/base/badges/chip"; import { Button } from "@/components/base/buttons/button"; import * as Modal from "@/components/base/modal/modal"; +import { SkillDeleteAction } from "./skill-delete-action"; import { fetchSkill, importSkillPaths } from "./skills-api"; import { sourceRepoLabel, @@ -113,6 +114,7 @@ function VariantBlock({ onRun, resync, onResync, + onDeleted, }: { skill: Skill; detail: VariantDetail; @@ -120,6 +122,8 @@ function VariantBlock({ onRun: (skill: Skill) => void; resync: ResyncState | undefined; onResync: (skill: Skill) => void; + /** A variant was deleted - refetch the library (and close if it was last). */ + onDeleted: () => void | Promise; }) { const imported = Boolean(skill.sourceRepo && skill.sourcePath); const syncing = resync?.status === "syncing"; @@ -207,6 +211,13 @@ function VariantBlock({ )} + + ); } @@ -328,6 +339,7 @@ export function SkillDetailDialog({ onRun={onRun} resync={resyncById[skill.id]} onResync={onResync} + onDeleted={onChanged} /> ))}