-
-
Notifications
You must be signed in to change notification settings - Fork 244
fix(trigger): avoid flushSync for synchronous-call dedup #622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
yezhonghu0503
wants to merge
3
commits into
react-component:master
Choose a base branch
from
yezhonghu0503:fix/avoid-flushsync-in-internal-trigger-open
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+536
−8
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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; | ||
| } | ||
| } | ||
|
|
||
| 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); | ||
| } | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: react-component/trigger
Length of output: 10687
🏁 Script executed:
Repository: react-component/trigger
Length of output: 26540
🏁 Script executed:
Repository: react-component/trigger
Length of output: 49269
🏁 Script executed:
Repository: react-component/trigger
Length of output: 250
🏁 Script executed:
Repository: react-component/trigger
Length of output: 250
🏁 Script executed:
Repository: react-component/trigger
Length of output: 583
修正回归测试,使其保留已提交的
Trigger。Boundary捕获错误后会持续渲染.target-fallback。后续rerender不会重新挂载Trigger。第 151、178、192 行的事件不会触发Trigger注入的处理器。末尾遍历断言也允许onOpenChange零次调用。使用
startTransition与Suspense构造未提交的中断更新,保留原目标元素,并在聚焦原目标后断言onOpenChange(true)恰好调用一次。🤖 Prompt for AI Agents