DataGrid - Summary - Refactor getSummaryOptions() by Tucchhaa · Pull Request #34864 · DevExpress/DevExtreme

Tucchhaa · 2026-08-21T10:32:21Z

No description provided.

Tucchhaa · 2026-08-21T10:35:00Z

        assert.deepEqual(headerFilterItems.map(function(item) { return item.value; }), [15, 18, 19, 20, 25], 'items are sorted');
    });

-    QUnit.test('findSummaryItem by custom name', function(assert) {


all cases were added to tests/find_summary_item.tests.ts

Tucchhaa · 2026-08-21T10:35:37Z

    }
-  }

-  private summary(summary?) {


It was used as setter only in QUnit

Tucchhaa · 2026-08-21T10:38:34Z

-  protected _createDataSourceAdapter(dataSource: DataSource) {
-    const dataSourceAdapter = super._createDataSourceAdapter(dataSource);
-
-    // @ts-expect-error summaryGetter is defined in summary DataSourceAdapterExtender
-    dataSourceAdapter.summaryGetter((currentRemoteOperations) => {
-      const result = this._getSummaryOptions(
-        currentRemoteOperations ?? dataSourceAdapter.remoteOperations(),
-      );
-      return result;
-    });
-
-    return dataSourceAdapter;
-  }


replaced this function by a simple getter in summaryDataSourceAdapterExtender:
https://github.com/DevExpress/DevExtreme/pull/34864/changes#diff-d57264845ac30f8d19ae92cce36b829978453305efb72ed33d907e6208d7f112R303

Copilot

Pull request overview

Refactors DataGrid summary option construction by extracting the logic into reusable utilities and moving summary-option generation into the Summary DataSourceAdapter extender, while shifting coverage from a removed QUnit internal-method test to new Jest unit tests for the extracted helpers.

Changes:

Extracted summary option building (getSummaryOptions), group-aggregate access, and summary-item lookup into dedicated utils under summary/utils/.
Simplified summary_data_controller by removing duplicated summary-option/aggregate-building logic and using the new findSummaryItem helper for total summary lookups.
Updated tests: adjusted DataSource QUnit tests to use a helper setter and added Jest coverage for the new utilities.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js	Adds a test helper (`setSummary`) and updates call sites to match the refactor.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataController.tests.js	Removes QUnit coverage for an internal summary lookup method that was refactored out.
packages/devextreme/js/__internal/grids/data_grid/summary/utils/get_summary_options.ts	New core utility that builds local/remote aggregate descriptors and sort-by-group summary info.
packages/devextreme/js/__internal/grids/data_grid/summary/utils/get_group_aggregates.ts	New helper to read group aggregates from `summary`/`aggregates` fields.
packages/devextreme/js/__internal/grids/data_grid/summary/utils/find_summary_item.ts	New helper to resolve a summary item index by several supported identifiers.
packages/devextreme/js/__internal/grids/data_grid/summary/utils/tests/get_summary_options.test.ts	Adds Jest coverage for `getSummaryOptions` behavior.
packages/devextreme/js/__internal/grids/data_grid/summary/utils/tests/find_summary_item.test.ts	Adds Jest coverage for `findSummaryItem`.
packages/devextreme/js/__internal/grids/data_grid/summary/types.ts	Updates summary-related internal types and introduces aggregate/sort types.
packages/devextreme/js/__internal/grids/data_grid/summary/summary_module.ts	Adds explicit typing for `defaultOptions()` return shape.
packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts	Removes `summaryGetter/summary` plumbing and uses `getSummaryOptions` directly in the adapter extender.
packages/devextreme/js/__internal/grids/data_grid/summary/extenders/summary_data_controller.ts	Removes summary-option construction logic from the controller extender and switches to shared helpers.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts:314

getSummaryOptions is wired with getUpdatedItemData: (data) => this._editingController.getUpdatedData(data), but _editingController is obtained via getController('editing') and can be undefined in modular setups/tests where the editing module is not included. If summary.recalculateWhileEditing is enabled in that scenario, selectors will throw at runtime. Consider guarding this with an identity fallback (e.g. this._editingController?.getUpdatedData ? ... : data).

      sortByGroupSummaryInfo: this.option('sortByGroupSummaryInfo'),
      remoteOperations: remoteOperations ?? this.remoteOperations(),
      getUpdatedItemData: (data) => this._editingController.getUpdatedData(data),
      columnOption: (id) => this._columnsController.columnOption(id),
      groupColumns: this._columnsController.getGroupColumns(),

anna-shakhova · 2026-08-21T11:17:55Z

+  public init(dataSource?: DataSource): void {
+    super.init(dataSource);

+    this._columnsController = this.getController('columns');


it is confusing that dataAdapter knows smth about grid controllers
I'm not sure we can resolve it right now, please, investigate on possible solutions

dataSourceAdapter is actually a controller:

DevExtreme/packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts

Line 31 in 258f0e8

export default class DataSourceAdapter extends modules.Controller {

So to me it is ok that it uses columnsController.

I think the real issue is:

that dataSourceAdapter acts as both a controller and a dataSource

and that dataSourceAdapter and dataController have overlapping responsibilities

So I think ideally we need to define what is dataSourceAdapter exactly and then refactor it. Right now, it would be hard to remove columnController/editingController deps from summaryDataSourceAdapterExtender

Alyar666 · 2026-08-21T11:25:26Z

+import { findSummaryItem } from './find_summary_item';
+import { getGroupAggregates } from './get_group_aggregates';
+
+export interface GetSummaryOptionsArgs {


Can we move the interfaces and types to types.ts?

I would prefer to keep both the GetSummaryOptionsArgs and types in this file, because types are very simple and used only here and GetSummaryOptionsArgs interface is needed only when getSummaryOptions is called.

So to avoid polluting types.ts I would keep types in this file, if you dont mind

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts:314

getSummary() passes getUpdatedItemData: (data) => this.editingController.getUpdatedData(data) unconditionally. getController('editing') can be undefined in some configurations (other modules guard it), so enabling summary.recalculateWhileEditing could throw at runtime when editingController is missing. Consider guarding here (e.g., this.editingController?.getUpdatedData(data) ?? data) or falling back to an identity updater when the controller is not available.

    const result = getSummaryOptions({
      summary,
      sortByGroupSummaryInfo: this.option('sortByGroupSummaryInfo'),
      remoteOperations: remoteOperations ?? this.remoteOperations(),
      getUpdatedItemData: (data) => this.editingController.getUpdatedData(data),
      columnOption: (id) => this.columnsController.columnOption(id),
      groupColumns: this.columnsController.getGroupColumns(),

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts:331

isLastLevelGroupItemsPagingLocal calls summary?.sortByGroups() which only guards summary, not the sortByGroups function. If getSummary() ever returns an object without sortByGroups (e.g., tests overriding getSummary / adapter stubs), this will throw a TypeError. Safer is summary?.sortByGroups?.() (or typeof summary?.sortByGroups === 'function' && summary.sortByGroups()).

  private isLastLevelGroupItemsPagingLocal() {
    const summary = this.getSummary();
    const sortByGroupsInfo = summary?.sortByGroups();

    return sortByGroupsInfo?.length;

packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts:314

getSummaryOptions is wired with getUpdatedItemData: (data) => this.editingController.getUpdatedData(data) but editingController can be absent (this file already guards it in calculateAggregates). If summary.recalculateWhileEditing is enabled in a build where the editing controller isn't available, selectors will throw at runtime. Consider guarding here as well (e.g., this.editingController?.getUpdatedData?.(data) ?? data) or make editingController optional in EditingControllerRequired and handle the undefined case explicitly.

This issue also appears on line 327 of the same file.

    const result = getSummaryOptions({
      summary,
      sortByGroupSummaryInfo: this.option('sortByGroupSummaryInfo'),
      remoteOperations: remoteOperations ?? this.remoteOperations(),
      getUpdatedItemData: (data) => this.editingController.getUpdatedData(data),
      columnOption: (id) => this.columnsController.columnOption(id),
      groupColumns: this.columnsController.getGroupColumns(),

packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:63

setSummary overrides dataSourceAdapter.getSummary to return the provided object, but the adapter code expects a SummaryOptions-like shape with a callable sortByGroups. Several tests pass only { groupAggregates, totalAggregates }, which can crash when isLastLevelGroupItemsPagingLocal() calls summary.sortByGroups(). Consider defaulting sortByGroups here (e.g., wrap the provided object and add sortByGroups: () => [] when missing).

    dataSourceAdapter.setSummary = (summary) => {
        dataSourceAdapter.getSummary = () => summary;
    };

Eldar Iusupzhanov added 6 commits August 21, 2026 18:28

move getSummaryOptions to utils

f9399e7

move getSummaryOptions to adapterExtender

e5231b9

add test for find_summary_item

156f7aa

fix datasource tests

1906059

add jest tests

ba0ad31

add type for default options

e23a95d

Tucchhaa self-assigned this Aug 21, 2026

Tucchhaa added the cherry-pick label Aug 21, 2026

Copilot AI lite review requested due to automatic review settings August 21, 2026 10:32

Tucchhaa requested a review from a team as a code owner August 21, 2026 10:32

Tucchhaa added the 26_2 label Aug 21, 2026

Copilot started reviewing on behalf of Tucchhaa August 21, 2026 10:32 View session

Tucchhaa commented Aug 21, 2026

View reviewed changes

Copilot AI reviewed Aug 21, 2026

View reviewed changes

Comment thread packages/devextreme/js/__internal/grids/data_grid/summary/types.ts

Comment thread packages/devextreme/js/__internal/grids/data_grid/summary/utils/get_summary_options.ts

apply copilot's review

258f0e8

Copilot AI review requested due to automatic review settings August 21, 2026 10:48

Copilot started reviewing on behalf of Tucchhaa August 21, 2026 10:48 View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

anna-shakhova reviewed Aug 21, 2026

View reviewed changes

Alyar666 requested changes Aug 21, 2026

View reviewed changes

apply review

1a422e6

Copilot AI review requested due to automatic review settings August 21, 2026 13:48

Copilot started reviewing on behalf of Tucchhaa August 21, 2026 13:49 View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

fix ts error

5705536

Copilot AI review requested due to automatic review settings August 21, 2026 13:56

Copilot started reviewing on behalf of Tucchhaa August 21, 2026 13:57 View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

Alyar666 approved these changes Aug 21, 2026

View reviewed changes

Uh oh!

Conversation

Tucchhaa commented Aug 21, 2026

Uh oh!

Tucchhaa Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Tucchhaa Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Tucchhaa Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

anna-shakhova Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Tucchhaa Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alyar666 Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Tucchhaa Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tucchhaa Aug 21, 2026 •

edited

Loading

Tucchhaa Aug 21, 2026 •

edited

Loading

Tucchhaa Aug 21, 2026 •

edited

Loading