diff --git a/changelog.d/tsk-nxmiby-decisionblock-ux.md b/changelog.d/tsk-nxmiby-decisionblock-ux.md new file mode 100644 index 000000000..211763d30 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/changelog.d/tsk-r6qnrv-decisionblock-error-prop.md b/changelog.d/tsk-r6qnrv-decisionblock-error-prop.md new file mode 100644 index 000000000..3488f35c9 --- /dev/null +++ b/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 diff --git a/desktop/src/apps/MessagesApp.tsx b/desktop/src/apps/MessagesApp.tsx index 3658727f7..a4d59a636 100644 --- a/desktop/src/apps/MessagesApp.tsx +++ b/desktop/src/apps/MessagesApp.tsx @@ -500,6 +500,7 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React const [error, setError] = useState(null); const [answer, setAnswer] = useState(""); const [answerError, setAnswerError] = useState(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 = { 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,7 +708,7 @@ export function DecisionBlock({ block }: { block: DecisionContentBlock }): React /> - {answerError && ( -
- {answerError} -
- )} + + )} + + {/* 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 && ( +
+ {answerError}
)} diff --git a/desktop/src/components/__tests__/DecisionBlock.test.tsx b/desktop/src/components/__tests__/DecisionBlock.test.tsx index 6da7f0523..0e74838c0 100644 --- a/desktop/src/components/__tests__/DecisionBlock.test.tsx +++ b/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(); + + 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(); + + 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(); + + 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(); + + 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"); + }); + }); }); diff --git a/desktop/src/components/__tests__/RedProof.trimspace.test.tsx b/desktop/src/components/__tests__/RedProof.trimspace.test.tsx index 7e38fa1d9..90aa218fa 100644 --- a/desktop/src/components/__tests__/RedProof.trimspace.test.tsx +++ b/desktop/src/components/__tests__/RedProof.trimspace.test.tsx @@ -20,6 +20,15 @@ function pendingFreeText(id: string) { return { ...baseFreeTextDecision, id, question: "Any notes?" }; } +function answeredFreeText(id: string, answerValue = "some answer") { + return { + ...baseFreeTextDecision, + id, + status: "answered" as const, + answer: { value: answerValue, answered_by: "someone", answered_at: 1700000001 }, + }; +} + describe("RED PROOF: controlled textarea trim-on-change", () => { afterEach(() => { vi.restoreAllMocks(); @@ -121,12 +130,13 @@ describe("RED PROOF: controlled textarea trim-on-change", () => { }); }); -describe("RED PROOF: server error reason surfaced in alert", () => { +describe("RED PROOF: 409 refetch + non-409 error surfacing", () => { afterEach(() => { vi.restoreAllMocks(); }); - it("surfaces the exact 409 server error message in role=alert", async () => { + it("409 refetches and flips the block to the answered state (no alert)", async () => { + let getCalls = 0; const fetchMock = vi.fn().mockImplementation(async (url: string, options?: RequestInit) => { if (options?.method === "POST") { return { @@ -135,13 +145,51 @@ describe("RED PROOF: server error reason surfaced in alert", () => { json: async () => ({ error: "already answered or not pending" }), }; } - return { ok: true, status: 200, json: async () => pendingFreeText("dec-err") }; + getCalls++; + if (getCalls === 1) { + return { ok: true, status: 200, json: async () => pendingFreeText("dec-409") }; + } + return { ok: true, status: 200, json: async () => answeredFreeText("dec-409") }; + }); + vi.stubGlobal("fetch", fetchMock); + + const block: DecisionContentBlock = { + kind: "decision", + decision_id: "dec-409", + }; + const { container } = render(); + + await waitFor(() => { + expect(container.querySelector('[data-decision-block="true"]')).not.toBeNull(); + }); + + const textarea = container.querySelector("textarea") as HTMLTextAreaElement; + + fireEvent.change(textarea, { target: { value: "some answer" } }); + fireEvent.keyDown(textarea, { key: "Enter", shiftKey: false }); + + await waitFor(() => { + expect(container.textContent).toContain("answered:"); + }); + expect(container.querySelector('[role="alert"]')).toBeNull(); + }); + + it("surfaces the exact non-409 server error message in role=alert", async () => { + const fetchMock = vi.fn().mockImplementation(async (url: string, options?: RequestInit) => { + if (options?.method === "POST") { + return { + ok: false, + status: 500, + json: async () => ({ error: "boom" }), + }; + } + return { ok: true, status: 200, json: async () => pendingFreeText("dec-500") }; }); vi.stubGlobal("fetch", fetchMock); const block: DecisionContentBlock = { kind: "decision", - decision_id: "dec-err", + decision_id: "dec-500", }; const { container } = render(); @@ -157,7 +205,7 @@ describe("RED PROOF: server error reason surfaced in alert", () => { await waitFor(() => { const alert = container.querySelector('[role="alert"]'); expect(alert).not.toBeNull(); - expect(alert!.textContent).toContain("already answered or not pending"); + expect(alert!.textContent).toContain("boom"); }); }); });