[DataGrid] Fix noRowsOverlay flicker between dataSource re-fetches#22465
Conversation
The navigation event handlers cleared rows synchronously before queueing the fetch. Because `setRows` rebuilds `state.rows` from `props.loading`, the synchronous clear also reset `loading` to false, exposing a render with `rows = []` and `loading = false` that triggered `noRowsOverlay`. The flash was most visible when navigating between already-cached pages. Now the handlers call `setLoading(true)` after `setRows([])`, so the loading state survives the `setRows` rebuild and the overlay selector picks `loadingOverlay` instead. Add the `dataSourceKeepPreviousData` prop to opt into keeping the previously displayed rows visible during the fetch (mirrors React Query's `placeholderData: keepPreviousData`). Fixes mui#22458 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
LukasTy
added
scope: data grid
LukasTy
changed the title
noRowsOverlay flicker between dataSource re-fetchesnoRowsOverlay flicker between dataSource re-fetches
…ousData` The error path in `handleDataUpdate` and `handleGroupedDataUpdate` cleared rows unconditionally, so a rejected refetch wiped the last known-good data even when `dataSourceKeepPreviousData` was on — the opposite of what users opt into the prop for. Gate both `setRows([])` calls behind the prop so a failed refetch leaves the previous rows visible while `onDataSourceError` delivers the error. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Hey Lukas, Im the original reporter of this issue. I figured I'll share my thinking here instead of my own PR since this one is more comprehensive anyway. Im personally happy with this approach and would love to have any resolution honestly. Just two things to consider:
<DataGrid
dataSource={{
fetchRows: ...,
keepPreviousData: true
}}The second is more subjective, but for the first I'll offer an alternative. You can keep the previous behavior (don't persist previous data) and get rid of the flash by removing the "debounce" on the My ideal solution would be to do both. Remove debounce (if acceptable) to improve default behavior for everyone out of the box and add this new prop for people like me who prefer keeping previous rows mounted for aesthetic purposes. |
|
@GMchris thank you for the response. In regards to the |
siriwatknp
left a comment
siriwatknp
left a comment
There was a problem hiding this comment.
👍 Thanks for taking care of this, here is what I found:
(1) setLoading(true) in handleRowGroupingModelChange looks asymmetric with the clear paths.
(2) Test misses state.rows.loading assertion.
(3) Guard on setRows([]) for non-standard strategies — is it intentional?
| if (!props.dataSourceKeepPreviousData) { | ||
| apiRef.current.setRows([]); | ||
| } | ||
| apiRef.current.setLoading(true); |
There was a problem hiding this comment.
(1) Is there a reason setLoading(true) is called unconditionally here?
From what I traced, the setLoading(false) paths in fetchRows (cache-hit at useGridDataSourceBase.ts:128-135 and finally at :182-187) only run when standardRowsUpdateStrategyActive.
Switching rowGroupingModel from ['a'] to ['b'] while the grouped strategy stays active sets loading true and never clears it — overlay stays on top of grouped rows.
The community handleFetchRowsOnParamsChange (useGridDataSourceBase.ts:377-389) guards both setRows([]) and setLoading(true) with the same standardRowsUpdateStrategyActive check. This handler is asymmetric.
Could wrap both calls in the same guard:
| apiRef.current.setLoading(true); | |
| if (standardRowsUpdateStrategyActive) { | |
| setLoading(true); | |
| } |
Or if loading must be set for grouped mode, please add a clear path in the cache-hit branch and finally too.
Either way, please add a Premium grouping regression test.
There was a problem hiding this comment.
Addressed by adding an inline strategy check in useGridDataSourcePremium. The event listener is wired without a strategy gate (runIf(!pivotActive && !!props.dataSource, ...)), so the handler can fire while a non-standard strategy is active. fetchRows only clears the loading flag when a standard strategy is active, so setLoading(true) could otherwise stick.
const handleRowGroupingModelChange = React.useCallback(() => {
// The event listener is wired regardless of the active strategy
// (see `runIf(!pivotActive && !!props.dataSource, ...)` below), so we have to gate the
// `setLoading(true)` here. `fetchRows` only clears the loading flag when a standard
// strategy is active, and we don't want to leave the overlay stuck on lazy-loading or
// other non-standard strategies.
const currentStrategy = apiRef.current.getActiveStrategy(GridStrategyGroup.DataSource);
const isStandardStrategyActive =
currentStrategy === DataSourceRowsUpdateStrategy.Default ||
currentStrategy === DataSourceRowsUpdateStrategy.GroupedData;
if (isStandardStrategyActive) {
if (!props.dataSourceKeepPreviousData) {
apiRef.current.setRows([]);
}
apiRef.current.setLoading(true);
}
stopPolling();
debouncedFetchRows();
}, [apiRef, props.dataSourceKeepPreviousData, debouncedFetchRows, stopPolling]);Also added two regression tests in dataSourceRowGrouping.DataGridPremium.test.tsx covering the loading-state toggle on grouping model change and the keep-previous-data behavior.
…mmetry The community handler had a redundant `if (standardRowsUpdateStrategyActive)` guard inside the callback even though the event listener is already wired with `runIf(standardRowsUpdateStrategyActive, ...)`. Drop the inner guard and add a comment so the gating intent is explicit. This also restores the pre-PR behavior of unconditionally clearing rows on non-standard strategies (the outer `runIf` skips the handler entirely instead). The Premium row-grouping handler is wired without a strategy check at the event level, so it could call `setLoading(true)` while `fetchRows` would not call the matching `setLoading(false)` (cleared only on standard strategies). Add an inline strategy check there so the loading flag is only flipped when `fetchRows` will eventually clear it again. Tests: - Add `state.rows.loading === false` assertion to the error-recovery test. - Add Premium row-grouping regression tests that exercise the loading toggle and the keep-previous-data behavior on grouping model change. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
| ### Keep previous data while fetching | ||
|
|
||
| By default, the Data Grid clears the visible rows and shows a loading overlay while it fetches new data after pagination, sorting, filtering, or row grouping changes. | ||
| Pass the `dataSourceKeepPreviousData` prop to keep the previously displayed rows visible until the next response arrives. |
There was a problem hiding this comment.
I assume the core problem (no rows flicker) would solved by setting the loading state after resetting the rows. Is that right?
In that case, I'd suggest to skip this prop for now or at least take this separately in order to make sure it doesn't causes issues like #21619 with nested data anymore.
There was a problem hiding this comment.
Indeed. Fixing the actual issue is fairly trivial and minimal.
The suggestion to split efforts makes a lot of sense. 👌
There was a problem hiding this comment.
@MBilalShafi, I have updated the solution in this PR to only fix the root issue.
MBilalShafi
changed the title
noRowsOverlay flicker between dataSource re-fetchesnoRowsOverlay flicker between dataSource re-fetches
MBilalShafi
added
the
feature: Server integration
Per review on mui#22465 (mui#22465 (comment)), the opt-in prop has the same shape as the bug mui#21619 fixed: skipping the synchronous `setRows([])` would leave tree-data sort/filter changes rendering rows in stale order until the user collapsed/re-expanded. Keep this PR focused on the reported `noRowsOverlay` flicker and ship the controllable prop in a follow-up that handles the tree-data interaction. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
LukasTy
added
type: bug
There was a problem hiding this comment.
Pull request overview
Fixes a transient noRowsOverlay flash when using the DataGrid dataSource API by ensuring the grid remains in a loading state while rows are temporarily cleared during query-parameter changes (pagination/sort/filter/row-grouping), including cache-hit scenarios.
Changes:
- Set
loading=trueimmediately aftersetRows([])on params changes to prevent a one-framerows=[] && loading=falsestate. - Clear the loading flag on cache-hit updates so synchronous cached responses don’t leave the grid stuck in a loading state.
- Add unit tests covering cache-hit, cache-miss, and empty-response overlay behavior, plus a Premium row-grouping loading-state test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/x-data-grid/src/hooks/features/dataSource/useGridDataSourceBase.ts | Adjusts loading state transitions around setRows([]) and cache-hit response application. |
| packages/x-data-grid-premium/src/hooks/features/dataSource/useGridDataSourcePremium.tsx | Updates row-grouping change handler to conditionally toggle loading based on active strategy. |
| packages/x-data-grid/src/tests/dataSource.DataGrid.test.tsx | Adds regression tests to ensure noRowsOverlay does not render between re-fetches (cache hit/miss) and still renders for empty results. |
| packages/x-data-grid-premium/src/tests/dataSourceRowGrouping.DataGridPremium.test.tsx | Adds a test validating loading state toggling during row-grouping model changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two fixes flagged in mui#22465 (review): 1. The cache-hit early-return in `fetchRows` did not bump `lastRequestId`, so a previously started cache-miss request could resolve after a cache-hit navigation and override the cached data with stale rows. Bump `lastRequestId.current` in the cache-hit branch so any in-flight request is treated as stale when it resolves. Add a regression test that asserts a stale page-1 response does not override the cached page-0 data. 2. `handleRowGroupingModelChange` in the Premium hook now wraps both `setRows([])` and `setLoading(true)` in the `isStandardStrategyActive` block — the PR description only intended to gate the loading toggle. Restore the pre-PR unconditional `setRows([])` so non-standard strategies (lazy loading) still reset their grouping state on rowGroupingModel change. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…w-grouping test Per mui#22465 (review), `resolveSecond(...)` triggers React state updates (the awaiting `fetchRows` completes and calls `setRows` + `setLoading(false)`) while the grid is mounted, so it has to be wrapped in `act()` to avoid "not wrapped in act" warnings under vitest-fail-on-console. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
mui#22465) Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Fixes #22458
Closes #22460.
Summary
Fixes the brief "No rows" overlay flash that appears between pagination, sort, filter, and row-grouping changes when using the
dataSourceAPI. The flash is especially visible on cached navigation, where the response is available synchronously but a stale empty render still appears for a frame.Root cause
The navigation event handlers (
handleFetchRowsOnParamsChangeinuseGridDataSourceBaseandhandleRowGroupingModelChangeinuseGridDataSourcePremium) calledapiRef.current.setRows([])synchronously and queueddebouncedFetchRows()afterwards. InsideuseGridRows,setRowsrebuildsstate.rowsfromprops.loadingviagetRowsStateFromCache. BecausedataSourcegrids don't pass aloadingprop, that rebuild resetstate.rows.loadingtoundefined, exposing a render withrows = []andloading = false. The overlay selector then pickednoRowsOverlayfor that single frame.Fix
In both navigation handlers, call
setLoading(true)immediately aftersetRows([])so the loading flag survives thesetRowsrebuild. The first render after the click now hasrows = []andloading = true, which picksloadingOverlay(skeleton variant) instead ofnoRowsOverlay. The cache-hit branch offetchRowsnow also callssetLoading(false)so the loading flag the handler just set is cleared when the cached response is applied synchronously.In the Premium row-grouping handler, the event listener is wired without a strategy gate, so the
setLoading(true)is wrapped in an inline strategy check —fetchRowsonly clears the loading flag when a standard strategy is active, so we mustn't flip it on for lazy-loading or other non-standard strategies.Follow-up
The original draft of this PR also added a
dataSourceKeepPreviousDataopt-in prop. Per the review on this PR, that interacts badly with the tree-data fix from #21619 (skipping the synchronoussetRows([])would leave sort/filter changes rendering rows in stale order). The prop is moved to a follow-up PR that handles the tree-data interaction properly.Test plan
dataSource.DataGrid.test.tsxcovering cache-hit, cache-miss, and empty-response cases.pnpm test:unit --project "x-data-grid*" --runclean.pnpm typescriptclean across the monorepo.pnpm eslint,pnpm prettier,pnpm markdownlintclean.