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.

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

perf(native-filters): Load native filters after charts #14443

Merged
merged 21 commits into from
May 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ describe('getFormDataWithExtraFilters', () => {
const filterId = 'native-filter-1';
const mockChart = {
id: chartId,
chartAlert: null,
chartStatus: null,
chartUpdateEndTime: null,
chartUpdateStartTime: 1,
lastRendered: 1,
latestQueryFormData: {},
sliceFormData: null,
queryController: null,
queriesResponse: null,
triggerQuery: false,
formData: {
viz_type: 'filter_select',
filters: [
Expand All @@ -43,7 +53,7 @@ describe('getFormDataWithExtraFilters', () => {
const mockArgs: GetFormDataWithExtraFiltersArguments = {
chartConfiguration: {},
charts: {
[chartId]: mockChart,
[chartId as number]: mockChart,
},
chart: mockChart,
filters: {
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ class Chart extends React.PureComponent {
showMessage={false}
>
<Styles
data-ui-anchor="chart"
className="chart-container"
data-test="chart-container"
height={height}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import React, { FC, SyntheticEvent, useEffect, useState } from 'react';
import { Sticky, StickyContainer } from 'react-sticky';
import { TabContainer } from 'react-bootstrap';
import { JsonObject, styled } from '@superset-ui/core';

import ErrorBoundary from 'src/components/ErrorBoundary';
import BuilderComponentPane from 'src/dashboard/components/BuilderComponentPane';
import DashboardHeader from 'src/dashboard/containers/DashboardHeader';
Expand All @@ -31,7 +30,6 @@ import DragDroppable from 'src/dashboard/components/dnd/DragDroppable';
import DashboardComponent from 'src/dashboard/containers/DashboardComponent';
import ToastPresenter from 'src/messageToasts/containers/ToastPresenter';
import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';

import getDirectPathToTabIndex from 'src/dashboard/util/getDirectPathToTabIndex';
import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
import { URL_PARAMS } from 'src/constants';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import mockDatasource, {
import FilterBar, { FILTER_BAR_TEST_ID } from '.';
import { FILTERS_CONFIG_MODAL_TEST_ID } from '../FiltersConfigModal/FiltersConfigModal';

jest.useFakeTimers();
// @ts-ignore
mockCore.makeApi = jest.fn();

Expand Down Expand Up @@ -244,9 +245,11 @@ describe('FilterBar', () => {
expect(screen.getByRole('img', { name: 'filter' })).toBeInTheDocument();
});

it('should render the filter control name', () => {
it('should render the filter control name', async () => {
renderWrapper();
expect(screen.getByText('test')).toBeInTheDocument();
expect(
await screen.findByText('test', {}, { timeout: 2000 }),
).toBeInTheDocument();
});

it('should toggle', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import { useImmer } from 'use-immer';
import { areObjectsEqual } from 'src/reduxUtils';
import { testWithId } from 'src/utils/testUtils';
import { Filter } from 'src/dashboard/components/nativeFilters/types';
import Loading from 'src/components/Loading';
import { getInitialDataMask } from 'src/dataMask/reducer';
import {
getOnlyExtraFormData,
mapParentFiltersToChildren,
Expand All @@ -46,11 +48,11 @@ import {
useFilters,
useFilterSets,
useFilterUpdates,
useInitialization,
} from './state';
import EditSection from './FilterSets/EditSection';
import Header from './Header';
import FilterControls from './FilterControls/FilterControls';
import { getInitialDataMask } from '../../../../dataMask/reducer';

const BAR_WIDTH = `250px`;

Expand Down Expand Up @@ -216,6 +218,9 @@ const FilterBar: React.FC<FiltersBarProps> = ({
getOnlyExtraFormData(dataMaskApplied),
{ ignoreUndefined: true },
) || dataSelectedValues.length !== dataAppliedValues.length;

const isInitialized = useInitialization();

return (
<BarWrapper {...getFilterBarTestId()} className={cx({ open: filtersOpen })}>
<CollapsedBar
Expand All @@ -238,7 +243,9 @@ const FilterBar: React.FC<FiltersBarProps> = ({
dataMaskSelected={dataMaskSelected}
dataMaskApplied={dataMaskApplied}
/>
{isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) ? (
{!isInitialized ? (
<Loading />
) : isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) ? (
Comment on lines +246 to +248
Copy link
Member

Choose a reason for hiding this comment

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

I think this causes an unnecessary visual artefact - I think it's ok to keep this as-is, i.e. showing the filters in spinning state until they've fully resolved.

Copy link
Contributor Author

@simcha90 simcha90 May 4, 2021

Choose a reason for hiding this comment

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

but this is the point not to render them in order they will not start to do requests to the server :)

Copy link
Member

Choose a reason for hiding this comment

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

Haha, makes sense 🤦

<StyledTabs
centered
onChange={setTab as HandlerFunction}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import {
DataMaskStateWithId,
DataMaskWithId,
} from 'src/dataMask/types';
import { useEffect } from 'react';
import { RootState } from 'src/dashboard/types';
import { useEffect, useState } from 'react';
import { ChartsState, RootState } from 'src/dashboard/types';
import { NATIVE_FILTER_PREFIX } from '../FiltersConfigModal/utils';

export const useFilterSets = () =>
Expand Down Expand Up @@ -72,3 +72,48 @@ export const useFilterUpdates = (
});
}, [dataMaskApplied, dataMaskSelected, filters, setDataMaskSelected]);
};

// Load filters after charts loaded
export const useInitialization = () => {
const [isInitialized, setIsInitialized] = useState<boolean>(false);
const charts = useSelector<RootState, ChartsState>(state => state.charts);

// We need to know how much charts now shown on dashboard to know how many of all charts should be loaded
let numberOfLoadingCharts = 0;
if (!isInitialized) {
numberOfLoadingCharts = document.querySelectorAll(
'[data-ui-anchor="chart"]',
).length;
}
useEffect(() => {
if (isInitialized) {
return;
}

// For some dashboards may be there are no charts on first page,
// so we check up to 1 sec if there is at least on chart to load
let filterTimeout: NodeJS.Timeout;
if (numberOfLoadingCharts === 0) {
filterTimeout = setTimeout(() => {
setIsInitialized(true);
}, 1000);
}

// @ts-ignore
if (numberOfLoadingCharts > 0 && filterTimeout !== undefined) {
clearTimeout(filterTimeout);
}

const numberOfLoadedCharts = Object.values(charts).filter(
({ chartStatus }) => chartStatus !== 'loading',
).length;
if (
numberOfLoadingCharts > 0 &&
numberOfLoadedCharts >= numberOfLoadingCharts
) {
setIsInitialized(true);
}
}, [charts, isInitialized, numberOfLoadingCharts]);

return isInitialized;
};
8 changes: 5 additions & 3 deletions superset-frontend/src/dashboard/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { chart } from 'src/chart/chartReducer';
import componentTypes from 'src/dashboard/util/componentTypes';
import { DataMaskStateWithId } from '../dataMask/types';
import { NativeFiltersState } from './reducers/types';
import { ChartState } from '../explore/types';

export type ChartReducerInitialState = typeof chart;

Expand All @@ -34,8 +35,7 @@ export interface ChartQueryPayload extends Partial<ChartReducerInitialState> {
}

/** Chart state of redux */
export type Chart = {
id: number;
export type Chart = ChartState & {
formData: {
viz_type: string;
};
Expand All @@ -54,11 +54,13 @@ export type DashboardInfo = {
metadata: { show_native_filters: boolean; chart_configuration: JsonObject };
};

export type ChartsState = { [key: string]: Chart };

/** Root state of redux */
export type RootState = {
datasources: JsonObject;
sliceEntities: JsonObject;
charts: { [key: string]: Chart };
charts: ChartsState;
dashboardLayout: DashboardLayoutState;
dashboardFilters: {};
dashboardState: DashboardState;
Expand Down