refactor

Signed-off-by: Adam Setch <[email protected]>

Author: setchy

src/renderer/hooks/useNotifications.ts

@@ -22,7 +22,7 @@ import { isMarkAsDoneFeatureSupported } from '../utils/features';
 import { rendererLogError } from '../utils/logger';
 import { raiseNativeNotification } from '../utils/notifications/native';
 import { getAllNotifications } from '../utils/notifications/notifications';
-import { removeNotifications } from '../utils/notifications/remove';
+import { removeNotificationsForAccount } from '../utils/notifications/remove';
 import { raiseSoundNotification } from '../utils/notifications/sound';
 import { getNewNotifications } from '../utils/notifications/utils';
 
@@ -129,7 +129,8 @@ export const useNotifications = (): NotificationsState => {
           ),
         );
 
-        const updatedNotifications = removeNotifications(
+        const updatedNotifications = removeNotificationsForAccount(
+          readNotifications[0].account,
           state.settings,
           readNotifications,
           notifications,
@@ -168,7 +169,8 @@ export const useNotifications = (): NotificationsState => {
           ),
         );
 
-        const updatedNotifications = removeNotifications(
+        const updatedNotifications = removeNotificationsForAccount(
+          doneNotifications[0].account,
           state.settings,
           doneNotifications,
           notifications,

src/renderer/utils/notifications/remove.test.ts

@@ -1,13 +1,14 @@
 import { mockSingleAccountNotifications } from '../../__mocks__/notifications-mocks';
 import { mockSettings } from '../../__mocks__/state-mocks';
 import { mockSingleNotification } from '../api/__mocks__/response-mocks';
-import { removeNotifications } from './remove';
+import { removeNotificationsForAccount } from './remove';
 
 describe('renderer/utils/remove.ts', () => {
   it('should remove a notification if it exists', () => {
     expect(mockSingleAccountNotifications[0].notifications.length).toBe(1);
 
-    const result = removeNotifications(
+    const result = removeNotificationsForAccount(
+      mockSingleAccountNotifications[0].account,
       { ...mockSettings, delayNotificationState: false },
       [mockSingleNotification],
       mockSingleAccountNotifications,
@@ -16,10 +17,11 @@ describe('renderer/utils/remove.ts', () => {
     expect(result[0].notifications.length).toBe(0);
   });
 
-  it('should skip notification removal if delay state enabled', () => {
+  it('should mark as read and skip notification removal if delay state enabled', () => {
     expect(mockSingleAccountNotifications[0].notifications.length).toBe(1);
 
-    const result = removeNotifications(
+    const result = removeNotificationsForAccount(
+      mockSingleAccountNotifications[0].account,
       { ...mockSettings, delayNotificationState: true },
       [mockSingleNotification],
       mockSingleAccountNotifications,
@@ -29,10 +31,30 @@ describe('renderer/utils/remove.ts', () => {
     expect(result[0].notifications[0].unread).toBe(false);
   });
 
+  it('should skip notification removal if delay state enabled and nothing to remove', () => {
+    expect(mockSingleAccountNotifications[0].notifications.length).toBe(1);
+
+    const result = removeNotificationsForAccount(
+      mockSingleAccountNotifications[0].account,
+      { ...mockSettings, delayNotificationState: true },
+      [
+        {
+          ...mockSingleNotification,
+          id: 'non-existent-id',
+        },
+      ],
+      mockSingleAccountNotifications,
+    );
+
+    expect(result[0].notifications.length).toBe(1);
+    expect(result[0].notifications[0].unread).toBe(true);
+  });
+
   it('should skip notification removal if nothing to remove', () => {
     expect(mockSingleAccountNotifications[0].notifications.length).toBe(1);
 
-    const result = removeNotifications(
+    const result = removeNotificationsForAccount(
+      mockSingleAccountNotifications[0].account,
       { ...mockSettings, delayNotificationState: false },
       [],
       mockSingleAccountNotifications,

src/renderer/utils/notifications/remove.ts

@@ -1,8 +1,14 @@
-import type { AccountNotifications, SettingsState } from '../../types';
+import type { Account, AccountNotifications, SettingsState } from '../../types';
 import type { Notification } from '../../typesGitHub';
-import { findAccountIndex } from './utils';
+import { getAccountUUID } from '../auth/utils';
 
-export function removeNotifications(
+/**
+ * Remove notifications from the account notifications list.
+ *
+ * If delayNotificationState is enabled in settings, mark notifications as read instead of removing them.
+ */
+export function removeNotificationsForAccount(
+  account: Account,
   settings: SettingsState,
   notificationsToRemove: Notification[],
   allNotifications: AccountNotifications[],
@@ -11,37 +17,24 @@ export function removeNotifications(
     return allNotifications;
   }
 
-  const removeIds = new Set(notificationsToRemove.map((n) => n.id));
-
-  // If delay notifications is enabled, mark notifications as read but do not remove them
-  if (settings.delayNotificationState) {
-    return allNotifications.map((accountNotifications) => ({
-      ...accountNotifications,
-      notifications: accountNotifications.notifications.map((notification) =>
-        removeIds.has(notification.id)
-          ? { ...notification, unread: false }
-          : notification,
-      ),
-    }));
-  }
-
-  const accountIndex = findAccountIndex(
-    allNotifications,
-    notificationsToRemove[0],
+  const notificationIDsToRemove = new Set(
+    notificationsToRemove.map((n) => n.id),
   );
 
-  if (accountIndex === -1) {
-    return allNotifications;
-  }
-
-  return allNotifications.map((account, index) =>
-    index === accountIndex
+  return allNotifications.map((accountNotifications) =>
+    getAccountUUID(account) === getAccountUUID(accountNotifications.account)
       ? {
-          ...account,
-          notifications: account.notifications.filter(
-            (notification) => !removeIds.has(notification.id),
-          ),
+          ...accountNotifications,
+          notifications: settings.delayNotificationState
+            ? accountNotifications.notifications.map((notification) =>
+                notificationIDsToRemove.has(notification.id)
+                  ? { ...notification, unread: false }
+                  : notification,
+              )
+            : accountNotifications.notifications.filter(
+                (notification) => !notificationIDsToRemove.has(notification.id),
+              ),
         }
-      : account,
+      : accountNotifications,
   );
 }

src/renderer/utils/notifications/utils.ts

@@ -2,24 +2,6 @@ import type { AccountNotifications } from '../../types';
 import type { Notification } from '../../typesGitHub';
 import { getAccountUUID } from '../auth/utils';
 
-/**
- * Find the account index for a given notification
- *
- * @param allNotifications - The list of all account notifications
- * @param notification - The notification to find the account index for
- * @returns The index of the account in the allNotifications array
- */
-export function findAccountIndex(
-  allNotifications: AccountNotifications[],
-  notification: Notification,
-): number {
-  return allNotifications.findIndex(
-    (accountNotifications) =>
-      getAccountUUID(accountNotifications.account) ===
-      getAccountUUID(notification.account),
-  );
-}
-
 /**
  * Find notifications that exist in newNotifications but not in previousNotifications
  */