feat: Floating Point Formatting for Scatter Point Chart #35915
Conversation
![korbit-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inefficient linear search in visibility function ▹ view | |
 |
Overly permissive numeric type detection ▹ view | |
 |
Duplicated Column Type Checking Logic ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
|
||
| const typeUpper = xAxisType.toUpperCase(); | ||
|
|
||
| return ['FLOAT', 'DOUBLE', 'REAL', 'NUMERIC', 'DECIMAL'].some(t => typeUpper.includes(t)); |
There was a problem hiding this comment.
Overly permissive numeric type detection
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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'); | ||
| }, |
There was a problem hiding this comment.
Duplicated Column Type Checking Logic
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type; | ||
|
|
||
| return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME'); |
There was a problem hiding this comment.
Inefficient linear search in visibility function
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
Interaction Diagram by BitosequenceDiagram
participant Dev as Developer
participant SharedCtrl as sharedControls.tsx<br/>🟩 Added | ●●● High
participant CtrlPanel as controlPanel.tsx<br/>🔄 Updated | ●●● High
participant Types as types.ts<br/>🔄 Updated | ●●○ Medium
participant Constants as constants.ts<br/>🔄 Updated | ●●○ Medium
participant TransformProps as transformProps.ts<br/>🔄 Updated | ●●● High
participant EchartsUI as EchartsTimeseries Component
Dev->>SharedCtrl: Define x_axis_number_format control
SharedCtrl-->>CtrlPanel: Export new control config
CtrlPanel->>CtrlPanel: Add visibility logic for numeric X-axis
CtrlPanel->>Types: Pass xAxisNumberFormat field
Types->>Constants: Initialize with SMART_NUMBER default
Constants->>TransformProps: Provide default format value
TransformProps->>TransformProps: Check if X-axis is Numeric type
TransformProps->>TransformProps: Apply getNumberFormatter(xAxisNumberFormat)
TransformProps-->>EchartsUI: Return formatted echartOptions
Critical path: Developer->sharedControls.tsx->controlPanel.tsx->transformProps.ts->EchartsTimeseries Component
|
![bito-code-review[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #2e97c7
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx - 1
- Incorrect time format visibility for DATE columns · Line 127-127
Review Details
-
Files reviewed - 5 · Commit Range:
0fab5cb..f6a95e7- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
|
|
||
| const xAxisType = xAxisOptions.find(option => option.column_name === xAxisColumn)?.type; | ||
|
|
||
| return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME'); |
There was a problem hiding this comment.
The visibility logic for the x_axis_time_format control only checks if the column type includes 'TIME', which excludes DATE columns. However, DATE is a time-related data type that should also have access to time formatting options. This prevents users from formatting DATE columns properly on the x-axis, impacting chart usability for date-based data. The fix extends the condition to include types containing 'DATE' as well.
Code suggestion
Check the AI-generated fix before applying
| return typeof xAxisType === 'string' && xAxisType.toUpperCase().includes('TIME'); | |
| return typeof xAxisType === 'string' && (xAxisType.toUpperCase().includes('TIME') || xAxisType.toUpperCase().includes('DATE')); |
Code Review Run #2e97c7
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
@alex241728 thanks for creating the PR. Do you mind adding tests to your new feature? |
You are welcome. Also, feel free to tell me the problems in your testing. |
3 participants
SUMMARY
According to the issue #32902, the scatter point charts lack of generic axes, which means the scatter point charts only allow x-axis to be formatted with time formats. So, to fix the problem mentioned in this issue, our team adds a new floating-point formats for x-axis.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The before screenshot can be checked in issue #32902
The after screenshot:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION