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 192 -1
=======================================
+ 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 用户
participant Filler as Filler
participant RO as ResizeObserver
participant Parent as onInnerResize
用户->>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) -> 使旧 raf 失效
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 (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (2)
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 |
|
最近事情比较多，我看看这几天 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 |
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 |
|
|
||
| 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]); |
![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)
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}
src/Filler.tsx
Outdated
| }; | ||
| } | ||
|
|
||
| const handleResize = React.useCallback( |
There was a problem hiding this comment.
src/hooks/useHeights.tsx
Outdated
| const promiseIdRef = useRef<number>(0); | ||
|
|
||
| function cancelRaf() { | ||
| const cancelRaf = React.useCallback(function cancelRaf() { |
There was a problem hiding this comment.
不需要包 useCallback，原本代码里也是不加到 deps 里的。这里加了下面放到 effect 的 deps 是没有必要的。
src/hooks/useHeights.tsx
Outdated
| }, []); | ||
|
|
||
| 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>
muzea
force-pushed
the
hotfix/avoid-unnecessary-rerender
branch
from
6a3ee26 to
169288c
Compare
|
@muzea is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/hooks/useHeights.tsx (3)
27-29: 参考先前的评审意见根据 zombieJ 之前的评论，
cancelRaf可能不需要包裹在useCallback中，因为它在原始代码中也没有被添加到依赖数组里。虽然这个优化在技术上没有问题，但请确认是否真的需要这个 memoization，特别是考虑到该函数非常简单且没有外部依赖。
31-70: 参考先前的评审意见zombieJ 之前指出
collectHeight应该不需要useCallback。虽然添加依赖[cancelRaf]在技术上是正确的，但请验证这个 memoization 是否真的能带来性能提升。如果collectHeight在下游组件中被用作依赖项，这个优化才有意义。基于学习
93-95: 建议添加cancelRaf到依赖数组虽然
cancelRaf通过空依赖数组 memoized 后是稳定的，但为了符合 React 的 exhaustive-deps 规则和代码一致性，建议将cancelRaf添加到 useEffect 的依赖数组中。这也与之前 Copilot 的评审建议一致。应用此 diff：
useEffect(() => { return cancelRaf; - }, []); + }, [cancelRaf]);基于学习
tests/scroll.test.js (1)
161-164: 考虑移除冗余的 act() 包裹。
presetList返回的scrollTo方法内部已经使用act()包裹并调用了jest.runAllTimers()（见 Line 134-137）。此处的外层act()包裹和额外的jest.runAllTimers()调用是冗余的。其他使用相同 helper 的测试（Line 146、154、170、176-182）都是直接调用
scrollTo而不额外包裹，建议保持一致：- act(() => { - scrollTo({ index: 0, align: 'bottom' }); - jest.runAllTimers(); - }); + scrollTo({ index: 0, align: 'bottom' });虽然嵌套的
act()和重复的定时器刷新不会导致错误，但移除冗余代码可以提高一致性。
📜 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 (7)
- src/Filler.tsx
- tests/props.test.js
- tests/scrollWidth.test.tsx
- tests/touch.test.js
- src/List.tsx
- src/hooks/useChildren.tsx
- tests/scroll-Firefox.test.js
🔇 Additional comments (5)
src/hooks/useHeights.tsx (1)
72-91: Based on the verification results, getKey is already wrapped with React.useCallback at the parent component level in List.tsx, with dependency [itemKey]. This means whenuseHeightsis called withgetKeyas a parameter, it receives a stable, memoized function reference. Therefore,setInstanceRef's dependency optimization is effective and working as intended.The original review comment incorrectly assumes that
getKeymay not be stable in the parent component. In reality, the code is already properly optimized.无需修改代码。父组件已正确地对
getKey进行了 memoization
getKey在src/List.tsx中已通过useCallback进行了包装，确保了稳定的函数引用。因此，setInstanceRef的依赖项优化是有效的，代码已正确实现。tests/scroll.test.js (4)
98-102: 测试同步处理正确。将
jest.runAllTimers()和scrollTo(null)包裹在act()中是正确的做法，确保 React 状态更新在断言前完成。
114-117: LGTM！正确地将滚动操作和定时器刷新包裹在
act()中，符合 React 测试最佳实践。
125-140: 辅助函数设计良好。
presetList辅助函数封装了act()包裹逻辑，使测试代码更简洁。返回的scrollTo方法内部已经处理了 React 状态同步，这是一个很好的抽象模式。
400-404: RTL 测试同步处理正确。与 Line 98-102 的模式一致，正确地在
act()中处理定时器和滚动操作。
4 participants
fix ant-design/ant-design#34182
Summary by CodeRabbit
重构
测试
文档
变更