Skip to content

[lens] immediately display loading panel when adding new ES|QL panel #209361

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.

Sign up for GitHub

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

Closed
nreese wants to merge 4 commits into from
Closed

[lens] immediately display loading panel when adding new ES|QL panel #209361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3a6c4c
[lens] immediately display loading panel when adding new ES|QL panel
nreese Feb 3, 2025
c2c961a
cleanup
nreese Feb 3, 2025
223ac25
clean up
nreese Feb 3, 2025
5991b64
tslint
nreese Feb 3, 2025
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
15 changes: 8 additions & 7 deletions x-pack/platform/plugins/shared/lens/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,13 @@ export class LensPlugin {
datasourceMap,
theme: core.theme,
uiSettings: core.uiSettings,
getEditorFrameService: async () => {
if (!this.editorFrameService) {
await this.initEditorFrameService();
}

return this.editorFrameService!;
},
};
};

Expand Down Expand Up @@ -683,13 +690,7 @@ export class LensPlugin {
);

// Displays the add ESQL panel in the dashboard add Panel menu
const createESQLPanelAction = new CreateESQLPanelAction(startDependencies, core, async () => {
if (!this.editorFrameService) {
await this.initEditorFrameService();
}

return this.editorFrameService!;
});
const createESQLPanelAction = new CreateESQLPanelAction(core);
startDependencies.uiActions.addTriggerAction(ADD_PANEL_TRIGGER, createESQLPanelAction);

