Skip to content

[WC-3390] DG2: improve virtual scroll behavior and height logic#2193

Open
yordan-st wants to merge 5 commits into
mainfrom
fix/WC-3390_virtual-scroll-bug
Open

[WC-3390] DG2: improve virtual scroll behavior and height logic#2193
yordan-st wants to merge 5 commits into
mainfrom
fix/WC-3390_virtual-scroll-bug

Conversation

@yordan-st
Copy link
Copy Markdown
Contributor

Pull request type

Bug fix (non-breaking change which fixes an issue)


Description

  • Virtual scrolling got stuck showing only the initial page of rows when the grid had many columns. The overflow detection compared measurements from different sources, incorrectly skipping the offset needed to make the body scrollable. Fixed by using consistent measurements for the overflow check and adding a fallback that loads more rows when the grid has no scrollbar but more data is available.

@yordan-st yordan-st requested a review from a team as a code owner April 28, 2026 13:06
@yordan-st yordan-st force-pushed the fix/WC-3390_virtual-scroll-bug branch 5 times, most recently from 28d92e3 to a3e363f Compare April 30, 2026 13:09
@yordan-st yordan-st force-pushed the fix/WC-3390_virtual-scroll-bug branch from f72a5e8 to 6a14738 Compare May 4, 2026 12:22
@yordan-st yordan-st force-pushed the fix/WC-3390_virtual-scroll-bug branch from 07233d2 to 673af4d Compare May 18, 2026 12:39
@github-actions
Copy link
Copy Markdown

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts Added fallback in lockGridContainerHeight to call bumpPage() when no scrollbar exists but more data is available
packages/pluggableWidgets/datagrid-web/CHANGELOG.md Added ### Fixed entry under [Unreleased]
packages/pluggableWidgets/datagrid-web/e2e/filtering/DataGridFilteringIntegration.spec.js-snapshots/datagridFilteringIntegration-chromium-linux.png Updated screenshot baseline

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

⚠️ Low — No unit tests for the new fallback path

File: packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts lines 116–119
Note: The new bumpPage() fallback (called when scrollHeight <= clientHeight but hasMoreItems is true) has no corresponding unit test. The store is otherwise untested. The scenario — grid body does not overflow but more items exist — is precisely the regression risk described in the PR. A unit test that mocks gridBodyRef.current with matching scrollHeight/clientHeight values and asserts setPageAction is called would lock this behaviour in.


⚠️ Low — lockGridContainerHeight re-checks hasMoreItems redundantly

File: packages/pluggableWidgets/datagrid-web/src/model/stores/GridSize.store.ts line 117
Note: The method already returns early at line 79 when !this.hasMoreItems, so the guard this.hasMoreItems && on line 117 is always true by the time execution reaches it. It is harmless but adds noise; the condition can be simplified to just gridBody && gridBody.scrollHeight <= gridBody.clientHeight.


Positives

  • The CHANGELOG entry under [Unreleased] is well-written: end-user language, describes both the symptom ("showing only the initial page of rows") and the trigger condition ("many columns") without exposing implementation details.
  • The fix is surgical — only 5 lines added, no unrelated changes pulled in, and the existing early-exit guard at line 79 ensures the new code only runs during virtual scrolling with pending data.
  • Updated the Playwright screenshot baseline alongside the fix, preventing a stale-baseline CI failure.

@yordan-st yordan-st force-pushed the fix/WC-3390_virtual-scroll-bug branch from 673af4d to fe821e3 Compare May 18, 2026 12:42
@r0b1n
Copy link
Copy Markdown
Collaborator

r0b1n commented May 18, 2026

It feels this has to be consolidated with #2167 and addressed together, while those might be different issues, they are caused by the came code which needs an overhaul/thinking though session.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants