codeant-ai-for-open-source[bot] commented on code in PR #36819:
URL: https://github.com/apache/superset/pull/36819#discussion_r2644682565
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -385,7 +385,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
const nums = data
Review Comment:
**Suggestion:** Runtime error when `data` is undefined: `nums` is derived
with optional chaining on `data` but `.filter(...)` is called directly after,
so if `data` is undefined `nums` will be undefined and `nums.length` will
throw. Fix by ensuring `nums` is always an array (e.g., default `data` to `[]`)
before calling `.map`/`.filter`. [null pointer]
**Severity Level:** Minor ⚠️
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -803,6 +804,77 @@ describe('plugin-chart-table', () => {
cells = document.querySelectorAll('td');
});
+ test('render cell bars even when column contains NULL values', () => {
Review Comment:
**Suggestion:** The test function performs DOM assertions that may depend on
async rendering or effect-driven updates but the test is synchronous. Make the
test async (add `async`) so you can `await` `waitFor` when asserting DOM nodes
that can appear after state/effects; this prevents flaky failures where the
assertion runs before the cell bars are rendered. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
test('render cell bars even when column contains NULL values', async
() => {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Making the test async (and awaiting any async UI updates) reduces flakiness
when DOM updates happen via effects or async layout; it's a safe improvement to
test stability.
</details>
<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-table/test/TableChart.test.tsx
**Line:** 807:807
**Comment:**
*Race Condition: The test function performs DOM assertions that may
depend on async rendering or effect-driven updates but the test is synchronous.
Make the test async (add `async`) so you can `await` `waitFor` when asserting
DOM nodes that can appear after state/effects; this prevents flaky failures
where the assertion runs before the cell bars are rendered.
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-table/test/TableChart.test.tsx:
##########
@@ -803,6 +804,77 @@ describe('plugin-chart-table', () => {
cells = document.querySelectorAll('td');
});
+ test('render cell bars even when column contains NULL values', () => {
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['category', 'value1', 'value2', 'value3', 'value4'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ data: [
+ {
+ category: 'Category A',
+ value1: 10,
+ value2: 20,
+ value3: 30,
+ value4: null,
+ },
+ {
+ category: 'Category B',
+ value1: 15,
+ value2: 25,
+ value3: 35,
+ value4: 100,
+ },
+ {
+ category: 'Category C',
+ value1: 18,
+ value2: 28,
+ value3: 38,
+ value4: null,
+ },
+ ],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: true,
+ metrics: ['value1', 'value2', 'value3', 'value4'],
+ },
+ });
+
+ const { container } = render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+
+ // Get all cell bars - should exist for both columns with and without
NULL values
+ const cellBars = container.querySelectorAll('div.cell-bar');
+
+ // Should have cell bars in all numeric columns, even those with NULL
values
+ // value1, value2, value3 all have 3 values, value4 has 1 non-NULL
value
+ // Total: 3 + 3 + 3 + 1 = 10 cell bars
+ expect(cellBars.length).toBeGreaterThan(0);
+
+ // Specifically check that value4 column (which has NULLs) still
renders bars for non-NULL cells
+ const rows = container.querySelectorAll('tbody tr');
+ expect(rows.length).toBe(3);
+
+ // Row 2 should have a cell bar in value4 column (value: 100)
+ const row2Cells = rows[1].querySelectorAll('td');
+ const value4Cell = row2Cells[4]; // 5th column (0-indexed)
+ const value4Bar = value4Cell.querySelector('div.cell-bar');
Review Comment:
**Suggestion:** Directly indexing into row cells with a hard-coded index
(row2Cells[4]) can be out-of-bounds if the table contains different/extra
columns (grouping headers, hidden columns, or additional leading columns). If
`row2Cells[4]` is undefined, calling `.querySelector` on it will throw. Find
the column index dynamically from the table headers (or locate the column by
header text) and assert the cell exists before querying its contents. [null
pointer]
**Severity Level:** Minor ⚠️
```suggestion
// Resolve the column index dynamically from the table headers to
avoid brittle hard-coded indices.
const headerCells = container.querySelectorAll('thead th');
const value4Index = Array.from(headerCells).findIndex(h =>
(h.textContent || '').trim() === 'value4',
);
expect(value4Index).toBeGreaterThanOrEqual(0);
const row2Cells = rows[1].querySelectorAll('td');
const value4Cell = row2Cells[value4Index];
expect(value4Cell).toBeDefined();
const value4Bar = value4Cell!.querySelector('div.cell-bar');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test currently assumes a fixed column ordering which is brittle;
resolving the column index from headers makes the assertion robust and prevents
potential runtime TypeError when table columns change. This is a valid,
low-risk improvement to test resilience.
</details>
<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-table/test/TableChart.test.tsx
**Line:** 872:874
**Comment:**
*Null Pointer: Directly indexing into row cells with a hard-coded index
(row2Cells[4]) can be out-of-bounds if the table contains different/extra
columns (grouping headers, hidden columns, or additional leading columns). If
`row2Cells[4]` is undefined, calling `.querySelector` on it will throw. Find
the column index dynamically from the table headers (or locate the column by
header text) and assert the cell exists before querying its contents.
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-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -67,8 +67,10 @@ function getValueRange(
alignPositiveNegative: boolean,
data: InputData[],
) {
- if (typeof data?.[0]?.[key] === 'number') {
- const nums = data.map(row => row[key]) as number[];
+ const nums = data
+ .map(row => row[key])
+ .filter(value => typeof value === 'number') as number[];
Review Comment:
**Suggestion:** The current code will exclude Number boxed objects (e.g.,
`new Number(5)`) and doesn't explicitly remove NaN/Infinity; normalize boxed
numbers and explicitly filter by `Number.isFinite` to avoid NaN/Infinity
contaminating range calculations. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
.map(row => {
const raw = row[key];
return raw instanceof Number ? raw.valueOf() : raw;
})
.filter((value): value is number =>
typeof value === 'number' && Number.isFinite(value),
) as number[];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code ignores boxed Number objects (new Number(5)) and doesn't
explicitly exclude NaN/Infinity. Normalizing boxed numbers via valueOf() and
guarding with Number.isFinite prevents non-finite values from contaminating the
range calculation — a sensible, small correctness fix.
</details>
<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-ag-grid-table/src/utils/useColDefs.ts
**Line:** 71:72
**Comment:**
*Possible Bug: The current code will exclude Number boxed objects
(e.g., `new Number(5)`) and doesn't explicitly remove NaN/Infinity; normalize
boxed numbers and explicitly filter by `Number.isFinite` to avoid NaN/Infinity
contaminating range calculations.
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-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -67,8 +67,10 @@ function getValueRange(
alignPositiveNegative: boolean,
data: InputData[],
) {
- if (typeof data?.[0]?.[key] === 'number') {
- const nums = data.map(row => row[key]) as number[];
+ const nums = data
+ .map(row => row[key])
+ .filter(value => typeof value === 'number') as number[];
+ if (nums.length > 0) {
return (
alignPositiveNegative ? [0, d3Max(nums.map(Math.abs))] : d3Extent(nums)
) as ValueRange;
Review Comment:
**Suggestion:** d3Max can return undefined in some cases (type definitions
allow it); using `[0, d3Max(...)]` can produce a range with undefined which
will break consumers—compute the max separately and fall back to 0, and ensure
d3Extent fallback is handled when it returns undefined. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const maxAbs = d3Max(nums.map(Math.abs));
if (alignPositiveNegative) {
return [0, maxAbs ?? 0] as ValueRange;
}
const extent = d3Extent(nums) as ValueRange | undefined;
return extent ?? [0, 0];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
d3's typings allow d3Max/d3Extent to return undefined even though with a
non-empty numeric array they should return numbers at runtime. Handling the
potential undefined explicitly (compute max separately and nullish-coalesce /
fallback) removes a fragile cast and makes the function safer and clearer to
readers and TypeScript. This is a real, low-risk defensive improvement.
</details>
<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-ag-grid-table/src/utils/useColDefs.ts
**Line:** 74:76
**Comment:**
*Possible Bug: d3Max can return undefined in some cases (type
definitions allow it); using `[0, d3Max(...)]` can produce a range with
undefined which will break consumers—compute the max separately and fall back
to 0, and ensure d3Extent fallback is handled when it returns undefined.
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-table/src/TableChart.tsx:
##########
@@ -958,6 +958,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
display: block;
top: 0;
${valueRange &&
Review Comment:
**Suggestion:** Inconsistent conditional: the PR added a `typeof value ===
'number'` guard inside the CSS template but the cell-bar div is still rendered
whenever `valueRange` is truthy. That leaves an inert/empty div (and incorrect
`'positive'` class for non-number values). Make the render conditional also
require `typeof value === 'number'` (or have the CSS hide the element) so
non-numeric/null cells don't render an empty bar element. [logic error]
**Severity Level:** Minor ⚠️
--
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]
