mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
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).
This commit is contained in:
+15
-63
@@ -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)}
|
||||
/>
|
||||
|
||||
|
||||
@@ -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([])
|
||||
})
|
||||
})
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -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<HostProfile[]>((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
|
||||
})
|
||||
})
|
||||
@@ -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<HostProfile[]> | null = null
|
||||
|
||||
export function shareHostListLoad(load: () => Promise<HostProfile[]>): Promise<HostProfile[]> {
|
||||
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
|
||||
}
|
||||
@@ -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<void>((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.
|
||||
|
||||
@@ -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<void> {
|
||||
|
||||
// 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<string, string>()
|
||||
let inflightLoad: Promise<HostProfile[]> | 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<void> = Promise.resolve()
|
||||
|
||||
@@ -103,13 +103,7 @@ export async function loadHosts(): Promise<HostProfile[]> {
|
||||
// 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<HostProfile[]> {
|
||||
@@ -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<void> {
|
||||
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<void> {
|
||||
export function resetHostStoreForTests(): void {
|
||||
hostListMutation = Promise.resolve()
|
||||
tokenCache.clear()
|
||||
inflightLoad = null
|
||||
dropSharedHostListLoad()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user