Fix Notification components possible memory leaks (#650)

Dispose subscriptions and timeouts when
the components are removed from the DOM

web/src/lib/components/shared-components/notification/__tests__/notification-card.spec.ts

@@ -0,0 +1,39 @@
+import { jest, describe, it } from '@jest/globals';
+import { render, cleanup, RenderResult } from '@testing-library/svelte';
+import { NotificationType } from '../notification';
+import NotificationCard from '../notification-card.svelte';
+import '@testing-library/jest-dom';
+
+describe('NotificationCard component', () => {
+	let sut: RenderResult<NotificationCard>;
+
+	it('disposes timeout if already removed from the DOM', () => {
+		jest.spyOn(window, 'clearTimeout');
+
+		sut = render(NotificationCard, {
+			notificationInfo: {
+				id: 1234,
+				message: 'Notification message',
+				timeout: 1000,
+				type: NotificationType.Info
+			}
+		});
+
+		cleanup();
+		expect(window.clearTimeout).toHaveBeenCalledTimes(1);
+	});
+
+	it('shows message and title', () => {
+		sut = render(NotificationCard, {
+			notificationInfo: {
+				id: 1234,
+				message: 'Notification message',
+				timeout: 1000,
+				type: NotificationType.Info
+			}
+		});
+
+		expect(sut.getByTestId('title')).toHaveTextContent('Info');
+		expect(sut.getByTestId('message')).toHaveTextContent('Notification message');
+	});
+});

web/src/lib/components/shared-components/notification/__tests__/notification-list.spec.ts

@@ -0,0 +1,44 @@
+import { jest, describe, it } from '@jest/globals';
+import { render, RenderResult, waitFor } from '@testing-library/svelte';
+import { notificationController, NotificationType } from '../notification';
+import { get } from 'svelte/store';
+import NotificationList from '../notification-list.svelte';
+import '@testing-library/jest-dom';
+
+function _getNotificationListElement(
+	sut: RenderResult<NotificationList>
+): HTMLAnchorElement | null {
+	return sut.container.querySelector('#notification-list');
+}
+
+describe('NotificationList component', () => {
+	const sut: RenderResult<NotificationList> = render(NotificationList);
+
+	beforeAll(() => {
+		jest.useFakeTimers();
+	});
+
+	afterAll(() => {
+		jest.useRealTimers();
+	});
+
+	it('shows a notification when added and closes it automatically after the delay timeout', async () => {
+		expect(_getNotificationListElement(sut)).not.toBeInTheDocument();
+
+		notificationController.show({
+			message: 'Notification',
+			type: NotificationType.Info,
+			timeout: 3000
+		});
+
+		await waitFor(() => expect(_getNotificationListElement(sut)).toBeInTheDocument());
+
+		expect(_getNotificationListElement(sut)?.children).toHaveLength(1);
+
+		jest.advanceTimersByTime(3000);
+		// due to some weirdness in svelte (or testing-library) need to check if it has been removed from the store to make sure it works.
+		expect(get(notificationController.notificationList)).toHaveLength(0);
+
+		await waitFor(() => expect(_getNotificationListElement(sut)).not.toBeInTheDocument());
+	});
+});

web/src/lib/components/shared-components/notification/notification-card.svelte

@@ -48,10 +48,14 @@
 		}
 	};
 
+	let removeNotificationTimeout: NodeJS.Timeout | undefined = undefined;
+
 	onMount(() => {
-		setTimeout(() => {
+		removeNotificationTimeout = setTimeout(() => {
 			notificationController.removeNotificationById(notificationInfo.id);
 		}, notificationInfo.timeout);
+
+		return () => clearTimeout(removeNotificationTimeout);
 	});
 </script>
 
@@ -63,8 +67,10 @@
 >
 	<div class="flex gap-2 place-items-center">
 		<svelte:component this={icon} color={primaryColor()} size="20" />
-		<h2 style:color={primaryColor()} class="font-medium">{notificationInfo.type.toString()}</h2>
+		<h2 style:color={primaryColor()} class="font-medium" data-testid="title">
+			{notificationInfo.type.toString()}
+		</h2>
 	</div>
 
-	<p class="text-sm pl-[28px] pr-[16px]">{notificationInfo.message}</p>
+	<p class="text-sm pl-[28px] pr-[16px]" data-testid="message">{notificationInfo.message}</p>
 </div>

web/src/lib/components/shared-components/notification/notification-list.svelte

@@ -1,25 +1,21 @@
 <script lang="ts">
-	import { ImmichNotification, notificationController } from './notification';
+	import { notificationController } from './notification';
 	import { fade } from 'svelte/transition';
 
 	import NotificationCard from './notification-card.svelte';
 	import { flip } from 'svelte/animate';
 	import { quintOut } from 'svelte/easing';
 
-	let notificationList: ImmichNotification[] = [];
+	const { notificationList } = notificationController;
-
-	notificationController.notificationList.subscribe((list) => {
-		notificationList = list;
-	});
 </script>
 
-{#if notificationList.length > 0}
+{#if $notificationList.length > 0}
 	<section
 		transition:fade={{ duration: 250 }}
 		id="notification-list"
 		class="absolute right-5 top-[80px] z-[99999999]"
 	>
-		{#each notificationList as notificationInfo (notificationInfo.id)}
+		{#each $notificationList as notificationInfo (notificationInfo.id)}
 			<div animate:flip={{ duration: 250, easing: quintOut }}>
 				<NotificationCard {notificationInfo} />
 			</div>