starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-30 16:47:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: preserve workspace review references when clearing active reviews (#32263)

Browse Source 
Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This commit is contained in:
OrcaWin
2026-10-23 02:10:20 -07:00
committed by GitHub
co-authored by OrcaWin
parent b4ee652bcf
commit 352b29a030
13 changed files with 116 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/main/persistence/loading-store/worktree-meta-write-normalization.test.ts
+20
View File
@@ -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,
src/renderer/src/store/github/pull-request-actions.ts
+2 -1
View File
@@ -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({
src/renderer/src/store/github/pull-request-execution.ts
+3 -2
View File
@@ -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,
src/renderer/src/store/github/refresh-event-actions.ts
+21 -28
View File
@@ -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
})
})
}
}
})
src/renderer/src/store/slices/github-pr-branch-coordinator-events.test.ts
+3 -2
View File
@@ -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()
src/renderer/src/store/slices/github-pr-branch-linked-pr-divergence.test.ts
+10 -4
View File
@@ -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)
src/renderer/src/store/slices/github-slice-test-harness.ts
+4 -1
View File
@@ -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 }
src/renderer/src/store/slices/worktrees-git-identity-review-clear-persistence.test.ts
+9
View File
@@ -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',
src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts
+4 -3
View File
@@ -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<Worktree, 'id' | 'branch'> &
Partial<Pick<Worktree, HostedReviewLinkKey | 'pushTarget' | 'head'>>
Partial<Pick<Worktree, HostedReviewLinkKey | 'pushTarget' | 'head' | 'linkedItems'>>
>(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<Worktree, 'id' | 'branch'> &
Partial<Pick<Worktree, HostedReviewLinkKey | 'pushTarget' | 'head'>>
Partial<Pick<Worktree, HostedReviewLinkKey | 'pushTarget' | 'head' | 'linkedItems'>>
>(worktrees: readonly T[], currentWorktrees?: readonly T[]): T[] {
let changed = false
const sanitized = worktrees.map((worktree) => {
src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts
+4 -2
View File
@@ -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)
: {})
}
})
src/shared/workspace-attachments.test.ts
+21
View File
@@ -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),
src/shared/workspace-attachments.ts
+5
View File
@@ -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,
src/shared/worktree/hosted-review-links.ts
+10 -6
View File
@@ -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<WorktreeMeta, HostedReviewLinkKey> = {
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
}