From 0688c10158c8c14fed04f3f47c2a800d99c52f66 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 4 Aug 2026 08:51:38 -0400 Subject: [PATCH 1/2] fix: rollback failed notification interaction with visual warning Signed-off-by: Adam Setch --- src/renderer/__helpers__/hook-mocks.ts | 2 + .../notifications/NotificationRow.test.tsx | 142 ++++++++++ .../notifications/NotificationRow.tsx | 69 +++-- .../RepositoryNotifications.test.tsx | 40 +++ .../notifications/RepositoryNotifications.tsx | 25 +- .../components/primitives/HoverButton.tsx | 6 +- src/renderer/constants.ts | 1 + src/renderer/hooks/useNotifications.test.tsx | 53 ++++ src/renderer/hooks/useNotifications.ts | 244 +++++++++++++++--- src/renderer/stores/index.ts | 1 + src/renderer/stores/types.ts | 72 ++++++ .../useNotificationActionFailuresStore.ts | 52 ++++ src/renderer/types.ts | 3 +- src/renderer/utils/api/errors.test.ts | 16 ++ src/renderer/utils/api/errors.ts | 2 +- src/renderer/utils/core/errors.ts | 5 + .../utils/notifications/mutations.test.ts | 126 +++++++++ src/renderer/utils/notifications/mutations.ts | 130 ++++++++++ 18 files changed, 917 insertions(+), 72 deletions(-) create mode 100644 src/renderer/stores/useNotificationActionFailuresStore.ts create mode 100644 src/renderer/utils/notifications/mutations.test.ts create mode 100644 src/renderer/utils/notifications/mutations.ts 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 2e1c478c1..f40400475 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; @@ -143,24 +163,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 ad1d58c5d..8eb2b6f98 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 24a9c7443..1348c8bdf 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, @@ -479,6 +480,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 d33ba797e..0031ac8ff 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -10,7 +10,14 @@ import { import { Constants } from '../constants'; -import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; +import { + type NotificationActionFailure, + type NotificationFailedActionType, + useAccountsStore, + useFiltersStore, + useNotificationActionFailuresStore, + useSettingsStore, +} from '../stores'; import { type Account, @@ -30,6 +37,11 @@ import { filterBaseNotifications, filterDetailedNotifications, } from '../utils/notifications/filters/filter'; +import { + restoreFailedNotifications, + settleNotificationActions, + type NotificationQuerySnapshot, +} from '../utils/notifications/mutations'; import { getAllNotifications, getNotificationCount, @@ -58,6 +70,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 { @@ -349,28 +368,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', @@ -386,37 +507,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', @@ -430,14 +564,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], @@ -447,9 +584,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', @@ -458,8 +622,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(() => {}); @@ -497,5 +659,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 85e61bbdb..95a02c52b 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'; @@ -241,6 +241,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 }; + }); +} From 8cc323db144bf987975dcfa2e637c6f37f53242d Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 8 Aug 2026 08:05:26 -0400 Subject: [PATCH 2/2] incorporate pr feedback Signed-off-by: Adam Setch --- src/renderer/__helpers__/hook-mocks.ts | 2 - .../notifications/NotificationRow.test.tsx | 85 ++++++-- .../notifications/NotificationRow.tsx | 41 ++-- .../RepositoryNotifications.test.tsx | 8 +- .../notifications/RepositoryNotifications.tsx | 6 +- src/renderer/hooks/useNotifications.test.tsx | 70 ++++++- src/renderer/hooks/useNotifications.ts | 195 ++++++++---------- src/renderer/stores/index.ts | 5 +- src/renderer/stores/types.ts | 11 +- .../useNotificationActionFailuresStore.ts | 39 +++- src/renderer/utils/core/errors.ts | 2 +- .../utils/notifications/mutations.test.ts | 17 ++ src/renderer/utils/notifications/mutations.ts | 13 +- 13 files changed, 325 insertions(+), 169 deletions(-) diff --git a/src/renderer/__helpers__/hook-mocks.ts b/src/renderer/__helpers__/hook-mocks.ts index cdfab069f..95016f9ef 100644 --- a/src/renderer/__helpers__/hook-mocks.ts +++ b/src/renderer/__helpers__/hook-mocks.ts @@ -41,8 +41,6 @@ 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 9a7bab289..b16451430 100644 --- a/src/renderer/components/notifications/NotificationRow.test.tsx +++ b/src/renderer/components/notifications/NotificationRow.test.tsx @@ -2,13 +2,14 @@ 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 { useNotificationActionFailuresStore } from '../../stores'; +import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; import { GroupBy } from '../../types'; @@ -268,15 +269,22 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { }); 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(, { - notificationFailures: {}, - }); + renderWithProviders(); expect(screen.getByTestId('notification-mark-as-read')).toHaveAttribute( 'title', @@ -284,17 +292,17 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { ); }); - it('colors the hover actions and explains the failure via their tooltip when the notification has a recorded failure', () => { + it('styles and explains only the action that failed', () => { const props: NotificationRowProps = { notification: mockGitifyNotification, isRepositoryAnimatingExit: false, }; - renderWithProviders(, { - notificationFailures: { - [mockGitifyNotification.id]: { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN }, - }, + useNotificationActionFailuresStore.getState().setFailure(failureKey, { + action: 'markAsRead', + error: Errors.ACTION_FORBIDDEN, }); + renderWithProviders(); const markAsReadButton = screen.getByTestId('notification-mark-as-read'); @@ -308,6 +316,42 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { '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( + <> + + + , + ); + + 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 () => { @@ -318,11 +362,12 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { isRepositoryAnimatingExit: false, }; + useNotificationActionFailuresStore.getState().setFailure(failureKey, { + action: 'markAsDone', + error: Errors.ACTION_FORBIDDEN, + }); renderWithProviders(, { markNotificationsAsDone: markNotificationsAsDoneMock, - notificationFailures: { - [mockGitifyNotification.id]: { action: 'markAsDone', error: Errors.ACTION_FORBIDDEN }, - }, }); await userEvent.click(screen.getByTestId('notification-mark-as-done')); @@ -339,11 +384,12 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { isRepositoryAnimatingExit: false, }; + useNotificationActionFailuresStore.getState().setFailure(failureKey, { + action: 'markAsRead', + error: Errors.ACTION_FORBIDDEN, + }); renderWithProviders(, { markNotificationsAsRead: markNotificationsAsReadMock, - notificationFailures: { - [mockGitifyNotification.id]: { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN }, - }, }); const markAsReadButton = screen.getByTestId('notification-mark-as-read'); @@ -360,7 +406,7 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { // (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, { + useNotificationActionFailuresStore.getState().setFailure(failureKey, { action: 'markAsRead', error: Errors.ACTION_FORBIDDEN, }); @@ -381,9 +427,6 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { 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')); @@ -394,15 +437,13 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { // The retry fails again; the store still has a (new) failure entry for // this notification once the mutation resolves. - useNotificationActionFailuresStore.getState().setFailure(mockGitifyNotification.id, { + useNotificationActionFailuresStore.getState().setFailure(failureKey, { 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 f40400475..4e0bf5777 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/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 { useNotificationActionFailuresStore, useSettingsStore } from '../../stores'; +import { + getNotificationFailureKey, + useNotificationActionFailuresStore, + useSettingsStore, +} from '../../stores'; import { HoverButton } from '../primitives/HoverButton'; import { HoverGroup } from '../primitives/HoverGroup'; @@ -32,12 +36,8 @@ export const NotificationRow: FC = ({ notification, isRepositoryAnimatingExit, }: NotificationRowProps) => { - const { - markNotificationsAsRead, - markNotificationsAsDone, - unsubscribeNotification, - notificationFailures, - } = useNotifications(); + const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } = + useNotifications(); const markAsDoneOnOpen = useSettingsStore((s) => s.markAsDoneOnOpen); const wrapNotificationTitle = useSettingsStore((s) => s.wrapNotificationTitle); @@ -47,7 +47,10 @@ export const NotificationRow: FC = ({ const shouldAnimateExit = shouldRemoveNotificationsFromState(); - const failure = notificationFailures[notification.id]; + 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 @@ -68,7 +71,7 @@ export const NotificationRow: FC = ({ await action(); - if (useNotificationActionFailuresStore.getState().failures[notification.id]) { + if (useNotificationActionFailuresStore.getState().failures[notificationFailureKey]) { setShouldAnimateNotificationExit(false); } }; @@ -163,27 +166,35 @@ export const NotificationRow: FC = ({ action={actionMarkAsRead} enabled={!isNotificationRead} icon={ReadIcon} - label={failureTooltip ?? 'Mark as read'} + label={ + failure?.action === 'markAsRead' ? (failureTooltip ?? 'Mark as read') : 'Mark as read' + } testid="notification-mark-as-read" - variant={failure ? 'danger' : 'invisible'} + variant={failure?.action === 'markAsRead' ? 'danger' : 'invisible'} /> )} diff --git a/src/renderer/components/notifications/RepositoryNotifications.test.tsx b/src/renderer/components/notifications/RepositoryNotifications.test.tsx index dfec93d68..121edc216 100644 --- a/src/renderer/components/notifications/RepositoryNotifications.test.tsx +++ b/src/renderer/components/notifications/RepositoryNotifications.test.tsx @@ -5,7 +5,7 @@ import { renderWithProviders } from '../../__helpers__/test-utils'; import { mockGitHubCloudGitifyNotifications } from '../../__mocks__/notifications-mocks'; import { mockSettings } from '../../__mocks__/state-mocks'; -import { useNotificationActionFailuresStore } from '../../stores'; +import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; import type { Link } from '../../types'; @@ -145,7 +145,11 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => // directly from it rather than through the mocked `useNotifications` // hook. const markNotificationsAsReadWithFailure = vi.fn().mockImplementation(async () => { - useNotificationActionFailuresStore.getState().setFailure(secondNotification.id, { + const failureKey = getNotificationFailureKey( + secondNotification.account, + secondNotification.id, + ); + useNotificationActionFailuresStore.getState().setFailure(failureKey, { action: 'markAsRead', error: { title: 'Action Forbidden', descriptions: [], emojis: [] }, }); diff --git a/src/renderer/components/notifications/RepositoryNotifications.tsx b/src/renderer/components/notifications/RepositoryNotifications.tsx index 25fa84e5e..acc62c5f7 100644 --- a/src/renderer/components/notifications/RepositoryNotifications.tsx +++ b/src/renderer/components/notifications/RepositoryNotifications.tsx @@ -4,7 +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 { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; import { HoverButton } from '../primitives/HoverButton'; import { HoverGroup } from '../primitives/HoverGroup'; @@ -51,7 +51,9 @@ export const RepositoryNotifications: FC = ({ await action(); const { failures } = useNotificationActionFailuresStore.getState(); - const hasFailure = repoNotifications.some((notification) => failures[notification.id]); + const hasFailure = repoNotifications.some( + (notification) => failures[getNotificationFailureKey(notification.account, notification.id)], + ); if (hasFailure) { setShouldAnimateRepositoryExit(false); diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 1348c8bdf..4068f7619 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -14,7 +14,13 @@ import { mockSingleAccountNotifications, } from '../__mocks__/notifications-mocks'; -import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; +import { + getNotificationFailureKey, + useAccountsStore, + useFiltersStore, + useNotificationActionFailuresStore, + useSettingsStore, +} from '../stores'; import type { AccountNotifications, Percentage } from '../types'; @@ -88,6 +94,7 @@ describe('renderer/hooks/useNotifications.ts', () => { clearServerPollIntervals(); useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + useNotificationActionFailuresStore.getState().reset(); // Reset mock notification state between tests since it's mutated mockGitifyNotification.unread = true; @@ -494,7 +501,11 @@ describe('renderer/hooks/useNotifications.ts', () => { // 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(); + const failureKey = getNotificationFailureKey( + mockGitifyNotification.account, + mockGitifyNotification.id, + ); + expect(useNotificationActionFailuresStore.getState().failures[failureKey]).toBeDefined(); }); it('tracks succeeded and failed notifications independently within a single bulk call', async () => { @@ -529,8 +540,13 @@ describe('renderer/hooks/useNotifications.ts', () => { 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(); + const failsKey = getNotificationFailureKey(failsNotification.account, failsNotification.id); + const succeedsKey = getNotificationFailureKey( + succeedsNotification.account, + succeedsNotification.id, + ); + expect(useNotificationActionFailuresStore.getState().failures[failsKey]).toBeDefined(); + expect(useNotificationActionFailuresStore.getState().failures[succeedsKey]).toBeUndefined(); }); }); @@ -581,6 +597,32 @@ describe('renderer/hooks/useNotifications.ts', () => { markAsDoneCapabilitySpy.mockRestore(); }); + + it('reconciles a failed mark-as-read fallback only once', async () => { + const markAsDoneCapabilitySpy = vi + .spyOn(githubAdapter.capabilities, 'markAsDone') + .mockReturnValue(false); + 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.markNotificationsAsDone([mockGitifyNotification]); + }); + + const failureKey = getNotificationFailureKey( + mockGitifyNotification.account, + mockGitifyNotification.id, + ); + expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1); + expect(useNotificationActionFailuresStore.getState().failures[failureKey]?.action).toBe( + 'markAsRead', + ); + + markAsDoneCapabilitySpy.mockRestore(); + }); }); describe('unsubscribeNotification', () => { @@ -636,6 +678,26 @@ describe('renderer/hooks/useNotifications.ts', () => { expect(markThreadAsDoneSpy).toHaveBeenCalledTimes(1); expect(markThreadAsReadSpy).not.toHaveBeenCalled(); }); + + it('keeps a failed follow-up action recorded after unsubscribe succeeds', async () => { + vi.spyOn(githubAdapter, 'unsubscribeThread').mockResolvedValue(undefined); + 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.unsubscribeNotification(mockGitifyNotification); + }); + + const failureKey = getNotificationFailureKey( + mockGitifyNotification.account, + mockGitifyNotification.id, + ); + expect(useNotificationActionFailuresStore.getState().failures[failureKey]).toBeDefined(); + expect(result.current.notificationCount).toBe(1); + }); }); describe('removeAccountNotifications', () => { diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 0031ac8ff..5cfd87f01 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -11,20 +11,20 @@ import { import { Constants } from '../constants'; import { - type NotificationActionFailure, type NotificationFailedActionType, + getNotificationFailureKey, useAccountsStore, useFiltersStore, useNotificationActionFailuresStore, useSettingsStore, } from '../stores'; -import { - type Account, - type AccountNotifications, - type GitifyError, - type GitifyNotification, - type Status, +import type { + Account, + AccountNotifications, + GitifyError, + GitifyNotification, + Status, } from '../types'; import { isMarkAsDoneFeatureSupported, isUnsubscribeThreadSupported } from '../utils/api/features'; @@ -38,6 +38,7 @@ import { filterDetailedNotifications, } from '../utils/notifications/filters/filter'; import { + type FailedNotificationAction, restoreFailedNotifications, settleNotificationActions, type NotificationQuerySnapshot, @@ -70,13 +71,6 @@ 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 { @@ -368,8 +362,6 @@ 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, @@ -384,11 +376,13 @@ export const useNotifications = ({ const unfilteredNotifications = queryClient.getQueryData(notificationsQueryKey) || []; - const currentNotificationIds = unfilteredNotifications.flatMap((accountNotifications) => - accountNotifications.notifications.map((notification) => notification.id), + const currentNotificationKeys = unfilteredNotifications.flatMap((accountNotifications) => + accountNotifications.notifications.map((notification) => + getNotificationFailureKey(notification.account, notification.id), + ), ); - useNotificationActionFailuresStore.getState().pruneFailures(currentNotificationIds); + useNotificationActionFailuresStore.getState().pruneFailures(currentNotificationKeys); }, [withSideEffects, notifications, queryClient, notificationsQueryKey]); // Shared by all three mutations' `onSuccess`. Records each failure's @@ -398,7 +392,7 @@ export const useNotifications = ({ // until they succeed, so this is normally a no-op). const reconcileFailedNotifications = useCallback( ( - failed: Array<{ notification: GitifyNotification; error: GitifyError; rawError: Error }>, + failed: FailedNotificationAction[], snapshot: NotificationQuerySnapshot | undefined, action: NotificationFailedActionType, context: string, @@ -418,10 +412,10 @@ export const useNotifications = ({ } for (const { notification, error, rawError } of failed) { - useNotificationActionFailuresStore.getState().setFailure(notification.id, { - action, - error, - }); + const notificationKey = getNotificationFailureKey(notification.account, notification.id); + useNotificationActionFailuresStore + .getState() + .setFailure(notificationKey, { action, error }); rendererLogError( context, `Error occurred while processing notification ${notification.id}`, @@ -449,6 +443,26 @@ export const useNotifications = ({ [queryClient], ); + const snapshotNotifications = useCallback(async () => { + // Prevent an older in-flight poll from overwriting the mutation's eventual cache update. + await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); + + const snapshot = queryClient.getQueriesData({ + queryKey: notificationsKeys.all, + }); + + return { snapshot }; + }, [queryClient]); + + const createMutationErrorHandler = useCallback( + (logContext: string, message: string) => + (err: Error, _variables: unknown, context?: { snapshot: NotificationQuerySnapshot }) => { + restoreSnapshot(context?.snapshot); + rendererLogError(logContext, message, toError(err)); + }, + [restoreSnapshot], + ); + const markNotificationsAsReadMutation = useMutation({ mutationFn: async ({ readNotifications }: { readNotifications: GitifyNotification[] }) => { return await settleNotificationActions(readNotifications, (notification) => @@ -456,15 +470,7 @@ export const useNotifications = ({ ); }, - onMutate: async () => { - await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); - - const snapshot = queryClient.getQueriesData({ - queryKey: notificationsKeys.all, - }); - - return { snapshot }; - }, + onMutate: snapshotNotifications, onSuccess: ({ succeeded, failed }, _variables, context) => { // Cache removal happens here (once the request resolves) rather than @@ -477,9 +483,13 @@ export const useNotifications = ({ ); } - for (const notification of succeeded) { - useNotificationActionFailuresStore.getState().clearFailure(notification.id); - } + useNotificationActionFailuresStore + .getState() + .clearFailures( + succeeded.map((notification) => + getNotificationFailureKey(notification.account, notification.id), + ), + ); reconcileFailedNotifications( failed, @@ -489,15 +499,10 @@ export const useNotifications = ({ ); }, - onError: (err, _variables, context) => { - restoreSnapshot(context?.snapshot); - - rendererLogError( - 'markNotificationsAsRead', - 'Error occurred while marking notifications as read', - toError(err), - ); - }, + onError: createMutationErrorHandler( + 'markNotificationsAsRead', + 'Error occurred while marking notifications as read', + ), }); const markNotificationsAsDoneMutation = useMutation({ @@ -517,15 +522,7 @@ export const useNotifications = ({ ); }, - onMutate: async () => { - await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); - - const snapshot = queryClient.getQueriesData({ - queryKey: notificationsKeys.all, - }); - - return { snapshot }; - }, + onMutate: snapshotNotifications, onSuccess: ({ succeeded, failed }, { doneNotifications }, context) => { // The mark-as-read fallback (for forges without a distinct "done" @@ -536,27 +533,28 @@ export const useNotifications = ({ ); } - for (const notification of succeeded) { - useNotificationActionFailuresStore.getState().clearFailure(notification.id); - } + useNotificationActionFailuresStore + .getState() + .clearFailures( + succeeded.map((notification) => + getNotificationFailureKey(notification.account, notification.id), + ), + ); - reconcileFailedNotifications( - failed, - context?.snapshot, - 'markAsDone', - 'markNotificationsAsDone', - ); + if (isMarkAsDoneFeatureSupported(doneNotifications[0].account)) { + reconcileFailedNotifications( + failed, + context?.snapshot, + 'markAsDone', + 'markNotificationsAsDone', + ); + } }, - onError: (err, _variables, context) => { - restoreSnapshot(context?.snapshot); - - rendererLogError( - 'markNotificationsAsDone', - 'Error occurred while marking notifications as done', - toError(err), - ); - }, + onError: createMutationErrorHandler( + 'markNotificationsAsDone', + 'Error occurred while marking notifications as done', + ), }); const unsubscribeNotificationMutation = useMutation({ @@ -575,33 +573,27 @@ export const useNotifications = ({ return result; } - if (markAsDoneOnUnsubscribe) { - await markNotificationsAsDoneMutation.mutateAsync({ - doneNotifications: [notification], - }); - } else { - await markNotificationsAsReadMutation.mutateAsync({ - readNotifications: [notification], - }); - } + const followUp = markAsDoneOnUnsubscribe + ? await markNotificationsAsDoneMutation.mutateAsync({ + doneNotifications: [notification], + }) + : await markNotificationsAsReadMutation.mutateAsync({ + readNotifications: [notification], + }); - return result; + return followUp.failed.length > 0 ? followUp : result; }, - onMutate: async () => { - await queryClient.cancelQueries({ queryKey: notificationsKeys.all }); - - const snapshot = queryClient.getQueriesData({ - queryKey: notificationsKeys.all, - }); - - return { snapshot }; - }, + onMutate: snapshotNotifications, onSuccess: ({ succeeded, failed }, _variables, context) => { - for (const notification of succeeded) { - useNotificationActionFailuresStore.getState().clearFailure(notification.id); - } + useNotificationActionFailuresStore + .getState() + .clearFailures( + succeeded.map((notification) => + getNotificationFailureKey(notification.account, notification.id), + ), + ); reconcileFailedNotifications( failed, @@ -611,15 +603,10 @@ export const useNotifications = ({ ); }, - onError: (err, _variables, context) => { - restoreSnapshot(context?.snapshot); - - rendererLogError( - 'unsubscribeNotification', - 'Error occurred while unsubscribing from notification thread', - toError(err), - ); - }, + onError: createMutationErrorHandler( + 'unsubscribeNotification', + 'Error occurred while unsubscribing from notification thread', + ), }); const markNotificationsAsRead = useCallback( @@ -659,7 +646,5 @@ export const useNotifications = ({ markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification, - - notificationFailures, }; }; diff --git a/src/renderer/stores/index.ts b/src/renderer/stores/index.ts index cfe9783d9..7fcb033af 100644 --- a/src/renderer/stores/index.ts +++ b/src/renderer/stores/index.ts @@ -3,5 +3,8 @@ 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 useNotificationActionFailuresStore, + getNotificationFailureKey, +} from './useNotificationActionFailuresStore'; export { default as useSettingsStore } from './useSettingsStore'; diff --git a/src/renderer/stores/types.ts b/src/renderer/stores/types.ts index 49c2feccb..e7ec8eeb6 100644 --- a/src/renderer/stores/types.ts +++ b/src/renderer/stores/types.ts @@ -244,18 +244,23 @@ export interface NotificationActionFailuresActions { /** * Records a failed action for a notification. */ - setFailure: (notificationId: string, failure: NotificationActionFailure) => void; + setFailure: (notificationKey: string, failure: NotificationActionFailure) => void; /** * Clears a recorded failure for a notification (e.g. after a successful retry). */ - clearFailure: (notificationId: string) => void; + clearFailure: (notificationKey: string) => void; + + /** + * Clears recorded failures for multiple notification keys. + */ + clearFailures: (notificationKeys: 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; + pruneFailures: (notificationKeys: string[]) => void; /** * Resets the store to its default (empty) state. diff --git a/src/renderer/stores/useNotificationActionFailuresStore.ts b/src/renderer/stores/useNotificationActionFailuresStore.ts index dd6dfa1c3..3cb19d77a 100644 --- a/src/renderer/stores/useNotificationActionFailuresStore.ts +++ b/src/renderer/stores/useNotificationActionFailuresStore.ts @@ -1,7 +1,14 @@ import { create } from 'zustand'; +import type { Account } from '../types'; import type { NotificationActionFailuresStore } from './types'; +import { getAccountUUID } from '../utils/auth/utils'; + +export function getNotificationFailureKey(account: Account, notificationId: string): string { + return `${getAccountUUID(account)}:${notificationId}`; +} + /** * Gitify Notification Action Failures store. * @@ -14,27 +21,41 @@ import type { NotificationActionFailuresStore } from './types'; const useNotificationActionFailuresStore = create((set, get) => ({ failures: {}, - setFailure: (notificationId, failure) => { - set((state) => ({ failures: { ...state.failures, [notificationId]: failure } })); + setFailure: (notificationKey, failure) => { + set((state) => ({ failures: { ...state.failures, [notificationKey]: failure } })); }, - clearFailure: (notificationId) => { + clearFailure: (notificationKey) => { const { failures } = get(); - if (!(notificationId in failures)) { + if (!(notificationKey in failures)) { return; } const nextFailures = { ...failures }; - delete nextFailures[notificationId]; + delete nextFailures[notificationKey]; set({ failures: nextFailures }); }, - pruneFailures: (notificationIds) => { + clearFailures: (notificationKeys) => { + const { failures } = get(); + const keysToClear = new Set(notificationKeys); + const remainingEntries = Object.entries(failures).filter( + ([notificationKey]) => !keysToClear.has(notificationKey), + ); + + if (remainingEntries.length === Object.keys(failures).length) { + return; + } + + set({ failures: Object.fromEntries(remainingEntries) }); + }, + + pruneFailures: (notificationKeys) => { const { failures } = get(); - const idsToKeep = new Set(notificationIds); + const keysToKeep = new Set(notificationKeys); - const remainingEntries = Object.entries(failures).filter(([notificationId]) => - idsToKeep.has(notificationId), + const remainingEntries = Object.entries(failures).filter(([notificationKey]) => + keysToKeep.has(notificationKey), ); if (remainingEntries.length === Object.keys(failures).length) { diff --git a/src/renderer/utils/core/errors.ts b/src/renderer/utils/core/errors.ts index b3a7b5bc2..fcc278b0e 100644 --- a/src/renderer/utils/core/errors.ts +++ b/src/renderer/utils/core/errors.ts @@ -7,7 +7,7 @@ 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.'], + descriptions: ['GitHub rejected this request for this account via Gitify.'], emojis: Constants.EMOJIS.ERRORS.ACTION_FORBIDDEN, }, BAD_CREDENTIALS: { diff --git a/src/renderer/utils/notifications/mutations.test.ts b/src/renderer/utils/notifications/mutations.test.ts index cd39df952..460c433b9 100644 --- a/src/renderer/utils/notifications/mutations.test.ts +++ b/src/renderer/utils/notifications/mutations.test.ts @@ -122,5 +122,22 @@ describe('renderer/utils/notifications/mutations.ts', () => { expect(result[0].notifications).toEqual([second]); expect(result[1].notifications).toEqual(mockGithubEnterpriseGitifyNotifications); }); + + it('preserves notifications added to current after the snapshot was taken', () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + const concurrent = { ...first, id: 'concurrent-notification' }; + const account = first.account; + + const snapshot: AccountNotifications[] = [ + { account, notifications: [first, second], error: null }, + ]; + const current: AccountNotifications[] = [ + { account, notifications: [first, concurrent], error: null }, + ]; + + const result = restoreFailedNotifications([second], snapshot, current); + + expect(result[0].notifications).toEqual([first, concurrent, second]); + }); }); }); diff --git a/src/renderer/utils/notifications/mutations.ts b/src/renderer/utils/notifications/mutations.ts index cf7a9f262..7d51b15b4 100644 --- a/src/renderer/utils/notifications/mutations.ts +++ b/src/renderer/utils/notifications/mutations.ts @@ -121,10 +121,17 @@ export function restoreFailedNotifications( const currentIds = new Set(accountEntry.notifications.map((notification) => notification.id)); - const restoredNotifications = snapshotEntry.notifications.filter( - (notification) => currentIds.has(notification.id) || failedIds.has(notification.id), + const missingNotifications = snapshotEntry.notifications.filter( + (notification) => failedIds.has(notification.id) && !currentIds.has(notification.id), ); - return { ...accountEntry, notifications: restoredNotifications }; + if (missingNotifications.length === 0) { + return accountEntry; + } + + return { + ...accountEntry, + notifications: [...accountEntry.notifications, ...missingNotifications], + }; }); }