Skip to content

Commit 53b41da

Browse files
momovdgstratoula
andauthored
[ES|QL] Create control flyout close button bug (#243365)
## Summary resolves: #243456 The flyout for 'create control' from the esql editor is not closing when clicking on the 'X' button. This PR address that bug and also incorporates this button to the telemetry tracking as a property when cancelling the creation of a control. EDIT: as an extra fix this pr addresses a small bug around telemetry when we click on save for a control on edit mode. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: Stratou <[email protected]>
1 parent 5695dcf commit 53b41da

File tree

7 files changed

+51
-19
lines changed

7 files changed

+51
-19
lines changed

src/platform/packages/shared/kbn-esql-types/src/esql_telemetry_types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@ export interface TelemetryQuerySubmittedProps {
2828
export enum ControlTriggerSource {
2929
SMART_SUGGESTION = 'smart_suggestion',
3030
QUESTION_MARK = 'question_mark',
31+
ADD_CONTROL_BTN = 'add_control_btn',
3132
}
3233

3334
export enum TelemetryControlCancelledReason {
3435
CANCEL_BUTTON = 'cancel_button',
36+
CLOSE_BUTTON = 'close_button',
3537
}

src/platform/plugins/shared/dashboard/public/dashboard_app/top_nav/add_menu/show_add_menu.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,12 @@ import {
2020
import type { CoreStart } from '@kbn/core/public';
2121
import { TIME_SLIDER_CONTROL } from '@kbn/controls-constants';
2222
import type { DefaultControlApi } from '@kbn/controls-plugin/public';
23-
import { ESQLVariableType, EsqlControlType, apiPublishesESQLVariables } from '@kbn/esql-types';
23+
import {
24+
ControlTriggerSource,
25+
ESQLVariableType,
26+
EsqlControlType,
27+
apiPublishesESQLVariables,
28+
} from '@kbn/esql-types';
2429
import { i18n } from '@kbn/i18n';
2530
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
2631
import { apiHasType, useStateFromPublishingSubject } from '@kbn/presentation-publishing';
@@ -211,6 +216,7 @@ export const AddMenu = ({ dashboardApi, anchorElement, coreServices }: AddMenuPr
211216
closePopover();
212217
},
213218
onCancelControl: closePopover,
219+
triggerSource: ControlTriggerSource.ADD_CONTROL_BTN,
214220
});
215221
} catch (e) {
216222
// eslint-disable-next-line no-console

src/platform/plugins/shared/esql/public/triggers/esql_controls/control_flyout/identifier_control_form.test.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ import { dataPluginMock } from '@kbn/data-plugin/public/mocks';
1717
import type { ESQLControlState } from '@kbn/esql-types';
1818
import { ControlTriggerSource, ESQLVariableType, EsqlControlType } from '@kbn/esql-types';
1919
import { ESQLControlsFlyout } from '.';
20+
import { ESQLEditorTelemetryService } from '@kbn/esql-editor';
2021

2122
jest.mock('@kbn/esql-utils', () => ({
2223
getESQLQueryColumnsRaw: jest.fn().mockResolvedValue([{ name: 'column1' }, { name: 'column2' }]),
2324
getValuesFromQueryField: jest.fn().mockReturnValue('field'),
2425
}));
2526

27+
const core = coreMock.createStart();
2628
const defaultProps = {
2729
initialVariableType: ESQLVariableType.FIELDS,
2830
queryString: 'FROM foo | WHERE field ==',
@@ -34,6 +36,7 @@ const defaultProps = {
3436
esqlVariables: [],
3537
ariaLabelledBy: 'esqlControlFlyoutTitle',
3638
telemetryTriggerSource: ControlTriggerSource.QUESTION_MARK,
39+
telemetryService: new ESQLEditorTelemetryService(core.analytics),
3740
};
3841

3942
const services = {

src/platform/plugins/shared/esql/public/triggers/esql_controls/control_flyout/index.tsx

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
* License v3.0 only", or the "Server Side Public License, v 1".
88
*/
99

10-
import React, { useCallback, useMemo, useState, useEffect, useRef } from 'react';
10+
import React, { useCallback, useMemo, useState, useEffect } from 'react';
1111
import { EuiFlyoutBody } from '@elastic/eui';
12-
import { ESQLEditorTelemetryService } from '@kbn/esql-editor';
12+
import type { ESQLEditorTelemetryService } from '@kbn/esql-editor';
1313
import type { TimeRange } from '@kbn/es-query';
1414
import {
1515
ESQLVariableType,
@@ -23,7 +23,6 @@ import {
2323
import { getValuesFromQueryField } from '@kbn/esql-utils';
2424
import type { ISearchGeneric } from '@kbn/search-types';
2525
import type { monaco } from '@kbn/monaco';
26-
import { useKibana } from '@kbn/kibana-react-plugin/public';
2726
import { ValueControlForm } from './value_control_form';
2827
import { Header, ControlType, VariableName, Footer } from './shared_form_components';
2928
import { IdentifierControlForm } from './identifier_control_form';
@@ -38,7 +37,6 @@ import {
3837
getVariableNamePrefix,
3938
checkVariableExistence,
4039
} from './helpers';
41-
import type { ServiceDeps } from '../../../kibana_services';
4240

4341
interface ESQLControlsFlyoutProps {
4442
search: ISearchGeneric;
@@ -53,7 +51,8 @@ interface ESQLControlsFlyoutProps {
5351
closeFlyout: () => void;
5452
ariaLabelledBy: string;
5553
currentApp?: string;
56-
telemetryTriggerSource: ControlTriggerSource;
54+
telemetryTriggerSource?: ControlTriggerSource;
55+
telemetryService: ESQLEditorTelemetryService;
5756
}
5857

5958
export function ESQLControlsFlyout({
@@ -70,6 +69,7 @@ export function ESQLControlsFlyout({
7069
ariaLabelledBy,
7170
currentApp,
7271
telemetryTriggerSource,
72+
telemetryService,
7373
}: ESQLControlsFlyoutProps) {
7474
// ?? or ?
7575
const [variableNamePrefix, setVariableNamePrefix] = useState(
@@ -127,11 +127,6 @@ export function ESQLControlsFlyout({
127127
: true;
128128
}, [variableType, controlState?.availableOptions]);
129129

130-
const kibana = useKibana<ServiceDeps>();
131-
const { core } = kibana.services;
132-
133-
const telemetryServiceRef = useRef(new ESQLEditorTelemetryService(core.analytics));
134-
135130
const onVariableNameChange = useCallback(
136131
(e: { target: { value: React.SetStateAction<string> } }) => {
137132
const text = validateVariableName(String(e.target.value), variableNamePrefix);
@@ -184,7 +179,12 @@ export function ESQLControlsFlyout({
184179
} else {
185180
await onSaveControl?.(controlState, '');
186181
}
187-
telemetryServiceRef.current.trackEsqlControlConfigSaved(variableType, telemetryTriggerSource);
182+
if (!isControlInEditMode) {
183+
telemetryService.trackEsqlControlConfigSaved(
184+
variableType,
185+
telemetryTriggerSource as ControlTriggerSource
186+
);
187+
}
188188
}
189189
closeFlyout();
190190
}, [
@@ -197,15 +197,16 @@ export function ESQLControlsFlyout({
197197
onSaveControl,
198198
variableType,
199199
telemetryTriggerSource,
200+
telemetryService,
200201
]);
201202

202203
const onCloseFlyout = useCallback(() => {
203-
telemetryServiceRef.current.trackEsqlControlConfigCancelled(
204+
telemetryService.trackEsqlControlConfigCancelled(
204205
initialVariableType,
205206
TelemetryControlCancelledReason.CANCEL_BUTTON
206207
);
207208
closeFlyout();
208-
}, [closeFlyout, initialVariableType]);
209+
}, [closeFlyout, initialVariableType, telemetryService]);
209210

210211
const formBody =
211212
variableNamePrefix === VariableNamePrefix.VALUE ? (

src/platform/plugins/shared/esql/public/triggers/esql_controls/control_flyout/value_control_form.test.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import { getESQLResults } from '@kbn/esql-utils';
1919
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
2020
import { __IntlProvider as IntlProvider } from '@kbn/i18n-react';
2121
import { ESQLControlsFlyout } from '.';
22+
import { ESQLEditorTelemetryService } from '@kbn/esql-editor';
2223

2324
jest.mock('@kbn/esql-utils', () => {
2425
return {
@@ -71,6 +72,7 @@ describe('ValueControlForm', () => {
7172
esqlVariables: [],
7273
ariaLabelledBy: 'esqlControlsFlyoutTitle',
7374
telemetryTriggerSource: ControlTriggerSource.QUESTION_MARK,
75+
telemetryService: new ESQLEditorTelemetryService(services.core.analytics),
7476
};
7577

7678
describe('Interval type', () => {

src/platform/plugins/shared/esql/public/triggers/esql_controls/esql_control_action.ts

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,21 @@
1010
import { i18n } from '@kbn/i18n';
1111
import { IncompatibleActionError, type Action } from '@kbn/ui-actions-plugin/public';
1212
import { firstValueFrom, of } from 'rxjs';
13-
import type { CoreStart } from '@kbn/core/public';
13+
import type { CoreStart, OverlayRef } from '@kbn/core/public';
1414
import type { TimefilterContract } from '@kbn/data-plugin/public';
1515
import type { ISearchGeneric } from '@kbn/search-types';
1616
import {
1717
ESQLVariableType,
1818
type ESQLControlVariable,
1919
type ESQLControlState,
2020
type ControlTriggerSource,
21+
TelemetryControlCancelledReason,
2122
} from '@kbn/esql-types';
2223
import type { monaco } from '@kbn/monaco';
2324
import { ENABLE_ESQL } from '@kbn/esql-utils';
2425
import { dismissAllFlyoutsExceptFor, DiscoverFlyouts } from '@kbn/discover-utils';
2526
import { openLazyFlyout } from '@kbn/presentation-util';
27+
import { ESQLEditorTelemetryService } from '@kbn/esql-editor';
2628
import { ACTION_CREATE_ESQL_CONTROL } from '../constants';
2729

2830
function isESQLVariableType(value: string): value is ESQLVariableType {
@@ -42,7 +44,7 @@ interface Context {
4244
cursorPosition?: monaco.Position;
4345
initialState?: ESQLControlState;
4446
parentApi?: unknown;
45-
triggerSource: ControlTriggerSource;
47+
triggerSource?: ControlTriggerSource;
4648
}
4749

4850
export class CreateESQLControlAction implements Action<Context> {
@@ -94,6 +96,17 @@ export class CreateESQLControlAction implements Action<Context> {
9496
// Flyouts don't exist or couldn't be closed, continue with opening the new flyout
9597
}
9698

99+
const telemetryService = new ESQLEditorTelemetryService(this.core.analytics);
100+
101+
const onClose = (flyoutRef: OverlayRef) => {
102+
onCancelControl?.();
103+
flyoutRef.close();
104+
telemetryService.trackEsqlControlConfigCancelled(
105+
variableType,
106+
TelemetryControlCancelledReason.CLOSE_BUTTON
107+
);
108+
};
109+
97110
openLazyFlyout({
98111
core: this.core,
99112
parentApi,
@@ -114,6 +127,7 @@ export class CreateESQLControlAction implements Action<Context> {
114127
closeFlyout,
115128
currentApp,
116129
triggerSource,
130+
telemetryService,
117131
});
118132
},
119133
flyoutProps: {
@@ -122,9 +136,9 @@ export class CreateESQLControlAction implements Action<Context> {
122136
maxWidth: 800,
123137
triggerId: 'dashboard-controls-menu-button',
124138
// When queryString is present (i.e. flyout opened from the ES|QL editor),
125-
// use onCancelControl as the onClose handler to ensure proper nested flyout closing behavior.
139+
// use the local onClose as the onClose handler to ensure proper nested flyout closing behavior.
126140
// In other scenarios (opened directly from the dashboard), we keep the default close behavior.
127-
...(queryString && { onClose: onCancelControl }),
141+
...(queryString && { onClose }),
128142
},
129143
});
130144
}

src/platform/plugins/shared/esql/public/triggers/esql_controls/esql_control_helpers.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
type ControlTriggerSource,
2020
} from '@kbn/esql-types';
2121
import type { monaco } from '@kbn/monaco';
22+
import type { ESQLEditorTelemetryService } from '@kbn/esql-editor';
2223
import { untilPluginStartServicesReady } from '../../kibana_services';
2324
import { ESQLControlsFlyout } from './control_flyout';
2425

@@ -36,7 +37,8 @@ interface Context {
3637
closeFlyout?: () => void;
3738
ariaLabelledBy: string;
3839
currentApp?: string;
39-
triggerSource: ControlTriggerSource;
40+
triggerSource?: ControlTriggerSource;
41+
telemetryService: ESQLEditorTelemetryService;
4042
}
4143

4244
export async function loadESQLControlFlyout({
@@ -54,6 +56,7 @@ export async function loadESQLControlFlyout({
5456
ariaLabelledBy,
5557
currentApp,
5658
triggerSource,
59+
telemetryService,
5760
}: Context) {
5861
const timeRange = timefilter.getTime();
5962
const deps = await untilPluginStartServicesReady();
@@ -79,6 +82,7 @@ export async function loadESQLControlFlyout({
7982
timeRange={timeRange}
8083
currentApp={currentApp}
8184
telemetryTriggerSource={triggerSource}
85+
telemetryService={telemetryService}
8286
/>
8387
</KibanaContextProvider>
8488
</KibanaRenderContextProvider>

0 commit comments

Comments
 (0)