diff --git a/packages/server/src/app.ts b/packages/server/src/app.ts index 431137c3..128c592b 100644 --- a/packages/server/src/app.ts +++ b/packages/server/src/app.ts @@ -594,7 +594,11 @@ export function createApp(deps: AppDeps): express.Express { /** Who one of the user's own pull requests is waiting on. */ const ownStatus = (request: ReviewRequestRow): string => { const parts = request.draft ? [draftBadge] : []; - const reviewers = request.requestedReviewers ?? []; + if (!request.requestedReviewers) { + parts.push('Status unknown'); + return parts.join(' '); + } + const reviewers = request.requestedReviewers; const openThreads = request.openThreads ?? 0; if (openThreads === 0 && request.moreThreads) { parts.push('Open threads unknown ·'); diff --git a/packages/server/src/github.ts b/packages/server/src/github.ts index 703014c0..1a7d4c73 100644 --- a/packages/server/src/github.ts +++ b/packages/server/src/github.ts @@ -113,34 +113,24 @@ const OWN_PULLS_GRAPHQL = `query($q:String!){ search(type:ISSUE,query:$q,first:50){ nodes{ ... on PullRequest{ - number - title url - updatedAt - isDraft - repository{owner{login} name} - author{login} reviewDecision reviewRequests(first:20){totalCount nodes{requestedReviewer{... on User{login} ... on Bot{login} ... on Team{name}}}} - reviewThreads(first:${THREADS_PAGE}){totalCount nodes{isResolved isOutdated}} + reviewThreads(first:${THREADS_PAGE}){totalCount nodes{isResolved}} } } } }`; interface RawOwnPull { - number?: number; - title?: string; url?: string; - updatedAt?: string; - isDraft?: boolean; - repository?: { owner?: { login?: string }; name?: string }; - author?: { login?: string } | null; reviewDecision?: string | null; - reviewRequests?: { totalCount?: number; nodes?: { requestedReviewer?: { login?: string; name?: string } | null }[] }; - reviewThreads?: { totalCount?: number; nodes?: { isResolved?: boolean }[] }; + reviewRequests?: { totalCount?: number; nodes?: { requestedReviewer?: { login?: string; name?: string } | null }[] } | null; + reviewThreads?: { totalCount?: number; nodes?: { isResolved?: boolean }[] } | null; } +type OwnPullStatus = Required>; + const REVIEW_THREADS_QUERY = `query($owner:String!,$repo:String!,$number:Int!){ repository(owner:$owner,name:$repo){ pullRequest(number:$number){ @@ -338,53 +328,53 @@ export class GitHubApi { } /** - * The token's own open pull requests, most recently updated first, with who each is waiting on. - * Review threads and requested reviewers live on GraphQL only. + * The token's own open pull requests, most recently updated first, with who each is waiting on where + * GitHub lets the token see it. Review threads and requested reviewers live on GraphQL only, which + * refuses them in repositories the App is not installed on; those pull requests come without them. */ async ownPulls(token: string): Promise { - let json: { data?: { search?: { nodes?: (RawOwnPull | null)[] } }; errors?: { message?: string }[] }; + const pulls = await this.searchPulls(token, OWN_PULLS_QUERY); + if (pulls.length === 0) { + return pulls; + } + let statuses: Map; try { - json = await this.json(token, '/graphql', { - method: 'POST', - body: { query: OWN_PULLS_GRAPHQL, variables: { q: `${OWN_PULLS_QUERY} sort:updated-desc` } }, - }); + statuses = await this.ownPullStatuses(token); } catch (err) { - if (err instanceof GitHubApiError && err.status === 422) { - console.warn(`Searching pull requests failed: ${err.message}`); - return []; - } - throw err; + console.warn(`Reading who pull requests wait on failed: ${err instanceof Error ? err.message : String(err)}`); + statuses = new Map(); } - // GraphQL answers a refused or partly failed query with 200 and errors, e.g. when rate limited. - if (!json.data?.search || (json.errors?.length ?? 0) > 0) { - throw new Error(`Searching pull requests failed: ${json.errors?.map(e => e.message).join('; ') || 'no data'}`); + return pulls.map(pull => ({ ...pull, ...statuses.get(pull.url) })); + } + + /** By pull request URL. A pull request GraphQL refuses is a null node with an error of its own, and is left out. */ + private async ownPullStatuses(token: string): Promise> { + const json = await this.json<{ data?: { search?: { nodes?: (RawOwnPull | null)[] } }; errors?: { message?: string }[] }>( + token, + '/graphql', + { method: 'POST', body: { query: OWN_PULLS_GRAPHQL, variables: { q: `${OWN_PULLS_QUERY} sort:updated-desc` } } }, + ); + if (!json.data?.search) { + throw new Error(json.errors?.map(e => e.message).join('; ') || 'no data'); } - return (json.data.search.nodes ?? []).flatMap(node => { - const owner = node?.repository?.owner?.login; - const repo = node?.repository?.name; - if (!node || !owner || !repo || node.number === undefined) { - return []; + const statuses = new Map(); + for (const node of json.data.search.nodes ?? []) { + if (!node?.url || !node.reviewThreads || !node.reviewRequests) { + continue; } - const threads = node.reviewThreads?.nodes ?? []; - const reviewers = (node.reviewRequests?.nodes ?? []) + const threads = node.reviewThreads.nodes ?? []; + const reviewers = (node.reviewRequests.nodes ?? []) .map(request => request.requestedReviewer?.login ?? request.requestedReviewer?.name) .filter((name): name is string => !!name); - return [{ - owner, - repo, - number: node.number, - title: node.title ?? '', - author: node.author?.login ?? '', - draft: !!node.isDraft, - updatedAt: node.updatedAt ?? '', - url: node.url ?? `https://github.com/${owner}/${repo}/pull/${node.number}`, + statuses.set(node.url, { openThreads: threads.filter(thread => !thread.isResolved).length, - moreThreads: (node.reviewThreads?.totalCount ?? 0) > threads.length, + moreThreads: (node.reviewThreads.totalCount ?? 0) > threads.length, requestedReviewers: reviewers, - requestedReviewerCount: node.reviewRequests?.totalCount ?? reviewers.length, + requestedReviewerCount: node.reviewRequests.totalCount ?? reviewers.length, reviewDecision: node.reviewDecision ?? null, - }]; - }); + }); + } + return statuses; } private async searchPulls(token: string, q: string): Promise { diff --git a/packages/server/tests/helpers.ts b/packages/server/tests/helpers.ts index e5884a52..e180a684 100644 --- a/packages/server/tests/helpers.ts +++ b/packages/server/tests/helpers.ts @@ -161,6 +161,8 @@ export interface FakeReviewRequest { threads?: boolean[]; /** Own pull requests only: the threads' totalCount, when more than `threads` holds. */ threadCount?: number; + /** Own pull requests only: GraphQL refuses it, as in a repository the App is not installed on. */ + forbidden?: boolean; /** Own pull requests only: requested users by login, or teams as `{ team }`. */ reviewers?: (string | { team: string })[]; reviewerCount?: number; @@ -287,7 +289,9 @@ export async function startFakeGitHub(repos: FakeRepo[]): Promise { send(200, { errors: fake.graphqlSearchErrors.map(message => ({ message })) }); return; } - const nodes = (token ? fake.ownPulls[token] ?? [] : []).map(pr => ({ + const pulls = token ? fake.ownPulls[token] ?? [] : []; + const errors = pulls.filter(pr => pr.forbidden).map(() => ({ type: 'FORBIDDEN', message: 'Resource not accessible by integration' })); + const nodes = pulls.map(pr => pr.forbidden ? null : ({ number: pr.number, title: pr.title, url: `https://github.com/${pr.owner}/${pr.repo}/pull/${pr.number}`, @@ -307,7 +311,7 @@ export async function startFakeGitHub(repos: FakeRepo[]): Promise { nodes: (pr.threads ?? []).map(isResolved => ({ isResolved, isOutdated: false })), }, })); - send(200, { data: { search: { nodes } } }); + send(200, { data: { search: { nodes } }, ...errors.length > 0 ? { errors } : {} }); return; } if (url.pathname === '/graphql' && req.method === 'POST') { diff --git a/packages/server/tests/http.test.ts b/packages/server/tests/http.test.ts index b0604500..bdcb7ffa 100644 --- a/packages/server/tests/http.test.ts +++ b/packages/server/tests/http.test.ts @@ -343,6 +343,7 @@ describe('pull requests awaiting review', () => { { owner: 'acme', repo: 'widgets', number: 18, title: 'Nobody', author: 'alice', updatedAt: '2026-10-04T09:00:00Z', reviewDecision: null }, { owner: 'acme', repo: 'widgets', number: 19, title: 'Unread threads', author: 'alice', updatedAt: '2026-10-04T09:00:00Z', threads: Array.from({ length: 100 }, () => true), threadCount: 150, reviewDecision: 'APPROVED' }, + { owner: 'other', repo: 'elsewhere', number: 20, title: 'Not installed', author: 'alice', draft: true, updatedAt: '2026-10-04T09:00:00Z', forbidden: true }, ]; await server.users.setGitHubToken(aliceId, 'alice-token-status'); try { @@ -362,6 +363,11 @@ describe('pull requests awaiting review', () => { expect(status(17)).toBe('Changes requested'); expect(status(18)).toBe('No reviewer requested'); expect(status(19)).toBe('Open threads unknown · Approved'); + const elsewhere = own.slice(own.indexOf('other/elsewhere#20<')); + expect(elsewhere.slice(0, elsewhere.indexOf(''))).toContain( + 'Draft Status unknown', + ); + expect(page).not.toContain('GitHub could not be asked'); expect(own).not.toContain(''); expect(own).toContain('Threads'); } finally { diff --git a/packages/server/tests/service.test.ts b/packages/server/tests/service.test.ts index b05c9431..7bec2457 100644 --- a/packages/server/tests/service.test.ts +++ b/packages/server/tests/service.test.ts @@ -288,10 +288,11 @@ describe('review requests', () => { } }); - it('asks GitHub’s GraphQL search for the token’s own pull requests, with threads and requested reviewers', async () => { + it('lists the token’s own pull requests, with threads and requested reviewers where GraphQL shows them', async () => { github.ownPulls['own-search-token'] = [ { owner: 'acme', repo: 'widgets', number: 8, title: 'Eight', author: 'erin', draft: true, updatedAt: '2026-10-02T08:30:00Z', threads: [false, true], threadCount: 120, reviewers: ['kirill', { team: 'platform' }], reviewerCount: 2, reviewDecision: 'REVIEW_REQUIRED' }, + { owner: 'other', repo: 'elsewhere', number: 9, title: 'Nine', author: 'erin', updatedAt: '2026-10-02T08:00:00Z', forbidden: true }, ]; const api = new GitHubApi(github.url); expect(await api.ownPulls('own-search-token')).toEqual([{ @@ -299,8 +300,14 @@ describe('review requests', () => { updatedAt: '2026-10-02T08:30:00Z', url: 'https://github.com/acme/widgets/pull/8', openThreads: 1, moreThreads: true, requestedReviewers: ['kirill', 'platform'], requestedReviewerCount: 2, reviewDecision: 'REVIEW_REQUIRED', + }, { + owner: 'other', repo: 'elsewhere', number: 9, title: 'Nine', author: 'erin', draft: false, + updatedAt: '2026-10-02T08:00:00Z', url: 'https://github.com/other/elsewhere/pull/9', }]); - expect(github.searches.at(-1)).toBe('is:pr is:open archived:false author:@me sort:updated-desc'); + expect(github.searches.slice(-2)).toEqual([ + 'is:pr is:open archived:false author:@me', + 'is:pr is:open archived:false author:@me sort:updated-desc', + ]); try { github.failSearchWith = 403; @@ -311,7 +318,7 @@ describe('review requests', () => { await expect(api.ownPulls('own-search-token')).rejects.toThrow('GitHub answered 500'); github.failSearchWith = null; github.graphqlSearchErrors = ['API rate limit exceeded']; - await expect(api.ownPulls('own-search-token')).rejects.toThrow('Searching pull requests failed: API rate limit exceeded'); + expect((await api.ownPulls('own-search-token')).map(pull => pull.requestedReviewers)).toEqual([undefined, undefined]); } finally { github.failSearchWith = null; github.graphqlSearchErrors = null; @@ -367,7 +374,7 @@ describe('review requests', () => { expect(own?.fetchedAt).toBe(now + 30_000); expect(own?.requests.map(r => r.number)).toEqual([4]); expect((await service.ownPulls(bob, { now: now + 40_000, fresh: true }))?.fetchedAt).toBe(now + 40_000); - expect(github.searches.length).toBe(searches + 4); + expect(github.searches.length).toBe(searches + 6); }); it('never links another user’s session, and answers null without GitHub access', async () => {