From 2e5bf34fe94eb934a634e78395fce554e8bcffd7 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Wed, 29 Jul 2026 17:11:35 +0200 Subject: [PATCH] perf(notifications): respect server-recommended poll interval --- src/renderer/hooks/useNotifications.test.tsx | 46 ++++++++++++++ src/renderer/hooks/useNotifications.ts | 7 ++- .../utils/forges/github/client.test.ts | 41 ++++++++++++ src/renderer/utils/forges/github/client.ts | 29 ++++++--- .../utils/notifications/pollInterval.test.ts | 62 +++++++++++++++++++ .../utils/notifications/pollInterval.ts | 61 ++++++++++++++++++ 6 files changed, 238 insertions(+), 8 deletions(-) create mode 100644 src/renderer/utils/notifications/pollInterval.test.ts create mode 100644 src/renderer/utils/notifications/pollInterval.ts diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 022e1272f..6812a3e99 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -21,6 +21,10 @@ import { Errors } from '../utils/core/errors'; import * as logger from '../utils/core/logger'; import { getAdapter } from '../utils/forges/registry'; import * as notificationsUtils from '../utils/notifications/notifications'; +import { + clearServerPollIntervals, + reportServerPollInterval, +} from '../utils/notifications/pollInterval'; import * as audio from '../utils/system/audio'; import * as native from '../utils/system/native'; import { useNotifications } from './useNotifications'; @@ -77,6 +81,7 @@ describe('renderer/hooks/useNotifications.ts', () => { raiseSoundNotificationSpy.mockClear(); raiseNativeNotificationSpy.mockClear(); getAllNotificationsMock.mockReset(); + clearServerPollIntervals(); useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); @@ -235,6 +240,47 @@ describe('renderer/hooks/useNotifications.ts', () => { vi.useRealTimers(); } }); + + it('stretches the polling interval to the server-recommended minimum', async () => { + vi.useFakeTimers(); + try { + // The forge client reports the `X-Poll-Interval` header while the + // fetch runs, so each poll refreshes the recommendation. + getAllNotificationsMock.mockImplementation(async () => { + reportServerPollInterval(mockGitHubCloudAccount, 5); + return mockSingleAccountNotifications; + }); + + useSettingsStore.setState({ + fetchInterval: 1000, + }); + + renderHook(() => useNotifications({ withSideEffects: true }), { + wrapper: createWrapper(), + }); + + // Initial mount fetch reports a 5s server minimum. + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(getAllNotificationsMock).toHaveBeenCalledTimes(1); + + // The 1s user interval would fire ~3 polls here; the server minimum + // suppresses them all. + await act(async () => { + await vi.advanceTimersByTimeAsync(3_500); + }); + expect(getAllNotificationsMock).toHaveBeenCalledTimes(1); + + // Just past the 5s server minimum, exactly one poll fires. + await act(async () => { + await vi.advanceTimersByTimeAsync(2_000); + }); + expect(getAllNotificationsMock).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); }); describe('sound and native notifications', () => { diff --git a/src/renderer/hooks/useNotifications.ts b/src/renderer/hooks/useNotifications.ts index 72ec490f4..3da487ed3 100644 --- a/src/renderer/hooks/useNotifications.ts +++ b/src/renderer/hooks/useNotifications.ts @@ -27,6 +27,7 @@ import { getNotificationCount, getUnreadNotificationCount, } from '../utils/notifications/notifications'; +import { computeRefetchIntervalMs } from '../utils/notifications/pollInterval'; import { removeNotificationsForAccount } from '../utils/notifications/remove'; import { getNewNotifications } from '../utils/notifications/utils'; import { raiseSoundNotification } from '../utils/system/audio'; @@ -159,7 +160,11 @@ export const useNotifications = ({ // Only the singleton side-effects host polls. Other consumers share the // cached data and would otherwise each schedule their own refetch timer. - refetchInterval: withSideEffects ? fetchIntervalMs : false, + // The interval is re-evaluated after each fetch so it stretches to the + // latest server-recommended minimum (`X-Poll-Interval`) when required. + refetchInterval: withSideEffects + ? () => computeRefetchIntervalMs(fetchIntervalMs, accounts) + : false, refetchOnMount: withSideEffects, refetchOnReconnect: withSideEffects, refetchOnWindowFocus: withSideEffects, diff --git a/src/renderer/utils/forges/github/client.test.ts b/src/renderer/utils/forges/github/client.test.ts index 0c791968a..4acdfe814 100644 --- a/src/renderer/utils/forges/github/client.test.ts +++ b/src/renderer/utils/forges/github/client.test.ts @@ -12,6 +12,10 @@ import { useSettingsStore } from '../../../stores'; import type { Link } from '../../../types'; +import { + clearServerPollIntervals, + computeRefetchIntervalMs, +} from '../../notifications/pollInterval'; import { fetchAuthenticatedUserDetails, fetchDiscussionByNumber, @@ -70,6 +74,8 @@ describe('renderer/utils/forges/github/client.ts', () => { const createOctokitClientUncachedSpy = vi.spyOn(octokitModule, 'createOctokitClientUncached'); beforeEach(() => { + clearServerPollIntervals(); + vi.mocked(apiRequests.performGraphQLRequest).mockReset(); vi.mocked(apiRequests.performGraphQLRequestString).mockReset(); @@ -206,7 +212,42 @@ describe('renderer/utils/forges/github/client.ts', () => { 'Cache-Control': 'no-cache', }, }, + expect.any(Function), + ); + }); + + it('should capture the X-Poll-Interval header for the account', async () => { + useSettingsStore.setState({ + participating: false, + fetchReadNotifications: false, + fetchAllNotifications: false, + }); + + mockOctokit.rest.activity.listNotificationsForAuthenticatedUser.mockResolvedValue({ + data: [], + status: 200, + headers: { 'x-poll-interval': '75' }, + }); + + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + + expect(computeRefetchIntervalMs(0, [mockGitHubCloudAccount])).toBe(75000); + }); + + it('should capture the X-Poll-Interval header when paginating', async () => { + useSettingsStore.setState({ + participating: false, + fetchReadNotifications: false, + fetchAllNotifications: true, + }); + + mockOctokit.paginate.mockImplementation((_endpoint, _parameters, mapFn) => + Promise.resolve(mapFn({ data: [], headers: { 'x-poll-interval': '120' } })), ); + + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + + expect(computeRefetchIntervalMs(0, [mockGitHubCloudAccount])).toBe(120000); }); }); diff --git a/src/renderer/utils/forges/github/client.ts b/src/renderer/utils/forges/github/client.ts index 9eb21c52c..7bcdd86a9 100644 --- a/src/renderer/utils/forges/github/client.ts +++ b/src/renderer/utils/forges/github/client.ts @@ -13,6 +13,7 @@ import type { MarkNotificationThreadAsReadResponse, } from './types'; +import { reportServerPollInterval } from '../../notifications/pollInterval'; import { supportsAnsweredDiscussion } from './capabilities'; import { FetchDiscussionByNumberDocument, @@ -47,6 +48,10 @@ export async function fetchAuthenticatedUserDetails(account: Account) { /** * List all notifications for the current user, sorted by most recently updated. * + * The server-recommended minimum poll interval (`X-Poll-Interval` header) is + * reported to the poll interval registry so the notifications query can slow + * down when GitHub asks clients to back off. + * * Endpoint documentation: https://docs.github.com/en/rest/activity/notifications#list-notifications-for-the-authenticated-user */ export async function listNotificationsForAuthenticatedUser( @@ -57,14 +62,22 @@ export async function listNotificationsForAuthenticatedUser( if (settings.fetchAllNotifications) { // Fetch all pages using Octokit's pagination - return await octokit.paginate(octokit.rest.activity.listNotificationsForAuthenticatedUser, { - participating: settings.participating, - all: settings.fetchReadNotifications, - per_page: 100, - headers: { - 'Cache-Control': 'no-cache', // Prevent caching + return await octokit.paginate( + octokit.rest.activity.listNotificationsForAuthenticatedUser, + { + participating: settings.participating, + all: settings.fetchReadNotifications, + per_page: 100, + headers: { + 'Cache-Control': 'no-cache', // Prevent caching + }, }, - }); + (response) => { + reportServerPollInterval(account, Number(response.headers['x-poll-interval'])); + + return response.data; + }, + ); } // Single page request @@ -77,6 +90,8 @@ export async function listNotificationsForAuthenticatedUser( }, }); + reportServerPollInterval(account, Number(response.headers['x-poll-interval'])); + return response.data; } diff --git a/src/renderer/utils/notifications/pollInterval.test.ts b/src/renderer/utils/notifications/pollInterval.test.ts new file mode 100644 index 000000000..724e8f93b --- /dev/null +++ b/src/renderer/utils/notifications/pollInterval.test.ts @@ -0,0 +1,62 @@ +import { + mockGitHubCloudAccount, + mockGitHubEnterpriseServerAccount, +} from '../../__mocks__/account-mocks'; + +import { + clearServerPollIntervals, + computeRefetchIntervalMs, + reportServerPollInterval, +} from './pollInterval'; + +describe('renderer/utils/notifications/pollInterval.ts', () => { + beforeEach(() => { + clearServerPollIntervals(); + }); + + it('returns the user interval when no server interval has been reported', () => { + expect(computeRefetchIntervalMs(60000, [mockGitHubCloudAccount])).toBe(60000); + }); + + it('stretches the interval to the slowest server recommendation', () => { + reportServerPollInterval(mockGitHubCloudAccount, 60); + reportServerPollInterval(mockGitHubEnterpriseServerAccount, 300); + + expect( + computeRefetchIntervalMs(60000, [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount]), + ).toBe(300000); + }); + + it('never polls faster than the user interval', () => { + reportServerPollInterval(mockGitHubCloudAccount, 60); + + expect(computeRefetchIntervalMs(120000, [mockGitHubCloudAccount])).toBe(120000); + }); + + it('ignores intervals reported for accounts no longer polled', () => { + reportServerPollInterval(mockGitHubEnterpriseServerAccount, 300); + + expect(computeRefetchIntervalMs(60000, [mockGitHubCloudAccount])).toBe(60000); + }); + + it('ignores invalid reported values', () => { + reportServerPollInterval(mockGitHubCloudAccount, Number.NaN); + reportServerPollInterval(mockGitHubCloudAccount, 0); + reportServerPollInterval(mockGitHubCloudAccount, -5); + + expect(computeRefetchIntervalMs(60000, [mockGitHubCloudAccount])).toBe(60000); + }); + + it('uses the latest reported value for an account', () => { + reportServerPollInterval(mockGitHubCloudAccount, 300); + reportServerPollInterval(mockGitHubCloudAccount, 90); + + expect(computeRefetchIntervalMs(60000, [mockGitHubCloudAccount])).toBe(90000); + }); + + it('returns the user interval when no accounts are polled', () => { + reportServerPollInterval(mockGitHubCloudAccount, 300); + + expect(computeRefetchIntervalMs(60000, [])).toBe(60000); + }); +}); diff --git a/src/renderer/utils/notifications/pollInterval.ts b/src/renderer/utils/notifications/pollInterval.ts new file mode 100644 index 000000000..30ae47fef --- /dev/null +++ b/src/renderer/utils/notifications/pollInterval.ts @@ -0,0 +1,61 @@ +import type { Account } from '../../types'; + +/** + * Last server-recommended minimum poll interval in seconds, per account. + * + * GitHub returns an `X-Poll-Interval` header on the notifications list + * endpoint indicating how frequently clients may poll it (usually 60 seconds, + * raised when the server wants clients to back off). Forge clients report the + * header value here after each list fetch and the notifications query reads it + * back to stretch its polling interval accordingly. + * + * @see https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api + */ +const serverPollIntervalsSeconds = new Map(); + +/** + * Stable per-account key, derived from the same identity fields as + * `getAccountUUID`. Kept local so this leaf module does not import the + * auth/forge module graph (which would create an import cycle back through + * the forge clients); the key only needs to be consistent within this + * registry. + */ +function accountKey(account: Account): string { + return `${account.hostname}-${account.user?.id}-${account.method}`; +} + +/** + * Record the server-recommended minimum poll interval for an account. + * Non-finite or non-positive values are ignored, so forges without the header + * simply never report. + */ +export function reportServerPollInterval(account: Account, seconds: number): void { + if (Number.isFinite(seconds) && seconds > 0) { + serverPollIntervalsSeconds.set(accountKey(account), seconds); + } +} + +/** + * Compute the effective notifications polling interval: the user-configured + * interval, stretched to the slowest server-recommended interval across the + * given accounts so polling never runs faster than a forge allows. + * + * @param userIntervalMs - The user-configured fetch interval in milliseconds. + * @param accounts - The accounts currently being polled. + * @returns The polling interval to use, in milliseconds. + */ +export function computeRefetchIntervalMs(userIntervalMs: number, accounts: Account[]): number { + const serverIntervalsMs = accounts.map( + (account) => (serverPollIntervalsSeconds.get(accountKey(account)) ?? 0) * 1000, + ); + + return Math.max(userIntervalMs, ...serverIntervalsMs); +} + +/** + * Clear all reported poll intervals. Intended for use in tests to keep the + * module-level state from leaking across cases. + */ +export function clearServerPollIntervals(): void { + serverPollIntervalsSeconds.clear(); +}