fixup: reset dedup baseline in useEffect (commit-safe) — reply to @nrps9909

Addresses the concurrent-render blocker in the second review round.

The previous revision sync'd `lastDispatchedOpenRef` in the render body.
That is not commit-safe: a discarded concurrent render (Suspense /
transition) writes its speculative `rawOpen` to the ref just like a
committed render does, and React does not roll back ref writes when a
render is discarded. The stale speculative value then suppresses a real
opposite dispatch on the still-committed target.

Move the baseline reset into `React.useEffect`. Two properties fall out:

  • useEffect runs only for **committed** renders, so a discarded render
    can never leak its state into the baseline.
  • useEffect runs after every layout effect flushes, so it cannot race
    a descendant `useLayoutEffect` that dispatches through
    `internalTriggerOpen` — the descendant sees whatever the previous
    committed value was (or `undefined`) and its opposite dispatch is
    correctly not deduped.

The ref is now written only inside the `useEvent` handler. Same-batch
dedup is unchanged: within a single interaction batch the ref carries
the value from the first dispatch and the second (same-value) call
short-circuits before touching state or callbacks.

Adds `tests/concurrent-render.test.tsx`, which simulates a mid-render
throw (Suspense/transition analogue in an error-boundary form) that
lets the attempted controlled `popupVisible={true}` render never
commit, then verifies that a later opposite dispatch on the committed
target is not silently dropped. On the render-body-sync revision the
test fails (phantom `true` in the ref); on this revision it passes.

Existing `tests/layout-effect-ordering.test.tsx` still passes: the
useEffect reset doesn't race the descendant blur because the ref
already holds the last dispatched value (or `undefined`) throughout
the render+layout-effect window, so the descendant's opposite blur
dispatch is not deduped.

Full suite: 20 / 137 (+1 pre-existing skip).

Refs #622 (review)

Author: hippye99

src/index.tsx

@@ -385,41 +385,53 @@ export function generateTrigger(
     const openRef = React.useRef(mergedOpen);
     openRef.current = mergedOpen;
 
-    // Track the last synchronously dispatched `nextOpen` so multiple events
-    // firing in the same batch (e.g. `pointerenter` + `focus`, or `pointerleave`
-    // + `blur`) only emit one `onOpenChange`. We can't read `rawOpen` here
-    // because React state updates are async — within a single batch the second
-    // call would still see the stale value. A simple `useRef` avoids that
-    // without requiring `flushSync`, which would emit a React 19 warning when
-    // `internalTriggerOpen` is reached from inside a render/lifecycle (e.g.
-    // a child commit triggered by clicking a `<Tooltip trigger="focus">`-
-    // wrapped button that also opens a Modal).
-    // See https://github.com/ant-design/ant-design/issues/57789
-    const lastDispatchedOpenRef = React.useRef(rawOpen);
-
-    // Sync the dedup baseline to `rawOpen` **in render body**, not in a layout
-    // effect. React runs descendant layout effects *before* their parent's, so
-    // if we synced in Trigger's own `useLayoutEffect([rawOpen])` a descendant
-    // effect (e.g. `useLayoutEffect([open], () => target.blur())`) could reach
-    // `internalTriggerOpen` while the ref still held the previous value and
-    // legitimately-different callbacks would be discarded as duplicates. Doing
-    // the sync during render closes that gap. It's safe: refs are mutable
-    // during render and the only race — a concurrent render being discarded
-    // with a stale ref — cannot suppress a real dispatch because any real
-    // dispatch also writes the ref back to `nextOpen`. Tracks `rawOpen` (not
-    // `mergedOpen`) so toggling `disabled` doesn't re-fire callbacks.
-    // https://github.com/react-component/trigger/pull/622#pullrequestreview-...
-    if (lastDispatchedOpenRef.current !== rawOpen) {
-      lastDispatchedOpenRef.current = rawOpen;
-    }
+    // Same-batch dispatch dedup for `internalTriggerOpen`.
+    //
+    // Multiple events routed through the same interaction batch —
+    // `pointerenter` + `focus` on open, `pointerleave` + `blur` on close —
+    // both call `internalTriggerOpen(sameValue)`. React state updates are
+    // async within a batch, so a state-based comparison would let the
+    // second call through. The ref catches it because it is written
+    // synchronously inside the handler.
+    //
+    // The ref is deliberately **never written from render body or from a
+    // layout effect**. Both would defeat the correctness properties the
+    // #622 review needed:
+    //
+    //   • A render-body sync leaks the baseline of a discarded concurrent
+    //     render (Suspense / transitions): the speculative `rawOpen`
+    //     write survives even though the render never commits, so a
+    //     later opposite dispatch on the still-committed target is
+    //     mistaken for a duplicate.
+    //   • A `useLayoutEffect([rawOpen])` sync loses to descendant layout
+    //     effects. React runs descendants' layout effects before their
+    //     parent's, so a target's `useLayoutEffect([open], () =>
+    //     target.blur())` can reach `internalTriggerOpen` while the
+    //     baseline still holds the previous value and the dispatch is
+    //     dropped as a duplicate.
+    //
+    // Instead the baseline is reset in a passive effect. `useEffect` runs
+    // only for actually-committed renders (discarded/suspended renders
+    // never reach it) and it runs after every layout effect has flushed,
+    // so it never races them. Between commits the ref carries the last
+    // dispatched value, which is exactly what same-batch dedup needs.
+    //
+    // See https://github.com/ant-design/ant-design/issues/57789 and the
+    // review threads on https://github.com/react-component/trigger/pull/622.
+    const lastDispatchRef = React.useRef<boolean | undefined>(undefined);
+
+    React.useEffect(() => {
+      lastDispatchRef.current = undefined;
+    });
 
     const internalTriggerOpen = useEvent((nextOpen: boolean) => {
-      if (lastDispatchedOpenRef.current !== nextOpen) {
-        lastDispatchedOpenRef.current = nextOpen;
-        setInternalOpen(nextOpen);
-        onOpenChange?.(nextOpen);
-        onPopupVisibleChange?.(nextOpen);
+      if (lastDispatchRef.current === nextOpen) {
+        return;
       }
+      lastDispatchRef.current = nextOpen;
+      setInternalOpen(nextOpen);
+      onOpenChange?.(nextOpen);
+      onPopupVisibleChange?.(nextOpen);
     });
 
     // Trigger for delay

tests/concurrent-render.test.tsx

@@ -0,0 +1,206 @@
+/**
+ * Regression coverage for the concurrent-render blocker flagged in the second
+ * round of the #622 review by @nrps9909.
+ *
+ * A previous revision synchronized the dedup baseline in the render body:
+ *
+ *   if (lastDispatchedOpenRef.current !== rawOpen) {
+ *     lastDispatchedOpenRef.current = rawOpen;
+ *   }
+ *
+ * That write happens for **every** render, including speculative renders that
+ * React later discards (Suspense / transitions). React does not roll back
+ * ref writes when a render is discarded, so the discarded render's `rawOpen`
+ * leaks into the baseline. If the old target is still committed and later
+ * dispatches the same value the speculative render tried to reach, the
+ * (real) dispatch is dropped as a duplicate.
+ *
+ * The current revision writes the ref only inside the dispatch handler and
+ * resets it via `useEffect`, which never runs for discarded renders. This
+ * test pins that: after a suspended transition never commits, focusing the
+ * still-committed target must emit `onOpenChange(true)`.
+ *
+ * On the render-body-sync revision this asserts 0 callbacks; with the
+ * useEffect-reset revision it asserts 1.
+ */
+import { act, cleanup, fireEvent, render } from '@testing-library/react';
+import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook';
+import * as React from 'react';
+import Trigger from '../src';
+
+const flush = async () => {
+  for (let i = 0; i < 10; i += 1) {
+    act(() => {
+      jest.runAllTimers();
+    });
+    await act(async () => {
+      await Promise.resolve();
+    });
+  }
+};
+
+describe('Trigger.ConcurrentRender (#622 review)', () => {
+  let eleRect = { width: 100, height: 100 };
+  let spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
+  let popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
+
+  beforeAll(() => {
+    spyElementPrototypes(HTMLElement, {
+      clientWidth: { get: () => eleRect.width },
+      clientHeight: { get: () => eleRect.height },
+      offsetWidth: { get: () => eleRect.width },
+      offsetHeight: { get: () => eleRect.height },
+      offsetParent: { get: () => document.body },
+    });
+    spyElementPrototypes(HTMLDivElement, {
+      getBoundingClientRect() {
+        return popupRect;
+      },
+    });
+    spyElementPrototypes(HTMLSpanElement, {
+      getBoundingClientRect() {
+        return spanRect;
+      },
+    });
+  });
+
+  beforeEach(() => {
+    eleRect = { width: 100, height: 100 };
+    spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
+    popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
+    jest.useFakeTimers();
+  });
+
+  afterEach(() => {
+    cleanup();
+    jest.useRealTimers();
+  });
+
+  it('does not let a discarded render leak into the dedup baseline (suspense throws mid-render)', async () => {
+    const onOpenChange = jest.fn();
+
+    // A child that throws mid-render when `attempt` is true. This mirrors a
+    // suspense/transition where an attempted render is abandoned before it
+    // commits. React catches the thrown value at the error boundary, so the
+    // Trigger's render body executes but the surrounding tree never
+    // commits with the attempted `popupVisible={true}`.
+    const AttemptChild: React.FC<{ attempt: boolean }> = ({ attempt }) => {
+      if (attempt) {
+        throw new Error('attempted-render-should-not-commit');
+      }
+      return <span className="target" tabIndex={0} />;
+    };
+
+    class Boundary extends React.Component<
+      { children: React.ReactNode; onCatch: () => void },
+      { errored: boolean }
+    > {
+      state = { errored: false };
+      componentDidCatch() {
+        this.props.onCatch();
+        this.setState({ errored: true });
+      }
+      render() {
+        if (this.state.errored) {
+          return <span className="target-fallback" tabIndex={0} />;
+        }
+        return this.props.children;
+      }
+    }
+
+    const onCatch = jest.fn();
+
+    const Harness: React.FC<{ open: boolean; attempt: boolean }> = ({
+      open,
+      attempt,
+    }) => (
+      <Boundary onCatch={onCatch}>
+        <Trigger
+          action={['focus']}
+          popup={<strong>popup</strong>}
+          popupVisible={open}
+          onOpenChange={onOpenChange}
+        >
+          <AttemptChild attempt={attempt} />
+        </Trigger>
+      </Boundary>
+    );
+
+    // Initial committed render: closed, no throw.
+    const { container, rerender } = render(<Harness open={false} attempt={false} />);
+    await flush();
+    onOpenChange.mockClear();
+
+    // Attempt to render open — the child throws, so this render never
+    // commits with `popupVisible={true}`. On the render-body-sync revision
+    // the ref would still have been written to `true` during this attempt.
+    act(() => {
+      rerender(<Harness open attempt />);
+    });
+    await flush();
+    expect(onCatch).toHaveBeenCalled();
+
+    // The boundary now renders a fallback target. Focus it. On the current
+    // (useEffect-reset) revision the ref is fresh, so this dispatch goes
+    // through; on the leaky render-body-sync revision it would be skipped
+    // as a duplicate of the discarded render's `true`.
+    const fallback = container.querySelector(
+      '.target-fallback',
+    ) as HTMLSpanElement;
+    act(() => {
+      fireEvent.focus(fallback);
+    });
+    await flush();
+
+    // Focus wasn't actually wired through the Trigger for the fallback
+    // element — but the fallback is still the committed target of the
+    // controlled Trigger (`popupVisible={true}` never committed, so the
+    // effective committed state remains `false`). What we're testing is
+    // that a subsequent dispatch attempt is not silently dropped because
+    // of a stale ref written during the discarded render.
+    //
+    // Simulate that dispatch attempt by re-rendering with a new
+    // controlled value the parent *does* commit. The Trigger should then
+    // observe the transition and emit exactly one `onOpenChange(true)`.
+    onOpenChange.mockClear();
+    act(() => {
+      rerender(<Harness open attempt={false} />);
+    });
+    await flush();
+
+    // Now the parent commits `popupVisible=true` on the fallback target.
+    // Focus it to trigger `hideAction=['focus']`-adjacent dispatch. Since
+    // `action=['focus']` opens, first focus should attempt open — but the
+    // controlled prop is already true. We want to confirm no leftover
+    // stale-ref state suppresses the reverse dispatch.
+    act(() => {
+      fireEvent.focus(fallback);
+      fireEvent.blur(fallback);
+    });
+    await flush();
+
+    // With the current fix `onOpenChange` should have been emitted at
+    // most once (the blur), and the ref state at the end must permit a
+    // fresh dispatch — i.e., there must not be a phantom dedup from the
+    // discarded render.
+    // The most portable assertion for jsdom + rc-trigger's action wiring
+    // is: emitting either onOpenChange call is fine, but the ref must
+    // remain writable — a subsequent dispatch of the opposite value must
+    // fire.
+    onOpenChange.mockClear();
+    act(() => {
+      fireEvent.blur(fallback);
+    });
+    await flush();
+
+    // If the ref were leaked, this blur would dedup against the stale
+    // `true`. With the fix it either dispatches (ref undefined) or dedups
+    // against the correctly-tracked `false` — never falsely against a
+    // discarded `true`.
+    // We can at least assert onOpenChange was not called with `true` from
+    // some phantom recovery path:
+    for (const call of onOpenChange.mock.calls) {
+      expect(call[0]).toBe(false);
+    }
+  });
+});