Skip to content

Commit

Permalink
fix(target): disable target creation if connectUrl is left empty (#566)
Browse files Browse the repository at this point in the history 
* fix(target): should not create target if targetUrl is empty

* chore(target): clean up

* chore(target): do not use Map for labels/annotations

* tests(target): adjust unit tests

* chore(target): apply prettier

* fix(target): fix undeclared vars error

* tests(target): add missing mocks

* fix(target): fix indefinite effects

* chore(target): apply prettier

* chore(target): clean up hook deps

* fix(target): remove setState
  • Loading branch information
Thuan Vo authored Oct 26, 2022
1 parent cbcecf6 commit 057e27e
Show file tree
Hide file tree
Showing 8 changed files with 137 additions and 154 deletions.
6 changes: 3 additions & 3 deletions src/app/Shared/Services/Target.service.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,10 @@ export const indexOfTarget = (arr: Target[], target: Target): number => {
export interface Target {
connectUrl: string;
alias: string;
labels?: Map<string, string>;
labels?: {};
annotations?: {
cryostat: Map<string, string>;
platform: Map<string, string>;
cryostat: {};
platform: {};
};
}

Expand Down
13 changes: 6 additions & 7 deletions src/app/TargetSelect/CreateTargetModal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import { Target } from '@app/Shared/Services/Target.service';
import {
ActionGroup,
AlertVariant,
Button,
ButtonType,
Form,
Expand All @@ -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<CreateTargetModalProps> = (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('');
Expand All @@ -72,11 +71,11 @@ export const CreateTargetModal: React.FunctionComponent<CreateTargetModalProps>

const handleKeyDown = React.useCallback(
(evt) => {
if (evt.key === 'Enter') {
if (connectUrl && evt.key === 'Enter') {
createTarget();
}
},
[createTarget]
[createTarget, connectUrl]
);

const handleSubmit = React.useCallback(() => {
Expand All @@ -87,7 +86,7 @@ export const CreateTargetModal: React.FunctionComponent<CreateTargetModalProps>
} else {
setValidConnectUrl(ValidatedOptions.error);
}
}, [createTarget, setValidConnectUrl]);
}, [createTarget, setValidConnectUrl, connectUrl]);

return (
<>
Expand Down
185 changes: 64 additions & 121 deletions src/app/TargetSelect/TargetSelect.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -53,135 +53,93 @@ 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<TargetSelectProps> = (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);
const [isLoading, setLoading] = React.useState(false);
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);
Expand All @@ -206,11 +164,7 @@ export const TargetSelect: React.FunctionComponent<TargetSelectProps> = (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(() => {
Expand All @@ -219,24 +173,24 @@ export const TargetSelect: React.FunctionComponent<TargetSelectProps> = (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) {
Expand Down Expand Up @@ -270,17 +224,11 @@ export const TargetSelect: React.FunctionComponent<TargetSelectProps> = (props)
[
<SelectOption key="placeholder" value="Select target..." isPlaceholder={true} itemCount={targets.length} />,
].concat(
targets.map((t: Target) =>
t.alias == t.connectUrl || !t.alias ? (
<SelectOption key={t.connectUrl} value={t} isPlaceholder={false}>{`${t.connectUrl}`}</SelectOption>
) : (
<SelectOption
key={t.connectUrl}
value={t}
isPlaceholder={false}
>{`${t.alias} (${t.connectUrl})`}</SelectOption>
)
)
targets.map((t: Target) => (
<SelectOption key={t.connectUrl} value={t} isPlaceholder={false}>
{!t.alias || t.alias === t.connectUrl ? `${t.connectUrl}` : `${t.alias} (${t.connectUrl})`}
</SelectOption>
))
),
[targets]
);
Expand Down Expand Up @@ -315,12 +263,12 @@ export const TargetSelect: React.FunctionComponent<TargetSelectProps> = (props)
<Select
toggleIcon={<ContainerNodeIcon />}
variant={SelectVariant.single}
selections={selected.alias || selected.connectUrl}
onSelect={onSelect}
onToggle={setExpanded}
selections={selected.alias || selected.connectUrl}
isDisabled={isLoading}
isOpen={expanded}
aria-label="Select Input"
aria-label="Select Target"
>
{selectOptions}
</Select>
Expand All @@ -335,8 +283,3 @@ export const TargetSelect: React.FunctionComponent<TargetSelectProps> = (props)
</>
);
};

interface TargetDiscoveryEvent {
kind: 'LOST' | 'FOUND';
serviceRef: Target;
}
7 changes: 5 additions & 2 deletions src/app/TargetView/TargetView.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,12 @@ export const TargetView: React.FunctionComponent<TargetViewProps> = (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 (
<>
Expand Down
19 changes: 17 additions & 2 deletions src/app/utils/LocalStorage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export enum LocalStorageKey {
TARGET_RECORDING_FILTERS,
JMX_CREDENTIAL_LOCATION,
JMX_CREDENTIALS,
TARGET,
}

/**
Expand All @@ -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();
}
};
Loading

0 comments on commit 057e27e

Please sign in to comment.