From cc58dfd32f9732e77a18b023f5e1935c1cee800d Mon Sep 17 00:00:00 2001 From: jaylfc Date: Sun, 16 Aug 2026 20:43:57 +0000 Subject: [PATCH 1/3] fix decision block answer submission: disable buttons during submit, clear answerError on new attempts, distinguish refresh failure, refetch on 409, reset state on decision_id change --- changelog.d/tsk-nxmiby-decisionblock-ux.md | 7 + desktop/src/apps/MessagesApp.tsx | 40 +++- .../__tests__/DecisionBlock.test.tsx | 173 ++++++++++++++++++ 3 files changed, 212 insertions(+), 8 deletions(-) create mode 100644 changelog.d/tsk-nxmiby-decisionblock-ux.md 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/desktop/src/apps/MessagesApp.tsx b/desktop/src/apps/MessagesApp.tsx index 3658727f7..e1fa48ace 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]); - async function answerDecision( + 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,16 @@ 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) { + const updatedRes = await fetch(`/api/decisions/${decision.id}`); + if (updatedRes.ok) { + const updated = await updatedRes.json(); + setDecision(updated as DecisionData); + } + setSubmitting(false); + return; + } throw new Error( typeof detail === "string" ? detail : "Could not record answer.", ); @@ -552,10 +570,16 @@ 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 +633,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 +685,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 +697,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 80fc98911..0e74838c0 100644 --- a/desktop/src/components/__tests__/DecisionBlock.test.tsx +++ b/desktop/src/components/__tests__/DecisionBlock.test.tsx @@ -556,49 +556,41 @@ describe("DecisionBlock", () => { 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" }) }; } - if (typeof url === "string" && url.endsWith("/dec-1")) { - // Refetch GET returns answered state after 409 handling - return { - ok: true, - json: async () => ({ - ...baseDecision, - id: "dec-1", - question: "Pick a framework", - type: "single_select", - options: [ - { label: "React", value: "react" }, - { label: "Vue", value: "vue" }, - ], - context: "ui library", - status: "answered", - answer: { value: "react", answered_by: "jay", answered_at: 1700000100 }, - created_at: 1700000000, - }), - }; - } - // Initial decision fetch + decisionGets += 1; return { ok: true, - json: async () => ({ - ...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, - }), + json: async () => (decisionGets === 1 ? pendingDec1 : answeredDec1), }; }); vi.stubGlobal("fetch", fetchMock); @@ -625,5 +617,64 @@ describe("DecisionBlock", () => { 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"); + }); }); });