bito-code-review[bot] commented on code in PR #36050:
URL: https://github.com/apache/superset/pull/36050#discussion_r2507780756
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -433,11 +570,35 @@ export class TableRenderer extends Component {
) {
colLabelClass += ' active';
}
+ const { maxRowVisible: maxRowIndex, maxColVisible } = pivotSettings;
+ const visibleSortIcon = maxColVisible - 1 === attrIdx;
const rowSpan = 1 + (attrIdx === colAttrs.length - 1 ? rowIncrSpan :
0);
const flatColKey = flatKey(colKey.slice(0, attrIdx + 1));
const onArrowClick = needToggle ? this.toggleColKey(flatColKey) : null;
+ const getSortIcon = key => {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Sort icon state not updating due to type
mismatch</b></div>
<div id="fix">
The getSortIcon function uses Object.keys(visibleColKeys)[i] as the key,
which returns a string index, but activeSortColumn is a number. This type
mismatch prevents the sort icons from toggling between unsorted, ascending, and
descending states, breaking the sorting UI functionality. Change the parameter
to use the numeric index i directly for proper comparison.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -580,22 +580,22 @@
const getSortIcon = key => {
const { activeSortColumn, sortingOrder } = this.state;
if (activeSortColumn !== key) {
return (
<FaSort
onClick={() =>
this.sortData(key, visibleColKeys, pivotData,
maxRowIndex)
}
/>
);
}
const SortIcon = sortingOrder[key] === 'asc' ? FaSortAsc :
FaSortDesc;
return (
<SortIcon
onClick={() =>
this.sortData(key, visibleColKeys, pivotData, maxRowIndex)
}
/>
);
};
+ const getSortIcon = i => {
+ const { activeSortColumn, sortingOrder } = this.state;
+
+ if (activeSortColumn !== i) {
+ return (
+ <FaSort
+ onClick={() =>
+ this.sortData(i, visibleColKeys, pivotData, maxRowIndex)
+ }
+ />
+ );
+ }
+
+ const SortIcon = sortingOrder[i] === 'asc' ? FaSortAsc :
FaSortDesc;
+ return (
+ <SortIcon
+ onClick={() =>
+ this.sortData(i, visibleColKeys, pivotData, maxRowIndex)
+ }
+ />
+ );
+ };
const headerCellFormattedValue =
dateFormatters &&
dateFormatters[attrName] &&
@@ -641,1 +641,1 @@
{visibleSortIcon &&
getSortIcon(Object.keys(visibleColKeys)[i])}
+ {visibleSortIcon && getSortIcon(i)}
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36050#issuecomment-3507808517>#534bb9</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +418,72 @@ export class TableRenderer extends Component {
return spans;
}
+ calculateGroups(pivotData, visibleColKey, columnIndex) {
+ const groups = {};
+ const rows = pivotData.rowKeys;
+ rows.forEach(rowKey => {
+ let current = groups;
+ let sumGroup = 0;
+ rowKey.forEach(key => {
+ if (!current[key]) {
+ current[key] = { currentVal: 0 };
+ }
+ sumGroup += pivotData
+ .getAggregator(rowKey, visibleColKey[columnIndex])
+ .value();
+ current[key].currentVal = sumGroup;
+ current = current[key];
+ });
+ });
+ return groups;
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong value accumulation in group calculation</b></div>
<div id="fix">
In calculateGroups, the currentVal is set using a cumulative sumGroup that
adds the same aggregator value multiple times for each level in the rowKey
hierarchy. This leads to incorrect values for nested groups and leaves,
breaking hierarchical sorting calculations. Move the aggregator call outside
the loop and use += to accumulate properly.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
calculateGroups(pivotData, visibleColKey, columnIndex) {
const groups = {};
const rows = pivotData.rowKeys;
rows.forEach(rowKey => {
let current = groups;
rowKey.forEach(key => {
if (!current[key]) {
current[key] = { currentVal: 0 };
}
current[key].currentVal += pivotData
.getAggregator(rowKey, visibleColKey[columnIndex])
.value();
current = current[key];
});
});
return groups;
}
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36050#issuecomment-3507808517>#534bb9</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
