codeant-ai-for-open-source[bot] commented on code in PR #36849:
URL: https://github.com/apache/superset/pull/36849#discussion_r2649691764
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -23,6 +23,26 @@ import PropTypes from 'prop-types';
import { PivotData, flatKey } from './utilities';
import { Styles } from './Styles';
+/**
+ * Computes the initial collapsed state map for a given set of keys and depth.
+ * Keys at the specified depth will be marked as collapsed, hiding their
children.
+ * @param {Array<Array>} keys - Array of key arrays (e.g., rowKeys or colKeys)
+ * @param {number} depth - The depth at which to collapse (1-based). Keys at
this depth will be collapsed.
+ * @returns {Object} A map of flatKey => true for all keys that should be
collapsed
+ */
+export function computeCollapsedMap(keys, depth) {
+ if (!depth || depth <= 0) {
+ return {};
+ }
+ const collapsed = {};
+ keys
+ .filter(k => k.length === depth)
+ .forEach(k => {
+ collapsed[flatKey(k)] = true;
Review Comment:
**Suggestion:** Logic bug: `computeCollapsedMap` only marks entire keys
whose length exactly equals `depth`, but row/col keys are typically full leaf
paths — to collapse nodes at a given depth you must compute the unique prefix
of length `depth` for each key and mark those prefixes collapsed. The current
implementation will usually return empty and not collapse anything unless some
keys already happen to be exactly `depth` long. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
export function computeCollapsedMap(keys = [], depth) {
const d = Number(depth);
if (!Number.isInteger(d) || d <= 0) {
return {};
}
const collapsed = {};
const seen = new Set();
keys.forEach(k => {
if (!Array.isArray(k) || k.length < d) {
return;
}
const prefix = k.slice(0, d);
const keyStr = flatKey(prefix);
if (!seen.has(keyStr)) {
seen.add(keyStr);
collapsed[keyStr] = true;
}
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
**Line:** 33:41
**Comment:**
*Logic Error: Logic bug: `computeCollapsedMap` only marks entire keys
whose length exactly equals `depth`, but row/col keys are typically full leaf
paths — to collapse nodes at a given depth you must compute the unique prefix
of length `depth` for each key and mark those prefixes collapsed. The current
implementation will usually return empty and not collapse anything unless some
keys already happen to be exactly `depth` long.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -585,6 +587,8 @@ export default function PivotTableChart(props:
PivotTableProps) {
namesMapping={verboseMap}
onContextMenu={handleContextMenu}
allowRenderHtml={allowRenderHtml}
+ defaultRowExpansionDepth={defaultRowExpansionDepth}
+ defaultColExpansionDepth={defaultColExpansionDepth}
Review Comment:
**Suggestion:** The values passed into the `PivotTable` should be
validated/coerced to safe non-negative integers; passing `undefined`, `NaN`,
negative numbers or non-integer values may lead to runtime errors or unexpected
expansion behavior—coerce and clamp to a 0+ integer when passing props. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
defaultRowExpansionDepth={Number.isFinite(defaultRowExpansionDepth) ?
Math.max(0, Math.trunc(defaultRowExpansionDepth)) : 0}
defaultColExpansionDepth={Number.isFinite(defaultColExpansionDepth) ?
Math.max(0, Math.trunc(defaultColExpansionDepth)) : 0}
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx
**Line:** 590:591
**Comment:**
*Type Error: The values passed into the `PivotTable` should be
validated/coerced to safe non-negative integers; passing `undefined`, `NaN`,
negative numbers or non-integer values may lead to runtime errors or unexpected
expansion behavior—coerce and clamp to a 0+ integer when passing props.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts:
##########
@@ -88,6 +88,8 @@ interface PivotTableCustomizeProps {
time_grain_sqla?: TimeGranularity;
granularity_sqla?: string;
allowRenderHtml?: boolean;
+ defaultRowExpansionDepth?: number;
+ defaultColExpansionDepth?: number;
Review Comment:
**Suggestion:** Compatibility bug: codebase and form data in this repository
use both camelCase and snake_case property names (see other fields like
`timeGrainSqla` and `time_grain_sqla`); adding only `defaultRowExpansionDepth`
(camelCase) can cause consumers that populate props using snake_case form data
to never receive this value. Add the snake_case equivalent
`default_row_expansion_depth` to preserve backwards/interop compatibility.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
default_row_expansion_depth?: number;
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts
**Line:** 92:92
**Comment:**
*Possible Bug: Compatibility bug: codebase and form data in this
repository use both camelCase and snake_case property names (see other fields
like `timeGrainSqla` and `time_grain_sqla`); adding only
`defaultRowExpansionDepth` (camelCase) can cause consumers that populate props
using snake_case form data to never receive this value. Add the snake_case
equivalent `default_row_expansion_depth` to preserve backwards/interop
compatibility.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -107,6 +107,8 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
timeGrainSqla,
currencyFormat,
allowRenderHtml,
+ defaultRowExpansionDepth,
+ defaultColExpansionDepth,
Review Comment:
**Suggestion:** Missing sensible defaults: `defaultRowExpansionDepth` and
`defaultColExpansionDepth` are destructured from `formData` without defaults,
so if the keys are absent they will be `undefined`. Downstream code that
expects numbers (e.g., arithmetic/comparison) can misbehave; provide numeric
default values during destructuring. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
defaultRowExpansionDepth = 0,
defaultColExpansionDepth = 0,
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts
**Line:** 110:111
**Comment:**
*Possible Bug: Missing sensible defaults: `defaultRowExpansionDepth`
and `defaultColExpansionDepth` are destructured from `formData` without
defaults, so if the keys are absent they will be `undefined`. Downstream code
that expects numbers (e.g., arithmetic/comparison) can misbehave; provide
numeric default values during destructuring.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -181,5 +183,7 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
onContextMenu,
timeGrainSqla,
allowRenderHtml,
+ defaultRowExpansionDepth,
+ defaultColExpansionDepth,
Review Comment:
**Suggestion:** Type coercion issue: the returned props currently pass
`defaultRowExpansionDepth` and `defaultColExpansionDepth` through as-is; if
those values come as strings (common in form payloads) consumers expecting
numbers will get incorrect types. Coerce to numbers in the returned object and
fallback to 0 when coercion fails. [type error]
**Severity Level:** Minor ⚠️
```suggestion
defaultRowExpansionDepth:
Number.isFinite(Number(defaultRowExpansionDepth)) &&
defaultRowExpansionDepth !== ''
? Number(defaultRowExpansionDepth)
: 0,
defaultColExpansionDepth:
Number.isFinite(Number(defaultColExpansionDepth)) &&
defaultColExpansionDepth !== ''
? Number(defaultColExpansionDepth)
: 0,
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts
**Line:** 186:187
**Comment:**
*Type Error: Type coercion issue: the returned props currently pass
`defaultRowExpansionDepth` and `defaultColExpansionDepth` through as-is; if
those values come as strings (common in form payloads) consumers expecting
numbers will get incorrect types. Coerce to numbers in the returned object and
fallback to 0 when coercion fails.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -183,6 +183,8 @@ export default function PivotTableChart(props:
PivotTableProps) {
onContextMenu,
timeGrainSqla,
allowRenderHtml,
+ defaultRowExpansionDepth,
+ defaultColExpansionDepth,
Review Comment:
**Suggestion:** Missing default values for the new props: if
`defaultRowExpansionDepth` or `defaultColExpansionDepth` are omitted they will
be `undefined` and could be forwarded to the PivotTable component which may
expect numeric values; provide safe defaults during destructuring to avoid
undefined being used later. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
defaultRowExpansionDepth = 0,
defaultColExpansionDepth = 0,
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx
**Line:** 186:187
**Comment:**
*Possible Bug: Missing default values for the new props: if
`defaultRowExpansionDepth` or `defaultColExpansionDepth` are omitted they will
be `undefined` and could be forwarded to the PivotTable component which may
expect numeric values; provide safe defaults during destructuring to avoid
undefined being used later.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -84,6 +104,57 @@ export class TableRenderer extends Component {
this.clickHandler = this.clickHandler.bind(this);
}
+ componentDidMount() {
+ // Apply initial collapse based on defaultRowExpansionDepth and
defaultColExpansionDepth
+ const { defaultRowExpansionDepth, defaultColExpansionDepth } = this.props;
+
+ // Only apply if at least one depth is configured
+ if (!defaultRowExpansionDepth && !defaultColExpansionDepth) {
Review Comment:
**Suggestion:** Robustness bug: the initial check uses truthiness (`if
(!defaultRowExpansionDepth && !defaultColExpansionDepth)`) which treats
non-positive or non-integer values incorrectly; explicitly validate depths as
positive integers before proceeding so invalid or zero values are treated as
"disabled" and other invalid inputs don't accidentally proceed. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
// Only apply if at least one positive integer depth is configured
const rowDepth = Number(defaultRowExpansionDepth);
const colDepth = Number(defaultColExpansionDepth);
if ((Number.isNaN(rowDepth) || rowDepth <= 0) && (Number.isNaN(colDepth)
|| colDepth <= 0)) {
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
**Line:** 111:112
**Comment:**
*Possible Bug: Robustness bug: the initial check uses truthiness (`if
(!defaultRowExpansionDepth && !defaultColExpansionDepth)`) which treats
non-positive or non-integer values incorrectly; explicitly validate depths as
positive integers before proceeding so invalid or zero values are treated as
"disabled" and other invalid inputs don't accidentally proceed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -84,6 +104,57 @@ export class TableRenderer extends Component {
this.clickHandler = this.clickHandler.bind(this);
}
+ componentDidMount() {
+ // Apply initial collapse based on defaultRowExpansionDepth and
defaultColExpansionDepth
+ const { defaultRowExpansionDepth, defaultColExpansionDepth } = this.props;
+
+ // Only apply if at least one depth is configured
+ if (!defaultRowExpansionDepth && !defaultColExpansionDepth) {
+ return;
+ }
+
+ const {
+ rowKeys,
+ colKeys,
+ rowAttrs,
+ colAttrs,
+ rowSubtotalDisplay,
+ colSubtotalDisplay,
+ } = this.getBasePivotSettings();
+
+ const updates = {};
+
+ // Apply row collapse if subtotals are enabled and we have enough depth
+ if (
+ defaultRowExpansionDepth &&
+ rowSubtotalDisplay.enabled &&
+ rowAttrs.length > 1 &&
+ defaultRowExpansionDepth < rowAttrs.length
+ ) {
+ updates.collapsedRows = computeCollapsedMap(
+ rowKeys,
+ defaultRowExpansionDepth,
+ );
+ }
+
+ // Apply column collapse if subtotals are enabled and we have enough depth
+ if (
+ defaultColExpansionDepth &&
+ colSubtotalDisplay.enabled &&
+ colAttrs.length > 1 &&
+ defaultColExpansionDepth < colAttrs.length
+ ) {
+ updates.collapsedCols = computeCollapsedMap(
+ colKeys,
+ defaultColExpansionDepth,
+ );
+ }
+
+ if (Object.keys(updates).length > 0) {
+ this.setState(updates);
Review Comment:
**Suggestion:** Logic bug: the component calls setState whenever any key
exists on the `updates` object, even if the computed
`collapsedRows`/`collapsedCols` maps are empty objects (no collapsed keys).
This will trigger an unnecessary state update and may overwrite existing
collapse state; only apply state updates when the collapsed maps actually
contain entries. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const collapsedRows = updates.collapsedRows || {};
const collapsedCols = updates.collapsedCols || {};
const hasCollapsedRows = Object.keys(collapsedRows).length > 0;
const hasCollapsedCols = Object.keys(collapsedCols).length > 0;
if (hasCollapsedRows || hasCollapsedCols) {
const stateUpdate = {};
if (hasCollapsedRows) stateUpdate.collapsedRows = collapsedRows;
if (hasCollapsedCols) stateUpdate.collapsedCols = collapsedCols;
this.setState(stateUpdate);
```
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
**Line:** 153:154
**Comment:**
*Logic Error: Logic bug: the component calls setState whenever any key
exists on the `updates` object, even if the computed
`collapsedRows`/`collapsedCols` maps are empty objects (no collapsed keys).
This will trigger an unnecessary state update and may overwrite existing
collapse state; only apply state updates when the collapsed maps actually
contain entries.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
