Add 'auto' and 'full' in sizing array #18
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds token-based snap points ('auto', 'full') with live content-height measurement and hysteresis; remaps snap indices when raw snap ordering changes; stabilizes resnap animations to avoid tiny repeated restarts; refactors tailwind-merge lazy init and cn fallback; extends types and tests; adds playground demo and updates Vite script references. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant RO as ResizeObserver
participant Snap as useDrawerSnap
participant Drawer as Drawer
participant Anim as AnimationEngine
App->>Drawer: open drawer with 'auto' snap
Drawer->>Snap: resolveSnapValueToPx('auto', availableH, measuredH)
Snap-->>Drawer: targetHeight
Drawer->>Anim: animate(currentH, targetHeight)
Anim-->>Drawer: animationComplete
Drawer->>Drawer: store lastRawSnap & lastTarget
App->>RO: content changes (loading → taller)
RO->>Snap: report new measuredHeight
Snap->>Snap: apply ≤1px hysteresis
alt height change > threshold
Snap->>Drawer: new targetHeight
Drawer->>Anim: animate(currentH, newTargetHeight)
Anim-->>Drawer: onAnimationComplete
else height change ≲ threshold
Note over Drawer: skip redundant animation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 1/3 review remaining, refill in 39 minutes and 26 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useDrawerSnap.ts (1)
381-409:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the active raw snap when mixed arrays reorder.
These lines make mixed sizing arrays recompute and re-sort as
measuredAutoHeightchanges. The new test insrc/hooks/useDrawerSnap.test.tsat Line 209 already shows['auto', 480]changing from[100, 480]to[480, 600].src/components/Drawer.tsxstill keeps the active stop assnapIndex, so a drawer parked on'auto'can silently switch to480once AUTO grows past it. The same problem affects controlledactiveSnapPoint="auto"because the prop value does not change, so the index is never re-resolved. Please preserve the active raw snap value and remap the index after recomputingheights/rawValues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDrawerSnap.ts` around lines 381 - 409, When recomputing sizing with resolveSizingToHeights (triggered by measuredAutoHeight changes) capture the current active raw snap (e.g. prevRaw = rawValues[snapIndex] or if controlled use activeSnapPoint) before calling resolveSizingToHeights, then after you get the new {heights, rawValues} remap the active index by locating the same raw value in the new rawValues (use strict equality for numbers and the 'auto' token) and update the component's snap index/state accordingly so a drawer parked on 'auto' preserves its logical stop instead of silently switching when measuredAutoHeight changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playground/src/PlaygroundApp.tsx`:
- Around line 398-419: The demo's default snap is inconsistent with its text:
currently the Drawer is configured with sizing={[SNAP_POINT.AUTO, 480,
DRAWER_SIZING.FULL]} and defaultSnapPoint={SNAP_POINT.AUTO} but the copy and
helper text claim it opens at 480px; either change defaultSnapPoint to the 480
stop (e.g., use the numeric snap or SNAP_POINT value representing 480) so the
behavior matches the text, or update the inline comment/title/helper paragraph
to state that the panel opens at AUTO (content-fit) instead of 480px; locate and
edit the defaultSnapPoint prop and the nearby descriptive strings (title and
paragraph) in PlaygroundApp.tsx to make them consistent.
---
Outside diff comments:
In `@src/hooks/useDrawerSnap.ts`:
- Around line 381-409: When recomputing sizing with resolveSizingToHeights
(triggered by measuredAutoHeight changes) capture the current active raw snap
(e.g. prevRaw = rawValues[snapIndex] or if controlled use activeSnapPoint)
before calling resolveSizingToHeights, then after you get the new {heights,
rawValues} remap the active index by locating the same raw value in the new
rawValues (use strict equality for numbers and the 'auto' token) and update the
component's snap index/state accordingly so a drawer parked on 'auto' preserves
its logical stop instead of silently switching when measuredAutoHeight changes.
🪄 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 Plus
Run ID: 4a79d9c9-5097-46eb-b98b-62ab79fbc87d
📒 Files selected for processing (10)
package.jsonplayground/src/PlaygroundApp.tsxplayground/vite.config.mtssrc/components/Drawer.tsxsrc/constants.tssrc/hooks/useDrawerKeyboardSnapMobile.tssrc/hooks/useDrawerSnap.test.tssrc/hooks/useDrawerSnap.tssrc/types.tssrc/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/Drawer.tsx`:
- Around line 358-379: The problem is that the "lockstep" useEffect writes
lastRawSnapRef from the current snapIndex before the remap effect can read the
previous logical raw value, so reordering can lose the prior identity; fix by
tracking the previous rawSnapValues snapshot in a ref (e.g.,
prevRawSnapValuesRef) and derive the previous raw from that snapshot (compute
prevRaw = prevRawSnapValuesRef.current?.[snapIndex] or via indexToRawValue
against the prior array) inside the remap effect so remapping uses the true
previous raw, then update prevRawSnapValuesRef (and only then update
lastRawSnapRef) so the two effects no longer clobber each other (update
references: lastRawSnapRef, rawSnapValues, snapIndex, and the two useEffect
blocks).
🪄 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 Plus
Run ID: 9f79622d-6105-4e13-8834-31f6e071375d
📒 Files selected for processing (2)
playground/src/PlaygroundApp.tsxsrc/components/Drawer.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- playground/src/PlaygroundApp.tsx
Summary by CodeRabbit
New Features
Bug Fixes / Stability
Tests
Chores