mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F084] scope per-control disable to the in-flight mutation, not all
FeatureFlagsCard disabled every switch while any one flag toggle was pending, and PlaybookReviewQueue disabled every row's Approve while any one approve was pending — so the operator couldn't act on an independent control during a slow round-trip. Gate the disable on the in-flight mutation's variables (matching key / id) so only the control being mutated locks; the others stay usable. The same-flag double-tap protection is preserved.
This commit is contained in:
@@ -0,0 +1,96 @@
|
|||||||
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
|
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
|
||||||
|
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||||
|
import type { ReactNode } from "react";
|
||||||
|
import type { Playbook } from "@/lib/api/playbooks";
|
||||||
|
|
||||||
|
const { resolveApproveRef } = vi.hoisted(() => ({
|
||||||
|
resolveApproveRef: { current: null as null | ((v: unknown) => void) },
|
||||||
|
}));
|
||||||
|
|
||||||
|
const { listDrafts, approve, reject } = vi.hoisted(() => ({
|
||||||
|
listDrafts: vi.fn(
|
||||||
|
async () =>
|
||||||
|
[
|
||||||
|
{
|
||||||
|
id: "pb-1",
|
||||||
|
title: "Recover a stuck claim lock",
|
||||||
|
slug: "recover-claim-lock",
|
||||||
|
problem: "an agent's claim TOCTOU wedges the task",
|
||||||
|
procedure: "1. ...",
|
||||||
|
tags: ["backend"],
|
||||||
|
team: "backend",
|
||||||
|
scope: "cell",
|
||||||
|
status: "draft",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
id: "pb-2",
|
||||||
|
title: "Rebase a behind-base branch",
|
||||||
|
slug: "rebase-behind-base",
|
||||||
|
problem: "the dev's branch fell behind master",
|
||||||
|
procedure: "1. ...",
|
||||||
|
tags: ["git"],
|
||||||
|
team: "backend",
|
||||||
|
scope: "cell",
|
||||||
|
status: "draft",
|
||||||
|
},
|
||||||
|
] as Playbook[],
|
||||||
|
),
|
||||||
|
// Deferred so the test can freeze the approve mid-flight.
|
||||||
|
approve: vi.fn(
|
||||||
|
() =>
|
||||||
|
new Promise((r) => {
|
||||||
|
resolveApproveRef.current = r as (v: unknown) => void;
|
||||||
|
}),
|
||||||
|
),
|
||||||
|
reject: vi.fn(async () => ({})),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@/lib/api", () => ({ playbooksApi: { listDrafts, approve, reject } }));
|
||||||
|
|
||||||
|
import { PlaybookReviewQueue } from "../playbook-review-queue";
|
||||||
|
|
||||||
|
function withQueryClient(ui: ReactNode) {
|
||||||
|
const client = new QueryClient({
|
||||||
|
defaultOptions: { queries: { retry: false }, mutations: { retry: false } },
|
||||||
|
});
|
||||||
|
return <QueryClientProvider client={client}>{ui}</QueryClientProvider>;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("PlaybookReviewQueue — per-row disable during an approve (F084)", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
listDrafts.mockClear();
|
||||||
|
approve.mockClear();
|
||||||
|
reject.mockClear();
|
||||||
|
resolveApproveRef.current = null;
|
||||||
|
});
|
||||||
|
afterEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("disables only the playbook being approved, not every row's Approve", async () => {
|
||||||
|
render(withQueryClient(<PlaybookReviewQueue />));
|
||||||
|
|
||||||
|
const approveButtons = await screen.findAllByRole("button", {
|
||||||
|
name: "Approve",
|
||||||
|
});
|
||||||
|
expect(approveButtons).toHaveLength(2);
|
||||||
|
expect(approveButtons[0]).not.toBeDisabled();
|
||||||
|
expect(approveButtons[1]).not.toBeDisabled();
|
||||||
|
|
||||||
|
// Approve the first playbook — the mutation stays pending (deferred fn).
|
||||||
|
fireEvent.click(approveButtons[0]);
|
||||||
|
await waitFor(() => expect(approve).toHaveBeenCalledWith("pb-1"));
|
||||||
|
|
||||||
|
// Row 1's Approve locks while its approve is in flight; row 2's Approve
|
||||||
|
// stays usable so the reviewer can act on an independent playbook at the
|
||||||
|
// same time. Before the fix every row shared `disabled={approveMutation.isPending}`.
|
||||||
|
await waitFor(() => expect(approveButtons[0]).toBeDisabled());
|
||||||
|
expect(approveButtons[1]).not.toBeDisabled();
|
||||||
|
|
||||||
|
// Mutation resolves → row 1's Approve unlocks again.
|
||||||
|
resolveApproveRef.current?.(undefined);
|
||||||
|
await waitFor(() => expect(approveButtons[0]).not.toBeDisabled());
|
||||||
|
expect(approveButtons[1]).not.toBeDisabled();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -132,7 +132,10 @@ export function PlaybookReviewQueue({ className }: { className?: string }) {
|
|||||||
<Button
|
<Button
|
||||||
size="sm"
|
size="sm"
|
||||||
className="bg-green-600 hover:bg-green-700"
|
className="bg-green-600 hover:bg-green-700"
|
||||||
disabled={approveMutation.isPending}
|
disabled={
|
||||||
|
approveMutation.isPending &&
|
||||||
|
approveMutation.variables === pb.id
|
||||||
|
}
|
||||||
onClick={() => approveMutation.mutate(pb.id)}
|
onClick={() => approveMutation.mutate(pb.id)}
|
||||||
>
|
>
|
||||||
<CheckCircle2 className="mr-1 h-4 w-4" />
|
<CheckCircle2 className="mr-1 h-4 w-4" />
|
||||||
|
|||||||
@@ -0,0 +1,81 @@
|
|||||||
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
|
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
|
||||||
|
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||||
|
import type { ReactNode } from "react";
|
||||||
|
|
||||||
|
// Deferred mutationFn: the test holds `resolveSet` so it can freeze the toggle
|
||||||
|
// mutation mid-flight and observe the per-control disabled state, then
|
||||||
|
// release it. This exercises the REAL useMutation isPending/variables state
|
||||||
|
// rather than a stubbed hook. `vi.hoisted` keeps the mock fns initialized
|
||||||
|
// before the hoisted vi.mock factory runs.
|
||||||
|
// Hold the deferred mutation resolver so the test can freeze the toggle
|
||||||
|
// mid-flight and observe the per-control disabled state, then release it.
|
||||||
|
const { resolveSetRef } = vi.hoisted(() => ({
|
||||||
|
resolveSetRef: { current: null as null | ((v: unknown) => void) },
|
||||||
|
}));
|
||||||
|
|
||||||
|
const { setFeatureFlag, getFeatureFlags } = vi.hoisted(() => ({
|
||||||
|
setFeatureFlag: vi.fn(
|
||||||
|
() =>
|
||||||
|
new Promise((r) => {
|
||||||
|
resolveSetRef.current = r as (v: unknown) => void;
|
||||||
|
}),
|
||||||
|
),
|
||||||
|
getFeatureFlags: vi.fn(async () => ({
|
||||||
|
flags: [
|
||||||
|
{ key: "alpha", label: "Alpha", enabled: true },
|
||||||
|
{ key: "beta", label: "Beta", enabled: false },
|
||||||
|
],
|
||||||
|
note: "Changes take effect on the next backend restart.",
|
||||||
|
})),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@/lib/api", () => ({
|
||||||
|
settingsApi: { getFeatureFlags, setFeatureFlag },
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { FeatureFlagsCard } from "../feature-flags-card";
|
||||||
|
|
||||||
|
function withQueryClient(ui: ReactNode) {
|
||||||
|
const client = new QueryClient({
|
||||||
|
defaultOptions: { queries: { retry: false }, mutations: { retry: false } },
|
||||||
|
});
|
||||||
|
return <QueryClientProvider client={client}>{ui}</QueryClientProvider>;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("FeatureFlagsCard — per-control disable during a toggle (F084)", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
setFeatureFlag.mockClear();
|
||||||
|
getFeatureFlags.mockClear();
|
||||||
|
resolveSetRef.current = null;
|
||||||
|
});
|
||||||
|
afterEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("disables only the flag being toggled, not every flag's switch", async () => {
|
||||||
|
render(withQueryClient(<FeatureFlagsCard />));
|
||||||
|
|
||||||
|
const alpha = await screen.findByRole("switch", { name: "Alpha" });
|
||||||
|
const beta = await screen.findByRole("switch", { name: "Beta" });
|
||||||
|
expect(alpha).not.toBeDisabled();
|
||||||
|
expect(beta).not.toBeDisabled();
|
||||||
|
|
||||||
|
// Toggle Alpha off — the mutation stays pending (deferred mutationFn).
|
||||||
|
fireEvent.click(alpha);
|
||||||
|
await waitFor(() =>
|
||||||
|
expect(setFeatureFlag).toHaveBeenCalledWith("alpha", false),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Alpha's switch locks while its toggle is in flight; Beta stays usable so
|
||||||
|
// the operator can flip an independent flag at the same time. Before the
|
||||||
|
// fix every switch shared `disabled={toggleMutation.isPending}`.
|
||||||
|
await waitFor(() => expect(alpha).toBeDisabled());
|
||||||
|
expect(beta).not.toBeDisabled();
|
||||||
|
|
||||||
|
// Mutation resolves → Alpha unlocks again.
|
||||||
|
resolveSetRef.current?.(undefined);
|
||||||
|
await waitFor(() => expect(alpha).not.toBeDisabled());
|
||||||
|
expect(beta).not.toBeDisabled();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -111,7 +111,10 @@ export function FeatureFlagsCard() {
|
|||||||
<Switch
|
<Switch
|
||||||
id={`flag-${flag.key}`}
|
id={`flag-${flag.key}`}
|
||||||
checked={flag.enabled}
|
checked={flag.enabled}
|
||||||
disabled={toggleMutation.isPending}
|
disabled={
|
||||||
|
toggleMutation.isPending &&
|
||||||
|
toggleMutation.variables?.key === flag.key
|
||||||
|
}
|
||||||
onCheckedChange={(checked) =>
|
onCheckedChange={(checked) =>
|
||||||
toggleMutation.mutate({ key: flag.key, enabled: checked })
|
toggleMutation.mutate({ key: flag.key, enabled: checked })
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user