codeant-ai-for-open-source[bot] commented on code in PR #36344:
URL: https://github.com/apache/superset/pull/36344#discussion_r2583390210
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx:
##########
@@ -578,6 +593,20 @@ const config: ControlPanelConfig = {
colnames = updatedColnames;
coltypes = updatedColtypes;
}
+
+ const chartColumns =
+ explore?.controls?.chart_columns?.value || [];
+ if (Array.isArray(chartColumns)) {
+ chartColumns.forEach(
+ (col: { key: string; label: string }) => {
+ if (!colnames.includes(col.label)) {
+ colnames.push(col.label);
+ coltypes.push(GenericDataType.Chart);
Review Comment:
**Suggestion:** The code uses `GenericDataType.Chart`, but the
`GenericDataType` enum only defines `Numeric`, `String`, `Temporal`, and
`Boolean`, so this access evaluates to `undefined` at runtime and leaves the
`coltypes` entry for chart columns invalid, which can break consumers like
`ColumnConfigControl` that expect a valid data type for each column. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
coltypes.push(GenericDataType.Numeric);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
`GenericDataType.Chart` flat-out does not exist in the enum exported from
`@apache-superset/core/api/core` (it only has Numeric, String, Temporal,
Boolean), and `rg` over the repo confirms no other usage of
`GenericDataType.Chart`. That means this code currently pushes `undefined`
into `coltypes`, which is a real runtime bug for consumers like
`ColumnConfigControl` that expect a valid `GenericDataType` per column.
Switching to `GenericDataType.Numeric` assigns a valid, existing enum
member, consistent with the fact that these chart columns visualize
numeric metrics. This is a functional bug fix, not a cosmetic tweak.
</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/controlPanel.tsx
**Line:** 604:604
**Comment:**
*Type Error: The code uses `GenericDataType.Chart`, but the
`GenericDataType` enum only defines `Numeric`, `String`, `Temporal`, and
`Boolean`, so this access evaluates to `undefined` at runtime and leaves the
`coltypes` entry for chart columns invalid, which can break consumers like
`ColumnConfigControl` that expect a valid data type for each column.
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/src/explore/components/controls/ChartColumnsControl/ChartColumnPopover.tsx:
##########
@@ -0,0 +1,149 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useState, useEffect, useCallback, useRef } from 'react';
+import { t } from '@superset-ui/core';
+import { styled } from '@apache-superset/core/ui';
+import { Popover, Input, Button, InputRef } from
'@superset-ui/core/components';
+import { ChartColumnPopoverProps, ChartColumnConfig } from './types';
+
+const PopoverContent = styled.div`
+ width: 300px;
+ padding: ${({ theme }) => theme.sizeUnit * 2}px;
+`;
+
+const FormItem = styled.div`
+ margin-bottom: ${({ theme }) => theme.sizeUnit * 3}px;
+`;
+
+const Label = styled.div`
+ margin-bottom: ${({ theme }) => theme.sizeUnit}px;
+ font-weight: ${({ theme }) => theme.fontWeightStrong};
+ font-size: ${({ theme }) => theme.fontSizeSM}px;
+`;
+
+const ButtonContainer = styled.div`
+ display: flex;
+ justify-content: space-between;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
+ margin-top: ${({ theme }) => theme.sizeUnit * 6}px;
+`;
+
+const StyledButton = styled(Button)`
+ flex: 1;
+`;
+
+export const ChartColumnPopover = ({
+ onChange,
+ config,
+ title,
+ children,
+ ...popoverProps
+}: ChartColumnPopoverProps) => {
+ const [isOpen, setIsOpen] = useState(false);
+ const [label, setLabel] = useState(config?.label || '');
+ const inputRef = useRef<InputRef>(null);
+
+ const handleSave = () => {
+ const newConfig: ChartColumnConfig = {
+ key: config?.key || `chart_${Date.now()}`,
+ label: label || t('Chart Column'),
+ };
+ onChange(newConfig);
+ setIsOpen(false);
+ // reset for next add
+ if (!config) {
+ setLabel('');
+ }
+ };
+
+ const handleCancel = useCallback(() => {
+ setIsOpen(false);
+ // reset to original values if editing
+ if (config) {
+ setLabel(config.label);
+ } else {
+ setLabel('');
+ }
+ }, [config]);
+
+ // focus input when popover opens
+ useEffect(() => {
+ if (isOpen && inputRef.current) {
+ inputRef.current.focus({ cursor: 'end' });
+ }
+ }, [isOpen]);
+
+ // close popover on ESC key press
+ useEffect(() => {
+ const handleKeyDown = (event: KeyboardEvent) => {
+ if (event.key === 'Escape' && isOpen) {
+ handleCancel();
+ }
+ };
+
+ if (isOpen) {
+ document.addEventListener('keydown', handleKeyDown);
+ }
+
+ return () => {
+ document.removeEventListener('keydown', handleKeyDown);
+ };
+ }, [isOpen, handleCancel]);
+
+ const content = (
+ <PopoverContent>
+ <FormItem>
+ <Label>{t('Column Label')}</Label>
+ <Input
+ ref={inputRef}
+ value={label}
+ onChange={e => setLabel(e.target.value)}
+ placeholder={t('Enter column label')}
+ />
+ </FormItem>
+ <ButtonContainer>
+ <StyledButton buttonStyle="secondary" onClick={handleCancel}>
+ {t('Close')}
+ </StyledButton>
+ <StyledButton
+ buttonStyle="primary"
+ onClick={handleSave}
+ disabled={!label.trim()}
+ >
+ {t('Save')}
+ </StyledButton>
+ </ButtonContainer>
+ </PopoverContent>
+ );
+
+ return (
+ <Popover
+ content={content}
+ title={title}
+ trigger="click"
+ open={isOpen}
+ onOpenChange={setIsOpen}
Review Comment:
**Suggestion:** When the popover is closed by clicking outside or by any
built-in Popover close action, only the local `isOpen` state is updated and
`handleCancel` is not called, so any unsaved label edits remain and will be
shown the next time the popover opens, unlike when closing via the Close button
or ESC which correctly reset the label; wire `onOpenChange` to also invoke
`handleCancel` on close so all close paths behave consistently. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
onOpenChange={open => {
setIsOpen(open);
if (!open) {
handleCancel();
}
}}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
As implemented, closing the popover via outside click (or any Popover-
driven close) only runs `setIsOpen(false)` and does NOT invoke
`handleCancel`, so any in-progress label edits remain in local state.
For an existing column, that means the stored config label stays e.g.
`"foo"` while reopening the popover shows the unsaved `"bar"`, unlike
closing via the Close button or ESC, both of which call `handleCancel`
and correctly reset the label. That's a real state/logic inconsistency
in the UI, not just style. Wiring `onOpenChange` so that `handleCancel`
runs on close makes all close paths behave the same and keeps the form
state in sync with the underlying config.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ChartColumnsControl/ChartColumnPopover.tsx
**Line:** 142:142
**Comment:**
*Logic Error: When the popover is closed by clicking outside or by any
built-in Popover close action, only the local `isOpen` state is updated and
`handleCancel` is not called, so any unsaved label edits remain and will be
shown the next time the popover opens, unlike when closing via the Close button
or ESC which correctly reset the label; wire `onOpenChange` to also invoke
`handleCancel` on close so all close paths behave consistently.
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/src/visualizations/TimeTable/components/SparklineCell/SparklineCell.test.tsx:
##########
@@ -143,3 +143,28 @@ test('should return empty div when all data is null', ()
=> {
expect(container).toBeInTheDocument();
expect(container.querySelector('svg')).toBeNull();
});
+
+test('should not render glyph points when showPoints is false', () => {
+ render(<SparklineCell {...defaultProps} showPoints={false} />);
+
+ const svg = document.querySelector('svg');
+ expect(svg).toBeInTheDocument();
+
+ const circles = svg!.querySelectorAll('circle');
+ expect(circles.length).toBe(0);
+});
+
+test('should apply custom color and strokeWidth to the series', () => {
+ render(
+ <SparklineCell
+ {...defaultProps}
+ color="#FF0000"
+ strokeWidth={4}
+ showPoints={true}
+ />,
+ );
+
+ const line = document.querySelector('[stroke="#FF0000"]');
+ expect(line).toBeInTheDocument();
+ expect(line).toHaveAttribute('stroke-width', '4');
+});
Review Comment:
**Suggestion:** This test expects custom `color` and `strokeWidth` props to
change the rendered series, but `SparklineCell` neither declares nor forwards
these props to the underlying chart, so the selector for `stroke="#FF0000"`
will never match and the test will consistently fail; it should be marked as a
TODO until the feature is implemented or rewritten to test existing behavior.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
test.todo('should apply custom color and strokeWidth to the series');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion is justified: the current test asserts a behavior that the
implementation simply does not have. `SparklineCell`'s props do not
declare `color`, `strokeWidth`, or `showPoints`, and the JSX where the
chart is rendered only passes `data`, `dataKey`, and accessors into the
`SeriesComponent`; it never forwards those extra props or otherwise uses
them. The chart theme hard-codes colors from `theme.colorText`, so no
element will ever have `stroke="#FF0000"` or `stroke-width="4"` due to
these props. As written, the test will always fail, not because of a
regression but because it's asserting a non-existent feature. Replacing it
with `test.todo(...)` correctly reflects that the behavior is not
implemented yet and removes a bogus failing test. This fixes a real logic
error in the test suite rather than being a cosmetic style change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/visualizations/TimeTable/components/SparklineCell/SparklineCell.test.tsx
**Line:** 157:170
**Comment:**
*Logic Error: This test expects custom `color` and `strokeWidth` props
to change the rendered series, but `SparklineCell` neither declares nor
forwards these props to the underlying chart, so the selector for
`stroke="#FF0000"` will never match and the test will consistently fail; it
should be marked as a TODO until the feature is implemented or rewritten to
test existing behavior.
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/sparklineHelpers.ts:
##########
@@ -0,0 +1,102 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { getTextDimension } from '@superset-ui/core';
+import { LinearScaleConfig } from '@visx/scale';
+import { AxisScaleOutput } from '@visx/axis';
+
+const TEXT_STYLE = {
+ fontSize: '12px',
+ fontWeight: 200,
+ letterSpacing: 0.4,
+} as const;
+
+/**
+ * Calculates the width needed for text in the sparkline
+ */
+export function getSparklineTextWidth(text: string): number {
+ return getTextDimension({ text, style: TEXT_STYLE }).width + 5;
+}
+
+/**
+ * Validates if a value can be used as a bound
+ */
+export function isValidBoundValue(value?: number | string): value is number {
+ return (
+ value !== null &&
+ value !== undefined &&
+ value !== '' &&
+ typeof value === 'number' &&
+ !Number.isNaN(value)
+ );
+}
+
+/**
+ * Calculates min and max values from valid data points
+ */
+export function getDataBounds(validData: number[]): [number, number] {
+ if (validData.length === 0) {
+ return [0, 0];
+ }
+
+ const min = Math.min(...validData);
+ const max = Math.max(...validData);
+ return [min, max];
+}
+
+/**
+ * Creates Y scale configuration based on data and bounds
+ */
+export function createYScaleConfig(
+ validData: number[],
+ yAxisBounds?: [number | undefined, number | undefined],
+): {
+ yScaleConfig: LinearScaleConfig<AxisScaleOutput>;
+ min: number;
+ max: number;
+} {
+ const [dataMin, dataMax] = getDataBounds(validData);
+ const [minBound, maxBound] = yAxisBounds || [undefined, undefined];
+
+ const hasMinBound = isValidBoundValue(minBound);
+ const hasMaxBound = isValidBoundValue(maxBound);
+
+ const finalMin = hasMinBound ? minBound! : dataMin;
+ const finalMax = hasMaxBound ? maxBound! : dataMax;
+
+ const config: LinearScaleConfig<AxisScaleOutput> = {
+ type: 'linear',
+ zero: hasMinBound && minBound! <= 0,
+ domain: [finalMin, finalMax],
+ };
+
+ return {
+ yScaleConfig: config,
+ min: finalMin,
+ max: finalMax,
+ };
+}
+
+/**
+ * Transforms raw data into chart data points
+ */
+export function transformChartData(
+ data: Array<number | null>,
+): Array<{ x: number; y: number }> {
+ return data.map((num, idx) => ({ x: idx, y: num ?? 0 }));
+}
Review Comment:
**Suggestion:** This file reimplements helper functions that already exist
in the TimeTable `sparklineHelpers` module, which introduces duplicated logic
that can drift over time; instead, you should import and re-export the existing
helpers so any future fixes or behavior changes stay consistent across both
usages. [code quality]
**Severity Level:** Minor ⚠️
```suggestion
export {
getSparklineTextWidth,
isValidBoundValue,
getDataBounds,
createYScaleConfig,
transformChartData,
} from
'src/visualizations/TimeTable/utils/sparklineHelpers/sparklineHelpers';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This file is a straight copy of the existing `TimeTable` sparkline helpers
(`src/visualizations/TimeTable/utils/sparklineHelpers/sparklineHelpers.ts`),
i.e. the functions, signatures, and logic are duplicated verbatim for the same
purpose. That is exactly the kind of real duplication the rubric calls out: two
modules implementing the same utility logic, which will drift if only one gets
bug fixes. Replacing the implementation here with a re-export of the shared
helpers removes that duplication and centralizes behavior, which is a concrete
code-quality improvement rather than mere cosmetics. The proposed `export { … }
from 'src/visualizations/TimeTable/utils/sparklineHelpers/sparklineHelpers';`
is syntactically valid and preserves the public API of this module; the only
change is where the logic lives.
</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/sparklineHelpers.ts
**Line:** 19:102
**Comment:**
*Code Quality: This file reimplements helper functions that already
exist in the TimeTable `sparklineHelpers` module, which introduces duplicated
logic that can drift over time; instead, you should import and re-export the
existing helpers so any future fixes or behavior changes stay consistent across
both usages.
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:
##########
@@ -239,8 +240,14 @@ export const useColDefs = ({
'last',
],
}),
- cellRenderer: (p: CellRendererProps) =>
- isTextColumn ? TextCellRenderer(p) : NumericCellRenderer(p),
+ cellRenderer: (p: CellRendererProps) => {
+ // Check if column should use a chart renderer
+ if (shouldUseChartRenderer(col)) {
+ return getChartRenderer(col.config?.chartType || 'sparkline')(p);
+ }
Review Comment:
**Suggestion:** The new chart-aware `cellRenderer` applies the chart
renderer to all rows in the column, including the pinned bottom
"Summary"/totals row, which previously relied on the special logic inside
`TextCellRenderer` and `NumericCellRenderer` (e.g. showing "Summary" label and
bold totals). This changes the behavior of the summary row and can result in
charts being rendered where totals/summary text were expected. To preserve
existing summary behavior, the cell renderer should always delegate pinned
bottom rows to the original text/numeric renderers and only use the chart
renderer for non-pinned data rows. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const { node } = p;
// Always use the existing renderers for pinned summary/total rows
if (node?.rowPinned === 'bottom') {
return isTextColumn ? TextCellRenderer(p) :
NumericCellRenderer(p);
}
// Check if column should use a chart renderer
if (shouldUseChartRenderer(col)) {
return getChartRenderer(col.config?.chartType || 'sparkline')(p);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion addresses a real behavioral regression, not just cosmetics.
The existing `cellRenderer` always routes pinned bottom rows (summary/totals)
through `getChartRenderer` whenever `shouldUseChartRenderer(col)` is true,
which bypasses the specialized summary logic that already exists in the
renderers:
- `TextCellRenderer` has explicit handling for `node?.rowPinned === 'bottom'`
to render the "Summary" label, tooltip, and to hide empty summary cells.
- `NumericCellRenderer` similarly checks `node?.rowPinned === 'bottom'` and
returns a bold `StyledTotalCell` showing the formatted total value.
By introducing chart rendering at the column level without guarding on
`rowPinned`, pinned summary rows start rendering charts instead of the
intended summary text/total styles. The proposed `improved_code` restores
the previous behavior by short-circuiting pinned bottom rows directly to
the existing text/numeric renderers, and only applying `getChartRenderer`
for non-pinned data rows. This is a clear logic fix tied to real,
existing behavior, not a stylistic preference.
</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:** 244:247
**Comment:**
*Logic Error: The new chart-aware `cellRenderer` applies the chart
renderer to all rows in the column, including the pinned bottom
"Summary"/totals row, which previously relied on the special logic inside
`TextCellRenderer` and `NumericCellRenderer` (e.g. showing "Summary" label and
bold totals). This changes the behavior of the summary row and can result in
charts being rendered where totals/summary text were expected. To preserve
existing summary behavior, the cell renderer should always delegate pinned
bottom rows to the original text/numeric renderers and only use the chart
renderer for non-pinned data rows.
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/renderers/SparklineRenderer.tsx:
##########
@@ -0,0 +1,82 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { styled, useTheme } from '@apache-superset/core/ui';
+import { CustomCellRendererProps } from
'@superset-ui/core/components/ThemedAgGridReact';
+import { InputColumn } from '../types';
+import SparklineCell from '../components/SparklineCell';
+import { parseArrayValue } from '../utils/formatValue';
+import { rgbToHex } from '../utils/chartRenderers';
+
+const CellContainer = styled.div<{ align?: string }>`
+ display: flex;
+ align-items: center;
+ justify-content: ${({ align }) => align || 'left'};
+ width: 100%;
+`;
+
+export const SparklineRenderer = (
+ params: CustomCellRendererProps & {
+ col: InputColumn;
+ },
+) => {
+ const { data, col } = params;
+ const value = parseArrayValue(data);
+
+ // Chart configuration is now processed in transformProps with proper
defaults
+ const chartConfig = col?.config || {};
+ const {
+ width = 100, // Default from transformProps
+ height = 60, // Default from transformProps
+ color,
+ strokeWidth = 1.5, // Default from transformProps
+ showValues = true, // Default from transformProps
+ showPoints = true, // Default from transformProps
+ } = chartConfig;
+
+ if (!Array.isArray(value)) {
+ return <CellContainer>N/A</CellContainer>;
+ }
+
+ const dataKey = col?.metricName || col?.key || 'value';
+ const ariaLabel = `Sparkline chart for ${col?.label || dataKey}`;
+ const theme = useTheme();
+ const chartColor =
+ typeof color === 'object' ? rgbToHex(color) : color || theme.colorText;
Review Comment:
**Suggestion:** The `typeof color === 'object'` check treats `null` as an
object, so if `chartConfig.color` is ever `null` at runtime the code will call
`rgbToHex(null)`, which will typically try to read properties on `null` and
throw, causing the entire cell render to fail; adding a truthiness check avoids
this null dereference. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
color && typeof color === 'object' ? rgbToHex(color) : color ||
theme.colorText;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
In JavaScript, `typeof null` is `'object'`, so this code path will call
`rgbToHex(color)` even when
`color` is `null`. The imported `rgbToHex` utility is designed to work with
real color data, not
`null`, and calling it with `null` is very likely to result in a property
access error and a broken
cell render. Since `InputColumn.config` is an untyped `Record<string, any>`,
there is nothing that
prevents `color` from being `null` at runtime (for example, from
user-configurable settings).
Guarding with `color && typeof color === 'object'` prevents this potential
null dereference while
preserving existing behavior for valid colors and falling back to
`theme.colorText` when `color` is
falsy. This is a genuine runtime-safety fix, not mere stylistic tinkering.
</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/renderers/SparklineRenderer.tsx
**Line:** 60:60
**Comment:**
*Null Pointer: The `typeof color === 'object'` check treats `null` as
an object, so if `chartConfig.color` is ever `null` at runtime the code will
call `rgbToHex(null)`, which will typically try to read properties on `null`
and throw, causing the entire cell render to fail; adding a truthiness check
avoids this null dereference.
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/renderers/SparklineRenderer.tsx:
##########
@@ -0,0 +1,82 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { styled, useTheme } from '@apache-superset/core/ui';
+import { CustomCellRendererProps } from
'@superset-ui/core/components/ThemedAgGridReact';
+import { InputColumn } from '../types';
+import SparklineCell from '../components/SparklineCell';
+import { parseArrayValue } from '../utils/formatValue';
+import { rgbToHex } from '../utils/chartRenderers';
+
+const CellContainer = styled.div<{ align?: string }>`
+ display: flex;
+ align-items: center;
+ justify-content: ${({ align }) => align || 'left'};
+ width: 100%;
+`;
+
+export const SparklineRenderer = (
+ params: CustomCellRendererProps & {
+ col: InputColumn;
+ },
+) => {
+ const { data, col } = params;
+ const value = parseArrayValue(data);
Review Comment:
**Suggestion:** The renderer is passing the entire row object (`data`) into
`parseArrayValue` instead of the cell's value, so for typical AG Grid usage
`parseArrayValue` will receive an object instead of the expected scalar/array
value and will usually fail to parse to an array, causing the renderer to show
`N/A` for all rows instead of the intended sparkline. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const { value: rawValue, data, col } = params;
const cellValue = rawValue ?? (col?.key ? data?.[col.key] : undefined);
const value = parseArrayValue(cellValue);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
In the AG Grid renderers in this codebase (see `NumericCellRenderer`),
`CustomCellRendererProps`
exposes `value` as the actual cell value and `data` as the full row object.
Here the renderer is
feeding the entire row object into `parseArrayValue`, and then immediately
checking
`Array.isArray(value)` and bailing out to `N/A` if it is not an array.
Passing a row-shaped object
instead of the cell's scalar/array payload is a classic logic bug and will
almost always cause the
check to fail, preventing valid sparkline data from rendering. The suggested
change uses
`params.value` first and falls back to `data[col.key]` when needed, which
aligns with how other
renderers obtain the cell value and directly addresses the functional bug
rather than just
restyling the code.
</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/renderers/SparklineRenderer.tsx
**Line:** 38:39
**Comment:**
*Logic Error: The renderer is passing the entire row object (`data`)
into `parseArrayValue` instead of the cell's value, so for typical AG Grid
usage `parseArrayValue` will receive an object instead of the expected
scalar/array value and will usually fail to parse to an array, causing the
renderer to show `N/A` for all rows instead of the intended sparkline.
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]
