-
Notifications
You must be signed in to change notification settings - Fork 279
Fix health check AlertNotification #2571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
AlertNotification has "Try again" button which doesn't do anything, because of some of the dependencies in useEffect are not updated or badly composed registerSetInterval function.
Hi, thanks for opening this PR! Regarding the issue here, it would be best to log it in the repo here And then you can link the issue to this PR and avoid having a super long PR description. This should generally be reserved for a short description of the changes, along with any helpful instructions to test the changes for anyone reviewing. Feel free to reference our contribution guidelines and reach out if you have any questions :) |
thanks for this fix! Appreciate it. A few notes about the CI build errors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a previous comment about how to address the CI job failures.
Hi @callmevladik . Are you able to follow up on the PR's comments? |
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
|
Describe the bug
AlertNotification has "Try again" button which doesn't do anything, the potential reason could be that some of the dependencies in useEffect are not updated or badly composed registerSetInterval function with some closure complexity.
To Reproduce
Steps to reproduce the bug:
For testing purposes
As I didn't know how to mock server healthz response, I just setup [counter, setCounter] state, added condition "if counter % 2 === 0" inside performHealthCheck with setting error if true, else "return checkerFunction()...", also I added click handler which increments counter and invokes performHealthCheck with new counter value.
So with my mocking the scenario was:
`export function PureAlertNotification({ checkerFunction }: PureAlertNotificationProps) {
const [networkStatusCheckTimeFactor, setNetworkStatusCheckTimeFactor] = React.useState(0);
const [error, setError] = React.useState<null | string | boolean>(null);
const { t } = useTranslation();
const { pathname } = useLocation();
const [counter, setCounter] = React.useState(0);
const performHealthCheck = React.useCallback((counter: number) => {
if (!window.navigator.onLine) {
setError('Offline');
return;
}
}, []);
React.useEffect(() => {
const interval = setInterval(
() => performHealthCheck(counter),
(networkStatusCheckTimeFactor + 1) * NETWORK_STATUS_CHECK_TIME
);
return () => clearInterval(interval);
}, [performHealthCheck, networkStatusCheckTimeFactor, counter]);
// Make sure we do not show the alert notification if we are not on a cluster route.
React.useEffect(() => {
if (!getCluster()) {
setError(null);
}
}, [pathname]);
const showOnRoute = React.useMemo(() => {
for (const route of ROUTES_WITHOUT_ALERT) {
const routePath = getRoutePath(getRoute(route));
if (matchPath(pathname, routePath)?.isExact) {
return false;
}
}
return true;
}, [pathname]);
if (!error || !showOnRoute) {
return null;
}
const handleClick = () => {
setCounter(prev => {
const newValue = prev + 1;
performHealthCheck(newValue);
return newValue;
});
};
return (
<Alert
variant="filled"
severity="error"
sx={theme => ({
color: theme.palette.common.white,
background: theme.palette.error.main,
textAlign: 'center',
display: 'flex',
paddingTop: theme.spacing(0.5),
paddingBottom: theme.spacing(1),
paddingRight: theme.spacing(3),
justifyContent: 'center',
position: 'fixed',
zIndex: theme.zIndex.snackbar + 1,
top: '0',
alignItems: 'center',
left: '50%',
width: 'auto',
transform: 'translateX(-50%)',
})}
action={
<Button
sx={theme => ({
color: theme.palette.error.main,
borderColor: theme.palette.error.main,
background: theme.palette.common.white,
lineHeight: theme.typography.body2.lineHeight,
'&:hover': {
color: theme.palette.common.white,
borderColor: theme.palette.common.white,
background: theme.palette.error.dark,
},
})}
onClick={handleClick}
size="small"
>
{t('translation|Try Again')}
}
>
<Typography
variant="body2"
sx={theme => ({
paddingTop: theme.spacing(0.5),
fontWeight: 'bold',
fontSize: '16px',
})}
>
{t('translation|Lost connection to the cluster.')}
);
}`