Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Oct 30, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4a66cc
perf(native-filters): improve native filter modal performance
stephenLYZ Oct 15, 2022
431bbbe
fix lint
stephenLYZ Oct 15, 2022
4a8d89e
fix test
stephenLYZ Oct 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions ...ntend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
56 changes: 20 additions & 36 deletions ...rontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
9 changes: 6 additions & 3 deletions ...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ const FiltersConfigForm = (
setErroredFilters,
validateDependencies,
getDependencySuggestion,
isActive,
}: FiltersConfigFormProps,
ref: React.RefObject<any>,
) => {
Expand All @@ -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');
Expand Down Expand Up @@ -1240,6 +1241,8 @@ const FiltersConfigForm = (
);
};

export default forwardRef<typeof FiltersConfigForm, FiltersConfigFormProps>(
FiltersConfigForm,
export default React.memo(
forwardRef<typeof FiltersConfigForm, FiltersConfigFormProps>(
FiltersConfigForm,
),
);
8 changes: 6 additions & 2 deletions ...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down
173 changes: 116 additions & 57 deletions ...frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,19 +153,27 @@ 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
const [orderedFilters, setOrderedFilters] = useState<string[]>(
getInitialFilterOrder(),
);

// State for rendered filter to improve performance
const [renderedFilters, setRenderedFilters] = useState<string[]>([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: use Set for this case.

const [renderedFilters, setRenderedFilters] = useState<Set[]>([

initialCurrentFilterId,
]);

const getActiveFilterPanelKey = (filterId: string) => [
`${filterId}-${FilterPanels.configuration.key}`,
`${filterId}-${FilterPanels.settings.key}`,
Expand All @@ -175,7 +183,7 @@ export function FiltersConfigModal({
string | string[]
>(getActiveFilterPanelKey(initialCurrentFilterId));

const onTabChange = (filterId: string) => {
const handleTabChange = (filterId: string) => {
setCurrentFilterId(filterId);
setActiveFilterPanelKey(getActiveFilterPanelKey(filterId));
};
Expand Down Expand Up @@ -229,6 +237,7 @@ export function FiltersConfigModal({
if (!isSaving) {
setOrderedFilters(getInitialFilterOrder());
}
setRenderedFilters([initialCurrentFilterId]);
form.resetFields(['filters']);
form.setFieldsValue({ changed: false });
};
Expand Down Expand Up @@ -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);
Expand All @@ -405,7 +414,7 @@ export function FiltersConfigModal({
return dependencyMap;
}, [filterConfigMap, form]);

const validateDependencies = () => {
const validateDependencies = useCallback(() => {
const dependencyMap = buildDependencyMap();
filterIds
.filter(id => !removedFilters[id])
Expand All @@ -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 =
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>
Expand Down