fix stale GitLab review relink updates

This commit is contained in:
Brennan Benson
2026-08-27 18:18:09 -07:00
parent 29df510842
commit 9d9d3116ae
6 changed files with 257 additions and 35 deletions
@@ -106,27 +106,146 @@ describe('useChecksPanelCheckAndReviewActions GitLab links', () => {
})
const openModal = vi.fn()
const input = makeInput({ fetchGitLabDetails, fetchHostedReviewForBranch, openModal })
const { result } = renderHook(() => useChecksPanelCheckAndReviewActions(input))
const hook = renderHook(({ model }) => useChecksPanelCheckAndReviewActions(model), {
initialProps: { model: input }
})
act(() => result.current.handleLinkAnotherReview())
act(() => hook.result.current.handleLinkAnotherReview())
const modal = openModal.mock.calls[0]?.[1]
expect(modal.suppressHostedReviewRefresh).toBe(true)
hook.rerender({
model: {
...input,
activeWorktree: { ...input.activeWorktree, linkedGitLabMR: 43 },
linkedGitLabMR: 43,
panelContextKey: 'context::gitlab::43'
} as Input
})
await act(async () => modal.afterSave({ updates: { linkedGitLabMR: 43 } }))
expect(fetchHostedReviewForBranch).toHaveBeenCalledWith(
'/workspace/repo',
'feature/mr',
expect.objectContaining({
force: true,
linkedGitLabMR: 43,
repoOwnerExecutionHostId: 'ssh:ssh-1',
repoOwnerCacheScope: 'ssh-1'
repoOwnerExecutionHostId: 'ssh:ssh-1'
})
)
expect(fetchGitLabDetails).toHaveBeenCalledWith({
mrNumberOverride: 43,
headShaOverride: 'abc123',
commitAsCurrent: true,
settingsOverride: input.ownerSettings
expect(fetchHostedReviewForBranch).toHaveBeenCalledOnce()
expect(fetchGitLabDetails).toHaveBeenCalledWith(
expect.objectContaining({
mrNumberOverride: 43,
headShaOverride: 'abc123',
commitAsCurrent: true,
settingsOverride: input.ownerSettings,
isRequestCurrent: expect.any(Function)
})
)
})
it('drops an older replacement MR refresh when a newer relink wins', async () => {
const reviewResolvers = new Map<number, (value: NonNullable<Input['activeReview']>) => void>()
const fetchHostedReviewForBranch = vi.fn(
(_repoPath: string, _branch: string, options?: { linkedGitLabMR?: number | null }) =>
new Promise<NonNullable<Input['activeReview']>>((resolve) => {
reviewResolvers.set(options?.linkedGitLabMR ?? 0, resolve)
})
)
const fetchGitLabDetails = vi.fn()
const openModal = vi.fn()
const input = makeInput({ fetchGitLabDetails, fetchHostedReviewForBranch, openModal })
const hook = renderHook(({ model }) => useChecksPanelCheckAndReviewActions(model), {
initialProps: { model: input }
})
act(() => hook.result.current.handleLinkAnotherReview())
const firstModal = openModal.mock.calls[0]?.[1]
hook.rerender({
model: {
...input,
activeWorktree: { ...input.activeWorktree, linkedGitLabMR: 43 },
linkedGitLabMR: 43,
panelContextKey: 'context::gitlab::43'
} as Input
})
const firstSave = firstModal.afterSave({ updates: { linkedGitLabMR: 43 } })
await act(() => Promise.resolve())
act(() => hook.result.current.handleLinkAnotherReview())
const secondModal = openModal.mock.calls[1]?.[1]
hook.rerender({
model: {
...input,
activeWorktree: { ...input.activeWorktree, linkedGitLabMR: 44 },
linkedGitLabMR: 44,
panelContextKey: 'context::gitlab::44'
} as Input
})
const secondSave = secondModal.afterSave({ updates: { linkedGitLabMR: 44 } })
await act(() => Promise.resolve())
reviewResolvers.get(43)?.({
...(input.activeReview as NonNullable<Input['activeReview']>),
number: 43,
headSha: 'head-43'
})
reviewResolvers.get(44)?.({
...(input.activeReview as NonNullable<Input['activeReview']>),
number: 44,
headSha: 'head-44'
})
await act(async () => firstSave)
await act(async () => secondSave)
expect(fetchGitLabDetails).toHaveBeenCalledOnce()
expect(fetchGitLabDetails).toHaveBeenCalledWith(
expect.objectContaining({ mrNumberOverride: 44, headShaOverride: 'head-44' })
)
})
it('drops replacement MR results after an external link mutation', async () => {
let resolveReview!: (value: NonNullable<Input['activeReview']>) => void
const fetchHostedReviewForBranch = vi.fn(
() =>
new Promise<NonNullable<Input['activeReview']>>((resolve) => {
resolveReview = resolve
})
)
const fetchGitLabDetails = vi.fn()
const openModal = vi.fn()
const input = makeInput({ fetchGitLabDetails, fetchHostedReviewForBranch, openModal })
const hook = renderHook(({ model }) => useChecksPanelCheckAndReviewActions(model), {
initialProps: { model: input }
})
act(() => hook.result.current.handleLinkAnotherReview())
const modal = openModal.mock.calls[0]?.[1]
hook.rerender({
model: {
...input,
activeWorktree: { ...input.activeWorktree, linkedGitLabMR: 43 },
linkedGitLabMR: 43,
panelContextKey: 'context::gitlab::43'
} as Input
})
const save = modal.afterSave({ updates: { linkedGitLabMR: 43 } })
await act(() => Promise.resolve())
hook.rerender({
model: {
...input,
activeWorktree: { ...input.activeWorktree, linkedGitLabMR: 44 },
linkedGitLabMR: 44,
panelContextKey: 'context::gitlab::44'
} as Input
})
resolveReview({
...(input.activeReview as NonNullable<Input['activeReview']>),
number: 43,
headSha: 'head-43'
})
await act(async () => save)
expect(fetchGitLabDetails).not.toHaveBeenCalled()
})
})
@@ -166,4 +166,79 @@ describe('useChecksPanelPolling live behavior', () => {
expect(model.asyncResultKeyRef.current).toContain('::18::none')
expect(model.asyncResultKeyRef.current).not.toContain('old-head')
})
it('drops replacement MR details when the relink scope changes in flight', async () => {
let resolveDetails!: (value: {
item: { projectRef: null }
pipelineJobs: PRCheckDetail[]
comments: []
}) => void
gitlab.fetchDetails.mockReturnValueOnce(
new Promise((resolve) => {
resolveDetails = resolve
})
)
let requestCurrent = true
const model = createModel()
const { result } = renderHook(() => useChecksPanelPolling(model))
const request = result.current.fetchGitLabDetails({
mrNumberOverride: 18,
commitAsCurrent: true,
isRequestCurrent: () => requestCurrent
})
await act(() => Promise.resolve())
requestCurrent = false
resolveDetails({
item: { projectRef: null },
pipelineJobs: [],
comments: []
})
await act(async () => request)
expect(model.setChecks).not.toHaveBeenCalled()
expect(model.setComments).not.toHaveBeenCalled()
expect(model.setChecksLoading).toHaveBeenLastCalledWith(false)
expect(model.setCommentsLoading).toHaveBeenLastCalledWith(false)
})
it('keeps loading owned by the newest replacement MR details request', async () => {
const detailsResolvers: ((value: {
item: { projectRef: null }
pipelineJobs: []
comments: []
}) => void)[] = []
gitlab.fetchDetails.mockImplementation(
() =>
new Promise((resolve) => {
detailsResolvers.push(resolve)
})
)
let firstRequestCurrent = true
const model = createModel()
const { result } = renderHook(() => useChecksPanelPolling(model))
const firstRequest = result.current.fetchGitLabDetails({
mrNumberOverride: 18,
commitAsCurrent: true,
isRequestCurrent: () => firstRequestCurrent
})
await act(() => Promise.resolve())
firstRequestCurrent = false
const secondRequest = result.current.fetchGitLabDetails({
mrNumberOverride: 18,
commitAsCurrent: true
})
await act(() => Promise.resolve())
detailsResolvers[0]?.({ item: { projectRef: null }, pipelineJobs: [], comments: [] })
await act(async () => firstRequest)
expect(model.setChecksLoading).not.toHaveBeenCalledWith(false)
expect(model.setCommentsLoading).not.toHaveBeenCalledWith(false)
detailsResolvers[1]?.({ item: { projectRef: null }, pipelineJobs: [], comments: [] })
await act(async () => secondRequest)
expect(model.setChecksLoading).toHaveBeenLastCalledWith(false)
expect(model.setCommentsLoading).toHaveBeenLastCalledWith(false)
})
})
@@ -1,4 +1,4 @@
import { useCallback, useEffect } from 'react'
import { useCallback, useEffect, useRef } from 'react'
import { installWindowVisibilityTimeoutPoller } from '@/lib/window-visibility-timeout-poller'
import { gitLabPipelineJobsToPRChecks } from '../../../../../shared/gitlab-pipeline-checks'
import {
@@ -56,6 +56,7 @@ export function useChecksPanelPolling(model: ChecksPanelPollingInput) {
setCommentsLoading,
gitLabProjectRefRef
} = model
const gitLabDetailsLoadingGenerationRef = useRef(0)
// Fetch checks via cached store method
const fetchChecks = useCallback(
async ({
@@ -139,12 +140,14 @@ export function useChecksPanelPolling(model: ChecksPanelPollingInput) {
mrNumberOverride,
headShaOverride,
commitAsCurrent = false,
settingsOverride
settingsOverride,
isRequestCurrent
}: {
mrNumberOverride?: number | null
headShaOverride?: string | null
commitAsCurrent?: boolean
settingsOverride?: ChecksPanelControllerState['settings']
isRequestCurrent?: () => boolean
} = {}) => {
const targetMRNumber = mrNumberOverride ?? activeGitLabReview?.number ?? null
const targetHeadSha =
@@ -159,9 +162,14 @@ export function useChecksPanelPolling(model: ChecksPanelPollingInput) {
targetMRNumber,
targetHeadSha
)
if (isRequestCurrent?.() === false) {
return
}
if (commitAsCurrent) {
asyncResultKeyRef.current = requestKey
}
const loadingGeneration = gitLabDetailsLoadingGenerationRef.current + 1
gitLabDetailsLoadingGenerationRef.current = loadingGeneration
setChecksLoading(true)
setCommentsLoading(true)
try {
@@ -172,7 +180,7 @@ export function useChecksPanelPolling(model: ChecksPanelPollingInput) {
iid: targetMRNumber,
repoOwnerExecutionHostId: activeWorktree?.hostId
})
if (!isCurrentAsyncResult(requestKey)) {
if (isRequestCurrent?.() === false || !isCurrentAsyncResult(requestKey)) {
return
}
gitLabProjectRefRef.current = details?.item.projectRef ?? null
@@ -186,14 +194,17 @@ export function useChecksPanelPolling(model: ChecksPanelPollingInput) {
: 30_000
prevChecksRef.current = signature
} catch (err) {
if (!isCurrentAsyncResult(requestKey)) {
if (isRequestCurrent?.() === false || !isCurrentAsyncResult(requestKey)) {
return
}
console.warn('Failed to fetch GitLab MR checks:', err)
setChecks([])
setComments([])
} finally {
if (isCurrentAsyncResult(requestKey)) {
if (
gitLabDetailsLoadingGenerationRef.current === loadingGeneration &&
isCurrentAsyncResult(requestKey)
) {
setChecksLoading(false)
setCommentsLoading(false)
}
@@ -1,5 +1,4 @@
import { useCallback, useRef } from 'react'
import { refreshHostedReviewCard } from '@/store/slices/hosted-review-card-refresh'
import type { ChecksPanelCheckAndReviewActionsInput } from './check-and-review-action-dependencies'
type RefreshLinkedGitHubPullRequest = (linkedPRNumber: number) => Promise<void>
@@ -37,7 +36,10 @@ export function useChecksPanelReviewLinkActions(
runtimeEnvironmentId,
localExecutionScope
])
const linkedGitLabMRRef = useRef(linkedGitLabMR)
const reviewLinkScopeKeyRef = useRef(reviewLinkScopeKey)
const reviewLinkActionGenerationRef = useRef(0)
linkedGitLabMRRef.current = linkedGitLabMR
reviewLinkScopeKeyRef.current = reviewLinkScopeKey
const handleUnlinkReview = useCallback(() => {
@@ -55,6 +57,7 @@ export function useChecksPanelReviewLinkActions(
if (!updates) {
return
}
reviewLinkActionGenerationRef.current += 1
void updateWorktreeMeta(activeWorktreeId, updates, { executionHostId: activeWorktree.hostId })
}, [activeReview, activeWorktree, activeWorktreeId, linkedGitLabMR, linkedPR, updateWorktreeMeta])
@@ -79,12 +82,18 @@ export function useChecksPanelReviewLinkActions(
: (activeWorktree.linkedPR ?? activeReview.number),
currentComment: activeWorktree.comment,
focus: 'pr',
suppressHostedReviewRefresh: true,
afterSave: async ({
updates
}: {
updates?: { linkedPR?: unknown; linkedGitLabMR?: unknown }
}) => {
if (reviewLinkScopeKeyRef.current !== openedScopeKey) {
const actionGeneration = reviewLinkActionGenerationRef.current + 1
reviewLinkActionGenerationRef.current = actionGeneration
const isActionCurrent = (): boolean =>
reviewLinkScopeKeyRef.current === openedScopeKey &&
reviewLinkActionGenerationRef.current === actionGeneration
if (!isActionCurrent()) {
return
}
if (provider === 'github') {
@@ -97,21 +106,19 @@ export function useChecksPanelReviewLinkActions(
if (typeof nextMR !== 'number') {
return
}
const refreshedReview = await refreshHostedReviewCard(fetchHostedReviewForBranch, {
repoPath: repo.path,
const isRequestCurrent = (): boolean =>
isActionCurrent() && linkedGitLabMRRef.current === nextMR
const refreshedReview = await fetchHostedReviewForBranch(repo.path, branch, {
repoId: repo.id,
branch,
repoOwnerExecutionHostId: activeWorktree.hostId,
linkedGitHubPR: null,
linkedGitLabMR: nextMR,
linkedBitbucketPR: null,
linkedAzureDevOpsPR: null,
linkedGiteaPR: null,
repoOwnerExecutionHostId: activeWorktree.hostId,
repoOwnerCacheScope:
localExecutionScope ?? runtimeEnvironmentId ?? repoConnectionId ?? activeWorktree.hostId
linkedGiteaPR: null
})
if (
reviewLinkScopeKeyRef.current !== openedScopeKey ||
!isRequestCurrent() ||
refreshedReview?.provider !== 'gitlab' ||
refreshedReview.number !== nextMR
) {
@@ -121,7 +128,8 @@ export function useChecksPanelReviewLinkActions(
mrNumberOverride: nextMR,
headShaOverride: refreshedReview.headSha ?? null,
commitAsCurrent: true,
settingsOverride: capturedOwnerSettings
settingsOverride: capturedOwnerSettings,
isRequestCurrent
})
}
})
@@ -132,14 +140,11 @@ export function useChecksPanelReviewLinkActions(
branch,
fetchGitLabDetails,
fetchHostedReviewForBranch,
localExecutionScope,
openModal,
ownerSettings,
refreshLinkedGitHubPullRequest,
repo,
repoConnectionId,
reviewLinkScopeKey,
runtimeEnvironmentId
reviewLinkScopeKey
])
return { handleUnlinkReview, handleLinkAnotherReview }
@@ -8,6 +8,7 @@ import type { FolderWorkspace } from '../../../../shared/folder-workspace-types'
import type { LinearIssue } from '../../../../shared/linear/issue-types'
import type { Repo } from '../../../../shared/repo-types'
import type { WorktreeMeta } from '../../../../shared/worktree/meta-types'
import type { WorktreeMetaUpdateOptions } from '@/store/slices/worktree-helpers'
import type { Worktree } from '../../../../shared/worktree/types'
import { folderWorkspaceKey } from '../../../../shared/workspace-scope'
@@ -61,7 +62,8 @@ const updateWorktreeMeta =
vi.fn<
(
id: string,
updates: Partial<WorktreeMeta>
updates: Partial<WorktreeMeta>,
options?: WorktreeMetaUpdateOptions
) => Promise<{ ok: true } | { ok: false; error: string }>
>()
const fetchLinearIssue = vi.fn<(...args: never[]) => Promise<LinearIssue | null>>()
@@ -136,6 +138,7 @@ function openDialog(
modalExecutionHostId?: string
modalReviewProvider?: 'github' | 'gitlab'
modalCurrentReview?: number
modalSuppressHostedReviewRefresh?: boolean
linearViewerOrganizationUrlKey?: string
} = {}
): void {
@@ -175,6 +178,7 @@ function openDialog(
...(options.modalExecutionHostId ? { executionHostId: options.modalExecutionHostId } : {}),
...(options.modalReviewProvider ? { reviewProvider: options.modalReviewProvider } : {}),
...(options.modalCurrentReview ? { currentReview: options.modalCurrentReview } : {}),
...(options.modalSuppressHostedReviewRefresh ? { suppressHostedReviewRefresh: true } : {}),
currentDisplayName: worktree.displayName,
currentComment: worktree.comment,
focus: 'comment'
@@ -233,7 +237,8 @@ describe('WorktreeMetaDialog issue link row', () => {
openDialog({
worktree: { linkedGitLabMR: 42 },
modalReviewProvider: 'gitlab',
modalCurrentReview: 42
modalCurrentReview: 42,
modalSuppressHostedReviewRefresh: true
})
const input = screen.getByPlaceholderText('MR ! or GitLab URL')
@@ -247,6 +252,7 @@ describe('WorktreeMetaDialog issue link row', () => {
expect.objectContaining({ linkedGitLabMR: 43 })
)
expect(updateWorktreeMeta.mock.calls[0]?.[1]).not.toHaveProperty('linkedPR')
expect(updateWorktreeMeta.mock.calls[0]?.[2]).toEqual({ suppressHostedReviewRefresh: true })
})
it('replaces a completed emoji shortcode in the display name', () => {
@@ -70,6 +70,7 @@ const WorktreeMetaDialog = React.memo(function WorktreeMetaDialog() {
const focusField = typeof modalData.focus === 'string' ? modalData.focus : 'comment'
const reviewProvider: WorktreeReviewProvider =
modalData.reviewProvider === 'gitlab' ? 'gitlab' : 'github'
const suppressHostedReviewRefresh = modalData.suppressHostedReviewRefresh === true
const afterSave =
typeof modalData.afterSave === 'function'
? (modalData.afterSave as (payload: WorktreeMetaSavedPayload) => void | Promise<void>)
@@ -241,9 +242,13 @@ const WorktreeMetaDialog = React.memo(function WorktreeMetaDialog() {
try {
const updates = buildWorktreeMetaUpdates(draft, snapshot, liveLinks, reviewProvider)
const result = executionHostId
? await updateWorktreeMeta(worktreeId, updates, { executionHostId })
: await updateWorktreeMeta(worktreeId, updates)
const result =
executionHostId || suppressHostedReviewRefresh
? await updateWorktreeMeta(worktreeId, updates, {
...(executionHostId ? { executionHostId } : {}),
...(suppressHostedReviewRefresh ? { suppressHostedReviewRefresh: true } : {})
})
: await updateWorktreeMeta(worktreeId, updates)
// Why: a failed save refetches and reverts the optimistic write. Closing
// here would report success for an edit that silently undid itself, and
// would discard the name, comment and PR changes in the same payload.
@@ -269,6 +274,7 @@ const WorktreeMetaDialog = React.memo(function WorktreeMetaDialog() {
}, [
worktreeId,
executionHostId,
suppressHostedReviewRefresh,
canSave,
draft,
snapshot,