diff --git a/packages/grafana-data/src/events/EventBus.test.ts b/packages/grafana-data/src/events/EventBus.test.ts index 6de48b5d54e..3708b47d78b 100644 --- a/packages/grafana-data/src/events/EventBus.test.ts +++ b/packages/grafana-data/src/events/EventBus.test.ts @@ -91,8 +91,10 @@ describe('EventBus', () => { it('Supports legacy events', () => { const bus = new EventBusSrv(); const events: LegacyEventPayload[] = []; - const handler = (event: LegacyEventPayload) => { - events.push(event); + const handler = (event?: LegacyEventPayload) => { + if (event) { + events.push(event); + } }; bus.on(legacyEvent, handler); @@ -111,7 +113,9 @@ describe('EventBus', () => { const newEvents: AlertSuccessEvent[] = []; bus.on(legacyEvent, (event) => { - legacyEvents.push(event); + if (event) { + legacyEvents.push(event); + } }); bus.subscribe(AlertSuccessEvent, (event) => { diff --git a/packages/grafana-data/src/events/types.ts b/packages/grafana-data/src/events/types.ts index 514b980d1b4..ce5499d00ed 100644 --- a/packages/grafana-data/src/events/types.ts +++ b/packages/grafana-data/src/events/types.ts @@ -133,12 +133,12 @@ export interface LegacyEmitter { /** * @deprecated use $on */ - off(event: AppEvent | string, handler: (payload?: T) => void): void; + off(event: AppEvent | string, handler: LegacyEventHandler): void; } /** @public */ export interface LegacyEventHandler { - (payload: T): void; + (payload?: T): void; wrapper?: (event: BusEvent) => void; } diff --git a/public/app/core/components/AppNotifications/AppNotificationList.test.tsx b/public/app/core/components/AppNotifications/AppNotificationList.test.tsx index 61500e0cd4c..3c3eb04a44d 100644 --- a/public/app/core/components/AppNotifications/AppNotificationList.test.tsx +++ b/public/app/core/components/AppNotifications/AppNotificationList.test.tsx @@ -98,6 +98,49 @@ describe('AppNotificationList', () => { }); }); + describe('Event listener cleanup', () => { + let onSpy: jest.SpyInstance; + let offSpy: jest.SpyInstance; + + const eventTypes = [AppEvents.alertWarning, AppEvents.alertSuccess, AppEvents.alertError, AppEvents.alertInfo]; + + beforeEach(() => { + onSpy = jest.spyOn(appEvents, 'on'); + offSpy = jest.spyOn(appEvents, 'off'); + }); + + afterEach(() => { + onSpy.mockRestore(); + offSpy.mockRestore(); + }); + + it('should register event listeners on mount', () => { + renderWithContext(); + + expect(onSpy).toHaveBeenCalledTimes(4); + eventTypes.forEach((eventType) => { + expect(onSpy).toHaveBeenCalledWith(eventType, expect.any(Function)); + }); + }); + + it('should unregister event listeners on unmount', () => { + const { unmount } = renderWithContext(); + + const handlers = eventTypes.map((eventType) => { + const handler = onSpy.mock.calls.find((call) => call[0] === eventType)?.[1]; + expect(handler).toBeDefined(); + return { eventType, handler }; + }); + + unmount(); + + expect(offSpy).toHaveBeenCalledTimes(4); + handlers.forEach(({ eventType, handler }) => { + expect(offSpy).toHaveBeenCalledWith(eventType, handler); + }); + }); + }); + describe('Edge cases', () => { it('should show error on dashboard page with uid and slug', async () => { renderWithContext(undefined, '/d/test-uid/test-slug'); diff --git a/public/app/core/components/AppNotifications/AppNotificationList.tsx b/public/app/core/components/AppNotifications/AppNotificationList.tsx index 29cc9a4b0e3..8d5a1d3161e 100644 --- a/public/app/core/components/AppNotifications/AppNotificationList.tsx +++ b/public/app/core/components/AppNotifications/AppNotificationList.tsx @@ -1,8 +1,8 @@ import { css } from '@emotion/css'; -import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; import { useLocation } from 'react-router-dom'; -import { AlertErrorPayload, AppEvents, GrafanaTheme2 } from '@grafana/data'; +import { AlertErrorPayload, AlertPayload, AppEvents, GrafanaTheme2 } from '@grafana/data'; import { useStyles2, Stack } from '@grafana/ui'; import { notifyApp, hideAppNotification } from 'app/core/actions'; import { appEvents } from 'app/core/app_events'; @@ -26,25 +26,60 @@ export function AppNotificationList() { const { chrome } = useGrafana(); const location = useLocation(); + // Store location ref to avoid re-registering listeners on route changes + const locationRef = useRef(location); + useEffect(() => { + locationRef.current = location; + }, [location]); + useEffect(() => { // Suppress error notifications in kiosk mode on dashboards. // Kiosk mode is typically used for TV displays which are non-interactive. // Backend errors like "Failed to fetch" cannot be dismissed and would remain visible, // degrading the viewing experience. Other notification types (success, warning, info) // are still shown as they indicate successful operations or important information. - const handleErrorAlert = (payload: AlertErrorPayload) => { - const isKioskDashboard = chrome.state.getValue().kioskMode && location.pathname.startsWith('/d/'); - - if (!isKioskDashboard) { - dispatch(notifyApp(createErrorNotification(...payload))); + const handleErrorAlert = (payload?: AlertErrorPayload) => { + const isKioskDashboard = chrome.state.getValue().kioskMode && locationRef.current.pathname.startsWith('/d/'); + if (isKioskDashboard || !payload) { + return; } + dispatch(notifyApp(createErrorNotification(...payload))); }; - appEvents.on(AppEvents.alertWarning, (payload) => dispatch(notifyApp(createWarningNotification(...payload)))); - appEvents.on(AppEvents.alertSuccess, (payload) => dispatch(notifyApp(createSuccessNotification(...payload)))); + const handleWarningAlert = (payload?: AlertPayload) => { + if (!payload) { + return; + } + dispatch(notifyApp(createWarningNotification(...payload))); + }; + + const handleSuccessAlert = (payload?: AlertPayload) => { + if (!payload) { + return; + } + dispatch(notifyApp(createSuccessNotification(...payload))); + }; + + const handleInfoAlert = (payload?: AlertPayload) => { + if (!payload) { + return; + } + dispatch(notifyApp(createInfoNotification(...payload))); + }; + + appEvents.on(AppEvents.alertWarning, handleWarningAlert); + appEvents.on(AppEvents.alertSuccess, handleSuccessAlert); appEvents.on(AppEvents.alertError, handleErrorAlert); - appEvents.on(AppEvents.alertInfo, (payload) => dispatch(notifyApp(createInfoNotification(...payload)))); - }, [dispatch, chrome, location.pathname]); + appEvents.on(AppEvents.alertInfo, handleInfoAlert); + + return () => { + // Unsubscribe from events on unmount to avoid memory leaks + appEvents.off(AppEvents.alertWarning, handleWarningAlert); + appEvents.off(AppEvents.alertSuccess, handleSuccessAlert); + appEvents.off(AppEvents.alertError, handleErrorAlert); + appEvents.off(AppEvents.alertInfo, handleInfoAlert); + }; + }, [dispatch, chrome]); const onClearAppNotification = (id: string) => { dispatch(hideAppNotification(id));