Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ App-level side effects are owned by a single host. `GlobalEffects` mounts exactl

Consumers read the shared query cache and stores, and **do not register their own** polling, alerts, wake, or focus listeners. This keeps background work (for example, firing one poll per interval) from multiplying with the number of mounted components. When you add a new background effect, add it to the host, not to a leaf component.

Subscription is separate from side effects, but it multiplies the same way: every TanStack Query observer re-runs the query's `select` and structural-sharing walk whenever the cached data changes. A component that renders once per notification — the notification row, the repository header — must therefore not subscribe. It obtains notification actions (mark as read/done, unsubscribe, remove an account, refetch) with `useNotificationActions`, which never calls `useQuery`. Components that read notification data use `useNotifications` and stay subscribed by design.

## Forge adapters

Multi-forge support is adapter-based. Each forge implements the `ForgeAdapter` interface in `src/renderer/utils/forges/`, and shared code routes through `getAdapter(forge)` (forge-wide members) or `getAccountAdapter(account)` (account-scoped operations). Shared code never imports forge-specific modules directly, and there is no forge-specific branching outside an adapter module. See `CONTRIBUTING.md` for the conditions for adding a forge.
Expand Down
19 changes: 19 additions & 0 deletions src/renderer/__helpers__/hook-mocks.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { useNotificationActions } from '../hooks/notifications/useNotificationActions';
import type { useLogins } from '../hooks/useLogins';
import type { useNotifications } from '../hooks/useNotifications';

Expand All @@ -15,6 +16,7 @@ import type { useNotifications } from '../hooks/useNotifications';

export type NotificationsState = ReturnType<typeof useNotifications>;
export type LoginsState = ReturnType<typeof useLogins>;
export type NotificationActionsState = ReturnType<typeof useNotificationActions>;

let notificationsDefaults: NotificationsState | null = null;
let notificationsOverrides: Partial<NotificationsState> = {};
Expand Down Expand Up @@ -64,6 +66,23 @@ export function getMockedNotificationsState(): NotificationsState {
return { ...notificationsDefaults, ...notificationsOverrides };
}

/**
* Current mocked `useNotificationActions` return value: the action subset of
* the merged notifications state, so overrides passed through
* `renderWithProviders` reach action-only consumers too.
*/
export function getMockedNotificationActionsState(): NotificationActionsState {
const state = getMockedNotificationsState();

return {
refetchNotifications: state.refetchNotifications,
removeAccountNotifications: state.removeAccountNotifications,
markNotificationsAsRead: state.markNotificationsAsRead,
markNotificationsAsDone: state.markNotificationsAsDone,
unsubscribeNotification: state.unsubscribeNotification,
};
}

/** Current mocked `useLogins` return value. */
export function getMockedLoginsState(): LoginsState {
loginsDefaults ??= buildLoginsDefaults();
Expand Down
11 changes: 11 additions & 0 deletions src/renderer/__helpers__/visual.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@ vi.mock('../hooks/useNotifications', async () => {
};
});

vi.mock('../hooks/notifications/useNotificationActions', async () => {
const actual = await vi.importActual<
typeof import('../hooks/notifications/useNotificationActions')
>('../hooks/notifications/useNotificationActions');
const { getMockedNotificationActionsState } = await import('./hook-mocks');
return {
...actual,
useNotificationActions: () => getMockedNotificationActionsState(),
};
});

