korbit-ai[bot] commented on code in PR #33559:
URL: https://github.com/apache/superset/pull/33559#discussion_r2102271384
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx:
##########
@@ -210,6 +224,8 @@ const config: ControlPanelConfig = {
columnsPropsObject: { colnames, coltypes },
};
},
+ visibility: ({ controls }) =>
+ Boolean(!controls?.is_normalised?.value),
Review Comment:
### Unnecessary Boolean constructor <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Boolean constructor is unnecessarily used with a logical operation that
already returns a boolean.
###### Why this matters
Adding complexity by wrapping a boolean expression in Boolean() makes the
code less readable without providing any benefit.
###### Suggested change ∙ *Feature Preview*
```typescript
visibility: ({ controls }) => !controls?.is_normalised?.value,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22943c81-dea0-4d1e-923b-cf80274b9ab0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22943c81-dea0-4d1e-923b-cf80274b9ab0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22943c81-dea0-4d1e-923b-cf80274b9ab0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22943c81-dea0-4d1e-923b-cf80274b9ab0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/22943c81-dea0-4d1e-923b-cf80274b9ab0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f7520d7f-69c5-401c-bd28-e8adc30af6d9 -->
[](f7520d7f-69c5-401c-bd28-e8adc30af6d9)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -259,6 +330,33 @@
},
];
+ const NormalisedTooltipFormaterr = function (params: any) {
Review Comment:
### Unsafe Tooltip Formatter Typing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tooltip formatter function uses 'any' type for params and has a typo in
its name (Formaterr), making it less type-safe and harder to maintain.
###### Why this matters
Using 'any' type bypasses TypeScript's type checking, which could lead to
runtime errors if the chart library changes its parameter structure.
###### Suggested change ∙ *Feature Preview*
Use proper typing for the tooltip formatter:
```typescript
const NormalizedTooltipFormatter = function (params: CallbackDataParams & {
color: string, name: string, value: number[] }) {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb263290-f136-458d-ba1b-125a70c90a9c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb263290-f136-458d-ba1b-125a70c90a9c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb263290-f136-458d-ba1b-125a70c90a9c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb263290-f136-458d-ba1b-125a70c90a9c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb263290-f136-458d-ba1b-125a70c90a9c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1927e622-0d88-4711-80a5-5bfcec447d11 -->
[](1927e622-0d88-4711-80a5-5bfcec447d11)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -51,13 +52,24 @@ export function formatLabel({
params,
labelType,
numberFormatter,
+ getDenormalisedSeriesValue,
+ isNormalised,
}: {
params: CallbackDataParams;
labelType: EchartsRadarLabelType;
numberFormatter: NumberFormatter;
+ getDenormalisedSeriesValue: (
+ seriesName: string,
+ normalisedValue: string,
+ ) => number;
+ isNormalised: boolean;
}): string {
const { name = '', value } = params;
- const formattedValue = numberFormatter(value as number);
+ const formattedValue = numberFormatter(
+ isNormalised
+ ? (getDenormalisedSeriesValue(name, String(value)) as number)
+ : (value as number),
+ );
Review Comment:
### Complex Value Formatting Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Complex nested ternary with multiple type assertions makes the value
formatting logic hard to read.
###### Why this matters
Dense code with multiple operations (type conversion, ternary, function
calls) increases cognitive load when trying to understand the logic flow.
###### Suggested change ∙ *Feature Preview*
```typescript
const { name = '', value } = params;
let valueToFormat = value as number;
if (isNormalised) {
valueToFormat = getDenormalisedSeriesValue(name, String(value));
}
const formattedValue = numberFormatter(valueToFormat);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c522aac-04ef-474b-8aac-b70fb7e461a0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c522aac-04ef-474b-8aac-b70fb7e461a0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c522aac-04ef-474b-8aac-b70fb7e461a0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c522aac-04ef-474b-8aac-b70fb7e461a0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1c522aac-04ef-474b-8aac-b70fb7e461a0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:53b2e6a1-898e-4626-a9f1-8a142d760442 -->
[](53b2e6a1-898e-4626-a9f1-8a142d760442)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -259,6 +330,33 @@
},
];
+ const NormalisedTooltipFormaterr = function (params: any) {
+ const { color } = params;
+ const seriesName = params.name || '';
+ const values = params.value;
+
+ const colorDot = `<span
style="display:inline-block;margin-right:5px;border-radius:50%;width:5px;height:5px;background-color:${color}"></span>`;
+
+ // Start with series name without dot
+ let tooltip = `<div
style="font-weight:bold;margin-bottom:5px;">${seriesName || 'series0'}</div>`;
Review Comment:
### Inconsistent Series Name Fallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tooltip formatter uses a hardcoded fallback 'series0' when seriesName is
empty, which doesn't align with the actual series naming convention.
###### Why this matters
Using an arbitrary fallback name could confuse users when they see
inconsistent series naming in tooltips.
###### Suggested change ∙ *Feature Preview*
Remove the hardcoded fallback and use an empty string or a more meaningful
default:
```typescript
let tooltip = `<div style="font-weight:bold;margin-bottom:5px;">${seriesName
|| ''}</div>`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7eaa5a89-1fb8-4e6e-a2e9-1afff94835f8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7eaa5a89-1fb8-4e6e-a2e9-1afff94835f8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7eaa5a89-1fb8-4e6e-a2e9-1afff94835f8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7eaa5a89-1fb8-4e6e-a2e9-1afff94835f8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7eaa5a89-1fb8-4e6e-a2e9-1afff94835f8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:49667fbc-2762-4e53-804d-10198461b16f -->
[](49667fbc-2762-4e53-804d-10198461b16f)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -111,11 +124,36 @@
const { setDataMask = () => {}, onContextMenu } = hooks;
const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
const numberFormatter = getNumberFormatter(numberFormat);
+ const denormalizedSeriesValues: SeriesNormalizedMap = {};
+
+ const getDenormalisedSeriesValue = (
+ seriesName: string,
+ normalisedValue: string,
+ ): number => {
+ if (
+ Object.prototype.hasOwnProperty.call(
+ denormalizedSeriesValues,
+ seriesName,
+ ) &&
+ Object.prototype.hasOwnProperty.call(
+ denormalizedSeriesValues[seriesName],
+ normalisedValue,
+ )
+ ) {
Review Comment:
### Inefficient Value Lookup Pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple hasOwnProperty checks and nested object lookups are performed for
every value lookup in getDenormalisedSeriesValue.
###### Why this matters
Repeated property existence checks and object traversals can impact
performance, especially when this function is called frequently during
rendering and interactions.
###### Suggested change ∙ *Feature Preview*
Use optional chaining for more efficient property access:
```typescript
const getDenormalisedSeriesValue = (seriesName: string, normalisedValue:
string): number => {
const value = denormalizedSeriesValues[seriesName]?.[normalisedValue];
return value ?? Number(normalisedValue);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3098d1bb-db07-4c1e-ae32-bb4ab3da6ca7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3098d1bb-db07-4c1e-ae32-bb4ab3da6ca7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3098d1bb-db07-4c1e-ae32-bb4ab3da6ca7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3098d1bb-db07-4c1e-ae32-bb4ab3da6ca7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3098d1bb-db07-4c1e-ae32-bb4ab3da6ca7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cd49a234-38f2-42f5-9d95-280343cf7192 -->
[](cd49a234-38f2-42f5-9d95-280343cf7192)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -212,7 +250,40 @@
{},
);
+ // Add normalization function
+ const normalizeArray = (arr: number[], decimals = 10, seriesName: string) =>
{
+ const max = Math.max(...arr);
+ return arr.map(value => {
+ const normalisedValue = Number((value / max).toFixed(decimals));
+ denormalizedSeriesValues[seriesName][String(normalisedValue)] = value;
+ return normalisedValue;
+ });
+ };
Review Comment:
### Normalization Zero Division Edge Case <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The normalization function fails to handle the edge case where all values in
the array are 0, which would result in division by zero.
###### Why this matters
When all values are 0, max will be 0, leading to NaN values in the
normalized data and potential chart rendering issues.
###### Suggested change ∙ *Feature Preview*
Add a check for zero max value in the normalization function:
```typescript
const normalizeArray = (arr: number[], decimals = 10, seriesName: string) =>
{
const max = Math.max(...arr);
if (max === 0) {
return arr.map(() => 0);
}
return arr.map(value => {
const normalisedValue = Number((value / max).toFixed(decimals));
denormalizedSeriesValues[seriesName][String(normalisedValue)] = value;
return normalisedValue;
});
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c6d892-5b9e-4297-84fe-a9e5f2439c87/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c6d892-5b9e-4297-84fe-a9e5f2439c87?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c6d892-5b9e-4297-84fe-a9e5f2439c87?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c6d892-5b9e-4297-84fe-a9e5f2439c87?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31c6d892-5b9e-4297-84fe-a9e5f2439c87)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fe2a7902-731e-447e-8511-0f994dd3e830 -->
[](fe2a7902-731e-447e-8511-0f994dd3e830)
--
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]
