app: skills review fixes — selection token, toggle race guard, save gate

Selection uses border-primary (brand stays reserved for pipeline
identity); review-pipeline toggles are disabled per-skill while a
command is in flight so a stale response can't clobber the final
click; handleSave re-asserts editability locally; failure rollback
re-reads before surfacing the error so the message survives. Store
tests pin all three behaviors.

Author: LifeLex

app/src/components/SkillsView.tsx

@@ -91,6 +91,7 @@ interface SkillRowProps {
   readonly onSelect: () => void;
   readonly inline: "toggle" | "badge";
   readonly onTogglePipeline: (enabled: boolean) => void;
+  readonly togglePending: boolean;
 }
 
 function SkillRow({
@@ -99,13 +100,14 @@ function SkillRow({
   onSelect,
   inline,
   onTogglePipeline,
+  togglePending,
 }: SkillRowProps) {
   return (
     <div
       className={cn(
         "flex items-center gap-2 border-l-2 pr-2 transition-colors",
         selected
-          ? "border-brand bg-muted"
+          ? "border-primary bg-muted"
           : "border-transparent hover:bg-muted/50",
       )}
     >
@@ -128,6 +130,7 @@ function SkillRow({
           size="sm"
           checked={skill.in_review_pipeline}
           onCheckedChange={onTogglePipeline}
+          disabled={togglePending}
           aria-label={`Toggle ${skill.name} in review pipeline`}
         />
       ) : (
@@ -160,6 +163,7 @@ export function SkillsView() {
   const deleteSkill = useAppStore((s) => s.deleteSkill);
   const syncSkills = useAppStore((s) => s.syncSkills);
   const setSkillReviewPipeline = useAppStore((s) => s.setSkillReviewPipeline);
+  const pendingSkillToggles = useAppStore((s) => s.pendingSkillToggles);
 
   const [selection, setSelection] = useState<Selection>({ kind: "none" });
   const [search, setSearch] = useState("");
@@ -232,7 +236,11 @@ export function SkillsView() {
         setFeedback({ kind: "error", message: "A skill name is required." });
         return;
       }
-    } else if (selection.kind === "skill" && selectedSkill !== null) {
+    } else if (
+      selection.kind === "skill" &&
+      selectedSkill !== null &&
+      isEditable(selectedSkill.source)
+    ) {
       name = selectedSkill.name;
     } else {
       return;
@@ -418,6 +426,9 @@ export function SkillsView() {
                             onTogglePipeline={(enabled) => {
                               handleToggle(skill, enabled);
                             }}
+                            togglePending={pendingSkillToggles.includes(
+                              skill.name,
+                            )}
                           />
                         ))}
                       </div>
@@ -474,6 +485,7 @@ export function SkillsView() {
               onToggle={(enabled) => {
                 handleToggle(selectedSkill, enabled);
               }}
+              togglePending={pendingSkillToggles.includes(selectedSkill.name)}
               onEdit={() => {
                 handleEdit(selectedSkill);
               }}
@@ -530,9 +542,10 @@ export function SkillsView() {
 interface UseInReviewCardProps {
   readonly enabled: boolean;
   readonly onToggle: (enabled: boolean) => void;
+  readonly pending: boolean;
 }
 
-function UseInReviewCard({ enabled, onToggle }: UseInReviewCardProps) {
+function UseInReviewCard({ enabled, onToggle, pending }: UseInReviewCardProps) {
   return (
     <div className="rounded-lg border border-border bg-card p-4">
       <div className="flex items-start justify-between gap-4">
@@ -551,6 +564,7 @@ function UseInReviewCard({ enabled, onToggle }: UseInReviewCardProps) {
         <Switch
           checked={enabled}
           onCheckedChange={onToggle}
+          disabled={pending}
           aria-label="Use this skill in the review pipeline"
         />
       </div>
@@ -562,6 +576,7 @@ function UseInReviewCard({ enabled, onToggle }: UseInReviewCardProps) {
 interface SkillDetailProps {
   readonly skill: Skill;
   readonly onToggle: (enabled: boolean) => void;
+  readonly togglePending: boolean;
   readonly onEdit: () => void;
   readonly onDelete: () => void;
   readonly deleting: boolean;
@@ -570,6 +585,7 @@ interface SkillDetailProps {
 function SkillDetail({
   skill,
   onToggle,
+  togglePending,
   onEdit,
   onDelete,
   deleting,
@@ -601,7 +617,11 @@ function SkillDetail({
         </div>
       )}
 
-      <UseInReviewCard enabled={skill.in_review_pipeline} onToggle={onToggle} />
+      <UseInReviewCard
+        enabled={skill.in_review_pipeline}
+        onToggle={onToggle}
+        pending={togglePending}
+      />
 
       {skill.description !== "" && (
         <p className="text-sm text-muted-foreground">{skill.description}</p>

app/src/store.test.ts

@@ -1,6 +1,7 @@
 import { describe, it, expect, beforeEach, vi } from "vitest";
 import type { PendingPermission } from "./store";
 import type { Review } from "./bindings/Review";
+import type { Skill } from "./bindings/Skill";
 import type { CiCheck } from "./bindings/CiCheck";
 import type { SubmitReviewResult } from "./bindings/SubmitReviewResult";
 import type { EvidenceSummary } from "./bindings/EvidenceSummary";
@@ -843,3 +844,74 @@ describe("applyRestackProgress", () => {
     expect(progress["root-b"]?.status).toBe("done");
   });
 });
+
+describe("setSkillReviewPipeline", () => {
+  const storeSkill = {
+    name: "house-style",
+    description: "d",
+    tags: [],
+    body: "b",
+    path: "/tmp/store/house-style",
+    source: { kind: "CockpitStore" },
+    in_review_pipeline: false,
+  } satisfies Skill;
+
+  it("flips optimistically, then reconciles with the returned list", async () => {
+    useAppStore.setState({ skills: [storeSkill] });
+    const returned = [{ ...storeSkill, in_review_pipeline: true }];
+    let observedDuringFlight: boolean | undefined;
+    mockInvoke("set_skill_review_pipeline", () => {
+      observedDuringFlight =
+        useAppStore.getState().skills[0]?.in_review_pipeline;
+      return returned;
+    });
+
+    await useAppStore
+      .getState()
+      .setSkillReviewPipeline("house-style", true);
+
+    expect(observedDuringFlight).toBe(true);
+    expect(useAppStore.getState().skills).toEqual(returned);
+    expect(useAppStore.getState().pendingSkillToggles).toEqual([]);
+  });
+
+  it("rolls back via a re-read when the command fails", async () => {
+    useAppStore.setState({ skills: [storeSkill] });
+    mockInvokeReject("set_skill_review_pipeline", "config write failed");
+    mockInvoke("list_skills", () => [storeSkill]);
+
+    await useAppStore
+      .getState()
+      .setSkillReviewPipeline("house-style", true);
+
+    expect(useAppStore.getState().skills[0]?.in_review_pipeline).toBe(false);
+    expect(useAppStore.getState().error).toContain("config write failed");
+    expect(useAppStore.getState().pendingSkillToggles).toEqual([]);
+  });
+
+  it("ignores a re-entrant toggle while one is in flight", async () => {
+    useAppStore.setState({ skills: [storeSkill] });
+    let release: (() => void) | undefined;
+    const gate = new Promise<void>((resolve) => {
+      release = resolve;
+    });
+    mockInvoke("set_skill_review_pipeline", async () => {
+      await gate;
+      return [{ ...storeSkill, in_review_pipeline: true }];
+    });
+
+    const first = useAppStore
+      .getState()
+      .setSkillReviewPipeline("house-style", true);
+    // Second click while the first is pending must be dropped, not queued.
+    const second = useAppStore
+      .getState()
+      .setSkillReviewPipeline("house-style", false);
+    release?.();
+    await Promise.all([first, second]);
+
+    expect(callsFor("set_skill_review_pipeline")).toHaveLength(1);
+    expect(useAppStore.getState().skills[0]?.in_review_pipeline).toBe(true);
+    expect(useAppStore.getState().pendingSkillToggles).toEqual([]);
+  });
+});

app/src/store.ts

@@ -415,13 +415,21 @@ interface AppStore {
   /** Sync skills from the configured GitHub source. */
   syncSkills: () => Promise<SyncReport | null>;
 
+  /**
+   * Skill names with a review-pipeline toggle currently in flight. The UI
+   * disables those switches so a rapid double-toggle can never race two
+   * commands whose stale response would clobber the user's final click.
+   */
+  readonly pendingSkillToggles: readonly string[];
+
   /**
    * Toggle a skill's membership in the review pipeline (explicit user action).
    *
    * Flips `in_review_pipeline` optimistically for instant feedback, then
    * reconciles `skills` with the refreshed list the command returns. On failure
    * it surfaces the error and re-reads the authoritative list so the optimistic
-   * flip never sticks.
+   * flip never sticks. Re-entrant toggles for a skill already in flight are
+   * ignored (the switch is disabled while pending).
    */
   setSkillReviewPipeline: (name: string, enabled: boolean) => Promise<void>;
 
@@ -535,6 +543,7 @@ export const useAppStore = create<AppStore>((set, get) => ({
   kickoffResult: null,
   skills: [],
   skillsLoading: false,
+  pendingSkillToggles: [],
   authoredPrs: [],
   reviewRequests: [],
   prFetchLoading: false,
@@ -1201,8 +1210,10 @@ export const useAppStore = create<AppStore>((set, get) => ({
   },
 
   setSkillReviewPipeline: async (name: string, enabled: boolean) => {
+    if (get().pendingSkillToggles.includes(name)) return;
     // Optimistic flip so the toggle feels instant; reconciled below.
     set({
+      pendingSkillToggles: [...get().pendingSkillToggles, name],
       skills: get().skills.map((s) =>
         s.name === name ? { ...s, in_review_pipeline: enabled } : s,
       ),
@@ -1215,9 +1226,17 @@ export const useAppStore = create<AppStore>((set, get) => ({
       });
       set({ skills });
     } catch (e: unknown) {
-      set({ error: String(e) });
-      // Re-read the authoritative list so the optimistic flip does not stick.
+      // Re-read the authoritative list so the optimistic flip does not stick,
+      // THEN surface the error — listSkills clears `error` on entry, so the
+      // opposite order would wipe the message before anyone saw it.
       await get().listSkills();
+      set({ error: String(e) });
+    } finally {
+      set({
+        pendingSkillToggles: get().pendingSkillToggles.filter(
+          (n) => n !== name,
+        ),
+      });
     }
   },