diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-check-and-review-actions.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-check-and-review-actions.test.tsx index 2b5b3f30a5a..e9ee0859370 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-check-and-review-actions.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-check-and-review-actions.test.tsx @@ -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) => void>() + const fetchHostedReviewForBranch = vi.fn( + (_repoPath: string, _branch: string, options?: { linkedGitLabMR?: number | null }) => + new Promise>((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), + number: 43, + headSha: 'head-43' + }) + reviewResolvers.get(44)?.({ + ...(input.activeReview as NonNullable), + 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) => void + const fetchHostedReviewForBranch = vi.fn( + () => + new Promise>((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), + number: 43, + headSha: 'head-43' + }) + await act(async () => save) + + expect(fetchGitLabDetails).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.test.tsx index 80e522c138e..9d177efa92b 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.test.tsx @@ -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) + }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.tsx index 91941e9741b..510a60cc025 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-polling.tsx @@ -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) } diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-review-link-actions.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-review-link-actions.tsx index 37974ff4483..1920f0c3a8c 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-review-link-actions.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-review-link-actions.tsx @@ -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 @@ -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 } diff --git a/src/renderer/src/components/sidebar/WorktreeMetaDialog.test.tsx b/src/renderer/src/components/sidebar/WorktreeMetaDialog.test.tsx index a14cd10c65e..d367c6ef44c 100644 --- a/src/renderer/src/components/sidebar/WorktreeMetaDialog.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeMetaDialog.test.tsx @@ -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 + updates: Partial, + options?: WorktreeMetaUpdateOptions ) => Promise<{ ok: true } | { ok: false; error: string }> >() const fetchLinearIssue = vi.fn<(...args: never[]) => Promise>() @@ -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', () => { diff --git a/src/renderer/src/components/sidebar/WorktreeMetaDialog.tsx b/src/renderer/src/components/sidebar/WorktreeMetaDialog.tsx index 1d26fc7240b..d35d26e8055 100644 --- a/src/renderer/src/components/sidebar/WorktreeMetaDialog.tsx +++ b/src/renderer/src/components/sidebar/WorktreeMetaDialog.tsx @@ -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) @@ -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,