fix: prevent carousel touch loss during pagination#1127
Conversation
…conflicting draggable modifier
📝 WalkthroughWalkthroughReplaced Compose Foundation draggable-based horizontal handling in LibraryCarouselPane with a custom pointerInput implementation that tracks mouse press/move/release, applies a pixel slop before dispatching horizontal deltas to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt (1)
99-136:⚠️ Potential issue | 🔴 CriticalHandle
PointerEventType.Exitand remove unnecessarylaunchwrapper ondispatchRawDelta.The code does not clear
isDraggingwhen the pointer exits the region. If the user drags, then moves the mouse away from the carousel while still holding the button,isDraggingremainstrueand subsequentMoveevents process with stalelastDragX, causing unintended scroll deltas. Additionally,dispatchRawDeltais a non-suspend function, so wrapping it inlaunch {}is incorrect and unnecessary—call it directly in thecoroutineScope.Also address the mismatch:
isMouseEventchecks if any change is a mouse, butfirstOrNull()?.positionmay read a different pointer type. Extract the mouse change directly and read its position.Suggested fix
- val isMouseEvent = event.changes.any { it.type == PointerType.Mouse } + val mouseChange = event.changes.firstOrNull { it.type == PointerType.Mouse } when (event.type) { PointerEventType.Scroll -> { val scrollDelta = event.changes.firstOrNull()?.scrollDelta @@ -116,11 +116,11 @@ PointerEventType.Press -> { - if (isMouseEvent) { - lastDragX = event.changes.firstOrNull()?.position?.x ?: 0f + if (mouseChange?.pressed == true) { + lastDragX = mouseChange.position.x isDragging = true } } PointerEventType.Move -> { - if (isDragging && isMouseEvent) { - val currentX = event.changes.firstOrNull()?.position?.x ?: lastDragX + if (isDragging && mouseChange != null) { + val currentX = mouseChange.position.x val delta = currentX - lastDragX lastDragX = currentX if (delta != 0f) { - launch { listState.dispatchRawDelta(-delta) } + listState.dispatchRawDelta(-delta) } } } @@ -134,6 +134,9 @@ PointerEventType.Release -> { isDragging = false } + PointerEventType.Exit -> { + isDragging = false + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt` around lines 99 - 136, Handle PointerEventType.Exit by setting isDragging = false to avoid stale drag state; stop wrapping dispatchRawDelta in launch (call listState.dispatchRawDelta(...) directly since it’s non-suspending). Also replace uses of event.changes.firstOrNull() with a dedicated mouseChange = event.changes.firstOrNull { it.type == PointerType.Mouse } and read mouseChange.position/scrollDelta so you only read positions from the actual mouse pointer; apply this to the Scroll and Move branches and use dominantDelta from mouseChange.scrollDelta.
🤖 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
`@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt`:
- Around line 99-136: Handle PointerEventType.Exit by setting isDragging = false
to avoid stale drag state; stop wrapping dispatchRawDelta in launch (call
listState.dispatchRawDelta(...) directly since it’s non-suspending). Also
replace uses of event.changes.firstOrNull() with a dedicated mouseChange =
event.changes.firstOrNull { it.type == PointerType.Mouse } and read
mouseChange.position/scrollDelta so you only read positions from the actual
mouse pointer; apply this to the Scroll and Move branches and use dominantDelta
from mouseChange.scrollDelta.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dbb63e2-4b15-4656-aada-ac9712746867
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt:129">
P2: Per-move coroutine launching in drag handling adds avoidable scheduling overhead in a high-frequency input loop and can cause less responsive/jittery scrolling.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt
Outdated
Show resolved
Hide resolved
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
`@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt`:
- Around line 116-130: The drag slop is missing: change the press/drag logic in
the PointerEventType.Press/Move handling so a separate "pressed" state (e.g.,
pressedStartX or isPressed) records the initial press X but does not set
isDragging immediately; on PointerEventType.Move compute horizontal distance
from pressedStartX and only set isDragging = true and set lastDragX when the
absolute delta exceeds a small threshold (e.g., a few pixels), and until
promoted ignore small moves (do not call listState.dispatchRawDelta). Use the
existing symbols mouseChange, lastDragX, isDragging, listState.dispatchRawDelta,
and promote the pressed state to active drag once threshold is crossed, updating
lastDragX to the currentX at that moment.
🪄 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: 4073f487-e991-4137-bf7d-367e40951c96
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt (1)
144-147: Mouse exit may leave carousel mid-scroll without snapping.When the mouse exits the carousel during a drag (
PointerEventType.Exit), the drag state resets butdispatchRawDeltaleaves the scroll position wherever it stopped. TheSnapFlingBehavioronLazyRowonly triggers on fling gestures, not on abrupt drag termination.Consider triggering a programmatic scroll-to-nearest-item on exit to maintain the carousel's snapped appearance, or let the existing
flingBehaviorhandle it by not resetting onExit(only onRelease).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt` around lines 144 - 147, PointerEventType.Exit currently clears isDragging and pressedStartX but leaves the scroll mid-position because dispatchRawDelta doesn't snap; update the pointer handling so Exit either doesn't reset the drag state (only reset on PointerEventType.Release) or, when handling PointerEventType.Exit, launch a coroutine that computes the nearest item from lazyListState (using firstVisibleItemIndex and firstVisibleItemScrollOffset) and programmatically scrolls/animates to that index (e.g., animateScrollToItem / scrollToItem) to enforce snapping; reference the onPointerEvent branch handling PointerEventType.Exit, isDragging, pressedStartX, dispatchRawDelta, SnapFlingBehavior, and lazyListState to locate where to add the coroutine-driven snap or change the reset behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt`:
- Around line 144-147: PointerEventType.Exit currently clears isDragging and
pressedStartX but leaves the scroll mid-position because dispatchRawDelta
doesn't snap; update the pointer handling so Exit either doesn't reset the drag
state (only reset on PointerEventType.Release) or, when handling
PointerEventType.Exit, launch a coroutine that computes the nearest item from
lazyListState (using firstVisibleItemIndex and firstVisibleItemScrollOffset) and
programmatically scrolls/animates to that index (e.g., animateScrollToItem /
scrollToItem) to enforce snapping; reference the onPointerEvent branch handling
PointerEventType.Exit, isDragging, pressedStartX, dispatchRawDelta,
SnapFlingBehavior, and lazyListState to locate where to add the coroutine-driven
snap or change the reset behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0bc62a3-d1f3-488f-9d4b-81ae833f484f
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryCarouselPane.kt
3 participants
Description
when you scroll to the end and pagination fires (new items appended, triggering recomposition), the
draggablemodifier's drag session gets reset, dropping the in-flight touch gesture.Fix: Touch events now flow exclusively through
LazyRow's own native scrolling, which is stable across pagination recomps.Discord report: https://discord.com/channels/1378308569287622737/1490021951224287443
Recording
screen-20260407-082626.mp4
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by CodeRabbit