From f719ec08458f4a188e16ac6ba654de34aa44cf5e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 21 Oct 2026 21:20:06 -0700 Subject: [PATCH] fix(mobile): persist browser destination in user selection order (#31689) Delayed hydration and out-of-order storage writes can reverse a browser destination selection. Order durable writes and preserve the confirmed value when saving fails. --- .../browser-settings-persistence.test.tsx | 91 +++++++++++++++++++ .../src/settings/browser-settings-screen.tsx | 33 +++++-- mobile/src/storage/preferences.test.ts | 21 +++++ mobile/src/storage/preferences.ts | 16 +++- 4 files changed, 153 insertions(+), 8 deletions(-) create mode 100644 mobile/src/settings/browser-settings-persistence.test.tsx diff --git a/mobile/src/settings/browser-settings-persistence.test.tsx b/mobile/src/settings/browser-settings-persistence.test.tsx new file mode 100644 index 00000000000..2a4ba21b10f --- /dev/null +++ b/mobile/src/settings/browser-settings-persistence.test.tsx @@ -0,0 +1,91 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import BrowserSettingsScreen from './browser-settings-screen' +import { PickerModal } from '../components/PickerModal' +import { + loadTerminalLinkOpenMode, + saveTerminalLinkOpenMode, + type MobileTerminalLinkOpenMode +} from '../storage/preferences' + +vi.mock('react-native', () => ({ + View: 'View', + Text: 'Text', + Pressable: 'Pressable', + ScrollView: 'ScrollView', + StyleSheet: { create: (value: unknown) => value } +})) +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => ({ top: 0, bottom: 0 }) +})) +vi.mock('expo-router', () => ({ useRouter: () => ({ back() {} }) })) +vi.mock('lucide-react-native', () => ({ ChevronLeft: 'Icon', ChevronRight: 'Icon', Globe: 'Icon' })) +vi.mock('../components/PickerModal', () => ({ PickerModal: () => null })) +vi.mock('../storage/preferences', () => ({ + loadTerminalLinkOpenMode: vi.fn(), + saveTerminalLinkOpenMode: vi.fn() +})) +let renderer: ReactTestRenderer + +afterEach(() => { + act(() => renderer?.unmount()) + vi.resetAllMocks() +}) + +describe('browser settings persistence', () => { + it('does not replace a new selection with an older pending hydration read', async () => { + let resolveLoad = (_value: MobileTerminalLinkOpenMode) => {} + vi.mocked(loadTerminalLinkOpenMode).mockImplementation( + () => + new Promise((resolve) => { + resolveLoad = resolve + }) + ) + vi.mocked(saveTerminalLinkOpenMode).mockResolvedValue() + await act(async () => { + renderer = create(createElement(BrowserSettingsScreen)) + }) + await act(async () => { + renderer.root.findByType(PickerModal).props.onSelect('phone-browser') + }) + await act(async () => { + resolveLoad('orca-browser') + }) + expect(renderer.root.findByType(PickerModal).props.selected).toBe('phone-browser') + expect(JSON.stringify(renderer.toJSON())).toContain('Phone browser') + }) + + it('restores the saved destination when changing the destination fails', async () => { + vi.mocked(loadTerminalLinkOpenMode).mockResolvedValue('orca-browser') + vi.mocked(saveTerminalLinkOpenMode).mockRejectedValue(new Error('storage unavailable')) + await act(async () => { + renderer = create(createElement(BrowserSettingsScreen)) + }) + await act(async () => { + renderer.root.findByType(PickerModal).props.onSelect('phone-browser') + }) + expect(renderer.root.findByType(PickerModal).props.selected).toBe('orca-browser') + expect(JSON.stringify(renderer.toJSON())).toContain('Could not save browser preferences') + }) + + it('restores the hydrated destination when the save fails before the first read finishes', async () => { + let resolveLoad = (_value: MobileTerminalLinkOpenMode) => {} + vi.mocked(loadTerminalLinkOpenMode).mockImplementation( + () => + new Promise((resolve) => { + resolveLoad = resolve + }) + ) + vi.mocked(saveTerminalLinkOpenMode).mockRejectedValue(new Error('storage unavailable')) + await act(async () => { + renderer = create(createElement(BrowserSettingsScreen)) + }) + await act(async () => { + renderer.root.findByType(PickerModal).props.onSelect('orca-browser') + }) + await act(async () => resolveLoad('phone-browser')) + expect(renderer.root.findByType(PickerModal).props.selected).toBe('phone-browser') + expect(JSON.stringify(renderer.toJSON())).toContain('Could not save browser preferences') + }) +}) diff --git a/mobile/src/settings/browser-settings-screen.tsx b/mobile/src/settings/browser-settings-screen.tsx index 924f7a7660b..7cc2f0ef032 100644 --- a/mobile/src/settings/browser-settings-screen.tsx +++ b/mobile/src/settings/browser-settings-screen.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { Pressable, ScrollView, StyleSheet, Text, View } from 'react-native' import { useSafeAreaInsets } from 'react-native-safe-area-context' import { useRouter } from 'expo-router' @@ -40,13 +40,24 @@ export default function BrowserSettingsScreen({ const [linkMode, setLinkMode] = useState('orca-browser') const [pickerOpen, setPickerOpen] = useState(false) const [error, setError] = useState(null) + const selectionRevision = useRef(0) + const failedSelectionRevision = useRef(null) + const savedMode = useRef('orca-browser') + const hasSaved = useRef(false) useEffect(() => { let active = true void loadTerminalLinkOpenMode().then( (mode) => { - if (active) { - setLinkMode(mode) + if (!hasSaved.current) { + savedMode.current = mode + } + if ( + active && + (selectionRevision.current === 0 || + failedSelectionRevision.current === selectionRevision.current) + ) { + setLinkMode(savedMode.current) } }, () => { @@ -62,10 +73,20 @@ export default function BrowserSettingsScreen({ const selectLinkMode = useCallback((mode: MobileTerminalLinkOpenMode) => { setError(null) - // Optimistic, as base was: the row shows the tapped mode before the write lands. + const revision = ++selectionRevision.current setLinkMode(mode) - void saveTerminalLinkOpenMode(mode).catch(() => - setError('Could not save browser preferences. Try again.') + void saveTerminalLinkOpenMode(mode).then( + () => { + savedMode.current = mode + hasSaved.current = true + }, + () => { + if (revision === selectionRevision.current) { + failedSelectionRevision.current = revision + setLinkMode(savedMode.current) + setError('Could not save browser preferences. Try again.') + } + } ) }, []) diff --git a/mobile/src/storage/preferences.test.ts b/mobile/src/storage/preferences.test.ts index 280d9ee9b19..7ce4b88c89b 100644 --- a/mobile/src/storage/preferences.test.ts +++ b/mobile/src/storage/preferences.test.ts @@ -500,6 +500,27 @@ describe('terminal link open mode preference', () => { await expect(loadTerminalLinkOpenMode()).resolves.toBe('orca-browser') }) + it('orders writes across settings mounts and makes readers wait for the latest choice', async () => { + const firstWrite = deferred() + let stored: string | null = 'orca-browser' + vi.mocked(AsyncStorage.getItem).mockImplementation(async () => stored) + vi.mocked(AsyncStorage.setItem).mockImplementation(async (_key, value) => { + if (value === 'phone-browser') { + await firstWrite.promise + } + stored = value + }) + const firstMount = saveTerminalLinkOpenMode('phone-browser') + const secondMount = saveTerminalLinkOpenMode('orca-browser') + const reader = loadTerminalLinkOpenMode() + await Promise.resolve() + expect(stored).toBe('orca-browser') + firstWrite.resolve() + await Promise.all([firstMount, secondMount]) + await expect(reader).resolves.toBe('orca-browser') + expect(stored).toBe('orca-browser') + }) + it('persists the selected mode', async () => { await saveTerminalLinkOpenMode('phone-browser') diff --git a/mobile/src/storage/preferences.ts b/mobile/src/storage/preferences.ts index 07bba732fee..93a4926c175 100644 --- a/mobile/src/storage/preferences.ts +++ b/mobile/src/storage/preferences.ts @@ -236,8 +236,10 @@ export type MobileTerminalLinkOpenMode = 'orca-browser' | 'phone-browser' const TERMINAL_LINK_OPEN_MODE_KEY = 'orca:terminalLinkOpenMode' export const DEFAULT_TERMINAL_LINK_OPEN_MODE: MobileTerminalLinkOpenMode = 'orca-browser' +let terminalLinkWriteBarrier: Promise | null = null export async function loadTerminalLinkOpenMode(): Promise { + await terminalLinkWriteBarrier try { const raw = await AsyncStorage.getItem(TERMINAL_LINK_OPEN_MODE_KEY) return raw === 'phone-browser' || raw === 'orca-browser' ? raw : DEFAULT_TERMINAL_LINK_OPEN_MODE @@ -246,8 +248,18 @@ export async function loadTerminalLinkOpenMode(): Promise { - await persistMirrored(TERMINAL_LINK_OPEN_MODE_KEY, mode) +export function saveTerminalLinkOpenMode(mode: MobileTerminalLinkOpenMode): Promise { + const write = (terminalLinkWriteBarrier ?? Promise.resolve()).then(() => + persistMirrored(TERMINAL_LINK_OPEN_MODE_KEY, mode) + ) + const barrier = write.catch(() => undefined) + terminalLinkWriteBarrier = barrier + void barrier.then(() => { + if (terminalLinkWriteBarrier === barrier) { + terminalLinkWriteBarrier = null + } + }) + return write } function stringArray(value: unknown): string[] {