[DataGrid] Potential fix for grid row state propagation

[lauri865]

• I have followed (at least) the PR section of the contributing guide.

After some further thoughts on #15616, probably better to try and fix the root cause of the row existence checks instead.

@romgrk, what if anything am I breaking here? Fixes the need for try/catch and out of order state propagation (currently on removing a row, first all the cells re-render to null, then the rows unmount. After this change, a row would be responsible for cells being rendered or not).

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[mui-bot]

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

Generated by 🚫 dangerJS against fa8f8cf

[lauri865]

All tests should pass now. To me this approach makes much more sense:

• Baseline row state is owned and provided by rows that render the cells.
• We sprinkle in reactivity to cells based on more atomic selectors, which makes state flow more understandable and debuggable. Currently, all the valueGetters, formatters, etc. have to re-evaluate on each and every state change, which is wasteful.

I was slightly confused why the master detail test started failing, but after looking into it, I'm more worried that it worked the way it did – a breeding ground for race conditions.

The only reason I can think of why the other way around may have some merit is if the cells would be atomically updated based on changing row values. But since all the APIs expose the row to all the cells anyways, that cannot be the case.

Also squeezed in a tiny change that fixes every row selection checkbox re-rendering on every selection model change. Was such a tiny change that I didn't bother making it a new.

[romgrk]

Is moving this block required?

I prefer to keep functional components organized as if it were a class component:

class GridRow {
  constructor() { ... }
  onClick() { ... }
  getCell() { ... }
  render() {}
}

function GridRow {
  /* initialization code */
  const onClick = () => { ... }
  const getCell = () => { ... }
  /* render code */
}

In particular for large components, it helps readability a lot to keep all render code grouped rather than spread in-between "class methods".

[lauri865]

I like grouping logic, but here the balance is between a pretty large dead code path (defining an anyonymous function that should never be called, unless rowNode exists) vs. grouping logic. Not that clear what's more readable.

After this change, if you were to accidentally remove the rowNode check on component-level, the types would break (good imho), which is better than adding an extra conditional in the getCell that would open up the potential for using the compnent in currently unintended/undesigned ways.

Probably the right question here to ask is why the component needs to (try to) mount without a rowNode to begin with.

[romgrk]

Probably the right question here to ask is why the component needs to (try to) mount without a rowNode to begin with.

Absolutely, the answer is often that the codebase went through many architectural & maintainer changes, and we haven't got around to refactoring everything.

[romgrk]

Haven't looked in details, but at first glance this looks wrong to me. This function does state reads, and those should always be wrapped in useGridSelector to enable reactive updates when the state changes. Is there a reason for which this doesn't need to be wrapped? If yes, then we probably need a comment to explain why we don't wrap with useGridSelector.

[lauri865]

That's actually one of the core changes that is a step in the right direction imho, tried to explain it briefly here: #15627 (comment)

The current behaviour casts an incredibly wide net to cell-level reactivity that opens it up to race conditions, and prevents from carefully managing state propagation.

Should cell valueGetter and valueFormatter of each cell in the datagrid (renderecontext) re-evaluate whenever any part of grid state changes? (focus, tabindex, rendercontext, etc.?). They currently do, but I would argue they shouldn't. They should only be re-evaluated if the row changes (through edits or row updates).

Imagine if you define a valueFormatter that formats the cell value based on external state. Changing that external state wouldn't update the rendered cell value, but changing the focus or scrolling a bit would suddenly trigger the cell to re-render with a new value. What?!

Want to understand why the cell re-rendered? Most of the cases, the answer is that getCellParams changed. But why did they change? Good luck understanding that.

So, ideally:

• Row data is provided by row (or editing api)
• Rest of the conditions why the cell should re-render independent of the row should be carefully managed based on atomic selectors. As you can see, the scope of selectors that should trigger cell-level reactivity currently is not that wide in order to make the tests pass. This should result in both more optimised code as well as more understandable state flow within the grid.

[cherniavskii]

Okay, this explains why tabIndex and hasFocus are overridden with the values using useGridSelector.

@romgrk Does this make sense to you?

[romgrk]

Imagine if you define a valueFormatter that formats the cell value based on external state. Changing that external state wouldn't update the rendered cell value, but changing the focus or scrolling a bit would suddenly trigger the cell to re-render with a new value. What?!

The solution to that would be the approach that we've been introducing of adding some props to the grid's store reactivity model. In this case, columns[.valueFormatter] changes should trigger a store update, though I'm a bit surprised it doesn't.

@romgrk Does this make sense to you?

No, it still doesn't. AFAICT, we would be losing reactivity for the cellParams values other than the ones overriden with useGridSelector values. And I see some of them for which it would be problematic, e.g. cellMode or isEditable.

Want to understand why the cell re-rendered? Most of the cases, the answer is that getCellParams changed. But why did they change? Good luck understanding that.

