docs(vue-virtual): use onUpdated to prevent scroll jumping#1125
docs(vue-virtual): use onUpdated to prevent scroll jumping#1125piecyk merged 4 commits intoTanStack:mainfrom
onUpdated to prevent scroll jumping#1125Conversation
|
Mini-ghost
force-pushed
the
docs/vue-dynamic-measure-timing
branch
from
ecdfd90 to
893ad75
Compare
|
View your CI Pipeline Execution ↗ for commit 9717dcb
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull request overview
Updates the Vue “dynamic” virtualizer examples to re-measure items after Vue DOM updates (using onUpdated) to address scroll stutter/jump issues reported in the Vue dynamic height demos.
Changes:
- Replaced per-item ref callback measurement with collecting item elements via a template ref array.
- Added
measureAll()and invoked it fromonMountedandonUpdatedto measure rendered items post-render. - Adjusted Vue imports accordingly across the affected example components.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| examples/vue/dynamic/src/components/RowVirtualizerDynamicWindow.vue | Switches to batched measuring via template refs + onUpdated for window-scrolling dynamic rows. |
| examples/vue/dynamic/src/components/RowVirtualizerDynamic.vue | Switches to batched measuring via template refs + onUpdated for container-scrolling dynamic rows. |
| examples/vue/dynamic/src/components/GridVirtualizerDynamic.vue | Switches to batched measuring via template refs + onUpdated for dynamic grid row measurement. |
| examples/vue/dynamic/src/components/ColumnVirtualizerDynamic.vue | Switches to batched measuring via template refs + onUpdated for dynamic columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function measureAll() { | ||
| virtualItemEls.value.forEach((el) => { | ||
| if (el) rowVirtualizer.value.measureElement(el) | ||
| }) | ||
| } |
There was a problem hiding this comment.
measureAll never calls rowVirtualizer.value.measureElement(null). In @tanstack/virtual-core, passing null is what triggers cleanup of disconnected nodes (unobserving/removing stale entries from elementsCache). Without that, items that scroll out/unmount can remain observed/retained. Consider calling measureElement(null) once per update (or using a callback ref that forwards null on unmount) in addition to measuring the current elements.
| if (!el) { | ||
| return | ||
| } | ||
| const virtualItemEls = shallowRef([]) |
There was a problem hiding this comment.
shallowRef([]) with an empty array literal infers never[] in TypeScript, which can make el in forEach be never and cause type errors when passing it to measureElement. Consider explicitly typing virtualItemEls (e.g. as an array of HTMLElement | null).
| const virtualItemEls = shallowRef([]) | |
| const virtualItemEls = shallowRef<(HTMLElement | null)[]>([]) |
| function measureAll() { | ||
| virtualItemEls.value.forEach((el) => { | ||
| if (el) rowVirtualizer.value.measureElement(el) | ||
| }) | ||
| } |
There was a problem hiding this comment.
measureAll never calls rowVirtualizer.value.measureElement(null). In @tanstack/virtual-core, passing null is what triggers cleanup of disconnected nodes (unobserving/removing stale entries from elementsCache). Without that, items that scroll out/unmount can remain observed/retained. Consider calling measureElement(null) once per update (or using a callback ref that forwards null on unmount) in addition to measuring the current elements.
| } | ||
|
|
||
| rowVirtualizer.value.measureElement(el) | ||
| const virtualItemEls = shallowRef([]) |
There was a problem hiding this comment.
shallowRef([]) with an empty array literal infers never[] in TypeScript, which can make el in forEach be never and cause type errors when passing it to measureElement. Consider explicitly typing virtualItemEls (e.g. as an array of HTMLElement | null).
| const virtualItemEls = shallowRef([]) | |
| const virtualItemEls = shallowRef<(HTMLElement | null)[]>([]) |
| function measureAll() { | ||
| virtualItemEls.value.forEach((el) => { | ||
| if (el) rowVirtualizer.value.measureElement(el) | ||
| }) | ||
| } |
There was a problem hiding this comment.
measureAll never calls rowVirtualizer.value.measureElement(null). In @tanstack/virtual-core, passing null is what triggers cleanup of disconnected nodes (unobserving/removing stale entries from elementsCache). Without that, rows that scroll out/unmount can remain observed/retained. Consider calling measureElement(null) once per update (or using a callback ref that forwards null on unmount) in addition to measuring the current elements.
| onMounted(measureAll) | ||
| onUpdated(measureAll) |
There was a problem hiding this comment.
onUpdated(measureAll) runs after every component update, including scroll-driven updates as virtualRows changes. Measuring all rendered items on every update can be expensive (forces layout reads) and may reintroduce scroll jank. Consider a more targeted trigger (e.g. defer measurement from an element ref callback post-render, or only measure newly mounted items) to avoid repeated full measurement passes.
| } | ||
|
|
||
| rowVirtualizer.value.measureElement(el) | ||
| const virtualItemEls = shallowRef([]) |
There was a problem hiding this comment.
shallowRef([]) with an empty array literal infers never[] in TypeScript, which can make el in forEach be never and cause type errors when passing it to measureElement. Consider explicitly typing virtualItemEls (e.g. as an array of HTMLElement | null).
| const virtualItemEls = shallowRef([]) | |
| const virtualItemEls = shallowRef<(HTMLElement | null)[]>([]) |
| function measureAll() { | ||
| virtualItemEls.value.forEach((el) => { | ||
| if (el) columnVirtualizer.value.measureElement(el) | ||
| }) | ||
| } |
There was a problem hiding this comment.
measureAll never calls columnVirtualizer.value.measureElement(null). In @tanstack/virtual-core, passing null is what triggers cleanup of disconnected nodes (unobserving/removing stale entries from elementsCache). Without that, columns that scroll out/unmount can remain observed/retained. Consider calling measureElement(null) once per update (or using a callback ref that forwards null on unmount) in addition to measuring the current elements.
| onMounted(measureAll) | ||
| onUpdated(measureAll) |
There was a problem hiding this comment.
onUpdated(measureAll) runs after every component update, including scroll-driven updates as virtualRows changes. Measuring all rendered items on every update can be expensive (forces layout reads) and may reintroduce scroll jank. Consider a more targeted trigger (e.g. defer measurement from an element ref callback post-render, or only measure newly mounted items) to avoid repeated full measurement passes.
| if (!el) { | ||
| return | ||
| } | ||
| const virtualItemEls = shallowRef([]) |
There was a problem hiding this comment.
shallowRef([]) with an empty array literal infers never[] in TypeScript, which can make el in forEach be never and cause type errors when passing it to measureElement. Consider explicitly typing virtualItemEls (e.g. as an array of HTMLElement | null).
| const virtualItemEls = shallowRef([]) | |
| const virtualItemEls = shallowRef<(HTMLElement | null)[]>([]) |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Let's add those rowVirtualizer.value.measureElement(null) to all measureAll
📝 WalkthroughWalkthroughRefactors Vue virtualizer components to replace per-element ref callbacks with a fixed array ref approach. Introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/vue/dynamic/src/components/RowVirtualizerDynamic.vue (1)
71-81: Add a browser regression for scrolling back up.This exact interaction has regressed across multiple issues, and the PR notes local tests weren't confirmed. A small end-to-end check that scrolls near the end and then back up slowly would make this fix much safer to keep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/vue/dynamic/src/components/RowVirtualizerDynamic.vue` around lines 71 - 81, Add a browser end-to-end regression that reproduces scrolling near the end and then scrolling back up slowly for the RowVirtualizerDynamic.vue virtualizer: write a test (Playwright/Cypress) that mounts the RowVirtualizerDynamic component, waits for onMounted/onUpdated to call measureAll (which uses virtualItemEls and rowVirtualizer.measureElement), programmatically scrolls the virtual container to near the bottom, waits for measurements/frames, then scrolls back up slowly in small increments and asserts no jump/regression (items render smoothly and scroll position updates as expected). Ensure the test targets the component named RowVirtualizerDynamic.vue and exercises the measureAll flow (virtualItemEls + rowVirtualizer) so future changes trigger this regression check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/vue/dynamic/src/components/RowVirtualizerDynamic.vue`:
- Around line 71-81: Add a browser end-to-end regression that reproduces
scrolling near the end and then scrolling back up slowly for the
RowVirtualizerDynamic.vue virtualizer: write a test (Playwright/Cypress) that
mounts the RowVirtualizerDynamic component, waits for onMounted/onUpdated to
call measureAll (which uses virtualItemEls and rowVirtualizer.measureElement),
programmatically scrolls the virtual container to near the bottom, waits for
measurements/frames, then scrolls back up slowly in small increments and asserts
no jump/regression (items render smoothly and scroll position updates as
expected). Ensure the test targets the component named RowVirtualizerDynamic.vue
and exercises the measureAll flow (virtualItemEls + rowVirtualizer) so future
changes trigger this regression check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0766215e-15f6-4b4a-8f85-af9284c222ca
📒 Files selected for processing (4)
examples/vue/dynamic/src/components/ColumnVirtualizerDynamic.vueexamples/vue/dynamic/src/components/GridVirtualizerDynamic.vueexamples/vue/dynamic/src/components/RowVirtualizerDynamic.vueexamples/vue/dynamic/src/components/RowVirtualizerDynamicWindow.vue
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit