From 30d1e8dba5feebcd1078aa7701f8afd86133a844 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 16:12:08 -0700 Subject: [PATCH] perf(gitlab): skip unused review details in Checks refreshes (#28643) --- src/main/gitlab/work-item-details.test.ts | 133 +++++++++++++++++- src/main/gitlab/work-item-details.ts | 30 ++-- src/main/ipc/gitlab-work-item-handlers.ts | 12 +- src/main/ipc/gitlab.test.ts | 19 ++- .../gitlab-and-pr-bases.spec.ts | 29 +++- src/main/runtime/rpc/methods/gitlab.test.ts | 34 ++++- src/main/runtime/rpc/methods/gitlab.ts | 8 +- .../runtime/runtime-gitlab-query-commands.ts | 6 +- src/preload/api/gitlab-api.ts | 1 + src/preload/gitlab-work-item-details.test.ts | 18 +++ src/preload/gitlab.ts | 1 + .../checks-panel/gitlab-review-client.test.ts | 10 +- .../checks-panel/gitlab-review-client.ts | 3 +- src/shared/rpc-contract/gitlab-params.ts | 3 +- 14 files changed, 281 insertions(+), 26 deletions(-) create mode 100644 src/preload/gitlab-work-item-details.test.ts diff --git a/src/main/gitlab/work-item-details.test.ts b/src/main/gitlab/work-item-details.test.ts index 86f326b59cd..1b26decc7f7 100644 --- a/src/main/gitlab/work-item-details.test.ts +++ b/src/main/gitlab/work-item-details.test.ts @@ -107,13 +107,23 @@ describe('getWorkItemDetails', () => { return { stdout: '[]' } } if (endpoint === 'projects/g%2Fp/merge_requests/12/reviewers') { - return { stdout: '[]' } + return { stdout: JSON.stringify([{ user: { id: 1, username: 'alice', name: 'Alice' } }]) } } if (endpoint === 'projects/g%2Fp/merge_requests/12/approvals') { - return { stdout: JSON.stringify({ approvals_required: 0, approvals_left: 0 }) } + return { + stdout: JSON.stringify({ + approvals_required: 1, + approvals_left: 0, + approved_by: [{ user: { id: 1, username: 'alice', name: 'Alice' } }] + }) + } } if (endpoint === 'projects/g%2Fp/merge_requests/12/approval_state') { - return { stdout: JSON.stringify({ rules: [] }) } + return { + stdout: JSON.stringify({ + rules: [{ id: 5, name: 'Maintainer', approvals_required: 1, approved: true }] + }) + } } if (endpoint === 'projects/g%2Fp/merge_requests/12/diffs?per_page=100') { return { @@ -139,11 +149,56 @@ describe('getWorkItemDetails', () => { additions: 1, deletions: 1 }) + expect(details?.reviewers).toMatchObject([{ id: 1, username: 'alice' }]) + expect(details?.approvalState).toMatchObject({ + approvalsRequired: 1, + approvalsLeft: 0, + approvedBy: [{ id: 1, username: 'alice' }], + rules: [{ id: 5, approved: true }] + }) + expect(glabExecFileAsyncMock.mock.calls).toHaveLength(8) expect(glabExecFileAsyncMock.mock.calls.map(([args]) => args)).toContainEqual([ 'api', 'projects/g%2Fp/merge_requests/12/diffs?per_page=100' ]) expect(glabExecFileAsyncMock.mock.calls.flatMap(([args]) => args)).not.toContain('--paginate') + + glabExecFileAsyncMock.mockClear() + const explicitFull = await getWorkItemDetails( + '/repo', + 12, + 'mr', + undefined, + undefined, + undefined, + {}, + { + includeReviewDetails: true + } + ) + expect(explicitFull).toEqual(details) + expect(glabExecFileAsyncMock.mock.calls).toHaveLength(8) + + glabExecFileAsyncMock.mockClear() + const checks = await getWorkItemDetails( + '/repo', + 12, + 'mr', + undefined, + undefined, + undefined, + {}, + { + includeReviewDetails: false + } + ) + expect(checks).toEqual({ ...details, files: [], reviewers: [], approvalState: undefined }) + expect(glabExecFileAsyncMock.mock.calls.map(([args]) => args.at(-1)).sort()).toEqual([ + 'projects/g%2Fp/merge_requests/12', + 'projects/g%2Fp/merge_requests/12/discussions?per_page=100', + 'projects/g%2Fp/pipelines/99/bridges?per_page=100', + 'projects/g%2Fp/pipelines/99/jobs?per_page=100' + ]) }) it('expands bridge child-pipeline jobs into the checks list', async () => { @@ -251,6 +306,78 @@ describe('getWorkItemDetails', () => { status: 'failed', pipelineId: 99 }) + + glabExecFileAsyncMock.mockClear() + const narrow = await getWorkItemDetails( + '/repo', + 12, + 'mr', + undefined, + undefined, + undefined, + {}, + { + includeReviewDetails: false + } + ) + expect(narrow).toEqual({ ...details, approvalState: undefined }) + expect(glabExecFileAsyncMock.mock.calls.map(([args]) => args.at(-1)).sort()).toEqual([ + 'projects/g%2Fp/merge_requests/12', + 'projects/g%2Fp/merge_requests/12/discussions?per_page=100', + 'projects/g%2Fp/pipelines/200/jobs?per_page=100', + 'projects/g%2Fp/pipelines/99/bridges?per_page=100', + 'projects/g%2Fp/pipelines/99/jobs?per_page=100' + ]) + }) + + it('preserves issue details when review enrichment is disabled', async () => { + glabExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args.at(-1) === 'projects/g%2Fp/issues/12') { + return { + stdout: JSON.stringify({ + iid: 12, + title: 'Issue', + state: 'opened', + description: 'Issue body', + assignees: [{ username: 'alice' }] + }) + } + } + if (args.at(-1) === 'projects/g%2Fp/issues/12/discussions?per_page=100') { + return { + stdout: JSON.stringify([ + { + id: 'issue-thread', + notes: [{ id: 7, body: 'Issue note', author: { username: 'alice' } }] + } + ]) + } + } + throw new Error(`unexpected issue detail endpoint: ${args.at(-1)}`) + }) + const full = await getWorkItemDetails('/repo', 12, 'issue') + const fullCalls = glabExecFileAsyncMock.mock.calls.map(([args]) => args) + expect(full).toMatchObject({ + body: 'Issue body', + assignees: ['alice'], + comments: [{ body: 'Issue note' }] + }) + glabExecFileAsyncMock.mockClear() + const narrow = await getWorkItemDetails( + '/repo', + 12, + 'issue', + undefined, + undefined, + undefined, + {}, + { + includeReviewDetails: false + } + ) + expect(narrow).toEqual(full) + expect(glabExecFileAsyncMock.mock.calls.map(([args]) => args)).toEqual(fullCalls) + expect(fullCalls).toHaveLength(2) }) // Why: each fetch spawns a `glab` binary (a remote exec over SSH) and this runs on the Checks diff --git a/src/main/gitlab/work-item-details.ts b/src/main/gitlab/work-item-details.ts index 637baa0c78b..025e6ca1949 100644 --- a/src/main/gitlab/work-item-details.ts +++ b/src/main/gitlab/work-item-details.ts @@ -57,7 +57,8 @@ export async function getWorkItemDetails( preference?: IssueSourcePreference, connectionId?: string | null, projectRefOverride?: ProjectRef | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + options: { includeReviewDetails?: boolean } = {} ): Promise { // Why: detail fetches must use the same project source as the list row // that opened them, otherwise forked repos can show a row from one remote @@ -81,7 +82,7 @@ export async function getWorkItemDetails( if (type === 'issue') { return await fetchIssueDetails(repoPath, projectRef, iid, connectionId, localGitOptions) } - return await fetchMRDetails(repoPath, projectRef, iid, connectionId, localGitOptions) + return await fetchMRDetails(repoPath, projectRef, iid, connectionId, localGitOptions, options) } catch { return null } finally { @@ -133,7 +134,8 @@ async function fetchMRDetails( projectRef: ProjectRef, iid: number, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + options: { includeReviewDetails?: boolean } = {} ): Promise { // Why: MR detail + discussions in parallel. The pipeline jobs fetch // depends on `head_pipeline.id` from the MR payload, so it has to @@ -166,14 +168,22 @@ async function fetchMRDetails( localGitOptions ).catch(() => []) : undefined + // Checks consumes the pipeline and discussions without the dialog's review enrichment. + const includeReviewDetails = options.includeReviewDetails !== false const [reviewers, approvalState, files] = await Promise.all([ - fetchMRReviewers(repoPath, projectRef, iid, connectionId, localGitOptions).catch(() => - (mrRaw.reviewers ?? []).map(mapGitLabUser).filter((u): u is GitLabAssignableUser => !!u) - ), - fetchMRApprovalState(repoPath, projectRef, iid, connectionId, localGitOptions).catch( - () => undefined - ), - fetchMRFiles(repoPath, projectRef, iid, connectionId, localGitOptions).catch(() => []) + includeReviewDetails + ? fetchMRReviewers(repoPath, projectRef, iid, connectionId, localGitOptions).catch(() => + (mrRaw.reviewers ?? []).map(mapGitLabUser).filter((u): u is GitLabAssignableUser => !!u) + ) + : [], + includeReviewDetails + ? fetchMRApprovalState(repoPath, projectRef, iid, connectionId, localGitOptions).catch( + () => undefined + ) + : undefined, + includeReviewDetails + ? fetchMRFiles(repoPath, projectRef, iid, connectionId, localGitOptions).catch(() => []) + : [] ]) return { item, diff --git a/src/main/ipc/gitlab-work-item-handlers.ts b/src/main/ipc/gitlab-work-item-handlers.ts index 4f98680d3f1..5ce25367d0a 100644 --- a/src/main/ipc/gitlab-work-item-handlers.ts +++ b/src/main/ipc/gitlab-work-item-handlers.ts @@ -49,7 +49,14 @@ export function registerGitLabWorkItemHandlers(store: Store): void { // Powers GitLabItemDialog's tabs. ipcMain.handle( 'gitlab:workItemDetails', - async (_event, args: GitLabRepoSelectorArgs & { iid: number; type: 'issue' | 'mr' }) => { + async ( + _event, + args: GitLabRepoSelectorArgs & { + iid: number + type: 'issue' | 'mr' + includeReviewDetails?: boolean + } + ) => { const repo = assertRegisteredRepo(args, store) return getWorkItemDetails( repo.path, @@ -58,7 +65,8 @@ export function registerGitLabWorkItemHandlers(store: Store): void { repo.issueSourcePreference, repoConnectionId(repo), undefined, - ...localGitOptionArgs(store, repo) + localGitOptionArgs(store, repo)[0] ?? {}, + { includeReviewDetails: args.includeReviewDetails } ) } ) diff --git a/src/main/ipc/gitlab.test.ts b/src/main/ipc/gitlab.test.ts index d4bd1235aad..7ab7f77e6e4 100644 --- a/src/main/ipc/gitlab.test.ts +++ b/src/main/ipc/gitlab.test.ts @@ -580,7 +580,24 @@ describe('GitLab IPC handlers', () => { undefined, null, undefined, - localGitOptions + localGitOptions, + { includeReviewDetails: undefined } + ) + await ipcHandlers.get('gitlab:workItemDetails')?.(null, { + repoPath: '/local/orca', + iid: 8, + type: 'mr', + includeReviewDetails: false + }) + expect(getWorkItemDetailsMock).toHaveBeenLastCalledWith( + '/local/orca', + 8, + 'mr', + undefined, + null, + undefined, + localGitOptions, + { includeReviewDetails: false } ) expect(closeMRMock).toHaveBeenCalledWith( '/local/orca', diff --git a/src/main/runtime/orca-runtime-tests/gitlab-and-pr-bases.spec.ts b/src/main/runtime/orca-runtime-tests/gitlab-and-pr-bases.spec.ts index 6033c00b815..adc2c0f3f87 100644 --- a/src/main/runtime/orca-runtime-tests/gitlab-and-pr-bases.spec.ts +++ b/src/main/runtime/orca-runtime-tests/gitlab-and-pr-bases.spec.ts @@ -244,7 +244,20 @@ describe('OrcaRuntimeService', () => { 'mr', 'origin', 'ssh-1', - undefined + undefined, + {}, + { includeReviewDetails: undefined } + ) + await runtime.getGitLabRepoWorkItemDetails(TEST_REPO_ID, 8, 'mr', undefined, false) + expect(getGitLabWorkItemDetailsMock).toHaveBeenLastCalledWith( + '/remote/repo', + 8, + 'mr', + 'origin', + 'ssh-1', + undefined, + {}, + { includeReviewDetails: false } ) expect(updateGitLabMRReviewersMock).toHaveBeenCalledWith( '/remote/repo', @@ -440,7 +453,19 @@ describe('OrcaRuntimeService', () => { undefined, null, undefined, - localGitOptions + localGitOptions, + { includeReviewDetails: undefined } + ) + await runtime.getGitLabRepoWorkItemDetails(TEST_REPO_ID, 8, 'mr', undefined, false) + expect(getGitLabWorkItemDetailsMock).toHaveBeenLastCalledWith( + TEST_REPO_PATH, + 8, + 'mr', + undefined, + null, + undefined, + localGitOptions, + { includeReviewDetails: false } ) expect(updateGitLabMRMock).toHaveBeenCalledWith( TEST_REPO_PATH, diff --git a/src/main/runtime/rpc/methods/gitlab.test.ts b/src/main/runtime/rpc/methods/gitlab.test.ts index f5e00136152..5111a5c3ce9 100644 --- a/src/main/runtime/rpc/methods/gitlab.test.ts +++ b/src/main/runtime/rpc/methods/gitlab.test.ts @@ -4,6 +4,7 @@ import { RpcDispatcher } from '../dispatcher' import type { RpcRequest } from '../core' import type { OrcaRuntimeService } from '../../orca-runtime' import { GITLAB_METHODS } from './gitlab' +import { WorkItemDetails } from '../../../../shared/rpc-contract/gitlab-params' function makeRequest(method: string, params?: unknown): RpcRequest { return { id: 'req-1', authToken: 'tok', method, params } @@ -292,7 +293,8 @@ describe('gitlab RPC methods', () => { 'id:repo-1', 8, 'mr', - projectRef + projectRef, + undefined ) expect(runtime.getGitLabRepoWorkItemByPath).toHaveBeenCalledWith( 'id:repo-1', @@ -302,6 +304,36 @@ describe('gitlab RPC methods', () => { ) }) + it('forwards the optional Checks scope while old params safely strip it', async () => { + const getGitLabRepoWorkItemDetails = vi.fn().mockResolvedValue({ body: 'Details' }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: Only the runtime identity and work-item method are exercised by this dispatcher. + const runtime = { + getRuntimeId: () => 'test-runtime', + getGitLabRepoWorkItemDetails + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: GITLAB_METHODS }) + const params = { repo: 'id:repo-1', iid: 8, type: 'mr', includeReviewDetails: false } + await dispatcher.dispatch(makeRequest('gitlab.workItemDetails', params)) + expect(getGitLabRepoWorkItemDetails).toHaveBeenCalledWith( + 'id:repo-1', + 8, + 'mr', + undefined, + false + ) + expect(WorkItemDetails.parse(params)).toHaveProperty('includeReviewDetails', false) + const oldParams = WorkItemDetails.omit({ includeReviewDetails: true }).parse(params) + expect(oldParams).not.toHaveProperty('includeReviewDetails') + await dispatcher.dispatch(makeRequest('gitlab.workItemDetails', oldParams)) + expect(getGitLabRepoWorkItemDetails).toHaveBeenLastCalledWith( + 'id:repo-1', + 8, + 'mr', + undefined, + undefined + ) + }) + it('accepts the negotiated ready-for-review update field', async () => { const runtime = { getRuntimeId: () => 'test-runtime', diff --git a/src/main/runtime/rpc/methods/gitlab.ts b/src/main/runtime/rpc/methods/gitlab.ts index d664c599442..d3a544079e1 100644 --- a/src/main/runtime/rpc/methods/gitlab.ts +++ b/src/main/runtime/rpc/methods/gitlab.ts @@ -201,7 +201,13 @@ export const GITLAB_METHODS = [ permission: 'workspace', params: WorkItemDetails, handler: async (params, { runtime }) => - runtime.getGitLabRepoWorkItemDetails(params.repo, params.iid, params.type, params.projectRef) + runtime.getGitLabRepoWorkItemDetails( + params.repo, + params.iid, + params.type, + params.projectRef, + params.includeReviewDetails + ) }), defineMethod({ name: 'gitlab.workItemByPath', diff --git a/src/main/runtime/runtime-gitlab-query-commands.ts b/src/main/runtime/runtime-gitlab-query-commands.ts index 14d46cc57b1..5a994cb9188 100644 --- a/src/main/runtime/runtime-gitlab-query-commands.ts +++ b/src/main/runtime/runtime-gitlab-query-commands.ts @@ -158,7 +158,8 @@ export class RuntimeGitLabQueryCommands { repoSelector: string, iid: number, type: 'issue' | 'mr', - projectRef?: GitLabProjectRef | null + projectRef?: GitLabProjectRef | null, + includeReviewDetails?: boolean ): Promise>> { const repo = await this.deps.resolveRepo(repoSelector) return getWorkItemDetails( @@ -168,7 +169,8 @@ export class RuntimeGitLabQueryCommands { repo.issueSourcePreference, repo.connectionId ?? null, projectRef, - ...this.deps.getLocalGitArgs(repo) + this.deps.getLocalGitArgs(repo)[0] ?? {}, + { includeReviewDetails } ) } diff --git a/src/preload/api/gitlab-api.ts b/src/preload/api/gitlab-api.ts index 3f034573deb..0d692771c1d 100644 --- a/src/preload/api/gitlab-api.ts +++ b/src/preload/api/gitlab-api.ts @@ -104,6 +104,7 @@ export type GitLabApi = { args: GitLabRepoSelectorArgs & { iid: number type: 'issue' | 'mr' + includeReviewDetails?: boolean } ) => Promise closeMR: ( diff --git a/src/preload/gitlab-work-item-details.test.ts b/src/preload/gitlab-work-item-details.test.ts new file mode 100644 index 00000000000..b7a7c955168 --- /dev/null +++ b/src/preload/gitlab-work-item-details.test.ts @@ -0,0 +1,18 @@ +import { beforeEach, expect, it, vi } from 'vitest' + +const invoke = vi.hoisted(() => vi.fn()) +vi.mock('electron', () => ({ ipcRenderer: { invoke } })) +import { glApi } from './gitlab' + +beforeEach(() => invoke.mockReset()) + +it('preserves the optional review scope through the existing desktop details channel', async () => { + const args = { repoPath: '/repo', iid: 12, type: 'mr' as const, includeReviewDetails: false } + const details = { body: 'MR body', comments: [], pipelineJobs: [] } + invoke.mockResolvedValue(details) + expect(await glApi.workItemDetails(args)).toBe(details) + expect(invoke).toHaveBeenCalledWith('gitlab:workItemDetails', args) + const defaultArgs = { repoPath: '/repo', iid: 12, type: 'mr' as const } + await glApi.workItemDetails(defaultArgs) + expect(invoke).toHaveBeenLastCalledWith('gitlab:workItemDetails', defaultArgs) +}) diff --git a/src/preload/gitlab.ts b/src/preload/gitlab.ts index 154e139e4c4..1fdb9057702 100644 --- a/src/preload/gitlab.ts +++ b/src/preload/gitlab.ts @@ -89,6 +89,7 @@ export const glApi = { args: GitLabRepoSelectorArgs & { iid: number type: 'issue' | 'mr' + includeReviewDetails?: boolean } ) => ipcRenderer.invoke('gitlab:workItemDetails', args), diff --git a/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.test.ts b/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.test.ts index 2d895a87251..4a5e5fc1f97 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.test.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.test.ts @@ -47,7 +47,8 @@ describe('GitLab checks-panel provider adapter', () => { repoId: 'repo-1', repoOwnerExecutionHostId: 'ssh:devbox', iid: 17, - type: 'mr' + type: 'mr', + includeReviewDetails: false }) expect(gl.resolveMRDiscussion).toHaveBeenCalledOnce() }) @@ -64,7 +65,12 @@ describe('GitLab checks-panel provider adapter', () => { expect(runtime.call).toHaveBeenCalledWith( { kind: 'environment', environmentId: 'owner-runtime' }, 'gitlab.workItemDetails', - expect.objectContaining({ repo: 'id:repo-1', iid: 17, type: 'mr' }), + expect.objectContaining({ + repo: 'id:repo-1', + iid: 17, + type: 'mr', + includeReviewDetails: false + }), { timeoutMs: 30_000 } ) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.ts b/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.ts index 665f8b3a5ee..0c62fa75d73 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.ts @@ -33,7 +33,8 @@ export async function fetchGitLabMRDetailsForChecks(args: { repoId: args.repoId, repoOwnerExecutionHostId: args.repoOwnerExecutionHostId, iid: args.iid, - type: 'mr' + type: 'mr', + includeReviewDetails: false }) } diff --git a/src/shared/rpc-contract/gitlab-params.ts b/src/shared/rpc-contract/gitlab-params.ts index 4165f190968..fa49afb6f50 100644 --- a/src/shared/rpc-contract/gitlab-params.ts +++ b/src/shared/rpc-contract/gitlab-params.ts @@ -138,7 +138,8 @@ export const RetryJob = RepoSelector.extend({ export const WorkItemDetails = RepoSelector.extend({ iid: z.number().int().positive(), type: z.enum(['issue', 'mr']), - projectRef: GitLabProjectRef + projectRef: GitLabProjectRef, + includeReviewDetails: z.boolean().optional() }) export const WorkItemByPath = RepoSelector.extend({