diff --git a/src/renderer/utils/forges/github/capabilities.test.ts b/src/renderer/utils/forges/github/capabilities.test.ts index b278953e9..cedee0941 100644 --- a/src/renderer/utils/forges/github/capabilities.test.ts +++ b/src/renderer/utils/forges/github/capabilities.test.ts @@ -5,6 +5,7 @@ import { import { githubCapabilities, + getGitHubCapabilities, supportsAnsweredDiscussion, supportsStackedPullRequests, } from './capabilities'; @@ -94,4 +95,20 @@ describe('renderer/utils/forges/github/capabilities.ts', () => { ).toBe(false); }); }); + + describe('getGitHubCapabilities', () => { + it('enables all gated capabilities for GitHub Cloud', () => { + expect(getGitHubCapabilities(mockGitHubCloudAccount)).toEqual({ + stackedPullRequests: true, + answeredDiscussion: true, + }); + }); + + it('disables gated capabilities for GitHub Enterprise Server', () => { + expect(getGitHubCapabilities(mockGitHubEnterpriseServerAccount)).toEqual({ + stackedPullRequests: false, + answeredDiscussion: false, + }); + }); + }); }); diff --git a/src/renderer/utils/forges/github/capabilities.ts b/src/renderer/utils/forges/github/capabilities.ts index e4472afa2..1edeb31ab 100644 --- a/src/renderer/utils/forges/github/capabilities.ts +++ b/src/renderer/utils/forges/github/capabilities.ts @@ -57,3 +57,25 @@ export function supportsAnsweredDiscussion(account: Account): boolean { export function supportsStackedPullRequests(account: Account): boolean { return isGitHubCloudHost(account.hostname); } + +/** + * The set of capabilities that gate GraphQL field selections via the custom + * `@gated(requires: ...)` directive. The keys must match the `requires` + * argument used in the GraphQL documents. + */ +export type GitHubGatedCapabilities = { + stackedPullRequests: boolean; + answeredDiscussion: boolean; +}; + +/** + * Resolve the gated-field capabilities for an account. Consumed by the query + * sanitizer in `graphql/utils.ts` to strip `@gated` selections that the + * account's GitHub platform/version does not support. + */ +export function getGitHubCapabilities(account: Account): GitHubGatedCapabilities { + return { + stackedPullRequests: supportsStackedPullRequests(account), + answeredDiscussion: supportsAnsweredDiscussion(account), + }; +} diff --git a/src/renderer/utils/forges/github/client.test.ts b/src/renderer/utils/forges/github/client.test.ts index 16fa1cfd5..7349c0d74 100644 --- a/src/renderer/utils/forges/github/client.test.ts +++ b/src/renderer/utils/forges/github/client.test.ts @@ -1,6 +1,9 @@ import type { ExecutionResult } from 'graphql'; -import { mockGitHubCloudAccount } from '../../../__mocks__/account-mocks'; +import { + mockGitHubCloudAccount, + mockGitHubEnterpriseServerAccount, +} from '../../../__mocks__/account-mocks'; import { mockGitHubCloudGitifyNotifications, mockPartialGitifyNotification, @@ -31,11 +34,9 @@ import { markNotificationThreadAsRead, } from './client'; import { - FetchDiscussionByNumberDocument, - type FetchDiscussionByNumberQuery, FetchIssueByNumberDocument, + type FetchDiscussionByNumberQuery, type FetchIssueByNumberQuery, - FetchPullRequestByNumberDocument, type FetchPullRequestByNumberQuery, } from './graphql/generated/graphql'; import type { OctokitClient } from './octokit'; @@ -312,8 +313,8 @@ describe('renderer/utils/forges/github/client.ts', () => { }); }); - it('fetchDiscussionByNumber calls performGraphQLRequest with correct args', async () => { - const performGraphQLRequestSpy = vi.mocked(apiRequests.performGraphQLRequest); + it('fetchDiscussionByNumber calls performGraphQLRequestString with sanitized query', async () => { + const performGraphQLRequestStringSpy = vi.mocked(apiRequests.performGraphQLRequestString); const mockNotification = mockPartialGitifyNotification({ title: 'Some discussion', @@ -321,23 +322,47 @@ describe('renderer/utils/forges/github/client.ts', () => { type: 'Discussion', }); - performGraphQLRequestSpy.mockResolvedValue({} as ExecutionResult); + performGraphQLRequestStringSpy.mockResolvedValue( + {} as ExecutionResult, + ); await fetchDiscussionByNumber(mockNotification); - expect(performGraphQLRequestSpy).toHaveBeenCalledWith( - mockNotification.account, - FetchDiscussionByNumberDocument, - { - 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, - includeIsAnswered: true, - }, + const [account, query, variables] = performGraphQLRequestStringSpy.mock.calls[0]; + expect(account).toBe(mockNotification.account); + expect(query).toContain('isAnswered'); + expect(query).not.toContain('@gated'); + expect(query).toContain('query FetchDiscussionByNumber'); + expect(variables).toEqual({ + 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, + }); + }); + + it('fetchDiscussionByNumber strips isAnswered for GitHub Enterprise Server accounts', async () => { + const performGraphQLRequestStringSpy = vi.mocked(apiRequests.performGraphQLRequestString); + + const mockNotification = mockPartialGitifyNotification({ + title: 'Some discussion', + url: 'https://github.gitify.io/api/v3/repos/gitify-app/gitify/discussion/123' as Link, + type: 'Discussion', + }); + mockNotification.account = mockGitHubEnterpriseServerAccount; + + performGraphQLRequestStringSpy.mockResolvedValue( + {} as ExecutionResult, ); + + await fetchDiscussionByNumber(mockNotification); + + const [account, query] = performGraphQLRequestStringSpy.mock.calls[0]; + expect(account).toBe(mockGitHubEnterpriseServerAccount); + expect(query).not.toContain('isAnswered'); + expect(query).not.toContain('@gated'); }); it('fetchIssueByNumber calls performGraphQLRequest with correct args', async () => { @@ -366,8 +391,8 @@ describe('renderer/utils/forges/github/client.ts', () => { ); }); - it('fetchPullByNumber calls performGraphQLRequest with correct args', async () => { - const performGraphQLRequestSpy = vi.mocked(apiRequests.performGraphQLRequest); + it('fetchPullByNumber calls performGraphQLRequestString with sanitized query', async () => { + const performGraphQLRequestStringSpy = vi.mocked(apiRequests.performGraphQLRequestString); const mockNotification = mockPartialGitifyNotification({ title: 'Some pull request', @@ -375,26 +400,49 @@ describe('renderer/utils/forges/github/client.ts', () => { type: 'PullRequest', }); - performGraphQLRequestSpy.mockResolvedValue( + performGraphQLRequestStringSpy.mockResolvedValue( {} as ExecutionResult, ); await fetchPullByNumber(mockNotification); - expect(performGraphQLRequestSpy).toHaveBeenCalledWith( - mockNotification.account, - FetchPullRequestByNumberDocument, - { - 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, - includeStackEntry: true, - }, + const [account, query, variables] = performGraphQLRequestStringSpy.mock.calls[0]; + expect(account).toBe(mockNotification.account); + expect(query).toContain('stackEntry'); + expect(query).not.toContain('@gated'); + expect(query).toContain('query FetchPullRequestByNumber'); + expect(variables).toEqual({ + 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, + }); + }); + + it('fetchPullByNumber strips stackEntry for GitHub Enterprise Server accounts', async () => { + const performGraphQLRequestStringSpy = vi.mocked(apiRequests.performGraphQLRequestString); + + const mockNotification = mockPartialGitifyNotification({ + title: 'Some pull request', + url: 'https://github.gitify.io/api/v3/repos/gitify-app/gitify/pulls/123' as Link, + type: 'PullRequest', + }); + mockNotification.account = mockGitHubEnterpriseServerAccount; + + performGraphQLRequestStringSpy.mockResolvedValue( + {} as ExecutionResult, ); + + await fetchPullByNumber(mockNotification); + + const [account, query] = performGraphQLRequestStringSpy.mock.calls[0]; + expect(account).toBe(mockGitHubEnterpriseServerAccount); + expect(query).not.toContain('stackEntry'); + expect(query).not.toContain('@gated'); + expect(query).toContain('query FetchPullRequestByNumber'); }); describe('fetchNotificationDetailsForList', () => { @@ -440,8 +488,6 @@ describe('renderer/utils/forges/github/client.ts', () => { { firstClosingIssues: 100, firstLabels: 100, - includeIsAnswered: true, - includeStackEntry: true, isDiscussionNotification0: false, isDiscussionNotification1: false, isIssueNotification0: true, @@ -460,6 +506,36 @@ describe('renderer/utils/forges/github/client.ts', () => { owner1: 'gitify-app', }, ); + + const query = performGraphQLRequestStringSpy.mock.calls[0][1]; + expect(query).toContain('stackEntry'); + expect(query).toContain('isAnswered'); + expect(query).not.toContain('@gated'); + }); + + it('fetchNotificationDetailsForList strips gated fields for GitHub Enterprise Server accounts', async () => { + const performGraphQLRequestStringSpy = vi.mocked(apiRequests.performGraphQLRequestString); + + const notifications = mockGitHubCloudGitifyNotifications.map((notification) => ({ + ...notification, + account: mockGitHubEnterpriseServerAccount, + })); + + performGraphQLRequestStringSpy.mockResolvedValue({ + data: {}, + headers: {}, + } as ExecutionResult); + + await fetchNotificationDetailsForList(notifications); + + const [account, query, variables] = performGraphQLRequestStringSpy.mock.calls[0]; + expect(account).toBe(mockGitHubEnterpriseServerAccount); + expect(query).not.toContain('stackEntry'); + expect(query).not.toContain('isAnswered'); + expect(query).not.toContain('@gated'); + expect(query).toContain('FetchMergedNotifications'); + expect(variables).not.toHaveProperty('includeStackEntry'); + expect(variables).not.toHaveProperty('includeIsAnswered'); }); }); }); diff --git a/src/renderer/utils/forges/github/client.ts b/src/renderer/utils/forges/github/client.ts index fee67607b..82d1ec704 100644 --- a/src/renderer/utils/forges/github/client.ts +++ b/src/renderer/utils/forges/github/client.ts @@ -14,7 +14,7 @@ import type { } from './types'; import { reportServerPollInterval } from '../../notifications/pollInterval'; -import { supportsAnsweredDiscussion, supportsStackedPullRequests } from './capabilities'; +import { getGitHubCapabilities } from './capabilities'; import { FetchDiscussionByNumberDocument, type FetchDiscussionByNumberQuery, @@ -25,6 +25,7 @@ import { type FetchPullRequestByNumberQuery, } from './graphql/generated/graphql'; import { MergeQueryBuilder } from './graphql/MergeQueryBuilder'; +import { stripGatedSelections } from './graphql/utils'; import { createNotificationHandler } from './handlers'; import { createOctokitClient, createOctokitClientUncached } from './octokit'; import { performGraphQLRequest, performGraphQLRequestString } from './request'; @@ -206,14 +207,18 @@ export async function fetchDiscussionByNumber( ): Promise { const number = getNumberFromUrl(notification.subject.url!); - return performGraphQLRequest(notification.account, FetchDiscussionByNumberDocument, { + const query = stripGatedSelections( + FetchDiscussionByNumberDocument.toString(), + getGitHubCapabilities(notification.account), + ); + + return performGraphQLRequestString(notification.account, query, { owner: notification.repository.owner.login, name: notification.repository.name, number: number, firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS, lastThreadedComments: Constants.GRAPHQL_ARGS.LAST_THREADED_COMMENTS, lastReplies: Constants.GRAPHQL_ARGS.LAST_REPLIES, - includeIsAnswered: supportsAnsweredDiscussion(notification.account), }); } @@ -242,7 +247,12 @@ export async function fetchPullByNumber( ): Promise { const number = getNumberFromUrl(notification.subject.url!); - return performGraphQLRequest(notification.account, FetchPullRequestByNumberDocument, { + const query = stripGatedSelections( + FetchPullRequestByNumberDocument.toString(), + getGitHubCapabilities(notification.account), + ); + + return performGraphQLRequestString(notification.account, query, { owner: notification.repository.owner.login, name: notification.repository.name, number: number, @@ -250,7 +260,6 @@ export async function fetchPullByNumber( firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS, lastComments: Constants.GRAPHQL_ARGS.LAST_COMMENTS, lastReviews: Constants.GRAPHQL_ARGS.LAST_REVIEWS, - includeStackEntry: supportsStackedPullRequests(notification.account), }); } /** * Fetch notification details for supported types (ie: Discussions, Issues and Pull Requests). @@ -297,8 +306,6 @@ export async function fetchNotificationDetailsForList( } builder.setSharedVariables({ - includeIsAnswered: supportsAnsweredDiscussion(notifications[0].account), - includeStackEntry: supportsStackedPullRequests(notifications[0].account), firstClosingIssues: Constants.GRAPHQL_ARGS.FIRST_CLOSING_ISSUES, firstLabels: Constants.GRAPHQL_ARGS.FIRST_LABELS, lastComments: Constants.GRAPHQL_ARGS.LAST_COMMENTS, @@ -307,7 +314,10 @@ export async function fetchNotificationDetailsForList( lastReviews: Constants.GRAPHQL_ARGS.LAST_REVIEWS, }); - const query = builder.getGraphQLQuery(); + const query = stripGatedSelections( + builder.getGraphQLQuery(), + getGitHubCapabilities(notifications[0].account), + ); const variables = builder.getGraphQLVariables(); const response = await performGraphQLRequestString(notifications[0].account, query, variables); diff --git a/src/renderer/utils/forges/github/graphql/MergeQueryBuilder.test.ts b/src/renderer/utils/forges/github/graphql/MergeQueryBuilder.test.ts index 2a3cf94bf..179b92c32 100644 --- a/src/renderer/utils/forges/github/graphql/MergeQueryBuilder.test.ts +++ b/src/renderer/utils/forges/github/graphql/MergeQueryBuilder.test.ts @@ -12,8 +12,6 @@ describe('renderer/utils/forges/github/graphql/MergeQueryBuilder.ts', () => { lastReviews: 4, firstLabels: 10, firstClosingIssues: 8, - includeIsAnswered: true, - includeStackEntry: true, }; const nodeVarsA: FetchBatchMergedTemplateIndexedBaseVariables = { @@ -51,8 +49,6 @@ describe('renderer/utils/forges/github/graphql/MergeQueryBuilder.ts', () => { expect(query).toContain('$lastReviews: Int'); expect(query).toContain('$firstLabels: Int'); expect(query).toContain('$firstClosingIssues: Int'); - expect(query).toContain('$includeIsAnswered: Boolean!'); - expect(query).toContain('$includeStackEntry: Boolean!'); expect(query).toContain('$owner0: String!'); expect(query).toContain('$name0: String!'); @@ -83,8 +79,6 @@ describe('renderer/utils/forges/github/graphql/MergeQueryBuilder.ts', () => { lastReviews: 4, firstLabels: 10, firstClosingIssues: 8, - includeIsAnswered: true, - includeStackEntry: true, owner0: 'octocat', name0: 'hello-world', number0: 123, diff --git a/src/renderer/utils/forges/github/graphql/common.graphql b/src/renderer/utils/forges/github/graphql/common.graphql index a031db794..a9d08ce29 100644 --- a/src/renderer/utils/forges/github/graphql/common.graphql +++ b/src/renderer/utils/forges/github/graphql/common.graphql @@ -1,3 +1,9 @@ +# Custom directive that marks a field selection as gated behind a GitHub +# capability (e.g. `stackedPullRequests`, `answeredDiscussion`). The query +# sanitizer strips the directive (and the field itself for unsupported +# capabilities) before the query is sent, so it never reaches GitHub. +directive @gated(requires: String!) on FIELD + fragment AuthorFields on Actor { login htmlUrl: url diff --git a/src/renderer/utils/forges/github/graphql/discussion.graphql b/src/renderer/utils/forges/github/graphql/discussion.graphql index 7f8f4c9cc..69060dec3 100644 --- a/src/renderer/utils/forges/github/graphql/discussion.graphql +++ b/src/renderer/utils/forges/github/graphql/discussion.graphql @@ -7,7 +7,6 @@ query FetchDiscussionByNumber( $lastThreadedComments: Int $lastReplies: Int $firstLabels: Int - $includeIsAnswered: Boolean! ) { repository(owner: $owner, name: $name) { discussion(number: $number) { @@ -21,7 +20,7 @@ fragment DiscussionDetails on Discussion { number title stateReason - isAnswered @include(if: $includeIsAnswered) + isAnswered @gated(requires: "answeredDiscussion") url author { ...AuthorFields diff --git a/src/renderer/utils/forges/github/graphql/generated/graphql.ts b/src/renderer/utils/forges/github/graphql/generated/graphql.ts index 032c60f65..a97ea88e3 100644 --- a/src/renderer/utils/forges/github/graphql/generated/graphql.ts +++ b/src/renderer/utils/forges/github/graphql/generated/graphql.ts @@ -131,11 +131,10 @@ export type FetchDiscussionByNumberQueryVariables = Exact<{ lastThreadedComments?: number | null | undefined; lastReplies?: number | null | undefined; firstLabels?: number | null | undefined; - includeIsAnswered: boolean; }>; -export type FetchDiscussionByNumberQuery = { repository: { discussion: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered?: boolean | null, url: Link, author: +export type FetchDiscussionByNumberQuery = { repository: { discussion: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered: boolean | null, url: Link, author: | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Bot' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'EnterpriseUserAccount' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Mannequin' } @@ -155,7 +154,7 @@ export type FetchDiscussionByNumberQuery = { repository: { discussion: { __typen | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'User' } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null> | null }, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; -export type DiscussionDetailsFragment = { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered?: boolean | null, url: Link, author: +export type DiscussionDetailsFragment = { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered: boolean | null, url: Link, author: | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Bot' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'EnterpriseUserAccount' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Mannequin' } @@ -247,12 +246,10 @@ export type FetchMergedDetailsTemplateQueryVariables = Exact<{ lastReviews?: number | null | undefined; firstLabels?: number | null | undefined; firstClosingIssues?: number | null | undefined; - includeIsAnswered: boolean; - includeStackEntry: boolean; }>; -export type FetchMergedDetailsTemplateQuery = { repository: { discussion?: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered?: boolean | null, url: Link, author: +export type FetchMergedDetailsTemplateQuery = { repository: { discussion?: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered: boolean | null, url: Link, author: | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Bot' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'EnterpriseUserAccount' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Mannequin' } @@ -306,9 +303,9 @@ export type FetchMergedDetailsTemplateQuery = { repository: { discussion?: { __t | { login: string } | { login: string } | { login: string } - | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry?: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; + | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; -export type MergedDetailsQueryTemplateFragment = { repository: { discussion?: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered?: boolean | null, url: Link, author: +export type MergedDetailsQueryTemplateFragment = { repository: { discussion?: { __typename: 'Discussion', number: number, title: string, stateReason: DiscussionStateReason | null, isAnswered: boolean | null, url: Link, author: | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Bot' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'EnterpriseUserAccount' } | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Mannequin' } @@ -362,7 +359,7 @@ export type MergedDetailsQueryTemplateFragment = { repository: { discussion?: { | { login: string } | { login: string } | { login: string } - | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry?: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; + | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; export type FetchPullRequestByNumberQueryVariables = Exact<{ owner: string; @@ -372,7 +369,6 @@ export type FetchPullRequestByNumberQueryVariables = Exact<{ lastComments?: number | null | undefined; lastReviews?: number | null | undefined; firstClosingIssues?: number | null | undefined; - includeStackEntry: boolean; }>; @@ -400,7 +396,7 @@ export type FetchPullRequestByNumberQuery = { repository: { pullRequest: { __typ | { login: string } | { login: string } | { login: string } - | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry?: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; + | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null } | null } | null }; export type PullRequestDetailsFragment = { __typename: 'PullRequest', number: number, title: string, url: Link, state: PullRequestState, merged: boolean, isDraft: boolean, isInMergeQueue: boolean, milestone: { state: MilestoneState, title: string } | null, author: | { login: string, htmlUrl: Link, avatarUrl: Link, type: 'Bot' } @@ -426,7 +422,7 @@ export type PullRequestDetailsFragment = { __typename: 'PullRequest', number: nu | { login: string } | { login: string } | { login: string } - | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry?: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null }; + | null } | null> | null } | null, labels: { nodes: Array<{ name: string, color: string } | null> | null } | null, closingIssuesReferences: { nodes: Array<{ number: number } | null> | null } | null, stackEntry: { position: number, stack: { size: number } | null } | null, reactions: { totalCount: number }, reactionGroups: Array<{ content: ReactionContent, reactors: { totalCount: number } }> | null }; export type PullRequestReviewFieldsFragment = { state: PullRequestReviewState, author: | { login: string } @@ -543,7 +539,7 @@ export const DiscussionDetailsFragmentDoc = new TypedDocumentString(` number title stateReason - isAnswered @include(if: $includeIsAnswered) + isAnswered @gated(requires: "answeredDiscussion") url author { ...AuthorFields @@ -743,7 +739,7 @@ export const PullRequestDetailsFragmentDoc = new TypedDocumentString(` number } } - stackEntry @include(if: $includeStackEntry) { + stackEntry @gated(requires: "stackedPullRequests") { position stack { size @@ -821,7 +817,7 @@ fragment DiscussionDetails on Discussion { number title stateReason - isAnswered @include(if: $includeIsAnswered) + isAnswered @gated(requires: "answeredDiscussion") url author { ...AuthorFields @@ -969,7 +965,7 @@ fragment PullRequestDetails on PullRequest { number } } - stackEntry @include(if: $includeStackEntry) { + stackEntry @gated(requires: "stackedPullRequests") { position stack { size @@ -989,7 +985,7 @@ fragment PullRequestReviewFields on PullRequestReview { } }`, {"fragmentName":"MergedDetailsQueryTemplate"}) as unknown as TypedDocumentString; export const FetchDiscussionByNumberDocument = new TypedDocumentString(` - query FetchDiscussionByNumber($owner: String!, $name: String!, $number: Int!, $lastThreadedComments: Int, $lastReplies: Int, $firstLabels: Int, $includeIsAnswered: Boolean!) { + query FetchDiscussionByNumber($owner: String!, $name: String!, $number: Int!, $lastThreadedComments: Int, $lastReplies: Int, $firstLabels: Int) { repository(owner: $owner, name: $name) { discussion(number: $number) { ...DiscussionDetails @@ -1017,7 +1013,7 @@ fragment DiscussionDetails on Discussion { number title stateReason - isAnswered @include(if: $includeIsAnswered) + isAnswered @gated(requires: "answeredDiscussion") url author { ...AuthorFields @@ -1135,7 +1131,7 @@ fragment IssueDetails on Issue { } }`) as unknown as TypedDocumentString; export const FetchMergedDetailsTemplateDocument = new TypedDocumentString(` - query FetchMergedDetailsTemplate($ownerINDEX: String!, $nameINDEX: String!, $numberINDEX: Int!, $isDiscussionNotificationINDEX: Boolean!, $isIssueNotificationINDEX: Boolean!, $isPullRequestNotificationINDEX: Boolean!, $lastComments: Int, $lastThreadedComments: Int, $lastReplies: Int, $lastReviews: Int, $firstLabels: Int, $firstClosingIssues: Int, $includeIsAnswered: Boolean!, $includeStackEntry: Boolean!) { + query FetchMergedDetailsTemplate($ownerINDEX: String!, $nameINDEX: String!, $numberINDEX: Int!, $isDiscussionNotificationINDEX: Boolean!, $isIssueNotificationINDEX: Boolean!, $isPullRequestNotificationINDEX: Boolean!, $lastComments: Int, $lastThreadedComments: Int, $lastReplies: Int, $lastReviews: Int, $firstLabels: Int, $firstClosingIssues: Int) { ...MergedDetailsQueryTemplate } fragment AuthorFields on Actor { @@ -1163,7 +1159,7 @@ fragment DiscussionDetails on Discussion { number title stateReason - isAnswered @include(if: $includeIsAnswered) + isAnswered @gated(requires: "answeredDiscussion") url author { ...AuthorFields @@ -1324,7 +1320,7 @@ fragment PullRequestDetails on PullRequest { number } } - stackEntry @include(if: $includeStackEntry) { + stackEntry @gated(requires: "stackedPullRequests") { position stack { size @@ -1344,7 +1340,7 @@ fragment PullRequestReviewFields on PullRequestReview { } }`) as unknown as TypedDocumentString; export const FetchPullRequestByNumberDocument = new TypedDocumentString(` - query FetchPullRequestByNumber($owner: String!, $name: String!, $number: Int!, $firstLabels: Int, $lastComments: Int, $lastReviews: Int, $firstClosingIssues: Int, $includeStackEntry: Boolean!) { + query FetchPullRequestByNumber($owner: String!, $name: String!, $number: Int!, $firstLabels: Int, $lastComments: Int, $lastReviews: Int, $firstClosingIssues: Int) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { ...PullRequestDetails @@ -1430,7 +1426,7 @@ fragment PullRequestDetails on PullRequest { number } } - stackEntry @include(if: $includeStackEntry) { + stackEntry @gated(requires: "stackedPullRequests") { position stack { size diff --git a/src/renderer/utils/forges/github/graphql/merged.graphql b/src/renderer/utils/forges/github/graphql/merged.graphql index 8af450f16..8bfd9da90 100644 --- a/src/renderer/utils/forges/github/graphql/merged.graphql +++ b/src/renderer/utils/forges/github/graphql/merged.graphql @@ -13,8 +13,6 @@ query FetchMergedDetailsTemplate( $lastReviews: Int $firstLabels: Int $firstClosingIssues: Int - $includeIsAnswered: Boolean! - $includeStackEntry: Boolean! ) { ...MergedDetailsQueryTemplate } diff --git a/src/renderer/utils/forges/github/graphql/pull.graphql b/src/renderer/utils/forges/github/graphql/pull.graphql index 5711e5d6f..8256124c1 100644 --- a/src/renderer/utils/forges/github/graphql/pull.graphql +++ b/src/renderer/utils/forges/github/graphql/pull.graphql @@ -8,7 +8,6 @@ query FetchPullRequestByNumber( $lastComments: Int $lastReviews: Int $firstClosingIssues: Int - $includeStackEntry: Boolean! ) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { @@ -76,7 +75,7 @@ fragment PullRequestDetails on PullRequest { number } } - stackEntry @include(if: $includeStackEntry) { + stackEntry @gated(requires: "stackedPullRequests") { position stack { size diff --git a/src/renderer/utils/forges/github/graphql/utils.test.ts b/src/renderer/utils/forges/github/graphql/utils.test.ts index 1f4caffbb..2e2d0902d 100644 --- a/src/renderer/utils/forges/github/graphql/utils.test.ts +++ b/src/renderer/utils/forges/github/graphql/utils.test.ts @@ -1,5 +1,6 @@ import { FetchMergedDetailsTemplateDocument, + FetchPullRequestByNumberDocument, IssueDetailsFragmentDoc, MergedDetailsQueryTemplateFragmentDoc, } from './generated/graphql'; @@ -9,6 +10,7 @@ import { extractNonIndexedVariableDefinitions, extractNonQueryFragments, extractQueryFragments, + stripGatedSelections, } from './utils'; describe('renderer/utils/forges/github/graphql/utils.ts', () => { @@ -87,7 +89,7 @@ describe('renderer/utils/forges/github/graphql/utils.ts', () => { ); expect(varDefs).not.toBeNull(); - expect(varDefs.length).toBe(8); + expect(varDefs.length).toBe(6); expect(varDefs.flatMap((v) => v.name)).toEqual([ 'lastComments', 'lastThreadedComments', @@ -95,8 +97,6 @@ describe('renderer/utils/forges/github/graphql/utils.ts', () => { 'lastReviews', 'firstLabels', 'firstClosingIssues', - 'includeIsAnswered', - 'includeStackEntry', ]); }); }); @@ -119,4 +119,60 @@ describe('renderer/utils/forges/github/graphql/utils.ts', () => { expect(result).not.toContain('$isIssueNotificationINDEX'); }); }); + + describe('stripGatedSelections', () => { + const allCapabilities = { + stackedPullRequests: true, + answeredDiscussion: true, + }; + + it('strips @gated directives but keeps gated fields when supported', () => { + const result = stripGatedSelections( + FetchPullRequestByNumberDocument.toString(), + allCapabilities, + ); + + expect(result).not.toContain('@gated'); + expect(result).not.toContain('gated'); + expect(result).toContain('stackEntry'); + expect(result).toContain('query FetchPullRequestByNumber'); + }); + + it('removes gated fields when their capability is unsupported', () => { + const result = stripGatedSelections(FetchPullRequestByNumberDocument.toString(), { + ...allCapabilities, + stackedPullRequests: false, + }); + + expect(result).not.toContain('stackEntry'); + expect(result).not.toContain('@gated'); + expect(result).toContain('query FetchPullRequestByNumber'); + expect(result).toContain('repository'); + }); + + it('strips @gated directives from the merged template when supported', () => { + const result = stripGatedSelections( + FetchMergedDetailsTemplateDocument.toString(), + allCapabilities, + ); + + expect(result).not.toContain('@gated'); + expect(result).toContain('stackEntry'); + expect(result).toContain('isAnswered'); + expect(result).toContain('query FetchMergedDetailsTemplate'); + }); + + it('removes gated fields from the merged template when unsupported', () => { + const result = stripGatedSelections(FetchMergedDetailsTemplateDocument.toString(), { + stackedPullRequests: false, + answeredDiscussion: false, + }); + + expect(result).not.toContain('stackEntry'); + expect(result).not.toContain('isAnswered'); + expect(result).not.toContain('@gated'); + expect(result).toContain('query FetchMergedDetailsTemplate'); + expect(result).toContain('PullRequestDetails'); + }); + }); }); diff --git a/src/renderer/utils/forges/github/graphql/utils.ts b/src/renderer/utils/forges/github/graphql/utils.ts index e170d8903..922f047fb 100644 --- a/src/renderer/utils/forges/github/graphql/utils.ts +++ b/src/renderer/utils/forges/github/graphql/utils.ts @@ -1,4 +1,4 @@ -import { type DocumentNode, parse, print, type TypeNode } from 'graphql'; +import { type DocumentNode, parse, print, type TypeNode, visit } from 'graphql'; import type { FragmentInfo, VariableDef } from './types'; @@ -6,6 +6,47 @@ import type { TypedDocumentString } from './generated/graphql'; const INDEXED_SUFFIX = 'INDEX'; +const GATED_DIRECTIVE = 'gated'; +const GATED_REQUIRES_ARG = 'requires'; + +/** + * Return a copy of a GraphQL document with `@gated(requires: ...)` selections + * processed for the given capabilities. + * + * Fields gated behind a capability that is not supported are removed entirely, + * and every `@gated` directive is stripped from the remaining selections. The + * directive is a client-side marker only and must never reach the GitHub server. + */ +export function stripGatedSelections(doc: string, capabilities: Record): string { + const ast: DocumentNode = parse(doc); + + const sanitized = visit(ast, { + Field(node) { + const gated = node.directives?.find((directive) => directive.name.value === GATED_DIRECTIVE); + if (!gated) { + return undefined; + } + + const requiresArg = gated.arguments?.find((arg) => arg.name.value === GATED_REQUIRES_ARG); + const capability = + requiresArg?.value.kind === 'StringValue' ? requiresArg.value.value : undefined; + + if (capability && !capabilities[capability]) { + return null; + } + + return { + ...node, + directives: node.directives?.filter( + (directive) => directive.name.value !== GATED_DIRECTIVE, + ), + }; + }, + }); + + return print(sanitized); +} + // AST-based helpers for robust fragment parsing and deduping function toDocumentNode(doc: TypedDocumentString): DocumentNode {