From 6a06a1399332b37204451dd666500420eea3ee51 Mon Sep 17 00:00:00 2001 From: hippye99 Date: Mon, 1 Jun 2026 16:27:03 +0800 Subject: [PATCH] fix(trigger): avoid flushSync for synchronous-call dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 ``-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 https://github.com/ant-design/ant-design/issues/57789 --- src/index.tsx | 35 +++++-- tests/no-flush-sync-warning.test.tsx | 142 +++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 tests/no-flush-sync-warning.test.tsx diff --git a/src/index.tsx b/src/index.tsx index bb5fb838..77e67d47 100644 --- a/src/index.tsx +++ b/src/index.tsx @@ -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 }; @@ -382,14 +381,34 @@ 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 `mergedOpen` 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 ``- + // wrapped button that also opens a Modal). + // See https://github.com/ant-design/ant-design/issues/57789 + const lastDispatchedOpenRef = React.useRef(mergedOpen); + + // Keep the ref in sync with `mergedOpen` after each render so that + // controlled updates from outside (or any internal state change that + // already committed) reset the dedup baseline. This preserves the + // behaviour fixed in #601 where the dedup state could leak across user + // interactions in controlled mode without re-renders. + useLayoutEffect(() => { + lastDispatchedOpenRef.current = mergedOpen; + }, [mergedOpen]); + const internalTriggerOpen = useEvent((nextOpen: boolean) => { - flushSync(() => { - if (mergedOpen !== nextOpen) { - setInternalOpen(nextOpen); - onOpenChange?.(nextOpen); - onPopupVisibleChange?.(nextOpen); - } - }); + if (lastDispatchedOpenRef.current !== nextOpen) { + lastDispatchedOpenRef.current = nextOpen; + setInternalOpen(nextOpen); + onOpenChange?.(nextOpen); + onPopupVisibleChange?.(nextOpen); + } }); // Trigger for delay diff --git a/tests/no-flush-sync-warning.test.tsx b/tests/no-flush-sync-warning.test.tsx new file mode 100644 index 00000000..6afca838 --- /dev/null +++ b/tests/no-flush-sync-warning.test.tsx @@ -0,0 +1,142 @@ +/** + * Regression coverage for https://github.com/ant-design/ant-design/issues/57789 + * + * Trigger used to wrap `setInternalOpen` / `onOpenChange` in `flushSync` for + * synchronous-call dedup (introduced in #601). React 19 warns + * + * `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 phase + * — e.g. clicking a button wrapped by `` that also + * opens a Modal: the click handler updates Modal state (entering React's + * render phase), the focus event in the same batch routes into Trigger's + * `internalTriggerOpen`, and `flushSync` then fires inside the render. + * + * This test pins the fix: opening a Trigger while React is mid-commit must + * not emit the warning. + */ +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.NoFlushSyncWarning', () => { + 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 emit a flushSync warning when open is triggered from inside a render/commit', async () => { + // Spy console.error so we can fail the test on the React 19 flushSync warning. + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + // Sibling whose commit cycle "owns" the render: when its `count` state + // increases it re-renders, and within that commit we synchronously fire a + // focus event on the Trigger target. That mirrors the Modal+Tooltip case + // in #57789 where focus handling lands inside React's render phase. + const Reproducer: React.FC = () => { + const targetRef = React.useRef(null); + const [count, setCount] = React.useState(0); + + React.useEffect(() => { + if (count === 1 && targetRef.current) { + // Synchronously focus the trigger target while we are still inside + // an effect that ran during commit. + targetRef.current.focus(); + } + }, [count]); + + return ( + <> + + popup}> + + + + ); + }; + + const { container } = render(); + + act(() => { + fireEvent.click(container.querySelector('.opener') as HTMLButtonElement); + }); + + await flush(); + + const flushSyncWarnings = errorSpy.mock.calls.filter((call) => + String(call[0]).includes('flushSync was called from inside a lifecycle'), + ); + expect(flushSyncWarnings).toEqual([]); + + errorSpy.mockRestore(); + }); + + it('does not import flushSync from react-dom (structural guard)', () => { + // Soft guard: if anyone re-introduces flushSync in src/index.tsx the + // structural intent of this fix should be reviewed alongside #57789. + // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require + const fs = require('node:fs') as typeof import('node:fs'); + const path = require('node:path') as typeof import('node:path'); + const source = fs.readFileSync( + path.resolve(__dirname, '../src/index.tsx'), + 'utf8', + ); + // Strip block + line comments so the explanatory comment that *mentions* + // flushSync doesn't trip the guard. + const code = source + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/.*$/gm, '$1'); + expect(code).not.toMatch(/from\s+['"]react-dom['"]/); + expect(code).not.toMatch(/\bflushSync\s*\(/); + }); +});