This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 82e2bc6181 fix(DatasourceModal): replace imperative modal updates with
declarative state (#35256)
82e2bc6181 is described below
commit 82e2bc61816ebe6cf4550f68ebc612a596899ebb
Author: Elizabeth Thompson <[email protected]>
AuthorDate: Fri Sep 26 17:54:17 2025 -0700
fix(DatasourceModal): replace imperative modal updates with declarative
state (#35256)
Co-authored-by: Claude <[email protected]>
---
.../DatasourceModal.useModal.test.tsx | 110 +++++++++++++++++++++
.../Datasource/DatasourceModal/index.tsx | 39 ++------
2 files changed, 118 insertions(+), 31 deletions(-)
diff --git
a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx
b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx
new file mode 100644
index 0000000000..038631ea27
--- /dev/null
+++
b/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();
+});
diff --git
a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
index ecbd8144dd..36bd4f815f 100644
--- a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
+++ b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
@@ -16,13 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import {
- FunctionComponent,
- useState,
- useRef,
- useEffect,
- useCallback,
-} from 'react';
+import { FunctionComponent, useState, 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 [confirmModal, setConfirmModal] = useState<any>(null);
+ const [syncColumns, setSyncColumns] = useState(false);
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;
- if (confirmModal) {
- confirmModal.update({
- content: getSaveDialog(),
- });
- }
+ setSyncColumns(!syncColumns);
}}
/>
<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 (
