diff --git a/canvas/src/components/ApprovalBanner.tsx b/canvas/src/components/ApprovalBanner.tsx index e9baa776..9834945f 100644 --- a/canvas/src/components/ApprovalBanner.tsx +++ b/canvas/src/components/ApprovalBanner.tsx @@ -16,6 +16,9 @@ interface PendingApproval { export function ApprovalBanner() { const [approvals, setApprovals] = useState([]); + // Double-submit guard: id of the approval currently being decided. + // When non-null, both buttons are disabled and show ellipsis. + const [pendingApprovalId, setPendingApprovalId] = useState(null); // Single endpoint — no N+1 per-workspace polling const pollApprovals = useCallback(async () => { @@ -35,6 +38,8 @@ export function ApprovalBanner() { }, [pollApprovals]); const handleDecide = async (approval: PendingApproval, decision: "approved" | "denied") => { + if (pendingApprovalId !== null) return; // guard against double-submit + setPendingApprovalId(approval.id); try { await api.post(`/workspaces/${approval.workspace_id}/approvals/${approval.id}/decide`, { decision, @@ -44,6 +49,8 @@ export function ApprovalBanner() { setApprovals((prev) => prev.filter((a) => a.id !== approval.id)); } catch { showToast("Failed to submit decision", "error"); + } finally { + setPendingApprovalId(null); } }; @@ -51,49 +58,53 @@ export function ApprovalBanner() { return (
- {approvals.map((approval) => ( -
-
-
- -
-
-
{approval.workspace_name} needs approval
-
{approval.action}
- {approval.reason && ( -
{approval.reason}
- )} -
- - + {approvals.map((approval) => { + const isPending = pendingApprovalId === approval.id; + return ( +
+
+
+ +
+
+
{approval.workspace_name} needs approval
+
{approval.action}
+ {approval.reason && ( +
{approval.reason}
+ )} +
+ + +
-
- ))} + ); + })}
); } diff --git a/canvas/src/components/__tests__/ApprovalBanner.test.tsx b/canvas/src/components/__tests__/ApprovalBanner.test.tsx index f6fcfca4..98be964d 100644 --- a/canvas/src/components/__tests__/ApprovalBanner.test.tsx +++ b/canvas/src/components/__tests__/ApprovalBanner.test.tsx @@ -318,3 +318,138 @@ describe("ApprovalBanner — handles empty list from server", () => { expect(screen.queryByRole("alert")).toBeNull(); }); }); + +// ─── Double-submit guard ─────────────────────────────────────────────────────── + +describe("ApprovalBanner — double-submit guard", () => { + it("disables both buttons while POST is in flight", async () => { + const approval = pendingApproval("a1", "ws-1"); + _mockGet.mockResolvedValueOnce([approval] as unknown[]); + // Deferred resolver — POST hangs until we resolve it + let resolvePost: (v: unknown) => void; + _mockPost.mockImplementation( + () => new Promise((r) => { resolvePost = r as (v: unknown) => void; }), + ); + + render(); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + + const approveBtn = screen.getByRole("button", { name: /approve/i }); + const denyBtn = screen.getByRole("button", { name: /deny/i }); + + // Click triggers synchronous state update; flush with act + await act(async () => { + fireEvent.click(approveBtn); + // Flush microtasks so the deferred promise is picked up + await new Promise(r => setTimeout(r, 0)); + }); + + // After click, buttons are disabled + expect(approveBtn.getAttribute("disabled")).toBe(""); + expect(denyBtn.getAttribute("disabled")).toBe(""); + + // Unblock the POST + resolvePost!({}); + }); + + it("re-enables both buttons after a successful decision", async () => { + const approval = pendingApproval("a1", "ws-1"); + _mockGet.mockResolvedValueOnce([approval] as unknown[]); + _mockPost.mockResolvedValueOnce({} as unknown); + + render(); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + + fireEvent.click(screen.getByRole("button", { name: /approve/i })); + + // After card is removed, no buttons exist — guard is confirmed cleared + await waitFor(() => { + expect(screen.queryByRole("button", { name: /approve/i })).toBeNull(); + }); + }); + + it("re-enables both buttons after a failed decision", async () => { + const approval = pendingApproval("a1", "ws-1"); + _mockGet.mockResolvedValueOnce([approval] as unknown[]); + _mockPost.mockImplementation( + () => new Promise((_, reject) => reject(new Error("Network error"))), + ); + + render(); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + + const approveBtn = screen.getByRole("button", { name: /approve/i }); + const denyBtn = screen.getByRole("button", { name: /deny/i }); + + fireEvent.click(approveBtn); + + // Wait for error toast (POST failed) + await waitFor(() => { + expect(_mockToast).toHaveBeenCalledWith("Failed to submit decision", "error"); + }); + + // Buttons re-enabled so the user can retry + await waitFor(() => { + expect(approveBtn.getAttribute("disabled")).toBeNull(); + expect(denyBtn.getAttribute("disabled")).toBeNull(); + }); + }); + + it("shows ellipsis on clicked button while POST is in flight", async () => { + const approval = pendingApproval("a1", "ws-1"); + _mockGet.mockResolvedValueOnce([approval] as unknown[]); + let resolvePost: (v: unknown) => void; + _mockPost.mockImplementation( + () => new Promise((r) => { resolvePost = r as (v: unknown) => void; }), + ); + + render(); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + + const denyBtn = screen.getByRole("button", { name: /deny/i }); + expect(denyBtn.textContent).toBe("Deny"); + + // Click Deny — flush state update with act + await act(async () => { + fireEvent.click(denyBtn); + await new Promise(r => setTimeout(r, 0)); + }); + + // Both buttons show ellipsis while pending; verify via textContent + const buttons = screen.getAllByRole("button"); + const texts = buttons.map(b => b.textContent); + expect(texts).toContain("…"); + expect(screen.queryByRole("button", { name: /deny/i })).toBeNull(); + + resolvePost!({}); + }); + + it("second button click is ignored while first is in flight", async () => { + const approval = pendingApproval("a1", "ws-1"); + _mockGet.mockResolvedValueOnce([approval] as unknown[]); + let resolvePost: (v: unknown) => void; + _mockPost.mockImplementation( + () => new Promise((r) => { resolvePost = r as (v: unknown) => void; }), + ); + + render(); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + + // Click Approve — starts pending; flush state + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /approve/i })); + await new Promise(r => setTimeout(r, 0)); + }); + + // Both buttons show ellipsis while pending + const buttons = screen.getAllByRole("button"); + const texts = buttons.map(b => b.textContent); + expect(texts).toContain("…"); + + // Only ONE POST call should have been made despite two clicks + await act(async () => { await new Promise((r) => setTimeout(r, 20)); }); + expect(_mockPost).toHaveBeenCalledTimes(1); + + resolvePost!({}); + }); +});