From a589412bb11e9c40cec4a32f4db6bd259ee5b02f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:29:15 -0700 Subject: [PATCH] fix(mobile): defer tasks until host stack commits --- mobile/app/h/[hostId]/index.tsx | 8 +- mobile/app/h/_layout.tsx | 5 +- mobile/src/tasks/MobileTaskHostRoute.test.ts | 99 --------------- mobile/src/tasks/MobileTaskHostRoute.tsx | 33 ----- .../use-mobile-task-host-navigation.test.ts | 114 ++++++++++++++++++ .../tasks/use-mobile-task-host-navigation.ts | 32 +++++ 6 files changed, 156 insertions(+), 135 deletions(-) delete mode 100644 mobile/src/tasks/MobileTaskHostRoute.test.ts delete mode 100644 mobile/src/tasks/MobileTaskHostRoute.tsx create mode 100644 mobile/src/tasks/use-mobile-task-host-navigation.test.ts create mode 100644 mobile/src/tasks/use-mobile-task-host-navigation.ts diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index 529a132b54f..d88fb6699aa 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -65,6 +65,7 @@ import { BottomDrawer } from '../../../src/components/BottomDrawer' import { useHostProtocolGates } from '../../../src/components/HostProtocolGate' import { AuthFailedBanner } from '../../../src/components/AuthFailedBanner' import { MobileSearchField } from '../../../src/components/MobileSearchField' +import { WorkspaceDetailPlaceholder } from '../../../src/components/WorkspaceDetailPlaceholder' import { getCachedWorktrees, setCachedWorktrees } from '../../../src/cache/worktree-cache' import { setCachedRepos } from '../../../src/cache/repo-cache' import { colors, radii, spacing, typography } from '../../../src/theme/mobile-theme' @@ -101,7 +102,6 @@ import { import type { RepoSummary } from '../../../src/worktree/host-worktree-rpc-types' import type { WorkspaceStatusDefinition } from '../../../../src/shared/types' import { DEFAULT_MOBILE_WORKSPACE_STATUSES } from '../../../src/worktree/mobile-workspace-statuses' -import { MobileTaskHostRoute } from '../../../src/tasks/MobileTaskHostRoute' function isErrorVerdict(v: ConnectionVerdict): boolean { return v.kind === 'warning' || v.kind === 'unreachable' || v.kind === 'auth-failed' @@ -1396,7 +1396,11 @@ export function HostScreen({ // On wide layouts the sidebar hosts the list, so this route is just the empty detail pane. export default function HostWorktreeRoute() { - return + const { isWideLayout } = useResponsiveLayout() + if (isWideLayout) { + return + } + return } function ListSeparator() { diff --git a/mobile/app/h/_layout.tsx b/mobile/app/h/_layout.tsx index 7f77c33b648..124c409d6be 100644 --- a/mobile/app/h/_layout.tsx +++ b/mobile/app/h/_layout.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { View, StyleSheet, PanResponder } from 'react-native' -import { Stack, useGlobalSearchParams, usePathname } from 'expo-router' +import { Stack, useGlobalSearchParams, usePathname, useRouter } from 'expo-router' import { colors } from '../../src/theme/mobile-theme' import { useResponsiveLayout } from '../../src/layout/responsive-layout' import { @@ -12,6 +12,7 @@ import { } from '../../src/storage/preferences' import { HostProtocolGate } from '../../src/components/HostProtocolGate' import { HostScreen } from './[hostId]/index' +import { useMobileTaskHostNavigation } from '../../src/tasks/use-mobile-task-host-navigation' // Keep at least this much room for the detail pane when resizing the sidebar. const MIN_DETAIL_WIDTH = 320 @@ -63,6 +64,8 @@ export default function HostGroupLayout() { const { isWideLayout, width: windowWidth } = useResponsiveLayout() const { hostId, action } = useGlobalSearchParams<{ hostId?: string; action?: string }>() const pathname = usePathname() + const router = useRouter() + useMobileTaskHostNavigation(router, hostId) const [sidebarOpen, setSidebarOpen] = useState(true) const [sidebarWidth, setSidebarWidth] = useState(HOST_SIDEBAR_DEFAULT_WIDTH) diff --git a/mobile/src/tasks/MobileTaskHostRoute.test.ts b/mobile/src/tasks/MobileTaskHostRoute.test.ts deleted file mode 100644 index 3ae36473a12..00000000000 --- a/mobile/src/tasks/MobileTaskHostRoute.test.ts +++ /dev/null @@ -1,99 +0,0 @@ -import { createElement } from 'react' -import { act, create, type ReactTestRenderer } from 'react-test-renderer' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { MobileTaskHostRoute } from './MobileTaskHostRoute' -import { - clearPendingMobileTasksNavigation, - getPendingMobileTasksNavigation, - navigateToMobileTasks -} from './mobile-task-navigation' - -const mocks = vi.hoisted(() => ({ - params: {} as { hostId?: string }, - isWideLayout: false -})) - -vi.mock('expo-router', () => ({ - Redirect: 'Redirect', - useLocalSearchParams: () => mocks.params -})) - -vi.mock('../layout/responsive-layout', () => ({ - useResponsiveLayout: () => ({ isWideLayout: mocks.isWideLayout }) -})) - -vi.mock('../components/WorkspaceDetailPlaceholder', () => ({ - WorkspaceDetailPlaceholder: 'WorkspaceDetailPlaceholder' -})) - -function HostScreen() { - return createElement('HostScreen') -} - -describe('MobileTaskHostRoute', () => { - let renderer: ReactTestRenderer | null = null - - beforeEach(() => { - globalThis.IS_REACT_ACT_ENVIRONMENT = true - }) - - afterEach(() => { - act(() => renderer?.unmount()) - renderer = null - mocks.params = {} - mocks.isWideLayout = false - const pending = getPendingMobileTasksNavigation() - if (pending) { - clearPendingMobileTasksNavigation(pending) - } - vi.restoreAllMocks() - }) - - async function renderRoute(): Promise { - const consoleError = vi.spyOn(console, 'error').mockImplementation((...args) => { - if (typeof args[0] !== 'string' || !args[0].includes('react-test-renderer is deprecated')) { - throw new Error(String(args[0])) - } - }) - try { - await act(async () => { - renderer = create(createElement(MobileTaskHostRoute, { hostScreen: HostScreen })) - }) - } finally { - consoleError.mockRestore() - } - return renderer! - } - - it('redirects from the cold host shell even when Expo drops its route params', async () => { - navigateToMobileTasks({ push: vi.fn() }, 'host/1', 'github') - const pending = getPendingMobileTasksNavigation() - - const route = await renderRoute() - - expect(route.root.findByType('Redirect').props.href).toBe('/h/host%2F1/tasks?taskSource=github') - expect(getPendingMobileTasksNavigation()).toBe(pending) - - act(() => route.unmount()) - renderer = null - expect(getPendingMobileTasksNavigation()).toBeNull() - }) - - it('clears a stale intent instead of redirecting a different mounted host', async () => { - navigateToMobileTasks({ push: vi.fn() }, 'host-1', 'linear') - mocks.params = { hostId: 'host-2' } - - const route = await renderRoute() - - expect(route.root.findByType('HostScreen')).toBeDefined() - expect(getPendingMobileTasksNavigation()).toBeNull() - }) - - it('preserves the wide-layout placeholder without a pending intent', async () => { - mocks.isWideLayout = true - - const route = await renderRoute() - - expect(route.root.findByType('WorkspaceDetailPlaceholder')).toBeDefined() - }) -}) diff --git a/mobile/src/tasks/MobileTaskHostRoute.tsx b/mobile/src/tasks/MobileTaskHostRoute.tsx deleted file mode 100644 index 2d7a9c53254..00000000000 --- a/mobile/src/tasks/MobileTaskHostRoute.tsx +++ /dev/null @@ -1,33 +0,0 @@ -import { useEffect, type ComponentType } from 'react' -import { Redirect, useLocalSearchParams } from 'expo-router' -import { useResponsiveLayout } from '../layout/responsive-layout' -import { WorkspaceDetailPlaceholder } from '../components/WorkspaceDetailPlaceholder' -import { - clearPendingMobileTasksNavigation, - getPendingMobileTasksNavigation, - mobileTasksRouteForMountedHost -} from './mobile-task-navigation' - -export function MobileTaskHostRoute({ hostScreen: HostScreen }: { hostScreen: ComponentType }) { - const params = useLocalSearchParams<{ hostId?: string }>() - const { isWideLayout } = useResponsiveLayout() - const intent = getPendingMobileTasksNavigation() - const tasksRoute = mobileTasksRouteForMountedHost(params.hostId, intent) - - // Why: unmount proves the redirect committed; identity cleanup cannot erase a newer tap. - useEffect(() => { - if (!intent) { - return - } - if (!tasksRoute) { - clearPendingMobileTasksNavigation(intent) - return - } - return () => clearPendingMobileTasksNavigation(intent) - }, [intent, tasksRoute]) - - if (tasksRoute) { - return - } - return isWideLayout ? : -} diff --git a/mobile/src/tasks/use-mobile-task-host-navigation.test.ts b/mobile/src/tasks/use-mobile-task-host-navigation.test.ts new file mode 100644 index 00000000000..b992c4a871c --- /dev/null +++ b/mobile/src/tasks/use-mobile-task-host-navigation.test.ts @@ -0,0 +1,114 @@ +import { createElement, type ReactNode, useEffect } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + clearPendingMobileTasksNavigation, + getPendingMobileTasksNavigation, + navigateToMobileTasks, + type MobileTasksRoute +} from './mobile-task-navigation' +import { useMobileTaskHostNavigation } from './use-mobile-task-host-navigation' + +function HostStackContent({ onCommit }: { onCommit: () => void }) { + useEffect(onCommit, [onCommit]) + return createElement('HostScreen') +} + +function HostLayoutHarness({ + children, + hostId, + replace +}: { + children: ReactNode + hostId?: string + replace: (route: MobileTasksRoute) => void +}) { + useMobileTaskHostNavigation({ replace }, hostId) + return children +} + +describe('useMobileTaskHostNavigation', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + const pending = getPendingMobileTasksNavigation() + if (pending) { + clearPendingMobileTasksNavigation(pending) + } + vi.restoreAllMocks() + }) + + async function renderLayout(args: { + hostId?: string + onCommit: () => void + replace: (route: MobileTasksRoute) => void + }): Promise { + const consoleError = vi.spyOn(console, 'error').mockImplementation((...errorArgs) => { + if ( + typeof errorArgs[0] !== 'string' || + !errorArgs[0].includes('react-test-renderer is deprecated') + ) { + throw new Error(String(errorArgs[0])) + } + }) + try { + await act(async () => { + renderer = create( + createElement( + HostLayoutHarness, + { hostId: args.hostId, replace: args.replace }, + createElement(HostStackContent, { onCommit: args.onCommit }) + ) + ) + }) + } finally { + consoleError.mockRestore() + } + } + + it('waits for host content to commit before replacing a cold route with lost params', async () => { + let hostCommitted = false + navigateToMobileTasks({ push: vi.fn() }, 'host/1', 'github') + const replace = vi.fn((route: MobileTasksRoute) => { + expect(hostCommitted).toBe(true) + expect(route).toBe('/h/host%2F1/tasks?taskSource=github') + }) + + await renderLayout({ + onCommit: () => { + hostCommitted = true + }, + replace + }) + + expect(replace).toHaveBeenCalledOnce() + expect(getPendingMobileTasksNavigation()).toBeNull() + }) + + it('clears a mismatched host intent without replacing', async () => { + navigateToMobileTasks({ push: vi.fn() }, 'host-1', 'linear') + const replace = vi.fn() + + await renderLayout({ hostId: 'host-2', onCommit: vi.fn(), replace }) + + expect(replace).not.toHaveBeenCalled() + expect(getPendingMobileTasksNavigation()).toBeNull() + }) + + it('consumes only the latest intent from repeated taps', async () => { + navigateToMobileTasks({ push: vi.fn() }, 'host-1', 'github') + navigateToMobileTasks({ push: vi.fn() }, 'host-2', 'gitlab') + const replace = vi.fn() + + await renderLayout({ hostId: 'host-2', onCommit: vi.fn(), replace }) + + expect(replace).toHaveBeenCalledWith('/h/host-2/tasks?taskSource=gitlab') + expect(getPendingMobileTasksNavigation()).toBeNull() + }) +}) diff --git a/mobile/src/tasks/use-mobile-task-host-navigation.ts b/mobile/src/tasks/use-mobile-task-host-navigation.ts new file mode 100644 index 00000000000..d86358c8878 --- /dev/null +++ b/mobile/src/tasks/use-mobile-task-host-navigation.ts @@ -0,0 +1,32 @@ +import { useEffect } from 'react' +import { + clearPendingMobileTasksNavigation, + getPendingMobileTasksNavigation, + mobileTasksRouteForMountedHost, + type MobileTasksRoute +} from './mobile-task-navigation' + +type MobileTasksReplaceRouter = { + replace: (route: MobileTasksRoute) => void +} + +export function useMobileTaskHostNavigation( + router: MobileTasksReplaceRouter, + hostId: string | undefined +): void { + // Why: this host-layout effect runs after HostStack's children commit, when nested replace is valid. + useEffect(() => { + const intent = getPendingMobileTasksNavigation() + if (!intent) { + return + } + try { + const route = mobileTasksRouteForMountedHost(hostId, intent) + if (route) { + router.replace(route) + } + } finally { + clearPendingMobileTasksNavigation(intent) + } + }, [hostId, router]) +}