Skip to content

fix(template): truncate DynamicSizeVirtualScrollStrategy virtual items when data shrinks - #1929

Draft
hoebbelsB wants to merge 1 commit into
mainfrom
fix/1923-dynamic-size-shrink-truncation
Draft

hoebbelsB wants to merge 1 commit into
mainfrom
fix/1923-dynamic-size-shrink-truncation

Conversation

@hoebbelsB

Copy link
Copy Markdown
Member

Problem

When the array bound to *rxVirtualFor shrinks (the reporter saw 2000 → 1213), DynamicSizeVirtualScrollStrategy sizes the runway correctly but positions rendered rows far past it. The reporter's measurements: viewport height 43696px (correct for 1213 rows) but the first rendered row at translateY(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.ts

The values$ subscription in maintainVirtualItems rebuilds item sizes with a loop bounded by the new length (for (let i = 0; i < dataArr.length; i++)) and assigns this._virtualItems[i] only when a size actually changed. Nothing ever shortens the array. Outside of detach() and the if (!dataLength) branch, _virtualItems is never reset.

Since contentLength is literally this._virtualItems.length, it keeps the pre-shrink count and desyncs from contentSize — 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:

  • calcRenderedRangerange.end = Math.min(this.contentLength, …) no longer caps at the data length;
  • calculateAnchoredItem — the forward walk while (delta > 0 && i < items.length && …) and the Math.min(i, items.length) clamp can both land past the real data;
  • scrollToIndex — clamps to contentLength - 1 and 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:

// drop stale entries when the data shrank, otherwise `contentLength`
// (= _virtualItems.length) desyncs from the data and the anchor math
// positions items outside of [0, contentSize]
if (this._virtualItems.length > dataLength) {
  this._virtualItems.length = dataLength;
  shouldRecalculateRange = true;
}

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 existing Math.min(i, items.length) clamp, so no separate anchor clamp is needed. The backwards walk there only reads items[i - 1] for i <= dataLength, all of which still exist, so no new undefined access is introduced.

No API surface changes — contentLength and _virtualItems are both private. appendOnly is unaffected: the Math.max(this._renderedRange.end, range.end) widening still runs after the Math.min(length, …) cap, and the pre-existing this._renderedRange.end = dataLength already resets it on shrink. Side benefit: scrollToIndex stops summing stale sizes past the end.

Deliberately out of scope

  • AutosizeVirtualScrollStrategy is not affected, contrary to the "Notes" section of the issue. It allocates new Array<VirtualViewItem>(dataLength), replaces this._virtualItems wholesale, and assigns this.contentLength = dataLength explicitly. Untouched.
  • FixedSizeVirtualScrollStrategy is not affected — it has no _virtualItems; contentSize is dataLength * itemSize.

How it was verified

npx nx component-test template --skipNxCache62/62 passing (autosize 20, dynamic-size 22, fixed-size 20). npx nx test template → 46 suites / 659 passing. nx lint template clean (warnings pre-existing).

Four new cases in dynamic-size.cy.ts under describe('data mutations') > describe('when the data shrinks'):

  1. keeps range and positions within the new bounds — after scrollToIndex(480) and a 500 → 200 shrink, the last emitted viewRange.end is at most 200 and every rendered row's translateY is inside [0, runwayHeight).
  2. truncates the virtual items to the new data length — asserts _virtualItems.length === 200 directly.
  3. renders the full range again when the data grows back — 500 → 200 → 500 restores the expected range and sentinel height.
  4. recovers when the data is emptied and repopulated — exercises the !dataLength branch 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

  • Only case 2 is a true regression test. Cases 1, 3 and 4 also pass on unpatched code, so they are invariant guards rather than proof. The re-anchor block from fix(template): properly calculate range & anchor when values reset #1747 already masks the gross positioning symptom for a single 500 → 200 shrink, and the residual self-corrects within Cypress's retry window. I tried three escalating variants to make case 1 discriminating (a sticky spy-call slice, a self-managed 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.
  • During one instrumented run on unpatched code I did capture a post-shrink emission of { start: 193, end: 210 }end exceeding the 200-item length, i.e. the desync leaking into the public viewRange — but I could not reproduce it deterministically. With the fix, contentLength caps range.end so it cannot occur.
  • I did not reproduce the reporter's exact translateY(66304px) in the component-test harness. Their setup uses rxVirtualScrollElement and 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.
  • Worth replying on the issue that autosize is not affected, rather than widening this PR.

Open questions

  • Only 1 of the 4 new tests (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?
  • On unpatched code I captured one post-shrink viewRange emission 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.
  • The reporter's exact symptom (translateY 66304px vs a 43696px runway) was not reproduced in the Cypress harness; their setup uses rxVirtualScrollElement plus repeated updates. Unclear whether that compounding path needs a further guard beyond the truncation, or whether a reproduction should be requested before merging.
  • The issue's Notes section claims AutosizeVirtualScrollStrategy is also affected; it is not (it reallocates the array and sets contentLength explicitly). Should this be answered as a comment on DynamicSizeVirtualScrollStrategy strands rendered items past the runway after the list shrinks (internal _virtualItems is never truncated) #1923 rather than expanding the PR?
  • No CHANGELOG entry was added (release tooling generates it, and the scope guard limited me to two files) — confirm that is the desired workflow.

…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
@github-actions github-actions Bot added the </> Template @rx-angular/template related label Aug 3, 2026
@nx-cloud

nx-cloud Bot commented Aug 3, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit f3d8f8f

Command Status Duration Result
nx-cloud record -- npx nx format:check ❌ Failed 2s View ↗
nx build docs ✅ Succeeded 32s View ↗
nx build demos --configuration=production ✅ Succeeded 22s View ↗

💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗.


☁️ Nx Cloud last updated this comment at 2026-08-03 23:11:44 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

</> Template @rx-angular/template related

Projects

None yet

1 participant