Skip to content

Commit

Permalink
Change the mechanism for differentiating between internal and externa…
Browse files Browse the repository at this point in the history
…l links
  • Loading branch information
Kerry350 committed Mar 9, 2020
1 parent e3640c2 commit c942eac
Show file tree
Hide file tree
Showing 14 changed files with 107 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ const getUptimeLink = (logItem: LogEntriesItem): LinkDescriptor | undefined => {
return undefined;
}
return {
app: 'uptime',
hash: '/',
search: {
search: `${searchExpressions.join(' or ')}`,
Expand Down Expand Up @@ -140,6 +141,7 @@ const getAPMLink = (logItem: LogEntriesItem): LinkDescriptor | undefined => {
: { rangeFrom: 'now-1y', rangeTo: 'now' };

return {
app: 'apm',
hash: getTraceUrl({ traceId: traceIdEntry.value, rangeFrom, rangeTo }),
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ export const MetricsExplorerChartContextMenu: React.FC<Props> = ({

const nodeType = source && options.groupBy && fieldToNodeType(source, options.groupBy);
const nodeDetailLinkProps = useLinkProps({
app: 'metrics',
...(nodeType ? createNodeDetailLink(nodeType, series.id, timeRange.from, timeRange.to) : {}),
});
const tsvbLinkProps = useLinkProps({
Expand All @@ -120,7 +121,7 @@ export const MetricsExplorerChartContextMenu: React.FC<Props> = ({
values: { name: nodeType },
}),
icon: 'metricsApp',
...nodeDetailLinkProps,
...(nodeType ? nodeDetailLinkProps : {}),
'data-test-subj': 'metricsExplorerAction-ViewNodeMetrics',
},
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ import { Route } from 'react-router-dom';

import { euiStyled } from '../../../../observability/public';
import { useLinkProps } from '../../hooks/use_link_props';
import { LinkDescriptor } from '../../hooks/use_link_props';

interface TabConfiguration {
interface TabConfig {
title: string | React.ReactNode;
path: string;
}

type TabConfiguration = TabConfig & LinkDescriptor;

interface RoutedTabsProps {
tabs: TabConfiguration[];
}
Expand All @@ -26,22 +28,22 @@ export const RoutedTabs = ({ tabs }: RoutedTabsProps) => {
return (
<EuiTabs display="condensed">
{tabs.map(tab => {
return <Tab key={`${tab.path}-${tab.title}`} {...tab} />;
return <Tab key={`${tab.pathname}-${tab.title}`} {...tab} />;
})}
</EuiTabs>
);
};

const Tab = ({ title, path }: TabConfiguration) => {
const linkProps = useLinkProps({ pathname: path });
const Tab = ({ title, pathname, app }: TabConfiguration) => {
const linkProps = useLinkProps({ app, pathname });
return (
<Route
path={path}
path={pathname}
children={({ match, history }) => {
return (
<TabContainer className="euiTab">
{/* eslint-disable-next-line @elastic/eui/href-or-on-click */}
<EuiLink {...linkProps} data-test-subj={`infrastructureNavLink_${path}`}>
<EuiLink {...linkProps} data-test-subj={`infrastructureNavLink_${pathname}`}>
<EuiTab onClick={noop} isSelected={match !== null}>
{title}
</EuiTab>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ import { useLinkProps } from '../../hooks/use_link_props';
interface ViewSourceConfigurationButtonProps {
'data-test-subj'?: string;
children: React.ReactNode;
app: 'logs' | 'metrics';
}

export const ViewSourceConfigurationButton = ({
'data-test-subj': dataTestSubj,
app,
children,
}: ViewSourceConfigurationButtonProps) => {
const linkProps = useLinkProps({ pathname: '/settings' });
const linkProps = useLinkProps({ app, pathname: '/settings' });
return (
<EuiButton data-test-subj={dataTestSubj} color="primary" {...linkProps}>
{children}
Expand Down
45 changes: 30 additions & 15 deletions x-pack/plugins/infra/public/hooks/use_link_props.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,32 @@
*/

import { encode } from 'rison-node';
import { createMemoryHistory } from 'history';
import { createMemoryHistory, LocationDescriptorObject } from 'history';
import { renderHook } from '@testing-library/react-hooks';
import React from 'react';
import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public';
import { HistoryContext } from '../utils/history_context';
import { coreMock } from 'src/core/public/mocks';
import { useLinkProps } from './use_link_props';
import { useLinkProps, LinkDescriptor } from './use_link_props';

const PREFIX = '/test-basepath/s/test-space/app/';

const coreStartMock = coreMock.createStart();

coreStartMock.application.getUrlForApp.mockImplementation((app, options) => {
return `/test-basepath/s/test-space/app/${app}${options?.path}`;
return `${PREFIX}${app}${options?.path}`;
});

// Note: Memory history doesn't support baseName
const INTERNAL_APP = 'metrics';

// Note: Memory history doesn't support basename,
// we'll work around this by re-assigning 'createHref' so that
// it includes a basename, this then acts as our browserHistory instance would.
const history = createMemoryHistory();
const originalCreateHref = history.createHref;
history.createHref = (location: LocationDescriptorObject): string => {
return `${PREFIX}${INTERNAL_APP}${originalCreateHref.call(history, location)}`;
};

const ProviderWrapper: React.FC = ({ children }) => {
return (
Expand All @@ -30,33 +40,37 @@ const ProviderWrapper: React.FC = ({ children }) => {
);
};

const renderUseLinkPropsHook = (props?: any) => {
return renderHook(() => useLinkProps(props), { wrapper: ProviderWrapper });
const renderUseLinkPropsHook = (props?: Partial<LinkDescriptor>) => {
return renderHook(() => useLinkProps({ app: INTERNAL_APP, ...props }), {
wrapper: ProviderWrapper,
});
};
describe('useLinkProps hook', () => {
describe('Handles internal linking', () => {
it('Provides the correct baseline props', () => {
const { result } = renderUseLinkPropsHook({});
expect(result.current.href).toBe('/');
const { result } = renderUseLinkPropsHook({ pathname: '/' });
expect(result.current.href).toBe('/test-basepath/s/test-space/app/metrics/');
expect(result.current.onClick).toBeDefined();
});

it('Provides the correct props with options', () => {
const { result } = renderUseLinkPropsHook({
pathname: 'inventory',
pathname: '/inventory',
search: {
type: 'host',
id: 'some-id',
count: '12345',
},
});
expect(result.current.href).toBe('/inventory?type=host&id=some-id&count=12345');
expect(result.current.href).toBe(
'/test-basepath/s/test-space/app/metrics/inventory?type=host&id=some-id&count=12345'
);
expect(result.current.onClick).toBeDefined();
});

it('Provides the correct props with more complex encoding', () => {
const { result } = renderUseLinkPropsHook({
pathname: 'inventory',
pathname: '/inventory',
search: {
type: 'host + host',
name: 'this name has spaces and ** and %',
Expand All @@ -66,7 +80,7 @@ describe('useLinkProps hook', () => {
},
});
expect(result.current.href).toBe(
'/inventory?type=host%20%2B%20host&name=this%20name%20has%20spaces%20and%20**%20and%20%25&id=some-id&count=12345&animals=dog,cat,bear'
'/test-basepath/s/test-space/app/metrics/inventory?type=host%20%2B%20host&name=this%20name%20has%20spaces%20and%20**%20and%20%25&id=some-id&count=12345&animals=dog,cat,bear'
);
expect(result.current.onClick).toBeDefined();
});
Expand All @@ -77,14 +91,14 @@ describe('useLinkProps hook', () => {
time: { from: 12345, to: 54321 },
};
const { result } = renderUseLinkPropsHook({
pathname: 'inventory',
pathname: '/inventory',
search: {
type: 'host + host',
state: encode(state),
},
});
expect(result.current.href).toBe(
'/inventory?type=host%20%2B%20host&state=(refreshInterval:(pause:!t,value:0),time:(from:12345,to:54321))'
'/test-basepath/s/test-space/app/metrics/inventory?type=host%20%2B%20host&state=(refreshInterval:(pause:!t,value:0),time:(from:12345,to:54321))'
);
expect(result.current.onClick).toBeDefined();
});
Expand All @@ -94,8 +108,9 @@ describe('useLinkProps hook', () => {
it('Provides the correct baseline props', () => {
const { result } = renderUseLinkPropsHook({
app: 'ml',
pathname: '/',
});
expect(result.current.href).toBe('/test-basepath/s/test-space/app/ml');
expect(result.current.href).toBe('/test-basepath/s/test-space/app/ml/');
expect(result.current.onClick).not.toBeDefined();
});

Expand Down
59 changes: 33 additions & 26 deletions x-pack/plugins/infra/public/hooks/use_link_props.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ import { useHistory } from '../utils/history_context';
type Search = Record<string, string | string[]>;

export interface LinkDescriptor {
// When an app isn't provided (for external linking) the history instance will
// be used to ensure either metrics or logs is used.
app?: string;
app: string;
pathname?: string;
hash?: string;
search?: Search;
Expand All @@ -37,34 +35,37 @@ export const useLinkProps = ({ app, pathname, hash, search }: LinkDescriptor): L
return search ? encodeSearch(search) : undefined;
}, [search]);

const href = useMemo(() => {
const internalLinkResult = useMemo(() => {
// When the logs / metrics apps are first mounted a history instance is setup with a 'basename' equal to the
// 'appBasePath' received from Core's 'AppMountParams', e.g. /BASE_PATH/s/SPACE_ID/app/APP_ID. With internal
// linking we are using 'createHref' and 'push' on top of this history instance. So a pathname of /inventory used within
// the metrics app will ultimatey end up as /BASE_PATH/s/SPACE_ID/app/metrics/inventory. React-router responds to this
// as it is instantiated with the same history instance.
if (!app) {
return history?.createHref({
pathname: pathname ? formatPathname(pathname) : undefined,
search: encodedSearch,
})
} else {
// The URI spec defines that the query should appear before the fragment
// https://tools.ietf.org/html/rfc3986#section-3 (e.g. url.format()). However, in Kibana, apps that use
// hash based routing expect the query to be part of the hash. This will handle that.
const mergedHash = hash && encodedSearch ? `${hash}?${encodedSearch}` : hash;

const link = url.format({
pathname,
hash: mergedHash,
search: !hash ? encodedSearch : undefined,
});
return prefixer(app, link);
}
}, [app, history, pathname, hash, encodedSearch, prefixer]);
return history?.createHref({
pathname: pathname ? formatPathname(pathname) : undefined,
search: encodedSearch,
});
}, [history, pathname, encodedSearch]);

const externalLinkResult = useMemo(() => {
// The URI spec defines that the query should appear before the fragment
// https://tools.ietf.org/html/rfc3986#section-3 (e.g. url.format()). However, in Kibana, apps that use
// hash based routing expect the query to be part of the hash. This will handle that.
const mergedHash = hash && encodedSearch ? `${hash}?${encodedSearch}` : hash;

const link = url.format({
pathname,
hash: mergedHash,
search: !hash ? encodedSearch : undefined,
});

return prefixer(app, link);
}, [hash, encodedSearch, pathname, prefixer, app]);

const onClick = useMemo(() => {
if (!app) {
// If these results are equal we know we're trying to navigate within the same application
// that the current history instance is representing
if (linksAreEquivalent(externalLinkResult, internalLinkResult)) {
return (e: React.MouseEvent | React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>) => {
e.preventDefault();
if (history) {
Expand All @@ -77,10 +78,10 @@ export const useLinkProps = ({ app, pathname, hash, search }: LinkDescriptor): L
} else {
return undefined;
}
}, [app, history, pathname, encodedSearch]);
}, [internalLinkResult, externalLinkResult, history, pathname, encodedSearch]);

return {
href,
href: externalLinkResult,
onClick,
};
};
Expand All @@ -100,3 +101,9 @@ const validateParams = ({ app, pathname, hash, search }: LinkDescriptor) => {
);
}
};

const linksAreEquivalent = (externalLink: string, internalLink: string | undefined): boolean => {
// Compares with trailing slashes removed. This handles the case where the pathname is '/'
// and 'createHref' will include the '/' but Kibana's 'getUrlForApp' will remove it.
return internalLink ? externalLink.replace(/\/$/, '') === internalLink.replace(/\/$/, '') : false;
};
9 changes: 6 additions & 3 deletions x-pack/plugins/infra/public/pages/infrastructure/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,25 @@ export const InfrastructurePage = ({ match }: RouteComponentProps) => {
<RoutedTabs
tabs={[
{
app: 'metrics',
title: i18n.translate('xpack.infra.homePage.inventoryTabTitle', {
defaultMessage: 'Inventory',
}),
path: '/inventory',
pathname: '/inventory',
},
{
app: 'metrics',
title: i18n.translate('xpack.infra.homePage.metricsExplorerTabTitle', {
defaultMessage: 'Metrics Explorer',
}),
path: '/explorer',
pathname: '/explorer',
},
{
app: 'metrics',
title: i18n.translate('xpack.infra.homePage.settingsTabTitle', {
defaultMessage: 'Settings',
}),
path: '/settings',
pathname: '/settings',
},
]}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ export const SnapshotPage = () => {
</EuiFlexItem>
{uiCapabilities?.infrastructure?.configureSource ? (
<EuiFlexItem>
<ViewSourceConfigurationButton data-test-subj="configureSourceButton">
<ViewSourceConfigurationButton
app="metrics"
data-test-subj="configureSourceButton"
>
{i18n.translate('xpack.infra.configureSourceActionLabel', {
defaultMessage: 'Change source configuration',
})}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const getNodeDetailUrl = ({
from?: number;
}): LinkDescriptor => {
return {
app: 'metrics',
pathname: `link-to/${nodeType}-detail/${nodeId}`,
search:
to && from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const getNodeLogsUrl = ({
time?: number;
}): LinkDescriptor => {
return {
app: 'logs',
pathname: `link-to/${nodeType}-logs/${nodeId}`,
search: time
? {
Expand Down
Loading

0 comments on commit c942eac

Please sign in to comment.