diff --git a/docs/floating-panels.md b/docs/floating-panels.md index 2e8668f45..d36df33a9 100644 --- a/docs/floating-panels.md +++ b/docs/floating-panels.md @@ -169,6 +169,11 @@ 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. +Do not treat `onChange(-1)` as a close by itself. In a stacked +`BottomSheetModal`, `-1` can also mean the sheet is temporarily hidden under +another pushed sheet. Close React state from `onDismiss`; use `onChange` only to +track phase. + ## Recipe for a new anchored panel Before you write a new one, ask: diff --git a/packages/app/e2e/provider-settings-refresh.spec.ts b/packages/app/e2e/provider-settings-refresh.spec.ts new file mode 100644 index 000000000..7bf9583b5 --- /dev/null +++ b/packages/app/e2e/provider-settings-refresh.spec.ts @@ -0,0 +1,109 @@ +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: "provider-sheet-stack-", + title: "Provider sheet stack e2e", + initialPrompt: "Prepare provider sheet stack test agent.", + }); + await openAgentRoute(page, session); + await expectComposerVisible(page); + await expect(page.getByRole("button", { name: /Select model/ })).toBeVisible({ + timeout: 30_000, + }); + return session; +} + +async function openProviderSettingsFromModelSelector(page: Page) { + await page.getByRole("button", { name: /Select model/ }).click(); + await expect(page.getByLabel("Bottom Sheet", { exact: true })).toBeVisible({ timeout: 10_000 }); + + const openCodeRow = page.getByText("OpenCode", { exact: true }).first(); + if (await openCodeRow.isVisible().catch(() => false)) { + await openCodeRow.click(); + } + + await page.getByRole("button", { name: /Open .* settings/ }).click(); + await expect(page.getByTestId("provider-settings-sheet")).toBeVisible({ timeout: 10_000 }); +} + +async function expectModelSelectorVisible(page: Page) { + await expect(page.getByRole("button", { name: /Open .* settings/ })).toBeVisible({ + timeout: 10_000, + }); +} + +async function closeTopSheet(page: Page) { + const closeTarget = page.getByLabel("Close", { exact: true }).last(); + if (await closeTarget.isVisible().catch(() => false)) { + await closeTarget.click({ force: true }); + return; + } + + const handle = page.getByRole("slider", { name: "Bottom sheet handle" }).last(); + const handleBox = await handle.boundingBox(); + if (!handleBox) { + throw new Error("Bottom sheet handle was not measurable"); + } + const startX = handleBox.x + handleBox.width / 2; + const startY = handleBox.y + handleBox.height / 2; + await page.mouse.move(startX, startY); + await page.mouse.down(); + await page.mouse.move(startX, startY + 400, { steps: 8 }); + await page.mouse.up(); +} + +async function expectProviderSettingsVisible(page: Page) { + await expect(page.getByTestId("provider-settings-sheet")).toBeVisible({ timeout: 10_000 }); + await expect(page.getByRole("button", { name: "Add model" })).toBeVisible(); + await expect(page.getByRole("button", { name: "Diagnostic", exact: true })).toBeVisible(); +} + +async function exerciseProviderSettingsStack(page: Page) { + await expectProviderSettingsVisible(page); + + await page.getByRole("button", { name: "Add model" }).click(); + await expect(page.getByTestId("add-custom-model-sheet")).toBeVisible({ timeout: 10_000 }); + await closeTopSheet(page); + await expectProviderSettingsVisible(page); + + await page.getByRole("button", { name: "Diagnostic", exact: true }).click(); + await expect(page.getByTestId("provider-diagnostic-sheet")).toBeVisible({ timeout: 10_000 }); + await page.getByRole("button", { name: /Refresh diagnostic/ }).click(); + await expect(page.getByTestId("provider-diagnostic-sheet")).toBeVisible({ timeout: 10_000 }); + await closeTopSheet(page); + await expectProviderSettingsVisible(page); + + await page.getByRole("button", { name: "Refresh", exact: true }).click(); + await expectProviderSettingsVisible(page); +} + +test.describe("provider settings bottom-sheet stack", () => { + test("provider settings and children close back through the model selector stack", async ({ + page, + }) => { + const session = await openMockAgentAtMobileBreakpoint(page); + + try { + await openProviderSettingsFromModelSelector(page); + await exerciseProviderSettingsStack(page); + await closeTopSheet(page); + + await expectModelSelectorVisible(page); + await page.getByRole("button", { name: /Open .* settings/ }).click(); + await expect(page.getByTestId("provider-settings-sheet")).toBeVisible({ timeout: 10_000 }); + await exerciseProviderSettingsStack(page); + await closeTopSheet(page); + + await expectModelSelectorVisible(page); + await closeTopSheet(page); + } finally { + await session.cleanup(); + } + }); +}); diff --git a/packages/app/src/app/_layout.tsx b/packages/app/src/app/_layout.tsx index 411ab7b28..8adfa7286 100644 --- a/packages/app/src/app/_layout.tsx +++ b/packages/app/src/app/_layout.tsx @@ -1,4 +1,5 @@ import "@/styles/unistyles"; +import { BottomSheetModalProvider } from "@gorhom/bottom-sheet"; import { PortalProvider } from "@gorhom/portal"; import { QueryClientProvider } from "@tanstack/react-query"; import * as Linking from "expo-linking"; @@ -931,17 +932,20 @@ function RuntimeProviders({ children }: { children: ReactNode }) { ); } -// PortalProvider must remain the innermost global provider here. +// PortalProvider must stay inside normal app-wide context providers here. // `@gorhom/portal` renders portaled children at the host's location in the // tree, so any context a portaled sheet might consume (QueryClient, theme, // auth, settings, …) must wrap PortalProvider — not be wrapped by it. -// Adding a new global provider? Put it above PortalProvider. +// BottomSheetModalProvider is the exception: Gorhom modals consume portal +// context and need one shared provider for sibling sheets to stack. function RootProviders({ children }: { children: ReactNode }) { return ( - {children} + + {children} + diff --git a/packages/app/src/components/adaptive-modal-sheet.tsx b/packages/app/src/components/adaptive-modal-sheet.tsx index 5b9e85d76..0ff84fa35 100644 --- a/packages/app/src/components/adaptive-modal-sheet.tsx +++ b/packages/app/src/components/adaptive-modal-sheet.tsx @@ -12,6 +12,7 @@ import { BottomSheetTextInput, type BottomSheetBackgroundProps, } from "@gorhom/bottom-sheet"; +import Animated from "react-native-reanimated"; import { ArrowLeft, Search, X } from "lucide-react-native"; import { FileDropZone } from "@/components/file-drop-zone"; import type { ImageAttachment } from "@/composer/types"; @@ -232,14 +233,14 @@ function SheetBackground({ style }: BottomSheetBackgroundProps) { () => [ style, { - backgroundColor: theme.colors.surface1, - borderTopLeftRadius: theme.borderRadius.xl, - borderTopRightRadius: theme.borderRadius.xl, + backgroundColor: theme.colors.surface0, + borderTopLeftRadius: theme.borderRadius["2xl"], + borderTopRightRadius: theme.borderRadius["2xl"], }, ], - [style, theme.colors.surface1, theme.borderRadius.xl], + [style, theme.colors.surface0, theme.borderRadius], ); - return ; + return ; } export type AdaptiveTextInputProps = TextInputProps & { @@ -444,6 +445,7 @@ export interface AdaptiveModalSheetProps { /** When provided, wraps the card content in a FileDropZone. */ onFilesDropped?: (files: ImageAttachment[]) => void; scrollable?: boolean; + presentation?: "push" | "replace"; } export function AdaptiveModalSheet({ @@ -457,6 +459,7 @@ export function AdaptiveModalSheet({ desktopMaxWidth, onFilesDropped, scrollable = true, + presentation, }: AdaptiveModalSheetProps) { const { theme } = useUnistyles(); const isMobile = useIsCompactFormFactor(); @@ -501,8 +504,8 @@ export function AdaptiveModalSheet({ [compactSafeAreaPadding.footerPaddingBottom], ); const handleIndicatorStyle = useMemo( - () => ({ backgroundColor: theme.colors.surface2 }), - [theme.colors.surface2], + () => ({ backgroundColor: theme.colors.palette.zinc[600] }), + [theme.colors.palette.zinc], ); const { sheetRef, handleSheetChange, handleSheetDismiss } = useIsolatedBottomSheetVisibility({ visible, @@ -543,6 +546,7 @@ export function AdaptiveModalSheet({ keyboardBehavior="extend" keyboardBlurBehavior="restore" accessible={false} + presentation={presentation} > {scrollable ? ( diff --git a/packages/app/src/components/combined-model-selector.tsx b/packages/app/src/components/combined-model-selector.tsx index ad14b64a7..5db10b5ce 100644 --- a/packages/app/src/components/combined-model-selector.tsx +++ b/packages/app/src/components/combined-model-selector.tsx @@ -685,10 +685,6 @@ export function CombinedModelSelector({ handleOpenChange(!isOpen); }, [handleOpenChange, isOpen]); - const handleClose = useCallback(() => { - handleOpenChange(false); - }, [handleOpenChange]); - const triggerStyle = useCallback( ({ pressed, hovered }: PressableStateCallbackType & { hovered?: boolean }) => [ styles.trigger, @@ -714,11 +710,10 @@ export function CombinedModelSelector({ setSearchQuery(value); }, []); - const openHeaderProviderSettings = useCallback(() => { + const openProviderSettings = useCallback(() => { if (!serverId || view.kind !== "provider") return; useProviderSettingsStore.getState().open({ serverId, provider: view.providerId }); - handleClose(); - }, [serverId, view, handleClose]); + }, [serverId, view]); const sheetHeader = useMemo(() => { if (view.kind === "all") { @@ -727,7 +722,7 @@ export function CombinedModelSelector({ const ProviderIconForView = getProviderIcon(view.providerId); const headerActions = ( Model ID @@ -339,6 +340,7 @@ function DiagnosticSubSheet({ onClose={onClose} snapPoints={DIAGNOSTIC_SNAP_POINTS} scrollable={false} + testID="provider-diagnostic-sheet" > {body} @@ -616,6 +618,7 @@ export function ProviderDiagnosticSheet({ header={sheetHeader} visible={visible} onClose={onClose} + testID="provider-settings-sheet" footer={renderProviderSheetFooter({ fetchedAtLabel, isCompact, diff --git a/packages/app/src/components/provider-settings-host.tsx b/packages/app/src/components/provider-settings-host.tsx index 30f5e2d07..ce8937a93 100644 --- a/packages/app/src/components/provider-settings-host.tsx +++ b/packages/app/src/components/provider-settings-host.tsx @@ -5,6 +5,7 @@ import { useProviderSettingsStore } from "@/stores/provider-settings-store"; export function ProviderSettingsHost() { const serverId = useProviderSettingsStore((state) => state.serverId); const provider = useProviderSettingsStore((state) => state.provider); + const visible = useProviderSettingsStore((state) => state.visible); const close = useProviderSettingsStore((state) => state.close); const handleClose = useCallback(() => { @@ -19,7 +20,7 @@ export function ProviderSettingsHost() { ); diff --git a/packages/app/src/components/ui/combobox.tsx b/packages/app/src/components/ui/combobox.tsx index fcf4b301c..2440a6bba 100644 --- a/packages/app/src/components/ui/combobox.tsx +++ b/packages/app/src/components/ui/combobox.tsx @@ -95,6 +95,7 @@ export interface ComboboxProps { */ header?: SheetHeader; mobileChildrenScrollEnabled?: boolean; + presentation?: "push" | "replace"; open?: boolean; onOpenChange?: (open: boolean) => void; desktopPlacement?: "top-start" | "bottom-start"; @@ -965,6 +966,7 @@ interface MobileBodyProps { searchable: boolean; hasChildren: boolean; mobileChildrenScrollEnabled: boolean; + presentation?: "push" | "replace"; searchResetKey: number; searchPlaceholder: string; searchQuery: string; @@ -1024,6 +1026,7 @@ function MobileComboboxBody(props: MobileBodyProps): ReactElement { handleIndicatorStyle={props.handleIndicatorStyle} keyboardBehavior="extend" keyboardBlurBehavior="none" + presentation={props.presentation} > {props.header ? ( @@ -1248,6 +1251,7 @@ export function Combobox({ title = "Select", header, mobileChildrenScrollEnabled = true, + presentation, open, onOpenChange, desktopPlacement = "top-start", @@ -1534,6 +1538,7 @@ export function Combobox({ searchable={searchable} hasChildren={hasChildren} mobileChildrenScrollEnabled={mobileChildrenScrollEnabled} + presentation={presentation} searchResetKey={searchResetKey} searchPlaceholder={effectiveSearchPlaceholder} searchQuery={searchQuery} diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.test.tsx b/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.test.tsx new file mode 100644 index 000000000..5b15bc96d --- /dev/null +++ b/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.test.tsx @@ -0,0 +1,76 @@ +/** + * @vitest-environment jsdom + */ +import React, { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@gorhom/portal", () => ({ + Portal: ({ children, hostName }: { children?: React.ReactNode; hostName?: string }) => + React.createElement("div", { "data-portal-host": hostName }, children), + PortalHost: ({ name }: { name?: string }) => React.createElement("div", { "data-host": name }), +})); + +vi.mock("@gorhom/bottom-sheet", () => ({ + BottomSheetModalProvider: ({ children }: { children?: React.ReactNode }) => + React.createElement("div", { "data-bottom-sheet-provider": true }, children), + BottomSheetModal: React.forwardRef( + ( + { + children, + stackBehavior, + }: { + children?: React.ReactNode; + stackBehavior?: string; + }, + _ref, + ) => React.createElement("div", { "data-stack-behavior": stackBehavior }, children), + ), +})); + +import { IsolatedBottomSheetModal } from "."; + +describe("IsolatedBottomSheetModal presentation", () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + it("defaults sibling top-level sheets to push instead of replacing by React ancestry", () => { + act(() => { + root.render( + <> + Settings + Diagnostic + , + ); + }); + + expect( + Array.from(container.querySelectorAll("[data-stack-behavior]")).map((node) => + node.getAttribute("data-stack-behavior"), + ), + ).toEqual(["push", "push"]); + }); + + it("only replaces when the callsite asks for replacement", () => { + act(() => { + root.render( + Selector, + ); + }); + + expect( + container.querySelector("[data-stack-behavior]")?.getAttribute("data-stack-behavior"), + ).toBe("replace"); + }); +}); diff --git a/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.tsx b/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.tsx index 6b3f20c91..3f5585936 100644 --- a/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.tsx +++ b/packages/app/src/components/ui/isolated-bottom-sheet-modal/index.tsx @@ -1,10 +1,8 @@ import { BottomSheetModal as GorhomBottomSheetModal, - BottomSheetModalProvider, type BottomSheetModalProps, } from "@gorhom/bottom-sheet"; -import { Portal } from "@gorhom/portal"; -import React, { createContext, useContext } from "react"; +import React from "react"; import { forwardRef, useCallback, useEffect, useMemo, useRef } from "react"; import type { ElementRef } from "react"; import { @@ -17,50 +15,29 @@ type GorhomBottomSheetModalMethods = ElementRef; type IsolatedBottomSheetModalProps = Omit< BottomSheetModalProps, "enableDismissOnClose" | "stackBehavior" ->; +> & { + presentation?: "push" | "replace"; +}; export type IsolatedBottomSheetModalRef = GorhomBottomSheetModalMethods; -const IsolatedBottomSheetScopeContext = createContext(false); - export const IsolatedBottomSheetModal = forwardRef< IsolatedBottomSheetModalRef, IsolatedBottomSheetModalProps >(function IsolatedBottomSheetModal(props, ref) { - const isNestedSheet = useContext(IsolatedBottomSheetScopeContext); - const { children, ...bottomSheetProps } = props; - const scopedChildren = - typeof children === "function" ? ( - (input: { data?: unknown }) => ( - - {children(input) as React.ReactNode} - - ) - ) : ( - - {children} - - ); + const { children, presentation = "push", ...bottomSheetProps } = props; const modal = ( - {scopedChildren} + {children} ); - if (isNestedSheet) { - return modal; - } - - return ( - - {modal} - - ); + return modal; }); export function useIsolatedBottomSheetVisibility({ 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 2866ceefb..d70dffdaa 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 @@ -70,17 +70,17 @@ describe("bottom sheet visibility tracker", () => { expect(sheet.events).toEqual([]); }); - it("only reports a user close when the sheet was visible", () => { + it("does not treat index -1 as a close because stacked sheets can be hidden without dismissing", () => { const { sheet, tracker, closeCount } = setup(); tracker.attachController(sheet); tracker.syncDesired({ visible: true }); tracker.handleSheetIndexChange(-1); - expect(closeCount()).toBe(1); + expect(closeCount()).toBe(0); tracker.syncDesired({ visible: false }); tracker.handleSheetIndexChange(-1); - expect(closeCount()).toBe(1); + expect(closeCount()).toBe(0); }); it("reports a dismiss while visible as a close request", () => { @@ -93,7 +93,7 @@ describe("bottom sheet visibility tracker", () => { expect(closeCount()).toBe(1); }); - it("deduplicates close notifications from change and dismiss callbacks", () => { + it("reports close once when a hidden sheet is actually dismissed", () => { const { sheet, tracker, closeCount } = setup(); tracker.attachController(sheet); tracker.syncDesired({ visible: true }); @@ -110,12 +110,14 @@ describe("bottom sheet visibility tracker", () => { tracker.syncDesired({ visible: true }); tracker.handleSheetIndexChange(-1); + tracker.handleSheetDismiss(); expect(closeCount()).toBe(1); tracker.syncDesired({ visible: false }); tracker.syncDesired({ visible: true }); tracker.handleSheetIndexChange(-1); + tracker.handleSheetDismiss(); expect(closeCount()).toBe(2); }); @@ -128,7 +130,7 @@ describe("bottom sheet visibility tracker", () => { tracker.attachController(null); tracker.attachController(sheet); - expect(closeCount()).toBe(1); + expect(closeCount()).toBe(0); expect(sheet.events).toEqual([{ 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 9bd351d3d..048d85f36 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 @@ -69,7 +69,7 @@ export function createBottomSheetVisibilityTracker(opts: { }, handleSheetIndexChange(index) { if (index !== -1) { - if (phase === "presenting") { + if (phase === "presenting" || phase === "dismissing") { phase = "presented"; } return; @@ -77,9 +77,6 @@ export function createBottomSheetVisibilityTracker(opts: { if (phase === "presenting" || phase === "presented") { phase = "dismissing"; } - if (visible) { - notifyClose(); - } }, handleSheetDismiss() { if (visible) { diff --git a/packages/app/src/stores/provider-settings-store.ts b/packages/app/src/stores/provider-settings-store.ts index cbe18a676..abd214c5a 100644 --- a/packages/app/src/stores/provider-settings-store.ts +++ b/packages/app/src/stores/provider-settings-store.ts @@ -8,6 +8,7 @@ interface ProviderSettingsTarget { interface ProviderSettingsStoreState { serverId: string | null; provider: string | null; + visible: boolean; open: (target: ProviderSettingsTarget) => void; close: () => void; } @@ -15,10 +16,11 @@ interface ProviderSettingsStoreState { export const useProviderSettingsStore = create()((set) => ({ serverId: null, provider: null, + visible: false, open: ({ serverId, provider }) => { - set({ serverId, provider }); + set({ serverId, provider, visible: true }); }, close: () => { - set({ serverId: null, provider: null }); + set({ visible: false }); }, }));