fix: orphaned notification groupings (#2337)

Signed-off-by: Adam Setch <adam.setch@outlook.com>

Author: setchy

src/renderer/components/notifications/AccountNotifications.tsx

@@ -52,7 +52,7 @@ export const AccountNotifications: FC<IAccountNotifications> = (
       { account, error, notifications: sortedNotifications },
     ]);
 
-    return Array.from(map.values());
+    return Array.from(map.entries());
   }, [account, error, sortedNotifications]);
 
   const hasNotifications = useMemo(
@@ -131,17 +131,13 @@ export const AccountNotifications: FC<IAccountNotifications> = (
           {props.error && <Oops error={props.error} fullHeight={false} />}
           {!hasNotifications && !props.error && <AllRead fullHeight={false} />}
           {isGroupByRepository(settings)
-            ? groupedNotifications.map((repoNotifications) => {
-                const repoSlug = repoNotifications[0].repository.full_name;
-
-                return (
-                  <RepositoryNotifications
-                    key={repoSlug}
-                    repoName={repoSlug}
-                    repoNotifications={repoNotifications}
-                  />
-                );
-              })
+            ? groupedNotifications.map(([repoSlug, repoNotifications]) => (
+                <RepositoryNotifications
+                  key={repoSlug}
+                  repoName={repoSlug}
+                  repoNotifications={repoNotifications}
+                />
+              ))
             : sortedNotifications.map((notification) => (
                 <NotificationRow
                   key={notification.id}

src/renderer/hooks/useNotifications.test.ts

@@ -370,7 +370,6 @@ describe('renderer/hooks/useNotifications.ts', () => {
       });
 
       expect(result.current.notifications.length).toBe(0);
-      expect(mockSingleNotification.unread).toBeFalsy();
     });
 
     it('should mark notifications as read with failure', async () => {
@@ -414,7 +413,6 @@ describe('renderer/hooks/useNotifications.ts', () => {
       });
 
       expect(result.current.notifications.length).toBe(0);
-      expect(mockSingleNotification.unread).toBeFalsy();
     });
 
     it('should mark notifications as done with failure', async () => {

src/renderer/hooks/useNotifications.ts

@@ -24,13 +24,25 @@ import {
 import { removeNotifications } from '../utils/notifications/remove';
 
 /**
- * Apply optimistic local updates for read state.  This helps with some
- * rendering edge cases between fetch notification intervals.
+ * Apply read state updates to all notifications, replacing the target notifications
+ * with new objects that have unread: false. This is used for optimistic UI updates.
  */
-function markNotificationsAsReadLocally(targetNotifications: Notification[]) {
-  for (const n of targetNotifications) {
-    n.unread = false;
-  }
+function applyReadStateToNotifications(
+  allNotifications: AccountNotifications[],
+  readNotifications: Notification[],
+): AccountNotifications[] {
+  const readNotificationIDs = new Set<string>(
+    readNotifications.map((notification) => notification.id),
+  );
+
+  return allNotifications.map((accountNotifications) => ({
+    ...accountNotifications,
+    notifications: accountNotifications.notifications.map((notification) =>
+      readNotificationIDs.has(notification.id)
+        ? { ...notification, unread: false }
+        : notification,
+    ),
+  }));
 }
 
 interface NotificationsState {
@@ -129,13 +141,16 @@ export const useNotifications = (): NotificationsState => {
           ),
         );
 
-        const updatedNotifications = removeNotifications(
+        let updatedNotifications = removeNotifications(
           state.settings,
           readNotifications,
           notifications,
         );
 
-        markNotificationsAsReadLocally(readNotifications);
+        updatedNotifications = applyReadStateToNotifications(
+          updatedNotifications,
+          readNotifications,
+        );
 
         setNotifications(updatedNotifications);
         setTrayIconColor(updatedNotifications);
@@ -171,13 +186,16 @@ export const useNotifications = (): NotificationsState => {
           ),
         );
 
-        const updatedNotifications = removeNotifications(
+        let updatedNotifications = removeNotifications(
           state.settings,
           doneNotifications,
           notifications,
         );
 
-        markNotificationsAsReadLocally(doneNotifications);
+        updatedNotifications = applyReadStateToNotifications(
+          updatedNotifications,
+          doneNotifications,
+        );
 
         setNotifications(updatedNotifications);
         setTrayIconColor(updatedNotifications);

src/renderer/utils/notifications/group.ts

@@ -11,6 +11,7 @@ export function isGroupByRepository(settings: SettingsState) {
 /**
  * Group notifications by repository.full_name preserving first-seen repo order.
  * Returns a Map where keys are repo full_names and values are arrays of notifications.
+ * Skips notifications without valid repository data.
  */
 export function groupNotificationsByRepository(
   accounts: AccountNotifications[],
@@ -19,7 +20,13 @@ export function groupNotificationsByRepository(
 
   for (const account of accounts) {
     for (const notification of account.notifications) {
-      const repo = notification.repository?.full_name ?? '';
+      const repo = notification.repository?.full_name;
+
+      // Skip notifications without valid repository data
+      if (!repo) {
+        continue;
+      }
+
       const group = repoGroups.get(repo);
 
       if (group) {