From 60caeebe5357f2cd189976022f5ac3c225ea7f26 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:17:02 -0700 Subject: [PATCH] fix(mobile): retain cold task navigation intent --- mobile/src/tasks/MobileTaskHostRoute.test.ts | 87 +++++++++++++++---- mobile/src/tasks/MobileTaskHostRoute.tsx | 29 +++++-- .../src/tasks/mobile-task-navigation.test.ts | 65 ++++++++++++-- mobile/src/tasks/mobile-task-navigation.ts | 57 +++++++----- 4 files changed, 185 insertions(+), 53 deletions(-) diff --git a/mobile/src/tasks/MobileTaskHostRoute.test.ts b/mobile/src/tasks/MobileTaskHostRoute.test.ts index 026e83db12a..3ae36473a12 100644 --- a/mobile/src/tasks/MobileTaskHostRoute.test.ts +++ b/mobile/src/tasks/MobileTaskHostRoute.test.ts @@ -1,8 +1,15 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +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; action?: string; taskSource?: string }, + params: {} as { hostId?: string }, isWideLayout: false })) @@ -20,25 +27,73 @@ vi.mock('../components/WorkspaceDetailPlaceholder', () => ({ })) function HostScreen() { - return null + return createElement('HostScreen') } -afterEach(() => { - mocks.params = {} - mocks.isWideLayout = false -}) - describe('MobileTaskHostRoute', () => { - it('redirects a mounted host action to its concrete Tasks route', () => { - mocks.params = { hostId: 'host/1', action: 'tasks', taskSource: 'github' } + let renderer: ReactTestRenderer | null = null - expect(MobileTaskHostRoute({ hostScreen: HostScreen })).toMatchObject({ - type: 'Redirect', - props: { href: '/h/host%2F1/tasks?taskSource=github' } - }) + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true }) - it('renders the normal host screen without the Tasks action', () => { - expect(MobileTaskHostRoute({ hostScreen: HostScreen })).toMatchObject({ type: HostScreen }) + 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 index ce4214d1fa1..2d7a9c53254 100644 --- a/mobile/src/tasks/MobileTaskHostRoute.tsx +++ b/mobile/src/tasks/MobileTaskHostRoute.tsx @@ -1,17 +1,30 @@ -import type { ComponentType } from 'react' +import { useEffect, type ComponentType } from 'react' import { Redirect, useLocalSearchParams } from 'expo-router' import { useResponsiveLayout } from '../layout/responsive-layout' import { WorkspaceDetailPlaceholder } from '../components/WorkspaceDetailPlaceholder' -import { mobileTasksRouteFromHostAction } from './mobile-task-navigation' +import { + clearPendingMobileTasksNavigation, + getPendingMobileTasksNavigation, + mobileTasksRouteForMountedHost +} from './mobile-task-navigation' export function MobileTaskHostRoute({ hostScreen: HostScreen }: { hostScreen: ComponentType }) { - const params = useLocalSearchParams<{ - hostId?: string - action?: string - taskSource?: string - }>() + const params = useLocalSearchParams<{ hostId?: string }>() const { isWideLayout } = useResponsiveLayout() - const tasksRoute = mobileTasksRouteFromHostAction(params.hostId, params.action, params.taskSource) + 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 diff --git a/mobile/src/tasks/mobile-task-navigation.test.ts b/mobile/src/tasks/mobile-task-navigation.test.ts index a71012e24d4..d6dd12a34a0 100644 --- a/mobile/src/tasks/mobile-task-navigation.test.ts +++ b/mobile/src/tasks/mobile-task-navigation.test.ts @@ -1,29 +1,76 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { + clearPendingMobileTasksNavigation, + getPendingMobileTasksNavigation, mobileTasksHostRoute, mobileTasksRoute, - mobileTasksRouteFromHostAction, + mobileTasksRouteForMountedHost, navigateToMobileTasks } from './mobile-task-navigation' +afterEach(() => { + const pending = getPendingMobileTasksNavigation() + if (pending) { + clearPendingMobileTasksNavigation(pending) + } +}) + describe('mobileTasksRoute', () => { it('builds a concrete encoded host route', () => { expect(mobileTasksRoute('host/1')).toBe('/h/host%2F1/tasks') expect(mobileTasksRoute('host/1', 'linear')).toBe('/h/host%2F1/tasks?taskSource=linear') }) - it('routes through the host index so its nested navigator owns the redirect', () => { + it('publishes the concrete route before mounting the encoded host index', () => { const push = vi.fn() navigateToMobileTasks({ push }, 'host/1', 'github') - expect(push).toHaveBeenCalledWith(mobileTasksHostRoute('host/1', 'github')) + expect(push).toHaveBeenCalledWith(mobileTasksHostRoute('host/1')) + expect(getPendingMobileTasksNavigation()).toEqual({ + hostId: 'host/1', + route: '/h/host%2F1/tasks?taskSource=github' + }) }) - it('resolves the host-owned Tasks action after the nested navigator mounts', () => { - expect(mobileTasksRouteFromHostAction('host/1', 'tasks', 'linear')).toBe( - '/h/host%2F1/tasks?taskSource=linear' + it('uses a pending route only for its intended host or a cold host with lost params', () => { + navigateToMobileTasks({ push: vi.fn() }, 'host-1', 'linear') + const pending = getPendingMobileTasksNavigation() + + expect(mobileTasksRouteForMountedHost('host-1', pending)).toBe( + '/h/host-1/tasks?taskSource=linear' ) - expect(mobileTasksRouteFromHostAction('host-1', 'newWorktree', 'github')).toBeNull() - expect(mobileTasksRouteFromHostAction(undefined, 'tasks', 'github')).toBeNull() + expect(mobileTasksRouteForMountedHost(undefined, pending)).toBe( + '/h/host-1/tasks?taskSource=linear' + ) + expect(mobileTasksRouteForMountedHost('host-2', pending)).toBeNull() + }) + + it('keeps a newer intent when stale cleanup races repeated navigation', () => { + navigateToMobileTasks({ push: vi.fn() }, 'host-1', 'github') + const first = getPendingMobileTasksNavigation()! + navigateToMobileTasks({ push: vi.fn() }, 'host-2', 'gitlab') + const second = getPendingMobileTasksNavigation()! + + clearPendingMobileTasksNavigation(first) + expect(getPendingMobileTasksNavigation()).toBe(second) + expect(second).toEqual({ hostId: 'host-2', route: '/h/host-2/tasks?taskSource=gitlab' }) + clearPendingMobileTasksNavigation(second) + expect(getPendingMobileTasksNavigation()).toBeNull() + }) + + it('clears the intent if opening the host route throws', () => { + const error = new Error('navigation failed') + + expect(() => + navigateToMobileTasks( + { + push: () => { + throw error + } + }, + 'host-1' + ) + ).toThrow(error) + expect(getPendingMobileTasksNavigation()).toBeNull() }) }) diff --git a/mobile/src/tasks/mobile-task-navigation.ts b/mobile/src/tasks/mobile-task-navigation.ts index dceb4e1d4e9..29e42e175c1 100644 --- a/mobile/src/tasks/mobile-task-navigation.ts +++ b/mobile/src/tasks/mobile-task-navigation.ts @@ -4,41 +4,48 @@ export type MobileTasksRoute = | `/h/${string}/tasks` | `/h/${string}/tasks?taskSource=${TaskProvider}` -export type MobileTasksHostRoute = - | `/h/${string}?action=tasks` - | `/h/${string}?action=tasks&taskSource=${TaskProvider}` +export type MobileTasksHostRoute = `/h/${string}` + +export type MobileTasksNavigationIntent = Readonly<{ + hostId: string + route: MobileTasksRoute +}> type MobileTasksRouter = { push: (href: MobileTasksHostRoute) => void } +// Why: cold Expo host stacks drop URL params; the mounted index consumes this bounded intent. +let pendingMobileTasksNavigation: MobileTasksNavigationIntent | null = null + export function mobileTasksRoute(hostId: string, provider?: TaskProvider): MobileTasksRoute { // Why: Expo Router can drop hostId from dynamic route objects in this cold nested stack. const pathname = `/h/${encodeURIComponent(hostId)}/tasks` as const return provider ? `${pathname}?taskSource=${provider}` : pathname } -export function mobileTasksHostRoute( - hostId: string, - provider?: TaskProvider -): MobileTasksHostRoute { - const pathname = `/h/${encodeURIComponent(hostId)}?action=tasks` as const - return provider ? `${pathname}&taskSource=${provider}` : pathname +export function mobileTasksHostRoute(hostId: string): MobileTasksHostRoute { + return `/h/${encodeURIComponent(hostId)}` } -export function mobileTasksRouteFromHostAction( - hostId: string | undefined, - action: string | undefined, - taskSource: string | undefined +export function getPendingMobileTasksNavigation(): MobileTasksNavigationIntent | null { + return pendingMobileTasksNavigation +} + +export function clearPendingMobileTasksNavigation(intent: MobileTasksNavigationIntent): void { + if (pendingMobileTasksNavigation === intent) { + pendingMobileTasksNavigation = null + } +} + +export function mobileTasksRouteForMountedHost( + mountedHostId: string | undefined, + intent: MobileTasksNavigationIntent | null ): MobileTasksRoute | null { - if (!hostId || action !== 'tasks') { + if (!intent || (mountedHostId && mountedHostId !== intent.hostId)) { return null } - const provider = - taskSource === 'github' || taskSource === 'gitlab' || taskSource === 'linear' - ? taskSource - : undefined - return mobileTasksRoute(hostId, provider) + return intent.route } export function navigateToMobileTasks( @@ -46,5 +53,15 @@ export function navigateToMobileTasks( hostId: string, provider?: TaskProvider ): void { - router.push(mobileTasksHostRoute(hostId, provider)) + const intent: MobileTasksNavigationIntent = { + hostId, + route: mobileTasksRoute(hostId, provider) + } + pendingMobileTasksNavigation = intent + try { + router.push(mobileTasksHostRoute(hostId)) + } catch (error) { + clearPendingMobileTasksNavigation(intent) + throw error + } }