perf: decouple notification actions from the notifications query · gitify-app/gitify@ccdf9b9 · GitHub
Skip to content

Commit ccdf9b9

Browse files
setchysetchy
committed
perf: decouple notification actions from the notifications query
Notification rows and other action-only leaf components called `useNotifications()`, so each mounted row subscribed to the notifications query as its own observer. Every cache write then re-ran the query's `select` and its structural-sharing walk once per mounted row, which is what froze the renderer for ~8.5s when marking a notification on a 1,274-notification inbox. Add `useNotificationActions()`, which exposes the three mutations plus `removeAccountNotifications` and `refetchNotifications` without calling `useQuery`, and move the action-only consumers onto it: `NotificationRow`, `RepositoryHeader`, `primitives/Header` and `useLogins`. The notifications query key is extracted into `useNotificationsQueryKey()` so the action surface targets the same cache entry as the readers without an observer. Readers keep `useNotifications()`, which now composes the reader and action surfaces, and structural sharing stays at its default. An unchanged poll therefore remains a no-op for observers. Alternative to #3320, which fixes the same freeze by disabling structural sharing for the query and so also drops the poll-path reference stability.
1 parent f7617e7 commit ccdf9b9

16 files changed

Lines changed: 435 additions & 71 deletions

‎ARCHITECTURE.md‎

Lines changed: 2 additions & 0 deletions

‎src/renderer/__helpers__/hook-mocks.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { useNotificationActions } from '../hooks/notifications/useNotificationActions';
12
import type { useLogins } from '../hooks/useLogins';
23
import type { useNotifications } from '../hooks/useNotifications';
34

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

1617
export type NotificationsState = ReturnType<typeof useNotifications>;
1718
export type LoginsState = ReturnType<typeof useLogins>;
19+
export type NotificationActionsState = ReturnType<typeof useNotificationActions>;
1820

1921
let notificationsDefaults: NotificationsState | null = null;
2022
let notificationsOverrides: Partial<NotificationsState> = {};
@@ -64,6 +66,23 @@ export function getMockedNotificationsState(): NotificationsState {
6466
return { ...notificationsDefaults, ...notificationsOverrides };
6567
}
6668

69+
/**
70+
* Current mocked `useNotificationActions` return value: the action subset of
71+
* the merged notifications state, so overrides passed through
72+
* `renderWithProviders` reach action-only consumers too.
73+
*/
74+
export function getMockedNotificationActionsState(): NotificationActionsState {
75+
const state = getMockedNotificationsState();
76+
77+
return {
78+
refetchNotifications: state.refetchNotifications,
79+
removeAccountNotifications: state.removeAccountNotifications,
80+
markNotificationsAsRead: state.markNotificationsAsRead,
81+
markNotificationsAsDone: state.markNotificationsAsDone,
82+
unsubscribeNotification: state.unsubscribeNotification,
83+
};
84+
}
85+
6786
/** Current mocked `useLogins` return value. */
6887
export function getMockedLoginsState(): LoginsState {
6988
loginsDefaults ??= buildLoginsDefaults();

‎src/renderer/__helpers__/vitest.setup.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,17 @@ vi.mock('../hooks/useNotifications', async () => {
3636
};
3737
});
3838

39+
vi.mock('../hooks/notifications/useNotificationActions', async () => {
40+
const actual = await vi.importActual<
41+
typeof import('../hooks/notifications/useNotificationActions')
42+
>('../hooks/notifications/useNotificationActions');
43+
const { getMockedNotificationActionsState } = await import('./hook-mocks');
44+
return {
45+
...actual,
46+
useNotificationActions: () => getMockedNotificationActionsState(),
47+
};
48+
});
49+
3950
vi.mock('../hooks/notifications/useNotificationsHost', async () => {
4051
const actual = await vi.importActual<
4152
typeof import('../hooks/notifications/useNotificationsHost')

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import { render } from '@testing-library/react';
2+
import type { ReactNode } from 'react';
3+
4+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
5+
6+
import { mockGitifyNotification } from '../../__mocks__/notifications-mocks';
7+
8+
import { NotificationRow } from './NotificationRow';
9+
10+
// Exercise the real actions hook (globally mocked in vitest.setup): a row must
11+
// obtain its actions without becoming an observer of the notifications query.
12+
vi.unmock('../../hooks/notifications/useNotificationActions');
13+
14+
const createWrapper = () => {
15+
const queryClient = new QueryClient({
16+
defaultOptions: {
17+
queries: {
18+
retry: false,
19+
refetchOnWindowFocus: false,
20+
refetchInterval: false,
21+
},
22+
},
23+
});
24+
25+
return {
26+
queryClient,
27+
wrapper: ({ children }: { children: ReactNode }) => (
28+
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
29+
),
30+
};
31+
};
32+
33+
describe('renderer/components/notifications/NotificationRow.tsx - subscriptions', () => {
34+
it('renders without subscribing to the notifications query', () => {
35+
const { queryClient, wrapper } = createWrapper();
36+
37+
render(
38+
<NotificationRow
39+
notification={mockGitifyNotification}
40+
isAnimatingExit={false}
41+
onAnimateExit={vi.fn()}
42+
/>,
43+
{ wrapper },
44+
);
45+
46+
expect(queryClient.getQueryCache().getAll()).toHaveLength(0);
47+
});
48+
49+
it('adds no query observers no matter how many rows are mounted', () => {
50+
const { queryClient, wrapper } = createWrapper();
51+
52+
render(
53+
<>
54+
{Array.from({ length: 50 }, (_, index) => (
55+
<NotificationRow
56+
key={index}
57+
notification={{ ...mockGitifyNotification, id: `${index}` }}
58+
isAnimatingExit={false}
59+
onAnimateExit={vi.fn()}
60+
/>
61+
))}
62+
</>,
63+
{ wrapper },
64+
);
65+
66+
const observers = queryClient
67+
.getQueryCache()
68+
.getAll()
69+
.flatMap((query) => query.observers);
70+
71+
expect(observers).toHaveLength(0);
72+
});
73+
});

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { Stack, Text, Tooltip } from '@primer/react';
33

44
import { cn } from 'cn';
55

6-
import { useNotifications } from '../../hooks/useNotifications';
6+
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';
77
import {
88
getNotificationFailureKey,
99
useNotificationActionFailuresStore,
@@ -38,7 +38,7 @@ export function NotificationRow({
3838
onAnimateExit,
3939
}: Readonly<NotificationRowProps>) {
4040
const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } =
41-
useNotifications();
41+
useNotificationActions();
4242

4343
const markAsDoneOnOpen = useSettingsStore((s) => s.markAsDoneOnOpen);
4444
const wrapNotificationTitle = useSettingsStore((s) => s.wrapNotificationTitle);

‎src/renderer/components/notifications/RepositoryHeader.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { Button, Stack } from '@primer/react';
55

66
import { cn } from 'cn';
77

8-
import { useNotifications } from '../../hooks/useNotifications';
8+
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';
99

1010
import { HoverButton } from '../primitives/HoverButton';
1111
import { HoverGroup } from '../primitives/HoverGroup';
@@ -35,7 +35,7 @@ export function RepositoryHeader({
3535
onToggle,
3636
onAnimateExit,
3737
}: Readonly<RepositoryHeaderProps>) {
38-
const { markNotificationsAsRead, markNotificationsAsDone } = useNotifications();
38+
const { markNotificationsAsRead, markNotificationsAsDone } = useNotificationActions();
3939

4040
const shouldAnimateExit = shouldRemoveNotificationsFromState();
4141

‎src/renderer/components/primitives/Header.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { useNavigate } from 'react-router-dom';
33
import { ArrowLeftIcon, type Icon } from '@primer/octicons-react';
44
import { IconButton, Stack } from '@primer/react';
55

6-
import { useNotifications } from '../../hooks/useNotifications';
6+
import { useNotificationActions } from '../../hooks/notifications/useNotificationActions';
77

88
import { Title } from './Title';
99

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

19-
const { refetchNotifications: fetchNotifications } = useNotifications();
19+
const { refetchNotifications: fetchNotifications } = useNotificationActions();
2020

2121
return (
2222
<div className="pl-4 pr-5 pt-3 pb-1">

‎src/renderer/hooks/notifications/useNotificationActions.test.tsx‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
import { act, renderHook, waitFor } from '@testing-library/react';
2+
import type { ReactNode } from 'react';
3+
4+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
5+
6+
import { mockGitHubCloudAccount } from '../../__mocks__/account-mocks';
7+
import {
8+
mockGitifyNotification,
9+
mockSingleAccountNotifications,
10+
} from '../../__mocks__/notifications-mocks';
11+
12+
import { useAccountsStore } from '../../stores';
13+
14+
import { getAdapter } from '../../utils/forges/registry';
15+
import * as notificationsUtils from '../../utils/notifications/notifications';
16+
import { useNotifications } from '../useNotifications';
17+
import { useNotificationActions } from './useNotificationActions';
18+
19+
// Exercise the real hooks (globally mocked in vitest.setup)
20+
vi.unmock('./useNotificationActions');
21+
vi.unmock('../useNotifications');
22+
23+
vi.mock('../../utils/notifications/notifications', async () => {
24+
const actual = await vi.importActual<typeof import('../../utils/notifications/notifications')>(
25+
'../../utils/notifications/notifications',
26+
);
27+
return {
28+
...actual,
29+
getAllNotifications: vi.fn(),
30+
};
31+
});
32+
33+
const getAllNotificationsMock = vi.mocked(notificationsUtils.getAllNotifications);
34+
35+
const createWrapper = () => {
36+
const queryClient = new QueryClient({
37+
defaultOptions: {
38+
queries: {
39+
retry: false,
40+
refetchOnWindowFocus: false,
41+
refetchInterval: false,
42+
},
43+
},
44+
});
45+
46+
return {
47+
queryClient,
48+
wrapper: ({ children }: { children: ReactNode }) => (
49+
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
50+
),
51+
};
52+
};
53+
54+
describe('renderer/hooks/notifications/useNotificationActions.ts', () => {
55+
const githubAdapter = getAdapter('github');
56+
57+
beforeEach(() => {
58+
getAllNotificationsMock.mockReset();
59+
useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] });
60+
mockGitifyNotification.unread = true;
61+
});
62+
63+
it('exposes the notification actions without fetching or creating a query', () => {
64+
const { queryClient, wrapper } = createWrapper();
65+
66+
const { result } = renderHook(() => useNotificationActions(), { wrapper });
67+
68+
expect(result.current.markNotificationsAsRead).toBeTypeOf('function');
69+
expect(result.current.markNotificationsAsDone).toBeTypeOf('function');
70+
expect(result.current.unsubscribeNotification).toBeTypeOf('function');
71+
expect(result.current.removeAccountNotifications).toBeTypeOf('function');
72+
expect(result.current.refetchNotifications).toBeTypeOf('function');
73+
74+
expect(getAllNotificationsMock).not.toHaveBeenCalled();
75+
expect(queryClient.getQueryCache().getAll()).toHaveLength(0);
76+
});
77+
78+
it('does not add a notifications query observer', async () => {
79+
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
80+
const { queryClient, wrapper } = createWrapper();
81+
82+
renderHook(() => useNotificationActions(), { wrapper });
83+
expect(queryClient.getQueryCache().getAll()).toHaveLength(0);
84+
85+
renderHook(() => useNotifications(), { wrapper });
86+
await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(1));
87+
88+
// Only the reader subscribed; the action-only consumer added no observer.
89+
const queries = queryClient.getQueryCache().getAll();
90+
const observers = queries.flatMap((query) => query.observers);
91+
92+
expect(queries).toHaveLength(1);
93+
expect(observers).toHaveLength(1);
94+
});
95+
96+
it('refetchNotifications refetches the shared notifications query', async () => {
97+
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
98+
const { wrapper } = createWrapper();
99+
100+
renderHook(() => useNotifications(), { wrapper });
101+
await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(1));
102+
103+
const actions = renderHook(() => useNotificationActions(), { wrapper });
104+
await act(async () => {
105+
await actions.result.current.refetchNotifications();
106+
});
107+
108+
expect(getAllNotificationsMock).toHaveBeenCalledTimes(2);
109+
});
110+
111+
it('removeAccountNotifications prunes the account from the shared cache', async () => {
112+
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
113+
const { wrapper } = createWrapper();
114+
115+
const reader = renderHook(() => useNotifications(), { wrapper });
116+
await waitFor(() => expect(reader.result.current.hasNotifications).toBe(true));
117+
118+
const actions = renderHook(() => useNotificationActions(), { wrapper });
119+
act(() => {
120+
actions.result.current.removeAccountNotifications(mockGitHubCloudAccount);
121+
});
122+
123+
await waitFor(() => expect(reader.result.current.notificationCount).toBe(0));
124+
});
125+
126+
it('markNotificationsAsRead reaches the shared cache so a reader reflects it', async () => {
127+
vi.spyOn(githubAdapter.accountOps, 'markThreadAsRead').mockResolvedValue(undefined);
128+
getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications);
129+
const { wrapper } = createWrapper();
130+
131+
const reader = renderHook(() => useNotifications(), { wrapper });
132+
await waitFor(() => expect(reader.result.current.hasNotifications).toBe(true));
133+
134+
const actions = renderHook(() => useNotificationActions(), { wrapper });
135+
await act(async () => {
136+
await actions.result.current.markNotificationsAsRead([mockGitifyNotification]);
137+
});
138+
139+
await waitFor(() => expect(reader.result.current.notificationCount).toBe(0));
140+
});
141+
});

‎src/renderer/hooks/notifications/useNotificationActions.ts‎

Lines changed: 59 additions & 0 deletions

0 commit comments

Comments
 (0)