[Discover-next] query editor and UI settings toggle #7001

kavilla · 2024-06-11T00:23:24Z

Description

Adds new query editor in replacement of query string input.

Utilizing:

data.enhancements.enabled: false

And enabling the Advanced Setting: query:enhancements:enabled

Also, cleans up the toggling since it is now two different components.

More information:

For 2.14, the query enhancements were nested within query string input bar.
For 2.15, query enhancements included the Monaco query editor.

This made the conditional logic for the query enhancements easier since we do not render even majority of the work if query enhancements are disabled.

First it will check the config file data.enhancements.enabled if it the feature is enabled (by default it is not).
If that is TRUE, then while rendering the Search Bar, it will check the Advanced Setting query:enhancements:enabled if it is enabled (by default it is not).
If data.enhancements.enabled && query:enhancements:enabled:
- renders query editor
If data.enhancements.enabled && NOT query:enhancements:enabled:
- renders query string input
If NOT data.enhancements.enabled && query:enhancements:enabled:
- renders query string input
If NOT data.enhancements.enabled && NOT query:enhancements:enabled:
- renders query string input

If user toggles on the setting in Advanced Settings, and queries a non-Native (DQL or Lucene) filter language from the language selector it will be treated as native query language. Existing features like local storage typeahead, userQueryString, and userQueryLanguage will support the new languages. However, when the users toggles off the setting in Advanced Setting, we will reset the these to be kuery and empty query string to reset the state. However, there exists an Advanced Setting for query language so we should consider best next steps here to improve this experience because we force this default state regardless if the user even used these extra langauges.

Issues Related:

#6067

Screenshot

Testing the changes

Changelog

feat: Query editor and UI settings toggle

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-06-11T00:38:23Z

Codecov Report

Attention: Patch coverage is 55.86207% with 64 lines in your changes missing coverage. Please review.

Project coverage is 67.46%. Comparing base (e74ed2c) to head (af675c9).
Report is 376 commits behind head on main.

Files with missing lines	Patch %	Lines
src/plugins/data/public/ui/settings/settings.ts	33.33%	16 Missing and 2 partials ⚠️
...c/plugins/data/public/ui/search_bar/search_bar.tsx	47.61%	6 Missing and 5 partials ⚠️
.../data_explorer/public/components/sidebar/index.tsx	52.38%	8 Missing and 2 partials ⚠️
.../data/public/ui/query_editor/language_selector.tsx	59.09%	8 Missing and 1 partial ⚠️
src/plugins/data/public/ui/ui_service.ts	0.00%	4 Missing ⚠️
...public/ui/query_string_input/language_switcher.tsx	78.57%	3 Missing ⚠️
...ns/data/public/ui/search_bar/create_search_bar.tsx	0.00%	3 Missing ⚠️
src/plugins/data/public/ui/query_editor/index.tsx	71.42%	2 Missing ⚠️
src/plugins/data/public/plugin.ts	0.00%	1 Missing ⚠️
src/plugins/data/public/ui/mocks.ts	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7001      +/-   ##
==========================================
+ Coverage   67.45%   67.46%   +0.01%     
==========================================
  Files        3442     3444       +2     
  Lines       67816    67865      +49     
  Branches    11027    11027              
==========================================
+ Hits        45742    45786      +44     
- Misses      19408    19416       +8     
+ Partials     2666     2663       -3

