Uh oh!
There was an error while loading. Please reload this page.
fix(trigger): avoid flushSync for synchronous-call dedup - #622
fix(trigger): avoid flushSync for synchronous-call dedup#622yezhonghu0503 wants to merge 1 commit into
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough该 PR 移除 Changes移除 flushSync 并重构去重逻辑
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Trigger
participant React
participant OpenCallbacks
Trigger->>Trigger: 比较并记录 nextOpen
Trigger->>React: 更新内部 open 状态
React-->>Trigger: 提交 rawOpen 变化
Trigger->>OpenCallbacks: 调用 onOpenChange 和 onPopupVisibleChange
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request replaces the use of flushSync in src/index.tsx with a ref-based tracking mechanism (lastDispatchedOpenRef) to avoid React 19 warnings when triggering state updates during a render or commit phase. It also adds regression tests to ensure no warnings are emitted and that flushSync is not imported. The reviewer identified a critical issue in controlled mode: if a parent component rejects or ignores the onOpenChange callback, the tracking ref gets stuck in an inconsistent state, preventing subsequent interactions. A code suggestion was provided to reset the ref to the last committed state using a microtask.
| // 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); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If Trigger is used in controlled mode (where popupVisible is controlled by the parent), and the parent component decides to ignore or reject the onOpenChange(true) call (for example, due to custom validation or conditional logic), mergedOpen will remain false.
Because mergedOpen remains false, the useLayoutEffect (which has [mergedOpen] as a dependency) will not run, and the ref lastDispatchedOpenRef.current will remain stuck at true. Consequently, any subsequent user interactions attempting to open the trigger (calling internalTriggerOpen(true)) will be silently ignored because lastDispatchedOpenRef.current !== nextOpen evaluates to false (true !== true). This completely breaks the ability to retry opening the trigger in controlled mode.
To fix this, we can schedule a microtask to reset lastDispatchedOpenRef.current back to the last committed state (openRef.current) after the current event batch/tick completes. This ensures that if the state update is rejected or ignored, subsequent interactions can still trigger the callbacks, while still successfully deduplicating synchronous events within the same batch.
// 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) => {
if (lastDispatchedOpenRef.current !== nextOpen) {
lastDispatchedOpenRef.current = nextOpen;
setInternalOpen(nextOpen);
onOpenChange?.(nextOpen);
onPopupVisibleChange?.(nextOpen);
// Reset the ref to the last committed state after the current event batch/tick.
// This ensures that if the state update is rejected or ignored in controlled mode,
// subsequent interactions can still trigger the callbacks.
Promise.resolve().then(() => {
lastDispatchedOpenRef.current = openRef.current;
});
}
});
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/no-flush-sync-warning.test.tsx (1)
124-142: 💤 Low value结构性守卫检查范围较宽
Line 139 的正则
/from\s+['"]react-dom['"]/会阻止任何react-dom导入，不仅限于flushSync。如果将来有人需要添加其他合法的react-dom导入（如createPortal），此测试会误报失败。考虑到当前
src/index.tsx通过@rc-component/portal封装来避免直接依赖react-dom，且注释已说明这是"soft guard"用于触发审查，现有方案可以接受。如需更精确的检查，可改为：expect(code).not.toMatch(/\bflushSync\b/);这样只检查
flushSync标识符，不会影响其他可能的react-dom导入。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/no-flush-sync-warning.test.tsx` around lines 124 - 142, The structural guard test named "does not import flushSync from react-dom (structural guard)" is too broad because the current assertion that checks for any "react-dom" import will false-positive if other react-dom APIs are added; update the test by removing or replacing the assertion that inspects imports (the expectation against the regex matching a react-dom import) and instead assert only that the source (the variable named code) does not contain the identifier "flushSync" (i.e., keep the expectation that checks for absence of flushSync and drop the generic react-dom import check) so the test only flags use of flushSync without blocking other valid react-dom imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/no-flush-sync-warning.test.tsx`:
- Around line 124-142: The structural guard test named "does not import
flushSync from react-dom (structural guard)" is too broad because the current
assertion that checks for any "react-dom" import will false-positive if other
react-dom APIs are added; update the test by removing or replacing the assertion
that inspects imports (the expectation against the regex matching a react-dom
import) and instead assert only that the source (the variable named code) does
not contain the identifier "flushSync" (i.e., keep the expectation that checks
for absence of flushSync and drop the generic react-dom import check) so the
test only flags use of flushSync without blocking other valid react-dom imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1e16d25-44dd-4b13-b7e4-22d0abcc198f
📒 Files selected for processing (2)
src/index.tsxtests/no-flush-sync-warning.test.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## master #622 +/- ##
=======================================
Coverage 97.28% 97.28% =======================================
Files 17 17 Lines 956 959 +3 Branches 268 278 +10 =======================================
+ Hits 930 933 +3
Misses 26 26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`internalTriggerOpen` wrapped `setInternalOpen` / `onOpenChange` / `onPopupVisibleChange` in `flushSync` (introduced in react-component#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 react-component#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 blur dedup behaviour is preserved. Refs ant-design/ant-design#57789
6a06a13 to
29ec54eCompare@hippye99 is attempting to deploy a commit to the afc163's projects Team on Vercel. A member of the Team first needs to authorize it. |
Summary
internalTriggerOpenwrapssetInternalOpen/onOpenChange/onPopupVisibleChangeinflushSync(introduced in #601 to dedup multi-event interactions likepointerenter+focus). Under React 19 that emitswhenever
internalTriggerOpenis reached from inside a render/commit phase. The reproduction in the linked antd issue is clicking a<Tooltip trigger="focus">-wrapped button that also opens a Modal:internalTriggerOpen.flushSyncthen fires inside the render → warning.The dedup is necessary (without it, both events would dispatch
onOpenChange(true)because state updates are async, so the second call would still see the stalemergedOpen), but it does not need to useflushSync.What this PR does
Replaces the
flushSyncgate with a singleuseRef(lastDispatchedOpenRef) that records the last valueinternalTriggerOpensynchronously dispatched. Subsequent calls in the same batch compare against the ref instead of state, so dedup still works without forcing a sync render.A
useLayoutEffectkeeps the ref in sync withmergedOpenafter each commit, so:popupVisibleprop) reset the dedup baseline.lastTriggerRef-leak case that fix(trigger): avoid render-based reset for interaction-level deduplication #601 originally fixed — dedup state leaking across user interactions when the parent doesn't re-render — still cannot occur, because the ref is reconciled with committed state every commit.Tests
tests/open-change.test.tsx(added in fix(trigger): avoid render-based reset for interaction-level deduplication #601): both dedup cases (pointerenter+focus,pointerleave+blur) keep passing —onOpenChangeis still called exactly once per interaction batch.tests/no-flush-sync-warning.test.tsx:focuson a Trigger target from inside a React effect, then asserts noflushSync was called from inside a lifecyclewarning landed onconsole.error. Verified to fail onmasterand pass on this branch.src/index.tsxno longer imports or callsflushSync(comments mentioning it are stripped before the regex check so the explanatory comment can stay).Full suite:
npm test→ 18 suites / 132 tests passing (1 pre-existing skip).Refs
flushSynccall this PR removes: fix(trigger): avoid render-based reset for interaction-level deduplication #601AI disclosure
Claude assisted with the regression hunt (locating #601 as the introduction point) and helped draft the test scaffolding. The fix design (ref +
useLayoutEffectsync) and the wording above are reviewed; the test was independently verified to fail on the pre-fix code and pass after, locally.Summary by CodeRabbit
发布说明
Bug 修复
flushSync警告的问题。测试