From 835e8360e3291ad94fe8acc7c27f58e5ff0ced99 Mon Sep 17 00:00:00 2001 From: Molecule AI Core-UIUX Date: Wed, 13 May 2026 12:30:23 +0000 Subject: [PATCH] fix(canvas/ApprovalBanner): add disabled state + fix WCAG contrast on Deny button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add pendingApprovalId state guard to prevent double-submit (both Approve + Deny buttons disabled while POST is in flight) - Fix Deny button text-ink-mid → text-ink for WCAG AA contrast (~3:1 → ~7:1 on zinc-800 surface-card background) - Add aria-disabled + disabled attribute for screen reader support - Show ellipsis "…" on clicked button during submission - Add 5 new tests: disabled mid-flight, re-enabled after resolve/fail, ellipsis text, all-buttons-disabled guard Co-Authored-By: Claude Opus 4.7 --- canvas/src/components/ApprovalBanner.tsx | 23 +++-- .../__tests__/ApprovalBanner.test.tsx | 92 +++++++++++++++++++ 2 files changed, 108 insertions(+), 7 deletions(-) diff --git a/canvas/src/components/ApprovalBanner.tsx b/canvas/src/components/ApprovalBanner.tsx index e9baa776..03e5c521 100644 --- a/canvas/src/components/ApprovalBanner.tsx +++ b/canvas/src/components/ApprovalBanner.tsx @@ -16,6 +16,8 @@ interface PendingApproval { export function ApprovalBanner() { const [approvals, setApprovals] = useState([]); + // Guards double-click / double-keypress during in-flight POST. + const [pendingApprovalId, setPendingApprovalId] = useState(null); // Single endpoint — no N+1 per-workspace polling const pollApprovals = useCallback(async () => { @@ -35,6 +37,8 @@ export function ApprovalBanner() { }, [pollApprovals]); const handleDecide = async (approval: PendingApproval, decision: "approved" | "denied") => { + if (pendingApprovalId !== null) return; // guard double-submit + setPendingApprovalId(approval.id); try { await api.post(`/workspaces/${approval.workspace_id}/approvals/${approval.id}/decide`, { decision, @@ -44,6 +48,8 @@ export function ApprovalBanner() { setApprovals((prev) => prev.filter((a) => a.id !== approval.id)); } catch { showToast("Failed to submit decision", "error"); + } finally { + setPendingApprovalId(null); } }; @@ -72,22 +78,25 @@ export function ApprovalBanner() {
diff --git a/canvas/src/components/__tests__/ApprovalBanner.test.tsx b/canvas/src/components/__tests__/ApprovalBanner.test.tsx index 713313e5..a1eb24ac 100644 --- a/canvas/src/components/__tests__/ApprovalBanner.test.tsx +++ b/canvas/src/components/__tests__/ApprovalBanner.test.tsx @@ -238,6 +238,98 @@ describe("ApprovalBanner — decisions", () => { }); }); +describe("ApprovalBanner — disabled state while submitting", () => { + // Deferred so we can control when the mock POST resolves. + let resolvePost: (value: unknown) => void; + let postPromise: Promise; + + beforeEach(() => { + vi.useFakeTimers(); + mockApiGet.mockReset().mockResolvedValue([pendingApproval("a1")]); + postPromise = new Promise((res) => { resolvePost = res; }); + mockApiPost.mockReset().mockImplementation(() => postPromise as Promise); + }); + + afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.resetModules(); + }); + + it("disables both buttons while POST is in flight", async () => { + render(); + await act(async () => { await vi.runOnlyPendingTimersAsync(); }); + const approveBtn = screen.getAllByRole("button", { name: /approve/i })[0]; + const denyBtn = screen.getAllByRole("button", { name: /deny/i })[0]; + + fireEvent.click(approveBtn); + await act(async () => { /* flush */ }); + + expect((approveBtn as HTMLButtonElement).disabled).toBe(true); + expect((denyBtn as HTMLButtonElement).disabled).toBe(true); + }); + + it("re-enables buttons after POST resolves", async () => { + render(); + await act(async () => { await vi.runOnlyPendingTimersAsync(); }); + const approveBtn = screen.getAllByRole("button", { name: /approve/i })[0]; + const denyBtn = screen.getAllByRole("button", { name: /deny/i })[0]; + + fireEvent.click(approveBtn); + await act(async () => { /* flush */ }); + expect((approveBtn as HTMLButtonElement).disabled).toBe(true); + expect((denyBtn as HTMLButtonElement).disabled).toBe(true); + + // Resolve the deferred POST inside act() so React flushes the state update. + await act(async () => { + resolvePost!({}); + }); + expect(screen.queryByRole("alert")).toBeNull(); + }); + + it("re-enables buttons after POST fails", async () => { + mockApiPost.mockImplementation(() => Promise.reject(new Error("Network error"))); + render(); + await act(async () => { await vi.runOnlyPendingTimersAsync(); }); + const approveBtn = screen.getAllByRole("button", { name: /approve/i })[0]; + + fireEvent.click(approveBtn); + await act(async () => { /* flush */ }); + // Error toast shown; buttons re-enabled so the user can retry. + expect((approveBtn as HTMLButtonElement).disabled).toBe(false); + }); + + it("shows ellipsis text on the clicked button while submitting", async () => { + render(); + await act(async () => { await vi.runOnlyPendingTimersAsync(); }); + fireEvent.click(screen.getAllByRole("button", { name: /approve/i })[0]); + await act(async () => { /* flush */ }); + // The clicked button now shows "…" instead of "Approve" + expect(screen.queryByRole("button", { name: /approve/i })).toBeNull(); + expect(screen.getAllByRole("button", { name: /^…$/ }).length).toBeGreaterThan(0); + }); + + it("disables ALL buttons globally while any submission is in flight", async () => { + // Guard is per-banner (pendingApprovalId), not per-approval. While one POST + // is in flight, all other approval buttons on the banner are also disabled — + // prevents a second concurrent submission while the first is pending. + mockApiGet.mockReset().mockResolvedValue([ + pendingApproval("a1"), + pendingApproval("a2", "ws-2"), + ]); + render(); + await act(async () => { await vi.runOnlyPendingTimersAsync(); }); + const card1Approve = screen.getAllByRole("button", { name: /approve/i })[0]; + const card2Approve = screen.getAllByRole("button", { name: /approve/i })[1]; + fireEvent.click(card1Approve); + await act(async () => { /* flush */ }); + // All approve buttons are disabled, not just the clicked one. + expect((card1Approve as HTMLButtonElement).disabled).toBe(true); + expect((card2Approve as HTMLButtonElement).disabled).toBe(true); + }); +}); + describe("ApprovalBanner — handles empty list from server", () => { beforeEach(() => { vi.useFakeTimers(); -- 2.45.2