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

[Discover] A bunch of navigation fixes #5168

Merged
merged 6 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

### 🛡 Security

- [CVE-2022-25869] Remove AngularJS 1.8 ([#5086](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5086))
- [CVE-2022-37599] Bump loader-utils from `2.0.3` to `2.0.4` ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031)). Backwards-compatible fixes included in v2.6.0 and v1.3.7 releases.
- [CVE-2022-37603] Bump loader-utils from `2.0.3` to `2.0.4` ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031)). Backwards-compatible fixes included in v2.6.0 and v1.3.7 releases.
- [WS-2021-0638] Bump mocha from `7.2.0` to `10.1.0` ([#2711](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2711))
Expand All @@ -36,8 +37,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster([#4612](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4612))
- [Advanced Settings] Consolidate settings into new "Appearance" category and add category IDs ([#4845](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4845))
- Adds Data explorer framework and implements Discover using it ([#4806](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4806))
- [Theme] Use themes' definitions to render the initial view ([#4936](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4936/))
- [Theme] Make `next` theme the default ([#4854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4854/))
- [Theme] Use themes' definitions to render the initial view ([#4936](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4936))
- [Theme] Make `next` theme the default ([#4854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4854))
- [Discover] Update embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081))

### 🐛 Bug Fixes

Expand All @@ -55,6 +57,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG] Fix buildPointSeriesData unit test fails due to local timezone ([#4992](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4992))
- [BUG][Data Explorer][Discover] Fix total hits issue for no time based data ([#5087](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5087))
- [BUG][Data Explorer][Discover] Add onQuerySubmit to top nav and allow force update to embeddable ([#5160](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5160))
- [BUG][Discover] Fix misc navigation issues ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))
- [BUG][Discover] Fix mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))

### 🚞 Infrastructure

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
import { loadReduxState, persistReduxState } from './redux_persistence';
import { DataExplorerServices } from '../../types';

const HYDRATE = 'HYDRATE';

export const hydrate = (newState: RootState) => ({
type: HYDRATE,
payload: newState,
});

const commonReducers = {
metadata: metadataReducer,
};
Expand All @@ -22,9 +29,20 @@

const rootReducer = combineReducers(dynamicReducers);

const createRootReducer = (): Reducer<RootState> => {
const combinedReducer = combineReducers(dynamicReducers);

Check warning on line 33 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L33

Added line #L33 was not covered by tests

return (state: RootState | undefined, action: any): RootState => {

Check warning on line 35 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L35

Added line #L35 was not covered by tests
if (action.type === HYDRATE) {
return action.payload;

Check warning on line 37 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L37

Added line #L37 was not covered by tests
}
return combinedReducer(state, action);

Check warning on line 39 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L39

Added line #L39 was not covered by tests
};
};

export const configurePreloadedStore = (preloadedState: PreloadedState<RootState>) => {
// After registering the slices the root reducer needs to be updated
const updatedRootReducer = combineReducers(dynamicReducers);
const updatedRootReducer = createRootReducer();

Check warning on line 45 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L45

Added line #L45 was not covered by tests

return configureStore({
reducer: updatedRootReducer,
Expand Down Expand Up @@ -62,6 +80,18 @@
// the store subscriber will automatically detect changes and call handleChange function
const unsubscribe = store.subscribe(handleChange);

// This is necessary because browser navigation updates URL state that isnt reflected in the redux state
services.scopedHistory.listen(async (location, action) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we hold this for better testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the reason for this is because of the shared data model of OSD where the state is maintained in both the URL and the react memory. So far the assumption i've made is to have redux be the source of truth and update every other place from there. The problem here is that when the user navigates back using browser history, the URL state is no longer in sync with the redux state. The opensearch_dashboard_util wrappers that do the url state sync do something very similar.

The one big concern I had with this was a race condition. This here is mitigated for both a URL update and Redux store update using the check to see if the url update was a pop action and if the states are identical. I'm open to improvements here but I think we should ship with this given that its a jarring expereince for users who have relied on browser navigation for discover.

const urlState = await loadReduxState(services);
const currentState = store.getState();

Check warning on line 86 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L84-L86

Added lines #L84 - L86 were not covered by tests

// If the url state is different from the current state, then we need to update the store
// the state should have a view property if it was loaded from the url
if (action === 'POP' && urlState.metadata?.view && !isEqual(urlState, currentState)) {
store.dispatch(hydrate(urlState as RootState));

Check warning on line 91 in src/plugins/data_explorer/public/utils/state_management/store.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_explorer/public/utils/state_management/store.ts#L91

Added line #L91 was not covered by tests
}
});

const onUnsubscribe = () => {
dynamicReducers = {
...commonReducers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export function SurroundingDocsApp() {

useEffect(() => {
chrome.setBreadcrumbs([
...getRootBreadcrumbs(baseUrl),
...getRootBreadcrumbs(),
{
text: i18n.translate('discover.context.breadcrumb', {
defaultMessage: `Context of #{docId}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export const SurroundingDocsView = ({ id, indexPattern }: SurroundingDocsViewPar
field,
values,
operation,
indexPattern.id
indexPattern.id || ''
);
return filterManager.addFilters(newFilters);
},
Expand All @@ -115,23 +115,25 @@ export const SurroundingDocsView = ({ id, indexPattern }: SurroundingDocsViewPar
[onAddFilter, rows, indexPattern, setContextAppState, contextQueryState, contextAppState]
);

if (isLoading) {
return null;
}

return (
!isLoading && (
<Fragment>
<TopNavMenu
appName={'discover.context.surroundingDocs.topNavMenu'}
showSearchBar={true}
showQueryBar={false}
showDatePicker={false}
indexPatterns={[indexPattern]}
useDefaultBehaviors={true}
/>
<EuiPage className="discover.context.appPage">
<EuiPageContent paddingSize="s" className="dscDocsContent">
{contextAppMemoized}
</EuiPageContent>
</EuiPage>
</Fragment>
)
<Fragment>
<TopNavMenu
appName={'discover.context.surroundingDocs.topNavMenu'}
showSearchBar={true}
showQueryBar={false}
showDatePicker={false}
indexPatterns={[indexPattern]}
useDefaultBehaviors={true}
/>
<EuiPage className="discover.context.appPage">
<EuiPageContent paddingSize="s" className="dscDocsContent">
{contextAppMemoized}
</EuiPageContent>
</EuiPage>
</Fragment>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,8 @@
&:focus {
opacity: 1;
}

@include ouiBreakpoint("xs", "s", "m") {
opacity: 1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
import { DiscoverState, setSavedSearchId } from '../../utils/state_management';
import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../../../common';
import { getSortForSearchSource } from '../../view_components/utils/get_sort_for_search_source';
import { getRootBreadcrumbs } from '../../helpers/breadcrumbs';

export const getTopNavLinks = (
services: DiscoverViewServices,
Expand All @@ -45,11 +46,9 @@ export const getTopNavLinks = (
defaultMessage: 'New Search',
}),
run() {
setTimeout(() => {
history().push('/');
// TODO: figure out why a history push doesn't update the app state. The page reload is a hack around it
window.location.reload();
}, 0);
core.application.navigateToApp('discover', {
path: '#/',
});
Comment on lines +49 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this reload the page if i was already on discover?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does :)

},
testId: 'discoverNewButton',
};
Expand Down Expand Up @@ -103,15 +102,7 @@ export const getTopNavLinks = (
history().push(`/view/${encodeURIComponent(id)}`);
} else {
chrome.docTitle.change(savedSearch.lastSavedTitle);
chrome.setBreadcrumbs([
{
text: i18n.translate('discover.discoverBreadcrumbTitle', {
defaultMessage: 'Discover',
}),
href: '#/',
},
{ text: savedSearch.title },
]);
chrome.setBreadcrumbs([...getRootBreadcrumbs(), { text: savedSearch.title }]);
}

// set App state to clean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
export function OpenSearchPanel({ onClose, makeUrl }: Props) {
const {
services: {
core: { uiSettings, savedObjects },
core: { uiSettings, savedObjects, application },
addBasePath,
},
} = useOpenSearchDashboards<DiscoverViewServices>();
Expand Down Expand Up @@ -90,12 +90,8 @@
},
]}
onChoose={(id) => {
setTimeout(() => {
window.location.assign(makeUrl(id));
// TODO: figure out why a history push doesn't update the app state. The page reload is a hack around it
window.location.reload();
onClose();
}, 0);
application.navigateToApp('discover', { path: `#/view/${id}` });
onClose();

Check warning on line 94 in src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/application/components/top_nav/open_search_panel.tsx#L93-L94

Added lines #L93 - L94 were not covered by tests
}}
uiSettings={uiSettings}
savedObjects={savedObjects}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,17 @@
*/

import { i18n } from '@osd/i18n';
import { EuiBreadcrumb } from '@elastic/eui';
import { getServices } from '../../opensearch_dashboards_services';

export function getRootBreadcrumbs(baseUrl: string) {
export function getRootBreadcrumbs(): EuiBreadcrumb[] {
const { core } = getServices();
return [
{
text: i18n.translate('discover.rootBreadcrumb', {
defaultMessage: 'Discover',
}),
href: baseUrl,
onClick: () => core.application.navigateToApp('discover'),
},
];
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { RootState, DefaultViewState } from '../../../../../data_explorer/public
import { buildColumns } from '../columns';
import * as utils from './common';
import { SortOrder } from '../../../saved_searches/types';
import { PLUGIN_ID } from '../../../../common';

export interface DiscoverState {
/**
Expand Down Expand Up @@ -79,6 +80,7 @@ export const getPreloadedState = async ({
preloadedState.root = {
metadata: {
indexPattern: indexPatternId,
view: PLUGIN_ID,
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { DiscoverChart } from '../../components/chart/chart';

export const DiscoverChartContainer = ({ hits, bucketInterval, chartData }: SearchData) => {
const { services } = useOpenSearchDashboards<DiscoverViewServices>();
const { uiSettings, data } = services;
const { uiSettings, data, core } = services;
const { indexPattern, savedSearch } = useDiscoverContext();

const isTimeBased = useMemo(() => (indexPattern ? indexPattern.isTimeBased() : false), [
Expand All @@ -30,8 +30,7 @@ export const DiscoverChartContainer = ({ hits, bucketInterval, chartData }: Sear
data={data}
hits={hits}
resetQuery={() => {
window.location.href = `#/view/${savedSearch?.id}`;
window.location.reload();
core.application.navigateToApp('discover', { path: `#/view/${savedSearch?.id}` });
}}
services={services}
showResetButton={!!savedSearch && !!savedSearch.id}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import React, { useEffect, useState, useRef, useCallback } from 'react';
import { EuiFlexGroup, EuiFlexItem, EuiPanel } from '@elastic/eui';
import { EuiPanel } from '@elastic/eui';
import { TopNav } from './top_nav';
import { ViewProps } from '../../../../../data_explorer/public';
import { DiscoverTable } from './discover_table';
Expand All @@ -20,6 +20,7 @@ import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_re
import { filterColumns } from '../utils/filter_columns';
import { DEFAULT_COLUMNS_SETTING } from '../../../../common';
import './discover_canvas.scss';

// eslint-disable-next-line import/no-default-export
export default function DiscoverCanvas({ setHeaderActionMenu, history }: ViewProps) {
const { data$, refetch$, indexPattern } = useDiscoverContext();
Expand Down Expand Up @@ -77,38 +78,36 @@ export default function DiscoverCanvas({ setHeaderActionMenu, history }: ViewPro
const timeField = indexPattern?.timeFieldName ? indexPattern.timeFieldName : undefined;

return (
<EuiFlexGroup direction="column" gutterSize="none" className="dscCanvas">
<EuiFlexItem grow={false}>
<TopNav
opts={{
setHeaderActionMenu,
onQuerySubmit,
}}
/>
</EuiFlexItem>
<EuiPanel
hasBorder={false}
hasShadow={false}
color="transparent"
paddingSize="none"
className="dscCanvas"
>
<TopNav
opts={{
setHeaderActionMenu,
onQuerySubmit,
}}
/>
{status === ResultStatus.NO_RESULTS && (
<EuiFlexItem>
<DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />
</EuiFlexItem>
<DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this appear at the vertical and horizontal center of the page?

Copy link
Member Author

Choose a reason for hiding this comment

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

It spans the width of the container but does not center to the vertical center. The flex was originally added to make the app appear as a full page application. Users have since complained about its usability like that so this change reverts it to a simpler layout allowing for scrolling, so having a centered component would not be really helpful.

)}
{status === ResultStatus.UNINITIALIZED && (
<DiscoverUninitialized onRefresh={() => refetch$.next()} />
)}
{status === ResultStatus.LOADING && <LoadingSpinner />}
{status === ResultStatus.READY && (
<>
<EuiFlexItem grow={false}>
<EuiPanel hasBorder={false} hasShadow={false} color="transparent" paddingSize="s">
<EuiPanel>
<DiscoverChartContainer {...fetchState} />
</EuiPanel>
<EuiPanel hasBorder={false} hasShadow={false} color="transparent" paddingSize="s">
<EuiPanel>
<DiscoverChartContainer {...fetchState} />
</EuiPanel>
</EuiFlexItem>
<EuiFlexItem>
<DiscoverTable history={history} />
</EuiFlexItem>
</EuiPanel>
<DiscoverTable history={history} />
</>
)}
</EuiFlexGroup>
</EuiPanel>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,9 @@ export const TopNav = ({ opts }: TopNavProps) => {
chrome.docTitle.change(`Discover${pageTitleSuffix}`);

if (savedSearch?.id) {
chrome.setBreadcrumbs([
...getRootBreadcrumbs(getUrlForApp(PLUGIN_ID)),
{ text: savedSearch.title },
]);
chrome.setBreadcrumbs([...getRootBreadcrumbs(), { text: savedSearch.title }]);
} else {
chrome.setBreadcrumbs([...getRootBreadcrumbs(getUrlForApp(PLUGIN_ID))]);
chrome.setBreadcrumbs([...getRootBreadcrumbs()]);
}
}, [chrome, getUrlForApp, savedSearch?.id, savedSearch?.title]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ const SearchContext = React.createContext<SearchContextValue>({} as SearchContex

// eslint-disable-next-line import/no-default-export
export default function DiscoverContext({ children }: React.PropsWithChildren<ViewProps>) {
const { services: deServices } = useOpenSearchDashboards<DataExplorerServices>();
const services = getServices();
const searchParams = useSearch(services);
const searchParams = useSearch({
...deServices,
...services,
});

const {
services: { osdUrlStateStorage },
} = useOpenSearchDashboards<DataExplorerServices>();
const { osdUrlStateStorage } = deServices;

// Connect the query service to the url state
useEffect(() => {
Expand Down
Loading
Loading