diff --git a/LANE-REPORT.md b/LANE-REPORT.md new file mode 100644 index 00000000000..02147e6a318 --- /dev/null +++ b/LANE-REPORT.md @@ -0,0 +1,302 @@ +# Lane 1 — Worktree-create flow: sheet never returns + slow picker + +Branch: `brennanb2025/mob-create-flow-perf` (off `origin/main` @ `fecdf0bde8`) +Worktree: `/Users/brennanbenson/orca/workspaces/orca/mob-create-flow-perf` +Simulator used: **iPhone 17 Pro Max** (`CCA8A76F-77D1-4461-8318-63C7ED69D28E`) only. + +--- + +## What was wrong, in one sentence each + +**Bug A — the form sheet never comes back after a source is picked.** +`BottomDrawerModalHost` keeps a single full-screen native `Modal` mounted for the whole create +flow, and `resolveNewWorktreeFormSheetVisible` rendered **no sheet at all** during the +`transition` state unless a render-read ref happened to say the form was pinned — so any beat +where the drawer swap does not complete leaves a transparent, tap-swallowing window with nothing +in it, and there was no recovery path of any kind; on device the failure lands one step further +in, where the create form's `MountedBottomDrawer` is left pinned at `progress = 1` and nothing +ever re-applies its enter transform when the fill picker hands the window back, so the sheet is +laid out but never painted. + +**Bug B — the picked source is lost.** +`NewWorktreeModal` bumped its remount key on the **identity of the `RpcClient` object** +(`clientEpochRef`), and `useHostClient` swaps that object for the *same host* on every reconnect, +`forceReconnect`, and foreground revival — so an ordinary network blip silently remounted the +live form and threw the user's picked PR away, with no user action involved. + +**Bug C — the picker is slow.** +`runSmartSearch` in `use-smart-workspace-source.ts` `await`ed the provider fan-out and *then* +`await`ed the pasted-item lookup, stacking two full host round trips on the single most common +path (typing a PR number). Measured on device: **2631 ms → 1480 ms** for the same query. + +--- + +## Evidence + +### 1. Frame forensics on the supplied recording + +`recording.mov`, re-extracted with **output seeking** (`ffmpeg -i FILE -ss N -to M`) at 20 fps and +measured with `signalstats` luma over fixed crops. All numbers are mean luma of the same pixel +band, so they are directly comparable: + +| t (s) | header band | sheet band | state | +|---|---|---|---| +| 6.95 | 22.88 | 45.17 | source picker open over the pinned form | +| 7.00 | 23.64 | 49.39 | selection applied | +| **7.05** | **29.60** | 32.98 | **no backdrop, no sheet** — bare list, host `Modal` still mounted | +| **7.10** | **29.62** | 31.65 | same | +| **7.15 → 25.0** | **17.36** | 18.11 | **full-opacity backdrop, no sheet** — 18.3 s of dead screen | +| 25.5 → 30.0 | 29.69 | — | modal fully closed (a tap finally landed on the backdrop) | +| 31.0 | 17.36 | — | modal reopened, **Name field empty** | + +Two things fall straight out of that table: + +* 7.05–7.10 is the `transition` beat with nothing rendered — 29.6 is exactly the undimmed value, + i.e. **no drawer at all**, while the host `Modal` is still mounted and eating taps. +* From 7.15 the dim is **17.36**, byte-identical to 17.36 at t=31.0 when the form sheet is + legitimately open. Same backdrop, same opacity — but a band-by-band ratio sweep of the whole + frame (dead vs. undimmed, every 100 px from y=0 to y=1500) returns a flat **0.52–0.61** + everywhere including the very bottom. There is no sheet anywhere on screen. Crop of the bottom + 280 px is in `lane1-evidence/06-user-video-dead-frame-bottom-crop.png`. + +So the 18 s is not a slow transition and not an async wait: the modal reached a state where the +backdrop paints and the sheet does not, and **nothing in the code can get out of it**. It ended +only because one of the user's taps hit the backdrop, and the form sheet's backdrop `dismiss()` +closes the *whole* modal — which is why the sheet came back empty at t=31 s. **Bug B as seen in +the video is a consequence of Bug A**; Bug B also has its own independent cause (below). + +### 2. Live reproduction on the simulator + +Rig: isolated dev host from this worktree +(`ORCA_DEV_USER_DATA_PATH=/tmp/lane1-orca-userdata ORCA_DEV_INSTANCE_KEY=lane1 pnpm dev`, proven +isolated with `repos.list() → []`), one repo added, LAN pairing deep link, Metro started with +`./node_modules/.bin/expo start --host lan --port 8091` (never bare `pnpm`), all UI interaction +through `orca emulator tap` / `type` on iPhone 17 Pro Max. + +Sequence: `+` FAB → Create worktree → tap the source field → tap a GitHub PR row. + +* `lane1-evidence/01-source-picker-open.png` — picker open with real PRs. +* `lane1-evidence/02-REPRO-dead-screen-after-pr-select.png` — **reproduced**: dimmed screen, no + sheet. It stayed that way for **77 s** of polling (not 18 s — it is permanent until the user + taps the backdrop). + +Instrumenting `MountedBottomDrawer` with `onLayout` and a worklet log then proved the sheet is +*there* and correctly positioned: + +``` +[DBG] drawer layout z=1000 {"x":0,"y":464.67,"width":440,"height":491.33} +[DBG] worklet z=1000 p=1 ty=0 kb=0 sh=956 +``` + +`progress = 1`, `translateY = 0`, layout `y=464.67, h=491.33` on a 956 pt screen — a perfectly +placed, on-screen sheet that is not painted. Temporarily tinting `styles.drawer` red made the +whole sheet appear **still holding the picked PR** — +`lane1-evidence/03-tinted-proof-sheet-laid-out-but-unpainted.png`. Any re-render restored it. + +That is the fingerprint of a stale native transform on a view that was rebuilt underneath +Reanimated: the pinned sheet holds `progress` at 1 for the entire time the fill picker owns the +window, so when the picker leaves, **nothing re-applies the enter transform**. On the runs where +the native view survived intact it looks fine; on the runs where it did not, the sheet is gone +for good. The reproduction is intermittent — 1 in ~12 attempts on this rig with a one-repo host; +the user hit it on the first try on a host with hundreds of worktrees. + +### 3. Timeline / drawer-state trace at the moment of the swap + +From the instrumented run (`z=1000` is the form sheet, `z=1100` the fill picker): + +``` +transitionDrawer -> form pinned= true +nav render drawerView= transition pinned= true +drawer render visible= true interactive= false z=1000 <- form pinned, backdrop opacity 0 +drawer render visible= false interactive= true z=1100 <- picker leaving +transition TIMER fired -> form +nav render drawerView= form pinned= false +drawer render visible= true interactive= true z=1000 <- form takes the window back +``` + +Note the last line: `interactive` flips `false → true` and **no effect re-runs** — `visible` never +changed, so `MountedBottomDrawer`'s only enter-animation effect (deps `[onHidden, visible]`) does +not fire. That is the hand-back with nothing re-asserted. + +--- + +## Fixes + +### A1 — `mobile/src/components/mounted-bottom-drawer.tsx` + +Re-assert the enter transform whenever a *visible* sheet takes the window back from a pinned +state (`interactive` false → true). This is the exact transition the trace above shows going +unhandled, and it makes the sheet self-heal instead of staying invisible forever. + +### A2 — `mobile/src/components/new-worktree-form-sheet-visibility.ts` + `use-new-worktree-drawer-navigation.ts` + +The create form is now the modal's floor: it stays mounted and visible through **every** +`transition`, not only the source-picker one. This deletes the "mounted host `Modal`, zero +sheets" state entirely, so a dropped or delayed transition timer degrades to "the picker did not +open" rather than "the app is dead". As a direct consequence the `formPinnedUnderSourceRef` — +a ref *read during render*, i.e. state that can go stale without scheduling a render — is no +longer needed and is deleted. The hook is 12 lines shorter. + +### B — `mobile/src/components/NewWorktreeModal.tsx` + +The form session is keyed on `hostId`, never on the `RpcClient` object. Every client-scoped hook +under the modal (`useNewWorkspaceRepositories`, `useSmartWorkspaceSource`, `useMobileComposerSource` +via `resolveTokenRef`, …) already drops responses from a superseded client, so the remount bought +nothing and cost the user their form. Switching hosts still starts a fresh session — and now +actually does, which it did **not** before: pre-fix, changing `hostId` while the client object +stayed the same (e.g. both `null`) left the previous host's form state in place. + +### C — `mobile/src/tasks/use-smart-workspace-source.ts` + +The pasted-item lookup and the provider fan-out hit different host endpoints and are now issued +together with `Promise.all` instead of one after the other. The paste branch is extracted into +`resolvePastedItem` so `runSmartSearch` stays readable. + +--- + +## Regression tests — each one fails against the unfixed code + +All runs below are `cd mobile && npx vitest run ` with the fix reverted, then restored. + +### A2 — `mobile/src/components/use-new-worktree-drawer-navigation.test.ts` + +``` + FAIL src/components/use-new-worktree-drawer-navigation.test.ts > useNewWorktreeDrawerNavigation + > keeps a sheet on screen while swapping to a content-sized picker +AssertionError: expected false to be true // Object.is equality + +- Expected ++ Received + +- true ++ false + Tests 1 failed | 2 passed (3) +``` + +(The companion `new-worktree-form-sheet-visibility.test.ts` case +`never leaves the mounted modal without a sheet during a drawer swap` also fails pre-fix — +`expected undefined to be true`.) + +### A1 — `mobile/src/components/bottom-drawer-window-handback.test.ts` + +``` + FAIL src/components/bottom-drawer-window-handback.test.ts > bottom drawer window hand-back + > re-asserts the enter transform when a pinned sheet takes the window back +AssertionError: expected 1 to be 2 // Object.is equality + +- Expected ++ Received + +- 2 ++ 1 + Tests 1 failed | 1 passed (2) +``` + +The second case in that file (`does not re-assert while the sheet stays pinned`) passes both +before and after on purpose — it is the guard that keeps the fix from firing on every render. + +### B — `mobile/src/components/NewWorktreeModal.test.tsx` + +``` + × ignores a stale repo list after the client changes + × keeps the picked source when a reconnect swaps the client for the same host + × starts a fresh form session when the modal switches hosts + +AssertionError: expected [ '', '' ] to deeply equal [ 'stale-client-name', …(1) ] +AssertionError: expected [ '', '' ] to deeply equal [ 'feat/keep-me', 'feat/keep-me' ] +AssertionError: expected [ 'host-one-name', 'host-one-name' ] to deeply equal [ '', '' ] + Tests 3 failed | 6 passed (9) +``` + +The first of those is the pre-existing test; its incidental `['', '']` assertion encoded the +remount-on-client-swap behaviour and is now flipped to assert the form survives, with a comment +pointing at the new reconnect test. + +### C — `mobile/src/tasks/smart-source-paste-concurrency.test.ts` + +``` + FAIL src/tasks/smart-source-paste-concurrency.test.ts > smart source paste lookup concurrency + > issues the pasted-number lookup while the fan-out is still in flight +AssertionError: expected [ 'github.listWorkItems', …(1) ] to include 'github.workItem' + Tests 1 failed (1) +``` + +Nothing in the stub ever settles, so the lookup can only be observed if it was issued +concurrently — the assertion cannot pass by accident. + +--- + +## Bug C — measured before / after + +Same device, same host, same repo, same query (`16831`), measured in the running app with a +temporary `console.log` around `runSmartSearch` (removed again before commit): + +``` +BEFORE [PERF] smartSearch q=16831 fanMs=739 pasteMs=1892 totalMs=2631 +AFTER [PERF] smartSearch q=16831 totalMs=1480 +AFTER [PERF] smartSearch q=16831 totalMs=1515 +``` + +**2631 ms → 1480 ms / 1515 ms: ≈1.13 s saved, a 43 % reduction** on the type-a-PR-number path. +The arithmetic is self-consistent — before is exactly `fan + paste` (739 + 1892), after is +`max(fan, paste)`. Only one "before" sample was captured before the fix went in; the two "after" +samples bracket it. On a host with a large repo the fan-out leg is the 4–5 s the brief describes, +so the absolute saving there is larger, not smaller. + +What this does **not** fix: the host-side `gh` round trip itself. `github.listWorkItems` is 0.5–1.5 s +here and several seconds on a busy repo, and that is host work. The picker already keeps the +previous rows rendered through the 200 ms debounce and shows a footer spinner, so it does not +blank out; making it faster than the host is out of this lane's reach. + +--- + +## Verification + +| Check | Result | +|---|---| +| `cd mobile && npx tsc --noEmit` | clean | +| `cd mobile && npx vitest run src` | **471 files, 3774 passed, 3 skipped, 0 failed** | +| changed-code quality gate | `code quality: 0` / `type-aware code quality: 0` / `React Doctor: 0` — **passed since fecdf0bde885** | +| `oxfmt` | run scoped to the 10 changed files only | +| `mobile/pnpm-lock.yaml` | untouched; `grep -c patchedDependencies` → `1` | + +`pnpm run check:code-quality:changed` crashes on this machine's Node 26 (the pnpm +`Unsupported engine` warning lands on stdout and `parseOxlintOutput` parses it). Ran as +`npm_config_loglevel=error node config/scripts/check-changed-code-quality.mjs`, which is the same +script with the warning silenced — output quoted above. + +--- + +## Emulator QA + +Done on **iPhone 17 Pro Max only**; iPhone 17 Pro and iPhone 17 were never touched. All UI +interaction went through `orca emulator tap` / `orca emulator type`; no Computer Use clicks, +keystrokes, scrolls, or focus changes were performed at any point. + +* `lane1-evidence/04-fixed-source-picker-open.png` — picker open on the fixed build. +* `lane1-evidence/05-fixed-form-returns-with-pr.png` — **the form sheet returns holding + PR #16836**, measured back on screen within 1 s of the tap (sheet-band luma 37.07 at t+1 s and + t+5 s, versus 8.78 in the dead state). + +One deviation from the skill worth flagging: `orca computer list-windows --app +com.apple.iphonesimulator` returns `windows: []` for this device (it was booted headless via +`simctl boot`), so there was no Simulator window to capture. The screenshots above are +device-pixel frames pulled from the Orca emulator helper's own MJPEG stream +(`http://127.0.0.1:3101/stream.mjpeg`) — the app itself, at native resolution, with no desktop +window and no focus stealing. I did not fall back to any other interaction method. + +--- + +## Honest limits + +* **The native trigger for A is not pinned down to a single line.** What is proven is: the sheet + reaches a laid-out-but-unpainted state (`progress = 1`, `translateY = 0`, correct frame), it is + reachable from the shipped source-picker flow, and the code has no way back out of it. Fix A1 + addresses the hand-back that the trace shows going unhandled; fix A2 removes the + no-sheet-at-all state that made it unrecoverable. If the underlying rebuild has another + trigger, A2 still guarantees the user is never stranded on a dead screen. +* The repro is intermittent (~1 in 12 on a one-repo isolated host). I could not make it fire on + demand, so "fixed" is supported by the mechanism plus a clean post-fix run, not by a + before/after on a deterministic repro. +* The `-ss`-before-`-i` trap in the brief is real; every frame timestamp here used output + seeking. diff --git a/mobile/src/components/NewWorktreeModal.test.tsx b/mobile/src/components/NewWorktreeModal.test.tsx index 0bbdb462d4c..8c48f35261c 100644 --- a/mobile/src/components/NewWorktreeModal.test.tsx +++ b/mobile/src/components/NewWorktreeModal.test.tsx @@ -348,7 +348,11 @@ describe('NewWorktreeModal project targets', () => { renderer.update(createElement(NewWorktreeModal, { ...modalProps, client: freshClient })) await Promise.resolve() }) - expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual(['', '']) + // The swap must not restart the form session — see the reconnect test below. + expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual([ + 'stale-client-name', + 'stale-client-name' + ]) resolveOldList?.({ ok: true, result: { repos } }) await flushUpdates() @@ -377,6 +381,64 @@ describe('NewWorktreeModal project targets', () => { expect(repoListCalls).toHaveLength(2) }) + // A reconnect / forceReconnect / foreground revival hands the same host a NEW + // RpcClient object (see useHostClient). Keying the form session on that object + // remounted the modal mid-edit and silently threw away the picked source. + it('keeps the picked source when a reconnect swaps the client for the same host', async () => { + const makeClient = () => + ({ + sendRequest: vi.fn().mockImplementation((method: string) => { + if (method === 'repo.list') { + return Promise.resolve({ ok: true, result: { repos } }) + } + if (method === 'status.get') { + return Promise.resolve({ ok: true, result: { hostPlatform: 'darwin' } }) + } + return new Promise(() => {}) + }) + }) as unknown as RpcClient + const modalProps = { + visible: true, + hostId: 'host-1', + onCreated: () => {}, + onClose: () => {} + } + + await act(async () => { + renderer = create(createElement(NewWorktreeModal, { ...modalProps, client: makeClient() })) + }) + act(() => sourceInputs(renderer)[0]!.props.onChangeText('feat/keep-me')) + + await act(async () => { + renderer.update(createElement(NewWorktreeModal, { ...modalProps, client: makeClient() })) + await Promise.resolve() + }) + + expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual([ + 'feat/keep-me', + 'feat/keep-me' + ]) + }) + + it('starts a fresh form session when the modal switches hosts', async () => { + setCachedRepos('host-2', repos) + const client = { + sendRequest: vi.fn().mockImplementation(() => new Promise(() => {})) + } as unknown as RpcClient + const modalProps = { visible: true, client, onCreated: () => {}, onClose: () => {} } + + await act(async () => { + renderer = create(createElement(NewWorktreeModal, { ...modalProps, hostId: 'host-1' })) + }) + act(() => sourceInputs(renderer)[0]!.props.onChangeText('host-one-name')) + + await act(async () => { + renderer.update(createElement(NewWorktreeModal, { ...modalProps, hostId: 'host-2' })) + }) + + expect(sourceInputs(renderer).map((input) => input.props.value)).toEqual(['', '']) + }) + it('starts with fresh form state after closing and reopening', async () => { const client = { sendRequest: vi.fn().mockImplementation(() => new Promise(() => {})) diff --git a/mobile/src/components/NewWorktreeModal.tsx b/mobile/src/components/NewWorktreeModal.tsx index 92bac7d59bb..7bbda7dd924 100644 --- a/mobile/src/components/NewWorktreeModal.tsx +++ b/mobile/src/components/NewWorktreeModal.tsx @@ -35,7 +35,7 @@ import { useNewWorktreeDrawerNavigation } from './use-new-worktree-drawer-naviga export function NewWorktreeModal(props: NewWorktreeModalProps) { const openEpochRef = useRef(0) const wasVisibleRef = useRef(false) - const clientEpochRef = useRef({ client: props.client, epoch: 0 }) + const hostEpochRef = useRef({ hostId: props.hostId, epoch: 0 }) // Why: each drawer opening is a fresh form session; remounting resets local // form state before paint instead of clearing it in a visible-prop Effect. @@ -43,13 +43,18 @@ export function NewWorktreeModal(props: NewWorktreeModalProps) { openEpochRef.current += 1 } wasVisibleRef.current = props.visible - if (clientEpochRef.current.client !== props.client) { - clientEpochRef.current = { client: props.client, epoch: clientEpochRef.current.epoch + 1 } + // Why: key the session on the HOST, never on the RpcClient object. A reconnect, + // forceReconnect, or foreground revival swaps that object for the same host + // (see useHostClient), and keying on it silently remounted this form mid-edit + // and threw away the picked source. Every client-scoped hook below already + // drops responses from a superseded client, so no remount is needed for that. + if (hostEpochRef.current.hostId !== props.hostId) { + hostEpochRef.current = { hostId: props.hostId, epoch: hostEpochRef.current.epoch + 1 } } return ( ) diff --git a/mobile/src/components/bottom-drawer-window-handback.test.ts b/mobile/src/components/bottom-drawer-window-handback.test.ts new file mode 100644 index 00000000000..2a5ee36d39b --- /dev/null +++ b/mobile/src/components/bottom-drawer-window-handback.test.ts @@ -0,0 +1,140 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const withTimingCalls = vi.hoisted(() => [] as { to: number; duration: number | undefined }[]) +const sharedWrites = vi.hoisted(() => [] as { key: string; value: unknown }[]) + +vi.mock('react-native', () => ({ + BackHandler: { addEventListener: () => ({ remove: () => {} }) }, + Keyboard: { + addListener: () => ({ remove: () => {} }), + dismiss: () => {}, + metrics: () => null + }, + Modal: 'Modal', + Platform: { OS: 'ios', select: (options: { ios?: unknown }) => options.ios }, + Pressable: 'Pressable', + ScrollView: 'ScrollView', + StyleSheet: { + create: (styles: T) => styles, + absoluteFillObject: {} + }, + View: 'View', + useWindowDimensions: () => ({ width: 440, height: 956 }) +})) +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => ({ top: 62, bottom: 34, left: 0, right: 0 }) +})) +vi.mock('react-native-gesture-handler', () => { + const chain: Record = {} + for (const method of [ + 'activeOffsetY', + 'simultaneousWithExternalGesture', + 'onBegin', + 'onUpdate', + 'onEnd' + ]) { + chain[method] = () => chain + } + return { + Gesture: { Pan: () => chain, Native: () => chain }, + GestureDetector: 'GestureDetector', + GestureHandlerRootView: 'GestureHandlerRootView' + } +}) +vi.mock('react-native-reanimated', () => { + function makeShared(key: string, initial: number) { + let value = initial + return { + get value() { + return value + }, + set value(next: number) { + value = next + sharedWrites.push({ key, value: next }) + } + } + } + let sharedIndex = 0 + return { + default: { View: 'AnimatedView', ScrollView: 'AnimatedScrollView' }, + useSharedValue: (initial: number) => makeShared(`shared-${sharedIndex++}`, initial), + useAnimatedStyle: () => ({}), + useAnimatedScrollHandler: () => () => {}, + withSpring: (to: number) => to, + withTiming: (to: number, config?: { duration?: number }) => { + withTimingCalls.push({ to, duration: config?.duration }) + return to + }, + runOnJS: (fn: () => void) => fn, + interpolate: () => 0, + Extrapolation: { CLAMP: 'clamp' } + } +}) + +import { MountedBottomDrawer } from './mounted-bottom-drawer' + +const noop = () => {} + +function render(interactive: boolean): ReactTestRenderer { + let renderer: ReactTestRenderer | null = null + act(() => { + renderer = create( + createElement( + MountedBottomDrawer, + { visible: true, interactive, onClose: noop, onHidden: noop }, + createElement('SheetBody') + ) + ) + }) + if (!renderer) { + throw new Error('drawer did not render') + } + return renderer +} + +function update(renderer: ReactTestRenderer, interactive: boolean): void { + act(() => { + renderer.update( + createElement( + MountedBottomDrawer, + { visible: true, interactive, onClose: noop, onHidden: noop }, + createElement('SheetBody') + ) + ) + }) +} + +// A sheet pinned under a fill picker keeps progress at 1 the whole time, so +// nothing re-applies its enter transform when the picker gives the window back. +// On device that left the create form laid out but unpainted — a dimmed screen +// with no sheet and no way back except dismissing the whole modal. +describe('bottom drawer window hand-back', () => { + afterEach(() => { + withTimingCalls.length = 0 + sharedWrites.length = 0 + }) + + it('re-asserts the enter transform when a pinned sheet takes the window back', () => { + const renderer = render(true) + update(renderer, false) + const beforeHandback = withTimingCalls.filter((call) => call.to === 1).length + + update(renderer, true) + + expect(withTimingCalls.filter((call) => call.to === 1).length).toBe(beforeHandback + 1) + act(() => renderer.unmount()) + }) + + it('does not re-assert while the sheet stays pinned', () => { + const renderer = render(true) + update(renderer, false) + const pinned = withTimingCalls.filter((call) => call.to === 1).length + + update(renderer, false) + + expect(withTimingCalls.filter((call) => call.to === 1).length).toBe(pinned) + act(() => renderer.unmount()) + }) +}) diff --git a/mobile/src/components/mounted-bottom-drawer.tsx b/mobile/src/components/mounted-bottom-drawer.tsx index 3331d7298d8..099dfc8d6cb 100644 --- a/mobile/src/components/mounted-bottom-drawer.tsx +++ b/mobile/src/components/mounted-bottom-drawer.tsx @@ -1,4 +1,4 @@ -import { type ReactNode, useCallback, useEffect, useState } from 'react' +import { type ReactNode, useCallback, useEffect, useRef, useState } from 'react' import { View, Pressable, @@ -89,6 +89,23 @@ export function MountedBottomDrawer({ }) : undefined + // Why: a sheet pinned under a fill picker keeps progress at 1 while the picker + // owns the window, so nothing re-applies its transform when the picker leaves. + // If the native view was rebuilt underneath (shared modal host swaps the window + // contents), it is left with no transform and never paints — a dimmed, dead + // screen the user can only escape by dismissing the whole modal. Re-assert the + // enter transform whenever a visible sheet takes the window back. + const wasInteractiveRef = useRef(interactive) + useEffect(() => { + const tookWindowBack = visible && interactive && !wasInteractiveRef.current + wasInteractiveRef.current = interactive + if (!tookWindowBack) { + return + } + translateY.value = 0 + progress.value = withTiming(1, { duration: SHOW_DURATION }) + }, [interactive, visible]) + useEffect(() => { if (visible) { translateY.value = 0 diff --git a/mobile/src/components/new-worktree-form-sheet-visibility.test.ts b/mobile/src/components/new-worktree-form-sheet-visibility.test.ts index eebe66015fe..dfbad5abf50 100644 --- a/mobile/src/components/new-worktree-form-sheet-visibility.test.ts +++ b/mobile/src/components/new-worktree-form-sheet-visibility.test.ts @@ -2,37 +2,30 @@ import { describe, expect, it } from 'vitest' import { resolveNewWorktreeFormSheetVisible } from './new-worktree-form-sheet-visibility' describe('resolveNewWorktreeFormSheetVisible', () => { - it('keeps the form under the source picker and its close transition', () => { + it('keeps the form under the source picker', () => { + expect(resolveNewWorktreeFormSheetVisible({ modalVisible: true, drawerView: 'source' })).toBe( + true + ) + }) + + // The host Modal stays mounted across every drawer swap, so a transition that + // renders no sheet is a transparent tap-swallowing screen with no way out if + // the queued transition never lands. + it('never leaves the mounted modal without a sheet during a drawer swap', () => { expect( - resolveNewWorktreeFormSheetVisible({ - modalVisible: true, - drawerView: 'source', - formPinnedUnderSource: true - }) - ).toBe(true) - expect( - resolveNewWorktreeFormSheetVisible({ - modalVisible: true, - drawerView: 'transition', - formPinnedUnderSource: true - }) + resolveNewWorktreeFormSheetVisible({ modalVisible: true, drawerView: 'transition' }) ).toBe(true) }) - it('hides the form for sequential repo/agent transitions', () => { - expect( - resolveNewWorktreeFormSheetVisible({ - modalVisible: true, - drawerView: 'transition', - formPinnedUnderSource: false - }) - ).toBe(false) - expect( - resolveNewWorktreeFormSheetVisible({ - modalVisible: true, - drawerView: 'project', - formPinnedUnderSource: false - }) - ).toBe(false) + it('yields the window to the content-sized pickers and the trust prompt', () => { + for (const drawerView of ['project', 'runTarget', 'agent', 'trust']) { + expect(resolveNewWorktreeFormSheetVisible({ modalVisible: true, drawerView })).toBe(false) + } + }) + + it('hides everything once the modal closes', () => { + expect(resolveNewWorktreeFormSheetVisible({ modalVisible: false, drawerView: 'form' })).toBe( + false + ) }) }) diff --git a/mobile/src/components/new-worktree-form-sheet-visibility.ts b/mobile/src/components/new-worktree-form-sheet-visibility.ts index 57ad63c0ef6..5125f112b7c 100644 --- a/mobile/src/components/new-worktree-form-sheet-visibility.ts +++ b/mobile/src/components/new-worktree-form-sheet-visibility.ts @@ -1,16 +1,18 @@ -// Why: pin the create form under the fill-height name picker (and during that -// picker's close transition) so dismiss reveals the original content height. +// Why: the create form is the modal's floor. Every other drawer layers above it, +// so the shared modal host is never mounted with no sheet in it — a beat with a +// transparent full-screen host swallows taps, and a dropped transition timer +// would strand the user there with no way back (#16165 follow-up). export function resolveNewWorktreeFormSheetVisible(input: { modalVisible: boolean drawerView: string - formPinnedUnderSource: boolean }): boolean { if (!input.modalVisible) { return false } - if (input.drawerView === 'form' || input.drawerView === 'source') { - return true - } - return input.drawerView === 'transition' && input.formPinnedUnderSource + return ( + input.drawerView === 'form' || + input.drawerView === 'source' || + input.drawerView === 'transition' + ) } diff --git a/mobile/src/components/use-new-worktree-drawer-navigation.test.ts b/mobile/src/components/use-new-worktree-drawer-navigation.test.ts new file mode 100644 index 00000000000..81bbfa7a206 --- /dev/null +++ b/mobile/src/components/use-new-worktree-drawer-navigation.test.ts @@ -0,0 +1,65 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + useNewWorktreeDrawerNavigation, + type NewWorktreeDrawerView +} from './use-new-worktree-drawer-navigation' + +type Nav = ReturnType + +function renderNavigation(modalVisible: boolean): { current: Nav } { + const handle = { current: null as unknown as Nav } + function Probe(props: { modalVisible: boolean }) { + handle.current = useNewWorktreeDrawerNavigation(props.modalVisible) + return null + } + let renderer: ReactTestRenderer | null = null + act(() => { + renderer = create(createElement(Probe, { modalVisible })) + }) + void renderer + return handle +} + +describe('useNewWorktreeDrawerNavigation', () => { + beforeEach(() => vi.useFakeTimers()) + afterEach(() => vi.useRealTimers()) + + // BottomDrawerModalHost keeps one native Modal mounted for the whole flow. A + // transition beat that renders no sheet is therefore a transparent full-screen + // window that eats every tap, and the queued timer is the only way out of it. + it('shows the form sheet for the whole transition, even if the queued timer never lands', () => { + const nav = renderNavigation(true) + act(() => nav.current.openSourceDrawer()) + expect(nav.current.drawerView).toBe('source') + + act(() => nav.current.transitionDrawer('form')) + expect(nav.current.drawerView).toBe('transition') + expect(nav.current.formSheetVisible).toBe(true) + expect(nav.current.formSheetInteractive).toBe(false) + }) + + it('keeps a sheet on screen while swapping to a content-sized picker', () => { + const nav = renderNavigation(true) + act(() => nav.current.transitionDrawer('agent')) + + expect(nav.current.drawerView).toBe('transition') + expect(nav.current.formSheetVisible).toBe(true) + + act(() => vi.advanceTimersByTime(500)) + expect(nav.current.drawerView).toBe('agent') + expect(nav.current.formSheetVisible).toBe(false) + }) + + it('hands the form back interactive once the transition lands', () => { + const nav = renderNavigation(true) + act(() => nav.current.openSourceDrawer()) + act(() => nav.current.transitionDrawer('form')) + act(() => vi.advanceTimersByTime(500)) + + expect(nav.current.drawerView).toBe('form') + expect(nav.current.formSheetVisible).toBe(true) + expect(nav.current.formSheetInteractive).toBe(true) + }) +}) diff --git a/mobile/src/components/use-new-worktree-drawer-navigation.ts b/mobile/src/components/use-new-worktree-drawer-navigation.ts index 4f3261cb685..34dfe956e53 100644 --- a/mobile/src/components/use-new-worktree-drawer-navigation.ts +++ b/mobile/src/components/use-new-worktree-drawer-navigation.ts @@ -23,11 +23,10 @@ export function useNewWorktreeDrawerNavigation(modalVisible: boolean): { openSourceDrawer: () => void } { const [drawerView, setDrawerView] = useState('form') - const formPinnedUnderSourceRef = useRef(false) const drawerTransitionTimerRef = useRef | null>(null) // Why: cancel any queued transition and reset when the modal closes, so a - // timer can't land after close and leave a stale drawer/pin for the next open. + // timer can't land after close and leave a stale drawer for the next open. useEffect(() => { if (modalVisible) { return @@ -36,7 +35,6 @@ export function useNewWorktreeDrawerNavigation(modalVisible: boolean): { clearTimeout(drawerTransitionTimerRef.current) drawerTransitionTimerRef.current = null } - formPinnedUnderSourceRef.current = false setDrawerView('form') }, [modalVisible]) @@ -55,31 +53,23 @@ export function useNewWorktreeDrawerNavigation(modalVisible: boolean): { setDrawerView('transition') drawerTransitionTimerRef.current = setTimeout(() => { drawerTransitionTimerRef.current = null - if (nextView === 'form') { - formPinnedUnderSourceRef.current = false - } setDrawerView(nextView) }, NEW_WORKTREE_DRAWER_TRANSITION_MS) } function openSourceDrawer(): void { - // Why: same-beat open; pin form under fill picker so outer content height - // is preserved when the name dialog dismisses. + // Why: same-beat open; the form stays mounted underneath so the outer + // content height is preserved when the fill picker dismisses. if (drawerTransitionTimerRef.current) { clearTimeout(drawerTransitionTimerRef.current) } drawerTransitionTimerRef.current = null - formPinnedUnderSourceRef.current = true setDrawerView('source') } return { drawerView, - formSheetVisible: resolveNewWorktreeFormSheetVisible({ - modalVisible, - drawerView, - formPinnedUnderSource: formPinnedUnderSourceRef.current - }), + formSheetVisible: resolveNewWorktreeFormSheetVisible({ modalVisible, drawerView }), formSheetInteractive: drawerView === 'form', transitionDrawer, openSourceDrawer diff --git a/mobile/src/tasks/smart-source-paste-concurrency.test.ts b/mobile/src/tasks/smart-source-paste-concurrency.test.ts new file mode 100644 index 00000000000..3b65b0549dc --- /dev/null +++ b/mobile/src/tasks/smart-source-paste-concurrency.test.ts @@ -0,0 +1,55 @@ +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 { useSmartWorkspaceSource } from './use-smart-workspace-source' + +function Probe(props: { client: RpcClient; query: string }) { + useSmartWorkspaceSource({ + client: props.client, + enabled: true, + mode: 'smart', + query: props.query, + repoId: 'repo-1', + githubAvailable: true, + gitlabAvailable: false, + linearAvailable: false, + mrStateFilter: 'opened', + repos: [{ id: 'repo-1', displayName: 'orca', slug: { owner: 'stablyai', repo: 'orca' } }] + }) + return null +} + +// The picker makes two independent host round trips for a pasted PR number: the +// provider fan-out and the exact-item lookup. Awaiting the fan-out first stacked +// them, so the rows appeared a whole extra round trip late. +describe('smart source paste lookup concurrency', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => vi.useFakeTimers()) + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + vi.useRealTimers() + }) + + it('issues the pasted-number lookup while the fan-out is still in flight', async () => { + const sent: string[] = [] + const sendRequest = vi.fn((method: string) => { + sent.push(method) + // Nothing ever settles: only requests issued concurrently can be observed. + return new Promise(() => {}) + }) + const client = { sendRequest } as unknown as RpcClient + + await act(async () => { + renderer = create(createElement(Probe, { client, query: '16831' })) + }) + await act(async () => { + await vi.advanceTimersByTimeAsync(300) + }) + + expect(sent).toContain('github.listWorkItems') + expect(sent).toContain('github.workItem') + }) +}) diff --git a/mobile/src/tasks/use-smart-workspace-source.ts b/mobile/src/tasks/use-smart-workspace-source.ts index 86c6ca2204a..a57c9f17d7d 100644 --- a/mobile/src/tasks/use-smart-workspace-source.ts +++ b/mobile/src/tasks/use-smart-workspace-source.ts @@ -180,6 +180,64 @@ export function useSmartWorkspaceSource(args: UseSmartWorkspaceSourceArgs) { } } +type PasteLookup = { paste: PasteResolved; crossRepoPrompt: SmartCrossRepoPrompt | null } + +const EMPTY_PASTE_LOOKUP: PasteLookup = { + paste: { github: null, gitlab: null }, + crossRepoPrompt: null +} + +// Resolves a pasted issue/PR/MR reference to the exact item it names. +async function resolvePastedItem(args: { + client: RpcClient + intent: NonNullable> + repoId: string + repos: readonly PasteRepoCandidate[] + repoSlugCache: Map +}): Promise { + const { client, intent, repoId, repos, repoSlugCache } = args + if (intent.kind === 'github-number') { + return { + paste: { + github: await lookupGitHubItemByNumber(client, repoId, intent.number), + gitlab: null + }, + crossRepoPrompt: null + } + } + if (intent.kind === 'github-link') { + const matchingRepo = await findRepoMatchingSlugForPaste( + client, + repos, + intent.link.slug, + repoSlugCache + ) + if (matchingRepo && matchingRepo.id !== repoId) { + return { + paste: { github: null, gitlab: null }, + crossRepoPrompt: { link: intent.link, matchingRepo } + } + } + return { + paste: { + github: await lookupGitHubItemByOwnerRepo( + client, + repoId, + intent.link.slug, + intent.link.number, + intent.link.type + ), + gitlab: null + }, + crossRepoPrompt: null + } + } + return { + paste: { github: null, gitlab: await lookupGitLabItemByPath(client, repoId, intent.link) }, + crossRepoPrompt: null + } +} + async function runSmartSearch(args: { client: RpcClient mode: SmartNameMode @@ -199,42 +257,21 @@ async function runSmartSearch(args: { crossRepoPrompt: SmartCrossRepoPrompt | null }> { const { client, mode, query, repoId, repos, dismissedPasteRef, repoSlugCache } = args - const fan = await fanOutSmartSearch(args) - const paste: PasteResolved = { github: null, gitlab: null } - let crossRepoPrompt: SmartCrossRepoPrompt | null = null - const intent = mode === 'branches' || dismissedPasteRef.current === query.trim() ? null : resolvePasteIntent(query) - if (intent && repoId) { - try { - if (intent.kind === 'github-number') { - paste.github = await lookupGitHubItemByNumber(client, repoId, intent.number) - } else if (intent.kind === 'github-link') { - const matchingRepo = await findRepoMatchingSlugForPaste( - client, - repos, - intent.link.slug, - repoSlugCache + // Why: the paste lookup and the provider fan-out hit different host endpoints, + // so awaiting the fan-out first stacked two full round trips on the one path a + // user is most likely to take — typing a PR/issue number. Run them together. + const [fan, pasteLookup] = await Promise.all([ + fanOutSmartSearch(args), + intent && repoId + ? resolvePastedItem({ client, intent, repoId, repos, repoSlugCache }).catch( + // Best-effort paste resolution; fall back to the fan-out results. + () => EMPTY_PASTE_LOOKUP ) - if (matchingRepo && matchingRepo.id !== repoId) { - crossRepoPrompt = { link: intent.link, matchingRepo } - } else { - paste.github = await lookupGitHubItemByOwnerRepo( - client, - repoId, - intent.link.slug, - intent.link.number, - intent.link.type - ) - } - } else if (intent.kind === 'gitlab-link') { - paste.gitlab = await lookupGitLabItemByPath(client, repoId, intent.link) - } - } catch { - // Best-effort paste resolution; fall back to the fan-out results. - } - } - return { fan, paste, crossRepoPrompt } + : Promise.resolve(EMPTY_PASTE_LOOKUP) + ]) + return { fan, paste: pasteLookup.paste, crossRepoPrompt: pasteLookup.crossRepoPrompt } }