fix: Adjust the top height of the proxy to solve the sliding issue (#… #1313
Conversation
…eact-component#1310) (cherry picked from commit 1b655a7)
Walkthrough在 BodyGrid 的运行时 ref 上新增 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Ref使用方
participant Body as BodyGrid
participant List as rc-virtual-list(listRef)
note over Body, List: 读取当前位置（getter）
User->>Body: 访问 ref.scrollTop
Body->>List: getScrollInfo()
List-->>Body: { x, y }
Body-->>User: y
rect rgba(230,245,255,0.5)
note right of User: 设置滚动位置（setter）
User->>Body: ref.scrollTop = 200
Body->>List: scrollTo({ top: 200 })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Wxh16144, 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 known issue where the scrollTop property of a virtualized table component was not functioning correctly, leading to a 'sliding issue' in the UI. The changes ensure that the scrollTop property accurately reflects and controls the vertical scroll position of the virtualized list, providing a smoother and more predictable user experience.
Highlights
- Scroll Behavior Fix: Implemented a custom
scrollTopgetter and setter on theBodyGridcomponent's ref object to correctly manage vertical scrolling within the virtualized table. - Bug Resolution: Resolved a specific Ant Design issue (#54734) where the
scrollTopproperty was not functioning as expected, which was causing a 'sliding issue' in the UI. - Test Coverage: Added comprehensive unit tests to validate the proper functioning of the
scrollTopproperty, ensuring accurate scroll position retrieval and setting.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request addresses a scrolling issue by introducing a scrollTop property on the virtual table's body grid ref, enabling programmatic vertical scrolling. The changes are implemented in BodyGrid.tsx and are accompanied by new tests in Virtual.spec.tsx that validate this new functionality. My feedback focuses on improving test reliability by ensuring proper state reset between test cases.
| setScrollLeft.mockReset(); | ||
| global.scrollToConfig = null; | ||
| global.scrollToConfig = vi.fn(identity); | ||
| // global.collectGetScrollInfoReturn.mockReset(); |
There was a problem hiding this comment.
To ensure proper test isolation and prevent state from leaking between tests, global.collectGetScrollInfoReturn should be reset to its default identity function before each test runs. The commented-out mockReset() call is not suitable here, as this global variable is not a mock function.
| // global.collectGetScrollInfoReturn.mockReset(); | |
| global.collectGetScrollInfoReturn = identity; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1313 +/- ##
==========================================
- Coverage 96.18% 96.07% -0.11%
==========================================
Files 57 57
Lines 3408 3416 +8
Branches 620 621 +1
==========================================
+ Hits 3278 3282 +4
- Misses 125 129 +4
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (7)
src/VirtualTable/BodyGrid.tsx (3)
107-117: 为 ref 新增 scrollTop 合理，但 getter/setter 可更稳健（改用 null 合并与额外可选链）
- 建议用
?? 0替代|| 0，避免极端情况下的真假值陷阱。- 同时对
getScrollInfo()的返回再做一次可选链，增强健壮性。- 可选：为属性设定
configurable: true，便于 HMR 或后续重定义（默认configurable: false）。可在本段内应用如下 diff：
- Object.defineProperty(obj, 'scrollTop', { - get: () => listRef.current?.getScrollInfo().y || 0, + Object.defineProperty(obj, 'scrollTop', { + get: () => listRef.current?.getScrollInfo()?.y ?? 0, set: (value: number) => { listRef.current?.scrollTo({ top: value, }); }, - }); + configurable: true, + });另外，为保持一致性，建议对上方
scrollLeft的 getter 也做同样处理（可在后续一起修改）。
15-19: 同步补全类型：在 GridRef 接口中声明 scrollTop运行时通过
defineProperty暴露了scrollTop，但类型未声明，消费者将被迫使用any/类型断言。该变更是向后兼容的（仅新增属性），建议补上类型声明。export interface GridRef { scrollLeft: number; + scrollTop: number; nativeElement: HTMLDivElement; scrollTo: (scrollConfig: ScrollConfig) => void; }
97-105: 可选：将 scrollLeft 的 getter 改为与 scrollTop 一致的空值处理，并设为 configurable与上方
scrollTop一致，提升一致性与健壮性。可选项，不影响当前功能。- Object.defineProperty(obj, 'scrollLeft', { - get: () => listRef.current?.getScrollInfo().x || 0, + Object.defineProperty(obj, 'scrollLeft', { + get: () => listRef.current?.getScrollInfo()?.x ?? 0, set: (value: number) => { listRef.current?.scrollTo({ left: value, }); }, - }); + configurable: true, + });tests/Virtual.spec.tsx (4)
12-12: 初始化全局转换器 OK，但建议在 beforeEach 中重置，避免跨用例状态泄漏当前仅在顶层赋值，后续个别用例（如“should get and set scrollTop correctly”）会覆盖该全局函数。为防止对其他用例造成干扰，建议在
beforeEach中也恢复默认的identity。在 66-73 行的
beforeEach中追加一行重置：beforeEach(() => { scrollLeftCalled = false; setScrollLeft.mockReset(); - global.scrollToConfig = vi.fn(identity); + global.scrollToConfig = vi.fn(identity); + global.collectGetScrollInfoReturn = identity; // global.collectGetScrollInfoReturn.mockReset(); vi.useFakeTimers(); resetWarned(); });
26-29: 给 getScrollInfo 增加兜底返回，避免底层实现变化导致的偶发报错当前假设
myRef.current.getScrollInfo一定存在。为让测试桩在底层库变更（或版本不一致）时更具韧性，可添加兜底{ x: 0, y: 0 }。- getScrollInfo: () => { - const originResult = myRef.current.getScrollInfo(); - return global.collectGetScrollInfoReturn(originResult); - }, + getScrollInfo: () => { + const current = myRef.current as any; + const originResult = + current && typeof current.getScrollInfo === 'function' + ? current.getScrollInfo() + : { x: 0, y: 0 }; + return global.collectGetScrollInfoReturn(originResult); + },
69-70: 初始化 scrollToConfig 为 vi.fn 会被后续覆盖为对象；可直接置空以避免歧义（可选）
rc-virtual-list的代理在每次调用时都会把global.scrollToConfig置为配置对象，因此这里用vi.fn并不会被真正利用。为了避免读者误以为该变量后续会保留为 mock 函数，可直接初始化为null。已删除的mockReset是正确的。- global.scrollToConfig = vi.fn(identity); + global.scrollToConfig = null; // global.collectGetScrollInfoReturn.mockReset();
618-638: 新增用例覆盖 ref.scrollTop 读写很到位；补充清理逻辑以避免对其他用例影响该用例会覆写
global.collectGetScrollInfoReturn，建议在用例结束后恢复，避免潜在的状态泄漏。可在本用例内加入 try/finally 包裹：
it('should get and set scrollTop correctly', async () => { const ref = React.createRef<any>(); global.collectGetScrollInfoReturn = (origin: any) => ({ ...origin, y: 50, }); - getTable({ internalRefs: { body: ref } }); - - expect(ref.current.scrollTop).toBe(50); - - ref.current.scrollTop = 200; - expect(global.scrollToConfig).toEqual({ - top: 200, - }); + try { + getTable({ internalRefs: { body: ref } }); + expect(ref.current.scrollTop).toBe(50); + ref.current.scrollTop = 200; + expect(global.scrollToConfig).toEqual({ + top: 200, + }); + } finally { + // 防止影响其他用例 + global.collectGetScrollInfoReturn = identity; + } });或替代地，在文件级
afterEach（75-79 行）中统一恢复也可行。
📜 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 (2)
src/VirtualTable/BodyGrid.tsx(1 hunks)tests/Virtual.spec.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bbb169
PR: react-component/table#1202
File: src/Table.tsx:903-904
Timestamp: 2024-11-08T12:53:09.293Z
Learning: 在 `src/Table.tsx` 文件的 React 组件 `Table` 中，即使 `bodyScrollLeft` 频繁更新，也需要在 `TableContextValue` 的 `useMemo` 依赖数组中包含 `bodyScrollLeft` 和 `headerCellRefs`，因为每次滚动时重新计算 `TableContextValue` 是解决该问题所必须的。
📚 Learning: 2024-11-08T12:53:09.293Z
Learnt from: bbb169
PR: react-component/table#1202
File: src/Table.tsx:903-904
Timestamp: 2024-11-08T12:53:09.293Z
Learning: 在 `src/Table.tsx` 文件的 React 组件 `Table` 中，即使 `bodyScrollLeft` 频繁更新，也需要在 `TableContextValue` 的 `useMemo` 依赖数组中包含 `bodyScrollLeft` 和 `headerCellRefs`，因为每次滚动时重新计算 `TableContextValue` 是解决该问题所必须的。
Applied to files:
src/VirtualTable/BodyGrid.tsxtests/Virtual.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (1)
tests/Virtual.spec.tsx (1)
9-10: 轻量工具函数命名与用途清晰，便于阅读顶层
identity的引入与后续复用合理，提升可读性。
2 participants
…1310)
(cherry picked from commit 1b655a7)
Summary by CodeRabbit