refactor: simplify TreeGrid connector scrollToIndex - #10050
Conversation
vursen
changed the title
|
The fix can be backported to 25.3 as is, but backporting to 25.2 and 25.1 would require a slightly different solution because there is no |
601ee32 to
308c01c
Compare
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 1 comment.
| Finding | |
|---|---|
| Page loads are now also unblocked while the server request is in flight, so a competing setViewportRange can undo the preload |
| @@ -55,7 +46,12 @@ function initLazy(grid: FlowTreeGrid) { | |||
| const flatIndex = await grid.$server.setViewportRangeByIndexPath(indexes, padding); | |||
There was a problem hiding this comment.
The new resolvePendingCallbacks() only cleans up requests that exist after the response arrives. Nothing prevents a request from being sent during the round trip anymore.
If the user scrolls while the request is in flight, the patched __updateVirtualizerElement prefetches the fetch-range edges, which starts a page load and arms the 150 ms request debouncer. When the response takes longer than that, the client sends setViewportRange for the old position. On the server this replaces the active range that setViewportRangeByIndexPath just preloaded, so the target range gets cleared and the grid scrolls to blank rows before re-fetching.
Only the fast path is confirmed to be safe: if the response lands first, the new cleanup cancels the debouncer. Both new tests resolve the promise immediately, so the slow-response window is untested.
treeGridConnector.ts:46 · correctness
There was a problem hiding this comment.
The issue isn't serious: the expected viewport will just get re-requested after scrolling, resulting in one extra round trip. Besides, fixing that was never the goal of the original solution.
308c01c to
0beef38
Compare
|
|
||
| /** The Flow grid element */ | ||
| export type FlowGrid = Grid<Item> & FlowGridInternals; | ||
| export type FlowGrid = FlowGridInternals & Grid<Item>; |
There was a problem hiding this comment.
note: Changing the order makes FlowGridInternals (and FlowTreeGridInternals) take priority over Grid<Item>'s properties
3aa73ef to
5ed4ebe
Compare
| $server: GridServer; | ||
| } & ConnectorFlowGrid & { | ||
| _flatSize: number; | ||
| _updateItem: (index: number, item: Item) => void; |
There was a problem hiding this comment.
note: _updateItem was left out because of no usages.
| }); | ||
| }); | ||
|
|
||
| describe('initially non-empty data set', () => { |
There was a problem hiding this comment.
This does exactly the same as should run the deferred scroll when the data set becomes non-empty
|
This fix kind of solves the reported issue, but I also wonder if it's correct that the grid attempts to scroll to an index after first being rendered with an empty data set and then a non-empty one. |
The connector blocked page loading while a scrollToIndex call was pending so the old range would not be re-requested while the preloaded range was applied. Resolving the stale requests right after the scroll handles that directly, so pending scrolls no longer affect page loading. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5ed4ebe to
8a34212
Compare
vursen
changed the title
|
3 participants
Description
Related to #10014
Related to vaadin/web-components#12691
The TreeGrid connector used
__pendingScrollToIndexesfor two things: to defer ascrollToIndexcall until the grid is ready, and to block page loading in_shouldLoadCachePage. The block was only ever needed for the short window in which the old range is cleared while still rendered. It also stayed on whenever a deferred call was not retried or the server request failed, which is how #10014 ended up with a grid that no longer lazy loads. The web component now retries a deferred call even when no rows are rendered (vaadin/web-components#12691), which fixes that issue on its own. This change handles the old range directly and removes the block, so page loading no longer depends on the scroll state at all._shouldLoadCachePageoverride fromtreeGridConnector.ts, so a pendingscrollToIndexno longer prevents page loadingscrollToIndexresolve the pending page requests right after scrolling to the preloaded rangetree-grid-connector-scroll-to-index.test.tscoveringscrollToIndexcalled before and after the grid has renderedsetViewportRangeByIndexPathwith a controllable promise in the shared test helpers and added theFlowTreeGridtype thereType of change