Flag	Coverage Δ
Linux_1	`33.07% <25.00%> (-0.01%)`	⬇️
Linux_2	`55.11% <ø> (ø)`
Linux_3	`45.24% <55.55%> (+0.03%)`	⬆️
Linux_4	`34.86% <27.02%> (-0.01%)`	⬇️
Windows_1	`33.12% <25.00%> (+0.02%)`	⬆️
Windows_2	`55.06% <ø> (ø)`
Windows_3	`45.24% <55.55%> (+0.02%)`	⬆️
Windows_4	`34.86% <27.02%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ashwin-pc · 2024-06-11T11:04:35Z

I think i understand how this keeps the new experience isolated from the existing one, but might be worth it to call it out in the PR how you test the change and how it safeguards against regressions.

src/plugins/data/public/ui/query_string_input/language_switcher.tsx

src/plugins/data/public/ui/query_string_input/language_switcher.test.tsx

src/plugins/data/public/ui/query_string_input/legacy_language_switcher.test.tsx

src/plugins/data/public/ui/query_string_input/legacy_language_switcher.tsx

Adds new query editor in replacement of query string input. Utilizing: ``` data.enhancements.enabled: true ``` And enabling the Advanced Setting: `query:enhancements:enabled` Also, cleans up the toggling since it is now two different components. Related issue: opensearch-project#6067 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

kavilla · 2024-06-12T19:29:59Z

src/plugins/data/public/ui/query_string_input/language_switcher.tsx

please note: this is actually restoring the language switcher to it's state before 2.14. Because the conditional logic renders different components then removed the query enhancements stuff and just having the switcher how it was before. This can be considered safe.

This changes can be considered safe.

kavilla · 2024-06-12T19:30:16Z

src/plugins/data/public/ui/query_string_input/language_switcher.test.tsx

please note: this is actually restoring the language switcher to it's state before 2.14. Because the conditional logic renders different components then removed the query enhancements stuff and just having the switcher how it was before. This can be considered safe.

kavilla · 2024-06-12T19:30:38Z

src/plugins/data/public/ui/query_string_input/legacy_language_switcher.test.tsx

please note: this isn't actually deleting the logic. this file was new for 2.14 to house the pre-2.14 code. this file is unneeded. since i'm restoring the language switcher in the original file (to retain the git history).

kavilla · 2024-06-12T19:30:42Z

src/plugins/data/public/ui/query_string_input/legacy_language_switcher.tsx

please note: this isn't actually deleting the logic. this file was new for 2.14 to house the pre-2.14 code. this file is unneeded. since i'm restoring the language switcher in the original file (to retain the git history).

kavilla · 2024-06-12T19:32:03Z

src/plugins/opensearch_dashboards_react/public/code_editor/editor_theme.ts

 import { monaco } from '@osd/monaco';

 import darkTheme from '@elastic/eui/dist/eui_theme_dark.json';
 import lightTheme from '@elastic/eui/dist/eui_theme_light.json';

 // NOTE: For talk around where this theme information will ultimately live,
 // please see this discuss issue: https://github.com/elastic/kibana/issues/43814
+const standardizeColor = (color: string) => new Color(color).hex().toLowerCase();


required, Monaco doesn't support shorthand hex code and will crash.

virajsanghvi · 2024-06-12T16:58:14Z

src/plugins/data/public/ui/filter_bar/_global_filter_group.scss

+.globalQueryEditor {
+  padding: 0 $euiSizeXS $euiSizeXS $euiSizeXS;
+}
+
+.globalQueryEditor:first-child {
+  padding-top: $euiSizeXS;
+}
+
+.globalQueryEditor:not(:empty) {
+  padding-bottom: $euiSizeXS;
+}
+
 .globalFilterGroup__filterBar {


Is this the right file? I guess kept here because other query bar styles are here?

correct. this isn't the right file and should be in it's own file along with the query bar style. i opted to going this route for 2.15 so that we can be more holistic in the clean up. along with even more refactoring related to search bar.

i've added a line item: #6957

virajsanghvi · 2024-06-12T17:00:05Z

src/plugins/data/public/ui/query_editor/_query_editor.scss

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+.osdQueryEditor__wrap {


curious, what's the difference between the osd query editor and the global query editor?

ported over from query bar but i agree this location and names of these are not accurate for their uses.

added here:
#6957

virajsanghvi · 2024-06-12T17:00:59Z

src/plugins/data/public/ui/query_editor/_query_editor.scss

+.osdQueryEditorHeader {
+  max-height: 400px;
+
+  // TODO fix styling: with "overflow: auto" the scroll bar appears although the content is below max-height


Do we have a mechanism to track todos to avoid long lived dead code?

good catch: ive added an item here to do start the ball rolling: #6957

virajsanghvi · 2024-06-12T17:02:34Z

src/plugins/data/public/ui/query_editor/_query_editor.scss

+  }
+}
+
+// IE specific fix for the datepicker to not collapse


Do we support IE or is this for Edge in IE mode?

I think Edge: https://opensearch.org/docs/1.2/dashboards/browser-compatibility/

But I can't find new doc about browser compatibility so I should check . Added an issue here to clean up this stuff and verify: #6957

src/plugins/data/public/ui/query_editor/_query_editor.scss

src/plugins/data/public/ui/query_editor/query_editor.tsx

virajsanghvi · 2024-06-12T19:57:53Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

+  index: number | null;
+  suggestions: QuerySuggestion[];
+  indexPatterns: IIndexPattern[];
+  queryEditorRect: DOMRect | undefined;


any reason for undefined over null?

this work might be unneeded based on what we do with suggestions and the query assist but reason between undefined over null is based on what the suggestions is expecting.

will make sure to track it here:
#6957

queryEditorRect can be undefined? so it is not required? should we use queryEditorRect?: DOMRect to show it is optional?

virajsanghvi · 2024-06-12T20:03:47Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

+  }
+
+  public componentWillUnmount() {
+    if (this.abortController) this.abortController.abort();


nit: is code style not to use braces on single line ifs? personally think they should be required (e.g. apple's tls bug)

this part was pretty much ported over.

i agree. one of things that we forked was the ability for plugins that have unrestricted control.

our repo has linters defined here https://github.com/opensearch-project/OpenSearch-Dashboards/tree/main/packages/opensearch-eslint-config-opensearch-dashboards

which we can add this rule (and more rules) but in the past worried about updating this. any plugin that uses our rules then will also have to update their code. And we tried our best to avoid breaking them by opening an issue in each repo and letting them know ahead of time.

i've added a line item for code clean up in both components:
#6957

virajsanghvi · 2024-06-12T20:04:58Z

src/plugins/data/public/ui/query_editor/query_editor.tsx

+    }
+
+    this.initPersistedLog();
+    // this.fetchIndexPatterns().then(this.updateSuggestions);


Didn't want to delete this code since this where we will get suggestions once query assist is added. Right now the suggestions is not wired up into the query editor but will be used once assistant work is added.

thanks for catching. added to this issue for tracking:
#6957

virajsanghvi · 2024-06-12T20:12:12Z

src/plugins/data/public/ui/search_bar/search_bar.tsx

+          onChange={this.onQueryBarChange}
+          isDirty={this.isDirty()}
+          customSubmitButton={
+            this.props.customSubmitButton ? this.props.customSubmitButton : undefined


nit: this.props.customSubmitButton || undefined

+1,

added here when we do clean up of both components:
#6957

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

mengweieric · 2024-06-13T18:56:28Z

src/plugins/data/public/ui/settings/settings.ts

+    }
+    this.isEnabled = enabled;
+    this.enabledQueryEnhancementsUpdated$.next(this.isEnabled);
+    return true;


nit. it feels to me that these return true statement are redundant as we are not having other conditional statement / async CURD operations to return false, and the set operation will always be true.

true i did this to mimic the other settings files within the repo. i think it's a little bit redundant as well. we should consider doing an audit

mengweieric · 2024-06-13T18:58:07Z

src/plugins/data/public/ui/settings/settings.ts

-export function createSettings({ storage, queryEnhancements }: Deps) {
-  return new Settings(storage, queryEnhancements);
+export function createSettings({ config, search, storage, queryEnhancements }: Deps) {
+  return new Settings(config, search, storage, queryEnhancements);


do we want to also passing params as an object to make it more scalable?

makes sense. added to tracker issue. hopefully we don't have to add any more params here but if we do ive tracked an action item for us to convert to an object.

ananzh

just did a local try out. approve as 2nd reviewer to unblock.

ananzh · 2024-06-13T20:44:08Z

src/plugins/data/public/ui/query_editor/index.tsx

+
+const LazyQueryEditorTopRow = React.lazy(() => import('./query_editor_top_row'));
+export const QueryEditorTopRow = (props: QueryEditorTopRowProps) => (
+  <React.Suspense fallback={<Fallback />}>


nit: instead of an empty
ad fallback, do we want something meaningful like spinner or a loading message. Should we also consider adding an error boundary to catch any errors that occur during the dynamic import or component rendering in case of crashing?

that's fair i feel like a fallback globally could be more helpful. added to the tracker issue

ananzh · 2024-06-13T20:57:40Z

src/plugins/data/public/ui/query_editor/language_selector.tsx

+      onChange={handleLanguageChange}
+      singleSelection={{ asPlainText: true }}
+      isClearable={false}
+      async


don't know there is an async for EuiComboBox.. good learning for me

ananzh · 2024-06-13T21:02:55Z

src/plugins/data/public/ui/query_editor/no_data_popover.tsx

+    }
+  }, [noDataPopoverDismissed, showNoDataPopover]);
+
+  return (


I see we are repeating query_string_input 's context in query_editor. I know that there are there are three cases that we use query string input and only one with both settings enabled to use query editor, so if that is the case can't we just simply reuse these repeat component from query_string_input? why duplicate these component?

yeah good catch. part of this issue: #6957.

the goal here was to release to this for 2.15, it would keep the query string input completely intact (even restored some of the files to their pre-2.14 state). to ensure the code paths are completely separate in case users toggle it on and off.

then targeting 2.16, with more automated testing start cleaning up these components.

ananzh · 2024-06-13T22:33:15Z

src/plugins/data/public/ui/search_bar/create_search_bar.tsx

          settings={settings}
+          containerRef={containerRef}


So the containerRef is added to access a DOM element within a QueryEditorUI?. or is there use cases that will cause DOM to lose?

* [Discover-next] query editor and UI settings toggle Adds new query editor in replacement of query string input. Utilizing: ``` data.enhancements.enabled: true ``` And enabling the Advanced Setting: `query:enhancements:enabled` Also, cleans up the toggling since it is now two different components. Related issue: #6067 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Remove commented out code Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> (cherry picked from commit a4aa682) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

joshuali925 · 2024-06-14T19:03:23Z

src/plugins/data/server/ui_settings.ts

+      name: i18n.translate('data.advancedSettings.query.enhancements.enableTitle', {
+        defaultMessage: 'Enable query enhancements',
+      }),
+      value: false,


this still needs user to set data.enhancements.enabled in osd config yml, would it be confusing if user only sets it to true here and see enhancements are still disabled because yml is not set?

* [Discover-next] query editor and UI settings toggle Adds new query editor in replacement of query string input. Utilizing: ``` data.enhancements.enabled: true ``` And enabling the Advanced Setting: `query:enhancements:enabled` Also, cleans up the toggling since it is now two different components. Related issue: #6067 * Remove commented out code --------- (cherry picked from commit a4aa682) Signed-off-by: Kawika Avilla <kavilla414@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

kavilla requested review from ananzh, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi and mengweieric as code owners June 11, 2024 00:23

github-actions bot added the distinguished-contributor label Jun 11, 2024

opensearch-changeset-bot bot added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 11, 2024

Changeset file for PR opensearch-project#7001 created/updated

9ef1158

kavilla added backport 2.x discover-next labels Jun 11, 2024

kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 11, 2024

Changeset file for PR opensearch-project#7001 created/updated

3c5dfd8

kavilla force-pushed the kavilla/editor_toggle branch from dd03ab9 to 0237362 Compare June 11, 2024 03:00

ashwin-pc added the discover for discover reinvent label Jun 11, 2024

bandinib-amzn pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 11, 2024

Changeset file for PR opensearch-project#7001 created/updated

88b2f25

bandinib-amzn force-pushed the kavilla/editor_toggle branch from 3d377bb to e882983 Compare June 11, 2024 18:32

bandinib-amzn assigned ashwin-pc Jun 11, 2024

ananzh assigned mengweieric Jun 11, 2024

kavilla force-pushed the kavilla/editor_toggle branch 2 times, most recently from d269095 to cee212d Compare June 12, 2024 10:15