diff --git a/src/renderer/utils/forges/github/enrich.test.ts b/src/renderer/utils/forges/github/enrich.test.ts index 8192cd19b..f99475601 100644 --- a/src/renderer/utils/forges/github/enrich.test.ts +++ b/src/renderer/utils/forges/github/enrich.test.ts @@ -58,4 +58,36 @@ describe('renderer/utils/forges/github/enrich.ts', () => { 'Continuing with base notification details', ); }); + + it('preserves the original subject reference only when enrichment fails', async () => { + vi.spyOn(logger, 'rendererLogError').mockImplementation(vi.fn()); + vi.spyOn(logger, 'rendererLogWarn').mockImplementation(vi.fn()); + + vi.mocked(client.fetchNotificationDetailsForList).mockResolvedValue(new Map()); + + const failingNotification = mockPartialGitifyNotification({ + title: 'This issue will fail to enrich', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, + }); + const succeedingNotification = mockPartialGitifyNotification({ + title: 'This issue will enrich', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/2' as Link, + }); + + vi.mocked(client.fetchIssueByNumber) + .mockRejectedValueOnce(new Error('Test error')) + .mockResolvedValueOnce({ repository: {} } as never); + + const [failed, succeeded] = await enrichGitHubNotifications([ + failingNotification, + succeedingNotification, + ]); + + // Adapter contract: a failed enrichment keeps the original `subject` + // reference so callers can detect it; a completed one returns a new one. + expect(failed.subject).toBe(failingNotification.subject); + expect(succeeded.subject).not.toBe(succeedingNotification.subject); + }); }); diff --git a/src/renderer/utils/forges/github/enrich.ts b/src/renderer/utils/forges/github/enrich.ts index 24c96d785..de4979bc9 100644 --- a/src/renderer/utils/forges/github/enrich.ts +++ b/src/renderer/utils/forges/github/enrich.ts @@ -18,6 +18,10 @@ export const GITHUB_API_MERGE_BATCH_SIZE = 100; * comment count, labels, etc.) by issuing a single batched GraphQL query * and dispatching to per-subject-type handlers. * + * Notifications whose detail fetch fails are returned unchanged, preserving + * their original `subject` reference so callers can detect the failure (see + * the `enrichNotifications` adapter contract). + * * Exposed via `githubAdapter.enrichNotifications` so the shared notification * orchestrator stays adapter-agnostic. */ @@ -80,6 +84,10 @@ async function enrichSingle( ); rendererLogWarn('enrichGitHubNotifications', 'Continuing with base notification details'); + + // Keep the original subject reference so callers can tell this + // notification was not enriched and retry it on a later poll. + return notification; } return { diff --git a/src/renderer/utils/forges/types.ts b/src/renderer/utils/forges/types.ts index 752c0a746..2a341667b 100644 --- a/src/renderer/utils/forges/types.ts +++ b/src/renderer/utils/forges/types.ts @@ -136,6 +136,11 @@ export interface ForgeAdapter { * enrichment (e.g. Gitea) omit this and the orchestrator returns the input * unchanged. * + * Implementations must return notifications in input order, and must return + * a notification whose detail fetch failed with its original `subject` + * reference unchanged, so callers can detect the failure (e.g. to avoid + * caching base details as if they were enriched). + * * @see ../notifications/notifications.ts `enrichNotifications` — orchestrator * that delegates here when the user has detailed notifications enabled. */ diff --git a/src/renderer/utils/notifications/notifications.test.ts b/src/renderer/utils/notifications/notifications.test.ts index cf8ff7dad..5cab69243 100644 --- a/src/renderer/utils/notifications/notifications.test.ts +++ b/src/renderer/utils/notifications/notifications.test.ts @@ -20,6 +20,7 @@ import { import * as apiClient from '../forges/github/client'; import { GITHUB_API_MERGE_BATCH_SIZE } from '../forges/github/enrich'; import { + clearEnrichmentCache, enrichNotifications, getNotificationCount, getUnreadNotificationCount, @@ -41,6 +42,7 @@ describe('renderer/utils/notifications/notifications.ts', () => { vi.mocked(apiClient.fetchNotificationDetailsForList).mockReset(); vi.mocked(apiClient.fetchNotificationDetailsForList).mockResolvedValue(new Map()); vi.mocked(apiClient.fetchIssueByNumber).mockReset(); + clearEnrichmentCache(); }); it('getNotificationCount', () => { @@ -194,5 +196,121 @@ describe('renderer/utils/notifications/notifications.ts', () => { expect(fetchNotificationDetailsForListSpy).toHaveBeenCalledTimes(1); expect(fetchNotificationDetailsForListSpy.mock.calls[0][0]).toHaveLength(50); }); + + it('should skip re-fetch when a notification updatedAt is unchanged', async () => { + const fetchNotificationDetailsForListSpy = vi.mocked( + apiClient.fetchNotificationDetailsForList, + ); + vi.mocked(apiClient.fetchIssueByNumber).mockResolvedValue({ repository: {} } as never); + fetchNotificationDetailsForListSpy.mockResolvedValue(new Map()); + + useSettingsStore.setState({ detailedNotifications: true }); + + const notification = { + ...(mockPartialGitifyNotification({ + title: 'Issue #1', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, + }) as GitifyNotification), + id: '1', + updatedAt: '2026-01-01T00:00:00Z', + }; + + // First poll fetches details; second poll with an unchanged + // `updatedAt` must reuse the cached subject and not re-fetch. + await enrichNotifications([notification]); + await enrichNotifications([notification]); + + expect(fetchNotificationDetailsForListSpy).toHaveBeenCalledTimes(1); + }); + + it('should re-fetch when a notification updatedAt changes', async () => { + const fetchNotificationDetailsForListSpy = vi.mocked( + apiClient.fetchNotificationDetailsForList, + ); + vi.mocked(apiClient.fetchIssueByNumber).mockResolvedValue({ repository: {} } as never); + fetchNotificationDetailsForListSpy.mockResolvedValue(new Map()); + + useSettingsStore.setState({ detailedNotifications: true }); + + const base = mockPartialGitifyNotification({ + title: 'Issue #1', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, + }) as GitifyNotification; + + // A changed `updatedAt` invalidates the cache and forces a re-fetch. + await enrichNotifications([{ ...base, id: '1', updatedAt: '2026-01-01T00:00:00Z' }]); + await enrichNotifications([{ ...base, id: '1', updatedAt: '2026-01-02T00:00:00Z' }]); + + expect(fetchNotificationDetailsForListSpy).toHaveBeenCalledTimes(2); + }); + + it('should keep cache entries for other accounts when pruning', async () => { + const fetchNotificationDetailsForListSpy = vi.mocked( + apiClient.fetchNotificationDetailsForList, + ); + vi.mocked(apiClient.fetchIssueByNumber).mockResolvedValue({ repository: {} } as never); + fetchNotificationDetailsForListSpy.mockResolvedValue(new Map()); + + useSettingsStore.setState({ detailedNotifications: true }); + + const base = mockPartialGitifyNotification({ + title: 'Issue #1', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, + }) as GitifyNotification; + + const cloudNotification = { + ...base, + id: '1', + updatedAt: '2026-01-01T00:00:00Z', + account: mockGitHubCloudAccount, + }; + const enterpriseNotification = { + ...base, + id: '2', + updatedAt: '2026-01-01T00:00:00Z', + account: mockGitHubEnterpriseServerAccount, + }; + + // `getAllNotifications` enriches each account in a separate call, so + // pruning one account's stale entries must not evict the other's. + await enrichNotifications([cloudNotification]); + await enrichNotifications([enterpriseNotification]); + await enrichNotifications([cloudNotification]); + await enrichNotifications([enterpriseNotification]); + + expect(fetchNotificationDetailsForListSpy).toHaveBeenCalledTimes(2); + }); + + it('should re-fetch on the next poll when enrichment failed', async () => { + const fetchNotificationDetailsForListSpy = vi.mocked( + apiClient.fetchNotificationDetailsForList, + ); + fetchNotificationDetailsForListSpy.mockResolvedValue(new Map()); + // First poll: the per-notification fallback fetch fails, so the + // notification is returned with base details only. + vi.mocked(apiClient.fetchIssueByNumber).mockRejectedValueOnce(new Error('transient outage')); + vi.mocked(apiClient.fetchIssueByNumber).mockResolvedValue({ repository: {} } as never); + + useSettingsStore.setState({ detailedNotifications: true }); + + const notification = { + ...(mockPartialGitifyNotification({ + title: 'Issue #1', + type: 'Issue', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, + }) as GitifyNotification), + id: '1', + updatedAt: '2026-01-01T00:00:00Z', + }; + + // The failed enrichment must not be cached, so the second poll retries. + await enrichNotifications([notification]); + await enrichNotifications([notification]); + + expect(fetchNotificationDetailsForListSpy).toHaveBeenCalledTimes(2); + }); }); }); diff --git a/src/renderer/utils/notifications/notifications.ts b/src/renderer/utils/notifications/notifications.ts index ce1cafefd..e7d53a89d 100644 --- a/src/renderer/utils/notifications/notifications.ts +++ b/src/renderer/utils/notifications/notifications.ts @@ -1,8 +1,14 @@ import { useAccountsStore, useSettingsStore } from '../../stores'; -import type { Account, AccountNotifications, RawGitifyNotification } from '../../types'; +import type { + Account, + AccountNotifications, + GitifySubject, + RawGitifyNotification, +} from '../../types'; import { determineFailureType } from '../api/errors'; +import { getAccountUUID } from '../auth/utils'; import { rendererLogError, toError } from '../core/logger'; import { getAdapter } from '../forges/registry'; import { filterBaseNotifications } from './filters/filter'; @@ -114,6 +120,14 @@ export async function getAllNotifications(): Promise { * original list unchanged otherwise. Details are fetched in batches via * GraphQL to avoid overwhelming the API. * + * To avoid re-fetching details for notifications that have not changed, + * previously-enriched subjects are cached keyed by account, notification id, + * and `updatedAt` (see {@link enrichmentCacheKey}). GitHub bumps a + * notification's `updatedAt` whenever its subject changes in a way worth + * surfacing, so an unchanged `updatedAt` means the cached subject is still + * valid and the detail fetch can be skipped entirely. This sharply reduces + * per-poll API request volume for users with many long-lived notifications. + * * @param notifications - The notifications to enrich. * @returns The same notifications with subject fields populated from the API. */ @@ -130,7 +144,84 @@ export async function enrichNotifications( if (!enrich) { return notifications; } - return enrich(notifications); + + // Reuse cached subjects for notifications whose `updatedAt` is unchanged and + // only fetch details for cache misses, so unchanged notifications are not + // re-fetched on every poll cycle. The cached subject is applied onto the + // freshly-fetched notification so read-state and ordering stay current. + const liveKeys = new Set(); + const result: RawGitifyNotification[] = []; + const misses: RawGitifyNotification[] = []; + const missIndexes: number[] = []; + + notifications.forEach((notification, index) => { + const key = enrichmentCacheKey(notification); + liveKeys.add(key); + + const cachedSubject = enrichmentCache.get(key); + if (cachedSubject) { + result[index] = { ...notification, subject: cachedSubject }; + } else { + misses.push(notification); + missIndexes.push(index); + } + }); + + if (misses.length) { + // Adapters return enriched notifications in the same order as their input, + // so map results back positionally rather than by id. + const enriched = await enrich(misses); + enriched.forEach((enrichedNotification, i) => { + const index = missIndexes[i]; + result[index] = enrichedNotification; + + // Adapters signal a failed enrichment by returning the notification + // with its original `subject` reference unchanged. Skip caching those + // so they are retried on the next poll instead of serving base details + // until `updatedAt` next changes. + if (enrichedNotification.subject !== misses[i].subject) { + enrichmentCache.set(enrichmentCacheKey(enrichedNotification), enrichedNotification.subject); + } + }); + } + + // Bound cache growth: drop this account's entries for notifications no + // longer present. Enrichment runs once per account (and concurrently across + // accounts), so the prune must not touch other accounts' entries, which are + // never in `liveKeys`. + const accountKeyPrefix = `${getAccountUUID(notifications[0].account)}:`; + for (const key of enrichmentCache.keys()) { + if (key.startsWith(accountKeyPrefix) && !liveKeys.has(key)) { + enrichmentCache.delete(key); + } + } + + return result; +} + +/** + * In-memory cache of enriched subjects keyed by {@link enrichmentCacheKey}. + * Persists across poll cycles so unchanged notifications can skip enrichment, + * and is pruned per account each cycle to that account's currently-present + * notifications so it stays bounded to the live notification working set. + */ +const enrichmentCache = new Map(); + +/** + * Build the enrichment cache key for a notification from its account, id, and + * last-updated timestamp. A change to any of these invalidates the cached + * subject and forces a re-fetch. + */ +function enrichmentCacheKey(notification: RawGitifyNotification): string { + return `${getAccountUUID(notification.account)}:${notification.id}:${notification.updatedAt}`; +} + +/** + * Clear the enrichment cache. Intended for use in tests to keep the + * module-level cache from leaking state across cases. + */ +export function clearEnrichmentCache(): void { + enrichmentCache.clear(); } /**