korbit-ai[bot] commented on code in PR #33146:
URL: https://github.com/apache/superset/pull/33146#discussion_r2219869898
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/types.ts:
##########
@@ -49,14 +49,20 @@
LegendFormData & {
increaseColor: RgbaColor;
decreaseColor: RgbaColor;
+ orientation: 'vertical' | 'horizontal';
+ showTotal: boolean;
+ boldLabels: string;
Review Comment:
### Unclear property type constraints <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The property name 'boldLabels' and its type (string) don't clearly indicate
what kind of string values are valid or what this setting controls.
###### Why this matters
Without clear type constraints or descriptive naming, developers will need
to look elsewhere to understand what values are valid and what elements this
property affects.
###### Suggested change ∙ *Feature Preview*
```typescript
type BoldLabelType = 'NONE' | 'TOTAL' | 'SUBTOTAL' | 'ALL';
boldLabels: BoldLabelType;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42e673a7-6f53-4496-9a9a-5944dc155c2a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42e673a7-6f53-4496-9a9a-5944dc155c2a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42e673a7-6f53-4496-9a9a-5944dc155c2a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42e673a7-6f53-4496-9a9a-5944dc155c2a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42e673a7-6f53-4496-9a9a-5944dc155c2a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a850bae1-61c7-4554-88af-7e2b1b754d76 -->
[](a850bae1-61c7-4554-88af-7e2b1b754d76)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -34,6 +34,51 @@ const config: ControlPanelConfig = {
expanded: true,
controlSetRows: [
['x_axis'],
+ [
+ {
+ name: 'xAxisSortByColumn',
+ config: {
+ type: 'SelectControl',
+ label: t('X axis sort by column'),
+ description: t(
+ 'Column to use for ordering the waterfall based on a column',
+ ),
+ mapStateToProps: state => ({
+ choices: [
+ ...(state.datasource?.columns || []).map(col => [
+ col.column_name,
+ col.column_name,
+ ]),
+ ],
+ default: '',
+ }),
+ renderTrigger: false,
+ clearable: true,
+ visibility: ({ controls }) => Boolean(controls?.x_axis?.value),
+ },
+ },
+ ],
+ [
+ {
+ name: 'xAxisSortByColumnOrder',
+ config: {
+ type: 'SelectControl',
+ label: t('X axis sort by column order direction'),
+ choices: [
+ ['NONE', t('None')],
+ ['ASC', t('Ascending')],
+ ['DESC', t('Descending')],
+ ],
Review Comment:
### Inconsistent enum value casing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inconsistent string enum values. 'NONE' uses uppercase while 'ASC' and
'DESC' also use uppercase, but 'none' is used elsewhere in the codebase for
similar controls.
###### Why this matters
Inconsistent enum values make the code harder to maintain and can lead to
confusion when comparing values across the codebase.
###### Suggested change ∙ *Feature Preview*
```typescript
choices: [
['none', t('None')],
['asc', t('Ascending')],
['desc', t('Descending')],
],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aaeeef9-743a-4b04-85d9-33c35bcf58f0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aaeeef9-743a-4b04-85d9-33c35bcf58f0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aaeeef9-743a-4b04-85d9-33c35bcf58f0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aaeeef9-743a-4b04-85d9-33c35bcf58f0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aaeeef9-743a-4b04-85d9-33c35bcf58f0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c666dd64-84ac-4ee8-92b6-d44ff311b950 -->
[](c666dd64-84ac-4ee8-92b6-d44ff311b950)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/EchartsWaterfall.tsx:
##########
@@ -23,9 +23,38 @@ import { EventHandlers } from '../types';
export default function EchartsWaterfall(
props: WaterfallChartTransformedProps,
) {
- const { height, width, echartOptions, refs, onLegendStateChanged } = props;
+ const {
+ height,
+ width,
+ echartOptions,
+ setDataMask,
+ onContextMenu,
+ refs,
+ onLegendStateChanged,
+ emitCrossFilters,
+ filterState,
+ handleCrossFilter,
+ } = props;
const eventHandlers: EventHandlers = {
+ click: params => {
+ if (!setDataMask || !emitCrossFilters) return;
+
+ const { name: value } = params;
+ if (value === 'Total') return;
+ const filterOptions = handleCrossFilter(
+ value,
+ filterState?.value === value,
+ );
+ if (!filterOptions) return;
Review Comment:
### Silent failure in cross-filter handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The early return when filterOptions is falsy silently fails without
providing any context about why the filtering operation failed.
###### Why this matters
Silent failures make it difficult to debug issues in production when
cross-filtering operations fail unexpectedly.
###### Suggested change ∙ *Feature Preview*
Add error logging to provide context about the failed operation:
```typescript
if (!filterOptions) {
console.warn('Cross-filtering failed: handleCrossFilter returned no filter
options for value:', value);
return;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6580c4c8-fb92-47c1-b382-3748a3542b1e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6580c4c8-fb92-47c1-b382-3748a3542b1e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6580c4c8-fb92-47c1-b382-3748a3542b1e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6580c4c8-fb92-47c1-b382-3748a3542b1e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6580c4c8-fb92-47c1-b382-3748a3542b1e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bc0f9115-c706-4c43-b296-ecf1cacced56 -->
[](bc0f9115-c706-4c43-b296-ecf1cacced56)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/types.ts:
##########
@@ -68,5 +74,14 @@ export interface EchartsWaterfallChartProps extends
ChartProps {
queriesData: ChartDataResponseResult[];
}
-export type WaterfallChartTransformedProps =
- BaseTransformedProps<EchartsWaterfallFormData>;
+export interface WaterfallChartTransformedProps
+ extends BaseTransformedProps<EchartsWaterfallFormData> {
+ emitCrossFilters?: boolean;
+ filterState?: {
+ value?: string;
+ };
+ handleCrossFilter: (
+ value: string,
+ isCurrentValue: boolean,
+ ) => { extraFormData: any; filterState: any } | null;
Review Comment:
### Undocumented cross-filter handler with any types <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `handleCrossFilter` function uses `any` types and lacks documentation
explaining its purpose and return value meaning.
###### Why this matters
The use of `any` without documentation makes it difficult to understand what
data structure is expected in extraFormData and filterState.
###### Suggested change ∙ *Feature Preview*
/** Handles cross-filtering interactions by generating filter data
* @param value - The value to filter by
* @param isCurrentValue - Whether the value is currently selected
* @returns Filter configuration object or null if filtering is not
applicable
*/
handleCrossFilter: (
value: string,
isCurrentValue: boolean,
) => { extraFormData: any; filterState: any } | null;
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d31044e-d85d-4cce-9d08-674db06721d6/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d31044e-d85d-4cce-9d08-674db06721d6?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d31044e-d85d-4cce-9d08-674db06721d6?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d31044e-d85d-4cce-9d08-674db06721d6?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d31044e-d85d-4cce-9d08-674db06721d6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c4e86f44-6757-49f9-b58b-41d49b66661d -->
[](c4e86f44-6757-49f9-b58b-41d49b66661d)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:
##########
@@ -156,11 +156,45 @@
legendState,
queriesData,
hooks,
+ filterState,
theme,
+ emitCrossFilters,
inContextMenu,
} = chartProps;
const refs: Refs = {};
- const { data = [] } = queriesData[0];
+ let { data = [] } = queriesData[0];
+
+ const { xAxisSortByColumn, xAxisSortByColumnOrder } = formData;
+ if (
+ xAxisSortByColumn &&
+ xAxisSortByColumnOrder &&
+ xAxisSortByColumnOrder !== 'NONE'
+ ) {
+ const sortColumn = xAxisSortByColumn;
+ const isAscending = xAxisSortByColumnOrder === 'ASC';
+
+ data = [...data].sort((a, b) => {
Review Comment:
### Inefficient array sorting with spread operator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new array with spread operator and then sorting it creates
unnecessary memory allocation for large datasets.
###### Why this matters
For large datasets, this operation will consume twice the memory temporarily
and cause extra GC pressure.
###### Suggested change ∙ *Feature Preview*
Sort the array in place using Array.prototype.sort directly if possible:
```typescript
data.sort((a, b) => {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fa49d2d-4359-427e-a10c-d3e8d92c1114/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fa49d2d-4359-427e-a10c-d3e8d92c1114?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fa49d2d-4359-427e-a10c-d3e8d92c1114?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fa49d2d-4359-427e-a10c-d3e8d92c1114?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fa49d2d-4359-427e-a10c-d3e8d92c1114)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0c13d424-d285-4072-b249-7329e2f818ca -->
[](0c13d424-d285-4072-b249-7329e2f818ca)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:
##########
@@ -452,14 +490,301 @@
series: barSeries,
};
+ const processSeriesData = (options: EChartsOption) => {
+ let processedOptions = { ...options };
+
+ // Handle subtotal styling
+ if (useFirstValueAsSubtotal) {
+ const processedSeries = ((options.series as any[]) || []).map(series => {
+ const newData = series.data.map((dataPoint: any, index: number) => {
+ if (index !== 0) return dataPoint;
+ if (dataPoint?.itemStyle?.color === 'transparent') return dataPoint;
+ if (dataPoint.value === '-') return dataPoint;
+
+ const updatedColor = `rgba(${totalColor.r}, ${totalColor.g},
${totalColor.b})`;
+ return {
+ ...dataPoint,
+ itemStyle: {
+ ...dataPoint.itemStyle,
+ color: updatedColor,
+ borderColor: updatedColor,
+ },
+ };
+ });
+
+ return {
+ ...series,
+ data: newData,
+ };
+ });
+
+ processedOptions = {
+ ...processedOptions,
+ series: processedSeries,
+ };
+ }
+
+ // Handle total visibility
+ if (!showTotal) {
+ const totalsIndex =
+ ((processedOptions.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || [];
+
+ const filteredData = [
+ ...((processedOptions.xAxis as { data: (string | number)[] }).data ||
+ []),
+ ].filter((_, index) => !totalsIndex.includes(index));
+
+ const filteredSeries = ((processedOptions.series as any[]) || []).map(
+ series => ({
+ ...series,
+ data: series.data.filter(
+ (_: any, index: number) => !totalsIndex.includes(index),
+ ),
+ }),
+ );
+
+ processedOptions = {
+ ...processedOptions,
+ xAxis: {
+ ...(processedOptions.xAxis as any),
+ data: filteredData,
+ },
+ series: filteredSeries,
+ };
+ }
+
+ // Handle orientation
+ if (orientation === 'horizontal') {
+ processedOptions = {
+ ...processedOptions,
+ xAxis: {
+ ...((processedOptions.yAxis as any) || {}),
+ type: 'value',
+ },
+ yAxis: {
+ ...((processedOptions.xAxis as any) || {}),
+ type: 'category',
+ data: [...(processedOptions.xAxis as any).data].reverse(),
+ },
+ series: Array.isArray(processedOptions.series)
+ ? processedOptions.series.map((series: any) => ({
+ ...series,
+ encode: {
+ x: series.encode?.y,
+ y: series.encode?.x,
+ },
+ data: [...series.data].reverse(),
+ label: {
+ ...(series.label || {}),
+ position: series.name === 'Decrease' ? 'left' : 'right',
+ },
+ }))
+ : [],
+ };
+ }
+
+ return processedOptions;
+ };
+
+ // adds more formatting to previous axisLabel.formatter
+ const getFormattedAxisOptions = (options: EChartsOption) => {
+ // If no formatting needed, return original options
+ if (boldLabels === 'none' && xTicksLayout !== 'flat') {
+ return options;
+ }
+
+ // Get total indices for bold formatting
+ const totalsIndex = ['total', 'both'].includes(boldLabels)
+ ? ((options.series as any[]) || [])
+ .find(series => series.name === 'Total')
+ ?.data.map((dataPoint: any, index: number) =>
+ dataPoint.value !== '-' ? index : -1,
+ )
+ .filter((index: number) => index !== -1) || []
+ : [];
Review Comment:
### Inefficient chained array operations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Chaining multiple array operations (find, map, filter) creates unnecessary
intermediate arrays and performs multiple iterations.
###### Why this matters
Each chained operation creates a new array and requires a full iteration,
impacting performance with large datasets.
###### Suggested change ∙ *Feature Preview*
Use a single reduce or forEach to collect indices:
```typescript
const totalsIndex = ['total', 'both'].includes(boldLabels)
? options.series?.reduce((indices, series) => {
if (series.name === 'Total') {
series.data.forEach((point, idx) => {
if (point.value !== '-') indices.push(idx);
});
}
return indices;
}, []) || []
: [];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e02acd0-1c53-482e-8805-879dfa631b63/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e02acd0-1c53-482e-8805-879dfa631b63?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e02acd0-1c53-482e-8805-879dfa631b63?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e02acd0-1c53-482e-8805-879dfa631b63?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e02acd0-1c53-482e-8805-879dfa631b63)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:21a12875-1204-4c51-ab13-c13cc420d45d -->
[](21a12875-1204-4c51-ab13-c13cc420d45d)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:
##########
@@ -156,11 +156,45 @@ export default function transformProps(
legendState,
queriesData,
hooks,
+ filterState,
theme,
+ emitCrossFilters,
inContextMenu,
} = chartProps;
const refs: Refs = {};
- const { data = [] } = queriesData[0];
+ let { data = [] } = queriesData[0];
+
+ const { xAxisSortByColumn, xAxisSortByColumnOrder } = formData;
+ if (
+ xAxisSortByColumn &&
+ xAxisSortByColumnOrder &&
+ xAxisSortByColumnOrder !== 'NONE'
+ ) {
+ const sortColumn = xAxisSortByColumn;
+ const isAscending = xAxisSortByColumnOrder === 'ASC';
+
+ data = [...data].sort((a, b) => {
+ const aValue = a[sortColumn];
+ const bValue = b[sortColumn];
+
+ // Handle null/undefined values
+ if (aValue == null && bValue == null) return 0;
+ if (aValue == null) return isAscending ? -1 : 1;
+ if (bValue == null) return isAscending ? 1 : -1;
+
+ // Handle numeric values
+ if (typeof aValue === 'number' && typeof bValue === 'number') {
+ return isAscending ? aValue - bValue : bValue - aValue;
+ }
+
+ // Handle string/other values
+ const aStr = String(aValue);
+ const bStr = String(bValue);
+ const comparison = aStr.localeCompare(bStr);
Review Comment:
### Inline complex sorting logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Complex sorting logic is embedded directly in the transformProps function,
making it difficult to understand and reuse.
###### Why this matters
Inline sorting logic reduces code clarity and makes it harder to maintain
consistent sorting behavior across the application. This logic could be reused
elsewhere.
###### Suggested change ∙ *Feature Preview*
Extract into a separate utility function:
```typescript
const sortByColumn = <T extends Record<string, any>>(
data: T[],
column: keyof T,
order: 'ASC' | 'DESC' | 'NONE'
): T[] => {
if (order === 'NONE') return data;
const isAscending = order === 'ASC';
return [...data].sort((a, b) => {
const aValue = a[column];
const bValue = b[column];
// ... rest of sorting logic ...
});
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d9727df-1c30-407d-bfe6-549fd84c616c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d9727df-1c30-407d-bfe6-549fd84c616c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d9727df-1c30-407d-bfe6-549fd84c616c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d9727df-1c30-407d-bfe6-549fd84c616c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d9727df-1c30-407d-bfe6-549fd84c616c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c71c4708-cef8-4777-a280-25ee929f0e12 -->
[](c71c4708-cef8-4777-a280-25ee929f0e12)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
