Scrolling fix#1208
Conversation
Added check to observeElementOffset to prevent continuous re-renders
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA guard is added in ChangesobserveElementOffset rerender fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
🤖 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 782-788: The current code in the scroll offset handling block only
calls maybeNotify() when the offset equals the current scrollOffset, but fails
to notify when the offset actually changes (offset !== this.scrollOffset). This
prevents onChange notifications from being emitted during normal scroll updates.
Add a notification call (maybeNotify()) for the case where the offset differs
from this.scrollOffset to ensure virtual range updates are properly triggered.
This fix should be applied to both occurrences mentioned, including the one
around line 814.
🪄 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: 555ddbcd-1cbe-4be9-8450-226ab25e8ff7
📒 Files selected for processing (2)
.changeset/cold-ads-lose.mdpackages/virtual-core/src/index.ts
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
@dracofulmen Hi, could you clarify what "continuous re-renders" means concretely here, what's measured as re-rendering, and under what interaction?
Basic maybeNotify is memoized on [isScrolling, startIndex, endIndex], so a scroll event at an unchanged offset can't cause a re-render unless isScrolling toggles (the range can't have moved)
|
@piecyk I'm using |
Hmm, that looks off. Could you prepare a reproducible example on stackblitz using the minimal example you mentioned, so we can investigate further? Once we have that, we can dig into why isScrolling remains stuck. |
|
@piecyk Here's the stackblitz reproduction |
@dracofulmen thanks! Could you share what environment you’re seeing this issue in? I tested it on Chrome and Firefox on macOS and it looks good, no flickering on my side. Also, virtualization is not enabled by default. You need to pass shouldVirtualize: true to useSelectModel to enable it. |
|
I'm using it in Firefox on Mac. |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks @dracofulmen, the re-render loop is real. I can reproduce it reliably in Firefox, though not in Chrome, with the virtualized Select.
After scrolling stops, it keeps re-rendering indefinitely, roughly every ~168ms, which lines up with isScrollingResetDelay.
Bit tweaked your fix to ignore a scroll event when both of these are true:
- the event lands on the offset we already have,
- it is not a self-write read-back.
Real scrolls still change the offset, so they pass through normally. Programmatic scrolls are still handled by the _intendedScrollOffset reconciliation below.
I tested it, and the loop is gone while scrolling still works.
One thing I want to be upfront about: I did not isolate exactly why the browser fires that extra scroll event. I know it is browser-generated and happens right after each render, but I have not pinned down the underlying mechanism.
It would be nice to move @workday/canvas-kit-react to directDomUpdates. That should reduce the number of re-renders while scrolling:
https://tanstack.com/virtual/latest/docs/framework/react/react-virtual#directdomupdates
Feel free to create a PR on their side and ping me for review.
Co-authored-by: Damian Pieczynski <piecyk@gmail.com>
Co-authored-by: Damian Pieczynski <piecyk@gmail.com>
Co-authored-by: Damian Pieczynski <piecyk@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/virtual-core/src/index.ts (1)
775-822: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the new no-op scroll branch.
This is a browser-specific change in a hot path, but the provided test context only covers adjacent contracts (
isScrollingdeferral and_intendedScrollOffsetreconciliation), not the newisScrolling && this._intendedScrollOffset === null && offset === this.scrollOffsetearly return. A focused unit test here would lock in the Safari/Firefox fix and protect against future regressions inonChange/isScrollingbehavior.🤖 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 775 - 822, Add a focused regression test around the scroll handling in `packages/virtual-core/src/index.ts` that covers the new early return in the `onChange`/scroll path when `isScrolling` is true, `_intendedScrollOffset` is null, and `offset === this.scrollOffset`. Verify this branch does not re-arm `isScrolling`, does not trigger an extra `maybeNotify`/re-render cycle, and preserves the existing `_intendedScrollOffset` reconciliation and `scheduleScrollReconcile` behavior for nearby cases.
🤖 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.
Nitpick comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 775-822: Add a focused regression test around the scroll handling
in `packages/virtual-core/src/index.ts` that covers the new early return in the
`onChange`/scroll path when `isScrolling` is true, `_intendedScrollOffset` is
null, and `offset === this.scrollOffset`. Verify this branch does not re-arm
`isScrolling`, does not trigger an extra `maybeNotify`/re-render cycle, and
preserves the existing `_intendedScrollOffset` reconciliation and
`scheduleScrollReconcile` behavior for nearby cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 798b27a3-9941-48b4-ac08-08b8be164412
📒 Files selected for processing (1)
packages/virtual-core/src/index.ts
This prevents hiccups during scrolling itself.
This reverts commit 2b4c600.
|
View your CI Pipeline Execution ↗ for commit 6ce9b8c
☁️ Nx Cloud last updated this comment at |
2 participants
Added check to observeElementOffset to prevent continuous re-renders
🎯 Changes
Currently,
observeElementOffsetcausesisScrollingto get stuck as true for as long asoffset > 0. This causes continuous rerenders even after scrolling stops. Thethis.maybeNotify();call also causes continuous rerenders while scrolling. Both of these issues cause the scroll bar to flicker. This fix adds a check to make sure that scrolling only causes notifications once, when the scrolling stops.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit