From 811af9ecd04754e7796840eb04fe20472f716284 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 20 Jan 2026 07:23:50 -0500 Subject: [PATCH 1/7] refactor(api): request function typing Signed-off-by: Adam Setch --- src/renderer/context/App.test.tsx | 10 +- src/renderer/utils/api/client.test.ts | 30 ++-- src/renderer/utils/api/client.ts | 88 +++++++---- src/renderer/utils/api/errors.test.ts | 189 +++++++++++++++--------- src/renderer/utils/api/errors.ts | 28 ++++ src/renderer/utils/api/graphql/types.ts | 9 ++ src/renderer/utils/api/request.test.ts | 18 ++- src/renderer/utils/api/request.ts | 111 ++++++++------ src/renderer/utils/api/types.ts | 9 ++ 9 files changed, 319 insertions(+), 173 deletions(-) diff --git a/src/renderer/context/App.test.tsx b/src/renderer/context/App.test.tsx index 4d68cfd5c..bd995b45e 100644 --- a/src/renderer/context/App.test.tsx +++ b/src/renderer/context/App.test.tsx @@ -184,8 +184,6 @@ describe('renderer/context/App.tsx', () => { }); describe('authentication methods', () => { - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); - it('should call loginWithGitHubApp', async () => { const { button } = renderContextButton('loginWithGitHubApp'); @@ -207,7 +205,9 @@ describe('renderer/context/App.tsx', () => { }); it('should call loginWithPersonalAccessToken', async () => { - apiRequestAuthSpy.mockResolvedValueOnce(null); + const performAuthenticatedRESTRequestSpy = jest + .spyOn(apiRequests, 'performAuthenticatedRESTRequest') + .mockResolvedValueOnce(null); const { button } = renderContextButton('loginWithPersonalAccessToken', { hostname: 'github.com' as Hostname, @@ -220,8 +220,8 @@ describe('renderer/context/App.tsx', () => { expect(mockFetchNotifications).toHaveBeenCalledTimes(1), ); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(apiRequestAuthSpy).toHaveBeenCalledWith( + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledTimes(1); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( 'https://api.github.com/notifications', 'HEAD', 'encrypted', diff --git a/src/renderer/utils/api/client.test.ts b/src/renderer/utils/api/client.test.ts index 71b5f4677..9cd51252b 100644 --- a/src/renderer/utils/api/client.test.ts +++ b/src/renderer/utils/api/client.test.ts @@ -14,6 +14,7 @@ import { import { Constants } from '../../constants'; import type { Hostname, Link, SettingsState, Token } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import * as logger from '../../utils/logger'; import { @@ -39,7 +40,6 @@ import { FetchPullRequestByNumberDocument, type FetchPullRequestByNumberQuery, } from './graphql/generated/graphql'; -import type { ExecutionResultWithHeaders } from './request'; import * as apiRequests from './request'; jest.mock('axios'); @@ -188,7 +188,10 @@ describe('renderer/utils/api/client.ts', () => { describe('getHtmlUrl', () => { it('should return the HTML URL', async () => { - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); + const performAuthenticatedRESTRequestSpy = jest.spyOn( + apiRequests, + 'performAuthenticatedRESTRequest', + ); const requestPromise = Promise.resolve({ data: { @@ -197,7 +200,7 @@ describe('renderer/utils/api/client.ts', () => { }, } as AxiosResponse) as AxiosPromise; - apiRequestAuthSpy.mockResolvedValue(requestPromise); + performAuthenticatedRESTRequestSpy.mockResolvedValue(requestPromise); const result = await getHtmlUrl( 'https://api.github.com/repos/gitify-app/notifications-test/issues/785' as Link, @@ -213,11 +216,14 @@ describe('renderer/utils/api/client.ts', () => { .spyOn(logger, 'rendererLogError') .mockImplementation(); - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); + const performAuthenticatedRESTRequestSpy = jest.spyOn( + apiRequests, + 'performAuthenticatedRESTRequest', + ); const mockError = new Error('Test error'); - apiRequestAuthSpy.mockRejectedValue(mockError); + performAuthenticatedRESTRequestSpy.mockRejectedValue(mockError); await getHtmlUrl( 'https://api.github.com/repos/gitify-app/gitify/issues/785' as Link, @@ -237,7 +243,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchAuthenticatedUserDetails(mockGitHubHostname, mockToken); @@ -263,7 +269,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchDiscussionByNumber(mockNotification); @@ -298,7 +304,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchIssueByNumber(mockNotification); @@ -331,7 +337,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchPullByNumber(mockNotification); @@ -367,7 +373,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList([mockNotification]); @@ -383,7 +389,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList([]); @@ -399,7 +405,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList(mockGitHubCloudGitifyNotifications); diff --git a/src/renderer/utils/api/client.ts b/src/renderer/utils/api/client.ts index 9b07f9281..024563c82 100644 --- a/src/renderer/utils/api/client.ts +++ b/src/renderer/utils/api/client.ts @@ -1,5 +1,4 @@ -import type { AxiosPromise } from 'axios'; -import type { ExecutionResult } from 'graphql'; +import type { AxiosResponse } from 'axios'; import { Constants } from '../../constants'; @@ -11,7 +10,9 @@ import type { SettingsState, Token, } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { + GitHubHtmlUrlResponse, NotificationThreadSubscription, RawCommit, RawCommitComment, @@ -35,8 +36,7 @@ import { } from './graphql/generated/graphql'; import { MergeQueryBuilder } from './graphql/MergeQueryBuilder'; import { - apiRequestAuth, - type ExecutionResultWithHeaders, + performAuthenticatedRESTRequest, performGraphQLRequest, performGraphQLRequestString, } from './request'; @@ -54,11 +54,15 @@ import { export function headNotifications( hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += 'notifications'; - return apiRequestAuth(url.toString() as Link, 'HEAD', token); + return performAuthenticatedRESTRequest( + 'HEAD', + url.toString() as Link, + token, + ); } /** @@ -69,15 +73,15 @@ export function headNotifications( export function listNotificationsForAuthenticatedUser( account: Account, settings: SettingsState, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(account.hostname); url.pathname += 'notifications'; url.searchParams.append('participating', String(settings.participating)); url.searchParams.append('all', String(settings.fetchReadNotifications)); - return apiRequestAuth( - url.toString() as Link, + return performAuthenticatedRESTRequest( 'GET', + url.toString() as Link, account.token, {}, settings.fetchAllNotifications, @@ -94,11 +98,16 @@ export function markNotificationThreadAsRead( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}`; - return apiRequestAuth(url.toString() as Link, 'PATCH', token, {}); + return performAuthenticatedRESTRequest( + 'PATCH', + url.toString() as Link, + token, + {}, + ); } /** @@ -113,11 +122,16 @@ export function markNotificationThreadAsDone( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}`; - return apiRequestAuth(url.toString() as Link, 'DELETE', token, {}); + return performAuthenticatedRESTRequest( + 'DELETE', + url.toString() as Link, + token, + {}, + ); } /** @@ -129,13 +143,18 @@ export function ignoreNotificationThreadSubscription( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}/subscription`; - return apiRequestAuth(url.toString() as Link, 'PUT', token, { - ignored: true, - }); + return performAuthenticatedRESTRequest( + 'PUT', + url.toString() as Link, + token, + { + ignored: true, + }, + ); } /** @@ -143,8 +162,11 @@ export function ignoreNotificationThreadSubscription( * * Endpoint documentation: https://docs.github.com/en/rest/commits/commits#get-a-commit */ -export function getCommit(url: Link, token: Token): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +export function getCommit( + url: Link, + token: Token, +): Promise> { + return performAuthenticatedRESTRequest('GET', url, token); } /** @@ -156,8 +178,8 @@ export function getCommit(url: Link, token: Token): AxiosPromise { export function getCommitComment( url: Link, token: Token, -): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +): Promise> { + return performAuthenticatedRESTRequest('GET', url, token); } /** @@ -165,16 +187,24 @@ export function getCommitComment( * * Endpoint documentation: https://docs.github.com/en/rest/releases/releases#get-a-release */ -export function getRelease(url: Link, token: Token): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +export function getRelease( + url: Link, + token: Token, +): Promise> { + return performAuthenticatedRESTRequest('GET', url, token); } /** * Get the `html_url` from the GitHub response */ -export async function getHtmlUrl(url: Link, token: Token): Promise { +export async function getHtmlUrl( + url: Link, + token: Token, +): Promise> { try { - const response = (await apiRequestAuth(url, 'GET', token)).data; + const response = ( + await performAuthenticatedRESTRequest + )('GET', url, token); return response.html_url; } catch (err) { @@ -192,7 +222,7 @@ export async function getHtmlUrl(url: Link, token: Token): Promise { export async function fetchAuthenticatedUserDetails( hostname: Hostname, token: Token, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(hostname); return performGraphQLRequest( @@ -207,7 +237,7 @@ export async function fetchAuthenticatedUserDetails( */ export async function fetchDiscussionByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); @@ -234,7 +264,7 @@ export async function fetchDiscussionByNumber( */ export async function fetchIssueByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); @@ -257,7 +287,7 @@ export async function fetchIssueByNumber( */ export async function fetchPullByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); diff --git a/src/renderer/utils/api/errors.test.ts b/src/renderer/utils/api/errors.test.ts index 90a9f31ac..4dac73023 100644 --- a/src/renderer/utils/api/errors.test.ts +++ b/src/renderer/utils/api/errors.test.ts @@ -3,89 +3,120 @@ import { AxiosError, type AxiosResponse } from 'axios'; import { EVENTS } from '../../../shared/events'; import type { Link } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { GitHubRESTError } from './types'; import { Errors } from '../errors'; -import { determineFailureType } from './errors'; +import * as rendererLogger from '../logger'; +import { assertNoGraphQLErrors, determineFailureType } from './errors'; describe('renderer/utils/api/errors.ts', () => { - it('network error', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_NETWORK, - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.NETWORK); - }); - - describe('bad request errors', () => { - it('bad credentials', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 401, - response: createMockResponse(401, 'Bad credentials'), - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.BAD_CREDENTIALS); - }); - - it('missing scopes', async () => { + describe('determineFailureType', () => { + it('network error', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse(403, "Missing the 'notifications' scope"), + code: AxiosError.ERR_NETWORK, }; const result = determineFailureType( mockError as AxiosError, ); - expect(result).toBe(Errors.MISSING_SCOPES); + expect(result).toBe(Errors.NETWORK); }); - it('rate limited - primary', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse(403, 'API rate limit exceeded'), - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.RATE_LIMITED); + describe('bad request errors', () => { + it('bad credentials', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 401, + response: createMockResponse(401, 'Bad credentials'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.BAD_CREDENTIALS); + }); + + it('missing scopes', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse( + 403, + "Missing the 'notifications' scope", + ), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.MISSING_SCOPES); + }); + + it('rate limited - primary', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse(403, 'API rate limit exceeded'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.RATE_LIMITED); + }); + + it('rate limited - secondary', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse( + 403, + 'You have exceeded a secondary rate limit', + ), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.RATE_LIMITED); + }); + + it('unhandled bad request error', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 400, + response: createMockResponse(403, 'Oops! Something went wrong.'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.UNKNOWN); + }); }); - it('rate limited - secondary', async () => { + it('bad credentials - safe storage', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse( - 403, - 'You have exceeded a secondary rate limit', - ), + message: `Error invoking remote method '${EVENTS.SAFE_STORAGE_DECRYPT}': Error: Error while decrypting the ciphertext provided to safeStorage.decryptString. Ciphertext does not appear to be encrypted.`, }; const result = determineFailureType( mockError as AxiosError, ); - expect(result).toBe(Errors.RATE_LIMITED); + expect(result).toBe(Errors.BAD_CREDENTIALS); }); - it('unhandled bad request error', async () => { + it('unknown error', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 400, - response: createMockResponse(403, 'Oops! Something went wrong.'), + code: 'anything', }; const result = determineFailureType( @@ -95,29 +126,43 @@ describe('renderer/utils/api/errors.ts', () => { expect(result).toBe(Errors.UNKNOWN); }); }); +}); - it('bad credentials - safe storage', async () => { - const mockError: Partial> = { - message: `Error invoking remote method '${EVENTS.SAFE_STORAGE_DECRYPT}': Error: Error while decrypting the ciphertext provided to safeStorage.decryptString. Ciphertext does not appear to be encrypted.`, - }; +describe('assertNoGraphQLErrors', () => { + it('throws and logs when GraphQL errors are present', () => { + const logSpy = jest + .spyOn(rendererLogger, 'rendererLogError') + .mockImplementation(); + + const payload = { + data: {}, + errors: [{}], + } as unknown as GitHubGraphQLResponse; - const result = determineFailureType( - mockError as AxiosError, + expect(() => assertNoGraphQLErrors('test-context', payload)).toThrow( + 'GraphQL request returned errors', ); - expect(result).toBe(Errors.BAD_CREDENTIALS); + expect(logSpy).toHaveBeenCalled(); }); - it('unknown error', async () => { - const mockError: Partial> = { - code: 'anything', + it('does not throw when errors array is empty or undefined', () => { + const payloadNoErrors: GitHubGraphQLResponse = { + data: {}, + headers: {}, + }; + const payloadEmptyErrors: GitHubGraphQLResponse = { + data: {}, + errors: [], + headers: {}, }; - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.UNKNOWN); + expect(() => + assertNoGraphQLErrors('test-context', payloadNoErrors), + ).not.toThrow(); + expect(() => + assertNoGraphQLErrors('test-context', payloadEmptyErrors), + ).not.toThrow(); }); }); diff --git a/src/renderer/utils/api/errors.ts b/src/renderer/utils/api/errors.ts index a9508a4d7..fb888dc92 100644 --- a/src/renderer/utils/api/errors.ts +++ b/src/renderer/utils/api/errors.ts @@ -1,10 +1,18 @@ import { AxiosError } from 'axios'; import type { GitifyError } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { GitHubRESTError } from './types'; import { Errors } from '../errors'; +import { rendererLogError } from '../logger'; +/** + * Determine the failure type based on an Axios error. + * + * @param err The Axios error + * @returns The Gitify error type + */ export function determineFailureType( err: AxiosError, ): GitifyError { @@ -44,3 +52,23 @@ export function determineFailureType( return Errors.UNKNOWN; } + +/** + * Assert that a GraphQL response does not contain errors. + * Logs and throws if `errors` array is present and non-empty. + * + * @param context The context of the GraphQL request + * @param payload The GraphQL response payload + */ +export function assertNoGraphQLErrors( + context: string, + payload: GitHubGraphQLResponse, // TODO should this be ExecutionResult? +) { + if (Array.isArray(payload.errors) && payload.errors.length > 0) { + const err = new Error('GraphQL request returned errors'); + + rendererLogError(context, 'GraphQL errors present in response', err); + + throw err; + } +} diff --git a/src/renderer/utils/api/graphql/types.ts b/src/renderer/utils/api/graphql/types.ts index f5960c472..8149c6dbe 100644 --- a/src/renderer/utils/api/graphql/types.ts +++ b/src/renderer/utils/api/graphql/types.ts @@ -1,3 +1,12 @@ +import type { ExecutionResult } from 'graphql'; + +/** + * GitHub GraphQL API response type with HTTP response headers + */ +export type GitHubGraphQLResponse = ExecutionResult & { + headers: Record; +}; + export type FragmentInfo = { name: string; typeCondition: string; diff --git a/src/renderer/utils/api/request.test.ts b/src/renderer/utils/api/request.test.ts index de501988b..824ab1e47 100644 --- a/src/renderer/utils/api/request.test.ts +++ b/src/renderer/utils/api/request.test.ts @@ -11,8 +11,8 @@ import type { Link } from '../../types'; import { FetchAuthenticatedUserDetailsDocument } from './graphql/generated/graphql'; import { - apiRequestAuth, getHeaders, + performAuthenticatedRESTRequest, performGraphQLRequest, performGraphQLRequestString, shouldRequestWithNoCache, @@ -36,7 +36,7 @@ describe('renderer/utils/api/request.ts', () => { it('should make an authenticated request with the correct parameters', async () => { const data = { key: 'value' }; - await apiRequestAuth(url, method, mockToken, data); + await performAuthenticatedRESTRequest(method, url, mockToken, data); expect(axios).toHaveBeenCalledWith({ method, @@ -49,7 +49,7 @@ describe('renderer/utils/api/request.ts', () => { it('should make an authenticated request with the correct parameters and default data', async () => { const data = {}; - await apiRequestAuth(url, method, mockToken); + await performAuthenticatedRESTRequest(method, url, mockToken, data); expect(axios).toHaveBeenCalledWith({ method, @@ -112,21 +112,25 @@ describe('renderer/utils/api/request.ts', () => { describe('shouldRequestWithNoCache', () => { it('shouldRequestWithNoCache', () => { expect( - shouldRequestWithNoCache('https://example.com/api/v3/notifications'), + shouldRequestWithNoCache( + 'https://example.com/api/v3/notifications' as Link, + ), ).toBe(true); expect( shouldRequestWithNoCache( - 'https://example.com/login/oauth/access_token', + 'https://example.com/login/oauth/access_token' as Link, ), ).toBe(true); expect( - shouldRequestWithNoCache('https://example.com/notifications'), + shouldRequestWithNoCache('https://example.com/notifications' as Link), ).toBe(true); expect( - shouldRequestWithNoCache('https://example.com/some/other/endpoint'), + shouldRequestWithNoCache( + 'https://example.com/some/other/endpoint' as Link, + ), ).toBe(false); }); }); diff --git a/src/renderer/utils/api/request.ts b/src/renderer/utils/api/request.ts index b3971b206..50498fd28 100644 --- a/src/renderer/utils/api/request.ts +++ b/src/renderer/utils/api/request.ts @@ -1,48 +1,38 @@ -import axios, { - type AxiosPromise, - type AxiosResponse, - type Method, -} from 'axios'; -import type { ExecutionResult } from 'graphql'; +import axios, { type AxiosResponse, type Method } from 'axios'; import type { Link, Token } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import { decryptValue } from '../comms'; import { rendererLogError } from '../logger'; +import { assertNoGraphQLErrors } from './errors'; import type { TypedDocumentString } from './graphql/generated/graphql'; import { getNextURLFromLinkHeader } from './utils'; /** - * ExecutionResult with HTTP response headers - */ -export type ExecutionResultWithHeaders = ExecutionResult & { - headers: Record; -}; - -/** - * Perform an authenticated API request + * Perform an authenticated REST API request * - * @param url - * @param method - * @param token - * @param data - * @param fetchAllRecords whether to fetch all records or just the first page - * @returns + * @param method The REST http method + * @param url The API url + * @param token A GitHub token (decrypted) + * @param data The API request body + * @param fetchAllRecords Whether to fetch all records or just the first page + * @returns Resolves to a GitHub REST response */ -export async function apiRequestAuth( - url: Link, +export async function performAuthenticatedRESTRequest( method: Method, + url: Link, token: Token, - data = {}, + data: Record = {}, fetchAllRecords = false, -): AxiosPromise | null { +): Promise { const headers = await getHeaders(url, token); if (!fetchAllRecords) { return axios({ method, url, data, headers }); } - let response: AxiosResponse | null = null; + let response: AxiosResponse | null = null; let combinedData = []; try { @@ -56,36 +46,39 @@ export async function apiRequestAuth( break; } + // Accumulate paginated array results combinedData = combinedData.concat(response.data); // Accumulate data nextUrl = getNextURLFromLinkHeader(response); } } catch (err) { - rendererLogError('apiRequestAuth', 'API request failed:', err); + rendererLogError( + 'performAuthenticatedRESTRequest', + 'API request failed:', + err, + ); throw err; } - return { - ...response, - data: combinedData, - } as AxiosResponse; + // Return the combined array of records as the result data + return combinedData as TResult; } /** - * Perform a GraphQL API request for account + * Perform a GraphQL API request with typed operation document * - * @param account - * @param query - * @param variables - * @returns + * @param url The API url + * @param query The GraphQL operation/query statement + * @param variables The GraphQL operation variables + * @returns Resolves to a GitHub GraphQL response */ export async function performGraphQLRequest( url: Link, token: Token, query: TypedDocumentString, ...[variables]: TVariables extends Record ? [] : [TVariables] -) { +): Promise> { const headers = await getHeaders(url, token); return axios({ @@ -97,24 +90,36 @@ export async function performGraphQLRequest( }, headers: headers, }).then((response) => { + // Check GraphQL errors + assertNoGraphQLErrors( + 'performGraphQLRequest', + response.data as GitHubGraphQLResponse, + ); + return { ...response.data, headers: response.headers, }; - }) as Promise>; + }) as Promise>; } /** * Perform a GraphQL API request using a raw query string instead of a TypedDocumentString. * - * Useful for dynamically composed queries (e.g., merged queries built at runtime). + * Useful for dynamically composed queries (e.g: merged queries built at runtime). + * + * @param url The API url + * @param token The GitHub token (decrypted) + * @param query The GraphQL operation/query statement + * @param variables The GraphQL operation variables + * @returns Resolves to a GitHub GraphQL response */ export async function performGraphQLRequestString( url: Link, token: Token, query: string, variables?: Record, -): Promise> { +): Promise> { const headers = await getHeaders(url, token); return axios({ @@ -126,20 +131,27 @@ export async function performGraphQLRequestString( }, headers: headers, }).then((response) => { + // Check GraphQL errors + assertNoGraphQLErrors( + 'performGraphQLRequestString', + response.data as GitHubGraphQLResponse, + ); + return { ...response.data, headers: response.headers, - } as ExecutionResultWithHeaders; + } as GitHubGraphQLResponse; }); } /** - * Return true if the request should be made with no-cache + * Determine if the API request should be made with no-cache header + * based on the API url path * - * @param url - * @returns boolean + * @param url The API url + * @returns Whether a cache heading should be set or not */ -export function shouldRequestWithNoCache(url: string) { +export function shouldRequestWithNoCache(url: Link): boolean { const parsedUrl = new URL(url); switch (parsedUrl.pathname) { @@ -155,11 +167,14 @@ export function shouldRequestWithNoCache(url: string) { /** * Construct headers for API requests * - * @param username - * @param token - * @returns + * @param username A GitHub account username + * @param token A GitHub token (decrypted) + * @returns A headers object to use with API requests */ -export async function getHeaders(url: Link, token?: Token) { +export async function getHeaders( + url: Link, + token?: Token, +): Promise> { const headers: Record = { Accept: 'application/json', 'Cache-Control': shouldRequestWithNoCache(url) ? 'no-cache' : '', diff --git a/src/renderer/utils/api/types.ts b/src/renderer/utils/api/types.ts index 2ada18d0b..bc86d1d24 100644 --- a/src/renderer/utils/api/types.ts +++ b/src/renderer/utils/api/types.ts @@ -19,3 +19,12 @@ export type RawGitHubNotification = components['schemas']['thread']; export type RawRelease = components['schemas']['release']; export type RawUser = components['schemas']['simple-user']; + +/** + * Minimal response for endpoints where we're only interested in the `html_url`. + * + * Used when following a notification thread's subject URL or latest comment URL. + */ +export type GitHubHtmlUrlResponse = { + html_url: string; +}; From 24f2d241cdd1b10f4421d4a8aab0333e2dc3229d Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 20 Jan 2026 07:35:03 -0500 Subject: [PATCH 2/7] refactor(api): request function typing Signed-off-by: Adam Setch --- src/renderer/utils/api/client.ts | 19 +++++-------------- src/renderer/utils/api/request.ts | 13 +++++++------ src/renderer/utils/helpers.test.ts | 20 +++++++++++++++++--- src/renderer/utils/helpers.ts | 21 +++++++++++++-------- 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/src/renderer/utils/api/client.ts b/src/renderer/utils/api/client.ts index 024563c82..0ee288fbe 100644 --- a/src/renderer/utils/api/client.ts +++ b/src/renderer/utils/api/client.ts @@ -21,7 +21,6 @@ import type { } from './types'; import { isAnsweredDiscussionFeatureSupported } from '../features'; -import { rendererLogError } from '../logger'; import { createNotificationHandler } from '../notifications/handlers'; import { FetchAuthenticatedUserDetailsDocument, @@ -201,19 +200,11 @@ export async function getHtmlUrl( url: Link, token: Token, ): Promise> { - try { - const response = ( - await performAuthenticatedRESTRequest - )('GET', url, token); - - return response.html_url; - } catch (err) { - rendererLogError( - 'getHtmlUrl', - `error occurred while fetching html url for ${url}`, - err, - ); - } + return performAuthenticatedRESTRequest( + 'GET', + url, + token, + ); } /** diff --git a/src/renderer/utils/api/request.ts b/src/renderer/utils/api/request.ts index 50498fd28..5a0eee767 100644 --- a/src/renderer/utils/api/request.ts +++ b/src/renderer/utils/api/request.ts @@ -17,7 +17,7 @@ import { getNextURLFromLinkHeader } from './utils'; * @param token A GitHub token (decrypted) * @param data The API request body * @param fetchAllRecords Whether to fetch all records or just the first page - * @returns Resolves to a GitHub REST response + * @returns Resolves to a GitHub REST response with the specified type */ export async function performAuthenticatedRESTRequest( method: Method, @@ -25,7 +25,7 @@ export async function performAuthenticatedRESTRequest( token: Token, data: Record = {}, fetchAllRecords = false, -): Promise { +): Promise> { const headers = await getHeaders(url, token); if (!fetchAllRecords) { @@ -33,7 +33,7 @@ export async function performAuthenticatedRESTRequest( } let response: AxiosResponse | null = null; - let combinedData = []; + let combinedData: unknown[] = []; try { let nextUrl: string | null = url; @@ -47,7 +47,7 @@ export async function performAuthenticatedRESTRequest( } // Accumulate paginated array results - combinedData = combinedData.concat(response.data); // Accumulate data + combinedData = combinedData.concat(response.data); nextUrl = getNextURLFromLinkHeader(response); } @@ -61,8 +61,9 @@ export async function performAuthenticatedRESTRequest( throw err; } - // Return the combined array of records as the result data - return combinedData as TResult; + // Return a response object with the combined array of records as data + response.data = combinedData as TResult; + return response; } /** diff --git a/src/renderer/utils/helpers.test.ts b/src/renderer/utils/helpers.test.ts index 44ea8d1ed..6012de59f 100644 --- a/src/renderer/utils/helpers.test.ts +++ b/src/renderer/utils/helpers.test.ts @@ -4,6 +4,8 @@ import { ChevronRightIcon, } from '@primer/octicons-react'; +import type { AxiosResponse } from 'axios'; + import { mockGitifyNotification } from '../__mocks__/notifications-mocks'; import { mockToken } from '../__mocks__/state-mocks'; @@ -66,7 +68,7 @@ describe('renderer/utils/helpers.ts', () => { describe('generateGitHubWebUrl', () => { const mockHtmlUrl = - 'https://github.com/gitify-app/notifications-test/issues/785'; + 'https://github.com/gitify-app/notifications-test/issues/785' as Link; const mockNotificationReferrer = 'notification_referrer_id=MDE4Ok5vdGlmaWNhdGlvblRocmVhZDEzODY2MTA5NjoxMjM0NTY3ODk%3D'; @@ -115,7 +117,13 @@ describe('renderer/utils/helpers.ts', () => { htmlUrl: mockSubjectHtmlUrl, } as GitifySubject; - getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); + getHtmlUrlSpy.mockResolvedValue( + Promise.resolve({ + data: { + html_url: mockHtmlUrl, + }, + } as AxiosResponse), + ); const result = await generateGitHubWebUrl({ ...mockGitifyNotification, @@ -144,7 +152,13 @@ describe('renderer/utils/helpers.ts', () => { htmlUrl: mockSubjectHtmlUrl, } as GitifySubject; - getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); + getHtmlUrlSpy.mockResolvedValue( + Promise.resolve({ + data: { + html_url: mockHtmlUrl, + }, + } as AxiosResponse), + ); const result = await generateGitHubWebUrl({ ...mockGitifyNotification, diff --git a/src/renderer/utils/helpers.ts b/src/renderer/utils/helpers.ts index 04788b63a..9d78dc88d 100644 --- a/src/renderer/utils/helpers.ts +++ b/src/renderer/utils/helpers.ts @@ -56,15 +56,20 @@ export async function generateGitHubWebUrl( } else { try { if (notification.subject.latestCommentUrl) { - url.href = await getHtmlUrl( - notification.subject.latestCommentUrl, - notification.account.token, - ); + const response = ( + await getHtmlUrl( + notification.subject.latestCommentUrl, + notification.account.token, + ) + ).data; + + url.href = response.html_url; } else if (notification.subject.url) { - url.href = await getHtmlUrl( - notification.subject.url, - notification.account.token, - ); + const response = ( + await getHtmlUrl(notification.subject.url, notification.account.token) + ).data; + + url.href = response.html_url; } } catch (err) { rendererLogError( From eb4d560104a2e4a79c939116f940bd60779db38b Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 20 Jan 2026 07:51:04 -0500 Subject: [PATCH 3/7] refactor(api): request function typing Signed-off-by: Adam Setch --- src/renderer/context/App.test.tsx | 2 +- src/renderer/utils/api/client.test.ts | 174 +++++++++++--------------- 2 files changed, 73 insertions(+), 103 deletions(-) diff --git a/src/renderer/context/App.test.tsx b/src/renderer/context/App.test.tsx index bd995b45e..d833d246f 100644 --- a/src/renderer/context/App.test.tsx +++ b/src/renderer/context/App.test.tsx @@ -222,8 +222,8 @@ describe('renderer/context/App.tsx', () => { expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledTimes(1); expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( - 'https://api.github.com/notifications', 'HEAD', + 'https://api.github.com/notifications', 'encrypted', ); }); diff --git a/src/renderer/utils/api/client.test.ts b/src/renderer/utils/api/client.test.ts index 9cd51252b..01e2e99ea 100644 --- a/src/renderer/utils/api/client.test.ts +++ b/src/renderer/utils/api/client.test.ts @@ -1,22 +1,15 @@ -import axios, { type AxiosPromise, type AxiosResponse } from 'axios'; - import { mockGitHubCloudAccount } from '../../__mocks__/account-mocks'; import { mockGitHubCloudGitifyNotifications, mockPartialGitifyNotification, } from '../../__mocks__/notifications-mocks'; import { mockToken } from '../../__mocks__/state-mocks'; -import { - mockAuthHeaders, - mockNonCachedAuthHeaders, -} from './__mocks__/request-mocks'; import { Constants } from '../../constants'; import type { Hostname, Link, SettingsState, Token } from '../../types'; import type { GitHubGraphQLResponse } from './graphql/types'; -import * as logger from '../../utils/logger'; import { fetchAuthenticatedUserDetails, fetchDiscussionByNumber, @@ -48,6 +41,11 @@ const mockGitHubHostname = 'github.com' as Hostname; const mockThreadId = '1234'; describe('renderer/utils/api/client.ts', () => { + const performAuthenticatedRESTRequestSpy = jest.spyOn( + apiRequests, + 'performAuthenticatedRESTRequest', + ); + afterEach(() => { jest.clearAllMocks(); }); @@ -55,12 +53,11 @@ describe('renderer/utils/api/client.ts', () => { it('headNotifications - should fetch notifications head', async () => { await headNotifications(mockGitHubHostname, mockToken); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications', - headers: mockNonCachedAuthHeaders, - method: 'HEAD', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'HEAD', + 'https://api.github.com/notifications', + mockToken, + ); }); describe('listNotificationsForAuthenticatedUser', () => { @@ -68,6 +65,7 @@ describe('renderer/utils/api/client.ts', () => { const mockSettings: Partial = { participating: true, fetchReadNotifications: false, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -75,18 +73,20 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=true&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=true&all=false', + mockGitHubCloudAccount.token, + {}, + false, + ); }); it('should list participating and watching notifications for user', async () => { const mockSettings: Partial = { participating: false, fetchReadNotifications: false, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -94,18 +94,20 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=false', + mockGitHubCloudAccount.token, + {}, + false, + ); }); - it('should include all=true when fetchReadNotifications is enabled', async () => { + it('should list read and done notifications for user', async () => { const mockSettings: Partial = { participating: false, fetchReadNotifications: true, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -113,18 +115,20 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=true', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=true', + mockGitHubCloudAccount.token, + {}, + false, + ); }); - it('should include all=false when fetchReadNotifications is disabled', async () => { + it('should unpaginate notifications list for user', async () => { const mockSettings: Partial = { participating: false, fetchReadNotifications: false, + fetchAllNotifications: true, }; await listNotificationsForAuthenticatedUser( @@ -132,12 +136,13 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=false ', + mockGitHubCloudAccount.token, + {}, + true, + ); }); }); @@ -148,12 +153,12 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}`, - headers: mockAuthHeaders, - method: 'PATCH', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'PATCH', + `https://api.github.com/notifications/threads/${mockThreadId}`, + mockToken, + {}, + ); }); it('markNotificationThreadAsDone - should mark notification thread as done', async () => { @@ -163,12 +168,12 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}`, - headers: mockAuthHeaders, - method: 'DELETE', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'DELETE', + `https://api.github.com/notifications/threads/${mockThreadId}`, + mockToken, + {}, + ); }); it('ignoreNotificationThreadSubscription - should ignore notification thread subscription', async () => { @@ -178,60 +183,25 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}/subscription`, - headers: mockAuthHeaders, - method: 'PUT', - data: { ignored: true }, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'PUT', + `https://api.github.com/notifications/threads/${mockThreadId}/subscription`, + mockToken, + { ignored: true }, + ); }); - describe('getHtmlUrl', () => { - it('should return the HTML URL', async () => { - const performAuthenticatedRESTRequestSpy = jest.spyOn( - apiRequests, - 'performAuthenticatedRESTRequest', - ); - - const requestPromise = Promise.resolve({ - data: { - html_url: - 'https://github.com/gitify-app/notifications-test/issues/785', - }, - } as AxiosResponse) as AxiosPromise; - - performAuthenticatedRESTRequestSpy.mockResolvedValue(requestPromise); - - const result = await getHtmlUrl( - 'https://api.github.com/repos/gitify-app/notifications-test/issues/785' as Link, - '123' as Token, - ); - expect(result).toBe( - 'https://github.com/gitify-app/notifications-test/issues/785', - ); - }); - - it('should handle error', async () => { - const rendererLogErrorSpy = jest - .spyOn(logger, 'rendererLogError') - .mockImplementation(); - - const performAuthenticatedRESTRequestSpy = jest.spyOn( - apiRequests, - 'performAuthenticatedRESTRequest', - ); - - const mockError = new Error('Test error'); - - performAuthenticatedRESTRequestSpy.mockRejectedValue(mockError); - - await getHtmlUrl( - 'https://api.github.com/repos/gitify-app/gitify/issues/785' as Link, - '123' as Token, - ); + it('getHtmlUrl - should return the HTML URL', async () => { + await getHtmlUrl( + 'https://api.github.com/repos/gitify-app/notifications-test/issues/785' as Link, + '123' as Token, + ); - expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1); - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/repos/gitify-app/notifications-test/issues/785', + '123', + ); }); it('fetchAuthenticatedUserDetails calls performGraphQLRequest with correct args', async () => { From 9cc842dbe692540c5b274034ab6295a7bc6af8d1 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Tue, 20 Jan 2026 07:55:12 -0500 Subject: [PATCH 4/7] refactor(api): request function typing Signed-off-by: Adam Setch --- src/renderer/utils/api/client.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/renderer/utils/api/client.test.ts b/src/renderer/utils/api/client.test.ts index 01e2e99ea..f31399293 100644 --- a/src/renderer/utils/api/client.test.ts +++ b/src/renderer/utils/api/client.test.ts @@ -125,6 +125,8 @@ describe('renderer/utils/api/client.ts', () => { }); it('should unpaginate notifications list for user', async () => { + performAuthenticatedRESTRequestSpy.mockImplementation(); + const mockSettings: Partial = { participating: false, fetchReadNotifications: false, @@ -138,7 +140,7 @@ describe('renderer/utils/api/client.ts', () => { expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( 'GET', - 'https://api.github.com/notifications?participating=false&all=false ', + 'https://api.github.com/notifications?participating=false&all=false', mockGitHubCloudAccount.token, {}, true, From a9bed256c67c177bee4374566ebdf8ed383a202c Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Thu, 22 Jan 2026 07:05:53 -0500 Subject: [PATCH 5/7] Merge branch 'main' into refactor/api-request-shape-2 Signed-off-by: Adam Setch --- .../utils/api/graphql/generated/graphql.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/renderer/utils/api/graphql/generated/graphql.ts b/src/renderer/utils/api/graphql/generated/graphql.ts index 57a1d9b18..e1294c5fb 100644 --- a/src/renderer/utils/api/graphql/generated/graphql.ts +++ b/src/renderer/utils/api/graphql/generated/graphql.ts @@ -12419,6 +12419,8 @@ export type Mutation = { reprioritizeSubIssue?: Maybe; /** Set review requests on a pull request. */ requestReviews?: Maybe; + /** Set review requests on a pull request using login strings instead of IDs. */ + requestReviewsByLogin?: Maybe; /** Rerequests an existing check suite. */ rerequestCheckSuite?: Maybe; /** Marks a review thread as resolved. */ @@ -13586,6 +13588,12 @@ export type MutationRequestReviewsArgs = { }; +/** The root query for implementing GraphQL mutations. */ +export type MutationRequestReviewsByLoginArgs = { + input: RequestReviewsByLoginInput; +}; + + /** The root query for implementing GraphQL mutations. */ export type MutationRerequestCheckSuiteArgs = { input: RerequestCheckSuiteInput; @@ -28217,6 +28225,35 @@ export type ReprioritizeSubIssuePayload = { issue?: Maybe; }; +/** Autogenerated input type of RequestReviewsByLogin */ +export type RequestReviewsByLoginInput = { + /** The logins of the bots to request reviews from, including the [bot] suffix (e.g., 'copilot-pull-request-reviewer[bot]'). */ + botLogins?: InputMaybe>; + /** A unique identifier for the client performing the mutation. */ + clientMutationId?: InputMaybe; + /** The Node ID of the pull request to modify. */ + pullRequestId: Scalars['ID']['input']; + /** The slugs of the teams to request reviews from (format: 'org/team-slug'). */ + teamSlugs?: InputMaybe>; + /** Add users to the set rather than replace. */ + union?: InputMaybe; + /** The login strings of the users to request reviews from. */ + userLogins?: InputMaybe>; +}; + +/** Autogenerated return type of RequestReviewsByLogin. */ +export type RequestReviewsByLoginPayload = { + __typename?: 'RequestReviewsByLoginPayload'; + /** Identifies the actor who performed the event. */ + actor?: Maybe; + /** A unique identifier for the client performing the mutation. */ + clientMutationId?: Maybe; + /** The pull request that is getting requests. */ + pullRequest?: Maybe; + /** The edge from the pull request to the requested reviewers. */ + requestedReviewersEdge?: Maybe; +}; + /** Autogenerated input type of RequestReviews */ export type RequestReviewsInput = { /** The Node IDs of the bot to request. */ From 0ba28fabbb447cd9c9d88e2f19988c2d22a5535b Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Thu, 22 Jan 2026 07:34:30 -0500 Subject: [PATCH 6/7] Merge branch 'main' into refactor/api-request-shape-2 Signed-off-by: Adam Setch --- src/renderer/utils/api/errors.test.ts | 37 +++++++++++++++++---------- src/renderer/utils/api/errors.ts | 15 ++++++++--- src/renderer/utils/api/request.ts | 11 +++----- 3 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/renderer/utils/api/errors.test.ts b/src/renderer/utils/api/errors.test.ts index 4dac73023..cd86b04f5 100644 --- a/src/renderer/utils/api/errors.test.ts +++ b/src/renderer/utils/api/errors.test.ts @@ -1,5 +1,7 @@ import { AxiosError, type AxiosResponse } from 'axios'; +import type { DeepPartial } from '../../__helpers__/test-utils'; + import { EVENTS } from '../../../shared/events'; import type { Link } from '../../types'; @@ -130,39 +132,46 @@ describe('renderer/utils/api/errors.ts', () => { describe('assertNoGraphQLErrors', () => { it('throws and logs when GraphQL errors are present', () => { - const logSpy = jest + const rendererLogErrorSpy = jest .spyOn(rendererLogger, 'rendererLogError') .mockImplementation(); - const payload = { + const payload: GitHubGraphQLResponse = { data: {}, - errors: [{}], - } as unknown as GitHubGraphQLResponse; + errors: [ + { + message: 'Something went wrong', + locations: [{ line: 1, column: 1 }], + }, + ], + } as DeepPartial< + GitHubGraphQLResponse + > as GitHubGraphQLResponse; expect(() => assertNoGraphQLErrors('test-context', payload)).toThrow( 'GraphQL request returned errors', ); - expect(logSpy).toHaveBeenCalled(); + expect(rendererLogErrorSpy).toHaveBeenCalled(); }); it('does not throw when errors array is empty or undefined', () => { - const payloadNoErrors: GitHubGraphQLResponse = { + const payload: GitHubGraphQLResponse = { data: {}, + errors: [], headers: {}, }; - const payloadEmptyErrors: GitHubGraphQLResponse = { + + expect(() => assertNoGraphQLErrors('test-context', payload)).not.toThrow(); + }); + + it('does not throw when errors array is undefined', () => { + const payload: GitHubGraphQLResponse = { data: {}, - errors: [], headers: {}, }; - expect(() => - assertNoGraphQLErrors('test-context', payloadNoErrors), - ).not.toThrow(); - expect(() => - assertNoGraphQLErrors('test-context', payloadEmptyErrors), - ).not.toThrow(); + expect(() => assertNoGraphQLErrors('test-context', payload)).not.toThrow(); }); }); diff --git a/src/renderer/utils/api/errors.ts b/src/renderer/utils/api/errors.ts index fb888dc92..93d38c649 100644 --- a/src/renderer/utils/api/errors.ts +++ b/src/renderer/utils/api/errors.ts @@ -60,12 +60,21 @@ export function determineFailureType( * @param context The context of the GraphQL request * @param payload The GraphQL response payload */ -export function assertNoGraphQLErrors( +export function assertNoGraphQLErrors( context: string, - payload: GitHubGraphQLResponse, // TODO should this be ExecutionResult? + payload: GitHubGraphQLResponse, // TODO should this be ExecutionResult? ) { if (Array.isArray(payload.errors) && payload.errors.length > 0) { - const err = new Error('GraphQL request returned errors'); + const errorMessages = payload.errors + .map((graphQLError) => graphQLError.message) + .filter((msg): msg is string => Boolean(msg)) + .join('; '); + + const err = new Error( + errorMessages + ? `GraphQL request returned errors: ${errorMessages}` + : 'GraphQL request returned errors', + ); rendererLogError(context, 'GraphQL errors present in response', err); diff --git a/src/renderer/utils/api/request.ts b/src/renderer/utils/api/request.ts index 5a0eee767..4f4d54aa1 100644 --- a/src/renderer/utils/api/request.ts +++ b/src/renderer/utils/api/request.ts @@ -91,11 +91,7 @@ export async function performGraphQLRequest( }, headers: headers, }).then((response) => { - // Check GraphQL errors - assertNoGraphQLErrors( - 'performGraphQLRequest', - response.data as GitHubGraphQLResponse, - ); + assertNoGraphQLErrors('performGraphQLRequest', response.data); return { ...response.data, @@ -132,10 +128,9 @@ export async function performGraphQLRequestString( }, headers: headers, }).then((response) => { - // Check GraphQL errors - assertNoGraphQLErrors( + assertNoGraphQLErrors( 'performGraphQLRequestString', - response.data as GitHubGraphQLResponse, + response.data, ); return { From bcdcb602fa0108adb7c47ea199412a60b799dd73 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Thu, 22 Jan 2026 09:17:45 -0500 Subject: [PATCH 7/7] Merge branch 'main' into refactor/api-request-shape-2 Signed-off-by: Adam Setch --- src/renderer/utils/api/errors.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderer/utils/api/errors.ts b/src/renderer/utils/api/errors.ts index 93d38c649..23e13a4fe 100644 --- a/src/renderer/utils/api/errors.ts +++ b/src/renderer/utils/api/errors.ts @@ -1,4 +1,5 @@ import { AxiosError } from 'axios'; +import type { GraphQLError } from 'graphql'; import type { GitifyError } from '../../types'; import type { GitHubGraphQLResponse } from './graphql/types'; @@ -62,12 +63,11 @@ export function determineFailureType( */ export function assertNoGraphQLErrors( context: string, - payload: GitHubGraphQLResponse, // TODO should this be ExecutionResult? + payload: GitHubGraphQLResponse, ) { if (Array.isArray(payload.errors) && payload.errors.length > 0) { const errorMessages = payload.errors - .map((graphQLError) => graphQLError.message) - .filter((msg): msg is string => Boolean(msg)) + .map((graphQLError: GraphQLError) => graphQLError.message) .join('; '); const err = new Error(