fix: refuse a second signup popup while an open one is still undecided (TWO-40)

[dgjlindsay]

Follow-up to #479: two sequential Sole Trader chip clicks stacked two hosted-signup popups — openingSignup only guards re-entry within one gesture, and the no-email branch #479 added inherits the same pre-existing gap as the resolved-no-match branch.

The guard in launchSignup is popup-scoped: refuse only while an open popup's outcome is undecided (a close watcher is live AND nothing adopted, or a re-signup pending). It never refuses a post-accept re-signup, a deferred applyPrefetch launch, a blocked-popup retry (a blocked popup creates no watcher), the note link, or a launch with only a prefetch flight outstanding — the two previously rejected predicates (bare watcher count; flight-inclusive isDeciding()) each broke one of those, and all five are now pinned by tests.

🤖 Generated with Claude Code

[github-actions]

🖌 Pre-commit success 🏆

Details

Downloading virtualenv (5.1MiB)
 Downloaded virtualenv
Installed 11 packages in 10ms
prettier.................................................................Passed

Exit code: 0

Author ✍️@dgjlindsay

[dgjlindsay]

HOLD — do not merge. Adversarial review round 3 found two HIGH regressions traced to round 2's own fix (the !win.closed disjunct breaks the "at most one undecided record" invariant the ACCEPTED pairing relies on: the handler can pair with a stale hand-closed record, refusing a sanctioned relaunch and stranding soleTraderReconfirmingCount). Per the TWO-40 review policy this is the third oscillation on this guard — hard stop, handed back for a human decision rather than another fix round.

Candidate direction (unverified, not applied): pair ACCEPTED with the live popup — find(c => !c.decided && !c.win.closed) or hold the record from launchSignup — and re-verify rounds' probes.

Review rounds 1–3 by Claude.

[dgjlindsay]

Round 3 hold resolved — pairing no longer searches by outcome

What was wrong

Round 2's !win.closed disjunct in the launch guard was correct on its own terms — a hand-closed popup genuinely should not refuse a fresh click during its stale 300ms poll window. But it deliberately allows a relaunch to open alongside a hand-closed record, and that falsified the invariant documented on activePopupWatchers and relied on by the ACCEPTED handler: "at most one record is ever undecided".

With two undecided records in the list, the handler's find(c => !c.decided) scanned forward — oldest first — and returned the stale hand-closed record in preference to the live popup that actually sent the message. Marking the wrong one decided caused both HIGH findings:

• Refused a sanctioned relaunch. The live popup stayed !decided with an open window, so the launch guard read it as "a popup is still deciding" and refused the "select a different sole trader" link the accept had just earned the buyer — for as long as the hosted flow left its own window up, i.e. potentially forever.
• Stranded soleTraderReconfirmingCount. The decrement was billed to staleRecord.isReconfirming. Where the stale record was a first-time signup and the accepting popup a re-signup, nothing decremented, the count stuck above zero, and isDeciding() then blocked every leave-sole-trader action.

Both were reproduced as failing tests before any fix — 5 of the 6 new pairing tests were red against the held tip.

What changed

The search is gone, not re-tuned. findPopupWatcher reads event.source: the browser names the window that posted, and a WindowProxy stays reference-comparable across origins, so the popup identifies itself and the number of undecided records stops mattering. Fallbacks exist only for a popup that closes in the same turn it posts (null source) and scan newest first, so a relaunch always wins over the stale record it opened over.

The decrement is now gated on the receipt that actually settles a popup, so a replayed ACCEPTED cannot spend a second decrement against one increment.

An unmatched non-null source deliberately still falls back rather than refusing to pair — documented at the call site, because refusing would strand the counter on any browser whose source isn't reference-equal to the window.open return, a worse failure than mis-marking a record in a replay no live window can send.

Why this is not the same class of bug as rounds 1-3

Rounds 1, 2 and 3 each re-tuned a predicate over the record list — soleTraderAdopted + count, then watcher.decided, then decided && !win.closed — and each round found the new predicate wrong for some other popup. This change removes the lookup-by-search entirely, so there is no predicate left to get wrong. The guard's predicate is untouched; round 2's fix is preserved intact (pinned by its own test).

The false invariant in the activePopupWatchers doc comment — the actual root cause, since round 2 broke a documented guarantee that round 1's pairing depended on and nobody updated the doc — is corrected to say the opposite explicitly.

Review rounds run

• Round 1 found three defects, two in my own initial fix: the mode-revert deferral asked !other.decided, which let a stale poll revert mode out from under a popup that was decided but still on screen after an accept resolved to no buyer — dropping the buyer's retry on the handler's own mode !== "sole_trader" gate. It now asks whether another popup is still ON SCREEN. Also: the newest-first scan order had no test covering it (verified by mutating both loops to forward scans — the suite stayed green), which is now closed.
• Round 2 found nothing behavioural introduced by round 1. One pre-existing, unreachable replay mis-pair was identified and accepted rather than hardened — a closed window cannot postMessage, and speculative hardening there is what produced the earlier oscillations. Documented at the call site so it isn't "fixed" later.
• Round 3 found only diff hygiene: an unrelated prettier reformat (local prettier 3.9.6 vs the 3.1.0 pre-commit pins — reverted, and formatting verified against the pinned version) and one awkward comment wrap. No behavioural finding, and nothing caused by round 2.

No oscillation: rounds 2 and 3 each found nothing behavioural introduced by the round before.

Tests

Nine new tests, each verified mutation-sensitive against the specific predicate it pins (reverted the predicate, confirmed the test goes red, restored): stale-record pairing, the refused post-accept re-signup, the stranded counter, the replayed ACCEPTED, both no-source fallbacks, the newest-first scan order, and the revert deferral in both directions.

Full suite: 644 passed, 18 suites, 0 failures (635 before this change). Prettier clean under the pinned 3.1.0.

Not merging

Flagging for human sign-off rather than self-merging, given three prior oscillations on this guard. The specific thing worth a human eye: event.source identity against the window.open return is verified in jsdom and is spec behaviour, but has not been exercised against the real hosted signup flow in a browser — that is the one assumption the whole fix rests on.

_{by Claude}