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

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

Merged
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use mirrors to download Node.js binaries to escape sporadic 404 errors ([#3619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3619))
- [Multiple DataSource] Refactor dev tool console to use opensearch-js client to send requests ([#3544](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3544))
- [Data] Add geo shape filter field ([#3605](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3605))
- [VisBuilder] Add UI actions handler ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [Dashboard] Indicate that IE is no longer supported ([#3641](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3641))
- [UI] Add support for comma delimiters in the global filter bar ([#3686](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3686))
- [Multiple DataSource] Allow create and distinguish index pattern with same name but from different datasources ([#3571](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3571))
Expand Down Expand Up @@ -117,6 +118,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Clean up and rebuild `@osd/pm` ([#3570](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3570))
- [Vega] Add Filter custom label for opensearchDashboardsAddFilter ([#3640](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3640))
- [Timeline] Fix y-axis label color in dark mode ([#3698](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3698))
- [VisBuilder] Fix multiple warnings thrown on page load ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [VisBuilder] Fix Firefox legend selection issue ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [VisBuilder] Fix type errors ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [Table Visualization] Fix table rendering empty unused space ([#3797](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3797))
- [Table Visualization] Fix data table not adjusting height on the initial load ([#3816](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3816))
- Cleanup unused url ([#3847](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3847))
Expand Down
5 changes: 3 additions & 2 deletions src/plugins/vis_builder/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"expressions",
"navigation",
"savedObjects",
"visualizations"
"visualizations",
"uiActions"
],
"requiredBundles": [
"charts",
Expand All @@ -20,4 +21,4 @@
"visDefaultEditor",
"visTypeVislib"
]
}
}
3 changes: 2 additions & 1 deletion src/plugins/vis_builder/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ import { DragDropProvider } from './utils/drag_drop/drag_drop_context';
import { LeftNav } from './components/left_nav';
import { TopNav } from './components/top_nav';
import { Workspace } from './components/workspace';
import './app.scss';
import { RightNav } from './components/right_nav';
import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
import { VisBuilderServices } from '../types';
import { syncQueryStateWithUrl } from '../../../data/public';

import './app.scss';

export const VisBuilderApp = () => {
const {
services: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from '../utils/state_management';
import { getPersistedAggParams } from '../utils/get_persisted_agg_params';

export const RightNav = () => {
export const RightNavUI = () => {
const { ui, name: activeVisName } = useVisualizationType();
const [confirmAggs, setConfirmAggs] = useState<ActiveVisPayload | undefined>();
const {
Expand Down Expand Up @@ -121,3 +121,7 @@ const OptionItem = ({ icon, title }: { icon: IconType; title: string }) => (
<span>{title}</span>
</>
);

// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.
// The app uses EuiResizableContainer that triggers a rerender for every mouseover action.

// To prevent this child component from unnecessarily rerendering in that instance, it needs to be memoized
export const RightNav = React.memo(RightNavUI);
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { i18n } from '@osd/i18n';
import { EuiEmptyPrompt, EuiFlexGroup, EuiFlexItem, EuiIcon, EuiPanel } from '@elastic/eui';
import React, { FC, useState, useMemo, useEffect, useLayoutEffect } from 'react';
import React, { useState, useMemo, useEffect, useLayoutEffect } from 'react';
import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
import { IExpressionLoaderParams } from '../../../../expressions/public';
import { VisBuilderServices } from '../../types';
Expand All @@ -19,17 +19,19 @@ import fields_bg from '../../assets/fields_bg.svg';

import './workspace.scss';
import { ExperimentalInfo } from './experimental_info';
import { handleVisEvent } from '../utils/handle_vis_event';

export const Workspace: FC = ({ children }) => {
export const WorkspaceUI = () => {
const {
services: {
expressions: { ReactExpressionRenderer },
notifications: { toasts },
data,
uiActions,
},
} = useOpenSearchDashboards<VisBuilderServices>();
const { toExpression, ui } = useVisualizationType();
const { aggConfigs } = useAggs();
const { aggConfigs, indexPattern } = useAggs();
const [expression, setExpression] = useState<string>();
const [searchContext, setSearchContext] = useState<IExpressionLoaderParams['searchContext']>({
query: data.query.queryString.getQuery(),
Expand All @@ -44,15 +46,17 @@ export const Workspace: FC = ({ children }) => {
async function loadExpression() {
const schemas = ui.containerConfig.data.schemas;

const noAggs = aggConfigs?.aggs?.length === 0;
const noAggs = (aggConfigs?.aggs?.length ?? 0) === 0;
Copy link
Member

Choose a reason for hiding this comment

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

opinion - this reads worse to me than

const noAggs = !aggConfigs?.aggs?.length;
or even
const noAggs = aggConfigs?.aggs?.length == false;

Also, for conditionals like this I'd prefer hasNoAggs.

const schemaValidation = validateSchemaState(schemas, rootState.visualization);
const aggValidation = validateAggregations(aggConfigs?.aggs || []);

if (noAggs || !aggValidation.valid || !schemaValidation.valid) {
if (!aggValidation.valid || !schemaValidation.valid) {
setExpression(undefined);
if (noAggs) return; // don't show error when there are no active aggregations

const err = schemaValidation.errorMsg || aggValidation.errorMsg;

if (err) toasts.addWarning(err);
setExpression(undefined);

return;
}
Expand Down Expand Up @@ -91,6 +95,7 @@ export const Workspace: FC = ({ children }) => {
expression={expression}
searchContext={searchContext}
uiState={uiState}
onEvent={(event) => handleVisEvent(event, uiActions, indexPattern?.timeFieldName)}
/>
) : (
<EuiFlexItem className="vbWorkspace__empty" data-test-subj="emptyWorkspace">
Expand Down Expand Up @@ -127,3 +132,7 @@ export const Workspace: FC = ({ children }) => {
</section>
);
};

// The app uses EuiResizableContainer that triggers a rerender for ever mouseover action.
// To prevent this child component from unnecessarily rerendering in that instance, it needs to be memoized
export const Workspace = React.memo(WorkspaceUI);

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ import {
showSaveModal,
} from '../../../../saved_objects/public';
import { VisBuilderServices } from '../..';
import { VisBuilderVisSavedObject } from '../../types';
import { VisBuilderSavedObject } from '../../types';
import { AppDispatch } from './state_management';
import { EDIT_PATH, VISBUILDER_SAVED_OBJECT } from '../../../common';
import { setEditorState } from './state_management/metadata_slice';
export interface TopNavConfigParams {
visualizationIdFromUrl: string;
savedVisBuilderVis: VisBuilderVisSavedObject;
savedVisBuilderVis: VisBuilderSavedObject;
saveDisabledReason?: string;
dispatch: AppDispatch;
originatingApp?: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { ExpressionRendererEvent } from '../../../../expressions/public';
import { VIS_EVENT_TO_TRIGGER } from '../../../../visualizations/public';
import { handleVisEvent } from './handle_vis_event';
import { uiActionsPluginMock } from '../../../../ui_actions/public/mocks';
import { Action, ActionType, createAction } from '../../../../ui_actions/public';

const executeFn = jest.fn();

function createTestAction<C extends object>(
type: string,
checkCompatibility: (context: C) => boolean,
autoExecutable = true
): Action<object> {
return createAction({
type: type as ActionType,
id: type,
isCompatible: (context: C) => Promise.resolve(checkCompatibility(context)),
execute: (context) => {
return executeFn(context);
},
shouldAutoExecute: () => Promise.resolve(autoExecutable),
});
}

let uiActions: ReturnType<typeof uiActionsPluginMock.createPlugin>;

describe('handleVisEvent', () => {
beforeEach(() => {
uiActions = uiActionsPluginMock.createPlugin();

executeFn.mockClear();
jest.useFakeTimers();
});

test('should trigger the correct event', async () => {
const event: ExpressionRendererEvent = {
name: 'filter',
data: {},
};
const action = createTestAction('test1', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.filter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
data: { timeFieldName },
})
);
});

test('should trigger the default trigger when not found', async () => {
const event: ExpressionRendererEvent = {
name: 'test',
data: {},
};
const action = createTestAction('test2', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.filter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
data: { timeFieldName },
})
);
});

test('should have the correct context for `applyfilter`', async () => {
const event: ExpressionRendererEvent = {
name: 'applyFilter',
data: {},
};
const action = createTestAction('test3', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.applyFilter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
timeFieldName,
})
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { ExpressionRendererEvent } from '../../../../expressions/public';
import { VIS_EVENT_TO_TRIGGER } from '../../../../visualizations/public';
import { UiActionsStart } from '../../../../ui_actions/public';

export const handleVisEvent = async (
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, do we need to add error handling here?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do see error handling being added here?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm i am mostly thinking if it's possible that await uiActions.getTrigger() fail?

event: ExpressionRendererEvent,
uiActions: UiActionsStart,
timeFieldName?: string
) => {
const triggerId = VIS_EVENT_TO_TRIGGER[event.name] ?? VIS_EVENT_TO_TRIGGER.filter;
const isApplyFilter = triggerId === VIS_EVENT_TO_TRIGGER.applyFilter;
const dataContext = {
timeFieldName,
...event.data,
};
const context = isApplyFilter ? dataContext : { data: dataContext };

await uiActions.getTrigger(triggerId).exec(context);
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,10 @@ import {
import { EDIT_PATH, PLUGIN_ID } from '../../../../common';
import { VisBuilderServices } from '../../../types';
import { getCreateBreadcrumbs, getEditBreadcrumbs } from '../breadcrumbs';
import { getSavedVisBuilderVis } from '../get_saved_vis_builder_vis';
import {
useTypedDispatch,
setStyleState,
setVisualizationState,
VisualizationState,
} from '../state_management';
import { useTypedDispatch, setStyleState, setVisualizationState } from '../state_management';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import { setEditorState } from '../state_management/metadata_slice';
import { validateVisBuilderState } from '../validations/vis_builder_state_validation';
import { getStateFromSavedObject } from '../../../saved_visualizations/transforms';

// This function can be used when instantiating a saved vis or creating a new one
// using url parameters, embedding and destroying it in DOM
Expand All @@ -39,6 +33,7 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined
history,
http: { basePath },
toastNotifications,
savedVisBuilderLoader,
} = services;
const toastNotification = (message: string) => {
toastNotifications.addDanger({
Expand All @@ -51,42 +46,22 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined

const loadSavedVisBuilderVis = async () => {
try {
const savedVisBuilderVis = await getSavedVisBuilderVis(services, visualizationIdFromUrl);
const savedVisBuilderVis = await getSavedVisBuilderVis(
savedVisBuilderLoader,
visualizationIdFromUrl
);

if (savedVisBuilderVis.id) {
chrome.setBreadcrumbs(getEditBreadcrumbs(savedVisBuilderVis.title, navigateToApp));
chrome.docTitle.change(savedVisBuilderVis.title);
const { title, state } = getStateFromSavedObject(savedVisBuilderVis);
chrome.setBreadcrumbs(getEditBreadcrumbs(title, navigateToApp));
chrome.docTitle.change(title);

dispatch(setStyleState(state.style));
dispatch(setVisualizationState(state.visualization));
} else {
chrome.setBreadcrumbs(getCreateBreadcrumbs(navigateToApp));
}

if (
savedVisBuilderVis.styleState !== '{}' &&
savedVisBuilderVis.visualizationState !== '{}'
) {
const styleState = JSON.parse(savedVisBuilderVis.styleState);
const vizStateWithoutIndex = JSON.parse(savedVisBuilderVis.visualizationState);
const visualizationState: VisualizationState = {
searchField: vizStateWithoutIndex.searchField,
activeVisualization: vizStateWithoutIndex.activeVisualization,
indexPattern: savedVisBuilderVis.searchSourceFields.index,
};

const validateResult = validateVisBuilderState({ styleState, visualizationState });
if (!validateResult.valid) {
throw new InvalidJSONProperty(
validateResult.errorMsg ||
i18n.translate('visBuilder.useSavedVisBuilderVis.genericJSONError', {
defaultMessage:
'Something went wrong while loading your saved object. The object may be corrupted or does not match the latest schema',
})
);
}

dispatch(setStyleState(styleState));
dispatch(setVisualizationState(visualizationState));
}

setSavedVisState(savedVisBuilderVis);
dispatch(setEditorState({ state: 'clean' }));
} catch (error) {
Expand Down Expand Up @@ -123,3 +98,12 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined

return savedVisState;
};

async function getSavedVisBuilderVis(
savedVisBuilderLoader: VisBuilderServices['savedVisBuilderLoader'],
visBuilderVisId?: string
) {
const savedVisBuilderVis = await savedVisBuilderLoader.get(visBuilderVisId);

return savedVisBuilderVis;
}
Loading