korbit-ai[bot] commented on code in PR #35915:
URL: https://github.com/apache/superset/pull/35915#discussion_r2478949749
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}.
${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option =>
option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' &&
xAxisType.toUpperCase().includes('TIME');
Review Comment:
### Inefficient linear search in visibility function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The visibility function performs a linear search through xAxisOptions array
on every render to find the matching column type.
###### Why this matters
This creates O(n) time complexity that executes repeatedly during UI
interactions, potentially causing performance degradation with large datasets
or frequent re-renders.
###### Suggested change ∙ *Feature Preview*
Cache the result or use a Map/object lookup for O(1) access. Consider
memoizing the visibility function result:
```typescript
const xAxisTypeMap = useMemo(() =>
new Map(xAxisOptions?.map(option => [option.column_name, option.type]) ||
[]),
[xAxisOptions]
);
const xAxisType = xAxisTypeMap.get(xAxisColumn);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/07378c51-9930-426d-9d6b-6edd60f2f393)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f2b38014-c44f-4595-b9ae-e6086b6c3c03 -->
[](f2b38014-c44f-4595-b9ae-e6086b6c3c03)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}.
${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option =>
option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' &&
xAxisType.toUpperCase().includes('TIME');
+ },
},
},
+ {
+ name: 'x_axis_number_format',
+ config: {
+ ...sharedControls.x_axis_number_format,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a floating-point column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option =>
option.column_name === xAxisColumn)?.type;
+
+ if (typeof xAxisType !== 'string')
+ {
+ return false;
+ }
+
+ const typeUpper = xAxisType.toUpperCase();
+
+ return ['FLOAT', 'DOUBLE', 'REAL', 'NUMERIC',
'DECIMAL'].some(t => typeUpper.includes(t));
Review Comment:
### Overly permissive numeric type detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type checking logic uses string inclusion which can produce false
positives for database types that contain these keywords as substrings.
###### Why this matters
This could incorrectly show the number format control for non-numeric types
like 'FLOAT_TEXT' or 'DOUBLE_PRECISION_STRING', leading to unexpected UI
behavior and potential formatting errors.
###### Suggested change ∙ *Feature Preview*
Use exact type matching or more precise pattern matching:
```typescript
const numericTypes = ['FLOAT', 'DOUBLE', 'REAL', 'NUMERIC', 'DECIMAL'];
return numericTypes.some(t =>
typeUpper === t ||
typeUpper.startsWith(t + '(') ||
typeUpper === t + '64' ||
typeUpper === t + '32'
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6ff7936-ed89-4a81-abd5-cd384717b2a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9960cfc4-bd40-447c-9a23-e996b521cc17 -->
[](9960cfc4-bd40-447c-9a23-e996b521cc17)
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -112,8 +112,49 @@ const config: ControlPanelConfig = {
...sharedControls.x_axis_time_format,
default: 'smart_date',
description: `${D3_TIME_FORMAT_DOCS}.
${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) => {
+ // check if x axis is a time column
+ const xAxisColumn = controls?.x_axis?.value;
+ const xAxisOptions = controls?.x_axis?.options;
+
+ if (!xAxisColumn || !Array.isArray(xAxisOptions))
+ {
+ return false;
+ }
+
+ const xAxisType = xAxisOptions.find(option =>
option.column_name === xAxisColumn)?.type;
+
+ return typeof xAxisType === 'string' &&
xAxisType.toUpperCase().includes('TIME');
+ },
Review Comment:
### Duplicated Column Type Checking Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The visibility logic for x_axis_time_format and x_axis_number_format
controls is duplicated with only minor differences in the type checking.
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies
when changes are needed. If the column type checking logic needs to change, it
would need to be updated in multiple places.
###### Suggested change ∙ *Feature Preview*
Extract the common column type checking logic into a reusable function:
```typescript
const getColumnType = (controls: any) => {
const xAxisColumn = controls?.x_axis?.value;
const xAxisOptions = controls?.x_axis?.options;
if (!xAxisColumn || !Array.isArray(xAxisOptions)) {
return null;
}
return xAxisOptions.find(option => option.column_name ===
xAxisColumn)?.type;
};
// Then use it in visibility checks:
visibility: ({ controls }) => {
const xAxisType = getColumnType(controls);
return typeof xAxisType === 'string' &&
xAxisType.toUpperCase().includes('TIME');
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c7d25f4-6f47-4721-80f5-6343526de003)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f18789c9-5753-4a15-88a1-26a3b34d5641 -->
[](f18789c9-5753-4a15-88a1-26a3b34d5641)
--
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]
