Fix mobile bottom sheets reopening after dismiss

docs/floating-panels.md

@@ -155,6 +155,20 @@ The "render invisible to measure, then reveal" pattern is the canonical
 solution to chicken-and-egg positioning in this codebase. Reach for it before
 anything fancier.
 
+## Gotcha 6 — Bottom sheet refs are not lifecycle truth
+
+`@gorhom/bottom-sheet` modals churn their imperative ref while presenting and
+dismissing. Do not treat `ref != null` as permission to call `present()`, and do
+not treat `ref == null` as the sheet being closed. The user-visible lifecycle is
+the desired `visible` prop plus the sheet callbacks (`onChange(-1)`,
+`onDismiss`).
+
+If a user closes a sheet with the backdrop or a pan gesture, the sheet may detach
+and reattach before React state has acknowledged `visible=false`. Re-presenting
+on that attach races Gorhom's dismiss path and leaves the modal unable to reopen.
+Track an explicit phase (`closed` / `presenting` / `presented` / `dismissing`) and
+ignore ref churn while dismissing.
+
 ## Recipe for a new anchored panel
 
 Before you write a new one, ask:

packages/app/e2e/bottom-sheet-reopen.spec.ts

@@ -0,0 +1,95 @@
+import { expect, test, type Page } from "./fixtures";
+import { expectComposerVisible } from "./helpers/composer";
+import { openAgentRoute, seedMockAgentWorkspace } from "./helpers/mock-agent";
+
+const MOBILE_VIEWPORT = { width: 390, height: 844 };
+
+async function openMockAgentAtMobileBreakpoint(page: Page) {
+  await page.setViewportSize(MOBILE_VIEWPORT);
+  const session = await seedMockAgentWorkspace({
+    repoPrefix: "bottom-sheet-reopen-",
+    title: "Bottom sheet reopen e2e",
+    initialPrompt: "Prepare a bottom sheet reopen test agent.",
+  });
+  await openAgentRoute(page, session);
+  await expect(page.getByTestId("workspace-tab-switcher-trigger")).toBeVisible({
+    timeout: 30_000,
+  });
+  await expectComposerVisible(page);
+  await expect(page.getByRole("button", { name: /Select model/ })).toBeVisible({
+    timeout: 30_000,
+  });
+  return session;
+}
+
+async function withMobileMockAgent(page: Page, run: () => Promise<void>) {
+  const session = await openMockAgentAtMobileBreakpoint(page);
+
+  try {
+    await run();
+  } finally {
+    await session.cleanup();
+  }
+}
+
+function bottomSheetBackdrop(page: Page) {
+  return page.getByRole("button", { name: "Bottom sheet backdrop" }).first();
+}
+
+async function expectBottomSheetOpen(page: Page) {
+  await expect(bottomSheetBackdrop(page)).toBeVisible({ timeout: 10_000 });
+}
+
+async function closeBottomSheetWithBackdrop(page: Page) {
+  const box = await bottomSheetBackdrop(page).boundingBox();
+  expect(box).not.toBeNull();
+  await page.mouse.click(box!.x + box!.width / 2, box!.y + 24);
+  await expect(bottomSheetBackdrop(page)).not.toBeVisible({ timeout: 10_000 });
+  // Guard against the regression where the sheet starts dismissing, then re-presents.
+  await page.waitForTimeout(500);
+  await expect(bottomSheetBackdrop(page)).not.toBeVisible({ timeout: 1_000 });
+}
+
+async function openTabSwitcher(page: Page) {
+  const trigger = page.getByRole("button", { name: /Switch tabs/ });
+  await trigger.click();
+  await expectBottomSheetOpen(page);
+}
+
+async function openModelSelector(page: Page) {
+  await page.getByRole("button", { name: /Select model/ }).click();
+  await expectBottomSheetOpen(page);
+  await expect(
+    page.getByLabel("Bottom Sheet", { exact: true }).getByText("Ten second stream", {
+      exact: true,
+    }),
+  ).toBeVisible({ timeout: 10_000 });
+}
+
+async function openAndCloseTabSwitcherTwice(page: Page) {
+  await openTabSwitcher(page);
+  await closeBottomSheetWithBackdrop(page);
+  await openTabSwitcher(page);
+  await closeBottomSheetWithBackdrop(page);
+}
+
+async function openAndCloseModelSelectorTwice(page: Page) {
+  await openModelSelector(page);
+  await closeBottomSheetWithBackdrop(page);
+  await openModelSelector(page);
+  await closeBottomSheetWithBackdrop(page);
+}
+
+test.describe("mobile bottom sheet reopen", () => {
+  test("tab switcher can open, close, reopen, and close again", async ({ page }) => {
+    await withMobileMockAgent(page, async () => {
+      await openAndCloseTabSwitcherTwice(page);
+    });
+  });
+
+  test("model selector can open, close, reopen, and close again", async ({ page }) => {
+    await withMobileMockAgent(page, async () => {
+      await openAndCloseModelSelectorTwice(page);
+    });
+  });
+});

packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts

@@ -118,4 +118,44 @@ describe("bottom sheet visibility tracker", () => {
     tracker.handleSheetIndexChange(-1);
     expect(closeCount()).toBe(2);
   });
+
+  it("does not re-present when the controller reattaches before parent state acknowledges a user dismiss", () => {
+    const { sheet, tracker, closeCount } = setup();
+    tracker.attachController(sheet);
+    tracker.syncDesired({ visible: true });
+
+    tracker.handleSheetIndexChange(-1);
+    tracker.attachController(null);
+    tracker.attachController(sheet);
+
+    expect(closeCount()).toBe(1);
+    expect(sheet.events).toEqual([{ type: "present" }]);
+  });
+
+  it("does not re-present when dismiss fires before parent state acknowledges a user dismiss", () => {
+    const { sheet, tracker, closeCount } = setup();
+    tracker.attachController(sheet);
+    tracker.syncDesired({ visible: true });
+
+    tracker.handleSheetDismiss();
+    tracker.attachController(null);
+    tracker.attachController(sheet);
+
+    expect(closeCount()).toBe(1);
+    expect(sheet.events).toEqual([{ type: "present" }]);
+  });
+
+  it("allows a fresh open after parent state acknowledges a dismissed sheet", () => {
+    const { sheet, tracker } = setup();
+    tracker.attachController(sheet);
+    tracker.syncDesired({ visible: true });
+
+    tracker.handleSheetIndexChange(-1);
+    tracker.attachController(null);
+    tracker.attachController(sheet);
+    tracker.syncDesired({ visible: false });
+    tracker.syncDesired({ visible: true });
+
+    expect(sheet.events).toEqual([{ type: "present" }, { type: "present" }]);
+  });
 });

packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts

@@ -15,25 +15,27 @@ export interface BottomSheetVisibilityTracker {
   handleSheetDismiss(): void;
 }
 
+type BottomSheetPhase = "closed" | "presenting" | "presented" | "dismissing";
+
 export function createBottomSheetVisibilityTracker(opts: {
   onClose: () => void;
 }): BottomSheetVisibilityTracker {
   let controller: BottomSheetController | null = null;
   let visible = false;
   let isEnabled: boolean | undefined;
-  let isPresented = false;
+  let phase: BottomSheetPhase = "closed";
   let hasNotifiedClose = false;
 
   function present(): void {
-    if (!controller || isPresented) return;
-    isPresented = true;
+    if (!controller || phase !== "closed") return;
+    phase = "presenting";
     hasNotifiedClose = false;
     controller.present();
   }
 
   function dismiss(): void {
-    if (!controller || !isPresented) return;
-    isPresented = false;
+    if (!controller || phase === "closed" || phase === "dismissing") return;
+    phase = "dismissing";
     controller.dismiss();
   }
 
@@ -58,19 +60,34 @@ export function createBottomSheetVisibilityTracker(opts: {
         present();
         return;
       }
+      if (phase === "dismissing") {
+        phase = "closed";
+        hasNotifiedClose = false;
+        return;
+      }
       dismiss();
     },
     handleSheetIndexChange(index) {
-      if (index === -1 && visible) {
+      if (index !== -1) {
+        if (phase === "presenting") {
+          phase = "presented";
+        }
+        return;
+      }
+      if (phase === "presenting" || phase === "presented") {
+        phase = "dismissing";
+      }
+      if (visible) {
         notifyClose();
       }
     },
     handleSheetDismiss() {
-      isPresented = false;
       if (visible) {
+        phase = "dismissing";
         notifyClose();
         return;
       }
+      phase = "closed";
       hasNotifiedClose = false;
     },
   };