[DataGrid] Fix crash when updating columns immediately after scrolling #13781
Conversation
cherniavskii
added
component: data grid
|
Deploy preview: https://deploy-preview-13781--material-ui-x.netlify.app/ |
| if (!column) { | ||
| // See https://github.com/mui/mui-x/issues/13719 | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This feels like it hides something.
| 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);
};
test/e2e/index.test.ts
Outdated
|
|
||
| await page.click('"Update columns"'); | ||
|
|
||
| await sleep(200); |
There was a problem hiding this comment.
is there a way to use some grid's callback to check if the error happened instead of waiting for some fixed time?
There was a problem hiding this comment.
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()).
There was a problem hiding this comment.
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 👍🏻
test/e2e/index.test.ts
Outdated
| 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"'); |
There was a problem hiding this comment.
Nitpick but I would group the page interactions together, shows more clearly the sequence of events that trigger the bug.
Fixes #13719
I believe this regression was introduced in #12353
I cannot reproduce the issue if I change this timeout to 100:
mui-x/packages/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
Line 272 in c6d8bf4