diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index d0d0eb3e493..7322b69c5f1 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -15,6 +15,10 @@ import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobi * * `enabled === null` is the flag read still settling, and it renders the native screen: a store * build never reaches storage at all, so that is the only frame it ever paints here. + * + * Encoded, not interpolated raw, for the reason `web.tsx` states: a deep-linked host id carrying + * `?`, `#` or whitespace would build a pathname the page refuses, and a refusal here is a failure + * screen rather than the native list this route already has. */ function HostListScreen() { const { hostId } = useLocalSearchParams<{ hostId: string }>() @@ -26,7 +30,7 @@ function HostListScreen() { return ( } /> ) diff --git a/mobile/src/mobile-web-shell/bridge/bridge-caps.ts b/mobile/src/mobile-web-shell/bridge/bridge-caps.ts index 155b9ee9f66..d13519ea659 100644 --- a/mobile/src/mobile-web-shell/bridge/bridge-caps.ts +++ b/mobile/src/mobile-web-shell/bridge/bridge-caps.ts @@ -54,8 +54,18 @@ export const BRIDGE_MAX_ROUTE_PARAM_CHARS = 1024 * Exported as source rather than as a regex because it is embedded in more than one pattern: the * `init` pathname and the hrefs a page hands back to the shell are the same vocabulary, and two * spellings of it would be two rules that drift. + * + * Which is why the dot-segment lookahead ends a segment at `?` as well as at `/` and at the end of + * the string. A pathname carries no query, but an href does, so `/h/..?x` reaches the shared rule. + * The harm there is not the climb `replaceState` performs on the pathname: the href's sink is the + * native router, which resolves a dot segment only for an href beginning with `.` and otherwise + * matches segments literally, so `..` is taken as a value for `[hostId]` and the shell opens a host + * screen for an id no host has. Different screen, same reason to refuse it. + * + * Widening the boundary cannot loosen the pathname pattern, where a `?` fails the character class + * wherever it appears. */ -export const BRIDGE_ROUTE_SEGMENT_SOURCE = String.raw`(?!(?:\.|%2[eE]){1,2}(?:/|$))[^/\\?#\s]+` +export const BRIDGE_ROUTE_SEGMENT_SOURCE = String.raw`(?!(?:\.|%2[eE]){1,2}(?:[/?]|$))[^/\\?#\s]+` /** The path half both patterns start from: rooted, and made of segments that name something. */ const ROUTE_PATH_SOURCE = `/(?:${BRIDGE_ROUTE_SEGMENT_SOURCE}(?:/${BRIDGE_ROUTE_SEGMENT_SOURCE})*/?)?` diff --git a/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.test.ts b/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.test.ts index 516346ec7c9..c91af0995d5 100644 --- a/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.test.ts +++ b/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.test.ts @@ -1,6 +1,7 @@ /** The page's outbound notify surface: what it posts, what it stays quiet about, and what it * answers when the shell granted nothing or the port refused the frame. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { BridgeClientNotReadyError } from './bridge-client-errors' import { BRIDGE_FAULT_GRANT, BRIDGE_PROTOCOL_VERSION } from './bridge-envelope' import { GRANTS, INIT, createPageClient } from './bridge-page-client-test-harness' @@ -58,3 +59,42 @@ describe('bridge client page faults', () => { expect(page.diagnostics.map((diagnostic) => diagnostic.kind)).toContain('send-failed') }) }) + +/** + * Which notifies reach the mount-order throw, pinned because the grant check is what decides it. + * + * A grant is read off the session, so before `init` there is no grant either and the two gated + * notifies answer false without ever asking for the session. That is the answer their callers + * already handle, and it must stay the answer: `useRouteHandoff` calls `notifyNavigate` uncaught + * inside `push`, where a throw would take down a tap handler nobody wrapped. + */ +describe('the notify guard before init', () => { + it('answers false for the grant-gated notifies and posts nothing', () => { + const page = createPageClient() + // Against what the handshake already put on the port, so this counts the notifies alone. + const beforeNotifies = page.sent.length + expect(page.client.notifyNavigate('/h/host-1')).toBe(false) + expect(page.client.notifyStorageWrite('orca:last-visited-worktree', 'value')).toBe(false) + expect(page.sent).toHaveLength(beforeNotifies) + }) + + it('still throws for the ungated ones, which is the mount-order bug the guard is for', () => { + const page = createPageClient() + expect(() => page.client.notifyForeground()).toThrow(BridgeClientNotReadyError) + expect(() => + page.client.updateTerminalSubscriptionViewport('terminal-1', { cols: 80, rows: 24 }) + ).toThrow(BridgeClientNotReadyError) + }) + + it('posts the gated ones once the shell has granted them', () => { + const page = createPageClient() + page.deliver({ ...INIT, grants: { ...GRANTS, native: ['navigate', 'storage'] } }) + expect(page.client.notifyNavigate('/h/host-1')).toBe(true) + expect(page.frames().at(-1)).toEqual({ + v: BRIDGE_PROTOCOL_VERSION, + type: 'notify', + name: 'navigate', + href: '/h/host-1' + }) + }) +}) diff --git a/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.ts b/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.ts index bbb297ab2dc..30d448822fb 100644 --- a/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.ts +++ b/mobile/src/mobile-web-shell/bridge/bridge-client-notifications.ts @@ -9,13 +9,19 @@ import { captureBridgeError } from './bridge-error-capture' /** * Everything the page posts and hears nothing back about. * - * Three of the four share one guard, and it is not the same guard `sendRequest` uses. A call before - * `init` is a mount-order bug and throws; a call after `close` is an unmounting screen posting one - * more nudge on its way out, which the native clients answer inertly rather than by throwing into a - * teardown path nobody wrote a catch for. Nothing here returns a promise, so nothing here can be - * awaited into a rejection either. + * Three of the four post through one guard, but only two reach its throw, and it is not the guard + * `sendRequest` uses. A call before `init` is a mount-order bug and throws; a call after `close` is + * an unmounting screen posting one more nudge on its way out, which the native clients answer + * inertly rather than by throwing into a teardown path nobody wrote a catch for. Nothing here + * returns a promise, so nothing here can be awaited into a rejection either. * - * `notifyPageFault` is the exception and reads the session instead of requiring it: its one caller + * Only the two ungated notifies reach that throw. A grant is read off the session, so before `init` + * there is no grant either and `navigate` and `storage` answer false without asking: that is the + * same false they answer a shell that withheld the grant, and both callers already handle it — + * `useRouteHandoff` pushes inside the page instead, where a throw would take down a tap handler + * nobody wrapped. + * + * `notifyPageFault` reads the session instead of requiring it for a different reason: its one caller * is an error boundary, and a report that threw would replace the page's last word with an error * nobody catches. */ diff --git a/mobile/src/mobile-web-shell/bridge/bridge-envelope.test.ts b/mobile/src/mobile-web-shell/bridge/bridge-envelope.test.ts index be8930ae695..be451a0abc8 100644 --- a/mobile/src/mobile-web-shell/bridge/bridge-envelope.test.ts +++ b/mobile/src/mobile-web-shell/bridge/bridge-envelope.test.ts @@ -679,6 +679,15 @@ describe('the segment rule both route patterns are built from', () => { } }) + it('refuses a trailing dot segment the query is what ends, not a slash', () => { + // The `notify` sink is `router.push`, which does not resolve these: it matches segments + // literally, so `..` becomes the `[hostId]` a screen is opened for. A different wrong screen + // from the spellings above, and the same reason one rule covers both patterns. + for (const spelling of ['/h/..?x', '/h/%2e%2e?x', '/h/.?x', '/h/a/..?x', '/h/..?']) { + expect(BRIDGE_ROUTE_HREF_PATTERN.test(spelling), spelling).toBe(false) + } + }) + it('takes an escape that is part of a name, in either position', () => { expect(BRIDGE_ROUTE_PATHNAME_PATTERN.test('/h/a%20b/%2ex/a%2fb')).toBe(true) expect(BRIDGE_ROUTE_HREF_PATTERN.test('/h/a%20b/%2ex?from=list')).toBe(true) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx new file mode 100644 index 00000000000..c93a835d9b7 --- /dev/null +++ b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx @@ -0,0 +1,76 @@ +import { createElement } from 'react' +import { act, create } from 'react-test-renderer' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +type RouteDependencies = { + storage: Map + pathnames: string[] + hostId: string +} + +const dependencies = vi.hoisted((): RouteDependencies => ({ + storage: new Map(), + pathnames: [], + hostId: 'host-1' +})) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: { + getItem: async (key: string) => dependencies.storage.get(key) ?? null, + setItem: async (key: string, value: string) => { + dependencies.storage.set(key, value) + } + } +})) + +vi.mock('expo-router', () => ({ + useLocalSearchParams: () => ({ hostId: dependencies.hostId }) +})) + +vi.mock('../components/WorkspaceDetailPlaceholder', () => ({ + WorkspaceDetailPlaceholder: () => null +})) + +vi.mock('../host-screen/HostScreen', () => ({ HostScreen: () => null })) + +vi.mock('../layout/responsive-layout', () => ({ + useResponsiveLayout: () => ({ isWideLayout: false }) +})) + +vi.mock('./MobileWebShellScreen', () => ({ + MobileWebShellScreen: (props: { hostId: string; route: { pathname: string } }) => { + dependencies.pathnames.push(props.route.pathname) + return null + } +})) + +import { BRIDGE_ROUTE_PATHNAME_PATTERN } from './bridge/bridge-caps' +import HostWorktreeRoute from '../../app/h/[hostId]/index' + +async function renderRoute(): Promise { + await act(async () => { + create(createElement(HostWorktreeRoute)) + }) +} + +describe('the native worktree-list route that hands off to the shell', () => { + beforeEach(() => { + dependencies.storage.clear() + dependencies.pathnames.length = 0 + dependencies.hostId = 'host-1' + Object.assign(globalThis, { __DEV__: true }) + dependencies.storage.set('orca:mobileWebShellEnabled', 'true') + }) + + it('encodes the host id into the pathname, like the shell route already does', async () => { + for (const hostId of ['a?b', 'a#b', 'a b', 'a/b', 'a\\b']) { + dependencies.hostId = hostId + dependencies.pathnames.length = 0 + await renderRoute() + const pathname = dependencies.pathnames[0] + expect(pathname, hostId).toBe(`/h/${encodeURIComponent(hostId)}`) + expect(BRIDGE_ROUTE_PATHNAME_PATTERN.test(pathname ?? ''), hostId).toBe(true) + expect(decodeURIComponent((pathname ?? '').slice('/h/'.length)), hostId).toBe(hostId) + } + }) +}) diff --git a/mobile/src/mobile-web-shell/use-mobile-web-shell-session.test.ts b/mobile/src/mobile-web-shell/use-mobile-web-shell-session.test.ts index 8860eca877e..f6fa2b2047f 100644 --- a/mobile/src/mobile-web-shell/use-mobile-web-shell-session.test.ts +++ b/mobile/src/mobile-web-shell/use-mobile-web-shell-session.test.ts @@ -477,6 +477,56 @@ describe('the wait for the page to speak', () => { }) }) + it('re-arms a session the route rebuilt, with the host and its gates unchanged', async () => { + // The route is the other half of the session identity: changing it throws the old session away, + // and a session nobody told the gates about never leaves `checking`. + const fake = createFakeStore() + const route = { pathname: '/h/host-1' } + const seen: MobileWebShellSessionState[] = [] + function Probe() { + const session = useMobileWebShellSession({ + hostId: HOST_ID, + routePathname: route.pathname, + runtime: { + createStore: () => fake.store, + mintSessionId: () => 'session-id', + now: () => 0, + setTimer: createTimerSeam().setTimer + } + }) + seen.push(session.state) + return null + } + const rendered: { tree: ReactTestRenderer | null } = { tree: null } + await act(async () => { + rendered.tree = create(createElement(Probe)) + }) + const tree = rendered.tree + if (tree === null) { + throw new Error('the hook did not mount') + } + await act(async () => { + fake.settleCacheRead(null) + }) + route.pathname = '/h/host-1/tasks' + seen.length = 0 + await act(async () => { + tree.update(createElement(Probe)) + }) + // The rebuilt session must open the cache of its own accord; settling a read it never asked + // for leaves it in `checking`, which is exactly what an un-armed session looks like. + await act(async () => { + fake.settleCacheRead(null) + }) + await flush() + // Pinned, not merely "moved on": `/h/host-1/tasks` is not the route the bundle lists, so a + // re-armed session settles on the native screen. A failure would also leave `checking`. + expect(seen.at(-1)?.kind).toBe('native-route') + await act(async () => { + tree.unmount() + }) + }) + it('cancels the armed deadline when the session it belongs to is torn down', async () => { const mounted = await ready() await act(async () => { diff --git a/mobile/src/mobile-web-shell/use-mobile-web-shell-session.ts b/mobile/src/mobile-web-shell/use-mobile-web-shell-session.ts index bc453f4a669..ff3ffcf3728 100644 --- a/mobile/src/mobile-web-shell/use-mobile-web-shell-session.ts +++ b/mobile/src/mobile-web-shell/use-mobile-web-shell-session.ts @@ -195,15 +195,16 @@ export function useMobileWebShellSession(args: { hostStatus: hostProtocolWindow } }) - // `hostId` is in the list for the host whose gates read identically to the last one's: the - // reducer now starts nothing on a repeat verdict, so a session that never re-armed would sit - // in `checking` forever. + // `hostId` and `routePathname` are in the list because they are what rebuilds the session + // above: the reducer starts nothing on a repeat verdict, so a fresh session nobody re-armed + // would sit in `checking` forever. Both, not just the host, because either one rebuilds it. }, [ dispatch, hostCapabilities, hostId, hostProtocolWindow, reachability, + routePathname, statusPending, statusReadable ]) diff --git a/mobile/src/worktree/last-visited-worktree-repo.test.ts b/mobile/src/worktree/last-visited-worktree-repo.test.ts index eebbf87754d..138f579edf1 100644 --- a/mobile/src/worktree/last-visited-worktree-repo.test.ts +++ b/mobile/src/worktree/last-visited-worktree-repo.test.ts @@ -1,9 +1,65 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' + +type StorageDouble = { rejection: unknown; written: string[] } + +const storage = vi.hoisted((): StorageDouble => ({ rejection: null, written: [] })) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: { + setItem: async (_key: string, value: string) => { + if (storage.rejection !== null) { + throw storage.rejection + } + storage.written.push(value) + }, + removeItem: async () => undefined + } +})) + import { readLastVisitedWorktreeRecord, - readLastVisitedWorktreeRepoId + readLastVisitedWorktreeRepoId, + writeLastVisitedWorktree } from './last-visited-worktree-repo' +/** Node reports an unhandled rejection at the end of a microtask checkpoint, so one macrotask is + * long enough to see it, and a listener is the only way to observe one from inside a test. */ +async function unhandledRejectionsWhile(run: () => void): Promise { + const seen: unknown[] = [] + const listener = (reason: unknown) => seen.push(reason) + process.on('unhandledRejection', listener) + try { + run() + await new Promise((resolve) => setImmediate(resolve)) + await new Promise((resolve) => setImmediate(resolve)) + } finally { + process.off('unhandledRejection', listener) + } + return seen +} + +// Why: the mirror reports this key as written the moment it is noted, so a store write that +// rejects must be handled where it is made. Nothing above it is holding a catch. +describe('writeLastVisitedWorktree', () => { + it('handles a store that refuses the write instead of leaving the rejection loose', async () => { + storage.rejection = new Error('quota exceeded') + const loose = await unhandledRejectionsWhile(() => { + writeLastVisitedWorktree({ hostId: 'host-1', worktreeId: 'repo-2::/tmp/worktree' }) + }) + storage.rejection = null + expect(loose).toEqual([]) + }) + + it('still persists the record when the store takes it', async () => { + storage.written.length = 0 + writeLastVisitedWorktree({ hostId: 'host-1', worktreeId: 'repo-2::/tmp/worktree' }) + await new Promise((resolve) => setImmediate(resolve)) + expect(storage.written).toEqual([ + JSON.stringify({ hostId: 'host-1', worktreeId: 'repo-2::/tmp/worktree' }) + ]) + }) +}) + describe('last visited worktree repo', () => { it('extracts the repo id for the current host', () => { const raw = JSON.stringify({ hostId: 'host-1', worktreeId: 'repo-2::/tmp/worktree' }) diff --git a/mobile/src/worktree/last-visited-worktree-repo.ts b/mobile/src/worktree/last-visited-worktree-repo.ts index f904833e7c3..a865e7a48d6 100644 --- a/mobile/src/worktree/last-visited-worktree-repo.ts +++ b/mobile/src/worktree/last-visited-worktree-repo.ts @@ -1,5 +1,4 @@ -import AsyncStorage from '@react-native-async-storage/async-storage' -import { noteMirroredWrite } from '../storage/mirrored-storage-keys' +import { writeMirroredStorage } from '../storage/mirrored-storage-keys' import { getRepoIdFromMobileWorktreeId } from '../session/mobile-session-route-helpers' export const LAST_VISITED_WORKTREE_STORAGE_KEY = 'orca:last-visited-worktree' @@ -57,7 +56,5 @@ export function readLastVisitedWorktreeRepoId(raw: string | null, hostId: string * would open on the repo the user left rather than the one they just came from. */ export function writeLastVisitedWorktree(record: LastVisitedWorktreeRecord): void { - const value = JSON.stringify(record) - noteMirroredWrite(LAST_VISITED_WORKTREE_STORAGE_KEY, value) - void AsyncStorage.setItem(LAST_VISITED_WORKTREE_STORAGE_KEY, value) + writeMirroredStorage(LAST_VISITED_WORKTREE_STORAGE_KEY, JSON.stringify(record)) }