From 421c8a43ca6a9de93d2875d2699fd820eb555ef8 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:13:39 -0700 Subject: [PATCH] fix(mobile): stop serving a pre-write host-list snapshot to loads issued after the write removeHost/persistHost await hostListMutation, but the in-flight loadHosts() de-dupe handed back a pass that started BEFORE the write committed, so a load issued after removal repainted the removed host card (#8791). Every durable write now drops the shared pass via host-list-load-sharing.ts so the next caller reads fresh; concurrent loads with no write between them still share one Keychain pass. Also extracts the host action sheet into host-list-action-sheet-actions.ts to pin closeBeforePress on Edit host + Remove (the freeze half of #8791, already fixed by #8536). --- mobile/app/index.tsx | 78 +++----------- .../host-list-action-sheet-actions.test.ts | 101 ++++++++++++++++++ mobile/src/host-list-action-sheet-actions.ts | 68 ++++++++++++ .../transport/host-list-load-sharing.test.ts | 62 +++++++++++ .../src/transport/host-list-load-sharing.ts | 29 +++++ mobile/src/transport/host-store.test.ts | 61 +++++++++++ mobile/src/transport/host-store.ts | 16 ++- 7 files changed, 343 insertions(+), 72 deletions(-) create mode 100644 mobile/src/host-list-action-sheet-actions.test.ts create mode 100644 mobile/src/host-list-action-sheet-actions.ts create mode 100644 mobile/src/transport/host-list-load-sharing.test.ts create mode 100644 mobile/src/transport/host-list-load-sharing.ts diff --git a/mobile/app/index.tsx b/mobile/app/index.tsx index f486fe99c78..ad9574632d4 100644 --- a/mobile/app/index.tsx +++ b/mobile/app/index.tsx @@ -2,17 +2,7 @@ import { useState, useCallback, useEffect, useMemo, useRef } from 'react' import { View, Text, StyleSheet, Pressable, FlatList, Alert } from 'react-native' import { SafeAreaView, useSafeAreaInsets } from 'react-native-safe-area-context' import { useRouter, useFocusEffect } from 'expo-router' -import { - QrCode, - Settings, - ChevronRight, - Terminal, - Plus, - RefreshCw, - PowerOff, - Edit3, - ListTodo -} from 'lucide-react-native' +import { QrCode, Settings, ChevronRight, Terminal, Plus, ListTodo } from 'lucide-react-native' import { ClaudeIcon, OpenAIIcon } from '../src/components/AgentIcons' import { type AccountsSnapshot, @@ -47,7 +37,8 @@ import { triggerMediumImpact } from '../src/platform/haptics' import { OrcaLogo } from '../src/components/OrcaLogo' import { MobileHostCard } from '../src/components/MobileHostCard' import { TaskProviderLogo } from '../src/components/TaskProviderLogo' -import { ActionSheetModal, type ActionSheetAction } from '../src/components/ActionSheetModal' +import { ActionSheetModal } from '../src/components/ActionSheetModal' +import { getHostListActionSheetActions } from '../src/host-list-action-sheet-actions' import { ConfirmModal } from '../src/components/ConfirmModal' import { setCachedWorktrees, getCachedWorktrees } from '../src/cache/worktree-cache' import { loadHomeSnapshot, saveHomeSnapshot } from '../src/cache/home-snapshot-cache' @@ -972,57 +963,18 @@ export default function HomeScreen() { visible={actionTarget != null} title={actionTarget?.name} message={actionTarget ? endpointLabel(actionTarget.endpoint) : undefined} - actions={(() => { - const host = actionTarget - if (!host) { - return [] - } - const state = hostStates[host.id] ?? 'connecting' - const isLive = - state === 'connected' || - state === 'connecting' || - state === 'handshaking' || - state === 'reconnecting' - // Why: label "Connect" (not "Reconnect") when never connected this session, so the verb matches the action. - const hasEverConnected = (hostLastConnected[host.id] ?? null) != null - const items: ActionSheetAction[] = [] - items.push({ - label: hasEverConnected && isLive ? 'Reconnect' : 'Connect', - icon: RefreshCw, - onPress: () => { - setActionTarget(null) - void forceReconnectHost(host.id) - } - }) - if (isLive) { - items.push({ - label: 'Disconnect', - icon: PowerOff, - onPress: () => { - setActionTarget(null) - closeHostClient(host.id) - } - }) - } - items.push({ - label: 'Edit host', - icon: Edit3, - closeBeforePress: true, - onPress: () => { - setActionTarget(null) - router.push(`/h/${host.id}/edit`) - } - }) - items.push({ - label: 'Remove', - destructive: true, - closeBeforePress: true, - onPress: () => { - setConfirmRemove(host) - } - }) - return items - })()} + actions={getHostListActionSheetActions({ + host: actionTarget, + state: actionTarget ? (hostStates[actionTarget.id] ?? 'connecting') : 'disconnected', + hasEverConnected: actionTarget + ? (hostLastConnected[actionTarget.id] ?? null) != null + : false, + onDismiss: () => setActionTarget(null), + onReconnect: (hostId) => void forceReconnectHost(hostId), + onDisconnect: closeHostClient, + onEdit: (hostId) => router.push(`/h/${hostId}/edit`), + onRemove: setConfirmRemove + })} onClose={() => setActionTarget(null)} /> diff --git a/mobile/src/host-list-action-sheet-actions.test.ts b/mobile/src/host-list-action-sheet-actions.test.ts new file mode 100644 index 00000000000..2fb90334082 --- /dev/null +++ b/mobile/src/host-list-action-sheet-actions.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it, vi } from 'vitest' +import { getHostListActionSheetActions } from './host-list-action-sheet-actions' +import type { ConnectionState, HostProfile } from './transport/types' + +vi.mock('lucide-react-native', () => ({ + Edit3: vi.fn(), + PowerOff: vi.fn(), + RefreshCw: vi.fn() +})) + +const HOST: HostProfile = { + id: 'host-1', + name: 'Host 1', + endpoint: 'ws://192.168.21.4:6768', + deviceToken: 'token', + publicKeyB64: 'key', + lastConnected: 0 +} + +function build(overrides: { state?: ConnectionState; hasEverConnected?: boolean } = {}) { + const spies = { + onDismiss: vi.fn(), + onReconnect: vi.fn(), + onDisconnect: vi.fn(), + onEdit: vi.fn(), + onRemove: vi.fn() + } + const actions = getHostListActionSheetActions({ + host: HOST, + state: overrides.state ?? 'connected', + hasEverConnected: overrides.hasEverConnected ?? true, + ...spies + }) + return { actions, spies } +} + +describe('getHostListActionSheetActions', () => { + // Why: both open a second drawer. Presenting one while this sheet's native + // Modal is still up freezes the whole screen on iOS — issue #8791. + it.each(['Edit host', 'Remove'])('defers %s until the action sheet has closed', (label) => { + const { actions } = build() + expect(actions.find((action) => action.label === label)).toMatchObject({ + closeBeforePress: true + }) + }) + + it('leaves the in-place actions undeferred so they fire on tap', () => { + const { actions, spies } = build() + const disconnect = actions.find((action) => action.label === 'Disconnect') + expect(disconnect?.closeBeforePress).toBeUndefined() + disconnect?.onPress() + expect(spies.onDisconnect).toHaveBeenCalledWith(HOST.id) + expect(spies.onDismiss).toHaveBeenCalled() + }) + + it('hands Remove the whole host so the confirm sheet can name it', () => { + const { actions, spies } = build() + actions.find((action) => action.label === 'Remove')?.onPress() + expect(spies.onRemove).toHaveBeenCalledWith(HOST) + }) + + it('routes Edit host to the edit screen', () => { + const { actions, spies } = build() + actions.find((action) => action.label === 'Edit host')?.onPress() + expect(spies.onEdit).toHaveBeenCalledWith(HOST.id) + }) + + it('offers Disconnect only while the socket is live', () => { + expect(build({ state: 'reconnecting' }).actions.map((action) => action.label)).toEqual([ + 'Reconnect', + 'Disconnect', + 'Edit host', + 'Remove' + ]) + expect(build({ state: 'disconnected' }).actions.map((action) => action.label)).toEqual([ + 'Connect', + 'Edit host', + 'Remove' + ]) + }) + + it('says Connect until the host has connected at least once this session', () => { + expect(build({ hasEverConnected: false }).actions[0]?.label).toBe('Connect') + expect(build({ hasEverConnected: true }).actions[0]?.label).toBe('Reconnect') + }) + + it('renders nothing without a target host', () => { + expect( + getHostListActionSheetActions({ + host: null, + state: 'disconnected', + hasEverConnected: false, + onDismiss: vi.fn(), + onReconnect: vi.fn(), + onDisconnect: vi.fn(), + onEdit: vi.fn(), + onRemove: vi.fn() + }) + ).toEqual([]) + }) +}) diff --git a/mobile/src/host-list-action-sheet-actions.ts b/mobile/src/host-list-action-sheet-actions.ts new file mode 100644 index 00000000000..b045db7eea4 --- /dev/null +++ b/mobile/src/host-list-action-sheet-actions.ts @@ -0,0 +1,68 @@ +import { Edit3, PowerOff, RefreshCw } from 'lucide-react-native' +import type { ActionSheetAction } from './components/ActionSheetModal' +import type { ConnectionState, HostProfile } from './transport/types' + +/** Builds the home-screen host long-press menu. Edit and Remove open a second + * drawer, so both must defer until this sheet's native Modal has unmounted — + * presenting into a live one freezes the whole screen on iOS (issue #8791). */ +export function getHostListActionSheetActions(args: { + host: HostProfile | null + state: ConnectionState + /** Label "Connect" (not "Reconnect") when never connected this session, so the verb matches the action. */ + hasEverConnected: boolean + onDismiss: () => void + onReconnect: (hostId: string) => void + onDisconnect: (hostId: string) => void + onEdit: (hostId: string) => void + onRemove: (host: HostProfile) => void +}): ActionSheetAction[] { + const { host } = args + if (!host) { + return [] + } + const isLive = + args.state === 'connected' || + args.state === 'connecting' || + args.state === 'handshaking' || + args.state === 'reconnecting' + + return [ + { + label: args.hasEverConnected && isLive ? 'Reconnect' : 'Connect', + icon: RefreshCw, + onPress: () => { + args.onDismiss() + args.onReconnect(host.id) + } + }, + ...(isLive + ? [ + { + label: 'Disconnect', + icon: PowerOff, + onPress: () => { + args.onDismiss() + args.onDisconnect(host.id) + } + } + ] + : []), + { + label: 'Edit host', + icon: Edit3, + closeBeforePress: true, + onPress: () => { + args.onDismiss() + args.onEdit(host.id) + } + }, + { + label: 'Remove', + destructive: true, + closeBeforePress: true, + onPress: () => { + args.onRemove(host) + } + } + ] +} diff --git a/mobile/src/transport/host-list-load-sharing.test.ts b/mobile/src/transport/host-list-load-sharing.test.ts new file mode 100644 index 00000000000..9d2f10e8b8d --- /dev/null +++ b/mobile/src/transport/host-list-load-sharing.test.ts @@ -0,0 +1,62 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { dropSharedHostListLoad, shareHostListLoad } from './host-list-load-sharing' +import type { HostProfile } from './types' + +function deferred() { + let resolve: (hosts: HostProfile[]) => void = () => {} + const promise = new Promise((res) => { + resolve = res + }) + return { promise, resolve } +} + +describe('shareHostListLoad', () => { + beforeEach(() => { + dropSharedHostListLoad() + }) + + it('runs one pass for callers that arrive while it is still open', async () => { + const pass = deferred() + const load = vi.fn(() => pass.promise) + + const first = shareHostListLoad(load) + const second = shareHostListLoad(load) + pass.resolve([]) + + expect(load).toHaveBeenCalledTimes(1) + expect(await first).toBe(await second) + }) + + it('starts a fresh pass for callers that arrive after a write dropped it', async () => { + const stale = deferred() + const fresh = deferred() + const load = vi.fn().mockReturnValueOnce(stale.promise).mockReturnValueOnce(fresh.promise) + + const before = shareHostListLoad(load) + dropSharedHostListLoad() + const after = shareHostListLoad(load) + + stale.resolve([]) + fresh.resolve([]) + expect(load).toHaveBeenCalledTimes(2) + expect(await after).not.toBe(await before) + }) + + it('keeps the replacement pass on offer when the dropped one settles late', async () => { + const stale = deferred() + const fresh = deferred() + const load = vi.fn().mockReturnValueOnce(stale.promise).mockReturnValueOnce(fresh.promise) + + const before = shareHostListLoad(load) + dropSharedHostListLoad() + const replacement = shareHostListLoad(load) + // The dropped pass settles after its replacement was registered. + stale.resolve([]) + await before + + expect(shareHostListLoad(load)).toBe(replacement) + expect(load).toHaveBeenCalledTimes(2) + fresh.resolve([]) + await replacement + }) +}) diff --git a/mobile/src/transport/host-list-load-sharing.ts b/mobile/src/transport/host-list-load-sharing.ts new file mode 100644 index 00000000000..40c1fa5a011 --- /dev/null +++ b/mobile/src/transport/host-list-load-sharing.ts @@ -0,0 +1,29 @@ +import type { HostProfile } from './types' + +// Why: Keychain reads are slow (50-200ms) and loadHosts() runs on every screen +// mount, so concurrent callers share one pass instead of each paying for their +// own. A pass that started before a write still holds the pre-write snapshot, +// though — handing it to a caller that loaded AFTER the write returns the host +// that caller just removed, or the name it just replaced (issue #8791). So every +// durable commit drops the shared pass and the next caller reads fresh. +let inflight: Promise | null = null + +export function shareHostListLoad(load: () => Promise): Promise { + if (inflight) { + return inflight + } + const started = load().finally(() => { + // Why: a dropped pass can settle after its replacement started; only retire + // the entry still on offer, or the replacement is silently discarded. + if (inflight === started) { + inflight = null + } + }) + inflight = started + return started +} + +/** Call after every durable host write so no later read is served a pre-write pass. */ +export function dropSharedHostListLoad(): void { + inflight = null +} diff --git a/mobile/src/transport/host-store.test.ts b/mobile/src/transport/host-store.test.ts index 918e6e92f93..c3ab632738a 100644 --- a/mobile/src/transport/host-store.test.ts +++ b/mobile/src/transport/host-store.test.ts @@ -319,6 +319,67 @@ describe('host-store list mutations', () => { expect(storedHostsRaw).toBe('{') }) + // Why: gates the (slow, real-device 50-200ms) Keychain pass so a load can be + // parked mid-flight while a write commits underneath it. + function gateKeychainReads(): () => void { + let release: () => void = () => {} + const gate = new Promise((resolve) => { + release = resolve + }) + secureStoreMock.getItemAsync.mockImplementation(async (key: string) => { + await gate + return key.endsWith(HOST_ONE.id) || key.endsWith(HOST_TWO.id) ? `token-${key.at(-1)}` : null + }) + return release + } + + it('does not serve a pre-removal snapshot to a load issued after the removal (#8791)', async () => { + const releaseKeychain = gateKeychainReads() + const parkedLoad = loadHosts() + await vi.waitFor(() => { + expect(secureStoreMock.getItemAsync).toHaveBeenCalled() + }) + + await removeHost(HOST_ONE.id) + const afterRemoval = loadHosts() + releaseKeychain() + + await parkedLoad + await expect(afterRemoval.then((hosts) => hosts.map((host) => host.id))).resolves.toEqual([ + HOST_TWO.id + ]) + }) + + it('does not serve a pre-rename snapshot to a load issued after the rename (#8791)', async () => { + const releaseKeychain = gateKeychainReads() + const parkedLoad = loadHosts() + await vi.waitFor(() => { + expect(secureStoreMock.getItemAsync).toHaveBeenCalled() + }) + + await updateHostNameAndEndpoint(HOST_ONE.id, { name: 'Living Room Mac' }) + const afterRename = loadHosts() + releaseKeychain() + + await parkedLoad + const hosts = await afterRename + expect(hosts.find((host) => host.id === HOST_ONE.id)?.name).toBe('Living Room Mac') + }) + + it('still shares one Keychain pass across loads with no write between them', async () => { + const releaseKeychain = gateKeychainReads() + const first = loadHosts() + await vi.waitFor(() => { + expect(secureStoreMock.getItemAsync).toHaveBeenCalled() + }) + const second = loadHosts() + releaseKeychain() + + const [firstHosts, secondHosts] = await Promise.all([first, second]) + expect(firstHosts).toBe(secondHosts) + expect(secureStoreMock.getItemAsync).toHaveBeenCalledTimes(2) + }) + it('resolves instead of rejecting when updateLastConnected hits unreadable storage', async () => { // Why: callers fire updateLastConnected with `void`; a rejection here would // surface as an unhandled promise rejection rather than a caught error. diff --git a/mobile/src/transport/host-store.ts b/mobile/src/transport/host-store.ts index 61ed6742d12..b3d5b0567c2 100644 --- a/mobile/src/transport/host-store.ts +++ b/mobile/src/transport/host-store.ts @@ -8,6 +8,7 @@ import { type StoredHostProfile } from './types' import { getNextHostNameFromHosts } from './host-names' +import { dropSharedHostListLoad, shareHostListLoad } from './host-list-load-sharing' import { retryPendingHostCredentialCleanups, scheduleHostCredentialCleanup @@ -73,7 +74,6 @@ async function deleteHostCredentials(hostId: string): Promise { // Why: Keychain reads are slow (50-200ms) and loadHosts() runs on every screen mount; cache per-hostId in memory, invalidate on save/remove. const tokenCache = new Map() -let inflightLoad: Promise | null = null // Why: serialize RMW of the shared hosts JSON; without a queue concurrent writers drop writes (resurrect a removed host, drop a rename). let hostListMutation: Promise = Promise.resolve() @@ -103,13 +103,7 @@ export async function loadHosts(): Promise { // Why: writers hold the mutation chain across their full RMW; wait so a load doesn't race a half-written list. await hostListMutation // Why: deduplicate concurrent loadHosts() calls so simultaneously mounting screens share one Keychain read pass. - if (inflightLoad) { - return inflightLoad - } - inflightLoad = doLoadHosts().finally(() => { - inflightLoad = null - }) - return inflightLoad + return shareHostListLoad(doLoadHosts) } async function doLoadHosts(): Promise { @@ -197,6 +191,7 @@ async function mutateStoredHosts( const current = await readStoredHostsForMutation() const next = update(current) await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(next)) + dropSharedHostListLoad() }) hostListMutation = mutation.catch(() => {}) return mutation @@ -258,6 +253,7 @@ async function persistHost(host: HostProfile, requireExisting: boolean): Promise relayHostId: validated.relayHostId, relay: validated.relay }) + dropSharedHostListLoad() } const overlayRemovalIds = [...duplicateHostIds] if (!validated.endpoints && updatedExistingHost) { @@ -266,6 +262,7 @@ async function persistHost(host: HostProfile, requireExisting: boolean): Promise if (overlayRemovalIds.length > 0) { // Why: reusing an id for direct-only re-pairing must not retain routing metadata from the previous transport state. await removeMobileRelayHostOverlays(overlayRemovalIds) + dropSharedHostListLoad() } for (const duplicateHostId of duplicateHostIds) { tokenCache.delete(duplicateHostId) @@ -282,6 +279,7 @@ export async function removeHost(hostId: string): Promise { tokenCache.delete(hostId) try { await removeMobileRelayHostOverlay(hostId) + dropSharedHostListLoad() } catch { // Base removal is authoritative; a retained overlay can't resurrect the host and is cleaned on a later retry. } @@ -341,5 +339,5 @@ export async function updateLastConnected(hostId: string): Promise { export function resetHostStoreForTests(): void { hostListMutation = Promise.resolve() tokenCache.clear() - inflightLoad = null + dropSharedHostListLoad() }