Conversation
…s when data shrinks `maintainVirtualItems` rebuilt the item sizes with a loop bounded by the new data length, but never shortened `_virtualItems`. `contentLength` therefore kept the pre-shrink count and desynced from `contentSize`, so the anchor math walked into stale trailing entries, positioning rendered rows outside of the runway and snapping the scroll position to the bottom. Closes #1923
|
View your CI Pipeline Execution ↗ for commit f3d8f8f
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
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
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
When the array bound to
*rxVirtualForshrinks (the reporter saw 2000 → 1213),DynamicSizeVirtualScrollStrategysizes the runway correctly but positions rendered rows far past it. The reporter's measurements: viewport height43696px(correct for 1213 rows) but the first rendered row attranslateY(66304px). The result is a blank viewport, the scroll position snapping to the bottom, and scrolling that only reconciles after many scroll events.Closes #1923
Root cause
libs/template/virtual-scrolling/src/lib/scroll-strategies/dynamic-size-virtual-scroll-strategy.tsThe
values$subscription inmaintainVirtualItemsrebuilds item sizes with a loop bounded by the new length (for (let i = 0; i < dataArr.length; i++)) and assignsthis._virtualItems[i]only when a size actually changed. Nothing ever shortens the array. Outside ofdetach()and theif (!dataLength)branch,_virtualItemsis never reset.Since
contentLengthis literallythis._virtualItems.length, it keeps the pre-shrink count and desyncs fromcontentSize— which is summed from the new data and is therefore correct. That is exactly the asymmetry the reporter measured: right runway, wrong positions.The stale tail is then consumed by:
calcRenderedRange—range.end = Math.min(this.contentLength, …)no longer caps at the data length;calculateAnchoredItem— the forward walkwhile (delta > 0 && i < items.length && …)and theMath.min(i, items.length)clamp can both land past the real data;scrollToIndex— clamps tocontentLength - 1and sums stale sizes.PR #1747 (fixes #1744) added the
if (dataLength < this._renderedRange.end)re-anchor block below this point, which papers over the worst case but deliberately did not truncate. On that issue the reporter noted the real fix is "not going into that state in the first place" and @hoebbelsB agreed. This is that residual.What changed
One hunk in the strategy, placed between the size-rebuild loop and the existing re-anchor block:
The ordering is load-bearing. Truncating before the re-anchor block means
calculateAnchoredItem({ index: dataLength, offset: 0 }, -visibleSize)gets the new length for free through its existingMath.min(i, items.length)clamp, so no separate anchor clamp is needed. The backwards walk there only readsitems[i - 1]fori <= dataLength, all of which still exist, so no new undefined access is introduced.No API surface changes —
contentLengthand_virtualItemsare bothprivate.appendOnlyis unaffected: theMath.max(this._renderedRange.end, range.end)widening still runs after theMath.min(length, …)cap, and the pre-existingthis._renderedRange.end = dataLengthalready resets it on shrink. Side benefit:scrollToIndexstops summing stale sizes past the end.Deliberately out of scope
AutosizeVirtualScrollStrategyis not affected, contrary to the "Notes" section of the issue. It allocatesnew Array<VirtualViewItem>(dataLength), replacesthis._virtualItemswholesale, and assignsthis.contentLength = dataLengthexplicitly. Untouched.FixedSizeVirtualScrollStrategyis not affected — it has no_virtualItems;contentSizeisdataLength * itemSize.How it was verified
npx nx component-test template --skipNxCache→ 62/62 passing (autosize 20, dynamic-size 22, fixed-size 20).npx nx test template→ 46 suites / 659 passing.nx lint templateclean (warnings pre-existing).Four new cases in
dynamic-size.cy.tsunderdescribe('data mutations') > describe('when the data shrinks'):keeps range and positions within the new bounds— afterscrollToIndex(480)and a 500 → 200 shrink, the last emittedviewRange.endis at most 200 and every rendered row'stranslateYis inside[0, runwayHeight).truncates the virtual items to the new data length— asserts_virtualItems.length === 200directly.renders the full range again when the data grows back— 500 → 200 → 500 restores the expected range and sentinel height.recovers when the data is emptied and repopulated— exercises the!dataLengthbranch alongside the new truncation.I stashed the production hunk and re-ran to confirm the tests bite. Case 2 fails on unpatched code with
expected 500 to equal 200— the desync, caught directly.Open questions / honest caveats
renderedRange$subscription, and a fixed settle wait) and none failed pre-fix reliably, so I kept the simple readable form rather than ship a test whose failure mode I could not reproduce on demand.{ start: 193, end: 210 }—endexceeding the 200-item length, i.e. the desync leaking into the publicviewRange— but I could not reproduce it deterministically. With the fix,contentLengthcapsrange.endso it cannot occur.translateY(66304px)in the component-test harness. Their setup usesrxVirtualScrollElementand repeated updates, which likely compounds the desync across several emissions. If someone can share that reproduction it would be worth confirming no second guard is needed.apps/demos/.../virtual-for-monkey-test.component.ts(added by fix(template): properly calculate range & anchor when values reset #1747 for exactly these reset/shrink scenarios) is the natural place to try.Open questions
truncates the virtual items to the new data length) actually fails on unpatched code. The behavioural runway/position test passes pre-fix because PR fix(template): properly calculate range & anchor when values reset #1747's re-anchor block already masks the gross symptom in a single-shrink scenario. Is a state-level regression test acceptable to the maintainer, or should the behavioural one be dropped as noise?viewRangeemission of{start:193,end:210}— end exceeding the 200-item data length, i.e. the desync leaking into the public output — but could not reproduce it deterministically across runs. The fix makes it structurally impossible, but I could not turn it into a stable failing assertion.rxVirtualScrollElementplus repeated updates. Unclear whether that compounding path needs a further guard beyond the truncation, or whether a reproduction should be requested before merging.contentLengthexplicitly). Should this be answered as a comment on DynamicSizeVirtualScrollStrategy strands rendered items past the runway after the list shrinks (internal_virtualItemsis never truncated) #1923 rather than expanding the PR?