diff --git a/src/index.tsx b/src/index.tsx index 22f29a0a..aec8ccf0 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 }; @@ -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(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 diff --git a/tests/concurrent-render.test.tsx b/tests/concurrent-render.test.tsx new file mode 100644 index 00000000..ba5f3471 --- /dev/null +++ b/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 ; + }; + + 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 ; + } + return this.props.children; + } + } + + const onCatch = jest.fn(); + + const Harness: React.FC<{ open: boolean; attempt: boolean }> = ({ + open, + attempt, + }) => ( + + popup} + popupVisible={open} + onOpenChange={onOpenChange} + > + + + + ); + + // Initial committed render: closed, no throw. + const { container, rerender } = render(); + 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(); + }); + 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(); + }); + 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); + } + }); +}); diff --git a/tests/layout-effect-ordering.test.tsx b/tests/layout-effect-ordering.test.tsx new file mode 100644 index 00000000..4fa53354 --- /dev/null +++ b/tests/layout-effect-ordering.test.tsx @@ -0,0 +1,142 @@ +/** + * Regression coverage for the layout-effect ordering gap flagged in the + * #622 review by @nrps9909. + * + * The dedup baseline (`lastDispatchedOpenRef`) used to be synchronized inside + * Trigger's own `useLayoutEffect([rawOpen])`. React runs descendant layout + * effects *before* their parent's, so during a render that flipped + * `popupVisible` a descendant `useLayoutEffect` could reach + * `internalTriggerOpen` while the ref still held the previous, stale value — + * a legitimate opposite dispatch would then be discarded as a duplicate and + * `onOpenChange` would never fire. + * + * The fix synchronizes the ref during render, so descendant layout effects + * see the up-to-date baseline. + * + * Concrete scenario from the review: + * + * 1. Render a controlled `` + * and focus the target. + * 2. Rerender with `popupVisible={true}`. + * 3. In the target component's `useLayoutEffect([open])`, call `target.blur()`. + * 4. Assert focus actually left the target *and* `onOpenChange(false)` fired + * exactly once. + * + * Before the fix: focus leaves but the callback count is 0. + * After the fix: the callback fires once. + */ +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.LayoutEffectOrdering (#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('accepts an opposite dispatch from a descendant layout effect after the parent commits a controlled open change', async () => { + const onOpenChange = jest.fn(); + + // Target that runs a layout effect on every `open` transition. When + // `open` becomes true it blurs itself synchronously — this executes + // *before* Trigger's own layout effects on the same commit, which is + // exactly the ordering window the original PR head mishandled. + // We fire a real blur event on the DOM node (not just `HTMLElement.blur()`) + // to ensure Trigger's `onBlur` handler runs under jsdom. + const Target = React.forwardRef< + HTMLSpanElement, + { open: boolean } & React.HTMLAttributes + >(({ open, ...rest }, forwardedRef) => { + const localRef = React.useRef(null); + React.useImperativeHandle(forwardedRef, () => localRef.current!); + React.useLayoutEffect(() => { + if (open && localRef.current) { + fireEvent.blur(localRef.current); + } + }, [open]); + // Forward any Trigger-injected handlers (onFocus/onBlur/etc.) onto + // the underlying span; without this, Trigger's `onBlur` never fires + // and the ordering gap can't be exercised. + return ; + }); + + const Harness: React.FC<{ open: boolean }> = ({ open }) => ( + popup} + popupVisible={open} + onOpenChange={onOpenChange} + > + + + ); + + const { container, rerender } = render(); + const target = container.querySelector('.target') as HTMLSpanElement; + + act(() => { + fireEvent.focus(target); + }); + await flush(); + + onOpenChange.mockClear(); + + // Parent commits false -> true. The descendant layout effect fires blur + // *during that commit*, before Trigger's own effects could have synced + // the dedup ref. With the render-body sync, Trigger sees the up-to-date + // baseline (`rawOpen === true`) and treats the blur-driven dispatch as + // a real transition to false. + act(() => { + rerender(); + }); + await flush(); + + expect(onOpenChange).toHaveBeenCalledTimes(1); + expect(onOpenChange).toHaveBeenLastCalledWith(false); + }); +}); 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*\(/); + }); +});