Cleanup OsdUrlStateStorage subscription in TopNav

changelogs/fragments/9167.yml

@@ -0,0 +1,2 @@
+fix:
+- Cleanup OsdUrlStateStorage subscription in TopNav ([#9167](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9167))

src/plugins/data/public/index.ts

@@ -465,6 +465,7 @@ export { Filter, Query, RefreshInterval, TimeRange } from '../common';
 export {
   createSavedQueryService,
   connectStorageToQueryState,
+  useConnectStorageToQueryState,
   connectToQueryState,
   syncQueryStateWithUrl,
   QueryState,

src/plugins/data/public/query/state_sync/connect_to_query_state.test.ts

@@ -126,13 +126,15 @@ describe('connect_storage_to_query_state', () => {
       sessionStorage: new DataStorage(window.sessionStorage, 'opensearch_dashboards.'),
       defaultSearchInterceptor: mockSearchInterceptor,
       application: setupMock.application,
+      notifications: setupMock.notifications,
     });
     queryServiceStart = queryService.start({
-      uiSettings: setupMock.uiSettings,
+      uiSettings: startMock.uiSettings,
       storage: new DataStorage(window.localStorage, 'opensearch_dashboards.'),
       savedObjectsClient: startMock.savedObjects.client,
       indexPatterns: indexPatternsMock,
       application: startMock.application,
+      notifications: startMock.notifications,
     });
     indexPatternsMock = ({
       get: jest.fn(),

src/plugins/data/public/query/state_sync/connect_to_query_state.ts

@@ -31,7 +31,6 @@
 import { Subscription } from 'rxjs';
 import { filter, map } from 'rxjs/operators';
 import _ from 'lodash';
-import { CoreStart } from 'opensearch-dashboards/public';
 import {
   BaseStateContainer,
   IOsdUrlStateStorage,
@@ -41,27 +40,28 @@ import { QueryState, QueryStateChange } from './types';
 import { FilterStateStore, COMPARE_ALL_OPTIONS, compareFilters } from '../../../common';
 import { validateTimeRange } from '../timefilter';
 
+export interface ISyncConfig {
+  filters: FilterStateStore;
+  query: boolean;
+  dataset?: boolean;
+}
+
 /**
  * Helper function to sync up filter and query services in data plugin
  * with a URL state storage so plugins can persist the app filter and query
  * values across refresh
  * @param QueryService: either setup or start
- * @param  OsdUrlStateStorage to use for syncing and store data
+ * @param OsdUrlStateStorage to use for syncing and store data
  * @param syncConfig app filter and query
  */
-export const connectStorageToQueryState = async (
+export const connectStorageToQueryState = (
   {
     filterManager,
     queryString,
     state$,
   }: Pick<QueryStart | QuerySetup, 'timefilter' | 'filterManager' | 'queryString' | 'state$'>,
   OsdUrlStateStorage: IOsdUrlStateStorage,
-  syncConfig: {
-    filters: FilterStateStore;
-    query: boolean;
-    dataSet?: boolean;
-  },
-  uiSettings?: CoreStart['uiSettings']
+  syncConfig: ISyncConfig
 ) => {
   try {
     const syncKeys: Array<keyof QueryStateChange> = [];

src/plugins/data/public/query/state_sync/index.ts

@@ -29,5 +29,6 @@
  */
 
 export { connectToQueryState, connectStorageToQueryState } from './connect_to_query_state';
+export { useConnectStorageToQueryState } from './use_connect_to_query_state';
 export { syncQueryStateWithUrl } from './sync_state_with_url';
 export { QueryState, QueryStateChange } from './types';

src/plugins/data/public/query/state_sync/use_connect_to_query_state.test.ts

@@ -0,0 +1,113 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { Subscription } from 'rxjs';
+import { FilterManager } from '../filter_manager';
+import { getFilter } from '../filter_manager/test_helpers/get_stub_filter';
+import {
+  DataStorage,
+  Filter,
+  FilterStateStore,
+  IndexPatternsService,
+  Query,
+} from '../../../common';
+import { coreMock } from '../../../../../core/public/mocks';
+import {
+  IOsdUrlStateStorage,
+  createOsdUrlStateStorage,
+} from '../../../../opensearch_dashboards_utils/public';
+import { QueryService, QueryStart } from '../query_service';
+import { connectStorageToQueryState } from './connect_to_query_state';
+import { createBrowserHistory, History } from 'history';
+import { QueryStringContract } from '../query_string';
+import { ISearchInterceptor } from '../../search';
+import { renderHook } from '@testing-library/react-hooks';
+import { useConnectStorageToQueryState } from './use_connect_to_query_state';
+
+jest.mock('./connect_to_query_state');
+
+const setupMock = coreMock.createSetup();
+const startMock = coreMock.createStart();
+
+describe('use_connect_storage_to_query_state', () => {
+  let queryServiceStart: QueryStart;
+  let queryString: QueryStringContract;
+  let queryChangeSub: Subscription;
+  let queryChangeTriggered = jest.fn();
+  let filterManager: FilterManager;
+  let filterManagerChangeSub: Subscription;
+  let filterManagerChangeTriggered = jest.fn();
+  let osdUrlStateStorage: IOsdUrlStateStorage;
+  let indexPatternsMock: IndexPatternsService;
+  let history: History;
+  let gF1: Filter;
+  let gF2: Filter;
+  let aF1: Filter;
+  let aF2: Filter;
+  let q1: Query;
+  let mockSearchInterceptor: jest.Mocked<ISearchInterceptor>;
+
+  beforeEach(() => {
+    const queryService = new QueryService();
+    mockSearchInterceptor = {} as jest.Mocked<ISearchInterceptor>;
+    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({
+      uiSettings: startMock.uiSettings,
+      storage: new DataStorage(window.localStorage, 'opensearch_dashboards.'),
+      savedObjectsClient: startMock.savedObjects.client,
+      indexPatterns: indexPatternsMock,
+      application: startMock.application,
+      notifications: startMock.notifications,
+    });
+    indexPatternsMock = ({
+      get: jest.fn(),
+    } as unknown) as IndexPatternsService;
+
+    queryString = queryServiceStart.queryString;
+    queryChangeTriggered = jest.fn();
+    queryChangeSub = queryString.getUpdates$().subscribe(queryChangeTriggered);
+
+    filterManager = queryServiceStart.filterManager;
+    filterManagerChangeTriggered = jest.fn();
+    filterManagerChangeSub = filterManager.getUpdates$().subscribe(filterManagerChangeTriggered);
+
+    window.location.href = '/';
+    history = createBrowserHistory();
+    osdUrlStateStorage = createOsdUrlStateStorage({ useHash: false, history });
+
+    gF1 = getFilter(FilterStateStore.GLOBAL_STATE, true, true, 'key1', 'value1');
+    gF2 = getFilter(FilterStateStore.GLOBAL_STATE, false, false, 'key2', 'value2');
+    aF1 = getFilter(FilterStateStore.APP_STATE, true, true, 'key3', 'value3');
+    aF2 = getFilter(FilterStateStore.APP_STATE, false, false, 'key4', 'value4');
+
+    q1 = {
+      query: 'count is less than 100',
+      language: 'kuery',
+    };
+  });
+
+  afterEach(() => {
+    filterManagerChangeSub.unsubscribe();
+    queryChangeSub.unsubscribe();
+  });
+
+  it('Should invoke connectStorageToQueryState', () => {
+    const { result } = renderHook(() =>
+      useConnectStorageToQueryState(queryServiceStart, osdUrlStateStorage, {
+        filters: FilterStateStore.APP_STATE,
+        query: true,
+      })
+    );
+    expect(connectStorageToQueryState).toHaveBeenCalledTimes(1);
+    expect(result.current).toBeUndefined();
+  });
+});

src/plugins/data/public/query/state_sync/use_connect_to_query_state.ts

@@ -0,0 +1,34 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { useEffect } from 'react';
+import { QuerySetup, QueryStart } from '../query_service';
+import { IOsdUrlStateStorage } from '../../../../opensearch_dashboards_utils/public';
+import { connectStorageToQueryState, ISyncConfig } from './connect_to_query_state';
+
+/**
+ * Hook version of connectStorageToQueryState, automatically unSub
+ * from OsdUrlStateStorage
+ * @param QueryService: either setup or start
+ * @param OsdUrlStateStorage to use for syncing and store data
+ * @param syncConfig app filter and query
+ */
+export const useConnectStorageToQueryState = (
+  QueryService: Pick<
+    QueryStart | QuerySetup,
+    'timefilter' | 'filterManager' | 'queryString' | 'state$'
+  >,
+  OsdUrlStateStorage: IOsdUrlStateStorage,
+  syncConfig: ISyncConfig
+) => {
+  useEffect(() => {
+    const unSub = connectStorageToQueryState(QueryService, OsdUrlStateStorage, syncConfig);
+    return () => {
+      if (unSub) {
+        unSub();
+      }
+    };
+  }, [OsdUrlStateStorage, QueryService, syncConfig]);
+};

src/plugins/discover/public/application/view_components/canvas/top_nav.tsx

@@ -10,7 +10,7 @@ import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem, EuiToolTip } from '@elastic/e
 import { i18n } from '@osd/i18n';
 import { AppMountParameters } from '../../../../../../core/public';
 import {
-  connectStorageToQueryState,
+  useConnectStorageToQueryState,
   opensearchFilters,
   QueryStatus,
 } from '../../../../../data/public';
@@ -65,15 +65,10 @@ export const TopNav = ({ opts, showSaveQuery, isEnhancementsEnabled }: TopNavPro
     ? getTopNavLinks(services, inspectorAdapters, savedSearch, isEnhancementsEnabled)
     : [];
 
-  connectStorageToQueryState(
-    services.data.query,
-    osdUrlStateStorage,
-    {
-      filters: opensearchFilters.FilterStateStore.APP_STATE,
-      query: true,
-    },
-    uiSettings
-  );
+  useConnectStorageToQueryState(services.data.query, osdUrlStateStorage, {
+    filters: opensearchFilters.FilterStateStore.APP_STATE,
+    query: true,
+  });
 
   useEffect(() => {
     const subscription = data$.subscribe((queryData) => {

src/plugins/vis_builder/public/application/components/top_nav.tsx

@@ -20,7 +20,7 @@ import { setEditorState } from '../utils/state_management/metadata_slice';
 import { useCanSave } from '../utils/use/use_can_save';
 import { saveStateToSavedObject } from '../../saved_visualizations/transforms';
 import { TopNavMenuData, TopNavMenuItemRenderType } from '../../../../navigation/public';
-import { opensearchFilters, connectStorageToQueryState } from '../../../../data/public';
+import { opensearchFilters, useConnectStorageToQueryState } from '../../../../data/public';
 import { RootState } from '../../../../data_explorer/public';
 
 function useDeepEffect(callback, dependencies) {
@@ -56,16 +56,17 @@ export const TopNav = () => {
 
   const saveDisabledReason = useCanSave();
   const savedVisBuilderVis = useSavedVisBuilderVis(visualizationIdFromUrl);
-  connectStorageToQueryState(services.data.query, services.osdUrlStateStorage, {
-    filters: opensearchFilters.FilterStateStore.APP_STATE,
-    query: true,
-  });
   const { selected: indexPattern } = useIndexPatterns();
   const [config, setConfig] = useState<TopNavMenuData[] | undefined>();
   const originatingApp = useTypedSelector((state) => {
     return state.metadata.originatingApp;
   });
 
+  useConnectStorageToQueryState(services.data.query, services.osdUrlStateStorage, {
+    filters: opensearchFilters.FilterStateStore.APP_STATE,
+    query: true,
+  });
+
   useEffect(() => {
     const getConfig = () => {
       if (!savedVisBuilderVis || !indexPattern) return;