fix(DatasourceModal): replace imperative modal updates with declarative state (#35256)

Co-authored-by: Claude <noreply@anthropic.com>

superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx

@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+  render,
+  screen,
+  fireEvent,
+  act,
+  defaultStore as store,
+} from 'spec/helpers/testing-library';
+import fetchMock from 'fetch-mock';
+import { Modal } from '@superset-ui/core/components';
+import mockDatasource from 'spec/fixtures/mockDatasource';
+import DatasourceModal from '.';
+
+const mockedProps = {
+  datasource: mockDatasource['7__table'],
+  addSuccessToast: jest.fn(),
+  addDangerToast: jest.fn(),
+  onChange: jest.fn(),
+  onHide: jest.fn(),
+  show: true,
+  onDatasourceSave: jest.fn(),
+};
+
+beforeEach(() => {
+  fetchMock.reset();
+  fetchMock.put('glob:*/api/v1/dataset/7?override_columns=*', {});
+  fetchMock.get('glob:*/api/v1/dataset/7', { result: {} });
+  fetchMock.get('glob:*/api/v1/database/?q=*', { result: [] });
+});
+
+afterEach(() => {
+  fetchMock.reset();
+  jest.clearAllMocks();
+});
+
+test('DatasourceModal - should use Modal.useModal hook instead of Modal.confirm directly', () => {
+  const useModalSpy = jest.spyOn(Modal, 'useModal');
+  const confirmSpy = jest.spyOn(Modal, 'confirm');
+
+  render(<DatasourceModal {...mockedProps} />, { store });
+
+  // Should use the useModal hook
+  expect(useModalSpy).toHaveBeenCalled();
+
+  // Should not call Modal.confirm during initial render
+  expect(confirmSpy).not.toHaveBeenCalled();
+
+  useModalSpy.mockRestore();
+  confirmSpy.mockRestore();
+});
+
+test('DatasourceModal - should handle sync columns state without imperative modal updates', async () => {
+  // Test that we can successfully click the save button without DOM errors
+  // The actual checkbox is only visible when SQL has changed
+  render(<DatasourceModal {...mockedProps} />, { store });
+
+  const saveButton = screen.getByTestId('datasource-modal-save');
+
+  // This should not throw any DOM errors
+  await act(async () => {
+    fireEvent.click(saveButton);
+  });
+
+  // Should show confirmation modal
+  expect(screen.getByText('Confirm save')).toBeInTheDocument();
+
+  // Should show the confirmation message
+  expect(
+    screen.getByText('Are you sure you want to save and apply changes?'),
+  ).toBeInTheDocument();
+});
+
+test('DatasourceModal - should not store modal instance in state', () => {
+  // Mock console.warn to catch any warnings about refs or imperatives
+  const consoleWarn = jest.spyOn(console, 'warn').mockImplementation();
+
+  render(<DatasourceModal {...mockedProps} />, { store });
+
+  // No warnings should be generated about improper React patterns
+  const reactWarnings = consoleWarn.mock.calls.filter(call =>
+    call.some(
+      arg =>
+        typeof arg === 'string' &&
+        (arg.includes('findDOMNode') ||
+          arg.includes('ref') ||
+          arg.includes('instance')),
+    ),
+  );
+
+  expect(reactWarnings).toHaveLength(0);
+
+  consoleWarn.mockRestore();
+});

superset-frontend/src/components/Datasource/DatasourceModal/index.tsx

@@ -16,13 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import {
+import { FunctionComponent, useState, useEffect, useCallback } from 'react';
-  FunctionComponent,
-  useState,
-  useRef,
-  useEffect,
-  useCallback,
-} from 'react';
 import { useSelector } from 'react-redux';
 import {
   styled,
@@ -101,8 +95,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
 }) => {
   const theme = useTheme();
   const [currentDatasource, setCurrentDatasource] = useState(datasource);
-  const syncColumnsRef = useRef(false);
+  const [syncColumns, setSyncColumns] = useState(false);
-  const [confirmModal, setConfirmModal] = useState<any>(null);
   const currencies = useSelector<
     {
       common: {
@@ -114,7 +107,6 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
   const [errors, setErrors] = useState<any[]>([]);
   const [isSaving, setIsSaving] = useState(false);
   const [isEditing, setIsEditing] = useState<boolean>(false);
-  const dialog = useRef<any>(null);
   const [modal, contextHolder] = Modal.useModal();
   const buildPayload = (datasource: Record<string, any>) => {
     const payload: Record<string, any> = {
@@ -196,7 +188,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
     setIsSaving(true);
     try {
       await SupersetClient.put({
-        endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumnsRef.current}`,
+        endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumns}`,
         jsonPayload: buildPayload(currentDatasource),
       });
 
@@ -281,14 +273,9 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
               impact the column definitions, you might want to skip this step.`)}
             />
             <Checkbox
-              checked={syncColumnsRef.current}
+              checked={syncColumns}
               onChange={() => {
-                syncColumnsRef.current = !syncColumnsRef.current;
+                setSyncColumns(!syncColumns);
-                if (confirmModal) {
-                  confirmModal.update({
-                    content: getSaveDialog(),
-                  });
-                }
               }}
             />
             <span
@@ -303,25 +290,17 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
         {t('Are you sure you want to save and apply changes?')}
       </div>
     ),
-    [currentDatasource.sql, datasource.sql, confirmModal],
+    [currentDatasource.sql, datasource.sql, syncColumns],
   );
 
-  useEffect(() => {
-    if (confirmModal) {
-      confirmModal.update({
-        content: getSaveDialog(),
-      });
-    }
-  }, [confirmModal, getSaveDialog]);
-
   useEffect(() => {
     if (datasource.sql !== currentDatasource.sql) {
-      syncColumnsRef.current = true;
+      setSyncColumns(true);
     }
   }, [datasource.sql, currentDatasource.sql]);
 
   const onClickSave = () => {
-    const modalInstance = modal.confirm({
+    modal.confirm({
       title: t('Confirm save'),
       content: getSaveDialog(),
       onOk: onConfirmSave,
@@ -329,8 +308,6 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       okText: t('OK'),
       cancelText: t('Cancel'),
     });
-    setConfirmModal(modalInstance);
-    dialog.current = modalInstance;
   };
 
   return (