From ebd02d7dde25b37ea7323c6c1974c939b403198c Mon Sep 17 00:00:00 2001 From: Josh Free Date: Tue, 28 Jul 2026 17:36:39 -0700 Subject: [PATCH] perf(notifications): use conditional requests for the notifications list Send If-None-Match / If-Modified-Since validators on the notifications list request and cache the previous response per account. When GitHub responds 304 Not Modified (nothing changed since the last poll) the cached list is returned without transferring or re-parsing the full body; a 304 also does not count against the primary rate limit. Refs gitify-app/gitify#3101 --- .../utils/forges/github/client.test.ts | 82 +++++++++++ src/renderer/utils/forges/github/client.ts | 131 +++++++++++++++--- 2 files changed, 196 insertions(+), 17 deletions(-) diff --git a/src/renderer/utils/forges/github/client.test.ts b/src/renderer/utils/forges/github/client.test.ts index 0c791968a..105c84cd9 100644 --- a/src/renderer/utils/forges/github/client.test.ts +++ b/src/renderer/utils/forges/github/client.test.ts @@ -13,6 +13,7 @@ import { useSettingsStore } from '../../../stores'; import type { Link } from '../../../types'; import { + clearNotificationsListCache, fetchAuthenticatedUserDetails, fetchDiscussionByNumber, fetchIssueByNumber, @@ -70,6 +71,8 @@ describe('renderer/utils/forges/github/client.ts', () => { const createOctokitClientUncachedSpy = vi.spyOn(octokitModule, 'createOctokitClientUncached'); beforeEach(() => { + clearNotificationsListCache(); + vi.mocked(apiRequests.performGraphQLRequest).mockReset(); vi.mocked(apiRequests.performGraphQLRequestString).mockReset(); @@ -206,8 +209,87 @@ describe('renderer/utils/forges/github/client.ts', () => { 'Cache-Control': 'no-cache', }, }, + expect.any(Function), ); }); + + it('sends a conditional request using the ETag from the previous response', async () => { + useSettingsStore.setState({ + participating: false, + fetchReadNotifications: false, + fetchAllNotifications: false, + }); + + mockOctokit.rest.activity.listNotificationsForAuthenticatedUser.mockResolvedValueOnce({ + data: [], + status: 200, + headers: { etag: 'W/"abc"' }, + }); + + // First poll primes the cache with the returned ETag. + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + // Second poll should replay the ETag as a conditional request. + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + + expect( + mockOctokit.rest.activity.listNotificationsForAuthenticatedUser, + ).toHaveBeenLastCalledWith({ + participating: false, + all: false, + per_page: 100, + headers: { + 'Cache-Control': 'no-cache', + 'If-None-Match': 'W/"abc"', + }, + }); + }); + + it('returns the cached notifications when GitHub responds 304 Not Modified', async () => { + useSettingsStore.setState({ + participating: false, + fetchReadNotifications: false, + fetchAllNotifications: false, + }); + + const cachedNotifications = [{ id: '123' }] as unknown as Awaited< + ReturnType + >; + + mockOctokit.rest.activity.listNotificationsForAuthenticatedUser.mockResolvedValueOnce({ + data: cachedNotifications, + status: 200, + headers: { etag: 'W/"abc"' }, + }); + // Octokit throws a RequestError with status 304 for unchanged conditional requests. + mockOctokit.rest.activity.listNotificationsForAuthenticatedUser.mockRejectedValueOnce({ + status: 304, + }); + + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + const result = await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + + expect(result).toEqual(cachedNotifications); + }); + + it('returns the cached notifications when a paginated poll responds 304 Not Modified', async () => { + useSettingsStore.setState({ + participating: false, + fetchReadNotifications: false, + fetchAllNotifications: true, + }); + + const cachedNotifications = [{ id: '123' }] as unknown as Awaited< + ReturnType + >; + + mockOctokit.paginate.mockResolvedValueOnce(cachedNotifications); + mockOctokit.paginate.mockRejectedValueOnce({ status: 304 }); + + await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + const result = await listNotificationsForAuthenticatedUser(mockGitHubCloudAccount); + + expect(result).toEqual(cachedNotifications); + }); }); it('markNotificationThreadAsRead - should mark notification thread as read', async () => { diff --git a/src/renderer/utils/forges/github/client.ts b/src/renderer/utils/forges/github/client.ts index 9eb21c52c..d9f6a46f2 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 { getAccountUUID } from '../../auth/utils'; import { supportsAnsweredDiscussion } from './capabilities'; import { FetchDiscussionByNumberDocument, @@ -44,9 +45,70 @@ export async function fetchAuthenticatedUserDetails(account: Account) { }); } +/** + * In-memory store of the last notifications-list response per account, used to + * make conditional requests. When GitHub responds `304 Not Modified` (because + * nothing has changed since the previous poll) the cached list is returned + * without transferring or re-parsing the full body. A `304` response also does + * not count against the primary rate limit. + */ +type NotificationsListCacheEntry = { + etag?: string; + lastModified?: string; + data: ListNotificationsForAuthenticatedUserResponse; +}; + +const notificationsListCache = new Map(); + +/** + * Clear the notifications-list conditional-request cache. + * Exposed primarily so tests can start from a clean state. + */ +export function clearNotificationsListCache(): void { + notificationsListCache.clear(); +} + +/** + * Build the request headers for the notifications list, adding conditional + * request validators (`If-None-Match` / `If-Modified-Since`) when a previous + * response has been cached for the account. + */ +function buildNotificationsListHeaders( + cached: NotificationsListCacheEntry | undefined, +): Record { + const headers: Record = { + 'Cache-Control': 'no-cache', // Force revalidation rather than using a stale cached response + }; + + if (cached?.etag) { + headers['If-None-Match'] = cached.etag; + } else if (cached?.lastModified) { + headers['If-Modified-Since'] = cached.lastModified; + } + + return headers; +} + +/** + * Determine whether an error thrown by Octokit represents a `304 Not Modified` + * response. Octokit throws a `RequestError` with `status === 304` for + * conditional requests that hit an unchanged resource. + */ +function isNotModifiedError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + 'status' in error && + (error as { status?: number }).status === 304 + ); +} + /** * List all notifications for the current user, sorted by most recently updated. * + * Uses conditional requests so that an unchanged notifications list is returned + * from cache on a `304 Not Modified` response instead of being re-downloaded. + * * Endpoint documentation: https://docs.github.com/en/rest/activity/notifications#list-notifications-for-the-authenticated-user */ export async function listNotificationsForAuthenticatedUser( @@ -55,29 +117,64 @@ export async function listNotificationsForAuthenticatedUser( const settings = useSettingsStore.getState(); const octokit = await createOctokitClient(account, 'rest'); - if (settings.fetchAllNotifications) { - // Fetch all pages using Octokit's pagination - return await octokit.paginate(octokit.rest.activity.listNotificationsForAuthenticatedUser, { + const cacheKey = getAccountUUID(account); + const cached = notificationsListCache.get(cacheKey); + const headers = buildNotificationsListHeaders(cached); + + try { + if (settings.fetchAllNotifications) { + // Fetch all pages using Octokit's pagination, capturing the validators + // from the first page so the next poll can be made conditional. + let captured = false; + let etag: string | undefined; + let lastModified: string | undefined; + + const data = await octokit.paginate( + octokit.rest.activity.listNotificationsForAuthenticatedUser, + { + participating: settings.participating, + all: settings.fetchReadNotifications, + per_page: 100, + headers, + }, + (response) => { + if (!captured) { + captured = true; + etag = response.headers.etag; + lastModified = response.headers['last-modified']; + } + + return response.data; + }, + ); + + notificationsListCache.set(cacheKey, { etag, lastModified, data }); + + return data; + } + + // Single page request + const response = await octokit.rest.activity.listNotificationsForAuthenticatedUser({ participating: settings.participating, all: settings.fetchReadNotifications, per_page: 100, - headers: { - 'Cache-Control': 'no-cache', // Prevent caching - }, + headers, }); - } - // Single page request - const response = await octokit.rest.activity.listNotificationsForAuthenticatedUser({ - participating: settings.participating, - all: settings.fetchReadNotifications, - per_page: 100, - headers: { - 'Cache-Control': 'no-cache', // Prevent caching - }, - }); + notificationsListCache.set(cacheKey, { + etag: response.headers.etag, + lastModified: response.headers['last-modified'], + data: response.data, + }); - return response.data; + return response.data; + } catch (error) { + if (isNotModifiedError(error) && cached) { + return cached.data; + } + + throw error; + } } /**