Skip to content

[Synthetics] Fix filters persistance #152543

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

Merged
shahzad31 merged 5 commits into from
Mar 6, 2023
Merged

[Synthetics] Fix filters persistance #152543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb5fe44
fix filters persistance
shahzad31 Mar 2, 2023
c285698
Merge branch 'main' of github.com:elastic/kibana into fix-persis-filters
shahzad31 Mar 2, 2023
8fa86ac
handle edge case
shahzad31 Mar 2, 2023
450c3ca
Merge branch 'main' of github.com:elastic/kibana into fix-persis-filters
shahzad31 Mar 2, 2023
8262a87
update code owners
shahzad31 Mar 2, 2023
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
5 changes: 5 additions & 0 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,12 @@ packages/kbn-yarn-lock-validator @elastic/kibana-operations
/x-pack/test/functional/services/uptime @elastic/uptime
/x-pack/test/api_integration/apis/uptime @elastic/uptime
/x-pack/plugins/observability/public/components/shared/exploratory_view @elastic/uptime
/x-pack/plugins/observability/public/components/shared/field_value_suggestions @elastic/uptime
/x-pack/plugins/observability/public/components/shared/core_web_vitals @elastic/uptime
/x-pack/plugins/observability/public/components/shared/load_when_in_view @elastic/uptime
/x-pack/plugins/observability/public/components/shared/filter_value_label @elastic/uptime
/x-pack/plugins/observability/public/utils/observability_data_views @elastic/uptime
/x-pack/plugins/observability/e2e @elastic/uptime

# Client Side Monitoring / Uptime (lives in APM directories but owned by Uptime)
/x-pack/plugins/apm/public/application/uxApp.tsx @elastic/uptime
Expand Down
3 changes: 2 additions & 1 deletion .../observability/public/components/shared/field_value_suggestions/field_value_selection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ export function FieldValueSelection({

useEffect(() => {
setOptions(formatOptions(values, selectedValue, excludedValue, showCount));
}, [values, selectedValue, showCount, excludedValue]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(values), JSON.stringify(selectedValue), showCount, excludedValue]);
Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Mar 6, 2023

Choose a reason for hiding this comment

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

Doesn't this impact performance since it uses JSON.stringify on every render?
Can't we use useMemo in the parent component instead?

Copy link
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Mar 6, 2023

Choose a reason for hiding this comment

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

i think in this specific case. it won't have any impact. Since it's only meant to be triggered on new fetch of the data.

Copy link
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Mar 6, 2023

Choose a reason for hiding this comment

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

if the new fetch of the data remains same, we don't want existing component to re-render since data selection should remain same.

Copy link
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Mar 6, 2023

Choose a reason for hiding this comment

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

Using useMemo won't help in parent component.

Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i think in this specific case. it won't have any impact. Since it's only meant to be triggered on new fetch of the data.

You mean FieldValueSelection only re-renders when there is new data? Where is this logic?

Using useMemo won't help in parent component.

Why is that?

Copy link
Copy Markdown
Contributor Author

@shahzad31 shahzad31 Mar 6, 2023

Choose a reason for hiding this comment

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

this is being used on auto-refresh in synthetics app. so when that happens. the new data remains same , but it will trigger useMemo re-render. this is why we are doing this deep comparison. Json.stringify impact is minimal here. since data isn't big.


const onButtonClick = () => {
setIsPopoverOpen(!isPopoverOpen);
Expand Down
49 changes: 44 additions & 5 deletions ...s/public/apps/synthetics/components/monitors_page/common/monitor_filters/filter_group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,33 @@ import React from 'react';
import { EuiFilterGroup } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { useSelector } from 'react-redux';
import { useGetUrlParams } from '../../../../hooks';
import { selectServiceLocationsState } from '../../../../state';

import {
SyntheticsMonitorFilterItem,
getSyntheticsFilterDisplayValues,
SyntheticsMonitorFilterChangeHandler,
LabelWithCountValue,
} from './filter_fields';
import { useFilters } from './use_filters';
import { FilterButton } from './filter_button';

const mixUrlValues = (
values?: LabelWithCountValue[],
urlLabels?: string[]
): LabelWithCountValue[] => {
const urlValues = urlLabels?.map((label) => ({ label, count: 0 })) ?? [];
const newValues = [...(values ?? [])];
// add url values that are not in the values
urlValues.forEach((urlValue) => {
if (!newValues.find((value) => value.label === urlValue.label)) {
newValues.push(urlValue);
}
});
return newValues;
};

export const FilterGroup = ({
handleFilterChange,
}: {
Expand All @@ -28,34 +45,56 @@ export const FilterGroup = ({

const { locations } = useSelector(selectServiceLocationsState);

const urlParams = useGetUrlParams();

const filters: SyntheticsMonitorFilterItem[] = [
{
label: TYPE_LABEL,
field: 'monitorTypes',
values: getSyntheticsFilterDisplayValues(data.monitorTypes, 'monitorTypes', locations),
values: getSyntheticsFilterDisplayValues(
mixUrlValues(data.monitorTypes, urlParams.monitorTypes),
'monitorTypes',
locations
),
},
{
label: LOCATION_LABEL,
field: 'locations',
values: getSyntheticsFilterDisplayValues(data.locations, 'locations', locations),
values: getSyntheticsFilterDisplayValues(
mixUrlValues(data.locations, urlParams.locations),
'locations',
locations
),
},
{
label: TAGS_LABEL,
field: 'tags',
values: getSyntheticsFilterDisplayValues(data.tags, 'tags', locations),
values: getSyntheticsFilterDisplayValues(
mixUrlValues(data.tags, urlParams.tags),
'tags',
locations
),
},
{
label: SCHEDULE_LABEL,
field: 'schedules',
values: getSyntheticsFilterDisplayValues(data.schedules, 'schedules', locations),
values: getSyntheticsFilterDisplayValues(
mixUrlValues(data.schedules, urlParams.schedules),
'schedules',
locations
),
},
];

if (data.projects.length > 0) {
filters.push({
label: PROJECT_LABEL,
field: 'projects',
values: getSyntheticsFilterDisplayValues(data.projects, 'projects', locations),
values: getSyntheticsFilterDisplayValues(
mixUrlValues(data.projects, urlParams.projects),
'projects',
locations
),
});
}

Expand Down
6 changes: 3 additions & 3 deletions ...s/public/apps/synthetics/components/monitors_page/common/monitor_filters/list_filters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
* 2.0.
*/

import React, { memo } from 'react';
import React from 'react';
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';

import { FilterGroup } from './filter_group';
import { SearchField } from '../search_field';
import { SyntheticsMonitorFilterChangeHandler } from './filter_fields';

export const ListFilters = memo(function ({
export const ListFilters = function ({
handleFilterChange,
}: {
handleFilterChange: SyntheticsMonitorFilterChangeHandler;
Expand All @@ -27,4 +27,4 @@ export const ListFilters = memo(function ({
</EuiFlexItem>
</EuiFlexGroup>
);
});
};
4 changes: 2 additions & 2 deletions x-pack/plugins/synthetics/public/apps/synthetics/state/monitor_list/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ export const monitorListReducer = createReducer(initialState, (builder) => {
delete state.monitorUpsertStatuses[action.payload];
}
})
.addCase(cleanMonitorListState, () => {
return initialState;
.addCase(cleanMonitorListState, (state) => {
return { ...initialState, pageState: state.pageState };
});
});

Expand Down