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

[Resolver] Remove currentPanelView selector #71154

Merged
merged 1 commit into from
Jul 8, 2020
Merged

[Resolver] Remove currentPanelView selector #71154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 0 additions & 12 deletions x-pack/plugins/security_solution/public/resolver/store/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,6 @@ interface UserBroughtProcessIntoView {
};
}

/**
* Dispatched to notify state that a different panel needs to be displayed
*/
interface AppDisplayedDifferentPanel {
readonly type: 'appDisplayedDifferentPanel';
/**
* The name of the panel to display
*/
readonly payload: string;
}

/**
* When an examination of query params in the UI indicates that state needs to
* be updated to reflect the new selection
Expand Down Expand Up @@ -130,5 +119,4 @@ export type ResolverAction =
| UserRequestedRelatedEventData
| UserSelectedRelatedEventCategory
| AppDetectedNewIdFromQueryParams
| AppDisplayedDifferentPanel
| AppDetectedMissingEventData;
6 changes: 0 additions & 6 deletions x-pack/plugins/security_solution/public/resolver/store/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const uiReducer: Reducer<ResolverUIState, ResolverAction> = (
activeDescendantId: null,
selectedDescendantId: null,
processEntityIdOfSelectedDescendant: null,
panelToDisplay: null,
},
action
) => {
Expand All @@ -39,11 +38,6 @@ const uiReducer: Reducer<ResolverUIState, ResolverAction> = (
selectedDescendantId: action.payload.nodeId,
processEntityIdOfSelectedDescendant: action.payload.selectedProcessId,
};
} else if (action.type === 'appDisplayedDifferentPanel') {
return {
...uiState,
panelToDisplay: action.payload,
};
} else if (
action.type === 'userBroughtProcessIntoView' ||
action.type === 'appDetectedNewIdFromQueryParams'
Expand Down
5 changes: 0 additions & 5 deletions x-pack/plugins/security_solution/public/resolver/store/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,6 @@ export const uiSelectedDescendantProcessId = composeSelectors(
uiSelectors.selectedDescendantProcessId
);

/**
* The current panel to display
*/
export const currentPanelView = composeSelectors(uiStateSelector, uiSelectors.currentPanelView);

/**
* Returns the camera state from within ResolverState
*/
Expand Down
5 changes: 0 additions & 5 deletions x-pack/plugins/security_solution/public/resolver/store/ui/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,3 @@ export const selectedDescendantProcessId = createSelector(
return processEntityIdOfSelectedDescendant;
}
);

// Select the current panel to be displayed
export const currentPanelView = (uiState: ResolverUIState) => {
return uiState.panelToDisplay;
};
4 changes: 0 additions & 4 deletions x-pack/plugins/security_solution/public/resolver/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ export interface ResolverUIState {
* The entity_id of the process for the resolver's currently selected descendant.
*/
readonly processEntityIdOfSelectedDescendant: string | null;
/**
* Which panel the ui should display
*/
readonly panelToDisplay: string | null;
}

/**
Expand Down
29 changes: 6 additions & 23 deletions x-pack/plugins/security_solution/public/resolver/view/panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, {
memo,
useCallback,
useMemo,
useContext,
useLayoutEffect,
useState,
useEffect,
} from 'react';
import React, { memo, useCallback, useMemo, useContext, useLayoutEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import { useHistory } from 'react-router-dom';
// eslint-disable-next-line import/no-nodejs-modules
Expand Down Expand Up @@ -205,21 +197,12 @@ const PanelContent = memo(function PanelContent() {
return 'processListWithCounts';
}, [uiSelectedEvent, crumbEvent, crumbId, graphableProcessEntityIds]);

useEffect(() => {
// dispatch `appDisplayedDifferentPanel` to sync state with which panel gets displayed
dispatch({
type: 'appDisplayedDifferentPanel',
payload: panelToShow,
});
}, [panelToShow, dispatch]);
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ So even though panelToShow has a dep on crumbEvent,crumbId -> which come from the useHistory hook (which should change when the URL changes?) it doesn't trigger this effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

disregard any stuff I said in chat. my original assumptions were wrong. this does trigger, but on the render that triggers, the panel still renders using the old currentPanelView value from State. that is fine except that those components get props which are (in part) based on data from useLocation. so it's like half the data is at time t and half is at time t + 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

triage mov
This gif shows me debugging the issue on master. First I added this code above the return in panel.tsx

  console.log(
    'panelToShow',
    panelToShow,
    'currentPanelView',
    currentPanelView,
    'relatedStatsForIdFromParams',
    relatedStatsForIdFromParams
  );

The goal is to show that panelToShow and currentPanelView are out of sync, and that other values which are used as props to panels aren't in sync with currentPanelView because they are (in part) based on useHistory (or useLocation, or anything else not in redux.) Normally redux makes all state transitions atomic and ensures that all pieces of state are in sync.

In the gif, when the error occurs I scroll up and you can see this:
image

currentPanelView is eventCountsForProcess. That renders the EventCountsForProcess component which takes relatedStatsForIdFromParams. In fact the code assumes that relatedStatsForIdFromParams will be truthy if currentPanelView is eventCountsForProcess. You can see that relatedStatsForIdFromParams is actually undefined. It is based (in part) on idFromParams which is in turn based on queryParams which is based on useHistory (or useLocation.)

Data from useHistory (or useLocation) will be up to date at the time of render. But currentPanelView is behind because this component dispatches and event which changes it whenever panelToView changes.

The solution in the PR isn't a good long term solution. Values calculated in the render method of this component use data from redux and data thats not from redux. It also updates the redux data (via dispatch) based on data not from redux. Without a single, atomically modified, state container that is shared across all components, keeping this stuff in sync will be tricky.

It is passed with the non-null assertion operator, so typescript doesn't catch the fact that it's undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Initially, you asked for that so that we at least had some valence with which panel was showing in state. Is there a way to synchronize it / make it work so it doesn't twitch when the URL changes?


const currentPanelView = useSelector(selectors.currentPanelView);
const terminatedProcesses = useSelector(selectors.terminatedProcesses);
const processEntityId = uiSelectedEvent ? event.entityId(uiSelectedEvent) : undefined;
const isProcessTerminated = processEntityId ? terminatedProcesses.has(processEntityId) : false;

const panelInstance = useMemo(() => {
if (currentPanelView === 'processDetails') {
if (panelToShow === 'processDetails') {
return (
<ProcessDetails
processEvent={uiSelectedEvent!}
Expand All @@ -230,7 +213,7 @@ const PanelContent = memo(function PanelContent() {
);
}

if (currentPanelView === 'eventCountsForProcess') {
if (panelToShow === 'eventCountsForProcess') {
return (
<EventCountsForProcess
processEvent={uiSelectedEvent!}
Expand All @@ -240,7 +223,7 @@ const PanelContent = memo(function PanelContent() {
);
}

if (currentPanelView === 'processEventListNarrowedByType') {
if (panelToShow === 'processEventListNarrowedByType') {
return (
<ProcessEventListNarrowedByType
processEvent={uiSelectedEvent!}
Expand All @@ -251,7 +234,7 @@ const PanelContent = memo(function PanelContent() {
);
}

if (currentPanelView === 'relatedEventDetail') {
if (panelToShow === 'relatedEventDetail') {
const parentCount: number = Object.values(
relatedStatsForIdFromParams?.events.byCategory || {}
).reduce((sum, val) => sum + val, 0);
Expand All @@ -278,7 +261,7 @@ const PanelContent = memo(function PanelContent() {
crumbId,
pushToQueryParams,
relatedStatsForIdFromParams,
currentPanelView,
panelToShow,
isProcessTerminated,
]);

Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,13 @@ const invalidDateText = i18n.translate(
}
);
/**
* @param {ConstructorParameters<typeof Date>[0]} timestamp To be passed through Date->Intl.DateTimeFormat
* @returns {string} A nicely formatted string for a date
*/
export function formatDate(timestamp: ConstructorParameters<typeof Date>[0]) {
export function formatDate(
/** To be passed through Date->Intl.DateTimeFormat */ timestamp: ConstructorParameters<
typeof Date
>[0]
): string {
const date = new Date(timestamp);
if (isFinite(date.getTime())) {
return formatter.format(date);
Expand Down