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

[mforce]

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

1. Extract the shared hook and migrate Sales onto it. Behaviour-neutral; proves the shape against the one screen already known correct.
2. 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.
3. Handle the non-dialog / panel callers separately; a panel is not a dialog session and the right semantics differ.
4. 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.

[mforce]

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.

[mforce]

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.