Fix InitialItemIndex is intermittently ignored on fresh page load - #68114
Conversation
…ition to interactivity.
There was a problem hiding this comment.
Pull request overview
Fixes a startup race in the Virtualize<TItem> initial-scroll path where the spacers’ IntersectionObserver initial callback can run before the initial programmatic alignment, intermittently resetting the window back to index 0 on fresh loads (especially with prerendering + interop latency).
Changes:
- Adds a new
suppressInitialSpacerCallbacksflag to the JSVirtualize.initinterop call so JS can ignore the observers’ initial callback until the component’s first programmatic-scroll decision is made. - Plumbs the new flag from
Virtualize→VirtualizeJsInterop→Virtualize.ts, enabling suppression whenInitialItemIndex > 0. - Adds coverage: a unit test verifying the init-argument roundtrip, plus an E2E test that validates
InitialItemIndexremains applied after transitioning to interactive server mode.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Components/Web/test/Virtualization/VirtualizeTest.cs | Adds a unit test verifying InitialItemIndex controls initial spacer-callback suppression passed to JS init. |
| src/Components/Web/src/Virtualization/VirtualizeJsInterop.cs | Extends InitializeAsync to pass the suppression flag to Blazor._internal.Virtualize.init. |
| src/Components/Web/src/Virtualization/Virtualize.cs | Enables initial suppression when InitialItemIndex > 0 during first render initialization. |
| src/Components/Web.JS/src/Virtualize.ts | Adds suppressInitialSpacerCallbacks param and initializes suppressSpacerCallbacks from it to ignore the initial IO callback. |
| src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Interactivity/VirtualizationTransitionToInteractivity.razor | Adds a query-switched path to exercise VirtualizationAnchorMode during transition-to-interactivity. |
| src/Components/test/testassets/BasicTestApp/VirtualizationAnchorMode.razor | Makes InitialItemIndex a component parameter and wires it through to the Virtualize component and UI. |
| src/Components/test/E2ETest/ServerRenderingTests/VirtualizationRenderModesTest.cs | Adds an E2E assertion that InitialItemIndex is applied after prerender → interactive server load. |
oroztocil
left a comment
There was a problem hiding this comment.
Looks correct but also as a fix for the symptom rather than the cause.
Idea (maybe misguided): add a bool userInitiated argument to OnSpacerBeforeVisible and OnSpacerAfterVisible that passes information if they were initiated do to user scroll or programmatic scroll. The JS side should be able to recognize/track that. ShouldSuppressSpacerCallback can use then this information so it does not have to infer user scroll vs programmatic scroll based on timing (which introduces the race, if I understand things correctly) and the JS suppression added in this PR would not be needed.
Thanks, that does make sense. The mechanism where c# had to detect the reason for the new callback was caused by conservative approach towards existing JSInterop. In the current PR the only fix option was to change the communication methods so the original constraint doesn't apply. I will take a look at the idea in a broader perspective. |
…67933) When the target list underfills the viewport (small overscan / large container), a bottom-spacer ViewportFill callback grows the window capacity to cover the viewport while keeping itemsBefore pinned to the aligned target. Builds on the spacer-visibility reason enum from #68099 (PR #68114). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The spacer-visibility redesign removed the boolean suppressSpacerCallbacks mechanism (replaced by the scrollActivity/SpacerVisibilityReason enum). Merging #68064 reintroduced a write to that now-undeclared variable, breaking the TypeScript build (TS2304). The write had no remaining reader, so removing it is behavior-preserving; ServerVirtualizationTest.AnchorMode_End_PrependAtTop_ViewportStaysStable passes 28/28 across 7 runs (both useItemsProvider cases). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…67933) When the target list underfills the viewport (small overscan / large container), a bottom-spacer ViewportFill callback grows the window capacity to cover the viewport while keeping itemsBefore pinned to the aligned target. Builds on the spacer-visibility reason enum from #68099 (PR #68114). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Agreed! I've updated the reviewer guidance and evals to prefer renames, named methods or variables, and smaller responsibilities first, keep internal mechanics out of public API docs, and only suggest comments for those durable nonlocal constraints. Thanks for pushing on this. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Components/Web/src/Virtualization/Virtualize.cs:632
_initialIndexis set to Pending forInitialItemIndex > 0, but nothing ever transitions it to Completed unless the user scrolls. That leaves the component permanently treating spacer callbacks as "initial alignment in flight" (e.g., ignoring ViewportFill inOnAfterSpacerVisible), which can prevent normal redistribution until the first user interaction.
private void RerenderSpacersIfItemSizeChanged(float previousItemSize)
{
if (_itemSize != previousItemSize)
{
StateHasChanged();
src/Components/Web.JS/src/Virtualize.ts:630
ScrollToOptions.behaviordoes not standardly support the value'instant'(supported values are typically'auto'or'smooth'). Using a non-standard value risks inconsistent behavior across browsers; use'auto'for an immediate scroll.
if (Math.abs(delta) > 0.5) {
beginAlign();
pendingJumpToStart = false;
pendingJumpToEnd = false;
scrollElement.scrollTo({ top: scrollElement.scrollTop + delta, behavior: 'instant' });
}
src/Components/Web/src/Virtualization/VirtualizeJsInterop.cs:37
reasoncomes from JS into a[JSInvokable]method and is cast directly toSpacerVisibilityReason. If an unexpected value is supplied (e.g., mismatched JS/C# versions or a malformed call), the component will treat it as an unknown enum value and execute redistribution logic under an unintended reason. Validate/clamp the value before dispatching to the owner.
[JSInvokable]
public void OnSpacerBeforeVisible(float spacerSize, float spacerSeparation, float containerSize, int reason)
{
_owner.OnBeforeSpacerVisible(spacerSize, spacerSeparation, containerSize, (SpacerVisibilityReason)reason);
}
src/Components/Web/src/Virtualization/Virtualize.cs:506
- When
InitialItemIndex > 0,_initialIndexis set to Pending beforeScrollToItemAsync. If the target can't be aligned (e.g., the provider returns 0 items /_lastRenderedItemCountstays 0), noRenderedContentMeasurementarrives to complete the state, so_initialIndexcan remain Pending indefinitely and suppress normal spacer redistribution.
This issue also appears on line 628 of the same file.
if (InitialItemIndex > 0)
{
_initialIndex.BeginPending(_itemSize);
await ScrollToItemAsync(InitialItemIndex);
}
…itialIndex was never cleared.
Interesting review quality: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTo#parameters |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/Components/Web/src/Virtualization/Virtualize.cs:759
spacerSeparation <= 0does not rejectNaN, so a NaN callback now falls through and permanently sets_totalMeasuredHeightto NaN. The previous positive-value check skipped NaN; reject non-finite measurements before accumulating them.
if (_lastRenderedItemCount <= 0 || _lastRenderedPlaceholderCount > 0 || spacerSeparation <= 0)
NaN from JS doesn't reach server code, interop would transforms it to |
kotlarmilos
left a comment
There was a problem hiding this comment.
LGTM with one comment below
… to events that can come only from user to differenciate between them.
Fixes #68099.
It's not visible with fast connection but with throttling the reproduction is stable, videos for reference.
Before the fix:
InitialItemIndex.-.Dan.race.mp4
After the fix:
InitialItemIndex.-.Dan.race.fixed.mp4
Problem
InitialItemIndexwas ccasionally ignored on a fresh page load. The newly-created spacerIntersectionObservers fired an "initial" callback atscrollTop == 0before the component's ownScrollToItemAsync(InitialItemIndex)round trip completes, and .NET couldn't tell that stale startup callback apart from a real user scroll. So it cancelled the pending initial scroll and reset the window to index 0. This race race reproduces reliably with latency, as above, on the first cold request after app starts.Fixes
JS now tells .NET why a spacer became visible (
SpacerVisibilityReason: UserScroll/ProgrammaticScroll/ViewportFill) instead of .NET trying to infer intent from timing via a suppression flag. A stale startup callback is aViewportFilland is ignored while the initial alignment is in flight. A real scroll is always classified asUserScroll` and always honored. This removes the race.(An earlier revision of this PR suppressed the observer's initial callback via a
suppressInitialSpacerCallbacksflag passed into JS'sinit(). That approach has since been replaced by the reason-based design above, based on review feedback see ).First scroll after
InitialItemIndexcould land on the wrong item:After
InitialItemIndexjumps to a distant index (distant == changes spacer sizes considerably), the rendered window is positioned far from both spacers. No spacer-visibility callback fires to measure real item height before the user's first scroll. The item-size estimate then stays at its default until that first scroll, where a single large recalibration (e.g. real rendered height including borders/padding vs. the defaultItemSize) gets applied across a huge already-rendered span. That was producing a large one-time miscalculation of the window position, visible as the list jumping to the wrong item, e.g. 515 -> 420.Fix:
C# needed an update from JS about the new content distibution after the jump-to-item. New
SpacerVisibilityReasonwas introduced for that:RenderedContentMeasurementand the callback to C# is triggered after the initial alignment lands.First render dispatching a spurious spacer callback (Virtualize first render has upper spacer #64029)
On the first render for id=X, X!=0, where both spacers are simultaneously visible, both callback fired. The existing dedup (skip one spacer when its height is 0) wasn't sufficient to prevent this, so the conflicting "after" callback would get processed too.
Fix:
Explicitly detect when both spacers intersect and reporting only the "before" spacer's callback in that case. Covered by the new
InitialRender_DispatchesSingleSpacerCallbacktest: 0332c35.Why the tests are not in the component we normally used for
InitialItemIndextests:The test had to be added to a component that contains "to interactivity" transition so I reused the
VirtualizeAnchorModecomponent inVirtualizationTransitionToInteractivity.razor.