Skip to content

Grids: type columnsController file-local members and public API - #35433

Merged
bit-byte0 merged 9 commits into
DevExpress:mainfrom
bit-byte0:refactor/columnscontroller-contract-modifiers-main
Oct 1, 2026
Merged

bit-byte0 merged 9 commits into
DevExpress:mainfrom
bit-byte0:refactor/columnscontroller-contract-modifiers-main

Conversation

@bit-byte0

Copy link
Copy Markdown
Contributor

What

Adds TypeScript types to the DataGrid and TreeList columns controller: its file-local members, state fields and public API now carry real types

How

Typed the private, protected and public members of m_columns_controller.ts (return types, parameters and state fields), moved the shared types into types.ts, and updated the consumer files that the tightened public signatures cascade into

@bit-byte0 bit-byte0 added the 26_2 label Oct 1, 2026
@bit-byte0 bit-byte0 self-assigned this Oct 1, 2026
@bit-byte0
bit-byte0 marked this pull request as ready for review October 1, 2026 06:35
@bit-byte0
bit-byte0 requested a review from a team as a code owner October 1, 2026 06:35
Copilot AI balanced review requested due to automatic review settings October 1, 2026 06:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The TreeList override exposes an unsound item type and can pass undefined node data into column callbacks.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds stronger TypeScript coverage to the shared grid columns controller and affected DataGrid/TreeList consumers.

Changes:

  • Types controller state, methods, column metadata, sorting, and grouping.
  • Updates dependent grid modules for tightened signatures.
  • Adjusts focus integration tests for nullable sort parameters.
File Description
tree_list/​m_columns_controller.ts Types TreeList item extraction.
validating/​m_validating.ts Uses typed array membership checks.
focus/​m_focus.ts Handles nullable sort parameters.
focus/​__tests__/​focus.integration.test.ts Updates nullable-result assertions.
filter/​filter_controller.ts Removes obsolete cast.
filter_sync/​m_filter_sync.ts Uses inferred path lookup type.
editing/​m_editing_cell_based.ts Separates column index lookup.
data_controller/​data_controller.ts Documents nullable sort/group interop.
columns_controller/​types.ts Adds shared internal column types.
columns_controller/​m_columns_controller.ts Types controller state and API.
columns_controller/​m_columns_controller_utils.ts Aligns utilities with new types.
ai_column/​controllers/​m_ai_column_controller.ts Documents initialized column-name invariant.
ai_assistant/​commands/​sorting.ts Adapts sorting command index type.
adaptivity/​m_adaptivity.ts Normalizes width parsing types.
summary/​extenders/​summary_data_controller.ts Adapts specialized column type.
keyboard_navigation/​m_group_panel_keyboard_navigation.ts Allows undefined sorting state.
grouping/​extenders/​grouping_columns_controller.ts Removes obsolete suppression.
data_grid/​focus/​m_focus.ts Documents function-selector support.
ai_assistant/​commands/​summary.ts Narrows required column shape.
ai_assistant/​commands/​grouping.ts Adapts grouping column lookup.

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

