diff --git a/docs/floating-panels.md b/docs/floating-panels.md index 7b4f752a7..2e8668f45 100644 --- a/docs/floating-panels.md +++ b/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: diff --git a/packages/app/e2e/bottom-sheet-reopen.spec.ts b/packages/app/e2e/bottom-sheet-reopen.spec.ts new file mode 100644 index 000000000..a0bbdda00 --- /dev/null +++ b/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) { + 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); + }); + }); +}); diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts index 762403a1d..2866ceefb 100644 --- a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.test.ts +++ b/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" }]); + }); }); diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts b/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts index a78a34e4a..9bd351d3d 100644 --- a/packages/app/src/components/ui/isolated-bottom-sheet-modal/visibility-tracker.ts +++ b/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; }, };