fix: resolve scroll key against latest data - #376
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough滚动同步状态新增可选 Changes按 key 同步滚动
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
React Doctor found no issues. 🎉
|
❌ Deploy failed
📋 Build log (last lines)🤖 Powered by surge-preview |
|||||||||
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 96.87% 96.88% +0.01%
==========================================
Files 19 19
Lines 832 836 +4
Branches 206 205 -1
==========================================
+ Hits 806 810 +4
Misses 26 26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| const { targetAlign, originAlign, index, offset: rawOffset } = syncState; | ||
| const { targetAlign, originAlign, offset: rawOffset } = syncState; | ||
| const index = | ||
| syncState.index < 0 && 'key' in syncState |
There was a problem hiding this comment.
逻辑应该是 index >= 0 就使用 index，而不需要关注 key 是否存在。只有 index 不合法才需要去查找 index。
|
|
||
| const height = containerRef.current.clientHeight; | ||
| let needCollectHeight = false; | ||
| let needCollectHeight = 'key' in syncState && index < 0; |
| } | ||
|
|
||
| const { offset: rawOffset = 0 } = arg; | ||
| const target = 'index' in arg ? { index: arg.index } : { index: -1, key: arg.key }; |
There was a problem hiding this comment.
不需要这个，你下面还是塞 index 和 key。上面让它自行判断是否 index 合法就行了。
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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/hooks/useScrollTo.tsx (1)
99-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win同时校验
index的上界。
index >= 0不能证明索引仍属于当前data。当列表缩短后，缓存索引可能满足index >= data.length。此时maxLen会把遍历截到最后一项，itemTop和itemBottom会对应错误的目标。空列表也会进入定位分支。请把index < data.length纳入有效性判断；如果存在syncState.key，对越界索引重新执行getKey查找。建议修改
const index = - syncState.index >= 0 + syncState.index >= 0 && syncState.index < data.length ? syncState.index - : data.findIndex((item) => getKey(item) === syncState.key); + : syncState.key !== undefined + ? data.findIndex((item) => getKey(item) === syncState.key) + : -1; ... - if (height && index >= 0) { + if (height && index >= 0 && index < data.length) {🤖 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 `@src/hooks/useScrollTo.tsx` around lines 99 - 105, Update the scroll-target validity check in the visible scroll handling logic to require index < data.length alongside height and index >= 0, preventing empty or shortened lists from entering the positioning path. When syncState.key is present and the cached index is out of bounds, rerun getKey to resolve the target index before calculating maxLen, itemTop, and itemBottom.
🤖 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.
Inline comments:
In `@src/hooks/useScrollTo.tsx`:
- Around line 85-89: 让 useScrollTo 中解析 syncState.index 的同步 effect 将 data
加入依赖数组，使数据更新后重新查找目标并执行滚动。保持现有 syncState 与容器引用依赖不变，并为目标跨两个 render 才出现在 data
中的场景补充回归测试。
---
Outside diff comments:
In `@src/hooks/useScrollTo.tsx`:
- Around line 99-105: Update the scroll-target validity check in the visible
scroll handling logic to require index < data.length alongside height and index
>= 0, preventing empty or shortened lists from entering the positioning path.
When syncState.key is present and the cached index is out of bounds, rerun
getKey to resolve the target index before calculating maxLen, itemTop, and
itemBottom.
🪄 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 Plus
Run ID: f88adea6-e578-4140-b8f8-600d544744ce
📒 Files selected for processing (2)
src/hooks/useScrollTo.tsxtests/scroll.test.js
| const { targetAlign, originAlign, offset: rawOffset } = syncState; | ||
| const index = | ||
| syncState.index >= 0 | ||
| ? syncState.index | ||
| : data.findIndex((item) => getKey(item) === syncState.key); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
请让同步 effect 在 data 变化时重新运行。
index 的解析依赖当前 data，但该 effect 的依赖数组没有 data。如果先调用 scrollTo({ key: '30' })，目标尚不存在，第一次 effect 会保存 index = -1 并结束。之后列表在另一次 render 中加入目标项时，syncState 和容器引用都不变，effect 不会再次执行，滚动请求会永久停留在 -1。请将 data 加入依赖数组，并补充跨两个 render 的回归测试。
建议修改
- }, [syncState, containerRef.current]);
+ }, [syncState, data, containerRef.current]);🤖 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 `@src/hooks/useScrollTo.tsx` around lines 85 - 89, 让 useScrollTo 中解析
syncState.index 的同步 effect 将 data 加入依赖数组，使数据更新后重新查找目标并执行滚动。保持现有 syncState
与容器引用依赖不变，并为目标跨两个 render 才出现在 data 中的场景补充回归测试。
1 participant
Summary
Resolve
scrollTo({ key })against the latest list data during scroll retries.Root cause
The key was converted to an index when
scrollTowas called. If the same update added the target item, retries kept using the stale-1index and could not scroll to it.Changes
Test
ut test -- --runInBandut run tscut run lint -- src/hooks/useScrollTo.tsx tests/scroll.test.jsRelated to ant-design/ant-design#58841.
Summary by CodeRabbit
新功能
问题修复
测试