feat(web): a per-place error store, with Sales as its first consumer (#479) by mforce · Pull Request #489 · mforce/cluckwork

mforce · 2026-08-10T03:15:35Z

First of three for #479. This one is the primitive plus one consumer; the other ten screens and the Help widening follow separately.

What is here

useDialogErrors keeps one error slot per place a message can appear — the page, and each dialog by scope. It landed in bc14839e with no consumers. This PR gives it its first, and grows it by the piece the extraction left behind.

That piece is the abandoned-attempt marker, which is the actual #474 fix. No dialog trigger anywhere is gated on busy, so a user can dismiss a form whose write is still out and reopen it immediately — and that late failure would then be reported against a session it knows nothing about. Sales carried it as a local ref plus a guard in its catch. Ten more screens need it across twenty-two dismiss sites, and a screen that forgets it looks correct in review while still passing both tests the issue asks for.

So the hook now owns the whole attempt lifecycle:

method	does
`beginAttempt(scope \| null)`	clears that slot, and un-mutes the scope
`abandon(scope)`	empties the slot and mutes the attempt still out
`report(scope \| null, text)`	routes a failure to its slot, or nowhere if abandoned

Muting is per attempt, not per dialog — otherwise one dismissal would silence the form the user reopened and is filling in now.

Folding the clear into abandon is what lets the open triggers go back to a plain setOpen. Dismissal and reopen are the same twenty-two transitions counted from opposite ends; clearing at the dismissal touches each once instead of twice.

DialogError is the render half — twenty-two sites, so the markup is decided once instead of copied, and role="alert" with it. Sales carried the role; Stock does not.

Nothing the user can see changes

Worth stating plainly, because an earlier draft of this section overstated it and drew a doc-sync review finding.

The effect that closes the payment form when the active order changes now empties that slot itself. Previously the dialog trigger cleared it on the way in; now the dismissal clears it on the way out. Both end with "the reopened form shows no stale message" — same pixels, different mechanism. That is exactly why Sales' 64 existing tests pass unchanged.

It uses abandon rather than clearDialog because the alternative needed an argument about what cannot happen, and that argument was wrong twice: the panel's own Close is ungated, and canSettle flips with no button at all when a transparent 401 refresh re-derives the role mid-write.

And a mutation says the choice is unobservable. Swapping it back to clearDialog breaks no test, and no test can be written that it would break — every route back into a payment form re-runs the effect, which clears the slot on the way in regardless. It buys the removal of a twice-wrong argument, not a fix. Deleting the line entirely is caught. Two tests written to pin the distinction were deleted rather than kept: one was disproved by its own mutant, the other only supported the argument that was dropped.

No Help or glossary change is due here for the same reason. The doc work this slice owes is item 2 of #479's "done when" — widening the line #478 narrowed — and that line states an app-wide behaviour that is only true once the remaining ten screens convert. It lands in PR 3.

Why the existing tests are the proof

Sales' 64 tests pass unchanged through the swap — that is the cheapest evidence the extraction is faithful, and the reason this PR converts no other screen. One test is added, for the order-change path, which had no coverage because nothing could observe it before.

Mutation-checked on a green baseline of 84:

mutant	result
drop the order-change clear	red — 1
`abandon` stops clearing	red — 3
`beginAttempt` stops un-muting	red — 2
`report` stops honouring the mute	red — 3
`DialogError` renders the page slot	red — 8
`clearDialog` loses its identity guard	survives

The survivor is honest and left as-is: it is a re-render optimisation with no observable behaviour, and no test claims otherwise.

Verification

1562 web tests green, typecheck clean, npm run build clean.
Coverage moved up on every metric, so the floors are unchanged: lines 92.55, statements 89.75, functions 85.2, branches 80.99 against floors 91 / 88 / 83 / 79.
No GLOSSARY.md or Help change here — the app-wide claim is only true once the remaining screens land, which is PR 3.

Follow-ups

PR 2: the other ten screens, StockPage included.
PR 3: widen the Help line narrowed in fix(web): give the sales dialogs their own error slot (#477) #478, after fix(web): one page, one scroll lock and one live dialog (#482) #483 merges.
The success message slot is shared the same way; separate issue to follow.

Sales spent four review rounds (#474 -> #477 -> #480 -> #481) learning that one error string per screen cannot be right once dialogs render their own copy. Each round was a different consequence of the same shape: a dialog showing a background read's failure as its own; a tagged slot that fixed attribution but let the second failure erase the first; and clearing 'the error' whenever anything started, which dropped failures the user had not dealt with. useDialogErrors keeps one slot per PLACE a message can appear - the page, and each dialog by scope - so all three are impossible rather than defended against. Seven tests, six mutants all killed. The screens migrate onto it in the commits that follow.

The hook landed in the previous commit with no consumers. This gives it its first one, and grows it by the piece Sales had that the extraction left behind. That piece is the abandoned-attempt marker. It is the actual #474 fix - no dialog trigger anywhere is gated on `busy`, so the user can dismiss a form whose write is still out and reopen it immediately, and that late failure would be reported against a session it knows nothing about. Sales carried it as a local ref and a guard in its catch. Ten more screens need it, across twenty-two dismiss sites, and a screen that forgets it looks correct in review and still passes both of the tests the issue asks for. So it moves into the hook as two methods: abandon - empties the dialog's slot AND mutes the attempt still out report - routes a failure to its slot, or nowhere if abandoned beginAttempt now un-mutes as well as clears, because muting is per attempt, not per dialog: without that, one dismissal would silence the form the user reopened and is filling in now. Folding the clear into `abandon` is what lets the open triggers go back to plain setOpen. The two ends are the same twenty-two transitions, and clearing at the dismissal touches each of them once instead of twice. DialogError is the render half. Twenty-two sites, so the markup is decided once rather than copied - and `role="alert"` with it, which Sales carried and Stock did not. One behaviour change, not a refactor: the effect that closes the payment form when the active order changes now clears that slot explicitly. Until now the trigger cleared it on the way back in, which covered this path by accident; with the clear moved to the dismissal, and the screen closing the form being no dismissal, a 422 about SO-9's money would be sitting in SO-10's form. `clearDialog`, not `abandon`, deliberately: the open trigger is disabled while busy and usePendingAction refuses a second action outright, so no write can be in flight here, and muting would be an unreachable guard reading as a real one. Sales' 64 existing tests are the proof the extraction is faithful - they pass unchanged through the swap. One is added for the order-change path, which had no coverage because nothing could observe it before. Mutation-checked on a green baseline of 84: dropping the order-change clear, stopping `abandon` from clearing, stopping beginAttempt from un-muting, stopping `report` from honouring the mute, and pointing DialogError at the page slot each turn it red. The identity guard in clearDialog survives, honestly - it is a re-render optimisation with no observable behaviour, and no test claims otherwise. 1562 web tests green, typecheck clean, build clean. Coverage moved up on every metric, so the floors are unchanged: lines 92.55, statements 89.75, functions 85.2, branches 80.99.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c21492b4b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-10T03:19:04Z

    setPaying(false);
+    errors.clearDialog("record-payment");


Document the payment-error behavior in the SPA Help

This explicitly changes user-visible behavior by clearing a previous order's payment failure before the next order's form opens, but the commit contains no corresponding SPA Help or in-app glossary update. The repository requires user-visible behavior and its documentation to land together, so include the error-scoping/dismissal guidance here rather than deferring it to a follow-up.

AGENTS.md reference: AGENTS.md:L159-L169

Useful? React with 👍 / 👎.

Checked this against the code rather than the PR prose, and the premise does not hold: PR 1 changes nothing the user can see. No Help or glossary update is due here. My own PR body caused this — it said "one behaviour change, not a refactor", which overstates what the line does. Corrected in 4e458c0c and in the description.

The reopen behaviour is identical before and after. Previously the dialog trigger cleared the slot on the way in (SalesPage.tsx:584/:814 on main); now the dismissal clears it on the way out. Both end with "the reopened form shows no stale message" — same pixels, different mechanism. That is exactly why Sales' 64 existing tests pass unchanged through the swap, which is the evidence this PR rests on.

The order-change path is the same story. Under main a payment failure left behind by an order switch was wiped by the clear-on-open when the next payment form was opened; now it is wiped when the panel closes. A user cannot reach a moment where the two differ — and that is not an assertion, it is a mutation result: swapping the new call back to clearDialog breaks no test, and no test can be written that it would break, because every route back into a payment form re-runs the effect and clears the slot on the way in regardless. That survivor is recorded in the PR body and in a comment at the call site.

Nothing else in the diff is user-facing: the dialog markup is byte-equivalent (role="alert" was already on both Sales dialog slots and DialogError preserves it), no strings were added or changed, and setDialog leaving the hook's public surface is internal.

On deferring: the doc work this slice owes is item 2 of #479's own "done when" — widening the Help line that #478 deliberately narrowed. That line can only be widened truthfully once the remaining ten screens carry the split, because it states an app-wide behaviour. Widening it now would put Help back in exactly the state #478's third commit was filed to correct: promising something ten screens do not do. It lands in PR 3 of 3, with en/es/tl together, restoring the reviewed wording from d157a97b.

Verification on 4e458c0c: 1566 web tests green, typecheck clean, build clean, coverage unchanged at 92.55 lines / 89.75 statements / 85.2 functions / 80.99 branches. Ten mutants killed, two documented survivors — the full ledger is in the PR comment above.

…gs (#479) Four reviewers on #489 - codex, pi, and two local agents. Every finding below is theirs; the mutation results are mine. `report` no longer consumes the mute. I added consumption in response to pi, which was wrong: reporting twice in one settle path - a catch plus a finally, a retry wrapper, a validation throw after a caught network error - would then put the SECOND message inside the dialog the user already dismissed, which is #474 reintroduced by its own safeguard. The case pi was protecting against cannot occur, because every dialog-scoped report is preceded by `beginAttempt`, which already prunes. Pruning belongs there and only there. `setDialog` is no longer returned. It writes a slot bypassing the mute and had no caller outside the two test fixtures, so the first of the ten screens to reach for the obvious-looking setter would have reintroduced #474 with nothing failing. `report` is the only way in now. The payments effect abandons rather than clears. My justification for clearing was that no write can be out when it runs, and that enumeration was wrong twice: the panel's own Close is ungated, and `canSettle` flips with no button at all when a transparent 401 refresh re-derives the role mid-write. Two misses of one shape means the method is wrong, so the code stops reasoning about reachability. Stated plainly because a mutation says so: swapping that call back to `clearDialog` breaks no test, and no test can be written that it would break - every route back into a payment form re-runs the effect, which clears the slot on the way in regardless. It buys the removal of a twice-wrong argument, not an observable fix. Deleting the line entirely IS caught. The comment says exactly this, and the two tests I had written to "pin" the distinction are deleted rather than kept, because one of them was disproved by its own mutant and the other only existed to support the argument I dropped. The effect also declares its real dependencies now, via destructured stable members rather than the `errors` object, which is rebuilt every render and would have re-fetched payments on each one. There is no eslint in this package to have caught it. Four hook tests added for gaps the test reviewer found: two scopes abandoned at once, un-muting one at a time, a page attempt not disturbing the mute set, and reporting twice while muted. Mutants, on a green baseline of 88 - ten killed: abandon stops clearing the slot red, 4 tests beginAttempt stops un-muting red, 7 report ignores the mute red, 7 report consumes the mute red, 1 abandon replaces the mute set red, 1 beginAttempt un-mutes every scope red, 1 beginAttempt null clears the mute set red, 1 DialogError renders the page slot red, 19 DialogError drops role=alert red, 2 effect drops its slot handling red, 1 Two survivors, both deliberate and both documented in the code: the identity guard in clearDialog is a re-render optimisation with no observable behaviour, and the abandon-versus-clear choice in the effect is the one described above. Three checks in my first run reported DID NOT APPLY and were repaired before being counted; one of those had been mutating a doc comment instead of the JSX. 1566 web tests green, typecheck clean, build clean, coverage unchanged at 92.55 lines / 89.75 statements / 85.2 functions / 80.99 branches.

mforce · 2026-08-10T03:39:57Z

Local review round: codex, pi, and two Claude agents (one adversarial, one on tests/conventions). Findings and what changed, in 4e458c0c.

Taken

report no longer consumes the mute. I had added consumption in response to pi's finding that abandon mutes on idle dismissals. That was the wrong fix: it makes report a mutating read, so a settle path that reports twice — a catch plus a finally, a retry wrapper, a validation throw after a caught network error — puts the second message inside the dialog the user already dismissed. That is #474, reintroduced by its own safeguard. pi's case is also unreachable: every dialog-scoped report is preceded by beginAttempt, which already prunes. Pruning belongs there and only there, and the test now pins that reporting twice while muted stays silent.

setDialog is no longer returned. It writes a slot bypassing the mute, and had no caller outside the two test fixtures. The first of the ten follow-up screens to reach for the obvious-looking setter in a settle path would have reintroduced #474 with nothing failing.

The payments effect abandons rather than clears. My justification for clearing was that no write can be out when it runs. That enumeration was wrong twice — the panel's own Close is ungated, and canSettle flips with no button at all when a transparent 401 refresh re-derives the role mid-write. Two misses of one shape means the method is wrong, so the code stops reasoning about reachability.

The effect declares its real dependencies, via destructured stable members rather than the errors object, which is rebuilt every render and would have re-fetched payments on each one. No eslint in this package to have caught it.

Four hook tests for gaps the test reviewer found: two scopes abandoned at once, un-muting one at a time, a page attempt leaving the mute set alone, and reporting twice while muted.

Reported honestly rather than fixed

Swapping the effect's abandon back to clearDialog breaks no test, and no test can be written that it would break — every route back into a payment form re-runs the effect, which clears the slot on the way in regardless. The change buys the removal of a twice-wrong argument, not an observable fix. Deleting the line entirely is caught. The two tests I had written to pin the distinction are deleted, not kept: one was disproved by its own mutant, the other only existed to support the argument I dropped.

Codex's "abandonment is keyed by dialog, not attempt" is real as a contract statement but unreachable in every consumer, because usePendingAction serialises on a synchronous ref, so two attempts on one scope cannot overlap. Attempt identity would need the hook to own the runner, which was considered and rejected up front — it collides with the existing usePendingAction / usePagedList wrappers every screen already uses. Noted as a constraint on the ten screens rather than papered over.

Mutants, green baseline of 88 — ten killed

mutant
`abandon` stops clearing the slot	red — 4
`beginAttempt` stops un-muting	red — 7
`report` ignores the mute	red — 7
`report` consumes the mute	red — 1
`abandon` replaces the mute set	red — 1
`beginAttempt` un-mutes every scope	red — 1
`beginAttempt(null)` clears the mute set	red — 1
`DialogError` renders the page slot	red — 19
`DialogError` drops `role="alert"`	red — 2
effect drops its slot handling	red — 1
`clearDialog` loses its identity guard	survives — re-render optimisation, documented
effect clears instead of abandoning	survives — the case above, documented

Three checks in my first pass reported DID NOT APPLY and were repaired before being counted; one had been mutating a doc comment instead of the JSX.

1566 web tests green, typecheck clean, build clean, coverage unchanged at 92.55 lines / 89.75 statements / 85.2 functions / 80.99 branches.

@codex please re-review.

mforce · 2026-08-10T03:41:18Z

@codex the doc-sync P1 is answered on its thread and the PR body wording that prompted it is corrected. Short version: PR 1 changes nothing the user can see — the reopen behaviour is identical, the clear simply moved from the dialog trigger to the dismissal, and a mutation confirms the remaining choice is unobservable. The Help widening this slice owes is item 2 of #479's "done when" and can only be stated truthfully once the other ten screens carry the split, so it lands in PR 3 with en/es/tl together. Latest commit 4e458c0c also addresses codex, pi and two local agents — ten mutants killed, two documented survivors.

chatgpt-codex-connector · 2026-08-10T03:42:27Z

No further findings on 4e458c0c.

The doc-sync P1 is resolved: the Sales dialog’s observable reopen behavior is unchanged, and documenting the app-wide behavior before the remaining screens migrate would be premature. The updated PR body now states that boundary clearly.

Checks

✅ git diff --check f273879c470647fc142f0d4afa06acd14497ed89..HEAD
⚠️ npm test -- --run src/components/useDialogErrors.test.tsx src/components/DialogError.test.tsx src/routes/SalesPage.test.tsx (environment has Node 20; the installed Vitest toolchain requires a newer Node runtime and exits on unsupported --no-experimental-webstorage)

No code changes were needed, so no commit or follow-up PR was created.

View task →

chatgpt-codex-connector · 2026-08-10T03:43:00Z

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 4e458c0c4c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…) (#495) #478 narrowed "a message about something that failed appears where the work is" to just "a failed save explains itself inside the form" — CustomersPage's background balance-load failure landed inside its own New customer dialog at the time, the exact opposite of the wider claim, so Help couldn't promise app-wide what only Sales did. #479 (#489, #491) gave every dialog screen its own page/dialog split: FlocksPage, InventoryPage, ProductsPage, UsersPage, GradesPage, ExpensesPage, HistoryPage, DailyEntryPage, CustomersPage. Every screen with a <Dialog> now uses useDialogErrors — a background read can no longer land inside an open form, and dismissing a form drops only its own message. The wider claim is true app-wide now, so this restores #478's original wording verbatim (en/es/tl) rather than writing new copy — it was already reviewed and correct, just early. No GLOSSARY.md change: same reasoning #478 recorded — "error" is not a domain concept, nothing here renames or redefines one. Test reverts to the wide assertion from #477's original commit; the narrow #478 wording fails it, confirmed by mutation. 1644 web tests green, typecheck and build clean. Co-authored-by: mforce <>

mforce added 2 commits August 9, 2026 18:03

chatgpt-codex-connector Bot reviewed Aug 10, 2026

View reviewed changes

mforce merged commit 4f493ec into main Aug 10, 2026
10 checks passed

mforce deleted the feat/479-dialog-error-split branch August 10, 2026 05:10

cluckwork-lockfix Bot mentioned this pull request Aug 10, 2026

chore(main): release 0.0.4 #461

Merged

This was referenced Aug 10, 2026

fix(web): give every dialog screen its own error slot (#479) #491

Merged

docs(web): widen the Help line #478 narrowed, now that it is true (#479) #495

Merged

Navigation Menu

feat(web): a per-place error store, with Sales as its first consumer (#479) - #489

feat(web): a per-place error store, with Sales as its first consumer (#479)#489
mforce merged 3 commits into
mainfrom
feat/479-dialog-error-split

mforce commented Aug 10, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Uh oh!

mforce Aug 10, 2026

Uh oh!

mforce commented Aug 10, 2026

Uh oh!

mforce commented Aug 10, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 10, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mforce commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is here

Nothing the user can see changes

Why the existing tests are the proof

Verification

Follow-ups

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

mforce Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

mforce commented Aug 10, 2026

Taken

Reported honestly rather than fixed

Mutants, green baseline of 88 — ten killed

Uh oh!

mforce commented Aug 10, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 10, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mforce commented Aug 10, 2026 •

edited

Loading