Skip to content

fix: preserve committed pages in connector range management in combo-box#9220

Closed
DiegoCardoso wants to merge 11 commits into
mainfrom
fix/combo-box/connector-range-management
Closed

fix: preserve committed pages in connector range management in combo-box#9220
DiegoCardoso wants to merge 11 commits into
mainfrom
fix/combo-box/connector-range-management

Conversation

@DiegoCardoso
Copy link
Copy Markdown
Contributor

@DiegoCardoso DiegoCardoso commented Apr 29, 2026

Summary

A lazy combo-box can get stuck on loading=true when the connector receives a programmatic scrollToIndex jump to an unloaded far page — the existing range-management logic wipes already-committed data and the data-fetch flow stalls. This fix tracks committed pages separately from pending callbacks so the active-range eviction trigger stays accurate, and the non-sequential branch fires a viewport-range RPC that covers all pending pages (the server's last-write-wins semantics mean a per-page RPC drops earlier-pending requests).

ComponentRenderer-rendered pages outside the new viewport are evicted client-side: the server passivates the rendered components when items leave the KeyMapper's active set, so cached node ids would otherwise bind to detached virtual children. Plain-data pages stay in cache.

This is a prerequisite for the focusSelectedItem feature in #9194, which exercises this path on every open with a far-positioned selected item.

Notes for reviewers

A summary comment with the deeper context behind the assertion changes in ComboBoxClientSideDataRangeIT, ItemCountUnknownComboBoxIT, and the commitPage / non-sequential-branch fixes is posted as a separate PR comment to keep the diff readable.

@vaadin vaadin deleted a comment from github-actions Bot Apr 29, 2026
DiegoCardoso added a commit that referenced this pull request Apr 29, 2026
…s on non-contiguous request

Two follow-on fixes to the connector range-management change:

1. Cover all pending pages in the non-sequential setViewportRange.

   `_scroller.scrollIntoView(N)` triggers per-page `dataProvider` calls
   for every visible placeholder page in one rAF batch. Each call hit
   the non-sequential branch and fired its own `setViewportRange` RPC.
   These RPCs overwrite each other on the server (last write wins), so
   any earlier-pending page that wasn't covered by the latest range
   never received data — its `pageCallbacks` entry lingered and the
   combo-box stayed `loading=true`. The fix expands the range to cover
   every entry in `pageCallbacks` so the latest RPC always covers the
   earlier ones.

2. Evict ComponentRenderer-rendered committed pages outside the new
   range so their stale node ids don't get re-bound on scroll-back.

   PR #9220 preserves committed pages by design, but server-side
   ComponentRenderer components are passivated when items leave the
   KeyMapper's active set — virtual children are detached and the
   `*_nodeid` properties cached on the client point at deleted state-
   tree nodes. Re-rendering from the cached items showed stale (or
   empty) renderer slots. The eviction is scoped to pages whose first
   item carries an `*_nodeid` property, so plain-data pages stay
   intact and `ComboBoxClientSideDataRangeIT`'s page-0-preserved
   assertion still holds.

Un-`@Ignore`s `LazyLoadingIT.setComponentRenderer_scrollDown_scrollUp_itemsRendered`. The pre-existing
`@Ignore` blamed FlowComponentRenderer; the actual cause was the
combination above.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso changed the title fix(combo-box): preserve committed pages in connector range management fix: preserve committed pages in connector range management in combo-box Apr 30, 2026
DiegoCardoso added a commit that referenced this pull request Apr 30, 2026
Drop comments that explain what the code USED to do (or describe the
PR rather than the resulting code). Keep only comments that document
non-obvious WHY of the resulting code — invariants, server-behavior
contracts, hidden constraints. Historical context moved to PR #9220.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso
Copy link
Copy Markdown
Contributor Author

DiegoCardoso commented Apr 30, 2026

Reviewer notes — context for the trimmed comments

Update (4e16446352) — took the review proposal and replaced the connector's local pageCallbacks object with a getter that reads comboBox.__dataProviderController.rootCache.pendingRequests directly off the data-provider controller. Algorithm semantics are unchanged: committedPages still tracks delivered pages, the controller's pendingRequests now plays the role the local pageCallbacks did, and ComboBoxClientSideDataRangeIT + ItemCountUnknownComboBoxIT continue to pass (9/9 locally). The notes below have been retouched to refer to pendingRequests rather than pageCallbacks.

The earlier commit (d54a46832d) removes some long inline comments that read more like commit-message material than code documentation. Posting the historical context here so it's preserved without bloating the source files. Each item below corresponds to a block that was shortened or dropped.

comboBoxConnector.js — non-sequential branch

The full rationale for "fire setViewportRange covering all currently-pending pages, not just the new one":

  • _scroller.scrollIntoView(N) jumps the virtualizer; visible placeholders fire index-requested → the data-provider-controller turns them into per-page dataProvider({page, ...}, callback) calls in one rAF batch.
  • With pageSize=50 and a render buffer that spans more than one page, the call comes in for two adjacent pages back-to-back (e.g. pages 5 and 6 when scrolling to index 300).
  • Each call previously fired its own setViewportRange RPC. These overwrite each other on the server (last write wins), so the earlier-pending page never received data and its entry in pendingRequests lingered → combo-box stuck on loading=true.
  • The new range covers every entry in pendingRequests so the latest RPC always covers the earlier ones.

comboBoxConnector.jscommitPage deletion of the pending-request entry

Without delete getPendingRequests()[page] after committing, every subsequent fetch for a different page would treat the just-committed entry as a gap in the active range and incorrectly invoke clearPageCallbacks() across all pages. The deletion is what makes the committedPages Set the single source of truth for "delivered" while pendingRequests reflects only in-flight requests.

ComboBoxClientSideDataRangeIT.scrollToEnd_* assertion change

The pre-PR assertion was "1 page loaded after jumping to the end" (only the last page). The PR's committedPages change preserves page 0 (loaded by openPopup) when the connector receives a non-contiguous request — the older code wiped already-committed pages on a jump as a side effect of the range-management bug. Memory still stays bounded by maxLoadedItemsCount, which kicks in only when total active pages exceed the cap.

ItemCountUnknownComboBoxITRangeLog assertions dropped

The original tests verified a specific server-side fetch cadence via RangeLog entries. That cadence depended on the connector's pre-fix range-management bug — every non-contiguous request triggered a clearPageCallbacks() that wiped previously-loaded pages, causing the WC to re-fetch them. With the connector fix, far-page fetches don't disturb already-loaded pages, so fewer round-trips happen and the index/range sequence is different (and inherently timing-sensitive). The behavior the tests still verify — item count growth, visible labels, count-mode switching — is what matters for the feature; the cadence assertions are tied to the old bug and not worth re-asserting under the new behavior.

DiegoCardoso added a commit that referenced this pull request May 4, 2026
…s on non-contiguous request

Two follow-on fixes to the connector range-management change:

1. Cover all pending pages in the non-sequential setViewportRange.

   `_scroller.scrollIntoView(N)` triggers per-page `dataProvider` calls
   for every visible placeholder page in one rAF batch. Each call hit
   the non-sequential branch and fired its own `setViewportRange` RPC.
   These RPCs overwrite each other on the server (last write wins), so
   any earlier-pending page that wasn't covered by the latest range
   never received data — its `pageCallbacks` entry lingered and the
   combo-box stayed `loading=true`. The fix expands the range to cover
   every entry in `pageCallbacks` so the latest RPC always covers the
   earlier ones.

2. Evict ComponentRenderer-rendered committed pages outside the new
   range so their stale node ids don't get re-bound on scroll-back.

   PR #9220 preserves committed pages by design, but server-side
   ComponentRenderer components are passivated when items leave the
   KeyMapper's active set — virtual children are detached and the
   `*_nodeid` properties cached on the client point at deleted state-
   tree nodes. Re-rendering from the cached items showed stale (or
   empty) renderer slots. The eviction is scoped to pages whose first
   item carries an `*_nodeid` property, so plain-data pages stay
   intact and `ComboBoxClientSideDataRangeIT`'s page-0-preserved
   assertion still holds.

Un-`@Ignore`s `LazyLoadingIT.setComponentRenderer_scrollDown_scrollUp_itemsRendered`. The pre-existing
`@Ignore` blamed FlowComponentRenderer; the actual cause was the
combination above.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DiegoCardoso added a commit that referenced this pull request May 4, 2026
Drop comments that explain what the code USED to do (or describe the
PR rather than the resulting code). Keep only comments that document
non-obvious WHY of the resulting code — invariants, server-behavior
contracts, hidden constraints. Historical context moved to PR #9220.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 4e16446 to dd555eb Compare May 4, 2026 16:24
}
// The page is committed: track it in committedPages and drop the
// pendingRequests entry so range-management sees only pending requests.
delete getPendingRequests()[page];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this? The page is already removed by DataProviderController when the data is received via callback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 41130c6 to ec9814b Compare May 5, 2026 11:39
@vaadin vaadin deleted a comment from github-actions Bot May 5, 2026
DiegoCardoso and others added 11 commits May 6, 2026 14:13
`commitPage` previously did not delete the entry from `pageCallbacks`,
so subsequent `dataProvider` requests for different pages saw stale
active-range entries — the non-sequential branch then wiped already-
committed data and the data-fetch flow stalled when a far-page request
came in (e.g. a programmatic `scrollToIndex` jump on lazy data). Track
committed pages in a dedicated set so the active-range eviction trigger
stays accurate while pageCallbacks reflects only pending requests.

