From 8b44499a3e3261fd39e7ce66007d2d117833dbbd Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 2 Jul 2026 14:56:52 -0400 Subject: [PATCH] fix(relay-reconnect): address Wes's CHANGES_REQUESTED findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (relayReconnectController.ts): subscribeToConnectionState can invoke its listener synchronously with the current state before returning. When that sync emission signals connected, onConnected() → finish() → cancelTimers() runs while unsubscribeConnectionState is still null — then the caller assigns all three handles after finish, leaking them for the app lifetime. Fix: subscribe first, then check resolved immediately after the return value is assigned. If resolved, unsubscribe the handle (which cancelTimers couldn't reach because the assignment hadn't happened yet) and return before installing the poll interval and backstop timer. Finding 2 (ProfileStep.tsx): reconnect() always returns false in phase 3, so runConnectivityAction never calls markSuccess(), and the onboarding component had no connection-state subscription. The error card stayed showing failure even after the relay healed. Fix: observe the shared relayConnectivitySuccess store (already used by the sidebar) via useSyncExternalStore + useEffect in OnboardingRelayConnectionErrorCard. Export subscribeRelayConnectivitySuccess and getRelayConnectivitySuccessSnapshot from useSidebarRelayConnectionCard.ts so both surfaces share exactly one success-signalling mechanism. Test: add a sync-emission unit test with a subscribeToConnectionState fake that calls the listener before returning its cleanup handle — asserts onSuccess fires once, no interval/backstop installed, cleanup called. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../features/onboarding/ui/ProfileStep.tsx | 24 ++++++++++ .../ui/useSidebarRelayConnectionCard.ts | 4 +- .../api/relayReconnectController.test.mjs | 45 +++++++++++++++++++ .../shared/api/relayReconnectController.ts | 18 ++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/onboarding/ui/ProfileStep.tsx b/desktop/src/features/onboarding/ui/ProfileStep.tsx index 914bc0d6b..710bdf9c0 100644 --- a/desktop/src/features/onboarding/ui/ProfileStep.tsx +++ b/desktop/src/features/onboarding/ui/ProfileStep.tsx @@ -2,6 +2,10 @@ 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 { useReconnectRelay } from "@/shared/api/useReconnectRelay"; import { cn } from "@/shared/lib/cn"; import { isRelayUnreachableError } from "@/shared/lib/relayError"; @@ -46,6 +50,16 @@ 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) { @@ -77,6 +91,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. + React.useEffect(() => { + if (relayConnectivitySuccess) { + markSuccess(); + } + }, [relayConnectivitySuccess, markSuccess]); + const runConnectivityAction = React.useCallback( (runAction: () => Promise) => { if (reconnectActionPendingRef.current) { diff --git a/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts b/desktop/src/features/sidebar/ui/useSidebarRelayConnectionCard.ts index 3b20de321..16365c28f 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; } -function subscribeRelayConnectivitySuccess(listener: () => void) { +export function subscribeRelayConnectivitySuccess(listener: () => void) { relayConnectivitySuccessListeners.add(listener); return () => relayConnectivitySuccessListeners.delete(listener); } -function getRelayConnectivitySuccessSnapshot( +export function getRelayConnectivitySuccessSnapshot( relayUrl: string | null | undefined, ) { return relayConnectivitySuccessKey === relaySuccessKey(relayUrl); diff --git a/desktop/src/shared/api/relayReconnectController.test.mjs b/desktop/src/shared/api/relayReconnectController.test.mjs index 99387ef66..37b942434 100644 --- a/desktop/src/shared/api/relayReconnectController.test.mjs +++ b/desktop/src/shared/api/relayReconnectController.test.mjs @@ -469,3 +469,48 @@ test("OSS build (hookConfigured returns false) — runHook never called", async "runHook not called in OSS build", ); }); + +// ── Synchronous connected emission ─────────────────────────────────────────── + +test("sync connected emission — onSuccess fires once, no interval/backstop installed, subscription cleaned up", async () => { + const ctrl = new RelayReconnectController(); + + // subscribeToConnectionState fake that invokes the listener synchronously + // with "connected" BEFORE returning the cleanup handle. This models the + // production subscribeToConnectionState documented behaviour: it fires the + // listener with the current state before returning. + let cleanupCalled = false; + const deps = makeDeps({ + preconnectResult: async () => { + throw new Error("relay unreachable"); + }, + hookConfiguredResult: async () => false, + }); + + // Override subscribeToConnectionState with the synchronous-emission fake. + deps.subscribeToConnectionState = mock.fn((listener) => { + // Invoke immediately — simulates "already connected" at subscribe time. + listener("connected"); + // Return cleanup handle (production unsubscribe fn). + const cleanup = () => { + cleanupCalled = true; + }; + return cleanup; + }); + + await ctrl.start(deps); + + assert.equal( + deps.onSuccess.mock.calls.length, + 1, + "onSuccess fires exactly once", + ); + assert.equal(deps._intervals.length, 0, "no poll interval installed"); + assert.equal(deps._timers.length, 0, "no backstop timer installed"); + assert.equal(cleanupCalled, true, "subscription cleanup handle was called"); + assert.deepEqual( + ctrl.getState(), + { isPending: false, isWaitingOnReconnectHook: false }, + "state is idle after sync success", + ); +}); diff --git a/desktop/src/shared/api/relayReconnectController.ts b/desktop/src/shared/api/relayReconnectController.ts index ff71f387a..c11e0a03b 100644 --- a/desktop/src/shared/api/relayReconnectController.ts +++ b/desktop/src/shared/api/relayReconnectController.ts @@ -194,12 +194,30 @@ export class RelayReconnectController { this.finish(onSuccess, true); }; + // 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. this.unsubscribeConnectionState = deps.subscribeToConnectionState( (state) => { if (state === "connected") onConnected(); }, ); + // 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. + if (resolved) { + if (this.unsubscribeConnectionState !== null) { + this.unsubscribeConnectionState(); + this.unsubscribeConnectionState = null; + } + return false; + } + this.pollIntervalId = deps.setInterval(() => { if (resolved || cancelled()) return; void deps