Fix crash with large grid and browser zoom #1052

lukasmasuch · 2025-06-19T20:17:46Z

There is an issue with the infinity scroll optimization, which supports scroll areas that exceed the max height of divs (33.5 million pixels). If you have a large grid and you change the device pixel ratio (e.g., via zoom out), the grid allows overscrolling and eventually runs into an out-of-memory error (reproducible with the silly numbers demo in storybook)

Screen.Recording.2025-06-19.at.22.18.44.mov

This PR fixes this issue by improving the scroll optimization logic.

Copilot

Pull Request Overview

This PR prevents out-of-memory errors with very large grids by clamping grid indices and improving the infinite scrolling logic to account for browser height limits and device pixel ratio changes.

Clamp cellY/cellBottom to valid row bounds in the grid scroller.
Track a virtual scroll position, reset on DPR (zoom) change, and map DOM scroll to full content height.
Split large padders into smaller segments to avoid browser div height limits.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
packages/core/src/internal/scrolling-data-grid/scrolling-data-grid.tsx	Clamps `cellY` and `cellBottom` to `[0, rows)` to prevent out‐of‐bounds row access.
packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx	Adds `virtualScrollY`, DPR‐change effect, refined delta/edge scroll mapping, and segmented padders under max height.

Comments suppressed due to low confidence (2)

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx:303

[nitpick] The variable h is ambiguous; rename it to something more descriptive like accumulatedHeight or remainingHeight for clarity.

    let h = 0;

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx:154

Add tests to verify that virtualScrollY and lastScrollY are correctly reset when window.devicePixelRatio changes (e.g., simulate zoom in/out).

    React.useEffect(() => {

packages/core/src/internal/scrolling-data-grid/scrolling-data-grid.tsx

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx

Copilot · 2025-06-19T20:24:43Z

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx

            }

+            // Ensure virtual Y is within valid bounds
+            virtualY = Math.max(0, Math.min(virtualY, scrollHeight - cHeight));
+            virtualScrollY.current = virtualY; // Keep tracked position in bounds too


This assignment duplicates updates inside the conditional branches; consider consolidating the update into a single place after clamping to avoid redundant writes.

lukasmasuch · 2025-06-19T20:25:34Z

packages/core/src/internal/scrolling-data-grid/scrolling-data-grid.tsx

+        // This is a safeguard to prevent unexpected out-of-bounds access with large datasets
+        cellY = Math.max(0, Math.min(cellY, rows - 1));
+        cellBottom = Math.max(cellY, Math.min(cellBottom, rows));
+


not necessary but an additional safeguard to prevent unexpected crashes.

…er.tsx Co-authored-by: Copilot <[email protected]>

BrianHung · 2025-06-19T23:40:23Z

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx

@@ -256,9 +304,21 @@ export const InfiniteScroller: React.FC<Props> = p => {

    let key = 0;
    let h = 0;
+
+    // Browser's maximum div height limit
+    const maxDomHeight = BROWSER_MAX_DIV_HEIGHT;


nit: just use BROWSER_MAX_DIV_HEIGHT directly

changed 👍

BrianHung

LGTM

BrianHung · 2025-06-19T23:42:51Z

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx

@@ -70,6 +70,9 @@ const ScrollRegionStyle = styled.div<{ isSafari: boolean }>`
    }
 `;

+const BROWSER_MAX_DIV_HEIGHT = 33_554_432;


https://stackoverflow.com/questions/16637530/whats-the-maximum-pixel-value-of-css-width-and-height-properties

Firefox: 33_554_400px
Chrome: 33_554_428px
Opera: 33_554_428px

Should we just use lowest / firefox constant?

I reduced it to: 33_554_400 👍

Would test in firefox with storybook as a smoke test.

I'm not too familiar with storybook smoke tests. Do we already have something like that in the GDG repo? Or do you mean just doing manual tests with with firefox and storybook?

Meant manual in firefox for now!

Sounds good 👍 I did some additional manual testing with chrome, firefox and safari and everything seems to work fine. There is still a floating-point precision issue, which sometimes prevents the grid from scrolling to exactly the last row with very large grids. But this already existed in the previous version as well. I might try a follow-up fix for this as well.

BrianHung · 2025-06-19T23:47:55Z

packages/core/src/internal/scrolling-data-grid/infinite-scroller.tsx

@@ -70,6 +70,9 @@ const ScrollRegionStyle = styled.div<{ isSafari: boolean }>`
    }
 `;

+const BROWSER_MAX_DIV_HEIGHT = 33_554_432;
+const MAX_PADDER_SEGMENT_HEIGHT = 5_000_000;


This is just an arbitrary constant under the BROWSER_MAX_DIV_HEIGHT?

5_000_000 was the current value, I think it's mostly arbitrary... but not 100% sure about that. I added a bit more comment above MAX_PADDER_SEGMENT_HEIGHT to explain what it is actually used for.

Yeah, I think it is arbitrary too, just to not have too many padders if we get to BROWSER_MAX_DIV_HEIGHT.

…crash

Fix crash with large grid and browser zoom

da488d1

lukasmasuch requested a review from Copilot June 19, 2025 20:22

lukasmasuch added 2 commits June 19, 2025 22:22

Fix

7b37c93

Simplify

468dbac

Copilot AI reviewed Jun 19, 2025

View reviewed changes

Update comment

b843936

lukasmasuch commented Jun 19, 2025

View reviewed changes

lukasmasuch and others added 2 commits June 19, 2025 22:28

Update packages/core/src/internal/scrolling-data-grid/infinite-scroll…

e706697

…er.tsx Co-authored-by: Copilot <[email protected]>

Improvements

841f01a

BrianHung reviewed Jun 19, 2025

View reviewed changes

BrianHung approved these changes Jun 20, 2025

View reviewed changes

lukasmasuch added 3 commits June 25, 2025 00:53

Apply comments

a3a626f

Update comment

e14f9a6

Merge remote-tracking branch 'upstream/main' into fix/large-grid-dpr-…

b95d29a

…crash

lukasmasuch merged commit ed43f13 into main Jun 25, 2025
8 checks passed

lukasmasuch deleted the fix/large-grid-dpr-crash branch June 25, 2025 10:01

Fix crash with large grid and browser zoom #1052

Fix crash with large grid and browser zoom #1052

Uh oh!

Conversation

lukasmasuch commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrianHung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasmasuch Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukasmasuch commented Jun 19, 2025 •

edited

Loading

lukasmasuch Jun 24, 2025 •

edited

Loading