From 29e87487a1d2739fbd56e581840e04fcd84fbe42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B6=82=E7=91=9C?= <861506831@qq.com> Date: Thu, 23 Apr 2026 10:25:01 +0800 Subject: [PATCH] fix(skills): prevent duplicate imports when import button is double-clicked (#2211) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2139 Two related defects let the installed-skills count balloon when users tap the import confirm button multiple times — either deliberately or because the button is still clickable while a slow import is in flight: - The confirm button only disabled itself while `selected.size === 0`, so it stayed clickable during a pending mutation. Each extra click triggered another `importFromApps` mutation. - `useImportSkillsFromApps` appended the server response to the installed cache without deduping, so re-firing the mutation stacked the same skills into the list again. Disable the confirm (and cancel) buttons while the mutation is pending — matching the `isRestoring` / `isDeleting` pattern already used by `RestoreSkillsDialog` — and merge success payloads by `InstalledSkill.id` so repeated results overwrite rather than accumulate. The merge is extracted as a pure `mergeImportedSkills` reducer to make the behaviour unit-testable and to short-circuit on an empty payload, returning the existing reference so React Query does not notify subscribers about a no-op cache update. --- src/components/skills/UnifiedSkillsPanel.tsx | 10 +++- src/hooks/useSkills.helpers.ts | 19 ++++++ src/hooks/useSkills.ts | 6 +- tests/hooks/useImportSkillsFromApps.test.tsx | 61 ++++++++++++++++++++ 4 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 src/hooks/useSkills.helpers.ts create mode 100644 tests/hooks/useImportSkillsFromApps.test.tsx diff --git a/src/components/skills/UnifiedSkillsPanel.tsx b/src/components/skills/UnifiedSkillsPanel.tsx index 77199ef17..215e3cbca 100644 --- a/src/components/skills/UnifiedSkillsPanel.tsx +++ b/src/components/skills/UnifiedSkillsPanel.tsx @@ -454,6 +454,7 @@ const UnifiedSkillsPanel = React.forwardRef< {importDialogOpen && unmanagedSkills && ( setImportDialogOpen(false)} /> @@ -600,6 +601,7 @@ interface ImportSkillsDialogProps { foundIn: string[]; path: string; }>; + isImporting: boolean; onImport: (imports: ImportSkillSelection[]) => void; onClose: () => void; } @@ -724,6 +726,7 @@ const RestoreSkillsDialog: React.FC = ({ const ImportSkillsDialog: React.FC = ({ skills, + isImporting, onImport, onClose, }) => { @@ -846,10 +849,13 @@ const ImportSkillsDialog: React.FC = ({
- -
diff --git a/src/hooks/useSkills.helpers.ts b/src/hooks/useSkills.helpers.ts new file mode 100644 index 000000000..01c7e011c --- /dev/null +++ b/src/hooks/useSkills.helpers.ts @@ -0,0 +1,19 @@ +import type { InstalledSkill } from "@/lib/api/skills"; + +/** + * 合并本次导入结果到已安装缓存，按 id 去重。 + * + * 同一技能重复出现时以新记录为准，避免 mutation 被重复触发时 + * 在 installed 列表中看到重复条目。imported 为空时返回原引用， + * 让 React Query 跳过无谓的订阅者通知。 + */ +export function mergeImportedSkills( + existing: InstalledSkill[] | undefined, + imported: InstalledSkill[], +): InstalledSkill[] { + if (!existing) return imported; + if (imported.length === 0) return existing; + const importedIds = new Set(imported.map((s) => s.id)); + const preserved = existing.filter((s) => !importedIds.has(s.id)); + return [...preserved, ...imported]; +} diff --git a/src/hooks/useSkills.ts b/src/hooks/useSkills.ts index ea641f099..095f14328 100644 --- a/src/hooks/useSkills.ts +++ b/src/hooks/useSkills.ts @@ -14,6 +14,7 @@ import { type SkillsShSearchResult, } from "@/lib/api/skills"; import type { AppId } from "@/lib/api/types"; +import { mergeImportedSkills } from "@/hooks/useSkills.helpers"; /** * 查询所有已安装的 Skills @@ -208,10 +209,7 @@ export function useImportSkillsFromApps() { // 直接更新 installed 缓存 queryClient.setQueryData( ["skills", "installed"], - (oldData) => { - if (!oldData) return importedSkills; - return [...oldData, ...importedSkills]; - }, + (oldData) => mergeImportedSkills(oldData, importedSkills), ); // 刷新 unmanaged 列表（已被导入的应该移除） queryClient.invalidateQueries({ queryKey: ["skills", "unmanaged"] }); diff --git a/tests/hooks/useImportSkillsFromApps.test.tsx b/tests/hooks/useImportSkillsFromApps.test.tsx new file mode 100644 index 000000000..27eea0875 --- /dev/null +++ b/tests/hooks/useImportSkillsFromApps.test.tsx @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { mergeImportedSkills } from "@/hooks/useSkills.helpers"; +import type { InstalledSkill } from "@/lib/api/skills"; + +function makeSkill(overrides: Partial = {}): InstalledSkill { + return { + id: "skill-a", + name: "Skill A", + directory: "skill-a", + apps: { + claude: true, + codex: false, + gemini: false, + opencode: false, + openclaw: false, + }, + installedAt: 0, + updatedAt: 0, + ...overrides, + }; +} + +// Regression coverage for issue #2139: when a user double-clicks the import +// button (or the mutation otherwise fires twice with the same payload), the +// installed cache must not accumulate duplicate entries for the same skill. +describe("mergeImportedSkills", () => { + it("returns the imported list as-is when no cache exists yet", () => { + const imported = [makeSkill()]; + expect(mergeImportedSkills(undefined, imported)).toEqual(imported); + }); + + it("dedupes by id when the same skill is imported twice in a row", () => { + const existing = [makeSkill()]; + const secondImport = [makeSkill()]; + const merged = mergeImportedSkills(existing, secondImport); + expect(merged).toHaveLength(1); + expect(merged[0]).toBe(secondImport[0]); + }); + + it("replaces stale cache entries with fresh imports for the same id", () => { + const stale = [makeSkill({ name: "Stale Name" })]; + const fresh = [makeSkill({ name: "Fresh Name" })]; + const merged = mergeImportedSkills(stale, fresh); + expect(merged).toHaveLength(1); + expect(merged[0].name).toBe("Fresh Name"); + }); + + it("returns the existing reference unchanged when the imported list is empty", () => { + const existing = [makeSkill()]; + expect(mergeImportedSkills(existing, [])).toBe(existing); + }); + + it("appends newly imported skills without dropping existing unrelated ones", () => { + const existing = [makeSkill({ id: "skill-a", directory: "skill-a" })]; + const imported = [ + makeSkill({ id: "skill-b", directory: "skill-b", name: "Skill B" }), + ]; + const merged = mergeImportedSkills(existing, imported); + expect(merged.map((s) => s.id).sort()).toEqual(["skill-a", "skill-b"]); + }); +});