fix(skills): prevent duplicate imports when import button is double-clicked (#2211)

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.
This commit is contained in:
涂瑜
2026-04-23 10:25:01 +08:00
committed by GitHub
parent 7bfe2609db
commit 29e87487a1
4 changed files with 90 additions and 6 deletions
@@ -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> = {}): 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"]);
});
});