Scrolling fix

[dracofulmen]

Added check to observeElementOffset to prevent continuous re-renders

🎯 Changes

Currently, observeElementOffset causes isScrolling to get stuck as true for as long as offset > 0. This causes continuous rerenders even after scrolling stops. The this.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

• [✅] I have followed the steps in the Contributing guide.
• [✅] I have tested this code locally with pnpm run test:pr.

🚀 Release Impact

• [✅] This change affects published code, and I have generated a changeset.
• This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

• Bug Fixes
  • Fixed continuous re-renders that occurred during scrolling operations with virtualized content, resulting in improved performance and smoother interactions.

[coderabbitai]

📝 Walkthrough

Walkthrough

A guard is added in Virtualizer._willUpdate's observeElementOffset callback: when the observed offset matches the current scrollOffset while isScrolling is true, it resets isScrolling, updates scrollOffset, and calls maybeNotify() to break rerender loops. The previously unconditional trailing maybeNotify() call is also removed.

Changes

observeElementOffset Rerender Fix

Layer / File(s)	Summary
Scroll offset guard and notification control packages/virtual-core/src/index.ts, .changeset/cold-ads-lose.md	Adds an early-exit guard in the scroll callback that detects when the browser reads back the same scroll position during an active scroll (offset === scrollOffset && isScrolling), resets isScrolling to false, and calls maybeNotify() to halt rerender loops. Removes the previously unconditional trailing this.maybeNotify() call from the same callback path. Changeset documents the patch release.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Suggested reviewers

• tannerlinsley

Poem

🐇 Hop, hop, the scroll spun round,
Rerenders echoing, never unwound.
A guard was placed, a check so neat—
"Same offset twice? I won't repeat!"
Now the virtualizer rests with grace,
No endless loops, just smooth white space. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Scrolling fix' is vague and generic, lacking specificity about the actual problem being solved (continuous re-renders from observeElementOffset).	Consider a more descriptive title such as 'Fix continuous re-renders in observeElementOffset' or 'Prevent scroll flickering by fixing observeElementOffset callback'.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description follows the required template structure with all major sections completed: Changes, Checklist, and Release Impact, providing clear context about the problem and solution.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 75ae896 and 4a61b19.

📒 Files selected for processing (2)

• .changeset/cold-ads-lose.md
• packages/virtual-core/src/index.ts

[piecyk]

@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)

[dracofulmen]

@piecyk I'm using @workday/canvas-kit-react v15.0.14. Here's the video of the issue: Reproduction.mov. And here's the code that I used to reproduce it: minimal.tsx. When I dove into the react devtools, I found that isScrolling gets stuck on true as long as the scroll bar isn't at the top. This is why I added the offset === this.scrollOffset && isScrolling === true check. However, without moving the this.maybeNotify() call, I still got the flashes during scrolling.

[piecyk]

@piecyk I'm using @workday/canvas-kit-react v15.0.14. Here's the video of the issue: Reproduction.mov. And here's the code that I used to reproduce it: minimal.tsx. When I dove into the react devtools, I found that isScrolling gets stuck on true as long as the scroll bar isn't at the top. This is why I added the offset === this.scrollOffset && isScrolling === true check. However, without moving the this.maybeNotify() call, I still got the flashes during scrolling.

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.

[dracofulmen]

@piecyk Here's the stackblitz reproduction