diff --git a/src/preload/index.ts b/src/preload/index.ts index 056df22bf..e3f137a77 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -185,9 +185,10 @@ export const api = { * Register a callback invoked when the main process requests an app reset. * * @param callback - Called when the reset event is received. + * @returns A function that removes the listener. */ onResetApp: (callback: () => void) => { - onRendererEvent(EVENTS.RESET_APP, () => callback()); + return onRendererEvent(EVENTS.RESET_APP, () => callback()); }, /** @@ -198,18 +199,20 @@ export const api = { * waiting for the next scheduled poll interval. * * @param callback - Called with no arguments on every wake event. + * @returns A function that removes the listener. */ onSystemWake: (callback: () => void) => { - onRendererEvent(EVENTS.SYSTEM_WAKE, () => callback()); + return onRendererEvent(EVENTS.SYSTEM_WAKE, () => callback()); }, /** * Register a callback invoked when the main process delivers an OAuth callback URL. * * @param callback - Called with the full callback URL (e.g. `gitify://oauth?code=...`). + * @returns A function that removes the listener. */ onAuthCallback: (callback: (url: string) => void) => { - onRendererEvent(EVENTS.AUTH_CALLBACK, (_, url) => callback(url)); + return onRendererEvent(EVENTS.AUTH_CALLBACK, (_, url) => callback(url)); }, /** diff --git a/src/preload/utils.test.ts b/src/preload/utils.test.ts index 069f4e59f..d0b3a2e9f 100644 --- a/src/preload/utils.test.ts +++ b/src/preload/utils.test.ts @@ -15,7 +15,15 @@ vi.mock('electron', () => { listeners[channel].push(listener); return this; }), - } satisfies Pick; + removeListener: vi.fn(function ( + this: Electron.IpcRenderer, + channel: string, + listener: Listener, + ) { + listeners[channel] = (listeners[channel] || []).filter((l) => l !== listener); + return this; + }), + } satisfies Pick; return { ipcRenderer: { ...ipcRendererStub, @@ -67,4 +75,19 @@ describe('preload/utils', () => { expect(ipcRenderer.on).toHaveBeenCalledWith(EVENTS.UPDATE_ICON_TITLE, handlerMock); expect(handlerMock).toHaveBeenCalledWith({}, 'payload'); }); + + it('onRendererEvent returns an unsubscribe function that removes the listener', () => { + const handlerMock = vi.fn(); + const unsubscribe = onRendererEvent(EVENTS.UPDATE_ICON_TITLE, handlerMock); + + unsubscribe(); + ( + ipcRenderer as unknown as { + __emit: (channel: string, ...a: unknown[]) => void; + } + ).__emit(EVENTS.UPDATE_ICON_TITLE, 'payload'); + + expect(ipcRenderer.removeListener).toHaveBeenCalledWith(EVENTS.UPDATE_ICON_TITLE, handlerMock); + expect(handlerMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/preload/utils.ts b/src/preload/utils.ts index 3b38a3613..8418a5f73 100644 --- a/src/preload/utils.ts +++ b/src/preload/utils.ts @@ -29,10 +29,19 @@ export async function invokeMainEvent( /** * Register a listener for an IPC event sent from the main process to the renderer. + * + * @returns A function that removes the listener. Callers that register + * listeners from React effects must return it as the effect cleanup, + * otherwise every remount leaks a permanent ipcRenderer listener. */ export function onRendererEvent( event: E, listener: (event: Electron.IpcRendererEvent, data: EventRequest) => void, -): void { - ipcRenderer.on(event, listener as Parameters[1]); +): () => void { + const typedListener = listener as Parameters[1]; + ipcRenderer.on(event, typedListener); + + return () => { + ipcRenderer.removeListener(event, typedListener); + }; } diff --git a/src/renderer/__helpers__/vitest.setup.ts b/src/renderer/__helpers__/vitest.setup.ts index 6c3128322..8587efbdf 100644 --- a/src/renderer/__helpers__/vitest.setup.ts +++ b/src/renderer/__helpers__/vitest.setup.ts @@ -100,9 +100,9 @@ function createGitifyBridgeApi(): Window['gitify'] { useUnreadActiveIcon: vi.fn(), }, notificationSoundPath: vi.fn(), - onAuthCallback: vi.fn(), - onResetApp: vi.fn(), - onSystemWake: vi.fn(), + onAuthCallback: vi.fn(() => vi.fn()), + onResetApp: vi.fn(() => vi.fn()), + onSystemWake: vi.fn(() => vi.fn()), setAutoLaunch: vi.fn(), setKeepWindowOnBlur: vi.fn(), applyKeyboardShortcut: vi.fn().mockResolvedValue({ success: true }), diff --git a/src/renderer/components/GlobalEffects.tsx b/src/renderer/components/GlobalEffects.tsx index 796707d73..5b2b77945 100644 --- a/src/renderer/components/GlobalEffects.tsx +++ b/src/renderer/components/GlobalEffects.tsx @@ -93,7 +93,7 @@ export const GlobalEffects: FC = () => { ]); useEffect(() => { - window.gitify.onResetApp(() => { + return window.gitify.onResetApp(() => { resetAccounts(); resetSettings(); resetFilters(); diff --git a/src/renderer/constants.ts b/src/renderer/constants.ts index 8f37f26ac..87e88f476 100644 --- a/src/renderer/constants.ts +++ b/src/renderer/constants.ts @@ -84,6 +84,9 @@ export const Constants = { // Query stale time in milliseconds, used by TanStack Query client QUERY_STALE_TIME_MS: 30 * 1000, // 30 seconds + // Cooldown before retrying a failed query, used by TanStack Query client + QUERY_RETRY_DELAY_MS: 30 * 1000, // 30 seconds + // GraphQL Argument Defaults GRAPHQL_ARGS: { FIRST_LABELS: 100, diff --git a/src/renderer/hooks/useAccounts.ts b/src/renderer/hooks/useAccounts.ts index cc9e0bf4f..2d5d1fa0d 100644 --- a/src/renderer/hooks/useAccounts.ts +++ b/src/renderer/hooks/useAccounts.ts @@ -53,7 +53,7 @@ export const useAccounts = (): AccountsState => { // Refetch accounts immediately when the system wakes from sleep or the user // unlocks their screen, without waiting for the next scheduled interval. useEffect(() => { - window.gitify.onSystemWake(() => { + return window.gitify.onSystemWake(() => { refetch(); }); }, [refetch]); diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index acc83fca5..a8baf72f6 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -16,6 +16,7 @@ import { import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; import type { AccountNotifications, Percentage } from '../types'; +import { FetchType } from '../types'; import { Errors } from '../utils/core/errors'; import * as logger from '../utils/core/logger'; @@ -174,6 +175,71 @@ describe('renderer/hooks/useNotifications.ts', () => { }); }); + describe('polling', () => { + it('only polls once per interval, regardless of consumer count', async () => { + vi.useFakeTimers(); + try { + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + useSettingsStore.setState({ + fetchType: FetchType.INTERVAL, + fetchInterval: 1000, + }); + + // A single shared wrapper so all consumers share one query cache + const wrapper = createWrapper(); + + // Singleton side-effects host (GlobalEffects) + renderHook(() => useNotifications({ withSideEffects: true }), { wrapper }); + + // Plain consumers (notification rows, repo groups, sidebar, etc.) + // mount staggered over time, like real components rendering across + // frames. With per-observer polling each schedules its own interval + // timer on a different offset, so their fetches cannot dedupe. + for (let i = 0; i < 3; i++) { + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + renderHook(() => useNotifications(), { wrapper }); + } + + await act(async () => { + await vi.advanceTimersByTimeAsync(3_000); + }); + + // 1 initial fetch + 3 interval polls. Without polling ownership the + // staggered consumers would each poll on their own timer (~10 extra + // fetches here, and far more in a full notification list). + expect(getAllNotificationsMock).toHaveBeenCalledTimes(4); + } finally { + vi.useRealTimers(); + } + }); + + it('plain consumers do not poll on their own', async () => { + vi.useFakeTimers(); + try { + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + useSettingsStore.setState({ + fetchType: FetchType.INTERVAL, + fetchInterval: 1000, + }); + + renderHook(() => useNotifications(), { wrapper: createWrapper() }); + + await act(async () => { + await vi.advanceTimersByTimeAsync(3_500); + }); + + // Initial fetch only; no interval polls without the side-effects host + expect(getAllNotificationsMock).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + }); + describe('sound and native notifications', () => { it('raises sound and native notifications for new notifications', async () => { useSettingsStore.setState({ @@ -386,5 +452,26 @@ describe('renderer/hooks/useNotifications.ts', () => { await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(2)); }); + + it('only the side-effects host registers a wake listener', async () => { + vi.mocked(window.gitify.onSystemWake).mockClear(); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const wrapper = createWrapper(); + + // Plain consumers (notification rows, repo groups, sidebar, etc.) + renderHook(() => useNotifications(), { wrapper }); + renderHook(() => useNotifications(), { wrapper }); + + await waitFor(() => expect(getAllNotificationsMock).toHaveBeenCalledTimes(1)); + + // Each consumer registering its own listener would leak an ipcRenderer + // listener per mounted row and multiply wake-triggered refetches. + expect(vi.mocked(window.gitify.onSystemWake)).not.toHaveBeenCalled(); + + renderHook(() => useNotifications({ withSideEffects: true }), { wrapper }); + + expect(vi.mocked(window.gitify.onSystemWake)).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 092dab94b..83aa849f9 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -54,9 +54,15 @@ interface NotificationsState { interface UseNotificationsOptions { /** - * Run the singleton side effects (sound/native alerts for new notifications - * and inactivity-based refetching). Only the app-level effects host should - * enable this; all other consumers share the query cache without them. + * Run the singleton side effects (sound/native alerts for new notifications, + * inactivity-based refetching, and interval/focus/reconnect polling). Only + * the app-level effects host should enable this; all other consumers share + * the query cache without them. + * + * Polling is owned by the singleton host because TanStack Query schedules + * `refetchInterval` per mounted observer, not per query. Every consumer + * (each notification row, repo group, sidebar, etc.) would otherwise run + * its own staggered poll timer, multiplying API requests. */ withSideEffects?: boolean; } @@ -154,9 +160,12 @@ export const useNotifications = ({ placeholderData: keepPreviousData, - refetchInterval: fetchType === FetchType.INTERVAL ? fetchIntervalMs : false, - refetchOnReconnect: true, - refetchOnWindowFocus: true, + // Only the singleton side-effects host polls. Other consumers share the + // cached data and would otherwise each schedule their own refetch timer. + refetchInterval: withSideEffects && fetchType === FetchType.INTERVAL ? fetchIntervalMs : false, + refetchOnMount: withSideEffects, + refetchOnReconnect: withSideEffects, + refetchOnWindowFocus: withSideEffects, }); // Inactivity-based fetching re-fetches once the user has been idle for the @@ -229,11 +238,18 @@ export const useNotifications = ({ // Refetch notifications immediately when the system wakes from sleep or the // user unlocks their screen, without waiting for the next poll interval. + // Owned by the singleton side-effects host: every consumer registering a + // wake listener would both multiply refetches and leak ipcRenderer + // listeners on unmount. useEffect(() => { - window.gitify.onSystemWake(() => { + if (!withSideEffects) { + return; + } + + return window.gitify.onSystemWake(() => { refetch(); }); - }, [refetch]); + }, [withSideEffects, refetch]); const removeAccountNotifications = useCallback( async (account: Account) => { diff --git a/src/renderer/hooks/useOnlineStatus.ts b/src/renderer/hooks/useOnlineStatus.ts index 852c904dd..d99717ded 100644 --- a/src/renderer/hooks/useOnlineStatus.ts +++ b/src/renderer/hooks/useOnlineStatus.ts @@ -22,9 +22,14 @@ export function useOnlineStatus(): boolean { // Re-probe network state when the system wakes from sleep or the user // unlocks their screen. The browser's online/offline events may not have // fired yet by the time the renderer runs after a sleep cycle. - window.gitify.onSystemWake(() => onlineManager.setOnline(navigator.onLine)); + const unsubscribeWake = window.gitify.onSystemWake(() => + onlineManager.setOnline(navigator.onLine), + ); - return () => unsubscribe(); + return () => { + unsubscribe(); + unsubscribeWake(); + }; }, []); return isOnline; diff --git a/src/renderer/utils/api/queryClient.ts b/src/renderer/utils/api/queryClient.ts index cc0b2337c..0dd974e9b 100644 --- a/src/renderer/utils/api/queryClient.ts +++ b/src/renderer/utils/api/queryClient.ts @@ -5,12 +5,16 @@ import { Constants } from '../../constants'; /** * TanStack Query client for all API state. * - * Retries are handled here rather than at the HTTP client layer. + * Retries are handled here rather than at the HTTP client layer. Failed + * queries retry once after a cooldown instead of the default near-instant + * backoff, so a struggling GitHub/GHES instance is not hammered with + * back-to-back polls while it recovers. */ export const queryClient = new QueryClient({ defaultOptions: { queries: { retry: 1, + retryDelay: Constants.QUERY_RETRY_DELAY_MS, refetchIntervalInBackground: true, staleTime: Constants.QUERY_STALE_TIME_MS, networkMode: 'online', diff --git a/src/renderer/utils/forges/github/flows.ts b/src/renderer/utils/forges/github/flows.ts index e13a2393b..b054a8611 100644 --- a/src/renderer/utils/forges/github/flows.ts +++ b/src/renderer/utils/forges/github/flows.ts @@ -46,8 +46,20 @@ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promis openExternalLink(url as Link); + // Assigned once the listener is registered below; optional-chained in + // `handleCallback` so a synchronously-firing callback cannot hit the + // temporal dead zone. + let unsubscribeAuthCallback: (() => void) | undefined; + const handleCallback = (callbackUrl: string) => { - const url = new URL(callbackUrl); + let url: URL; + try { + url = new URL(callbackUrl); + } catch { + unsubscribeAuthCallback?.(); + reject(new Error(`Received an invalid authentication callback URL: ${callbackUrl}`)); + return; + } const type = url.hostname; const code = url.searchParams.get('code'); @@ -56,12 +68,14 @@ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promis const errorUri = url.searchParams.get('error_uri'); if (code && type === 'oauth') { + unsubscribeAuthCallback?.(); resolve({ authMethod: 'OAuth App', authCode: code as AuthCode, authOptions: authOptions, }); } else if (error) { + unsubscribeAuthCallback?.(); reject( new Error( `Oops! Something went wrong and we couldn't log you in using GitHub. Please try again. Reason: ${errorDescription} Docs: ${errorUri}`, @@ -70,7 +84,9 @@ export function performGitHubWebOAuth(authOptions: LoginOAuthWebOptions): Promis } }; - window.gitify.onAuthCallback((callbackUrl: string) => { + // Unsubscribed inside `handleCallback` once the flow settles so repeated + // login attempts do not accumulate stale ipcRenderer listeners. + unsubscribeAuthCallback = window.gitify.onAuthCallback((callbackUrl: string) => { rendererLogInfo( 'renderer:auth-callback', `received authentication callback URL ${callbackUrl}`,