diff --git a/docs/mobile-relay-ux-findings.md b/docs/mobile-relay-ux-findings.md new file mode 100644 index 000000000..3fbbd1419 --- /dev/null +++ b/docs/mobile-relay-ux-findings.md @@ -0,0 +1,442 @@ +# Mobile Relay UX — Investigation Findings & Fix Plan + +Scope: phone-side presentation/state-machine issues behind three reported symptoms on Android over +the cloud relay. The relay protocol and server-side assignment are healthy; nothing here changes +desktop or relay-server code. All file references are in `mobile/` of this worktree. + +## 1. Symptom → root-cause summary + +| # | Symptom | Root cause (verified) | +|---|---------|----------------------| +| S1 | Resume lands on an empty "Host" page, grey dot | Bare cross-stack `router.push` into a cold nested host navigator resolves to the host index route **without the `hostId` param**; every screen below then runs with `hostId: undefined` | +| S2 | Tapping a healthy relay host shows grey 1–2s before green | Every screen focus funnels into the network-handoff recovery path, which **suspends the healthy relay session** (publishes `disconnected`) and re-dials; the re-dial is invisible because `migrateTo` binds new-session state only after authentication | +| S3 | Relay-forced pairing looks dead ~5–10s | The pairing relay path has **no log sink** (only direct-path entries reach the "Pairing log"), and post-pairing the app dials the unreachable LAN endpoint for up to 12s before relay recovery is even eligible | + +## 2. Verified end-to-end causal chains + +### S1 — Resume dead-ends on the host index page + +1. Home renders the Resume card only once `hostStates[lastVisited.hostId] === 'connected'` + (`app/index.tsx:488`); over relay that is seconds after the host list paints, and the card + inserts **above** the Tasks card in the same footer (`app/index.tsx:733-780`) — a layout shift + under the thumb. +2. Tap → bare `router.push(createMobileSessionHref(...))` (`app/index.tsx:740-746`) targeting + `/h/[hostId]/session/[worktreeId]`. +3. With the `h` group cold (cold start, or host never visited this session), Expo Router resolves + the push to the host stack's **index route with no `hostId` param**. This exact failure mode is + documented twice in-repo ("cold Expo deep links resolve to index" — + `src/transport/host-edit-navigation.ts:52`, `src/tasks/mobile-task-navigation.ts:90`) and is the + root cause named by PR #12001. +4. `app/h/_layout.tsx:64` reads `hostId` via `useGlobalSearchParams` → `undefined`. + `HostProtocolGate` gets `hostId: undefined`; `useHostClient(undefined)` returns + `state: 'disconnected'` (`src/transport/client-context.tsx:344`) → **grey dot**. +5. The host index screen renders the fallback title `'Host'` (`app/h/[hostId]/index.tsx:821`), and + every fetch no-ops on `!client || connState !== 'connected'` + (`app/h/[hostId]/index.tsx:298,364,418,520`) → **empty list**. The Filter/Recent/Repo chips are + static toolbar UI, so the page looks "real" but dead. +6. "Sometimes": a warm host stack resolves the same push correctly, so the bug is intermittent by + navigation history. + +Corrections to the preliminary sweep: the empty page is primarily the missing `hostId` param, not +the connection-gated fetches or the cold 30s worktree cache (those matter only when landing *with* +a valid `hostId`, e.g. the mistap-strand case). Also, the Resume target "validation" is weaker than +it looks: `getCachedWorktrees` is seeded from the persisted home snapshot at hydration +(`app/index.tsx:264-277`) and the 30s TTL is stamped at seed time (`src/cache/worktree-cache.ts:20`), +so a worktree deleted while the phone was off still passes until a live `worktree.ps` overwrites it. + +**Fix**: PR #12001 ("open the Resume workspace through a mounted host stack") routes Resume through +the same mount-then-replace mechanism Tasks uses, extracted to `src/navigation/host-stack-navigation.ts`. +Reviewed and validated per Jinwoo; **merged to main as `7948e46db855`** after final validation +(see §4, F0). Residual S1 items it does not cover: bare notification/accounts/deep-link pushes (F4), +catalog validation + not-found bounce (F7), Resume-card layout shift (F8), gate unmount hazard (F9). + +### S2 — grey blink when focusing a healthy relay host + +1. Every focus of the host screen fires `notifyForeground()` + (`app/h/[hostId]/index.tsx:512-517`, deliberately empty deps). +2. `openHostLogicalClient` wraps that into `endpointLifecycle.setForeground(true)` + (`src/transport/host-logical-client.ts:31-33`); the lifecycle forwards without dedupe + (`src/transport/mobile-endpoint-lifecycle.ts:62-64`). +3. `MobileEndpointSupervisor.setForeground(true)` computes `wasForeground = true` and calls + `RelayReconnectController.handleForeground` (`src/transport/mobile-endpoint-supervisor.ts:115-119`). +4. `handleForeground` with `wasForeground && state === 'connected'` **suspends the healthy session** + (`src/transport/mobile-relay-reconnect-controller.ts:53-60`). `suspendActiveRelay` early-returns + unless the active path is `'relay'` (`:77-84`) — which is why LAN hosts never blink. +5. `suspendActiveSession` closes the physical session, disposes all subscriptions, and publishes + `'disconnected'` (`src/transport/stable-logical-rpc-client.ts:164-180`) → grey dot + (`src/components/StatusDot.tsx:11`), worktree queries blocked. +6. `onRetry()` → `recoverRelay()` → `openRelay` + `migrateTo`. During the dial the logical state + **stays 'disconnected'**: `migrateTo` only binds the new session's state after + `waitForAuthenticated` resolves (`src/transport/stable-logical-rpc-client.ts:188,213`), and the + dialing session's own `connecting`/`handshaking` publishes (`src/transport/mobile-relay-rpc-session.ts:43,74`) + fire with no listeners attached. Grey persists the full 1–2s (happy path has no artificial + delays; any failure adds ≥250ms full-jitter backoff, `src/transport/mobile-relay-retry-delays.ts:3-5`). +7. `migrateTo` completes → `'connected'` → green; subscriptions replay; gated fetches rerun. + +Second trigger for the same path: OS network-revival nudges call `notifyForeground()` on every live +client (`src/transport/client-context.tsx:286-292`, `src/transport/connection-revival-triggers.ts`) +— any Wi-Fi↔cellular transition or came-online event grey-blinks every connected relay host. + +Design context: the suspend-on-repeat-foreground is pinned by the supervisor test as the +network-handoff half-open case (`src/transport/mobile-endpoint-supervisor.test.ts:~160-197`). The +asymmetry is that **direct sockets probe instead of tearing down** — `notifyForeground` on a +connected direct client runs an activity probe that detects a half-open socket in ≤8s +(`src/transport/rpc-client.ts:1119-1124`) — while relay sessions have a no-op `notifyForeground` +(`src/transport/mobile-relay-rpc-session.ts:106`) and the supervisor's only tool is +suspend-then-redial. There is also an in-repo make-before-break precedent: lease rotation calls +`recoverRelay(forceReplacement = true)` and migrates a **live** session with zero visible blink +(`src/transport/mobile-endpoint-supervisor.ts:56-59,146,249`). + +Divergent mount defaults (secondary): home renders `hostStates[id] ?? 'connecting'` (amber, +`app/index.tsx:707`) while `getState()`/`useHostClient` return `'disconnected'` (grey) for a +missing store entry (`src/transport/client-context.tsx:221,344`) — so host screens flash grey +during the async client acquire (Keychain read) that home never shows. + +### S3 — silent 5–10s relay-forced pairing + +Pairing phase: + +1. `pair-confirm.tsx` / `pair-scan.tsx` pass `connectOptions.onLog` into `startPreProfilePairing` + (`app/pair-confirm.tsx:91-99`, `app/pair-scan.tsx:135-143`). +2. The coordinator threads it **only to the direct candidate** + (`src/transport/pre-profile-pairing-coordinator.ts:152-157`). The relay candidate + (`:161-187`) gets nothing: `connectMobileRelayForPairing` has no log parameter at all + (`src/transport/mobile-relay-physical-client.ts:22-30`), nor do the director resolution, + journal writes, or the recovery loop in `src/transport/pairing-relay-candidate.ts`. +3. With LAN unreachable, the visible "Pairing log" shows only the direct dial stalling toward its + 12s connect timeout while the relay path does the real work silently: cell WebSocket + E2EE + handshake + `pairing.provisionRelay` + `pairing.getEndpoints` + credential-bundle write + (`pre-profile-pairing-coordinator.ts:206-233`). The error copy even says "see log below for + where it stalled" (`app/pair-confirm.tsx:138`) — the log cannot show it. +4. Un-logged waits in the relay recovery loop: each of up to 3 attempts wraps a 5s director + resolution (`src/transport/mobile-relay-invite-director.ts:16`) plus full-jitter sleeps capped + at 100/200/400ms (`src/transport/pairing-relay-candidate.ts:58-59,70-71`) — worst case ~15s of + silence. (Correction: the preliminary "~3×2s of backoff" was wrong; the sleeps are small, the + director resolves dominate.) The relay E2EE layer itself has **no timers**: a pairing relay + request is unbounded except the screen's 25s cap (`app/pair-confirm.tsx:27`). +5. Pairing logs also never reach `connectionLogStore` (single producer: + `src/transport/client-context.tsx:132`), so the Connection Log screen shows nothing about a + pairing that just failed. + +Post-pairing phase: + +6. `pair-confirm` calls `closeHost(hostId)` then replaces to `/h/` + (`app/pair-confirm.tsx:118-123`). +7. The destination re-acquires a client asynchronously (grey `'disconnected'` default during the + Keychain read, `src/transport/client-context.tsx:221`), then dials the **LAN endpoint first** + (`src/transport/host-logical-client.ts:12`) — amber for up to `CONNECT_TIMEOUT_MS = 12s` + (`src/transport/rpc-client.ts:126`) on a black-holed LAN. +8. Relay recovery cannot start earlier: `needsRecovery` treats `connecting`/`handshaking` as live + progress (`src/transport/mobile-relay-reconnect-controller.ts:73-75`), checked at supervisor + start and on every retry (`src/transport/mobile-endpoint-supervisor.ts:106,146`). +9. When the direct dial finally fails, the relay dial runs invisibly (same `migrateTo` mechanism + as S2) → green. Worst case with a director resolution failure and grace-credential retry: + ~29–58s under the old session's labels. +10. The "Orca Relay" path label only renders once `state === 'connected'` + (`src/components/MobileHostCard.tsx:23,47`) — the user learns the phone is using relay only + after the wait ends, and `classifyConnection` has no relay-aware branch + (`src/transport/connection-health.ts:45-100`). + +## 3. Anti-pattern sweep + +### (a) Uncoordinated deep pushes into `/h` from outside the host stack + +Coordinated today (mount-then-replace): host edit (`src/transport/host-edit-navigation.ts`) and +Tasks (`src/tasks/mobile-task-navigation.ts`). Note host-edit's predicate is weaker — it checks the +root route only and can fire its `replace` while the nested stack is still gated/unmounted; Tasks +proves the nested stack exists (`mountedHostStack`, `mobile-task-navigation.ts:53-70`). + +Bare pushes remaining (host stack plausibly cold at each): + +| Call site | Target | Cold scenario | +|---|---|---| +| `app/_layout.tsx:127` via `src/notifications/notification-routing.ts:58,64` | `/h//session/` or `/h/` | **Coldest path** — `getLastNotificationResponse()` after launch from a killed app, plus the warm listener | +| `app/index.tsx:740-746` (Resume) | `/h/[hostId]/session/[worktreeId]` | Fixed by PR #12001 | +| `app/index.tsx:835` (Account-usage card) | `/h//accounts` | Home is the root route | +| `orca://` deep links (scheme in `app.json:9`, no linking config) | any `/h/...` | Default filesystem linking, zero coordination; `app/_layout.tsx:52-58` only intercepts pairing codes | +| `app/h/[hostId]/history/[worktreeId].tsx:15`, `pr/[worktreeId].tsx:16` | redirect to source-control | A cold deep link to these hits the same cold-navigator resolution first | + +Shallow index-only pushes (`app/index.tsx:723,803`, onboarding/pair flows) don't need coordination. +No ``, `navigationRef`, or `router.navigate` anywhere in `mobile/`. + +Related hazard: `HostProtocolGate` **unmounts the mounted HostStack mid-connect** for a first-visit +host — stack mounts while `connecting`, is replaced by a spinner when `status.get` goes in flight +(`statusPending` true only when connected: `src/transport/host-status-gates.ts:111`), then remounts +(`src/components/HostProtocolGate.tsx:34-44`). A deep navigation that resolved into the first mount +can be destroyed by the gate cycle. + +### (b) Surfaces that render grey 'disconnected' during expected transients + +Store defaults: every read API on the canonical store defaults to `'disconnected'` for a missing +entry — `getState` (`src/transport/client-context.tsx:221`), `useHostClient` seed/re-seed/unbound +fallback (`:343-345,377,393`). Exactly one call site defaults to amber instead: the home screen's +`hostStates[id] ?? 'connecting'` (`app/index.tsx:707,903`), whose reconciliation effect also +refuses to write `'disconnected'` for a never-tracked host (`:378-394`) — home already solved +locally what every other surface gets wrong. + +The grey window is not one frame: `openEntry` awaits `loadHosts()` (a Keychain/SecureStore pass) +**before** inserting the store entry (`client-context.tsx:87-161`, insert at `:153`), so a cold +start or deep link into `/h/[hostId]` shows grey for the whole Keychain latency. The physical +client is not the cause — it already reports `'connecting'` synchronously by the time `connect()` +returns (`rpc-client.ts:302,967`). Additionally, `forceReconnect` deletes the entry then awaits the +async reopen (`client-context.tsx:201-218`), so **every Retry button drives the UI grey before +amber**. + +Surfaces that show grey / "disconnected" copy for a healthy host during these transients (all via +`useHostClient`): host header dot (`app/h/[hostId]/index.tsx:819`); host toolbar + FAB disabled +(`:850-860,941-1000,1063-1078,1209`); the workspace list body renders **nothing at all** for +`disconnected` — `selectHostWorkspaceListState` falls through to `null`, not even a spinner +(`src/worktree/host-workspace-list-state.ts:17-24`); tasks header dot + "Connect to a host" empty +state (`app/h/[hostId]/tasks.tsx:8681,8661-8663`); session dot (no `verdict` prop at all, +`session/[worktreeId].tsx:4434`) and the literal "Disconnected" chip (`:4246-4255`); native-chat +composer lock (`src/session/MobileNativeChatView.tsx:427-432`); source-control / git-history / +diff-review / file-explorer / agent-history "Waiting for desktop…" states; the connection-log +screen prints the raw enum (`app/connection-log.tsx:113-117`); home's Resume/Accounts/Tasks/Quick +Action gates all read `=== 'connected'`; voice settings goes fully inert +(`app/voice-settings.tsx:50-55`). Counter-examples that behave well: home host card, the accounts +screen ("Connecting to {host}…" + cached snapshot, `app/h/[hostId]/accounts.tsx:366-370`). + +Deliberate transients that publish `'disconnected'` while healthy work proceeds: relay suspend on +focus/network nudges (S2); background suspend (`src/transport/mobile-endpoint-supervisor.ts:123` — +correct per billing, but state stays grey through the entire foreground re-dial rather than +flipping to `'connecting'`); post-migration cleanup (`:251-253`); `closeHost` during the +pair-confirm handoff (`src/transport/client-context.tsx:83`); three open-failure paths +(`:107,114,135`). + +Destructive companion pattern — state flips don't just recolor, they **wipe loaded data**: +`host-status-gates.ts:32,100-112` wipes cached host capabilities on every disconnect; +`tasks.tsx:2774-2800` resets the whole screen's hydration and force-closes ~15 sheets; +`session/[worktreeId].tsx:2043,2432-2439,3713-3716` clears diff comments/capability flags/agent +lists; the PR sidebar hides entirely (`src/session/use-mobile-pr-branch-context.ts:59-66` → +`use-mobile-pr-sidebar-controller.ts:113-118`); git history blanks rows on the **reconnect** branch +(`src/source-control/MobileGitHistoryList.tsx:63-68`); the repo cache survives disconnect but is +wiped by the rejected in-flight call (`NewWorktreeModal.tsx:330-333`); the worktree cache is read +only at mount/hostId change (`app/h/[hostId]/index.tsx:129,330`), never on reconnect, so a >30s +entry means an empty remount. + +Same bug class in a second enum: `workspaceSshStatusLabel` defaults a `null` SSH status to +"Disconnected" (`src/tasks/workspace-ssh-gate.ts:14-37`, rendered in `NewWorktreeModal.tsx:863` +and `tasks.tsx:10970`). + +### (c) Invisible relay establishment phases + +- `connectMobileRelayRpcSession` (normal relay connects) has **no onLog** — the entire relay + session lifecycle emits nothing (`src/transport/mobile-relay-rpc-session.ts:30-39`); the + supervisor logs only coarse post-hoc lines (`mobile-endpoint-supervisor.ts:185-188,261`). +- `migrateTo` structurally discards the dialing session's `connecting`/`handshaking` states + (`src/transport/stable-logical-rpc-client.ts:182-223,267-299`). +- Direct→relay upgrade path has no sink at all (`src/transport/mobile-endpoint-lifecycle.ts:49-58`, + `mobile-relay-direct-upgrade-controller.ts:19`). +- Pairing relay path fully silent (S3 above); pairing logs never reach `connectionLogStore`. +- Path label ("Orca Relay") gated on `connected` (`src/components/MobileHostCard.tsx:47`); + `classifyConnection` collapses `connecting`/`handshaking`/`reconnecting` and has no relay branch. +- Regression suite for connect-label stalls exists for the direct path only + (`src/transport/cellular-connecting-label-stall.test.ts`); no relay equivalent. + +## 4. Fix plan + +Ordered by felt-flakiness-removed per unit risk. All fixes are phone-local; none change the wire +protocol, so every old/new phone × old/new desktop pairing keeps working unless noted. + +### F0 (S1, quick win) — land PR #12001 ✅ MERGED + +Squash-merged to main as `7948e46db855` (2026-08-04) after validation: CI fully green; drift check +against current main clean (only overlap, #12575, touches different regions of `app/index.tsx` and +auto-merges); full mobile suite (411 files, 3110 tests) passed on a local merge of main into the PR +branch. The PR routes Resume through the shared mount-then-replace mechanism +(`src/navigation/host-stack-navigation.ts`) and adds a source-guard test against reintroducing the +bare push. This branch has since been fast-forwarded onto that merge, and F4 builds on the +extracted module. +Backward compat: navigation-only, none. +Residuals tracked as F4/F7/F8/F9. + +### F1 (S2, quick win) — stop suspending a healthy relay on focus ✅ IMPLEMENTED (this branch) + +Approach: split the nudge reasons that today all funnel into `setForeground(true)`: + +- Screen-focus nudge (`app/h/[hostId]/index.tsx:515`): must not suspend. For the relay path, either + no-op (state changes already drive the UI) or run a cheap liveness probe (an RPC with a short + budget) and only enter recovery on failure — mirroring the direct path's activity probe. +- Network-change / app-resume nudges: keep half-open protection, but **verify by replacement** + instead of break-before-make: call the existing `recoverRelay(forceReplacement = true)` path + (proven by lease rotation) so `migrateTo` swaps sessions with the dot staying green; only if the + replacement dial fails, fall back to `suspendActiveRelay` so a genuinely dead link stops lying + green and the retry loop re-arms (plain `recoverRelay` early-returns while the stale state is + still `'connected'`, so the fallback suspend is required for convergence). + +Files: `src/transport/mobile-relay-reconnect-controller.ts` (`handleForeground`), +`src/transport/mobile-endpoint-supervisor.ts` (thread a nudge reason; failure-path suspend), +`src/transport/mobile-endpoint-lifecycle.ts`, `src/transport/host-logical-client.ts` (reason-tagged +`notifyForeground`), optionally `src/transport/rpc-client.ts` type for the reason parameter. +Risk: PEER_DROPPED/LIMIT_EXCEEDED churn if replacement dials overlap — reuse the existing +`shouldDefer` cooldown; billed duplicate socket for the overlap window (lease rotation already +accepts this). Half-open regression risk is covered by the fallback suspend. +Tests: split `mobile-endpoint-supervisor.test.ts:~160-197` into (focus nudge → no suspend, dot +stays green) and (network handoff → replacement dial; failure → suspend + cooldown). Keep the +background-suspend test unchanged. + +### F2 (S2/S3, quick win) — unify mount defaults to 'connecting' ✅ IMPLEMENTED (this branch) + +Approach: `getState(hostId)` returns `'connecting'` when the host is known (primed profile or +pending open) and no entry exists yet; `'disconnected'` only for unknown/closed hosts. Aligns every +host screen with home's `?? 'connecting'`. Two companion changes in the same class: +- `forceReconnect` should notify `'connecting'` (or insert a placeholder entry) instead of leaving + the deleted-entry window grey (`src/transport/client-context.tsx:201-218`) — every Retry button + currently drives the UI grey before amber. +- Optionally have `openEntry` insert a `'connecting'` placeholder before the Keychain read so the + cold-start gap (`client-context.tsx:87-153`) is amber too. + +**Required interaction fix**: `app/h/[hostId]/index.tsx:738-741` falls back to +`lastKnownWorktrees` only for `disconnected | reconnecting | auth-failed`; `connecting`/ +`handshaking` fall through to the live (empty on fresh mount) array. Flipping the default without +extending that predicate would silently disable the stale-list fallback and blank the list — +extend it to every not-connected state (or key it on "no live fetch has succeeded this mount"). +Files: `src/transport/client-context.tsx` (`getState`, `useHostClient`, `forceReconnect`, +`openEntry`), `app/h/[hostId]/index.tsx` (fallback predicate), +`src/worktree/host-workspace-list-state.ts` (render a spinner for the not-connected states instead +of `null`). +Risk: a permanently unreachable host now shows amber briefly before the verdict system escalates — +acceptable; `classifyConnection` already owns escalation. Audit the §3(b) "wipe" sites for any that +key on `'disconnected'` specifically. +Tests: `client-context.test.ts` known-vs-unknown host defaults + forceReconnect state sequence; +host screen test for the stale-list fallback under `'connecting'`. + +### F3 (S3, quick win) — give the pairing relay path a log sink ✅ IMPLEMENTED (this branch) + +Approach: add an optional `onLog` to `connectMobileRelayForPairing`, +`createRecoveringPairingRelayCandidate`, and `resolvePairingInviteThroughDirector`; thread +`connectOptions.onLog` from the coordinator to the relay candidate; emit phase lines ("relay: +resolving director…", "relay: cell connected", "relay: E2EE handshake…", "relay: authenticated", +"relay: installing credential…"). Optionally also append pairing logs into `connectionLogStore` +under the resolved host id so the Connection Log screen has a record post-pairing. +Files: `src/transport/mobile-relay-physical-client.ts`, `pairing-relay-candidate.ts`, +`mobile-relay-invite-director.ts`, `pre-profile-pairing-coordinator.ts`. +Risk: none (additive, phone-local). Old desktops: unaffected — logging only. +Tests: coordinator test asserting relay-path log entries arrive through `connectOptions.onLog`; +extend `pairing-relay-candidate.test.ts` for per-attempt lines. + +### F4 (S1 class, quick win after F0) — coordinate the remaining bare deep pushes + +Approach: route notification taps (`app/_layout.tsx:127` + `src/notifications/notification-routing.ts`) +and the Account-usage card (`app/index.tsx:835`) through `src/navigation/host-stack-navigation.ts` +once #12001 lands; migrate host-edit onto the same stricter mechanism (#12001's own noted +follow-up). `orca://` deep links can follow later via a route-level guard. +Risk: notification cold-start ordering (push before root nav ready) — the mechanism already +tolerates that by waiting for state commits. +Tests: reuse the `host-stack-navigation.test.ts` harness for a notification-shaped target. + +### F5 (S2/S3, deeper) — make relay dials visible through `migrateTo` + +Approach: while the logical client is `suspended`/`'disconnected'`, have `migrateTo` forward the +dialing session's state publishes (`connecting`/`handshaking`) to `publishState`, unbinding on +success (normal bind takes over) or failure (restore `'disconnected'`). Guard: never downgrade a +still-`'connected'` previous session (make-before-break migrations must stay green). Follow-on UI: +show the path being dialed ("Connecting via Orca Relay…") by exposing the pending path, and let +`MobileHostCard`/`classifyConnection` render it while not yet connected. +Files: `src/transport/stable-logical-rpc-client.ts` (+ its test), `src/transport/connection-health.ts`, +`src/components/MobileHostCard.tsx`, `src/transport/mobile-connection-path-label.ts`. +Risk: state-ordering regressions in the pinned stable-client and connecting-label suites; keep the +forwarding strictly gated on suspended/disconnected. +Tests: add a relay-path analog of `cellular-connecting-label-stall.test.ts`; stable-client cases: +forwarded states during suspended dial, no forwarding during live-session replacement, failure +restores `'disconnected'`. + +### F6 (S3, deeper) — happy-eyeballs relay start post-pairing + +Approach: when a relay credential bundle exists and the direct dial has not authenticated within a +short grace (2–3s), start the relay dial in parallel instead of waiting for the 12s direct failure; +first authenticated path wins via the existing `migrateTo`/hysteresis machinery. Scope initially to +the first connect after pairing (or hosts whose last success was relay) to avoid pointless relay +sockets on healthy LANs. +Files: `src/transport/mobile-endpoint-supervisor.ts` (start/needsRecovery gating), possibly a +phone-local `HostProfile` hint field (no protocol impact; old desktops never present `relay`, so +the path is naturally guarded). +Risk: racing direct is exactly what `needsRecovery`'s design avoids — needs the mutex +(`operationInFlight`) audit and dwell/hysteresis respect; billed relay data on LANs if scoped too +broadly. +Tests: supervisor fake-timer cases: black-holed LAN converges in ~3-5s; healthy LAN never opens a +relay socket; relay loser closed after direct wins. + +### F7 (S1, deeper) — catalog-validate resume targets + not-found bounce + +Approach: (1) on Resume tap with the host connected, validate the target against the freshest +`worktree.ps` result (not the snapshot-seeded cache); if absent, open the host index instead. +(2) In the session screen, once connected and the catalog is known, bounce unknown `worktreeId`s +(exempting `folder:` and floating-workspace sentinels, `app/h/[hostId]/session/[worktreeId].tsx:852-854`) +to the host index with a notice. (3) Use the validating reader in +`src/worktree/last-visited-worktree-repo.ts` on home instead of the raw `JSON.parse` +(`app/index.tsx:315-322`), and import the storage-key constant at both literal call sites. +Risk: false bounces during slow catalog loads — only bounce on a *confirmed* fresh catalog miss. +Tests: repo tests for the validating reader on home; session-screen bounce cases incl. sentinel +exemptions. + +### F8 (S1 aggravator, cheap) — stop the Resume/Tasks layout shift + +Approach: reserve the Resume card's slot (fixed-height placeholder or render-below-Tasks) so its +late arrival cannot move the Tasks card under the thumb; alternatively render the card immediately +from the snapshot in a disabled state until the host connects. +Files: `app/index.tsx` footer. +Risk: none. +Tests: render test asserting footer order/height stability across `resumeWorktree` arrival. + +### F9 (S1 class, deeper) — HostProtocolGate should not unmount a mounted stack + +Approach: once the HostStack has mounted for a host, keep it mounted and overlay the pending +spinner instead of replacing children, preserving in-flight nested navigation; keep the hard +replace only for the `blocked` verdict. +Files: `src/components/HostProtocolGate.tsx`. +Risk: the gate exists so child routes don't call too-new RPCs while compatibility is unknown — an +overlay must still block interaction until resolved; verify child mount effects don't fire gated +RPCs pre-verdict before choosing overlay vs. current behavior. +Tests: gate test asserting no unmount across `statusPending` for an already-mounted host. + +### F10 (S2 class, deeper) — stop wiping loaded data on transient state flips + +Approach: audit the §3(b) destructive-clear sites and make each preserve data across a +not-`'connected'` blip, clearing only on host change or explicit sign-out. Top offenders by felt +impact: git history blanking rows on the reconnect branch +(`src/source-control/MobileGitHistoryList.tsx:63-68` — refetch without `setRows(null)`); the repo +cache wiped by the rejected in-flight call (`NewWorktreeModal.tsx:330-333` — keep last-good on +error); the diff-review "ready-state preserved" branch that is dead code because it sits after the +early return (`src/session/use-mobile-diff-review-controller.ts:85-89`); host capability wipe +(`src/transport/host-status-gates.ts:32`); the tasks-screen full re-hydration +(`app/h/[hostId]/tasks.tsx:2774-2800`); worktree-cache re-read on reconnect, not only at mount +(`app/h/[hostId]/index.tsx:129,330`). +Risk: showing stale data as if live — pair each preservation with the existing staleness verdicts +rather than inventing new indicators. The in-repo reference pattern is +`src/worktree/home-worktree-info.ts:27-47`: counts older than a 10min TTL render as +"Last known: N worktrees" instead of being dropped, and `markHomeWorktreeCatalogUnavailable` +preserves proven counts across a failed refresh, flagging only `staleCounts`. +Tests: per-surface "data survives disconnect→reconnect" cases; F1 largely removes the *trigger* +(suspend blips), so this is hardening, not the primary fix. + +## 5. Backward compatibility (old/new phone × old/new desktop) + +- Every fix above is phone-app-local; no RPC methods, close codes, credential formats, or pairing + steps change. Old phones against any desktop are untouched (they don't have the code). +- New phone + old desktop without relay support: `host.relay` is absent → F1/F5/F6 relay paths + never activate; pairing keeps the existing `method_not_found` downgrade + (`src/transport/pre-profile-pairing-coordinator.ts:210-217`); F3 logging is inert (no relay + candidate is created). +- New phone + old desktop with relay: all paths use existing RPCs (`status.get`, + `pairing.provisionRelay`, resume confirm) — no new calls introduced. F1's replacement dial reuses + the same resume-credential flow lease rotation already exercises against production desktops. +- F6's profile hint (if added) is a phone-local persisted field; absent values behave as today. + +## 6. Constants appendix (verified) + +| Constant | Value | Where | +|---|---|---| +| Direct connect timeout | 12s | `src/transport/rpc-client.ts:126` | +| Direct handshake timeout | 5s | `rpc-client.ts:127` | +| Direct reconnect ladder | 0.5→60s, give up 12, trickle 90s | `rpc-client.ts:113-117` | +| `migrateTo` auth timeout | 12s | `src/transport/stable-logical-rpc-client.ts:182` | +| Relay backoff | 250ms floor, 500ms base, 30s ceiling, full jitter | `src/transport/mobile-relay-retry-delays.ts:3-5` | +| Host-offline relay retry | 5–15s | `mobile-relay-retry-delays.ts:7-8` | +| Gate reprobe cadence | 60s→15min | `mobile-relay-retry-delays.ts:13-14` | +| Director resolve timeout | 5s (invite & resume) | `mobile-relay-invite-director.ts:16`, `mobile-relay-resume-director.ts:21` | +| Pairing relay recovery | ≤3 attempts × (5s director + ≤100/200/400ms jitter) | `src/transport/pairing-relay-candidate.ts:42,58-71` | +| Pairing overall cap | 25s | `app/pair-confirm.tsx:27` | +| Relay E2EE layer timers | none | `mobile-relay-e2ee-link.ts`, `mobile-e2ee-v2-*.ts` | +| Worktree cache TTL | 30s from write/seed | `src/cache/worktree-cache.ts:12` | +| Direct activity probe (foreground) | detects half-open ≤8s | `rpc-client.ts:1119-1124` | diff --git a/mobile/app/_layout.tsx b/mobile/app/_layout.tsx index d5cbd0e0f..1292b6e5f 100644 --- a/mobile/app/_layout.tsx +++ b/mobile/app/_layout.tsx @@ -8,7 +8,8 @@ import * as Linking from 'expo-linking' import { colors } from '../src/theme/mobile-theme' import { OrcaLogo } from '../src/components/OrcaLogo' import { RpcClientProvider } from '../src/transport/client-context' -import { getNotificationNavigationPath } from '../src/notifications/notification-routing' +import { getNotificationNavigationTarget } from '../src/notifications/notification-routing' +import { useOpenNotificationRoute } from '../src/notifications/use-open-notification-route' import { loadHosts } from '../src/transport/host-store' import { extractPairingCodeFromUrl } from '../src/transport/pairing' import { recoverMobileRelayPairing } from '../src/transport/mobile-relay-pairing-recovery' @@ -34,6 +35,7 @@ Notifications.setNotificationHandler({ export default function RootLayout() { const router = useRouter() + const openNotificationRoute = useOpenNotificationRoute() const handledNotificationIdsRef = useRef>(new Set()) useEffect(() => { @@ -68,6 +70,7 @@ export default function RootLayout() { return () => sub.remove() }, [router]) + // ─── Notification tap routing ─── // Why: iOS delivers local notification taps through expo-notifications, // not Linking. Route both cold-start and warm-start responses to the host // and worktree that scheduled the notification. @@ -91,9 +94,9 @@ export default function RootLayout() { } } - async function getNavigationPath(data: unknown): Promise { + async function getNavigationTarget(data: unknown) { const hosts = await loadHosts().catch(() => null) - return getNotificationNavigationPath(data, { + return getNotificationNavigationTarget(data, { knownHostIds: hosts ? new Set(hosts.map((host) => host.id)) : undefined }) } @@ -118,13 +121,13 @@ export default function RootLayout() { } } - const path = await getNavigationPath(response.notification.request.content.data) + const target = await getNavigationTarget(response.notification.request.content.data) clearLastNotificationResponse() if (disposed) { return } - if (path) { - router.push(path) + if (target) { + openNotificationRoute(target) } } @@ -140,7 +143,8 @@ export default function RootLayout() { disposed = true sub.remove() } - }, [router]) + }, [openNotificationRoute]) + // ─── End notification tap routing ─── // Why: hide the native splash only once the navigation Stack has been laid // out — this is the earliest moment the user will see actual app content. diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index b71f171ce..debd57a92 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -55,6 +55,8 @@ import { ConfirmModal } from '../../../src/components/ConfirmModal' import { BottomDrawer } from '../../../src/components/BottomDrawer' import { useHostProtocolGates } from '../../../src/components/HostProtocolGate' import { AuthFailedBanner } from '../../../src/components/AuthFailedBanner' +import { HostRouteNoticeBanner } from '../../../src/components/HostRouteNoticeBanner' +import { visibleHostRouteNotice } from '../../../src/host-route-notice' import { MobileSearchField } from '../../../src/components/MobileSearchField' import { WorkspaceDetailPlaceholder } from '../../../src/components/WorkspaceDetailPlaceholder' import { getCachedWorktrees, setCachedWorktrees } from '../../../src/cache/worktree-cache' @@ -117,9 +119,12 @@ export function HostScreen({ action: actionProp, onHideSidebar }: HostScreenProps = {}) { - const params = useLocalSearchParams<{ hostId: string; action?: string }>() + const params = useLocalSearchParams<{ hostId: string; action?: string; notice?: string }>() const hostId = hostIdProp ?? params.hostId const action = actionProp ?? params.action + const [dismissedNotice, setDismissedNotice] = useState(null) + const noticeParam = params.notice?.trim() + const routeNotice = visibleHostRouteNotice(embedded, noticeParam, dismissedNotice) const router = useRouter() const pathname = usePathname() const insets = useSafeAreaInsets() @@ -461,7 +466,7 @@ export function HostScreen({ setWorktreesLoaded(true) // Why (#8498): overwrite the home-written cache with the confirmed snapshot so a reconnect/remount can't serve a stale list. if (hostId) { - setCachedWorktrees(hostId, confirmed) + setCachedWorktrees(hostId, confirmed, { proven: true }) } // Drop the optimistic active override once the host reports it active, so later desktop changes win. setOptimisticActiveWorktreeId((pending) => @@ -512,7 +517,8 @@ export function HostScreen({ useFocusEffect( useCallback(() => { // Why: focus nudges reconnect and probes a possibly half-open socket; empty deps fire per focus, not per state flip (which defeats backoff). - clientRef.current?.notifyForeground() + // 'focus' keeps a healthy relay green — probe, never suspend (S2 grey blink). + clientRef.current?.notifyForeground('focus') }, []) ) @@ -734,10 +740,9 @@ export function HostScreen({ ) const displayWorktrees = useMemo(() => { - const base = - connState === 'disconnected' || connState === 'reconnecting' || connState === 'auth-failed' - ? lastKnownWorktrees - : worktrees + // Why: live `worktrees` is authoritative only while connected; under the amber + // mount default, connecting/handshaking must keep the pre-reconnect list too. + const base = connState === 'connected' ? worktrees : lastKnownWorktrees if (sleptIds.size === 0 && optimisticActiveWorktreeId === null) { return base } @@ -1100,6 +1105,14 @@ export function HostScreen({ /> )} + {/* Why a bounced route landed here (e.g. the workspace was deleted on the desktop). */} + {routeNotice && ( + setDismissedNotice(noticeParam ?? null)} + /> + )} + {/* Search bar */} {showSearch && ( diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index efb0b4962..61ef053cb 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -198,6 +198,9 @@ import { resolveMobileFileTabDoc } from '../../../../src/files/mobile-file-tab-d import { captureMobileFileMutationOwnership } from '../../../../src/files/mobile-file-mutation-ownership' import { useMobileFileTapHandlers } from '../../../../src/session/use-mobile-file-tap-handlers' import { useLiveWorktreeName } from '../../../../src/session/use-live-worktree-name' +import { useMissingWorktreeBounce } from '../../../../src/session/use-missing-worktree-bounce' +import { hostRouteWithNotice } from '../../../../src/host-route-notice' +import { LAST_VISITED_WORKTREE_STORAGE_KEY } from '../../../../src/worktree/last-visited-worktree-repo' import { acceptSessionSnapshot, applyClosedTabTombstones, @@ -859,12 +862,19 @@ export default function SessionScreen() { const reconnectAttempts = useReconnectAttempt(hostId) const lastConnectedAt = useLastConnectedAt(hostId) const forceReconnectHost = useForceReconnect() - const worktreeName = useLiveWorktreeName({ + const { name: worktreeName, resolution: worktreeResolution } = useLiveWorktreeName({ client, connState, routeName: routeWorktreeName, worktreeId }) + // Why: a workspace deleted on the desktop leaves every RPC on this route failing forever. + useMissingWorktreeBounce({ + hostId, + worktreeId, + resolution: worktreeResolution, + bounce: (id) => router.replace(hostRouteWithNotice(id, 'worktree-missing')) + }) // Master-detail state: wide layouts dock a tapped panel beside the session; narrow keeps it null and pushes full-screen routes. const { isWideLayout } = useResponsiveLayout() const [activePanel, setActivePanel] = useState(null) @@ -2640,7 +2650,7 @@ export default function SessionScreen() { useEffect(() => { if (hostId && worktreeId) { void AsyncStorage.setItem( - 'orca:last-visited-worktree', + LAST_VISITED_WORKTREE_STORAGE_KEY, JSON.stringify({ hostId, worktreeId }) ) } diff --git a/mobile/app/index.tsx b/mobile/app/index.tsx index 7bfe5ee67..642cbfac5 100644 --- a/mobile/app/index.tsx +++ b/mobile/app/index.tsx @@ -24,12 +24,8 @@ import type { HomeWorktreeSummary, HostWorktreeInfo } from '../src/worktree/home import type { RpcClient } from '../src/transport/rpc-client' import { createHostConnectRefetchGate } from '../src/transport/host-connect-refetch-gate' import { sendSingleFlightRequest } from '../src/transport/request-single-flight' -import { - useAllHostClients, - useCloseHost, - useForceReconnect, - usePrimeHosts -} from '../src/transport/client-context' +import { useCloseHost, useForceReconnect, usePrimeHosts } from '../src/transport/client-context' +import { useAllHostClients } from '../src/transport/use-all-host-clients' import { classifyConnection } from '../src/transport/connection-health' import { subscribeToDesktopNotifications } from '../src/notifications/mobile-notifications' import { @@ -44,7 +40,15 @@ import { TaskProviderLogo } from '../src/components/TaskProviderLogo' import { ActionSheetModal } from '../src/components/ActionSheetModal' import { getHostListActionSheetActions } from '../src/host-list-action-sheet-actions' import { ConfirmModal } from '../src/components/ConfirmModal' -import { setCachedWorktrees, getCachedWorktrees } from '../src/cache/worktree-cache' +import { + setCachedWorktrees, + getCachedWorktrees, + getProvenCachedWorktrees +} from '../src/cache/worktree-cache' +import { + LAST_VISITED_WORKTREE_STORAGE_KEY, + readLastVisitedWorktreeRecord +} from '../src/worktree/last-visited-worktree-repo' import { loadHomeSnapshot, saveHomeSnapshot } from '../src/cache/home-snapshot-cache' import { colors, spacing, radii } from '../src/theme/mobile-theme' import { @@ -55,6 +59,13 @@ import { import { useOpenMobileTasks } from '../src/tasks/use-open-mobile-tasks' import { useResponsiveLayout } from '../src/layout/responsive-layout' import { useOpenMobileSession } from '../src/session/use-open-mobile-session' +import { useOpenMobileAccounts } from '../src/accounts/use-open-mobile-accounts' +import { + isResumeTargetConfirmedMissing, + selectHomeResumeCard, + type HomeResumeCard +} from '../src/worktree/home-resume-card' +import { hostRouteWithNotice } from '../src/host-route-notice' function endpointLabel(endpoint: string): string { try { @@ -209,6 +220,7 @@ export default function HomeScreen() { const openMobileHostEdit = useOpenMobileHostEdit() const openMobileTasks = useOpenMobileTasks() const openMobileSession = useOpenMobileSession() + const openMobileAccounts = useOpenMobileAccounts() const insets = useSafeAreaInsets() // Why: cap/center content on wide/tablet canvases so cards don't stretch edge-to-edge on iPad. const { isWideLayout, contentMaxWidth } = useResponsiveLayout() @@ -313,13 +325,13 @@ export default function HomeScreen() { router.replace(mobileOnboardingDestination(onboardingSteps)) } }) - void AsyncStorage.getItem('orca:last-visited-worktree').then((raw) => { - if (stale || !raw) { + void AsyncStorage.getItem(LAST_VISITED_WORKTREE_STORAGE_KEY).then((raw) => { + if (stale) { return } - try { - setLastVisited(JSON.parse(raw)) - } catch {} + // Why the validating reader: this record becomes the Resume card's navigation target, + // so a malformed or older-shaped payload must read as no history, not a broken route. + setLastVisited(readLastVisitedWorktreeRecord(raw)) }) for (const entry of allClientsRef.current) { if (entry.client.getState() === 'connected') { @@ -482,28 +494,43 @@ export default function HomeScreen() { .join(',') ]) - // Why: prefer the worktree last opened on this device so Resume reflects mobile session history. - // Why: don't gate on 'connected' so the card doesn't flash empty for ~1s on cold-start; cached data holds until fresh RPC lands. - const resumeWorktree = useMemo(() => { - // Why: only surface Resume for connected hosts; a stale worktree taps into a route that can't load. - if (lastVisited && hostStates[lastVisited.hostId] === 'connected') { - const cached = getCachedWorktrees(lastVisited.hostId) as HomeWorktreeSummary[] | null - const match = cached?.find((w) => w.worktreeId === lastVisited.worktreeId) - if (match) { - return { hostId: lastVisited.hostId, worktree: match } + // Why: the card renders from cached/snapshot data the moment a candidate exists — see + // selectHomeResumeCard for why its slot must not wait for the host to connect. + const resumeCard = useMemo( + () => + selectHomeResumeCard({ + hosts: sortedHosts, + hostStates, + worktreeInfo, + lastVisited, + cachedWorktrees: (hostId) => getCachedWorktrees(hostId) as HomeWorktreeSummary[] | null + }), + [sortedHosts, hostStates, worktreeInfo, lastVisited] + ) + + // Why: the card is drawn from a snapshot that can name a workspace the desktop has since + // deleted. When the host has proven otherwise, open its workspace list rather than a session + // screen whose every RPC would fail. An unproven catalog is not evidence — that tap goes + // through and the session screen bounces once the host answers (F7). + const openResume = useCallback( + (card: HomeResumeCard) => { + if ( + isResumeTargetConfirmedMissing( + card, + getProvenCachedWorktrees(card.hostId) as HomeWorktreeSummary[] | null + ) + ) { + router.push(hostRouteWithNotice(card.hostId, 'worktree-missing')) + return } - } - for (const host of sortedHosts) { - if (hostStates[host.id] !== 'connected') { - continue - } - const info = worktreeInfo[host.id] - if (info?.lastActiveWorktree) { - return { hostId: host.id, worktree: info.lastActiveWorktree } - } - } - return null - }, [sortedHosts, hostStates, worktreeInfo, lastVisited]) + openMobileSession({ + hostId: card.hostId, + worktreeId: card.worktree.worktreeId, + name: card.worktree.displayName || card.worktree.repo + }) + }, + [openMobileSession, router] + ) // Why: only show Account usage for connected hosts; stale cached usage would imply live data. const accountsHosts = useMemo(() => { @@ -545,7 +572,7 @@ export default function HomeScreen() { disabled={!primaryConnectedHost} style={({ pressed }) => [ styles.taskHomeCard, - !primaryConnectedHost && styles.quickActionDisabled, + !primaryConnectedHost && styles.cardDisabled, pressed && styles.hostCardPressed ]} onPress={() => { @@ -732,51 +759,45 @@ export default function HomeScreen() { ListFooterComponent={ {/* ─── Resume card ─── */} - {resumeWorktree ? ( + {resumeCard ? ( <> Resume [styles.resumeCard, pressed && styles.hostCardPressed]} - onPress={() => - openMobileSession({ - hostId: resumeWorktree.hostId, - worktreeId: resumeWorktree.worktree.worktreeId, - name: resumeWorktree.worktree.displayName || resumeWorktree.worktree.repo - }) - } + disabled={!resumeCard.actionable} + style={({ pressed }) => [ + styles.resumeCard, + !resumeCard.actionable && styles.cardDisabled, + pressed && styles.hostCardPressed + ]} + onPress={() => openResume(resumeCard)} > - {resumeWorktree.worktree.displayName} + {resumeCard.worktree.displayName} - {resumeWorktree.worktree.repo} + {resumeCard.worktree.repo} {' · '} - {resumeWorktree.worktree.branch} + {resumeCard.worktree.branch} - Tasks - {renderTaskHomeCard()} - ) : ( - <> - Tasks - {renderTaskHomeCard()} - - )} + ) : null} + Tasks + {renderTaskHomeCard()} {/* ─── Quick actions ─── */} Quick Actions @@ -794,7 +815,7 @@ export default function HomeScreen() { disabled={!primaryConnectedHost} style={({ pressed }) => [ styles.quickAction, - !primaryConnectedHost && styles.quickActionDisabled, + !primaryConnectedHost && styles.cardDisabled, pressed && styles.hostCardPressed ]} onPress={() => { @@ -831,7 +852,7 @@ export default function HomeScreen() { styles.accountsCard, pressed && styles.hostCardPressed ]} - onPress={() => router.push(`/h/${host.id}/accounts`)} + onPress={() => openMobileAccounts(host.id)} > {showHostName ? ( @@ -1221,7 +1242,7 @@ const styles = StyleSheet.create({ alignItems: 'center', gap: 10 }, - quickActionDisabled: { + cardDisabled: { opacity: 0.45 }, quickActionIcon: { diff --git a/mobile/app/terminal-settings.tsx b/mobile/app/terminal-settings.tsx index 261293e99..e6aa691c7 100644 --- a/mobile/app/terminal-settings.tsx +++ b/mobile/app/terminal-settings.tsx @@ -12,7 +12,7 @@ import { ChevronLeft, ChevronRight, Smartphone, Type } from 'lucide-react-native import { colors, radii, spacing, typography } from '../src/theme/mobile-theme' import { loadHosts } from '../src/transport/host-store' import type { HostProfile } from '../src/transport/types' -import { useAllHostClients } from '../src/transport/client-context' +import { useAllHostClients } from '../src/transport/use-all-host-clients' import type { RpcClient } from '../src/transport/rpc-client' import { PickerModal, type PickerOption } from '../src/components/PickerModal' import { TerminalShortcutSettings } from '../src/components/TerminalShortcutSettings' diff --git a/mobile/app/voice-settings.tsx b/mobile/app/voice-settings.tsx index 4a0f6d07c..a09d964b3 100644 --- a/mobile/app/voice-settings.tsx +++ b/mobile/app/voice-settings.tsx @@ -14,7 +14,7 @@ import { ChevronLeft, ChevronRight } from 'lucide-react-native' import { colors, radii, spacing, typography } from '../src/theme/mobile-theme' import { loadHosts } from '../src/transport/host-store' import type { HostProfile } from '../src/transport/types' -import { useAllHostClients } from '../src/transport/client-context' +import { useAllHostClients } from '../src/transport/use-all-host-clients' import type { RpcClient } from '../src/transport/rpc-client' import { BottomDrawer } from '../src/components/BottomDrawer' import { VoiceModelList } from '../src/components/VoiceModelList' diff --git a/mobile/src/accounts/mobile-accounts-route.test.ts b/mobile/src/accounts/mobile-accounts-route.test.ts new file mode 100644 index 000000000..03893215f --- /dev/null +++ b/mobile/src/accounts/mobile-accounts-route.test.ts @@ -0,0 +1,96 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it, vi } from 'vitest' +import { mobileAccountsRouteTarget } from './mobile-accounts-route' +import { + hostStackHostRoute, + navigateToHostStackRoute, + type HostStackNavigationState +} from '../navigation/host-stack-navigation' + +const homeSource = readFileSync(new URL('../../app/index.tsx', import.meta.url), 'utf8') + +function navigationHarness(initialState: HostStackNavigationState) { + const stateListeners = new Set<() => void>() + let state = initialState + const navigation = { + addListener: vi.fn((_event: 'state', listener: () => void) => { + stateListeners.add(listener) + return () => stateListeners.delete(listener) + }), + dispatch: vi.fn(), + getState: () => state + } + return { + navigation, + setState(nextState: HostStackNavigationState) { + state = nextState + for (const listener of stateListeners) { + listener() + } + } + } +} + +describe('mobile accounts route', () => { + it('keeps the host id raw for the navigator to encode', () => { + expect(mobileAccountsRouteTarget('host/one')).toEqual({ + name: '[hostId]/accounts', + params: { hostId: 'host/one' } + }) + }) + + it('mounts the host before replacing it with the accounts route', () => { + const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) + const push = vi.fn() + + navigateToHostStackRoute( + harness.navigation, + { push }, + 'host/one', + mobileAccountsRouteTarget('host/one') + ) + + expect(push).toHaveBeenCalledWith(hostStackHostRoute('host/one')) + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + + harness.setState({ + index: 1, + routes: [ + { name: 'index' }, + { + name: 'h', + state: { + key: '/h', + index: 0, + routes: [ + { + key: 'host-index', + name: '[hostId]/index', + params: { hostId: encodeURIComponent('host/one') } + } + ] + } + } + ] + }) + + expect(harness.navigation.dispatch).toHaveBeenCalledWith({ + type: 'REPLACE', + target: '/h', + source: 'host-index', + payload: mobileAccountsRouteTarget('host/one') + }) + }) + + it('opens the home account-usage card through the cold-navigator-safe transition', () => { + const start = homeSource.indexOf('{/* ─── Account usage ─── */}') + + // Assert the marker first: a renamed banner would otherwise slice garbage and report a + // missing call instead of the real cause. + expect(start).toBeGreaterThanOrEqual(0) + + const accountsSection = homeSource.slice(start) + expect(accountsSection).toContain('openMobileAccounts(host.id)') + expect(accountsSection).not.toContain('/accounts`') + }) +}) diff --git a/mobile/src/accounts/mobile-accounts-route.ts b/mobile/src/accounts/mobile-accounts-route.ts new file mode 100644 index 000000000..1ab134dc2 --- /dev/null +++ b/mobile/src/accounts/mobile-accounts-route.ts @@ -0,0 +1,10 @@ +import type { HostStackRouteTarget } from '../navigation/host-stack-navigation' + +/** Host id stays raw — the navigator owns the params, so pre-encoding one would + * reach the accounts screen still escaped. */ +export function mobileAccountsRouteTarget(hostId: string): HostStackRouteTarget { + return { + name: '[hostId]/accounts', + params: { hostId } + } +} diff --git a/mobile/src/accounts/use-open-mobile-accounts.ts b/mobile/src/accounts/use-open-mobile-accounts.ts new file mode 100644 index 000000000..de6df04bd --- /dev/null +++ b/mobile/src/accounts/use-open-mobile-accounts.ts @@ -0,0 +1,14 @@ +import { useCallback } from 'react' +import { useOpenHostStackRoute } from '../navigation/use-open-host-stack-route' +import { mobileAccountsRouteTarget } from './mobile-accounts-route' + +export function useOpenMobileAccounts(): (hostId: string) => void { + const openHostStackRoute = useOpenHostStackRoute() + + return useCallback( + (hostId) => { + openHostStackRoute(hostId, mobileAccountsRouteTarget(hostId)) + }, + [openHostStackRoute] + ) +} diff --git a/mobile/src/cache/worktree-cache.test.ts b/mobile/src/cache/worktree-cache.test.ts index f167c6c54..d8ed620eb 100644 --- a/mobile/src/cache/worktree-cache.test.ts +++ b/mobile/src/cache/worktree-cache.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { setCachedWorktrees, getCachedWorktrees } from './worktree-cache' +import { setCachedWorktrees, getCachedWorktrees, getProvenCachedWorktrees } from './worktree-cache' // Why: AC #8498 guarantees a reconnect refetch writes through the // same cache path the host detail screen seeds from, so a reconnect can't @@ -41,3 +41,51 @@ describe('worktree-cache write-through', () => { expect(getCachedWorktrees(hostId)).toEqual(reconnected) }) }) + +// Why (F7): home seeds this cache from a persisted cold-start snapshot as well as from a live +// worktree.ps, and only the latter can prove a workspace *absent* — the Resume tap redirects +// off that distinction, so a seeded entry must never look authoritative. +describe('worktree-cache provenance', () => { + it('withholds unmarked writes from the proven reader', () => { + const hostId = 'host-seeded' + const seeded = [{ worktreeId: 'a' }] + + setCachedWorktrees(hostId, seeded) + + expect(getCachedWorktrees(hostId)).toEqual(seeded) + expect(getProvenCachedWorktrees(hostId)).toBeNull() + }) + + it('exposes a host-listed catalog to the proven reader', () => { + const hostId = 'host-proven' + const listed = [{ worktreeId: 'a' }, { worktreeId: 'b' }] + + setCachedWorktrees(hostId, listed, { proven: true }) + + expect(getProvenCachedWorktrees(hostId)).toEqual(listed) + }) + + it('keeps a fresh proven catalog when an unproven seed lands after it', () => { + const hostId = 'host-kept' + const listed = [{ worktreeId: 'a' }, { worktreeId: 'b' }] + setCachedWorktrees(hostId, listed, { proven: true }) + + // A cold-start snapshot seed must neither truncate nor de-prove the host-listed rows. + setCachedWorktrees(hostId, [{ worktreeId: 'a' }]) + + expect(getProvenCachedWorktrees(hostId)).toEqual(listed) + }) + + it('lets an unproven seed replace another unproven entry', () => { + const hostId = 'host-reseeded' + setCachedWorktrees(hostId, [{ worktreeId: 'a' }]) + setCachedWorktrees(hostId, [{ worktreeId: 'b' }]) + + expect(getCachedWorktrees(hostId)).toEqual([{ worktreeId: 'b' }]) + expect(getProvenCachedWorktrees(hostId)).toBeNull() + }) + + it('reports nothing proven for a host it has never cached', () => { + expect(getProvenCachedWorktrees('host-never-seen')).toBeNull() + }) +}) diff --git a/mobile/src/cache/worktree-cache.ts b/mobile/src/cache/worktree-cache.ts index 0b05f78b2..03da01bd1 100644 --- a/mobile/src/cache/worktree-cache.ts +++ b/mobile/src/cache/worktree-cache.ts @@ -5,6 +5,9 @@ type CachedWorktrees = { worktrees: unknown[] at: number + // Whether the host itself listed these rows this session, as opposed to a cold-start seed + // rebuilt from a persisted snapshot. Only a proven list can prove a worktree *absent*. + proven: boolean } const cache = new Map() @@ -12,12 +15,23 @@ const cache = new Map() const MAX_AGE_MS = 30_000 const MAX_ENTRIES = 20 -export function setCachedWorktrees(hostId: string, worktrees: unknown[]): void { +export function setCachedWorktrees( + hostId: string, + worktrees: unknown[], + options?: { proven?: boolean } +): void { + // Why: a cold-start snapshot seed landing after a live worktree.ps must not erase + // the proof — or truncate the host-listed rows — the resume check depends on. + if (options?.proven !== true && readFreshEntry(hostId)?.proven) { + return + } // Why: Map.set on an existing key does not move it to the end of iteration // order. Delete first so the re-inserted key becomes the newest entry, // giving us true LRU eviction when the cap is hit. cache.delete(hostId) - cache.set(hostId, { worktrees, at: Date.now() }) + // Default false: a caller that has not said where the rows came from must never be taken + // as grounds for redirecting the user away from a workspace. + cache.set(hostId, { worktrees, at: Date.now(), proven: options?.proven === true }) if (cache.size > MAX_ENTRIES) { const oldest = cache.keys().next().value if (oldest) { @@ -27,6 +41,17 @@ export function setCachedWorktrees(hostId: string, worktrees: unknown[]): void { } export function getCachedWorktrees(hostId: string): unknown[] | null { + return readFreshEntry(hostId)?.worktrees ?? null +} + +/** The rows only when the host listed them itself — null whenever absence cannot be trusted, + * which is every unproven or expired entry. */ +export function getProvenCachedWorktrees(hostId: string): unknown[] | null { + const entry = readFreshEntry(hostId) + return entry?.proven ? entry.worktrees : null +} + +function readFreshEntry(hostId: string): CachedWorktrees | null { const entry = cache.get(hostId) if (!entry) { return null @@ -35,5 +60,5 @@ export function getCachedWorktrees(hostId: string): unknown[] | null { cache.delete(hostId) return null } - return entry.worktrees + return entry } diff --git a/mobile/src/components/HostProtocolGate.test.ts b/mobile/src/components/HostProtocolGate.test.ts index 91d3b858a..94cae6d91 100644 --- a/mobile/src/components/HostProtocolGate.test.ts +++ b/mobile/src/components/HostProtocolGate.test.ts @@ -1,4 +1,4 @@ -import { createElement } from 'react' +import { createElement, useEffect } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' @@ -14,7 +14,10 @@ vi.mock('react-native', () => ({ Linking: { openURL: nativeTestState.openUrl }, Platform: nativeTestState.platform, Pressable: 'Pressable', - StyleSheet: { create: (styles: T) => styles }, + StyleSheet: { + create: (styles: T) => styles, + absoluteFillObject: { position: 'absolute', top: 0, left: 0, right: 0, bottom: 0 } + }, Text: 'Text', View: 'View' })) @@ -41,11 +44,20 @@ function GateConsumer() { return createElement('GateStatus', null, hostCapabilities.join(',')) } +// Counts mounts so a test can prove the routes were never torn down, which presence alone can't. +const probeMounts = { count: 0 } +function MountProbe() { + useEffect(() => { + probeMounts.count += 1 + }, []) + return createElement('MountProbe') +} + function gateElement() { return createElement( HostProtocolGate, { hostId: 'host-1' }, - createElement('HostContent', null, createElement(GateConsumer)) + createElement('HostContent', null, createElement(GateConsumer), createElement(MountProbe)) ) } @@ -69,6 +81,7 @@ describe('HostProtocolGate', () => { globalThis.IS_REACT_ACT_ENVIRONMENT = true nativeTestState.openUrl.mockClear() nativeTestState.platform.OS = 'ios' + probeMounts.count = 0 }) afterEach(() => { @@ -157,9 +170,56 @@ describe('HostProtocolGate', () => { const output = renderedText(renderer) expect(output).toContain('Checking host compatibility') expect(output).not.toContain('HostContent') + expect(probeMounts.count).toBe(0) expect(client.sendRequest).toHaveBeenCalledOnce() }) + it('overlays the pending spinner instead of unmounting routes mounted while connecting', async () => { + hostClient.current = { client: null, state: 'connecting' } + renderer = await renderGate() + expect(renderedText(renderer)).toContain('HostContent') + expect(probeMounts.count).toBe(1) + + const client = { + sendRequest: vi.fn().mockReturnValue(new Promise(() => {})) + } as unknown as RpcClient + await act(async () => { + hostClient.current = { client, state: 'connected' } + renderer?.update(gateElement()) + await Promise.resolve() + }) + + const output = renderedText(renderer) + expect(output).toContain('HostContent') + expect(output).toContain('Checking host compatibility') + // Why: the cold-start remount this replaces is exactly what destroys in-flight deep navigation. + expect(probeMounts.count).toBe(1) + const overlay = renderer.root + .findAllByType('View') + .find((node) => node.props.accessibilityViewIsModal === true) + expect(overlay?.props.pointerEvents).toBe('auto') + }) + + it('still replaces mounted routes when the verdict comes back blocked', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + hostClient.current = { client: null, state: 'connecting' } + renderer = await renderGate() + expect(renderedText(renderer)).toContain('HostContent') + + await act(async () => { + hostClient.current = { + client: clientWithStatus({ protocolVersion: 5, minCompatibleMobileVersion: 999 }), + state: 'connected' + } + renderer?.update(gateElement()) + await Promise.resolve() + }) + + const output = renderedText(renderer) + expect(output).toContain('Update Orca Mobile') + expect(output).not.toContain('HostContent') + }) + it('keeps an already-validated host route mounted while reconnect status is pending', async () => { const client = { sendRequest: vi @@ -183,7 +243,10 @@ describe('HostProtocolGate', () => { await Promise.resolve() }) - expect(renderedText(renderer)).toContain('HostContent') + const output = renderedText(renderer) + expect(output).toContain('HostContent') + // Why: the host already answered once, so a reconnect probe must not dim the UI it validated. + expect(output).not.toContain('Checking host compatibility') expect(client.sendRequest).toHaveBeenCalledTimes(2) }) diff --git a/mobile/src/components/HostProtocolGate.tsx b/mobile/src/components/HostProtocolGate.tsx index 565784e25..4d9c0c019 100644 --- a/mobile/src/components/HostProtocolGate.tsx +++ b/mobile/src/components/HostProtocolGate.tsx @@ -1,4 +1,4 @@ -import { createContext, useContext, useRef, type ReactNode } from 'react' +import { createContext, useContext, useEffect, useRef, type ReactNode } from 'react' import { ActivityIndicator, StyleSheet, View } from 'react-native' import { useHostClient } from '../transport/client-context' import { useHostStatusGates, type HostStatusGates } from '../transport/host-status-gates' @@ -27,12 +27,31 @@ export function HostProtocolGate({ hostId, children }: Props) { const gates = useHostStatusGates({ hostId, client, connState: state }) const { compatVerdict, statusPending } = gates const resolvedHostIdRef = useRef(null) + const mountedHostIdRef = useRef(null) const hostKey = hostId ?? null - if (state === 'connected' && client && !statusPending) { - resolvedHostIdRef.current = hostKey - } - if (statusPending && resolvedHostIdRef.current !== hostKey) { - // Why: child routes may call newer RPCs on mount, so wait until compatibility is known. + const resolvedNow = state === 'connected' && client !== null && !statusPending + const blocked = compatVerdict.kind === 'blocked' + const pending = statusPending && resolvedHostIdRef.current !== hostKey + const holdBack = pending && mountedHostIdRef.current !== hostKey + + // Why: React can replay or discard a render, so the latches record committed + // outcomes only — a discarded children render must not count as mounted. + useEffect(() => { + if (resolvedNow) { + resolvedHostIdRef.current = hostKey + } + if (blocked) { + // Why: the block screen unmounts the routes, so a later pending window + // must not assume a live tree it can overlay. + mountedHostIdRef.current = null + } else if (!holdBack) { + mountedHostIdRef.current = hostKey + } + }) + + if (holdBack) { + // Why: nothing is mounted yet for this host, so hold the routes back entirely + // rather than letting them mount (and fire their connect RPCs) pre-verdict. return ( ) } - if (compatVerdict.kind === 'blocked') { + if (blocked) { return } // Why: the host sidebar needs the same status fields; sharing the result avoids a second status.get per route. - return {children} + return ( + + + + {children} + + {pending ? ( + // Why: once the stack is mounted, unmounting it for a pending status.get destroys + // in-flight nested navigation, so cover it instead. Mount effects underneath still + // run — they wait for connState 'connected' and every capability-dependent call + // re-probes status.get itself, so nothing newer than the baseline fires here. + + + + ) : null} + + + ) } const styles = StyleSheet.create({ @@ -55,5 +105,17 @@ const styles = StyleSheet.create({ alignItems: 'center', justifyContent: 'center', backgroundColor: colors.bgBase + }, + // Stays mounted across the overlay toggling so the routes below keep their identity. + host: { + flex: 1 + }, + pendingOverlay: { + ...StyleSheet.absoluteFillObject, + alignItems: 'center', + justifyContent: 'center', + backgroundColor: colors.bgBase, + zIndex: 1000, + elevation: 1000 } }) diff --git a/mobile/src/components/HostRouteNoticeBanner.tsx b/mobile/src/components/HostRouteNoticeBanner.tsx new file mode 100644 index 000000000..28b43e71a --- /dev/null +++ b/mobile/src/components/HostRouteNoticeBanner.tsx @@ -0,0 +1,43 @@ +import { Pressable, StyleSheet, Text, View } from 'react-native' +import { X } from 'lucide-react-native' +import { colors, spacing } from '../theme/mobile-theme' + +// Informational, not an error: the host is healthy and the user's target simply went away, +// so this stays monochrome rather than borrowing the auth-failed red. +export function HostRouteNoticeBanner({ + message, + onDismiss +}: { + message: string + onDismiss: () => void +}) { + return ( + + {message} + + + + + ) +} + +const styles = StyleSheet.create({ + banner: { + flexDirection: 'row', + alignItems: 'center', + gap: spacing.md, + backgroundColor: colors.bgPanel, + paddingVertical: spacing.sm, + paddingHorizontal: spacing.lg, + borderBottomWidth: 1, + borderBottomColor: colors.borderSubtle + }, + text: { flex: 1, color: colors.textSecondary, fontSize: 13 }, + dismiss: { padding: spacing.xs } +}) diff --git a/mobile/src/components/MobileHostCard.test.tsx b/mobile/src/components/MobileHostCard.test.tsx index a18897939..6138e3111 100644 --- a/mobile/src/components/MobileHostCard.test.tsx +++ b/mobile/src/components/MobileHostCard.test.tsx @@ -2,7 +2,8 @@ import { createElement } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { ConnectionVerdict } from '../transport/connection-health' -import type { HostProfile } from '../transport/types' +import type { MobileConnectionPath } from '../transport/stable-logical-rpc-client' +import type { ConnectionState, HostProfile } from '../transport/types' import { markHomeWorktreeCatalogUnavailable, type HostWorktreeInfo @@ -47,14 +48,21 @@ describe('MobileHostCard', () => { renderer = null }) - async function renderCard(worktreeInfo: HostWorktreeInfo | undefined): Promise { + async function renderCard( + worktreeInfo: HostWorktreeInfo | undefined, + overrides?: { + state?: ConnectionState + verdict?: ConnectionVerdict + path?: MobileConnectionPath + } + ): Promise { await act(async () => { renderer = create( createElement(MobileHostCard, { host, - state: 'connected', - verdict, - path: 'lan', + state: overrides?.state ?? 'connected', + verdict: overrides?.verdict ?? verdict, + path: overrides?.path ?? 'lan', worktreeInfo, onPress: () => {}, onLongPress: () => {} @@ -84,6 +92,47 @@ describe('MobileHostCard', () => { ) }) + it('names the relay while the dial is still in flight', async () => { + const lines = await renderCard(undefined, { + state: 'connecting', + verdict: { kind: 'normal', label: 'Connecting…' }, + path: 'relay' + }) + + expect(lines).toContain('Connecting…') + expect(lines).toContain(' · Orca Relay') + }) + + it('names the relay while a failed direct dial is still retrying', async () => { + const lines = await renderCard(undefined, { + state: 'reconnecting', + verdict: { kind: 'normal', label: 'Reconnecting…' }, + path: 'relay' + }) + + expect(lines).toContain(' · Orca Relay') + }) + + it('leaves an idle disconnected host unlabelled', async () => { + const lines = await renderCard(undefined, { + state: 'disconnected', + verdict: { kind: 'normal', label: 'Disconnected' }, + path: 'relay' + }) + + expect(lines).not.toContain(' · Orca Relay') + }) + + it('does not guess a direct path before the dial resolves', async () => { + const lines = await renderCard(undefined, { + state: 'connecting', + verdict: { kind: 'normal', label: 'Connecting…' }, + path: 'lan' + }) + + expect(lines).not.toContain(' · Direct · LAN') + }) + it('shows no worktree line before the first read lands', async () => { const lines = await renderCard(undefined) diff --git a/mobile/src/components/MobileHostCard.tsx b/mobile/src/components/MobileHostCard.tsx index 79a58bf76..f81f93640 100644 --- a/mobile/src/components/MobileHostCard.tsx +++ b/mobile/src/components/MobileHostCard.tsx @@ -21,6 +21,11 @@ export function MobileHostCard(props: { onLongPress: () => void }) { const connected = props.state === 'connected' + // Why: a relay dial can run for seconds behind "Connecting…"/"Reconnecting…"; naming the + // path mid-wait tells the user the phone is off-LAN rather than hung (F5). Only 'relay' is + // named — 'lan' doubles as the unknown-path default, so it would be a guess before connect. + const dialingPath = + ['connecting', 'handshaking', 'reconnecting'].includes(props.state) && props.path === 'relay' const isError = ['warning', 'unreachable', 'auth-failed'].includes(props.verdict.kind) const worktreeSummary = homeHostWorktreeSummary(props.worktreeInfo) return ( @@ -44,7 +49,7 @@ export function MobileHostCard(props: { {verdictDisplayLabel(props.verdict)} - {connected ? ` · ${mobileConnectionPathLabel(props.path)}` : ''} + {connected || dialingPath ? ` · ${mobileConnectionPathLabel(props.path)}` : ''} {connected && worktreeSummary ? ( diff --git a/mobile/src/components/NewWorktreeModal.test.tsx b/mobile/src/components/NewWorktreeModal.test.tsx new file mode 100644 index 000000000..3fb76dd31 --- /dev/null +++ b/mobile/src/components/NewWorktreeModal.test.tsx @@ -0,0 +1,98 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' + +const asyncStorage = vi.hoisted(() => ({ + getItem: vi.fn().mockResolvedValue(null), + setItem: vi.fn().mockResolvedValue(undefined), + removeItem: vi.fn().mockResolvedValue(undefined) +})) +vi.mock('@react-native-async-storage/async-storage', () => ({ default: asyncStorage })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + FlatList: 'FlatList', + Image: 'Image', + Keyboard: { dismiss: vi.fn() }, + Linking: { openURL: vi.fn() }, + Modal: 'Modal', + ScrollView: 'ScrollView', + Platform: { OS: 'ios', select: (options: { ios?: unknown }) => options.ios }, + Pressable: 'Pressable', + StyleSheet: { create: (styles: T) => styles }, + Switch: 'Switch', + Text: 'Text', + TextInput: 'TextInput', + View: 'View' +})) +// Every icon in the drawer tree renders as a host element named after itself. +vi.mock( + 'lucide-react-native', + () => + new Proxy( + {}, + { + get: (_target, name) => (typeof name === 'string' ? name : undefined), + has: () => true + } + ) +) +vi.mock('./BottomDrawer', () => ({ BottomDrawer: 'BottomDrawer' })) +vi.mock('./bottom-drawer-modal-host', () => ({ BottomDrawerModalHost: 'BottomDrawerModalHost' })) +vi.mock('./PickerListDrawer', () => ({ PickerListDrawer: 'PickerListDrawer' })) +vi.mock('./MobileAgentIcon', () => ({ MobileAgentIcon: 'MobileAgentIcon' })) +vi.mock('./TaskProviderLogo', () => ({ TaskProviderLogo: 'TaskProviderLogo' })) + +import { setCachedRepos } from '../cache/repo-cache' +import { NewWorktreeModal } from './NewWorktreeModal' + +const repos = [{ id: 'repo-1', displayName: 'orca', path: '/src/orca', kind: 'git' }] + +function repoPickerNames(renderer: ReactTestRenderer | null): string[] { + const pickers = renderer?.root.findAll((node) => node.type === 'PickerListDrawer') ?? [] + const repoPicker = pickers.find((node) => node.props.title === 'Repository') + return ((repoPicker?.props.items ?? []) as { label: string }[]).map((item) => item.label) +} + +describe('NewWorktreeModal repo list', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + setCachedRepos('host-1', repos) + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + it('keeps the cached repos when the in-flight repo.list rejects on a dropped connection', async () => { + const sendRequest = vi.fn().mockImplementation((method: string) => { + if (method === 'repo.list') { + return Promise.reject(new Error('connection closed')) + } + return new Promise(() => {}) + }) + const client = { sendRequest } as unknown as RpcClient + + await act(async () => { + renderer = create( + createElement(NewWorktreeModal, { + visible: true, + client, + hostId: 'host-1', + onCreated: () => {}, + onClose: () => {} + }) + ) + await Promise.resolve() + }) + await act(async () => { + await Promise.resolve() + }) + + expect(sendRequest).toHaveBeenCalledWith('repo.list') + expect(repoPickerNames(renderer)).toEqual(['orca']) + }) +}) diff --git a/mobile/src/components/NewWorktreeModal.tsx b/mobile/src/components/NewWorktreeModal.tsx index 21a69ec7d..3fca3261e 100644 --- a/mobile/src/components/NewWorktreeModal.tsx +++ b/mobile/src/components/NewWorktreeModal.tsx @@ -327,11 +327,9 @@ function NewWorktreeModalContent({ }) } }) - .catch(() => { - if (!stale) { - setRepos([]) - } - }) + // Why (F10): a dropped connection rejects this call — keep the last-good list (the content + // remounts with the new host's cache when the client changes) instead of emptying the picker. + .catch(() => undefined) .finally(() => { if (!stale) { setLoading(false) diff --git a/mobile/src/host-route-notice.test.ts b/mobile/src/host-route-notice.test.ts new file mode 100644 index 000000000..46ba00a85 --- /dev/null +++ b/mobile/src/host-route-notice.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest' +import { + HOST_ROUTE_NOTICES, + hostRouteNoticeMessage, + hostRouteWithNotice, + visibleHostRouteNotice +} from './host-route-notice' + +describe('hostRouteNoticeMessage', () => { + it('maps a known code to its banner text', () => { + expect(hostRouteNoticeMessage('worktree-missing')).toBe(HOST_ROUTE_NOTICES['worktree-missing']) + }) + + it('renders nothing for absent or unrecognized codes', () => { + expect(hostRouteNoticeMessage(undefined)).toBeNull() + expect(hostRouteNoticeMessage('')).toBeNull() + // A newer build's code must not leak the raw param into the UI. + expect(hostRouteNoticeMessage('some-future-code')).toBeNull() + }) + + // A plain lookup returns Object.prototype members, which would hand the banner a function. + it('renders nothing for prototype keys', () => { + expect(hostRouteNoticeMessage('toString')).toBeNull() + expect(hostRouteNoticeMessage('constructor')).toBeNull() + expect(hostRouteNoticeMessage('__proto__')).toBeNull() + }) +}) + +describe('hostRouteWithNotice', () => { + it('encodes the host id into the noticed route', () => { + expect(hostRouteWithNotice('host/one', 'worktree-missing')).toBe( + '/h/host%2Fone?notice=worktree-missing' + ) + }) +}) + +describe('visibleHostRouteNotice', () => { + const message = HOST_ROUTE_NOTICES['worktree-missing'] + + it('shows a notice the user has not dismissed', () => { + expect(visibleHostRouteNotice(false, 'worktree-missing', null)).toBe(message) + }) + + it('stays silent once that code is dismissed', () => { + expect(visibleHostRouteNotice(false, 'worktree-missing', 'worktree-missing')).toBeNull() + }) + + // Dismissal is keyed by code so a later, different bounce still gets to speak. + it('still shows a different code after one was dismissed', () => { + expect(visibleHostRouteNotice(false, 'worktree-missing', 'some-other-code')).toBe(message) + }) + + it('draws nothing in the embedded sidebar, which shares the route', () => { + expect(visibleHostRouteNotice(true, 'worktree-missing', null)).toBeNull() + }) +}) diff --git a/mobile/src/host-route-notice.ts b/mobile/src/host-route-notice.ts new file mode 100644 index 000000000..2e6d952f1 --- /dev/null +++ b/mobile/src/host-route-notice.ts @@ -0,0 +1,38 @@ +// Why a route param rather than a toast: the screen that learns the bad news (the session) +// unmounts as it bounces, so the message has to travel with the navigation to survive. + +export const HOST_ROUTE_NOTICES = { + 'worktree-missing': 'That workspace no longer exists on this host.' +} as const + +export type HostRouteNotice = keyof typeof HOST_ROUTE_NOTICES + +/** The banner text for a route param, or null when absent/unrecognized — an unknown code + * from a future build must render nothing rather than leak the raw param. */ +export function hostRouteNoticeMessage(notice: string | undefined): string | null { + // Why hasOwn: the param is attacker-adjacent URL text, and a plain lookup of 'toString' + // would hand the banner a function off the prototype instead of missing. + if (!notice || !Object.hasOwn(HOST_ROUTE_NOTICES, notice)) { + return null + } + return HOST_ROUTE_NOTICES[notice as HostRouteNotice] +} + +export function hostRouteWithNotice(hostId: string, notice: HostRouteNotice): string { + return `/h/${encodeURIComponent(hostId)}?notice=${notice}` +} + +/** The banner the host screen should draw, if any. + * `embedded` is the tablet sidebar, which shares the route with the routed screen — one + * bounce must not draw two banners. `dismissed` is keyed by code rather than a boolean so + * closing one notice cannot swallow a later, different one. */ +export function visibleHostRouteNotice( + embedded: boolean, + notice: string | undefined, + dismissed: string | null +): string | null { + if (embedded || (notice && notice === dismissed)) { + return null + } + return hostRouteNoticeMessage(notice) +} diff --git a/mobile/src/navigation/host-stack-navigation.test.ts b/mobile/src/navigation/host-stack-navigation.test.ts index 2dd3cc04e..dc3a94cfe 100644 --- a/mobile/src/navigation/host-stack-navigation.test.ts +++ b/mobile/src/navigation/host-stack-navigation.test.ts @@ -14,7 +14,7 @@ const OTHER_TARGET = { // Removal is modeled for real: a no-op unsubscribe would let `setState` keep // calling a canceled listener, testing the `active` guard instead of teardown. -function navigationHarness(initialState: HostStackNavigationState) { +function navigationHarness(initialState: HostStackNavigationState | undefined) { const stateListeners = new Set<() => void>() let state = initialState const unsubscribe = vi.fn() @@ -33,7 +33,7 @@ function navigationHarness(initialState: HostStackNavigationState) { navigation, unsubscribe, listenerCount: () => stateListeners.size, - setState(nextState: HostStackNavigationState) { + setState(nextState: HostStackNavigationState | undefined) { state = nextState for (const listener of stateListeners) { listener() @@ -58,6 +58,12 @@ function committedHostState(hostIdParam: string): HostStackNavigationState { } } +// The shape app/_layout.tsx sees: Expo Router mounts it as a screen of its own internal +// navigator, so every route the host stack lives in sits one level below. +function rootLayoutScopedState(inner: HostStackNavigationState): HostStackNavigationState { + return { key: 'internal', index: 0, routes: [{ key: '__root', name: '__root', state: inner }] } +} + describe('host stack navigation', () => { it('matches a host committed as the encoded segment it was pushed as', () => { const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) @@ -78,6 +84,46 @@ describe('host stack navigation', () => { }) }) + it('replaces the host stack seen from the root layout, one navigator further down', () => { + const harness = navigationHarness( + rootLayoutScopedState({ index: 0, routes: [{ name: 'index' }] }) + ) + + navigateToHostStackRoute(harness.navigation, { push: vi.fn() }, 'host/one', TARGET) + harness.setState(rootLayoutScopedState(committedHostState('host/one'))) + + expect(harness.navigation.dispatch).toHaveBeenCalledWith({ + type: 'REPLACE', + target: '/h', + source: 'host-index', + payload: TARGET + }) + }) + + it('survives the state emitted before the root navigator has hydrated', () => { + const harness = navigationHarness(undefined) + + navigateToHostStackRoute(harness.navigation, { push: vi.fn() }, 'host/one', TARGET) + + expect(() => harness.setState(undefined)).not.toThrow() + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + + harness.setState(rootLayoutScopedState(committedHostState('host/one'))) + expect(harness.navigation.dispatch).toHaveBeenCalledTimes(1) + }) + + it('abandons the transition when navigation leaves the host route it was waiting on', () => { + const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) + + navigateToHostStackRoute(harness.navigation, { push: vi.fn() }, 'host/one', TARGET) + harness.setState({ index: 0, routes: [{ name: 'h' }] }) + harness.setState({ index: 0, routes: [{ name: 'index' }] }) + harness.setState(committedHostState('host/one')) + + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + expect(harness.listenerCount()).toBe(0) + }) + it('ignores a different host whose id merely decodes badly', () => { const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) diff --git a/mobile/src/navigation/host-stack-navigation.ts b/mobile/src/navigation/host-stack-navigation.ts index 4fcdff8ca..6f73dd3e5 100644 --- a/mobile/src/navigation/host-stack-navigation.ts +++ b/mobile/src/navigation/host-stack-navigation.ts @@ -28,7 +28,9 @@ export type HostStackReplaceAction = Readonly<{ export type HostStackRootNavigation = { addListener: (event: 'state', listener: () => void) => () => void dispatch: (action: HostStackReplaceAction) => void - getState: () => HostStackNavigationState + // Why: the root layout's navigator has no committed state until it hydrates, and a + // notification tap can arm the transition before that first commit. + getState: () => HostStackNavigationState | undefined } export type HostStackHostRoute = `/h/${string}` @@ -68,15 +70,31 @@ function hostParamMatches(param: unknown, expectedHostId: string): boolean { } } +/** The focused `h` route, however deep the caller's navigator sits above it: a screen + * inside the root stack sees it at the top, but app/_layout.tsx is itself a screen of + * Expo Router's internal navigator, so from there the root stack is one level down. */ +function focusedHostRoute(state: HostStackNavigationState): HostStackNavigationRoute | null { + let current: HostStackNavigationState | undefined = state + while (current) { + const route: HostStackNavigationRoute | undefined = current.routes[current.index] + if (!route) { + return null + } + if (route.name === 'h') { + return route + } + current = route.state + } + return null +} + function mountedHostStack( - state: HostStackNavigationState, + hostContainer: HostStackNavigationRoute, expectedHostId: string ): { key: string; routeKey: string } | null { - const hostContainer = state.routes[state.index] - const hostState = hostContainer?.state + const hostState = hostContainer.state const hostRoute = hostState?.routes[hostState.index] if ( - hostContainer?.name !== 'h' || !hostState?.key || hostRoute?.name !== '[hostId]/index' || !hostRoute.key || @@ -114,14 +132,17 @@ export function navigateToHostStackRoute( return } const state = navigation.getState() - const currentRoute = state.routes[state.index] - if (currentRoute?.name === 'h') { + if (!state) { + return + } + const hostContainer = focusedHostRoute(state) + if (hostContainer) { hostRouteSeen = true } else if (hostRouteSeen) { dispose() return } - const hostStack = mountedHostStack(state, hostId) + const hostStack = hostContainer && mountedHostStack(hostContainer, hostId) if (!hostStack) { return } diff --git a/mobile/src/navigation/use-open-host-stack-route.ts b/mobile/src/navigation/use-open-host-stack-route.ts index 674ec5c34..7347d01f2 100644 --- a/mobile/src/navigation/use-open-host-stack-route.ts +++ b/mobile/src/navigation/use-open-host-stack-route.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect } from 'react' +import { useCallback, useEffect, useRef } from 'react' import { useNavigation, useRouter } from 'expo-router' import { coordinateHostStackNavigation, @@ -14,11 +14,17 @@ let pendingNavigation: PendingHostStackNavigation | null = null export function useOpenHostStackRoute(): (hostId: string, target: HostStackRouteTarget) => void { const navigation = useNavigation() const router = useRouter() + // Why: an unmounting screen may only cancel a transition it armed itself — home + // unmounting for onboarding must not kill a notification push still in flight. + const armedRef = useRef(null) useEffect( () => () => { - pendingNavigation?.controller.cancel() - pendingNavigation = null + if (armedRef.current && armedRef.current === pendingNavigation) { + pendingNavigation.controller.cancel() + pendingNavigation = null + } + armedRef.current = null }, [] ) @@ -32,6 +38,7 @@ export function useOpenHostStackRoute(): (hostId: string, target: HostStackRoute hostId, target ) + armedRef.current = pendingNavigation }, [navigation, router] ) diff --git a/mobile/src/notifications/notification-route-coordination.test.ts b/mobile/src/notifications/notification-route-coordination.test.ts new file mode 100644 index 000000000..e41df45a8 --- /dev/null +++ b/mobile/src/notifications/notification-route-coordination.test.ts @@ -0,0 +1,112 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it, vi } from 'vitest' +import { getNotificationNavigationTarget } from './notification-routing' +import { + hostStackHostRoute, + navigateToHostStackRoute, + type HostStackNavigationState +} from '../navigation/host-stack-navigation' + +const rootLayoutSource = readFileSync(new URL('../../app/_layout.tsx', import.meta.url), 'utf8') + +function navigationHarness(initialState: HostStackNavigationState | undefined) { + const stateListeners = new Set<() => void>() + let state = initialState + const navigation = { + addListener: vi.fn((_event: 'state', listener: () => void) => { + stateListeners.add(listener) + return () => stateListeners.delete(listener) + }), + dispatch: vi.fn(), + getState: () => state + } + return { + navigation, + setState(nextState: HostStackNavigationState | undefined) { + state = nextState + for (const listener of stateListeners) { + listener() + } + } + } +} + +// A notification tap is handled by app/_layout.tsx, which Expo Router mounts as a screen of its +// own internal navigator — hence the extra `__root` level around the app's root stack. +function rootLayoutScopedState(inner: HostStackNavigationState): HostStackNavigationState { + return { key: 'internal', index: 0, routes: [{ key: '__root', name: '__root', state: inner }] } +} + +describe('notification route coordination', () => { + it('mounts the host before replacing it with the notification session, from a cold navigator', () => { + const target = getNotificationNavigationTarget({ + hostId: 'host/one', + worktreeId: 'repo::/Users/me/orca/workspaces/feature' + }) + // Cold start: the tap is handled before the root navigator has committed any state. + const harness = navigationHarness(undefined) + const push = vi.fn() + + navigateToHostStackRoute(harness.navigation, { push }, target!.hostId, target!.sessionTarget!) + + expect(push).toHaveBeenCalledWith(hostStackHostRoute('host/one')) + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + + harness.setState(rootLayoutScopedState({ index: 0, routes: [{ name: 'index' }] })) + harness.setState( + rootLayoutScopedState({ + index: 1, + routes: [{ name: 'index' }, { name: 'h', state: undefined }] + }) + ) + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + + harness.setState( + rootLayoutScopedState({ + index: 1, + routes: [ + { name: 'index' }, + { + name: 'h', + state: { + key: '/h', + index: 0, + routes: [ + { + key: 'host-index', + name: '[hostId]/index', + params: { hostId: encodeURIComponent('host/one') } + } + ] + } + } + ] + }) + ) + + expect(harness.navigation.dispatch).toHaveBeenCalledWith({ + type: 'REPLACE', + target: '/h', + source: 'host-index', + payload: target!.sessionTarget + }) + }) + + it('leaves a host-only notification as a shallow push with nothing to coordinate', () => { + expect(getNotificationNavigationTarget({ hostId: 'host-1' })?.sessionTarget).toBeNull() + }) + + it('routes notification taps through the coordinated transition, not a bare push', () => { + const start = rootLayoutSource.indexOf('// ─── Notification tap routing ───') + const end = rootLayoutSource.indexOf('// ─── End notification tap routing ───', start) + + // Assert the markers first: a renamed banner would otherwise slice garbage and report a + // missing call instead of the real cause. + expect(start).toBeGreaterThanOrEqual(0) + expect(end).toBeGreaterThan(start) + + const notificationEffect = rootLayoutSource.slice(start, end) + expect(notificationEffect).toContain('openNotificationRoute(target)') + expect(notificationEffect).not.toContain('router.push(') + }) +}) diff --git a/mobile/src/notifications/notification-routing.test.ts b/mobile/src/notifications/notification-routing.test.ts index ebf09e74d..6b5762aef 100644 --- a/mobile/src/notifications/notification-routing.test.ts +++ b/mobile/src/notifications/notification-routing.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { buildLocalNotificationData, getNotificationNavigationPath } from './notification-routing' +import { buildLocalNotificationData, getNotificationNavigationTarget } from './notification-routing' describe('notification routing', () => { it('includes the host id in locally scheduled notification data', () => { @@ -20,26 +20,36 @@ describe('notification routing', () => { }) }) + // Identities stay raw: the target is dispatched as navigator params, not a URL. it('routes notification taps to the worktree terminal screen', () => { expect( - getNotificationNavigationPath({ + getNotificationNavigationTarget({ hostId: 'host-1', worktreeId: 'repo::/Users/me/orca/workspaces/feature' }) - ).toBe('/h/host-1/session/repo%3A%3A%2FUsers%2Fme%2Forca%2Fworkspaces%2Ffeature') + ).toEqual({ + hostId: 'host-1', + sessionTarget: { + name: '[hostId]/session/[worktreeId]', + params: { hostId: 'host-1', worktreeId: 'repo::/Users/me/orca/workspaces/feature' } + } + }) }) it('falls back to the host screen when the payload has no worktree id', () => { - expect(getNotificationNavigationPath({ hostId: 'host-1' })).toBe('/h/host-1') + expect(getNotificationNavigationTarget({ hostId: 'host-1' })).toEqual({ + hostId: 'host-1', + sessionTarget: null + }) }) it('ignores payloads that cannot identify the paired host', () => { - expect(getNotificationNavigationPath({ worktreeId: 'repo::/tmp/worktree' })).toBeNull() + expect(getNotificationNavigationTarget({ worktreeId: 'repo::/tmp/worktree' })).toBeNull() }) it('ignores payloads for hosts that are no longer paired', () => { expect( - getNotificationNavigationPath( + getNotificationNavigationTarget( { hostId: 'removed-host', worktreeId: 'repo::/tmp/worktree' }, { knownHostIds: new Set(['host-1']) } ) diff --git a/mobile/src/notifications/notification-routing.ts b/mobile/src/notifications/notification-routing.ts index f38a06ec1..feda52c59 100644 --- a/mobile/src/notifications/notification-routing.ts +++ b/mobile/src/notifications/notification-routing.ts @@ -1,3 +1,6 @@ +import type { HostStackRouteTarget } from '../navigation/host-stack-navigation' +import { mobileSessionRouteTarget } from '../session/mobile-session-route' + export type DesktopNotificationSource = 'agent-task-complete' | 'terminal-bell' | 'test' export type DesktopNotificationEvent = { @@ -38,10 +41,17 @@ export function buildLocalNotificationData( return data } -export function getNotificationNavigationPath( +/** Where a tap should land. `sessionTarget` is null for a host-only notification, whose + * `/h/` push is shallow enough to need no host-stack coordination. */ +export type NotificationNavigationTarget = Readonly<{ + hostId: string + sessionTarget: HostStackRouteTarget | null +}> + +export function getNotificationNavigationTarget( data: unknown, options: NotificationNavigationOptions = {} -): string | null { +): NotificationNavigationTarget | null { if (!data || typeof data !== 'object') { return null } @@ -55,11 +65,9 @@ export function getNotificationNavigationPath( return null } - const hostPath = `/h/${encodeURIComponent(hostId)}` const worktreeId = readNonEmptyString(record.worktreeId) - if (!worktreeId) { - return hostPath + return { + hostId, + sessionTarget: worktreeId ? mobileSessionRouteTarget({ hostId, worktreeId }) : null } - - return `${hostPath}/session/${encodeURIComponent(worktreeId)}` } diff --git a/mobile/src/notifications/use-open-notification-route.ts b/mobile/src/notifications/use-open-notification-route.ts new file mode 100644 index 000000000..3a0809c7b --- /dev/null +++ b/mobile/src/notifications/use-open-notification-route.ts @@ -0,0 +1,21 @@ +import { useCallback } from 'react' +import { useRouter } from 'expo-router' +import { hostStackHostRoute } from '../navigation/host-stack-navigation' +import { useOpenHostStackRoute } from '../navigation/use-open-host-stack-route' +import type { NotificationNavigationTarget } from './notification-routing' + +export function useOpenNotificationRoute(): (target: NotificationNavigationTarget) => void { + const openHostStackRoute = useOpenHostStackRoute() + const router = useRouter() + + return useCallback( + (target) => { + if (target.sessionTarget) { + openHostStackRoute(target.hostId, target.sessionTarget) + return + } + router.push(hostStackHostRoute(target.hostId)) + }, + [openHostStackRoute, router] + ) +} diff --git a/mobile/src/session/mobile-session-route.test.ts b/mobile/src/session/mobile-session-route.test.ts index e97af9df3..56b9fac7a 100644 --- a/mobile/src/session/mobile-session-route.test.ts +++ b/mobile/src/session/mobile-session-route.test.ts @@ -106,7 +106,19 @@ describe('mobile session route', () => { expect(end).toBeGreaterThan(start) const resumeCard = homeSource.slice(start, end) - expect(resumeCard).toContain('openMobileSession({') + expect(resumeCard).toContain('openResume(') expect(resumeCard).not.toContain('router.push(') + + // The tap handler itself must go through the coordinated transition; its only + // direct push is the shallow noticed host-index route for a proven-missing target. + const handlerStart = homeSource.indexOf('const openResume = useCallback(') + const handlerEnd = homeSource.indexOf('[openMobileSession, router]', handlerStart) + expect(handlerStart).toBeGreaterThanOrEqual(0) + expect(handlerEnd).toBeGreaterThan(handlerStart) + + const openResume = homeSource.slice(handlerStart, handlerEnd) + expect(openResume).toContain('openMobileSession({') + expect(openResume.match(/router\.push\(/g)).toHaveLength(1) + expect(openResume).toContain('router.push(hostRouteWithNotice(') }) }) diff --git a/mobile/src/session/synthetic-workspace-route.ts b/mobile/src/session/synthetic-workspace-route.ts new file mode 100644 index 000000000..5dc47e530 --- /dev/null +++ b/mobile/src/session/synthetic-workspace-route.ts @@ -0,0 +1,8 @@ +import { isFloatingWorkspaceWorktreeId } from './floating-workspace' + +/** Route ids that name no managed worktree, so the host can never list or resolve them. + * Every "is this workspace still there?" check must exempt them, or their permanent + * absence from the catalog reads as a deletion. */ +export function isSyntheticWorkspaceRoute(worktreeId: string): boolean { + return worktreeId.startsWith('folder:') || isFloatingWorkspaceWorktreeId(worktreeId) +} diff --git a/mobile/src/session/use-live-worktree-name.test.ts b/mobile/src/session/use-live-worktree-name.test.ts index 8ac565c76..45e64205d 100644 --- a/mobile/src/session/use-live-worktree-name.test.ts +++ b/mobile/src/session/use-live-worktree-name.test.ts @@ -175,7 +175,7 @@ describe('useLiveWorktreeName request volume', () => { connState: 'connected', routeName: undefined, worktreeId: 'global-floating-terminal' - }) + }).name return null } @@ -202,7 +202,7 @@ describe('useLiveWorktreeName request volume', () => { let renderer: ReactTestRenderer | null = null function RouteHarness(props: { routeName?: string; worktreeId: string }): null { - const name = useLiveWorktreeName({ + const { name } = useLiveWorktreeName({ client, connState: 'connected', routeName: props.routeName, @@ -248,4 +248,62 @@ describe('useLiveWorktreeName request volume', () => { expect(firstNameByWorktree.get('global-floating-terminal')).toBe('Floating Workspace') expect(firstNameByWorktree.get('repo-2::/worktree')).toBe('Next workspace') }) + + // The bounce in use-missing-worktree-bounce.ts rides this poll rather than adding a second + // RPC, so the failure branch has to publish a verdict instead of returning early. + it('reports the host-proven verdict from the same poll', async () => { + let resolution = '' + function VerdictHarness(): null { + resolution = useLiveWorktreeName({ + client, + connState: 'connected', + routeName: 'Route name', + worktreeId: 'repo-1::/worktree' + }).resolution + return null + } + const mount = async (): Promise => { + const restoreConsoleError = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(VerdictHarness)) + await Promise.resolve() + }) + } finally { + restoreConsoleError() + } + } + + await mount() + expect(resolution).toBe('present') + + act(() => renderer?.unmount()) + renderer = null + sendRequest.mockResolvedValue({ + id: 'worktree-show', + ok: false, + error: { code: 'selector_not_found', message: 'Selector not found' }, + _meta: { runtimeId: 'runtime-1' } + }) + await mount() + // Why: a transient desktop repo-scan rejection also answers selector_not_found, + // so one miss stays unproven; only the confirming poll may say 'missing'. + expect(resolution).toBe('unknown') + await act(async () => { + await vi.advanceTimersByTimeAsync(3_000) + }) + expect(resolution).toBe('missing') + + act(() => renderer?.unmount()) + renderer = null + // A dropped socket is not a deletion, so it must leave the verdict unproven. + sendRequest.mockResolvedValue({ + id: 'worktree-show', + ok: false, + error: { code: 'runtime_busy', message: 'Runtime busy' }, + _meta: { runtimeId: 'runtime-1' } + }) + await mount() + expect(resolution).toBe('unknown') + }) }) diff --git a/mobile/src/session/use-live-worktree-name.ts b/mobile/src/session/use-live-worktree-name.ts index 53ef1cf1b..a2bc6d117 100644 --- a/mobile/src/session/use-live-worktree-name.ts +++ b/mobile/src/session/use-live-worktree-name.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { useFocusEffect } from 'expo-router' import type { RuntimeClientEventStreamMessage } from '../../../src/shared/runtime-client-events' import { getRepoIdFromWorktreeId } from '../../../src/shared/worktree-id' @@ -6,6 +6,10 @@ import type { RpcClient } from '../transport/rpc-client' import type { ConnectionState, RpcSuccess } from '../transport/types' import { getLiveWorktreeDisplayName, type WorktreeDisplayNameSource } from './worktree-display-name' import { FLOATING_WORKSPACE_TITLE, isFloatingWorkspaceWorktreeId } from './floating-workspace' +import { + classifyWorktreeShowResponse, + type WorktreeShowResolution +} from '../worktree/worktree-show-resolution' const WORKTREE_NAME_FALLBACK_POLL_MS = 3000 @@ -16,7 +20,19 @@ type Params = { worktreeId: string } -export function useLiveWorktreeName({ client, connState, routeName, worktreeId }: Params): string { +export type LiveWorktreeName = { + name: string + /** What the host last proved about this worktree still existing — see the bounce in + * use-missing-worktree-bounce.ts. */ + resolution: WorktreeShowResolution +} + +export function useLiveWorktreeName({ + client, + connState, + routeName, + worktreeId +}: Params): LiveWorktreeName { // Why: the floating sentinel has no worktree record, so worktree.show would // fail forever and keep the 3s fallback poll alive; its title is fixed. const isFloatingWorkspace = isFloatingWorkspaceWorktreeId(worktreeId) @@ -25,6 +41,15 @@ export function useLiveWorktreeName({ client, connState, routeName, worktreeId } worktreeId, name: routeNameHint })) + // Why: keyed by id so a verdict about the previous route can never survive into the next one. + const [resolved, setResolved] = useState<{ + worktreeId: string + resolution: WorktreeShowResolution + }>(() => ({ worktreeId, resolution: 'unknown' })) + // Why: a transient desktop repo-scan rejection collapses the catalog to zero rows and + // answers selector_not_found for a live worktree — one miss is suspicion, not proof. + // The fallback poll guarantees a confirming read (a failed show never stops it). + const missingStreakRef = useRef({ worktreeId, count: 0 }) useEffect(() => { setWorktreeName((current) => @@ -34,6 +59,12 @@ export function useLiveWorktreeName({ client, connState, routeName, worktreeId } ) }, [routeNameHint, worktreeId]) + useEffect(() => { + setResolved((current) => + current.worktreeId === worktreeId ? current : { worktreeId, resolution: 'unknown' } + ) + }, [worktreeId]) + useFocusEffect( useCallback(() => { if (isFloatingWorkspace || !client || connState !== 'connected') { @@ -60,7 +91,26 @@ export function useLiveWorktreeName({ client, connState, routeName, worktreeId } const response = await client.sendRequest('worktree.show', { worktree: `id:${worktreeId}` }) - if (stale || generation !== refreshGeneration || !response.ok) { + if (stale || generation !== refreshGeneration) { + return + } + const classified = classifyWorktreeShowResponse(response) + const streak = missingStreakRef.current + missingStreakRef.current = { + worktreeId, + count: + classified === 'missing' + ? (streak.worktreeId === worktreeId ? streak.count : 0) + 1 + : 0 + } + const resolution = + classified === 'missing' && missingStreakRef.current.count < 2 ? 'unknown' : classified + setResolved((current) => + current.worktreeId === worktreeId && current.resolution === resolution + ? current + : { worktreeId, resolution } + ) + if (!response.ok) { return } const result = (response as RpcSuccess).result as { @@ -148,7 +198,11 @@ export function useLiveWorktreeName({ client, connState, routeName, worktreeId } ) if (isFloatingWorkspace) { - return FLOATING_WORKSPACE_TITLE + // The sentinel has no worktree record to resolve, so nothing is ever proven about it. + return { name: FLOATING_WORKSPACE_TITLE, resolution: 'unknown' } + } + return { + name: worktreeName.worktreeId === worktreeId ? worktreeName.name : routeNameHint, + resolution: resolved.worktreeId === worktreeId ? resolved.resolution : 'unknown' } - return worktreeName.worktreeId === worktreeId ? worktreeName.name : routeNameHint } diff --git a/mobile/src/session/use-missing-worktree-bounce.test.ts b/mobile/src/session/use-missing-worktree-bounce.test.ts new file mode 100644 index 000000000..209486bc8 --- /dev/null +++ b/mobile/src/session/use-missing-worktree-bounce.test.ts @@ -0,0 +1,79 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { WorktreeShowResolution } from '../worktree/worktree-show-resolution' +import { isSyntheticWorkspaceRoute } from './synthetic-workspace-route' +import { + shouldBounceMissingWorktree, + useMissingWorktreeBounce +} from './use-missing-worktree-bounce' + +describe('shouldBounceMissingWorktree', () => { + it('bounces only a host-proven missing worktree', () => { + expect(shouldBounceMissingWorktree('repo::wt', 'missing')).toBe(true) + expect(shouldBounceMissingWorktree('repo::wt', 'unknown')).toBe(false) + expect(shouldBounceMissingWorktree('repo::wt', 'present')).toBe(false) + }) + + it('never bounces synthetic routes the host cannot resolve', () => { + expect(isSyntheticWorkspaceRoute('folder:/Users/x/dir')).toBe(true) + expect(shouldBounceMissingWorktree('folder:/Users/x/dir', 'missing')).toBe(false) + expect(shouldBounceMissingWorktree('global-floating-terminal', 'missing')).toBe(false) + }) +}) + +describe('useMissingWorktreeBounce', () => { + let renderer: ReactTestRenderer | null = null + const bounce = vi.fn() + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + bounce.mockReset() + renderer = null + }) + + function Harness(props: { worktreeId: string; resolution: WorktreeShowResolution }): null { + useMissingWorktreeBounce({ + hostId: 'host-1', + worktreeId: props.worktreeId, + resolution: props.resolution, + bounce + }) + return null + } + + function render(worktreeId: string, resolution: WorktreeShowResolution): void { + act(() => { + const element = createElement(Harness, { worktreeId, resolution }) + if (renderer) { + renderer.update(element) + } else { + renderer = create(element) + } + }) + } + + it('bounces exactly once per worktree even across re-renders', () => { + render('repo::wt', 'unknown') + expect(bounce).not.toHaveBeenCalled() + + render('repo::wt', 'missing') + expect(bounce).toHaveBeenCalledExactlyOnceWith('host-1') + + // Why: navigation lands after the render, so the pre-unmount renders must not re-fire. + render('repo::wt', 'missing') + expect(bounce).toHaveBeenCalledTimes(1) + act(() => renderer?.unmount()) + }) + + it('re-arms for a different worktree on the reused screen', () => { + render('repo::wt-1', 'missing') + expect(bounce).toHaveBeenCalledTimes(1) + + render('repo::wt-2', 'unknown') + expect(bounce).toHaveBeenCalledTimes(1) + render('repo::wt-2', 'missing') + expect(bounce).toHaveBeenCalledTimes(2) + act(() => renderer?.unmount()) + }) +}) diff --git a/mobile/src/session/use-missing-worktree-bounce.ts b/mobile/src/session/use-missing-worktree-bounce.ts new file mode 100644 index 000000000..cadc9ea79 --- /dev/null +++ b/mobile/src/session/use-missing-worktree-bounce.ts @@ -0,0 +1,41 @@ +import { useEffect, useRef } from 'react' +import { isSyntheticWorkspaceRoute } from './synthetic-workspace-route' +import type { WorktreeShowResolution } from '../worktree/worktree-show-resolution' + +export function shouldBounceMissingWorktree( + worktreeId: string, + resolution: WorktreeShowResolution +): boolean { + return resolution === 'missing' && !isSyntheticWorkspaceRoute(worktreeId) +} + +/** Sends the route back to the host index once the host has *proven* the worktree is gone — + * a workspace deleted on the desktop while the phone held the link (Resume, a notification, + * a cold deep link) otherwise lands on a session screen whose every RPC fails. */ +export function useMissingWorktreeBounce(args: { + hostId: string + worktreeId: string + resolution: WorktreeShowResolution + bounce: (hostId: string) => void +}): void { + const { hostId, worktreeId, resolution } = args + // Why: navigation takes effect after this render, so without a latch the renders before + // unmount would each fire again — and it lets callers pass an inline bounce closure. + const bouncedRef = useRef(null) + const bounceRef = useRef(args.bounce) + // Why: synced in an effect (render must stay pure); declared first so the + // bounce effect below always sees the freshest closure in the same commit. + useEffect(() => { + bounceRef.current = args.bounce + }) + useEffect(() => { + if (!hostId || bouncedRef.current === worktreeId) { + return + } + if (!shouldBounceMissingWorktree(worktreeId, resolution)) { + return + } + bouncedRef.current = worktreeId + bounceRef.current(hostId) + }, [hostId, worktreeId, resolution]) +} diff --git a/mobile/src/session/use-mobile-diff-review-controller.test.ts b/mobile/src/session/use-mobile-diff-review-controller.test.ts new file mode 100644 index 000000000..7c0a80455 --- /dev/null +++ b/mobile/src/session/use-mobile-diff-review-controller.test.ts @@ -0,0 +1,133 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { ConnectionState } from '../transport/types' +import type { ReviewScreenState } from './mobile-diff-review-screen-model' +import { useMobileDiffReviewController } from './use-mobile-diff-review-controller' + +const loadSnapshot = vi.hoisted(() => vi.fn()) +vi.mock('./mobile-diff-review-loaders', () => ({ + loadMobileDiffReviewSnapshot: loadSnapshot, + loadMobileDiffReviewDiff: vi.fn().mockResolvedValue({ kind: 'idle' }) +})) +vi.mock('react-native', () => ({ Platform: { OS: 'ios' } })) +vi.mock('expo-haptics', () => ({ + impactAsync: vi.fn(), + notificationAsync: vi.fn(), + selectionAsync: vi.fn(), + performAndroidHapticsAsync: vi.fn(), + AndroidHaptics: {}, + ImpactFeedbackStyle: {}, + NotificationFeedbackType: {} +})) +vi.mock('expo-clipboard', () => ({ setStringAsync: vi.fn() })) + +const client = { sendRequest: vi.fn() } as unknown as RpcClient + +function readySnapshot(branch: string): ReviewScreenState { + return { + kind: 'ready', + status: { entries: [], branch, head: 'abc123' }, + comments: [], + reviewState: { reviewedKeys: [] }, + branchCompare: null + } as unknown as ReviewScreenState +} + +describe('useMobileDiffReviewController', () => { + let renderer: ReactTestRenderer | null = null + let screenState: ReviewScreenState = { kind: 'loading' } + + function Probe({ connState }: { connState: ConnectionState }): null { + const controller = useMobileDiffReviewController({ + client, + connState, + hostId: 'host-1', + worktreeId: 'wt-1', + name: 'review', + initialFilter: 'all', + initialTarget: null, + onOpenSession: () => {}, + onReconnect: () => {} + }) + screenState = controller.screenState + return null + } + + async function update(connState: ConnectionState): Promise { + await act(async () => { + renderer?.update(createElement(Probe, { connState })) + await Promise.resolve() + }) + } + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + loadSnapshot.mockReset() + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + it('keeps the loaded review across a disconnect and its reconnect reload', async () => { + let releaseReload: (() => void) | null = null + loadSnapshot.mockResolvedValueOnce(readySnapshot('feature/one')).mockImplementationOnce( + () => + new Promise((resolve) => { + releaseReload = () => resolve(readySnapshot('feature/two')) + }) + ) + + await act(async () => { + renderer = create(createElement(Probe, { connState: 'connected' })) + await Promise.resolve() + }) + expect(screenState).toMatchObject({ kind: 'ready' }) + + await update('reconnecting') + expect(screenState).toMatchObject({ kind: 'ready', status: { branch: 'feature/one' } }) + + await update('connected') + expect(screenState).toMatchObject({ kind: 'ready', status: { branch: 'feature/one' } }) + + await act(async () => { + releaseReload?.() + await Promise.resolve() + }) + expect(screenState).toMatchObject({ kind: 'ready', status: { branch: 'feature/two' } }) + }) + + it('keeps the loaded review when the reconnect reload rejects', async () => { + loadSnapshot + .mockResolvedValueOnce(readySnapshot('feature/one')) + .mockRejectedValueOnce(new Error('snapshot fetch failed')) + + await act(async () => { + renderer = create(createElement(Probe, { connState: 'connected' })) + await Promise.resolve() + }) + expect(screenState).toMatchObject({ kind: 'ready' }) + + await update('reconnecting') + await update('connected') + expect(loadSnapshot).toHaveBeenCalledTimes(2) + // Why (F10): a failed refresh must not replace the review on screen with an error. + expect(screenState).toMatchObject({ kind: 'ready', status: { branch: 'feature/one' } }) + }) + + it('waits for the desktop when the drop lands before the review loads', async () => { + loadSnapshot.mockReturnValueOnce(new Promise(() => {})) + + await act(async () => { + renderer = create(createElement(Probe, { connState: 'connected' })) + await Promise.resolve() + }) + expect(screenState).toMatchObject({ kind: 'loading' }) + + await update('disconnected') + expect(screenState).toMatchObject({ kind: 'error', message: 'Waiting for desktop...' }) + }) +}) diff --git a/mobile/src/session/use-mobile-diff-review-controller.ts b/mobile/src/session/use-mobile-diff-review-controller.ts index 5b1b5fcf9..d57276f1f 100644 --- a/mobile/src/session/use-mobile-diff-review-controller.ts +++ b/mobile/src/session/use-mobile-diff-review-controller.ts @@ -16,15 +16,12 @@ import { findMobileDiffReviewInitialIndex, type MobileDiffReviewInitialTarget } from './mobile-diff-review-positioning' -import { - loadMobileDiffReviewDiff, - loadMobileDiffReviewSnapshot -} from './mobile-diff-review-loaders' +import { loadMobileDiffReviewSnapshot } from './mobile-diff-review-loaders' +import { useMobileDiffReviewDiffLoading } from './use-mobile-diff-review-diff-loading' import { canOpenMobileBranchCompareDiff } from '../source-control/mobile-branch-compare' import type { ComposerState, ReviewDiffLine, - ReviewDiffState, ReviewScreenState, SendSheetState } from './mobile-diff-review-screen-model' @@ -60,7 +57,6 @@ export function useMobileDiffReviewController(input: ControllerInput) { const seededInitialTargetRef = useRef(false) const initialTargetKey = initialTarget ? `${initialTarget.area}\0${initialTarget.filePath}` : '' const [screenState, setScreenState] = useState({ kind: 'loading' }) - const [diffState, setDiffState] = useState({ kind: 'idle' }) const [filter, setFilter] = useState(initialFilter) const [currentIndex, setCurrentIndex] = useState(0) const [activeHunkIndex, setActiveHunkIndex] = useState(null) @@ -82,11 +78,15 @@ export function useMobileDiffReviewController(input: ControllerInput) { setScreenState({ kind: 'error', message: 'Missing worktree' }) return } + // Why (F10): a loaded review outlives a blip — the waiting state is for a screen with nothing + // to show, and this branch (not the one below it) is the one a drop actually reaches. + const keepReady = (fallback: ReviewScreenState) => (prev: ReviewScreenState) => + prev.kind === 'ready' ? prev : fallback if (!client || connState !== 'connected') { - setScreenState({ kind: 'error', message: 'Waiting for desktop...' }) + setScreenState(keepReady({ kind: 'error', message: 'Waiting for desktop...' })) return } - setScreenState((prev) => (prev.kind === 'ready' ? prev : { kind: 'loading' })) + setScreenState(keepReady({ kind: 'loading' })) try { const nextState = await loadMobileDiffReviewSnapshot(client, worktreeId) if (!isCurrent()) { @@ -96,10 +96,14 @@ export function useMobileDiffReviewController(input: ControllerInput) { setActionError(nextState.kind === 'ready' ? (nextState.branchError ?? null) : null) } catch (err) { if (isCurrent()) { - setScreenState({ - kind: 'error', - message: err instanceof Error ? err.message : 'Unable to load review' - }) + // Why (F10): a failed refresh after reconnect must not destroy the review + // already on screen; the error state is for a screen with nothing to show. + setScreenState( + keepReady({ + kind: 'error', + message: err instanceof Error ? err.message : 'Unable to load review' + }) + ) } } }, [client, connState, worktreeId]) @@ -160,42 +164,14 @@ export function useMobileDiffReviewController(input: ControllerInput) { } }, [currentIndex, filteredQueue.length]) - useEffect(() => { - setActiveHunkIndex(null) - if (!currentItem || screenState.kind !== 'ready') { - setDiffState({ kind: 'idle' }) - return - } - if (!client || connState !== 'connected') { - setDiffState({ kind: 'error', itemKey: currentItem.key, message: 'Waiting for desktop...' }) - return - } - let stale = false - setDiffState({ kind: 'loading', itemKey: currentItem.key }) - void loadMobileDiffReviewDiff({ - client, - worktreeId, - item: currentItem, - branchCompare: screenState.branchCompare - }) - .then((nextState) => { - if (!stale) { - setDiffState(nextState) - } - }) - .catch((err: unknown) => { - if (!stale) { - setDiffState({ - kind: 'error', - itemKey: currentItem.key, - message: err instanceof Error ? err.message : 'Unable to load diff' - }) - } - }) - return () => { - stale = true - } - }, [client, connState, currentItem, screenState, worktreeId]) + const diffState = useMobileDiffReviewDiffLoading({ + client, + connState, + worktreeId, + currentItem, + screenState, + setActiveHunkIndex + }) const commentsForCurrentItem = useMemo(() => { if (!currentItem || screenState.kind !== 'ready') { diff --git a/mobile/src/session/use-mobile-diff-review-diff-loading.test.ts b/mobile/src/session/use-mobile-diff-review-diff-loading.test.ts new file mode 100644 index 000000000..3245c2282 --- /dev/null +++ b/mobile/src/session/use-mobile-diff-review-diff-loading.test.ts @@ -0,0 +1,119 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { ConnectionState } from '../transport/types' +import type { MobileDiffReviewQueueItem } from './mobile-diff-review-queue' +import type { ReviewDiffState, ReviewScreenState } from './mobile-diff-review-screen-model' +import { useMobileDiffReviewDiffLoading } from './use-mobile-diff-review-diff-loading' + +const loadDiff = vi.hoisted(() => vi.fn()) +vi.mock('./mobile-diff-review-loaders', () => ({ loadMobileDiffReviewDiff: loadDiff })) + +const client = { sendRequest: vi.fn() } as unknown as RpcClient +// Stable identity, like the controller's useState setter: it is an effect dependency. +const setActiveHunkIndex = () => {} +const currentItem = { key: 'item-1', filePath: 'src/app.ts' } as MobileDiffReviewQueueItem +const readyScreen = { kind: 'ready', branchCompare: null } as unknown as ReviewScreenState + +function readyDiff(firstLine: string): ReviewDiffState { + return { + kind: 'ready', + itemKey: 'item-1', + lines: [{ kind: 'context', text: firstLine }], + hunks: [], + truncated: false + } as unknown as ReviewDiffState +} + +describe('useMobileDiffReviewDiffLoading', () => { + let renderer: ReactTestRenderer | null = null + let diffState: ReviewDiffState = { kind: 'idle' } + + function Probe({ connState }: { connState: ConnectionState }): null { + diffState = useMobileDiffReviewDiffLoading({ + client, + connState, + worktreeId: 'wt-1', + currentItem, + screenState: readyScreen, + setActiveHunkIndex + }) + return null + } + + async function render(connState: ConnectionState): Promise { + await act(async () => { + renderer = create(createElement(Probe, { connState })) + await Promise.resolve() + }) + } + + async function update(connState: ConnectionState): Promise { + await act(async () => { + renderer?.update(createElement(Probe, { connState })) + await Promise.resolve() + }) + } + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + loadDiff.mockReset() + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + it('keeps the loaded diff when the reconnect refetch rejects', async () => { + loadDiff + .mockResolvedValueOnce(readyDiff('before the drop')) + .mockRejectedValueOnce(new Error('diff fetch failed')) + + await render('connected') + expect(diffState).toMatchObject({ kind: 'ready' }) + + await update('reconnecting') + await update('connected') + // Why (F10): a failed refetch must not erase the diff (or hunk context) on screen. + expect(diffState).toMatchObject({ kind: 'ready', lines: [{ text: 'before the drop' }] }) + expect(loadDiff).toHaveBeenCalledTimes(2) + }) + + it('keeps the loaded diff through a disconnect and its reconnect refetch', async () => { + let releaseRefetch: (() => void) | null = null + loadDiff.mockResolvedValueOnce(readyDiff('before the drop')).mockImplementationOnce( + () => + new Promise((resolve) => { + releaseRefetch = () => resolve(readyDiff('after the drop')) + }) + ) + + await render('connected') + expect(diffState).toMatchObject({ kind: 'ready' }) + + await update('reconnecting') + expect(diffState).toMatchObject({ kind: 'ready', lines: [{ text: 'before the drop' }] }) + + await update('connected') + expect(diffState).toMatchObject({ kind: 'ready', lines: [{ text: 'before the drop' }] }) + + await act(async () => { + releaseRefetch?.() + await Promise.resolve() + }) + expect(diffState).toMatchObject({ kind: 'ready', lines: [{ text: 'after the drop' }] }) + expect(loadDiff).toHaveBeenCalledTimes(2) + }) + + it('waits for the desktop when the drop lands before any diff is loaded', async () => { + loadDiff.mockReturnValueOnce(new Promise(() => {})) + + await render('connected') + expect(diffState).toMatchObject({ kind: 'loading', itemKey: 'item-1' }) + + await update('disconnected') + expect(diffState).toMatchObject({ kind: 'error', message: 'Waiting for desktop...' }) + }) +}) diff --git a/mobile/src/session/use-mobile-diff-review-diff-loading.ts b/mobile/src/session/use-mobile-diff-review-diff-loading.ts new file mode 100644 index 000000000..db2e1540e --- /dev/null +++ b/mobile/src/session/use-mobile-diff-review-diff-loading.ts @@ -0,0 +1,79 @@ +import { useEffect, useRef, useState } from 'react' +import type { ConnectionState } from '../transport/types' +import type { RpcClient } from '../transport/rpc-client' +import { loadMobileDiffReviewDiff } from './mobile-diff-review-loaders' +import type { MobileDiffReviewQueueItem } from './mobile-diff-review-queue' +import type { ReviewDiffState, ReviewScreenState } from './mobile-diff-review-screen-model' + +type DiffLoadingInput = { + client: RpcClient | null + connState: ConnectionState + worktreeId: string + currentItem: MobileDiffReviewQueueItem | null + screenState: ReviewScreenState + setActiveHunkIndex: (index: number | null) => void +} + +// Owns the diff body for the reviewed item. Split out of the review controller so the loaded diff +// can survive a transport blip: a drop re-runs this effect, and (F10) a diff already on screen for +// the same item stays there instead of being replaced by "Waiting for desktop..." or a spinner. +export function useMobileDiffReviewDiffLoading(input: DiffLoadingInput): ReviewDiffState { + const { client, connState, worktreeId, currentItem, screenState, setActiveHunkIndex } = input + const [diffState, setDiffState] = useState({ kind: 'idle' }) + const hunkResetKeyRef = useRef(null) + // Why: depend on the two fields this effect reads, not the screenState object — + // an identity-only change must not restart the git.diff request. + const screenReady = screenState.kind === 'ready' + const branchCompare = screenState.kind === 'ready' ? screenState.branchCompare : null + + useEffect(() => { + // Why (F10): a connection blip re-runs this effect; the reader's hunk position must + // survive it and reset only when the reviewed item actually changes. + const hunkKey = currentItem?.key ?? null + if (hunkResetKeyRef.current !== hunkKey) { + hunkResetKeyRef.current = hunkKey + setActiveHunkIndex(null) + } + if (!currentItem || !screenReady) { + setDiffState({ kind: 'idle' }) + return + } + const itemKey = currentItem.key + const keepLoadedDiff = (fallback: ReviewDiffState) => (prev: ReviewDiffState) => + prev.kind === 'ready' && prev.itemKey === itemKey ? prev : fallback + if (!client || connState !== 'connected') { + setDiffState(keepLoadedDiff({ kind: 'error', itemKey, message: 'Waiting for desktop...' })) + return + } + let stale = false + setDiffState(keepLoadedDiff({ kind: 'loading', itemKey })) + void loadMobileDiffReviewDiff({ + client, + worktreeId, + item: currentItem, + branchCompare + }) + .then((nextState) => { + if (!stale) { + setDiffState(nextState) + } + }) + .catch((err: unknown) => { + if (!stale) { + // Why (F10): a rejected reconnect refetch must not erase the diff on screen. + setDiffState( + keepLoadedDiff({ + kind: 'error', + itemKey, + message: err instanceof Error ? err.message : 'Unable to load diff' + }) + ) + } + }) + return () => { + stale = true + } + }, [client, connState, currentItem, screenReady, branchCompare, setActiveHunkIndex, worktreeId]) + + return diffState +} diff --git a/mobile/src/source-control/MobileGitHistoryList.test.tsx b/mobile/src/source-control/MobileGitHistoryList.test.tsx new file mode 100644 index 000000000..50b0fa7fe --- /dev/null +++ b/mobile/src/source-control/MobileGitHistoryList.test.tsx @@ -0,0 +1,168 @@ +import { createElement, type ReactElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { ConnectionState } from '../transport/types' +import { MobileGitHistoryList } from './MobileGitHistoryList' + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + FlatList: ({ + data, + renderItem + }: { + data: { id: string }[] + renderItem: (info: { item: { id: string } }) => ReactElement + }) => + createElement( + 'FlatList', + null, + data.map((item) => createElement('Row', { key: item.id }, renderItem({ item }))) + ), + Pressable: 'Pressable', + StyleSheet: { create: (styles: T) => styles }, + Text: 'Text', + View: 'View' +})) +vi.mock('lucide-react-native', () => ({ ChevronDown: 'ChevronDown', ChevronRight: 'ChevronRight' })) +vi.mock('../transport/client-context', () => ({ useForceReconnect: () => vi.fn() })) + +function historyResponse(subject: string) { + return { + ok: true, + result: { + items: [{ id: 'commit-1', displayId: 'c0mm1t1', subject, author: 'Ada', parentIds: [] }] + } + } +} + +const compareResponse = { + ok: true, + result: { entries: [{ path: 'src/app.ts', added: 3, removed: 1 }] } +} + +describe('MobileGitHistoryList', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + }) + + function listElement(client: RpcClient | null, connState: ConnectionState) { + return createElement(MobileGitHistoryList, { + client, + connState, + worktreeId: 'wt-1', + hostId: 'host-1', + bottomInset: 0 + }) + } + + async function render(client: RpcClient, connState: ConnectionState): Promise { + await act(async () => { + renderer = create(listElement(client, connState)) + await Promise.resolve() + }) + } + + async function update(client: RpcClient | null, connState: ConnectionState): Promise { + await act(async () => { + renderer?.update(listElement(client, connState)) + await Promise.resolve() + }) + } + + function tree(): string { + return JSON.stringify(renderer?.toJSON()) + } + + it('keeps loaded commits visible across a disconnect and its reconnect refetch', async () => { + let releaseRefetch: (() => void) | null = null + const sendRequest = vi + .fn() + .mockResolvedValueOnce(historyResponse('first load')) + .mockImplementationOnce( + () => + new Promise((resolve) => { + releaseRefetch = () => resolve(historyResponse('after reconnect')) + }) + ) + const client = { sendRequest } as unknown as RpcClient + + await render(client, 'connected') + expect(tree()).toContain('first load') + + await update(client, 'reconnecting') + expect(tree()).toContain('first load') + + // The refetch is in flight: old rows must stay up instead of flashing empty. + await update(client, 'connected') + expect(tree()).toContain('first load') + + await act(async () => { + releaseRefetch?.() + await Promise.resolve() + }) + expect(tree()).toContain('after reconnect') + expect(sendRequest).toHaveBeenCalledTimes(2) + }) + + it('wipes commits when the worktree identity changes', async () => { + const sendRequest = vi + .fn() + .mockResolvedValueOnce(historyResponse('worktree one')) + .mockReturnValueOnce(new Promise(() => {})) + const client = { sendRequest } as unknown as RpcClient + + await render(client, 'connected') + expect(tree()).toContain('worktree one') + + await act(async () => { + renderer?.update( + createElement(MobileGitHistoryList, { + client, + connState: 'connected', + worktreeId: 'wt-2', + hostId: 'host-1', + bottomInset: 0 + }) + ) + }) + expect(tree()).not.toContain('worktree one') + }) + + it('refetches the expanded commit files after a reconnect instead of caching the outage answer', async () => { + const sendRequest = vi.fn().mockImplementation((method: string) => { + if (method === 'git.history') { + return Promise.resolve(historyResponse('expandable')) + } + return Promise.resolve(compareResponse) + }) + const client = { sendRequest } as unknown as RpcClient + + await render(client, 'connected') + await update(client, 'disconnected') + + const row = renderer?.root.findAll( + (node) => node.type === 'Pressable' && node.props.onPress !== undefined + )[0] + await act(async () => { + row?.props.onPress() + }) + // Offline expand cannot request anything, so nothing is cached as "no file changes". + expect(tree()).toContain('Waiting for desktop...') + expect(sendRequest).toHaveBeenCalledTimes(1) + + await update(client, 'connected') + expect(sendRequest).toHaveBeenCalledWith('git.commitCompare', { + worktree: 'id:wt-1', + commitId: 'commit-1' + }) + expect(tree()).toContain('src/app.ts') + }) +}) diff --git a/mobile/src/source-control/MobileGitHistoryList.tsx b/mobile/src/source-control/MobileGitHistoryList.tsx index d88fae540..dd1a626fd 100644 --- a/mobile/src/source-control/MobileGitHistoryList.tsx +++ b/mobile/src/source-control/MobileGitHistoryList.tsx @@ -43,14 +43,14 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ const [expanded, setExpanded] = useState(null) const [filesById, setFilesById] = useState>({}) - // Worktree identity change must wipe history immediately — even while + // Host or worktree identity change must wipe history immediately — even while // disconnected — so a kept-mounted hub segment never shows another tree's commits. useEffect(() => { setRows(null) setError(null) setExpanded(null) setFilesById({}) - }, [worktreeId]) + }, [hostId, worktreeId]) useEffect(() => { let active = true @@ -59,12 +59,9 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ // resolveMobileHistoryScreenView keeps them visible (STA-1511). return } - // Reset prior error/rows so a successful retry doesn't stay stuck behind a - // stale error (error wins render precedence). + // Why (F10): clear only the error (it wins render precedence, so a stale one would outlive a + // successful retry) — the loaded rows stay up until fresh ones land instead of flashing empty. setError(null) - setRows(null) - setExpanded(null) - setFilesById({}) void (async () => { try { const result = await fetchMobileGitHistory(client, worktreeId) @@ -95,45 +92,43 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ setReloadNonce((n) => n + 1) }, [connState, forceReconnect, hostId]) - const toggleCommit = useCallback( - (row: MobileCommitRow) => { - const next = expanded === row.id ? null : row.id - setExpanded(next) - if (next && !filesById[row.id]) { - // No client (disconnected while cached rows stay visible): resolve to an - // empty file list so the row shows "No file changes" instead of a spinner - // that never completes — no request can be made. - if (!client) { - setFilesById((prev) => ({ ...prev, [row.id]: [] })) - return + const toggleCommit = useCallback((row: MobileCommitRow) => { + setExpanded((current) => (current === row.id ? null : row.id)) + }, []) + + // Why (F10): the expanded commit's files load here, not in the tap handler, so a row expanded + // during an outage refetches on reconnect instead of caching the outage's answer forever. + useEffect(() => { + if (!expanded || !client || connState !== 'connected') { + return + } + const commitId = expanded + let stale = false + setFilesById((prev) => (prev[commitId] ? prev : { ...prev, [commitId]: 'loading' })) + void client + .sendRequest('git.commitCompare', { worktree: `id:${worktreeId}`, commitId }) + .then((response) => { + const entries = response.ok + ? ((response as RpcSuccess).result as { entries: GitBranchChangeEntry[] }).entries + : [] + if (!stale) { + setFilesById((prev) => ({ ...prev, [commitId]: entries })) } - setFilesById((prev) => ({ ...prev, [row.id]: 'loading' })) - void client - .sendRequest('git.commitCompare', { worktree: `id:${worktreeId}`, commitId: row.id }) - .then((response) => { - const entries = response.ok - ? ((response as RpcSuccess).result as { entries: GitBranchChangeEntry[] }).entries - : [] - setFilesById((prev) => { - // Drop stale responses if the row is no longer loading (collapsed + re-opened). - if (prev[row.id] !== 'loading') { - return prev - } - return { ...prev, [row.id]: entries } - }) - }) - .catch(() => - setFilesById((prev) => { - if (prev[row.id] !== 'loading') { - return prev - } - return { ...prev, [row.id]: [] } - }) + }) + .catch(() => { + // Keep an already-loaded list; a first load that fails resolves to "No file changes". + if (!stale) { + setFilesById((prev) => + prev[commitId] === 'loading' ? { ...prev, [commitId]: [] } : prev ) - } - }, - [client, expanded, filesById, worktreeId] - ) + } + }) + return () => { + stale = true + } + }, [client, connState, expanded, worktreeId]) + + const connected = client !== null && connState === 'connected' const renderCommit = useCallback( ({ item }: { item: MobileCommitRow }) => { @@ -162,7 +157,12 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ {isOpen ? ( {files === 'loading' || files === undefined ? ( - + // No request can complete while disconnected, so say so instead of spinning forever. + connected ? ( + + ) : ( + Waiting for desktop... + ) ) : files.length === 0 ? ( No file changes ) : ( @@ -183,14 +183,10 @@ export const MobileGitHistoryList = memo(function MobileGitHistoryList({ ) }, - [expanded, filesById, toggleCommit] + [connected, expanded, filesById, toggleCommit] ) - const view = resolveMobileHistoryScreenView({ - connected: client !== null && connState === 'connected', - rows, - error - }) + const view = resolveMobileHistoryScreenView({ connected, rows, error }) if (view.kind === 'error' || view.kind === 'waiting') { return ( diff --git a/mobile/src/transport/client-context.test.ts b/mobile/src/transport/client-context.test.ts index eacb68ab0..e7c00008e 100644 --- a/mobile/src/transport/client-context.test.ts +++ b/mobile/src/transport/client-context.test.ts @@ -20,7 +20,7 @@ vi.mock('./connection-revival-triggers', () => ({ subscribeConnectionRevivalTriggers: () => () => {} })) -import { RpcClientProvider, useCloseHost, useHostClient } from './client-context' +import { RpcClientProvider, useCloseHost, useForceReconnect, useHostClient } from './client-context' type FakeClient = RpcClient & { emitState: (state: ConnectionState) => void @@ -172,7 +172,7 @@ describe('useHostClient', () => { } }) - it('stays disconnected while a reused screen resolves an uncached host', async () => { + it('shows connecting while a reused screen resolves an uncached host', async () => { const client = makeFakeClient('connected') connectMock.mockReturnValue(client) loadHostsMock.mockResolvedValueOnce([HOST]).mockReturnValueOnce(new Promise(() => {})) @@ -194,18 +194,20 @@ describe('useHostClient', () => { }) expect(stateByRenderTick.get(0)).toBe('connected') + // Why (S2): the unresolved-open window is amber, not grey — 'disconnected' + // here made every host swap flash a dead host while the Keychain read ran. selectedHostId = 'missing-host' renderTick = 1 await act(async () => { renderer?.update(createElement(RpcClientProvider, null, createElement(Probe))) }) - expect(stateByRenderTick.get(1)).toBe('disconnected') + expect(stateByRenderTick.get(1)).toBe('connecting') renderTick = 2 await act(async () => { renderer?.update(createElement(RpcClientProvider, null, createElement(Probe))) }) - expect(stateByRenderTick.get(2)).toBe('disconnected') + expect(stateByRenderTick.get(2)).toBe('connecting') } finally { restore() act(() => renderer?.unmount()) @@ -245,6 +247,73 @@ describe('useHostClient', () => { harness.unmount() }) + it('seeds connecting during the async open instead of flashing disconnected', async () => { + let resolveHosts: ((hosts: (typeof HOST)[]) => void) | null = null + const hostLookup = new Promise<(typeof HOST)[]>((resolve) => { + resolveHosts = resolve + }) + connectMock.mockReturnValue(makeFakeClient('connecting')) + loadHostsMock.mockReturnValue(hostLookup) + + const states: ConnectionState[] = [] + let renderer: ReactTestRenderer | null = null + function Probe(): null { + states.push(useHostClient(HOST.id).state) + return null + } + const restore = suppressReactTestRendererDeprecationWarning() + try { + act(() => { + renderer = create(createElement(RpcClientProvider, null, createElement(Probe))) + }) + expect(states.at(-1)).toBe('connecting') + + await act(async () => { + resolveHosts?.([HOST]) + await hostLookup + }) + expect(states.at(-1)).toBe('connecting') + expect(states).not.toContain('disconnected') + } finally { + restore() + act(() => renderer?.unmount()) + } + }) + + it('keeps Retry amber through forceReconnect instead of grey-then-amber', async () => { + const first = makeFakeClient('connected') + const second = makeFakeClient('connecting') + connectMock.mockReturnValueOnce(first).mockReturnValueOnce(second) + loadHostsMock.mockResolvedValue([HOST]) + + const states: ConnectionState[] = [] + let forceReconnect: ((hostId: string) => Promise) | null = null + let renderer: ReactTestRenderer | null = null + function Probe(): null { + forceReconnect = useForceReconnect() + states.push(useHostClient(HOST.id).state) + return null + } + const restore = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(RpcClientProvider, null, createElement(Probe))) + await Promise.resolve() + }) + expect(states.at(-1)).toBe('connected') + + await act(async () => { + await forceReconnect?.(HOST.id) + }) + expect(first.closeMock).toHaveBeenCalled() + expect(states.at(-1)).toBe('connecting') + expect(states).not.toContain('disconnected') + } finally { + restore() + act(() => renderer?.unmount()) + } + }) + it('does not open a client after the host is closed during an in-flight lookup', async () => { let resolveHosts: ((hosts: (typeof HOST)[]) => void) | null = null const hostLookup = new Promise<(typeof HOST)[]>((resolve) => { diff --git a/mobile/src/transport/client-context.tsx b/mobile/src/transport/client-context.tsx index eec5ba404..8c00a11b2 100644 --- a/mobile/src/transport/client-context.tsx +++ b/mobile/src/transport/client-context.tsx @@ -32,6 +32,8 @@ export type RpcClientContextValue = { forceReconnect: (hostId: string) => Promise closeHost: (hostId: string) => void getState: (hostId: string) => ConnectionState + // null = host has no client entry and no open in flight; callers pick the default. + getKnownState: (hostId: string) => ConnectionState | null getReconnectAttempt: (hostId: string) => number // Why: ms-epoch of the last 'connected' (null if never this session); UI escalates "Reconnecting…" into a re-pair prompt. getLastConnectedAt: (hostId: string) => number | null @@ -95,6 +97,8 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { resolve = res }) const pendingOpen = pendingOpensRef.current.register(hostId, promise) + // Why: already-mounted subscribers must go amber for the Keychain read too. + notifyHostState(hostId, 'connecting') try { // Why: prefer the primed cache so we don't serialize a second Keychain pass on cold start. @@ -209,6 +213,8 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { entry.client.close() storeRef.current.delete(hostId) } + // Why: Retry must read amber for the whole reopen, not grey-then-amber. + notifyHostState(hostId, 'connecting') const fresh = await openEntry(hostId) if (fresh) { fresh.refCount = savedRefCount @@ -217,10 +223,22 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { [openEntry] ) - const getState = useCallback((hostId: string): ConnectionState => { - return storeRef.current.get(hostId)?.state ?? 'disconnected' + // null = no entry and no open in flight; callers pick their own default. + const getKnownState = useCallback((hostId: string): ConnectionState | null => { + const entry = storeRef.current.get(hostId) + if (entry) { + return entry.state + } + // Why: the async open (a Keychain pass) predates the store entry; reading that + // window as 'disconnected' made every host screen flash dead on mount (S2). + return pendingOpensRef.current.getActivePromise(hostId) ? 'connecting' : null }, []) + const getState = useCallback( + (hostId: string): ConnectionState => getKnownState(hostId) ?? 'disconnected', + [getKnownState] + ) + const getReconnectAttempt = useCallback((hostId: string): number => { return storeRef.current.get(hostId)?.client.getReconnectAttempt() ?? 0 }, []) @@ -284,9 +302,9 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { // Why: nudge live clients when the OS signals the link may be back so sessions recover without a restart (issue #5049). useEffect(() => { - return subscribeConnectionRevivalTriggers(() => { + return subscribeConnectionRevivalTriggers((reason) => { for (const entry of storeRef.current.values()) { - entry.client.notifyForeground() + entry.client.notifyForeground(reason) } }) }, []) @@ -298,6 +316,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { forceReconnect, closeHost: closeEntry, getState, + getKnownState, getReconnectAttempt, getLastConnectedAt, getActivePath, @@ -312,6 +331,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { forceReconnect, closeEntry, getState, + getKnownState, getReconnectAttempt, getLastConnectedAt, getActivePath, @@ -340,8 +360,10 @@ export function useHostClient(hostId: string | undefined): { } { const ctx = useRpcClientContext() const [, force] = useState(0) + // Why: an absent entry at mount is almost always the open racing the render, not a + // dead host — seed amber; a failed open notifies 'disconnected' moments later. const [state, setState] = useState(() => - hostId ? ctx.getState(hostId) : 'disconnected' + hostId ? (ctx.getKnownState(hostId) ?? 'connecting') : 'disconnected' ) const clientRef = useRef(null) const clientHostIdRef = useRef(hostId) @@ -374,7 +396,7 @@ export function useHostClient(hostId: string | undefined): { }) const initial = ctx.acquire(hostId) clientRef.current = initial - setState(ctx.getState(hostId)) + setState(ctx.getKnownState(hostId) ?? 'connecting') if (initial) { // Why: two cached hosts can both be connected, so equal state values cannot reveal the replacement client. force((n) => n + 1) @@ -390,63 +412,14 @@ export function useHostClient(hostId: string | undefined): { // Why: Expo can reuse the screen before effects bind the next host; never expose the prior host's client or state in that render. const bound = clientHostIdRef.current === hostId - const boundState = bound ? state : hostId ? ctx.getState(hostId) : 'disconnected' + const boundState = bound + ? state + : hostId + ? (ctx.getKnownState(hostId) ?? 'connecting') + : 'disconnected' return { client: bound ? clientRef.current : null, state: boundState } } -// Why: refcounting prevents a double-open when a host-detail screen shares one of these hosts. -export function useAllHostClients(hostIds: string[]) { - const ctx = useRpcClientContext() - // Stable key so we don't tear down on every render of the array. - const key = useMemo(() => [...hostIds].sort().join(','), [hostIds]) - const [tick, setTick] = useState(0) - - useEffect(() => { - if (hostIds.length === 0) { - return - } - for (const id of hostIds) { - ctx.acquire(id) - } - const unsubs: Array<() => void> = [] - for (const id of hostIds) { - unsubs.push(ctx.subscribeHostState(id, () => setTick((n) => n + 1))) - } - unsubs.push(ctx.subscribeAllHosts(() => setTick((n) => n + 1))) - return () => { - for (const u of unsubs) { - u() - } - for (const id of hostIds) { - ctx.release(id) - } - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [key]) - - return useMemo(() => { - const out: Array<{ - hostId: string - client: RpcClient - state: ConnectionState - path: MobileConnectionPath - }> = [] - for (const id of hostIds) { - const all = ctx.getAllClients().find((entry) => entry.hostId === id) - if (all) { - out.push({ - hostId: id, - client: all.client, - state: ctx.getState(id), - path: ctx.getActivePath(id) - }) - } - } - return out - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [key, tick]) -} - // Why: host-store's removeHost() must close the live client but has no React-side handle; this hook bridges to it. export function useCloseHost(): (hostId: string) => void { const ctx = useRpcClientContext() @@ -467,5 +440,10 @@ export function usePrimeHosts(): (hosts: HostProfile[]) => void { function clientActivePath(client: RpcClient | undefined): MobileConnectionPath { const logical = client as Partial | undefined - return typeof logical?.getActivePath === 'function' ? logical.getActivePath() : 'lan' + if (typeof logical?.getActivePath !== 'function') { + return 'lan' + } + // Why: mid-migration the active path still names the session being replaced; the + // pending one is what the user is actually waiting on (F5). + return logical.getPendingPath?.() ?? logical.getActivePath() } diff --git a/mobile/src/transport/connection-revival-triggers.ts b/mobile/src/transport/connection-revival-triggers.ts index 1391607d4..a7e5bb857 100644 --- a/mobile/src/transport/connection-revival-triggers.ts +++ b/mobile/src/transport/connection-revival-triggers.ts @@ -6,10 +6,12 @@ import { addNetworkStateListener, getNetworkStateAsync, type NetworkState } from // without an onclose. Both leave clients waiting out long backoff timers or // parked at the reconnect give-up cap (issue #5049). Surface every "the link // probably just came back" OS signal as a single nudge callback. -export function subscribeConnectionRevivalTriggers(nudge: () => void): () => void { +export function subscribeConnectionRevivalTriggers( + nudge: (reason: 'app-resume' | 'network-change') => void +): () => void { const appStateSub = AppState.addEventListener('change', (next) => { if (next === 'active') { - nudge() + nudge('app-resume') } }) let lastNetwork: Pick | null = null @@ -39,7 +41,7 @@ export function subscribeConnectionRevivalTriggers(nudge: () => void): () => voi type: state.type, cameOnline }) - nudge() + nudge('network-change') } }) return () => { diff --git a/mobile/src/transport/host-edit-navigation.test.ts b/mobile/src/transport/host-edit-navigation.test.ts index 2e3e8a9f5..cd0de4612 100644 --- a/mobile/src/transport/host-edit-navigation.test.ts +++ b/mobile/src/transport/host-edit-navigation.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { mobileHostEditHostRoute, - mobileHostEditRoute, + mobileHostEditRouteTarget, navigateToMobileHostEdit, type MobileHostEditNavigationState } from './host-edit-navigation' @@ -15,6 +15,7 @@ function navigationHarness(initialState: MobileHostEditNavigationState) { stateListener = listener return unsubscribeState }), + dispatch: vi.fn(), getState: () => state } return { @@ -27,51 +28,83 @@ function navigationHarness(initialState: MobileHostEditNavigationState) { } } +// Edit now waits for the nested host stack, not just the root `h` route, so every committed +// state below carries the stack the replacement targets. +function committedHostState(hostIdParam: string): MobileHostEditNavigationState { + return { + index: 1, + routes: [ + { name: 'index' }, + { + name: 'h', + state: { + key: '/h', + index: 0, + routes: [{ key: 'host-index', name: '[hostId]/index', params: { hostId: hostIdParam } }] + } + } + ] + } +} + describe('mobile host edit navigation', () => { - it('waits for the expected host route to commit before replacing it with Edit', () => { + it('waits for the expected host stack to commit before replacing it with Edit', () => { const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) const push = vi.fn() - const replace = vi.fn() - navigateToMobileHostEdit(harness.navigation, { push, replace }, 'host/1') + navigateToMobileHostEdit(harness.navigation, { push }, 'host/1') expect(push).toHaveBeenCalledWith(mobileHostEditHostRoute('host/1')) - expect(replace).not.toHaveBeenCalled() + expect(harness.navigation.dispatch).not.toHaveBeenCalled() - harness.setState({ - index: 1, - routes: [{ name: 'index' }, { name: 'h', params: { hostId: 'host/1' } }] - }) + // The host route commits before its stack mounts; the old root-only predicate fired here. + harness.setState({ index: 1, routes: [{ name: 'index' }, { name: 'h' }] }) + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + + harness.setState(committedHostState('host%2F1')) expect(harness.unsubscribeState).toHaveBeenCalledOnce() - expect(replace).toHaveBeenCalledWith(mobileHostEditRoute('host/1')) + expect(harness.navigation.dispatch).toHaveBeenCalledWith({ + type: 'REPLACE', + target: '/h', + source: 'host-index', + payload: mobileHostEditRouteTarget('host/1') + }) }) it('does not replace an unrelated host route', () => { const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) - const replace = vi.fn() - navigateToMobileHostEdit(harness.navigation, { push: vi.fn(), replace }, 'host-1') - harness.setState({ index: 0, routes: [{ name: 'h', params: { hostId: 'host-2' } }] }) + navigateToMobileHostEdit(harness.navigation, { push: vi.fn() }, 'host-1') + harness.setState(committedHostState('host-2')) - expect(replace).not.toHaveBeenCalled() + expect(harness.navigation.dispatch).not.toHaveBeenCalled() + }) + + it('disposes itself when navigation leaves the host flow without cancel()', () => { + const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) + navigateToMobileHostEdit(harness.navigation, { push: vi.fn() }, 'host-1') + + // Enter the host flow for a different host, then leave it entirely. + harness.setState(committedHostState('host-2')) + harness.setState({ index: 0, routes: [{ name: 'index' }] }) + expect(harness.unsubscribeState).toHaveBeenCalledOnce() + + // A late matching commit must not resurrect the replacement. + harness.setState(committedHostState('host-1')) + expect(harness.navigation.dispatch).not.toHaveBeenCalled() }) it('cancels a pending replacement when navigation leaves the host flow', () => { const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) - const replace = vi.fn() - const controller = navigateToMobileHostEdit( - harness.navigation, - { push: vi.fn(), replace }, - 'host-1' - ) + const controller = navigateToMobileHostEdit(harness.navigation, { push: vi.fn() }, 'host-1') - harness.setState({ index: 0, routes: [{ name: 'h', params: { hostId: 'host-2' } }] }) + harness.setState(committedHostState('host-2')) controller.cancel() - harness.setState({ index: 0, routes: [{ name: 'h', params: { hostId: 'host-1' } }] }) + harness.setState(committedHostState('host-1')) expect(harness.unsubscribeState).toHaveBeenCalledOnce() - expect(replace).not.toHaveBeenCalled() + expect(harness.navigation.dispatch).not.toHaveBeenCalled() }) it('unsubscribes when mounting the host throws synchronously', () => { @@ -84,8 +117,7 @@ describe('mobile host edit navigation', () => { { push: () => { throw error - }, - replace: vi.fn() + } }, 'host-1' ) diff --git a/mobile/src/transport/host-edit-navigation.ts b/mobile/src/transport/host-edit-navigation.ts index 765e36d7b..c1e667c5c 100644 --- a/mobile/src/transport/host-edit-navigation.ts +++ b/mobile/src/transport/host-edit-navigation.ts @@ -1,34 +1,27 @@ -export type MobileHostEditNavigationState = Readonly<{ - index: number - routes: readonly MobileHostEditNavigationRoute[] -}> +import { + hostStackHostRoute, + navigateToHostStackRoute, + type HostStackHostRoute, + type HostStackNavigationController, + type HostStackNavigationState, + type HostStackRootNavigation, + type HostStackRouteTarget, + type HostStackRouter +} from '../navigation/host-stack-navigation' -export type MobileHostEditNavigationRoute = Readonly<{ - name: string - params?: Readonly<{ hostId?: unknown }> -}> +export type MobileHostEditHostRoute = HostStackHostRoute +export type MobileHostEditNavigationState = HostStackNavigationState +export type MobileHostEditRootNavigation = HostStackRootNavigation +export type MobileHostEditRouter = HostStackRouter +export type MobileHostEditNavigationController = HostStackNavigationController -export type MobileHostEditRootNavigation = { - addListener: (event: 'state', listener: () => void) => () => void - getState: () => MobileHostEditNavigationState +export function mobileHostEditHostRoute(hostId: string): MobileHostEditHostRoute { + return hostStackHostRoute(hostId) } -export type MobileHostEditRouter = { - push: (href: `/h/${string}`) => void - replace: (href: ReturnType) => void -} - -export type MobileHostEditNavigationController = Readonly<{ - cancel: () => void -}> - -export function mobileHostEditHostRoute(hostId: string): `/h/${string}` { - return `/h/${encodeURIComponent(hostId)}` -} - -export function mobileHostEditRoute(hostId: string) { +export function mobileHostEditRouteTarget(hostId: string): HostStackRouteTarget { return { - pathname: '/h/[hostId]/edit' as const, + name: '[hostId]/edit', params: { hostId } } } @@ -38,44 +31,5 @@ export function navigateToMobileHostEdit( router: MobileHostEditRouter, hostId: string ): MobileHostEditNavigationController { - let active = true - let hostRouteSeen = false - let unsubscribeState = () => {} - const dispose = () => { - if (!active) { - return - } - active = false - unsubscribeState() - } - - // Why: cold Expo deep links resolve to index; target Edit after the host route commits. - const onState = () => { - if (!active) { - return - } - const state = navigation.getState() - const currentRoute = state.routes[state.index] - if (currentRoute?.name !== 'h') { - if (hostRouteSeen) { - dispose() - } - return - } - hostRouteSeen = true - if (currentRoute.params?.hostId !== hostId) { - return - } - dispose() - router.replace(mobileHostEditRoute(hostId)) - } - - try { - unsubscribeState = navigation.addListener('state', onState) - router.push(mobileHostEditHostRoute(hostId)) - } catch (error) { - dispose() - throw error - } - return { cancel: dispose } + return navigateToHostStackRoute(navigation, router, hostId, mobileHostEditRouteTarget(hostId)) } diff --git a/mobile/src/transport/host-logical-client.ts b/mobile/src/transport/host-logical-client.ts index 7d90b32c8..7682fc959 100644 --- a/mobile/src/transport/host-logical-client.ts +++ b/mobile/src/transport/host-logical-client.ts @@ -28,9 +28,11 @@ export function openHostLogicalClient(host: HostProfile, onLog: ConnectionLogSin closeLogical() } const notifyLogicalForeground = logical.notifyForeground - logical.notifyForeground = () => { - endpointLifecycle.setForeground(true) - notifyLogicalForeground() + logical.notifyForeground = (reason = 'focus') => { + // Why: a nudge while already foreground must not re-enter setForeground — + // that path suspended healthy relays; the supervisor probes or replaces instead. + endpointLifecycle.nudge(reason) + notifyLogicalForeground(reason) } return logical } diff --git a/mobile/src/transport/host-status-gates.test.ts b/mobile/src/transport/host-status-gates.test.ts index ce5974b1f..483cfcca5 100644 --- a/mobile/src/transport/host-status-gates.test.ts +++ b/mobile/src/transport/host-status-gates.test.ts @@ -120,7 +120,7 @@ describe('useHostStatusGates', () => { } }) - it('fails closed while the same client reconnects', async () => { + it('keeps the proven gates while the same client reconnects, pending until it re-answers', async () => { let resolveReconnect: ((response: unknown) => void) | null = null const pendingReconnect = new Promise((resolve) => { resolveReconnect = resolve @@ -152,12 +152,20 @@ describe('useHostStatusGates', () => { await act(async () => { renderer?.update(createElement(Probe, { connState: 'disconnected' })) }) + // Why (F10): the drop invalidates nothing the host already proved — capabilities survive it. + expect(gates).toMatchObject({ + hostCapabilities: ['browser.screencast.v1'], + floatingWorkspaceEnabled: true, + statusPending: false + }) + await act(async () => { renderer?.update(createElement(Probe, { connState: 'connected' })) }) expect(gates).toMatchObject({ - hostCapabilities: [], - floatingWorkspaceEnabled: false + hostCapabilities: ['browser.screencast.v1'], + floatingWorkspaceEnabled: true, + statusPending: true }) await act(async () => { @@ -169,11 +177,48 @@ describe('useHostStatusGates', () => { }) expect(gates).toMatchObject({ hostCapabilities: ['terminal.quick-commands.v1'], - floatingWorkspaceEnabled: true + floatingWorkspaceEnabled: true, + statusPending: false }) } finally { restore() renderer?.unmount() } }) + + it('fails closed when the same host reconnects on a replaced client', async () => { + const firstClient = { + sendRequest: vi.fn().mockResolvedValue({ + ok: true, + result: { capabilities: ['browser.screencast.v1'], floatingWorkspaceEnabled: true } + }) + } as unknown as RpcClient + const secondClient = { + sendRequest: vi.fn().mockReturnValue(new Promise(() => {})) + } as unknown as RpcClient + let gates: HostStatusGates | null = null + let renderer: ReactTestRenderer | null = null + + function Probe({ client }: { client: RpcClient }): null { + gates = useHostStatusGates({ hostId: 'host-1', client, connState: 'connected' }) + return null + } + + const restore = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(Probe, { client: firstClient })) + await Promise.resolve() + }) + expect(gates?.hostCapabilities).toEqual(['browser.screencast.v1']) + + await act(async () => { + renderer?.update(createElement(Probe, { client: secondClient })) + }) + expect(gates).toMatchObject({ hostCapabilities: [], statusPending: true }) + } finally { + restore() + renderer?.unmount() + } + }) }) diff --git a/mobile/src/transport/host-status-gates.ts b/mobile/src/transport/host-status-gates.ts index 8a26f5cb6..f9837a8ac 100644 --- a/mobile/src/transport/host-status-gates.ts +++ b/mobile/src/transport/host-status-gates.ts @@ -11,7 +11,8 @@ export type HostStatusGates = { statusPending: boolean } -type LoadedHostStatusGates = HostStatusGates & { +// statusPending is not stored: pending-ness belongs to the live connection, not to the answer. +type LoadedHostStatusGates = Omit & { hostId: string | undefined client: RpcClient } @@ -27,15 +28,21 @@ export function useHostStatusGates(args: { }): HostStatusGates { const { hostId, client, connState } = args const [loaded, setLoaded] = useState(null) + // Why (F10): a drop must not erase proven capabilities, but it does invalidate them — this keeps + // statusPending true across the reconnect refetch, so gates stay "unknown" while the data survives. + const [unverified, setUnverified] = useState(false) useEffect(() => { if (connState !== 'connected' || !client) { - // Why: reconnecting the same host/client must revalidate gates instead of reviving its prior status response. - setLoaded(null) + setUnverified(true) return } let cancelled = false const requestClient = client + const settle = (gates: Omit) => { + setLoaded({ hostId, client: requestClient, ...gates }) + setUnverified(false) + } void (async () => { try { const response = await requestClient.sendRequest('status.get') @@ -43,13 +50,10 @@ export function useHostStatusGates(args: { return } if (!response.ok) { - setLoaded({ - hostId, - client: requestClient, + settle({ hostCapabilities: [], floatingWorkspaceEnabled: false, - compatVerdict: { kind: 'ok' }, - statusPending: false + compatVerdict: { kind: 'ok' } }) return } @@ -60,13 +64,10 @@ export function useHostStatusGates(args: { desktopProtocolVersion: status.protocolVersion, desktopMinCompatibleMobileVersion: status.minCompatibleMobileVersion }) - setLoaded({ - hostId, - client: requestClient, + settle({ hostCapabilities: status.capabilities ?? [], floatingWorkspaceEnabled: status.floatingWorkspaceEnabled === true, - compatVerdict: verdict, - statusPending: false + compatVerdict: verdict }) if (verdict.kind === 'blocked') { // Why: support breadcrumb to confirm a block fired vs a render bug; no PII, just version ints. @@ -80,13 +81,10 @@ export function useHostStatusGates(args: { } catch { // Why: a transient status failure must not trap navigation; conservative feature gates remain disabled. if (!cancelled) { - setLoaded({ - hostId, - client: requestClient, + settle({ hostCapabilities: [], floatingWorkspaceEnabled: false, - compatVerdict: { kind: 'ok' }, - statusPending: false + compatVerdict: { kind: 'ok' } }) } } @@ -97,13 +95,8 @@ export function useHostStatusGates(args: { }, [client, connState, hostId]) // Why: effects run after render, so key loaded gates by host and client to fail closed during route reuse. - if ( - connState !== 'connected' || - !client || - !loaded || - loaded.hostId !== hostId || - loaded.client !== client - ) { + const proven = loaded && loaded.hostId === hostId && loaded.client === client ? loaded : null + if (!proven) { return { hostCapabilities: EMPTY_HOST_CAPABILITIES, floatingWorkspaceEnabled: false, @@ -112,9 +105,11 @@ export function useHostStatusGates(args: { } } return { - hostCapabilities: loaded.hostCapabilities, - floatingWorkspaceEnabled: loaded.floatingWorkspaceEnabled, - compatVerdict: loaded.compatVerdict, - statusPending: false + hostCapabilities: proven.hostCapabilities, + floatingWorkspaceEnabled: proven.floatingWorkspaceEnabled, + compatVerdict: proven.compatVerdict, + // Why (F10): unchanged pending timing — the reconnect refetch is still "unknown", it just no + // longer blanks the capabilities this same host already proved. + statusPending: connState === 'connected' && unverified } } diff --git a/mobile/src/transport/migration-dial-state-forwarder.ts b/mobile/src/transport/migration-dial-state-forwarder.ts new file mode 100644 index 000000000..de97c9a56 --- /dev/null +++ b/mobile/src/transport/migration-dial-state-forwarder.ts @@ -0,0 +1,53 @@ +import type { RpcClient } from './rpc-client' +import type { ConnectionState } from './types' + +// Why: a replacement session publishes these to nobody until migrateTo binds it, so a +// suspended client shows grey for the whole dial (S2). 'connected' stays excluded — the +// migration tail owns it, and forwarding it early would advertise a session that still +// rejects requests. +const DIAL_PHASES: ConnectionState[] = ['connecting', 'handshaking', 'reconnecting'] + +export type MigrationDialStateForwarder = { + forwarded: () => boolean + stop: () => void +} + +// Why: start only from an idle logical client and abandon the moment anything else +// publishes 'connected' — make-before-break replacement dials (lease rotation, the +// supervisor's suspectActive recovery) must never blink the dot on their way through. +// 'reconnecting' is deliberately not a start state: the previous session's retry loop +// is still bound and cycling, so two publishers would fight over a dot that is already +// amber. That case is served by the pending path label instead, not by forwarding. +function canForward(started: boolean, state: ConnectionState, suspended: boolean): boolean { + if (suspended) { + return true + } + return started ? state !== 'connected' : state === 'disconnected' +} + +export function forwardMigrationDialState(args: { + session: RpcClient + snapshot: () => { state: ConnectionState; suspended: boolean } + publish: (state: ConnectionState) => void +}): MigrationDialStateForwarder { + let started = false + let stopped = false + const unsubscribe = args.session.onStateChange((next) => { + if (stopped || !DIAL_PHASES.includes(next)) { + return + } + const { state, suspended } = args.snapshot() + if (!canForward(started, state, suspended)) { + return + } + started = true + args.publish(next) + }) + return { + forwarded: () => started, + stop: () => { + stopped = true + unsubscribe() + } + } +} diff --git a/mobile/src/transport/mobile-endpoint-lifecycle.ts b/mobile/src/transport/mobile-endpoint-lifecycle.ts index d41898152..76ba92547 100644 --- a/mobile/src/transport/mobile-endpoint-lifecycle.ts +++ b/mobile/src/transport/mobile-endpoint-lifecycle.ts @@ -1,5 +1,5 @@ import * as ExpoCrypto from 'expo-crypto' -import type { ConnectionLogSink, HostProfile } from './types' +import type { ConnectionLogSink, ForegroundNudgeReason, HostProfile } from './types' import { connect } from './rpc-client' import { MobileEndpointSupervisor } from './mobile-endpoint-supervisor' import { connectMobileRelayRpcSession } from './mobile-relay-rpc-session' @@ -15,6 +15,7 @@ import type { StableLogicalRpcClient } from './stable-logical-rpc-client' type EndpointLifecycle = { setForeground(foreground: boolean): void + nudge(reason: ForegroundNudgeReason): void stop(): void } @@ -63,6 +64,14 @@ export function startMobileEndpointLifecycle( foreground = next owner.setForeground(next) }, + nudge(reason) { + // Why: a focus nudge can precede the AppState listener; keep the closure in + // sync or a later supervisor swap would start with a stale background flag. + if (reason !== 'network-change') { + foreground = true + } + owner.nudge(reason) + }, stop() { stopped = true owner.stop() diff --git a/mobile/src/transport/mobile-endpoint-nudge-router.test.ts b/mobile/src/transport/mobile-endpoint-nudge-router.test.ts new file mode 100644 index 000000000..a8014a902 --- /dev/null +++ b/mobile/src/transport/mobile-endpoint-nudge-router.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it, vi } from 'vitest' +import { MobileEndpointNudgeRouter } from './mobile-endpoint-nudge-router' +import type { RelayReconnectController } from './mobile-relay-reconnect-controller' +import type { StableLogicalRpcClient } from './stable-logical-rpc-client' + +function routerFixture() { + let foreground = false + const logical = { + getActivePath: vi.fn(() => 'relay'), + getState: vi.fn(() => 'connected'), + getGeneration: vi.fn(() => 1), + sendRequest: vi.fn(async () => ({})) + } as unknown as StableLogicalRpcClient + const handleActiveNudge = vi.fn(() => 'probe' as const) + const setForeground = vi.fn((next: boolean) => { + foreground = next + }) + const scheduleDirectProbe = vi.fn() + const router = new MobileEndpointNudgeRouter({ + logical, + controller: { handleActiveNudge } as unknown as RelayReconnectController, + now: () => 20_000, + isStopped: () => false, + isForeground: () => foreground, + setForeground, + replaceRelay: vi.fn(), + recoverAfterDeadProbe: vi.fn(), + scheduleDirectProbe + }) + return { handleActiveNudge, logical, router, scheduleDirectProbe, setForeground } +} + +describe('MobileEndpointNudgeRouter', () => { + it('processes a focus nudge after restoring lagging foreground state', () => { + const fixture = routerFixture() + + fixture.router.nudge('focus') + + expect(fixture.setForeground).toHaveBeenCalledWith(true) + expect(fixture.handleActiveNudge).toHaveBeenCalledWith(fixture.logical, 'focus') + expect(fixture.logical.sendRequest).toHaveBeenCalledWith('status.get', null, { + timeoutMs: 4000 + }) + expect(fixture.scheduleDirectProbe).toHaveBeenCalledOnce() + }) + + it('ignores a network nudge while backgrounded', () => { + const fixture = routerFixture() + + fixture.router.nudge('network-change') + + expect(fixture.setForeground).not.toHaveBeenCalled() + expect(fixture.handleActiveNudge).not.toHaveBeenCalled() + expect(fixture.scheduleDirectProbe).not.toHaveBeenCalled() + }) +}) diff --git a/mobile/src/transport/mobile-endpoint-nudge-router.ts b/mobile/src/transport/mobile-endpoint-nudge-router.ts new file mode 100644 index 000000000..d59c99b7c --- /dev/null +++ b/mobile/src/transport/mobile-endpoint-nudge-router.ts @@ -0,0 +1,53 @@ +import { MobileRelayFocusProbe } from './mobile-relay-focus-probe' +import type { RelayReconnectController } from './mobile-relay-reconnect-controller' +import type { StableLogicalRpcClient } from './stable-logical-rpc-client' +import type { ForegroundNudgeReason } from './types' + +// Routes attention/network nudges: focus and app-resume probe a healthy relay, +// a network change replaces it make-before-break, everything else re-enters recovery. +export class MobileEndpointNudgeRouter { + private readonly focusProbe: MobileRelayFocusProbe + + constructor( + private readonly args: { + logical: StableLogicalRpcClient + controller: RelayReconnectController + now: () => number + isStopped: () => boolean + isForeground: () => boolean + setForeground: (foreground: boolean) => void + replaceRelay: () => void + recoverAfterDeadProbe: (detail: string) => void + scheduleDirectProbe: () => void + } + ) { + this.focusProbe = new MobileRelayFocusProbe({ + logical: args.logical, + now: args.now, + canProbe: () => !args.isStopped() && args.isForeground(), + onDead: args.recoverAfterDeadProbe + }) + } + + nudge(reason: ForegroundNudgeReason): void { + const { args } = this + if (args.isStopped()) { + return + } + if (!args.isForeground()) { + // Why: a background network flap must not re-open a billed relay splice; + // focus/app-resume imply the app is visible even if AppState lags. + if (reason === 'network-change') { + return + } + args.setForeground(true) + } + const verdict = args.controller.handleActiveNudge(args.logical, reason) + if (verdict === 'probe') { + void this.focusProbe.probe() + } else if (verdict === 'replace') { + args.replaceRelay() + } + args.scheduleDirectProbe() + } +} diff --git a/mobile/src/transport/mobile-endpoint-supervisor-nudge.test.ts b/mobile/src/transport/mobile-endpoint-supervisor-nudge.test.ts new file mode 100644 index 000000000..b839732cf --- /dev/null +++ b/mobile/src/transport/mobile-endpoint-supervisor-nudge.test.ts @@ -0,0 +1,216 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { RelayOuterError } from './mobile-relay-e2ee-link' +import { + bundle, + dependencies, + FakeLogicalClient, + FakeRelaySession, + FakeSession, + host +} from './mobile-endpoint-supervisor-test-fakes' +import { MobileEndpointSupervisor } from './mobile-endpoint-supervisor' +import { MobileRelaySessionEstablisher } from './mobile-relay-session-establisher' +import type { RelayReconnectController } from './mobile-relay-reconnect-controller' + +vi.mock('react-native', () => ({ Platform: { OS: 'ios' } })) +vi.mock('expo-secure-store', () => ({ WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'when-unlocked' })) +vi.mock('expo-crypto', () => ({ getRandomBytes: (length: number) => new Uint8Array(length) })) + +// Focus/network nudge routing, make-before-break replacement, and the +// happy-eyeballs race — split from the main supervisor suite (max-lines). +describe('mobile endpoint supervisor nudges', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-07-13T12:00:00Z')) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('replaces a relay make-before-break on a network nudge without going grey', async () => { + const logical = new FakeLogicalClient('disconnected', 'lan') + const deps = dependencies({ + openDirect: vi.fn(() => new FakeSession('disconnected')) + }) + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + expect(deps.openRelay).toHaveBeenCalledOnce() + expect(logical.getActivePath()).toBe('relay') + + // The OS reports a network handoff, but the relay never published onclose. + // The replacement authenticates, migrateTo swaps sessions — never disconnected. + supervisor.nudge('network-change') + await vi.advanceTimersByTimeAsync(0) + expect(deps.openRelay).toHaveBeenCalledTimes(2) + expect(logical.suspendActiveSession).not.toHaveBeenCalled() + expect(logical.getState()).toBe('connected') + supervisor.stop() + }) + + it('suspends only after a failed replacement dial, then backs off further nudges', async () => { + const logical = new FakeLogicalClient('disconnected', 'lan') + const openRelay = vi + .fn() + .mockReturnValueOnce(new FakeRelaySession('connected')) + .mockImplementation(() => new FakeRelaySession('disconnected', new RelayOuterError(4408))) + const deps = dependencies({ + openRelay, + // Keep direct unavailable so relay recovery stays the only path under test. + openDirect: vi.fn(() => new FakeSession('disconnected')), + // Deterministic full jitter: fraction 0.5 → half the backoff window. + randomBytes: () => new Uint8Array([128, 0]) + }) + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + expect(openRelay).toHaveBeenCalledOnce() + + // PEER_DROPPED on the replacement: the suspect session comes down so the + // armed retry can run, and the failure books the shared cooldown. + supervisor.nudge('network-change') + await vi.advanceTimersByTimeAsync(0) + expect(logical.suspendActiveSession).toHaveBeenCalledOnce() + expect(openRelay).toHaveBeenCalledTimes(2) + + // More flap nudges share the existing cooldown rather than opening sockets. + for (let i = 0; i < 5; i++) { + supervisor.nudge('network-change') + await vi.advanceTimersByTimeAsync(0) + } + expect(openRelay).toHaveBeenCalledTimes(2) + + // Exactly one retry fires at the 250 ms deterministic backoff boundary. + await vi.advanceTimersByTimeAsync(249) + expect(openRelay).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(1) + expect(openRelay).toHaveBeenCalledTimes(3) + supervisor.stop() + }) + + it('probes a healthy relay on a focus nudge and keeps it untouched', async () => { + const logical = new FakeLogicalClient('connected', 'relay') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + await supervisor.start() + + supervisor.nudge('focus') + await vi.advanceTimersByTimeAsync(0) + expect(logical.sendRequest).toHaveBeenCalledWith('status.get', null, { timeoutMs: 4000 }) + expect(logical.suspendActiveSession).not.toHaveBeenCalled() + expect(deps.openRelay).not.toHaveBeenCalled() + + // Repeated focus events inside the probe window coalesce into one probe. + supervisor.nudge('focus') + await vi.advanceTimersByTimeAsync(0) + expect(logical.sendRequest).toHaveBeenCalledOnce() + supervisor.stop() + }) + + it('suspends and re-dials when the focus probe fails', async () => { + const logical = new FakeLogicalClient('connected', 'relay') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + await supervisor.start() + + logical.sendRequest.mockRejectedValueOnce(new Error('relay RPC timed out: status.get')) + supervisor.nudge('focus') + await vi.advanceTimersByTimeAsync(0) + expect(logical.suspendActiveSession).toHaveBeenCalledOnce() + await vi.waitFor(() => expect(deps.openRelay).toHaveBeenCalledOnce()) + expect(logical.getState()).toBe('connected') + supervisor.stop() + }) + + it('queues a network replacement that lands while another dial owns the mutex', async () => { + const logical = new FakeLogicalClient('disconnected', 'lan') + let resolveWrite: (() => void) | null = null + const deps = dependencies({ + openDirect: vi.fn(() => new FakeSession('disconnected')), + writeBundle: vi + .fn() + .mockResolvedValueOnce(undefined) + .mockImplementation( + () => + new Promise((resolve) => { + resolveWrite = resolve + }) + ) + }) + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + await supervisor.start() + expect(deps.openRelay).toHaveBeenCalledOnce() + + // First handoff nudge dials a replacement whose bookkeeping write is slow, + // holding the recovery mutex; the second nudge arriving then must neither + // suspend the session nor be dropped. + supervisor.nudge('network-change') + await vi.advanceTimersByTimeAsync(0) + expect(deps.openRelay).toHaveBeenCalledTimes(2) + supervisor.nudge('network-change') + expect(logical.suspendActiveSession).not.toHaveBeenCalled() + + resolveWrite?.() + await vi.advanceTimersByTimeAsync(0) + // The in-flight replacement satisfies the queued intent — no third socket. + expect(deps.openRelay).toHaveBeenCalledTimes(2) + expect(logical.getState()).toBe('connected') + supervisor.stop() + }) + + it('keeps a healthy relay bound when a nudge finds no dialable credential', async () => { + const logical = new FakeLogicalClient('connected', 'relay') + const expired = { + ...bundle, + current: { ...bundle.current, expiresAt: 1 } + } + const deps = dependencies({ + readBundle: vi.fn(async () => expired), + randomBytes: () => new Uint8Array([128, 0]) + }) + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + await supervisor.start() + + supervisor.nudge('network-change') + await vi.advanceTimersByTimeAsync(0) + // Why: no dial happened, so nothing has disproven the live session. + expect(logical.suspendActiveSession).not.toHaveBeenCalled() + expect(deps.openRelay).not.toHaveBeenCalled() + expect(logical.getState()).toBe('connected') + supervisor.stop() + }) + + it('withdraws the racing relay dial when direct authenticated while it was in flight', async () => { + // Direct won between the grace timer firing and the relay session + // authenticating; the migration must withdraw instead of closing the winner. + const logical = new FakeLogicalClient('connected', 'lan') + const relaySession = new FakeRelaySession('connected') + const setActiveSession = vi.fn() + const establisher = new MobileRelaySessionEstablisher({ + logical, + controller: { setActiveSession } as unknown as RelayReconnectController, + openRelay: vi.fn(() => relaySession), + randomBytes: (length) => new Uint8Array(length), + writeBundle: vi.fn(async () => {}), + isActive: () => true, + isForeground: () => true, + relay: () => host.relay, + resolveRelay: vi.fn(async ({ relay: endpoint }) => endpoint), + persistResolvedRelay: vi.fn(async () => {}), + bundle: () => bundle, + adoptBundle: vi.fn(), + recordMigration: vi.fn(), + scheduleLease: vi.fn(), + scheduleDirectProbe: vi.fn(), + onBookkeepingError: vi.fn(), + onDialFailure: vi.fn() + }) + + const outcome = await establisher.dialEligible([bundle.current]) + expect(outcome).toEqual({ outcome: 'aborted' }) + expect(setActiveSession).not.toHaveBeenCalled() + expect(relaySession.close).toHaveBeenCalled() + expect(logical.getActivePath()).toBe('lan') + }) +}) diff --git a/mobile/src/transport/mobile-endpoint-supervisor-support.ts b/mobile/src/transport/mobile-endpoint-supervisor-support.ts index 28318ffd4..002ac15fe 100644 --- a/mobile/src/transport/mobile-endpoint-supervisor-support.ts +++ b/mobile/src/transport/mobile-endpoint-supervisor-support.ts @@ -1,8 +1,62 @@ import { RelayOuterError } from './mobile-relay-e2ee-link' import { MobileE2EEAuthenticationError } from './mobile-e2ee-v2-physical-channel' +import type { RelayReconnectController } from './mobile-relay-reconnect-controller' +import type { StableLogicalRpcClient } from './stable-logical-rpc-client' import type { HostProfile } from './types' import type { MobileRelayEndpoint } from '../../../src/shared/mobile-relay-credential-contract' +// Why: a suspect session that survived a failed replacement dial must come down, +// else the armed unforced retry dead-ends on needsRecovery seeing stale 'connected'. +export function suspendRelayIfStillConnected( + controller: RelayReconnectController, + logical: StableLogicalRpcClient +): void { + if (logical.getState() === 'connected') { + controller.suspendActiveRelay(logical) + } +} + +type RelayDialResult = { ok: true } | { ok: false; error: Error } + +// Why: a locally-aborted dial (background/stop/missing state) proves nothing about the +// cell assignment, so the director fallback must not burn a resolution round on it. +export class RelayDialAbortedError extends Error { + constructor() { + super('relay dial aborted before opening a session') + } +} + +// One credential attempt: dial, and on a director-class failure re-resolve the +// cell assignment, persist it durably, then dial once more. +export async function dialRelayThroughDirectorFallback(args: { + resumeToken: string + relay: () => HostProfile['relay'] + dial: () => Promise + resolveRelay: (input: { + relay: NonNullable + resumeToken: string + }) => Promise + persistResolvedRelay: (resolved: MobileRelayEndpoint) => Promise +}): Promise { + const first = await args.dial() + const relay = args.relay() + if ( + first.ok || + first.error instanceof RelayDialAbortedError || + !isDirectorResolutionFailure(first.error) || + !relay + ) { + return first + } + try { + const resolved = await args.resolveRelay({ relay, resumeToken: args.resumeToken }) + await args.persistResolvedRelay(resolved) + return await args.dial() + } catch (error) { + return { ok: false, error: toError(error) } + } +} + export function isDirectorResolutionFailure(error: Error): boolean { return ( !(error instanceof MobileE2EEAuthenticationError) && diff --git a/mobile/src/transport/mobile-endpoint-supervisor-test-fakes.ts b/mobile/src/transport/mobile-endpoint-supervisor-test-fakes.ts index d483c6536..2ef13c0b5 100644 --- a/mobile/src/transport/mobile-endpoint-supervisor-test-fakes.ts +++ b/mobile/src/transport/mobile-endpoint-supervisor-test-fakes.ts @@ -72,17 +72,31 @@ export class FakeLogicalClient extends FakeSession implements StableLogicalRpcCl this.path = path } - migrateTo = vi.fn(async (session: RpcClient, path: MobileConnectionPath) => { - if (session.getState() !== 'connected') { - session.close() - throw new Error(`replacement session ${session.getState()}`) + migrateTo = vi.fn( + async ( + session: RpcClient, + path: MobileConnectionPath, + _timeoutMs?: number, + shouldAbort?: () => boolean + ) => { + if (session.getState() !== 'connected') { + session.close() + throw new Error(`replacement session ${session.getState()}`) + } + // Mirrors the real client: a racing caller withdraws after auth, before the swap. + if (shouldAbort?.()) { + session.close() + throw new Error('migration superseded') + } + this.path = path + this.generation += 1 + this.publishState('connected') } - this.path = path - this.generation += 1 - this.publishState('connected') - }) + ) suspendActiveSession = vi.fn(() => this.publishState('disconnected')) getActivePath = () => this.path + // This fake migrates instantly, so no dial is ever in flight to name. + getPendingPath = () => null getGeneration = () => this.generation } diff --git a/mobile/src/transport/mobile-endpoint-supervisor.test.ts b/mobile/src/transport/mobile-endpoint-supervisor.test.ts index d1b8b6918..bddc09795 100644 --- a/mobile/src/transport/mobile-endpoint-supervisor.test.ts +++ b/mobile/src/transport/mobile-endpoint-supervisor.test.ts @@ -35,7 +35,12 @@ describe('mobile endpoint supervisor', () => { await supervisor.start() - expect(logical.migrateTo).toHaveBeenCalledWith(expect.any(FakeRelaySession), 'relay') + expect(logical.migrateTo).toHaveBeenCalledWith( + expect.any(FakeRelaySession), + 'relay', + undefined, + expect.any(Function) + ) expect(logical.getActivePath()).toBe('relay') expect(deps.writeBundle).toHaveBeenCalledWith( expect.objectContaining({ current: expect.objectContaining({ version: 2 }) }) @@ -60,7 +65,12 @@ describe('mobile endpoint supervisor', () => { logical.publishState('reconnecting') await vi.waitFor(() => expect(logical.getActivePath()).toBe('relay')) - expect(logical.migrateTo).toHaveBeenCalledWith(expect.any(FakeRelaySession), 'relay') + expect(logical.migrateTo).toHaveBeenCalledWith( + expect.any(FakeRelaySession), + 'relay', + undefined, + expect.any(Function) + ) supervisor.stop() }) @@ -71,7 +81,12 @@ describe('mobile endpoint supervisor', () => { await supervisor.start() - expect(logical.migrateTo).toHaveBeenCalledWith(expect.any(FakeRelaySession), 'relay') + expect(logical.migrateTo).toHaveBeenCalledWith( + expect.any(FakeRelaySession), + 'relay', + undefined, + expect.any(Function) + ) expect(logical.getActivePath()).toBe('relay') supervisor.stop() }) @@ -156,46 +171,6 @@ describe('mobile endpoint supervisor', () => { supervisor.stop() }) - it('replaces a half-open relay on a network nudge, then backs off failed resumes', async () => { - const logical = new FakeLogicalClient('disconnected', 'lan') - const openRelay = vi - .fn() - .mockReturnValueOnce(new FakeRelaySession('connected')) - .mockImplementation(() => new FakeRelaySession('disconnected', new RelayOuterError(4408))) - const deps = dependencies({ - openRelay, - // Keep direct unavailable so relay recovery stays the only path under test. - openDirect: vi.fn(() => new FakeSession('disconnected')), - // Deterministic full jitter: fraction 0.5 → half the backoff window. - randomBytes: () => new Uint8Array([128, 0]) - }) - const supervisor = new MobileEndpointSupervisor(logical, host, deps) - - await supervisor.start() - expect(openRelay).toHaveBeenCalledOnce() - - // The OS reports a network handoff, but the dead relay never published onclose. - supervisor.setForeground(true) - await vi.advanceTimersByTimeAsync(0) - expect(logical.suspendActiveSession).toHaveBeenCalledOnce() - expect(openRelay).toHaveBeenCalledTimes(2) - - // The relay cell rejects the replacement with PEER_DROPPED; more flap nudges - // must share the existing cooldown rather than opening more sockets. - for (let i = 0; i < 5; i++) { - supervisor.setForeground(true) - await vi.advanceTimersByTimeAsync(0) - } - expect(openRelay).toHaveBeenCalledTimes(2) - - // Exactly one retry fires at the 250 ms deterministic backoff boundary. - await vi.advanceTimersByTimeAsync(249) - expect(openRelay).toHaveBeenCalledTimes(2) - await vi.advanceTimersByTimeAsync(1) - expect(openRelay).toHaveBeenCalledTimes(3) - supervisor.stop() - }) - it('backs off a close from the active relay before opening its replacement', async () => { const logical = new FakeLogicalClient('disconnected', 'lan') const openRelay = vi @@ -714,7 +689,10 @@ describe('mobile endpoint supervisor', () => { await vi.advanceTimersByTimeAsync(60_000) expect(openRelay).toHaveBeenCalledTimes(2) - supervisor.setForeground(true) + // A network nudge inside the cooldown must not dial early and must not tear + // down the healthy session; the queued intent runs at the 250 ms boundary. + supervisor.nudge('network-change') + expect(logical.suspendActiveSession).not.toHaveBeenCalled() await vi.advanceTimersByTimeAsync(249) expect(openRelay).toHaveBeenCalledTimes(2) await vi.advanceTimersByTimeAsync(1) @@ -768,6 +746,91 @@ describe('mobile endpoint supervisor', () => { supervisor.stop() }) + it('races a relay dial when the direct dial stalls unauthenticated', async () => { + const logical = new FakeLogicalClient('connecting', 'lan') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + await vi.advanceTimersByTimeAsync(2_499) + expect(deps.openRelay).not.toHaveBeenCalled() + expect(logical.getState()).toBe('connecting') + + // The direct dial never authenticates; the relay wins the race through migrateTo. + await vi.advanceTimersByTimeAsync(1) + await vi.waitFor(() => expect(logical.getActivePath()).toBe('relay')) + expect(logical.migrateTo).toHaveBeenCalledWith( + expect.any(FakeRelaySession), + 'relay', + undefined, + expect.any(Function) + ) + supervisor.stop() + }) + + it('cancels the grace race when the direct dial authenticates first', async () => { + const logical = new FakeLogicalClient('connecting', 'lan') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + logical.publishState('connected') + expect(vi.getTimerCount()).toBe(0) + + await vi.advanceTimersByTimeAsync(5_000) + expect(deps.openRelay).not.toHaveBeenCalled() + expect(logical.getActivePath()).toBe('lan') + supervisor.stop() + }) + + it('never races a relay dial against a desktop with no relay endpoint', async () => { + const logical = new FakeLogicalClient('connecting', 'lan') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, { ...host, relay: undefined }, deps) + + await supervisor.start() + await vi.advanceTimersByTimeAsync(5_000) + + expect(deps.openRelay).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + supervisor.stop() + }) + + it('drops the pending grace race when the phone backgrounds', async () => { + const logical = new FakeLogicalClient('connecting', 'lan') + const deps = dependencies() + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + supervisor.setForeground(false) + await vi.advanceTimersByTimeAsync(5_000) + + expect(deps.openRelay).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + supervisor.stop() + }) + + it('books the shared cooldown when the grace race loses its dial', async () => { + const logical = new FakeLogicalClient('connecting', 'lan') + const openRelay = vi.fn(() => new FakeRelaySession('disconnected', new RelayOuterError(4408))) + const deps = dependencies({ openRelay, randomBytes: () => new Uint8Array([128, 0]) }) + const supervisor = new MobileEndpointSupervisor(logical, host, deps) + + await supervisor.start() + await vi.advanceTimersByTimeAsync(2_500) + expect(openRelay).toHaveBeenCalledOnce() + + // The armed retry runs unforced, so it yields to the still-progressing direct + // dial: the race gets one attempt, never a socket-per-cooldown loop. + await vi.advanceTimersByTimeAsync(60_000) + expect(openRelay).toHaveBeenCalledOnce() + + // Direct finally gives up: ordinary recovery still owns the failure. + logical.publishState('reconnecting') + await vi.waitFor(() => expect(openRelay).toHaveBeenCalledTimes(2)) + supervisor.stop() + }) + it('releases a background relay session and reconnects it on foreground', async () => { const logical = new FakeLogicalClient('connected', 'relay') const deps = dependencies() diff --git a/mobile/src/transport/mobile-endpoint-supervisor.ts b/mobile/src/transport/mobile-endpoint-supervisor.ts index be295fdcd..13cb1293e 100644 --- a/mobile/src/transport/mobile-endpoint-supervisor.ts +++ b/mobile/src/transport/mobile-endpoint-supervisor.ts @@ -4,21 +4,21 @@ import { RelayReconnectController } from './mobile-relay-reconnect-controller' import { RelayLeaseRotationTimer } from './mobile-relay-lease-rotation-timer' import { MobileEndpointHysteresis } from './mobile-endpoint-hysteresis' import { - encodeBase64Url, - isDirectorResolutionFailure, persistRelayHost, - toError + suspendRelayIfStillConnected } from './mobile-endpoint-supervisor-support' import { selectDialableRelayCredentials } from './mobile-relay-credential-selection' import { createRelayRecoveryLog, type RelayRecoveryLog } from './mobile-relay-recovery-log' import { - applyResumeConfirmation, mobileRelayCredentialNeedsRotation, rotateMobileRelayCredential } from './mobile-relay-credential-rotation' import type { MobileRelayCredentialBundle } from './mobile-relay-credential-bundle' +import { MobileEndpointNudgeRouter } from './mobile-endpoint-nudge-router' +import { MobileRelayDirectGraceTimer } from './mobile-relay-direct-grace-timer' +import { MobileRelaySessionEstablisher } from './mobile-relay-session-establisher' import type { StableLogicalRpcClient } from './stable-logical-rpc-client' -import type { HostProfile } from './types' +import type { ForegroundNudgeReason, HostProfile } from './types' export type { MobileEndpointSupervisorDependencies } from './mobile-endpoint-supervisor-contract' @@ -31,6 +31,8 @@ export class MobileEndpointSupervisor { private stopped = false private foreground = true private operationInFlight = false + private pendingReplace = false + private readonly nudgeRouter: MobileEndpointNudgeRouter private credentialRotationInFlight = false private relayRotationPending = false private unsubscribeState: (() => void) | null = null @@ -39,6 +41,8 @@ export class MobileEndpointSupervisor { private readonly leaseRotation: RelayLeaseRotationTimer private readonly logRelay: RelayRecoveryLog private readonly directProbe: DirectReturnProbe + private readonly directGrace: MobileRelayDirectGraceTimer + private readonly sessionEstablisher: MobileRelaySessionEstablisher constructor( private readonly logical: StableLogicalRpcClient, @@ -53,10 +57,61 @@ export class MobileEndpointSupervisor { }) this.logRelay = createRelayRecoveryLog(dependencies.now, dependencies.onLog) this.relayReconnect = new RelayReconnectController(dependencies, this.recoverRelay.bind(this)) + this.nudgeRouter = new MobileEndpointNudgeRouter({ + logical, + controller: this.relayReconnect, + now: dependencies.now, + isStopped: () => this.stopped, + isForeground: () => this.foreground, + setForeground: (foreground) => this.setForeground(foreground), + replaceRelay: () => void this.recoverRelay(true, true), + recoverAfterDeadProbe: (detail) => { + this.logRelay('relay probe failed; recovering', detail) + suspendRelayIfStillConnected(this.relayReconnect, this.logical) + void this.recoverRelay() + }, + scheduleDirectProbe: () => this.directProbe.schedule(0) + }) this.leaseRotation = new RelayLeaseRotationTimer(dependencies, () => { this.relayRotationPending = true void this.recoverRelay(true) }) + // Why: the race owns recovery exactly like a network-change replacement — its + // failure must book the shared cooldown. recoverRelay's own guards already + // cover stopped/background/no-relay, so the timer needs no scope check. + this.directGrace = new MobileRelayDirectGraceTimer(dependencies, logical, () => { + void this.recoverRelay(true, true) + }) + this.sessionEstablisher = new MobileRelaySessionEstablisher({ + logical, + controller: this.relayReconnect, + openRelay: dependencies.openRelay, + randomBytes: dependencies.randomBytes, + writeBundle: dependencies.writeBundle, + isActive: () => !this.stopped && this.foreground, + isForeground: () => this.foreground, + relay: () => this.host.relay, + resolveRelay: dependencies.resolveRelay, + persistResolvedRelay: async (resolved) => { + this.host = await persistRelayHost(this.host, resolved, dependencies.saveHost) + }, + bundle: () => this.bundle, + adoptBundle: (bundle) => { + this.bundle = bundle + }, + recordMigration: () => { + this.relayRotationPending = false + this.hysteresis.recordMigration(dependencies.now()) + this.logRelay('runtime channel migrated to relay') + }, + scheduleLease: (expiry) => + this.leaseRotation.scheduleFromLease(this.stopped || !this.foreground ? null : expiry), + scheduleDirectProbe: () => this.directProbe.schedule(), + onBookkeepingError: (error) => + this.logRelay('relay bookkeeping failed after migration', error.message.slice(0, 80)), + onDialFailure: (error) => + this.logRelay('relay dial failed', `${error.name}: ${String(error.message).slice(0, 80)}`) + }) this.directProbe = new DirectReturnProbe(dependencies, { hysteresis: this.hysteresis, host: () => this.host, @@ -74,7 +129,11 @@ export class MobileEndpointSupervisor { }, afterProbe: () => { this.operationInFlight = false - if (this.relayRotationPending || this.logical.getState() !== 'connected') { + if ( + this.pendingReplace || + this.relayRotationPending || + this.logical.getState() !== 'connected' + ) { void this.recoverRelay(this.relayRotationPending) } } @@ -93,6 +152,7 @@ export class MobileEndpointSupervisor { } this.unsubscribeState = this.logical.onStateChange((state) => { if (state === 'connected') { + this.directGrace.clear() if (this.logical.getActivePath() !== 'relay') { void this.rotateCredentialIfNeeded(this.relayReconnect.resetForDirectConnection()) } @@ -109,6 +169,7 @@ export class MobileEndpointSupervisor { await this.recoverRelay() } else { this.directProbe.schedule() + this.directGrace.arm() } } @@ -118,15 +179,21 @@ export class MobileEndpointSupervisor { if (foreground) { this.relayReconnect.handleForeground(this.logical, wasForeground) this.directProbe.schedule(0) + this.directGrace.arm() } else { // Why: background phones must not hold billed relay data splices. this.relayReconnect.suspendActiveRelay(this.logical) this.directProbe.clear() this.relayReconnect.clear() this.leaseRotation.clear() + this.directGrace.clear() } } + nudge(reason: ForegroundNudgeReason): void { + this.nudgeRouter.nudge(reason) + } + stop(): void { this.stopped = true this.unsubscribeState?.() @@ -134,22 +201,42 @@ export class MobileEndpointSupervisor { this.directProbe.clear() this.relayReconnect.clear() this.leaseRotation.clear() + this.directGrace.clear() } - private async recoverRelay(forceReplacement = false): Promise { - // Why: connecting/handshaking is live direct progress; a relay dial would race it. - if ( - this.stopped || - !this.foreground || - this.operationInFlight || - !this.host.relay || - (!forceReplacement && !this.relayReconnect.needsRecovery(this.logical.getState())) - ) { + // forceReplacement: dial past the "direct still looks live" guard — a lease + // rotation, a network-change replacement, or the happy-eyeballs grace race. + // ownsRecovery: this dial is the connection's only hope, so a failure books the + // shared cooldown and any session left stale-'connected' by a half-open socket + // comes down; lease rotation clears it because armRetry owns its own retry. + private async recoverRelay(forceReplacement = false, ownsRecovery = false): Promise { + if (this.stopped || !this.foreground || !this.host.relay) { + return + } + if (this.operationInFlight) { + // Why: a 12s direct probe can own the mutex when a network handoff lands; + // afterProbe replays the queued replacement so the signal is never lost. + this.pendingReplace ||= forceReplacement && ownsRecovery + return + } + if (this.pendingReplace) { + this.pendingReplace = false + forceReplacement = true + ownsRecovery = true + } + // Why: connecting/handshaking is live direct progress; an unforced relay dial + // would race it before the grace timer has given direct its head start. + if (!forceReplacement && !this.relayReconnect.needsRecovery(this.logical.getState())) { return } // Why: revival and lease timers can overlap resume failures; one shared cooldown // prevents PEER_DROPPED/LIMIT_EXCEEDED reconnect churn. if (this.relayReconnect.shouldDefer()) { + if (ownsRecovery) { + // Why: never tear down a session no dial has disproven — the intent stays + // queued so the armed retry runs forced once the cooldown lapses. + this.pendingReplace = true + } this.logRelay('recovery deferred by cooldown or gate') return } @@ -173,30 +260,33 @@ export class MobileEndpointSupervisor { : 'no relay credential bundle; slow reprobe armed' ) this.relayReconnect.armCredentialReprobe() + if (ownsRecovery) { + // Why: no dial happened — keep the session and the intent; the reprobe + // runs forced and replaces make-before-break once a credential exists. + this.pendingReplace = true + } return } - for (const credential of selection.credentials) { - const result = await this.tryRelayCredential(credential) - if (result.ok) { - retryAfterOperation = this.logical.getState() !== 'connected' - return - } - lastError = result.error - this.logRelay( - 'relay dial failed', - `${result.error.name}: ${String(result.error.message).slice(0, 80)}` - ) - if (this.relayReconnect.shouldTryGraceAfterRelayFailure(result.error)) { - // Why: a rejected version stays invalid; retry only the grace credential. - this.relayReconnect.recordRejectedCredential(credential.version) - } else { - break - } + const dialed = await this.sessionEstablisher.dialEligible(selection.credentials) + if (dialed.outcome === 'established') { + // Why: a fresh socket satisfies any replacement intent queued mid-dial. + this.pendingReplace = false + retryAfterOperation = this.logical.getState() !== 'connected' + return } + if (dialed.outcome === 'aborted') { + // Why: direct won the race or the supervisor went inactive — not a + // failure; booking backoff would delay the next genuine recovery. + return + } + lastError = dialed.error // Why: cleanup may happen while a relay dial is awaiting the network; // record its outcome without recreating a foreground retry timer. - const scheduleRetry = !forceReplacement && this.foreground && !this.stopped + const scheduleRetry = (!forceReplacement || ownsRecovery) && this.foreground && !this.stopped this.relayReconnect.registerFailure(lastError, scheduleRetry) + if (ownsRecovery) { + suspendRelayIfStillConnected(this.relayReconnect, this.logical) + } } finally { this.operationInFlight = false if (forceReplacement && this.relayRotationPending && !this.stopped && this.foreground) { @@ -209,77 +299,6 @@ export class MobileEndpointSupervisor { } } - private async tryRelayCredential(credential: { - token: string - version: number - }): Promise<{ ok: true } | { ok: false; error: Error }> { - const first = await this.openAndMigrateRelay(credential) - if (first.ok) { - return first - } - if (!isDirectorResolutionFailure(first.error) || !this.host.relay) { - return first - } - try { - const resolved = await this.dependencies.resolveRelay({ - relay: this.host.relay, - resumeToken: credential.token - }) - this.host = await persistRelayHost(this.host, resolved, this.dependencies.saveHost) - return await this.openAndMigrateRelay(credential) - } catch (error) { - return { ok: false, error: toError(error) } - } - } - - private async openAndMigrateRelay(credential: { - token: string - version: number - }): Promise<{ ok: true } | { ok: false; error: Error }> { - // Why: director resolution and grace fallback can finish after background/stop. - if (this.stopped || !this.foreground || !this.host.relay || !this.bundle) { - return { ok: false, error: new Error('relay state missing') } - } - const session = this.dependencies.openRelay( - this.host.relay, - credential, - `confirm-${encodeBase64Url(this.dependencies.randomBytes(16))}` - ) - try { - await this.logical.migrateTo(session, 'relay') - this.relayReconnect.setActiveSession(session) - if (!this.foreground) { - this.relayReconnect.suspendActiveRelay(this.logical) - } - this.relayRotationPending = false - this.hysteresis.recordMigration(this.dependencies.now()) - this.logRelay('runtime channel migrated to relay') - const confirmation = session.getResumeConfirmation() - if (confirmation) { - this.bundle = applyResumeConfirmation(this.bundle, credential.version, confirmation) - // Why: the relay is already authenticated; a SecureStore failure must - // not open another socket or count against transport recovery backoff. - await this.dependencies.writeBundle(this.bundle).catch(() => {}) - } - // Why: async persistence can finish after stop/background; never recreate a stale timer. - // Why: rotate against the resume credential's expiry, never the hello's - // leaseExpiresAt — that field is the cell's ~10s attach-reservation - // deadline, and using it forced a session replacement every second. - // Why: renewed=false means a re-resume provably returns the same - // unchanged deadline — rotating then just churns one replacement per - // clamp floor until a fresh credential arrives over direct or disk. - this.leaseRotation.scheduleFromLease( - this.stopped || !this.foreground || confirmation?.renewed === false - ? null - : (confirmation?.resumeExpiresAt ?? session.getResumeExpiresAt()) - ) - this.directProbe.schedule() - return { ok: true } - } catch (error) { - return { ok: false, error: session.getFailure() ?? toError(error) } - } - } - private async rotateCredentialIfNeeded(force = false): Promise { if ( this.stopped || diff --git a/mobile/src/transport/mobile-relay-credential-rotation.ts b/mobile/src/transport/mobile-relay-credential-rotation.ts index 190524725..9b8a038e8 100644 --- a/mobile/src/transport/mobile-relay-credential-rotation.ts +++ b/mobile/src/transport/mobile-relay-credential-rotation.ts @@ -130,6 +130,38 @@ export function applyResumeConfirmation( return bundle } +// Applies a migrated session's resume confirmation to the durable bundle and +// derives the lease expiry to rotate against. +// Why: rotate against the resume credential's expiry, never the hello's +// leaseExpiresAt — that field is the cell's ~10s attach-reservation deadline, +// and using it forced a session replacement every second. +// Why: renewed=false means a re-resume provably returns the same unchanged +// deadline — rotating then just churns one replacement per clamp floor until a +// fresh credential arrives over direct or disk. +export async function persistResumeConfirmation(args: { + session: { + getResumeConfirmation(): DeviceResumeConfirmed | null + getResumeExpiresAt(): number | null + } + bundle: MobileRelayCredentialBundle + usedCredentialVersion: number + writeBundle: (bundle: MobileRelayCredentialBundle) => Promise +}): Promise<{ bundle: MobileRelayCredentialBundle; leaseExpiry: number | null }> { + const confirmation = args.session.getResumeConfirmation() + let bundle = args.bundle + if (confirmation) { + bundle = applyResumeConfirmation(bundle, args.usedCredentialVersion, confirmation) + // Why: the relay is already authenticated; a SecureStore failure must not + // open another socket or count against transport recovery backoff. + await args.writeBundle(bundle).catch(() => {}) + } + const leaseExpiry = + confirmation?.renewed === false + ? null + : (confirmation?.resumeExpiresAt ?? args.session.getResumeExpiresAt()) + return { bundle, leaseExpiry } +} + async function getEndpoints(client: RpcClient, installReqId: string) { const response = await client.sendRequest('pairing.getEndpoints', { installReqId }) if (!response.ok) { diff --git a/mobile/src/transport/mobile-relay-direct-grace-timer.ts b/mobile/src/transport/mobile-relay-direct-grace-timer.ts new file mode 100644 index 000000000..df3c1ba14 --- /dev/null +++ b/mobile/src/transport/mobile-relay-direct-grace-timer.ts @@ -0,0 +1,48 @@ +import type { StableLogicalRpcClient } from './stable-logical-rpc-client' + +// Why: on a black-holed LAN endpoint the direct dial sits in 'connecting' for the +// whole 12s connect timeout (rpc-client CONNECT_TIMEOUT_MS), and relay recovery +// cannot even start meanwhile because connecting/handshaking count as live direct +// progress. Happy eyeballs: give direct this much of a head start, then race the +// relay dial — migrateTo hands the logical client to whichever authenticates first. +const DIRECT_DIAL_GRACE_MS = 2500 + +type DirectGraceTimerDependencies = { + setTimer: typeof setTimeout + clearTimer: typeof clearTimeout +} + +// One-shot timer that releases the relay dial when the direct dial has not +// authenticated within the grace. The supervisor arms it at start and on +// foreground restore, and clears it on connect, background, and stop. +export class MobileRelayDirectGraceTimer { + private timer: ReturnType | null = null + + constructor( + private readonly dependencies: DirectGraceTimerDependencies, + private readonly logical: StableLogicalRpcClient, + private readonly dialRelay: () => void + ) {} + + // No-op unless the direct dial is still unauthenticated, so a healthy LAN and + // an already-failed direct path (recovery owns that) never open a relay socket. + arm(): void { + const state = this.logical.getState() + if (this.timer || (state !== 'connecting' && state !== 'handshaking')) { + return + } + this.timer = this.dependencies.setTimer(() => { + this.timer = null + if (this.logical.getState() !== 'connected') { + this.dialRelay() + } + }, DIRECT_DIAL_GRACE_MS) + } + + clear(): void { + if (this.timer) { + this.dependencies.clearTimer(this.timer) + this.timer = null + } + } +} diff --git a/mobile/src/transport/mobile-relay-direct-upgrade-controller.ts b/mobile/src/transport/mobile-relay-direct-upgrade-controller.ts index 1cd57cfcb..be0e1ccfb 100644 --- a/mobile/src/transport/mobile-relay-direct-upgrade-controller.ts +++ b/mobile/src/transport/mobile-relay-direct-upgrade-controller.ts @@ -40,6 +40,12 @@ export class MobileRelayDirectUpgradeController { } } + // No relay session exists yet; the direct socket already self-probes on + // notifyForeground, so a nudge only retries the pending upgrade. + nudge(): void { + this.setForeground(true) + } + stop(): void { this.stopped = true this.unsubscribe?.() diff --git a/mobile/src/transport/mobile-relay-focus-probe.ts b/mobile/src/transport/mobile-relay-focus-probe.ts new file mode 100644 index 000000000..2671a62cd --- /dev/null +++ b/mobile/src/transport/mobile-relay-focus-probe.ts @@ -0,0 +1,53 @@ +import { + isLogicalClientCutoverError, + type StableLogicalRpcClient +} from './stable-logical-rpc-client' + +// Why: a probe must fail well before the 12s connect ceiling so a dead socket +// still recovers in a few seconds, but outlast one slow cellular RTT. +const PROBE_TIMEOUT_MS = 4_000 +// Why: focus fires per navigation event; one liveness answer covers them all. +const PROBE_MIN_INTERVAL_MS = 10_000 + +// Cheap liveness check for a focus nudge on an active relay: a healthy session +// answers and nothing visible changes; only a dead one is handed to onDead. +export class MobileRelayFocusProbe { + private lastProbeAt = 0 + + constructor( + private readonly args: { + logical: StableLogicalRpcClient + now: () => number + canProbe: () => boolean + onDead: (detail: string) => void + } + ) {} + + async probe(): Promise { + const { logical, now } = this.args + const at = now() + if ( + !this.args.canProbe() || + at - this.lastProbeAt < PROBE_MIN_INTERVAL_MS || + logical.getActivePath() !== 'relay' || + logical.getState() !== 'connected' + ) { + return + } + this.lastProbeAt = at + const generation = logical.getGeneration() + try { + await logical.sendRequest('status.get', null, { timeoutMs: PROBE_TIMEOUT_MS }) + } catch (error) { + if ( + isLogicalClientCutoverError(error) || + !this.args.canProbe() || + logical.getGeneration() !== generation || + logical.getActivePath() !== 'relay' + ) { + return + } + this.args.onDead(String((error as Error).message).slice(0, 80)) + } + } +} diff --git a/mobile/src/transport/mobile-relay-physical-client.test.ts b/mobile/src/transport/mobile-relay-physical-client.test.ts index e4aaa0fe4..77972cc6d 100644 --- a/mobile/src/transport/mobile-relay-physical-client.test.ts +++ b/mobile/src/transport/mobile-relay-physical-client.test.ts @@ -27,6 +27,7 @@ vi.mock('./mobile-e2ee-v2-physical-channel', () => ({ })) import { connectMobileRelayForPairing, RelayOuterError } from './mobile-relay-physical-client' +import type { ConnectionLogEntry } from './types' class FakeSocket { readonly OPEN = 1 @@ -132,4 +133,58 @@ describe('mobile relay physical pairing client', () => { await expect(status).rejects.toEqual(new RelayOuterError(4404)) expect(fakes.start).not.toHaveBeenCalled() }) + + it('narrates dial, outer auth, handshake and authentication without leaking the invite', async () => { + const socket = new FakeSocket() + const entries: ConnectionLogEntry[] = [] + const client = connectMobileRelayForPairing({ + relay, + deviceToken: 'device-token', + desktopPublicKeyB64: 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=', + createSocket: () => socket as unknown as WebSocket, + onLog: (entry) => entries.push(entry) + }) + socket.onopen?.() + socket.receive( + JSON.stringify({ + type: 'relay-hello', + ok: true, + credentialKind: 'invite', + leaseExpiresAt: Date.now() + 60_000 + }) + ) + await vi.waitFor(() => expect(fakes.start).toHaveBeenCalledOnce()) + fakes.channelOptions!.onAuthenticated() + client.close() + + expect(entries.map((entry) => `${entry.level}|${entry.message}|${entry.detail}`)).toEqual([ + 'info|Relay: dialing cell|relay-c1.onorca.dev', + 'info|Relay: cell socket open|Sending relay credential', + 'info|Relay: cell accepted credential|Starting E2EE handshake', + 'success|Relay: authenticated|Channel ready for RPC', + 'info|Relay: pairing socket closed|relay-c1.onorca.dev' + ]) + expect(JSON.stringify(entries)).not.toContain(relay.inviteToken) + }) + + it('logs the relay close code when the cell rejects the credential', async () => { + const socket = new FakeSocket() + const entries: ConnectionLogEntry[] = [] + const client = connectMobileRelayForPairing({ + relay, + deviceToken: 'device-token', + desktopPublicKeyB64: 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=', + createSocket: () => socket as unknown as WebSocket, + onLog: (entry) => entries.push(entry) + }) + const status = client.sendRequest('status.get') + socket.receive(JSON.stringify({ type: 'relay-hello', ok: false, code: 4404 })) + + await expect(status).rejects.toEqual(new RelayOuterError(4404)) + expect(entries.at(-1)).toMatchObject({ + level: 'warn', + message: 'Relay: pairing socket closed', + detail: 'relay close code 4404' + }) + }) }) diff --git a/mobile/src/transport/mobile-relay-physical-client.ts b/mobile/src/transport/mobile-relay-physical-client.ts index acf264b8c..37b5d6485 100644 --- a/mobile/src/transport/mobile-relay-physical-client.ts +++ b/mobile/src/transport/mobile-relay-physical-client.ts @@ -2,8 +2,10 @@ import type { PairingRelay } from '../../../src/shared/mobile-relay-pairing-offe import { RelayPhoneHelloSchema } from '../../../src/shared/mobile-relay-phone-protocol' import { MobileE2EEV2ClientSession } from './mobile-e2ee-v2-client-session' import { MobileE2EEV2PhysicalChannel } from './mobile-e2ee-v2-physical-channel' +import { createPairingRelayLogger, pairingRelayErrorDetail } from './pairing-relay-log' import { isRpcResponse } from './rpc-response-shape' -import type { RpcResponse } from './types' +import { redactSocketEndpoint } from './socket-event-debug' +import type { ConnectionLogSink, RpcResponse } from './types' import { websocketPayloadToUint8 } from './websocket-payload-bytes' export { RelayOuterError } from './mobile-relay-e2ee-link' import { RelayOuterError } from './mobile-relay-e2ee-link' @@ -27,9 +29,13 @@ export function connectMobileRelayForPairing(args: { expectedCredentialKind?: 'invite' | 'resume' requestTimeoutMs?: number createSocket?: (url: string) => WebSocket + onLog?: ConnectionLogSink }): PairingCandidateClient { const requestTimeoutMs = args.requestTimeoutMs ?? 30_000 const socketUrl = relayPhoneWebSocketUrl(args.relay) + const log = createPairingRelayLogger(args.onLog) + const cellHost = redactSocketEndpoint(socketUrl) + log('info', 'Relay: dialing cell', cellHost) const socket = (args.createSocket ?? ((url) => new WebSocket(url)))(socketUrl) const session = MobileE2EEV2ClientSession.create({ desktopPublicKeyB64: args.desktopPublicKeyB64, @@ -39,6 +45,7 @@ export function connectMobileRelayForPairing(args: { const pending = new Map() let requestCounter = 0 let closed = false + let intentionallyClosed = false let outerReady = false let authenticated = false let resolveAuthenticated!: () => void @@ -54,6 +61,7 @@ export function connectMobileRelayForPairing(args: { decodeBinary: websocketPayloadToUint8, onAuthenticated: () => { authenticated = true + log('success', 'Relay: authenticated', 'Channel ready for RPC') resolveAuthenticated() }, onText: (plaintext) => { @@ -78,6 +86,7 @@ export function connectMobileRelayForPairing(args: { }) socket.onopen = () => { + log('info', 'Relay: cell socket open', 'Sending relay credential') socket.send( JSON.stringify({ type: 'relay-auth', @@ -126,6 +135,7 @@ export function connectMobileRelayForPairing(args: { throw new Error('relay credential resolved as an unexpected credential kind') } outerReady = true + log('info', 'Relay: cell accepted credential', 'Starting E2EE handshake') channel.start() } @@ -134,6 +144,11 @@ export function connectMobileRelayForPairing(args: { return } closed = true + if (intentionallyClosed) { + log('info', 'Relay: pairing socket closed', cellHost) + } else { + log('warn', 'Relay: pairing socket closed', pairingRelayErrorDetail(error)) + } channel.dispose() rejectAuthenticated(error) for (const request of pending.values()) { @@ -166,7 +181,10 @@ export function connectMobileRelayForPairing(args: { } }) }, - close: () => fail(new Error('relay pairing client closed')) + close: () => { + intentionallyClosed = true + fail(new Error('relay pairing client closed')) + } } } diff --git a/mobile/src/transport/mobile-relay-reconnect-controller.ts b/mobile/src/transport/mobile-relay-reconnect-controller.ts index 9ec5c1398..2955819ea 100644 --- a/mobile/src/transport/mobile-relay-reconnect-controller.ts +++ b/mobile/src/transport/mobile-relay-reconnect-controller.ts @@ -8,7 +8,7 @@ import { MobileE2EEAuthenticationError } from './mobile-e2ee-v2-physical-channel import { RelayOuterError } from './mobile-relay-e2ee-link' import { RELAY_STABLE_CONNECTION_MS, RelayRetryDelays } from './mobile-relay-retry-delays' import type { StableLogicalRpcClient } from './stable-logical-rpc-client' -import type { ConnectionState } from './types' +import type { ConnectionState, ForegroundNudgeReason } from './types' export type RelayReconnectDependencies = { now: () => number @@ -50,18 +50,31 @@ export class RelayReconnectController { } else if (this.recoveryGate === 'external-signal') { this.liftGate() } - if ( - wasForeground && - this.recoveryGate !== 'fresh-credential' && - logical.getState() === 'connected' - ) { - // Why: a network handoff can leave the relay half-open without publishing a close. - this.suspendActiveRelay(logical) - } // Why: revival nudges must honor failure cooldowns even when lease rotation is pending. this.onRetry() } + // Classifies a nudge that arrives while already foreground. A healthy relay is + // never suspended here: focus/app-resume probe it, a network change replaces it + // make-before-break — suspending first was the grey-blink bug (S2). + handleActiveNudge( + logical: StableLogicalRpcClient, + reason: ForegroundNudgeReason + ): 'probe' | 'replace' | 'recover' { + if (this.recoveryGate === 'external-signal') { + this.liftGate() + } + if ( + this.recoveryGate !== 'fresh-credential' && + logical.getActivePath() === 'relay' && + logical.getState() === 'connected' + ) { + return reason === 'network-change' ? 'replace' : 'probe' + } + this.onRetry() + return 'recover' + } + handleStateFailure(logical: StableLogicalRpcClient, state: ConnectionState): void { if (!this.needsRecovery(state)) { return diff --git a/mobile/src/transport/mobile-relay-session-establisher.ts b/mobile/src/transport/mobile-relay-session-establisher.ts new file mode 100644 index 000000000..d4fa52854 --- /dev/null +++ b/mobile/src/transport/mobile-relay-session-establisher.ts @@ -0,0 +1,137 @@ +import type { MobileEndpointSupervisorDependencies } from './mobile-endpoint-supervisor-contract' +import { + dialRelayThroughDirectorFallback, + encodeBase64Url, + RelayDialAbortedError, + toError +} from './mobile-endpoint-supervisor-support' +import { persistResumeConfirmation } from './mobile-relay-credential-rotation' +import type { MobileRelayCredentialBundle } from './mobile-relay-credential-bundle' +import type { RelayReconnectController } from './mobile-relay-reconnect-controller' +import type { StableLogicalRpcClient } from './stable-logical-rpc-client' +import type { MobileRelayEndpoint } from '../../../src/shared/mobile-relay-credential-contract' +import type { HostProfile } from './types' + +type EstablishResult = { ok: true } | { ok: false; error: Error } + +function directWon(logical: StableLogicalRpcClient): boolean { + return logical.getActivePath() !== 'relay' && logical.getState() === 'connected' +} + +// Turns one relay credential into the active runtime session: resolve the cell +// assignment if the director rejects the cached one, open the cell socket, +// migrate the logical client onto it, then persist the resume confirmation and +// re-arm the supervisor's timers. +export class MobileRelaySessionEstablisher { + constructor( + private readonly args: { + logical: StableLogicalRpcClient + controller: RelayReconnectController + openRelay: MobileEndpointSupervisorDependencies['openRelay'] + randomBytes: (length: number) => Uint8Array + writeBundle: (bundle: MobileRelayCredentialBundle) => Promise + isActive: () => boolean + isForeground: () => boolean + relay: () => HostProfile['relay'] + resolveRelay: MobileEndpointSupervisorDependencies['resolveRelay'] + persistResolvedRelay: (resolved: MobileRelayEndpoint) => Promise + bundle: () => MobileRelayCredentialBundle | null + adoptBundle: (bundle: MobileRelayCredentialBundle) => void + // Hysteresis stamp + rotation-pending clear + recovery log line. + recordMigration: () => void + // Owns the stopped/background null-out so a late resolve never re-arms a stale timer. + scheduleLease: (expiry: number | null) => void + scheduleDirectProbe: () => void + onBookkeepingError: (error: Error) => void + onDialFailure: (error: Error) => void + } + ) {} + + // Tries each eligible credential until one establishes. Only a grace-repairable + // failure (BAD_OUTER_CREDENTIAL) moves on to the next credential. + async dialEligible( + credentials: { token: string; version: number }[] + ): Promise< + { outcome: 'established' } | { outcome: 'aborted' } | { outcome: 'failed'; error: Error | null } + > { + let lastError: Error | null = null + for (const credential of credentials) { + const result = await this.dial(credential) + if (result.ok) { + return { outcome: 'established' } + } + if (result.error instanceof RelayDialAbortedError) { + return { outcome: 'aborted' } + } + lastError = result.error + this.args.onDialFailure(result.error) + if (!this.args.controller.shouldTryGraceAfterRelayFailure(result.error)) { + break + } + // Why: a rejected version stays invalid; retry only the grace credential. + this.args.controller.recordRejectedCredential(credential.version) + } + return { outcome: 'failed', error: lastError } + } + + // One credential attempt: a director-class failure re-resolves the cell + // assignment, persists it, and dials once more against the authoritative target. + dial(credential: { token: string; version: number }): Promise { + return dialRelayThroughDirectorFallback({ + resumeToken: credential.token, + relay: this.args.relay, + dial: () => this.establish(credential), + resolveRelay: this.args.resolveRelay, + persistResolvedRelay: this.args.persistResolvedRelay + }) + } + + private async establish(credential: { + token: string + version: number + }): Promise { + const { args } = this + const relay = args.relay() + const bundle = args.bundle() + // Why: director resolution and grace fallback can finish after background/stop. + if (!args.isActive() || !relay || !bundle) { + return { ok: false, error: new RelayDialAbortedError() } + } + const session = args.openRelay( + relay, + credential, + `confirm-${encodeBase64Url(args.randomBytes(16))}` + ) + try { + // Why: if an authenticated non-relay session appears while this dial is in + // flight (the grace race), withdraw instead of cutting over the winner. + await args.logical.migrateTo(session, 'relay', undefined, () => directWon(args.logical)) + } catch (error) { + if (directWon(args.logical)) { + return { ok: false, error: new RelayDialAbortedError() } + } + return { ok: false, error: session.getFailure() ?? toError(error) } + } + args.controller.setActiveSession(session) + if (!args.isForeground()) { + args.controller.suspendActiveRelay(args.logical) + } + args.recordMigration() + try { + const applied = await persistResumeConfirmation({ + session, + bundle, + usedCredentialVersion: credential.version, + writeBundle: args.writeBundle + }) + args.adoptBundle(applied.bundle) + args.scheduleLease(applied.leaseExpiry) + } catch (error) { + // Why: the session is live and registered — reporting bookkeeping as a dial + // failure would book backoff against it and can suspend the healthy session. + args.onBookkeepingError(toError(error)) + } + args.scheduleDirectProbe() + return { ok: true } + } +} diff --git a/mobile/src/transport/pairing-relay-candidate.test.ts b/mobile/src/transport/pairing-relay-candidate.test.ts index 742af991c..07d315a48 100644 --- a/mobile/src/transport/pairing-relay-candidate.test.ts +++ b/mobile/src/transport/pairing-relay-candidate.test.ts @@ -3,6 +3,7 @@ import type { PairingCandidateClient } from './mobile-relay-physical-client' import { RelayOuterError } from './mobile-relay-physical-client' import { createRecoveringPairingRelayCandidate } from './pairing-relay-candidate' import type { MobileRelayPairingJournal } from './mobile-relay-pairing-journal' +import type { ConnectionLogEntry } from './types' vi.mock('react-native', () => ({ Platform: { OS: 'ios' } })) vi.mock('expo-crypto', () => ({ @@ -163,4 +164,76 @@ describe('recovering pairing relay candidate', () => { expect(resolveDirector).toHaveBeenCalledTimes(3) expect(sleep.mock.calls.map(([delay]) => delay)).toEqual([50, 100, 200]) }) + + it('narrates every director attempt, cell move and backoff to the pairing log', async () => { + const entries: ConnectionLogEntry[] = [] + const stale = client(Promise.reject(new Error('HTTP 503'))) + const target = client(Promise.resolve(success())) + const resolveDirector = vi + .fn() + .mockRejectedValueOnce(new Error('HTTP 504')) + .mockImplementationOnce(async (relay) => ({ + ...relay, + cellUrl: 'https://relay-c2.onorca.dev', + assignmentEpoch: 8 + })) + let connects = 0 + const candidate = createRecoveringPairingRelayCandidate({ + journal, + connect: () => (connects++ === 0 ? stale : target), + resolveDirector, + persistMove: vi.fn(async () => {}), + now: () => 1, + random: () => 0.5, + sleep: async () => {}, + onLog: (entry) => entries.push(entry) + }) + + await expect(candidate.sendRequest('status.get')).resolves.toEqual(success()) + expect(entries.map((entry) => `${entry.level}|${entry.message}`)).toEqual([ + 'warn|Relay: cell dial failed', + 'info|Relay: resolving director (attempt 1/3)', + 'warn|Relay: recovery attempt 1 failed', + 'info|Relay: backing off 50ms', + 'info|Relay: resolving director (attempt 2/3)', + 'info|Relay: cell moved', + 'info|Relay: backing off 100ms' + ]) + expect(entries[0]!.detail).toBe('Error: HTTP 503') + expect(entries[1]!.detail).toBe('relay.onorca.dev') + expect(entries[2]!.detail).toBe('Error: HTTP 504') + expect(entries[5]!.detail).toBe('relay-c1.onorca.dev → relay-c2.onorca.dev') + expect(new Set(entries.map((entry) => entry.id)).size).toBe(entries.length) + }) + + it('reports the final give-up once the recovery budget is spent', async () => { + const entries: ConnectionLogEntry[] = [] + const stale = client(Promise.reject(new RelayOuterError(4409))) + const candidate = createRecoveringPairingRelayCandidate({ + journal, + connect: () => stale, + resolveDirector: vi.fn(async () => { + throw new Error('relay director resolution timed out') + }), + persistMove: vi.fn(async () => {}), + now: () => 1, + random: () => 0, + sleep: async () => {}, + maxRecoveryAttempts: 2, + onLog: (entry) => entries.push(entry) + }) + + await expect(candidate.sendRequest('status.get')).rejects.toThrow(/timed out/) + expect(entries[0]).toMatchObject({ + level: 'warn', + message: 'Relay: cell dial failed', + detail: 'relay close code 4409' + }) + expect(entries.filter((entry) => entry.level === 'error')).toEqual([ + expect.objectContaining({ + message: 'Relay: recovery gave up', + detail: 'after 2 attempt(s)' + }) + ]) + }) }) diff --git a/mobile/src/transport/pairing-relay-candidate.ts b/mobile/src/transport/pairing-relay-candidate.ts index 50a7108b5..2293ce185 100644 --- a/mobile/src/transport/pairing-relay-candidate.ts +++ b/mobile/src/transport/pairing-relay-candidate.ts @@ -1,19 +1,24 @@ import type { PairingRelay } from '../../../src/shared/mobile-relay-pairing-offer' import type { MobileRelayPairingJournal } from './mobile-relay-pairing-journal' import { RelayOuterError, type PairingCandidateClient } from './mobile-relay-physical-client' +import { createPairingRelayLogger, pairingRelayErrorDetail } from './pairing-relay-log' +import { redactSocketEndpoint } from './socket-event-debug' +import type { ConnectionLogSink } from './types' export function createRecoveringPairingRelayCandidate(args: { journal: MobileRelayPairingJournal - connect: (relay: PairingRelay) => PairingCandidateClient + connect: (relay: PairingRelay, onLog?: ConnectionLogSink) => PairingCandidateClient resolveDirector: (relay: PairingRelay) => Promise persistMove: (relay: PairingRelay) => Promise now: () => number random?: () => number sleep?: (delayMs: number) => Promise maxRecoveryAttempts?: number + onLog?: ConnectionLogSink }): PairingCandidateClient { + const log = createPairingRelayLogger(args.onLog) let relay = pairingRelayFromJournal(args.journal) - let client = args.connect(relay) + let client = args.connect(relay, args.onLog) let closed = false return { @@ -43,32 +48,52 @@ export function createRecoveringPairingRelayCandidate(args: { const random = args.random ?? Math.random const sleep = args.sleep ?? ((delayMs) => new Promise((resolve) => setTimeout(resolve, delayMs))) + const backOff = async (attempt: number): Promise => { + const capMs = Math.min(2_000, 100 * 2 ** attempt) + const delayMs = Math.floor(random() * (capMs + 1)) + if (delayMs > 0) { + log('info', `Relay: backing off ${delayMs}ms`) + } + await sleep(delayMs) + } let lastError = initialError + log('warn', 'Relay: cell dial failed', pairingRelayErrorDetail(initialError)) for (let attempt = 0; attempt < maxAttempts; attempt += 1) { if (closed || relay.inviteExpiresAt <= args.now()) { + log('error', 'Relay: recovery gave up', closed ? 'pairing cancelled' : 'invite expired') throw lastError } + log( + 'info', + `Relay: resolving director (attempt ${attempt + 1}/${maxAttempts})`, + redactSocketEndpoint(relay.directorUrl) + ) try { const moved = await args.resolveDirector(relay) // Why: the authenticated newer assignment must be durable before a // target dial so a crash cannot revert to the known-stale cell. await args.persistMove(moved) + log( + 'info', + 'Relay: cell moved', + `${redactSocketEndpoint(relay.cellUrl)} → ${redactSocketEndpoint(moved.cellUrl)}` + ) client.close() relay = moved - const capMs = Math.min(2_000, 100 * 2 ** attempt) - await sleep(Math.floor(random() * (capMs + 1))) + await backOff(attempt) if (closed) { throw new Error('relay pairing client closed') } - client = args.connect(relay) + client = args.connect(relay, args.onLog) return await client.sendRequest(method, params) } catch (error) { lastError = error + log('warn', `Relay: recovery attempt ${attempt + 1} failed`, pairingRelayErrorDetail(error)) if (!isDirectorRecoverable(error) || attempt + 1 >= maxAttempts) { + log('error', 'Relay: recovery gave up', `after ${attempt + 1} attempt(s)`) throw error } - const capMs = Math.min(2_000, 100 * 2 ** attempt) - await sleep(Math.floor(random() * (capMs + 1))) + await backOff(attempt) } } throw lastError diff --git a/mobile/src/transport/pairing-relay-log.ts b/mobile/src/transport/pairing-relay-log.ts new file mode 100644 index 000000000..78653bddd --- /dev/null +++ b/mobile/src/transport/pairing-relay-log.ts @@ -0,0 +1,29 @@ +import { RelayOuterError } from './mobile-relay-e2ee-link' +import type { ConnectionLogLevel, ConnectionLogSink } from './types' + +export type PairingRelayLogger = ( + level: ConnectionLogLevel, + message: string, + detail?: string +) => void + +// Why: every relay dial and recovery attempt builds its own logger but they all +// feed one list, so the id sequence has to be process-wide to stay a unique key. +let sequence = 0 + +export function createPairingRelayLogger(onLog?: ConnectionLogSink): PairingRelayLogger { + if (!onLog) { + return () => {} + } + return (level, message, detail) => { + onLog({ id: `relay-pair-log-${++sequence}`, ts: Date.now(), level, message, detail }) + } +} + +export function pairingRelayErrorDetail(error: unknown): string { + if (error instanceof RelayOuterError) { + return `relay close code ${error.code}` + } + const failure = error instanceof Error ? error : new Error(String(error)) + return `${failure.name}: ${String(failure.message).slice(0, 80)}` +} diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts index 782c02aed..e5951dbdc 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts @@ -3,7 +3,7 @@ import type { MobileRelayCredentialBundle } from './mobile-relay-credential-bund import type { MobileRelayPairingJournal } from './mobile-relay-pairing-journal' import { racePairingCandidates } from './pairing-candidate-race' import { startPreProfilePairing } from './pre-profile-pairing-coordinator' -import type { HostProfile, PairingOffer, RpcResponse } from './types' +import type { ConnectionLogEntry, HostProfile, PairingOffer, RpcResponse } from './types' import type { RpcClient } from './rpc-client' vi.mock('react-native', () => ({ Platform: { OS: 'ios' } })) @@ -53,6 +53,44 @@ function fakeClient(responses: RpcResponse[]) { } as unknown as RpcClient } +function relayProvisioningClient(getJournal: () => MobileRelayPairingJournal) { + return { + sendRequest: vi.fn(async (method: string) => { + if (method === 'status.get') { + return success({ path: 'relay' }) + } + const installed = { + v: 1 as const, + reqId: getJournal().metadata.installReqId, + authorizationMode: 'relay-basis' as const, + currentVersion: 1, + resumeExpiresAt: now + 86_400_000 + } + if (method === 'pairing.provisionRelay') { + return success(installed) + } + return success({ + v: 1, + relay: { + v: 1, + directorUrl: relayOffer.relay!.directorUrl, + cellUrl: relayOffer.relay!.cellUrl, + assignmentEpoch: 7, + relayHostId: relayOffer.relay!.relayHostId, + e2eeFraming: 2 + }, + installStatus: { + v: 1, + reqId: getJournal().metadata.installReqId, + state: 'committed', + result: installed + } + }) + }), + close: vi.fn() + } as unknown as RpcClient +} + function dependencies(client: RpcClient, events: string[]) { const unavailableRelay = fakeClient([]) ;(unavailableRelay.sendRequest as ReturnType).mockRejectedValue( @@ -280,41 +318,7 @@ describe('pre-profile pairing coordinator', () => { const direct = fakeClient([]) ;(direct.sendRequest as ReturnType).mockRejectedValue(new Error('LAN down')) let journal: MobileRelayPairingJournal | null = null - const relay = { - sendRequest: vi.fn(async (method: string) => { - if (method === 'status.get') { - return success({ path: 'relay' }) - } - const installed = { - v: 1 as const, - reqId: journal!.metadata.installReqId, - authorizationMode: 'relay-basis' as const, - currentVersion: 1, - resumeExpiresAt: now + 86_400_000 - } - if (method === 'pairing.provisionRelay') { - return success(installed) - } - return success({ - v: 1, - relay: { - v: 1, - directorUrl: relayOffer.relay!.directorUrl, - cellUrl: relayOffer.relay!.cellUrl, - assignmentEpoch: 7, - relayHostId: relayOffer.relay!.relayHostId, - e2eeFraming: 2 - }, - installStatus: { - v: 1, - reqId: journal!.metadata.installReqId, - state: 'committed', - result: installed - } - }) - }), - close: vi.fn() - } as unknown as RpcClient + const relay = relayProvisioningClient(() => journal!) const deps = dependencies(direct, []) deps.connectRelay.mockReturnValue(relay) deps.saveJournal.mockImplementation(async (value) => { @@ -338,6 +342,46 @@ describe('pre-profile pairing coordinator', () => { ) }) + it('streams the relay path and the winning path into the pairing log', async () => { + const entries: ConnectionLogEntry[] = [] + const direct = fakeClient([]) + ;(direct.sendRequest as ReturnType).mockRejectedValue(new Error('LAN down')) + let journal: MobileRelayPairingJournal | null = null + const relay = relayProvisioningClient(() => journal!) + const deps = dependencies(direct, []) + deps.saveJournal.mockImplementation(async (value) => { + journal = value + }) + // Why: stands in for the physical client's own dial line, proving the sink + // actually reaches connectMobileRelayForPairing and not just the candidate. + deps.connectRelay.mockImplementation((connectArgs) => { + connectArgs.onLog?.({ + id: 'relay-dial', + ts: now, + level: 'info', + message: 'Relay: dialing cell', + detail: 'relay-c1.onorca.dev' + }) + return relay + }) + + const attempt = startPreProfilePairing({ + offer: relayOffer, + timeoutMs: 5_000, + connectOptions: { onLog: (entry) => entries.push(entry) }, + dependencies: deps + }) + await expect(attempt.result).resolves.toEqual({ hostId: `host-${now}` }) + + expect(entries.map((entry) => entry.message)).toEqual([ + 'Relay: pairing candidate started', + 'Relay: dialing cell', + 'Pairing path selected' + ]) + expect(entries[0]!.detail).toBe('relay-c1.onorca.dev') + expect(entries[2]).toMatchObject({ level: 'success', detail: 'winner: relay' }) + }) + it('cancels the disposable physical client without publishing a host', async () => { let resolveStatus!: (response: RpcResponse) => void const status = new Promise((resolve) => { diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.ts b/mobile/src/transport/pre-profile-pairing-coordinator.ts index 82bd989be..7518e68e2 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.ts @@ -28,6 +28,8 @@ import { import { racePairingCandidates, type PairingCandidate } from './pairing-candidate-race' import { resolvePairingInviteThroughDirector } from './mobile-relay-invite-director' import { createRecoveringPairingRelayCandidate } from './pairing-relay-candidate' +import { createPairingRelayLogger } from './pairing-relay-log' +import { redactSocketEndpoint } from './socket-event-debug' export type PreProfilePairingAttempt = { readonly result: Promise<{ hostId: string }> @@ -157,14 +159,21 @@ async function runPairing( ) clients.add(directClient) const candidates: PairingCandidate[] = [{ path: 'direct', client: directClient }] + const log = createPairingRelayLogger(connectOptions?.onLog) if (journal) { + log( + 'info', + 'Relay: pairing candidate started', + redactSocketEndpoint(journal.metadata.relay.cellUrl) + ) const relayClient = createRecoveringPairingRelayCandidate({ journal, - connect: (relay) => + connect: (relay, onLog) => dependencies.connectRelay({ relay, deviceToken: offer.deviceToken, - desktopPublicKeyB64: offer.publicKeyB64 + desktopPublicKeyB64: offer.publicKeyB64, + onLog }), resolveDirector: (relay) => dependencies.resolveInviteDirector({ relay }), persistMove: async (relay) => { @@ -181,12 +190,14 @@ async function runPairing( } await dependencies.updateJournal(journal.metadata.journalId, () => journal!.metadata) }, - now: dependencies.now + now: dependencies.now, + onLog: connectOptions?.onLog }) clients.add(relayClient) candidates.push({ path: 'relay', client: relayClient }) } const winner = await racePairingCandidates(candidates) + log('success', 'Pairing path selected', `winner: ${winner.path}`) assertActive(isDisposed) if (!journal) { diff --git a/mobile/src/transport/replacement-session-authentication.ts b/mobile/src/transport/replacement-session-authentication.ts new file mode 100644 index 000000000..0ba54a9d6 --- /dev/null +++ b/mobile/src/transport/replacement-session-authentication.ts @@ -0,0 +1,44 @@ +import type { RpcClient } from './rpc-client' + +// Why: a migration must not cut over to a session that has only opened a socket — the +// replacement has to reach 'connected' (E2EE authenticated) first, and a relay dial can +// sit in handshaking for seconds, so the wait is bounded by the caller's timeout. +export function waitForAuthenticated(session: RpcClient, timeoutMs: number): Promise { + if (session.getState() === 'connected') { + return Promise.resolve() + } + return new Promise((resolve, reject) => { + let settled = false + let unsubscribe: (() => void) | null = null + // Why: armed before subscribing — a synchronous notification during registration + // must find a timer to clear, or a settled wait leaves it running for 12s. + const timer = setTimeout(() => { + finish() + reject(new Error('replacement session authentication timed out')) + }, timeoutMs) + unsubscribe = session.onStateChange((state) => { + if (state === 'connected') { + finish() + resolve() + } else if (state === 'auth-failed' || state === 'disconnected') { + finish() + reject(new Error(`replacement session ${state}`)) + } + }) + if (settled) { + // Why: the notification fired inside onStateChange, before we held the handle. + unsubscribe() + unsubscribe = null + } + + function finish(): void { + if (settled) { + return + } + settled = true + clearTimeout(timer) + unsubscribe?.() + unsubscribe = null + } + }) +} diff --git a/mobile/src/transport/rpc-client.ts b/mobile/src/transport/rpc-client.ts index 07891e84d..9b0afe937 100644 --- a/mobile/src/transport/rpc-client.ts +++ b/mobile/src/transport/rpc-client.ts @@ -3,7 +3,8 @@ import type { RpcSuccess, ConnectionState, ConnectionLogLevel, - ConnectionLogSink + ConnectionLogSink, + ForegroundNudgeReason } from './types' import { generateKeyPair, @@ -105,7 +106,8 @@ export type RpcClient = { getLastConnectedAt: () => number | null onStateChange: (listener: (state: ConnectionState) => void) => () => void // Why: app-resume hook — iOS/Android can kill the TCP path while backgrounded; call on AppState 'active' to recover. - notifyForeground: () => void + // The reason routes relay handling (probe vs replace); the direct socket probes regardless. + notifyForeground: (reason?: ForegroundNudgeReason) => void close: () => void } diff --git a/mobile/src/transport/stable-logical-rpc-client.test.ts b/mobile/src/transport/stable-logical-rpc-client.test.ts index e0b8cb5ee..2171a690b 100644 --- a/mobile/src/transport/stable-logical-rpc-client.test.ts +++ b/mobile/src/transport/stable-logical-rpc-client.test.ts @@ -184,6 +184,134 @@ describe('stable logical RPC client', () => { ) }) + it('publishes the replacement dial phases while the client is suspended', async () => { + const oldSession = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(oldSession, 'relay') + const states: ConnectionState[] = [] + client.onStateChange((next) => states.push(next)) + + client.suspendActiveSession() + expect(client.getPendingPath()).toBeNull() + + const migrating = client.migrateTo(replacement, 'relay') + replacement.setState('connecting') + expect(client.getState()).toBe('connecting') + expect(client.getPendingPath()).toBe('relay') + replacement.setState('handshaking') + expect(client.getState()).toBe('handshaking') + replacement.setState('connected') + await migrating + + expect(states).toEqual(['disconnected', 'connecting', 'handshaking', 'connected']) + expect(client.getPendingPath()).toBeNull() + expect(client.getActivePath()).toBe('relay') + }) + + // A replacement relay session that retries internally publishes 'reconnecting' as one of + // its own dial phases. Forwarding it is the point: amber "Reconnecting…" beats the grey + // the suspended client would otherwise hold for the whole dial. + it('forwards a reconnecting phase published by the dialing session itself', async () => { + const oldSession = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(oldSession, 'relay') + client.suspendActiveSession() + const states: ConnectionState[] = [] + client.onStateChange((next) => states.push(next)) + + const migrating = client.migrateTo(replacement, 'relay') + replacement.setState('connecting') + replacement.setState('reconnecting') + expect(client.getState()).toBe('reconnecting') + replacement.setState('handshaking') + replacement.setState('connected') + await migrating + + expect(states).toEqual(['connecting', 'reconnecting', 'handshaking', 'connected']) + }) + + // The dominant relay case: the direct dial failed, so the client sits in 'reconnecting' + // (never 'disconnected') while its retry loop lives. The dot is already amber there, so + // nothing is forwarded — but the pending path must still name what is being dialed. + it('names a relay dial started from reconnecting without disturbing the state', async () => { + const direct = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(direct, 'lan') + direct.setState('reconnecting') + const states: ConnectionState[] = [] + client.onStateChange((next) => states.push(next)) + + const migrating = client.migrateTo(replacement, 'relay') + expect(client.getPendingPath()).toBe('relay') + replacement.setState('connecting') + replacement.setState('handshaking') + + // The still-bound direct session keeps cycling; the forwarder must not fight it. + expect(client.getState()).toBe('reconnecting') + expect(states).toEqual([]) + + replacement.setState('connected') + await migrating + + expect(states).toEqual(['connected']) + expect(client.getPendingPath()).toBeNull() + expect(client.getActivePath()).toBe('relay') + }) + + it('drops the pending path when the previous session recovers mid-dial', async () => { + const direct = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(direct, 'lan') + direct.setState('reconnecting') + + const migrating = client.migrateTo(replacement, 'relay') + expect(client.getPendingPath()).toBe('relay') + + direct.setState('connected') + expect(client.getPendingPath()).toBeNull() + + replacement.setState('connected') + await migrating + }) + + it('never downgrades a live session while a make-before-break replacement dials', async () => { + const oldSession = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(oldSession, 'relay') + const states: ConnectionState[] = [] + client.onStateChange((next) => states.push(next)) + + const migrating = client.migrateTo(replacement, 'relay') + replacement.setState('connecting') + replacement.setState('handshaking') + + expect(client.getState()).toBe('connected') + expect(client.getPendingPath()).toBeNull() + + replacement.setState('connected') + await migrating + + expect(states).toEqual(['connected']) + }) + + it('restores disconnected when a dial it was narrating fails', async () => { + const session = new FakeSession('connected') + const replacement = new FakeSession('connecting') + const client = createStableLogicalRpcClient(session, 'relay') + client.suspendActiveSession() + const states: ConnectionState[] = [] + client.onStateChange((next) => states.push(next)) + + const migrating = client.migrateTo(replacement, 'relay') + replacement.setState('handshaking') + replacement.setState('disconnected') + + await expect(migrating).rejects.toThrow(/disconnected/) + expect(states).toEqual(['handshaking', 'disconnected']) + expect(client.getState()).toBe('disconnected') + expect(client.getPendingPath()).toBeNull() + }) + it('closes a replacement that fails authentication and preserves the active session', async () => { const oldSession = new FakeSession('connected') const replacement = new FakeSession('connecting') diff --git a/mobile/src/transport/stable-logical-rpc-client.ts b/mobile/src/transport/stable-logical-rpc-client.ts index 1fd19c9c0..9f472c1cd 100644 --- a/mobile/src/transport/stable-logical-rpc-client.ts +++ b/mobile/src/transport/stable-logical-rpc-client.ts @@ -1,5 +1,10 @@ import type { ConnectionState, RpcResponse } from './types' import type { RpcClient } from './rpc-client' +import { + forwardMigrationDialState, + type MigrationDialStateForwarder +} from './migration-dial-state-forwarder' +import { waitForAuthenticated } from './replacement-session-authentication' export type MobileConnectionPath = 'lan' | 'tailscale' | 'relay' @@ -31,9 +36,19 @@ type PendingRequest = { } export type StableLogicalRpcClient = RpcClient & { - migrateTo(session: RpcClient, path: MobileConnectionPath, timeoutMs?: number): Promise + migrateTo( + session: RpcClient, + path: MobileConnectionPath, + timeoutMs?: number, + // Checked after the replacement authenticates, before the swap — lets a racing + // caller withdraw when another path won while this dial was in flight. + shouldAbort?: () => boolean + ): Promise suspendActiveSession(): void getActivePath(): MobileConnectionPath + // Non-null only while a migration dial is publishing its own phases — the path the + // user is waiting on, which the still-bound active path can't name. + getPendingPath(): MobileConnectionPath | null getGeneration(): number } @@ -43,6 +58,7 @@ export function createStableLogicalRpcClient( ): StableLogicalRpcClient { let activeSession = initialSession let activePath = initialPath + let pendingPath: MobileConnectionPath | null = null let generation = 1 let closed = false let suspended = false @@ -138,9 +154,9 @@ export function createStableLogicalRpcClient( stateListeners.add(listener) return () => stateListeners.delete(listener) }, - notifyForeground: () => { + notifyForeground: (reason) => { if (!suspended) { - activeSession.notifyForeground() + activeSession.notifyForeground(reason) } }, close() { @@ -179,21 +195,45 @@ export function createStableLogicalRpcClient( publishState('disconnected') }, - async migrateTo(nextSession, path, timeoutMs = 12_000) { + async migrateTo(nextSession, path, timeoutMs = 12_000, shouldAbort) { if (closed) { nextSession.close() throw new Error('Client closed') } + // Why: naming the dial is independent of narrating it. The dominant relay case + // (direct dial fails) sits in 'reconnecting' — already amber, so forwarding adds + // nothing, but the user still has no idea relay is what's being tried. + if (suspended || state !== 'connected') { + pendingPath = path + } + const forwarder = forwardMigrationDialState({ + session: nextSession, + snapshot: () => ({ state, suspended }), + // Why: close() during the dial already published 'disconnected'; a late + // forwarded phase must not resurrect a closed client's dot. + publish: (next) => { + if (!closed) { + publishState(next) + } + } + }) try { await waitForAuthenticated(nextSession, timeoutMs) + if (closed) { + throw new Error('Client closed') + } + // Why: cutting over anyway would close a live winner and strand the user + // on the slower path (the happy-eyeballs race is first-authenticated-wins). + if (shouldAbort?.()) { + throw new Error('migration superseded') + } } catch (error) { + endDialForwarding(forwarder, true) nextSession.close() throw error } - if (closed) { - nextSession.close() - throw new Error('Client closed') - } + // Why: unbind before bindActiveState so the replacement has exactly one publisher. + endDialForwarding(forwarder, false) const previous = activeSession const previousStateUnsubscribe = activeStateUnsubscribe const nextGeneration = generation + 1 @@ -223,11 +263,24 @@ export function createStableLogicalRpcClient( }, getActivePath: () => activePath, + // Why: a previous session that recovers mid-dial makes the pending path a lie — + // once we're connected the user is no longer waiting on anything. + getPendingPath: () => (state === 'connected' ? null : pendingPath), getGeneration: () => generation } return logical + function endDialForwarding(forwarder: MigrationDialStateForwarder, failed: boolean): void { + forwarder.stop() + pendingPath = null + // Why: only walk back phases we published ourselves — a 'connected' here came from + // the still-live previous session and outranks the dead dial. + if (failed && forwarder.forwarded() && state !== 'connected') { + publishState('disconnected') + } + } + function attachSubscription( record: SubscriptionRecord, session: RpcClient, @@ -263,37 +316,3 @@ export function createStableLogicalRpcClient( } } } - -function waitForAuthenticated(session: RpcClient, timeoutMs: number): Promise { - if (session.getState() === 'connected') { - return Promise.resolve() - } - return new Promise((resolve, reject) => { - let settled = false - let timer: ReturnType | null = null - const unsubscribe = session.onStateChange((state) => { - if (state === 'connected') { - finish() - resolve() - } else if (state === 'auth-failed' || state === 'disconnected') { - finish() - reject(new Error(`replacement session ${state}`)) - } - }) - timer = setTimeout(() => { - finish() - reject(new Error('replacement session authentication timed out')) - }, timeoutMs) - - function finish(): void { - if (settled) { - return - } - settled = true - if (timer) { - clearTimeout(timer) - } - unsubscribe() - } - }) -} diff --git a/mobile/src/transport/types.ts b/mobile/src/transport/types.ts index d031d11ff..400deee90 100644 --- a/mobile/src/transport/types.ts +++ b/mobile/src/transport/types.ts @@ -59,6 +59,10 @@ export type ConnectionState = | 'reconnecting' | 'auth-failed' +// Why: a user-attention nudge must not tear down a healthy relay (probe it); only a +// network-change nudge marks the socket suspect enough to replace it. +export type ForegroundNudgeReason = 'focus' | 'app-resume' | 'network-change' + export type HostProfile = { id: string name: string diff --git a/mobile/src/transport/use-all-host-clients.ts b/mobile/src/transport/use-all-host-clients.ts new file mode 100644 index 000000000..a1e429b45 --- /dev/null +++ b/mobile/src/transport/use-all-host-clients.ts @@ -0,0 +1,56 @@ +import { useEffect, useMemo, useState } from 'react' +import type { RpcClient } from './rpc-client' +import type { MobileConnectionPath } from './stable-logical-rpc-client' +import type { ConnectionState } from './types' +import { useRpcClientContext } from './client-context' + +// Why: refcounting prevents a double-open when a host-detail screen shares one of these hosts. +export function useAllHostClients(hostIds: string[]) { + const ctx = useRpcClientContext() + // Stable key so we don't tear down on every render of the array. + const key = useMemo(() => [...hostIds].sort().join(','), [hostIds]) + const [tick, setTick] = useState(0) + + useEffect(() => { + if (hostIds.length === 0) { + return + } + for (const id of hostIds) { + ctx.acquire(id) + } + const unsubs: (() => void)[] = [] + for (const id of hostIds) { + unsubs.push(ctx.subscribeHostState(id, () => setTick((n) => n + 1))) + } + unsubs.push(ctx.subscribeAllHosts(() => setTick((n) => n + 1))) + return () => { + for (const u of unsubs) { + u() + } + for (const id of hostIds) { + ctx.release(id) + } + } + }, [key]) + + return useMemo(() => { + const out: { + hostId: string + client: RpcClient + state: ConnectionState + path: MobileConnectionPath + }[] = [] + for (const id of hostIds) { + const all = ctx.getAllClients().find((entry) => entry.hostId === id) + if (all) { + out.push({ + hostId: id, + client: all.client, + state: ctx.getState(id), + path: ctx.getActivePath(id) + }) + } + } + return out + }, [key, tick]) +} diff --git a/mobile/src/transport/use-open-mobile-host-edit.ts b/mobile/src/transport/use-open-mobile-host-edit.ts index 68c20c5ae..a6517e3a3 100644 --- a/mobile/src/transport/use-open-mobile-host-edit.ts +++ b/mobile/src/transport/use-open-mobile-host-edit.ts @@ -1,29 +1,14 @@ -import { useCallback, useEffect, useRef } from 'react' -import { useNavigation, useRouter } from 'expo-router' -import { - navigateToMobileHostEdit, - type MobileHostEditNavigationController, - type MobileHostEditRootNavigation -} from './host-edit-navigation' +import { useCallback } from 'react' +import { useOpenHostStackRoute } from '../navigation/use-open-host-stack-route' +import { mobileHostEditRouteTarget } from './host-edit-navigation' export function useOpenMobileHostEdit(): (hostId: string) => void { - const navigation = useNavigation() - const router = useRouter() - const pendingRef = useRef(null) - - useEffect( - () => () => { - pendingRef.current?.cancel() - pendingRef.current = null - }, - [] - ) + const openHostStackRoute = useOpenHostStackRoute() return useCallback( (hostId) => { - pendingRef.current?.cancel() - pendingRef.current = navigateToMobileHostEdit(navigation, router, hostId) + openHostStackRoute(hostId, mobileHostEditRouteTarget(hostId)) }, - [navigation, router] + [openHostStackRoute] ) } diff --git a/mobile/src/worktree/home-host-worktree-fetch.ts b/mobile/src/worktree/home-host-worktree-fetch.ts index 7c6dafe6b..72b9e572b 100644 --- a/mobile/src/worktree/home-host-worktree-fetch.ts +++ b/mobile/src/worktree/home-host-worktree-fetch.ts @@ -47,7 +47,7 @@ export function fetchHomeHostWorktreeInfo( } const result = response.result as { worktrees?: HomeWorktreeSummary[] } const worktrees = result.worktrees ?? [] - setCachedWorktrees(hostId, worktrees) + setCachedWorktrees(hostId, worktrees, { proven: true }) const active = worktrees.filter((w) => w.status && ACTIVE_STATUSES.has(w.status)) // Mirror the desktop's focused workspace (see pickResumeWorktree). const lastActive = pickResumeWorktree(worktrees) diff --git a/mobile/src/worktree/home-resume-card.test.ts b/mobile/src/worktree/home-resume-card.test.ts new file mode 100644 index 000000000..949ef1577 --- /dev/null +++ b/mobile/src/worktree/home-resume-card.test.ts @@ -0,0 +1,175 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it } from 'vitest' +import { + isResumeTargetConfirmedMissing, + selectHomeResumeCard, + type HomeResumeCardInput +} from './home-resume-card' +import type { HomeWorktreeSummary, HostWorktreeInfo } from './home-worktree-info' + +const homeSource = readFileSync(new URL('../../app/index.tsx', import.meta.url), 'utf8') + +function worktree(worktreeId: string): HomeWorktreeSummary { + return { + worktreeId, + repo: 'orca', + branch: 'main', + displayName: worktreeId, + liveTerminalCount: 0 + } +} + +function info(hostId: string, lastActive: HomeWorktreeSummary | null): HostWorktreeInfo { + return { hostId, totalWorktrees: 1, activeCount: 0, lastActiveWorktree: lastActive } +} + +function input(overrides: Partial = {}): HomeResumeCardInput { + return { + hosts: [{ id: 'host-1' }], + hostStates: {}, + worktreeInfo: { 'host-1': info('host-1', worktree('repo::/tmp/wt')) }, + lastVisited: null, + cachedWorktrees: () => null, + ...overrides + } +} + +describe('home resume card', () => { + it('reserves the slot from snapshot data while the host is still connecting', () => { + const connecting = selectHomeResumeCard(input({ hostStates: { 'host-1': 'connecting' } })) + + expect(connecting).toEqual({ + hostId: 'host-1', + worktree: worktree('repo::/tmp/wt'), + actionable: false + }) + }) + + it('keeps the same card in place once the host connects, only enabling it', () => { + const before = selectHomeResumeCard(input({ hostStates: { 'host-1': 'connecting' } })) + const after = selectHomeResumeCard(input({ hostStates: { 'host-1': 'connected' } })) + + // Same host and worktree before and after: the footer's Tasks card cannot shift down. + expect(after?.hostId).toBe(before?.hostId) + expect(after?.worktree.worktreeId).toBe(before?.worktree.worktreeId) + expect(after?.actionable).toBe(true) + }) + + it('leaves Tasks first when no host has resume history at all', () => { + expect( + selectHomeResumeCard(input({ worktreeInfo: { 'host-1': info('host-1', null) } })) + ).toBeNull() + }) + + it('prefers the worktree last opened on this device, enabled with its host', () => { + const visited = worktree('repo::/tmp/visited') + const fromLastVisited = (state: 'connecting' | 'connected') => + selectHomeResumeCard( + input({ + hostStates: { 'host-1': state }, + lastVisited: { hostId: 'host-1', worktreeId: visited.worktreeId }, + cachedWorktrees: (hostId) => (hostId === 'host-1' ? [visited] : null) + }) + ) + + expect(fromLastVisited('connecting')).toEqual({ + hostId: 'host-1', + worktree: visited, + actionable: false + }) + expect(fromLastVisited('connected')?.actionable).toBe(true) + }) + + it('prefers a connected host over an unconnected one holding older snapshot data', () => { + const card = selectHomeResumeCard( + input({ + hosts: [{ id: 'host-1' }, { id: 'host-2' }], + hostStates: { 'host-1': 'connecting', 'host-2': 'connected' }, + worktreeInfo: { + 'host-1': info('host-1', worktree('repo::/tmp/one')), + 'host-2': info('host-2', worktree('repo::/tmp/two')) + } + }) + ) + + expect(card).toEqual({ + hostId: 'host-2', + worktree: worktree('repo::/tmp/two'), + actionable: true + }) + }) + + it('gives a connected host precedence over an offline last-visited worktree', () => { + const visited = worktree('repo::/tmp/visited') + const card = selectHomeResumeCard( + input({ + hosts: [{ id: 'host-1' }, { id: 'host-2' }], + hostStates: { 'host-1': 'disconnected', 'host-2': 'connected' }, + worktreeInfo: { 'host-2': info('host-2', worktree('repo::/tmp/two')) }, + lastVisited: { hostId: 'host-1', worktreeId: visited.worktreeId }, + cachedWorktrees: (hostId) => (hostId === 'host-1' ? [visited] : null) + }) + ) + + // Reserving the slot must not cost the user a card they could actually open. + expect(card).toEqual({ + hostId: 'host-2', + worktree: worktree('repo::/tmp/two'), + actionable: true + }) + }) + + it('renders the home Resume card inert until its host connects', () => { + const start = homeSource.indexOf('{/* ─── Resume card ─── */}') + const end = homeSource.indexOf('{/* ─── Quick actions ─── */}', start) + + // Assert the markers first: a renamed banner would otherwise slice garbage and report a + // missing prop instead of the real cause. + expect(start).toBeGreaterThanOrEqual(0) + expect(end).toBeGreaterThan(start) + + const resumeCard = homeSource.slice(start, end) + expect(resumeCard).toContain('disabled={!resumeCard.actionable}') + expect(resumeCard).toContain('!resumeCard.actionable && styles.cardDisabled') + }) +}) + +// Why (F7): the card is drawn from a snapshot that can name a workspace the desktop deleted +// while the phone was away, and tapping it lands on a session screen whose every RPC fails. +describe('isResumeTargetConfirmedMissing', () => { + const card = { + hostId: 'host-1', + worktree: worktree('repo::/tmp/wt'), + actionable: true + } as const + + it('confirms a target the host listed without', () => { + expect(isResumeTargetConfirmedMissing(card, [{ worktreeId: 'repo::/tmp/other' }])).toBe(true) + }) + + it('clears a target present in the listing', () => { + expect( + isResumeTargetConfirmedMissing(card, [ + { worktreeId: 'repo::/tmp/other' }, + { worktreeId: 'repo::/tmp/wt' } + ]) + ).toBe(false) + }) + + // An unproven catalog is silence, not evidence — the session screen bounces later instead. + it('never confirms without a proven catalog', () => { + expect(isResumeTargetConfirmedMissing(card, null)).toBe(false) + }) + + it('confirms the target when the host proves it has no workspaces at all', () => { + expect(isResumeTargetConfirmedMissing(card, [])).toBe(true) + }) + + it('exempts synthetic routes the catalog can never list', () => { + const folder = { ...card, worktree: worktree('folder:/Users/x/dir') } + const floating = { ...card, worktree: worktree('global-floating-terminal') } + + expect(isResumeTargetConfirmedMissing(folder, [])).toBe(false) + expect(isResumeTargetConfirmedMissing(floating, [])).toBe(false) + }) +}) diff --git a/mobile/src/worktree/home-resume-card.ts b/mobile/src/worktree/home-resume-card.ts new file mode 100644 index 000000000..3a53090af --- /dev/null +++ b/mobile/src/worktree/home-resume-card.ts @@ -0,0 +1,89 @@ +import { isSyntheticWorkspaceRoute } from '../session/synthetic-workspace-route' +import type { ConnectionState } from '../transport/types' +import type { HomeWorktreeSummary, HostWorktreeInfo } from './home-worktree-info' + +// Picks what the home screen's Resume card shows, and whether tapping it can do anything. +// Why the two are separate: gating the card itself on 'connected' made it appear above Tasks +// seconds after first paint, sliding Tasks out from under the user's thumb. A candidate known +// from the persisted snapshot reserves the slot immediately and stays inert until a host connects. + +export type HomeResumeCard = Readonly<{ + hostId: string + worktree: HomeWorktreeSummary + actionable: boolean +}> + +export type HomeResumeCardInput = Readonly<{ + /** Home's sorted hosts — order decides which snapshot wins when several have history. */ + hosts: readonly { id: string }[] + hostStates: Readonly> + worktreeInfo: Readonly> + lastVisited: Readonly<{ hostId: string; worktreeId: string }> | null + cachedWorktrees: (hostId: string) => HomeWorktreeSummary[] | null +}> + +/** The worktree last opened on this device, so Resume reflects mobile session history. */ +function lastVisitedCard({ + lastVisited, + cachedWorktrees, + hostStates +}: HomeResumeCardInput): HomeResumeCard | null { + if (!lastVisited) { + return null + } + const match = cachedWorktrees(lastVisited.hostId)?.find( + (worktree) => worktree.worktreeId === lastVisited.worktreeId + ) + if (!match) { + return null + } + return { + hostId: lastVisited.hostId, + worktree: match, + actionable: hostStates[lastVisited.hostId] === 'connected' + } +} + +function hostHistoryCard( + { hosts, hostStates, worktreeInfo }: HomeResumeCardInput, + connectedOnly: boolean +): HomeResumeCard | null { + for (const host of hosts) { + const worktree = worktreeInfo[host.id]?.lastActiveWorktree + if (!worktree) { + continue + } + const actionable = hostStates[host.id] === 'connected' + if (actionable || !connectedOnly) { + return { hostId: host.id, worktree, actionable } + } + } + return null +} + +/** Whether tapping this card would open a workspace the host has already listed without. + * Deliberately one-directional: `provenWorktrees` is null whenever the catalog is a cold-start + * snapshot or still loading, and an unproven catalog is not evidence of a deletion — that case + * navigates as before and the session screen bounces once the host answers. */ +export function isResumeTargetConfirmedMissing( + card: HomeResumeCard, + provenWorktrees: readonly { worktreeId: string }[] | null +): boolean { + if (!provenWorktrees || isSyntheticWorkspaceRoute(card.worktree.worktreeId)) { + return false + } + return !provenWorktrees.some((worktree) => worktree.worktreeId === card.worktree.worktreeId) +} + +export function selectHomeResumeCard(input: HomeResumeCardInput): HomeResumeCard | null { + const visited = lastVisitedCard(input) + if (visited?.actionable) { + return visited + } + const connected = hostHistoryCard(input, true) + if (connected) { + return connected + } + // Nothing live to resume yet: hold the slot with whatever the snapshot remembers. + return visited ?? hostHistoryCard(input, false) +} diff --git a/mobile/src/worktree/host-workspace-list-state.ts b/mobile/src/worktree/host-workspace-list-state.ts index c14b5859e..6bd47ca28 100644 --- a/mobile/src/worktree/host-workspace-list-state.ts +++ b/mobile/src/worktree/host-workspace-list-state.ts @@ -14,9 +14,15 @@ export function selectHostWorkspaceListState( input: HostWorkspaceListStateInput ): 'loading' | 'catalog-error' | 'empty' | null { const { connState, worktreesLoaded, displayCount, sectionCount, catalogError } = input - const connecting = connState === 'connecting' || connState === 'reconnecting' + // Why: a blank disconnected list read as "no workspaces"; spin instead — the + // header verdict owns escalating a long outage. auth-failed keeps its own UI. + const pending = + connState === 'connecting' || + connState === 'handshaking' || + connState === 'reconnecting' || + connState === 'disconnected' if ( - (connecting && displayCount === 0) || + (pending && displayCount === 0) || (connState === 'connected' && !worktreesLoaded && displayCount === 0 && !catalogError) ) { return 'loading' diff --git a/mobile/src/worktree/last-visited-worktree-repo.test.ts b/mobile/src/worktree/last-visited-worktree-repo.test.ts index 4d484fbe7..eebbf8775 100644 --- a/mobile/src/worktree/last-visited-worktree-repo.test.ts +++ b/mobile/src/worktree/last-visited-worktree-repo.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from 'vitest' -import { readLastVisitedWorktreeRepoId } from './last-visited-worktree-repo' +import { + readLastVisitedWorktreeRecord, + readLastVisitedWorktreeRepoId +} from './last-visited-worktree-repo' describe('last visited worktree repo', () => { it('extracts the repo id for the current host', () => { @@ -19,3 +22,36 @@ describe('last visited worktree repo', () => { expect(readLastVisitedWorktreeRepoId(JSON.stringify({ hostId: 'host-1' }), 'host-1')).toBeNull() }) }) + +// Why (F7): home's Resume card navigates off this record, so anything it accepts becomes a route. +describe('readLastVisitedWorktreeRecord', () => { + it('reads a well-formed record', () => { + const raw = JSON.stringify({ hostId: 'host-1', worktreeId: 'repo-2::/tmp/worktree' }) + + expect(readLastVisitedWorktreeRecord(raw)).toEqual({ + hostId: 'host-1', + worktreeId: 'repo-2::/tmp/worktree' + }) + }) + + it('reads absent, truncated, and wrong-shaped payloads as no history', () => { + expect(readLastVisitedWorktreeRecord(null)).toBeNull() + expect(readLastVisitedWorktreeRecord('')).toBeNull() + expect(readLastVisitedWorktreeRecord('{"hostId":"host-1"')).toBeNull() + expect(readLastVisitedWorktreeRecord('null')).toBeNull() + expect(readLastVisitedWorktreeRecord('"a string"')).toBeNull() + expect(readLastVisitedWorktreeRecord(JSON.stringify({ hostId: 'host-1' }))).toBeNull() + expect( + readLastVisitedWorktreeRecord(JSON.stringify({ hostId: 'host-1', worktreeId: 42 })) + ).toBeNull() + }) + + it('rejects empty ids that would build a route to nowhere', () => { + expect( + readLastVisitedWorktreeRecord(JSON.stringify({ hostId: '', worktreeId: 'repo::/wt' })) + ).toBeNull() + expect( + readLastVisitedWorktreeRecord(JSON.stringify({ hostId: 'host-1', worktreeId: '' })) + ).toBeNull() + }) +}) diff --git a/mobile/src/worktree/last-visited-worktree-repo.ts b/mobile/src/worktree/last-visited-worktree-repo.ts index 4ac9349c9..59e1a0244 100644 --- a/mobile/src/worktree/last-visited-worktree-repo.ts +++ b/mobile/src/worktree/last-visited-worktree-repo.ts @@ -2,7 +2,7 @@ import { getRepoIdFromMobileWorktreeId } from '../session/mobile-session-route-h export const LAST_VISITED_WORKTREE_STORAGE_KEY = 'orca:last-visited-worktree' -type LastVisitedWorktreeRecord = { +export type LastVisitedWorktreeRecord = { hostId: string worktreeId: string } @@ -11,7 +11,12 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null } -function readLastVisitedWorktreeRecord(raw: string | null): LastVisitedWorktreeRecord | null { +/** Why exported: home drives its Resume card — and therefore a navigation — off this record, + * so a truncated or older-shaped payload must read as "no history" rather than reach the + * router as a half-built route. */ +export function readLastVisitedWorktreeRecord( + raw: string | null +): LastVisitedWorktreeRecord | null { if (!raw) { return null } @@ -20,7 +25,10 @@ function readLastVisitedWorktreeRecord(raw: string | null): LastVisitedWorktreeR if ( !isRecord(parsed) || typeof parsed.hostId !== 'string' || - typeof parsed.worktreeId !== 'string' + typeof parsed.worktreeId !== 'string' || + // An empty id builds a route to nowhere, so it is history we cannot act on. + parsed.hostId === '' || + parsed.worktreeId === '' ) { return null } diff --git a/mobile/src/worktree/worktree-show-resolution.test.ts b/mobile/src/worktree/worktree-show-resolution.test.ts new file mode 100644 index 000000000..c2357b90d --- /dev/null +++ b/mobile/src/worktree/worktree-show-resolution.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { classifyWorktreeShowResponse } from './worktree-show-resolution' +import type { RpcResponse } from '../transport/types' + +const meta = { runtimeId: 'runtime-1' } + +function failure(code: string, message: string): RpcResponse { + return { id: '1', ok: false, error: { code, message }, _meta: meta } +} + +describe('classifyWorktreeShowResponse', () => { + it('reads a successful show as present', () => { + expect( + classifyWorktreeShowResponse({ id: '1', ok: true, result: { worktree: {} }, _meta: meta }) + ).toBe('present') + }) + + it('reads the structured not-found code as missing', () => { + expect(classifyWorktreeShowResponse(failure('selector_not_found', 'Selector not found'))).toBe( + 'missing' + ) + }) + + it('reads an older desktop runtime_error carrying the bare token as missing', () => { + expect(classifyWorktreeShowResponse(failure('runtime_error', 'selector_not_found'))).toBe( + 'missing' + ) + }) + + it('reads a wrapped token after a message boundary as missing', () => { + expect( + classifyWorktreeShowResponse( + failure('runtime_error', "Error invoking remote method 'worktree.show': selector_not_found") + ) + ).toBe('missing') + }) + + // The bounce this feeds is destructive, so anything short of a definite answer must not trigger it. + it('leaves transient and ambiguous failures unknown', () => { + expect(classifyWorktreeShowResponse(failure('selector_ambiguous', 'Ambiguous'))).toBe('unknown') + expect(classifyWorktreeShowResponse(failure('method_not_found', 'Unknown method'))).toBe( + 'unknown' + ) + expect(classifyWorktreeShowResponse(failure('runtime_busy', 'Runtime busy'))).toBe('unknown') + }) + + it('does not read prose that merely mentions the token as missing', () => { + expect( + classifyWorktreeShowResponse( + failure('runtime_error', 'Access denied after a prior selector_not_found') + ) + ).toBe('unknown') + expect(classifyWorktreeShowResponse(failure('runtime_error', 'stale_selector_not_found'))).toBe( + 'unknown' + ) + }) +}) diff --git a/mobile/src/worktree/worktree-show-resolution.ts b/mobile/src/worktree/worktree-show-resolution.ts new file mode 100644 index 000000000..aff9394d4 --- /dev/null +++ b/mobile/src/worktree/worktree-show-resolution.ts @@ -0,0 +1,38 @@ +import type { RpcResponse } from '../transport/types' + +// What a `worktree.show` answer proves about the target still existing on the host. +// 'unknown' is the safe verdict: every transient failure lands there, so a caller may +// only act destructively (bounce the route) on 'missing'. +export type WorktreeShowResolution = 'present' | 'missing' | 'unknown' + +const NOT_FOUND_CODE = 'selector_not_found' + +// Why: older desktops predate the passthrough allowlist and answer a missing selector as +// runtime_error carrying the token as its whole message, so the code alone under-detects. +// The token must end the message after a real boundary — prose that merely trails off in +// it ("…a prior selector_not_found diagnostic") is not a not-found answer. +const CODE_TOKEN_BOUNDARY = /(?:: |\n)[ \t]*$/ + +function endsWithNotFoundToken(message: string): boolean { + const trimmed = message.trimEnd() + if (!trimmed.endsWith(NOT_FOUND_CODE)) { + return false + } + const prefix = trimmed.slice(0, -NOT_FOUND_CODE.length) + return prefix.trim() === '' || CODE_TOKEN_BOUNDARY.test(prefix) +} + +export function classifyWorktreeShowResponse(response: RpcResponse): WorktreeShowResolution { + if (response.ok) { + return 'present' + } + if (response.error.code === NOT_FOUND_CODE) { + return 'missing' + } + // Why: the wrapped-token form only ever ships as runtime_error; any other code + // whose message trails off in the token proves nothing about the worktree. + if (response.error.code !== 'runtime_error') { + return 'unknown' + } + return endsWithNotFoundToken(response.error.message) ? 'missing' : 'unknown' +}