From c9efec294b59e8980f5ed3326f9c3498431d5bb1 Mon Sep 17 00:00:00 2001 From: mrzhao Date: Mon, 18 May 2026 11:45:54 +0800 Subject: [PATCH] fix(skills): install correct skill from skills.sh search results (#2784) * fix(skills): install correct skill from skills.sh search results When multiple skills share the same directory name across different repos, SkillCard was passing directory to onInstall/onUninstall, causing handleInstall to always match the first result. Switch to using the unique key field (directory:repoOwner:repoName) for precise identification. * test(skills): add regression test for skills.sh install by key Verifies that clicking install on the second card when two skills share the same directory name correctly installs the second skill, not the first. --------- Co-authored-by: mrzhao --- src/components/skills/SkillCard.tsx | 8 +- src/components/skills/SkillsPage.tsx | 10 +- tests/components/SkillsPageInstall.test.tsx | 159 ++++++++++++++++++++ 3 files changed, 166 insertions(+), 11 deletions(-) create mode 100644 tests/components/SkillsPageInstall.test.tsx diff --git a/src/components/skills/SkillCard.tsx b/src/components/skills/SkillCard.tsx index 2b47c40ca..3fbedff1c 100644 --- a/src/components/skills/SkillCard.tsx +++ b/src/components/skills/SkillCard.tsx @@ -18,8 +18,8 @@ type SkillCardSkill = DiscoverableSkill & { installed: boolean }; interface SkillCardProps { skill: SkillCardSkill; - onInstall: (directory: string) => Promise; - onUninstall: (directory: string) => Promise; + onInstall: (key: string) => Promise; + onUninstall: (key: string) => Promise; installs?: number; } @@ -35,7 +35,7 @@ export function SkillCard({ const handleInstall = async () => { setLoading(true); try { - await onInstall(skill.directory); + await onInstall(skill.key); } finally { setLoading(false); } @@ -44,7 +44,7 @@ export function SkillCard({ const handleUninstall = async () => { setLoading(true); try { - await onUninstall(skill.directory); + await onUninstall(skill.key); } finally { setLoading(false); } diff --git a/src/components/skills/SkillsPage.tsx b/src/components/skills/SkillsPage.tsx index c24d68e94..c5a801e3a 100644 --- a/src/components/skills/SkillsPage.tsx +++ b/src/components/skills/SkillsPage.tsx @@ -194,20 +194,16 @@ export const SkillsPage = forwardRef( readmeUrl: s.readmeUrl, }); - const handleInstall = async (directory: string) => { + const handleInstall = async (key: string) => { let skill: DiscoverableSkill | undefined; if (searchSource === "skillssh") { - const found = accumulatedResults.find((s) => s.directory === directory); + const found = accumulatedResults.find((s) => s.key === key); if (found) { skill = toDiscoverableSkill(found); } } else { - skill = discoverableSkills?.find( - (s) => - s.directory === directory || - s.directory.split("/").pop() === directory, - ); + skill = discoverableSkills?.find((s) => s.key === key); } if (!skill) { diff --git a/tests/components/SkillsPageInstall.test.tsx b/tests/components/SkillsPageInstall.test.tsx new file mode 100644 index 000000000..09b520129 --- /dev/null +++ b/tests/components/SkillsPageInstall.test.tsx @@ -0,0 +1,159 @@ +import { createRef } from "react"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi, beforeEach } from "vitest"; + +import { + SkillsPage, + type SkillsPageHandle, +} from "@/components/skills/SkillsPage"; +import type { + DiscoverableSkill, + SkillsShDiscoverableSkill, + SkillsShSearchResult, +} from "@/lib/api/skills"; + +const installMutateAsyncMock = vi.fn(); + +// Stable cache so repeated renders see referentially-equal data. +// SkillsPage has `useEffect([skillsShResult, ...])` that calls setState — a +// fresh object every render would loop forever. +const searchCache = new Map< + string, + { data: SkillsShSearchResult | undefined; isLoading: boolean; isFetching: boolean } +>(); + +const setSearchResult = ( + query: string, + offset: number, + result: SkillsShSearchResult | undefined, +) => { + searchCache.set(`${query}:${offset}`, { + data: result, + isLoading: false, + isFetching: false, + }); +}; + +vi.mock("sonner", () => ({ + toast: { + success: vi.fn(), + error: vi.fn(), + info: vi.fn(), + }, +})); + +vi.mock("@/hooks/useSkills", () => ({ + useDiscoverableSkills: () => ({ + data: [] as DiscoverableSkill[], + isLoading: false, + isFetching: false, + refetch: vi.fn(), + }), + useInstalledSkills: () => ({ + data: [], + isLoading: false, + }), + useInstallSkill: () => ({ + mutateAsync: installMutateAsyncMock, + }), + useSkillRepos: () => ({ + data: [], + refetch: vi.fn(), + }), + useAddSkillRepo: () => ({ + mutateAsync: vi.fn(), + }), + useRemoveSkillRepo: () => ({ + mutateAsync: vi.fn(), + }), + useSearchSkillsSh: (query: string, _limit: number, offset: number) => { + const cached = searchCache.get(`${query}:${offset}`); + if (cached) return cached; + return { data: undefined, isLoading: false, isFetching: false }; + }, +})); + +const makeSkillsShSkill = ( + overrides: Partial = {}, +): SkillsShDiscoverableSkill => ({ + key: "agent-browser:owner-a:repo-a", + name: "Agent Browser", + directory: "agent-browser", + repoOwner: "owner-a", + repoName: "repo-a", + repoBranch: "main", + installs: 100, + readmeUrl: "https://example.com/a", + ...overrides, +}); + +describe("SkillsPage - skills.sh install (regression)", () => { + beforeEach(() => { + installMutateAsyncMock.mockReset(); + installMutateAsyncMock.mockResolvedValue({}); + searchCache.clear(); + }); + + it("installs the second skill when two results share the same directory", async () => { + const first = makeSkillsShSkill({ + key: "agent-browser:owner-a:repo-a", + name: "Agent Browser A", + repoOwner: "owner-a", + repoName: "repo-a", + }); + const second = makeSkillsShSkill({ + key: "agent-browser:owner-b:repo-b", + name: "Agent Browser B", + repoOwner: "owner-b", + repoName: "repo-b", + }); + + setSearchResult("agent", 0, { + skills: [first, second], + totalCount: 2, + query: "agent", + }); + + const ref = createRef(); + render(); + + const user = userEvent.setup(); + + // Switch to skills.sh source + await user.click(screen.getByRole("button", { name: /skills\.sh/i })); + + // Type a query and submit + const input = screen.getByPlaceholderText( + "skills.skillssh.searchPlaceholder", + ); + await user.type(input, "agent"); + await user.click(screen.getByRole("button", { name: "skills.search" })); + + // Wait for both cards to render + await waitFor(() => { + expect(screen.getByText("Agent Browser A")).toBeInTheDocument(); + expect(screen.getByText("Agent Browser B")).toBeInTheDocument(); + }); + + // Click install on the SECOND card (Agent Browser B) + const secondCard = screen + .getByText("Agent Browser B") + .closest("div.glass-card"); + expect(secondCard).not.toBeNull(); + const installButton = secondCard!.querySelector( + "button:last-of-type", + ) as HTMLButtonElement; + expect(installButton).not.toBeNull(); + await user.click(installButton); + + // Verify the SECOND skill was passed to the install mutation, not the first + await waitFor(() => { + expect(installMutateAsyncMock).toHaveBeenCalledTimes(1); + }); + const callArgs = installMutateAsyncMock.mock.calls[0][0]; + expect(callArgs.skill.repoOwner).toBe("owner-b"); + expect(callArgs.skill.repoName).toBe("repo-b"); + expect(callArgs.skill.name).toBe("Agent Browser B"); + }); +});