Skip to content

Commit ebbf139

Browse files
eschuthoclaude
andcommitted
fix(modals): use Modal.useModal hook for proper dark mode theming
Modal.confirm() bypasses the theme provider context by rendering directly to document.body. This change uses the useModal hook which renders modals within the component tree, ensuring they inherit theme context properly. Changes: - Replace Modal.confirm() with modal.confirm() from useModal hook - Add contextHolder to JSX to provide rendering container - Applied to ThemeList and EmbeddedModal components This fixes dark mode theming issues where confirmation modals would appear with default Ant Design styling instead of themed styling. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 36daa2d commit ebbf139

File tree

6 files changed

+272
-33
lines changed

6 files changed

+272
-33
lines changed
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
import {
20+
render,
21+
screen,
22+
fireEvent,
23+
act,
24+
defaultStore as store,
25+
} from 'spec/helpers/testing-library';
26+
import fetchMock from 'fetch-mock';
27+
import { Modal } from '@superset-ui/core/components';
28+
import mockDatasource from 'spec/fixtures/mockDatasource';
29+
import DatasourceModal from '.';
30+
31+
const mockedProps = {
32+
datasource: mockDatasource['7__table'],
33+
addSuccessToast: jest.fn(),
34+
addDangerToast: jest.fn(),
35+
onChange: jest.fn(),
36+
onHide: jest.fn(),
37+
show: true,
38+
onDatasourceSave: jest.fn(),
39+
};
40+
41+
describe('DatasourceModal - useModal Hook Tests', () => {
42+
beforeEach(() => {
43+
fetchMock.reset();
44+
fetchMock.put('glob:*/api/v1/dataset/7?override_columns=*', {});
45+
fetchMock.get('glob:*/api/v1/dataset/7', { result: {} });
46+
fetchMock.get('glob:*/api/v1/database/?q=*', { result: [] });
47+
});
48+
49+
afterEach(() => {
50+
fetchMock.reset();
51+
jest.clearAllMocks();
52+
});
53+
54+
test('should use Modal.useModal hook instead of Modal.confirm directly', () => {
55+
const useModalSpy = jest.spyOn(Modal, 'useModal');
56+
const confirmSpy = jest.spyOn(Modal, 'confirm');
57+
58+
render(<DatasourceModal {...mockedProps} />, { store });
59+
60+
// Should use the useModal hook
61+
expect(useModalSpy).toHaveBeenCalled();
62+
63+
// Should not call Modal.confirm during initial render
64+
expect(confirmSpy).not.toHaveBeenCalled();
65+
66+
useModalSpy.mockRestore();
67+
confirmSpy.mockRestore();
68+
});
69+
70+
test('should handle sync columns state without imperative modal updates', async () => {
71+
// Test that we can successfully click the save button without DOM errors
72+
// The actual checkbox is only visible when SQL has changed
73+
render(<DatasourceModal {...mockedProps} />, { store });
74+
75+
const saveButton = screen.getByTestId('datasource-modal-save');
76+
77+
// This should not throw any DOM errors
78+
await act(async () => {
79+
fireEvent.click(saveButton);
80+
});
81+
82+
// Should show confirmation modal
83+
expect(screen.getByText('Confirm save')).toBeInTheDocument();
84+
85+
// Should show the confirmation message
86+
expect(
87+
screen.getByText('Are you sure you want to save and apply changes?'),
88+
).toBeInTheDocument();
89+
});
90+
91+
test('should not store modal instance in state', () => {
92+
// Mock console.warn to catch any warnings about refs or imperatives
93+
const consoleWarn = jest.spyOn(console, 'warn').mockImplementation();
94+
95+
render(<DatasourceModal {...mockedProps} />, { store });
96+
97+
// No warnings should be generated about improper React patterns
98+
const reactWarnings = consoleWarn.mock.calls.filter(call =>
99+
call.some(
100+
arg =>
101+
typeof arg === 'string' &&
102+
(arg.includes('findDOMNode') ||
103+
arg.includes('ref') ||
104+
arg.includes('instance')),
105+
),
106+
);
107+
108+
expect(reactWarnings).toHaveLength(0);
109+
110+
consoleWarn.mockRestore();
111+
});
112+
});

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

