codeant-ai-for-open-source[bot] commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3601185252
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps)
{
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
Review Comment:
**Suggestion:** This new block references variables that are not defined in
this row-header scope (`attrName`, `colKey`, `attrIdx`), which will throw when
rendering. Use the row-scope variables (`settingsRowAttrs[i]`, `r`, and the row
index) instead of column-header-only variables. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Frontend build fails, blocking pivot table plugin usage.
⚠️ CI pipelines fail on TypeScript compilation errors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx`
and locate the `renderColHeaderRow` callback around lines 860-1135 (from the
`Read` tool),
where `attrName`, `attrIdx`, `colKey`, `colLabelClass`, and related
variables are defined
and used to build column header cells.
2. In the same file, inspect the `renderTableRow` callback starting at line
116 in the
`Read` output (diff hunk around 1316) that renders row header cells; within
the
`rowKey.map((r, i) => { ... })` callback, the newly added block at diff
lines 1314-1317
defines `const headerCellFormattedValue =
dateFormatters?.[attrName]?.(convertToNumberIfNumeric(colKey[attrIdx])) ??
colKey[attrIdx];`.
3. Verify from the surrounding `renderTableRow` scope (lines 120-145 from
`Read`) that
`settingsRowAttrs`, `rowKey`, and `dateFormatters` are defined, but there is
no definition
of `attrName`, `colKey`, or `attrIdx` in this function; those identifiers
only exist as
parameters to `renderColHeaderRow` and are not in scope inside
`renderTableRow`.
4. Run the TypeScript type-check or frontend build (for example `cd
superset-frontend &&
npm run build`), and the compiler will flag `attrName`, `colKey`, and
`attrIdx` in
`TableRenderers.tsx` (diff lines 1314-1317) as undefined identifiers,
causing compilation
to fail and blocking the pivot table plugin from being built or used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=26c6c5be958e462ba49c881c593fb2a4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=26c6c5be958e462ba49c881c593fb2a4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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.tsx
**Line:** 1314:1317
**Comment:**
*Incorrect Variable Usage: This new block references variables that are
not defined in this row-header scope (`attrName`, `colKey`, `attrIdx`), which
will throw when rendering. Use the row-scope variables (`settingsRowAttrs[i]`,
`r`, and the row index) instead of column-header-only variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=07bbfc667ba25d0d450c865fbf46730d74f764d22ecb66af5dca7aa9656fc1b2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=07bbfc667ba25d0d450c865fbf46730d74f764d22ecb66af5dca7aa9656fc1b2&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1741,6 +1741,36 @@ test('should assign distinct dash patterns for multiple
time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
Review Comment:
**Suggestion:** The exact same test case name and behavior is added twice,
creating duplicate coverage and ambiguous failure reporting in CI. Keep only
one copy of this test and remove the duplicate block. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Duplicate jest test names complicate CI failure triage.
⚠️ Redundant tests increase suite runtime without added coverage.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts`
and locate the first test named `should adjust dataZoom bottom when chart
height changes`
added in the hunk starting at diff line 1744, which constructs small and
large charts and
compares their `dataZoom[0].bottom` values.
2. In the same file, scroll down to the later hunk starting around diff line
1871 and
observe a second test with the identical name `should adjust dataZoom bottom
when chart
height changes` that also creates `smallChart` and `largeChart`, calls
`transformProps`,
and asserts that the `dataZoom[0].bottom` values differ.
3. Run the frontend test suite (for example `cd superset-frontend && npm
test --
plugin-chart-echarts/test/Timeseries/transformProps.test.ts`) and note that
Jest reports
two separate tests with the exact same description string, both invoking
`transformProps`
and asserting the bottom offset behavior.
4. When a regression occurs in this behavior, CI output will show a failure
for `should
adjust dataZoom bottom when chart height changes` without distinguishing
which of the two
blocks failed, making diagnosis harder and executing redundant assertions
for the same
scenario.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=541283171f49486daeec0fdf713569f5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=541283171f49486daeec0fdf713569f5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1744:1744
**Comment:**
*Code Quality: The exact same test case name and behavior is added
twice, creating duplicate coverage and ambiguous failure reporting in CI. Keep
only one copy of this test and remove the duplicate block.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=2825673bf71c31f58a684cf39081d8b528076ec44cca7f9d0b1a4373d5a17073&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=2825673bf71c31f58a684cf39081d8b528076ec44cca7f9d0b1a4373d5a17073&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps)
{
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
+ const { backgroundColor, color } = getCellColor(
+ [attrName],
+ headerCellFormattedValue,
+ cellColorFormatters,
+ isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor,
+ );
+ const style = {
+ backgroundColor,
+ ...(color ? { color } : {}),
+ };
+ attrValueCells.push(
+ <th
Review Comment:
**Suggestion:** Pushing into `attrValueCells` from inside the callback that
is used to initialize `attrValueCells` creates a self-reference during
initialization and breaks the row rendering flow. Keep this path as a pure
`map` return (as before) and avoid mutating the array being constructed inside
its own initializer. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Pivot table rows crash with ReferenceError during render.
⚠️ Users see blank pivot tables instead of data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `TableRenderers.tsx`, within the `renderTableRow` callback (lines
148-199 from the
`Read` output), note that `const attrValueCells = rowKey.map((r: string, i:
number) => {
... });` initializes `attrValueCells` by mapping over `rowKey`, so
`attrValueCells` is
defined by the result of this `map` call.
2. Inside the same `rowKey.map` callback, inspect the newly added block at
diff lines
1328-1329 where `attrValueCells.push(<th ... > ...)` is called, meaning the
callback
attempts to push additional header cells into `attrValueCells` while
`attrValueCells`
itself is still being initialized.
3. From JavaScript semantics, referencing `attrValueCells` inside its own
initializer
(`const attrValueCells = rowKey.map(() => { attrValueCells.push(...); })`)
places that
reference in the temporal dead zone; at runtime, when `rowKey.map` invokes
the callback
for the first element, `attrValueCells` has not yet been assigned, so any
read or `push`
on `attrValueCells` triggers a `ReferenceError: Cannot access
'attrValueCells' before
initialization`.
4. Once the TypeScript compile-time issues in this block are resolved and a
pivot table
chart (rendered via `PivotTable.tsx` which delegates to `TableRenderer`)
reaches this
path, rendering rows will immediately crash with the temporal-dead-zone
`ReferenceError`,
preventing any pivot table rows from displaying.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4381993ad6cd48fd99bc7c17dc7ad8ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4381993ad6cd48fd99bc7c17dc7ad8ed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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.tsx
**Line:** 1328:1329
**Comment:**
*Logic Error: Pushing into `attrValueCells` from inside the callback
that is used to initialize `attrValueCells` creates a self-reference during
initialization and breaks the row rendering flow. Keep this path as a pure
`map` return (as before) and avoid mutating the array being constructed inside
its own initializer.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=d7e337fbf08add829bf1fee109dc1636e555d0d597cacbefeb2222552ca9b9f1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=d7e337fbf08add829bf1fee109dc1636e555d0d597cacbefeb2222552ca9b9f1&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps)
{
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
+ const { backgroundColor, color } = getCellColor(
+ [attrName],
+ headerCellFormattedValue,
+ cellColorFormatters,
+ isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor,
+ );
+ const style = {
+ backgroundColor,
+ ...(color ? { color } : {}),
+ };
+ attrValueCells.push(
+ <th
+ className={colLabelClass}
+ key={`colKey-${flatColKey}`}
+ style={style}
+ colSpan={colSpan}
+ rowSpan={rowSpan}
+ role="columnheader button"
+ onClick={this.clickHeaderHandler(
+ pivotData,
+ colKey,
+ this.props.cols,
+ attrIdx,
+ this.props.tableOptions.clickColumnHeaderCallback,
+ )}
Review Comment:
**Suggestion:** This component is a function component, so using
`this`-based handlers/state will fail at runtime because `this` is `undefined`.
Replace these calls with the already scoped hook/local variables
(`clickHeaderHandler`, `cols`, `tableOptions`, `collapsedCols`,
`activeSortColumn`, `sortingOrder`) used elsewhere in this file. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Pivot table charts fail rendering due to runtime TypeError.
⚠️ Cross-filtering from row headers becomes completely unusable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx`,
confirm that `TableRenderer` is a function component using React hooks,
defined at lines
334-453 (from the `export function TableRenderer(props: TableRendererProps)`
declaration
and subsequent `useState` and `useCallback` hooks returned by the `Read`
tool).
2. Within the same file, inspect the `renderTableRow` callback (useCallback
starting
around diff line 1316) where row header cells are rendered; the new hunk at
diff lines
1336-1342 sets `onClick={this.clickHeaderHandler(pivotData, colKey,
this.props.cols,
attrIdx, this.props.tableOptions.clickColumnHeaderCallback)}` on a header
`<th>` element
instead of using the `clickHeaderHandler` hook defined earlier.
3. Observe via the code that `clickHeaderHandler` in this file is a
top-level hook-scoped
function (lines 59-88 from the `Read` output) and there is no `this` binding
or class
instance for `TableRenderer`; therefore, evaluating
`this.clickHeaderHandler` inside the
render path will attempt to read a property from `this`, which is
`undefined` for function
components.
4. Render any pivot table chart (for example by using the `PivotTable`
wrapper in
`plugins/plugin-chart-pivot-table/src/react-pivottable/PivotTable.tsx`,
which returns
`<TableRenderer {...props} />` at lines 5-8) and when React executes
`renderTableRow`, the
expression `this.clickHeaderHandler(...)` is evaluated during render,
causing a runtime
TypeError (`Cannot read properties of undefined`) and preventing the pivot
table from
rendering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b4d3990568340a383e116e456c017a2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8b4d3990568340a383e116e456c017a2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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.tsx
**Line:** 1336:1342
**Comment:**
*Api Mismatch: This component is a function component, so using
`this`-based handlers/state will fail at runtime because `this` is `undefined`.
Replace these calls with the already scoped hook/local variables
(`clickHeaderHandler`, `cols`, `tableOptions`, `collapsedCols`,
`activeSortColumn`, `sortingOrder`) used elsewhere in this file.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=f1fbd3fbca398d1f8a5aaf42e3257720022dfe35c9b4d1726b4af4b68908964f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=f1fbd3fbca398d1f8a5aaf42e3257720022dfe35c9b4d1726b4af4b68908964f&reaction=dislike'>👎</a>
--
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]
