[Discover-next] Address comments for search bar extensions and query …

…assist (opensearch-project#6933) * pass dependencies to isEnabled func Signed-off-by: Joshua Li <joshuali925@gmail.com> * add lazy and memo to search bar extensions Signed-off-by: Joshua Li <joshuali925@gmail.com> * move ppl specific string out from query assist Signed-off-by: Joshua Li <joshuali925@gmail.com> * prevent setstate after hook unmounts Signed-off-by: Joshua Li <joshuali925@gmail.com> * add max-height to search bar extensions Signed-off-by: Joshua Li <joshuali925@gmail.com> * prevent setstate after component unmounts Signed-off-by: Joshua Li <joshuali925@gmail.com> * move ml-commons API to common/index.ts Signed-off-by: Joshua Li <joshuali925@gmail.com> * improve i18n and accessibility usages Signed-off-by: Joshua Li <joshuali925@gmail.com> * add hard-coded suggestions for sample data indices Signed-off-by: Joshua Li <joshuali925@gmail.com> --------- Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit 4aade0f)
joshuali925 · Jun 14, 2024 · 08a81e0 · 08a81e0
1 parent d90734a
commit 08a81e0
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 73 deletions.
diff --git a/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx b/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx
@@ -255,8 +255,8 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
     return (
       <SearchBarExtensions
         configs={props.queryEnhancements?.get(queryLanguage!)?.searchBar?.extensions}
-        dependencies={{ indexPatterns: props.indexPatterns }}
-        portalInsert={{ sibling: queryEditorHeaderRef.current, position: 'before' }}
+        portalContainer={queryEditorHeaderRef.current}
+        indexPatterns={props.indexPatterns}
       />
     );
   }

diff --git a/src/plugins/data/public/ui/search_bar_extensions/index.ts b/src/plugins/data/public/ui/search_bar_extensions/index.ts
diff --git a/src/plugins/data/public/ui/search_bar_extensions/index.tsx b/src/plugins/data/public/ui/search_bar_extensions/index.tsx
@@ -0,0 +1,17 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import React, { ComponentProps } from 'react';
+
+const Fallback = () => <div />;
+
+const LazySearchBarExtensions = React.lazy(() => import('./search_bar_extensions'));
+export const SearchBarExtensions = (props: ComponentProps<typeof LazySearchBarExtensions>) => (
+  <React.Suspense fallback={<Fallback />}>
+    <LazySearchBarExtensions {...props} />
+  </React.Suspense>
+);
+
+export { SearchBarExtensionConfig } from './search_bar_extension';
diff --git a/src/plugins/data/public/ui/search_bar_extensions/search_bar_extension.test.tsx b/src/plugins/data/public/ui/search_bar_extensions/search_bar_extension.test.tsx
@@ -8,10 +8,9 @@ import React, { ComponentProps } from 'react';
 import { IIndexPattern } from '../../../common';
 import { SearchBarExtension } from './search_bar_extension';
 
