From 2067bebf50f46f13e10e2761c9d0ba2b3e0247d8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 31 May 2026 02:58:24 -0700 Subject: [PATCH] perf: clean up mobile pairing attempts (#4058) --- mobile/app/pair-confirm.tsx | 49 ++++++++++++++----- mobile/app/pair-scan.tsx | 49 ++++++++++++++----- .../pairing-connection-attempt.test.ts | 41 ++++++++++++++++ .../transport/pairing-connection-attempt.ts | 46 +++++++++++++++++ 4 files changed, 163 insertions(+), 22 deletions(-) create mode 100644 mobile/src/transport/pairing-connection-attempt.test.ts create mode 100644 mobile/src/transport/pairing-connection-attempt.ts diff --git a/mobile/app/pair-confirm.tsx b/mobile/app/pair-confirm.tsx index 78e9627a9..6e28b2fd4 100644 --- a/mobile/app/pair-confirm.tsx +++ b/mobile/app/pair-confirm.tsx @@ -1,9 +1,13 @@ -import { useCallback, useRef, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { View, Text, StyleSheet, Pressable, ActivityIndicator, BackHandler } from 'react-native' import { useSafeAreaInsets } from 'react-native-safe-area-context' import { useFocusEffect, useLocalSearchParams, useRouter } from 'expo-router' import { ChevronLeft } from 'lucide-react-native' import { resolvePairConfirmRouteState } from '../src/transport/pair-confirm-state' +import { + startPairingConnectionAttempt, + type PairingConnectionAttempt +} from '../src/transport/pairing-connection-attempt' import { connect } from '../src/transport/rpc-client' import { saveHost, getNextHostName } from '../src/transport/host-store' import type { ConnectionLogEntry, RpcResponse } from '../src/transport/types' @@ -30,6 +34,8 @@ export default function PairConfirmScreen() { // over the initial state setter) always sees the freshest list and we // batch fewer setState calls when entries arrive in bursts. const logsRef = useRef([]) + const mountedRef = useRef(true) + const activePairingAttemptRef = useRef(null) const routeState = resolvePairConfirmRouteState(params.code) const offer = routeState.offer @@ -54,35 +60,54 @@ export default function PairConfirmScreen() { }, [cancel]) ) + useEffect(() => { + return () => { + mountedRef.current = false + activePairingAttemptRef.current?.dispose() + activePairingAttemptRef.current = null + } + }, []) + async function confirm() { if (!offer) return setStatus('connecting') logsRef.current = [] setLogs([]) let client: ReturnType | null = null + activePairingAttemptRef.current?.dispose() // Why: split the try/catch around the network call vs the local save // so a Keychain or AsyncStorage failure doesn't masquerade as a // "Cannot connect" error. let response: RpcResponse - let timedOut = false - const overallTimer = setTimeout(() => { - timedOut = true - client?.close() - }, PAIRING_OVERALL_TIMEOUT_MS) + const attempt = startPairingConnectionAttempt({ + timeoutMs: PAIRING_OVERALL_TIMEOUT_MS, + closeClient: () => client?.close() + }) + activePairingAttemptRef.current = attempt try { client = connect(offer.endpoint, offer.deviceToken, offer.publicKeyB64, { onLog: (entry) => { + if (!mountedRef.current || activePairingAttemptRef.current !== attempt) return logsRef.current = [...logsRef.current, entry] setLogs(logsRef.current) } }) response = await client.sendRequest('status.get') - clearTimeout(overallTimer) - client.close() - client = null + const attemptIsCurrent = activePairingAttemptRef.current === attempt + attempt.dispose() + if (activePairingAttemptRef.current === attempt) { + activePairingAttemptRef.current = null + } + if (!mountedRef.current || !attemptIsCurrent) return } catch (err) { - clearTimeout(overallTimer) + const timedOut = attempt.timedOut + const attemptIsCurrent = activePairingAttemptRef.current === attempt + attempt.dispose() + if (activePairingAttemptRef.current === attempt) { + activePairingAttemptRef.current = null + } + if (!mountedRef.current || !attemptIsCurrent) return console.warn('[pair-confirm] connect failed', err) setStatus('error') setErrorMessage( @@ -90,11 +115,11 @@ export default function PairConfirmScreen() { ? `Couldn't connect within ${PAIRING_OVERALL_TIMEOUT_MS / 1000}s — see log below for where it stalled` : 'Cannot connect — check that your computer is on the same network' ) - client?.close() return } if (!response.ok) { + if (!mountedRef.current) return setStatus('error') setErrorMessage( response.error.code === 'unauthorized' @@ -115,8 +140,10 @@ export default function PairConfirmScreen() { publicKeyB64: offer.publicKeyB64, lastConnected: Date.now() }) + if (!mountedRef.current) return router.replace(`/h/${hostId}`) } catch (err) { + if (!mountedRef.current) return console.warn('[pair-confirm] save failed', err) setStatus('error') setErrorMessage( diff --git a/mobile/app/pair-scan.tsx b/mobile/app/pair-scan.tsx index b64ea284a..84dfdbabe 100644 --- a/mobile/app/pair-scan.tsx +++ b/mobile/app/pair-scan.tsx @@ -1,10 +1,14 @@ -import { useState, useRef, useCallback } from 'react' +import { useState, useRef, useCallback, useEffect } from 'react' import { View, Text, StyleSheet, Pressable, ActivityIndicator, Linking } from 'react-native' import { useSafeAreaInsets } from 'react-native-safe-area-context' import { CameraView, useCameraPermissions } from 'expo-camera' import { useRouter } from 'expo-router' import { ChevronLeft, Clipboard as ClipboardIcon, QrCode } from 'lucide-react-native' import { decodePairingUrl, parsePairingCode } from '../src/transport/pairing' +import { + startPairingConnectionAttempt, + type PairingConnectionAttempt +} from '../src/transport/pairing-connection-attempt' import { connect } from '../src/transport/rpc-client' import { saveHost, getNextHostName } from '../src/transport/host-store' import type { ConnectionLogEntry, PairingOffer, RpcResponse } from '../src/transport/types' @@ -38,6 +42,16 @@ export default function PairScanScreen() { const [logs, setLogs] = useState([]) const logsRef = useRef([]) const processingRef = useRef(false) + const mountedRef = useRef(true) + const activePairingAttemptRef = useRef(null) + + useEffect(() => { + return () => { + mountedRef.current = false + activePairingAttemptRef.current?.dispose() + activePairingAttemptRef.current = null + } + }, []) const handleBarCodeScanned = useCallback( ({ data }: { data: string }) => { @@ -78,30 +92,41 @@ export default function PairScanScreen() { logsRef.current = [] setLogs([]) let client: ReturnType | null = null + activePairingAttemptRef.current?.dispose() // Why: split the try/catch around the network call vs the local save // so a Keychain or AsyncStorage failure doesn't masquerade as a // "Cannot connect — same network?" error. Pairing reached the // desktop fine; the failure is local persistence. let response: RpcResponse - let timedOut = false - const overallTimer = setTimeout(() => { - timedOut = true - client?.close() - }, PAIRING_OVERALL_TIMEOUT_MS) + const attempt = startPairingConnectionAttempt({ + timeoutMs: PAIRING_OVERALL_TIMEOUT_MS, + closeClient: () => client?.close() + }) + activePairingAttemptRef.current = attempt try { client = connect(offer.endpoint, offer.deviceToken, offer.publicKeyB64, { onLog: (entry) => { + if (!mountedRef.current || activePairingAttemptRef.current !== attempt) return logsRef.current = [...logsRef.current, entry] setLogs(logsRef.current) } }) response = await client.sendRequest('status.get') - clearTimeout(overallTimer) - client.close() - client = null + const attemptIsCurrent = activePairingAttemptRef.current === attempt + attempt.dispose() + if (activePairingAttemptRef.current === attempt) { + activePairingAttemptRef.current = null + } + if (!mountedRef.current || !attemptIsCurrent) return } catch (err) { - clearTimeout(overallTimer) + const timedOut = attempt.timedOut + const attemptIsCurrent = activePairingAttemptRef.current === attempt + attempt.dispose() + if (activePairingAttemptRef.current === attempt) { + activePairingAttemptRef.current = null + } + if (!mountedRef.current || !attemptIsCurrent) return console.warn('[pair] connect failed', err) setStatus('error') setErrorMessage( @@ -110,11 +135,11 @@ export default function PairScanScreen() { : 'Cannot connect — check that your computer is on the same network' ) processingRef.current = false - client?.close() return } if (!response.ok) { + if (!mountedRef.current) return if (response.error.code === 'unauthorized') { setStatus('error') setErrorMessage('Authentication failed — token may be expired') @@ -138,8 +163,10 @@ export default function PairScanScreen() { publicKeyB64: offer.publicKeyB64, lastConnected: Date.now() }) + if (!mountedRef.current) return router.replace(`/h/${hostId}`) } catch (err) { + if (!mountedRef.current) return console.warn('[pair] save failed', err) setStatus('error') setErrorMessage( diff --git a/mobile/src/transport/pairing-connection-attempt.test.ts b/mobile/src/transport/pairing-connection-attempt.test.ts new file mode 100644 index 000000000..b05867eb7 --- /dev/null +++ b/mobile/src/transport/pairing-connection-attempt.test.ts @@ -0,0 +1,41 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { startPairingConnectionAttempt } from './pairing-connection-attempt' + +describe('pairing connection attempt cleanup', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('closes the temporary client when the overall pairing timeout fires', () => { + vi.useFakeTimers() + const closeClient = vi.fn() + + const attempt = startPairingConnectionAttempt({ timeoutMs: 25_000, closeClient }) + + expect(attempt.timedOut).toBe(false) + vi.advanceTimersByTime(24_999) + expect(closeClient).not.toHaveBeenCalled() + + vi.advanceTimersByTime(1) + expect(attempt.timedOut).toBe(true) + expect(closeClient).toHaveBeenCalledTimes(1) + + attempt.dispose() + expect(closeClient).toHaveBeenCalledTimes(1) + }) + + it('clears the timeout and closes the temporary client when disposed early', () => { + vi.useFakeTimers() + const closeClient = vi.fn() + + const attempt = startPairingConnectionAttempt({ timeoutMs: 25_000, closeClient }) + attempt.dispose() + + expect(attempt.timedOut).toBe(false) + expect(closeClient).toHaveBeenCalledTimes(1) + + vi.advanceTimersByTime(25_000) + expect(attempt.timedOut).toBe(false) + expect(closeClient).toHaveBeenCalledTimes(1) + }) +}) diff --git a/mobile/src/transport/pairing-connection-attempt.ts b/mobile/src/transport/pairing-connection-attempt.ts new file mode 100644 index 000000000..c2c1372ff --- /dev/null +++ b/mobile/src/transport/pairing-connection-attempt.ts @@ -0,0 +1,46 @@ +export type PairingConnectionAttempt = { + readonly timedOut: boolean + dispose: () => void +} + +export function startPairingConnectionAttempt({ + timeoutMs, + closeClient +}: { + timeoutMs: number + closeClient: () => void +}): PairingConnectionAttempt { + let disposed = false + let clientClosed = false + let timedOut = false + let timer: ReturnType | null = null + + function closeClientOnce() { + if (clientClosed) return + clientClosed = true + closeClient() + } + + function dispose() { + if (disposed) return + disposed = true + if (timer) { + clearTimeout(timer) + timer = null + } + closeClientOnce() + } + + timer = setTimeout(() => { + timer = null + timedOut = true + dispose() + }, timeoutMs) + + return { + get timedOut() { + return timedOut + }, + dispose + } +}