Also relax the non-sequential branch to fetch only the new page rather
than wiping everything: the original wipe-and-refetch was a side effect
of the stale-pageCallbacks bug, and removing it lets the connector
serve far-page requests correctly without triggering a stuck-loading
cycle.

Adjusts ItemCountUnknownComboBoxIT and ComboBoxClientSideDataRangeIT
to match the new fetch cadence (fewer redundant fetches now that
committed data isn't needlessly evicted on non-sequential scroll).
The behavior under test is preserved; the implementation-detail
RangeLog assertions and over-aggressive eviction expectations are
relaxed, with comments pointing to the connector fix.

Marks LazyLoadingIT.setComponentRenderer_scrollDown_scrollUp_itemsRendered
as @ignore — pre-existing FlowComponentRenderer behavior that relied
on the connector's wipe-and-refetch to re-attach component-renderer
slots after scroll. Tracked separately as a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s on non-contiguous request

Two follow-on fixes to the connector range-management change:

1. Cover all pending pages in the non-sequential setViewportRange.

   `_scroller.scrollIntoView(N)` triggers per-page `dataProvider` calls
   for every visible placeholder page in one rAF batch. Each call hit
   the non-sequential branch and fired its own `setViewportRange` RPC.
   These RPCs overwrite each other on the server (last write wins), so
   any earlier-pending page that wasn't covered by the latest range
   never received data — its `pageCallbacks` entry lingered and the
   combo-box stayed `loading=true`. The fix expands the range to cover
   every entry in `pageCallbacks` so the latest RPC always covers the
   earlier ones.

2. Evict ComponentRenderer-rendered committed pages outside the new
   range so their stale node ids don't get re-bound on scroll-back.

   PR #9220 preserves committed pages by design, but server-side
   ComponentRenderer components are passivated when items leave the
   KeyMapper's active set — virtual children are detached and the
   `*_nodeid` properties cached on the client point at deleted state-
   tree nodes. Re-rendering from the cached items showed stale (or
   empty) renderer slots. The eviction is scoped to pages whose first
   item carries an `*_nodeid` property, so plain-data pages stay
   intact and `ComboBoxClientSideDataRangeIT`'s page-0-preserved
   assertion still holds.

Un-`@Ignore`s `LazyLoadingIT.setComponentRenderer_scrollDown_scrollUp_itemsRendered`. The pre-existing
`@Ignore` blamed FlowComponentRenderer; the actual cause was the
combination above.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Flatten the filter callback to a direct boolean expression instead
  of an inline early-return guard.
- Drop the redundant length check: clearPageCallbacks runs forEach
  over its input, which is a no-op on an empty array.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop comments that explain what the code USED to do (or describe the
PR rather than the resulting code). Keep only comments that document
non-obvious WHY of the resulting code — invariants, server-behavior
contracts, hidden constraints. Historical context moved to PR #9220.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a `non-contiguous page requests` describe block to the connector
WTR suite covering the three connector behaviors changed in this PR:

1. A multi-page rAF batch fires one setViewportRange covering every
   pending page, not per-page (regression guard for the stuck-loading
   bug — `setViewportRange` is last-write-wins on the server).
2. Already-committed pages no longer drag their entries into a later
   non-contiguous request range.
3. ComponentRenderer-rendered committed pages are evicted on a
   non-contiguous jump while plain-data pages stay cached.

Also moves the `confirmUpdate` server stub and `plainItems` /
`rendererItems` factories into `shared.ts` so additional test cases
can use them.

Run via `node scripts/wtr.js combo-box`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…store

Replaces the connector's local pageCallbacks object with a getter that
reads comboBox.__dataProviderController.rootCache.pendingRequests off
the data-provider controller. Algorithm semantics — including the
active-range / committedPages logic and non-contiguous handling — are
unchanged; only the storage location moves.

ComboBoxClientSideDataRangeIT and ItemCountUnknownComboBoxIT (9 tests)
continue to pass. Approach proposed during PR review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eviction

Replaces the per-item `*_nodeid`-suffix detection in the non-contiguous
branch's eviction filter with a single `comboBox.renderer` check. Same
semantics in practice — Flow's setRenderer always sets `comboBox.renderer`
and produces items with renderer-namespaced node ids — but the gate is
expressed once on the combo-box rather than re-evaluated per page from
item shape.

ComboBoxClientSideDataRangeIT and ItemCountUnknownComboBoxIT continue
to pass (9/9). The TS unit tests in combo-box-connector.test.ts are
updated to set `comboBox.renderer` directly instead of seeding items
with synthesized renderer-shaped node ids; the now-unused `rendererItems`
helper is removed from shared.ts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…page

Two safety nets in the non-contiguous active-range branch:

1. If the bounding box of pending pages would exceed the memory cap
   (e.g. a deferred page-0 re-fetch racing a deep `scrollToIndex` jump
   that has page 0 still pending and pages near the target also
   pending), drop the pending page furthest from the freshest request
   and recurse. Without this guard the connector sent
   `setViewportRange(0, 5000)` and the server warned about exceeding
   500 items per fetch.

2. The renderer-set eviction filter now skips the page that contains
   the currently focused index. The WC's `scrollIntoView` runs
   `_visibleItemsCount`, which re-anchors the virtualizer at index 0
   and triggers `index-requested` for any placeholder there. Without
   this guard the resulting page-0 request evicted the focused page,
   `_setDropdownItems` reset `_focusedIndex` to -1, and the
   focusSelectedItem-driven scroll was undone.

Adds two TS unit tests covering both paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pendingRequests delete

`commitPage` previously did `delete getPendingRequests()[page]` after
firing the page callback, but the data-provider-controller's callback
wrapper already deletes that entry as part of its receive-data flow
(see vaadin-combo-box-data-provider-mixin / data-provider-controller).
The explicit delete is a no-op; drop it (per PR review feedback).

Also tightens block comments around `committedPages`, `clearPageCallbacks`,
the active-range computation, the non-contiguous branch, and the renderer
eviction. WHY-comments stay; WHAT-comments shrink or go.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two non-contiguous pages pending simultaneously (e.g. a deferred page-0
re-fetch racing a deep scrollToIndex jump) used to ask the server for
the full bounding-box range — exceeding the DataCommunicator's
500-item-per-fetch cap, leaving the far page's callback stuck on
loading=true. The cap-recurse in the connector's non-contiguous branch
prevents that. The new test pins the request count to the cap so a
removal of the recurse fails loudly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 6e45890 to 7da590b Compare May 6, 2026 12:15
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

@DiegoCardoso
Copy link
Copy Markdown
Contributor Author

Closing in favor of #9238

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants