Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export type {

import UniqueProvider, { type UniqueProviderProps } from './UniqueProvider';
import { useControlledState } from '@rc-component/util';
import { flushSync } from 'react-dom';

export { UniqueProvider };
export type { UniqueProviderProps };
Expand Down Expand Up @@ -386,14 +385,53 @@ export function generateTrigger(
const openRef = React.useRef(mergedOpen);
openRef.current = mergedOpen;

// 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) => {
flushSync(() => {
if (rawOpen !== nextOpen) {
setInternalOpen(nextOpen);
onOpenChange?.(nextOpen);
onPopupVisibleChange?.(nextOpen);
}
});
if (lastDispatchRef.current === nextOpen) {
return;
}
lastDispatchRef.current = nextOpen;
setInternalOpen(nextOpen);
onOpenChange?.(nextOpen);
onPopupVisibleChange?.(nextOpen);
});

// Trigger for delay
Expand Down
206 changes: 206 additions & 0 deletions tests/concurrent-render.test.tsx
Original file line number Diff line number Diff line change
@@ -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;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

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);
}
});
});
Loading