From da620456df7e063a5c531d294bb2e034af977710 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 9 Aug 2026 12:44:26 -0700 Subject: [PATCH 1/4] fix(web): render a sales mutation error inside the dialog that raised it (#474) SalesPage renders its error paragraph three times - once inside the new-order dialog, once inside the payment dialog, once for the page - and all three carried the PAGE's guard: {error && !creatingOrder && !paying &&
{error}
} `!creatingOrder` / `!paying` are false exactly when the dialog holding that copy is open, so the in-dialog copies could never render. A failed createOrder or recordPayment cleared its spinner and said nothing; the message only appeared once the user closed the dialog, by which point it reads as a page-level error with no context. The two in-dialog copies drop the guard entirely - Dialog returns null while closed, so its presence IS the condition - and gain role="alert", since focus is trapped in the panel and nothing else announces the failure. The page copy keeps `!creatingOrder && !paying` so the message is never rendered twice. Pre-existing on main; found during the #469 review and deliberately left out of that PR's scope. Three tests, none of which existed before (which is how three identical guards drifted into two wrong places): a rejected create-order and a rejected payment each assert the message inside the open dialog AND that exactly one copy exists, and a rejected add-item asserts the page copy still renders with no dialog open. Mutation-checked from both sides - re-adding either dialog guard, unguarding the page copy, and deleting the page copy each turn a named test red, on a green baseline. 1516 web tests green, typecheck clean, coverage above every floor. --- web/src/routes/SalesPage.test.tsx | 61 +++++++++++++++++++++++++++++++ web/src/routes/SalesPage.tsx | 17 ++++++--- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/web/src/routes/SalesPage.test.tsx b/web/src/routes/SalesPage.test.tsx index bc5508fba..d7d63c4bb 100644 --- a/web/src/routes/SalesPage.test.tsx +++ b/web/src/routes/SalesPage.test.tsx @@ -1002,3 +1002,64 @@ 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); + }); + + 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..99bef3891 100644 --- a/web/src/routes/SalesPage.tsx +++ b/web/src/routes/SalesPage.tsx @@ -528,8 +528,12 @@ export function SalesPage() { setOrderDate(e.target.value)} /> - {/* An open dialog renders its own copy of the error. */} - {error && !creatingOrder && !paying &&{error}
} + {/* #474 — no `!creatingOrder` guard here: this copy lives INSIDE the + dialog, which renders nothing while closed, so the page's + suppression condition would have hidden the message exactly when + the dialog it belongs to is up. role="alert" because focus is + trapped in the panel and nothing else announces the failure. */} + {error &&{error}
}{error}
} + {/* #474 — inside the dialog, so no `!paying` guard: see the + new-order dialog above. */} + {error &&{error}
}{error}
} {message &&{message}
} From 62fa7e8b79fc702985a19bf24d9f1aeade9d575e Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 9 Aug 2026 12:52:08 -0700 Subject: [PATCH 2/4] fix(web): scope a sales error to the action that raised it (#474) Codex P2 on #476, and both internal reviewers found the same thing: the first commit stopped HIDING the error inside the dialogs but did not say whose error it was. `error` is one flat string 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 as the dialog's own: - open a confirmed order, then open New order before its listOrderPayments read settles - the rejection renders "Could not load this order's payments." inside the new-order dialog; - start a panel write (Add line), open New order, let the write reject - a line-item message appears in a dialog that has no lines; - void a payment, open Record payment, let the void reject - same shape one dialog over. The error now carries the scope that raised it - the same string run() already takes - and each dialog renders only its own. The page copy is suppressed for exactly the message the open dialog is showing, so an unrelated failure is still reported rather than swallowed, and nothing is ever rendered twice. This mirrors StockPage, which already keeps its dialog failure in a separate `dialogError`. Three more tests, one per source above, each asserting the message is ABSENT from the open dialog and PRESENT on the page. The mutation set is now eight, all killed on a green baseline (51 SalesPage tests): both dialog copies re-guarded on the page condition, both rendered unscoped, the page copy unguarded / deleted / suppressed without the scope test, and run() tagging every failure alike. An earlier version of the scope mutant SURVIVED - the only unrelated failure under test came from the payments effect, which run() never tags. The panel-write test exists because of that survivor. 1519 web tests green, typecheck clean. --- web/src/routes/SalesPage.test.tsx | 75 +++++++++++++++++++++++++++++++ web/src/routes/SalesPage.tsx | 52 ++++++++++++++------- 2 files changed, 111 insertions(+), 16 deletions(-) diff --git a/web/src/routes/SalesPage.test.tsx b/web/src/routes/SalesPage.test.tsx index d7d63c4bb..a0cc7bfc6 100644 --- a/web/src/routes/SalesPage.test.tsx +++ b/web/src/routes/SalesPage.test.tsx @@ -1049,6 +1049,81 @@ describe("SalesPage in-dialog errors (#474)", () => { 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("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. diff --git a/web/src/routes/SalesPage.tsx b/web/src/routes/SalesPage.tsx index 99bef3891..dae9e5763 100644 --- a/web/src/routes/SalesPage.tsx +++ b/web/src/routes/SalesPage.tsx @@ -144,7 +144,15 @@ export function SalesPage() { }; const clearKey = (scope: string) => keys.current.delete(scope); - const [error, setError] = useState{setupError}
{t("loading")}
{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}
}{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}
}{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}
}