-
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
[ML] Anomaly Explorer: Fixes handling of job group IDs when opening from dashboard panels #203224
Merged
Merged
Changes from 38 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
a83459d
useGlobalUrlState service
rbrtj 8a063ac
remove set_charts_data_loading explorer action
rbrtj 6fc0569
anomaly explorer refactor
rbrtj bb30891
remove unused ExplorerPage props
rbrtj 50172b8
Badges refactor
rbrtj 8885e91
improve memoization for useUrlStateService
rbrtj c23276f
pass groups to dashboard embeddable instead of individual jobs
rbrtj 334f736
global state interface comment
rbrtj 7b8dd0e
remove invalidTimeRangeError
rbrtj 91facf8
remove unused import
rbrtj 3612c08
Merge branch 'main' into anomaly-explorer-enhancements
rbrtj 0727a30
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine 654ef68
fix eslint issues
rbrtj 30c841c
fix apply time range on job selection
rbrtj f4ca91c
remove unused translations
rbrtj 70acbcf
Merge remote-tracking branch 'upstream/main' into anomaly-explorer-en…
rbrtj e20f30a
remove file after merge conflicts
rbrtj 2f673e6
support replace state in global state
rbrtj 752cfcb
GroupWithTimeRange type
rbrtj b926fc4
anomaly explorer common state service - preserving reference while ex…
rbrtj 0debdc0
improve job not found error message
rbrtj 7584684
formatJobNotFound extracted to a function
rbrtj 692d97b
fix navigation from overview panel to anomaly explorer
rbrtj e0c1de2
Merge remote-tracking branch 'upstream/main' into anomaly-explorer-en…
rbrtj d18d929
fix action for adding anomaly charts to a dashboard
rbrtj a257c9f
remove unused translations
rbrtj 22a85e0
move id badges test to tsx && fix tests
rbrtj a4c8b49
move new selection id badges test to tsx && fix tests
rbrtj 844adfa
timeseries explorer selected jobs fix
rbrtj b687d6f
basic unit tests for usePageUrlState and useGlobalUrlState hooks
rbrtj 7894672
rename GroupWithTimerange for consistency && share TimeRange type
rbrtj f42b979
use set to remove duplicates from jobsInSelectedGroups list
rbrtj f725b4d
remove unused smv jobs from anomaly explorer common state
rbrtj e2e5c00
remove unused explorer constants
rbrtj 74778d6
unit tests for getIndexPattern and getMergedGroupsAndJobsIds
rbrtj ab69786
new selection id badges tests type fix
rbrtj cdc4801
Merge branch 'main' into anomaly-explorer-enhancements
rbrtj 9c5851e
Merge branch 'main' into anomaly-explorer-enhancements
rbrtj 7df5acb
url state hooks test improvements
rbrtj e854c74
use job selection hook
rbrtj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import type { Observable } from 'rxjs'; | |
import { BehaviorSubject } from 'rxjs'; | ||
import { distinctUntilChanged } from 'rxjs'; | ||
import { isPopulatedObject } from '@kbn/ml-is-populated-object'; | ||
import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect'; | ||
|
||
export interface Dictionary<TValue> { | ||
[id: string]: TValue; | ||
|
@@ -211,43 +212,42 @@ export const useUrlState = ( | |
/** | ||
* Service for managing URL state of particular page. | ||
*/ | ||
export class PageUrlStateService<T> { | ||
private _pageUrlState$ = new BehaviorSubject<T | null>(null); | ||
private _pageUrlStateCallback: ((update: Partial<T>, replaceState?: boolean) => void) | null = | ||
null; | ||
export class UrlStateService<T> { | ||
private _urlState$ = new BehaviorSubject<T | null>(null); | ||
private _urlStateCallback: ((update: Partial<T>, replaceState?: boolean) => void) | null = null; | ||
|
||
/** | ||
* Provides updates for the page URL state. | ||
*/ | ||
public getPageUrlState$(): Observable<T> { | ||
return this._pageUrlState$.pipe(distinctUntilChanged(isEqual)); | ||
public getUrlState$(): Observable<T> { | ||
return this._urlState$.pipe(distinctUntilChanged(isEqual)); | ||
} | ||
|
||
public getPageUrlState(): T | null { | ||
return this._pageUrlState$.getValue(); | ||
public getUrlState(): T | null { | ||
return this._urlState$.getValue(); | ||
} | ||
|
||
public updateUrlState(update: Partial<T>, replaceState?: boolean): void { | ||
if (!this._pageUrlStateCallback) { | ||
if (!this._urlStateCallback) { | ||
throw new Error('Callback has not been initialized.'); | ||
} | ||
this._pageUrlStateCallback(update, replaceState); | ||
this._urlStateCallback(update, replaceState); | ||
} | ||
|
||
/** | ||
* Populates internal subject with currently active state. | ||
* @param currentState | ||
*/ | ||
public setCurrentState(currentState: T): void { | ||
this._pageUrlState$.next(currentState); | ||
this._urlState$.next(currentState); | ||
} | ||
|
||
/** | ||
* Sets the callback for the state update. | ||
* @param callback | ||
*/ | ||
public setUpdateCallback(callback: (update: Partial<T>, replaceState?: boolean) => void): void { | ||
this._pageUrlStateCallback = callback; | ||
this._urlStateCallback = callback; | ||
} | ||
} | ||
|
||
|
@@ -256,32 +256,53 @@ export interface PageUrlState { | |
pageUrlState: object; | ||
} | ||
|
||
/** | ||
* Hook for managing the URL state of the page. | ||
*/ | ||
export const usePageUrlState = <T extends PageUrlState>( | ||
pageKey: T['pageKey'], | ||
defaultState?: T['pageUrlState'] | ||
): [ | ||
T['pageUrlState'], | ||
(update: Partial<T['pageUrlState']>, replaceState?: boolean) => void, | ||
PageUrlStateService<T['pageUrlState']> | ||
] => { | ||
const [appState, setAppState] = useUrlState('_a'); | ||
const pageState = appState?.[pageKey]; | ||
interface AppStateOptions<T> { | ||
pageKey: string; | ||
defaultState?: T; | ||
} | ||
|
||
const setCallback = useRef<typeof setAppState>(); | ||
interface GlobalStateOptions<T> { | ||
defaultState?: T; | ||
} | ||
|
||
type UrlStateOptions<K extends Accessor, T> = K extends '_a' | ||
? AppStateOptions<T> | ||
: GlobalStateOptions<T>; | ||
|
||
function isAppStateOptions<T>( | ||
_stateKey: Accessor, | ||
options: Partial<AppStateOptions<T>> | ||
): options is AppStateOptions<T> { | ||
return 'pageKey' in options; | ||
} | ||
|
||
export const useUrlStateService = <K extends Accessor, T>( | ||
stateKey: K, | ||
options: UrlStateOptions<K, T> | ||
): [T, (update: Partial<T>, replaceState?: boolean) => void, UrlStateService<T>] => { | ||
const optionsRef = useRef(options); | ||
|
||
useDeepCompareEffect(() => { | ||
optionsRef.current = options; | ||
}, [options]); | ||
|
||
const [state, setState] = useUrlState(stateKey); | ||
const urlState = isAppStateOptions<T>(stateKey, optionsRef.current) | ||
? state?.[optionsRef.current.pageKey] | ||
: state; | ||
|
||
const setCallback = useRef<typeof setState>(); | ||
|
||
useEffect(() => { | ||
setCallback.current = setAppState; | ||
}, [setAppState]); | ||
setCallback.current = setState; | ||
}, [setState]); | ||
|
||
const prevPageState = useRef<T['pageUrlState'] | undefined>(); | ||
const prevPageState = useRef<T | undefined>(); | ||
|
||
const resultPageState: T['pageUrlState'] = useMemo(() => { | ||
const resultState: T = useMemo(() => { | ||
const result = { | ||
...(defaultState ?? {}), | ||
...(pageState ?? {}), | ||
...(optionsRef.current.defaultState ?? {}), | ||
...(urlState ?? {}), | ||
}; | ||
|
||
if (isEqual(result, prevPageState.current)) { | ||
|
@@ -300,38 +321,82 @@ export const usePageUrlState = <T extends PageUrlState>( | |
prevPageState.current = result; | ||
|
||
return result; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [pageState]); | ||
}, [urlState]); | ||
|
||
const onStateUpdate = useCallback( | ||
(update: Partial<T['pageUrlState']>, replaceState?: boolean) => { | ||
(update: Partial<T>, replaceState?: boolean) => { | ||
if (!setCallback?.current) { | ||
throw new Error('Callback for URL state update has not been initialized.'); | ||
} | ||
|
||
setCallback.current( | ||
pageKey, | ||
{ | ||
...resultPageState, | ||
...update, | ||
}, | ||
replaceState | ||
); | ||
if (isAppStateOptions<T>(stateKey, optionsRef.current)) { | ||
setCallback.current( | ||
optionsRef.current.pageKey, | ||
{ | ||
...resultState, | ||
...update, | ||
}, | ||
replaceState | ||
); | ||
} else { | ||
setCallback.current({ ...resultState, ...update }, replaceState); | ||
} | ||
}, | ||
[pageKey, resultPageState] | ||
[stateKey, resultState] | ||
); | ||
|
||
const pageUrlStateService = useMemo(() => new PageUrlStateService<T['pageUrlState']>(), []); | ||
const urlStateService = useMemo(() => new UrlStateService<T>(), []); | ||
|
||
useEffect( | ||
function updatePageUrlService() { | ||
pageUrlStateService.setCurrentState(resultPageState); | ||
pageUrlStateService.setUpdateCallback(onStateUpdate); | ||
function updateUrlStateService() { | ||
urlStateService.setCurrentState(resultState); | ||
urlStateService.setUpdateCallback(onStateUpdate); | ||
}, | ||
[pageUrlStateService, onStateUpdate, resultPageState] | ||
[urlStateService, onStateUpdate, resultState] | ||
); | ||
|
||
return useMemo(() => { | ||
return [resultPageState, onStateUpdate, pageUrlStateService]; | ||
}, [resultPageState, onStateUpdate, pageUrlStateService]); | ||
return useMemo( | ||
() => [resultState, onStateUpdate, urlStateService], | ||
[resultState, onStateUpdate, urlStateService] | ||
); | ||
}; | ||
|
||
/** | ||
* Hook for managing the URL state of the page. | ||
*/ | ||
export const usePageUrlState = <T extends PageUrlState>( | ||
pageKey: T['pageKey'], | ||
defaultState?: T['pageUrlState'] | ||
): [ | ||
T['pageUrlState'], | ||
(update: Partial<T['pageUrlState']>, replaceState?: boolean) => void, | ||
UrlStateService<T['pageUrlState']> | ||
] => { | ||
return useUrlStateService<'_a', T['pageUrlState']>('_a', { pageKey, defaultState }); | ||
}; | ||
|
||
/** | ||
* Global state type, to add more state types, add them here | ||
*/ | ||
|
||
export interface GlobalState { | ||
ml: { | ||
jobIds: string[]; | ||
}; | ||
time?: { | ||
from: string; | ||
to: string; | ||
}; | ||
} | ||
|
||
/** | ||
* Hook for managing the global URL state. | ||
*/ | ||
export const useGlobalUrlState = ( | ||
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. It would be great if you could add some basic tests to 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. Done in: #b687d6f |
||
defaultState?: GlobalState | ||
): [ | ||
GlobalState, | ||
(update: Partial<GlobalState>, replaceState?: boolean) => void, | ||
UrlStateService<GlobalState> | ||
] => { | ||
return useUrlStateService<'_g', GlobalState>('_g', { defaultState }); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
what is the motivation of using components in tests? did yo try rendering the hook itself?
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.
good point, I just aligned with the existing tests, but rendering the test component seems redundant, updated in: #7df5acb