From 4a77a0e9858bfa2ceee69e49e24a20e1bcc47d53 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:05:34 -0700 Subject: [PATCH] fix(native-chat): harden mobile view preferences --- mobile/app/_layout.tsx | 1 + mobile/app/native-chat-settings.tsx | 8 +- .../use-mobile-session-view-mode.test.ts | 128 ++++++++++++++++ .../session/use-mobile-session-view-mode.ts | 137 +++++++++++++++--- 4 files changed, 251 insertions(+), 23 deletions(-) diff --git a/mobile/app/_layout.tsx b/mobile/app/_layout.tsx index 1059c95502a..fe89a82c4a0 100644 --- a/mobile/app/_layout.tsx +++ b/mobile/app/_layout.tsx @@ -184,6 +184,7 @@ export default function RootLayout() { /> + diff --git a/mobile/app/native-chat-settings.tsx b/mobile/app/native-chat-settings.tsx index bc940765e46..ddbf4e6eccc 100644 --- a/mobile/app/native-chat-settings.tsx +++ b/mobile/app/native-chat-settings.tsx @@ -34,7 +34,12 @@ export default function NativeChatSettingsScreen() { return ( - router.back()}> + router.back()} + > Native chat @@ -57,6 +62,7 @@ export default function NativeChatSettingsScreen() { {chatDefault ? 'On' : 'Off'} (): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + vi.mock('expo-router', async () => { const react = await import('react') return { @@ -105,4 +113,124 @@ describe('useMobileSessionViewMode', () => { expect(vi.mocked(saveSessionViewOverrides).mock.calls.at(-1)?.[2].get('t1')).toBe('chat') expect(controller?.isTabChatView('t1')).toBe(true) }) + + it('does not expose overrides from the previous host while the next scope loads', async () => { + const nextScopeLoad = deferred>() + vi.mocked(loadSessionViewOverrides).mockImplementation((hostId) => + hostId === 'h1' + ? Promise.resolve(new Map([['same-tab-id', 'chat' as const]])) + : nextScopeLoad.promise + ) + function Harness(props: { hostId: string; worktreeId: string }): null { + controller = useMobileSessionViewMode(props) + return null + } + await act(async () => { + renderer = create(createElement(Harness, { hostId: 'h1', worktreeId: 'w1' })) + await Promise.resolve() + await Promise.resolve() + }) + expect(controller?.isTabChatView('same-tab-id')).toBe(true) + + await act(async () => { + renderer?.update(createElement(Harness, { hostId: 'h2', worktreeId: 'w2' })) + await Promise.resolve() + }) + expect(controller?.isTabChatView('same-tab-id')).toBe(false) + + await act(async () => { + nextScopeLoad.resolve(new Map()) + await Promise.resolve() + }) + }) + + it('merges a toggle made during load with the other persisted overrides', async () => { + const overridesLoad = deferred>() + vi.mocked(loadSessionViewOverrides).mockReturnValue(overridesLoad.promise) + function Harness(): null { + controller = useMobileSessionViewMode({ hostId: 'h', worktreeId: 'w' }) + return null + } + await act(async () => { + renderer = create(createElement(Harness)) + await Promise.resolve() + }) + + act(() => controller?.toggleTabChatView('new-tab')) + expect(controller?.isTabChatView('new-tab')).toBe(true) + expect(saveSessionViewOverrides).not.toHaveBeenCalled() + + await act(async () => { + overridesLoad.resolve(new Map([['saved-tab', 'chat']])) + await Promise.resolve() + await Promise.resolve() + }) + + expect(controller?.isTabChatView('saved-tab')).toBe(true) + const saved = vi.mocked(saveSessionViewOverrides).mock.calls.at(-1)?.[2] + expect([...(saved?.entries() ?? [])]).toEqual([ + ['saved-tab', 'chat'], + ['new-tab', 'chat'] + ]) + }) + + it('serializes rapid persistence writes so the latest state lands last', async () => { + const firstSave = deferred() + await mount({ defaultView: 'terminal' }) + vi.mocked(saveSessionViewOverrides) + .mockImplementationOnce(() => firstSave.promise) + .mockResolvedValue(undefined) + + await act(async () => { + controller?.toggleTabChatView('t1') + await Promise.resolve() + }) + expect(saveSessionViewOverrides).toHaveBeenCalledTimes(1) + + await act(async () => { + controller?.toggleTabChatView('t2') + await Promise.resolve() + }) + expect(saveSessionViewOverrides).toHaveBeenCalledTimes(1) + + await act(async () => { + firstSave.resolve() + await Promise.resolve() + await Promise.resolve() + }) + expect(saveSessionViewOverrides).toHaveBeenCalledTimes(2) + const latest = vi.mocked(saveSessionViewOverrides).mock.calls.at(-1)?.[2] + expect([...(latest?.entries() ?? [])]).toEqual([ + ['t1', 'chat'], + ['t2', 'chat'] + ]) + }) + + it('finishes an early toggle save after the route unmounts', async () => { + const overridesLoad = deferred>() + vi.mocked(loadSessionViewOverrides).mockReturnValue(overridesLoad.promise) + function Harness(): null { + controller = useMobileSessionViewMode({ hostId: 'h', worktreeId: 'w' }) + return null + } + await act(async () => { + renderer = create(createElement(Harness)) + await Promise.resolve() + }) + act(() => controller?.toggleTabChatView('new-tab')) + act(() => renderer?.unmount()) + renderer = null + + await act(async () => { + overridesLoad.resolve(new Map([['saved-tab', 'terminal']])) + await Promise.resolve() + await Promise.resolve() + }) + + const saved = vi.mocked(saveSessionViewOverrides).mock.calls.at(-1)?.[2] + expect([...(saved?.entries() ?? [])]).toEqual([ + ['saved-tab', 'terminal'], + ['new-tab', 'chat'] + ]) + }) }) diff --git a/mobile/src/session/use-mobile-session-view-mode.ts b/mobile/src/session/use-mobile-session-view-mode.ts index b5d46a44b9a..1544c6547bf 100644 --- a/mobile/src/session/use-mobile-session-view-mode.ts +++ b/mobile/src/session/use-mobile-session-view-mode.ts @@ -7,6 +7,38 @@ import { type MobileSessionView } from '../storage/preferences' +type ViewOverridesState = { + hostId: string + worktreeId: string + overrides: Map + loaded: boolean +} + +type ViewOverridesRuntime = { + hostId: string + worktreeId: string + loadPromise: Promise> + currentOverrides: Map + mutationRevision: number + lastQueuedMutationRevision: number + saveQueue: Promise +} + +function isOverrideScope(state: ViewOverridesState, hostId: string, worktreeId: string): boolean { + return state.hostId === hostId && state.worktreeId === worktreeId +} + +function mergeOverrides( + persisted: ReadonlyMap, + current: ReadonlyMap +): Map { + const merged = new Map(persisted) + for (const [tabId, view] of current) { + merged.set(tabId, view) + } + return merged +} + export type MobileSessionViewModeController = { /** Whether a tab's effective view is chat (per-tab override, else the default). */ isTabChatView: (tabId: string) => boolean @@ -21,8 +53,32 @@ export function useMobileSessionViewMode(args: { worktreeId: string }): MobileSessionViewModeController { const { hostId, worktreeId } = args - const [viewOverrides, setViewOverrides] = useState>(new Map()) - const viewOverridesToggledRef = useRef(false) + const [viewOverridesState, setViewOverridesState] = useState(() => ({ + hostId, + worktreeId, + overrides: new Map(), + loaded: false + })) + const viewOverridesStateRef = useRef(viewOverridesState) + viewOverridesStateRef.current = viewOverridesState + const viewOverridesRuntimeRef = useRef(null) + const ensureViewOverridesRuntime = useCallback((scopeHostId: string, scopeWorktreeId: string) => { + const current = viewOverridesRuntimeRef.current + if (current?.hostId === scopeHostId && current.worktreeId === scopeWorktreeId) { + return current + } + const next: ViewOverridesRuntime = { + hostId: scopeHostId, + worktreeId: scopeWorktreeId, + loadPromise: loadSessionViewOverrides(scopeHostId, scopeWorktreeId), + currentOverrides: new Map(), + mutationRevision: 0, + lastQueuedMutationRevision: 0, + saveQueue: Promise.resolve() + } + viewOverridesRuntimeRef.current = next + return next + }, []) const [defaultView, setDefaultView] = useState('terminal') // Why: the toggle callback reads the live default without depending on it, so // its identity stays stable and it never captures a stale default. @@ -31,19 +87,23 @@ export function useMobileSessionViewMode(args: { useEffect(() => { let active = true - // Re-arm per host/worktree so the fresh load can seed the new overrides. - viewOverridesToggledRef.current = false - void loadSessionViewOverrides(hostId, worktreeId).then((overrides) => { - // Why: a toggle before this load resolves is authoritative; the load must - // not revert the user's in-memory choice back to the persisted state. - if (active && !viewOverridesToggledRef.current) { - setViewOverrides(new Map(overrides)) + const runtime = ensureViewOverridesRuntime(hostId, worktreeId) + void runtime.loadPromise.then((persisted) => { + if (!active) { + return } + // Why: toggles made during the read are authoritative, but must not + // discard unrelated persisted overrides from the same worktree. + const merged = mergeOverrides(persisted, runtime.currentOverrides) + runtime.currentOverrides = merged + const next = { hostId, worktreeId, overrides: merged, loaded: true } + viewOverridesStateRef.current = next + setViewOverridesState(next) }) return () => { active = false } - }, [hostId, worktreeId]) + }, [ensureViewOverridesRuntime, hostId, worktreeId]) // Why: reload on focus so returning from Settings picks up a changed default. useFocusEffect( @@ -61,24 +121,57 @@ export function useMobileSessionViewMode(args: { ) const isTabChatView = useCallback( - (tabId: string): boolean => (viewOverrides.get(tabId) ?? defaultView) === 'chat', - [viewOverrides, defaultView] + (tabId: string): boolean => { + if (!isOverrideScope(viewOverridesState, hostId, worktreeId)) { + return false + } + const override = viewOverridesState.overrides.get(tabId) + // Until this scope loads, only an immediate user toggle is authoritative; + // defaulting other tabs to terminal avoids activating stale cross-host chat. + return (override ?? (viewOverridesState.loaded ? defaultView : 'terminal')) === 'chat' + }, + [defaultView, hostId, viewOverridesState, worktreeId] ) const toggleTabChatView = useCallback( (tabId: string) => { - viewOverridesToggledRef.current = true - setViewOverrides((previous) => { - const next = new Map(previous) - // Flip from the tab's effective view (its override, else the default), so - // a tab following a chat default can still be pinned back to terminal. - const currentlyChat = (previous.get(tabId) ?? defaultViewRef.current) === 'chat' - next.set(tabId, currentlyChat ? 'terminal' : 'chat') - void saveSessionViewOverrides(hostId, worktreeId, next) - return next + const current = viewOverridesStateRef.current + const currentScope = isOverrideScope(current, hostId, worktreeId) + ? current + : { + hostId, + worktreeId, + overrides: new Map(), + loaded: false + } + const overrides = new Map(currentScope.overrides) + // Flip from the tab's effective view (its override, else the default), so + // a tab following a chat default can still be pinned back to terminal. + const currentlyChat = (overrides.get(tabId) ?? defaultViewRef.current) === 'chat' + overrides.set(tabId, currentlyChat ? 'terminal' : 'chat') + const next = { ...currentScope, overrides } + viewOverridesStateRef.current = next + setViewOverridesState(next) + + const runtime = ensureViewOverridesRuntime(hostId, worktreeId) + runtime.currentOverrides = overrides + runtime.mutationRevision += 1 + // Persistence follows the already-started read so an early toggle merges + // with saved siblings, and it continues even if the route unmounts. + void runtime.loadPromise.then((persisted) => { + if (runtime.mutationRevision <= runtime.lastQueuedMutationRevision) { + return + } + runtime.lastQueuedMutationRevision = runtime.mutationRevision + const snapshot = mergeOverrides(persisted, runtime.currentOverrides) + // Why: serialized writes prevent an older, slower AsyncStorage write from + // landing after a newer toggle and restoring stale persisted state. + runtime.saveQueue = runtime.saveQueue + .then(() => saveSessionViewOverrides(hostId, worktreeId, snapshot)) + .catch(() => undefined) }) }, - [hostId, worktreeId] + [ensureViewOverridesRuntime, hostId, worktreeId] ) return { isTabChatView, toggleTabChatView }