codeant-ai-for-open-source[bot] commented on code in PR #37779: URL: https://github.com/apache/superset/pull/37779#discussion_r2779169275
########## superset-frontend/plugins/plugin-chart-unified-list-bar/src/components/Row.tsx: ########## @@ -0,0 +1,208 @@ +/** + * 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 React from 'react'; +import { TimeseriesDataRecord, safeHtmlSpan } from '@superset-ui/core'; +import { + RowContainer, + KeySection, + KeyField, + KeySubField, + ContentSection, + SecondaryFieldsContainer, + SecondaryField, + BarSection, + MetricValue, + BarContainer, + BarFill, + IconContainer, +} from '../styles'; +import { UnifiedListBarChartCustomizeProps } from '../types'; + +// Built-in severity icon mapping (0=none, 1=warning, 2=error, 3=critical) +const getSeverityIcon = (severityValue: any) => { + const numVal = Number(severityValue); + switch (numVal) { + case 0: return null; // No icon + case 1: return { icon: '⚠', color: '#f39c12' }; // Warning - yellow/orange + case 2: return { icon: '✖', color: '#e74c3c' }; // Error - red + case 3: return { icon: '🔥', color: '#c0392b' }; // Critical - dark red + default: return null; + } +}; + +// Helper to convert hex color (with or without #) to valid CSS color +const ensureHexColor = (color: any): string => { + if (!color) return '#000000'; + const colorStr = String(color).trim(); + if (colorStr.startsWith('#')) return colorStr; + if (/^[0-9a-fA-F]{6}$/.test(colorStr)) return `#${colorStr}`; + if (/^[0-9a-fA-F]{3}$/.test(colorStr)) return `#${colorStr}`; + return '#000000'; // Default black +}; + +interface RowProps { + record: TimeseriesDataRecord; + customize: UnifiedListBarChartCustomizeProps; + maxMetricValue: number; +} + +export const Row: React.FC<RowProps> = ({ record, customize, maxMetricValue }) => { + const { + keyColumn, + keySubColumn, + secondaryColumns, + metricColumn, + maxMetricColumn, + severityColumn, + colorColumn, + rowsPerItem, + showBar, + showMetricValue, + keyFontSize, + keyColor, + keySubFontSize, + secondaryFontSize, + barColorPositive, + } = customize; + + // DEBUG: Log all customize values + console.log('=== ROW DEBUG ==='); + console.log('keyColumn:', keyColumn); + console.log('keySubColumn:', keySubColumn); + console.log('colorColumn:', colorColumn); + console.log('metricColumn:', metricColumn); + console.log('showBar:', showBar, 'showMetricValue:', showMetricValue); + console.log('keyFontSize:', keyFontSize, 'keySubFontSize:', keySubFontSize, 'secondaryFontSize:', secondaryFontSize); + console.log('record keys:', Object.keys(record)); + console.log('record:', record); Review Comment: **Suggestion:** The unconditional debug `console.log` calls inside the row render will execute for every row render, which can severely degrade performance and flood the browser console when many rows are displayed, especially in large tables or frequent re-renders. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Unified list bar chart renders many rows simultaneously. - ⚠️ Console flooded by logs from Row.tsx:84-93. - ⚠️ Rendering slows with thousands of row-level console logs. ``` </details> ```suggestion ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Build and run the Superset frontend with this PR, ensuring the unified list bar plugin is available under `superset-frontend/plugins/plugin-chart-unified-list-bar`. 2. Configure any chart to use the unified list bar visualization so that its main component renders one `<Row>` per result record from the backend (Row component defined in `src/components/Row.tsx:65`). 3. When the chart renders N records, React calls `Row` N times; for each call, the debug logging block at `Row.tsx:84-93` executes multiple `console.log` statements, including logging the entire `record` object. 4. Open the browser devtools console while scrolling or triggering re-renders (e.g., filter changes); observe the console being flooded with log entries and rendering becoming noticeably slower for large result sets, due to repeated stringification and logging of rows. ``` </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-unified-list-bar/src/components/Row.tsx **Line:** 84:93 **Comment:** *Possible Bug: The unconditional debug `console.log` calls inside the row render will execute for every row render, which can severely degrade performance and flood the browser console when many rows are displayed, especially in large tables or frequent re-renders. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=424e7467310ee8cdf5e8129ca07c82d927851ec767f353acdba13610a909be23&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=424e7467310ee8cdf5e8129ca07c82d927851ec767f353acdba13610a909be23&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-unified-list-bar/src/UnifiedListBarChart.tsx: ########## @@ -0,0 +1,69 @@ +/** + * 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 React from 'react'; +import { UnifiedListBarChartProps } from './types'; +import { Styles } from './styles'; +import { Row } from './components/Row'; + +export default function UnifiedListBarChart(props: UnifiedListBarChartProps) { + const { data, height, width, customize } = props; + const { metricColumn, maxMetricColumn } = customize; + + console.log('UnifiedListBarChart render props:', props); + + // Calculate Max Metric for Bars if not provided via column + let maxMetricValue = 0; + if (maxMetricColumn) { + // If a column is specified, we might take the max of that column across the dataset + // or assume it's per-row. Re-reading reqs: "max_metric_column -> optional". + // Usually "max" implies the denominator for the percentage calculation. + // If provided, we likely want the max *value* in that column, or maybe it is the max target per row? + // "percent = value / max_value" implies row-level max if it varies, or global max. + // Let's assume global max of the max_metric_column for now, or fall back to max of metric_column. + + // Actually, if it's "max_metric: 20" in the example, it looks like a row-level target. + // But for calculating bar width relative to each other, we usually want a global scale. + // However, the example "16 / 20" suggests row-level target. + // Let's handle both: if maxMetricColumn exists, use that record's value as denominator. + // If NOT, find the global max of metricColumn. + } else if (metricColumn) { + maxMetricValue = Math.max(...data.map(d => (d[metricColumn] as number) || 0)); + } + + return ( + <Styles height={height} width={width}> + {data.map((record, index) => { + // Determine row-specific max + let rowMax = maxMetricValue; + if (maxMetricColumn) { + rowMax = (record[maxMetricColumn] as number) || 0; + } + + return ( + <Row + key={index} Review Comment: **Suggestion:** Using the array index as the React key for list items can lead to incorrect row updates and visual glitches when the data order changes or items are inserted/removed, because React will reuse components based on the index rather than the actual record identity; a more stable key derived from the record should be used instead. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Rows may show stale content after data reordering. - ⚠️ Potential visual glitches when rows inserted or removed. ``` </details> ```suggestion key={metricColumn ? String(record[metricColumn]) : String(index)} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open or create a chart in Superset using the "Unified List Bar" visualization so that `UnifiedListBarChart` in `superset-frontend/plugins/plugin-chart-unified-list-bar/src/UnifiedListBarChart.tsx:24` is used to render each row from the query `data` array. 2. In `UnifiedListBarChart`, the JSX at lines 51–66 executes `data.map((record, index) => { ... return (<Row key={index} ... />); })`, rendering one `<Row>` per record while using the array index as the React `key`. 3. While staying on the same Explore view (so the chart component instance remains mounted via the chart plugin registry in `superset-frontend/packages/superset-ui-core/src/models/Registry.ts:204-285`), change controls so that the query result order or membership changes (e.g., apply a new filter or change sorting), causing the `data` array passed into `UnifiedListBarChart` to have rows inserted/removed or reordered. 4. React reconciles children using `key={index}`, so when the `data` array shape or order changes, existing `<Row>` components are reused for different `record` objects, which can lead to incorrect component reuse and potential stale internal state or visual glitches at the row level if `<Row>` holds local state or effects tied to a specific record identity. ``` </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-unified-list-bar/src/UnifiedListBarChart.tsx **Line:** 60:60 **Comment:** *Logic Error: Using the array index as the React key for list items can lead to incorrect row updates and visual glitches when the data order changes or items are inserted/removed, because React will reuse components based on the index rather than the actual record identity; a more stable key derived from the record should be used instead. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=4224624c8a9e6931b5a75a9323853995b74700cb23d2565f81e01f104a23143d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=4224624c8a9e6931b5a75a9323853995b74700cb23d2565f81e01f104a23143d&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-unified-list-bar/src/plugin/transformProps.ts: ########## @@ -0,0 +1,123 @@ +/** + * 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 { ChartProps, TimeseriesDataRecord, ensureIsArray } from '@superset-ui/core'; +import { UnifiedListBarChartProps, UnifiedListBarChartCustomizeProps } from '../types'; + +// Helper to extract clean column name from formData value (which can be string, array, or object) +const getColumnName = (col: any): string => { + if (!col) return ''; + if (typeof col === 'string') return col; + if (Array.isArray(col) && col.length > 0) return getColumnName(col[0]); + if (typeof col === 'object') { + if (col.label) return col.label; + if (col.column_name) return col.column_name; + if (col.column && col.column.column_name) return col.column.column_name; + if (col.sqlExpression) return col.sqlExpression; + } + return String(col); +}; + +// Helper to convert ColorPickerControl value (RGBA object) to CSS color string +const rgbaToString = (color: any): string => { + if (typeof color === 'string') return color; + if (color && typeof color === 'object' && 'r' in color && 'g' in color && 'b' in color) { + const a = color.a !== undefined ? color.a : 1; + return `rgba(${color.r}, ${color.g}, ${color.b}, ${a})`; + } + return '#000000'; // Default black +}; + +export default function transformProps(chartProps: ChartProps): UnifiedListBarChartProps { + const { width, height, formData, queriesData } = chartProps; + + // Extract values using camelCase (Superset converts snake_case control names to camelCase) + const { + keyColumn, + keySubColumn, + secondaryColumns, + metricColumn, + maxMetricColumn, + severityColumn, + colorColumn, + rowsPerItem = '2', + alignMetric = 'right', + showBar = true, + showMetricValue = true, + keyFontSize = 16, + keyColor, + keySubFontSize = 11, + secondaryFontSize = 12, + barColorPositive, + barColorNegative, + conditionalColorRules, + iconRules, + } = formData; + + const data = queriesData[0].data as TimeseriesDataRecord[]; + + // DEBUG: Log to verify values + console.log('=== TRANSFORM PROPS DEBUG ==='); + console.log('keyColumn:', keyColumn); + console.log('keySubColumn:', keySubColumn); + console.log('colorColumn:', colorColumn); + console.log('metricColumn:', metricColumn); + console.log('keyFontSize:', keyFontSize, 'secondaryFontSize:', secondaryFontSize); + + // Extract clean column names + const keyColumnName = getColumnName(keyColumn); + const keySubColumnName = getColumnName(keySubColumn); + const secondaryColumnNames = ensureIsArray(secondaryColumns).map(getColumnName).filter(Boolean); + const metricColumnName = getColumnName(metricColumn); + const maxMetricColumnName = getColumnName(maxMetricColumn); + const severityColumnName = getColumnName(severityColumn); + const colorColumnName = getColumnName(colorColumn); + + const customize: UnifiedListBarChartCustomizeProps = { + keyColumn: keyColumnName, + keySubColumn: keySubColumnName || undefined, + secondaryColumns: secondaryColumnNames, + metricColumn: metricColumnName || undefined, + maxMetricColumn: maxMetricColumnName || undefined, + severityColumn: severityColumnName || undefined, + colorColumn: colorColumnName || undefined, + rowsPerItem: rowsPerItem, + alignMetric: alignMetric, + showBar: showBar, + showMetricValue: showMetricValue, + keyFontSize: Number(keyFontSize) || 16, + keyColor: rgbaToString(keyColor), + keySubFontSize: Number(keySubFontSize) || 11, + secondaryFontSize: Number(secondaryFontSize) || 12, + barColorPositive: rgbaToString(barColorPositive), + barColorNegative: rgbaToString(barColorNegative), + conditionalColorRules: typeof conditionalColorRules === 'string' + ? JSON.parse(conditionalColorRules) + : conditionalColorRules, + iconRules: typeof iconRules === 'string' + ? JSON.parse(iconRules) Review Comment: **Suggestion:** Using JSON.parse directly on the string values for the conditional and icon rules will throw a runtime error when those controls are left empty (commonly an empty string), causing the chart to fail to render instead of simply having no rules applied. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Unified List Bar chart crashes with empty rule fields. - ⚠️ Users cannot safely clear conditional color rules. - ⚠️ Any consumer of transformProps must defensively validate inputs. ``` </details> ```suggestion conditionalColorRules: typeof conditionalColorRules === 'string' ? conditionalColorRules.trim() ? JSON.parse(conditionalColorRules) : undefined : conditionalColorRules, iconRules: typeof iconRules === 'string' ? iconRules.trim() ? JSON.parse(iconRules) : undefined ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import `transformProps` from `superset-frontend/plugins/plugin-chart-unified-list-bar/src/plugin/transformProps.ts`. 2. Construct a `ChartProps` instance (from `@superset-ui/core`) where `formData` includes `conditionalColorRules: ''` (empty string) and omits `iconRules`; provide a minimal valid `queriesData[0].data` array. 3. Call `transformProps(chartProps)`; inside the function at lines 109–111, `typeof conditionalColorRules === 'string'` evaluates to true for the empty string. 4. `JSON.parse(conditionalColorRules)` is executed with `''` (empty string) at `transformProps.ts:110`, which throws a `SyntaxError: Unexpected end of JSON input`, causing the transform (and thus any chart rendering using these props) to fail. ``` </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-unified-list-bar/src/plugin/transformProps.ts **Line:** 109:113 **Comment:** *Possible Bug: Using JSON.parse directly on the string values for the conditional and icon rules will throw a runtime error when those controls are left empty (commonly an empty string), causing the chart to fail to render instead of simply having no rules applied. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=b49cad816a1426f3c81f9147267ef907a2efef8d7f18d0dc2588b0f5d5f99a9e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=b49cad816a1426f3c81f9147267ef907a2efef8d7f18d0dc2588b0f5d5f99a9e&reaction=dislike'>👎</a> ########## docker/pythonpath_dev/superset_config.py: ########## @@ -142,3 +142,94 @@ class CeleryConfig: ) except ImportError: logger.info("Using default Docker config...") + + +EXTRA_CATEGORICAL_COLOR_SCHEMES = [ + { + "id": 'risk_meter', + "description": '', + "label": 'Rik Meter Theme RYG', + "colors": + ['#008450','#D19900','#B81D13'] + }, + { + "id": 'ey_color_palette', + "description": '', + "label": 'EY Color Palette I', + "isDefault": False, + "colors": + ['#E0B0FF', '#FF736A', '#FFB46A', '#8CE8AD', '#42C9C2', '#87D3F2', '#9C82D4','#922B73','#168736'] + }, + { + "id": 'ey_color_palette_2', + "description": '', + "label": 'EY Color Palette II', + "isDefault": False, + "colors": + ['#B14891', '#FF736A', '#FF9831', '#c9ffe0', '#27ACAA', '#4EBEEB', '#D3888D','#e0b0ff'] + }, + { + "id": 'subtle_color_palette_1', + "description": '', + "label": 'Subtle Color Palette I', + "isDefault": False, + "colors": + ['#D45E77', '#D07687', '#D3888D', '#D4A4A9', '#DCBABF', '#DEC3BE', '#E3D0D5','#E7DDEC','#EDE5DC'] + }, + { + "id": 'subtle_color_palette_2', + "description": '', + "label": 'Subtle Color Palette II', + "isDefault": False, + "colors": + ['#794F6D', '#93727B', '#A5808B', '#AD9489', '#BCA28A', '#CAB095', '#F0D5C7','#FCA692'] + }, + { + "id": 'subtle_color_palette_3', + "description": '', + "label": 'Subtle Color Palette III', + "isDefault": False, + "colors": + ['#B0ABAF', '#C1AEB1', '#C2C1C3', '#D2CCC9', '#D9D7D3', '#D4AF9B', '#DEB891','#E7C188','#EECA81'] + }, + { + "id": 'subtle_color_palette_4', + "description": '', + "label": 'Subtle Color Palette IV', + "isDefault": False, + "colors": + ['#85C7DE', '#A3A5A9', '#D9C8C4', '#D3888D', '#E7C188', '#D07687','#9DA891','#93727B','#A0C4E2'] + }, + { + "id": 'subtle_color_palette_blue_shades', + "description": '', + "label": 'Subtle Color Palette Blue Shades', + "isDefault": False, + "colors": + ['#91C8E4','#37B7C3','#92C7CF', '#AAD7D9', '#BFCFE7', '#E5E1DA','#ADC4CE'] + }, + { + "id": 'subtle_color_palette_piechart', + "description": '', + "label": 'Subtle Color Palette Pie Charts', + "isDefault": False, + "colors": + [ '#083153','#0A558E','#1275C1','#55A6FC','#AFCDFB'] + }, + { + "id": 'subtle_color_palette_dounut', + "description": '', + "label": 'Subtle Color Palette Dounut', + "isDefault": False, + "colors": + [ '#AFCDFB','#55A6FC','#1275C1','#0A558E','#083153'] + }, + { + "id": 'subtle_color_palette_for_bars', + "description": '', + "label": 'Subtle Color Palette For Bars', + "isDefault": True, + "colors": + ['#D2E1FA','#AFCDFB','#85B9FD','#55A6FC','#188CE5','#1275C1','#0F69AE','#0A558E','#064372','#083153'] + } + ] Review Comment: **Suggestion:** Defining the color schemes unconditionally after importing `superset_config_docker` prevents local Docker configuration from overriding `EXTRA_CATEGORICAL_COLOR_SCHEMES`, which contradicts the stated intent of the import block and makes any custom color scheme definitions in `superset_config_docker.py` ineffective. Wrap this assignment so it only applies when no prior value has been set, preserving the override behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Local Docker config cannot override categorical color schemes. - ⚠️ Custom themes defined in superset_config_docker.py are ignored. ``` </details> ```suggestion EXTRA_CATEGORICAL_COLOR_SCHEMES = globals().get( "EXTRA_CATEGORICAL_COLOR_SCHEMES", [ { "id": 'risk_meter', "description": '', "label": 'Rik Meter Theme RYG', "colors": ['#008450', '#D19900', '#B81D13'], }, { "id": 'ey_color_palette', "description": '', "label": 'EY Color Palette I', "isDefault": False, "colors": [ '#E0B0FF', '#FF736A', '#FFB46A', '#8CE8AD', '#42C9C2', '#87D3F2', '#9C82D4', '#922B73', '#168736', ], }, { "id": 'ey_color_palette_2', "description": '', "label": 'EY Color Palette II', "isDefault": False, "colors": [ '#B14891', '#FF736A', '#FF9831', '#c9ffe0', '#27ACAA', '#4EBEEB', '#D3888D', '#e0b0ff', ], }, { "id": 'subtle_color_palette_1', "description": '', "label": 'Subtle Color Palette I', "isDefault": False, "colors": [ '#D45E77', '#D07687', '#D3888D', '#D4A4A9', '#DCBABF', '#DEC3BE', '#E3D0D5', '#E7DDEC', '#EDE5DC', ], }, { "id": 'subtle_color_palette_2', "description": '', "label": 'Subtle Color Palette II', "isDefault": False, "colors": [ '#794F6D', '#93727B', '#A5808B', '#AD9489', '#BCA28A', '#CAB095', '#F0D5C7', '#FCA692', ], }, { "id": 'subtle_color_palette_3', "description": '', "label": 'Subtle Color Palette III', "isDefault": False, "colors": [ '#B0ABAF', '#C1AEB1', '#C2C1C3', '#D2CCC9', '#D9D7D3', '#D4AF9B', '#DEB891', '#E7C188', '#EECA81', ], }, { "id": 'subtle_color_palette_4', "description": '', "label": 'Subtle Color Palette IV', "isDefault": False, "colors": [ '#85C7DE', '#A3A5A9', '#D9C8C4', '#D3888D', '#E7C188', '#D07687', '#9DA891', '#93727B', '#A0C4E2', ], }, { "id": 'subtle_color_palette_blue_shades', "description": '', "label": 'Subtle Color Palette Blue Shades', "isDefault": False, "colors": [ '#91C8E4', '#37B7C3', '#92C7CF', '#AAD7D9', '#BFCFE7', '#E5E1DA', '#ADC4CE', ], }, { "id": 'subtle_color_palette_piechart', "description": '', "label": 'Subtle Color Palette Pie Charts', "isDefault": False, "colors": ['#083153', '#0A558E', '#1275C1', '#55A6FC', '#AFCDFB'], }, { "id": 'subtle_color_palette_dounut', "description": '', "label": 'Subtle Color Palette Dounut', "isDefault": False, "colors": ['#AFCDFB', '#55A6FC', '#1275C1', '#0A558E', '#083153'], }, { "id": 'subtle_color_palette_for_bars', "description": '', "label": 'Subtle Color Palette For Bars', "isDefault": True, "colors": [ '#D2E1FA', '#AFCDFB', '#85B9FD', '#55A6FC', '#188CE5', '#1275C1', '#0F69AE', '#0A558E', '#064372', '#083153', ], }, ], ) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create `superset_config_docker.py` on the Python path that defines `EXTRA_CATEGORICAL_COLOR_SCHEMES = [{'id': 'custom_scheme', ...}]`. 2. Start Superset using the Docker dev image, which loads `docker/pythonpath_dev/superset_config.py` as the config module. 3. During module import, the `try` block at lines 131–142 in `docker/pythonpath_dev/superset_config.py` imports `superset_config_docker` and `from superset_config_docker import *`, evaluating its `EXTRA_CATEGORICAL_COLOR_SCHEMES` assignment. 4. Immediately after, the assignment at lines 147–235 in `docker/pythonpath_dev/superset_config.py` rebinds `EXTRA_CATEGORICAL_COLOR_SCHEMES` to the hard-coded list, so any custom value from `superset_config_docker.py` is lost and the UI only exposes the hard-coded color schemes. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docker/pythonpath_dev/superset_config.py **Line:** 147:235 **Comment:** *Logic Error: Defining the color schemes unconditionally after importing `superset_config_docker` prevents local Docker configuration from overriding `EXTRA_CATEGORICAL_COLOR_SCHEMES`, which contradicts the stated intent of the import block and makes any custom color scheme definitions in `superset_config_docker.py` ineffective. Wrap this assignment so it only applies when no prior value has been set, preserving the override 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=af0fd17b087745ecfb853aae73303a8c925479bc47355a0f4208ef62162f5887&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=af0fd17b087745ecfb853aae73303a8c925479bc47355a0f4208ef62162f5887&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-unified-list-bar/src/components/Row.tsx: ########## @@ -0,0 +1,208 @@ +/** + * 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 React from 'react'; +import { TimeseriesDataRecord, safeHtmlSpan } from '@superset-ui/core'; +import { + RowContainer, + KeySection, + KeyField, + KeySubField, + ContentSection, + SecondaryFieldsContainer, + SecondaryField, + BarSection, + MetricValue, + BarContainer, + BarFill, + IconContainer, +} from '../styles'; +import { UnifiedListBarChartCustomizeProps } from '../types'; + +// Built-in severity icon mapping (0=none, 1=warning, 2=error, 3=critical) +const getSeverityIcon = (severityValue: any) => { + const numVal = Number(severityValue); + switch (numVal) { + case 0: return null; // No icon + case 1: return { icon: '⚠', color: '#f39c12' }; // Warning - yellow/orange + case 2: return { icon: '✖', color: '#e74c3c' }; // Error - red + case 3: return { icon: '🔥', color: '#c0392b' }; // Critical - dark red + default: return null; + } +}; + +// Helper to convert hex color (with or without #) to valid CSS color +const ensureHexColor = (color: any): string => { + if (!color) return '#000000'; + const colorStr = String(color).trim(); + if (colorStr.startsWith('#')) return colorStr; + if (/^[0-9a-fA-F]{6}$/.test(colorStr)) return `#${colorStr}`; + if (/^[0-9a-fA-F]{3}$/.test(colorStr)) return `#${colorStr}`; + return '#000000'; // Default black +}; + +interface RowProps { + record: TimeseriesDataRecord; + customize: UnifiedListBarChartCustomizeProps; + maxMetricValue: number; +} + +export const Row: React.FC<RowProps> = ({ record, customize, maxMetricValue }) => { + const { + keyColumn, + keySubColumn, + secondaryColumns, + metricColumn, + maxMetricColumn, + severityColumn, + colorColumn, + rowsPerItem, + showBar, + showMetricValue, + keyFontSize, + keyColor, + keySubFontSize, + secondaryFontSize, + barColorPositive, + } = customize; + + // DEBUG: Log all customize values + console.log('=== ROW DEBUG ==='); + console.log('keyColumn:', keyColumn); + console.log('keySubColumn:', keySubColumn); + console.log('colorColumn:', colorColumn); + console.log('metricColumn:', metricColumn); + console.log('showBar:', showBar, 'showMetricValue:', showMetricValue); + console.log('keyFontSize:', keyFontSize, 'keySubFontSize:', keySubFontSize, 'secondaryFontSize:', secondaryFontSize); + console.log('record keys:', Object.keys(record)); + console.log('record:', record); + + let effectiveKeyColumn = keyColumn; + + // FALLBACK: If keyColumn is empty, use the first column + if (!effectiveKeyColumn && record && Object.keys(record).length > 0) { + effectiveKeyColumn = Object.keys(record)[0]; + } + + // FALLBACK: If no secondary columns configured, auto-discover from record + // EXCLUDE: key column, key sub column, metric column, max metric column, severity column, color column + let effectiveSecondaryColumns = secondaryColumns; + if (!effectiveSecondaryColumns || effectiveSecondaryColumns.length === 0) { + const excludedColumns = [ + effectiveKeyColumn, + keySubColumn, + metricColumn, + maxMetricColumn, + severityColumn, + colorColumn, + ].filter(Boolean); + + effectiveSecondaryColumns = Object.keys(record).filter(key => !excludedColumns.includes(key)); + } + + const keyValue = record[effectiveKeyColumn]; + const keySubValue = keySubColumn ? record[keySubColumn] : undefined; + const metricValue = metricColumn ? (record[metricColumn] as number) : 0; + const severityValue = severityColumn ? record[severityColumn] : undefined; + + // Get color from data column if specified, otherwise use default/configured color + const rawColorValue = colorColumn ? record[colorColumn] : null; + const dataColor = colorColumn ? ensureHexColor(rawColorValue) : null; + const effectiveKeyColor = dataColor || keyColor || '#000000'; + const effectiveBarColor = dataColor || barColorPositive || '#4caf50'; + + // DEBUG: Log extracted values + console.log('keyValue:', keyValue); + console.log('keySubValue:', keySubValue); + console.log('metricValue:', metricValue); + console.log('rawColorValue:', rawColorValue, '-> dataColor:', dataColor); + console.log('effectiveKeyColor:', effectiveKeyColor); + console.log('effectiveBarColor:', effectiveBarColor); + console.log('maxMetricValue:', maxMetricValue); + + // DEBUG: If key value is undefined, show error + if (keyValue === undefined) { + return ( + <RowContainer rowsPerItem={rowsPerItem} style={{ border: '1px solid red', backgroundColor: '#fff0f0' }}> + <div style={{ color: 'red', fontFamily: 'monospace', fontSize: '12px', padding: '8px' }}> + <strong>Error: Key Column "{String(keyColumn)}" not found.</strong> + <br /> + <strong>Available Keys:</strong> {Object.keys(record).join(', ')} + </div> + </RowContainer> Review Comment: **Suggestion:** When the key column is misconfigured or missing, the component currently renders a red debug error row with internal details and inline styles, which is likely unintended in production and results in a broken-looking chart instead of failing gracefully. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Misconfigured key renders red debug row in chart. - ⚠️ Exposes internal column names to end users. - ⚠️ Suggestion changes UX; debug row maybe intentional. ``` </details> ```suggestion if (keyValue === undefined) { return null; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a test harness or Storybook, import `Row` from `superset-frontend/plugins/plugin-chart-unified-list-bar/src/components/Row.tsx`. 2. Render `<Row>` with a `record` object that does not contain the property referenced by `customize.keyColumn` (e.g., `customize.keyColumn = 'nonexistent_col'` and `record = { some_other_col: 'value' }`), and a valid `rowsPerItem` value. 3. During render, at `Row.tsx:118`, `const keyValue = record[effectiveKeyColumn];` evaluates to `undefined` because `effectiveKeyColumn` does not exist on `record`. 4. The conditional block at `Row.tsx:138-148` detects `keyValue === undefined` and returns a `<RowContainer>` styled with a red border and monospace error text listing available keys, resulting in a developer-style error row being rendered into the chart instead of failing silently or via a user-facing error mechanism. ``` </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-unified-list-bar/src/components/Row.tsx **Line:** 138:147 **Comment:** *Logic Error: When the key column is misconfigured or missing, the component currently renders a red debug error row with internal details and inline styles, which is likely unintended in production and results in a broken-looking chart instead of failing gracefully. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=52bb4624de434c6a50b5e3c44bb9e354bb57ff0d5c8ba88983aa6a71cf514778&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37779&comment_hash=52bb4624de434c6a50b5e3c44bb9e354bb57ff0d5c8ba88983aa6a71cf514778&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]
