镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
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
14 changes: 14 additions & 0 deletions src/renderer/components/metrics/LabelsPill.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,20 @@ describe('renderer/components/metrics/LabelsPill.tsx', () => {
expect(tree.container).toMatchSnapshot();
});

it('reports the total label count when the fetched list is capped', () => {
const props: LabelsPillProps = {
labels: Array.from({ length: 25 }, (_, index) => ({
name: `label-${index}`,
color: 'a2eeef',
})),
labelCount: 30,
};

const tree = renderWithProviders(<LabelsPill {...props} />);

expect(tree.getByRole('button', { name: /30 labels/ })).toBeInTheDocument();
});

it('renders field tokens when there are no labels', () => {
const props: LabelsPillProps = {
labels: [],
Expand Down
9 changes: 6 additions & 3 deletions src/renderer/components/metrics/LabelsPill.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ import { MetricPill } from './MetricPill';
export interface LabelsPillProps {
labels: GitifyLabels[];
issueFields?: GitifyIssueField[];
labelCount?: number;
}

export function LabelsPill({ labels, issueFields }: Readonly<LabelsPillProps>) {
export function LabelsPill({ labels, issueFields, labelCount }: Readonly<LabelsPillProps>) {
const fieldTokens = (issueFields ?? []).map((field) => ({
text: `${field.name}: ${field.value}`,
fillColor: field.fillColor,
}));

const totalLabelCount = labelCount ?? labels?.length ?? 0;

const labelsContent =
labels?.length || fieldTokens.length ? (
<LabelGroup>
Expand Down Expand Up @@ -52,11 +55,11 @@ export function LabelsPill({ labels, issueFields }: Readonly<LabelsPillProps>) {

return (
<MetricPill
ariaLabel={`${formatMetricDescription(labelNames.length, 'label')}: ${labelNames.join(', ')}`}
ariaLabel={`${formatMetricDescription(fieldTokens.length + totalLabelCount, 'label')}: ${labelNames.join(', ')}`}
color={IconColor.GRAY}
contents={labelsContent}
icon={TagIcon}
metric={fieldTokens.length + (labels?.length ?? 0)}
metric={fieldTokens.length + totalLabelCount}
/>
);
}
1 change: 1 addition & 0 deletions src/renderer/components/metrics/MetricGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export function MetricGroup({ notification }: Readonly<MetricGroupProps>) {
<LabelsPill
labels={notification.subject.labels ?? []}
issueFields={notification.subject.issueFields ?? []}
labelCount={notification.subject.labelCount}
/>
</div>
);
Expand Down
14 changes: 7 additions & 7 deletions src/renderer/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,14 @@ export const Constants = {

// GraphQL Argument Defaults
GRAPHQL_ARGS: {
FIRST_LABELS: 100,
FIRST_CLOSING_ISSUES: 100,
FIRST_ISSUE_FIELD_VALUES: 100,
FIRST_LABELS: 25,
FIRST_CLOSING_ISSUES: 10,
FIRST_ISSUE_FIELD_VALUES: 25,
LAST_COMMENTS: 1,
LAST_THREADED_COMMENTS: 10,
LAST_REPLIES: 10,
LAST_REVIEWS: 100,
FIRST_REVIEW_THREADS: 100,
LAST_THREADED_COMMENTS: 5,
LAST_REPLIES: 5,
LAST_REVIEWS: 25,
FIRST_REVIEW_THREADS: 25,
},

// GitHub Docs
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,8 @@ export interface GitifySubject {
commentCount?: number;
/** Labels names and colors */
labels?: GitifyLabels[];
/** Total label count, including labels beyond the fetched list */
labelCount?: number;
/** Whether the PR is part of a GitHub native stacked PR series */
isStacked?: boolean;
/** This PR's 1-indexed position in the stack, when part of a stacked PR series */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export function mockIssueResponseNode(mocks: {
stateReason: mocks.stateReason ?? null,
url: 'https://github.com/gitify-app/notifications-test/issues/123' as Link,
author: mockAuthor,
labels: { nodes: [] },
labels: { totalCount: 0, nodes: [] },
comments: { totalCount: 0, nodes: [] },
milestone: null,
issueType: null,
Expand Down Expand Up @@ -136,7 +136,7 @@ export function mockPullRequestResponseNode(mocks: {
reviewDecision: null,
url: 'https://github.com/gitify-app/notifications-test/pulls/123' as Link,
author: mockAuthor,
labels: { nodes: [] },
labels: { totalCount: 0, nodes: [] },
comments: {
totalCount: 0,
nodes: [],
Expand Down
38 changes: 18 additions & 20 deletions src/renderer/utils/forges/github/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import {
mockPartialGitifyNotification,
} from '../../../__mocks__/notifications-mocks';

import { Constants } from '../../../constants';

import { useSettingsStore } from '../../../stores';

import type { Link } from '../../../types';
Expand Down Expand Up @@ -336,9 +334,9 @@ describe('renderer/utils/forges/github/client.ts', () => {
owner: mockNotification.repository.owner.login,
name: mockNotification.repository.name,
number: 123,
firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS,
lastThreadedComments: Constants.GRAPHQL_ARGS.LAST_THREADED_COMMENTS,
lastReplies: Constants.GRAPHQL_ARGS.LAST_REPLIES,
firstLabels: 25,
lastThreadedComments: 5,
lastReplies: 5,
});
});

Expand Down Expand Up @@ -386,9 +384,9 @@ describe('renderer/utils/forges/github/client.ts', () => {
owner: mockNotification.repository.owner.login,
name: mockNotification.repository.name,
number: 123,
firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS,
firstIssueFieldValues: Constants.GRAPHQL_ARGS.FIRST_ISSUE_FIELD_VALUES,
lastComments: Constants.GRAPHQL_ARGS.LAST_COMMENTS,
firstLabels: 25,
firstIssueFieldValues: 25,
lastComments: 1,
},
);

Expand Down Expand Up @@ -452,11 +450,11 @@ describe('renderer/utils/forges/github/client.ts', () => {
owner: mockNotification.repository.owner.login,
name: mockNotification.repository.name,
number: 123,
firstClosingIssues: Constants.GRAPHQL_ARGS.FIRST_CLOSING_ISSUES,
firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS,
lastComments: Constants.GRAPHQL_ARGS.LAST_COMMENTS,
lastReviews: Constants.GRAPHQL_ARGS.LAST_REVIEWS,
firstReviewThreads: Constants.GRAPHQL_ARGS.FIRST_REVIEW_THREADS,
firstClosingIssues: 10,
firstLabels: 25,
lastComments: 1,
lastReviews: 25,
firstReviewThreads: 25,
});
});

Expand Down Expand Up @@ -524,20 +522,20 @@ describe('renderer/utils/forges/github/client.ts', () => {
mockGitHubCloudGitifyNotifications[0].account,
expect.stringMatching(/node0|node1/),
{
firstClosingIssues: 100,
firstIssueFieldValues: 100,
firstLabels: 100,
firstReviewThreads: 100,
firstClosingIssues: 10,
firstIssueFieldValues: 25,
firstLabels: 25,
firstReviewThreads: 25,
isDiscussionNotification0: false,
isDiscussionNotification1: false,
isIssueNotification0: true,
isIssueNotification1: true,
isPullRequestNotification0: false,
isPullRequestNotification1: false,
lastComments: 1,
lastReplies: 10,
lastReviews: 100,
lastThreadedComments: 10,
lastReplies: 5,
lastReviews: 25,
lastThreadedComments: 5,
name0: 'notifications-test',
name1: 'notifications-test',
number0: 1,
Expand Down
89 changes: 89 additions & 0 deletions src/renderer/utils/forges/github/enrich.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,95 @@ vi.mock('./client', async () => {
});

describe('renderer/utils/forges/github/enrich.ts', () => {
describe('batch split retry', () => {
const resourceLimitError = new Error(
'GraphQL request returned errors: Resource limits for this query exceeded',
);

function mockNotifications(count: number) {
return Array.from({ length: count }, (_, index) =>
mockPartialGitifyNotification({
title: `Issue ${index}`,
type: 'Issue',
url: `https://github.com/proxy/api.github.com/repos/gitify-app/notifications-test/issues/${index}` as Link,
}),
);
}

beforeEach(() => {
vi.mocked(client.fetchNotificationDetailsForList).mockReset();
vi.mocked(client.fetchIssueByNumber).mockReset();
vi.mocked(client.fetchIssueByNumber).mockResolvedValue({ repository: {} } as never);
vi.spyOn(logger, 'rendererLogError').mockImplementation(vi.fn());
vi.spyOn(logger, 'rendererLogWarn').mockImplementation(vi.fn());
});

it('sends a single request for a 100-notification batch', async () => {
vi.mocked(client.fetchNotificationDetailsForList).mockResolvedValue(new Map());

await enrichGitHubNotifications(mockNotifications(100));

expect(client.fetchNotificationDetailsForList).toHaveBeenCalledTimes(1);
expect(vi.mocked(client.fetchNotificationDetailsForList).mock.calls[0][0]).toHaveLength(100);
});

it('splits a failed 100-notification batch into two 50-notification requests', async () => {
vi.mocked(client.fetchNotificationDetailsForList).mockImplementation(async (batch) => {
if (batch.length > 50) {
throw resourceLimitError;
}
return new Map();
});

await enrichGitHubNotifications(mockNotifications(100));

const sizes = vi
.mocked(client.fetchNotificationDetailsForList)
.mock.calls.map(([batch]) => batch.length)
.sort((a, b) => a - b);

expect(sizes).toEqual([50, 50, 100]);
});

it('splits a failed 50-notification batch into two 25-notification requests', async () => {
vi.mocked(client.fetchNotificationDetailsForList).mockImplementation(async (batch) => {
if (batch.length > 25) {
throw resourceLimitError;
}
return new Map();
});

await enrichGitHubNotifications(mockNotifications(50));

const sizes = vi
.mocked(client.fetchNotificationDetailsForList)
.mock.calls.map(([batch]) => batch.length)
.sort((a, b) => a - b);

expect(sizes).toEqual([25, 25, 50]);
});

it('does not split a failed batch of 25 or fewer notifications', async () => {
vi.mocked(client.fetchNotificationDetailsForList).mockRejectedValue(resourceLimitError);

await enrichGitHubNotifications(mockNotifications(25));

expect(client.fetchNotificationDetailsForList).toHaveBeenCalledTimes(1);
expect(vi.mocked(client.fetchNotificationDetailsForList).mock.calls[0][0]).toHaveLength(25);
});

it('does not split a non-resource error', async () => {
vi.mocked(client.fetchNotificationDetailsForList).mockRejectedValue(
new Error('GraphQL request returned errors: Something else'),
);

await enrichGitHubNotifications(mockNotifications(50));

expect(client.fetchNotificationDetailsForList).toHaveBeenCalledTimes(1);
expect(vi.mocked(client.fetchNotificationDetailsForList).mock.calls[0][0]).toHaveLength(50);
});
});

it('logs and continues when a per-notification handler throws', async () => {
const rendererLogErrorSpy = vi.spyOn(logger, 'rendererLogError').mockImplementation(vi.fn());
const rendererLogWarnSpy = vi.spyOn(logger, 'rendererLogWarn').mockImplementation(vi.fn());
Expand Down
73 changes: 54 additions & 19 deletions src/renderer/utils/forges/github/enrich.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import { createNotificationHandler } from './handlers';

/**
* Maximum number of notifications batched into a single merged GraphQL query
* by `enrichGitHubNotifications`. Each aliased fragment pulls nested
* connections (labels, comments, replies), so larger batches (50+) trip
* GitHub's "Resource limits for this query exceeded" error and fail the
* whole batch.
* by `enrichGitHubNotifications` before the batch is split. The merged query
* pulls nested connections for every notification, so batches that exceed
* GitHub's undocumented resource limit are retried in progressively smaller
* halves by `fetchInBatches`.
*/
export const GITHUB_API_MERGE_BATCH_SIZE = 25;
export const GITHUB_API_MERGE_BATCH_SIZE = 100;

/**
* Enrich GitHub notifications with additional subject details (state, user,
Expand Down Expand Up @@ -49,20 +49,7 @@ async function fetchInBatches(
batches.push(supportedNotifications.slice(start, start + batchSize));
}

const batchResults = await Promise.all(
batches.map(async (slice, index) => {
try {
return await fetchNotificationDetailsForList(slice);
} catch (err) {
rendererLogError(
'enrichGitHubNotifications',
`Failed to fetch merged notification details for batch ${index + 1}`,
toError(err),
);
return new Map<RawGitifyNotification, FetchMergedDetailsTemplateQuery['repository']>();
}
}),
);
const batchResults = await Promise.all(batches.map((batch) => fetchBatch(batch)));

const merged = new Map<RawGitifyNotification, FetchMergedDetailsTemplateQuery['repository']>();
for (const results of batchResults) {
Expand All @@ -74,6 +61,54 @@ async function fetchInBatches(
return merged;
}

/** Batches at or below this size are not split further; they fall through to per-notification fallback. */
const MIN_SPLIT_BATCH_SIZE = 25;

const RESOURCE_LIMIT_ERROR_MESSAGE = 'Resource limits for this query exceeded';

function isResourceLimitError(error: unknown): boolean {
return error instanceof Error && error.message.includes(RESOURCE_LIMIT_ERROR_MESSAGE);
}

async function fetchBatch(
notifications: RawGitifyNotification[],
): Promise<Map<RawGitifyNotification, FetchMergedDetailsTemplateQuery['repository']>> {
try {
return await fetchNotificationDetailsForList(notifications);
} catch (err) {
if (isResourceLimitError(err)) {
if (notifications.length <= MIN_SPLIT_BATCH_SIZE) {
rendererLogError(
'enrichGitHubNotifications',
`Failed to fetch merged notification details for ${notifications.length} notifications`,
toError(err),
);
return new Map();
}

const midpoint = Math.ceil(notifications.length / 2);
rendererLogWarn(
'enrichGitHubNotifications',
`Splitting merged notification batch of ${notifications.length} after resource limit`,
);

const [first, second] = await Promise.all([
fetchBatch(notifications.slice(0, midpoint)),
fetchBatch(notifications.slice(midpoint)),
]);

return new Map([...first, ...second]);
}

rendererLogError(
'enrichGitHubNotifications',
`Failed to fetch merged notification details for ${notifications.length} notifications`,
toError(err),
);
return new Map();
}
}

async function enrichSingle(
notification: RawGitifyNotification,
fetchedData: FetchMergedDetailsTemplateQuery['repository'] | undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ fragment DiscussionDetails on Discussion {
}
}
labels(first: $firstLabels) {
totalCount
nodes {
...LabelFields
}
Expand Down
Loading
Loading