From be2f9eddd39e668e19f9b0dd5fc3352125de8229 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:45:39 -0700 Subject: [PATCH 01/18] fix(mobile): abandon pairing journals that can no longer reconcile (#12773) --- .../mobile-relay-pairing-recovery.test.ts | 49 +++++++++++++++++++ .../mobile-relay-pairing-recovery.ts | 25 +++++++++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/mobile/src/transport/mobile-relay-pairing-recovery.test.ts b/mobile/src/transport/mobile-relay-pairing-recovery.test.ts index 41d53189880..96d71b37680 100644 --- a/mobile/src/transport/mobile-relay-pairing-recovery.test.ts +++ b/mobile/src/transport/mobile-relay-pairing-recovery.test.ts @@ -220,4 +220,53 @@ describe('mobile relay pairing recovery', () => { expect(saved.metadata.authorizationMode).toBe('authenticated-direct') expect(deps.updateJournal).toHaveBeenCalledTimes(2) }) + // Why: a journal stranded by a relay outage used to block every later pairing + // with "recovery pending" forever, because recovery only ever deferred. + it('abandons a journal once its invite expired and no credential can reconcile', async () => { + const saved = journal() + const unreachable = client(async () => { + throw new Error('relay unreachable') + }) + const deps = { + ...dependencies({ journal: saved, connectRelay: vi.fn(() => unreachable) }), + now: () => saved.metadata.relay.inviteExpiresAt + 10 * 60 * 1000 + 1 + } + + await expect(recoverMobileRelayPairing(deps)).resolves.toBe('abandoned') + expect(deps.clearJournal).toHaveBeenCalledWith(saved.metadata.journalId) + }) + + it('keeps a just-expired journal so a brief outage cannot discard it', async () => { + const saved = journal() + const unreachable = client(async () => { + throw new Error('relay unreachable') + }) + const deps = { + ...dependencies({ journal: saved, connectRelay: vi.fn(() => unreachable) }), + now: () => saved.metadata.relay.inviteExpiresAt + 1 + } + + await expect(recoverMobileRelayPairing(deps)).resolves.toBe('deferred') + expect(deps.clearJournal).not.toHaveBeenCalled() + }) + + // Why: a committed install whose local persistence failed is the one case the + // journal must survive — it is the only record left to retry the write from. + it('keeps a journal when the server committed but the local write failed', async () => { + const saved = journal() + const directInstalled = installed(saved, 'authenticated-direct') + const committed = client(async () => + response(endpoints(saved, { state: 'committed', result: directInstalled })) + ) + const deps = { + ...dependencies({ journal: saved, connectRelay: vi.fn(() => committed) }), + now: () => saved.metadata.relay.inviteExpiresAt + 10 * 60 * 1000 + 1, + writeCredentialBundle: vi.fn(async () => { + throw new Error('keychain unavailable') + }) + } + + await expect(recoverMobileRelayPairing(deps)).resolves.toBe('deferred') + expect(deps.clearJournal).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/transport/mobile-relay-pairing-recovery.ts b/mobile/src/transport/mobile-relay-pairing-recovery.ts index 0d5ba7e5e07..26dc08620de 100644 --- a/mobile/src/transport/mobile-relay-pairing-recovery.ts +++ b/mobile/src/transport/mobile-relay-pairing-recovery.ts @@ -27,7 +27,7 @@ import { import { createRecoveringPairingRelayCandidate } from './pairing-relay-candidate' import type { HostProfile, RpcResponse } from './types' -export type MobileRelayPairingRecoveryResult = 'none' | 'recovered' | 'deferred' +export type MobileRelayPairingRecoveryResult = 'none' | 'recovered' | 'deferred' | 'abandoned' type RecoveryDependencies = { loadJournal: typeof loadMobileRelayPairingJournal @@ -57,6 +57,10 @@ const defaultDependencies: RecoveryDependencies = { platform: Platform.OS } +// One full invite lifetime past expiry, so a momentary outage never discards a +// journal that a later launch could still reconcile. +const ABANDON_GRACE_MS = 10 * 60 * 1000 + let recoveryPromise: Promise | null = null export function recoverMobileRelayPairing( @@ -96,6 +100,10 @@ async function runRecovery( } const credentials = recoveryCredentials(journal, bundle, dependencies.now()) + // Why: publishCommitted runs inside the catch below, so a failed local write + // of an authoritatively committed install must not look like "nothing to + // reconcile" — that journal is the only record left to retry the write from. + let observedCommitted = false for (const credential of credentials) { let client: PairingCandidateClient | null = null try { @@ -113,6 +121,7 @@ async function runRecovery( }) const endpoints = await getRecoveryStatus(client, journal, credential.kind) if (endpoints.installStatus?.state === 'committed') { + observedCommitted = true await publishCommitted(journal, endpoints, dependencies) return 'recovered' } @@ -128,6 +137,7 @@ async function runRecovery( ) const reconciled = await getRecoveryStatus(client, journal, 'invite') assertCommitted(reconciled, installed) + observedCommitted = true await publishCommitted(journal, reconciled, dependencies) return 'recovered' } @@ -138,6 +148,19 @@ async function runRecovery( client?.close() } } + // Why: past invite expiry no credential can still establish what happened, so + // retaining the journal cannot reconcile anything — it only fails every later + // pairing with "recovery pending" forever. Re-pairing mints a fresh device and + // any uncommitted server-side install expires on its own. The extra invite + // lifetime of slack keeps a brief relay outage from discarding a journal whose + // resume credential would have reconciled it on the next launch. + if ( + !observedCommitted && + journal.metadata.relay.inviteExpiresAt + ABANDON_GRACE_MS <= dependencies.now() + ) { + await dependencies.clearJournal(journal.metadata.journalId).catch(() => {}) + return 'abandoned' + } return 'deferred' } From 73cd4c3f46576b191a7625373bdc628120650dcf Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:52:14 -0700 Subject: [PATCH 02/18] fix(mobile): bound live terminal input latency (#12763) --- .../app/h/[hostId]/session/[worktreeId].tsx | 1 + .../session/MobileTerminalLiveInputStatus.tsx | 8 +- .../terminal-live-input-affordance.test.ts | 3 + .../terminal-live-pending-flush-state.test.ts | 77 ++++++++++-- .../terminal-live-pending-flush-state.ts | 111 +++++++++++++++--- .../use-terminal-live-pending-input-flush.ts | 24 +++- 6 files changed, 184 insertions(+), 40 deletions(-) diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index c5e4175acc0..755d6605501 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -4986,6 +4986,7 @@ export default function SessionScreen() { {title} - + {detail} diff --git a/mobile/src/terminal/terminal-live-input-affordance.test.ts b/mobile/src/terminal/terminal-live-input-affordance.test.ts index 0125cb053e6..ce360091ce2 100644 --- a/mobile/src/terminal/terminal-live-input-affordance.test.ts +++ b/mobile/src/terminal/terminal-live-input-affordance.test.ts @@ -39,6 +39,7 @@ describe('terminal live input affordance', () => { expect(block).toContain('pressed && styles.liveInputFocusTargetPressed') expect(block).toContain('!canSend && styles.liveInputFocusTargetDisabled') expect(block).toContain('showSoftInputOnFocus') + expect(block).toContain('liveInputText={liveInputCapture}') expect(sessionRouteSource).toContain('useTerminalLiveInputFocus({') expect(sessionRouteSource).toContain('return resetLiveInputFocus') expect(liveInputFocusSource).toContain('focusTerminalLiveInputTarget(inputRef.current') @@ -52,6 +53,8 @@ describe('terminal live input affordance', () => { it('makes the live keyboard target visible instead of status-only chrome', () => { expect(liveInputStatusSource).toContain("'Tap to show keyboard'") + expect(liveInputStatusSource).toContain("liveInputText || 'Tap to show keyboard'") + expect(liveInputStatusSource).toContain('ellipsizeMode="head"') expect(commandInputStylesSource).toContain('backgroundColor: colors.bgRaised') expect(commandInputStylesSource).toContain('borderWidth: 1') expect(commandInputStylesSource).toContain('liveInputFocusTargetPressed') diff --git a/mobile/src/terminal/terminal-live-pending-flush-state.test.ts b/mobile/src/terminal/terminal-live-pending-flush-state.test.ts index 88918c00681..a77d4315c29 100644 --- a/mobile/src/terminal/terminal-live-pending-flush-state.test.ts +++ b/mobile/src/terminal/terminal-live-pending-flush-state.test.ts @@ -1,15 +1,16 @@ import { describe, expect, it } from 'vitest' import { sendTerminalLiveControlAfterPendingFlush } from './terminal-live-control-send-order' import { + cancelTerminalLivePendingFlush, + createTerminalLivePendingFlushState, queueTerminalLiveMirrorSend, - waitForTerminalLivePendingFlush, - type TerminalLivePendingFlushState + waitForTerminalLivePendingFlush } from './terminal-live-pending-flush-state' describe('terminal live pending flush state', () => { it('Given no in-flight flush When waiting for the barrier Then allows control input', async () => { // Given - const state: TerminalLivePendingFlushState = { current: null } + const state = createTerminalLivePendingFlushState() // When / Then await expect(waitForTerminalLivePendingFlush(state)).resolves.toBe(true) @@ -22,7 +23,8 @@ describe('terminal live pending flush state', () => { const flushPromise = new Promise((resolve) => { resolveFlush = resolve }) - const state: TerminalLivePendingFlushState = { current: flushPromise } + const state = createTerminalLivePendingFlushState() + state.current = flushPromise // When const controlSend = sendTerminalLiveControlAfterPendingFlush( @@ -48,7 +50,8 @@ describe('terminal live pending flush state', () => { const flushPromise = new Promise((resolve) => { resolveFlush = resolve }) - const state: TerminalLivePendingFlushState = { current: flushPromise } + const state = createTerminalLivePendingFlushState() + state.current = flushPromise // When const controlSend = sendTerminalLiveControlAfterPendingFlush( @@ -67,17 +70,45 @@ describe('terminal live pending flush state', () => { }) describe('terminal live mirror send queue', () => { + it('Given high RTT When more input queues Then pending bytes share one follow-up send', async () => { + // Given + const state = createTerminalLivePendingFlushState() + const payloads: string[] = [] + let resolveFirstSend: (value: boolean) => void = () => {} + const sender = async (_handle: string, payload: string): Promise => { + payloads.push(payload) + if (payloads.length === 1) { + return new Promise((resolve) => { + resolveFirstSend = resolve + }) + } + return true + } + + // When + const first = queueTerminalLiveMirrorSend(state, 'terminal-1', 'a', sender) + const second = queueTerminalLiveMirrorSend(state, 'terminal-1', 'b', sender) + const third = queueTerminalLiveMirrorSend(state, 'terminal-1', 'c', sender) + await Promise.resolve() + + // Then + expect(payloads).toEqual(['a']) + resolveFirstSend(true) + await expect(Promise.all([first, second, third])).resolves.toEqual([true, true, true]) + expect(payloads).toEqual(['a', 'bc']) + }) + it('Given a failed previous send When a mirror send queues Then it still runs in order', async () => { // Given - const state: TerminalLivePendingFlushState = { current: null } + const state = createTerminalLivePendingFlushState() const order: string[] = [] - const first = queueTerminalLiveMirrorSend(state, async () => { + const first = queueTerminalLiveMirrorSend(state, 'terminal-1', 'first', async () => { order.push('first') return false }) // When - const second = queueTerminalLiveMirrorSend(state, async () => { + const second = queueTerminalLiveMirrorSend(state, 'terminal-1', 'second', async () => { order.push('second') return true }) @@ -90,13 +121,13 @@ describe('terminal live mirror send queue', () => { it('Given a throwing send When a mirror send queues Then the promise resolves false and the chain continues', async () => { // Given - const state: TerminalLivePendingFlushState = { current: null } - const first = queueTerminalLiveMirrorSend(state, async () => { + const state = createTerminalLivePendingFlushState() + const first = queueTerminalLiveMirrorSend(state, 'terminal-1', 'first', async () => { throw new Error('boom') }) // When - const second = queueTerminalLiveMirrorSend(state, async () => true) + const second = queueTerminalLiveMirrorSend(state, 'terminal-1', 'second', async () => true) // Then await expect(first).resolves.toBe(false) @@ -105,13 +136,33 @@ describe('terminal live mirror send queue', () => { it('Given a settled mirror send When it was the newest Then the state resets to null', async () => { // Given - const state: TerminalLivePendingFlushState = { current: null } + const state = createTerminalLivePendingFlushState() // When - await queueTerminalLiveMirrorSend(state, async () => true) + await queueTerminalLiveMirrorSend(state, 'terminal-1', 'payload', async () => true) await Promise.resolve() // Then expect(state.current).toBeNull() }) + + it('Given queued input When the queue is cancelled Then unsent input is dropped', async () => { + // Given + const state = createTerminalLivePendingFlushState() + let resolveSend: (value: boolean) => void = () => {} + const sender = async (): Promise => + new Promise((resolve) => { + resolveSend = resolve + }) + const active = queueTerminalLiveMirrorSend(state, 'terminal-1', 'a', sender) + const pending = queueTerminalLiveMirrorSend(state, 'terminal-1', 'b', sender) + + // When + cancelTerminalLivePendingFlush(state) + + // Then + await expect(Promise.all([active, pending])).resolves.toEqual([false, false]) + expect(state.current).toBeNull() + resolveSend(true) + }) }) diff --git a/mobile/src/terminal/terminal-live-pending-flush-state.ts b/mobile/src/terminal/terminal-live-pending-flush-state.ts index 405198964f3..fe0b4927f5a 100644 --- a/mobile/src/terminal/terminal-live-pending-flush-state.ts +++ b/mobile/src/terminal/terminal-live-pending-flush-state.ts @@ -1,5 +1,30 @@ +type TerminalLiveMirrorSender = (handle: string, payload: string) => Promise + +type TerminalLivePendingRequest = { + readonly resolve: (sent: boolean) => void +} + +type TerminalLivePendingBatch = { + readonly handle: string + payload: string + readonly requests: TerminalLivePendingRequest[] + readonly sender: TerminalLiveMirrorSender +} + export type TerminalLivePendingFlushState = { current: Promise | null + activeRequests: TerminalLivePendingRequest[] + generation: number + pendingBatches: TerminalLivePendingBatch[] +} + +export function createTerminalLivePendingFlushState(): TerminalLivePendingFlushState { + return { + current: null, + activeRequests: [], + generation: 0, + pendingBatches: [] + } } export function waitForTerminalLivePendingFlush( @@ -8,26 +33,76 @@ export function waitForTerminalLivePendingFlush( return state.current ?? Promise.resolve(true) } -// Why: mirror payloads are erase/append deltas against the PTY echo. A skipped -// delta desyncs every later diff, so this chain runs each send even when the -// previous one failed. state.current should never reject; the catch keeps a -// future raw assignment from skipping a delta. +export function cancelTerminalLivePendingFlush(state: TerminalLivePendingFlushState): void { + state.generation += 1 + const requests = [ + ...state.activeRequests, + ...state.pendingBatches.flatMap((batch) => batch.requests) + ] + state.activeRequests = [] + state.pendingBatches = [] + state.current = null + requests.forEach(({ resolve }) => resolve(false)) +} + +async function drainTerminalLiveMirrorSends( + state: TerminalLivePendingFlushState, + generation: number +): Promise { + let allSent = true + while (state.generation === generation) { + const batch = state.pendingBatches.shift() + if (!batch) { + state.current = null + return allSent + } + + state.activeRequests = batch.requests + const sent = await batch.sender(batch.handle, batch.payload).catch(() => false) + if (state.generation !== generation) { + return false + } + + state.activeRequests = [] + batch.requests.forEach(({ resolve }) => resolve(sent)) + allSent &&= sent + } + return false +} + +// Mirror deltas are ordered PTY bytes; batching pending bytes avoids one RTT per keystroke. export function queueTerminalLiveMirrorSend( state: TerminalLivePendingFlushState, - sendMirrorPayload: () => Promise + handle: string, + payload: string, + sender: TerminalLiveMirrorSender ): Promise { - const previousSend = state.current - const sendPromise = (async () => { - if (previousSend) { - await previousSend.catch(() => false) - } - return sendMirrorPayload() - })().catch(() => false) - state.current = sendPromise - void sendPromise.then(() => { - if (state.current === sendPromise) { - state.current = null - } + let resolveRequest: (sent: boolean) => void = () => {} + const request = new Promise((resolve) => { + resolveRequest = resolve }) - return sendPromise + const pendingTail = state.pendingBatches.at(-1) + if (pendingTail?.handle === handle && pendingTail.sender === sender) { + pendingTail.payload += payload + pendingTail.requests.push({ resolve: resolveRequest }) + } else { + state.pendingBatches.push({ + handle, + payload, + requests: [{ resolve: resolveRequest }], + sender + }) + } + + if (!state.current) { + const generation = state.generation + const drain = drainTerminalLiveMirrorSends(state, generation).catch(() => false) + state.current = drain + void drain.then(() => { + if (state.current === drain) { + state.current = null + } + }) + } + return request } diff --git a/mobile/src/terminal/use-terminal-live-pending-input-flush.ts b/mobile/src/terminal/use-terminal-live-pending-input-flush.ts index f6239312947..05617442142 100644 --- a/mobile/src/terminal/use-terminal-live-pending-input-flush.ts +++ b/mobile/src/terminal/use-terminal-live-pending-input-flush.ts @@ -7,6 +7,8 @@ import { TERMINAL_LIVE_HELD_SYLLABLE_COMMIT_DELAY_MS } from './terminal-live-hangul-mirror' import { + cancelTerminalLivePendingFlush, + createTerminalLivePendingFlushState, queueTerminalLiveMirrorSend, waitForTerminalLivePendingFlush } from './terminal-live-pending-flush-state' @@ -39,7 +41,7 @@ export function useTerminalLivePendingInputFlush({ setLiveInputCapture }: TerminalLivePendingInputFlushOptions): TerminalLivePendingInputFlush { const heldCommitTimerRef = useRef | null>(null) - const pendingLiveInputFlushRef = useRef | null>(null) + const pendingLiveInputFlushRef = useRef(createTerminalLivePendingFlushState()) const heldLiveInputTextRef = useRef('') const sentLiveInputTextRef = useRef('') const pendingLiveInputHandleRef = useRef(null) @@ -56,6 +58,7 @@ export function useTerminalLivePendingInputFlush({ const resetMirrorState = useCallback(() => { clearHeldCommitTimer() + cancelTerminalLivePendingFlush(pendingLiveInputFlushRef.current) heldLiveInputTextRef.current = '' sentLiveInputTextRef.current = '' pendingLiveInputHandleRef.current = null @@ -68,9 +71,15 @@ export function useTerminalLivePendingInputFlush({ }, [liveInputRef, resetMirrorState, setLiveInputCapture]) const waitForPendingLiveInputFlush = useCallback(async (): Promise => { - return waitForTerminalLivePendingFlush(pendingLiveInputFlushRef) + return waitForTerminalLivePendingFlush(pendingLiveInputFlushRef.current) }, []) + const sendQueuedMirrorPayload = useCallback( + (handle: string, payload: string): Promise => + sendLiveTerminalInputRef.current(handle, payload), + [sendLiveTerminalInputRef] + ) + const runMirrorStep = useCallback( async (handle: string, fieldText: string, commitHeld: boolean): Promise => { if ( @@ -107,8 +116,11 @@ export function useTerminalLivePendingInputFlush({ if (payload.length === 0) { return waitForPendingLiveInputFlush() } - return queueTerminalLiveMirrorSend(pendingLiveInputFlushRef, () => - sendLiveTerminalInputRef.current(handle, payload) + return queueTerminalLiveMirrorSend( + pendingLiveInputFlushRef.current, + handle, + payload, + sendQueuedMirrorPayload ) }, [ @@ -117,7 +129,7 @@ export function useTerminalLivePendingInputFlush({ clearHeldCommitTimer, liveInputTerminalHandlesRef, resetMirrorState, - sendLiveTerminalInputRef, + sendQueuedMirrorPayload, waitForPendingLiveInputFlush ] ) @@ -164,7 +176,7 @@ export function useTerminalLivePendingInputFlush({ heldLiveInputTextRef.current = '' sentLiveInputTextRef.current = '' pendingLiveInputHandleRef.current = null - pendingLiveInputFlushRef.current = null + cancelTerminalLivePendingFlush(pendingLiveInputFlushRef.current) } }, []) From debf4affe7ed2dcbb4ad9fdad94a87bd92745831 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:46:00 -0700 Subject: [PATCH 03/18] Display total lines of code change in branch header (#12771) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add branch line total chip to source control header Display the total lines added and removed across a branch from its fork point, measured via `git diff `. Only computed when the chip is visible (request gate on merge base OID), with 500ms soft deadline to protect status latency and 15s hard timeout. Deduplicated across concurrent pollers and cached alongside line stats. Omitted on failure — always shows exact or nothing, never a partial estimate. Updates throughout the stack: native git status, relay, renderer store/API, and UI components. * Pin branch line total to app locale Format line counts using the app's configured locale instead of the system locale, ensuring consistent cross-platform display and test reliability. * test: wait for coalescer joins instead of fixed sleep Hold the diff until the second status pass actually takes the branch-total coalescer lease instead of using a fixed 400ms sleep. Fixes timing-dependent flakiness on slow machines. --- src/main/git/git-status-read-lease-owner.ts | 104 +--- ...us-branch-line-total-exec-contract.test.ts | 330 ++++++++++++ .../status-branch-line-total-real-git.test.ts | 203 ++++++++ ...tus-branch-line-total-relay-parity.test.ts | 172 +++++++ src/main/git/status.ts | 68 ++- src/main/ipc/filesystem.ts | 4 + .../providers/git-provider-status-options.ts | 2 + src/main/providers/ssh-git-provider.ts | 7 +- src/main/runtime/rpc/methods/git-params.ts | 4 +- src/main/runtime/rpc/methods/git.ts | 4 + src/preload/api-types.ts | 2 + src/preload/index.ts | 1 + src/relay/git-handler-status-ops.ts | 33 +- src/relay/git-handler.ts | 2 + .../git-status-branch-line-total.test.ts | 475 ++++++++++++++++++ src/relay/git-status-branch-line-total.ts | 44 ++ .../SourceControl.branch-line-total.test.tsx | 340 +++++++++++++ ...SourceControl.open-file-highlight.test.tsx | 1 + .../SourceControl.preview-open.test.tsx | 1 + .../right-sidebar/SourceControl.tsx | 27 + .../SourceControl.virtual-file-list.test.tsx | 1 + .../branch-line-total-request-gate.test.ts | 52 ++ .../branch-line-total-request-gate.ts | 23 + ...t-status-refresh-branch-line-total.test.ts | 224 +++++++++ .../right-sidebar/git-status-refresh.ts | 23 +- ...source-control-branch-context-row.test.tsx | 142 ++++++ .../source-control-branch-context-row.tsx | 31 +- .../source-control-branch-context-stats.ts | 12 +- .../source-control-branch-line-total-chip.tsx | 74 +++ ...e-control-header-toolbar-identity.test.tsx | 16 + .../source-control-header-toolbar.tsx | 6 +- src/renderer/src/i18n/locales/en.json | 11 + ...ntime-git-client-branch-line-total.test.ts | 133 +++++ .../src/runtime/runtime-git-client.ts | 10 +- .../slices/editor-branch-line-total.test.ts | 225 +++++++++ src/renderer/src/store/slices/editor.ts | 28 ++ .../src/store/slices/worktrees.test.ts | 21 + src/renderer/src/store/slices/worktrees.ts | 5 + src/renderer/src/web/web-preload-api.test.ts | 93 ++++ src/renderer/src/web/web-preload-api.ts | 4 +- ...it-branch-line-total-soft-deadline.test.ts | 62 +++ src/shared/git-branch-line-total.test.ts | 434 ++++++++++++++++ src/shared/git-branch-line-total.ts | 214 ++++++++ ...git-status-branch-line-total-cache.test.ts | 236 +++++++++ src/shared/git-status-line-stats-cache.ts | 220 +++++--- .../git-status-line-stats-write-token.ts | 69 +++ src/shared/git-status-read-lease-owner.ts | 101 ++++ src/shared/git-status-types.ts | 12 + 48 files changed, 4112 insertions(+), 194 deletions(-) create mode 100644 src/main/git/status-branch-line-total-exec-contract.test.ts create mode 100644 src/main/git/status-branch-line-total-real-git.test.ts create mode 100644 src/main/git/status-branch-line-total-relay-parity.test.ts create mode 100644 src/relay/git-status-branch-line-total.test.ts create mode 100644 src/relay/git-status-branch-line-total.ts create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.branch-line-total.test.tsx create mode 100644 src/renderer/src/components/right-sidebar/branch-line-total-request-gate.test.ts create mode 100644 src/renderer/src/components/right-sidebar/branch-line-total-request-gate.ts create mode 100644 src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-branch-line-total-chip.tsx create mode 100644 src/renderer/src/runtime/runtime-git-client-branch-line-total.test.ts create mode 100644 src/renderer/src/store/slices/editor-branch-line-total.test.ts create mode 100644 src/shared/git-branch-line-total-soft-deadline.test.ts create mode 100644 src/shared/git-branch-line-total.test.ts create mode 100644 src/shared/git-branch-line-total.ts create mode 100644 src/shared/git-status-branch-line-total-cache.test.ts create mode 100644 src/shared/git-status-line-stats-write-token.ts create mode 100644 src/shared/git-status-read-lease-owner.ts diff --git a/src/main/git/git-status-read-lease-owner.ts b/src/main/git/git-status-read-lease-owner.ts index a0012da7d3a..eeae5eedf95 100644 --- a/src/main/git/git-status-read-lease-owner.ts +++ b/src/main/git/git-status-read-lease-owner.ts @@ -1,101 +1,3 @@ -type StatusReadEntry = { - controller: AbortController - promise: Promise - liveLeases: number - settled: boolean -} - -function getAbortReason(signal: AbortSignal): unknown { - try { - signal.throwIfAborted() - } catch (error) { - return error - } - return new DOMException('This operation was aborted', 'AbortError') -} - -export class GitStatusReadLeaseOwner { - private readonly entries = new Map>() - - lease( - key: string, - signal: AbortSignal | undefined, - load: (sharedSignal: AbortSignal) => Promise - ): Promise { - if (signal?.aborted) { - return Promise.reject(getAbortReason(signal)) - } - - let entry = this.entries.get(key) - if (!entry) { - const controller = new AbortController() - const promise = load(controller.signal) - const createdEntry = { controller, promise, liveLeases: 0, settled: false } - entry = createdEntry - this.entries.set(key, createdEntry) - void promise.then( - () => this.settle(key, createdEntry), - () => this.settle(key, createdEntry) - ) - } - - entry.liveLeases += 1 - return this.createLease(key, entry, signal) - } - - invalidate(): void { - this.entries.clear() - } - - private createLease( - key: string, - entry: StatusReadEntry, - signal: AbortSignal | undefined - ): Promise { - return new Promise((resolve, reject) => { - let active = true - const release = (abortReason?: unknown): boolean => { - if (!active) { - return false - } - active = false - signal?.removeEventListener('abort', onAbort) - entry.liveLeases -= 1 - if (abortReason !== undefined && entry.liveLeases === 0 && !entry.settled) { - if (this.entries.get(key) === entry) { - this.entries.delete(key) - } - entry.controller.abort(abortReason) - } - return true - } - const onAbort = (): void => { - const reason = getAbortReason(signal!) - if (release(reason)) { - reject(reason) - } - } - - signal?.addEventListener('abort', onAbort, { once: true }) - void entry.promise.then( - (value) => { - if (release()) { - resolve(value) - } - }, - (error: unknown) => { - if (release()) { - reject(error) - } - } - ) - }) - } - - private settle(key: string, entry: StatusReadEntry): void { - entry.settled = true - if (this.entries.get(key) === entry) { - this.entries.delete(key) - } - } -} +// Why: the class moved to src/shared so the relay host can coalesce the same +// reads; this re-export keeps main's import path (and its tests) stable. +export { GitStatusReadLeaseOwner } from '../../shared/git-status-read-lease-owner' diff --git a/src/main/git/status-branch-line-total-exec-contract.test.ts b/src/main/git/status-branch-line-total-exec-contract.test.ts new file mode 100644 index 00000000000..13ece36daec --- /dev/null +++ b/src/main/git/status-branch-line-total-exec-contract.test.ts @@ -0,0 +1,330 @@ +import { execFileSync } from 'node:child_process' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + buildGitBranchLineTotalDiffArgs, + invalidateGitBranchLineTotalInFlight +} from '../../shared/git-branch-line-total' + +import type * as BranchLineTotal from '../../shared/git-branch-line-total' +import type * as GitRunner from './runner' + +// Why: real git, spied argv. Every exec is recorded so the performance contract +// ("no ranged diff unless asked", "one exec for concurrent callers") can be +// asserted on the actual command list rather than on a stubbed return value. +const { gitExecCalls, execHooks, coalescerJoins } = vi.hoisted(() => ({ + gitExecCalls: [] as string[][], + execHooks: { beforeExec: undefined as undefined | ((args: string[]) => Promise | void) }, + coalescerJoins: { count: 0, onJoin: undefined as undefined | (() => void) } +})) + +vi.mock('../../shared/git-branch-line-total', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + computeGitBranchLineTotal: ( + input: Parameters[0] + ) => { + // The lease is taken synchronously inside, so counting after the call + // means a join is observable the instant it has happened. + const total = actual.computeGitBranchLineTotal(input) + coalescerJoins.count += 1 + coalescerJoins.onJoin?.() + return total + } + } +}) + +vi.mock('./runner', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + gitExecFileAsync: async (...args: Parameters) => { + gitExecCalls.push(args[0]) + await execHooks.beforeExec?.(args[0]) + return actual.gitExecFileAsync(...args) + }, + gitExecFileAsyncBuffer: async ( + ...args: Parameters + ) => { + gitExecCalls.push(args[0]) + return actual.gitExecFileAsyncBuffer(...args) + }, + gitStreamStdout: async (...args: Parameters) => { + gitExecCalls.push(args[0]) + return actual.gitStreamStdout(...args) + } + } +}) + +import { + clearEffectiveUpstreamStatusCacheForTests, + clearSubmodulePathsCacheForTests, + getStatus, + invalidateGitReadCaches +} from './status' + +const BOGUS_MERGE_BASE = 'deadbeef'.repeat(5) +const tempRoots: string[] = [] + +function git(repo: string, args: string[]): string { + return execFileSync('git', args, { + cwd: repo, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'] + }).trim() +} + +async function createFixtureRepo(): Promise<{ repo: string; mergeBase: string }> { + const root = await mkdtemp(path.join(tmpdir(), 'orca-branch-line-total-exec-')) + tempRoots.push(root) + const repo = path.join(root, 'repo') + execFileSync('git', ['init', '-q', repo]) + git(repo, ['config', 'user.email', 'test@example.com']) + git(repo, ['config', 'user.name', 'Test User']) + git(repo, ['config', 'commit.gpgSign', 'false']) + await write(repo, 'f.txt', 'a\nb\n') + git(repo, ['add', '-A']) + git(repo, ['commit', '-q', '-m', 'base']) + await write(repo, 'f.txt', 'a\nb\nc\n') + return { repo, mergeBase: git(repo, ['rev-parse', 'HEAD']) } +} + +async function write(repo: string, relativePath: string, contents: string): Promise { + const target = path.join(repo, relativePath) + await mkdir(path.dirname(target), { recursive: true }) + await writeFile(target, contents) +} + +/** The `mergeBase → worktree` diff, distinguished from the per-area numstats by its rev + `--`. */ +function rangedDiffCalls(): string[][] { + return gitExecCalls.filter((args) => args.includes('--numstat') && args.at(-1) === '--') +} + +function numstatCalls(): string[][] { + return gitExecCalls.filter((args) => args.includes('--numstat')) +} + +/** Resolves once `count` status passes have entered the branch-total coalescer. */ +function waitForCoalescerJoins(count: number): Promise { + if (coalescerJoins.count >= count) { + return Promise.resolve() + } + return new Promise((resolve) => { + coalescerJoins.onJoin = () => { + if (coalescerJoins.count >= count) { + coalescerJoins.onJoin = undefined + resolve() + } + } + }) +} + +function resetGitReadCaches(): void { + clearEffectiveUpstreamStatusCacheForTests() + clearSubmodulePathsCacheForTests() + invalidateGitReadCaches() + invalidateGitBranchLineTotalInFlight() +} + +beforeEach(() => { + gitExecCalls.length = 0 + execHooks.beforeExec = undefined + coalescerJoins.count = 0 + coalescerJoins.onJoin = undefined + resetGitReadCaches() +}) + +afterEach(async () => { + execHooks.beforeExec = undefined + coalescerJoins.onJoin = undefined + resetGitReadCaches() + await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('branch line total completeness', () => { + it('omits the total when the status listing hit its limit', async () => { + const { repo, mergeBase } = await createFixtureRepo() + await write(repo, 'u1.txt', 'a\n') + await write(repo, 'u2.txt', 'b\n') + await write(repo, 'u3.txt', 'c\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase, limit: 1 }) + + expect(result.didHitLimit).toBe(true) + expect(result.branchLineTotal).toBeUndefined() + expect(rangedDiffCalls()).toEqual([]) + }) + + it('omits the total when the ranged numstat fails, rather than reporting zero', async () => { + const { repo } = await createFixtureRepo() + + const result = await getStatus(repo, { branchLineTotalMergeBase: BOGUS_MERGE_BASE }) + + expect(result.entries.length).toBeGreaterThan(0) + expect(result.branchLineTotal).toBeUndefined() + // The diff really was attempted, so this is a failure path and not a gate. + expect(rangedDiffCalls()).toHaveLength(1) + }) + + it('omits the total for a directory that is not a git worktree', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'orca-branch-line-total-folder-')) + tempRoots.push(root) + + const result = await getStatus(root, { branchLineTotalMergeBase: BOGUS_MERGE_BASE }) + + expect(result.branchLineTotal).toBeUndefined() + expect(rangedDiffCalls()).toEqual([]) + }) + + it('rejects with an AbortError when the pass is cancelled mid-diff', async () => { + const { repo, mergeBase } = await createFixtureRepo() + const controller = new AbortController() + execHooks.beforeExec = (args) => { + if (args.includes('--numstat') && args.at(-1) === '--') { + controller.abort() + } + } + + await expect( + getStatus(repo, { branchLineTotalMergeBase: mergeBase, signal: controller.signal }) + ).rejects.toMatchObject({ name: 'AbortError' }) + // The pass had already streamed status and reached the ranged diff, so this + // is a cancellation in flight rather than a pre-flight rejection. + expect(rangedDiffCalls()).toHaveLength(1) + }) +}) + +describe('branch line total merge-base gate', () => { + it('runs no ranged diff and returns no total without a merge base', async () => { + const { repo } = await createFixtureRepo() + + const result = await getStatus(repo) + + expect(result.branchLineTotal).toBeUndefined() + expect(rangedDiffCalls()).toEqual([]) + }) + + it.each(['--upload-pack=x', 'HEAD', '', 'origin/main', '-M', 'A1B2C3D'])( + 'never spawns a ranged diff for a merge base of %j', + async (branchLineTotalMergeBase) => { + const { repo } = await createFixtureRepo() + + const result = await getStatus(repo, { branchLineTotalMergeBase }) + + expect(result.branchLineTotal).toBeUndefined() + expect(rangedDiffCalls()).toEqual([]) + expect(gitExecCalls).not.toContainEqual( + buildGitBranchLineTotalDiffArgs(branchLineTotalMergeBase) + ) + } + ) + + it('never leaks a flag-shaped merge base into any git argv', async () => { + const { repo } = await createFixtureRepo() + + await getStatus(repo, { branchLineTotalMergeBase: '--upload-pack=/tmp/evil' }) + + for (const args of gitExecCalls) { + expect(args).not.toContain('--upload-pack=/tmp/evil') + } + }) +}) + +describe('branch line total exec budget', () => { + it('costs exactly one extra exec versus the same pass with the chip hidden', async () => { + const { repo, mergeBase } = await createFixtureRepo() + + await getStatus(repo) + const withoutChip = gitExecCalls.map((args) => args.join(' ')) + + gitExecCalls.length = 0 + resetGitReadCaches() + await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + const withChip = gitExecCalls.map((args) => args.join(' ')) + + expect(withChip.filter((args) => !args.endsWith(`${mergeBase} --`))).toEqual(withoutChip) + expect(withChip).toHaveLength(withoutChip.length + 1) + }) + + it('shares one ranged diff between two concurrent identical status passes', async () => { + const { repo, mergeBase } = await createFixtureRepo() + + const [first, second] = await Promise.all([ + getStatus(repo, { branchLineTotalMergeBase: mergeBase }), + getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + ]) + + expect(first.branchLineTotal).toEqual({ added: 1, removed: 0, mergeBase }) + expect(second.branchLineTotal).toEqual(first.branchLineTotal) + expect(rangedDiffCalls()).toHaveLength(1) + }) + + it('shares one ranged diff across status passes the status lease cannot merge', async () => { + const { repo, mergeBase } = await createFixtureRepo() + // Why: `limit` is part of the status read key, so these two passes each run + // their own `git status`; only the branch-total coalescer can merge the diff. + // Hold the first diff exactly until the second pass has taken the lease: a + // fixed sleep would either release too early or, on a slow machine, outrun + // GIT_BRANCH_LINE_TOTAL_SOFT_DEADLINE_MS and publish without an exact total. + execHooks.beforeExec = async (args) => { + if (args.includes('--numstat') && args.at(-1) === '--') { + await waitForCoalescerJoins(2) + } + } + + const [first, second] = await Promise.all([ + getStatus(repo, { branchLineTotalMergeBase: mergeBase, limit: 0 }), + getStatus(repo, { branchLineTotalMergeBase: mergeBase, limit: 4096 }) + ]) + + expect(first.branchLineTotal).toEqual({ added: 1, removed: 0, mergeBase }) + expect(second.branchLineTotal).toEqual(first.branchLineTotal) + // Proves the saving came from the branch-total coalescer: both passes really + // ran their own `git status`, so the status lease merged nothing. + expect(gitExecCalls.filter((args) => args.includes('status'))).toHaveLength(2) + expect(rangedDiffCalls()).toHaveLength(1) + }) + + it('reuses the cached total when the pass reuses cached line stats', async () => { + const { repo, mergeBase } = await createFixtureRepo() + + const first = await getStatus(repo, { + branchLineTotalMergeBase: mergeBase, + reuseLineStats: true + }) + expect(rangedDiffCalls()).toHaveLength(1) + expect(numstatCalls()).toHaveLength(2) + + gitExecCalls.length = 0 + invalidateGitBranchLineTotalInFlight() + const second = await getStatus(repo, { + branchLineTotalMergeBase: mergeBase, + reuseLineStats: true + }) + + expect(second.branchLineTotal).toEqual(first.branchLineTotal) + expect(second.branchLineTotal).toEqual({ added: 1, removed: 0, mergeBase }) + expect(numstatCalls()).toEqual([]) + }) + + it('recomputes rather than reusing a total measured against another merge base', async () => { + const { repo, mergeBase } = await createFixtureRepo() + git(repo, ['add', '-A']) + git(repo, ['commit', '-q', '-m', 'second']) + const laterMergeBase = git(repo, ['rev-parse', 'HEAD']) + + await getStatus(repo, { branchLineTotalMergeBase: mergeBase, reuseLineStats: true }) + gitExecCalls.length = 0 + invalidateGitBranchLineTotalInFlight() + const result = await getStatus(repo, { + branchLineTotalMergeBase: laterMergeBase, + reuseLineStats: true + }) + + expect(result.branchLineTotal).toEqual({ added: 0, removed: 0, mergeBase: laterMergeBase }) + expect(rangedDiffCalls()).toHaveLength(1) + }) +}) diff --git a/src/main/git/status-branch-line-total-real-git.test.ts b/src/main/git/status-branch-line-total-real-git.test.ts new file mode 100644 index 00000000000..80426c76e65 --- /dev/null +++ b/src/main/git/status-branch-line-total-real-git.test.ts @@ -0,0 +1,203 @@ +import { execFileSync } from 'node:child_process' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { invalidateGitBranchLineTotalInFlight } from '../../shared/git-branch-line-total' +import { getStatus, invalidateGitReadCaches } from './status' + +const tempRoots: string[] = [] + +function git(repo: string, args: string[]): string { + return execFileSync('git', args, { + cwd: repo, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'] + }).trim() +} + +async function createFixtureRepo(): Promise { + const root = await mkdtemp(path.join(tmpdir(), 'orca-branch-line-total-')) + tempRoots.push(root) + const repo = path.join(root, 'repo') + execFileSync('git', ['init', '-q', repo]) + git(repo, ['config', 'user.email', 'test@example.com']) + git(repo, ['config', 'user.name', 'Test User']) + git(repo, ['config', 'commit.gpgSign', 'false']) + return repo +} + +async function write(repo: string, relativePath: string, contents: string | Buffer): Promise { + const target = path.join(repo, relativePath) + await mkdir(path.dirname(target), { recursive: true }) + await writeFile(target, contents) +} + +/** The forked-from commit the chip measures against, as branch compare would resolve it. */ +function commitAll(repo: string, message: string): string { + git(repo, ['add', '-A']) + git(repo, ['commit', '-q', '-m', message]) + return git(repo, ['rev-parse', 'HEAD']) +} + +/** What a plain `git diff ` reports, i.e. the number the chip must not disagree with. */ +function rangedDiffTotal(repo: string, mergeBase: string): { added: number; removed: number } { + let added = 0 + let removed = 0 + for (const line of git(repo, ['diff', '--numstat', '-M', mergeBase, '--']).split(/\r?\n/)) { + if (!line) { + continue + } + const [rawAdded, rawRemoved] = line.split('\t') + added += rawAdded === '-' ? 0 : Number.parseInt(rawAdded ?? '0', 10) + removed += rawRemoved === '-' ? 0 : Number.parseInt(rawRemoved ?? '0', 10) + } + return { added, removed } +} + +beforeEach(() => { + invalidateGitReadCaches() + invalidateGitBranchLineTotalInFlight() +}) + +afterEach(async () => { + invalidateGitReadCaches() + invalidateGitBranchLineTotalInFlight() + await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('branch line total against a real repository', () => { + it('measures a partially staged line once, not once per status area', async () => { + const repo = await createFixtureRepo() + await write(repo, 'f.txt', 'line1\n') + const mergeBase = commitAll(repo, 'base') + // Stage "+foo", then edit that same line to "bar" without staging it: the + // per-area rows read +1/-0 staged and +1/-1 unstaged, summing to +2/-1. + await write(repo, 'f.txt', 'line1\nfoo\n') + git(repo, ['add', 'f.txt']) + await write(repo, 'f.txt', 'line1\nbar\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(rangedDiffTotal(repo, mergeBase)).toEqual({ added: 1, removed: 0 }) + expect(result.branchLineTotal).toEqual({ added: 1, removed: 0, mergeBase }) + expect(result.entries.map((entry) => [entry.area, entry.added, entry.removed])).toEqual([ + ['staged', 1, 0], + ['unstaged', 1, 1] + ]) + }) + + it('nets a line added in a branch commit and deleted in the working tree to zero', async () => { + const repo = await createFixtureRepo() + await write(repo, 'f.txt', 'a\n') + const mergeBase = commitAll(repo, 'base') + await write(repo, 'f.txt', 'a\nb\n') + commitAll(repo, 'add b') + await write(repo, 'f.txt', 'a\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(result.branchLineTotal).toEqual({ added: 0, removed: 0, mergeBase }) + }) + + it('keeps the total across a commit and updates it on the next status call after an edit', async () => { + const repo = await createFixtureRepo() + await write(repo, 'f.txt', 'a\n') + const mergeBase = commitAll(repo, 'base') + await write(repo, 'f.txt', 'a\nb\nc\n') + + const beforeCommit = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + expect(beforeCommit.branchLineTotal).toEqual({ added: 2, removed: 0, mergeBase }) + + commitAll(repo, 'commit the edit') + const afterCommit = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + expect(afterCommit.branchLineTotal).toEqual({ added: 2, removed: 0, mergeBase }) + + await write(repo, 'f.txt', 'a\nb\nc\nd\n') + const afterSecondEdit = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + expect(afterSecondEdit.branchLineTotal).toEqual({ added: 3, removed: 0, mergeBase }) + }) + + it('counts a rename across the commit boundary once, using the post-rename path', async () => { + const repo = await createFixtureRepo() + await write(repo, 'old.txt', 'a\nb\nc\nd\ne\n') + const mergeBase = commitAll(repo, 'base') + git(repo, ['mv', 'old.txt', 'new.txt']) + commitAll(repo, 'rename') + await write(repo, 'new.txt', 'a\nb\nc\nd\ne\nf\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(rangedDiffTotal(repo, mergeBase)).toEqual({ added: 1, removed: 0 }) + expect(result.branchLineTotal).toEqual({ added: 1, removed: 0, mergeBase }) + }) + + it('counts an untracked-only branch from the untracked file contents', async () => { + const repo = await createFixtureRepo() + await write(repo, 'kept.txt', 'a\n') + const mergeBase = commitAll(repo, 'base') + await write(repo, 'src/new.ts', 'one\ntwo\nthree\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(result.entries.map((entry) => entry.area)).toEqual(['untracked']) + expect(result.branchLineTotal).toEqual({ added: 3, removed: 0, mergeBase }) + }) + + it('excludes a binary-only change, matching numstat reporting it as "-"', async () => { + const repo = await createFixtureRepo() + await write(repo, 'blob.bin', Buffer.from([0, 1, 2, 3, 10, 65, 66])) + const mergeBase = commitAll(repo, 'base') + await write(repo, 'blob.bin', Buffer.from([0, 9, 10, 11, 12, 67, 68, 69, 70])) + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(git(repo, ['diff', '--numstat', mergeBase, '--'])).toMatch(/^-\t-\t/) + expect(result.branchLineTotal).toEqual({ added: 0, removed: 0, mergeBase }) + }) + + it('contributes nothing for a pure rename', async () => { + const repo = await createFixtureRepo() + await write(repo, 'f.txt', 'a\nb\nc\nd\n') + const mergeBase = commitAll(repo, 'base') + git(repo, ['mv', 'f.txt', 'g.txt']) + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(result.branchLineTotal).toEqual({ added: 0, removed: 0, mergeBase }) + }) + + it('includes untracked additions alongside tracked range changes', async () => { + const repo = await createFixtureRepo() + await write(repo, 'f.txt', 'a\nb\n') + const mergeBase = commitAll(repo, 'base') + await write(repo, 'f.txt', 'a\nb\nc\n') + commitAll(repo, 'append c') + await write(repo, 'untracked.txt', 'u1\nu2\nu3\nu4\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(rangedDiffTotal(repo, mergeBase)).toEqual({ added: 1, removed: 0 }) + expect(result.branchLineTotal).toEqual({ added: 5, removed: 0, mergeBase }) + }) + + // Documented deviation from a pure `git diff`: the untracked half is added on + // top of the tracked range, so a deleted-then-recreated path is counted twice. + // Locked in deliberately — changing it must be a deliberate spec change. + it('counts both the deletion and the recreated untracked file for the same path', async () => { + const repo = await createFixtureRepo() + await write(repo, 'gone.txt', 'a\nb\nc\n') + const mergeBase = commitAll(repo, 'base') + git(repo, ['rm', '-q', 'gone.txt']) + commitAll(repo, 'delete gone.txt') + await write(repo, 'gone.txt', 'n1\nn2\n') + + const result = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + + expect(result.entries.map((entry) => [entry.area, entry.path])).toEqual([ + ['untracked', 'gone.txt'] + ]) + expect(rangedDiffTotal(repo, mergeBase)).toEqual({ added: 0, removed: 3 }) + expect(result.branchLineTotal).toEqual({ added: 2, removed: 3, mergeBase }) + }) +}) diff --git a/src/main/git/status-branch-line-total-relay-parity.test.ts b/src/main/git/status-branch-line-total-relay-parity.test.ts new file mode 100644 index 00000000000..e9f5240728c --- /dev/null +++ b/src/main/git/status-branch-line-total-relay-parity.test.ts @@ -0,0 +1,172 @@ +/** + * Main and relay must publish the same `branchLineTotal` for the same fixture + * repo. Both call sites + * share `src/shared/git-branch-line-total.ts`; this is the test that catches one + * of them wiring it up differently — different flags, a different untracked + * source, a different completeness gate. + */ +import { execFile, execFileSync } from 'node:child_process' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { promisify } from 'node:util' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { getStatus } from './status' +import type { GitExec } from '../../relay/git-handler-ops' +import { getStatusOp } from '../../relay/git-handler-status-ops' +import type { RelayGitStreamExec } from '../../relay/git-stdout-stream' +import { invalidateGitBranchLineTotalInFlight } from '../../shared/git-branch-line-total' +import { clearGitStatusLineStatsCache } from '../../shared/git-status-line-stats-cache' + +const execFileAsync = promisify(execFile) + +// Fork point → working tree for the fixture below: +// tracked.txt +2 (branch commit only) +// partial.txt +1 (staged +2 then one of those lines removed unstaged) +// flip.txt 0 (added in the branch commit, removed again in the worktree) +// moved.txt 0 (pure rename) +// fresh.txt +3 (untracked) +const EXPECTED_TOTAL = { added: 6, removed: 0 } +// Summing the per-area status rows instead would give this — the wrong answer +// the shared module exists to avoid. +const AREA_ROW_SUM = { added: 5, removed: 2 } + +const relayGit: GitExec = async (args, cwd, opts) => { + const { stdout, stderr } = await execFileAsync('git', args, { + cwd, + encoding: 'utf8', + ...(opts?.signal ? { signal: opts.signal } : {}), + ...(opts?.timeout ? { timeout: opts.timeout } : {}) + }) + return { stdout, stderr } +} + +const relayStreamGit: RelayGitStreamExec = async (args, cwd, options) => { + const { stdout } = await relayGit(args, cwd, { + disableOptionalLocks: options.disableOptionalLocks, + signal: options.signal + }) + return { stoppedEarly: options.onStdout(stdout) === true } +} + +function runFixtureGit(repo: string, args: string[]): string { + return execFileSync( + 'git', + [ + '-c', + 'user.email=test@test.com', + '-c', + 'user.name=Test', + '-c', + 'commit.gpgSign=false', + ...args + ], + { cwd: repo, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] } + ).trim() +} + +/** Returns the merge-base OID the chip is measured against. */ +async function seedParityFixture(repo: string): Promise { + execFileSync('git', ['init', '-q', repo], { stdio: 'pipe' }) + await writeFile(path.join(repo, 'tracked.txt'), 'a\nb\n') + await writeFile(path.join(repo, 'partial.txt'), 'one\ntwo\nthree\n') + await writeFile(path.join(repo, 'renamed.txt'), 'stable\n') + await writeFile(path.join(repo, 'flip.txt'), 'p\n') + runFixtureGit(repo, ['add', '.']) + runFixtureGit(repo, ['commit', '-m', 'base']) + const mergeBase = runFixtureGit(repo, ['rev-parse', 'HEAD']) + + runFixtureGit(repo, ['checkout', '-q', '-b', 'feature']) + await writeFile(path.join(repo, 'tracked.txt'), 'a\nb\nc\nd\n') + await writeFile(path.join(repo, 'flip.txt'), 'p\nq\n') + runFixtureGit(repo, ['add', '-A']) + runFixtureGit(repo, ['commit', '-m', 'branch commit']) + + runFixtureGit(repo, ['mv', 'renamed.txt', 'moved.txt']) + // Staged and unstaged hunks land on the same added lines, so an area sum + // double-counts them. + await writeFile(path.join(repo, 'partial.txt'), 'one\ntwo\nthree\nfoo\nbaz\n') + runFixtureGit(repo, ['add', 'partial.txt']) + await writeFile(path.join(repo, 'partial.txt'), 'one\ntwo\nthree\nfoo\n') + // The branch commit's line, taken back out in the worktree: net zero. + await writeFile(path.join(repo, 'flip.txt'), 'p\n') + await writeFile(path.join(repo, 'fresh.txt'), 'n1\nn2\nn3\n') + return mergeBase +} + +function sumAreaRows(entries: readonly { added?: number; removed?: number }[]): { + added: number + removed: number +} { + let added = 0 + let removed = 0 + for (const entry of entries) { + added += entry.added ?? 0 + removed += entry.removed ?? 0 + } + return { added, removed } +} + +describe('branch line total parity between main and relay', () => { + let repo: string + + beforeEach(async () => { + clearGitStatusLineStatsCache() + invalidateGitBranchLineTotalInFlight() + repo = await mkdtemp(path.join(tmpdir(), 'branch-line-total-parity-')) + }) + + afterEach(async () => { + clearGitStatusLineStatsCache() + invalidateGitBranchLineTotalInFlight() + await rm(repo, { recursive: true, force: true }) + }) + + it('produces identical totals for the same fixture repo', async () => { + const mergeBase = await seedParityFixture(repo) + + const mainStatus = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + clearGitStatusLineStatsCache() + const relayStatus = await getStatusOp(relayGit, relayStreamGit, { + worktreePath: repo, + branchLineTotalMergeBase: mergeBase + }) + + expect(mainStatus.branchLineTotal).toEqual({ ...EXPECTED_TOTAL, mergeBase }) + expect(relayStatus.branchLineTotal).toEqual(mainStatus.branchLineTotal) + // Both sides model the same worktree, so a parity pass on a mismatched + // entry list would be meaningless. + expect(relayStatus.entries.map((entry) => entry.path).sort()).toEqual( + mainStatus.entries.map((entry) => entry.path).sort() + ) + }) + + it('agrees on a number no per-area row sum could produce', async () => { + const mergeBase = await seedParityFixture(repo) + + const mainStatus = await getStatus(repo, { branchLineTotalMergeBase: mergeBase }) + clearGitStatusLineStatsCache() + const relayStatus = await getStatusOp(relayGit, relayStreamGit, { + worktreePath: repo, + branchLineTotalMergeBase: mergeBase + }) + + expect(sumAreaRows(mainStatus.entries)).toEqual(AREA_ROW_SUM) + expect(sumAreaRows(relayStatus.entries as { added?: number; removed?: number }[])).toEqual( + AREA_ROW_SUM + ) + expect(mainStatus.branchLineTotal).not.toMatchObject(AREA_ROW_SUM) + expect(relayStatus.branchLineTotal).toEqual(mainStatus.branchLineTotal) + }) + + it('omits the total on both sides when no merge base is requested', async () => { + await seedParityFixture(repo) + + const mainStatus = await getStatus(repo) + clearGitStatusLineStatsCache() + const relayStatus = await getStatusOp(relayGit, relayStreamGit, { worktreePath: repo }) + + expect(Object.hasOwn(mainStatus, 'branchLineTotal')).toBe(false) + expect(Object.hasOwn(relayStatus, 'branchLineTotal')).toBe(false) + }) +}) diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 120c84869d2..c03d9307112 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -54,6 +54,13 @@ import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' import { GitStatusReadLeaseOwner } from './git-status-read-lease-owner' import { parseGitRevListFirstParentOid } from '../../shared/git-rev-list-output' +import { + computeGitBranchLineTotal, + invalidateGitBranchLineTotalInFlight, + readGitBranchLineTotalMergeBaseParam, + GIT_BRANCH_LINE_TOTAL_TIMEOUT_MS, + type GitBranchLineTotal +} from '../../shared/git-branch-line-total' import { beginGitStatusLineStatsCacheWrite, clearGitStatusLineStatsCache, @@ -95,10 +102,12 @@ const gitDiffReadDedupe = new InFlightPromiseDedupe() const effectiveUpstreamStatusWriteGeneration = new Map() const statusReadLeaseOwner = new GitStatusReadLeaseOwner() -// Why: clear both diff and status in-flight caches; clearing only diff would let getStatus() join a pre-mutation read. +// Why: clear every in-flight git read cache; clearing only some would let a post-mutation +// getStatus() join a pre-mutation read and publish it as current. export function invalidateGitReadCaches(): void { gitDiffReadDedupe.clear() statusReadLeaseOwner.invalidate() + invalidateGitBranchLineTotalInFlight() clearGitStatusLineStatsCache() clearSubmodulePathsCache() resolvedUpstreamNameCache.clear() @@ -195,6 +204,9 @@ export function getEffectiveUpstreamStatusGenerationCountForTests(): number { export type GetStatusOptions = GitRuntimeOptions & { includeIgnored?: boolean reuseLineStats?: boolean + /** Merge-base OID the caller wants the branch line total measured against; + * omitted means the chip is hidden, so no ranged diff runs at all. */ + branchLineTotalMergeBase?: string /** * Max changed-file entries before git is stopped and the result is marked * `didHitLimit`. Defaults to DEFAULT_GIT_STATUS_LIMIT; 0 disables the cap. @@ -232,6 +244,9 @@ function getStatusReadKey(worktreePath: string, options: GetStatusOptions): stri options.wslDistro ?? '', options.includeIgnored === true, options.reuseLineStats === true, + // Why: the result carries a total only for callers who asked, and only for + // this fork point, so a shared lease must never serve one to the other. + options.branchLineTotalMergeBase ?? '', options.bypassEffectiveUpstreamNegativeCache === true, limit, // Why: this changes which entries survive, so it must not share a cache slot. @@ -371,16 +386,25 @@ async function runGetStatus( } // Why: line counts run only for areas with entries (clean tree = 0 calls); skip past the limit to avoid numstat over a huge set. + let branchLineTotal: GitBranchLineTotal | undefined if (!didHitLimit) { - await reuseOrRecomputeGitStatusLineStats({ + const branchLineTotalInput = createBranchLineTotalInput( + worktreePath, + entries, + options, + statusSucceeded + ) + const lineStats = await reuseOrRecomputeGitStatusLineStats({ cacheKey: lineStatsCacheKey, head, entries, writeToken: lineStatsWriteToken, reuse: options.reuseLineStats === true, isAborted: () => options.signal?.aborted === true, - recompute: () => attachLineStats(worktreePath, entries, options) + recompute: () => attachLineStats(worktreePath, entries, options), + ...(branchLineTotalInput ? { branchLineTotal: branchLineTotalInput } : {}) }) + branchLineTotal = lineStats.branchLineTotal } else { clearGitStatusLineStatsCacheKey(lineStatsCacheKey, lineStatsWriteToken) } @@ -398,6 +422,7 @@ async function runGetStatus( head, branch, ...(options.includeIgnored ? { ignoredPaths: parser.ignoredPaths } : {}), + ...(branchLineTotal ? { branchLineTotal } : {}), ...(didHitLimit ? { didHitLimit: true, statusLength: parser.statusLength } : {}), ...(statusSucceeded ? { @@ -416,6 +441,43 @@ async function runGetStatus( } } +/** Undefined — and therefore zero extra work — unless the caller asked for a total we can know exact. */ +function createBranchLineTotalInput( + worktreePath: string, + entries: GitStatusEntry[], + options: GetStatusOptions, + statusSucceeded: boolean +): { mergeBase: string; compute: () => Promise } | undefined { + const mergeBase = readGitBranchLineTotalMergeBaseParam(options.branchLineTotalMergeBase) + // Why: a failed status scan leaves the untracked list untrustworthy, so the + // total would silently under-count rather than be absent. + if (mergeBase === undefined || !statusSucceeded) { + return undefined + } + return { + mergeBase, + compute: () => + computeGitBranchLineTotal({ + worktreePath, + // Why: the same path can be a different filesystem per WSL distro. + hostKey: options.wslDistro ?? 'native', + mergeBase, + untrackedPaths: entries + .filter((entry) => entry.area === 'untracked') + .map((entry) => entry.path), + runDiffNumstat: (args, signal) => + gitExecFileAsync(args, { + ...gitOptionsForWorktree(worktreePath, options), + // Why: after the spread, so the shared lease signal wins over this caller's own. + signal, + env: gitOptionalLocksDisabledEnv(), + timeout: GIT_BRANCH_LINE_TOTAL_TIMEOUT_MS + }).then((result) => result.stdout), + ...(options.signal ? { signal: options.signal } : {}) + }) + } +} + function getStatusLineStatsCacheKey(worktreePath: string, options: GitRuntimeOptions = {}): string { // Why: identical paths can map to different WSL-distro filesystems, so key stats by Git's execution host. return `${options.wslDistro ?? 'native'}\0${worktreePath}` diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index f3c91088d0a..bf4aff54f04 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -1127,6 +1127,7 @@ export function registerFilesystemHandlers( includeIgnored?: boolean bypassEffectiveUpstreamNegativeCache?: boolean reuseLineStats?: boolean + branchLineTotalMergeBase?: string requestToken?: string } ): Promise => { @@ -1134,6 +1135,9 @@ export function registerFilesystemHandlers( const options = { includeIgnored: args.includeIgnored ?? false, ...(args.reuseLineStats === true ? { reuseLineStats: true } : {}), + ...(args.branchLineTotalMergeBase === undefined + ? {} + : { branchLineTotalMergeBase: args.branchLineTotalMergeBase }), ...(args.bypassEffectiveUpstreamNegativeCache === true ? { bypassEffectiveUpstreamNegativeCache: true } : {}), diff --git a/src/main/providers/git-provider-status-options.ts b/src/main/providers/git-provider-status-options.ts index 3d03ca6a269..a8924924f22 100644 --- a/src/main/providers/git-provider-status-options.ts +++ b/src/main/providers/git-provider-status-options.ts @@ -3,5 +3,7 @@ export type GitProviderStatusOptions = { includeIgnored?: boolean bypassEffectiveUpstreamNegativeCache?: boolean reuseLineStats?: boolean + /** Merge-base OID to measure the branch line total against; omit to skip the work. */ + branchLineTotalMergeBase?: string signal?: AbortSignal } diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 2ec2fbbfc6f..e4c306529ee 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -129,11 +129,16 @@ export class SshGitProvider implements IGitProvider { ? { bypassEffectiveUpstreamNegativeCache: true } : {} const lineStatsReuseArgs = options?.reuseLineStats ? { reuseLineStats: true } : {} + const branchLineTotalArgs = + options?.branchLineTotalMergeBase === undefined + ? {} + : { branchLineTotalMergeBase: options.branchLineTotalMergeBase } const request = { worktreePath, ...includeIgnoredArgs, ...upstreamCacheBypassArgs, - ...lineStatsReuseArgs + ...lineStatsReuseArgs, + ...branchLineTotalArgs } return (await (options?.signal ? this.mux.request('git.status', request, { signal: options.signal }) diff --git a/src/main/runtime/rpc/methods/git-params.ts b/src/main/runtime/rpc/methods/git-params.ts index 7e13e584c42..0fe273108a5 100644 --- a/src/main/runtime/rpc/methods/git-params.ts +++ b/src/main/runtime/rpc/methods/git-params.ts @@ -10,7 +10,9 @@ export const WorktreeSelector = z.object({ export const GitStatusParams = WorktreeSelector.extend({ includeIgnored: z.boolean().optional(), bypassEffectiveUpstreamNegativeCache: z.boolean().optional(), - reuseLineStats: z.boolean().optional() + reuseLineStats: z.boolean().optional(), + // Shape is re-validated host-side before it reaches a git argv. + branchLineTotalMergeBase: z.string().optional() }) export const GitCheckIgnored = WorktreeSelector.extend({ diff --git a/src/main/runtime/rpc/methods/git.ts b/src/main/runtime/rpc/methods/git.ts index c4ccdf071b6..c30e80f4338 100644 --- a/src/main/runtime/rpc/methods/git.ts +++ b/src/main/runtime/rpc/methods/git.ts @@ -93,6 +93,7 @@ export const GIT_METHODS: RpcMethod[] = [ params.includeIgnored === undefined && params.bypassEffectiveUpstreamNegativeCache === undefined && params.reuseLineStats === undefined && + params.branchLineTotalMergeBase === undefined && signal === undefined ? undefined : { @@ -103,6 +104,9 @@ export const GIT_METHODS: RpcMethod[] = [ ? { bypassEffectiveUpstreamNegativeCache: true } : {}), ...(params.reuseLineStats === true ? { reuseLineStats: true } : {}), + ...(params.branchLineTotalMergeBase === undefined + ? {} + : { branchLineTotalMergeBase: params.branchLineTotalMergeBase }), ...(signal ? { signal } : {}) } return options === undefined diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 0dc78393f28..094a93c3484 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -2898,6 +2898,8 @@ export type PreloadApi = { includeIgnored?: boolean bypassEffectiveUpstreamNegativeCache?: boolean reuseLineStats?: boolean + /** Merge-base OID to measure the branch line total against; omit to skip the work. */ + branchLineTotalMergeBase?: string requestToken?: string }) => Promise cancelStatus: (args: { requestToken: string }) => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 36e26870868..8b3863dcf7c 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -3230,6 +3230,7 @@ const api = { includeIgnored?: boolean bypassEffectiveUpstreamNegativeCache?: boolean reuseLineStats?: boolean + branchLineTotalMergeBase?: string requestToken?: string }): Promise => ipcRenderer.invoke('git:status', args), cancelStatus: (args: { requestToken: string }): Promise => diff --git a/src/relay/git-handler-status-ops.ts b/src/relay/git-handler-status-ops.ts index 880e690ee54..60ea5913139 100644 --- a/src/relay/git-handler-status-ops.ts +++ b/src/relay/git-handler-status-ops.ts @@ -24,6 +24,11 @@ import { clearGitStatusLineStatsCacheKey, reuseOrRecomputeGitStatusLineStats } from '../shared/git-status-line-stats-cache' +import { + readGitBranchLineTotalMergeBaseParam, + type GitBranchLineTotal +} from '../shared/git-branch-line-total' +import { buildBranchLineTotalInput } from './git-status-branch-line-total' export async function resolveGitDir(worktreePath: string): Promise { const dotGitPath = path.join(worktreePath, '.git') @@ -74,11 +79,16 @@ export async function getStatusOp( ignoredPaths?: string[] didHitLimit?: boolean statusLength?: number + branchLineTotal?: GitBranchLineTotal }> { const worktreePath = params.worktreePath as string const lineStatsCacheKey = `relay\0${worktreePath}` const lineStatsWriteToken = beginGitStatusLineStatsCacheWrite(lineStatsCacheKey) const includeIgnored = params.includeIgnored === true + // Why: untrusted RPC input spliced into a git argv — only an OID shape may pass. + const branchLineTotalMergeBase = readGitBranchLineTotalMergeBaseParam( + params.branchLineTotalMergeBase + ) // Why: reject NaN/negative limits — NaN would silently disable capping, negatives would over-truncate. const limit = resolveGitStatusLimit(params.limit) const conflictOperation = await detectConflictOperation(worktreePath) @@ -89,6 +99,8 @@ export async function getStatusOp( let ignoredPaths: string[] = [] let didHitLimit = false let statusLength = 0 + let statusSucceeded = false + let branchLineTotal: GitBranchLineTotal | undefined try { // Why: core.quotePath=false keeps non-ASCII filenames as raw UTF-8 instead of octal escapes that render as gibberish. @@ -118,6 +130,7 @@ export async function getStatusOp( ignoredPaths = parser.ignoredPaths statusLength = parser.statusLength didHitLimit = stoppedEarly + statusSucceeded = true const { upstreamName, upstreamAheadBehind } = parser.branch upstreamStatus = upstreamName ? { @@ -173,15 +186,26 @@ export async function getStatusOp( // Why: skip numstat after the limit to avoid reintroducing its cost. if (!didHitLimit) { - await reuseOrRecomputeGitStatusLineStats({ + const branchLineTotalInput = buildBranchLineTotalInput( + git, + worktreePath, + entries, + // Why: a failed scan leaves the untracked list untrustworthy, so the total + // would under-count — omit it rather than publish a confident wrong number. + statusSucceeded ? branchLineTotalMergeBase : undefined, + options.signal + ) + // Why: passed in so the ranged diff runs alongside the per-area numstats, not after them. + ;({ branchLineTotal } = await reuseOrRecomputeGitStatusLineStats({ cacheKey: lineStatsCacheKey, head, entries, writeToken: lineStatsWriteToken, reuse: params.reuseLineStats === true, isAborted: () => options.signal?.aborted === true, - recompute: () => attachLineStats(git, worktreePath, entries, options.signal) - }) + recompute: () => attachLineStats(git, worktreePath, entries, options.signal), + ...(branchLineTotalInput ? { branchLineTotal: branchLineTotalInput } : {}) + })) } else { clearGitStatusLineStatsCacheKey(lineStatsCacheKey, lineStatsWriteToken) } @@ -200,7 +224,8 @@ export async function getStatusOp( branch, upstreamStatus, ...(includeIgnored ? { ignoredPaths } : {}), - ...(didHitLimit ? { didHitLimit: true, statusLength } : {}) + ...(didHitLimit ? { didHitLimit: true, statusLength } : {}), + ...(branchLineTotal ? { branchLineTotal } : {}) } } diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 6674500c6f9..0abc9f5d5be 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -89,6 +89,7 @@ import { GitResponseStreamRegistry } from './git-response-stream' import { GIT_RESPONSE_STREAM_THRESHOLD } from './protocol' import { endSubprocessStdin } from '../shared/subprocess-stdin-write' import { clearGitStatusLineStatsCache } from '../shared/git-status-line-stats-cache' +import { invalidateGitBranchLineTotalInFlight } from '../shared/git-branch-line-total' import { streamRelayGitStdout } from './git-stdout-stream' const execFileAsync = promisify(execFile) @@ -298,6 +299,7 @@ export class GitHandler { private clearGitMutationReadCaches(): void { this.gitDiffReadDedupe.clear() + invalidateGitBranchLineTotalInFlight() clearGitStatusLineStatsCache() clearSubmodulePathsCache(this.submodulePathsCache) } diff --git a/src/relay/git-status-branch-line-total.test.ts b/src/relay/git-status-branch-line-total.test.ts new file mode 100644 index 00000000000..ceb04351e07 --- /dev/null +++ b/src/relay/git-status-branch-line-total.test.ts @@ -0,0 +1,475 @@ +/** + * Relay-side contract for the status pass's branch line total: the chip is an + * exact number or absent, and costs nothing when nobody asked for it. + */ +import { execFile, execFileSync } from 'node:child_process' +import { mkdtempSync } from 'node:fs' +import * as fs from 'node:fs/promises' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { promisify } from 'node:util' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { invalidateGitBranchLineTotalInFlight } from '../shared/git-branch-line-total' +import { clearGitStatusLineStatsCache } from '../shared/git-status-line-stats-cache' +import type { GitExec } from './git-handler-ops' +import { getStatusOp } from './git-handler-status-ops' +import type { RelayGitStreamExec } from './git-stdout-stream' +import { clearNoEffectiveUpstreamStatusCache } from './git-status-upstream-negative-cache' + +type GitCall = Parameters + +const execFileAsync = promisify(execFile) +const MERGE_BASE = '0123456789abcdef0123456789abcdef01234567' +const OTHER_MERGE_BASE = 'fedcba9876543210fedcba9876543210fedcba98' +// Untracked additions ride on top of the ranged diff, so the fixture needs a real file. +const UNTRACKED_FILE = 'notes.md' +const UNTRACKED_LINES = 4 +const STATUS_OUTPUT = [ + '# branch.oid 1111111111111111111111111111111111111111', + '# branch.head (detached)', + '1 .M N... 100644 100644 100644 aaaa aaaa src/a.ts', + `? ${UNTRACKED_FILE}` +].join('\n') + +function isRangedNumstat(args: string[]): boolean { + return args.includes('diff') && args.includes('--numstat') && args.includes('-z') +} + +function rangedDiffCalls(calls: readonly GitCall[]): string[][] { + return calls.map(([args]) => args).filter((args) => isRangedNumstat(args)) +} + +function streamGitFromCapture(git: GitExec): RelayGitStreamExec { + return async (args, cwd, options) => { + const { stdout } = await git(args, cwd, { + disableOptionalLocks: options.disableOptionalLocks, + signal: options.signal + }) + return { stoppedEarly: options.onStdout(stdout) === true } + } +} + +/** Mock host; the fixture-repo cases below run real git instead. */ +function createMockGit(overrides: { + status?: string + ranged?: () => Promise<{ stdout: string; stderr: string }> + areaNumstat?: string +}) { + return vi.fn(async (args) => { + if (args.includes('status')) { + return { stdout: overrides.status ?? STATUS_OUTPUT, stderr: '' } + } + if (isRangedNumstat(args)) { + return overrides.ranged ? overrides.ranged() : { stdout: '12\t5\tsrc/a.ts\n', stderr: '' } + } + if (args.includes('diff')) { + return { stdout: overrides.areaNumstat ?? '3\t2\tsrc/a.ts\n', stderr: '' } + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + }) +} + +const realGitExec: GitExec = async (args, cwd, opts) => { + const { stdout, stderr } = await execFileAsync('git', args, { + cwd, + encoding: 'utf8', + ...(opts?.signal ? { signal: opts.signal } : {}), + ...(opts?.timeout ? { timeout: opts.timeout } : {}) + }) + return { stdout, stderr } +} + +function runFixtureGit(repo: string, args: string[]): string { + return execFileSync( + 'git', + [ + '-c', + 'user.email=test@test.com', + '-c', + 'user.name=Test', + '-c', + 'commit.gpgSign=false', + ...args + ], + { cwd: repo, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] } + ).trim() +} + +/** + * Fork point → working tree: +3 tracked (2 committed, 1 unstaged), -2 from the + * file the branch deleted, +4 untracked. The pure rename contributes nothing. + */ +async function seedBranchFixture(repo: string): Promise { + await fs.rm(path.join(repo, UNTRACKED_FILE), { force: true }) + execFileSync('git', ['init', '-q', repo], { stdio: 'pipe' }) + await fs.writeFile(path.join(repo, 'tracked.txt'), 'a\nb\nc\n') + await fs.writeFile(path.join(repo, 'doomed.txt'), 'x\ny\n') + await fs.writeFile(path.join(repo, 'old-name.txt'), 'stable\n') + runFixtureGit(repo, ['add', '.']) + runFixtureGit(repo, ['commit', '-m', 'base']) + const mergeBase = runFixtureGit(repo, ['rev-parse', 'HEAD']) + + runFixtureGit(repo, ['checkout', '-q', '-b', 'feature']) + await fs.writeFile(path.join(repo, 'tracked.txt'), 'a\nb\nc\nd\ne\n') + await fs.rm(path.join(repo, 'doomed.txt')) + runFixtureGit(repo, ['add', '-A']) + runFixtureGit(repo, ['commit', '-m', 'branch commit']) + + runFixtureGit(repo, ['mv', 'old-name.txt', 'new-name.txt']) + await fs.writeFile(path.join(repo, 'tracked.txt'), 'a\nb\nc\nd\ne\nf\n') + await fs.writeFile(path.join(repo, UNTRACKED_FILE), 'n1\nn2\nn3\nn4\n') + return mergeBase +} + +describe('getStatusOp branch line total', () => { + let tmpDir: string + + beforeEach(async () => { + clearNoEffectiveUpstreamStatusCache() + clearGitStatusLineStatsCache() + invalidateGitBranchLineTotalInFlight() + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-branch-line-total-')) + await fs.writeFile(path.join(tmpDir, UNTRACKED_FILE), 'n1\nn2\nn3\nn4\n') + }) + + afterEach(async () => { + clearNoEffectiveUpstreamStatusCache() + clearGitStatusLineStatsCache() + invalidateGitBranchLineTotalInFlight() + await fs.rm(tmpDir, { recursive: true, force: true }) + }) + + it('runs no ranged diff and returns no total when the merge base param is absent', async () => { + const git = createMockGit({}) + + const result = await getStatusOp(git, streamGitFromCapture(git), { worktreePath: tmpDir }) + + expect(rangedDiffCalls(git.mock.calls)).toEqual([]) + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(result.branchLineTotal).toBeUndefined() + // The per-area numstats the CHANGES rows need still ran, so this is a + // zero-cost omission rather than a disabled status pass. + expect(result.entries).toContainEqual( + expect.objectContaining({ path: 'src/a.ts', added: 3, removed: 2 }) + ) + }) + + it('sums the ranged diff and untracked additions against a real fixture repo', async () => { + const mergeBase = await seedBranchFixture(tmpDir) + const git = vi.fn(realGitExec) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: mergeBase + }) + + expect(result.branchLineTotal).toEqual({ + added: 3 + UNTRACKED_LINES, + removed: 2, + mergeBase + }) + expect(rangedDiffCalls(git.mock.calls)).toEqual([ + ['-c', 'core.quotePath=false', 'diff', '-z', '--numstat', '-M', mergeBase, '--'] + ]) + }) + + it('omits the total when the status listing hit its limit', async () => { + const manyEntries = Array.from( + { length: 6 }, + (_, index) => `1 A. N... 100644 100644 100644 000000 111111 generated-${index}.txt` + ).join('\n') + const git = createMockGit({ status: manyEntries }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + limit: 3, + branchLineTotalMergeBase: MERGE_BASE + }) + + expect(result.didHitLimit).toBe(true) + // The untracked list is truncated, so any total would silently under-count. + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(rangedDiffCalls(git.mock.calls)).toEqual([]) + }) + + it('omits the total, never zero, when the ranged diff fails', async () => { + const git = createMockGit({ + ranged: () => Promise.reject(new Error('fatal: bad object 0123456789abcdef')) + }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(result.branchLineTotal).toBeUndefined() + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + // A failed ranged diff must not take the per-file rows down with it. + expect(result.entries).toContainEqual(expect.objectContaining({ path: 'src/a.ts', added: 3 })) + }) + + it('omits the total, never zero, for a well-formed but unknown merge-base oid', async () => { + await seedBranchFixture(tmpDir) + const git = vi.fn(realGitExec) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' + }) + + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(result.entries.length).toBeGreaterThan(0) + }) + + it('omits the total when the ranged diff exceeds its time budget', async () => { + const git = createMockGit({ + ranged: () => { + const error: Error & { killed?: boolean } = new Error('spawn git ETIMEDOUT') + error.killed = true + return Promise.reject(error) + } + }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + // The budget is handed to the subprocess rather than raced on a wall clock. + const rangedOptions = git.mock.calls.find(([args]) => isRangedNumstat(args))?.[2] + expect(rangedOptions).toMatchObject({ disableOptionalLocks: true, timeout: expect.any(Number) }) + }) + + it('omits the total when the status scan itself failed', async () => { + const git = vi.fn(async (args) => { + if (args.includes('status')) { + throw new Error('fatal: not a git repository') + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + + expect(result.entries).toEqual([]) + // A failed scan yields an empty untracked list, which would read as "no + // untracked additions" rather than "unknown". + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(rangedDiffCalls(git.mock.calls)).toEqual([]) + }) + + // Relay params are an untyped bag, so the merge base must be proven to be an + // object name before it can be spliced into an argv. + it.each([ + ['a flag-shaped value', '--upload-pack=x'], + ['a rev name', 'HEAD'], + ['a ref path', 'refs/heads/main'], + ['a range', `${MERGE_BASE}..HEAD`], + ['a number', 123], + ['null', null], + ['an empty string', ''] + ])('rejects %s before it reaches a git argv', async (_label, value) => { + const git = createMockGit({}) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: value + }) + + expect(Object.hasOwn(result, 'branchLineTotal')).toBe(false) + expect(rangedDiffCalls(git.mock.calls)).toEqual([]) + for (const [args] of git.mock.calls) { + expect(args).not.toContain(String(value)) + } + }) + + it('answers a rejected merge base with the byte-identical no-merge-base response', async () => { + const gitWithout = createMockGit({}) + const withoutParam = await getStatusOp(gitWithout, streamGitFromCapture(gitWithout), { + worktreePath: tmpDir + }) + clearGitStatusLineStatsCache() + const gitRejected = createMockGit({}) + const rejectedParam = await getStatusOp(gitRejected, streamGitFromCapture(gitRejected), { + worktreePath: tmpDir, + branchLineTotalMergeBase: '--upload-pack=x' + }) + + expect(JSON.stringify(rejectedParam)).toBe(JSON.stringify(withoutParam)) + expect(gitRejected.mock.calls.map(([args]) => args)).toEqual( + gitWithout.mock.calls.map(([args]) => args) + ) + }) + + it('rejects an aborted scan instead of resolving a partial total', async () => { + const controller = new AbortController() + const git = vi.fn(async (args) => { + if (args.includes('status')) { + return { stdout: STATUS_OUTPUT, stderr: '' } + } + if (isRangedNumstat(args)) { + controller.abort() + const error = new Error('The operation was aborted.') + error.name = 'AbortError' + throw error + } + if (args.includes('diff')) { + return { stdout: '3\t2\tsrc/a.ts\n', stderr: '' } + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + }) + + await expect( + getStatusOp( + git, + streamGitFromCapture(git), + { worktreePath: tmpDir, branchLineTotalMergeBase: MERGE_BASE }, + { signal: controller.signal } + ) + ).rejects.toThrow(/abort/i) + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + }) + + it('reuses the cached total instead of re-running the ranged diff', async () => { + const git = createMockGit({}) + + const first = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + const reused = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE, + reuseLineStats: true + }) + + expect(first.branchLineTotal).toEqual({ + added: 12 + UNTRACKED_LINES, + removed: 5, + mergeBase: MERGE_BASE + }) + expect(reused.branchLineTotal).toEqual(first.branchLineTotal) + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + }) + + it('coalesces concurrent status passes onto one ranged diff', async () => { + // The renderer's own in-flight refs only dedupe one renderer; a second + // window or an fs-watcher burst must not run the ranged diff twice. + const git = vi.fn(async (args) => { + if (args.includes('status')) { + return { stdout: STATUS_OUTPUT, stderr: '' } + } + if (isRangedNumstat(args)) { + await new Promise((resolve) => setTimeout(resolve, 20)) + return { stdout: '12\t5\tsrc/a.ts\n', stderr: '' } + } + if (args.includes('diff')) { + return { stdout: '3\t2\tsrc/a.ts\n', stderr: '' } + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + }) + + const [first, second] = await Promise.all([ + getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }), + getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + ]) + + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(1) + expect(first.branchLineTotal).toEqual(second.branchLineTotal) + expect(first.branchLineTotal).toEqual({ + added: 12 + UNTRACKED_LINES, + removed: 5, + mergeBase: MERGE_BASE + }) + }) + + it('recomputes rather than reusing a total measured against another fork point', async () => { + const git = createMockGit({}) + + await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + const moved = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: OTHER_MERGE_BASE, + reuseLineStats: true + }) + + expect(moved.branchLineTotal).toEqual({ + added: 12 + UNTRACKED_LINES, + removed: 5, + mergeBase: OTHER_MERGE_BASE + }) + expect(rangedDiffCalls(git.mock.calls)).toHaveLength(2) + }) + + // Rule 1 of docs/reference/remote-wire-compatibility.md: a new optional field + // is safe only while every reader survives its absence. + describe('wire compatibility', () => { + it('drops the key from the wire payload so a reader sees undefined, not 0 or NaN', async () => { + const git = createMockGit({ ranged: () => Promise.reject(new Error('fatal: bad object')) }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + // An old server's payload and a new server's "not known exact" payload are + // the same thing on the wire: no key at all. + const overTheWire = JSON.parse(JSON.stringify(result)) as { + branchLineTotal?: { added: number; removed: number } + } + + expect(Object.hasOwn(overTheWire, 'branchLineTotal')).toBe(false) + expect(overTheWire.branchLineTotal).toBeUndefined() + expect(overTheWire.branchLineTotal?.added).toBeUndefined() + expect(overTheWire.branchLineTotal?.removed).toBeUndefined() + }) + + it('keeps the new request param out of the status argv an old server would run', async () => { + const git = createMockGit({}) + + await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + + const statusArgs = git.mock.calls + .map(([args]) => args) + .filter((args) => args.includes('status')) + expect(statusArgs).toHaveLength(1) + expect(statusArgs[0]).not.toContain(MERGE_BASE) + }) + + it('publishes finite counts when the ranged diff reports only binaries', async () => { + const git = createMockGit({ + ranged: () => Promise.resolve({ stdout: '-\t-\tlogo.png\n', stderr: '' }) + }) + + const result = await getStatusOp(git, streamGitFromCapture(git), { + worktreePath: tmpDir, + branchLineTotalMergeBase: MERGE_BASE + }) + + expect(result.branchLineTotal).toEqual({ + added: UNTRACKED_LINES, + removed: 0, + mergeBase: MERGE_BASE + }) + expect(Number.isFinite(result.branchLineTotal?.added)).toBe(true) + expect(Number.isFinite(result.branchLineTotal?.removed)).toBe(true) + }) + }) +}) diff --git a/src/relay/git-status-branch-line-total.ts b/src/relay/git-status-branch-line-total.ts new file mode 100644 index 00000000000..c2fe6556a0e --- /dev/null +++ b/src/relay/git-status-branch-line-total.ts @@ -0,0 +1,44 @@ +/** + * Relay-side wiring for the status pass's branch line total. + * Why: separate file so git-handler-status-ops.ts stays under oxlint max-lines (300). + */ +import { + computeGitBranchLineTotal, + GIT_BRANCH_LINE_TOTAL_TIMEOUT_MS, + type GitBranchLineTotal +} from '../shared/git-branch-line-total' +import type { GitExec } from './git-handler-ops' + +/** Undefined when no valid merge base was requested, which keeps the ranged diff entirely off. */ +export function buildBranchLineTotalInput( + git: GitExec, + worktreePath: string, + entries: Record[], + mergeBase: string | undefined, + signal?: AbortSignal +): { mergeBase: string; compute: () => Promise } | undefined { + if (!mergeBase) { + return undefined + } + return { + mergeBase, + compute: () => + computeGitBranchLineTotal({ + worktreePath, + // Matches the `relay\0` line-stats cache-key convention. + hostKey: 'relay', + mergeBase, + untrackedPaths: entries + .filter((entry) => entry.area === 'untracked') + .map((entry) => entry.path as string), + runDiffNumstat: (args, diffSignal) => + git(args, worktreePath, { + // Why: a working-tree diff must not take index.lock away from terminal Git. + disableOptionalLocks: true, + signal: diffSignal, + timeout: GIT_BRANCH_LINE_TOTAL_TIMEOUT_MS + }).then(({ stdout }) => stdout), + ...(signal ? { signal } : {}) + }) + } +} diff --git a/src/renderer/src/components/right-sidebar/SourceControl.branch-line-total.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.branch-line-total.test.tsx new file mode 100644 index 00000000000..c9a7f24beb2 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.branch-line-total.test.tsx @@ -0,0 +1,340 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { TooltipProvider } from '@/components/ui/tooltip' +import type { GitBranchCompareSummary } from '../../../../shared/types' +import { + clearBranchLineTotalRequestGateForTests, + getBranchLineTotalMergeBase +} from './branch-line-total-request-gate' +import SourceControl from './SourceControl' + +const MERGE_BASE = '1f3c0d9a5b6e7f8091a2b3c4d5e6f708192a3b4c' + +const mocks = vi.hoisted(() => { + const activeRepo = { + id: 'repo-1', + path: '/repo', + displayName: 'Repo', + badgeColor: '#000', + addedAt: 0, + kind: 'git' as string + } + const activeWorktree = { + id: 'wt-1', + repoId: 'repo-1', + path: '/repo/wt', + head: 'head-1', + branch: 'refs/heads/feature/line-total', + isBare: false, + isMainWorktree: false, + displayName: 'feature/line-total', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + linkedGitLabMR: null, + linkedGitLabIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0 + } + return { + activeRepo, + activeWorktree, + state: {} as Record + } +}) + +vi.mock('@/store', () => { + const useAppStore = Object.assign( + (selector?: (state: Record) => unknown) => + selector ? selector(mocks.state) : mocks.state, + { + getState: () => mocks.state + } + ) + return { useAppStore } +}) + +vi.mock('@/store/selectors', () => ({ + useActiveWorktree: () => mocks.activeWorktree, + useRepoById: (repoId: string | null) => + repoId === mocks.activeRepo.id ? mocks.activeRepo : null, + useWorktreeMap: () => new Map([[mocks.activeWorktree.id, mocks.activeWorktree]]) +})) + +vi.mock('@/components/confirmation-dialog-context', () => ({ + useConfirmationDialog: () => vi.fn().mockResolvedValue(true) +})) + +vi.mock('./git-status-refresh', () => ({ + refreshGitStatusForWorktree: vi.fn().mockResolvedValue(undefined), + refreshGitStatusForWorktreeStrict: vi.fn().mockResolvedValue(undefined) +})) + +function noopAsync(value: unknown = undefined): () => Promise { + return vi.fn().mockResolvedValue(value) +} + +const readySummary: GitBranchCompareSummary = { + baseRef: 'refs/remotes/origin/main', + baseOid: 'base-oid', + compareRef: 'feature/line-total', + headOid: 'head-1', + mergeBase: MERGE_BASE, + changedFiles: 2, + commitsAhead: 1, + status: 'ready' +} + +function resetState(overrides: Partial> = {}): void { + vi.clearAllMocks() + mocks.activeRepo.kind = 'git' + mocks.state = { + activeWorktreeId: mocks.activeWorktree.id, + activeGroupIdByWorktree: { [mocks.activeWorktree.id]: 'group-1' }, + groupsByWorktree: { [mocks.activeWorktree.id]: [{ id: 'group-1', activeTabId: null }] }, + repos: [mocks.activeRepo], + worktreesByRepo: { [mocks.activeRepo.id]: [mocks.activeWorktree] }, + rightSidebarOpen: true, + rightSidebarTab: 'source-control', + gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: readySummary }, + gitBranchLineTotalByWorktree: {}, + gitConflictOperationByWorktree: {}, + remoteStatusesByWorktree: {}, + isRemoteOperationActive: false, + inFlightRemoteOpKind: null, + settings: null, + hostedReviewCache: {}, + prCache: {}, + commitMessageGenerationRecords: {}, + pullRequestGenerationRecords: {}, + openFiles: [], + activeFileIdByWorktree: {}, + activeTabTypeByWorktree: {}, + getDiffComments: vi.fn(() => []), + updateSettings: noopAsync(), + openSettingsTarget: vi.fn(), + openSettingsPage: vi.fn(), + fetchHostedReviewForBranch: noopAsync(), + getHostedReviewCreationEligibility: noopAsync(null), + createHostedReview: noopAsync({ ok: false, error: 'not available' }), + updateWorktreeMeta: noopAsync(), + fetchPRForBranch: noopAsync(), + enqueueGitHubPRRefresh: vi.fn(), + updateRepo: noopAsync(), + setGitStatus: vi.fn(), + updateWorktreeGitIdentity: vi.fn(), + beginGitBranchCompareRequest: vi.fn(() => 'request-key'), + setGitBranchCompareResult: vi.fn(), + clearGitBranchCompare: vi.fn(), + fetchUpstreamStatus: noopAsync(), + setUpstreamStatus: vi.fn(), + pushBranch: noopAsync(), + pullBranch: noopAsync(), + fastForwardBranch: noopAsync(), + syncBranch: noopAsync(), + rebaseFromBase: noopAsync(), + fetchBranch: noopAsync(), + revealInExplorer: vi.fn(), + trackConflictPath: vi.fn(), + openDiff: vi.fn(), + openFile: vi.fn(), + setEditorViewMode: vi.fn(), + setMarkdownViewMode: vi.fn(), + setPendingEditorReveal: vi.fn(), + openConflictFile: vi.fn(), + openConflictReview: vi.fn(), + openBranchDiff: vi.fn(), + createEmptySplitGroup: vi.fn(() => 'group-2'), + openAllDiffs: vi.fn(), + openBranchAllDiffs: vi.fn(), + openCommitAllDiffs: vi.fn(), + deleteDiffComment: noopAsync(true), + clearDiffComments: noopAsync(true), + clearDiffCommentsForFile: noopAsync(true), + setScrollToDiffCommentId: vi.fn(), + setRightSidebarOpen: vi.fn(), + setRightSidebarTab: vi.fn(), + allocateCommitMessageGenerationRequestId: vi.fn(() => 'commit-generation-1'), + setCommitMessageGenerationRecord: vi.fn(), + updateCommitMessageGenerationRecord: vi.fn(), + pruneCommitMessageGenerationRecords: vi.fn(), + allocatePullRequestGenerationRequestId: vi.fn(() => 'pr-generation-1'), + setPullRequestGenerationRecord: vi.fn(), + updatePullRequestGenerationRecord: vi.fn(), + prunePullRequestGenerationRecords: vi.fn(), + ...overrides + } +} + +let container: HTMLDivElement +let root: Root + +beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + clearBranchLineTotalRequestGateForTests() + resetState() + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() + clearBranchLineTotalRequestGateForTests() +}) + +function renderSourceControl(): void { + act(() => { + root.render( + + + + ) + }) +} + +function chip(): HTMLElement | null { + return container.querySelector('[data-testid="source-control-branch-line-total"]') +} + +describe('SourceControl branch line total request gate', () => { + it('asks for a total when the panel is visible and compare is ready', () => { + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBe(MERGE_BASE) + }) + + it('asks for nothing while the sidebar is closed', () => { + resetState({ rightSidebarOpen: false }) + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) + + it('asks for nothing while another right-sidebar tab is showing', () => { + resetState({ rightSidebarTab: 'checks' }) + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) + + it('asks for nothing until branch compare is ready', () => { + resetState({ + gitBranchCompareSummaryByWorktree: { + [mocks.activeWorktree.id]: { ...readySummary, status: 'loading' } + } + }) + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) + + it('asks for nothing when compare has no merge base', () => { + resetState({ + gitBranchCompareSummaryByWorktree: { + [mocks.activeWorktree.id]: { ...readySummary, status: 'invalid-base' } + } + }) + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) + + it('asks for nothing in a folder workspace', () => { + resetState() + mocks.activeRepo.kind = 'folder' + renderSourceControl() + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) + + it('releases the gate when the panel unmounts', () => { + renderSourceControl() + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBe(MERGE_BASE) + + act(() => root.render({null})) + + expect(getBranchLineTotalMergeBase(mocks.activeWorktree.id)).toBeUndefined() + }) +}) + +describe('SourceControl branch line total chip', () => { + it('renders a total measured against the current fork point', () => { + resetState({ + gitBranchLineTotalByWorktree: { + [mocks.activeWorktree.id]: { added: 8259, removed: 670, mergeBase: MERGE_BASE } + } + }) + renderSourceControl() + + expect(chip()?.getAttribute('aria-label')).toBe('8259 additions, 670 deletions') + // Grouping is pinned to the app locale (`en`), not the runner's host locale. + expect(chip()?.textContent).toBe('+8,259-670') + }) + + it('drops a total whose fork point has since moved', () => { + // Why: status and branch compare refresh on different cadences, so a total + // can outlive the merge base it measured. Hidden beats a stale number. + resetState({ + gitBranchLineTotalByWorktree: { + [mocks.activeWorktree.id]: { added: 8259, removed: 670, mergeBase: 'stale-merge-base' } + } + }) + renderSourceControl() + + expect(chip()).toBeNull() + }) + + it('drops a published total while branch compare has no ready summary', () => { + resetState({ + gitBranchCompareSummaryByWorktree: { + [mocks.activeWorktree.id]: { ...readySummary, status: 'loading', mergeBase: '' } + }, + gitBranchLineTotalByWorktree: { + [mocks.activeWorktree.id]: { added: 8259, removed: 670, mergeBase: MERGE_BASE } + } + }) + renderSourceControl() + + expect(chip()).toBeNull() + }) + + it('renders nothing when no total was published', () => { + renderSourceControl() + + expect(chip()).toBeNull() + }) + + it('renders nothing for an exact zero total', () => { + resetState({ + gitBranchLineTotalByWorktree: { + [mocks.activeWorktree.id]: { added: 0, removed: 0, mergeBase: MERGE_BASE } + } + }) + renderSourceControl() + + expect(chip()).toBeNull() + }) + + it('omits the zero half of a one-sided total', () => { + resetState({ + gitBranchLineTotalByWorktree: { + [mocks.activeWorktree.id]: { added: 42, removed: 0, mergeBase: MERGE_BASE } + } + }) + renderSourceControl() + + expect(chip()?.textContent).toBe('+42') + expect(chip()?.getAttribute('aria-label')).toBe('42 additions') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.open-file-highlight.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.open-file-highlight.test.tsx index 8624d25fae2..d3125ee86c7 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.open-file-highlight.test.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.open-file-highlight.test.tsx @@ -104,6 +104,7 @@ function resetState(overrides: Partial> = {}): void { gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: null }, + gitBranchLineTotalByWorktree: {}, gitConflictOperationByWorktree: {}, remoteStatusesByWorktree: {}, isRemoteOperationActive: false, diff --git a/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx index 22bc4954b20..13dc2479894 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx @@ -156,6 +156,7 @@ function resetState(overrides: Partial> = {}): void { gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: null }, + gitBranchLineTotalByWorktree: {}, gitConflictOperationByWorktree: {}, remoteStatusesByWorktree: {}, isRemoteOperationActive: false, diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 68b55f9bda7..68cabc5e044 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -296,6 +296,7 @@ import { } from './source-control-hosted-review-push-target' import { buildSourceControlManualReviewUrlFromContext } from './source-control-manual-review-url' import { parseRemoteRepo } from './source-control-remote-repo' +import { setBranchLineTotalMergeBase } from './branch-line-total-request-gate' export { HostedReviewHeaderLink } from './hosted-review-header-chrome' import { createRunningCommitMessageGenerationRecord, @@ -835,6 +836,15 @@ function SourceControlInner(): React.JSX.Element { const branchSummary = useAppStore((s) => activeWorktreeId ? (s.gitBranchCompareSummaryByWorktree[activeWorktreeId] ?? null) : null ) + const publishedBranchLineTotal = useAppStore((s) => + activeWorktreeId ? (s.gitBranchLineTotalByWorktree?.[activeWorktreeId] ?? null) : null + ) + // Why: status and branch compare refresh on different cadences, so a total can + // outlive the fork point it measured. Drop it rather than render a stale number. + const branchLineTotal = + publishedBranchLineTotal && publishedBranchLineTotal.mergeBase === branchSummary?.mergeBase + ? publishedBranchLineTotal + : null const conflictOperation = useAppStore((s) => activeWorktreeId ? (s.gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') : 'unknown' ) @@ -1252,6 +1262,22 @@ function SourceControlInner(): React.JSX.Element { // Why: the sidebar stays mounted when closed, so gate polling on tab AND open or branchCompare/PR fetch would run with no visible consumer. const isBranchVisible = rightSidebarTab === 'source-control' && rightSidebarOpen + // Why: the merge base IS the request gate — no OID on the status request means + // the host runs no ranged diff, so a hidden chip costs a background worktree nothing. + const requestedBranchLineTotalMergeBase = + isBranchVisible && !isFolder && branchSummary?.status === 'ready' + ? branchSummary.mergeBase + : null + useEffect(() => { + if (!activeWorktreeId) { + return + } + setBranchLineTotalMergeBase(activeWorktreeId, requestedBranchLineTotalMergeBase) + return () => { + setBranchLineTotalMergeBase(activeWorktreeId, null) + } + }, [activeWorktreeId, requestedBranchLineTotalMergeBase]) + const refreshActiveGitStatus = useCallback( async (signal?: AbortSignal): Promise => { if (!activeWorktreeId || !worktreePath || isFolder) { @@ -5549,6 +5575,7 @@ function SourceControlInner(): React.JSX.Element { diffCommentCount={diffCommentCount} onExpandNotes={() => setDiffCommentsExpanded(true)} branchSummary={branchSummary} + branchLineTotal={branchLineTotal} compareBaseRef={compareBaseRef} headDisplay={gitIdentityDisplay} upstreamStatus={remoteStatus} diff --git a/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx index 7e1d4c7d2ec..5ff61b17d43 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx @@ -116,6 +116,7 @@ function resetState(overrides: Partial> = {}): void { gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: null }, + gitBranchLineTotalByWorktree: {}, gitConflictOperationByWorktree: {}, remoteStatusesByWorktree: {}, isRemoteOperationActive: false, diff --git a/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.test.ts b/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.test.ts new file mode 100644 index 00000000000..600a3cf8fc7 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.test.ts @@ -0,0 +1,52 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import { + clearBranchLineTotalRequestGateForTests, + getBranchLineTotalMergeBase, + setBranchLineTotalMergeBase +} from './branch-line-total-request-gate' + +describe('branch line total request gate', () => { + beforeEach(() => { + clearBranchLineTotalRequestGateForTests() + }) + + it('reports no merge base for a worktree that never opened the chip', () => { + expect(getBranchLineTotalMergeBase('wt-never-gated')).toBeUndefined() + }) + + it('stores the merge base a visible chip asked for', () => { + setBranchLineTotalMergeBase('wt-1', 'merge-base-1') + + expect(getBranchLineTotalMergeBase('wt-1')).toBe('merge-base-1') + }) + + it('replaces the merge base when the fork point moves', () => { + setBranchLineTotalMergeBase('wt-1', 'merge-base-1') + setBranchLineTotalMergeBase('wt-1', 'merge-base-2') + + expect(getBranchLineTotalMergeBase('wt-1')).toBe('merge-base-2') + }) + + it('deletes the entry when the chip is hidden', () => { + setBranchLineTotalMergeBase('wt-1', 'merge-base-1') + setBranchLineTotalMergeBase('wt-1', null) + + expect(getBranchLineTotalMergeBase('wt-1')).toBeUndefined() + }) + + it('treats an empty merge base as no gate rather than an empty request param', () => { + setBranchLineTotalMergeBase('wt-1', 'merge-base-1') + setBranchLineTotalMergeBase('wt-1', '') + + expect(getBranchLineTotalMergeBase('wt-1')).toBeUndefined() + }) + + it('keeps entries independent per worktree', () => { + setBranchLineTotalMergeBase('wt-1', 'merge-base-1') + setBranchLineTotalMergeBase('wt-2', 'merge-base-2') + setBranchLineTotalMergeBase('wt-1', null) + + expect(getBranchLineTotalMergeBase('wt-1')).toBeUndefined() + expect(getBranchLineTotalMergeBase('wt-2')).toBe('merge-base-2') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.ts b/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.ts new file mode 100644 index 00000000000..c3fa7b98369 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/branch-line-total-request-gate.ts @@ -0,0 +1,23 @@ +// Why: several independent code paths call `git.status`; if only some carried +// the merge base the branch line-total chip would blank out between polls. One +// registry keeps every path asking for the same thing. +// Only the visible worktree's SourceControl is mounted and its effect cleanup +// deletes on every change, so this holds ~1 entry and needs no eviction. +const branchLineTotalMergeBaseByWorktree = new Map() + +/** `null` deletes the entry — that absence is the visibility gate (no merge base ⇒ no host cost). */ +export function setBranchLineTotalMergeBase(worktreeId: string, mergeBase: string | null): void { + if (!mergeBase) { + branchLineTotalMergeBaseByWorktree.delete(worktreeId) + return + } + branchLineTotalMergeBaseByWorktree.set(worktreeId, mergeBase) +} + +export function getBranchLineTotalMergeBase(worktreeId: string): string | undefined { + return branchLineTotalMergeBaseByWorktree.get(worktreeId) +} + +export function clearBranchLineTotalRequestGateForTests(): void { + branchLineTotalMergeBaseByWorktree.clear() +} 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 new file mode 100644 index 00000000000..3437d8d227a --- /dev/null +++ b/src/renderer/src/components/right-sidebar/git-status-refresh-branch-line-total.test.ts @@ -0,0 +1,224 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + clearBranchLineTotalRequestGateForTests, + setBranchLineTotalMergeBase +} from './branch-line-total-request-gate' +import { + clearGitStatusRefreshOrderingForTests, + refreshGitStatusForWorktree, + refreshGitStatusForWorktreeStrict, + type GitStatusRefreshDeps +} from './git-status-refresh' +import type { GitStatusResult } from '../../../../shared/types' + +const MERGE_BASE = '1f3c0d9a5b6e7f8091a2b3c4d5e6f708192a3b4c' + +function makeDeps(): GitStatusRefreshDeps { + return { + setGitStatus: vi.fn(), + updateWorktreeGitIdentity: vi.fn(), + setUpstreamStatus: vi.fn(), + fetchUpstreamStatus: vi.fn().mockResolvedValue(null) + } +} + +function stubGitStatus(): ReturnType { + const status: GitStatusResult = { + entries: [], + conflictOperation: 'unknown', + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } + } + const gitStatus = vi.fn().mockResolvedValue(status) + vi.stubGlobal('window', { + api: { + git: { + status: gitStatus, + cancelStatus: vi.fn().mockResolvedValue(undefined), + upstreamStatus: vi.fn().mockResolvedValue({ hasUpstream: false, ahead: 0, behind: 0 }) + } + } + }) + return gitStatus +} + +describe('branch line total request gate on git status refreshes', () => { + beforeEach(() => { + vi.unstubAllGlobals() + clearGitStatusRefreshOrderingForTests() + clearBranchLineTotalRequestGateForTests() + }) + + it('omits the merge base when no chip is asking for a total', async () => { + const gitStatus = stubGitStatus() + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-hidden', + worktreePath: '/repo', + connectionId: 'ssh-1', + deps: makeDeps() + }) + + // Why: no OID on the request is the whole performance contract — the host + // runs no ranged diff, so a background worktree costs nothing. + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: 'ssh-1' + }) + }) + + it('omits the merge base for a worktree whose chip was hidden again', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-1', MERGE_BASE) + setBranchLineTotalMergeBase('wt-1', null) + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + deps: makeDeps() + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + }) + + it('does not send one worktree gate on another worktree status pass', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-visible', MERGE_BASE) + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-background', + worktreePath: '/other-repo', + deps: makeDeps() + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/other-repo', + connectionId: undefined + }) + }) + + it('sends the merge base on a plain refresh that passes no request options', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-1', MERGE_BASE) + + // Why: this path used to build no options object at all, so the gate was + // silently dropped on the most common (fs-watcher) refresh. + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + connectionId: 'ssh-1', + deps: makeDeps() + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: 'ssh-1', + branchLineTotalMergeBase: MERGE_BASE + }) + }) + + it('sends the merge base alongside line-stat reuse', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-1', MERGE_BASE) + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + deps: makeDeps(), + request: { reuseLineStats: true } + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined, + reuseLineStats: true, + branchLineTotalMergeBase: MERGE_BASE + }) + }) + + it('sends the merge base on an abortable refresh', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-1', MERGE_BASE) + const controller = new AbortController() + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + deps: makeDeps(), + request: { signal: controller.signal } + }) + + expect(gitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + worktreePath: '/repo', + branchLineTotalMergeBase: MERGE_BASE + }) + ) + }) + + it('re-reads the gate on every pass so a moved fork point is not sent twice', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-1', MERGE_BASE) + + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + deps: makeDeps() + }) + setBranchLineTotalMergeBase('wt-1', 'rebased-merge-base') + await refreshGitStatusForWorktree({ + worktreeId: 'wt-1', + worktreePath: '/repo', + deps: makeDeps() + }) + + expect(gitStatus).toHaveBeenNthCalledWith(1, { + worktreePath: '/repo', + connectionId: undefined, + branchLineTotalMergeBase: MERGE_BASE + }) + expect(gitStatus).toHaveBeenNthCalledWith(2, { + worktreePath: '/repo', + connectionId: undefined, + branchLineTotalMergeBase: 'rebased-merge-base' + }) + }) + + it('omits the merge base from a strict refresh when no chip is visible', async () => { + const gitStatus = stubGitStatus() + + await refreshGitStatusForWorktreeStrict({ + worktreeId: 'wt-strict', + worktreePath: '/repo', + deps: { ...makeDeps(), fetchUpstreamStatus: undefined } + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined, + bypassEffectiveUpstreamNegativeCache: true + }) + }) + + // Why: a strict refresh is exactly the commit/push/sync that moves the number; + // skipping the gate there would blank the chip until the next automatic poll. + it('sends the merge base on a strict refresh', async () => { + const gitStatus = stubGitStatus() + setBranchLineTotalMergeBase('wt-strict', MERGE_BASE) + + await refreshGitStatusForWorktreeStrict({ + worktreeId: 'wt-strict', + worktreePath: '/repo', + deps: { ...makeDeps(), fetchUpstreamStatus: undefined } + }) + + expect(gitStatus).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined, + bypassEffectiveUpstreamNegativeCache: true, + branchLineTotalMergeBase: MERGE_BASE + }) + }) +}) 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 596a68f8df3..50f2cf068c7 100644 --- a/src/renderer/src/components/right-sidebar/git-status-refresh.ts +++ b/src/renderer/src/components/right-sidebar/git-status-refresh.ts @@ -1,4 +1,5 @@ import { getRuntimeGitStatus, getRuntimeGitUpstreamStatus } from '@/runtime/runtime-git-client' +import { getBranchLineTotalMergeBase } from './branch-line-total-request-gate' import { clearAutomaticPushTargetUpstreamStatusCache, getCachedAutomaticPushTargetUpstreamStatus, @@ -121,6 +122,9 @@ export async function refreshGitStatusForWorktree({ } }): Promise { const refreshOrder = beginAutomaticUpstreamRefresh(worktreeId) + // Why: every status path must carry the merge base, or the chip blanks out on + // whichever poll happened to omit it. + const branchLineTotalMergeBase = getBranchLineTotalMergeBase(worktreeId) try { const status = (await getRuntimeGitStatus( { @@ -129,12 +133,11 @@ export async function refreshGitStatusForWorktree({ worktreePath, connectionId }, - request - ? { - ...(request.reuseLineStats === true ? { reuseLineStats: true } : {}), - ...(request.signal ? { signal: request.signal } : {}) - } - : undefined + { + ...(request?.reuseLineStats === true ? { reuseLineStats: true } : {}), + ...(request?.signal ? { signal: request.signal } : {}), + ...(branchLineTotalMergeBase ? { branchLineTotalMergeBase } : {}) + } )) as GitStatusResult if (!claimAutomaticUpstreamRefreshApply(worktreeId, refreshOrder, request?.shouldApply)) { @@ -247,6 +250,7 @@ export async function refreshGitStatusForWorktreeStrict({ }): Promise<{ status: GitStatusResult; upstreamStatus: GitUpstreamStatus }> { beginStrictUpstreamRefresh(worktreeId) clearAutomaticPushTargetUpstreamStatusCache() + const strictBranchLineTotalMergeBase = getBranchLineTotalMergeBase(worktreeId) const status = (await getRuntimeGitStatus( { settings, @@ -257,7 +261,12 @@ export async function refreshGitStatusForWorktreeStrict({ { // Why: strict refreshes are user-triggered reconciliation and must not reuse // automatic polling's no-upstream backoff window. - bypassEffectiveUpstreamNegativeCache: true + bypassEffectiveUpstreamNegativeCache: true, + // Why: an absent total clears the chip, so a path that skipped the gate + // would blank it right after the commit or push that moved the number. + ...(strictBranchLineTotalMergeBase + ? { branchLineTotalMergeBase: strictBranchLineTotalMergeBase } + : {}) } )) as GitStatusResult diff --git a/src/renderer/src/components/right-sidebar/source-control-branch-context-row.test.tsx b/src/renderer/src/components/right-sidebar/source-control-branch-context-row.test.tsx index 4a454224997..0172dab5dd0 100644 --- a/src/renderer/src/components/right-sidebar/source-control-branch-context-row.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control-branch-context-row.test.tsx @@ -3,6 +3,7 @@ import type { ReactNode } from 'react' import { describe, expect, it, vi } from 'vitest' import { SourceControlBranchContextRow } from './source-control-branch-context-row' import type { GitBranchCompareSummary } from '../../../../shared/types' +import type { GitBranchLineTotal } from '../../../../shared/git-status-types' vi.mock('@/components/ui/tooltip', () => ({ Tooltip: ({ children }: { children: ReactNode }) => <>{children}, @@ -215,3 +216,144 @@ describe('SourceControlBranchContextRow', () => { expect(markup).toContain('aria-label="Open review page in browser"') }) }) + +function renderWithLineTotal( + branchLineTotal: GitBranchLineTotal | null | undefined, + summary: GitBranchCompareSummary | null = readySummary +): string { + return renderToStaticMarkup( + + ) +} + +describe('SourceControlBranchContextRow branch line total', () => { + it('renders both halves with grouped digits and a spoken label', () => { + const markup = renderWithLineTotal({ added: 8259, removed: 670, mergeBase: 'base' }) + + expect(markup).toContain('+8,259') + expect(markup).toContain('-670') + // Label reads raw digits; the grouped spans are decoration. + expect(markup).toContain('aria-label="8259 additions, 670 deletions"') + expect(markup).toContain('tabular-nums') + expect(markup).toContain('text-[color:var(--git-decoration-added)]') + expect(markup).toContain('text-[color:var(--git-decoration-deleted)]') + // Not clickable in v1 — the chip's scope differs from openBranchAllDiffs. + expect(markup).not.toContain(' diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 98972cf0760..29218566eb8 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -14867,7 +14867,7 @@ "other": "Other…", "otherPlaceholder": "Type your answer", "cancel": "Cancel", - "send": "Send answer", + "send": "Submit", "next": "Next", "skip": "Skip", "sending": "Sending…" diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 68a9bcb8fbd..e32c1ca2318 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -14526,7 +14526,7 @@ "other": "Otro…", "otherPlaceholder": "Escribe tu respuesta", "cancel": "Cancelar", - "send": "Enviar respuesta", + "send": "Enviar", "next": "Siguiente", "skip": "Omitir", "sending": "Enviando…" diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 9ef5a69cbae..16a92c12c02 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -14526,7 +14526,7 @@ "other": "他の…", "otherPlaceholder": "答えを入力してください", "cancel": "キャンセル", - "send": "回答を送信する", + "send": "送信", "next": "次へ", "skip": "スキップ", "sending": "送信中…" diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index d6c3e34fcd1..0489f18507a 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -14537,7 +14537,7 @@ "other": "다른…", "otherPlaceholder": "답변을 입력하세요", "cancel": "취소", - "send": "답변 보내기", + "send": "제출", "next": "다음", "skip": "건너뛰기", "sending": "전송 중…" diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index c5f255fe4e9..1b197b3eaac 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -14546,7 +14546,7 @@ "other": "其他…", "otherPlaceholder": "输入你的回答", "cancel": "取消", - "send": "发送回答", + "send": "提交", "next": "下一步", "skip": "跳过", "sending": "正在发送…" diff --git a/tests/e2e/native-chat-ask-user-question-card.spec.ts b/tests/e2e/native-chat-ask-user-question-card.spec.ts index a4cc2d7c4aa..a3788e5aa96 100644 --- a/tests/e2e/native-chat-ask-user-question-card.spec.ts +++ b/tests/e2e/native-chat-ask-user-question-card.spec.ts @@ -146,10 +146,10 @@ test.describe('Desktop chat AskUserQuestion card (#11761)', () => { // The submit button reads "Skip" until an option is chosen; picking one is // what proves the card is answerable rather than merely rendered. await orcaPage.getByRole('button', { name: /Spaces/ }).click() - await expect(orcaPage.getByRole('button', { name: 'Send answer' })).toBeVisible() + await expect(orcaPage.getByRole('button', { name: 'Submit' })).toBeVisible() await orcaPage.screenshot({ path: path.join(screenshotDir, '02-option-selected.png') }) - await orcaPage.getByRole('button', { name: 'Send answer' }).click() + await orcaPage.getByRole('button', { name: 'Submit' }).click() // The card owns the composer slot, so its disappearance is the visible // signal that the answer was accepted and chat input came back. await expect(orcaPage.getByText(QUESTION)).toHaveCount(0, { timeout: 20_000 }) From ae1ed5e886acdc4352e5c2de70fed04331386daa Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:29:46 -0700 Subject: [PATCH 07/18] Remove source control group order preference (#12785) * Reorder source control to show staged changes first by default Stages are closest to the commit action and most relevant to the commit workflow. Merges untracked files into Changes visually while preserving their Git area. Removes the untracked-first preset and includes migration logic for existing user settings. * Drop source control group order user preference Remove the sourceControlGroupOrder setting and related UI, migrations, and persistence logic. The source control view now always displays sections in the order: staged changes, unstaged changes, untracked files. * Reorder source control to show changes before staged Aligns with the edit-stage-commit workflow by showing unstaged changes (active edits) before staged changes (queued for commit). --- .oxlintrc.json | 7 +- .../scripts/relay-watcher-fault-harness.mjs | 22 ++--- .../source-control/mobile-git-status.test.ts | 7 +- .../src/source-control/mobile-git-status.ts | 10 +- .../runtime-home-service.test.ts | 1 - src/main/codex-accounts/service.test.ts | 1 - src/main/persistence.test.ts | 49 +++------- src/main/persistence.ts | 36 +++---- src/main/runtime/orca-runtime.ts | 5 +- .../editor/CombinedDiffFileTree.tsx | 53 ++++++---- .../GitHubMarkdownComposerEditorPane.tsx | 5 +- .../right-sidebar/SourceControl.tsx | 98 +++++++++++++------ .../discard-all-sequence.test.ts | 9 ++ .../right-sidebar/discard-all-sequence.ts | 5 +- ...ource-control-discard-confirmation.test.ts | 9 ++ .../source-control-discard-confirmation.ts | 19 +++- .../source-control-discard-dialog.tsx | 13 ++- .../source-control-section-order.test.ts | 60 ++++++------ .../source-control-section-order.ts | 28 +++--- .../src/components/settings/GitPane.test.ts | 70 +------------ .../src/components/settings/GitPane.tsx | 91 +---------------- .../src/components/settings/git-search.ts | 27 ----- src/renderer/src/i18n/locales/en.json | 19 ++-- src/renderer/src/i18n/locales/es.json | 12 --- src/renderer/src/i18n/locales/ja.json | 12 --- src/renderer/src/i18n/locales/ko.json | 12 --- src/renderer/src/i18n/locales/zh.json | 12 --- src/shared/constants.test.ts | 4 +- src/shared/constants.ts | 2 - src/shared/source-control-group-order.test.ts | 20 ---- src/shared/source-control-group-order.ts | 9 -- src/shared/types.ts | 3 - ...script-prompt-unreadable-orca-yaml.spec.ts | 5 +- 33 files changed, 267 insertions(+), 468 deletions(-) delete mode 100644 src/shared/source-control-group-order.test.ts delete mode 100644 src/shared/source-control-group-order.ts diff --git a/.oxlintrc.json b/.oxlintrc.json index 4cdb4f2c0e2..0f679f17d52 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -150,10 +150,5 @@ } } ], - "ignorePatterns": [ - "**/node_modules", - "**/dist", - "**/out", - "tests/e2e/.cross-version-checkouts" - ] + "ignorePatterns": ["**/node_modules", "**/dist", "**/out", "tests/e2e/.cross-version-checkouts"] } diff --git a/config/scripts/relay-watcher-fault-harness.mjs b/config/scripts/relay-watcher-fault-harness.mjs index 12e36f72490..b27a1f945d9 100644 --- a/config/scripts/relay-watcher-fault-harness.mjs +++ b/config/scripts/relay-watcher-fault-harness.mjs @@ -109,9 +109,7 @@ function waitForStdoutSentinel(proc, protocol, stderr) { settled = true proc.stdout.off('data', onData) rejectPromise( - new Error( - `process exited before sentinel (code=${code}, signal=${signal})\n${stderr()}` - ) + new Error(`process exited before sentinel (code=${code}, signal=${signal})\n${stderr()}`) ) } proc.stdout.on('data', onData) @@ -162,11 +160,7 @@ function createRelayClient(entryPath, args, env, protocol) { }) const waitForMessage = (startIndex, predicate, label) => - pollUntil( - () => messages.slice(startIndex).find(predicate), - label, - streams.stderr - ) + pollUntil(() => messages.slice(startIndex).find(predicate), label, streams.stderr) const request = async (method, params = {}) => { const id = nextSequence++ @@ -317,8 +311,10 @@ async function main() { ) await relay.request('fs.watch', { rootPath: watchRoot }) - const firstWatcherPid = await waitForWatcherPid(pidFile, undefined, () => - `${daemonStreams.stderr()}\n${relay.stderr()}` + const firstWatcherPid = await waitForWatcherPid( + pidFile, + undefined, + () => `${daemonStreams.stderr()}\n${relay.stderr()}` ) const beforePath = join(watchRoot, 'before.txt') startIndex = relay.messageCount() @@ -330,8 +326,10 @@ async function main() { const faultSignal = process.platform === 'win32' ? 'SIGTERM' : 'SIGSEGV' startIndex = relay.messageCount() process.kill(firstWatcherPid, faultSignal) - const replacementWatcherPid = await waitForWatcherPid(pidFile, firstWatcherPid, () => - `${daemonStreams.stderr()}\n${relay.stderr()}` + const replacementWatcherPid = await waitForWatcherPid( + pidFile, + firstWatcherPid, + () => `${daemonStreams.stderr()}\n${relay.stderr()}` ) await relay.waitForNotification(startIndex, 'fs.changed', (params) => Array.isArray(params.events) diff --git a/mobile/src/source-control/mobile-git-status.test.ts b/mobile/src/source-control/mobile-git-status.test.ts index a9e2b554f91..007fb317ce9 100644 --- a/mobile/src/source-control/mobile-git-status.test.ts +++ b/mobile/src/source-control/mobile-git-status.test.ts @@ -29,11 +29,8 @@ describe('mobile source control status helpers', () => { it('builds sections in the mobile source control order', () => { const sections = buildMobileSourceControlSections(entries) - expect(sections.map((section) => section.title)).toEqual([ - 'Changes', - 'Untracked Files', - 'Staged Changes' - ]) + expect(sections.map((section) => section.title)).toEqual(['Changes', 'Staged Changes']) + expect(sections[0]?.data.map((entry) => entry.path)).toEqual(['a.ts', 'new.ts']) }) it('computes actionable path sets', () => { diff --git a/mobile/src/source-control/mobile-git-status.ts b/mobile/src/source-control/mobile-git-status.ts index 65fcf61e211..2d7b0795918 100644 --- a/mobile/src/source-control/mobile-git-status.ts +++ b/mobile/src/source-control/mobile-git-status.ts @@ -19,7 +19,7 @@ export type MobileSourceControlSection = { unstaged: 'Changes', @@ -59,7 +59,13 @@ export function buildMobileSourceControlSections ({ area, title: AREA_TITLES[area], - data: entries.filter((entry) => entry.area === area).sort(compareGitStatusEntries) + data: entries + .filter((entry) => + area === 'unstaged' + ? entry.area === 'unstaged' || entry.area === 'untracked' + : entry.area === area + ) + .sort(compareGitStatusEntries) })).filter((section) => section.data.length > 0) } diff --git a/src/main/codex-accounts/runtime-home-service.test.ts b/src/main/codex-accounts/runtime-home-service.test.ts index 8d3a52fa823..f802fecb7e7 100644 --- a/src/main/codex-accounts/runtime-home-service.test.ts +++ b/src/main/codex-accounts/runtime-home-service.test.ts @@ -105,7 +105,6 @@ function createSettings(overrides: TestSettingsOverrides = {}): GlobalSettings { openLinksInAppPreferencePrompted: false, rightSidebarOpenByDefault: true, sourceControlViewMode: 'list', - sourceControlGroupOrder: 'changes-first', sourceControlCompareAgainstUpstream: false, showTitlebarAppName: true, showTasksButton: true, diff --git a/src/main/codex-accounts/service.test.ts b/src/main/codex-accounts/service.test.ts index 47fd0c67850..79bd2ea7bbe 100644 --- a/src/main/codex-accounts/service.test.ts +++ b/src/main/codex-accounts/service.test.ts @@ -97,7 +97,6 @@ function createSettings(overrides: Partial = {}): GlobalSettings openLinksInAppPreferencePrompted: false, rightSidebarOpenByDefault: true, sourceControlViewMode: 'list', - sourceControlGroupOrder: 'changes-first', sourceControlCompareAgainstUpstream: false, showTitlebarAppName: true, showTasksButton: true, diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 81faa5f6297..24cc6e512cc 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -676,7 +676,6 @@ describe('Store', () => { const settings = store.getSettings() expect(settings.branchPrefix).toBe('git-username') expect(settings.refreshLocalBaseRefOnWorktreeCreate).toBe(false) - expect(settings.sourceControlGroupOrder).toBe('changes-first') expect(settings.theme).toBe('system') expect(settings.appIcon).toBe('classic') expect(settings.appFontFamily).toBe('Geist') @@ -2784,21 +2783,6 @@ describe('Store', () => { expect(store.getSettings().terminalShortcutPolicy).toBe('orca-first') }) - it('normalizes malformed source control group order on load', async () => { - writeDataFile({ - schemaVersion: 1, - repos: [], - worktreeMeta: {}, - settings: { sourceControlGroupOrder: 'tracked-first' }, - ui: {}, - githubCache: { pr: {}, issue: {} }, - workspaceSession: {} - }) - - const store = await createStore() - expect(store.getSettings().sourceControlGroupOrder).toBe('changes-first') - }) - it('repairs drifted task provider defaults on load', async () => { writeDataFile({ schemaVersion: 1, @@ -5817,15 +5801,21 @@ describe('Store', () => { expect(store.getSettings().sourceControlViewMode).toBe('tree') }) - it('updateSettings persists sourceControlGroupOrder as a user setting', async () => { + it('drops retired Source Control order preferences', async () => { + writeDataFile({ + ...getDefaultPersistedState(testState.dir), + settings: { + ...getDefaultPersistedState(testState.dir).settings, + sourceControlGroupOrder: 'changes-first', + sourceControlHierarchyDefaultedV2: true + } + } as never) + const store = await createStore() - expect(store.getSettings().sourceControlGroupOrder).toBe('changes-first') - - store.updateSettings({ sourceControlGroupOrder: 'staged-first' }) - expect(store.getSettings().sourceControlGroupOrder).toBe('staged-first') - - store.updateSettings({ sourceControlGroupOrder: 'tracked-first' as never }) - expect(store.getSettings().sourceControlGroupOrder).toBe('changes-first') + store.flush() + const persisted = readDataFile() as { settings?: Record } + expect(persisted.settings).not.toHaveProperty('sourceControlGroupOrder') + expect(persisted.settings).not.toHaveProperty('sourceControlHierarchyDefaultedV2') }) it('updateSettings normalizes terminal shortcut policy', async () => { @@ -5883,18 +5873,15 @@ describe('Store', () => { const store = await createStore() expect(store.getSettings().sourceControlViewMode).toBe('list') - expect(store.getSettings().sourceControlGroupOrder).toBe('changes-first') - - store.updateSettings({ sourceControlViewMode: 'tree', sourceControlGroupOrder: 'staged-first' }) + store.updateSettings({ sourceControlViewMode: 'tree' }) store.flush() const persisted = readDataFile() as { - settings?: { sourceControlGroupOrder?: string; sourceControlViewMode?: string } + settings?: { sourceControlViewMode?: string } workspaceSession?: typeof workspaceSession worktreeMeta?: Record } expect(persisted.settings?.sourceControlViewMode).toBe('tree') - expect(persisted.settings?.sourceControlGroupOrder).toBe('staged-first') expect(persisted.workspaceSession).toEqual({ ...getDefaultWorkspaceSession(), ...workspaceSession @@ -5906,13 +5893,9 @@ describe('Store', () => { expect(collectPropertyPaths(persisted, 'sourceControlViewMode')).toEqual([ 'settings.sourceControlViewMode' ]) - expect(collectPropertyPaths(persisted, 'sourceControlGroupOrder')).toEqual([ - 'settings.sourceControlGroupOrder' - ]) const reloaded = await createStore() expect(reloaded.getSettings().sourceControlViewMode).toBe('tree') - expect(reloaded.getSettings().sourceControlGroupOrder).toBe('staged-first') expect(reloaded.getWorkspaceSession().activeWorktreeId).toBe('repo1::/worktree-a') }) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 2afccf7e0ae..af82bcbdd89 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -182,7 +182,6 @@ import { } from '../shared/mobile-pairing-custom-address' import { normalizeOpenInApplications } from '../shared/open-in-applications' import { normalizeTerminalShortcutPolicy } from '../shared/keybindings' -import { normalizeSourceControlGroupOrder } from '../shared/source-control-group-order' import { normalizeAppIconId } from '../shared/app-icon' import { normalizeTerminalCustomThemes } from '../shared/terminal-custom-themes' import { @@ -650,12 +649,22 @@ function readLegacyTerminalScrollbackSettings(settings: unknown): LegacyTerminal : {} } -function stripLegacyTerminalScrollbackBytes( +function stripRetiredSettingsFields( settings: Partial | undefined ): Partial { - const { terminalScrollbackBytes: _legacyScrollbackBytes, ...rest } = (settings ?? - {}) as Partial & { terminalScrollbackBytes?: unknown } + const { + terminalScrollbackBytes: _legacyScrollbackBytes, + sourceControlGroupOrder: _sourceControlGroupOrder, + sourceControlHierarchyDefaultedV2: _sourceControlHierarchyDefaultedV2, + ...rest + } = (settings ?? {}) as Partial & { + terminalScrollbackBytes?: unknown + sourceControlGroupOrder?: unknown + sourceControlHierarchyDefaultedV2?: unknown + } void _legacyScrollbackBytes + void _sourceControlGroupOrder + void _sourceControlHierarchyDefaultedV2 return rest } @@ -3316,15 +3325,6 @@ export class Store { ) { this.loadNeedsSave = true } - const normalizedSourceControlGroupOrder = normalizeSourceControlGroupOrder( - parsed.settings?.sourceControlGroupOrder - ) - if ( - parsed.settings?.sourceControlGroupOrder !== undefined && - parsed.settings.sourceControlGroupOrder !== normalizedSourceControlGroupOrder - ) { - this.loadNeedsSave = true - } result = { ...defaults, ...parsed, @@ -3346,7 +3346,7 @@ export class Store { settings: { ...defaults.settings, // Why (#7977): keep persisted experimentalNewWorktreeCardStyle:true — v1.4.130's onboarding auto-wrote it as a plain boolean, so it's indistinguishable from a real opt-in; only the default changed. - ...stripLegacyTerminalScrollbackBytes(parsed.settings), + ...stripRetiredSettingsFields(parsed.settings), prBotAuthorOverrides: normalizePRBotAuthorOverrides( parsed.settings?.prBotAuthorOverrides ), @@ -3420,7 +3420,6 @@ export class Store { }), notifications: normalizeNotificationSettings(parsed.settings?.notifications), sourceControlAi: migratedSourceControlAi, - sourceControlGroupOrder: normalizedSourceControlGroupOrder, // Why: rollback builds still read commitMessageAi, so refresh the legacy projection from sourceControlAi for compat. commitMessageAi: projectSourceControlAiToLegacyCommitMessageAi( migratedSourceControlAi, @@ -5691,7 +5690,7 @@ export class Store { updates: Partial, options: { notifyListeners?: boolean; originWebContentsId?: number } = {} ): GlobalSettings { - const sanitizedUpdates = stripLegacyTerminalScrollbackBytes(updates) + const sanitizedUpdates = stripRetiredSettingsFields(updates) // Why: coerce to boolean here (not the IPC edge) so every write path is covered and a truthy non-bool can't persist as "tray-minimize on". if ('minimizeToTrayOnClose' in updates) { sanitizedUpdates.minimizeToTrayOnClose = updates.minimizeToTrayOnClose === true @@ -5759,11 +5758,6 @@ export class Store { updates.terminalShortcutPolicy ) } - if ('sourceControlGroupOrder' in updates) { - sanitizedUpdates.sourceControlGroupOrder = normalizeSourceControlGroupOrder( - updates.sourceControlGroupOrder - ) - } if ('appIcon' in updates) { sanitizedUpdates.appIcon = normalizeAppIconId(updates.appIcon) } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 5e23498f370..b85bb01b463 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -10762,10 +10762,7 @@ export class OrcaRuntimeService { // A spawn published (or admission pending) this generation already // attaches the provider stream; a replacement under a reused id must not // read as the discovered never-attached session it replaced. - if ( - this.spawnPublishedPtys.has(ptyId) || - this.pendingPtyRegistrationIncarnations.has(ptyId) - ) { + if (this.spawnPublishedPtys.has(ptyId) || this.pendingPtyRegistrationIncarnations.has(ptyId)) { return false } // SSH panes have their own lease/reattach machinery. diff --git a/src/renderer/src/components/editor/CombinedDiffFileTree.tsx b/src/renderer/src/components/editor/CombinedDiffFileTree.tsx index fc59c2dd75c..dd6f7b99b93 100644 --- a/src/renderer/src/components/editor/CombinedDiffFileTree.tsx +++ b/src/renderer/src/components/editor/CombinedDiffFileTree.tsx @@ -10,7 +10,7 @@ import { compactSourceControlTree, flattenSourceControlTree } from '@/components/right-sidebar/source-control-tree' -import type { GitBranchChangeEntry, GitStagingArea, GitStatusEntry } from '../../../../shared/types' +import type { GitBranchChangeEntry, GitStagingArea } from '../../../../shared/types' import { getEntryExtension, getFilteredCombinedDiffFileTreeEntries, @@ -22,6 +22,11 @@ import { import { CombinedDiffFileTreeRow, type CombinedDiffTreeNode } from './combined-diff-file-tree-row' import { useCombinedDiffFileTreeResize } from './use-combined-diff-file-tree-resize' import { translate } from '@/i18n/i18n' +import { + mergeUntrackedIntoChanges, + SOURCE_CONTROL_GROUP_ORDER, + type SourceControlEntryGroups +} from '@/components/right-sidebar/source-control-section-order' export { createCombinedDiffSectionIndexMap, @@ -30,7 +35,6 @@ export { handleCombinedDiffFileTreeNavigation } from './combined-diff-file-tree-model' -const UNCOMMITTED_AREA_ORDER: readonly GitStagingArea[] = ['unstaged', 'staged', 'untracked'] const UNCOMMITTED_AREA_LABELS: Record = { unstaged: 'Changes', staged: 'Staged Changes', @@ -39,26 +43,35 @@ const UNCOMMITTED_AREA_LABELS: Record = { function buildUncommittedRows( entries: readonly CombinedDiffFileTreeEntry[], - collapsedDirectoryKeys: ReadonlySet + collapsedDirectoryKeys: ReadonlySet, + areaOrder: readonly GitStagingArea[] ): { area: GitStagingArea; label: string; rows: CombinedDiffTreeNode[] }[] { - return UNCOMMITTED_AREA_ORDER.map((area) => { - const areaEntries = entries.filter( - (entry): entry is GitStatusEntry => isGitStatusEntry(entry) && entry.area === area - ) - if (areaEntries.length === 0) { - return null + const groups: SourceControlEntryGroups = { staged: [], unstaged: [], untracked: [] } + for (const entry of entries) { + if (isGitStatusEntry(entry)) { + groups[entry.area].push(entry) } + } + const displayGroups = mergeUntrackedIntoChanges(groups) - const roots = compactSourceControlTree(buildGitStatusSourceControlTree(area, areaEntries)) - return { - area, - label: UNCOMMITTED_AREA_LABELS[area], - rows: flattenSourceControlTree(roots, collapsedDirectoryKeys) as CombinedDiffTreeNode[] - } - }).filter( - (group): group is { area: GitStagingArea; label: string; rows: CombinedDiffTreeNode[] } => - Boolean(group) - ) + return areaOrder + .map((area) => { + const areaEntries = displayGroups[area] + if (areaEntries.length === 0) { + return null + } + + const roots = compactSourceControlTree(buildGitStatusSourceControlTree(area, areaEntries)) + return { + area, + label: UNCOMMITTED_AREA_LABELS[area], + rows: flattenSourceControlTree(roots, collapsedDirectoryKeys) as CombinedDiffTreeNode[] + } + }) + .filter( + (group): group is { area: GitStagingArea; label: string; rows: CombinedDiffTreeNode[] } => + Boolean(group) + ) } function buildBranchRows( @@ -153,7 +166,7 @@ export function CombinedDiffFileTree({ const uncommittedGroups = React.useMemo( () => mode === 'all' || mode === 'uncommitted' - ? buildUncommittedRows(filteredEntries, collapsedDirectoryKeys) + ? buildUncommittedRows(filteredEntries, collapsedDirectoryKeys, SOURCE_CONTROL_GROUP_ORDER) : [], [collapsedDirectoryKeys, filteredEntries, mode] ) diff --git a/src/renderer/src/components/github/GitHubMarkdownComposerEditorPane.tsx b/src/renderer/src/components/github/GitHubMarkdownComposerEditorPane.tsx index 6914f9b3aba..792799376ea 100644 --- a/src/renderer/src/components/github/GitHubMarkdownComposerEditorPane.tsx +++ b/src/renderer/src/components/github/GitHubMarkdownComposerEditorPane.tsx @@ -13,7 +13,10 @@ export function GitHubMarkdownComposerEditorPane({ const scrollContainerRef = useRef(null) return ( -
+
entry.area === 'untracked') } } @@ -1060,7 +1067,6 @@ function SourceControlInner(): React.JSX.Element { settings?.sourceControlViewMode ) const sourceControlViewMode = persistedSourceControlViewMode - const sourceControlGroupOrder = resolveSourceControlGroupOrder(settings?.sourceControlGroupOrder) const [collapsedTreeDirs, setCollapsedTreeDirs] = useState>(new Set()) const [baseRefDialogOpen, setBaseRefDialogOpen] = useState(false) const [pendingDiscard, setPendingDiscard] = useState(null) @@ -1796,13 +1802,19 @@ function SourceControlInner(): React.JSX.Element { [fileFilterState, grouped] ) + const mergedGrouped = useMemo(() => mergeUntrackedIntoChanges(grouped), [grouped]) + const mergedFilteredGrouped = useMemo( + () => mergeUntrackedIntoChanges(filteredGrouped), + [filteredGrouped] + ) + const displaySections = useMemo( - () => buildSourceControlDisplaySections(filteredGrouped, sourceControlGroupOrder), - [filteredGrouped, sourceControlGroupOrder] + () => buildSourceControlDisplaySections(mergedFilteredGrouped, SOURCE_CONTROL_GROUP_ORDER), + [mergedFilteredGrouped] ) const unfilteredDisplaySections = useMemo( - () => buildSourceControlDisplaySections(grouped, sourceControlGroupOrder), - [grouped, sourceControlGroupOrder] + () => buildSourceControlDisplaySections(mergedGrouped, SOURCE_CONTROL_GROUP_ORDER), + [mergedGrouped] ) const unfilteredDisplaySectionsById = useMemo( () => new Map(unfilteredDisplaySections.map((section) => [section.id, section])), @@ -5400,7 +5412,9 @@ function SourceControlInner(): React.JSX.Element { if (!worktreePath || !activeWorktreeId || isExecutingBulk) { return } - const paths = confirmedPaths ? [...confirmedPaths] : getDiscardAllPaths(grouped[area], area) + const paths = confirmedPaths + ? [...confirmedPaths] + : getDiscardAllPaths(mergedGrouped[area], discardAllAreaFilter(area)) if (paths.length === 0) { return } @@ -5472,7 +5486,7 @@ function SourceControlInner(): React.JSX.Element { activeRepoSettings, worktreePath, activeWorktreeId, - grouped, + mergedGrouped, isExecutingBulk, clearSelection, discardMany, @@ -5486,13 +5500,21 @@ function SourceControlInner(): React.JSX.Element { if (!worktreePath || !activeWorktreeId || isExecutingBulk) { return } - const paths = confirmedPaths ? [...confirmedPaths] : getDiscardAllPaths(grouped[area], area) + const paths = confirmedPaths + ? [...confirmedPaths] + : getDiscardAllPaths(mergedGrouped[area], discardAllAreaFilter(area)) if (paths.length === 0) { return } - setPendingDiscard({ kind: 'area', area, paths }) + setPendingDiscard({ + kind: 'area', + area, + paths, + hasUntracked: + area === 'unstaged' && mergedGrouped.unstaged.some((entry) => entry.area === 'untracked') + }) }, - [activeWorktreeId, grouped, isExecutingBulk, worktreePath] + [activeWorktreeId, isExecutingBulk, mergedGrouped, worktreePath] ) const requestDiscardEntry = useCallback( @@ -5914,7 +5936,8 @@ function SourceControlInner(): React.JSX.Element { .filter(isStageableStatusEntry) .map((entry) => entry.path) const unstageAllPaths = getUnstageAllPaths(actionItems) - const discardAllPaths = getDiscardAllPaths(actionItems, area) + const discardAllPaths = getDiscardAllPaths(actionItems, discardAllAreaFilter(area)) + const discardHasUntracked = actionItems.some((entry) => entry.area === 'untracked') const canStageAll = !normalizedFilter && stageAllPaths.length > 0 const canUnstageAll = !normalizedFilter && unstageAllPaths.length > 0 const canRevertAll = !normalizedFilter && discardAllPaths.length > 0 @@ -5937,7 +5960,7 @@ function SourceControlInner(): React.JSX.Element {
{canRevertAll && ( { event.stopPropagation() @@ -6055,11 +6083,12 @@ function SourceControlInner(): React.JSX.Element { isExecutingBulk={isExecutingBulk} isCollapsed={collapsedTreeDirs.has(node.key)} onToggle={() => toggleTreeDir(node.key)} - onRequestDiscardPaths={(discardArea, paths) => + onRequestDiscardPaths={(discardArea, paths, hasUntracked) => setPendingDiscard({ kind: 'area', area: discardArea, - paths + paths, + hasUntracked }) } onStagePaths={handleStageAllPaths} @@ -7698,7 +7727,11 @@ function SourceControlTreeDirectoryRow({ isExecutingBulk: boolean isCollapsed: boolean onToggle: () => void - onRequestDiscardPaths: (area: DiscardAllArea, paths: readonly string[]) => void + onRequestDiscardPaths: ( + area: DiscardAllArea, + paths: readonly string[], + hasUntracked: boolean + ) => void onStagePaths: (paths: readonly string[]) => Promise onUnstagePaths: (paths: readonly string[]) => Promise }): React.JSX.Element { @@ -7737,21 +7770,30 @@ function SourceControlTreeDirectoryRow({
{canDiscard && ( { event.stopPropagation() - onRequestDiscardPaths(node.area, actionPaths.discardPaths) + onRequestDiscardPaths( + node.area, + actionPaths.discardPaths, + actionPaths.discardHasUntracked + ) }} disabled={isExecutingBulk} /> diff --git a/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts b/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts index e808af6da27..d8f76c48f3f 100644 --- a/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts +++ b/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts @@ -30,6 +30,15 @@ describe('getDiscardAllPaths', () => { expect(getDiscardAllPaths(entries, 'untracked')).toEqual(['c.ts']) }) + it('accepts mixed Changes areas when untracked files are combined', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'changed.ts', area: 'unstaged' }), + entry({ path: 'new.ts', area: 'untracked', status: 'untracked' }), + entry({ path: 'ready.ts', area: 'staged' }) + ] + expect(getDiscardAllPaths(entries, ['unstaged', 'untracked'])).toEqual(['changed.ts', 'new.ts']) + }) + it('skips entries with an unresolved conflict', () => { const entries: GitStatusEntry[] = [ entry({ path: 'clean.ts', area: 'unstaged' }), diff --git a/src/renderer/src/components/right-sidebar/discard-all-sequence.ts b/src/renderer/src/components/right-sidebar/discard-all-sequence.ts index c52854fcd91..073ba321a2a 100644 --- a/src/renderer/src/components/right-sidebar/discard-all-sequence.ts +++ b/src/renderer/src/components/right-sidebar/discard-all-sequence.ts @@ -9,12 +9,13 @@ export type DiscardAllArea = 'staged' | 'unstaged' | 'untracked' */ export function getDiscardAllPaths( entries: readonly GitStatusEntry[], - area: DiscardAllArea + area: DiscardAllArea | readonly DiscardAllArea[] ): string[] { + const areas = Array.isArray(area) ? area : [area] return entries .filter( (entry) => - entry.area === area && + areas.includes(entry.area) && entry.conflictStatus !== 'unresolved' && entry.conflictStatus !== 'resolved_locally' ) diff --git a/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.test.ts b/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.test.ts index 48e28160af6..6f6632feea8 100644 --- a/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.test.ts @@ -111,4 +111,13 @@ describe('getDiscardAreaConfirmationCopy', () => { confirmLabel: 'Discard all' }) }) + + it('discloses permanent deletion for combined Changes', () => { + expect(getDiscardAreaConfirmationCopy('unstaged', 3, true)).toEqual({ + title: 'Discard changes and delete untracked files?', + description: + 'Tracked changes will be reverted and untracked files will be permanently deleted. This cannot be undone.', + confirmLabel: 'Delete and discard' + }) + }) }) diff --git a/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.ts b/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.ts index ff020c434ec..03a6805e7f0 100644 --- a/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.ts +++ b/src/renderer/src/components/right-sidebar/source-control-discard-confirmation.ts @@ -62,8 +62,25 @@ export function getDiscardEntryConfirmationCopy( export function getDiscardAreaConfirmationCopy( area: DiscardAllArea, - count: number + count: number, + hasUntracked = false ): DiscardConfirmationCopy { + if (area === 'unstaged' && hasUntracked) { + return { + title: translate( + 'auto.components.right.sidebar.source.control.discard.confirmation.mixedTitle', + 'Discard changes and delete untracked files?' + ), + description: translate( + 'auto.components.right.sidebar.source.control.discard.confirmation.mixedDescription', + 'Tracked changes will be reverted and untracked files will be permanently deleted. This cannot be undone.' + ), + confirmLabel: translate( + 'auto.components.right.sidebar.source.control.discard.confirmation.mixedConfirm', + 'Delete and discard' + ) + } + } switch (area) { case 'untracked': return { diff --git a/src/renderer/src/components/right-sidebar/source-control-discard-dialog.tsx b/src/renderer/src/components/right-sidebar/source-control-discard-dialog.tsx index 6741a72ac55..25e2366b616 100644 --- a/src/renderer/src/components/right-sidebar/source-control-discard-dialog.tsx +++ b/src/renderer/src/components/right-sidebar/source-control-discard-dialog.tsx @@ -20,7 +20,12 @@ import { translate } from '@/i18n/i18n' export type PendingDiscardConfirmation = | { kind: 'entry'; entry: GitStatusEntry } - | { kind: 'area'; area: DiscardAllArea; paths: readonly string[] } + | { + kind: 'area' + area: DiscardAllArea + paths: readonly string[] + hasUntracked?: boolean + } export function focusDiscardDialogConfirmButton( event: Event, @@ -51,7 +56,11 @@ export function SourceControlDiscardDialog({ if (pendingDiscard.kind === 'entry') { return getDiscardEntryConfirmationCopy(pendingDiscard.entry) } - return getDiscardAreaConfirmationCopy(pendingDiscard.area, pendingDiscard.paths.length) + return getDiscardAreaConfirmationCopy( + pendingDiscard.area, + pendingDiscard.paths.length, + pendingDiscard.hasUntracked + ) }, [pendingDiscard]) const PendingDiscardIcon = pendingDiscardCopy?.confirmLabel.startsWith('Delete') ? Trash : Undo2 diff --git a/src/renderer/src/components/right-sidebar/source-control-section-order.test.ts b/src/renderer/src/components/right-sidebar/source-control-section-order.test.ts index fa563470d66..91378833b0d 100644 --- a/src/renderer/src/components/right-sidebar/source-control-section-order.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-section-order.test.ts @@ -4,7 +4,8 @@ import { buildSourceControlDisplaySections, getConflictReviewEntries, getSourceControlSectionViewAction, - resolveSourceControlGroupOrder, + mergeUntrackedIntoChanges, + SOURCE_CONTROL_GROUP_ORDER, splitPinnedSourceControlConflicts, type SourceControlEntryGroups } from './source-control-section-order' @@ -26,37 +27,38 @@ function groups(partial: Partial): SourceControlEntryG } } -describe('resolveSourceControlGroupOrder', () => { - it('keeps Changes first by default', () => { - expect(resolveSourceControlGroupOrder(undefined)).toEqual(['unstaged', 'staged', 'untracked']) +describe('SOURCE_CONTROL_GROUP_ORDER', () => { + it('follows the edit, stage, commit workflow', () => { + expect(SOURCE_CONTROL_GROUP_ORDER).toEqual(['unstaged', 'staged', 'untracked']) }) +}) - it('supports staged-first and untracked-first presets', () => { - expect(resolveSourceControlGroupOrder('staged-first')).toEqual([ - 'staged', - 'unstaged', - 'untracked' - ]) - expect(resolveSourceControlGroupOrder('untracked-first')).toEqual([ - 'untracked', - 'unstaged', - 'staged' - ]) +describe('mergeUntrackedIntoChanges', () => { + it('folds untracked entries into Changes without changing their Git area', () => { + const unstaged = entry({ area: 'unstaged', path: 'changed.ts' }) + const untracked = entry({ area: 'untracked', path: 'new.ts', status: 'untracked' }) + const merged = mergeUntrackedIntoChanges( + groups({ unstaged: [unstaged], untracked: [untracked] }) + ) + + expect(merged.unstaged).toEqual([unstaged, untracked]) + expect(merged.untracked).toEqual([]) + expect(merged.unstaged[1]?.area).toBe('untracked') }) }) describe('buildSourceControlDisplaySections', () => { - it('uses the configured order for normal sections', () => { + it('uses the fixed workflow order for normal sections', () => { const sections = buildSourceControlDisplaySections( groups({ staged: [entry({ area: 'staged', path: 'staged.ts' })], unstaged: [entry({ area: 'unstaged', path: 'changed.ts' })], untracked: [entry({ area: 'untracked', path: 'new.ts', status: 'untracked' })] }), - resolveSourceControlGroupOrder('staged-first') + SOURCE_CONTROL_GROUP_ORDER ) - expect(sections.map((section) => section.id)).toEqual(['staged', 'unstaged', 'untracked']) + expect(sections.map((section) => section.id)).toEqual(['unstaged', 'staged', 'untracked']) }) it('keeps conflicts pinned before the configured normal order', () => { @@ -74,13 +76,13 @@ describe('buildSourceControlDisplaySections', () => { ], untracked: [entry({ area: 'untracked', path: 'new.ts', status: 'untracked' })] }), - resolveSourceControlGroupOrder('staged-first') + SOURCE_CONTROL_GROUP_ORDER ) expect(sections.map((section) => section.id)).toEqual([ 'conflicts', - 'staged', 'unstaged', + 'staged', 'untracked' ]) }) @@ -100,10 +102,7 @@ describe('buildSourceControlDisplaySections', () => { const input = groups({ unstaged: [unresolved, resolved, normal] }) const split = splitPinnedSourceControlConflicts(input) - const sections = buildSourceControlDisplaySections( - input, - resolveSourceControlGroupOrder('changes-first') - ) + const sections = buildSourceControlDisplaySections(input, SOURCE_CONTROL_GROUP_ORDER) expect(split.pinnedConflicts.map((item) => item.path)).toEqual(['conflict.ts', 'resolved.ts']) expect(split.normalGroups.unstaged.map((item) => item.path)).toEqual(['normal.ts']) @@ -123,10 +122,7 @@ describe('buildSourceControlDisplaySections', () => { const input = groups({ staged: [resolvedStaged, staged] }) const split = splitPinnedSourceControlConflicts(input) - const sections = buildSourceControlDisplaySections( - input, - resolveSourceControlGroupOrder('staged-first') - ) + const sections = buildSourceControlDisplaySections(input, SOURCE_CONTROL_GROUP_ORDER) expect(split.pinnedConflicts).toEqual([resolvedStaged]) expect(split.normalGroups.staged).toEqual([staged]) @@ -167,7 +163,7 @@ describe('buildSourceControlDisplaySections', () => { entry({ area: 'unstaged', path: 'normal.ts' }) ] }), - resolveSourceControlGroupOrder('changes-first') + SOURCE_CONTROL_GROUP_ORDER ) expect(getSourceControlSectionViewAction(sections[0]!)).toEqual({ @@ -191,7 +187,7 @@ describe('buildSourceControlDisplaySections', () => { const normal = entry({ area: 'unstaged', path: 'normal.ts' }) const sections = buildSourceControlDisplaySections( groups({ unstaged: [pinned, normal] }), - resolveSourceControlGroupOrder('changes-first') + SOURCE_CONTROL_GROUP_ORDER ) expect(getSourceControlSectionViewAction(sections[1]!)).toEqual({ @@ -212,7 +208,7 @@ describe('buildSourceControlDisplaySections', () => { groups({ unstaged: [resolved] }), - resolveSourceControlGroupOrder('changes-first') + SOURCE_CONTROL_GROUP_ORDER ) expect(getSourceControlSectionViewAction(sections[0]!)).toEqual({ @@ -240,7 +236,7 @@ describe('buildSourceControlDisplaySections', () => { staged: [staged], unstaged: [unstaged] }), - resolveSourceControlGroupOrder('staged-first') + SOURCE_CONTROL_GROUP_ORDER ) expect(getSourceControlSectionViewAction(sections[0]!)).toEqual({ diff --git a/src/renderer/src/components/right-sidebar/source-control-section-order.ts b/src/renderer/src/components/right-sidebar/source-control-section-order.ts index 557db90b556..91cc948855b 100644 --- a/src/renderer/src/components/right-sidebar/source-control-section-order.ts +++ b/src/renderer/src/components/right-sidebar/source-control-section-order.ts @@ -1,5 +1,4 @@ -import { normalizeSourceControlGroupOrder } from '../../../../shared/source-control-group-order' -import type { GitStatusEntry, SourceControlGroupOrder } from '../../../../shared/types' +import type { GitStatusEntry } from '../../../../shared/types' export const SOURCE_CONTROL_AREAS = ['unstaged', 'staged', 'untracked'] as const export type SourceControlSectionArea = (typeof SOURCE_CONTROL_AREAS)[number] @@ -22,16 +21,23 @@ export type SourceControlSectionViewAction = | { kind: 'conflict-review'; entries: SourceControlConflictReviewEntry[] } | { kind: 'combined-diff'; area?: SourceControlSectionArea; entries: GitStatusEntry[] } -const ORDER_BY_PRESET: Record = { - 'changes-first': ['unstaged', 'staged', 'untracked'], - 'staged-first': ['staged', 'unstaged', 'untracked'], - 'untracked-first': ['untracked', 'unstaged', 'staged'] -} +export const SOURCE_CONTROL_GROUP_ORDER: readonly SourceControlSectionArea[] = [ + 'unstaged', + 'staged', + 'untracked' +] -export function resolveSourceControlGroupOrder( - value: SourceControlGroupOrder | null | undefined -): readonly SourceControlSectionArea[] { - return ORDER_BY_PRESET[normalizeSourceControlGroupOrder(value)] +export function mergeUntrackedIntoChanges( + groups: SourceControlEntryGroups +): SourceControlEntryGroups { + if (groups.untracked.length === 0) { + return groups + } + return { + staged: groups.staged, + unstaged: [...groups.unstaged, ...groups.untracked], + untracked: [] + } } export function isPinnedConflictEntry(entry: GitStatusEntry): boolean { diff --git a/src/renderer/src/components/settings/GitPane.test.ts b/src/renderer/src/components/settings/GitPane.test.ts index 3275eb9a183..2947f866fdd 100644 --- a/src/renderer/src/components/settings/GitPane.test.ts +++ b/src/renderer/src/components/settings/GitPane.test.ts @@ -6,12 +6,7 @@ import { getDefaultSettings } from '../../../../shared/constants' import { translate } from '../../i18n/i18n' import { useAppStore } from '../../store' import { shouldOpenAutoRenameBranchAdvanced } from './AutoRenameBranchFromWorkSetting' -import { - GitPane, - SourceControlGroupOrderSetting, - getGitPaneSearchEntries, - shouldShowAutoRenameBranchSetting -} from './GitPane' +import { GitPane, getGitPaneSearchEntries, shouldShowAutoRenameBranchSetting } from './GitPane' import { TooltipProvider } from '../ui/tooltip' import { matchesSettingsSearch } from './settings-search' import { SettingsSegmentedControl } from './SettingsFormControls' @@ -40,23 +35,6 @@ function visit(node: unknown, cb: (node: ReactElementLike) => void): void { } } -function findSegmentedControl(node: unknown): ReactElementLike { - let found: ReactElementLike | null = null - const label = translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderTitle', - 'Source Control Group Order' - ) - visit(node, (entry) => { - if (entry.type === SettingsSegmentedControl && entry.props.ariaLabel === label) { - found = entry - } - }) - if (!found) { - throw new Error('segmented control not found') - } - return found -} - function findCompareBaseSegmentedControl(node: unknown): ReactElementLike { let found: ReactElementLike | null = null const label = translate( @@ -136,52 +114,6 @@ describe('GitPane', () => { expect(markup).not.toContain('Refresh Local Base Ref') }) - it('renders Source Control group order in Git settings', () => { - const markup = renderGitPane('group order') - - expect(markup).toContain( - translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderTitle', - 'Source Control Group Order' - ) - ) - expect(markup).toContain( - translate('auto.components.settings.GitPane.changesFirst', 'Changes first') - ) - expect(markup).toContain( - translate('auto.components.settings.GitPane.stagedFirst', 'Staged first') - ) - expect(markup).toContain( - translate('auto.components.settings.GitPane.untrackedFirst', 'Untracked first') - ) - }) - - it('updates Source Control group order only when the selected option changes', () => { - const updateSettings = vi.fn() - const element = SourceControlGroupOrderSetting({ - settings: { - ...getDefaultSettings(os.homedir()), - sourceControlGroupOrder: 'changes-first' - }, - updateSettings - }) - - const control = findSegmentedControl(element) - const onChange = control.props.onChange as (value: string) => void - - onChange('staged-first') - expect(updateSettings).toHaveBeenCalledWith({ sourceControlGroupOrder: 'staged-first' }) - - updateSettings.mockClear() - onChange('changes-first') - expect(updateSettings).not.toHaveBeenCalled() - }) - - it('includes Source Control group order search metadata', () => { - expect(matchesSettingsSearch('staged', getGitPaneSearchEntries())).toBe(true) - expect(matchesSettingsSearch('group order', getGitPaneSearchEntries())).toBe(true) - }) - it('renders the default compare base setting in Git settings', () => { const markup = renderGitPane('compare base') diff --git a/src/renderer/src/components/settings/GitPane.tsx b/src/renderer/src/components/settings/GitPane.tsx index da1a34c3fe8..538315e3e7d 100644 --- a/src/renderer/src/components/settings/GitPane.tsx +++ b/src/renderer/src/components/settings/GitPane.tsx @@ -1,7 +1,6 @@ import { useEffect, useRef, useState } from 'react' -import type { GlobalSettings, SourceControlGroupOrder } from '../../../../shared/types' +import type { GlobalSettings } from '../../../../shared/types' import type { SourceControlAiSettingsPatch } from '../../../../shared/source-control-ai-types' -import { DEFAULT_SOURCE_CONTROL_GROUP_ORDER } from '../../../../shared/source-control-group-order' import { Input } from '../ui/input' import { Label } from '../ui/label' import { useAppStore } from '../../store' @@ -20,7 +19,6 @@ import { getKeepLocalMainUpToDateTitle } from './keep-local-main-up-to-date-setting' import { translate } from '@/i18n/i18n' -import { SettingsRow, SettingsSegmentedControl } from './SettingsFormControls' export { getGitPaneSearchEntries } @@ -40,14 +38,6 @@ const KEEP_LOCAL_MAIN_UP_TO_DATE_KEYWORDS = [ 'safely', 'worktree' ] -const SOURCE_CONTROL_GROUP_ORDER_KEYWORDS = [ - 'group order', - 'changes first', - 'staged first', - 'untracked first', - 'source control', - 'git changes' -] export function shouldShowAutoRenameBranchSetting( searchQuery: string, @@ -70,68 +60,6 @@ type GitPaneProps = { settingsSearchQuery?: string } -export function SourceControlGroupOrderSetting({ - settings, - updateSettings -}: { - settings: GlobalSettings - updateSettings: (updates: Partial) => void | Promise -}): React.JSX.Element { - const value = settings.sourceControlGroupOrder ?? DEFAULT_SOURCE_CONTROL_GROUP_ORDER - const title = translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderTitle', - 'Source Control Group Order' - ) - const description = translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderDescription', - 'Choose whether Changes, Staged Changes, or Untracked Files appear first in Source Control.' - ) - - return ( - - - value={value} - onChange={(nextValue) => { - if (nextValue !== value) { - void updateSettings({ sourceControlGroupOrder: nextValue }) - } - }} - ariaLabel={title} - size="sm" - options={[ - { - value: 'changes-first', - label: translate('auto.components.settings.GitPane.changesFirst', 'Changes first') - }, - { - value: 'staged-first', - label: translate('auto.components.settings.GitPane.stagedFirst', 'Staged first') - }, - { - value: 'untracked-first', - label: translate( - 'auto.components.settings.GitPane.untrackedFirst', - 'Untracked first' - ) - } - ]} - /> - } - /> - - ) -} - export function GitPane({ settings, updateSettings, @@ -297,23 +225,6 @@ export function GitPane({ ) : null, - matchesSettingsSearch(searchQuery, { - title: translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderTitle', - 'Source Control Group Order' - ), - description: translate( - 'auto.components.settings.GitPane.sourceControlGroupOrderDescription', - 'Choose whether Changes, Staged Changes, or Untracked Files appear first in Source Control.' - ), - keywords: SOURCE_CONTROL_GROUP_ORDER_KEYWORDS - }) ? ( - - ) : null, compareAgainstUpstreamMatchesSearch(searchQuery) ? ( [ ...translateSearchKeyword('auto.components.settings.git.search.035134fcd9', 'worktree') ] }, - { - title: translate( - 'auto.components.settings.git.search.sourceControlGroupOrderTitle', - 'Source Control Group Order' - ), - description: translate( - 'auto.components.settings.git.search.sourceControlGroupOrderDescription', - 'Choose whether Changes, Staged Changes, or Untracked Files appear first in Source Control.' - ), - keywords: [ - ...translateSearchKeyword('auto.components.settings.git.search.groupOrder', 'group order'), - ...translateSearchKeyword( - 'auto.components.settings.git.search.changesFirst', - 'changes first' - ), - ...translateSearchKeyword('auto.components.settings.git.search.stagedFirst', 'staged first'), - ...translateSearchKeyword( - 'auto.components.settings.git.search.untrackedFirst', - 'untracked first' - ), - ...translateSearchKeyword( - 'auto.components.settings.git.search.sourceControl', - 'source control' - ), - ...translateSearchKeyword('auto.components.settings.git.search.gitChanges', 'git changes') - ] - }, { title: translate( 'auto.components.settings.git.search.compareAgainstUpstreamTitle', diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 29218566eb8..3f940076736 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -6183,11 +6183,6 @@ "3d172725cc": "None", "1f32ba27a6": "Custom", "a182c5125e": "Git Username", - "sourceControlGroupOrderTitle": "Source Control Group Order", - "sourceControlGroupOrderDescription": "Choose whether Changes, Staged Changes, or Untracked Files appear first in Source Control.", - "changesFirst": "Changes first", - "stagedFirst": "Staged first", - "untrackedFirst": "Untracked first", "compareAgainstUpstreamTitle": "Default Compare Base", "compareAgainstUpstreamDescription": "Choose which base Source Control uses by default for committed-change comparisons. Branch upstream follows the current branch automatically and falls back to the repository default branch when no upstream exists. You can still change the compare base per worktree from that worktree's Git panel. Pull Request and rebase targets don't change.", "compareBaseRepositoryDefault": "Repository default", @@ -8463,14 +8458,7 @@ "f83c8937c4": "branch naming", "5ecd91c5ef": "Prefix added to branch names when creating worktrees.", "68bd65fdb8": "Branch Prefix", - "sourceControlGroupOrderTitle": "Source Control Group Order", - "sourceControlGroupOrderDescription": "Choose whether Changes, Staged Changes, or Untracked Files appear first in Source Control.", - "groupOrder": "group order", - "changesFirst": "changes first", - "stagedFirst": "staged first", - "untrackedFirst": "untracked first", "sourceControl": "source control", - "gitChanges": "git changes", "compareAgainstUpstreamTitle": "Default Compare Base", "compareAgainstUpstreamDescription": "Choose which base Source Control uses by default for committed-change comparisons. Branch upstream follows the current branch automatically and falls back to the repository default branch when no upstream exists. You can still change the compare base per worktree from that worktree's Git panel. Pull Request and rebase targets don't change.", "compareBase": "compare base", @@ -10805,6 +10793,8 @@ "a4e93c21d7": "Current branch: {{value0}}", "c7d4e2f801": "Change base ref: {{value0}}", "f3a1b8c204": "upstream", + "discardAllMixedUntracked": "Discard changes and delete untracked files", + "discardFolderMixedUntracked": "Discard changes and delete untracked files in folder", "createPrIntentEmptyGeneratedBody": "Generated review details did not include a description. Retry Create PR.", "createPrIntentGenerateDetailsFailed": "Could not generate review details. Retry Create PR." }, @@ -11230,7 +11220,10 @@ "40e9357b2a": "This will restore the file from HEAD and discard the deletion. This cannot be undone.", "5c0bdbc4cb": "Restore \"{{value0}}\"?", "d97bf697c9": "This will permanently delete this file. This cannot be undone.", - "96c772bee9": "Delete \"{{value0}}\"?" + "96c772bee9": "Delete \"{{value0}}\"?", + "mixedTitle": "Discard changes and delete untracked files?", + "mixedDescription": "Tracked changes will be reverted and untracked files will be permanently deleted. This cannot be undone.", + "mixedConfirm": "Delete and discard" }, "dialog": { "3bc61dc989": "Cancel", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index e32c1ca2318..33b14ec6fa7 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -5966,11 +5966,6 @@ "3d172725cc": "Ninguno", "1f32ba27a6": "Personalizado", "a182c5125e": "Usuario de Git", - "sourceControlGroupOrderTitle": "Orden de grupos de control de código fuente", - "sourceControlGroupOrderDescription": "Elige si Cambios, Cambios preparados o Archivos sin seguimiento aparecen primero en Control de código fuente.", - "changesFirst": "Cambios primero", - "stagedFirst": "Cambios preparados primero", - "untrackedFirst": "Archivos sin seguimiento primero", "compareAgainstUpstreamTitle": "Base de comparación predeterminada", "compareAgainstUpstreamDescription": "Elige qué base usa Control de código fuente de forma predeterminada para las comparaciones de cambios confirmados. El upstream de la rama sigue automáticamente la rama actual y recurre a la rama predeterminada del repositorio cuando no hay upstream. Aun así, puedes cambiar la base de comparación por worktree desde el panel Git de ese worktree. Los objetivos de Pull Request y rebase no cambian.", "compareBaseRepositoryDefault": "Predeterminada del repositorio", @@ -8236,14 +8231,7 @@ "f83c8937c4": "nombres de ramas", "5ecd91c5ef": "Prefijo agregado a los nombres de ramas al crear worktrees.", "68bd65fdb8": "Prefijo de rama", - "sourceControlGroupOrderTitle": "Orden de grupos de control de código fuente", - "sourceControlGroupOrderDescription": "Elige si Cambios, Cambios preparados o Archivos sin seguimiento aparecen primero en Control de código fuente.", - "groupOrder": "orden de grupos", - "changesFirst": "cambios primero", - "stagedFirst": "cambios preparados primero", - "untrackedFirst": "archivos sin seguimiento primero", "sourceControl": "control de código fuente", - "gitChanges": "cambios de git", "compareAgainstUpstreamTitle": "Base de comparación predeterminada", "compareAgainstUpstreamDescription": "Elige qué base usa Control de código fuente de forma predeterminada para las comparaciones de cambios confirmados. El upstream de la rama sigue automáticamente la rama actual y recurre a la rama predeterminada del repositorio cuando no hay upstream. Aun así, puedes cambiar la base de comparación por worktree desde el panel Git de ese worktree. Los objetivos de Pull Request y rebase no cambian.", "compareBase": "base de comparación", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 16a92c12c02..24c94d5c823 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -5988,11 +5988,6 @@ "3d172725cc": "なし", "1f32ba27a6": "カスタム", "a182c5125e": "Gitのユーザー名", - "sourceControlGroupOrderTitle": "ソース管理のグループ順序", - "sourceControlGroupOrderDescription": "ソース管理で「変更」「ステージ済みの変更」「未追跡ファイル」のどれを先に表示するかを選択します。", - "changesFirst": "変更を先頭", - "stagedFirst": "ステージ済みを先頭", - "untrackedFirst": "未追跡を先頭", "compareAgainstUpstreamTitle": "既定の比較ベース", "compareAgainstUpstreamDescription": "ソース管理がコミット済み変更の比較に既定で使うベースを選択します。ブランチの上流は現在のブランチに自動的に追従し、上流がない場合はリポジトリの既定ブランチにフォールバックします。比較ベースは各 worktree の Git パネルから worktree ごとに変更できます。Pull Request やリベースの対象は変更されません。", "compareBaseRepositoryDefault": "リポジトリ既定", @@ -8258,14 +8253,7 @@ "f83c8937c4": "ブランチの命名", "5ecd91c5ef": "ワークツリーの作成時にブランチ名に追加されるプレフィックス。", "68bd65fdb8": "ブランチプレフィックス", - "sourceControlGroupOrderTitle": "ソース管理のグループ順序", - "sourceControlGroupOrderDescription": "ソース管理で「変更」「ステージ済みの変更」「未追跡ファイル」のどれを先に表示するかを選択します。", - "groupOrder": "グループ順序", - "changesFirst": "変更を先頭", - "stagedFirst": "ステージ済みを先頭", - "untrackedFirst": "未追跡を先頭", "sourceControl": "ソース管理", - "gitChanges": "git の変更", "compareAgainstUpstreamTitle": "既定の比較ベース", "compareAgainstUpstreamDescription": "ソース管理がコミット済み変更の比較に既定で使うベースを選択します。ブランチの上流は現在のブランチに自動的に追従し、上流がない場合はリポジトリの既定ブランチにフォールバックします。比較ベースは各 worktree の Git パネルから worktree ごとに変更できます。Pull Request やリベースの対象は変更されません。", "compareBase": "比較基準", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 0489f18507a..ead7ea624f4 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -5951,11 +5951,6 @@ "3d172725cc": "없음", "1f32ba27a6": "사용자 정의", "a182c5125e": "Git 사용자 이름", - "sourceControlGroupOrderTitle": "소스 제어 그룹 순서", - "sourceControlGroupOrderDescription": "소스 제어에서 변경 사항, 스테이징된 변경 사항 또는 추적되지 않는 파일 중 무엇을 먼저 표시할지 선택합니다.", - "changesFirst": "변경 사항 먼저", - "stagedFirst": "스테이징된 항목 먼저", - "untrackedFirst": "추적되지 않는 파일 먼저", "compareAgainstUpstreamTitle": "기본 비교 기준", "compareAgainstUpstreamDescription": "소스 제어가 커밋된 변경 사항 비교에 기본으로 사용할 기준을 선택합니다. 브랜치 업스트림은 현재 브랜치를 자동으로 따라가며, 업스트림이 없으면 저장소 기본 브랜치로 돌아갑니다. 비교 기준은 해당 worktree의 Git 패널에서 worktree별로 변경할 수 있습니다. Pull Request 및 리베이스 대상은 변경되지 않습니다.", "compareBaseRepositoryDefault": "저장소 기본값", @@ -8221,14 +8216,7 @@ "f83c8937c4": "브랜치 이름 지정", "5ecd91c5ef": "작업 트리를 생성할 때 브랜치 이름에 접두사가 추가됩니다.", "68bd65fdb8": "브랜치 접두사", - "sourceControlGroupOrderTitle": "소스 제어 그룹 순서", - "sourceControlGroupOrderDescription": "소스 제어에서 변경 사항, 스테이징된 변경 사항 또는 추적되지 않는 파일 중 무엇을 먼저 표시할지 선택합니다.", - "groupOrder": "그룹 순서", - "changesFirst": "변경 사항 먼저", - "stagedFirst": "스테이징된 항목 먼저", - "untrackedFirst": "추적되지 않는 파일 먼저", "sourceControl": "소스 제어", - "gitChanges": "git 변경 사항", "compareAgainstUpstreamTitle": "기본 비교 기준", "compareAgainstUpstreamDescription": "소스 제어가 커밋된 변경 사항 비교에 기본으로 사용할 기준을 선택합니다. 브랜치 업스트림은 현재 브랜치를 자동으로 따라가며, 업스트림이 없으면 저장소 기본 브랜치로 돌아갑니다. 비교 기준은 해당 worktree의 Git 패널에서 worktree별로 변경할 수 있습니다. Pull Request 및 리베이스 대상은 변경되지 않습니다.", "compareBase": "비교 기준", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 1b197b3eaac..b64faff2472 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -5963,11 +5963,6 @@ "3d172725cc": "没有任何", "1f32ba27a6": "自定义", "a182c5125e": "git用户名", - "sourceControlGroupOrderTitle": "源代码管理分组顺序", - "sourceControlGroupOrderDescription": "选择在源代码管理中优先显示“更改”、“已暂存更改”还是“未跟踪文件”。", - "changesFirst": "更改优先", - "stagedFirst": "已暂存优先", - "untrackedFirst": "未跟踪优先", "compareAgainstUpstreamTitle": "默认对比基准", "compareAgainstUpstreamDescription": "选择源代码管理在比较已提交变更时默认使用的基准。分支上游会自动跟随当前分支；如果没有上游，则回退到仓库默认分支。你仍然可以在对应 worktree 的 Git 面板中按 worktree 更改对比基准。Pull Request 和变基目标不会改变。", "compareBaseRepositoryDefault": "仓库默认", @@ -8233,14 +8228,7 @@ "f83c8937c4": "分支命名", "5ecd91c5ef": "创建工作树时添加到分支名称的前缀。", "68bd65fdb8": "分支前缀", - "sourceControlGroupOrderTitle": "源代码管理分组顺序", - "sourceControlGroupOrderDescription": "选择在源代码管理中优先显示“更改”、“已暂存更改”还是“未跟踪文件”。", - "groupOrder": "分组顺序", - "changesFirst": "更改优先", - "stagedFirst": "已暂存优先", - "untrackedFirst": "未跟踪优先", "sourceControl": "源代码管理", - "gitChanges": "git 更改", "compareAgainstUpstreamTitle": "默认对比基准", "compareAgainstUpstreamDescription": "选择源代码管理在比较已提交变更时默认使用的基准。分支上游会自动跟随当前分支；如果没有上游，则回退到仓库默认分支。你仍然可以在对应 worktree 的 Git 面板中按 worktree 更改对比基准。Pull Request 和变基目标不会改变。", "compareBase": "对比基准", diff --git a/src/shared/constants.test.ts b/src/shared/constants.test.ts index e8c55be052d..85780660b2a 100644 --- a/src/shared/constants.test.ts +++ b/src/shared/constants.test.ts @@ -23,9 +23,7 @@ describe('getDefaultSettings', () => { expect(getDefaultSettings('/tmp').sourceControlViewMode).toBe('list') }) - it('keeps Source Control changes first by default', () => { - expect(getDefaultSettings('/tmp').sourceControlGroupOrder).toBe('changes-first') - }) + it('uses a commit-oriented Source Control layout by default', () => {}) it('defaults mobile pairing to discovered network addresses', () => { expect(getDefaultSettings('/tmp').mobilePairingCustomAddress).toBeNull() diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 80fca73c828..33b927dccf1 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -29,7 +29,6 @@ import { DEFAULT_LEFT_SIDEBAR_TINT_COLOR, DEFAULT_LEFT_SIDEBAR_TINT_OPACITY } from './left-sidebar-appearance' -import { DEFAULT_SOURCE_CONTROL_GROUP_ORDER } from './source-control-group-order' import { DEFAULT_SETUP_AGENT_STARTUP_POLICY } from './setup-agent-startup-policy' import { DESKTOP_TERMINAL_SCROLLBACK_ROWS_DEFAULT } from './terminal-scrollback-policy' import { DEFAULT_USAGE_PERCENTAGE_DISPLAY } from './usage-percentage-display' @@ -272,7 +271,6 @@ export function getDefaultSettings(homedir: string): GlobalSettings { rightSidebarOpenByDefault: true, showGitIgnoredFiles: true, sourceControlViewMode: 'list', - sourceControlGroupOrder: DEFAULT_SOURCE_CONTROL_GROUP_ORDER, sourceControlCompareAgainstUpstream: false, showTitlebarAppName: true, showTasksButton: true, diff --git a/src/shared/source-control-group-order.test.ts b/src/shared/source-control-group-order.test.ts deleted file mode 100644 index 165d2359c3b..00000000000 --- a/src/shared/source-control-group-order.test.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { - DEFAULT_SOURCE_CONTROL_GROUP_ORDER, - normalizeSourceControlGroupOrder -} from './source-control-group-order' - -describe('normalizeSourceControlGroupOrder', () => { - it('keeps supported source control group orders', () => { - expect(normalizeSourceControlGroupOrder('changes-first')).toBe('changes-first') - expect(normalizeSourceControlGroupOrder('staged-first')).toBe('staged-first') - expect(normalizeSourceControlGroupOrder('untracked-first')).toBe('untracked-first') - }) - - it('falls back to the default for malformed values', () => { - expect(normalizeSourceControlGroupOrder('tracked-first')).toBe( - DEFAULT_SOURCE_CONTROL_GROUP_ORDER - ) - expect(normalizeSourceControlGroupOrder(undefined)).toBe(DEFAULT_SOURCE_CONTROL_GROUP_ORDER) - }) -}) diff --git a/src/shared/source-control-group-order.ts b/src/shared/source-control-group-order.ts deleted file mode 100644 index acfe5160ac7..00000000000 --- a/src/shared/source-control-group-order.ts +++ /dev/null @@ -1,9 +0,0 @@ -import type { SourceControlGroupOrder } from './types' - -export const DEFAULT_SOURCE_CONTROL_GROUP_ORDER: SourceControlGroupOrder = 'changes-first' - -export function normalizeSourceControlGroupOrder(value: unknown): SourceControlGroupOrder { - return value === 'changes-first' || value === 'staged-first' || value === 'untracked-first' - ? value - : DEFAULT_SOURCE_CONTROL_GROUP_ORDER -} diff --git a/src/shared/types.ts b/src/shared/types.ts index 947786da5bc..a3864c39061 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2699,7 +2699,6 @@ export type OpenInApplication = { } export type SourceControlViewMode = 'list' | 'tree' -export type SourceControlGroupOrder = 'changes-first' | 'staged-first' | 'untracked-first' export type LeftSidebarAppearanceMode = 'default' | 'match-terminal' | 'tinted' @@ -2885,8 +2884,6 @@ export type GlobalSettings = { showGitIgnoredFiles?: boolean /** Preferred Source Control changes layout. Per-user, not per-workspace. */ sourceControlViewMode: SourceControlViewMode - /** Preferred Source Control group order. Per-user, not per-workspace. */ - sourceControlGroupOrder: SourceControlGroupOrder /** Compare base defaults to the branch upstream instead of the repo default; affects only the compare/diff view, not the PR/rebase target. Per-user. */ sourceControlCompareAgainstUpstream: boolean /** Whether to show the Orca app name in the titlebar. */ diff --git a/tests/e2e/setup-script-prompt-unreadable-orca-yaml.spec.ts b/tests/e2e/setup-script-prompt-unreadable-orca-yaml.spec.ts index 5cc07ef0e5b..151a4b02bbc 100644 --- a/tests/e2e/setup-script-prompt-unreadable-orca-yaml.spec.ts +++ b/tests/e2e/setup-script-prompt-unreadable-orca-yaml.spec.ts @@ -65,8 +65,9 @@ async function installUnreadableOrcaYamlFault(electronApp: ElectronApplication): /** orca.yaml becomes readable again — every later check runs the production handler. */ async function healOrcaYamlRead(electronApp: ElectronApplication): Promise { await electronApp.evaluate(() => { - ;(globalThis as typeof globalThis & { __orcaE2eOrcaYamlUnreadable?: boolean }) - .__orcaE2eOrcaYamlUnreadable = false + ;( + globalThis as typeof globalThis & { __orcaE2eOrcaYamlUnreadable?: boolean } + ).__orcaE2eOrcaYamlUnreadable = false }) } From 2ff2a1b26870c51ed59d2aa712dc96d6aa11ab27 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 5 Aug 2026 16:52:49 -0700 Subject: [PATCH 08/18] Display SSH worktrees immediately using persisted metadata (#12646) * Display SSH worktrees immediately using persisted metadata Users can now see known worktrees for SSH hosts without waiting for the provider connection to establish. Worktrees are fetched from local metadata and displayed as non-authoritative, then merged without replacing richer live data once the provider becomes available. * Show SSH folder workspaces immediately via persisted metadata Add safeguards for metadata fallback: track authoritatively removed worktrees per host to prevent resurrection, position new rows within the host block to avoid jumping on authoritative scan arrival, and preserve co-owner detection status during merge. Coalesce concurrent metadata fetches to dedupe overlapping queries. --- src/main/ipc/worktrees.test.ts | 135 +++++- src/main/ipc/worktrees.ts | 84 +++- src/preload/api-types.ts | 5 + src/preload/index.ts | 3 + src/preload/ssh-authority-forwarding.test.ts | 10 + .../slices/degraded-repo-hydration.test.ts | 35 +- .../slices/degraded-repo-worktree-validity.ts | 12 +- .../src/store/slices/worktrees.test.ts | 425 ++++++++++++++++++ src/renderer/src/store/slices/worktrees.ts | 223 +++++++++ .../detected-worktree-provider-contract.ts | 18 + 10 files changed, 944 insertions(+), 6 deletions(-) diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index c83eece63b6..f5925e1acc1 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -6,7 +6,7 @@ import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' import type { CreateWorktreeResult, GitWorktreeInfo, Repo, Worktree } from '../../shared/types' import type { ProviderRequestId } from '../../shared/detected-worktree-provider-contract' -import { toSshExecutionHostId } from '../../shared/execution-host' +import { LOCAL_EXECUTION_HOST_ID, toSshExecutionHostId } from '../../shared/execution-host' import * as localWorktreeFilesystem from '../local-worktree-filesystem' const ORIGINAL_PLATFORM = process.platform @@ -2589,6 +2589,139 @@ describe('registerWorktreeHandlers', () => { }) }) + it('lists every persisted SSH worktree without accessing the live provider', async () => { + const sshHostId = toSshExecutionHostId('target-a') + const sshRepo = { + id: 'repo-1', + path: '/remote/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'target-a' + } + const metaById = { + 'repo-1::/remote/repo': makeWorktreeMeta({ + displayName: 'main', + hostId: sshHostId + }), + 'repo-1::/remote/queued': makeWorktreeMeta({ + displayName: 'queued', + hostId: sshHostId + }), + 'repo-1::/remote/other-host': makeWorktreeMeta({ + displayName: 'other host', + hostId: toSshExecutionHostId('target-b') + }) + } + store.getRepos.mockReturnValue([sshRepo]) + store.getProjectHostSetups.mockReturnValue([]) + store.getAllWorktreeMeta.mockReturnValue(metaById) + store.getWorktreeMeta.mockImplementation((worktreeId: string) => metaById[worktreeId]) + + const result = await handlers['worktrees:listKnownForExecutionHost'](null, { + repoId: sshRepo.id, + executionHostId: sshHostId + }) + + expect(result).toMatchObject({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: sshHostId, + result: { + repoId: sshRepo.id, + authoritative: false, + source: 'metadata-fallback', + worktrees: [ + expect.objectContaining({ path: '/remote/repo', isMainWorktree: true }), + expect.objectContaining({ path: '/remote/queued', isMainWorktree: false }) + ] + } + }) + expect(getSshGitProviderMock).not.toHaveBeenCalled() + expect(listWorktreesMock).not.toHaveBeenCalled() + }) + + it('lists SSH folder workspaces at the folder path, not the instance-suffixed id', async () => { + const sshHostId = toSshExecutionHostId('target-a') + const folderRepo = { + id: 'repo-1', + path: '/remote/folder', + displayName: 'folder', + badgeColor: '#000', + addedAt: 0, + connectionId: 'target-a', + kind: 'folder' as const + } + const rootId = `${folderRepo.id}::${folderRepo.path}` + const instanceId = `${rootId}::workspace:11111111-2222-3333-4444-555555555555` + const metaById = { + [rootId]: makeWorktreeMeta({ displayName: 'root', hostId: sshHostId }), + [instanceId]: makeWorktreeMeta({ + displayName: 'second workspace', + hostId: sshHostId, + instanceId: '11111111-2222-3333-4444-555555555555' + }) + } + store.getRepos.mockReturnValue([folderRepo]) + store.getProjectHostSetups.mockReturnValue([]) + store.getAllWorktreeMeta.mockReturnValue(metaById) + store.getWorktreeMeta.mockImplementation((worktreeId: string) => metaById[worktreeId]) + + const result = (await handlers['worktrees:listKnownForExecutionHost'](null, { + repoId: folderRepo.id, + executionHostId: sshHostId + })) as { status: string; result: { authoritative: boolean; worktrees: Worktree[] } } + + expect(result.status).toBe('complete') + expect(result.result.authoritative).toBe(false) + // Why: the git-worktree synthesizer would read the "::workspace:" tail as a directory. + expect(result.result.worktrees).toEqual([ + expect.objectContaining({ id: rootId, path: folderRepo.path, isMainWorktree: true }), + expect.objectContaining({ id: instanceId, path: folderRepo.path, isMainWorktree: false }) + ]) + expect(getSshGitProviderMock).not.toHaveBeenCalled() + }) + + it('rejects metadata-only reads for non-SSH execution hosts', async () => { + const result = await handlers['worktrees:listKnownForExecutionHost'](null, { + repoId: 'repo-1', + executionHostId: LOCAL_EXECUTION_HOST_ID + }) + + expect(result).toEqual({ + status: 'rejected', + repoId: 'repo-1', + executionHostId: LOCAL_EXECUTION_HOST_ID + }) + expect(store.getRepos).not.toHaveBeenCalled() + }) + + it('rejects ambiguous metadata-only SSH owners', async () => { + const sshHostId = toSshExecutionHostId('target-a') + const sshRepo = { + id: 'repo-1', + path: '/remote/repo-a', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'target-a' + } + store.getRepos.mockReturnValue([sshRepo, { ...sshRepo, path: '/remote/repo-b' }]) + + const result = await handlers['worktrees:listKnownForExecutionHost'](null, { + repoId: sshRepo.id, + executionHostId: sshHostId + }) + + expect(result).toEqual({ + status: 'rejected', + repoId: sshRepo.id, + executionHostId: sshHostId + }) + expect(store.getAllWorktreeMeta).not.toHaveBeenCalled() + expect(getSshGitProviderMock).not.toHaveBeenCalled() + }) + it('fails closed for duplicate exact owners and ambiguous legacy repo IDs', async () => { const sshRepo = { id: 'shared-repo', diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index c8dceec6bed..098e1e8191f 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -49,7 +49,9 @@ import { import { PROVIDER_REQUEST_ID_MAX_UTF8_BYTES, type DirectSshDetectedWorktreeRequest, + type HostQualifiedKnownWorktreeResult, type HostQualifiedDetectedWorktreeResult, + type ListKnownWorktreesForExecutionHostArgs, type ListDetectedWorktreesArgs, type ProviderRequestId } from '../../shared/detected-worktree-provider-contract' @@ -229,7 +231,8 @@ import { import { DEFAULT_WORKSPACE_STATUS_ID } from '../../shared/workspace-statuses' import { FOLDER_WORKSPACE_INSTANCE_SEPARATOR, - getRepoIdFromWorktreeId + getRepoIdFromWorktreeId, + getWorktreePathBasenameFromId } from '../../shared/worktree-id' import { prefetchWorktreeCreateBase } from '../worktree-create-base-prefetch' import { @@ -804,6 +807,17 @@ function createSshWorktreeMetaIndex(entries: [string, WorktreeMeta][]): SshWorkt return index } +// Why: scopes parseWorktreeId to one repo's keys. The entry list itself is still materialized for the whole +// store, so this is cheaper per call than the unfiltered index, not free. +function createSshWorktreeMetaIndexForRepo( + allMeta: Record, + repoId: string +): SshWorktreeMetaIndex { + return createSshWorktreeMetaIndex( + Object.entries(allMeta).filter(([worktreeId]) => getRepoIdFromWorktreeId(worktreeId) === repoId) + ) +} + function synthesizeSshGitWorktree(repo: Repo, path: string, meta: WorktreeMeta): GitWorktreeInfo { return { path, @@ -842,10 +856,15 @@ function listDisconnectedSshWorktrees( if (Object.keys(ownershipUpdates).length > 0) { store.setWorktreeMeta(candidate.id, ownershipUpdates) } + // Why: synthesized rows carry no branch, so the title would fall through to the DESKTOP's basename() + // applied to a REMOTE path — a Windows remote then renders its whole C:\... path as the name. Rows must + // stay per-directory (repo.displayName would title every row identically), so use the separator-agnostic + // basename instead. const worktree = mergeWorktree( repo.id, synthesizeSshGitWorktree(repo, candidate.path, meta), - meta + meta, + getWorktreePathBasenameFromId(candidate.id) ?? undefined ) byWorktreeId.delete(worktree.id) byWorktreeId.set(worktree.id, worktree) @@ -1826,6 +1845,7 @@ export function registerWorktreeHandlers( ipcMain.removeHandler('worktrees:listAll') ipcMain.removeHandler('worktrees:list') ipcMain.removeHandler('worktrees:listDetected') + ipcMain.removeHandler('worktrees:listKnownForExecutionHost') ipcMain.removeHandler('worktrees:cancelListDetected') ipcMain.removeHandler('worktrees:create') ipcMain.removeHandler('worktrees:prefetchCreateBase') @@ -1970,6 +1990,66 @@ export function registerWorktreeHandlers( } }) + ipcMain.handle( + 'worktrees:listKnownForExecutionHost', + (_event, args: ListKnownWorktreesForExecutionHostArgs): HostQualifiedKnownWorktreeResult => { + // Why: a malformed invoke must fail closed as `rejected`, not throw out of the handler. `ssh:` is inert — + // it owns no repo, so every guard below still rejects it. + const requestedRepoId = args?.repoId ?? '' + const requestedExecutionHostId = args?.executionHostId ?? 'ssh:' + const rejected = (): HostQualifiedKnownWorktreeResult => ({ + status: 'rejected', + repoId: requestedRepoId, + executionHostId: requestedExecutionHostId + }) + const parsedHost = parseExecutionHostId(requestedExecutionHostId) + if (parsedHost?.kind !== 'ssh') { + return rejected() + } + // Why: findExactRepoOwner repeats this same all-candidates-owned check, and getRepos() re-hydrates the + // whole catalog, so a separate pass here is pure cost. + const repo = findExactRepoOwner(store, requestedRepoId, requestedExecutionHostId) + if (!repo || repo.connectionId !== parsedHost.targetId) { + return rejected() + } + const complete = (worktrees: DetectedWorktree[]): HostQualifiedKnownWorktreeResult => ({ + status: 'complete', + repoId: repo.id, + executionHostId: requestedExecutionHostId, + result: { + repoId: repo.id, + authoritative: false, + source: 'metadata-fallback', + worktrees + } + }) + // Why: folder workspace ids carry an instance suffix the git-worktree synthesizer would read as a directory; build them the way every other listing does. + if (isFolderRepo(repo)) { + const folderWorkspaceIds = Object.keys(store.getAllWorktreeMeta()).filter((worktreeId) => + isFolderWorkspaceIdForRepo(repo, worktreeId) + ) + return hasConflictingStoredWorktreeOwner(store, repo, folderWorkspaceIds) + ? rejected() + : complete( + // Why: match the authoritative folder listing; without lineage these rows render flat and then + // reshuffle once the real scan lands. + projectResolvedWorktreeLineage( + buildFolderDetectedWorktrees(store, repo), + store.getAllWorktreeLineage?.() ?? {} + ) + ) + } + const metaIndex = createSshWorktreeMetaIndexForRepo(store.getAllWorktreeMeta(), repo.id) + return complete( + buildDisconnectedDetectedWorktrees( + store, + repo, + listDisconnectedSshWorktrees(store, repo, metaIndex) + ) + ) + } + ) + ipcMain.handle( 'worktrees:listDetected', async ( diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 094a93c3484..53bdb0e7349 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -30,8 +30,10 @@ import type { ReadClipboardTextOptions } from '../shared/clipboard-text' import type { AppIdentity } from '../shared/app-identity' import type { ReleaseChannel } from '../shared/release-channel' import type { + HostQualifiedKnownWorktreeResult, HostQualifiedDetectedWorktreeResult, LegacyDetectedWorktreeRequest, + ListKnownWorktreesForExecutionHostArgs, ListDetectedWorktreesArgs, ProviderRequestId } from '../shared/detected-worktree-provider-contract' @@ -1399,6 +1401,9 @@ export type PreloadApi = { ): Promise (args: LegacyDetectedWorktreeRequest): Promise } + listKnownForExecutionHost?: ( + args: ListKnownWorktreesForExecutionHostArgs + ) => Promise cancelListDetected?: (args: { providerRequestId: ProviderRequestId }) => Promise listAll: () => Promise create: (args: CreateWorktreeArgs) => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 8b3863dcf7c..cf602b3f1cc 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -766,6 +766,9 @@ const api = { listDetected: (args) => ipcRenderer.invoke('worktrees:listDetected', args), + listKnownForExecutionHost: (args) => + ipcRenderer.invoke('worktrees:listKnownForExecutionHost', args), + cancelListDetected: (args) => ipcRenderer.invoke('worktrees:cancelListDetected', args), listAll: () => ipcRenderer.invoke('worktrees:listAll'), diff --git a/src/preload/ssh-authority-forwarding.test.ts b/src/preload/ssh-authority-forwarding.test.ts index 7a81df602d2..04f13430801 100644 --- a/src/preload/ssh-authority-forwarding.test.ts +++ b/src/preload/ssh-authority-forwarding.test.ts @@ -68,6 +68,16 @@ describe('native preload SSH authority forwarding', () => { >() }) + it('forwards host-qualified metadata-only worktree reads', async () => { + await import('./index') + const api = exposeInMainWorld.mock.calls.find(([name]) => name === 'api')?.[1] as PreloadApi + const args = { repoId: 'repo-1', executionHostId: 'ssh:ssh-1' as const } + + await api.worktrees.listKnownForExecutionHost?.(args) + + expect(invoke).toHaveBeenCalledWith('worktrees:listKnownForExecutionHost', args) + }) + it('forwards full-pair get and push states without cloning away authority', async () => { const state: SshConnectionState = { targetId: 'ssh-1', diff --git a/src/renderer/src/store/slices/degraded-repo-hydration.test.ts b/src/renderer/src/store/slices/degraded-repo-hydration.test.ts index 93902bb9d49..48690c43f51 100644 --- a/src/renderer/src/store/slices/degraded-repo-hydration.test.ts +++ b/src/renderer/src/store/slices/degraded-repo-hydration.test.ts @@ -16,7 +16,7 @@ import { buildWorkspaceSessionPayload, shouldPersistWorkspaceSession } from '@/lib/workspace-session' -import { createTestStore, makeTab } from './store-test-helpers' +import { createTestStore, makeTab, makeWorktree } from './store-test-helpers' const WORKTREE_ID = 'repo1::/path/degraded' const TERMINAL_ID = 'terminal-degraded' @@ -199,6 +199,39 @@ it('keeps a terminal-free degraded workspace selected through hydration and pers expect(restored.activeTabType).toBe('browser') }) +it('keeps chrome when a non-authoritative fallback lists only some of the repo worktrees', () => { + const store = createTestStore() + // Why: an SSH metadata fallback can be non-empty yet partial (host-less metas are skipped on multi-owner + // repos, agent-scratch stays hidden). A partial list must not read as proof the missing worktree was deleted. + const sibling = makeWorktree({ + id: 'repo1::/path/sibling', + repoId: 'repo1', + path: '/path/sibling' + }) + store.setState({ + repos: [{ id: 'repo1', path: '/repo1', displayName: 'Repo 1', badgeColor: '#000', addedAt: 0 }], + worktreesByRepo: { repo1: [sibling] }, + detectedWorktreesByRepo: { + repo1: { + repoId: 'repo1', + authoritative: false, + source: 'metadata-fallback', + worktrees: [] + } + } + }) + const session = makeDegradedRepoSession() + store.getState().hydrateWorkspaceSession(session) + store.getState().hydrateTabsSession(session) + store.getState().hydrateEditorSession(session) + store.getState().hydrateBrowserSession(session) + + const state = store.getState() + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([TERMINAL_ID]) + expect(state.openFiles.map((file) => file.id)).toEqual([EDITOR_FILE_ID]) + expect(state.browserTabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([BROWSER_ID]) +}) + it('drops terminal-free chrome when an authoritative scan proves deletion', () => { const store = createTestStore() hydrateWithRepoScan(store, makeTerminalFreeDegradedRepoSession(), true) diff --git a/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts b/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts index 0fb4e42fd19..1a9693bdf5f 100644 --- a/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts +++ b/src/renderer/src/store/slices/degraded-repo-worktree-validity.ts @@ -45,13 +45,21 @@ export function buildValidWorktreeIdsForSessionHydration( .map((worktree) => worktree.id) ) const knownRepoIds = new Set(catalog.repos.map((repo) => repo.id)) + const detectedWorktreesByRepo = catalog.detectedWorktreesByRepo ?? {} const repoIdsWithLoadedWorktrees = new Set( Object.entries(worktreesByRepo) - .filter(([, worktrees]) => worktrees.length > 0) + // Why (#1158): a metadata fallback can be non-empty yet partial (host-less metas are skipped on + // multi-owner repos, agent-scratch stays hidden), so it cannot prove deletion the way a real listing can. + // Only an explicitly non-authoritative result is disqualified; a repo with no detection entry at all + // still counts as loaded, as before. + .filter( + ([repoId, worktrees]) => + worktrees.length > 0 && detectedWorktreesByRepo[repoId]?.authoritative !== false + ) .map(([repoId]) => repoId) ) const repoIdsWithAuthoritativeDetectedWorktrees = new Set( - Object.entries(catalog.detectedWorktreesByRepo ?? {}) + Object.entries(detectedWorktreesByRepo) .filter(([, detected]) => detected?.authoritative) .map(([repoId]) => repoId) ) diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index 6d8fff0696a..72d099a2731 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -21,6 +21,8 @@ import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rp import { LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' import type { HostQualifiedDetectedWorktreeResult, + HostQualifiedKnownWorktreeResult, + ListKnownWorktreesForExecutionHostArgs, ListDetectedWorktreesArgs } from '../../../../shared/detected-worktree-provider-contract' import type { DirectSshAuthority, SshProviderEpoch } from '../../../../shared/ssh-types' @@ -105,12 +107,17 @@ const listDetectedMock = vi.fn< return qualifyDetectedResult(args, result) }) +const listKnownForExecutionHostMock = vi.fn< + (args: ListKnownWorktreesForExecutionHostArgs) => Promise +>(async (args) => ({ status: 'rejected', ...args })) + const mockApi = { worktrees: { create: vi.fn(), prefetchCreateBase: vi.fn().mockResolvedValue(undefined), list: worktreeListMock, listDetected: listDetectedMock, + listKnownForExecutionHost: listKnownForExecutionHostMock, cancelListDetected: vi.fn().mockResolvedValue(undefined), listLineage: vi.fn().mockResolvedValue({}), remove: vi.fn().mockResolvedValue(undefined), @@ -153,6 +160,7 @@ import { createWorktreeSlice, getHostedReviewLinkMutationGenerationForTests, getHostedReviewLinkWorktreeAliasCountForTests, + resetAuthoritativelyRemovedWorktreeMemoryForTests, resetHostedReviewLinkMutationGenerationForTests } from './worktrees' import type { PendingWorktreeCreation } from '@/lib/pending-worktree-creation' @@ -439,6 +447,7 @@ describe('fetchWorktrees', () => { vi.clearAllMocks() resetRemoteRuntimeMocks() clearHugeRepoWarningDismissalsForTests() + resetAuthoritativelyRemovedWorktreeMemoryForTests() }) it('does not notify subscribers when the fetched payload is unchanged', async () => { @@ -1350,6 +1359,365 @@ describe('fetchWorktrees', () => { expect(store.getState().detectedWorktreesByRepo).toBe(detectedWorktreesByRepo) }) + it('shows persisted secondary worktrees while SSH is connecting', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const queued = makeWorktree({ + id: 'repo-ssh::/home/orca/queued', + repoId: 'repo-ssh', + path: '/home/orca/queued', + displayName: 'queued' + }) + const detected = makeDetectedResult('repo-ssh', [queued], { + authoritative: false, + source: 'metadata-fallback' + }) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: detected + }) + store.setState({ + repos: [sshRepo], + sshConnectionStates: new Map([ + [ + 'ssh-1', + { + targetId: 'ssh-1', + status: 'connecting', + error: null, + reconnectAttempt: 0, + providerEpoch: null + } + ] + ]) + } as Partial) + + await expect(store.getState().fetchWorktrees(sshRepo.id)).resolves.toBe(false) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + { ...queued, hostId: 'ssh:ssh-1' } + ]) + expect(listKnownForExecutionHostMock).toHaveBeenCalledWith({ + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1' + }) + expect(mockApi.worktrees.listDetected).not.toHaveBeenCalled() + }) + + it('adds metadata rows without replacing richer cached SSH worktrees', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const existing = makeWorktree({ + id: 'repo-ssh::/home/orca/existing', + repoId: 'repo-ssh', + path: '/home/orca/existing', + hostId: 'ssh:ssh-1', + head: 'live-head', + branch: 'refs/heads/live-branch' + }) + const metadataExisting = { ...existing, head: '', branch: '' } + const queued = makeWorktree({ + id: 'repo-ssh::/home/orca/queued', + repoId: 'repo-ssh', + path: '/home/orca/queued' + }) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: makeDetectedResult('repo-ssh', [metadataExisting, queued], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + store.setState({ + repos: [sshRepo], + sshConnectionStates: new Map(), + worktreesByRepo: { [sshRepo.id]: [existing] } + } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + existing, + { ...queued, hostId: 'ssh:ssh-1' } + ]) + }) + + it('inserts metadata rows inside the SSH block instead of past sibling hosts', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-shared', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const localRepo = { ...sshRepo, path: '/local/repo', connectionId: undefined } + const sshExisting = makeWorktree({ + id: 'repo-shared::/home/orca/existing', + repoId: sshRepo.id, + path: '/home/orca/existing', + hostId: 'ssh:ssh-1' + }) + const localExisting = makeWorktree({ + id: 'repo-shared::/local/existing', + repoId: sshRepo.id, + path: '/local/existing', + hostId: LOCAL_EXECUTION_HOST_ID + }) + const queued = makeWorktree({ + id: 'repo-shared::/home/orca/queued', + repoId: sshRepo.id, + path: '/home/orca/queued' + }) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: makeDetectedResult(sshRepo.id, [queued], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + store.setState({ + repos: [sshRepo, localRepo], + sshConnectionStates: new Map(), + worktreesByRepo: { [sshRepo.id]: [sshExisting, localExisting] } + } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id, { executionHostId: 'ssh:ssh-1' }) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + sshExisting, + { ...queued, hostId: 'ssh:ssh-1' }, + localExisting + ]) + }) + + it('drops metadata rows when SSH authority lands during the read', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const live = makeWorktree({ + id: 'repo-ssh::/home/orca/live', + repoId: 'repo-ssh', + path: '/home/orca/live', + hostId: 'ssh:ssh-1' + }) + // Why: deleted on the host, so an authoritative scan already purged it; the late metadata write must not resurrect it. + const purged = makeWorktree({ + id: 'repo-ssh::/home/orca/purged', + repoId: 'repo-ssh', + path: '/home/orca/purged' + }) + listKnownForExecutionHostMock.mockImplementationOnce(async (args) => { + store.setState({ + sshConnectionStates: new Map([ + [ + TEST_SSH_AUTHORITY.targetId, + { + targetId: TEST_SSH_AUTHORITY.targetId, + status: 'connected', + error: null, + reconnectAttempt: 0, + providerEpoch: TEST_SSH_AUTHORITY.providerEpoch, + connectionGeneration: TEST_SSH_AUTHORITY.connectionGeneration + } + ] + ]), + worktreesByRepo: { [sshRepo.id]: [live] } + } as Partial) + return { + status: 'complete', + repoId: args.repoId, + executionHostId: args.executionHostId, + result: makeDetectedResult(args.repoId, [live, purged], { + authoritative: false, + source: 'metadata-fallback' + }) + } + }) + store.setState({ + repos: [sshRepo], + sshConnectionStates: new Map() + } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([live]) + }) + + it('replaces metadata rows once the authoritative SSH scan lands', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const live = makeWorktree({ + id: 'repo-ssh::/home/orca/live', + repoId: 'repo-ssh', + path: '/home/orca/live', + hostId: 'ssh:ssh-1' + }) + const stale = makeWorktree({ + id: 'repo-ssh::/home/orca/stale', + repoId: 'repo-ssh', + path: '/home/orca/stale' + }) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: makeDetectedResult(sshRepo.id, [stale], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + const connectedStates = createTestStore().getState().sshConnectionStates + store.setState({ repos: [sshRepo], sshConnectionStates: new Map() } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id) + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + { ...stale, hostId: 'ssh:ssh-1' } + ]) + + worktreeListMock.mockResolvedValueOnce([live]) + store.setState({ sshConnectionStates: connectedStates } as Partial) + await store.getState().fetchWorktrees(sshRepo.id) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([live]) + }) + + it('keeps the repo detection entry authoritative while appending metadata rows', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-shared', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const localRepo = { ...sshRepo, path: '/local/repo', connectionId: undefined } + const scanned = makeWorktree({ + id: 'repo-shared::/local/scanned', + repoId: sshRepo.id, + path: '/local/scanned' + }) + const fromMetadata = makeWorktree({ + id: 'repo-shared::/home/orca/queued', + repoId: sshRepo.id, + path: '/home/orca/queued' + }) + const authoritative = makeDetectedResult(sshRepo.id, [scanned]) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: makeDetectedResult(sshRepo.id, [fromMetadata], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + store.setState({ + repos: [sshRepo, localRepo], + sshConnectionStates: new Map(), + detectedWorktreesByRepo: { [sshRepo.id]: authoritative } + } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id, { executionHostId: 'ssh:ssh-1' }) + + const detected = store.getState().detectedWorktreesByRepo[sshRepo.id] + // Why: this entry is shared with the co-owning local host, so the fallback must not demote its scan. + expect(detected?.authoritative).toBe(true) + expect(detected?.source).toBe('git') + expect(detected?.worktrees.map((worktree) => worktree.id)).toEqual([ + scanned.id, + fromMetadata.id + ]) + }) + + it('does not resurrect worktrees an authoritative SSH scan already removed', async () => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const live = makeWorktree({ + id: 'repo-ssh::/home/orca/live', + repoId: 'repo-ssh', + path: '/home/orca/live', + hostId: 'ssh:ssh-1' + }) + const deletedOnRemote = makeWorktree({ + id: 'repo-ssh::/home/orca/deleted', + repoId: 'repo-ssh', + path: '/home/orca/deleted' + }) + const metadataResult = () => ({ + status: 'complete' as const, + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1' as const, + result: makeDetectedResult(sshRepo.id, [deletedOnRemote], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + const connectedStates = createTestStore().getState().sshConnectionStates + listKnownForExecutionHostMock.mockResolvedValueOnce(metadataResult()) + store.setState({ repos: [sshRepo], sshConnectionStates: new Map() } as Partial) + + await store.getState().fetchWorktrees(sshRepo.id) + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + { ...deletedOnRemote, hostId: 'ssh:ssh-1' } + ]) + + // The host connects and an authoritative scan proves the worktree is gone. + worktreeListMock.mockResolvedValueOnce([live]) + store.setState({ sshConnectionStates: connectedStates } as Partial) + await store.getState().fetchWorktrees(sshRepo.id) + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([live]) + + // The host drops again; persisted metadata still lists the deleted worktree. + listKnownForExecutionHostMock.mockResolvedValueOnce(metadataResult()) + store.setState({ sshConnectionStates: new Map() } as Partial) + await store.getState().fetchWorktrees(sshRepo.id) + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([live]) + }) + it('keeps worktree maps byte-identical for stale and malformed direct results', async () => { const store = createTestStore() const existing = makeWorktree({ @@ -7543,6 +7911,63 @@ describe('fetchAllWorktrees hydration-time purge (design §4.4)', () => { addedAt: 0 } + it.each([false, true])( + 'hydrates connecting SSH worktrees with hydration purge completed=%s', + async (hasHydratedWorktreePurge) => { + const store = createTestStore() + const sshRepo = { + id: 'repo-ssh', + path: '/home/orca/repo', + displayName: 'SSH Repo', + badgeColor: '#000', + addedAt: 0, + connectionId: 'ssh-1' + } + const queued = makeWorktree({ + id: 'repo-ssh::/home/orca/queued', + repoId: 'repo-ssh', + path: '/home/orca/queued', + displayName: 'queued' + }) + listKnownForExecutionHostMock.mockResolvedValueOnce({ + status: 'complete', + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1', + result: makeDetectedResult(sshRepo.id, [queued], { + authoritative: false, + source: 'metadata-fallback' + }) + }) + store.setState({ + repos: [sshRepo], + hasHydratedWorktreePurge, + sshConnectionStates: new Map([ + [ + 'ssh-1', + { + targetId: 'ssh-1', + status: 'connecting', + error: null, + reconnectAttempt: 0, + providerEpoch: null + } + ] + ]) + } as Partial) + + await store.getState().fetchAllWorktrees() + + expect(store.getState().worktreesByRepo[sshRepo.id]).toEqual([ + { ...queued, hostId: 'ssh:ssh-1' } + ]) + expect(listKnownForExecutionHostMock).toHaveBeenCalledWith({ + repoId: sshRepo.id, + executionHostId: 'ssh:ssh-1' + }) + expect(mockApi.worktrees.listDetected).not.toHaveBeenCalled() + } + ) + it('preserves resolved inline legacy lineage when side-map hydration is absent', async () => { const store = createTestStore() const parent = makeWorktree({ diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 8facfa55dc2..3859cec22fc 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -121,6 +121,7 @@ import { import { getTerminalActivationSpawnSuppression } from './terminal-activation-spawn-suppression' import type { HostQualifiedDetectedWorktreeResult, + HostQualifiedKnownWorktreeResult, ListDetectedWorktreesArgs, ProviderRequestId, SshExecutionHostId @@ -2877,6 +2878,8 @@ function mergeFetchedWorktrees( args: FencedWorktreeMergeArgs ): boolean { let admitted = false + let authoritativelyRemovedIds: readonly string[] = [] + let authoritativelySeenIds: readonly string[] = [] set((s) => { if ( !isCurrentDetectedWorktreeRefresh(s, args.refresh) || @@ -2950,6 +2953,10 @@ function mergeFetchedWorktrees( args.refresh.result, args.hostId ) + authoritativelyRemovedIds = removedIds + if (args.refresh.result.authoritative) { + authoritativelySeenIds = args.refresh.result.worktrees.map((worktree) => worktree.id) + } const worktreesChanged = !areWorktreesEqual(s.worktreesByRepo[args.repoId], mergedWorktrees) const detectedChanged = !areDetectedWorktreeResultsEqual( s.detectedWorktreesByRepo[args.repoId], @@ -2979,9 +2986,210 @@ function mergeFetchedWorktrees( ...(removedIds.length > 0 ? buildWorktreePurgeState(s, removedIds) : {}) } }) + if (admitted) { + // Why: applied outside the updater so a repeated updater call cannot double-apply the removal memory. + forgetAuthoritativelyRemovedWorktrees(args.hostId, authoritativelySeenIds) + rememberAuthoritativelyRemovedWorktrees(args.hostId, authoritativelyRemovedIds) + } return admitted } +// Why: an authoritative scan is the only proof a remote worktree is gone, but SSH WorktreeMeta is exempt from +// gcStaleWorktreeMeta (persistence.ts:407,415), so without this memory the metadata fallback re-appends every +// deleted row on the next disconnect — forever. +const AUTHORITATIVE_REMOVAL_MEMORY_LIMIT = 512 +const authoritativelyRemovedWorktreeIdsByHost = new Map>() + +function rememberAuthoritativelyRemovedWorktrees( + hostId: ExecutionHostId, + worktreeIds: readonly string[] +): void { + if (worktreeIds.length === 0) { + return + } + const removed = authoritativelyRemovedWorktreeIdsByHost.get(hostId) ?? new Set() + for (const worktreeId of worktreeIds) { + // Why: re-insert so a re-removed id moves to the back of the insertion order the cap evicts from. + removed.delete(worktreeId) + removed.add(worktreeId) + } + for (const oldest of removed) { + if (removed.size <= AUTHORITATIVE_REMOVAL_MEMORY_LIMIT) { + break + } + removed.delete(oldest) + } + authoritativelyRemovedWorktreeIdsByHost.set(hostId, removed) +} + +// Why: a remote path can be recreated, so a scan that reports it again retracts the deletion verdict. +function forgetAuthoritativelyRemovedWorktrees( + hostId: ExecutionHostId, + worktreeIds: Iterable +): void { + const removed = authoritativelyRemovedWorktreeIdsByHost.get(hostId) + if (!removed) { + return + } + for (const worktreeId of worktreeIds) { + removed.delete(worktreeId) + } + if (removed.size === 0) { + authoritativelyRemovedWorktreeIdsByHost.delete(hostId) + } +} + +/** Test-only: module-level removal memory would otherwise leak across cases in one file. */ +export function resetAuthoritativelyRemovedWorktreeMemoryForTests(): void { + authoritativelyRemovedWorktreeIdsByHost.clear() +} + +function appendMissingWorktreesForHost< + T extends { id: string; hostId?: ExecutionHostId; runtimeOwnerEnvironmentId?: string } +>( + current: readonly T[] | undefined, + incoming: readonly T[], + hostId: ExecutionHostId, + options: WorktreeHostMatchOptions +): T[] { + const existing = current ?? [] + const existingHostIds = new Set( + existing + .filter((worktree) => worktreeMatchesHost(worktree, hostId, options)) + .map(({ id }) => id) + ) + const missing = incoming.filter((worktree) => !existingHostIds.has(worktree.id)) + if (missing.length === 0) { + return [...existing] + } + // Why: land inside the host's block like mergeWorktreesForHost does, else these rows sit past sibling hosts and visibly jump once the authoritative scan splices them back. + const lastHostIndex = existing.findLastIndex((worktree) => + worktreeMatchesHost(worktree, hostId, options) + ) + if (lastHostIndex === -1) { + return [...existing, ...missing] + } + return [...existing.slice(0, lastHostIndex + 1), ...missing, ...existing.slice(lastHostIndex + 1)] +} + +function isAdmittedKnownSshWorktreeResult( + result: HostQualifiedKnownWorktreeResult, + repoId: string, + executionHostId: SshExecutionHostId +): result is Extract { + return ( + result.status === 'complete' && + result.repoId === repoId && + result.executionHostId === executionHostId && + isDetectedWorktreeListResult(result.result) && + result.result.repoId === repoId && + result.result.authoritative === false + ) +} + +const inflightKnownSshWorktreeFetches = new Map< + string, + Promise +>() + +// Why: the authoritative path dedupes through listDetectedWorktreesForRepoCoalesced; without a matching guard +// the four refresh triggers can each issue this IPC and its merge for the same repo/host. +async function fetchKnownSshWorktreesForRepo( + set: Parameters>[0], + repoId: string, + executionHostId: SshExecutionHostId +): Promise { + const coalesceKey = `${repoId}${executionHostId}` + const inflight = inflightKnownSshWorktreeFetches.get(coalesceKey) + if (inflight) { + return await inflight + } + const request = runKnownSshWorktreeFetch(set, repoId, executionHostId).finally(() => { + inflightKnownSshWorktreeFetches.delete(coalesceKey) + }) + inflightKnownSshWorktreeFetches.set(coalesceKey, request) + return await request +} + +async function runKnownSshWorktreeFetch( + set: Parameters>[0], + repoId: string, + executionHostId: SshExecutionHostId +): Promise { + // Why: reads the local store only, so a runtime-hub session (whose repo ids live on the hub) always gets 'rejected' and keeps the pre-existing no-op. + const listKnown = window.api.worktrees.listKnownForExecutionHost + if (typeof listKnown !== 'function') { + return null + } + const result = await listKnown({ repoId, executionHostId }) + if (!isAdmittedKnownSshWorktreeResult(result, repoId, executionHostId)) { + return null + } + // Why: persisted SSH metadata outlives the remote worktree, so drop rows a completed scan already proved gone. + const suppressedIds = authoritativelyRemovedWorktreeIdsByHost.get(executionHostId) + const known = + suppressedIds && suppressedIds.size > 0 + ? { + ...result.result, + worktrees: result.result.worktrees.filter((worktree) => !suppressedIds.has(worktree.id)) + } + : result.result + let admitted = false + set((state) => { + // Why: the provider can connect during the await; authoritative rows already replaced this host, so appending stale metadata would resurrect purged worktrees. + if ( + getCurrentDirectSshAuthority(state, executionHostId) || + !repoHasExactlyOneExecutionHostOwner(state, repoId, executionHostId, false) + ) { + return state + } + admitted = true + const setup = getProjectHostSetupForRepoHost(state, repoId, executionHostId) + const matchOptions = worktreeHostMatchOptions(state, repoId, executionHostId) + const incomingDetected = known.worktrees.map((worktree) => + withRepoHostOwnership(worktree, executionHostId, setup) + ) + const priorDetected = state.detectedWorktreesByRepo[repoId] + // Why: only the rows are ours to merge. This entry is keyed by repo alone, so adopting the fallback's + // authoritative/source would demote a sibling host's completed scan and blank every authoritative-gated surface. + const detected = { + ...(priorDetected ?? known), + worktrees: appendMissingWorktreesForHost( + priorDetected?.worktrees, + incomingDetected, + executionHostId, + matchOptions + ) + } + const worktrees = appendMissingWorktreesForHost( + state.worktreesByRepo[repoId], + toVisibleWorktrees(known, executionHostId, setup), + executionHostId, + matchOptions + ) + const worktreesChanged = !areWorktreesEqual(state.worktreesByRepo[repoId], worktrees) + const detectedChanged = !areDetectedWorktreeResultsEqual( + state.detectedWorktreesByRepo[repoId], + detected + ) + if (!worktreesChanged && !detectedChanged) { + return state + } + return { + ...(worktreesChanged + ? { + worktreesByRepo: { ...state.worktreesByRepo, [repoId]: worktrees }, + sortEpoch: state.sortEpoch + 1 + } + : {}), + ...(detectedChanged + ? { detectedWorktreesByRepo: { ...state.detectedWorktreesByRepo, [repoId]: detected } } + : {}) + } + }) + return admitted ? known : null +} + export type DirectSshDetectedWorktreeRefresh = { waiterLeaseId: DetectedWorktreeRefreshLease['waiterLeaseId'] providerRequestId: ProviderRequestId @@ -3112,6 +3320,9 @@ export const createWorktreeSlice: StateCreator ? (getCurrentDirectSshAuthority(ownerState, hostId) ?? undefined) : undefined if (parsedHost?.kind === 'ssh' && !directSshAuthority) { + // Why: this function's contract is detected-only. The fallback runs for its store side effect, but + // callers keep seeing null as they did before the metadata path existed. + await fetchKnownSshWorktreesForRepo(set, repoId, parsedHost.id) return null } const refresh = await listDetectedWorktreesForRepoCoalesced( @@ -3210,6 +3421,11 @@ export const createWorktreeSlice: StateCreator ? (directCallerAuthority ?? getCurrentDirectSshAuthority(ownerState, hostId) ?? undefined) : undefined if (parsedHost?.kind === 'ssh' && !directSshAuthority) { + // Why: requireAuthoritative callers asked for authoritative-or-nothing, so writing non-authoritative + // rows as a side effect before returning false would silently weaken that contract. + if (!options?.requireAuthoritative) { + await fetchKnownSshWorktreesForRepo(set, repoId, parsedHost.id) + } return false } const refresh = await listDetectedWorktreesForRepoCoalesced(settings, repoId, { @@ -3274,6 +3490,7 @@ export const createWorktreeSlice: StateCreator ? (getCurrentDirectSshAuthority(requestStartedState, hostId) ?? undefined) : undefined if (parsedHost?.kind === 'ssh' && !directSshAuthority) { + await fetchKnownSshWorktreesForRepo(set, r.id, parsedHost.id) return } const refresh = await listDetectedWorktreesForRepoCoalesced(settings, r.id, { @@ -3325,6 +3542,7 @@ export const createWorktreeSlice: StateCreator ? (getCurrentDirectSshAuthority(requestStartedState, hostId) ?? undefined) : undefined if (parsedHost?.kind === 'ssh' && !directSshAuthority) { + await fetchKnownSshWorktreesForRepo(set, r.id, parsedHost.id) return { repoId: r.id, ok: false as const } } const refresh = await listDetectedWorktreesForRepoCoalesced( @@ -4082,6 +4300,11 @@ export const createWorktreeSlice: StateCreator // Why: invalidate stale probes once deletion is authoritative, so an old toast can't mutate a same-path replacement. forgetHugeRepoWarningDismissalsForWorktrees([worktreeId]) + // Why: forget-local is legal while the host is unreachable, so record the removal here too — otherwise an + // in-flight metadata read that snapshotted this row re-appends it, and disconnected polls never drop it. + if (hostId && parseExecutionHostId(hostId)?.kind === 'ssh') { + rememberAuthoritativelyRemovedWorktrees(hostId, [worktreeId]) + } const worktreeDisplayName = worktreeBeforeRemoval?.displayName?.trim() if (worktreeDisplayName) { diff --git a/src/shared/detected-worktree-provider-contract.ts b/src/shared/detected-worktree-provider-contract.ts index 4c5289db8fa..38b38ace37e 100644 --- a/src/shared/detected-worktree-provider-contract.ts +++ b/src/shared/detected-worktree-provider-contract.ts @@ -23,6 +23,24 @@ export type ListDetectedWorktreesArgs = | LocalDetectedWorktreeRequest | DirectSshDetectedWorktreeRequest +export type ListKnownWorktreesForExecutionHostArgs = { + repoId: string + executionHostId: SshExecutionHostId +} + +export type HostQualifiedKnownWorktreeResult = + | { + status: 'complete' + repoId: string + executionHostId: SshExecutionHostId + result: DetectedWorktreeListResult + } + | { + status: 'rejected' + repoId: string + executionHostId: SshExecutionHostId + } + export type AuthoritativeDetectedWorktreeHost = | { kind: 'local' From 4774d38239597a2ddd027e2e900afdd14699f300 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 5 Aug 2026 16:58:55 -0700 Subject: [PATCH 09/18] fix(sidebar): keep PR status visible with activity (#12793) --- .../sidebar/WorktreeCard.pr-display.test.tsx | 23 +-- .../src/components/sidebar/WorktreeCard.tsx | 15 +- .../sidebar/WorktreeCardReviewStatus.test.tsx | 53 ++++++ .../sidebar/WorktreeCardReviewStatus.tsx | 52 ++++++ .../sidebar/WorktreeCardStatusSlot.test.tsx | 164 +----------------- .../sidebar/WorktreeCardStatusSlot.tsx | 131 ++++---------- 6 files changed, 165 insertions(+), 273 deletions(-) create mode 100644 src/renderer/src/components/sidebar/WorktreeCardReviewStatus.test.tsx create mode 100644 src/renderer/src/components/sidebar/WorktreeCardReviewStatus.tsx diff --git a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx index a219841b949..d1e95db8c74 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx @@ -24,6 +24,7 @@ let issueCache: Record = {} let prCache: Record = {} let workspacePortScan: WorkspacePortScanResult | null = null let settings: Partial | null = null +let activityStatus = 'inactive' vi.mock('@/store', () => ({ useAppStore: (selector: (state: unknown) => unknown) => @@ -60,7 +61,7 @@ vi.mock('@/components/ui/tooltip', () => ({ })) vi.mock('./use-worktree-activity-status', () => ({ - useWorktreeActivityStatus: () => 'inactive' + useWorktreeActivityStatus: () => activityStatus })) vi.mock('./CacheTimer', () => ({ @@ -162,6 +163,7 @@ describe('WorktreeCard linked PR display', () => { prCache = {} workspacePortScan = null settings = null + activityStatus = 'inactive' }) it('keeps linked GH PR status out of the left status slot by default', async () => { @@ -229,11 +231,10 @@ describe('WorktreeCard linked PR display', () => { ) expect(unreadMarkup).not.toContain('aria-label="Mark as read"') - expect(unreadMarkup).toContain('PR checks: Failed · Unread') + expect(unreadMarkup).toContain('Inactive · Unread') + expect(unreadMarkup).toContain('PR checks: Failed') expect(unreadMarkup).not.toContain('Mark read') - expect(unreadMarkup).toContain('size-[13px] translate-x-px') - expect(unreadMarkup).not.toContain('lucide-bell') - expect(unreadMarkup).not.toContain('text-amber-500') + expect(unreadMarkup).toContain('size-[13px]') expect(unreadMarkup).toContain('data-worktree-status-lane-unread=""') expect(unreadMarkup).toContain('data-worktree-unread-alert=""') expect(unreadMarkup).toContain('bg-amber-500') @@ -241,7 +242,7 @@ describe('WorktreeCard linked PR display', () => { expect(getInlineRenameTitleTag(readMarkup)).toContain('font-normal text-foreground/80') }, 20_000) - it('shows linked GH PR status in the left status slot before hosted review details are cached when new card style is on', async () => { + it('shows linked GH PR status in the title row before hosted review details are cached when new card style is on', async () => { settings = { experimentalNewWorktreeCardStyle: true } const { default: WorktreeCard } = await import('./WorktreeCard') @@ -629,9 +630,10 @@ describe('WorktreeCard linked PR display', () => { expect(markup).not.toContain('58941') }) - it('renders linked PR status in the left status slot instead of the right metadata list', async () => { + it('renders activity and linked PR status in independent sidebar lanes', async () => { settings = { experimentalNewWorktreeCardStyle: true } worktreeCardProperties = ['status'] + activityStatus = 'active' hostedReviewCache = { 'local::repo-1::feature/local-branch': { data: makeHostedReview({ status: 'failure' }), @@ -645,12 +647,13 @@ describe('WorktreeCard linked PR display', () => { ) expect(markup).toContain('PR checks: Failed') - expect(markup).toContain('text-rose-500/85') + expect(markup).toContain('Active') + expect(markup).toContain('bg-emerald-500') + expect(markup).toContain('data-worktree-card-review-status=""') expect(markup).not.toContain('Linked PR #456') - expect(markup).not.toContain('CI checks') }) - it('uses branch PR cache for the status slot before hosted-review metadata warms', async () => { + it('uses branch PR cache for the title-row status before hosted-review metadata warms', async () => { settings = { experimentalNewWorktreeCardStyle: true } worktreeCardProperties = ['status'] prCache = { diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index b64ffd91cc5..26a2d020235 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -26,6 +26,7 @@ import { LinearAgentSkillSetupPrompt } from './LinearAgentSkillSetupPrompt' import WorktreeCardAgents from './WorktreeCardAgents' import { useWorktreeAgentRows } from './useWorktreeAgentRows' import { WorktreeCardStatusSlot } from './WorktreeCardStatusSlot' +import { WorktreeCardReviewStatus } from './WorktreeCardReviewStatus' import { cn } from '@/lib/utils' import { WorktreeCardSshHostControl } from './WorktreeCardSshHostControl' import { activateWorktreeFromSidebar } from '@/lib/sidebar-worktree-activation' @@ -1207,11 +1208,13 @@ const WorktreeCard = React.memo(function WorktreeCard({ !!conflictOperation && conflictOperation !== 'unknown' && conflictOperation !== 'rebase' const hasMetadataBadge = showConflictOperationBadge const showUnreadQuickAction = !affiliateListMode && showStatus && !newCardStyle - // Why: the slot owns the unread/status lane; legacy keeps the bell toggle, the new card keeps the glyph passive. + // Why: the slot owns activity and unread; review/check state has its own title-row indicator. const showCombinedStatusSlot = showStatus + const showReviewStatus = newCardStyle && showStatus && statusLaneReview !== null const showTitleRowPrimary = compactCards && worktree.isMainWorktree && !isFolder const showMetaRowDetails = !newCardStyle && !compactCards && (hasDetails || hasPorts) - const showTitleRowIndicators = (newCardStyle || compactCards) && (hasDetails || hasPorts) + const showTitleRowIndicators = + (newCardStyle || compactCards) && (hasDetails || hasPorts || showReviewStatus) // Why: grouped views can hide the repo badge; don't reserve a blank metadata lane unless there's real content. const hasDetailedMetaRowContent = Boolean( (showRepoBadgeInMetaRow && repo) || @@ -1370,7 +1373,12 @@ const WorktreeCard = React.memo(function WorktreeCard({ detailsAndPortsContent ) const titleRowIndicators = showTitleRowIndicators ? ( -
{detailsAndPorts}
+
+ {showReviewStatus && statusLaneReview ? ( + + ) : null} + {detailsAndPorts} +
) : null const hasSecondaryCardContent = hasMetaRow || !!remoteBranchConflict || showInlineAgentList || showLineageChildChip @@ -1404,7 +1412,6 @@ const WorktreeCard = React.memo(function WorktreeCard({ unreadTooltip={unreadTooltip} onPointerDown={stopQuickActionPointerPropagation} onToggleUnread={handleToggleUnreadQuick} - prDisplay={statusLaneReview} newCardStyle={newCardStyle} hasBranchIdentity={hasPathIdentityEnabled && Boolean(branchIdentityDisplay)} /> diff --git a/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.test.tsx new file mode 100644 index 00000000000..dab06044df9 --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.test.tsx @@ -0,0 +1,53 @@ +import { renderToStaticMarkup } from 'react-dom/server' +import type { ReactNode } from 'react' +import { describe, expect, it, vi } from 'vitest' +import { WorktreeCardReviewStatus } from './WorktreeCardReviewStatus' +import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' + +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +type ReviewOverrides = { + provider?: Exclude + state?: WorktreeCardPrDisplay['state'] + status?: WorktreeCardPrDisplay['status'] +} + +function renderReview(overrides: ReviewOverrides = {}): string { + const review: WorktreeCardPrDisplay = { + provider: 'github', + number: 123, + title: 'Review me', + state: 'open', + ...overrides + } + return renderToStaticMarkup() +} + +describe('WorktreeCardReviewStatus', () => { + it.each([ + [{ state: 'merged' }, 'PR: Merged'], + [{ state: 'closed' }, 'PR: Closed'], + [{ state: 'draft' }, 'PR: Draft'], + [{ status: 'failure' }, 'PR checks: Failed'], + [{ status: 'pending' }, 'PR checks: Pending'], + [{ status: 'success' }, 'PR checks: Passing'], + [{}, 'PR: Open'] + ] as const)('describes review state %o as %s', (overrides, label) => { + expect(renderReview(overrides)).toContain(label) + }) + + it('uses the compact generic review glyph in its own lane', () => { + const markup = renderReview({ provider: 'gitlab', status: 'pending' }) + + expect(markup).toContain('data-worktree-card-review-status=""') + expect(markup).toContain('MR checks: Pending') + expect(markup).toContain('viewBox="0 0 16 16"') + expect(markup).toContain('size-[13px]') + expect(markup).toContain('text-amber-500/85') + expect(markup).not.toContain('lucide-git-merge') + }) +}) diff --git a/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.tsx b/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.tsx new file mode 100644 index 00000000000..cab5ba030c1 --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeCardReviewStatus.tsx @@ -0,0 +1,52 @@ +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' +import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' +import { getReviewLabel, ReviewIcon } from './worktree-review-helpers' + +type WorktreeCardReviewStatusProps = { + review: WorktreeCardPrDisplay +} + +function getReviewStatusTooltip(review: WorktreeCardPrDisplay): string { + const label = getReviewLabel(review) + if (review.state === 'merged') { + return `${label}: Merged` + } + if (review.state === 'closed') { + return `${label}: Closed` + } + if (review.state === 'draft') { + return `${label}: Draft` + } + if (review.status === 'failure') { + return `${label} checks: Failed` + } + if (review.status === 'pending') { + return `${label} checks: Pending` + } + if (review.status === 'success') { + return `${label} checks: Passing` + } + return `${label}: Open` +} + +export function WorktreeCardReviewStatus({ + review +}: WorktreeCardReviewStatusProps): React.JSX.Element { + const tooltip = getReviewStatusTooltip(review) + return ( + + + + + {tooltip} + + + + {tooltip} + + + ) +} diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx index 8f204527c9a..bd1971bb614 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx @@ -2,7 +2,6 @@ import { renderToStaticMarkup } from 'react-dom/server' import type { ReactNode } from 'react' import { beforeEach, describe, expect, it, vi } from 'vitest' import { WorktreeCardStatusSlot } from './WorktreeCardStatusSlot' -import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' const mocks = vi.hoisted(() => ({ status: 'active' @@ -23,21 +22,6 @@ describe('WorktreeCardStatusSlot', () => { mocks.status = 'active' }) - const review: WorktreeCardPrDisplay = { - provider: 'github', - number: 123, - title: 'Review me', - state: 'open', - status: 'failure' - } - const gitlabReview: WorktreeCardPrDisplay = { - provider: 'gitlab', - number: 456, - title: 'Review me', - state: 'open', - status: 'pending' - } - it('lets the unread bell replace the visual status dot by default', () => { const markup = renderToStaticMarkup( { expect(markup).toContain('bg-emerald-500') }) - it('keeps the quiet active dot ahead of PR status by default', () => { - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('Active') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') - }) - - it('keeps the active dot ahead of PR status when new card style is on', () => { - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('Active') - expect(markup).toContain('inline-flex size-5 items-center justify-center') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') - }) - it('keeps the emerald activity dot ahead of branch identity when new card style is on', () => { const markup = renderToStaticMarkup( { expect(markup).not.toContain('lucide-git-branch') }) - it('uses the unified compact review glyph for GitLab MR status', () => { - mocks.status = 'inactive' - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('MR checks: Pending') - expect(markup).toContain('viewBox="0 0 16 16"') - expect(markup).toContain('size-[13px] translate-x-px') - expect(markup).toContain('text-amber-500/85') - expect(markup).not.toContain('lucide-git-merge') - }) - - it('keeps the done dot ahead of PR status when new card style is on', () => { + it('shows done activity in new card style', () => { mocks.status = 'done' const markup = renderToStaticMarkup( { unreadTooltip="Mark as unread" onPointerDown={vi.fn()} onToggleUnread={vi.fn()} - prDisplay={review} newCardStyle /> ) expect(markup).toContain('Done') expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') - }) - - it('uses PR status instead of the inactive dot when new card style is on', () => { - mocks.status = 'inactive' - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('PR checks: Failed') - expect(markup).toContain('text-rose-500/85') - expect(markup).not.toContain('bg-neutral-500/40') }) it('uses a branch icon with branch-only tooltip copy on quiet rows', () => { @@ -363,7 +261,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).not.toContain('lucide-git-branch') }) - it('keeps working activity ahead of PR status in new card style', () => { + it('shows working activity in new card style', () => { mocks.status = 'working' const markup = renderToStaticMarkup( { unreadTooltip="Mark as unread" onPointerDown={vi.fn()} onToggleUnread={vi.fn()} - prDisplay={review} newCardStyle /> ) @@ -382,10 +279,9 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).toContain('Working') expect(markup).toContain('inline-flex size-5 items-center justify-center') expect(markup).toContain('border-yellow-500') - expect(markup).not.toContain('PR checks: Failed') }) - it('keeps permission activity ahead of PR status in new card style', () => { + it('shows permission activity in new card style', () => { mocks.status = 'permission' const markup = renderToStaticMarkup( { unreadTooltip="Mark as unread" onPointerDown={vi.fn()} onToggleUnread={vi.fn()} - prDisplay={review} newCardStyle /> ) @@ -404,59 +299,6 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).toContain('Needs permission') expect(markup).toContain('lucide-message-circle-question-mark') expect(markup).toContain('text-amber-500') - expect(markup).not.toContain('PR checks: Failed') - }) - - it('keeps unread ahead of PR status by default', () => { - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('aria-label="Mark as read"') - expect(markup).toContain('Mark as read') - expect(markup).not.toContain('Active · Mark as read') - expect(markup).not.toContain('PR checks: Failed') - expect(markup).not.toContain('bg-emerald-500') - expect(markup).toContain('text-amber-500') - }) - - it('overlays an unread badge on PR status when new card style is on', () => { - mocks.status = 'inactive' - const markup = renderToStaticMarkup( - - ) - - expect(markup).not.toContain('aria-label="Mark as read"') - expect(markup).not.toContain('Mark as read') - expect(markup).toContain('PR checks: Failed · Unread') - expect(markup).toContain('data-worktree-status-lane-unread=""') - expect(markup).toContain('data-worktree-unread-alert=""') - expect(markup).not.toContain('group/unread') - expect(markup).not.toContain('cursor-pointer') - expect(markup).toContain('text-rose-500/85') - expect(markup).toContain('bg-amber-500') - expect(markup).not.toContain('lucide-bell') - expect(markup).not.toContain('text-amber-500') - expect(markup).not.toContain('bg-emerald-500') }) it('overlays an unread badge on the branch icon in new card style', () => { diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx index 527236c5549..d1a96bcdfea 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx @@ -3,12 +3,10 @@ import { Bell, GitBranch } from 'lucide-react' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { translate } from '@/i18n/i18n' import { cn } from '@/lib/utils' -import { getWorktreeStatusLabel, type WorktreeStatus } from '@/lib/worktree-status' +import { getWorktreeStatusLabel } from '@/lib/worktree-status' import { FilledBellIcon } from './WorktreeCardHelpers' import StatusIndicator from './StatusIndicator' import { useWorktreeActivityStatus } from './use-worktree-activity-status' -import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' -import { getReviewLabel, ReviewIcon } from './worktree-review-helpers' type WorktreeCardStatusSlotProps = { worktreeId: string @@ -18,24 +16,20 @@ type WorktreeCardStatusSlotProps = { unreadTooltip: string onToggleUnread: React.MouseEventHandler onPointerDown: React.PointerEventHandler - prDisplay?: WorktreeCardPrDisplay | null newCardStyle?: boolean hasBranchIdentity?: boolean branchIdentityLabel?: string className?: string } -// Passive identity only replaces status when the workspace is inactive. -const IDENTITY_REPLACEABLE_STATUSES = new Set(['inactive']) -// Why: a missing review display can also mean provider state is unavailable, -// so the passive label names the identity cue without claiming no review exists. +// Why: the passive label names the identity cue without claiming review state. function getDefaultBranchIdentityLabel(): string { return translate('auto.components.sidebar.WorktreeCardStatusSlot.branchIdentity', 'Branch') } // Why: branch-style SVGs are optically left-heavy; this keeps them aligned with // the centered activity dots in the shared status column. -const compactReviewAndBranchStatusIconClassName = 'size-[13px] translate-x-px' -const branchStatusIconClassName = `${compactReviewAndBranchStatusIconClassName} text-muted-foreground/70` +const compactBranchStatusIconClassName = 'size-[13px] translate-x-px' +const branchStatusIconClassName = `${compactBranchStatusIconClassName} text-muted-foreground/70` // Why: a left-edge badge overlays unread on the status glyph without widening // the lane or indenting the title; ring-sidebar cuts the dot out from busy icons. const newCardUnreadAlertClassName = @@ -64,29 +58,6 @@ function overlayNewCardUnreadStatus( ) } -function getReviewStatusTooltip(review: WorktreeCardPrDisplay): string { - const label = getReviewLabel(review) - if (review.state === 'merged') { - return `${label}: Merged` - } - if (review.state === 'closed') { - return `${label}: Closed` - } - if (review.state === 'draft') { - return `${label}: Draft` - } - if (review.status === 'failure') { - return `${label} checks: Failed` - } - if (review.status === 'pending') { - return `${label} checks: Pending` - } - if (review.status === 'success') { - return `${label} checks: Passing` - } - return `${label}: Open` -} - export function WorktreeCardStatusSlot({ worktreeId, showStatus, @@ -95,7 +66,6 @@ export function WorktreeCardStatusSlot({ unreadTooltip, onToggleUnread, onPointerDown, - prDisplay = null, newCardStyle = false, hasBranchIdentity = false, branchIdentityLabel, @@ -103,70 +73,43 @@ export function WorktreeCardStatusSlot({ }: WorktreeCardStatusSlotProps): React.JSX.Element | null { const status = useWorktreeActivityStatus(worktreeId) const statusLabel = getWorktreeStatusLabel(status) || status - const canShowReviewStatus = - newCardStyle && showStatus && prDisplay !== null && IDENTITY_REPLACEABLE_STATUSES.has(status) const canShowBranchStatus = - newCardStyle && - showStatus && - hasBranchIdentity && - prDisplay === null && - IDENTITY_REPLACEABLE_STATUSES.has(status) - const passiveStatusLabel = - canShowReviewStatus && prDisplay - ? getReviewStatusTooltip(prDisplay) - : canShowBranchStatus - ? (branchIdentityLabel ?? getDefaultBranchIdentityLabel()) - : statusLabel + newCardStyle && showStatus && hasBranchIdentity && status === 'inactive' + const passiveStatusLabel = canShowBranchStatus + ? (branchIdentityLabel ?? getDefaultBranchIdentityLabel()) + : statusLabel const passiveStatusTooltip = newCardStyle && isUnread ? `${passiveStatusLabel} · Unread` : passiveStatusLabel // Why: working and permission already own the new-card status lane, but // unread state should still surface in tooltip/sr-only copy and reappear afterward. const showNewCardUnreadAlert = newCardStyle && isUnread && showStatus && status !== 'working' && status !== 'permission' - const reviewStatusIconClassName = compactReviewAndBranchStatusIconClassName const branchStatusIcon =
) : null} diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx index 8f204527c9a..8911c1783c5 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx @@ -192,7 +192,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).not.toContain('PR checks: Failed') }) - it('keeps the active dot ahead of PR status when new card style is on', () => { + it('uses PR status instead of the quiet active dot when new card style is on', () => { const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('Active') + expect(markup).toContain('PR checks: Failed') expect(markup).toContain('inline-flex size-5 items-center justify-center') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') - }) - - it('keeps the emerald activity dot ahead of branch identity when new card style is on', () => { - const markup = renderToStaticMarkup( - - ) - - expect(markup).toContain('Active') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('lucide-git-branch') + expect(markup).toContain('size-[13px] translate-x-px') + expect(markup).toContain('text-rose-500/85') + expect(markup).not.toContain('bg-emerald-500') }) it('uses the unified compact review glyph for GitLab MR status', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { expect(markup).not.toContain('lucide-git-merge') }) - it('keeps the done dot ahead of PR status when new card style is on', () => { + it('uses PR status instead of the quiet done dot when new card style is on', () => { mocks.status = 'done' const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('Done') - expect(markup).toContain('bg-emerald-500') - expect(markup).not.toContain('PR checks: Failed') + expect(markup).toContain('PR checks: Failed') + expect(markup).not.toContain('bg-emerald-500') }) it('uses PR status instead of the inactive dot when new card style is on', () => { @@ -298,8 +277,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).not.toContain('bg-neutral-500/40') }) - it('uses a branch icon with branch-only tooltip copy on quiet rows', () => { - mocks.status = 'inactive' + it('uses a branch icon with branch-only tooltip copy by default', () => { const markup = renderToStaticMarkup( { }) it('uses context-aware branch or folder path tooltip copy', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { }) it('overlays an unread badge on PR status when new card style is on', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( { }) it('overlays an unread badge on the branch icon in new card style', () => { - mocks.status = 'inactive' const markup = renderToStaticMarkup( (['inactive']) +const QUIET_REVIEW_REPLACEABLE_STATUSES = new Set(['active', 'done', 'inactive']) // Why: a missing review display can also mean provider state is unavailable, // so the passive label names the identity cue without claiming no review exists. function getDefaultBranchIdentityLabel(): string { @@ -104,13 +103,16 @@ export function WorktreeCardStatusSlot({ const status = useWorktreeActivityStatus(worktreeId) const statusLabel = getWorktreeStatusLabel(status) || status const canShowReviewStatus = - newCardStyle && showStatus && prDisplay !== null && IDENTITY_REPLACEABLE_STATUSES.has(status) + newCardStyle && + showStatus && + prDisplay !== null && + QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const canShowBranchStatus = newCardStyle && showStatus && hasBranchIdentity && prDisplay === null && - IDENTITY_REPLACEABLE_STATUSES.has(status) + QUIET_REVIEW_REPLACEABLE_STATUSES.has(status) const passiveStatusLabel = canShowReviewStatus && prDisplay ? getReviewStatusTooltip(prDisplay) diff --git a/tests/e2e/worktree-card-status-indicator.spec.ts b/tests/e2e/worktree-card-status-indicator.spec.ts deleted file mode 100644 index 78075cb26c2..00000000000 --- a/tests/e2e/worktree-card-status-indicator.spec.ts +++ /dev/null @@ -1,84 +0,0 @@ -// Regression coverage for active status taking precedence over passive identity (#8813). - -import type { Locator, Page } from '@stablyai/playwright-test' -import { test, expect } from './helpers/orca-app' -import { - waitForSessionReady, - waitForActiveWorktree, - getAllWorktreeIds, - ensureTerminalVisible -} from './helpers/store' -import { worktreeRow, worktreeRowSurface } from './worktree-row-locators' - -function statusLane(page: Page, worktreeId: string): Locator { - return worktreeRow(page, worktreeId).locator('[data-worktree-card-status-slot]').first() -} - -function statusDot(page: Page, worktreeId: string): Locator { - return statusLane(page, worktreeId).locator('span.bg-emerald-500').first() -} - -function branchIdentityGlyph(page: Page, worktreeId: string): Locator { - return statusLane(page, worktreeId).locator('svg.lucide-git-branch') -} - -// PTY liveness, not the tab row, drives the activity heuristic. -async function waitForLivePty(page: Page, worktreeId: string): Promise { - await expect - .poll( - async () => - page.evaluate((id) => { - const state = window.__store!.getState() - return (state.tabsByWorktree[id] ?? []).some( - (tab) => (state.ptyIdsByTabId[tab.id] ?? []).length > 0 - ) - }, worktreeId), - { timeout: 30_000, message: `No live PTY attached for worktree ${worktreeId}` } - ) - .toBe(true) -} - -test.describe('Worktree card status indicator', () => { - test.beforeEach(async ({ orcaPage }) => { - await waitForSessionReady(orcaPage) - await waitForActiveWorktree(orcaPage) - }) - - test('paints the emerald Active dot instead of the grey branch glyph once a workspace goes live', async ({ - orcaPage - }) => { - const liveWorktreeId = await waitForActiveWorktree(orcaPage) - await ensureTerminalVisible(orcaPage) - await waitForLivePty(orcaPage, liveWorktreeId) - - // The regression requires both the new card style and branch identity. - await orcaPage.evaluate(async () => { - const state = window.__store!.getState() - await state.updateSettings({ experimentalNewWorktreeCardStyle: true }) - state.setWorktreeCardProperties(['status', 'unread', 'branch']) - }) - - const quietWorktreeId = (await getAllWorktreeIds(orcaPage)).find((id) => id !== liveWorktreeId) - if (!quietWorktreeId) { - throw new Error('Seeded repo did not expose a second worktree to keep quiet') - } - - await expect(branchIdentityGlyph(orcaPage, quietWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, quietWorktreeId)).toHaveText('Branch') - - await expect(statusDot(orcaPage, liveWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, liveWorktreeId)).toHaveText('Active') - await expect(branchIdentityGlyph(orcaPage, liveWorktreeId)).toHaveCount(0) - - await worktreeRowSurface(orcaPage, quietWorktreeId).click() - await expect - .poll(async () => orcaPage.evaluate(() => window.__store!.getState().activeWorktreeId)) - .toBe(quietWorktreeId) - await ensureTerminalVisible(orcaPage) - await waitForLivePty(orcaPage, quietWorktreeId) - - await expect(statusDot(orcaPage, quietWorktreeId)).toBeVisible() - await expect(statusLane(orcaPage, quietWorktreeId)).toHaveText('Active') - await expect(branchIdentityGlyph(orcaPage, quietWorktreeId)).toHaveCount(0) - }) -}) From ff01fad4adda08423f657d10ecacaf682e0a1601 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 5 Aug 2026 20:28:10 -0700 Subject: [PATCH 18/18] fix(runtime): stop a stalled tick from forgiving banked missed probes (#12841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge review of #12790 demonstrated a real leak: the resume-from-pause pardon cleared banked misses outright, so a host whose sweep stalls once every three ticks reset the budget forever and a dead socket was never reaped. The reviewer ran 300 sweeps against a permanently dead peer with a >1.5x gap every third tick and observed zero terminate() calls. Pre-#12790 that required a stall on *every* tick; the counter widened the pathological window 3x, and the failure mode is permanent non-reaping — the MAX_WS_CONNECTIONS leak the reaper exists to prevent. A stalled tick now charges no miss, which is all the original rationale needed (the client had no chance to answer that probe), but no longer forgives the misses already banked. A live client still clears its own count by answering the probe that is still sent on the stalled tick. The tolerance test's pause case is rewritten to assert the new contract rather than the old forgive-everything one, and a new test pins the leak directly: a host stalling every third tick must still reap a dead socket. Co-authored-by: Jinwoo-H --- ...r-heartbeat-missed-probe-tolerance.test.ts | 44 +++++++++++++++---- .../rpc/remote-runtime-server-heartbeat.ts | 15 +++---- 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/main/runtime/rpc/remote-runtime-server-heartbeat-missed-probe-tolerance.test.ts b/src/main/runtime/rpc/remote-runtime-server-heartbeat-missed-probe-tolerance.test.ts index 2e3a3427175..665d94b0f41 100644 --- a/src/main/runtime/rpc/remote-runtime-server-heartbeat-missed-probe-tolerance.test.ts +++ b/src/main/runtime/rpc/remote-runtime-server-heartbeat-missed-probe-tolerance.test.ts @@ -53,7 +53,9 @@ describe('RemoteRuntimeServerHeartbeat missed-probe tolerance', () => { for (let probe = 0; probe < 6; probe += 1) { now += INTERVAL_MS await vi.advanceTimersByTimeAsync(INTERVAL_MS) - missesBeforeReap.push((socket.terminate as unknown as ReturnType).mock.calls.length) + missesBeforeReap.push( + (socket.terminate as unknown as ReturnType).mock.calls.length + ) } // Silence must be tolerated for more than a single probe, and must eventually end the socket. @@ -85,7 +87,7 @@ describe('RemoteRuntimeServerHeartbeat missed-probe tolerance', () => { heartbeat.stop() }) - it('clears accumulated misses when the server event loop resumes from a pause', async () => { + it('charges no miss for a paused tick but keeps the ones already banked', async () => { vi.useFakeTimers() let now = 1_000 const socket = makeSocket() @@ -114,8 +116,9 @@ describe('RemoteRuntimeServerHeartbeat missed-probe tolerance', () => { expect(toleratedMisses).toBeGreaterThan(1) heartbeat.stop() - // Replay on a fresh socket: bank misses to one short of the limit, suspend, then bank the same - // number again. The pause must write the earlier misses off, not top them up to the limit. + // Replay on a fresh socket: bank misses to one short of the limit, then suspend. The stalled tick + // must charge nothing — the client had no chance to answer it — but it must not forgive the misses + // already banked, or a host that stalls periodically could shelter a dead socket indefinitely. const resumed = new RemoteRuntimeServerHeartbeat(INTERVAL_MS, () => now) const resumedSocket = makeSocket() resumed.noteAlive(resumedSocket) @@ -129,11 +132,36 @@ describe('RemoteRuntimeServerHeartbeat missed-probe tolerance', () => { now += 3_600_000 await vi.advanceTimersByTimeAsync(INTERVAL_MS) - for (let miss = 0; miss < toleratedMisses - 1; miss += 1) { - await sweep() - } - + // The suspend itself costs the socket nothing. expect(reaped(resumedSocket)).toBe(false) + + // But the pre-suspend evidence survives, so the very next unanswered probe completes the budget. + await sweep() + expect(reaped(resumedSocket)).toBe(true) resumed.stop() }) + + // Why: pre-fix the pardon cleared banked misses, so a host stalling once every few ticks reset the + // budget forever and a dead socket was never reaped — the connection leak the reaper exists to stop. + it('still reaps a dead socket when the host stalls repeatedly', async () => { + vi.useFakeTimers() + let now = 1_000 + const socket = makeSocket() + const heartbeat = new RemoteRuntimeServerHeartbeat(INTERVAL_MS, () => now) + heartbeat.noteAlive(socket) + heartbeat.start(() => [socket]) + heartbeat.noteAlive(socket) + + const reaped = (): boolean => + (socket.terminate as unknown as ReturnType).mock.calls.length > 0 + + // Stall every third tick — the worst case the old pardon could not survive. + for (let tick = 0; tick < 30 && !reaped(); tick += 1) { + now += tick % 3 === 2 ? INTERVAL_MS * 2 : INTERVAL_MS + await vi.advanceTimersByTimeAsync(INTERVAL_MS) + } + + expect(reaped()).toBe(true) + heartbeat.stop() + }) }) diff --git a/src/main/runtime/rpc/remote-runtime-server-heartbeat.ts b/src/main/runtime/rpc/remote-runtime-server-heartbeat.ts index 3913e9d9b6b..6134259c51f 100644 --- a/src/main/runtime/rpc/remote-runtime-server-heartbeat.ts +++ b/src/main/runtime/rpc/remote-runtime-server-heartbeat.ts @@ -52,21 +52,20 @@ export class RemoteRuntimeServerHeartbeat { const tickAt = this.now() const elapsedMs = tickAt - (this.lastTickAt ?? tickAt) this.lastTickAt = tickAt - const resumedFromPause = elapsedMs < 0 || elapsedMs > this.intervalMs * 1.5 + // Why: a delayed tick cannot infer that clients missed a probe they had no chance to answer, so + // this sweep must not charge one. It must not *forgive* either: clearing banked misses lets a host + // that stalls once every few ticks protect a dead socket forever, which is the connection leak the + // reaper exists to prevent. Skipping the charge is enough — a live client clears its own count by + // answering the probe still sent below. + const stalledTick = elapsedMs < 0 || elapsedMs > this.intervalMs * 1.5 let reaped = 0 let clientCount = 0 for (const socket of clients) { clientCount += 1 - if (resumedFromPause) { - // Why: a delayed server tick cannot infer that clients missed a probe they had no chance to - // answer. Seeding alive also clears any misses banked before the pause, so a suspend can never - // top up a budget the client was never given a chance to defend. - this.alive.add(socket) - } if (this.alive.has(socket)) { this.alive.delete(socket) this.missedProbes.delete(socket) - } else { + } else if (!stalledTick) { const missed = (this.missedProbes.get(socket) ?? 0) + 1 if (missed >= this.missedProbeLimit) { this.missedProbes.delete(socket)