Skip to content

Commit 6a06a13

Browse files
committed
fix(trigger): avoid flushSync for synchronous-call dedup
`internalTriggerOpen` wrapped `setInternalOpen` / `onOpenChange` / `onPopupVisibleChange` in `flushSync` (introduced in #601) to dedup within a single user interaction batch, because reading `mergedOpen` between two synchronous calls would otherwise see the stale value. Under React 19 that emits flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. whenever `internalTriggerOpen` is reached from inside a render/commit — for example clicking a `<Tooltip trigger="focus">`-wrapped button that opens a Modal: the click updates Modal state (entering React's render phase) and the focus event in the same batch routes into Trigger's `internalTriggerOpen`, so `flushSync` fires mid-render. Replace the flushSync gate with a single `useRef` that tracks the last synchronously dispatched `nextOpen`, plus a `useLayoutEffect` that syncs that ref to `mergedOpen` after each commit so controlled updates from outside (and the `lastTriggerRef`-leak case #601 originally fixed) remain handled without depending on a render reset. Adds `tests/no-flush-sync-warning.test.tsx` covering: - No `flushSync was called from inside a lifecycle` warning when open is triggered from inside a commit (the antd#57789 scenario). - Structural guard: `src/index.tsx` no longer imports or calls `flushSync`. Existing `tests/open-change.test.tsx` (the dedup coverage added in #601) keeps passing, so the synchronous pointer+focus / pointerleave+ blur dedup behaviour is preserved. Refs ant-design/ant-design#57789
1 parent 220358d commit 6a06a13

2 files changed

Lines changed: 169 additions & 8 deletions

File tree

src/index.tsx

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ export type {
4141

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

4645
export { UniqueProvider };
4746
export type { UniqueProviderProps };
@@ -382,14 +381,34 @@ export function generateTrigger(
382381
const openRef = React.useRef(mergedOpen);
383382
openRef.current = mergedOpen;
384383

384+
// Track the last synchronously dispatched `nextOpen` so multiple events
385+
// firing in the same batch (e.g. `pointerenter` + `focus`, or `pointerleave`
386+
// + `blur`) only emit one `onOpenChange`. We can't read `mergedOpen` here
387+
// because React state updates are async — within a single batch the second
388+
// call would still see the stale value. A simple `useRef` avoids that
389+
// without requiring `flushSync`, which would emit a React 19 warning when
390+
// `internalTriggerOpen` is reached from inside a render/lifecycle (e.g.
391+
// a child commit triggered by clicking a `<Tooltip trigger="focus">`-
392+
// wrapped button that also opens a Modal).
393+
// See https://github.com/ant-design/ant-design/issues/57789
394+
const lastDispatchedOpenRef = React.useRef(mergedOpen);
395+
396+
// Keep the ref in sync with `mergedOpen` after each render so that
397+
// controlled updates from outside (or any internal state change that
398+
// already committed) reset the dedup baseline. This preserves the
399+
// behaviour fixed in #601 where the dedup state could leak across user
400+
// interactions in controlled mode without re-renders.
401+
useLayoutEffect(() => {
402+
lastDispatchedOpenRef.current = mergedOpen;
403+
}, [mergedOpen]);
404+
385405
const internalTriggerOpen = useEvent((nextOpen: boolean) => {
386-
flushSync(() => {
387-
if (mergedOpen !== nextOpen) {
388-
setInternalOpen(nextOpen);
389-
onOpenChange?.(nextOpen);
390-
onPopupVisibleChange?.(nextOpen);
391-
}
392-
});
406+
if (lastDispatchedOpenRef.current !== nextOpen) {
407+
lastDispatchedOpenRef.current = nextOpen;
408+
setInternalOpen(nextOpen);
409+
onOpenChange?.(nextOpen);
410+
onPopupVisibleChange?.(nextOpen);
411+
}
393412
});
394413

395414
// Trigger for delay
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/**
2+
* Regression coverage for https://github.com/ant-design/ant-design/issues/57789
3+
*
4+
* Trigger used to wrap `setInternalOpen` / `onOpenChange` in `flushSync` for
5+
* synchronous-call dedup (introduced in #601). React 19 warns
6+
*
7+
* `flushSync was called from inside a lifecycle method. React cannot flush
8+
* when React is already rendering.`
9+
*
10+
* whenever `internalTriggerOpen` is reached from inside a render/commit phase
11+
* — e.g. clicking a button wrapped by `<Tooltip trigger="focus">` that also
12+
* opens a Modal: the click handler updates Modal state (entering React's
13+
* render phase), the focus event in the same batch routes into Trigger's
14+
* `internalTriggerOpen`, and `flushSync` then fires inside the render.
15+
*
16+
* This test pins the fix: opening a Trigger while React is mid-commit must
17+
* not emit the warning.
18+
*/
19+
import { act, cleanup, fireEvent, render } from '@testing-library/react';
20+
import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook';
21+
import * as React from 'react';
22+
import Trigger from '../src';
23+
24+
const flush = async () => {
25+
for (let i = 0; i < 10; i += 1) {
26+
act(() => {
27+
jest.runAllTimers();
28+
});
29+
await act(async () => {
30+
await Promise.resolve();
31+
});
32+
}
33+
};
34+
35+
describe('Trigger.NoFlushSyncWarning', () => {
36+
let eleRect = { width: 100, height: 100 };
37+
let spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
38+
let popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
39+
40+
beforeAll(() => {
41+
spyElementPrototypes(HTMLElement, {
42+
clientWidth: { get: () => eleRect.width },
43+
clientHeight: { get: () => eleRect.height },
44+
offsetWidth: { get: () => eleRect.width },
45+
offsetHeight: { get: () => eleRect.height },
46+
offsetParent: { get: () => document.body },
47+
});
48+
spyElementPrototypes(HTMLDivElement, {
49+
getBoundingClientRect() {
50+
return popupRect;
51+
},
52+
});
53+
spyElementPrototypes(HTMLSpanElement, {
54+
getBoundingClientRect() {
55+
return spanRect;
56+
},
57+
});
58+
});
59+
60+
beforeEach(() => {
61+
eleRect = { width: 100, height: 100 };
62+
spanRect = { x: 0, y: 0, left: 0, top: 0, width: 1, height: 1 };
63+
popupRect = { x: 0, y: 0, left: 0, top: 0, width: 100, height: 100 };
64+
jest.useFakeTimers();
65+
});
66+
67+
afterEach(() => {
68+
cleanup();
69+
jest.useRealTimers();
70+
});
71+
72+
it('does not emit a flushSync warning when open is triggered from inside a render/commit', async () => {
73+
// Spy console.error so we can fail the test on the React 19 flushSync warning.
74+
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
75+
76+
// Sibling whose commit cycle "owns" the render: when its `count` state
77+
// increases it re-renders, and within that commit we synchronously fire a
78+
// focus event on the Trigger target. That mirrors the Modal+Tooltip case
79+
// in #57789 where focus handling lands inside React's render phase.
80+
const Reproducer: React.FC = () => {
81+
const targetRef = React.useRef<HTMLSpanElement>(null);
82+
const [count, setCount] = React.useState(0);
83+
84+
React.useEffect(() => {
85+
if (count === 1 && targetRef.current) {
86+
// Synchronously focus the trigger target while we are still inside
87+
// an effect that ran during commit.
88+
targetRef.current.focus();
89+
}
90+
}, [count]);
91+
92+
return (
93+
<>
94+
<button
95+
type="button"
96+
className="opener"
97+
onClick={() => setCount((c) => c + 1)}
98+
>
99+
open
100+
</button>
101+
<Trigger action={['focus']} popup={<strong>popup</strong>}>
102+
<span className="target" ref={targetRef} tabIndex={0} />
103+
</Trigger>
104+
</>
105+
);
106+
};
107+
108+
const { container } = render(<Reproducer />);
109+
110+
act(() => {
111+
fireEvent.click(container.querySelector('.opener') as HTMLButtonElement);
112+
});
113+
114+
await flush();
115+
116+
const flushSyncWarnings = errorSpy.mock.calls.filter((call) =>
117+
String(call[0]).includes('flushSync was called from inside a lifecycle'),
118+
);
119+
expect(flushSyncWarnings).toEqual([]);
120+
121+
errorSpy.mockRestore();
122+
});
123+
124+
it('does not import flushSync from react-dom (structural guard)', () => {
125+
// Soft guard: if anyone re-introduces flushSync in src/index.tsx the
126+
// structural intent of this fix should be reviewed alongside #57789.
127+
// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require
128+
const fs = require('node:fs') as typeof import('node:fs');
129+
const path = require('node:path') as typeof import('node:path');
130+
const source = fs.readFileSync(
131+
path.resolve(__dirname, '../src/index.tsx'),
132+
'utf8',
133+
);
134+
// Strip block + line comments so the explanatory comment that *mentions*
135+
// flushSync doesn't trip the guard.
136+
const code = source
137+
.replace(/\/\*[\s\S]*?\*\//g, '')
138+
.replace(/(^|[^:])\/\/.*$/gm, '$1');
139+
expect(code).not.toMatch(/from\s+['"]react-dom['"]/);
140+
expect(code).not.toMatch(/\bflushSync\s*\(/);
141+
});
142+
});

0 commit comments

Comments
 (0)