refactor(checkout): rollout follow-up — P1 correctness + cleanup (#3276)

Squashed rebase onto current main (which now includes #3259, #3260,
#3261, #3270, #3273, #3274, #3265). PR-3276 was originally written
against PR-11's tip WITHOUT PR-14 (#3270) in its ancestry; now that
#3270 is merged, this rebase reconciles PR-3276's refactors with
the referral + nested-event-shape fixes that landed in main
independently.

Conflicts resolved (5 regions in src/services/checkout.ts):
  1. event shape read: kept main's nested event.data.message.status
     check + renamed _successFired → successFired (PR-3276's closure
     refactor)
  2. startCheckout session reset: applied PR-3276's _resetOverlaySession
     hook AND kept main's effectiveReferral / loadActiveReferral from
     #3270
  3. already-entitled banner branch: kept main's auto-dismiss fix
     (from #3261/#3265). PR-3276 was written without this fix; not
     regressing it. Used PR-3276's inlined isEntitled() check
     (computeInitialBannerState deletion per its P1 #251).
  4+5. banner timeout / active branches: kept main's stopEmailWatchers
     + currentState + currentMaskedEmail AND applied PR-3276's
     _currentBannerCleanup = null cleanup hook (per its P1 #254
     re-mount leak fix)

Also removed stale `origin: 'dashboard'` field from saveCheckoutAttempt
call (PR-3276 deleted that field from CheckoutAttempt interface per
its P1 #251 — dead write-only field).

Net refactor delivers all of PR-3276's intended fixes:
- #247 Module-scoped _successFired → per-session closure
- #249 #3163 hard-dep code markers
- #251 Delete over-engineered primitives (-65 LOC)
- #254 Banner re-mount listener leak cleanup

Tests pass (checkout-attempt-lifecycle + checkout-banner-initial-state).
Typecheck clean.
This commit is contained in:
Elie Habib
2026-04-22 16:08:43 +04:00
committed by GitHub
parent 7bc2dc03f8
commit 156cc4b86b
7 changed files with 99 additions and 161 deletions

View File

@@ -80,7 +80,6 @@ import {
capturePendingCheckoutIntentFromUrl,
initCheckoutWatchers,
resumePendingCheckout,
sweepAbandonedCheckoutAttempt,
} from '@/services/checkout';
import { captureReferralFromUrl } from '@/services/referral-capture';
import {
@@ -992,14 +991,12 @@ export class App {
// any capture/resume path runs, so a stale session from a prior
// user can't bleed into the current one.
initCheckoutWatchers();
// Abandonment sweep: clear long-stale attempt records when the
// current page load carries no Dodo return params. Runs before
// capture (which repopulates the record for /pro handoff intent).
const hasCheckoutReturnParams = (() => {
const p = new URL(window.location.href).searchParams;
return !!(p.get('subscription_id') || p.get('payment_id'));
})();
sweepAbandonedCheckoutAttempt(hasCheckoutReturnParams);
// Stale attempt records are ignored by loadCheckoutAttempt() via
// the 24h TTL — no separate sweep needed. The attempt record's
// only consumer (the failure-retry banner) runs handleCheckoutReturn
// synchronously during panel-layout mount, which is after the
// captureePendingCheckoutIntentFromUrl repopulates it for any /pro
// handoff — so no race exists that would want to sweep pre-capture.
const pendingCheckout = capturePendingCheckoutIntentFromUrl();
if (pendingCheckout) {
// Checkout intent from /pro page redirect. Resume immediately if

View File

@@ -240,6 +240,15 @@ export class PanelLayoutManager implements AppModule {
// transition detector would swallow that snapshot as "legacy-pro" and
// the user would see locked panels until a manual refresh — exactly the
// symptom that caused the 2026-04-17/18 duplicate-subscription incident.
//
// REQUIRES_SKIP_INITIAL_SNAPSHOT_BEHAVIOR — the watcher is the SOLE
// automatic reload source for post-checkout success (the overlay
// handler in checkout.ts deliberately does NOT reload). If PR #3163's
// fix to `skipInitialSnapshot` is ever reverted, this detector
// swallows the activation silently and users see locked panels for
// 30s until the extended-unlock timeout fires a manual-refresh CTA.
// Regression guard: tests/entitlement-transition.test.mts locks the
// "incident sequence" semantics; see mirror marker in checkout.ts.
let lastEntitled: boolean | null = returnedFromCheckout ? false : null;
this.unsubscribeEntitlementChange = onEntitlementChange(() => {
const entitled = isEntitled();

View File

@@ -1,5 +1,5 @@
/**
* Primitive A — checkout attempt lifecycle (retry context store).
* Checkout attempt lifecycle retry context store.
*
* Separate from `PENDING_CHECKOUT_KEY` in checkout.ts because the two
* keys have different terminal-clear rules:
@@ -28,30 +28,23 @@ export interface CheckoutAttempt {
referralCode?: string;
discountCode?: string;
startedAt: number;
origin: 'dashboard' | 'pro';
}
export type CheckoutAttemptClearReason =
| 'success'
| 'duplicate'
| 'signout'
| 'dismissed'
| 'abandoned';
| 'dismissed';
/**
* Maximum age of a saved attempt before we treat it as stale and
* ignore on read. Generous (24h) so a user declined this morning can
* return this afternoon and retry the exact product they picked.
* This is the SOLE staleness gate — there is no separate abandonment
* sweep; records older than this are ignored by `loadCheckoutAttempt`.
*/
export const CHECKOUT_ATTEMPT_MAX_AGE_MS = 24 * 60 * 60 * 1000;
/**
* Silent-abandon cutoff for the mount-time sweep. Older than this AND
* no Dodo return params on the current URL → treat as abandoned and
* clear so a much-later visit doesn't resurface a stale retry banner.
*/
export const CHECKOUT_ATTEMPT_ABANDONED_MS = 30 * 60 * 1000;
export function saveCheckoutAttempt(attempt: CheckoutAttempt): void {
try {
sessionStorage.setItem(LAST_CHECKOUT_ATTEMPT_KEY, JSON.stringify(attempt));
@@ -97,24 +90,3 @@ export function clearCheckoutAttempt(reason: CheckoutAttemptClearReason): void {
clearReferralOnAttribution();
}
}
/**
* Mount-time defensive cleanup. Caller passes `hasReturnParams` so we
* never clear an attempt that a freshly-loaded ?status=failed URL is
* about to consume (the failed-redirect race).
*/
export function sweepAbandonedCheckoutAttempt(hasReturnParams: boolean): void {
if (hasReturnParams) return;
try {
const raw = sessionStorage.getItem(LAST_CHECKOUT_ATTEMPT_KEY);
if (!raw) return;
const parsed = JSON.parse(raw) as CheckoutAttempt;
const age = Date.now() - (parsed?.startedAt ?? 0);
if (age > CHECKOUT_ATTEMPT_ABANDONED_MS) {
sessionStorage.removeItem(LAST_CHECKOUT_ATTEMPT_KEY);
}
} catch {
// Malformed record — clear defensively.
try { sessionStorage.removeItem(LAST_CHECKOUT_ATTEMPT_KEY); } catch { /* noop */ }
}
}

View File

@@ -22,19 +22,6 @@ export const EXTENDED_UNLOCK_TIMEOUT_MS = 30_000;
*/
export const CLASSIC_AUTO_DISMISS_MS = 5_000;
/**
* Decide the initial banner state at mount time.
*
* If the user already has pro entitlement when the banner fires
* (e.g., the post-reload `consumePostCheckoutFlag` path where the
* entitlement watcher already flipped true), skip "pending" and go
* straight to "active" so the banner doesn't falsely suggest the
* webhook is still in flight.
*/
export function computeInitialBannerState(entitledNow: boolean): CheckoutSuccessBannerState {
return entitledNow ? 'active' : 'pending';
}
/**
* Mask an email address for display in the success banner so the
* full address isn't rendered in plaintext (privacy — a top-of-

View File

@@ -32,7 +32,6 @@ import { isEntitled, onEntitlementChange } from './entitlements';
import {
CLASSIC_AUTO_DISMISS_MS,
EXTENDED_UNLOCK_TIMEOUT_MS,
computeInitialBannerState,
maskEmail,
type CheckoutSuccessBannerState,
} from './checkout-banner-state';
@@ -42,7 +41,6 @@ import { resolvePlanDisplayName } from './checkout-plan-names';
export {
EXTENDED_UNLOCK_TIMEOUT_MS,
computeInitialBannerState,
maskEmail,
type CheckoutSuccessBannerState,
} from './checkout-banner-state';
@@ -51,7 +49,6 @@ export {
saveCheckoutAttempt,
loadCheckoutAttempt,
clearCheckoutAttempt,
sweepAbandonedCheckoutAttempt,
type CheckoutAttempt,
type CheckoutAttemptClearReason,
} from './checkout-attempt';
@@ -131,7 +128,7 @@ const PENDING_INTENT_TTL_MS = 15 * 60 * 1000;
let initialized = false;
let onSuccessCallback: (() => void) | null = null;
let _successFired = false;
let _resetOverlaySession: (() => void) | null = null;
let _watchersInitialized = false;
/**
@@ -147,6 +144,19 @@ export function initCheckoutOverlay(onSuccess?: () => void): void {
const env = import.meta.env.VITE_DODO_ENVIRONMENT;
// `successFired` must be scoped per-overlay-session, NOT module.
// Previously this was `let _successFired = false;` at module scope,
// which leaked state across sessions: if a user's success path ran
// and then a later `openCheckout` call re-entered the overlay, the
// stale `true` made the close handler skip the pending-intent clear,
// leaving PENDING_CHECKOUT_KEY populated for a silent auto-retry.
// DodoPayments.Initialize is idempotent (guarded by `initialized`),
// so there's only ever one onEvent closure — but ONE session's state
// must reset when a new overlay opens. `openCheckout` resets this
// flag via the exported `resetOverlaySessionState()` helper below.
let successFired = false;
_resetOverlaySession = () => { successFired = false; };
DodoPayments.Initialize({
mode: env === 'live_mode' ? 'live' : 'test',
displayType: 'overlay',
@@ -162,7 +172,7 @@ export function initCheckoutOverlay(onSuccess?: () => void): void {
? rawData.status
: (rawData?.message as Record<string, unknown> | undefined)?.status;
if (status === 'succeeded') {
_successFired = true;
successFired = true;
onSuccessCallback?.();
// Terminal success: clear both keys. LAST_CHECKOUT_ATTEMPT_KEY
// is no longer needed (no retry context required); PENDING is
@@ -174,14 +184,20 @@ export function initCheckoutOverlay(onSuccess?: () => void): void {
// detector would treat the first pro snapshot as "legacy-pro
// baseline" and swallow the activation.
//
// Reload ownership: as of PR-4, the entitlement watcher in
// panel-layout.ts is the SINGLE reload source (fires on
// free→pro transition). We no longer schedule a 3s setTimeout
// reload here — that competed with the entitlement watcher's
// reload and made "still unlocking" UX impossible because the
// banner was guaranteed to be wiped at 3s regardless of
// webhook latency. The watcher's reload depends on the
// 2026-04-18-001 fix landing first (#3163, merged).
// Reload ownership: the entitlement watcher in panel-layout.ts
// is the SINGLE reload source (fires on free→pro transition).
// We no longer schedule a belt-and-braces setTimeout reload
// here — that competed with the watcher and made "still
// unlocking" UX impossible because the banner was guaranteed
// to be wiped at 3s regardless of webhook latency.
//
// REQUIRES_SKIP_INITIAL_SNAPSHOT_BEHAVIOR — the watcher's
// first-snapshot seeding depends on PR #3163 (merged
// 2026-04-18) having fixed the swallow-first-snapshot bug.
// If that PR is ever reverted or its behavior regresses,
// tests in tests/entitlement-transition.test.mts will fail
// (specifically "simulates the incident sequence" case); see
// the mirror marker in panel-layout.ts.
markPostCheckout();
}
break;
@@ -194,7 +210,7 @@ export function initCheckoutOverlay(onSuccess?: () => void): void {
// the retry CTA. The attempt record will be cleared later by
// the terminal path that actually resolves (success, dismissed,
// duplicate, or the mount-time abandonment sweep).
if (!_successFired) {
if (!successFired) {
clearPendingCheckoutIntent();
}
break;
@@ -364,7 +380,6 @@ export function capturePendingCheckoutIntentFromUrl(): PendingCheckoutIntent | n
referralCode: intent.referralCode,
discountCode: intent.discountCode,
startedAt: Date.now(),
origin: 'pro',
});
url.searchParams.delete(CHECKOUT_PRODUCT_PARAM);
@@ -424,6 +439,11 @@ export async function resumePendingCheckout(options?: {
*/
export function openCheckout(checkoutUrl: string): void {
initCheckoutOverlay();
// Reset the per-session successFired flag so a prior session's
// terminal state can't leak into this one. (The flag lives in a
// closure inside initCheckoutOverlay's event handler; this resets
// it.)
_resetOverlaySession?.();
DodoPayments.Checkout.open({
checkoutUrl,
@@ -498,7 +518,6 @@ export async function startCheckout(
saveCheckoutAttempt({
...intent,
startedAt: Date.now(),
origin: 'dashboard',
});
openSignIn();
}
@@ -506,7 +525,7 @@ export async function startCheckout(
}
_checkoutInFlight = true;
_successFired = false;
_resetOverlaySession?.();
// Fall back to the stored referral when the caller doesn't pass one.
// A dashboard-origin upgrade click has no ref in hand — it arrives
// from a locked-panel CTA or the Manage Billing surface — but the
@@ -523,7 +542,6 @@ export async function startCheckout(
referralCode: effectiveReferral,
discountCode: options?.discountCode,
startedAt: Date.now(),
origin: 'dashboard',
});
try {
let token = await getClerkToken();
@@ -733,9 +751,22 @@ function renderCheckoutErrorSurface(
* "Refresh if features haven't unlocked" CTA + Sentry
* warning. Never silently disappears.
*/
// Module-scoped cleanup for the currently-mounted success banner.
// When `showCheckoutSuccess` is called a second time before the first
// resolves (e.g., Dodo has historically double-fired checkout.status
// — see docs/plans/2026-04-18-001-fix-pro-activation-race-*), this
// tears down the prior banner's entitlement subscription + timeout
// before mounting the new one. Without this, the prior `onEntitlementChange`
// listener stays in the Set with a closure over a detached DOM node,
// firing on every future entitlement update for the page lifetime.
let _currentBannerCleanup: (() => void) | null = null;
export function showCheckoutSuccess(
options?: { waitForEntitlement?: boolean; email?: string | null },
): void {
_currentBannerCleanup?.();
_currentBannerCleanup = null;
const existing = document.getElementById('checkout-success-banner');
if (existing) existing.remove();
@@ -833,13 +864,14 @@ export function showCheckoutSuccess(
return;
}
const initial = computeInitialBannerState(isEntitled());
if (initial === 'active') {
// Already entitled at mount. Auto-dismiss via CLASSIC_AUTO_DISMISS_MS
// (merged from PR-4's fix for the fast-path hang). PR-11 adds the
// email-banner handling + email-watcher cleanup so the stop callback
// doesn't leak into the tab's lifetime when this fast-path fires
// with an email-backfill subscription still active.
// If the user is already entitled when the banner fires (PR-4 merged
// the inline check — PR-3276's P1 #251 deleted the one-line
// computeInitialBannerState helper as an identity function, so the
// check is now direct). Auto-dismiss via CLASSIC_AUTO_DISMISS_MS
// (PR-4 fix for the fast-path hang). PR-11 adds email-banner +
// email-watcher cleanup so the stop callback doesn't leak into the
// tab's lifetime when this fast-path fires with a backfill sub active.
if (isEntitled()) {
currentState = 'active';
setBannerText(banner, 'active', currentMaskedEmail);
setTimeout(() => {
@@ -855,6 +887,7 @@ export function showCheckoutSuccess(
resolved = true;
unsubscribe();
stopEmailWatchers();
_currentBannerCleanup = null;
currentState = 'timeout';
setBannerText(banner, 'timeout', currentMaskedEmail);
Sentry.captureMessage('Checkout entitlement-activation timeout', {
@@ -870,9 +903,20 @@ export function showCheckoutSuccess(
clearTimeout(timeoutHandle);
unsubscribe();
stopEmailWatchers();
_currentBannerCleanup = null;
currentState = 'active';
setBannerText(banner, 'active', currentMaskedEmail);
});
// Register cleanup so a re-entrant showCheckoutSuccess call (e.g. a
// double-fire of `checkout.status=succeeded`) tears down this
// banner's listener + timer before mounting a replacement.
_currentBannerCleanup = () => {
if (resolved) return;
resolved = true;
clearTimeout(timeoutHandle);
unsubscribe();
};
}
function setBannerText(

View File

@@ -1,12 +1,11 @@
/**
* Exercises the save/load/clear primitives for LAST_CHECKOUT_ATTEMPT_KEY
* and the abandonment sweep. The key invariant under test is the two-key
* separation: Primitive A's PENDING_CHECKOUT_KEY and LAST_CHECKOUT_ATTEMPT_KEY
* have different terminal-clear triggers (see the plan's Primitive A section).
* Exercises the save/load/clear primitives for LAST_CHECKOUT_ATTEMPT_KEY.
* The two-key separation (attempt record vs pending auto-resume intent)
* and the 24h staleness gate are the invariants under test.
*
* Only pure storage helpers are exercised here — startCheckout() and the
* Dodo overlay event handlers require a browser/SDK environment and are
* covered by manual + E2E paths per the PR plan.
* covered by manual + E2E paths.
*/
import { describe, it, beforeEach, before, after } from 'node:test';
@@ -58,12 +57,8 @@ beforeEach(() => {
_sessionStorage.clear();
});
const {
saveCheckoutAttempt,
loadCheckoutAttempt,
clearCheckoutAttempt,
sweepAbandonedCheckoutAttempt,
} = await import('../src/services/checkout-attempt.ts');
const checkout = await import('../src/services/checkout-attempt.ts');
const { saveCheckoutAttempt, loadCheckoutAttempt, clearCheckoutAttempt } = checkout;
describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
it('round-trips a fresh attempt', () => {
@@ -71,12 +66,10 @@ describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
productId: 'pdt_X',
referralCode: 'abc',
startedAt: Date.now(),
origin: 'dashboard',
});
const loaded = loadCheckoutAttempt();
assert.equal(loaded?.productId, 'pdt_X');
assert.equal(loaded?.referralCode, 'abc');
assert.equal(loaded?.origin, 'dashboard');
});
it('returns null when nothing stored', () => {
@@ -91,7 +84,7 @@ describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
it('returns null for stored records missing productId', () => {
_sessionStorage.setItem(
LAST_CHECKOUT_ATTEMPT_KEY,
JSON.stringify({ startedAt: Date.now(), origin: 'dashboard' }),
JSON.stringify({ startedAt: Date.now() }),
);
assert.equal(loadCheckoutAttempt(), null);
});
@@ -101,7 +94,6 @@ describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
saveCheckoutAttempt({
productId: 'pdt_X',
startedAt: twentyFiveHoursAgo,
origin: 'dashboard',
});
assert.equal(loadCheckoutAttempt(), null);
});
@@ -111,7 +103,6 @@ describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
saveCheckoutAttempt({
productId: 'pdt_X',
startedAt: twentyThreeHoursAgo,
origin: 'pro',
});
assert.equal(loadCheckoutAttempt()?.productId, 'pdt_X');
});
@@ -119,18 +110,16 @@ describe('saveCheckoutAttempt / loadCheckoutAttempt', () => {
describe('clearCheckoutAttempt', () => {
it('clears the stored record regardless of reason', () => {
const reasons: Array<'success' | 'duplicate' | 'signout' | 'dismissed' | 'abandoned'> = [
const reasons: Array<'success' | 'duplicate' | 'signout' | 'dismissed'> = [
'success',
'duplicate',
'signout',
'dismissed',
'abandoned',
];
for (const reason of reasons) {
saveCheckoutAttempt({
productId: 'pdt_X',
startedAt: Date.now(),
origin: 'dashboard',
});
clearCheckoutAttempt(reason);
assert.equal(loadCheckoutAttempt(), null, `reason=${reason} should clear the record`);
@@ -141,50 +130,3 @@ describe('clearCheckoutAttempt', () => {
assert.doesNotThrow(() => clearCheckoutAttempt('success'));
});
});
describe('sweepAbandonedCheckoutAttempt', () => {
it('does not clear when return params are present (failed-redirect race guard)', () => {
const oldAttempt = {
productId: 'pdt_X',
startedAt: Date.now() - 40 * 60 * 1000, // 40min old, past abandon cutoff
origin: 'dashboard' as const,
};
saveCheckoutAttempt(oldAttempt);
// hasReturnParams = true means the page carries ?status=failed (or
// similar) — we must NOT clear because the failure banner is about
// to consume the attempt record to populate retry.
sweepAbandonedCheckoutAttempt(true);
assert.equal(loadCheckoutAttempt()?.productId, 'pdt_X');
});
it('clears records older than 30min when no return params', () => {
saveCheckoutAttempt({
productId: 'pdt_X',
startedAt: Date.now() - 45 * 60 * 1000,
origin: 'dashboard',
});
sweepAbandonedCheckoutAttempt(false);
assert.equal(loadCheckoutAttempt(), null);
});
it('preserves records younger than 30min when no return params', () => {
saveCheckoutAttempt({
productId: 'pdt_X',
startedAt: Date.now() - 5 * 60 * 1000,
origin: 'dashboard',
});
sweepAbandonedCheckoutAttempt(false);
assert.equal(loadCheckoutAttempt()?.productId, 'pdt_X');
});
it('clears malformed records defensively', () => {
_sessionStorage.setItem(LAST_CHECKOUT_ATTEMPT_KEY, '{not json');
sweepAbandonedCheckoutAttempt(false);
assert.equal(_sessionStorage.getItem(LAST_CHECKOUT_ATTEMPT_KEY), null);
});
it('is a no-op when nothing is stored', () => {
sweepAbandonedCheckoutAttempt(false);
assert.equal(loadCheckoutAttempt(), null);
});
});

View File

@@ -1,30 +1,17 @@
/**
* Unit tests for the banner state helpers. DOM-level transitions
* (pending → active → timeout) are async + event-driven and are
* covered by manual verification per the plan; this locks the pure
* decision we CAN exercise in plain TS so future refactors don't
* silently flip the mount-time behavior.
* Unit tests for the banner timing constants. DOM-level state transitions
* (pending → active → timeout) are async + event-driven and are covered
* by manual verification.
*/
import { describe, it } from 'node:test';
import assert from 'node:assert/strict';
import {
computeInitialBannerState,
EXTENDED_UNLOCK_TIMEOUT_MS,
CLASSIC_AUTO_DISMISS_MS,
} from '../src/services/checkout-banner-state.ts';
describe('computeInitialBannerState', () => {
it('returns "pending" when the user is not yet entitled', () => {
assert.equal(computeInitialBannerState(false), 'pending');
});
it('returns "active" when the user is already entitled at mount time', () => {
assert.equal(computeInitialBannerState(true), 'active');
});
});
describe('banner timing constants', () => {
it('EXTENDED_UNLOCK_TIMEOUT_MS is 30s — covers webhook/propagation long tail', () => {
assert.equal(EXTENDED_UNLOCK_TIMEOUT_MS, 30_000);