vi.mock('../hooks/useLogins', async () => {
const actual = await vi.importActual<typeof import('../hooks/useLogins')>('../hooks/useLogins');
const { getMockedLoginsState } = await import('./hook-mocks');
Expand Down
11 changes: 11 additions & 0 deletions src/renderer/__helpers__/vitest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ vi.mock('../hooks/useNotifications', async () => {
};
});

vi.mock('../hooks/notifications/useNotificationActions', async () => {
const actual = await vi.importActual<
typeof import('../hooks/notifications/useNotificationActions')
>('../hooks/notifications/useNotificationActions');
const { getMockedNotificationActionsState } = await import('./hook-mocks');
return {
...actual,
useNotificationActions: () => getMockedNotificationActionsState(),
};
});

vi.mock('../hooks/notifications/useNotificationsHost', async () => {
const actual = await vi.importActual<
typeof import('../hooks/notifications/useNotificationsHost')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { render } from '@testing-library/react';
import type { ReactNode } from 'react';

import { QueryClient, QueryClientProvider } from '@tanstack/react-query';

import { mockGitifyNotification } from '../../__mocks__/notifications-mocks';

import { NotificationRow } from './NotificationRow';

// Exercise the real actions hook (globally mocked in vitest.setup): a row must
// obtain its actions without becoming an observer of the notifications query.
vi.unmock('../../hooks/notifications/useNotificationActions');

const createWrapper = () => {
const queryClient = new QueryClient({
defaultOptions: {
queries: {
retry: false,
refetchOnWindowFocus: false,
refetchInterval: false,
},
},
});

return {
queryClient,
wrapper: ({ children }: { children: ReactNode }) => (
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
),
};
};

describe('renderer/components/notifications/NotificationRow.tsx - subscriptions', () => {
it('renders without subscribing to the notifications query', () => {
const { queryClient, wrapper } = createWrapper();

render(
<NotificationRow
notification={mockGitifyNotification}
isAnimatingExit={false}
onAnimateExit={vi.fn()}
/>,
{ wrapper },
);

expect(queryClient.getQueryCache().getAll()).toHaveLength(0);
});

it('adds no query observers no matter how many rows are mounted', () => {
const { queryClient, wrapper } = createWrapper();

render(
<>
{Array.from({ length: 50 }, (_, index) => (
<NotificationRow
key={index}
notification={{ ...mockGitifyNotification, id: `${index}` }}
isAnimatingExit={false}
onAnimateExit={vi.fn()}
/>
))}
</>,
{ wrapper },
);

const observers = queryClient
.getQueryCache()
.getAll()
.flatMap((query) => query.observers);

expect(observers).toHaveLength(0);
});
});
4 changes: 2 additions & 2 deletions src/renderer/components/notifications/NotificationRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Stack, Text, Tooltip } from '@primer/react';

import { cn } from 'cn';

import { useNotifications } from '../../hooks/useNotifications';
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';
import {
getNotificationFailureKey,
useNotificationActionFailuresStore,
Expand Down Expand Up @@ -38,7 +38,7 @@ export function NotificationRow({
onAnimateExit,
}: Readonly<NotificationRowProps>) {
const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } =
useNotifications();
useNotificationActions();

const markAsDoneOnOpen = useSettingsStore((s) => s.markAsDoneOnOpen);
const wrapNotificationTitle = useSettingsStore((s) => s.wrapNotificationTitle);
Expand Down
4 changes: 2 additions & 2 deletions src/renderer/components/notifications/RepositoryHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Button, Stack } from '@primer/react';

import { cn } from 'cn';

import { useNotifications } from '../../hooks/useNotifications';
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';

import { HoverButton } from '../primitives/HoverButton';
import { HoverGroup } from '../primitives/HoverGroup';
Expand Down Expand Up @@ -35,7 +35,7 @@ export function RepositoryHeader({
onToggle,
onAnimateExit,
}: Readonly<RepositoryHeaderProps>) {
const { markNotificationsAsRead, markNotificationsAsDone } = useNotifications();
const { markNotificationsAsRead, markNotificationsAsDone } = useNotificationActions();

const shouldAnimateExit = shouldRemoveNotificationsFromState();

Expand Down
4 changes: 2 additions & 2 deletions src/renderer/components/primitives/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useNavigate } from 'react-router-dom';
import { ArrowLeftIcon, type Icon } from '@primer/octicons-react';
import { IconButton, Stack } from '@primer/react';

import { useNotifications } from '../../hooks/useNotifications';
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';

import { Title } from './Title';

Expand All @@ -16,7 +16,7 @@ interface HeaderProps {
export function Header(props: Readonly<HeaderProps>) {
const navigate = useNavigate();

const { refetchNotifications: fetchNotifications } = useNotifications();
const { refetchNotifications: fetchNotifications } = useNotificationActions();

return (
<div className="pl-4 pr-5 pt-3 pb-1">
Expand Down
141 changes: 141 additions & 0 deletions src/renderer/hooks/notifications/useNotificationActions.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import { act, renderHook, waitFor } from '@testing-library/react';
import type { ReactNode } from 'react';

import { QueryClient, QueryClientProvider } from '@tanstack/react-query';

import { mockGitHubCloudAccount } from '../../__mocks__/account-mocks';
import {
mockGitifyNotification,
mockSingleAccountNotifications,
} from '../../__mocks__/notifications-mocks';

import { useAccountsStore } from '../../stores';

import { getAdapter } from '../../utils/forges/registry';
import * as notificationsUtils from '../../utils/notifications/notifications';
import { useNotifications } from '../useNotifications';
import { useNotificationActions } from './useNotificationActions';

// Exercise the real hooks (globally mocked in vitest.setup)
vi.unmock('./useNotificationActions');
vi.unmock('../useNotifications');

vi.mock('../../utils/notifications/notifications', async () => {
const actual = await vi.importActual<typeof import('../../utils/notifications/notifications')>(
'../../utils/notifications/notifications',
);
return {
...actual,
getAllNotifications: vi.fn(),
};
});

const getAllNotificationsMock = vi.mocked(notificationsUtils.getAllNotifications);

const createWrapper = () => {
const queryClient = new QueryClient({
defaultOptions: {
queries: {
retry: false,
refetchOnWindowFocus: false,
refetchInterval: false,
},
},
});

return {
queryClient,
wrapper: ({ children }: { children: ReactNode }) => (
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
),
};
};

describe('renderer/hooks/notifications/useNotificationActions.ts', () => {
const githubAdapter = getAdapter('github');

beforeEach(() => {
getAllNotificationsMock.mockReset();
useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] });
mockGitifyNotification.unread = true;
});

it('exposes the notification actions without fetching or creating a query', () => {
const { queryClient, wrapper } = createWrapper();

const { result } = renderHook(() => useNotificationActions(), { wrapper });

expect(result.current.markNotificationsAsRead).toBeTypeOf('function');
expect(result.current.markNotificationsAsDone).toBeTypeOf('function');
expect(result.current.unsubscribeNotification).toBeTypeOf('function');
expect(result.current.removeAccountNotifications).toBeTypeOf('function');
expect(result.current.refetchNotifications).toBeTypeOf('function');

expect(getAllNotificationsMock).not.toHaveBeenCalled();
expect(queryClient.getQueryCache().getAll()).toHaveLength(0);
});

it('does not add a notifications query observer', async () => {
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
const { queryClient, wrapper } = createWrapper();

renderHook(() => useNotificationActions(), { wrapper });
expect(queryClient.getQueryCache().getAll()).toHaveLength(0);

renderHook(() => useNotifications(), { wrapper });
await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(1));

// Only the reader subscribed; the action-only consumer added no observer.
const queries = queryClient.getQueryCache().getAll();
const observers = queries.flatMap((query) => query.observers);

expect(queries).toHaveLength(1);
expect(observers).toHaveLength(1);
});

it('refetchNotifications refetches the shared notifications query', async () => {
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
const { wrapper } = createWrapper();

renderHook(() => useNotifications(), { wrapper });
await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(1));

const actions = renderHook(() => useNotificationActions(), { wrapper });
await act(async () => {
await actions.result.current.refetchNotifications();
});

expect(getAllNotificationsMock).toHaveBeenCalledTimes(2);
});

it('removeAccountNotifications prunes the account from the shared cache', async () => {
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
const { wrapper } = createWrapper();

const reader = renderHook(() => useNotifications(), { wrapper });
await waitFor(() => expect(reader.result.current.hasNotifications).toBe(true));

const actions = renderHook(() => useNotificationActions(), { wrapper });
act(() => {
actions.result.current.removeAccountNotifications(mockGitHubCloudAccount);
});

await waitFor(() => expect(reader.result.current.notificationCount).toBe(0));
});

it('markNotificationsAsRead reaches the shared cache so a reader reflects it', async () => {
vi.spyOn(githubAdapter.accountOps, 'markThreadAsRead').mockResolvedValue(undefined);
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
const { wrapper } = createWrapper();

const reader = renderHook(() => useNotifications(), { wrapper });
await waitFor(() => expect(reader.result.current.hasNotifications).toBe(true));

const actions = renderHook(() => useNotificationActions(), { wrapper });
await act(async () => {
await actions.result.current.markNotificationsAsRead([mockGitifyNotification]);
});

await waitFor(() => expect(reader.result.current.notificationCount).toBe(0));
});
});
Loading
Loading