From cdb09e8c15accb94be18a96b299b0c5eb260eba1 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 2 Jul 2026 15:07:52 -0400 Subject: [PATCH] fix(onboarding): use relay connection state for phase-3 success signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProfileStep was observing the sidebar's relayConnectivitySuccess store to detect phase-3 reconnect success. That store is only ever produced by useSidebarRelayConnectionCard, which never mounts during onboarding — AppReady returns OnboardingFlow before the sidebar path renders. Replace the dead cross-surface coupling with a local approach: - Add useRelayConnection() to OnboardingRelayConnectionErrorCard - Add hadActiveReconnectRef to gate markSuccess() on a genuine attempt - Set the ref true when runConnectivityAction starts; clear it and call markSuccess() synchronously on phase-1 success, or leave it armed for the connection-state effect to fire on phase-3 connected transition - Remove the two exports added to useSidebarRelayConnectionCard (no dead API surface) Also fix the controller comment: the first comment incorrectly implied cancelTimers() reached the subscription handle during sync emission. In the assignment-order race, cancelTimers() ran before the handle was assigned; cleanup happens in the if (resolved) block. Say that directly. Add OnboardingRelayConnectionErrorCard.test.mjs: 5 tests covering the guard contract — phase-3 async success, disconnected no-op, no-attempt guard, phase-1 sync success idempotency, and double-connected no-op. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- ...nboardingRelayConnectionErrorCard.test.mjs | 143 ++++++++++++++++++ .../features/onboarding/ui/ProfileStep.tsx | 41 ++--- .../ui/useSidebarRelayConnectionCard.ts | 4 +- .../shared/api/relayReconnectController.ts | 14 +- 4 files changed, 173 insertions(+), 29 deletions(-) create mode 100644 desktop/src/features/onboarding/ui/OnboardingRelayConnectionErrorCard.test.mjs diff --git a/desktop/src/features/onboarding/ui/OnboardingRelayConnectionErrorCard.test.mjs b/desktop/src/features/onboarding/ui/OnboardingRelayConnectionErrorCard.test.mjs new file mode 100644 index 0000000000..6e6ddf260a --- /dev/null +++ b/desktop/src/features/onboarding/ui/OnboardingRelayConnectionErrorCard.test.mjs @@ -0,0 +1,143 @@ +/** + * Unit tests for the OnboardingRelayConnectionErrorCard phase-3 reconnect contract. + * + * Tests the guard logic that ties relay connection state to markSuccess(): + * - phase-3 path (reconnect() returns false): card marks success when relay + * transitions to "connected" while a reconnect was attempted. + * - disconnected state never triggers success. + * - no prior reconnect attempt: connected state is ignored. + * + * These tests exercise the observable contract without React rendering by + * simulating the component's ref/effect logic as a pure state machine. + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +/** + * Minimal simulation of the OnboardingRelayConnectionErrorCard guard logic. + * + * Models the three-variable contract: + * hadActiveReconnect (ref) — set true when a reconnect is started + * relayConnectionState — driven by useRelayConnection + * hasSuccess (state) — becomes true when guard fires markSuccess + */ +function makeCard() { + let hadActiveReconnect = false; + let hasSuccess = false; + let markSuccessCallCount = 0; + + const markSuccess = () => { + hasSuccess = true; + markSuccessCallCount++; + }; + + // Simulates what runConnectivityAction does: arm the ref, call reconnect, + // then either mark success synchronously (phase 1) or leave ref armed (phase 3). + const startReconnect = async (reconnectResult) => { + hadActiveReconnect = true; + const didReconnect = await reconnectResult; + if (didReconnect !== false) { + // Phase 1 sync success — clear ref and mark immediately. + hadActiveReconnect = false; + markSuccess(); + } + // Phase 3: ref stays armed, waiting for connection-state effect. + }; + + // Simulates the useEffect(() => { if (connected && ref) markSuccess() }) + const driveConnectionState = (state) => { + if (state === "connected" && hadActiveReconnect) { + hadActiveReconnect = false; + markSuccess(); + } + }; + + return { + startReconnect, + driveConnectionState, + get hasSuccess() { + return hasSuccess; + }, + get markSuccessCallCount() { + return markSuccessCallCount; + }, + }; +} + +// ── Phase 3: async success via connection-state ─────────────────────────────── + +test("phase-3 path — reconnect() returns false, connected fires, markSuccess called once", async () => { + const card = makeCard(); + + // Simulate reconnect() entering phase 3 (returns false). + await card.startReconnect(Promise.resolve(false)); + + // Controller eventually drives relay to connected. + card.driveConnectionState("connected"); + + assert.equal(card.hasSuccess, true, "card marks success on connected"); + assert.equal(card.markSuccessCallCount, 1, "markSuccess called exactly once"); +}); + +test("phase-3 path — disconnected state does not mark success", async () => { + const card = makeCard(); + + await card.startReconnect(Promise.resolve(false)); + + // Drive through several non-connected states. + card.driveConnectionState("disconnected"); + card.driveConnectionState("reconnecting"); + card.driveConnectionState("stalled"); + + assert.equal(card.hasSuccess, false, "no success on non-connected states"); + assert.equal(card.markSuccessCallCount, 0, "markSuccess never called"); +}); + +test("no reconnect attempt — connected state does not mark success", async () => { + const card = makeCard(); + + // Drive to connected without any reconnect attempt. + card.driveConnectionState("connected"); + + assert.equal( + card.hasSuccess, + false, + "pre-existing connected state does not mark success", + ); + assert.equal(card.markSuccessCallCount, 0, "markSuccess never called"); +}); + +test("phase-1 sync success — markSuccess fires immediately, ref cleared, later connected is ignored", async () => { + const card = makeCard(); + + // Phase 1: reconnect() returns true (sync success). + await card.startReconnect(Promise.resolve(true)); + + assert.equal(card.hasSuccess, true, "success marked synchronously"); + assert.equal(card.markSuccessCallCount, 1, "called once"); + + // A subsequent connected transition should NOT call markSuccess again. + card.driveConnectionState("connected"); + + assert.equal( + card.markSuccessCallCount, + 1, + "no double-success on later connected", + ); +}); + +test("phase-3 path — connected fires multiple times, markSuccess still called only once", async () => { + const card = makeCard(); + + await card.startReconnect(Promise.resolve(false)); + + card.driveConnectionState("connected"); + card.driveConnectionState("connected"); // spurious second emission + + assert.equal( + card.markSuccessCallCount, + 1, + "ref cleared after first connected, no double-success", + ); +}); diff --git a/desktop/src/features/onboarding/ui/ProfileStep.tsx b/desktop/src/features/onboarding/ui/ProfileStep.tsx index 710bdf9c09..a39193e0e7 100644 --- a/desktop/src/features/onboarding/ui/ProfileStep.tsx +++ b/desktop/src/features/onboarding/ui/ProfileStep.tsx @@ -2,10 +2,7 @@ import * as React from "react"; import { toast } from "sonner"; import { SidebarRelayConnectionCompactCard } from "@/features/sidebar/ui/SidebarRelayConnectionCard"; -import { - getRelayConnectivitySuccessSnapshot, - subscribeRelayConnectivitySuccess, -} from "@/features/sidebar/ui/useSidebarRelayConnectionCard"; +import { useRelayConnection } from "@/shared/api/useRelayConnection"; import { useReconnectRelay } from "@/shared/api/useReconnectRelay"; import { cn } from "@/shared/lib/cn"; import { isRelayUnreachableError } from "@/shared/lib/relayError"; @@ -39,6 +36,10 @@ function OnboardingRelayConnectionErrorCard({ isWaitingOnReconnectHook, reconnect, } = useReconnectRelay(); + // Track whether a reconnect attempt was ever initiated from this card so we + // don't call markSuccess() on a "connected" state that pre-dates any click. + const hadActiveReconnectRef = React.useRef(false); + const relayConnectionState = useRelayConnection(); const [dismissedErrorMessage, setDismissedErrorMessage] = React.useState< string | null >(null); @@ -50,16 +51,6 @@ function OnboardingRelayConnectionErrorCard({ const wasSavingRef = React.useRef(isSaving); const isActionPending = isReconnectActionPending || isReconnectPending; - // Observe the shared relay-connectivity-success store. When the relay - // recovers during phase 3 (controller reconnects without a click return - // value), the sidebar and this card share the same success signal so the - // onboarding card also flips to success state. - const relayConnectivitySuccess = React.useSyncExternalStore( - subscribeRelayConnectivitySuccess, - () => getRelayConnectivitySuccessSnapshot(undefined), - () => false, - ); - React.useEffect(() => { return () => { if (successTimeoutRef.current !== null) { @@ -91,15 +82,16 @@ function OnboardingRelayConnectionErrorCard({ }, ONBOARDING_CONNECTIVITY_SUCCESS_AUTO_DISMISS_MS); }, [message]); - // When the shared relay-connectivity-success store signals success (set by - // either the sidebar's handleReconnectRelay or the auto-heal path), mark - // this card as successful too. This covers the phase-3 case where - // reconnect() returns false but the relay later becomes reachable. + // Observe real relay connection state. When this card initiated a reconnect + // attempt (phase-3 path: reconnect() returns false) and the relay later + // becomes connected, mark success. Guard: only fire when we actually + // started a reconnect so a pre-existing connected state doesn't trigger it. React.useEffect(() => { - if (relayConnectivitySuccess) { + if (relayConnectionState === "connected" && hadActiveReconnectRef.current) { + hadActiveReconnectRef.current = false; markSuccess(); } - }, [relayConnectivitySuccess, markSuccess]); + }, [relayConnectionState, markSuccess]); const runConnectivityAction = React.useCallback( (runAction: () => Promise) => { @@ -107,6 +99,7 @@ function OnboardingRelayConnectionErrorCard({ return; } + hadActiveReconnectRef.current = true; reconnectActionPendingRef.current = true; setIsReconnectActionPending(true); setHasSuccess(false); @@ -114,10 +107,18 @@ function OnboardingRelayConnectionErrorCard({ .then(runAction) .then((didReconnect) => { if (didReconnect !== false) { + // Synchronous success (phase 1) — clear the ref and mark success + // immediately. The connection-state effect won't fire because the + // ref was just cleared. + hadActiveReconnectRef.current = false; markSuccess(); } + // didReconnect === false means phase 3 is active; the + // connection-state effect will call markSuccess() when the relay + // becomes connected. }) .catch((error) => { + hadActiveReconnectRef.current = false; const detail = error instanceof Error ? error.message : String(error); toast.error(`Could not reconnect to the relay. ${detail}`); }) diff --git a/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts b/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts index 16365c28f1..3b20de321a 100644 --- a/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts +++ b/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts @@ -14,12 +14,12 @@ function relaySuccessKey(relayUrl: string | null | undefined) { return relayUrl ?? DEFAULT_RELAY_SUCCESS_KEY; } -export function subscribeRelayConnectivitySuccess(listener: () => void) { +function subscribeRelayConnectivitySuccess(listener: () => void) { relayConnectivitySuccessListeners.add(listener); return () => relayConnectivitySuccessListeners.delete(listener); } -export function getRelayConnectivitySuccessSnapshot( +function getRelayConnectivitySuccessSnapshot( relayUrl: string | null | undefined, ) { return relayConnectivitySuccessKey === relaySuccessKey(relayUrl); diff --git a/desktop/src/shared/api/relayReconnectController.ts b/desktop/src/shared/api/relayReconnectController.ts index c11e0a03b8..45bba3666c 100644 --- a/desktop/src/shared/api/relayReconnectController.ts +++ b/desktop/src/shared/api/relayReconnectController.ts @@ -196,9 +196,11 @@ export class RelayReconnectController { // Subscribe FIRST. subscribeToConnectionState may invoke the listener // synchronously with the current state (documented on the production - // implementation). If that sync emission resolves us, finish() runs and - // cancelTimers() cleans up the subscription handle before we reach the - // timer-install lines below. + // implementation). If that sync emission fires onConnected(), `resolved` + // is set to true inside the call — but the return value (the cleanup + // handle) hasn't been assigned to `this.unsubscribeConnectionState` yet, + // so cancelTimers() inside finish() cannot reach it. The `if (resolved)` + // block below handles that late-assignment cleanup. this.unsubscribeConnectionState = deps.subscribeToConnectionState( (state) => { if (state === "connected") onConnected(); @@ -206,10 +208,8 @@ export class RelayReconnectController { ); // If the sync emission already finished the attempt, don't install timers - // that would live for the app lifetime. Also, cancelTimers() ran inside - // finish() before the return value of subscribeToConnectionState was - // assigned, so the subscription handle may not have been cleaned up yet — - // call unsubscribe now if it is still set. + // that would live for the app lifetime. The cleanup handle was assigned + // after cancelTimers() ran inside finish(), so unsubscribe it now. if (resolved) { if (this.unsubscribeConnectionState !== null) { this.unsubscribeConnectionState();