Lines changed: 8 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,7 @@
1616
* specific language governing permissions and limitations
1717
* under the License.
1818
*/
19-
import {
20-
FunctionComponent,
21-
useState,
22-
useRef,
23-
useEffect,
24-
useCallback,
25-
} from 'react';
19+
import { FunctionComponent, useState, useEffect, useCallback } from 'react';
2620
import { useSelector } from 'react-redux';
2721
import {
2822
styled,
@@ -101,8 +95,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
10195
}) => {
10296
const theme = useTheme();
10397
const [currentDatasource, setCurrentDatasource] = useState(datasource);
104-
const syncColumnsRef = useRef(false);
105-
const [confirmModal, setConfirmModal] = useState<any>(null);
98+
const [syncColumns, setSyncColumns] = useState(false);
10699
const currencies = useSelector<
107100
{
108101
common: {
@@ -114,7 +107,6 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
114107
const [errors, setErrors] = useState<any[]>([]);
115108
const [isSaving, setIsSaving] = useState(false);
116109
const [isEditing, setIsEditing] = useState<boolean>(false);
117-
const dialog = useRef<any>(null);
118110
const [modal, contextHolder] = Modal.useModal();
119111
const buildPayload = (datasource: Record<string, any>) => {
120112
const payload: Record<string, any> = {
@@ -196,7 +188,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
196188
setIsSaving(true);
197189
try {
198190
await SupersetClient.put({
199-
endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumnsRef.current}`,
191+
endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumns}`,
200192
jsonPayload: buildPayload(currentDatasource),
201193
});
202194

@@ -281,14 +273,9 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
281273
impact the column definitions, you might want to skip this step.`)}
282274
/>
283275
<Checkbox
284-
checked={syncColumnsRef.current}
276+
checked={syncColumns}
285277
onChange={() => {
286-
syncColumnsRef.current = !syncColumnsRef.current;
287-
if (confirmModal) {
288-
confirmModal.update({
289-
content: getSaveDialog(),
290-
});
291-
}
278+
setSyncColumns(!syncColumns);
292279
}}
293280
/>
294281
<span
@@ -303,34 +290,24 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
303290
{t('Are you sure you want to save and apply changes?')}
304291
</div>
305292
),
306-
[currentDatasource.sql, datasource.sql, confirmModal],
293+
[currentDatasource.sql, datasource.sql, syncColumns],
307294
);
308295

309-
useEffect(() => {
310-
if (confirmModal) {
311-
confirmModal.update({
312-
content: getSaveDialog(),
313-
});
314-
}
315-
}, [confirmModal, getSaveDialog]);
316-
317296
useEffect(() => {
318297
if (datasource.sql !== currentDatasource.sql) {
319-
syncColumnsRef.current = true;
298+
setSyncColumns(true);
320299
}
321300
}, [datasource.sql, currentDatasource.sql]);
322301

323302
const onClickSave = () => {
324-
const modalInstance = modal.confirm({
303+
modal.confirm({
325304
title: t('Confirm save'),
326305
content: getSaveDialog(),
327306
onOk: onConfirmSave,
328307
icon: null,
329308
okText: t('OK'),
330309
cancelText: t('Cancel'),
331310
});
332-
setConfirmModal(modalInstance);
333-
dialog.current = modalInstance;
334311
};
335312

336313
return (

superset-frontend/src/dashboard/components/EmbeddedModal/EmbeddedModal.test.tsx

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
getExtensionsRegistry,
2929
makeApi,
3030
} from '@superset-ui/core';
31+
import { Modal } from '@superset-ui/core/components';
3132
import setupCodeOverrides from 'src/setup/setupCodeOverrides';
3233
import DashboardEmbedModal from './index';
3334

@@ -169,3 +170,85 @@ test('adds extension to DashboardEmbedModal', async () => {
169170
await screen.findByText('dashboard.embed.modal.extension component'),
170171
).toBeInTheDocument();
171172
});
173+
174+
describe('Modal.useModal integration', () => {
175+
beforeEach(() => {
176+
jest.clearAllMocks();
177+
});
178+
179+
test('uses Modal.useModal hook for confirmation dialogs', () => {
180+
const useModalSpy = jest.spyOn(Modal, 'useModal');
181+
setup();
182+
183+
// Verify that useModal is called when the component mounts
184+
expect(useModalSpy).toHaveBeenCalled();
185+
186+
useModalSpy.mockRestore();
187+
});
188+
189+
test('renders contextHolder for proper theming', async () => {
190+
const { container } = render(<DashboardEmbedModal {...defaultProps} />, {
191+
useRedux: true,
192+
});
193+
194+
// Wait for component to be rendered
195+
await screen.findByText('Embed');
196+
197+
// The contextHolder is rendered in the component tree
198+
// Check that modal root elements exist for theming
199+
const modalRootElements = container.querySelectorAll('.ant-modal-root');
200+
expect(modalRootElements).toBeDefined();
201+
});
202+
203+
test('confirmation modal inherits theme context', async () => {
204+
setup();
205+
206+
// Click deactivate to trigger the confirmation modal
207+
const deactivate = await screen.findByRole('button', {
208+
name: 'Deactivate',
209+
});
210+
fireEvent.click(deactivate);
211+
212+
// Wait for the modal to appear
213+
const modalTitle = await screen.findByText('Disable embedding?');
214+
expect(modalTitle).toBeInTheDocument();
215+
216+
// Check that the modal is rendered within the component tree (not on body directly)
217+
const modal = modalTitle.closest('.ant-modal-wrap');
218+
expect(modal).toBeInTheDocument();
219+
});
220+
221+
test('does not use Modal.confirm directly', () => {
222+
// Spy on the static Modal.confirm method
223+
const confirmSpy = jest.spyOn(Modal, 'confirm');
224+
225+
setup();
226+
227+
// The component should not call Modal.confirm directly
228+
expect(confirmSpy).not.toHaveBeenCalled();
229+
230+
confirmSpy.mockRestore();
231+
});
232+
233+
test('modal actions work correctly with useModal', async () => {
234+
setup();
235+
236+
// Click deactivate
237+
const deactivate = await screen.findByRole('button', {
238+
name: 'Deactivate',
239+
});
240+
fireEvent.click(deactivate);
241+
242+
// Modal should appear
243+
expect(await screen.findByText('Disable embedding?')).toBeInTheDocument();
244+
245+
// Click Cancel to close modal
246+
const cancelBtn = screen.getByRole('button', { name: 'Cancel' });
247+
fireEvent.click(cancelBtn);
248+
249+
// Modal should close
250+
await waitFor(() => {
251+
expect(screen.queryByText('Disable embedding?')).not.toBeInTheDocument();
252+
});
253+
});
254+
});

superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
6565
const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); // the embedded dashboard config
6666
const [allowedDomains, setAllowedDomains] = useState<string>('');
6767

68+
// Use Modal.useModal hook to ensure proper theming
69+
const [modal, contextHolder] = Modal.useModal();
70+
6871
const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`;
6972
// whether saveable changes have been made to the config
7073
const isDirty =
@@ -100,7 +103,7 @@ export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
100103
}, [endpoint, allowedDomains]);
101104

102105
const disableEmbedded = useCallback(() => {
103-
Modal.confirm({
106+
modal.confirm({
104107
title: t('Disable embedding?'),
105108
content: t('This will remove your current embed configuration.'),
106109
okType: 'danger',
@@ -167,6 +170,7 @@ export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
167170

168171
return (
169172
<>
173+
{contextHolder}
170174
{embedded ? (
171175
DocsConfigDetails ? (
172176
<DocsConfigDetails embeddedId={embedded.uuid} />

0 commit comments

Comments
 (0)