fix: Bubble.List scroll compatible with safari #1392
Conversation
|
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 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🎯 概览新增 useCompatibleScroll React 钩子实现 Safari 兼容的反向弹性容器滚动锁定，集成至 BubbleList 组件，新增comprehensive测试套件，更新文档演示及样式注释。 📋 变更
🔄 序列图sequenceDiagram
participant App as 应用
participant BubbleList as BubbleList<br/>组件
participant useCSHook as useCompatibleScroll<br/>钩子
participant DOM as DOM/<br/>容器
participant IO as IntersectionObserver
participant MO as MutationObserver
participant ScrollEvent as 滚动事件
App->>BubbleList: 挂载/传入 autoScroll
BubbleList->>useCSHook: 初始化钩子<br/>(scrollBoxRef)
alt 启用滚动锁定
useCSHook->>DOM: 创建哨兵元素<br/>(底部标记)
useCSHook->>IO: 注册IntersectionObserver<br/>监听哨兵可见性
useCSHook->>MO: 注册MutationObserver<br/>监听DOM变化
end
Note over DOM,ScrollEvent: 用户交互或内容变化
DOM->>ScrollEvent: 触发滚动事件
ScrollEvent->>useCSHook: 捕获阶段监听<br/>更新 lockedScrollBottomPos
alt 内容高度变化
DOM->>MO: 变动回调触发
MO->>useCSHook: 检测是否需要<br/>锁定滚动
alt 需要锁定
useCSHook->>useCSHook: enforceScrollLock()<br/>计算目标 scrollTop
useCSHook->>DOM: 应用 scrollTop
end
end
IO->>useCSHook: 哨兵可见性变化
useCSHook->>useCSHook: 更新 isAtBottom 状态
BubbleList->>useCSHook: 调用 reset()<br/>清除锁定状态
alt 卸载/禁用
useCSHook->>IO: 移除观察器
useCSHook->>MO: 移除观察器
useCSHook->>DOM: 移除哨兵元素
end
⏱️ 代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 需要重点关注的区域：
🐰 诗赋
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @anxLiang, 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 resolves a bug affecting the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
这个 PR 旨在修复 Bubble.List 组件在 auto-scroll 模式下（即 flex-direction: column-reverse）于 Safari 浏览器中的滚动兼容性问题。核心实现是新增了一个自定义 Hook useCompatibleScroll，它通过 IntersectionObserver 和 MutationObserver 来实现当用户向上滚动后锁定视窗位置的功能。这是一个很棒的解决方案。同时，作者为这个新 Hook 编写了非常全面的测试用例，值得称赞。
我对 useCompatibleScroll 这个 Hook 有两个建议，以提升其代码健壮性和性能。其中一个是关于 handleScroll 中存在陈旧闭包（stale closure）的高优先级 bug 修复，另一个是关于缓存一个昂贵计算的性能优化。
总体而言，这是一个高质量的 PR。
Deploying ant-design-x with
|
| Latest commit: |
7d45a4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a46beb82.ant-design-x.pages.dev |
| Branch Preview URL: | https://fix-bubble.ant-design-x.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
packages/x/components/bubble/style/list.ts (1)
28-28: 移除无用的注释代码或添加说明。这行被注释的代码看起来像是调试或探索性代码的遗留。如果不需要，建议删除；如果有保留的原因（例如作为未来参考），请添加说明注释。
[`${componentCls}-list-scroll-box`]: { - // position: 'relative', overflowY: 'auto',packages/x/components/bubble/demo/_test.md (1)
5-7: 英文文档的语法可以优化。根据静态分析提示，建议进行以下改进：
so as to→to（更简洁）high performance→high-performance（作为形容词修饰 rendering 时应使用连字符）-Bubble list with preset styles, supports automatic scrolling, supports using `role` to define different types of bubbles and set properties. **Bubble.List** is a controlled component, and **Bubble** is internally memoized, so it is recommended to use **setState** callback form to modify the `items` property, and try to ensure the stability of the configuration of non-essential rendering data items, so as to ensure the high performance rendering of **Bubble.List**. +Bubble list with preset styles, supports automatic scrolling, supports using `role` to define different types of bubbles and set properties. **Bubble.List** is a controlled component, and **Bubble** is internally memoized, so it is recommended to use **setState** callback form to modify the `items` property, and try to ensure the stability of the configuration of non-essential rendering data items to ensure the high-performance rendering of **Bubble.List**.packages/x/components/bubble/demo/_test.tsx (2)
21-23: 全局变量id在热重载时可能导致问题。
let id = 0是模块级变量，在热模块替换（HMR）期间不会重置，可能导致 key 不一致或调试困难。对于演示代码这可能是可接受的，但建议使用useRef来管理 ID 计数器。
63-66:useBubbleList返回值未完全使用。hook 返回
[items, setItems, add, update]，但只解构了[items, set]。add和update函数未被使用。如果这些函数是为了演示更多用法而设计的，这是合理的；否则可以简化 hook 的返回值。packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
35-75:enforceScrollLock应添加到 effect 的依赖数组中。
enforceScrollLock在 MutationObserver 回调中被调用，但未包含在 effect 的依赖数组中。虽然当前实现因为enforceScrollLock只依赖dom（已在依赖中）而可能不会出问题，但这不符合 React hooks 的最佳实践，且 ESLint exhaustive-deps 规则会报警。return () => { intersectionObserver.disconnect(); mutationObserver.disconnect(); }; - }, [dom, disable]); + }, [dom, disable, enforceScrollLock]);注意：这需要将
enforceScrollLock的定义移到 effect 之前，或者使用useRef来存储函数引用以避免循环依赖。packages/x/components/bubble/__tests__/list-scroll.test.tsx (6)
4-20: UA mock 建议恢复原值以提升测试隔离性这里通过
Object.defineProperty(navigator, 'userAgent', …)改写全局 UA，但没有在afterEach/afterAll中恢复原值。当前文件内每个依赖 UA 的用例都会显式调用mockUserAgent，所以功能上没问题，但如果后续有其他测试依赖真实 UA，可能被这里的 mock 污染。建议在beforeAll里缓存原始 UA，在afterAll里恢复，或者使用jest.spyOn(window.navigator, 'userAgent', 'get')方式更易维护。
90-130: 全局重写 IntersectionObserver/MutationObserver/requestAnimationFrame 建议显式恢复
beforeEach中直接重写了global.IntersectionObserver、global.MutationObserver和global.requestAnimationFrame，afterEach只做了jest.clearAllMocks()，不会还原全局对象本身。这意味着一旦这个文件被执行，其 mock 会在整个测试进程中常驻，可能影响其他依赖这些 Web API 的用例。建议：
- 在
beforeAll缓存原始引用；- 在
afterAll显式恢复全局到原始实现。这样既不影响本文件测试，又能避免对其它测试套件的隐性影响。
244-275: 部分滚动相关用例断言过于薄弱，未真正验证目标行为
it('should handle intersection observer callback correctly', …)和it('should handle scroll event and update locked position', …)：
- 前者只在调用
intersectionCallback后断言mockDom.scrollTop === 0，但scrollTop初始就是 0，而且IntersectionObserver回调实现本身并不会修改scrollTop，所以即使回调完全不生效，这个断言也会通过。- 后者的描述是 “update locked position”，但测试只断言
scrollTop仍然是 -500，并没有验证lockedScrollBottomPos.current是否按预期更新。这类测试名和实际断言不匹配，容易给人一种“覆盖到了逻辑”的错觉。建议要么删除这些用例，要么改成更贴近实现的断言（例如通过触发 mutation 并检查最终
scrollTop是否按锁定位置计算变化），避免纯覆盖驱动的“空心”测试。
345-360:resetToBottom用例与描述不符，实质只重复类型检查
it('should reset to bottom correctly', …)用例内部只是调用了一次result.current.resetToBottom()，最后又断言了一次typeof result.current.resetToBottom === 'function'。这与文件开头it('should return resetToBottom function', …)的用例基本重复，并没有验证“重置到底部”的行为（例如内部 ref 状态或后续滚动效果）。建议要么移除这个用例，要么改为通过先模拟离开底部（intersection false + scroll），再调用
resetToBottom()并结合 BubbleList 行为验证“回到底部”的实际效果。
362-372: “cleanup observers on unmount” 用例未真正断言清理逻辑
it('should cleanup observers on unmount', …)当前只校验了mockIntersectionObserver和mockMutationObserver被调用过，这证明观察者被创建，但并没有断言disconnect是否在unmount()时被调用。如果目的是验证卸载清理，建议在 mock 返回的对象上挂载可观察的
disconnect：mockIntersectionObserver.mockImplementation((callback) => { intersectionCallback = callback; return { - observe: jest.fn(), - disconnect: jest.fn(), + observe: jest.fn(), + disconnect: jest.fn(), unobserve: jest.fn(), }; });然后在用例中断言
disconnect被调用一次，才能真正覆盖清理逻辑。
485-553: 多组 disable 分支相关用例存在明显重复且偏向覆盖驱动从
it('should skip initialization when disable is true (dom is null)', …)到it('should verify disable branch coverage for enforceScrollLock', …)这一大段测试，大多是：
- 与前文已有用例功能重复（例如前面已经验证过
dom === null或onlySafari && !Safari时不会初始化 observer）；- 或仅通过“mock 未被调用 / 不抛异常”来声明覆盖了某个“disable 分支”，而不是从外部行为角度验证逻辑；
- 部分描述还直接引用实现文件的“行号”，后续一旦重构实现，描述会很快失效。
建议适当收缩这一组用例，例如：
- 保留一两条典型场景（
dom === null、onlySafari+ 非 Safari）验证 disable 行为；- 去掉那些仅为覆盖率而写、行为重复或依赖具体行号的测试；
- 如果确实需要保证分支覆盖，可以通过参数化测试来减少重复代码。
这样可以让测试套件更聚焦于行为，而不是行号级别的覆盖。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/x/components/bubble/__tests__/__snapshots__/demo-extend.test.ts.snapis excluded by!**/*.snappackages/x/components/bubble/__tests__/__snapshots__/demo.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/x/components/bubble/BubbleList.tsx(3 hunks)packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)packages/x/components/bubble/demo/_test.md(1 hunks)packages/x/components/bubble/demo/_test.tsx(1 hunks)packages/x/components/bubble/hooks/useCompatibleScroll.ts(1 hunks)packages/x/components/bubble/index.zh-CN.md(1 hunks)packages/x/components/bubble/style/list.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: afc163
Repo: ant-design/x PR: 0
File: :0-0
Timestamp: 2025-04-11T14:47:09.527Z
Learning: 当评审 ant-design/x 仓库中的 PR 时，需要用中文回复中文评论。该项目的文档支持中英双语。
Learnt from: afc163
Repo: ant-design/x PR: 0
File: :0-0
Timestamp: 2025-04-11T14:47:09.527Z
Learning: 当评审 ant-design/x 仓库中的 PR 时，需要用中文回复中文评论。该项目的文档支持中英双语。
📚 Learning: 2025-01-27T09:36:11.490Z
Learnt from: YumoImer
Repo: ant-design/x PR: 479
File: components/bubble/demo/debug-list.tsx:39-57
Timestamp: 2025-01-27T09:36:11.490Z
Learning: In the Bubble.List debug demo component, the different typing behavior between pushBubble (sets typing: true) and unshiftBubble (no typing property) operations is intentional to facilitate testing different scenarios.
Applied to files:
packages/x/components/bubble/demo/_test.mdpackages/x/components/bubble/demo/_test.tsxpackages/x/components/bubble/style/list.tspackages/x/components/bubble/__tests__/list-scroll.test.tsxpackages/x/components/bubble/index.zh-CN.md
🧬 Code graph analysis (2)
packages/x/components/bubble/BubbleList.tsx (1)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
useCompatibleScroll(18-114)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (2)
isSafari(3-12)useCompatibleScroll(18-114)packages/x-sdk/tests/utils.tsx (1)
renderHook(41-53)
🪛 LanguageTool
packages/x/components/bubble/demo/_test.md
[style] ~7-~7: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ... of non-essential rendering data items, so as to ensure the high performance rendering o...
(SO_AS_TO)
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...ing data items, so as to ensure the high performance rendering of Bubble.List...
(QB_NEW_EN_HYPHEN)
⏰ 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). (5)
- GitHub Check: build preview
- GitHub Check: size
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/x/components/bubble/BubbleList.tsx (1)
169-173: 滚动逻辑改进看起来合理。在执行标准滚动逻辑之前调用
resetToBottom()来重置状态，这符合 Safari 兼容性修复的目的。依赖数组中正确包含了resetToBottom。packages/x/components/bubble/index.zh-CN.md (1)
22-32: 确认隐藏演示代码是否为预期行为。多个列表相关的演示代码被注释掉了（包括
list.tsx、list-scroll.tsx、semantic-list-custom.tsx、list-extra.tsx）。如果这是为了暂时隐藏调试用的演示或者尚未准备好发布的功能，这是合理的。请确认这些演示是否应该在后续重新启用，或者这只是针对此 PR 的临时调整。
size-limit report 📦
|
Bundle ReportChanges will increase total bundle size by 5.15MB (124.9%) ⬆️
ℹ️ *Bundle size includes cached data from a previous commit Affected Assets, Files, and Routes:view changes for bundle: x-markdown-array-pushAssets Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
16-16: 建议使用useMemo缓存disable计算结果。
disable常量在每次渲染时都会重新计算，其中包含开销较大的getComputedStyle(dom)调用，可能触发样式重新计算。建议使用useMemo缓存这个计算结果，以避免不必要的重复计算，特别是当使用此 Hook 的组件频繁重新渲染时。应用此 diff 来缓存计算结果：
+import { useCallback, useEffect, useMemo, useRef } from 'react'; // ... - const disable = !dom || getComputedStyle(dom).flexDirection !== 'column-reverse'; + const disable = useMemo( + () => !dom || getComputedStyle(dom).flexDirection !== 'column-reverse', + [dom], + );
🧹 Nitpick comments (2)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (2)
66-82: 添加空值检查以避免运行时错误。回调函数在第 67 行使用了
dom!非空断言，但没有运行时检查。如果dom在渲染之间变为null，访问其属性会导致运行时错误。应用此 diff 添加安全检查：
const handleScroll = useCallback(() => { + if (!dom) return; const { scrollTop, scrollHeight } = dom; // 倒序， top 在变化，但 bottom 固定 lockedScrollBottomPos.current = scrollHeight + scrollTop; // 检测并恢复自然触发状态 if (callOnScrollNotNative.current) { callOnScrollNotNative.current = false; return; } if (scrolling.current) { clearTimeout(scrolling.current); } scrolling.current = setTimeout(() => { clearTimeout(scrolling.current); scrolling.current = undefined; }, 50); }, [dom]);
111-116: 代码逻辑正确，但可以更明确地处理类型安全。虽然第 112 行的
disable检查已经隐式保证了dom不为空（因为disable在!dom时为true），但 TypeScript 可能无法识别这种控制流。可以考虑添加显式的空值检查以提高类型安全性和代码可读性。可选的改进方案：
const resetToBottom = useCallback(() => { - if (disable) return; + if (!dom || disable) return; isAtBottom.current = true; shouldLock.current = false; lockedScrollBottomPos.current = dom.scrollHeight; }, [dom, disable]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/x/components/bubble/__tests__/__snapshots__/demo-extend.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)packages/x/components/bubble/demo/_test.tsx(1 hunks)packages/x/components/bubble/hooks/useCompatibleScroll.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/x/components/bubble/tests/list-scroll.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-27T09:36:11.490Z
Learnt from: YumoImer
Repo: ant-design/x PR: 479
File: components/bubble/demo/debug-list.tsx:39-57
Timestamp: 2025-01-27T09:36:11.490Z
Learning: In the Bubble.List debug demo component, the different typing behavior between pushBubble (sets typing: true) and unshiftBubble (no typing property) operations is intentional to facilitate testing different scenarios.
Applied to files:
packages/x/components/bubble/demo/_test.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). (4)
- GitHub Check: size
- GitHub Check: build preview
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
19-64: 哨兵元素和观察器的管理实现良好。哨兵元素的创建、插入和清理逻辑实现正确：
- 哨兵元素在需要时被创建并插入 DOM
- IntersectionObserver 和 MutationObserver 正确设置并观察相应的目标
- 清理函数完整地断开观察器连接并移除哨兵元素（第 59-62 行）
- 依赖数组包含了所有必要的依赖项
这解决了之前关于哨兵元素清理的问题。
packages/x/components/bubble/demo/_test.tsx (4)
70-70: 定时器清理逻辑正确实现。第 70 行的
useEffect清理函数正确地在组件卸载时清理定时器，解决了之前关于定时器未清理的问题。各个按钮的点击处理中也正确地调用了clearInterval并将定时器引用设置为null。
36-61: 自定义 Hook 实现良好。
useBubbleListHook 的实现正确：
- 使用
useCallback正确地记忆化了add和update回调- 两个回调都使用函数式更新
setItems，因此空依赖数组是正确的- Hook 接口清晰，可复用性强
72-88: 角色配置正确使用了记忆化。
memoRole配置正确地使用useMemo进行记忆化，避免每次渲染时创建新的角色配置对象。配置结构合理，包含了 AI 和用户角色的头像、标题和操作项等属性。
90-139: 演示组件实现完整且正确。App 组件提供了完整的测试演示界面：
- 控制面板提供了启动、停止、清空和自动滚动切换功能
- 定时器逻辑正确地在 AI 和用户消息之间交替添加内容
- 使用
Bubble.List的所有相关属性（ref、role、items、autoScroll）- 这个演示很好地测试了新的 Safari 兼容滚动行为
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (1)
409-455: 关于“unmount 后 dom 变为 null/undefined”的注释依然容易产生误导在以下两个 edge case 用例中：
- Line 435 注释：
// Unmount to set dom to null- Line 532 注释：
// Unmount to set dom to undefined实际实现里，
useCompatibleScroll接收的是一个普通的dom参数（非 ref），React 组件卸载并不会把这个闭包变量改成null/undefined。unmount 只会触发 effect cleanup，让IntersectionObserver/MutationObserver停止工作，但闭包里的dom仍然指向原始元素。因此：
it('should not throw error when enforcing scroll lock with null dom', …)实际验证的是“在已 unmount 后手动调用mutationCallback不会抛错”，但并没有真正覆盖dom为null的逻辑分支；it('should not throw error when dom becomes undefined after mount', …)同理，是在卸载后调用resetToBottom不抛错，而不是dom真的变成undefined。建议：
- 如果当前目标仅是“卸载后调用相关回调不抛异常”，可以把注释改成类似“// Unmount to simulate calls after cleanup” 并适度调整用例名称，避免提到“set dom to null/undefined”；
- 如果确实需要覆盖
dom === null/undefined的防御分支，可以考虑让 hook 从一个ref.current读取 DOM，并在 cleanup 时显式置空，然后在测试里通过修改 ref 来驱动该分支。这些点与之前 review 中对“dom 为空分支未真正覆盖”的反馈很接近，建议一并调整，减少后续阅读和维护时的困惑。
Also applies to: 521-541
🧹 Nitpick comments (2)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
190-212: 多处测试用例断言过弱或缺失，容易产生“虚假覆盖”以下几个用例的名称与实际断言不太匹配，基本只是在验证“代码不抛异常”，而没有真正检查行为是否符合预期：
- Line 191–211：
'should update isAtBottom state when sentinel intersects'—— 只调用了intersectionCallback，没有任何断言，看不出isAtBottom/shouldLock是否真的被更新。- Line 298–318：
'should reset internal state to bottom position'—— 实际只验证resetToBottom是函数，这一点前面的“Hook Return Value”用例已经覆盖，并未检查滚动锁定状态/内部 ref 是否被重置。- Line 333–364：
'should calculate correct target scroll position'—— 只是手工算了一遍700 - 1200 = -500，没有通过触发mutationCallback等路径去验证 hook 实际算出来的 target scroll 是否正确。- Line 459–490：
'should handle multiple rapid mutations'、以及 Line 492–518：'should not enforce scroll lock when at bottom'—— 都是多次触发mutationCallback/scroll，但没有检查最终scrollTop、锁定位置或者任何状态变化。- Line 543–563：
'should handle scroll event correctly when not at bottom'—— 只触发了 scroll，没有断言lockedScrollBottomPos或相关效果。从测试可维护性和信心角度来看，建议：
- 要么为这些用例补充针对性断言（例如通过更精细的 mock 捕获
enforceScrollLock内部计算结果、最终scrollTop或内部状态 ref 的变化）；- 要么将用例名称改成更符合当前行为的描述，如“… should not throw when …”，避免给人“关键逻辑已被验证”的错觉。
这样可以显著提升这些复杂滚动场景测试的真实覆盖度。
Also applies to: 298-318, 332-364, 458-490, 492-518, 543-563
566-587: Cleanup 用例目前只验证了“创建过 observer”，并未真正覆盖清理逻辑在
describe('Cleanup', …)的用例中：const { unmount } = renderHook(() => useCompatibleScroll(mockDom)); expect(mockDom.firstChild).toBeTruthy(); unmount(); expect(mockIntersectionObserver).toHaveBeenCalled(); expect(mockMutationObserver).toHaveBeenCalled();这里的断言只保证了在 mount 阶段确实 new 过
IntersectionObserver/MutationObserver，但并没有检查：
disconnect/unobserve是否在 unmount 时被调用；- 底部 sentinel 元素是否从 DOM 中被移除；
- 滚动监听器是否被
removeEventListener正确解绑。也就是说，即使把 effect 的 cleanup 完全删掉，只保留创建逻辑，这个测试也仍然会通过。
建议：
- 在 mock 实现里保存每个 observer 实例的
disconnect/unobserve函数，并在用例里断言它们在unmount()后被调用。- 在 unmount 前后分别检查
mockDom.firstChild（或通过querySelector）以确认 sentinel 元素被移除。- 如有需要，可以复用
addEventListener/removeEventListener的 mock 模式，断言 scroll 监听也在 cleanup 中解绑。这样可以让 “Cleanup” 这个用例名与真正验证的行为更一致，提高对资源释放、避免泄漏的信心。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)packages/x/components/bubble/hooks/useCompatibleScroll.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/x/components/bubble/hooks/useCompatibleScroll.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
packages/x-sdk/tests/utils.tsx (1)
renderHook(41-53)packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
useCompatibleScroll(7-120)
⏰ 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). (5)
- GitHub Check: size
- GitHub Check: build preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
41-88: 整体测试结构和 mock 布局设计得比较清晰整体的
beforeEach/afterEach里集中创建mockDom、挂载到document.body，并统一 mockIntersectionObserver/MutationObserver，结构清晰、可读性和复用性都不错，为后续细分用例打了一个比较扎实的基础。
240-266: 滚动监听 capture 选项的用例覆盖点很好这里通过替换
addEventListener/removeEventListener并断言：expect(mockAddEventListener).toHaveBeenCalledWith( 'scroll', expect.any(Function), { capture: true }, );明确保证了 hook 在 Safari 兼容场景下是用捕获阶段监听
scroll事件的（避免只在冒泡阶段拿不到事件的问题），这一点与 PR 目标高度相关，测试写得比较到位。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (2)
16-16: 避免在每次渲染中重复调用getComputedStyle，建议用useMemo缓存disable当前
disable每次渲染都会调用一次getComputedStyle(dom)，在频繁重渲染场景下可能触发不必要的样式计算。可以用useMemo缓存结果，只在dom变化时重新计算，例如：-import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; - const disable = !dom || getComputedStyle(dom).flexDirection !== 'column-reverse'; + const disable = useMemo( + () => !dom || getComputedStyle(dom).flexDirection !== 'column-reverse', + [dom], + );这对 Safari 兼容逻辑没有行为变化，只是减少 layout 相关的开销。
91-120: 为enforceScrollLock增加 DOM 空值防御，提升未来改动下的健壮性目前实现依赖“只在
dom有值且非disable时才注册MutationObserver”，因此运行时dom理论上不会为null，dom!也不会真的报错。不过一旦未来将dom换成ref.current或在外层出现动态置空（例如卸载前后复用 hook），这里会是第一处潜在 NPE。可以通过在函数开头增加一次空值检查，同时合并对
scrolling.current的判断，去掉!断言，更安全一些：- const enforceScrollLock = useCallback(() => { + const enforceScrollLock = useCallback(() => { + if (!dom || scrolling.current) return; @@ - // requestAnimationFrame(() => { - if (scrolling.current) return; - const targetScroll = lockedScrollBottomPos.current - dom!.scrollHeight; - dom!.scrollTop = targetScroll; + // requestAnimationFrame(() => { + const targetScroll = lockedScrollBottomPos.current - dom.scrollHeight; + dom.scrollTop = targetScroll; @@ - }, [dom]); + }, [dom]);这不会改变当前行为，只是为后续重构留出更大的安全空间。
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
85-88: 在afterEach中增加jest.restoreAllMocks()，避免getComputedStyle等 spy 泄漏到其他用例多个用例中用
jest.spyOn(window, 'getComputedStyle')并调用mockImplementation，但afterEach只做了jest.clearAllMocks()。这只会清空调用记录，不会把被 spy 的属性还原，导致后续测试仍然使用 mock 版本的getComputedStyle，容易产生测试之间的隐式耦合。建议在
afterEach中同时恢复所有 spy：- afterEach(() => { - document.body.innerHTML = ''; - jest.clearAllMocks(); - }); + afterEach(() => { + document.body.innerHTML = ''; + jest.restoreAllMocks(); + jest.clearAllMocks(); + });这样每个用例都会从干净的浏览器 API 状态开始。
268-295: 避免直接覆盖global.clearTimeout，改用jest.spyOn以防全局状态污染当前测试通过赋值方式替换全局
clearTimeout：const mockClearTimeout = jest.fn(); global.clearTimeout = mockClearTimeout;问题在于：
- 这不是 Jest 的 spy，对应的原实现不会在
jest.clearAllMocks()中自动恢复；- 钩子在 effect cleanup 里依赖真实的
clearTimeout进行资源回收，被替换后可能影响其他测试的行为。更推荐用
jest.spyOn并在用例结束后恢复：- // Set up a timeout to simulate existing scrolling.current - const mockClearTimeout = jest.fn(); - global.clearTimeout = mockClearTimeout; + const clearTimeoutSpy = jest.spyOn(global, 'clearTimeout'); @@ - expect(mockClearTimeout).toHaveBeenCalled(); + expect(clearTimeoutSpy).toHaveBeenCalled(); + clearTimeoutSpy.mockRestore();如果配合上方
afterEach中的jest.restoreAllMocks()，可以省略显式的mockRestore()。
🧹 Nitpick comments (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (1)
50-83: 全局替换IntersectionObserver/MutationObserver建议在测试结束时显式恢复
beforeEach中通过赋值方式把global.IntersectionObserver、global.MutationObserver替换为 jest.fn 实现，这对本文件内部测试是方便的，但同样会在文件级别之外保留 mock 版本，潜在影响其他依赖这些原生 API 的测试。可以考虑：
- 在
beforeAll保存原实现，在afterAll里还原；或- 在
afterEach中显式恢复为保存的原实现。示例：
let OriginalIntersectionObserver = global.IntersectionObserver; let OriginalMutationObserver = global.MutationObserver; beforeAll(() => { OriginalIntersectionObserver = global.IntersectionObserver; OriginalMutationObserver = global.MutationObserver; }); afterAll(() => { global.IntersectionObserver = OriginalIntersectionObserver; global.MutationObserver = OriginalMutationObserver; });这样既保留当前文件的可测试性，又降低整个测试套件的全局副作用。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)packages/x/components/bubble/hooks/useCompatibleScroll.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
packages/x-sdk/tests/utils.tsx (1)
renderHook(41-53)packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
useCompatibleScroll(7-121)
⏰ 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). (5)
- GitHub Check: test
- GitHub Check: build preview
- GitHub Check: size
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
19-64: 哨兵插入与观察者清理逻辑整体合理
- 只在
disable === false时创建 sentinel 和两个 Observer，避免在非倒序/无 DOM 时做多余工作。- cleanup 中同时
disconnect()、clearTimeout(scrolling.current)，并从父节点移除 sentinel，避免 DOM 污染和计时器泄漏。- effect 依赖
[dom, disable]，在容器/布局变化时会重新挂载观察逻辑，和滚动锁定语义一致。这块实现从鲁棒性和资源清理角度看都比较到位。
packages/x/components/bubble/__tests__/list-scroll.test.tsx (1)
190-583: 测试覆盖面很完整，基本行为与边界情况都已覆盖整体来看，这个测试文件已经很好地覆盖了新 hook 的关键路径：
- 初始化/禁用条件（
dom为null、非column-reverse）；- sentinel 创建及样式；
- IntersectionObserver / MutationObserver 配置与回调行为；
- 滚动事件监听（包括 capture 选项、timeout 清理）、滚动锁定与
resetToBottom；- 多次 mutation、滚动中/在底部等边界行为；
- 卸载后的清理和不会抛错的场景。
在解决上面几个 mock 污染问题后，这套测试对于防止未来改动引入 Safari 回归会非常有帮助。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (1)
333-364: 目标滚动位置计算测试可增强断言当前测试仅验证了
expectedTargetScroll的计算结果（-500），但未直接验证 hook 是否真正将scrollTop设置为该值。虽然测试计算逻辑本身是有效的，但如果能够在触发mutationCallback后验证mockDom.scrollTop的实际值，测试会更加健壮。可以考虑在计算后触发 mutation 并验证实际的 scrollTop 值：
// Change scrollHeight to simulate content being added Object.defineProperty(mockDom, 'scrollHeight', { value: 1200, writable: true }); +// Trigger mutation to enforce scroll lock +act(() => { + intersectionCallback([{ isIntersecting: false }]); // Ensure shouldLock is true + mutationCallback(); +}); + +// Verify scrollTop was set to the calculated target +expect(mockDom.scrollTop).toBe(-500); + // Check that the target scroll position would be calculated correctly // targetScroll = lockedScrollBottomPos.current - dom!.scrollHeight // targetScroll = 700 - 1200 = -500 // We can't directly access lockedScrollBottomPos, but we can verify the calculation // by checking the expected result const expectedTargetScroll = 700 - 1200; expect(expectedTargetScroll).toBe(-500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (2)
packages/x-sdk/tests/utils.tsx (1)
renderHook(41-53)packages/x/components/bubble/hooks/useCompatibleScroll.ts (1)
useCompatibleScroll(7-121)
⏰ 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). (5)
- GitHub Check: build preview
- GitHub Check: test
- GitHub Check: size
- GitHub Check: Cloudflare Pages
- GitHub Check: test
🔇 Additional comments (6)
packages/x/components/bubble/__tests__/list-scroll.test.tsx (6)
85-89: 测试清理逻辑完善
afterEach钩子中正确调用了jest.restoreAllMocks()和jest.clearAllMocks()，确保了测试之间的隔离性，避免了 mock 状态泄漏。这很好地解决了之前评审中指出的问题。
98-165: 初始化场景测试覆盖全面测试套件充分覆盖了各种初始化场景：
- dom 为 null 时不初始化
- flexDirection 非 column-reverse 时不初始化
- 正常场景下正确初始化 observers
- MutationObserver 配置选项验证
测试使用
jest.spyOn方式 mockgetComputedStyle，配合afterEach中的restoreAllMocks()确保了测试间的隔离。
269-294: 快速滚动时 timeout 清理测试改进测试现在正确使用
jest.spyOn(global, 'clearTimeout')代替直接赋值，避免了全局状态污染。spy 会在afterEach中通过jest.restoreAllMocks()自动恢复，确保不会影响后续测试。
297-330: Reset to Bottom 功能测试充分测试覆盖了
resetToBottom()API 的正常场景和边界情况（hook 被禁用时），验证了该方法不会在禁用状态下抛出异常，体现了良好的防御性设计。
454-514: Mutation 处理测试覆盖完整测试充分验证了 MutationObserver 的行为：
- 快速多次 DOM 变更场景
- 在底部时不强制锁定滚动位置
循环模拟 5 次快速 mutation 的压力测试很好地验证了 hook 在高频变更下的稳定性。
516-584: 边界情况和清理逻辑测试完备测试套件的收尾部分充分验证了：
- 组件卸载后调用 API 不抛异常
- 非底部滚动事件的正确处理
- observers 和哨兵元素的正确清理
整体测试文件结构清晰、覆盖全面，充分验证了
useCompatibleScrollhook 在各种场景下的行为，包括正常流程、边界情况和异常处理。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1392 +/- ##
==========================================
+ Coverage 94.21% 94.25% +0.04%
==========================================
Files 144 145 +1
Lines 3701 3763 +62
Branches 1036 1043 +7
==========================================
+ Hits 3487 3547 +60
- Misses 212 214 +2
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (2)
16-17: disable 每次渲染都会触发一次 getComputedStyle，建议做轻量缓存当前实现功能上是正确的，但在频繁重渲染的场景下，
getComputedStyle(dom)可能成为不必要的性能开销；可以用useMemo包一层，只在dom（以及后续若增加的开关，例如 onlySafari）变化时重新计算 disable。
91-109: enforceScrollLock 建议增加 dom 判空以提升防御性从当前调用链看（只有在
disable === false且 dom 已存在时创建 Observer 并触发回调），dom!理论上不会为 null，但这里完全依赖调用方不误用 Hook；在函数顶部加一行if (!dom) return;并去掉后续的非空断言，可以在保持现有行为不变的前提下，让未来扩展或复用时更安全可预期。
🧹 Nitpick comments (2)
packages/x/components/bubble/hooks/useCompatibleScroll.ts (2)
8-15: 部分内部 ref（例如 isAtBottom）当前未被读取，容易造成困惑现在只在 IntersectionObserver 回调里更新
isAtBottom.current，但从未被读出；如果短期没有对外暴露或内部使用的计划，可以考虑删除这个 ref，或在返回值中一起暴露状态，减少“看起来要用但其实没用”的干扰。
66-82: 滚动处理逻辑清晰，可以考虑抽出 debounce 时间为常量
handleScroll通过lockedScrollBottomPos记录“视窗底部”的绝对位置，并用callOnScrollNotNative区分程序触发与自然滚动，依赖数组也更新为[dom]避免陈旧闭包；可以把50ms 提取成常量（例如SCROLL_IDLE_DELAY），方便后续调参和在单测中缩短等待时间。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/x/components/bubble/BubbleList.tsx(3 hunks)packages/x/components/bubble/__tests__/list-scroll.test.tsx(1 hunks)packages/x/components/bubble/hooks/useCompatibleScroll.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/x/components/bubble/tests/list-scroll.test.tsx
🔇 Additional comments (5)
packages/x/components/bubble/BubbleList.tsx (2)
10-10: 引入 useCompatibleScroll 作为独立 Hook 的方向是合理的BubbleList 只负责选择性地把 scrollBox DOM 传给 Hook，本身不再关心 Safari 兼容细节，职责边界更清晰。
142-173: 在滚动副作用中调用 reset 与滚动指令的顺序是合理的在
if (!scrollBoxRef.current) return;的保护下，再调用reset()与scrollTo，既避免了首帧 ref 为空的异常，也能在最后一条消息变化或autoScroll切换时先重置锁定基准，再执行滚动，行为符合预期；依赖数组里加入reset也避免了闭包旧函数的问题。packages/x/components/bubble/hooks/useCompatibleScroll.ts (3)
18-64: 哨兵元素与 Observer 的初始化和清理现在比较完备在
disable为真时直接返回，启用时只在sentinelRef.current为空时插入一个哨兵节点，并在清理阶段断开 Intersection/MutationObserver、清除scrolling定时器并从 DOM 中移除 sentinel，这样多次挂载或切换容器时不会遗留脏节点，整体资源管理是可靠的。
84-89: 滚动事件监听的绑定与清理路径是合理的仅在
disable === false时才绑定捕获阶段的scroll监听，并在清理时通过dom?.removeEventListener解注册，既避免了 dom 为空时的异常，也不会在禁用场景下多余地监听滚动事件。
111-120: reset 的语义与 BubbleList 的使用方式契合先在
reset中检查disable再读取dom.scrollHeight，避免 dom 为空时访问属性的问题，同时把lockedScrollBottomPos重置为当前内容高度，使得在外部（如 BubbleList）追加消息前先调用reset()时，后续的锁底逻辑有一个正确的基准点。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/x/components/bubble/index.zh-CN.md (1)
155-162: 建议补充 Bubble.List API 文档根据 PR 摘要，新增了
useCompatibleScroll钩子并在BubbleList中集成，包括reset()方法。建议在 Bubble.List 的 API 部分补充相关说明，例如：
- 说明新的 Safari 兼容性改进
- 文档化
reset()方法的用途和用法（如果这是公开 API）- 更新
autoScroll行为的说明这将帮助用户更好地理解和使用新功能。
总体评价
这份文档文件的改动主要是表现层调整，包括：
- ✅ 添加了
<!-- prettier-ignore -->注释以控制代码风格- ✅ 重新组织了演示代码的格式
⚠️ 新增的测试演示被注释掉（请确认这是预期行为）建议：
- 如果测试演示已准备好，请取消注释以发布到文档
- 考虑在 Bubble.List API 部分补充 Safari 兼容性改进的相关说明（如新增的
reset()方法的用途）Based on learnings, 这份回复使用中文以符合 ant-design/x 仓库的评审习惯。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/x/components/bubble/index.zh-CN.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: afc163
Repo: ant-design/x PR: 0
File: :0-0
Timestamp: 2025-04-11T14:47:09.527Z
Learning: 当评审 ant-design/x 仓库中的 PR 时，需要用中文回复中文评论。该项目的文档支持中英双语。
Learnt from: afc163
Repo: ant-design/x PR: 0
File: :0-0
Timestamp: 2025-04-11T14:47:09.527Z
Learning: 当评审 ant-design/x 仓库中的 PR 时，需要用中文回复中文评论。该项目的文档支持中英双语。
📚 Learning: 2025-01-27T09:36:11.490Z
Learnt from: YumoImer
Repo: ant-design/x PR: 479
File: components/bubble/demo/debug-list.tsx:39-57
Timestamp: 2025-01-27T09:36:11.490Z
Learning: In the Bubble.List debug demo component, the different typing behavior between pushBubble (sets typing: true) and unshiftBubble (no typing property) operations is intentional to facilitate testing different scenarios.
Applied to files:
packages/x/components/bubble/index.zh-CN.md
⏰ 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). (5)
- GitHub Check: size
- GitHub Check: build preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/x/components/bubble/index.zh-CN.md (1)
21-35: 注意：测试演示已被注释掉第 21 行的测试演示被注释掉了，这表示该演示还未准备好发布到正式文档中。请确认这是预期行为。如果测试演示已经准备好，应取消注释以供用户查看。
格式化改动（添加
<!-- prettier-ignore -->和重新整理 demo 条目）本身没有问题，有助于保持代码风格一致。
中文版模板 / Chinese template
🤔 This is a ...
🔗 Related Issues
💡 Background and Solution
📝 Change Log
Summary by CodeRabbit
发布说明
新功能
测试
文档
✏️ Tip: You can customize this high-level summary in your review settings.