diff --git a/src/features/skills/ui/SkillEditor.tsx b/src/features/skills/ui/SkillEditor.tsx index dd58b2d50..5b3e24d6a 100644 --- a/src/features/skills/ui/SkillEditor.tsx +++ b/src/features/skills/ui/SkillEditor.tsx @@ -3,7 +3,17 @@ import { useTranslation } from "react-i18next"; import { AlertCircle, Copy, Trash2 } from "lucide-react"; import { cn } from "@/shared/lib/cn"; import { formatAcpErrorMessage } from "@/shared/api/acpErrors"; -import { Button } from "@/shared/ui/button"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@/shared/ui/alert-dialog"; +import { Button, buttonVariants } from "@/shared/ui/button"; import { Input } from "@/shared/ui/input"; import { Label } from "@/shared/ui/label"; import { Textarea } from "@/shared/ui/textarea"; @@ -64,6 +74,10 @@ function getDuplicateSourceName(message: string): string | null { return match?.[1] ?? match?.[2] ?? null; } +function normalizeDescription(description: string): string { + return description.trim(); +} + interface SkillEditorProps { isOpen: boolean; onClose: () => void; @@ -88,6 +102,7 @@ export function SkillEditor({ const [saveLocation, setSaveLocation] = useState(GLOBAL_VALUE); const [saving, setSaving] = useState(false); const [error, setError] = useState(null); + const [discardDialogOpen, setDiscardDialogOpen] = useState(false); const [formHasScrollBelow, setFormHasScrollBelow] = useState(false); // null = "no explicit pick yet"; the hero falls through to the deterministic // name-hash tone so the editor still has visual identity before the user @@ -125,6 +140,7 @@ export function SkillEditor({ // the cards in SkillsView show. setColor(editingSkill.color ?? null); setError(null); + setDiscardDialogOpen(false); } else if (isOpen) { setName(""); setDescription(""); @@ -132,11 +148,32 @@ export function SkillEditor({ setSaveLocation(initialProjectId ?? GLOBAL_VALUE); setColor(null); setError(null); + setDiscardDialogOpen(false); + } else { + setDiscardDialogOpen(false); } } + const initialName = editingSkill?.name ?? ""; + const initialDescription = editingSkill?.description ?? ""; + const initialInstructions = editingSkill?.instructions ?? ""; + const initialSaveLocation = editingSkill + ? GLOBAL_VALUE + : (initialProjectId ?? GLOBAL_VALUE); + const initialColor = editingSkill?.color ?? null; + const heroToneSeed = name || editingSkill?.name || "new"; + const currentEffectiveColor = color ?? resolveSkillPillTone(heroToneSeed); + const isDirty = + name !== initialName || + normalizeDescription(description) !== + normalizeDescription(initialDescription) || + instructions !== initialInstructions || + saveLocation !== initialSaveLocation || + color !== initialColor; + const nameValid = isValidSkillName(name); - const canSave = nameValid && description.trim().length > 0 && !saving; + const canSave = + nameValid && normalizeDescription(description).length > 0 && !saving; const showNameValidationError = name.length > 0 && !nameValid; const nameInputDescribedBy = [ showNameValidationError ? "skill-name-validation" : null, @@ -154,7 +191,8 @@ export function SkillEditor({ setError((current) => (current?.kind === "nameConflict" ? current : null)); }; - const handleClose = () => { + const discardAndClose = () => { + setDiscardDialogOpen(false); setName(""); setDescription(""); setInstructions(""); @@ -164,11 +202,19 @@ export function SkillEditor({ onClose(); }; + const requestClose = () => { + if (saving) return; + if (isDirty) { + setDiscardDialogOpen(true); + return; + } + discardAndClose(); + }; + // Effective tone: user pick wins, otherwise derive from the seed. Same // resolver used by SkillsView cards so an unpicked skill shows identical // color in both surfaces. - const heroToneSeed = name || editingSkill?.name || "new"; - const effectiveColor = color ?? resolveSkillPillTone(heroToneSeed); + const effectiveColor = currentEffectiveColor; const fallbackPanelTone = resolveSkillPillTone(heroToneSeed); const selectedPanelColor = pillCssColor(effectiveColor) ?? @@ -187,7 +233,7 @@ export function SkillEditor({ savedSkill = await updateSkill( editingSkill.path, name, - description.trim(), + normalizeDescription(description), instructions, effectiveColor, ); @@ -196,7 +242,7 @@ export function SkillEditor({ saveLocation !== GLOBAL_VALUE ? saveLocation : undefined; savedSkill = await createSkill( name, - description.trim(), + normalizeDescription(description), instructions, effectiveColor, { projectId }, @@ -264,7 +310,7 @@ export function SkillEditor({ const isBuiltIn = false; return ( - !open && handleClose()}> + !open && requestClose()}> @@ -511,6 +557,29 @@ export function SkillEditor({ + + + + + {t("dialog.discardTitle")} + + {t("dialog.discardDescription")} + + + + {t("dialog.keepEditing")} + + {t("dialog.discard")} + + + + ); } diff --git a/src/features/skills/ui/__tests__/SkillEditor.test.tsx b/src/features/skills/ui/__tests__/SkillEditor.test.tsx index b8f13f83d..0489c6b77 100644 --- a/src/features/skills/ui/__tests__/SkillEditor.test.tsx +++ b/src/features/skills/ui/__tests__/SkillEditor.test.tsx @@ -1,6 +1,7 @@ -import { render, screen } from "@testing-library/react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { describe, expect, it, vi, beforeEach } from "vitest"; +import { resolveSkillPillTone } from "../../lib/resolveSkillPillTone"; import { SkillEditor } from "../SkillEditor"; vi.mock("../../api/skills", () => ({ @@ -64,6 +65,191 @@ describe("SkillEditor", () => { }); }); + // ── Closing ─────────────────────────────────────────────────────── + + describe("closing", () => { + it("closes immediately when no fields have changed", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onClose).toHaveBeenCalledOnce(); + expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument(); + }); + + it("asks for confirmation before closing with unsaved input", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + const nameInput = screen.getByPlaceholderText("my-skill-name"); + await user.type(nameInput, "work-in-progress"); + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onClose).not.toHaveBeenCalled(); + expect( + screen.getByRole("alertdialog", { + name: "Discard unsaved changes?", + }), + ).toBeInTheDocument(); + expect(nameInput).toHaveValue("work-in-progress"); + }); + + it("keeps the editor and its input when discard is canceled", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + const nameInput = screen.getByPlaceholderText("my-skill-name"); + await user.type(nameInput, "work-in-progress"); + await user.click(screen.getByRole("button", { name: "Cancel" })); + await user.click(screen.getByRole("button", { name: "Keep editing" })); + + expect(onClose).not.toHaveBeenCalled(); + expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument(); + expect(nameInput).toHaveValue("work-in-progress"); + }); + + it("discards input and closes after confirmation", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.type( + screen.getByPlaceholderText("my-skill-name"), + "work-in-progress", + ); + await user.click(screen.getByRole("button", { name: "Cancel" })); + await user.click(screen.getByRole("button", { name: "Discard" })); + + expect(onClose).toHaveBeenCalledOnce(); + }); + + it("asks for confirmation when Escape would close dirty input", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.type( + screen.getByPlaceholderText("my-skill-name"), + "work-in-progress", + ); + await act(async () => { + fireEvent.keyDown(screen.getByRole("dialog"), { key: "Escape" }); + }); + + expect(onClose).not.toHaveBeenCalled(); + expect(await screen.findByRole("alertdialog")).toBeInTheDocument(); + }); + + it("asks for confirmation when clicking outside dirty input", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.type( + screen.getByPlaceholderText("my-skill-name"), + "work-in-progress", + ); + const overlay = document.querySelector( + '[data-slot="sheet-overlay"]', + ); + expect(overlay).not.toBeNull(); + if (!overlay) throw new Error("Expected the skill editor overlay"); + await user.click(overlay); + + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole("alertdialog")).toBeInTheDocument(); + }); + + it("treats pinning the derived color as an unsaved change", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + const skillName = "code-review"; + render( + , + ); + + const derivedColor = resolveSkillPillTone(skillName); + await user.click( + screen.getByRole("button", { name: `Color ${derivedColor}` }), + ); + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole("alertdialog")).toBeInTheDocument(); + }); + + it("ignores description whitespace that is trimmed when saved", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render( + , + ); + + const descriptionInput = screen.getByPlaceholderText( + "What it does and when to use it...", + ); + await user.type(descriptionInput, " "); + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onClose).toHaveBeenCalledOnce(); + expect(screen.queryByRole("alertdialog")).not.toBeInTheDocument(); + }); + + it("asks for confirmation before discarding edits to an existing skill", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render( + , + ); + + const descriptionInput = screen.getByPlaceholderText( + "What it does and when to use it...", + ); + await user.type(descriptionInput, " with extra care"); + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole("alertdialog")).toBeInTheDocument(); + }); + }); + // ── Name validation ──────────────────────────────────────────────── describe("name validation", () => { diff --git a/src/shared/i18n/locales/en/skills.json b/src/shared/i18n/locales/en/skills.json index 1681f8bbd..64014bd44 100644 --- a/src/shared/i18n/locales/en/skills.json +++ b/src/shared/i18n/locales/en/skills.json @@ -6,8 +6,12 @@ "customize": "Customize", "customizeComingSoon": "Customize (coming soon)", "description": "Description", + "discard": "Discard", + "discardDescription": "Your unsaved changes will be lost.", + "discardTitle": "Discard unsaved changes?", "duplicate": "Duplicate", "descriptionPlaceholder": "What it does and when to use it...", + "keepEditing": "Keep editing", "editTitle": "Edit skill", "global": "Personal", "globalHint": "Available to all sessions", diff --git a/src/shared/i18n/locales/es/skills.json b/src/shared/i18n/locales/es/skills.json index bad56a9c4..94a19cd4c 100644 --- a/src/shared/i18n/locales/es/skills.json +++ b/src/shared/i18n/locales/es/skills.json @@ -6,8 +6,12 @@ "customize": "Personalizar", "customizeComingSoon": "Personalizar (próximamente)", "description": "Descripción", + "discard": "Descartar", + "discardDescription": "Se perderán los cambios que no hayas guardado.", + "discardTitle": "¿Descartar los cambios sin guardar?", "duplicate": "Duplicar", "descriptionPlaceholder": "Qué hace y cuándo usarla...", + "keepEditing": "Seguir editando", "editTitle": "Editar skill", "global": "Personal", "globalHint": "Disponible para todas las sesiones",