test/use resize observer func target closure#226
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a subtle but important edge case in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 94.16% 94.28% +0.12%
==========================================
Files 6 6
Lines 137 140 +3
Branches 40 41 +1
==========================================
+ Hits 129 132 +3
Misses 7 7
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request modifies the useResizeObserver hook to correctly handle scenarios where the target element is provided by a function (getTarget) that might initially return null but later resolves to an actual DOM element. This is achieved by introducing a funcTargetIdRef that, when incremented, forces the useEffect to re-run and re-evaluate the getTarget function, ensuring the latest element is observed. A new test case has been added to validate this dynamic behavior, particularly when useEvent is used to stabilize the getTarget function's identity.
概览修改 变更
预估代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟 可能相关的 PR
诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/useResizeObserver.ts (1)
74-87:⚠️ Potential issue | 🟠 Major缺少对"非空元素切换"场景的处理。
现有的
funcTargetIdRef只在target为null时递增（第 74-75 行），这样当稳定的getTarget函数从返回元素 A 变为返回元素 B 时，依赖数组[enabled, funcTargetIdRef.current]不会变化，effect 不会重新运行，observer 会继续挂在元素 A 上。像条件渲染或动态替换元素这样的场景就会踩到这个问题。建议的解决方案是在每次 render 时都重新求值并比较节点是否变化，或者单独缓存上一次解析出的节点并按节点变化做
observe/unobserve。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/useResizeObserver.ts` around lines 74 - 87, The effect doesn't handle the case where a stable getTarget function begins returning a different non-null element, so the observer can stay attached to the old node; update the logic in the effect that uses funcTargetIdRef and getTarget so you resolve the current target each render (call getTarget or read target), keep a prevResolvedTargetRef, and when the resolved node changes call unobserve(prevResolvedTargetRef.current, onInternalResize) then observe(newTarget, onInternalResize) (or increment funcTargetIdRef when the resolved target changes) and update prevResolvedTargetRef; ensure onInternalResize and unobserve/observe calls are used as in the existing effect so the effect reacts to node swaps even when getTarget identity doesn't change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/useResizeObserver.spec.tsx`:
- Around line 11-16: The test uses a getter getTarget and currently asserts it
returns HTMLElement, but the runtime and useResizeObserver(handle in function
useResizeObserver) already accept null targets; update the public type
signatures so the target and getter types allow null (e.g., accept HTMLElement |
null and () => HTMLElement | null) and change the test to remove the unsafe as
HTMLElement cast by using the nullable getter type; ensure symbols referenced
are getTarget (test getter) and useResizeObserver (hook) so callers and tests
align with the runtime null-handling branches (check the conditional handling
around target && enabled and the special function-target null handling).
---
Outside diff comments:
In `@src/useResizeObserver.ts`:
- Around line 74-87: The effect doesn't handle the case where a stable getTarget
function begins returning a different non-null element, so the observer can stay
attached to the old node; update the logic in the effect that uses
funcTargetIdRef and getTarget so you resolve the current target each render
(call getTarget or read target), keep a prevResolvedTargetRef, and when the
resolved node changes call unobserve(prevResolvedTargetRef.current,
onInternalResize) then observe(newTarget, onInternalResize) (or increment
funcTargetIdRef when the resolved target changes) and update
prevResolvedTargetRef; ensure onInternalResize and unobserve/observe calls are
used as in the existing effect so the effect reacts to node swaps even when
getTarget identity doesn't change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f76fc4-ebdb-42d9-bede-c035a48239f4
📒 Files selected for processing (2)
src/useResizeObserver.tstests/useResizeObserver.spec.tsx
| // `useEvent` keeps the getter identity stable while the closed-over DOM node | ||
| // comes from state. If the hook only checks the function reference, it misses | ||
| // the later state update from `null` to the actual element and never observes it. | ||
| const getTarget = useEvent(() => element as HTMLElement); | ||
|
|
||
| useResizeObserver(true, getTarget); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "\.(tsx?|jsx?)$" | head -20Repository: react-component/resize-observer
Length of output: 479
🏁 Script executed:
fd -e tsx -e ts -e jsx -e js | grep -i "use.*resize" | head -20Repository: react-component/resize-observer
Length of output: 132
🏁 Script executed:
fd "useResizeObserver" --type fRepository: react-component/resize-observer
Length of output: 132
🏁 Script executed:
find . -name "*.tsx" -o -name "*.ts" | xargs grep -l "export.*useResizeObserver" | head -10Repository: react-component/resize-observer
Length of output: 117
🏁 Script executed:
cat -n tests/useResizeObserver.spec.tsx | head -40Repository: react-component/resize-observer
Length of output: 1558
🏁 Script executed:
cat -n src/useResizeObserver.tsRepository: react-component/resize-observer
Length of output: 3508
🏁 Script executed:
cat -n src/index.tsxRepository: react-component/resize-observer
Length of output: 2831
类型签名与运行时实现不匹配，导致需要不安全的类型断言。
测试中 getter 函数初始返回 null（当 element 状态为 null），后来返回真实元素。但当前签名只允许 () => HTMLElement，强制测试使用 as HTMLElement 绕过类型检查。
实现已在第 72 行明确处理了 null 目标（if (target && enabled)），包括第 74-75 行对函数目标 null 情况的特殊处理。既然运行时支持这个场景，公开类型应同步允许 HTMLElement | null 和 () => HTMLElement | null。
建议的修改
export default function useResizeObserver(
enabled: boolean,
- getTarget: HTMLElement | (() => HTMLElement),
+ getTarget: HTMLElement | null | (() => HTMLElement | null),
onDelayResize?: OnResize,
onSyncResize?: OnResize,
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/useResizeObserver.spec.tsx` around lines 11 - 16, The test uses a
getter getTarget and currently asserts it returns HTMLElement, but the runtime
and useResizeObserver(handle in function useResizeObserver) already accept null
targets; update the public type signatures so the target and getter types allow
null (e.g., accept HTMLElement | null and () => HTMLElement | null) and change
the test to remove the unsafe as HTMLElement cast by using the nullable getter
type; ensure symbols referenced are getTarget (test getter) and
useResizeObserver (hook) so callers and tests align with the runtime
null-handling branches (check the conditional handling around target && enabled
and the special function-target null handling).
1 participant
fix ant-design/ant-design#57292
这是一个复合问题，原因在于 rc-motion 的时序变化会导致 ResizeObserver 在 ref 监听的时候拿到的是一个 null 对象，导致监听尺寸变化的时候元素没有 ready。让 AI 总结了一下：
Tooltip unique smooth-transition 问题排查总结
现象
components/tooltip/demo/smooth-transition.tsx中，鼠标首次移入 Tooltip 时，背景框尺寸不正确。表现特征：
涉及链路
本次问题涉及的主要文件：
components/tooltip/UniqueProvider/MotionContent.tsxnode_modules/@rc-component/trigger/es/Popup/index.jsnode_modules/@rc-component/trigger/es/UniqueProvider/index.jsnode_modules/@rc-component/resize-observer/es/useResizeObserver.jsnode_modules/@rc-component/motion/*链路大致如下：
UniqueProviderUniqueProvider依赖Popup的onResize获取 popup 尺寸Popup内部通过ResizeObserver监听外层 popup DOMMotionContent包了一层CSSMotion关键发现
1.
onPrepare能执行，但拿到的 rect 是空的在
rc-trigger的UniqueProvider里打印onPrepare时，可以看到：onPrepare的确会执行getBoundingClientRect()的结果类似：DOMRect { x: -11950, y: -9580, width: 0, height: 0, ... }这里的
x/y基本对应-1000vw / -1000vh，说明量到的是 rc-trigger prepare 阶段临时停在屏幕外的 popup 壳子。结论：
2.
MotionContent在首次 appear 时没有及时产出可布局内容MotionContent是 antd 自己封装的一层内容动画壳：components/tooltip/UniqueProvider/MotionContent.tsx排查结论是：
appear的那一拍，里面的CSSMotion没有及时产出可布局的内容0 x 0或空尺寸3. 真正导致背景框错误的直接原因是
onResize没有补回来继续往下看发现，更直接的问题是：
Popup/index.js里的ResizeObserver本来应该在内容真正渲染出来后触发onResizeonResize没有再触发UniqueProvider拿不到正确的popupSize为什么
onResize没有触发排查
@rc-component/resize-observer后，确认了一个关键点：useResizeObserver里是通过getTarget()拿目标 DOM 的getTarget是函数时，effect 不会随着ref.current变化自动重新执行null，就不会调用observe(target, ...)也就是说，问题不是单纯“尺寸变了但 observer 没回调”，而是：
observer 很可能在第一次就错过了订阅时机。
临时实验
为了验证这个判断，临时修改了：
node_modules/@rc-component/resize-observer/es/useResizeObserver.jsnode_modules/@rc-component/resize-observer/lib/useResizeObserver.js实验内容：
getTarget()返回null时getTarget()observe实验结果：
这说明：
onResize就能把正确尺寸补回来最终结论
这次问题其实是两段因果链叠在一起：
根因
@rc-component/motion在首次appear阶段的渲染语义变了，导致 popup 相关 DOM 没有在第一时间以“可观测/可布局”的方式挂出。直接触发错误展示的原因
@rc-component/resize-observer对“第一次getTarget()为 null，后续 DOM 才出现”的场景不够健壮，导致 observer 可能错过订阅，onResize没有补回来。修复建议
最佳正式修复
优先修
@rc-component/motion。原因：
null，上层 target 就能按预期存在ResizeObserver也就不会错过首次订阅时机推荐修法：
styleReady === 'NONE'的触发范围null，而是至少保留可挂载节点可选的健壮性增强
@rc-component/resize-observer可以考虑补一个兜底：getTarget()为null时这不是本次问题的最优主修，但它能提升对“late mount target”场景的健壮性。
一句话总结
这次 Tooltip 背景框尺寸错误，不是单纯的尺寸算法问题，而是：
首次 appear 时 DOM 挂载时机变化，导致 ResizeObserver 错过订阅，最终 popup 尺寸没有被正确补回。