fix(mobile): keep the create form on screen through drawer swaps and survive reconnects

The create-worktree flow could reach a state where the shared modal host was
mounted with no sheet in it: a full-screen transparent window that swallows
every tap with no way out. Frame analysis of the reported recording and a live
simulator repro both land on the same state - the form sheet laid out at the
right frame with progress=1, backdrop painting, sheet not painted.

- Keep the form sheet mounted through every drawer transition, so the host
  Modal is never on screen without a sheet, and drop the render-read pin ref.
- Re-assert a pinned sheet's enter transform when it takes the window back
  from a fill picker; nothing re-applied it before.
- Key the form session on hostId, not on the RpcClient object: useHostClient
  swaps that object on every reconnect, which silently remounted the form and
  threw away the picked source.
- Run the pasted-item lookup concurrently with the provider fan-out instead of
  after it (measured 2631ms -> 1480ms for a typed PR number).
This commit is contained in:
Brennan Benson
2026-08-28 01:32:17 -07:00
parent 86b770e448
commit 28ac0caf95
11 changed files with 756 additions and 88 deletions
+302
View File
@@ -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 <path>` 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.
@@ -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(() => {}))
+9 -4
View File
@@ -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 (
<NewWorktreeModalContent
key={`${openEpochRef.current}:${clientEpochRef.current.epoch}`}
key={`${openEpochRef.current}:${hostEpochRef.current.epoch}`}
{...props}
/>
)
@@ -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: <T>(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<string, unknown> = {}
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())
})
})
@@ -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
@@ -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
)
})
})
@@ -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'
)
}
@@ -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<typeof useNewWorktreeDrawerNavigation>
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<NewWorktreeDrawerView>('source')
act(() => nav.current.transitionDrawer('form'))
expect(nav.current.drawerView).toBe<NewWorktreeDrawerView>('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<NewWorktreeDrawerView>('transition')
expect(nav.current.formSheetVisible).toBe(true)
act(() => vi.advanceTimersByTime(500))
expect(nav.current.drawerView).toBe<NewWorktreeDrawerView>('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<NewWorktreeDrawerView>('form')
expect(nav.current.formSheetVisible).toBe(true)
expect(nav.current.formSheetInteractive).toBe(true)
})
})
@@ -23,11 +23,10 @@ export function useNewWorktreeDrawerNavigation(modalVisible: boolean): {
openSourceDrawer: () => void
} {
const [drawerView, setDrawerView] = useState<NewWorktreeDrawerView>('form')
const formPinnedUnderSourceRef = useRef(false)
const drawerTransitionTimerRef = useRef<ReturnType<typeof setTimeout> | 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
@@ -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')
})
})
+70 -33
View File
@@ -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<ReturnType<typeof resolvePasteIntent>>
repoId: string
repos: readonly PasteRepoCandidate[]
repoSlugCache: Map<string, { owner: string; repo: string; host?: string } | null>
}): Promise<PasteLookup> {
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 }
}