Open
Conversation
- Design spec capturing the Drawer primitive surface, gesture model, snap-point semantics, nested drawer indent effect, and modal tiers. - Initial implementation plan (phased rollout of Root, Content, gestures, snap points, Provider/Indent, stories, tests). - BaseUI parity plan covering the follow-up fix pass: C1-C5 critical items, I1-I10 important items, and minor fixes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dent effect
Vue port of BaseUI's Drawer component system. Standalone primitive
built on FocusScope, DismissableLayer, Presence, and Teleport.
Parts: Root, Trigger, Close, Title, Description, Portal, Overlay
(aliased as Backdrop), Content (aliased as Popup), Viewport, Handle,
SwipeArea, Provider, Indent, IndentBackground.
Features:
- 4-sided swipe-to-dismiss (top/right/bottom/left) with pointer +
touch event handling, axis locking, directional damping, reversal
cancel, and release-velocity tracking.
- Swipe-to-open via DrawerSwipeArea for off-screen edge triggers.
- Snap points as fractions (0-1), absolute pixels, 'Npx' strings, or
'Nrem' strings. Unknown units are dropped. Active snap point
offsets are written to --drawer-snap-point-offset.
- Release math ported line-for-line from BaseUI DrawerViewport
(velocity-boosted target offset with SNAP_VELOCITY_MULTIPLIER=300,
close-vs-closest-snap distance comparison, and sequential-mode
shouldAdvance + physical-crossing rules).
- --drawer-swipe-strength CSS var on release via
computeSwipeReleaseScalar (remaining-distance / velocity scalar,
clamped to [0.1, 1]).
- Scroll-edge swipe allowance so scrollable drawer content is still
dismissable once the scroll container reaches the relevant edge.
- CSS.registerProperty for all high-frequency vars (swipeMovementX/Y,
snapPointOffset, swipeProgress, swipeStrength).
- Modal tiers: true (focus trap + scroll lock + block outside events),
'trap-focus' (focus trap only, outside events pass through), false
(non-modal).
- update:open carries ChangeEventDetails.reason ('trigger-press',
'close-press', 'swipe', 'escape-key', 'outside-press') and
update:openComplete fires on transition end.
- initialFocus / finalFocus props on DrawerContent.
- Nested drawers: Provider tracks running nestedOpenDrawerCount,
parent popups subscribe to nestedSwipeProgressStore to animate in
reverse while child swipes away, and keepHeightWhileNested guards
ResizeObserver so opening a child doesn't mutate parent snap
geometry.
- Indent effect: DrawerIndent subscribes to visualStateStore and
writes --drawer-swipe-progress imperatively for consumer CSS to
interpolate transforms in sync with the drag.
- Backdrop (DrawerOverlayImpl) carries data-swiping /
data-swipe-direction so [data-swiping] selectors work without
reaching through to the popup.
- Close-animation continuity: on swipe-dismiss the drag transform is
preserved through the close transition (no 1-frame snap-back), and
data-swipe-dismissed is set on the popup for consumer styling.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stories cover the full surface: default bottom drawer, 4-side variants, snap points, non-modal, swipe-area (swipe-to-open), nested drawers, mobile nav, scrollable content, form, and the indent-effect recipe (which interpolates the indent transform via --drawer-swipe-progress and zeros transition-duration during drag using the canonical BaseUI --indent-transition 1-or-0 multiplier). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drawer.test.ts: render + click + keyboard + a11y integration tests, including update:open ChangeEventDetails.reason assertions for trigger-press and close-press. - useDrawerSnapPoints.test.ts: 11 unit tests pinning down the ported BaseUI release math — snap-point resolution, unknown-unit parseSnapPoint filter, close-vs-snap distance decision, velocity-boost dismiss, up-drawer sign convention, and sequential-mode shouldAdvance + physical-crossing rules. - utils.test.ts: getDisplacement sign conventions across all 4 directions and computeSwipeReleaseScalar (null-below-min-velocity, null-on-zero-remaining, [0.1, 1] range, fast/slow scalar endpoints, up-direction sign handling, snapPointOffset contribution). 32 tests total, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Drawer feature: ~15 Vue components, composables for swipe gestures and snap-point math, provider/root context for nesting and CSS-variable coordination, utilities, tests, Storybook demos, docs/specs/plans, and package exports. All additions; no removals. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger as DrawerTrigger
participant Root as DrawerRoot
participant Provider as DrawerProvider
participant Overlay as DrawerOverlay
participant Content as DrawerContent
User->>Trigger: click
Trigger->>Root: onOpenChange(true, 'trigger-press')
Root->>Provider: register openById
Root->>Overlay: mount (if modal)
Root->>Content: mount (open=true)
Content->>Content: measure height & write CSS vars
Overlay->>Overlay: useBodyScrollLock activate
Provider->>DrawerIndent: visualStateStore updates
sequenceDiagram
participant User
participant SwipeArea as DrawerSwipeArea
participant Swipe as useSwipeDismiss
participant CSS as CSS_Vars
participant Snap as useDrawerSnapPoints
participant Root as DrawerRoot
participant Content as DrawerContent
User->>SwipeArea: pointer/touch start
SwipeArea->>Swipe: init tracking
User->>SwipeArea: drag
Swipe->>CSS: update --drawer-swipe-movement-*
Swipe->>Swipe: sample velocity & emit onProgress
User->>SwipeArea: release
Swipe->>Snap: snapToNearest(delta, velocity, direction)
Snap->>Root: decide snap OR dismiss
alt Dismiss
Root->>Root: onOpenChange(false, 'swipe')
Root->>Content: close flow
else Snap
Root->>Content: set activeSnapPoint, update --drawer-snap-point-offset
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
packages/core/src/Drawer/Drawer.test.ts (1)
71-74: Mock cleanup order should be reversed.
mockRestore()restores the originalconsole.warn, so callingmockClear()afterward onconsoleWarnMockFunctionis redundant (it clears the mock's call history, but the spy is already detached). If you intend to clear call history between tests while keeping the spy active, swap the order or removemockClear()entirely sincebeforeEachre-creates the spy anyway.🔧 Suggested fix
afterEach(() => { - consoleWarnMock.mockRestore() - consoleWarnMockFunction.mockClear() + consoleWarnMock.mockRestore() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/Drawer.test.ts` around lines 71 - 74, The afterEach cleanup currently calls consoleWarnMock.mockRestore() before consoleWarnMockFunction.mockClear(), which is redundant and can error because mockRestore removes the spy; in the afterEach for Drawer tests (referencing consoleWarnMock and consoleWarnMockFunction), either swap the calls so mockClear() runs before mockRestore(), or remove mockClear() entirely since beforeEach re-creates the spy; update the afterEach accordingly to clear the mock history before restoring or just restore only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-04-drawer-design.md`:
- Around line 19-42: The fenced code blocks showing the Drawer file tree
(starting with "packages/core/src/Drawer/") and the "return-shape" example need
explicit language tags to satisfy markdownlint MD040; change the file-tree fence
to use "text" (```text) and the return-shape example fence to use "ts" (```ts)
so the tree and TypeScript return-shape are properly annotated (also apply the
same edits to the other occurrence noted around lines 212-220).
In `@packages/core/src/Drawer/composables/useSwipeDismiss.ts`:
- Around line 265-278: The code is promoting a pending swipe even when dir is
undefined; only promote when an allowed direction is identified. In the
pendingSwipe block (variables: pendingSwipe, pendingSwipeStartPos, dir,
getDisplacement, MIN_DRAG_THRESHOLD), first ensure dir is defined (i.e., bail
out if dir is undefined) before computing displacement and clearing
pendingSwipe; only then set intendedDirection, swipeDirection.value, call
setSwiping(true) and onSwipeStart?.(); remove the usage of the fallback (dir ??
toValue(directions)[0]) so promotion only happens for an explicitly determined
allowed direction.
- Around line 484-487: The first touch/move event that flips the gesture into
swiping isn't being prevented because the code checks isSwiping.value before
calling processMove(), which is what sets isSwiping.value = true; modify the
move handler in useSwipeDismiss.ts so you capture the previous swiping state
(e.g., const wasSwiping = isSwiping.value), call processMove(el, pos,
e.timeStamp), then if (wasSwiping || isSwiping.value) call e.preventDefault();
this ensures the same event that transitions into swiping is prevented and
avoids the mobile scroll hitch.
In `@packages/core/src/Drawer/DrawerContent.vue`:
- Around line 75-84: In the interact-outside handler inside DrawerContent.vue
you're reassigning the ref variables instead of mutating their .value; change
the assignments to set hasInteractedOutside.value = true and
hasPointerDownOutside.value = true, and when checking the trigger element use
rootContext.triggerElement.value?.contains(target) as already done; update any
other direct ref reassignments in this handler to mutate .value so the
template/reactivity works correctly.
- Around line 67-74: The close-auto-focus handler is using the ref objects
directly instead of their values, so replace checks and resets to use
hasInteractedOutside.value and hasPointerDownOutside.value; specifically, in the
`@close-auto-focus` callback (the function that currently checks if
(!hasInteractedOutside) and later sets hasInteractedOutside = false /
hasPointerDownOutside = false), change the condition to if
(!hasInteractedOutside.value) before focusing rootContext.triggerElement.value,
and reset both refs by assigning hasInteractedOutside.value = false and
hasPointerDownOutside.value = false.
In `@packages/core/src/Drawer/DrawerIndent.vue`:
- Around line 55-56: The template sets :data-active and :data-inactive using
providerContext?.active.value which, when providerContext is missing, makes
!providerContext?.active.value truthy and always applies inactive styles; update
the bindings for data-active and data-inactive to first guard that
providerContext exists and has an active property (e.g. check providerContext !=
null and providerContext.active != null) before reading
providerContext.active.value so that neither attribute is set when there is no
provider; locate the bindings in DrawerIndent.vue (the :data-active and
:data-inactive attributes) and change their expressions to only evaluate
active.value when providerContext is present.
In `@packages/core/src/Drawer/DrawerOverlayImpl.vue`:
- Line 17: The call useBodyScrollLock(true) unconditionally locks scrolling even
when the drawer is mounted but closed (e.g. with forceMount); replace the
literal true with the drawer's open state so the lock is active only while
open—i.e., pass the reactive isOpen/ref (or a computed that returns
isOpen.value) to useBodyScrollLock in DrawerOverlayImpl.vue instead of true
(ensure you reference the same open state used by the overlay component).
In `@packages/core/src/Drawer/DrawerRoot.vue`:
- Around line 226-230: The current watch on open uses queueMicrotask to emit
'update:openComplete' too early; remove that microtask-based emit and instead
emit 'update:openComplete' from the content transition lifecycle (e.g., the
component's transition end hooks or a content element transitionend handler).
Concretely: stop emitting inside the watch(open, ...) block and add an
onAfterEnter/onAfterLeave or a transitionend handler (e.g., a method like
onContentTransitionEnd) that calls emit('update:openComplete', /* current open
state */) so the event fires only after the enter/exit CSS transition completes.
In `@packages/core/src/Drawer/DrawerSwipeArea.vue`:
- Around line 43-57: The directions array is currently fixed to the initial
value because it uses [openDirection.value]; update the useSwipeDismiss call so
the directions argument is a reactive computed that returns an array based on
openDirection (e.g., computed(() => [openDirection.value])), keeping the change
local to the directions property in the useSwipeDismiss(...) invocation; the
composable (useSwipeDismiss) already accepts MaybeRef<SwipeDirection[]>, so no
signature changes are needed and rootContext callbacks
(onDismiss/onSwipingChange) can remain untouched.
In `@packages/core/src/Drawer/DrawerTrigger.vue`:
- Around line 8-19: The onMounted-only assignment to rootContext.triggerElement
(done in the DrawerTrigger.vue using onMounted, rootContext, currentElement, and
useForwardExpose) will become stale when currentElement changes; replace it with
a reactive watch on currentElement that updates rootContext.triggerElement.value
whenever currentElement.value changes (and also set it initially), and ensure
you stop the watcher on unmount if needed so the triggerElement stays
synchronized for focus restoration in DrawerContent.
In `@packages/core/src/Drawer/DrawerViewport.vue`:
- Around line 20-31: DrawerViewport currently passes only :as, :as-child and ref
to Primitive, dropping other PrimitiveProps (class, style, id, data-attrs,
listeners); update the template for DrawerViewport to forward all remaining
props to Primitive by binding the component props object (the props returned
from defineProps / the local props variable) with v-bind (e.g., v-bind="props")
while keeping explicit :as, :as-child and :ref="forwardRef" and the data-state
attr; ensure you reference DrawerViewport, Primitive, forwardRef and asChild so
the change forwards attributes/listeners consistently with sibling components
that use v-bind="props".
In `@packages/core/src/Drawer/story/Drawer.story.vue`:
- Line 460: The min-height rule in Drawer.story.vue uses the non-existent CSS
variable --drawer-snap-height so the drawer stays at auto; update that rule to
use the actual variable --drawer-height (or delete the min-height declaration if
undesired) so the story reflects the Drawer primitive's written height; locate
the CSS block containing "min-height: var(--drawer-snap-height, auto)" and
replace the variable name with "--drawer-height" (or remove the line).
---
Nitpick comments:
In `@packages/core/src/Drawer/Drawer.test.ts`:
- Around line 71-74: The afterEach cleanup currently calls
consoleWarnMock.mockRestore() before consoleWarnMockFunction.mockClear(), which
is redundant and can error because mockRestore removes the spy; in the afterEach
for Drawer tests (referencing consoleWarnMock and consoleWarnMockFunction),
either swap the calls so mockClear() runs before mockRestore(), or remove
mockClear() entirely since beforeEach re-creates the spy; update the afterEach
accordingly to clear the mock history before restoring or just restore only.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d35700be-da56-4634-af98-3f1dd5054d6c
📒 Files selected for processing (29)
docs/superpowers/plans/2026-04-05-drawer.mddocs/superpowers/plans/2026-04-09-drawer-baseui-parity-fixes.mddocs/superpowers/specs/2026-04-04-drawer-design.mdpackages/core/src/Drawer/Drawer.test.tspackages/core/src/Drawer/DrawerClose.vuepackages/core/src/Drawer/DrawerContent.vuepackages/core/src/Drawer/DrawerContentImpl.vuepackages/core/src/Drawer/DrawerDescription.vuepackages/core/src/Drawer/DrawerHandle.vuepackages/core/src/Drawer/DrawerIndent.vuepackages/core/src/Drawer/DrawerIndentBackground.vuepackages/core/src/Drawer/DrawerOverlay.vuepackages/core/src/Drawer/DrawerOverlayImpl.vuepackages/core/src/Drawer/DrawerPortal.vuepackages/core/src/Drawer/DrawerProvider.vuepackages/core/src/Drawer/DrawerRoot.vuepackages/core/src/Drawer/DrawerSwipeArea.vuepackages/core/src/Drawer/DrawerTitle.vuepackages/core/src/Drawer/DrawerTrigger.vuepackages/core/src/Drawer/DrawerViewport.vuepackages/core/src/Drawer/composables/useDrawerSnapPoints.test.tspackages/core/src/Drawer/composables/useDrawerSnapPoints.tspackages/core/src/Drawer/composables/useSwipeDismiss.tspackages/core/src/Drawer/index.tspackages/core/src/Drawer/story/Drawer.story.vuepackages/core/src/Drawer/story/_Drawer.vuepackages/core/src/Drawer/utils.test.tspackages/core/src/Drawer/utils.tspackages/core/src/index.ts
Critical: - DrawerContent.vue: non-modal close-auto-focus and interact-outside handlers were reading/writing bare ref variables in template inline handlers. Move them to named functions in <script setup> so the hasInteractedOutside / hasPointerDownOutside refs are accessed via .value explicitly. Fixes broken focus restoration in non-modal mode. - useSwipeDismiss.ts: preventDefault was guarded on isSwiping.value BEFORE processMove, so the very move that crosses the swipe threshold (and sets isSwiping=true) was not prevented, causing a one-frame scroll hitch on mobile. Capture wasSwiping before processMove and prevent on either side of the transition. Major: - useSwipeDismiss.ts: the pendingSwipe block was promoting to an active swipe even when dir === undefined (user dragging against the dismiss direction), which could steal scroll/drag on one-direction drawers. Bail out early when no allowed direction is identified. - DrawerOverlayImpl.vue: useBodyScrollLock was locking unconditionally, which could keep page scroll blocked while forceMount kept the overlay mounted with the drawer closed. Use the returned `locked` ref and sync it with rootContext.open.value via a watcher. - DrawerRoot.vue: update:openComplete was firing via queueMicrotask, which completes almost immediately after the open state changes -- well before any CSS transition finishes. Move the emit to DrawerContentImpl which attaches a one-shot transitionend/animationend listener on the popup element and calls rootContext.notifyOpenComplete only when the transition actually finishes. Guards against child element transitions via event.target === el. - DrawerSwipeArea.vue: directions array captured the initial value only. Wrap in computed so prop/state changes propagate to useSwipeDismiss. - DrawerViewport.vue: only :as and :as-child were forwarded to Primitive, dropping class/style/id/listeners/data-attrs. Use v-bind="props" to match sibling components. Minor: - DrawerIndent.vue: the data-inactive attribute was always set when providerContext was missing. Only set when active is explicitly false. - DrawerTrigger.vue: triggerElement was assigned in onMounted only. Replace with watch on currentElement so v-if / conditional rendering scenarios keep the trigger reference in sync. Clear on unmount. - docs/superpowers/specs/2026-04-04-drawer-design.md: add language tags to fenced blocks (text) to satisfy markdownlint MD040. Not applied: - story --drawer-snap-height -> --drawer-height: CodeRabbit claimed the snap-height var is not produced by the primitive, but it is written by DrawerContentImpl:77 when a snap point is active (and cleared on 81). Changing the story would break snap-point height behavior. Tests: 32/32 still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed CodeRabbit review in 63ea0fb. Fixed (11/12): Critical
Major
Minor
Pushed back on (1/12):
Verification: 32/32 tests passing, Drawer typecheck clean. 🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/Drawer/composables/useSwipeDismiss.ts (3)
206-214: Unused return value fromgetElementTransform.The return value from
getElementTransform(el)is discarded. If the intent is to prime a cache or trigger a side effect, consider adding a comment. Otherwise, if the transform data is needed later, store it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/composables/useSwipeDismiss.ts` around lines 206 - 214, The call to getElementTransform(el) inside startSwipe discards its return value; either store that transform for later use (e.g., assign to a new variable like initialTransform or previousTransform at the top-level or in the startSwipe scope) and use it where transform/state adjustments occur, or if the function is only invoked for a side-effect, add a concise comment explaining that side-effect to avoid linter warnings; update startSwipe to assign the result when the returned transform is needed by other logic (references: function startSwipe, getElementTransform, variables dragStartPos/pendingSwipeStartPos/elementSize).
436-443: Single-axis scrollable ancestor detection may miss horizontal scrollables in bi-directional drawers.When the drawer allows both vertical and horizontal swipe directions, this logic always prioritizes
'vertical'for scrollable ancestor detection. If a horizontal scrollable exists but not a vertical one, it won't be found, potentially causing unexpected scroll conflicts.💡 Suggested improvement
if (!options.ignoreScrollableAncestors) { - const axis = hasVertical.value ? 'vertical' : 'horizontal' - const scrollable = findScrollableAncestor(target, axis) + // Check both axes when drawer supports both directions + let scrollable: HTMLElement | null = null + if (hasVertical.value) + scrollable = findScrollableAncestor(target, 'vertical') + if (!scrollable && hasHorizontal.value) + scrollable = findScrollableAncestor(target, 'horizontal') if (scrollable) { swipeFromScrollable = true scrollableAncestor = scrollable } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/composables/useSwipeDismiss.ts` around lines 436 - 443, The current logic inside useSwipeDismiss uses hasVertical.value to choose axis and calls findScrollableAncestor(target, axis), which biases to 'vertical' and can miss horizontal scrollables when bi-directional swipes are allowed; update the detection to check both axes when the drawer supports both directions: first check vertical (hasVertical) then fallback to horizontal (or vice versa based on hasHorizontal if present), or explicitly call findScrollableAncestor for both 'vertical' and 'horizontal' when not ignoring scrollable ancestors; set swipeFromScrollable and scrollableAncestor based on the first non-null result so horizontal scrollables are detected in bi-directional drawers (refer to options.ignoreScrollableAncestors, hasVertical, findScrollableAncestor, swipeFromScrollable, scrollableAncestor).
394-397: Empty catch block silently swallows errors.Silently ignoring
setPointerCapturefailures can mask issues during development. Consider logging in dev mode or adding a comment explaining why it's safe to ignore.💡 Suggested improvement
try { el.setPointerCapture(e.pointerId) } - catch {} + catch { + // Pointer capture may fail if the element is removed or not connected. + // Safe to ignore since we track the pointer via activePointerId. + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/composables/useSwipeDismiss.ts` around lines 394 - 397, The empty catch in useSwipeDismiss around el.setPointerCapture(e.pointerId) silently swallows errors; update the catch to either log the caught error in non-production/dev builds (e.g., console.warn or processLogger.warn) or add a clear comment explaining why ignoring the error is safe (pointer capture failures are expected on some browsers/devices). Target the try/catch surrounding setPointerCapture inside the useSwipeDismiss composable and ensure the handler surfaces the error only in dev (NODE_ENV check) or documents rationale if intentionally suppressed.packages/core/src/Drawer/DrawerContentImpl.vue (1)
197-220: Use proper event types instead ofany.The event handlers
onPointerDownOutside,onFocusOutside, andonInteractOutsideuseevent: any. Using the proper types fromDismissableLayerimproves type safety and IDE support.💡 Suggested fix
+import type { FocusOutsideEvent, PointerDownOutsideEvent } from '@/DismissableLayer' + // ... -function onPointerDownOutside(event: any) { +function onPointerDownOutside(event: PointerDownOutsideEvent) { if (isSwiping.value) { event.preventDefault() return } pendingDismissReason = 'outside-press' emits('pointerDownOutside', event) } -function onFocusOutside(event: any) { +function onFocusOutside(event: FocusOutsideEvent) { if (isSwiping.value) { event.preventDefault() return } emits('focusOutside', event) } -function onInteractOutside(event: any) { +function onInteractOutside(event: PointerDownOutsideEvent | FocusOutsideEvent) { if (isSwiping.value) { event.preventDefault() return } emits('interactOutside', event) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/DrawerContentImpl.vue` around lines 197 - 220, The three handlers use event: any; change them to use the proper DismissableLayer event types by importing the appropriate types from the DismissableLayer module and updating the signatures: onPointerDownOutside(event: DismissableLayerPointerDownOutsideEvent), onFocusOutside(event: DismissableLayerFocusOutsideEvent), onInteractOutside(event: DismissableLayerInteractOutsideEvent); ensure the imports are added at the top of the file and update any related tooling/type references so the handlers keep the same logic but now have strong typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Drawer/DrawerContentImpl.vue`:
- Around line 235-256: The watcher for rootContext.open in DrawerContentImpl
currently waits only for transitionend/animationend which can never fire if
there are no CSS transitions; update the handler to detect and fallback: inside
the watch (using currentElement.value and
clearOpenCompleteListener/openCompleteCleanup), read getComputedStyle(el) for
transition-duration and animation-duration (and their delays) and if all
durations are zero call rootContext.notifyOpenComplete(isOpen) immediately; if
durations exist, register the existing transitionend/animationend handlers but
also start a safety timeout calculated from the computed durations plus a small
buffer that will call rootContext.notifyOpenComplete(isOpen) if events never
arrive; ensure the timeout is cleared in
clearOpenCompleteListener/openCompleteCleanup along with removing event
listeners to avoid leaks or double-calls.
---
Nitpick comments:
In `@packages/core/src/Drawer/composables/useSwipeDismiss.ts`:
- Around line 206-214: The call to getElementTransform(el) inside startSwipe
discards its return value; either store that transform for later use (e.g.,
assign to a new variable like initialTransform or previousTransform at the
top-level or in the startSwipe scope) and use it where transform/state
adjustments occur, or if the function is only invoked for a side-effect, add a
concise comment explaining that side-effect to avoid linter warnings; update
startSwipe to assign the result when the returned transform is needed by other
logic (references: function startSwipe, getElementTransform, variables
dragStartPos/pendingSwipeStartPos/elementSize).
- Around line 436-443: The current logic inside useSwipeDismiss uses
hasVertical.value to choose axis and calls findScrollableAncestor(target, axis),
which biases to 'vertical' and can miss horizontal scrollables when
bi-directional swipes are allowed; update the detection to check both axes when
the drawer supports both directions: first check vertical (hasVertical) then
fallback to horizontal (or vice versa based on hasHorizontal if present), or
explicitly call findScrollableAncestor for both 'vertical' and 'horizontal' when
not ignoring scrollable ancestors; set swipeFromScrollable and
scrollableAncestor based on the first non-null result so horizontal scrollables
are detected in bi-directional drawers (refer to
options.ignoreScrollableAncestors, hasVertical, findScrollableAncestor,
swipeFromScrollable, scrollableAncestor).
- Around line 394-397: The empty catch in useSwipeDismiss around
el.setPointerCapture(e.pointerId) silently swallows errors; update the catch to
either log the caught error in non-production/dev builds (e.g., console.warn or
processLogger.warn) or add a clear comment explaining why ignoring the error is
safe (pointer capture failures are expected on some browsers/devices). Target
the try/catch surrounding setPointerCapture inside the useSwipeDismiss
composable and ensure the handler surfaces the error only in dev (NODE_ENV
check) or documents rationale if intentionally suppressed.
In `@packages/core/src/Drawer/DrawerContentImpl.vue`:
- Around line 197-220: The three handlers use event: any; change them to use the
proper DismissableLayer event types by importing the appropriate types from the
DismissableLayer module and updating the signatures: onPointerDownOutside(event:
DismissableLayerPointerDownOutsideEvent), onFocusOutside(event:
DismissableLayerFocusOutsideEvent), onInteractOutside(event:
DismissableLayerInteractOutsideEvent); ensure the imports are added at the top
of the file and update any related tooling/type references so the handlers keep
the same logic but now have strong typing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a8fd102-267b-4eb1-ab27-750516f3cbb2
📒 Files selected for processing (10)
docs/superpowers/specs/2026-04-04-drawer-design.mdpackages/core/src/Drawer/DrawerContent.vuepackages/core/src/Drawer/DrawerContentImpl.vuepackages/core/src/Drawer/DrawerIndent.vuepackages/core/src/Drawer/DrawerOverlayImpl.vuepackages/core/src/Drawer/DrawerRoot.vuepackages/core/src/Drawer/DrawerSwipeArea.vuepackages/core/src/Drawer/DrawerTrigger.vuepackages/core/src/Drawer/DrawerViewport.vuepackages/core/src/Drawer/composables/useSwipeDismiss.ts
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/specs/2026-04-04-drawer-design.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/Drawer/DrawerTrigger.vue
- packages/core/src/Drawer/DrawerIndent.vue
- packages/core/src/Drawer/DrawerContent.vue
- packages/core/src/Drawer/DrawerOverlayImpl.vue
- packages/core/src/Drawer/DrawerRoot.vue
The Snap Points story used `min-height: var(--drawer-snap-height)` on the popup, which the library wrote based on `popupHeight - offset`. Since popupHeight was measured from the popup's rendered height, and that height depended on min-height, snap resolution couldn't exceed the popup's natural content height: both 0.5 and 1.0 snap points got clamped to popupHeight (~270px of natural content) and deduplicated into a single offset=0 snap. Clicking "Snap to 100%" changed the active snap state but nothing moved visually. BaseUI doesn't use a snap-height CSS var at all. In their snap-points demo the popup has `max-height: 100dvh` and flex-column layout with a scrollable inner region, and the snap point is applied purely as a transform via `--drawer-snap-point-offset`. popupHeight measures as the full viewport height, so 0.5/1.0 resolve to distinct offsets and the transform slides the popup visually. Changes: - Remove `--drawer-snap-height` writes from DrawerContentImpl. Snap points are now pure transform-driven (consumer sets popup height independently, library only writes --drawer-snap-point-offset). - Drop min-height/transition from .drawer-content-bottom in the story and remove its snap-point-offset contribution (bottom variants that don't use snap points don't need it). - Add a dedicated .drawer-content-snap class that mirrors BaseUI's snap recipe: `height: 100dvh; max-height: 100dvh; flex-column` with a `.drawer-snap-scroll` child that takes `flex: 1 1 auto; min-height: 0; overflow-y: auto`. Transform is driven by --drawer-snap-point-offset + --drawer-swipe-movement-y. - Rewire the Snap Points variant to use the new class. Verified in histoire: Snap to 0.5 → drawer fills bottom 50% of viewport, Snap to 1.0 → drawer fills the whole viewport, transitions are smooth. Note on "Invalid vnode type when creating vnode: undefined" warning: could not reproduce locally in tests (JSDOM) or in histoire dev (Position-Top variant open/close, variant switching, sandbox direct load). If the warning is still showing after this commit, please share the exact repro path and I'll dig deeper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/Drawer/DrawerContentImpl.vue (1)
235-256:⚠️ Potential issue | 🟡 MinorAdd fallback for
notifyOpenCompletewhen drawer has no CSS transition/animation.If the drawer element lacks CSS transitions or animations, neither
transitionendnoranimationendwill fire, causingnotifyOpenCompleteto never execute. Consumers relying on this callback would hang indefinitely.Consider detecting zero-duration transitions via
getComputedStyleand firing synchronously, or adding a safety timeout based on the computed duration:Example fallback approach
watch(() => rootContext.open.value, (isOpen) => { clearOpenCompleteListener() const el = currentElement.value if (!el) { rootContext.notifyOpenComplete(isOpen) return } + const cs = getComputedStyle(el) + const hasTransition = cs.transitionDuration.split(',').some(d => parseFloat(d) > 0) + const hasAnimation = cs.animationDuration.split(',').some(d => parseFloat(d) > 0) + if (!hasTransition && !hasAnimation) { + rootContext.notifyOpenComplete(isOpen) + return + } const handler = (event: Event) => { if (event.target !== el) return clearOpenCompleteListener() rootContext.notifyOpenComplete(isOpen) } // ... rest unchanged })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Drawer/DrawerContentImpl.vue` around lines 235 - 256, The watcher for rootContext.open currently only listens for transitionend/animationend and misses cases where the drawer has no CSS transition/animation; update the handler in the watch callback (where currentElement.value, el, handler, openCompleteCleanup and rootContext.notifyOpenComplete are used) to compute computedStyle = getComputedStyle(el), parse transition-duration/transition-delay and animation-duration/animation-delay into milliseconds and derive a maxDuration; if maxDuration is 0 call rootContext.notifyOpenComplete(isOpen) synchronously; otherwise set a safety timeout equal to maxDuration plus a small buffer to call notifyOpenComplete if no events fire; ensure the timeout is cleared in openCompleteCleanup alongside removing the event listeners so there are no leaks or duplicate notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Drawer/story/Drawer.story.vue`:
- Around line 400-402: The close control rendered by the DrawerClose component
uses only the "×" glyph which is ambiguous for assistive tech; update the
DrawerClose element (the DrawerClose component instance) to include an explicit
accessible name via an aria-label (for example aria-label="Close" or
aria-label="Close drawer") so the icon-only button is announced correctly to
screen readers; ensure the aria-label is added to the DrawerClose JSX/ template
attributes and not to a nested element.
- Around line 317-340: When the outer drawer closes the nested state innerOpen
isn't reset so the inner drawer can reopen unexpectedly; add logic to set
innerOpen = false whenever the outer drawer's open state changes to false
(either by watching the outer drawer's v-model variable — e.g., outerOpen — or
by handling the outer DrawerRoot's openChange/close event) so the inner
DrawerRoot/innerOpen pair is always reset when the outer drawer is dismissed.
---
Duplicate comments:
In `@packages/core/src/Drawer/DrawerContentImpl.vue`:
- Around line 235-256: The watcher for rootContext.open currently only listens
for transitionend/animationend and misses cases where the drawer has no CSS
transition/animation; update the handler in the watch callback (where
currentElement.value, el, handler, openCompleteCleanup and
rootContext.notifyOpenComplete are used) to compute computedStyle =
getComputedStyle(el), parse transition-duration/transition-delay and
animation-duration/animation-delay into milliseconds and derive a maxDuration;
if maxDuration is 0 call rootContext.notifyOpenComplete(isOpen) synchronously;
otherwise set a safety timeout equal to maxDuration plus a small buffer to call
notifyOpenComplete if no events fire; ensure the timeout is cleared in
openCompleteCleanup alongside removing the event listeners so there are no leaks
or duplicate notifications.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1b732b3-f189-4da3-9bf6-c981cdf8c813
📒 Files selected for processing (2)
packages/core/src/Drawer/DrawerContentImpl.vuepackages/core/src/Drawer/story/Drawer.story.vue
Problem: when the drawer opens at snap=0.5, the CSS keyframe animation `drawer-slide-bottom-in` was animating the `transform` property from translateY(100%) to translateY(0). Because `transform` is the same CSS property used to apply the snap-point offset, the keyframe clobbered the inline transform for the full 450ms of the enter animation. The drawer slid in all the way to translateY(0) (snap=1.0), then the animation ended and the inline transform reasserted, snapping the drawer down to translateY(400) (snap=0.5). Visible as a brief "slide past 50% then drop back" flicker. Fix: switch the snap-points drawer to a dedicated `drawer-snap-slide-in` / `drawer-snap-slide-out` pair keyed on the independent `translate` CSS property instead of `transform`. `translate` and `transform` are composable: the keyframe animates `translate: 0 100dvh -> 0 0` while the inline `transform: translateY(calc(snap-offset + swipe-y))` stays applied throughout. Enter/exit now slide from below the viewport up to the active snap point in a single continuous motion. Only the snap-points drawer needs this; the regular bottom/top/left/ right variants keep the original `transform`-keyframe animations because they have no snap-point offset to preserve. Verified in histoire: open at 0.5 slides in to 50%, Snap 100% expands smoothly via inline transform transition, Snap 50% returns smoothly, Close slides off-screen. 32/32 tests still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three interacting bugs in the snap-points path:
1. **Overshoot past viewport top during drag.** The snap-points drawer
accepts `directions = ['down', 'up']` so dragging up is an allowed
direction with no damping. Dragging aggressively from snap=0.5 could
push `--drawer-swipe-movement-y` to -500px or further, making the
computed transform negative (`translateY(400 + -500) = -100px`) and
the drawer would fly past the top edge of the viewport.
Fix: clamp the story CSS transform with `max(0px, ...)` so the
drawer can't translate above the viewport edge regardless of raw
delta. BaseUI's snap-points demo can get away without this because
their useSwipeDismiss damps non-allowed directions, but we allow
both directions for snap-point navigation.
2. **Leftover `--drawer-swipe-movement-y` after snap release.** When
bug 1's overshoot drag crossed the swipe threshold in the 'up'
direction, `finishSwipe` evaluated `shouldDismiss = true` (the
displacement along intendedDirection='up' exceeded threshold) and
took the "keep transform for close animation" branch added by the
earlier flicker fix. CSS vars were preserved, onDismiss fired but
did nothing (hasSnapPoints guard), and the stale -500px movement
value composed with the new snap=1.0 offset=0 → drawer stuck at
translateY(-500).
Fix: in `DrawerContentImpl.onRelease`, explicitly clear
`--drawer-swipe-movement-{x,y}` after calling `snapToNearest` when
snap points are active. The flicker-fix's transform preservation
is only correct when the drawer is actually closing.
3. **Reopen shows wrong snap position.** `popupHeight` is stored on
the root context and persists across close/reopen cycles. On the
first open it's 0 → `useDrawerSnapPoints` computes resolvedSnapPoints
with zero-offset entries → `activeSnapPointOffset.value` is 0/null
→ lazy `watch(activeSnapPointOffset, ...)` fires on the subsequent
ResizeObserver-driven change to popupHeight=800 → CSS var set.
But on reopen, popupHeight is already 800 from the previous mount
→ `activeSnapPointOffset.value` is correct immediately → nothing
changes → the lazy watcher never fires → the fresh element renders
with snapOffset=0 (appearing as snap=1.0 regardless of the actual
activeSnapPoint).
Fix: factor the CSS-var write into a named `writeSnapPointOffset()`
function and call it both from the watcher (for live updates) and
from `onMounted` (for the initial value on each mount). Tried
`{ immediate: true, flush: 'post' }` first but it didn't reliably
pick up the element; an explicit mount-time write is simpler and
correct.
Verified all three in live histoire with aggressive drag sequences:
- Drag up past top → drawer pinned at y=0, not overshooting
- Release → snapOffset=0, swipeY=0, active=1 (clean state)
- Snap 50% → close → reopen → y=400, snapOffset=400px, active=0.5
32/32 tests still passing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/Drawer/DrawerContentImpl.vue`:
- Around line 57-61: The snap-point dismissal path in DrawerContentImpl.vue's
onSnapPointChange currently calls rootContext.onOpenChange(false) and drops the
close reason; update that branch so when point === null you call
rootContext.onOpenChange(false, { reason: 'swipe' }) (or otherwise pass a
details object with reason: 'swipe') instead of the single-arg call, leaving the
setActiveSnapPoint path unchanged so DrawerRoot.vue receives details.reason for
swipe dismissals.
- Around line 204-223: The handlers onEscapeKeyDown and onPointerDownOutside
currently set pendingDismissReason before emitting cancelable events, which can
leak a stale reason if the event is prevented; change both functions so they set
pendingDismissReason, call emits('escapeKeyDown', event) /
emits('pointerDownOutside', event), then check event.defaultPrevented and if
true clear pendingDismissReason (set to undefined) and return without
proceeding; ensure onDismiss still uses pendingDismissReason when performing the
actual close so the reason is only preserved when the event wasn't canceled.
- Around line 254-275: The watcher for rootContext.open currently only attaches
transition/animationend listeners on subsequent open changes, missing the case
where the content mounts while open; update the mount logic (e.g., in onMounted
or immediately after currentElement is set) to perform the same arming: if
currentElement.value exists and rootContext.open.value is true, register the
same transitionend/animationend handler that calls
rootContext.notifyOpenComplete(isOpen) and set openCompleteCleanup accordingly
(mirroring the handler and cleanup created in the watch block), and fall back to
calling rootContext.notifyOpenComplete(true) synchronously if no element is
present; ensure you reuse the same handler/clearOpenCompleteListener behavior to
avoid duplicate listeners.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00db3fe9-a56d-4a50-813a-d1334ad5fe99
📒 Files selected for processing (2)
packages/core/src/Drawer/DrawerContentImpl.vuepackages/core/src/Drawer/story/Drawer.story.vue
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/Drawer/story/Drawer.story.vue
Comment on lines
+57
to
+61
| onSnapPointChange: (point) => { | ||
| if (point === null) | ||
| rootContext.onOpenChange(false) | ||
| else | ||
| rootContext.setActiveSnapPoint(point) |
Contributor
There was a problem hiding this comment.
Propagate the swipe close reason from snap-point dismissal.
When snapToNearest() resolves to null, this path closes with onOpenChange(false) and drops the reason entirely. DrawerRoot.vue only includes details.reason when a reason is passed, so snap-point swipe dismissals currently emit update:open without the advertised 'swipe' reason.
Proposed fix
onSnapPointChange: (point) => {
if (point === null)
- rootContext.onOpenChange(false)
+ rootContext.onOpenChange(false, 'swipe')
else
rootContext.setActiveSnapPoint(point)
},📝 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.
Suggested change
| onSnapPointChange: (point) => { | |
| if (point === null) | |
| rootContext.onOpenChange(false) | |
| else | |
| rootContext.setActiveSnapPoint(point) | |
| onSnapPointChange: (point) => { | |
| if (point === null) | |
| rootContext.onOpenChange(false, 'swipe') | |
| else | |
| rootContext.setActiveSnapPoint(point) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Drawer/DrawerContentImpl.vue` around lines 57 - 61, The
snap-point dismissal path in DrawerContentImpl.vue's onSnapPointChange currently
calls rootContext.onOpenChange(false) and drops the close reason; update that
branch so when point === null you call rootContext.onOpenChange(false, { reason:
'swipe' }) (or otherwise pass a details object with reason: 'swipe') instead of
the single-arg call, leaving the setActiveSnapPoint path unchanged so
DrawerRoot.vue receives details.reason for swipe dismissals.
Comment on lines
+204
to
+223
| function onDismiss() { | ||
| if (isSwiping.value) | ||
| return | ||
| rootContext.onOpenChange(false, pendingDismissReason ?? 'outside-press') | ||
| pendingDismissReason = undefined | ||
| } | ||
|
|
||
| function onEscapeKeyDown(event: KeyboardEvent) { | ||
| pendingDismissReason = 'escape-key' | ||
| emits('escapeKeyDown', event) | ||
| } | ||
|
|
||
| function onPointerDownOutside(event: any) { | ||
| if (isSwiping.value) { | ||
| event.preventDefault() | ||
| return | ||
| } | ||
| pendingDismissReason = 'outside-press' | ||
| emits('pointerDownOutside', event) | ||
| } |
Contributor
There was a problem hiding this comment.
Clear pendingDismissReason when the dismiss gets canceled.
onEscapeKeyDown() and onPointerDownOutside() cache the reason before emitting a cancelable event. If user code calls preventDefault(), dismiss never follows, so the stale reason can leak into the next close attempt.
Proposed fix
function onEscapeKeyDown(event: KeyboardEvent) {
pendingDismissReason = 'escape-key'
emits('escapeKeyDown', event)
+ if (event.defaultPrevented)
+ pendingDismissReason = undefined
}
function onPointerDownOutside(event: any) {
if (isSwiping.value) {
event.preventDefault()
return
}
pendingDismissReason = 'outside-press'
emits('pointerDownOutside', event)
+ if (event.defaultPrevented)
+ pendingDismissReason = undefined
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Drawer/DrawerContentImpl.vue` around lines 204 - 223, The
handlers onEscapeKeyDown and onPointerDownOutside currently set
pendingDismissReason before emitting cancelable events, which can leak a stale
reason if the event is prevented; change both functions so they set
pendingDismissReason, call emits('escapeKeyDown', event) /
emits('pointerDownOutside', event), then check event.defaultPrevented and if
true clear pendingDismissReason (set to undefined) and return without
proceeding; ensure onDismiss still uses pendingDismissReason when performing the
actual close so the reason is only preserved when the event wasn't canceled.
Comment on lines
+254
to
+275
| watch(() => rootContext.open.value, (isOpen) => { | ||
| clearOpenCompleteListener() | ||
| const el = currentElement.value | ||
| if (!el) { | ||
| // If there's no element (e.g. closed without ever mounting), fire | ||
| // synchronously so consumers still get the contract. | ||
| rootContext.notifyOpenComplete(isOpen) | ||
| return | ||
| } | ||
| const handler = (event: Event) => { | ||
| if (event.target !== el) | ||
| return | ||
| clearOpenCompleteListener() | ||
| rootContext.notifyOpenComplete(isOpen) | ||
| } | ||
| el.addEventListener('transitionend', handler) | ||
| el.addEventListener('animationend', handler) | ||
| openCompleteCleanup = () => { | ||
| el.removeEventListener('transitionend', handler) | ||
| el.removeEventListener('animationend', handler) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Arm openComplete for the initial mount-open transition too.
This watcher only runs on later open changes. In the normal Presence flow the content is mounted after open is already true, so no listener is registered for the enter transition and update:openComplete(true) never fires on open.
Proposed fix
let openCompleteCleanup: (() => void) | undefined
+function armOpenCompleteListener(isOpen: boolean) {
+ clearOpenCompleteListener()
+ const el = currentElement.value
+ if (!el) {
+ rootContext.notifyOpenComplete(isOpen)
+ return
+ }
+ const handler = (event: Event) => {
+ if (event.target !== el)
+ return
+ clearOpenCompleteListener()
+ rootContext.notifyOpenComplete(isOpen)
+ }
+ el.addEventListener('transitionend', handler)
+ el.addEventListener('animationend', handler)
+ openCompleteCleanup = () => {
+ el.removeEventListener('transitionend', handler)
+ el.removeEventListener('animationend', handler)
+ }
+}
+
-watch(() => rootContext.open.value, (isOpen) => {
- clearOpenCompleteListener()
- const el = currentElement.value
- if (!el) {
- rootContext.notifyOpenComplete(isOpen)
- return
- }
- const handler = (event: Event) => {
- if (event.target !== el)
- return
- clearOpenCompleteListener()
- rootContext.notifyOpenComplete(isOpen)
- }
- el.addEventListener('transitionend', handler)
- el.addEventListener('animationend', handler)
- openCompleteCleanup = () => {
- el.removeEventListener('transitionend', handler)
- el.removeEventListener('animationend', handler)
- }
-})
+watch(() => rootContext.open.value, armOpenCompleteListener)
onMounted(() => {
+ if (rootContext.open.value)
+ armOpenCompleteListener(true)
+
rootContext.contentElement.value = currentElement.value
rootContext.notifyParentHasNestedDrawer?.(true)📝 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.
Suggested change
| watch(() => rootContext.open.value, (isOpen) => { | |
| clearOpenCompleteListener() | |
| const el = currentElement.value | |
| if (!el) { | |
| // If there's no element (e.g. closed without ever mounting), fire | |
| // synchronously so consumers still get the contract. | |
| rootContext.notifyOpenComplete(isOpen) | |
| return | |
| } | |
| const handler = (event: Event) => { | |
| if (event.target !== el) | |
| return | |
| clearOpenCompleteListener() | |
| rootContext.notifyOpenComplete(isOpen) | |
| } | |
| el.addEventListener('transitionend', handler) | |
| el.addEventListener('animationend', handler) | |
| openCompleteCleanup = () => { | |
| el.removeEventListener('transitionend', handler) | |
| el.removeEventListener('animationend', handler) | |
| } | |
| }) | |
| let openCompleteCleanup: (() => void) | undefined | |
| function armOpenCompleteListener(isOpen: boolean) { | |
| clearOpenCompleteListener() | |
| const el = currentElement.value | |
| if (!el) { | |
| // If there's no element (e.g. closed without ever mounting), fire | |
| // synchronously so consumers still get the contract. | |
| rootContext.notifyOpenComplete(isOpen) | |
| return | |
| } | |
| const handler = (event: Event) => { | |
| if (event.target !== el) | |
| return | |
| clearOpenCompleteListener() | |
| rootContext.notifyOpenComplete(isOpen) | |
| } | |
| el.addEventListener('transitionend', handler) | |
| el.addEventListener('animationend', handler) | |
| openCompleteCleanup = () => { | |
| el.removeEventListener('transitionend', handler) | |
| el.removeEventListener('animationend', handler) | |
| } | |
| } | |
| watch(() => rootContext.open.value, armOpenCompleteListener) | |
| onMounted(() => { | |
| if (rootContext.open.value) | |
| armOpenCompleteListener(true) | |
| rootContext.contentElement.value = currentElement.value | |
| rootContext.notifyParentHasNestedDrawer?.(true) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Drawer/DrawerContentImpl.vue` around lines 254 - 275, The
watcher for rootContext.open currently only attaches transition/animationend
listeners on subsequent open changes, missing the case where the content mounts
while open; update the mount logic (e.g., in onMounted or immediately after
currentElement is set) to perform the same arming: if currentElement.value
exists and rootContext.open.value is true, register the same
transitionend/animationend handler that calls
rootContext.notifyOpenComplete(isOpen) and set openCompleteCleanup accordingly
(mirroring the handler and cleanup created in the watch block), and fall back to
calling rootContext.notifyOpenComplete(true) synchronously if no element is
present; ensure you reuse the same handler/clearOpenCompleteListener behavior to
avoid duplicate listeners.
…ort bottom When dragging up from snap=0.5 toward snap=1.0 and releasing, the drawer would snap down by the full viewport height (100dvh) and then slide back up to the new snap position over 450ms, instead of continuing smoothly from where the user released. Two interacting causes: 1. **Sequencing in DrawerContentImpl.onRelease.** We were clearing --drawer-swipe-movement-y before the activeSnapPoint watcher had a chance to fire, so the computed transform visibly jumped from the drag value (translateY(50)) back to the old snap position (translateY(400)) for one frame before the new snap offset arrived. Fixed by calling `writeSnapPointOffset()` synchronously after `snapToNearest` and before clearing the movement vars — both writes land in the same frame and CSS transitions from the current rendered transform to the new target. 2. **CSS animation restart in the snap story.** The bigger issue was the story's `.drawer-content-snap[data-swiping]` rule setting `animation: none`, which transitioned animation-name from `drawer-snap-slide-in` to `none` during the drag. When `data-swiping` was removed at release, animation-name went back to `drawer-snap-slide-in` — which the browser interprets as a NEW animation declaration and runs from keyframe 0 (`translate: 0 100dvh`). The element's rendered position became drag-transform (190) + translate (800) = 990 for one frame, then the transform transitioned from 190→0 while the translate animated from 800→0 — visible as "drop to the bottom of the screen then slide all the way up" on every release. Fixed by NOT touching `animation` in the data-swiping override. The enter animation completes within 450ms of mount, so it's inert by the time the user can drag. We only need to disable the `transform` transition during active swiping so it doesn't fight the raw pointer-driven transform. Verified with raf-level instrumentation in histoire at 400x800: - End of drag: y=190, swY=-210, tDur=0s - First frame after release: y=190, snap=0, swY=0, tDur=0.45s - Subsequent frames: 181.9 → 173.1 → 164.1 → ... → 0 The transform transition now starts exactly at the user's finger position and smoothly interpolates to the new snap target. Regression tests: snap-to-50% via button, small downward drag (stay at 50%), aggressive downward drag (dismiss), reopen — all correct. 32/32 tests still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zernonia
added a commit
that referenced
this pull request
Apr 9, 2026
Critical: - DrawerContent.vue: non-modal close-auto-focus and interact-outside handlers were reading/writing bare ref variables in template inline handlers. Move them to named functions in <script setup> so the hasInteractedOutside / hasPointerDownOutside refs are accessed via .value explicitly. Fixes broken focus restoration in non-modal mode. - useSwipeDismiss.ts: preventDefault was guarded on isSwiping.value BEFORE processMove, so the very move that crosses the swipe threshold (and sets isSwiping=true) was not prevented, causing a one-frame scroll hitch on mobile. Capture wasSwiping before processMove and prevent on either side of the transition. Major: - useSwipeDismiss.ts: the pendingSwipe block was promoting to an active swipe even when dir === undefined (user dragging against the dismiss direction), which could steal scroll/drag on one-direction drawers. Bail out early when no allowed direction is identified. - DrawerOverlayImpl.vue: useBodyScrollLock was locking unconditionally, which could keep page scroll blocked while forceMount kept the overlay mounted with the drawer closed. Use the returned `locked` ref and sync it with rootContext.open.value via a watcher. - DrawerRoot.vue: update:openComplete was firing via queueMicrotask, which completes almost immediately after the open state changes -- well before any CSS transition finishes. Move the emit to DrawerContentImpl which attaches a one-shot transitionend/animationend listener on the popup element and calls rootContext.notifyOpenComplete only when the transition actually finishes. Guards against child element transitions via event.target === el. - DrawerSwipeArea.vue: directions array captured the initial value only. Wrap in computed so prop/state changes propagate to useSwipeDismiss. - DrawerViewport.vue: only :as and :as-child were forwarded to Primitive, dropping class/style/id/listeners/data-attrs. Use v-bind="props" to match sibling components. Minor: - DrawerIndent.vue: the data-inactive attribute was always set when providerContext was missing. Only set when active is explicitly false. - DrawerTrigger.vue: triggerElement was assigned in onMounted only. Replace with watch on currentElement so v-if / conditional rendering scenarios keep the trigger reference in sync. Clear on unmount. - docs/superpowers/specs/2026-04-04-drawer-design.md: add language tags to fenced blocks (text) to satisfy markdownlint MD040. Not applied: - story --drawer-snap-height -> --drawer-height: CodeRabbit claimed the snap-height var is not produced by the primitive, but it is written by DrawerContentImpl:77 when a snap point is active (and cleared on 81). Changing the story would break snap-point height behavior. Tests: 32/32 still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers every library-level bug fixed in this PR's follow-up rounds.
Browser-only rendering issues (animation keyframe restart, CSS
clamping via max(0px,...)) can't be reproduced in JSDOM, but the
library behaviors they depend on are all now pinned down by tests.
New files:
**useSwipeDismiss.test.ts** (3 tests) — dismiss vs cancel CSS var
clearing behavior. Pins down the close-animation flicker fix:
- dismiss path preserves --drawer-swipe-movement-{x,y} so the close
transition runs smoothly from the dragged position
- cancel path (release below threshold) clears the vars so the drawer
animates back to rest
- data-swipe-dismissed marker attribute is set only on dismiss
- onRelease fires with a measured velocity vector
JSDOM polyfills for PointerEvent and setPointerCapture are included
in-file so the test is self-contained.
**Drawer.snap.test.ts** (6 tests) — integration tests for the three
snap-point bugs, stubbing ResizeObserver and window.innerHeight so
popupHeight resolves deterministically to 800px:
1. Initial open at snap=0.5 writes --drawer-snap-point-offset=400px.
Sanity check for the baseline snap-offset CSS var write.
2. Reopen after close writes the offset on mount (lazy-watcher bug).
The REGRESSION here: on reopen, popupHeight is already cached on
the root context, so activeSnapPointOffset is immediately correct
and a lazy `watch(activeSnapPointOffset, ...)` would never fire.
Drawer rendered as if snap=1.0 regardless of actual activeSnapPoint.
Fix: explicit writeSnapPointOffset() call in onMounted.
3. Compound close→reopen cycle preserves the correct offset.
4. Initial open at defaultSnapPoint=1 produces offset 0px.
5. Snap-to-snap release clears --drawer-swipe-movement-y. Regression
for the bug where the flicker fix's "preserve transform on dismiss"
branch left the swipe movement var set after a snap transition,
so the next frame's transform composed stale drag state into the
new snap offset.
6. Snap-to-snap release updates --drawer-snap-point-offset and clears
the movement var synchronously (sequencing regression for the
smooth drag-to-next-snap animation).
**useDrawerSnapPoints.test.ts** — 3 new overshoot-clamping tests
covering the Base UI snap-release math's `Math.max(0, Math.min(ph, ...))`
dragTargetOffset clamp. Verifies that aggressive up-drags past the
fully-open position (huge negative drag deltas) resolve to the
nearest valid snap rather than returning a negative offset. Includes
the sign-flipped equivalent for `up` drawers.
Test count: 32 → 44.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zernonia
changed the title
Ports BaseUI's `uncontained` drawer demo — an iOS-style action sheet where the popup itself is a `pointer-events: none` flex container that carries the swipe transform but has no visible background, and the visible "surfaces" inside (an action list card and a danger button card) re-enable pointer events and render as independently-rounded floating cards. This was the one BaseUI demo with no equivalent in our story. The other variants are either direct ports (Indent Effect) or reka-flavored versions of BaseUI patterns that cover more API corners (4 Position variants instead of 1; interactive snap-points with toggle buttons). Wire-compatible with BaseUI: - Controlled `v-model:open` with explicit close on button click - Visually-hidden Drawer.Title/Description so screen readers still announce the sheet purpose - Visually-hidden Drawer.Close on the first list item so keyboard and AT users have a reachable close action without a visible Cancel button (the visible buttons just toggle the controlled state) - Popup max-width 28rem, bottom-gap layout with 0.75rem between the action list card and the danger button card Verified live in histoire at 400x800: two separate rounded cards stacked with the danger "Block User" button in its own surface, swipe transform on the popup only affects the outer flex container. 44/44 tests still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Default story and the 4 Position variants all reproduced the same
animation-restart bug that the Snap Points variant had:
Cause 1: CSS \`animation: none\` on \`[data-swiping]\`
During drag, the \`[data-swiping]\` rule set \`animation: none\`,
transitioning animation-name from e.g. \`drawer-slide-bottom-in\` to
\`none\`. When data-swiping was removed on release, animation-name
went BACK to \`drawer-slide-bottom-in\` — which the browser interprets
as a brand-new animation declaration and restarts from keyframe 0.
The drawer visibly bounced to its off-screen start position
(translateY(100%), translateY(-100%), translateX(-100%),
translateX(100%)) and then slid all the way back after every drag
release.
Cause 2: Keyframes animated \`transform\`
The existing keyframes used \`transform: translateY(100%)\` etc, which
is the same CSS property used to apply the drag offset. Even without
cause 1, any keyframe running on top of the drag would clobber the
inline transform for its duration.
Fix (ported from the earlier Snap Points fix):
- Rewrite all 8 slide keyframes to animate the independent \`translate\`
CSS property instead of \`transform\`, so they compose with the inline
\`transform\` on the popup.
- Drop \`animation: none\` from the \`[data-swiping]\` overrides for all
5 position/default variants. The enter animation is inert within
450ms of mount, so there's no reason to suppress it during drag.
We only need to zero transition-duration so the transform transition
doesn't fight the raw pointer delta.
- Delete the duplicate \`<style>\` block from \`_Drawer.vue\` (it had its
own copy of the buggy keyframe + the \`animation: none\` rule). The
Default variant now inherits from \`Drawer.story.vue\`'s global styles
like every other variant, so the fix lives in exactly one place.
Verified with raf-level instrumentation in histoire at 400x800 for
both the Default (bottom) and Position-Top variants:
Default + small downward drag to +20px:
dt=1023 end-of-drag: y=616, transform=translateY(20), tDur=0s
dt=1197 first release frame: y=616, transform=translateY(20),
tDur=0.45s <-- starts exactly where drag ended
dt=1205: translateY(19.15)
dt=1213: translateY(18.23)
...smooth interpolation to translateY(0)
Position-Top + small upward drag:
y=0 -> drag -15px -> release -> settles at translateY(0) cleanly
44/44 tests still passing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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.
Summary
Vue port of BaseUI's Drawer as a standalone primitive (closes #2515). Built on
FocusScope,DismissableLayer,Presence, andTeleportPrimitive— not wrapping Dialog.Parts:
Root,Trigger,Close,Title,Description,Portal,Overlay(aliased asBackdrop),Content(aliased asPopup),Viewport,Handle,SwipeArea,Provider,Indent,IndentBackground.Features
DrawerSwipeAreafor off-screen edge triggers.0-1, absolute pixels,'Npx'strings, or'Nrem'strings. Unknown units are dropped.DrawerViewport.tsx(velocity-boosted target offset withSNAP_VELOCITY_MULTIPLIER=300, close-vs-closest-snap distance comparison, sequential-modeshouldAdvance+ physical-crossing rules).--drawer-swipe-strengthCSS var written on release viacomputeSwipeReleaseScalar(remaining-distance / velocity scalar, clamped to[0.1, 1]) so consumer transitions can scale duration with flick speed.canSwipeFromScrollEdgeOnPendingMove).CSS.registerPropertyfor all high-frequency vars (--drawer-swipe-movement-x/y,--drawer-snap-point-offset,--drawer-swipe-progress,--drawer-swipe-strength).true(focus trap + scroll lock + block outside events),'trap-focus'(focus trap only, outside events pass through),false(non-modal).update:openchange details — carries{ reason: 'trigger-press' | 'close-press' | 'swipe' | 'escape-key' | 'outside-press' }so consumers can distinguish dismiss sources.update:openCompletefires after the transition.initialFocus/finalFocusprops onDrawerContent.Providertracks runningnestedOpenDrawerCount, parent popups subscribe tonestedSwipeProgressStoreto animate in reverse while a child swipes away, and akeepHeightWhileNestedguard prevents opening a child from mutating parent snap geometry.DrawerIndentsubscribes tovisualStateStoreand writes--drawer-swipe-progressimperatively, so consumer CSS can interpolate transforms in sync with the drag (seeindent-providerstory for the canonical--indent-transitionrecipe).DrawerOverlayImplcarriesdata-swiping/data-swipe-directionfor[data-swiping] [data-slot="drawer-backdrop"]CSS patterns.data-swipe-dismissedis set on the popup for consumer styling.Tests
32 tests, all passing:
Drawer.test.ts(11) — render + click + keyboard + a11y integration, plusupdate:openreason assertions for trigger-press and close-press.useDrawerSnapPoints.test.ts(11) — unit tests pinning the BaseUI release math: snap-point resolution, unknown-unitparseSnapPointfilter, close-vs-snap distance decision, velocity-boost dismiss, up-drawer sign convention, sequential-modeshouldAdvance+ physical-crossing rules.utils.test.ts(10) —getDisplacementsign conventions across all 4 directions andcomputeSwipeReleaseScalar(null-below-min-velocity, null-on-zero-remaining,[0.1, 1]range, fast/slow scalar endpoints, up-direction sign,snapPointOffsetcontribution).Stories (Histoire)
Mirror the BaseUI demos: default bottom drawer, 4-side variants, snap points, non-modal, swipe-area (swipe-to-open), nested drawers, mobile nav, scrollable content, form, and the indent-effect recipe.
Test plan
pnpm --filter reka-ui test Drawer— 32 tests passpnpm --filter reka-ui story:dev— open the Drawer stories and walk through each variant🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests