fix(modals): use Modal.useModal hook for proper dark mode theming #35198

eschutho · 2025-09-18T22:06:21Z

Summary

Fixes dark mode theming issues in modal confirmation dialogs by replacing Modal.confirm() with the Modal.useModal() hook approach.

Problem

Modal.confirm() bypasses the theme provider context by rendering directly to document.body. This causes confirmation modals to appear with default Ant Design styling instead of the current theme styling, breaking the user experience in dark mode.

Solution

Replace Modal.confirm() with modal.confirm(): Use the modal instance from useModal hook instead of static method
Add contextHolder to JSX: Render modals within the component tree to inherit theme context
Applied to multiple components: Fixed ThemeList and EmbeddedModal components
Add comprehensive tests: Verify proper hook usage and prevent regressions

Technical Details

Before: Modals rendered directly to body, bypassing theme context:

Modal.confirm({ ... }); // ❌ Renders to document.body, no theme context

After: Modals rendered within component tree with theme context:

const [modal, contextHolder] = Modal.useModal();
modal.confirm({ ... }); // ✅ Renders within component tree with theme context
return <>{contextHolder}...</>;

Test Plan

All existing component tests pass
New tests verify Modal.useModal hook usage instead of direct Modal.confirm
Tests confirm modal interactions work without DOM errors
Tests verify contextHolder is properly rendered for theming
TypeScript compilation successful
Pre-commit hooks pass (ESLint, type checking)

🤖 Generated with Claude Code

Copilot

Pull Request Overview

This PR fixes a theming issue where confirmation dialogs created with Modal.confirm were not respecting dark mode, appearing with white backgrounds instead of the appropriate dark theme colors.

Restores the Ant Design App component wrapper in the theme provider to provide theme context for portaled elements
Updates the Modal component to use a themed wrapper for confirm dialogs
Adds documentation explaining the dependency on the App wrapper for proper theme inheritance

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx	Adds App import and wraps children with App component to provide theme context
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx	Introduces themedConfirm wrapper function with documentation for Modal.confirm

Copilot · 2025-09-18T22:07:01Z

superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx

 };
 CustomModal.displayName = 'Modal';

+// Theme-aware confirmation modal - now inherits theme through App wrapper in SupersetThemeProvider


[nitpick] The themedConfirm function is a simple passthrough that doesn't add any theming logic. Consider adding a comment explaining that the actual theming is handled by the App wrapper in SupersetThemeProvider, or remove this wrapper if it's not adding value beyond documentation.

Suggested change

// Theme-aware confirmation modal - now inherits theme through App wrapper in SupersetThemeProvider

// The themedConfirm function is a simple passthrough to AntdModal.confirm.

// Actual theming is handled by the App wrapper in SupersetThemeProvider.

// This wrapper exists only for documentation and API consistency.

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Unnecessary function wrapper adds runtime overhead ▹ view	✅ Fix detected

Files scanned

File Path	Reviewed
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx	✅
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx

+const themedConfirm = (props: Parameters<typeof AntdModal.confirm>[0]) =>
+  AntdModal.confirm(props);


github-actions · 2025-09-18T23:41:26Z

🎪 Showtime is building environment on GHA for 542949f

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Inconsistent modal theming implementation ▹ view	🧠 Not in standard

Files scanned

File Path	Reviewed
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx	✅
superset-frontend/src/pages/ThemeList/index.tsx	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

superset-frontend/src/pages/ThemeList/index.tsx

  const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null);

+  // Use Modal.useModal hook to ensure proper theming
+  const [modal, contextHolder] = Modal.useModal();


github-actions · 2025-09-18T23:54:16Z

🎪 Showtime deployed environment on GHA for 542949f

• Environment: http://34.215.184.202:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

…on dialogs Replaces Modal.confirm() with Modal.useModal() hook to ensure proper theming support in confirmation dialogs. This fixes dark mode theming issues where confirmation modals were not inheriting the correct theme context. Changes: - Add Modal.useModal() hook to DashboardEmbedControls component - Replace Modal.confirm() with modal.confirm() for disable confirmation - Add contextHolder to component JSX for proper modal theming - Update dependency array to include modal instance 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

mistercrunch

LGTM

