From 60dd66eba3e26d4859e35eb47e4572cb9e476883 Mon Sep 17 00:00:00 2001 From: Joey Liu Date: Wed, 5 Feb 2025 21:58:10 +0000 Subject: [PATCH 1/4] [Discover] fix: Clean up sync URL subscription in Discover plugin topNav Signed-off-by: Joey Liu --- .../utils/use/use_dashboard_app_state.tsx | 2 +- src/plugins/data/public/index.ts | 1 + .../data/public/query/state_sync/index.ts | 1 + .../state_sync/sync_state_with_url.test.ts | 2 + .../use_sync_state_with_url.test.ts | 122 +++++++ .../state_sync/use_sync_state_with_url.ts | 41 +++ .../components/top_nav/get_top_nav_links.tsx | 24 +- .../view_components/canvas/top_nav.tsx | 13 +- .../save_modal/saved_object_save_modal.tsx | 302 +++++++++--------- 9 files changed, 343 insertions(+), 165 deletions(-) create mode 100644 src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts create mode 100644 src/plugins/data/public/query/state_sync/use_sync_state_with_url.ts 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 index a31e1eb45fdc..f83251ea1a84 100644 --- 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 index 620bbd12f042..cc3b8e872c1e 100644 --- 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 index 1a80c36d0d67..4ba125c17779 100644 --- 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 index 24abf6951cde..d2a5e723fb0b 100644 --- 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,6 +106,7 @@ 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, @@ -113,6 +114,7 @@ describe('sync_query_state_with_url', () => { 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 new file mode 100644 index 000000000000..8ac960a139fd --- /dev/null +++ b/src/plugins/data/public/query/state_sync/use_sync_state_with_url.test.ts @@ -0,0 +1,122 @@ +/* + * 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 { syncQueryStateWithUrl } from './sync_state_with_url'; + +jest.mock('./sync_state_with_url'); + +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; + + 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'); + }); + afterEach(() => { + filterManagerChangeSub.unsubscribe(); + }); + + it('Should invoke connectStorageToQueryState', () => { + const { result } = renderHook(() => + useSyncQueryStateWithUrl(queryServiceStart, osdUrlStateStorage) + ); + + act(() => { + result.current.startSyncingQueryStateWithUrl(); + }); + + expect(syncQueryStateWithUrl).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 new file mode 100644 index 000000000000..e4b4016ec73a --- /dev/null +++ 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, + 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 index d87e1e47f702..08711f62fc71 100644 --- 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 => { 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(); @@ -361,7 +365,7 @@ export const getTopNavLinks = ( newCopyOnSave, isTitleDuplicateConfirmed, onTitleDuplicate, - }: OnSaveProps) => { + }: OnSaveProps): Promise => { 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 index cb407c0ca0df..3e3cf791fcf8 100644 --- 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(() => { diff --git a/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx b/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx index fcb013573ebc..b3d48dcd3f06 100644 --- a/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx +++ b/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx @@ -47,9 +47,11 @@ import { EuiCompressedTextArea, } from '@elastic/eui'; import { FormattedMessage } from '@osd/i18n/react'; -import React from 'react'; +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { EuiText } from '@elastic/eui'; import { i18n } from '@osd/i18n'; +import { unstable_batchedUpdates } from 'react-dom'; +import { SaveResult } from './show_saved_object_save_modal'; export interface OnSaveProps { newTitle: string; @@ -60,7 +62,7 @@ export interface OnSaveProps { } interface Props { - onSave: (props: OnSaveProps) => void; + onSave: (props: OnSaveProps) => void | Promise; onClose: () => void; title: string; showCopyOnSave: boolean; @@ -81,98 +83,29 @@ export interface SaveModalState { visualizationDescription: string; } -const generateId = htmlIdGenerator(); - -export class SavedObjectSaveModal extends React.Component { - private warning = React.createRef(); - public readonly state = { - title: this.props.title, - copyOnSave: Boolean(this.props.initialCopyOnSave), - isTitleDuplicateConfirmed: false, - hasTitleDuplicate: false, - isLoading: false, - visualizationDescription: this.props.description ? this.props.description : '', - }; - - public render() { - const { isTitleDuplicateConfirmed, hasTitleDuplicate, title } = this.state; - const duplicateWarningId = generateId(); - - return ( - - - - -

- -

- - - - - - {this.renderDuplicateTitleCallout(duplicateWarningId)} - - - {!this.props.showDescription && this.props.description && ( - - {this.props.description} - - )} - - - - {this.renderCopyOnSave()} - - - } - > - - - - {this.renderViewDescription()} - - {typeof this.props.options === 'function' - ? this.props.options(this.state) - : this.props.options} - - - - - - - - - {this.renderConfirmButton()} - - - ); - } - - private renderViewDescription = () => { - if (!this.props.showDescription) { +export const SavedObjectSaveModal = (props: Props) => { + const { + onSave, + onClose, + showCopyOnSave, + initialCopyOnSave, + objectType, + confirmButtonLabel, + options, + description, + showDescription, + } = props; + const warning = useRef(null); + const [title, setTitle] = useState(props.title); + const [copyOnSave, setCopyOnSave] = useState(!!initialCopyOnSave); + const [isTitleDuplicateConfirmed, setIsTitleDuplicateConfirmed] = useState(false); + const [hasTitleDuplicate, setHasTitleDuplicate] = useState(false); + const [isLoading, setIsLoading] = useState(false); + const [visualizationDescription, setVisualizationDescription] = useState(description || ''); + const duplicateWarningId = useMemo(() => htmlIdGenerator()(), []); + + const renderViewDescription = () => { + if (!showDescription) { return; } @@ -188,82 +121,69 @@ export class SavedObjectSaveModal extends React.Component > ); }; - private onDescriptionChange = (event: React.ChangeEvent) => { - this.setState({ - visualizationDescription: event.target.value, - }); + const onDescriptionChange = (event: React.ChangeEvent) => { + setVisualizationDescription(event.target.value); }; - private onTitleDuplicate = () => { - this.setState({ - isLoading: false, - isTitleDuplicateConfirmed: true, - hasTitleDuplicate: true, + const onTitleDuplicate = () => { + unstable_batchedUpdates(() => { + setIsLoading(false); + setIsTitleDuplicateConfirmed(true); + setHasTitleDuplicate(true); }); - if (this.warning.current) { - this.warning.current.focus(); + if (warning.current) { + warning.current.focus(); } }; - private saveSavedObject = async () => { - if (this.state.isLoading) { + const saveSavedObject = useCallback(async () => { + if (isLoading) { // ignore extra clicks return; } - this.setState({ - isLoading: true, - }); + setIsLoading(true); - await this.props.onSave({ - newTitle: this.state.title, - newCopyOnSave: this.state.copyOnSave, - isTitleDuplicateConfirmed: this.state.isTitleDuplicateConfirmed, - onTitleDuplicate: this.onTitleDuplicate, - newDescription: this.state.visualizationDescription, + await onSave({ + newTitle: title, + newCopyOnSave: copyOnSave, + isTitleDuplicateConfirmed, + onTitleDuplicate, + newDescription: visualizationDescription, }); - }; + }, [copyOnSave, isLoading, isTitleDuplicateConfirmed, onSave, title, visualizationDescription]); - private onTitleChange = (event: React.ChangeEvent) => { - this.setState({ - title: event.target.value, - isTitleDuplicateConfirmed: false, - hasTitleDuplicate: false, + const onTitleChange = (event: React.ChangeEvent) => { + unstable_batchedUpdates(() => { + setTitle(event.target.value); + setIsTitleDuplicateConfirmed(false); + setHasTitleDuplicate(false); }); }; - private onCopyOnSaveChange = (event: EuiSwitchEvent) => { - this.setState({ - copyOnSave: event.target.checked, - }); + const onCopyOnSaveChange = (event: EuiSwitchEvent) => { + setCopyOnSave(event.target.checked); }; - private onFormSubmit = (event: React.FormEvent) => { + const onFormSubmit = (event: React.FormEvent) => { event.preventDefault(); - this.saveSavedObject(); + saveSavedObject(); }; - private renderConfirmButton = () => { - const { isLoading, title } = this.state; - - let confirmLabel: string | React.ReactNode = i18n.translate( - 'savedObjects.saveModal.saveButtonLabel', - { + const renderConfirmButton = () => { + const confirmLabel: string | React.ReactNode = + confirmButtonLabel ?? + i18n.translate('savedObjects.saveModal.saveButtonLabel', { defaultMessage: 'Save', - } - ); - - if (this.props.confirmButtonLabel) { - confirmLabel = this.props.confirmButtonLabel; - } + }); return ( ); }; - private renderDuplicateTitleCallout = (duplicateWarningId: string) => { - if (!this.state.hasTitleDuplicate) { + const renderDuplicateTitleCallout = () => { + if (!hasTitleDuplicate) { return; } return ( <> -
+
} color="warning" @@ -304,7 +224,7 @@ export class SavedObjectSaveModal extends React.Component id="savedObjects.saveModal.duplicateTitleDescription" defaultMessage="Saving '{title}' creates a duplicate title." values={{ - title: this.state.title, + title, }} />

@@ -315,8 +235,8 @@ export class SavedObjectSaveModal extends React.Component ); }; - private renderCopyOnSave = () => { - if (!this.props.showCopyOnSave) { + const renderCopyOnSave = () => { + if (!showCopyOnSave) { return; } @@ -324,13 +244,13 @@ export class SavedObjectSaveModal extends React.Component <> } /> @@ -338,4 +258,80 @@ export class SavedObjectSaveModal extends React.Component ); }; -} + + return ( + + + + +

+ +

+ + + + + + {renderDuplicateTitleCallout()} + + + {!showDescription && description && ( + + {description} + + )} + + + + {renderCopyOnSave()} + + + } + > + + + + {renderViewDescription()} + + {typeof options === 'function' + ? options({ + title, + copyOnSave, + isTitleDuplicateConfirmed, + hasTitleDuplicate, + isLoading, + visualizationDescription, + }) + : options} + + + + + + + + + {renderConfirmButton()} + + + ); +}; From 12c19572d82afdd1c94f6b14f9dcbe46b3449024 Mon Sep 17 00:00:00 2001 From: "opensearch-changeset-bot[bot]" <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Date: Wed, 5 Feb 2025 21:58:11 +0000 Subject: [PATCH 2/4] Changeset file for PR #9316 created/updated --- changelogs/fragments/9316.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/9316.yml diff --git a/changelogs/fragments/9316.yml b/changelogs/fragments/9316.yml new file mode 100644 index 000000000000..38a9a4279dd7 --- /dev/null +++ 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)) \ No newline at end of file From 39f4f2cedd49c4d329e0fe62e9e6d7dfe5e74975 Mon Sep 17 00:00:00 2001 From: Joey Liu Date: Wed, 5 Feb 2025 21:58:11 +0000 Subject: [PATCH 3/4] Update unit test Signed-off-by: Joey Liu --- .../use_sync_state_with_url.test.ts | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) 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 index 8ac960a139fd..35b1f04ca0f9 100644 --- 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 @@ -24,9 +24,12 @@ 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 { syncQueryStateWithUrl } from './sync_state_with_url'; +import * as SyncQueryStateWithUrl from './sync_state_with_url'; -jest.mock('./sync_state_with_url'); +const mockStopSync = jest.fn(); +const mockSyncQueryStateWithUrl = jest.fn().mockReturnValue({ + stop: mockStopSync, +}); const setupMock = coreMock.createSetup(); const startMock = coreMock.createStart(); @@ -103,6 +106,10 @@ describe('use_sync_query_state_with_url', () => { 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(); @@ -117,6 +124,16 @@ describe('use_sync_query_state_with_url', () => { result.current.startSyncingQueryStateWithUrl(); }); - expect(syncQueryStateWithUrl).toHaveBeenCalledTimes(1); + expect(mockSyncQueryStateWithUrl).toHaveBeenCalledTimes(1); + }); + + it('Should call stop callback when hook unmount', () => { + const { unmount } = renderHook(() => + useSyncQueryStateWithUrl(queryServiceStart, osdUrlStateStorage) + ); + + unmount(); + + expect(mockStopSync).toHaveBeenCalledTimes(1); }); }); From 2a0bfadafe3a94b90fcacf5b8b759f96ca90d06d Mon Sep 17 00:00:00 2001 From: Joey Liu Date: Wed, 5 Feb 2025 22:41:47 +0000 Subject: [PATCH 4/4] Revert save modal change Signed-off-by: Joey Liu --- .../save_modal/saved_object_save_modal.tsx | 302 +++++++++--------- 1 file changed, 153 insertions(+), 149 deletions(-) diff --git a/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx b/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx index b3d48dcd3f06..fcb013573ebc 100644 --- a/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx +++ b/src/plugins/saved_objects/public/save_modal/saved_object_save_modal.tsx @@ -47,11 +47,9 @@ import { EuiCompressedTextArea, } from '@elastic/eui'; import { FormattedMessage } from '@osd/i18n/react'; -import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import React from 'react'; import { EuiText } from '@elastic/eui'; import { i18n } from '@osd/i18n'; -import { unstable_batchedUpdates } from 'react-dom'; -import { SaveResult } from './show_saved_object_save_modal'; export interface OnSaveProps { newTitle: string; @@ -62,7 +60,7 @@ export interface OnSaveProps { } interface Props { - onSave: (props: OnSaveProps) => void | Promise; + onSave: (props: OnSaveProps) => void; onClose: () => void; title: string; showCopyOnSave: boolean; @@ -83,29 +81,98 @@ export interface SaveModalState { visualizationDescription: string; } -export const SavedObjectSaveModal = (props: Props) => { - const { - onSave, - onClose, - showCopyOnSave, - initialCopyOnSave, - objectType, - confirmButtonLabel, - options, - description, - showDescription, - } = props; - const warning = useRef(null); - const [title, setTitle] = useState(props.title); - const [copyOnSave, setCopyOnSave] = useState(!!initialCopyOnSave); - const [isTitleDuplicateConfirmed, setIsTitleDuplicateConfirmed] = useState(false); - const [hasTitleDuplicate, setHasTitleDuplicate] = useState(false); - const [isLoading, setIsLoading] = useState(false); - const [visualizationDescription, setVisualizationDescription] = useState(description || ''); - const duplicateWarningId = useMemo(() => htmlIdGenerator()(), []); - - const renderViewDescription = () => { - if (!showDescription) { +const generateId = htmlIdGenerator(); + +export class SavedObjectSaveModal extends React.Component { + private warning = React.createRef(); + public readonly state = { + title: this.props.title, + copyOnSave: Boolean(this.props.initialCopyOnSave), + isTitleDuplicateConfirmed: false, + hasTitleDuplicate: false, + isLoading: false, + visualizationDescription: this.props.description ? this.props.description : '', + }; + + public render() { + const { isTitleDuplicateConfirmed, hasTitleDuplicate, title } = this.state; + const duplicateWarningId = generateId(); + + return ( + + + + +

+ +

+ + + + + + {this.renderDuplicateTitleCallout(duplicateWarningId)} + + + {!this.props.showDescription && this.props.description && ( + + {this.props.description} + + )} + + + + {this.renderCopyOnSave()} + + + } + > + + + + {this.renderViewDescription()} + + {typeof this.props.options === 'function' + ? this.props.options(this.state) + : this.props.options} + + + + + + + + + {this.renderConfirmButton()} + + + ); + } + + private renderViewDescription = () => { + if (!this.props.showDescription) { return; } @@ -121,69 +188,82 @@ export const SavedObjectSaveModal = (props: Props) => { > ); }; - const onDescriptionChange = (event: React.ChangeEvent) => { - setVisualizationDescription(event.target.value); + private onDescriptionChange = (event: React.ChangeEvent) => { + this.setState({ + visualizationDescription: event.target.value, + }); }; - const onTitleDuplicate = () => { - unstable_batchedUpdates(() => { - setIsLoading(false); - setIsTitleDuplicateConfirmed(true); - setHasTitleDuplicate(true); + private onTitleDuplicate = () => { + this.setState({ + isLoading: false, + isTitleDuplicateConfirmed: true, + hasTitleDuplicate: true, }); - if (warning.current) { - warning.current.focus(); + if (this.warning.current) { + this.warning.current.focus(); } }; - const saveSavedObject = useCallback(async () => { - if (isLoading) { + private saveSavedObject = async () => { + if (this.state.isLoading) { // ignore extra clicks return; } - setIsLoading(true); + this.setState({ + isLoading: true, + }); - await onSave({ - newTitle: title, - newCopyOnSave: copyOnSave, - isTitleDuplicateConfirmed, - onTitleDuplicate, - newDescription: visualizationDescription, + await this.props.onSave({ + newTitle: this.state.title, + newCopyOnSave: this.state.copyOnSave, + isTitleDuplicateConfirmed: this.state.isTitleDuplicateConfirmed, + onTitleDuplicate: this.onTitleDuplicate, + newDescription: this.state.visualizationDescription, }); - }, [copyOnSave, isLoading, isTitleDuplicateConfirmed, onSave, title, visualizationDescription]); + }; - const onTitleChange = (event: React.ChangeEvent) => { - unstable_batchedUpdates(() => { - setTitle(event.target.value); - setIsTitleDuplicateConfirmed(false); - setHasTitleDuplicate(false); + private onTitleChange = (event: React.ChangeEvent) => { + this.setState({ + title: event.target.value, + isTitleDuplicateConfirmed: false, + hasTitleDuplicate: false, }); }; - const onCopyOnSaveChange = (event: EuiSwitchEvent) => { - setCopyOnSave(event.target.checked); + private onCopyOnSaveChange = (event: EuiSwitchEvent) => { + this.setState({ + copyOnSave: event.target.checked, + }); }; - const onFormSubmit = (event: React.FormEvent) => { + private onFormSubmit = (event: React.FormEvent) => { event.preventDefault(); - saveSavedObject(); + this.saveSavedObject(); }; - const renderConfirmButton = () => { - const confirmLabel: string | React.ReactNode = - confirmButtonLabel ?? - i18n.translate('savedObjects.saveModal.saveButtonLabel', { + private renderConfirmButton = () => { + const { isLoading, title } = this.state; + + let confirmLabel: string | React.ReactNode = i18n.translate( + 'savedObjects.saveModal.saveButtonLabel', + { defaultMessage: 'Save', - }); + } + ); + + if (this.props.confirmButtonLabel) { + confirmLabel = this.props.confirmButtonLabel; + } return ( { ); }; - const renderDuplicateTitleCallout = () => { - if (!hasTitleDuplicate) { + private renderDuplicateTitleCallout = (duplicateWarningId: string) => { + if (!this.state.hasTitleDuplicate) { return; } return ( <> -
+
} color="warning" @@ -224,7 +304,7 @@ export const SavedObjectSaveModal = (props: Props) => { id="savedObjects.saveModal.duplicateTitleDescription" defaultMessage="Saving '{title}' creates a duplicate title." values={{ - title, + title: this.state.title, }} />

@@ -235,8 +315,8 @@ export const SavedObjectSaveModal = (props: Props) => { ); }; - const renderCopyOnSave = () => { - if (!showCopyOnSave) { + private renderCopyOnSave = () => { + if (!this.props.showCopyOnSave) { return; } @@ -244,13 +324,13 @@ export const SavedObjectSaveModal = (props: Props) => { <> } /> @@ -258,80 +338,4 @@ export const SavedObjectSaveModal = (props: Props) => { ); }; - - return ( - - - - -

- -

- - - - - - {renderDuplicateTitleCallout()} - - - {!showDescription && description && ( - - {description} - - )} - - - - {renderCopyOnSave()} - - - } - > - - - - {renderViewDescription()} - - {typeof options === 'function' - ? options({ - title, - copyOnSave, - isTitleDuplicateConfirmed, - hasTitleDuplicate, - isLoading, - visualizationDescription, - }) - : options} - - - - - - - - - {renderConfirmButton()} - - - ); -}; +}