From 7e214dec91dfb4f2bebb1dd641dc7a044852b6bd Mon Sep 17 00:00:00 2001 From: Tom-Achache Date: Thu, 14 May 2026 14:57:09 -0400 Subject: [PATCH 1/4] Skip duplicate handoff exchange under React Strict Mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bootstrap effect in useManagedAuthSession calls exchangeHandoffCode on mount with no "already started" guard. Under React 18+ Strict Mode, effects run mount → cleanup → mount in dev: the first mount consumes the handoff code server-side, and the second mount re-fires the exchange with an already-consumed code. The component lands in the error state and fires onError("Failed to start session"), even though auth would have worked fine. The existing ``cancelled`` flag only stops React state updates; it can't unsend the HTTP request that consumed the code on the first mount. Add a useRef guard keyed on (sessionId, handoffCode) — same pattern as the existing callbackFiredRef — so the second mount short-circuits while a genuine prop change still re-runs the effect. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/fix-strict-mode-double-exchange.md | 5 +++++ .../src/session/useManagedAuthSession.ts | 17 +++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 .changeset/fix-strict-mode-double-exchange.md diff --git a/.changeset/fix-strict-mode-double-exchange.md b/.changeset/fix-strict-mode-double-exchange.md new file mode 100644 index 0000000..0472c9a --- /dev/null +++ b/.changeset/fix-strict-mode-double-exchange.md @@ -0,0 +1,5 @@ +--- +"@onkernel/managed-auth-react": patch +--- + +Guard the bootstrap effect in `useManagedAuthSession` against React 18+ Strict Mode's mount → cleanup → mount double-invocation. Without the guard, the second mount re-fires `exchangeHandoffCode` with a now-consumed handoff code and the component lands in the error state ("Failed to start session") even when auth would have worked. Tracked per `(sessionId, handoffCode)` so a genuine prop change still triggers a fresh exchange. diff --git a/packages/managed-auth-react/src/session/useManagedAuthSession.ts b/packages/managed-auth-react/src/session/useManagedAuthSession.ts index 515db26..dd36cdc 100644 --- a/packages/managed-auth-react/src/session/useManagedAuthSession.ts +++ b/packages/managed-auth-react/src/session/useManagedAuthSession.ts @@ -86,6 +86,13 @@ export function useManagedAuthSession( success: false, error: false, }); + // Guards against React 18+ Strict Mode's dev-only mount → cleanup → mount + // double-invocation. The handoff code is one-shot on the server, so the + // second mount's exchange call would always 4xx and surface as + // "Failed to start session" even when auth would have worked fine. + // Stores the (sessionId, handoffCode) pair so a genuine prop change still + // triggers a fresh exchange; only repeats of the same pair are skipped. + const exchangedKeyRef = useRef(null); const stopPolling = useCallback(() => { if (pollDelayRef.current) { @@ -167,6 +174,16 @@ export function useManagedAuthSession( ); useEffect(() => { + // Strict-mode-safe one-shot init: skip the duplicate mount that would + // re-exchange an already-consumed handoff code. ``cancelled`` alone + // can't help here — it stops the second render's setState calls but + // can't unsend the HTTP request that consumed the code on the first + // mount. Same idea as ``callbackFiredRef`` above, scoped per + // (sessionId, handoffCode) so a real prop change still re-runs. + const exchangeKey = `${sessionId}::${handoffCode}`; + if (exchangedKeyRef.current === exchangeKey) return; + exchangedKeyRef.current = exchangeKey; + let cancelled = false; (async () => { try { From 90c173b77c4e8b5a7de33e84efe748f6ac10fdc8 Mon Sep 17 00:00:00 2001 From: Tom-Achache Date: Thu, 14 May 2026 15:08:23 -0400 Subject: [PATCH 2/4] Adopt in-flight exchange via ref identity instead of cancelled flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor flagged the original guard: under Strict Mode's mount → cleanup → mount cycle, the cleanup flipped the closure-local ``cancelled`` to true while the second mount short-circuited at the new ref guard. The first mount's in-flight exchange would then resolve, read ``cancelled === true``, and skip its own ``setJwt`` call — leaving the component silently stuck with jwt === null after the server had already consumed the handoff code. That's strictly worse than the original bug (visible error → invisible dead state). Replace the closure-local flag with a ref-identity check. Each distinct (sessionId, handoffCode) gets a fresh ``{ key }`` object on ``exchangeRef``; the in-flight async reads the ref and bails iff the ref has been *replaced* (genuine prop change). A Strict Mode re-mount sees the same key and returns early without touching the ref — so when the first mount's async resolves, ``exchangeRef.current === ref`` still holds and the JWT is committed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/session/useManagedAuthSession.ts | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/managed-auth-react/src/session/useManagedAuthSession.ts b/packages/managed-auth-react/src/session/useManagedAuthSession.ts index dd36cdc..a045862 100644 --- a/packages/managed-auth-react/src/session/useManagedAuthSession.ts +++ b/packages/managed-auth-react/src/session/useManagedAuthSession.ts @@ -86,13 +86,14 @@ export function useManagedAuthSession( success: false, error: false, }); - // Guards against React 18+ Strict Mode's dev-only mount → cleanup → mount - // double-invocation. The handoff code is one-shot on the server, so the - // second mount's exchange call would always 4xx and surface as - // "Failed to start session" even when auth would have worked fine. - // Stores the (sessionId, handoffCode) pair so a genuine prop change still - // triggers a fresh exchange; only repeats of the same pair are skipped. - const exchangedKeyRef = useRef(null); + // Tracks the in-flight (or completed) bootstrap exchange so the second + // mount of a React 18+ Strict Mode mount → cleanup → mount cycle can + // adopt the result of the first mount's exchange instead of refiring it + // with a now-consumed handoff code. Keyed by ``{ key }`` (not the raw + // string) so the *identity* of the object identifies a single exchange: + // a real prop change replaces the object, naturally invalidating the + // previous async's setState calls via the staleness check below. + const exchangeRef = useRef<{ key: string } | null>(null); const stopPolling = useCallback(() => { if (pollDelayRef.current) { @@ -174,17 +175,27 @@ export function useManagedAuthSession( ); useEffect(() => { - // Strict-mode-safe one-shot init: skip the duplicate mount that would - // re-exchange an already-consumed handoff code. ``cancelled`` alone - // can't help here — it stops the second render's setState calls but - // can't unsend the HTTP request that consumed the code on the first - // mount. Same idea as ``callbackFiredRef`` above, scoped per - // (sessionId, handoffCode) so a real prop change still re-runs. + // Strict-Mode-safe one-shot init. Under React 18+ Strict Mode in dev, + // effects run mount → cleanup → mount. The handoff code is one-shot + // server-side, so the original code refired ``exchangeHandoffCode`` + // on the second mount and landed in the error state. + // + // A closure-local ``cancelled`` flag doesn't work as a guard either: + // the cleanup would flip it to true and the first mount's in-flight + // exchange would skip its own ``setJwt`` on resolve, leaving the + // component silently stuck with jwt === null (Cursor #10 review). + // + // Instead, store an object on a ref. The second mount sees the same + // ``key`` and short-circuits without touching the ref — so the first + // mount's async resolves, the ref-identity check passes, and the JWT + // is committed. A real (sessionId, handoffCode) change replaces the + // ref with a new object; the previous async's staleness check then + // fails by identity and its setState calls are dropped cleanly. const exchangeKey = `${sessionId}::${handoffCode}`; - if (exchangedKeyRef.current === exchangeKey) return; - exchangedKeyRef.current = exchangeKey; + if (exchangeRef.current?.key === exchangeKey) return; + const ref = { key: exchangeKey }; + exchangeRef.current = ref; - let cancelled = false; (async () => { try { const token = await exchangeHandoffCode( @@ -192,10 +203,10 @@ export function useManagedAuthSession( handoffCode, options, ); - if (cancelled) return; + if (exchangeRef.current !== ref) return; setJwt(token); const initial = await retrieveManagedAuth(sessionId, token, options); - if (cancelled) return; + if (exchangeRef.current !== ref) return; setState(initial); const derived = deriveUIState(initial); if ( @@ -230,7 +241,7 @@ export function useManagedAuthSession( setUIState("prime"); } } catch (err) { - if (cancelled) return; + if (exchangeRef.current !== ref) return; const message = err instanceof Error ? err.message : "Failed to start session"; setInitError(message); @@ -242,7 +253,6 @@ export function useManagedAuthSession( } })(); return () => { - cancelled = true; stopPolling(); }; // eslint-disable-next-line react-hooks/exhaustive-deps From 10ada60c373173a6e84e5a0a441849819d9b2b57 Mon Sep 17 00:00:00 2001 From: Tom-Achache Date: Thu, 14 May 2026 15:14:22 -0400 Subject: [PATCH 3/4] Track active flag on exchangeRef + always return cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor flagged two correct bugs in 90c173b: 1. The short-circuit ``return;`` on the Strict Mode second mount returned undefined, replacing the first mount's cleanup — so when the component actually unmounted later, ``stopPolling`` never ran and any started interval/timeout leaked. 2. Without the closure-local ``cancelled`` flag, a real unmount no longer signaled the in-flight async to stop. The ref-identity check only caught a *prop change*, not unmount, so the async kept firing ``setState``/``startPolling`` against a dead component. Both fix together by adding ``active: boolean`` to the ref object. Cleanup flips it false; the matching-key remount flips it back true. The async checks both ``exchangeRef.current !== ref`` (stale identity) and ``!ref.active`` (real unmount) before each ``setState``. And the short-circuit path now returns the same cleanup function so React preserves it across the synthetic unmount/remount. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/session/useManagedAuthSession.ts | 71 ++++++++++++------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/packages/managed-auth-react/src/session/useManagedAuthSession.ts b/packages/managed-auth-react/src/session/useManagedAuthSession.ts index a045862..973133e 100644 --- a/packages/managed-auth-react/src/session/useManagedAuthSession.ts +++ b/packages/managed-auth-react/src/session/useManagedAuthSession.ts @@ -88,12 +88,18 @@ export function useManagedAuthSession( }); // Tracks the in-flight (or completed) bootstrap exchange so the second // mount of a React 18+ Strict Mode mount → cleanup → mount cycle can - // adopt the result of the first mount's exchange instead of refiring it - // with a now-consumed handoff code. Keyed by ``{ key }`` (not the raw - // string) so the *identity* of the object identifies a single exchange: - // a real prop change replaces the object, naturally invalidating the - // previous async's setState calls via the staleness check below. - const exchangeRef = useRef<{ key: string } | null>(null); + // adopt the result of the first mount's exchange instead of refiring + // it with a now-consumed handoff code. + // + // ``key`` identifies *which* (sessionId, handoffCode) exchange this is + // (so a real prop change replaces the object and stales the previous + // async by identity). ``active`` is flipped to false by cleanup and + // back to true by the matching-key remount — so the in-flight async + // can distinguish a Strict Mode synthetic unmount/remount (active goes + // false then true again before resolve) from a real unmount (stays + // false), and stop mid-flight calls or post-unmount ``startPolling`` + // from acting on a dead component. + const exchangeRef = useRef<{ key: string; active: boolean } | null>(null); const stopPolling = useCallback(() => { if (pollDelayRef.current) { @@ -178,22 +184,37 @@ export function useManagedAuthSession( // Strict-Mode-safe one-shot init. Under React 18+ Strict Mode in dev, // effects run mount → cleanup → mount. The handoff code is one-shot // server-side, so the original code refired ``exchangeHandoffCode`` - // on the second mount and landed in the error state. + // on the second mount and landed in the error state. The fix has + // three moving parts (Cursor #10 review iterations): // - // A closure-local ``cancelled`` flag doesn't work as a guard either: - // the cleanup would flip it to true and the first mount's in-flight - // exchange would skip its own ``setJwt`` on resolve, leaving the - // component silently stuck with jwt === null (Cursor #10 review). - // - // Instead, store an object on a ref. The second mount sees the same - // ``key`` and short-circuits without touching the ref — so the first - // mount's async resolves, the ref-identity check passes, and the JWT - // is committed. A real (sessionId, handoffCode) change replaces the - // ref with a new object; the previous async's staleness check then - // fails by identity and its setState calls are dropped cleanly. + // 1. Guard the exchange by ref identity, not a closure-local + // ``cancelled`` flag — a closure flag set by the synthetic + // cleanup would orphan the first mount's in-flight result. + // 2. Track an ``active`` flag on the ref so the async can also + // distinguish a real unmount (active stays false) from a + // Strict Mode unmount/remount (active flips false → true + // synchronously before the async resolves). + // 3. Always return the cleanup, even on the short-circuit path — + // React only keeps the most recent effect's cleanup, so a bare + // ``return`` from the second mount would orphan ``stopPolling`` + // and leak the interval at real unmount. const exchangeKey = `${sessionId}::${handoffCode}`; - if (exchangeRef.current?.key === exchangeKey) return; - const ref = { key: exchangeKey }; + const cleanup = () => { + if (exchangeRef.current?.key === exchangeKey) { + exchangeRef.current.active = false; + } + stopPolling(); + }; + + if (exchangeRef.current?.key === exchangeKey) { + // Strict Mode remount of the same exchange: cleanup just flipped + // active=false; flip it back so the in-flight async can commit. + // Return the cleanup so a later *real* unmount still stops polling. + exchangeRef.current.active = true; + return cleanup; + } + + const ref = { key: exchangeKey, active: true }; exchangeRef.current = ref; (async () => { @@ -203,10 +224,10 @@ export function useManagedAuthSession( handoffCode, options, ); - if (exchangeRef.current !== ref) return; + if (exchangeRef.current !== ref || !ref.active) return; setJwt(token); const initial = await retrieveManagedAuth(sessionId, token, options); - if (exchangeRef.current !== ref) return; + if (exchangeRef.current !== ref || !ref.active) return; setState(initial); const derived = deriveUIState(initial); if ( @@ -241,7 +262,7 @@ export function useManagedAuthSession( setUIState("prime"); } } catch (err) { - if (exchangeRef.current !== ref) return; + if (exchangeRef.current !== ref || !ref.active) return; const message = err instanceof Error ? err.message : "Failed to start session"; setInitError(message); @@ -252,9 +273,7 @@ export function useManagedAuthSession( } } })(); - return () => { - stopPolling(); - }; + return cleanup; // eslint-disable-next-line react-hooks/exhaustive-deps }, [sessionId, handoffCode]); From 26e1885e7335de85a704a953738fbddfe979a37f Mon Sep 17 00:00:00 2001 From: Tom-Achache Date: Thu, 14 May 2026 15:49:03 -0400 Subject: [PATCH 4/4] Tighten Strict Mode comments per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the parenthetical pointing at the review tool — process refs don't belong in published source. - Collapse the exchangeRef declaration block to one short sentence naming the fields; defer the invariants to the effect comment so the strict-mode rationale lives in exactly one place. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/session/useManagedAuthSession.ts | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/packages/managed-auth-react/src/session/useManagedAuthSession.ts b/packages/managed-auth-react/src/session/useManagedAuthSession.ts index 973133e..fe45e64 100644 --- a/packages/managed-auth-react/src/session/useManagedAuthSession.ts +++ b/packages/managed-auth-react/src/session/useManagedAuthSession.ts @@ -86,19 +86,10 @@ export function useManagedAuthSession( success: false, error: false, }); - // Tracks the in-flight (or completed) bootstrap exchange so the second - // mount of a React 18+ Strict Mode mount → cleanup → mount cycle can - // adopt the result of the first mount's exchange instead of refiring - // it with a now-consumed handoff code. - // - // ``key`` identifies *which* (sessionId, handoffCode) exchange this is - // (so a real prop change replaces the object and stales the previous - // async by identity). ``active`` is flipped to false by cleanup and - // back to true by the matching-key remount — so the in-flight async - // can distinguish a Strict Mode synthetic unmount/remount (active goes - // false then true again before resolve) from a real unmount (stays - // false), and stop mid-flight calls or post-unmount ``startPolling`` - // from acting on a dead component. + // Tracks the in-flight bootstrap exchange. ``key`` identifies which + // (sessionId, handoffCode) pair it belongs to; ``active`` is false + // between cleanup and the matching-key remount. See the effect below + // for the invariants these fields enforce. const exchangeRef = useRef<{ key: string; active: boolean } | null>(null); const stopPolling = useCallback(() => { @@ -182,15 +173,14 @@ export function useManagedAuthSession( useEffect(() => { // Strict-Mode-safe one-shot init. Under React 18+ Strict Mode in dev, - // effects run mount → cleanup → mount. The handoff code is one-shot - // server-side, so the original code refired ``exchangeHandoffCode`` - // on the second mount and landed in the error state. The fix has - // three moving parts (Cursor #10 review iterations): + // effects run mount → cleanup → mount; the handoff code is one-shot + // server-side, so a naive remount refires the exchange against an + // already-consumed code. Three invariants make this safe: // // 1. Guard the exchange by ref identity, not a closure-local // ``cancelled`` flag — a closure flag set by the synthetic // cleanup would orphan the first mount's in-flight result. - // 2. Track an ``active`` flag on the ref so the async can also + // 2. Track an ``active`` flag on the ref so the async can // distinguish a real unmount (active stays false) from a // Strict Mode unmount/remount (active flips false → true // synchronously before the async resolves).