fix(app): run menu selections that close the menu on iOS

Selecting an item parked its action in a ref and waited for the Modal's onDismiss to flush it. MenuOverlay returns null as soon as the menu closes, so the Modal unmounted and onDismiss never fired: every close-on-select action was dropped. Creating a terminal from the compact workspace menu did nothing at all. Wait on a timer instead. The grace period was always about giving UIKit time to tear the surface down before a native presenter launches, and a callback fired from inside a subtree being removed is racing its own removal. The sheet had no flush at all, so it was broken the same way for a different reason.
2026-08-04 12:03:00 +02:00
parent 56cb70fa59
commit bf6cd09d6c
5 changed files with 88 additions and 38 deletions
@@ -26,6 +26,17 @@ when you have actually looked at it on a phone.
 `ContextMenu` is the exception: it defaults to `compactMode="sheet"` and enables native long press.
 Disable mobile triggering explicitly on draggable rows, where long press belongs to drag instead.

+## Selecting an item on iOS
+
+An item that closes the menu runs its action after a fixed grace period on iOS, not immediately:
+a native presenter launched while UIKit is still tearing down the surface can hang. Both surfaces
+unmount the moment the menu closes, so the wait is a timer rather than the surface's own dismissal
+callback — a callback fired from inside a subtree that is being removed is racing its own removal,
+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.
+
 ## Pages

 A submenu is a page, declared as data on the surface and reached by a `MenuSubTrigger` whose `id`
@@ -0,0 +1,63 @@
+/**
+ * @vitest-environment jsdom
+ */
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { act, renderHook } from "@testing-library/react";
+import { Platform } from "react-native";
+import { IOS_TEARDOWN_GRACE_MS, useMenuState } from "./menu-context";
+
+/**
+ * `Platform.OS` is a plain property on the react-native-web double, so the iOS branch is
+ * reachable by writing to it. The grace period only exists on iOS, which is exactly the platform
+ * whose selections used to be dropped.
+ */
+const originalPlatform = Platform.OS;
+
+function setPlatform(os: string) {
+  Object.assign(Platform, { OS: os });
+}
+
+beforeEach(() => vi.useFakeTimers());
+
+afterEach(() => {
+  vi.useRealTimers();
+  setPlatform(originalPlatform);
+});
+
+describe("useMenuState selectItem on iOS", () => {
+  beforeEach(() => setPlatform("ios"));
+
+  it("runs a closing item's action once the surface has had time to go away", () => {
+    const onSelect = vi.fn();
+    const { result } = renderHook(() => useMenuState({ defaultOpen: true }));
+
+    act(() => result.current.selectItem(onSelect, true));
+    expect(result.current.open).toBe(false);
+    expect(onSelect).not.toHaveBeenCalled();
+
+    act(() => vi.advanceTimersByTime(IOS_TEARDOWN_GRACE_MS));
+    expect(onSelect).toHaveBeenCalledTimes(1);
+  });
+
+  it("leaves the menu open for an item that does not close it", () => {
+    const onSelect = vi.fn();
+    const { result } = renderHook(() => useMenuState({ defaultOpen: true }));
+
+    act(() => result.current.selectItem(onSelect, false));
+    expect(result.current.open).toBe(true);
+    expect(onSelect).toHaveBeenCalledTimes(1);
+  });
+});
+
+describe("useMenuState selectItem elsewhere", () => {
+  beforeEach(() => setPlatform("android"));
+
+  it("runs a closing item's action immediately", () => {
+    const onSelect = vi.fn();
+    const { result } = renderHook(() => useMenuState({ defaultOpen: true }));
+
+    act(() => result.current.selectItem(onSelect, true));
+    expect(result.current.open).toBe(false);
+    expect(onSelect).toHaveBeenCalledTimes(1);
+  });
+});
@@ -24,12 +24,14 @@ export type MenuPresentation = "popover" | "sheet";
 /** What a menu does on a compact screen. Wide screens are always a popover. */
 export type MenuCompactMode = "popover" | "sheet";

