fix: preserve scrollToIndex preload range on initial render - #9769
fix: preserve scrollToIndex preload range on initial render#9769vursen wants to merge 7 commits into
Conversation
The viewport range set by scrollToIndex was reset back to the first page by the grid's ArrayUpdater.initialize() during the first data flush, which runs after all user code in the roundtrip. The client then had to fetch the target rows in an extra request, briefly showing empty rows. Reset the range in onAttach instead, and skip the reset when a scroll is pending. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f6ffe32 to
10d30b0
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Node attach listeners run before onAttach, so the range preloaded by scrollToIndex and scrollToItem was overwritten by the viewport reset in onAttach. Component attach listeners run after onAttach, so the preload is deferred with those instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 3 comments.
| Finding | |
|---|---|
onAttach now unconditionally resets the viewport range, wiping a preserved scroll range when a grid is moved within one roundtrip. |
|
When called before attach, scrollToEnd() loses to an earlier scrollToIndex/scrollToItem, breaking "last scroll wins". |
|
| 🧹 | The 3-line comment explaining runWhenComponentAttached is copy-pasted verbatim in scrollToIndex and scrollToItem. |
onAttach re-runs the scroll preload after its viewport range reset, so the preload set before attach is no longer overwritten and deferring it until attach is unnecessary.
704bbcf to
d94ceee
Compare
d94ceee to
92a66a4
Compare
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 2 comments.
| Finding | |
|---|---|
On reattach or @PreserveOnRefresh of a scrolled grid, the NO-OP initialize() no longer resets the range, so the fresh client gets a stale scrolled range and does an extra roundtrip. |
|
| 👀 | The ui.add(grid) added to the existing *_preload* tests is cosmetic — without ui.fakeClientCommunication() no flush runs, so these tests still pass against the old buggy code. |
|
/code-review |
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 3 comments.
| Finding | |
|---|---|
Moving the viewport-range reset from initialize() to onDetach misses the @PreserveOnRefresh path, where the client re-initializes without a detach. |
|
| 👀 | The new setViewportRange(0, getPageSize()) in onDetach — the actual behavioral addition — has no test. |
| 👀 | The emptied initialize() is marked only // NO-OP, with no note on why it must stay empty. |
| @Override | ||
| public void initialize() { | ||
| setViewportRange(0, getPageSize()); | ||
| // NO-OP |
There was a problem hiding this comment.
initialize() to onDetach misses the @PreserveOnRefresh path, where the client re-initializes without a detach.
Scroll a Grid inside a @PreserveOnRefresh view (server range becomes e.g. 250-300), then reload the browser. Flow moves the component to the new UI via UIInternalUpdater.moveToNewUI, which calls element.removeFromTree(false). That runs StateNode.reset(false), setting wasAttached=false without firing detach listeners, so Grid.onDetach() — the new home of the reset — never runs. On the following flush isClientSideInitialized() is false, so DataCommunicator calls arrayUpdater.initialize(), which is now a NO-OP, and DataCommunicator.reset() leaves the viewportRange field untouched.
The fresh browser grid starts at scrollTop 0 requesting rows 0-50, but the server still holds the stale 250-300 range and sends those rows first — the exact "extra request, briefly empty rows" symptom this PR set out to remove, now reappearing on refresh. Before the PR, initialize() reset the range to page 0 on this path. Consider resetting the range where re-initialization is detected (or in onAttach guarded so it does not clobber a pre-attach scrollToIndex) rather than only in onDetach.
Grid.java:1469 · correctness · plausible
|
|
||
| @Override | ||
| protected void onDetach(DetachEvent detachEvent) { | ||
| setViewportRange(0, getPageSize()); |
There was a problem hiding this comment.
👀 The new setViewportRange(0, getPageSize()) in onDetach — the actual behavioral addition — has no test.
No test detaches (or detaches and reattaches) a scrolled Grid and asserts the viewport range returns to 0-pageSize. Deleting line 4116 leaves every new and existing test green, so a later change that drops or breaks this reset would ship undetected — a reattached Grid could render from a stale scrolled range instead of the top.
The new *_viewportRangePreloaded tests only cover preservation across attach; add a detach/reattach test to lock in the reset that compensates for the removed initialize() reset.
Grid.java:4116 · test-coverage · plausible
| @Override | ||
| public void initialize() { | ||
| setViewportRange(0, getPageSize()); | ||
| // NO-OP |
There was a problem hiding this comment.
👀 The emptied initialize() is marked only // NO-OP, with no note on why it must stay empty.
Flow still calls this override on the first flush after every (re)attach (DataCommunicator line 1204). A developer seeing an empty overridden method with just // NO-OP could reasonably re-add per-flush initialization here, silently reintroducing the range-clobbering bug this PR fixes.
A one-line comment stating that resetting the range here would overwrite a same-roundtrip scrollToIndex preload would prevent that regression.
Grid.java:1469 · maintainability · plausible
|
2 participants
When
scrollToIndexorscrollToItemwas called in the same roundtrip that attached the grid (for example, in a view constructor), the preloaded viewport range was overwritten back to the first page byGridArrayUpdater.initialize()during the first data flush. The client then had to fetch the target rows in an extra request, briefly showing empty rows.The PR removes the viewport range reset from
GridArrayUpdater.initialize(), both to fix this issue and because it was redundant: the Grid constructor already ensures the initial viewport range during thesetPageSizecall