fix(virtual-core): add touch provenance flag to iOS deferral gate - #1254
Open
waterWang wants to merge 1 commit into
Open
fix(virtual-core): add touch provenance flag to iOS deferral gate#1254waterWang wants to merge 1 commit into
waterWang wants to merge 1 commit into
Conversation
`applyScrollAdjustment` and the anchor-deferral gate guard on `isScrolling`, but `isScrolling` is set by *any* scroll event, including the echo of a programmatic scrollTop write. On iOS this makes every programmatic scrollToIndex/scrollToOffset landing defer its size-change compensation past a paint, producing a visible sag-then-snap (TanStack#1250). Introduce `_isUserScrolling` — set on `touchstart`, kept true while `isScrolling` remains true (covering the whole momentum phase), and cleared when the scroll fully settles. The deferral gates now use `_isUserScrolling` instead of bare `isScrolling`, so touch-initiated sequences defer exactly as before (fixing TanStack#884) while app-initiated scrolls land compensated on their first painted frame. Closes TanStack#1250
📝 WalkthroughWalkthroughThe virtual core now tracks whether scrolling began with a touch gesture. iOS adjustment deferral and anchor synchronization use this state, while programmatic scrolls remain eligible for immediate adjustments. ChangesiOS scroll provenance
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/virtual-core/src/index.ts (1)
712-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both provenance paths.
The supplied
packages/virtual-core/tests/index.test.ts:1922-1939test covers an active touch._iosTouchingalready defers that case. Add tests that verifyscrollToIndexandscrollToOffsetcompensation remains synchronous before paint, and that momentum remains deferred aftertouchend.Also applies to: 976-976
🤖 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 `@packages/virtual-core/src/index.ts` at line 712, Add regression tests in the existing virtual-core test suite covering both compensation provenance paths: verify scrollToIndex and scrollToOffset remain synchronous before paint during active touch, while momentum compensation remains deferred after touchend. Reuse the existing touch/scroll test setup and assert timing separately for _iosTouching and _iosJustTouchEnded behavior.
🤖 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 `@packages/virtual-core/src/index.ts`:
- Around line 881-887: Update the scroll-settling logic around _isUserScrolling
so it is not cleared while _iosTouching or _iosJustTouchEnded is active; clear
it only when the 150 ms timer expires and !this.isScrolling. Extend the
onTouchEnd registration to include touchcancel so cancelled gestures and taps
cannot leave stale touch state.
---
Nitpick comments:
In `@packages/virtual-core/src/index.ts`:
- Line 712: Add regression tests in the existing virtual-core test suite
covering both compensation provenance paths: verify scrollToIndex and
scrollToOffset remain synchronous before paint during active touch, while
momentum compensation remains deferred after touchend. Reuse the existing
touch/scroll test setup and assert timing separately for _iosTouching and
_iosJustTouchEnded behavior.
🪄 Autofix
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: f6154467-0c4a-43cd-b91e-8b8e45ce25fb
📒 Files selected for processing (1)
packages/virtual-core/src/index.ts
Comment on lines
+881
to
+887
| // When the scroll fully settles, touch provenance for the sequence | ||
| // ends: programmatic scrolls never set it, and a user sequence is | ||
| // done once the momentum/subsequent scroll events stop. Clearing it | ||
| // here lets a later app-initiated scroll (which fires its own | ||
| // scroll events) take the synchronous adjustment path as intended | ||
| // (#1250). | ||
| if (!isScrolling) this._isUserScrolling = false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'observeElementOffset|isScrollingResetDelay|touch(start|end|cancel)|_isUserScrolling' \
packages/virtual-core/src packages/virtual-core/testsRepository: TanStack/virtual
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- observeOffset implementation ---'
sed -n '160,225p' packages/virtual-core/src/index.ts
printf '%s\n' '--- lifecycle and flush code ---'
sed -n '820,1035p' packages/virtual-core/src/index.ts
printf '%s\n' '--- relevant tests and test files ---'
rg -n -C 8 \
'_iosTouching|_iosJustTouchEnded|_iosTouchEndTimerId|_isUserScrolling|touchcancel|touchend|isScrollingResetDelay|resizeItem' \
packages/virtual-core/tests packages/virtual-core/src/index.ts \
| head -n 500Repository: TanStack/virtual
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class State:
user: bool = False
touching: bool = False
just_ended: bool = False
scrolling: bool = False
def gated(s: State) -> bool:
return s.user or s.touching or s.just_ended
def existing_tap():
s = State()
s.user = True
s.touching = True
s.touching = False
s.just_ended = True
s.just_ended = False # 150 ms timer
return s, gated(s)
def existing_pause_resume():
s = State()
s.user = True
s.touching = True
s.scrolling = True
s.scrolling = False # debounce callback during touch pause
s.user = False # current reset
s.scrolling = True # resumed movement; no second touchstart
s.touching = False
s.just_ended = True
s.just_ended = False # timer expires while momentum continues
return s, gated(s)
def proposed_tap():
s = State()
s.user = True
s.touching = True
s.touching = False
s.just_ended = True
s.just_ended = False
if not s.scrolling:
s.user = False
return s, gated(s)
def proposed_pause_resume():
s = State()
s.user = True
s.touching = True
s.scrolling = True
s.scrolling = False # guarded reset does not clear while touching
s.scrolling = True
s.touching = False
s.just_ended = True
s.just_ended = False
if not s.scrolling:
s.user = False
return s, gated(s)
for name, probe in [
("existing tap", existing_tap),
("existing pause/resume", existing_pause_resume),
("proposed tap", proposed_tap),
("proposed pause/resume", proposed_pause_resume),
]:
state, blocked = probe()
print(f"{name}: {state}; iOS adjustment deferred={blocked}")
PYRepository: TanStack/virtual
Length of output: 622
Keep _isUserScrolling active until the touch sequence settles.
When isScrolling === false occurs during _iosTouching or _iosJustTouchEnded, do not clear _isUserScrolling. A resumed gesture has no new touchstart; after _iosJustTouchEnded expires, momentum can pass the iOS adjustment gates. Clear the flag from the 150 ms timer only when !this.isScrolling. Register onTouchEnd for touchcancel to prevent taps and cancelled gestures from leaving stale state.
🤖 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 `@packages/virtual-core/src/index.ts` around lines 881 - 887, Update the
scroll-settling logic around _isUserScrolling so it is not cleared while
_iosTouching or _iosJustTouchEnded is active; clear it only when the 150 ms
timer expires and !this.isScrolling. Extend the onTouchEnd registration to
include touchcancel so cancelled gestures and taps cannot leave stale touch
state.
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.
Problem
On iOS WebKit, the iOS scroll-adjustment deferral engages for programmatic scrolls, not just touch-driven ones. The deferral gate is
isScrolling, andisScrollingcarries no provenance —observeOffsetsets it from anyscrollevent, including the ones a programmatic scroll write generates itself.The result is that dynamic-measurement compensation which would normally apply pre-paint is deferred past a paint, so a
scrollToIndex/scrollToOffsetlanding paints at a visibly wrong offset and then snaps into place a beat later.Root Cause
The gate in
applyScrollAdjustment:The
this.isScrollingterm over-captures: it's true for ALL scroll events, including the echo of the app's ownscrollTopwrite. The existing touch-provenance flags (_iosTouching,_iosJustTouchEnded) are already correct — they only fire for real touch events — but they only cover the active-touch and 150ms-post-touchend windows. Momentum continues well past that, andisScrollingis the only flag that stays true through the rest of the fling.Fix
Introduce
_isUserScrolling— a touch-provenance flag that covers the entire user-initiated scroll sequence:touchstart: set_isUserScrolling = trueisScrollingtransitions to false: clear_isUserScrolling(scroll fully settled)_isUserScrollinginstead of bareisScrollingTouch-initiated sequences defer exactly as before (fixing #884). Programmatic scrolls never set
_isUserScrolling, so their adjustments stay on the synchronous pre-paint path — landing compensated on the first painted frame.Fixes #1250
Summary by CodeRabbit