pull-request-size bot added the size/S label Sep 18, 2025

github-actions bot added the packages label Sep 18, 2025

eschutho requested a review from Copilot September 18, 2025 22:06

Copilot AI reviewed Sep 18, 2025

View reviewed changes

dosubot bot added change:frontend Requires changing the frontend global:theming Related to theming Superset labels Sep 18, 2025

eschutho marked this pull request as draft September 18, 2025 22:07

korbit-ai bot reviewed Sep 18, 2025

View reviewed changes

superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx Outdated

Comment on lines 372 to 373

const themedConfirm = (props: Parameters<typeof AntdModal.confirm>[0]) =>

AntdModal.confirm(props);

This comment was marked as resolved.

Sign in to view

pull-request-size bot added size/L and removed size/S labels Sep 18, 2025

github-actions bot added the preset-io label Sep 18, 2025

eschutho force-pushed the fix/modal-confirm-dark-mode-theming branch from f0327d2 to 542949f Compare September 18, 2025 23:39

github-actions bot removed the packages label Sep 18, 2025

eschutho added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Sep 18, 2025

github-actions bot added 🎪 542949f 🚦 deploying Environment 542949f status: deploying and removed 🎪 542949f 🚦 building Environment 542949f status: building labels Sep 18, 2025

eschutho marked this pull request as ready for review September 18, 2025 23:51

korbit-ai bot reviewed Sep 18, 2025

View reviewed changes

superset-frontend/src/pages/ThemeList/index.tsx Outdated

const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null);

// Use Modal.useModal hook to ensure proper theming

const [modal, contextHolder] = Modal.useModal();

This comment was marked as resolved.

Sign in to view

github-actions bot removed the 🎪 🎯 542949f Active environment pointer - 542949f is receiving traffic label Sep 18, 2025

msyavuz self-requested a review September 19, 2025 17:34

eschutho changed the title ~~fix(modals): restore App wrapper to fix Modal.confirm dark mode theming~~ fix(modals): use Modal.useModal hook for proper dark mode theming Sep 24, 2025

eschutho force-pushed the fix/modal-confirm-dark-mode-theming branch 2 times, most recently from 72ce58c to d37caf9 Compare September 25, 2025 17:09

pull-request-size bot added size/S and removed size/L labels Sep 25, 2025

eschutho force-pushed the fix/modal-confirm-dark-mode-theming branch 2 times, most recently from 8f3dd6a to 5cd88c5 Compare September 25, 2025 17:44

pull-request-size bot added size/L and removed size/S labels Sep 25, 2025

eschutho force-pushed the fix/modal-confirm-dark-mode-theming branch from 5cd88c5 to 0d61441 Compare September 25, 2025 23:26

pull-request-size bot added size/XS and removed size/L labels Sep 25, 2025

mistercrunch approved these changes Sep 26, 2025

View reviewed changes

msyavuz approved these changes Sep 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(modals): use Modal.useModal hook for proper dark mode theming #35198

fix(modals): use Modal.useModal hook for proper dark mode theming #35198

eschutho commented Sep 18, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

korbit-ai bot left a comment •

edited

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions bot commented Sep 18, 2025

Uh oh!

korbit-ai bot left a comment •

edited

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions bot commented Sep 18, 2025

Uh oh!

mistercrunch left a comment

Uh oh!

Uh oh!

-// Theme-aware confirmation modal - now inherits theme through App wrapper in SupersetThemeProvider
+// The themedConfirm function is a simple passthrough to AntdModal.confirm.
+// Actual theming is handled by the App wrapper in SupersetThemeProvider.
+// This wrapper exists only for documentation and API consistency.

		const themedConfirm = (props: Parameters<typeof AntdModal.confirm>[0]) =>
		AntdModal.confirm(props);

fix(modals): use Modal.useModal hook for proper dark mode theming #35198

Are you sure you want to change the base?

fix(modals): use Modal.useModal hook for proper dark mode theming #35198

Conversation

eschutho commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Technical Details

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions bot commented Sep 18, 2025

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions bot commented Sep 18, 2025

Uh oh!

mistercrunch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eschutho commented Sep 18, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading

korbit-ai bot left a comment •

edited

Loading