fix(mobile): follow-ups from the C1 stack review, one commit per finding (OTA phase C, C1.8) (#21570)

* fix(mobile): encode the host id the native list hands the shell

`web.tsx` encodes the host id into the pathname it opens the shell on; the
worktree-list route beside it still interpolated it raw. `useLocalSearchParams`
answers the decoded value, so a host id carrying `?`, `#` or whitespace builds a
pathname that is no longer one segment.

That shape is not refused where it is built. `matchesRoutePattern` splits on `/`
alone, so `/h/a?b` reads as the single segment `/h/[hostId]` names and the
session starts; the bridge's pathname rule is what refuses it, one `init` later,
and the shell turns that refusal into `document-load-failed`. The route ends on
a failure screen instead of the native list it already has and was about to
render anyway.

The fix sits at the interpolation rather than at the pattern or the bridge,
because the other two are right: the pathname rule is what a path may be, and
the page decodes the segment back when it matches `[hostId]`, so the screen it
opens is the same one. A deep link is the way such an id arrives, which is what
the sibling route's own test already establishes.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(mobile): end a route segment at the query, not only at a slash

The dot-segment lookahead both route patterns are built from treated `/` and
end-of-string as the only things that close a segment. An href may carry a
query, so the last segment can also be closed by `?`, and there the lookahead
never fired: `/h/..?x`, `/h/%2e%2e?x` and `/h/.?x` all passed
`BRIDGE_ROUTE_HREF_PATTERN` while their slash-terminated spellings were refused.

The sink is `router.push`, and a URL parser resolves `/h/..?x` to `/?x` exactly
as it resolves `/h/../x` to `/x`. That is the climb out of the `/h/` prefix the
rule exists to stop, reached through the one punctuation the rule did not treat
as a boundary.

Fixed in `BRIDGE_ROUTE_SEGMENT_SOURCE`, which is the single place the segment
rule is written and the reason the two patterns cannot drift apart. The pathname
pattern is unaffected: a `?` fails its character class wherever it appears, so
widening the boundary cannot let anything new through there. The existing
segment-rule block gains the query-terminated spellings beside the ones it
already pins.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* docs(mobile): say which notifies actually reach the mount-order throw

The header said a call before `init` is a mount-order bug and throws, and named
`notifyPageFault` as the one exception. Two more never reach that throw: a grant
is read off the session, so before `init` there is no grant either, and the `&&`
in `navigate` and `storage` short-circuits before `post` can require one.

The code is right and the comment was not, so the comment is what changed. False
is already these two members' refusal answer — it is what they give a shell that
withheld the grant — and both callers handle it. `useRouteHandoff` calls
`notifyNavigate` uncaught inside `push` and falls back to routing inside the
page, so making this path throw would turn an early tap into an unhandled error
in a handler nobody wrapped, which is the same reason the close path answers
inertly rather than throwing.

Pinned rather than left to the prose: the two gated notifies answer false and
post nothing before `init`, the two ungated ones still throw, and the gated ones
post once the shell has granted them. Not a red-first test — there is no defect
here to reproduce — but the contract now has a test holding it in place.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(mobile): re-arm the shell session the route rebuilt, not only the host

Two effects share one session. The first rebuilds it from `hostId` and
`routePathname`; the second is the only thing that ever tells the reducer what
the gates say, and it listed the host alone. A fresh session starts in
`checking` and moves on nothing but `gates-changed`, so a route that changed
under an unchanged host and unchanged gates threw the old session away and left
the new one with no effect to run and no verdict to wait for.

`routePathname` joins the gates effect's dependency list, beside the `hostId`
that is already there for the same reason: both are what rebuild the session
above, so both have to re-arm it. Fixing it in the dependency list rather than
by having the reducer restart on a repeat verdict keeps the reducer's rule
intact — a repeat verdict genuinely is nothing new — and keeps the coupling
stated where the coupling lives.

No caller can reach this today: both routes derive the pathname from the host
id, so the one cannot change without the other. The test drives the hook
directly and holds the invariant the wiring is supposed to have, since the thing
protecting it was a property of the call sites and not of this hook.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(mobile): persist the last-visited worktree through the mirrored writer

`writeLastVisitedWorktree` noted the write on the mirror and then dropped the
store's promise with `void`. The mirror reports the key as written the moment it
is noted, so a store that refuses the write leaves a value the page is handed on
every `init` and that nothing ever persisted, and the rejection escapes as an
unhandled one because no caller above it holds a catch.

`writeMirroredStorage` in the same module is already exactly this: note first,
persist second, and swallow the rejection deliberately, because a pin that
failed to persist is not a reason to take the workspace off screen. This writer
had grown its own copy of that pair without the last part. Reusing it rather
than adding a local `.catch` is what stops the two copies drifting again, and it
is the boundary that owns the relationship between the mirror and the store.

The test drives a store that refuses the write and listens for an unhandled
rejection, which is the failure the `void` produced and the only way to observe
it from inside a test.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* refactor(mobile): pin the re-arm oracle, and move the query rationale to the rule

Two review nits from round 1, neither changing behaviour.

The re-arm test asserted the session was no longer `checking`, which a failure
state satisfies just as well as a recovery does — the test would have passed on
the opposite of what it is for. It now pins `native-route`, which is the state a
re-armed session actually settles on here: `/h/host-1/tasks` is not the route
the bundle lists, so the reducer answers with the native screen.

The sentence about `?` closing a segment sat in the doc block for the `init`
pathname bounds, which opens by saying that pathname carries no query. Read
top to bottom the block contradicted itself. The rationale belongs beside
`BRIDGE_ROUTE_SEGMENT_SOURCE`, where the shared rule is written and where the
reason is legible: an href carries a query even though a pathname does not, both
are held to the one segment rule, and widening its boundary cannot loosen the
pathname pattern because a `?` fails that character class anywhere.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* docs(mobile): state what the native router really does with a dot segment

Round 2 review, comment text only.

The rationale beside `BRIDGE_ROUTE_SEGMENT_SOURCE` claimed `/h/..?x` resolves to
`/?x`, borrowing the climb `history.replaceState` performs on the `init`
pathname. That is the wrong sink. An href's sink is the native router, and
expo-router's `resolveHrefStringWithSegments` normalises only an href beginning
with `.`; a rooted one is passed through, its query stripped, and the forked
`getStateFromPath` then matches segments literally against the route patterns. A
dynamic segment compiles to `([^/]+\/)`, which takes `..` as happily as any
other value.

So the harm is not a climb and it is not Unmatched either: `..` is read as the
`[hostId]` a screen is opened for, and the shell opens a host screen for an id
no host has. A different wrong screen from the slash-terminated spellings, and
the same reason one rule covers both patterns. The boundary and the test that
pins it are unchanged; only the sentences describing them are.

The notify header opened by saying three of the four share one guard, one
paragraph above the one explaining that only two ever reach its throw. It now
says both in the same breath.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
This commit is contained in:
Jinwoo Hong
2026-09-18 23:26:28 -04:00
committed by GitHub
parent 030a1e0c77
commit 0817476b2c
10 changed files with 267 additions and 18 deletions
+5 -1
View File
@@ -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 (
<MobileWebShellScreen
hostId={hostId}
route={{ pathname: `/h/${hostId}` }}
route={{ pathname: `/h/${encodeURIComponent(hostId)}` }}
fallback={<HostScreen />}
/>
)
@@ -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})*/?)?`
@@ -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'
})
})
})
@@ -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.
*/
@@ -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)
@@ -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<string, string>
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<void> {
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)
}
})
})
@@ -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 () => {
@@ -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
])
@@ -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<unknown[]> {
const seen: unknown[] = []
const listener = (reason: unknown) => seen.push(reason)
process.on('unhandledRejection', listener)
try {
run()
await new Promise<void>((resolve) => setImmediate(resolve))
await new Promise<void>((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<void>((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' })
@@ -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))
}