From 385418e0db1c230d573b1cd2e4dd04346d0cc6f4 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 22 Oct 2026 21:07:32 -0400 Subject: [PATCH] fix(mobile): the shell returns to a screen already on its stack (#32110) * fix(mobile): the shell returns to a screen already on its stack The page's own "back to hosts" chevron calls dismissTo('/') inside the page, which the page's router hands to the shell as one navigate message. The shell pushed every such href, so leaving a host from its page left [Home, Host, Home] with the old host page still mounted (WebView and live subscriptions), and every round trip stacked another one. System Back then walked back through stale host pages. The shell now reads the navigation container's state first: an href that is already a screen below this one (same path and query) is popped back to, and an href that is this screen is dropped. Anything else is pushed as before. The pop names its navigator and source route explicitly, because a screen's dispatch otherwise stamps its own key as the source and a parent stack that cannot find it drops the pop. The wide host-route dismissTo stays for the hrefs this rule does not cover: a host route whose query differs from the one below (the missing-worktree bounce), and a cold deep link with no host route below to return to. Overlay replace is unchanged, and so is the bridge message. * fix(mobile): a param cleared to empty and a fragment still match a screen on the stack The session screen clears paneKey with setParams({ paneKey: '' }), which leaves ?paneKey= on the stack entry, so a later navigate to the plain session href found no match and pushed a duplicate. Empty query values are now dropped when canonicalising, and the fragment is stripped since expo-router keeps it as a param no href spells the same way. --- .../mobile-web-shell/MobileWebShellScreen.tsx | 29 ++++- .../catch-all-page-route-states.test.tsx | 1 + .../host-area-leave-session.test.tsx | 62 +++++++--- .../mobile-web-shell-screen-test-harness.tsx | 9 ++ .../mobile-web-shell-screen-test-mocks.tsx | 6 +- .../native-stack-return.test.ts | 70 +++++++++++ .../mobile-web-shell/native-stack-return.ts | 116 ++++++++++++++++++ 7 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 mobile/src/mobile-web-shell/native-stack-return.test.ts create mode 100644 mobile/src/mobile-web-shell/native-stack-return.ts diff --git a/mobile/src/mobile-web-shell/MobileWebShellScreen.tsx b/mobile/src/mobile-web-shell/MobileWebShellScreen.tsx index c50689c654b..e3793e64b08 100644 --- a/mobile/src/mobile-web-shell/MobileWebShellScreen.tsx +++ b/mobile/src/mobile-web-shell/MobileWebShellScreen.tsx @@ -1,6 +1,6 @@ import { useEffect, useState, type ReactNode } from 'react' import { Linking, Pressable, StyleSheet, Text, View } from 'react-native' -import { useNavigation, useRouter } from 'expo-router' +import { useNavigation, useNavigationContainerRef, useRouter } from 'expo-router' import { useSafeAreaInsets } from 'react-native-safe-area-context' import { OrcaMobileWebShellView, @@ -30,6 +30,7 @@ import { useMobileWebShellSession } from './use-mobile-web-shell-session' import { usePageHostSnapshot } from './use-page-host-snapshot' import { useReportedHostAreaServing } from './host-area-serving' import { isHostRouteOf } from './page-route-policy' +import { nativeStackReturn } from './native-stack-return' import { PageRouteUnavailableScreen } from './PageRouteUnavailableScreen' import { useResponsiveLayout } from '../layout/responsive-layout' import { storageReadParamsSchema, type BridgeNativeVerb } from './bridge/bridge-native-verbs' @@ -142,6 +143,7 @@ export function MobileWebShellScreen({ const overlay = onOverlayLoad !== undefined const router = useRouter() const navigation = useNavigation() + const navigationContainer = useNavigationContainerRef() const popShellStack = useShellStackPop() const { isWideLayout } = useResponsiveLayout() const { droppedBinaryFrames, reportDroppedBinaryFrames } = useMobileWebShellDroppedFrames() @@ -252,15 +254,32 @@ export function MobileWebShellScreen({ console.warn('[web-shell] refused to open this screen', issue) reportShellFailure('document-load-failed') }, - // Pushed, never replaced: the page stays mounted underneath, so Back reveals it with no - // download and no second `init`. + // A new screen is pushed, never replaced: the page stays mounted underneath, so Back reveals it + // with no download and no second `init`. onNavigate: (href: string) => { // An overlay hands its place to where the page went: Back there returns to the screen beneath. if (overlay) { return void router.replace(href) } - // Wide, this host's route is the area-owning session below: back to it, or in place of this - // one from a cold deep link, never a second on top. `dismissTo` takes the href's params. + // A screen already on the stack is returned to, as system Back would: the page's own exits + // ask for the screen beneath it, and a push would stack a second copy over a live one. + const rootState = navigationContainer.getRootState() + const onStack = rootState ? nativeStackReturn(rootState, href) : null + if (onStack === 'current') { + return + } + if (onStack) { + navigation.dispatch({ + type: 'POP', + payload: { count: onStack.count }, + target: onStack.target, + source: onStack.source + }) + return + } + // Wide, this host's route is the area-owning session even when it is not on the stack as + // asked: `dismissTo` pops to it by name with the href's params, or replaces this one after a + // cold deep link, so it is never a second on top. if (isWideLayout && isHostRouteOf(href, hostId)) { router.dismissTo(href) return diff --git a/mobile/src/mobile-web-shell/catch-all-page-route-states.test.tsx b/mobile/src/mobile-web-shell/catch-all-page-route-states.test.tsx index fa42ad9b426..5719cd76600 100644 --- a/mobile/src/mobile-web-shell/catch-all-page-route-states.test.tsx +++ b/mobile/src/mobile-web-shell/catch-all-page-route-states.test.tsx @@ -102,6 +102,7 @@ vi.mock('expo-router', () => ({ }), usePathname: () => '/h/host-1', useNavigation: () => ({ setOptions: vi.fn() }), + useNavigationContainerRef: () => ({ getRootState: () => undefined }), useFocusEffect: () => {} })) vi.mock('../../modules/orca-mobile-web-shell/src', async () => { diff --git a/mobile/src/mobile-web-shell/host-area-leave-session.test.tsx b/mobile/src/mobile-web-shell/host-area-leave-session.test.tsx index def7f3e679e..e6372993c95 100644 --- a/mobile/src/mobile-web-shell/host-area-leave-session.test.tsx +++ b/mobile/src/mobile-web-shell/host-area-leave-session.test.tsx @@ -1,8 +1,8 @@ /** - * Leaving a detail session for the host route on a wide layout: the page's back chevron - * (`leaveSession`, which replaces to `/h/` when nothing is below it) and the missing-worktree - * bounce (`/h/?notice=…`). Under a page that owns the host area that hop reaches the shell, - * which must land on one host route rather than stack a second host-area session on the session. + * Leaving a detail session for the host route: the page's back chevron (`leaveSession`, which + * replaces to `/h/` when nothing is below it) and the missing-worktree bounce + * (`/h/?notice=…`). That hop reaches the shell, which must land on one host route rather than + * stack a second host-area session on the session. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -44,6 +44,7 @@ import { createElement } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { clientFrame, createFakeRpcClient } from './bridge-host-test-fakes' import { MobileWebShellScreen } from './MobileWebShellScreen' +import type { NativeStackState } from './native-stack-return' const HOST = '/h/host-1' const DETAIL = '/h/host-1/session/wt-1' @@ -54,16 +55,38 @@ const PHONE = { width: 390, height: 844 } /** A stack entry: the route's pathname and its search, as the native stack holds them. */ type Entry = string +/** The entries as the navigation container holds them: the host stack inside the root one. */ +function rootStateOf(stack: readonly Entry[]): NativeStackState { + const routes = stack.map((entry, index) => { + const [path = '', search = ''] = entry.split('?') + const [, , hostId = '', area, worktreeId] = path.split('/') + const query = Object.fromEntries(new URLSearchParams(search)) + return area === undefined + ? { key: `entry-${index}`, name: '[hostId]/index', params: { hostId, ...query } } + : { + key: `entry-${index}`, + name: `[hostId]/${area}/[worktreeId]`, + params: { hostId, worktreeId: worktreeId ?? '', ...query } + } + }) + const hostStack = { key: 'host-stack', index: routes.length - 1, routes } + return { key: 'root-stack', index: 0, routes: [{ key: 'h', name: 'h', state: hostStack }] } +} + /** - * The native stack the screen's calls act on, with expo-router 55's semantics: `push` adds, - * `dismissTo` is react-navigation's POP_TO — pop back to the nearest entry of that route and take - * the new params, or replace the current entry when there is none below it. + * The native stack the screen's calls act on, with expo-router 55's semantics: `push` adds, a + * dispatched `POP` removes that many, and `dismissTo` is react-navigation's POP_TO — pop back to + * the nearest entry of that route and take the new params, or replace the current entry when there + * is none below it. */ function applied(stack: readonly Entry[]): Entry[] { const next = [...stack] for (const [href] of dependencies.push.mock.calls) { next.push(String(href)) } + for (const [action] of dependencies.dispatch.mock.calls) { + next.splice(next.length - action.payload.count) + } for (const [href] of device.dismissTo.mock.calls) { const target = String(href) const route = target.split('?')[0] @@ -78,9 +101,14 @@ function applied(stack: readonly Entry[]): Entry[] { } /** The session page on screen, asking the shell for `href`. */ -async function hopFromSession(viewport: { width: number; height: number }, href: string) { +async function hopFromSession( + viewport: { width: number; height: number }, + stack: readonly Entry[], + href: string +) { device.width = viewport.width device.height = viewport.height + dependencies.rootState = rootStateOf(stack) dependencies.client = createFakeRpcClient() dependencies.state = harness.readyState('session-one') dependencies.pageRoutes = ['/h/[hostId]', '/h/[hostId]/session/[worktreeId]'] @@ -114,25 +142,31 @@ afterEach(() => { describe('leaving a wide detail session for its host route', () => { it('returns to the host-area session below it, so Back does not reopen the session', async () => { - await hopFromSession(IPAD, HOST) + await hopFromSession(IPAD, [HOST, DETAIL], HOST) expect(applied([HOST, DETAIL])).toEqual([HOST]) }) it('carries the bounce notice to that host route, and still stacks nothing', async () => { - await hopFromSession(IPAD, BOUNCE) + await hopFromSession(IPAD, [HOST, DETAIL], BOUNCE) expect(applied([HOST, DETAIL])).toEqual([BOUNCE]) }) it('lands on one host route from a cold deep link, with nothing below the session', async () => { - await hopFromSession(IPAD, BOUNCE) + await hopFromSession(IPAD, [DETAIL], BOUNCE) expect(applied([DETAIL])).toEqual([BOUNCE]) }) }) describe('the same hop on a phone', () => { - it('pushes, as it always has', async () => { - await hopFromSession(PHONE, HOST) + it('returns to the host route below rather than stacking a second one', async () => { + await hopFromSession(PHONE, [HOST, DETAIL], HOST) expect(device.dismissTo).not.toHaveBeenCalled() - expect(applied([HOST, DETAIL])).toEqual([HOST, DETAIL, HOST]) + expect(applied([HOST, DETAIL])).toEqual([HOST]) + }) + + it('pushes a route the stack does not hold, such as another worktree', async () => { + const other = '/h/host-1/session/wt-2' + await hopFromSession(PHONE, [HOST, DETAIL], other) + expect(applied([HOST, DETAIL])).toEqual([HOST, DETAIL, other]) }) }) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-harness.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-harness.tsx index 46013be226d..7bd0a28f136 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-harness.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-harness.tsx @@ -12,6 +12,7 @@ import type { MobileWebShellUpdateNotice } from './mobile-web-shell-session-contract' import type { ShellPageFrame } from './shell-page-frame' +import type { NativeStackState } from './native-stack-return' /** * Everything the screen's cases mock away, as one mutable record, plus the helpers that mount it. @@ -33,6 +34,10 @@ export type ScreenDependencies = { back: Mock /** What the native stack answers: false is a page opened as the first screen on it. */ canGoBack: boolean + /** The navigation container's whole state; undefined before it is ready. */ + rootState: NativeStackState | undefined + /** Actions the screen dispatched onto its own place on the stack. */ + dispatch: Mock pathname: string pageRoutes: readonly string[] routeGrants: readonly string[] @@ -103,6 +108,8 @@ export function createScreenDependencies(): ScreenDependencies { push: vi.fn(), back: vi.fn(), canGoBack: true, + rootState: undefined, + dispatch: vi.fn(), pathname: '/h/host-1', pageRoutes: ['/h/[hostId]'], routeGrants: DEFAULT_ROUTE_GRANTS, @@ -154,6 +161,8 @@ export function resetScreenDependencies(dependencies: ScreenDependencies): void dependencies.openUrl.mockReset() dependencies.openUrl.mockImplementation(() => Promise.resolve(true)) dependencies.canGoBack = true + dependencies.rootState = undefined + dependencies.dispatch.mockReset() dependencies.pathname = '/h/host-1' dependencies.updateNotice = null dependencies.backHandlers.clear() diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-mocks.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-mocks.tsx index 02b85754ba5..b13db66e613 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-mocks.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-screen-test-mocks.tsx @@ -117,7 +117,11 @@ export function screenModuleMocks(dependencies: ScreenDependencies) { // Read by the pop latch, which clears on the route this shell is mounted at changing. usePathname: () => dependencies.pathname, // The screen's own place on the stack, which is where the iOS swipe-back is taken away. - useNavigation: () => ({ setOptions: dependencies.setScreenOptions }), + useNavigation: () => ({ + setOptions: dependencies.setScreenOptions, + dispatch: dependencies.dispatch + }), + useNavigationContainerRef: () => ({ getRootState: () => dependencies.rootState }), useFocusEffect: (effect: () => undefined | (() => void)) => React.useEffect(effect, [effect]) }), // A component rather than a host string: the React key is what makes a retry a rebuilt WebView, diff --git a/mobile/src/mobile-web-shell/native-stack-return.test.ts b/mobile/src/mobile-web-shell/native-stack-return.test.ts new file mode 100644 index 00000000000..3d1d80e6005 --- /dev/null +++ b/mobile/src/mobile-web-shell/native-stack-return.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { nativeStackReturn, type NativeStackState } from './native-stack-return' + +type Route = NativeStackState['routes'][number] + +function stack(key: string, routes: readonly Route[]): NativeStackState { + return { + key, + index: routes.length - 1, + routes: routes.map((route, index) => ({ ...route, key: `${key}/${index}` })) + } +} + +function hostStack(...routes: Route[]): Route { + return { name: 'h', state: stack('host-stack', routes) } +} + +// The root layout is a screen of expo-router's own navigator, so the walk starts one level up. +function rootStack(...routes: Route[]): NativeStackState { + return stack('expo-root', [{ name: '__root', state: stack('root-stack', routes) }]) +} + +const HOME = { name: 'index' } +const HOST = { name: '[hostId]/index', params: { hostId: 'mac/1' } } +const SESSION = { + name: '[hostId]/session/[worktreeId]', + params: { hostId: 'mac/1', worktreeId: 'wt-a' } +} + +describe('nativeStackReturn', () => { + it('pops back to Home under the host instead of stacking a second Home', () => { + expect(nativeStackReturn(rootStack(HOME, hostStack(HOST)), '/')).toEqual({ + target: 'root-stack', + source: 'root-stack/1', + count: 1 + }) + }) + + it('finds nothing when the host is the root, as after a cold deep link', () => { + expect(nativeStackReturn(rootStack(hostStack(HOST)), '/')).toBeNull() + }) + + it('returns to an exact path and query, however the href spells it', () => { + const files = { name: '[hostId]/files/[worktreeId]', params: SESSION.params } + const tabbed = { ...SESSION, params: { ...SESSION.params, tab: 'b', pane: '1' } } + const state = rootStack(HOME, hostStack(HOST, tabbed, files)) + expect(nativeStackReturn(state, '/h/mac%2F1/session/wt-a?pane=1&tab=b')).toEqual({ + target: 'host-stack', + source: 'host-stack/2', + count: 1 + }) + expect(nativeStackReturn(state, '/h/mac%2F1')).toMatchObject({ target: 'host-stack', count: 2 }) + expect(nativeStackReturn(state, '/h/mac%2F1/session/wt-a?pane=1&tab=c')).toBeNull() + expect(nativeStackReturn(state, '/h/mac%2F1/session/wt-b?pane=1&tab=b')).toBeNull() + }) + + it('treats a param cleared to empty as absent', () => { + const cleared = { ...SESSION, params: { ...SESSION.params, paneKey: '' } } + const state = rootStack( + HOME, + hostStack(HOST, cleared, { name: '[hostId]/files/[worktreeId]', params: SESSION.params }) + ) + expect(nativeStackReturn(state, '/h/mac%2F1/session/wt-a')).toMatchObject({ count: 1 }) + }) + + it('names the focused screen as current rather than a pop', () => { + const state = rootStack(HOME, hostStack(HOST, SESSION)) + expect(nativeStackReturn(state, '/h/mac%2F1/session/wt-a')).toBe('current') + }) +}) diff --git a/mobile/src/mobile-web-shell/native-stack-return.ts b/mobile/src/mobile-web-shell/native-stack-return.ts new file mode 100644 index 00000000000..e9d3a879469 --- /dev/null +++ b/mobile/src/mobile-web-shell/native-stack-return.ts @@ -0,0 +1,116 @@ +import { stringifyRouteHref } from '../navigation/route-href' + +export type NativeStackState = Readonly<{ + key?: string + index?: number + routes: readonly NativeStackRoute[] +}> + +type NativeStackRoute = Readonly<{ + key?: string + name: string + params?: object + state?: NativeStackState +}> + +/** A pop of `count` screens off the navigator keyed `target`, counted from its route `source`. + * `source` is explicit because a screen's `dispatch` stamps its own key there otherwise, and a parent + * navigator that cannot find that key among its routes drops the pop. */ +export type NativeStackReturn = Readonly<{ target: string; source: string; count: number }> + +/** Expo-router file segments that name no part of the URL: `index`, `(group)` and its own `__root`. */ +function isUrlSegment(segment: string): boolean { + return segment !== 'index' && !/^\(.*\)$/.test(segment) && !segment.startsWith('__') +} + +/** A state not yet rehydrated may omit `index`; React Navigation reads that as its last route. */ +function focusedIndex(state: NativeStackState): number { + return state.index ?? state.routes.length - 1 +} + +function decodeSegment(segment: string): string { + try { + return decodeURIComponent(segment) + } catch { + return segment + } +} + +/** One spelling per URL: decoded path segments and the query sorted, as expo-router parses both. + * An empty value is dropped because screens clear a param with `''` (session `paneKey`), and the + * fragment because expo-router keeps it as a param no href spells the same way. */ +function canonicalHref(href: string): string { + const [path = '', search = ''] = href.split('#')[0].split('?') + const segments = path.split('/').filter(Boolean).map(decodeSegment) + const query = [...new URLSearchParams(search)] + .filter(([, value]) => value !== '') + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + return `/${segments.join('/')}?${new URLSearchParams(query).toString()}` +} + +/** The URL a route on the stack shows, or null when its params hold something no URL carries. */ +function routeHref(route: NativeStackRoute, parentNames: readonly string[]): string | null { + const names = [...parentNames, route.name] + const nested = route.state?.routes[focusedIndex(route.state)] + if (nested) { + return routeHref(nested, names) + } + const params: Record = {} + for (const [key, value] of Object.entries(route.params ?? {})) { + if (typeof value === 'string') { + params[key] = value + } else if (Array.isArray(value) && value.every((one) => typeof one === 'string')) { + params[key] = value + } else if (value !== undefined) { + return null + } + } + const pathname = `/${names + .flatMap((name) => name.split('/')) + .filter(isUrlSegment) + .join('/')}` + return canonicalHref(stringifyRouteHref({ pathname, params })) +} + +function returnWithin( + level: NativeStackState, + parentNames: readonly string[], + wanted: string +): NativeStackReturn | 'current' | null { + const current = focusedIndex(level) + const focused = level.routes[current] + if (!focused) { + return null + } + if (focused.state) { + // The deeper navigator first: its screens sit nearer the top than anything at this level. + const deeper = returnWithin(focused.state, [...parentNames, focused.name], wanted) + if (deeper) { + return deeper + } + } else if (routeHref(focused, parentNames) === wanted) { + return 'current' + } + if (!level.key || !focused.key) { + return null + } + for (let index = current - 1; index >= 0; index--) { + if (routeHref(level.routes[index], parentNames) === wanted) { + return { target: level.key, source: focused.key, count: current - index } + } + } + return null +} + +/** + * Where `href` already sits on the native stack: `'current'` when it is the focused screen, the pop + * that returns to the nearest screen below showing it, or null when it is nowhere on the stack. + * + * Path and query both count, so another worktree or tab under the same file is a different screen. + */ +export function nativeStackReturn( + state: NativeStackState, + href: string +): NativeStackReturn | 'current' | null { + return returnWithin(state, [], canonicalHref(href)) +}