-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Resolver] Remove currentPanelView
selector
#71154
Conversation
The `currentPanelView` selector returns a value that's out of sync with the component that uses it.
type: 'appDisplayedDifferentPanel', | ||
payload: panelToShow, | ||
}); | ||
}, [panelToShow, dispatch]); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
There was a problem hiding this comment.
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?
💚 Build SucceededBuild metrics
To update your PR or re-run it, just comment with: |
The `currentPanelView` selector returns a value that's out of sync with the component that uses it.
The `currentPanelView` selector returns a value that's out of sync with the component that uses it.
* master: (39 commits) [APM] Add warning to notify user about legacy ML jobs (elastic#71030) updates consumer to siem (elastic#71117) Index pattern creation flow - fix spelling (elastic#71192) [Security Solution][Endpoint] User Manifest Cleanup + Artifact Compression (elastic#70759) [SECURITY] Rearrange rule name's column in Alert Table (elastic#71020) [SECURITY] Alerts back to Detections (elastic#71142) [Security Solution][Exceptions Builder] - Fixes operator selection bug (elastic#71178) [SIEM][Detection Engine] Speeds up value list imports by enabling streaming of files. [APM] Update ML job ID in data telemetry tasks (elastic#71044) [Resolver] Remove `currentPanelView` selector (elastic#71154) add meta.managed to index templates (elastic#71135) Clarify trial subscription levels (elastic#70900) [Security Solution] fix panel links (elastic#71148) skip flaky suite (elastic#69632) skip suite failing ES Promotion (elastic#71018) [ML] DF Analytics: add results field to wizard and show regression stats (elastic#70893) [SIEM] update wordings (elastic#71119) [SECURITY SOLUTION] Rename to hosts and administration (elastic#70913) [ML] Improvements for urlState hook. (elastic#70576) Removing uptime guide (elastic#71124) ...
The
currentPanelView
selector returns a value that's out of syncwith the component that uses it.
Remove the faulty selector, piece of state, and action.
Summary
Checklist
Delete any items that are not applicable to this PR.
For maintainers