diff --git a/web/src/routes/SalesPage.test.tsx b/web/src/routes/SalesPage.test.tsx index bc5508fba..43de96016 100644 --- a/web/src/routes/SalesPage.test.tsx +++ b/web/src/routes/SalesPage.test.tsx @@ -1002,3 +1002,216 @@ describe("SalesPage cross-window display while loading (#469)", () => { expect(screen.queryByText("SO-OLD")).not.toBeInTheDocument(); }); }); + +// #474 — the screen renders the error paragraph three times: once per dialog +// and once for the page. All three carried the PAGE's guard (`!creatingOrder && +// !paying`), which is false exactly when the dialog holding that copy is open — +// so a mutation that failed under a dialog cleared its spinner and said +// nothing. The dialog copies are guarded by the Dialog itself (it renders +// nothing while closed), and the page copy keeps the suppression so the message +// is never duplicated. +describe("SalesPage in-dialog errors (#474)", () => { + it("shows a failed create-order inside the new-order dialog", async () => { + await renderReady(); + mockCreateOrder.mockRejectedValueOnce( + new ApiError(422, "Validation failed", "Order date cannot be in the future.")); + + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { + fireEvent.click(within(dialog()).getByRole("button", { name: "New draft order" })); + }); + + // The dialog stays up (a throw keeps it open) and now says why. + expect(within(dialog()).getByText("Order date cannot be in the future.")).toBeInTheDocument(); + // Exactly one copy: the page-level paragraph stays suppressed behind the + // open dialog, so a fix that simply dropped the page guard fails here. + expect(screen.getAllByText("Order date cannot be in the future.")).toHaveLength(1); + }); + + it("shows a failed payment inside the payment dialog", async () => { + mockListOrderPayments.mockResolvedValue({ + items: [], paidMinorUnits: 0, outstandingMinorUnits: 12000, totalMinorUnits: 12000, + currencyCode: "BHD", currencyMinorUnit: 3, + }); + await openOrder( + { ...draftEmpty(3, "BHD", "o9"), referenceNumber: "SO-9", status: "Confirmed", totalMinorUnits: 12000, items: [ITEM_A] }, + /Grade A Dozen/); + fireEvent.click(await screen.findByRole("button", { name: "Record payment" })); + + mockRecordPayment.mockRejectedValueOnce( + new ApiError(422, "Validation failed", "Payment exceeds the outstanding balance.")); + fireEvent.change(within(dialog()).getByLabelText(/Amount/), { target: { value: "99" } }); + await act(async () => { + fireEvent.click(within(dialog()).getByRole("button", { name: "Record payment" })); + }); + + expect(within(dialog()).getByText("Payment exceeds the outstanding balance.")).toBeInTheDocument(); + expect(screen.getAllByText("Payment exceeds the outstanding balance.")).toHaveLength(1); + }); + + // Codex review of #476: `error` is ONE state shared by every action on the + // screen, and neither dialog trigger is disabled while another request is in + // flight. So an unconditional in-dialog render presents someone else's + // failure — a payments read, a write started before the dialog opened — as + // the dialog's own. The error carries the scope that raised it, and each + // dialog shows only its own. + const CONFIRMED_9: SalesOrder = { + ...draftEmpty(2, "USD", "o9"), referenceNumber: "SO-9", status: "Confirmed", + totalMinorUnits: 2900, items: [ITEM_A], + }; + + it("keeps an unrelated failure out of the new-order dialog", async () => { + let rejectPayments!: (e: unknown) => void; + mockListOrderPayments.mockReturnValueOnce( + new Promise((_, rej) => { rejectPayments = rej; }) as never); + await openOrder(CONFIRMED_9, /Grade A Dozen/); + + // The trigger is live while that read is still out. + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { rejectPayments(new Error("boom")); }); + + const message = "Could not load this order's payments."; + expect(within(dialog()).queryByText(message)).not.toBeInTheDocument(); + // Not swallowed either — it belongs to the page, and says so there. + expect(screen.getByText(message)).toBeInTheDocument(); + }); + + it("keeps a panel write's failure out of the new-order dialog", async () => { + // The other source: not a background read but another WRITE, started + // before the dialog was opened. Its scope is the one run() was called + // with, so tagging every failure alike would land it here. + await openOrder(DRAFT_TWO, /Grade A Dozen/); + let rejectAdd!: (e: unknown) => void; + vi.mocked(addOrderItem).mockReturnValue( + new Promise((_, rej) => { rejectAdd = rej; }) as never); + fireEvent.click(screen.getByRole("button", { name: "Add line" })); + + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { rejectAdd(new ApiError(422, "Validation failed", "That product is no longer sellable.")); }); + + expect(within(dialog()).queryByText("That product is no longer sellable.")).not.toBeInTheDocument(); + expect(screen.getByText("That product is no longer sellable.")).toBeInTheDocument(); + }); + + it("keeps an unrelated failure out of the payment dialog", async () => { + mockListOrderPayments.mockResolvedValue({ + items: [{ + id: "pay1", salesOrderId: "o9", customerId: "c1", amountMinorUnits: 500, + currencyCode: "USD", currencyMinorUnit: 2, method: "Cash", paymentDate: "2026-07-20", + referenceNumber: "R1", note: null, voided: false, voidReason: null, version: 1, + }], + paidMinorUnits: 500, outstandingMinorUnits: 2400, totalMinorUnits: 2900, + currencyCode: "USD", currencyMinorUnit: 2, + }); + let rejectVoid!: (e: unknown) => void; + vi.mocked(voidPayment).mockReturnValue( + new Promise((_, rej) => { rejectVoid = rej; }) as never); + await openOrder(CONFIRMED_9, /Grade A Dozen/); + + // Start voiding a payment, leave it in flight… + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "void" })); + }); + fireEvent.change(within(dialog()).getByLabelText("Reason *"), { target: { value: "wrong order" } }); + await act(async () => { + fireEvent.click(within(dialog()).getByRole("button", { name: "Void payment" })); + }); + // …then open the payment dialog and let the void fail underneath it. + fireEvent.click(screen.getByRole("button", { name: "Record payment" })); + await act(async () => { rejectVoid(new ApiError(409, "Conflict", "That payment was already voided.")); }); + + expect(within(dialog()).queryByText("That payment was already voided.")).not.toBeInTheDocument(); + expect(screen.getByText("That payment was already voided.")).toBeInTheDocument(); + }); + + it("drops the dialog's own error when the dialog is dismissed", async () => { + // #474's own complaint, the other way round: a message about an abandoned + // attempt, left on the page after its dialog is gone, "reads as a + // page-level error with no context". The attempt is over — so is the + // message. + await renderReady(); + mockCreateOrder.mockRejectedValueOnce( + new ApiError(422, "Validation failed", "Order date cannot be in the future.")); + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { + fireEvent.click(within(dialog()).getByRole("button", { name: "New draft order" })); + }); + expect(within(dialog()).getByText("Order date cannot be in the future.")).toBeInTheDocument(); + + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(screen.queryByText("Order date cannot be in the future.")).not.toBeInTheDocument(); + }); + + it("says nothing when the dialog is dismissed before its write fails", async () => { + // Codex P2 + pi, same hole: dismissing only cleared an error that had + // ALREADY landed. A slow request the user gave up on still reported at + // page level afterwards — the context-free message again, now with the + // form that explains it gone. Cancel is live during `busy`, and Escape and + // the backdrop close the dialog too, so this is the ordinary case. + await renderReady(); + let rejectCreate!: (e: unknown) => void; + mockCreateOrder.mockReturnValueOnce(new Promise((_, rej) => { rejectCreate = rej; }) as never); + fireEvent.click(screen.getByRole("button", { name: "New order" })); + fireEvent.click(within(dialog()).getByRole("button", { name: "New draft order" })); + + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + await act(async () => { + rejectCreate(new ApiError(422, "Validation failed", "Order date cannot be in the future.")); + }); + + expect(screen.queryByText("Order date cannot be in the future.")).not.toBeInTheDocument(); + }); + + it("reports the next attempt after an abandoned one", async () => { + // The abandonment is per-attempt, not permanent: reopening and failing + // again must still say so, or the first Cancel would mute the dialog for + // the rest of the session. + await renderReady(); + let rejectCreate!: (e: unknown) => void; + mockCreateOrder.mockReturnValueOnce(new Promise((_, rej) => { rejectCreate = rej; }) as never); + fireEvent.click(screen.getByRole("button", { name: "New order" })); + fireEvent.click(within(dialog()).getByRole("button", { name: "New draft order" })); + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + await act(async () => { rejectCreate(new ApiError(500, "Server error", "abandoned")); }); + + mockCreateOrder.mockRejectedValueOnce( + new ApiError(422, "Validation failed", "Order date cannot be in the future.")); + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { + fireEvent.click(within(dialog()).getByRole("button", { name: "New draft order" })); + }); + + expect(within(dialog()).getByText("Order date cannot be in the future.")).toBeInTheDocument(); + }); + + it("keeps someone else's error when a dialog is dismissed", async () => { + // Only the dialog's OWN message goes with it. A failure that was never + // this dialog's is still the page's to report. + let rejectPayments!: (e: unknown) => void; + mockListOrderPayments.mockReturnValueOnce( + new Promise((_, rej) => { rejectPayments = rej; }) as never); + await openOrder(CONFIRMED_9, /Grade A Dozen/); + fireEvent.click(screen.getByRole("button", { name: "New order" })); + await act(async () => { rejectPayments(new Error("boom")); }); + + fireEvent.click(within(dialog()).getByRole("button", { name: "Cancel" })); + + expect(screen.getByText("Could not load this order's payments.")).toBeInTheDocument(); + }); + + it("still renders a page-level error with no dialog open", async () => { + // The panel's own writes are not behind a dialog — their errors must keep + // landing on the page, which is what the page copy's guard exists for. + await openOrder(DRAFT_TWO, /Grade A Dozen/); + vi.mocked(addOrderItem).mockRejectedValueOnce( + new ApiError(422, "Validation failed", "That product is no longer sellable.")); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Add line" })); + }); + + expect(screen.getByText("That product is no longer sellable.")).toBeInTheDocument(); + }); +}); diff --git a/web/src/routes/SalesPage.tsx b/web/src/routes/SalesPage.tsx index 91263a3a2..551accc5a 100644 --- a/web/src/routes/SalesPage.tsx +++ b/web/src/routes/SalesPage.tsx @@ -144,7 +144,19 @@ export function SalesPage() { }; const clearKey = (scope: string) => keys.current.delete(scope); - const [error, setError] = useState(null); + // #474 — the error carries the SCOPE that raised it, not just its text. One + // flat string was enough while every copy of it rendered in the same place; + // it is not enough now that the dialogs render their own, because neither + // dialog trigger is disabled while another request is in flight. A payments + // read (or a panel write started before the dialog was opened) that rejects + // underneath an open dialog would otherwise be presented as that dialog's + // own failure (codex review of #476). Scopes are the same strings run() + // already takes. + const [error, setError] = useState<{ scope: string; text: string } | null>(null); + // Scopes whose dialog was dismissed while their write was still in flight. + // A ref, not state: it is read in the settle path of a request that is + // already running, and must never be a render behind (#474). + const abandoned = useRef>(new Set()); const [message, setMessage] = useState(null); // Payments (#89, admin-only money data) — settlement state of the open @@ -253,7 +265,9 @@ export function SalesPage() { let cancelled = false; listOrderPayments(activeId) .then((p) => { if (!cancelled) setPayments(p); }) - .catch(() => { if (!cancelled) setError(i18n.t("sales:loadPaymentsFailed")); }); + .catch(() => { + if (!cancelled) setError({ scope: "payments", text: i18n.t("sales:loadPaymentsFailed") }); + }); return () => { cancelled = true; }; }, [activeId, activeStatus, canSettle]); @@ -279,15 +293,36 @@ export function SalesPage() { // guard but deliberately get no BusyButton treatment (#236 is writes). const run = (scope: string, fn: () => Promise) => runPending(scope, async () => { + // Cleared per attempt: abandoning one attempt must not mute the next. + abandoned.current.delete(scope); setError(null); setMessage(null); try { await fn(); } catch (err) { - setError(errText(err)); + // #474 — a dialog dismissed while its own write was still out has + // abandoned that attempt, and the failure has nowhere honest to land: + // reporting it on the page is the context-free message the issue was + // filed about, now without even the form that would explain it. Cancel + // stays live during `busy`, and Escape and the backdrop dismiss too, so + // this is ordinary, not a corner (codex + pi review of #476). + if (abandoned.current.has(scope)) return; + setError({ scope, text: errText(err) }); } }); + // #474 — dismissing a dialog abandons its attempt, so its failure goes with + // it. Leaving the message behind produces exactly what the issue was filed + // about: a page-level error with no context. Its OWN message only — a + // failure that was never this dialog's is still the page's to report. + const dismiss = (scope: string, setOpen: (open: boolean) => void) => { + setOpen(false); + abandoned.current.add(scope); + setError((current) => (current?.scope === scope ? null : current)); + }; + const closeNewOrder = () => dismiss("create-order", setCreatingOrder); + const closePayment = () => dismiss("record-payment", setPaying); + const onCreateOrder = () => run("create-order", async () => { // runWrite claims the list ticket before the POST, so a filter change // made while it is in flight keeps the view (#469). @@ -490,6 +525,12 @@ export function SalesPage() { // is part-way through editing (#469). Only the setup reads — customers and // products, without which no form on this screen can function — still gate // the page. + // Exactly the two cases a dialog renders itself (#474) — the page copy skips + // those and shows everything else. + const shownInDialog = + (creatingOrder && error?.scope === "create-order") + || (paying && error?.scope === "record-payment"); + if (setupError) return

