feat(ScrollArea): Add leading/trailing slots#6201
feat(ScrollArea): Add leading/trailing slots#6201mikenewbon wants to merge 2 commits intonuxt:v4from
Conversation
mikenewbon
force-pushed
the
feat/scroll-area-slots
branch
from
bd366bd to
91eb932
Compare
📝 WalkthroughWalkthroughAdded Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/components/ScrollArea.spec.ts (1)
29-29: Keep explicit per-item default-slot coverage in the matrix.Line 29 currently validates default-slot rendering without
items. Consider adding a separate case withpropsso per-item slot props (item,index) remain directly covered.♻️ Suggested test matrix tweak
- ['with default slot', { slots: { default: () => 'Default slot' } }], + ['with default slot and items', { props, slots: { default: ({ item }) => item.name } }], + ['with default slot (no items)', { slots: { default: () => 'Default slot' } }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/ScrollArea.spec.ts` at line 29, Add an explicit matrix case that covers per-item default-slot props by duplicating the existing "with default slot" entry but adding props: { items: [...] } and replacing the slot factory to accept and assert item/index (e.g., slots: { default: (item, index) => /* use item/index in output or assertions */ }). Locate the current matrix entry "with default slot" (slots: { default: () => 'Default slot' }) in ScrollArea.spec.ts and add a new case like "with default slot and items" that passes a small items array so the test verifies per-item slot props are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/components/ScrollArea.spec.ts`:
- Line 29: Add an explicit matrix case that covers per-item default-slot props
by duplicating the existing "with default slot" entry but adding props: { items:
[...] } and replacing the slot factory to accept and assert item/index (e.g.,
slots: { default: (item, index) => /* use item/index in output or assertions */
}). Locate the current matrix entry "with default slot" (slots: { default: () =>
'Default slot' }) in ScrollArea.spec.ts and add a new case like "with default
slot and items" that passes a small items array so the test verifies per-item
slot props are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e151a0f-1a74-4c2c-9244-4a732bdb09fc
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/ScrollArea-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/ScrollArea.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
docs/app/components/content/examples/scroll-area/ScrollAreaInfiniteScrollExample.vuedocs/app/components/content/examples/scroll-area/ScrollAreaLeadingTrailingExample.vuedocs/content/docs/2.components/scroll-area.mdplaygrounds/nuxt/app/pages/components/scroll-area.vuesrc/runtime/components/ScrollArea.vuetest/components/ScrollArea.spec.ts
commit: |
howwohmm
left a comment
howwohmm
left a comment
There was a problem hiding this comment.
the leading/trailing slots are a useful addition. one concern with virtualized mode:
the leading slot renders as a sibling of the viewport div (confirmed in the snapshot: Leading content<div data-slot="viewport">). the virtualizer's getScrollElement returns the root Primitive (rootRef.value?.$el), so scrollTop includes the leading slot's height. but scrollMargin defaults to 0 and the PR doesn't adjust it.
this means the virtualizer's scroll offset is off by the leading slot's height — it thinks the user has scrolled further than they actually have relative to the virtualized content. visual item positioning is fine (absolute within the viewport div), but:
- the visible range calculation will be wrong during scroll (off by a few items)
scrollToIndexwill scroll to the wrong position
the fix would be auto-measuring the leading slot height and passing it as scrollMargin in the virtualizer options, or documenting that users must set it manually.
also: the PR checklist has "linked issue" unchecked — might want to add that.
mikenewbon
force-pushed
the
feat/scroll-area-slots
branch
from
91eb932 to
3070f9b
Compare
… rendering - Introduced `#leading` and `#trailing` slots to the ScrollArea component, allowing for customizable content before and after the scrollable items. - Updated examples and documentation to demonstrate the new slot functionality. - Enhanced loading state handling in the infinite scroll example.
mikenewbon
force-pushed
the
feat/scroll-area-slots
branch
from
3070f9b to
457a974
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/runtime/components/Sidebar.vue (1)
385-385: Avoid usinganytype for the ref callback parameter.The explicit
anycast bypasses type checking. Consider using the proper element type.Proposed fix
-:ref="isResizable ? (el: any) => { containerEl = el } : undefined" +:ref="isResizable ? (el: Element | ComponentPublicInstance | null) => { containerEl = el as HTMLElement | null } : undefined"Or more simply, since
containerElexpectsHTMLElement | null:-:ref="isResizable ? (el: any) => { containerEl = el } : undefined" +:ref="isResizable ? (el) => { containerEl = el as HTMLElement | null } : undefined"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Sidebar.vue` at line 385, The ref callback in Sidebar.vue uses the unsafe any type: replace the parameter typing in the ternary ref expression (:ref="isResizable ? (el: any) => { containerEl = el } : undefined") with the correct element type that matches containerEl (e.g. (el: HTMLElement | null) => { containerEl = el }) so you avoid bypassing type checking and keep containerEl typed as HTMLElement | null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Sidebar.vue`:
- Around line 210-213: The synchronous assignment that sets modelOpen.value
based on isMobile, canCollapse and isCollapsed risks SSR hydration mismatch;
move this logic out of setup and into an onMounted callback so it only runs on
the client after hydration: in Sidebar.vue remove the immediate block that
checks isMobile.value && canCollapse.value && isCollapsed.value and instead
perform the same check inside onMounted() and set modelOpen.value = false there
(referencing isMobile, canCollapse, isCollapsed and modelOpen to locate the
code).
- Around line 215-216: The module-level mutable variable didDrag must be moved
into the component instance as a reactive ref to avoid cross-instance
interference: remove the top-level `let didDrag = false` and instead create
`const didDrag = ref(false)` inside the component's setup (importing ref from
'vue'), then update all usages/assignments to `didDrag.value` (e.g. in your
mouse/drag handlers such as onRailMouseDown, onDocumentMouseMove,
onDocumentMouseUp or whatever functions reference didDrag) so each Sidebar
instance has its own drag state.
---
Nitpick comments:
In `@src/runtime/components/Sidebar.vue`:
- Line 385: The ref callback in Sidebar.vue uses the unsafe any type: replace
the parameter typing in the ternary ref expression (:ref="isResizable ? (el:
any) => { containerEl = el } : undefined") with the correct element type that
matches containerEl (e.g. (el: HTMLElement | null) => { containerEl = el }) so
you avoid bypassing type checking and keep containerEl typed as HTMLElement |
null.
🪄 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
Run ID: a3c3e560-dc10-43fb-a6b5-814ce591ea86
⛔ Files ignored due to path filters (4)
test/components/__snapshots__/ScrollArea-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/ScrollArea.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Sidebar-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Sidebar.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
docs/app/components/content/examples/scroll-area/ScrollAreaInfiniteScrollExample.vuedocs/app/components/content/examples/scroll-area/ScrollAreaLeadingTrailingExample.vuedocs/app/components/content/examples/sidebar/SidebarExample.vuedocs/content/docs/2.components/dashboard-sidebar.mddocs/content/docs/2.components/scroll-area.mddocs/content/docs/2.components/sidebar.mdplaygrounds/nuxt/app/pages/components/scroll-area.vuesrc/runtime/components/ScrollArea.vuesrc/runtime/components/Sidebar.vuesrc/theme/sidebar.tstest/components/ScrollArea.spec.ts
✅ Files skipped from review due to trivial changes (3)
- docs/content/docs/2.components/dashboard-sidebar.md
- docs/app/components/content/examples/sidebar/SidebarExample.vue
- docs/app/components/content/examples/scroll-area/ScrollAreaInfiniteScrollExample.vue
🚧 Files skipped from review as they are similar to previous changes (4)
- test/components/ScrollArea.spec.ts
- docs/content/docs/2.components/scroll-area.md
- src/runtime/components/ScrollArea.vue
- docs/app/components/content/examples/scroll-area/ScrollAreaLeadingTrailingExample.vue
src/runtime/components/Sidebar.vue
Outdated
| // Sync initial persisted collapsed state to open model | ||
| if (!isMobile.value && canCollapse.value && isCollapsed.value) { | ||
| modelOpen.value = false | ||
| } |
There was a problem hiding this comment.
Potential SSR hydration mismatch when syncing persisted collapse state.
This synchronous assignment during component setup runs before hydration completes. If the server renders with modelOpen=true (default) but the client cookie has collapsed=true, the initial DOM won't match, potentially causing a hydration mismatch and a visible layout shift.
Consider deferring this sync to onMounted to ensure it runs only on the client after hydration:
Proposed fix
-// Sync initial persisted collapsed state to open model
-if (!isMobile.value && canCollapse.value && isCollapsed.value) {
- modelOpen.value = false
-}
+// Sync initial persisted collapsed state to open model (after hydration)
+onMounted(() => {
+ if (!isMobile.value && canCollapse.value && isCollapsed.value) {
+ modelOpen.value = false
+ }
+})📝 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.
| // Sync initial persisted collapsed state to open model | |
| if (!isMobile.value && canCollapse.value && isCollapsed.value) { | |
| modelOpen.value = false | |
| } | |
| // Sync initial persisted collapsed state to open model (after hydration) | |
| onMounted(() => { | |
| if (!isMobile.value && canCollapse.value && isCollapsed.value) { | |
| modelOpen.value = false | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/Sidebar.vue` around lines 210 - 213, The synchronous
assignment that sets modelOpen.value based on isMobile, canCollapse and
isCollapsed risks SSR hydration mismatch; move this logic out of setup and into
an onMounted callback so it only runs on the client after hydration: in
Sidebar.vue remove the immediate block that checks isMobile.value &&
canCollapse.value && isCollapsed.value and instead perform the same check inside
onMounted() and set modelOpen.value = false there (referencing isMobile,
canCollapse, isCollapsed and modelOpen to locate the code).
src/runtime/components/Sidebar.vue
Outdated
| // Track whether mousedown resulted in a drag (to distinguish click vs drag on the rail) | ||
| let didDrag = false |
There was a problem hiding this comment.
Module-level let didDrag may cause issues with multiple component instances.
The didDrag variable is declared outside any reactive scope as a plain let. If multiple Sidebar components are rendered simultaneously, they will share this variable, causing incorrect drag detection behavior.
Proposed fix using ref
-// Track whether mousedown resulted in a drag (to distinguish click vs drag on the rail)
-let didDrag = false
+// Track whether mousedown resulted in a drag (to distinguish click vs drag on the rail)
+const didDrag = ref(false)
function onRailMouseDown(e: MouseEvent) {
- didDrag = false
+ didDrag.value = false
const startX = e.clientX
const onMove = (ev: MouseEvent) => {
- if (Math.abs(ev.clientX - startX) > 3) didDrag = true
+ if (Math.abs(ev.clientX - startX) > 3) didDrag.value = true
}
// ...
}
function onRailClick() {
if (!isResizable.value) return (open.value = !open.value)
- if (!didDrag && canCollapse.value) collapse(!isCollapsed.value)
+ if (!didDrag.value && canCollapse.value) collapse(!isCollapsed.value)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/Sidebar.vue` around lines 215 - 216, The module-level
mutable variable didDrag must be moved into the component instance as a reactive
ref to avoid cross-instance interference: remove the top-level `let didDrag =
false` and instead create `const didDrag = ref(false)` inside the component's
setup (importing ref from 'vue'), then update all usages/assignments to
`didDrag.value` (e.g. in your mouse/drag handlers such as onRailMouseDown,
onDocumentMouseMove, onDocumentMouseUp or whatever functions reference didDrag)
so each Sidebar instance has its own drag state.
…ate scrollMargin handling
|
#6237 for discussion |
2 participants
❓ Type of change
📚 Description
Adds #leading and #trailing named slots to ScrollArea, allowing content to be rendered before and after the scroll items inside the scroll container.
Why: Currently, when using the virtualize, the default slot is consumed per-item — there's no way to inject content like headers, footers, or loading indicators within the scrollable area. These slots fill that gap without breaking changes.
📝 Checklist