Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/renderer/utils/forges/github/enrich.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
8 changes: 8 additions & 0 deletions src/renderer/utils/forges/github/enrich.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/utils/forges/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
118 changes: 118 additions & 0 deletions src/renderer/utils/notifications/notifications.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
});
95 changes: 93 additions & 2 deletions src/renderer/utils/notifications/notifications.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -114,6 +120,14 @@ export async function getAllNotifications(): Promise<AccountNotifications[]> {
* 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.
*/
Expand All @@ -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<string>();
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<string, GitifySubject>();

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

/**
Expand Down
Loading