Skip to content

[DataGrid] Prevent React state update before mount#22374

Merged
arminmeh merged 18 commits into
mui:masterfrom
arminmeh:ssr-error
Jun 11, 2026
Merged

[DataGrid] Prevent React state update before mount#22374
arminmeh merged 18 commits into
mui:masterfrom
arminmeh:ssr-error

Conversation

@arminmeh

@arminmeh arminmeh commented May 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #17077

The old approach subscribed during render, so it could catch mount-time store updates immediately, but it could also subscribe to components that never mounted.
That caused the SSR warning.

The new approach subscribes only after commit, so unmounted/suspended renders cannot receive store updates. To avoid stale values, storeState is compared in the first effect pass.

Besides the test for the related issue, I have added tests for Avoid <GridRoot /> double-render pass on mount in SPA mode #15648, and another to test batched updates based on this conversation https://github.com/mui/mui-x/pull/15648/changes#r1937393895 (which also mentions that useSyncExternalStore is not strictly necessary, but covers all requirements).

All tests are failing on the version before #15648 is merged.
Afterwards, only the SSR test is failing, which is a confirmation that they are not false positives and that they can catch future regressions on any of these points.

Before: https://stackblitz.com/edit/github-wpmjdujl-g9fqin9v?file=package.json
After: https://stackblitz.com/edit/github-wpmjdujl-3b5khgfj?file=package.json

CC @lauri865

@arminmeh arminmeh added scope: data grid Changes related to the data grid. type: regression A bug, but worse, it used to behave as expected. feature: Rendering layout Related to the data grid Rendering engine needs cherry-pick The PR should be cherry-picked to master after merge. feature: Server integration Better integration with backends, e.g. data source v8.x labels May 7, 2026
@arminmeh arminmeh changed the title [DataGrid] Prevent Can't perform a React state update on a component that hasn't mounted yet error [DataGrid] Fix Can't perform a React state update on a component that hasn't mounted yet error May 7, 2026
@arminmeh arminmeh changed the title [DataGrid] Fix Can't perform a React state update on a component that hasn't mounted yet error [DataGrid] Prevent React state update before mount May 7, 2026
@code-infra-dashboard

code-infra-dashboard Bot commented May 7, 2026

Copy link
Copy Markdown

Deploy preview

https://deploy-preview-22374--material-ui-x.netlify.app/

Bundle size

