fix: keep renamed skills open in detail view (#8935)

This commit is contained in:
Kalvin C
2026-04-30 15:07:26 -07:00
committed by GitHub
parent ef73610cc1
commit 43478a2bf6
5 changed files with 107 additions and 20 deletions
@@ -11,21 +11,26 @@ import {
DialogHeader, DialogHeader,
DialogTitle, DialogTitle,
} from "@/shared/ui/dialog"; } from "@/shared/ui/dialog";
import { createSkill, updateSkill, type EditingSkill } from "../api/skills"; import {
createSkill,
updateSkill,
type EditingSkill,
type SkillInfo,
} from "../api/skills";
import { formatSkillName, isValidSkillName } from "../lib/skillsHelpers"; import { formatSkillName, isValidSkillName } from "../lib/skillsHelpers";
import { getRenamedSkillFileLocation } from "../lib/skillsPath"; import { getRenamedSkillFileLocation } from "../lib/skillsPath";
interface SkillEditorProps { interface SkillEditorProps {
isOpen: boolean; isOpen: boolean;
onClose: () => void; onClose: () => void;
onCreated?: () => void; onSaved?: (savedSkill?: SkillInfo) => void | Promise<void>;
editingSkill?: EditingSkill; editingSkill?: EditingSkill;
} }
export function SkillEditor({ export function SkillEditor({
isOpen, isOpen,
onClose, onClose,
onCreated, onSaved,
editingSkill, editingSkill,
}: SkillEditorProps) { }: SkillEditorProps) {
const { t } = useTranslation(["skills", "common"]); const { t } = useTranslation(["skills", "common"]);
@@ -74,8 +79,9 @@ export function SkillEditor({
setSaving(true); setSaving(true);
setError(null); setError(null);
try { try {
let savedSkill: SkillInfo | undefined;
if (isEditing) { if (isEditing) {
await updateSkill( savedSkill = await updateSkill(
editingSkill.path, editingSkill.path,
name, name,
description.trim(), description.trim(),
@@ -87,7 +93,7 @@ export function SkillEditor({
setName(""); setName("");
setDescription(""); setDescription("");
setInstructions(""); setInstructions("");
onCreated?.(); await onSaved?.(savedSkill);
onClose(); onClose();
} catch (err) { } catch (err) {
setError(String(err)); setError(String(err));
@@ -16,7 +16,7 @@ import type { EditingSkill, SkillInfo } from "../api/skills";
interface SkillsDialogsProps { interface SkillsDialogsProps {
dialogOpen: boolean; dialogOpen: boolean;
onDialogClose: () => void; onDialogClose: () => void;
onCreated: () => void | Promise<void>; onSaved: (savedSkill?: SkillInfo) => void | Promise<void>;
editingSkill?: EditingSkill; editingSkill?: EditingSkill;
deletingSkill: SkillInfo | null; deletingSkill: SkillInfo | null;
onDeletingSkillChange: (skill: SkillInfo | null) => void; onDeletingSkillChange: (skill: SkillInfo | null) => void;
@@ -26,7 +26,7 @@ interface SkillsDialogsProps {
export function SkillsDialogs({ export function SkillsDialogs({
dialogOpen, dialogOpen,
onDialogClose, onDialogClose,
onCreated, onSaved,
editingSkill, editingSkill,
deletingSkill, deletingSkill,
onDeletingSkillChange, onDeletingSkillChange,
@@ -39,7 +39,7 @@ export function SkillsDialogs({
<SkillEditor <SkillEditor
isOpen={dialogOpen} isOpen={dialogOpen}
onClose={onDialogClose} onClose={onDialogClose}
onCreated={onCreated} onSaved={onSaved}
editingSkill={editingSkill} editingSkill={editingSkill}
/> />
@@ -55,7 +55,7 @@ export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) {
const [expandedSectionIds, setExpandedSectionIds] = useState<string[]>([]); const [expandedSectionIds, setExpandedSectionIds] = useState<string[]>([]);
const loadRequestIdRef = useRef(0); const loadRequestIdRef = useRef(0);
const loadSkills = useCallback(async () => { const loadSkills = useCallback(async (): Promise<SkillViewInfo[]> => {
const requestId = loadRequestIdRef.current + 1; const requestId = loadRequestIdRef.current + 1;
loadRequestIdRef.current = requestId; loadRequestIdRef.current = requestId;
setLoading(true); setLoading(true);
@@ -64,16 +64,19 @@ export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) {
const projectDirs = projects.flatMap((project) => project.workingDirs); const projectDirs = projects.flatMap((project) => project.workingDirs);
const result = await listSkills(projectDirs); const result = await listSkills(projectDirs);
if (loadRequestIdRef.current !== requestId) { if (loadRequestIdRef.current !== requestId) {
return; return [];
} }
setSkills( const nextSkills = withInferredSkillCategories(
withInferredSkillCategories(hydrateProjectNames(result, projects)), hydrateProjectNames(result, projects),
); );
setSkills(nextSkills);
return nextSkills;
} catch { } catch {
if (loadRequestIdRef.current === requestId) { if (loadRequestIdRef.current === requestId) {
setSkills([]); setSkills([]);
toast.error(t("view.loadError")); toast.error(t("view.loadError"));
} }
return [];
} finally { } finally {
if (loadRequestIdRef.current === requestId) { if (loadRequestIdRef.current === requestId) {
setLoading(false); setLoading(false);
@@ -190,6 +193,23 @@ export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) {
setDialogOpen(true); setDialogOpen(true);
}; };
const handleSkillSaved = useCallback(
async (savedSkill?: SkillInfo) => {
const refreshedSkills = await loadSkills();
if (
savedSkill &&
refreshedSkills.some((skill) => skill.id === savedSkill.id)
) {
setActiveSkillId(savedSkill.id);
}
},
[loadSkills],
);
const refreshSkills = useCallback(async () => {
await loadSkills();
}, [loadSkills]);
const { const {
fileInputRef, fileInputRef,
isDragOver, isDragOver,
@@ -197,7 +217,7 @@ export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) {
handleFileChange, handleFileChange,
openFilePicker, openFilePicker,
handleExport, handleExport,
} = useSkillImportExport(loadSkills); } = useSkillImportExport(refreshSkills);
const handleSelectSkill = (skill: SkillViewInfo) => { const handleSelectSkill = (skill: SkillViewInfo) => {
setActiveSkillId(skill.id); setActiveSkillId(skill.id);
@@ -207,7 +227,7 @@ export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) {
<SkillsDialogs <SkillsDialogs
dialogOpen={dialogOpen} dialogOpen={dialogOpen}
onDialogClose={handleDialogClose} onDialogClose={handleDialogClose}
onCreated={loadSkills} onSaved={handleSkillSaved}
editingSkill={editingSkill} editingSkill={editingSkill}
deletingSkill={deletingSkill} deletingSkill={deletingSkill}
onDeletingSkillChange={setDeletingSkill} onDeletingSkillChange={setDeletingSkill}
@@ -20,7 +20,7 @@ const { createSkill, updateSkill } = await import("../../api/skills");
const defaultProps = { const defaultProps = {
isOpen: true, isOpen: true,
onClose: vi.fn(), onClose: vi.fn(),
onCreated: vi.fn(), onSaved: vi.fn(),
}; };
describe("SkillEditor", () => { describe("SkillEditor", () => {
@@ -308,10 +308,10 @@ describe("SkillEditor", () => {
); );
}); });
it("calls onCreated callback after successful save", async () => { it("calls onSaved callback after successful save", async () => {
const user = userEvent.setup(); const user = userEvent.setup();
const onCreated = vi.fn(); const onSaved = vi.fn();
render(<SkillEditor {...defaultProps} onCreated={onCreated} />); render(<SkillEditor {...defaultProps} onSaved={onSaved} />);
await user.type(screen.getByPlaceholderText("my-skill-name"), "my-skill"); await user.type(screen.getByPlaceholderText("my-skill-name"), "my-skill");
await user.type( await user.type(
@@ -320,7 +320,7 @@ describe("SkillEditor", () => {
); );
await user.click(screen.getByRole("button", { name: /create skill/i })); await user.click(screen.getByRole("button", { name: /create skill/i }));
expect(onCreated).toHaveBeenCalled(); expect(onSaved).toHaveBeenCalled();
}); });
it("clears fields after save", async () => { it("clears fields after save", async () => {
@@ -62,6 +62,18 @@ const mockSkills: SkillInfo[] = [
vi.mock("../../api/skills", () => ({ vi.mock("../../api/skills", () => ({
listSkills: vi.fn().mockResolvedValue([]), listSkills: vi.fn().mockResolvedValue([]),
createSkill: vi.fn().mockResolvedValue(undefined),
updateSkill: vi.fn().mockResolvedValue({
id: "global:/path/renamed-review",
name: "renamed-review",
description: "Reviews code",
instructions: "Review the code...",
path: "/path/renamed-review",
fileLocation: "/path/renamed-review/SKILL.md",
sourceKind: "global",
sourceLabel: "Personal",
projectLinks: [],
}),
deleteSkill: vi.fn().mockResolvedValue(undefined), deleteSkill: vi.fn().mockResolvedValue(undefined),
exportSkill: vi exportSkill: vi
.fn() .fn()
@@ -75,11 +87,12 @@ vi.mock("@/features/projects/stores/projectStore", () => ({
) => selector({ projects: mockProjects }), ) => selector({ projects: mockProjects }),
})); }));
const { listSkills, deleteSkill } = (await import( const { listSkills, deleteSkill, updateSkill } = (await import(
"../../api/skills" "../../api/skills"
)) as unknown as { )) as unknown as {
listSkills: ReturnType<typeof vi.fn>; listSkills: ReturnType<typeof vi.fn>;
deleteSkill: ReturnType<typeof vi.fn>; deleteSkill: ReturnType<typeof vi.fn>;
updateSkill: ReturnType<typeof vi.fn>;
}; };
beforeEach(() => { beforeEach(() => {
@@ -288,6 +301,54 @@ describe("SkillsView", () => {
expect(screen.queryByText("code-review")).not.toBeInTheDocument(); expect(screen.queryByText("code-review")).not.toBeInTheDocument();
}); });
it("stays on the detail page after renaming a skill", async () => {
const renamedSkill: SkillInfo = {
...mockSkills[1],
id: "global:/path/renamed-review",
name: "renamed-review",
path: "/path/renamed-review",
fileLocation: "/path/renamed-review/SKILL.md",
};
listSkills
.mockResolvedValueOnce(mockSkills)
.mockResolvedValueOnce([mockSkills[0], renamedSkill, mockSkills[2]]);
updateSkill.mockResolvedValueOnce(renamedSkill);
const user = userEvent.setup();
render(<SkillsView />);
await screen.findByText("code-review");
await user.click(
screen.getByRole("button", { name: "Open code-review details" }),
);
await user.click(screen.getByRole("button", { name: "Edit" }));
const nameInput = screen.getByPlaceholderText("my-skill-name");
await user.clear(nameInput);
await user.type(nameInput, "renamed-review");
await user.click(screen.getByRole("button", { name: /save changes/i }));
await waitFor(() => {
expect(updateSkill).toHaveBeenCalledWith(
"/path/code-review",
"renamed-review",
"Reviews code",
"Review the code...",
);
});
await waitFor(() => {
expect(
screen.getByRole("button", { name: "Back to skills" }),
).toBeInTheDocument();
expect(
screen.getByRole("heading", { name: "renamed-review" }),
).toBeInTheDocument();
});
expect(
screen.queryByPlaceholderText("my-skill-name"),
).not.toBeInTheDocument();
});
it("filters skills by search text", async () => { it("filters skills by search text", async () => {
listSkills.mockResolvedValue(mockSkills); listSkills.mockResolvedValue(mockSkills);
const user = userEvent.setup(); const user = userEvent.setup();