{t("title")}

{setupError}

; if (orders.rows === null) return

{t("title")}

{t("loading")}

; @@ -517,7 +558,7 @@ export function SalesPage() { {/* Deliberately NOT a
: these controls were button-driven, so wrapping them in one would newly enforce min/step and swallow the screen's own money messages (codex review of #132). */} - setCreatingOrder(false)}> +
- {/* An open dialog renders its own copy of the error. */} - {error && !creatingOrder && !paying &&

{error}

} + {/* #474 — this copy lives INSIDE the dialog, which renders nothing + while closed, so the page's `!creatingOrder` condition hid the + message exactly when the dialog it belongs to was up. The scope + test replaces it: the dialog reports its OWN write, never + whatever else happened to fail underneath it. role="alert" + because focus is trapped in the panel and nothing else announces + the failure. */} + {error?.scope === "create-order" &&

{error.text}

}
- + {t("newDraftOrder")}
@@ -733,7 +780,7 @@ export function SalesPage() {
)} - setPaying(false)}> +
- {/* An open dialog renders its own copy of the error. */} - {error && !creatingOrder && !paying &&

{error}

} + {/* #474 — this dialog's own write only: see the new-order + dialog above. A void raised from the payments table can + land while this is open, and is not this form's failure. */} + {error?.scope === "record-payment" &&

{error.text}

}
- + {t("recordPayment")} @@ -793,8 +842,11 @@ export function SalesPage() {
)} - {/* An open dialog renders its own copy of the error. */} - {error && !creatingOrder && !paying &&

{error}

} + {/* The page's own copy, for everything not behind a dialog — and for a + failure that is nobody's dialog even while one is open, rather than + swallowing it. Suppressed only for the message the open dialog is + already showing, so it is never rendered twice (#474). */} + {error && !shownInDialog &&

{error.text}

} {message &&

{message}

}

{t("ordersHeading")}