Fix navigateToApp logic when navigating to the current app.#80809
Fix navigateToApp logic when navigating to the current app.#80809pgayvallet merged 1 commit intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-platform (Team:Platform) |
| if (await this.shouldNavigate(overlays)) { | ||
| const currentAppId = this.currentAppId$.value; | ||
| const navigatingToSameApp = currentAppId === appId; | ||
| const shouldNavigate = navigatingToSameApp ? true : await this.shouldNavigate(overlays); |
There was a problem hiding this comment.
KP Route doesn't know about plugin internal routes, why we use it to perform navigations inside an app?
We even have a test for this
kibana/src/core/public/application/integration_tests/router.test.tsx
Lines 300 to 309 in 3094ca1
There was a problem hiding this comment.
Well, apps can/are using this API to perform intra-app navigation, with utilities such as RedirectAppLinks, and overall all usages of navigateToApp(app, { path }) or navigateToUrl('{appRoute}/{path}') used to navigate within your own app. it was never meant to exclusively be used to execute app-to-app navigation.
This test is valid: it shouldn't remount the app. When navigating to the same prefix as the currently mounted app, it should basically just history.push to let the underlying app router catch the pushState event itself via its ScopedHistory instance.
The issue with intra-app link was only introduced when we added logic to navigateToApp before it actually performs the call to history.push (when we added leaveHandler, then quite recently, setAppActionMenu)
There was a problem hiding this comment.
could you add a comment why we don't call shouldNavigate? it's not obvious that shouldNavigate calls confirmation modal under the hood.
* master: (51 commits) [Discover] Unskip flaky test (elastic#80670) Fix security solution template label (elastic#80754) [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721) Moving loader to logo in header, add a slight 250ms pause (elastic#78879) [Security Solution][Cases] Fix bug with case connectors (elastic#80642) Update known-plugins.asciidoc (elastic#75388) [Lens] Add median operation (elastic#79453) Fix navigateToApp logic when navigating to the current app. (elastic#80809) [Visualizations] Fix bad color mapping with multiple split series (elastic#80801) [ILM] Add esErrorHandler for the new es js client (elastic#80302) Fix codeowners (elastic#80826) skip flaky suite (elastic#79463) [Timelion] Remove kui usage (elastic#80287) [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779) [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579) Allow the default space to be accessed via `/s/default` (elastic#77109) Add script to identify plugin dependencies for TS project references migration (elastic#80463) [Search] Client side session service (elastic#76889) feat: 🎸 add separator for different context menu groups (elastic#80498) Lazy load reporting (elastic#80492) ...
Summary
Fix logic of
navigateToAppto consider when we are performing internal navigation (navigating to the current app).When navigating to the current app:
appLeaveHandlerappActionMenuthat the current app may have registeredTechnically, when navigating to the same app, the
AppContainerdoes not remount the application (callmountagain), meaning thatnavigateTo(currentApp)was causing all the handlers registered during the initialmountto be cleaned.cc @constancecchen