const discoverLocator = startDependencies.share?.url.locators.get('DISCOVER_APP_LOCATOR');
Expand Down
2 changes: 1 addition & 1 deletion x-pack/platform/plugins/shared/lens/public/react_embeddable/data_loader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ async function expectRerenderOnDataLoder(
parentApi,
};
const getState = jest.fn(() => runtimeState);
const internalApi = getLensInternalApiMock();
const internalApi = await getLensInternalApiMock();
const services = makeEmbeddableServices(new BehaviorSubject<string>(''), undefined, {
visOverrides: { id: 'lnsXY' },
dataOverrides: { id: 'form_based' },
Expand Down
12 changes: 6 additions & 6 deletions ...tform/plugins/shared/lens/public/react_embeddable/initializers/initialize_actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jest.mock('../../app_plugin/show_underlying_data', () => {
};
});

function setupActionsApi(
async function setupActionsApi(
stateOverrides?: Partial<LensRuntimeState>,
contextOverrides?: Omit<VisualizationContext, 'doc'>
) {
Expand All @@ -46,7 +46,7 @@ function setupActionsApi(
const runtimeState = getLensRuntimeStateMock(stateOverrides);
const apiMock = getLensApiMock();
// create the internal API and customize internal state
const internalApi = getLensInternalApiMock();
const internalApi = await getLensInternalApiMock();
internalApi.updateVisualizationContext({
...contextOverrides,
activeAttributes: runtimeState.attributes,
Expand Down Expand Up @@ -74,12 +74,12 @@ function setupActionsApi(
describe('Dashboard actions', () => {
describe('Drilldowns', () => {
it('should expose drilldowns for DSL based visualization', async () => {
const api = setupActionsApi();
const api = await setupActionsApi();
expect(api.enhancements).toBeDefined();
});

it('should not expose drilldowns for ES|QL chart types', async () => {
const api = setupActionsApi(
const api = await setupActionsApi(
createEmptyLensState('lnsXY', faker.lorem.words(), faker.lorem.text(), {
esql: 'FROM index',
})
Expand Down Expand Up @@ -120,13 +120,13 @@ describe('Dashboard actions', () => {
activeData: {},
};
it('should expose the "explore in discover" capability for DSL based visualization when compatible', async () => {
const api = setupActionsApi(undefined, visualizationContextMockOverrides);
const api = await setupActionsApi(undefined, visualizationContextMockOverrides);
api.loadViewUnderlyingData();
expect(api.canViewUnderlyingData$.getValue()).toBe(true);
});

it('should expose the "explore in discover" capability for ES|QL chart types', async () => {
const api = setupActionsApi(
const api = await setupActionsApi(
createEmptyLensState('lnsXY', faker.lorem.words(), faker.lorem.text(), {
esql: 'FROM index',
}),
Expand Down
10 changes: 5 additions & 5 deletions ...ins/shared/lens/public/react_embeddable/initializers/initialize_dashboard_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ import { initializeDashboardServices } from './initialize_dashboard_services';
import { faker } from '@faker-js/faker';
import { createEmptyLensState } from '../helper';

function setupDashboardServicesApi(runtimeOverrides?: Partial<LensRuntimeState>) {
async function setupDashboardServicesApi(runtimeOverrides?: Partial<LensRuntimeState>) {
const services = makeEmbeddableServices();
const internalApiMock = getLensInternalApiMock();
const internalApiMock = await getLensInternalApiMock();
const runtimeState = getLensRuntimeStateMock(runtimeOverrides);
const stateManagementConfig = initializeStateManagement(runtimeState, internalApiMock);
const titleManager = initializeTitleManager(runtimeState);
Expand All @@ -33,18 +33,18 @@ function setupDashboardServicesApi(runtimeOverrides?: Partial<LensRuntimeState>)

describe('Transformation API', () => {
it("should not save to library if there's already a saveObjectId", async () => {
const api = setupDashboardServicesApi({ savedObjectId: faker.string.uuid() });
const api = await setupDashboardServicesApi({ savedObjectId: faker.string.uuid() });
expect(await api.canLinkToLibrary()).toBe(false);
});

it("should save to library if there's no saveObjectId declared", async () => {
const api = setupDashboardServicesApi();
const api = await setupDashboardServicesApi();
expect(await api.canLinkToLibrary()).toBe(true);
});

it('should not save to library for ES|QL chart types', async () => {
// setup a state with an ES|QL query
const api = setupDashboardServicesApi(
const api = await setupDashboardServicesApi(
createEmptyLensState('lnsXY', faker.lorem.words(), faker.lorem.text(), {
esql: 'FROM index',
})
Expand Down
12 changes: 6 additions & 6 deletions ...platform/plugins/shared/lens/public/react_embeddable/initializers/initialize_edit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import { BehaviorSubject } from 'rxjs';
import { ApplicationStart } from '@kbn/core/public';
import { LensEmbeddableStartServices } from '../types';

function createEditApi(servicesOverrides: Partial<LensEmbeddableStartServices> = {}) {
const internalApi = getLensInternalApiMock();
async function createEditApi(servicesOverrides: Partial<LensEmbeddableStartServices> = {}) {
const internalApi = await getLensInternalApiMock();
const runtimeState = getLensRuntimeStateMock();
const api = getLensApiMock();
const services = {
Expand All @@ -43,13 +43,13 @@ function createEditApi(servicesOverrides: Partial<LensEmbeddableStartServices> =
}

describe('edit features', () => {
it('should be editable if visualize library privileges allow it', () => {
const editApi = createEditApi();
it('should be editable if visualize library privileges allow it', async () => {
const editApi = await createEditApi();
expect(editApi.api.isEditingEnabled()).toBe(true);
});

it('should not be editable if visualize library privileges do not allow it', () => {
const editApi = createEditApi({
it('should not be editable if visualize library privileges do not allow it', async () => {
const editApi = await createEditApi({
capabilities: {
visualize_v2: {
// cannot save
Expand Down
28 changes: 23 additions & 5 deletions ...tform/plugins/shared/lens/public/react_embeddable/initializers/initialize_internal_api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import type {
import { apiHasAbortController, apiHasLensComponentProps } from '../type_guards';
import type { UserMessage } from '../../types';

export function initializeInternalApi(
export async function initializeInternalApi(
initialState: LensRuntimeState,
parentApi: unknown,
titleManager: ReturnType<typeof initializeTitleManager>,
{ visualizationMap }: LensEmbeddableStartServices
): LensInternalApi {
services: LensEmbeddableStartServices
): Promise<LensInternalApi> {
const [hasRenderCompleted$] = buildObservableVariable<boolean>(false);
const [expressionParams$] = buildObservableVariable<ExpressionWrapperProps | null>(null);
const expressionAbortController$ = new BehaviorSubject<AbortController | undefined>(undefined);
Expand All @@ -35,8 +35,26 @@ export function initializeInternalApi(
}
const [renderCount$] = buildObservableVariable<number>(0);

async function getInitialAttributes() {
if (initialState.attributes) {
return initialState.attributes;
}

if (initialState.isNewPanel) {
try {
const { createNewEsqlAttributes } = await import('../../async_services');
const attributes = await createNewEsqlAttributes(services);
if (attributes) return attributes;
} catch (error) {
// fallback to empty state
}
}

return createEmptyLensState().attributes;
}

const attributes$ = new BehaviorSubject<LensRuntimeState['attributes']>(
initialState.attributes || createEmptyLensState().attributes
await getInitialAttributes()
Copy link
Copy Markdown
Contributor

@dej611 dej611 Feb 4, 2025

Choose a reason for hiding this comment

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

While I agree on the final goal, I think the solution here is not exactly what I was expecting.
I would expect the initializers to be sync code that simply creates all the right things to watch before calling the loader at the end.

In the ES|QL scenario I understand there's some extra async preparation for this, so we're forcefully forwarding the async behaviour into the entire lens embeddable stack and make everything async.
Why not change this into a simpler flow?

  • add new panel const embeddable = addNewPanel(...)
  • call to update the attributes once done: embeddable.updateAttributes(esqlAttributes);
  • call to onEdit: embeddable.onEdit()

Maybe a the loading state until the update call is reached can be communicated in the parentApi somehow, but it would be a bit cleaner to me.
What do you think @nreese ?

Copy link
Copy Markdown
Contributor Author

@nreese nreese Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

buildEmbeddable is async because initializing an embeddable will likely require async actions. For example, many embeddables load a dataView.

When buildEmbeddable returns, the embeddable is initialized. I don't think it makes sense to add complexity to parentApi to add a work around for this.

For lens, we could keep initializeInternalApi sync and do something like the below. Would that resolve your concerns?

const internalApi = initializeInternalApi(
  initialState: {
    ...initialState,
    attributes: 
      initialState.isNewPanel
      ? await createNewEsqlAttributes(services)
      : initialState.attributes,
  },
  parentApi,
  titleManager,
  services
);

Copy link
Copy Markdown
Contributor

@dej611 dej611 Feb 4, 2025

Choose a reason for hiding this comment

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

What I mean is that we could move the await createNewEsqlAttributes(services) into the loader code. No?

Copy link
Copy Markdown
Contributor Author

@nreese nreese Feb 4, 2025

Choose a reason for hiding this comment

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

I am not familiar with loader code. Can you provide a file reference and maybe a pointer to where you are thinking. Some sample code would help

);
const overrides$ = new BehaviorSubject(initialState.overrides);
const disableTriggers$ = new BehaviorSubject(initialState.disableTriggers);
Expand Down Expand Up @@ -120,7 +138,7 @@ export function initializeInternalApi(
}

let displayOptions =
visualizationMap[latestAttributes.visualizationType]?.getDisplayOptions?.() ?? {};
services.visualizationMap[latestAttributes.visualizationType]?.getDisplayOptions?.() ?? {};

if (apiHasLensComponentProps(parentApi) && parentApi.noPadding != null) {
displayOptions = {
Expand Down
1 change: 1 addition & 0 deletions ...tform/plugins/shared/lens/public/react_embeddable/inline_editing/setup_inline_editing.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export function prepareInlineEditPanel(
| 'theme'
| 'uiSettings'
| 'attributeService'
| 'getEditorFrameService'
>,
navigateToLensEditor?: (
stateTransfer: EmbeddableStateTransfer,
Expand Down
7 changes: 6 additions & 1 deletion x-pack/platform/plugins/shared/lens/public/react_embeddable/lens_embeddable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ export const createLensEmbeddableFactory = (
* Observables and functions declared here are used internally to store mutating state values
* This is an internal API not exposed outside of the embeddable.
*/
const internalApi = initializeInternalApi(initialState, parentApi, titleManager, services);
const internalApi = await initializeInternalApi(
initialState,
parentApi,
titleManager,
services
);

/**
* Initialize various configurations required to build all the required
Expand Down
17 changes: 13 additions & 4 deletions x-pack/platform/plugins/shared/lens/public/react_embeddable/mocks/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import { createMockDatasource, createMockVisualization, makeDefaultServices } from '../../mocks';
import { Datasource, DatasourceMap, Visualization, VisualizationMap } from '../../types';
import { initializeInternalApi } from '../initializers/initialize_internal_api';
import { EditorFrameService } from '../../async_services';

function getDefaultLensApiMock() {
const LensApiMock: LensApi = {
Expand Down Expand Up @@ -206,6 +207,12 @@ export function makeEmbeddableServices(
} as unknown as ReactEmbeddableDynamicActionsApi)
),
},
getEditorFrameService: jest.fn(() =>
Promise.resolve({
loadVisualizations: jest.fn(),
loadDatasources: jest.fn(),
} as unknown as EditorFrameService)
),
};
}

Expand Down Expand Up @@ -276,9 +283,9 @@ export function getValidExpressionParams(
};
}

function getInternalApiWithFunctionWrappers() {
async function getInternalApiWithFunctionWrappers() {
const mockRuntimeState = getLensRuntimeStateMock();
const newApi = initializeInternalApi(
const newApi = await initializeInternalApi(
mockRuntimeState,
{},
initializeTitleManager(mockRuntimeState),
Expand All @@ -295,9 +302,11 @@ function getInternalApiWithFunctionWrappers() {
return newApi;
}

export function getLensInternalApiMock(overrides: Partial<LensInternalApi> = {}): LensInternalApi {
export async function getLensInternalApiMock(
overrides: Partial<LensInternalApi> = {}
): Promise<LensInternalApi> {
return {
...getInternalApiWithFunctionWrappers(),
...(await getInternalApiWithFunctionWrappers()),
...overrides,
};
}
Expand Down
12 changes: 6 additions & 6 deletions ...m/plugins/shared/lens/public/react_embeddable/renderer/lens_embeddable_component.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ jest.mock('../expression_wrapper', () => ({

type GetValueType<Type> = Type extends PublishingSubject<infer X> ? X : never;

function getDefaultProps({
async function getDefaultProps({
internalApiOverrides = undefined,
apiOverrides = undefined,
}: { internalApiOverrides?: Partial<LensInternalApi>; apiOverrides?: Partial<LensApi> } = {}) {
const internalApi = getLensInternalApiMock(internalApiOverrides);
const internalApi = await getLensInternalApiMock(internalApiOverrides);
// provide a valid expression to render
internalApi.updateExpressionParams(getValidExpressionParams());
return {
Expand All @@ -36,8 +36,8 @@ function getDefaultProps({
}

describe('Lens Embeddable component', () => {
it('should not render the visualization if any error arises', () => {
const props = getDefaultProps({
it('should not render the visualization if any error arises', async () => {
const props = await getDefaultProps({
internalApiOverrides: {
expressionParams$: new BehaviorSubject<GetValueType<LensInternalApi['expressionParams$']>>(
null
Expand All @@ -49,9 +49,9 @@ describe('Lens Embeddable component', () => {
expect(screen.queryByTestId('lens-embeddable')).not.toBeInTheDocument();
});

it('shoud not render the title if the visualization forces the title to be hidden', () => {
it('shoud not render the title if the visualization forces the title to be hidden', async () => {
const getDisplayOptions = jest.fn(() => ({ noPanelTitle: true }));
const props = getDefaultProps({
const props = await getDefaultProps({
internalApiOverrides: {
getDisplayOptions,
},
Expand Down
2 changes: 2 additions & 0 deletions x-pack/platform/plugins/shared/lens/public/react_embeddable/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ import type { FormBasedPersistedState } from '..';
import type { TextBasedPersistedState } from '../datasources/text_based/types';
import type { GaugeVisualizationState } from '../visualizations/gauge/constants';
import type { MetricVisualizationState } from '../visualizations/metric/types';
import type { EditorFrameService } from '../editor_frame_service';

// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface LensApiProps {}
Expand Down Expand Up @@ -145,6 +146,7 @@ export type LensEmbeddableStartServices = Simplify<
theme: ThemeServiceStart;
uiSettings: IUiSettingsClient;
attributeService: LensAttributesService;
getEditorFrameService: () => Promise<EditorFrameService>;
}
>;

Expand Down
Loading