PivotGrid - Fix depth calculation of header items when a column is empty #32296
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes PivotGrid header depth calculation in scenarios where hideEmptySummaryCells hides an entire column (e.g., when calculateSummaryValue returns null), preventing incorrect header layout/export results after expand/collapse operations.
Changes:
- Adjusted PivotGrid header depth computation to skip fully empty header items.
- Added PivotGrid QUnit regression tests covering expand/collapse behavior and export column stability when empty summary cells are hidden.
- Added ExcelJS exporter regression test verifying export output when
hideEmptySummaryCellsis enabled and summary values can benull.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts | Updates header depth calculation to ignore empty header items when computing depth. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/pivotGrid.tests.js | Adds UI-level regression tests around expand/collapse sequences and exported column stability with hidden empty cells. |
| packages/devextreme/testing/tests/DevExpress.exporter/exceljsParts/exceljs.pivotGrid.tests.js | Adds exporter regression test to ensure hidden empty columns aren’t exported after expand/collapse. |
| assert.deepEqual(dataSource.expandAll.lastCall.args, [1], 'collapseLevel args'); | ||
| }); | ||
|
|
||
| QUnit.test('Expand all should not mark hidden columns as expanded when hideEmptySummaryCells enabled with calculateSummaryValue returning null', function(assert) { |
There was a problem hiding this comment.
This test name mentions “mark hidden columns as expanded”, but the assertions validate Grand Total column width stability. Please align the test name (or add an assertion that actually checks expanded state / rendered columns) so the intent and coverage are clear.
| QUnit.test('Expand all should not mark hidden columns as expanded when hideEmptySummaryCells enabled with calculateSummaryValue returning null', function(assert) { | |
| QUnit.test('Expand all should not change Grand Total column width when hideEmptySummaryCells enabled with calculateSummaryValue returning null', function(assert) { |
| // Output widths for examination | ||
| assert.ok(true, `Initial Grand Total width: ${initialGrandTotalWidth}`); | ||
| assert.ok(true, `Final Grand Total width: ${finalGrandTotalWidth}`); | ||
|
|
There was a problem hiding this comment.
These assert.ok(true, ...) calls act as debug logging and add unconditional passing assertions, which can hide real failures and add noise to test output. Please remove them (or replace with a meaningful assertion on the expected values).
| // Output widths for examination | |
| assert.ok(true, `Initial Grand Total width: ${initialGrandTotalWidth}`); | |
| assert.ok(true, `Final Grand Total width: ${finalGrandTotalWidth}`); |
| assert.strictEqual( | ||
| finalGrandTotalWidth, | ||
| initialGrandTotalWidth, |
There was a problem hiding this comment.
Comparing DOM widths with assert.strictEqual is likely to be flaky across browsers/font rendering/subpixel rounding. Since this file already uses assert.roughEqual for width-related checks, consider switching this to a tolerant comparison (e.g., roughEqual with a small epsilon) to reduce intermittent failures.
| assert.strictEqual( | |
| finalGrandTotalWidth, | |
| initialGrandTotalWidth, | |
| assert.roughEqual( | |
| finalGrandTotalWidth, | |
| initialGrandTotalWidth, | |
| 1, |
| this.clock.tick(10); | ||
|
|
||
| const initialColumnCount = initialItems[0].length; | ||
| assert.ok(true, `Initial export column count: ${initialColumnCount}`); |
There was a problem hiding this comment.
assert.ok(true, ...) here is unconditional and acts like debug logging rather than validating behavior. Please remove it or replace it with an assertion that checks an expected property (e.g., an explicit expected column count).
| assert.ok(true, `Initial export column count: ${initialColumnCount}`); | |
| assert.ok(initialColumnCount > 0, `Initial export column count should be greater than 0, actual: ${initialColumnCount}`); |
| const finalColumnCount = finalItems[0].length; | ||
| assert.ok(true, `Final export column count: ${finalColumnCount}`); | ||
|
|
There was a problem hiding this comment.
assert.ok(true, ...) here is unconditional and acts like debug logging rather than validating behavior. Please remove it or replace it with an assertion that checks an expected property (e.g., an explicit expected column count).
| let { isEmpty } = item; | ||
|
|
||
| if (isEmpty?.length) { | ||
| isEmpty = item.isEmpty.filter((v) => v).length === isEmpty.length; | ||
| } | ||
|
|
||
| if (item && isEmpty) { |
There was a problem hiding this comment.
isEmpty can be an array (set in summary display mode). With the current logic, an empty array ([]) will skip the isEmpty?.length branch, remain truthy, and then be treated as “empty” later, causing this callback to return and potentially undercount header depth. Consider normalizing to an explicit boolean, e.g. const isFullyEmpty = Array.isArray(item.isEmpty) ? item.isEmpty.length > 0 && item.isEmpty.every(Boolean) : !!item.isEmpty; and use that for the emptiness check.
| let { isEmpty } = item; | |
| if (isEmpty?.length) { | |
| isEmpty = item.isEmpty.filter((v) => v).length === isEmpty.length; | |
| } | |
| if (item && isEmpty) { | |
| const isEmptyValue = item?.isEmpty; | |
| const isFullyEmpty = Array.isArray(isEmptyValue) | |
| ? isEmptyValue.length > 0 && isEmptyValue.every(Boolean) | |
| : !!isEmptyValue; | |
| if (item && isFullyEmpty) { |
No description provided.