Skip to content
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

Bug 1795707: improve notification drawer performance #4192

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions analyze.sh
Expand Up @@ -14,11 +14,12 @@ if [ -d "$ARTIFACT_DIR" ]; then
cp public/dist/report.html "${ARTIFACT_DIR}"
fi

MAX_BYTES=2621440 # ~2.5 MiB
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do floating point calculations in bash (2.5 * 1024 * 1024), so I just hard-coded the value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett i ran into this too...was wondering how you wanted this to be modified...

VENDORS_MAIN_BYTES=$(jq -r '.assets[] | select(.name | match("^vendors~main-chunk.*js$")) | .size' public/dist/stats.json)
DISPLAY_VALUE=$(awk "BEGIN {printf \"%.2f\n\", $VENDORS_MAIN_BYTES/1024/1024}")
echo "Main vendor bundle size: $DISPLAY_VALUE MiB"
if (( $VENDORS_MAIN_BYTES > (2 * 1024 * 1024) )); then
echo "FAILURE: Main vendor bundle is larger than the 2 MiB limit."
if (( VENDORS_MAIN_BYTES > MAX_BYTES )); then
echo "FAILURE: Main vendor bundle is larger than the 2.5 MiB limit."
echo "If you haven't added a new dependency, an import might have accidentally pulled an existing dependency into the main vendor bundle."
echo "If adding a large dependency, consider lazy loading the component with AsyncComponent."
exit 1
Expand Down
14 changes: 4 additions & 10 deletions frontend/public/components/app.jsx
Expand Up @@ -12,8 +12,9 @@ import { detectFeatures } from '../actions/features';
import AppContents from './app-contents';
import { getBrandingDetails, Masthead } from './masthead';
import { ConsoleNotifier } from './console-notifier';
import { ConnectedNotificationDrawer } from './notification-drawer';
import { Navigation } from './nav';
import { history, Firehose, AsyncComponent } from './utils';
import { history, Firehose } from './utils';
import * as UIActions from '../actions/ui';
import { fetchSwagger, getCachedResources, referenceForModel } from '../module/k8s';
import { receivedResources, watchAPIServices } from '../actions/k8s';
Expand Down Expand Up @@ -41,13 +42,6 @@ const cvResource = [
// Edge lacks URLSearchParams
import 'url-search-params-polyfill';

const NotificationDrawer = (props) => (
<AsyncComponent
loader={() => import('./notification-drawer').then((c) => c.ConnectedNotificationDrawer)}
{...props}
/>
);

class App extends React.PureComponent {
constructor(props) {
super(props);
Expand Down Expand Up @@ -147,9 +141,9 @@ class App extends React.PureComponent {
}
>
<Firehose resources={cvResource}>
<NotificationDrawer isDesktop={isDrawerInline}>
<ConnectedNotificationDrawer isDesktop={isDrawerInline}>
<AppContents />
</NotificationDrawer>
</ConnectedNotificationDrawer>
</Firehose>
</Page>
<ConsoleNotifier location="BannerBottom" />
Expand Down
28 changes: 12 additions & 16 deletions frontend/public/components/notification-drawer.tsx
Expand Up @@ -31,12 +31,7 @@ import {

import { coFetchJSON } from '../co-fetch';
import { FirehoseResult } from './utils';
import {
ClusterUpdate,
ClusterVersionKind,
hasAvailableUpdates,
K8sResourceKind,
} from '../module/k8s';
import { ClusterUpdate, ClusterVersionKind } from '../module/k8s';
import { getSortedUpdates } from './modals/cluster-update-modal';
import { usePrevious } from '@console/metal3-plugin/src/hooks';

Expand Down Expand Up @@ -145,10 +140,10 @@ export const ConnectedNotificationDrawer_: React.FC<ConnectedNotificationDrawerP
}
return () => pollerTimeout.clearTimeout;
}, []);
const cv: ClusterVersionKind = _.get(resources.cv, 'data') as ClusterVersionKind;
const cvLoaded: boolean = _.get(resources.cv, 'loaded');
const updateData: ClusterUpdate[] = hasAvailableUpdates(cv) ? getSortedUpdates(cv) : [];
const { data, loaded, loadError } = alerts;
const cv: ClusterVersionKind = resources.cv?.data;
const cvLoaded: boolean = resources.cv?.loaded;
const updateData: ClusterUpdate[] = getSortedUpdates(cv);
const { data, loaded, loadError } = alerts || {};

const updateList: React.ReactNode[] = getUpdateNotificationEntries(
cvLoaded,
Expand Down Expand Up @@ -176,11 +171,12 @@ export const ConnectedNotificationDrawer_: React.FC<ConnectedNotificationDrawerP
const [isClusterUpdateExpanded, toggleClusterUpdateExpanded] = React.useState<boolean>(false);
const prevDrawerToggleState = usePrevious(isDrawerExpanded);

const hasCriticalAlerts = criticalAlertList.length > 0;
React.useEffect(() => {
if (criticalAlertList.length > 0 && !prevDrawerToggleState && isDrawerExpanded) {
toggleAlertExpanded(!_.isEmpty(criticalAlertList));
if (hasCriticalAlerts && !prevDrawerToggleState && isDrawerExpanded) {
toggleAlertExpanded(true);
}
}, [criticalAlertList, isAlertExpanded, isDrawerExpanded, prevDrawerToggleState]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

criticalAlertList was different every render, so this always fired

}, [hasCriticalAlerts, isAlertExpanded, isDrawerExpanded, prevDrawerToggleState]);

const emptyState = !_.isEmpty(loadError) ? (
<AlertErrorState errorText={loadError.message} />
Expand Down Expand Up @@ -246,7 +242,7 @@ type NotificationPoll = (url: string, dataHandler: (data) => any) => void;
export type WithNotificationsProps = {
isDrawerExpanded: boolean;
notificationsRead: boolean;
alerts: {
alerts?: {
data: Alert[];
loaded: boolean;
loadError?: {
Expand All @@ -269,14 +265,14 @@ export type ConnectedNotificationDrawerProps = {
};
};
resources?: {
[key: string]: FirehoseResult | FirehoseResult<K8sResourceKind>;
cv: FirehoseResult<ClusterVersionKind>;
};
};

const notificationStateToProps = ({ UI }: RootState): WithNotificationsProps => ({
isDrawerExpanded: !!UI.getIn(['notifications', 'isExpanded']),
notificationsRead: !!UI.getIn(['notifications', 'isRead']),
alerts: UI.getIn(['monitoring', 'notificationAlerts']) || {},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a different empty object each time caused unnecessary renders.

alerts: UI.getIn(['monitoring', 'notificationAlerts']),
});

type AlertErrorProps = {
Expand Down