Bundle Parsed size Gzip size
@mui/x-data-grid ▼-31B(-0.01%) ▼-72B(-0.06%)
@mui/x-data-grid-pro ▼-12B(0.00%) 🔺+17B(+0.01%)
@mui/x-data-grid-premium ▼-37B(-0.01%) ▼-12B(-0.01%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)
@mui/x-license 0B(0.00%) 0B(0.00%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

@arminmeh
arminmeh marked this pull request as ready for review May 8, 2026 08:27
@arminmeh
arminmeh requested review from cherniavskii and romgrk May 8, 2026 08:27
Comment thread packages/x-data-grid/src/tests/layout.DataGrid.test.tsx

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, so to better understand these changes, I've compared this implementation to the one before #15648 was merged.

The notable changes are:

  1. updateState call on mount, before subscription is active – to pick up store updates missed before mount
  2. storeState comparison in updateState (not sure I understand why – is it really necessary?)
  3. Subscribe in useLayoutEffect instead of useEffect (useEnhancedEffect vs useOnMount).
diff --git a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
index 6680e010ecb..217d27ecf6b 100644
--- a/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
+++ b/packages/x-data-grid/src/hooks/utils/useGridSelector.ts
@@ -1,34 +1,12 @@
+'use client';
 import * as React from 'react';
-import { RefObject } from '@mui/x-internals/types';
+import useEnhancedEffect from '@mui/utils/useEnhancedEffect';
+import type { RefObject } from '@mui/x-internals/types';
 import { fastObjectShallowCompare } from '@mui/x-internals/fastObjectShallowCompare';
 import { warnOnce } from '@mui/x-internals/warning';
 import type { GridApiCommon } from '../../models/api/gridApiCommon';
-import type { OutputSelector } from '../../utils/createSelector';
+import type { GridStateCommunity } from '../../models/gridStateCommunity';
 import { useLazyRef } from './useLazyRef';
-import { useOnMount } from './useOnMount';
-import type { GridCoreApi } from '../../models/api/gridCoreApi';
-
-function isOutputSelector<Api extends GridApiCommon, Args, T>(
-  selector: any,
-): selector is OutputSelector<Api['state'], Args, T> {
-  return selector.acceptsApiRef;
-}
-
-type Selector<Api extends GridApiCommon, Args, T> =
-  | ((state: Api['state']) => T)
-  | OutputSelector<Api['state'], Args, T>;
-
-function applySelector<Api extends GridApiCommon, Args, T>(
-  apiRef: RefObject<Api>,
-  selector: Selector<Api, Args, T>,
-  args: Args,
-  instanceId: GridCoreApi['instanceId'],
-) {
-  if (isOutputSelector(selector)) {
-    return selector(apiRef, args);
-  }
-  return selector(apiRef.current.state, args, instanceId);
-}
 
 const defaultCompare = Object.is;
 export const objectShallowCompare = fastObjectShallowCompare as (a: unknown, b: unknown) => boolean;
@@ -40,7 +18,7 @@ const arrayShallowCompare = (a: any[], b: any[]) => {
   return a.length === b.length && a.every((v, i) => v === b[i]);
 };
 
-export const argsEqual = (prev: any, curr: any) => {
+const argsEqual = (prev: any, curr: any) => {
   let fn = Object.is;
   if (curr instanceof Array) {
     fn = arrayShallowCompare;
@@ -50,72 +28,96 @@ export const argsEqual = (prev: any, curr: any) => {
   return fn(prev, curr);
 };
 
-const createRefs = () => ({ state: null, equals: null, selector: null, args: null }) as any;
+const createRefs = () =>
+  ({ state: null, equals: null, selector: null, args: undefined, storeState: null }) as any;
+
+const EMPTY = [] as unknown[];
 
-export const useGridSelector = <Api extends GridApiCommon, Args, T>(
+type Refs<T> = {
+  // `state` stores the selected value returned by this hook. `storeState` stores the
+  // grid store object that produced it, so the mounted-phase catch-up can detect updates
+  // that happened before the subscription was attached without subscribing during render.
+  state: T;
+  storeState: GridStateCommunity | null;
+  equals: <U = T>(a: U, b: U) => boolean;
+  selector: Function;
+  args: any;
+};
+
+export function useGridSelector<Api extends GridApiCommon, T>(
+  apiRef: RefObject<Api>,
+  selector: (apiRef: RefObject<Api>) => T,
+  args?: undefined,
+  equals?: <U = T>(a: U, b: U) => boolean,
+): T;
+export function useGridSelector<Api extends GridApiCommon, T, Args>(
   apiRef: RefObject<Api>,
-  selector: Selector<Api, Args, T>,
+  selector: (apiRef: RefObject<Api>, a1: Args) => T,
+  args: Args,
+  equals?: <U = T>(a: U, b: U) => boolean,
+): T;
+export function useGridSelector<Api extends GridApiCommon, Args, T>(
+  apiRef: RefObject<Api>,
+  selector: Function,
   args: Args = undefined as Args,
   equals: <U = T>(a: U, b: U) => boolean = defaultCompare,
-) => {
-  if (process.env.NODE_ENV !== 'production') {
-    if (!apiRef.current.state) {
-      warnOnce([
-        'MUI X: `useGridSelector` has been called before the initialization of the state.',
-        'This hook can only be used inside the context of the grid.',
-      ]);
-    }
+) {
+  if (!apiRef.current.state) {
+    warnOnce([
+      'MUI X: `useGridSelector` has been called before the initialization of the state.',
+      'This hook can only be used inside the context of the grid.',
+    ]);
   }
 
-  const refs = useLazyRef<
-    {
-      state: T;
-      equals: typeof equals;
-      selector: typeof selector;
-      args: typeof args;
-    },
-    never
-  >(createRefs);
+  const refs = useLazyRef<Refs<T>, never>(createRefs);
   const didInit = refs.current.selector !== null;
 
   const [state, setState] = React.useState<T>(
     // We don't use an initialization function to avoid allocations
-    (didInit ? null : applySelector(apiRef, selector, args, apiRef.current.instanceId)) as T,
+    (didInit ? null : selector(apiRef, args)) as T,
   );
 
   refs.current.state = state;
+  if (!didInit) {
+    refs.current.storeState = apiRef.current.store.state;
+  }
   refs.current.equals = equals;
   refs.current.selector = selector;
   const prevArgs = refs.current.args;
   refs.current.args = args;
 
   if (didInit && !argsEqual(prevArgs, args)) {
-    const newState = applySelector(
-      apiRef,
-      refs.current.selector,
-      refs.current.args,
-      apiRef.current.instanceId,
-    ) as T;
+    const newState = refs.current.selector(apiRef, refs.current.args) as T;
     if (!refs.current.equals(refs.current.state, newState)) {
       refs.current.state = newState;
       setState(newState);
     }
+    refs.current.storeState = apiRef.current.store.state;
   }
 
-  useOnMount(() => {
-    return apiRef.current.store.subscribe(() => {
-      const newState = applySelector(
-        apiRef,
-        refs.current.selector,
-        refs.current.args,
-        apiRef.current.instanceId,
-      ) as T;
-      if (!refs.current.equals(refs.current.state, newState)) {
-        refs.current.state = newState;
-        setState(newState);
+  const updateState = React.useCallback(
+    () => {
+      const storeState = apiRef.current.store.state;
+
+      if (refs.current.storeState !== storeState) {
+        const newState = refs.current.selector(apiRef, refs.current.args) as T;
+        refs.current.storeState = storeState;
+
+        if (!refs.current.equals(refs.current.state, newState)) {
+          refs.current.state = newState;
+          setState(newState);
+        }
       }
-    });
-  });
+    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    EMPTY,
+  );
+
+  useEnhancedEffect(() => {
+    updateState();
+    return apiRef.current.store.subscribe(updateState);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, EMPTY);
 
   return state;
-};
+}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I should have taken the approach of reverting to the state before #15648 and working from there instead of building on top of the changes that were already there. The code could be more aligned.

Things that can be changed for points 2 and 3.

storeState comparison in updateState (not sure I understand why – is it really necessary?)

It is not strictly necessary, and the benefit depends on the usage of the useGridSelector hook.
That check returns before the selector is even called. In most cases, the selector call will be a call to createSelector/createSelectorMemoized, so the extra call is cheap, but the impact can be greater with selectors made in the files calling the hook, like

const pipesClassName = useGridSelector(apiRef, () =>
  apiRef.current
    .unstable_applyPipeProcessors('cellClassName', [], {
      id: rowId,
      field,
    })
    .filter(Boolean)
    .join(' '),
);

or some other more complex selector created by users.

I am fine dropping this check for more clean code, as this should not create a huge impact.

Subscribe in useLayoutEffect instead of useEffect (useEnhancedEffect vs useOnMount).

Technically, both can work, and in most cases, there will be no difference.
The only difference is in the case when the store is updated synchronously during the commit phase. If that update is related to something the component using the hook renders, there could be a flicker on mount.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I asked Claude to do an extensive analysis of the impact of removing the check. Here is the result

  • The current code comment (lines 37–39) is misleading — it implies storeState is what enables the catch-up to detect pre-mount updates. It isn't. The catch-up detection is done
    by re-reading store.state inside updateState(); storeState is just the "last value I processed" baseline.
  • What the guard actually does: it makes the mount catch-up (and StrictMode's repeated catch-up) a no-op when the store didn't change between render and commit. It does not skip
    selectors for "unaffected subscribers" on real store changes — because Store.setState always creates a new state object, every subscriber's updateState always passes the identity
    check on a real change and re-runs its selector. So the notification-time perf benefit ≈ 0.
  • Is it safe to drop? I had an agent exhaustively scan all 320 useGridSelector call sites. Result: zero match the only pattern where dropping changes behavior (an inline selector
    returning a fresh object/array under default Object.is). All 7 inline selectors return primitives; the lone fresh-object one (GridRow.tsx:243) already passes objectShallowCompare.
    The verifier's counterexample (GridCell:216) was wrong — it ends in .filter(Boolean).join(' '), returning a string.
  • Bottom line: dropping the guard is behavior-neutral for today's codebase (you and the reviewer are right), costing only one redundant — and cheap — selector call per subscriber
    on mount. Keeping it is cheap forward-protection against a future fresh-reference selector + a genuinely nice "catch-up is a clean no-op" property.

So, I would then keep it in there. The description is updated.

Comment thread packages/x-data-grid/src/hooks/utils/useGridSelector.ts Outdated
Comment on lines -126 to -127
useSyncExternalStore(unsubscribe, subscribe, emptyGetSnapshot);

@romgrk romgrk May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For context, useGridSelector was the precursor to the Store's useStore method. We did the reactivity switch for perf gains, but at the time useSyncExternalStore wasn't widely available yet so I re-wrote useGridSelector as a shim (and also for legacy reasons), with the intent to moving to a simpler implementation in the future. Ideally, we should move towards using useStore (and uSES indirectly), not away from it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the details.

In this case, we didn't use useSyncExternalStore as it is intended, since we used the snapshot getter to actually subscribe to the state changes, and we didn't use the callback at all.

This is more of a cleanup than moving away from the uSES.

@lauri865

lauri865 commented May 11, 2026

Copy link
Copy Markdown
Contributor

Been some time, so I don't have the context in my head though, but from what I recall:

  1. The hacky useSyncExternalStore workaround can be avoided altogether if React 17 support can be dropped. useInsertionEffect would catch everything from what I recall. The edge case was when store was updated within ref callbacks on mount, which wasn't caught in the previous setup. So, might be worth to add a test for that.
  2. Might be worth to run the tests with both strict mode enabled and disabled, as there were approaches that worked for one, but didn't work for the other case from what I can recall, as the internal store didn't strictly follow the Rules of React, strict mode can give false positives (again if I recall correctly, which I may not anymore without digging deeper again).

@arminmeh

arminmeh commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Been some time, so I don't have the context in my head though, but from what I recall:

  1. The hacky useSyncExternalStore workaround can be avoided altogether if React 17 support can be dropped. useInsertionEffect would catch everything from what I recall. The edge case was when store was updated within ref callbacks on mount, which wasn't caught in the previous setup. So, might be worth to add a test for that.

useGridSelector.test.tsx → "should catch store updates fired before the selector subscription is attached" covers this

  1. Might be worth to run the tests with both strict mode enabled and disabled, as there were approaches that worked for one, but didn't work for the other case from what I can recall, as the internal store didn't strictly follow the Rules of React, strict mode can give false positives (again if I recall correctly, which I may not anymore without digging deeper again).

Updated the existing test to run in both modes

@arminmeh
arminmeh requested a review from cherniavskii June 5, 2026 08:37
@arminmeh
arminmeh requested a review from romgrk June 5, 2026 08:56

@romgrk romgrk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This hook is a monster and we should get rid of it in favor of useStore, but the current useSyncExternalStore use is indeed a hack. I would highly prefer that we move towards useStore but I don't see any reason to block this PR.

@arminmeh
arminmeh merged commit 2ac30d3 into mui:master Jun 11, 2026
21 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Cherry-pick PRs will be created targeting branches: v8.x

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

Labels

feature: Rendering layout Related to the data grid Rendering engine feature: Server integration Better integration with backends, e.g. data source needs cherry-pick The PR should be cherry-picked to master after merge. scope: data grid Changes related to the data grid. type: regression A bug, but worse, it used to behave as expected. v8.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[data grid] console.error() "Can't perform a React state update on a component that hasn't mounted yet"

4 participants