fix(PropertiesModal): do not show validation errors while loading (#35215)

Author: msyavuz

superset-frontend/src/components/Modal/StandardModal.tsx

@@ -18,7 +18,7 @@
  */
 import { ReactNode } from 'react';
 import { styled, t } from '@superset-ui/core';
-import { Modal } from '@superset-ui/core/components';
+import { Modal, Loading, Flex } from '@superset-ui/core/components';
 import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon';
 
 interface StandardModalProps {
@@ -39,6 +39,7 @@ interface StandardModalProps {
   destroyOnClose?: boolean;
   maskClosable?: boolean;
   wrapProps?: object;
+  contentLoading?: boolean;
 }
 
 // Standard modal widths
@@ -113,12 +114,13 @@ export function StandardModal({
   destroyOnClose = true,
   maskClosable = false,
   wrapProps,
+  contentLoading = false,
 }: StandardModalProps) {
   const primaryButtonName = saveText || (isEditMode ? t('Save') : t('Add'));
 
   return (
     <StyledModal
-      disablePrimaryButton={saveDisabled || saveLoading}
+      disablePrimaryButton={saveDisabled || saveLoading || contentLoading}
       primaryButtonLoading={saveLoading}
       primaryTooltipMessage={errorTooltip}
       onHandledPrimaryAction={onSave}
@@ -139,7 +141,13 @@ export function StandardModal({
         )
       }
     >
-      {children}
+      {contentLoading ? (
+        <Flex justify="center" align="center" style={{ minHeight: 200 }}>
+          <Loading />
+        </Flex>
+      ) : (
+        children
+      )}
     </StyledModal>
   );
 }

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

@@ -354,9 +354,19 @@ describe('PropertiesModal', () => {
     mockedIsFeatureEnabled.mockReturnValue(false);
     const props = createProps();
     props.onlyApply = false;
-    render(<PropertiesModal {...props} />, {
+    // Pass dashboardInfo to avoid loading state
+    const propsWithDashboardInfo = {
+      ...props,
+      dashboardInfo: {
+        ...dashboardInfo,
+        json_metadata: mockedJsonMetadata,
+      },
+    };
+    render(<PropertiesModal {...propsWithDashboardInfo} />, {
       useRedux: true,
     });
+
+    // Wait for the form to be visible
     expect(
       await screen.findByTestId('dashboard-edit-properties-form'),
     ).toBeInTheDocument();
@@ -379,9 +389,19 @@ describe('PropertiesModal', () => {
     mockedIsFeatureEnabled.mockReturnValue(false);
     const props = createProps();
     props.onlyApply = true;
-    render(<PropertiesModal {...props} />, {
+    // Pass dashboardInfo to avoid loading state
+    const propsWithDashboardInfo = {
+      ...props,
+      dashboardInfo: {
+        ...dashboardInfo,
+        json_metadata: mockedJsonMetadata,
+      },
+    };
+    render(<PropertiesModal {...propsWithDashboardInfo} />, {
       useRedux: true,
     });
+
+    // Wait for the form to be visible
     expect(
       await screen.findByTestId('dashboard-edit-properties-form'),
     ).toBeInTheDocument();

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

@@ -112,7 +112,7 @@ const PropertiesModal = ({
   const dispatch = useDispatch();
   const [form] = Form.useForm();
 
-  const [isLoading, setIsLoading] = useState(false);
+  const [isLoading, setIsLoading] = useState(true);
   const [isApplying, setIsApplying] = useState(false);
   const [colorScheme, setCurrentColorScheme] = useState(currentColorScheme);
   const [jsonMetadata, setJsonMetadata] = useState('');
@@ -207,7 +207,6 @@ const PropertiesModal = ({
   );
 
   const fetchDashboardDetails = useCallback(() => {
-    setIsLoading(true);
     // We fetch the dashboard details because not all code
     // that renders this component have all the values we need.
     // At some point when we have a more consistent frontend
@@ -382,10 +381,6 @@ const PropertiesModal = ({
     if (onlyApply) {
       setIsApplying(true);
       try {
-        console.log('Apply CSS debug:', {
-          css_being_sent: customCss,
-          onSubmitProps_css: onSubmitProps.css,
-        });
         onSubmit(onSubmitProps);
         onHide();
         addSuccessToast(t('Dashboard properties updated'));
@@ -422,10 +417,15 @@ const PropertiesModal = ({
 
   useEffect(() => {
     if (show) {
+      // Reset loading state when modal opens
+      setIsLoading(true);
+
       if (!currentDashboardInfo) {
         fetchDashboardDetails();
       } else {
         handleDashboardData(currentDashboardInfo);
+        // Data is immediately available, so we can stop loading
+        setIsLoading(false);
       }
 
       // Fetch themes (excluding system themes)
@@ -621,10 +621,9 @@ const PropertiesModal = ({
       }}
       title={t('Dashboard properties')}
       isEditMode
-      saveDisabled={
-        isLoading || dashboardInfo?.isManagedExternally || hasErrors
-      }
+      saveDisabled={dashboardInfo?.isManagedExternally || hasErrors}
       saveLoading={isApplying}
+      contentLoading={isLoading}
       errorTooltip={
         dashboardInfo?.isManagedExternally
           ? t(
@@ -665,7 +664,6 @@ const PropertiesModal = ({
               children: (
                 <BasicInfoSection
                   form={form}
-                  isLoading={isLoading}
                   validationStatus={validationStatus}
                 />
               ),

superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.test.tsx

@@ -21,8 +21,9 @@ import { Form } from '@superset-ui/core/components';
 import BasicInfoSection from './BasicInfoSection';
 
 const defaultProps = {
-  form: {} as any,
-  isLoading: false,
+  form: {
+    getFieldValue: jest.fn(() => 'Test Dashboard'),
+  } as any,
   validationStatus: {
     basic: { hasErrors: false, errors: [], name: 'Basic' },
   },
@@ -50,16 +51,6 @@ test('shows required asterisk for name field', () => {
   expect(screen.getByText('*')).toBeInTheDocument();
 });
 
-test('disables inputs when loading', () => {
-  render(
-    <Form>
-      <BasicInfoSection {...defaultProps} isLoading />
-    </Form>,
-  );
-
-  expect(screen.getByTestId('dashboard-title-input')).toBeDisabled();
-});
-
 test('shows error message when name is empty and has validation errors', () => {
   const mockForm = {
     getFieldValue: jest.fn(field => (field === 'title' ? '' : 'test')),

superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx

@@ -23,62 +23,59 @@ import { ValidationObject } from 'src/components/Modal/useModalValidation';
 
 interface BasicInfoSectionProps {
   form: FormInstance;
-  isLoading: boolean;
   validationStatus: ValidationObject;
 }
 
 const BasicInfoSection = ({
   form,
-  isLoading,
   validationStatus,
-}: BasicInfoSectionProps) => (
-  <>
-    <ModalFormField
-      label={t('Name')}
-      required
-      testId="dashboard-name-field"
-      error={
-        validationStatus.basic?.hasErrors &&
-        (!form.getFieldValue('title') ||
-          form.getFieldValue('title').trim().length === 0)
-          ? t('Dashboard name is required')
-          : undefined
-      }
-    >
-      <FormItem
-        name="title"
-        noStyle
-        rules={[
-          {
-            required: true,
-            message: t('Dashboard name is required'),
-            whitespace: true,
-          },
-        ]}
+}: BasicInfoSectionProps) => {
+  const titleValue = form.getFieldValue('title');
+  const hasError =
+    validationStatus.basic?.hasErrors &&
+    (!titleValue || titleValue.trim().length === 0);
+
+  return (
+    <>
+      <ModalFormField
+        label={t('Name')}
+        required
+        testId="dashboard-name-field"
+        error={hasError ? t('Dashboard name is required') : undefined}
+      >
+        <FormItem
+          name="title"
+          noStyle
+          rules={[
+            {
+              required: true,
+              message: t('Dashboard name is required'),
+              whitespace: true,
+            },
+          ]}
+        >
+          <Input
+            placeholder={t('The display name of your dashboard')}
+            data-test="dashboard-title-input"
+            type="text"
+          />
+        </FormItem>
+      </ModalFormField>
+      <ModalFormField
+        label={t('URL Slug')}
+        testId="dashboard-slug-field"
+        bottomSpacing={false}
       >
-        <Input
-          placeholder={t('The display name of your dashboard')}
-          data-test="dashboard-title-input"
-          type="text"
-          disabled={isLoading}
-        />
-      </FormItem>
-    </ModalFormField>
-    <ModalFormField
-      label={t('URL Slug')}
-      testId="dashboard-slug-field"
-      bottomSpacing={false}
-    >
-      <FormItem name="slug" noStyle>
-        <Input
-          placeholder={t('A readable URL for your dashboard')}
-          data-test="dashboard-slug-input"
-          type="text"
-          disabled={isLoading}
-        />
-      </FormItem>
-    </ModalFormField>
-  </>
-);
+        <FormItem name="slug" noStyle>
+          <Input
+            placeholder={t('A readable URL for your dashboard')}
+            data-test="dashboard-slug-input"
+            type="text"
+          />
+        </FormItem>
+      </ModalFormField>
+    </>
+  );
+};
 
 export default BasicInfoSection;