-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enhance: Scroll top should not shaking #296
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
examples/basic.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /.eslintrc.js
概述演练这次更改涉及虚拟列表组件的性能和滚动行为优化。主要修改包括在 变更
诗歌
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #296 +/- ##
==========================================
- Coverage 98.32% 97.49% -0.84%
==========================================
Files 19 19
Lines 778 799 +21
Branches 187 188 +1
==========================================
+ Hits 765 779 +14
- Misses 13 20 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/hooks/useHeights.tsx (1)
29-29
: 以自增的方式替代原先raf
取消逻辑。
通过promiseIdRef.current += 1
来阻断上一次非同步流程,减少了对浏览器帧调度的依赖,但需确保在过多异步场景中不会导致过度频繁的promiseIdRef
更新。examples/basic.tsx (2)
13-14
: 将动态高度改为固定 100 的实现可能掩盖部分真实场景。
在演示环境下此做法确实可简化,但若要测试不同高度对滚动的影响,仍需保留原来的动态逻辑或额外提供测试用例。
48-48
: 注释掉console.log
可减少调试输出,但请保持必要的日志。
若仍需观测滚动位置,建议改用可控的日志或埋点,以便排查滚动问题。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/basic.tsx
(3 hunks)src/List.tsx
(1 hunks)src/hooks/useHeights.tsx
(2 hunks)src/utils/CacheMap.ts
(1 hunks)
🔇 Additional comments (8)
src/utils/CacheMap.ts (3)
11-12
: 在 CacheMap 中新增diffKeys
属性的做法值得肯定。
此属性可以追踪键值变动,有助于后续渲染逻辑判定何时触发更新。
20-20
: 在set
方法中将 key 加入diffKeys
时,注意类型转换。
当前通过key as string
转换类型后存储,若后续需要直接使用纯粹的React.Key
,请确保冲突情形(如数字型 key 与 string 同值)能正确排查。
27-37
: 新增的resetRecord
、getRecord
方法能够有效管理并访问被修改的 key 列表。
resetRecord()
清空变动记录后,有助于在后续渲染或滚动同步时避免重复计算。getRecord()
返回Set<React.Key>
,便于其他组件判断是否有变动。src/hooks/useHeights.tsx (2)
25-26
: 通过useRef(new CacheMap())
与promiseIdRef
配合,简化了高度收集的管理。
heightsRef
中使用自定义容器可更灵活地跟踪高度变化,promiseIdRef
则在异步场景下有效区分多次更新。
59-65
: 使用 Promise 的异步回调替代requestAnimationFrame
。
此做法能保证只执行最新批次的高度收集逻辑,但仍需确认在某些低性能环境中是否存在丢帧或时序问题。若后续需要更流畅的视觉效果,或可考虑结合requestIdleCallback
等手段。examples/basic.tsx (1)
57-62
: 使用useEffect
在挂载时强制滚动到较大索引。
此做法可检验列表在极端索引下的渲染与滚动情况,但在手机端或性能弱的设备上可能有卡顿风险。
[approve]src/List.tsx (2)
274-290
: 在useLayoutEffect
中同步首个可见条目的真实高度,避免向上滚动时的跳动问题。
- 逻辑仅限记录一处变动 (
changedRecord.size === 1
),若实际场景中第一可见条目同时发生多种变动,后续可能还需扩展。- 当真实高度与
itemHeight
不匹配时,通过syncScrollTop
调整滚动位置能有效防止抖动。
291-293
: 调用heights.resetRecord()
清空变动记录。
此操作可防止重复侦测同一条目的高度变动,让后续渲染周期更精准。
ref ant-design/ant-design#52121
Note: jsdom 下无法对该行为进行模拟
Summary by CodeRabbit
新功能
性能优化
requestAnimationFrame
依赖Bug 修复