From ccdf9b929c6192885e111a145113eb6625938cbc Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 16 Oct 2026 08:16:49 -0400 Subject: [PATCH 1/3] 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. --- ARCHITECTURE.md | 2 + src/renderer/__helpers__/hook-mocks.ts | 19 +++ src/renderer/__helpers__/vitest.setup.ts | 11 ++ .../NotificationRow.subscription.test.tsx | 73 +++++++++ .../notifications/NotificationRow.tsx | 4 +- .../notifications/RepositoryHeader.tsx | 4 +- src/renderer/components/primitives/Header.tsx | 4 +- .../useNotificationActions.test.tsx | 141 ++++++++++++++++++ .../notifications/useNotificationActions.ts | 59 ++++++++ .../notifications/useNotificationsHost.ts | 6 +- .../notifications/useNotificationsQuery.ts | 55 +------ .../useNotificationsQueryKey.test.ts | 64 ++++++++ .../notifications/useNotificationsQueryKey.ts | 32 ++++ src/renderer/hooks/useLogins.ts | 4 +- src/renderer/hooks/useNotifications.test.tsx | 1 + src/renderer/hooks/useNotifications.ts | 27 ++-- 16 files changed, 435 insertions(+), 71 deletions(-) create mode 100644 src/renderer/components/notifications/NotificationRow.subscription.test.tsx create mode 100644 src/renderer/hooks/notifications/useNotificationActions.test.tsx create mode 100644 src/renderer/hooks/notifications/useNotificationActions.ts create mode 100644 src/renderer/hooks/notifications/useNotificationsQueryKey.test.ts create mode 100644 src/renderer/hooks/notifications/useNotificationsQueryKey.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index be4ca2da6..f5e89f569 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -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. diff --git a/src/renderer/__helpers__/hook-mocks.ts b/src/renderer/__helpers__/hook-mocks.ts index fcff10597..45ac3a13a 100644 --- a/src/renderer/__helpers__/hook-mocks.ts +++ b/src/renderer/__helpers__/hook-mocks.ts @@ -1,3 +1,4 @@ +import type { useNotificationActions } from '../hooks/notifications/useNotificationActions'; import type { useLogins } from '../hooks/useLogins'; import type { useNotifications } from '../hooks/useNotifications'; @@ -15,6 +16,7 @@ import type { useNotifications } from '../hooks/useNotifications'; export type NotificationsState = ReturnType; export type LoginsState = ReturnType; +export type NotificationActionsState = ReturnType; let notificationsDefaults: NotificationsState | null = null; let notificationsOverrides: Partial = {}; @@ -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(); diff --git a/src/renderer/__helpers__/vitest.setup.ts b/src/renderer/__helpers__/vitest.setup.ts index 87d475fa6..62f733325 100644 --- a/src/renderer/__helpers__/vitest.setup.ts +++ b/src/renderer/__helpers__/vitest.setup.ts @@ -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') diff --git a/src/renderer/components/notifications/NotificationRow.subscription.test.tsx b/src/renderer/components/notifications/NotificationRow.subscription.test.tsx new file mode 100644 index 000000000..ba991bca5 --- /dev/null +++ b/src/renderer/components/notifications/NotificationRow.subscription.test.tsx @@ -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 }) => ( + {children} + ), + }; +}; + +describe('renderer/components/notifications/NotificationRow.tsx - subscriptions', () => { + it('renders without subscribing to the notifications query', () => { + const { queryClient, wrapper } = createWrapper(); + + render( + , + { 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) => ( + + ))} + , + { wrapper }, + ); + + const observers = queryClient + .getQueryCache() + .getAll() + .flatMap((query) => query.observers); + + expect(observers).toHaveLength(0); + }); +}); diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index 23adfe5b9..6e198b338 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -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, @@ -38,7 +38,7 @@ export function NotificationRow({ onAnimateExit, }: Readonly) { const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } = - useNotifications(); + useNotificationActions(); const markAsDoneOnOpen = useSettingsStore((s) => s.markAsDoneOnOpen); const wrapNotificationTitle = useSettingsStore((s) => s.wrapNotificationTitle); diff --git a/src/renderer/components/notifications/RepositoryHeader.tsx b/src/renderer/components/notifications/RepositoryHeader.tsx index e79e0047b..d91344855 100644 --- a/src/renderer/components/notifications/RepositoryHeader.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.tsx @@ -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'; @@ -35,7 +35,7 @@ export function RepositoryHeader({ onToggle, onAnimateExit, }: Readonly) { - const { markNotificationsAsRead, markNotificationsAsDone } = useNotifications(); + const { markNotificationsAsRead, markNotificationsAsDone } = useNotificationActions(); const shouldAnimateExit = shouldRemoveNotificationsFromState(); diff --git a/src/renderer/components/primitives/Header.tsx b/src/renderer/components/primitives/Header.tsx index 585ffb39f..6efc61c92 100644 --- a/src/renderer/components/primitives/Header.tsx +++ b/src/renderer/components/primitives/Header.tsx @@ -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'; @@ -16,7 +16,7 @@ interface HeaderProps { export function Header(props: Readonly) { const navigate = useNavigate(); - const { refetchNotifications: fetchNotifications } = useNotifications(); + const { refetchNotifications: fetchNotifications } = useNotificationActions(); return (
diff --git a/src/renderer/hooks/notifications/useNotificationActions.test.tsx b/src/renderer/hooks/notifications/useNotificationActions.test.tsx new file mode 100644 index 000000000..4acf3f5ec --- /dev/null +++ b/src/renderer/hooks/notifications/useNotificationActions.test.tsx @@ -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( + '../../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 }) => ( + {children} + ), + }; +}; + +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)); + }); +}); diff --git a/src/renderer/hooks/notifications/useNotificationActions.ts b/src/renderer/hooks/notifications/useNotificationActions.ts new file mode 100644 index 000000000..6f00230e8 --- /dev/null +++ b/src/renderer/hooks/notifications/useNotificationActions.ts @@ -0,0 +1,59 @@ +import { useCallback } from 'react'; + +import { useQueryClient } from '@tanstack/react-query'; + +import type { Account, AccountNotifications } from '../../types'; + +import { getAccountUUID } from '../../utils/auth/utils'; +import { type NotificationMutations, useNotificationMutations } from './useNotificationMutations'; +import { useNotificationsQueryKey } from './useNotificationsQueryKey'; + +export interface NotificationActions extends NotificationMutations { + removeAccountNotifications: (account: Account) => void; + refetchNotifications: () => Promise; +} + +/** + * The action side of notification state, for consumers that perform actions but + * do not read notification data. + * + * Unlike `useNotificationsQuery`, it never calls `useQuery`, so a consumer that + * only needs these actions does not become an observer of the notifications + * query. That matters most for `NotificationRow`, which mounts once per visible + * item: as an observer it would re-run the query's `select` and structural-sharing + * walk once per row on every cache write. + * + * The key is derived from the same stores the reader uses, so both target the + * same cache entry. + */ +export const useNotificationActions = (): NotificationActions => { + const queryClient = useQueryClient(); + const notificationsQueryKey = useNotificationsQueryKey(); + + const mutations = useNotificationMutations(notificationsQueryKey); + + const removeAccountNotifications = useCallback( + (account: Account) => { + const accountUUID = getAccountUUID(account); + + queryClient.setQueryData( + notificationsQueryKey, + (existing) => + existing?.filter( + (accountNotifications) => getAccountUUID(accountNotifications.account) !== accountUUID, + ) ?? [], + ); + }, + [queryClient, notificationsQueryKey], + ); + + const refetchNotifications = useCallback(async () => { + await queryClient.refetchQueries({ queryKey: notificationsQueryKey }); + }, [queryClient, notificationsQueryKey]); + + return { + ...mutations, + removeAccountNotifications, + refetchNotifications, + }; +}; diff --git a/src/renderer/hooks/notifications/useNotificationsHost.ts b/src/renderer/hooks/notifications/useNotificationsHost.ts index 3de9f579c..1aeebb1d6 100644 --- a/src/renderer/hooks/notifications/useNotificationsHost.ts +++ b/src/renderer/hooks/notifications/useNotificationsHost.ts @@ -1,6 +1,6 @@ import { buildNotificationsState, type NotificationsState } from '../useNotifications'; +import { useNotificationActions } from './useNotificationActions'; import { useNotificationEffects } from './useNotificationEffects'; -import { useNotificationMutations } from './useNotificationMutations'; import { useNotificationsQuery } from './useNotificationsQuery'; /** @@ -15,7 +15,7 @@ import { useNotificationsQuery } from './useNotificationsQuery'; */ export const useNotificationsHost = (): NotificationsState => { const query = useNotificationsQuery({ poll: true }); - const mutations = useNotificationMutations(query.notificationsQueryKey); + const actions = useNotificationActions(); useNotificationEffects({ enabled: true, @@ -27,5 +27,5 @@ export const useNotificationsHost = (): NotificationsState => { fetchIntervalMs: query.fetchIntervalMs, }); - return buildNotificationsState(query, mutations); + return buildNotificationsState(query, actions); }; diff --git a/src/renderer/hooks/notifications/useNotificationsQuery.ts b/src/renderer/hooks/notifications/useNotificationsQuery.ts index 8810a1e0f..0e495bcba 100644 --- a/src/renderer/hooks/notifications/useNotificationsQuery.ts +++ b/src/renderer/hooks/notifications/useNotificationsQuery.ts @@ -1,21 +1,13 @@ -import { useCallback, useMemo } from 'react'; +import { useMemo } from 'react'; -import { keepPreviousData, type QueryKey, useQuery, useQueryClient } from '@tanstack/react-query'; +import { keepPreviousData, type QueryKey, useQuery } from '@tanstack/react-query'; import { Constants } from '../../constants'; import { useAccountsStore, useFiltersStore, useSettingsStore } from '../../stores'; -import type { - Account, - AccountNotifications, - GitifyError, - GitifyNotification, - Status, -} from '../../types'; +import type { AccountNotifications, GitifyError, GitifyNotification, Status } from '../../types'; -import { notificationsKeys } from '../../utils/api/queryKeys'; -import { getAccountUUID } from '../../utils/auth/utils'; import { filterBaseNotifications, filterDetailedNotifications, @@ -27,6 +19,7 @@ import { } from '../../utils/notifications/notifications'; import { computeRefetchIntervalMs } from '../../utils/notifications/pollInterval'; import { deriveGlobalError, deriveStatus } from '../../utils/notifications/status'; +import { useNotificationsQueryKey } from './useNotificationsQueryKey'; export interface NotificationsQueryResult { notifications: AccountNotifications[]; @@ -42,8 +35,6 @@ export interface NotificationsQueryResult { notificationsQueryKey: QueryKey; fetchIntervalMs: number; refetch: () => void; - refetchNotifications: () => Promise; - removeAccountNotifications: (account: Account) => void; } /** @@ -56,8 +47,6 @@ export interface NotificationsQueryResult { * entry but schedule nothing. See `useNotificationsHost`. */ export const useNotificationsQuery = ({ poll }: { poll: boolean }): NotificationsQueryResult => { - const queryClient = useQueryClient(); - // Account store values const accounts = useAccountsStore((s) => s.accounts); @@ -72,21 +61,12 @@ export const useNotificationsQuery = ({ poll }: { poll: boolean }): Notification const reviewRequestTypes = useFiltersStore((s) => s.reviewRequestTypes); // Setting store values - const fetchReadNotifications = useSettingsStore((s) => s.fetchReadNotifications); - const fetchParticipatingNotifications = useSettingsStore((s) => s.participating); const fetchIntervalMs = useSettingsStore((s) => s.fetchInterval); // Query key excludes filters: the cache holds unfiltered data and filter - // changes re-run the select below, applying instantly without refetching - const notificationsQueryKey = useMemo( - () => - notificationsKeys.list( - accounts.map(getAccountUUID), - fetchReadNotifications, - fetchParticipatingNotifications, - ), - [accounts, fetchReadNotifications, fetchParticipatingNotifications], - ); + // changes re-run the select below, applying instantly without refetching. + // Shared with the action-only surface so both target the same cache entry. + const notificationsQueryKey = useNotificationsQueryKey(); // Create select function that depends on filter state const selectFilteredNotifications = useMemo( @@ -175,25 +155,6 @@ export const useNotificationsQuery = ({ poll }: { poll: boolean }): Notification [isPaused, notifications], ); - const refetchNotifications = useCallback(async () => { - await refetch(); - }, [refetch]); - - const removeAccountNotifications = useCallback( - (account: Account) => { - const accountUUID = getAccountUUID(account); - - queryClient.setQueryData( - notificationsQueryKey, - (existing) => - existing?.filter( - (accountNotifications) => getAccountUUID(accountNotifications.account) !== accountUUID, - ) ?? [], - ); - }, - [queryClient, notificationsQueryKey], - ); - return { notifications, isLoading, @@ -208,7 +169,5 @@ export const useNotificationsQuery = ({ poll }: { poll: boolean }): Notification notificationsQueryKey, fetchIntervalMs, refetch, - refetchNotifications, - removeAccountNotifications, }; }; diff --git a/src/renderer/hooks/notifications/useNotificationsQueryKey.test.ts b/src/renderer/hooks/notifications/useNotificationsQueryKey.test.ts new file mode 100644 index 000000000..d61530d94 --- /dev/null +++ b/src/renderer/hooks/notifications/useNotificationsQueryKey.test.ts @@ -0,0 +1,64 @@ +import { act, renderHook } from '@testing-library/react'; + +import { + mockGitHubCloudAccount, + mockGitHubEnterpriseServerAccount, +} from '../../__mocks__/account-mocks'; + +import { useAccountsStore, useSettingsStore } from '../../stores'; + +import { notificationsKeys } from '../../utils/api/queryKeys'; +import { getAccountUUID } from '../../utils/auth/utils'; +import { useNotificationsQueryKey } from './useNotificationsQueryKey'; + +describe('renderer/hooks/notifications/useNotificationsQueryKey.ts', () => { + it('derives the notifications list key from the accounts and settings stores', () => { + useAccountsStore.setState({ + accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], + }); + useSettingsStore.setState({ fetchReadNotifications: true, participating: true }); + + const { result } = renderHook(() => useNotificationsQueryKey()); + + expect(result.current).toEqual( + notificationsKeys.list( + [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount].map(getAccountUUID), + true, + true, + ), + ); + }); + + it('keeps the same key reference while the source state is unchanged', () => { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + + const { result, rerender } = renderHook(() => useNotificationsQueryKey()); + const initialKey = result.current; + + rerender(); + + expect(result.current).toBe(initialKey); + }); + + it('recomputes the key when an account is added', () => { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + + const { result } = renderHook(() => useNotificationsQueryKey()); + const initialKey = result.current; + + act(() => { + useAccountsStore.setState({ + accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], + }); + }); + + expect(result.current).toEqual( + notificationsKeys.list( + [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount].map(getAccountUUID), + useSettingsStore.getState().fetchReadNotifications, + useSettingsStore.getState().participating, + ), + ); + expect(result.current).not.toBe(initialKey); + }); +}); diff --git a/src/renderer/hooks/notifications/useNotificationsQueryKey.ts b/src/renderer/hooks/notifications/useNotificationsQueryKey.ts new file mode 100644 index 000000000..2a45bad32 --- /dev/null +++ b/src/renderer/hooks/notifications/useNotificationsQueryKey.ts @@ -0,0 +1,32 @@ +import { useMemo } from 'react'; + +import type { QueryKey } from '@tanstack/react-query'; + +import { useAccountsStore, useSettingsStore } from '../../stores'; + +import { notificationsKeys } from '../../utils/api/queryKeys'; +import { getAccountUUID } from '../../utils/auth/utils'; + +/** + * The query key for the notifications list, derived from the stores that + * parameterise it rather than from the query observer itself. + * + * Kept separate from `useNotificationsQuery` so that consumers which only need + * notification actions - and so must not subscribe to notification data - can + * still target the exact same cache entry as the readers. + */ +export const useNotificationsQueryKey = (): QueryKey => { + const accounts = useAccountsStore((s) => s.accounts); + const fetchReadNotifications = useSettingsStore((s) => s.fetchReadNotifications); + const fetchParticipatingNotifications = useSettingsStore((s) => s.participating); + + return useMemo( + () => + notificationsKeys.list( + accounts.map(getAccountUUID), + fetchReadNotifications, + fetchParticipatingNotifications, + ), + [accounts, fetchReadNotifications, fetchParticipatingNotifications], + ); +}; diff --git a/src/renderer/hooks/useLogins.ts b/src/renderer/hooks/useLogins.ts index 24ac225b4..c119fc08f 100644 --- a/src/renderer/hooks/useLogins.ts +++ b/src/renderer/hooks/useLogins.ts @@ -14,7 +14,7 @@ import type { import { notificationsKeys } from '../utils/api/queryKeys'; import { getAccountAdapter, getAdapter } from '../utils/forges/registry'; import { encryptValue } from '../utils/system/comms'; -import { useNotifications } from './useNotifications'; +import { useNotificationActions } from './notifications/useNotificationActions'; interface LoginsState { loginWithDeviceFlowStart: ( @@ -43,7 +43,7 @@ export const useLogins = (): LoginsState => { const createAccountInStore = useAccountsStore((s) => s.createAccount); const removeAccount = useAccountsStore((s) => s.removeAccount); - const { removeAccountNotifications } = useNotifications(); + const { removeAccountNotifications } = useNotificationActions(); /** * Create (or update) an account, then refetch notifications. Re-authenticating diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 091d8456e..22e8930b9 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -39,6 +39,7 @@ import { useNotifications } from './useNotifications'; // Exercise the real hook implementation (globally mocked in vitest.setup) vi.unmock('./useNotifications'); +vi.unmock('./notifications/useNotificationActions'); vi.unmock('./notifications/useNotificationsHost'); vi.mock('../utils/notifications/notifications', async () => { diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 66fdc744d..a567c42aa 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -7,9 +7,9 @@ import type { } from '../types'; import { - type NotificationMutations, - useNotificationMutations, -} from './notifications/useNotificationMutations'; + type NotificationActions, + useNotificationActions, +} from './notifications/useNotificationActions'; import { type NotificationsQueryResult, useNotificationsQuery, @@ -38,12 +38,12 @@ export interface NotificationsState { /** * Assemble the public notification reader/actions surface from the query and - * mutation halves. Shared by the reader hook and the host hook so both expose + * actions halves. Shared by the reader hook and the host hook so both expose * an identical API. */ export const buildNotificationsState = ( query: NotificationsQueryResult, - mutations: NotificationMutations, + actions: NotificationActions, ): NotificationsState => ({ status: query.status, globalError: query.globalError, @@ -56,12 +56,12 @@ export const buildNotificationsState = ( hasNotifications: query.hasNotifications, hasUnreadNotifications: query.hasUnreadNotifications, - refetchNotifications: query.refetchNotifications, - removeAccountNotifications: query.removeAccountNotifications, + refetchNotifications: actions.refetchNotifications, + removeAccountNotifications: actions.removeAccountNotifications, - markNotificationsAsRead: mutations.markNotificationsAsRead, - markNotificationsAsDone: mutations.markNotificationsAsDone, - unsubscribeNotification: mutations.unsubscribeNotification, + markNotificationsAsRead: actions.markNotificationsAsRead, + markNotificationsAsDone: actions.markNotificationsAsDone, + unsubscribeNotification: actions.unsubscribeNotification, }); /** @@ -71,11 +71,14 @@ export const buildNotificationsState = ( * listeners, and shares the query cache with every other consumer. The * app-level host (`useNotificationsHost`) owns those side effects. * + * A consumer that only needs the actions should use `useNotificationActions` + * directly so it does not subscribe to notification data. + * * @returns The current notifications state and action callbacks. */ export const useNotifications = (): NotificationsState => { const query = useNotificationsQuery({ poll: false }); - const mutations = useNotificationMutations(query.notificationsQueryKey); + const actions = useNotificationActions(); - return buildNotificationsState(query, mutations); + return buildNotificationsState(query, actions); }; From 7f7cbd6bf4a10371643cb58d9e2b9172f878d75e Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 16 Oct 2026 08:20:56 -0400 Subject: [PATCH 2/3] fix(test): mock useNotificationActions in the visual setup The visual setup mirrors the app-hook mocks from vitest.setup.ts, but the new useNotificationActions mock was only added to vitest.setup.ts. Visual tests therefore rendered NotificationRow and RepositoryHeader against the real actions hook, so `setNotificationsOverrides({ markNotificationsAsRead })` never applied: the row stayed out of its pending/exit state and the real mutation attempted a network request that the visual setup rejects. Add the matching mock so action-only consumers resolve mocked actions in the visual project too. --- src/renderer/__helpers__/visual.setup.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/renderer/__helpers__/visual.setup.ts b/src/renderer/__helpers__/visual.setup.ts index c6bda06ec..fddd2c2bd 100644 --- a/src/renderer/__helpers__/visual.setup.ts +++ b/src/renderer/__helpers__/visual.setup.ts @@ -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('../hooks/useLogins'); const { getMockedLoginsState } = await import('./hook-mocks'); From 3e9f48b76588c5949f3755bb83dc5b5801b2c1ff Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 16 Oct 2026 22:07:30 -0400 Subject: [PATCH 3/3] vite rule Signed-off-by: Adam Setch --- vite.config.mts | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/vite.config.mts b/vite.config.mts index 33b9bbb6a..1f56690df 100644 --- a/vite.config.mts +++ b/vite.config.mts @@ -282,6 +282,38 @@ export default defineConfig({ ], }, }, + { + // Renderer boundary: the notification UI renders once per notification + // (rows, repository headers, account headers) and receives its data via + // props from the route. It must not subscribe to the notifications query: + // every observer re-runs `select` and the structural-sharing walk on each + // cache write, so subscribing here multiplies that cost by the number of + // mounted items. Action-only consumers use `useNotificationActions`. + // See ARCHITECTURE.md. + files: [ + 'src/renderer/components/notifications/**/*.ts', + 'src/renderer/components/notifications/**/*.tsx', + ], + excludeFiles: ['**/*.test.ts', '**/*.test.tsx'], + rules: { + 'no-restricted-imports': [ + 'error', + { + patterns: [ + { + group: [ + '**/hooks/useNotifications', + '**/hooks/notifications/useNotificationsQuery', + '**/hooks/notifications/useNotificationsHost', + ], + message: + 'Notification UI must not subscribe to the notifications query; it receives data via props. Use `useNotificationActions` for actions (see ARCHITECTURE.md).', + }, + ], + }, + ], + }, + }, { // Renderer boundary: `icons` is a presentational leaf. It may not // depend on any implementation layer. See ARCHITECTURE.md.