diff --git a/docs/menus.md b/docs/menus.md index 580762d17..6cf31b342 100644 --- a/docs/menus.md +++ b/docs/menus.md @@ -37,6 +37,12 @@ and it loses. Selections used to be dropped entirely for exactly that reason. The consequence for callers: an `onSelect` on iOS runs a beat after the press. Don't add a second delay on top of it, and don't read state that the same press mutated. +This is why a row inside a menu surface goes through `selectItem` rather than calling its handler +from its own `Pressable`. `MenuItem` is not the only row shape on the engine — `ComposerTrackRow` +is another, with its own icons and hover-revealed actions — but both hand the press to the engine +and take `closeOnSelect` to say whether choosing them ends the surface. A row that owns its press +outright leaves the surface open behind whatever it opened, and skips the iOS wait. + ## Pages A submenu is a page, declared as data on the surface and reached by a `MenuSubTrigger` whose `id` diff --git a/packages/app/e2e/browser/provider-subagents.real.spec.ts b/packages/app/e2e/browser/provider-subagents.real.spec.ts index 0b735c308..9141e4abc 100644 --- a/packages/app/e2e/browser/provider-subagents.real.spec.ts +++ b/packages/app/e2e/browser/provider-subagents.real.spec.ts @@ -89,6 +89,9 @@ test.describe("real provider subagent timelines", () => { } await rows.first().click(); + // Choosing a row is a menu selection: the track panel goes with it. + await expect(page.getByTestId("subagents-track-header-panel")).toBeHidden(); + const panel = page.getByTestId("provider-subagent-panel"); await expect(panel).toBeVisible({ timeout: 30_000 }); if (scenario.expectedSubtitle) { diff --git a/packages/app/src/composer/tracks-panel.browser.test.tsx b/packages/app/src/composer/tracks-panel.browser.test.tsx new file mode 100644 index 000000000..52833b9dc --- /dev/null +++ b/packages/app/src/composer/tracks-panel.browser.test.tsx @@ -0,0 +1,183 @@ +import React, { act, type ReactNode } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { Pressable, Text } from "react-native"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ComposerTrackPill, ComposerTrackRow } from "./tracks"; + +// App sources compile against the classic JSX runtime, which expects React on the global. +beforeEach(() => vi.stubGlobal("React", React)); + +/** + * The real menu engine, in a real browser, because both things under test only exist there: the + * panel's dismissal runs through `selectItem`, and the running mark is a Web Animations rotation + * that jsdom cannot report. + */ + +interface Mounted { + root: Root; + container: HTMLDivElement; +} + +const mounted: Mounted[] = []; + +function mount(node: ReactNode): HTMLDivElement { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + act(() => root.render(node)); + mounted.push({ root, container }); + return container; +} + +afterEach(() => { + for (const entry of mounted.splice(0)) { + act(() => entry.root.unmount()); + entry.container.remove(); + } +}); + +function click(element: Element): void { + act(() => { + (element as HTMLElement).click(); + }); +} + +function pill(): HTMLElement { + const trigger = document.querySelector('[data-testid="pill"]'); + if (!(trigger instanceof HTMLElement)) { + throw new Error("track pill did not render"); + } + return trigger; +} + +function openPanel(): void { + click(pill()); +} + +function row(testID: string): Element | null { + return document.querySelector(`[data-testid="${testID}"]`); +} + +function isPanelOpen(): boolean { + return pill().getAttribute("aria-expanded") === "true"; +} + +describe("composer track panel", () => { + it("dismisses itself when a row is chosen", () => { + const onPress = vi.fn(); + mount( + + + Subagent one + + , + ); + + openPanel(); + const target = row("row"); + expect(target).not.toBeNull(); + + click(target as Element); + + expect(onPress).toHaveBeenCalledTimes(1); + expect(isPanelOpen()).toBe(false); + expect(row("row")).toBeNull(); + }); + + it("stays open for a row whose result lands in the panel", () => { + const onPress = vi.fn(); + mount( + + + Archive finished + + , + ); + + openPanel(); + click(row("row") as Element); + + expect(onPress).toHaveBeenCalledTimes(1); + expect(isPanelOpen()).toBe(true); + expect(row("row")).not.toBeNull(); + }); + + it("leaves the panel alone when a row's own action button is pressed", () => { + const onPress = vi.fn(); + const onAction = vi.fn(); + mount( + + + Subagent one + + Archive + + + , + ); + + openPanel(); + click(row("row-archive") as Element); + + expect(onAction).toHaveBeenCalledTimes(1); + expect(onPress).not.toHaveBeenCalled(); + expect(isPanelOpen()).toBe(true); + }); +}); + +describe("composer track pill status mark", () => { + function mountMark(statusBucket: "running" | "failed"): HTMLElement { + const container = mount( + + rows + , + ); + const mark = container.querySelector('[data-testid="pill"] > div'); + if (!(mark instanceof HTMLElement)) { + throw new Error("pill did not render a status mark"); + } + return mark; + } + + it("spins the shared ring while a child is running", () => { + const mark = mountMark("running"); + const animated = [...mark.querySelectorAll("*")].filter( + (element) => element.getAnimations().length > 0, + ); + + expect(animated).toHaveLength(1); + // The rotation is on the carrier; the quarter arc it turns is the coloured top border inside. + const arc = animated[0]?.firstElementChild as HTMLElement; + const arcStyle = getComputedStyle(arc); + expect(arcStyle.borderTopColor).toBe("rgb(38, 138, 224)"); + expect(arcStyle.borderLeftColor).toBe("rgba(0, 0, 0, 0)"); + }); + + it("draws a still dot for every other state", () => { + const mark = mountMark("failed"); + const animated = [...mark.querySelectorAll("*")].filter( + (element) => element.getAnimations().length > 0, + ); + + expect(animated).toHaveLength(0); + const dot = mark.firstElementChild as HTMLElement; + expect(getComputedStyle(dot).backgroundColor).toBe("rgb(241, 46, 47)"); + }); + + it("reserves one slot whatever the state, so the label never steps sideways", () => { + const running = mountMark("running").getBoundingClientRect().width; + const failed = mountMark("failed").getBoundingClientRect().width; + + expect(running).toBe(failed); + }); +}); diff --git a/packages/app/src/composer/tracks.test.tsx b/packages/app/src/composer/tracks.test.tsx index 1c7c95dc5..e8b0d73f5 100644 --- a/packages/app/src/composer/tracks.test.tsx +++ b/packages/app/src/composer/tracks.test.tsx @@ -12,6 +12,9 @@ vi.mock("react-native", () => ({ }, Pressable: "button", Text: "span", + // The status ring reaches for plain React Native styles rather than Unistyles ones — see the + // note on `rotatorStyles` in status-ring/frame.tsx. + StyleSheet: { create: (styles: T): T => styles }, View: ({ children, style, diff --git a/packages/app/src/composer/tracks.tsx b/packages/app/src/composer/tracks.tsx index 976717822..a964d4b3c 100644 --- a/packages/app/src/composer/tracks.tsx +++ b/packages/app/src/composer/tracks.tsx @@ -8,8 +8,11 @@ import { useMenuContext, type MenuTriggerState, } from "@/components/ui/menu"; +import { StatusRing } from "@/components/status-ring"; +import { STATUS_RING_FRAME_SIZE } from "@/components/status-ring/geometry"; import { MAX_CONTENT_WIDTH } from "@/constants/layout"; import { isWeb } from "@/constants/platform"; +import { getStatusDotColor } from "@/utils/status-dot-color"; import { STATUS_INDICATOR_FILLED_DOT_SIZE } from "@/utils/status-indicator-geometry"; import type { SidebarStateBucket } from "@/utils/sidebar-agent-state"; import { COMPOSER_PILL_CLEARANCE, composerPillStyles } from "./pill-styles"; @@ -43,7 +46,7 @@ export interface ComposerTrackPillProps { panelTitle: string; testID: string; accessibilityLabel?: string; - /** Drawn as a leading dot. `null` leaves the pill text-only. */ + /** Drawn as a leading mark — a dot, or the running ring. `null` leaves the pill text-only. */ statusBucket?: SidebarStateBucket | null; /** Panel body. Rendered into a popover on wide screens and a sheet on compact ones. */ children: ReactNode; @@ -135,7 +138,7 @@ function ComposerTrackPillTrigger({ {...ariaExpandedProps} style={pillStyle} > - + {label} @@ -148,6 +151,12 @@ export interface ComposerTrackRowProps { children: ReactNode | ((state: { active: boolean }) => ReactNode); /** Rows that open something are pressable and fill on press or hover. A read-only row is not. */ onPress?: () => void; + /** + * Dismiss the panel when this row is chosen. Same name, same default as `MenuItem`, because it + * is the same decision: a row that navigates away is done with the panel, a row whose result + * lands inside the panel is not. + */ + closeOnSelect?: boolean; disabled?: boolean; accessibilityLabel?: string; testID?: string; @@ -160,17 +169,27 @@ export interface ComposerTrackRowProps { * * Hover lives on the outer plain View and press on the inner Pressable, per docs/hover.md: rows * carry action buttons, and a Pressable tracking its own hover fights every Pressable inside it. + * + * A press is a menu selection, not a bare callback: the panel is a menu surface, so dismissal — + * and on iOS the wait for UIKit to finish tearing the sheet down before the action runs — belongs + * to the engine that opened it. The row only decides whether choosing it ends the panel. */ export function ComposerTrackRow({ children, onPress, + closeOnSelect = true, disabled = false, accessibilityLabel, testID, }: ComposerTrackRowProps): ReactElement { + const { selectItem } = useMenuContext("ComposerTrackRow"); const [hovered, setHovered] = useState(false); const handlePointerEnter = useCallback(() => setHovered(true), []); const handlePointerLeave = useCallback(() => setHovered(false), []); + const handleSelect = useCallback( + () => selectItem(onPress, closeOnSelect), + [closeOnSelect, onPress, selectItem], + ); const renderRow = useCallback( (active: boolean) => ( @@ -196,7 +215,7 @@ export function ComposerTrackRow({ accessibilityLabel={accessibilityLabel} testID={testID} disabled={disabled} - onPress={onPress} + onPress={handleSelect} > {renderPressed} @@ -204,23 +223,27 @@ export function ComposerTrackRow({ ); } -function ComposerTrackDot({ bucket }: { bucket: SidebarStateBucket | null }): ReactElement | null { - const colorStyle = bucket ? dotColorStyle(bucket) : null; - const dotStyle = useMemo(() => [styles.dot, colorStyle], [colorStyle]); - if (!colorStyle) { +/** + * The pill's leading state mark. Running is the ring every other running indicator in the app + * uses; the rest are the dot it grows from. + */ +function ComposerTrackMark({ bucket }: { bucket: SidebarStateBucket | null }): ReactElement | null { + if (!bucket) { return null; } - return ; + return ( + + {bucket === "running" ? : } + + ); } -function dotColorStyle(bucket: SidebarStateBucket) { +function dotColorStyle(bucket: Exclude) { switch (bucket) { case "needs_input": return styles.dotNeedsInput; case "failed": return styles.dotFailed; - case "running": - return styles.dotRunning; case "attention": return styles.dotAttention; case "done": @@ -228,67 +251,71 @@ function dotColorStyle(bucket: SidebarStateBucket) { } } -const styles = StyleSheet.create((theme) => ({ - bar: { - position: "absolute", - left: 0, - right: 0, - bottom: 0, - alignItems: "center", - paddingHorizontal: theme.spacing[4], - paddingBottom: { - xs: COMPOSER_PILL_CLEARANCE.compact, - md: COMPOSER_PILL_CLEARANCE.wide, - }, - }, - track: { - width: "100%", - maxWidth: MAX_CONTENT_WIDTH, - flexDirection: "row", - alignItems: "center", - gap: theme.spacing[1], - }, - // The rail every panel row sits on: inset from the panel edge so the fill is a rounded block - // inside it, and tall enough that revealing an action button cannot resize the row. - row: { - flexDirection: "row", - alignItems: "center", - gap: theme.spacing[2], - minHeight: 32, - marginHorizontal: theme.spacing[1], - paddingHorizontal: theme.spacing[2], - paddingVertical: theme.spacing[1], - borderRadius: theme.borderRadius.md, - }, - rowActive: { - flexDirection: "row", - alignItems: "center", - gap: theme.spacing[2], - minHeight: 32, - marginHorizontal: theme.spacing[1], - paddingHorizontal: theme.spacing[2], - paddingVertical: theme.spacing[1], - borderRadius: theme.borderRadius.md, - backgroundColor: theme.colors.surface2, - }, - dot: { +const styles = StyleSheet.create((theme) => { + // Colours come from the one bucket-to-colour map so the pill cannot drift from the status dots + // everywhere else, and are baked into each variant so the style prop stays a stable object. + const statusDot = (bucket: Exclude) => ({ width: STATUS_INDICATOR_FILLED_DOT_SIZE, height: STATUS_INDICATOR_FILLED_DOT_SIZE, borderRadius: theme.borderRadius.full, - }, - dotNeedsInput: { - backgroundColor: theme.colors.statusDotWarning, - }, - dotFailed: { - backgroundColor: theme.colors.statusDotDanger, - }, - dotRunning: { - backgroundColor: theme.colors.statusDotRunning, - }, - dotAttention: { - backgroundColor: theme.colors.statusDotSuccess, - }, - dotDone: { - backgroundColor: theme.colors.border, - }, -})); + backgroundColor: getStatusDotColor({ theme, bucket, showDoneAsInactive: true }) ?? undefined, + }); + + return { + bar: { + position: "absolute", + left: 0, + right: 0, + bottom: 0, + alignItems: "center", + paddingHorizontal: theme.spacing[4], + paddingBottom: { + xs: COMPOSER_PILL_CLEARANCE.compact, + md: COMPOSER_PILL_CLEARANCE.wide, + }, + }, + track: { + width: "100%", + maxWidth: MAX_CONTENT_WIDTH, + flexDirection: "row", + alignItems: "center", + gap: theme.spacing[1], + }, + // The rail every panel row sits on: inset from the panel edge so the fill is a rounded block + // inside it, and tall enough that revealing an action button cannot resize the row. + row: { + flexDirection: "row", + alignItems: "center", + gap: theme.spacing[2], + minHeight: 32, + marginHorizontal: theme.spacing[1], + paddingHorizontal: theme.spacing[2], + paddingVertical: theme.spacing[1], + borderRadius: theme.borderRadius.md, + }, + rowActive: { + flexDirection: "row", + alignItems: "center", + gap: theme.spacing[2], + minHeight: 32, + marginHorizontal: theme.spacing[1], + paddingHorizontal: theme.spacing[2], + paddingVertical: theme.spacing[1], + borderRadius: theme.borderRadius.md, + backgroundColor: theme.colors.surface2, + }, + // Sized for the widest mark that can land in it, which is the ring rather than the dot. The + // slot is the same width whatever the state, so the label does not step sideways when the last + // child stops running. + mark: { + width: STATUS_RING_FRAME_SIZE, + height: STATUS_RING_FRAME_SIZE, + alignItems: "center", + justifyContent: "center", + }, + dotNeedsInput: statusDot("needs_input"), + dotFailed: statusDot("failed"), + dotAttention: statusDot("attention"), + dotDone: statusDot("done"), + }; +}); diff --git a/packages/app/src/subagents/track.tsx b/packages/app/src/subagents/track.tsx index a98f70ae3..940a2cab5 100644 --- a/packages/app/src/subagents/track.tsx +++ b/packages/app/src/subagents/track.tsx @@ -155,6 +155,9 @@ function ArchiveFinishedRow({ accessibilityLabel={t("subagents.archiveFinishedAction")} testID="subagents-track-archive-finished" disabled={disabled} + // Progress and the retry count land on this row, so the panel is where the result of + // pressing it shows up. Dismissing would hide the thing the press produces. + closeOnSelect={false} onPress={onPress} > {renderRow} diff --git a/packages/app/test-stubs/gorhom-bottom-sheet.ts b/packages/app/test-stubs/gorhom-bottom-sheet.ts new file mode 100644 index 000000000..5be5f7b5f --- /dev/null +++ b/packages/app/test-stubs/gorhom-bottom-sheet.ts @@ -0,0 +1,13 @@ +// The package ships untranspiled Flow, which the unit project cannot parse. The sheet is a native +// presentation; tests that reach a menu do so through the popover, so rendering nothing is enough +// to keep the module graph importable. +const Stub = () => null; +const PassThrough = ({ children }: { children?: unknown }) => children; + +export default Stub; +export const BottomSheetModal = Stub; +export const BottomSheetModalProvider = PassThrough; +export const BottomSheetBackdrop = Stub; +export const BottomSheetScrollView = PassThrough; +export const BottomSheetView = PassThrough; +export const BottomSheetTextInput = Stub; diff --git a/packages/app/test-stubs/lucide-react-native.ts b/packages/app/test-stubs/lucide-react-native.ts index a7f4f76b1..716a0e2d6 100644 --- a/packages/app/test-stubs/lucide-react-native.ts +++ b/packages/app/test-stubs/lucide-react-native.ts @@ -22,6 +22,7 @@ export const Blocks = StubIcon; export const Bot = StubIcon; export const Brain = StubIcon; export const Check = StubIcon; +export const CheckCircle = StubIcon; export const CheckCircle2 = StubIcon; export const ChevronDown = StubIcon; export const ChevronLeft = StubIcon; diff --git a/packages/app/test-stubs/react-native-fabric-shim.ts b/packages/app/test-stubs/react-native-fabric-shim.ts new file mode 100644 index 000000000..344607a33 --- /dev/null +++ b/packages/app/test-stubs/react-native-fabric-shim.ts @@ -0,0 +1,8 @@ +// Reanimated's native code path imports `react-native/Libraries/Renderer/shims/ReactFabric`. +// The `react-native` alias below points at a react-native-web file, so the subpath cannot +// resolve; on web reanimated takes `findHostInstance.web.js` and never reaches this. +export function findHostInstance_DEPRECATED(): null { + return null; +} + +export default { findHostInstance_DEPRECATED }; diff --git a/packages/app/test-stubs/react-native-safe-area-context.ts b/packages/app/test-stubs/react-native-safe-area-context.ts new file mode 100644 index 000000000..ae793cd92 --- /dev/null +++ b/packages/app/test-stubs/react-native-safe-area-context.ts @@ -0,0 +1,12 @@ +// The package ships untranspiled Flow, which the unit project cannot parse. Insets are a device +// fact no test asserts on, so they are zero. +const ZERO_INSETS = { top: 0, right: 0, bottom: 0, left: 0 } as const; + +export const useSafeAreaInsets = () => ZERO_INSETS; +export const useSafeAreaFrame = () => ({ x: 0, y: 0, width: 0, height: 0 }); +export const SafeAreaProvider = ({ children }: { children?: unknown }) => children; +export const SafeAreaView = ({ children }: { children?: unknown }) => children; +export const initialWindowMetrics = { + insets: ZERO_INSETS, + frame: { x: 0, y: 0, width: 0, height: 0 }, +}; diff --git a/packages/app/test-stubs/react-native-unistyles.ts b/packages/app/test-stubs/react-native-unistyles.ts index 9b7015334..1484e7a7d 100644 --- a/packages/app/test-stubs/react-native-unistyles.ts +++ b/packages/app/test-stubs/react-native-unistyles.ts @@ -7,6 +7,11 @@ const testTheme = { statusDanger: "#b91c1c", statusWarning: "#d97706", statusMerged: "#7c3aed", + // The light band's values, so a test can name the colour it expects. + statusDotSuccess: "#299f51", + statusDotDanger: "#f12e2f", + statusDotWarning: "#b37824", + statusDotRunning: "#268ae0", accent: "#2563eb", accentForeground: "#ffffff", destructive: "#dc2626", @@ -19,6 +24,7 @@ const testTheme = { palette: { amber: { 500: "#f59e0b" }, blue: { 300: "#93c5fd" }, + green: { 500: "#22c55e" }, red: { 300: "#fca5a5" }, white: "#ffffff", }, @@ -45,8 +51,28 @@ const testTheme = { xl: 12, full: 9999, }, - iconSize: { sm: 16 }, + iconSize: { sm: 16, md: 20 }, opacity: { 50: 0.5 }, + shadow: { + sm: { + shadowColor: "rgba(0, 0, 0, 0.02)", + shadowOffset: { width: 0, height: 2 }, + shadowRadius: 8, + elevation: 2, + }, + md: { + shadowColor: "rgba(0, 0, 0, 0.04)", + shadowOffset: { width: 0, height: 4 }, + shadowRadius: 16, + elevation: 4, + }, + lg: { + shadowColor: "rgba(0, 0, 0, 0.08)", + shadowOffset: { width: 0, height: 8 }, + shadowRadius: 24, + elevation: 8, + }, + }, }; type StyleFactory = (theme: typeof testTheme) => T; diff --git a/packages/app/test-stubs/reanimated-validate-worklets-version.ts b/packages/app/test-stubs/reanimated-validate-worklets-version.ts new file mode 100644 index 000000000..b0247ab2b --- /dev/null +++ b/packages/app/test-stubs/reanimated-validate-worklets-version.ts @@ -0,0 +1,3 @@ +// Reanimated imports this CJS script with a default import. Unbundled, the browser cannot interop +// it, and the check it performs is a native-build concern with nothing to say in a browser test. +export default function validateWorkletsVersion(): void {} diff --git a/packages/app/vitest.config.ts b/packages/app/vitest.config.ts index fa3ec72e0..5c1b82060 100644 --- a/packages/app/vitest.config.ts +++ b/packages/app/vitest.config.ts @@ -60,6 +60,20 @@ export default defineConfig({ }, }, }, + // Reanimated ships one file per platform and picks between them by extension + // (`findHostInstance.web.js`). Vite's dependency optimizer does not apply `resolve.extensions`, + // so it scans the native files and dies on imports react-native-web has no answer for. + // Unbundled, the same imports go through the resolver below and land on the web files. + optimizeDeps: { + exclude: ["react-native-reanimated"], + }, + // The globals a React Native bundler defines, which esbuild is no longer there to supply for + // the package excluded above. + define: { + "process.env.JEST_WORKER_ID": "undefined", + __DEV__: "false", + global: "globalThis", + }, resolve: { extensions: [ ".web.mjs", @@ -86,6 +100,13 @@ export default defineConfig({ replacement: path.resolve(__dirname, "../relay/src/index.ts"), }, { find: "@", replacement: path.resolve(__dirname, "src") }, + // Must precede the `react-native` alias: a string `find` matches by prefix, so this subpath + // would otherwise resolve inside a react-native-web *file* and break the dependency scan. + // Reanimated only imports it on the native path, which no test takes. + { + find: /^react-native\/Libraries\/Renderer\/shims\/ReactFabric$/, + replacement: path.resolve(__dirname, "test-stubs/react-native-fabric-shim.ts"), + }, // Point to the ESM build so Vite can transform its imports and apply the // react alias below (the CJS build uses require('react') which bypasses // Vite alias resolution). @@ -114,6 +135,20 @@ export default defineConfig({ find: /^react-native-svg$/, replacement: path.resolve(__dirname, "test-stubs/react-native-svg.ts"), }, + // Both ship untranspiled Flow and fail to parse on import, which takes out any test that + // mounts a menu surface. + { + find: /^react-native-safe-area-context$/, + replacement: path.resolve(__dirname, "test-stubs/react-native-safe-area-context.ts"), + }, + { + find: /^@gorhom\/bottom-sheet$/, + replacement: path.resolve(__dirname, "test-stubs/gorhom-bottom-sheet.ts"), + }, + { + find: /^react-native-reanimated\/scripts\/validate-worklets-version$/, + replacement: path.resolve(__dirname, "test-stubs/reanimated-validate-worklets-version.ts"), + }, { find: /^expo-linking$/, replacement: path.resolve(__dirname, "test-stubs/expo-linking.ts"),