V2 release#19
Merged
Merged
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 (2)
📝 WalkthroughWalkthroughMigrates drawer API from v1 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI / Example)
participant Drawer as Drawer Component
participant Hook as useDrawerSnap
participant View as Viewport/Measurement
participant Parent as Parent State (setActiveSnapPoint)
Client->>Drawer: mount/open with `snapPoints: [...]`
Drawer->>Hook: resolveSnapPointsToHeights(snapPoints, measurement)
Hook->>View: request measurement if `'auto'` present
View-->>Hook: measuredHeight px (or fallback)
Hook-->>Drawer: heights[], rawValues[] (SnapPoint[])
Drawer->>Drawer: animateToHeight(targetHeight)
alt uncontrolled OR notify flag true
Drawer->>Parent: setActiveSnapPoint(rawValue, index)
else parent-controlled (activeSnapPoint present)
Note right of Drawer: suppress notification to avoid feedback loop
end
Parent-->>Drawer: (optional) controlled activeSnapPoint prop update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 25 minutes and 32 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/useDrawerSnap.ts (1)
68-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the raw value as
'auto'in the single-stop fast path.This returns the measured height as the raw snap value, so
getActiveSnapPoint()/restore flows stop treating the active stop as'auto'and start treating it as a fixed pixel snap. In particular,useDrawerKeyboardSnapMobilewill restore a stale px value instead of the auto slot after content size changes.💡 Suggested fix
- return { heights: [resolvedHeight], rawValues: [resolvedHeight] } + return { heights: [resolvedHeight], rawValues: ['auto'] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDrawerSnap.ts` around lines 68 - 78, In the single-stop fast path inside useDrawerSnap where snapPoints is ['auto'], keep the computed pixel value for rendering but preserve the original 'auto' raw snap value so restore flows still treat the stop as auto; specifically, return heights: [resolvedHeight] but set rawValues: ['auto'] (or rawValues: [snapPoints[0]]) instead of rawValues: [resolvedHeight], so getActiveSnapPoint()/useDrawerKeyboardSnapMobile continue to recognize and restore the auto slot after content size changes.src/components/Drawer.tsx (1)
235-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setActiveSnapPointis still drag-only here.Because this call is gated behind
dragMeta,snapTo()/expand()/collapse(),activeSnapPoint-driven resnaps, and the initialdefaultSnapPointopen path never emit the callback. That breaks the documented contract and leaves controlled wrappers/listeners blind to non-drag snap changes.💡 Suggested direction
if (dragMeta && raw !== null) { const dragInfo: DragEndInfo = { y: dragMeta.endY, velocity: dragMeta.velocityY, progress: dragMeta.progress, targetSnapPoint: raw, } onDragEnd?.( new PointerEvent('pointerup') as unknown as PointerEvent, dragInfo, ) - setActiveSnapPoint?.(raw, nextIdx) } + if (raw !== null) { + setActiveSnapPoint?.(raw, nextIdx) + }You'll also want the initial open/default-snap animation to go through the same notifier path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Drawer.tsx` around lines 235 - 248, The code only calls setActiveSnapPoint inside the dragMeta branch (when handling drag end), so programmatic snaps (snapTo/expand/collapse), resnaps driven by activeSnapPoint, and the initial defaultSnapPoint open path never notify external controllers; move or add calls to setActiveSnapPoint(raw, nextIdx) (and the same onDragEnd notifier behavior) into the general snap/animation completion flow rather than only under dragMeta — specifically ensure indexToRawValue(nextIdx) is used to compute raw and then call setActiveSnapPoint(raw, nextIdx) after non-drag snap operations (snapTo, expand, collapse) and also trigger it during the initial default-open path so controlled wrappers receive the callback for all snap changes (refer to symbols: setActiveSnapPoint, onDragEnd, indexToRawValue, snapTo, expand, collapse, defaultSnapPoint, activeSnapPoint).playground/src/PlaygroundApp.tsx (1)
241-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPick an initial snap that actually exists in
snapPoints.
activeSnapPointanddefaultSnapPointboth start at0.3, but this demo only exposes[0.25, 0.5, 0.75]. The drawer resolves that to0.25, so the displayed/logged state starts out disagreeing with the actual stop.💡 Suggested fix
- const [active, setActive] = useState<SnapPoint>(0.3) + const [active, setActive] = useState<SnapPoint>(0.25) ... - defaultSnapPoint={0.3} + defaultSnapPoint={0.25}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/PlaygroundApp.tsx` around lines 241 - 275, The initial active/default snap (active state via active and defaultSnapPoint) is set to 0.3 which does not exist in the Drawer prop snapPoints ([0.25,0.5,0.75]) causing a mismatch; update the initial active state and defaultSnapPoint to one of the real snapPoints (e.g., 0.25 or 0.5) or alternatively add 0.3 to the snapPoints array so active/defaultSnapPoint matches the declared snap points; update the constants using active, setActive, defaultSnapPoint, and the Drawer props (snapPoints, defaultSnapPoint, activeSnapPoint) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MIGRATION.md`:
- Around line 91-98: The example types `active` as the entire hook tuple by
doing `useState<SnapPoint>(0.5)`; instead destructure the state hook so `active`
is the SnapPoint value (e.g., use `[active, setActive] =
useState<SnapPoint>(0.5)`) so that `active` is a SnapPoint when passed to props
like `activeSnapPoint`, and keep `stops: SnapPoint[] = ['auto', 480, 'full']`
unchanged; update occurrences referencing `active` to use the value or setter
names accordingly.
In `@src/hooks/useDrawerSnap.test.ts`:
- Around line 159-163: The test assumes a zero fallback for a single 'auto' snap
but the resolver (resolveSnapPointsToHeights) currently returns a non-zero
fallback; update the test to match the resolver contract by asserting heights[0]
equals the resolver's defined fallback instead of 0—either import the exported
fallback constant from src/hooks/useDrawerSnap.ts (if available) or compute the
expected fallback via a direct call/reference to resolveSnapPointsToHeights and
use that value in the expect; ensure any other tests asserting zero are updated
consistently or, if you prefer to change behavior, modify
resolveSnapPointsToHeights and all related tests together to return zero.
---
Outside diff comments:
In `@playground/src/PlaygroundApp.tsx`:
- Around line 241-275: The initial active/default snap (active state via active
and defaultSnapPoint) is set to 0.3 which does not exist in the Drawer prop
snapPoints ([0.25,0.5,0.75]) causing a mismatch; update the initial active state
and defaultSnapPoint to one of the real snapPoints (e.g., 0.25 or 0.5) or
alternatively add 0.3 to the snapPoints array so active/defaultSnapPoint matches
the declared snap points; update the constants using active, setActive,
defaultSnapPoint, and the Drawer props (snapPoints, defaultSnapPoint,
activeSnapPoint) accordingly.
In `@src/components/Drawer.tsx`:
- Around line 235-248: The code only calls setActiveSnapPoint inside the
dragMeta branch (when handling drag end), so programmatic snaps
(snapTo/expand/collapse), resnaps driven by activeSnapPoint, and the initial
defaultSnapPoint open path never notify external controllers; move or add calls
to setActiveSnapPoint(raw, nextIdx) (and the same onDragEnd notifier behavior)
into the general snap/animation completion flow rather than only under dragMeta
— specifically ensure indexToRawValue(nextIdx) is used to compute raw and then
call setActiveSnapPoint(raw, nextIdx) after non-drag snap operations (snapTo,
expand, collapse) and also trigger it during the initial default-open path so
controlled wrappers receive the callback for all snap changes (refer to symbols:
setActiveSnapPoint, onDragEnd, indexToRawValue, snapTo, expand, collapse,
defaultSnapPoint, activeSnapPoint).
In `@src/hooks/useDrawerSnap.ts`:
- Around line 68-78: In the single-stop fast path inside useDrawerSnap where
snapPoints is ['auto'], keep the computed pixel value for rendering but preserve
the original 'auto' raw snap value so restore flows still treat the stop as
auto; specifically, return heights: [resolvedHeight] but set rawValues: ['auto']
(or rawValues: [snapPoints[0]]) instead of rawValues: [resolvedHeight], so
getActiveSnapPoint()/useDrawerKeyboardSnapMobile continue to recognize and
restore the auto slot after content size 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: d8167158-9152-45c9-a113-29dcc3266fce
📒 Files selected for processing (15)
MIGRATION.mdREADME.mdpackage.jsonplayground/src/PlaygroundApp.tsxsrc/components/Drawer.coverage.test.tsxsrc/components/Drawer.test.tsxsrc/components/Drawer.tsxsrc/components/drawer/DrawerParts.test.tsxsrc/constants.tssrc/hooks/useDrawerKeyboardSnapMobile.tssrc/hooks/useDrawerSnap.hook.test.tsxsrc/hooks/useDrawerSnap.test.tssrc/hooks/useDrawerSnap.tssrc/index.tssrc/types.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Drawer.tsx (1)
291-337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
activeSnapPointduring the initial open.The intro effect always animates to
defaultIndexand emitssetActiveSnapPoint, even when the drawer is controlled. That can briefly open to the wrong stop and push a stale value back upstream before the controlled effect runs.🔧 Suggested fix
- const h = snapHeights[defaultIndex] ?? minSnap + const initialIndex = + activeSnapPoint === undefined + ? defaultIndex + : resolveSnapToIndex(activeSnapPoint) + const h = snapHeights[initialIndex] ?? minSnap @@ - setSnapIndex(defaultIndex) - // Notify controllers of the resolved opening stop. Important when the - // drawer opens uncontrolled or via `defaultSnapPoint`: the parent - // otherwise has no way to know which numerical/raw stop the intro - // animation actually targeted (e.g. a measured 'auto' height) until - // the user drags or hits a ref control. - const introRaw = indexToRawValue(defaultIndex) - if (introRaw !== null) { - setActiveSnapPoint?.(introRaw, defaultIndex) - } + setSnapIndex(initialIndex) + if (activeSnapPoint === undefined) { + const introRaw = indexToRawValue(initialIndex) + if (introRaw !== null) { + setActiveSnapPoint?.(introRaw, initialIndex) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Drawer.tsx` around lines 291 - 337, The intro useEffect currently always targets defaultIndex and calls setActiveSnapPoint, which can override a controlled activeSnapPoint; modify the effect to honor a controlled activeSnapPoint by deriving the intro target from the current activeSnapPoint (convert it to an index via your raw-to-index helper or reverse mapping) when activeSnapPoint is not null/undefined, otherwise fall back to defaultIndex; setSnapIndex, animate, heightMv and updateProgress should use that computed targetIndex and only call setActiveSnapPoint when the component is uncontrolled (i.e., when activeSnapPoint is null), and ensure you skip the intro if the resolved target height from snapHeights is missing or <= 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/Drawer.tsx`:
- Around line 291-337: The intro useEffect currently always targets defaultIndex
and calls setActiveSnapPoint, which can override a controlled activeSnapPoint;
modify the effect to honor a controlled activeSnapPoint by deriving the intro
target from the current activeSnapPoint (convert it to an index via your
raw-to-index helper or reverse mapping) when activeSnapPoint is not
null/undefined, otherwise fall back to defaultIndex; setSnapIndex, animate,
heightMv and updateProgress should use that computed targetIndex and only call
setActiveSnapPoint when the component is uncontrolled (i.e., when
activeSnapPoint is null), and ensure you skip the intro if the resolved target
height from snapHeights is missing or <= 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03783380-f073-47d5-b122-db4547166a66
📒 Files selected for processing (8)
MIGRATION.mdplayground/src/PlaygroundApp.tsxsrc/components/Drawer.coverage.test.tsxsrc/components/Drawer.test.tsxsrc/components/Drawer.tsxsrc/components/drawer/DrawerParts.test.tsxsrc/hooks/useDrawerSnap.test.tssrc/hooks/useDrawerSnap.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/Drawer.coverage.test.tsx
- src/hooks/useDrawerSnap.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Drawer.test.tsx
- src/hooks/useDrawerSnap.test.ts
- playground/src/PlaygroundApp.tsx
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/hooks/useDrawerSnap.ts (1)
89-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDedupe can erase the
'auto'snap identity.When
'auto'resolves to the same pixel height as a fixed stop (or'full'), this keeps only the first raw value and drops'auto'. DownstreamindexToRawValue()/snap-index remapping then treat that slot as the fixed stop, so later content-size changes stop tracking the intrinsic-height snap. Preserving'auto'on equal-height collisions, or storing aliases instead of a single raw value, would avoid that contract break.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDrawerSnap.ts` around lines 89 - 100, The current dedupe loop in useDrawerSnap.ts (building pairs from snapPoints via resolveSnapValueToPx and then populating heights and rawValues) removes the 'auto' raw value when its resolved px equals a fixed stop, which breaks indexToRawValue/mapping; change the logic so equal-px collisions preserve 'auto' (or store aliases) instead of discarding it—e.g., when encountering a px already in heights, if p.raw is the intrinsic 'auto' value append or prefer it in rawValues for that px (or switch to a structure that stores an array of raw values per px and adjust indexToRawValue accordingly) so the identity of 'auto' is retained for downstream remapping.
🤖 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/hooks/useDrawerSnap.ts`:
- Around line 57-60: The hook useDrawerSnap should not return an empty
heights/rawValues set for snapPoints === [] because Drawer.tsx can end up with
open === true but no reachable height; update useDrawerSnap to either
throw/console.warn when snapPoints is an empty array (reject [] up front) or
normalize an empty snapPoints to a sane default (e.g., a single minimum stop)
before returning so heights/rawValues are never empty; locate the early-return
in useDrawerSnap that checks snapPoints.length === 0 and replace it with
normalization or an explicit error path and ensure Drawer.tsx still receives a
non-empty heights array when open is true.
In `@src/types.ts`:
- Around line 58-65: Update the JSDoc for setActiveSnapPoint to accurately
reflect when the callback is invoked: state that setActiveSnapPoint (signature:
setActiveSnapPoint?: (point: SnapPoint, index: number) => void) is called for
user/requested snap changes (drag, ref controls, defaultSnapPoint resolution, or
programmatic requests initiated by this component) but is deliberately NOT
invoked for parent-driven controlled updates to activeSnapPoint; replace wording
that claims it fires on arbitrary programmatic/parent updates with this
clarified controlled-state contract.
---
Outside diff comments:
In `@src/hooks/useDrawerSnap.ts`:
- Around line 89-100: The current dedupe loop in useDrawerSnap.ts (building
pairs from snapPoints via resolveSnapValueToPx and then populating heights and
rawValues) removes the 'auto' raw value when its resolved px equals a fixed
stop, which breaks indexToRawValue/mapping; change the logic so equal-px
collisions preserve 'auto' (or store aliases) instead of discarding it—e.g.,
when encountering a px already in heights, if p.raw is the intrinsic 'auto'
value append or prefer it in rawValues for that px (or switch to a structure
that stores an array of raw values per px and adjust indexToRawValue
accordingly) so the identity of 'auto' is retained for downstream remapping.
🪄 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: 096ccb84-9065-432f-a5a3-b0e3d739da67
📒 Files selected for processing (3)
src/components/Drawer.tsxsrc/hooks/useDrawerSnap.tssrc/types.ts
Comment on lines
+57
to
+60
| // Empty array: caller passed `snapPoints={[]}` (rare but legal). Keep the | ||
| // hook output empty so the Drawer renders nothing snappable. | ||
| if (snapPoints.length === 0) { | ||
| return { heights: [], rawValues: [] } |
There was a problem hiding this comment.
Empty snapPoints currently opens a backdrop-only drawer.
Returning an empty snap set here leaves Drawer.tsx with open === true but no reachable height, so the overlay/body lock/focus handling still mount while the panel stays at 0px. In modal mode that effectively traps the page behind an invisible drawer. Please either reject [] up front or normalize it to a real stop before the component renders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useDrawerSnap.ts` around lines 57 - 60, The hook useDrawerSnap
should not return an empty heights/rawValues set for snapPoints === [] because
Drawer.tsx can end up with open === true but no reachable height; update
useDrawerSnap to either throw/console.warn when snapPoints is an empty array
(reject [] up front) or normalize an empty snapPoints to a sane default (e.g., a
single minimum stop) before returning so heights/rawValues are never empty;
locate the early-return in useDrawerSnap that checks snapPoints.length === 0 and
replace it with normalization or an explicit error path and ensure Drawer.tsx
still receives a non-empty heights array when open is true.
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 by CodeRabbit
Breaking Changes
sizingprop →snapPointsarray;SnapPointValue→SnapPointonSnapPointChange→ controlled APIsetActiveSnapPoint(point, index)DRAWER_SIZINGremoved; useSNAP_POINTtokens;SNAP_POINT.FULLis token'full'(previous 0.9 →NEAR_FULL)New Features
defaultOpenhandleOnly,fadeFromIndex,snapToSequentialPoint,nestedDocumentation