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

[Bug fix] Update nav link when it belongs to the same plugin #58008

Merged
merged 9 commits into from
Feb 24, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export const DashboardConstants = {
CREATE_NEW_DASHBOARD_URL: '/dashboard',
ADD_EMBEDDABLE_ID: 'addEmbeddableId',
ADD_EMBEDDABLE_TYPE: 'addEmbeddableType',
DASHBOARDS_ID: 'dashboards',
DASHBOARD_ID: 'dashboard',
};

export function createDashboardEditUrl(id: string) {
Expand Down
15 changes: 11 additions & 4 deletions src/legacy/core_plugins/kibana/public/dashboard/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,14 @@ export class DashboardPlugin implements Plugin {
);
const { appMounted, appUnMounted, stop: stopUrlTracker } = createKbnUrlTracker({
baseUrl: core.http.basePath.prepend('/app/kibana'),
defaultSubUrl: '#/dashboards',
defaultSubUrl: `#${DashboardConstants.LANDING_PAGE_PATH}`,
shouldTrackUrlUpdate: pathname => {
const targetAppName = pathname.split('/')[1];
return (
targetAppName === DashboardConstants.DASHBOARDS_ID ||
targetAppName === DashboardConstants.DASHBOARD_ID
);
},
storageKey: 'lastUrl:dashboard',
navLinkUpdater$: this.appStateUpdater,
toastNotifications: core.notifications.toasts,
Expand Down Expand Up @@ -150,15 +157,15 @@ export class DashboardPlugin implements Plugin {
};
kibanaLegacy.registerLegacyApp({
...app,
id: 'dashboard',
id: DashboardConstants.DASHBOARD_ID,
// only register the updater in once app, otherwise all updates would happen twice
updater$: this.appStateUpdater.asObservable(),
navLinkId: 'kibana:dashboard',
});
kibanaLegacy.registerLegacyApp({ ...app, id: 'dashboards' });
kibanaLegacy.registerLegacyApp({ ...app, id: DashboardConstants.DASHBOARDS_ID });

home.featureCatalogue.register({
id: 'dashboard',
id: DashboardConstants.DASHBOARD_ID,
title: i18n.translate('kbn.dashboard.featureCatalogue.dashboardTitle', {
defaultMessage: 'Dashboard',
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('kbnUrlTracker', () => {
let navLinkUpdaterSubject: BehaviorSubject<(app: AppBase) => { activeUrl?: string } | undefined>;
let toastService: jest.Mocked<ToastsSetup>;

function createTracker() {
function createTracker(shouldTrackUrlUpdate?: (pathname: string) => boolean) {
urlTracker = createKbnUrlTracker({
baseUrl: '/app/test',
defaultSubUrl: '#/start',
Expand All @@ -57,6 +57,7 @@ describe('kbnUrlTracker', () => {
],
navLinkUpdater$: navLinkUpdaterSubject,
toastNotifications: toastService,
shouldTrackUrlUpdate,
});
}

Expand All @@ -82,44 +83,44 @@ describe('kbnUrlTracker', () => {
});

test('set nav link to session storage value if defined', () => {
storage.setItem('storageKey', '#/deep/path');
storage.setItem('storageKey', '#/start/deep/path');
createTracker();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path');
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path');
});

test('set nav link to default if app gets mounted', () => {
storage.setItem('storageKey', '#/deep/path');
storage.setItem('storageKey', '#/start/deep/path');
createTracker();
urlTracker.appMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start');
});

test('keep nav link to default if path gets changed while app mounted', () => {
storage.setItem('storageKey', '#/deep/path');
storage.setItem('storageKey', '#/start/deep/path');
createTracker();
urlTracker.appMounted();
history.push('/deep/path/2');
history.push('/start/deep/path/2');
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start');
});

test('change nav link to last visited url within app after unmount', () => {
createTracker();
urlTracker.appMounted();
history.push('/deep/path/2');
history.push('/deep/path/3');
history.push('/start/deep/path/2');
history.push('/start/deep/path/3');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/3');
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/3');
});

test('unhash all urls that are recorded while app is mounted', () => {
(unhashUrl as jest.Mock).mockImplementation(x => x + '?unhashed');
createTracker();
urlTracker.appMounted();
history.push('/deep/path/2');
history.push('/deep/path/3');
history.push('/start/deep/path/2');
history.push('/start/deep/path/3');
urlTracker.appUnMounted();
expect(unhashUrl).toHaveBeenCalledTimes(2);
expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/3?unhashed');
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/3?unhashed');
});

test('show warning and use hashed url if unhashing does not work', () => {
Expand All @@ -128,17 +129,17 @@ describe('kbnUrlTracker', () => {
});
createTracker();
urlTracker.appMounted();
history.push('/deep/path/2');
history.push('/start/deep/path/2');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/deep/path/2');
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path/2');
expect(toastService.addDanger).toHaveBeenCalledWith('unhash broke');
});

test('change nav link back to default if app gets mounted again', () => {
createTracker();
urlTracker.appMounted();
history.push('/deep/path/2');
history.push('/deep/path/3');
history.push('/start/deep/path/2');
history.push('/start/deep/path/3');
urlTracker.appUnMounted();
urlTracker.appMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start');
Expand All @@ -151,11 +152,11 @@ describe('kbnUrlTracker', () => {
});

test('update state param without overwriting rest of the url when app is not mounted', () => {
storage.setItem('storageKey', '#/deep/path?extrastate=1');
storage.setItem('storageKey', '#/start/deep/path?extrastate=1');
createTracker();
state1Subject.next({ key1: 'abc' });
expect(getActiveNavLinkUrl()).toMatchInlineSnapshot(
`"/app/test#/deep/path?extrastate=1&state1=(key1:abc)"`
`"/app/test#/start/deep/path?extrastate=1&state1=(key1:abc)"`
);
});

Expand Down Expand Up @@ -184,7 +185,45 @@ describe('kbnUrlTracker', () => {

test('set url to storage when setActiveUrl was called', () => {
createTracker();
urlTracker.setActiveUrl('/deep/path/4');
expect(storage.getItem('storageKey')).toEqual('#/deep/path/4');
urlTracker.setActiveUrl('/start/deep/path/4');
expect(storage.getItem('storageKey')).toEqual('#/start/deep/path/4');
});

describe('shouldTrackUrlUpdate', () => {
test('change nav link when shouldTrackUrlUpdate is not overridden', () => {
storage.setItem('storageKey', '#/start/deep/path');
createTracker();
urlTracker.appMounted();
history.push('/start/path');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/path');
});

test('not change nav link when shouldTrackUrlUpdate is not overridden', () => {
storage.setItem('storageKey', '#/start/deep/path');
createTracker();
urlTracker.appMounted();
history.push('/setup/path/2');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path');
});

test('change nav link when shouldTrackUrlUpdate is overridden', () => {
storage.setItem('storageKey', '#/start/deep/path');
createTracker(() => true);
urlTracker.appMounted();
history.push('/setup/path/2');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/setup/path/2');
});

test('not change nav link when shouldTrackUrlUpdate is overridden', () => {
storage.setItem('storageKey', '#/start/deep/path');
createTracker(() => false);
urlTracker.appMounted();
history.push('/setup/path/2');
urlTracker.appUnMounted();
expect(getActiveNavLinkUrl()).toEqual('/app/test#/start/deep/path');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ export function createKbnUrlTracker({
toastNotifications,
history,
storage,
shouldTrackUrlUpdate = pathname => {
const currentAppName = defaultSubUrl.slice(2); // cut hash and slash symbols
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me realize that kbn_url_tracker works only for current Kibana's url format and hash router. Will need to revisit when / if someone wants to use it in np app with browser router

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, I didn't want to introduce too many degrees on freedom on this helper just yet to keep the complexity low. Let's generalize once there are use cases for this

const targetAppName = pathname.split('/')[1];

return currentAppName === targetAppName;
},
}: {
/**
* Base url of the current app. This will be used as a prefix for the
Expand All @@ -82,7 +88,7 @@ export function createKbnUrlTracker({
stateUpdate$: Observable<unknown>;
}>;
/**
* Key used to store the current sub url in session storage. This key should only be used for one active url tracker at any given ntime.
* Key used to store the current sub url in session storage. This key should only be used for one active url tracker at any given time.
*/
storageKey: string;
/**
Expand All @@ -101,6 +107,13 @@ export function createKbnUrlTracker({
* Storage object to use to persist currently active url. If this isn't provided, the browser wide session storage instance will be used.
*/
storage?: Storage;
/**
* Checks if pathname belongs to current app. It's used in history listener to define whether it's necessary to set pathname as active url or not.
* The default implementation compares the app name to the first part of pathname. Consumers can override this function for more complex cases.
*
* @param {string} pathname A location's pathname which comes to history listener
*/
shouldTrackUrlUpdate?: (pathname: string) => boolean;
}): KbnUrlTracker {
const historyInstance = history || createHashHistory();
const storageInstance = storage || sessionStorage;
Expand Down Expand Up @@ -148,7 +161,9 @@ export function createKbnUrlTracker({
unsubscribe();
// track current hash when within app
unsubscribeURLHistory = historyInstance.listen(location => {
setActiveUrl(location.pathname + location.search);
if (shouldTrackUrlUpdate(location.pathname)) {
setActiveUrl(location.pathname + location.search);
}
});
}

Expand Down