SPA: generalise the abandoned-success guard — shared action hook for all dialog screens (follow-up to #477)

Delivery status — 2026-09-08

PRs 1–5 are merged: #704 (shared hook + Sales modal dialogs), #705 (Customers/Daily Entry/Flocks/Grades/Products), #706 (Inventory/Expenses/History/Stock), #710 (Users and hook load support), and #711 (Sales order-detail panel callers). PR5 merged as f0f749293c443be12cd310b3828339ada1a1987d.

PR6 is implemented in #715 and awaits owner merge. Owner chose option B plus a decision record: typed session-edge helpers, unchanged runtime behavior, and explicit acceptance of the remaining scope risks. Driver verification and final-head CI pass; independent review is complete. This issue remains open until #715 merges.

Separately tracked follow-ups stay outside this series: #708/#709 (PR3 test residuals), #712 (primary Open-read race), and #713 (live-editor reconciliation after refetch/status changes). The defect/inventory descriptions below are the original scope, not a claim that merged slices remain broken.

Follow-up to #477 / PR #702, which fixed Sales only. Three findings were raised by review there, deferred deliberately, and recorded on the closed issue rather than filed. This is them.

The defect, restated

An in-flight write whose dialog was dismissed runs its success side effects unconditionally when it lands: it resets fields the user has since typed into and force-closes the dialog that replaced it, discarding their input. useDialogErrors (#479) stops an abandoned attempt reporting a failure; nothing covered the success direction, which is worse because it destroys work rather than withholding a message.

#702 fixed this on Sales with useDialogSession — a per-scope monotonic generation, claimed before the first await, checked before any write to state the on-screen session owns.

What is still broken

1. The same hijack on the other #479 screens. Customers, Daily Entry, Flocks, Grades and Products all have equivalent create paths. CustomersPage.tsx:127 is representative: submit New customer, cancel, reopen and type, then let the old success land — it resets the fields and closes the replacement form.

2. Non-dialog run callers are unprotected. Sales gates only dialog scopes; everything else is permanently "current" by design. The same stale-success shape exists on panel actions — start Add line, close the panel, let the request finish, and the panel reopens. Update / remove / confirm / void have the same shape.

3. useDialogSession fails open on a mistyped scope. begin("create") with claim("create-order") never invalidates. No current path makes that mistake, and it is a deliberate default — a screen that has not adopted begin behaves exactly as it did before the hook existed, rather than silently gating every success off — but it is a sharp edge once the hook is used widely.

Why a shared hook, not five more copies

Measured on main:

19 run("scope", …) action call sites across 13 screens.
11 screens hand-write the same beginAttempt → try → report wiring inline.
Only SalesPage wrapped it in a run() helper — and that wrapper is where the correct gating now lives.

So the wiring is re-derived per screen, and each re-derivation is a chance to get it wrong. That is not hypothetical: this exact class has now cost four review rounds (#474 → #477 → #479 → #702).

Promote SalesPage's run wrapper into a shared hook that performs beginAttempt, claims the session, wraps the try/catch, calls report, and hands the action a current() predicate. Each action then loses ~6 lines of plumbing and gets the gate by default.

What this does NOT fix — read before scoping

A shared hook makes the check available and consistent. It does not make the bug impossible. It cannot decide what a superseded success must still do, and that judgement is where two of the five defects found in #702 actually were:

key rotation / clearKey must run when superseded — skip it and the next write replays the abandoned one under a spent idempotency key;
a money confirmation must run — the payment happened, and silence invites paying twice;
form resets and the dialog close must not run.

Every migrated call site needs that question answered individually. A <FormDialog> component owning open/close/session/errors would close more of the gap by construction, but it rewrites every dialog's markup — noted as an alternative, not proposed here.

Suggested scope — not one PR

Screen inventory, measured on main — modal count and errors.beginAttempt count per route file. The original step 2 named five screens — the #479 list copied forward, not a measurement. Five more screens have dialogs and gated actions and carry the identical bug. Corrected here.

Screen	Dialogs	Gated actions	Step	Wiring today
Sales	4	1	1	already correct (#702)
Customers	4	2	2	inline, hand-written ×2
Daily Entry	2	1	2	inline
Flocks	6	1	2	`run(scope, errorScope, …)` wrapper
Grades	4	1	2	`run(scope, dialogScope, …)` wrapper
Products	6	3	2	mixed — one wrapper, two inline
Users	14	8	2b	inline ×8
Inventory	8	3	2b	mixed
Expenses	4	2	2b	mixed
History	2	2	2b	inline
Stock	2	1	2b	inline
Feed	0	0	—	not affected — inline form, no dialog session to supersede; `runWrite` already covers the list
Water	0	0	—	same

Extract the shared hook and migrate Sales onto it. Behaviour-neutral; proves the shape against the one screen already known correct.
Migrate Customers, Daily Entry, Flocks, Grades, Products — 8 actions, answering the superseded-safe question per action. Flocks/Grades/Products already have the wrapper, so most of the diff is Customers + Daily Entry.
2b. Migrate Users, Inventory, Expenses, History, Stock — 16 actions. Users takes its own PR: 8 actions including set-password, change-role, change-email and disable/enable, where a wrong gate decision is expensive to detect.
Handle the non-dialog / panel callers separately; a panel is not a dialog session and the right semantics differ.
Decide on finding 3 (fail-open scope) once the hook has more than one consumer.

19 call sites is a diff large enough to hide a defect, which is the main argument for splitting it.

One ordering that must survive the migration

Flocks, Grades and Products already run refresh() before clearKey(scope), deliberately: if the refresh throws, the key survives and a retry replays the idempotent write instead of duplicating it. Both lines are facts about the world, so both must still run when superseded, and in that order. Gating either one is #702's P1 repeated.

Verify

Per migrated screen: a test that submits, dismisses, reopens, then lands the original success — asserting the replacement session's input survives and its dialog stays open. Mutation-check each gate (removing it must redden that screen's named test). Pin anything that must still run when superseded — key rotation especially — with its own test, because that is the direction that silently loses money or orders.

Amended the scope section: the screen list is now measured rather than inherited.

What was wrong. Step 2 named Customers, Daily Entry, Flocks, Grades and Products. That is #479's screen list carried forward, not a survey of what actually has the defect. Counting modals and errors.beginAttempt call sites per route file on main finds five more screens with dialogs and gated actions: Users (8 actions), Inventory (3), Expenses (2), History (2), Stock (1) — 16 actions, more than double the eight in the original step 2. Users alone has more than all five originally-named screens combined.

Those five are now step 2b, with Users on its own PR: set-password, change-role, change-email and disable/enable are where a wrong gate decision is expensive to detect.

Two things the survey also settled:

Feed and Water are not affected. Zero modals — inline forms, so there is no dialog session to supersede. runWrite already covers their list side.
Flocks, Grades and Products already carry the run(scope, errorScope, action) wrapper, so those three are near-mechanical swaps; most of step 2's diff is Customers and Daily Entry, which hand-write the wiring inline.

Also recorded in the body: those three wrappers run refresh() before clearKey(scope) on purpose — if the refresh throws, the key survives and a retry replays the idempotent write instead of duplicating it. Both are facts about the world, so both must still run when superseded, in that order. Gating either is #702's P1 repeated.

Correction to the amendment above, and a revised PR split. Reading the success paths rather than counting call sites changes the picture.

"Users is the biggest gap" was wrong

Users has the most gated actions (8), but it already implements this pattern by hand in six places — activeCreate / activePw / activeRole / activeEmail / activeUser refs carrying { targetId, generation }, each with an isCurrentDialog() checked before the message and the close. It also already answers the superseded-safe question correctly: clearKey before the refresh, refresh unconditional, message and dialog close gated.

Users is a consolidation, not a rescue — with two genuine exceptions.

The two real Users defects

onSubmitStepUp (disable/enable, UsersPage.tsx:685) and onUpdate (edit-user, UsersPage.tsx:735) guard on identity, not generation:

if (activeStepUp.current !== target.id) return;   // dialog moved on
if (activeEdit.current !== target.id) return;

Close and reopen the same user: the id matches, the guard passes, and the stale success closes the reopened dialog. A generation separates the two sessions; an id cannot. Same class as #625, which is why Customers' edit path already carries a generation.

Customers is split, not broken

edit-customer has a generation guard (#625) and is correct. create has none and is the reproducer in the issue body. Migration deletes the hand-rolled generation rather than layering a second one beside it.

Revised split

PR	Scope	Actions
1	Extract the hook; migrate Sales (behaviour-neutral)	1
2	Customers, Daily Entry, Flocks, Grades, Products	8
3	Inventory, Expenses, History, Stock — no existing guards, each answered from scratch	8
4	Users — consolidation + the two id→generation fixes	8
5	Non-dialog / panel callers	~4
6	Finding 3 (fail-open scope)	—

Not 2+3+4 in one PR: 24 gate decisions in one diff is enough to hide the defect class this ticket exists to remove.

One constraint any migration must not break

Flocks/Grades/Products refresh before clearKey; Customers' edit and Users' onUpdate clear the key before the refresh (#163 review). Both are correct for their screen and neither is this ticket's to change. Preserve whichever the screen has, and gate neither line — both are facts about the world.

A full implementer handoff (measured tables, the hook shape to extract, six traps each traceable to a past review round, and the verification recipe) is written up at docs/plans/703-shared-dialog-session/00-handoff.md — uncommitted; say the word and it goes in with the first PR.

test(web): pin that useDialogAction claims the session before onAttem…

fix(web): every dialog-session edge both mutes the attempt still out …

test(web): pin that a useDialogAction attempt un-mutes the scope its …

test(web): pin that a useDialogAction dismissal alone supersedes the …

docs(plans): the #703 implementer handoff — measured screen inventory…

docs(plans): amend the #703 handoff to the shipped openDialog/dismiss…

docs(plans): the #703 handoff points at the shipped useDialogAction, …

refactor(web): extract SalesPage's dialog-write wrapper into a shared…

fix(web): an abandoned dialog attempt's success no longer hijacks the…

PR 3 of 6 shipped — #706, squash 60a49978 (2026-09-07): Inventory, Expenses, History and Stock migrated onto useDialogAction, per statement (write/key/refresh RUN in each screen's own order; resets/close/message GATED on current()).

What landed beyond the migration, for whoever picks up PR 4:

the two PR 2 review leaks closed where they apply: if (busy) return; ahead of every pre-run beginAttempt (Inventory purchase/adjust, Stock write-off) and the admin-demotion useEffect on Inventory create/edit/adjust + Stock write-off (two new *.roleChange.test.tsx files mock useAuth the way DailyEntryPage.test.tsx does — the token-seeded AuthProvider cannot flip a role mid-render);
Stock's hand-rolled activeWriteOffLotId ref deleted; its write-off trigger stays mouse-reachable (no disabled={busy}), so its same-lot reopen after a dismissal is pinned by a real behaviour test;
same-id re-entry on History startAdjust and Stock openWriteOff is spared (if (x === null || x.id !== id) openDialog(scope)) — unlike PR 2's unconditional openDialog; the 409 rebinds (Expenses, History) are not open edges and keep one explicit beginAttempt each;
review round 1: Expenses onToggleCategory now spends its idempotency key once the toggle lands even when the categories reload fails on transport (owner-approved scope amendment; the sibling runWrite callers were shown NOT to have this defect — usePagedList.refreshWindow returns {status:"failed"} rather than throwing).

Remaining on this issue: PR 4 (Users), PR 5 (Sales' panel callers), PR 6 (the hook's fail-open default). Not closing.

docs(web): record the per-screen idempotency-key policies and runWrit…

fix(web): a disable/enable dismissed during step-up issuance no longe…

docs(web): two Users comments describe the reachability that exists (#…

docs(web): startLoad and the flock-access load say why the slot is le…

style(web): flock-access load block at its own depth; the rebind comm…

fix(web): an abandoned edit's success no longer hijacks the dialog th…

PR 4 shipped — #710, squash 778faab9 (2026-09-08).

What landed:

useDialogAction gains run(scope, action, { dialog }) (the message slot and the session route by the named dialog while the pending scope stays per record/payload) and startLoad(scope) (a dialog that loads before it opens: the latest click wins, the slot is left alone until the load lands). Both additive and typed by the screen's own dialogScopes; the ten existing callers are untouched.
All seven Users dialogs are on the hook; every hand-rolled {id, generation} ref and isCurrentDialog() closure is gone; every per-record run scope and isPending site is byte-identical.
The two id-only guards are fixed: edit-user's same-user close-and-reopen hijack (RED on main before the fix), and disable/enable's — whose reopen is unreachable through the UI (both row triggers are disabled={busy}) and is pinned by the close-on-success wiring test.
Found in review and fixed in the same PR: disable/enable was the only step-up write with no current() check between the grant and the write, so a Cancel during issuance still landed the disable (contract INV-13; test does not let a dismissed disable continuation write). One pre-existing vacuous test repaired (the same-worker stale-assign test never picked a flock since Three screens pick a default flock from the capped flock list #646).

Recorded, not changed (pre-existing, outside the locked scope — for whoever picks up PR 5/6 or the next Users touch):

change-email gates current() BEFORE clearKey, and closeEmail clears the key while a write may still be out (owner item 3, kept verbatim; two review seats converged on it).
change-email swallows a SELF step-up 401 Auth.CredentialsSuperseded silently at any stage, including stepUp itself.
One pre-existing test comment (UsersPage.test.tsx ~3190, "worker A's verdict must survive…") still describes a route Dialog.tsx's inert (Sales: both dialogs can be open at once, so the untagged dialog-error slot misattributes #480) removed; the code comments were corrected, the test comment was left (existing tests were oracles).

What remains on this issue: PR 5 (Sales' panel callers onto session semantics) and PR 6 (the hook's fail-open default on a mistyped run scope — note { dialog } is typed, so a misspelt dialog name is already a compile error). #703 stays open.

fix(web): keep Sales order panels closed after pending writes (#711)

test(web): pin the two #703 PR 3 residual guards (Stock key rotation,…

fix(web): constrain dialog session helpers to declared scopes (#715)

Complete. The abandoned-success guard is now shared across every dialog screen via web/src/components/useDialogAction.ts.

PR 1 refactor(web): extract SalesPage's dialog-write wrapper into a shared useDialogAction hook (#703) #704 — extract the hook, migrate Sales.
PR 2 fix(web): an abandoned dialog attempt's success no longer hijacks the replacement on Customers, Daily Entry, Flocks, Grades and Products (#703) #705 — Customers, Daily Entry, Flocks, Grades, Products.
PR 3 fix(web): an abandoned dialog attempt's success no longer hijacks the replacement on Inventory, Expenses, History and Stock (#703) #706 — Inventory, Expenses, History, Stock.
PR 4 fix(web): an abandoned edit's success no longer hijacks the dialog that replaced it on Users (#703) #710 — Users consolidation (six hand-rolled generation refs → current()) + the two id→generation fixes + a review-found step-up-issuance defect.
PR 5 fix(web): keep Sales order panels closed after pending writes #711 — Sales order-detail panel liveness (activeIdRef).
PR 6 fix(web): constrain dialog session helpers to declared scopes #715 — type the session-edge helpers (openDialog/dismissDialog/startLoad) to the declared scope union S (a mistyped scope is now a compile error) + a decision record for the accepted residual fail-open (docs/decisions/703-dialog-session-scope-types.md).
Test residuals test(web): pin Stock write-off's clearKey-before-refresh ordering (#703 PR 3 residual) #708 / test(web): pin History void→adjust cross-scope close on the success path (#703 PR 3 residual) #709 — test(web): pin the two #703 PR 3 residual guards (Stock key rotation, History void→adjust close) #714.

Pre-existing follow-ups found during PR 5 review remain open: #712 (Sales Close vs the primary Open read) and #713 (line-editor reconcile after refetch/status change).

chore(main): release 0.1.0 (#542)

Navigation Menu

Repository navigation

SPA: generalise the abandoned-success guard — shared action hook for all dialog screens (follow-up to #477) #703

Delivery status — 2026-09-08

The defect, restated

What is still broken

Why a shared hook, not five more copies

What this does NOT fix — read before scoping

Suggested scope — not one PR

One ordering that must survive the migration

Verify

"Users is the biggest gap" was wrong

The two real Users defects

Customers is split, not broken

Revised split

One constraint any migration must not break

7 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

SPA: generalise the abandoned-success guard — shared action hook for all dialog screens (follow-up to #477) #703

Description

Delivery status — 2026-09-08

The defect, restated

What is still broken

Why a shared hook, not five more copies

What this does NOT fix — read before scoping

Suggested scope — not one PR

One ordering that must survive the migration

Verify

Activity

mforce commented on Sep 6, 2026

mforce commented on Sep 6, 2026

"Users is the biggest gap" was wrong

The two real Users defects

Customers is split, not broken

Revised split

One constraint any migration must not break

7 remaining items

mforce commented on Sep 7, 2026

mforce commented on Sep 8, 2026

mforce commented on Sep 8, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions