Repository navigation
fix(web): render a sales mutation error inside the dialog that raised it (#474) #476
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,7 +144,19 @@ export function SalesPage() { | |
| }; | ||
| const clearKey = (scope: string) => keys.current.delete(scope); | ||
|
|
||
| // #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<Set<string>>(new Set()); | ||
| const [message, setMessage] = useState<string | null>(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({ 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<void>) => | ||
| 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) { | ||
| // #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)); | ||
|
Comment on lines
+318
to
+321
There was a problem hiding this comment.
If the user submits either dialog and then dismisses it before the request rejects, Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Real, and fixed in bab7f66. pi found the identical hole independently, with the same reasoning — the previous commit only cleared an error that had already landed, and the realistic ordering is the opposite one: the user gives up on a slow request and cancels, then it rejects.
I took your first suggestion (invalidate the eventual error) rather than the second (block dismissal while pending): Two tests: dismiss-then-reject says nothing anywhere; reopen-and-fail still reports. Three more mutants — the late-failure guard removed, the dismissal never recorded, the marker never cleared — bringing the set to thirteen, all killed on a green baseline of 55 SalesPage tests. One related thing I am not changing, so it is on the record rather than silently skipped: 1523 web tests green, typecheck clean. |
||
| }; | ||
| 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 <section><h2>{t("title")}</h2><p className="error">{setupError}</p></section>; | ||
| if (orders.rows === null) return <section><h2>{t("title")}</h2><p className="muted">{t("loading")}</p></section>; | ||
|
|
||
|
|
@@ -517,7 +558,7 @@ export function SalesPage() { | |
| {/* Deliberately NOT a <form>: 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). */} | ||
| <Dialog open={creatingOrder} title={t("newOrder")} onClose={closeNewOrder}> | ||
| <div className="form-grid"> | ||
| <label>{t("customer")} | ||
| <select value={customerId} onChange={(e) => setCustomerId(e.target.value)}> | ||
|
|
@@ -528,10 +569,16 @@ export function SalesPage() { | |
| <input type="date" value={orderDate} max={today} | ||
| onChange={(e) => setOrderDate(e.target.value)} /> | ||
| </label> | ||
| {/* #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" && <p className="error" role="alert">{error.text}</p>} | ||
| <div className="dialog-foot"> | ||
| <button type="button" className="link" onClick={closeNewOrder}>{tc("cancel")}</button> | ||
| <BusyButton disabled={busy || !customerId} busy={isPending("create-order")} | ||
| onClick={onCreateOrder}>{t("newDraftOrder")}</BusyButton> | ||
| </div> | ||
|
|
@@ -733,7 +780,7 @@ export function SalesPage() { | |
| </div> | ||
| )} | ||
|
|
||
| <Dialog open={paying} title={t("recordPayment")} onClose={closePayment}> | ||
| <div className="form-grid"> | ||
| <label>{t("date")} | ||
| <input type="date" value={payDate} max={today} | ||
|
|
@@ -760,10 +807,12 @@ export function SalesPage() { | |
| <input value={payNote} maxLength={500} | ||
| onChange={(e) => setPayNote(e.target.value)} /> | ||
| </label> | ||
| {/* #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" && <p className="error" role="alert">{error.text}</p>} | ||
| <div className="dialog-foot"> | ||
| <button type="button" className="link" onClick={closePayment}>{tc("cancel")}</button> | ||
| <BusyButton disabled={busy || !payAmount} busy={isPending("record-payment")} | ||
| onClick={onRecordPayment}> | ||
| {t("recordPayment")} | ||
|
|
@@ -793,8 +842,11 @@ export function SalesPage() { | |
| </div> | ||
| )} | ||
|
|
||
| {/* 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 && <p className="error">{error.text}</p>} | ||
| {message && <p className="success">{message}</p>} | ||
|
|
||
| <h3>{t("ordersHeading")}</h3> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a confirmed order's
listOrderPaymentscall remains pending, the user can open New order and submit because this read does not set the sharedbusyguard. IfcreateOrderrejects first, its actionable 422 appears in the dialog, but a later payment-read rejection executes this assignment and replaces the sole error state with thepaymentsscope, making the create-order explanation disappear from the still-open modal. Keep errors per scope, or avoid overwriting an active dialog's own error with this background-read failure.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Real, and you are right that I under-rated it: I had written this off in the PR body as "pre-existing, accepted, needs a per-scope map". The sharp version is not a message is lost, it is the message the user is acting on is replaced while they read it.
#476 merged before this round closed, so the fix is in #478 (issue #477): the state splits into
error(the page's, untagged) anddialogError(whichever dialog is open — they are modal, so at most one ever is, and each clears it on open).run()routes by aDIALOG_SCOPESlist, and each attempt clears only its own slot, so neither can overwrite the other and trying something new never drops a failure the user has not dealt with. It removes more than it adds:shownInDialog, the page copy's guard, the scope tag on the page error, and the clear-on-dismiss all go.Four more tests, thirteen mutants all killed on a green baseline of 59 — including one that renders the dialog slot on the page, which is what keeps the dismissal tests honest now that dismissal clears nothing.
An internal reviewer also found a second, still-open half in the same family: an abandoned attempt's success (not failure) force-closes and re-navigates a session the user reopened. That is tracked in #477 and deliberately not in #478 — it needs a per-session generation.