diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/data_panel_wrapper.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/data_panel_wrapper.test.tsx index c0592d1574fad..64656a2eedf63 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/data_panel_wrapper.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/data_panel_wrapper.test.tsx @@ -12,6 +12,7 @@ import { DragDropIdentifier } from '../../drag_drop'; import { UiActionsStart } from 'src/plugins/ui_actions/public'; import { mockStoreDeps, mountWithProvider } from '../../mocks'; import { disableAutoApply } from '../../state_management/lens_slice'; +import { selectTriggerApplyChanges } from '../../state_management'; describe('Data Panel Wrapper', () => { describe('Datasource data panel properties', () => { @@ -59,28 +60,27 @@ describe('Data Panel Wrapper', () => { describe('setState', () => { it('applies state immediately when option true', async () => { lensStore.dispatch(disableAutoApply()); + selectTriggerApplyChanges(lensStore.getState()); const newDatasourceState = { age: 'new' }; datasourceDataPanelProps.setState(newDatasourceState, { applyImmediately: true }); - const lensState = lensStore.getState().lens; - expect(lensState.datasourceStates.activeDatasource.state).toEqual(newDatasourceState); - expect(lensState.appliedState?.datasourceStates.activeDatasource.state).toEqual( + expect(lensStore.getState().lens.datasourceStates.activeDatasource.state).toEqual( newDatasourceState ); + expect(selectTriggerApplyChanges(lensStore.getState())).toBeTruthy(); }); it('does not apply state immediately when option false', async () => { lensStore.dispatch(disableAutoApply()); + selectTriggerApplyChanges(lensStore.getState()); const newDatasourceState = { age: 'new' }; datasourceDataPanelProps.setState(newDatasourceState, { applyImmediately: false }); const lensState = lensStore.getState().lens; expect(lensState.datasourceStates.activeDatasource.state).toEqual(newDatasourceState); - expect(lensState.appliedState?.datasourceStates.activeDatasource.state).not.toEqual( - newDatasourceState - ); + expect(selectTriggerApplyChanges(lensStore.getState())).toBeFalsy(); }); }); }); diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx index 32e1bc0505b1c..2f9c7bdff60cb 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx @@ -30,6 +30,7 @@ import { setToggleFullscreen, VisualizationState, } from '../../state_management'; +import { setChangesApplied } from '../../state_management/lens_slice'; const SELECTORS = { APPLY_CHANGES_BUTTON: 'button[data-test-subj="lnsSuggestionApplyChanges"]', @@ -132,12 +133,8 @@ describe('suggestion_panel', () => { something: 'changed', }, } as VisualizationState, - // including appliedState signals that auto-apply has been disabled - appliedState: { - activeDatasourceId: 'foobar', - visualization: preloadedState.visualization as VisualizationState, - datasourceStates: preloadedState.datasourceStates as DatasourceStates, - }, + changesApplied: false, + autoApplyDisabled: true, }, }); @@ -147,9 +144,11 @@ describe('suggestion_panel', () => { instance.find(SELECTORS.APPLY_CHANGES_BUTTON).simulate('click'); // check changes applied - const newLensState = lensStore.getState().lens; - expect(newLensState.appliedState?.visualization).toEqual(newLensState.visualization); - expect(newLensState.appliedState?.datasourceStates).toEqual(newLensState.datasourceStates); + expect(lensStore.dispatch).toHaveBeenCalledWith(applyChanges()); + + // simulate workspace panel behavior + lensStore.dispatch(setChangesApplied(true)); + instance.update(); // check UI updated expect(instance.exists(SELECTORS.APPLY_CHANGES_BUTTON)).toBeFalsy(); diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index b62ea4271ce1c..70295350ebe18 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -8,7 +8,7 @@ import React from 'react'; import { act } from 'react-dom/test-utils'; import { ReactExpressionRendererProps } from '../../../../../../../src/plugins/expressions/public'; -import { DatasourcePublicAPI, FramePublicAPI, Visualization } from '../../../types'; +import { FramePublicAPI, Visualization } from '../../../types'; import { createMockVisualization, createMockDatasource, @@ -177,6 +177,8 @@ describe('workspace_panel', () => { instance = mounted.instance; + instance.update(); + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` "kibana | lens_merge_tables layerIds=\\"first\\" tables={datasource} @@ -184,26 +186,14 @@ describe('workspace_panel', () => { `); }); - it('should use applied state when available (when auto-apply is disabled)', async () => { + it('should give user control when auto-apply disabled', async () => { const framePublicAPI = createMockFramePublicAPI(); framePublicAPI.datasourceLayers = { first: mockDatasource.publicAPIMock, }; - framePublicAPI.appliedDatasourceLayers = { - first: { ...mockDatasource.publicAPIMock, applied: true } as DatasourcePublicAPI, - }; - mockDatasource.toExpression.mockReturnValue('datasource'); mockDatasource.getLayers.mockReturnValue(['first']); - const toExpression = jest.fn( - ( - state: unknown, - datasourceLayers: Record, - attributes?: Partial<{ title: string; description: string }> - ) => 'testVis' - ); - const mounted = await mountWithProvider( { }} framePublicAPI={framePublicAPI} visualizationMap={{ - testVis: { ...mockVisualization, toExpression }, + testVis: { ...mockVisualization, toExpression: () => 'testVis' }, }} ExpressionRenderer={expressionRendererMock} />, { preloadedState: { - appliedState: { - activeDatasourceId: 'testDatasource', - visualization: { - activeId: 'testVis', - state: { applied: true }, - }, - datasourceStates: { - [mockDatasource.id]: { isLoading: false, state: { applied: true } }, - }, - }, + autoApplyDisabled: true, }, } ); instance = mounted.instance; - expect(toExpression).toHaveBeenCalled(); - const [appliedVisualizationState, appliedDatasourceLayers] = toExpression.mock.calls[0]; - expect((appliedVisualizationState as { applied: boolean }).applied).toBe(true); - expect( - (appliedDatasourceLayers?.first as DatasourcePublicAPI & { applied: boolean }).applied - ).toBe(true); + instance.update(); - // START NEW SCENARIO — make sure it switches back to using working state when auto-apply is reenabled + // allows initial render + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={datasource} + | testVis" + `); + + mockDatasource.toExpression.mockReturnValue('new-datasource'); + instance.setProps({ + visualizationMap: { + testVis: { ...mockVisualization, toExpression: () => 'new-vis' }, + }, + }); + + // nothing should change + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={datasource} + | testVis" + `); - // have to do this part manually in the test, but in the application it will happen in the frame API selector which will be called in the parent component tree + mounted.lensStore.dispatch(applyChanges()); + instance.update(); + + // should update + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={new-datasource} + | new-vis" + `); + + mockDatasource.toExpression.mockReturnValue('other-new-datasource'); instance.setProps({ - framePublicAPI: { ...framePublicAPI, appliedDatasourceLayers: undefined }, + visualizationMap: { + testVis: { ...mockVisualization, toExpression: () => 'other-new-vis' }, + }, }); - toExpression.mockClear(); + + // should not update + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={new-datasource} + | new-vis" + `); mounted.lensStore.dispatch(enableAutoApply()); + instance.update(); - expect(toExpression).toHaveBeenCalledTimes(1); - const [visualizationState, datasourceLayers] = toExpression.mock.calls[0]; - expect((visualizationState as { applied: boolean }).applied).toBeUndefined(); - expect( - (datasourceLayers?.first as DatasourcePublicAPI & { applied: boolean }).applied - ).toBeUndefined(); + // reenabling auto-apply triggers an update as well + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={other-new-datasource} + | other-new-vis" + `); + }); + + it('should base saveability on working changes when auto-apply disabled', async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + mockVisualization.getErrorMessages.mockImplementation((currentVisualizationState: any) => { + if (currentVisualizationState.hasProblem) { + return [{ shortMessage: 'An error occurred', longMessage: 'An long description here' }]; + } else { + return []; + } + }); + + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + mockDatasource.toExpression.mockReturnValue('datasource'); + mockDatasource.getLayers.mockReturnValue(['first']); + + const mounted = await mountWithProvider( + 'testVis' }, + }} + ExpressionRenderer={expressionRendererMock} + /> + ); + + instance = mounted.instance; + const isSaveable = () => mounted.lensStore.getState().lens.isSaveable; + + instance.update(); + + // allows initial render + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={datasource} + | testVis" + `); + expect(isSaveable()).toBe(true); + + act(() => { + mounted.lensStore.dispatch( + updateVisualizationState({ + visualizationId: 'testVis', + newState: { activeId: 'testVis', hasProblem: true }, + }) + ); + }); + instance.update(); + + expect(isSaveable()).toBe(false); + }); + + it('should allow empty workspace as initial render when auto-apply disabled', async () => { + mockVisualization.toExpression.mockReturnValue('testVis'); + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + + const mounted = await mountWithProvider( + , + { + preloadedState: { + autoApplyDisabled: true, + }, + } + ); + + instance = mounted.instance; + + expect(instance.exists('[data-test-subj="empty-workspace"]')).toBeTruthy(); }); it('should execute a trigger on expression event', async () => { @@ -371,6 +472,7 @@ describe('workspace_panel', () => { } ); instance = mounted.instance; + instance.update(); const ast = fromExpression(instance.find(expressionRendererMock).prop('expression') as string); @@ -654,6 +756,9 @@ describe('workspace_panel', () => { /> ); instance = mounted.instance; + act(() => { + instance.update(); + }); expect(instance.find('[data-test-subj="configuration-failure"]').exists()).toBeTruthy(); expect(instance.find(expressionRendererMock)).toHaveLength(0); @@ -739,8 +844,8 @@ describe('workspace_panel', () => { }); mockDatasource.getLayers.mockReturnValue(['first']); // eslint-disable-next-line @typescript-eslint/no-explicit-any - mockVisualization.getErrorMessages.mockImplementation((currentDatasourceState: any) => { - if (currentDatasourceState.hasProblem) { + mockVisualization.getErrorMessages.mockImplementation((currentVisualizationState: any) => { + if (currentVisualizationState.hasProblem) { return [{ shortMessage: 'An error occurred', longMessage: 'An long description here' }]; } else { return []; @@ -779,7 +884,7 @@ describe('workspace_panel', () => { expect(showingErrors()).toBeFalsy(); - // introduce some issues in working state + // introduce some issues lensStore.dispatch( updateDatasourceState({ datasourceId: 'testDatasource', diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index c98b40c275b03..80d7c8a611360 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -38,7 +38,6 @@ import { DatasourceMap, DatasourceFixAction, Suggestion, - Visualization, } from '../../../types'; import { DragDrop, DragContext, DragDropIdentifier } from '../../../drag_drop'; import { switchToSuggestion } from '../suggestion_helpers'; @@ -67,14 +66,12 @@ import { selectDatasourceStates, selectActiveDatasourceId, selectSearchSessionId, - selectAppliedState, - VisualizationState, - DatasourceStates, - AppliedState, - selectChangesApplied, + selectAutoApplyEnabled, + selectTriggerApplyChanges, } from '../../../state_management'; import type { LensInspector } from '../../../lens_inspector_service'; import { inferTimeField } from '../../../utils'; +import { setChangesApplied } from '../../../state_management/lens_slice'; export interface WorkspacePanelProps { visualizationMap: VisualizationMap; @@ -94,6 +91,7 @@ interface WorkspaceState { fixAction?: DatasourceFixAction; }>; expandError: boolean; + initialRenderComplete: boolean; } const dropProps = { @@ -124,74 +122,50 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac ); }); -/** - * This function returns the appropriate arguments depending on whether - * or not appliedState is passed in - */ -const getBuildExpressionArgs = ({ - appliedState, - visualization, - datasourceStates, - activeDatasourceId, - framePublicAPI: { datasourceLayers, appliedDatasourceLayers }, +let expressionToRender: null | undefined | string; + +// Exported for testing purposes only. +export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ + framePublicAPI, visualizationMap, -}: { - appliedState?: AppliedState; - visualization: VisualizationState; - datasourceStates: DatasourceStates; - activeDatasourceId: string | null; - framePublicAPI: FramePublicAPI; - visualizationMap: VisualizationMap; -}) => { - const visualizationState = appliedState?.visualization || visualization; - return { - visualization: visualizationState, - datasourceStates: appliedState?.datasourceStates || datasourceStates, - activeDatasourceId: appliedState?.activeDatasourceId || activeDatasourceId, - datasourceLayers: appliedDatasourceLayers || datasourceLayers, - activeVisualization: visualization.activeId - ? visualizationMap[visualizationState.activeId as string] - : null, - }; -}; + datasourceMap, + core, + plugins, + ExpressionRenderer: ExpressionRendererComponent, + suggestionForDraggedField, + lensInspector, +}: Omit & { + suggestionForDraggedField: Suggestion | undefined; +}) { + const dispatchLens = useLensDispatch(); + const isFullscreen = useLensSelector(selectIsFullscreenDatasource); + const visualization = useLensSelector(selectVisualization); + const activeDatasourceId = useLensSelector(selectActiveDatasourceId); + const datasourceStates = useLensSelector(selectDatasourceStates); + const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); + const triggerApply = useLensSelector(selectTriggerApplyChanges); -interface ErrorDescription { - shortMessage: string; - longMessage: React.ReactNode; -} + const [localState, setLocalState] = useState({ + expressionBuildError: undefined, + expandError: false, + initialRenderComplete: false, + }); -const checkConfiguration = ({ - activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, - datasourceMap, - datasourceLayers, -}: { - activeVisualization: Visualization | null; - visualization: VisualizationState; - activeDatasourceId: string | null; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; - datasourceLayers: FramePublicAPI['datasourceLayers']; -}) => - validateDatasourceAndVisualization( - activeDatasourceId ? datasourceMap[activeDatasourceId] : null, - activeDatasourceId && datasourceStates[activeDatasourceId]?.state, - activeVisualization, - visualization.state, - { datasourceLayers } - ); + useEffect(() => { + return () => { + expressionToRender = null; + }; + }, []); + + const shouldApplyExpression = + autoApplyEnabled || !localState.initialRenderComplete || triggerApply; + + const { datasourceLayers } = framePublicAPI; + + const activeVisualization = visualization.activeId + ? visualizationMap[visualization.activeId] + : null; -const checkMissingRefs = ({ - activeDatasourceId, - datasourceMap, - datasourceStates, -}: { - activeDatasourceId: string | null; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; -}) => { const missingIndexPatterns = getMissingIndexPattern( activeDatasourceId ? datasourceMap[activeDatasourceId] : null, activeDatasourceId ? datasourceStates[activeDatasourceId] : null @@ -213,264 +187,93 @@ const checkMissingRefs = ({ ] : []; - return missingRefsErrors; -}; - -const checkUnknownVis = ( - visualization: VisualizationState, - activeVisualization: Visualization | null -) => { - return visualization.activeId && !activeVisualization; -}; - -const generateExpression = ({ - errors: { hasConfigurationValidationError, hasMissingRefsErrors, hasUnknownVisError }, - activeVisualization, - visualization, - datasourceMap, - datasourceStates, - datasourceLayers, -}: { - errors: { - hasConfigurationValidationError: boolean; - hasMissingRefsErrors: boolean; - hasUnknownVisError: boolean; - }; - activeVisualization: Visualization | null; - visualization: VisualizationState; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; - datasourceLayers: FramePublicAPI['datasourceLayers']; -}) => { - let expression; - let expressionBuildError: ErrorDescription[] | undefined; - if (!hasConfigurationValidationError && !hasMissingRefsErrors && !hasUnknownVisError) { - try { - const ast = buildExpression({ - visualization: activeVisualization, - visualizationState: visualization.state, - datasourceMap, - datasourceStates, - datasourceLayers, - }); - - if (ast) { - // expression has to be turned into a string for dirty checking - if the ast is rebuilt, - // turning it into a string will make sure the expression renderer only re-renders if the - // expression actually changed. - expression = toExpression(ast); - } else { - expression = null; - } - } catch (e) { - const buildMessages = activeVisualization?.getErrorMessages(visualization.state); - const defaultMessage = { - shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { - defaultMessage: 'An unexpected error occurred while preparing the chart', - }), - longMessage: e.toString(), - }; - // Most likely an error in the expression provided by a datasource or visualization - expressionBuildError = buildMessages ?? [defaultMessage]; - } - } - if (hasUnknownVisError) { - expressionBuildError = [getUnknownVisualizationTypeError(visualization.activeId!)]; - } - - return { expression, expressionBuildError }; -}; - -/** - * Checks whether the user's unapplied changes are saveable (can generate a valid expression) - * - * Since state is applied when the user saves the visualization, the save button - * should be disabled when the visualization configuration that would actually be saved is invalid - */ -const areUnappliedChangesSaveable = ({ - _visualization, - _datasourceStates, - _activeDatasourceId, - visualizationMap, - datasourceMap, - framePublicAPI, -}: { - _visualization: VisualizationState; - _datasourceStates: DatasourceStates; - _activeDatasourceId: string | null; - visualizationMap: VisualizationMap; - datasourceMap: DatasourceMap; - framePublicAPI: FramePublicAPI; -}) => { - const unappliedArgs = getBuildExpressionArgs({ - appliedState: undefined, // get the UNAPPLIED state args - visualization: _visualization, - datasourceStates: _datasourceStates, - activeDatasourceId: _activeDatasourceId, - framePublicAPI, - visualizationMap, - }); - - const { activeDatasourceId, datasourceStates, visualization, activeVisualization } = - unappliedArgs; - - const configurationValidationError = checkConfiguration({ - activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, - datasourceMap, - datasourceLayers: framePublicAPI.datasourceLayers, - }); - - const missingRefsErrors = checkMissingRefs({ - activeDatasourceId, - datasourceStates, - datasourceMap, - }); - - const unknownVisError = checkUnknownVis(visualization, activeVisualization); - - const { expression: workingExpression } = generateExpression({ - ...unappliedArgs, - datasourceMap, - errors: { - hasConfigurationValidationError: Boolean(configurationValidationError?.length), - hasMissingRefsErrors: Boolean(missingRefsErrors.length), - hasUnknownVisError: Boolean(unknownVisError), - }, - }); - return Boolean(workingExpression); -}; - -const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ - framePublicAPI, - visualizationMap, - datasourceMap, - core, - plugins, - ExpressionRenderer: ExpressionRendererComponent, - suggestionForDraggedField, - lensInspector, -}: Omit & { - suggestionForDraggedField: Suggestion | undefined; -}) { - const dispatchLens = useLensDispatch(); - const isFullscreen = useLensSelector(selectIsFullscreenDatasource); - - // The following variables shouldn't be used for logic in this component - // Use the non-underscore versions instead, because they will reflect the applied state - // even when the user has unapplied changes - const _visualization = useLensSelector(selectVisualization); - const _datasourceStates = useLensSelector(selectDatasourceStates); - const _activeDatasourceId = useLensSelector(selectActiveDatasourceId); - const _appliedState = useLensSelector(selectAppliedState); - - const { - visualization, - datasourceStates, - activeDatasourceId, - datasourceLayers, - activeVisualization, - } = getBuildExpressionArgs({ - visualization: _visualization, - datasourceStates: _datasourceStates, - activeDatasourceId: _activeDatasourceId, - appliedState: _appliedState, - visualizationMap, - framePublicAPI, - }); - - const [localState, setLocalState] = useState({ - expressionBuildError: undefined, - expandError: false, - }); + const unknownVisError = visualization.activeId && !activeVisualization; // Note: mind to all these eslint disable lines: the frameAPI will change too frequently // and to prevent race conditions it is ok to leave them there. const configurationValidationError = useMemo( () => - checkConfiguration({ + validateDatasourceAndVisualization( + activeDatasourceId ? datasourceMap[activeDatasourceId] : null, + activeDatasourceId && datasourceStates[activeDatasourceId]?.state, activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, - datasourceMap, - datasourceLayers, - }), + visualization.state, + framePublicAPI + ), // eslint-disable-next-line react-hooks/exhaustive-deps [activeVisualization, visualization.state, activeDatasourceId, datasourceMap, datasourceStates] ); - const missingRefsErrors = checkMissingRefs({ - activeDatasourceId, - datasourceMap, - datasourceStates, - }); - const unknownVisError = checkUnknownVis(visualization, activeVisualization); - - const expression = useMemo(() => { - const { expression: _expression, expressionBuildError } = generateExpression({ - errors: { - hasConfigurationValidationError: Boolean(configurationValidationError?.length), - hasMissingRefsErrors: Boolean(missingRefsErrors?.length), - hasUnknownVisError: Boolean(unknownVisError), - }, - activeVisualization, - visualization, - datasourceMap, - datasourceStates, - datasourceLayers, - }); - - if (expressionBuildError) { - setLocalState((state) => ({ ...state, expressionBuildError })); - } + const _expression = useMemo(() => { + if (!configurationValidationError?.length && !missingRefsErrors.length && !unknownVisError) { + try { + const ast = buildExpression({ + visualization: activeVisualization, + visualizationState: visualization.state, + datasourceMap, + datasourceStates, + datasourceLayers, + }); - return _expression; + if (ast) { + // expression has to be turned into a string for dirty checking - if the ast is rebuilt, + // turning it into a string will make sure the expression renderer only re-renders if the + // expression actually changed. + return toExpression(ast); + } else { + return null; + } + } catch (e) { + const buildMessages = activeVisualization?.getErrorMessages(visualization.state); + const defaultMessage = { + shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { + defaultMessage: 'An unexpected error occurred while preparing the chart', + }), + longMessage: e.toString(), + }; + // Most likely an error in the expression provided by a datasource or visualization + setLocalState((s) => ({ + ...s, + expressionBuildError: buildMessages ?? [defaultMessage], + })); + } + } + if (unknownVisError) { + setLocalState((s) => ({ + ...s, + expressionBuildError: [getUnknownVisualizationTypeError(visualization.activeId!)], + })); + } }, [ - configurationValidationError?.length, - missingRefsErrors?.length, - unknownVisError, activeVisualization, - visualization, + visualization.state, datasourceMap, datasourceStates, datasourceLayers, - ]); - - const expressionExists = Boolean(expression); - - const changesApplied = useLensSelector(selectChangesApplied); - - const isSaveable = useMemo(() => { - if (changesApplied) { - return expressionExists; - } else { - return areUnappliedChangesSaveable({ - _visualization, - _datasourceStates, - _activeDatasourceId, - visualizationMap, - datasourceMap, - framePublicAPI, - }); - } - }, [ - _activeDatasourceId, - _datasourceStates, - _visualization, - changesApplied, - datasourceMap, - expressionExists, - framePublicAPI, - visualizationMap, + configurationValidationError?.length, + missingRefsErrors.length, + unknownVisError, + visualization.activeId, ]); useEffect(() => { - dispatchLens(setSaveable(isSaveable)); - }, [isSaveable, dispatchLens]); + dispatchLens(setSaveable(Boolean(_expression))); + }, [_expression, dispatchLens]); + + if (shouldApplyExpression) { + expressionToRender = _expression; + } + + if (!autoApplyEnabled) { + dispatchLens(setChangesApplied(_expression === expressionToRender)); + } + + const expressionExists = Boolean(expressionToRender); + // null signals an empty workspace which should count as an initial render + if ((expressionExists || expressionToRender === null) && !localState.initialRenderComplete) { + setLocalState((s) => ({ ...s, initialRenderComplete: true })); + } const onEvent = useCallback( (event: ExpressionRendererEvent) => { @@ -572,12 +375,12 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }; const renderVisualization = () => { - if (expression === null) { + if (expressionToRender === null) { return renderEmptyWorkspace(); } return ( { let mockVisualization: jest.Mocked; @@ -168,12 +170,19 @@ describe('workspace_panel_wrapper', () => { newState: { something: 'changed' }, }) ); + // simulate workspace panel behavior + store.dispatch(setChangesApplied(false)); harness.update(); expect(harness.applyChangesDisabled).toBeFalsy(); harness.applyChanges(); + expect(selectTriggerApplyChanges(store.getState())).toBeTruthy(); + // simulate workspace panel behavior + store.dispatch(setChangesApplied(true)); + harness.update(); + expect(harness.applyChangesDisabled).toBeTruthy(); }); @@ -186,6 +195,7 @@ describe('workspace_panel_wrapper', () => { newState: { something: 'changed' }, }) ); + store.dispatch(setChangesApplied(false)); // simulate workspace panel behavior harness.update(); expect(harness.applyChangesDisabled).toBeFalsy(); diff --git a/x-pack/plugins/lens/public/state_management/lens_slice.test.ts b/x-pack/plugins/lens/public/state_management/lens_slice.test.ts index 501787ecf135b..4a183c11d896b 100644 --- a/x-pack/plugins/lens/public/state_management/lens_slice.test.ts +++ b/x-pack/plugins/lens/public/state_management/lens_slice.test.ts @@ -17,12 +17,13 @@ import { removeOrClearLayer, addLayer, LensRootStore, - DatasourceStates, + selectTriggerApplyChanges, + selectChangesApplied, } from '.'; import { layerTypes } from '../../common'; import { makeLensStore, defaultState, mockStoreDeps } from '../mocks'; import { DatasourceMap, VisualizationMap } from '../types'; -import { applyChanges, disableAutoApply, enableAutoApply } from './lens_slice'; +import { applyChanges, disableAutoApply, enableAutoApply, setChangesApplied } from './lens_slice'; import { LensAppState } from './types'; describe('lensSlice', () => { @@ -43,59 +44,51 @@ describe('lensSlice', () => { describe('auto-apply-related actions', () => { it('should disable auto apply', () => { - expect(store.getState().lens.appliedState).toBeUndefined(); + expect(store.getState().lens.autoApplyDisabled).toBeUndefined(); + expect(store.getState().lens.changesApplied).toBeUndefined(); store.dispatch(disableAutoApply()); - const newState = store.getState().lens; - - expect(newState.appliedState).toBeDefined(); - expect(newState.appliedState).toEqual({ - activeDatasourceId: newState.activeDatasourceId, - visualization: newState.visualization, - datasourceStates: newState.datasourceStates, - }); - // check for deep copy - expect(newState.appliedState?.visualization).not.toBe(newState.visualization); - expect(newState.appliedState?.datasourceStates).not.toBe(newState.datasourceStates); + expect(store.getState().lens.autoApplyDisabled).toBe(true); + expect(store.getState().lens.changesApplied).toBe(true); }); it('should enable auto-apply', () => { store.dispatch(disableAutoApply()); - expect(store.getState().lens.appliedState).toBeDefined(); + expect(store.getState().lens.autoApplyDisabled).toBe(true); store.dispatch(enableAutoApply()); - expect(store.getState().lens.appliedState).toBeUndefined(); + expect(store.getState().lens.autoApplyDisabled).toBe(false); }); it('applies changes when auto-apply disabled', () => { store.dispatch(disableAutoApply()); - const newState = { - visualization: { activeId: 'fooid', state: { foo: 'bar' } }, - datasourceStates: { another: 'foo' } as unknown as DatasourceStates, - }; - - store.dispatch(setState(newState)); - store.dispatch(applyChanges()); - const stateAfterApply = store.getState().lens; - expect(stateAfterApply.appliedState?.visualization).toEqual(newState.visualization); - expect(stateAfterApply.appliedState?.datasourceStates).toEqual(newState.datasourceStates); - // check for deep copy - expect(stateAfterApply.appliedState?.visualization).not.toBe(newState.visualization); - expect(stateAfterApply.appliedState?.datasourceStates).not.toBe(newState.datasourceStates); + expect(selectTriggerApplyChanges(store.getState())).toBe(true); }); it('does not apply changes if auto-apply enabled', () => { - expect(store.getState().lens.appliedState).toBeUndefined(); + expect(store.getState().lens.autoApplyDisabled).toBeUndefined(); store.dispatch(applyChanges()); - expect(store.getState().lens.appliedState).toBeUndefined(); + expect(selectTriggerApplyChanges(store.getState())).toBe(false); + }); + + it('sets changes-applied flag', () => { + expect(store.getState().lens.changesApplied).toBeUndefined(); + + store.dispatch(setChangesApplied(true)); + + expect(selectChangesApplied(store.getState())).toBe(true); + + store.dispatch(setChangesApplied(false)); + + expect(selectChangesApplied(store.getState())).toBe(true); }); }); diff --git a/x-pack/plugins/lens/public/state_management/lens_slice.ts b/x-pack/plugins/lens/public/state_management/lens_slice.ts index 232f17895a18c..56ff89f506c85 100644 --- a/x-pack/plugins/lens/public/state_management/lens_slice.ts +++ b/x-pack/plugins/lens/public/state_management/lens_slice.ts @@ -7,7 +7,7 @@ import { createAction, createReducer, current, PayloadAction } from '@reduxjs/toolkit'; import { VisualizeFieldContext } from 'src/plugins/ui_actions/public'; -import { cloneDeep, mapValues } from 'lodash'; +import { mapValues } from 'lodash'; import { History } from 'history'; import { LensEmbeddableInput } from '..'; import { getDatasourceLayers } from '../editor_frame_service/editor_frame'; @@ -85,8 +85,8 @@ export const onActiveDataChange = createAction('lens/onAc export const setSaveable = createAction('lens/setSaveable'); export const enableAutoApply = createAction('lens/enableAutoApply'); export const disableAutoApply = createAction('lens/disableAutoApply'); -// this action is only relevant when auto-apply is disabled export const applyChanges = createAction('lens/applyChanges'); +export const setChangesApplied = createAction('lens/setChangesApplied'); export const updateState = createAction<{ updater: (prevState: LensAppState) => LensAppState; }>('lens/updateState'); @@ -169,6 +169,7 @@ export const lensActions = { enableAutoApply, disableAutoApply, applyChanges, + setChangesApplied, updateState, updateDatasourceState, updateVisualizationState, @@ -188,14 +189,6 @@ export const lensActions = { setLayerDefaultDimension, }; -const buildAppliedState = (state: LensAppState) => { - return { - activeDatasourceId: state.activeDatasourceId, - visualization: cloneDeep(state.visualization), - datasourceStates: cloneDeep(state.datasourceStates), - }; -}; - export const makeLensReducer = (storeDeps: LensStoreDeps) => { const { datasourceMap, visualizationMap } = storeDeps; return createReducer(initialState, { @@ -218,15 +211,20 @@ export const makeLensReducer = (storeDeps: LensStoreDeps) => { }; }, [enableAutoApply.type]: (state) => { - delete state.appliedState; + state.autoApplyDisabled = false; }, [disableAutoApply.type]: (state) => { - state.appliedState = buildAppliedState(state); + state.autoApplyDisabled = true; + state.changesApplied = true; }, [applyChanges.type]: (state) => { - if (state.appliedState) { - state.appliedState = buildAppliedState(state); + if (typeof state.applyChangesCounter === 'undefined') { + state.applyChangesCounter = 0; } + state.applyChangesCounter!++; + }, + [setChangesApplied.type]: (state, { payload: applied }) => { + state.changesApplied = applied; }, [updateState.type]: ( state, diff --git a/x-pack/plugins/lens/public/state_management/selectors.test.ts b/x-pack/plugins/lens/public/state_management/selectors.test.ts index bed8c46458360..2313d341b7e03 100644 --- a/x-pack/plugins/lens/public/state_management/selectors.test.ts +++ b/x-pack/plugins/lens/public/state_management/selectors.test.ts @@ -5,114 +5,45 @@ * 2.0. */ -import { LensAppState, selectAppliedState, selectChangesApplied } from '.'; +import { LensAppState, selectTriggerApplyChanges, selectChangesApplied } from '.'; describe('lens selectors', () => { - it('should select applied state', () => { - const lensState = { - appliedState: { - activeDatasourceId: 'foobar', - visualization: { - activeId: 'some-id', - state: {}, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: {}, - }, - }, - }, - } as Partial; - - expect(selectAppliedState({ lens: lensState as LensAppState })).toStrictEqual( - lensState.appliedState - ); - }); - describe('selecting changes applied', () => { - it('should be true when applied state matches working state', () => { + it('should be true when auto-apply disabled and flag is set', () => { const lensState = { - activeDatasourceId: 'foobar', - visualization: { - activeId: 'some-id', - state: {}, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: {}, - }, - }, - appliedState: { - activeDatasourceId: 'foobar', - visualization: { - activeId: 'some-id', - state: {}, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: {}, - }, - }, - }, + changesApplied: true, + autoApplyDisabled: true, } as Partial; expect(selectChangesApplied({ lens: lensState as LensAppState })).toBeTruthy(); }); - it('should be true when no applied state (auto-apply enabled)', () => { + it('should be false when auto-apply disabled and flag is false', () => { const lensState = { - visualization: { - activeId: 'some-id', - state: {}, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: {}, - }, - }, - appliedState: undefined, + changesApplied: false, + autoApplyDisabled: true, } as Partial; - expect(selectChangesApplied({ lens: lensState as LensAppState })).toBeTruthy(); + expect(selectChangesApplied({ lens: lensState as LensAppState })).toBeFalsy(); }); - it('should be false when applied state differs from working state', () => { + it('should be true when auto-apply enabled no matter what', () => { const lensState = { - activeDatasourceId: 'foobar', - visualization: { - activeId: 'some-other-id', - state: { - something: 'changed', - }, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: { - something: 'changed', - }, - }, - }, - appliedState: { - activeDatasourceId: 'foobar', - visualization: { - activeId: 'some-id', - state: {}, - }, - datasourceStates: { - indexpattern: { - isLoading: false, - state: {}, - }, - }, - }, - } as unknown as LensAppState; + changesApplied: false, + autoApplyDisabled: false, + } as Partial; - expect(selectChangesApplied({ lens: lensState as LensAppState })).toBeFalsy(); + expect(selectChangesApplied({ lens: lensState as LensAppState })).toBeTruthy(); }); }); + it('should select apply changes trigger', () => { + selectTriggerApplyChanges({ lens: { applyChangesCounter: 1 } as LensAppState }); // get the counters in sync + + expect( + selectTriggerApplyChanges({ lens: { applyChangesCounter: 2 } as LensAppState }) + ).toBeTruthy(); + expect( + selectTriggerApplyChanges({ lens: { applyChangesCounter: 2 } as LensAppState }) + ).toBeFalsy(); + }); }); diff --git a/x-pack/plugins/lens/public/state_management/selectors.ts b/x-pack/plugins/lens/public/state_management/selectors.ts index a183197109739..26a0d70d068f5 100644 --- a/x-pack/plugins/lens/public/state_management/selectors.ts +++ b/x-pack/plugins/lens/public/state_management/selectors.ts @@ -8,9 +8,8 @@ import { createSelector } from '@reduxjs/toolkit'; import { SavedObjectReference } from 'kibana/server'; import { FilterManager } from 'src/plugins/data/public'; -import { isEqual } from 'lodash'; import { LensState } from './types'; -import { Datasource, DatasourceMap, FramePublicAPI, VisualizationMap } from '../types'; +import { Datasource, DatasourceMap, VisualizationMap } from '../types'; import { getDatasourceLayers } from '../editor_frame_service/editor_frame'; export const selectPersistedDoc = (state: LensState) => state.lens.persistedDoc; @@ -20,27 +19,21 @@ export const selectFilters = (state: LensState) => state.lens.filters; export const selectResolvedDateRange = (state: LensState) => state.lens.resolvedDateRange; export const selectVisualization = (state: LensState) => state.lens.visualization; export const selectStagedPreview = (state: LensState) => state.lens.stagedPreview; -export const selectAppliedState = (state: LensState) => state.lens.appliedState; +export const selectAutoApplyEnabled = (state: LensState) => !state.lens.autoApplyDisabled; +export const selectChangesApplied = (state: LensState) => + !state.lens.autoApplyDisabled || Boolean(state.lens.changesApplied); export const selectDatasourceStates = (state: LensState) => state.lens.datasourceStates; export const selectActiveDatasourceId = (state: LensState) => state.lens.activeDatasourceId; export const selectActiveData = (state: LensState) => state.lens.activeData; export const selectIsFullscreenDatasource = (state: LensState) => Boolean(state.lens.isFullscreenDatasource); -export const selectChangesApplied = createSelector( - [selectAppliedState, selectVisualization, selectDatasourceStates, selectActiveDatasourceId], - (appliedState, visualization, datasourceStates, activeDatasourceId) => { - if (!appliedState) return true; // auto-apply is enabled - - const workingState = { activeDatasourceId, visualization, datasourceStates }; - return isEqual(appliedState, workingState); - } -); - -export const selectAutoApplyEnabled = createSelector( - [selectAppliedState], - (appliedState) => !Boolean(appliedState) -); +let applyChangesCounter: number | undefined; +export const selectTriggerApplyChanges = (state: LensState) => { + const shouldApply = state.lens.applyChangesCounter !== applyChangesCounter; + applyChangesCounter = state.lens.applyChangesCounter; + return shouldApply; +}; export const selectExecutionContext = createSelector( [selectQuery, selectFilters, selectResolvedDateRange], @@ -167,23 +160,13 @@ export const selectDatasourceLayers = createSelector( export const selectFramePublicAPI = createSelector( [ selectDatasourceStates, - selectAppliedState, selectActiveData, selectInjectedDependencies as SelectInjectedDependenciesFunction, ], - (datasourceStates, appliedState, activeData, datasourceMap) => { - const api: FramePublicAPI = { + (datasourceStates, activeData, datasourceMap) => { + return { datasourceLayers: getDatasourceLayers(datasourceStates, datasourceMap), activeData, }; - - if (appliedState) { - api.appliedDatasourceLayers = getDatasourceLayers( - appliedState.datasourceStates, - datasourceMap - ); - } - - return api; } ); diff --git a/x-pack/plugins/lens/public/state_management/types.ts b/x-pack/plugins/lens/public/state_management/types.ts index 38cc580cf525d..0c902f944072d 100644 --- a/x-pack/plugins/lens/public/state_management/types.ts +++ b/x-pack/plugins/lens/public/state_management/types.ts @@ -30,15 +30,12 @@ export interface PreviewState { visualization: VisualizationState; datasourceStates: DatasourceStates; } -export interface AppliedState { - activeDatasourceId: EditorFrameState['activeDatasourceId']; - visualization: VisualizationState; - datasourceStates: DatasourceStates; -} export interface EditorFrameState extends PreviewState { activeDatasourceId: string | null; stagedPreview?: PreviewState; - appliedState?: AppliedState; + autoApplyDisabled?: boolean; + applyChangesCounter?: number; + changesApplied?: boolean; isFullscreenDatasource?: boolean; } export interface LensAppState extends EditorFrameState {