perf: skipping re-rendering #190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
冲突了 |
muzea
force-pushed
the
hotfix/avoid-unnecessary-rerender
branch
from
c734dd8 to
83d9a75
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #190 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 19 19
Lines 796 797 +1
Branches 193 193
=======================================
+ Hits 779 780 +1
Misses 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@afc163 大佬，为啥这个还没有合并呢？都几年了，这个问题很严重啊 |
|
@muzea @XianZhengquan 冲突了 |
@muzea 大佬，瞅一瞅呀 |
muzea
force-pushed
the
hotfix/avoid-unnecessary-rerender
branch
from
b505549 to
6b1ebb8
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough在若干组件与钩子中使用 useMemo/useCallback 稳定函数与对象引用，移除 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant Filler as Filler
participant RO as ResizeObserver
participant Parent as onInnerResize
User->>Filler: 渲染组件
Filler->>RO: observe(inner)
RO-->>Filler: resize(entry)
alt entry.offsetHeight > 0
Filler->>Filler: handleResize (useCallback)
Filler->>Parent: onInnerResize(offsetHeight)
else offsetHeight == 0
note right of Filler: 不调用 onInnerResize
end
sequenceDiagram
autonumber
participant List as List
participant useChildren as useChildren (useMemo)
participant renderFn as renderFunc
participant Item as Item
List->>useChildren: 提供 list,startIndex,endIndex,getKey,offsetX,scrollWidth
useChildren->>useChildren: useMemo 缓存 children[]（依赖未变则复用）
alt 依赖未变
useChildren-->>List: 返回缓存 children[]
else 依赖变化
loop i in [startIndex, endIndex)
useChildren->>renderFn: 调用 renderFunc(i, width, offsetX)
renderFn-->>useChildren: React 元素
useChildren->>Item: 包装并设 key = getKey(...)
end
useChildren-->>List: 返回新 children[]
end
sequenceDiagram
autonumber
participant Hook as useHeights
participant RAF as requestAnimationFrame
participant Instances as instancesRef
Hook->>Hook: cancelRaf (useCallback) -> 增加 promiseIdRef，使旧任务失效
Hook->>Hook: collectHeight (useCallback) -> 计算累积 heights，视情况更新 updatedMark
Hook->>Hook: setInstanceRef (useCallback) -> 更新 instancesRef，触发 collectHeight 与 onItemAdd/remove
alt 高度变更
Hook-->>Hook: bump updatedMark
else 无变更
Hook-->>Hook: 无改动
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (5)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
最近事情比较多，我看看这几天 rebase 一下代码 |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes performance by preventing unnecessary re-renders through the addition of React memoization techniques. The changes wrap functions and objects in useCallback and useMemo hooks to maintain reference stability across renders.
- Wraps functions in
useCallbackto prevent recreation on every render - Adds
useMemoto objects and computed values to maintain reference equality - Includes a Chinese comment explaining potential behavioral differences
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/hooks/useHeights.tsx | Converts functions to useCallback and adds explanatory comment |
| src/hooks/useChildren.tsx | Wraps return value in useMemo with dependency array |
| src/List.tsx | Memoizes sharedConfig object creation |
| src/Filler.tsx | Reorders imports and wraps resize handler in useCallback |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/hooks/useHeights.tsx
Outdated
| return cancelRaf; | ||
| }, []); | ||
|
|
||
| // 这里稍显迷惑性，当 heightsRef.current.set 被调用时，updatedMark 会变化，进而导致 heightsRef.current 也出现变化 |
There was a problem hiding this comment.
The comment is in Chinese. Consider translating to English for consistency with the rest of the codebase: // This is somewhat confusing: when heightsRef.current.set is called, updatedMark changes, which causes heightsRef.current to also change
| // 这里稍显迷惑性，当 heightsRef.current.set 被调用时，updatedMark 会变化，进而导致 heightsRef.current 也出现变化 | |
| // This is somewhat confusing: when heightsRef.current.set is called, updatedMark changes, which causes heightsRef.current to also change |
Copilot uses AI. Check for mistakes.
src/hooks/useChildren.tsx
Outdated
| }, | ||
| offsetX, | ||
| }) as React.ReactElement; | ||
| // 可能存在 list 不变但是里面的数据存在变化的情况，会与之前写法存在不同的行为 |
There was a problem hiding this comment.
The comment is in Chinese. Consider translating to English for consistency: // There may be cases where the list doesn't change but the data inside it does, which will behave differently from the previous implementation
| // 可能存在 list 不变但是里面的数据存在变化的情况，会与之前写法存在不同的行为 | |
| // There may be cases where the list reference does not change, but the data inside it does, which will behave differently from the previous implementation |
Copilot uses AI. Check for mistakes.
|
|
||
| useEffect(() => { | ||
| return cancelRaf; | ||
| }, []); |
There was a problem hiding this comment.
The useEffect cleanup function should include cancelRaf in the dependency array since it's now a memoized callback. Change the dependency array from [] to [cancelRaf].
| }, []); | |
| }, [cancelRaf]); |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/Filler.tsx (1)
72-79: 为 handleResize 标注类型，避免隐式 any；保持回调稳定性当前解构参数未标注类型，在开启 noImplicitAny 时会报错。建议使用 rc-resize-observer 的类型来声明，既避免隐式 any，又与 onResize 的签名一致。
应用如下修改：
-import ResizeObserver from 'rc-resize-observer'; +import ResizeObserver from 'rc-resize-observer'; +import type { ResizeObserverProps } from 'rc-resize-observer';- const handleResize = React.useCallback( - ({ offsetHeight }) => { + const handleResize: ResizeObserverProps['onResize'] = React.useCallback( + ({ offsetHeight }) => { if (offsetHeight && onInnerResize) { onInnerResize(); } }, [onInnerResize], );Also applies to: 84-85
src/hooks/useChildren.tsx (2)
15-16: 确认“list 引用不变但内部数据变化”的行为差异是否可接受这里明确引入了记忆化，若外部在原地 mutate 数组（保持引用不变），则不会重新计算子元素，可能导致 UI 不更新。请确认库的契约是否要求数据不可变更新（推荐做法）。若需兼容原地变更，建议增加一个外部可控的“数据版本号”或“无效化”信号参与依赖。
如需，我可以起草一个 dataVersion（或 invalidate）的 prop 方案，并补充到 List 与 useChildren 的依赖中。
16-33: 记忆化计算本身 LGTM，但可补充 list.length 捕获原地增删场景当前依赖包含 list 本身，但无法捕获“同引用但长度变化”的极端情况。增加 list.length 作为低成本的折中方案，可覆盖 push/pop 等原地操作（仍无法捕获仅内容变更但长度不变的情况）。
- }, [list, startIndex, endIndex, setNodeRef, renderFunc, getKey, offsetX, scrollWidth]); + }, [list, list.length, startIndex, endIndex, setNodeRef, renderFunc, getKey, offsetX, scrollWidth]);src/hooks/useHeights.tsx (3)
31-71: 微调：避免重复自增 promiseIdRefcollectHeight 一开始已调用 cancelRaf() 自增了 promiseIdRef；异步分支中再次自增会造成语义上的冗余（功能上无害）。建议删除第二次自增，提升可读性。
- promiseIdRef.current += 1; const id = promiseIdRef.current; Promise.resolve().then(() => { if (id === promiseIdRef.current) { doCollect(); } });
72-92: setInstanceRef 记忆化良好；布尔差异判断可读性可提升当前用法
!origin !== !instance属于“真假异或”，可读性略差。建议改为显式布尔转换对比。- // Instance changed - if (!origin !== !instance) { + // Instance changed + if (Boolean(origin) !== Boolean(instance)) { if (instance) { onItemAdd?.(item); } else { onItemRemove?.(item); } }
93-96: useEffect 依赖建议包含 cancelRaf 以满足 exhaustive-deps虽然 cancelRaf 已稳定，但加入依赖可避免 ESLint 提示。
- useEffect(() => { - return cancelRaf; - }, []); + useEffect(() => { + return cancelRaf; + }, [cancelRaf]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Filler.tsx(2 hunks)src/List.tsx(1 hunks)src/hooks/useChildren.tsx(1 hunks)src/hooks/useHeights.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/List.tsx (1)
src/interface.ts (1)
SharedConfig(7-9)
src/hooks/useChildren.tsx (1)
src/Item.tsx (1)
Item(8-16)
🔇 Additional comments (4)
src/Filler.tsx (1)
2-3: 引入 rc-resize-observer 没问题依赖与用法与全局保持一致。
src/hooks/useHeights.tsx (2)
27-30: 将 cancelRaf 记忆化以稳定引用：LGTM命名 + 空依赖的 useCallback 简洁明了，有助于下游依赖稳定。
97-99: 返回 heightsRef.current 的模式保持不变：LGTM通过 updatedMark 驱动外部重新计算，高度缓存实例保持稳定引用，符合现有用法。
src/List.tsx (1)
168-173: sharedConfig 使用 useMemo 稳定引用：LGTM以 getKey 为依赖可避免每次 render 产生新对象，降低子树无谓重渲染，符合本 PR 的目标。
src/List.tsx
Outdated
| return { | ||
| getKey, | ||
| }; | ||
| }, [getKey]); |
There was a problem hiding this comment.
这个 sharedConfig 里只有一个 getKey，不如把参数直接拍平，不要这层 sharedConfig 了。
|
@zombieJ 也看看 |
感谢大佬 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/scrollWidth.test.tsx (1)
233-235: 如后续 scrollTo 存在异步链路，可考虑切换为 await act当前实现是同步断言，没问题；若未来 scrollTo 内部通过计时器/raf 异步更新，建议切换为 await act(async () => …) 并在内部推进计时器，减少潜在偶现。
Also applies to: 238-240
tests/scroll.test.js (1)
98-103: 建议调整 act 中的语句顺序：先触发 scrollTo 再推进计时器当前先 runAllTimers 再 scrollTo，若 scrollTo 内部存在异步（如 raf/setTimeout），会遗漏本次 scrollTo 产生的任务，存在偶发不稳定风险。建议交换顺序：
@@ - act(() => { - jest.runAllTimers(); - - listRef.current.scrollTo(null); - }); + act(() => { + listRef.current.scrollTo(null); + jest.runAllTimers(); + }); @@ - act(() => { - jest.runAllTimers(); - - listRef.current.scrollTo(null); - }); + act(() => { + listRef.current.scrollTo(null); + jest.runAllTimers(); + });Also applies to: 400-405
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/scroll-Firefox.test.js(2 hunks)tests/scroll.test.js(5 hunks)tests/scrollWidth.test.tsx(1 hunks)tests/touch.test.js(3 hunks)
🔇 Additional comments (10)
tests/scrollWidth.test.tsx (1)
233-235: 将 ref.scrollTo 调用包裹到 act 中，行为更稳健把同步的 scrollTo 调用放入 act，符合 React 对受控更新的测试约定，避免 “not wrapped in act(...)” 的告警。断言读取 getScrollInfo 也是同步的，改动合理。
Also applies to: 238-240
tests/scroll-Firefox.test.js (2)
1-1: 统一使用 @testing-library/react 的 act，方向正确改为从 RTL 引入 act 与本仓库其余测试保持一致，避免跨包 act 实例不一致的问题。
127-130: 滚动到底部与计时器推进放进 act，避免状态未刷新断言将 scrollTo 与 jest.runAllTimers 放到同一个 act 中，有助于确保副作用在断言前已生效。改动到位。
tests/scroll.test.js (1)
114-117: 将 scrollTo 与计时器统一包裹到 act 中，符合测试最佳实践这些位置的顺序与时机处理合理：先触发，再推进计时器，保证状态在断言前被刷新。改动 +1。
Also applies to: 135-137, 161-164
tests/touch.test.js (6)
74-91: 触摸序列整体包裹进单个 act，降低异步时序带来的不确定性start/move/end 与计时器推进放在同一 act 中，能确保副作用在断言前完成。实现合理。
106-117: “不可滚动时调用 preventDefault” 场景的事件派发包裹在 act 中，合理在 act 中构造与派发 touch 事件并注入 preventDefault mock，能稳定覆盖逻辑分支。
121-137: 重新起一轮触摸交互并在 act 内重置 mock，保证隔离性在同一 act 中推进计时器、reset mock 并再次派发事件，保证前后两段交互不串扰。用法到位。
146-150: 容器 touchstart 包裹 act，确保同步副作用按期落地轻量但必要的包裹，避免 React 对未包裹更新的告警。
155-170: 嵌套用例迁移到 RTL 并显式指定 itemKey，稳定性更好使用 RTL 的 render + container 模式，并为外层 List 指定 itemKey，可避免 key 生成差异导致的不必要重渲染。合理的微调。
182-185: 在异步 act 中推进计时器，确保嵌套滚动副作用完成advanceTimersByTime 放在 await act(async () => …) 中，能保证最终断言观察到稳定状态。改动得当。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/props.test.js (1)
26-28: prefixCls 用例风格统一 OK；建议补充清理当前 mount 后未显式卸载，长期会增加测试间耦合风险。可在断言后补一行
wrapper.unmount()。可以直接在该用例末尾追加：
expect(wrapper.find('.prefix-holder-inner').length).toBeTruthy(); + wrapper.unmount();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/props.test.js(3 hunks)
🔇 Additional comments (3)
tests/props.test.js (3)
2-2: 导入顺序微调 OK将 React 提前导入无副作用，风格一致性更好。
14-16: itemKey 与断言写法统一化 OK
- 单参箭头函数加括号、children 解构写法清晰。
- key 断言覆盖两项数据，语义准确。
Also applies to: 19-21
37-42: offsetX 渲染函数用例 OK
- 解构第 3 个参数拿到
offsetX并断言为 0 的路径清晰。- 单参箭头函数风格已统一。
| it('no unnecessary re-render', () => { | ||
| const renderItem = sinon.fake(({ id, key }) => <div key={key}>{id}</div>); | ||
| const data = [{ id: 1, key: 1 }]; | ||
| function Wrapper() { | ||
| const [state, setState] = React.useState(0); | ||
|
|
||
| React.useEffect(() => { | ||
| setState(1); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <div> | ||
| <h1>{state}</h1> | ||
| <List data={data} itemKey="key" prefixCls="prefix"> | ||
| {renderItem} | ||
| </List> | ||
| </div> | ||
| ); | ||
| } | ||
| const wrapper = mount(<Wrapper />); | ||
| expect(wrapper.find('h1').text()).toBe('1'); | ||
| expect(renderItem.callCount).toBe(1); | ||
| }); |
There was a problem hiding this comment.
修复：测试依赖未导入且存在异步更新竞争，导致用例不稳定
- 未导入
sinon：本文件直接使用sinon.fake，在大多数 CI（Jest + Enzyme）环境下会ReferenceError。本仓库更推荐使用jest.fn，避免额外依赖。 - 未用
act()/wrapper.update()刷新useEffect触发的异步setState。直接在mount后断言h1为1具有竞态，测试可能偶现失败。
建议改为 Jest 原生 mock，并用 act 明确冲刷副作用；同时在用例结束时 unmount：
@@
-it('no unnecessary re-render', () => {
- const renderItem = sinon.fake(({ id, key }) => <div key={key}>{id}</div>);
+it('no unnecessary re-render', async () => {
+ const renderItem = jest.fn(({ id, key }) => <div key={key}>{id}</div>);
const data = [{ id: 1, key: 1 }];
function Wrapper() {
const [state, setState] = React.useState(0);
React.useEffect(() => {
setState(1);
}, []);
@@
- const wrapper = mount(<Wrapper />);
- expect(wrapper.find('h1').text()).toBe('1');
- expect(renderItem.callCount).toBe(1);
+ const wrapper = mount(<Wrapper />);
+ // 冲刷 useEffect 导致的异步 setState
+ await act(async () => {
+ await new Promise(resolve => setTimeout(resolve, 0));
+ });
+ wrapper.update();
+ expect(wrapper.find('h1').text()).toBe('1');
+ expect(renderItem).toHaveBeenCalledTimes(1);
+ wrapper.unmount();
});并在文件顶部补充 act 导入（不与现有导入顺序冲突即可）：
import { mount } from 'enzyme';
import React from 'react';
+import { act } from 'react-dom/test-utils';
import List from '../src';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('no unnecessary re-render', () => { | |
| const renderItem = sinon.fake(({ id, key }) => <div key={key}>{id}</div>); | |
| const data = [{ id: 1, key: 1 }]; | |
| function Wrapper() { | |
| const [state, setState] = React.useState(0); | |
| React.useEffect(() => { | |
| setState(1); | |
| }, []); | |
| return ( | |
| <div> | |
| <h1>{state}</h1> | |
| <List data={data} itemKey="key" prefixCls="prefix"> | |
| {renderItem} | |
| </List> | |
| </div> | |
| ); | |
| } | |
| const wrapper = mount(<Wrapper />); | |
| expect(wrapper.find('h1').text()).toBe('1'); | |
| expect(renderItem.callCount).toBe(1); | |
| }); | |
| // at the top of tests/props.test.js | |
| import { mount } from 'enzyme'; | |
| import React from 'react'; | |
| import { act } from 'react-dom/test-utils'; | |
| import List from '../src'; | |
| it('no unnecessary re-render', async () => { | |
| const renderItem = jest.fn(({ id, key }) => <div key={key}>{id}</div>); | |
| const data = [{ id: 1, key: 1 }]; | |
| function Wrapper() { | |
| const [state, setState] = React.useState(0); | |
| React.useEffect(() => { | |
| setState(1); | |
| }, []); | |
| return ( | |
| <div> | |
| <h1>{state}</h1> | |
| <List data={data} itemKey="key" prefixCls="prefix"> | |
| {renderItem} | |
| </List> | |
| </div> | |
| ); | |
| } | |
| const wrapper = mount(<Wrapper />); | |
| // Flush the async setState from useEffect | |
| await act(async () => { | |
| await new Promise(resolve => setTimeout(resolve, 0)); | |
| }); | |
| wrapper.update(); | |
| expect(wrapper.find('h1').text()).toBe('1'); | |
| expect(renderItem).toHaveBeenCalledTimes(1); | |
| wrapper.unmount(); | |
| }); |
🤖 Prompt for AI Agents
In tests/props.test.js around lines 48 to 70, the test uses sinon.fake (which is
not imported) and asserts state updated by useEffect without flushing effects,
causing ReferenceError and flaky timing; replace sinon.fake with jest.fn (remove
sinon dependency), add an import for act from react-dom/test-utils at the top,
wrap the mount and the async state flush in act (or call act(() => { wrapper =
mount(<Wrapper />); }) then call wrapper.update()), assert after update, and
finally call wrapper.unmount() to clean up; this ensures no missing imports and
eliminates the async race.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/touch.test.js (1)
94-95: 建议在 afterEach 中统一清理虽然手动调用
unmount()不是错误，但建议将清理逻辑统一放在afterEach钩子中，以保持测试的一致性和避免遗漏。+ let wrapper; + + afterEach(() => { + if (wrapper) { + wrapper.unmount(); + wrapper = null; + } jest.useRealTimers(); }); // 在每个测试中 - const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) }); + wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) }); // ... 测试代码 - wrapper.unmount();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/touch.test.js(3 hunks)
🔇 Additional comments (5)
tests/touch.test.js (5)
74-91: 改动看起来不错！将触摸事件模拟和定时器推进包装在
act()中是正确的做法，确保了 React 的状态更新和副作用正确同步。
97-129: 新增的 origin scroll 测试合理测试验证了向下滑动（pageY 增加）时滚动位置保持为 0 的行为，这与向上滑动的测试形成了良好的对比。
act()包装确保了状态更新正确同步。
140-170: 正确使用 act() 包装异步操作将触摸事件处理和
jest.runAllTimers()包装在act()中是必要的，确保了 React 组件状态更新和定时器副作用的正确同步。
189-233: 迁移到 React Testing Library 的改动良好从 enzyme 迁移到 React Testing Library 是正确的选择，RTL 提供了更符合用户行为的测试方式。使用
fireEvent和act的组合确保了异步更新的正确处理。
191-191: 确认：tests 中 itemKey 用法与 getKey 实现一致
仓库中未找到 SharedConfig；src/List.tsx 接受 itemKey（string | function）、实现 getKey 并将其传入 useHeights、useDiffItem、useGetSize 等 hooks；tests 中使用 itemKey="id" 与实现匹配，可保持不变。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/hooks/useHeights.tsx (1)
93-96: useEffect 清理函数依赖应包含取消函数既然取消函数已被
useCallback包装，建议将其加入依赖数组以满足 exhaustive-deps 规则（若按上文重命名则替换为cancelSchedule）。- useEffect(() => { - return cancelRaf; - }, []); + useEffect(() => { + return cancelSchedule; + }, [cancelSchedule]);src/hooks/useChildren.tsx (1)
26-31: ref 回调可能捕获过期的 setRef（跨文件影响）Item.tsx 中
useCallback([])会固定首次的setRef，当这里传入的新回调变化时（例如切片或依赖变更后），旧的 ref 闭包可能被继续使用，导致卸载/替换阶段调用过期的回调。请在 src/Item.tsx 将依赖改为
[setRef]（保持函数签名不变）：export function Item({ children, setRef }: ItemProps) { - const refFunc = React.useCallback(node => { - setRef(node); - }, []); + const refFunc = React.useCallback((node) => { + setRef(node); + }, [setRef]); return React.cloneElement(children, { ref: refFunc, }); }
🧹 Nitpick comments (6)
src/hooks/useChildren.tsx (2)
15-16: useMemo 仅依赖 list 引用：原地修改 list 时可能跳过重算，导致子节点不更新若上游存在“就地变更 list（不创建新引用）”的用法，titleRender 等基于 item 内容的渲染可能不刷新。两种出路：
- 明确要求“不可变更新”并在文档注明；
- 或提供一个可选的“版本号”/“外部依赖”参数以显式触发重算（兼容但更稳妥）。
参考补丁（新增可选 dataVersion 依赖，向后兼容；调用方在就地变更时递增它）：
export default function useChildren<T>( list: T[], startIndex: number, endIndex: number, scrollWidth: number, offsetX: number, setNodeRef: (item: T, element: HTMLElement | null) => void, renderFunc: RenderFunc<T>, getKey: GetKey<T>, + dataVersion?: number, -) { +): React.ReactElement[] { // ... - }, [list, startIndex, endIndex, setNodeRef, renderFunc, getKey, offsetX, scrollWidth]); + }, [list, startIndex, endIndex, setNodeRef, renderFunc, getKey, offsetX, scrollWidth, dataVersion]); }Also applies to: 33-33
11-12: 类型不精确：setNodeRef 第二参应允许 nullref 回调在卸载时会传入
null。当前签名为HTMLElement容易造成类型与实际不符。建议改为：
- setNodeRef: (item: T, element: HTMLElement) => void, + setNodeRef: (item: T, element: HTMLElement | null) => void,src/hooks/useHeights.tsx (4)
27-30: 命名与实现不符：cancelRaf 实际取消的是 microtask 排程为避免误导维护者，建议重命名为
cancelSchedule（或更贴切命名），并统一替换引用。- const cancelRaf = React.useCallback(function cancelRaf() { + const cancelSchedule = React.useCallback(function cancelSchedule() { promiseIdRef.current += 1; }, []);并将后续
cancelRaf()/依赖项/cleanup 一并替换为cancelSchedule。
31-70: 从 rAF 改为 microtask：测量时机可能提前，需验证布局读取的准确性在复杂样式或异步布局下，microtask 可能更早触发布局读取（
offsetHeight+getComputedStyle），存在强制同步布局与抖动风险。请验证在 Expand/Collapse、滚动、窗口缩放等场景下高度是否稳定；如有问题，建议优先使用 rAF，无法使用时再回退到 microtask。可选改造（轻量回退到 rAF）：
- promiseIdRef.current += 1; - const id = promiseIdRef.current; - Promise.resolve().then(() => { - if (id === promiseIdRef.current) { - doCollect(); - } - }); + promiseIdRef.current += 1; + const id = promiseIdRef.current; + const schedule = + typeof window !== 'undefined' && 'requestAnimationFrame' in window + ? (cb: () => void) => window.requestAnimationFrame(() => cb()) + : (cb: () => void) => Promise.resolve().then(cb); + schedule(() => { + if (id === promiseIdRef.current) { + doCollect(); + } + });
16-17: 类型不准确：instance 实际可能为 nullref 回调在卸载时传入
null。建议将类型改为HTMLElement | null，避免 TS 与运行时不一致。- setInstanceRef: (item: T, instance: HTMLElement) => void, + setInstanceRef: (item: T, instance: HTMLElement | null) => void,- const setInstanceRef = React.useCallback(function setInstanceRef(item: T, instance: HTMLElement) { + const setInstanceRef = React.useCallback(function setInstanceRef(item: T, instance: HTMLElement | null) {Also applies to: 72-72
53-56: 注释与实现不一致代码仅在
changed为 true 时递增updatedMark，注释里的 “Always trigger” 与之不符，建议修正文案以免误导。- // Always trigger update mark to tell parent that should re-calculate heights when resized + // Trigger update mark only when heights actually changed-// This is somewhat confusing: when heightsRef.current.set is called, updatedMark changes, which in turn causes heightsRef.current to also change. +// Note: when heightsRef.current.set writes a new value, updatedMark increases to notify parent; the CacheMap instance stays the same.Also applies to: 97-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useChildren.tsx(2 hunks)src/hooks/useHeights.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useChildren.tsx (2)
src/interface.ts (1)
GetKey(7-7)src/Item.tsx (1)
Item(8-16)
🔇 Additional comments (1)
src/hooks/useChildren.tsx (1)
2-2: 公共 API 变更 — getKey 现为 useChildren 的直接参数：确认调用点与发版策略
- 仓库搜索结果：useChildren 定义于 src/hooks/useChildren.tsx（签名包含 getKey）；调用点为 src/List.tsx；未发现 SharedConfig 引用。
- 动作：确认 src/List.tsx 已按新签名传入 getKey；若对外行为变化，按语义化版本发布（至少 minor；破坏性改动为 major）。
muzea
force-pushed
the
hotfix/avoid-unnecessary-rerender
branch
from
1d636f8 to
e82a20d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hooks/useChildren.tsx (1)
16-33: 确认数据不可变契约，避免 useMemo 因原地 mutation 而漏更新useMemo 依赖包含
list引用但无法感知“原地修改 list 内部项”的变化。若使用方不更换数组引用，仅对项字段做 mutation，将导致不重算子元素、渲染停滞。请确认库契约要求传入的data为不可变（更换引用）或在文档中明确；否则需引入额外依赖信号（如范围内 key 快照或版本号）来触发更新。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Filler.tsx(2 hunks)src/List.tsx(2 hunks)src/hooks/useChildren.tsx(2 hunks)src/hooks/useHeights.tsx(3 hunks)src/interface.ts(0 hunks)tests/props.test.js(3 hunks)tests/scroll-Firefox.test.js(2 hunks)tests/scroll.test.js(5 hunks)tests/scrollWidth.test.tsx(1 hunks)tests/touch.test.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/interface.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/scroll.test.js
- tests/scroll-Firefox.test.js
- tests/props.test.js
- src/Filler.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useChildren.tsx (2)
src/interface.ts (1)
GetKey(7-7)src/Item.tsx (1)
Item(8-16)
🔇 Additional comments (11)
src/hooks/useHeights.tsx (1)
93-95: useEffect 清理依赖建议包含 cancelRaf
cancelRaf已用 useCallback 包装，建议将依赖从[]调整为[cancelRaf]以符合惯例并利于未来重构；当前行为无差异。- useEffect(() => { - return cancelRaf; - }, []); + useEffect(() => { + return cancelRaf; + }, [cancelRaf]);tests/scrollWidth.test.tsx (2)
233-236: 用 act 包裹 ref.scrollTo 调用，LGTM正确地将同步更新包裹在 act 内，避免测试时机问题与告警。
238-241: 二次 scrollTo 包装一致，LGTM同样的 act 包裹，保证断言前更新已冲刷完成。
src/List.tsx (2)
563-572: useChildren 按新签名直接传入 getKey，LGTM参数顺序与新定义一致（list, start, end, scrollWidth, offsetX, setRef, render, getKey）。
21-21: 全库核验通过：无 SharedConfig 残留，useChildren 调用已更新 全库未发现SharedConfig类型或变量引用，所有useChildren调用（仅在 List.tsx）均与新签名一致。tests/touch.test.js (5)
74-91: 将触摸交互与定时器推进放入 act，LGTM能确保更新批处理完成后再断言，提升用例稳定性。
97-127: 新增“origin scroll”用例并使用 act 包裹，LGTM覆盖向上滑动回弹为 0 的路径，行为更可控。
140-171: 在非可滚动场景下用 act 包裹并校验 preventDefault，LGTM时序更严谨，断言更可靠。
180-185: 容器 preventDefault 用 act 包裹，LGTM符合事件处理在测试中的最佳实践。
191-204: 嵌套用例切换至 RTL 风格并显式传入 itemKey，LGTM更贴近实际使用路径，也与本 PR 的 API 调整保持一致。
src/hooks/useChildren.tsx (1)
26-31: 修复：Item.tsx 中 ref 回调捕获过期 setRef，需更新 useCallback 依赖此处传入的是每次计算都会新建的内联 setRef。如果
src/Item.tsx里的refFunc仍然是useCallback([], ...)，会捕获首次传入的 setRef，后续替换/卸载场景可能失效。请将Item.tsx的依赖改为[setRef]（此前已有同类评论）。可在
src/Item.tsx中修改如下（示例代码片段）：export function Item({ children, setRef }: ItemProps) { const refFunc = React.useCallback((node) => { setRef(node); }, [setRef]); return React.cloneElement(children, { ref: refFunc }); }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/touch.test.js (2)
200-206: 可选：将 mockReset() 放到 act 外，拆分为两个独立的 act 以更清晰的时序避免把纯测试操作与 React 更新批在一起，读写边界更清楚（行为不变）。
- act(() => { - // ======= Not call since scroll to the bottom ======= - jest.runAllTimers(); - preventDefault.mockReset(); - // start - const touchEvent2 = new Event('touchstart'); - touchEvent2.touches = [{ pageY: 500 }]; - getElement().dispatchEvent(touchEvent2); - // move - const moveEvent2 = new Event('touchmove'); - moveEvent2.touches = [{ pageY: 0 }]; - moveEvent2.preventDefault = preventDefault; - getElement().dispatchEvent(moveEvent2); - }); + act(() => { + // ======= Not call since scroll to the bottom ======= + jest.runAllTimers(); + }); + preventDefault.mockReset(); + act(() => { + // start + const touchEvent2 = new Event('touchstart'); + touchEvent2.touches = [{ pageY: 500 }]; + getElement().dispatchEvent(touchEvent2); + // move + const moveEvent2 = new Event('touchmove'); + moveEvent2.touches = [{ pageY: 0 }]; + moveEvent2.preventDefault = preventDefault; + getElement().dispatchEvent(moveEvent2); + });
234-234: 确认 fireEvent 是否已自动包裹 act（依赖版本相关）多数版本下 @testing-library/react 的 fireEvent 会自动在 act 中执行，但不同版本行为略有差异。若需要完全一致的风格，可显式用 act 包裹这两次 fireEvent 调用（纯样式调整，不影响断言）。
是否需要我基于当前仓库的依赖版本帮你确认 fireEvent 的 act 包裹行为，并给出是否显式包裹的建议？
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/touch.test.js(3 hunks)
🔇 Additional comments (5)
tests/touch.test.js (5)
74-92: 用 act 包裹整段触摸序列与定时器推进，方向正确这能避免 “not wrapped in act(...)” 警告并提升稳定性。LGTM。
97-129: 第二个 “origin scroll” 场景同步到 act，保持一致性事件与定时器均放入 act，逻辑清晰。LGTM。
183-194: 首段“不可滚动时调用 preventDefault”的交互已放入 act，OK时序与上文一致。LGTM。
198-214: 第二段交互与定时器推进放入 act，OK整体一致性良好。LGTM。
223-227: 容器 preventDefault 也用 act 包裹，保持一致性能避免潜在的异步更新告警。LGTM。
tests/touch.test.js
Outdated
| it('origin scroll', () => { | ||
| const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) }); | ||
|
|
||
| function getElement() { | ||
| return wrapper.find('.rc-virtual-list-holder').instance(); | ||
| } | ||
|
|
||
| // start | ||
| const touchEvent = new Event('touchstart'); | ||
| touchEvent.touches = [{ pageY: 100 }]; | ||
| getElement().dispatchEvent(touchEvent); | ||
|
|
||
| // move | ||
| const moveEvent = new Event('touchmove'); | ||
| moveEvent.touches = [{ pageY: 90 }]; | ||
| getElement().dispatchEvent(moveEvent); | ||
| const moveEvent1 = new Event('touchmove'); | ||
| moveEvent1.touches = [{ pageY: 110 }]; | ||
| getElement().dispatchEvent(moveEvent1); | ||
|
|
||
| // move | ||
| const moveEvent2 = new Event('touchmove'); | ||
| moveEvent2.touches = [{ pageY: 150 }]; | ||
| getElement().dispatchEvent(moveEvent2); | ||
|
|
||
| // move | ||
| const moveEvent3 = new Event('touchmove'); | ||
| moveEvent3.touches = [{ pageY: 20 }]; | ||
| getElement().dispatchEvent(moveEvent3); | ||
|
|
||
| // move | ||
| const moveEvent4 = new Event('touchmove'); | ||
| moveEvent4.touches = [{ pageY: 100 }]; | ||
| getElement().dispatchEvent(moveEvent4); | ||
|
|
||
| // end | ||
| const endEvent = new Event('touchend'); | ||
| getElement().dispatchEvent(endEvent); | ||
|
|
||
| // smooth | ||
| jest.runAllTimers(); | ||
| expect(wrapper.find('ul').instance().scrollTop > 10).toBeTruthy(); | ||
|
|
||
| expect(wrapper.find('ul').instance().scrollTop).toBe(0); | ||
| wrapper.unmount(); |
There was a problem hiding this comment.
这里的触摸事件未包裹在 act 中，易产生告警与竞态
这段与前两段用例不一致，可能导致 “An update was not wrapped in act(...)” 警告或偶发现象。建议与上方一致，用 act 包裹整段事件与定时器推进；另外该测试名称与上一个相同，建议改为唯一名称，便于定位。
可按如下方式调整：
-it('origin scroll', () => {
+it('origin scroll - multi moves and bounce back', () => {
const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) });
function getElement() {
return wrapper.find('.rc-virtual-list-holder').instance();
}
- // start
- const touchEvent = new Event('touchstart');
- touchEvent.touches = [{ pageY: 100 }];
- getElement().dispatchEvent(touchEvent);
-
- // move
- const moveEvent1 = new Event('touchmove');
- moveEvent1.touches = [{ pageY: 110 }];
- getElement().dispatchEvent(moveEvent1);
-
- // move
- const moveEvent2 = new Event('touchmove');
- moveEvent2.touches = [{ pageY: 150 }];
- getElement().dispatchEvent(moveEvent2);
-
- // move
- const moveEvent3 = new Event('touchmove');
- moveEvent3.touches = [{ pageY: 20 }];
- getElement().dispatchEvent(moveEvent3);
-
- // move
- const moveEvent4 = new Event('touchmove');
- moveEvent4.touches = [{ pageY: 100 }];
- getElement().dispatchEvent(moveEvent4);
-
- // end
- const endEvent = new Event('touchend');
- getElement().dispatchEvent(endEvent);
-
- // smooth
- jest.runAllTimers();
+ act(() => {
+ // start
+ const touchEvent = new Event('touchstart');
+ touchEvent.touches = [{ pageY: 100 }];
+ getElement().dispatchEvent(touchEvent);
+
+ // move
+ const moveEvent1 = new Event('touchmove');
+ moveEvent1.touches = [{ pageY: 110 }];
+ getElement().dispatchEvent(moveEvent1);
+
+ // move
+ const moveEvent2 = new Event('touchmove');
+ moveEvent2.touches = [{ pageY: 150 }];
+ getElement().dispatchEvent(moveEvent2);
+
+ // move
+ const moveEvent3 = new Event('touchmove');
+ moveEvent3.touches = [{ pageY: 20 }];
+ getElement().dispatchEvent(moveEvent3);
+
+ // move
+ const moveEvent4 = new Event('touchmove');
+ moveEvent4.touches = [{ pageY: 100 }];
+ getElement().dispatchEvent(moveEvent4);
+
+ // end
+ const endEvent = new Event('touchend');
+ getElement().dispatchEvent(endEvent);
+
+ // smooth
+ jest.runAllTimers();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('origin scroll', () => { | |
| const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) }); | |
| function getElement() { | |
| return wrapper.find('.rc-virtual-list-holder').instance(); | |
| } | |
| // start | |
| const touchEvent = new Event('touchstart'); | |
| touchEvent.touches = [{ pageY: 100 }]; | |
| getElement().dispatchEvent(touchEvent); | |
| // move | |
| const moveEvent = new Event('touchmove'); | |
| moveEvent.touches = [{ pageY: 90 }]; | |
| getElement().dispatchEvent(moveEvent); | |
| const moveEvent1 = new Event('touchmove'); | |
| moveEvent1.touches = [{ pageY: 110 }]; | |
| getElement().dispatchEvent(moveEvent1); | |
| // move | |
| const moveEvent2 = new Event('touchmove'); | |
| moveEvent2.touches = [{ pageY: 150 }]; | |
| getElement().dispatchEvent(moveEvent2); | |
| // move | |
| const moveEvent3 = new Event('touchmove'); | |
| moveEvent3.touches = [{ pageY: 20 }]; | |
| getElement().dispatchEvent(moveEvent3); | |
| // move | |
| const moveEvent4 = new Event('touchmove'); | |
| moveEvent4.touches = [{ pageY: 100 }]; | |
| getElement().dispatchEvent(moveEvent4); | |
| // end | |
| const endEvent = new Event('touchend'); | |
| getElement().dispatchEvent(endEvent); | |
| // smooth | |
| jest.runAllTimers(); | |
| expect(wrapper.find('ul').instance().scrollTop > 10).toBeTruthy(); | |
| expect(wrapper.find('ul').instance().scrollTop).toBe(0); | |
| wrapper.unmount(); | |
| it('origin scroll - multi moves and bounce back', () => { | |
| const wrapper = genList({ itemHeight: 20, height: 100, data: genData(100) }); | |
| function getElement() { | |
| return wrapper.find('.rc-virtual-list-holder').instance(); | |
| } | |
| act(() => { | |
| // start | |
| const touchEvent = new Event('touchstart'); | |
| touchEvent.touches = [{ pageY: 100 }]; | |
| getElement().dispatchEvent(touchEvent); | |
| // move | |
| const moveEvent1 = new Event('touchmove'); | |
| moveEvent1.touches = [{ pageY: 110 }]; | |
| getElement().dispatchEvent(moveEvent1); | |
| // move | |
| const moveEvent2 = new Event('touchmove'); | |
| moveEvent2.touches = [{ pageY: 150 }]; | |
| getElement().dispatchEvent(moveEvent2); | |
| // move | |
| const moveEvent3 = new Event('touchmove'); | |
| moveEvent3.touches = [{ pageY: 20 }]; | |
| getElement().dispatchEvent(moveEvent3); | |
| // move | |
| const moveEvent4 = new Event('touchmove'); | |
| moveEvent4.touches = [{ pageY: 100 }]; | |
| getElement().dispatchEvent(moveEvent4); | |
| // end | |
| const endEvent = new Event('touchend'); | |
| getElement().dispatchEvent(endEvent); | |
| // smooth | |
| jest.runAllTimers(); | |
| }); | |
| expect(wrapper.find('ul').instance().scrollTop).toBe(0); | |
| wrapper.unmount(); | |
| }); |
🤖 Prompt for AI Agents
tests/touch.test.js around lines 131 to 171: the touch event sequence and timer
advances are not wrapped in React's act and the test name duplicates the
previous one; wrap the entire sequence of dispatching
touchstart/touchmove/touchend events and jest.runAllTimers() inside act(...) to
avoid "An update was not wrapped in act(...)" warnings and change the it(...)
description to a unique name (e.g., "origin scroll - touch events wrapped in
act") so the test is distinguishable.
|
@gemini-code-assist pls help to check on this. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| }; | ||
| } | ||
|
|
||
| const handleResize = React.useCallback( |
There was a problem hiding this comment.
| const promiseIdRef = useRef<number>(0); | ||
|
|
||
| function cancelRaf() { | ||
| const cancelRaf = React.useCallback(function cancelRaf() { |
There was a problem hiding this comment.
不需要包 useCallback，原本代码里也是不加到 deps 里的。这里加了下面放到 effect 的 deps 是没有必要的。
| }, []); | ||
|
|
||
| function collectHeight(sync = false) { | ||
| const collectHeight = React.useCallback(function (sync = false) { |
There was a problem hiding this comment.
collectHeight 应该是没有作为过条件，也是不需要 useCallback 的
| </Item> | ||
| ); | ||
| }); | ||
| }, [list, startIndex, endIndex, setNodeRef, renderFunc, getKey, offsetX, scrollWidth]); |
There was a problem hiding this comment.
稍微有点复杂了，未来如果加 props 也容易 break。如果是只是为了 Item 不需要重新渲染，可以考虑 Item 直接用 React.memo 包一下，条件里直接忽略 setRef 即可。
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
fix ant-design/ant-design#34182
Summary by CodeRabbit
重构
测试
文档
变更