fix(virtual-core): Notify framework when count changes to update getTotalSize() #1085
Conversation
🦋 Changeset detectedLatest commit: 0024982 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
shunn2
changed the title
|
View your CI Pipeline Execution ↗ for commit 0024982
☁️ Nx Cloud last updated this comment at |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks for the PR, great work!
Please rebase onto the latest master and update the memo to use skipInitialOnChange (added in #1088). This will ensure we avoid the extra initial notify while preserving correct onChange semantics.
After that, we can merge.
| onChange: () => { | ||
| // Notify when measurement options change as they affect total size | ||
| this.notify(false) | ||
| }, |
There was a problem hiding this comment.
@shunn2 I created a PR earlier (#1088) that adds an initialization guard to the memo system so we avoid the extra initial notify call. This keeps the onChange semantics consistent (only firing when state actually changes), preserves the existing React re-render count expectations, and still fixes the stale getTotalSize issue.
With that now merged into master, you can simplify this block to
| onChange: () => { | |
| // Notify when measurement options change as they affect total size | |
| this.notify(false) | |
| }, | |
| skipInitialOnChange: true | |
| onChange: () => { | |
| // Notify when measurement options change as they affect total size | |
| this.notify(this.isScrolling) | |
| }, |
There was a problem hiding this comment.
@piecyk Thank you for the helpful changes!
I think your work will lead to even better results.
I will reflect your feedback and let you know.
There was a problem hiding this comment.
Passing this.isScrolling is correct here, as options can change while the user is scrolling, and in that case we do want the framework to know the update happened in a scrolling context. Using this.isScrolling keeps the onChange notification semantics aligned with the rest of the virtualizer and preserves the adapters’ scroll-specific optimizations.
There was a problem hiding this comment.
Yes, I agree. Thanks for the explanation.
2 participants
🎯 Changes
Fixes issue where getTotalSize() returns stale values when count changes during filtering/search, causing blank space or inaccessible items.
Added onChange callback to getMeasurementOptions memo that auto-notifies framework when count/measurement options change.
✅ Checklist