fix(core): use rememberUpdatedState to avoid stale callback capture in WheelPicker#135
Merged
Merged
Conversation
…n WheelPicker Co-authored-by: Junie <junie@jetbrains.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
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
WheelPicker's two long-livedLaunchedEffect+snapshotFlowcollectors capturedonScrollChanged/onScrollFinishedas the lambda instance that was current when the effect started. The effects are keyed onlazyListStateandcount, so when a parent recomposed and passed new callback instances the effects did not restart (correct) but kept invoking the old lambdas (stale-capture bug).Root cause
WheelPicker.kt:onScrollChanged/onScrollFinishedare function types ((Int) -> Unit,(Int) -> Int?), which Compose treats as unstable.WheelTextPicker(public API) forwards them straight through, so any caller passing an un-remembered inline lambda produces a new instance per recomposition — and the running collector keeps calling the one captured at first composition.Fix
Wrap both callbacks with
rememberUpdatedStateand call the wrappers inside the effects:This is the canonical Compose pattern for "long-running effect that should not restart but must invoke the latest handler."
Why
rememberUpdatedState(not keys)?Adding the callbacks as
LaunchedEffectkeys would cancel and re-subscribe thesnapshotFlowon every lambda identity change — wrong, because:lazyListState/count, not callback identity;distinctUntilChanged()state, so the current snapped index is re-emitted to the parent on every lambda identity change.The callbacks are event handlers, not lifecycle identifiers, so
rememberUpdatedStateis the correct tool.Impact
Callers that pass fresh lambdas now have their latest callback invoked on every scroll snap, instead of the one captured at first composition. Concretely:
StandardWheelDatePickerandCJKWheelDatePicker— inline lambdas capturingsnappedDateplus the user-suppliedonSnappedDate/onSnappedDateChanged.StandardWheelTimePickerandAdaptiveWheelDateTimePicker— inline lambdas capturingsnappedTime/snappedDateTimeplus the user-suppliedonSnappedTime/onSnappedDateTimecallbacks.Real-world trigger
When a sibling wheel rewrites
snappedDate/snappedTime/snappedDateTime, the parent recomposes and passes fresh lambdas down toWheelPicker. The effect does not restart —lazyListState(fromrememberLazyListState(startIndex)) is stable acrosssnappedDatechanges, andcountonly changes when the item count actually changes (e.g. Jan→Feb: 31→28 restarts it, but Jan→Mar: 31→31 does not). So for same-count transitions the collector keeps invoking the lambda captured at first composition, which closes over the user-suppliedonSnappedDate/onSnappedDateChanged(or time equivalents) from that point — not the latest one.Note: the library's own
snappedDate/snappedTime/snappedDateTimeare backed bymutableStateOf, so they are read freshly even from a stale lambda. The deriveddayOfMonthslist is plainremember-ed and does go stale, but its content is identical across same-day-count months (values1..Nwith the same index mapping), so the staleness doesn't produce observable drift inside this library. The stale-capture path that actually matters is the user-supplied callback — a plain function type with no snapshot-state guarantee.Test coverage
No regression test added — the repo has no Compose UI test harness today (only pure-Kotlin
commonTestfor locale/numeral logic). A behavioral test would require bootstrappingui-teston a jvm target and carefully drivingsnapshotFlow+ recomposition. Flagged as follow-up rather than blocking this fix.