anna-shakhova
anna-shakhova previously approved these changes Oct 1, 2026
execute: (component, { success, failure }) => (args): Promise<CommandResult> => {
const columnsController = component.getController('columns');
const column: Column | undefined = columnsController.columnOption(args.dataField);
const column = columnsController.columnOption(args.dataField) as Column | undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets remove assertion, better add ts-expect-error to next line with proper description, so we see the type issue and could fix it sometime

Suggested change
const column = columnsController.columnOption(args.dataField) as Column | undefined;
const column = columnsController.columnOption(args.dataField);

for (const groupItem of groupItems) {
const columnName = groupItem.showInColumn ?? groupItem.column;
const column = this._columnsController.columnOption(columnName);
const column = this._columnsController.columnOption(columnName) as Column | undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets revert this change, better add ts-expect-error to next line with proper description, so we see the type issue and could fix it sometime

try {
// Handles remote operations via data controller listening for the `sorting` change
columnsController.changeSortOrder(column.index, args.sortOrder);
columnsController.changeSortOrder(column.index as number, args.sortOrder);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets just check index before try

Suggested change
columnsController.changeSortOrder(column.index as number, args.sortOrder);
if (!column || !columnsController.allowColumnSorting(column) || column.index === undefined) {
...
columnsController.changeSortOrder(column.index, args.sortOrder);

const userStateColumnOptions = (that._columnsUserState
&& isUserStateColumn(columnOptions as ColumnUserState, that._columnsUserState[currentIndex])
&& that._columnsUserState[currentIndex];
&& that._columnsUserState[currentIndex]) || undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this difficult to read, I suggest to move it to func args:

Suggested change
&& that._columnsUserState[currentIndex]) || undefined;
const column = createColumn(that, columnOptions, userStateColumnOptions || undefined, bandColumn);

const commonColumnSettings = this.getCommonColumnSettings(column);
const groupingOptions: any = this.option('grouping') ?? {};
const groupPanelOptions: any = this.option('groupPanel') ?? {};
const groupingOptions: Record<string, unknown> = this.option('grouping') ?? {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const groupingOptions: Record<string, unknown> = this.option('grouping') ?? {};
import type { Grouping, GroupPanel } from '@js/ui/data_grid';
...
const groupingOptions: Grouping = this.option('grouping') ?? {};
const groupPanelOptions: GroupPanel = this.option('groupPanel') ?? {};

let isColumnFixing = this.option('columnFixing.enabled');

!isColumnFixing && each(this._columns, (_, column): any => {
!isColumnFixing && each(this._columns, (_, column): boolean | undefined => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check, please, I think it can be replaced with this._columns.some()
when callback inside each returns false, the cycle breaks, also I think better return true here on line 691, cause it is not quite clear why callback always return negative value


expandColumns = map(expandColumns, (column) => extend(
// eslint-disable-next-line @typescript-eslint/no-unsafe-return -- extend has an untyped result
expandColumns = map(expandColumns, (column): Column => extend(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expandColumns = map(expandColumns, (column): Column => extend(
expandColumns = map(expandColumns, (column: Column): Column => extend(


public hasVisibleDataColumns(): boolean {
const columns: any[] = this._columns;
const columns: Column[] = this._columns;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const columns: Column[] = this._columns;
const columns = this._columns;

public refresh(updateNewLookupsOnly?) {
const deferreds: any = [];
public refresh(updateNewLookupsOnly?: boolean): DeferredObj<unknown> {
const deferreds: unknown[] = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const deferreds: unknown[] = [];
const deferreds: (DeferredObj<unknown> | undefined)[] = [];

Comment on lines +1466 to +1467
.filter((column) => isString(column.calculateGroupValue))
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
.map((column): string => column.calculateGroupValue);
.map((column): string => column.calculateGroupValue as string);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let convert filter callback to type guard

.filter((column): column is Column & { calculateGroupValue: string } => isString(column.calculateGroupValue))
.map((column) => column.calculateGroupValue);

Copilot AI balanced review requested due to automatic review settings October 1, 2026 10:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Several new signatures inaccurately narrow supported lookup results and existing column state.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity

Open (4)

Comment thread packages/devextreme/js/__internal/grids/grid_core/columns_controller/types.ts Outdated
// @ts-expect-error
// @ts-expect-error Deferred does not describe construction with new
// eslint-disable-next-line @typescript-eslint/no-unsafe-return -- Deferred is not fully typed
return new Deferred().reject().promise();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new Deferred().reject().promise();
return Deferred().reject().promise();

anna-shakhova
anna-shakhova previously approved these changes Oct 1, 2026
Copilot AI balanced review requested due to automatic review settings October 1, 2026 10:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The new filter-expression return contract excludes string values that can still escape at runtime.

Review effort: Balanced
Findings: None

Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Widen filter expression return types to include string results

packages/​devextreme/​js/​__internal/​grids/​grid_core/​columns_controller/​m_columns_controller.ts:1951

The explicit return type excludes a valid runtime result. Column.calculateFilterExpression is declared to return string | Array<any> | Function, and existing search tests use an empty-string result (grid_core/search/__tests__/utils.test.ts:120); this wrapper returns that string unchanged, despite now advertising only DataFilter. The @ts-expect-error below masks the mismatch for callers. Please widen/normalize both ColumnFilterExpression and createFilterExpression so the signature reflects every value that can escape.

anna-shakhova
anna-shakhova previously approved these changes Oct 1, 2026
Copilot AI balanced review requested due to automatic review settings October 1, 2026 11:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad core-controller API typing changes and numerous downstream suppressions warrant final human validation.

Review effort: Balanced
Findings: None

@bit-byte0
bit-byte0 added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 1, 2026
Copilot AI balanced review requested due to automatic review settings October 1, 2026 14:05
anna-shakhova
anna-shakhova previously approved these changes Oct 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Several tightened signatures remain inaccurate for valid TreeList data, sorting descriptors, focus extensions, and persisted state.

Review effort: Balanced
Findings: 4 Medium severity

Open (4)

Copilot AI balanced review requested due to automatic review settings October 1, 2026 14:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The tightened contracts are consistently propagated and targeted regression tests cover the behavior-affecting adjustments.

Review effort: Balanced
Findings: None

Resolved since last review (4)

@bit-byte0
bit-byte0 added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@bit-byte0
bit-byte0 added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 1, 2026
@bit-byte0
bit-byte0 added this pull request to the merge queue Oct 1, 2026
Merged via the queue into DevExpress:main with commit fc2d77e Oct 1, 2026
149 checks passed
@bit-byte0
bit-byte0 deleted the refactor/columnscontroller-contract-modifiers-main branch October 1, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants