From 46756b386a59978ab18bb85579e4a53be73d698b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:43:35 -0700 Subject: [PATCH 001/134] fix(terminal): retain WebGL for recently hidden worktrees (#13372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): retain WebGL for recently hidden worktrees Suspending a hidden worktree disposes every pane's WebGL addon, so every switch-back presents DOM-renderer frames (unfloored ~5% wider advance, and the channel through which any poisoned cell metrics reach the screen) until reattach completes — 0.5-3s on loaded sessions. Keep the live addons for the most recently hidden worktrees instead: LRU capped at 6 contexts (Chromium allows ~16/page, visible worktrees use 1-4), least-recent evicted to dispose exactly as before. Reveal then has no renderer swap and nothing to flash. Window-occlusion callers pass no retention context and are unchanged. * fix(terminal): pause retained hidden cursor work * fix(terminal): preserve retained WebGL through deferred rebuilds --- .../terminal-visibility-resume.ts | 3 +- .../src/lib/pane-manager/pane-dom-creation.ts | 1 + .../lib/pane-manager/pane-lifecycle.test.ts | 13 ++ .../src/lib/pane-manager/pane-lifecycle.ts | 15 +- .../lib/pane-manager/pane-manager-types.ts | 2 + .../src/lib/pane-manager/pane-manager.ts | 9 +- .../pane-manager/pane-rendering-control.ts | 43 ++++- .../pane-webgl-context-recovery.test.ts | 45 +++++- .../lib/pane-manager/pane-webgl-reattach.ts | 5 + .../lib/pane-manager/pane-webgl-renderer.ts | 10 ++ .../terminal-webgl-hidden-retention.test.ts | 152 ++++++++++++++++++ .../terminal-webgl-hidden-retention.ts | 81 ++++++++++ 12 files changed, 361 insertions(+), 18 deletions(-) create mode 100644 src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.test.ts create mode 100644 src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.ts diff --git a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts index 34907085ccb..ee86edeba18 100644 --- a/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts +++ b/src/renderer/src/components/terminal-pane/terminal-visibility-resume.ts @@ -131,8 +131,7 @@ export function hideTerminalVisibility({ captureViewportPositions(false) } if (!isWorktreeActive && (wasVisible || surfaceBecameHidden)) { - // Suspend WebGL when going hidden. xterm.write() continues to land in the - // DOM-renderer fallback terminal; the suspend is purely a GPU resource decision. + // xterm.write() keeps updating the hidden buffer; suspension only changes renderer lifetime. manager.suspendRendering() return { hiddenReason: 'surface', renderingSuspended: true } } diff --git a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts index f383dfa3c53..d3b6b2b6ffd 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts @@ -121,6 +121,7 @@ export function createPaneDOM( gpuRenderingEnabled: ENABLE_WEBGL_RENDERER, webglAttachmentDeferred: false, webglDisabledAfterContextLoss: false, + webglRebuildDeferred: false, hasComplexScriptOutput: false, fitAddon, fitResizeObserver: null, diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts index 26b19453f64..9cb442a2073 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts @@ -430,6 +430,19 @@ describe('attachLigatures', () => { expect(pane.terminal.refresh).toHaveBeenCalledWith(0, 23) expect(pane.ligaturesAddon).not.toBeNull() }) + + it('defers a retained WebGL rebuild while hidden', () => { + const pane = createPane() + const retainedAddon = { dispose: vi.fn() } as never + pane.webglAddon = retainedAddon + pane.webglAttachmentDeferred = true + + attachLigatures(pane) + + expect(pane.webglAddon).toBe(retainedAddon) + expect(pane.webglRebuildDeferred).toBe(true) + expect(pane.terminal.refresh).not.toHaveBeenCalled() + }) }) describe('openTerminal — addon and provider wiring', () => { diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts index 131a947312e..3c89a6ec723 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts @@ -16,6 +16,7 @@ import { import { installTerminalLinkifierHoverResetOnWrite } from './terminal-linkifier-hover-reset-on-write' import { attachDomRendererFocusClassSync } from './pane-dom-focus-class-sync' import { attachWebgl, cancelPendingWebglRefresh, disposeWebgl } from './pane-webgl-renderer' +import { rebuildAttachedWebgl } from './pane-webgl-reattach' import { configureLazyArabicShapingJoiner } from './terminal-arabic-shaping-joiner' import { TerminalLigaturesAddon } from './terminal-ligatures-addon' import { installTerminalImeCandidateAnchor } from './terminal-ime-candidate-anchor' @@ -136,16 +137,15 @@ export function attachLigatures(pane: ManagedPaneInternal): void { pane.ligaturesAddon = ligaturesAddon // Why: ligatures can be enabled after rows already rendered, especially // from Settings. Force existing glyph runs to be recomputed immediately. - pane.terminal.refresh(0, pane.terminal.rows - 1) + if (!pane.webglAttachmentDeferred) { + pane.terminal.refresh(0, pane.terminal.rows - 1) + } // Why: the WebGL renderer builds its glyph texture atlas at activation // time, so `font-feature-settings` applied after WebGL loaded won't // reach the GPU-rendered cells until the atlas is rebuilt. The upstream // docs call this out explicitly — reactivating WebGL after ligatures // forces a fresh atlas that includes the ligated glyphs. - if (pane.webglAddon) { - disposeWebgl(pane) - attachWebgl(pane) - } + rebuildAttachedWebgl(pane) } catch (err) { console.warn('[terminal] ligatures addon failed to attach for pane', pane.id, err) pane.ligaturesAddon = null @@ -162,10 +162,7 @@ export function setLigaturesEnabled(pane: ManagedPaneInternal, enabled: boolean) // Why: ligatures lived inside the WebGL atlas, so after disposing the // addon the atlas still holds the ligated glyphs. Rebuild it so text // renders as the non-ligated fallback immediately. - if (pane.webglAddon) { - disposeWebgl(pane) - attachWebgl(pane) - } + rebuildAttachedWebgl(pane) } } diff --git a/src/renderer/src/lib/pane-manager/pane-manager-types.ts b/src/renderer/src/lib/pane-manager/pane-manager-types.ts index 113f10af80d..34b42c4b591 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-types.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-types.ts @@ -142,6 +142,8 @@ export type ManagedPaneInternal = { gpuRenderingEnabled: boolean webglAttachmentDeferred: boolean webglDisabledAfterContextLoss: boolean + // Hidden retained renderers rebuild at the resume boundary, never behind the hidden surface. + webglRebuildDeferred?: boolean // Why per-pane: one pane's failed WebGL attach must not strand every other // pane on the DOM renderer until the next recovery boundary. Optional so // absent means "never failed"; only the attach failure path sets it. diff --git a/src/renderer/src/lib/pane-manager/pane-manager.ts b/src/renderer/src/lib/pane-manager/pane-manager.ts index 82d6f5b1ada..fbdbe1f5afe 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager.ts @@ -41,6 +41,7 @@ import { } from './pane-rendering-control' import type { TerminalLeafId } from '../../../../shared/stable-pane-id' import { registerLivePaneManager, unregisterLivePaneManager } from './pane-manager-registry' +import { releaseHiddenWebglRetention } from './terminal-webgl-hidden-retention' import { schedulePaneRevealPresent, schedulePaneRevealRepaint } from './pane-reveal-repaint' import { fitRevealedPane } from './pane-reveal-fit' import { PaneIdentityRegistry } from './pane-identity-registry' @@ -391,12 +392,15 @@ export class PaneManager { suspendRendering(): void { this.renderingSuspended = true - suspendPaneRendering(this.panes.values()) + suspendPaneRendering(this.panes.values(), { + owner: this, + livePanes: () => (this.destroyed ? [] : this.panes.values()) + }) } resumeRendering(): void { this.renderingSuspended = false - resumePaneRendering(this.panes.values()) + resumePaneRendering(this.panes.values(), this) } movePane(sourcePaneId: number, targetPaneId: number, zone: DropZone): void { @@ -410,6 +414,7 @@ export class PaneManager { destroy(): void { this.destroyed = true unregisterLivePaneManager(this) + releaseHiddenWebglRetention(this) cancelActivePaneDrag(this.dragState) this.cancelPendingPaneReparentFrames() for (const pane of this.panes.values()) { diff --git a/src/renderer/src/lib/pane-manager/pane-rendering-control.ts b/src/renderer/src/lib/pane-manager/pane-rendering-control.ts index 5477fd4f3b0..2c005f0ff05 100644 --- a/src/renderer/src/lib/pane-manager/pane-rendering-control.ts +++ b/src/renderer/src/lib/pane-manager/pane-rendering-control.ts @@ -4,10 +4,15 @@ import { attachWebgl, clearTerminalWebglAttachBackoff, disposeWebgl, + isPaneWebglContextLost, markComplexScriptOutput, resetWebglTextureAtlas } from './pane-webgl-renderer' -import { reattachWebglIfNeeded } from './pane-webgl-reattach' +import { rebuildAttachedWebgl, reattachWebglIfNeeded } from './pane-webgl-reattach' +import { + releaseHiddenWebglRetention, + tryRetainHiddenPanesWebgl +} from './terminal-webgl-hidden-retention' export function setPaneGpuRenderingState( panes: Map, @@ -42,21 +47,51 @@ export function markPaneComplexScriptOutput( } } -export function suspendPaneRendering(panes: Iterable): void { - for (const pane of panes) { +export function suspendPaneRendering( + panes: Iterable, + retention?: { owner: object; livePanes: () => Iterable } +): void { + const suspended = Array.from(panes) + for (const pane of suspended) { pane.webglAttachmentDeferred = true + } + // Keep recent hidden worktrees on live WebGL so switch-back never presents + // DOM-fallback frames; evicted/over-cap owners fall back to dispose. + if (retention && tryRetainHiddenPanesWebgl(retention.owner, retention.livePanes)) { + // Why: retained WebGL keeps xterm's focused cursor timer alive behind the hidden surface. + for (const pane of suspended) { + pane.terminal.blur() + } + return + } + for (const pane of suspended) { disposeWebgl(pane) } } -export function resumePaneRendering(panes: Iterable): void { +export function resumePaneRendering( + panes: Iterable, + retentionOwner?: object +): void { + if (retentionOwner) { + releaseHiddenWebglRetention(retentionOwner) + } for (const pane of panes) { // Why: resume (worktree foreground, window wake) is the WebGL retry // boundary — Chromium may have restored the GPU process since a context // loss, and bounding retries to resume events cannot loop on live loss. clearTerminalWebglAttachBackoff(pane) + const rebuildDeferred = pane.webglRebuildDeferred === true pane.webglAttachmentDeferred = false pane.webglDisabledAfterContextLoss = false + pane.webglRebuildDeferred = false + if (pane.webglAddon && isPaneWebglContextLost(pane)) { + disposeWebgl(pane) + } + if (rebuildDeferred && pane.webglAddon) { + rebuildAttachedWebgl(pane) + continue + } reattachWebglIfNeeded(pane) } } diff --git a/src/renderer/src/lib/pane-manager/pane-webgl-context-recovery.test.ts b/src/renderer/src/lib/pane-manager/pane-webgl-context-recovery.test.ts index 785009ac605..128057e463d 100644 --- a/src/renderer/src/lib/pane-manager/pane-webgl-context-recovery.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-webgl-context-recovery.test.ts @@ -1,8 +1,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { setTerminalWebglDiagnosticRecorder } from '../../../../shared/terminal-webgl-diagnostics' import type { ManagedPaneInternal } from './pane-manager-types' -import { resumePaneRendering } from './pane-rendering-control' +import { resumePaneRendering, suspendPaneRendering } from './pane-rendering-control' import { attachWebgl, resetTerminalWebglSuggestion } from './pane-webgl-renderer' +import { rebuildAttachedWebgl } from './pane-webgl-reattach' function createPane(options: { loadAddon?: () => void } = {}): ManagedPaneInternal { const leafId = '11111111-1111-4111-8111-111111111111' as never @@ -14,6 +15,7 @@ function createPane(options: { loadAddon?: () => void } = {}): ManagedPaneIntern cols: 80, rows: 24, refresh: vi.fn(), + blur: vi.fn(), loadAddon: vi.fn(options.loadAddon) } as never, container: {} as never, @@ -112,6 +114,47 @@ describe('terminal WebGL context recovery', () => { expect(pane.webglAddon).not.toBeNull() }) + it('defers retained replay rebuilds until rendering resumes', () => { + const owner = {} + const pane = createPane() + attachWebgl(pane) + const retainedAddon = pane.webglAddon + suspendPaneRendering([pane], { owner, livePanes: () => [pane] }) + + rebuildAttachedWebgl(pane) + + expect(pane.webglAddon).toBe(retainedAddon) + expect(pane.webglRebuildDeferred).toBe(true) + expect(pane.terminal.loadAddon).toHaveBeenCalledTimes(1) + + resumePaneRendering([pane], owner) + + expect(pane.webglAddon).not.toBe(retainedAddon) + expect(pane.webglRebuildDeferred).toBe(false) + expect(pane.terminal.loadAddon).toHaveBeenCalledTimes(2) + }) + + it('replaces a retained context lost before xterm dispatches its loss event', () => { + const pane = createPane() + const dispose = vi.fn() + const lostAddon = { + dispose, + _renderer: { + _gl: { + getExtension: vi.fn(() => null), + isContextLost: vi.fn(() => true) + } + } + } + pane.webglAddon = lostAddon as never + + resumePaneRendering([pane]) + + expect(dispose).toHaveBeenCalledTimes(1) + expect(pane.webglAddon).not.toBe(lostAddon) + expect(pane.terminal.loadAddon).toHaveBeenCalledTimes(1) + }) + it('re-latches when the retried context is lost again', () => { const pane = createPane() diff --git a/src/renderer/src/lib/pane-manager/pane-webgl-reattach.ts b/src/renderer/src/lib/pane-manager/pane-webgl-reattach.ts index 15018a1d185..98c5c10e079 100644 --- a/src/renderer/src/lib/pane-manager/pane-webgl-reattach.ts +++ b/src/renderer/src/lib/pane-manager/pane-webgl-reattach.ts @@ -11,6 +11,11 @@ export function rebuildAttachedWebgl(pane: ManagedPaneInternal): void { if (!pane.webglAddon || pane.webglDisabledAfterContextLoss) { return } + if (pane.webglAttachmentDeferred) { + pane.webglRebuildDeferred = true + return + } + pane.webglRebuildDeferred = false disposeWebgl(pane) // Why: the live addon just proved context creation works, so a stale attach // backoff from an earlier failure must not downgrade this pane to DOM. diff --git a/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts b/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts index a4248612fd2..f162052cebe 100644 --- a/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts +++ b/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts @@ -23,6 +23,7 @@ let suggestedRendererType: 'dom' | undefined type ReleasableWebglContext = { getExtension(name: 'WEBGL_lose_context'): WEBGL_lose_context | null + isContextLost?: () => boolean } type XtermWebglAddonInternals = { @@ -74,6 +75,15 @@ export function cancelPendingWebglRefresh(pane: ManagedPaneInternal): void { pane.pendingWebglRefreshRafId = null } +export function isPaneWebglContextLost(pane: ManagedPaneInternal): boolean { + try { + const renderer = (pane.webglAddon as unknown as XtermWebglAddonInternals | null)?._renderer + return renderer?._gl?.isContextLost?.() === true + } catch { + return true + } +} + export function disposeWebgl( pane: ManagedPaneInternal, options?: { refreshDimensions?: boolean } diff --git a/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.test.ts b/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.test.ts new file mode 100644 index 00000000000..cfe3fba4e37 --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.test.ts @@ -0,0 +1,152 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { ManagedPaneInternal } from './pane-manager-types' +import { resumePaneRendering, suspendPaneRendering } from './pane-rendering-control' +import { + releaseHiddenWebglRetention, + resetHiddenWebglRetentionForTest, + retainedHiddenWebglOwnerCountForTest, + tryRetainHiddenPanesWebgl +} from './terminal-webgl-hidden-retention' + +function createPane(withAddon = true): ManagedPaneInternal { + return { + terminal: { blur: vi.fn() }, + webglAddon: withAddon + ? ({ dispose: vi.fn() } as unknown as ManagedPaneInternal['webglAddon']) + : null, + webglAttachmentDeferred: false, + webglDisabledAfterContextLoss: false, + webglAttachFailedSinceRecovery: false, + gpuRenderingEnabled: true, + pendingWebglRefreshRafId: null + } as unknown as ManagedPaneInternal +} + +function retentionFor(owner: object, panes: ManagedPaneInternal[]) { + return { owner, livePanes: () => panes } +} + +describe('terminal-webgl-hidden-retention', () => { + beforeEach(() => { + resetHiddenWebglRetentionForTest() + }) + + it('suspend retains live WebGL addons and still defers attachment', () => { + const owner = {} + const panes = [createPane(), createPane()] + suspendPaneRendering(panes, retentionFor(owner, panes)) + expect(panes.every((pane) => pane.webglAttachmentDeferred)).toBe(true) + expect(panes.every((pane) => pane.webglAddon != null)).toBe(true) + expect(panes.every((pane) => vi.mocked(pane.terminal.blur).mock.calls.length === 1)).toBe(true) + expect(retainedHiddenWebglOwnerCountForTest()).toBe(1) + }) + + it('suspend without retention context disposes addons (window-level callers unchanged)', () => { + const panes = [createPane()] + const addon = panes[0].webglAddon + suspendPaneRendering(panes) + expect(addon?.dispose).toHaveBeenCalled() + expect(panes[0].webglAddon).toBeNull() + expect(panes[0].terminal.blur).not.toHaveBeenCalled() + }) + + it('evicts the least-recently-hidden owner over the context cap', () => { + const ownerA = {} + const panesA = [createPane(), createPane(), createPane()] + const addonsA = panesA.map((pane) => pane.webglAddon) + suspendPaneRendering(panesA, retentionFor(ownerA, panesA)) + + const ownerB = {} + const panesB = [createPane(), createPane(), createPane()] + suspendPaneRendering(panesB, retentionFor(ownerB, panesB)) + + // A(3) + B(3) fits the cap of 6; C(2) forces A out. + const ownerC = {} + const panesC = [createPane(), createPane()] + suspendPaneRendering(panesC, retentionFor(ownerC, panesC)) + + expect(panesA.every((pane) => pane.webglAddon === null)).toBe(true) + for (const addon of addonsA) { + expect(addon?.dispose).toHaveBeenCalled() + } + expect(panesB.every((pane) => pane.webglAddon != null)).toBe(true) + expect(panesC.every((pane) => pane.webglAddon != null)).toBe(true) + expect(retainedHiddenWebglOwnerCountForTest()).toBe(2) + }) + + it('re-suspending an owner refreshes its LRU position', () => { + const ownerA = {} + const panesA = [createPane(), createPane(), createPane()] + suspendPaneRendering(panesA, retentionFor(ownerA, panesA)) + const ownerB = {} + const panesB = [createPane(), createPane(), createPane()] + suspendPaneRendering(panesB, retentionFor(ownerB, panesB)) + + // Touch A again: B becomes the eviction candidate. + suspendPaneRendering(panesA, retentionFor(ownerA, panesA)) + const ownerC = {} + const panesC = [createPane()] + suspendPaneRendering(panesC, retentionFor(ownerC, panesC)) + + expect(panesB.every((pane) => pane.webglAddon === null)).toBe(true) + expect(panesA.every((pane) => pane.webglAddon != null)).toBe(true) + }) + + it('does not retain when the owner has no live addons', () => { + const owner = {} + const panes = [createPane(false)] + suspendPaneRendering(panes, retentionFor(owner, panes)) + expect(retainedHiddenWebglOwnerCountForTest()).toBe(0) + }) + + it('does not retain a single owner wider than the whole cap', () => { + const owner = {} + const panes = Array.from({ length: 7 }, () => createPane()) + const addons = panes.map((pane) => pane.webglAddon) + suspendPaneRendering(panes, retentionFor(owner, panes)) + expect(retainedHiddenWebglOwnerCountForTest()).toBe(0) + for (const addon of addons) { + expect(addon?.dispose).toHaveBeenCalled() + } + }) + + it('resume releases retention bookkeeping and keeps the live addon', () => { + const owner = {} + const panes = [createPane()] + suspendPaneRendering(panes, retentionFor(owner, panes)) + resumePaneRendering(panes, owner) + expect(retainedHiddenWebglOwnerCountForTest()).toBe(0) + expect(panes[0].webglAddon).not.toBeNull() + expect(panes[0].webglAttachmentDeferred).toBe(false) + // Released owner no longer counts toward the cap: three more 3-pane owners fit. + const others = [{}, {}] + for (const other of others) { + const otherPanes = [createPane(), createPane(), createPane()] + suspendPaneRendering(otherPanes, retentionFor(other, otherPanes)) + expect(otherPanes.every((pane) => pane.webglAddon != null)).toBe(true) + } + }) + + it('releaseHiddenWebglRetention never disposes addons (destroy path owns that)', () => { + const owner = {} + const panes = [createPane()] + expect(tryRetainHiddenPanesWebgl(owner, () => panes)).toBe(true) + releaseHiddenWebglRetention(owner) + expect(panes[0].webglAddon).not.toBeNull() + expect(retainedHiddenWebglOwnerCountForTest()).toBe(0) + }) + + it('eviction accounting survives addons disposed after retention (GPU toggle off)', () => { + const ownerA = {} + const panesA = [createPane(), createPane(), createPane(), createPane()] + suspendPaneRendering(panesA, retentionFor(ownerA, panesA)) + // GPU-off path disposes retained addons directly. + for (const pane of panesA) { + pane.webglAddon = null + } + const ownerB = {} + const panesB = Array.from({ length: 6 }, () => createPane()) + suspendPaneRendering(panesB, retentionFor(ownerB, panesB)) + expect(panesB.every((pane) => pane.webglAddon != null)).toBe(true) + }) +}) diff --git a/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.ts b/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.ts new file mode 100644 index 00000000000..e4ca8fa63b0 --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-webgl-hidden-retention.ts @@ -0,0 +1,81 @@ +import type { ManagedPaneInternal } from './pane-manager-types' +import { disposeWebgl } from './pane-webgl-renderer' + +// Orca raises Blink's active-context ceiling to 128, but retained contexts +// still consume GPU memory. Six keeps recent switch-backs on WebGL without +// letting hidden worktrees grow that cost with the mounted-pane population. +const MAX_RETAINED_HIDDEN_WEBGL_CONTEXTS = 6 + +type RetainedHiddenEntry = { + owner: object + livePanes: () => Iterable +} + +// LRU: oldest suspend first. +const retainedEntries: RetainedHiddenEntry[] = [] + +function liveContextCount(entry: RetainedHiddenEntry): number { + let count = 0 + for (const pane of entry.livePanes()) { + if (pane.webglAddon) { + count += 1 + } + } + return count +} + +function disposeEntryContexts(entry: RetainedHiddenEntry): void { + for (const pane of entry.livePanes()) { + disposeWebgl(pane) + } +} + +function removeEntry(owner: object): void { + const index = retainedEntries.findIndex((entry) => entry.owner === owner) + if (index !== -1) { + retainedEntries.splice(index, 1) + } +} + +/** + * Try to keep the owner's live WebGL addons across a hide. Returns true when + * retained — the caller must then skip its dispose pass. Evicts (disposes) + * least-recently-hidden owners to stay under the context cap. + */ +export function tryRetainHiddenPanesWebgl( + owner: object, + livePanes: () => Iterable +): boolean { + removeEntry(owner) + const entry: RetainedHiddenEntry = { owner, livePanes } + const ownCount = liveContextCount(entry) + // Nothing to retain (GPU off / first-mount hidden), or a single tab too wide + // for the cap — normal dispose keeps eviction from thrashing every other tab. + if (ownCount === 0 || ownCount > MAX_RETAINED_HIDDEN_WEBGL_CONTEXTS) { + return false + } + let total = ownCount + for (const other of retainedEntries) { + total += liveContextCount(other) + } + while (total > MAX_RETAINED_HIDDEN_WEBGL_CONTEXTS && retainedEntries.length > 0) { + const evicted = retainedEntries.shift()! + total -= liveContextCount(evicted) + disposeEntryContexts(evicted) + } + retainedEntries.push(entry) + return true +} + +/** Drop retention bookkeeping on reveal/destroy; never disposes live addons. */ +export function releaseHiddenWebglRetention(owner: object): void { + removeEntry(owner) +} + +export function retainedHiddenWebglOwnerCountForTest(): number { + return retainedEntries.length +} + +export function resetHiddenWebglRetentionForTest(): void { + retainedEntries.length = 0 +} From e6eec11b9fe2bdf9a2418065b9af995e07ea9882 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Tue, 11 Aug 2026 15:29:33 -0700 Subject: [PATCH 002/134] fix(wsl): preserve single-letter POSIX terminal cwd (#13859) --- ...n-foreground-confirmation-protocol.test.ts | 3 +- .../daemon/daemon-protocol-version.test.ts | 6 +- src/main/daemon/daemon-protocol-version.ts | 7 +- src/main/daemon/daemon-pty-router.test.ts | 9 +- src/main/daemon/pty-subprocess.test.ts | 55 +++-- src/main/ipc/pty.test.ts | 230 +++++++++++++----- src/main/ipc/pty.ts | 171 +++++++------ src/main/providers/local-pty-provider.test.ts | 57 +++-- src/main/providers/local-pty-provider.ts | 4 +- src/main/providers/pty-provider-contract.ts | 2 + src/main/providers/windows-shell-args.test.ts | 22 +- src/main/providers/windows-shell-args.ts | 8 +- .../local-build-compatibility-contract.json | 4 +- .../local-build-compatibility-contract.ts | 4 +- 14 files changed, 379 insertions(+), 203 deletions(-) diff --git a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts index 07f291a60ff..7ce08eaf83b 100644 --- a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts +++ b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts @@ -3,7 +3,7 @@ import { PREVIOUS_DAEMON_PROTOCOL_VERSIONS, PROTOCOL_VERSION } from './types' describe('foreground-confirmation daemon protocol', () => { it('rejects daemons from before the fresh-confirmation RPC', () => { - expect(PROTOCOL_VERSION).toBe(32) + expect(PROTOCOL_VERSION).toBe(33) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(19) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(22) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(23) @@ -15,5 +15,6 @@ describe('foreground-confirmation daemon protocol', () => { expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(29) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(30) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(31) + expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(32) }) }) diff --git a/src/main/daemon/daemon-protocol-version.test.ts b/src/main/daemon/daemon-protocol-version.test.ts index 53ad0c06341..d47eecc9722 100644 --- a/src/main/daemon/daemon-protocol-version.test.ts +++ b/src/main/daemon/daemon-protocol-version.test.ts @@ -8,6 +8,7 @@ import { MODE_2031_UNSUBSCRIBE_FACT_PROTOCOL_VERSION, SNAPSHOT_SERIALIZER_FIDELITY_DAEMON_PROTOCOL_VERSION, STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION, + WSL_POSIX_CWD_DAEMON_PROTOCOL_VERSION, PREVIOUS_DAEMON_PROTOCOL_VERSIONS, PROTOCOL_VERSION, supportsMode2031UnsubscribeFact @@ -15,7 +16,8 @@ import { describe('daemon protocol version', () => { it('ships bounded history transfer after the 2031-unsubscribe fact', () => { - expect(PROTOCOL_VERSION).toBe(32) + expect(PROTOCOL_VERSION).toBe(33) + expect(WSL_POSIX_CWD_DAEMON_PROTOCOL_VERSION).toBe(33) expect(SNAPSHOT_SERIALIZER_FIDELITY_DAEMON_PROTOCOL_VERSION).toBe(32) expect(STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION).toBe(31) expect(HISTORY_SEED_TRANSFER_PROTOCOL_VERSION).toBe(30) @@ -25,7 +27,7 @@ describe('daemon protocol version', () => { expect(AGENT_SESSION_CLAIM_DAEMON_PROTOCOL_VERSION).toBe(26) expect(AGENT_SESSION_CREATE_OPERATION_DAEMON_PROTOCOL_VERSION).toBe(26) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toEqual( - Array.from({ length: 31 }, (_, index) => index + 1) + Array.from({ length: 32 }, (_, index) => index + 1) ) }) diff --git a/src/main/daemon/daemon-protocol-version.ts b/src/main/daemon/daemon-protocol-version.ts index fee06b235f2..64c4d559670 100644 --- a/src/main/daemon/daemon-protocol-version.ts +++ b/src/main/daemon/daemon-protocol-version.ts @@ -1,6 +1,7 @@ // Why: daemons survive app updates, so wire behavior must be version-gated. -// v32 carries the corrected terminal snapshot serializer; older owners remain attachable. -export const PROTOCOL_VERSION = 32 +// v33 preserves selected-WSL POSIX cwd semantics; older owners remain attachable. +export const PROTOCOL_VERSION = 33 +export const WSL_POSIX_CWD_DAEMON_PROTOCOL_VERSION = 33 export const SNAPSHOT_SERIALIZER_FIDELITY_DAEMON_PROTOCOL_VERSION = 32 export const STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION = 31 export const HISTORY_SEED_TRANSFER_PROTOCOL_VERSION = 30 @@ -26,7 +27,7 @@ export const CLEAN_DISCONNECT_PROTOCOL_VERSION = 24 export const MODE_2031_UNSUBSCRIBE_FACT_PROTOCOL_VERSION = 29 export const PREVIOUS_DAEMON_PROTOCOL_VERSIONS = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, - 28, 29, 30, 31 + 28, 29, 30, 31, 32 ] as const export function supportsPtyStartupIngress(protocolVersion: number): boolean { diff --git a/src/main/daemon/daemon-pty-router.test.ts b/src/main/daemon/daemon-pty-router.test.ts index 026e8430c28..41cc8016169 100644 --- a/src/main/daemon/daemon-pty-router.test.ts +++ b/src/main/daemon/daemon-pty-router.test.ts @@ -408,7 +408,7 @@ describe('DaemonPtyRouter', () => { expect(current.confirmForegroundProcess).toHaveBeenCalledWith('current-session') }) - it('preserves v30/v31 session owners and routes new sessions to v32', async () => { + it('preserves older session owners and routes new sessions to v33', async () => { const current = createAdapter('current', [], undefined, PROTOCOL_VERSION) const legacyV30 = createAdapter( 'v30', @@ -422,22 +422,27 @@ describe('DaemonPtyRouter', () => { undefined, STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION ) - const router = new DaemonPtyRouter({ current, legacy: [legacyV30, legacyV31] }) + const legacyV32 = createAdapter('v32', ['v32-session'], undefined, 32) + const router = new DaemonPtyRouter({ current, legacy: [legacyV30, legacyV31, legacyV32] }) await router.discoverLegacySessions() await router.spawn({ sessionId: 'v30-session', cols: 80, rows: 24 }) await router.spawn({ sessionId: 'v31-session', cols: 80, rows: 24 }) + await router.spawn({ sessionId: 'v32-session', cols: 80, rows: 24 }) const fresh = await router.spawn({ cols: 80, rows: 24 }) router.write('v30-session', 'old-v30\n') router.write('v31-session', 'old-v31\n') + router.write('v32-session', 'old-v32\n') router.write(fresh.id, 'new\n') expect(legacyV30.spawn).toHaveBeenCalledWith({ sessionId: 'v30-session', cols: 80, rows: 24 }) expect(legacyV31.spawn).toHaveBeenCalledWith({ sessionId: 'v31-session', cols: 80, rows: 24 }) + expect(legacyV32.spawn).toHaveBeenCalledWith({ sessionId: 'v32-session', cols: 80, rows: 24 }) expect(current.spawn).toHaveBeenCalledWith({ cols: 80, rows: 24 }) expect(legacyV30.write).toHaveBeenCalledWith('v30-session', 'old-v30\n') expect(legacyV31.write).toHaveBeenCalledWith('v31-session', 'old-v31\n') + expect(legacyV32.write).toHaveBeenCalledWith('v32-session', 'old-v32\n') expect(current.write).toHaveBeenCalledWith(fresh.id, 'new\n') }) diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index 089595e85b7..5322bd6294b 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -2965,36 +2965,39 @@ describe('createPtySubprocess', () => { expect(spawnCall[2].env.WSLENV ?? '').not.toContain(POWERLEVEL10K_WIZARD_DISABLE_ENV) }) - it('keeps daemon WSL split panes in their distro when cwd is already POSIX', () => { - const proc = mockPtyProcess() - spawnMock.mockReturnValue(proc) - const platform = Object.getOwnPropertyDescriptor(process, 'platform') + it.each(['/home/jin/repo/subdir', '/a', '/c'])( + 'keeps daemon WSL split panes in their distro when cwd is POSIX (%s)', + (cwd) => { + const proc = mockPtyProcess() + spawnMock.mockReturnValue(proc) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') - Object.defineProperty(process, 'platform', { value: 'win32' }) + Object.defineProperty(process, 'platform', { value: 'win32' }) - try { - createPtySubprocess({ - sessionId: 'repo::\\\\wsl.localhost\\Ubuntu\\home\\jin\\repo@@deadbeef', - cols: 80, - rows: 24, - cwd: '/home/jin/repo/subdir', - shellOverride: 'wsl.exe' - }) - } finally { - if (platform) { - Object.defineProperty(process, 'platform', platform) + try { + createPtySubprocess({ + sessionId: 'repo::\\\\wsl.localhost\\Ubuntu\\home\\jin\\repo@@deadbeef', + cols: 80, + rows: 24, + cwd, + shellOverride: 'wsl.exe' + }) + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } } - } - expect(validateWorkingDirectoryMock).toHaveBeenCalledWith( - '\\\\wsl.localhost\\Ubuntu\\home\\jin\\repo\\subdir' - ) - expect(spawnMock).toHaveBeenCalledWith( - 'wsl.exe', - ['-d', 'Ubuntu', '--', 'sh', '-c', expect.stringContaining("cd '/home/jin/repo/subdir'")], - expect.objectContaining({ cwd: expect.any(String) }) - ) - }) + expect(validateWorkingDirectoryMock).toHaveBeenCalledWith( + `\\\\wsl.localhost\\Ubuntu${cwd.replaceAll('/', '\\')}` + ) + expect(spawnMock).toHaveBeenCalledWith( + 'wsl.exe', + ['-d', 'Ubuntu', '--', 'sh', '-c', expect.stringContaining(`cd '${cwd}'`)], + expect.objectContaining({ cwd: expect.any(String) }) + ) + } + ) // Why: node-pty's UnixTerminal.destroy() registers _socket.once('close', () => // this.kill('SIGHUP')), and the socket 'close' event can fire concurrently diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index d462541ed72..cb8908e5c8f 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -9261,13 +9261,26 @@ describe('registerPtyHandlers', () => { store.persistPtyBinding.mockClear() mainWindow.webContents.send.mockClear() + wslUncDirectoryExistsAsyncMock.mockClear() const mountArgs = { cols: 120, rows: 40, - cwd, + cwd: '/a', + cwdFallback: 'worktree', command: 'claude --resume provider-session', launchAgent: 'claude', worktreeId, + projectRuntime: { + status: 'resolved', + runtime: { + kind: 'wsl', + hostPlatform: 'wsl', + projectId: 'repo-1', + distro: 'Ubuntu-24.04', + reason: 'project-override', + cacheKey: 'repo-1:wsl' + } + }, tabId, leafId, env: { @@ -9300,6 +9313,7 @@ describe('registerPtyHandlers', () => { sessionId: 'pty-live-owner' }) expect(providerSpawn.mock.calls[1]?.[0].command).toBeUndefined() + expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled() expect(runtime.createPreAllocatedTerminalHandle).not.toHaveBeenCalled() expect(prepareClaudeAuth).not.toHaveBeenCalled() expect(runtime.registerPreAllocatedHandleForPty).not.toHaveBeenCalled() @@ -12180,51 +12194,60 @@ describe('registerPtyHandlers', () => { expect(result.startupCwdFallback).toEqual({ kind: 'worktree', cwd: worktreePath }) }) - it('keeps an existing POSIX startup cwd for the selected WSL runtime', async () => { - const originalPlatform = process.platform - const providerSpawn = vi.fn().mockResolvedValue({ id: 'pty-wsl-cwd' }) - Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - wslUncDirectoryExistsAsyncMock.mockResolvedValue(true) - statSyncMock.mockImplementation((target: string) => { - if (target === '/home/alice/repo') { - throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) - } - return { isDirectory: () => true, mode: 0o755 } - }) - - try { - installDaemonTestProvider({ spawn: providerSpawn }) - registerPtyHandlers(mainWindow as never) - await handlers.get('pty:spawn')!(null, { - cols: 80, - rows: 24, - cwd: '/home/alice/repo', - cwdFallback: 'worktree', - worktreeId: 'repo-1::C:\\Users\\alice\\repo', - projectRuntime: { - status: 'resolved', - runtime: { - kind: 'wsl', - hostPlatform: 'wsl', - projectId: 'repo-1', - distro: 'Ubuntu-24.04', - reason: 'project-override', - cacheKey: 'repo-1:wsl' - } + it.each(['/home/alice/repo', '/a', '/c'])( + 'keeps an existing POSIX startup cwd for the selected WSL runtime (%s)', + async (startupCwd) => { + const originalPlatform = process.platform + const providerSpawn = vi.fn().mockResolvedValue({ id: 'pty-wsl-cwd' }) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + wslUncDirectoryExistsAsyncMock.mockResolvedValue(true) + statSyncMock.mockImplementation((target: string) => { + if (target === startupCwd) { + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) } + return { isDirectory: () => true, mode: 0o755 } }) - expect(statSyncMock).not.toHaveBeenCalledWith('/home/alice/repo') - expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledWith( - '\\\\wsl.localhost\\Ubuntu-24.04\\home\\alice\\repo' - ) - expect(providerSpawn).toHaveBeenCalledWith( - expect.objectContaining({ cwd: '/home/alice/repo', shellOverride: 'wsl.exe' }) - ) - } finally { - Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + try { + installDaemonTestProvider({ spawn: providerSpawn }) + registerPtyHandlers(mainWindow as never) + await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: startupCwd, + cwdFallback: 'worktree', + worktreeId: 'repo-1::C:\\Users\\alice\\repo', + projectRuntime: { + status: 'resolved', + runtime: { + kind: 'wsl', + hostPlatform: 'wsl', + projectId: 'repo-1', + distro: 'Ubuntu-24.04', + reason: 'project-override', + cacheKey: 'repo-1:wsl' + } + } + }) + + const expectedValidationCwd = `\\\\wsl.localhost\\Ubuntu-24.04${startupCwd.replaceAll('/', '\\')}` + expect(statSyncMock).not.toHaveBeenCalledWith(startupCwd) + expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledTimes(1) + expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledWith(expectedValidationCwd) + expect(providerSpawn).toHaveBeenCalledWith( + expect.objectContaining({ + cwd: startupCwd, + shellOverride: 'wsl.exe' + }) + ) + expect(providerSpawn).toHaveBeenCalledWith( + expect.not.objectContaining({ prevalidatedCwd: expect.anything() }) + ) + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } } - }) + ) it('falls back when the selected WSL runtime reports a POSIX cwd missing', async () => { const originalPlatform = process.platform @@ -12239,7 +12262,7 @@ describe('registerPtyHandlers', () => { const result = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24, - cwd: '/home/alice/deleted', + cwd: '/a', cwdFallback: 'worktree', worktreeId: `repo-1::${worktreePath}`, projectRuntime: { @@ -12255,13 +12278,71 @@ describe('registerPtyHandlers', () => { } })) as { startupCwdFallback?: { kind: string; cwd: string } } + expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledTimes(1) expect(providerSpawn).toHaveBeenCalledWith(expect.objectContaining({ cwd: worktreePath })) + expect(providerSpawn).toHaveBeenCalledWith( + expect.not.objectContaining({ prevalidatedCwd: expect.anything() }) + ) expect(result.startupCwdFallback).toEqual({ kind: 'worktree', cwd: worktreePath }) } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) + it('does not probe or forward local WSL cwd evidence for SSH fallback spawns', async () => { + const sshSpawn = vi.fn(async () => ({ id: 'ssh-wsl-looking-cwd' })) + registerSshPtyProvider('ssh-1', { + spawn: sshSpawn, + write: vi.fn(), + resize: vi.fn(), + shutdown: vi.fn(), + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: vi.fn(async () => []), + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + registerPtyHandlers(mainWindow as never) + + await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/a', + cwdFallback: 'worktree', + connectionId: 'ssh-1', + worktreeId: 'repo-1::C:\\Users\\alice\\repo', + shellOverride: 'wsl.exe', + projectRuntime: { + status: 'resolved', + runtime: { + kind: 'wsl', + hostPlatform: 'wsl', + projectId: 'repo-1', + distro: 'Ubuntu-24.04', + reason: 'project-override', + cacheKey: 'repo-1:wsl' + } + } + }) + + expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled() + expect(sshSpawn).toHaveBeenCalledWith(expect.objectContaining({ cwd: '/a' })) + expect(sshSpawn).toHaveBeenCalledWith( + expect.not.objectContaining({ prevalidatedCwd: expect.anything() }) + ) + }) + it('preserves a POSIX WSL cwd when the distro probe is inconclusive', async () => { const originalPlatform = process.platform const providerSpawn = vi.fn().mockResolvedValue({ id: 'pty-wsl-inconclusive-cwd' }) @@ -12274,15 +12355,13 @@ describe('registerPtyHandlers', () => { const result = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24, - cwd: '/home/alice/repo', + cwd: '/a', cwdFallback: 'worktree', worktreeId: 'repo-1::C:/Users/alice/repo', shellOverride: 'wsl.exe' })) as { startupCwdFallback?: unknown } - expect(providerSpawn).toHaveBeenCalledWith( - expect.objectContaining({ cwd: '/home/alice/repo' }) - ) + expect(providerSpawn).toHaveBeenCalledWith(expect.objectContaining({ cwd: '/a' })) expect(result.startupCwdFallback).toBeUndefined() } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) @@ -12301,17 +12380,15 @@ describe('registerPtyHandlers', () => { const result = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24, - cwd: '/home/alice/repo', + cwd: '/c', cwdFallback: 'worktree', worktreeId: 'repo-1::C:/Users/alice/repo', shellOverride: 'wsl.exe' })) as { startupCwdFallback?: unknown } - expect(statSyncMock).not.toHaveBeenCalledWith('/home/alice/repo') + expect(statSyncMock).not.toHaveBeenCalledWith('/c') expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled() - expect(providerSpawn).toHaveBeenCalledWith( - expect.objectContaining({ cwd: '/home/alice/repo' }) - ) + expect(providerSpawn).toHaveBeenCalledWith(expect.objectContaining({ cwd: '/c' })) expect(result.startupCwdFallback).toBeUndefined() } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) @@ -12319,7 +12396,11 @@ describe('registerPtyHandlers', () => { }) it.each([ - { exists: true, expectedCwd: '/home/alice/repo', expectedFallback: undefined }, + { + exists: true, + expectedCwd: '/home/alice/repo', + expectedFallback: undefined + }, { exists: false, expectedCwd: '\\\\wsl.localhost\\Ubuntu\\home\\alice', @@ -12348,9 +12429,13 @@ describe('registerPtyHandlers', () => { worktreeId: 'repo-1::\\\\wsl.localhost\\Ubuntu\\home\\alice' })) as { startupCwdFallback?: { kind: string; cwd: string } } + expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledTimes(testCase.exists ? 1 : 2) expect(providerSpawn).toHaveBeenCalledWith( expect.objectContaining({ cwd: testCase.expectedCwd }) ) + expect(providerSpawn).toHaveBeenCalledWith( + expect.not.objectContaining({ prevalidatedCwd: expect.anything() }) + ) expect(result.startupCwdFallback).toEqual(testCase.expectedFallback) } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) @@ -12369,15 +12454,16 @@ describe('registerPtyHandlers', () => { const result = (await handlers.get('pty:spawn')!(null, { cols: 80, rows: 24, - cwd: '/home/alice/deleted', + cwd: '/c', cwdFallback: 'worktree', worktreeId: 'repo-1::\\\\wsl.localhost\\Ubuntu\\home\\alice' })) as { startupCwdFallback?: unknown } expect(wslUncDirectoryExistsAsyncMock).toHaveBeenCalledTimes(2) expect(providerSpawn).toHaveBeenCalledWith( - expect.objectContaining({ cwd: '/home/alice/deleted' }) + expect.not.objectContaining({ prevalidatedCwd: expect.anything() }) ) + expect(providerSpawn).toHaveBeenCalledWith(expect.objectContaining({ cwd: '/c' })) expect(result.startupCwdFallback).toBeUndefined() } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) @@ -12440,12 +12526,46 @@ describe('registerPtyHandlers', () => { expect(providerSpawn).toHaveBeenCalledWith( expect.objectContaining({ cwd: worktreePath, shellOverride: 'wsl.exe' }) ) + expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled() expect(result.startupCwdFallback).toEqual({ kind: 'worktree', cwd: worktreePath }) } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) } }) + it('probes Git Bash /c as its existing native drive root without falling back', async () => { + const originalPlatform = process.platform + const providerSpawn = vi.fn().mockResolvedValue({ id: 'pty-git-bash-cwd' }) + const worktreePath = 'C:/Users/alice/repo' + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + + try { + installDaemonTestProvider({ spawn: providerSpawn }) + registerPtyHandlers(mainWindow as never) + const result = (await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/c', + cwdFallback: 'worktree', + worktreeId: `repo-1::${worktreePath}`, + shellOverride: 'C:\\Program Files\\Git\\bin\\bash.exe' + })) as { startupCwdFallback?: { kind: string; cwd: string } } + + expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled() + expect(statSyncMock).toHaveBeenCalledWith('C:\\') + expect(statSyncMock).not.toHaveBeenCalledWith('/c') + expect(providerSpawn).toHaveBeenCalledWith( + expect.objectContaining({ + cwd: '/c', + shellOverride: expect.stringContaining('bash') + }) + ) + expect(result.startupCwdFallback).toBeUndefined() + } finally { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + } + }) + it('keeps a missing cwd unchanged without the fallback flag', async () => { registerPtyHandlers(mainWindow as never) existsSyncMock.mockImplementation((target: string) => target !== '/repo/app/deleted-folder') diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index b44abad7f37..e0c5fad3442 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -70,6 +70,7 @@ import { } from '../../shared/pi-agent-kind' import { isPwshAvailableAsync } from '../pwsh' import { LocalPtyProvider } from '../providers/local-pty-provider' +import { normalizeWindowsTerminalCwd } from '../providers/windows-shell-args' import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types' import { isPtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' import { @@ -5870,83 +5871,9 @@ export function registerPtyHandlers( if (startupPromise) { await startupPromise } - // Why: honor the fallback only for fresh local spawns — reattach needs exact cwd and SSH can't probe the local filesystem. - const requestedMissingCwdFallback = - !args.connectionId && !args.sessionId && args.cwdFallback === 'worktree' - const isWslOwnedPosixCwd = - args.cwd?.startsWith('/') === true && !/^\/[A-Za-z](?:\/|$)/.test(args.cwd) - const startupWorkspaceCwd = - requestedMissingCwdFallback && isWslOwnedPosixCwd - ? resolvePtySpawnStartupCwd(args.worktreeId, '.') - : undefined - const initiallyResolvedStartupCwd = - requestedMissingCwdFallback && isWslOwnedPosixCwd - ? resolvePtySpawnStartupCwd(args.worktreeId, args.cwd) - : undefined - const startupTerminalRuntimeOptions = - requestedMissingCwdFallback && process.platform === 'win32' - ? resolveLocalWindowsTerminalRuntimeOptions({ - requestedShellOverride: args.shellOverride, - settings: getSettings?.(), - projectRuntime: args.projectRuntime, - fallbackHostShell: process.env.COMSPEC || 'powershell.exe' - }) - : undefined - const wslRuntimeOwnsStartupCwd = - requestedMissingCwdFallback && - isWslOwnedPosixCwd && - (isWslShellName(startupTerminalRuntimeOptions?.shellOverride) || - isWslUncPath(startupWorkspaceCwd ?? '')) - const startupWslContext = wslRuntimeOwnsStartupCwd - ? resolveWslSessionContext({ - cwd: startupWorkspaceCwd, - shellOverride: startupTerminalRuntimeOptions?.shellOverride, - terminalWindowsWslDistro: startupTerminalRuntimeOptions?.terminalWindowsWslDistro - }) - : undefined - let wslStartupCwdExists: boolean | null = null - let wslWorkspaceCwdExists: boolean | null = null - if (startupWslContext && initiallyResolvedStartupCwd) { - const validationCwd = toWindowsWslPath( - initiallyResolvedStartupCwd, - startupWslContext.distro - ) - wslStartupCwdExists = isWslUncPath(validationCwd) - ? await wslUncDirectoryExistsAsync(validationCwd) - : localStartupCwdDirectoryExists(validationCwd) - if (wslStartupCwdExists === false && startupWorkspaceCwd) { - wslWorkspaceCwdExists = isWslUncPath(startupWorkspaceCwd) - ? await wslUncDirectoryExistsAsync(startupWorkspaceCwd) - : localStartupCwdDirectoryExists(startupWorkspaceCwd) - } - } - const allowMissingCwdFallback = - requestedMissingCwdFallback && - (!wslRuntimeOwnsStartupCwd || - (wslStartupCwdExists === false && wslWorkspaceCwdExists === true)) - let didFallbackToWorkspaceRootCwd = false - const cwd = resolvePtySpawnStartupCwd( - args.worktreeId, - args.cwd, - allowMissingCwdFallback - ? { - directoryExists: (path) => - startupWslContext && - wslStartupCwdExists === false && - path === initiallyResolvedStartupCwd - ? false - : startupWslContext && path === startupWorkspaceCwd - ? wslWorkspaceCwdExists === true - : localStartupCwdDirectoryExists(path), - onFallbackToWorkspaceRoot: () => { - didFallbackToWorkspaceRootCwd = true - } - } - : undefined - ) - const startupCwdFallback = - didFallbackToWorkspaceRootCwd && cwd ? ({ kind: 'worktree', cwd } as const) : undefined - spawnTiming.mark('preflight') + let cwd = resolvePtySpawnStartupCwd(args.worktreeId, args.cwd) + let prevalidatedCwd: string | undefined + let startupCwdFallback: { kind: 'worktree'; cwd: string } | undefined const earlyLeafId = typeof args.leafId === 'string' && isTerminalLeafId(args.leafId) ? args.leafId : null const earlyPaneKey = @@ -5997,6 +5924,7 @@ export function registerPtyHandlers( await assertFolderWorkspacePtyPathUsable(args.worktreeId) } const provider = getProvider(args.connectionId) + spawnTiming.mark('stable_adoption_setup') const preAdoptedStablePane = earlyStablePaneOwner && earlyWorktreeId ? await adoptStablePane({ @@ -6010,9 +5938,97 @@ export function registerPtyHandlers( ownsPaneSpawnReservation: true }) : null + spawnTiming.mark('stable_adoption') if (earlyStablePaneOwner && !preAdoptedStablePane) { await assertFolderWorkspacePtyPathUsable(args.worktreeId) } + if (!preAdoptedStablePane) { + // Why: reattach needs exact cwd, SSH cannot probe locally, and successful stable-pane adoption needs no launch preflight. + const requestedMissingCwdFallback = + !args.connectionId && !args.sessionId && args.cwdFallback === 'worktree' + const isPosixStartupCwd = args.cwd?.startsWith('/') === true + const startupWorkspaceCwd = + requestedMissingCwdFallback && isPosixStartupCwd + ? resolvePtySpawnStartupCwd(args.worktreeId, '.') + : undefined + const initiallyResolvedStartupCwd = + requestedMissingCwdFallback && isPosixStartupCwd ? cwd : undefined + const startupTerminalRuntimeOptions = + requestedMissingCwdFallback && process.platform === 'win32' + ? resolveLocalWindowsTerminalRuntimeOptions({ + requestedShellOverride: args.shellOverride, + settings: getSettings?.(), + projectRuntime: args.projectRuntime, + fallbackHostShell: process.env.COMSPEC || 'powershell.exe' + }) + : undefined + const wslRuntimeOwnsStartupCwd = + requestedMissingCwdFallback && + isPosixStartupCwd && + (isWslShellName(startupTerminalRuntimeOptions?.shellOverride) || + isWslUncPath(startupWorkspaceCwd ?? '')) + const startupWslContext = wslRuntimeOwnsStartupCwd + ? resolveWslSessionContext({ + cwd: startupWorkspaceCwd, + shellOverride: startupTerminalRuntimeOptions?.shellOverride, + terminalWindowsWslDistro: startupTerminalRuntimeOptions?.terminalWindowsWslDistro + }) + : undefined + let wslStartupCwdExists: boolean | null = null + let wslWorkspaceCwdExists: boolean | null = null + if (startupWslContext && initiallyResolvedStartupCwd) { + const validationCwd = toWindowsWslPath( + initiallyResolvedStartupCwd, + startupWslContext.distro + ) + wslStartupCwdExists = isWslUncPath(validationCwd) + ? await wslUncDirectoryExistsAsync(validationCwd) + : localStartupCwdDirectoryExists(validationCwd) + if (wslStartupCwdExists === true) { + prevalidatedCwd = validationCwd + } + if (wslStartupCwdExists === false && startupWorkspaceCwd) { + wslWorkspaceCwdExists = isWslUncPath(startupWorkspaceCwd) + ? await wslUncDirectoryExistsAsync(startupWorkspaceCwd) + : localStartupCwdDirectoryExists(startupWorkspaceCwd) + if (wslWorkspaceCwdExists === true) { + prevalidatedCwd = startupWorkspaceCwd + } + } + } + const allowMissingCwdFallback = + requestedMissingCwdFallback && + (!wslRuntimeOwnsStartupCwd || + (wslStartupCwdExists === false && wslWorkspaceCwdExists === true)) + let didFallbackToWorkspaceRootCwd = false + cwd = resolvePtySpawnStartupCwd( + args.worktreeId, + args.cwd, + allowMissingCwdFallback + ? { + directoryExists: (path) => + startupWslContext && + wslStartupCwdExists === false && + path === initiallyResolvedStartupCwd + ? false + : startupWslContext && path === startupWorkspaceCwd + ? wslWorkspaceCwdExists === true + : localStartupCwdDirectoryExists( + process.platform === 'win32' ? normalizeWindowsTerminalCwd(path) : path + ), + onFallbackToWorkspaceRoot: () => { + didFallbackToWorkspaceRootCwd = true + } + } + : undefined + ) + if (didFallbackToWorkspaceRootCwd && wslWorkspaceCwdExists === true && cwd) { + prevalidatedCwd = cwd + } + startupCwdFallback = + didFallbackToWorkspaceRootCwd && cwd ? { kind: 'worktree', cwd } : undefined + } + spawnTiming.mark('preflight') const freshSpawnRecovery = preAdoptedStablePane ? undefined : recoverFreshSpawnProviderRouting(provider, args.connectionId, args.sessionId) @@ -6351,6 +6367,7 @@ export function registerPtyHandlers( cols: args.cols, rows: args.rows, cwd, + ...(prevalidatedCwd && !isDaemonHostSpawn ? { prevalidatedCwd } : {}), env: spawnEnv, ...(isMintedSessionId ? { isNewSession: true } : {}) } diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index a7b675e78da..03e7127ce4c 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -14,7 +14,8 @@ const { resolveAgentForegroundProcessMock, readWindowsConptyProcessIdsMock, killWithDescendantSweepMock, - isWslAvailableAsyncMock + isWslAvailableAsyncMock, + wslUncDirectoryExistsMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), statSyncMock: vi.fn(), @@ -26,7 +27,8 @@ const { resolveAgentForegroundProcessMock: vi.fn(), readWindowsConptyProcessIdsMock: vi.fn(), killWithDescendantSweepMock: vi.fn(), - isWslAvailableAsyncMock: vi.fn() + isWslAvailableAsyncMock: vi.fn(), + wslUncDirectoryExistsMock: vi.fn() })) vi.mock('fs', () => ({ @@ -102,7 +104,7 @@ vi.mock('../wsl', () => ({ isWslAvailableAsync: () => isWslAvailableAsyncMock(), // Why: WSL worktree validation now asks the distro; these tests use WSL UNC // cwds that are meant to exist, so report them present without spawning wsl.exe. - wslUncDirectoryExists: () => true + wslUncDirectoryExists: (...args: unknown[]) => wslUncDirectoryExistsMock(...args) })) import { @@ -170,6 +172,8 @@ describe('LocalPtyProvider', () => { readWindowsConptyProcessIdsMock.mockResolvedValue(null) isWslAvailableAsyncMock.mockReset() isWslAvailableAsyncMock.mockResolvedValue(true) + wslUncDirectoryExistsMock.mockReset() + wslUncDirectoryExistsMock.mockReturnValue(true) exitCb = undefined mockProc = { @@ -978,29 +982,50 @@ describe('LocalPtyProvider', () => { expect(spawnCall[2].env.HISTFILE).toContain('terminal-history-wsl/Debian') }) - it('translates a POSIX cwd through the preferred WSL distro', async () => { + it.each(['/home/jin/repo', '/a', '/c'])( + 'preserves a POSIX cwd through the preferred WSL distro (%s)', + async (cwd) => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + + await provider.spawn({ + cols: 80, + rows: 24, + worktreeId: 'repo-1::C:\\Users\\jin\\repo', + cwd, + prevalidatedCwd: `\\\\wsl.localhost\\Debian${cwd.replaceAll('/', '\\')}`, + shellOverride: 'wsl.exe', + terminalWindowsWslDistro: 'Debian' + }) + + const spawnCall = spawnMock.mock.calls.at(-1)! + expect(spawnCall[0]).toBe('wsl.exe') + expect(spawnCall[1]).toEqual([ + '-d', + 'Debian', + '--', + 'sh', + '-c', + expect.stringContaining(`cd '${cwd}'`) + ]) + expect(spawnCall[2].cwd).not.toBe(cwd) + expect(wslUncDirectoryExistsMock).not.toHaveBeenCalled() + } + ) + + it('revalidates when cwd evidence does not exactly match the resolved WSL path', async () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) await provider.spawn({ cols: 80, rows: 24, worktreeId: 'repo-1::C:\\Users\\jin\\repo', - cwd: '/home/jin/repo', + cwd: '/a', + prevalidatedCwd: '\\\\wsl.localhost\\Debian\\c', shellOverride: 'wsl.exe', terminalWindowsWslDistro: 'Debian' }) - const spawnCall = spawnMock.mock.calls.at(-1)! - expect(spawnCall[0]).toBe('wsl.exe') - expect(spawnCall[1]).toEqual([ - '-d', - 'Debian', - '--', - 'sh', - '-c', - expect.stringContaining("cd '/home/jin/repo'") - ]) - expect(spawnCall[2].cwd).not.toBe('/home/jin/repo') + expect(wslUncDirectoryExistsMock).toHaveBeenCalledWith('\\\\wsl.localhost\\Debian\\a') }) it('resolves and persists the default distro for Windows cwd WSL terminals', async () => { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index d1ba44e5f2e..aafd04b9e1b 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -670,7 +670,9 @@ export class LocalPtyProvider implements IPtyProvider { } ensureNodePtySpawnHelperExecutable() - validateWorkingDirectory(validationCwd) + if (args.prevalidatedCwd !== validationCwd) { + validateWorkingDirectory(validationCwd) + } const spawnEnv: Record = { ...mergeGitConfigEnvProtocol(stripInheritedBuildModeEnv(process.env), args.env), diff --git a/src/main/providers/pty-provider-contract.ts b/src/main/providers/pty-provider-contract.ts index 398729efe17..e1017cab2c4 100644 --- a/src/main/providers/pty-provider-contract.ts +++ b/src/main/providers/pty-provider-contract.ts @@ -38,6 +38,8 @@ export type PtySpawnOptions = { cols: number rows: number cwd?: string + /** Exact per-spawn cwd already proven by main; providers validate any other resolved path. */ + prevalidatedCwd?: string env?: Record envToDelete?: string[] /** Main-validated home provenance for an automatic Codex session resume. */ diff --git a/src/main/providers/windows-shell-args.test.ts b/src/main/providers/windows-shell-args.test.ts index 53a83e6638d..c3bead6c5b2 100644 --- a/src/main/providers/windows-shell-args.test.ts +++ b/src/main/providers/windows-shell-args.test.ts @@ -219,7 +219,7 @@ describe('resolveWindowsShellLaunchArgs', () => { it('starts Git Bash as an interactive login shell with UTF-8 console setup', () => { const result = resolveWindowsShellLaunchArgs( 'C:\\Program Files\\Git\\bin\\bash.exe', - 'C:\\Users\\alice\\code', + '/c', 'C:\\Users\\alice' ) @@ -237,8 +237,8 @@ describe('resolveWindowsShellLaunchArgs', () => { // Must stay fail-open: `;` (not `&&`) so a missing chcp.com can't abort the // exec and kill the terminal on startup. expect(bashCommand).not.toContain('&&') - expect(result.effectiveCwd).toBe('C:\\Users\\alice\\code') - expect(result.validationCwd).toBe('C:\\Users\\alice\\code') + expect(result.effectiveCwd).toBe('C:\\') + expect(result.validationCwd).toBe('C:\\') }) it('does not apply Git Bash launch args to unrelated bash.exe paths', () => { @@ -307,17 +307,15 @@ describe('resolveWindowsShellLaunchArgs', () => { expect(result.validationCwd).toBe('C:\\Users\\alice\\project') }) - it('does not treat MSYS drive cwd as a WSL POSIX cwd', () => { - const result = resolveWindowsShellLaunchArgs( - 'wsl.exe', - '/c/Users/alice/project', - 'C:\\Users\\alice', - { distro: 'Ubuntu', treatPosixCwdAsWsl: true } - ) + it.each(['/a', '/c'])('keeps a selected WSL runtime single-letter cwd exact (%s)', (cwd) => { + const result = resolveWindowsShellLaunchArgs('wsl.exe', cwd, 'C:\\Users\\alice', { + distro: 'Ubuntu', + treatPosixCwdAsWsl: true + }) - expect(result.shellArgs).toEqual(expectedWslArgs('/mnt/c/Users/alice/project', 'Ubuntu')) + expect(result.shellArgs).toEqual(expectedWslArgs(cwd, 'Ubuntu')) expect(result.effectiveCwd).toBe('C:\\Users\\alice') - expect(result.validationCwd).toBe('C:\\Users\\alice\\project') + expect(result.validationCwd).toBe(`\\\\wsl.localhost\\Ubuntu\\${cwd.slice(1)}`) }) it('escapes single quotes when translating a WSL cwd', () => { diff --git a/src/main/providers/windows-shell-args.ts b/src/main/providers/windows-shell-args.ts index bde8a872efb..772ef8d9f05 100644 --- a/src/main/providers/windows-shell-args.ts +++ b/src/main/providers/windows-shell-args.ts @@ -131,7 +131,8 @@ function buildWslShellArgs(linuxCwd: string, distro?: string): string[] { return distro ? ['-d', distro, ...shellArgs] : shellArgs } -function normalizeMsysDrivePath(cwd: string): string { +/** Converts an MSYS drive spelling to the native cwd used by Windows terminal processes. */ +export function normalizeWindowsTerminalCwd(cwd: string): string { const match = cwd.match(/^\/([A-Za-z])(?:\/(.*))?$/) if (!match) { return cwd @@ -161,8 +162,7 @@ export function resolveWindowsShellLaunchArgs( startupCommand?: string ): WindowsShellLaunchArgs { const shellBasename = pathWin32.basename(shellPath).toLowerCase() - const nativeCwd = normalizeMsysDrivePath(cwd) - const isMsysDriveCwd = nativeCwd !== cwd + const nativeCwd = normalizeWindowsTerminalCwd(cwd) if (shellBasename === 'cmd.exe') { const shellArgStartupCommand = getCmdShellArgStartupCommand(startupCommand) @@ -210,7 +210,7 @@ export function resolveWindowsShellLaunchArgs( validationCwd: cwd } } - if (wslContext?.treatPosixCwdAsWsl && cwd.startsWith('/') && !isMsysDriveCwd) { + if (wslContext?.treatPosixCwdAsWsl && cwd.startsWith('/')) { return { shellArgs: buildWslShellArgs(cwd, wslContext.distro), effectiveCwd: defaultCwd, diff --git a/src/shared/local-build-compatibility-contract.json b/src/shared/local-build-compatibility-contract.json index d7b46db0959..705d1d92f99 100644 --- a/src/shared/local-build-compatibility-contract.json +++ b/src/shared/local-build-compatibility-contract.json @@ -3,9 +3,9 @@ "appId": "com.stablyai.orca", "stateSchemaVersion": 1, "readableStateSchemaVersions": [1], - "daemonProtocolVersion": 32, + "daemonProtocolVersion": 33, "attachableDaemonProtocolVersions": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, - 27, 28, 29, 30, 31, 32 + 27, 28, 29, 30, 31, 32, 33 ] } diff --git a/src/shared/local-build-compatibility-contract.ts b/src/shared/local-build-compatibility-contract.ts index 07bbacd3d3a..39556be77a9 100644 --- a/src/shared/local-build-compatibility-contract.ts +++ b/src/shared/local-build-compatibility-contract.ts @@ -3,9 +3,9 @@ export const LOCAL_BUILD_COMPATIBILITY_CONTRACT = { appId: 'com.stablyai.orca', stateSchemaVersion: 1, readableStateSchemaVersions: [1], - daemonProtocolVersion: 32, + daemonProtocolVersion: 33, attachableDaemonProtocolVersions: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, - 27, 28, 29, 30, 31, 32 + 27, 28, 29, 30, 31, 32, 33 ] } as const From 63271a59334a8f38c55d0ac2c76fac99e3eadbaf Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:32:13 -0700 Subject: [PATCH 003/134] feat(bitbucket): connect Bitbucket from Settings and create pull requests (#5832) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(bitbucket): connect Bitbucket from Settings with encrypted credential storage Bitbucket Cloud was the only review provider with no in-app auth: GitHub and GitLab delegate to the gh/glab CLIs, but Bitbucket has no comparable first-party CLI, so the only option was ORCA_BITBUCKET_* env vars plus a restart (discussion #5364). Adds a Connect/Edit/Disconnect flow on the Bitbucket integration card, modeled on Linear and Jira: - Credentials are verified against /user before they are persisted, so a dead token is rejected inline instead of silently stored. - The secret is encrypted with safeStorage (0600 plaintext fallback when no OS keyring); non-secret metadata lives in a separate plaintext file so status reads render the connected account without decrypting. Opening Settings therefore never triggers a keychain prompt. - Env vars keep precedence over stored credentials, so existing headless and SSH setups are unaffected. Env-managed connections hide Disconnect. - connect/disconnect reset the preflight cache, so no relaunch is needed. The Bitbucket card moves to its own file to stay under the tsx max-lines cap. * feat(bitbucket): support creating pull requests from Orca Bitbucket was the only configured provider whose Create button reported "This repository provider does not support creating a pull request from Orca" — supportsReviewCreation was false and the forge provider had no createReview, so even a correctly authenticated setup was blocked. Adds createBitbucketPullRequest against POST /repositories/{ws}/{repo}/ pullrequests, using the same env-first / stored-credential resolution as PR lookups (extracted into resolve-auth.ts so both share one path). Bitbucket Cloud has no draft pull requests, so a draft request is rejected with a clear message rather than silently publishing a live PR. * fix(bitbucket): hide the draft toggle where drafts do not exist, plus review fixes Bitbucket Cloud has no draft pull requests, so the composer no longer offers the toggle for it and forces the flag off at submit — better than failing after the user has filled the form in. Review fixes: - writeFileSync's `mode` only applies when it creates the file, so rewriting a credential kept whatever permissions it already had. chmod after every write, for the secret and the metadata. - An explicit ORCA_BITBUCKET_API_BASE_URL now wins over a stored base URL. Env precedence is per-setting, not all-or-nothing. - Enter in the credentials dialog only submits from a text field, so it no longer hijacks Cancel and the docs link. - Replace the chmod-based delete-failure test with a mocked unlinkSync: file modes are not portable to Windows and elevated runners unlink anyway. * fix(bitbucket): stop a merged pull request from blocking the branch's next one Reported on #5832: with a merged PR on a branch, Create reported "Pull request already exists" and offered no way forward. The branch lookup queries every PR state and returns the most recently updated one, so a merged PR came back as the branch's current review and eligibility blocked on it. Bitbucket only discarded such a match on the repo default branch (#9171), while GitHub already drops any merged PR it matched by branch alone — "a merged PR without an explicit link is just a historical branch match, not implicit review context". Applies that rule to Bitbucket. An explicitly linked review still resolves through the linked-number fallback, so merging a PR Orca knows about keeps showing it. * fix(bitbucket): add bitbucket to the shared review-creation provider list Reported on #5832: on a Bitbucket repo with no existing PR, Create still said "This repository provider does not support creating a pull request from Orca", even after the forge provider gained createReview. There are two capability lists. Enabling supportsReviewCreation on the forge provider was necessary but not sufficient — the blocker and the whole renderer read the separate shared list, which never included bitbucket. Adds it, gives Bitbucket its own provider name so review copy stops saying "GitHub", and asserts the two lists agree so they cannot drift apart again. * fix(bitbucket): persist pull request links after creation * fix(bitbucket): fetch linked pull requests by number first * fix(i18n): use generated Bitbucket integration keys * test(bitbucket): cover forge creation delegation * fix(bitbucket): fall back when linked pull request is stale * docs(bitbucket): explain notFoundIsNull and fix a garbled permissions comment notFoundIsNull arrived without the rationale its sibling flag carries, and reads as a bare `true` at the only call site that opts in. * fix(bitbucket): address review findings before merge Two of these made the feature unusable in real setups: - Create PR checked GitHub authentication for Bitbucket. isProviderAuthenticated fell through to isGitHubAuthenticated, which was unreachable while Bitbucket could not create reviews at all. Anyone with Bitbucket connected but no `gh auth login` got auth_required with no way forward. - The draft flag was only gated in ChecksPanel, not the two SourceControl call sites. With "create as draft" saved as a default, the composer hides the toggle for Bitbucket, so the flag could not be cleared and creation failed every time. Bitbucket now ignores draft instead of rejecting it. Also: - Blocked-create copy said "GitHub is not authenticated. Run gh auth login" on Bitbucket repos, in both the main-process and renderer paths. - A decryption failure resolved to an anonymous config and queried anyway; a private repo answers 404, which reads as "no pull request" and offers Create for a branch that already has one. Requests now fail closed. - Hiding non-open implicit branch matches was too broad: a declined PR became permanently invisible off the default branch. Scoped to merged, restoring the default-branch rule (#9171) for the rest. - A failed disconnect rejected unhandled and the card silently re-rendered as connected; a partial delete left the secret live in memory for the session. - The credentials dialog refused to open on a remote runtime, so a local repo could never store a credential. Now only the storage note changes, matching the Jira dialog. --------- Co-authored-by: devatnull <59279509+devatnull@users.noreply.github.com> --- src/main/bitbucket/bitbucket-auth-config.ts | 42 ++ src/main/bitbucket/client.test.ts | 99 +++++ src/main/bitbucket/client.ts | 156 +++---- .../bitbucket/credential-connection.test.ts | 163 ++++++++ src/main/bitbucket/credential-connection.ts | 110 +++++ src/main/bitbucket/credential-store.test.ts | 218 ++++++++++ src/main/bitbucket/credential-store.ts | 202 +++++++++ .../bitbucket/pull-request-creation.test.ts | 180 ++++++++ src/main/bitbucket/pull-request-creation.ts | 221 ++++++++++ src/main/bitbucket/resolve-auth.ts | 48 +++ src/main/bitbucket/status-no-decrypt.test.ts | 127 ++++++ src/main/bitbucket/user-request.ts | 39 ++ src/main/global-fetch-call-site-audit.test.ts | 1 + src/main/integration-credential-file.ts | 32 +- src/main/ipc/bitbucket.ts | 59 +++ src/main/ipc/register-core-handlers.test.ts | 8 + src/main/ipc/register-core-handlers.ts | 2 + .../source-control/forge-provider.test.ts | 38 +- src/main/source-control/forge-provider.ts | 6 +- .../source-control/hosted-review-creation.ts | 14 + src/preload/api-types.ts | 11 + src/preload/index.ts | 15 + .../components/right-sidebar/ChecksPanel.tsx | 34 +- .../CreateHostedReviewComposer.tsx | 11 +- .../CreateHostedReviewComposerFields.tsx | 42 +- .../right-sidebar/SourceControl.tsx | 25 +- ...ce-control-create-review-blocked-action.ts | 3 + ...source-control-created-review-link.test.ts | 25 ++ .../source-control-created-review-link.ts | 45 ++ ...view-creation-eligibility-snapshot.test.ts | 4 +- .../settings/bitbucket-credentials-dialog.tsx | 387 ++++++++++++++++++ .../settings/bitbucket-integration-card.tsx | 273 ++++++++++++ .../settings/integrations-search.ts | 20 +- .../source-control-integration-cards.tsx | 2 +- ...token-source-control-integration-cards.tsx | 108 ----- .../src/i18n/hosted-review-localized-copy.ts | 8 + src/renderer/src/i18n/locales/en.json | 49 ++- src/shared/bitbucket-credentials.ts | 28 ++ .../hosted-review-creation-providers.test.ts | 24 ++ .../hosted-review-creation-providers.ts | 8 +- src/shared/hosted-review.ts | 6 + src/shared/integration-credential-errors.ts | 5 +- 42 files changed, 2650 insertions(+), 248 deletions(-) create mode 100644 src/main/bitbucket/bitbucket-auth-config.ts create mode 100644 src/main/bitbucket/credential-connection.test.ts create mode 100644 src/main/bitbucket/credential-connection.ts create mode 100644 src/main/bitbucket/credential-store.test.ts create mode 100644 src/main/bitbucket/credential-store.ts create mode 100644 src/main/bitbucket/pull-request-creation.test.ts create mode 100644 src/main/bitbucket/pull-request-creation.ts create mode 100644 src/main/bitbucket/resolve-auth.ts create mode 100644 src/main/bitbucket/status-no-decrypt.test.ts create mode 100644 src/main/bitbucket/user-request.ts create mode 100644 src/main/ipc/bitbucket.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-created-review-link.test.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-created-review-link.ts create mode 100644 src/renderer/src/components/settings/bitbucket-credentials-dialog.tsx create mode 100644 src/renderer/src/components/settings/bitbucket-integration-card.tsx create mode 100644 src/shared/bitbucket-credentials.ts create mode 100644 src/shared/hosted-review-creation-providers.test.ts diff --git a/src/main/bitbucket/bitbucket-auth-config.ts b/src/main/bitbucket/bitbucket-auth-config.ts new file mode 100644 index 00000000000..83a4c7cf49d --- /dev/null +++ b/src/main/bitbucket/bitbucket-auth-config.ts @@ -0,0 +1,42 @@ +import { Buffer } from 'node:buffer' + +export const DEFAULT_API_BASE_URL = 'https://api.bitbucket.org/2.0' + +// Two interchangeable Bitbucket Cloud auth shapes: a single access token (Bearer) +// or an email + API token pair (HTTP Basic). Either satisfies `hasAuth`, and both +// may come from env vars or the in-app credential store. +export type BitbucketAuthConfig = { + baseUrl: string + accessToken: string | null + email: string | null + apiToken: string | null +} + +export function envValue(name: string): string | null { + const value = process.env[name]?.trim() ?? '' + return value.length > 0 ? value : null +} + +export function getEnvAuthConfig(): BitbucketAuthConfig { + return { + baseUrl: envValue('ORCA_BITBUCKET_API_BASE_URL') ?? DEFAULT_API_BASE_URL, + accessToken: envValue('ORCA_BITBUCKET_ACCESS_TOKEN'), + email: envValue('ORCA_BITBUCKET_EMAIL'), + apiToken: envValue('ORCA_BITBUCKET_API_TOKEN') + } +} + +export function hasAuth(config: BitbucketAuthConfig): boolean { + return Boolean(config.accessToken || (config.email && config.apiToken)) +} + +export function authHeaders(config: BitbucketAuthConfig): Record { + if (config.accessToken) { + return { Authorization: `Bearer ${config.accessToken}` } + } + if (config.email && config.apiToken) { + const encoded = Buffer.from(`${config.email}:${config.apiToken}`).toString('base64') + return { Authorization: `Basic ${encoded}` } + } + return {} +} diff --git a/src/main/bitbucket/client.test.ts b/src/main/bitbucket/client.test.ts index dd55bfad6d2..3b844725353 100644 --- a/src/main/bitbucket/client.test.ts +++ b/src/main/bitbucket/client.test.ts @@ -124,6 +124,105 @@ describe('Bitbucket client', () => { ).resolves.toMatchObject({ number: 42 }) }) + it('hides a MERGED PR matched only by feature-branch name so a new PR can be created', async () => { + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + return Response.json({ + values: [ + { ...bitbucketPr(7), state: 'MERGED', source: { branch: { name: 'feature/login' } } } + ] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + // Why: a merged branch match is history. Returning it made eligibility + // report "a pull request already exists" and blocked the branch's next PR. + await expect( + getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/login') + ).resolves.toBeNull() + }) + + it('keeps a DECLINED PR on a feature branch visible', async () => { + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + return Response.json({ + values: [ + { ...bitbucketPr(9), state: 'DECLINED', source: { branch: { name: 'feature/login' } } } + ] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + // Why: only merged matches are hidden. Hiding declined too made a declined + // PR permanently invisible off the default branch, unlike every other provider. + await expect( + getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/login') + ).resolves.toMatchObject({ number: 9, state: 'closed' }) + }) + + it('returns null instead of querying anonymously when no credential resolves', async () => { + delete process.env.ORCA_BITBUCKET_EMAIL + delete process.env.ORCA_BITBUCKET_API_TOKEN + const fetchMock = vi.fn() + vi.stubGlobal('fetch', fetchMock) + + // Why: an unauthenticated query 404s on private repos, which would read as + // "no pull request" and offer Create for a branch that already has one. + await expect( + getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/login') + ).resolves.toBeNull() + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('fetches an explicitly linked DECLINED PR before the branch index', async () => { + const fetchMock = vi.fn(async (url: string) => { + if (url.endsWith('/pullrequests/7')) { + return Response.json({ ...bitbucketPr(7), state: 'DECLINED' }) + } + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + return new Response('branch index unavailable', { status: 503 }) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + getBitbucketPullRequestForBranchOrThrow('/repo', 'refs/heads/feature/login', 7) + ).resolves.toMatchObject({ number: 7, state: 'closed' }) + expect(fetchMock.mock.calls.map(([url]) => url)).not.toContainEqual( + expect.stringContaining('/pullrequests?') + ) + }) + + it('falls back to the branch index when a linked PR number is stale', async () => { + const branchPR = bitbucketPr(7) + const fetchMock = vi.fn(async (url: string) => { + if (url.endsWith('/pullrequests/99')) { + return new Response('not found', { status: 404 }) + } + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + return Response.json({ + values: [{ ...branchPR, source: { ...branchPR.source, branch: { name: 'feature/login' } } }] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + getBitbucketPullRequestForBranchOrThrow('/repo', 'refs/heads/feature/login', 99) + ).resolves.toMatchObject({ number: 7, state: 'open' }) + expect(fetchMock.mock.calls.map(([url]) => url)).toEqual([ + expect.stringContaining('/pullrequests/99'), + expect.stringContaining('/pullrequests?'), + expect.stringContaining('/statuses/build') + ]) + }) + it('fetches a branch pull request and commit build status', async () => { const fetchMock = vi.fn(async (url: string, _init?: RequestInit) => { if (url.includes('/statuses/build')) { diff --git a/src/main/bitbucket/client.ts b/src/main/bitbucket/client.ts index e5163bd525a..34e4fe2a4fe 100644 --- a/src/main/bitbucket/client.ts +++ b/src/main/bitbucket/client.ts @@ -1,4 +1,3 @@ -import { Buffer } from 'node:buffer' import type { CheckStatus } from '../../shared/types' import { deriveBitbucketBuildStatus, @@ -8,25 +7,26 @@ import { type RawBitbucketBuildStatus, type RawBitbucketPullRequest } from './pull-request-mappers' -import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { getBitbucketRepoRef, type BitbucketRepoRef } from './repository-ref' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { getHostedReviewLocalGitOptions, type HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' import { cancelUnreadResponseBody } from '../lib/unread-response-body' +import { authHeaders, getEnvAuthConfig, hasAuth } from './bitbucket-auth-config' +import { accountNameFromUser, fetchBitbucketUser } from './user-request' +import { + getStoredBitbucketCredentialError, + getStoredBitbucketMetadata, + hasStoredBitbucketCredential, + loadStoredBitbucketSecret +} from './credential-store' +import { resolveBitbucketAuthConfig, storedAuthConfig } from './resolve-auth' -const DEFAULT_API_BASE_URL = 'https://api.bitbucket.org/2.0' const REQUEST_TIMEOUT_MS = 5000 const ALL_PULL_REQUEST_STATES = ['OPEN', 'MERGED', 'DECLINED', 'SUPERSEDED'] as const -type BitbucketAuthConfig = { - baseUrl: string - accessToken: string | null - email: string | null - apiToken: string | null -} - export type BitbucketAuthStatus = { configured: boolean authenticated: boolean @@ -38,42 +38,16 @@ type RequestOptions = { timeoutMs?: number } -function envValue(name: string): string | null { - const value = process.env[name]?.trim() ?? '' - return value.length > 0 ? value : null -} - -function getAuthConfig(): BitbucketAuthConfig { - return { - baseUrl: envValue('ORCA_BITBUCKET_API_BASE_URL') ?? DEFAULT_API_BASE_URL, - accessToken: envValue('ORCA_BITBUCKET_ACCESS_TOKEN'), - email: envValue('ORCA_BITBUCKET_EMAIL'), - apiToken: envValue('ORCA_BITBUCKET_API_TOKEN') - } -} - -function hasAuth(config: BitbucketAuthConfig): boolean { - return Boolean(config.accessToken || (config.email && config.apiToken)) -} - -function authHeaders(config: BitbucketAuthConfig): Record { - if (config.accessToken) { - return { Authorization: `Bearer ${config.accessToken}` } - } - if (config.email && config.apiToken) { - const encoded = Buffer.from(`${config.email}:${config.apiToken}`).toString('base64') - return { Authorization: `Basic ${encoded}` } - } - return {} -} - function isStringArray(value: string | readonly string[]): value is readonly string[] { return Array.isArray(value) } -function apiUrl(path: string, searchParams?: RequestOptions['searchParams']): string { - const config = getAuthConfig() - const base = config.baseUrl.replace(/\/+$/, '') +function apiUrl( + baseUrl: string, + path: string, + searchParams?: RequestOptions['searchParams'] +): string { + const base = baseUrl.replace(/\/+$/, '') const url = new URL(`${base}${path}`) if (searchParams) { for (const [key, value] of Object.entries(searchParams)) { @@ -95,11 +69,24 @@ async function requestJson( // Why: the existing-review lookup behind Create must distinguish a real // transport/auth failure from an accepted "no PR". When true, a failed request // throws instead of collapsing to null so callers never report false not_found. - throwOnFailure = false + throwOnFailure = false, + // Why: a linked PR number can be stale (deleted PR, wrong repo). A 404 there + // must fall through to the branch lookup rather than throw and hide the + // branch's real review, so only that caller opts into it. + notFoundIsNull = false ): Promise { - const config = getAuthConfig() + const config = resolveBitbucketAuthConfig() + // Why: a denied keychain prompt leaves no usable credential. Issuing the + // request anyway gets a 404 on private repos, which reads as "no pull + // request" and offers Create for a branch that already has one. + if (!hasAuth(config)) { + if (throwOnFailure) { + throw new Error('Bitbucket request failed: no usable credential') + } + return null + } try { - const response = await fetch(apiUrl(path, options.searchParams), { + const response = await fetch(apiUrl(config.baseUrl, path, options.searchParams), { headers: { Accept: 'application/json', ...authHeaders(config) @@ -108,6 +95,9 @@ async function requestJson( }) if (!response.ok) { await cancelUnreadResponseBody(response) + if (response.status === 404 && notFoundIsNull) { + return null + } if (throwOnFailure) { throw new Error(`Bitbucket request failed: HTTP ${response.status}`) } @@ -157,21 +147,37 @@ async function normalizePullRequest( return mapBitbucketPullRequest(raw, status) } +// Never decrypts. Env credentials are checked live; a stored credential is +// revalidated only when its secret already sits in memory from an earlier API +// call, and otherwise trusted from plaintext metadata — decrypting here would +// prompt for keychain access every time Settings opens. export async function getBitbucketAuthStatus(): Promise { - const config = getAuthConfig() - if (!hasAuth(config)) { - return { configured: false, authenticated: false, account: null } + const env = getEnvAuthConfig() + if (hasAuth(env)) { + const user = await fetchBitbucketUser(env) + return { + configured: true, + authenticated: user !== null, + account: accountNameFromUser(user) + } } - const user = await requestJson<{ - username?: string | null - display_name?: string | null - account_id?: string | null - }>('/user', { timeoutMs: 4000 }) - return { - configured: true, - authenticated: user !== null, - account: user?.username ?? user?.display_name ?? user?.account_id ?? null + const metadata = getStoredBitbucketMetadata() + if (metadata && hasStoredBitbucketCredential()) { + if (getStoredBitbucketCredentialError()) { + return { configured: true, authenticated: false, account: metadata.account } + } + const cached = loadStoredBitbucketSecret() + if (!cached) { + return { configured: true, authenticated: true, account: metadata.account } + } + const user = await fetchBitbucketUser(storedAuthConfig(metadata, cached)) + return { + configured: true, + authenticated: user !== null, + account: accountNameFromUser(user) ?? metadata.account + } } + return { configured: false, authenticated: false, account: null } } export async function getBitbucketPullRequest( @@ -216,6 +222,18 @@ export async function getBitbucketPullRequestForBranch( return null } + if (typeof linkedPRNumber === 'number') { + const raw = await requestJson( + `/repositories/${encodedRepoPath(repo)}/pullrequests/${encodeURIComponent(String(linkedPRNumber))}`, + {}, + throwOnFailure, + true + ) + if (raw) { + return normalizePullRequest(repo, raw) + } + } + if (branchName) { const query = [ `source.branch.name = "${escapeBitbucketQueryString(branchName)}"`, @@ -235,10 +253,16 @@ export async function getBitbucketPullRequestForBranch( ) const raw = list?.values?.[0] if (raw) { - // Why (#9171): discard a non-open implicit branch match on the repo - // default branch and fall through to the linked-number fallback below. + const state = mapBitbucketPullRequestState(raw.state) + // Why: a merged PR we only matched by branch name is history, not review + // context (GitHub's isMergedImplicitPR rule). Keeping it reported "a pull + // request already exists" and blocked the branch's next PR. Scoped to + // merged so a declined PR stays visible off the default branch, matching + // every other provider; the linked lookup above already returned early + // for an explicitly linked review. + const isMergedImplicitMatch = state === 'merged' const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ - state: mapBitbucketPullRequestState(raw.state), + state, reviewNumber: raw.id ?? null, linkedReviewNumber: linkedPRNumber, branchName, @@ -246,21 +270,13 @@ export async function getBitbucketPullRequestForBranch( connectionId, localGitOptions: getHostedReviewLocalGitOptions(options) }) - if (!hideOnDefaultBranch) { + if (!isMergedImplicitMatch && !hideOnDefaultBranch) { return normalizePullRequest(repo, raw) } } } - if (typeof linkedPRNumber !== 'number') { - return null - } - const raw = await requestJson( - `/repositories/${encodedRepoPath(repo)}/pullrequests/${encodeURIComponent(String(linkedPRNumber))}`, - {}, - throwOnFailure - ) - return raw ? normalizePullRequest(repo, raw) : null + return null } /** diff --git a/src/main/bitbucket/credential-connection.test.ts b/src/main/bitbucket/credential-connection.test.ts new file mode 100644 index 00000000000..25c51e81f87 --- /dev/null +++ b/src/main/bitbucket/credential-connection.test.ts @@ -0,0 +1,163 @@ +import { mkdtempSync } from 'node:fs' +import { tmpdir } from 'node:os' +import type * as Os from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const OLD_ENV = process.env +const OLD_FETCH = globalThis.fetch +let tempHome = '' + +async function loadModule() { + vi.resetModules() + vi.doMock('electron', () => ({ + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (value: string) => Buffer.from(value), + decryptString: (value: Buffer) => value.toString('utf-8') + } + })) + vi.doMock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { ...actual, homedir: () => tempHome } + }) + return import('./credential-connection') +} + +beforeEach(() => { + process.env = { ...OLD_ENV } + for (const key of [ + 'ORCA_BITBUCKET_ACCESS_TOKEN', + 'ORCA_BITBUCKET_EMAIL', + 'ORCA_BITBUCKET_API_TOKEN', + 'ORCA_BITBUCKET_API_BASE_URL' + ]) { + delete process.env[key] + } + tempHome = mkdtempSync(join(tmpdir(), 'orca-bitbucket-conn-')) +}) + +afterEach(() => { + process.env = OLD_ENV + globalThis.fetch = OLD_FETCH +}) + +describe('Bitbucket credential connection', () => { + it('verifies credentials before saving and reports a stored connection', async () => { + const conn = await loadModule() + globalThis.fetch = vi.fn(async () => + Response.json({ username: 'ada' }) + ) as unknown as typeof fetch + + await expect( + conn.connectBitbucket({ + authMode: 'basic', + email: 'ada@example.com', + apiToken: 'tok' + }) + ).resolves.toEqual({ ok: true, account: 'ada' }) + + expect(conn.getBitbucketConnectionStatus()).toEqual({ + configured: true, + source: 'stored', + account: 'ada', + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null + }) + }) + + it('sends Basic auth built from the entered email and API token', async () => { + const conn = await loadModule() + const fetchSpy = vi.fn(async () => Response.json({ username: 'ada' })) + globalThis.fetch = fetchSpy as unknown as typeof fetch + + await conn.connectBitbucket({ authMode: 'basic', email: 'ada@example.com', apiToken: 'tok' }) + + const [url, init] = fetchSpy.mock.calls[0] as unknown as [string, RequestInit] + expect(url).toBe('https://api.bitbucket.org/2.0/user') + expect((init.headers as Record).Authorization).toBe( + `Basic ${Buffer.from('ada@example.com:tok').toString('base64')}` + ) + }) + + it('rejects credentials that fail the /user check without saving them', async () => { + const conn = await loadModule() + globalThis.fetch = vi.fn( + async () => new Response(null, { status: 401 }) + ) as unknown as typeof fetch + + const result = await conn.connectBitbucket({ + authMode: 'token', + accessToken: 'bad' + }) + expect(result.ok).toBe(false) + expect(conn.getBitbucketConnectionStatus().source).toBe('none') + }) + + it('rejects an incomplete basic-auth credential before making a request', async () => { + const conn = await loadModule() + const fetchSpy = vi.fn() + globalThis.fetch = fetchSpy as unknown as typeof fetch + + const result = await conn.connectBitbucket({ + authMode: 'basic', + email: 'ada@example.com' + }) + expect(result.ok).toBe(false) + expect(fetchSpy).not.toHaveBeenCalled() + }) + + it('reports environment variables as the source and takes precedence over stored creds', async () => { + const conn = await loadModule() + globalThis.fetch = vi.fn(async () => + Response.json({ username: 'ada' }) + ) as unknown as typeof fetch + await conn.connectBitbucket({ + authMode: 'basic', + email: 'ada@example.com', + apiToken: 'tok' + }) + + process.env.ORCA_BITBUCKET_ACCESS_TOKEN = 'env-token' + expect(conn.getBitbucketConnectionStatus()).toMatchObject({ + configured: true, + source: 'environment', + authMode: 'token' + }) + }) + + it('lets an env API base URL override the stored one without env credentials', async () => { + const conn = await loadModule() + globalThis.fetch = vi.fn(async () => + Response.json({ username: 'ada' }) + ) as unknown as typeof fetch + await conn.connectBitbucket({ + authMode: 'basic', + email: 'ada@example.com', + apiToken: 'tok', + baseUrl: 'https://stored.example.com/2.0' + }) + + // Only the base URL is in the env, so `hasAuth(env)` is false — precedence + // is per-setting, not all-or-nothing. + process.env.ORCA_BITBUCKET_API_BASE_URL = 'https://env.example.com/2.0' + const { resolveBitbucketAuthConfig } = await import('./resolve-auth') + expect(resolveBitbucketAuthConfig().baseUrl).toBe('https://env.example.com/2.0') + }) + + it('clears the stored connection on disconnect', async () => { + const conn = await loadModule() + globalThis.fetch = vi.fn(async () => + Response.json({ username: 'ada' }) + ) as unknown as typeof fetch + await conn.connectBitbucket({ + authMode: 'basic', + email: 'ada@example.com', + apiToken: 'tok' + }) + + conn.disconnectBitbucket() + expect(conn.getBitbucketConnectionStatus().source).toBe('none') + }) +}) diff --git a/src/main/bitbucket/credential-connection.ts b/src/main/bitbucket/credential-connection.ts new file mode 100644 index 00000000000..821af4a4d2f --- /dev/null +++ b/src/main/bitbucket/credential-connection.ts @@ -0,0 +1,110 @@ +import { + DEFAULT_API_BASE_URL, + envValue, + getEnvAuthConfig, + hasAuth, + type BitbucketAuthConfig +} from './bitbucket-auth-config' +import { accountNameFromUser, fetchBitbucketUser } from './user-request' +import { + clearStoredBitbucketCredential, + getStoredBitbucketMetadata, + hasStoredBitbucketCredential, + saveBitbucketCredential +} from './credential-store' +import type { + BitbucketConnectArgs, + BitbucketConnectResult, + BitbucketConnectionStatus +} from '../../shared/bitbucket-credentials' + +export type { BitbucketConnectArgs, BitbucketConnectResult, BitbucketConnectionStatus } + +const VERIFY_TIMEOUT_MS = 6000 + +function normalize(value: string | null | undefined): string | null { + const trimmed = value?.trim() ?? '' + return trimmed.length > 0 ? trimmed : null +} + +function buildCandidateConfig(input: BitbucketConnectArgs): BitbucketAuthConfig { + return { + baseUrl: normalize(input.baseUrl) ?? DEFAULT_API_BASE_URL, + accessToken: input.authMode === 'token' ? normalize(input.accessToken) : null, + email: input.authMode === 'basic' ? normalize(input.email) : null, + apiToken: input.authMode === 'basic' ? normalize(input.apiToken) : null + } +} + +// Verifying against `/user` before persisting keeps the stored "connected +// account" honest and lets the dialog reject a dead token inline. +export async function connectBitbucket( + input: BitbucketConnectArgs +): Promise { + const config = buildCandidateConfig(input) + if (!hasAuth(config)) { + return { + ok: false, + error: + input.authMode === 'token' + ? 'Enter an access token.' + : 'Enter both an email and an API token.' + } + } + const user = await fetchBitbucketUser(config, VERIFY_TIMEOUT_MS) + if (!user) { + return { + ok: false, + error: 'Could not authenticate with Bitbucket. Check the credentials and try again.' + } + } + const account = accountNameFromUser(user) + saveBitbucketCredential({ + authMode: input.authMode, + email: config.email, + baseUrl: normalize(input.baseUrl), + account, + accessToken: config.accessToken, + apiToken: config.apiToken + }) + return { ok: true, account } +} + +export function disconnectBitbucket(): void { + clearStoredBitbucketCredential() +} + +// Reads env vars and plaintext metadata only — never decrypts — so the Settings +// card can call it on every open without a keychain prompt. +export function getBitbucketConnectionStatus(): BitbucketConnectionStatus { + const env = getEnvAuthConfig() + if (hasAuth(env)) { + return { + configured: true, + source: 'environment', + account: null, + authMode: env.accessToken ? 'token' : 'basic', + email: env.email, + baseUrl: envValue('ORCA_BITBUCKET_API_BASE_URL') + } + } + if (hasStoredBitbucketCredential()) { + const metadata = getStoredBitbucketMetadata() + return { + configured: true, + source: 'stored', + account: metadata?.account ?? null, + authMode: metadata?.authMode ?? null, + email: metadata?.email ?? null, + baseUrl: metadata?.baseUrl ?? null + } + } + return { + configured: false, + source: 'none', + account: null, + authMode: null, + email: null, + baseUrl: null + } +} diff --git a/src/main/bitbucket/credential-store.test.ts b/src/main/bitbucket/credential-store.test.ts new file mode 100644 index 00000000000..6c6a6754b12 --- /dev/null +++ b/src/main/bitbucket/credential-store.test.ts @@ -0,0 +1,218 @@ +import { existsSync, mkdtempSync, statSync } from 'node:fs' +import type * as Fs from 'node:fs' +import { tmpdir } from 'node:os' +import type * as Os from 'node:os' +import { join } from 'node:path' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +let tempHome = '' +const decryptStringMock = vi.fn((value: Buffer) => value.toString('utf-8')) + +async function loadStore(options: { unlinkError?: NodeJS.ErrnoException } = {}) { + vi.resetModules() + vi.doMock('electron', () => ({ + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (value: string) => Buffer.from(value), + decryptString: decryptStringMock + } + })) + vi.doMock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { ...actual, homedir: () => tempHome } + }) + if (options.unlinkError) { + // Why mocked: chmod-based failure injection is not portable — Windows has no + // POSIX modes and a root/elevated runner can unlink through a 0500 directory. + const error = options.unlinkError + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + unlinkSync: () => { + throw error + } + } + }) + } + return import('./credential-store') +} + +beforeEach(() => { + tempHome = mkdtempSync(join(tmpdir(), 'orca-bitbucket-store-')) + decryptStringMock.mockClear() +}) + +describe('Bitbucket credential store', () => { + it('persists plaintext metadata and an encrypted secret, then reads them back', async () => { + const store = await loadStore() + store.saveBitbucketCredential({ + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null, + account: 'ada', + accessToken: null, + apiToken: 'secret-token' + }) + + expect(store.hasStoredBitbucketCredential()).toBe(true) + expect(store.getStoredBitbucketMetadata()).toMatchObject({ + authMode: 'basic', + email: 'ada@example.com', + account: 'ada' + }) + expect(store.loadStoredBitbucketSecret()).toEqual({ + accessToken: null, + apiToken: 'secret-token' + }) + }) + + it('writes both credential files 0600', async () => { + const store = await loadStore() + store.saveBitbucketCredential({ + authMode: 'token', + email: null, + baseUrl: null, + account: 'dev', + accessToken: 'access-secret', + apiToken: null + }) + + for (const file of ['bitbucket-credential.enc', 'bitbucket-credential.json']) { + expect(statSync(join(tempHome, '.orca', file)).mode & 0o777).toBe(0o600) + } + }) + + it('re-tightens permissions when overwriting an existing credential', async () => { + const store = await loadStore() + const save = (account: string): void => + store.saveBitbucketCredential({ + authMode: 'token', + email: null, + baseUrl: null, + account, + accessToken: 'access-secret', + apiToken: null + }) + save('first') + // Why: writeFileSync's mode is ignored for an existing file, so a loosened + // credential would stay world-readable across a reconnect. + const { chmodSync } = await import('node:fs') + for (const file of ['bitbucket-credential.enc', 'bitbucket-credential.json']) { + chmodSync(join(tempHome, '.orca', file), 0o644) + } + save('second') + + for (const file of ['bitbucket-credential.enc', 'bitbucket-credential.json']) { + expect(statSync(join(tempHome, '.orca', file)).mode & 0o777).toBe(0o600) + } + }) + + it('does not decrypt for metadata/status reads — only on a forced secret load', async () => { + const store = await loadStore() + store.saveBitbucketCredential({ + authMode: 'token', + email: null, + baseUrl: 'https://api.bitbucket.org/2.0', + account: 'dev', + accessToken: 'access-secret', + apiToken: null + }) + + // Simulate a fresh session: caches cleared, files still on disk. + store._resetBitbucketCredentialCache() + + expect(store.getStoredBitbucketMetadata()?.account).toBe('dev') + expect(store.hasStoredBitbucketCredential()).toBe(true) + expect(decryptStringMock).not.toHaveBeenCalled() + + // Without force, the secret stays unread. + expect(store.loadStoredBitbucketSecret()).toBeNull() + expect(decryptStringMock).not.toHaveBeenCalled() + + // Forcing the load decrypts exactly once, then caches. + expect(store.loadStoredBitbucketSecret({ force: true })).toEqual({ + accessToken: 'access-secret', + apiToken: null + }) + expect(decryptStringMock).toHaveBeenCalledTimes(1) + expect(store.loadStoredBitbucketSecret()).not.toBeNull() + expect(decryptStringMock).toHaveBeenCalledTimes(1) + }) + + it('rejects non-string fields from hand-edited metadata and secret files', async () => { + const store = await loadStore() + const { writeFileSync } = await import('node:fs') + store.saveBitbucketCredential({ + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null, + account: 'ada', + accessToken: null, + apiToken: 'secret-token' + }) + + writeFileSync( + join(tempHome, '.orca', 'bitbucket-credential.json'), + JSON.stringify({ version: 1, authMode: 'basic', email: { evil: true }, account: 42 }) + ) + writeFileSync( + join(tempHome, '.orca', 'bitbucket-credential.enc'), + JSON.stringify({ accessToken: ['nope'], apiToken: 7 }) + ) + store._resetBitbucketCredentialCache() + + expect(store.getStoredBitbucketMetadata()).toMatchObject({ email: null, account: null }) + expect(store.loadStoredBitbucketSecret({ force: true })).toEqual({ + accessToken: null, + apiToken: null + }) + }) + + it('clears both files and in-memory state on disconnect', async () => { + const store = await loadStore() + store.saveBitbucketCredential({ + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null, + account: 'ada', + accessToken: null, + apiToken: 'secret-token' + }) + + store.clearStoredBitbucketCredential() + + expect(store.hasStoredBitbucketCredential()).toBe(false) + expect(store.getStoredBitbucketMetadata()).toBeNull() + expect(existsSync(join(tempHome, '.orca', 'bitbucket-credential.enc'))).toBe(false) + expect(existsSync(join(tempHome, '.orca', 'bitbucket-credential.json'))).toBe(false) + }) + + it('surfaces a non-ENOENT delete failure instead of silently keeping the files', async () => { + const denied: NodeJS.ErrnoException = Object.assign(new Error('permission denied'), { + code: 'EACCES' + }) + const store = await loadStore({ unlinkError: denied }) + store.saveBitbucketCredential({ + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null, + account: 'ada', + accessToken: null, + apiToken: 'secret-token' + }) + + // Clearing memory while the files survive would resurrect the credential on + // the next launch, so the failure has to reach the caller. + expect(() => store.clearStoredBitbucketCredential()).toThrow(/permission denied/) + expect(existsSync(join(tempHome, '.orca', 'bitbucket-credential.enc'))).toBe(true) + }) + + it('ignores a missing file on disconnect', async () => { + const missing: NodeJS.ErrnoException = Object.assign(new Error('no such file'), { + code: 'ENOENT' + }) + const store = await loadStore({ unlinkError: missing }) + expect(() => store.clearStoredBitbucketCredential()).not.toThrow() + }) +}) diff --git a/src/main/bitbucket/credential-store.ts b/src/main/bitbucket/credential-store.ts new file mode 100644 index 00000000000..17b3bb56d2c --- /dev/null +++ b/src/main/bitbucket/credential-store.ts @@ -0,0 +1,202 @@ +import { existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs' +import { homedir } from 'node:os' +import { join } from 'node:path' +import { + CredentialDecryptionError, + credentialFileHasContent, + readStoredCredentialToken, + restrictCredentialFileToOwner, + writeEncryptedCredential +} from '../integration-credential-file' +import type { BitbucketAuthMode } from '../../shared/bitbucket-credentials' + +// Why: the secret stays encrypted via safeStorage while this metadata stays +// plaintext, so status reads render the connected account without decrypting — +// otherwise every Settings open would trigger an OS keychain prompt. +export type BitbucketStoredMetadata = { + version: 1 + authMode: BitbucketAuthMode + email: string | null + baseUrl: string | null + account: string | null + updatedAt: string +} + +export type BitbucketStoredSecret = { + accessToken: string | null + apiToken: string | null +} + +export type BitbucketCredentialSaveInput = { + authMode: BitbucketAuthMode + email: string | null + baseUrl: string | null + account: string | null + accessToken: string | null + apiToken: string | null +} + +let cachedMetadata: BitbucketStoredMetadata | null = null +let metadataLoadedFromDisk = false +let cachedSecret: BitbucketStoredSecret | null = null +let credentialError: string | null = null + +function getOrcaDir(): string { + return join(homedir(), '.orca') +} + +function getMetadataPath(): string { + return join(getOrcaDir(), 'bitbucket-credential.json') +} + +function getSecretPath(): string { + return join(getOrcaDir(), 'bitbucket-credential.enc') +} + +function ensureOrcaDir(): void { + const dir = getOrcaDir() + if (!existsSync(dir)) { + mkdirSync(dir, { recursive: true }) + } +} + +// Why: hand-edited or truncated JSON must not put a non-string into an auth +// header, so every field is narrowed rather than trusted. +function asOptionalString(value: unknown): string | null { + return typeof value === 'string' ? value : null +} + +function readMetadataFromDisk(): BitbucketStoredMetadata | null { + const path = getMetadataPath() + if (!existsSync(path)) { + return null + } + try { + const parsed = JSON.parse(readFileSync(path, 'utf-8')) as Partial + if (parsed.authMode !== 'token' && parsed.authMode !== 'basic') { + return null + } + return { + version: 1, + authMode: parsed.authMode, + email: asOptionalString(parsed.email), + baseUrl: asOptionalString(parsed.baseUrl), + account: asOptionalString(parsed.account), + updatedAt: asOptionalString(parsed.updatedAt) ?? '' + } + } catch { + return null + } +} + +export function getStoredBitbucketMetadata(): BitbucketStoredMetadata | null { + if (!metadataLoadedFromDisk) { + cachedMetadata = readMetadataFromDisk() + metadataLoadedFromDisk = true + } + return cachedMetadata +} + +// Cheap "is a credential saved?" check: metadata present and a non-empty secret +// file. Never decrypts, so it is safe on every status poll. +export function hasStoredBitbucketCredential(): boolean { + return getStoredBitbucketMetadata() !== null && credentialFileHasContent(getSecretPath()) +} + +export function getStoredBitbucketCredentialError(): string | null { + return credentialError +} + +// Cache-first; only touches the keychain when `force` is set and the secret is +// not already in memory (mirrors Linear's `loadToken({ force })`). Throws +// CredentialDecryptionError when ciphertext cannot be decrypted. +export function loadStoredBitbucketSecret( + options: { force?: boolean } = {} +): BitbucketStoredSecret | null { + if (cachedSecret !== null) { + return cachedSecret + } + if (!options.force) { + return null + } + const path = getSecretPath() + if (!existsSync(path)) { + return null + } + try { + const token = readStoredCredentialToken('Bitbucket', readFileSync(path)) + if (!token) { + return null + } + const parsed = JSON.parse(token) as Partial + cachedSecret = { + accessToken: asOptionalString(parsed.accessToken), + apiToken: asOptionalString(parsed.apiToken) + } + credentialError = null + return cachedSecret + } catch (error) { + if (error instanceof CredentialDecryptionError) { + credentialError = error.message + throw error + } + return null + } +} + +export function saveBitbucketCredential(input: BitbucketCredentialSaveInput): void { + ensureOrcaDir() + const secret: BitbucketStoredSecret = { + accessToken: input.accessToken, + apiToken: input.apiToken + } + writeEncryptedCredential('Bitbucket', getSecretPath(), JSON.stringify(secret)) + const metadata: BitbucketStoredMetadata = { + version: 1, + authMode: input.authMode, + email: input.email, + baseUrl: input.baseUrl, + account: input.account, + updatedAt: new Date().toISOString() + } + writeFileSync(getMetadataPath(), JSON.stringify(metadata, null, 2), { + encoding: 'utf-8', + mode: 0o600 + }) + restrictCredentialFileToOwner(getMetadataPath()) + cachedMetadata = metadata + metadataLoadedFromDisk = true + cachedSecret = secret + credentialError = null +} + +// Why: swallowing a non-ENOENT unlink failure would clear memory while the +// files survive, so the credential silently returns on the next launch. +export function clearStoredBitbucketCredential(): void { + try { + for (const path of [getSecretPath(), getMetadataPath()]) { + try { + unlinkSync(path) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error + } + } + } + } finally { + // Why: on a partial delete the caches must still be dropped, or the session + // keeps authenticating with a credential the user just disconnected. + cachedMetadata = null + metadataLoadedFromDisk = true + cachedSecret = null + credentialError = null + } +} + +/** @internal - tests need a clean in-memory cache between cases. */ +export function _resetBitbucketCredentialCache(): void { + cachedMetadata = null + metadataLoadedFromDisk = false + cachedSecret = null + credentialError = null +} diff --git a/src/main/bitbucket/pull-request-creation.test.ts b/src/main/bitbucket/pull-request-creation.test.ts new file mode 100644 index 00000000000..ab06200e22e --- /dev/null +++ b/src/main/bitbucket/pull-request-creation.test.ts @@ -0,0 +1,180 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createBitbucketPullRequest, + isBitbucketReviewCreationAuthenticated +} from './pull-request-creation' +import { _resetBitbucketRepoRefCache } from './repository-ref' + +const { gitExecFileAsyncMock, getSshGitProviderMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock, + getSshGitProviderGeneration: () => 0 +})) + +vi.mock('../source-control/pull-request-template', () => ({ + readHostedPullRequestTemplate: vi.fn(async () => 'Template body') +})) + +const OLD_ENV = process.env +const OLD_FETCH = globalThis.fetch + +const CREATE_INPUT = { + provider: 'bitbucket', + base: 'main', + head: 'feature/login', + title: 'Add login' +} as const + +function createdPullRequestResponse(): Response { + return Response.json({ + id: 42, + title: 'Add login', + state: 'OPEN', + updated_on: '2026-08-11T00:00:00Z', + links: { html: { href: 'https://bitbucket.org/team/repo/pull-requests/42' } }, + source: { branch: { name: 'feature/login' } }, + destination: { branch: { name: 'main' } } + }) +} + +describe('Bitbucket pull request creation', () => { + beforeEach(() => { + process.env = { ...OLD_ENV, ORCA_BITBUCKET_ACCESS_TOKEN: 'bb-token' } + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'https://bitbucket.org/team/repo.git\n', + stderr: '' + }) + _resetBitbucketRepoRefCache() + }) + + afterEach(() => { + process.env = OLD_ENV + globalThis.fetch = OLD_FETCH + _resetBitbucketRepoRefCache() + }) + + it('posts the Bitbucket source/destination create body to the repository endpoint', async () => { + const fetchMock = vi.fn(async (input: string | URL | Request, init?: RequestInit) => { + const url = new URL(String(input)) + expect(url.origin).toBe('https://api.bitbucket.org') + expect(url.pathname).toBe('/2.0/repositories/team/repo/pullrequests') + const requestInit = init! + expect(requestInit.method).toBe('POST') + expect((requestInit.headers as Record).Authorization).toBe('Bearer bb-token') + expect(JSON.parse(String(requestInit.body))).toEqual({ + title: 'Add login', + description: '', + source: { branch: { name: 'feature/login' } }, + destination: { branch: { name: 'main' } } + }) + return createdPullRequestResponse() + }) + globalThis.fetch = fetchMock as unknown as typeof fetch + + await expect(createBitbucketPullRequest('/repo', CREATE_INPUT)).resolves.toEqual({ + ok: true, + number: 42, + url: 'https://bitbucket.org/team/repo/pull-requests/42' + }) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('uses the stored credential when no environment variable is set', async () => { + delete process.env.ORCA_BITBUCKET_ACCESS_TOKEN + const fetchMock = vi.fn(async () => createdPullRequestResponse()) + globalThis.fetch = fetchMock as unknown as typeof fetch + + const result = await createBitbucketPullRequest('/repo', CREATE_INPUT) + + // No env var and no stored credential: fail closed rather than POST anonymously. + expect(result).toMatchObject({ ok: false, code: 'auth_required' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('ignores a draft request rather than dead-ending a hidden persisted default', async () => { + const fetchMock = vi.fn(async () => createdPullRequestResponse()) + globalThis.fetch = fetchMock as unknown as typeof fetch + + // Why: the composer hides the Draft toggle for Bitbucket, so a `true` here + // is an unreachable persisted default the user cannot clear. + await expect( + createBitbucketPullRequest('/repo', { ...CREATE_INPUT, draft: true }) + ).resolves.toMatchObject({ ok: true, number: 42 }) + expect(JSON.parse(String((fetchMock.mock.calls[0] as never[])[1]['body']))).not.toHaveProperty( + 'draft' + ) + }) + + it('reports authenticated only when a credential resolves', async () => { + expect(isBitbucketReviewCreationAuthenticated()).toBe(true) + delete process.env.ORCA_BITBUCKET_ACCESS_TOKEN + expect(isBitbucketReviewCreationAuthenticated()).toBe(false) + }) + + it('maps a duplicate-branch rejection to already_exists with the existing review', async () => { + let call = 0 + const fetchMock = vi.fn(async () => { + call += 1 + if (call === 1) { + return Response.json( + { error: { message: 'There is already a pull request for this branch.' } }, + { status: 400 } + ) + } + return Response.json({ + values: [ + { + id: 7, + title: 'Add login', + state: 'OPEN', + updated_on: '2026-08-11T00:00:00Z', + links: { html: { href: 'https://bitbucket.org/team/repo/pull-requests/7' } }, + source: { branch: { name: 'feature/login' } }, + destination: { branch: { name: 'main' } } + } + ] + }) + }) + globalThis.fetch = fetchMock as unknown as typeof fetch + + expect(await createBitbucketPullRequest('/repo', CREATE_INPUT)).toMatchObject({ + ok: false, + code: 'already_exists', + existingReview: { number: 7, url: 'https://bitbucket.org/team/repo/pull-requests/7' } + }) + }) + + it('maps a 401 to auth_required pointing at both credential paths', async () => { + globalThis.fetch = vi.fn(async () => + Response.json({ error: { message: 'Unauthorized' } }, { status: 401 }) + ) as unknown as typeof fetch + + const result = await createBitbucketPullRequest('/repo', CREATE_INPUT) + + expect(result).toMatchObject({ ok: false, code: 'auth_required' }) + expect(!result.ok && result.error).toContain('Settings') + }) + + it('refuses a non-Bitbucket remote', async () => { + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'https://github.com/team/repo.git\n', + stderr: '' + }) + globalThis.fetch = vi.fn() as unknown as typeof fetch + + await expect(createBitbucketPullRequest('/repo', CREATE_INPUT)).resolves.toMatchObject({ + ok: false, + code: 'unsupported_provider' + }) + }) +}) diff --git a/src/main/bitbucket/pull-request-creation.ts b/src/main/bitbucket/pull-request-creation.ts new file mode 100644 index 00000000000..86a44f49b4e --- /dev/null +++ b/src/main/bitbucket/pull-request-creation.ts @@ -0,0 +1,221 @@ +import type { CreateHostedReviewInput, CreateHostedReviewResult } from '../../shared/hosted-review' +import { + normalizeHostedReviewBaseRef, + normalizeHostedReviewHeadRef +} from '../../shared/hosted-review-refs' +import { + HostedReviewApiRequestError, + requestHostedReviewJson +} from '../source-control/hosted-review-api-request' +import { readHostedPullRequestTemplate } from '../source-control/pull-request-template' +import { authHeaders, hasAuth } from './bitbucket-auth-config' +import { resolveBitbucketAuthConfig } from './resolve-auth' +import { getBitbucketPullRequestForBranch } from './client' +import { mapBitbucketPullRequest, type RawBitbucketPullRequest } from './pull-request-mappers' +import { getBitbucketRepoRef, type BitbucketRepoRef } from './repository-ref' +import type { HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' +import { getHostedReviewLocalGitOptions } from '../source-control/hosted-review-git-options' + +const CREATE_REQUEST_TIMEOUT_MS = 60_000 + +export function isBitbucketReviewCreationAuthenticated(): boolean { + return hasAuth(resolveBitbucketAuthConfig()) +} + +function encodedRepoPath(repo: BitbucketRepoRef): string { + return `${encodeURIComponent(repo.workspace)}/${encodeURIComponent(repo.repoSlug)}` +} + +function apiErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + +function classifyCreateError(error: unknown): CreateHostedReviewResult { + const message = apiErrorMessage(error) + if (message) { + console.warn('createBitbucketPullRequest failed:', message) + } + const lower = message.toLowerCase() + const status = error instanceof HostedReviewApiRequestError ? error.status : null + if (status === 401 || status === 403 || lower.includes('unauthorized')) { + return { + ok: false, + code: 'auth_required', + error: + 'Create PR failed: Bitbucket is not authenticated. Next step: connect Bitbucket in Settings > Integrations, or set ORCA_BITBUCKET_* in this environment.' + } + } + // Bitbucket answers a duplicate source branch with 400 plus this phrasing + // rather than a 409, so the text is the only reliable signal. + if (lower.includes('already exists') || lower.includes('already a pull request')) { + return { + ok: false, + code: 'already_exists', + error: 'A pull request already exists for this branch.' + } + } + if (error instanceof HostedReviewApiRequestError && error.timedOut) { + return { + ok: false, + code: 'unknown_completion', + error: 'PR creation may have completed. Refreshing branch review state...' + } + } + if (status === 400 || status === 422) { + return { + ok: false, + code: 'validation', + error: + 'Create PR failed: Bitbucket rejected the pull request. Check the base branch and branch state, then try again.' + } + } + if (status === 404) { + return { + ok: false, + code: 'validation', + error: + 'Create PR failed: Bitbucket could not find the repository. Check that the credential has write access to it.' + } + } + return { + ok: false, + code: 'unknown', + error: 'Create PR failed: Bitbucket could not create the pull request. Try again in a moment.' + } +} + +async function findExistingPullRequest( + repoPath: string, + head: string, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise<{ number: number; url: string } | null> { + const existing = await getBitbucketPullRequestForBranch( + repoPath, + head, + null, + connectionId, + options + ) + return existing ? { number: existing.number, url: existing.url } : null +} + +export async function createBitbucketPullRequest( + repoPath: string, + input: CreateHostedReviewInput, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + if (input.provider !== 'bitbucket') { + return { + ok: false, + code: 'unsupported_provider', + error: 'Creating reviews for this provider is not supported yet.' + } + } + + const config = resolveBitbucketAuthConfig() + if (!hasAuth(config)) { + return { + ok: false, + code: 'auth_required', + error: + 'Create PR failed: Bitbucket is not connected. Next step: connect Bitbucket in Settings > Integrations.' + } + } + + const repo = await getBitbucketRepoRef( + repoPath, + connectionId, + getHostedReviewLocalGitOptions(options) + ) + if (!repo) { + return { + ok: false, + code: 'unsupported_provider', + error: 'Creating pull requests requires a Bitbucket remote.' + } + } + + const base = normalizeHostedReviewBaseRef(input.base) + const head = input.head ? normalizeHostedReviewHeadRef(input.head) : '' + const title = input.title.trim() + if (!base || !head || !title) { + return { + ok: false, + code: 'validation', + error: 'Create PR failed: base branch, head branch, and title are required.' + } + } + if (head.toLowerCase() === base.toLowerCase()) { + return { + ok: false, + code: 'validation', + error: 'Create PR failed: choose a different base branch before creating a pull request.' + } + } + // Why: Bitbucket Cloud has no draft pull requests and the composer hides the + // toggle, so `draft` here can only be an unreachable persisted default — + // rejecting it would dead-end the user with no control to clear. + const description = + input.useTemplate && !input.body?.trim() + ? await readHostedPullRequestTemplate(repoPath, connectionId) + : (input.body ?? '') + const requestBody = { + title, + description, + source: { branch: { name: head } }, + destination: { branch: { name: base } } + } + + try { + const raw = await requestHostedReviewJson( + new URL( + `${config.baseUrl.replace(/\/+$/, '')}/repositories/${encodedRepoPath(repo)}/pullrequests` + ), + { + method: 'POST', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + ...authHeaders(config) + }, + body: JSON.stringify(requestBody) + }, + CREATE_REQUEST_TIMEOUT_MS + ) + const created = mapBitbucketPullRequest(raw, 'neutral') + if (created) { + return { ok: true, number: created.number, url: created.url } + } + const found = await findExistingPullRequest(repoPath, head, connectionId, options).catch( + () => null + ) + return found + ? { ok: true, ...found } + : { + ok: false, + code: 'unknown_completion', + error: 'PR creation may have completed. Refreshing branch review state...' + } + } catch (error) { + const classified = classifyCreateError(error) + if ( + !classified.ok && + (classified.code === 'already_exists' || classified.code === 'unknown_completion') + ) { + const existing = await findExistingPullRequest(repoPath, head, connectionId, options).catch( + () => null + ) + if (existing) { + return { + ok: false, + code: 'already_exists', + error: 'A pull request already exists for this branch.', + existingReview: existing + } + } + } + return classified + } +} diff --git a/src/main/bitbucket/resolve-auth.ts b/src/main/bitbucket/resolve-auth.ts new file mode 100644 index 00000000000..8e5835ab57e --- /dev/null +++ b/src/main/bitbucket/resolve-auth.ts @@ -0,0 +1,48 @@ +import { + DEFAULT_API_BASE_URL, + envValue, + getEnvAuthConfig, + hasAuth, + type BitbucketAuthConfig +} from './bitbucket-auth-config' +import { + getStoredBitbucketMetadata, + loadStoredBitbucketSecret, + type BitbucketStoredMetadata, + type BitbucketStoredSecret +} from './credential-store' + +export function storedAuthConfig( + metadata: BitbucketStoredMetadata, + secret: BitbucketStoredSecret +): BitbucketAuthConfig { + return { + // Why: an explicit ORCA_BITBUCKET_API_BASE_URL still wins even when the + // credential itself is stored — env precedence is per-setting, not all-or-nothing. + baseUrl: envValue('ORCA_BITBUCKET_API_BASE_URL') ?? metadata.baseUrl ?? DEFAULT_API_BASE_URL, + accessToken: metadata.authMode === 'token' ? secret.accessToken : null, + email: metadata.authMode === 'basic' ? metadata.email : null, + apiToken: metadata.authMode === 'basic' ? secret.apiToken : null + } +} + +// Env vars win over in-app credentials so existing headless/SSH setups keep +// working unchanged. The stored secret is decrypted lazily and only here, on a +// real API call — never on a status read. +export function resolveBitbucketAuthConfig(): BitbucketAuthConfig { + const env = getEnvAuthConfig() + if (hasAuth(env)) { + return env + } + const metadata = getStoredBitbucketMetadata() + if (!metadata) { + return env + } + try { + const secret = loadStoredBitbucketSecret({ force: true }) + return secret ? storedAuthConfig(metadata, secret) : env + } catch { + // Decryption denied or unavailable: fall through as unauthenticated. + return env + } +} diff --git a/src/main/bitbucket/status-no-decrypt.test.ts b/src/main/bitbucket/status-no-decrypt.test.ts new file mode 100644 index 00000000000..946f1aea4aa --- /dev/null +++ b/src/main/bitbucket/status-no-decrypt.test.ts @@ -0,0 +1,127 @@ +import { mkdtempSync } from 'node:fs' +import { tmpdir } from 'node:os' +import type * as Os from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Why: opening Settings renders the Bitbucket card, which runs the preflight +// status (getBitbucketAuthStatus) and the card's status IPC +// (getBitbucketConnectionStatus). Neither may decrypt the stored secret, or the +// OS would prompt to unlock the keychain on every Settings open. These tests +// simulate a cold session (files on disk, memory cache empty). + +const decryptSpy = vi.fn((value: Buffer) => value.toString('utf-8')) +const OLD_ENV = process.env +const OLD_FETCH = globalThis.fetch +let tempHome = '' + +vi.mock('../git/runner', () => ({ gitExecFileAsync: vi.fn() })) + +async function loadModules() { + vi.resetModules() + vi.doMock('electron', () => ({ + safeStorage: { + isEncryptionAvailable: () => true, + encryptString: (value: string) => Buffer.from(value), + decryptString: decryptSpy + } + })) + vi.doMock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { ...actual, homedir: () => tempHome } + }) + // Stub repo detection so the PR fetch reaches the auth-resolution path. + vi.doMock('./repository-ref', () => ({ + getBitbucketRepoRef: async () => ({ workspace: 'acme', repoSlug: 'app' }) + })) + const store = await import('./credential-store') + const client = await import('./client') + const connection = await import('./credential-connection') + return { store, client, connection } +} + +beforeEach(() => { + process.env = { ...OLD_ENV } + for (const key of [ + 'ORCA_BITBUCKET_ACCESS_TOKEN', + 'ORCA_BITBUCKET_EMAIL', + 'ORCA_BITBUCKET_API_TOKEN', + 'ORCA_BITBUCKET_API_BASE_URL' + ]) { + delete process.env[key] + } + tempHome = mkdtempSync(join(tmpdir(), 'orca-bb-nodecrypt-')) + decryptSpy.mockClear() +}) + +afterEach(() => { + process.env = OLD_ENV + globalThis.fetch = OLD_FETCH +}) + +const STORED_BASIC_CREDENTIAL = { + authMode: 'basic', + email: 'ada@example.com', + baseUrl: null, + account: 'ada', + accessToken: null, + apiToken: 'secret-token' +} as const + +describe('Bitbucket status reads never decrypt the stored secret', () => { + it('renders connected state from plaintext metadata without touching the keychain', async () => { + const { store, client, connection } = await loadModules() + const fetchSpy = vi.fn() + globalThis.fetch = fetchSpy as unknown as typeof fetch + store.saveBitbucketCredential(STORED_BASIC_CREDENTIAL) + + // Simulate relaunch: secret is on disk but the memory cache is cold, so a + // decrypt here would surface to the user as a keychain prompt. + store._resetBitbucketCredentialCache() + decryptSpy.mockClear() + + const auth = await client.getBitbucketAuthStatus() + const status = connection.getBitbucketConnectionStatus() + + expect(auth).toEqual({ configured: true, authenticated: true, account: 'ada' }) + expect(status).toMatchObject({ source: 'stored', account: 'ada', authMode: 'basic' }) + expect(decryptSpy).not.toHaveBeenCalled() + // A stored credential is trusted from metadata rather than revalidated. + expect(fetchSpy).not.toHaveBeenCalled() + }) + + it('stays cold across repeated status reads, then decrypts once for a real API call', async () => { + const { store, client, connection } = await loadModules() + store.saveBitbucketCredential(STORED_BASIC_CREDENTIAL) + store._resetBitbucketCredentialCache() + decryptSpy.mockClear() + + for (let i = 0; i < 3; i += 1) { + await client.getBitbucketAuthStatus() + connection.getBitbucketConnectionStatus() + } + expect(decryptSpy).not.toHaveBeenCalled() + + globalThis.fetch = vi.fn(async () => Response.json({ values: [] })) as unknown as typeof fetch + await client.getBitbucketPullRequest('/repo', 1, null) + expect(decryptSpy).toHaveBeenCalledTimes(1) + }) + + it('revalidates a stored credential once its secret is already warm in memory', async () => { + const { store, client } = await loadModules() + store.saveBitbucketCredential(STORED_BASIC_CREDENTIAL) + decryptSpy.mockClear() + + // Saving leaves the secret cached, so status can check /user for free. + const fetchSpy = vi.fn(async () => new Response(null, { status: 401 })) + globalThis.fetch = fetchSpy as unknown as typeof fetch + + await expect(client.getBitbucketAuthStatus()).resolves.toEqual({ + configured: true, + authenticated: false, + account: 'ada' + }) + expect(fetchSpy).toHaveBeenCalledTimes(1) + expect(decryptSpy).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/bitbucket/user-request.ts b/src/main/bitbucket/user-request.ts new file mode 100644 index 00000000000..6f83703d58e --- /dev/null +++ b/src/main/bitbucket/user-request.ts @@ -0,0 +1,39 @@ +import { authHeaders, type BitbucketAuthConfig } from './bitbucket-auth-config' +import { cancelUnreadResponseBody } from '../lib/unread-response-body' + +const USER_REQUEST_TIMEOUT_MS = 4000 + +export type RawBitbucketUser = { + username?: string | null + display_name?: string | null + account_id?: string | null +} + +export function accountNameFromUser(user: RawBitbucketUser | null): string | null { + return user?.username ?? user?.display_name ?? user?.account_id ?? null +} + +// Shared by live env-var status checks and by connect-time verification, so a +// credential is proven against `/user` before it is ever persisted. +export async function fetchBitbucketUser( + config: BitbucketAuthConfig, + timeoutMs: number = USER_REQUEST_TIMEOUT_MS +): Promise { + try { + const base = config.baseUrl.replace(/\/+$/, '') + const response = await fetch(`${base}/user`, { + headers: { + Accept: 'application/json', + ...authHeaders(config) + }, + signal: AbortSignal.timeout(timeoutMs) + }) + if (!response.ok) { + await cancelUnreadResponseBody(response) + return null + } + return (await response.json()) as RawBitbucketUser + } catch { + return null + } +} diff --git a/src/main/global-fetch-call-site-audit.test.ts b/src/main/global-fetch-call-site-audit.test.ts index 625d9fa2b04..b46c1b793be 100644 --- a/src/main/global-fetch-call-site-audit.test.ts +++ b/src/main/global-fetch-call-site-audit.test.ts @@ -18,6 +18,7 @@ const AUDITED_GLOBAL_FETCH_LINES = new Map([ ['main/artifacts/artifact-cloud-request.ts', 1], ['main/azure-devops/azure-devops-api-request.ts', 1], ['main/bitbucket/client.ts', 1], + ['main/bitbucket/user-request.ts', 1], ['main/gitea/client.ts', 1], ['main/orca-profiles/profile-cloud-client.ts', 1], ['main/orca-profiles/profile-cloud-org-members-client.ts', 1], diff --git a/src/main/integration-credential-file.ts b/src/main/integration-credential-file.ts index 2ad90492f1b..c65dcc1fa7a 100644 --- a/src/main/integration-credential-file.ts +++ b/src/main/integration-credential-file.ts @@ -1,4 +1,4 @@ -import { statSync } from 'node:fs' +import { chmodSync, statSync, writeFileSync } from 'node:fs' import { safeStorage } from 'electron' import { credentialDecryptionMessage, @@ -15,6 +15,36 @@ export function credentialFileHasContent(path: string): boolean { } } +// Falls back to 0600 plaintext when the OS keyring is unavailable, matching the +// Linear/Jira token writers so a keyring-less box still connects. +export function writeEncryptedCredential( + service: IntegrationCredentialService, + path: string, + value: string +): void { + if (safeStorage.isEncryptionAvailable()) { + writeFileSync(path, safeStorage.encryptString(value), { mode: 0o600 }) + restrictCredentialFileToOwner(path) + return + } + console.warn( + `[${service.toLowerCase()}] safeStorage encryption unavailable — storing credential in plaintext` + ) + writeFileSync(path, value, { encoding: 'utf-8', mode: 0o600 }) + restrictCredentialFileToOwner(path) +} + +// Why: writeFileSync's `mode` only applies when it creates the file, so +// rewriting an existing credential would silently keep looser permissions. +// Best-effort: Windows has no POSIX modes, so chmod is a documented no-op there. +export function restrictCredentialFileToOwner(path: string): void { + try { + chmodSync(path, 0o600) + } catch { + // Best-effort hardening; the write itself already succeeded. + } +} + export class CredentialDecryptionError extends Error { constructor(service: IntegrationCredentialService) { super(credentialDecryptionMessage(service)) diff --git a/src/main/ipc/bitbucket.ts b/src/main/ipc/bitbucket.ts new file mode 100644 index 00000000000..2b9b030c691 --- /dev/null +++ b/src/main/ipc/bitbucket.ts @@ -0,0 +1,59 @@ +import { ipcMain } from 'electron' +import { + connectBitbucket, + disconnectBitbucket, + getBitbucketConnectionStatus, + type BitbucketConnectArgs, + type BitbucketConnectResult, + type BitbucketConnectionStatus +} from '../bitbucket/credential-connection' +import { _resetPreflightCache } from './preflight' + +function optionalString(value: unknown): string | null { + return typeof value === 'string' ? value : null +} + +function normalizeConnectInput(value: unknown): BitbucketConnectArgs | null { + if (!value || typeof value !== 'object') { + return null + } + const raw = value as Record + if (raw.authMode !== 'token' && raw.authMode !== 'basic') { + return null + } + return { + authMode: raw.authMode, + accessToken: optionalString(raw.accessToken), + email: optionalString(raw.email), + apiToken: optionalString(raw.apiToken), + baseUrl: optionalString(raw.baseUrl) + } +} + +export function registerBitbucketHandlers(): void { + ipcMain.handle( + 'bitbucket:connect', + async (_event, args: unknown): Promise => { + const input = normalizeConnectInput(args) + if (!input) { + return { ok: false, error: 'Invalid Bitbucket credentials' } + } + const result = await connectBitbucket(input) + if (result.ok) { + // Preflight caches source-control status per session; reset so the card + // reflects the new connection without a relaunch. + _resetPreflightCache() + } + return result + } + ) + + ipcMain.handle('bitbucket:disconnect', async (): Promise => { + disconnectBitbucket() + _resetPreflightCache() + }) + + ipcMain.handle('bitbucket:status', async (): Promise => { + return getBitbucketConnectionStatus() + }) +} diff --git a/src/main/ipc/register-core-handlers.test.ts b/src/main/ipc/register-core-handlers.test.ts index b9aa768213b..9bee285bff4 100644 --- a/src/main/ipc/register-core-handlers.test.ts +++ b/src/main/ipc/register-core-handlers.test.ts @@ -48,6 +48,7 @@ const { registerAppHandlersMock, registerLinearHandlersMock, registerJiraHandlersMock, + registerBitbucketHandlersMock, registerGitLabHandlersMock, registerHostedReviewHandlersMock, registerExportHandlersMock, @@ -112,6 +113,7 @@ const { registerAppHandlersMock: vi.fn(), registerLinearHandlersMock: vi.fn(), registerJiraHandlersMock: vi.fn(), + registerBitbucketHandlersMock: vi.fn(), registerGitLabHandlersMock: vi.fn(), registerHostedReviewHandlersMock: vi.fn(), registerExportHandlersMock: vi.fn(), @@ -360,6 +362,10 @@ vi.mock('./jira', () => ({ registerJiraHandlers: registerJiraHandlersMock })) +vi.mock('./bitbucket', () => ({ + registerBitbucketHandlers: registerBitbucketHandlersMock +})) + vi.mock('./gitlab', () => ({ registerGitLabHandlers: registerGitLabHandlersMock })) @@ -424,6 +430,7 @@ describe('registerCoreHandlers', () => { registerAppHandlersMock.mockReset() registerLinearHandlersMock.mockReset() registerJiraHandlersMock.mockReset() + registerBitbucketHandlersMock.mockReset() registerGitLabHandlersMock.mockReset() registerHostedReviewHandlersMock.mockReset() registerExportHandlersMock.mockReset() @@ -504,6 +511,7 @@ describe('registerCoreHandlers', () => { expect(registerGitHubHandlersMock).toHaveBeenCalledWith(store, stats) expect(registerLinearHandlersMock).toHaveBeenCalled() expect(registerJiraHandlersMock).toHaveBeenCalled() + expect(registerBitbucketHandlersMock).toHaveBeenCalled() expect(registerGitLabHandlersMock).toHaveBeenCalledWith(store) expect(registerHostedReviewHandlersMock).toHaveBeenCalledWith(store, stats) expect(registerFeedbackHandlersMock).toHaveBeenCalled() diff --git a/src/main/ipc/register-core-handlers.ts b/src/main/ipc/register-core-handlers.ts index f372cb3f772..7825e73c57e 100644 --- a/src/main/ipc/register-core-handlers.ts +++ b/src/main/ipc/register-core-handlers.ts @@ -14,6 +14,7 @@ import { registerGitLabHandlers } from './gitlab' import { registerHostedReviewHandlers } from './hosted-review' import { registerLinearHandlers } from './linear' import { registerJiraHandlers } from './jira' +import { registerBitbucketHandlers } from './bitbucket' import { registerFeedbackHandlers } from './feedback' import { registerCrashReportingHandlers } from './crash-reporting' import { registerExportHandlers } from './export' @@ -155,6 +156,7 @@ export function registerCoreHandlers( registerHostedReviewHandlers(store, stats) registerLinearHandlers() registerJiraHandlers() + registerBitbucketHandlers() registerFeedbackHandlers() if (crashReports) { registerCrashReportingHandlers(crashReports) diff --git a/src/main/source-control/forge-provider.test.ts b/src/main/source-control/forge-provider.test.ts index ae5e5623a97..c31d84b3c87 100644 --- a/src/main/source-control/forge-provider.test.ts +++ b/src/main/source-control/forge-provider.test.ts @@ -1,7 +1,9 @@ +import { supportsHostedReviewCreation } from '../../shared/hosted-review-creation-providers' import { beforeEach, describe, expect, it, vi } from 'vitest' const { createGitHubPullRequestMock, + createBitbucketPullRequestMock, createGitLabMergeRequestMock, createAzureDevOpsPullRequestMock, createGiteaPullRequestMock, @@ -16,6 +18,7 @@ const { getEnterpriseGitHubRepoSlugMock } = vi.hoisted(() => ({ createGitHubPullRequestMock: vi.fn(), + createBitbucketPullRequestMock: vi.fn(), createGitLabMergeRequestMock: vi.fn(), createAzureDevOpsPullRequestMock: vi.fn(), createGiteaPullRequestMock: vi.fn(), @@ -60,6 +63,10 @@ vi.mock('../bitbucket/client', () => ({ getBitbucketPullRequest: vi.fn() })) +vi.mock('../bitbucket/pull-request-creation', () => ({ + createBitbucketPullRequest: createBitbucketPullRequestMock +})) + vi.mock('../azure-devops/client', () => ({ getAzureDevOpsRepoSlug: getAzureDevOpsRepoSlugMock, getAzureDevOpsPullRequestForBranch: vi.fn(), @@ -99,6 +106,7 @@ describe('forge provider interface', () => { beforeEach(() => { createGitHubPullRequestMock.mockReset() createGitLabMergeRequestMock.mockReset() + createBitbucketPullRequestMock.mockReset() createAzureDevOpsPullRequestMock.mockReset() createGiteaPullRequestMock.mockReset() getAzureDevOpsRepoSlugMock.mockReset() @@ -170,10 +178,16 @@ describe('forge provider interface', () => { ).toEqual([ ['gitlab', true], ['github', true], - ['bitbucket', false], + ['bitbucket', true], ['azure-devops', true], ['gitea', true] ]) + // Why: the shared list is what the Create blocker and the renderer read. + // When it drifted from this one, Bitbucket had a working createReview but + // still reported "provider does not support creating a pull request". + for (const provider of FORGE_PROVIDERS) { + expect(supportsHostedReviewCreation(provider.id)).toBe(provider.supportsReviewCreation) + } createGitHubPullRequestMock.mockResolvedValue({ ok: true, number: 12, @@ -201,6 +215,28 @@ describe('forge provider interface', () => { }) }) + it('routes Bitbucket review creation through the shared provider contract', async () => { + createBitbucketPullRequestMock.mockResolvedValue({ + ok: true, + number: 23, + url: 'https://bitbucket.org/team/orca/pull-requests/23' + }) + + const provider = getForgeProviderById('bitbucket') + const input = { + provider: 'bitbucket' as const, + base: 'main', + head: 'feature/provider-interface', + title: 'Add provider interface' + } + await expect(provider.createReview?.('/repo', input)).resolves.toEqual({ + ok: true, + number: 23, + url: 'https://bitbucket.org/team/orca/pull-requests/23' + }) + expect(createBitbucketPullRequestMock).toHaveBeenCalledWith('/repo', input) + }) + it('routes GitLab review creation through the shared provider contract', async () => { createGitLabMergeRequestMock.mockResolvedValue({ ok: true, diff --git a/src/main/source-control/forge-provider.ts b/src/main/source-control/forge-provider.ts index 19526ad71fa..775943052da 100644 --- a/src/main/source-control/forge-provider.ts +++ b/src/main/source-control/forge-provider.ts @@ -15,6 +15,7 @@ import { getBitbucketPullRequestForBranchOrThrow, getBitbucketRepoSlug } from '../bitbucket/client' +import { createBitbucketPullRequest } from '../bitbucket/pull-request-creation' import { getGiteaPullRequest, getGiteaPullRequestForBranchOrThrow, @@ -198,7 +199,7 @@ const gitHubForgeProvider = { const bitbucketForgeProvider = { id: 'bitbucket', - supportsReviewCreation: false, + supportsReviewCreation: true, resolveRepository: (context) => getBitbucketRepoSlug( context.repoPath, @@ -225,7 +226,8 @@ const bitbucketForgeProvider = { ...hostedReviewExecutionArgs(input) ) return pr ? mapBitbucketReview(pr) : null - } + }, + createReview: createBitbucketPullRequest } satisfies ForgeProvider const azureDevOpsForgeProvider = { diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index f28c319f3ea..58136542917 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -18,6 +18,7 @@ import { } from '../../shared/hosted-review-creation-providers' import { isAzureDevOpsReviewCreationAuthenticated } from '../azure-devops/pull-request-creation' import { isGiteaReviewCreationAuthenticated } from '../gitea/pull-request-creation' +import { isBitbucketReviewCreationAuthenticated } from '../bitbucket/pull-request-creation' import { getEnterpriseGitHubRepoSlug } from '../github/github-enterprise-repository' import { acquire, ghExecFileAsync, gitExecFileAsync, release } from '../github/gh-utils' import { isNoUpstreamError, normalizeGitErrorMessage } from '../../shared/git-remote-error' @@ -291,6 +292,14 @@ function reviewCopy(provider: HostedReviewProvider): { authInstruction: 'Set ORCA_GITEA_TOKEN' } } + if (provider === 'bitbucket') { + return { + shortLabel: 'PR', + reviewLabel: 'pull request', + providerName: 'Bitbucket', + authInstruction: 'Connect Bitbucket in Settings > Integrations' + } + } return { shortLabel: 'PR', reviewLabel: 'pull request', @@ -314,6 +323,11 @@ async function isProviderAuthenticated( if (provider === 'gitea') { return isGiteaReviewCreationAuthenticated() } + if (provider === 'bitbucket') { + // Why: falling through to the GitHub check made Create PR unusable for + // anyone with Bitbucket connected but no `gh auth login`. + return isBitbucketReviewCreationAuthenticated() + } return isGitHubAuthenticated(repoPath, connectionId, options) } diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 6d7bc269b25..17c64db922c 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -8,6 +8,10 @@ import type { HostedReviewInfo, HostedReviewProvider } from '../shared/hosted-review' +import type { + BitbucketConnectArgs, + BitbucketConnectionStatus +} from '../shared/bitbucket-credentials' import type { NativeFileDropPayload } from '../shared/native-file-drop' import type { ComputerAwakeStatus } from '../shared/computer-awake-mode' import type { BrowserFindSource } from '../shared/browser-find-source' @@ -2127,6 +2131,13 @@ export type PreloadApi = { } ) => Promise | null> } + bitbucket: { + connect: ( + args: BitbucketConnectArgs + ) => Promise<{ ok: true; account: string | null } | { ok: false; error: string }> + disconnect: () => Promise + status: () => Promise + } linear: { connect: (args: { apiKey: string diff --git a/src/preload/index.ts b/src/preload/index.ts index cef3ce65085..1254203b969 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1721,6 +1721,21 @@ const api = { // Why: GitLab bindings live in `./gitlab` so `gl.*` changes don't conflict on every upstream sync of this central file. gl: glApi, + bitbucket: { + connect: (args: { + authMode: 'token' | 'basic' + accessToken?: string | null + email?: string | null + apiToken?: string | null + baseUrl?: string | null + }): Promise<{ ok: true; account: string | null } | { ok: false; error: string }> => + ipcRenderer.invoke('bitbucket:connect', args), + + disconnect: (): Promise => ipcRenderer.invoke('bitbucket:disconnect'), + + status: (): Promise => ipcRenderer.invoke('bitbucket:status') + }, + linear: { connect: (args: { apiKey: string diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 3ce46b39dbb..de63d4ffd0e 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -100,9 +100,10 @@ import type { } from './pr-comments-ai-launch-ack' import { parseGitHubIssueOrPRLink } from '../../../../shared/github-links' import { startFixChecksAgent } from '@/lib/fix-checks-agent-launch' -import type { - HostedReviewCreationEligibility, - HostedReviewProvider +import { + hostedReviewProviderSupportsDraft, + type HostedReviewCreationEligibility, + type HostedReviewProvider } from '../../../../shared/hosted-review' import { resolveHostedReviewCreationProvider } from '../../../../shared/hosted-review-creation-providers' import { normalizeGlobalWindowsRuntimeDefault } from '../../../../shared/project-execution-runtime' @@ -188,6 +189,7 @@ import { resolveSourceControlLaunchPlatform } from '@/lib/source-control-launch- import { getLocalProjectExecutionRuntimeContext } from '@/lib/local-preflight-context' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { CreateHostedReviewComposer } from './CreateHostedReviewComposer' +import { resolveCreatedHostedReviewLink } from './source-control-created-review-link' import { formatCreateError } from './create-pull-request-review-copy' import { stripBaseRef, useCreatePullRequestDialogFields } from './useCreatePullRequestDialogFields' import { localizedHostedReviewCopy } from '@/i18n/hosted-review-localized-copy' @@ -3906,26 +3908,18 @@ export default function ChecksPanel(): React.JSX.Element { setRightSidebarOpen(true) setRightSidebarTab('checks') try { - if (activeWorktreeId && result.provider === 'github') { - await updateWorktreeMeta(activeWorktreeId, { linkedPR: result.number }) - } - if (activeWorktreeId && result.provider === 'gitlab') { - await updateWorktreeMeta(activeWorktreeId, { linkedGitLabMR: result.number }) - } - if (activeWorktreeId && result.provider === 'azure-devops') { - await updateWorktreeMeta(activeWorktreeId, { linkedAzureDevOpsPR: result.number }) - } - if (activeWorktreeId && result.provider === 'gitea') { - await updateWorktreeMeta(activeWorktreeId, { linkedGiteaPR: result.number }) + const createdLink = resolveCreatedHostedReviewLink(result.provider, result.number) + if (activeWorktreeId && result.provider !== 'unsupported') { + await updateWorktreeMeta(activeWorktreeId, createdLink.worktree) } const linkedReviewNumbers = { - linkedGitHubPR: result.provider === 'github' ? result.number : linkedPR, + linkedGitHubPR: linkedPR, fallbackGitHubPR: fallbackGitHubPRNumber, - linkedGitLabMR: result.provider === 'gitlab' ? result.number : linkedGitLabMR, + linkedGitLabMR, linkedBitbucketPR, - linkedAzureDevOpsPR: - result.provider === 'azure-devops' ? result.number : linkedAzureDevOpsPR, - linkedGiteaPR: result.provider === 'gitea' ? result.number : linkedGiteaPR + linkedAzureDevOpsPR, + linkedGiteaPR, + ...createdLink.lookup } if (result.provider === 'gitlab') { const refreshedReview = await refreshHostedReviewCard(fetchHostedReviewForBranch, { @@ -4036,7 +4030,7 @@ export default function ChecksPanel(): React.JSX.Element { head: normalizeHostedReviewHeadRef(branch), title, body: prBody, - draft: prDraft, + draft: prDraft && hostedReviewProviderSupportsDraft(hostedReviewCreateProvider), worktreePath, useTemplate: prCreationDefaults.useTemplate }) diff --git a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx index 7caf09dd128..63c66b01ddc 100644 --- a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx +++ b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx @@ -21,7 +21,10 @@ import { resolveSupportedHostedReviewCopyProvider } from '@/i18n/hosted-review-localized-copy' import { translate } from '@/i18n/i18n' -import type { HostedReviewProvider } from '../../../../shared/hosted-review' +import { + hostedReviewProviderSupportsDraft, + type HostedReviewProvider +} from '../../../../shared/hosted-review' import { stripBaseRef } from './useCreatePullRequestDialogFields' import type { DropdownActionKind, DropdownEntry } from './source-control-dropdown-items' import { CreateHostedReviewComposerFields } from './CreateHostedReviewComposerFields' @@ -104,6 +107,9 @@ export function CreateHostedReviewComposer({ onDropdownAction }: CreateHostedReviewComposerProps): React.JSX.Element { const copy = localizedHostedReviewCopy(resolveSupportedHostedReviewCopyProvider(provider)) + // Providers without draft reviews must not carry a stale draft flag into submit. + const supportsDraft = hostedReviewProviderSupportsDraft(provider) + const effectiveDraft = supportsDraft && draft const ReviewIcon = provider === 'gitlab' ? GitMerge : GitPullRequestArrow const normalizedBase = stripBaseRef(base) const strippedBranch = stripBaseRef(branch) @@ -229,6 +235,7 @@ export function CreateHostedReviewComposer({ setBody={setBody} draft={draft} setDraft={setDraft} + supportsDraft={supportsDraft} baseQuery={baseQuery} setBaseQuery={setBaseQuery} baseResults={baseResults} @@ -265,7 +272,7 @@ export function CreateHostedReviewComposer({ {getCreateButtonLabel({ isCreating, pushBeforeCreate, - draft, + draft: effectiveDraft, shortLabel: copy.shortLabel })} diff --git a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposerFields.tsx b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposerFields.tsx index 68591818ec1..035f5b44186 100644 --- a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposerFields.tsx +++ b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposerFields.tsx @@ -14,6 +14,7 @@ type CreateHostedReviewComposerFieldsProps = { setBody: (value: string) => void draft: boolean setDraft: (value: boolean) => void + supportsDraft: boolean baseQuery: string setBaseQuery: (value: string) => void baseResults: string[] @@ -38,6 +39,7 @@ export function CreateHostedReviewComposerFields({ setBody, draft, setDraft, + supportsDraft, baseQuery, setBaseQuery, baseResults, @@ -159,25 +161,27 @@ export function CreateHostedReviewComposerFields({ - + {supportsDraft ? ( + + ) : null} {baseResults.length > 0 ? (
diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index d70ad7dc4d8..8d087ac16ad 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -246,6 +246,7 @@ import { } from './source-control-pull-policy-error-notice' import { SourceControlTextGenerationDialog } from './SourceControlTextGenerationDialog' import { CreateHostedReviewComposer } from './CreateHostedReviewComposer' +import { resolveCreatedHostedReviewLink } from './source-control-created-review-link' import { hasConfiguredCommitMessageGenerationDefaults, hasConfiguredSourceControlTextGenerationDefaults @@ -2753,26 +2754,18 @@ function SourceControlInner(): React.JSX.Element { setRightSidebarTab('checks') } try { - if (worktreeId && result.provider === 'github') { - await updateWorktreeMeta(worktreeId, { linkedPR: result.number }) - } - if (worktreeId && result.provider === 'gitlab') { - await updateWorktreeMeta(worktreeId, { linkedGitLabMR: result.number }) - } - if (worktreeId && result.provider === 'azure-devops') { - await updateWorktreeMeta(worktreeId, { linkedAzureDevOpsPR: result.number }) - } - if (worktreeId && result.provider === 'gitea') { - await updateWorktreeMeta(worktreeId, { linkedGiteaPR: result.number }) + const createdLink = resolveCreatedHostedReviewLink(result.provider, result.number) + if (worktreeId && result.provider !== 'unsupported') { + await updateWorktreeMeta(worktreeId, createdLink.worktree) } const linkedReviewNumbers = { - linkedGitHubPR: result.provider === 'github' ? result.number : linkedGitHubPR, + linkedGitHubPR, fallbackGitHubPR: fallbackGitHubPRNumber, - linkedGitLabMR: result.provider === 'gitlab' ? result.number : linkedGitLabMR, + linkedGitLabMR, linkedBitbucketPR, - linkedAzureDevOpsPR: - result.provider === 'azure-devops' ? result.number : linkedAzureDevOpsPR, - linkedGiteaPR: result.provider === 'gitea' ? result.number : linkedGiteaPR + linkedAzureDevOpsPR, + linkedGiteaPR, + ...createdLink.lookup } if (result.provider === 'gitlab') { await fetchHostedReviewForBranch(repoPath, branch, { diff --git a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts index 1e5c02efb84..fd0824d41cc 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts @@ -33,6 +33,9 @@ export function resolveHostedReviewAuthInstruction(provider: HostedReviewProvide if (provider === 'gitea') { return 'Set ORCA_GITEA_TOKEN' } + if (provider === 'bitbucket') { + return 'Connect Bitbucket in Settings > Integrations' + } return 'Run gh auth login' } diff --git a/src/renderer/src/components/right-sidebar/source-control-created-review-link.test.ts b/src/renderer/src/components/right-sidebar/source-control-created-review-link.test.ts new file mode 100644 index 00000000000..f427fcc3b7a --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-created-review-link.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from 'vitest' +import type { HostedReviewProvider } from '../../../../shared/hosted-review' +import { resolveCreatedHostedReviewLink } from './source-control-created-review-link' + +describe('resolveCreatedHostedReviewLink', () => { + it.each([ + ['github', { linkedPR: 42 }, { linkedGitHubPR: 42 }], + ['gitlab', { linkedGitLabMR: 42 }, { linkedGitLabMR: 42 }], + ['bitbucket', { linkedBitbucketPR: 42 }, { linkedBitbucketPR: 42 }], + ['azure-devops', { linkedAzureDevOpsPR: 42 }, { linkedAzureDevOpsPR: 42 }], + ['gitea', { linkedGiteaPR: 42 }, { linkedGiteaPR: 42 }] + ] as const)('links a created %s review by number', (provider, worktree, lookup) => { + expect(resolveCreatedHostedReviewLink(provider as HostedReviewProvider, 42)).toEqual({ + worktree, + lookup + }) + }) + + it('does not link an unsupported review provider', () => { + expect(resolveCreatedHostedReviewLink('unsupported', 42)).toEqual({ + worktree: {}, + lookup: {} + }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-created-review-link.ts b/src/renderer/src/components/right-sidebar/source-control-created-review-link.ts new file mode 100644 index 00000000000..0ca3d3db8ad --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-created-review-link.ts @@ -0,0 +1,45 @@ +import type { HostedReviewProvider } from '../../../../shared/hosted-review' + +type WorktreeReviewLink = Partial<{ + linkedPR: number + linkedGitLabMR: number + linkedBitbucketPR: number + linkedAzureDevOpsPR: number + linkedGiteaPR: number +}> + +type HostedReviewLookupLink = Partial<{ + linkedGitHubPR: number + linkedGitLabMR: number + linkedBitbucketPR: number + linkedAzureDevOpsPR: number + linkedGiteaPR: number +}> + +export type CreatedHostedReviewLink = { + worktree: WorktreeReviewLink + lookup: HostedReviewLookupLink +} + +export function resolveCreatedHostedReviewLink( + provider: HostedReviewProvider, + number: number +): CreatedHostedReviewLink { + switch (provider) { + case 'github': + return { worktree: { linkedPR: number }, lookup: { linkedGitHubPR: number } } + case 'gitlab': + return { worktree: { linkedGitLabMR: number }, lookup: { linkedGitLabMR: number } } + case 'azure-devops': + return { + worktree: { linkedAzureDevOpsPR: number }, + lookup: { linkedAzureDevOpsPR: number } + } + case 'gitea': + return { worktree: { linkedGiteaPR: number }, lookup: { linkedGiteaPR: number } } + case 'bitbucket': + return { worktree: { linkedBitbucketPR: number }, lookup: { linkedBitbucketPR: number } } + case 'unsupported': + return { worktree: {}, lookup: {} } + } +} diff --git a/src/renderer/src/components/right-sidebar/source-control-hosted-review-creation-eligibility-snapshot.test.ts b/src/renderer/src/components/right-sidebar/source-control-hosted-review-creation-eligibility-snapshot.test.ts index ec4031c7aed..395e9bb9a3c 100644 --- a/src/renderer/src/components/right-sidebar/source-control-hosted-review-creation-eligibility-snapshot.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-hosted-review-creation-eligibility-snapshot.test.ts @@ -189,7 +189,7 @@ describe('buildLocalBlockerHostedReviewCreationEligibility', () => { it('returns null for providers that do not support hosted review creation', () => { expect( - buildLocalBlockerHostedReviewCreationEligibility('bitbucket', { + buildLocalBlockerHostedReviewCreationEligibility('unsupported', { ...featureBranch, hasUncommittedChanges: true, hasUpstream: false, @@ -258,7 +258,7 @@ describe('buildCreatePrIntentUnavailableEligibility', () => { it('never synthesizes intent eligibility for an unsupported remote provider', () => { expect( - buildCreatePrIntentUnavailableEligibility('bitbucket', { + buildCreatePrIntentUnavailableEligibility('unsupported', { ...featureBranch, hasUncommittedChanges: true, hasUpstream: true, diff --git a/src/renderer/src/components/settings/bitbucket-credentials-dialog.tsx b/src/renderer/src/components/settings/bitbucket-credentials-dialog.tsx new file mode 100644 index 00000000000..f95fd29b31d --- /dev/null +++ b/src/renderer/src/components/settings/bitbucket-credentials-dialog.tsx @@ -0,0 +1,387 @@ +import { useId, useLayoutEffect, useState } from 'react' +import { ExternalLink, LoaderCircle, Lock } from 'lucide-react' +import type { BitbucketAuthMode } from '../../../../shared/bitbucket-credentials' +import { useAppStore } from '@/store' +import { useMountedRef } from '@/hooks/useMountedRef' +import { Button } from '@/components/ui/button' +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle +} from '@/components/ui/dialog' +import { Input } from '@/components/ui/input' +import { Label } from '@/components/ui/label' +import { ToggleGroup, ToggleGroupItem } from '@/components/ui/toggle-group' +import { cn } from '@/lib/utils' +import { hasRemoteProviderRuntime } from '@/lib/provider-runtime-context' +import { translate } from '@/i18n/i18n' + +const API_TOKEN_DOCS_URL = 'https://support.atlassian.com/bitbucket-cloud/docs/using-api-tokens/' + +type ConnectState = 'idle' | 'connecting' | 'error' + +type BitbucketCredentialsDialogProps = { + open: boolean + onOpenChange: (open: boolean) => void + /** Latest stored metadata, used to prefill the non-secret fields when editing. */ + initialAuthMode?: BitbucketAuthMode | null + initialEmail?: string | null + initialBaseUrl?: string | null + /** Env vars win over stored credentials, so saving here would have no effect. */ + environmentManaged?: boolean + onConnected?: () => void + overlayClassName?: string + contentClassName?: string +} + +export function BitbucketCredentialsDialog({ + open, + onOpenChange, + initialAuthMode, + initialEmail, + initialBaseUrl, + environmentManaged = false, + onConnected, + overlayClassName, + contentClassName +}: BitbucketCredentialsDialogProps): React.JSX.Element { + const settings = useAppStore((s) => s.settings) + const mountedRef = useMountedRef() + const emailId = useId() + const apiTokenId = useId() + const accessTokenId = useId() + const baseUrlId = useId() + const errorId = useId() + + const [authMode, setAuthMode] = useState('basic') + const [email, setEmail] = useState('') + const [apiToken, setApiToken] = useState('') + const [accessToken, setAccessToken] = useState('') + const [baseUrl, setBaseUrl] = useState('') + const [connectState, setConnectState] = useState('idle') + const [connectError, setConnectError] = useState(null) + + // Re-sync from the latest stored metadata on every open, not just on mount, so + // the Edit flow never shows values from a previous connection. Secrets always + // start empty: the main process never hands them back. + useLayoutEffect(() => { + if (!open) { + return + } + setAuthMode(initialAuthMode ?? 'basic') + setEmail(initialEmail ?? '') + setBaseUrl(initialBaseUrl ?? '') + setApiToken('') + setAccessToken('') + setConnectState('idle') + setConnectError(null) + // oxlint-disable-next-line react-hooks/exhaustive-deps -- keyed on `open` alone: a status refresh mid-edit must not overwrite what the user is typing. + }, [open]) + + const remoteRuntime = hasRemoteProviderRuntime(settings) + // Why: only an env-managed connection makes saving pointless. A remote runtime + // just means the credential is stored on this machine, so the form stays + // usable and only the storage note changes (matching the Jira dialog). + const locked = environmentManaged + const connecting = connectState === 'connecting' + const isTokenMode = authMode === 'token' + const hasRequiredFields = isTokenMode + ? Boolean(accessToken.trim()) + : Boolean(email.trim()) && Boolean(apiToken.trim()) + const canSubmit = !locked && !connecting && hasRequiredFields + + const clearErrorOnEdit = (): void => { + if (connectState === 'error') { + setConnectState('idle') + setConnectError(null) + } + } + + const handleOpenChange = (nextOpen: boolean): void => { + if (!connecting) { + onOpenChange(nextOpen) + } + } + + const handleConnect = async (): Promise => { + if (!canSubmit) { + return + } + setConnectState('connecting') + setConnectError(null) + try { + const result = await window.api.bitbucket.connect({ + authMode, + accessToken: isTokenMode ? accessToken.trim() : null, + email: isTokenMode ? null : email.trim(), + apiToken: isTokenMode ? null : apiToken.trim(), + baseUrl: baseUrl.trim() || null + }) + if (!mountedRef.current) { + return + } + if (result.ok) { + setApiToken('') + setAccessToken('') + setConnectState('idle') + onOpenChange(false) + onConnected?.() + return + } + setConnectState('error') + setConnectError(result.error) + } catch (error) { + if (mountedRef.current) { + setConnectState('error') + setConnectError( + error instanceof Error + ? error.message + : translate( + 'auto.components.settings.bitbucket.credentials.dialog.connectFailed', + 'Connection failed' + ) + ) + } + } + } + + return ( + + { + // Only from a text field: Enter on Cancel or the docs link must do + // what that control does, not submit the form. + if (!(event.target instanceof HTMLInputElement)) { + return + } + if (event.key === 'Enter' && canSubmit) { + event.preventDefault() + void handleConnect() + } + }} + > + + + {translate( + 'auto.components.settings.bitbucket.credentials.dialog.title', + 'Connect Bitbucket' + )} + + + {translate( + 'auto.components.settings.bitbucket.credentials.dialog.description', + 'Use a Bitbucket Cloud credential to browse pull requests and build statuses. Orca verifies it before saving.' + )} + + + {locked ? ( +

+ {translate( + 'auto.components.settings.bitbucket.credentials.dialog.environmentManaged', + 'Bitbucket is already configured through ORCA_BITBUCKET_* environment variables, which take precedence. Unset them to save a credential in Orca.' + )} +

+ ) : ( +
+ { + if (!value || connecting) { + return + } + setAuthMode(value as BitbucketAuthMode) + // An API token and an access token are different secrets; drop + // the typed one so it can't be submitted as the other. + setApiToken('') + setAccessToken('') + clearErrorOnEdit() + }} + aria-label={translate( + 'auto.components.settings.bitbucket.credentials.dialog.authModeLabel', + 'Bitbucket authentication method' + )} + > + + {translate( + 'auto.components.settings.bitbucket.credentials.dialog.modeBasic', + 'Email & API token' + )} + + + {translate( + 'auto.components.settings.bitbucket.credentials.dialog.modeToken', + 'Access token' + )} + + + {isTokenMode ? ( +
+ + { + setAccessToken(event.target.value) + clearErrorOnEdit() + }} + disabled={connecting} + aria-invalid={connectState === 'error'} + aria-describedby={connectState === 'error' ? errorId : undefined} + /> +
+ ) : ( + <> +
+ + { + setEmail(event.target.value) + clearErrorOnEdit() + }} + disabled={connecting} + /> +
+
+ + { + setApiToken(event.target.value) + clearErrorOnEdit() + }} + disabled={connecting} + aria-invalid={connectState === 'error'} + aria-describedby={connectState === 'error' ? errorId : undefined} + /> +
+ + )} +
+ + { + setBaseUrl(event.target.value) + clearErrorOnEdit() + }} + disabled={connecting} + /> +
+ {connectState === 'error' && connectError ? ( +

+ {connectError} +

+ ) : null} +
+

+ {isTokenMode + ? translate( + 'auto.components.settings.bitbucket.credentials.dialog.tokenHint', + 'Repository, project, and workspace access tokens are created from the matching Bitbucket settings page and need read access to pull requests.' + ) + : translate( + 'auto.components.settings.bitbucket.credentials.dialog.basicHint', + 'Create an Atlassian API token for your account, then pair it with the email address that owns it.' + )} +

+ +
+

+ + {remoteRuntime + ? translate( + 'auto.components.settings.bitbucket.credentials.dialog.remoteRuntime', + 'Stored on this machine, not on the active remote runtime — set ORCA_BITBUCKET_* there instead. Environment variables always take precedence over what you save here.' + ) + : translate( + 'auto.components.settings.bitbucket.credentials.dialog.storageNote', + 'Stored on this machine with encrypted storage when the OS keychain is available. ORCA_BITBUCKET_* environment variables always take precedence over what you save here.' + )} +

+
+ )} + + + + +
+
+ ) +} diff --git a/src/renderer/src/components/settings/bitbucket-integration-card.tsx b/src/renderer/src/components/settings/bitbucket-integration-card.tsx new file mode 100644 index 00000000000..5d8809551cb --- /dev/null +++ b/src/renderer/src/components/settings/bitbucket-integration-card.tsx @@ -0,0 +1,273 @@ +import { useCallback, useEffect, useState } from 'react' +import { ExternalLink, GitPullRequestArrow, LoaderCircle, Unlink } from 'lucide-react' +import type { BitbucketConnectionStatus } from '../../../../shared/bitbucket-credentials' +import { Button } from '@/components/ui/button' +import { useMountedRef } from '@/hooks/useMountedRef' +import { IntegrationCardDetails, IntegrationCardShell } from './integration-card-shell' +import { useIntegrationSubordinateRowClass } from './integration-card-presentation' +import type { BitbucketStatus } from './integrations-pane-status' +import { usePreflightCardStatuses } from './source-control-preflight-card-status' +import { tokenProviderStatusLabel } from './token-source-control-status' +import { BitbucketCredentialsDialog } from './bitbucket-credentials-dialog' +import { translate } from '@/i18n/i18n' + +const API_TOKEN_DOCS_URL = 'https://support.atlassian.com/bitbucket-cloud/docs/using-api-tokens/' +const DEFAULT_API_BASE_URL = 'https://api.bitbucket.org/2.0' + +export function BitbucketIntegrationCard(): React.JSX.Element { + const { statuses, unavailable, refresh } = usePreflightCardStatuses('bitbucket') + const status = unavailable ? 'unavailable' : statuses.bitbucketStatus + const connected = status === 'connected' + const mountedRef = useMountedRef() + const subordinateRowClass = useIntegrationSubordinateRowClass('flex items-center gap-3') + const [connection, setConnection] = useState(null) + const [dialogOpen, setDialogOpen] = useState(false) + const [disconnecting, setDisconnecting] = useState(false) + const [disconnectError, setDisconnectError] = useState(null) + + // Reads plaintext metadata only — never the encrypted secret — so mounting the + // pane cannot trigger a keychain prompt. + const loadConnection = useCallback(async () => { + try { + const next = await window.api.bitbucket.status() + if (mountedRef.current) { + setConnection(next) + } + } catch { + // Best-effort: the preflight-driven parts of the card still render. + } + }, [mountedRef]) + + useEffect(() => { + void loadConnection() + }, [loadConnection]) + + const envManaged = connection?.source === 'environment' + const storedCredential = connection?.source === 'stored' + const account = connection?.account ?? statuses.bitbucketAccount + // Only surface a base URL the user actually overrode; the default is noise. + const baseUrlOverride = + connection?.baseUrl && connection.baseUrl !== DEFAULT_API_BASE_URL ? connection.baseUrl : null + const authModeLabel = connection?.authMode + ? connection.authMode === 'token' + ? translate( + 'auto.components.settings.bitbucket.integration.card.authModeToken', + 'Access token' + ) + : translate( + 'auto.components.settings.bitbucket.integration.card.authModeBasic', + 'Email & API token' + ) + : null + const credentialSummary = [authModeLabel, baseUrlOverride].filter(Boolean).join(' · ') + + const handleConnected = (): void => { + void loadConnection() + refresh() + } + + const handleDisconnect = async (): Promise => { + setDisconnecting(true) + setDisconnectError(null) + try { + await window.api.bitbucket.disconnect() + } catch (error) { + // Why: disconnect rethrows when a credential file cannot be deleted. + // Unhandled, the card silently re-renders as still connected. + if (mountedRef.current) { + setDisconnectError( + error instanceof Error + ? error.message + : translate( + 'auto.components.settings.bitbucket.integration.card.disconnectFailed', + 'Could not remove the saved Bitbucket credential.' + ) + ) + } + } finally { + if (mountedRef.current) { + setDisconnecting(false) + } + void loadConnection() + refresh() + } + } + + return ( + } + name="Bitbucket" + description={ + connected + ? account + ? translate( + 'auto.components.settings.token.source.control.integration.cards.ea204f5e03', + '{{value0}} · Pull requests and build statuses', + { value0: account } + ) + : translate( + 'auto.components.settings.token.source.control.integration.cards.0fa5629dad', + 'Pull requests and build statuses' + ) + : translate( + 'auto.components.settings.bitbucket.integration.card.description', + 'Pull requests and build statuses for Bitbucket Cloud.' + ) + } + checking={status === 'checking'} + statusTone={connected ? 'connected' : 'attention'} + statusLabel={tokenProviderStatusLabel({ configured: connected, status })} + actions={ + status !== 'checking' && !envManaged ? ( + + ) : null + } + > + {status !== 'checking' ? ( + + {connected ? ( +
+
+

+ {account ?? + translate( + 'auto.components.settings.bitbucket.integration.card.accountUnknown', + 'Bitbucket Cloud' + )} +

+ {credentialSummary ? ( +

{credentialSummary}

+ ) : null} +
+ {storedCredential ? ( + + ) : null} +
+ ) : null} + {disconnectError ?

{disconnectError}

: null} + +
+ {!connected ? ( + + ) : null} + +
+
+ ) : null} + + +
+ ) +} + +function BitbucketCardNote(props: { + envManaged: boolean + status: BitbucketStatus | 'unavailable' + storedCredential: boolean +}): React.JSX.Element { + if (props.status === 'unavailable') { + return ( +

+ {translate( + 'auto.components.settings.token.source.control.integration.cards.24ac1c69dc', + 'Bitbucket status is not available in this runtime yet.' + )} +

+ ) + } + if (props.envManaged) { + return ( +

+ {translate( + 'auto.components.settings.bitbucket.integration.card.envManaged', + 'Configured via environment variables. Unset the ORCA_BITBUCKET_* variables to manage this credential in Orca.' + )} +

+ ) + } + if (props.status === 'not-authenticated') { + return ( +

+ {props.storedCredential + ? translate( + 'auto.components.settings.bitbucket.integration.card.storedAuthFailed', + 'The saved Bitbucket credential could not authenticate. Edit it, or check that the token still has pull request access.' + ) + : translate( + 'auto.components.settings.token.source.control.integration.cards.6154b02093', + 'Bitbucket credentials are configured but could not authenticate. Check the token and repository permissions, then restart Orca if environment variables changed.' + )} +

+ ) + } + if (props.storedCredential) { + return ( +

+ {translate( + 'auto.components.settings.bitbucket.integration.card.storedCredential', + 'Saved in Orca on this machine. ORCA_BITBUCKET_* environment variables take precedence when set.' + )} +

+ ) + } + return ( +

+ {translate( + 'auto.components.settings.bitbucket.integration.card.notConfigured', + 'Connect a Bitbucket Cloud account with an Atlassian API token or an access token. ORCA_BITBUCKET_* environment variables work too and take precedence.' + )} +

+ ) +} diff --git a/src/renderer/src/components/settings/integrations-search.ts b/src/renderer/src/components/settings/integrations-search.ts index 2a742728c3c..8d8b6eff1f5 100644 --- a/src/renderer/src/components/settings/integrations-search.ts +++ b/src/renderer/src/components/settings/integrations-search.ts @@ -56,8 +56,8 @@ export const getIntegrationsPaneSearchEntries = createLocalizedCatalog(() => [ 'Bitbucket Integration' ), description: translate( - 'auto.components.settings.integrations.search.c97d58a0f3', - 'Bitbucket Cloud authentication via API token environment variables.' + 'auto.components.settings.integrations.search.760e2446e0', + 'Bitbucket Cloud authentication with a saved API token or environment variables.' ), keywords: [ ...translateSearchKeyword( @@ -75,6 +75,22 @@ export const getIntegrationsPaneSearchEntries = createLocalizedCatalog(() => [ ...translateSearchKeyword( 'auto.components.settings.integrations.search.2ec2bd328c', 'api token' + ), + ...translateSearchKeyword( + 'auto.components.settings.integrations.search.3c3d3d8ffa', + 'connect' + ), + ...translateSearchKeyword( + 'auto.components.settings.integrations.search.a626990bd2', + 'disconnect' + ), + ...translateSearchKeyword( + 'auto.components.settings.integrations.search.20540996ef', + 'credentials' + ), + ...translateSearchKeyword( + 'auto.components.settings.integrations.search.af5ae87847', + 'access token' ) ] }, diff --git a/src/renderer/src/components/settings/source-control-integration-cards.tsx b/src/renderer/src/components/settings/source-control-integration-cards.tsx index f43b1003d43..a42a9e88637 100644 --- a/src/renderer/src/components/settings/source-control-integration-cards.tsx +++ b/src/renderer/src/components/settings/source-control-integration-cards.tsx @@ -4,8 +4,8 @@ export { } from './cli-source-control-integration-cards' export { deriveCliProviderCardState } from './source-control-preflight-card-status' export type { CliProviderCardState } from './source-control-preflight-card-status' +export { BitbucketIntegrationCard } from './bitbucket-integration-card' export { AzureDevOpsIntegrationCard, - BitbucketIntegrationCard, GiteaIntegrationCard } from './token-source-control-integration-cards' diff --git a/src/renderer/src/components/settings/token-source-control-integration-cards.tsx b/src/renderer/src/components/settings/token-source-control-integration-cards.tsx index 5c609f0543d..1bf0499c628 100644 --- a/src/renderer/src/components/settings/token-source-control-integration-cards.tsx +++ b/src/renderer/src/components/settings/token-source-control-integration-cards.tsx @@ -5,114 +5,6 @@ import { usePreflightCardStatuses } from './source-control-preflight-card-status import { translate } from '@/i18n/i18n' import { tokenProviderStatusLabel } from './token-source-control-status' -export function BitbucketIntegrationCard(): React.JSX.Element { - const { statuses, unavailable, refresh } = usePreflightCardStatuses('bitbucket') - const status = unavailable ? 'unavailable' : statuses.bitbucketStatus - const connected = status === 'connected' - - return ( - } - name="Bitbucket" - description={ - connected - ? statuses.bitbucketAccount - ? translate( - 'auto.components.settings.token.source.control.integration.cards.ea204f5e03', - '{{value0}} · Pull requests and build statuses', - { value0: statuses.bitbucketAccount } - ) - : translate( - 'auto.components.settings.token.source.control.integration.cards.0fa5629dad', - 'Pull requests and build statuses' - ) - : translate( - 'auto.components.settings.token.source.control.integration.cards.a924e8dcd1', - 'Pull requests and build statuses via Bitbucket Cloud API tokens.' - ) - } - checking={status === 'checking'} - statusTone={connected ? 'connected' : 'attention'} - statusLabel={tokenProviderStatusLabel({ configured: connected, status })} - > - {status !== 'checking' && !connected ? ( - -

- {status === 'unavailable' ? ( - translate( - 'auto.components.settings.token.source.control.integration.cards.24ac1c69dc', - 'Bitbucket status is not available in this runtime yet.' - ) - ) : status === 'not-configured' ? ( - <> - {translate( - 'auto.components.settings.token.source.control.integration.cards.7bbc9c64f0', - 'Set' - )}{' '} - - {translate( - 'auto.components.settings.token.source.control.integration.cards.63a7f47392', - 'ORCA_BITBUCKET_EMAIL' - )} - {' '} - {translate( - 'auto.components.settings.token.source.control.integration.cards.fc71a0e7aa', - 'and' - )}{' '} - - {translate( - 'auto.components.settings.token.source.control.integration.cards.19416c874c', - 'ORCA_BITBUCKET_API_TOKEN' - )} - - {translate( - 'auto.components.settings.token.source.control.integration.cards.087feb92f1', - ', or set' - )}{' '} - - {translate( - 'auto.components.settings.token.source.control.integration.cards.e63fe8f627', - 'ORCA_BITBUCKET_ACCESS_TOKEN' - )} - - . - - ) : ( - translate( - 'auto.components.settings.token.source.control.integration.cards.6154b02093', - 'Bitbucket credentials are configured but could not authenticate. Check the token and repository permissions, then restart Orca if environment variables changed.' - ) - )} -

-
- - -
-
- ) : null} -
- ) -} - export function AzureDevOpsIntegrationCard(): React.JSX.Element { const { statuses, unavailable, refresh } = usePreflightCardStatuses('azureDevOps') const status = unavailable ? 'unavailable' : statuses.azureDevOpsStatus diff --git a/src/renderer/src/i18n/hosted-review-localized-copy.ts b/src/renderer/src/i18n/hosted-review-localized-copy.ts index be6a6b7ad6a..2071d8aa82e 100644 --- a/src/renderer/src/i18n/hosted-review-localized-copy.ts +++ b/src/renderer/src/i18n/hosted-review-localized-copy.ts @@ -47,6 +47,14 @@ export function localizedHostedReviewCopy( providerName: 'Gitea' } } + if (provider === 'bitbucket') { + return { + shortLabel: translate('auto.i18n.hostedReview.copy.f0a4b8c2d1', 'PR'), + reviewLabel: translate('auto.i18n.hostedReview.copy.e9f3a7b1c0', 'pull request'), + titleLabel: translate('auto.i18n.hostedReview.copy.d8e2f6a0b9', 'Pull Request'), + providerName: 'Bitbucket' + } + } return { shortLabel: translate('auto.i18n.hostedReview.copy.f0a4b8c2d1', 'PR'), reviewLabel: translate('auto.i18n.hostedReview.copy.e9f3a7b1c0', 'pull request'), diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index c7d7c0b0020..0555bfbac8e 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -8738,7 +8738,9 @@ "41ccade05c": "gh", "b79c21bd42": "github", "7166b9090c": "GitHub authentication via the gh CLI.", - "f16e41cc72": "GitHub Integration" + "f16e41cc72": "GitHub Integration", + "760e2446e0": "Bitbucket Cloud authentication with a saved API token or environment variables.", + "af5ae87847": "access token" } }, "jira": { @@ -10591,6 +10593,51 @@ "on": "On", "auto": "Agent", "off": "Off" + }, + "bitbucket": { + "credentials": { + "dialog": { + "connectFailed": "Connection failed", + "title": "Connect Bitbucket", + "description": "Use a Bitbucket Cloud credential to browse pull requests and build statuses. Orca verifies it before saving.", + "remoteRuntime": "Bitbucket credentials saved here are stored on this local machine only. Set ORCA_BITBUCKET_* environment variables on the remote runtime instead.", + "environmentManaged": "Bitbucket is already configured through ORCA_BITBUCKET_* environment variables, which take precedence. Unset them to save a credential in Orca.", + "authModeLabel": "Bitbucket authentication method", + "modeBasic": "Email & API token", + "modeToken": "Access token", + "accessToken": "Access token", + "accessTokenPlaceholder": "Repository, project, or workspace access token", + "email": "Atlassian account email", + "emailPlaceholder": "you@example.com", + "apiToken": "API token", + "apiTokenPlaceholder": "Atlassian API token", + "baseUrl": "API base URL (optional)", + "baseUrlPlaceholder": "https://api.bitbucket.org/2.0", + "tokenHint": "Repository, project, and workspace access tokens are created from the matching Bitbucket settings page and need read access to pull requests.", + "basicHint": "Create an Atlassian API token for your account, then pair it with the email address that owns it.", + "docsLink": "Bitbucket API token docs", + "storageNote": "Stored on this machine with encrypted storage when the OS keychain is available. ORCA_BITBUCKET_* environment variables always take precedence over what you save here.", + "cancel": "Cancel", + "verifying": "Verifying...", + "connect": "Connect" + } + }, + "integration": { + "card": { + "description": "Pull requests and build statuses for Bitbucket Cloud.", + "edit": "Edit credentials", + "connect": "Connect", + "accountUnknown": "Bitbucket Cloud", + "authModeToken": "Access token", + "authModeBasic": "Email & API token", + "disconnect": "Disconnect Bitbucket", + "envManaged": "Configured via environment variables. Unset the ORCA_BITBUCKET_* variables to manage this credential in Orca.", + "storedAuthFailed": "The saved Bitbucket credential could not authenticate. Edit it, or check that the token still has pull request access.", + "storedCredential": "Saved in Orca on this machine. ORCA_BITBUCKET_* environment variables take precedence when set.", + "notConfigured": "Connect a Bitbucket Cloud account with an Atlassian API token or an access token. ORCA_BITBUCKET_* environment variables work too and take precedence.", + "disconnectFailed": "Could not remove the saved Bitbucket credential." + } + } } }, "right": { diff --git a/src/shared/bitbucket-credentials.ts b/src/shared/bitbucket-credentials.ts new file mode 100644 index 00000000000..90ae6223153 --- /dev/null +++ b/src/shared/bitbucket-credentials.ts @@ -0,0 +1,28 @@ +export type BitbucketAuthMode = 'token' | 'basic' + +// Where the active credential comes from. Drives whether the UI offers +// Disconnect, which is only meaningful for in-app `stored` credentials. +export type BitbucketCredentialSource = 'environment' | 'stored' | 'none' + +export type BitbucketConnectArgs = { + authMode: BitbucketAuthMode + accessToken?: string | null + email?: string | null + apiToken?: string | null + baseUrl?: string | null +} + +// Deliberately excludes the secret: it never crosses the IPC boundary back to +// the renderer. +export type BitbucketConnectionStatus = { + configured: boolean + source: BitbucketCredentialSource + account: string | null + authMode: BitbucketAuthMode | null + email: string | null + baseUrl: string | null +} + +export type BitbucketConnectResult = + | { ok: true; account: string | null } + | { ok: false; error: string } diff --git a/src/shared/hosted-review-creation-providers.test.ts b/src/shared/hosted-review-creation-providers.test.ts new file mode 100644 index 00000000000..67e62aad54b --- /dev/null +++ b/src/shared/hosted-review-creation-providers.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest' +import { + resolveHostedReviewCreationProvider, + supportsHostedReviewCreation +} from './hosted-review-creation-providers' + +describe('hosted review creation providers', () => { + // Why: this list is the gate behind the "provider does not support creating a + // pull request" blocker. A provider whose forge adapter implements + // createReview but is missing here reports unsupported to the user forever. + it('accepts every provider Orca can create a review on', () => { + for (const provider of ['github', 'gitlab', 'bitbucket', 'azure-devops', 'gitea'] as const) { + expect(supportsHostedReviewCreation(provider)).toBe(true) + expect(resolveHostedReviewCreationProvider(provider)).toBe(provider) + } + }) + + it('rejects unsupported and missing providers, falling back to GitHub copy', () => { + expect(supportsHostedReviewCreation('unsupported')).toBe(false) + expect(supportsHostedReviewCreation(null)).toBe(false) + expect(supportsHostedReviewCreation(undefined)).toBe(false) + expect(resolveHostedReviewCreationProvider('unsupported')).toBe('github') + }) +}) diff --git a/src/shared/hosted-review-creation-providers.ts b/src/shared/hosted-review-creation-providers.ts index b32cfebfbd9..bbe0dc6b912 100644 --- a/src/shared/hosted-review-creation-providers.ts +++ b/src/shared/hosted-review-creation-providers.ts @@ -1,6 +1,11 @@ import type { HostedReviewProvider } from './hosted-review' -export type HostedReviewCreationProvider = 'github' | 'gitlab' | 'azure-devops' | 'gitea' +export type HostedReviewCreationProvider = + | 'github' + | 'gitlab' + | 'bitbucket' + | 'azure-devops' + | 'gitea' export function supportsHostedReviewCreation( provider: HostedReviewProvider | null | undefined @@ -8,6 +13,7 @@ export function supportsHostedReviewCreation( return ( provider === 'github' || provider === 'gitlab' || + provider === 'bitbucket' || provider === 'azure-devops' || provider === 'gitea' ) diff --git a/src/shared/hosted-review.ts b/src/shared/hosted-review.ts index 0d25a637e41..49a64df4b72 100644 --- a/src/shared/hosted-review.ts +++ b/src/shared/hosted-review.ts @@ -16,6 +16,12 @@ export type HostedReviewProvider = export type HostedReviewState = 'open' | 'closed' | 'merged' | 'draft' +// Why: Bitbucket Cloud's API has no draft pull requests, so offering the toggle +// there would either publish a live PR or fail at submit. +export function hostedReviewProviderSupportsDraft(provider: HostedReviewProvider): boolean { + return provider !== 'bitbucket' +} + /** A linked review is identified by a positive integer PR/MR number. */ export function isPositiveHostedReviewNumber(value: unknown): value is number { return typeof value === 'number' && Number.isInteger(value) && value > 0 diff --git a/src/shared/integration-credential-errors.ts b/src/shared/integration-credential-errors.ts index 3a2cc55613d..c997802becc 100644 --- a/src/shared/integration-credential-errors.ts +++ b/src/shared/integration-credential-errors.ts @@ -1,4 +1,4 @@ -export type IntegrationCredentialService = 'Linear' | 'Jira' +export type IntegrationCredentialService = 'Linear' | 'Jira' | 'Bitbucket' export function credentialDecryptionMessage(service: IntegrationCredentialService): string { return `Could not decrypt saved ${service} credential. Approve Keychain access or reconnect ${service}.` @@ -10,6 +10,7 @@ export function isIntegrationCredentialDecryptionError(error: unknown): boolean const message = error instanceof Error ? error.message : String(error) return ( message.includes(credentialDecryptionMessage('Linear')) || - message.includes(credentialDecryptionMessage('Jira')) + message.includes(credentialDecryptionMessage('Jira')) || + message.includes(credentialDecryptionMessage('Bitbucket')) ) } From 077f5a11cd45d00ac871de6cd52ef8e3fa1cbb0a Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:48:57 -0700 Subject: [PATCH 004/134] feat(github): create stacked pull requests (#13750) Adds GitHub stacked pull request creation: a contextual "Stack this PR above #N" option that appears only when the selected base branch has an open PR, plus the main-process stack preflight and registration. Also reworks the create-review composer for cohesion: shadcn Checkbox and Label primitives, base label above a full-width searchable combobox with attached results, keyboard navigation, and a unified field skin, spacing and typography scale. Verified end to end against real GitHub: extending an existing stack and creating a new one. --- src/main/github/github-stack-api-responses.ts | 52 +++ src/main/github/stacked-pr-creation.test.ts | 273 ++++++++++++++ src/main/github/stacked-pr-creation.ts | 310 +++++++++++++++ src/main/ipc/hosted-review.test.ts | 36 ++ src/main/ipc/hosted-review.ts | 42 +++ src/main/runtime/orca-runtime.test.ts | 52 +++ src/main/runtime/orca-runtime.ts | 33 ++ .../runtime/rpc/methods/hosted-review.test.ts | 35 ++ src/main/runtime/rpc/methods/hosted-review.ts | 16 + src/main/runtime/runtime-rpc.ts | 1 + ...hosted-review-creation-eligibility.test.ts | 26 +- .../source-control/hosted-review-creation.ts | 11 +- .../stacked-hosted-review-creation.test.ts | 92 +++++ .../stacked-hosted-review-creation.ts | 61 +++ src/preload/api-types.ts | 3 + src/preload/index.ts | 4 +- .../components/right-sidebar/ChecksPanel.tsx | 353 ++++++++++-------- .../CreateHostedReviewBasePicker.tsx | 292 +++++++++++++++ .../CreateHostedReviewComposer.tsx | 77 ++-- .../CreateHostedReviewComposerFields.tsx | 308 +++++++-------- .../CreateHostedReviewComposerMessage.tsx | 15 + ...lRequestComposer.generate-tooltip.test.tsx | 338 ++++++++++++++++- .../right-sidebar/SourceControl.tsx | 334 +++++++++-------- .../create-hosted-review-button-label.ts | 54 +++ ...eate-hosted-review-composer-field-class.ts | 13 + .../create-pull-request-review-copy.ts | 7 +- ...llRequestDialogFields.repo-default.test.ts | 145 +++++++ .../useCreatePullRequestDialogFields.ts | 35 +- .../useHostedReviewStackParent.test.tsx | 154 ++++++++ .../useHostedReviewStackParent.ts | 92 +++++ src/renderer/src/i18n/locales/en.json | 22 ++ .../src/store/slices/hosted-review.test.ts | 67 +++- .../src/store/slices/hosted-review.ts | 36 ++ src/shared/hosted-review.ts | 25 ++ 34 files changed, 2876 insertions(+), 538 deletions(-) create mode 100644 src/main/github/github-stack-api-responses.ts create mode 100644 src/main/github/stacked-pr-creation.test.ts create mode 100644 src/main/github/stacked-pr-creation.ts create mode 100644 src/main/source-control/stacked-hosted-review-creation.test.ts create mode 100644 src/main/source-control/stacked-hosted-review-creation.ts create mode 100644 src/renderer/src/components/right-sidebar/CreateHostedReviewBasePicker.tsx create mode 100644 src/renderer/src/components/right-sidebar/CreateHostedReviewComposerMessage.tsx create mode 100644 src/renderer/src/components/right-sidebar/create-hosted-review-button-label.ts create mode 100644 src/renderer/src/components/right-sidebar/create-hosted-review-composer-field-class.ts create mode 100644 src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.repo-default.test.ts create mode 100644 src/renderer/src/components/right-sidebar/useHostedReviewStackParent.test.tsx create mode 100644 src/renderer/src/components/right-sidebar/useHostedReviewStackParent.ts diff --git a/src/main/github/github-stack-api-responses.ts b/src/main/github/github-stack-api-responses.ts new file mode 100644 index 00000000000..727d72331ef --- /dev/null +++ b/src/main/github/github-stack-api-responses.ts @@ -0,0 +1,52 @@ +import type { HostedReviewSummary } from '../../shared/hosted-review' + +export type NumberedHostedReviewSummary = Omit & { number: number } + +export type GitHubStackPullRequest = NumberedHostedReviewSummary & { + headRefName: string + baseRefName: string +} + +export type GitHubStack = { + number: number + open: boolean + pull_requests: { number: number }[] +} + +export function parseGitHubStackPullRequests(stdout: string): GitHubStackPullRequest[] { + const pullRequests = JSON.parse(stdout) as { + number?: unknown + html_url?: unknown + head?: { ref?: unknown } + base?: { ref?: unknown } + }[] + return pullRequests.flatMap((pullRequest) => { + const number = Number(pullRequest.number) + const url = typeof pullRequest.html_url === 'string' ? pullRequest.html_url : '' + const headRefName = typeof pullRequest.head?.ref === 'string' ? pullRequest.head.ref : '' + const baseRefName = typeof pullRequest.base?.ref === 'string' ? pullRequest.base.ref : '' + return Number.isInteger(number) && number > 0 && url && headRefName && baseRefName + ? [{ number, url, headRefName, baseRefName }] + : [] + }) +} + +export function parseGitHubStacks(stdout: string): GitHubStack[] { + const stacks = JSON.parse(stdout) as { + number?: unknown + open?: unknown + pull_requests?: { number?: unknown }[] + }[] + return stacks.flatMap((stack) => { + const number = Number(stack.number) + const pullRequests = (stack.pull_requests ?? []).flatMap((pullRequest) => { + const pullRequestNumber = Number(pullRequest.number) + return Number.isInteger(pullRequestNumber) && pullRequestNumber > 0 + ? [{ number: pullRequestNumber }] + : [] + }) + return Number.isInteger(number) && number > 0 + ? [{ number, open: stack.open === true, pull_requests: pullRequests }] + : [] + }) +} diff --git a/src/main/github/stacked-pr-creation.test.ts b/src/main/github/stacked-pr-creation.test.ts new file mode 100644 index 00000000000..e97ea14f2a4 --- /dev/null +++ b/src/main/github/stacked-pr-creation.test.ts @@ -0,0 +1,273 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { ghExecFileAsyncMock, repositoryMock } = vi.hoisted(() => ({ + ghExecFileAsyncMock: vi.fn(), + repositoryMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + ghExecFileAsync: ghExecFileAsyncMock, + ghRepoExecOptions: (context: { repoPath: string; connectionId?: string | null }) => + context.connectionId ? {} : { cwd: context.repoPath }, + githubRepoContext: ( + repoPath: string, + connectionId?: string | null, + localGitOptions?: Record + ) => ({ repoPath, connectionId, localGitOptions }) +})) + +vi.mock('./github-api-repository', () => ({ + getOriginGitHubApiRepository: repositoryMock, + githubHostExecOptions: (repository: { host?: string }) => ({ host: repository.host }) +})) + +import { + prepareGitHubStackedPullRequest, + registerGitHubStackedPullRequest +} from './stacked-pr-creation' + +const repository = { owner: 'acme', repo: 'orca', host: 'github.com' } +const parentReview = { number: 41, url: 'https://github.com/acme/orca/pull/41' } +const currentReview = { number: 42, url: 'https://github.com/acme/orca/pull/42' } + +function pullRequest(number: number, head: string, base: string) { + return { + number, + html_url: `https://github.com/acme/orca/pull/${number}`, + head: { ref: head }, + base: { ref: base } + } +} + +function stack(number: number, pullRequests: number[]) { + return { + number, + open: true, + pull_requests: pullRequests.map((pullRequestNumber) => ({ number: pullRequestNumber })) + } +} + +beforeEach(() => { + ghExecFileAsyncMock.mockReset() + repositoryMock.mockReset() + repositoryMock.mockResolvedValue(repository) +}) + +describe('prepareGitHubStackedPullRequest', () => { + it('resolves an open parent PR and an existing current PR', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([pullRequest(42, 'stack/child', 'stack/parent')]) + }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [40, 41])]) }) + .mockResolvedValueOnce({ stdout: '[]' }) + + const result = await prepareGitHubStackedPullRequest('/repo', { + provider: 'github', + base: 'origin/stack/parent', + head: 'refs/heads/stack/child', + title: 'Child' + }) + + expect(result).toMatchObject({ + ok: true, + parentReview: { number: 41 }, + currentReview: { number: 42 } + }) + expect(ghExecFileAsyncMock.mock.calls[0][0]).toEqual([ + 'api', + 'repos/acme/orca/pulls?head=acme%3Astack%2Fparent&state=open&per_page=2' + ]) + expect(ghExecFileAsyncMock.mock.calls[1][0]).toEqual([ + 'api', + 'repos/acme/orca/pulls?head=acme%3Astack%2Fchild&base=stack%2Fparent&state=open&per_page=2' + ]) + }) + + it('allows an idempotent retry after the child was already registered', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([pullRequest(42, 'stack/child', 'stack/parent')]) + }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) }) + + const result = await prepareGitHubStackedPullRequest('/repo', { + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + + expect(result).toMatchObject({ ok: true, currentReview: { number: 42 } }) + }) + + it('requires an open PR for the selected parent branch', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: '[]' }) + + const result = await prepareGitHubStackedPullRequest('/repo', { + provider: 'github', + base: 'feature/parent', + head: 'feature/child', + title: 'Child' + }) + + expect(result).toMatchObject({ ok: false, code: 'validation' }) + if (!result.ok) { + expect(result.error).toContain('does not have an open pull request') + } + }) + + it('rejects a parent that is not the top of its stack', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([pullRequest(41, 'stack/parent', 'main')]) }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 45])]) }) + + const result = await prepareGitHubStackedPullRequest('/repo', { + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + + expect(result).toMatchObject({ ok: false, code: 'validation' }) + if (!result.ok) { + expect(result.error).toContain('top pull request') + } + }) + + it('does not offer stacks on GitHub Enterprise Server', async () => { + repositoryMock.mockResolvedValue({ + owner: 'acme', + repo: 'orca', + host: 'github.acme.test' + }) + + const result = await prepareGitHubStackedPullRequest('/repo', { + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + + expect(result).toMatchObject({ ok: false, code: 'validation' }) + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) + +describe('registerGitHubStackedPullRequest', () => { + it('creates a new stack with the parent and current PR', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: JSON.stringify({ number: 50 }) }) + + const result = await registerGitHubStackedPullRequest({ + repoPath: '/repo', + repository, + parentReview, + currentReview + }) + + expect(result).toMatchObject({ ok: true, number: 42, stackNumber: 50 }) + expect(ghExecFileAsyncMock.mock.calls[2][0]).toEqual([ + 'api', + '-X', + 'POST', + 'repos/acme/orca/stacks', + '-F', + 'pull_requests[]=41', + '-F', + 'pull_requests[]=42' + ]) + }) + + it('appends the current PR when the parent is the existing top', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [40, 41])]) }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: JSON.stringify({ number: 50 }) }) + + const result = await registerGitHubStackedPullRequest({ + repoPath: '/repo', + repository, + parentReview, + currentReview, + connectionId: 'ssh-1' + }) + + expect(result).toMatchObject({ ok: true, stackNumber: 50 }) + expect(ghExecFileAsyncMock.mock.calls[2][0]).toEqual([ + 'api', + '-X', + 'POST', + 'repos/acme/orca/stacks/50/add', + '-F', + 'pull_requests[]=42' + ]) + expect(ghExecFileAsyncMock.mock.calls[2][1]).not.toHaveProperty('cwd') + }) + + it('treats an already registered parent-child pair as success', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [41, 42])]) }) + + const result = await registerGitHubStackedPullRequest({ + repoPath: '/repo', + repository, + parentReview, + currentReview + }) + + expect(result).toMatchObject({ ok: true, stackNumber: 50 }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not claim registration when the stack no longer holds the parent', async () => { + // A concurrent stack edit can drop the parent while the child sits at index 0. + // Reading index 0 off a findIndex miss would report that pair as registered. + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [42])]) }) + .mockResolvedValueOnce({ stdout: JSON.stringify([stack(50, [42])]) }) + + const result = await registerGitHubStackedPullRequest({ + repoPath: '/repo', + repository, + parentReview, + currentReview + }) + + expect(result).toMatchObject({ + ok: false, + error: 'The pull request already belongs to a different GitHub stack.', + createdReview: currentReview + }) + }) + + it('preserves the created PR when registration fails', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockRejectedValueOnce(new Error('HTTP 422')) + + const result = await registerGitHubStackedPullRequest({ + repoPath: '/repo', + repository, + parentReview, + currentReview + }) + + expect(result).toMatchObject({ + ok: false, + createdReview: currentReview + }) + }) +}) diff --git a/src/main/github/stacked-pr-creation.ts b/src/main/github/stacked-pr-creation.ts new file mode 100644 index 00000000000..357f78bcbc6 --- /dev/null +++ b/src/main/github/stacked-pr-creation.ts @@ -0,0 +1,310 @@ +import type { + CreateStackedHostedReviewInput, + CreateStackedHostedReviewResult +} from '../../shared/hosted-review' +import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' +import { + normalizeHostedReviewBaseRef, + normalizeHostedReviewHeadRef +} from '../../shared/hosted-review-refs' +import { acquire, ghExecFileAsync, ghRepoExecOptions, githubRepoContext, release } from './gh-utils' +import { + getOriginGitHubApiRepository, + githubHostExecOptions, + type GitHubApiRepository +} from './github-api-repository' +import { + getHostedReviewLocalGitOptions, + type HostedReviewExecutionOptions +} from '../source-control/hosted-review-git-options' +import { + parseGitHubStackPullRequests, + parseGitHubStacks, + type GitHubStack, + type GitHubStackPullRequest, + type NumberedHostedReviewSummary +} from './github-stack-api-responses' + +type StackedPullRequestPlan = + | { + ok: true + repository: GitHubApiRepository + parentReview: GitHubStackPullRequest + currentReview: GitHubStackPullRequest | null + } + | Extract + +function creationError(error: string): Extract { + return { ok: false, code: 'validation', error } +} + +function isStacksUnavailableError(error: unknown): boolean { + const message = error instanceof Error ? error.message.toLowerCase() : String(error).toLowerCase() + return message.includes('http 404') || message.includes('feature not supported') +} + +function ghOptions( + repoPath: string, + repository: GitHubApiRepository, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +) { + return { + ...ghRepoExecOptions( + githubRepoContext(repoPath, connectionId, getHostedReviewLocalGitOptions(options)) + ), + ...githubHostExecOptions(repository), + timeout: 60_000 + } +} + +async function findOpenPullRequestsForBranch( + repoPath: string, + repository: GitHubApiRepository, + branch: string, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {}, + base?: string +): Promise { + const head = encodeURIComponent(`${repository.owner}:${branch}`) + const baseQuery = base ? `&base=${encodeURIComponent(base)}` : '' + const endpoint = `repos/${repository.owner}/${repository.repo}/pulls?head=${head}${baseQuery}&state=open&per_page=2` + const { stdout } = await ghExecFileAsync( + ['api', endpoint], + ghOptions(repoPath, repository, connectionId, options) + ) + return parseGitHubStackPullRequests(stdout) +} + +async function getStacksForPullRequest( + repoPath: string, + repository: GitHubApiRepository, + pullRequestNumber: number, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + const endpoint = `repos/${repository.owner}/${repository.repo}/stacks?pull_request=${pullRequestNumber}` + const { stdout } = await ghExecFileAsync( + ['api', endpoint], + ghOptions(repoPath, repository, connectionId, options) + ) + return parseGitHubStacks(stdout) +} + +function validateParentStack( + parentReview: NumberedHostedReviewSummary, + stacks: GitHubStack[] +): Extract | null { + if (stacks.length > 1) { + return creationError('The selected parent pull request belongs to multiple stacks.') + } + const stack = stacks[0] + if (!stack) { + return null + } + if (!stack.open) { + return creationError('The selected parent belongs to a closed stack.') + } + if (stack.pull_requests.at(-1)?.number !== parentReview.number) { + return creationError( + 'Choose the top pull request in the stack as the base branch before adding another layer.' + ) + } + return null +} + +export async function prepareGitHubStackedPullRequest( + repoPath: string, + input: CreateStackedHostedReviewInput, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + if (input.provider !== 'github') { + return creationError('Stacked pull request creation is available only for GitHub repositories.') + } + const repository = await getOriginGitHubApiRepository( + repoPath, + connectionId, + getHostedReviewLocalGitOptions(options) + ) + if (!repository || !isDefaultGitHubHost(repository.host)) { + return creationError('GitHub stacked pull requests are available only on GitHub.com.') + } + const base = normalizeHostedReviewBaseRef(input.base).trim() + const head = input.head ? normalizeHostedReviewHeadRef(input.head).trim() : '' + if (!base || !head || base.toLowerCase() === head.toLowerCase()) { + return creationError('Choose a different parent branch before creating a stacked pull request.') + } + + await acquire() + try { + const [parentPullRequests, currentPullRequests] = await Promise.all([ + findOpenPullRequestsForBranch(repoPath, repository, base, connectionId, options), + findOpenPullRequestsForBranch(repoPath, repository, head, connectionId, options, base) + ]) + if (parentPullRequests.length === 0) { + return creationError( + `The parent branch ${base} does not have an open pull request. Create that pull request first.` + ) + } + if (parentPullRequests.length !== 1) { + return creationError(`Orca found multiple open pull requests for the parent branch ${base}.`) + } + if (currentPullRequests.length > 1) { + return creationError(`Orca found multiple open pull requests for the current branch ${head}.`) + } + const parentReview = parentPullRequests[0] + const currentReview = currentPullRequests[0] ?? null + const [parentStacks, currentStacks] = await Promise.all([ + getStacksForPullRequest(repoPath, repository, parentReview.number, connectionId, options), + currentReview + ? getStacksForPullRequest(repoPath, repository, currentReview.number, connectionId, options) + : Promise.resolve([]) + ]) + if ( + currentReview && + registeredStackNumber(parentReview, currentReview, parentStacks, currentStacks) + ) { + return { ok: true, repository, parentReview, currentReview } + } + if (currentStacks.length > 0) { + return creationError('The pull request already belongs to a different GitHub stack.') + } + const parentError = validateParentStack(parentReview, parentStacks) + return ( + parentError ?? { + ok: true, + repository, + parentReview, + currentReview + } + ) + } catch (error) { + console.warn('GitHub stack creation preflight failed:', error) + return { + ok: false, + code: isStacksUnavailableError(error) ? 'validation' : 'unknown', + error: isStacksUnavailableError(error) + ? 'GitHub stacked pull requests are not available for this repository.' + : 'Orca could not verify the parent pull request. Retry in a moment.' + } + } finally { + release() + } +} + +function registeredStackNumber( + parentReview: NumberedHostedReviewSummary, + currentReview: NumberedHostedReviewSummary, + parentStacks: GitHubStack[], + currentStacks: GitHubStack[] +): number | null { + const parentStack = parentStacks[0] + const currentStack = currentStacks[0] + if (!parentStack || !currentStack || parentStack.number !== currentStack.number) { + return null + } + const parentPosition = parentStack.pull_requests.findIndex( + (pullRequest) => pullRequest.number === parentReview.number + ) + // Why: a miss is -1, and -1 + 1 reads the first entry — which reports "already + // registered" whenever the current PR heads a stack the parent has left. + if (parentPosition < 0) { + return null + } + return parentStack.pull_requests[parentPosition + 1]?.number === currentReview.number + ? parentStack.number + : null +} + +export async function registerGitHubStackedPullRequest(args: { + repoPath: string + repository: GitHubApiRepository + parentReview: NumberedHostedReviewSummary + currentReview: NumberedHostedReviewSummary + connectionId?: string | null + options?: HostedReviewExecutionOptions +}): Promise { + const options = args.options ?? {} + await acquire() + try { + const [parentStacks, currentStacks] = await Promise.all([ + getStacksForPullRequest( + args.repoPath, + args.repository, + args.parentReview.number, + args.connectionId, + options + ), + getStacksForPullRequest( + args.repoPath, + args.repository, + args.currentReview.number, + args.connectionId, + options + ) + ]) + const existingStackNumber = registeredStackNumber( + args.parentReview, + args.currentReview, + parentStacks, + currentStacks + ) + if (existingStackNumber) { + return { + ok: true, + ...args.currentReview, + stackNumber: existingStackNumber, + parentReview: args.parentReview + } + } + if (currentStacks.length > 0) { + return { + ...creationError('The pull request already belongs to a different GitHub stack.'), + createdReview: args.currentReview + } + } + const parentError = validateParentStack(args.parentReview, parentStacks) + if (parentError) { + return { ...parentError, createdReview: args.currentReview } + } + + const parentStack = parentStacks[0] + const endpoint = parentStack + ? `repos/${args.repository.owner}/${args.repository.repo}/stacks/${parentStack.number}/add` + : `repos/${args.repository.owner}/${args.repository.repo}/stacks` + const pullRequests = parentStack + ? [args.currentReview.number] + : [args.parentReview.number, args.currentReview.number] + const command = ['api', '-X', 'POST', endpoint] + for (const pullRequest of pullRequests) { + command.push('-F', `pull_requests[]=${pullRequest}`) + } + const { stdout } = await ghExecFileAsync(command, { + ...ghOptions(args.repoPath, args.repository, args.connectionId, options), + idempotent: false + }) + const stackNumber = Number((JSON.parse(stdout) as { number?: unknown }).number) + if (!Number.isInteger(stackNumber) || stackNumber <= 0) { + throw new Error('GitHub returned an invalid stack response.') + } + return { + ok: true, + ...args.currentReview, + stackNumber, + parentReview: args.parentReview + } + } catch (error) { + console.warn('GitHub stack registration failed:', error) + return { + ok: false, + code: isStacksUnavailableError(error) ? 'validation' : 'unknown', + error: isStacksUnavailableError(error) + ? 'The pull request was created, but GitHub stacks are not available for this repository.' + : 'The pull request was created, but GitHub could not add it to the stack. Retry to finish stack registration.', + createdReview: args.currentReview + } + } finally { + release() + } +} diff --git a/src/main/ipc/hosted-review.test.ts b/src/main/ipc/hosted-review.test.ts index cef5d7f1d66..0399446e462 100644 --- a/src/main/ipc/hosted-review.test.ts +++ b/src/main/ipc/hosted-review.test.ts @@ -13,6 +13,7 @@ function setPlatform(platform: NodeJS.Platform): void { const { handleMock, createHostedReviewMock, + createStackedHostedReviewMock, getHostedReviewCreationEligibilityMock, getHostedReviewForBranchMock, resolveRegisteredWorktreePathMock, @@ -20,6 +21,7 @@ const { } = vi.hoisted(() => ({ handleMock: vi.fn(), createHostedReviewMock: vi.fn(), + createStackedHostedReviewMock: vi.fn(), getHostedReviewCreationEligibilityMock: vi.fn(), getHostedReviewForBranchMock: vi.fn(), resolveRegisteredWorktreePathMock: vi.fn(), @@ -37,6 +39,10 @@ vi.mock('../source-control/hosted-review-creation', () => ({ getHostedReviewCreationEligibility: getHostedReviewCreationEligibilityMock })) +vi.mock('../source-control/stacked-hosted-review-creation', () => ({ + createStackedHostedReview: createStackedHostedReviewMock +})) + vi.mock('../source-control/hosted-review', () => ({ getHostedReviewForBranch: getHostedReviewForBranchMock })) @@ -87,6 +93,7 @@ describe('registerHostedReviewHandlers', () => { setPlatform(ORIGINAL_PLATFORM) handleMock.mockReset() createHostedReviewMock.mockReset() + createStackedHostedReviewMock.mockReset() getHostedReviewCreationEligibilityMock.mockReset() getHostedReviewForBranchMock.mockReset() resolveRegisteredWorktreePathMock.mockReset() @@ -384,6 +391,35 @@ describe('registerHostedReviewHandlers', () => { ) }) + it('routes stacked creation through its dedicated SSH-safe handler', async () => { + createStackedHostedReviewMock.mockResolvedValueOnce({ + ok: true, + number: 43, + url: 'https://github.com/acme/orca/pull/43', + stackNumber: 50, + parentReview: { number: 42, url: 'https://github.com/acme/orca/pull/42' } + }) + registerHostedReviewHandlers(store as never, stats as never) + + await handlers['hostedReview:createStacked'](null, { + repoPath, + repoId: repo.id, + worktreePath, + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + + expect(createStackedHostedReviewMock).toHaveBeenCalledWith( + worktreePath, + expect.objectContaining({ base: 'stack/parent', head: 'stack/child' }), + 'ssh-1', + {} + ) + expect(createHostedReviewMock).not.toHaveBeenCalled() + }) + it('rejects creation when repoId and repoPath point at different registered repos', async () => { store.getRepo.mockImplementation((repoId: string) => repoId === repo.id ? { ...repo, path: '/other/repo' } : null diff --git a/src/main/ipc/hosted-review.ts b/src/main/ipc/hosted-review.ts index 670064d3e54..736668e1f0d 100644 --- a/src/main/ipc/hosted-review.ts +++ b/src/main/ipc/hosted-review.ts @@ -2,6 +2,7 @@ import { ipcMain } from 'electron' import { posix, resolve } from 'node:path' import type { CreateHostedReviewArgs, + CreateStackedHostedReviewArgs, HostedReviewCreationEligibilityArgs, HostedReviewForBranchArgs } from '../../shared/hosted-review' @@ -12,6 +13,7 @@ import { createHostedReview, getHostedReviewCreationEligibility } from '../source-control/hosted-review-creation' +import { createStackedHostedReview } from '../source-control/stacked-hosted-review-creation' import { getHostedReviewForBranch } from '../source-control/hosted-review' import { resolveRegisteredWorktreePath } from './filesystem-auth' import { listRepoWorktrees } from '../repo-worktrees' @@ -160,4 +162,44 @@ export function registerHostedReviewHandlers(store: Store, stats: StatsCollector } return result }) + + ipcMain.handle( + 'hostedReview:createStacked', + async (_event, args: CreateStackedHostedReviewArgs) => { + const repo = assertRegisteredRepo(args.repoPath, store, args.repoId) + const worktreePath = await resolveHostedReviewWorktreePath(repo, store, args.worktreePath) + const localGitOptions = getLocalProjectWorktreeGitOptions(store, repo) + const sharedLinkPaths = repo.connectionId ? [] : getWorktreeSharedLinkPaths(repo) + const executionOptions = { + ...(Object.keys(localGitOptions).length > 0 + ? { localGitExecOptions: localGitOptions } + : {}), + ...(sharedLinkPaths.length > 0 ? { sharedLinkPaths } : {}) + } + const input = { + provider: args.provider, + base: args.base, + head: args.head, + title: args.title, + body: args.body, + draft: args.draft, + ...(args.useTemplate !== undefined ? { useTemplate: args.useTemplate } : {}) + } + const result = await createStackedHostedReview( + worktreePath, + input, + repo.connectionId ?? null, + executionOptions + ) + if (result.ok && !stats.hasCountedPR(result.url)) { + stats.record({ + type: 'pr_created', + at: Date.now(), + repoId: repo.id, + meta: { prNumber: result.number, prUrl: result.url } + }) + } + return result + } + ) } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index aa40c7ee64b..e5bacc5eb9e 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -234,6 +234,7 @@ const { invalidateAuthorizedRootsCacheMock, prepareLocalWorktreeRootForRepoMock, createHostedReviewMock, + createStackedHostedReviewMock, getHostedReviewCreationEligibilityMock, getHostedReviewForBranchMock, getPRForBranchMock, @@ -341,6 +342,7 @@ const { invalidateAuthorizedRootsCacheMock: vi.fn(), prepareLocalWorktreeRootForRepoMock: vi.fn(), createHostedReviewMock: vi.fn(), + createStackedHostedReviewMock: vi.fn(), getHostedReviewCreationEligibilityMock: vi.fn(), getHostedReviewForBranchMock: vi.fn(), getPRForBranchMock: vi.fn().mockResolvedValue(null), @@ -513,6 +515,10 @@ vi.mock('../source-control/hosted-review-creation', () => ({ getHostedReviewCreationEligibility: getHostedReviewCreationEligibilityMock })) +vi.mock('../source-control/stacked-hosted-review-creation', () => ({ + createStackedHostedReview: createStackedHostedReviewMock +})) + vi.mock('../source-control/hosted-review', () => ({ getHostedReviewForBranch: getHostedReviewForBranchMock })) @@ -732,6 +738,14 @@ function resetRuntimeTestMocks(): void { number: 1, url: 'https://example.com/pull/1' }) + createStackedHostedReviewMock.mockReset() + createStackedHostedReviewMock.mockResolvedValue({ + ok: true, + number: 2, + url: 'https://example.com/pull/2', + stackNumber: 10, + parentReview: { number: 1, url: 'https://example.com/pull/1' } + }) getHostedReviewCreationEligibilityMock.mockReset() getHostedReviewCreationEligibilityMock.mockResolvedValue({ provider: 'github', @@ -7005,6 +7019,15 @@ describe('OrcaRuntimeService', () => { body: '', draft: false }) + await runtime.createStackedHostedReview({ + repoSelector: `id:${TEST_REPO_ID}`, + provider: 'github', + base: 'stack/parent', + head: 'feature/ssh', + title: 'Feature SSH', + body: '', + draft: false + }) expect(getHostedReviewCreationEligibilityMock).toHaveBeenCalledWith( expect.objectContaining({ @@ -7022,6 +7045,16 @@ describe('OrcaRuntimeService', () => { }), 'ssh-1' ) + expect(createStackedHostedReviewMock).toHaveBeenCalledWith( + '/remote/repo', + expect.objectContaining({ + provider: 'github', + base: 'stack/parent', + head: 'feature/ssh' + }), + 'ssh-1', + {} + ) }) it('routes local WSL project hosted review flows through runtime git options', async () => { @@ -7084,6 +7117,15 @@ describe('OrcaRuntimeService', () => { body: '', draft: false }) + await runtime.createStackedHostedReview({ + repoSelector: `id:${TEST_REPO_ID}`, + provider: 'github', + base: 'stack/parent', + head: 'feature/wsl', + title: 'Feature WSL', + body: '', + draft: false + }) expect(getHostedReviewCreationEligibilityMock).toHaveBeenCalledWith( expect.objectContaining({ @@ -7112,6 +7154,16 @@ describe('OrcaRuntimeService', () => { null, { localGitExecOptions: { wslDistro: 'Ubuntu' } } ) + expect(createStackedHostedReviewMock).toHaveBeenCalledWith( + TEST_REPO_PATH, + expect.objectContaining({ + provider: 'github', + base: 'stack/parent', + head: 'feature/wsl' + }), + null, + { localGitExecOptions: { wslDistro: 'Ubuntu' } } + ) }) it('treats SSH worktree drift as unknown without local git probes', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 691486d6dce..0b234b24160 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -676,6 +676,8 @@ import { inspectSetupScriptImportCandidates } from '../../shared/setup-script-im import type { CreateHostedReviewInput, CreateHostedReviewResult, + CreateStackedHostedReviewInput, + CreateStackedHostedReviewResult, HostedReviewCreationEligibility, HostedReviewCreationEligibilityArgs, HostedReviewInfo @@ -685,6 +687,7 @@ import { createHostedReview as createHostedReviewFromRepo, getHostedReviewCreationEligibility as getHostedReviewCreationEligibilityFromRepo } from '../source-control/hosted-review-creation' +import { createStackedHostedReview as createStackedHostedReviewFromRepo } from '../source-control/stacked-hosted-review-creation' import { getLocalProjectGitExecOptions, getLocalProjectWorktreeGitOptions, @@ -19688,6 +19691,36 @@ export class OrcaRuntimeService { return result } + async createStackedHostedReview( + args: CreateStackedHostedReviewInput & { repoSelector: string; worktreeSelector?: string } + ): Promise { + const { repo, repoPath } = await this.resolveHostedReviewTarget(args) + const executionOptions = this.getHostedReviewExecutionOptions(repo) + const result = await createStackedHostedReviewFromRepo( + repoPath, + { + provider: args.provider, + base: args.base, + head: args.head, + title: args.title, + body: args.body, + draft: args.draft, + ...(args.useTemplate !== undefined ? { useTemplate: args.useTemplate } : {}) + }, + repo.connectionId ?? null, + executionOptions ?? {} + ) + if (result.ok && this.stats && !this.stats.hasCountedPR(result.url)) { + this.stats.record({ + type: 'pr_created', + at: Date.now(), + repoId: repo.id, + meta: { prNumber: result.number, prUrl: result.url } + }) + } + return result + } + async listGitLabRepoWorkItems( repoSelector: string, state?: MRListState, diff --git a/src/main/runtime/rpc/methods/hosted-review.test.ts b/src/main/runtime/rpc/methods/hosted-review.test.ts index 3d132e82e41..ba4b05790b7 100644 --- a/src/main/runtime/rpc/methods/hosted-review.test.ts +++ b/src/main/runtime/rpc/methods/hosted-review.test.ts @@ -162,4 +162,39 @@ describe('hosted review RPC methods', () => { result: { ok: true, number: 51 } }) }) + + it('dispatches stacked creation through a distinct runtime method', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + createStackedHostedReview: vi.fn().mockResolvedValue({ + ok: true, + number: 52, + url: 'https://github.com/acme/orca/pull/52', + stackNumber: 60, + parentReview: { number: 51, url: 'https://github.com/acme/orca/pull/51' } + }) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: HOSTED_REVIEW_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest('hostedReview.createStacked', { + repo: 'repo-1', + worktree: 'path:/worktrees/child', + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + ) + + expect(runtime.createStackedHostedReview).toHaveBeenCalledWith({ + repoSelector: 'repo-1', + worktreeSelector: 'path:/worktrees/child', + provider: 'github', + base: 'stack/parent', + head: 'stack/child', + title: 'Child' + }) + expect(response).toMatchObject({ ok: true, result: { ok: true, stackNumber: 60 } }) + }) }) diff --git a/src/main/runtime/rpc/methods/hosted-review.ts b/src/main/runtime/rpc/methods/hosted-review.ts index b658c4c74cb..0d48b02fb19 100644 --- a/src/main/runtime/rpc/methods/hosted-review.ts +++ b/src/main/runtime/rpc/methods/hosted-review.ts @@ -105,5 +105,21 @@ export const HOSTED_REVIEW_METHODS: RpcMethod[] = [ draft: params.draft, useTemplate: params.useTemplate }) + }), + defineMethod({ + name: 'hostedReview.createStacked', + params: HostedReviewCreate, + handler: async (params, { runtime }) => + runtime.createStackedHostedReview({ + repoSelector: params.repo, + worktreeSelector: params.worktree, + provider: params.provider, + base: params.base, + head: params.head, + title: params.title, + body: params.body, + draft: params.draft, + useTemplate: params.useTemplate + }) }) ] diff --git a/src/main/runtime/runtime-rpc.ts b/src/main/runtime/runtime-rpc.ts index 8b5a24f15d2..b783382c544 100644 --- a/src/main/runtime/runtime-rpc.ts +++ b/src/main/runtime/runtime-rpc.ts @@ -316,6 +316,7 @@ const MOBILE_RPC_METHOD_ALLOWLIST = new Set([ 'host.wsl.isAvailable', 'host.wsl.listDistros', 'hostedReview.create', + 'hostedReview.createStacked', 'hostedReview.forBranch', 'hostedReview.getCreationEligibility', 'linear.getCustomView', diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts index f2fa711ee54..2cf262e8cca 100644 --- a/src/main/source-control/hosted-review-creation-eligibility.test.ts +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -472,29 +472,31 @@ describe('getHostedReviewCreationEligibility', () => { blockedReason: null, nextAction: null, defaultBaseRef: 'origin/main', - head: 'feature/create-pr' + head: 'feature/create-pr', + stackedCreationSupported: true }) }) it('detects a GitHub Enterprise Server branch as the GitHub provider (#8312)', async () => { mockGitHubEnterpriseProvider() - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/create-pr', - base: 'origin/main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ + const result = await getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/create-pr', + base: 'origin/main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + + expect(result).toMatchObject({ provider: 'github', canCreate: true, blockedReason: null, nextAction: null }) + expect(result).not.toHaveProperty('stackedCreationSupported') // Enterprise auth was already confirmed during detection; the gate must not // fire a redundant gh probe. diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 58136542917..0fa50ef3a32 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -20,6 +20,8 @@ import { isAzureDevOpsReviewCreationAuthenticated } from '../azure-devops/pull-r import { isGiteaReviewCreationAuthenticated } from '../gitea/pull-request-creation' import { isBitbucketReviewCreationAuthenticated } from '../bitbucket/pull-request-creation' import { getEnterpriseGitHubRepoSlug } from '../github/github-enterprise-repository' +import { getRepoSlug } from '../github/client' +import { isDefaultGitHubHost } from '../../shared/github-repository-identity-key' import { acquire, ghExecFileAsync, gitExecFileAsync, release } from '../github/gh-utils' import { isNoUpstreamError, normalizeGitErrorMessage } from '../../shared/git-remote-error' import type { GitUpstreamStatus } from '../../shared/types' @@ -535,12 +537,19 @@ export async function getHostedReviewCreationEligibility( : lookupFailed ? 'unavailable' : 'not_found' + const githubRepository = + provider === 'github' + ? await getRepoSlug(args.repoPath, args.connectionId, args).catch(() => null) + : null const baseResult = { provider, review: review ? { number: review.number, url: review.url } : null, reviewLookupOutcome, defaultBaseRef, - head: branch || null + head: branch || null, + ...(githubRepository && isDefaultGitHubHost(githubRepository.host) + ? { stackedCreationSupported: true } + : {}) } if (!branch || branch === 'HEAD') { diff --git a/src/main/source-control/stacked-hosted-review-creation.test.ts b/src/main/source-control/stacked-hosted-review-creation.test.ts new file mode 100644 index 00000000000..7814e66e6c8 --- /dev/null +++ b/src/main/source-control/stacked-hosted-review-creation.test.ts @@ -0,0 +1,92 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { prepareMock, registerMock, createMock } = vi.hoisted(() => ({ + prepareMock: vi.fn(), + registerMock: vi.fn(), + createMock: vi.fn() +})) + +vi.mock('../github/stacked-pr-creation', () => ({ + prepareGitHubStackedPullRequest: prepareMock, + registerGitHubStackedPullRequest: registerMock +})) + +vi.mock('./hosted-review-creation', () => ({ createHostedReview: createMock })) + +import { createStackedHostedReview } from './stacked-hosted-review-creation' + +const input = { + provider: 'github' as const, + base: 'stack/parent', + head: 'stack/child', + title: 'Child' +} +const repository = { owner: 'acme', repo: 'orca', host: 'github.com' } +const parentReview = { number: 41, url: 'https://github.com/acme/orca/pull/41' } +const currentReview = { number: 42, url: 'https://github.com/acme/orca/pull/42' } + +beforeEach(() => { + prepareMock.mockReset() + registerMock.mockReset() + createMock.mockReset() +}) + +describe('createStackedHostedReview', () => { + it('creates the current PR before registering the stack', async () => { + prepareMock.mockResolvedValue({ + ok: true, + repository, + parentReview, + currentReview: null + }) + createMock.mockResolvedValue({ ok: true, ...currentReview }) + registerMock.mockResolvedValue({ + ok: true, + ...currentReview, + parentReview, + stackNumber: 50 + }) + + const result = await createStackedHostedReview('/repo', input, 'ssh-1') + + expect(result).toMatchObject({ ok: true, stackNumber: 50 }) + expect(createMock).toHaveBeenCalledWith('/repo', input, 'ssh-1', {}) + expect(registerMock).toHaveBeenCalledWith( + expect.objectContaining({ parentReview, currentReview, connectionId: 'ssh-1' }) + ) + }) + + it('retries registration without creating a duplicate PR', async () => { + prepareMock.mockResolvedValue({ + ok: true, + repository, + parentReview, + currentReview + }) + registerMock.mockResolvedValue({ + ok: true, + ...currentReview, + parentReview, + stackNumber: 50 + }) + + await createStackedHostedReview('/repo', input) + + expect(createMock).not.toHaveBeenCalled() + expect(registerMock).toHaveBeenCalledOnce() + }) + + it('does not create a PR when the parent topology is invalid', async () => { + prepareMock.mockResolvedValue({ + ok: false, + code: 'validation', + error: 'Choose the top pull request.' + }) + + const result = await createStackedHostedReview('/repo', input) + + expect(result).toMatchObject({ ok: false, code: 'validation' }) + expect(createMock).not.toHaveBeenCalled() + expect(registerMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/source-control/stacked-hosted-review-creation.ts b/src/main/source-control/stacked-hosted-review-creation.ts new file mode 100644 index 00000000000..0a36588f493 --- /dev/null +++ b/src/main/source-control/stacked-hosted-review-creation.ts @@ -0,0 +1,61 @@ +import type { + CreateStackedHostedReviewInput, + CreateStackedHostedReviewResult, + HostedReviewSummary +} from '../../shared/hosted-review' +import { + prepareGitHubStackedPullRequest, + registerGitHubStackedPullRequest +} from '../github/stacked-pr-creation' +import { createHostedReview } from './hosted-review-creation' +import type { HostedReviewExecutionOptions } from './hosted-review-git-options' + +export async function createStackedHostedReview( + repoPath: string, + input: CreateStackedHostedReviewInput, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + const plan = await prepareGitHubStackedPullRequest(repoPath, input, connectionId, options) + if (!plan.ok) { + return plan + } + + let currentReview: (HostedReviewSummary & { number: number }) | null = plan.currentReview + if (!currentReview) { + const created = await createHostedReview(repoPath, input, connectionId, options) + if (!created.ok) { + if (!created.existingReview?.number) { + return created + } + return { + ok: false, + code: 'validation', + error: + 'An open pull request already exists for this branch but does not target the selected parent branch.', + createdReview: { + number: created.existingReview.number, + url: created.existingReview.url + } + } + } else { + currentReview = { number: created.number, url: created.url } + } + } + if (!currentReview) { + return { + ok: false, + code: 'unknown_completion', + error: 'Pull request creation may have completed. Retry to finish stack registration.' + } + } + + return registerGitHubStackedPullRequest({ + repoPath, + repository: plan.repository, + parentReview: plan.parentReview, + currentReview, + connectionId, + options + }) +} diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 17c64db922c..cf59c18719c 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -2,6 +2,8 @@ import type { CreateHostedReviewArgs, CreateHostedReviewResult, + CreateStackedHostedReviewArgs, + CreateStackedHostedReviewResult, HostedReviewCreationEligibility, HostedReviewCreationEligibilityArgs, HostedReviewForBranchArgs, @@ -1986,6 +1988,7 @@ export type PreloadApi = { args: HostedReviewCreationEligibilityArgs ) => Promise create: (args: CreateHostedReviewArgs) => Promise + createStacked: (args: CreateStackedHostedReviewArgs) => Promise } // ── GitLab — parallel to gh, MR/issue surface only in v1 ──────── // Shapes mirror gh.* except where GitLab's API differs (MR states, host-qualified project path, `glab api -i` paging). diff --git a/src/preload/index.ts b/src/preload/index.ts index 1254203b969..70ce60249b1 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1715,7 +1715,9 @@ const api = { ipcRenderer.invoke('hostedReview:forBranch', args), getCreationEligibility: (args: unknown): Promise => ipcRenderer.invoke('hostedReview:getCreationEligibility', args), - create: (args: unknown): Promise => ipcRenderer.invoke('hostedReview:create', args) + create: (args: unknown): Promise => ipcRenderer.invoke('hostedReview:create', args), + createStacked: (args: unknown): Promise => + ipcRenderer.invoke('hostedReview:createStacked', args) }, // Why: GitLab bindings live in `./gitlab` so `gl.*` changes don't conflict on every upstream sync of this central file. diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index de63d4ffd0e..c109c46b1a3 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -189,6 +189,7 @@ import { resolveSourceControlLaunchPlatform } from '@/lib/source-control-launch- import { getLocalProjectExecutionRuntimeContext } from '@/lib/local-preflight-context' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { CreateHostedReviewComposer } from './CreateHostedReviewComposer' +import { useHostedReviewStackParent } from './useHostedReviewStackParent' import { resolveCreatedHostedReviewLink } from './source-control-created-review-link' import { formatCreateError } from './create-pull-request-review-copy' import { stripBaseRef, useCreatePullRequestDialogFields } from './useCreatePullRequestDialogFields' @@ -476,6 +477,7 @@ export default function ChecksPanel(): React.JSX.Element { (s) => s.getHostedReviewCreationEligibility ) const createHostedReview = useAppStore((s) => s.createHostedReview) + const createStackedHostedReview = useAppStore((s) => s.createStackedHostedReview) const enqueueGitHubPRRefresh = useAppStore((s) => s.enqueueGitHubPRRefresh) const conflictOperation = useAppStore((s) => activeWorktreeId ? (s.gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') : 'unknown' @@ -1320,10 +1322,13 @@ export default function ChecksPanel(): React.JSX.Element { setBody: setPrBody, draft: prDraft, setDraft: setPrDraft, + stackedCreationSupported: prStackedCreationSupported, + repoDefaultBaseRef: prRepoDefaultBaseRef, baseQuery: prBaseQuery, setBaseQuery: setPrBaseQuery, baseResults: prBaseResults, setBaseResults: setPrBaseResults, + baseSearchPending: prBaseSearchPending, baseSearchError: prBaseSearchError, generating: prGenerating, generateError: prGenerateError, @@ -1360,6 +1365,17 @@ export default function ChecksPanel(): React.JSX.Element { onCancelGenerate: handleCancelGeneratePullRequestFieldsForActive } }) + const stackParentReview = useHostedReviewStackParent({ + enabled: hostedReviewCreateProvider === 'github' && prStackedCreationSupported, + repoPath: repo?.path ?? '', + repoId: repo?.id ?? null, + base: prBase, + // Why: the repo default, not eligibility's defaultBaseRef — that one resolves to + // the worktree's own base, which is exactly the branch a stacked PR targets. + repoDefaultBase: prRepoDefaultBaseRef, + head: branch, + fetchHostedReviewForBranch + }) useEffect(() => { // Why: PR generation can finish while this composer is hidden by a worktree switch; hydrate once the original composer is visible again. if ( @@ -3970,121 +3986,86 @@ export default function ChecksPanel(): React.JSX.Element { ] ) - const handleCreatePullRequest = useCallback(async (): Promise => { - if (!repo || !branch || !createComposerOpen || prGenerating || createPrInFlightRef.current) { - return - } + const handleCreatePullRequest = useCallback( + async (stacked = false): Promise => { + if (!repo || !branch || !createComposerOpen || prGenerating || createPrInFlightRef.current) { + return + } - const requestContextKey = panelContextKey - const isCurrentCreateRequest = (): boolean => - panelContextKeyRef.current === requestContextKey && - createPrInFlightRef.current === requestContextKey - const base = stripBaseRef(prBase).trim() - const title = prTitle.trim() - const worktreePath = activeWorktreePath ?? repo.path - if (!title) { - setCreatePrError( - translate( - 'auto.components.right.sidebar.SourceControl.f3a8b2c1d0e5', - 'Enter a {{value0}} title.', - { - value0: hostedReviewCreateCopy.reviewLabel + const requestContextKey = panelContextKey + const isCurrentCreateRequest = (): boolean => + panelContextKeyRef.current === requestContextKey && + createPrInFlightRef.current === requestContextKey + const base = stripBaseRef(prBase).trim() + const title = prTitle.trim() + const worktreePath = activeWorktreePath ?? repo.path + if (!title) { + setCreatePrError( + translate( + 'auto.components.right.sidebar.SourceControl.f3a8b2c1d0e5', + 'Enter a {{value0}} title.', + { + value0: hostedReviewCreateCopy.reviewLabel + } + ) + ) + return + } + if (!base || stripBaseRef(base).toLowerCase() === stripBaseRef(branch).toLowerCase()) { + setCreatePrError( + translate( + 'auto.components.right.sidebar.SourceControl.ae743199cd', + 'Choose a different base branch before creating a {{value0}}.', + { value0: hostedReviewCreateCopy.reviewLabel } + ) + ) + return + } + + createPrInFlightRef.current = requestContextKey + setIsCreatingPr(true) + setCreatePrError(null) + let pushed = false + try { + const shouldPushBeforeCreate = + createPrPushFirst || hostedReviewCreation?.blockedReason === 'needs_push' + if (shouldPushBeforeCreate) { + const ok = await pushBeforeCreatePullRequest() + if (!isCurrentCreateRequest()) { + return } - ) - ) - return - } - if (!base || stripBaseRef(base).toLowerCase() === stripBaseRef(branch).toLowerCase()) { - setCreatePrError( - translate( - 'auto.components.right.sidebar.SourceControl.ae743199cd', - 'Choose a different base branch before creating a {{value0}}.', - { value0: hostedReviewCreateCopy.reviewLabel } - ) - ) - return - } - - createPrInFlightRef.current = requestContextKey - setIsCreatingPr(true) - setCreatePrError(null) - let pushed = false - try { - const shouldPushBeforeCreate = - createPrPushFirst || hostedReviewCreation?.blockedReason === 'needs_push' - if (shouldPushBeforeCreate) { - const ok = await pushBeforeCreatePullRequest() + if (!ok) { + setCreatePrError('Push failed. Resolve the push error, then try again.') + return + } + pushed = true + } + const createInput = { + repoId: repo.id, + provider: hostedReviewCreateProvider, + base, + head: normalizeHostedReviewHeadRef(branch), + title, + body: prBody, + draft: prDraft && hostedReviewProviderSupportsDraft(hostedReviewCreateProvider), + worktreePath, + useTemplate: prCreationDefaults.useTemplate + } + const result = stacked + ? await createStackedHostedReview(repo.path, createInput) + : await createHostedReview(repo.path, createInput) if (!isCurrentCreateRequest()) { return } - if (!ok) { - setCreatePrError('Push failed. Resolve the push error, then try again.') - return - } - pushed = true - } - const result = await createHostedReview(repo.path, { - repoId: repo.id, - provider: hostedReviewCreateProvider, - base, - head: normalizeHostedReviewHeadRef(branch), - title, - body: prBody, - draft: prDraft && hostedReviewProviderSupportsDraft(hostedReviewCreateProvider), - worktreePath, - useTemplate: prCreationDefaults.useTemplate - }) - if (!isCurrentCreateRequest()) { - return - } - if (result.ok) { - await handlePullRequestCreated({ - provider: hostedReviewCreateProvider, - number: result.number, - url: result.url - }) - if (prCreationDefaults.openAfterCreate) { - openHttpLink(result.url, { worktreeId: activeWorktreeId }) - } - if (activePullRequestGenerationKey) { - updatePullRequestGenerationRecord( - activePullRequestGenerationKey, - clearPullRequestGenerationRequiresPushBeforeCreate - ) - } - return - } - if (result.existingReview?.url) { - const number = result.existingReview.number - toast.success( - number - ? translate( - 'auto.components.right.sidebar.ChecksPanel.b6ce28da5b', - '{{value0}} #{{value1}} is already open', - { value0: hostedReviewCreateCopy.titleLabel, value1: number } - ) - : translate( - 'auto.components.right.sidebar.ChecksPanel.cf9e69f3be', - '{{value0}} is already open', - { value0: hostedReviewCreateCopy.titleLabel } - ), - { - action: { - label: translate( - 'auto.components.right.sidebar.ChecksPanel.192e686e57', - 'Open on {{value0}}', - { value0: hostedReviewCreateCopy.providerName } - ), - onClick: () => window.api.shell.openUrl(result.existingReview!.url) - } - } - ) - if (number) { + if (result.ok) { await handlePullRequestCreated({ provider: hostedReviewCreateProvider, - number, - url: result.existingReview.url + number: result.number, + url: result.url }) + if (prCreationDefaults.openAfterCreate) { + openHttpLink(result.url, { worktreeId: activeWorktreeId }) + } if (activePullRequestGenerationKey) { updatePullRequestGenerationRecord( activePullRequestGenerationKey, @@ -4093,55 +4074,110 @@ export default function ChecksPanel(): React.JSX.Element { } return } + if ('existingReview' in result && result.existingReview?.url) { + const number = result.existingReview.number + toast.success( + number + ? translate( + 'auto.components.right.sidebar.ChecksPanel.b6ce28da5b', + '{{value0}} #{{value1}} is already open', + { value0: hostedReviewCreateCopy.titleLabel, value1: number } + ) + : translate( + 'auto.components.right.sidebar.ChecksPanel.cf9e69f3be', + '{{value0}} is already open', + { value0: hostedReviewCreateCopy.titleLabel } + ), + { + action: { + label: translate( + 'auto.components.right.sidebar.ChecksPanel.192e686e57', + 'Open on {{value0}}', + { value0: hostedReviewCreateCopy.providerName } + ), + onClick: () => window.api.shell.openUrl(result.existingReview!.url) + } + } + ) + if (number) { + await handlePullRequestCreated({ + provider: hostedReviewCreateProvider, + number, + url: result.existingReview.url + }) + if (activePullRequestGenerationKey) { + updatePullRequestGenerationRecord( + activePullRequestGenerationKey, + clearPullRequestGenerationRequiresPushBeforeCreate + ) + } + return + } + } + // Why: stacked creation can create the pull request and still fail to register + // the stack. Link the review that exists before surfacing the stack failure, or + // the workspace stays unaware of a PR the user can already see on GitHub. + if ('createdReview' in result && result.createdReview?.url) { + const { number, url } = result.createdReview + if (number) { + await handlePullRequestCreated({ + provider: hostedReviewCreateProvider, + number, + url + }) + } + } + setCreatePrError(formatCreateError(result, pushed, hostedReviewCreateCopy.shortLabel)) + } catch (error) { + if (!isCurrentCreateRequest()) { + return + } + setCreatePrError( + error instanceof Error + ? error.message + : translate( + 'auto.components.right.sidebar.SourceControl.e2b7a1c0d9f4', + 'Failed to create {{value0}}', + { value0: hostedReviewCreateCopy.reviewLabel } + ) + ) + } finally { + if (createPrInFlightRef.current === requestContextKey) { + createPrInFlightRef.current = null + setIsCreatingPr(false) + setGitStatusRefreshNonce((value) => value + 1) + } } - setCreatePrError(formatCreateError(result, pushed, hostedReviewCreateCopy.shortLabel)) - } catch (error) { - if (!isCurrentCreateRequest()) { - return - } - setCreatePrError( - error instanceof Error - ? error.message - : translate( - 'auto.components.right.sidebar.SourceControl.e2b7a1c0d9f4', - 'Failed to create {{value0}}', - { value0: hostedReviewCreateCopy.reviewLabel } - ) - ) - } finally { - if (createPrInFlightRef.current === requestContextKey) { - createPrInFlightRef.current = null - setIsCreatingPr(false) - setGitStatusRefreshNonce((value) => value + 1) - } - } - }, [ - activeWorktreePath, - activeWorktreeId, - activePullRequestGenerationKey, - branch, - createComposerOpen, - createHostedReview, - createPrPushFirst, - handlePullRequestCreated, - hostedReviewCreateCopy.providerName, - hostedReviewCreateCopy.reviewLabel, - hostedReviewCreateCopy.shortLabel, - hostedReviewCreateCopy.titleLabel, - hostedReviewCreateProvider, - hostedReviewCreation?.blockedReason, - panelContextKey, - prBase, - prBody, - prCreationDefaults.openAfterCreate, - prCreationDefaults.useTemplate, - prDraft, - prGenerating, - prTitle, - pushBeforeCreatePullRequest, - repo, - updatePullRequestGenerationRecord - ]) + }, + [ + activeWorktreePath, + activeWorktreeId, + activePullRequestGenerationKey, + branch, + createComposerOpen, + createHostedReview, + createStackedHostedReview, + createPrPushFirst, + handlePullRequestCreated, + hostedReviewCreateCopy.providerName, + hostedReviewCreateCopy.reviewLabel, + hostedReviewCreateCopy.shortLabel, + hostedReviewCreateCopy.titleLabel, + hostedReviewCreateProvider, + hostedReviewCreation?.blockedReason, + panelContextKey, + prBase, + prBody, + prCreationDefaults.openAfterCreate, + prCreationDefaults.useTemplate, + prDraft, + prGenerating, + prTitle, + pushBeforeCreatePullRequest, + repo, + updatePullRequestGenerationRecord + ] + ) // ── Empty state ── if (!activeWorktree) { @@ -4283,10 +4319,12 @@ export default function ChecksPanel(): React.JSX.Element { {!operationInProgress && createComposerOpen ? (
void handleGeneratePullRequestFields()} onCancelGenerate={handleCancelGeneratePullRequestFields} - onPrimaryAction={() => void handleCreatePullRequest()} + onPrimaryAction={(stacked) => void handleCreatePullRequest(stacked)} />
) : null} diff --git a/src/renderer/src/components/right-sidebar/CreateHostedReviewBasePicker.tsx b/src/renderer/src/components/right-sidebar/CreateHostedReviewBasePicker.tsx new file mode 100644 index 00000000000..3f1cc1d37a2 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CreateHostedReviewBasePicker.tsx @@ -0,0 +1,292 @@ +import { Check, ChevronDown } from 'lucide-react' +import { useId, useRef, useState } from 'react' +import { Input } from '@/components/ui/input' +import { Label } from '@/components/ui/label' +import { cn } from '@/lib/utils' +import { translate } from '@/i18n/i18n' +import type { LocalizedHostedReviewCopy } from '@/i18n/hosted-review-localized-copy' +import { COMPOSER_FIELD_CLASS } from './create-hosted-review-composer-field-class' +import { CreateHostedReviewComposerMessage } from './CreateHostedReviewComposerMessage' +import { stripBaseRef } from './useCreatePullRequestDialogFields' + +type CreateHostedReviewBasePickerProps = { + copy: LocalizedHostedReviewCopy + base: string + setBase: (value: string) => void + /** The repo's default branch, where an emptied field lands. Null until resolved. */ + repoDefaultBase: string | null + editing: boolean + setEditing: (value: boolean) => void + baseQuery: string + setBaseQuery: (value: string) => void + baseResults: string[] + setBaseResults: (value: string[]) => void + baseSearchPending: boolean + baseSearchError: string | null + fieldsLocked: boolean + strippedBranch: string + baseSameAsBranch: boolean +} + +/** + * The composer's merge target: a labelled full-width combobox whose results stay + * attached to it, plus the base-scoped errors. + */ +export function CreateHostedReviewBasePicker({ + copy, + base, + setBase, + repoDefaultBase, + editing, + setEditing, + baseQuery, + setBaseQuery, + baseResults, + setBaseResults, + baseSearchPending, + baseSearchError, + fieldsLocked, + strippedBranch, + baseSameAsBranch +}: CreateHostedReviewBasePickerProps): React.JSX.Element { + const [activeResult, setActiveResult] = useState(-1) + const inputRef = useRef(null) + // Why: marks a blur that an explicit Enter/Escape already resolved. + const settledRef = useRef(false) + const fieldId = useId() + const resultsId = useId() + + const trimmedQuery = baseQuery.trim() + const trimmedRepoDefault = repoDefaultBase?.trim() ?? '' + const showResults = editing && baseResults.length > 0 + // Why: emptying the field is how you say "not this branch"; name where it lands so + // the reset isn't an invisible behaviour. + const showRepoDefaultHint = editing && trimmedQuery.length === 0 && trimmedRepoDefault.length > 0 + // Why: only claim "no branches match" once a search has actually settled, so the + // debounce window can't report an absence the app hasn't observed yet. + const showNoResults = + editing && + baseResults.length === 0 && + trimmedQuery.length >= 2 && + !baseSearchPending && + !baseSearchError + + const closeSearch = (): void => { + setEditing(false) + setBaseQuery('') + setBaseResults([]) + setActiveResult(-1) + } + + const commitSearch = (value: string): void => { + // Why: an emptied field commits the repo default rather than silently restoring + // the branch the user just cleared. With no default resolved yet there is nothing + // honest to fall back to, so the committed base stands. + const nextBase = value.trim() || trimmedRepoDefault + if (nextBase) { + setBase(nextBase) + } + settledRef.current = true + closeSearch() + inputRef.current?.blur() + } + + const cancelSearch = (): void => { + settledRef.current = true + closeSearch() + inputRef.current?.blur() + } + + const handleBlur = (): void => { + // Why: Enter and Escape blur the input themselves; without this they would + // re-enter the commit path below with the pre-close query still in scope. + if (settledRef.current) { + settledRef.current = false + closeSearch() + return + } + // Why: clicking away from an emptied field means what pressing Enter on it + // means — land on the repo default instead of restoring what was cleared. + // A partial query still cancels; only an empty one is an instruction. + if (trimmedQuery.length === 0 && trimmedRepoDefault) { + setBase(trimmedRepoDefault) + } + closeSearch() + } + + const moveActiveResult = (delta: number): void => { + if (baseResults.length === 0) { + return + } + setActiveResult((current) => { + const next = current + delta + if (next < 0) { + return baseResults.length - 1 + } + return next >= baseResults.length ? 0 : next + }) + } + + const handleKeyDown = (event: React.KeyboardEvent): void => { + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + event.preventDefault() + moveActiveResult(event.key === 'ArrowDown' ? 1 : -1) + return + } + if (event.key === 'Enter') { + event.preventDefault() + commitSearch(baseResults[activeResult] ?? baseQuery) + return + } + if (event.key === 'Escape') { + event.preventDefault() + cancelSearch() + } + } + + return ( + // Why: the base owns the full sidebar width — branch names are long — and the + // head branch rides the label row instead of costing another line. +
+
+ {/* Why: the label holds its line and the head branch absorbs the squeeze — + a wrapping two-word label next to a one-line ref reads as broken. */} + + + {translate( + 'auto.components.right.sidebar.CreateHostedReviewBasePicker.bb4b41d563', + 'from {{value0}}', + { value0: strippedBranch } + )} + +
+ +
+ = 0 ? `${resultsId}-${activeResult}` : undefined + } + aria-invalid={baseSameAsBranch || undefined} + // Why: an input can't ellipsize, and base refs routinely overflow the sidebar. + title={editing ? undefined : base} + value={editing ? baseQuery : base} + disabled={fieldsLocked} + onFocus={(event) => { + // Why: a programmatic blur that never fired would otherwise leave the + // flag set and swallow the next real one. + settledRef.current = false + setEditing(true) + setBaseQuery(event.currentTarget.value) + }} + onBlur={handleBlur} + onChange={(event) => { + setBaseQuery(event.target.value) + setActiveResult(-1) + }} + onKeyDown={handleKeyDown} + // Why: the placeholder is where an emptied field lands, so it has to be the + // repo's real default — a hardcoded "main" lies on a trunk-named repo. + placeholder={ + trimmedRepoDefault || + translate('auto.components.right.sidebar.SourceControl.e64a632456', 'main') + } + className={cn(COMPOSER_FIELD_CLASS, 'pl-2 pr-7')} + /> +
+ + {showResults ? ( +
+ {baseResults.map((ref, index) => { + const selected = stripBaseRef(base) === ref + return ( + + ) + })} +
+ ) : null} + + {showRepoDefaultHint ? ( +

+ {translate( + 'auto.components.right.sidebar.CreateHostedReviewBasePicker.da4d57c9c2', + 'Leave empty to use {{value0}}.', + { value0: trimmedRepoDefault } + )} +

+ ) : null} + + {showNoResults ? ( +

+ {translate( + 'auto.components.right.sidebar.CreateHostedReviewBasePicker.5a9315b61a', + 'No branches match “{{value0}}”. Press Enter to use it anyway.', + { value0: trimmedQuery } + )} +

+ ) : null} + + {/* Why: base problems belong to the base field, not to the block of + operation errors above the submit button. */} + {baseSameAsBranch ? ( + + {translate( + 'auto.components.right.sidebar.SourceControl.ae743199cd', + 'Choose a different base branch before creating a {{value0}}.', + { value0: copy.reviewLabel } + )} + + ) : null} + {baseSearchError ? ( + {baseSearchError} + ) : null} +
+ ) +} diff --git a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx index 63c66b01ddc..7876e8dca25 100644 --- a/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx +++ b/src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx @@ -1,3 +1,4 @@ +import { useState } from 'react' import { ChevronDown, GitMerge, @@ -26,8 +27,10 @@ import { type HostedReviewProvider } from '../../../../shared/hosted-review' import { stripBaseRef } from './useCreatePullRequestDialogFields' +import type { HostedReviewStackParent } from './useHostedReviewStackParent' import type { DropdownActionKind, DropdownEntry } from './source-control-dropdown-items' import { CreateHostedReviewComposerFields } from './CreateHostedReviewComposerFields' +import { getCreateButtonLabel } from './create-hosted-review-button-label' import { RIGHT_SIDEBAR_MORPHING_PRIMARY_BUTTON_CLASS, RIGHT_SIDEBAR_PRIMARY_BUTTON_LABEL_CLASS, @@ -47,16 +50,20 @@ export type CreateHostedReviewComposerProps = { branch: string base: string setBase: (value: string) => void + repoDefaultBase: string | null title: string setTitle: (value: string) => void body: string setBody: (value: string) => void draft: boolean setDraft: (value: boolean) => void + stackedCreationSupported: boolean + stackParentReview: HostedReviewStackParent | null baseQuery: string setBaseQuery: (value: string) => void baseResults: string[] setBaseResults: (value: string[]) => void + baseSearchPending: boolean baseSearchError: string | null aiGenerationEnabled: boolean generating: boolean @@ -70,7 +77,7 @@ export type CreateHostedReviewComposerProps = { dropdownItems?: DropdownEntry[] onGenerate: () => void onCancelGenerate: () => void - onPrimaryAction: () => void + onPrimaryAction: (stacked: boolean) => void onDropdownAction?: (kind: DropdownActionKind) => void } @@ -80,16 +87,20 @@ export function CreateHostedReviewComposer({ branch, base, setBase, + repoDefaultBase, title, setTitle, body, setBody, draft, setDraft, + stackedCreationSupported, + stackParentReview, baseQuery, setBaseQuery, baseResults, setBaseResults, + baseSearchPending, baseSearchError, aiGenerationEnabled, generating, @@ -111,7 +122,20 @@ export function CreateHostedReviewComposer({ const supportsDraft = hostedReviewProviderSupportsDraft(provider) const effectiveDraft = supportsDraft && draft const ReviewIcon = provider === 'gitlab' ? GitMerge : GitPullRequestArrow + const stackedModeAvailable = provider === 'github' && stackedCreationSupported const normalizedBase = stripBaseRef(base) + const stackSelectionKey = stackParentReview + ? `${normalizedBase}:${stackParentReview.number}` + : null + const [stackSelection, setStackSelection] = useState({ key: '', enabled: false }) + const effectiveStacked = + stackedModeAvailable && + stackSelectionKey !== null && + stackSelection.key === stackSelectionKey && + stackSelection.enabled + const setStacked = (enabled: boolean): void => { + setStackSelection({ key: stackSelectionKey ?? '', enabled }) + } const strippedBranch = stripBaseRef(branch) const baseSameAsBranch = normalizedBase.toLowerCase() === strippedBranch.toLowerCase() const createDisabled = @@ -197,7 +221,9 @@ export function CreateHostedReviewComposer({ return (
-
+ {/* Why: one gap between groups, tighter gaps inside them — the form reads as + content → merge target → options → action instead of a stack of boxes. */} +