getCellParams has been problematic for a long time (perf-wise and logic-wise), I haven't been able to get rid of it because it would be a breaking change, some of the API uses GridCellParams. I think if we want to fix everything cleanly, we need to decompose that function into more logical parts than "here's every bit of cell data". But that would prevent cherry-picking this PR in v7.

Thoughts?

[lauri865]

I don't think that's quite true that reactivity is lost. A bunch of tests would be failing if that were the case, and cell editing wouldn't work. Any colDef update propagates through the row as it is already. Whereas tabIndex and focus live in unrelated store to columns/rows.

Complete decomposition and granular cell-level reactivity would be a much bigger rewrite. My quick assessment currently is that it's not worth it and would needlessly complicate the code, as there's too many interrelated layers. One would need to ask what are the specific use cases where this would bring meaningful performance gains – I'm personally not seeing it. Maybe editing in theory, but since editing apis expose the whole row for various reasons, that's also not really the case either.

[romgrk]

A bunch of tests would be failing if that were the case

But I don't think we have 100% test coverage, I think we're in the 80s or 90s.

I don't think that's quite true that reactivity is lost.

But there is still nothing that explicitly binds cell reactivity to those values; the binding is more like a coincidental side-effect that might break under edge-cases or unrelated refactors. If you ask me why tabIndex works, it's because it's bound through useGridSelector. If you ask me why isEditable works: I don't know. And I feel uncomfortable merging code that works for unknown reasons.

Complete decomposition and granular cell-level reactivity would be a much bigger rewrite.

It doesn't feel like a big change to me, splitting getCellParams into a few small functions is all it would take. I'll take a look at the code again tomorrow to see if I can do something v7-compatible.

[lauri865]

I agree with the rationale that it shouldn't work due to side-effects. However, isEditable is not the case – every input to isEditable already triggers a re-render through the row->cell tree. It's not a side-effect by any means, but logical flow of state. Adding a selector as well just introduces extraneous work for no benefit. It will simply trigger a re-render from both the Cell within and Row as well.

On a second look, the only thing I actually think needs extracting out is cellMode, which does probably work due to side-effects.

The mental model to me is:

1. Everything in the row -> cell tree that depends directly on the row / cell store shouldn't need additional selectors, as the mere existence of a rendered cell means it's already reactive to these slices of the store.
2. Additional global state that affects how the particular cell is rendered (tabIndex, focus, cellMode) needs selectors.

Does that make sense to you or am I thinking completely wrong here?

[lauri865]

Or actually, cellMode is not even a side-effect – it depends on the same slice of the state that is already selected in the GridRow:

mui-x/packages/x-data-grid/src/components/GridRow.tsx

Line 346 in 2700d7e

editCellState={editCellState}

editCellState prop:

mui-x/packages/x-data-grid/src/components/GridRow.tsx

Line 322 in 2700d7e

const editCellState = editRowsState[rowId]?.[column.field] ?? null;

getCellMode prop depends on the same slice of the state:

mui-x/packages/x-data-grid/src/hooks/features/editing/useGridCellEditing.ts

Lines 254 to 261 in 2700d7e

	const getCellMode = React.useCallback<GridCellEditingApi['getCellMode']>(
	(id, field) => {
	const editingState = gridEditRowsStateSelector(apiRef.current.state);
	const isEditing = editingState[id] && editingState[id][field];
	return isEditing ? GridCellModes.Edit : GridCellModes.View;
	},
	[apiRef],
	);

So, there are two options:

1. cellMode should be a prop of a cell
2. editRowsState selector should be removed from the row, and there should be a editCellState selector withing GridCell. On first look, I don't see why the whole row needs to re-render in response to that, since it's only used to pass a prop to cells, so it could be handled within cells themselves. Should work the same, regardless of whether the whole row is editable or only individual cells.

[romgrk]

As long as it's an explicit reactive binding (through a prop or through a selector) I'm happy with that, but option 2 sounds better. I didn't touch the editing code when I did the reactivity refactor so there's leftover stuff from the old "reactivity" architecture (re-render everything anytime anything changes).

Once the editing state is split into an editCellState selector, all that is left to do would be to split value and formattedValue logic, and we could get rid of getCellParams in GridCell. The other values (id, field, row, rowNode, colDef) are all available through props.

If you want to go ahead with that please do, otherwise I'll see if I can complete this tomorrow. Btw we appreciate all the PRs you submitted, we're a bit short on time with v8 preparation and we've been slow to review & merge them, but they're all great.

[lauri865]

Fixed it up now. Seems to work well. There was one thing that seems to have worked due to side-effects (already before this PR) in the GridRow, moved it to a discrete selector:
Before:

mui-x/packages/x-data-grid/src/components/GridRow.tsx

Line 113 in 878a538

const editing = apiRef.current.getRowMode(rowId) === GridRowModes.Edit;

After:

mui-x/packages/x-data-grid/src/components/GridRow.tsx

Line 127 in 33cc4c9

const editing = useGridSelector(apiRef, gridRowIsEditingSelector, rowId);

There are some additional re-renderings that can be fixed when stopping editing that should speed up editing in large grids, but that should be done when other PRs have been merged around memoization + GridRoot + forwardRef patch.

