[Discover] fix: Clean up sync URL subscription in Discover plugin top…

…Nav (#9316) * [Discover] fix: Clean up sync URL subscription in Discover plugin topNav Signed-off-by: Joey Liu <[email protected]> * Changeset file for PR #9316 created/updated * Update unit test Signed-off-by: Joey Liu <[email protected]> * Revert save modal change Signed-off-by: Joey Liu <[email protected]> --------- Signed-off-by: Joey Liu <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
opensearch-project · Feb 12, 2025 · 8cc5f84 · 8cc5f84
1 parent 4da33a1
commit 8cc5f84
Show file tree

Hide file tree

Showing 9 changed files with 213 additions and 12 deletions.
diff --git a/changelogs/fragments/9316.yml b/changelogs/fragments/9316.yml
@@ -0,0 +1,2 @@
+fix:
+- Clean up sync URL subscription in Discover plugin topNav ([#9316](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9316))
diff --git a/src/plugins/dashboard/public/application/utils/use/use_dashboard_app_state.tsx b/src/plugins/dashboard/public/application/utils/use/use_dashboard_app_state.tsx
@@ -69,7 +69,7 @@ export const useDashboardAppAndGlobalState = ({
         stopSyncingQueryServiceStateWithUrl,
       } = createDashboardGlobalAndAppState({
         stateDefaults,
-        osdUrlStateStorage: services.osdUrlStateStorage,
+        osdUrlStateStorage,
         services,
         savedDashboardInstance,
       });

diff --git a/src/plugins/data/public/index.ts b/src/plugins/data/public/index.ts
@@ -468,6 +468,7 @@ export {
   useConnectStorageToQueryState,
   connectToQueryState,
   syncQueryStateWithUrl,
+  useSyncQueryStateWithUrl,
   QueryState,
   getDefaultQuery,
   FilterManager,

diff --git a/src/plugins/data/public/query/state_sync/index.ts b/src/plugins/data/public/query/state_sync/index.ts
@@ -30,5 +30,6 @@
 
 export { connectToQueryState, connectStorageToQueryState } from './connect_to_query_state';
 export { useConnectStorageToQueryState } from './use_connect_to_query_state';
+export { useSyncQueryStateWithUrl } from './use_sync_state_with_url';
 export { syncQueryStateWithUrl } from './sync_state_with_url';
 export { QueryState, QueryStateChange } from './types';
diff --git a/src/plugins/data/public/query/state_sync/sync_state_with_url.test.ts b/src/plugins/data/public/query/state_sync/sync_state_with_url.test.ts
@@ -106,13 +106,15 @@ describe('sync_query_state_with_url', () => {
       sessionStorage: new DataStorage(window.sessionStorage, 'opensearch_dashboards.'),
       defaultSearchInterceptor: mockSearchInterceptor,
       application: setupMock.application,
+      notifications: setupMock.notifications,
     });
     queryServiceStart = queryService.start({
       indexPatterns: indexPatternsMock,
       uiSettings: startMock.uiSettings,
       storage: new DataStorage(window.localStorage, 'opensearch_dashboards.'),
       savedObjectsClient: startMock.savedObjects.client,
       application: startMock.application,
+      notifications: startMock.notifications,
     });
     filterManager = queryServiceStart.filterManager;
     timefilter = queryServiceStart.timefilter.timefilter;

diff --git a/src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts b/src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts
@@ -0,0 +1,139 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { Subscription } from 'rxjs';
+import { createBrowserHistory, History } from 'history';
+import { FilterManager } from '../filter_manager';
+import { getFilter } from '../filter_manager/test_helpers/get_stub_filter';
+import {
+  DataStorage,
+  Filter,
+  FilterStateStore,
+  IndexPatternsService,
+  UI_SETTINGS,
+} from '../../../common';
+import { coreMock } from '../../../../../core/public/mocks';
+import {
+  createOsdUrlStateStorage,
+  IOsdUrlStateStorage,
+} from '../../../../opensearch_dashboards_utils/public';
+import { QueryService, QueryStart } from '../query_service';
+import { TimefilterContract } from '../timefilter';
+import { ISearchInterceptor } from '../../search';
+import { act, renderHook } from '@testing-library/react-hooks';
+import { useSyncQueryStateWithUrl } from './use_sync_state_with_url';
+import * as SyncQueryStateWithUrl from './sync_state_with_url';
+
+const mockStopSync = jest.fn();
+const mockSyncQueryStateWithUrl = jest.fn().mockReturnValue({
+  stop: mockStopSync,
+});
+
+const setupMock = coreMock.createSetup();
+const startMock = coreMock.createStart();
+
+setupMock.uiSettings.get.mockImplementation((key: string) => {
+  switch (key) {
+    case 'defaultIndex':
+      return 'logstash-*';
+    case UI_SETTINGS.FILTERS_PINNED_BY_DEFAULT:
+      return true;
+    case 'timepicker:timeDefaults':
+      return { from: 'now-15m', to: 'now' };
+    case 'search:queryLanguage':
+      return 'kuery';
+    case UI_SETTINGS.TIMEPICKER_REFRESH_INTERVAL_DEFAULTS:
+      return { pause: false, value: 0 };
+    case UI_SETTINGS.QUERY_ENHANCEMENTS_ENABLED:
+      return false;
+    case UI_SETTINGS.SEARCH_MAX_RECENT_DATASETS:
+      return 4;
+    default:
+      throw new Error(`sync_query test: not mocked uiSetting: ${key}`);
+  }
+});
+
+describe('use_sync_query_state_with_url', () => {
+  let queryServiceStart: QueryStart;
+  let filterManager: FilterManager;
+  let timefilter: TimefilterContract;
+  let osdUrlStateStorage: IOsdUrlStateStorage;
+  let history: History;
+  let indexPatternsMock: IndexPatternsService;
+
+  beforeEach(() => {
+    indexPatternsMock = ({
+      get: jest.fn(),
+    } as unknown) as IndexPatternsService;
+  });
+
+  let filterManagerChangeSub: Subscription;
+  let filterManagerChangeTriggered = jest.fn();
+  let mockSearchInterceptor: jest.Mocked<ISearchInterceptor>;
+
+  let gF: Filter;
+  let aF: Filter;
+
+  beforeEach(() => {
+    const queryService = new QueryService();
+    queryService.setup({
+      uiSettings: setupMock.uiSettings,
+      storage: new DataStorage(window.localStorage, 'opensearch_dashboards.'),
+      sessionStorage: new DataStorage(window.sessionStorage, 'opensearch_dashboards.'),
+      defaultSearchInterceptor: mockSearchInterceptor,
+      application: setupMock.application,
+      notifications: setupMock.notifications,
+    });
+    queryServiceStart = queryService.start({
+      indexPatterns: indexPatternsMock,
+      uiSettings: startMock.uiSettings,
+      storage: new DataStorage(window.localStorage, 'opensearch_dashboards.'),
+      savedObjectsClient: startMock.savedObjects.client,
+      application: startMock.application,
+      notifications: startMock.notifications,
+    });
+    filterManager = queryServiceStart.filterManager;
+    timefilter = queryServiceStart.timefilter.timefilter;
+
+    filterManagerChangeTriggered = jest.fn();
+    filterManagerChangeSub = filterManager.getUpdates$().subscribe(filterManagerChangeTriggered);
+
+    window.location.href = '/';
+    history = createBrowserHistory();
+    osdUrlStateStorage = createOsdUrlStateStorage({ useHash: false, history });
+
+    gF = getFilter(FilterStateStore.GLOBAL_STATE, true, true, 'key1', 'value1');
+    aF = getFilter(FilterStateStore.APP_STATE, true, true, 'key3', 'value3');
+
+    jest
+      .spyOn(SyncQueryStateWithUrl, 'syncQueryStateWithUrl')
+      .mockImplementation(mockSyncQueryStateWithUrl);
+  });
+  afterEach(() => {
+    filterManagerChangeSub.unsubscribe();
+  });
+
+  it('Should invoke connectStorageToQueryState', () => {
+    const { result } = renderHook(() =>
+      useSyncQueryStateWithUrl(queryServiceStart, osdUrlStateStorage)
+    );
+
+    act(() => {
+      result.current.startSyncingQueryStateWithUrl();
+    });
+
+    expect(mockSyncQueryStateWithUrl).toHaveBeenCalledTimes(1);
+  });
+
+  it('Should call stop callback when hook unmount', () => {
+    const { unmount } = renderHook(() =>
+      useSyncQueryStateWithUrl(queryServiceStart, osdUrlStateStorage)
+    );
+
+    unmount();
+
+    expect(mockStopSync).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/src/plugins/data/public/query/state_sync/use_sync_state_with_url.ts b/src/plugins/data/public/query/state_sync/use_sync_state_with_url.ts
@@ -0,0 +1,41 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { useEffect, useState } from 'react';
+import { IOsdUrlStateStorage } from '../../../../opensearch_dashboards_utils/public';
+import { QuerySetup, QueryStart } from '../query_service';
+import { syncQueryStateWithUrl } from './sync_state_with_url';
+
+/**
+ * Hook version of syncQueryStateWithUrl, automatically clean up subscriptions
+ * @param QueryService: either setup or start
+ * @param osdUrlStateStorage to use for syncing
+ */
+export const useSyncQueryStateWithUrl = (
+  query: Pick<QueryStart | QuerySetup, 'filterManager' | 'timefilter' | 'queryString' | 'state$'>,
+  osdUrlStateStorage: IOsdUrlStateStorage
+) => {
+  const [startSync, setStartSync] = useState(false);
+  useEffect(() => {
+    let stopSync: () => void;
+
+    if (startSync) {
+      // starts syncing `_g` portion of url with query services
+      stopSync = syncQueryStateWithUrl(query, osdUrlStateStorage).stop;
+    }
+
+    return () => {
+      if (stopSync) {
+        stopSync();
+      }
+    };
+  }, [osdUrlStateStorage, query, startSync]);
+
+  const startSyncingQueryStateWithUrl = () => {
+    setStartSync(true);
+  };
+
+  return { startSyncingQueryStateWithUrl };
+};
diff --git a/src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx b/src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx
@@ -13,6 +13,7 @@ import { ISearchSource, unhashUrl } from '../../../opensearch_dashboards_service
 import {
   OnSaveProps,
   SavedObjectSaveModal,
+  SaveResult,
   showSaveModal,
 } from '../../../../../saved_objects/public';
 import {
@@ -23,13 +24,13 @@ import { DiscoverState, setSavedSearchId } from '../../utils/state_management';
 import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../../../common';
 import { getSortForSearchSource } from '../../view_components/utils/get_sort_for_search_source';
 import { getRootBreadcrumbs } from '../../helpers/breadcrumbs';
-import { syncQueryStateWithUrl } from '../../../../../data/public';
 import { OpenSearchPanel } from './open_search_panel';
 
 const getLegacyTopNavLinks = (
   services: DiscoverViewServices,
   inspectorAdapters: Adapters,
   savedSearch: SavedSearch,
+  startSyncingQueryStateWithUrl: () => void,
   isEnhancementEnabled: boolean = false
 ) => {
   const {
@@ -41,8 +42,6 @@ const getLegacyTopNavLinks = (
     toastNotifications,
     chrome,
     store,
-    data: { query },
-    osdUrlStateStorage,
   } = services;
 
   const newSearch: TopNavMenuData = {
@@ -83,7 +82,7 @@ const getLegacyTopNavLinks = (
         newCopyOnSave,
         isTitleDuplicateConfirmed,
         onTitleDuplicate,
-      }: OnSaveProps) => {
+      }: OnSaveProps): Promise<SaveResult | undefined> => {
         const currentTitle = savedSearch.title;
         savedSearch.title = newTitle;
         savedSearch.copyOnSave = newCopyOnSave;
@@ -124,7 +123,7 @@ const getLegacyTopNavLinks = (
             store!.dispatch({ type: setSavedSearchId.type, payload: id });
 
             // starts syncing `_g` portion of url with query services
-            syncQueryStateWithUrl(query, osdUrlStateStorage);
+            startSyncingQueryStateWithUrl();
 
             return { id };
           }
@@ -277,6 +276,7 @@ export const getTopNavLinks = (
   services: DiscoverViewServices,
   inspectorAdapters: Adapters,
   savedSearch: SavedSearch,
+  startSyncingQueryStateWithUrl: () => void,
   isEnhancementEnabled: boolean = false,
   useNoIndexPatternsTopNav: boolean = false
 ) => {
@@ -289,14 +289,18 @@ export const getTopNavLinks = (
     toastNotifications,
     chrome,
     store,
-    data: { query },
-    osdUrlStateStorage,
     uiSettings,
   } = services;
 
   const showActionsInGroup = uiSettings.get('home:useNewHomePage');
   if (!showActionsInGroup)
-    return getLegacyTopNavLinks(services, inspectorAdapters, savedSearch, isEnhancementEnabled);
+    return getLegacyTopNavLinks(
+      services,
+      inspectorAdapters,
+      savedSearch,
+      startSyncingQueryStateWithUrl,
+      isEnhancementEnabled
+    );
 
   const topNavLinksMap = new Map<string, TopNavMenuData>();
 
@@ -361,7 +365,7 @@ export const getTopNavLinks = (
           newCopyOnSave,
           isTitleDuplicateConfirmed,
           onTitleDuplicate,
-        }: OnSaveProps) => {
+        }: OnSaveProps): Promise<SaveResult | undefined> => {
           const currentTitle = savedSearch.title;
           savedSearch.title = newTitle;
           savedSearch.copyOnSave = newCopyOnSave;
@@ -402,7 +406,7 @@ export const getTopNavLinks = (
               store!.dispatch({ type: setSavedSearchId.type, payload: id });
 
               // starts syncing `_g` portion of url with query services
-              syncQueryStateWithUrl(query, osdUrlStateStorage);
+              startSyncingQueryStateWithUrl();
 
               return { id };
             }

diff --git a/src/plugins/discover/public/application/view_components/canvas/top_nav.tsx b/src/plugins/discover/public/application/view_components/canvas/top_nav.tsx
@@ -13,6 +13,7 @@ import {
   useConnectStorageToQueryState,
   opensearchFilters,
   QueryStatus,
+  useSyncQueryStateWithUrl,
 } from '../../../../../data/public';
 import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
 import { PLUGIN_ID } from '../../../../common';
@@ -59,10 +60,20 @@ export const TopNav = ({ opts, showSaveQuery, isEnhancementsEnabled }: TopNavPro
     uiSettings,
   } = services;
 
+  const { startSyncingQueryStateWithUrl } = useSyncQueryStateWithUrl(
+    data.query,
+    osdUrlStateStorage
+  );
   const showActionsInGroup = uiSettings.get('home:useNewHomePage');
 
   const topNavLinks = savedSearch
-    ? getTopNavLinks(services, inspectorAdapters, savedSearch, isEnhancementsEnabled)
+    ? getTopNavLinks(
+        services,
+        inspectorAdapters,
+        savedSearch,
+        startSyncingQueryStateWithUrl,
+        isEnhancementsEnabled
+      )
     : [];
 
   const syncConfig = useMemo(() => {
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		fix:
		- Clean up sync URL subscription in Discover plugin topNav ([#9316](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9316))