Repository navigation
fix(virtual-scroll): record layout where the viewport is measured - #17739
Open
viktorkombov wants to merge 1 commit into
Open
viktorkombov wants to merge 1 commit into
viktorkombov wants to merge 1 commit into
Conversation
viktorkombov
requested review from
rkaraivanov
and
a balanced review from Copilot
October 7, 2026 09:41
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the reported regression and includes focused coverage for detached and reattached viewport behavior.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes detached virtual-scroll hosts from emitting transient window states that trigger redundant remote combo requests.
Changes:
- Defers detached
stateChangeemissions until reattachment. - Adds regression coverage for combo reopening and pending requests.
- Documents the revised emission behavior.
| File | Description |
|---|---|
virtual-scroll.component.ts |
Tracks layout and deferred reattachment reports. |
virtual-scroll.component.spec.ts |
Tests detached-host reporting. |
virtual-scroll/README.md |
Documents emission semantics. |
simple-combo.component.spec.ts |
Covers remote page preservation and reopening. |
combo.component.spec.ts |
Covers request lifecycle and restored windows. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.
Closes #17688
Description
igx-virtual-scrollno longer emitsstateChangewhile its host is detached after it has been laid out. When the host re-attaches, it emits the current window once, if that window differs from the last one reported.Rendering is unchanged. A detached host still follows its reset offset, as #17662 needs for the ESF re-attach case; only the report is held back.
Motivation / Context
Regression from #17662 (
2a54d50c3c), shipped in 22.2.0-rc.2.Closing a remote-bound
igx-comboorigx-simple-combodetaches the list._syncScrollPositionfollows the detached host'sscrollTopof 0, sostateChangereported{0..k}and the combo emitteddataPreLoadfor the first page after the drop-down had closed. Reopening restored the offset and requested the original page again. That is two extra requests per open/close cycle after any scroll. A page still in flight at close was cancelled by the consumer, so the reopened list stayed blank for one round trip.Why not ignore the window in the combo while it is collapsed: that removes the requests, but the virtual scroll has still emitted
{0..k}. Reopening at the top produces the same range, so no signal changes, theafterRenderEffectnever re-runs, and the list stays blank with no request.IgxSimpleComboComponent.onClickdoes exactly this with an empty input. The re-attach report (_attachTick) covers that case.Why
_wasLaidOut: a plain_isLaidOut()check would also suppress the report made before the first open, while the list is not in the DOM yet, and the combo relies on that report.Type of Change (check all that apply):
Component(s) / Area(s) Affected:
Virtual Scroll; Combo and Simple Combo (remote binding)
How Has This Been Tested?
Six regression specs: three for the requests on close and reopen, three for reopening at the top. The last two assert the bound page and rendered rows, not the request count.
Suites: combo, simple-combo, virtual-scroll, drop-down, grid and tree-grid filtering - 874/874. Lint: 0 errors.
Also checked in headless Chrome by an automated script, using real input events through CDP and real Northwind pages in the simple-combo showcase. Escape and outside-click closes sent no request, and the toggle reopen requested index 0 and rendered rows 0-5. This was not a manual click-through.
Test Configuration:
Screenshots / Recordings
None - no visual change.
Notes
totalItemCountshrinking below the current window leaves a blank list with no request. This is pre-existing.onClosedassigns a page the combo did not request (samples repo).🤖 Generated with Claude Code