From 352b29a030fa4241e3303ec5d2345a31ce159f23 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Fri, 23 Oct 2026 02:10:20 -0700 Subject: [PATCH] fix: preserve workspace review references when clearing active reviews (#32263) Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../worktree-meta-write-normalization.test.ts | 20 ++++++++ .../src/store/github/pull-request-actions.ts | 3 +- .../store/github/pull-request-execution.ts | 5 +- .../src/store/github/refresh-event-actions.ts | 49 ++++++++----------- ...ithub-pr-branch-coordinator-events.test.ts | 5 +- ...hub-pr-branch-linked-pr-divergence.test.ts | 14 ++++-- .../store/slices/github-slice-test-harness.ts | 5 +- ...-identity-review-clear-persistence.test.ts | 9 ++++ .../metadata/hosted-review-link-mutation.ts | 7 +-- .../metadata/worktree-git-identity-update.ts | 6 ++- src/shared/workspace-attachments.test.ts | 21 ++++++++ src/shared/workspace-attachments.ts | 5 ++ src/shared/worktree/hosted-review-links.ts | 16 +++--- 13 files changed, 116 insertions(+), 49 deletions(-) diff --git a/src/main/persistence/loading-store/worktree-meta-write-normalization.test.ts b/src/main/persistence/loading-store/worktree-meta-write-normalization.test.ts index f21d447a92c..206ce390822 100644 --- a/src/main/persistence/loading-store/worktree-meta-write-normalization.test.ts +++ b/src/main/persistence/loading-store/worktree-meta-write-normalization.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import type { WorktreeMeta } from '../../../shared/worktree/meta-types' import { mergeWorktreeMetaForWrite } from './worktree-meta-write-normalization' +import { CLEARED_HOSTED_REVIEW_LINK_UPDATES } from '../../../shared/worktree/hosted-review-links' const existingMeta: WorktreeMeta = { displayName: 'Feature', @@ -17,6 +18,25 @@ const existingMeta: WorktreeMeta = { } describe('worktree metadata write normalization', () => { + it('retains all five PRs while each successive branch clears its active review', () => { + let meta = existingMeta + const numbers = [32201, 32206, 32211, 32216, 32218] + for (const number of numbers) { + meta = mergeWorktreeMetaForWrite(meta, { + linkedPR: number, + pushTarget: { remoteName: 'origin', branchName: `review-${number}` } + }) + meta = mergeWorktreeMetaForWrite(meta, CLEARED_HOSTED_REVIEW_LINK_UPDATES) + expect(meta.linkedPR).toBeNull() + expect(meta.pushTarget).toBeUndefined() + } + expect(meta.linkedItems?.map((item) => item.number)).toEqual(numbers) + + const selected = mergeWorktreeMetaForWrite(meta, { linkedPR: 32211 }) + const unlinked = mergeWorktreeMetaForWrite(selected, { linkedPR: null }) + expect(unlinked.linkedItems?.map((item) => item.number)).toEqual([32201, 32206, 32216, 32218]) + }) + it('clears GitHub PR suppression on every positive linked PR update', () => { const updated = mergeWorktreeMetaForWrite(existingMeta, { linkedPR: 42, diff --git a/src/renderer/src/store/github/pull-request-actions.ts b/src/renderer/src/store/github/pull-request-actions.ts index 72152e006fc..f869fbc1fb9 100644 --- a/src/renderer/src/store/github/pull-request-actions.ts +++ b/src/renderer/src/store/github/pull-request-actions.ts @@ -1,3 +1,4 @@ +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from '../../../../shared/worktree/hosted-review-links' import type { StateCreator } from 'zustand' import type { AppState } from '../types' import type { GitHubSlice } from './slice-types' @@ -88,7 +89,7 @@ export const createPullRequestActions = ( ) { void get().updateWorktreeMeta( options.worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: (worktree) => shouldApplyDivergedLinkedPRClear({ diff --git a/src/renderer/src/store/github/pull-request-execution.ts b/src/renderer/src/store/github/pull-request-execution.ts index 10a74ea23e5..8cfdd7f66f9 100644 --- a/src/renderer/src/store/github/pull-request-execution.ts +++ b/src/renderer/src/store/github/pull-request-execution.ts @@ -1,3 +1,4 @@ +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from '../../../../shared/worktree/hosted-review-links' import type { StateCreator } from 'zustand' import type { RuntimeClientTarget } from '../../../../shared/runtime-client-target' import type { AppState } from '../types' @@ -209,7 +210,7 @@ export function startPullRequestLookup(args: { // Why: only clear the durable link that produced this exact probe; drift means the stale result no longer owns the worktree. void get().updateWorktreeMeta( options.worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: () => shouldApplyDivergedLinkedPRClear({ @@ -240,7 +241,7 @@ export function startPullRequestLookup(args: { ) { void get().updateWorktreeMeta( options.worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { // Why: the branch-scoped PR refetch below updates both caches; the generic metadata refresh would duplicate provider work. suppressHostedReviewRefresh: true, diff --git a/src/renderer/src/store/github/refresh-event-actions.ts b/src/renderer/src/store/github/refresh-event-actions.ts index 41654288a3b..80c19aa079e 100644 --- a/src/renderer/src/store/github/refresh-event-actions.ts +++ b/src/renderer/src/store/github/refresh-event-actions.ts @@ -1,3 +1,4 @@ +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from '../../../../shared/worktree/hosted-review-links' import type { StateCreator } from 'zustand' import type { AppState } from '../types' import type { GitHubSlice } from './slice-types' @@ -241,36 +242,28 @@ export const createRefreshEventActions = ( : s }) for (const clear of divergedLinkedPRClears) { - void get().updateWorktreeMeta( - clear.worktreeId, - { linkedPR: null }, - { - shouldApply: () => - shouldApplyDivergedLinkedPRClear({ - worktree: - findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, - linkedPRNumber: clear.linkedPRNumber, - branch: clear.branch, - requestHeadOid: clear.requestHeadOid - }) - } - ) + void get().updateWorktreeMeta(clear.worktreeId, CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { + shouldApply: () => + shouldApplyDivergedLinkedPRClear({ + worktree: + findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, + linkedPRNumber: clear.linkedPRNumber, + branch: clear.branch, + requestHeadOid: clear.requestHeadOid + }) + }) } for (const clear of branchMismatchedLinkedPRClears) { - void get().updateWorktreeMeta( - clear.worktreeId, - { linkedPR: null }, - { - shouldApply: () => - shouldApplyBranchMismatchedLinkedPRClear({ - worktree: - findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, - linkedPRNumber: clear.linkedPRNumber, - branch: clear.branch, - requestHeadOid: clear.requestHeadOid - }) - } - ) + void get().updateWorktreeMeta(clear.worktreeId, CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { + shouldApply: () => + shouldApplyBranchMismatchedLinkedPRClear({ + worktree: + findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, + linkedPRNumber: clear.linkedPRNumber, + branch: clear.branch, + requestHeadOid: clear.requestHeadOid + }) + }) } } }) diff --git a/src/renderer/src/store/slices/github-pr-branch-coordinator-events.test.ts b/src/renderer/src/store/slices/github-pr-branch-coordinator-events.test.ts index 63ca5ab4192..7b1e1d4d4a1 100644 --- a/src/renderer/src/store/slices/github-pr-branch-coordinator-events.test.ts +++ b/src/renderer/src/store/slices/github-pr-branch-coordinator-events.test.ts @@ -1,3 +1,4 @@ +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from '../../../../shared/worktree/hosted-review-links' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { _clearGitHubPRRefreshStartedEntriesForTest } from '../github/request-coordination' import { @@ -76,7 +77,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: expect.any(Function) } ) expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() @@ -126,7 +127,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: expect.any(Function) } ) expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() diff --git a/src/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.ts b/src/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.ts index 8b42bf3e56f..487021f3aa5 100644 --- a/src/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.ts +++ b/src/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.ts @@ -1,3 +1,4 @@ +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from '../../../../shared/worktree/hosted-review-links' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { _clearGitHubPRRefreshStartedEntriesForTest } from '../github/request-coordination' import { @@ -142,10 +143,15 @@ describe('createGitHubSlice.fetchPRForBranch', () => { expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: expect.any(Function) } ) expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() + expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedItems).toContainEqual({ + provider: 'github', + type: 'pr', + number: 12 + }) expect(store.getState().prCache[`${repoId}::${branch}`]).toBeUndefined() }) @@ -198,7 +204,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { }) expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { suppressHostedReviewRefresh: true, shouldApply: expect.any(Function) @@ -256,7 +262,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { expect(mockApi.gh.refreshPRNow).not.toHaveBeenCalled() expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: expect.any(Function) } ) expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() @@ -572,7 +578,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { await expect(request).resolves.toMatchObject({ number: 12 }) expect(updateWorktreeMeta).toHaveBeenCalledWith( worktreeId, - { linkedPR: null }, + CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, { shouldApply: expect.any(Function) } ) expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBe(12) diff --git a/src/renderer/src/store/slices/github-slice-test-harness.ts b/src/renderer/src/store/slices/github-slice-test-harness.ts index 82afe0ffb9b..67642e21929 100644 --- a/src/renderer/src/store/slices/github-slice-test-harness.ts +++ b/src/renderer/src/store/slices/github-slice-test-harness.ts @@ -11,6 +11,7 @@ import { } from '../../runtime/runtime-compatibility-test-fixture' import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' import type { TaskSourceContext } from '../../../../shared/task-source-context' +import { normalizeWorkspaceAttachmentUpdate } from '../../../../shared/workspace-attachments' import type { GitHubPRRefreshCandidate, GitHubPRRefreshReason @@ -189,7 +190,9 @@ export function installLinkedPRClearStub( const nextWorktrees = { ...state.worktreesByRepo, [args.repoId]: (state.worktreesByRepo[args.repoId] ?? []).map((worktree) => - worktree.id === worktreeId ? { ...worktree, ...updates } : worktree + worktree.id === worktreeId + ? { ...worktree, ...normalizeWorkspaceAttachmentUpdate(worktree, updates) } + : worktree ) } const nextPRCache = { ...state.prCache } diff --git a/src/renderer/src/store/slices/worktrees-git-identity-review-clear-persistence.test.ts b/src/renderer/src/store/slices/worktrees-git-identity-review-clear-persistence.test.ts index 25b18feadb0..63d0f9fecf2 100644 --- a/src/renderer/src/store/slices/worktrees-git-identity-review-clear-persistence.test.ts +++ b/src/renderer/src/store/slices/worktrees-git-identity-review-clear-persistence.test.ts @@ -41,6 +41,7 @@ describe('updateWorktreeGitIdentity', () => { repoId: 'repo1', path: '/path/wt1', branch: 'refs/heads/stack/one', + linkedPR: 101, pushTarget: { remoteName: 'fork', branchName: 'old/review-head' } }) @@ -55,6 +56,9 @@ describe('updateWorktreeGitIdentity', () => { branch: 'refs/heads/stack/two', pushTarget: undefined }) + expect(store.getState().worktreesByRepo.repo1[0]?.linkedItems).toEqual([ + { provider: 'github', type: 'pr', number: 101 } + ]) expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: 'repo1::/path/wt1', updates: { @@ -369,6 +373,11 @@ describe('updateWorktreeGitIdentity', () => { mockApi.worktrees.list.mockResolvedValue([staleRefetch]) await store.getState().fetchWorktrees('repo1') + expect(store.getState().worktreesByRepo.repo1[0]?.linkedItems).toContainEqual({ + provider: 'github', + type: 'pr', + number: 101 + }) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: 'refs/heads/stack/two', head: 'new-head', diff --git a/src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts b/src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts index 1f490602dc0..1f8a17c8dd7 100644 --- a/src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts +++ b/src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts @@ -4,6 +4,7 @@ import type { WorktreeSlice } from '../../worktree-helpers' import { applyWorktreeUpdates } from '../../worktree-helpers' import type { Worktree } from '../../../../../../shared/worktree/types' import type { WorktreeMeta } from '../../../../../../shared/worktree/meta-types' +import { normalizeWorkspaceAttachmentUpdate } from '../../../../../../shared/workspace-attachments' import { applyDetectedWorktreeUpdates } from '../listing/detected-worktree-meta' import type { HostedReviewLinkKey, @@ -177,7 +178,7 @@ export function rememberHostedReviewLinkClear( export function sanitizeHostedReviewLinksForBranchClear< T extends Pick & - Partial> + Partial> >(worktree: T, currentWorktrees?: readonly T[]): T { const hostedReviewWorktreeId = resolveHostedReviewLinkWorktreeId(worktree.id) const tombstone = hostedReviewLinkClearTombstonesByWorktreeId.get(hostedReviewWorktreeId) @@ -208,13 +209,13 @@ export function sanitizeHostedReviewLinksForBranchClear< ...worktree, branch: guardBranch, ...(guardHead !== undefined ? { head: guardHead } : {}), - ...CLEARED_HOSTED_REVIEW_LINK_UPDATES + ...normalizeWorkspaceAttachmentUpdate(worktree, CLEARED_HOSTED_REVIEW_LINK_UPDATES) } } export function sanitizeHostedReviewLinksForBranchClears< T extends Pick & - Partial> + Partial> >(worktrees: readonly T[], currentWorktrees?: readonly T[]): T[] { let changed = false const sanitized = worktrees.map((worktree) => { diff --git a/src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts b/src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts index 8c58b1ac2b6..f045126e55d 100644 --- a/src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts +++ b/src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts @@ -2,6 +2,7 @@ import type { WorktreeSlice } from '../../worktree-helpers' import { runtimeTargetForWorktreeOwner } from '../listing/worktree-owner-target' import type { WorktreeSliceGet, WorktreeSliceSet } from '../listing/worktree-slice-types' import type { Worktree } from '../../../../../../shared/worktree/types' +import { normalizeWorkspaceAttachmentUpdate } from '../../../../../../shared/workspace-attachments' import { getRepoIdFromWorktreeId } from '../../worktree-helpers' import { branchName } from '@/lib/git-utils' import { detachedHeadAutoDerivedDisplayNames } from './detached-head-display-name' @@ -108,8 +109,9 @@ export function createUpdateWorktreeGitIdentity( head: nextHead, branch: nextBranch, displayName: nextDisplayName, - // Why: linked reviews are branch-scoped; keeping the old link on a branch switch would refresh the old PR. - ...(shouldClearHostedReviewContext ? CLEARED_HOSTED_REVIEW_LINK_UPDATES : {}) + ...(shouldClearHostedReviewContext + ? normalizeWorkspaceAttachmentUpdate(worktree, CLEARED_HOSTED_REVIEW_LINK_UPDATES) + : {}) } }) diff --git a/src/shared/workspace-attachments.test.ts b/src/shared/workspace-attachments.test.ts index 3ac930179ff..c923de764f9 100644 --- a/src/shared/workspace-attachments.test.ts +++ b/src/shared/workspace-attachments.test.ts @@ -11,10 +11,31 @@ import { } from './workspace-attachment-normalization' import { WorktreeSet } from './rpc-contract/worktree-params' import { FolderWorkspaceUpdate } from './rpc-contract/folder-workspace-params' +import { CLEARED_HOSTED_REVIEW_SELECTION_UPDATES } from './worktree/hosted-review-links' const pr = (number: number): WorkspaceAttachment => ({ provider: 'github', type: 'pr', number }) describe('workspace attachments', () => { + it.each([ + ['linkedPR', 'github', 'pr'], + ['linkedGitLabMR', 'gitlab', 'mr'], + ['linkedBitbucketPR', 'bitbucket', 'pr'], + ['linkedAzureDevOpsPR', 'azure-devops', 'pr'], + ['linkedGiteaPR', 'gitea', 'pr'] + ] as const)('keeps legacy %s references when deselecting a review', (slot, provider, type) => { + const existing = { [slot]: 7, linkedIssue: 42 } + const result = { + ...existing, + ...normalizeWorkspaceAttachmentUpdate(existing, CLEARED_HOSTED_REVIEW_SELECTION_UPDATES) + } + expect(result[slot]).toBeNull() + expect(result.linkedIssue).toBe(42) + expect(result.linkedItems).toEqual([ + { provider: 'github', type: 'issue', number: 42 }, + { provider, type, number: 7 } + ]) + }) + it('reads old metadata and enriches matching collection entries without duplicate compatibility links', () => { const item = { ...pr(7), diff --git a/src/shared/workspace-attachments.ts b/src/shared/workspace-attachments.ts index b90dc709f68..fe4d8d09885 100644 --- a/src/shared/workspace-attachments.ts +++ b/src/shared/workspace-attachments.ts @@ -188,7 +188,12 @@ export function normalizeWorkspaceAttachmentUpdate( linkedWorkItem: existing?.linkedWorkItem, linkedTaskSourceContext: existing?.linkedTaskSourceContext })[0] + const selectionCleared = reviewSlots.every(([slot]) => requested[slot] === null) for (const fields of WORKSPACE_ATTACHMENT_LEGACY_FIELDS) { + // Clearing the active review must not delete the workspace's review history. + if (selectionCleared && reviewSlots.some(([slot]) => slot === fields[0])) { + continue + } if (updates[fields[0]] === null) { items = removeSelectedWorkspaceAttachments( items, diff --git a/src/shared/worktree/hosted-review-links.ts b/src/shared/worktree/hosted-review-links.ts index 6527e7086c4..9dd6ebc7810 100644 --- a/src/shared/worktree/hosted-review-links.ts +++ b/src/shared/worktree/hosted-review-links.ts @@ -15,16 +15,20 @@ export const HOSTED_REVIEW_LINK_KEYS: readonly HostedReviewLinkKey[] = [ 'linkedGiteaPR' ] -/** Linked reviews and the push target are branch-scoped: a branch switch clears all of them. */ -export const CLEARED_HOSTED_REVIEW_LINK_UPDATES: Pick< - WorktreeMeta, - HostedReviewLinkKey | 'pushTarget' -> = { +export const CLEARED_HOSTED_REVIEW_SELECTION_UPDATES: Pick = { linkedPR: null, linkedGitLabMR: null, linkedBitbucketPR: null, linkedAzureDevOpsPR: null, - linkedGiteaPR: null, + linkedGiteaPR: null +} + +/** Branch switches clear the active review and push target, preserving workspace references. */ +export const CLEARED_HOSTED_REVIEW_LINK_UPDATES: Pick< + WorktreeMeta, + HostedReviewLinkKey | 'pushTarget' +> = { + ...CLEARED_HOSTED_REVIEW_SELECTION_UPDATES, pushTarget: undefined }