From 8a4f5bc3eb156d79e3184dee38f4b1c63785e6cb Mon Sep 17 00:00:00 2001 From: Filipe Guerreiro Date: Thu, 17 Sep 2026 15:49:42 +0900 Subject: [PATCH 1/4] perf: virtualize the notification list One repository can hold hundreds of notifications, so accounts, repository groups and rows are flattened into a single item sequence and windowed across group boundaries. Headers no longer own their rows, so collapse and exit-animation state moves up to the list and survives rows unmounting on scroll. --- package.json | 1 + pnpm-lock.yaml | 3 + src/renderer/components/layout/Contents.tsx | 5 +- .../notifications/AccountHeader.test.tsx | 109 + .../notifications/AccountHeader.tsx | 92 + .../AccountNotifications.test.tsx | 208 -- .../notifications/AccountNotifications.tsx | 150 -- .../notifications/NotificationList.test.tsx | 164 ++ .../notifications/NotificationList.tsx | 248 ++ ...ons.test.tsx => RepositoryHeader.test.tsx} | 132 +- .../notifications/RepositoryHeader.tsx | 132 ++ .../notifications/RepositoryNotifications.tsx | 153 -- .../__snapshots__/AccountHeader.test.tsx.snap | 193 ++ .../AccountNotifications.test.tsx.snap | 2063 ----------------- ...sx.snap => RepositoryHeader.test.tsx.snap} | 227 +- src/renderer/routes/Notifications.test.tsx | 6 +- src/renderer/routes/Notifications.tsx | 20 +- .../__snapshots__/Notifications.test.tsx.snap | 22 +- 18 files changed, 1035 insertions(+), 2893 deletions(-) create mode 100644 src/renderer/components/notifications/AccountHeader.test.tsx create mode 100644 src/renderer/components/notifications/AccountHeader.tsx delete mode 100644 src/renderer/components/notifications/AccountNotifications.test.tsx delete mode 100644 src/renderer/components/notifications/AccountNotifications.tsx create mode 100644 src/renderer/components/notifications/NotificationList.test.tsx create mode 100644 src/renderer/components/notifications/NotificationList.tsx rename src/renderer/components/notifications/{RepositoryNotifications.test.tsx => RepositoryHeader.test.tsx} (52%) create mode 100644 src/renderer/components/notifications/RepositoryHeader.tsx delete mode 100644 src/renderer/components/notifications/RepositoryNotifications.tsx create mode 100644 src/renderer/components/notifications/__snapshots__/AccountHeader.test.tsx.snap delete mode 100644 src/renderer/components/notifications/__snapshots__/AccountNotifications.test.tsx.snap rename src/renderer/components/notifications/__snapshots__/{RepositoryNotifications.test.tsx.snap => RepositoryHeader.test.tsx.snap} (69%) diff --git a/package.json b/package.json index 4b5ced6c8..4375a2f1a 100644 --- a/package.json +++ b/package.json @@ -106,6 +106,7 @@ "@primer/react": "38.39.0", "@tailwindcss/vite": "4.3.3", "@tanstack/react-query": "5.102.8", + "@tanstack/react-virtual": "3.14.13", "@testing-library/jest-dom": "7.0.1", "@testing-library/react": "16.3.3", "@testing-library/user-event": "14.6.7", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8f786507e..a2c200beb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -283,6 +283,9 @@ importers: '@tanstack/react-query': specifier: 5.102.8 version: 5.102.8(react@19.3.0) + '@tanstack/react-virtual': + specifier: 3.14.13 + version: 3.14.13(react-dom@19.3.0(react@19.3.0))(react@19.3.0) '@testing-library/jest-dom': specifier: 7.0.1 version: 7.0.1(@testing-library/dom@10.4.2)(vitest@4.1.11) diff --git a/src/renderer/components/layout/Contents.tsx b/src/renderer/components/layout/Contents.tsx index 458a59a53..daa71bb5f 100644 --- a/src/renderer/components/layout/Contents.tsx +++ b/src/renderer/components/layout/Contents.tsx @@ -1,4 +1,4 @@ -import type { FC, ReactNode } from 'react'; +import type { FC, ReactNode, Ref } from 'react'; import { cn } from 'cn'; @@ -7,6 +7,7 @@ interface IContents { paddingHorizontal?: boolean; paddingBottom?: boolean; scrollFade?: boolean; + ref?: Ref; } /** @@ -18,6 +19,7 @@ export const Contents: FC = ({ paddingHorizontal = true, paddingBottom = false, scrollFade = false, + ref, }) => { return (
= ({ paddingBottom && 'pb-2', scrollFade && 'gitify-scroll-fade', )} + ref={ref} > {children}
diff --git a/src/renderer/components/notifications/AccountHeader.test.tsx b/src/renderer/components/notifications/AccountHeader.test.tsx new file mode 100644 index 000000000..e6facd4fa --- /dev/null +++ b/src/renderer/components/notifications/AccountHeader.test.tsx @@ -0,0 +1,109 @@ +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { renderWithProviders } from '../../__helpers__/test-utils'; +import { mockGitHubCloudAccount } from '../../__mocks__/account-mocks'; + +import * as links from '../../utils/system/links'; +import { AccountHeader, type AccountHeaderProps } from './AccountHeader'; + +describe('renderer/components/notifications/AccountHeader.tsx', () => { + const props: AccountHeaderProps = { + account: mockGitHubCloudAccount, + error: null, + notificationCount: 3, + isCollapsed: false, + onToggle: vi.fn(), + }; + + it('renders the managed GitHub account identity', () => { + renderWithProviders( + , + ); + + expect(screen.getByText('octocat_gitify')).toBeInTheDocument(); + expect(screen.getByAltText('octocat_gitify')).toBeInTheDocument(); + }); + + it('should open profile when clicked', async () => { + const openAccountProfileSpy = vi.spyOn(links, 'openAccountProfile').mockImplementation(vi.fn()); + const onToggle = vi.fn(); + + renderWithProviders(); + + await userEvent.click(screen.getByTestId('account-profile')); + + expect(openAccountProfileSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); + // The header's own click toggles collapse; the profile button must not. + expect(onToggle).not.toHaveBeenCalled(); + }); + + it('should open my issues when clicked', async () => { + const openHostIssuesSpy = vi.spyOn(links, 'openHostIssues').mockImplementation(vi.fn()); + + renderWithProviders(); + + await userEvent.click(screen.getByTestId('account-issues')); + + expect(openHostIssuesSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); + }); + + it('should open my pull requests when clicked', async () => { + const openHostPullsSpy = vi.spyOn(links, 'openHostPulls').mockImplementation(vi.fn()); + + renderWithProviders(); + + await userEvent.click(screen.getByTestId('account-pull-requests')); + + expect(openHostPullsSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); + }); + + it('should request a collapse toggle when toggled', async () => { + const onToggle = vi.fn(); + + renderWithProviders(); + + await userEvent.click(screen.getByTestId('account-toggle')); + + expect(onToggle).toHaveBeenCalledTimes(1); + }); + + it('should label the toggle by collapsed state', () => { + const { unmount } = renderWithProviders()!; + + expect(screen.getByTestId('account-toggle')).toHaveAttribute( + 'title', + 'Hide account notifications', + ); + + unmount(); + renderWithProviders(); + + expect(screen.getByTestId('account-toggle')).toHaveAttribute( + 'title', + 'Show account notifications', + ); + }); + + it('should render an error background when the account errored', () => { + const tree = renderWithProviders( + , + ); + + expect(tree!.container).toMatchSnapshot(); + }); +}); diff --git a/src/renderer/components/notifications/AccountHeader.tsx b/src/renderer/components/notifications/AccountHeader.tsx new file mode 100644 index 000000000..ff5ef936d --- /dev/null +++ b/src/renderer/components/notifications/AccountHeader.tsx @@ -0,0 +1,92 @@ +import { type FC, type MouseEvent } from 'react'; + +import { GitPullRequestIcon, IssueOpenedIcon } from '@primer/octicons-react'; +import { Button, Stack } from '@primer/react'; + +import { cn } from 'cn'; + +import { HoverButton } from '../primitives/HoverButton'; +import { HoverGroup } from '../primitives/HoverGroup'; + +import { type Account, type GitifyError, Size } from '../../types'; + +import { getAdapter } from '../../utils/forges/registry'; +import { openAccountProfile, openHostIssues, openHostPulls } from '../../utils/system/links'; +import { getChevronDetails } from '../../utils/ui/display'; +import { AvatarWithFallback } from '../avatars/AvatarWithFallback'; + +export interface AccountHeaderProps { + account: Account; + error: GitifyError | null; + notificationCount: number; + isCollapsed: boolean; + onToggle: () => void; +} + +export const AccountHeader: FC = ({ + account, + error, + notificationCount, + isCollapsed, + onToggle, +}) => { + const Chevron = getChevronDetails(notificationCount > 0, !isCollapsed, 'account'); + + return ( + + + + + openHostIssues(account)} + icon={IssueOpenedIcon} + label="My issues ↗" + testid="account-issues" + /> + + openHostPulls(account)} + icon={GitPullRequestIcon} + label="My pull requests ↗" + testid="account-pull-requests" + /> + + + + + ); +}; diff --git a/src/renderer/components/notifications/AccountNotifications.test.tsx b/src/renderer/components/notifications/AccountNotifications.test.tsx deleted file mode 100644 index b7f12fa50..000000000 --- a/src/renderer/components/notifications/AccountNotifications.test.tsx +++ /dev/null @@ -1,208 +0,0 @@ -import { act, screen } from '@testing-library/react'; -import userEvent from '@testing-library/user-event'; - -import { renderWithProviders } from '../../__helpers__/test-utils'; -import { - mockGitHubCloudAccount, - mockGitHubEnterpriseServerAccount, -} from '../../__mocks__/account-mocks'; -import { mockGitHubCloudGitifyNotifications } from '../../__mocks__/notifications-mocks'; -import { mockSettings } from '../../__mocks__/state-mocks'; - -import { GroupBy } from '../../types'; - -import * as links from '../../utils/system/links'; -import { AccountNotifications, type AccountNotificationsProps } from './AccountNotifications'; - -vi.mock('./RepositoryNotifications', () => ({ - RepositoryNotifications: () =>
RepositoryNotifications
, -})); - -describe('renderer/components/notifications/AccountNotifications.tsx', () => { - it('renders the managed GitHub account identity in the account header', () => { - const account = { - ...mockGitHubCloudAccount, - user: { - ...mockGitHubCloudAccount.user!, - login: 'octocat_gitify', - name: 'Mona Lisa Octocat', - }, - }; - - renderWithProviders( - , - ); - - expect(screen.getByText('octocat_gitify')).toBeInTheDocument(); - expect(screen.getByAltText('octocat_gitify')).toBeInTheDocument(); - }); - - it('should render itself - group notifications by repositories', () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: mockGitHubCloudGitifyNotifications, - showAccountHeader: true, - error: null, - }; - - const tree = renderWithProviders(, { - settings: { ...mockSettings, groupBy: GroupBy.REPOSITORY }, - }); - - expect(tree!.container).toMatchSnapshot(); - }); - - it('should render itself - group notifications by date', () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: mockGitHubCloudGitifyNotifications, - showAccountHeader: true, - error: null, - }; - - const tree = renderWithProviders(, { - settings: { ...mockSettings, groupBy: GroupBy.DATE }, - }); - - expect(tree!.container).toMatchSnapshot(); - }); - - it('should render itself - no notifications', async () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - showAccountHeader: true, - error: null, - }; - - let tree: ReturnType | null = null; - - await act(async () => { - tree = renderWithProviders(); - }); - - expect(tree!.container).toMatchSnapshot(); - }); - - it('should render itself - account error for single account', async () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - error: { - title: 'Error title', - descriptions: ['Error description'], - emojis: ['🔥'], - }, - showAccountHeader: true, - }; - - let tree: ReturnType | null = null; - - await act(async () => { - tree = renderWithProviders(, { - accounts: [mockGitHubCloudAccount], - }); - }); - - expect(tree!.container).toMatchSnapshot(); - }); - - it('should render itself - account error for multiple accounts', async () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - error: { - title: 'Error title', - descriptions: ['Error description'], - emojis: ['🔥'], - }, - showAccountHeader: true, - }; - - let tree: ReturnType | null = null; - - await act(async () => { - tree = renderWithProviders(, { - accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], - }); - }); - - expect(tree!.container).toMatchSnapshot(); - }); - - it('should open profile when clicked', async () => { - const openAccountProfileSpy = vi.spyOn(links, 'openAccountProfile').mockImplementation(vi.fn()); - - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - showAccountHeader: true, - error: null, - }; - - renderWithProviders(); - - await userEvent.click(screen.getByTestId('account-profile')); - - expect(openAccountProfileSpy).toHaveBeenCalledTimes(1); - expect(openAccountProfileSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); - }); - - it('should open my issues when clicked', async () => { - const openHostIssuesSpy = vi.spyOn(links, 'openHostIssues').mockImplementation(vi.fn()); - - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - showAccountHeader: true, - error: null, - }; - - renderWithProviders(); - - await userEvent.click(screen.getByTestId('account-issues')); - - expect(openHostIssuesSpy).toHaveBeenCalledTimes(1); - expect(openHostIssuesSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); - }); - - it('should open my pull requests when clicked', async () => { - const openHostPullsSpy = vi.spyOn(links, 'openHostPulls').mockImplementation(vi.fn()); - - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: [], - showAccountHeader: true, - error: null, - }; - - renderWithProviders(); - - await userEvent.click(screen.getByTestId('account-pull-requests')); - - expect(openHostPullsSpy).toHaveBeenCalledTimes(1); - expect(openHostPullsSpy).toHaveBeenCalledWith(mockGitHubCloudAccount); - }); - - it('should toggle account notifications visibility', async () => { - const props: AccountNotificationsProps = { - account: mockGitHubCloudAccount, - notifications: mockGitHubCloudGitifyNotifications, - showAccountHeader: true, - error: null, - }; - - renderWithProviders(); - - await userEvent.click(screen.getByTestId('account-toggle')); - - const tree = renderWithProviders(); - - expect(tree!.container).toMatchSnapshot(); - }); -}); diff --git a/src/renderer/components/notifications/AccountNotifications.tsx b/src/renderer/components/notifications/AccountNotifications.tsx deleted file mode 100644 index 17dbf44a4..000000000 --- a/src/renderer/components/notifications/AccountNotifications.tsx +++ /dev/null @@ -1,150 +0,0 @@ -import { type FC, type MouseEvent, useMemo, useState } from 'react'; - -import { GitPullRequestIcon, IssueOpenedIcon } from '@primer/octicons-react'; -import { Button, Stack } from '@primer/react'; - -import { cn } from 'cn'; - -import { useAccountsStore } from '../../stores'; - -import { HoverButton } from '../primitives/HoverButton'; -import { HoverGroup } from '../primitives/HoverGroup'; - -import { type Account, type GitifyError, type GitifyNotification, Size } from '../../types'; - -import { getAdapter } from '../../utils/forges/registry'; -import { - groupNotificationsByRepository, - isGroupByRepository, -} from '../../utils/notifications/group'; -import { openAccountProfile, openHostIssues, openHostPulls } from '../../utils/system/links'; -import { getChevronDetails } from '../../utils/ui/display'; -import { AllRead } from '../AllRead'; -import { AvatarWithFallback } from '../avatars/AvatarWithFallback'; -import { Oops } from '../Oops'; -import { NotificationRow } from './NotificationRow'; -import { RepositoryNotifications } from './RepositoryNotifications'; - -export interface AccountNotificationsProps { - account: Account; - notifications: GitifyNotification[]; - error: GitifyError | null; - showAccountHeader: boolean; -} - -export const AccountNotifications: FC = ( - props: AccountNotificationsProps, -) => { - const { account, showAccountHeader, notifications } = props; - - const hasMultipleAccounts = useAccountsStore((s) => s.hasMultipleAccounts()); - - const [isAccountNotificationsVisible, setIsAccountNotificationsVisible] = useState(true); - - const sortedNotifications = useMemo( - () => [...notifications].sort((a, b) => a.order - b.order), - [notifications], - ); - - const groupedNotifications = useMemo(() => { - const map = groupNotificationsByRepository(sortedNotifications); - - return Array.from(map.entries()); - }, [sortedNotifications]); - - const hasNotifications = useMemo(() => notifications.length > 0, [notifications]); - - const actionToggleAccountNotifications = () => { - setIsAccountNotificationsVisible(!isAccountNotificationsVisible); - }; - - const Chevron = getChevronDetails(hasNotifications, isAccountNotificationsVisible, 'account'); - - return ( - <> - {showAccountHeader && ( - - - - - openHostIssues(account)} - icon={IssueOpenedIcon} - label="My issues ↗" - testid="account-issues" - /> - - openHostPulls(account)} - icon={GitPullRequestIcon} - label="My pull requests ↗" - testid="account-pull-requests" - /> - - - - - )} - - {isAccountNotificationsVisible && ( - <> - {props.error && } - - {!hasNotifications && !props.error && } - - {isGroupByRepository() - ? groupedNotifications.map(([repoSlug, repoNotifications]) => ( - - )) - : sortedNotifications.map((notification) => ( - - ))} - - )} - - ); -}; diff --git a/src/renderer/components/notifications/NotificationList.test.tsx b/src/renderer/components/notifications/NotificationList.test.tsx new file mode 100644 index 000000000..b1ff0a7a5 --- /dev/null +++ b/src/renderer/components/notifications/NotificationList.test.tsx @@ -0,0 +1,164 @@ +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { renderWithProviders } from '../../__helpers__/test-utils'; +import { + mockGitHubCloudAccount, + mockGitHubEnterpriseServerAccount, +} from '../../__mocks__/account-mocks'; +import { + mockGitHubCloudGitifyNotifications, + mockGitifyNotification, +} from '../../__mocks__/notifications-mocks'; +import { mockSettings } from '../../__mocks__/state-mocks'; + +import { type AccountNotifications, GroupBy } from '../../types'; + +import { NotificationList } from './NotificationList'; + +const VIEWPORT_HEIGHT = 300; +const ITEM_HEIGHT = 50; + +// happy-dom reports `offsetHeight` as 0, leaving the virtualizer a 0-height +// viewport that renders nothing. +beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { + configurable: true, + get(this: HTMLElement) { + return this.dataset.index === undefined ? VIEWPORT_HEIGHT : ITEM_HEIGHT; + }, + }); +}); + +afterAll(() => { + Reflect.deleteProperty(HTMLElement.prototype, 'offsetHeight'); +}); + +const singleAccount: AccountNotifications[] = [ + { + account: mockGitHubCloudAccount, + notifications: mockGitHubCloudGitifyNotifications, + error: null, + }, +]; + +const mountedRows = (container: HTMLElement) => container.querySelectorAll('[data-index]').length; + +const hasRow = (container: HTMLElement, notificationId: string) => + container.querySelector(`[id="${notificationId}"]`) !== null; + +describe('renderer/components/notifications/NotificationList.tsx', () => { + it('renders a repository header and its notifications when grouping by repository', () => { + const tree = renderWithProviders( + , + { settings: { ...mockSettings, groupBy: GroupBy.REPOSITORY } }, + ); + + expect(screen.getByTestId('open-repository')).toBeInTheDocument(); + expect(hasRow(tree.container, mockGitifyNotification.id)).toBe(true); + }); + + it('renders notifications without repository headers when grouping by date', () => { + const tree = renderWithProviders( + , + { settings: { ...mockSettings, groupBy: GroupBy.DATE } }, + ); + + expect(screen.queryByTestId('open-repository')).not.toBeInTheDocument(); + expect(hasRow(tree.container, mockGitifyNotification.id)).toBe(true); + }); + + it('renders the account header only when asked to', () => { + const { unmount } = renderWithProviders( + , + ); + + expect(screen.queryByTestId('account-profile')).not.toBeInTheDocument(); + + unmount(); + renderWithProviders( + , + ); + + expect(screen.getByTestId('account-profile')).toBeInTheDocument(); + }); + + it('hides an account\u2019s notifications when its header is collapsed', async () => { + const tree = renderWithProviders( + , + { settings: { ...mockSettings, groupBy: GroupBy.DATE } }, + ); + + await userEvent.click(screen.getByTestId('account-toggle')); + + expect(hasRow(tree.container, mockGitifyNotification.id)).toBe(false); + expect(screen.getByTestId('account-profile')).toBeInTheDocument(); + }); + + it('hides a repository\u2019s notifications when its header is collapsed', async () => { + const tree = renderWithProviders( + , + { settings: { ...mockSettings, groupBy: GroupBy.REPOSITORY } }, + ); + + await userEvent.click(screen.getByTestId('repository-toggle')); + + expect(hasRow(tree.container, mockGitifyNotification.id)).toBe(false); + expect(screen.getByTestId('open-repository')).toBeInTheDocument(); + }); + + it('renders an account error instead of its notifications', () => { + renderWithProviders( + , + ); + + expect(screen.getByText('Error title')).toBeInTheDocument(); + }); + + it('renders every account in turn', () => { + renderWithProviders( + , + ); + + expect(screen.getAllByTestId('account-profile')).toHaveLength(2); + }); + + it('mounts only the notifications within the scroll window', () => { + const notifications = Array.from({ length: 500 }, (_, index) => ({ + ...mockGitifyNotification, + id: `notification-${index}`, + order: index, + })); + + const tree = renderWithProviders( + , + { settings: { ...mockSettings, groupBy: GroupBy.DATE } }, + ); + + // A 300px window over 50px rows, plus overscan either side - not 500 rows. + expect(mountedRows(tree.container)).toBeLessThan(30); + expect(mountedRows(tree.container)).toBeGreaterThan(0); + }); +}); diff --git a/src/renderer/components/notifications/NotificationList.tsx b/src/renderer/components/notifications/NotificationList.tsx new file mode 100644 index 000000000..15341d88d --- /dev/null +++ b/src/renderer/components/notifications/NotificationList.tsx @@ -0,0 +1,248 @@ +import { type FC, useCallback, useMemo, useState } from 'react'; + +import { useVirtualizer } from '@tanstack/react-virtual'; + +import { useAccountsStore, useSettingsStore } from '../../stores'; + +import { Contents } from '../layout/Contents'; + +import type { Account, AccountNotifications, GitifyError, GitifyNotification } from '../../types'; + +import { getAccountUUID } from '../../utils/auth/utils'; +import { groupNotificationsByRepository } from '../../utils/notifications/group'; +import { AllRead } from '../AllRead'; +import { Oops } from '../Oops'; +import { AccountHeader } from './AccountHeader'; +import { NotificationRow } from './NotificationRow'; +import { RepositoryHeader } from './RepositoryHeader'; + +type ListItem = + | { key: string; kind: 'account'; account: Account; error: GitifyError | null; count: number } + | { key: string; kind: 'error'; error: GitifyError; fullHeight: boolean } + | { key: string; kind: 'all-read' } + | { + key: string; + kind: 'repository'; + repoKey: string; + repoName: string; + notifications: GitifyNotification[]; + } + | { + key: string; + kind: 'notification'; + notification: GitifyNotification; + isRepositoryAnimatingExit: boolean; + }; + +const ESTIMATED_HEIGHT = { notification: 58, header: 36 }; + +export interface NotificationListProps { + accountNotifications: AccountNotifications[]; + showAccountHeader: boolean; +} + +/** + * Flattened so one virtualizer windows across group boundaries, and so collapse + * and exit-animation state survives rows unmounting on scroll. + */ +export const NotificationList: FC = ({ + accountNotifications, + showAccountHeader, +}) => { + const groupBy = useSettingsStore((s) => s.groupBy); + const hasMultipleAccounts = useAccountsStore((s) => s.hasMultipleAccounts()); + + const [scrollElement, setScrollElement] = useState(null); + const [collapsedAccounts, setCollapsedAccounts] = useState>(new Set()); + const [collapsedRepositories, setCollapsedRepositories] = useState>( + new Set(), + ); + const [animatingRepositories, setAnimatingRepositories] = useState>( + new Set(), + ); + + const toggleCollapsedAccount = useCallback((accountUUID: string) => { + setCollapsedAccounts((current) => { + const next = new Set(current); + + if (!next.delete(accountUUID)) { + next.add(accountUUID); + } + + return next; + }); + }, []); + + const toggleCollapsedRepository = useCallback((repoKey: string) => { + setCollapsedRepositories((current) => { + const next = new Set(current); + + if (!next.delete(repoKey)) { + next.add(repoKey); + } + + return next; + }); + }, []); + + const setRepositoryAnimatingExit = useCallback((repoKey: string, animate: boolean) => { + setAnimatingRepositories((current) => { + const next = new Set(current); + + if (animate) { + next.add(repoKey); + } else { + next.delete(repoKey); + } + + return next; + }); + }, []); + + const items = useMemo(() => { + const list: ListItem[] = []; + + for (const { account, error, notifications } of accountNotifications) { + const accountUUID = getAccountUUID(account); + + if (showAccountHeader) { + list.push({ + key: `account-${accountUUID}`, + kind: 'account', + account, + error, + count: notifications.length, + }); + } + + if (collapsedAccounts.has(accountUUID)) { + continue; + } + + if (error) { + list.push({ + key: `error-${accountUUID}`, + kind: 'error', + error, + fullHeight: !hasMultipleAccounts, + }); + } else if (notifications.length === 0) { + list.push({ key: `all-read-${accountUUID}`, kind: 'all-read' }); + } + + const sorted = [...notifications].sort((a, b) => a.order - b.order); + + if (groupBy !== 'REPOSITORY') { + for (const notification of sorted) { + list.push({ + key: `notification-${notification.id}`, + kind: 'notification', + notification, + isRepositoryAnimatingExit: false, + }); + } + + continue; + } + + for (const [repoName, repoNotifications] of groupNotificationsByRepository(sorted)) { + const repoKey = `${accountUUID}-${repoName}`; + + list.push({ + key: `repository-${repoKey}`, + kind: 'repository', + repoKey, + repoName, + notifications: repoNotifications, + }); + + if (collapsedRepositories.has(repoKey)) { + continue; + } + + for (const notification of repoNotifications) { + list.push({ + key: `notification-${notification.id}`, + kind: 'notification', + notification, + isRepositoryAnimatingExit: animatingRepositories.has(repoKey), + }); + } + } + } + + return list; + }, [ + accountNotifications, + animatingRepositories, + collapsedAccounts, + collapsedRepositories, + groupBy, + hasMultipleAccounts, + showAccountHeader, + ]); + + // oxlint-disable-next-line react/incompatible-library -- Its values are only read in this component's own JSX, never passed to a memoized child + const virtualizer = useVirtualizer({ + count: items.length, + getScrollElement: () => scrollElement, + estimateSize: (index) => + items[index].kind === 'notification' + ? ESTIMATED_HEIGHT.notification + : ESTIMATED_HEIGHT.header, + getItemKey: (index) => items[index].key, + overscan: 8, + }); + + return ( + +
+ {virtualizer.getVirtualItems().map((virtualItem) => { + const item = items[virtualItem.index]; + + return ( +
+ {item.kind === 'account' && ( + toggleCollapsedAccount(getAccountUUID(item.account))} + /> + )} + + {item.kind === 'error' && } + + {item.kind === 'all-read' && } + + {item.kind === 'repository' && ( + setRepositoryAnimatingExit(item.repoKey, animate)} + onToggle={() => toggleCollapsedRepository(item.repoKey)} + repoName={item.repoName} + repoNotifications={item.notifications} + /> + )} + + {item.kind === 'notification' && ( + + )} +
+ ); + })} +
+ + ); +}; diff --git a/src/renderer/components/notifications/RepositoryNotifications.test.tsx b/src/renderer/components/notifications/RepositoryHeader.test.tsx similarity index 52% rename from src/renderer/components/notifications/RepositoryNotifications.test.tsx rename to src/renderer/components/notifications/RepositoryHeader.test.tsx index 121edc216..d20541df4 100644 --- a/src/renderer/components/notifications/RepositoryNotifications.test.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.test.tsx @@ -1,4 +1,4 @@ -import { act, screen } from '@testing-library/react'; +import { screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { renderWithProviders } from '../../__helpers__/test-utils'; @@ -10,53 +10,42 @@ import { getNotificationFailureKey, useNotificationActionFailuresStore } from '. import type { Link } from '../../types'; import * as comms from '../../utils/system/comms'; -import { - RepositoryNotifications, - type RepositoryNotificationsProps, -} from './RepositoryNotifications'; - -vi.mock('./NotificationRow', () => ({ - NotificationRow: () =>
NotificationRow
, -})); - -describe('renderer/components/notifications/RepositoryNotifications.tsx', () => { - const markNotificationsAsReadMock = vi.fn(); - const markNotificationsAsDoneMock = vi.fn(); +import { RepositoryHeader, type RepositoryHeaderProps } from './RepositoryHeader'; + +describe('renderer/components/notifications/RepositoryHeader.tsx', () => { + const props: RepositoryHeaderProps = { + repoName: 'gitify-app/notifications-test', + repoNotifications: mockGitHubCloudGitifyNotifications, + isCollapsed: false, + isAnimatingExit: false, + onToggle: vi.fn(), + onAnimateExit: vi.fn(), + }; it('should render itself & its children', () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; - - const tree = renderWithProviders(); + const tree = renderWithProviders(); expect(tree.container).toMatchSnapshot(); }); it('should render itself & its children - all notifications are read', () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications.map((n) => ({ - ...n, - unread: false, - })), - }; - - const tree = renderWithProviders(); + const tree = renderWithProviders( + ({ + ...n, + unread: false, + }))} + />, + ); expect(tree.container).toMatchSnapshot(); }); it('should open the browser when clicking on the repo name', async () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; - const openExternalLinkSpy = vi.spyOn(comms, 'openExternalLink').mockImplementation(vi.fn()); - renderWithProviders(); + renderWithProviders(); await userEvent.click(screen.getByTestId('open-repository')); @@ -67,12 +56,9 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => }); it('should mark a repo as read', async () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; + const markNotificationsAsReadMock = vi.fn(); - renderWithProviders(, { + renderWithProviders(, { settings: { ...mockSettings }, markNotificationsAsRead: markNotificationsAsReadMock, }); @@ -83,12 +69,9 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => }); it('should mark a repo as done', async () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; + const markNotificationsAsDoneMock = vi.fn(); - renderWithProviders(, { + renderWithProviders(, { settings: { ...mockSettings }, markNotificationsAsDone: markNotificationsAsDoneMock, }); @@ -99,32 +82,46 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => }); it('should use default repository icon when avatar is not available', () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; + const repoNotifications = mockGitHubCloudGitifyNotifications.map((n) => ({ + ...n, + repository: { ...n.repository, owner: { ...n.repository.owner, avatarUrl: '' as Link } }, + })); - props.repoNotifications[0].repository.owner.avatarUrl = '' as Link; - - const tree = renderWithProviders(); + const tree = renderWithProviders( + , + ); expect(tree.container).toMatchSnapshot(); }); - it('should toggle repository notifications visibility', async () => { - const props: RepositoryNotificationsProps = { - repoName: 'gitify-app/notifications-test', - repoNotifications: mockGitHubCloudGitifyNotifications, - }; + it('should request a collapse toggle when toggled', async () => { + const onToggle = vi.fn(); - await act(async () => { - renderWithProviders(); - }); + renderWithProviders(); await userEvent.click(screen.getByTestId('repository-toggle')); - const tree = renderWithProviders(); - expect(tree.container).toMatchSnapshot(); + expect(onToggle).toHaveBeenCalledTimes(1); + }); + + it('should hide its hover actions while animating out', () => { + renderWithProviders(); + + expect(screen.queryByTestId('repository-mark-as-read')).not.toBeInTheDocument(); + expect(screen.queryByTestId('repository-toggle')).not.toBeInTheDocument(); + }); + + it('starts the group exit animation when marking the repository', async () => { + const onAnimateExit = vi.fn(); + + renderWithProviders(, { + settings: { ...mockSettings }, + markNotificationsAsRead: vi.fn(), + }); + + await userEvent.click(screen.getByTestId('repository-mark-as-read')); + + expect(onAnimateExit.mock.calls).toEqual([[true]]); }); describe('partial bulk failure', () => { @@ -133,12 +130,8 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => }); 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; + const onAnimateExit = vi.fn(); // Simulate the mutation reconciliation that records a failure in the // real (non-mocked) failure store, since `runGroupAction` reads @@ -155,17 +148,14 @@ describe('renderer/components/notifications/RepositoryNotifications.tsx', () => }); }); - renderWithProviders(, { + 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(); + expect(onAnimateExit.mock.calls).toEqual([[true], [false]]); }); }); }); diff --git a/src/renderer/components/notifications/RepositoryHeader.tsx b/src/renderer/components/notifications/RepositoryHeader.tsx new file mode 100644 index 000000000..95c9b86a7 --- /dev/null +++ b/src/renderer/components/notifications/RepositoryHeader.tsx @@ -0,0 +1,132 @@ +import { type FC, type MouseEvent } from 'react'; + +import { CheckIcon, ReadIcon } from '@primer/octicons-react'; +import { Button, Stack } from '@primer/react'; + +import { cn } from 'cn'; + +import { useNotifications } from '../../hooks/useNotifications'; +import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; + +import { HoverButton } from '../primitives/HoverButton'; +import { HoverGroup } from '../primitives/HoverGroup'; + +import { type GitifyNotification, Opacity, Size } from '../../types'; + +import { isMarkAsDoneFeatureSupported } from '../../utils/api/features'; +import { shouldRemoveNotificationsFromState } from '../../utils/notifications/remove'; +import { openRepository } from '../../utils/system/links'; +import { getChevronDetails } from '../../utils/ui/display'; +import { AvatarWithFallback } from '../avatars/AvatarWithFallback'; + +export interface RepositoryHeaderProps { + repoNotifications: GitifyNotification[]; + repoName: string; + isCollapsed: boolean; + isAnimatingExit: boolean; + onToggle: () => void; + onAnimateExit: (animate: boolean) => void; +} + +export const RepositoryHeader: FC = ({ + repoName, + repoNotifications, + isCollapsed, + isAnimatingExit, + onToggle, + onAnimateExit, +}) => { + const { markNotificationsAsRead, markNotificationsAsDone } = useNotifications(); + + const shouldAnimateExit = shouldRemoveNotificationsFromState(); + + // 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) => { + onAnimateExit(shouldAnimateExit); + + await action(); + + const { failures } = useNotificationActionFailuresStore.getState(); + const hasFailure = repoNotifications.some( + (notification) => failures[getNotificationFailureKey(notification.account, notification.id)], + ); + + if (hasFailure) { + onAnimateExit(false); + } + }; + + const areAllRepoNotificationsRead = repoNotifications.every( + (notification) => !notification.unread, + ); + + const Chevron = getChevronDetails(true, !isCollapsed, 'repository'); + + return ( + + + + {!isAnimatingExit && ( + + runGroupAction(() => markNotificationsAsRead(repoNotifications))} + enabled={!areAllRepoNotificationsRead} + icon={ReadIcon} + label="Mark repository as read" + testid="repository-mark-as-read" + /> + + runGroupAction(() => markNotificationsAsDone(repoNotifications))} + enabled={ + isMarkAsDoneFeatureSupported(repoNotifications[0].account) && + !areAllRepoNotificationsRead + } + icon={CheckIcon} + label="Mark repository as done" + testid="repository-mark-as-done" + /> + + + + )} + + ); +}; diff --git a/src/renderer/components/notifications/RepositoryNotifications.tsx b/src/renderer/components/notifications/RepositoryNotifications.tsx deleted file mode 100644 index 6fae35d9d..000000000 --- a/src/renderer/components/notifications/RepositoryNotifications.tsx +++ /dev/null @@ -1,153 +0,0 @@ -import { type FC, type MouseEvent, useState } from 'react'; - -import { CheckIcon, ReadIcon } from '@primer/octicons-react'; -import { Button, Stack } from '@primer/react'; - -import { cn } from 'cn'; - -import { useNotifications } from '../../hooks/useNotifications'; -import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; - -import { HoverButton } from '../primitives/HoverButton'; -import { HoverGroup } from '../primitives/HoverGroup'; - -import { type GitifyNotification, Opacity, Size } from '../../types'; - -import { isMarkAsDoneFeatureSupported } from '../../utils/api/features'; -import { shouldRemoveNotificationsFromState } from '../../utils/notifications/remove'; -import { openRepository } from '../../utils/system/links'; -import { getChevronDetails } from '../../utils/ui/display'; -import { AvatarWithFallback } from '../avatars/AvatarWithFallback'; -import { NotificationRow } from './NotificationRow'; - -export interface RepositoryNotificationsProps { - repoNotifications: GitifyNotification[]; - repoName: string; -} - -export const RepositoryNotifications: FC = ({ - repoName, - repoNotifications, -}) => { - const { markNotificationsAsRead, markNotificationsAsDone } = useNotifications(); - - const [shouldAnimateRepositoryExit, setShouldAnimateRepositoryExit] = useState(false); - const [isRepositoryNotificationsVisible, setIsRepositoryNotificationsVisible] = useState(true); - - const avatarUrl = repoNotifications[0].repository.owner.avatarUrl; - const shouldAnimateExit = shouldRemoveNotificationsFromState(); - - const actionRepositoryInteraction = () => { - openRepository(repoNotifications[0].repository); - }; - - // 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); - - await action(); - - const { failures } = useNotificationActionFailuresStore.getState(); - const hasFailure = repoNotifications.some( - (notification) => failures[getNotificationFailureKey(notification.account, notification.id)], - ); - - if (hasFailure) { - setShouldAnimateRepositoryExit(false); - } - }; - - const actionMarkAsDone = () => runGroupAction(() => markNotificationsAsDone(repoNotifications)); - - const actionMarkAsRead = () => runGroupAction(() => markNotificationsAsRead(repoNotifications)); - - const actionToggleRepositoryNotifications = () => { - setIsRepositoryNotificationsVisible(!isRepositoryNotificationsVisible); - }; - - const areAllRepoNotificationsRead = repoNotifications.every( - (notification) => !notification.unread, - ); - - const Chevron = getChevronDetails(true, isRepositoryNotificationsVisible, 'repository'); - - return ( - <> - - - - {!shouldAnimateRepositoryExit && ( - - - - - - - - )} - - - {isRepositoryNotificationsVisible && - repoNotifications.map((notification) => ( - - ))} - - ); -}; diff --git a/src/renderer/components/notifications/__snapshots__/AccountHeader.test.tsx.snap b/src/renderer/components/notifications/__snapshots__/AccountHeader.test.tsx.snap new file mode 100644 index 000000000..e8421f3ce --- /dev/null +++ b/src/renderer/components/notifications/__snapshots__/AccountHeader.test.tsx.snap @@ -0,0 +1,193 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`renderer/components/notifications/AccountHeader.tsx > should render an error background when the account errored 1`] = ` +
+ +
+`; diff --git a/src/renderer/components/notifications/__snapshots__/AccountNotifications.test.tsx.snap b/src/renderer/components/notifications/__snapshots__/AccountNotifications.test.tsx.snap deleted file mode 100644 index 2c104881d..000000000 --- a/src/renderer/components/notifications/__snapshots__/AccountNotifications.test.tsx.snap +++ /dev/null @@ -1,2063 +0,0 @@ -// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html - -exports[`renderer/components/notifications/AccountNotifications.tsx > should render itself - account error for multiple accounts 1`] = ` -
- -
-
-
-
- 🔥 -
-
- Error title -
-
-
- Error description -
-
-
-
-`; - -exports[`renderer/components/notifications/AccountNotifications.tsx > should render itself - account error for single account 1`] = ` -
- -
-
-
-
- 🔥 -
-
- Error title -
-
-
- Error description -
-
-
-
-`; - -exports[`renderer/components/notifications/AccountNotifications.tsx > should render itself - group notifications by date 1`] = ` -
- -
-
- - -
-
-
- -
- 123 -
-
-
-
- - - I am a robot and this is a test! - - - -
-
- -
- - Updated - - - May 20, 2017 - -
-
- - - - -
-
-
-
-
- - - -
-
-
-
- - -
-
-
- -
- 456 -
-
-
-
- - - Improve the UI - - - -
-
-
- -
-
- - Authored - - - May 20, 2017 - -
-
-
-
-
-
- - - -
-
-
-`; - -exports[`renderer/components/notifications/AccountNotifications.tsx > should render itself - group notifications by repositories 1`] = ` -
- -
- RepositoryNotifications -
-
-`; - -exports[`renderer/components/notifications/AccountNotifications.tsx > should render itself - no notifications 1`] = ` -
- -
-
-
-
- 🎉 -
-
- No new notifications -
-
-
-
-
-`; - -exports[`renderer/components/notifications/AccountNotifications.tsx > should toggle account notifications visibility 1`] = ` -
- -
- RepositoryNotifications -
-
-`; diff --git a/src/renderer/components/notifications/__snapshots__/RepositoryNotifications.test.tsx.snap b/src/renderer/components/notifications/__snapshots__/RepositoryHeader.test.tsx.snap similarity index 69% rename from src/renderer/components/notifications/__snapshots__/RepositoryNotifications.test.tsx.snap rename to src/renderer/components/notifications/__snapshots__/RepositoryHeader.test.tsx.snap index 22902ca03..f0868c21e 100644 --- a/src/renderer/components/notifications/__snapshots__/RepositoryNotifications.test.tsx.snap +++ b/src/renderer/components/notifications/__snapshots__/RepositoryHeader.test.tsx.snap @@ -1,6 +1,6 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html -exports[`renderer/components/notifications/RepositoryNotifications.tsx > should render itself & its children - all notifications are read 1`] = ` +exports[`renderer/components/notifications/RepositoryHeader.tsx > should render itself & its children - all notifications are read 1`] = `
should
-
- NotificationRow -
-
- NotificationRow -
`; -exports[`renderer/components/notifications/RepositoryNotifications.tsx > should render itself & its children 1`] = ` +exports[`renderer/components/notifications/RepositoryHeader.tsx > should render itself & its children 1`] = `
should
-
- NotificationRow -
-
- NotificationRow -
- -`; - -exports[`renderer/components/notifications/RepositoryNotifications.tsx > should toggle repository notifications visibility 1`] = ` -
-
- -
- - - -
-
-
- NotificationRow -
-
- NotificationRow -
`; -exports[`renderer/components/notifications/RepositoryNotifications.tsx > should use default repository icon when avatar is not available 1`] = ` +exports[`renderer/components/notifications/RepositoryHeader.tsx > should use default repository icon when avatar is not available 1`] = `
should
-
- NotificationRow -
-
- NotificationRow -
`; diff --git a/src/renderer/routes/Notifications.test.tsx b/src/renderer/routes/Notifications.test.tsx index 21f412ef1..5a7b38bf5 100644 --- a/src/renderer/routes/Notifications.test.tsx +++ b/src/renderer/routes/Notifications.test.tsx @@ -7,8 +7,8 @@ import { mockSettings } from '../__mocks__/state-mocks'; import { Errors } from '../utils/core/errors'; import { NotificationsRoute } from './Notifications'; -vi.mock('../components/notifications/AccountNotifications', () => ({ - AccountNotifications: () =>

AccountNotifications

, +vi.mock('../components/notifications/NotificationList', () => ({ + NotificationList: () =>

NotificationList

, })); vi.mock('../components/AllRead', () => ({ @@ -23,6 +23,7 @@ describe('renderer/routes/Notifications.tsx', () => { it('should render itself & its children (with notifications)', () => { const tree = renderWithProviders(, { notifications: mockMultipleAccountNotifications, + hasNotifications: true, }); expect(tree.container).toMatchSnapshot(); @@ -36,6 +37,7 @@ describe('renderer/routes/Notifications.tsx', () => { it('should render itself & its children (show account header)', () => { const tree = renderWithProviders(, { notifications: [mockMultipleAccountNotifications[0]], + hasNotifications: true, settings: { ...mockSettings, showAccountHeader: true }, }); expect(tree.container).toMatchSnapshot(); diff --git a/src/renderer/routes/Notifications.tsx b/src/renderer/routes/Notifications.tsx index fa5dc5284..0feb613e8 100644 --- a/src/renderer/routes/Notifications.tsx +++ b/src/renderer/routes/Notifications.tsx @@ -5,9 +5,8 @@ import { useOnlineStatus } from '../hooks/useOnlineStatus'; import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; import { AllRead } from '../components/AllRead'; -import { Contents } from '../components/layout/Contents'; import { Page } from '../components/layout/Page'; -import { AccountNotifications } from '../components/notifications/AccountNotifications'; +import { NotificationList } from '../components/notifications/NotificationList'; import { Oops } from '../components/Oops'; import { getAccountUUID } from '../utils/auth/utils'; @@ -48,19 +47,10 @@ export const NotificationsRoute: FC = () => { return ( - - {visibleNotifications.map((accountNotification) => { - return ( - - ); - })} - + ); }; diff --git a/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap b/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap index de6f03b76..913470782 100644 --- a/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap @@ -50,16 +50,26 @@ exports[`renderer/routes/Notifications.tsx > should render itself & its children exports[`renderer/routes/Notifications.tsx > should render itself & its children (show account header) 1`] = `
-

- AllRead -

+
+

+ NotificationList +

+
`; exports[`renderer/routes/Notifications.tsx > should render itself & its children (with notifications) 1`] = `
-

- AllRead -

+
+

+ NotificationList +

+
`; From ae5ec43829d44b3dab25de3b8a343bb0e3b14d0d Mon Sep 17 00:00:00 2001 From: Filipe Guerreiro Date: Sun, 20 Sep 2026 20:34:44 +0900 Subject: [PATCH 2/4] fix: restore repositories after bulk actions --- .../notifications/NotificationList.test.tsx | 55 ++++++++++++++++ .../notifications/NotificationList.tsx | 66 ++++++++++++++----- .../notifications/RepositoryHeader.tsx | 6 +- 3 files changed, 107 insertions(+), 20 deletions(-) diff --git a/src/renderer/components/notifications/NotificationList.test.tsx b/src/renderer/components/notifications/NotificationList.test.tsx index b1ff0a7a5..5a2e9a64c 100644 --- a/src/renderer/components/notifications/NotificationList.test.tsx +++ b/src/renderer/components/notifications/NotificationList.test.tsx @@ -107,6 +107,61 @@ describe('renderer/components/notifications/NotificationList.tsx', () => { expect(screen.getByTestId('open-repository')).toBeInTheDocument(); }); + it('shows a repository again after its notifications are reloaded', async () => { + const markNotificationsAsRead = vi.fn(); + const tree = renderWithProviders( + , + { + settings: { ...mockSettings, groupBy: GroupBy.REPOSITORY }, + markNotificationsAsRead, + }, + ); + + await userEvent.click(screen.getByTestId('repository-mark-as-read')); + + tree.rerender( + , + ); + tree.rerender( + , + ); + + expect(tree.container.querySelector(`[id="${mockGitifyNotification.id}"]`)).not.toHaveClass( + 'opacity-0', + ); + }); + + it('shows a new notification when repository remains after group action', async () => { + const newNotification = { ...mockGitifyNotification, id: 'new-notification' }; + const tree = renderWithProviders( + , + { + settings: { ...mockSettings, groupBy: GroupBy.REPOSITORY }, + markNotificationsAsRead: vi.fn(), + }, + ); + + await userEvent.click(screen.getByTestId('repository-mark-as-read')); + + tree.rerender( + , + ); + + expect(tree.container.querySelector('[id="new-notification"]')).not.toHaveClass('opacity-0'); + }); + it('renders an account error instead of its notifications', () => { renderWithProviders( = ({ const [collapsedRepositories, setCollapsedRepositories] = useState>( new Set(), ); - const [animatingRepositories, setAnimatingRepositories] = useState>( - new Set(), - ); + const [animatingRepositories, setAnimatingRepositories] = useState< + ReadonlyMap> + >(new Map()); const toggleCollapsedAccount = useCallback((accountUUID: string) => { setCollapsedAccounts((current) => { @@ -85,19 +85,53 @@ export const NotificationList: FC = ({ }); }, []); - const setRepositoryAnimatingExit = useCallback((repoKey: string, animate: boolean) => { - setAnimatingRepositories((current) => { - const next = new Set(current); + const setRepositoryAnimatingExit = useCallback( + (repoKey: string, notifications: GitifyNotification[], animate: boolean) => { + setAnimatingRepositories((current) => { + const next = new Map(current); + + if (animate) { + next.set( + repoKey, + new Set( + notifications.map( + (notification) => `${getAccountUUID(notification.account)}:${notification.id}`, + ), + ), + ); + } else { + next.delete(repoKey); + } - if (animate) { - next.add(repoKey); - } else { - next.delete(repoKey); - } + return next; + }); + }, + [], + ); - return next; + const notificationKeys = useMemo( + () => + new Set( + accountNotifications.flatMap(({ notifications }) => + notifications.map( + (notification) => `${getAccountUUID(notification.account)}:${notification.id}`, + ), + ), + ), + [accountNotifications], + ); + + useEffect(() => { + setAnimatingRepositories((current) => { + const next = new Map( + [...current].filter(([, actedNotifications]) => + [...actedNotifications].some((notificationKey) => notificationKeys.has(notificationKey)), + ), + ); + + return next.size === current.size ? current : next; }); - }, []); + }, [notificationKeys]); const items = useMemo(() => { const list: ListItem[] = []; @@ -226,7 +260,9 @@ export const NotificationList: FC = ({ setRepositoryAnimatingExit(item.repoKey, animate)} + onAnimateExit={(animate) => + setRepositoryAnimatingExit(item.repoKey, item.notifications, animate) + } onToggle={() => toggleCollapsedRepository(item.repoKey)} repoName={item.repoName} repoNotifications={item.notifications} diff --git a/src/renderer/components/notifications/RepositoryHeader.tsx b/src/renderer/components/notifications/RepositoryHeader.tsx index 95c9b86a7..f90f9f81b 100644 --- a/src/renderer/components/notifications/RepositoryHeader.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.tsx @@ -40,11 +40,7 @@ export const RepositoryHeader: FC = ({ const shouldAnimateExit = shouldRemoveNotificationsFromState(); - // 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. + // Successful actions clear when their rows disappear; failures must clear here. const runGroupAction = async (action: () => Promise) => { onAnimateExit(shouldAnimateExit); From a28111f39282900aea04e422ede616f33bb8b7b4 Mon Sep 17 00:00:00 2001 From: Filipe Guerreiro Date: Sun, 20 Sep 2026 21:08:02 +0900 Subject: [PATCH 3/4] fix: restore notifications after direct action failure --- src/renderer/__helpers__/hook-mocks.ts | 6 ++-- .../notifications/NotificationRow.test.tsx | 16 ++++++++++ .../notifications/NotificationRow.tsx | 10 ++++--- .../notifications/RepositoryHeader.test.tsx | 13 +++++++++ .../notifications/RepositoryHeader.tsx | 16 +++++----- src/renderer/hooks/useNotifications.test.tsx | 29 +++++++++++++++++++ src/renderer/hooks/useNotifications.ts | 27 +++++++++++++---- 7 files changed, 97 insertions(+), 20 deletions(-) diff --git a/src/renderer/__helpers__/hook-mocks.ts b/src/renderer/__helpers__/hook-mocks.ts index 631795783..fe2da839e 100644 --- a/src/renderer/__helpers__/hook-mocks.ts +++ b/src/renderer/__helpers__/hook-mocks.ts @@ -40,9 +40,9 @@ function buildNotificationsDefaults(): NotificationsState { refetchNotifications: vi.fn(), removeAccountNotifications: vi.fn(), - markNotificationsAsRead: vi.fn(), - markNotificationsAsDone: vi.fn(), - unsubscribeNotification: vi.fn(), + markNotificationsAsRead: vi.fn().mockResolvedValue(true), + markNotificationsAsDone: vi.fn().mockResolvedValue(true), + unsubscribeNotification: vi.fn().mockResolvedValue(true), }; } diff --git a/src/renderer/components/notifications/NotificationRow.test.tsx b/src/renderer/components/notifications/NotificationRow.test.tsx index b16451430..6f843e487 100644 --- a/src/renderer/components/notifications/NotificationRow.test.tsx +++ b/src/renderer/components/notifications/NotificationRow.test.tsx @@ -281,6 +281,7 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { it('shows hover actions in their normal (non-danger) state when there is no recorded failure', () => { const props: NotificationRowProps = { notification: mockGitifyNotification, + isRepositoryAnimatingExit: false, }; @@ -291,6 +292,21 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { 'Mark as read', ); }); + it('restores its actions when the mutation fails directly', async () => { + const props: NotificationRowProps = { + notification: mockGitifyNotification, + isRepositoryAnimatingExit: false, + }; + + renderWithProviders(, { + settings: { ...mockSettings, delayNotificationState: false, fetchReadNotifications: false }, + markNotificationsAsRead: vi.fn().mockResolvedValue(false), + }); + + await userEvent.click(screen.getByTestId('notification-mark-as-read')); + + expect(screen.getByTestId('notification-mark-as-read')).toBeInTheDocument(); + }); it('styles and explains only the action that failed', () => { const props: NotificationRowProps = { diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index 783600f82..9261dca89 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -67,12 +67,14 @@ export const NotificationRow: FC = ({ // 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) => { + const runAction = async (action: () => Promise) => { setShouldAnimateNotificationExit(shouldAnimateExit); - await action(); - - if (useNotificationActionFailuresStore.getState().failures[notificationFailureKey]) { + const succeeded = await action(); + if ( + succeeded === false || + useNotificationActionFailuresStore.getState().failures[notificationFailureKey] + ) { setShouldAnimateNotificationExit(false); } }; diff --git a/src/renderer/components/notifications/RepositoryHeader.test.tsx b/src/renderer/components/notifications/RepositoryHeader.test.tsx index d20541df4..5e4154781 100644 --- a/src/renderer/components/notifications/RepositoryHeader.test.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.test.tsx @@ -157,5 +157,18 @@ describe('renderer/components/notifications/RepositoryHeader.tsx', () => { expect(onAnimateExit.mock.calls).toEqual([[true], [false]]); }); + + it('reverts the group exit animation when the mutation fails directly', async () => { + const onAnimateExit = vi.fn(); + + renderWithProviders(, { + settings: { ...mockSettings }, + markNotificationsAsRead: vi.fn().mockResolvedValue(false), + }); + + await userEvent.click(screen.getByTestId('repository-mark-as-read')); + + expect(onAnimateExit.mock.calls).toEqual([[true], [false]]); + }); }); }); diff --git a/src/renderer/components/notifications/RepositoryHeader.tsx b/src/renderer/components/notifications/RepositoryHeader.tsx index f90f9f81b..0232ee326 100644 --- a/src/renderer/components/notifications/RepositoryHeader.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.tsx @@ -40,16 +40,18 @@ export const RepositoryHeader: FC = ({ const shouldAnimateExit = shouldRemoveNotificationsFromState(); - // Successful actions clear when their rows disappear; failures must clear here. - const runGroupAction = async (action: () => Promise) => { + // Successful actions clear when rows disappear; failures must clear here. + const runGroupAction = async (action: () => Promise) => { onAnimateExit(shouldAnimateExit); - await action(); - + const succeeded = await action(); const { failures } = useNotificationActionFailuresStore.getState(); - const hasFailure = repoNotifications.some( - (notification) => failures[getNotificationFailureKey(notification.account, notification.id)], - ); + const hasFailure = + succeeded === false || + repoNotifications.some( + (notification) => + failures[getNotificationFailureKey(notification.account, notification.id)], + ); if (hasFailure) { onAnimateExit(false); diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 3f66a9d21..a4b9ef412 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -562,6 +562,35 @@ describe('renderer/hooks/useNotifications.ts', () => { expect(rendererLogErrorSpy).toHaveBeenCalled(); }); + it('reports a direct mutation failure while retaining the restored notification', async () => { + const invalidNotification = { + ...mockGitifyNotification, + account: { + ...mockGitifyNotification.account, + forge: 'unsupported' as typeof mockGitifyNotification.account.forge, + }, + }; + const accountNotifications: AccountNotifications[] = [ + { + account: invalidNotification.account, + notifications: [invalidNotification], + error: null, + }, + ]; + getAllNotificationsMock.mockResolvedValue(accountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + let actionSucceeded: unknown; + await act(async () => { + actionSucceeded = await result.current.markNotificationsAsRead([invalidNotification]); + }); + + expect(actionSucceeded).toBe(false); + expect(result.current.notificationCount).toBe(1); + }); + it('rolls back the cache for a failed notification while a failed request does not affect it', async () => { vi.spyOn(githubAdapter.accountOps, 'markThreadAsRead').mockRejectedValue(new Error('boom')); getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 9dcd0dd7c..3f64fe1bb 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -71,9 +71,9 @@ interface NotificationsState { refetchNotifications: () => Promise; removeAccountNotifications: (account: Account) => Promise; - markNotificationsAsRead: (notifications: GitifyNotification[]) => Promise; - markNotificationsAsDone: (notifications: GitifyNotification[]) => Promise; - unsubscribeNotification: (notification: GitifyNotification) => Promise; + markNotificationsAsRead: (notifications: GitifyNotification[]) => Promise; + markNotificationsAsDone: (notifications: GitifyNotification[]) => Promise; + unsubscribeNotification: (notification: GitifyNotification) => Promise; } interface UseNotificationsOptions { @@ -617,21 +617,36 @@ export const useNotifications = ({ const markNotificationsAsRead = useCallback( async (readNotifications: GitifyNotification[]) => { - await markNotificationsAsReadMutation.mutateAsync({ readNotifications }).catch(() => {}); + try { + await markNotificationsAsReadMutation.mutateAsync({ readNotifications }); + return true; + } catch { + return false; + } }, [markNotificationsAsReadMutation], ); const markNotificationsAsDone = useCallback( async (doneNotifications: GitifyNotification[]) => { - await markNotificationsAsDoneMutation.mutateAsync({ doneNotifications }).catch(() => {}); + try { + await markNotificationsAsDoneMutation.mutateAsync({ doneNotifications }); + return true; + } catch { + return false; + } }, [markNotificationsAsDoneMutation], ); const unsubscribeNotification = useCallback( async (notification: GitifyNotification) => { - await unsubscribeNotificationMutation.mutateAsync({ notification }).catch(() => {}); + try { + await unsubscribeNotificationMutation.mutateAsync({ notification }); + return true; + } catch { + return false; + } }, [unsubscribeNotificationMutation], ); From c88fd357555ab674edfdc4322789fe2ff1d32038 Mon Sep 17 00:00:00 2001 From: Filipe Guerreiro Date: Mon, 21 Sep 2026 14:20:48 +0900 Subject: [PATCH 4/4] fix: preserve notification action outcomes --- src/renderer/hooks/useNotifications.test.tsx | 31 ++----------------- src/renderer/hooks/useNotifications.ts | 16 ++++++---- .../utils/notifications/mutations.test.ts | 20 ++++++++++++ src/renderer/utils/notifications/mutations.ts | 2 +- 4 files changed, 33 insertions(+), 36 deletions(-) diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index a4b9ef412..dd8e51165 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -555,40 +555,13 @@ describe('renderer/hooks/useNotifications.ts', () => { const { result } = renderNotificationsHook(); await waitFor(() => expect(result.current.hasNotifications).toBe(true)); - await act(async () => { - await result.current.markNotificationsAsRead([mockGitifyNotification]).catch(() => {}); - }); - - expect(rendererLogErrorSpy).toHaveBeenCalled(); - }); - - it('reports a direct mutation failure while retaining the restored notification', async () => { - const invalidNotification = { - ...mockGitifyNotification, - account: { - ...mockGitifyNotification.account, - forge: 'unsupported' as typeof mockGitifyNotification.account.forge, - }, - }; - const accountNotifications: AccountNotifications[] = [ - { - account: invalidNotification.account, - notifications: [invalidNotification], - error: null, - }, - ]; - getAllNotificationsMock.mockResolvedValue(accountNotifications); - - const { result } = renderNotificationsHook(); - await waitFor(() => expect(result.current.hasNotifications).toBe(true)); - let actionSucceeded: unknown; await act(async () => { - actionSucceeded = await result.current.markNotificationsAsRead([invalidNotification]); + actionSucceeded = await result.current.markNotificationsAsRead([mockGitifyNotification]); }); expect(actionSucceeded).toBe(false); - expect(result.current.notificationCount).toBe(1); + expect(rendererLogErrorSpy).toHaveBeenCalled(); }); it('rolls back the cache for a failed notification while a failed request does not affect it', async () => { diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 3f64fe1bb..6120252c6 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -618,8 +618,10 @@ export const useNotifications = ({ const markNotificationsAsRead = useCallback( async (readNotifications: GitifyNotification[]) => { try { - await markNotificationsAsReadMutation.mutateAsync({ readNotifications }); - return true; + const { failed } = await markNotificationsAsReadMutation.mutateAsync({ + readNotifications, + }); + return failed.length === 0; } catch { return false; } @@ -630,8 +632,10 @@ export const useNotifications = ({ const markNotificationsAsDone = useCallback( async (doneNotifications: GitifyNotification[]) => { try { - await markNotificationsAsDoneMutation.mutateAsync({ doneNotifications }); - return true; + const { failed } = await markNotificationsAsDoneMutation.mutateAsync({ + doneNotifications, + }); + return failed.length === 0; } catch { return false; } @@ -642,8 +646,8 @@ export const useNotifications = ({ const unsubscribeNotification = useCallback( async (notification: GitifyNotification) => { try { - await unsubscribeNotificationMutation.mutateAsync({ notification }); - return true; + const { failed } = await unsubscribeNotificationMutation.mutateAsync({ notification }); + return failed.length === 0; } catch { return false; } diff --git a/src/renderer/utils/notifications/mutations.test.ts b/src/renderer/utils/notifications/mutations.test.ts index 460c433b9..99567c9f9 100644 --- a/src/renderer/utils/notifications/mutations.test.ts +++ b/src/renderer/utils/notifications/mutations.test.ts @@ -40,6 +40,26 @@ describe('renderer/utils/notifications/mutations.ts', () => { expect(result.failed[0].rawError).toBe(forbiddenError); }); + it('isolates a synchronous failure and continues later actions', async () => { + const [first, second] = mockGitHubCloudGitifyNotifications; + const forbiddenError = new RequestError('Forbidden', 403, { + request: { method: 'GET', url: 'https://api.github.com', headers: {} }, + }); + const action = vi + .fn() + .mockImplementationOnce(() => { + throw forbiddenError; + }) + .mockResolvedValueOnce(undefined); + + const result = await settleNotificationActions([first, second], action); + + expect(result.succeeded).toEqual([second]); + expect(result.failed[0].notification).toEqual(first); + expect(result.failed[0].error).toBe(Errors.ACTION_FORBIDDEN); + expect(action).toHaveBeenCalledTimes(2); + }); + it('classifies each failure independently using determineFailureType', async () => { const [first, second] = mockGitHubCloudGitifyNotifications; diff --git a/src/renderer/utils/notifications/mutations.ts b/src/renderer/utils/notifications/mutations.ts index 7d51b15b4..e96f82339 100644 --- a/src/renderer/utils/notifications/mutations.ts +++ b/src/renderer/utils/notifications/mutations.ts @@ -46,7 +46,7 @@ export async function settleNotificationActions( action: (notification: GitifyNotification) => Promise, ): Promise { const results = await Promise.allSettled( - notifications.map((notification) => action(notification)), + notifications.map((notification) => Promise.resolve().then(() => action(notification))), ); const succeeded: GitifyNotification[] = [];