perf(bubble-list): better way to enable default value #346
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe component BubbleList changed how initStyle() is invoked: removed its onMounted call and moved initialization to a watcher on props (maxHeight, btnIconSize) with immediate: true, causing initStyle() to run at creation and on subsequent prop changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Parent
participant BL as BubbleList
participant W as Watcher
participant IS as initStyle()
rect rgba(200,230,255,0.25)
note over BL: New flow
App->>BL: Create component
BL->>W: Setup watch([maxHeight, btnIconSize], immediate: true)
W->>IS: Invoke immediately on creation
IS-->>BL: Styles initialized
App->>BL: Update maxHeight/btnIconSize
BL->>W: Watcher detects change
W->>IS: Re-invoke
IS-->>BL: Styles updated
end
sequenceDiagram
autonumber
participant App as Parent
participant BL as BubbleList
participant OM as onMounted
participant IS as initStyle()
rect rgba(255,235,200,0.25)
note over BL: Previous flow (for comparison)
App->>BL: Create component
BL->>OM: onMounted hook
OM->>IS: Invoke on mount
IS-->>BL: Styles initialized
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
![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
🧹 Nitpick comments (1)
packages/core/src/components/BubbleList/index.vue (1)
130-160: Guard when fewer than 2 bubbles to avoid runtime error
secondLastItemcan beundefinedwhenlistBubbles.length < 2, leading to a TypeError ongetBoundingClientRect().- const secondLastItem = listBubbles[listBubbles.length - 2]; - const { top, bottom } = secondLastItem.getBoundingClientRect(); - const { top: containerTop, bottom: containerBottom } = - scrollContainer.value!.getBoundingClientRect(); - // 之前的最后一个是现在的第二个，判断是否可见，如果可见则自动滚动 - const isVisible = top < containerBottom && bottom > containerTop; - if (isVisible) { - scrollToBottom(); - stopAutoScrollToBottom.value = false; - } + if (listBubbles.length >= 2) { + const secondLastItem = listBubbles[listBubbles.length - 2] as HTMLElement; + const { top, bottom } = secondLastItem.getBoundingClientRect(); + const { top: containerTop, bottom: containerBottom } = + scrollContainer.value!.getBoundingClientRect(); + // 之前的最后一个是现在的第二个，判断是否可见，如果可见则自动滚动 + const isVisible = top < containerBottom && bottom > containerTop; + if (isVisible) { + scrollToBottom(); + stopAutoScrollToBottom.value = false; + } + } else { + // 首条或仅一条气泡时，直接保持底部对齐 + scrollToBottom(); + stopAutoScrollToBottom.value = false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/components/BubbleList/index.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: imddc
PR: element-plus-x/Element-Plus-X#343
File: packages/core/src/components/BubbleList/index.vue:27-35
Timestamp: 2025-09-14T00:44:10.411Z
Learning: User imddc prefers minimal code modifications for bug fixes rather than comprehensive refactors, focusing on solving the immediate issue with the least amount of change to existing code.
Comment on lines
44
to
52
| watch( | ||
| () => [props.maxHeight, props.btnIconSize], | ||
| () => { | ||
| initStyle(); | ||
| }, | ||
| { | ||
| immediate: true | ||
| } | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SSR-safety: immediate watcher invokes DOM code during SSR; also risks global CSS var clobbering
immediate: true runs the watcher callback at setup time. In SSR that executes on the server, so initStyle() will access document and crash. Additionally, writing CSS vars on document.documentElement creates cross-instance interference.
- Minimal fix: guard DOM access inside
initStyle().
function initStyle() {
if (typeof window === 'undefined' || typeof document === 'undefined') return;
const root = document.documentElement;
root.style.setProperty('--el-bubble-list-max-height', props.maxHeight || '100%');
root.style.setProperty('--el-bubble-list-btn-size', `${props.btnIconSize}px`);
}- Recommended (scoped) approach: bind CSS vars on the component root instead of mutating
documentElementto avoid conflicts and be SSR-friendly:
const cssVars = computed(() => ({
'--el-bubble-list-max-height': props.maxHeight || '100%',
'--el-bubble-list-btn-size': `${props.btnIconSize}px`,
}));Template (on the wrapper div):
<div class="el-bubble-list-wrapper" :style="cssVars">Please confirm whether SSR is a requirement for this library; if yes, we should at least add the DOM guard now, and consider the scoped CSS vars approach next.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root: $(pwd)"
echo "--- file exists check ---"
if [ -f packages/core/src/components/BubbleList/index.vue ]; then
echo "FOUND: packages/core/src/components/BubbleList/index.vue"
else
echo "MISSING: packages/core/src/components/BubbleList/index.vue"
fi
echo "--- show file (first 300 lines) ---"
sed -n '1,300p' packages/core/src/components/BubbleList/index.vue || true
echo "--- search for relevant symbols in file ---"
rg -n --hidden --no-ignore-vcs --color never "initStyle|documentElement|document\.|setProperty|immediate:\s*true|watch\(" packages/core/src/components/BubbleList/index.vue || true
echo "--- grep for initStyle definition elsewhere ---"
rg -n --hidden --no-ignore-vcs --color never "function\s+initStyle|const\s+initStyle\s*=" || trueLength of output: 8723
Fix SSR-safety: guard DOM access and avoid mutating documentElement
initStyle() writes CSS vars to document.documentElement and the watcher uses immediate: true — this runs during SSR and will throw.
- Minimal fix: guard DOM access in initStyle():
function initStyle() {
if (typeof window === 'undefined' || typeof document === 'undefined') return;
const root = document.documentElement;
root.style.setProperty('--el-bubble-list-max-height', props.maxHeight || '100%');
root.style.setProperty('--el-bubble-list-btn-size', `${props.btnIconSize}px`);
}- Recommended (scoped) refactor: compute CSS vars and bind to the component root instead of mutating documentElement:
const cssVars = computed(() => ({
'--el-bubble-list-max-height': props.maxHeight || '100%',
'--el-bubble-list-btn-size': `${props.btnIconSize}px`,
}));template:
<div class="el-bubble-list-wrapper" :style="cssVars">Location: packages/core/src/components/BubbleList/index.vue — initStyle() + watch(...) (lines ~33–52).
🤖 Prompt for AI Agents
In packages/core/src/components/BubbleList/index.vue around lines 33 to 52,
initStyle() mutates document.documentElement and the watcher is immediate,
causing SSR crashes; either guard DOM access at the top of initStyle() and
return early when window or document is undefined, and then keep setting CSS
vars on document.documentElement, or better compute a scoped cssVars reactive
object from props and bind it to the component root element's style (replace
writing to document.documentElement) and remove DOM mutation in initStyle();
ensure the watcher no longer performs direct DOM writes during SSR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
优化了代码逻辑，初始化时不需要在
onMounted钩子中调用#343
在这个pr中，我做了同样的修改，但由于未同步上游，导致结构混乱，故重新提交
Summary by CodeRabbit