fix(onboarding): use relay connection state for phase-3 success signal

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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

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",
+  );
+});

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<boolean | undefined>) => {
@@ -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}`);
         })

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);

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();