From 786a233e0198c89ed34ee2599fc6b54fcbe90c9e Mon Sep 17 00:00:00 2001 From: tulsi Date: Wed, 2 Sep 2026 12:21:00 -0400 Subject: [PATCH 1/2] fix: confirm before discarding skill changes --- src/features/skills/ui/SkillEditor.tsx | 77 ++++++++++- .../skills/ui/__tests__/SkillEditor.test.tsx | 130 +++++++++++++++++- src/shared/i18n/locales/en/skills.json | 4 + src/shared/i18n/locales/es/skills.json | 4 + 4 files changed, 208 insertions(+), 7 deletions(-) diff --git a/src/features/skills/ui/SkillEditor.tsx b/src/features/skills/ui/SkillEditor.tsx index dd58b2d50..a1c824a48 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"; @@ -88,6 +98,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 +136,7 @@ export function SkillEditor({ // the cards in SkillsView show. setColor(editingSkill.color ?? null); setError(null); + setDiscardDialogOpen(false); } else if (isOpen) { setName(""); setDescription(""); @@ -132,9 +144,30 @@ 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 initialEffectiveColor = + initialColor ?? resolveSkillPillTone(initialName || "new"); + const isDirty = + name !== initialName || + description !== initialDescription || + instructions !== initialInstructions || + saveLocation !== initialSaveLocation || + currentEffectiveColor !== initialEffectiveColor; + const nameValid = isValidSkillName(name); const canSave = nameValid && description.trim().length > 0 && !saving; const showNameValidationError = name.length > 0 && !nameValid; @@ -154,7 +187,8 @@ export function SkillEditor({ setError((current) => (current?.kind === "nameConflict" ? current : null)); }; - const handleClose = () => { + const discardAndClose = () => { + setDiscardDialogOpen(false); setName(""); setDescription(""); setInstructions(""); @@ -164,11 +198,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) ?? @@ -264,7 +306,7 @@ export function SkillEditor({ const isBuiltIn = false; return ( - !open && handleClose()}> + !open && requestClose()}> @@ -511,6 +553,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..509c4d942 100644 --- a/src/features/skills/ui/__tests__/SkillEditor.test.tsx +++ b/src/features/skills/ui/__tests__/SkillEditor.test.tsx @@ -1,4 +1,4 @@ -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 { SkillEditor } from "../SkillEditor"; @@ -64,6 +64,134 @@ 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("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", From 79794386c00d2b2e8a25b1896d14295322d88f62 Mon Sep 17 00:00:00 2001 From: tulsi Date: Wed, 2 Sep 2026 13:40:01 -0400 Subject: [PATCH 2/2] fix: align skill dirty state with persistence --- src/features/skills/ui/SkillEditor.tsx | 18 +++--- .../skills/ui/__tests__/SkillEditor.test.tsx | 58 +++++++++++++++++++ 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/features/skills/ui/SkillEditor.tsx b/src/features/skills/ui/SkillEditor.tsx index a1c824a48..5b3e24d6a 100644 --- a/src/features/skills/ui/SkillEditor.tsx +++ b/src/features/skills/ui/SkillEditor.tsx @@ -74,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; @@ -159,17 +163,17 @@ export function SkillEditor({ const initialColor = editingSkill?.color ?? null; const heroToneSeed = name || editingSkill?.name || "new"; const currentEffectiveColor = color ?? resolveSkillPillTone(heroToneSeed); - const initialEffectiveColor = - initialColor ?? resolveSkillPillTone(initialName || "new"); const isDirty = name !== initialName || - description !== initialDescription || + normalizeDescription(description) !== + normalizeDescription(initialDescription) || instructions !== initialInstructions || saveLocation !== initialSaveLocation || - currentEffectiveColor !== initialEffectiveColor; + 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, @@ -229,7 +233,7 @@ export function SkillEditor({ savedSkill = await updateSkill( editingSkill.path, name, - description.trim(), + normalizeDescription(description), instructions, effectiveColor, ); @@ -238,7 +242,7 @@ export function SkillEditor({ saveLocation !== GLOBAL_VALUE ? saveLocation : undefined; savedSkill = await createSkill( name, - description.trim(), + normalizeDescription(description), instructions, effectiveColor, { projectId }, diff --git a/src/features/skills/ui/__tests__/SkillEditor.test.tsx b/src/features/skills/ui/__tests__/SkillEditor.test.tsx index 509c4d942..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 { 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", () => ({ @@ -163,6 +164,63 @@ describe("SkillEditor", () => { 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();