feat: stabilize notification order during interactions (#2298)

* feat: stabilize notification order during notification interactions

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

* feat: stabilize notification order during notification interactions

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

* feat: stabilize notification order during notification interactions

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

* feat: stabilize notification order during notification interactions

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

* feat: stabilize notification order during notification interactions

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

---------

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

Author: setchy

src/renderer/components/notifications/AccountNotifications.tsx

@@ -13,6 +13,10 @@ import {
   openGitHubIssues,
   openGitHubPulls,
 } from '../../utils/links';
+import {
+  groupNotificationsByRepository,
+  isGroupByRepository,
+} from '../../utils/notifications/group';
 import { AllRead } from '../AllRead';
 import { AvatarWithFallback } from '../avatars/AvatarWithFallback';
 import { Oops } from '../Oops';
@@ -31,28 +35,26 @@ interface IAccountNotifications {
 export const AccountNotifications: FC<IAccountNotifications> = (
   props: IAccountNotifications,
 ) => {
-  const { account, showAccountHeader, notifications } = props;
+  const { account, showAccountHeader, error, notifications } = props;
 
   const { settings } = useContext(AppContext);
 
   const [showAccountNotifications, setShowAccountNotifications] =
     useState(true);
 
-  const groupedNotifications = Object.values(
-    notifications.reduce(
-      (acc: { [key: string]: Notification[] }, notification) => {
-        const key = notification.repository.full_name;
-        if (!acc[key]) {
-          acc[key] = [];
-        }
-
-        acc[key].push(notification);
-        return acc;
-      },
-      {},
-    ),
+  const sortedNotifications = useMemo(
+    () => [...notifications].sort((a, b) => a.order - b.order),
+    [notifications],
   );
 
+  const groupedNotifications = useMemo(() => {
+    const map = groupNotificationsByRepository([
+      { account, error, notifications: sortedNotifications },
+    ]);
+
+    return Array.from(map.values());
+  }, [account, error, sortedNotifications]);
+
   const hasNotifications = useMemo(
     () => notifications.length > 0,
     [notifications],
@@ -68,8 +70,6 @@ export const AccountNotifications: FC<IAccountNotifications> = (
     'account',
   );
 
-  const isGroupByRepository = settings.groupBy === 'REPOSITORY';
-
   return (
     <>
       {showAccountHeader && (
@@ -130,7 +130,7 @@ export const AccountNotifications: FC<IAccountNotifications> = (
         <>
           {props.error && <Oops error={props.error} fullHeight={false} />}
           {!hasNotifications && !props.error && <AllRead fullHeight={false} />}
-          {isGroupByRepository
+          {isGroupByRepository(settings)
             ? Object.values(groupedNotifications).map((repoNotifications) => {
                 const repoSlug = repoNotifications[0].repository.full_name;
 
@@ -142,7 +142,7 @@ export const AccountNotifications: FC<IAccountNotifications> = (
                   />
                 );
               })
-            : notifications.map((notification) => (
+            : sortedNotifications.map((notification) => (
                 <NotificationRow
                   key={notification.id}
                   notification={notification}

src/renderer/routes/Notifications.tsx

@@ -39,17 +39,19 @@ export const NotificationsRoute: FC = () => {
   return (
     <Page testId="notifications">
       <Contents paddingHorizontal={false}>
-        {notifications.map((accountNotifications) => (
-          <AccountNotifications
-            account={accountNotifications.account}
-            error={accountNotifications.error}
-            key={getAccountUUID(accountNotifications.account)}
-            notifications={accountNotifications.notifications}
-            showAccountHeader={
-              hasMultipleAccounts || settings.showAccountHeader
-            }
-          />
-        ))}
+        {notifications.map((accountNotification) => {
+          return (
+            <AccountNotifications
+              account={accountNotification.account}
+              error={accountNotification.error}
+              key={getAccountUUID(accountNotification.account)}
+              notifications={accountNotification.notifications}
+              showAccountHeader={
+                hasMultipleAccounts || settings.showAccountHeader
+              }
+            />
+          );
+        })}
       </Contents>
     </Page>
   );

src/renderer/typesGitHub.ts

@@ -112,6 +112,7 @@ export interface GitHubNotification {
 // Note: This is not in the official GitHub API. We add this to make notification interactions easier.
 export interface GitifyNotification {
   account: Account;
+  order: number;
 }
 
 export type UserDetails = User & UserProfile;

src/renderer/utils/api/__mocks__/response-mocks.ts

@@ -45,6 +45,7 @@ export const mockNotificationUser: User = {
 export const mockGitHubNotifications: Notification[] = [
   {
     account: mockGitHubCloudAccount,
+    order: 0,
     id: '138661096',
     unread: true,
     reason: 'subscribed',
@@ -197,6 +198,7 @@ export const mockGitHubNotifications: Notification[] = [
   },
   {
     account: mockGitHubCloudAccount,
+    order: 1,
     id: '148827438',
     unread: true,
     reason: 'author',
@@ -260,6 +262,7 @@ export const mockGitHubNotifications: Notification[] = [
 export const mockEnterpriseNotifications: Notification[] = [
   {
     account: mockGitHubEnterpriseServerAccount,
+    order: 0,
     id: '3',
     unread: true,
     reason: 'subscribed',
@@ -316,6 +319,7 @@ export const mockEnterpriseNotifications: Notification[] = [
   },
   {
     account: mockGitHubEnterpriseServerAccount,
+    order: 1,
     id: '4',
     unread: true,
     reason: 'subscribed',

src/renderer/utils/notifications/group.ts

@@ -0,0 +1,50 @@
+import type { AccountNotifications, SettingsState } from '../../types';
+import type { Notification } from '../../typesGitHub';
+
+/**
+ * Returns true when settings say to group by repository.
+ */
+export function isGroupByRepository(settings: SettingsState) {
+  return settings.groupBy === 'REPOSITORY';
+}
+
+/**
+ * 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.
+ */
+export function groupNotificationsByRepository(
+  accounts: AccountNotifications[],
+): Map<string, Notification[]> {
+  const repoGroups = new Map<string, Notification[]>();
+
+  for (const account of accounts) {
+    for (const notification of account.notifications) {
+      const repo = notification.repository?.full_name ?? '';
+      const group = repoGroups.get(repo);
+
+      if (group) {
+        group.push(notification);
+      } else {
+        repoGroups.set(repo, [notification]);
+      }
+    }
+  }
+
+  return repoGroups;
+}
+
+/**
+ * Returns a flattened, ordered Notification[] according to repository-first-seen order
+ * (when grouped) or the natural account->notification order otherwise.
+ */
+export function getFlattenedNotificationsByRepo(
+  accounts: AccountNotifications[],
+  settings: SettingsState,
+): Notification[] {
+  if (isGroupByRepository(settings)) {
+    const repoGroups = groupNotificationsByRepository(accounts);
+    return Array.from(repoGroups.values()).flat();
+  }
+
+  return accounts.flatMap((a) => a.notifications);
+}

src/renderer/utils/notifications/notifications.ts

@@ -12,6 +12,7 @@ import {
   filterBaseNotifications,
   filterDetailedNotifications,
 } from './filters/filter';
+import { getFlattenedNotificationsByRepo } from './group';
 import { createNotificationHandler } from './handlers';
 
 export function setTrayIconColor(notifications: AccountNotifications[]) {
@@ -90,6 +91,9 @@ export async function getAllNotifications(
       }),
   );
 
+  // Set the order property for the notifications
+  stabilizeNotificationsOrder(notifications, state.settings);
+
   return notifications;
 }
 
@@ -141,3 +145,26 @@ export async function enrichNotification(
     },
   };
 }
+
+/**
+ * Assign an order property to each notification to stabilize how they are displayed
+ * during notification interaction events (mark as read, mark as done, etc.)
+ *
+ * @param notifications
+ * @param settings
+ */
+export function stabilizeNotificationsOrder(
+  notifications: AccountNotifications[],
+  settings: SettingsState,
+) {
+  const flattenedNotifications = getFlattenedNotificationsByRepo(
+    notifications,
+    settings,
+  );
+
+  let orderIndex = 0;
+
+  for (const n of flattenedNotifications) {
+    n.order = orderIndex++;
+  }
+}