-jest.mock('@elastic/eui', () => ({
-  ...jest.requireActual('@elastic/eui'),
-  EuiPortal: jest.fn(({ children }) => <div>{children}</div>),
-  EuiErrorBoundary: jest.fn(({ children }) => <div>{children}</div>),
+jest.mock('react-dom', () => ({
+  ...jest.requireActual('react-dom'),
+  createPortal: jest.fn((element) => element),
 }));
 
 type SearchBarExtensionProps = ComponentProps<typeof SearchBarExtension>;
@@ -45,7 +44,7 @@ describe('SearchBarExtension', () => {
     dependencies: {
       indexPatterns: [mockIndexPattern],
     },
-    portalInsert: { sibling: document.createElement('div'), position: 'after' },
+    portalContainer: document.createElement('div'),
   };
 
   beforeEach(() => {
@@ -78,17 +77,4 @@ describe('SearchBarExtension', () => {
 
     expect(isEnabledMock).toHaveBeenCalled();
   });
-
-  it('calls isEnabled and getComponent correctly', async () => {
-    isEnabledMock.mockResolvedValue(true);
-    getComponentMock.mockReturnValue(<div>Test Component</div>);
-
-    render(<SearchBarExtension {...defaultProps} />);
-
-    await waitFor(() => {
-      expect(isEnabledMock).toHaveBeenCalled();
-    });
-
-    expect(getComponentMock).toHaveBeenCalledWith(defaultProps.dependencies);
-  });
 });
diff --git a/src/plugins/data/public/ui/search_bar_extensions/search_bar_extension.tsx b/src/plugins/data/public/ui/search_bar_extensions/search_bar_extension.tsx
@@ -3,15 +3,15 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { EuiErrorBoundary, EuiPortal } from '@elastic/eui';
-import { EuiPortalProps } from '@opensearch-project/oui';
-import React, { useEffect, useMemo, useState } from 'react';
+import { EuiErrorBoundary } from '@elastic/eui';
+import React, { useEffect, useMemo, useRef, useState } from 'react';
+import ReactDOM from 'react-dom';
 import { IIndexPattern } from '../../../common';
 
 interface SearchBarExtensionProps {
   config: SearchBarExtensionConfig;
   dependencies: SearchBarExtensionDependencies;
-  portalInsert: EuiPortalProps['insert'];
+  portalContainer: Element;
 }
 
 export interface SearchBarExtensionDependencies {
@@ -34,7 +34,7 @@ export interface SearchBarExtensionConfig {
    * A function that determines if the search bar extension is enabled and should be rendered on UI.
    * @returns whether the extension is enabled.
    */
-  isEnabled: () => Promise<boolean>;
+  isEnabled: (dependencies: SearchBarExtensionDependencies) => Promise<boolean>;
   /**
    * A function that returns the mount point for the search bar extension.
    * @param dependencies - The dependencies required for the extension.
@@ -45,21 +45,30 @@ export interface SearchBarExtensionConfig {
 
 export const SearchBarExtension: React.FC<SearchBarExtensionProps> = (props) => {
   const [isEnabled, setIsEnabled] = useState(false);
+  const isMounted = useRef(true);
 
   const component = useMemo(() => props.config.getComponent(props.dependencies), [
     props.config,
     props.dependencies,
   ]);
 
   useEffect(() => {
-    props.config.isEnabled().then(setIsEnabled);
+    isMounted.current = true;
+    return () => {
+      isMounted.current = false;
+    };
+  }, []);
+
+  useEffect(() => {
+    props.config.isEnabled(props.dependencies).then((enabled) => {
+      if (isMounted.current) setIsEnabled(enabled);
+    });
   }, [props.dependencies, props.config]);
 
   if (!isEnabled) return null;
 
-  return (
-    <EuiPortal insert={props.portalInsert}>
-      <EuiErrorBoundary>{component}</EuiErrorBoundary>
-    </EuiPortal>
+  return ReactDOM.createPortal(
+    <EuiErrorBoundary>{component}</EuiErrorBoundary>,
+    props.portalContainer
   );
 };
diff --git a/src/plugins/data/public/ui/search_bar_extensions/search_bar_extensions.test.tsx b/src/plugins/data/public/ui/search_bar_extensions/search_bar_extensions.test.tsx
@@ -6,7 +6,7 @@
 import { render, waitFor } from '@testing-library/react';
 import React, { ComponentProps } from 'react';
 import { SearchBarExtension } from './search_bar_extension';
-import { SearchBarExtensions } from './search_bar_extensions';
+import SearchBarExtensions from './search_bar_extensions';
 
 type SearchBarExtensionProps = ComponentProps<typeof SearchBarExtension>;
 type SearchBarExtensionsProps = ComponentProps<typeof SearchBarExtensions>;
@@ -15,32 +15,30 @@ jest.mock('./search_bar_extension', () => ({
   SearchBarExtension: jest.fn(({ config, dependencies }: SearchBarExtensionProps) => (
     <div>
       Mocked SearchBarExtension {config.id} with{' '}
-      {dependencies.indexPatterns?.map((i) => i.title).join(', ')}
+      {dependencies.indexPatterns?.map((i) => (typeof i === 'string' ? i : i.title)).join(', ')}
     </div>
   )),
 }));
 
 describe('SearchBarExtensions', () => {
   const defaultProps: SearchBarExtensionsProps = {
-    dependencies: {
-      indexPatterns: [
-        {
-          id: '1234',
-          title: 'logstash-*',
-          fields: [
-            {
-              name: 'response',
-              type: 'number',
-              esTypes: ['integer'],
-              aggregatable: true,
-              filterable: true,
-              searchable: true,
-            },
-          ],
-        },
-      ],
-    },
-    portalInsert: { sibling: document.createElement('div'), position: 'after' },
+    indexPatterns: [
+      {
+        id: '1234',
+        title: 'logstash-*',
+        fields: [
+          {
+            name: 'response',
+            type: 'number',
+            esTypes: ['integer'],
+            aggregatable: true,
+            filterable: true,
+            searchable: true,
+          },
+        ],
+      },
+    ],
+    portalContainer: document.createElement('div'),
   };
 
   beforeEach(() => {
@@ -89,7 +87,7 @@ describe('SearchBarExtensions', () => {
 
     expect(SearchBarExtension).toHaveBeenCalledWith(
       expect.objectContaining({
-        dependencies: defaultProps.dependencies,
+        dependencies: { indexPatterns: defaultProps.indexPatterns },
       }),
       expect.anything()
     );

diff --git a/src/plugins/data/public/ui/search_bar_extensions/search_bar_extensions.tsx b/src/plugins/data/public/ui/search_bar_extensions/search_bar_extensions.tsx
@@ -3,45 +3,49 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { EuiPortalProps } from '@elastic/eui';
 import React, { useMemo } from 'react';
 import {
   SearchBarExtension,
   SearchBarExtensionConfig,
   SearchBarExtensionDependencies,
 } from './search_bar_extension';
 
-interface SearchBarExtensionsProps {
+interface SearchBarExtensionsProps extends SearchBarExtensionDependencies {
   configs?: SearchBarExtensionConfig[];
-  dependencies: SearchBarExtensionDependencies;
-  portalInsert: EuiPortalProps['insert'];
+  portalContainer: Element;
 }
 
-export const SearchBarExtensions: React.FC<SearchBarExtensionsProps> = (props) => {
-  const configs = useMemo(() => {
-    if (!props.configs) return [];
+const SearchBarExtensions: React.FC<SearchBarExtensionsProps> = React.memo((props) => {
+  const { configs, portalContainer, ...dependencies } = props;
+
+  const sortedConfigs = useMemo(() => {
+    if (!configs) return [];
 
     const seenIds = new Set();
-    props.configs.forEach((config) => {
+    configs.forEach((config) => {
       if (seenIds.has(config.id)) {
         throw new Error(`Duplicate search bar extension id '${config.id}' found.`);
       }
       seenIds.add(config.id);
     });
 
-    return [...props.configs].sort((a, b) => a.order - b.order);
-  }, [props.configs]);
+    return [...configs].sort((a, b) => a.order - b.order);
+  }, [configs]);
 
   return (
     <>
-      {configs.map((config) => (
+      {sortedConfigs.map((config) => (
         <SearchBarExtension
           key={config.id}
           config={config}
-          dependencies={props.dependencies}
-          portalInsert={props.portalInsert}
+          dependencies={dependencies}
+          portalContainer={portalContainer}
         />
       ))}
     </>
   );
-};
+});
+
+// Needed for React.lazy
+// eslint-disable-next-line import/no-default-export
+export default SearchBarExtensions;