-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] Add hook for reading/writing resolver query params #70809
[Security Solution] Add hook for reading/writing resolver query params #70809
Conversation
const queryParams: CrumbInfo = useMemo(() => { | ||
return { crumbId: '', crumbEvent: '', ...querystring.parse(urlSearch.slice(1)) }; | ||
const queryParams: any = useMemo(() => { | ||
return { uniqueCrumbId: '', uniqueCrumbEvent: '', ...querystring.parse(urlSearch.slice(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.
ℹ️ [documentLocation + 'crumbId']: '' would let you have any number of them as long as documentLocation is stable and unique across pageloads, right?
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.
oh wait nvm, I see you're already doing that above
2ba047e
to
1206284
Compare
}, | ||
[history, urlSearch] | ||
); | ||
const { pushToQueryParams } = useResolverQueryParams(documentLocation); |
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.
ℹ️ very niiice
@@ -28,6 +29,7 @@ export const Resolver = React.memo(function ({ | |||
* Used as the origin of the Resolver graph. | |||
*/ | |||
databaseDocumentID?: string; | |||
documentLocation: string; |
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.
please add doc comment
@@ -39,6 +40,7 @@ export const ResolverMap = React.memo(function ({ | |||
* Used as the origin of the Resolver graph. | |||
*/ | |||
databaseDocumentID?: string; | |||
documentLocation: string; |
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.
please add doc comment
[uniqueCrumbEventKey]: '', | ||
...querystring.parse(urlSearch.slice(1)), | ||
}; | ||
newParams.crumbId = newParams[uniqueCrumbIdKey]; |
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.
If you're setting crumbId
and crumbEvent
here, why is CrumbInfo
typed with an index type?
import { useHistory, useLocation } from 'react-router-dom'; | ||
import { CrumbInfo } from './panels/panel_content_utilities'; | ||
|
||
export function useResolverQueryParams(documentLocation: string) { |
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.
instead of passing this around, put the documentLocation
in the action dispatched by useStateSyncingActions
, then use useSelector
in this hook to get it.
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.
Are there some cool things you could do by having it passed in as an argument like this, though? Just thinking in the future stuff like "pre-focusing" a Resolver by pushing to query params (I guess if you knew its document location) before you open it?
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.
Please keep the document location ID in state instead of passing it around
import { useHistory, useLocation } from 'react-router-dom'; | ||
import { CrumbInfo } from './panels/panel_content_utilities'; | ||
|
||
export function useResolverQueryParams(documentLocation: string) { |
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.
❔ My only question here would be if this is "passable" in a URL they way we envisioned. I think we put it in this way now regardless, but maybe we should test that someone on the same Kibana instance can always open the URL and restore/rehydrate context based on this.
* A string literal describing where in the app resolver is located, | ||
* used to prevent collisions in things like query params | ||
*/ | ||
documentLocation: string; |
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.
❔ defer for now, but something like Pick<TimelineModel,'id'>
might be a little more descriptive/helpful here.
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.
...but maybe also too narrow, carry on actually.
@@ -27,8 +27,7 @@ const BetaHeader = styled(`header`)` | |||
* The two query parameters we read/write on to control which view the table presents: | |||
*/ | |||
export interface CrumbInfo { | |||
readonly crumbId: string; | |||
readonly crumbEvent: string; | |||
readonly [x: string]: string; |
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.
❔ (defer: do do not this until after merge if you decide to) At this point, you could probably just change it to a Record<string, string>
for clarity.
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.
cool
@@ -11,13 +11,15 @@ import { ResolverAction } from '../actions'; | |||
const initialState: DataState = { | |||
relatedEvents: new Map(), | |||
relatedEventsReady: new Map(), | |||
documentLocation: '', |
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.
just use undefined.
f226888
to
b4c9faf
Compare
}); | ||
it('should need to abort the request for the databaseDocumentID', () => { | ||
expect(selectors.databaseDocumentIDToFetch(state())).toBe(secondDatabaseDocumentID); | ||
}); | ||
it('should use the correct location for the first resolver', () => { | ||
expect(selectors.resolverComponentInstanceID(state())).toBe(resolverComponentInstanceID1); |
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.
❔ How does this work? It looks like we expect different things back to back from the same selector...
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.
❔ If it's because of the aborted request, that's really hard to understand here.
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.
It fetched the first resolver, and then this is it resolved. Technically the second one is in-progress
at this point and resolves in the checks below. It's the way the actions resolve for the state()
calls
@@ -177,6 +177,7 @@ export interface DataState { | |||
* The id used for the pending request, if there is one. | |||
*/ | |||
readonly pendingRequestDatabaseDocumentID?: string; | |||
readonly resolverComponentInstanceID: string; |
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.
❔ A doc comment
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.
My comments here are all ❔ . Merge.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* master: (314 commits) [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) Search across spaces (elastic#67644) ...
…t-apps-page-titles * 'master' of github.com:elastic/kibana: (88 commits) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) ... # Conflicts: # x-pack/plugins/index_management/public/application/index.tsx
* master: (72 commits) [test] Skips test preventing promotion of ES snapshot elastic#71612 [Logs UI] Remove UUID from Alert Instances (elastic#71340) [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) ...
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This pr adds a hook for use in resolver components that requires a unique key, at this point timeline.Id from siem code, to read and write to url query params without colliding if more than one instance of resolver is on the page at once.
Checklist
Delete any items that are not applicable to this PR.
For maintainers