Add relative path handling to `application.navigateToUrl` #78565

pgayvallet · 2020-09-28T10:30:45Z

Summary

Fix #78007

fix a bug causing navigateToUrl to prepend the basePath when invoking it with a non basePath-prefixed absolute path to a valid app instead of redirecting to the given absolute path.
Add relative path support to navigateToUrl, that follows the browser's <a href> resolution logic.
move the application service utilities and associated tests to distinct files
fix doc that was sometimes misleadingly using the relative path term instead of the correct absolute path one.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-09-28T10:36:19Z

@flash1293 could you confirm the PR addresses both issues from #78007?

elasticmachine · 2020-09-28T12:08:05Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-09-28T13:33:46Z

src/core/public/application/utils/parse_app_url.ts

+export const parseAppUrl = (
+  url: string,
+  basePath: IBasePath,
+  apps: Map<string, App<unknown>>,
+  currentUrl: string = window.location.href
+): ParsedAppUrl | undefined => {
+  const currentOrigin = getUrlOrigin(currentUrl);
+  if (!currentOrigin) {


The actual diff for the changes on parseAppUrl can be seen in this commit: e14858f

flash1293

Tested in Chrome and the navigation using a URL like ../app/kibana#/dashboard/edf84fe0-e1a0-11e7-b6d5-4dc382ef7f5b works fine. This fixes #77733 LGTM

rudolf

Mostly left documentation nits

src/core/public/application/types.ts

rudolf · 2020-09-30T13:11:59Z

src/core/public/application/utils/get_app_info.ts

+        ? AppNavLinkStatus.hidden
+        : AppNavLinkStatus.visible
+      : app.navLinkStatus!;
+  const { updater$, mount, ...infos } = app;


nit: I think it would be safer to pick the safe properties instead of using the rest spread so that we don't forget to remove any future sensitive properties (we'll probably get a type error in the test, but still feels more brittle).

rudolf · 2020-09-30T13:15:36Z

src/core/public/application/utils/parse_app_url.ts

+import { App, ParsedAppUrl } from '../types';
+
+/**
+ * Parse given url and return the associated app id and path if any app matches, or undefined if none do.


Suggested change

* Parse given url and return the associated app id and path if any app matches, or undefined if none do.

* Parse given URL and return the associated app id and path if any app matches, or undefined if none do.

rudolf · 2020-09-30T13:15:44Z

src/core/public/application/utils/parse_app_url.ts

+ * Parse given url and return the associated app id and path if any app matches, or undefined if none do.
+ * Input can either be:
+ *
+ * - a absolute path containing the basePath,


Suggested change

* - a absolute path containing the basePath,

* - an absolute path containing the basePath,

rudolf · 2020-09-30T13:15:57Z

src/core/public/application/utils/parse_app_url.ts

+ * - a absolute path containing the basePath,
+ *   e.g `/base-path/app/my-app/some-path`
+ *
+ * - an absolute url matching the `origin` of the kibana instance (as seen by the browser),


Suggested change

* - an absolute url matching the `origin` of the kibana instance (as seen by the browser),

* - an absolute URL matching the `origin` of the Kibana instance (as seen by the browser),

rudolf · 2020-09-30T13:20:21Z

docs/development/core/public/kibana-plugin-core-public.applicationstart.navigatetourl.md

@@ -4,9 +4,11 @@

 ## ApplicationStart.navigateToUrl() method

-Navigate to given url, which can either be an absolute url or a relative path, in a SPA friendly way when possible.
+Navigate to given url in a SPA friendly way when possible (when the url will redirect to a valid application within the current basePath).


Suggested change

Navigate to given url in a SPA friendly way when possible (when the url will redirect to a valid application within the current basePath).

Navigate to given URL in a SPA friendly way when possible (when the URL will redirect to a valid application within the current basePath).

rudolf · 2020-09-30T13:21:00Z

docs/development/core/public/kibana-plugin-core-public.applicationstart.navigatetourl.md


-If all these criteria are true for the given url: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)
+The method resolves pathnames the same way browsers do when resolving a `<a href>` value. The provided url can be: - an absolute url - an absolute path - a path relative to the current url (window.location.href)


Suggested change

The method resolves pathnames the same way browsers do when resolving a `<a href>` value. The provided url can be: - an absolute url - an absolute path - a path relative to the current url (window.location.href)

The method resolves pathnames the same way browsers do when resolving an `<a href>` value. The provided URL can be: - an absolute URL - an absolute path - a path relative to the current URL (window.location.href)

rudolf · 2020-09-30T13:21:28Z

docs/development/core/public/kibana-plugin-core-public.applicationstart.navigatetourl.md

-If all these criteria are true for the given url: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)
+The method resolves pathnames the same way browsers do when resolving a `<a href>` value. The provided url can be: - an absolute url - an absolute path - a path relative to the current url (window.location.href)
+
+If all these criteria are true for the given url: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The resolved pathname of the provided URL/path starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)


Suggested change

If all these criteria are true for the given url: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The resolved pathname of the provided URL/path starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)

If all these criteria are true for the given URL: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The resolved pathname of the provided URL/path starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)

…e-navigateToUrl

kibanamachine · 2020-10-01T14:16:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 12dd5cf

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`core`	433	438	+5

async chunks size

id	before	after	diff
`graph`	1.3MB	1.3MB	+380.0B

page load bundle size

id	before	after	diff
`core`	659.8KB	660.6KB	+832.0B
`data`	1.3MB	1.3MB	+380.0B
`ingestManager`	509.8KB	510.2KB	+380.0B
`uiActionsEnhanced`	320.7KB	321.0KB	+380.0B
total			+1.9KB

History

💚 Build #77327 succeeded 4dc3cc6

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

) * split application utilities and associated tests to distinct files * do not match app if path does not start with the basePath * add relative paths support to `navigateToUrl` * add null-check error * update generated doc * nits on doc

…79139) * split application utilities and associated tests to distinct files * do not match app if path does not start with the basePath * add relative paths support to `navigateToUrl` * add null-check error * update generated doc * nits on doc

pgayvallet added 5 commits September 28, 2020 10:20

split application utilities and associated tests to distinct files

add095a

do not match app if path does not start with the basePath

462301a

add relative paths support to navigateToUrl

e14858f

add null-check error

b0686e1

update generated doc

4dc3cc6

pgayvallet added 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 labels Sep 28, 2020

pgayvallet requested a review from flash1293 September 28, 2020 10:35

pgayvallet marked this pull request as ready for review September 28, 2020 12:08

pgayvallet requested a review from a team as a code owner September 28, 2020 12:08

pgayvallet commented Sep 28, 2020

View reviewed changes

flash1293 approved these changes Sep 30, 2020

View reviewed changes

rudolf approved these changes Sep 30, 2020

View reviewed changes

pgayvallet added 2 commits October 1, 2020 14:28

Merge remote-tracking branch 'upstream/master' into kbn-78007-relativ…

4f7ab15

…e-navigateToUrl

nits on doc

12dd5cf

pgayvallet merged commit 4ee3677 into elastic:master Oct 1, 2020

pgayvallet mentioned this pull request Oct 1, 2020

[7.x] Add relative path handling to application.navigateToUrl (#78565) #79139

Merged

pgayvallet mentioned this pull request Oct 16, 2020

Inspect saved object doesn't respect base path #80827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add relative path handling to `application.navigateToUrl` #78565

Add relative path handling to `application.navigateToUrl` #78565

pgayvallet commented Sep 28, 2020 •

edited

Loading

pgayvallet commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

pgayvallet Sep 28, 2020

flash1293 left a comment

rudolf left a comment

rudolf Sep 30, 2020

rudolf Sep 30, 2020

rudolf Sep 30, 2020

rudolf Sep 30, 2020

rudolf Sep 30, 2020

rudolf Sep 30, 2020

rudolf Sep 30, 2020

kibanamachine commented Oct 1, 2020

	* Parse given url and return the associated app id and path if any app matches, or undefined if none do.
	* Parse given URL and return the associated app id and path if any app matches, or undefined if none do.

	* - a absolute path containing the basePath,
	* - an absolute path containing the basePath,

	* - an absolute url matching the `origin` of the kibana instance (as seen by the browser),
	* - an absolute URL matching the `origin` of the Kibana instance (as seen by the browser),

	Navigate to given url in a SPA friendly way when possible (when the url will redirect to a valid application within the current basePath).
	Navigate to given URL in a SPA friendly way when possible (when the URL will redirect to a valid application within the current basePath).


		If all these criteria are true for the given url: - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app/<id>/ or any application's `appRoute` configuration)
		The method resolves pathnames the same way browsers do when resolving a `<a href>` value. The provided url can be: - an absolute url - an absolute path - a path relative to the current url (window.location.href)

Add relative path handling to application.navigateToUrl #78565

Add relative path handling to application.navigateToUrl #78565

Conversation

pgayvallet commented Sep 28, 2020 • edited Loading

Summary

Checklist

pgayvallet commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 1, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

Add relative path handling to `application.navigateToUrl` #78565

Add relative path handling to `application.navigateToUrl` #78565

pgayvallet commented Sep 28, 2020 •

edited

Loading