DataGrid: refactor applyChangesOnly branch - #34937
Conversation
7042eda to
3adc3e2
Compare
3adc3e2 to
f38316a
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the DataController.applyChangesOnly refresh/update path by extracting row-state comparisons into widget-specific extenders (editing, grouping, summary, TreeList) and by switching from a mutable ChangedRows accumulator to a list-based UpdateRowChange[] flow that is converted into an UpdateChange.
Changes:
- Split
isSameRowStatelogic: keep base value comparison inDataController, and move editing/grouping/summary-specific state comparisons into their respective extenders. - Replace the
markUpdateChange/ChangedRowsbuilding flow inapplyChangesOnlywithUpdateRowChange[]+convertToUpdateChange. - Add Jest coverage for row-change detection across
DataController, editing, grouping, and summary modules (plus small new utilities in grouping/summary).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/grid_core/editing/extenders/editing_data_controller.ts | Adds editing-specific isSameRowState override to include editing flags in row diffing. |
| packages/devextreme/js/__internal/grids/grid_core/editing/extenders/tests/editing_data_controller.row_changes.test.ts | New tests validating editing-flag changes trigger row updates. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/utils/row_changes.ts | Introduces convertToUpdateChange and helper conversion from UpdateRowChange[] to ChangedRows. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/utils/tests/row_changes.test.ts | Updates tests from markUpdateChange to convertToUpdateChange and expands scenarios. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Refactors applyChangesOnly to produce UpdateRowChange[] and centralizes base isSameRowState to value comparison. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/tests/data_controller.row_changes.test.ts | Adds tests validating base isSameRowState behavior (value changes vs no changes). |
| packages/devextreme/js/__internal/grids/grid_core/tests/mock/helpers/row_changes.ts | Adds shared test helper to simulate applyChangesOnly on a single row. |
| packages/devextreme/js/__internal/grids/data_grid/summary/utils.ts | Adds hasSummaryCells helper for summary row-state checks. |
| packages/devextreme/js/__internal/grids/data_grid/summary/extenders/summary_data_controller.ts | Adds summary-specific isSameRowState override (summaryCells + continuation flags). |
| packages/devextreme/js/__internal/grids/data_grid/summary/extenders/tests/summary_data_controller.row_changes.test.ts | New tests covering summary row-state change detection. |
| packages/devextreme/js/__internal/grids/data_grid/summary/tests/utils.test.ts | Adds unit tests for hasSummaryCells. |
| packages/devextreme/js/__internal/grids/data_grid/grouping/utils.ts | Adds grouping state comparison helpers (isSameExpandedState, isSameContinuationState). |
| packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping.ts | Adds grouping-specific isSameRowState override for group rows. |
| packages/devextreme/js/__internal/grids/data_grid/grouping/extenders/tests/grouping_data_controller.row_changes.test.ts | New tests validating grouping row-state change detection (including master-detail guard). |
| packages/devextreme/js/__internal/grids/data_grid/grouping/tests/utils.test.ts | Adds unit tests for the new grouping comparison helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function toChangedRows(updateRowChanges: UpdateRowChange[]): ChangedRows { | ||
| return { | ||
| items: updateRowChanges | ||
| .map(({ item }) => item) | ||
| .filter((item): item is ProcessedItem => !!item), | ||
| rowIndices: updateRowChanges.map(({ rowIndex }) => rowIndex), | ||
| changeTypes: updateRowChanges.map(({ changeType }) => changeType), | ||
| columnIndices: updateRowChanges.map(({ columnIndices }) => columnIndices), | ||
| }; |
There was a problem hiding this comment.
current implementation keeps existing behavior
| import type { ModuleType, OptionChanged } from '@ts/grids/grid_core/m_types'; | ||
|
|
||
| import type { ProcessGroupItemsOptions } from '../../grouping/types'; | ||
| import { isSameContinuationState } from '../../grouping/utils'; |
There was a problem hiding this comment.
since both summary and grouping now consume isSameContinuationState, and it's really the continuation half of isSameGroupRowState, would it fit better in grid_core/data_controller/utils so summary doesn't reach into grouping?
There was a problem hiding this comment.
It is not core functionality - it could be placed in some common utils in data_grid, but still not sure about it - looks like grouping leaks into summary in some methods and vice versa grouping handles some cases for summary. They should be unbound, but it is quite big task and out of scope of this task
Also summary already imports types from grouping module
|
|
||
| import { DATAGRID_GROUP_FOOTER_ROW_TYPE } from './const'; | ||
|
|
||
| export function hasSummaryCells(item: ProcessedItem): boolean { |
There was a problem hiding this comment.
hasSummaryCells reads like it checks item.summaryCells, but it's really a rowType predicate - would isSummaryRow say the intent more directly?
There was a problem hiding this comment.
I agree that the current name is not quite accurate.
If we need to determine which rows can have summary cells, I suggest naming it canHaveSummaryCells.
Alternatively, we could name the method isGroupRow / isGroupOrGroupFooterRow and move it to the grouping utils.
No description provided.