Notifications: Prevent triggering duplicate notifications (#114497)

* fix(notifications): prevent event listener re-registration on route changes

* refactor(notifications): rename alert handling functions for clarity

* refactor(notifications): simplify alert handling by using spread operator for payloads

* refactor(events): address feedback - update LegacyEmitter and LegacyEventHandler interfaces for improved type safety

* fix(events): ensure event handlers handle undefined events gracefully in tests

* test(notifications): add tests for event listener registration and cleanup in AppNotificationList

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) => {

packages/grafana-data/src/events/types.ts

@@ -133,12 +133,12 @@ export interface LegacyEmitter {
   /**
    * @deprecated use $on
    */
-  off<T>(event: AppEvent<T> | string, handler: (payload?: T) => void): void;
+  off<T>(event: AppEvent<T> | string, handler: LegacyEventHandler<T>): void;
 }
 
 /** @public */
 export interface LegacyEventHandler<T> {
-  (payload: T): void;
+  (payload?: T): void;
   wrapper?: (event: BusEvent) => void;
 }
 

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');

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));