Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Fix crash when updating columns immediately after scrolling #13781

Merged
merged 9 commits into from
Jul 29, 2024
Merged

[DataGrid] Fix crash when updating columns immediately after scrolling #13781

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c3c7ba8
add test
cherniavskii Jul 9, 2024
c6d8bf4
fix failing test
cherniavskii Jul 9, 2024
639483b
reset frozen context on columns change
cherniavskii Jul 10, 2024
8222ac7
rename component
cherniavskii Jul 10, 2024
4c53949
Merge branch 'master' into dynamic-virtualization-range-update-columns
cherniavskii Jul 11, 2024
4e4e5f7
Merge branch 'master' into dynamic-virtualization-range-update-columns
cherniavskii Jul 29, 2024
fcf353b
add test in karma
cherniavskii Jul 29, 2024
a52f7f6
empty
cherniavskii Jul 29, 2024
b460348
skip in jsdom
cherniavskii Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/x-data-grid/src/components/GridRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
}

const getCell = (
column: GridStateColDef,
column: GridStateColDef | undefined,
indexInSection: number,
indexRelativeToAllColumns: number,
sectionLength: number,
Expand All @@ -357,6 +357,11 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
indexRelativeToAllColumns,
);

if (!column) {
// See https://github.com/mui/mui-x/issues/13719
return null;
}
Copy link
Member

@oliviertassinari oliviertassinari Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This feels like it hides something.

Suggested change
if (!column) {
// See https://github.com/mui/mui-x/issues/13719
return null;
}

Looking a bit at it, it seems that the issue is that the frozen render context isn't reset when the column changes.

diff --git a/packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx b/packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
index 334d65759..d87187caa 100644
--- a/packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
+++ b/packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
@@ -277,6 +277,7 @@ export const useGridVirtualScroller = () => {
   const forceUpdateRenderContext = () => {
     const inputs = inputsSelector(apiRef, rootProps, enabled, enabledForColumns);
     const nextRenderContext = computeRenderContext(inputs, scrollPosition.current, scrollCache);
+    // Reset the frozen context when the render context changes, see the illustration in https://github.com/mui/mui-x/pull/12353
+    frozenContext.current = undefined;
     updateRenderContext(nextRenderContext);
   };

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!


if (cellColSpanInfo?.spannedByColSpan) {
return null;
}
Expand Down
28 changes: 28 additions & 0 deletions test/e2e/fixtures/DataGrid/DynamicVirtualizationRange.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import * as React from 'react';
import { DataGrid, GridColDef } from '@mui/x-data-grid';
import Button from '@mui/material/Button';

const rows = [
{ id: 1, value: 'A' },
{ id: 2, value: 'B' },
{ id: 3, value: 'C' },
{ id: 4, value: 'D' },
{ id: 5, value: 'E' },
{ id: 6, value: 'E' },
{ id: 7, value: 'F' },
{ id: 8, value: 'G' },
{ id: 9, value: 'H' },
];

export default function CheckboxSelectionGrid() {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
const [columns, setColumns] = React.useState<GridColDef[]>([{ field: 'id' }, { field: 'value' }]);

return (
<div style={{ width: '100%' }}>
<Button onClick={() => setColumns([{ field: 'id' }])}>Update columns</Button>
<div style={{ height: 400 }}>
<DataGrid rows={rows} columns={columns} />
</div>
</div>
);
}
18 changes: 18 additions & 0 deletions test/e2e/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,24 @@ async function initializeEnvironment(
expect(groupHeaderWidth).to.equal(groupHeaderColumnsTotalWidth);
expect(subGroupHeaderWidth).to.equal(subGroupHeaderColumnsTotalWidth);
});

it('should not crash when updating columns immediately after scrolling', async () => {
await renderFixture('DataGrid/DynamicVirtualizationRange');

await page.mouse.move(200, 200);
await page.mouse.wheel(0, 1000);

let thrownError: Error | null = null;
context.once('weberror', (webError: WebError) => {
thrownError = webError.error();
console.error(thrownError);
});

await page.click('"Update columns"');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but I would group the page interactions together, shows more clearly the sequence of events that trigger the bug.


await sleep(200);
Copy link
Contributor

@arminmeh arminmeh Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is there a way to use some grid's callback to check if the error happened instead of waiting for some fixed time?

Copy link
Member

@oliviertassinari oliviertassinari Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It feels like an end to end test is a bit too heavy (slow) for this bug. It seems that to reproduce, it's a matter of scrolling down to create a frozen context and update the columns to break part of the frozen context. So I would expect a Karma test to run significantly faster (would be curious to measure the test speed difference, maybe I'm dillusional).

It seems to break instantly as well, without the need to sleep (needs an act()).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, karma is faster.
Here are the results for running the same test 100 times:

  • Karma:

     Chrome Headless 125.0.6422.26 (Mac OS 10.15.7): Executed 100 of 6802 SUCCESS (5.168 secs / 4.887 secs)

  • Playwright:

     Running on: chromium, version: 125.0.6422.26.
   100 passing (15s)

So Karma is ~3x faster.
I've added a test in Karma instead of the e2e test 👍🏻

expect(thrownError).to.equal(null);
});
});

describe('<DatePicker />', () => {
Expand Down
Loading