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; + } } /**