[EDR Workflows][Artifacts Regrouping] Re-organize all artifacts to a single page with separate tabs#257001
Conversation
|
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
denar50
left a comment
There was a problem hiding this comment.
Code review only. LGTM for files owned by the Detection Engine.
…nto task/regroup-artifacts
gergoabraham
left a comment
There was a problem hiding this comment.
hey hey, nice work!
tested it out together with #257983, by merging them locally on top of each other, and happy to share, that, apart from a trivial merge conflict, they work together without any issue 👍
also tested your PR on its own, and works nice. 👏 I got one finding: if a user doesn't have any artifact access, the 'Artifacts' link is still shown, both in Classic and Security views
Screen.Recording.2026-04-02.at.13.43.20.mov
| canReadTrustedDevices || | ||
| canReadEventFilters || | ||
| canReadHostIsolationExceptions || | ||
| canReadBlocklist; |
There was a problem hiding this comment.
+1 on making experimentalFeatures a required parameter, it doesn't seem like an optional thing for calculating the links.
| core: CoreStart, | ||
| plugins: StartPlugins | ||
| plugins: StartPlugins, | ||
| params?: GetFilteredLinksParams |
There was a problem hiding this comment.
this also should be mandatory. and experimentalFeatures up in line 56 as well.
nit: also, GetFilteredLinksParams doesn't seem to be very useful, maybe it'd be enough to simply have experimentalFeatures as the 3rd parameter here. same for getManagementFilteredLinks()
| export interface GetManagementFilteredLinksParams { | ||
| experimentalFeatures?: Pick< | ||
| ExperimentalFeatures, | ||
| 'endpointExceptionsMovedUnderManagement' | 'trustedDevices' | ||
| >; | ||
| } |
There was a problem hiding this comment.
nit: same as in my other comment above: this interface seems superfluous. it's perfectly fine to just pass the experimentalFeatures as the 3rd parameter, without wrapping it inside an interface, or without picking the 2 parameters we actually use.
Hi @PhilippeOberti, the The Let me know if that makes sense or if there were any other questions. |
…nto task/regroup-artifacts
szwarckonrad
left a comment
There was a problem hiding this comment.
Good job on this one, thanks for addressing all the review comments! The RBAC-aware artifact link resolution, telemetry preservation, and the cleanup of old container files all look solid. 👍
A few minor non-blocking observations for your consideration (can be addressed in a follow-up if you'd like):
-
getActiveTabFromPathnamehardcoded fallback — the function still falls back toAdministrationSubTab.trustedAppswhen the pathname doesn't match any tab. It would be slightly more defensive to fall back tovisibleTabs[0]instead, so it always lands on a tab the user can actually see. -
getFirstAllowedArtifactPathfinal fallback — when no artifact is accessible, the function returnsgetTrustedAppsListPath(). This should be unreachable sincecanReadAnyArtifactwould be false and the link excluded, but a small comment documenting that assumption would help future readers. -
Redundant condition in
canReadAnyArtifact—(showHostIsolationExceptions && canReadHostIsolationExceptions)— the&& canReadHostIsolationExceptionsis redundant sinceshowHostIsolationExceptionsalready factors incanReadHostIsolationExceptions. Not wrong, just unnecessary.
…nto task/regroup-artifacts
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
History
|
| core: CoreStart, | ||
| plugins: StartPlugins | ||
| plugins: StartPlugins, | ||
| experimentalFeatures: ExperimentalFeatures |
There was a problem hiding this comment.
probably want to coordinate with @ashokaditya , he's making very similar changes to this exact function for this same reason https://github.com/elastic/kibana/pull/260429/files#diff-d7b43749c559b9c4036c0e2cc6d922ea086a41ed1ea89097fb37b5372166b9e6R58 experimentalFeatures is an optional param in that branch, so whoever ends up merging 2nd should be careful.
There was a problem hiding this comment.
Sorry I just approved the PR! Thanks for pointing this out @kqualters-elastic I missed that
PhilippeOberti
left a comment
There was a problem hiding this comment.
LGTM and thanks for the explanation on the Timeline hook!
…#261845) The artifacts regrouping PR (#257001) reorganized all endpoint artifacts into a unified page with tabs. This changed the page structure so that `header-page-title` now belongs to the unified "Artifacts" page and is always present, regardless of whether individual tabs have entries. Previously, `trusted_apps_list.ts` was skipped (flaky). Our PR #261180 unskipped it after fixing the underlying flakiness — but by that point the regrouping had already landed, making the `header-page-title` assertions invalid. The test immediately started failing on merge. `artifact_entries_list.ts` has the same `header-page-title` assertions for all artifact types (trusted apps, event filters, blocklist, host isolation exceptions) — these would fail for the same reason once hit. **Fix:** Replace `missingOrFail('header-page-title')` with `existOrFail('*-emptyState')` in both test files. The empty state test subject is the correct indicator that a tab has no entries. Closes #261827 Closes #261829 Closes #261833 Closes #261849 Closes #261850
Summary
Screenshots