diff --git a/src/app/Shared/Services/Target.service.tsx b/src/app/Shared/Services/Target.service.tsx index d2dbb6a12..a6abd8c30 100644 --- a/src/app/Shared/Services/Target.service.tsx +++ b/src/app/Shared/Services/Target.service.tsx @@ -60,10 +60,10 @@ export const indexOfTarget = (arr: Target[], target: Target): number => { export interface Target { connectUrl: string; alias: string; - labels?: Map; + labels?: {}; annotations?: { - cryostat: Map; - platform: Map; + cryostat: {}; + platform: {}; }; } diff --git a/src/app/TargetSelect/CreateTargetModal.tsx b/src/app/TargetSelect/CreateTargetModal.tsx index f136ddde8..0237ead38 100644 --- a/src/app/TargetSelect/CreateTargetModal.tsx +++ b/src/app/TargetSelect/CreateTargetModal.tsx @@ -38,7 +38,6 @@ import { Target } from '@app/Shared/Services/Target.service'; import { ActionGroup, - AlertVariant, Button, ButtonType, Form, @@ -56,14 +55,14 @@ export interface CreateTargetModalProps { onDismiss: () => void; } +const jmxServiceUrlFormat = /service:jmx:([a-zA-Z0-9-]+)/g; +const hostPortPairFormat = /([a-zA-Z0-9-]+):([0-9]+)/g; + export const CreateTargetModal: React.FunctionComponent = (props) => { const [connectUrl, setConnectUrl] = React.useState(''); const [validConnectUrl, setValidConnectUrl] = React.useState(ValidatedOptions.default); const [alias, setAlias] = React.useState(''); - const jmxServiceUrlFormat = /service:jmx:([a-zA-Z0-9-]+)/g; - const hostPortPairFormat = /([a-zA-Z0-9-]+):([0-9]+)/g; - const createTarget = React.useCallback(() => { props.onSubmit({ connectUrl, alias: alias.trim() || connectUrl }); setConnectUrl(''); @@ -72,11 +71,11 @@ export const CreateTargetModal: React.FunctionComponent const handleKeyDown = React.useCallback( (evt) => { - if (evt.key === 'Enter') { + if (connectUrl && evt.key === 'Enter') { createTarget(); } }, - [createTarget] + [createTarget, connectUrl] ); const handleSubmit = React.useCallback(() => { @@ -87,7 +86,7 @@ export const CreateTargetModal: React.FunctionComponent } else { setValidConnectUrl(ValidatedOptions.error); } - }, [createTarget, setValidConnectUrl]); + }, [createTarget, setValidConnectUrl, connectUrl]); return ( <> diff --git a/src/app/TargetSelect/TargetSelect.tsx b/src/app/TargetSelect/TargetSelect.tsx index 3aa2f03fe..d00404708 100644 --- a/src/app/TargetSelect/TargetSelect.tsx +++ b/src/app/TargetSelect/TargetSelect.tsx @@ -38,7 +38,7 @@ import * as React from 'react'; import { ServiceContext } from '@app/Shared/Services/Services'; import { NotificationsContext } from '@app/Notifications/Notifications'; -import { NO_TARGET, Target } from '@app/Shared/Services/Target.service'; +import { isEqualTarget, NO_TARGET, Target } from '@app/Shared/Services/Target.service'; import { useSubscriptions } from '@app/utils/useSubscriptions'; import { Button, @@ -53,22 +53,23 @@ import { Text, TextVariants, } from '@patternfly/react-core'; -import { ContainerNodeIcon, PlusCircleIcon, Spinner2Icon, TrashIcon } from '@patternfly/react-icons'; +import { ContainerNodeIcon, PlusCircleIcon, TrashIcon } from '@patternfly/react-icons'; import { of } from 'rxjs'; import { catchError, first } from 'rxjs/operators'; import { CreateTargetModal } from './CreateTargetModal'; -import _ from 'lodash'; -import { NotificationCategory } from '@app/Shared/Services/NotificationChannel.service'; import { DeleteWarningType } from '@app/Modal/DeleteWarningUtils'; import { DeleteWarningModal } from '@app/Modal/DeleteWarningModal'; +import { getFromLocalStorage, removeFromLocalStorage, saveToLocalStorage } from '@app/utils/LocalStorage'; export const CUSTOM_TARGETS_REALM = 'Custom Targets'; + export interface TargetSelectProps {} export const TargetSelect: React.FunctionComponent = (props) => { - const TARGET_KEY = 'target'; const notifications = React.useContext(NotificationsContext); const context = React.useContext(ServiceContext); + const addSubscription = useSubscriptions(); + const [selected, setSelected] = React.useState(NO_TARGET); const [targets, setTargets] = React.useState([] as Target[]); const [expanded, setExpanded] = React.useState(false); @@ -76,112 +77,69 @@ export const TargetSelect: React.FunctionComponent = (props) const [isModalOpen, setModalOpen] = React.useState(false); const [warningModalOpen, setWarningModalOpen] = React.useState(false); - const addSubscription = useSubscriptions(); - const setCachedTargetSelection = React.useCallback( - (target) => localStorage.setItem(TARGET_KEY, JSON.stringify(target)), - [localStorage] + (target, errorCallback?) => saveToLocalStorage('TARGET', target, errorCallback), + [] ); - const removeCachedTargetSelection = React.useCallback(() => { - localStorage.removeItem(TARGET_KEY); - }, [localStorage]); - - const getCachedTargetSelection = React.useCallback(() => { - const cachedTarget = localStorage.getItem(TARGET_KEY); - return cachedTarget ? JSON.parse(cachedTarget) : NO_TARGET; - }, [localStorage]); - - const selectTargetFromCache = React.useCallback( - (targets) => { - if (targets.length === 0) { - return; - } + const removeCachedTargetSelection = React.useCallback(() => removeFromLocalStorage('TARGET'), []); - try { - const cachedTarget = getCachedTargetSelection(); - const cachedTargetExists = targets.some((target) => _.isEqual(cachedTarget, target)); + const getCachedTargetSelection = React.useCallback(() => getFromLocalStorage('TARGET', NO_TARGET), []); - if (cachedTargetExists) { - context.target.setTarget(cachedTarget); - } else { - removeCachedTargetSelection(); - } - } catch (error) { - context.target.setTarget(NO_TARGET); - removeCachedTargetSelection(); - } - }, - [context, context.target, getCachedTargetSelection, removeCachedTargetSelection] - ); + const resetTargetSelection = React.useCallback(() => { + context.target.setTarget(NO_TARGET); + removeCachedTargetSelection(); + }, [context.target, removeCachedTargetSelection]); const onSelect = React.useCallback( + // ATTENTION: do not add onSelect as deps for effect hook as it updates with selected states (evt, selection, isPlaceholder) => { if (isPlaceholder) { - context.target.setTarget(NO_TARGET); - removeCachedTargetSelection(); + resetTargetSelection(); } else { - if (selection != selected) { - try { - context.target.setTarget(selection); - setCachedTargetSelection(selection); - } catch (error) { - notifications.danger('Cannot set target', (error as any).message); + if (!isEqualTarget(selection, selected)) { + context.target.setTarget(selection); + setCachedTargetSelection(selection, () => { + notifications.danger('Cannot set target'); context.target.setTarget(NO_TARGET); - } + }); } } setExpanded(false); }, - [ - context, - context.target, - selected, - notifications, - setExpanded, - removeCachedTargetSelection, - setCachedTargetSelection, - ] + [context.target, notifications, setExpanded, setCachedTargetSelection, resetTargetSelection, selected] ); - const selectNone = React.useCallback(() => { - onSelect(undefined, undefined, true); - }, [onSelect]); + const selectTargetFromCache = React.useCallback( + (targets) => { + if (!targets.length) { + // Ignore first emitted value + return; + } + const cachedTarget = getCachedTargetSelection(); + const cachedTargetExists = targets.some((target: Target) => isEqualTarget(cachedTarget, target)); + if (cachedTargetExists) { + context.target.setTarget(cachedTarget); + } else { + resetTargetSelection(); + } + }, + [context.target, getCachedTargetSelection, resetTargetSelection] + ); React.useEffect(() => { addSubscription( context.targets.targets().subscribe((targets) => { + // Target Discovery notifications will trigger an event here. setTargets(targets); selectTargetFromCache(targets); }) ); - }, [addSubscription, context, context.targets, setTargets, selectTargetFromCache]); + }, [addSubscription, context.targets, setTargets, selectTargetFromCache]); React.useEffect(() => { - addSubscription( - context.notificationChannel.messages(NotificationCategory.TargetJvmDiscovery).subscribe((v) => { - const evt: TargetDiscoveryEvent = v.message.event; - if (evt.kind === 'LOST') { - const target: Target = { - connectUrl: evt.serviceRef.connectUrl, - alias: evt.serviceRef.alias, - }; - context.target - .target() - .pipe(first()) - .subscribe((currentTarget) => { - if (currentTarget.connectUrl === target.connectUrl && currentTarget.alias === target.alias) { - selectNone(); - } - }); - } - }) - ); - }, [addSubscription, context, context.notificationChannel, context.target, selectNone]); - - React.useLayoutEffect(() => { addSubscription(context.target.target().subscribe(setSelected)); - }, [addSubscription, context, context.target, setSelected]); + }, [addSubscription, context.target, setSelected]); const showCreateTargetModal = React.useCallback(() => { setModalOpen(true); @@ -206,11 +164,7 @@ export const TargetSelect: React.FunctionComponent = (props) }) ); }, - [addSubscription, context, context.api, notifications, setLoading, setModalOpen] - ); - const deletionDialogsEnabled = React.useMemo( - () => context.settings.deletionDialogsEnabledFor(DeleteWarningType.DeleteCustomTargets), - [context, context.settings, context.settings.deletionDialogsEnabledFor] + [addSubscription, context.api, notifications, setLoading, setModalOpen] ); const deleteTarget = React.useCallback(() => { @@ -219,24 +173,24 @@ export const TargetSelect: React.FunctionComponent = (props) context.api .deleteTarget(selected) .pipe(first()) - .subscribe( - () => { - selectNone(); - setLoading(false); - }, - () => { + .subscribe({ + next: () => setLoading(false), + error: () => { setLoading(false); - let id: string; - if (selected.alias === selected.connectUrl) { - id = selected.alias; - } else { - id = `${selected.alias} [${selected.connectUrl}]`; - } + const id = + !selected.alias || selected.alias === selected.connectUrl + ? selected.connectUrl + : `${selected.alias} [${selected.connectUrl}]`; notifications.danger('Target Deletion Failed', `The selected target (${id}) could not be deleted`); - } - ) + }, + }) ); - }, [addSubscription, context, context.api, notifications, selected, setLoading, selectNone]); + }, [addSubscription, context.api, notifications, selected, setLoading]); + + const deletionDialogsEnabled = React.useMemo( + () => context.settings.deletionDialogsEnabledFor(DeleteWarningType.DeleteCustomTargets), + [context.settings] + ); const handleDeleteButton = React.useCallback(() => { if (deletionDialogsEnabled) { @@ -270,17 +224,11 @@ export const TargetSelect: React.FunctionComponent = (props) [ , ].concat( - targets.map((t: Target) => - t.alias == t.connectUrl || !t.alias ? ( - {`${t.connectUrl}`} - ) : ( - {`${t.alias} (${t.connectUrl})`} - ) - ) + targets.map((t: Target) => ( + + {!t.alias || t.alias === t.connectUrl ? `${t.connectUrl}` : `${t.alias} (${t.connectUrl})`} + + )) ), [targets] ); @@ -315,12 +263,12 @@ export const TargetSelect: React.FunctionComponent = (props) @@ -335,8 +283,3 @@ export const TargetSelect: React.FunctionComponent = (props) ); }; - -interface TargetDiscoveryEvent { - kind: 'LOST' | 'FOUND'; - serviceRef: Target; -} diff --git a/src/app/TargetView/TargetView.tsx b/src/app/TargetView/TargetView.tsx index 99fd15e87..07619df68 100644 --- a/src/app/TargetView/TargetView.tsx +++ b/src/app/TargetView/TargetView.tsx @@ -60,9 +60,12 @@ export const TargetView: React.FunctionComponent = (props) => { setConnected(!!target && !!target.connectUrl); }) ); - }, [context.target, addSubscription]); + }, [context.target, addSubscription, setConnected]); - const compact = props.compactSelect == null ? true : props.compactSelect; + const compact = React.useMemo( + () => (props.compactSelect == null ? true : props.compactSelect), + [props.compactSelect] + ); return ( <> diff --git a/src/app/utils/LocalStorage.ts b/src/app/utils/LocalStorage.ts index ded7821f1..e9d8d815e 100644 --- a/src/app/utils/LocalStorage.ts +++ b/src/app/utils/LocalStorage.ts @@ -40,6 +40,7 @@ export enum LocalStorageKey { TARGET_RECORDING_FILTERS, JMX_CREDENTIAL_LOCATION, JMX_CREDENTIALS, + TARGET, } /** @@ -60,10 +61,24 @@ export const getFromLocalStorage = (key: LocalStorageKeyStrings, defaultValue: a } }; -export const saveToLocalStorage = (key: LocalStorageKeyStrings, value: any) => { +export const saveToLocalStorage = (key: LocalStorageKeyStrings, value: any, error?: () => void) => { try { if (typeof window !== 'undefined') { window.localStorage.setItem(key, JSON.stringify(value)); } - } catch (error) {} // If error (i.e. users disable storage for the site), saving is aborted and skipped. + } catch (err) { + console.warn(err); + error && error(); + } +}; + +export const removeFromLocalStorage = (key: LocalStorageKeyStrings, error?: () => void): any => { + try { + if (typeof window !== 'undefined') { + window.localStorage.removeItem(key); + } + } catch (err) { + console.warn(err); + error && error(); + } }; diff --git a/src/test/Rules/CreateRule.test.tsx b/src/test/Rules/CreateRule.test.tsx index 6a6f81e09..0e75ed1c8 100644 --- a/src/test/Rules/CreateRule.test.tsx +++ b/src/test/Rules/CreateRule.test.tsx @@ -105,8 +105,8 @@ const createSpy = jest.spyOn(defaultServices.api, 'createRule').mockReturnValue( jest.spyOn(defaultServices.notificationChannel, 'messages').mockReturnValue(of(mockTargetFoundNotification)); jest.spyOn(defaultServices.api, 'doGet').mockReturnValue(of([mockEventTemplate])); jest.spyOn(defaultServices.target, 'target').mockReturnValue(of(mockTarget)); +jest.spyOn(defaultServices.target, 'setTarget').mockReturnValue(); jest.spyOn(defaultServices.targets, 'targets').mockReturnValue(of([mockTarget])); -jest.spyOn(defaultServices.targets, 'queryForTargets').mockReturnValue(of()); jest.spyOn(defaultServices.target, 'authFailure').mockReturnValue(of()); describe('', () => { @@ -134,6 +134,7 @@ describe('', () => { const subj = new Subject(); const mockTargetSvc = { target: () => of(mockTarget), + setTarget: (target) => {}, authFailure: () => subj.asObservable(), } as TargetService; const services: Services = { diff --git a/src/test/Targets/TargetSelect.test.tsx b/src/test/TargetSelect/TargetSelect.test.tsx similarity index 88% rename from src/test/Targets/TargetSelect.test.tsx rename to src/test/TargetSelect/TargetSelect.test.tsx index 11034102d..a47da91bf 100644 --- a/src/test/Targets/TargetSelect.test.tsx +++ b/src/test/TargetSelect/TargetSelect.test.tsx @@ -36,7 +36,7 @@ * SOFTWARE. */ import * as React from 'react'; -import renderer from 'react-test-renderer'; +import renderer, { act } from 'react-test-renderer'; import { cleanup, render, screen, waitFor, within } from '@testing-library/react'; import '@testing-library/jest-dom'; import { createMemoryHistory } from 'history'; @@ -51,15 +51,15 @@ const mockFooConnectUrl = 'service:jmx:rmi://someFooUrl'; const mockBarConnectUrl = 'service:jmx:rmi://someBarUrl'; const mockBazConnectUrl = 'service:jmx:rmi://someBazUrl'; -// Test fails if new Map([['REALM', 'Custom Targets']]) is used, most likely since 'cryostat' Map is not being utilized -const cryostatAnnotation = new Map(); -cryostatAnnotation['REALM'] = CUSTOM_TARGETS_REALM; +const cryostatAnnotation = { + REALM: CUSTOM_TARGETS_REALM, +}; const mockFooTarget: Target = { connectUrl: mockFooConnectUrl, alias: 'fooTarget', annotations: { cryostat: cryostatAnnotation, - platform: new Map(), + platform: {}, }, }; const mockBarTarget: Target = { ...mockFooTarget, connectUrl: mockBarConnectUrl, alias: 'barTarget' }; @@ -73,6 +73,10 @@ jest.mock('react-router-dom', () => ({ useHistory: () => history, })); +jest.mock('@app/Shared/Services/Target.service', () => ({ + ...jest.requireActual('@app/Shared/Services/Target.service'), // Require actual implementation of utility functions for Target +})); + jest .spyOn(defaultServices.target, 'target') .mockReturnValueOnce(of()) // renders correctly @@ -94,10 +98,6 @@ jest.spyOn(defaultServices.api, 'createTarget').mockReturnValue(of(true)); jest.spyOn(defaultServices.api, 'deleteTarget').mockReturnValue(of(true)); -jest.spyOn(defaultServices.targets, 'queryForTargets').mockReturnValue(of()); - -jest.spyOn(defaultServices.notificationChannel, 'messages').mockReturnValue(of()); - jest .spyOn(defaultServices.settings, 'deletionDialogsEnabledFor') .mockReturnValueOnce(true) // renders correctly @@ -107,15 +107,18 @@ jest .mockReturnValueOnce(false) // deletes target when delete button clicked .mockReturnValue(true); // other tests -afterEach(cleanup); - describe('', () => { - it('renders correctly', () => { - const tree = renderer.create( - - - - ); + afterEach(cleanup); + + it('renders correctly', async () => { + let tree; + await act(async () => { + tree = renderer.create( + + + + ); + }); expect(tree.toJSON()).toMatchSnapshot(); }); @@ -145,7 +148,7 @@ describe('', () => { expect(screen.getByText(`fooTarget`)).toBeInTheDocument(); userEvent.click(screen.getByLabelText('Options menu')); - expect(screen.getByLabelText('Select Input')).toBeInTheDocument(); + expect(screen.getByLabelText('Select Target')).toBeInTheDocument(); expect(screen.getByText(`Select target...`)).toBeInTheDocument(); expect(screen.getByText(`fooTarget (service:jmx:rmi://someFooUrl)`)).toBeInTheDocument(); expect(screen.getByText(`barTarget (service:jmx:rmi://someBarUrl)`)).toBeInTheDocument(); @@ -233,4 +236,23 @@ describe('', () => { expect(deleteTargetRequestSpy).toBeCalledTimes(1); expect(deleteTargetRequestSpy).toBeCalledWith(mockFooTarget); }); + + it('does not create a target if user leaves connectUrl empty', () => { + const createTargetRequestSpy = jest.spyOn(defaultServices.api, 'createTarget'); + render( + + + + ); + const createButton = screen.getByLabelText('Create target'); + userEvent.click(createButton); + + const confirmButton = screen.getByText('Create'); + expect(confirmButton).toBeInTheDocument(); + expect(confirmButton).toBeVisible(); + expect(confirmButton).toBeDisabled(); + + userEvent.keyboard('{Enter}'); + expect(createTargetRequestSpy).toBeCalledTimes(0); + }); }); diff --git a/src/test/Targets/__snapshots__/TargetSelect.test.tsx.snap b/src/test/TargetSelect/__snapshots__/TargetSelect.test.tsx.snap similarity index 100% rename from src/test/Targets/__snapshots__/TargetSelect.test.tsx.snap rename to src/test/TargetSelect/__snapshots__/TargetSelect.test.tsx.snap