I'm a bit hesitant to drop getCellParams altogether in GridCell as it opens up an easy divergence between the public API and internal logic. I refactored it a bit to make sure we don't do any unnecessary operations there though, but still keep a solid contract. Unless we need to extract value and formattedValue logic into selector hooks, I don't really see the point either. And I don't currently see a reason why we would need to use hooks there. If we do, we should come up with a test case that it solves, otherwise it's probably just redundant operations.

[cherniavskii]

@romgrk It should be safe to cherry-pick this PR to v7 once we merge it. Any objections?

[romgrk]

See comment above.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[lauri865]

@romgrk, there's one last bug I'd love to fix as a part of this PR (present already before the PR)

If you pin a column to left, you see a visible flash of content:
https://next.mui.com/x/react-data-grid/#commercial-licenses

There's two issues:

• initially RenderContext is out of sync with pinned columns – GridRow tries to render the newly pinned column both as a part of pinnedColumns as well as part of the middle cells. I don't think it's ever painted though, but still extra render pass.
• columnPositions is out of sync with the new ordering for at least 2 render passes (which I guess is the cause of the actual visible flash)

Just quickly playing around, this seems to fix the 2nd issue, but I'm not sure if there's a better way:

const visibleColumnsSelector = React.useCallback(
    () => () =>
      listView
        ? [gridListColumnSelector(apiRef.current.state)!]
        : gridVisibleColumnDefinitionsSelector(apiRef),
    [apiRef, listView],
  );
const visibleColumns = useGridSelector(apiRef, visibleColumnsSelector);

Any thoughts?

[lauri865]

I actually don't even understand why the above works. How can selector even be a function that returns a function? Was a copy-paste mistake, but it still returns visible columns.

What does work however is to call hydrateColumns directly in pinColumn and unpinColumn:

apiRef.current.setPinnedColumns(newPinnedColumns);
apiRef.current.requestPipeProcessorsApplication('hydrateColumns');

Seems to fix both of the issues. I'm not sure what's currently even calling the hydration, but it happens a couple of render passes later.

[lauri865]

Aha, I understand what's happening currently (I think). Pinned columns change -> changes viewportInnerSize in useGridDimensions -> triggers handleGridSizeChange in useGridColumns -> triggers hydrateColumns. Quite complex chain.

With the above change now hydrateColumns would be triggered twice I guess though. So, I wonder if we can get rid of that somehow as well.

[lauri865]

After more debugging, I finally understand what trigger column hydration here.

1. useGridColumnPinningPreProcessors
2. Dependency array of reorderPinnedColumns changes
3. useGridRegisterPipeProcessor registers registerPreProcessor in useEffect (crux of the issue)
4. registerPipeProcessor in useGridPipeProcessing detects that processor has changed
5. Triggers runAppliers that triggers hydrateColumns

I will make triggering hydrateColumns more explicit to fix this, and make the callback stable.

[lauri865]

Ok, managed to fix this. With the memoization PR together, changing pinning will now happen in one render pass, instead of 3-4 I think (meaning all rows re-rendered 4 times due to it).

I also refactored useGridColumns to rehydrate column widths based on viewportOuterWidth instead of innerWidth. innerWidth can change independent of outerWidth only due to pinnedColumns, and what happens is we rehydrate columns twice when pinned columns change, triggering unnecessary re-rendering and operations.

Please take a look when you have a moment @romgrk 🙏

Edit: will check tomorrow why the flex col tests started failing

[lauri865]

The last fix I added as a part of this PR could be considered breaking, although could also potentially be classified as a bug.

viewportInnerSize.width currently is calculated by subtracting pinnedColumns, except for the autoHeight implementation, which doesn't (and which I would consider the right of the two approaches, both semantically and functionality-wise, unless I'm missing something). I wonder if it is legacy artefact that's been left in when pinnedColumns were actually rendered in separate containers / rows.

This has two negative implications:

• Potentially recursive event loop (viewportInnerSize.width depends on column widths and columnWidths depend on viewportInnerSize.width). I think it could be the root cause of some maximum update depth errors that have been popping up that have so far been fixed with dimension rounding instead.
• Plus any pinned column that gets resized, added or removed will trigger re-evaluation of column widths twice

None of the internal consumers actually seem to depend on the width without pinned columns, but rather on the actual inner viewport width. Meaning that there's likely many unnecessary operations happening when pinned columns are present, resized or modified.

---

On a slightly different note. It also seems like the below code doesn't seem to serve any function, as it always returns 0 for cellOffset, at least after the state propagation has been fixed. I left it in for the time being, as it does serve some styling function for us to detect last/first columns in respective pinned sections in CSS. However, by simply adding firstPinnedColumn and lastPinnedColumn classes, the prop + dom element + calculation can probably be removed in v8. Unless there's an additional function to it that I'm not seeing?

<div
  role="presentation"
  className={gridClasses.cellOffsetLeft}
  style={{ width: offsetLeft }}
/>