fix: rollback failed notification interaction with visual warning (#3145)

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

Author: setchy

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

@@ -2,14 +2,18 @@ import { screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 
 import { renderWithProviders } from '../../__helpers__/test-utils';
+import { mockGitHubEnterpriseServerAccount } from '../../__mocks__/account-mocks';
 import {
   mockGiteaGitifyNotification,
   mockGitifyNotification,
 } from '../../__mocks__/notifications-mocks';
 import { mockSettings } from '../../__mocks__/state-mocks';
 
+import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores';
+
 import { GroupBy } from '../../types';
 
+import { Errors } from '../../utils/core/errors';
 import * as comms from '../../utils/system/comms';
 import * as links from '../../utils/system/links';
 import { NotificationRow, type NotificationRowProps } from './NotificationRow';
@@ -263,4 +267,183 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => {
       expect(screen.queryByTestId('notification-unsubscribe-from-thread')).not.toBeInTheDocument();
     });
   });
+
+  describe('failure recovery', () => {
+    const failureKey = getNotificationFailureKey(
+      mockGitifyNotification.account,
+      mockGitifyNotification.id,
+    );
+
+    afterEach(() => {
+      useNotificationActionFailuresStore.getState().reset();
+    });
+
+    it('shows hover actions in their normal (non-danger) state when there is no recorded failure', () => {
+      const props: NotificationRowProps = {
+        notification: mockGitifyNotification,
+        isRepositoryAnimatingExit: false,
+      };
+
+      renderWithProviders(<NotificationRow {...props} />);
+
+      expect(screen.getByTestId('notification-mark-as-read')).toHaveAttribute(
+        'title',
+        'Mark as read',
+      );
+    });
+
+    it('styles and explains only the action that failed', () => {
+      const props: NotificationRowProps = {
+        notification: mockGitifyNotification,
+        isRepositoryAnimatingExit: false,
+      };
+
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsRead',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+      renderWithProviders(<NotificationRow {...props} />);
+
+      const markAsReadButton = screen.getByTestId('notification-mark-as-read');
+
+      // The row's actions remain available - the row is not in a broken state
+      expect(markAsReadButton).toBeInTheDocument();
+      expect(markAsReadButton).toHaveAttribute(
+        'title',
+        expect.stringContaining(Errors.ACTION_FORBIDDEN.title),
+      );
+      expect(markAsReadButton).toHaveAttribute(
+        'title',
+        expect.stringContaining('You can also try opening this notification in the browser.'),
+      );
+      expect(screen.getByTestId('notification-mark-as-done')).toHaveAttribute(
+        'title',
+        'Mark as done',
+      );
+      expect(screen.getByTestId('notification-unsubscribe-from-thread')).toHaveAttribute(
+        'title',
+        'Unsubscribe from thread',
+      );
+    });
+
+    it('isolates failures for notifications with the same id across accounts', () => {
+      const sameIdOtherAccount = {
+        ...mockGitifyNotification,
+        account: mockGitHubEnterpriseServerAccount,
+      };
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsRead',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+
+      renderWithProviders(
+        <>
+          <NotificationRow
+            isRepositoryAnimatingExit={false}
+            notification={mockGitifyNotification}
+          />
+          <NotificationRow isRepositoryAnimatingExit={false} notification={sameIdOtherAccount} />
+        </>,
+      );
+
+      const [failedButton, unaffectedButton] = screen.getAllByTestId('notification-mark-as-read');
+      expect(failedButton).toHaveAttribute(
+        'title',
+        expect.stringContaining(Errors.ACTION_FORBIDDEN.title),
+      );
+      expect(unaffectedButton).toHaveAttribute('title', 'Mark as read');
+    });
+
+    it('re-invokes the same action on click, acting as a retry, when a failure is recorded', async () => {
+      const markNotificationsAsDoneMock = vi.fn();
+
+      const props: NotificationRowProps = {
+        notification: mockGitifyNotification,
+        isRepositoryAnimatingExit: false,
+      };
+
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsDone',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+      renderWithProviders(<NotificationRow {...props} />, {
+        markNotificationsAsDone: markNotificationsAsDoneMock,
+      });
+
+      await userEvent.click(screen.getByTestId('notification-mark-as-done'));
+
+      expect(markNotificationsAsDoneMock).toHaveBeenCalledTimes(1);
+      expect(markNotificationsAsDoneMock).toHaveBeenCalledWith([mockGitifyNotification]);
+    });
+
+    it('does not disable retrying even for a permanently-failing classification like ACTION_FORBIDDEN', async () => {
+      const markNotificationsAsReadMock = vi.fn();
+
+      const props: NotificationRowProps = {
+        notification: mockGitifyNotification,
+        isRepositoryAnimatingExit: false,
+      };
+
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsRead',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+      renderWithProviders(<NotificationRow {...props} />, {
+        markNotificationsAsRead: markNotificationsAsReadMock,
+      });
+
+      const markAsReadButton = screen.getByTestId('notification-mark-as-read');
+      expect(markAsReadButton).toBeEnabled();
+
+      await userEvent.click(markAsReadButton);
+
+      expect(markNotificationsAsReadMock).toHaveBeenCalledTimes(1);
+    });
+
+    it('gives a retry its own exit-animation cycle even though the previous failure is still recorded', async () => {
+      // Regression test: the revert logic reads the *current* failure store
+      // state after each action settles, rather than an effect keyed off a
+      // (possibly stale, still-present-from-the-previous-attempt) failure
+      // map - so a retry always gets to animate out and, if it fails again,
+      // animate back in, instead of being short-circuited immediately.
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsRead',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+
+      let resolveRetry: () => void = () => {};
+      const markNotificationsAsReadMock = vi.fn().mockImplementation(
+        () =>
+          new Promise<void>((resolve) => {
+            resolveRetry = resolve;
+          }),
+      );
+
+      const props: NotificationRowProps = {
+        notification: mockGitifyNotification,
+        isRepositoryAnimatingExit: false,
+      };
+
+      renderWithProviders(<NotificationRow {...props} />, {
+        settings: { ...mockSettings, delayNotificationState: false, fetchReadNotifications: false },
+        markNotificationsAsRead: markNotificationsAsReadMock,
+      });
+
+      await userEvent.click(screen.getByTestId('notification-mark-as-read'));
+
+      // While the retry is still in flight, the row is animating out again -
+      // its hover actions are hidden, exactly like the very first attempt.
+      expect(screen.queryByTestId('notification-mark-as-read')).not.toBeInTheDocument();
+
+      // The retry fails again; the store still has a (new) failure entry for
+      // this notification once the mutation resolves.
+      useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+        action: 'markAsRead',
+        error: Errors.ACTION_FORBIDDEN,
+      });
+      resolveRetry();
+
+      await screen.findByTestId('notification-mark-as-read');
+    });
+  });
 });

src/renderer/components/notifications/NotificationRow.tsx

@@ -4,7 +4,11 @@ import { BellSlashIcon, CheckIcon, ReadIcon } from '@primer/octicons-react';
 import { Stack, Text, Tooltip } from '@primer/react';
 
 import { useNotifications } from '../../hooks/useNotifications';
-import { useSettingsStore } from '../../stores';
+import {
+  getNotificationFailureKey,
+  useNotificationActionFailuresStore,
+  useSettingsStore,
+} from '../../stores';
 
 import { HoverButton } from '../primitives/HoverButton';
 import { HoverGroup } from '../primitives/HoverGroup';
@@ -43,31 +47,50 @@ export const NotificationRow: FC<NotificationRowProps> = ({
 
   const shouldAnimateExit = shouldRemoveNotificationsFromState();
 
-  const actionNotificationInteraction = () => {
+  const notificationFailureKey = getNotificationFailureKey(notification.account, notification.id);
+  const failure = useNotificationActionFailuresStore(
+    (state) => state.failures[notificationFailureKey],
+  );
+
+  // Explains the failed action and suggests the browser as a fallback,
+  // rather than a dedicated retry control - clicking the (now red) hover
+  // action again re-attempts it. Phrased as "You can also..." rather than
+  // "...instead", since some descriptions already suggest waiting/retrying
+  // (e.g. `RATE_LIMITED`), which "instead" would read as contradicting.
+  const failureTooltip = failure
+    ? `${failure.error.title}: ${failure.error.descriptions.join(' ')} You can also try opening this notification in the browser.`
+    : undefined;
+
+  // Starts the exit animation immediately, then reverts it if this specific
+  // action failed, checked directly against the failure store once it
+  // settles. Checking a stale value (e.g. via an effect watching the failure
+  // map) would wrongly revert a retry's animation using the previous
+  // attempt's still-present entry.
+  const runAction = async (action: () => Promise<void>) => {
     setShouldAnimateNotificationExit(shouldAnimateExit);
-    openNotification(notification);
 
-    if (markAsDoneOnOpen) {
-      markNotificationsAsDone([notification]);
-    } else {
-      markNotificationsAsRead([notification]);
+    await action();
+
+    if (useNotificationActionFailuresStore.getState().failures[notificationFailureKey]) {
+      setShouldAnimateNotificationExit(false);
     }
   };
 
-  const actionMarkAsDone = () => {
-    setShouldAnimateNotificationExit(shouldAnimateExit);
-    markNotificationsAsDone([notification]);
-  };
+  const actionNotificationInteraction = () => {
+    openNotification(notification);
 
-  const actionMarkAsRead = () => {
-    setShouldAnimateNotificationExit(shouldAnimateExit);
-    markNotificationsAsRead([notification]);
+    runAction(() =>
+      markAsDoneOnOpen
+        ? markNotificationsAsDone([notification])
+        : markNotificationsAsRead([notification]),
+    );
   };
 
-  const actionUnsubscribeFromThread = () => {
-    setShouldAnimateNotificationExit(shouldAnimateExit);
-    unsubscribeNotification(notification);
-  };
+  const actionMarkAsDone = () => runAction(() => markNotificationsAsDone([notification]));
+
+  const actionMarkAsRead = () => runAction(() => markNotificationsAsRead([notification]));
+
+  const actionUnsubscribeFromThread = () => runAction(() => unsubscribeNotification(notification));
 
   const NotificationIcon = notification.display.icon.type;
   const isNotificationRead = !notification.unread;
@@ -143,24 +166,35 @@ export const NotificationRow: FC<NotificationRowProps> = ({
             action={actionMarkAsRead}
             enabled={!isNotificationRead}
             icon={ReadIcon}
-            label="Mark as read"
+            label={
+              failure?.action === 'markAsRead' ? (failureTooltip ?? 'Mark as read') : 'Mark as read'
+            }
             testid="notification-mark-as-read"
+            variant={failure?.action === 'markAsRead' ? 'danger' : 'invisible'}
           />
 
           <HoverButton
             action={actionMarkAsDone}
             enabled={isMarkAsDoneFeatureSupported(notification.account) && notification.unread}
             icon={CheckIcon}
-            label="Mark as done"
+            label={
+              failure?.action === 'markAsDone' ? (failureTooltip ?? 'Mark as done') : 'Mark as done'
+            }
             testid="notification-mark-as-done"
+            variant={failure?.action === 'markAsDone' ? 'danger' : 'invisible'}
           />
 
           <HoverButton
             action={actionUnsubscribeFromThread}
             enabled={isUnsubscribeThreadSupported(notification.account)}
             icon={BellSlashIcon}
-            label="Unsubscribe from thread"
+            label={
+              failure?.action === 'unsubscribe'
+                ? (failureTooltip ?? 'Unsubscribe from thread')
+                : 'Unsubscribe from thread'
+            }
             testid="notification-unsubscribe-from-thread"
+            variant={failure?.action === 'unsubscribe' ? 'danger' : 'invisible'}
           />
         </HoverGroup>
       )}

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

@@ -5,6 +5,8 @@ import { renderWithProviders } from '../../__helpers__/test-utils';
 import { mockGitHubCloudGitifyNotifications } from '../../__mocks__/notifications-mocks';
 import { mockSettings } from '../../__mocks__/state-mocks';
 
+import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores';
+
 import type { Link } from '../../types';
 
 import * as comms from '../../utils/system/comms';
@@ -124,4 +126,46 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () =>
     const tree = renderWithProviders(<RepositoryNotifications {...props} />);
     expect(tree.container).toMatchSnapshot();
   });
+
+  describe('partial bulk failure', () => {
+    afterEach(() => {
+      useNotificationActionFailuresStore.getState().reset();
+    });
+
+    it('reverts the group exit animation when a notification within the bulk action failed', async () => {
+      const props: RepositoryNotificationsProps = {
+        repoName: 'gitify-app/notifications-test',
+        repoNotifications: mockGitHubCloudGitifyNotifications,
+      };
+
+      const [, secondNotification] = mockGitHubCloudGitifyNotifications;
+
+      // Simulate the mutation reconciliation that records a failure in the
+      // real (non-mocked) failure store, since `runGroupAction` reads
+      // directly from it rather than through the mocked `useNotifications`
+      // hook.
+      const markNotificationsAsReadWithFailure = vi.fn().mockImplementation(async () => {
+        const failureKey = getNotificationFailureKey(
+          secondNotification.account,
+          secondNotification.id,
+        );
+        useNotificationActionFailuresStore.getState().setFailure(failureKey, {
+          action: 'markAsRead',
+          error: { title: 'Action Forbidden', descriptions: [], emojis: [] },
+        });
+      });
+
+      renderWithProviders(<RepositoryNotifications {...props} />, {
+        settings: { ...mockSettings },
+        markNotificationsAsRead: markNotificationsAsReadWithFailure,
+      });
+
+      await userEvent.click(screen.getByTestId('repository-mark-as-read'));
+
+      // Since one of this group's notifications has a recorded failure, the
+      // repository row's own exit animation is reverted - its hover actions
+      // remain reachable rather than staying hidden.
+      expect(screen.getByTestId('repository-mark-as-read')).toBeInTheDocument();
+    });
+  });
 });