Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Changes span several stateful grid algorithms, so behavior parity warrants final human review.
Review effort: Balanced
Findings: None
What changed in this PR
Cleans up lint issues and strengthens typing in the shared ColumnsController while aiming to preserve DataGrid and TreeList behavior.
Changes:
- Simplifies variables, callbacks, and control flow with narrowly scoped lint exceptions.
- Adds regression tests for column generation, visibility, selectors, and filter callback behavior.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller.ts | Refactors controller logic and improves typing. |
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/__tests__/columns_controller.integration.test.ts | Adds behavior-preservation regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!columns[i].dataType || (checkSerializers && columns[i].deserializeValue && columns[i].serializationFormat === undefined)) { | ||
| return false; | ||
| for (const column of columns) { | ||
| if (column.dataField || column.calculateCellValue !== column.defaultCalculateCellValue) { |
There was a problem hiding this comment.
nitpick: we can combine both conditions into one, just for readability create variable that will describe what this conditions are about
also for other conditions that are not fit in a single line it would be more readable to introduce variables with proper names
| columns = columns || that._columns; | ||
|
|
||
| each(columns, (_, column) => { | ||
| each(sourceColumns, (_, column) => { |
There was a problem hiding this comment.
lets replace it with forEach to preserve types
also, please, check other places where each is used, we can also check extend, but it requires care
| const that = this; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- module option (never-typed) | ||
| const customizeColumns: any = that.option('customizeColumns'); | ||
| const customizeColumns: any = this.option('customizeColumns'); |
There was a problem hiding this comment.
| const customizeColumns: any = this.option('customizeColumns'); | |
| const { customizeColumns } = this.option() as ColumnsControllerOptions; |
What
Clean up remaining lint issues in
ColumnsControllerwithout changing DataGrid or TreeList behaviorHow
Simplify local variables, callbacks, and two nested methods while preserving existing falsy-value handling. Keep necessary type and nesting exceptions narrowly scoped