DecisionBlock UX hardening take 2: keep error surfacing alive while adding in-flight disable + 409 refetch (supersedes PR #2452)

changelog.d/tsk-nxmiby-decisionblock-ux.md

@@ -0,0 +1,7 @@
+### Fixed
+
+- Disable option buttons and Submit button while a POST is in flight, preventing duplicate submissions that cause 409 errors
+- Clear answerError at the start of each new submission attempt
+- Distinguish refresh-failure from submit-failure: when POST succeeds but follow-up GET fails, do not show "Failed to answer"
+- On 409 (someone else answered first), refetch the decision so the block flips to its answered state
+- Reset answer and answerError state when block.decision_id changes

changelog.d/tsk-r6qnrv-decisionblock-error-prop.md

@@ -0,0 +1,4 @@
+### Fixed
+
+- Restore error propagation from `answerDecision` so non-409 server failures (500, network errors, 4xx) surface the server-provided reason in the alert region instead of being swallowed
+- Surface a fallback message when the post-409 refetch itself fails, rather than leaving the block pending with no feedback

desktop/src/apps/MessagesApp.tsx

@@ -500,6 +500,7 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
   const [error, setError] = useState<string | null>(null);
   const [answer, setAnswer] = useState("");
   const [answerError, setAnswerError] = useState<string | null>(null);
+  const [submitting, setSubmitting] = useState(false);
 
   useEffect(() => {
     let cancelled = false;
@@ -524,12 +525,19 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
     return () => { cancelled = true; };
   }, [block.decision_id]);
 
+  useEffect(() => {
+    setAnswer("");
+    setAnswerError(null);
+  }, [block.decision_id]);
+
   async function answerDecision(
     value: string | string[],
     otherValue?: string,
     note?: string
   ) {
+    setAnswerError(null);
     if (!decision || decision.status !== "pending") return;
+    setSubmitting(true);
     const body: Record<string, unknown> = { value };
     if (otherValue !== undefined) body.other_value = otherValue;
     if (note !== undefined) body.note = note;
@@ -543,6 +551,26 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
       if (!res.ok) {
         const data = await res.json().catch(() => ({}));
         const detail = data?.error ?? data?.detail;
+        // 409 = someone else answered first; refetch so the block flips to answered
+        if (res.status === 409) {
+          // The refetch itself can fail (network reject, invalid JSON) --
+          // fall through to the conflict fallback rather than surfacing a
+          // generic "Failed to answer" for an answer that someone else won.
+          try {
+            const updatedRes = await fetch(`/api/decisions/${decision.id}`);
+            if (updatedRes.ok) {
+              const updated = await updatedRes.json();
+              setDecision(updated as DecisionData);
+              return;
+            }
+          } catch {
+            // fall through
+          }
+          setAnswerError(
+            "This decision was already answered -- refresh to see the outcome",
+          );
+          return;
+        }
         throw new Error(
           typeof detail === "string" ? detail : "Could not record answer.",
         );
@@ -552,10 +580,17 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
       if (updatedRes.ok) {
         const updated = await updatedRes.json();
         setDecision(updated as DecisionData);
+      } else {
+        // Refresh failed: answer was recorded, don't show "Failed to answer"
+        // (the SSE broker path will also correct it)
+        setSubmitting(false);
+        setAnswerError(null);
       }
     } catch (e) {
       console.error("Failed to answer decision:", e);
       throw e;
+    } finally {
+      setSubmitting(false);
     }
   }
 
@@ -609,12 +644,12 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
                 key={opt.value}
                 type="button"
                 onClick={() => {
-                  if (!isOpen) return;
+                  if (!isOpen || submitting) return;
                   answerDecision(opt.value).catch((e) =>
                     setAnswerError(`Failed to answer: ${e.message}`)
                   );
                 }}
-                disabled={!isOpen}
+                disabled={!isOpen || submitting}
                 className={[
                   "flex w-full flex-col gap-0.5 rounded-lg border px-3 py-1.5 text-left transition-colors",
                   "disabled:cursor-not-allowed disabled:opacity-60",
@@ -661,7 +696,7 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
               onKeyDown={(e) => {
                 if (e.key === "Enter" && !e.shiftKey) {
                   e.preventDefault();
-                  if (!isOpen) return;
+                  if (!isOpen || submitting) return;
                   const trimmed = e.currentTarget.value.trim();
                   if (trimmed) {
                     answerDecision(trimmed).catch((e) =>
@@ -673,26 +708,30 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React
             />
             <button
               onClick={() => {
-                if (!isOpen) return;
+                if (!isOpen || submitting) return;
                 const trimmed = answer.trim();
                 if (trimmed) {
                   answerDecision(trimmed).catch((e) =>
                     setAnswerError(`Failed to answer: ${e.message}`)
                   );
                 }
               }}
-              disabled={!answer || answer.trim() === ""}
+              disabled={!answer || answer.trim() === "" || submitting}
               className="flex items-center gap-1.5 rounded-lg border px-3 py-1.5 text-[12px] text-shell-text hover:text-shell-text-hover hover:bg-shell-surface-focus focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent/40"
               aria-label="Submit answer"
             >
               <ChevronRight size={12} aria-hidden="true" /> Submit
             </button>
           </div>
-          {answerError && (
-            <div className="mt-2 text-[12px] text-red-400" role="alert">
-              {answerError}
-            </div>
-          )}
+        </div>
+      )}
+
+      {/* submission errors: one shared alert region for option and
+          free-text answers alike (option errors were invisible when this
+          lived inside the free_text branch) */}
+      {answerError && (
+        <div className="mt-2 px-3 text-[12px] text-red-400" role="alert">
+          {answerError}
         </div>
       )}
 

desktop/src/components/__tests__/DecisionBlock.test.tsx

@@ -453,4 +453,228 @@ describe("DecisionBlock", () => {
     // First answer is retained in the UI - verify answer is displayed
     expect(container2.textContent).toContain("answered: React");
   });
+
+  it("double-click while in-flight produces exactly ONE POST", async () => {
+    // --- Open decision with single option ---
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      json: async () => ({
+        ...baseDecision,
+        question: "Pick a framework",
+        type: "single_select",
+        options: [
+          { label: "React", value: "react" },
+          { label: "Vue", value: "vue" },
+        ],
+        context: "ui library",
+        status: "pending",
+        answer: null,
+        created_at: 1700000000,
+      }),
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    const block: DecisionContentBlock = {
+      kind: "decision",
+      decision_id: "dec-1",
+    };
+    const { container } = render(<DecisionBlock block={block} />);
+
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    // Click first option (React) - first answer
+    const enabledBtns = container.querySelectorAll('button:not([disabled])');
+    fireEvent.click(enabledBtns[0]);
+
+    // Immediately double-click the same button while first POST is in-flight.
+    // The submitting state should prevent a second POST.
+    fireEvent.click(enabledBtns[0]);
+
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    // Verify only one POST to /answer was made (double-click is prevented
+    // by the submitting state disabling buttons)
+    const answerCalls = fetchMock.mock.calls.filter(
+      ([url]) => url === "/api/decisions/dec-1/answer"
+    );
+    expect(answerCalls.length).toBe(1);
+  });
+
+  it("double-click while in-flight produces exactly ONE POST (free_text via Enter key)", async () => {
+    // --- Open free_text decision ---
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      json: async () => ({
+        ...baseDecision,
+        id: "dec-8",
+        question: "Any notes?",
+        type: "free_text",
+        options: [],
+        status: "pending",
+        answer: null,
+        created_at: 1700000000,
+      }),
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    const block: DecisionContentBlock = {
+      kind: "decision",
+      decision_id: "dec-8",
+    };
+    const { container } = render(<DecisionBlock block={block} />);
+
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    const textarea = container.querySelector("textarea");
+    expect(textarea).not.toBeNull();
+
+    // Type some text and press Enter twice rapidly while in-flight.
+    // The submitting state should prevent a second POST.
+    fireEvent.change(textarea, { target: { value: "test answer" } });
+
+    // First Enter key press
+    fireEvent.keyDown(textarea, { key: "Enter", shiftKey: false });
+
+    // Second Enter key press while still in-flight - should be blocked by submitting state
+    fireEvent.keyDown(textarea, { key: "Enter", shiftKey: false });
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    // Verify only one POST to /answer was made
+    const answerCalls = fetchMock.mock.calls.filter(
+      ([url]) => url === "/api/decisions/dec-8/answer"
+    );
+    expect(answerCalls.length).toBe(1);
+  });
+
+  it("409 path triggers a refetch so block flips to answered state", async () => {
+    // --- Open decision with single option ---
+    // The GET mock MUST be request-ordered: the first GET (initial load)
+    // returns the pending decision so the option button is live and the
+    // POST actually runs; only the post-409 refetch returns the answered
+    // state. A url-matched mock that returned "answered" for every GET made
+    // this test pass without ever exercising conflict recovery.
+    const pendingDec1 = {
+      ...baseDecision,
+      id: "dec-1",
+      question: "Pick a framework",
+      type: "single_select",
+      options: [
+        { label: "React", value: "react" },
+        { label: "Vue", value: "vue" },
+      ],
+      context: "ui library",
+      status: "pending",
+      answer: null,
+      created_at: 1700000000,
+    };
+    const answeredDec1 = {
+      ...pendingDec1,
+      status: "answered",
+      answer: { value: "react", answered_by: "jay", answered_at: 1700000100 },
+    };
+    let decisionGets = 0;
+    const fetchMock = vi.fn().mockImplementation(async (req) => {
+      const url = req.url ?? req;
+      if (typeof url === "string" && url.endsWith("/answer")) {
+        // Answer POST returns 409 (someone else answered first)
+        return { status: 409, ok: false, json: async () => ({ error: "already answered" }) };
+      }
+      decisionGets += 1;
+      return {
+        ok: true,
+        json: async () => (decisionGets === 1 ? pendingDec1 : answeredDec1),
+      };
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    const block: DecisionContentBlock = {
+      kind: "decision",
+      decision_id: "dec-1",
+    };
+    const { container } = render(<DecisionBlock block={block} />);
+
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    // Click first option - this will get a 409 from the server
+    // (simulating someone else already answered first)
+    // Use container.querySelector to find the first button
+    const button = container.querySelector('button');
+    fireEvent.click(button);
+
+    // After the 409 handler refetches, the decision should flip to answered
+    // The refetched decision should have status "answered"
+    await waitFor(() => {
+      expect(container.textContent).toContain("answered: React");
+      expect(container.textContent).not.toContain("open");
+    });
+
+    // The POST must actually have run -- guards against the block starting
+    // out answered (disabled button, no-op click, vacuous pass).
+    const answerCalls = fetchMock.mock.calls.filter(([req]) => {
+      const url = req.url ?? req;
+      return typeof url === "string" && url.endsWith("/answer");
+    });
+    expect(answerCalls.length).toBe(1);
+  });
+
+  it("shows the conflict fallback when the 409 refetch rejects", async () => {
+    const pendingDec2 = {
+      ...baseDecision,
+      id: "dec-2",
+      question: "Pick a framework",
+      type: "single_select",
+      options: [
+        { label: "React", value: "react" },
+        { label: "Vue", value: "vue" },
+      ],
+      context: "ui library",
+      status: "pending",
+      answer: null,
+      created_at: 1700000000,
+    };
+    let decisionGets = 0;
+    const fetchMock = vi.fn().mockImplementation(async (req) => {
+      const url = req.url ?? req;
+      if (typeof url === "string" && url.endsWith("/answer")) {
+        return { status: 409, ok: false, json: async () => ({ error: "already answered" }) };
+      }
+      decisionGets += 1;
+      if (decisionGets === 1) {
+        return { ok: true, json: async () => pendingDec2 };
+      }
+      // Post-409 refetch dies on the network: the user must still learn
+      // their answer lost the race, not see a generic "Failed to answer".
+      throw new TypeError("network down");
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    const block: DecisionContentBlock = {
+      kind: "decision",
+      decision_id: "dec-2",
+    };
+    const { container } = render(<DecisionBlock block={block} />);
+
+    await waitFor(() => {
+      expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull();
+    });
+
+    const button = container.querySelector('button');
+    fireEvent.click(button);
+
+    await waitFor(() => {
+      const alert = container.querySelector('[role="alert"]');
+      expect(alert).not.toBeNull();
+      expect(alert.textContent).toContain("already answered");
+    });
+  });
 });