From ae4cbe46f1c01b89faa83bc3953d9160ae46c4e7 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Sat, 14 Sep 2024 21:37:29 +0800 Subject: [PATCH] Address comments Signed-off-by: Liyun Xiu --- .../public/ui/query_editor/query_editor.tsx | 27 +-- .../common/query_assist/index.ts | 2 +- .../common/query_assist/types.ts | 6 + .../{sparkle_color.svg => sparkle_mark.svg} | 0 .../public/query_assist/_index.scss | 11 ++ .../components/query_assist_button.test.tsx | 6 +- .../components/query_assist_button.tsx | 12 +- .../components/query_assist_summary.test.tsx | 42 +++-- .../components/query_assist_summary.tsx | 165 ++++++++++++------ .../query_assist/utils/create_extension.tsx | 2 +- 10 files changed, 181 insertions(+), 92 deletions(-) rename src/plugins/query_enhancements/public/assets/{sparkle_color.svg => sparkle_mark.svg} (100%) diff --git a/src/plugins/data/public/ui/query_editor/query_editor.tsx b/src/plugins/data/public/ui/query_editor/query_editor.tsx index f05bc68d775e..9d5deaba489b 100644 --- a/src/plugins/data/public/ui/query_editor/query_editor.tsx +++ b/src/plugins/data/public/ui/query_editor/query_editor.tsx @@ -14,10 +14,9 @@ import { EuiText, PopoverAnchorPosition, } from '@elastic/eui'; -import { BehaviorSubject } from 'rxjs'; import classNames from 'classnames'; import { isEqual } from 'lodash'; -import React, { Component, createRef, RefObject } from 'react'; +import React, { Component, createRef, RefObject, useMemo } from 'react'; import { monaco } from '@osd/monaco'; import { IDataPluginServices, @@ -338,16 +337,22 @@ export default class QueryEditorUI extends Component { }; private renderExtensionSearchBarButtion = () => { - const supported = 'query-assist'; if (!this.extensionMap || Object.keys(this.extensionMap).length === 0) return null; - if (!(supported in this.extensionMap) || !this.extensionMap[supported].getSearchBarButton) - return null; - return this.extensionMap[supported].getSearchBarButton({ - language: this.props.query.language, - onSelectLanguage: this.onSelectLanguage, - isCollapsed: this.state.isCollapsed, - setIsCollapsed: this.setIsCollapsed, - }); + const sortedConfigs = Object.values(this.extensionMap).sort((a, b) => a.order - b.order); + return ( + <> + {sortedConfigs.map((config) => { + return config.getSearchBarButton + ? config.getSearchBarButton({ + language: this.props.query.language, + onSelectLanguage: this.onSelectLanguage, + isCollapsed: this.state.isCollapsed, + setIsCollapsed: this.setIsCollapsed, + }) + : null; + })} + + ); }; public render() { diff --git a/src/plugins/query_enhancements/common/query_assist/index.ts b/src/plugins/query_enhancements/common/query_assist/index.ts index 9469a3a2771e..7c577db88834 100644 --- a/src/plugins/query_enhancements/common/query_assist/index.ts +++ b/src/plugins/query_enhancements/common/query_assist/index.ts @@ -3,4 +3,4 @@ * SPDX-License-Identifier: Apache-2.0 */ -export { QueryAssistParameters, QueryAssistResponse } from './types'; +export { QueryAssistParameters, QueryAssistResponse, QueryAssistContextType } from './types'; diff --git a/src/plugins/query_enhancements/common/query_assist/types.ts b/src/plugins/query_enhancements/common/query_assist/types.ts index 057ff1e708d8..d191befad5e7 100644 --- a/src/plugins/query_enhancements/common/query_assist/types.ts +++ b/src/plugins/query_enhancements/common/query_assist/types.ts @@ -17,3 +17,9 @@ export interface QueryAssistParameters { // for MDS dataSourceId?: string; } + +export enum QueryAssistContextType { + QUESTION, + QUERY, + DATA, +} diff --git a/src/plugins/query_enhancements/public/assets/sparkle_color.svg b/src/plugins/query_enhancements/public/assets/sparkle_mark.svg similarity index 100% rename from src/plugins/query_enhancements/public/assets/sparkle_color.svg rename to src/plugins/query_enhancements/public/assets/sparkle_mark.svg diff --git a/src/plugins/query_enhancements/public/query_assist/_index.scss b/src/plugins/query_enhancements/public/query_assist/_index.scss index 39973e26a8c9..c2c814846944 100644 --- a/src/plugins/query_enhancements/public/query_assist/_index.scss +++ b/src/plugins/query_enhancements/public/query_assist/_index.scss @@ -8,6 +8,17 @@ margin-top: $euiSizeXS; } + &.queryAssist__summary_banner { + /* stylelint-disable @osd/stylelint/no_restricted_values */ + background: linear-gradient(to left, #edf7ff, #f9f4ff); + padding: $euiSizeXS; + } + + &.queryAssist__summary_banner_dark { + background: transparent; + padding: $euiSizeXS; + } + &.queryAssist__callout { margin-top: $euiSizeXS; } diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.test.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.test.tsx index ecc03e4480d8..3173ac955e3e 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.test.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.test.tsx @@ -3,8 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ import { fireEvent, render, screen } from '@testing-library/react'; -import React from 'react'; -import { QueryAssistButton, QueryAssistButtonProps } from './query_assist_button'; +import React, { ComponentProps } from 'react'; +import { QueryAssistButton } from './query_assist_button'; import { useQueryAssist } from '../hooks'; jest.mock('../hooks', () => ({ @@ -15,7 +15,7 @@ describe('query assist button', () => { const setIsCollapsed = jest.fn(); const updateIsQueryAssistCollapsed = jest.fn(); - const props: QueryAssistButtonProps = { + const props: ComponentProps = { dependencies: { isCollapsed: false, setIsCollapsed, diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.tsx index 661be8b6acff..25a198288442 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_button.tsx @@ -3,28 +3,28 @@ * SPDX-License-Identifier: Apache-2.0 */ import { EuiButtonIcon, EuiFlexItem } from '@elastic/eui'; -import React from 'react'; +import React, { useCallback } from 'react'; import { i18n } from '@osd/i18n'; import { useQueryAssist } from '../hooks'; import collapsedIcon from '../../assets/sparkle_solid.svg'; -import expandIcon from '../../assets/sparkle_color.svg'; +import expandIcon from '../../assets/sparkle_mark.svg'; import { QueryEditorExtensionDependencies } from '../../../../data/public'; -export interface QueryAssistButtonProps { +interface QueryAssistButtonProps { dependencies: QueryEditorExtensionDependencies; } export const QueryAssistButton: React.FC = (props) => { const { isQueryAssistCollapsed, updateIsQueryAssistCollapsed } = useQueryAssist(); - const onClick = () => { + const onClick = useCallback(() => { if (props.dependencies.isCollapsed) { props.dependencies.setIsCollapsed(false); updateIsQueryAssistCollapsed(false); } else { updateIsQueryAssistCollapsed(!isQueryAssistCollapsed); } - }; + }, [props.dependencies, isQueryAssistCollapsed, updateIsQueryAssistCollapsed]); return ( @@ -32,7 +32,7 @@ export const QueryAssistButton: React.FC = (props) => { iconType={ !props.dependencies.isCollapsed && !isQueryAssistCollapsed ? expandIcon : collapsedIcon } - aria-label={i18n.translate('discover.queryControls.queryAssistToggle', { + aria-label={i18n.translate('queryEnhancements.queryAssist.button.ariaLable', { defaultMessage: `Query Assist Toggle`, })} onClick={onClick} diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx index b96096f627cc..7347a08a94fa 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.test.tsx @@ -3,10 +3,10 @@ * SPDX-License-Identifier: Apache-2.0 */ import { fireEvent, render, screen } from '@testing-library/react'; -import React from 'react'; +import React, { ComponentProps } from 'react'; import { coreMock } from '../../../../../core/public/mocks'; import { BehaviorSubject } from 'rxjs'; -import { QueryAssistSummary, QueryAssistSummaryProps, QueryContext } from './query_assist_summary'; +import { QueryAssistSummary } from './query_assist_summary'; import { useQueryAssist } from '../hooks'; jest.mock('react', () => ({ @@ -21,36 +21,35 @@ jest.mock('../hooks', () => ({ describe('query assist summary', () => { const PPL = 'ppl'; const question = 'Are there any errors in my logs?'; - const queryContext: QueryContext = { + const queryContext = { question, query: PPL, queryResults: [{ size: 1 }], }; - const emptyResultQueryContext: QueryContext = { + const emptyResultQueryContext = { question, query: PPL, queryResults: [], }; + const coreSetupMock = coreMock.createSetup({}); + const httpMock = coreSetupMock.http; + const data = new BehaviorSubject([]); + const question$ = new BehaviorSubject(''); + const query$ = new BehaviorSubject(''); const reportUiStatsMock = jest.fn(); - const getQuery = jest.fn(); const setSummary = jest.fn(); const setLoading = jest.fn(); const setQueryContext = jest.fn(); const setFeedback = jest.fn(); const setIsAssistantEnabledByCapability = jest.fn(); - - const coreSetupMock = coreMock.createSetup({}); - const httpMock = coreSetupMock.http; - const data = new BehaviorSubject([]); - const question$ = new BehaviorSubject(''); - const query$ = new BehaviorSubject(''); + const getQuery = jest.fn(); const dataMock = { query: { queryString: { + getUpdates$: () => query$, getQuery, - query$, }, }, search: { @@ -73,7 +72,7 @@ describe('query assist summary', () => { CLICK: 'click', }, }; - const props: QueryAssistSummaryProps = { + const props: ComponentProps = { data: dataMock, http: httpMock, usageCollection: usageCollectionMock, @@ -132,6 +131,7 @@ describe('query assist summary', () => { isAssistantEnabledByCapability, setIsAssistantEnabledByCapability, ]); + React.useState.mockImplementationOnce(() => [undefined, jest.fn()]); useQueryAssist.mockImplementationOnce(() => ({ question: 'question', question$, @@ -212,7 +212,7 @@ describe('query assist summary', () => { fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbup')); expect(setFeedback).toHaveBeenCalledWith(true); expect(reportUiStatsMock).toHaveBeenCalledWith( - 'query-assistant', + 'query-assist', 'click', expect.stringMatching(/^thumbup/) ); @@ -226,7 +226,7 @@ describe('query assist summary', () => { fireEvent.click(screen.getByTestId('queryAssist_summary_buttons_thumbdown')); expect(setFeedback).toHaveBeenCalledWith(true); expect(reportUiStatsMock).toHaveBeenCalledWith( - 'query-assistant', + 'query-assist', 'click', expect.stringMatching(/^thumbdown/) ); @@ -251,6 +251,9 @@ describe('query assist summary', () => { question, ppl: PPL, }), + query: { + dataSourceId: undefined, + }, }); await sleep(2000); expect(setSummary).toHaveBeenNthCalledWith(1, null); @@ -318,4 +321,13 @@ describe('query assist summary', () => { await sleep(2000); expect(setQueryContext).toHaveBeenCalledTimes(1); }); + + it('should reset feedback state if re-fetch summary', async () => { + mockUseState('summary', LOADING.NO, queryContext, FEEDBACK.YES); + const RESPONSE_TEXT = 'response'; + httpMock.post.mockResolvedValue(RESPONSE_TEXT); + renderQueryAssistSummary(COLLAPSED.NO); + await sleep(2000); + expect(setFeedback).toHaveBeenCalledWith(FEEDBACK.NO); + }); }); diff --git a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx index f5c6d2246477..5f089b1fc72d 100644 --- a/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx +++ b/src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx @@ -14,7 +14,7 @@ import { EuiCopy, } from '@elastic/eui'; -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useState, useCallback } from 'react'; import { i18n } from '@osd/i18n'; import { IDataFrame } from 'src/plugins/data/common'; import { v4 as uuidv4 } from 'uuid'; @@ -22,11 +22,14 @@ import { isEmpty } from 'lodash'; import { merge, of } from 'rxjs'; import { filter, distinctUntilChanged, mergeMap } from 'rxjs/operators'; import { HttpSetup } from 'opensearch-dashboards/public'; +import { Dataset } from '../../../../data/common'; import { useQueryAssist } from '../hooks'; import { DataPublicPluginSetup, QueryEditorExtensionDependencies } from '../../../../data/public'; import { UsageCollectionSetup } from '../../../../usage_collection/public'; import { CoreSetup } from '../../../../../core/public'; +import { QueryAssistContextType } from '../../../common/query_assist'; import sparkleHollowSvg from '../../assets/sparkle_hollow.svg'; +import sparkleSolidSvg from '../../assets/sparkle_solid.svg'; export interface QueryContext { question: string; @@ -34,7 +37,7 @@ export interface QueryContext { queryResults: any; } -export interface QueryAssistSummaryProps { +interface QueryAssistSummaryProps { data: DataPublicPluginSetup; http: HttpSetup; usageCollection: UsageCollectionSetup; @@ -43,15 +46,21 @@ export interface QueryAssistSummaryProps { } export const QueryAssistSummary: React.FC = (props) => { + const { query, search } = props.data; const [summary, setSummary] = useState(null); // store fetched data const [loading, setLoading] = useState(false); // track loading state const [queryContext, setQueryContext] = useState(undefined); const [feedback, setFeedback] = useState(false); - const [isAssistantEnabledByCapability, setIsAssistantEnabledByCapability] = useState(false); // track loading state + const [isEnabledByCapability, setIsEnabledByCapability] = useState(false); + const [selectedDataset, setSelectedDataset] = useState( + query.queryString.getQuery()?.dataset + ); const { question$, isQueryAssistCollapsed } = useQueryAssist(); - const METRIC_APP = `query-assistant`; - const afterFeedbackTip = - 'Thank you for the feedback. Try again by adjusting your question so that I have the opportunity to better assist you.'; + const METRIC_APP = `query-assist`; + const afterFeedbackTip = i18n.translate('queryEnhancements.queryAssist.summary.afterFeedback', { + defaultMessage: + 'Thank you for the feedback. Try again by adjusting your question so that I have the opportunity to better assist you.', + }); const sampleSize = 10; @@ -74,20 +83,22 @@ export const QueryAssistSummary: React.FC = (props) => return hits; }; - const { query, search } = props.data; - - const reportMetric = props.usageCollection - ? (metric: string) => { + const reportMetric = useCallback( + (metric: string) => { + if (props.usageCollection) { props.usageCollection.reportUiStats( METRIC_APP, props.usageCollection.METRIC_TYPE.CLICK, metric + '-' + uuidv4() ); } - : () => {}; + }, + [props.usageCollection, METRIC_APP] + ); - const reportCountMetric = props.usageCollection - ? (metric: string, count: number) => { + const reportCountMetric = useCallback( + (metric: string, count: number) => { + if (props.usageCollection) { props.usageCollection.reportUiStats( METRIC_APP, props.usageCollection.METRIC_TYPE.COUNT, @@ -95,48 +106,62 @@ export const QueryAssistSummary: React.FC = (props) => count ); } - : () => {}; + }, + [props.usageCollection, METRIC_APP] + ); + + useEffect(() => { + const subscription = query.queryString.getUpdates$().subscribe((_query) => { + setSelectedDataset(_query?.dataset); + }); + return () => subscription.unsubscribe(); + }, [query.queryString]); useEffect(() => { let dataStack = []; - const enum DataType { - QUESTION, - QUERY, - DATA, - } - const combineSubscription = merge( + const subscription = merge( question$.pipe( filter((value) => !isEmpty(value)), - mergeMap((value) => of({ type: DataType.QUESTION, data: value })) + mergeMap((value) => of({ type: QueryAssistContextType.QUESTION, data: value })) ), - query.queryString.query$.pipe( + query.queryString.getUpdates$().pipe( filter((value) => !isEmpty(value)), - mergeMap((value) => of({ type: DataType.QUERY, data: value })) + mergeMap((value) => of({ type: QueryAssistContextType.QUERY, data: value })) ), search.df?.df$?.pipe( distinctUntilChanged(), filter((value) => !isEmpty(value) && !isEmpty(value?.fields)), - mergeMap((value) => of({ type: DataType.DATA, data: value })) + mergeMap((value) => of({ type: QueryAssistContextType.DATA, data: value })) ) ).subscribe((value) => { // to ensure we only trigger summary when user hits the query assist button with natual language input - if (value.type === DataType.QUESTION) { - dataStack = [value.data]; - } else if (value.type === DataType.QUERY && dataStack.length === 1) { - dataStack.push(value.data.query); - } else if (value.type === DataType.DATA && dataStack.length === 2) { - dataStack.push(value.data); - setQueryContext({ - question: dataStack[0], - query: dataStack[1], - queryResults: convertResult(dataStack[2]), - }); - dataStack = []; + switch (value.type) { + case QueryAssistContextType.QUESTION: + dataStack = [value.data]; + break; + case QueryAssistContextType.QUERY: + if (dataStack.length === 1) { + dataStack.push(value.data.query); + } + break; + case QueryAssistContextType.DATA: + if (dataStack.length === 2) { + dataStack.push(value.data); + setQueryContext({ + question: dataStack[0], + query: dataStack[1], + queryResults: convertResult(dataStack[2]), + }); + dataStack = []; + } + break; + default: + break; } }); return () => { - combineSubscription.unsubscribe(); + subscription.unsubscribe(); }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ -146,6 +171,7 @@ export const QueryAssistSummary: React.FC = (props) => if (isEmpty(queryContext?.queryResults)) return; setLoading(true); setSummary(null); + setFeedback(false); const SUCCESS_METRIC = 'fetch_summary_success'; try { const actualSampleSize = Math.min(sampleSize, queryContext?.queryResults?.length); @@ -159,6 +185,9 @@ export const QueryAssistSummary: React.FC = (props) => question: queryContext?.question, ppl: queryContext?.query, }), + query: { + dataSourceId: selectedDataset?.dataSource?.id, + }, }); setSummary(response); reportCountMetric(SUCCESS_METRIC, 1); @@ -175,20 +204,24 @@ export const QueryAssistSummary: React.FC = (props) => useEffect(() => { props.core.getStartServices().then(([coreStart, depsStart]) => { - const assistantEnabled = coreStart.application.capabilities?.assistant?.enabled === true; - setIsAssistantEnabledByCapability(assistantEnabled); + const assistantEnabled = !!coreStart.application.capabilities?.assistant?.enabled; + setIsEnabledByCapability(assistantEnabled); }); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - const onFeedback = (satisfied: boolean) => { - if (feedback) return; - setFeedback(true); - reportMetric(satisfied ? 'thumbup' : 'thumbdown'); - }; + const onFeedback = useCallback( + (satisfied: boolean) => { + if (feedback) return; + setFeedback(true); + reportMetric(satisfied ? 'thumbup' : 'thumbdown'); + }, + [feedback, reportMetric] + ); - if (props.dependencies.isCollapsed || isQueryAssistCollapsed || !isAssistantEnabledByCapability) + if (props.dependencies.isCollapsed || isQueryAssistCollapsed || !isEnabledByCapability) return null; + const isDarkMode = props.core.uiSettings.get('theme:darkMode'); return ( = (props) => borderRadius="none" > - + - {i18n.translate('assistantDashboards.vizSummary.response', { + {i18n.translate('queryEnhancements.queryAssist.summary.panelTitle', { defaultMessage: 'Response', })} @@ -219,7 +256,7 @@ export const QueryAssistSummary: React.FC = (props) => @@ -229,7 +266,13 @@ export const QueryAssistSummary: React.FC = (props) => aria-label="feedback thumbs up" color="text" iconType="thumbsUp" - title={!feedback ? 'Good response' : afterFeedbackTip} + title={ + !feedback + ? i18n.translate('queryEnhancements.queryAssist.summary.goodResponse', { + defaultMessage: `Good response`, + }) + : afterFeedbackTip + } onClick={() => onFeedback(true)} data-test-subj="queryAssist_summary_buttons_thumbup" /> @@ -238,7 +281,13 @@ export const QueryAssistSummary: React.FC = (props) => onFeedback(false)} data-test-subj="queryAssist_summary_buttons_thumbdown" @@ -249,8 +298,10 @@ export const QueryAssistSummary: React.FC = (props) => {(copy) => ( = (props) => {!summary && !loading && ( - Ask a question to generate a summary. + {i18n.translate('queryEnhancements.queryAssist.summary.placeholder', { + defaultMessage: `Ask a question to generate summary.`, + })} )} {loading && ( - Generating response... + {i18n.translate('queryEnhancements.queryAssist.summary.generating', { + defaultMessage: `Generating response...`, + })} )} {summary && !loading && ( diff --git a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx index 0d8984e72194..b74455a80553 100644 --- a/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx +++ b/src/plugins/query_enhancements/public/query_assist/utils/create_extension.tsx @@ -92,7 +92,7 @@ export const createQueryAssistExtension = ( usageCollection: UsageCollectionSetup ): QueryEditorExtensionConfig => { const http: HttpSetup = core.http; - const isQueryAssistCollapsed$ = new BehaviorSubject(true); + const isQueryAssistCollapsed$ = new BehaviorSubject(false); const question$ = new BehaviorSubject(''); return { id: 'query-assist',