perf(native-filters): improve native filter modal form performance (#21821)

2022-10-30 23:55:48 +08:00 · 2022-10-30 23:55:48 +08:00 · bf001931c8
parent 3ea8f20f71
commit bf001931c8
5 changed files with 148 additions and 106 deletions
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
@ -26,7 +26,6 @@ const scrollMock = jest.fn();
 Element.prototype.scroll = scrollMock;

 const defaultProps = {
-  children: jest.fn(),
  getFilterTitle: (id: string) => id,
  onChange: jest.fn(),
  onAdd: jest.fn(),
@ -63,13 +62,6 @@ beforeEach(() => {
  scrollMock.mockClear();
 });

-test('renders form', async () => {
-  await act(async () => {
-    defaultRender();
-  });
-  expect(defaultProps.children).toHaveBeenCalledTimes(3);
-});
-
 test('drag and drop', async () => {
  defaultRender();
  // Drag the state and country filter above the product filter
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
@ -22,7 +22,7 @@ import FilterTitlePane from './FilterTitlePane';
 import { FilterRemoval } from './types';

 interface Props {
-  children: (filterId: string) => React.ReactNode;
+  children?: React.ReactNode;
  getFilterTitle: (filterId: string) => string;
  onChange: (activeKey: string) => void;
  onAdd: (type: NativeFilterType) => void;
@ -62,40 +62,24 @@ const FilterConfigurePane: React.FC<Props> = ({
  currentFilterId,
  filters,
  removedFilters,
-}) => {
-  const active = filters.filter(id => id === currentFilterId)[0];
-  return (
-    <Container>
-      <TitlesContainer>
-        <FilterTitlePane
-          currentFilterId={currentFilterId}
-          filters={filters}
-          removedFilters={removedFilters}
-          erroredFilters={erroredFilters}
-          getFilterTitle={getFilterTitle}
-          onChange={onChange}
-          onAdd={(type: NativeFilterType) => onAdd(type)}
-          onRearrange={onRearrange}
-          onRemove={(id: string) => onRemove(id)}
-          restoreFilter={restoreFilter}
-        />
-      </TitlesContainer>
-      <ContentHolder>
-        {filters.map(id => (
-          <div
-            key={id}
-            style={{
-              height: '100%',
-              overflowY: 'auto',
-              display: id === active ? '' : 'none',
-            }}
-          >
-            {children(id)}
-          </div>
-        ))}
-      </ContentHolder>
-    </Container>
-  );
-};
+}) => (
+  <Container>
+    <TitlesContainer>
+      <FilterTitlePane
+        currentFilterId={currentFilterId}
+        filters={filters}
+        removedFilters={removedFilters}
+        erroredFilters={erroredFilters}
+        getFilterTitle={getFilterTitle}
+        onChange={onChange}
+        onAdd={(type: NativeFilterType) => onAdd(type)}
+        onRearrange={onRearrange}
+        onRemove={(id: string) => onRemove(id)}
+        restoreFilter={restoreFilter}
+      />
+    </TitlesContainer>
+    <ContentHolder>{children}</ContentHolder>
+  </Container>
+);

 export default FilterConfigurePane;
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@ -332,6 +332,7 @@ const FiltersConfigForm = (
    setErroredFilters,
    validateDependencies,
    getDependencySuggestion,
+    isActive,
  }: FiltersConfigFormProps,
  ref: React.RefObject<any>,
 ) => {
@ -346,7 +347,7 @@ const FiltersConfigForm = (
    string,
    any
  > | null>(null);
-  const forceUpdate = useForceUpdate();
+  const forceUpdate = useForceUpdate(isActive);
  const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
  const defaultFormFilter = useMemo(() => ({}), []);
  const filters = form.getFieldValue('filters');
@ -1240,6 +1241,8 @@ const FiltersConfigForm = (
  );
 };

-export default forwardRef<typeof FiltersConfigForm, FiltersConfigFormProps>(
-  FiltersConfigForm,
+export default React.memo(
+  forwardRef<typeof FiltersConfigForm, FiltersConfigFormProps>(
+    FiltersConfigForm,
+  ),
 );
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
@ -38,9 +38,13 @@ export const FILTER_SUPPORTED_TYPES = {
  filter_range: [GenericDataType.NUMERIC],
 };

-export const useForceUpdate = () => {
+export const useForceUpdate = (isActive = true) => {
  const [, updateState] = React.useState({});
-  return React.useCallback(() => updateState({}), []);
+  return React.useCallback(() => {
+    if (isActive) {
+      updateState({});
+    }
+  }, [isActive]);
 };

 export const setNativeFilterFieldValues = (
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@ -153,12 +153,15 @@ export function FiltersConfigModal({

  const unsavedFiltersIds = newFilterIds.filter(id => !removedFilters[id]);
  // brings back a filter that was previously removed ("Undo")
-  const restoreFilter = (id: string) => {
-    const removal = removedFilters[id];
-    // gotta clear the removal timeout to prevent the filter from getting deleted
-    if (removal?.isPending) clearTimeout(removal.timerId);
-    setRemovedFilters(current => ({ ...current, [id]: null }));
-  };
+  const restoreFilter = useCallback(
+    (id: string) => {
+      const removal = removedFilters[id];
+      // gotta clear the removal timeout to prevent the filter from getting deleted
+      if (removal?.isPending) clearTimeout(removal.timerId);
+      setRemovedFilters(current => ({ ...current, [id]: null }));
+    },
+    [removedFilters],
+  );
  const getInitialFilterOrder = () => Object.keys(filterConfigMap);

  // State for tracking the re-ordering of filters
@ -166,6 +169,11 @@ export function FiltersConfigModal({
    getInitialFilterOrder(),
  );

+  // State for rendered filter to improve performance
+  const [renderedFilters, setRenderedFilters] = useState<string[]>([
+    initialCurrentFilterId,
+  ]);
+
  const getActiveFilterPanelKey = (filterId: string) => [
    `${filterId}-${FilterPanels.configuration.key}`,
    `${filterId}-${FilterPanels.settings.key}`,
@ -175,7 +183,7 @@ export function FiltersConfigModal({
    string | string[]
  >(getActiveFilterPanelKey(initialCurrentFilterId));

-  const onTabChange = (filterId: string) => {
+  const handleTabChange = (filterId: string) => {
    setCurrentFilterId(filterId);
    setActiveFilterPanelKey(getActiveFilterPanelKey(filterId));
  };
@ -229,6 +237,7 @@ export function FiltersConfigModal({
    if (!isSaving) {
      setOrderedFilters(getInitialFilterOrder());
    }
+    setRenderedFilters([initialCurrentFilterId]);
    form.resetFields(['filters']);
    form.setFieldsValue({ changed: false });
  };
@ -379,7 +388,7 @@ export function FiltersConfigModal({
      handleConfirmCancel();
    }
  };
-  const onRearrange = (dragIndex: number, targetIndex: number) => {
+  const handleRearrange = (dragIndex: number, targetIndex: number) => {
    const newOrderedFilter = [...orderedFilters];
    const removed = newOrderedFilter.splice(dragIndex, 1)[0];
    newOrderedFilter.splice(targetIndex, 0, removed);
@ -405,7 +414,7 @@ export function FiltersConfigModal({
    return dependencyMap;
  }, [filterConfigMap, form]);

-  const validateDependencies = () => {
+  const validateDependencies = useCallback(() => {
    const dependencyMap = buildDependencyMap();
    filterIds
      .filter(id => !removedFilters[id])
@ -418,26 +427,35 @@ export function FiltersConfigModal({
        form.setFields([field]);
      });
    handleErroredFilters();
-  };
+  }, [
+    buildDependencyMap,
+    filterIds,
+    form,
+    handleErroredFilters,
+    removedFilters,
+  ]);

-  const getDependencySuggestion = (filterId: string) => {
-    const dependencyMap = buildDependencyMap();
-    const possibleDependencies = orderedFilters.filter(
-      key => key !== filterId && canBeUsedAsDependency(key),
-    );
-    const found = possibleDependencies.find(filter => {
-      const dependencies = dependencyMap.get(filterId) || [];
-      dependencies.push(filter);
-      if (hasCircularDependency(dependencyMap, filterId)) {
-        dependencies.pop();
-        return false;
-      }
-      return true;
-    });
-    return found || possibleDependencies[0];
-  };
+  const getDependencySuggestion = useCallback(
+    (filterId: string) => {
+      const dependencyMap = buildDependencyMap();
+      const possibleDependencies = orderedFilters.filter(
+        key => key !== filterId && canBeUsedAsDependency(key),
+      );
+      const found = possibleDependencies.find(filter => {
+        const dependencies = dependencyMap.get(filterId) || [];
+        dependencies.push(filter);
+        if (hasCircularDependency(dependencyMap, filterId)) {
+          dependencies.pop();
+          return false;
+        }
+        return true;
+      });
+      return found || possibleDependencies[0];
+    },
+    [buildDependencyMap, canBeUsedAsDependency, orderedFilters],
+  );

-  const onValuesChange = useMemo(
+  const handleValuesChange = useMemo(
    () =>
      debounce((changes: any, values: NativeFiltersForm) => {
        const didChangeFilterName =
@ -466,32 +484,73 @@ export function FiltersConfigModal({
    );
  }, [removedFilters]);

-  const getForm = (id: string) => {
-    const isDivider = id.startsWith(NATIVE_FILTER_DIVIDER_PREFIX);
-    return isDivider ? (
-      <DividerConfigForm
-        componentId={id}
-        divider={filterConfigMap[id] as Divider}
-      />
-    ) : (
-      <FiltersConfigForm
-        ref={configFormRef}
-        form={form}
-        filterId={id}
-        filterToEdit={filterConfigMap[id] as Filter}
-        removedFilters={removedFilters}
-        restoreFilter={restoreFilter}
-        getAvailableFilters={getAvailableFilters}
-        key={id}
-        activeFilterPanelKeys={activeFilterPanelKey}
-        handleActiveFilterPanelChange={key => setActiveFilterPanelKey(key)}
-        isActive={currentFilterId === id}
-        setErroredFilters={setErroredFilters}
-        validateDependencies={validateDependencies}
-        getDependencySuggestion={getDependencySuggestion}
-      />
-    );
-  };
+  useEffect(() => {
+    if (!renderedFilters.includes(currentFilterId)) {
+      setRenderedFilters([...renderedFilters, currentFilterId]);
+    }
+  }, [currentFilterId]);
+
+  const handleActiveFilterPanelChange = useCallback(
+    key => setActiveFilterPanelKey(key),
+    [setActiveFilterPanelKey],
+  );
+
+  const formList = useMemo(
+    () =>
+      orderedFilters.map(id => {
+        if (!renderedFilters.includes(id)) return null;
+        const isDivider = id.startsWith(NATIVE_FILTER_DIVIDER_PREFIX);
+        const isActive = currentFilterId === id;
+        return (
+          <div
+            key={id}
+            style={{
+              height: '100%',
+              overflowY: 'auto',
+              display: isActive ? '' : 'none',
+            }}
+          >
+            {isDivider ? (
+              <DividerConfigForm
+                componentId={id}
+                divider={filterConfigMap[id] as Divider}
+              />
+            ) : (
+              <FiltersConfigForm
+                ref={configFormRef}
+                form={form}
+                filterId={id}
+                filterToEdit={filterConfigMap[id] as Filter}
+                removedFilters={removedFilters}
+                restoreFilter={restoreFilter}
+                getAvailableFilters={getAvailableFilters}
+                key={id}
+                activeFilterPanelKeys={activeFilterPanelKey}
+                handleActiveFilterPanelChange={handleActiveFilterPanelChange}
+                isActive={isActive}
+                setErroredFilters={setErroredFilters}
+                validateDependencies={validateDependencies}
+                getDependencySuggestion={getDependencySuggestion}
+              />
+            )}
+          </div>
+        );
+      }),
+    [
+      renderedFilters,
+      orderedFilters,
+      currentFilterId,
+      filterConfigMap,
+      form,
+      removedFilters,
+      restoreFilter,
+      getAvailableFilters,
+      activeFilterPanelKey,
+      validateDependencies,
+      getDependencySuggestion,
+      handleActiveFilterPanelChange,
+    ],
+  );

  return (
    <StyledModalWrapper
@ -519,22 +578,22 @@ export function FiltersConfigModal({
        <StyledModalBody>
          <StyledForm
            form={form}
-            onValuesChange={onValuesChange}
+            onValuesChange={handleValuesChange}
            layout="vertical"
          >
            <FilterConfigurePane
              erroredFilters={erroredFilters}
              onRemove={handleRemoveItem}
              onAdd={addFilter}
-              onChange={onTabChange}
+              onChange={handleTabChange}
              getFilterTitle={getFilterTitle}
              currentFilterId={currentFilterId}
              removedFilters={removedFilters}
              restoreFilter={restoreFilter}
-              onRearrange={onRearrange}
+              onRearrange={handleRearrange}
              filters={orderedFilters}
            >
-              {(id: string) => getForm(id)}
+              {formList}
            </FilterConfigurePane>
          </StyledForm>
        </StyledModalBody>