From c46cb2f3d141e4b73e02c610f960f0f0037ba59e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:07:40 -0700 Subject: [PATCH] Add continue actions for merge, rebase, and cherry-pick - Detect progress via sequencer marker refs (MERGE_HEAD/REBASE_HEAD/CHERRY_PICK_HEAD) instead of HEAD to avoid false positives from concurrent worktree commits. - Consolidate abort/continue error handling and state management. - Gracefully handle older remote hosts that don't yet support continue. --- .../MobileSourceControlBranchCard.tsx | 30 ++++- .../MobileSourceControlPanel.tsx | 13 +- ...bile-source-control-conflict-abort.test.ts | 28 ----- .../mobile-source-control-conflict-abort.ts | 18 --- ...le-source-control-conflict-actions.test.ts | 54 +++++++++ .../mobile-source-control-conflict-actions.ts | 38 ++++++ .../mobile-source-control-screen-state.ts | 10 ++ .../use-mobile-source-control-state.ts | 9 +- src/main/git/sequencer-actions.test.ts | 54 +++++++-- src/main/git/sequencer-actions.ts | 27 +++-- src/main/git/source-control/status-read.ts | 5 +- .../git/status-conflict-operations.test.ts | 36 +++++- src/relay/git-handler-status-ops.ts | 5 +- src/relay/git-handler.test.ts | 32 +++++ src/relay/git-handler.ts | 37 ++++-- ...SourceControl.remote-action-errors.test.ts | 36 ++++++ ...e-control-header-toolbar-identity.test.tsx | 10 ++ .../listing/conflict-status-cards.tsx | 4 +- .../listing/operation-banner-actions.tsx | 7 +- .../listing/operation-banner.test.tsx | 15 ++- .../panel/branch-context-row.tsx | 4 +- .../source-control/sync/remote-refresh.ts | 4 + .../source-control/sync/use-conflict-abort.ts | 75 +++++------- .../sync/use-conflict-advance.ts | 102 +++++----------- .../sync/use-conflict-operation-runner.ts | 114 ++++++++++++++++++ .../src/runtime/runtime-git-sync-client.ts | 44 +++++-- 26 files changed, 577 insertions(+), 234 deletions(-) delete mode 100644 mobile/src/source-control/mobile-source-control-conflict-abort.test.ts delete mode 100644 mobile/src/source-control/mobile-source-control-conflict-abort.ts create mode 100644 mobile/src/source-control/mobile-source-control-conflict-actions.test.ts create mode 100644 mobile/src/source-control/mobile-source-control-conflict-actions.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-operation-runner.ts diff --git a/mobile/src/source-control/MobileSourceControlBranchCard.tsx b/mobile/src/source-control/MobileSourceControlBranchCard.tsx index 9be5430c807..9894918e02d 100644 --- a/mobile/src/source-control/MobileSourceControlBranchCard.tsx +++ b/mobile/src/source-control/MobileSourceControlBranchCard.tsx @@ -4,7 +4,10 @@ import { colors } from '../theme/mobile-theme' import { styles } from './mobile-source-control-styles' import { MobileSourceControlPrChip } from './MobileSourceControlPrChip' import type { MobilePrChipSummary } from './mobile-pr-chip-summary' -import { mobileConflictAbortLabel } from './mobile-source-control-conflict-abort' +import { + mobileConflictAbortLabel, + mobileConflictContinueLabel +} from './mobile-source-control-conflict-actions' type Props = { branchLabel: string @@ -13,11 +16,16 @@ type Props = { stagedCount: number branchCount: number conflictOperation: string | null - // True while any serial git IO is in flight — disables Abort so ops don't race. + // Why: git refuses `--continue` while any file is unmerged — hide the button then. + hasUnresolvedConflicts: boolean + // True while any serial git IO is in flight — disables Abort/Continue so ops don't race. conflictBusy: boolean // True only while abort-merge / abort-rebase itself is running (label accuracy). conflictAborting: boolean + // True only while the matching continue call itself is running (label accuracy). + conflictAdvancing: boolean onAbortConflict: (operation: string) => void + onContinueConflict: (operation: string) => void // The PR chip is shown only on repos with a hosted-review remote; null hides it. prChip: MobilePrChipSummary | null onOpenPr: () => void @@ -33,9 +41,12 @@ export function MobileSourceControlBranchCard({ stagedCount, branchCount, conflictOperation, + hasUnresolvedConflicts, conflictBusy, conflictAborting, + conflictAdvancing, onAbortConflict, + onContinueConflict, prChip, onOpenPr }: Props) { @@ -60,6 +71,21 @@ export function MobileSourceControlBranchCard({ {showConflict ? ( {conflictOperation} + {!hasUnresolvedConflicts ? ( + [ + styles.abortButton, + conflictBusy && styles.abortButtonDisabled, + pressed && !conflictBusy && styles.abortPressed + ]} + disabled={conflictBusy} + onPress={() => onContinueConflict(conflictOperation)} + > + + {mobileConflictContinueLabel(conflictOperation, conflictAdvancing)} + + + ) : null} {conflictOperation === 'merge' || conflictOperation === 'rebase' ? ( [ diff --git a/mobile/src/source-control/MobileSourceControlPanel.tsx b/mobile/src/source-control/MobileSourceControlPanel.tsx index 3244d440b01..5f2da543330 100644 --- a/mobile/src/source-control/MobileSourceControlPanel.tsx +++ b/mobile/src/source-control/MobileSourceControlPanel.tsx @@ -14,7 +14,10 @@ import { styles } from './mobile-source-control-styles' import { hubStyles } from './mobile-source-control-hub-styles' import type { SourceControlHubTab } from './mobile-source-control-hub-tab' import { buildMobilePrChipSummary, countUnresolvedReviewThreads } from './mobile-pr-chip-summary' -import { isMobileConflictAborting } from './mobile-source-control-conflict-abort' +import { + isMobileConflictAborting, + isMobileConflictAdvancing +} from './mobile-source-control-conflict-actions' import { useMobilePrSidebarController } from '../session/use-mobile-pr-sidebar-controller' import { prSidebarDetailsNeedFetch } from '../session/mobile-pr-sidebar-state' import { MobilePrViewPanelBody } from '../components/pr-sidebar/MobilePrViewPanel' @@ -110,8 +113,10 @@ export function MobileSourceControlPanel({ syncLabel, unstagedCount, stagedCount, + hasUnresolvedConflicts, branchEntries, - abortConflictOperation + abortConflictOperation, + continueConflictOperation } = state const ioBusy = busyAction !== null || openingPath !== null || openingBranchPath !== null const ready = screenState.kind === 'ready' @@ -275,6 +280,7 @@ export function MobileSourceControlPanel({ // Git status always reports a conflictOperation enum; 'unknown' means none. const hasActiveConflict = conflictOperation != null && conflictOperation !== 'unknown' const conflictAborting = isMobileConflictAborting(busyAction, conflictOperation) + const conflictAdvancing = isMobileConflictAdvancing(busyAction, conflictOperation) return ( @@ -297,9 +303,12 @@ export function MobileSourceControlPanel({ stagedCount={stagedCount} branchCount={branchEntries.length} conflictOperation={conflictOperation} + hasUnresolvedConflicts={hasUnresolvedConflicts} conflictBusy={busyAction !== null} conflictAborting={conflictAborting} + conflictAdvancing={conflictAdvancing} onAbortConflict={(operation) => void abortConflictOperation(operation)} + onContinueConflict={(operation) => void continueConflictOperation(operation)} prChip={prChip} onOpenPr={openPrTab} /> diff --git a/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts b/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts deleted file mode 100644 index fa9e40a4746..00000000000 --- a/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { - isMobileConflictAborting, - mobileConflictAbortLabel -} from './mobile-source-control-conflict-abort' - -describe('isMobileConflictAborting', () => { - it('is true only for the matching abort action', () => { - expect(isMobileConflictAborting('abort-merge', 'merge')).toBe(true) - expect(isMobileConflictAborting('abort-rebase', 'rebase')).toBe(true) - }) - - it('is false for other busy actions (stage/commit must not look like abort)', () => { - expect(isMobileConflictAborting('stage-all', 'merge')).toBe(false) - expect(isMobileConflictAborting('commit', 'rebase')).toBe(false) - expect(isMobileConflictAborting(null, 'merge')).toBe(false) - expect(isMobileConflictAborting('abort-merge', 'rebase')).toBe(false) - expect(isMobileConflictAborting('abort-merge', 'unknown')).toBe(false) - }) -}) - -describe('mobileConflictAbortLabel', () => { - it('shows Aborting only while abort is in flight', () => { - expect(mobileConflictAbortLabel('merge', true)).toBe('Aborting…') - expect(mobileConflictAbortLabel('merge', false)).toBe('Abort merge') - expect(mobileConflictAbortLabel('rebase', false)).toBe('Abort rebase') - }) -}) diff --git a/mobile/src/source-control/mobile-source-control-conflict-abort.ts b/mobile/src/source-control/mobile-source-control-conflict-abort.ts deleted file mode 100644 index ac71bcbf9ed..00000000000 --- a/mobile/src/source-control/mobile-source-control-conflict-abort.ts +++ /dev/null @@ -1,18 +0,0 @@ -// Pure helpers for the branch-card conflict Abort control. Kept free of React so -// the busy-label rule (abort-in-flight only) is unit-testable. - -/** True while git.abortMerge / git.abortRebase is the active serial action. */ -export function isMobileConflictAborting( - busyAction: string | null, - conflictOperation: string | null -): boolean { - if (conflictOperation !== 'merge' && conflictOperation !== 'rebase') { - return false - } - return busyAction === `abort-${conflictOperation}` -} - -/** Label for the Abort control — never says "Aborting…" for unrelated busy work. */ -export function mobileConflictAbortLabel(conflictOperation: string, aborting: boolean): string { - return aborting ? 'Aborting…' : `Abort ${conflictOperation}` -} diff --git a/mobile/src/source-control/mobile-source-control-conflict-actions.test.ts b/mobile/src/source-control/mobile-source-control-conflict-actions.test.ts new file mode 100644 index 00000000000..a30c46f8d89 --- /dev/null +++ b/mobile/src/source-control/mobile-source-control-conflict-actions.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from 'vitest' +import { + isMobileConflictAborting, + isMobileConflictAdvancing, + mobileConflictAbortLabel, + mobileConflictContinueLabel +} from './mobile-source-control-conflict-actions' + +describe('isMobileConflictAborting', () => { + it('is true only for the matching abort action', () => { + expect(isMobileConflictAborting('abort-merge', 'merge')).toBe(true) + expect(isMobileConflictAborting('abort-rebase', 'rebase')).toBe(true) + }) + + it('is false for other busy actions (stage/commit must not look like abort)', () => { + expect(isMobileConflictAborting('stage-all', 'merge')).toBe(false) + expect(isMobileConflictAborting('commit', 'rebase')).toBe(false) + expect(isMobileConflictAborting(null, 'merge')).toBe(false) + expect(isMobileConflictAborting('abort-merge', 'rebase')).toBe(false) + expect(isMobileConflictAborting('abort-merge', 'unknown')).toBe(false) + }) +}) + +describe('isMobileConflictAdvancing', () => { + it('is true only for the matching continue action', () => { + expect(isMobileConflictAdvancing('continue-merge', 'merge')).toBe(true) + expect(isMobileConflictAdvancing('continue-rebase', 'rebase')).toBe(true) + expect(isMobileConflictAdvancing('continue-cherry-pick', 'cherry-pick')).toBe(true) + }) + + it('is false for other busy actions (abort/stage must not look like continue)', () => { + expect(isMobileConflictAdvancing('abort-merge', 'merge')).toBe(false) + expect(isMobileConflictAdvancing('stage-all', 'rebase')).toBe(false) + expect(isMobileConflictAdvancing(null, 'cherry-pick')).toBe(false) + expect(isMobileConflictAdvancing('continue-merge', 'rebase')).toBe(false) + expect(isMobileConflictAdvancing('continue-merge', 'unknown')).toBe(false) + }) +}) + +describe('mobileConflictAbortLabel', () => { + it('shows Aborting only while abort is in flight', () => { + expect(mobileConflictAbortLabel('merge', true)).toBe('Aborting…') + expect(mobileConflictAbortLabel('merge', false)).toBe('Abort merge') + expect(mobileConflictAbortLabel('rebase', false)).toBe('Abort rebase') + }) +}) + +describe('mobileConflictContinueLabel', () => { + it('shows Continuing only while continue is in flight', () => { + expect(mobileConflictContinueLabel('rebase', true)).toBe('Continuing…') + expect(mobileConflictContinueLabel('rebase', false)).toBe('Continue rebase') + expect(mobileConflictContinueLabel('cherry-pick', false)).toBe('Continue cherry-pick') + }) +}) diff --git a/mobile/src/source-control/mobile-source-control-conflict-actions.ts b/mobile/src/source-control/mobile-source-control-conflict-actions.ts new file mode 100644 index 00000000000..009a97dbb64 --- /dev/null +++ b/mobile/src/source-control/mobile-source-control-conflict-actions.ts @@ -0,0 +1,38 @@ +// Pure helpers for the branch-card conflict Abort/Continue controls. Kept free of +// React so the busy-label rules (matching action in flight only) are unit-testable. + +/** True while git.abortMerge / git.abortRebase is the active serial action. */ +export function isMobileConflictAborting( + busyAction: string | null, + conflictOperation: string | null +): boolean { + if (conflictOperation !== 'merge' && conflictOperation !== 'rebase') { + return false + } + return busyAction === `abort-${conflictOperation}` +} + +/** True while the matching git.continue* call is the active serial action. */ +export function isMobileConflictAdvancing( + busyAction: string | null, + conflictOperation: string | null +): boolean { + if ( + conflictOperation !== 'merge' && + conflictOperation !== 'rebase' && + conflictOperation !== 'cherry-pick' + ) { + return false + } + return busyAction === `continue-${conflictOperation}` +} + +/** Label for the Abort control — never says "Aborting…" for unrelated busy work. */ +export function mobileConflictAbortLabel(conflictOperation: string, aborting: boolean): string { + return aborting ? 'Aborting…' : `Abort ${conflictOperation}` +} + +/** Label for the Continue control — never says "Continuing…" for unrelated busy work. */ +export function mobileConflictContinueLabel(conflictOperation: string, advancing: boolean): string { + return advancing ? 'Continuing…' : `Continue ${conflictOperation}` +} diff --git a/mobile/src/source-control/mobile-source-control-screen-state.ts b/mobile/src/source-control/mobile-source-control-screen-state.ts index 78a588d19ff..c9da107d5d3 100644 --- a/mobile/src/source-control/mobile-source-control-screen-state.ts +++ b/mobile/src/source-control/mobile-source-control-screen-state.ts @@ -134,6 +134,16 @@ export function formatBranchLabel(branch: string | undefined, head: string | und return branch || head?.slice(0, 7) || 'No branch' } +/** Branch-card sync summary; null while upstream status is still unknown. */ +export function formatUpstreamSyncLabel( + upstream: { hasUpstream: boolean; ahead: number; behind: number } | undefined +): string | null { + if (!upstream) { + return null + } + return upstream.hasUpstream ? `${upstream.ahead} ahead, ${upstream.behind} behind` : 'No upstream' +} + export function statusColor(status: MobileGitFileStatus): string { switch (status) { case 'added': diff --git a/mobile/src/source-control/use-mobile-source-control-state.ts b/mobile/src/source-control/use-mobile-source-control-state.ts index 1f541739d9a..4d57680e992 100644 --- a/mobile/src/source-control/use-mobile-source-control-state.ts +++ b/mobile/src/source-control/use-mobile-source-control-state.ts @@ -29,6 +29,7 @@ import { useMobileSourceControlCommitFailure } from './use-mobile-source-control import { buildMobileGitStatusEntryViews, formatBranchLabel, + formatUpstreamSyncLabel, type MobileBranchEntryView } from './mobile-source-control-screen-state' @@ -163,12 +164,7 @@ export function useMobileSourceControlState(params: MobileSourceControlStatePara const branchLabel = formatBranchLabel(status?.branch, status?.head) const upstream = status?.upstreamStatus const upstreamKnown = upstream !== undefined - const syncLabel = - upstream && upstream.hasUpstream - ? `${upstream.ahead} ahead, ${upstream.behind} behind` - : upstream && !upstream.hasUpstream - ? 'No upstream' - : null + const syncLabel = formatUpstreamSyncLabel(upstream) const { sendGitRequest, sendCommitRequest, runGitSyncSteps } = useMobileGitRequests({ client, @@ -300,6 +296,7 @@ export function useMobileSourceControlState(params: MobileSourceControlStatePara unstageablePaths, stagedCount, unstagedCount, + hasUnresolvedConflicts, branchLabel, upstream, upstreamKnown, diff --git a/src/main/git/sequencer-actions.test.ts b/src/main/git/sequencer-actions.test.ts index ccdda3ebe81..a84575fb7fa 100644 --- a/src/main/git/sequencer-actions.test.ts +++ b/src/main/git/sequencer-actions.test.ts @@ -27,7 +27,7 @@ const CASES: readonly [string, SequencerAction, string[]][] = [ ['continueCherryPick', continueCherryPick, ['cherry-pick', '--continue']] ] -// The HEAD probe runs before the sequencer step, so calls are matched by argv, not index. +// The marker probe runs before the sequencer step, so calls are matched by argv, not index. function optionsFor(args: readonly string[]): { env?: NodeJS.ProcessEnv } | undefined { const call = gitExecFileAsyncMock.mock.calls.find( (called: unknown[]) => (called[0] as string[]).join(' ') === args.join(' ') @@ -35,7 +35,7 @@ function optionsFor(args: readonly string[]): { env?: NodeJS.ProcessEnv } | unde return call?.[1] as { env?: NodeJS.ProcessEnv } | undefined } -function headProbe(oid: string) { +function markerProbe(oid: string) { return (args: readonly string[]) => args[0] === 'rev-parse' ? Promise.resolve({ stdout: `${oid}\n`, stderr: '' }) @@ -45,7 +45,7 @@ function headProbe(oid: string) { describe('git sequencer actions', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() - gitExecFileAsyncMock.mockImplementation(headProbe('abc123')) + gitExecFileAsyncMock.mockImplementation(markerProbe('abc123')) }) it.each(CASES)('%s runs the matching git command in the worktree', async (_name, run, args) => { @@ -74,21 +74,37 @@ describe('git sequencer actions', () => { }) // `git rebase --continue` exits nonzero when it lands the resolution and then stops on the - // NEXT commit's conflict. HEAD moved, so that is the sequencer advancing, not a failed step. - it('treats a stop on the next commit as progress once HEAD has moved', async () => { - let head = 'aaa111' + // NEXT commit's conflict. REBASE_HEAD now names that next commit — the sequencer advancing. + it('treats a stop on the next commit as progress once the marker has moved', async () => { + let rebaseHead = 'aaa111' gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args[0] === 'rev-parse') { - return Promise.resolve({ stdout: `${head}\n`, stderr: '' }) + expect(args).toEqual(['rev-parse', '-q', '--verify', 'REBASE_HEAD']) + return Promise.resolve({ stdout: `${rebaseHead}\n`, stderr: '' }) } - head = 'bbb222' + rebaseHead = 'bbb222' return Promise.reject(new Error('error: could not apply ec9b3362... feat: add thing')) }) await expect(continueRebase('/repo')).resolves.toBeUndefined() }) - it('still fails a step that refused to run, leaving HEAD where it was', async () => { + it('treats a marker that cleared as the operation completing', async () => { + let mergeHead: string | null = 'aaa111' + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'rev-parse') { + return mergeHead + ? Promise.resolve({ stdout: `${mergeHead}\n`, stderr: '' }) + : Promise.reject(new Error('fatal: needed a single revision')) + } + mergeHead = null + return Promise.reject(new Error('warning: post-commit cleanup failed')) + }) + + await expect(continueMerge('/repo')).resolves.toBeUndefined() + }) + + it('still fails a step that refused to run, leaving the marker where it was', async () => { gitExecFileAsyncMock.mockImplementation((args: string[]) => args[0] === 'rev-parse' ? Promise.resolve({ stdout: 'aaa111\n', stderr: '' }) @@ -98,8 +114,24 @@ describe('git sequencer actions', () => { await expect(continueRebase('/repo')).rejects.toThrow('needs merge') }) - // An unborn HEAD (or an unreadable one) proves nothing, so the original failure stands. - it('rethrows when HEAD cannot be read', async () => { + // The whole point of probing the marker instead of HEAD: another actor committing in + // the worktree moves HEAD but not the sequencer's own ref, so a refused step still fails. + it('is not fooled by a concurrent commit in the worktree', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => + args[0] === 'rev-parse' + ? Promise.resolve({ stdout: 'aaa111\n', stderr: '' }) + : Promise.reject(new Error('f.txt: needs merge')) + ) + + await expect(continueRebase('/repo')).rejects.toThrow('needs merge') + expect(gitExecFileAsyncMock).not.toHaveBeenCalledWith( + ['rev-parse', '--verify', 'HEAD'], + expect.anything() + ) + }) + + // No marker before the step proves nothing ran, so the original failure stands. + it('rethrows when the marker was already absent', async () => { gitExecFileAsyncMock.mockImplementation((args: string[]) => args[0] === 'rev-parse' ? Promise.reject(new Error('fatal: bad revision')) diff --git a/src/main/git/sequencer-actions.ts b/src/main/git/sequencer-actions.ts index 46d4d00a0d9..dc9e68045e5 100644 --- a/src/main/git/sequencer-actions.ts +++ b/src/main/git/sequencer-actions.ts @@ -4,13 +4,14 @@ import { gitOptionsForWorktree } from './git-runtime-options' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' -async function readHeadOid( +async function readSequencerMarkerOid( + marker: string, worktreePath: string, options: GitRuntimeOptions ): Promise { try { const { stdout } = await gitExecFileAsync( - ['rev-parse', '--verify', 'HEAD'], + ['rev-parse', '-q', '--verify', marker], gitOptionsForWorktree(worktreePath, options) ) return stdout.trim() || null @@ -19,14 +20,15 @@ async function readHeadOid( } } -// Why: every subcommand here predates the Git 2.25 baseline (`merge --continue` 2.12, -// the rest older), so no capability probe or fallback is needed. +// Why: everything here predates the Git 2.25 baseline (`merge --continue` 2.12, +// REBASE_HEAD 2.17), so no capability probe or fallback is needed. async function runSequencerAction( args: readonly [string, string], + marker: string, worktreePath: string, options: GitRuntimeOptions ): Promise { - const headBefore = await readHeadOid(worktreePath, options) + const markerBefore = await readSequencerMarkerOid(marker, worktreePath, options) try { await runWithGitReadCacheInvalidation(() => gitExecFileAsync([...args], { @@ -37,10 +39,11 @@ async function runSequencerAction( ) } catch (error) { // Why: `--continue` also exits nonzero when it DID commit the resolution and the - // sequencer then stopped on the next commit. A moved HEAD is the proof it advanced; - // only an unmoved one means the step refused and nothing happened. - const headAfter = await readHeadOid(worktreePath, options) - if (!headBefore || !headAfter || headAfter === headBefore) { + // sequencer then stopped on the next commit. The operation's own marker ref moving + // (or clearing) is the proof it advanced — unlike HEAD, no concurrent commit in the + // worktree can touch it, so a refused step can never masquerade as progress. + const markerAfter = await readSequencerMarkerOid(marker, worktreePath, options) + if (!markerBefore || markerAfter === markerBefore) { throw error } } @@ -50,19 +53,19 @@ export async function continueMerge( worktreePath: string, options: GitRuntimeOptions = {} ): Promise { - await runSequencerAction(['merge', '--continue'], worktreePath, options) + await runSequencerAction(['merge', '--continue'], 'MERGE_HEAD', worktreePath, options) } export async function continueRebase( worktreePath: string, options: GitRuntimeOptions = {} ): Promise { - await runSequencerAction(['rebase', '--continue'], worktreePath, options) + await runSequencerAction(['rebase', '--continue'], 'REBASE_HEAD', worktreePath, options) } export async function continueCherryPick( worktreePath: string, options: GitRuntimeOptions = {} ): Promise { - await runSequencerAction(['cherry-pick', '--continue'], worktreePath, options) + await runSequencerAction(['cherry-pick', '--continue'], 'CHERRY_PICK_HEAD', worktreePath, options) } diff --git a/src/main/git/source-control/status-read.ts b/src/main/git/source-control/status-read.ts index 972743688d9..b6b5ad955bf 100644 --- a/src/main/git/source-control/status-read.ts +++ b/src/main/git/source-control/status-read.ts @@ -115,10 +115,11 @@ async function runGetStatus( // Why: detectConflictOperation and git status are independent, so run them concurrently to save I/O latency. const conflictPromise = detectConflictOperation(worktreePath) - // Why: only sequencer operations have state on disk, so a clean repo reads nothing. + // Why: only a rebase has readable step state (rebase-merge/rebase-apply) — a + // cherry-pick's sequencer dir has no reader here, so probing it would be pure ENOENT churn. const operationProgressPromise = conflictPromise .then(async (operation) => - operation === 'rebase' || operation === 'cherry-pick' + operation === 'rebase' ? await readGitRebaseProgress(await resolveGitDir(worktreePath)) : undefined ) diff --git a/src/main/git/status-conflict-operations.test.ts b/src/main/git/status-conflict-operations.test.ts index 7185c03a86d..eca50ad04e3 100644 --- a/src/main/git/status-conflict-operations.test.ts +++ b/src/main/git/status-conflict-operations.test.ts @@ -155,25 +155,53 @@ describe('getStatus operationProgress', () => { gitStreamOptionsMock.mockReset() readFileMock.mockReset() statMock.mockReset() - existsSyncMock.mockReset() + accessMock.mockReset() gitExecFileAsyncMock.mockResolvedValue({ stdout: '' }) }) - it('omits operationProgress when a sequencer operation has no rebase state on disk', async () => { + it('omits operationProgress when a rebase has no readable state on disk', async () => { // Only `.git` itself resolves; every rebase-merge/rebase-apply read misses. readFileMock.mockImplementation(async (target: string) => target.endsWith('.git') ? 'gitdir: /repo/.git/worktrees/feature\n' : Promise.reject(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) ) - existsSyncMock.mockImplementation((target: string) => target.endsWith('CHERRY_PICK_HEAD')) + accessMock.mockImplementation(async (target: string) => { + if (target.endsWith('rebase-merge')) { + return undefined + } + throw Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) + }) const result = await getStatus('/repo') - expect(result.conflictOperation).toBe('cherry-pick') + expect(result.conflictOperation).toBe('rebase') expect(result.operationProgress).toBeUndefined() expect('operationProgress' in result).toBe(false) // The reader ran and degraded — it did not skip the state directory. expect(readFileMock).toHaveBeenCalledWith(expect.stringContaining('rebase-merge'), 'utf-8') }) + + // A cherry-pick has no rebase state dir by definition, so probing it would be + // guaranteed ENOENT churn on every poll — the read must not run at all. + it('skips the rebase state read entirely during a cherry-pick', async () => { + readFileMock.mockImplementation(async (target: string) => + target.endsWith('.git') + ? 'gitdir: /repo/.git/worktrees/feature\n' + : Promise.reject(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + ) + accessMock.mockImplementation(async (target: string) => { + if (target.endsWith('CHERRY_PICK_HEAD')) { + return undefined + } + throw Object.assign(new Error(`ENOENT: ${target}`), { code: 'ENOENT' }) + }) + + const result = await getStatus('/repo') + + expect(result.conflictOperation).toBe('cherry-pick') + expect('operationProgress' in result).toBe(false) + expect(readFileMock).not.toHaveBeenCalledWith(expect.stringContaining('rebase-merge'), 'utf-8') + expect(readFileMock).not.toHaveBeenCalledWith(expect.stringContaining('rebase-apply'), 'utf-8') + }) }) diff --git a/src/relay/git-handler-status-ops.ts b/src/relay/git-handler-status-ops.ts index d18df874dfa..d9cf39e7b9e 100644 --- a/src/relay/git-handler-status-ops.ts +++ b/src/relay/git-handler-status-ops.ts @@ -96,10 +96,11 @@ export async function getStatusOp( // Why: reject NaN/negative limits — NaN would silently disable capping, negatives would over-truncate. const limit = resolveGitStatusLimit(params.limit) const conflictPromise = detectConflictOperation(worktreePath) - // Why: only the sequencer operations have state on disk, so chain the read off the probe — a clean repo reads nothing. + // Why: only a rebase has readable step state (rebase-merge/rebase-apply) — a + // cherry-pick's sequencer dir has no reader here, so probing it would be pure ENOENT churn. const operationProgressPromise = conflictPromise .then(async (operation) => - operation === 'rebase' || operation === 'cherry-pick' + operation === 'rebase' ? await readGitRebaseProgress(await resolveGitDir(worktreePath)) : undefined ) diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 5d00bf2320b..d989205df26 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -216,6 +216,38 @@ describe('GitHandler', () => { await expect(readFileText()).resolves.toBe('resolved\n') }) + it('treats a continue that advances into the next conflict as success', async () => { + const baseBranch = seedDivergedBranches() + execFileSync('git', ['checkout', 'feature'], { cwd: tmpDir, stdio: 'pipe' }) + writeFileSync(path.join(tmpDir, 'file.txt'), 'feature two\n') + gitCommit(tmpDir, 'feature change two') + expect(() => + execFileSync('git', ['rebase', baseBranch], { cwd: tmpDir, stdio: 'pipe' }) + ).toThrow() + resolveConflict() + + // Git exits nonzero here (it committed step 1, then stopped on step 2's + // conflict) — the moved HEAD must read as success, exactly like the local path. + await dispatcher.callRequest('git.continueRebase', { worktreePath: tmpDir }) + + await expect(fs.access(path.join(tmpDir, '.git', 'rebase-merge'))).resolves.toBeUndefined() + }) + + it('still rejects a continue that could not advance', async () => { + const baseBranch = seedDivergedBranches() + execFileSync('git', ['checkout', 'feature'], { cwd: tmpDir, stdio: 'pipe' }) + expect(() => + execFileSync('git', ['rebase', baseBranch], { cwd: tmpDir, stdio: 'pipe' }) + ).toThrow() + + // Conflict left unresolved: git refuses, HEAD does not move, the error surfaces. + await expect( + dispatcher.callRequest('git.continueRebase', { worktreePath: tmpDir }) + ).rejects.toThrow() + + await expect(fs.access(path.join(tmpDir, '.git', 'rebase-merge'))).resolves.toBeUndefined() + }) + it('continues a conflicted merge without waiting on an editor', async () => { seedDivergedBranches() expect(() => diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index e032c89f33c..67e2773a747 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -104,19 +104,13 @@ export class GitHandler { (params, context) => this.cancelResponseStream(params, context) ) this.dispatcher.onRequest('git.continueMerge', (p) => - this.sequencerAction(p, ['merge', '--continue']) + this.sequencerAction(p, ['merge', '--continue'], 'MERGE_HEAD') ) this.dispatcher.onRequest('git.continueRebase', (p) => - this.sequencerAction(p, ['rebase', '--continue']) + this.sequencerAction(p, ['rebase', '--continue'], 'REBASE_HEAD') ) this.dispatcher.onRequest('git.continueCherryPick', (p) => - this.sequencerAction(p, ['cherry-pick', '--continue']) - ) - this.dispatcher.onRequest('git.skipRebase', (p) => - this.sequencerAction(p, ['rebase', '--skip']) - ) - this.dispatcher.onRequest('git.skipCherryPick', (p) => - this.sequencerAction(p, ['cherry-pick', '--skip']) + this.sequencerAction(p, ['cherry-pick', '--continue'], 'CHERRY_PICK_HEAD') ) // Why: a detached client's git.responseAck frames never arrive; wake any pump parked on the ack window so it re-checks staleness and exits. this.dispatcher.onClientDetached?.(() => this.responseStreams.wakeAll()) @@ -221,12 +215,35 @@ export class GitHandler { return stdout } + private async readSequencerMarkerOid( + worktreePath: string, + marker: string + ): Promise { + try { + const { stdout } = await this.git(['rev-parse', '-q', '--verify', marker], worktreePath) + return stdout.trim() || null + } catch { + return null + } + } + // Why: sequencer continuations must not open an interactive commit-message editor. - private async sequencerAction(params: Record, args: string[]) { + private async sequencerAction( + params: Record, + args: string[], + marker: string + ) { this.clearGitMutationReadCaches() const worktreePath = params.worktreePath as string + const markerBefore = await this.readSequencerMarkerOid(worktreePath, marker) try { await this.git(args, worktreePath, { suppressEditor: true, terminationBarrier: true }) + } catch (error) { + // Why: --continue may advance to another conflicted step and still exit nonzero. + const markerAfter = await this.readSequencerMarkerOid(worktreePath, marker) + if (!markerBefore || markerAfter === markerBefore) { + throw error + } } finally { this.clearGitMutationReadCaches() } diff --git a/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts index b04fd1bea15..ed8fbb85fd1 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts @@ -56,6 +56,42 @@ describe('SourceControl remote action error reconciliation', () => { ).toBe(errors) }) + it('clears a failed Continue once its operation settles, however it ended', () => { + const errors = { + 'wt-1': { + kind: 'continue_operation' as const, + message: 'Continue rebase failed', + rawError: 'Continue rebase failed' + } + } + + expect( + clearRemoteActionErrorsForCompletedConflictOperations({ + remoteActionErrors: errors, + previousConflictOperations: { 'wt-1': 'rebase' }, + currentConflictOperations: { 'wt-1': 'unknown' } + }) + ).toEqual({ 'wt-1': null }) + }) + + it('keeps a failed Continue while its operation is still in progress', () => { + const errors = { + 'wt-1': { + kind: 'continue_operation' as const, + message: 'Continue cherry-pick failed', + rawError: 'Continue cherry-pick failed' + } + } + + expect( + clearRemoteActionErrorsForCompletedConflictOperations({ + remoteActionErrors: errors, + previousConflictOperations: { 'wt-1': 'cherry-pick' }, + currentConflictOperations: { 'wt-1': 'cherry-pick' } + }) + ).toBe(errors) + }) + it('clears pull and sync conflict errors after their merge operation completes', () => { const errors = { 'wt-pull': { diff --git a/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx b/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx index 474c7972465..ff5ed0d4a33 100644 --- a/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control-header-toolbar-identity.test.tsx @@ -221,4 +221,14 @@ describe('SourceControlHeaderToolbar identity during a rebase', () => { // The base ref itself still shows — only counts measured against a transient commit go away. expect(running).toContain('origin/main') }) + + // A merge keeps HEAD on the real branch, so its counts stay honest — and shown. + it('keeps the ahead/behind counter during a merge', () => { + const merging = renderToolbar({ + headDisplay: { kind: 'branch', branchName: 'triage-e2e' }, + conflictOperation: 'merge' + }) + + expect(merging).toContain('↑1') + }) }) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx index fc9f137588f..7edccfc037d 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx @@ -1,6 +1,7 @@ import React from 'react' import { GitMerge, GitPullRequestArrow, TriangleAlert } from 'lucide-react' import { translate } from '@/i18n/i18n' +import { shortGitHead } from '@/lib/worktree-git-identity-display' import type { GitConflictOperation, GitOperationProgress @@ -49,8 +50,9 @@ function conflictsHeading(conflictOperation: GitConflictOperation): string { } // Full 40-char oids only get truncated by CSS, which reads as a broken value. +// shortGitHead keeps this abbreviation in step with the head identity chip's. function shortenOnto(onto: string | undefined): string | undefined { - return onto && /^[0-9a-f]{40}$/i.test(onto) ? onto.slice(0, 8) : onto + return onto && /^[0-9a-f]{40}$/i.test(onto) ? shortGitHead(onto) : onto } function inProgressHeading(conflictOperation: GitConflictOperation): string { diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner-actions.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner-actions.tsx index 5847731e71f..7f8ba0f06a6 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner-actions.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner-actions.tsx @@ -43,6 +43,8 @@ type OperationAction = { onClick: () => void // Escape hatches never take the primary slot, so they never read as a way forward. quiet?: boolean + // Read-only navigation stays clickable while a mutation is in flight. + enabledWhileBusy?: boolean } const SPINNER = @@ -93,7 +95,8 @@ export function SourceControlOperationBannerActions({ 'Review conflicts' ), icon: , - onClick: onReviewConflicts + onClick: onReviewConflicts, + enabledWhileBusy: true }) } if ((conflictOperation === 'merge' || conflictOperation === 'rebase') && onAbortOperation) { @@ -120,7 +123,7 @@ export function SourceControlOperationBannerActions({ variant={variant} size="sm" className="h-7 w-full min-w-0 text-xs" - disabled={busy} + disabled={busy && !action.enabledWhileBusy} onClick={action.onClick} > {action.icon} diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx index 0468aec261a..3636d4a34c9 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx @@ -48,8 +48,9 @@ describe('OperationBanner heading', () => { operationProgress: { ...progress, onto: 'bc98655a3965fe350f77acb14bf4a53f1e2d3c4b' } }) - expect(markup).toContain('Rebasing onto bc98655a') - expect(markup).not.toContain('bc98655a3965fe350f77acb14bf4a53f1e2d3c4b') + // 7 chars — the same abbreviation the head identity chip uses. + expect(markup).toContain('Rebasing onto bc98655') + expect(markup).not.toContain('bc98655a') }) // Wire compatibility: a host that predates operationProgress omits it entirely. @@ -134,6 +135,16 @@ describe('ConflictSummaryCard', () => { /> ) + // Review opens a read-only view, so it must stay reachable while an agent resolves. + it('keeps Review conflicts clickable while Resolve with AI is in flight', () => { + const markup = renderSummary({ isResolvingWithAI: true }) + + // `disabled=""` is the rendered attribute; a bare "disabled" also matches Tailwind's disabled: variants. + expect(buttonContaining(markup, 'Review conflicts')).not.toContain('disabled=""') + expect(buttonContaining(markup, 'Resolve with AI')).toContain('disabled=""') + expect(buttonContaining(markup, 'Abort rebase')).toContain('disabled=""') + }) + it('offers only Resolve with AI, Review and Abort while conflicts are unresolved', () => { const markup = renderSummary() diff --git a/src/renderer/src/components/right-sidebar/source-control/panel/branch-context-row.tsx b/src/renderer/src/components/right-sidebar/source-control/panel/branch-context-row.tsx index a4af4f5e520..7c693b72883 100644 --- a/src/renderer/src/components/right-sidebar/source-control/panel/branch-context-row.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/panel/branch-context-row.tsx @@ -335,10 +335,12 @@ export function SourceControlBranchContextRow({ ) } + // Why: only a rebase moves HEAD onto transient replay commits; a merge or + // cherry-pick keeps HEAD on the branch, so its counts stay honest and stay shown. const compareStatNodes = buildSourceControlCompareBaseStats( summary, displayedBaseRef, - conflictOperation !== undefined && conflictOperation !== 'unknown' + conflictOperation === 'rebase' ).map((stat) => ) return ( diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/remote-refresh.ts b/src/renderer/src/components/right-sidebar/source-control/sync/remote-refresh.ts index 0571ab25625..e0c41c8e372 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/remote-refresh.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/remote-refresh.ts @@ -47,6 +47,10 @@ function remoteActionErrorMatchesSettledConflictOperation( if (kind === 'pull' || kind === 'sync') { return operation === 'merge' || operation === 'rebase' } + // Why: a failed Continue is moot once its operation is gone, however it ended. + if (kind === 'continue_operation') { + return operation === 'merge' || operation === 'rebase' || operation === 'cherry-pick' + } return false } diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-abort.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-abort.ts index 6e7a16ca040..4dc6f593565 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-abort.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-abort.ts @@ -1,14 +1,12 @@ import { useCallback } from 'react' -import { toast } from 'sonner' import { useConfirmationDialog } from '@/components/confirmation-dialog-context' import { translate } from '@/i18n/i18n' -import { getConnectionId } from '@/lib/connection-context' import { abortRuntimeGitMerge, abortRuntimeGitRebase } from '@/runtime/runtime-git-client' import type { GitConflictOperation } from '../../../../../../shared/git-status-types' import type { AbortConflictOperation } from '../listing/operation-target' import type { SourceControlWorktreeContext } from '../listing/use-worktree-context' import type { SourceControlWorktreeOperationState } from '../panel/use-worktree-operation-state' -import { refreshSourceControlAfterRemoteAction } from './remote-refresh' +import { useSourceControlConflictOperationRunner } from './use-conflict-operation-runner' import type { SourceControlStatusRefresh } from './use-status-refresh' /** @@ -38,9 +36,23 @@ export function useSourceControlConflictAbort({ worktreePath: string | null }) { const confirmAction = useConfirmationDialog() + const runConflictOperation = useSourceControlConflictOperationRunner({ + activeRepoSettings, + activeWorktreeId, + conflictOperation, + isBlocked: isAbortingOperation, + refreshActiveGitStatusAfterMutation, + refreshBranchCompareRef, + refreshGitHistoryRef, + setInFlightByWorktree: setAbortOperationInFlightByWorktree, + setRemoteActionErrors, + worktreePath + }) const handleAbortOperation = useCallback( async (requestedOperation: AbortConflictOperation): Promise => { + // Why: re-checked by the runner, but guarded here too so a mismatched or + // already-running operation never even shows the confirmation dialog. if ( !activeWorktreeId || !worktreePath || @@ -66,57 +78,24 @@ export function useSourceControlConflictAbort({ return } - const connectionId = getConnectionId(activeWorktreeId) ?? undefined - setAbortOperationInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: true })) - setRemoteActionErrors((prev) => ({ ...prev, [activeWorktreeId]: null })) - try { - const context = { - // Why: route the abort by the repo OWNER host, not the focused runtime. - settings: activeRepoSettings, - worktreeId: activeWorktreeId, - worktreePath, - connectionId - } - const abortGitOperation = isRebase ? abortRuntimeGitRebase : abortRuntimeGitMerge - await abortGitOperation(context) - } catch (error) { - const message = error instanceof Error ? error.message : `Failed to abort ${label}` - toast.error( - translate( - 'auto.components.right.sidebar.SourceControl.f99560ab29', - 'Abort {{value0}} failed', - { value0: label } - ), - { description: message } - ) - setRemoteActionErrors((prev) => ({ - ...prev, - [activeWorktreeId]: { - kind: isRebase ? 'abort_rebase' : 'abort_merge', - message, - rawError: message - } - })) - } finally { - setAbortOperationInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) - refreshSourceControlAfterRemoteAction({ - refreshGitStatus: refreshActiveGitStatusAfterMutation, - refreshBranchCompare: refreshBranchCompareRef.current, - refreshGitHistory: refreshGitHistoryRef.current - }) - } + await runConflictOperation({ + requestedOperation, + errorKind: isRebase ? 'abort_rebase' : 'abort_merge', + failureToast: translate( + 'auto.components.right.sidebar.SourceControl.f99560ab29', + 'Abort {{value0}} failed', + { value0: label } + ), + fallbackMessage: `Failed to abort ${label}`, + run: (context) => (isRebase ? abortRuntimeGitRebase : abortRuntimeGitMerge)(context) + }) }, [ - activeRepoSettings, activeWorktreeId, confirmAction, conflictOperation, isAbortingOperation, - refreshActiveGitStatusAfterMutation, - refreshBranchCompareRef, - refreshGitHistoryRef, - setAbortOperationInFlightByWorktree, - setRemoteActionErrors, + runConflictOperation, worktreePath ] ) diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-advance.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-advance.ts index f5fc8024f8d..43343d461f4 100644 --- a/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-advance.ts +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-advance.ts @@ -1,7 +1,5 @@ import { useCallback } from 'react' -import { toast } from 'sonner' import { translate } from '@/i18n/i18n' -import { getConnectionId } from '@/lib/connection-context' import { continueRuntimeGitCherryPick, continueRuntimeGitMerge, @@ -10,7 +8,7 @@ import { import type { GitConflictOperation } from '../../../../../../shared/git-status-types' import type { SourceControlWorktreeContext } from '../listing/use-worktree-context' import type { SourceControlWorktreeOperationState } from '../panel/use-worktree-operation-state' -import { refreshSourceControlAfterRemoteAction } from './remote-refresh' +import { useSourceControlConflictOperationRunner } from './use-conflict-operation-runner' import type { SourceControlStatusRefresh } from './use-status-refresh' const CONTINUE_RUNNERS = { @@ -45,81 +43,37 @@ export function useSourceControlConflictAdvance({ setRemoteActionErrors: SourceControlWorktreeOperationState['setRemoteActionErrors'] worktreePath: string | null }) { - const runAdvance = useCallback( - async (requestedOperation: GitConflictOperation): Promise => { - if ( - !activeWorktreeId || - !worktreePath || - conflictOperation !== requestedOperation || - isAdvancingOperation || - isAbortingOperation - ) { - return - } - const runner = CONTINUE_RUNNERS[requestedOperation as keyof typeof CONTINUE_RUNNERS] - if (!runner) { - return - } - - const connectionId = getConnectionId(activeWorktreeId) ?? undefined - setAdvanceOperationInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: true })) - setRemoteActionErrors((prev) => ({ ...prev, [activeWorktreeId]: null })) - try { - await runner({ - // Why: route by the repo OWNER host, not the focused runtime. - settings: activeRepoSettings, - worktreeId: activeWorktreeId, - worktreePath, - connectionId - }) - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - toast.error( - translate( - 'auto.components.right.sidebar.source.control.sync.use.conflict.advance.b84fcd7ea6', - 'Continue {{value0}} failed', - { value0: requestedOperation } - ), - { description: message } - ) - setRemoteActionErrors((prev) => ({ - ...prev, - [activeWorktreeId]: { - kind: 'continue_operation', - message, - rawError: message - } - })) - } finally { - setAdvanceOperationInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) - // Why: continue can land straight in a NEW conflict, so the banner must re-read status. - refreshSourceControlAfterRemoteAction({ - refreshGitStatus: refreshActiveGitStatusAfterMutation, - refreshBranchCompare: refreshBranchCompareRef.current, - refreshGitHistory: refreshGitHistoryRef.current - }) - } - }, - [ - activeRepoSettings, - activeWorktreeId, - conflictOperation, - isAbortingOperation, - isAdvancingOperation, - refreshActiveGitStatusAfterMutation, - refreshBranchCompareRef, - refreshGitHistoryRef, - setAdvanceOperationInFlightByWorktree, - setRemoteActionErrors, - worktreePath - ] - ) + const runConflictOperation = useSourceControlConflictOperationRunner({ + activeRepoSettings, + activeWorktreeId, + conflictOperation, + isBlocked: isAdvancingOperation || isAbortingOperation, + refreshActiveGitStatusAfterMutation, + refreshBranchCompareRef, + refreshGitHistoryRef, + setInFlightByWorktree: setAdvanceOperationInFlightByWorktree, + setRemoteActionErrors, + worktreePath + }) const handleContinueOperation = useCallback( (operation: GitConflictOperation): void => { - void runAdvance(operation) + const runner = CONTINUE_RUNNERS[operation as keyof typeof CONTINUE_RUNNERS] + if (!runner) { + return + } + void runConflictOperation({ + requestedOperation: operation, + errorKind: 'continue_operation', + failureToast: translate( + 'auto.components.right.sidebar.source.control.sync.use.conflict.advance.b84fcd7ea6', + 'Continue {{value0}} failed', + { value0: operation } + ), + run: (context) => runner(context) + }) }, - [runAdvance] + [runConflictOperation] ) return { handleContinueOperation } diff --git a/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-operation-runner.ts b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-operation-runner.ts new file mode 100644 index 00000000000..4676c830289 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/sync/use-conflict-operation-runner.ts @@ -0,0 +1,114 @@ +import { useCallback } from 'react' +import { toast } from 'sonner' +import { getConnectionId } from '@/lib/connection-context' +import type { GitConflictOperation } from '../../../../../../shared/git-status-types' +import type { SourceControlWorktreeContext } from '../listing/use-worktree-context' +import type { SourceControlWorktreeOperationState } from '../panel/use-worktree-operation-state' +import type { SourceControlActionErrorKind } from './action-error' +import { refreshSourceControlAfterRemoteAction } from './remote-refresh' +import type { SourceControlStatusRefresh } from './use-status-refresh' + +export type ConflictOperationRunContext = { + settings: SourceControlWorktreeContext['activeRepoSettings'] + worktreeId: string + worktreePath: string + connectionId: string | undefined +} + +export type ConflictOperationRunSpec = { + requestedOperation: GitConflictOperation + errorKind: SourceControlActionErrorKind + /** Already-translated toast title for the failure case. */ + failureToast: string + /** Message when the thrown value is not an Error; defaults to String(error). */ + fallbackMessage?: string + run: (context: ConflictOperationRunContext) => Promise +} + +/** + * Shared choreography for the conflict banner's mutating actions (Continue, Abort): + * per-worktree in-flight flag, error record, failure toast, and the post-action + * status/compare/history refresh. Only the operation-specific work arrives as `run`. + */ +export function useSourceControlConflictOperationRunner({ + activeRepoSettings, + activeWorktreeId, + conflictOperation, + isBlocked, + refreshActiveGitStatusAfterMutation, + refreshBranchCompareRef, + refreshGitHistoryRef, + setInFlightByWorktree, + setRemoteActionErrors, + worktreePath +}: { + activeRepoSettings: SourceControlWorktreeContext['activeRepoSettings'] + activeWorktreeId: string | null + conflictOperation: GitConflictOperation + /** True while another banner action is already running. */ + isBlocked: boolean + refreshActiveGitStatusAfterMutation: SourceControlStatusRefresh['refreshActiveGitStatusAfterMutation'] + refreshBranchCompareRef: React.RefObject<() => Promise> + refreshGitHistoryRef: React.RefObject<() => Promise> + setInFlightByWorktree: SourceControlWorktreeOperationState['setAbortOperationInFlightByWorktree'] + setRemoteActionErrors: SourceControlWorktreeOperationState['setRemoteActionErrors'] + worktreePath: string | null +}) { + return useCallback( + async (spec: ConflictOperationRunSpec): Promise => { + if ( + !activeWorktreeId || + !worktreePath || + conflictOperation !== spec.requestedOperation || + isBlocked + ) { + return + } + + const connectionId = getConnectionId(activeWorktreeId) ?? undefined + setInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: true })) + setRemoteActionErrors((prev) => ({ ...prev, [activeWorktreeId]: null })) + try { + await spec.run({ + // Why: route by the repo OWNER host, not the focused runtime. + settings: activeRepoSettings, + worktreeId: activeWorktreeId, + worktreePath, + connectionId + }) + } catch (error) { + const message = + error instanceof Error ? error.message : (spec.fallbackMessage ?? String(error)) + toast.error(spec.failureToast, { description: message }) + setRemoteActionErrors((prev) => ({ + ...prev, + [activeWorktreeId]: { + kind: spec.errorKind, + message, + rawError: message + } + })) + } finally { + setInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) + // Why: the action can land straight in a NEW conflict, so the banner must re-read status. + refreshSourceControlAfterRemoteAction({ + refreshGitStatus: refreshActiveGitStatusAfterMutation, + refreshBranchCompare: refreshBranchCompareRef.current, + refreshGitHistory: refreshGitHistoryRef.current + }) + } + }, + [ + activeRepoSettings, + activeWorktreeId, + conflictOperation, + isBlocked, + refreshActiveGitStatusAfterMutation, + refreshBranchCompareRef, + refreshGitHistoryRef, + setInFlightByWorktree, + setRemoteActionErrors, + worktreePath + ] + ) +} diff --git a/src/renderer/src/runtime/runtime-git-sync-client.ts b/src/renderer/src/runtime/runtime-git-sync-client.ts index 5d8efd19312..2550805fa7c 100644 --- a/src/renderer/src/runtime/runtime-git-sync-client.ts +++ b/src/renderer/src/runtime/runtime-git-sync-client.ts @@ -2,6 +2,7 @@ import type { GitForkSyncExpectedUpstream, GitForkSyncResult } from '../../../sh import type { GitUpstreamStatus } from '../../../shared/git-status-types' import { REBASE_FROM_BASE_RPC_TIMEOUT_MS } from '../../../shared/git-rebase-source' import type { GitPushTarget } from '../../../shared/worktree/types' +import { isRuntimeMethodNotFoundError } from '@/store/slices/worktrees/listing/runtime-worktree-rpc-errors' import { resolveLocalWorktreePath, type RuntimeGitContext } from './runtime-git-client-context' import { callRuntimeRpc, getActiveRuntimeTarget } from './runtime-rpc-client' import { toRuntimeWorktreeSelector } from './runtime-worktree-selector' @@ -49,11 +50,11 @@ export async function continueRuntimeGitMerge(context: RuntimeGitContext): Promi }) return } - await callRuntimeRpc( + await callSequencerContinueRpc( target, 'git.continueMerge', - { worktree: toRuntimeWorktreeSelector(context.worktreeId) }, - { timeoutMs: 30_000 } + context.worktreeId, + 'continue a merge' ) } @@ -66,11 +67,11 @@ export async function continueRuntimeGitRebase(context: RuntimeGitContext): Prom }) return } - await callRuntimeRpc( + await callSequencerContinueRpc( target, 'git.continueRebase', - { worktree: toRuntimeWorktreeSelector(context.worktreeId) }, - { timeoutMs: 30_000 } + context.worktreeId, + 'continue a rebase' ) } @@ -83,14 +84,39 @@ export async function continueRuntimeGitCherryPick(context: RuntimeGitContext): }) return } - await callRuntimeRpc( + await callSequencerContinueRpc( target, 'git.continueCherryPick', - { worktree: toRuntimeWorktreeSelector(context.worktreeId) }, - { timeoutMs: 30_000 } + context.worktreeId, + 'continue a cherry-pick' ) } +// Why: mixed client/host versions are the normal state; the raw method-not-found +// text would otherwise surface verbatim in the failure toast. +async function callSequencerContinueRpc( + target: Parameters[0], + method: string, + worktreeId: string, + action: string +): Promise { + try { + await callRuntimeRpc( + target, + method, + { worktree: toRuntimeWorktreeSelector(worktreeId) }, + { timeoutMs: 30_000 } + ) + } catch (error) { + if (isRuntimeMethodNotFoundError(error)) { + throw new Error( + `This remote Orca host is running an older version that cannot ${action}. Update Orca on the host, then try again.` + ) + } + throw error + } +} + export async function getRuntimeGitUpstreamStatus( context: RuntimeGitContext, pushTarget?: GitPushTarget