[Security solution] Template wrapper fixes, route changes.#212808
Draft
kqualters-elastic wants to merge 20 commits intoelastic:mainfrom
Draft
[Security solution] Template wrapper fixes, route changes.#212808kqualters-elastic wants to merge 20 commits intoelastic:mainfrom
kqualters-elastic wants to merge 20 commits intoelastic:mainfrom
Conversation
kqualters-elastic
force-pushed
the
fix-wrappers-via-routes
branch
from
243d765 to
ed80b7b
Compare
kqualters-elastic
changed the title
kqualters-elastic
added
v9.1.0
release_note:skip
Contributor
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
kqualters-elastic
requested review from
CAWilson94,
dhurley14,
gergoabraham,
jkelas and
paul-tavares
| <ApplicationUsageTrackingProvider> | ||
| {children ?? | ||
| (subPluginRoutes && <AppRoutes subPluginRoutes={subPluginRoutes} services={services} />)} | ||
| <AppRoutes subPluginRoutes={subPluginRoutes} services={services} /> |
Contributor
Author
There was a problem hiding this comment.
TODO: Keep this change, render
const { ManagementSettings } = await this.lazyAssistantSettingsManagement();
in a different way
jkelas
reviewed
Apr 3, 2025
| path: RULES_LANDING_PATH, | ||
| component: RulesLandingPage, | ||
| }, | ||
| // { |
Contributor
There was a problem hiding this comment.
Please remove commented code
| ]; | ||
|
|
||
| const RulesContainerComponent: React.FC = () => { | ||
| const RulesContainerComponent: React.FC = (props) => { |
Contributor
There was a problem hiding this comment.
Is this PR still maintained? I can see no actions for 2 months.
Contributor
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
|
seanrathier
approved these changes
Apr 22, 2025
Contributor
seanrathier
left a comment
seanrathier
left a comment
There was a problem hiding this comment.
Approving: x-pack/solutions/security/plugins/security_solution/public/cloud_security_posture/routes.tsx
lgestc
approved these changes
Apr 22, 2025
Contributor
There was a problem hiding this comment.
clicked around this a bit as well, looking good:) I like the changes though threat intelligence plugin seems to fail
if you can hold this off for a while, we might be able to remove some of the code that allows reconfiguring props on the template for threat intelligence plugin
lgestc
reviewed
Apr 22, 2025
Contributor
lgestc
left a comment
lgestc
left a comment
There was a problem hiding this comment.
other than failing tests looks alright and seems to improve things a lot, thanks
Contributor
Hey there! Is this PR currently on hold for this? |
Contributor
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Summary
This pr is an attempt at fixing several performance issues in security solution via reworking routes and page templates. In short, the idea behind showTimeline: boolean in security solution sub plugins is currently essentially pointless, as the PluginTemplateWrapper component is destroyed and re-rendered in full any time a user navigates to a new page. With timeline now being based around discover, this results in a whole lot of needlessly duplicated bootstrapping logic running on every page change. This pr changes it so that PluginTemplateWrapper is rendered only once on initially navigating to the security solution, and destroyed when leaving. We probably won't even need showTimeline anymore, but can revisit this later. Navigating from page to page should be significantly faster with this change. Originally this came about from an investigation into an SDH, where I saw a user who had a large amount of fields, try to navigate from 'sub plugin' to 'sub plugin' in security solution rapidly, and the browser would block for upwards of 10 seconds on some pages. Navigation is definitely faster, both in terms of less renders overall and in less renders that block the page for extended periods of time, and nothing should have changed from a user experience point of view. Will update this description with more specific numbers soon. Changes unrelated to the routes and page/plugin wrappers in security solution were mostly found from a combination of why-did-you-render and react devtools.
Checklist