starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-11-02 18:17:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

perf(gitlab): skip unused review details in Checks refreshes (#28643)

Browse Source
This commit is contained in:
Neil
2026-10-19 16:12:08 -07:00
committed by GitHub
parent c5d03fc475
commit 30d1e8dba5
14 changed files with 281 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/main/gitlab/work-item-details.test.ts
+130 -3
View File
@@ -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
src/main/gitlab/work-item-details.ts
+20 -10
View File
@@ -57,7 +57,8 @@ export async function getWorkItemDetails(
preference?: IssueSourcePreference,
connectionId?: string | null,
projectRefOverride?: ProjectRef | null,
localGitOptions: LocalGitExecOptions = {}
localGitOptions: LocalGitExecOptions = {},
options: { includeReviewDetails?: boolean } = {}
): Promise<GitLabWorkItemDetails | null> {
// 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<GitLabWorkItemDetails | null> {
// 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,
src/main/ipc/gitlab-work-item-handlers.ts
+10 -2
View File
@@ -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 }
)
}
)
src/main/ipc/gitlab.test.ts
+18 -1
View File
@@ -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',
src/main/runtime/orca-runtime-tests/gitlab-and-pr-bases.spec.ts
+27 -2
View File
@@ -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,
src/main/runtime/rpc/methods/gitlab.test.ts
+33 -1
View File
@@ -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',
src/main/runtime/rpc/methods/gitlab.ts
+7 -1
View File
@@ -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',
src/main/runtime/runtime-gitlab-query-commands.ts
+4 -2
View File
@@ -158,7 +158,8 @@ export class RuntimeGitLabQueryCommands {
repoSelector: string,
iid: number,
type: 'issue' | 'mr',
projectRef?: GitLabProjectRef | null
projectRef?: GitLabProjectRef | null,
includeReviewDetails?: boolean
): Promise<Awaited<ReturnType<typeof getWorkItemDetails>>> {
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 }
)
}
src/preload/api/gitlab-api.ts
+1
View File
@@ -104,6 +104,7 @@ export type GitLabApi = {
args: GitLabRepoSelectorArgs & {
iid: number
type: 'issue' | 'mr'
includeReviewDetails?: boolean
}
) => Promise<GitLabWorkItemDetails | null>
closeMR: (
src/preload/gitlab-work-item-details.test.ts
+18
View File
@@ -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)
})
src/preload/gitlab.ts
+1
View File
@@ -89,6 +89,7 @@ export const glApi = {
args: GitLabRepoSelectorArgs & {
iid: number
type: 'issue' | 'mr'
includeReviewDetails?: boolean
}
) => ipcRenderer.invoke('gitlab:workItemDetails', args),
src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.test.ts
+8 -2
View File
@@ -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 }
)
})
src/renderer/src/components/right-sidebar/checks-panel/gitlab-review-client.ts
+2 -1
View File
@@ -33,7 +33,8 @@ export async function fetchGitLabMRDetailsForChecks(args: {
repoId: args.repoId,
repoOwnerExecutionHostId: args.repoOwnerExecutionHostId,
iid: args.iid,
type: 'mr'
type: 'mr',
includeReviewDetails: false
})
}
src/shared/rpc-contract/gitlab-params.ts
+2 -1
View File
@@ -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({