Skip to content
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

Fix navigateToApp logic when navigating to the current app. #80809

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Oct 16, 2020

Summary

Fix logic of navigateToApp to consider when we are performing internal navigation (navigating to the current app).

When navigating to the current app:

  • we don't execute 'should navigate' logic if the app registered an appLeaveHandler
  • we don't clear the appActionMenu that the current app may have registered

Technically, when navigating to the same app, the AppContainer does not remount the application (call mount again), meaning that navigateTo(currentApp) was causing all the handlers registered during the initial mount to be cleaned.

cc @constancecchen

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.11.0 labels Oct 16, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
core 655.1KB 655.4KB +292.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet marked this pull request as ready for review October 16, 2020 11:26
@pgayvallet pgayvallet requested a review from a team as a code owner October 16, 2020 11:26
@elasticmachine
Copy link
Contributor

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);
Copy link
Contributor

@mshustov mshustov Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it('should not remount when changing pages within app', async () => {
const { mounter, unmount } = mounters.get('app1')!;
await navigate('/app/app1/page1');
expect(mounter.mount).toHaveBeenCalledTimes(1);
// Navigating to page within app does not trigger re-render
await navigate('/app/app1/page2');
expect(mounter.mount).toHaveBeenCalledTimes(1);
expect(unmount).not.toHaveBeenCalled();
});

Copy link
Contributor Author

@pgayvallet pgayvallet Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a comment why we don't call shouldNavigate? it's not obvious that shouldNavigate calls confirmation modal under the hood.

@pgayvallet pgayvallet merged commit 949c5a5 into elastic:master Oct 16, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Oct 16, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Oct 16, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants