feat(Sidebar): add resizable functionality#6340
Conversation
|
@benjamincanac ha, sorry about that, misread the silence! Reopening this. No rush on review, happy to address feedback whenever you get to it - was just trying to avoid being part of the spam/bloat train 😅 |
📝 WalkthroughWalkthroughAdds optional resizable behavior to Sidebar via a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
src/runtime/components/Sidebar.vue (2)
239-241: Consider adding a clarifying comment for the sync condition.The condition
isCollapsed.value === vworks correctly becausemodelOpenandisCollapsedhave inverted semantics (when they're the same boolean value, they're actually out of sync). However, this logic is non-obvious at first glance.💡 Suggested clarification
watch(modelOpen, (v) => { + // isCollapsed and modelOpen have inverted semantics, so same values mean out-of-sync if (!isMobile.value && canCollapse.value && isCollapsed.value === v) collapse(!v) })🤖 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 239 - 241, Add a short clarifying comment above the watch(...) that explains the inverted semantics: note that modelOpen and isCollapsed purposely have opposite meanings (when modelOpen.value === isCollapsed.value they are out of sync), so the check isCollapsed.value === v is intentional and collapse(!v) flips the UI to sync them; keep the existing guards (isMobile, canCollapse) and reference the watch, modelOpen, isMobile, canCollapse, isCollapsed, and collapse symbols in the comment so future readers understand the non‑obvious boolean inversion.
388-391: Event handler pattern works but has a subtle inefficiency.When
isResizableis false,handleMouseDown($event)is still evaluated (the function call is prepared even thoughundefinedis returned). While functionally correct, you could simplify with conditional binding.That said, this is a minor nitpick and the current approach is readable. No action required unless you prefer the alternative pattern.
💡 Alternative using short-circuit evaluation
- `@mousedown`="isResizable ? handleMouseDown($event) : undefined" - `@touchstart`="isResizable ? handleTouchStart($event) : undefined" - `@dblclick`="isResizable ? handleDoubleClick($event) : undefined" + `@mousedown`="isResizable && handleMouseDown($event)" + `@touchstart`="isResizable && handleTouchStart($event)" + `@dblclick`="isResizable && handleDoubleClick($event)"🤖 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 388 - 391, The template currently conditionally calls handlers with ternaries (e.g., `@mousedown`="isResizable ? handleMouseDown($event) : undefined") which still evaluates the call expression; replace these with short-circuit bindings so the handler function reference is only used when isResizable is true: change `@mousedown`, `@touchstart`, and `@dblclick` to use expressions like `@mousedown`="isResizable && handleMouseDown", `@touchstart`="isResizable && handleTouchStart", `@dblclick`="isResizable && handleDoubleClick" (leave `@click`="onRailClick" as-is); this uses isResizable, handleMouseDown, handleTouchStart, and handleDoubleClick to avoid unnecessary call-site preparation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/Sidebar.vue`:
- Around line 239-241: Add a short clarifying comment above the watch(...) that
explains the inverted semantics: note that modelOpen and isCollapsed purposely
have opposite meanings (when modelOpen.value === isCollapsed.value they are out
of sync), so the check isCollapsed.value === v is intentional and collapse(!v)
flips the UI to sync them; keep the existing guards (isMobile, canCollapse) and
reference the watch, modelOpen, isMobile, canCollapse, isCollapsed, and collapse
symbols in the comment so future readers understand the non‑obvious boolean
inversion.
- Around line 388-391: The template currently conditionally calls handlers with
ternaries (e.g., `@mousedown`="isResizable ? handleMouseDown($event) : undefined")
which still evaluates the call expression; replace these with short-circuit
bindings so the handler function reference is only used when isResizable is
true: change `@mousedown`, `@touchstart`, and `@dblclick` to use expressions like
`@mousedown`="isResizable && handleMouseDown", `@touchstart`="isResizable &&
handleTouchStart", `@dblclick`="isResizable && handleDoubleClick" (leave
`@click`="onRailClick" as-is); this uses isResizable, handleMouseDown,
handleTouchStart, and handleDoubleClick to avoid unnecessary call-site
preparation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58d6f574-ae1b-4d1f-9690-14a462550597
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Sidebar-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Sidebar.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/app/components/content/examples/sidebar/SidebarExample.vuedocs/content/docs/2.components/dashboard-sidebar.mddocs/content/docs/2.components/sidebar.mdsrc/runtime/components/Sidebar.vuesrc/theme/sidebar.ts
…treamline event handling
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/components/Sidebar.vue (1)
162-171:⚠️ Potential issue | 🟡 Minor
isCollapsedandmodelOpencan diverge across viewport transitions.On
mobile → desktopthe watcher on line 162 restoresmodelOpenfromdesktopOpen, butisCollapsed(owned byuseResizable) is not reconciled. If the user toggled/collapsed on desktop, went to mobile (forcingmodelOpen = false), then returned to desktop, the restoredmodelOpenmay equal the staleisCollapsedin a way the "inverted-semantics" guard at line 238 won't catch reliably (no value change onmodelOpenfires the watcher). Consider reconciling explicitly after restoration, e.g.:Proposed fix
watch(isMobile, (mobile) => { if (mobile) { desktopOpen.value = modelOpen.value modelOpen.value = false } else { modelOpen.value = desktopOpen.value + if (canCollapse.value && isCollapsed.value === modelOpen.value) { + collapse(!modelOpen.value) + } } }, { immediate: true })Also applies to: 233-239
🤖 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 162 - 171, The watcher on isMobile restores modelOpen from desktopOpen but doesn't reconcile the isCollapsed state from useResizable, causing divergence; update the watcher so that after restoring modelOpen.value = desktopOpen.value you also explicitly set isCollapsed.value (or call the useResizable sync method) to the correct value (e.g., isCollapsed.value = !modelOpen.value or the project’s equivalent inversion) so both modelOpen and isCollapsed remain consistent when switching mobile↔desktop; ensure the assignment happens after the modelOpen restore (use nextTick if needed) so any dependent watchers run correctly.
🧹 Nitpick comments (1)
src/runtime/components/Sidebar.vue (1)
219-224:railCursoronly reflects themaxSizeboundary, notminSize.At
minSize(when still expanded, not yet snapped tocollapsedSize), the cursor stays bidirectional instead of indicating the only valid direction. Consider mirroring themaxSizebranch forexpandedWidth.value <= props.minSize.🤖 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 219 - 224, The railCursor computed currently only handles the maxSize boundary; add a mirrored branch that checks expandedWidth.value <= props.minSize (and only when not collapsed) and returns the opposite directional cursor to indicate only expansion is allowed — specifically, in the new branch return props.side === 'left' ? 'e-resize' : 'w-resize'; update the railCursor computation (which references isResizable, isCollapsed, expandedWidth, props.maxSize and props.minSize) to include this minSize branch before falling back to 'ew-resize'.
🤖 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`:
- Line 194: Make sidebarId reactive and avoid double-prefixing by generating a
stable fallback id once (const generatedId = useId()), then create sidebarId as
a computed: const sidebarId = computed(() => props.id ??
`sidebar-${generatedId}`); this way a user-supplied props.id is used verbatim
and later changes to props.id are reflected. Also update the useResizable call
to receive the actual string (pass sidebarId.value or unwrap it where storageKey
is built) so the persisted storageKey matches the consumer-provided id.
- Around line 209-212: The code unconditionally forces modelOpen.value = false
when a persisted collapsed state exists, which can override a parent-controlled
v-model:open; update the setup logic around canCollapse, isCollapsed and
modelOpen so you only apply the persisted collapse when the parent did not
explicitly provide an open value (i.e., detect absence of an external
v-model:open and only then set modelOpen.value = false), or add a boolean prop
(e.g., persistent) and guard the sync with that prop (apply the persisted state
only when persistent is true); adjust the check where modelOpen is set to
consult either the absence of an external open prop or the new persistent prop
before changing modelOpen.value.
---
Outside diff comments:
In `@src/runtime/components/Sidebar.vue`:
- Around line 162-171: The watcher on isMobile restores modelOpen from
desktopOpen but doesn't reconcile the isCollapsed state from useResizable,
causing divergence; update the watcher so that after restoring modelOpen.value =
desktopOpen.value you also explicitly set isCollapsed.value (or call the
useResizable sync method) to the correct value (e.g., isCollapsed.value =
!modelOpen.value or the project’s equivalent inversion) so both modelOpen and
isCollapsed remain consistent when switching mobile↔desktop; ensure the
assignment happens after the modelOpen restore (use nextTick if needed) so any
dependent watchers run correctly.
---
Nitpick comments:
In `@src/runtime/components/Sidebar.vue`:
- Around line 219-224: The railCursor computed currently only handles the
maxSize boundary; add a mirrored branch that checks expandedWidth.value <=
props.minSize (and only when not collapsed) and returns the opposite directional
cursor to indicate only expansion is allowed — specifically, in the new branch
return props.side === 'left' ? 'e-resize' : 'w-resize'; update the railCursor
computation (which references isResizable, isCollapsed, expandedWidth,
props.maxSize and props.minSize) to include this minSize branch before falling
back to 'ew-resize'.
🪄 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: 7001c7c1-7f78-430d-8d8b-7507c253201a
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Sidebar-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Sidebar.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
playgrounds/nuxt/app/pages/components/sidebar.vuesrc/runtime/components/Sidebar.vuetest/components/Sidebar.spec.ts
✅ Files skipped from review due to trivial changes (1)
- playgrounds/nuxt/app/pages/components/sidebar.vue
| // Resizable rail integration | ||
| const isResizable = computed(() => props.rail && props.resizable) | ||
| const canCollapse = computed(() => isResizable.value && props.collapsible !== 'none') | ||
| const sidebarId = `sidebar-${props.id || useId()}` |
There was a problem hiding this comment.
sidebarId double-prefixes user-provided id and isn't reactive.
When a consumer passes id="my-sidebar", the resulting id becomes sidebar-my-sidebar, which is then used as the storageKey inside useResizable. This makes it hard for users to predict/align with the persisted cookie/localStorage key. Also, because the expression runs once in setup with plain ||, later changes to props.id aren't reflected.
Proposed fix
-const sidebarId = `sidebar-${props.id || useId()}`
+const fallbackId = useId()
+const sidebarId = computed(() => props.id ?? `sidebar-${fallbackId}`)And pass sidebarId.value (or wrap the useResizable call accordingly) so the storage key matches what consumers expect.
🤖 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 194, Make sidebarId reactive and
avoid double-prefixing by generating a stable fallback id once (const
generatedId = useId()), then create sidebarId as a computed: const sidebarId =
computed(() => props.id ?? `sidebar-${generatedId}`); this way a user-supplied
props.id is used verbatim and later changes to props.id are reflected. Also
update the useResizable call to receive the actual string (pass sidebarId.value
or unwrap it where storageKey is built) so the persisted storageKey matches the
consumer-provided id.
| // Sync initial persisted collapsed state to open model | ||
| if (canCollapse.value && isCollapsed.value) { | ||
| modelOpen.value = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the existing dashboard sidebar handles persisted-collapsed vs v-model open sync
fd -t f 'DashboardSidebar.vue' | xargs -I{} sed -n '1,260p' {}Repository: nuxt/ui
Length of output: 22224
🏁 Script executed:
fd -t f 'Sidebar.vue' | grep -v DashboardSidebarRepository: nuxt/ui
Length of output: 140
🏁 Script executed:
cat -n src/runtime/components/Sidebar.vueRepository: nuxt/ui
Length of output: 17013
Establish parent intent before syncing persisted collapsed state to open model.
At setup time, lines 209–212 unconditionally set modelOpen.value = false whenever a persisted collapsed state exists, even if the parent explicitly passed v-model:open="true". This silently overrides the parent's intent without a guard. Consider only applying this sync when the parent did not explicitly provide open (treat the default true as "no external control"), or introduce an opt-in persistent prop to gate the behavior.
🤖 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 209 - 212, The code
unconditionally forces modelOpen.value = false when a persisted collapsed state
exists, which can override a parent-controlled v-model:open; update the setup
logic around canCollapse, isCollapsed and modelOpen so you only apply the
persisted collapse when the parent did not explicitly provide an open value
(i.e., detect absence of an external v-model:open and only then set
modelOpen.value = false), or add a boolean prop (e.g., persistent) and guard the
sync with that prop (apply the persisted state only when persistent is true);
adjust the check where modelOpen is set to consult either the absence of an
external open prop or the new persistent prop before changing modelOpen.value.
2 participants
🔗 Linked issue
Discussion in #6038
❓ Type of change
📚 Description
The new sidebar component is great, I have a similar pattern built into my own apps, but without the resizing I still couldn't use it. I've reused the composables from dashboard sidebar and tried to mimic as much of the patterns used there.
📝 Checklist