Repository navigation
fix(dataset-editor): add missing Data type label in calculated columns tab - #37165
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #52fe9aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| label={t('Data type')} | ||
| control={ | ||
| <Select | ||
| <SelectControl |
There was a problem hiding this comment.
I am not sure I am clear about why Select would not handle this properly. The Field component should care about the label to be displayed and the Select should work ok with this too. Is there a deeper issue going on in the components?
There was a problem hiding this comment.
Looking a little bit deeper and yes, the Fieldcomponent correctly passes the label prop to child. However, the raw Select component uses a header prop for displaying labels, not a label. The SelectControl wrapper handles this by converting the label prop into a ControlHeader and passing it as the header prop that Select expects.
But yes there is a way to keep the Select component here, that is add this header prop. I'll use the old component with the new prop
|
Based on the PR diff, the change replaces the generic |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
| <Select | ||
| ariaLabel={t('Data type')} | ||
| header={<FormLabel>{t('Data type')}</FormLabel>} |
There was a problem hiding this comment.
Suggestion: The added JSX uses the generic Select component which doesn't integrate with the form Field API and likely won't wire the value/onChange into the dataset column item; replace the Select usage with SelectControl and provide the controlId so the Field/Fieldset will correctly bind this input to the type field (this ensures the value is saved and the label is handled by the form control). [possible bug]
Severity Level: Critical 🚨
- ❌ Data type may not persist on calculated columns.
- ⚠️ Dataset editor form binding inconsistent.
- ⚠️ Users may lose edits when saving dataset.| <Select | |
| ariaLabel={t('Data type')} | |
| header={<FormLabel>{t('Data type')}</FormLabel>} | |
| <SelectControl | |
| controlId="type" | |
| ariaLabel={t('Data type')} |
Steps of Reproduction ✅
1. Open
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx
and find the Field for "type" in the calculated-columns expandFieldset. The control JSX is
at hunk lines 317-324 (317 <Select ... 324 />).
2. Note the pattern used elsewhere in this file: controls wired to fields typically use
form-aware controls (e.g., TextControl has controlId set at lines ~296-300 for
"verbose_name"). Those controls integrate with Field/Fieldset binding.
3. Start the web app, go to dataset list, click Edit on a dataset, switch to Calculated
columns tab (the render for this tab mounts ColumnCollectionTable with allowEditDataType
enabled — see the render of the Calculated columns tab where ColumnCollectionTable is
rendered).
4. Add or edit a calculated column and change the Data type using the rendered Select (the
JSX at lines 317-324). Because the code uses the generic Select without
controlId/form-binding semantics, the Field/Fieldset machinery is not receiving input
through the same API other controls use. This can be observed by comparing other Field
controls in the same file that supply controlId (e.g., TextControl at lines ~296-300).
Replacing the JSX with <SelectControl controlId="type" .../> at the same location binds
this input to the Field API, ensuring values are persisted when the dataset is saved.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx
**Line:** 317:319
**Comment:**
*Possible Bug: The added JSX uses the generic `Select` component which doesn't integrate with the form Field API and likely won't wire the value/onChange into the dataset column item; replace the `Select` usage with `SelectControl` and provide the `controlId` so the Field/Fieldset will correctly bind this input to the `type` field (this ensures the value is saved and the label is handled by the form control).
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.|
CodeAnt AI Incremental review completed. |
Code Review Agent Run #92d95dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Almost merged this, but spotted some suggested cahnges. Please slap a change request or hold label on this if you don't want a trigger happy merge to occur ;) |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
You can merge it now, thanks! |
|
CodeAnt AI Incremental review completed. |
Code Review Agent Run #892da7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |

SUMMARY
The "Data type" field in the calculated columns tab of the dataset editor was missing its label. This was caused by using the
Selectcomponent which doesn't render form labels passed via props. Fixed by replacingSelectwithSelectControl, which includesControlHeaderfor proper label rendering - matching the pattern used by other fields in the same form.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:

AFTER:

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION