diff --git a/src/renderer/__helpers__/hook-mocks.ts b/src/renderer/__helpers__/hook-mocks.ts index 95016f9ef..cdfab069f 100644 --- a/src/renderer/__helpers__/hook-mocks.ts +++ b/src/renderer/__helpers__/hook-mocks.ts @@ -41,6 +41,8 @@ function buildNotificationsDefaults(): NotificationsState { markNotificationsAsRead: vi.fn(), markNotificationsAsDone: vi.fn(), unsubscribeNotification: vi.fn(), + + notificationFailures: {}, }; } diff --git a/src/renderer/components/notifications/NotificationRow.test.tsx b/src/renderer/components/notifications/NotificationRow.test.tsx index 50d04e1eb..9a7bab289 100644 --- a/src/renderer/components/notifications/NotificationRow.test.tsx +++ b/src/renderer/components/notifications/NotificationRow.test.tsx @@ -8,8 +8,11 @@ import { } from '../../__mocks__/notifications-mocks'; import { mockSettings } from '../../__mocks__/state-mocks'; +import { 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 +266,143 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { expect(screen.queryByTestId('notification-unsubscribe-from-thread')).not.toBeInTheDocument(); }); }); + + describe('failure recovery', () => { + it('shows hover actions in their normal (non-danger) state when there is no recorded failure', () => { + const props: NotificationRowProps = { + notification: mockGitifyNotification, + isRepositoryAnimatingExit: false, + }; + + renderWithProviders(, { + notificationFailures: {}, + }); + + expect(screen.getByTestId('notification-mark-as-read')).toHaveAttribute( + 'title', + 'Mark as read', + ); + }); + + it('colors the hover actions and explains the failure via their tooltip when the notification has a recorded failure', () => { + const props: NotificationRowProps = { + notification: mockGitifyNotification, + isRepositoryAnimatingExit: false, + }; + + renderWithProviders(, { + notificationFailures: { + [mockGitifyNotification.id]: { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN }, + }, + }); + + 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.'), + ); + }); + + 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, + }; + + renderWithProviders(, { + markNotificationsAsDone: markNotificationsAsDoneMock, + notificationFailures: { + [mockGitifyNotification.id]: { action: 'markAsDone', error: Errors.ACTION_FORBIDDEN }, + }, + }); + + 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, + }; + + renderWithProviders(, { + markNotificationsAsRead: markNotificationsAsReadMock, + notificationFailures: { + [mockGitifyNotification.id]: { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN }, + }, + }); + + 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(mockGitifyNotification.id, { + action: 'markAsRead', + error: Errors.ACTION_FORBIDDEN, + }); + + let resolveRetry: () => void = () => {}; + const markNotificationsAsReadMock = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveRetry = resolve; + }), + ); + + const props: NotificationRowProps = { + notification: mockGitifyNotification, + isRepositoryAnimatingExit: false, + }; + + renderWithProviders(, { + settings: { ...mockSettings, delayNotificationState: false, fetchReadNotifications: false }, + markNotificationsAsRead: markNotificationsAsReadMock, + notificationFailures: { + [mockGitifyNotification.id]: { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN }, + }, + }); + + 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(mockGitifyNotification.id, { + action: 'markAsRead', + error: Errors.ACTION_FORBIDDEN, + }); + resolveRetry(); + + await screen.findByTestId('notification-mark-as-read'); + + useNotificationActionFailuresStore.getState().reset(); + }); + }); }); diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index 086a14b3e..e16c0853d 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -4,7 +4,7 @@ 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 { useNotificationActionFailuresStore, useSettingsStore } from '../../stores'; import { HoverButton } from '../primitives/HoverButton'; import { HoverGroup } from '../primitives/HoverGroup'; @@ -32,8 +32,12 @@ export const NotificationRow: FC = ({ notification, isRepositoryAnimatingExit, }: NotificationRowProps) => { - const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } = - useNotifications(); + const { + markNotificationsAsRead, + markNotificationsAsDone, + unsubscribeNotification, + notificationFailures, + } = useNotifications(); const markAsDoneOnOpen = useSettingsStore((s) => s.markAsDoneOnOpen); const wrapNotificationTitle = useSettingsStore((s) => s.wrapNotificationTitle); @@ -43,31 +47,47 @@ export const NotificationRow: FC = ({ const shouldAnimateExit = shouldRemoveNotificationsFromState(); - const actionNotificationInteraction = () => { + const failure = notificationFailures[notification.id]; + + // 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) => { setShouldAnimateNotificationExit(shouldAnimateExit); - openNotification(notification); - if (markAsDoneOnOpen) { - markNotificationsAsDone([notification]); - } else { - markNotificationsAsRead([notification]); + await action(); + + if (useNotificationActionFailuresStore.getState().failures[notification.id]) { + 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; @@ -134,24 +154,27 @@ export const NotificationRow: FC = ({ action={actionMarkAsRead} enabled={!isNotificationRead} icon={ReadIcon} - label="Mark as read" + label={failureTooltip ?? 'Mark as read'} testid="notification-mark-as-read" + variant={failure ? 'danger' : 'invisible'} /> )} diff --git a/src/renderer/components/notifications/RepositoryNotifications.test.tsx b/src/renderer/components/notifications/RepositoryNotifications.test.tsx index b2f521a24..dfec93d68 100644 --- a/src/renderer/components/notifications/RepositoryNotifications.test.tsx +++ b/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 { useNotificationActionFailuresStore } from '../../stores'; + import type { Link } from '../../types'; import * as comms from '../../utils/system/comms'; @@ -124,4 +126,42 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => const tree = renderWithProviders(); 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 () => { + useNotificationActionFailuresStore.getState().setFailure(secondNotification.id, { + action: 'markAsRead', + error: { title: 'Action Forbidden', descriptions: [], emojis: [] }, + }); + }); + + renderWithProviders(, { + 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(); + }); + }); }); diff --git a/src/renderer/components/notifications/RepositoryNotifications.tsx b/src/renderer/components/notifications/RepositoryNotifications.tsx index 989d14fdc..25fa84e5e 100644 --- a/src/renderer/components/notifications/RepositoryNotifications.tsx +++ b/src/renderer/components/notifications/RepositoryNotifications.tsx @@ -4,6 +4,7 @@ import { CheckIcon, ReadIcon } from '@primer/octicons-react'; import { Button, Stack } from '@primer/react'; import { useNotifications } from '../../hooks/useNotifications'; +import { useNotificationActionFailuresStore } from '../../stores'; import { HoverButton } from '../primitives/HoverButton'; import { HoverGroup } from '../primitives/HoverGroup'; @@ -39,16 +40,28 @@ export const RepositoryNotifications: FC = ({ openRepository(repoNotifications[0].repository); }; - const actionMarkAsDone = () => { + // Starts the group's exit animation immediately, then reverts it if any + // notification in this bulk action failed, checked directly against the + // failure store once it settles (see `NotificationRow`'s `runAction` for + // why not a stale-state effect). There is no group-level rollup indicator; + // only the specific failed row(s) recolor their own hover actions. + const runGroupAction = async (action: () => Promise) => { setShouldAnimateRepositoryExit(shouldAnimateExit); - markNotificationsAsDone(repoNotifications); - }; - const actionMarkAsRead = () => { - setShouldAnimateRepositoryExit(shouldAnimateExit); - markNotificationsAsRead(repoNotifications); + await action(); + + const { failures } = useNotificationActionFailuresStore.getState(); + const hasFailure = repoNotifications.some((notification) => failures[notification.id]); + + if (hasFailure) { + setShouldAnimateRepositoryExit(false); + } }; + const actionMarkAsDone = () => runGroupAction(() => markNotificationsAsDone(repoNotifications)); + + const actionMarkAsRead = () => runGroupAction(() => markNotificationsAsRead(repoNotifications)); + const actionToggleRepositoryNotifications = () => { setIsRepositoryNotificationsVisible(!isRepositoryNotificationsVisible); }; diff --git a/src/renderer/components/primitives/HoverButton.tsx b/src/renderer/components/primitives/HoverButton.tsx index 5071d1676..bbc3f82a6 100644 --- a/src/renderer/components/primitives/HoverButton.tsx +++ b/src/renderer/components/primitives/HoverButton.tsx @@ -3,16 +3,20 @@ import type { FC } from 'react'; import type { Icon } from '@primer/octicons-react'; import { IconButton } from '@primer/react'; +import type { VariantType } from '../../types'; + interface HoverButtonProps { label: string; icon: Icon; enabled?: boolean; testid: string; action: () => void; + variant?: VariantType; } export const HoverButton: FC = ({ enabled = true, + variant = 'invisible', ...props }: HoverButtonProps) => { return ( @@ -28,7 +32,7 @@ export const HoverButton: FC = ({ size="small" title={props.label} unsafeDisableTooltip={true} - variant="invisible" + variant={variant} /> ) ); diff --git a/src/renderer/constants.ts b/src/renderer/constants.ts index 87e88f476..33236e55b 100644 --- a/src/renderer/constants.ts +++ b/src/renderer/constants.ts @@ -65,6 +65,7 @@ export const Constants = { EMOJIS: { ALL_READ: ['🎉', '🎊', '🥳', '👏', '🙌', '😎', '🏖️', '🚀', '✨', '🏆'], ERRORS: { + ACTION_FORBIDDEN: ['🚫'], BAD_CREDENTIALS: ['🔓'], MISSING_SCOPES: ['🔭'], NETWORK: ['🛜'], diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 6e37269e4..b751bba80 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -8,6 +8,7 @@ import { mockGitHubEnterpriseServerAccount, } from '../__mocks__/account-mocks'; import { + mockGitHubCloudGitifyNotifications, mockGitifyNotification, mockMultipleAccountNotifications, mockSingleAccountNotifications, @@ -447,6 +448,58 @@ describe('renderer/hooks/useNotifications.ts', () => { expect(rendererLogErrorSpy).toHaveBeenCalled(); }); + + it('rolls back the cache for a failed notification while a failed request does not affect it', async () => { + vi.spyOn(githubAdapter, 'markThreadAsRead').mockRejectedValue(new Error('boom')); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + await act(async () => { + await result.current.markNotificationsAsRead([mockGitifyNotification]).catch(() => {}); + }); + + // The notification remains in the cache since its action failed + await waitFor(() => expect(result.current.notificationCount).toBe(1)); + expect(result.current.notificationFailures[mockGitifyNotification.id]).toBeDefined(); + }); + + it('tracks succeeded and failed notifications independently within a single bulk call', async () => { + const [succeedsNotification, failsNotification] = mockGitHubCloudGitifyNotifications; + + getAllNotificationsMock.mockResolvedValue([ + { + account: succeedsNotification.account, + notifications: [succeedsNotification, failsNotification], + error: null, + }, + ]); + + vi.spyOn(githubAdapter, 'markThreadAsRead').mockImplementation(async (_account, id) => { + if (id === failsNotification.id) { + throw new Error('boom'); + } + }); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.notificationCount).toBe(2)); + + await act(async () => { + await result.current + .markNotificationsAsRead([succeedsNotification, failsNotification]) + .catch(() => {}); + }); + + // The succeeded notification is removed; the failed one remains and is + // recorded in the failure map, not the other way around. + await waitFor(() => expect(result.current.notificationCount).toBe(1)); + expect( + result.current.notifications[0]?.notifications.some((n) => n.id === failsNotification.id), + ).toBe(true); + expect(result.current.notificationFailures[failsNotification.id]).toBeDefined(); + expect(result.current.notificationFailures[succeedsNotification.id]).toBeUndefined(); + }); }); describe('markNotificationsAsDone', () => { diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 57affdc60..ff13b893e 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -8,7 +8,14 @@ import { useQueryClient, } from '@tanstack/react-query'; -import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; +import { + type NotificationActionFailure, + type NotificationFailedActionType, + useAccountsStore, + useFiltersStore, + useNotificationActionFailuresStore, + useSettingsStore, +} from '../stores'; import { type Account, @@ -28,6 +35,11 @@ import { filterBaseNotifications, filterDetailedNotifications, } from '../utils/notifications/filters/filter'; +import { + restoreFailedNotifications, + settleNotificationActions, + type NotificationQuerySnapshot, +} from '../utils/notifications/mutations'; import { getAllNotifications, getNotificationCount, @@ -56,6 +68,13 @@ interface NotificationsState { markNotificationsAsRead: (notifications: GitifyNotification[]) => Promise; markNotificationsAsDone: (notifications: GitifyNotification[]) => Promise; unsubscribeNotification: (notification: GitifyNotification) => Promise; + + /** + * Session-local map of notification ID to the classified error from its + * most recent failed mark-as-read/mark-as-done/unsubscribe action attempt. + * Not persisted and not part of the notifications data itself. + */ + notificationFailures: Record; } interface UseNotificationsOptions { @@ -338,28 +357,130 @@ export const useNotifications = ({ notificationsQueryKey, ]); + const notificationFailures = useNotificationActionFailuresStore((s) => s.failures); + + // Session-local failure entries are independent of the notifications + // cache, so they must be pruned separately once a notification no longer + // appears in the (unfiltered) list - e.g. actioned successfully elsewhere, + // or its account was removed. Owned by the singleton side-effects host so + // it runs once per notifications update rather than once per mounted + // row/consumer. + useEffect(() => { + if (!withSideEffects) { + return; + } + + const unfilteredNotifications = + queryClient.getQueryData(notificationsQueryKey) || []; + + const currentNotificationIds = unfilteredNotifications.flatMap((accountNotifications) => + accountNotifications.notifications.map((notification) => notification.id), + ); + + useNotificationActionFailuresStore.getState().pruneFailures(currentNotificationIds); + }, [withSideEffects, notifications, queryClient, notificationsQueryKey]); + + // Shared by all three mutations' `onSuccess`. Records each failure's + // classified error in the session-local failure map and logs it; also + // re-applies `snapshot` for any failed notification missing from the + // cache, as a defensive guard (these mutations don't remove notifications + // until they succeed, so this is normally a no-op). + const reconcileFailedNotifications = useCallback( + ( + failed: Array<{ notification: GitifyNotification; error: GitifyError; rawError: Error }>, + snapshot: NotificationQuerySnapshot | undefined, + action: NotificationFailedActionType, + context: string, + ) => { + if (failed.length === 0) { + return; + } + + const failedNotifications = failed.map((f) => f.notification); + + if (snapshot) { + for (const [queryKey, snapshotData] of snapshot) { + queryClient.setQueryData(queryKey, (existing) => + restoreFailedNotifications(failedNotifications, snapshotData ?? [], existing ?? []), + ); + } + } + + for (const { notification, error, rawError } of failed) { + useNotificationActionFailuresStore.getState().setFailure(notification.id, { + action, + error, + }); + rendererLogError( + context, + `Error occurred while processing notification ${notification.id}`, + rawError, + ); + } + }, + [queryClient], + ); + + // Full-cache restore for `onError`, covering the rare case where a + // mutation function throws directly (e.g. a bug) rather than resolving via + // `settleNotificationActions`. Per-notification failures are handled by + // `reconcileFailedNotifications` above instead. + const restoreSnapshot = useCallback( + (snapshot?: NotificationQuerySnapshot) => { + if (!snapshot) { + return; + } + + for (const [queryKey, data] of snapshot) { + queryClient.setQueryData(queryKey, data); + } + }, + [queryClient], + ); + const markNotificationsAsReadMutation = useMutation({ mutationFn: async ({ readNotifications }: { readNotifications: GitifyNotification[] }) => { - await Promise.all( - readNotifications.map((notification) => - getAdapter(notification.account).markThreadAsRead(notification.account, notification.id), - ), + return await settleNotificationActions(readNotifications, (notification) => + getAdapter(notification.account).markThreadAsRead(notification.account, notification.id), ); }, - onSuccess: (_, { readNotifications }) => { - // Update the cached (unfiltered) data in place so filtered-out - // notifications are preserved and concurrent mutations compose. - queryClient.setQueryData(notificationsQueryKey, (existing) => - removeNotificationsForAccount( - readNotifications[0].account, - readNotifications, - existing ?? [], - ), + onMutate: async () => { + await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); + + const snapshot = queryClient.getQueriesData({ + queryKey: notificationsKeys.all, + }); + + return { snapshot }; + }, + + onSuccess: ({ succeeded, failed }, _variables, context) => { + // Cache removal happens here (once the request resolves) rather than + // optimistically in `onMutate`, so the row's exit animation - started + // synchronously on click - has time to play before the notification + // disappears from the list. + if (succeeded.length > 0) { + queryClient.setQueryData(notificationsQueryKey, (existing) => + removeNotificationsForAccount(succeeded[0].account, succeeded, existing ?? []), + ); + } + + for (const notification of succeeded) { + useNotificationActionFailuresStore.getState().clearFailure(notification.id); + } + + reconcileFailedNotifications( + failed, + context?.snapshot, + 'markAsRead', + 'markNotificationsAsRead', ); }, - onError: (err) => { + onError: (err, _variables, context) => { + restoreSnapshot(context?.snapshot); + rendererLogError( 'markNotificationsAsRead', 'Error occurred while marking notifications as read', @@ -375,37 +496,50 @@ export const useNotifications = ({ // Forges that don't support a distinct "done" state fall back to // marking as read so the user-visible action still removes the thread. if (!isMarkAsDoneFeatureSupported(account)) { - await markNotificationsAsReadMutation.mutateAsync({ + return await markNotificationsAsReadMutation.mutateAsync({ readNotifications: doneNotifications, }); - return false; } - await Promise.all( - doneNotifications.map((notification) => - getAdapter(notification.account).markThreadAsDone(notification.account, notification.id), - ), + return await settleNotificationActions(doneNotifications, (notification) => + getAdapter(notification.account).markThreadAsDone(notification.account, notification.id), ); + }, + + onMutate: async () => { + await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); + + const snapshot = queryClient.getQueriesData({ + queryKey: notificationsKeys.all, + }); - return true; + return { snapshot }; }, - onSuccess: (didMarkAsDone, { doneNotifications }) => { - // The mark-as-read fallback already updated the cache. - if (!didMarkAsDone) { - return; + onSuccess: ({ succeeded, failed }, { doneNotifications }, context) => { + // The mark-as-read fallback (for forges without a distinct "done" + // state) already updated the cache via its own mutation/onSuccess. + if (succeeded.length > 0 && isMarkAsDoneFeatureSupported(doneNotifications[0].account)) { + queryClient.setQueryData(notificationsQueryKey, (existing) => + removeNotificationsForAccount(succeeded[0].account, succeeded, existing ?? []), + ); + } + + for (const notification of succeeded) { + useNotificationActionFailuresStore.getState().clearFailure(notification.id); } - queryClient.setQueryData(notificationsQueryKey, (existing) => - removeNotificationsForAccount( - doneNotifications[0].account, - doneNotifications, - existing ?? [], - ), + reconcileFailedNotifications( + failed, + context?.snapshot, + 'markAsDone', + 'markNotificationsAsDone', ); }, - onError: (err) => { + onError: (err, _variables, context) => { + restoreSnapshot(context?.snapshot); + rendererLogError( 'markNotificationsAsDone', 'Error occurred while marking notifications as done', @@ -419,14 +553,17 @@ export const useNotifications = ({ // Forges without thread-subscription support cannot unsubscribe; the UI // already hides the action, but treat duplicate calls as no-ops. if (!isUnsubscribeThreadSupported(notification.account)) { - return; + return { succeeded: [notification], failed: [] }; } - await getAdapter(notification.account).unsubscribeThread( - notification.account, - notification.id, + const result = await settleNotificationActions([notification], (n) => + getAdapter(n.account).unsubscribeThread(n.account, n.id), ); + if (result.failed.length > 0) { + return result; + } + if (markAsDoneOnUnsubscribe) { await markNotificationsAsDoneMutation.mutateAsync({ doneNotifications: [notification], @@ -436,9 +573,36 @@ export const useNotifications = ({ readNotifications: [notification], }); } + + return result; + }, + + onMutate: async () => { + await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); + + const snapshot = queryClient.getQueriesData({ + queryKey: notificationsKeys.all, + }); + + return { snapshot }; }, - onError: (err) => { + onSuccess: ({ succeeded, failed }, _variables, context) => { + for (const notification of succeeded) { + useNotificationActionFailuresStore.getState().clearFailure(notification.id); + } + + reconcileFailedNotifications( + failed, + context?.snapshot, + 'unsubscribe', + 'unsubscribeNotification', + ); + }, + + onError: (err, _variables, context) => { + restoreSnapshot(context?.snapshot); + rendererLogError( 'unsubscribeNotification', 'Error occurred while unsubscribing from notification thread', @@ -447,8 +611,6 @@ export const useNotifications = ({ }, }); - // Mutation failures are logged via each mutation's onError handler and - // swallowed here so UI callers can fire-and-forget these actions. const markNotificationsAsRead = useCallback( async (readNotifications: GitifyNotification[]) => { await markNotificationsAsReadMutation.mutateAsync({ readNotifications }).catch(() => {}); @@ -486,5 +648,7 @@ export const useNotifications = ({ markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification, + + notificationFailures, }; }; diff --git a/src/renderer/stores/index.ts b/src/renderer/stores/index.ts index 3282752b9..cfe9783d9 100644 --- a/src/renderer/stores/index.ts +++ b/src/renderer/stores/index.ts @@ -3,4 +3,5 @@ export * from './types'; export * from './defaults'; export { default as useAccountsStore } from './useAccountsStore'; export { default as useFiltersStore } from './useFiltersStore'; +export { default as useNotificationActionFailuresStore } from './useNotificationActionFailuresStore'; export { default as useSettingsStore } from './useSettingsStore'; diff --git a/src/renderer/stores/types.ts b/src/renderer/stores/types.ts index e5502b999..49c2feccb 100644 --- a/src/renderer/stores/types.ts +++ b/src/renderer/stores/types.ts @@ -3,6 +3,7 @@ import type { AccountUUID, FilterStateType, Forge, + GitifyError, Hostname, Reason, ReviewRequestType, @@ -196,3 +197,74 @@ export interface SettingsActions { * Complete settings store type. */ export type SettingsStore = SettingsState & SettingsActions; + +// ============================================================================ +// Notification Action Failures Store Types +// ============================================================================ + +/** + * The notification action that most recently failed for a given notification. + */ +export type NotificationFailedActionType = 'markAsRead' | 'markAsDone' | 'unsubscribe'; + +/** + * A recorded action failure for a single notification. + */ +export interface NotificationActionFailure { + /** + * The action that failed (used to know what to re-invoke on retry). + */ + action: NotificationFailedActionType; + + /** + * The classified error for the failure. + */ + error: GitifyError; +} + +/** + * Ephemeral, session-local state tracking per-notification action failures. + * + * Not persisted and not part of the TanStack Query cache - a failed + * mark-as-read/mark-as-done/unsubscribe action for a notification is + * recorded here so `NotificationRow` can recolor/re-label that row's hover + * actions, independent of the notification data itself. + */ +export interface NotificationActionFailuresState { + /** + * Map of notification ID to the details of its most recent failed action attempt. + */ + failures: Record; +} + +/** + * Actions for managing per-notification action failures. + */ +export interface NotificationActionFailuresActions { + /** + * Records a failed action for a notification. + */ + setFailure: (notificationId: string, failure: NotificationActionFailure) => void; + + /** + * Clears a recorded failure for a notification (e.g. after a successful retry). + */ + clearFailure: (notificationId: string) => void; + + /** + * Clears any recorded failures for notification IDs not present in `notificationIds` + * (e.g. once a notification no longer appears in the notifications list). + */ + pruneFailures: (notificationIds: string[]) => void; + + /** + * Resets the store to its default (empty) state. + */ + reset: () => void; +} + +/** + * Complete notification action failures store type. + */ +export type NotificationActionFailuresStore = NotificationActionFailuresState & + NotificationActionFailuresActions; diff --git a/src/renderer/stores/useNotificationActionFailuresStore.ts b/src/renderer/stores/useNotificationActionFailuresStore.ts new file mode 100644 index 000000000..dd6dfa1c3 --- /dev/null +++ b/src/renderer/stores/useNotificationActionFailuresStore.ts @@ -0,0 +1,52 @@ +import { create } from 'zustand'; + +import type { NotificationActionFailuresStore } from './types'; + +/** + * Gitify Notification Action Failures store. + * + * Ephemeral, session-local state (not persisted, not part of the TanStack + * Query cache) tracking which notifications had their most recent + * mark-as-read/mark-as-done/unsubscribe action fail, and with what + * classified error. Cleared on successful retry or when a notification no + * longer appears in the notifications list. + */ +const useNotificationActionFailuresStore = create((set, get) => ({ + failures: {}, + + setFailure: (notificationId, failure) => { + set((state) => ({ failures: { ...state.failures, [notificationId]: failure } })); + }, + + clearFailure: (notificationId) => { + const { failures } = get(); + if (!(notificationId in failures)) { + return; + } + + const nextFailures = { ...failures }; + delete nextFailures[notificationId]; + set({ failures: nextFailures }); + }, + + pruneFailures: (notificationIds) => { + const { failures } = get(); + const idsToKeep = new Set(notificationIds); + + const remainingEntries = Object.entries(failures).filter(([notificationId]) => + idsToKeep.has(notificationId), + ); + + if (remainingEntries.length === Object.keys(failures).length) { + return; + } + + set({ failures: Object.fromEntries(remainingEntries) }); + }, + + reset: () => { + set({ failures: {} }); + }, +})); + +export default useNotificationActionFailuresStore; diff --git a/src/renderer/types.ts b/src/renderer/types.ts index 9a27a34ce..8fbe29d45 100644 --- a/src/renderer/types.ts +++ b/src/renderer/types.ts @@ -4,7 +4,7 @@ import type { Icon, OcticonProps } from '@primer/octicons-react'; import type { Button } from '@primer/react'; // Derived from public @primer/react component props rather than internal types -type VariantType = NonNullable['variant']>; +export type VariantType = NonNullable['variant']>; import type { AuthMethod, PlatformType } from './utils/auth/types'; @@ -230,6 +230,7 @@ export interface GitifyErrorAction { * The different types of errors which may be encountered. */ export type ErrorType = + | 'ACTION_FORBIDDEN' | 'BAD_CREDENTIALS' | 'MISSING_SCOPES' | 'NETWORK' diff --git a/src/renderer/utils/api/errors.test.ts b/src/renderer/utils/api/errors.test.ts index 897cf8795..99d944a2d 100644 --- a/src/renderer/utils/api/errors.test.ts +++ b/src/renderer/utils/api/errors.test.ts @@ -74,6 +74,22 @@ describe('renderer/utils/api/errors.ts', () => { expect(result).toBe(Errors.RATE_LIMITED); }); + it('action forbidden - unmatched 403', () => { + const mockError = new RequestError( + 'As an Enterprise Managed User, you cannot access this content', + 403, + { + request: { + method: 'GET', + url: 'https://api.github.com', + headers: {}, + }, + }, + ); + const result = determineFailureType(mockError); + expect(result).toBe(Errors.ACTION_FORBIDDEN); + }); + it('network error - no status', () => { const mockError = new RequestError('Network error', 500, { request: { diff --git a/src/renderer/utils/api/errors.ts b/src/renderer/utils/api/errors.ts index 96feafb27..f45047d1b 100644 --- a/src/renderer/utils/api/errors.ts +++ b/src/renderer/utils/api/errors.ts @@ -44,7 +44,7 @@ export function determineFailureType( return Errors.RATE_LIMITED; } - break; + return Errors.ACTION_FORBIDDEN; case 500: return Errors.NETWORK; default: diff --git a/src/renderer/utils/core/errors.ts b/src/renderer/utils/core/errors.ts index 8d58a7afe..b3a7b5bc2 100644 --- a/src/renderer/utils/core/errors.ts +++ b/src/renderer/utils/core/errors.ts @@ -5,6 +5,11 @@ import { Constants } from '../../constants'; import type { AccountNotifications, ErrorType, GitifyError } from '../../types'; export const Errors: Record = { + ACTION_FORBIDDEN: { + title: 'Action Forbidden', + descriptions: ['GitHub rejected this action for this account when performed via Gitify.'], + emojis: Constants.EMOJIS.ERRORS.ACTION_FORBIDDEN, + }, BAD_CREDENTIALS: { title: 'Bad Credentials', descriptions: ['Your credentials are either invalid or expired.'], diff --git a/src/renderer/utils/notifications/mutations.test.ts b/src/renderer/utils/notifications/mutations.test.ts new file mode 100644 index 000000000..cd39df952 --- /dev/null +++ b/src/renderer/utils/notifications/mutations.test.ts @@ -0,0 +1,126 @@ +import { RequestError } from '@octokit/request-error'; + +import { + mockGitHubCloudGitifyNotifications, + mockGithubEnterpriseGitifyNotifications, +} from '../../__mocks__/notifications-mocks'; + +import type { AccountNotifications } from '../../types'; + +import { Errors } from '../core/errors'; +import { restoreFailedNotifications, settleNotificationActions } from './mutations'; + +describe('renderer/utils/notifications/mutations.ts', () => { + describe('settleNotificationActions', () => { + it('tracks all notifications as succeeded when every action resolves', async () => { + const notifications = mockGitHubCloudGitifyNotifications; + const action = vi.fn().mockResolvedValue(undefined); + + const result = await settleNotificationActions(notifications, action); + + expect(result.succeeded).toEqual(notifications); + expect(result.failed).toEqual([]); + expect(action).toHaveBeenCalledTimes(notifications.length); + }); + + it('tracks a partial failure within a bulk action independently', async () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + const forbiddenError = new RequestError('Forbidden', 403, { + request: { method: 'GET', url: 'https://api.github.com', headers: {} }, + }); + + const action = vi.fn().mockResolvedValueOnce(undefined).mockRejectedValueOnce(forbiddenError); + + const result = await settleNotificationActions([first, second], action); + + expect(result.succeeded).toEqual([first]); + expect(result.failed).toHaveLength(1); + expect(result.failed[0].notification).toEqual(second); + expect(result.failed[0].error).toBe(Errors.ACTION_FORBIDDEN); + expect(result.failed[0].rawError).toBe(forbiddenError); + }); + + it('classifies each failure independently using determineFailureType', async () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + + const action = vi + .fn() + .mockRejectedValueOnce( + new RequestError("Missing the 'notifications' scope", 403, { + request: { method: 'GET', url: 'https://api.github.com', headers: {} }, + }), + ) + .mockRejectedValueOnce( + new RequestError('Forbidden', 403, { + request: { method: 'GET', url: 'https://api.github.com', headers: {} }, + }), + ); + + const result = await settleNotificationActions([first, second], action); + + expect(result.succeeded).toEqual([]); + expect(result.failed[0].error).toBe(Errors.MISSING_SCOPES); + expect(result.failed[1].error).toBe(Errors.ACTION_FORBIDDEN); + }); + }); + + describe('restoreFailedNotifications', () => { + it('returns current data unchanged when there are no failed notifications', () => { + const current: AccountNotifications[] = [ + { account: mockGitHubCloudGitifyNotifications[0].account, notifications: [], error: null }, + ]; + + const result = restoreFailedNotifications([], current, current); + + expect(result).toBe(current); + }); + + it('restores a failed notification back into its account entry, preserving original data', () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + const account = first.account; + + const snapshot: AccountNotifications[] = [ + { account, notifications: [first, second], error: null }, + ]; + + // `current` simulates both notifications already being absent from + // this account entry (e.g. removed by a prior cache update). + const current: AccountNotifications[] = [{ account, notifications: [], error: null }]; + + const result = restoreFailedNotifications([second], snapshot, current); + + expect(result[0].notifications).toEqual([second]); + }); + + it('leaves succeeded (still-removed) notifications out and keeps unrelated accounts untouched', () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + const account = first.account; + const otherAccount = mockGithubEnterpriseGitifyNotifications[0].account; + + const snapshot: AccountNotifications[] = [ + { account, notifications: [first, second], error: null }, + { + account: otherAccount, + notifications: mockGithubEnterpriseGitifyNotifications, + error: null, + }, + ]; + + // `first` succeeded (absent from `current`), `second` failed and must + // be restored from `snapshot`. + const current: AccountNotifications[] = [ + { account, notifications: [], error: null }, + { + account: otherAccount, + notifications: mockGithubEnterpriseGitifyNotifications, + error: null, + }, + ]; + + const result = restoreFailedNotifications([second], snapshot, current); + + expect(result[0].notifications).toEqual([second]); + expect(result[1].notifications).toEqual(mockGithubEnterpriseGitifyNotifications); + }); + }); +}); diff --git a/src/renderer/utils/notifications/mutations.ts b/src/renderer/utils/notifications/mutations.ts new file mode 100644 index 000000000..cf7a9f262 --- /dev/null +++ b/src/renderer/utils/notifications/mutations.ts @@ -0,0 +1,130 @@ +import type { QueryKey } from '@tanstack/react-query'; + +import type { AccountNotifications, GitifyError, GitifyNotification } from '../../types'; + +import { determineFailureType } from '../api/errors'; +import { getAccountUUID } from '../auth/utils'; +import { toError } from '../core/logger'; + +/** + * The classified outcome of a single notification's failed action request + * within a bulk/group mutation. + */ +export interface FailedNotificationAction { + notification: GitifyNotification; + error: GitifyError; + rawError: Error; +} + +/** + * The per-notification outcome of a bulk/group mutation: which notifications + * succeeded, and which failed (with their classified error). + */ +export interface SettledNotificationActions { + succeeded: GitifyNotification[]; + failed: FailedNotificationAction[]; +} + +/** + * A query key paired with its snapshotted `AccountNotifications[]` data, as + * returned by `queryClient.getQueriesData`. + */ +export type NotificationQuerySnapshot = [QueryKey, AccountNotifications[] | undefined][]; + +/** + * Run an action against each notification independently via + * `Promise.allSettled`, so one failing notification doesn't obscure the + * outcome of the rest of a bulk/group action. + * + * @param notifications The notifications to execute the action against. + * @param action The per-notification async action to execute. + * @returns The notifications that succeeded, and the notifications that + * failed alongside their classified error. + */ +export async function settleNotificationActions( + notifications: GitifyNotification[], + action: (notification: GitifyNotification) => Promise, +): Promise { + const results = await Promise.allSettled( + notifications.map((notification) => action(notification)), + ); + + const succeeded: GitifyNotification[] = []; + const failed: FailedNotificationAction[] = []; + + results.forEach((result, index) => { + const notification = notifications[index]; + + if (result.status === 'fulfilled') { + succeeded.push(notification); + return; + } + + const rawError = toError(result.reason); + failed.push({ + notification, + error: determineFailureType(rawError), + rawError, + }); + }); + + return { succeeded, failed }; +} + +/** + * Restore notifications that failed their action back into a query's cached + * data, using a pre-mutation snapshot as the source of truth for their + * original data (e.g. `unread` state, ordering). Notifications that + * succeeded or were otherwise already absent from `current` are left as-is. + * + * Cache changes for these mutations are applied in `onSuccess` (not + * optimistically), so in practice failed notifications are rarely absent + * from `current` to begin with - this exists as a defensive guard for any + * concurrent cache change that removed them in the meantime. + * + * @param failedNotifications The notifications whose action failed and should be present. + * @param snapshot The pre-mutation snapshot of `AccountNotifications[]` to restore from. + * @param current The current `AccountNotifications[]`. + * @returns A new `AccountNotifications[]` with failed notifications present. + */ +export function restoreFailedNotifications( + failedNotifications: GitifyNotification[], + snapshot: AccountNotifications[], + current: AccountNotifications[], +): AccountNotifications[] { + if (failedNotifications.length === 0) { + return current; + } + + const failedIdsByAccount = new Map>(); + for (const notification of failedNotifications) { + const accountKey = getAccountUUID(notification.account); + if (!failedIdsByAccount.has(accountKey)) { + failedIdsByAccount.set(accountKey, new Set()); + } + failedIdsByAccount.get(accountKey)?.add(notification.id); + } + + return current.map((accountEntry) => { + const accountKey = getAccountUUID(accountEntry.account); + const failedIds = failedIdsByAccount.get(accountKey); + + if (!failedIds) { + return accountEntry; + } + + const snapshotEntry = snapshot.find((entry) => getAccountUUID(entry.account) === accountKey); + + if (!snapshotEntry) { + return accountEntry; + } + + const currentIds = new Set(accountEntry.notifications.map((notification) => notification.id)); + + const restoredNotifications = snapshotEntry.notifications.filter( + (notification) => currentIds.has(notification.id) || failedIds.has(notification.id), + ); + + return { ...accountEntry, notifications: restoredNotifications }; + }); +}