Skip to content

Commit

Permalink
Refactor app state and cleanup unused imports (#4504)
Browse files Browse the repository at this point in the history
Clean up app state for Dashboards plugin.

* Removes the dashboard container hook in place of a single dashboard app state container
* Still recovers some follow-ups and clean up
* Skips test for rendering of a legacy test.

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

---------

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
Co-authored-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
  • Loading branch information
3 people committed Jul 11, 2023
1 parent 98a6b4b commit bd72786
Show file tree
Hide file tree
Showing 29 changed files with 1,685 additions and 792 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React, { useEffect, useState } from 'react';
import React, { useState } from 'react';
import { useParams } from 'react-router-dom';
import { EventEmitter } from 'events';
import { DashboardTopNav } from '../components/dashboard_top_nav';
Expand All @@ -12,95 +12,54 @@ import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react
import { useSavedDashboardInstance } from '../utils/use/use_saved_dashboard_instance';
import { DashboardServices } from '../../types';
import { useDashboardAppAndGlobalState } from '../utils/use/use_dashboard_app_state';
import { useDashboardContainer } from '../utils/use/use_dashboard_container';
import { useEditorUpdates } from '../utils/use/use_editor_updates';
import {
setBreadcrumbsForExistingDashboard,
setBreadcrumbsForNewDashboard,
} from '../utils/breadcrumbs';

export const DashboardEditor = () => {
const { id: dashboardIdFromUrl } = useParams<{ id: string }>();
const { services } = useOpenSearchDashboards<DashboardServices>();
const { chrome } = services;
const isChromeVisible = useChromeVisibility(chrome);
const isChromeVisible = useChromeVisibility({ chrome });
const [eventEmitter] = useState(new EventEmitter());

Check warning on line 22 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L18-L22

Added lines #L18 - L22 were not covered by tests

const { savedDashboard: savedDashboardInstance, dashboard } = useSavedDashboardInstance(
const { savedDashboard: savedDashboardInstance, dashboard } = useSavedDashboardInstance({

Check warning on line 24 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L24

Added line #L24 was not covered by tests
services,
eventEmitter,
isChromeVisible,
dashboardIdFromUrl
);
dashboardIdFromUrl,
});

const { appState } = useDashboardAppAndGlobalState(
const { appState, currentContainer, indexPatterns } = useDashboardAppAndGlobalState({

Check warning on line 31 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L31

Added line #L31 was not covered by tests
services,
eventEmitter,
savedDashboardInstance
);

const { dashboardContainer, indexPatterns } = useDashboardContainer(
services,
dashboard,
savedDashboardInstance,
appState
);
dashboard,
});

const { isEmbeddableRendered, currentAppState } = useEditorUpdates(
const { isEmbeddableRendered, currentAppState } = useEditorUpdates({

Check warning on line 38 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L38

Added line #L38 was not covered by tests
services,
eventEmitter,
dashboard,
savedDashboardInstance,
dashboardContainer,
appState
);

useEffect(() => {
if (currentAppState && dashboard) {
if (savedDashboardInstance?.id) {
chrome.setBreadcrumbs(
setBreadcrumbsForExistingDashboard(
savedDashboardInstance.title,
currentAppState.viewMode,
dashboard.isDirty
)
);
chrome.docTitle.change(savedDashboardInstance.title);
} else {
chrome.setBreadcrumbs(
setBreadcrumbsForNewDashboard(currentAppState.viewMode, dashboard.isDirty)
);
}
}
}, [currentAppState, savedDashboardInstance, chrome, dashboard]);

useEffect(() => {
// clean up all registered listeners if any is left
return () => {
eventEmitter.removeAllListeners();
};
}, [eventEmitter]);
dashboard,
dashboardContainer: currentContainer,
appState,
});

return (

Check warning on line 47 in src/plugins/dashboard/public/application/components/dashboard_editor.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_editor.tsx#L47

Added line #L47 was not covered by tests
<div>
<div>
{savedDashboardInstance &&
appState &&
dashboardContainer &&
currentAppState &&
dashboard && (
<DashboardTopNav
isChromeVisible={isChromeVisible}
savedDashboardInstance={savedDashboardInstance}
stateContainer={appState}
dashboard={dashboard}
currentAppState={currentAppState}
isEmbeddableRendered={isEmbeddableRendered}
indexPatterns={indexPatterns}
dashboardContainer={dashboardContainer}
dashboardIdFromUrl={dashboardIdFromUrl}
/>
)}
{savedDashboardInstance && appState && currentAppState && currentContainer && dashboard && (
<DashboardTopNav
isChromeVisible={isChromeVisible}
savedDashboardInstance={savedDashboardInstance}
appState={appState!}
dashboard={dashboard}
currentAppState={currentAppState}
isEmbeddableRendered={isEmbeddableRendered}
indexPatterns={indexPatterns}
currentContainer={currentContainer}
dashboardIdFromUrl={dashboardIdFromUrl}
/>
)}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import { Dashboard } from '../../dashboard';
interface DashboardTopNavProps {
isChromeVisible: boolean;
savedDashboardInstance: any;
stateContainer: DashboardAppStateContainer;
appState: DashboardAppStateContainer;
dashboard: Dashboard;
currentAppState: DashboardAppState;
isEmbeddableRendered: boolean;
indexPatterns: IndexPattern[];
dashboardContainer?: DashboardContainer;
currentContainer?: DashboardContainer;
dashboardIdFromUrl?: string;
}

Expand All @@ -37,11 +37,11 @@ export enum UrlParams {
const TopNav = ({
isChromeVisible,
savedDashboardInstance,
stateContainer,
appState,
dashboard,
currentAppState,
isEmbeddableRendered,
dashboardContainer,
currentContainer,
indexPatterns,
dashboardIdFromUrl,
}: DashboardTopNavProps) => {
Expand All @@ -57,11 +57,11 @@ const TopNav = ({

const handleRefresh = useCallback(

Check warning on line 58 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L58

Added line #L58 was not covered by tests
(_payload: any, isUpdate?: boolean) => {
if (!isUpdate && dashboardContainer) {
dashboardContainer.reload();
if (!isUpdate && currentContainer) {
currentContainer.reload();

Check warning on line 61 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L61

Added line #L61 was not covered by tests
}
},
[dashboardContainer]
[currentContainer]
);

const isEmbeddedExternally = Boolean(queryParameters.get('embed'));

Check warning on line 67 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L67

Added line #L67 was not covered by tests
Expand All @@ -78,12 +78,12 @@ const TopNav = ({
useEffect(() => {

Check warning on line 78 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L78

Added line #L78 was not covered by tests
if (isEmbeddableRendered) {
const navActions = getNavActions(

Check warning on line 80 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L80

Added line #L80 was not covered by tests
stateContainer,
appState,
savedDashboardInstance,
services,
dashboard,
dashboardIdFromUrl,
dashboardContainer
currentContainer
);
setTopNavMenu(

Check warning on line 88 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L88

Added line #L88 was not covered by tests
getTopNavConfig(
Expand All @@ -97,9 +97,9 @@ const TopNav = ({
currentAppState,
services,
dashboardConfig,
dashboardContainer,
currentContainer,
savedDashboardInstance,
stateContainer,
appState,
isEmbeddableRendered,
dashboard,
dashboardIdFromUrl,
Expand Down Expand Up @@ -139,7 +139,7 @@ const TopNav = ({
showSaveQuery={services.dashboardCapabilities.saveQuery as boolean}
savedQuery={undefined}
onSavedQueryIdChange={(savedQueryId?: string) => {
stateContainer.transitions.set('savedQuery', savedQueryId);
appState.transitions.set('savedQuery', savedQueryId);

Check warning on line 142 in src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/components/dashboard_top_nav.tsx#L142

Added line #L142 was not covered by tests
}}
savedQueryId={currentAppState?.savedQuery}
onQuerySubmit={handleRefresh}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,9 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
public readonly type = DASHBOARD_CONTAINER_TYPE;

public renderEmpty?: undefined | (() => React.ReactNode);
public getChangesFromAppStateForContainerState?: (containerInput: any) => any;
public updateAppStateUrl?: undefined | ((pathname: string, replace: boolean) => void);
public updateAppStateUrl?:
| undefined
| (({ replace, pathname }: { replace: boolean; pathname?: string }) => void);

private embeddablePanel: EmbeddableStart['EmbeddablePanel'];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import 'react-grid-layout/css/styles.css';
import 'react-resizable/css/styles.css';

// @ts-ignore
Expand All @@ -39,7 +38,7 @@ import classNames from 'classnames';
import _ from 'lodash';
import React from 'react';
import { Subscription } from 'rxjs';
import ReactGridLayout, { Layout } from 'react-grid-layout';
import ReactGridLayout, { Layout, ReactGridLayoutProps } from 'react-grid-layout';
import { GridData } from '../../../../common';
import { ViewMode, EmbeddableChildPanel, EmbeddableStart } from '../../../embeddable_plugin';
import { DASHBOARD_GRID_COLUMN_COUNT, DASHBOARD_GRID_HEIGHT } from '../dashboard_constants';
Expand Down Expand Up @@ -76,9 +75,9 @@ function ResponsiveGrid({
size: { width: number };
isViewMode: boolean;
layout: Layout[];
onLayoutChange: () => void;
onLayoutChange: ReactGridLayoutProps['onLayoutChange'];
children: JSX.Element[];
maximizedPanelId: string;
maximizedPanelId?: string;
useMargins: boolean;
}) {
// This is to prevent a bug where view mode changes when the panel is expanded. View mode changes will trigger
Expand Down Expand Up @@ -171,7 +170,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
let layout;
try {
layout = this.buildLayoutFromPanels();
} catch (error) {
} catch (error: any) {
console.error(error); // eslint-disable-line no-console

isLayoutInvalid = true;
Expand Down Expand Up @@ -283,6 +282,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
}}
>
<EmbeddableChildPanel
key={panel.type}
embeddableId={panel.explicitInput.id}
container={this.props.container}
PanelComponent={this.props.PanelComponent}
Expand All @@ -304,7 +304,7 @@ class DashboardGridUi extends React.Component<DashboardGridProps, State> {
isViewMode={isViewMode}
layout={this.buildLayoutFromPanels()}
onLayoutChange={this.onLayoutChange}
maximizedPanelId={this.state.expandedPanelId}
maximizedPanelId={this.state.expandedPanelId!}
useMargins={this.state.useMargins}
>
{this.renderPanels()}
Expand Down
7 changes: 2 additions & 5 deletions src/plugins/dashboard/public/application/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,14 @@ import { DashboardServices } from '../types';
export * from './embeddable';
export * from './actions';

export const renderApp = (
{ element, appBasePath, onAppLeave }: AppMountParameters,
services: DashboardServices
) => {
export const renderApp = ({ element }: AppMountParameters, services: DashboardServices) => {
addHelpMenuToAppChrome(services.chrome, services.docLinks);

Check warning on line 24 in src/plugins/dashboard/public/application/index.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/index.tsx#L24

Added line #L24 was not covered by tests

const app = (
<Router history={services.history}>

Check warning on line 27 in src/plugins/dashboard/public/application/index.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/index.tsx#L27

Added line #L27 was not covered by tests
<OpenSearchDashboardsContextProvider services={services}>
<services.i18n.Context>
<DashboardApp onAppLeave={onAppLeave} />
<DashboardApp />
</services.i18n.Context>
</OpenSearchDashboardsContextProvider>
</Router>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ export function migrateAppState(
delete appState.uiState;
}

appState.panels.forEach((panel) => {
panel.version = opensearchDashboardsVersion;
});
// appState.panels.forEach((panel) => {
// panel.version = opensearchDashboardsVersion;
// });

return appState;
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { updateSavedDashboard } from './update_saved_dashboard';

import { DashboardAppStateContainer } from '../../types';
import { Dashboard } from '../../dashboard';
import { SavedObjectDashboard } from '../../saved_dashboards';

/**
* Saves the dashboard.
Expand All @@ -43,7 +44,7 @@ import { Dashboard } from '../../dashboard';
export function saveDashboard(
timeFilter: TimefilterContract,
stateContainer: DashboardAppStateContainer,
savedDashboard: any,
savedDashboard: SavedObjectDashboard,
saveOptions: SavedObjectSaveOpts,
dashboard: Dashboard
): Promise<string> {
Expand All @@ -54,7 +55,9 @@ export function saveDashboard(
// TODO: should update Dashboard class in the if(id) block
return savedDashboard.save(saveOptions).then((id: string) => {
if (id) {
dashboard.id = id;
return id;

Check warning on line 59 in src/plugins/dashboard/public/application/lib/save_dashboard.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/dashboard/public/application/lib/save_dashboard.ts#L58-L59

Added lines #L58 - L59 were not covered by tests
}
return id;
});
}
Loading

0 comments on commit bd72786

Please sign in to comment.