Re: [PR] fix: Users can't skip column sync when saving virtual datasets [superset]

Fri, 01 Aug 2025 13:29:07 -0700 


korbit-ai[bot] commented on code in PR #34509:
URL: https://github.com/apache/superset/pull/34509#discussion_r2248816437


##########
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
+              given the changes in your SQL query. If your changes don't
+              impact columns, 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],
   );
 
+  useEffect(() => {
+    if (confirmModal) {
+      confirmModal.update({
+        content: getSaveDialog(),
+      });
+    }
+  }, [confirmModal, getSaveDialog]);
+
+  useEffect(() => {
+    if (datasource.sql !== currentDatasource.sql) {
+      syncColumnsRef.current = true;
+    }

Review Comment:
   ### Sync Columns Checkbox State Not Reset <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The sync columns checkbox is automatically checked when SQL changes, but it 
doesn't get unchecked when SQL reverts to its original state.
   
   
   ###### Why this matters
   Users may be confused when the checkbox remains checked even after reverting 
their SQL changes, potentially leading to unintended column synchronization.
   
   ###### Suggested change ∙ *Feature Preview*
   Update the useEffect to handle SQL reversion:
   ```typescript
   useEffect(() => {
     syncColumnsRef.current = datasource.sql !== currentDatasource.sql;
   }, [datasource.sql, currentDatasource.sql]);
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17a10732-fadf-46cb-92ff-c1a6a6564fd6/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17a10732-fadf-46cb-92ff-c1a6a6564fd6?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17a10732-fadf-46cb-92ff-c1a6a6564fd6?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17a10732-fadf-46cb-92ff-c1a6a6564fd6?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/17a10732-fadf-46cb-92ff-c1a6a6564fd6)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:24eb6fe7-a336-4371-a72d-67cbc4a22535 -->
   
   
   [](24eb6fe7-a336-4371-a72d-67cbc4a22535)



-- 
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]

Reply via email to