-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Security solution] Fix side effects in useRefetchByRestartingSession
#148591
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
Changes from all commits
2043a7f
fdabf91
4ca2362
4af8501
5711fa8
36a92d0
f181923
eac0554
71637ed
ee27615
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,14 @@ | |
| * 2.0. | ||
| */ | ||
|
|
||
| import { useCallback, useRef, useEffect, useMemo } from 'react'; | ||
| import { useCallback, useRef, useEffect, useState } from 'react'; | ||
| import { useDispatch } from 'react-redux'; | ||
| import { useDeepEqualSelector } from '../../hooks/use_selector'; | ||
| import { useKibana } from '../../lib/kibana'; | ||
| import { inputsSelectors } from '../../store'; | ||
| import { inputsActions } from '../../store/actions'; | ||
| import { InputsModelId } from '../../store/inputs/constants'; | ||
| import type { Refetch } from '../../store/inputs/model'; | ||
| import { useDeepEqualSelector } from '../../common/hooks/use_selector'; | ||
| import { useKibana } from '../../common/lib/kibana'; | ||
| import { inputsSelectors } from '../../common/store'; | ||
| import { inputsActions } from '../../common/store/actions'; | ||
| import { InputsModelId } from '../../common/store/inputs/constants'; | ||
| import type { Refetch } from '../../common/store/inputs/model'; | ||
|
|
||
| interface UseRefetchByRestartingSessionProps { | ||
| inputId?: InputsModelId; | ||
|
|
@@ -24,14 +24,15 @@ export const useRefetchByRestartingSession = ({ | |
| inputId, | ||
| queryId, | ||
| }: UseRefetchByRestartingSessionProps): { | ||
| searchSessionId: string; | ||
| searchSessionId?: string; | ||
| refetchByRestartingSession: Refetch; | ||
| } => { | ||
| const dispatch = useDispatch(); | ||
| const { data } = useKibana().services; | ||
|
|
||
| const session = useRef(data.search.session); | ||
| const searchSessionId = useMemo(() => session.current.start(), [session]); | ||
| const [searchSessionId, setSearchSessionId] = useState<string | undefined>(undefined); | ||
|
|
||
| const getGlobalQuery = inputsSelectors.globalQueryByIdSelector(); | ||
| const getTimelineQuery = inputsSelectors.timelineQueryByIdSelector(); | ||
| const { selectedInspectIndex } = useDeepEqualSelector((state) => | ||
|
|
@@ -57,10 +58,12 @@ export const useRefetchByRestartingSession = ({ | |
| }, [dispatch, queryId, selectedInspectIndex]); | ||
|
|
||
| useEffect(() => { | ||
| setSearchSessionId(session.current.start()); | ||
| const currentSession = session.current; | ||
| return () => { | ||
| data.search.session.clear(); | ||
| currentSession.clear(); | ||
| }; | ||
| }); | ||
| }, []); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. react wants empty dependency argument now that we're using a |
||
|
|
||
| return { searchSessionId, refetchByRestartingSession }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ import * as i18n from './translations'; | |
| import { useQueryToggle } from '../../../../../common/containers/query_toggle'; | ||
| import type { UsersKpiProps } from '../types'; | ||
| import { InputsModelId } from '../../../../../common/store/inputs/constants'; | ||
| import { useRefetchByRestartingSession } from '../../../../../common/components/page/use_refetch_by_session'; | ||
| import { useRefetchByRestartingSession } from '../../../../containers/use_refetch_by_session'; | ||
| import { useIsExperimentalFeatureEnabled } from '../../../../../common/hooks/use_experimental_features'; | ||
|
|
||
| enum ChartColors { | ||
|
|
@@ -79,7 +79,7 @@ const UsersKpiAuthenticationsComponent: React.FC<UsersKpiProps> = ({ | |
| endDate: to, | ||
| indexNames, | ||
| startDate: from, | ||
| skip: querySkip, | ||
| skip: querySkip || isChartEmbeddablesEnabled, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I happened to notice this was the only chart embeddables query without this condition, I think this is correct??
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the check! |
||
| }); | ||
|
|
||
| const { searchSessionId, refetchByRestartingSession } = useRefetchByRestartingSession({ | ||
|
|
||
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.
I updated this to use the
session.currentref since its the only place we're not using the ref. I think it was done like this in the first place because of the following error:Thus the variable on L62