Copilot commented on code in PR #34509:
URL: https://github.com/apache/superset/pull/34509#discussion_r2248997853
##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -228,34 +236,82 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
setErrors(err);
};
- const renderSaveDialog = () => (
- <div>
- <Alert
- css={theme => ({
- marginTop: theme.gridUnit * 4,
- marginBottom: theme.gridUnit * 4,
- })}
- type="warning"
- showIcon
- message={t(`The dataset configuration exposed here
+ const getSaveDialog = useCallback(
+ () => (
+ <div>
+ <Alert
+ css={theme => ({
+ marginTop: theme.gridUnit * 4,
+ marginBottom: theme.gridUnit * 4,
+ })}
+ type="warning"
+ showIcon={false}
+ message={t(`The dataset configuration exposed here
affects all the charts using this dataset.
Be mindful that changing settings
here may affect other charts
in undesirable ways.`)}
- />
- {t('Are you sure you want to save and apply changes?')}
- </div>
+ />
+ {datasource.sql !== currentDatasource.sql && (
+ <>
+ <Alert
+ css={theme => ({
+ marginTop: theme.gridUnit * 4,
+ marginBottom: theme.gridUnit * 4,
+ })}
+ type="info"
+ showIcon={false}
+ message={t(`The dataset columns will be automatically synced
+ based on the changes in your SQL query. If your changes don't
+ impact the column definitions, you might want to skip this
step.`)}
+ />
+ <Checkbox
+ checked={syncColumnsRef.current}
+ onChange={() => {
+ syncColumnsRef.current = !syncColumnsRef.current;
+ if (confirmModal) {
+ confirmModal.update({
+ content: getSaveDialog(),
+ });
+ }
+ }}
+ />
+ <span className="m-l-5">{t('Automatically sync columns')}</span>
+ <br />
+ <br />
+ </>
+ )}
+ {t('Are you sure you want to save and apply changes?')}
+ </div>
+ ),
+ [currentDatasource.sql, datasource.sql, confirmModal],
Review Comment:
[nitpick] Including confirmModal in the dependency array may cause
unnecessary re-renders of getSaveDialog. Since confirmModal is only used for
the update call, consider removing it from dependencies and handling the update
separately.
```suggestion
[currentDatasource.sql, datasource.sql],
```
##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -228,34 +236,82 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
setErrors(err);
};
- const renderSaveDialog = () => (
- <div>
- <Alert
- css={theme => ({
- marginTop: theme.gridUnit * 4,
- marginBottom: theme.gridUnit * 4,
- })}
- type="warning"
- showIcon
- message={t(`The dataset configuration exposed here
+ const getSaveDialog = useCallback(
+ () => (
+ <div>
+ <Alert
+ css={theme => ({
+ marginTop: theme.gridUnit * 4,
+ marginBottom: theme.gridUnit * 4,
+ })}
+ type="warning"
+ showIcon={false}
+ message={t(`The dataset configuration exposed here
affects all the charts using this dataset.
Be mindful that changing settings
here may affect other charts
in undesirable ways.`)}
- />
- {t('Are you sure you want to save and apply changes?')}
- </div>
+ />
+ {datasource.sql !== currentDatasource.sql && (
+ <>
+ <Alert
+ css={theme => ({
+ marginTop: theme.gridUnit * 4,
+ marginBottom: theme.gridUnit * 4,
+ })}
+ type="info"
+ showIcon={false}
+ message={t(`The dataset columns will be automatically synced
+ based on the changes in your SQL query. If your changes don't
+ impact the column definitions, you might want to skip this
step.`)}
+ />
+ <Checkbox
+ checked={syncColumnsRef.current}
+ onChange={() => {
+ syncColumnsRef.current = !syncColumnsRef.current;
+ if (confirmModal) {
+ confirmModal.update({
+ content: getSaveDialog(),
+ });
+ }
+ }}
Review Comment:
[nitpick] The onChange handler directly mutates syncColumnsRef.current and
accesses confirmModal state, creating complex side effects. Consider extracting
this logic into a separate function for better readability and testability.
```suggestion
onChange={handleSyncColumnsChange}
```
##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -92,6 +99,8 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
show,
}) => {
const [currentDatasource, setCurrentDatasource] = useState(datasource);
+ const syncColumnsRef = useRef(false);
+ const [confirmModal, setConfirmModal] = useState<any>(null);
Review Comment:
Using 'any' type reduces type safety. Consider defining a proper interface
or using a more specific type for the modal instance.
```suggestion
const [confirmModal, setConfirmModal] = useState<ModalFuncProps |
null>(null);
```
--
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]
