GridCore - CustomLoadPipeline - Refactor - #35002
Open
Tucchhaa wants to merge 6 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the GridCore custom-load pipeline by routing custom store loads through a dedicated customLoader instance and renaming the DataController “load all” API to better reflect its behavior.
Changes:
- Replace direct
dataSource.load({...})custom-load calls withdataSource.customLoader.load({...})across GridCore/DataGrid/TreeList. - Rename
DataController.loadAll→loadAllItemsand update internal usages + tests accordingly. - Adjust adapters/tests to use
customLoader.loadFromStore,customLoader.loadAll, andcustomLoader.processLoadedData.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js | Updates QUnit coverage to call the refactored custom-load entrypoint via customLoader.load. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataController.tests.js | Renames loadAll usages in QUnit tests to loadAllItems. |
| packages/devextreme/js/__internal/grids/tree_list/m_focus.ts | Switches focus-related data fetches to customLoader.load. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts | Routes store loads through customLoader.loadFromStore / customLoader.load. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.test.ts | Updates Jest test mocking to hook customLoader.loadFromStore. |
| packages/devextreme/js/__internal/grids/tree_list/ai_assistant/commands/selection.integration.test.ts | Updates test wording to match loadAllItems rename. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts | Replaces custom-loading checks with customLoader.isLoading()/isLoadingAll() and simplifies adapter load() override. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/extenders/virtual_scrolling_data_controller.ts | Uses customLoader.isLoading() to detect custom loading during virtual scrolling updates. |
| packages/devextreme/js/__internal/grids/grid_core/selection/m_selection.ts | Uses customLoader.load when selection logic needs a custom store load. |
| packages/devextreme/js/__internal/grids/grid_core/m_utils.ts | Uses customLoader.load for unique relevant-item loading under grouping/lookup scenarios. |
| packages/devextreme/js/__internal/grids/grid_core/header_filter/m_header_filter.ts | Uses customLoader.load for header filter data retrieval. |
| packages/devextreme/js/__internal/grids/grid_core/focus/m_focus.ts | Uses customLoader.load for focus-related item resolution. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts | Exposes customLoader and removes adapter wrapper methods in favor of direct customLoader usage. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/custom_loader.ts | Refactors load operation creation + result handling inside the custom-load pipeline. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/tests/custom_loader.test.ts | Updates CustomLoader Jest tests to use the internal DataSource class and new types. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Renames loadAll → loadAllItems and routes “load all” and “process preloaded data” through customLoader. |
| packages/devextreme/js/__internal/grids/grid_core/ai_assistant/commands/tests/selection.test.ts | Updates Jest tests/comments to reference loadAllItems. |
| packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_expanded.ts | Uses customLoader.load for totalCount retrieval under grouping. |
| packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_collapsed.ts | Uses customLoader.loadFromStore and customLoader.load in collapsed grouping flows. |
| packages/devextreme/js/__internal/grids/data_grid/focus/m_focus.ts | Uses customLoader.load in DataGrid focus extender. |
| packages/devextreme/js/__internal/grids/data_grid/export/m_export.ts | Switches exporting flow to call loadAllItems. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
added 3 commits
September 2, 2026 16:27
Tucchhaa
force-pushed
the
refactor_custom_load_pipeline_26_2
branch
from
September 2, 2026 09:57
440d11a to
47662f8
Compare
Tucchhaa
commented
Sep 2, 2026
| | 'customizeLoadResult' | ||
| | 'changing'; | ||
|
|
||
| export interface DataSource extends PublicDataSource { |
Contributor
Author
There was a problem hiding this comment.
I have replaced usages of this temp interface by the DataSource class
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several updated QUnit tests and the grid test mocks still treat customLoader.load as if it resolves the raw data array instead of { data, extra }, which will cause failures until corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (11)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:6985
customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
This issue also appears in the following locations of the same file:
- line 7022
- line 7064
- line 7106
- line 7144
- line 7187
- ...and 5 more
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7029
customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7071customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7110customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7151customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7193customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7235customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7277customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7305customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7396customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7448customLoader.loadresolves with a{ data, extra }object; this test currently assigns the whole result tocustomLoadData, so the subsequent deepEqual assertions compare an object to an array.
- Files reviewed: 36/36 changed files
- Comments generated: 2
- Review effort level: Lite
Tucchhaa
commented
Sep 2, 2026
| } | ||
|
|
||
| export interface ItemOperationOptions extends ItemChangeOptions { | ||
| newItems: ProcessedItem[]; |
Contributor
Author
There was a problem hiding this comment.
it wasn't used anywhere
added 2 commits
September 2, 2026 18:32
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Several DataGrid QUnit tests still treat customLoader.load().done(...) as returning the raw data array (instead of { data, extra }), and grouping total-count validation now fails to reject undefined/NaN counts.
Review details
Suppressed comments (12)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:6985
- CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the data array. This will make assertions compare against the wrapper object instead of the loaded data.
This issue also appears in the following locations of the same file:
- line 7022
- line 7064
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7029
- CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the data array. This will make assertions compare against the wrapper object instead of the loaded data.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7071 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the data array. This will make assertions compare against the wrapper object instead of the loaded data.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7110 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
This issue also appears in the following locations of the same file:
- line 7144
- line 7187
- line 7229
- line 7271
- line 7299
- ...and 2 more
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7151
- CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7193 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7235 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7277 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7305 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7396 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7448 - CustomLoader.load resolves with a single { data, extra } object, but this test still treats the first done() argument as the loaded items array.
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_collapsed.ts:344 - The count validation no longer rejects undefined/NaN results (because it’s gated by
count &&). With requireTotalCount/requireGroupCount enabled, missing/non-finite counts should still be treated as an error to avoid propagating invalid group sizes.
dataSource.customLoader.load(loadOptions).done(({ extra }) => {
const count = extra && (isGrouping ? extra.groupCount : extra.totalCount);
if (count && !isFinite(count)) {
d.reject(dataErrors.Error(isGrouping ? 'E4022' : 'E4021'));
return;
}
d.resolve(count);
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a broad cross-cutting refactor of core grid loading semantics (including result-shape changes) and should get a final human review despite tests being updated.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_collapsed.ts:355
- The
@ts-expect-errormessage has a typo (“typedDataSourceAdapter”); add the missing space for consistency with other suppression comments.
This issue also appears on line 386 of the same file.
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_expanded.ts:385
- The
@ts-expect-errormessage has a typo (“typedDataSourceAdapter”) which reduces readability and is inconsistent with nearby comments; add the missing space.
This issue also appears in the following locations of the same file:
- line 387
- line 425
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_expanded.ts:388
- The
@ts-expect-errormessage has a typo (“typedDataSourceAdapter”); add the missing space for clarity.
// @ts-expect-error badly typedDataSourceAdapter.pageSize
const offset = correctSkipLoadOption(that, beginPageIndex * dataSource.pageSize());
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_expanded.ts:426
- The
@ts-expect-errormessage has a typo (“typedDataSourceAdapter”); add the missing space to keep the comment readable.
// @ts-expect-error badly typedDataSourceAdapter._eventsStrategy
dataSource._eventsStrategy.fireEvent('loadError', arguments);
packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping_collapsed.ts:387
- The
@ts-expect-errormessage has a typo (“typedDataSourceAdapter”); add the missing space for readability.
// @ts-expect-error badly typedDataSourceAdapter.pageSize
const pageSize = that._dataSource.pageSize();
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.