+/** How long UIKit is given to finish removing the menu surface before an action runs. */
+export const IOS_TEARDOWN_GRACE_MS = 250;
+
 export interface MenuContextValue {
  open: boolean;
  setOpen: (open: boolean) => void;
  /** Runs an item's action, closing the menu first when the item asks for it. */
  selectItem: (onSelect: (() => void) | undefined, closeOnSelect: boolean) => void;
-  flushPendingSelect: () => void;
  triggerRef: React.RefObject<View | null>;
  /** Point anchor for menus opened from a gesture rather than a trigger box. */
  anchorRect: Rect | null;
@@ -104,7 +106,6 @@ export function useMenuState({
  compactMode?: MenuCompactMode;
 }): MenuContextValue {
  const triggerRef = useRef<View>(null);
-  const pendingSelectRef = useRef<(() => void) | null>(null);
  const [anchorRect, setAnchorRect] = useState<Rect | null>(null);
  const [path, setPath] = useState<MenuPath>(MENU_ROOT_PATH);
  const isCompact = useIsCompactFormFactor();
@@ -126,21 +127,6 @@ export function useMenuState({
    [setIsOpenState],
  );

-  const flushPendingSelect = useCallback(() => {
-    const pendingSelect = pendingSelectRef.current;
-    pendingSelectRef.current = null;
-    if (!pendingSelect) return;
-
-    if (Platform.OS === "ios") {
-      // Native presenters such as PHPicker can hang if launched while an RN
-      // Modal is still completing dismissal on UIKit's side.
-      setTimeout(pendingSelect, 250);
-      return;
-    }
-
-    pendingSelect();
-  }, []);
-
  const selectItem = useCallback(
    (onSelect: (() => void) | undefined, closeOnSelect: boolean) => {
      if (!closeOnSelect) {
@@ -148,14 +134,19 @@ export function useMenuState({
        return;
      }

+      setOpen(false);
+      if (!onSelect) return;
+
      if (Platform.OS === "ios") {
-        pendingSelectRef.current = onSelect ?? null;
-        setOpen(false);
+        // Native presenters such as PHPicker can hang if launched while UIKit is still
+        // tearing down the surface this menu was drawn in. The wait is timed rather than
+        // driven by the surface's own dismissal callback: both surfaces unmount as soon as
+        // the menu closes, so any such callback is racing its own removal and usually loses.
+        setTimeout(onSelect, IOS_TEARDOWN_GRACE_MS);
        return;
      }

-      setOpen(false);
-      onSelect?.();
+      onSelect();
    },
    [setOpen],
  );
@@ -177,7 +168,6 @@ export function useMenuState({
      open: isOpen,
      setOpen,
      selectItem,
-      flushPendingSelect,
      triggerRef,
      anchorRect,
      setAnchorRect,
@@ -187,17 +177,6 @@ export function useMenuState({
      closeSub,
      goBack,
    }),
-    [
-      isOpen,
-      setOpen,
-      selectItem,
-      flushPendingSelect,
-      anchorRect,
-      presentation,
-      path,
-      openSub,
-      closeSub,
-      goBack,
-    ],
+    [isOpen, setOpen, selectItem, anchorRect, presentation, path, openSub, closeSub, goBack],
  );
 }
@@ -384,12 +384,10 @@ export function AnchoredSurface({
 export function MenuOverlay({
  visible,
  onClose,
-  onDismiss,
  children,
 }: {
  visible: boolean;
  onClose: () => void;
-  onDismiss?: () => void;
  children: ReactElement | null;
 }): ReactElement | null {
  const floatingLayer = useOverlayLayer("floating");
@@ -438,7 +436,6 @@ export function MenuOverlay({
      transparent
      animationType="none"
      statusBarTranslucent={Platform.OS === "android"}
-      onDismiss={onDismiss}
      onRequestClose={onClose}
    >
      {overlay}
@@ -206,7 +206,7 @@ function MenuPopoverSurface({

  return (
    <MenuSurfaceContext.Provider value={surfaceValue}>
-      <MenuOverlay visible={menu.open} onClose={handleClose} onDismiss={menu.flushPendingSelect}>
+      <MenuOverlay visible={menu.open} onClose={handleClose}>
        <>
          <AnchoredSurface
            open={menu.open}