From e03786fb90f5a8d1b273a25440b6c35ecd2bdec9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 25 Oct 2026 05:59:28 -0700 Subject: [PATCH] fix(worktrees): keep each same-path copy on its own host branch (#33428) * fix(worktrees): one host's git status no longer rewrites a same-path twin's branch (#32918) * fix(worktrees): match a twin through its runtime alias and persist its review clear on the same copy * fix(worktrees): keep the id-only patch for a lone copy; background refreshes refuse an unplaced twin * fix(worktrees): a twin's identity patch skips the id-keyed review clear instead of reaching its sibling * fix(worktrees): count runtime provenance when finding twins; Source Control refresh refuses an unplaced twin * fix(worktrees): a twin's branch switch clears its own push target and review link on its host * fix(worktrees): a twin's review clear keeps its stale-refetch guard and relink check, scoped to its own copy * fix(worktrees): a twin tombstone names its full owner, and detected rows are stamped before sanitizing * fix(worktrees): each twin copy keeps its own review-clear tombstone * fix(worktrees): a rename carries each twin copy's review-clear tombstone * fix(worktrees): a copy's tombstone from its twin days still drives its clear retry * chore(worktrees): name why the existing review-link cast is safe * fix(worktrees): one resolver names the copy for every git-identity read and write * fix(worktrees): a Create PR run refuses when its copy no longer resolves; local null stays local * fix(worktrees): an unselected twin stays unplaced; a legacy row's guard survives its first stamp * fix(worktrees): a Create PR run keeps the copy it pinned at start; a legacy row's relink drops its stamped guard * fix(worktrees): key review clears by the full stamp the next listing gives the row * fix(worktrees): fill only missing stamp parts when keying review clears * fix(worktrees): stamp review-clear keys from repo registrations, not the focused host * fix(worktrees): a local row keeps its review-clear guard when discovery stamps it local * fix(worktrees): stamp review-clear keys from the sole repo registration first * fix(worktrees): a runtime-hosted copy's owner key ignores backfilled provenance * fix(worktrees): Create PR and post-generation refresh read the pinned copy's push target * fix(worktrees): a lone copy's push target still answers when the host index misses * fix(worktrees): the push-target fallback accepts only an unstamped lone copy for a named host * fix(worktrees): only SSH copies borrow a registration's provenance for review-clear keys --- config/workspace-record-read-baseline.txt | 4 +- .../manual-refresh-dependencies.ts | 1 + .../use-checks-panel-controller-state.tsx | 3 + .../use-checks-panel-generation.tsx | 8 +- ...e-checks-panel-git-status-effects.test.tsx | 8 + .../use-checks-panel-git-status-effects.tsx | 24 +- .../use-checks-panel-manual-refresh.test.tsx | 1 + .../use-checks-panel-manual-refresh.tsx | 28 +- ...t-status-refresh-branch-line-total.test.ts | 10 + .../right-sidebar/git-status-refresh.test.ts | 52 ++- .../right-sidebar/git-status-refresh.ts | 47 +- .../git-status-twin-copy.test.ts | 143 ++++++ ...push-target-upstream-refresh-cache.test.ts | 2 + ...urce-control-create-pr-intent-flow.test.ts | 12 +- .../listing/operation-target.ts | 7 +- .../source-control/panel/use-panel-state.ts | 3 +- .../review/create-pr-intent-flow.ts | 6 + .../create-pr-intent-operation-target.test.ts | 22 + .../use-create-pr-intent-jira-title.test.tsx | 1 + .../review/use-create-pr-intent-probes.ts | 1 + .../review/use-create-pr-intent-run.ts | 5 +- .../review/use-create-pr-intent-target.ts | 47 +- .../review/use-pull-request-generation.ts | 8 +- .../review/use-review-context.ts | 3 - .../use-status-refresh.local-ignore.test.tsx | 3 +- .../source-control/sync/use-status-refresh.ts | 40 +- .../right-sidebar/useGitStatusPolling.test.ts | 29 +- .../right-sidebar/useGitStatusPolling.ts | 21 +- .../worktree-head-identity-apply.test.ts | 24 +- .../src/hooks/worktree-head-identity-apply.ts | 14 +- .../src/lib/connection-owner-resolution.ts | 37 +- src/renderer/src/lib/worktree-git-copy.ts | 119 +++++ .../slices/pull-request-generation-state.ts | 3 + .../src/store/slices/worktree-helpers.ts | 4 +- .../worktree-listing-branch-switch.test.ts | 24 +- .../slices/worktree-listing-branch-switch.ts | 21 +- .../worktrees-fetch-listing-merge.test.ts | 2 +- ...orktrees-git-identity-branch-title.test.ts | 168 ++++--- ...-identity-review-clear-persistence.test.ts | 124 +++-- .../worktrees-git-identity-twin-host.test.ts | 427 ++++++++++++++++++ .../worktrees-identity-migration.test.ts | 20 +- .../listing/detected-worktree-host-merge.ts | 5 +- .../metadata/hosted-review-link-mutation.ts | 117 +++-- .../metadata/update-worktree-meta.ts | 2 +- .../metadata/worktree-copy-review-clear.ts | 112 +++++ .../metadata/worktree-git-identity-update.ts | 149 +++++- 46 files changed, 1598 insertions(+), 313 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/git-status-twin-copy.test.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-operation-target.test.ts create mode 100644 src/renderer/src/lib/worktree-git-copy.ts create mode 100644 src/renderer/src/store/slices/worktrees-git-identity-twin-host.test.ts create mode 100644 src/renderer/src/store/slices/worktrees/metadata/worktree-copy-review-clear.ts diff --git a/config/workspace-record-read-baseline.txt b/config/workspace-record-read-baseline.txt index ad41872708a..19ec5df5914 100644 --- a/config/workspace-record-read-baseline.txt +++ b/config/workspace-record-read-baseline.txt @@ -71,10 +71,10 @@ 13 src/renderer/src/store/slices/worktrees/listing/fetched-worktree-merge.ts # store writer 6 src/renderer/src/store/slices/worktrees/listing/known-ssh-worktree-fetch.ts # store writer 3 src/renderer/src/store/slices/worktrees/listing/worktree-sort-order-patch.ts # store writer -6 src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts # store writer +4 src/renderer/src/store/slices/worktrees/metadata/hosted-review-link-mutation.ts # store writer 6 src/renderer/src/store/slices/worktrees/metadata/update-worktree-meta.ts # store writer 8 src/renderer/src/store/slices/worktrees/metadata/update-worktrees-meta.ts # store writer -3 src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts # store writer +2 src/renderer/src/store/slices/worktrees/metadata/worktree-git-identity-update.ts # store writer 1 src/renderer/src/store/slices/worktrees/metadata/worktree-lineage-refresh.ts # store writer 6 src/renderer/src/store/slices/worktrees/session/worktree-activity-bump.ts # store writer 6 src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-row-removal.ts # store writer diff --git a/src/renderer/src/components/right-sidebar/checks-panel/manual-refresh-dependencies.ts b/src/renderer/src/components/right-sidebar/checks-panel/manual-refresh-dependencies.ts index ca26485043c..e64b6465817 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/manual-refresh-dependencies.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel/manual-refresh-dependencies.ts @@ -18,6 +18,7 @@ export type ChecksPanelManualRefreshInput = Pick< | 'fetchPRForBranch' | 'gitStatusSnapshot' | 'ownerTarget' + | 'ownerCopyHostId' | 'panelContextKey' | 'panelContextKeyRef' | 'pollIntervalRef' diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-controller-state.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-controller-state.tsx index 4d43f5f5cfe..ae319a8e87e 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-controller-state.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-controller-state.tsx @@ -1,5 +1,6 @@ import { useCallback, useMemo, useRef, useState, type SetStateAction } from 'react' import { useWorktreeGitOwnerTarget } from '../worktree-git-owner-target' +import { useWorktreeGitCopyHostId } from '@/lib/worktree-git-copy' import { useAppStore, type AppState } from '@/store' import { useActiveWorktree, useRepoById } from '@/store/selectors' import { useChecksPanelTerminalWorktree } from '../use-checks-panel-terminal-worktree' @@ -234,6 +235,7 @@ export function useChecksPanelControllerState() { ) // Route Git by workspace ownership rather than focused server. const ownerTarget = useWorktreeGitOwnerTarget(activeWorktreeId) + const ownerCopyHostId = useWorktreeGitCopyHostId(activeWorktreeId) const repoConnectionId = repo?.connectionId?.trim() || null // Separate local host/WSL distros; remote IDs already scope SSH/runtime contexts. const localExecutionScope = useMemo(() => { @@ -388,6 +390,7 @@ export function useChecksPanelControllerState() { runtimeEnvironmentId, ownerSettings, ownerTarget, + ownerCopyHostId, repoConnectionId, localExecutionScope, sshConnectionStatus, diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-generation.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-generation.tsx index 9aa1f2335f2..1fa90b10775 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-generation.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-generation.tsx @@ -5,6 +5,7 @@ import type { PullRequestGenerationOutcome } from '../create-pull-request-dialog import { settlePullRequestGenerationRequest } from '../pull-request-generation-request-outcome' import { useAppStore } from '@/store' import { getConnectionId } from '@/lib/connection-context' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import { generateRuntimePullRequestFields, cancelRuntimeGeneratePullRequestFields @@ -86,10 +87,13 @@ export function useChecksPanelGeneration(model: ChecksPanelGenerationInput) { return undefined } const requestId = allocatePullRequestGenerationRequestId() + // Why one resolution: the result must land on this copy, not a same-path twin. + const copy = activeWorktreeId ? resolveWorktreeGitCopyNow(activeWorktreeId) : null const context: PullRequestGenerationContext = { worktreeId: activeWorktreeId, worktreePath: activeWorktreePath, - connectionId: getConnectionId(activeWorktreeId) ?? undefined, + connectionId: copy ? copy.connectionId : (getConnectionId(activeWorktreeId) ?? undefined), + ownerHostId: copy?.hostId ?? null, jiraIssueKey: (hostedReviewCreateProvider === 'github' || hostedReviewCreateProvider === 'gitlab') && activeWorktreeId @@ -102,7 +106,7 @@ export function useChecksPanelGeneration(model: ChecksPanelGenerationInput) { requestId, repoId: repo.id, branch, - runtimeTarget: ownerTarget + runtimeTarget: copy?.transport ?? ownerTarget } const seed = { ...fields } const previousRequiresPushBeforeCreate = diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx index d169cec7115..ef80d3bd7cf 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.test.tsx @@ -15,6 +15,14 @@ vi.mock('@/runtime/runtime-git-status-client', () => ({ vi.mock('@/runtime/runtime-git-sync-client', () => ({ getRuntimeGitUpstreamStatus: mocks.getRuntimeGitUpstreamStatus })) +vi.mock('@/lib/worktree-git-copy', () => ({ + resolveWorktreeGitCopyNow: (worktreeId: string) => ({ + worktreeId, + hostId: 'local', + connectionId: undefined, + transport: { kind: 'local' } + }) +})) import { useChecksPanelGitStatusEffects } from './use-checks-panel-git-status-effects' import { deferred, flush, mountProbe, unmountProbes } from '../source-control-hook-test-harness' diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx index 80b18675633..141194a301d 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-git-status-effects.tsx @@ -1,6 +1,7 @@ import { useEffect, useRef } from 'react' import { getRuntimeGitStatus } from '@/runtime/runtime-git-status-client' import { getRuntimeGitUpstreamStatus } from '@/runtime/runtime-git-sync-client' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import { buildChecksPanelEligibilityGitFingerprint, shouldClearChecksPanelGitStatusSnapshot, @@ -157,7 +158,6 @@ export function useChecksPanelGitStatusEffects(model: ChecksPanelGitStatusEffect } let stale = false const requestContextKey = panelContextKey - const connectionId = activeConnectionId ?? undefined // Worktree-keyed global maps only invalidate; fetch an owner-scoped local snapshot. if (gitStatusSnapshotRetryTimerRef.current) { clearTimeout(gitStatusSnapshotRetryTimerRef.current) @@ -166,12 +166,18 @@ export function useChecksPanelGitStatusEffects(model: ChecksPanelGitStatusEffect setGitStatusSnapshot((snapshot) => shouldClearChecksPanelGitStatusSnapshot(snapshot, requestContextKey) ? null : snapshot ) + // Why one resolution: a same-path twin must not get this copy's transport, SSH target or answer. + const copy = resolveWorktreeGitCopyNow(activeWorktreeId) + if (!copy) { + return + } const context = { - target: ownerTarget, + target: copy.transport, worktreeId: activeWorktreeId, worktreePath: activeWorktreePath, - connectionId + connectionId: copy.connectionId } + const ownerHostId = copy.hostId const runRequest = async (): Promise => { try { const status = await getRuntimeGitStatus(context, { admissionTier: 'status' }) @@ -180,10 +186,14 @@ export function useChecksPanelGitStatusEffects(model: ChecksPanelGitStatusEffect shouldCommitChecksPanelGitStatusSnapshot(panelContextKeyRef.current, requestContextKey) ) { // Checks may be the sole Git surface; commit branch identity before upstream refresh can fail. - updateWorktreeGitIdentity(activeWorktreeId, { - head: status.head, - branch: status.branch ?? (status.head ? null : undefined) - }) + updateWorktreeGitIdentity( + activeWorktreeId, + { + head: status.head, + branch: status.branch ?? (status.head ? null : undefined) + }, + ownerHostId + ) } let freshRemoteStatus = status.upstreamStatus if (activeWorktreePushTarget) { diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.test.tsx index 58dff3ad449..dd1ea643dcb 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.test.tsx @@ -74,6 +74,7 @@ describe('useChecksPanelManualRefresh ordering', () => { linkedGiteaPR: null, linkedGitLabMR: null, linkedPR: null, + ownerCopyHostId: null, ownerTarget: { kind: 'local' }, panelContextKey: 'repo-1::worktree-1::main', panelContextKeyRef: { current: 'repo-1::worktree-1::main' }, diff --git a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.tsx b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.tsx index 6a5f177d060..da8785691ce 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel/use-checks-panel-manual-refresh.tsx @@ -4,6 +4,7 @@ import { buildGitHubPRRefreshStateClearToken } from '@/store/github/pr-refresh-s import { refreshHostedReviewCard } from '@/store/slices/hosted-review-card-refresh' import { getRuntimeGitStatus } from '@/runtime/runtime-git-status-client' import { getRuntimeGitUpstreamStatus } from '@/runtime/runtime-git-sync-client' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import { hasChecksPanelGitStatusBranchChanged, readChecksPanelRefreshGitIdentitySnapshot, @@ -40,6 +41,7 @@ export function useChecksPanelManualRefresh(model: ChecksPanelManualRefreshInput linkedGiteaPR, linkedGitLabMR, linkedPR, + ownerCopyHostId, ownerTarget, panelContextKey, panelContextKeyRef, @@ -97,35 +99,40 @@ export function useChecksPanelManualRefresh(model: ChecksPanelManualRefreshInput }) try { if (activeWorktreeId && activeWorktreePath && !isFolder && ownerTarget) { + // Why one resolution: a same-path twin must not get this copy's transport, SSH target or answer. + const copy = resolveWorktreeGitCopyNow(activeWorktreeId, ownerCopyHostId) + const ownerHostId = copy?.hostId ?? null const snapshotIdentity = readChecksPanelRefreshGitIdentitySnapshot({ snapshot: gitStatusSnapshot, contextKey: panelContextKey, currentBranch: branch }) if (snapshotIdentity.kind === 'changed') { - updateWorktreeGitIdentity(activeWorktreeId, { - head: snapshotIdentity.head, - branch: snapshotIdentity.branch - }) + updateWorktreeGitIdentity( + activeWorktreeId, + { head: snapshotIdentity.head, branch: snapshotIdentity.branch }, + ownerHostId + ) // A discovered terminal branch switch must restart branch-keyed effects rather than refresh old PR data. refreshOutcome = 'branch-changed' return } try { const statusContext = { - target: ownerTarget, + target: copy?.transport ?? ownerTarget, worktreeId: activeWorktreeId, worktreePath: activeWorktreePath, - connectionId: activeConnectionId ?? undefined + connectionId: copy ? copy.connectionId : (activeConnectionId ?? undefined) } const status = await getRuntimeGitStatus(statusContext, { admissionTier: 'interactive' }) const observedBranch = status.branch ?? (status.head ? null : undefined) - updateWorktreeGitIdentity(activeWorktreeId, { - head: status.head, - branch: observedBranch - }) + updateWorktreeGitIdentity( + activeWorktreeId, + { head: status.head, branch: observedBranch }, + ownerHostId + ) if ( observedBranch !== undefined && hasChecksPanelGitStatusBranchChanged({ @@ -381,6 +388,7 @@ export function useChecksPanelManualRefresh(model: ChecksPanelManualRefreshInput fetchHostedReviewForBranch, expireGitHubPRRefreshState, isCurrentAsyncResult, + ownerCopyHostId, ownerTarget, updateWorktreeGitIdentity, panelContextKeyRef, diff --git a/src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts b/src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts index 4d836d40b8d..ae302a1430a 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts @@ -53,6 +53,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-hidden', worktreePath: '/repo', connectionId: 'ssh-1', @@ -74,6 +75,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps: makeDeps() @@ -92,6 +94,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-background', worktreePath: '/other-repo', deps: makeDeps() @@ -111,6 +114,7 @@ describe('branch line total request gate on git status refreshes', () => { // The common watcher refresh used to omit options and silently lose the gate. await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', connectionId: 'ssh-1', @@ -131,6 +135,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps: makeDeps(), @@ -153,6 +158,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps: makeDeps(), @@ -173,6 +179,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps: makeDeps() @@ -180,6 +187,7 @@ describe('branch line total request gate on git status refreshes', () => { setBranchLineTotalMergeBase('wt-1', 'rebased-merge-base') await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps: makeDeps() @@ -204,6 +212,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-strict', worktreePath: '/repo', deps: { ...makeDeps(), fetchUpstreamStatus: undefined } @@ -224,6 +233,7 @@ describe('branch line total request gate on git status refreshes', () => { await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-strict', worktreePath: '/repo', deps: { ...makeDeps(), fetchUpstreamStatus: undefined } diff --git a/src/renderer/src/components/right-sidebar/git-status-refresh.test.ts b/src/renderer/src/components/right-sidebar/git-status-refresh.test.ts index 83aabf1817b..45570fce944 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh.test.ts @@ -56,6 +56,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', connectionId: 'ssh-1', @@ -68,10 +69,14 @@ describe('refreshGitStatusForWorktree', () => { admissionTier: 'status' }) expect(deps.setGitStatus).toHaveBeenCalledWith('wt-1', status) - expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith('wt-1', { - head: 'abc123', - branch: 'refs/heads/feature' - }) + expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'wt-1', + { + head: 'abc123', + branch: 'refs/heads/feature' + }, + null + ) expect(deps.setUpstreamStatus).toHaveBeenCalledWith('wt-1', status.upstreamStatus) expect(deps.fetchUpstreamStatus).not.toHaveBeenCalled() }) @@ -93,6 +98,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', deps @@ -118,6 +124,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-2', worktreePath: '/repo', connectionId: 'ssh-2', @@ -125,10 +132,14 @@ describe('refreshGitStatusForWorktree', () => { }) expect(deps.setGitStatus).toHaveBeenCalledWith('wt-2', status) - expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith('wt-2', { - head: 'def456', - branch: 'refs/heads/main' - }) + expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'wt-2', + { + head: 'def456', + branch: 'refs/heads/main' + }, + null + ) expect(deps.setUpstreamStatus).not.toHaveBeenCalled() expect(deps.fetchUpstreamStatus).toHaveBeenCalledWith('wt-2', '/repo', 'ssh-2', undefined, { runtimeTarget: { kind: 'local' }, @@ -148,12 +159,14 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-normal', worktreePath: '/repo', deps }) await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-strict', worktreePath: '/repo', deps @@ -198,6 +211,7 @@ describe('refreshGitStatusForWorktree', () => { const automatic = refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-race', worktreePath: '/repo', deps @@ -206,6 +220,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-race', worktreePath: '/repo', deps @@ -236,6 +251,7 @@ describe('refreshGitStatusForWorktree', () => { const olderRefresh = refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-automatic-race', worktreePath: '/repo', deps @@ -243,6 +259,7 @@ describe('refreshGitStatusForWorktree', () => { await vi.waitFor(() => expect(gitStatus).toHaveBeenCalledTimes(1)) await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-automatic-race', worktreePath: '/repo', deps @@ -269,6 +286,7 @@ describe('refreshGitStatusForWorktree', () => { const olderRefresh = refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-failed-veto', worktreePath: '/repo', deps @@ -277,6 +295,7 @@ describe('refreshGitStatusForWorktree', () => { await expect( refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-failed-veto', worktreePath: '/repo', deps @@ -306,6 +325,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-stale', worktreePath: '/repo', deps, @@ -330,6 +350,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-current', worktreePath: '/repo', deps, @@ -366,6 +387,7 @@ describe('refreshGitStatusForWorktree', () => { const automatic = refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-fetch-race', worktreePath: '/repo', deps @@ -374,6 +396,7 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-fetch-race', worktreePath: '/repo', deps @@ -400,14 +423,19 @@ describe('refreshGitStatusForWorktree', () => { await refreshGitStatusForWorktree({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-detached', worktreePath: '/repo', deps }) - expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith('wt-detached', { - head: 'abc123456789', - branch: null - }) + expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'wt-detached', + { + head: 'abc123456789', + branch: null + }, + null + ) }) }) diff --git a/src/renderer/src/components/right-sidebar/git-status-refresh.ts b/src/renderer/src/components/right-sidebar/git-status-refresh.ts index acc107e4b9d..f4b13d1e781 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh.ts @@ -9,6 +9,7 @@ import { } from './push-target-upstream-refresh-cache' import type { GitStatusResult, GitUpstreamStatus } from '../../../../shared/git-status-types' import type { RuntimeClientTarget } from '../../../../shared/runtime-client-target' +import type { ExecutionHostId } from '../../../../shared/execution-host' import type { GitPushTarget } from '../../../../shared/worktree/types' import { beginAutomaticUpstreamRefresh, @@ -24,7 +25,8 @@ export type GitStatusRefreshDeps = { setGitStatus: (worktreeId: string, status: GitStatusResult) => void updateWorktreeGitIdentity: ( worktreeId: string, - identity: { head?: string; branch?: string | null } + identity: { head?: string; branch?: string | null }, + ownerHostId: ExecutionHostId | null ) => void setUpstreamStatus: (worktreeId: string, status: GitUpstreamStatus) => void fetchUpstreamStatus: ( @@ -99,6 +101,7 @@ export function clearGitStatusRefreshOrderingForTests(): void { export async function refreshGitStatusForWorktree({ target, + ownerHostId, worktreeId, worktreePath, connectionId, @@ -108,6 +111,8 @@ export async function refreshGitStatusForWorktree({ }: { /** The worktree owner's transport. */ target: RuntimeClientTarget + /** The copy `target` answers for, read when the request starts. */ + ownerHostId: ExecutionHostId | null worktreeId: string worktreePath: string connectionId?: string @@ -146,11 +151,15 @@ export async function refreshGitStatusForWorktree({ deps.setGitStatus(worktreeId, status) // Read shell-driven branch changes from git status --branch without another worktree-list poll. - deps.updateWorktreeGitIdentity(worktreeId, { - head: status.head, - // Detached HEAD needs explicit null to clear any stale branch name. - branch: status.branch ?? (status.head ? null : undefined) - }) + deps.updateWorktreeGitIdentity( + worktreeId, + { + head: status.head, + // Detached HEAD needs explicit null to clear any stale branch name. + branch: status.branch ?? (status.head ? null : undefined) + }, + ownerHostId + ) request?.onStatusAccepted?.(status) if (pushTarget) { // Use Orca's explicit publish target for PR-created worktrees rather than Git's configured upstream. @@ -179,7 +188,14 @@ export async function refreshGitStatusForWorktree({ if (upstreamStatus) { // Briefly reuse unchanged automatic comparisons because explicit targets require several Git subprocesses. storeCachedAutomaticPushTargetUpstreamStatus( - { target, worktreeId, worktreePath, connectionId, pushTarget, status }, + { + target, + worktreeId, + worktreePath, + connectionId, + pushTarget, + status + }, upstreamStatus ) } @@ -225,6 +241,7 @@ export async function refreshGitStatusForWorktree({ export async function refreshGitStatusForWorktreeStrict({ target, + ownerHostId, worktreeId, worktreePath, connectionId, @@ -233,6 +250,8 @@ export async function refreshGitStatusForWorktreeStrict({ }: { /** The worktree owner's transport. */ target: RuntimeClientTarget + /** The copy `target` answers for, read when the request starts. */ + ownerHostId: ExecutionHostId | null worktreeId: string worktreePath: string connectionId?: string @@ -264,11 +283,15 @@ export async function refreshGitStatusForWorktreeStrict({ deps.setGitStatus(worktreeId, status) // Read shell-driven branch changes from git status --branch without another worktree-list poll. - deps.updateWorktreeGitIdentity(worktreeId, { - head: status.head, - // Detached HEAD needs explicit null to clear any stale branch name. - branch: status.branch ?? (status.head ? null : undefined) - }) + deps.updateWorktreeGitIdentity( + worktreeId, + { + head: status.head, + // Detached HEAD needs explicit null to clear any stale branch name. + branch: status.branch ?? (status.head ? null : undefined) + }, + ownerHostId + ) if (pushTarget) { // Use Orca's explicit publish target for PR-created worktrees rather than Git's configured upstream. const upstreamStatus = await getRuntimeGitUpstreamStatus( diff --git a/src/renderer/src/components/right-sidebar/git-status-twin-copy.test.ts b/src/renderer/src/components/right-sidebar/git-status-twin-copy.test.ts new file mode 100644 index 00000000000..dcd48c9eb78 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/git-status-twin-copy.test.ts @@ -0,0 +1,143 @@ +// @vitest-environment happy-dom + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type { Worktree } from '../../../../shared/worktree/types' +import type { GitStatusResult } from '../../../../shared/git-status-types' +import { useAppStore } from '@/store' +import { resolveWorktreeGitCopyNow, worktreeCopyPushTarget } from '@/lib/worktree-git-copy' +import { refreshGitStatusForWorktree } from './git-status-refresh' + +// #32918: the same project at the same path on this machine and on an SSH host. +const id = 'repo1::/srv/ai-tools' +const initialAppState = useAppStore.getInitialState() + +function repo(overrides: Partial): Repo { + return { + id: 'repo1', + path: '/srv/ai-tools', + displayName: 'ai-tools', + badgeColor: '#000', + addedAt: 0, + kind: 'git', + ...overrides + } +} + +function copy(overrides: Partial): Worktree { + return { + id, + repoId: 'repo1', + path: '/srv/ai-tools', + head: 'aaa', + branch: 'refs/heads/master', + isBare: false, + isMainWorktree: true, + displayName: 'master', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0, + ...overrides + } +} + +const status = vi.fn<(args: { connectionId?: string }) => Promise>() + +beforeEach(() => { + status.mockResolvedValue({ + entries: [], + head: 'bbb2', + branch: 'refs/heads/feature', + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, + conflictOperation: 'unknown' + }) + vi.stubGlobal('window', { ...window, api: { git: { status } } }) + useAppStore.setState({ + repos: [repo({ connectionId: null }), repo({ connectionId: 'box-b' })], + worktreesByRepo: { + repo1: [ + copy({ hostId: 'local' }), + copy({ + hostId: 'ssh:box-b', + head: 'bbb', + branch: 'refs/heads/feature', + displayName: 'feature' + }) + ] + }, + activeWorktreeId: id, + activeWorkspaceExecutionHostId: 'ssh:box-b' + }) +}) + +afterEach(() => { + useAppStore.setState(initialAppState, true) + vi.unstubAllGlobals() +}) + +describe('git status poll for a same-path twin', () => { + it('refuses an unselected twin instead of guessing a copy', () => { + useAppStore.setState({ activeWorktreeId: null, activeWorkspaceExecutionHostId: null }) + + expect(resolveWorktreeGitCopyNow(id)).toBeNull() + }) + + it("asks the selected SSH copy's host and patches only that copy", async () => { + const target = resolveWorktreeGitCopyNow(id) + expect(target).toMatchObject({ hostId: 'ssh:box-b', connectionId: 'box-b' }) + if (!target) { + return + } + const state = useAppStore.getState() + + await refreshGitStatusForWorktree({ + target: target.transport, + ownerHostId: target.hostId, + worktreeId: id, + worktreePath: '/srv/ai-tools', + connectionId: target.connectionId, + deps: { + setGitStatus: state.setGitStatus, + updateWorktreeGitIdentity: state.updateWorktreeGitIdentity, + setUpstreamStatus: state.setUpstreamStatus, + fetchUpstreamStatus: async () => null + } + }) + + expect(status).toHaveBeenCalledWith(expect.objectContaining({ connectionId: 'box-b' })) + const [local, remote] = useAppStore.getState().worktreesByRepo.repo1 + expect(local).toMatchObject({ head: 'aaa', branch: 'refs/heads/master' }) + expect(remote).toMatchObject({ head: 'bbb2', branch: 'refs/heads/feature' }) + }) +}) + +describe('push target for a same-path twin', () => { + it("names the pinned SSH copy's remote and branch, not the local copy's", () => { + const [local, remote] = useAppStore.getState().worktreesByRepo.repo1 + useAppStore.setState({ + worktreesByRepo: { + repo1: [ + { ...local, pushTarget: { remoteName: 'origin', branchName: 'local-review' } }, + { ...remote, pushTarget: { remoteName: 'fork', branchName: 'remote-review' } } + ] + } + }) + + expect(worktreeCopyPushTarget(useAppStore.getState(), id, 'ssh:box-b')).toEqual({ + remoteName: 'fork', + branchName: 'remote-review' + }) + // An unplaced host must not fall back to either twin, nor to a lone copy stamped elsewhere. + expect(worktreeCopyPushTarget(useAppStore.getState(), id, 'ssh:other')).toBeUndefined() + useAppStore.setState({ + worktreesByRepo: { repo1: [useAppStore.getState().worktreesByRepo.repo1[0]] } + }) + expect(worktreeCopyPushTarget(useAppStore.getState(), id, 'ssh:box-b')).toBeUndefined() + }) +}) diff --git a/src/renderer/src/components/right-sidebar/push-target-upstream-refresh-cache.test.ts b/src/renderer/src/components/right-sidebar/push-target-upstream-refresh-cache.test.ts index 0d07dc7f06e..03bec91aa54 100644 --- a/src/renderer/src/components/right-sidebar/push-target-upstream-refresh-cache.test.ts +++ b/src/renderer/src/components/right-sidebar/push-target-upstream-refresh-cache.test.ts @@ -71,6 +71,7 @@ async function refreshAutomatically(options: { }): Promise { await refreshGitStatusForWorktree({ target: runtimeTargetForOwnerEnvironment(options.runtimeEnvironmentId ?? null), + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', connectionId: options.connectionId, @@ -232,6 +233,7 @@ describe('push-target upstream refresh cache', () => { await refreshAutomatically({ deps }) await refreshGitStatusForWorktreeStrict({ target: { kind: 'local' }, + ownerHostId: null, worktreeId: 'wt-1', worktreePath: '/repo', pushTarget, diff --git a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts index 92147c39183..539e1c844c2 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts @@ -25,7 +25,8 @@ describe('source-control Create PR intent flow helpers', () => { worktreePath: '/repo', branch: 'feature', provider: 'github', - baseRef: 'origin/main' + baseRef: 'origin/main', + pinnedCopy: null }) expect(token.startedAt).toBe(123) @@ -49,7 +50,8 @@ describe('source-control Create PR intent flow helpers', () => { worktreeId: 'wt-1', worktreePath: '/repo', branch: 'feature/pr', - provider: 'github' + provider: 'github', + pinnedCopy: null }) expect(createPrIntentGitStatusMatchesToken(token, { branch: 'refs/heads/feature/pr' })).toBe( @@ -69,7 +71,8 @@ describe('source-control Create PR intent flow helpers', () => { worktreeId: 'wt-1', worktreePath: wt1Path, branch: 'feature/pr', - provider: 'github' + provider: 'github', + pinnedCopy: null }) expect( @@ -99,7 +102,8 @@ describe('source-control Create PR intent flow helpers', () => { worktreePath, branch: 'feature/pr', provider: 'github', - baseRef: 'refs/remotes/origin/main' + baseRef: 'refs/remotes/origin/main', + pinnedCopy: null }) expect( diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/operation-target.ts b/src/renderer/src/components/right-sidebar/source-control/listing/operation-target.ts index 11be99c922c..dbdebbb022d 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/operation-target.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/operation-target.ts @@ -3,6 +3,7 @@ import type { RuntimeClientTarget } from '../../../../../../shared/runtime-clien import { requireGitOwnerTarget } from '../../worktree-git-owner-target' import type { GitConflictOperation } from '../../../../../../shared/git-status-types' import type { GitPushTarget } from '../../../../../../shared/worktree/types' +import type { ExecutionHostId } from '../../../../../../shared/execution-host' export type AbortConflictOperation = Extract @@ -11,6 +12,8 @@ export type SourceControlOperationTarget = Omit & { /** The repo owner's transport; `null` when no repo row names one, which refuses the call. */ target: RuntimeClientTarget | null worktreeId: string + /** The worktree copy `target` answers for; a same-path twin on another host shares the id. */ + ownerHostId?: ExecutionHostId | null pushTarget?: GitPushTarget } @@ -19,5 +22,7 @@ export function operationGitContext( operation: SourceControlOperationTarget, prefs?: RuntimeGitContext['prefs'] ): RuntimeGitContext { - return { ...operation, target: requireGitOwnerTarget(operation.target), prefs } + // Why drop ownerHostId: it names the store copy to patch and is not part of the request. + const { ownerHostId: _ownerHostId, ...request } = operation + return { ...request, target: requireGitOwnerTarget(operation.target), prefs } } diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/use-panel-state.ts b/src/renderer/src/components/right-sidebar/source-control/panel/use-panel-state.ts index 7556bf8579b..6c03dcc3148 100644 --- a/src/renderer/src/components/right-sidebar/source-control/panel/use-panel-state.ts +++ b/src/renderer/src/components/right-sidebar/source-control/panel/use-panel-state.ts @@ -54,8 +54,7 @@ export function useSourceControlPanelState() { isFolder, repositoryHuge, activeConnectionId, - activeWorktreeInstanceId, - worktreeMap + activeWorktreeInstanceId }) return { diff --git a/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-flow.ts b/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-flow.ts index 258ea86141e..93c596e5660 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-flow.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-flow.ts @@ -1,3 +1,4 @@ +import type { WorktreeGitCopy } from '@/lib/worktree-git-copy' import { isBehindOnlyUpstream, shouldForcePushWithLeaseForUpstream @@ -29,6 +30,11 @@ export type CreatePrIntentRunToken = { branch: string provider: HostedReviewProvider baseRef?: string | null + /** + * The worktree copy the run started on, pinned with its transport: a same-path twin on another + * host shares the id, and re-resolving later could land on it. Null refuses every call. + */ + pinnedCopy: WorktreeGitCopy | null startedAt: number } diff --git a/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-operation-target.test.ts b/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-operation-target.test.ts new file mode 100644 index 00000000000..aaac3437b3f --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/review/create-pr-intent-operation-target.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { createPrIntentOperationTarget } from './use-create-pr-intent-target' + +// #32918: the same project at the same path locally and on an SSH host share one worktree id. +describe('createPrIntentOperationTarget', () => { + it("refuses when the run's copy did not resolve, never using the local twin", () => { + const target = createPrIntentOperationTarget( + { + repoId: 'repo1', + worktreeId: 'repo1::/srv/ai-tools', + worktreePath: '/srv/ai-tools', + branch: 'feature', + provider: 'github', + pinnedCopy: null, + startedAt: 0 + }, + undefined + ) + + expect(target).toMatchObject({ target: null, connectionId: undefined, ownerHostId: null }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-jira-title.test.tsx b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-jira-title.test.tsx index b947c0f2bf5..faf270f176c 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-jira-title.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-jira-title.test.tsx @@ -46,6 +46,7 @@ it.each(['github', 'gitlab'] as const)( branch: 'user/alt-42-fix-login', provider, baseRef: null, + pinnedCopy: null, startedAt: 1 } const eligibility: HostedReviewCreationEligibility = { diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-probes.ts b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-probes.ts index 20d5446ff15..80e8ea32603 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-probes.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-probes.ts @@ -151,6 +151,7 @@ export function useSourceControlCreatePrIntentProbes({ return await refreshGitStatusForWorktreeStrict({ // Background completion must check the worktree that started the intent. target: requireGitOwnerTarget(target.target), + ownerHostId: target.ownerHostId ?? null, worktreeId: target.worktreeId, worktreePath: target.worktreePath, connectionId: target.connectionId, diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-run.ts b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-run.ts index bf3ee6d7018..0318ab80636 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-run.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-run.ts @@ -1,6 +1,7 @@ import { useCallback } from 'react' import { translate } from '@/i18n/i18n' import { createCreatePrIntentRunToken } from './create-pr-intent-flow' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import type { SourceControlCommitAction } from '../commit/use-commit-action' import type { SourceControlWorktreeContext } from '../listing/use-worktree-context' import type { SourceControlFileListing } from '../listing/use-file-listing' @@ -107,7 +108,9 @@ export function useSourceControlCreatePrIntentRun({ // Preserve the UI provider so failed eligibility probes can synthesize the right host’s local prep steps. provider: provisionalHostedReviewProvider, // Pin the selected base across asynchronous commit/push steps. - baseRef: effectiveBaseRef ?? null + baseRef: effectiveBaseRef ?? null, + // The run outlives navigation; its git answers stay on the copy it started from. + pinnedCopy: resolveWorktreeGitCopyNow(activeWorktreeId) }) const operationTarget = getCreatePrIntentOperationTarget(token) const snapshot = createCreatePrIntentRunSnapshot({ diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-target.ts b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-target.ts index b57f5fd32f3..d0bd7e61a7b 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-target.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-create-pr-intent-target.ts @@ -1,28 +1,24 @@ import { useCallback, useEffect } from 'react' -import { getConnectionId } from '@/lib/connection-context' +import { useAppStore } from '@/store' +import { worktreeCopyPushTarget } from '@/lib/worktree-git-copy' import type { CreatePrIntentRunToken } from './create-pr-intent-flow' import type { SourceControlOperationTarget } from '../listing/operation-target' -import type { SourceControlWorktreeContext } from '../listing/use-worktree-context' import type { SourceControlWorktreeOperationState } from '../panel/use-worktree-operation-state' /** Track foreground ownership and pin in-flight Git calls to their original host. */ export function useSourceControlCreatePrIntentTarget({ activeRepoId, - activeRepoTarget, activeWorktreeId, branchName, createPrIntentCurrentTargetRef, effectiveBaseRef, - worktreeMap, worktreePath }: { activeRepoId: string | null - activeRepoTarget: SourceControlWorktreeContext['activeRepoTarget'] activeWorktreeId: string | null branchName: string createPrIntentCurrentTargetRef: SourceControlWorktreeOperationState['createPrIntentCurrentTargetRef'] effectiveBaseRef: string | null - worktreeMap: SourceControlWorktreeContext['worktreeMap'] worktreePath: string | null }) { useEffect(() => { @@ -43,19 +39,42 @@ export function useSourceControlCreatePrIntentTarget({ ]) const getCreatePrIntentOperationTarget = useCallback( - (token: CreatePrIntentRunToken): SourceControlOperationTarget => ({ - target: activeRepoTarget, - worktreeId: token.worktreeId, - worktreePath: token.worktreePath, - connectionId: getConnectionId(token.worktreeId) ?? undefined, - pushTarget: worktreeMap.get(token.worktreeId)?.pushTarget - }), - [activeRepoTarget, worktreeMap] + (token: CreatePrIntentRunToken): SourceControlOperationTarget => + createPrIntentOperationTarget( + token, + // Why the pinned copy's row: a same-path twin's push target names another remote and branch. + worktreeCopyPushTarget( + useAppStore.getState(), + token.worktreeId, + token.pinnedCopy?.hostId ?? null + ) + ), + [] ) return { getCreatePrIntentOperationTarget } } +/** + * Pins a Create PR run's calls to the copy and transport captured when it started, never + * re-resolved: a copy that did not resolve gets no transport, so the run refuses instead of + * committing or pushing on a same-path twin's checkout. + */ +export function createPrIntentOperationTarget( + token: CreatePrIntentRunToken, + pushTarget: SourceControlOperationTarget['pushTarget'] +): SourceControlOperationTarget { + const copy = token.pinnedCopy + return { + worktreeId: token.worktreeId, + worktreePath: token.worktreePath, + pushTarget, + target: copy?.transport ?? null, + connectionId: copy?.connectionId, + ownerHostId: copy?.hostId ?? null + } +} + export type SourceControlCreatePrIntentTarget = ReturnType< typeof useSourceControlCreatePrIntentTarget > diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-pull-request-generation.ts b/src/renderer/src/components/right-sidebar/source-control/review/use-pull-request-generation.ts index fe792b62c0a..3bd863bb6e8 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-pull-request-generation.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-pull-request-generation.ts @@ -2,6 +2,7 @@ import { getRepoExecutionHostId } from '../../../../../../shared/execution-host' import { getWorkspaceJiraIssueKey } from '../../../../../../shared/jira/jira-review-title' import { useCallback } from 'react' import { getConnectionId } from '@/lib/connection-context' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import { cancelRuntimeGeneratePullRequestFields, generateRuntimePullRequestFields @@ -101,10 +102,13 @@ export function useSourceControlPullRequestGeneration({ return undefined } const requestId = allocatePullRequestGenerationRequestId() + // Why one resolution: the result must land on this copy, not a same-path twin. + const copy = activeWorktreeId ? resolveWorktreeGitCopyNow(activeWorktreeId) : null const context: PullRequestGenerationContext = { worktreeId: activeWorktreeId, worktreePath, - connectionId: getConnectionId(activeWorktreeId) ?? undefined, + connectionId: copy ? copy.connectionId : (getConnectionId(activeWorktreeId) ?? undefined), + ownerHostId: copy?.hostId ?? null, jiraIssueKey: (hostedReviewCreateProvider === 'github' || hostedReviewCreateProvider === 'gitlab') && activeWorktreeId @@ -117,7 +121,7 @@ export function useSourceControlPullRequestGeneration({ requestId, repoId: activeRepo.id, branch: branchName, - runtimeTarget: activeRepoTarget + runtimeTarget: copy?.transport ?? activeRepoTarget } const seed = { ...fields } setPullRequestGenerationRecord( diff --git a/src/renderer/src/components/right-sidebar/source-control/review/use-review-context.ts b/src/renderer/src/components/right-sidebar/source-control/review/use-review-context.ts index eda65873ea7..4d74fde07a4 100644 --- a/src/renderer/src/components/right-sidebar/source-control/review/use-review-context.ts +++ b/src/renderer/src/components/right-sidebar/source-control/review/use-review-context.ts @@ -29,7 +29,6 @@ export function useSourceControlReviewContext(panelState: SourceControlPanelStat isFolder, remoteStatus, settings, - worktreeMap, worktreePath } = panelState @@ -77,12 +76,10 @@ export function useSourceControlReviewContext(panelState: SourceControlPanelStat }) const createPrIntentTarget = useSourceControlCreatePrIntentTarget({ activeRepoId, - activeRepoTarget, activeWorktreeId, branchName, createPrIntentCurrentTargetRef, effectiveBaseRef, - worktreeMap, worktreePath }) const linkedReviews = useSourceControlLinkedReviews({ diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.local-ignore.test.tsx b/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.local-ignore.test.tsx index 9d185ab8287..62b84c90c6b 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.local-ignore.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.local-ignore.test.tsx @@ -59,8 +59,7 @@ async function renderOwner( worktreePath: '/same/path', isFolder: false, repositoryHuge: { limit: 1000 }, - activeConnectionId: connectionId, - worktreeMap: new Map() + activeConnectionId: connectionId }) return null } diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.ts index 95f21f17a62..aa4ec1f84ae 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-status-refresh.ts @@ -1,7 +1,11 @@ import { useCallback, useEffect } from 'react' import type { RuntimeClientTarget } from '../../../../../../shared/runtime-client-target' import { toast } from 'sonner' -import { getConnectionId } from '@/lib/connection-context' +import { + resolveWorktreeGitCopyNow, + useWorktreeGitCopyHostId, + worktreeCopyPushTarget +} from '@/lib/worktree-git-copy' import { getLocalPathOpenOwnerForRoute, isLocalPathOpenBlocked } from '@/lib/local-path-open-guard' import { beginHugeRepoWarningProbe, @@ -30,8 +34,7 @@ export function useSourceControlStatusRefresh({ isFolder, repositoryHuge, activeConnectionId, - activeWorktreeInstanceId, - worktreeMap + activeWorktreeInstanceId }: { activeRepoTarget: RuntimeClientTarget | null activeWorktreeId: string | null @@ -41,12 +44,12 @@ export function useSourceControlStatusRefresh({ repositoryHuge: { limit: number } | null | undefined activeConnectionId: string | null activeWorktreeInstanceId?: string - worktreeMap: ReadonlyMap }): SourceControlStatusRefresh { const setGitStatus = useAppStore((s) => s.setGitStatus) const updateWorktreeGitIdentity = useAppStore((s) => s.updateWorktreeGitIdentity) const setUpstreamStatus = useAppStore((s) => s.setUpstreamStatus) const fetchUpstreamStatus = useAppStore((s) => s.fetchUpstreamStatus) + const activeCopyHostId = useWorktreeGitCopyHostId(activeWorktreeId) const localIgnoreBlocked = isLocalPathOpenBlocked( getLocalPathOpenOwnerForRoute({ runtimeEnvironmentId: @@ -59,13 +62,17 @@ export function useSourceControlStatusRefresh({ if (!activeWorktreeId || !worktreePath || isFolder || !activeRepoTarget) { return } - const connectionId = getConnectionId(activeWorktreeId) ?? undefined + // Why the captured host: a mutation callback can outlive a switch to a same-path twin. + const copy = resolveWorktreeGitCopyNow(activeWorktreeId, activeCopyHostId) + if (!copy) { + return + } await refreshGitStatusForWorktree({ - // Read status on the repo owner, which may differ from the focused host. - target: activeRepoTarget, + target: copy.transport, + ownerHostId: copy.hostId, worktreeId: activeWorktreeId, worktreePath, - connectionId, + connectionId: copy.connectionId, pushTarget: activePushTarget, deps: { setGitStatus, @@ -77,6 +84,7 @@ export function useSourceControlStatusRefresh({ }) }, [ + activeCopyHostId, activeRepoTarget, activeWorktreeId, activePushTarget, @@ -167,10 +175,15 @@ export function useSourceControlStatusRefresh({ await refreshGitStatusForWorktree({ // Generation may finish after host navigation; refresh its original owner. target: context.runtimeTarget, + ownerHostId: context.ownerHostId ?? null, worktreeId: context.worktreeId, worktreePath: context.worktreePath, connectionId: context.connectionId, - pushTarget: worktreeMap.get(context.worktreeId)?.pushTarget, + pushTarget: worktreeCopyPushTarget( + useAppStore.getState(), + context.worktreeId, + context.ownerHostId ?? null + ), deps: { setGitStatus, updateWorktreeGitIdentity, @@ -182,14 +195,7 @@ export function useSourceControlStatusRefresh({ console.warn('[SourceControl] post-generation git status refresh failed', error) } }, - [ - fetchUpstreamStatus, - isFolder, - setGitStatus, - setUpstreamStatus, - updateWorktreeGitIdentity, - worktreeMap - ] + [fetchUpstreamStatus, isFolder, setGitStatus, setUpstreamStatus, updateWorktreeGitIdentity] ) return { diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.test.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.test.ts index 94a4f28b255..e1f8c6c4667 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.test.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.test.ts @@ -12,6 +12,11 @@ import { const worktree = { id: 'repo-1::/repo', repoId: 'repo-1', path: '/repo' } const repo = { id: 'repo-1', path: '/repo', kind: 'git', connectionId: null as string | null } +const LOCAL = { kind: 'local' } as const +const localGitOwnerModule = () => ({ getWorktreeGitOwnerTarget: () => LOCAL }) +const gitCopyModule = (connectionId?: string) => () => ({ + resolveWorktreeGitCopyNow: (id: string) => ({ worktreeId: id, transport: LOCAL, connectionId }) +}) type PollState = { activeWorktreeId: string @@ -94,9 +99,7 @@ async function usePollingOnce( } }) - vi.doMock('./worktree-git-owner-target', () => ({ - getWorktreeGitOwnerTarget: () => ({ kind: 'local' }) - })) + vi.doMock('./worktree-git-owner-target', localGitOwnerModule) vi.doMock('@/store', () => ({ useAppStore: Object.assign((selector: (s: PollState) => unknown) => selector(state), { getState: () => ({ settings: null }) @@ -116,6 +119,7 @@ async function usePollingOnce( vi.doMock('@/lib/connection-context', () => ({ getConnectionId: () => options.connectionId ?? undefined })) + vi.doMock('@/lib/worktree-git-copy', gitCopyModule(options.connectionId ?? undefined)) vi.stubGlobal('window', { api: { @@ -378,9 +382,7 @@ describe('useGitStatusPolling', () => { useRef: (initial: T) => ({ current: initial }) } }) - vi.doMock('./worktree-git-owner-target', () => ({ - getWorktreeGitOwnerTarget: () => ({ kind: 'local' }) - })) + vi.doMock('./worktree-git-owner-target', localGitOwnerModule) vi.doMock('@/store', () => ({ useAppStore: Object.assign((selector: (s: PollState) => unknown) => selector(state), { getState: () => ({ settings: null }) @@ -394,6 +396,7 @@ describe('useGitStatusPolling', () => { useRepoMap: () => new Map([[repo.id, repo]]) })) vi.doMock('@/lib/connection-context', () => ({ getConnectionId: () => undefined })) + vi.doMock('@/lib/worktree-git-copy', gitCopyModule()) vi.stubGlobal('window', { api: { git: { status: gitStatus }, @@ -494,9 +497,7 @@ describe('useGitStatusPolling', () => { useRef: (initial: T) => ({ current: initial }) } }) - vi.doMock('./worktree-git-owner-target', () => ({ - getWorktreeGitOwnerTarget: () => ({ kind: 'local' }) - })) + vi.doMock('./worktree-git-owner-target', localGitOwnerModule) vi.doMock('@/store', () => ({ useAppStore: Object.assign((selector: (s: PollState) => unknown) => selector(state), { getState: () => ({ settings: null }) @@ -510,6 +511,7 @@ describe('useGitStatusPolling', () => { useRepoMap: () => new Map([[repo.id, repo]]) })) vi.doMock('@/lib/connection-context', () => ({ getConnectionId: () => undefined })) + vi.doMock('@/lib/worktree-git-copy', gitCopyModule()) vi.stubGlobal('window', { api: { git: { status: gitStatus }, @@ -621,9 +623,7 @@ describe('useGitStatusPolling', () => { } } }) - vi.doMock('./worktree-git-owner-target', () => ({ - getWorktreeGitOwnerTarget: () => ({ kind: 'local' }) - })) + vi.doMock('./worktree-git-owner-target', localGitOwnerModule) vi.doMock('@/store', () => ({ useAppStore: Object.assign((selector: (s: PollState) => unknown) => selector(state), { getState: () => ({ settings: null }) @@ -637,6 +637,7 @@ describe('useGitStatusPolling', () => { useRepoMap: () => new Map([[repo.id, repo]]) })) vi.doMock('@/lib/connection-context', () => ({ getConnectionId: () => undefined })) + vi.doMock('@/lib/worktree-git-copy', gitCopyModule()) vi.stubGlobal('window', { api: { git: { status: gitStatus }, @@ -804,9 +805,7 @@ describe('useGitStatusPolling', () => { } }) - vi.doMock('./worktree-git-owner-target', () => ({ - getWorktreeGitOwnerTarget: () => ({ kind: 'local' }) - })) + vi.doMock('./worktree-git-owner-target', localGitOwnerModule) vi.doMock('@/store', () => ({ useAppStore: Object.assign((selector: (s: PollState) => unknown) => selector(state), { getState: () => ({ settings: null }) diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts index 7329d31a55e..a97fbc39fb4 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts @@ -2,7 +2,6 @@ import { useCallback, useEffect, useRef } from 'react' import { useAppStore } from '@/store' import { useAllWorktrees, useRepoById, useRepoMap, useWorktreeById } from '@/store/selectors' import { isGitRepoKind } from '../../../../shared/repo-kind' -import { getConnectionId } from '@/lib/connection-context' import { getExecutionHostIdForWorktree } from '@/lib/worktree-runtime-owner' import { refreshGitStatusForWorktree } from './git-status-refresh' import { isWindowVisible } from '@/lib/window-visibility-interval' @@ -10,7 +9,7 @@ import { hasInteractiveActiveGitStatusConsumer, shouldPollActiveGitStatus } from '@/lib/passive-macos-app-data-access' -import { getWorktreeGitOwnerTarget } from './worktree-git-owner-target' +import { resolveWorktreeGitCopyNow } from '@/lib/worktree-git-copy' import { useGitStatusFileWatchRefresh } from './git-status-file-watch-refresh' import { useGitStatusPushSignalRefresh } from './git-status-push-signal-refresh' import { useStaleConflictOperationPolling } from './stale-conflict-operation-poll' @@ -127,23 +126,19 @@ export function useGitStatusPolling(options: { enabled?: boolean } = {}): void { return } try { - const connectionId = getConnectionId(activeWorktreeId) ?? undefined - const target = getWorktreeGitOwnerTarget(activeWorktreeId) - if (!target) { + // Why one resolution: a same-path twin must not get this copy's transport, SSH target or answer. + const copy = resolveWorktreeGitCopyNow(activeWorktreeId) + if (!copy) { return } await refreshGitStatusForWorktree({ - target, + target: copy.transport, + ownerHostId: copy.hostId, worktreeId: activeWorktreeId, worktreePath, - connectionId, + connectionId: copy.connectionId, pushTarget: activePushTarget, - deps: { - setGitStatus, - updateWorktreeGitIdentity, - setUpstreamStatus, - fetchUpstreamStatus - }, + deps: { setGitStatus, updateWorktreeGitIdentity, setUpstreamStatus, fetchUpstreamStatus }, request: { admissionTier: admissionTierForGitStatusRefreshReason(request.reason), ...(request.reason === 'safety' ? { reuseLineStats: true } : {}), diff --git a/src/renderer/src/hooks/worktree-head-identity-apply.test.ts b/src/renderer/src/hooks/worktree-head-identity-apply.test.ts index 35028c5fa7f..34002f3fc3e 100644 --- a/src/renderer/src/hooks/worktree-head-identity-apply.test.ts +++ b/src/renderer/src/hooks/worktree-head-identity-apply.test.ts @@ -30,10 +30,14 @@ describe('applyWorktreeHeadIdentities', () => { } ) expect(updateWorktreeGitIdentity).toHaveBeenCalledTimes(1) - expect(updateWorktreeGitIdentity).toHaveBeenCalledWith('repo-1::/repos/project/wt-a', { - head: 'bbb222', - branch: 'refs/heads/feature' - }) + expect(updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'repo-1::/repos/project/wt-a', + { + head: 'bbb222', + branch: 'refs/heads/feature' + }, + 'local' + ) }) it('matches Windows-flavored paths that differ only by separators and casing', () => { @@ -50,10 +54,14 @@ describe('applyWorktreeHeadIdentities', () => { updateWorktreeGitIdentity } ) - expect(updateWorktreeGitIdentity).toHaveBeenCalledWith('repo-1::C:/repos/project/wt-a', { - head: 'bbb222', - branch: null - }) + expect(updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'repo-1::C:/repos/project/wt-a', + { + head: 'bbb222', + branch: null + }, + 'local' + ) }) it('skips identities whose worktree row does not exist', () => { diff --git a/src/renderer/src/hooks/worktree-head-identity-apply.ts b/src/renderer/src/hooks/worktree-head-identity-apply.ts index 64f411220b0..ba0293ece6e 100644 --- a/src/renderer/src/hooks/worktree-head-identity-apply.ts +++ b/src/renderer/src/hooks/worktree-head-identity-apply.ts @@ -1,11 +1,13 @@ import type { Worktree, WorktreeHeadIdentity } from '../../../shared/worktree/types' +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../../shared/execution-host' import { normalizeRuntimePathForComparison } from '../../../shared/cross-platform-path' export type WorktreeHeadIdentityApplyDeps = { getWorktreesForRepo: (repoId: string) => Worktree[] | undefined updateWorktreeGitIdentity: ( worktreeId: string, - identity: { head?: string; branch?: string | null } + identity: { head?: string; branch?: string | null }, + ownerHostId: ExecutionHostId | null ) => void } @@ -35,9 +37,11 @@ export function applyWorktreeHeadIdentities( if (!worktree) { continue } - deps.updateWorktreeGitIdentity(worktree.id, { - head: identity.head, - branch: identity.branch - }) + // Why: this watcher reads this machine's checkouts, never a same-path twin on another host. + deps.updateWorktreeGitIdentity( + worktree.id, + { head: identity.head, branch: identity.branch }, + LOCAL_EXECUTION_HOST_ID + ) } } diff --git a/src/renderer/src/lib/connection-owner-resolution.ts b/src/renderer/src/lib/connection-owner-resolution.ts index 918f15fc86b..d7546ab0278 100644 --- a/src/renderer/src/lib/connection-owner-resolution.ts +++ b/src/renderer/src/lib/connection-owner-resolution.ts @@ -17,7 +17,13 @@ import { getFolderWorkspaceCandidateRepos, getFolderWorkspaceConnectionId } from './folder-workspace-connection' -import { selectFolderWorkspaces, selectWorktreesByRepo } from '@/store/workspace-record-selectors' +import { + selectFolderWorkspaces, + selectWorkspaceByHostTarget, + selectWorktreesByRepo +} from '@/store/workspace-record-selectors' +import type { ExecutionHostId } from '../../../shared/execution-host' +import { hostWorktreeTarget } from '../../../shared/worktree/host-qualified-identity' type ConnectionOwnerState = Pick< AppState, @@ -128,3 +134,32 @@ export function getConnectionIdForFileFromState( ) return connectionIds.size === 1 ? ([...connectionIds][0] ?? null) : undefined } + +/** + * The SSH connectionId of one host's copy of `worktreeId`. Ids repeat across hosts, so the bare-id + * lookup above answers `undefined` for a same-path twin; naming the copy's host resolves it. + */ +export function getConnectionIdForWorktreeCopy( + state: ConnectionOwnerState, + worktreeId: string, + hostId: ExecutionHostId +): string | null | undefined { + const row = selectWorkspaceByHostTarget( + { + worktreesByRepo: selectWorktreesByRepo(state), + folderWorkspaces: selectFolderWorkspaces(state) + }, + hostWorktreeTarget({ id: worktreeId, hostId }) + ) + if (!row) { + return undefined + } + const resolution = resolveWorktreeExecutionHost( + { + byId: (id) => resolveIndexedRepoOwner(state.repos, id), + byHost: (id, repoHostId) => findIndexedRepoOwnerForHost(state.repos, id, repoHostId) + }, + { repoId: row.repoId, hostId: row.hostId ?? hostId } + ) + return resolution.kind === 'resolved' ? resolution.connectionId : undefined +} diff --git a/src/renderer/src/lib/worktree-git-copy.ts b/src/renderer/src/lib/worktree-git-copy.ts new file mode 100644 index 00000000000..b633b0de364 --- /dev/null +++ b/src/renderer/src/lib/worktree-git-copy.ts @@ -0,0 +1,119 @@ +import { useAppStore } from '@/store' +import type { AppState } from '@/store/types' +import type { RuntimeClientTarget } from '../../../shared/runtime-client-target' +import { parseRoutableExecutionHostId, type ExecutionHostId } from '../../../shared/execution-host' +import { getKnownExecutionHostIdForWorktree } from './worktree-runtime-owner' +import { getActiveWorkspaceExecutionHostId } from './active-workspace-host' +import { + selectWorkspaceByHostTarget, + selectWorktreesByRepo +} from '@/store/workspace-record-selectors' +import { hostWorktreeTarget } from '../../../shared/worktree/host-qualified-identity' +import { getRepoIdFromWorktreeId } from '../../../shared/worktree/id' +import type { GitPushTarget } from '../../../shared/worktree/types' +import { worktreeCopyOwnerKey } from '@/store/slices/worktrees/metadata/worktree-copy-review-clear' +import { runtimeTargetForWorkspaceOwner } from './resolve-owner' +import { + getConnectionIdForWorktreeCopy, + getConnectionIdFromState +} from './connection-owner-resolution' + +/** + * One host's copy of a worktree, for git work whose answer is written back to the store. Worktree + * ids are `repoId::path` and repeat across hosts, so the transport, the SSH target and the copy + * the answer lands on must all come from this one resolution. + */ +export type WorktreeGitCopy = { + readonly worktreeId: string + /** The copy's host; null only when no row places it, which leaves a same-id twin untouched. */ + readonly hostId: ExecutionHostId | null + readonly connectionId: string | undefined + readonly transport: RuntimeClientTarget +} + +/** + * The selected copy's host, else the owner's when the unqualified owner resolves. An ambiguous twin + * with no selection stays unplaced, so its git calls refuse instead of guessing a copy. + */ +function placedHostId(state: AppState, worktreeId: string): ExecutionHostId | null { + const selected = getActiveWorkspaceExecutionHostId(state, worktreeId) + if (selected) { + return parseRoutableExecutionHostId(selected)?.id ?? null + } + if (!runtimeTargetForWorkspaceOwner(state, { workspaceId: worktreeId })) { + return null + } + return ( + parseRoutableExecutionHostId(getKnownExecutionHostIdForWorktree(state, worktreeId))?.id ?? null + ) +} + +/** + * Resolves `worktreeId` on `hostId`, or on the selected copy when no host is named. Null when the + * owner's transport is unknown, so callers skip the call instead of reading the path locally. + */ +export function resolveWorktreeGitCopy( + state: AppState, + worktreeId: string, + hostId?: ExecutionHostId | null +): WorktreeGitCopy | null { + const copyHostId = hostId ?? placedHostId(state, worktreeId) + const transport = runtimeTargetForWorkspaceOwner(state, { + workspaceId: worktreeId, + ...(copyHostId ? { hostId: copyHostId } : {}) + }) + if (!transport) { + return null + } + // Why the copy lookup: the bare-id one answers undefined for a twin, which a caller would + // otherwise turn into a local `git status` of the SSH copy's path. + // A resolved null (local) is an answer; only "unknown" falls through to the copy lookup. + const bareConnectionId = getConnectionIdFromState(state, worktreeId) + const connectionId = + bareConnectionId === undefined && copyHostId + ? getConnectionIdForWorktreeCopy(state, worktreeId, copyHostId) + : bareConnectionId + return { + worktreeId, + hostId: copyHostId, + connectionId: connectionId ?? undefined, + transport + } +} + +/** {@link resolveWorktreeGitCopy} against the live store. */ +export function resolveWorktreeGitCopyNow( + worktreeId: string, + hostId?: ExecutionHostId | null +): WorktreeGitCopy | null { + return resolveWorktreeGitCopy(useAppStore.getState(), worktreeId, hostId) +} + +/** The selected copy's host, for a callback that must keep answering for it after a switch. */ +export function useWorktreeGitCopyHostId(worktreeId: string | null): ExecutionHostId | null { + return useAppStore((state) => (worktreeId ? placedHostId(state, worktreeId) : null)) +} + +/** + * The publish target of one copy. Ids repeat across hosts, so an id-keyed lookup could hand a Create + * PR or refresh the other copy's remote and branch; with no host, only a lone copy answers. + */ +export function worktreeCopyPushTarget( + state: AppState, + worktreeId: string, + hostId: ExecutionHostId | null +): GitPushTarget | undefined { + const placed = hostId + ? selectWorkspaceByHostTarget(state, hostWorktreeTarget({ id: worktreeId, hostId })) + : undefined + if (placed) { + return placed.pushTarget + } + // Why the lone-copy fallback: the host index files an unstamped row only under `local`. A row + // stamped for another host is never this copy, so a named host accepts only an unstamped one. + const rows = (selectWorktreesByRepo(state)[getRepoIdFromWorktreeId(worktreeId)] ?? []).filter( + (row) => row.id === worktreeId + ) + const lone = new Set(rows.map(worktreeCopyOwnerKey)).size === 1 ? rows[0] : undefined + return lone && (!hostId || lone.hostId === undefined) ? lone.pushTarget : undefined +} diff --git a/src/renderer/src/store/slices/pull-request-generation-state.ts b/src/renderer/src/store/slices/pull-request-generation-state.ts index a54a2f175f8..e5b591f3830 100644 --- a/src/renderer/src/store/slices/pull-request-generation-state.ts +++ b/src/renderer/src/store/slices/pull-request-generation-state.ts @@ -1,4 +1,5 @@ import type { RuntimeClientTarget } from '../../../../shared/runtime-client-target' +import type { ExecutionHostId } from '../../../../shared/execution-host' export type PullRequestFieldName = 'base' | 'title' | 'body' | 'draft' export type PullRequestFieldRevisions = Record @@ -20,6 +21,8 @@ export type PullRequestGenerationContext = { jiraIssueKey?: string | null /** The worktree owner's transport, captured when the request started. */ runtimeTarget: RuntimeClientTarget + /** The copy `runtimeTarget` answers for, so a same-path twin isn't patched with its result. */ + ownerHostId?: ExecutionHostId | null } export type PullRequestGenerationStatus = 'idle' | 'running' | 'canceled' | 'failed' | 'succeeded' diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index 3261ec73a79..29ab5ad73c8 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -289,9 +289,11 @@ export type WorktreeSlice = { purgeStaleRuntimeHostState: (removedEnvironmentIds: Iterable) => void /** Move worktree-scoped maps/pointers on path-derived ID rename, preserving live tabs/selection; equal IDs are inert. */ migrateWorktreeIdentity: (oldWorktreeId: string, newWorktreeId: string) => void + /** `ownerHostId` names the copy git answered for; null patches only an id no twin shares. */ updateWorktreeGitIdentity: ( worktreeId: string, - identity: { head?: string; branch?: string | null } + identity: { head?: string; branch?: string | null }, + ownerHostId: ExecutionHostId | null ) => void updateWorktreeBaseStatus: (event: WorktreeBaseStatusEvent) => void updateWorktreeRemoteBranchConflict: (event: WorktreeRemoteBranchConflictEvent) => void diff --git a/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts b/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts index 281fa019e9e..31d84cebccc 100644 --- a/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts +++ b/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts @@ -50,10 +50,14 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { }) expect(updateWorktreeGitIdentity).toHaveBeenCalledTimes(1) - expect(updateWorktreeGitIdentity).toHaveBeenCalledWith('repo1::/path/wt1', { - head: 'def456', - branch: 'refs/heads/feature-two' - }) + expect(updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'repo1::/path/wt1', + { + head: 'def456', + branch: 'refs/heads/feature-two' + }, + null + ) }) it('does nothing when the branch is unchanged', () => { @@ -156,10 +160,14 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { updateWorktreeGitIdentity }) - expect(updateWorktreeGitIdentity).toHaveBeenCalledWith('repo1::/path/wt1', { - head: 'def456', - branch: null - }) + expect(updateWorktreeGitIdentity).toHaveBeenCalledWith( + 'repo1::/path/wt1', + { + head: 'def456', + branch: null + }, + null + ) }) it('rejects a listing response when the branch changed again after the request started', () => { diff --git a/src/renderer/src/store/slices/worktree-listing-branch-switch.ts b/src/renderer/src/store/slices/worktree-listing-branch-switch.ts index 4cfa2982c74..c3969f86675 100644 --- a/src/renderer/src/store/slices/worktree-listing-branch-switch.ts +++ b/src/renderer/src/store/slices/worktree-listing-branch-switch.ts @@ -1,4 +1,5 @@ import type { Worktree } from '../../../../shared/worktree/types' +import type { ExecutionHostId } from '../../../../shared/execution-host' function indexUnambiguousWorktrees( worktrees: readonly Worktree[], @@ -35,7 +36,8 @@ export function routeListingBranchSwitchesThroughGitIdentity(args: { hasBranchScopedReviewContext: (worktree: Worktree) => boolean updateWorktreeGitIdentity: ( worktreeId: string, - identity: { head?: string; branch?: string | null } + identity: { head?: string; branch?: string | null }, + ownerHostId: ExecutionHostId | null ) => void }): Worktree[] { const { @@ -80,12 +82,17 @@ export function routeListingBranchSwitchesThroughGitIdentity(args: { ) { continue } - updateWorktreeGitIdentity(worktree.id, { - head: worktree.head, - // Empty branch means detached HEAD in listing results; null is the - // explicit detached signal expected by updateWorktreeGitIdentity. - branch: worktree.branch === '' ? null : worktree.branch - }) + updateWorktreeGitIdentity( + worktree.id, + { + head: worktree.head, + // Empty branch means detached HEAD in listing results; null is the + // explicit detached signal expected by updateWorktreeGitIdentity. + branch: worktree.branch === '' ? null : worktree.branch + }, + // Why null: ambiguous ids were skipped above, so this id has one copy. + null + ) } return reconciled ?? incoming } diff --git a/src/renderer/src/store/slices/worktrees-fetch-listing-merge.test.ts b/src/renderer/src/store/slices/worktrees-fetch-listing-merge.test.ts index 40308199560..eeff53ebfbf 100644 --- a/src/renderer/src/store/slices/worktrees-fetch-listing-merge.test.ts +++ b/src/renderer/src/store/slices/worktrees-fetch-listing-merge.test.ts @@ -416,7 +416,7 @@ describe('fetchWorktrees', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) await store.getState().fetchWorktrees('repo1') - store.getState().updateWorktreeGitIdentity(existing.id, { branch: 'refs/heads/next' }) + store.getState().updateWorktreeGitIdentity(existing.id, { branch: 'refs/heads/next' }, null) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ displayName: 'feature', diff --git a/src/renderer/src/store/slices/worktrees-git-identity-branch-title.test.ts b/src/renderer/src/store/slices/worktrees-git-identity-branch-title.test.ts index 0ff8fbab2b9..2841feba4d9 100644 --- a/src/renderer/src/store/slices/worktrees-git-identity-branch-title.test.ts +++ b/src/renderer/src/store/slices/worktrees-git-identity-branch-title.test.ts @@ -46,10 +46,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] }, sortEpoch: 3 } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: 'refs/heads/feature' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: 'refs/heads/feature' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ head: 'new-head', @@ -74,14 +78,22 @@ describe('updateWorktreeGitIdentity', () => { notifications += 1 }) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'old-head', - branch: 'refs/heads/main' - }) - store.getState().updateWorktreeGitIdentity('repo1::/path/missing', { - head: 'new-head', - branch: 'refs/heads/feature' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'old-head', + branch: 'refs/heads/main' + }, + null + ) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/missing', + { + head: 'new-head', + branch: 'refs/heads/feature' + }, + null + ) unsubscribe() expect(notifications).toBe(0) @@ -105,9 +117,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: 'refs/heads/stack/two', @@ -133,9 +149,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/one' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/one' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: 'refs/heads/stack/one', @@ -159,10 +179,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: 'refs/heads/stack/one' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: 'refs/heads/stack/one' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ head: 'new-head', @@ -184,9 +208,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0].displayName).toBe('main') }) @@ -203,9 +231,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0].displayName).toBe('My Cool Work') }) @@ -222,9 +254,13 @@ describe('updateWorktreeGitIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0].displayName).toBe('feature') }) @@ -241,9 +277,13 @@ describe('updateWorktreeGitIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0].displayName).toBe('feature') }) @@ -261,10 +301,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] }, sortEpoch: 3 } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: null - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: null + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ head: 'new-head', @@ -287,10 +331,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: null - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: null + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: '', @@ -311,14 +359,22 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'detached-head', - branch: null - }) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'reattached-head', - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'detached-head', + branch: null + }, + null + ) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'reattached-head', + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: 'refs/heads/main', @@ -339,10 +395,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'reattached-head', - branch: 'refs/heads/main' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'reattached-head', + branch: 'refs/heads/main' + }, + null + ) expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ branch: 'refs/heads/main', 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 438a814da6c..949ba401844 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 @@ -48,9 +48,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await Promise.resolve() expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({ @@ -100,9 +104,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await Promise.resolve() await store.getState().updateWorktreeMeta('repo1::/path/wt1', { linkedGitLabMR: 202 }) resolveClearPersist() @@ -148,9 +156,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await Promise.resolve() await store.getState().updateWorktreeMeta('repo1::/path/wt1', { pushTarget: nextPushTarget }) resolveClearPersist() @@ -196,13 +208,21 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await Promise.resolve() - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/three' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/three' + }, + null + ) releaseFirstClear() await vi.waitFor(() => { @@ -243,9 +263,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await Promise.resolve() const switched = store.getState().worktreesByRepo.repo1[0] store.setState({ @@ -295,9 +319,13 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) const switched = store.getState().worktreesByRepo.repo1[0] store.setState({ worktreesByRepo: { @@ -353,10 +381,14 @@ describe('updateWorktreeGitIdentity', () => { store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: 'refs/heads/stack/two' + }, + null + ) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: 'repo1::/path/wt1', @@ -405,9 +437,13 @@ describe('updateWorktreeGitIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: 'repo1::/path/wt1', @@ -415,9 +451,13 @@ describe('updateWorktreeGitIdentity', () => { }) }) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/one' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/one' + }, + null + ) worktreeListMock.mockResolvedValue([laterBranch]) await store.getState().fetchWorktrees('repo1') @@ -455,10 +495,14 @@ describe('updateWorktreeGitIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - head: 'new-head', - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + head: 'new-head', + branch: 'refs/heads/stack/two' + }, + null + ) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: 'repo1::/path/wt1', @@ -497,9 +541,13 @@ describe('updateWorktreeGitIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial) - store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', { - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + 'repo1::/path/wt1', + { + branch: 'refs/heads/stack/two' + }, + null + ) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: 'repo1::/path/wt1', diff --git a/src/renderer/src/store/slices/worktrees-git-identity-twin-host.test.ts b/src/renderer/src/store/slices/worktrees-git-identity-twin-host.test.ts new file mode 100644 index 00000000000..e1a00f57e72 --- /dev/null +++ b/src/renderer/src/store/slices/worktrees-git-identity-twin-host.test.ts @@ -0,0 +1,427 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Worktree } from '../../../../shared/worktree/types' +import { resetHostedReviewLinkMutationGenerationForTests } from './worktrees' +import { makeWorktree } from './worktrees-slice-test-fixtures' +import { + bumpHostedReviewLinkMutationGeneration, + migrateHostedReviewLinkMutationGeneration, + sanitizeHostedReviewLinksForBranchClears +} from './worktrees/metadata/hosted-review-link-mutation' +import { + createTestStore, + mockApi, + resetRemoteRuntimeMocks, + resetWorktreeSliceModuleMemory +} from './worktrees-slice-test-harness' + +vi.mock('sonner', () => ({ + toast: { warning: vi.fn(), info: vi.fn(), success: vi.fn(), error: vi.fn(), dismiss: vi.fn() } +})) + +beforeEach(() => { + vi.clearAllMocks() + resetWorktreeSliceModuleMemory() + resetRemoteRuntimeMocks() + resetHostedReviewLinkMutationGenerationForTests() +}) + +// #32918: one project at the same path on two hosts shares the id `repoId::path`. +const id = 'repo1::/srv/ai-tools' + +function storeWith(...rows: Partial[]) { + const store = createTestStore() + const worktrees = rows.map((row) => + makeWorktree({ id, repoId: 'repo1', path: '/srv/ai-tools', ...row }) + ) + store.setState({ worktreesByRepo: { repo1: worktrees } }) + return store +} + +function twinStore() { + return storeWith( + { hostId: 'runtime:env-a', head: 'aaa', branch: 'refs/heads/master', displayName: 'master' }, + { hostId: 'runtime:env-b', head: 'bbb', branch: 'refs/heads/feature', displayName: 'feature' } + ) +} + +describe('updateWorktreeGitIdentity with a same-path twin on another host', () => { + it("writes one host's git status only to that host's copy", () => { + const store = twinStore() + + store + .getState() + .updateWorktreeGitIdentity(id, { head: 'aaa2', branch: 'refs/heads/master' }, 'runtime:env-a') + + const [onA, onB] = store.getState().worktreesByRepo.repo1 + expect(onA).toMatchObject({ head: 'aaa2', branch: 'refs/heads/master' }) + expect(onB).toMatchObject({ head: 'bbb', branch: 'refs/heads/feature', displayName: 'feature' }) + }) + + it('leaves both copies alone when the answering host is unknown', () => { + const store = twinStore() + + store + .getState() + .updateWorktreeGitIdentity(id, { head: 'ccc', branch: 'refs/heads/other' }, null) + + const [onA, onB] = store.getState().worktreesByRepo.repo1 + expect(onA.branch).toBe('refs/heads/master') + expect(onB.branch).toBe('refs/heads/feature') + }) + + it("clears only the switching twin's push target and review link, on its own host", async () => { + const pushTarget = { remoteName: 'fork', branchName: 'old/review-head' } + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/stack/two', linkedPR: 7, pushTarget }, + { hostId: 'ssh:box-b', branch: 'refs/heads/stack/one', linkedPR: 101, pushTarget } + ) + + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/stack/two' }, 'ssh:box-b') + + await vi.waitFor(() => expect(mockApi.worktrees.updateMeta).toHaveBeenCalledTimes(1)) + expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith( + expect.objectContaining({ + worktreeId: id, + executionHostId: 'ssh:box-b', + updates: expect.objectContaining({ linkedPR: null, pushTarget: undefined }) + }) + ) + const [localAfter, remoteAfter] = store.getState().worktreesByRepo.repo1 + expect(localAfter).toMatchObject({ linkedPR: 7, pushTarget }) + expect(remoteAfter).toMatchObject({ branch: 'refs/heads/stack/two', pushTarget: undefined }) + }) +}) + +describe('a twin review-clear tombstone', () => { + it("guards only the switching copy's refetched rows, never its sibling's", () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/stack/two', linkedPR: 7 }, + { hostId: 'ssh:box-b', branch: 'refs/heads/stack/one', linkedPR: 101 } + ) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/stack/two' }, 'ssh:box-b') + + const [sibling, staleRefetch] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'local', + branch: 'refs/heads/stack/two', + linkedPR: 7 + }), + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'ssh:box-b', + branch: 'refs/heads/stack/one', + linkedPR: 101 + }) + ]) + + expect(sibling.linkedPR).toBe(7) + expect(staleRefetch).toMatchObject({ branch: 'refs/heads/stack/two', linkedPR: null }) + }) +}) + +describe('twin review-clear tombstones when both copies switch', () => { + it('keep guarding the first copy after the second clears', () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }, + { hostId: 'ssh:box-b', branch: 'refs/heads/b1', linkedPR: 2 } + ) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'local') + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/b2' }, 'ssh:box-b') + + const [first] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }) + ]) + + expect(first).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) +}) + +describe('a twin review-clear tombstone across a rename', () => { + it('still guards the renamed copy', () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/a1' }, + { hostId: 'ssh:box-b', branch: 'refs/heads/b1', linkedPR: 2 } + ) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/b2' }, 'ssh:box-b') + const renamed = 'repo1::/srv/ai-tools-renamed' + migrateHostedReviewLinkMutationGeneration(id, renamed) + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id: renamed, + repoId: 'repo1', + hostId: 'ssh:box-b', + branch: 'refs/heads/b1', + linkedPR: 2 + }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/b2', linkedPR: null }) + }) +}) + +describe('a twin review-clear tombstone after the sibling goes away', () => { + it('still drives a retry of the clear on the next branch switch', async () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/a1' }, + { hostId: 'ssh:box-b', branch: 'refs/heads/b1', linkedPR: 2 } + ) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/b2' }, 'ssh:box-b') + await vi.waitFor(() => expect(mockApi.worktrees.updateMeta).toHaveBeenCalledTimes(1)) + const [, remote] = store.getState().worktreesByRepo.repo1 + store.setState({ worktreesByRepo: { repo1: [remote] } }) + + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/b3' }, 'ssh:box-b') + + await vi.waitFor(() => expect(mockApi.worktrees.updateMeta).toHaveBeenCalledTimes(2)) + }) +}) + +describe('a provenance twin review-clear tombstone', () => { + it('spares the sibling another paired server publishes under the same SSH host id', () => { + const shared = { hostId: 'ssh:shared' as const, branch: 'refs/heads/stack/one' } + const store = storeWith( + { ...shared, runtimeOwnerEnvironmentId: 'hub-a', linkedPR: 1 }, + { ...shared, runtimeOwnerEnvironmentId: 'hub-b', linkedPR: 2 } + ) + store + .getState() + .updateWorktreeGitIdentity(id, { branch: 'refs/heads/stack/two' }, 'runtime:hub-a') + + const [sibling] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + ...shared, + runtimeOwnerEnvironmentId: 'hub-b', + linkedPR: 2 + }) + ]) + + expect(sibling.linkedPR).toBe(2) + expect(store.getState().worktreesByRepo.repo1[1].linkedPR).toBe(2) + }) +}) + +describe('updateWorktreeGitIdentity host matching', () => { + it("reaches a paired server's SSH checkout through its runtime alias, not its twin", () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/master' }, + { + hostId: 'ssh:private-target', + runtimeOwnerEnvironmentId: 'hub-a', + branch: 'refs/heads/master' + } + ) + + store + .getState() + .updateWorktreeGitIdentity(id, { branch: 'refs/heads/feature' }, 'runtime:hub-a') + + const [local, paired] = store.getState().worktreesByRepo.repo1 + expect(local.branch).toBe('refs/heads/master') + expect(paired.branch).toBe('refs/heads/feature') + }) + + it('keeps two paired servers apart when both publish the same SSH host id', () => { + const store = storeWith( + { hostId: 'ssh:shared', runtimeOwnerEnvironmentId: 'hub-a', branch: 'refs/heads/main' }, + { hostId: 'ssh:shared', runtimeOwnerEnvironmentId: 'hub-b', branch: 'refs/heads/feature' } + ) + + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/main' }, 'runtime:hub-a') + + expect(store.getState().worktreesByRepo.repo1[1].branch).toBe('refs/heads/feature') + }) +}) + +describe('review-clear tombstones are always keyed by the copy', () => { + it('still guard a legacy unstamped row after its listing stamps it', () => { + const store = storeWith({ branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'ssh:box') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', hostId: 'ssh:box', branch: 'refs/heads/a1', linkedPR: 1 }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it('never rewrite a same-path twin that arrives after a lone copy cleared', () => { + const store = storeWith({ hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'local') + + const [newcomer] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'ssh:box-b', + branch: 'refs/heads/b1', + linkedPR: 2 + }) + ]) + + expect(newcomer).toMatchObject({ branch: 'refs/heads/b1', linkedPR: 2 }) + }) + + it('still guard a runtime row once its listing adds the server provenance', () => { + const store = storeWith({ hostId: 'runtime:env-a', branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'runtime:env-a') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'runtime:env-a', + runtimeOwnerEnvironmentId: 'env-a', + branch: 'refs/heads/a1', + linkedPR: 1 + }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it('still guard a legacy local row after discovery stamps it local', () => { + const store = storeWith({ branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'local') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it("add the publishing server's provenance to a partially stamped SSH row", () => { + const store = storeWith({ hostId: 'ssh:x', branch: 'refs/heads/a1', linkedPR: 1 }) + store.setState({ + repos: [ + { + id: 'repo1', + path: '/srv/ai-tools', + displayName: 'ai-tools', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'runtime:hub' + } + ] + }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'ssh:x') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'ssh:x', + runtimeOwnerEnvironmentId: 'hub', + branch: 'refs/heads/a1', + linkedPR: 1 + }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it("follow a runtime row's own host, not its repo's, when provenance is backfilled", () => { + const store = storeWith({ hostId: 'runtime:env-b', branch: 'refs/heads/a1', linkedPR: 1 }) + store.setState({ + repos: [ + { + id: 'repo1', + path: '/srv/ai-tools', + displayName: 'ai-tools', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'runtime:env-a' + } + ] + }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'runtime:env-b') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'runtime:env-b', + runtimeOwnerEnvironmentId: 'env-b', + branch: 'refs/heads/a1', + linkedPR: 1 + }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it("never give a local copy a paired server's provenance", () => { + const store = storeWith({ hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }) + store.setState({ + repos: [ + { + id: 'repo1', + path: '/srv/ai-tools', + displayName: 'ai-tools', + badgeColor: '#000', + addedAt: 0, + executionHostId: 'runtime:hub' + } + ] + }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'local') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it("keep a paired server's provenance when its SSH host answers", () => { + const paired = { hostId: 'ssh:x' as const, runtimeOwnerEnvironmentId: 'hub' } + const store = storeWith({ ...paired, branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'ssh:x') + + const [stale] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', ...paired, branch: 'refs/heads/a1', linkedPR: 1 }) + ]) + + expect(stale).toMatchObject({ branch: 'refs/heads/a2', linkedPR: null }) + }) + + it('let a relink of the still-unstamped legacy row drop its stamped guard', () => { + const store = storeWith({ branch: 'refs/heads/a1', linkedPR: 1 }) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'ssh:box') + bumpHostedReviewLinkMutationGeneration(id, {}) + + const [relinked] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ id, repoId: 'repo1', hostId: 'ssh:box', branch: 'refs/heads/a2', linkedPR: 9 }) + ]) + + expect(relinked.linkedPR).toBe(9) + }) + + it("keep the sibling's guard when one copy is relinked", () => { + const store = storeWith( + { hostId: 'local', branch: 'refs/heads/a1', linkedPR: 1 }, + { hostId: 'ssh:box-b', branch: 'refs/heads/b1', linkedPR: 2 } + ) + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/a2' }, 'local') + store.getState().updateWorktreeGitIdentity(id, { branch: 'refs/heads/b2' }, 'ssh:box-b') + bumpHostedReviewLinkMutationGeneration(id, { hostId: 'local' }) + + const [staleRemote] = sanitizeHostedReviewLinksForBranchClears([ + makeWorktree({ + id, + repoId: 'repo1', + hostId: 'ssh:box-b', + branch: 'refs/heads/b1', + linkedPR: 2 + }) + ]) + + expect(staleRemote).toMatchObject({ branch: 'refs/heads/b2', linkedPR: null }) + }) +}) diff --git a/src/renderer/src/store/slices/worktrees-identity-migration.test.ts b/src/renderer/src/store/slices/worktrees-identity-migration.test.ts index 40e199bac68..0593b391b23 100644 --- a/src/renderer/src/store/slices/worktrees-identity-migration.test.ts +++ b/src/renderer/src/store/slices/worktrees-identity-migration.test.ts @@ -251,7 +251,7 @@ describe('migrateWorktreeIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [oldWorktree] } } as Partial) - store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }) + store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }, null) await vi.waitFor(() => { expect(getHostedReviewLinkMutationGenerationForTests(OLD)).toBe(0) expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ @@ -303,10 +303,14 @@ describe('migrateWorktreeIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [oldWorktree] } } as Partial) - store.getState().updateWorktreeGitIdentity(OLD, { - head: 'new-head', - branch: 'refs/heads/stack/two' - }) + store.getState().updateWorktreeGitIdentity( + OLD, + { + head: 'new-head', + branch: 'refs/heads/stack/two' + }, + null + ) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: OLD, @@ -348,7 +352,7 @@ describe('migrateWorktreeIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [oldWorktree] } } as Partial) - store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }) + store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }, null) await vi.waitFor(() => { expect(mockApi.worktrees.updateMeta).toHaveBeenCalledWith({ worktreeId: OLD, @@ -377,7 +381,7 @@ describe('migrateWorktreeIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [oldWorktree] } } as Partial) - store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }) + store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }, null) const switched = store.getState().worktreesByRepo.repo1[0] store.setState({ worktreesByRepo: { @@ -433,7 +437,7 @@ describe('migrateWorktreeIdentity', () => { }) store.setState({ worktreesByRepo: { repo1: [oldWorktree] } } as Partial) - store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }) + store.getState().updateWorktreeGitIdentity(OLD, { branch: 'refs/heads/stack/two' }, null) await oldPersistStartedPromise const switched = store.getState().worktreesByRepo.repo1[0] store.setState({ diff --git a/src/renderer/src/store/slices/worktrees/listing/detected-worktree-host-merge.ts b/src/renderer/src/store/slices/worktrees/listing/detected-worktree-host-merge.ts index b1fb7f7bafb..9a337c488a3 100644 --- a/src/renderer/src/store/slices/worktrees/listing/detected-worktree-host-merge.ts +++ b/src/renderer/src/store/slices/worktrees/listing/detected-worktree-host-merge.ts @@ -12,10 +12,11 @@ export function mergeDetectedWorktreesForHost( setup?: ProjectHostSetup, options?: WorktreeHostMatchOptions ): DetectedWorktreeListResult { + // Why stamp first: a twin's review-clear tombstone matches rows by their owner stamp. const refreshedForHost = sanitizeHostedReviewLinksForBranchClears( - refreshed.worktrees, + refreshed.worktrees.map((worktree) => withRepoHostOwnership(worktree, hostId, setup)), current?.worktrees - ).map((worktree) => withRepoHostOwnership(worktree, hostId, setup)) + ) const worktrees = mergeWorktreesForHost(current?.worktrees, refreshedForHost, hostId, options) if ( current && 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 1f8a17c8dd7..317f6d88a53 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 @@ -1,7 +1,15 @@ import type { StateCreator } from 'zustand' import type { AppState } from '../../../types' import type { WorktreeSlice } from '../../worktree-helpers' -import { applyWorktreeUpdates } from '../../worktree-helpers' +import { + clearCopyReviewLinks, + deleteCopyTombstones, + dropCopyTombstone, + moveCopyTombstones, + reviewClearTombstoneKey, + type WorktreeCopyClear, + worktreeCopyOwnerKey +} from './worktree-copy-review-clear' import type { Worktree } from '../../../../../../shared/worktree/types' import type { WorktreeMeta } from '../../../../../../shared/worktree/meta-types' import { normalizeWorkspaceAttachmentUpdate } from '../../../../../../shared/workspace-attachments' @@ -23,9 +31,18 @@ export { } export const hostedReviewLinkMutationGenerationByWorktreeId = new Map() -export const hostedReviewLinkClearTombstonesByWorktreeId = new Map< +export const hostedReviewLinkClearTombstonesByCopy = new Map< string, - { branch: string; branchIdentity: string; generation: number; head?: string } + { + branch: string + branchIdentity: string + generation: number + head?: string + /** The guarded copy's stamped {@link worktreeCopyOwnerKey}; part of the map key too. */ + owner: string + /** Its key before a listing stamped it; a relink of the unstamped row still matches. */ + rowOwner: string + } >() export const hostedReviewLinkWorktreeIdAliases = new Map() @@ -52,19 +69,34 @@ export function getHostedReviewLinkMutationGeneration(worktreeId: string): numbe return hostedReviewLinkMutationGenerationByWorktreeId.get(worktreeId) ?? 0 } -export function bumpHostedReviewLinkMutationGeneration(worktreeId: string): void { - hostedReviewLinkMutationGenerationByWorktreeId.set( +/** A manual relink; naming the relinked copy keeps a same-path twin's own clear guard. */ +export function bumpHostedReviewLinkMutationGeneration( + worktreeId: string, + relinkedCopy?: Partial> +): void { + const generation = getHostedReviewLinkMutationGeneration(worktreeId) + 1 + hostedReviewLinkMutationGenerationByWorktreeId.set(worktreeId, generation) + const owner = relinkedCopy ? worktreeCopyOwnerKey(relinkedCopy) : undefined + if (owner === undefined) { + deleteCopyTombstones(hostedReviewLinkClearTombstonesByCopy, worktreeId) + pruneHostedReviewLinkWorktreeAliasesForId(worktreeId) + return + } + const siblingGuarded = dropCopyTombstone( + hostedReviewLinkClearTombstonesByCopy, worktreeId, - getHostedReviewLinkMutationGeneration(worktreeId) + 1 + owner, + generation ) - hostedReviewLinkClearTombstonesByWorktreeId.delete(worktreeId) - pruneHostedReviewLinkWorktreeAliasesForId(worktreeId) + if (!siblingGuarded) { + pruneHostedReviewLinkWorktreeAliasesForId(worktreeId) + } } export function pruneHostedReviewLinkMutationGenerations(worktreeIds: Iterable): void { for (const worktreeId of worktreeIds) { hostedReviewLinkMutationGenerationByWorktreeId.delete(worktreeId) - hostedReviewLinkClearTombstonesByWorktreeId.delete(worktreeId) + deleteCopyTombstones(hostedReviewLinkClearTombstonesByCopy, worktreeId) hostedReviewLinkWorktreeIdAliases.delete(worktreeId) for (const [oldWorktreeId, newWorktreeId] of hostedReviewLinkWorktreeIdAliases) { if (newWorktreeId === worktreeId) { @@ -104,10 +136,14 @@ export function migrateHostedReviewLinkMutationGeneration( oldWorktreeId: string, newWorktreeId: string ): void { - const tombstone = hostedReviewLinkClearTombstonesByWorktreeId.get(oldWorktreeId) + const hasTombstone = moveCopyTombstones( + hostedReviewLinkClearTombstonesByCopy, + oldWorktreeId, + newWorktreeId + ) for (const [alias, target] of hostedReviewLinkWorktreeIdAliases) { if (target === oldWorktreeId) { - if (tombstone) { + if (hasTombstone) { hostedReviewLinkWorktreeIdAliases.set(alias, newWorktreeId) } else { hostedReviewLinkWorktreeIdAliases.delete(alias) @@ -115,7 +151,7 @@ export function migrateHostedReviewLinkMutationGeneration( } } const hasGeneration = hostedReviewLinkMutationGenerationByWorktreeId.has(oldWorktreeId) - if (tombstone) { + if (hasTombstone) { hostedReviewLinkWorktreeIdAliases.set(oldWorktreeId, newWorktreeId) } if (hasGeneration) { @@ -125,10 +161,6 @@ export function migrateHostedReviewLinkMutationGeneration( ) hostedReviewLinkMutationGenerationByWorktreeId.delete(oldWorktreeId) } - if (tombstone) { - hostedReviewLinkClearTombstonesByWorktreeId.set(newWorktreeId, tombstone) - hostedReviewLinkClearTombstonesByWorktreeId.delete(oldWorktreeId) - } } export function getHostedReviewLinkMutationGenerationForTests(worktreeId: string): number { @@ -141,7 +173,7 @@ export function getHostedReviewLinkWorktreeAliasCountForTests(): number { export function resetHostedReviewLinkMutationGenerationForTests(): void { hostedReviewLinkMutationGenerationByWorktreeId.clear() - hostedReviewLinkClearTombstonesByWorktreeId.clear() + hostedReviewLinkClearTombstonesByCopy.clear() hostedReviewLinkWorktreeIdAliases.clear() } @@ -166,22 +198,38 @@ export function rememberHostedReviewLinkClear( worktreeId: string, branch: string, generation: number, - head?: string + head: string | undefined, + copy: Pick ): void { - hostedReviewLinkClearTombstonesByWorktreeId.set(worktreeId, { + hostedReviewLinkClearTombstonesByCopy.set(reviewClearTombstoneKey(worktreeId, copy.owner), { branch, branchIdentity: canonicalHostedReviewBranchIdentity(branch), generation, - head + head, + owner: copy.owner, + rowOwner: copy.rowOwner }) } export function sanitizeHostedReviewLinksForBranchClear< T extends Pick & - Partial> + Partial< + Pick< + Worktree, + | HostedReviewLinkKey + | 'pushTarget' + | 'head' + | 'linkedItems' + | 'hostId' + | 'runtimeOwnerEnvironmentId' + > + > >(worktree: T, currentWorktrees?: readonly T[]): T { const hostedReviewWorktreeId = resolveHostedReviewLinkWorktreeId(worktree.id) - const tombstone = hostedReviewLinkClearTombstonesByWorktreeId.get(hostedReviewWorktreeId) + // Why the copy key only: a bare-id guard would also rewrite a same-path twin that arrives later. + const tombstone = hostedReviewLinkClearTombstonesByCopy.get( + reviewClearTombstoneKey(hostedReviewWorktreeId, worktreeCopyOwnerKey(worktree)) + ) const hasBranchScopedContext = HOSTED_REVIEW_LINK_KEYS.some((key) => worktree[key] != null) || worktree.pushTarget !== undefined @@ -194,8 +242,9 @@ export function sanitizeHostedReviewLinksForBranchClear< } const current = currentWorktrees?.find( (entry) => - entry.id === worktree.id || - resolveHostedReviewLinkWorktreeId(entry.id) === hostedReviewWorktreeId + worktreeCopyOwnerKey(entry) === tombstone.owner && + (entry.id === worktree.id || + resolveHostedReviewLinkWorktreeId(entry.id) === hostedReviewWorktreeId) ) const currentClean = current && @@ -230,27 +279,23 @@ export function sanitizeHostedReviewLinksForBranchClears< export function applyHostedReviewLinkClear( set: Parameters>[0], - worktreeId: string + worktreeId: string, + copy: WorktreeCopyClear ): void { set((s) => { - const nextWorktrees = applyWorktreeUpdates( - s.worktreesByRepo, - worktreeId, - CLEARED_HOSTED_REVIEW_LINK_UPDATES - ) + const byRepo = s.worktreesByRepo + const nextWorktrees = clearCopyReviewLinks(byRepo, worktreeId, copy) const nextDetectedWorktrees = applyDetectedWorktreeUpdates( s.detectedWorktreesByRepo, worktreeId, - CLEARED_HOSTED_REVIEW_LINK_UPDATES + CLEARED_HOSTED_REVIEW_LINK_UPDATES, + copy.hostId ) - if ( - nextWorktrees === s.worktreesByRepo && - nextDetectedWorktrees === s.detectedWorktreesByRepo - ) { + if (nextWorktrees === byRepo && nextDetectedWorktrees === s.detectedWorktreesByRepo) { return s } return { - ...(nextWorktrees !== s.worktreesByRepo + ...(nextWorktrees !== byRepo ? { worktreesByRepo: nextWorktrees, sortEpoch: s.sortEpoch + 1 } : {}), ...(nextDetectedWorktrees !== s.detectedWorktreesByRepo diff --git a/src/renderer/src/store/slices/worktrees/metadata/update-worktree-meta.ts b/src/renderer/src/store/slices/worktrees/metadata/update-worktree-meta.ts index 93e1ca22abc..748a31f64cb 100644 --- a/src/renderer/src/store/slices/worktrees/metadata/update-worktree-meta.ts +++ b/src/renderer/src/store/slices/worktrees/metadata/update-worktree-meta.ts @@ -254,7 +254,7 @@ export function createUpdateWorktreeMeta( return { ok: true } } if (hasHostedReviewLinkUpdates(enriched)) { - bumpHostedReviewLinkMutationGeneration(worktreeId) + bumpHostedReviewLinkMutationGeneration(worktreeId, worktreeForUpdate) } try { diff --git a/src/renderer/src/store/slices/worktrees/metadata/worktree-copy-review-clear.ts b/src/renderer/src/store/slices/worktrees/metadata/worktree-copy-review-clear.ts new file mode 100644 index 00000000000..cfcc75d92ae --- /dev/null +++ b/src/renderer/src/store/slices/worktrees/metadata/worktree-copy-review-clear.ts @@ -0,0 +1,112 @@ +import type { Worktree } from '../../../../../../shared/worktree/types' +import { + LOCAL_EXECUTION_HOST_ID, + parseExecutionHostId, + type ExecutionHostId +} from '../../../../../../shared/execution-host' +import { normalizeWorkspaceAttachmentUpdate } from '../../../../../../shared/workspace-attachments' +import { CLEARED_HOSTED_REVIEW_LINK_UPDATES } from '../../../../../../shared/worktree/hosted-review-links' +import { getRepoIdFromWorktreeId } from '../../../../../../shared/worktree/id' + +// Why this module: worktree ids repeat across hosts (`repoId::path`), so branch-switch review +// clears are keyed by id plus the copy's owner and never by the bare id. + +/** + * One copy's owner: its host plus the paired server that published it (two can share an `ssh:` id). + * Listing paths must stamp rows (`withRepoHostOwnership`) before comparing this key. + */ +export function worktreeCopyOwnerKey( + row: Partial> +): string { + // Why local folds into unstamped: discovery backfills `hostId: 'local'` on the same checkout. + const hostId = row.hostId === LOCAL_EXECUTION_HOST_ID ? undefined : row.hostId + // A `runtime:` host is its own server, so backfilled provenance must not change the key. + if (parseExecutionHostId(hostId)?.kind === 'runtime') { + return `${hostId}\0` + } + return `${hostId ?? ''}\0${row.runtimeOwnerEnvironmentId ?? ''}` +} + +/** The copy a review clear is for. */ +export type WorktreeCopyClear = { + /** The owner key of the row once stamped; tombstones use it. */ + owner: string + /** The row's own key at clear time, which differs only while a legacy row is unstamped. */ + rowOwner: string + hostId: ExecutionHostId | undefined +} + +/** Clears branch-scoped review context on the one copy `copy` names, stamped or not. */ +export function clearCopyReviewLinks( + byRepo: Record, + worktreeId: string, + copy: Pick +): Record { + const repoId = getRepoIdFromWorktreeId(worktreeId) + let changed = false + const next = byRepo[repoId]?.map((row) => { + const rowOwner = worktreeCopyOwnerKey(row) + if (row.id !== worktreeId || (rowOwner !== copy.owner && rowOwner !== copy.rowOwner)) { + return row + } + changed = true + return { + ...row, + ...normalizeWorkspaceAttachmentUpdate(row, CLEARED_HOSTED_REVIEW_LINK_UPDATES) + } + }) + return changed && next ? { ...byRepo, [repoId]: next } : byRepo +} + +export function reviewClearTombstoneKey(worktreeId: string, owner: string): string { + return `${worktreeId}\0${owner}` +} + +/** Drops every copy's tombstone for `worktreeId`. */ +export function deleteCopyTombstones(tombstones: Map, worktreeId: string): void { + for (const key of Array.from(tombstones.keys())) { + if (key === worktreeId || key.startsWith(`${worktreeId}\0`)) { + tombstones.delete(key) + } + } +} + +/** Re-keys every copy's tombstone after a rename; true when any moved. */ +export function moveCopyTombstones( + tombstones: Map, + oldWorktreeId: string, + newWorktreeId: string +): boolean { + const prefix = `${oldWorktreeId}\0` + let moved = false + for (const [key, tombstone] of Array.from(tombstones)) { + if (key.startsWith(prefix)) { + tombstones.set(`${newWorktreeId}\0${key.slice(prefix.length)}`, tombstone) + tombstones.delete(key) + moved = true + } + } + return moved +} + +/** + * A relink of one copy: drops that copy's tombstone (matched stamped or not) and moves the + * siblings' guards to `generation` so they stay live. True when a sibling is still guarded. + */ +export function dropCopyTombstone< + T extends { owner: string; rowOwner: string; generation: number } +>(tombstones: Map, worktreeId: string, owner: string, generation: number): boolean { + let siblingGuarded = false + for (const [key, tombstone] of Array.from(tombstones)) { + if (!key.startsWith(`${worktreeId}\0`)) { + continue + } + if (tombstone.owner === owner || tombstone.rowOwner === owner) { + tombstones.delete(key) + } else { + tombstones.set(key, { ...tombstone, generation }) + siblingGuarded = true + } + } + return siblingGuarded +} 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 f045126e55d..98bf2add3e0 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 @@ -13,30 +13,92 @@ import { getHostedReviewLinkMutationGeneration, getHostedReviewLinkUpdates, hasBranchScopedHostedReviewContext, - hostedReviewLinkClearTombstonesByWorktreeId, + hostedReviewLinkClearTombstonesByCopy, hostedReviewLinksAreCleared, rememberHostedReviewLinkClear, resolveHostedReviewLinkWorktreeId } from './hosted-review-link-mutation' +import { + reviewClearTombstoneKey, + worktreeCopyOwnerKey, + type WorktreeCopyClear +} from './worktree-copy-review-clear' import { persistWorktreeMeta } from './worktree-meta-persist' +import { withRepoHostOwnership } from '../listing/worktree-host-ownership' +import { getRepoRegistrationHostIds } from '../../../../../../shared/repo-host-identity' import { isRuntimeSelectorNotFoundError } from '../listing/runtime-worktree-rpc-errors' +import { parseExecutionHostId, type ExecutionHostId } from '../../../../../../shared/execution-host' +import { hostWorktreeTarget } from '../../../../../../shared/worktree/host-qualified-identity' +import { + selectWorkspaceByHostTarget, + selectWorktreesByRepo, + type WorkspaceRecordState +} from '../../../workspace-record-selectors' + +type GitIdentityTargets = { + rows: Set + /** Rows with this id exist on more than one host. */ + isTwin: boolean +} + +/** + * The rows one host's git answer for `worktreeId` describes. Ids are `repoId::path`, so a same-path + * twin on another host shares it. An id on one host keeps the id-only patch; a twin needs the + * answering host, and without one gets nothing until its listing places each copy. + */ +function gitIdentityTargets( + state: WorkspaceRecordState, + worktreeId: string, + ownerHostId: ExecutionHostId | null +): GitIdentityTargets { + const rows = (selectWorktreesByRepo(state)[getRepoIdFromWorktreeId(worktreeId)] ?? []).filter( + (worktree) => worktree.id === worktreeId + ) + // Why provenance too: two paired servers can each publish the same `ssh:` host id. + const owners = new Set(rows.map(worktreeCopyOwnerKey)) + if (owners.size <= 1) { + return { rows: new Set(rows), isTwin: false } + } + if (!ownerHostId) { + return { rows: new Set(), isTwin: true } + } + // Why the host target: it also places a paired server's SSH checkout under its `runtime:` alias. + const row = selectWorkspaceByHostTarget( + state, + hostWorktreeTarget({ id: worktreeId, hostId: ownerHostId }) + ) + return { rows: new Set(row ? [row] : []), isTwin: true } +} + +/** The one host registering `repoId`, which its listings stamp rows with; else undefined. */ +function soleRegistrationHostId( + repos: Parameters[0], + repoId: string +): ExecutionHostId | undefined { + const hosts = getRepoRegistrationHostIds(repos, repoId) + return hosts.size === 1 ? [...hosts][0] : undefined +} export function createUpdateWorktreeGitIdentity( set: WorktreeSliceSet, get: WorktreeSliceGet ): WorktreeSlice['updateWorktreeGitIdentity'] { - return (worktreeId, identity) => { + return (worktreeId, identity, ownerHostId) => { let shouldPersistHostedReviewClear = false let clearedBranch: string | null = null + let clearedCopy: WorktreeCopyClear | undefined + let clearedOnTwin = false let clearGeneration = getHostedReviewLinkMutationGeneration(worktreeId) const repoId = getRepoIdFromWorktreeId(worktreeId) - const existing = get().worktreesByRepo[repoId]?.find((worktree) => worktree.id === worktreeId) - if (!existing) { - return - } - const expectedHead = identity.head ?? existing.head - const expectedBranch = identity.branch === null ? '' : (identity.branch ?? existing.branch) - if (expectedHead === existing.head && expectedBranch === existing.branch) { + const nextHeadFor = (worktree: Worktree): string => identity.head ?? worktree.head + const nextBranchFor = (worktree: Worktree): string => + identity.branch === null ? '' : (identity.branch ?? worktree.branch) + const initialTargets = gitIdentityTargets(get(), worktreeId, ownerHostId) + const anyTargetChanges = [...initialTargets.rows].some( + (worktree) => + nextHeadFor(worktree) !== worktree.head || nextBranchFor(worktree) !== worktree.branch + ) + if (!anyTargetChanges) { return } @@ -47,12 +109,13 @@ export function createUpdateWorktreeGitIdentity( } let changed = false + const targets = gitIdentityTargets(s, worktreeId, ownerHostId) const next = current.map((worktree) => { - if (worktree.id !== worktreeId) { + if (!targets.rows.has(worktree)) { return worktree } - const nextHead = identity.head ?? worktree.head - const nextBranch = identity.branch === null ? '' : (identity.branch ?? worktree.branch) + const nextHead = nextHeadFor(worktree) + const nextBranch = nextBranchFor(worktree) if (nextHead === worktree.head && nextBranch === worktree.branch) { return worktree } @@ -62,16 +125,41 @@ export function createUpdateWorktreeGitIdentity( canonicalHostedReviewBranchIdentity(worktree.branch) const shouldClearHostedReviewContext = hostedReviewBranchChanged && hasBranchScopedHostedReviewContext(worktree) + // Why the copy: a same-path twin shares the id, so every guard and clear names this row. + // Keyed by the stamp the next listing gives the row, so legacy or partial stamps still match. + // Only missing parts are filled in; a row's own host or publishing server is never replaced. + const stampHostId = soleRegistrationHostId(s.repos, worktree.repoId) ?? ownerHostId + const listingStamp = stampHostId ? withRepoHostOwnership(worktree, stampHostId) : worktree + const stampedHostId = worktree.hostId ?? listingStamp.hostId + // Provenance only distinguishes SSH copies; local listings never carry it. + const stamped = { + hostId: stampedHostId, + runtimeOwnerEnvironmentId: + worktree.runtimeOwnerEnvironmentId ?? + (parseExecutionHostId(stampedHostId)?.kind === 'ssh' + ? listingStamp.runtimeOwnerEnvironmentId + : undefined) + } + const copy = { + owner: worktreeCopyOwnerKey(stamped), + rowOwner: worktreeCopyOwnerKey(worktree), + hostId: worktree.hostId + } if (shouldClearHostedReviewContext) { shouldPersistHostedReviewClear = true clearedBranch = nextBranch + clearedCopy = copy + clearedOnTwin = targets.isTwin clearGeneration = getHostedReviewLinkMutationGeneration(worktreeId) - rememberHostedReviewLinkClear(worktreeId, nextBranch, clearGeneration, nextHead) + rememberHostedReviewLinkClear(worktreeId, nextBranch, clearGeneration, nextHead, copy) } else { - const tombstone = hostedReviewLinkClearTombstonesByWorktreeId.get(worktreeId) + const tombstoneKey = reviewClearTombstoneKey(worktreeId, copy.owner) + const tombstone = hostedReviewLinkClearTombstonesByCopy.get(tombstoneKey) if (tombstone) { + clearedCopy = copy + clearedOnTwin = targets.isTwin const nextBranchIdentity = canonicalHostedReviewBranchIdentity(nextBranch) - hostedReviewLinkClearTombstonesByWorktreeId.set(worktreeId, { + hostedReviewLinkClearTombstonesByCopy.set(tombstoneKey, { ...tombstone, branch: nextBranch, branchIdentity: nextBranchIdentity, @@ -127,6 +215,16 @@ export function createUpdateWorktreeGitIdentity( if (!shouldPersistHostedReviewClear || clearedBranch === null) { return } + const clearCopy = clearedCopy + if (!clearCopy) { + return + } + // Why: for a twin every read, clear and save below stays on the copy this answer patched. + const ownerForTwin = clearedOnTwin ? (ownerHostId ?? undefined) : undefined + const readRow = (id: string) => get().getKnownWorktreeById(id, ownerForTwin) + const persistTo = (id: string) => runtimeTargetForWorktreeOwner(get(), id, ownerForTwin) + // Why both: main matches an unstamped local twin by the answering host, not the row stamp. + const persistHostId = ownerHostId ?? clearCopy.hostId void Promise.resolve() .then(async () => { @@ -138,7 +236,7 @@ export function createUpdateWorktreeGitIdentity( return } persistedWorktreeIds.add(currentWorktreeId) - let current = get().getKnownWorktreeById(currentWorktreeId) + let current = readRow(currentWorktreeId) if ( !current || current.branch !== clearedBranch || @@ -148,16 +246,17 @@ export function createUpdateWorktreeGitIdentity( } if (!hostedReviewLinksAreCleared(current as Worktree)) { // Why: a refetch can rehydrate stale linked-review metadata before this async clear starts; clear it again. - applyHostedReviewLinkClear(set, currentWorktreeId) - current = get().getKnownWorktreeById(currentWorktreeId) + applyHostedReviewLinkClear(set, currentWorktreeId, clearCopy) + current = readRow(currentWorktreeId) if (!current || current.branch !== clearedBranch) { return } } await persistWorktreeMeta( - runtimeTargetForWorktreeOwner(get(), currentWorktreeId), + persistTo(currentWorktreeId), currentWorktreeId, - CLEARED_HOSTED_REVIEW_LINK_UPDATES + CLEARED_HOSTED_REVIEW_LINK_UPDATES, + persistHostId ) const migratedWorktreeId = resolveHostedReviewLinkWorktreeId(currentWorktreeId) if (migratedWorktreeId === currentWorktreeId) { @@ -166,7 +265,7 @@ export function createUpdateWorktreeGitIdentity( // Why: worktree creation can migrate ids mid-IPC; persist the clear under the new durable id too. currentWorktreeId = migratedWorktreeId } - const latest = get().getKnownWorktreeById(currentWorktreeId) + const latest = readRow(currentWorktreeId) if ( !latest || latest.branch !== clearedBranch || @@ -177,14 +276,16 @@ export function createUpdateWorktreeGitIdentity( if (getHostedReviewLinkMutationGeneration(currentWorktreeId) !== clearGeneration) { // Why: a delayed branch-switch clear must not win over a newer manual relink. await persistWorktreeMeta( - runtimeTargetForWorktreeOwner(get(), currentWorktreeId), + persistTo(currentWorktreeId), currentWorktreeId, - getHostedReviewLinkUpdates(latest as Worktree) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: a visible or detected row; both carry every review-link field read here. + getHostedReviewLinkUpdates(latest as Worktree), + persistHostId ) return } // Why: a refetch can rehydrate old metadata before the branch-switch clear reaches disk; don't write the stale link back. - applyHostedReviewLinkClear(set, currentWorktreeId) + applyHostedReviewLinkClear(set, currentWorktreeId, clearCopy) }) .catch((err) => { if (isRuntimeSelectorNotFoundError(err)) {