fix: centre without a viewport and keep the selected value - #22
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesDate picker selection stability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReactNative
participant DateDrumPicker
participant DrumPickerNative
participant NativeWheel
ReactNative->>DateDrumPicker: provide controlled day, month, year
DateDrumPicker->>DrumPickerNative: pass resolved column items and selectedIndex
DrumPickerNative->>NativeWheel: apply selection after layout
NativeWheel-->>DrumPickerNative: report user-driven selection
DrumPickerNative-->>DateDrumPicker: emit onChange for user interaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
React Native never runs an Android layout pass for native children: ReactViewGroup.requestLayout() and ReactViewGroup.onLayout() are both empty, so scrollToPositionWithOffset only parked an anchor nobody consumed. Combined with centering scheduled through nested post() calls off a height that was still 0, a picker mounted in a collapsed container stayed on index 0 with only its centre row drawn. Centering is now layout-driven and drives the RecyclerView's measure/layout itself. selectedIndex was also clamped against the items list it was replacing. Fabric hands props over as an unordered map, so a range change permanently moved the selection and emitted onChange for a value the user never picked. The raw requested index is kept and re-resolved against the new list, prop application order is fixed, and change events are suppressed while props are applied. Inside a Modal the same layout fix applies — DialogRootViewGroup only reports its size asynchronously — and gestures are handed over via NativeGestureUtil, since that root discards requestDisallowInterceptTouchEvent. iOS gets the same re-resolution plus a layoutSubviews reassert. Android instrumented tests and the iOS test additions were not executed; see CHANGELOG for what was and was not verified. BREAKING CHANGE: DateDrumPickerValue now requires day, month and year; use DateDrumPickerPartialValue where an incomplete date is intentional.
![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
🧹 Nitpick comments (4)
ios/DrumPickerTests/DrumPickerWheelViewTests.swift (1)
150-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover user-driven and circular re-resolution paths.
These cases only use
setSelectedIndex; add list-swap regressions afterpickerView(_:didSelectRow:inComponent:)and circular recentering. Also run the iOS test target before merge—the PR notes it was not executed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/DrumPickerTests/DrumPickerWheelViewTests.swift` around lines 150 - 201, Extend the DrumPicker wheel tests to cover selection preservation when items are replaced after pickerView(_:didSelectRow:inComponent:) updates the selection, and when circular recentering re-resolves the selected value. Add regressions for both list-swap paths alongside the existing setSelectedIndex tests, then run the iOS test target and confirm it passes.src/DrumPicker.native.tsx (1)
244-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueZero-height warning can fire for the collapsed-mount case this PR explicitly supports.
A picker mounted inside a closed accordion/collapsed container for longer than 1.5s is a documented, supported scenario (
DateDrumPickervalueJSDoc,src/DateDrumPicker.tsxlines 63-67), yet it will log this warning until the container expands. Consider skipping the warning while height is 0 and the view has never received a layout event at all, or downgrading the wording to "not measured yet".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DrumPicker.native.tsx` around lines 244 - 269, Update the zero-height warning logic around hasMeasuredRef and handleLayout so a picker that has never received any layout event, including one mounted in a collapsed container, does not emit the “will never be visible” warning. Track whether layout has occurred separately from whether its height is positive, and only warn for a measured view that remains zero-height; preserve the existing cleanup and development-only behavior.android/src/androidTest/java/com/drumpicker/DrumPickerRegressionTest.kt (1)
53-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests bypass
DrumPickerViewManager.updateProperties, so the new prop ordering is untested.Each test calls
setItemsProp/setSelectedIndexPropdirectly in a hand-chosen order, which validatesDrumPickerViewbut not theORDERED_PROPStwo-phase application that this PR added to guard against Fabric's unordered map. A small unit test that feeds aReactStylesDiffMapwithselectedIndexand a shrinkingitemsand asserts the resulting selection would close that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/androidTest/java/com/drumpicker/DrumPickerRegressionTest.kt` around lines 53 - 116, The regression tests currently call DrumPickerView setters directly and do not exercise DrumPickerViewManager.updateProperties or its ORDERED_PROPS two-phase ordering. Add a focused manager/unit test that applies a ReactStylesDiffMap containing selectedIndex and a shrinking items collection, then assert the final selection is valid and matches the ordered application behavior. Keep the existing view-level tests unchanged.src/__tests__/DateDrumPickerControlled.test.tsx (1)
19-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
readColumnsderives month from the raw index while day/year read labels.
month: monthPicker!.props.selectedIndex + 1silently assumes the month column always starts at January. It holds for these fixtures (minDatemonth is 1), but any future case whereminDate/maxDatenarrows the month range in the selected year will make this read wrong rather than fail loudly. Reading the label keeps all three columns consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/DateDrumPickerControlled.test.tsx` around lines 19 - 29, Update readColumns so the month value is derived from the selected monthPicker item via the existing label helper, matching the day and year columns. Remove the raw selectedIndex + 1 assumption while preserving the returned one-based month value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/src/main/java/com/drumpicker/DrumPickerView.kt`:
- Around line 657-693: Update the animated path in centerOnSelectedIndex so
suppressChangeEvent cannot remain latched when smoothScrollToPosition does not
produce a SCROLL_STATE_IDLE transition. After determining the current centered
index, clear the flag when the target is already centered or otherwise ensure
SCROLL_STATE_DRAGGING resets it before user interaction; preserve suppression
for genuine programmatic animations until completion.
In `@example/src/App.tsx`:
- Around line 77-84: Update the booking date initialization around bookingDate
and bookingMinMax to derive both values from one stable today snapshot captured
during initial setup. Reuse that shared Date value for the day, month, year, and
booking bounds so the controlled value and minDate remain consistent across
midnight.
---
Nitpick comments:
In `@android/src/androidTest/java/com/drumpicker/DrumPickerRegressionTest.kt`:
- Around line 53-116: The regression tests currently call DrumPickerView setters
directly and do not exercise DrumPickerViewManager.updateProperties or its
ORDERED_PROPS two-phase ordering. Add a focused manager/unit test that applies a
ReactStylesDiffMap containing selectedIndex and a shrinking items collection,
then assert the final selection is valid and matches the ordered application
behavior. Keep the existing view-level tests unchanged.
In `@ios/DrumPickerTests/DrumPickerWheelViewTests.swift`:
- Around line 150-201: Extend the DrumPicker wheel tests to cover selection
preservation when items are replaced after
pickerView(_:didSelectRow:inComponent:) updates the selection, and when circular
recentering re-resolves the selected value. Add regressions for both list-swap
paths alongside the existing setSelectedIndex tests, then run the iOS test
target and confirm it passes.
In `@src/__tests__/DateDrumPickerControlled.test.tsx`:
- Around line 19-29: Update readColumns so the month value is derived from the
selected monthPicker item via the existing label helper, matching the day and
year columns. Remove the raw selectedIndex + 1 assumption while preserving the
returned one-based month value.
In `@src/DrumPicker.native.tsx`:
- Around line 244-269: Update the zero-height warning logic around
hasMeasuredRef and handleLayout so a picker that has never received any layout
event, including one mounted in a collapsed container, does not emit the “will
never be visible” warning. Track whether layout has occurred separately from
whether its height is positive, and only warn for a measured view that remains
zero-height; preserve the existing cleanup and development-only behavior.
🪄 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: fc369ba0-dc7c-4437-8bb3-539cf0e7d9d5
⛔ Files ignored due to path filters (2)
src/__tests__/snapshots/__snapshots__/DateDrumPicker.snapshot.test.tsx.snapis excluded by!**/*.snapsrc/__tests__/snapshots/__snapshots__/DrumPicker.snapshot.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdandroid/src/androidTest/AndroidManifest.xmlandroid/src/androidTest/java/com/drumpicker/CollapsedTestActivity.ktandroid/src/androidTest/java/com/drumpicker/DrumPickerRegressionTest.ktandroid/src/main/java/com/drumpicker/DrumPickerView.ktandroid/src/main/java/com/drumpicker/DrumPickerViewManager.ktexample/src/App.tsxios/DrumPickerTests/DrumPickerWheelViewTests.swiftios/DrumPickerView.swiftpackage.jsonsrc/DateDrumPicker.tsxsrc/DrumPicker.native.tsxsrc/__tests__/DateDrumPickerControlled.test.tsxsrc/dateDrumPickerLogic.tssrc/index.tsx
…mprove centering logic
1 participant
React Native never runs an Android layout pass for native children: ReactViewGroup.requestLayout() and ReactViewGroup.onLayout() are both empty, so scrollToPositionWithOffset only parked an anchor nobody consumed. Combined with centering scheduled through nested post() calls off a height that was still 0, a picker mounted in a collapsed container stayed on index 0 with only its centre row drawn. Centering is now layout-driven and drives the RecyclerView's measure/layout itself.
selectedIndex was also clamped against the items list it was replacing. Fabric hands props over as an unordered map, so a range change permanently moved the selection and emitted onChange for a value the user never picked. The raw requested index is kept and re-resolved against the new list, prop application order is fixed, and change events are suppressed while props are applied.
Inside a Modal the same layout fix applies — DialogRootViewGroup only reports its size asynchronously — and gestures are handed over via NativeGestureUtil, since that root discards requestDisallowInterceptTouchEvent.
iOS gets the same re-resolution plus a layoutSubviews reassert.
Android instrumented tests and the iOS test additions were not executed; see CHANGELOG for what was and was not verified.
BREAKING CHANGE: DateDrumPickerValue now requires day, month and year; use DateDrumPickerPartialValue where an incomplete date is intentional.
Summary
What does this PR change and why?
Type of change
Checklist
yarn lint,yarn build,yarn typecheck, andyarn testfrom the repo rootyarn.lockifpackage.jsondependencies changedsrc/,android/, orios/(example app only)CI
PRs to
mainrun the full CI workflow automatically. New tests in standard locations are picked up without editingci.yml.Screenshots / recordings (UI changes)
If applicable, add before/after visuals.
Related issues
Fixes # (issue number)
Summary by CodeRabbit
New Features
Bug Fixes
onChangefrom firing during programmatic updates.Documentation