From 6d342a0039feed3040ef2166088b16e82e36c16a Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 21:20:17 -0700 Subject: [PATCH] Reuse issue metadata from existing detail conversation queries (#35546) * Reuse GraphQL detail reads for work-item metadata * Keep PR metadata aligned with cached file listings * Recover incomplete issue assignee metadata without reloading conversation --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- src/main/github/client/fetch/get-work-item.ts | 26 ++- .../client/list/work-item-issue-page.ts | 24 +- src/main/github/client/map/issue-actor.ts | 20 ++ src/main/github/issue-work-item-details.ts | 71 +++++- .../work-item-details-config-change.test.ts | 4 +- .../work-item-details-query-reuse.test.ts | 218 ++++++++++++++++++ .../github/work-item-details-test-lookup.ts | 3 +- src/main/github/work-item-details.ts | 29 ++- 8 files changed, 347 insertions(+), 48 deletions(-) create mode 100644 src/main/github/client/map/issue-actor.ts create mode 100644 src/main/github/work-item-details-query-reuse.test.ts diff --git a/src/main/github/client/fetch/get-work-item.ts b/src/main/github/client/fetch/get-work-item.ts index cf4583e33c5..3be89be3695 100644 --- a/src/main/github/client/fetch/get-work-item.ts +++ b/src/main/github/client/fetch/get-work-item.ts @@ -8,6 +8,10 @@ import { } from '../../github-api-repository' import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key' import type { MainWorkItem } from './../map/work-item-field-coercion' +import { + getIssueDetailsViaGraphQL, + type CollapsedIssueDetails +} from '../../issue-work-item-details' import { fetchIssueWorkItem, fetchPullRequestWorkItem, @@ -35,6 +39,7 @@ export async function getWorkItem( export type WorkItemRepositoryLookup = { item: MainWorkItem | null repository: GitHubApiRepository | null + issueDetails?: CollapsedIssueDetails | null } export async function getWorkItemWithRepository( @@ -43,7 +48,8 @@ export async function getWorkItemWithRepository( type?: 'issue' | 'pr', connectionId?: string | null, localGitOptions: LocalGitExecOptions = {}, - preference?: IssueSourcePreference + preference?: IssueSourcePreference, + includeIssueDetails = false ): Promise { await acquire() try { @@ -59,9 +65,23 @@ export async function getWorkItemWithRepository( if (!source && preference === 'origin') { return { item: null, repository: null } } + const issueDetails = + includeIssueDetails && source + ? await getIssueDetailsViaGraphQL( + repoPath, + number, + source, + connectionId, + localGitOptions, + true + ) + : undefined return { - item: await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions), - repository: source + item: + issueDetails?.item ?? + (await fetchIssueWorkItem(repoPath, source, number, connectionId, localGitOptions)), + repository: source, + ...(includeIssueDetails ? { issueDetails } : {}) } } if (type === 'pr') { diff --git a/src/main/github/client/list/work-item-issue-page.ts b/src/main/github/client/list/work-item-issue-page.ts index ac1273887d2..de7ca4bccde 100644 --- a/src/main/github/client/list/work-item-issue-page.ts +++ b/src/main/github/client/list/work-item-issue-page.ts @@ -5,6 +5,7 @@ import { noteRepositoryRateLimitSpend } from '../../rate-limit' import type { GitHubRepoExecOptions } from '../../github-api-repository' import { fetchIssueWorkItem } from '../fetch/work-item-fetch' import { mapIssueWorkItem } from '../map/work-item' +import { restIssueActor } from '../map/issue-actor' import type { MainWorkItem } from '../map/work-item-field-coercion' import { buildWorkItemListRequest } from './work-item-list-request' import { buildSearchQueryString } from './work-item-search-query' @@ -29,25 +30,6 @@ const ISSUE_NODE_SELECTION = `__typename ... on Issue { assignees(first: 100) { nodes { __typename login avatarUrl } pageInfo { hasNextPage } } }` -function restActor(actor: Actor | null): Record | null { - if (!actor) { - return null - } - const login = - actor.__typename === 'Bot' && !actor.login.endsWith('[bot]') - ? `${actor.login}[bot]` - : actor.login - let avatar = actor.avatarUrl - if (avatar) { - const url = new URL(avatar) - if (url.hostname === 'avatars.githubusercontent.com') { - url.searchParams.delete('u') - avatar = url.toString() - } - } - return { login, avatar_url: avatar } -} - export async function listIssueWorkItemPage(args: { repoPath: string ownerRepo: OwnerRepo @@ -110,9 +92,9 @@ export async function listIssueWorkItemPage(args: { mapIssueWorkItem({ ...node, state: node.state.toLowerCase(), - user: restActor(node.author), + user: restIssueActor(node.author), labels: node.labels.nodes, - assignees: node.assignees.nodes.map(restActor) + assignees: node.assignees.nodes.map(restIssueActor) }) ) } diff --git a/src/main/github/client/map/issue-actor.ts b/src/main/github/client/map/issue-actor.ts new file mode 100644 index 00000000000..fae9f64d394 --- /dev/null +++ b/src/main/github/client/map/issue-actor.ts @@ -0,0 +1,20 @@ +type Actor = { __typename?: string; login: string; avatarUrl?: string } + +export function restIssueActor(actor: Actor | null): Record | null { + if (!actor) { + return null + } + const login = + actor.__typename === 'Bot' && !actor.login.endsWith('[bot]') + ? `${actor.login}[bot]` + : actor.login + let avatar = actor.avatarUrl + if (avatar) { + const url = new URL(avatar) + if (url.hostname === 'avatars.githubusercontent.com') { + url.searchParams.delete('u') + avatar = url.toString() + } + } + return { login, avatar_url: avatar } +} diff --git a/src/main/github/issue-work-item-details.ts b/src/main/github/issue-work-item-details.ts index 5cb95f50d48..9a3e538a356 100644 --- a/src/main/github/issue-work-item-details.ts +++ b/src/main/github/issue-work-item-details.ts @@ -1,16 +1,32 @@ import type { GitHubIssueTimelineItem, PRComment } from '../../shared/github/comment-types' import type { GitHubAssignableUser } from '../../shared/github/pull-request-types' +import { z } from 'zod' +import { mapIssueWorkItem } from './client/map/work-item' +import { restIssueActor } from './client/map/issue-actor' +import type { MainWorkItem } from './client/map/work-item-field-coercion' import { ghExecFileAsync, ghRepoExecOptions, githubRepoContext } from './gh-utils' import type { LocalGitExecOptions } from './gh-utils' import { githubHostExecOptions, type GitHubApiRepository } from './github-api-repository' import { getIssueTimelineItems } from './issue-timeline' import { noteRepositoryRateLimitSpend, repositoryRateLimitGuard } from './rate-limit' -const ISSUE_DETAILS_QUERY = `query($owner: String!, $repo: String!, $number: Int!) { +const ISSUE_DETAILS_QUERY = `query($owner: String!, $repo: String!, $number: Int!, $includeItem: Boolean!) { repository(owner: $owner, name: $repo) { issue(number: $number) { + number @include(if: $includeItem) title @include(if: $includeItem) + state @include(if: $includeItem) + url @include(if: $includeItem) + updatedAt @include(if: $includeItem) + author @include(if: $includeItem) { __typename login avatarUrl(size: 48) } + labels(first: 100) @include(if: $includeItem) { + nodes { name } + pageInfo { hasNextPage } + } body - assignees(first: 50) { nodes { login avatarUrl(size: 48) ... on User { name } } } + assignees(first: 50) { + nodes { login avatarUrl(size: 48) ... on User { name } } + pageInfo { hasNextPage } + } participants(first: 100) { nodes { login avatarUrl(size: 48) ... on User { name } } } @@ -31,12 +47,29 @@ const ISSUE_DETAILS_QUERY = `query($owner: String!, $repo: String!, $number: Int } }` +const issueItemSchema = z.object({ + number: z.number().int().positive(), + title: z.string(), + state: z.enum(['OPEN', 'CLOSED']), + url: z.string(), + updatedAt: z.string(), + author: z.object({ __typename: z.string(), login: z.string(), avatarUrl: z.string() }).nullable(), + assignees: z.object({ + nodes: z.array(z.object({ login: z.string().min(1) })), + pageInfo: z.object({ hasNextPage: z.literal(false) }) + }), + labels: z.object({ + nodes: z.array(z.object({ name: z.string() })), + pageInfo: z.object({ hasNextPage: z.literal(false) }) + }) +}) + type GraphQLIssueDetailsResponse = { data?: { repository?: { issue?: { body?: string | null - assignees?: { nodes?: { login?: string; avatarUrl?: string; name?: string | null }[] } + assignees?: { nodes?: (Partial | null)[] | null } participants?: { nodes?: GitHubAssignableUser[] } comments?: { nodes?: { @@ -58,6 +91,7 @@ type GraphQLIssueDetailsResponse = { } export type CollapsedIssueDetails = { + item?: MainWorkItem body: string comments: PRComment[] assignees: string[] @@ -71,7 +105,8 @@ export async function getIssueDetailsViaGraphQL( issueNumber: number, repository: GitHubApiRepository | null, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + includeItem = false ): Promise { const ghOptions = { ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), @@ -96,7 +131,9 @@ export async function getIssueDetailsViaGraphQL( '-f', `repo=${repository.repo}`, '-F', - `number=${issueNumber}` + `number=${issueNumber}`, + '-F', + `includeItem=${includeItem}` ], ghOptions ) @@ -108,6 +145,10 @@ export async function getIssueDetailsViaGraphQL( if (!issue) { return null } + const itemData = includeItem ? issueItemSchema.safeParse(issue) : undefined + if (itemData?.success && itemData.data.number !== issueNumber) { + return null + } const comments: PRComment[] = (issue.comments?.nodes ?? []) .filter((comment) => typeof comment.databaseId === 'number') .map((comment) => ({ @@ -121,7 +162,7 @@ export async function getIssueDetailsViaGraphQL( })) const assigneeUsers: GitHubAssignableUser[] = (issue.assignees?.nodes ?? []) .filter((assignee): assignee is { login: string; avatarUrl?: string; name?: string | null } => - Boolean(assignee.login) + Boolean(assignee?.login) ) .map((assignee) => ({ login: assignee.login, @@ -136,6 +177,17 @@ export async function getIssueDetailsViaGraphQL( avatarUrl: user.avatarUrl ?? '' })) return { + ...(itemData?.success + ? { + item: mapIssueWorkItem({ + ...itemData.data, + state: itemData.data.state.toLowerCase(), + user: restIssueActor(itemData.data.author), + labels: itemData.data.labels.nodes, + assignees: assigneeUsers + }) + } + : {}), body: issue.body ?? '', comments, assignees: assigneeUsers.map((assignee) => assignee.login), @@ -154,12 +206,7 @@ export async function getIssueBodyAndComments( repository: GitHubApiRepository | null, connectionId?: string | null, localGitOptions: LocalGitExecOptions = {} -): Promise<{ - body: string - comments: PRComment[] - assignees: string[] - timelineItems: GitHubIssueTimelineItem[] -}> { +): Promise> { const ghOptions = { ...ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)), ...githubHostExecOptions(repository) diff --git a/src/main/github/work-item-details-config-change.test.ts b/src/main/github/work-item-details-config-change.test.ts index 0b8ffa19b8c..5143b39113d 100644 --- a/src/main/github/work-item-details-config-change.test.ts +++ b/src/main/github/work-item-details-config-change.test.ts @@ -161,7 +161,9 @@ it.each(['issue', undefined] as const)( expect(details?.body).toBe('ORIGIN body') expect(details?.assignees).toEqual(['ORIGIN-assignee']) expect(details?.comments.map((comment) => comment.body)).toEqual(['ORIGIN comment']) - expect(fixture.requests[1].args).toContain('owner=fork-owner') + expect( + fixture.requests.filter(({ args }) => args.includes('graphql')).map(({ args }) => args) + ).toEqual([expect.arrayContaining(['owner=fork-owner'])]) } ) diff --git a/src/main/github/work-item-details-query-reuse.test.ts b/src/main/github/work-item-details-query-reuse.test.ts new file mode 100644 index 00000000000..49dd70b776d --- /dev/null +++ b/src/main/github/work-item-details-query-reuse.test.ts @@ -0,0 +1,218 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import type { GitHubApiRepository } from './github-api-repository' +import type { LocalGitExecOptions } from './gh-utils' + +const mocks = vi.hoisted(() => ({ + exec: vi.fn(), + source: vi.fn(), + acquire: vi.fn(), + release: vi.fn() +})) +vi.mock('./gh-utils', () => ({ + ghExecFileAsync: mocks.exec, + acquire: mocks.acquire, + release: mocks.release, + ghRepoExecOptions: (context: { repoPath: string; connectionId?: string; wslDistro?: string }) => + context.connectionId + ? {} + : { cwd: context.repoPath, ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) }, + githubRepoContext: (repoPath: string, connectionId?: string, options = {}) => ({ + repoPath, + connectionId, + ...options + }), + classifyGhError: () => ({ type: 'not_found' }) +})) +vi.mock('./github-api-repository', () => ({ + resolveIssueGitHubApiRepositorySource: mocks.source, + githubHostExecOptions: (repository?: GitHubApiRepository) => ({ host: repository?.host }) +})) +vi.mock('./client', async () => ({ + ...(await import('./client/fetch/get-work-item')), + getPRChecks: vi.fn(), + getPRComments: vi.fn() +})) +vi.mock('./rate-limit', () => ({ + repositoryRateLimitGuard: () => ({ blocked: false }), + noteRepositoryRateLimitSpend: vi.fn() +})) + +import { getWorkItemDetails } from './work-item-details' +import { getWorkItemWithRepository } from './client/fetch/get-work-item' + +const repository = { owner: 'team', repo: 'widgets', host: 'github.example.com:8443' } +let issue: Record +beforeEach(() => { + vi.clearAllMocks() + mocks.source.mockResolvedValue({ source: repository, fellBack: false }) + issue = { + number: 5, + title: 'Closed issue', + state: 'CLOSED', + url: 'https://github.example.com:8443/team/widgets/issues/5', + updatedAt: '2026-10-01T00:00:00Z', + author: { __typename: 'User', login: 'author', avatarUrl: 'https://avatars.example/author' }, + labels: { nodes: [{ name: 'bug' }, { name: 'urgent' }], pageInfo: { hasNextPage: false } }, + body: 'Description', + assignees: { + pageInfo: { hasNextPage: false }, + nodes: [ + { login: 'assigned', name: 'Assigned User', avatarUrl: 'https://avatars.example/assigned' } + ] + }, + participants: { nodes: [] }, + comments: { + nodes: [ + { + databaseId: 10, + body: 'Comment', + createdAt: '2026-10-02T00:00:00Z', + url: 'https://example/comment', + author: { login: 'bot', __typename: 'Bot' } + } + ] + } + } + mocks.exec.mockImplementation(async (args: string[]) => ({ + stdout: JSON.stringify( + args.includes('graphql') + ? { data: { repository: { issue } } } + : args.some((arg) => arg.includes('/timeline?')) + ? [] + : { + number: 5, + title: 'REST issue', + state: 'open', + labels: [{ name: 'all labels' }], + assignees: [{ login: 'rest-assigned', name: 'REST Assigned User' }] + } + ) + })) +}) + +it.each<{ connectionId?: string; options: LocalGitExecOptions; execution: object }>([ + { options: {}, execution: { cwd: '/repo', host: repository.host } }, + { + options: { wslDistro: 'Ubuntu' }, + execution: { cwd: '/repo', wslDistro: 'Ubuntu', host: repository.host } + }, + { connectionId: 'ssh-1', options: {}, execution: { host: repository.host } } +])( + 'loads issue metadata and conversation with one GraphQL read: $execution', + async ({ connectionId, options, execution }) => { + const details = await getWorkItemDetails('/repo', 5, 'issue', connectionId, options, 'origin') + expect(details?.item).toMatchObject({ + title: 'Closed issue', + state: 'closed', + labels: ['bug', 'urgent'], + author: 'author', + assignees: [{ login: 'assigned', name: 'Assigned User' }] + }) + expect(details).toMatchObject({ + body: 'Description', + comments: [{ id: 10, isBot: true }], + assignees: ['assigned'], + timelineItems: [] + }) + expect(mocks.source).toHaveBeenCalledWith('/repo', 'origin', connectionId, options) + expect(mocks.exec).toHaveBeenCalledTimes(2) + expect(mocks.exec.mock.calls[0][0]).toContain('includeItem=true') + expect(mocks.exec.mock.calls[1][0]).toContain( + 'repos/team/widgets/issues/5/timeline?per_page=100&page=1' + ) + expect( + mocks.exec.mock.calls.every( + ([, options]) => JSON.stringify(options) === JSON.stringify(execution) + ) + ).toBe(true) + expect(mocks.release).toHaveBeenCalledTimes(mocks.acquire.mock.calls.length) + } +) + +it.each(['incomplete labels', 'invalid metadata', 'partial errors'])( + 'retains the REST item fallback for %s', + async (reason) => { + if (reason === 'incomplete labels') { + issue.labels = { nodes: [{ name: 'partial' }], pageInfo: { hasNextPage: true } } + } + if (reason === 'invalid metadata') { + issue.number = 6 + } + if (reason === 'partial errors') { + mocks.exec.mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { repository: { issue } }, + errors: [{ message: 'Unavailable metadata' }] + }) + }) + } + const result = await getWorkItemWithRepository( + '/repo', + 5, + 'issue', + undefined, + {}, + 'origin', + true + ) + expect(result.item).toMatchObject({ title: 'REST issue', labels: ['all labels'] }) + if (reason === 'incomplete labels') { + expect(result.issueDetails).toMatchObject({ body: 'Description', timelineItems: [] }) + expect(mocks.exec).toHaveBeenCalledTimes(3) + } else { + expect(result.issueDetails).toBeNull() + expect(mocks.exec).toHaveBeenCalledTimes(2) + } + expect(mocks.exec.mock.calls.at(-1)?.[0]).toEqual(['api', 'repos/team/widgets/issues/5']) + } +) + +it('keeps ordinary metadata lookups on their existing REST path', async () => { + const result = await getWorkItemWithRepository('/repo', 5, 'issue') + expect(result.item?.title).toBe('REST issue') + expect(mocks.exec).toHaveBeenCalledTimes(1) + expect(mocks.exec.mock.calls[0][0]).toEqual(['api', 'repos/team/widgets/issues/5']) +}) + +it.each([ + { reason: 'missing nodes', nodes: undefined }, + { reason: 'null nodes', nodes: null }, + { reason: 'null entry', nodes: [null] }, + { reason: 'missing login', nodes: [{}] }, + { reason: 'empty login', nodes: [{ login: '' }] } +])('retains conversation and recovers issue assignees for $reason', async ({ nodes }) => { + issue.assignees = { nodes, pageInfo: { hasNextPage: false } } + const result = await getWorkItemWithRepository('/repo', 5, 'issue', undefined, {}, 'origin', true) + expect(result.item).toMatchObject({ + title: 'REST issue', + assignees: [{ login: 'rest-assigned', name: 'REST Assigned User' }] + }) + expect(result.issueDetails).toMatchObject({ + body: 'Description', + comments: [{ id: 10, isBot: true }], + timelineItems: [] + }) + expect(mocks.exec.mock.calls.map(([args]) => args)).toEqual([ + expect.arrayContaining(['api', 'graphql', 'includeItem=true']), + expect.arrayContaining(['api', 'repos/team/widgets/issues/5/timeline?per_page=100&page=1']), + ['api', 'repos/team/widgets/issues/5'] + ]) +}) + +it('does not query an unavailable explicitly selected source', async () => { + mocks.source.mockResolvedValue({ source: null, fellBack: false }) + expect( + (await getWorkItemWithRepository('/repo', 5, 'issue', undefined, {}, 'origin', true)).item + ).toBeNull() + expect(mocks.exec).not.toHaveBeenCalled() +}) + +it('retains REST bot login normalization for issue metadata', async () => { + issue.author = { + __typename: 'Bot', + login: 'automation', + avatarUrl: 'https://avatars.example/bot' + } + const result = await getWorkItemWithRepository('/repo', 5, 'issue', undefined, {}, 'origin', true) + expect(result.item?.author).toBe('automation[bot]') +}) diff --git a/src/main/github/work-item-details-test-lookup.ts b/src/main/github/work-item-details-test-lookup.ts index 69c1daf0777..c2532854c35 100644 --- a/src/main/github/work-item-details-test-lookup.ts +++ b/src/main/github/work-item-details-test-lookup.ts @@ -5,7 +5,8 @@ import type { WorkItemRepositoryLookup } from './client/fetch/get-work-item' // Why: detail tests replace item lookup; retain its selected-source metadata. export function makeWorkItemDetailsLookupMock(lookup: typeof getWorkItem) { return async (...args: Parameters): Promise => { - const item = await lookup(...args) + const [repoPath, number, type, connectionId, localGitOptions, preference] = args + const item = await lookup(repoPath, number, type, connectionId, localGitOptions, preference) return { item, repository: diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 09d55e59f5d..147db244d54 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -82,7 +82,11 @@ export async function getWorkItemDetails( return null } } - const { item, repository: selectedRepository } = issueRepositoryOverride + const { + item, + repository: selectedRepository, + issueDetails + } = issueRepositoryOverride ? { item: await getWorkItemByOwnerRepo( repoPath, @@ -92,7 +96,8 @@ export async function getWorkItemDetails( connectionId, localGitOptions ), - repository: issueRepositoryOverride + repository: issueRepositoryOverride, + issueDetails: undefined } : await getWorkItemWithRepository( repoPath, @@ -100,7 +105,8 @@ export async function getWorkItemDetails( type, connectionId, localGitOptions, - preference + preference, + type === 'issue' ) if (!item) { return null @@ -115,13 +121,16 @@ export async function getWorkItemDetails( if (item.type === 'issue') { return withWorkItemDetailsPermit(async () => { - const collapsed = await getIssueDetailsViaGraphQL( - repoPath, - item.number, - resolvedRepository, - connectionId, - localGitOptions - ) + const collapsed = + issueDetails === undefined + ? await getIssueDetailsViaGraphQL( + repoPath, + item.number, + resolvedRepository, + connectionId, + localGitOptions + ) + : issueDetails if (collapsed) { return { item: enrichItemDisplayAvatars(item, [