refactor(BubbleList): 重构布局与自动滚动逻辑 #285
Conversation
WalkthroughThe BubbleList component's rendering and scrolling logic was inverted to display items in reverse order, with corresponding updates to scroll calculations and event handling. The auto-scroll mechanism now uses a MutationObserver, and the back-to-bottom button was repositioned and made customizable. Styles were updated to use column-reverse flex direction. A story example was enhanced with a timed message appending effect. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BubbleList
participant ScrollContainer
participant MutationObserver
User->>BubbleList: Add new message
BubbleList->>BubbleList: Update wrapList (reversed)
BubbleList->>ScrollContainer: Scroll to bottom (set scrollTop = 0)
BubbleList->>MutationObserver: Observe DOM changes
MutationObserver-->>BubbleList: DOM mutation detected
BubbleList->>ScrollContainer: Auto-scroll to bottom if not stopped
User->>ScrollContainer: Scroll event
ScrollContainer->>BubbleList: handleScroll()
BubbleList->>BubbleList: Update auto-scroll state based on position
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/components/BubbleList/index.vue (3)
68-68: Fix ESLint formatting issues.Multiple formatting issues detected by ESLint should be addressed for code consistency.
function setContainerScrollTop(num: number) { const container = scrollContainer.value; - if (!container) return; + if (!container) + return; container.scrollTop = num; } function scrollToBottom() { const container = scrollContainer.value; - if (!container) return; + if (!container) + return; setContainerScrollTop(0); autoScroll(); } function scrollToBubble(index: number) { const container = scrollContainer.value; - if (!container) return; + if (!container) + return; const bubbles = container.querySelectorAll('.el-bubble'); - if (index >= bubbles.length) return; + if (index >= bubbles.length) + return; function autoScroll() { const container = scrollContainer.value; - if (!container) return; - if (mutationObserver) mutationObserver.disconnect(); + if (!container) + return; + if (mutationObserver) + mutationObserver.disconnect(); function handleScroll() { const container = scrollContainer.value; - if (!container) return; + if (!container) + return;Also applies to: 87-87, 94-94, 98-98, 118-118, 119-119, 169-169
128-128: Fix brace style issues.ESLint detected incorrect brace placement that should be fixed.
setContainerScrollTop(0); - } else { + } + else { mutationObserver.disconnect(); } // 重置累积距离 accumulatedScrollUpDistance.value = 0; - } - } else { + } + } + else { // 如果用户停止向上滚动或开始向下滚动，重置累积距离 accumulatedScrollUpDistance.value = 0; }Also applies to: 204-204
176-181: Consider simplifying the scroll proximity calculation.The magic number
30for bottom proximity detection could be made configurable or better documented.+ // Threshold for detecting when user is "close" to bottom (in pixels) + const SCROLL_BOTTOM_THRESHOLD = 30; + // 判断是否距离底部小于阈值 (这里吸附值大一些会体验更好) - const isCloseToBottom = scrollTop + clientHeight >= scrollHeight - 30; + const isCloseToBottom = scrollTop + clientHeight >= scrollHeight - SCROLL_BOTTOM_THRESHOLD;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/components/BubbleList/index.vue(6 hunks)packages/core/src/components/BubbleList/style.scss(1 hunks)packages/core/src/stories/BubbleList/CustomSolt.vue(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/src/components/BubbleList/style.scss (1)
packages/core/src/components/BubbleList/types.d.ts (2)
BubbleListInstance(31-35)BubbleListProps(12-25)
packages/core/src/components/BubbleList/index.vue (1)
packages/core/src/components/BubbleList/types.d.ts (5)
BubbleListInstance(31-35)BubbleListProps(12-25)BubbleListEmits(27-29)BubbleListItemProps(5-5)BackButtonPosition(7-10)
🪛 ESLint
packages/core/src/components/BubbleList/index.vue
[error] 68-68: Expect newline after if
(antfu/if-newline)
[error] 87-87: Expect newline after if
(antfu/if-newline)
[error] 94-94: Expect newline after if
(antfu/if-newline)
[error] 98-98: Expect newline after if
(antfu/if-newline)
[error] 118-118: Expect newline after if
(antfu/if-newline)
[error] 119-119: Expect newline after if
(antfu/if-newline)
[error] 128-128: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 169-169: Expect newline after if
(antfu/if-newline)
[error] 204-204: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
🔇 Additional comments (4)
packages/core/src/components/BubbleList/style.scss (1)
3-3: LGTM! The flex-direction change aligns with the refactored scroll logic.Changing to
column-reverseis a clever approach that simplifies the scroll-to-bottom implementation, asscrollTop = 0now naturally scrolls to the bottom of the list.packages/core/src/components/BubbleList/index.vue (3)
27-28: LGTM! Clever use of list reversal for simplified scroll logic.The combination of reversing the list in JavaScript and using
column-reversein CSS maintains the visual order while making scroll-to-bottom operations more intuitive (scrollTop = 0).
65-70: Good practice: Centralized scroll position setter.Centralizing the scroll position logic in
setContainerScrollTopensures consistency across all scroll operations.
235-260: Great improvement: Back-to-bottom button integration.Moving the button inside the scroll container and adding slot support provides better flexibility and maintains proper positioning with the sticky CSS.
| const bubbles = container.querySelectorAll('.el-bubble'); | ||
| if (index >= bubbles.length) return; |
There was a problem hiding this comment.
Fix the bounds check to use the correct list length.
The bounds check should compare against wrapList.value.length instead of bubbles.length for consistency.
- if (index >= bubbles.length) return;
+ if (index >= wrapList.value.length) return;📝 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.
| if (index >= bubbles.length) return; | |
| if (index >= wrapList.value.length) return; |
🧰 Tools
🪛 ESLint
[error] 98-98: Expect newline after if
(antfu/if-newline)
🤖 Prompt for AI Agents
In packages/core/src/components/BubbleList/index.vue at line 98, the bounds
check incorrectly compares the index against bubbles.length instead of
wrapList.value.length. Update the condition to use wrapList.value.length to
ensure consistency with the list being referenced.
| let mutationObserver: MutationObserver; | ||
| // 组件内部触发方法，跟随打字器滚动，滚动底部 | ||
| function autoScroll() { | ||
| if (scrollContainer.value) { | ||
| const listBubbles = scrollContainer.value!.querySelectorAll( | ||
| '.el-bubble-content-wrapper' | ||
| ); | ||
| // 如果页面上有监听节点，先移除 | ||
| if (resizeObserver.value) { | ||
| resizeObserver.value.disconnect(); | ||
| const container = scrollContainer.value; | ||
| if (!container) return; | ||
| if (mutationObserver) mutationObserver.disconnect(); | ||
| mutationObserver = new MutationObserver(() => { | ||
| if (stopAutoScrollToBottom.value) { | ||
| mutationObserver.disconnect(); | ||
| return; | ||
| } | ||
| const lastItem = listBubbles[listBubbles.length - 1]; | ||
| if (lastItem) { | ||
| resizeObserver.value = new ResizeObserver(() => { | ||
| if (!stopAutoScrollToBottom.value) { | ||
| scrollToBottom(); | ||
| } | ||
| }); | ||
| resizeObserver.value.observe(lastItem); | ||
| const scrollTop = Math.abs(container.scrollTop); | ||
| if (scrollTop > 0) { | ||
| setContainerScrollTop(0); | ||
| } else { | ||
| mutationObserver.disconnect(); | ||
| } | ||
| } | ||
| }); | ||
| mutationObserver.observe(container, { | ||
| childList: true, | ||
| subtree: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ensure MutationObserver is properly cleaned up.
The MutationObserver should be disconnected when the component unmounts to prevent memory leaks.
Add cleanup in the component lifecycle:
+onUnmounted(() => {
+ if (mutationObserver) {
+ mutationObserver.disconnect();
+ }
+});📝 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.
| let mutationObserver: MutationObserver; | |
| // 组件内部触发方法，跟随打字器滚动，滚动底部 | |
| function autoScroll() { | |
| if (scrollContainer.value) { | |
| const listBubbles = scrollContainer.value!.querySelectorAll( | |
| '.el-bubble-content-wrapper' | |
| ); | |
| // 如果页面上有监听节点，先移除 | |
| if (resizeObserver.value) { | |
| resizeObserver.value.disconnect(); | |
| const container = scrollContainer.value; | |
| if (!container) return; | |
| if (mutationObserver) mutationObserver.disconnect(); | |
| mutationObserver = new MutationObserver(() => { | |
| if (stopAutoScrollToBottom.value) { | |
| mutationObserver.disconnect(); | |
| return; | |
| } | |
| const lastItem = listBubbles[listBubbles.length - 1]; | |
| if (lastItem) { | |
| resizeObserver.value = new ResizeObserver(() => { | |
| if (!stopAutoScrollToBottom.value) { | |
| scrollToBottom(); | |
| } | |
| }); | |
| resizeObserver.value.observe(lastItem); | |
| const scrollTop = Math.abs(container.scrollTop); | |
| if (scrollTop > 0) { | |
| setContainerScrollTop(0); | |
| } else { | |
| mutationObserver.disconnect(); | |
| } | |
| } | |
| }); | |
| mutationObserver.observe(container, { | |
| childList: true, | |
| subtree: true | |
| }); | |
| } | |
| let mutationObserver: MutationObserver; | |
| // 组件内部触发方法，跟随打字器滚动，滚动底部 | |
| function autoScroll() { | |
| const container = scrollContainer.value; | |
| if (!container) return; | |
| if (mutationObserver) mutationObserver.disconnect(); | |
| mutationObserver = new MutationObserver(() => { | |
| if (stopAutoScrollToBottom.value) { | |
| mutationObserver.disconnect(); | |
| return; | |
| } | |
| const scrollTop = Math.abs(container.scrollTop); | |
| if (scrollTop > 0) { | |
| setContainerScrollTop(0); | |
| } else { | |
| mutationObserver.disconnect(); | |
| } | |
| }); | |
| mutationObserver.observe(container, { | |
| childList: true, | |
| subtree: true | |
| }); | |
| } | |
| onUnmounted(() => { | |
| if (mutationObserver) { | |
| mutationObserver.disconnect(); | |
| } | |
| }); |
🧰 Tools
🪛 ESLint
[error] 118-118: Expect newline after if
(antfu/if-newline)
[error] 119-119: Expect newline after if
(antfu/if-newline)
[error] 128-128: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
🤖 Prompt for AI Agents
In packages/core/src/components/BubbleList/index.vue around lines 113 to 136,
the MutationObserver is created but not disconnected when the component
unmounts, risking memory leaks. To fix this, add a cleanup step in the
component's unmounted lifecycle hook that calls mutationObserver.disconnect() if
the observer exists, ensuring proper resource release.
| let num = 50; | ||
| const T = setInterval(() => { | ||
| if (num < 1) { | ||
| clearInterval(T); | ||
| } | ||
| bubbleItems.value[bubbleItems.value.length - 1].content += | ||
| '欢迎使用 Element Plus X .'; | ||
| num--; | ||
| }, 100); |
There was a problem hiding this comment.
Potential memory leak: Clear interval on component unmount.
The interval continues running even if the component unmounts, which could cause memory leaks and errors when trying to update unmounted components.
Store the interval ID at the component level and clear it on unmount:
+const messageIntervals = new Set<NodeJS.Timeout>();
function addMessage() {
// ... existing code ...
- let num = 50;
+ let count = 50;
const T = setInterval(() => {
- if (num < 1) {
+ if (count < 1) {
clearInterval(T);
+ messageIntervals.delete(T);
}
bubbleItems.value[bubbleItems.value.length - 1].content +=
'欢迎使用 Element Plus X .';
- num--;
+ count--;
}, 100);
+ messageIntervals.add(T);
}
+
+onUnmounted(() => {
+ messageIntervals.forEach(interval => clearInterval(interval));
+});Also renamed the local variable from num to count to avoid shadowing the component-level num ref.
🤖 Prompt for AI Agents
In packages/core/src/stories/BubbleList/CustomSolt.vue around lines 46 to 54,
the setInterval is not cleared when the component unmounts, causing a potential
memory leak. To fix this, store the interval ID in a component-level variable
and use the component's unmount lifecycle hook to clear the interval. Also,
rename the local variable from num to count to avoid shadowing any
component-level num ref.
1 participant
Summary by CodeRabbit
New Features
Improvements
Style
Demo Updates