refactor: centralize notification removal logic and re-enable mark as done (#2518)

* refactor: centralize notification removal logic and re-enable mark as done

- Add shouldRemoveNotificationsFromState helper to centralize the logic
  for determining if notifications should be removed or kept in-place
- Re-enable "Mark as done" button for unread notifications regardless
  of fetchReadNotifications setting (it still works, just appears as
  "read" after refresh due to GitHub API limitation)
- Simplify useNotifications hook by using removeNotificationsForAccount
  consistently for both settings
- Update tests to reflect new behavior

* chore: remove auto-fix from lint-staged biome check

* style: fix indentation in remove.test.ts

Author: afonsojramos

package.json

@@ -146,7 +146,7 @@
     ]
   },
   "lint-staged": {
-    "*": "biome check --fix --no-errors-on-unmatched",
+    "*": "biome check --no-errors-on-unmatched",
     "*.{js,ts,tsx}": "pnpm test --findRelatedTests --passWithNoTests --updateSnapshot"
   }
 }

src/renderer/components/notifications/NotificationRow.test.tsx

@@ -136,7 +136,7 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => {
       expect(markNotificationsAsDoneMock).toHaveBeenCalledTimes(1);
     });
 
-    it('should mark as read (not done) when markAsDoneOnOpen is true but fetchReadNotifications is enabled', async () => {
+    it('should mark as done when markAsDoneOnOpen is true even with fetchReadNotifications enabled', async () => {
       const markNotificationsAsReadMock = jest.fn();
       const markNotificationsAsDoneMock = jest.fn();
 
@@ -158,8 +158,8 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => {
       await userEvent.click(screen.getByTestId('notification-row'));
 
       expect(links.openNotification).toHaveBeenCalledTimes(1);
-      expect(markNotificationsAsReadMock).toHaveBeenCalledTimes(1);
-      expect(markNotificationsAsDoneMock).not.toHaveBeenCalled();
+      expect(markNotificationsAsDoneMock).toHaveBeenCalledTimes(1);
+      expect(markNotificationsAsReadMock).not.toHaveBeenCalled();
     });
 
     it('should mark notifications as read', async () => {
@@ -216,7 +216,25 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => {
       expect(markNotificationsAsDoneMock).toHaveBeenCalledTimes(1);
     });
 
-    it('should hide mark as done button when fetchReadNotifications is enabled', async () => {
+    it('should hide mark as done button when notification is already read', async () => {
+      const readNotification = {
+        ...mockGitifyNotification,
+        unread: false,
+      };
+
+      const props = {
+        notification: readNotification,
+        account: mockGitHubCloudAccount,
+      };
+
+      renderWithAppContext(<NotificationRow {...props} />);
+
+      expect(
+        screen.queryByTestId('notification-mark-as-done'),
+      ).not.toBeInTheDocument();
+    });
+
+    it('should show mark as done button when fetchReadNotifications is enabled and notification is unread', async () => {
       const props = {
         notification: mockGitifyNotification,
         account: mockGitHubCloudAccount,
@@ -227,8 +245,8 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => {
       });
 
       expect(
-        screen.queryByTestId('notification-mark-as-done'),
-      ).not.toBeInTheDocument();
+        screen.getByTestId('notification-mark-as-done'),
+      ).toBeInTheDocument();
       expect(
         screen.getByTestId('notification-mark-as-read'),
       ).toBeInTheDocument();

src/renderer/components/notifications/NotificationRow.tsx

@@ -9,6 +9,7 @@ import { cn } from '../../utils/cn';
 import { isMarkAsDoneFeatureSupported } from '../../utils/features';
 import { openNotification } from '../../utils/links';
 import { isGroupByDate } from '../../utils/notifications/group';
+import { shouldRemoveNotificationsFromState } from '../../utils/notifications/remove';
 import { HoverButton } from '../primitives/HoverButton';
 import { HoverGroup } from '../primitives/HoverGroup';
 import { NotificationFooter } from './NotificationFooter';
@@ -31,17 +32,13 @@ export const NotificationRow: FC<NotificationRowProps> = ({
   } = useAppContext();
   const [animateExit, setAnimateExit] = useState(false);
 
+  const shouldAnimateExit = shouldRemoveNotificationsFromState(settings);
+
   const handleNotification = useCallback(() => {
-    // Don't animate exit when fetchReadNotifications is enabled
-    // as notifications will stay in the list with reduced opacity
-    const shouldAnimateExit =
-      !settings.delayNotificationState && !settings.fetchReadNotifications;
     setAnimateExit(shouldAnimateExit);
     openNotification(notification);
 
-    // Fall back to mark as read when fetchReadNotifications is enabled
-    // since marking as done won't work correctly (API limitation)
-    if (settings.markAsDoneOnOpen && !settings.fetchReadNotifications) {
+    if (settings.markAsDoneOnOpen) {
       markNotificationsAsDone([notification]);
     } else {
       markNotificationsAsRead([notification]);
@@ -51,19 +48,16 @@ export const NotificationRow: FC<NotificationRowProps> = ({
     markNotificationsAsRead,
     markNotificationsAsDone,
     settings,
+    shouldAnimateExit,
   ]);
 
   const actionMarkAsDone = () => {
-    setAnimateExit(!settings.delayNotificationState);
+    setAnimateExit(shouldAnimateExit);
     markNotificationsAsDone([notification]);
   };
 
   const actionMarkAsRead = () => {
-    // Don't animate exit when fetchReadNotifications is enabled
-    // as the notification will stay in the list with reduced opacity
-    if (!settings.fetchReadNotifications) {
-      setAnimateExit(!settings.delayNotificationState);
-    }
+    setAnimateExit(shouldAnimateExit);
     markNotificationsAsRead([notification]);
   };
 
@@ -152,7 +146,7 @@ export const NotificationRow: FC<NotificationRowProps> = ({
             action={actionMarkAsDone}
             enabled={
               isMarkAsDoneFeatureSupported(notification.account) &&
-              !settings.fetchReadNotifications
+              notification.unread
             }
             icon={CheckIcon}
             label="Mark as done"

src/renderer/components/notifications/RepositoryNotifications.tsx

@@ -9,6 +9,7 @@ import { cn } from '../../utils/cn';
 import { isMarkAsDoneFeatureSupported } from '../../utils/features';
 import { getChevronDetails } from '../../utils/helpers';
 import { openRepository } from '../../utils/links';
+import { shouldRemoveNotificationsFromState } from '../../utils/notifications/remove';
 import { AvatarWithFallback } from '../avatars/AvatarWithFallback';
 import { HoverButton } from '../primitives/HoverButton';
 import { HoverGroup } from '../primitives/HoverGroup';
@@ -30,18 +31,15 @@ export const RepositoryNotifications: FC<RepositoryNotificationsProps> = ({
     useState(true);
 
   const avatarUrl = repoNotifications[0].repository.owner.avatarUrl;
+  const shouldAnimateExit = shouldRemoveNotificationsFromState(settings);
 
   const actionMarkAsDone = () => {
-    setAnimateExit(!settings.delayNotificationState);
+    setAnimateExit(shouldAnimateExit);
     markNotificationsAsDone(repoNotifications);
   };
 
   const actionMarkAsRead = () => {
-    // Don't animate exit when fetchReadNotifications is enabled
-    // as the notifications will stay in the list with reduced opacity
-    if (!settings.fetchReadNotifications) {
-      setAnimateExit(!settings.delayNotificationState);
-    }
+    setAnimateExit(shouldAnimateExit);
     markNotificationsAsRead(repoNotifications);
   };
 
@@ -107,7 +105,7 @@ export const RepositoryNotifications: FC<RepositoryNotificationsProps> = ({
               action={actionMarkAsDone}
               enabled={
                 isMarkAsDoneFeatureSupported(repoNotifications[0].account) &&
-                !settings.fetchReadNotifications
+                !areAllRepoNotificationsRead
               }
               icon={CheckIcon}
               label="Mark repository as done"