From 0fe04e2c916e8a1e699b0c1affba2303adea0010 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 01:48:14 -0700 Subject: [PATCH 001/216] perf(sqlite): cache prepared statements in SyncDatabase (#13769) prepare() recompiled every statement, so orchestration reads re-parsed the same SQL on the main thread. Adds a bounded LRU keyed by SQL, cleared on close() and before schema-changing exec(). Wildcard selects are excluded: node:sqlite builds the first post-schema-change row from stale column names, so a reused SELECT * can silently drop a freshly added column. PRAGMAs stay uncached. Co-authored-by: Orca --- src/main/sqlite/sync-database.test.ts | 153 ++++++++++++++++++++++++++ src/main/sqlite/sync-database.ts | 36 +++++- 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 src/main/sqlite/sync-database.test.ts diff --git a/src/main/sqlite/sync-database.test.ts b/src/main/sqlite/sync-database.test.ts new file mode 100644 index 00000000000..34677095b26 --- /dev/null +++ b/src/main/sqlite/sync-database.test.ts @@ -0,0 +1,153 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import SyncDatabase from './sync-database' + +const temporaryDirectories: string[] = [] +const openDatabases: SyncDatabase.Database[] = [] + +async function createDatabase(): Promise { + const directory = await mkdtemp(join(tmpdir(), 'orca-sync-database-')) + temporaryDirectories.push(directory) + const db = new SyncDatabase(join(directory, 'test.db')) + openDatabases.push(db) + db.exec( + 'CREATE TABLE items (id TEXT PRIMARY KEY, label TEXT); ' + + "INSERT INTO items (id, label) VALUES ('a', 'alpha'), ('b', 'beta')" + ) + return db +} + +afterEach(async () => { + for (const db of openDatabases.splice(0)) { + try { + db.close() + } catch { + // already closed by the test + } + } + await Promise.all( + temporaryDirectories.splice(0).map((dir) => rm(dir, { recursive: true, force: true })) + ) +}) + +describe('SyncDatabase statement cache', () => { + it('reuses the same statement object for identical SQL', async () => { + const db = await createDatabase() + const sql = 'SELECT label FROM items WHERE id = ?' + + expect(db.prepare(sql)).toBe(db.prepare(sql)) + expect(db.prepare('SELECT id FROM items WHERE id = ?')).not.toBe(db.prepare(sql)) + }) + + it('returns correct rows when a reused statement is bound to different values', async () => { + const db = await createDatabase() + const sql = 'SELECT label FROM items WHERE id = ?' + + expect(db.prepare(sql).get('a')).toEqual({ label: 'alpha' }) + expect(db.prepare(sql).get('b')).toEqual({ label: 'beta' }) + expect(db.prepare(sql).get('missing')).toBeUndefined() + expect(db.prepare(sql).all('a')).toEqual([{ label: 'alpha' }]) + }) + + it('bounds the cache so per-arity SQL cannot grow it without limit', async () => { + const db = await createDatabase() + const first = 'SELECT 0 AS n' + const firstStatement = db.prepare(first) + for (let index = 1; index <= 256; index += 1) { + db.prepare(`SELECT ${index} AS n`) + } + + expect(db.prepare(first)).not.toBe(firstStatement) + expect(db.prepare('SELECT 256 AS n')).toBe(db.prepare('SELECT 256 AS n')) + }) + + it('drops cached statements on close', async () => { + const db = await createDatabase() + db.prepare('SELECT label FROM items WHERE id = ?') + db.close() + + const cache = (db as unknown as { statementCache: Map }).statementCache + expect(cache.size).toBe(0) + }) + + it('does not serve a stale statement after DDL adds a column', async () => { + const db = await createDatabase() + expect(db.prepare('SELECT * FROM items WHERE id = ?').all('a')).toEqual([ + { id: 'a', label: 'alpha' } + ]) + + db.exec("ALTER TABLE items ADD COLUMN note TEXT; UPDATE items SET note = 'noted'") + + expect(db.prepare('SELECT * FROM items WHERE id = ?').all('a')).toEqual([ + { id: 'a', label: 'alpha', note: 'noted' } + ]) + expect(db.prepare('SELECT label FROM items WHERE id = ?').all('a')).toEqual([ + { label: 'alpha' } + ]) + }) + + it('keeps a cached statement correct when another connection changes the schema', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-sync-database-')) + temporaryDirectories.push(directory) + const path = join(directory, 'shared.db') + const writer = new SyncDatabase(path) + const reader = new SyncDatabase(path) + openDatabases.push(writer, reader) + writer.exec("CREATE TABLE items (id TEXT PRIMARY KEY); INSERT INTO items VALUES ('a')") + const sql = 'SELECT id FROM items ORDER BY id' + expect(reader.prepare(sql).all()).toEqual([{ id: 'a' }]) + + writer.exec("ALTER TABLE items ADD COLUMN note TEXT; INSERT INTO items VALUES ('b', 'noted')") + + expect(reader.prepare(sql).all()).toEqual([{ id: 'a' }, { id: 'b' }]) + expect(reader.prepare('SELECT note FROM items WHERE id = ?').all('b')).toEqual([ + { note: 'noted' } + ]) + }) + + it('does not cache wildcard selects or pragma statements', async () => { + const db = await createDatabase() + + expect(db.prepare('SELECT * FROM items')).not.toBe(db.prepare('SELECT * FROM items')) + expect(db.prepare('PRAGMA table_info(items)')).not.toBe(db.prepare('PRAGMA table_info(items)')) + expect(db.prepare('SELECT COUNT(*) AS n FROM items')).toBe( + db.prepare('SELECT COUNT(*) AS n FROM items') + ) + }) + + it('keeps cached statements across transaction control and other non-DDL exec calls', async () => { + const db = await createDatabase() + const sql = 'SELECT label FROM items WHERE id = ?' + const statement = db.prepare(sql) + + db.exec('BEGIN IMMEDIATE') + db.exec("INSERT INTO items (id, label) VALUES ('c', 'gamma')") + db.exec('COMMIT') + + expect(db.prepare(sql)).toBe(statement) + expect(statement.get('c')).toEqual({ label: 'gamma' }) + }) + + it('preserves pragma and exec behavior', async () => { + const db = await createDatabase() + + expect(db.pragma('journal_mode', { simple: true })).toBe('delete') + expect(db.pragma('table_info(items)')).toEqual([ + expect.objectContaining({ name: 'id' }), + expect.objectContaining({ name: 'label' }) + ]) + expect(db.pragma('table_info(missing_table)')).toEqual([]) + expect(db.pragma('table_info(missing_table)', { simple: true })).toBeUndefined() + }) + + it('rejects a missing file when fileMustExist is set', async () => { + const directory = await mkdtemp(join(tmpdir(), 'orca-sync-database-')) + temporaryDirectories.push(directory) + + expect(() => new SyncDatabase(join(directory, 'absent.db'), { fileMustExist: true })).toThrow( + /does not exist/ + ) + }) +}) diff --git a/src/main/sqlite/sync-database.ts b/src/main/sqlite/sync-database.ts index 70d8fa1c06a..68faef8b83c 100644 --- a/src/main/sqlite/sync-database.ts +++ b/src/main/sqlite/sync-database.ts @@ -15,6 +15,18 @@ type PragmaOptions = { export type SqliteStatement = StatementSync +// Why: dynamic `IN (?,?,…)` clauses mint a new SQL string per arity, so the cache must stay bounded. +const STATEMENT_CACHE_LIMIT = 256 +const AGGREGATE_STAR = /\(\s*\*\s*\)/g +const PRAGMA_STATEMENT = /^\s*PRAGMA\b/i +const SCHEMA_CHANGING_SQL = /\b(?:ALTER|CREATE|DROP|REINDEX|VACUUM|ATTACH|DETACH)\b/i + +// Why: node:sqlite builds the first post-schema-change row from stale column names, so a reused +// wildcard SELECT can drop a freshly added column; PRAGMAs are one-shot config, never hot-path. +function isStatementCacheable(sql: string): boolean { + return !PRAGMA_STATEMENT.test(sql) && !sql.replace(AGGREGATE_STAR, '').includes('*') +} + // Why: SSH companions target Node 18 and import this adapter without opening SQLite. function loadDatabaseSync(): typeof DatabaseSync { if (typeof process.getBuiltinModule !== 'function') { @@ -26,6 +38,7 @@ function loadDatabaseSync(): typeof DatabaseSync { class SyncDatabase { private readonly db: DatabaseSync + private readonly statementCache = new Map() constructor(path: SqlitePath, options: SyncDatabaseOptions = {}) { if ( @@ -44,11 +57,31 @@ class SyncDatabase { } exec(sql: string): void { + // Why: drop cached statements before DDL lands so a partially applied batch cannot leave stale ones. + if (SCHEMA_CHANGING_SQL.test(sql)) { + this.statementCache.clear() + } this.db.exec(sql) } prepare(sql: string): StatementSync { - return this.db.prepare(sql) + const cached = this.statementCache.get(sql) + if (cached) { + this.statementCache.delete(sql) + this.statementCache.set(sql, cached) + return cached + } + const statement = this.db.prepare(sql) + if (isStatementCacheable(sql)) { + if (this.statementCache.size >= STATEMENT_CACHE_LIMIT) { + const oldest = this.statementCache.keys().next().value + if (oldest !== undefined) { + this.statementCache.delete(oldest) + } + } + this.statementCache.set(sql, statement) + } + return statement } pragma(sql: string, options?: PragmaOptions): unknown { @@ -64,6 +97,7 @@ class SyncDatabase { } close(): void { + this.statementCache.clear() this.db.close() } } From e8904cf7d22b7ddcd33e0ecb88d08cd937443420 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 01:48:17 -0700 Subject: [PATCH 002/216] perf(renderer): skip runtime status map churn on unchanged re-probes (#13771) setRuntimeEnvironmentStatus always cloned runtimeStatusByEnvironmentId, so a re-probe returning an identical status still invalidated every subscriber. It now returns the state unchanged when the computed entry matches the stored one. checkedAt is excluded from the comparison: it is Date.now() per probe and no consumer reads it, so including it would make the guard dead code. Generation advance and the disconnect toasts still run on every call. Co-authored-by: Orca --- .../runtime-environment-status-equality.ts | 12 ++ .../src/store/slices/runtime-status.test.ts | 104 ++++++++++++++++++ .../src/store/slices/runtime-status.ts | 25 +++-- 3 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/store/slices/runtime-environment-status-equality.ts diff --git a/src/renderer/src/store/slices/runtime-environment-status-equality.ts b/src/renderer/src/store/slices/runtime-environment-status-equality.ts new file mode 100644 index 00000000000..04570e461e3 --- /dev/null +++ b/src/renderer/src/store/slices/runtime-environment-status-equality.ts @@ -0,0 +1,12 @@ +import type { RuntimeEnvironmentStatus } from './runtime-status' +import { persistedUIValuesEqual } from '../../../../shared/persisted-ui-equality' + +// Why: `checkedAt` is dropped — no consumer reads it, and a per-probe timestamp +// would make every entry unequal and defeat the caller's identity guard. +// Everything else, including fields added later, is compared structurally. +export function runtimeEnvironmentStatusesEqual( + { checkedAt: _leftCheckedAt, ...left }: RuntimeEnvironmentStatus, + { checkedAt: _rightCheckedAt, ...right }: RuntimeEnvironmentStatus +): boolean { + return persistedUIValuesEqual(left, right) +} diff --git a/src/renderer/src/store/slices/runtime-status.test.ts b/src/renderer/src/store/slices/runtime-status.test.ts index 49610ff07ce..afdadb266a9 100644 --- a/src/renderer/src/store/slices/runtime-status.test.ts +++ b/src/renderer/src/store/slices/runtime-status.test.ts @@ -201,6 +201,110 @@ describe('runtime-status slice', () => { ).toBe('client-device') }) + it('still learns a paired device id when the re-probed status is unchanged', () => { + const store = createSliceStore() + // Why: the status arrives before the catalog, so only the second (identical) probe + // can carry the id onto the environment — the map guard must not swallow it. + const status = makeStatus({ pairedDeviceId: 'client-device' }) + store.getState().setRuntimeEnvironmentStatus('env-a', { status, checkedAt: 1 }) + store.getState().setRuntimeEnvironments([makeEnvironment()]) + const before = store.getState().runtimeStatusByEnvironmentId + + store.getState().setRuntimeEnvironmentStatus('env-a', { status, checkedAt: 2 }) + + expect(store.getState().runtimeEnvironments[0].pairedDeviceId).toBe('client-device') + expect(store.getState().runtimeStatusByEnvironmentId).toBe(before) + }) + + it('keeps the map reference when a re-probe returns an identical status', () => { + const store = createSliceStore() + store.getState().setRuntimeEnvironmentStatus('env-a', { status: makeStatus(), checkedAt: 1 }) + const before = store.getState().runtimeStatusByEnvironmentId + const entryBefore = before.get('env-a') + const generationBefore = getRuntimeEnvironmentConnectionGeneration('env-a') + + store.getState().setRuntimeEnvironmentStatus('env-a', { status: makeStatus(), checkedAt: 2 }) + + expect(store.getState().runtimeStatusByEnvironmentId).toBe(before) + expect(store.getState().runtimeStatusByEnvironmentId.get('env-a')).toBe(entryBefore) + expect(getRuntimeEnvironmentConnectionGeneration('env-a')).toBe(generationBefore) + }) + + it('keeps the map reference when an unreachable server is re-probed', () => { + const store = createSliceStore() + store.setState({ runtimeEnvironments: [makeEnvironment()] }) + store.getState().setRuntimeEnvironmentStatus('env-a', { status: null, checkedAt: 1 }) + const before = store.getState().runtimeStatusByEnvironmentId + + store.getState().setRuntimeEnvironmentStatus('env-a', { status: null, checkedAt: 2 }) + + expect(store.getState().runtimeStatusByEnvironmentId).toBe(before) + expect(toast.warning).not.toHaveBeenCalled() + }) + + it('replaces the map when any status field changes, nested ones included', () => { + const store = createSliceStore() + store.getState().setRuntimeEnvironmentStatus('env-a', { + status: makeStatus({ liveTabCount: 1, capabilities: ['a'] }), + checkedAt: 1 + }) + const afterFirst = store.getState().runtimeStatusByEnvironmentId + + store.getState().setRuntimeEnvironmentStatus('env-a', { + status: makeStatus({ liveTabCount: 2, capabilities: ['a'] }), + checkedAt: 2 + }) + const afterScalarChange = store.getState().runtimeStatusByEnvironmentId + expect(afterScalarChange).not.toBe(afterFirst) + expect(afterScalarChange.get('env-a')?.checkedAt).toBe(2) + + store.getState().setRuntimeEnvironmentStatus('env-a', { + status: makeStatus({ liveTabCount: 2, capabilities: ['a', 'b'] }), + checkedAt: 3 + }) + const afterNestedChange = store.getState().runtimeStatusByEnvironmentId + expect(afterNestedChange).not.toBe(afterScalarChange) + expect(afterNestedChange.get('env-a')?.status?.capabilities).toEqual(['a', 'b']) + }) + + it('still writes and toasts across null and non-null transitions', () => { + const store = createSliceStore() + store.setState({ runtimeEnvironments: [makeEnvironment()] }) + store.getState().setRuntimeEnvironmentStatus('env-a', { status: makeStatus(), checkedAt: 1 }) + const connected = store.getState().runtimeStatusByEnvironmentId + const generationConnected = getRuntimeEnvironmentConnectionGeneration('env-a') + + store.getState().setRuntimeEnvironmentStatus('env-a', { status: null, checkedAt: 2 }) + const disconnected = store.getState().runtimeStatusByEnvironmentId + expect(disconnected).not.toBe(connected) + expect(disconnected.get('env-a')?.status).toBeNull() + expect(toast.warning).toHaveBeenCalledTimes(1) + + store.getState().setRuntimeEnvironmentStatus('env-a', { status: makeStatus(), checkedAt: 3 }) + const reconnected = store.getState().runtimeStatusByEnvironmentId + expect(reconnected).not.toBe(disconnected) + expect(getRuntimeEnvironmentConnectionGeneration('env-a')).toBe(generationConnected + 1) + expect(reconnected.get('env-a')?.connectionGeneration).toBe(generationConnected + 1) + expect(toast.dismiss).toHaveBeenCalledWith('runtime-environment-disconnected:env-a') + }) + + it('replaces the map when the same status arrives under a new runtime id', () => { + const store = createSliceStore() + store.getState().setRuntimeEnvironmentStatus('env-a', { + status: makeStatus({ runtimeId: 'runtime-a' }), + checkedAt: 1 + }) + const before = store.getState().runtimeStatusByEnvironmentId + + store.getState().setRuntimeEnvironmentStatus('env-a', { + status: makeStatus({ runtimeId: 'runtime-b' }), + checkedAt: 2 + }) + + expect(store.getState().runtimeStatusByEnvironmentId).not.toBe(before) + expect(store.getState().runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration).toBe(2) + }) + it('does not toast when the first probe finds a saved server offline', () => { const store = createSliceStore() store.setState({ runtimeEnvironments: [makeEnvironment()] }) diff --git a/src/renderer/src/store/slices/runtime-status.ts b/src/renderer/src/store/slices/runtime-status.ts index fffa3a0bb3d..a5bd778273a 100644 --- a/src/renderer/src/store/slices/runtime-status.ts +++ b/src/renderer/src/store/slices/runtime-status.ts @@ -2,6 +2,7 @@ import type { StateCreator } from 'zustand' import type { AppState } from '../types' import type { PublicKnownRuntimeEnvironment } from '../../../../shared/runtime-environments' import type { RuntimeStatus } from '../../../../shared/runtime-types' +import { runtimeEnvironmentStatusesEqual } from './runtime-environment-status-equality' import { clearRecentRuntimeCompatibilityFailure, clearRuntimeCompatibilityCache @@ -21,6 +22,8 @@ import { refreshRuntimeEnvironmentStatus } from './runtime-status-refresh' export type RuntimeEnvironmentStatus = { status: RuntimeStatus | null appVersion?: string | null + /** When the stored status was last *observed to change*; an unchanged re-probe + * is dropped rather than rewritten, so this is not a probe-freshness clock. */ checkedAt: number connectionGeneration?: number } @@ -186,7 +189,6 @@ export const createRuntimeStatusSlice: StateCreator { - const next = new Map(s.runtimeStatusByEnvironmentId) const sessionEnded = status.status === null && previous?.status != null const connectionChanged = status.status !== null && @@ -200,10 +202,13 @@ export const createRuntimeStatusSlice: StateCreator environment.id === environmentId) : -1 @@ -214,9 +219,15 @@ export const createRuntimeStatusSlice: StateCreator Date: Tue, 11 Aug 2026 01:50:37 -0700 Subject: [PATCH 003/216] perf(renderer): keep catalog array identity and path-status cache on no-op refetches (#13770) * perf(renderer): keep catalog array identity and path-status cache on no-op refetches mergeByIdentity always allocated a fresh array, so project-group and folder-workspace refetches replaced state by reference even when nothing changed, refiring a forced folderWorkspace.getPathStatus sweep per row on every repos:changed. Unchanged merges now return the previous array, and the paired folderWorkspacePathStatuses resets are gated on the same check so a no-op refetch neither clears the cache nor needs to refill it. Real catalog changes clear and refire exactly as before. Co-authored-by: Orca * refactor(renderer): make the catalog arrays readonly The identity guard can now return the caller's array, so a future in-place push or sort on state.projectGroups / state.folderWorkspaces would alias store state. Typing the merge helpers and the slice fields readonly removes both `as T[]` casts the guard introduced and adds none, matching what #13744 did for state.repos. Partial test fixtures cast the element rather than the array, so no `as unknown as` laundering is needed. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../dashboard-worktree-launch-options.test.ts | 11 +- .../components/dashboard/useRetainedAgents.ts | 4 +- ...ich-markdown-html-superscript-link.test.ts | 3 +- .../src/hooks/useAgentDetectionTarget.test.ts | 6 +- .../src/lib/folder-workspace-connection.ts | 4 +- .../repos-catalog-merge-identity.test.ts | 280 ++++++++++++++++++ src/renderer/src/store/slices/repos.ts | 167 ++++++++--- 7 files changed, 424 insertions(+), 51 deletions(-) create mode 100644 src/renderer/src/store/slices/repos-catalog-merge-identity.test.ts diff --git a/src/renderer/src/components/dashboard/dashboard-worktree-launch-options.test.ts b/src/renderer/src/components/dashboard/dashboard-worktree-launch-options.test.ts index 42b63e3e993..a0aacdcc504 100644 --- a/src/renderer/src/components/dashboard/dashboard-worktree-launch-options.test.ts +++ b/src/renderer/src/components/dashboard/dashboard-worktree-launch-options.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import { DASHBOARD_MAX_LAUNCH_WORKTREES } from '../../../../shared/dashboard-snapshot' import type { DashboardCard, DashboardWorkspace } from '../../../../shared/dashboard-snapshot' +import type { FolderWorkspace, ProjectGroup } from '../../../../shared/types' import { folderWorkspaceKey } from '../../../../shared/workspace-scope' import { buildDashboardWorktreeLaunchOptions } from './dashboard-worktree-launch-options' @@ -133,9 +134,13 @@ describe('buildDashboardWorktreeLaunchOptions', () => { const options = buildDashboardWorktreeLaunchOptions( state({ folderWorkspaces: [ - { id: 'folder-1', projectGroupId: 'group-1', connectionId: 'ssh-folder' } - ] as LaunchState['folderWorkspaces'], - projectGroups: [{ id: 'group-1' }] as LaunchState['projectGroups'], + { + id: 'folder-1', + projectGroupId: 'group-1', + connectionId: 'ssh-folder' + } as FolderWorkspace + ], + projectGroups: [{ id: 'group-1' } as ProjectGroup], remoteDetectedAgentIds: { 'ssh-folder': ['goose'] } }), [card({ repoId: 'folder-workspace:group-1', worktreeId })] diff --git a/src/renderer/src/components/dashboard/useRetainedAgents.ts b/src/renderer/src/components/dashboard/useRetainedAgents.ts index 1b88c32bb57..6fa1f94fda7 100644 --- a/src/renderer/src/components/dashboard/useRetainedAgents.ts +++ b/src/renderer/src/components/dashboard/useRetainedAgents.ts @@ -24,7 +24,7 @@ type RetainedAgentSnapshot = Map - folderWorkspaces: FolderWorkspace[] + folderWorkspaces: readonly FolderWorkspace[] tabsByWorktree: Record agentStatusByPaneKey: Record } @@ -40,7 +40,7 @@ function paneKeyTabId(paneKey: string): string | null { function buildLiveTabIndex(args: { repos: readonly Repo[] worktreesByRepo: Record - folderWorkspaces: FolderWorkspace[] + folderWorkspaces: readonly FolderWorkspace[] tabsByWorktree: Record }): { existingWorktreeIds: Set diff --git a/src/renderer/src/components/editor/rich-markdown-html-superscript-link.test.ts b/src/renderer/src/components/editor/rich-markdown-html-superscript-link.test.ts index a7dfc9a912e..90591cfdb17 100644 --- a/src/renderer/src/components/editor/rich-markdown-html-superscript-link.test.ts +++ b/src/renderer/src/components/editor/rich-markdown-html-superscript-link.test.ts @@ -29,6 +29,7 @@ import { handleRichMarkdownCitationKey } from './rich-markdown-citation-keyboard import { resolveRichMarkdownWorktreeRoot } from './useRichMarkdownSuperscriptLinkSetup' import { folderWorkspaceKey } from '../../../../shared/workspace-scope' import type { AppState } from '@/store/types' +import type { FolderWorkspace } from '../../../../shared/types' import { inspectRichMarkdownSourceOwningSlice, RICH_MARKDOWN_SOURCE_OWNING_PASTE_LIMIT @@ -309,7 +310,7 @@ describe('rich Markdown HTML superscript links', () => { it('uses a folder workspace path as the citation source root', () => { const state = { - folderWorkspaces: [{ id: 'folder-1', folderPath: '/workspace/platform' }], + folderWorkspaces: [{ id: 'folder-1', folderPath: '/workspace/platform' } as FolderWorkspace], worktreesByRepo: {} } as Pick expect(resolveRichMarkdownWorktreeRoot(state, folderWorkspaceKey('folder-1'))).toBe( diff --git a/src/renderer/src/hooks/useAgentDetectionTarget.test.ts b/src/renderer/src/hooks/useAgentDetectionTarget.test.ts index 1d1c4fa1417..22ad26ae016 100644 --- a/src/renderer/src/hooks/useAgentDetectionTarget.test.ts +++ b/src/renderer/src/hooks/useAgentDetectionTarget.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import type { Repo } from '../../../shared/types' +import type { FolderWorkspace, ProjectGroup, Repo } from '../../../shared/types' import { folderWorkspaceKey } from '../../../shared/workspace-scope' import { getAgentDetectionTargetKeyForWorktree } from './useAgentDetectionTarget' @@ -32,14 +32,14 @@ describe('getAgentDetectionTargetKeyForWorktree', () => { id: 'runtime-folder', projectGroupId: 'runtime-group', folderPath: '/workspace' - } + } as FolderWorkspace ], projectGroups: [ { id: 'runtime-group', connectionId: null, executionHostId: 'runtime:owner-env' - } + } as ProjectGroup ], repos, worktreesByRepo: {} diff --git a/src/renderer/src/lib/folder-workspace-connection.ts b/src/renderer/src/lib/folder-workspace-connection.ts index ee15c61e761..2c9376fc284 100644 --- a/src/renderer/src/lib/folder-workspace-connection.ts +++ b/src/renderer/src/lib/folder-workspace-connection.ts @@ -4,8 +4,8 @@ import { getProjectGroupSubtreeIds } from '../../../shared/project-groups' import { parseExecutionHostId } from '../../../shared/execution-host' export type FolderWorkspaceConnectionState = { - folderWorkspaces: FolderWorkspace[] - projectGroups: ProjectGroup[] + folderWorkspaces: readonly FolderWorkspace[] + projectGroups: readonly ProjectGroup[] repos: readonly Repo[] } diff --git a/src/renderer/src/store/slices/repos-catalog-merge-identity.test.ts b/src/renderer/src/store/slices/repos-catalog-merge-identity.test.ts new file mode 100644 index 00000000000..69f73e589b5 --- /dev/null +++ b/src/renderer/src/store/slices/repos-catalog-merge-identity.test.ts @@ -0,0 +1,280 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { FolderWorkspace, ProjectGroup, Repo } from '../../../../shared/types' +import type { FolderWorkspacePathStatusCacheEntry } from './repos' +import { createTestStore } from './store-test-helpers' + +const projectGroup: ProjectGroup = { + id: 'group-1', + name: 'Group 1', + parentPath: '/parent', + parentGroupId: null, + createdFrom: 'manual', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 +} + +const secondProjectGroup: ProjectGroup = { + ...projectGroup, + id: 'group-2', + name: 'Group 2', + parentPath: '/parent-2', + tabOrder: 1 +} + +const folderWorkspace: FolderWorkspace = { + id: 'folder-1', + projectGroupId: 'group-1', + name: 'Folder 1', + folderPath: '/parent/folder-1', + linkedTask: { + provider: 'github', + type: 'issue', + number: 7, + title: 'Issue 7', + url: 'https://example.test/7' + }, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1 +} + +const secondFolderWorkspace: FolderWorkspace = { + ...folderWorkspace, + id: 'folder-2', + name: 'Folder 2', + folderPath: '/parent/folder-2', + linkedTask: null, + sortOrder: 1 +} + +const repo: Repo = { + id: 'repo-1', + path: '/repo-1', + displayName: 'Repo 1', + badgeColor: '#000', + addedAt: 1, + executionHostId: 'local' +} + +const cachedPathStatuses: Record = { + 'local:folder-workspace:folder-1': { + status: { path: '/parent/folder-1', exists: false, reason: 'missing' }, + checkedAt: Date.now(), + requestSnapshot: 'snapshot' + } +} + +const projectGroupsList = vi.fn() +const folderWorkspacesList = vi.fn() +const reposList = vi.fn() +const projectsList = vi.fn() +const listHostSetups = vi.fn() + +// Why: catalogs arrive over IPC, so every fetch must hand back freshly allocated objects. +function clone(value: T): T { + return structuredClone(value) +} + +beforeEach(() => { + projectGroupsList.mockReset() + folderWorkspacesList.mockReset() + reposList.mockReset() + projectsList.mockReset() + listHostSetups.mockReset() + projectGroupsList.mockImplementation(async () => [clone(projectGroup)]) + folderWorkspacesList.mockImplementation(async () => [clone(folderWorkspace)]) + reposList.mockImplementation(async () => [clone(repo)]) + projectsList.mockImplementation(async () => []) + listHostSetups.mockImplementation(async () => []) + + vi.stubGlobal('window', { + api: { + projectGroups: { list: projectGroupsList }, + folderWorkspaces: { list: folderWorkspacesList }, + repos: { list: reposList }, + projects: { list: projectsList, listHostSetups } + }, + dispatchEvent: vi.fn() + }) +}) + +describe('catalog merge referential stability', () => { + it('keeps the same projectGroups array when a refetch changes nothing', async () => { + const store = createTestStore() + + await store.getState().fetchProjectGroups() + const first = store.getState().projectGroups + expect(first).toEqual([{ ...projectGroup, executionHostId: 'local' }]) + + await store.getState().fetchProjectGroups() + + expect(store.getState().projectGroups).toBe(first) + }) + + it('keeps the same folderWorkspaces array (and entries) when a refetch changes nothing', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + + await store.getState().fetchFolderWorkspaces() + const first = store.getState().folderWorkspaces + expect(first).toHaveLength(1) + const firstEntry = first[0] + + await store.getState().fetchFolderWorkspaces() + + expect(store.getState().folderWorkspaces).toBe(first) + expect(store.getState().folderWorkspaces[0]).toBe(firstEntry) + }) + + it('appends new entries and replaces changed ones while keeping order', async () => { + const store = createTestStore() + + await store.getState().fetchProjectGroups() + const first = store.getState().projectGroups + + projectGroupsList.mockImplementation(async () => [ + clone({ ...projectGroup, name: 'Renamed' }), + clone(secondProjectGroup) + ]) + await store.getState().fetchProjectGroups() + + const merged = store.getState().projectGroups + expect(merged).not.toBe(first) + expect(merged).toEqual([ + { ...projectGroup, name: 'Renamed', executionHostId: 'local' }, + { ...secondProjectGroup, executionHostId: 'local' } + ]) + }) + + it('appends new folder workspaces without disturbing existing order', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + + await store.getState().fetchFolderWorkspaces() + const firstEntry = store.getState().folderWorkspaces[0] + + folderWorkspacesList.mockImplementation(async () => [ + clone(secondFolderWorkspace), + clone(folderWorkspace) + ]) + await store.getState().fetchFolderWorkspaces() + + const merged = store.getState().folderWorkspaces + expect(merged.map((workspace) => workspace.id)).toEqual(['folder-1', 'folder-2']) + // Unchanged entries keep their reference even when the array changes. + expect(merged[0]).toBe(firstEntry) + }) + + it('drops entries that vanish from the fetched catalog', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + + projectGroupsList.mockImplementation(async () => [clone(secondProjectGroup)]) + await store.getState().fetchProjectGroups() + + expect(store.getState().projectGroups).toEqual([ + { ...secondProjectGroup, executionHostId: 'local' } + ]) + }) + + it('treats a nested linkedTask change as a change', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + + await store.getState().fetchFolderWorkspaces() + const first = store.getState().folderWorkspaces + + folderWorkspacesList.mockImplementation(async () => [ + clone({ + ...folderWorkspace, + linkedTask: { ...folderWorkspace.linkedTask!, title: 'Issue 7 renamed' } + }) + ]) + await store.getState().fetchFolderWorkspaces() + + expect(store.getState().folderWorkspaces).not.toBe(first) + expect(store.getState().folderWorkspaces[0]?.linkedTask?.title).toBe('Issue 7 renamed') + }) +}) + +// The sidebar burst that refills this cache only runs when these arrays change reference, so a +// no-op fetch must not wipe it — otherwise stale-folder rows silently fail open forever. +describe('folder path status cache retention across catalog fetches', () => { + it('keeps cached path statuses when a project-group refetch changes nothing', async () => { + const store = createTestStore() + await store.getState().fetchProjectGroups() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + await store.getState().fetchProjectGroups() + + expect(store.getState().folderWorkspacePathStatuses).toBe(cachedPathStatuses) + }) + + it('clears cached path statuses when the project-group catalog changes', async () => { + const store = createTestStore() + await store.getState().fetchProjectGroups() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + projectGroupsList.mockImplementation(async () => [ + clone(projectGroup), + clone(secondProjectGroup) + ]) + await store.getState().fetchProjectGroups() + + expect(store.getState().folderWorkspacePathStatuses).toEqual({}) + }) + + it('keeps cached path statuses when a folder-workspace refetch changes nothing', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + await store.getState().fetchFolderWorkspaces() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + await store.getState().fetchFolderWorkspaces() + + expect(store.getState().folderWorkspacePathStatuses).toBe(cachedPathStatuses) + }) + + it('clears cached path statuses when a folder workspace path changes', async () => { + const store = createTestStore() + store.setState({ projectGroups: [{ ...projectGroup, executionHostId: 'local' }] }) + await store.getState().fetchFolderWorkspaces() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + folderWorkspacesList.mockImplementation(async () => [ + clone({ ...folderWorkspace, folderPath: '/parent/folder-1-renamed' }) + ]) + await store.getState().fetchFolderWorkspaces() + + expect(store.getState().folderWorkspacePathStatuses).toEqual({}) + }) + + it('keeps cached path statuses when a repo refetch changes nothing', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + await store.getState().fetchRepos() + + expect(store.getState().folderWorkspacePathStatuses).toBe(cachedPathStatuses) + }) + + it('clears cached path statuses when the repo catalog changes', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + store.setState({ folderWorkspacePathStatuses: cachedPathStatuses }) + + reposList.mockImplementation(async () => [clone({ ...repo, path: '/repo-1-moved' })]) + await store.getState().fetchRepos() + + expect(store.getState().folderWorkspacePathStatuses).toEqual({}) + }) +}) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 3adbd0ff766..39b9edd20e1 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -859,24 +859,85 @@ function mergeFetchedProjectCompatibilityForHost({ } } +function isPlainCatalogObject(value: unknown): value is Record { + if (typeof value !== 'object' || value === null) { + return false + } + const prototype = Object.getPrototypeOf(value) + return prototype === Object.prototype || prototype === null +} + +// Why: catalog fetches rebuild every entry from IPC, so identity alone never matches; +// structural equality is what lets an unchanged refetch stay a no-op. +function areCatalogEntriesEqual(a: unknown, b: unknown): boolean { + if (a === b) { + return true + } + if (Array.isArray(a) || Array.isArray(b)) { + return ( + Array.isArray(a) && + Array.isArray(b) && + a.length === b.length && + a.every((entry, index) => areCatalogEntriesEqual(entry, b[index])) + ) + } + if (!isPlainCatalogObject(a) || !isPlainCatalogObject(b)) { + return false + } + const keys = Object.keys(a) + if (keys.length !== Object.keys(b).length) { + return false + } + return keys.every( + (key) => Object.prototype.hasOwnProperty.call(b, key) && areCatalogEntriesEqual(a[key], b[key]) + ) +} + +// Why: returning `base` unchanged keeps referential-equality selectors quiet, so a +// `repos:changed` echo doesn't re-force folder path-status fetches for every row. function mergeByIdentity( base: readonly T[], overlay: readonly T[], getIdentity: (entry: T) => string -): T[] { +): readonly T[] { const merged = [...base] const indexById = new Map(merged.map((entry, index) => [getIdentity(entry), index])) + let changed = false for (const entry of overlay) { const identity = getIdentity(entry) const index = indexById.get(identity) if (index === undefined) { indexById.set(identity, merged.length) merged.push(entry) - } else { - merged[index] = entry + changed = true + continue } + if (areCatalogEntriesEqual(merged[index], entry)) { + continue + } + merged[index] = entry + changed = true } - return merged + return changed ? merged : base +} + +// Why: `preserved` keeps `previous`'s order and element refs, so an equal-length no-op +// merge can hand the original store array straight back. +function unchangedMergeSource( + previous: readonly T[], + preserved: readonly T[], + merged: readonly T[] +): readonly T[] { + return merged === preserved && preserved.length === previous.length ? previous : merged +} + +// Why: the sidebar effect watching these catalog arrays is the only thing that refills the +// folder path-status cache, so a no-op refetch must not wipe it — nothing would repopulate it. +function catalogRowsUnchanged(next: readonly T[], previous: readonly T[]): boolean { + return ( + next === previous || + (next.length === previous.length && next.every((row, index) => row === previous[index])) + ) } function mergeFetchedReposForHost( @@ -976,7 +1037,7 @@ function mergeFetchedProjectGroupsForHost( previous: readonly ProjectGroup[], fetched: ProjectGroup[], hostId: string -): ProjectGroup[] { +): readonly ProjectGroup[] { const fetchedIdentities = new Set(fetched.map(getProjectGroupHostIdentity)) const preserved = previous.filter((group) => { const existingHostId = getProjectGroupHostId(group) @@ -985,7 +1046,11 @@ function mergeFetchedProjectGroupsForHost( fetchedIdentities.has(getProjectGroupHostIdentity(group)) ) }) - return mergeByIdentity(preserved, fetched, getProjectGroupHostIdentity) + return unchangedMergeSource( + previous, + preserved, + mergeByIdentity(preserved, fetched, getProjectGroupHostIdentity) + ) } function getFolderWorkspaceHostId( @@ -1033,7 +1098,7 @@ function mergeFetchedFolderWorkspacesForHost({ fetched: FolderWorkspace[] projectGroups: readonly ProjectGroup[] hostId: string -}): FolderWorkspace[] { +}): readonly FolderWorkspace[] { const fetchedIdentities = new Set( fetched.map((workspace) => getFolderWorkspaceHostIdentity(workspace, projectGroups)) ) @@ -1044,8 +1109,12 @@ function mergeFetchedFolderWorkspacesForHost({ fetchedIdentities.has(getFolderWorkspaceHostIdentity(workspace, projectGroups)) ) }) - return mergeByIdentity(preserved, fetched, (workspace) => - getFolderWorkspaceHostIdentity(workspace, projectGroups) + return unchangedMergeSource( + previous, + preserved, + mergeByIdentity(preserved, fetched, (workspace) => + getFolderWorkspaceHostIdentity(workspace, projectGroups) + ) ) } @@ -1240,7 +1309,7 @@ async function fetchProjectGroupCatalogForTarget( function mergeFetchedProjectGroupCatalog( catalog: FetchedProjectGroupCatalog, currentProjectGroups: readonly ProjectGroup[] -): { projectGroups: ProjectGroup[]; hostId: ReturnType } { +): { projectGroups: readonly ProjectGroup[]; hostId: ReturnType } { return { projectGroups: mergeFetchedProjectGroupsForHost( currentProjectGroups, @@ -1293,7 +1362,7 @@ function mergeFetchedFolderWorkspaceCatalog( currentFolderWorkspaces: readonly FolderWorkspace[], projectGroups: readonly ProjectGroup[] ): { - folderWorkspaces: FolderWorkspace[] + folderWorkspaces: readonly FolderWorkspace[] hostId: ReturnType } { return { @@ -1623,8 +1692,8 @@ export type RepoSlice = { repos: readonly Repo[] projects: Project[] projectHostSetups: ProjectHostSetup[] - projectGroups: ProjectGroup[] - folderWorkspaces: FolderWorkspace[] + projectGroups: readonly ProjectGroup[] + folderWorkspaces: readonly FolderWorkspace[] folderWorkspacePathStatuses: Record activeRepoId: string | null // Monotonic sequence so overlapping catalog fetches can drop stale same-host results (#7020). @@ -1933,7 +2002,9 @@ export const createRepoSlice: StateCreator = (set, pendingSshRepoReadoptions: reconciliation.pendingReadoptions, ...reconcileReadoptedSshWorktreeState(s, s.pendingSshRepoReadoptions), ...mergedProjectCompatibility, - folderWorkspacePathStatuses: {}, + ...(catalogRowsUnchanged(prunedRepos, s.repos) + ? {} + : { folderWorkspacePathStatuses: {} }), activeRepoId: s.activeRepoId && validRepoIds.has(s.activeRepoId) ? s.activeRepoId : null, filterRepoIds: s.filterRepoIds.filter((projectId) => validRepoIds.has(projectId)), setupScriptPromptDismissedRepoIds: filterSetupScriptPromptDismissalsToValidRepos( @@ -2084,7 +2155,9 @@ export const createRepoSlice: StateCreator = (set, pendingSshRepoReadoptions: reconciliation.pendingReadoptions, ...reconcileReadoptedSshWorktreeState(s, s.pendingSshRepoReadoptions), ...mergedProjectCompatibility, - folderWorkspacePathStatuses: {}, + ...(catalogRowsUnchanged(finalizedRepos, s.repos) + ? {} + : { folderWorkspacePathStatuses: {} }), activeRepoId: s.activeRepoId, filterRepoIds: s.filterRepoIds, setupScriptPromptDismissedRepoIds: s.setupScriptPromptDismissedRepoIds @@ -2166,15 +2239,18 @@ export const createRepoSlice: StateCreator = (set, if (!isHostCatalogFenceCurrent(get, fence)) { return } - set((current) => - isHostCatalogFenceCurrent(get, fence) - ? { - projectGroups: mergeFetchedProjectGroupCatalog(catalog, current.projectGroups) - .projectGroups, - folderWorkspacePathStatuses: {} - } - : current - ) + set((current) => { + if (!isHostCatalogFenceCurrent(get, fence)) { + return current + } + const { projectGroups } = mergeFetchedProjectGroupCatalog(catalog, current.projectGroups) + return { + projectGroups, + ...(catalogRowsUnchanged(projectGroups, current.projectGroups) + ? {} + : { folderWorkspacePathStatuses: {} }) + } + }) } catch (err) { console.error('Failed to fetch project groups:', err) } @@ -2186,15 +2262,18 @@ export const createRepoSlice: StateCreator = (set, if (!isHostCatalogFenceCurrent(get, fence)) { return } - set((s) => - isHostCatalogFenceCurrent(get, fence) - ? { - projectGroups: mergeFetchedProjectGroupCatalog(catalog, s.projectGroups) - .projectGroups, - folderWorkspacePathStatuses: {} - } - : s - ) + set((s) => { + if (!isHostCatalogFenceCurrent(get, fence)) { + return s + } + const { projectGroups } = mergeFetchedProjectGroupCatalog(catalog, s.projectGroups) + return { + projectGroups, + ...(catalogRowsUnchanged(projectGroups, s.projectGroups) + ? {} + : { folderWorkspacePathStatuses: {} }) + } + }) } try { @@ -2252,7 +2331,12 @@ export const createRepoSlice: StateCreator = (set, current.folderWorkspaces, current.projectGroups ) - return { folderWorkspaces, folderWorkspacePathStatuses: {} } + return { + folderWorkspaces, + ...(catalogRowsUnchanged(folderWorkspaces, current.folderWorkspaces) + ? {} + : { folderWorkspacePathStatuses: {} }) + } }) } catch (err) { console.error('Failed to fetch folder workspaces:', err) @@ -2280,13 +2364,16 @@ export const createRepoSlice: StateCreator = (set, current.projectGroups ) ) + const { folderWorkspaces } = mergeFetchedFolderWorkspaceCatalog( + catalog, + current.folderWorkspaces, + current.projectGroups + ) return { - folderWorkspaces: mergeFetchedFolderWorkspaceCatalog( - catalog, - current.folderWorkspaces, - current.projectGroups - ).folderWorkspaces, - folderWorkspacePathStatuses: {} + folderWorkspaces, + ...(catalogRowsUnchanged(folderWorkspaces, current.folderWorkspaces) + ? {} + : { folderWorkspacePathStatuses: {} }) } }) } From 383ae50a3572465d834816cdfe4f683efa7fad67 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 02:04:10 -0700 Subject: [PATCH 004/216] fix(utf8): stop reading a code point past the end of a sliced string (#13782) Once V8 optimizes the calling function, `String.prototype.codePointAt` on a sliced string pairs a trailing high surrogate with the code unit that follows the SLICE inside its parent, returning a code point the string does not contain. Every UTF-8 byte scan built on `codePointAt` therefore reported one byte too many for a prefix slice cut mid-pair, but only after tier-up, which is what made terminal-stream-byte-length.test.ts fail intermittently on the same commit. Read the units explicitly with `charCodeAt`, which stays bounds-correct in every tier, via a shared `readUtf8CodePointAt`. --- .../rpc/terminal-stream-byte-length.test.ts | 54 +++++++++++++- .../rich-markdown-source-owning-slice.ts | 7 +- .../editor/rich-markdown-source-reconcile.ts | 10 +-- .../terminal-pane/terminal-paste-chunks.ts | 7 +- .../src/lib/agent-draft-paste-content.ts | 11 ++- .../src/lib/comment-body-submit-state.ts | 7 +- .../src/lib/paste-payload-metadata.ts | 11 ++- src/shared/clipboard-text.ts | 3 +- src/shared/utf8-byte-limits.test.ts | 74 ++++++++++++++++++- src/shared/utf8-byte-limits.ts | 28 ++++++- 10 files changed, 184 insertions(+), 28 deletions(-) diff --git a/src/main/runtime/rpc/terminal-stream-byte-length.test.ts b/src/main/runtime/rpc/terminal-stream-byte-length.test.ts index bf07fe38fc8..60c4c86fe44 100644 --- a/src/main/runtime/rpc/terminal-stream-byte-length.test.ts +++ b/src/main/runtime/rpc/terminal-stream-byte-length.test.ts @@ -7,9 +7,24 @@ import { } from './terminal-stream-byte-length' import { TERMINAL_OUTPUT_BATCH_MAX_BYTES } from '../../../shared/terminal-multiplex-flow-control' -// Byte-for-byte copy of the pre-change implementation (shared/clipboard-text.ts +// Copy of the pre-change implementation (shared/clipboard-text.ts // measureClipboardTextByteLength), kept here so equivalence is checked against the -// ACTUAL old code path rather than a paraphrase of it. +// ACTUAL old code path rather than a paraphrase of it. The one deliberate deviation is +// `legacyCodePointAt`: raw `String.prototype.codePointAt` reads one code unit past the end +// of a sliced string once V8 optimizes its caller, so the naive copy is not a stable +// reference. See src/shared/utf8-byte-limits.ts (readUtf8CodePointAt). +function legacyCodePointAt(text: string, index: number): number { + const leadUnit = text.charCodeAt(index) + if (leadUnit < 0xd800 || leadUnit > 0xdbff || index + 1 >= text.length) { + return leadUnit + } + const trailUnit = text.charCodeAt(index + 1) + if (trailUnit < 0xdc00 || trailUnit > 0xdfff) { + return leadUnit + } + return (leadUnit - 0xd800) * 0x400 + (trailUnit - 0xdc00) + 0x10000 +} + function legacyUtf8ByteLengthForCodePoint(codePoint: number): number { if (codePoint <= 0x7f) { return 1 @@ -30,7 +45,7 @@ function legacyMeasure( const stopAfterBytes = options.stopAfterBytes let byteLength = 0 for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = legacyCodePointAt(text, index) byteLength += legacyUtf8ByteLengthForCodePoint(codePoint) if (Number.isFinite(stopAfterBytes) && byteLength > (stopAfterBytes ?? 0)) { return { byteLength, exceededLimit: true } @@ -128,6 +143,39 @@ const EDGE_STRINGS = [ `${'é'.repeat(500)}\ud800` ] +// Regression for the intermittent "measurement diverged at 13 units" failure: the fuzzers +// build a rope and cut it at a fixed code-unit count, which can split a surrogate pair and +// leave the low half in the parent just past the slice. Optimized `codePointAt` pairs across +// that boundary, so the scan measured one byte too many, but only after the enclosing function +// tiered up, which made the failure look load-dependent. 13 code units is V8's minimum length +// for a sliced string, which is why the divergence started exactly there. +describe('measuring a prefix slice that cuts a surrogate pair in half', () => { + // Kept first in the file so the scan is still specializing on this shape when it tiers up. + it('measures the slice like the encoder does in every JIT tier', () => { + const sliced = 'abcdefghijkl\u{1f600}'.slice(0, 13) + expect(sliced.length).toBe(13) + expect(sliced.charCodeAt(12)).toBe(0xd83d) + // 12 ASCII bytes plus U+FFFD for the orphaned high surrogate. + expect(Buffer.byteLength(sliced, 'utf8')).toBe(15) + + const observedByteLengths = new Set() + const observedExceeded = new Set() + const observedMeasurements = new Set() + for (let iteration = 0; iteration < 200_000; iteration += 1) { + observedByteLengths.add(terminalStreamByteLength(sliced)) + observedExceeded.add(terminalStreamByteLengthExceeds(sliced, 15)) + observedMeasurements.add( + JSON.stringify(measureTerminalStreamByteLength(sliced, { stopAfterBytes: 15 })) + ) + } + expect([...observedByteLengths]).toEqual([15]) + expect([...observedExceeded]).toEqual([false]) + expect([...observedMeasurements]).toEqual([ + JSON.stringify({ byteLength: 15, exceededLimit: false }) + ]) + }) +}) + describe('terminal stream byte length equivalence with the legacy code-point scan', () => { it('matches the legacy total byte length on edge strings', () => { for (const text of EDGE_STRINGS) { diff --git a/src/renderer/src/components/editor/rich-markdown-source-owning-slice.ts b/src/renderer/src/components/editor/rich-markdown-source-owning-slice.ts index 51807b15557..622d5c0bc8b 100644 --- a/src/renderer/src/components/editor/rich-markdown-source-owning-slice.ts +++ b/src/renderer/src/components/editor/rich-markdown-source-owning-slice.ts @@ -1,5 +1,8 @@ import type { Slice } from '@tiptap/pm/model' -import { getUtf8ByteLengthForCodePoint } from '../../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../../shared/utf8-byte-limits' import { getRichMarkdownLeafVisibleText, isRichMarkdownVisibleBlockStart @@ -90,7 +93,7 @@ function addUtf8BytesWithinLimit( } let byteLength = current for (let index = 0; index < value.length; index += 1) { - const codePoint = value.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(value, index) byteLength += getUtf8ByteLengthForCodePoint(codePoint) if (byteLength > limit) { return { byteLength, exceeded: true } diff --git a/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts b/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts index 2cff1ed560f..27a776ee163 100644 --- a/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts +++ b/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts @@ -5,7 +5,10 @@ import { makeDiff, makePatches } from '@sanity/diff-match-patch' -import { getUtf8ByteLengthForCodePoint } from '../../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../../shared/utf8-byte-limits' // Why: cap document size in UTF-16 code units (`.length`) since re-parse cost scales with length — the per-commit throwaway TipTap safety re-parse (~50-67ms here) must stay under the 300ms serialize debounce so it can't stall the main thread on slow/SSH hosts. const RECONCILE_SIZE_CAP_CODE_UNITS = 50_000 @@ -147,10 +150,7 @@ function getUtf8OffsetsAtCodeUnitIndices( for (const target of targets) { const boundedTarget = Math.max(0, Math.min(target, text.length)) while (codeUnitIndex < boundedTarget) { - const codePoint = text.codePointAt(codeUnitIndex) - if (codePoint === undefined) { - break - } + const codePoint = readUtf8CodePointAt(text, codeUnitIndex) byteOffset += getUtf8ByteLengthForCodePoint(codePoint) codeUnitIndex += codePoint > 0xffff ? 2 : 1 } diff --git a/src/renderer/src/components/terminal-pane/terminal-paste-chunks.ts b/src/renderer/src/components/terminal-pane/terminal-paste-chunks.ts index 7082f7aff9d..a67e2e6f4ca 100644 --- a/src/renderer/src/components/terminal-pane/terminal-paste-chunks.ts +++ b/src/renderer/src/components/terminal-pane/terminal-paste-chunks.ts @@ -1,7 +1,10 @@ import { BRACKETED_PASTE_END, BRACKETED_PASTE_START } from './terminal-bracketed-paste' import { TERMINAL_PASTE_CHUNK_MAX_BYTES } from './terminal-paste-limits' import type { TerminalPastePlan } from './terminal-paste-coordinator' -import { getUtf8ByteLengthForCodePoint } from '../../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../../shared/utf8-byte-limits' const TERMINAL_PASTE_ESCAPE_CODE_POINT = 0x1b const TERMINAL_PASTE_INERT_ESCAPE_CODE_POINT = 0x241b @@ -38,7 +41,7 @@ function* iterateTextByUtf8Bytes( let chunk = '' let chunkBytes = 0 for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) const codeUnitLength = codePoint > 0xffff ? 2 : 1 // Why: iterator normalization avoids a full-size copy and keeps CRLF atomic across chunks. if ( diff --git a/src/renderer/src/lib/agent-draft-paste-content.ts b/src/renderer/src/lib/agent-draft-paste-content.ts index b6c381dc5a9..526e717de27 100644 --- a/src/renderer/src/lib/agent-draft-paste-content.ts +++ b/src/renderer/src/lib/agent-draft-paste-content.ts @@ -1,6 +1,9 @@ import { yieldToEventLoop } from '../../../shared/event-loop-yield' import type { GlobalSettings } from '../../../shared/types' -import { getUtf8ByteLengthForCodePoint } from '../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../shared/utf8-byte-limits' import { BRACKETED_PASTE_END, BRACKETED_PASTE_START, @@ -110,7 +113,7 @@ export function* iterateAgentDraftPasteContentChunks( let chunkBytes = 0 for (let index = 0; index < terminalContent.length; index += 1) { - const codePoint = terminalContent.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(terminalContent, index) const codeUnitLength = codePoint > 0xffff ? 2 : 1 const sanitizedEscape = codePoint === AGENT_DRAFT_PASTE_ESCAPE_CODE_POINT const sanitized = sanitizedEscape @@ -150,7 +153,7 @@ function measureSanitizedUtf8ByteLength( let byteLength = 0 const stopAfterBytes = options.stopAfterBytes for (let index = 0; index < content.length; index += 1) { - const codePoint = content.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(content, index) byteLength += getSanitizedUtf8ByteLengthForCodePoint(codePoint) if (Number.isFinite(stopAfterBytes) && byteLength > (stopAfterBytes ?? 0)) { return { byteLength, exceededLimit: true } @@ -166,7 +169,7 @@ async function isSanitizedDraftPasteOverLimit(content: string, maxBytes: number) let byteLength = 0 let nextYieldAt = AGENT_DRAFT_PASTE_PREFLIGHT_YIELD_CODE_UNITS for (let index = 0; index < content.length; index += 1) { - const codePoint = content.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(content, index) byteLength += getSanitizedUtf8ByteLengthForCodePoint(codePoint) if (byteLength > maxBytes) { return true diff --git a/src/renderer/src/lib/comment-body-submit-state.ts b/src/renderer/src/lib/comment-body-submit-state.ts index 4a7066f6408..460200d3e10 100644 --- a/src/renderer/src/lib/comment-body-submit-state.ts +++ b/src/renderer/src/lib/comment-body-submit-state.ts @@ -1,4 +1,7 @@ -import { getUtf8ByteLengthForCodePoint } from '../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../shared/utf8-byte-limits' export const COMMENT_BODY_NONBLANK_SCAN_MAX_BYTES = 64 * 1024 @@ -16,7 +19,7 @@ function getCommentBodyPresence( let scannedBytes = 0 for (let index = 0; index < body.length; index += 1) { - const codePoint = body.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(body, index) const codeUnitLength = codePoint > 0xffff ? 2 : 1 scannedBytes += getUtf8ByteLengthForCodePoint(codePoint) if (scannedBytes > maxScanBytes) { diff --git a/src/renderer/src/lib/paste-payload-metadata.ts b/src/renderer/src/lib/paste-payload-metadata.ts index f28e49b24f2..f29894446b2 100644 --- a/src/renderer/src/lib/paste-payload-metadata.ts +++ b/src/renderer/src/lib/paste-payload-metadata.ts @@ -1,5 +1,8 @@ import { yieldToEventLoop } from '../../../shared/event-loop-yield' -import { getUtf8ByteLengthForCodePoint } from '../../../shared/utf8-byte-limits' +import { + getUtf8ByteLengthForCodePoint, + readUtf8CodePointAt +} from '../../../shared/utf8-byte-limits' export type PastePayloadMetadata = { byteLength: number @@ -25,7 +28,7 @@ export function measurePastePayloadMetadata( let previousWasCarriageReturn = false for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) byteLength += getUtf8ByteLengthForCodePoint(codePoint) hasControlSequences ||= isPasteControlSequenceCodePoint(codePoint) if (codePoint === 0x0d) { @@ -73,7 +76,7 @@ export async function measurePastePayloadMetadataWithYield( let previousWasCarriageReturn = false for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) byteLength += getUtf8ByteLengthForCodePoint(codePoint) hasControlSequences ||= isPasteControlSequenceCodePoint(codePoint) if (codePoint === 0x0d) { @@ -110,7 +113,7 @@ export function countPastePayloadLines(text: string): number { export function hasPastePayloadControlSequence(text: string): boolean { for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) if (isPasteControlSequenceCodePoint(codePoint)) { return true } diff --git a/src/shared/clipboard-text.ts b/src/shared/clipboard-text.ts index e8170af606b..c5d641e93d6 100644 --- a/src/shared/clipboard-text.ts +++ b/src/shared/clipboard-text.ts @@ -2,6 +2,7 @@ import { yieldToEventLoop } from './event-loop-yield' import { getUtf8ByteLengthForCodePoint, measureUtf8ByteLength, + readUtf8CodePointAt, type Utf8ByteLengthMeasurement } from './utf8-byte-limits' @@ -50,7 +51,7 @@ export async function measureClipboardTextByteLengthWithYield( let byteLength = 0 for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) byteLength += getUtf8ByteLengthForCodePoint(codePoint) if (Number.isFinite(stopAfterBytes) && byteLength > (stopAfterBytes ?? 0)) { return { byteLength, exceededLimit: true } diff --git a/src/shared/utf8-byte-limits.test.ts b/src/shared/utf8-byte-limits.test.ts index 9b3c63c9cea..9556a5c39bc 100644 --- a/src/shared/utf8-byte-limits.test.ts +++ b/src/shared/utf8-byte-limits.test.ts @@ -1,5 +1,77 @@ import { describe, expect, it } from 'vitest' -import { getUtf8ChunkEndIndex, isUtf8ByteLengthWithinLimit } from './utf8-byte-limits' +import { + clampUtf8TextPrefix, + getUtf8ChunkEndIndex, + isUtf8ByteLengthWithinLimit, + measureUtf8ByteLength, + readUtf8CodePointAt +} from './utf8-byte-limits' + +// Once V8 optimizes the calling function, `String.prototype.codePointAt` on a sliced string +// pairs a trailing high surrogate with the code unit that follows the SLICE inside its parent +// (reproduced on Node 24 and 26). A prefix slice cut mid-pair then reads a code point the +// string does not contain, so a scan reports one byte too many — but only after tier-up, which +// is why it surfaced as an intermittent CI failure rather than a deterministic one. +// Build a rope, cut it mid-pair, and hammer the scan so the optimizing tier is under test. +const SLICE_BOUNDARY_PARENT_UNITS = [ + 0x72, 0x2e6, 0x7b, 0x54, 0xda9b, 0x568, 0x52, 0x26, 0x46, 0xc15b, 0x7, 0x768, 0xd9f6, 0xdcde +] +// V8 only creates a sliced string (rather than copying) at 13 code units or more. +const SLICE_BOUNDARY_UNITS = 13 +const TIER_UP_ITERATIONS = 200_000 + +function buildSliceEndingInLoneHighSurrogate(): string { + let text = '' + for (const unit of SLICE_BOUNDARY_PARENT_UNITS) { + text += String.fromCharCode(unit) + } + return text.slice(0, SLICE_BOUNDARY_UNITS) +} + +describe('scanning a prefix slice whose parent continues past the slice', () => { + const sliced = buildSliceEndingInLoneHighSurrogate() + + it('is a sliced string that ends in a lone high surrogate', () => { + expect(sliced.length).toBe(SLICE_BOUNDARY_UNITS) + expect(sliced.charCodeAt(SLICE_BOUNDARY_UNITS - 1)).toBe(0xd9f6) + expect(Buffer.byteLength(sliced, 'utf8')).toBe(22) + }) + + it('reads the trailing lone surrogate without pairing past the end in every JIT tier', () => { + const observed = new Set() + for (let iteration = 0; iteration < TIER_UP_ITERATIONS; iteration += 1) { + observed.add(readUtf8CodePointAt(sliced, SLICE_BOUNDARY_UNITS - 1)) + } + expect([...observed]).toEqual([0xd9f6]) + }) + + it('measures the same byte length as the encoder in every JIT tier', () => { + const expected = Buffer.byteLength(sliced, 'utf8') + const observed = new Set() + for (let iteration = 0; iteration < TIER_UP_ITERATIONS; iteration += 1) { + observed.add(measureUtf8ByteLength(sliced).byteLength) + } + expect([...observed]).toEqual([expected]) + }) + + it('does not report an exceeded limit at the true byte length in every JIT tier', () => { + const limit = Buffer.byteLength(sliced, 'utf8') + const observed = new Set() + for (let iteration = 0; iteration < TIER_UP_ITERATIONS; iteration += 1) { + observed.add(measureUtf8ByteLength(sliced, { stopAfterBytes: limit }).exceededLimit) + } + expect([...observed]).toEqual([false]) + }) + + it('keeps the whole slice when clamping to its true byte length in every JIT tier', () => { + const limit = Buffer.byteLength(sliced, 'utf8') + const observed = new Set() + for (let iteration = 0; iteration < TIER_UP_ITERATIONS; iteration += 1) { + observed.add(clampUtf8TextPrefix(sliced, limit).length) + } + expect([...observed]).toEqual([SLICE_BOUNDARY_UNITS]) + }) +}) describe('getUtf8ChunkEndIndex', () => { it.each([ diff --git a/src/shared/utf8-byte-limits.ts b/src/shared/utf8-byte-limits.ts index b32bd968d57..df8f99f68ad 100644 --- a/src/shared/utf8-byte-limits.ts +++ b/src/shared/utf8-byte-limits.ts @@ -8,6 +8,26 @@ export type Utf8TextTail = { bytes: number } +/** + * Bounds-safe replacement for `String.prototype.codePointAt`. + * + * Why: once V8 optimizes the calling function, `codePointAt` on a sliced string pairs a + * trailing high surrogate with the code unit that follows the SLICE inside its parent, so a + * prefix slice cut mid-pair reports a code point the string does not contain (and one byte + * too many). `charCodeAt` stays bounds-correct in every tier, so pair the units explicitly. + */ +export function readUtf8CodePointAt(text: string, index: number): number { + const leadUnit = text.charCodeAt(index) + if (leadUnit < 0xd800 || leadUnit > 0xdbff || index + 1 >= text.length) { + return leadUnit + } + const trailUnit = text.charCodeAt(index + 1) + if (trailUnit < 0xdc00 || trailUnit > 0xdfff) { + return leadUnit + } + return (leadUnit - 0xd800) * 0x400 + (trailUnit - 0xdc00) + 0x10000 +} + export function measureUtf8ByteLength( text: string, options: { stopAfterBytes?: number } = {} @@ -15,7 +35,7 @@ export function measureUtf8ByteLength( const stopAfterBytes = options.stopAfterBytes let byteLength = 0 for (let index = 0; index < text.length; index += 1) { - const codePoint = text.codePointAt(index) ?? 0 + const codePoint = readUtf8CodePointAt(text, index) byteLength += getUtf8ByteLengthForCodePoint(codePoint) if (Number.isFinite(stopAfterBytes) && byteLength > (stopAfterBytes ?? 0)) { return { byteLength, exceededLimit: true } @@ -69,7 +89,7 @@ export function clampUtf8TextPrefix(text: string, maxBytes: number): string { let bytes = 0 let end = 0 while (end < text.length) { - const codePoint = text.codePointAt(end) ?? 0 + const codePoint = readUtf8CodePointAt(text, end) const codePointBytes = getUtf8ByteLengthForCodePoint(codePoint) if (bytes + codePointBytes > maxBytes) { break @@ -84,7 +104,7 @@ export function getUtf8ChunkEndIndex(text: string, startIndex: number, maxBytes: let bytes = 0 let endIndex = startIndex while (endIndex < text.length) { - const codePoint = text.codePointAt(endIndex) ?? 0 + const codePoint = readUtf8CodePointAt(text, endIndex) const codePointBytes = getUtf8ByteLengthForCodePoint(codePoint) if (bytes > 0 && bytes + codePointBytes > maxBytes) { break @@ -123,6 +143,6 @@ function getPreviousUtf8CodePoint( } return { start, - bytes: getUtf8ByteLengthForCodePoint(text.codePointAt(start) ?? codeUnit) + bytes: getUtf8ByteLengthForCodePoint(readUtf8CodePointAt(text, start)) } } From 3713dd73760a963f20fe8c444f70fe1dd0da96d7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 02:18:29 -0700 Subject: [PATCH 005/216] perf(git): reuse pinned OIDs for SSH file diffs (#13586) Forward the renderer's already-pinned {mergeBase, headOid} to the SSH relay so a single-file branch diff reads the two blobs directly instead of rediscovering live HEAD. Six sequential git processes become two concurrent reads, and a branch move mid-review no longer changes which revision is displayed. Equivalence with the legacy route is proven against real Git across 14 change types; wire compatibility is proven over a real SSH socket against relay bundles built from main and from the pre-merge-base. --- src/main/ipc/filesystem.test.ts | 36 ++ src/main/ipc/filesystem.ts | 1 + src/main/providers/git-provider-contract.ts | 2 +- src/main/providers/ssh-git-provider.test.ts | 119 ++++++ src/main/providers/ssh-git-provider.ts | 14 +- .../orca-runtime-git-branch-diff.test.ts | 69 +++ src/main/runtime/orca-runtime-git.ts | 1 + ...it-handler-branch-diff-equivalence.test.ts | 207 +++++++++ src/relay/git-handler-branch-diff-ops.ts | 57 +++ src/relay/git-handler-branch-diff.test.ts | 392 ++++++++++++++++++ src/relay/git-handler.ts | 28 +- .../useEditorPanelContentState.test.tsx | 2 +- src/shared/git-binary-compatibility.test.ts | 21 + 13 files changed, 942 insertions(+), 7 deletions(-) create mode 100644 src/main/runtime/orca-runtime-git-branch-diff.test.ts create mode 100644 src/relay/git-handler-branch-diff-equivalence.test.ts create mode 100644 src/relay/git-handler-branch-diff-ops.ts create mode 100644 src/relay/git-handler-branch-diff.test.ts diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index ad68010cbb7..2c66ab9ad09 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -3098,6 +3098,42 @@ describe('registerFilesystemHandlers', () => { ) }) + it('forwards the pinned head through SSH branch diff queries', async () => { + const result = { + kind: 'text', + originalContent: 'left', + modifiedContent: 'right', + originalIsBinary: false, + modifiedIsBinary: false + } + const getBranchDiff = vi.fn().mockResolvedValue([result]) + getSshGitProviderMock.mockReturnValue({ getBranchDiff }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:branchDiff')!(null, { + worktreePath: '/home/user/project', + compare: { + baseRef: 'origin/main', + baseOid: 'base-oid', + headOid: 'head-oid', + mergeBase: 'merge-base-oid' + }, + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts', + connectionId: 'conn-1' + }) + ).resolves.toEqual(result) + + expect(getBranchDiff).toHaveBeenCalledWith('/home/user/project', 'merge-base-oid', { + includePatch: true, + headOid: 'head-oid', + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }) + }) + // Why: the original SSH Quick Open bug had two halves — relay-side policy // drift AND the main dispatcher silently dropping excludePaths before the // provider saw them. This test guards the second half: regardless of diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index d5fb98086f8..887acaf539f 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -2076,6 +2076,7 @@ export function registerFilesystemHandlers( } const results = await provider.getBranchDiff(args.worktreePath, args.compare.mergeBase, { includePatch: true, + headOid: args.compare.headOid, filePath: args.filePath, oldPath: args.oldPath }) diff --git a/src/main/providers/git-provider-contract.ts b/src/main/providers/git-provider-contract.ts index c204b9d9016..d13bf5addf9 100644 --- a/src/main/providers/git-provider-contract.ts +++ b/src/main/providers/git-provider-contract.ts @@ -66,7 +66,7 @@ export type IGitProvider = { getBranchDiff( worktreePath: string, baseRef: string, - options?: { includePatch?: boolean; filePath?: string; oldPath?: string } + options?: { includePatch?: boolean; filePath?: string; oldPath?: string; headOid?: string } ): Promise getCommitDiff( worktreePath: string, diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index c0613f35f2e..20ebd9293a1 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -1081,6 +1081,125 @@ describe('SshGitProvider', () => { expect(result).toEqual(diffs) }) + it('getBranchDiff forwards a pinned head OID without adding it when omitted', async () => { + const headOid = 'a'.repeat(40) + mux.request.mockResolvedValue([]) + + await provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid + }) + await provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/other-file.ts', + headOid: undefined + }) + + expect(mux.request).toHaveBeenNthCalledWith(1, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts', + headOid, + __streamResponse: true + }) + expect(mux.request).toHaveBeenNthCalledWith(2, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/other-file.ts', + __streamResponse: true + }) + }) + + // Why: an unpinned compare snapshot reaches getBranchDiff as null despite the type. + it('omits a null head OID and shares the absent-head dedupe key', async () => { + const diffs = [{ kind: 'text', originalContent: 'old', modifiedContent: 'new' }] + const pendingDiff = deferredValue(diffs) + const headOid = 'a'.repeat(40) + mux.request.mockReturnValue(pendingDiff.promise) + + const reads = [ + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid: null as unknown as undefined + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid + }) + ] + + await waitForRequestCount(mux.request, 2) + expect(mux.request).toHaveBeenCalledTimes(2) + expect(mux.request).toHaveBeenNthCalledWith(1, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts', + __streamResponse: true + }) + expect(mux.request).toHaveBeenNthCalledWith(2, 'git.branchDiff', { + worktreePath: '/home/user/repo', + baseRef: 'main', + includePatch: true, + filePath: 'src/file.ts', + headOid, + __streamResponse: true + }) + + pendingDiff.resolve() + await expect(Promise.all(reads)).resolves.toEqual(Array.from({ length: 3 }, () => diffs)) + }) + + it('coalesces matching branch diff heads while keeping distinct and absent heads separate', async () => { + const diffs = [{ kind: 'text', originalContent: 'old', modifiedContent: 'new' }] + const pendingDiff = deferredValue(diffs) + const firstHeadOid = 'a'.repeat(40) + const secondHeadOid = 'b'.repeat(40) + mux.request.mockReturnValue(pendingDiff.promise) + + const reads = [ + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid: firstHeadOid + }), + provider.getBranchDiff('/home/user/repo', 'main', { + filePath: 'src/file.ts', + headOid: firstHeadOid, + includePatch: true + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid: secondHeadOid + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts' + }), + provider.getBranchDiff('/home/user/repo', 'main', { + includePatch: true, + filePath: 'src/file.ts', + headOid: undefined + }) + ] + + await waitForRequestCount(mux.request, 3) + expect(mux.request).toHaveBeenCalledTimes(3) + + pendingDiff.resolve() + await expect(Promise.all(reads)).resolves.toEqual(Array.from({ length: 5 }, () => diffs)) + }) + it('coalesces concurrent identical diff RPCs while in flight', async () => { const diff = { kind: 'text', diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index e4c306529ee..51e2b91e1c2 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -41,6 +41,7 @@ type NonInteractiveExecQueueEntry = { } const NON_INTERACTIVE_TRANSPORT_TIMEOUT_MARGIN_MS = 5_000 +const ABSENT_BRANCH_DIFF_HEAD_OID = { absent: true } as const function isJsonRpcMethodNotFoundError(error: unknown): boolean { if (!error || typeof error !== 'object') { @@ -658,9 +659,14 @@ export class SshGitProvider implements IGitProvider { async getBranchDiff( worktreePath: string, baseRef: string, - options?: { includePatch?: boolean; filePath?: string; oldPath?: string } + options?: { includePatch?: boolean; filePath?: string; oldPath?: string; headOid?: string } ): Promise { const keyOptions = options ?? {} + const { headOid: rawHeadOid, ...relayOptions } = keyOptions + // Why: compare snapshots type headOid as `string | null`, so collapse an + // unpinned null to absent instead of putting a field on the wire that a + // pinned-OID relay must reject. + const headOid = rawHeadOid == null ? undefined : rawHeadOid return this.gitDiffReadDedupe.run( stableInFlightKey([ 'branchDiff', @@ -668,13 +674,15 @@ export class SshGitProvider implements IGitProvider { baseRef, keyOptions.includePatch ?? null, keyOptions.filePath ?? null, - keyOptions.oldPath ?? null + keyOptions.oldPath ?? null, + headOid === undefined ? ABSENT_BRANCH_DIFF_HEAD_OID : headOid ]), async () => (await requestGitStreamable(this.mux, 'git.branchDiff', { worktreePath, baseRef, - ...options + ...relayOptions, + ...(headOid === undefined ? {} : { headOid }) })) as GitDiffResult[] ) as Promise } diff --git a/src/main/runtime/orca-runtime-git-branch-diff.test.ts b/src/main/runtime/orca-runtime-git-branch-diff.test.ts new file mode 100644 index 00000000000..4c469758760 --- /dev/null +++ b/src/main/runtime/orca-runtime-git-branch-diff.test.ts @@ -0,0 +1,69 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GlobalSettings } from '../../shared/types' +import { RuntimeGitCommands, type ResolvedRuntimeGitWorktree } from './orca-runtime-git' + +const getSshGitProviderMock = vi.hoisted(() => vi.fn()) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +function makeWorktree(path: string): ResolvedRuntimeGitWorktree { + const worktree = { + id: 'wt-1', + repoId: 'repo-1', + path, + linkedIssue: null, + git: { + path, + branch: 'main', + isBare: false, + isMainWorktree: false, + head: 'a'.repeat(40) + } + } satisfies Partial + return worktree as unknown as ResolvedRuntimeGitWorktree +} + +describe('RuntimeGitCommands branch diff', () => { + beforeEach(() => { + getSshGitProviderMock.mockReset() + }) + + it('forwards the pinned head through normalized nested SSH paths', async () => { + const mergeBase = 'a'.repeat(40) + const headOid = 'b'.repeat(40) + const result = { + kind: 'text', + originalContent: 'left', + modifiedContent: 'right', + originalIsBinary: false, + modifiedIsBinary: false + } + const provider = { getBranchDiff: vi.fn().mockResolvedValue([result]) } + getSshGitProviderMock.mockReturnValue(provider) + const commands = new RuntimeGitCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree: makeWorktree('/remote/repo'), + connectionId: 'conn-1' + }), + getRuntimeSettings: () => ({}) as GlobalSettings + }) + + await expect( + commands.getRuntimeGitBranchDiff( + 'id:wt-1', + { mergeBase, headOid }, + 'src\\file.ts', + 'src\\old-file.ts' + ) + ).resolves.toEqual(result) + + expect(provider.getBranchDiff).toHaveBeenCalledWith('/remote/repo', mergeBase, { + includePatch: true, + headOid, + filePath: 'src/file.ts', + oldPath: 'src/old-file.ts' + }) + }) +}) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index 88fb102f7e9..0c1444c0294 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -538,6 +538,7 @@ export class RuntimeGitCommands { } const results = await provider.getBranchDiff(target.worktree.path, compare.mergeBase, { includePatch: true, + headOid: compare.headOid, filePath: relativePath, oldPath: oldRelativePath }) diff --git a/src/relay/git-handler-branch-diff-equivalence.test.ts b/src/relay/git-handler-branch-diff-equivalence.test.ts new file mode 100644 index 00000000000..d33886b4f5a --- /dev/null +++ b/src/relay/git-handler-branch-diff-equivalence.test.ts @@ -0,0 +1,207 @@ +import { execFileSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { RelayContext } from './context' +import { GitHandler } from './git-handler' +import { + createMockDispatcher, + gitCommit, + gitInit, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' + +// Why this file exists: the pinned route replaced the legacy route's own +// `diff --name-status -M -C` rediscovery with values the caller already holds. +// These tests drive the real product contract against real Git and assert the +// two routes agree entry-for-entry, so "SSH still renders what it used to" is +// evidence rather than an argument about call sites. + +type BranchCompareResult = { + summary: { mergeBase: string; headOid: string; status: string; changedFiles: number } + entries: { path: string; oldPath?: string; status: string }[] +} + +type DiffEntry = Record + +function git(repoPath: string, args: string[]): string { + return execFileSync('git', args, { cwd: repoPath, encoding: 'utf8' }) +} + +/** + * Builds one repo whose base..head range exercises every shape the review + * panel can hand to a single-file branch diff. + */ +function buildScenarioRepo(): { repoPath: string; baseOid: string } { + const repoPath = mkdtempSync(path.join(tmpdir(), 'relay-branch-diff-equivalence-')) + gitInit(repoPath) + + const write = (relativePath: string, contents: string | Buffer): void => { + const target = path.join(repoPath, relativePath) + mkdirSync(path.dirname(target), { recursive: true }) + writeFileSync(target, contents) + } + + write('modified.txt', 'before\n') + write('deleted.txt', 'doomed\n') + write('renamed-from.txt', 'stable rename payload\n') + write('renamed-and-edited-from.txt', 'rename plus edit, line one\nline two\nline three\n') + write('copied-source.txt', 'copy me, line one\nline two\nline three\nline four\n') + write('nested/deep/inner.txt', 'nested before\n') + write('spaced name.txt', 'spaced before\n') + write('ünïcode-ページ.txt', 'unicode before\n') + write('binary-modified.bin', Buffer.from([0, 1, 2, 3, 0, 255])) + write('binary-deleted.bin', Buffer.from([9, 8, 7, 0])) + write('emptied.txt', 'about to be emptied\n') + write('crlf.txt', 'crlf before\r\nsecond line\r\n') + write('mode-changed.sh', '#!/bin/sh\necho hi\n') + gitCommit(repoPath, 'base') + const baseOid = git(repoPath, ['rev-parse', 'HEAD']).trim() + + write('modified.txt', 'after\n') + rmSync(path.join(repoPath, 'deleted.txt')) + rmSync(path.join(repoPath, 'binary-deleted.bin')) + git(repoPath, ['mv', 'renamed-from.txt', 'renamed-to.txt']) + git(repoPath, ['mv', 'renamed-and-edited-from.txt', 'renamed-and-edited-to.txt']) + write('renamed-and-edited-to.txt', 'rename plus edit, line one\nline two CHANGED\nline three\n') + write('copied-target.txt', 'copy me, line one\nline two\nline three\nline four\n') + write('added.txt', 'brand new\n') + write('binary-added.bin', Buffer.from([4, 4, 0, 4])) + write('binary-modified.bin', Buffer.from([0, 1, 2, 3, 0, 254])) + write('nested/deep/inner.txt', 'nested after\n') + write('spaced name.txt', 'spaced after\n') + write('ünïcode-ページ.txt', 'unicode after\n') + write('emptied.txt', '') + write('crlf.txt', 'crlf after\r\nsecond line\r\n') + git(repoPath, ['update-index', '--chmod=+x', 'mode-changed.sh']) + gitCommit(repoPath, 'head') + + return { repoPath, baseOid } +} + +describe('pinned and legacy branch diff equivalence against real Git', () => { + let dispatcher: MockDispatcher + let handler: GitHandler + let repoPath = '' + let baseOid = '' + + beforeEach(() => { + dispatcher = createMockDispatcher() + handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + const built = buildScenarioRepo() + repoPath = built.repoPath + baseOid = built.baseOid + }) + + afterEach(() => { + handler.dispose() + if (repoPath) { + rmSync(repoPath, { recursive: true, force: true }) + } + }) + + async function branchCompare(): Promise { + return (await dispatcher.callRequest('git.branchCompare', { + worktreePath: repoPath, + baseRef: baseOid + })) as BranchCompareResult + } + + async function branchDiff(params: Record): Promise { + return (await dispatcher.callRequest('git.branchDiff', { + worktreePath: repoPath, + baseRef: baseOid, + includePatch: true, + ...params + })) as DiffEntry[] + } + + it('produces identical results for every changed file the review panel can open', async () => { + const compare = await branchCompare() + expect(compare.summary.status).toBe('ready') + // Guard the guard: a truncated scenario set would make this test vacuously pass. + expect(compare.entries.length).toBeGreaterThanOrEqual(14) + + const divergences: string[] = [] + for (const entry of compare.entries) { + // Exactly what the renderer sends: paths from the compare entry list, + // OIDs from the compare summary that produced that same list. + const callerShape = { filePath: entry.path, oldPath: entry.oldPath } + const legacy = await branchDiff(callerShape) + const pinned = await branchDiff({ + ...callerShape, + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid + }) + + if (JSON.stringify(legacy) !== JSON.stringify(pinned)) { + divergences.push( + `${entry.status} ${entry.path}${entry.oldPath ? ` (from ${entry.oldPath})` : ''}\n` + + ` legacy: ${JSON.stringify(legacy)}\n pinned: ${JSON.stringify(pinned)}` + ) + } + } + + expect(divergences.join('\n')).toBe('') + }) + + it('agrees on content for renames, additions, deletions and binaries specifically', async () => { + const compare = await branchCompare() + const byPath = new Map(compare.entries.map((entry) => [entry.path, entry])) + + // Why assert content and not just equality: two identically-empty results + // would satisfy the equivalence test above while rendering nothing. + const rename = byPath.get('renamed-and-edited-to.txt') + expect(rename?.oldPath).toBe('renamed-and-edited-from.txt') + const [renamePinned] = await branchDiff({ + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid, + filePath: rename!.path, + oldPath: rename!.oldPath + }) + expect(renamePinned).toMatchObject({ + originalContent: 'rename plus edit, line one\nline two\nline three\n', + modifiedContent: 'rename plus edit, line one\nline two CHANGED\nline three\n' + }) + + const [addedPinned] = await branchDiff({ + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid, + filePath: 'added.txt' + }) + expect(addedPinned).toMatchObject({ originalContent: '', modifiedContent: 'brand new\n' }) + + const [deletedPinned] = await branchDiff({ + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid, + filePath: 'deleted.txt' + }) + expect(deletedPinned).toMatchObject({ originalContent: 'doomed\n', modifiedContent: '' }) + + const [binaryPinned] = await branchDiff({ + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid, + filePath: 'binary-modified.bin' + }) + expect(binaryPinned).toMatchObject({ kind: 'binary' }) + }) + + it('holds the pinned revision when HEAD moves mid-review, where legacy drifts', async () => { + const compare = await branchCompare() + writeFileSync(path.join(repoPath, 'modified.txt'), 'drifted after the snapshot\n') + gitCommit(repoPath, 'drift') + + const pinned = await branchDiff({ + baseRef: compare.summary.mergeBase, + headOid: compare.summary.headOid, + filePath: 'modified.txt' + }) + const legacy = await branchDiff({ filePath: 'modified.txt' }) + + expect(pinned[0]).toMatchObject({ modifiedContent: 'after\n' }) + // The divergence is the fix: legacy silently re-resolves live HEAD. + expect(legacy[0]).toMatchObject({ modifiedContent: 'drifted after the snapshot\n' }) + }) +}) diff --git a/src/relay/git-handler-branch-diff-ops.ts b/src/relay/git-handler-branch-diff-ops.ts new file mode 100644 index 00000000000..1da0de82be8 --- /dev/null +++ b/src/relay/git-handler-branch-diff-ops.ts @@ -0,0 +1,57 @@ +import { buildDiffResult } from './git-diff-result' +import { readBlobAtOid, type GitBufferExec } from './git-handler-ops' + +const FULL_GIT_OBJECT_ID_PATTERN = /^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/ + +function assertFullGitObjectId(value: unknown, label: string): asserts value is string { + if (typeof value !== 'string' || !FULL_GIT_OBJECT_ID_PATTERN.test(value)) { + throw new Error(`${label} must be a full git object id`) + } +} + +export function isFullGitObjectId(value: unknown): value is string { + return typeof value === 'string' && FULL_GIT_OBJECT_ID_PATTERN.test(value) +} + +export function parseOptionalBranchDiffHeadOid( + params: Record +): string | undefined { + const { headOid } = params + // Why: GitBranchCompareSummary.headOid is `string | null`, so a mixed-version + // client can put an explicit null on the wire. Treat it as unpinned rather + // than rejecting a request the legacy path would have served. + if (headOid == null) { + return undefined + } + assertFullGitObjectId(headOid, 'headOid') + return headOid +} + +export async function branchDiffEntryAtPinnedOids( + gitBuffer: GitBufferExec, + worktreePath: string, + baseOid: string, + headOid: string, + filePath: string, + oldPath?: string +) { + assertFullGitObjectId(baseOid, 'baseRef') + assertFullGitObjectId(headOid, 'headOid') + try { + const [left, right] = await Promise.all([ + readBlobAtOid(gitBuffer, worktreePath, baseOid, oldPath ?? filePath), + readBlobAtOid(gitBuffer, worktreePath, headOid, filePath) + ]) + return [buildDiffResult(left.content, right.content, left.isBinary, right.isBinary, filePath)] + } catch { + return [ + { + kind: 'text' as const, + originalContent: '', + modifiedContent: '', + originalIsBinary: false, + modifiedIsBinary: false + } + ] + } +} diff --git a/src/relay/git-handler-branch-diff.test.ts b/src/relay/git-handler-branch-diff.test.ts new file mode 100644 index 00000000000..ce60c8cd439 --- /dev/null +++ b/src/relay/git-handler-branch-diff.test.ts @@ -0,0 +1,392 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { RelayContext } from './context' +import { + branchDiffEntryAtPinnedOids, + isFullGitObjectId, + parseOptionalBranchDiffHeadOid +} from './git-handler-branch-diff-ops' +import { GitHandler } from './git-handler' +import { branchDiffEntries, type GitBufferExec, type GitExec } from './git-handler-ops' +import { + createMockDispatcher, + gitCommit, + gitInit, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' + +const BASE_OID = 'a'.repeat(40) +const HEAD_OID = 'b'.repeat(40) +const OTHER_HEAD_OID = 'c'.repeat(40) +const MERGE_BASE_OID = 'd'.repeat(40) +const FILE_PATH = 'src/file.ts' + +type GitBufferTarget = { + gitBuffer(args: string[], cwd: string): Promise +} + +type GitTarget = { + git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }> +} + +function deferredBuffer(): { promise: Promise; resolve: (content: string) => void } { + let resolve!: (value: Buffer) => void + const promise = new Promise((innerResolve) => { + resolve = innerResolve + }) + return { promise, resolve: (content) => resolve(Buffer.from(content)) } +} + +describe('pinned relay branch diff operation', () => { + it.each([ + { + name: 'addition', + status: `A\t${FILE_PATH}\n`, + left: new Error('missing'), + right: Buffer.from('added\n') + }, + { + name: 'deletion', + status: `D\t${FILE_PATH}\n`, + left: Buffer.from('deleted\n'), + right: new Error('missing') + }, + { + name: 'binary content', + status: `M\t${FILE_PATH}\n`, + left: Buffer.from([0, 1]), + right: Buffer.from([0, 2]) + }, + { + name: 'blob read failure', + status: `M\t${FILE_PATH}\n`, + left: new Error('left failed'), + right: new Error('right failed') + } + ])('matches the legacy $name result', async ({ status, left, right }) => { + const createGitBuffer = (): GitBufferExec => + vi.fn(async (args) => { + const value = args[2].startsWith(`${BASE_OID}:`) ? left : right + if (value instanceof Error) { + throw value + } + return value + }) + const git = vi.fn(async (args) => { + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return { stdout: `${HEAD_OID}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' || args[0] === 'merge-base') { + return { stdout: `${BASE_OID}\n`, stderr: '' } + } + return { stdout: status, stderr: '' } + }) + + const pinned = await branchDiffEntryAtPinnedOids( + createGitBuffer(), + '/repo', + BASE_OID, + HEAD_OID, + FILE_PATH + ) + const legacy = await branchDiffEntries(git, createGitBuffer(), '/repo', BASE_OID, { + includePatch: true, + filePath: FILE_PATH + }) + + expect(pinned).toEqual(legacy) + }) + + // Why: GitBranchCompareSummary.headOid is `string | null`, so an unpinned + // snapshot arrives as an explicit null and must stay servable. + it.each([ + { name: 'absent', params: {} }, + { name: 'null', params: { headOid: null } }, + { name: 'undefined', params: { headOid: undefined } } + ])('treats a $name head OID as unpinned', ({ params }) => { + expect(parseOptionalBranchDiffHeadOid(params)).toBeUndefined() + }) + + it.each([ + { name: 'SHA-1', headOid: HEAD_OID }, + { name: 'SHA-256', headOid: 'b'.repeat(64) } + ])('returns a supplied $name head OID', ({ headOid }) => { + expect(parseOptionalBranchDiffHeadOid({ headOid })).toBe(headOid) + }) + + it.each([{ headOid: 'abc123' }, { headOid: '' }, { headOid: 'main' }, { headOid: 123 }])( + 'rejects the supplied malformed head OID $headOid', + ({ headOid }) => { + expect(() => parseOptionalBranchDiffHeadOid({ headOid })).toThrow( + 'headOid must be a full git object id' + ) + } + ) + + it('recognizes only full object ids', () => { + expect(isFullGitObjectId(BASE_OID)).toBe(true) + expect(isFullGitObjectId('A'.repeat(64))).toBe(true) + for (const value of ['origin/main', 'abc123', '', 'g'.repeat(40), null, undefined, 123, {}]) { + expect(isFullGitObjectId(value)).toBe(false) + } + }) + + // Why: the route guards this today, but the export is reachable on its own — + // a symbolic ref here would resolve `git show origin/main:` at live HEAD. + it('rejects a symbolic ref at the module boundary', async () => { + const gitBuffer = vi.fn(async () => Buffer.from('')) + await expect( + branchDiffEntryAtPinnedOids(gitBuffer, '/repo', 'origin/main', HEAD_OID, FILE_PATH) + ).rejects.toThrow('baseRef must be a full git object id') + await expect( + branchDiffEntryAtPinnedOids(gitBuffer, '/repo', BASE_OID, 'HEAD', FILE_PATH) + ).rejects.toThrow('headOid must be a full git object id') + expect(gitBuffer).not.toHaveBeenCalled() + }) +}) + +describe('GitHandler pinned branch diff route', () => { + let dispatcher: MockDispatcher + let handler: GitHandler + + beforeEach(() => { + dispatcher = createMockDispatcher() + handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + }) + + afterEach(() => { + handler.dispose() + vi.restoreAllMocks() + }) + + function request(overrides: Record = {}): Promise { + return dispatcher.callRequest('git.branchDiff', { + worktreePath: '/repo', + baseRef: BASE_OID, + headOid: HEAD_OID, + includePatch: true, + filePath: FILE_PATH, + ...overrides + }) + } + + function mockLegacyGit(nameStatus = '') { + return vi.spyOn(handler as unknown as GitTarget, 'git').mockImplementation(async (args) => { + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return { stdout: `${HEAD_OID}\n`, stderr: '' } + } + if (args[0] === 'rev-parse') { + return { stdout: `${BASE_OID}\n`, stderr: '' } + } + if (args[0] === 'merge-base') { + return { stdout: `${MERGE_BASE_OID}\n`, stderr: '' } + } + return { stdout: nameStatus, stderr: '' } + }) + } + + it('starts exactly two pinned blob reads concurrently without metadata commands', async () => { + const left = deferredBuffer() + const right = deferredBuffer() + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockImplementation((args) => { + if (args[2] === `${BASE_OID}:${FILE_PATH}`) { + return left.promise + } + if (args[2] === `${HEAD_OID}:${FILE_PATH}`) { + return right.promise + } + throw new Error(`unexpected blob spec: ${args[2]}`) + }) + const gitSpy = vi + .spyOn(handler as unknown as GitTarget, 'git') + .mockRejectedValue(new Error('metadata command should not run')) + + const resultPromise = request() + await vi.waitFor(() => expect(gitBufferSpy).toHaveBeenCalledTimes(2)) + + expect(gitSpy).not.toHaveBeenCalled() + expect(gitBufferSpy.mock.calls.map(([args]) => args)).toEqual([ + ['show', '--end-of-options', `${BASE_OID}:${FILE_PATH}`], + ['show', '--end-of-options', `${HEAD_OID}:${FILE_PATH}`] + ]) + + left.resolve('before\n') + right.resolve('after\n') + await expect(resultPromise).resolves.toEqual([ + { + kind: 'text', + originalContent: 'before\n', + modifiedContent: 'after\n', + originalIsBinary: false, + modifiedIsBinary: false + } + ]) + }) + + it('reads the old path only from the base OID', async () => { + const oldPath = 'src/old-file.ts' + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockImplementation(async (args) => Buffer.from(args[2].startsWith(BASE_OID) ? 'old' : 'new')) + + await request({ oldPath }) + + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${BASE_OID}:${oldPath}`, + `${HEAD_OID}:${FILE_PATH}` + ]) + }) + + it('coalesces the same head OID while keeping different heads independent', async () => { + const pending = deferredBuffer() + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockImplementation(() => pending.promise) + + const requests = [request(), request(), request({ headOid: OTHER_HEAD_OID })] + await vi.waitFor(() => expect(gitBufferSpy).toHaveBeenCalledTimes(4)) + + const specs = gitBufferSpy.mock.calls.map(([args]) => args[2]) + expect(specs.filter((spec) => spec === `${BASE_OID}:${FILE_PATH}`)).toHaveLength(2) + expect(specs.filter((spec) => spec === `${HEAD_OID}:${FILE_PATH}`)).toHaveLength(1) + expect(specs.filter((spec) => spec === `${OTHER_HEAD_OID}:${FILE_PATH}`)).toHaveLength(1) + + pending.resolve('content\n') + await Promise.all(requests) + }) + + it('rejects every supplied malformed head OID before running Git', async () => { + const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + const invalidHeadOids = [7, {}, '', 'a'.repeat(39), 'g'.repeat(40)] + + for (const headOid of invalidHeadOids) { + await expect(request({ headOid })).rejects.toThrow('headOid must be a full git object id') + } + + expect(gitBufferSpy).not.toHaveBeenCalled() + }) + + it('accepts SHA-256 object IDs', async () => { + const baseOid = 'A'.repeat(64) + const headOid = 'b'.repeat(64) + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockResolvedValue(Buffer.from('content\n')) + + await request({ baseRef: baseOid, headOid }) + + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${baseOid}:${FILE_PATH}`, + `${headOid}:${FILE_PATH}` + ]) + }) + + // Why: a symbolic base ref used to select the pinned route and then throw. + it('selects the pinned route only when the base ref is a full object id', async () => { + const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) + const gitBufferSpy = vi + .spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + .mockResolvedValue(Buffer.from('content\n')) + + await expect(request({ baseRef: 'origin/main' })).resolves.toHaveLength(1) + + expect(gitSpy.mock.calls.map(([args]) => args.join(' '))).toEqual([ + 'rev-parse --verify HEAD', + 'rev-parse --verify origin/main', + `merge-base ${BASE_OID} ${HEAD_OID}`, + `-c core.quotePath=false diff --name-status -M -C ${MERGE_BASE_OID} ${HEAD_OID}` + ]) + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${MERGE_BASE_OID}:${FILE_PATH}`, + `${HEAD_OID}:${FILE_PATH}` + ]) + + gitSpy.mockClear() + gitBufferSpy.mockClear() + await request() + + expect(gitSpy).not.toHaveBeenCalled() + expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([ + `${BASE_OID}:${FILE_PATH}`, + `${HEAD_OID}:${FILE_PATH}` + ]) + }) + + it('serves an explicitly null head OID through the legacy path', async () => { + const gitSpy = mockLegacyGit(`M\t${FILE_PATH}\n`) + const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + + await expect(request({ headOid: null, includePatch: false })).resolves.toEqual([ + { + kind: 'text', + originalContent: '', + modifiedContent: '', + originalIsBinary: false, + modifiedIsBinary: false + } + ]) + + expect(gitSpy).toHaveBeenCalledTimes(4) + expect(gitBufferSpy).not.toHaveBeenCalled() + }) + + it('uses the legacy path when patch or file-path conditions are not met', async () => { + const gitSpy = mockLegacyGit() + const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer') + + await Promise.all([ + request({ baseRef: 'main', includePatch: false }), + request({ baseRef: 'main', filePath: '' }), + request({ baseRef: 'main', filePath: { malformed: true } }) + ]) + + expect(gitSpy).toHaveBeenCalledTimes(12) + expect(gitBufferSpy).not.toHaveBeenCalled() + }) + + it('keeps the requested head pinned after repository HEAD moves', async () => { + const repoPath = mkdtempSync(path.join(tmpdir(), 'relay-pinned-branch-diff-')) + try { + gitInit(repoPath) + writeFileSync(path.join(repoPath, 'file.txt'), 'base\n') + gitCommit(repoPath, 'base') + const baseOid = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repoPath, + encoding: 'utf8' + }).trim() + + writeFileSync(path.join(repoPath, 'file.txt'), 'pinned\n') + gitCommit(repoPath, 'pinned') + const pinnedHeadOid = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repoPath, + encoding: 'utf8' + }).trim() + + writeFileSync(path.join(repoPath, 'file.txt'), 'moved\n') + gitCommit(repoPath, 'moved') + + const result = (await request({ + worktreePath: repoPath, + baseRef: baseOid, + headOid: pinnedHeadOid, + filePath: 'file.txt' + })) as { originalContent: string; modifiedContent: string }[] + + expect(result[0]).toMatchObject({ + originalContent: 'base\n', + modifiedContent: 'pinned\n' + }) + expect( + execFileSync('git', ['show', 'HEAD:file.txt'], { cwd: repoPath, encoding: 'utf8' }) + ).toBe('moved\n') + } finally { + rmSync(repoPath, { recursive: true, force: true }) + } + }) +}) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 0abc9f5d5be..3e8a20ea992 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -18,6 +18,11 @@ import { validateGitExecArgs, type GitExec } from './git-handler-ops' +import { + branchDiffEntryAtPinnedOids, + isFullGitObjectId, + parseOptionalBranchDiffHeadOid +} from './git-handler-branch-diff-ops' import { buildSubmoduleInnerCommitRangeDiff, computeSubmodulePointerDiff, @@ -1148,6 +1153,7 @@ export class GitHandler { if (baseRef.startsWith('-')) { throw new Error('Base ref must not start with "-"') } + const headOid = parseOptionalBranchDiffHeadOid(params) const options = { includePatch: params.includePatch as boolean | undefined, filePath: params.filePath as string | undefined, @@ -1158,18 +1164,36 @@ export class GitHandler { 'branchDiff', worktreePath, baseRef, + headOid ?? null, options.includePatch ?? null, options.filePath ?? null, options.oldPath ?? null ]), - () => - branchDiffEntries( + () => { + if ( + headOid && + isFullGitObjectId(baseRef) && + options.includePatch === true && + typeof options.filePath === 'string' && + options.filePath.length > 0 + ) { + return branchDiffEntryAtPinnedOids( + this.gitBuffer.bind(this), + worktreePath, + baseRef, + headOid, + options.filePath, + options.oldPath + ) + } + return branchDiffEntries( this.git.bind(this), this.gitBuffer.bind(this), worktreePath, baseRef, options ) + } ) return this.maybeStreamResponse(result, params, context) } diff --git a/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx b/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx index 3a5ed862d8e..e72ff75858e 100644 --- a/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx +++ b/src/renderer/src/components/editor/useEditorPanelContentState.test.tsx @@ -426,7 +426,7 @@ describe('useEditorPanelContentState', () => { connectionId: 'ssh-1' }), expect.objectContaining({ - compare: expect.objectContaining({ mergeBase: 'merge-base' }), + compare: expect.objectContaining({ headOid: 'head', mergeBase: 'merge-base' }), filePath: 'api/src/file.ts' }) ) diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 660a2b7aaad..c8fe65d53bb 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -226,4 +226,25 @@ describeBinaryCompatibility('real Git binary compatibility', () => { expect(lines[0]).toBe('--end-of-options') expect(lines.find((line) => line !== '--end-of-options')).toMatch(/^refs\//) }) + + // Why pin this: `show --end-of-options :` is the only Git command on the + // pinned SSH branch-diff path, and both blob sides depend on it resolving against the + // named commit rather than live HEAD, and on failing (not falling back) for a path + // absent at that commit — that failure is what renders additions and deletions. + it('reads a blob at a pinned object id', async () => { + await writeFile(join(repoPath, 'pinned.txt'), 'pinned\n') + await runGit(['add', 'pinned.txt']) + await runGit(['commit', '-qm', 'pinned']) + const pinnedOid = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + + await writeFile(join(repoPath, 'pinned.txt'), 'moved on\n') + await runGit(['commit', '-qam', 'after pinned']) + + await expect( + runGit(['show', '--end-of-options', `${pinnedOid}:pinned.txt`]) + ).resolves.toMatchObject({ stdout: 'pinned\n' }) + await expect( + runGit(['show', '--end-of-options', `${pinnedOid}:absent.txt`]) + ).rejects.toBeDefined() + }) }) From 1f2e18acc1c318110e648d66791117fd60c13006 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 11 Aug 2026 03:07:44 -0700 Subject: [PATCH 006/216] fix(e2e): repair seven specs whose assertions drifted from shipped behavior (#13785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(e2e): repair seven specs whose assertions drifted from shipped behavior The E2E suite could not collect at all until #13758, so these seven had been failing unobserved. Each is a stale test, not a product defect — verified individually against src/ rather than by making the assertion pass. - worktree-jump-palette-filter: the palette placeholder gained chats and terminals. Hoisted to one SEARCH_PLACEHOLDER const. - tab-create-entry-file-paths: matched the omnibox by its translated aria-label. Switched to aria-controls, which is structural and unlocalized. - browser-local-https-certificate-trust: once a load settles, the toolbar reload button relabels itself "Retry" alongside the failure overlay's own Retry, so the slot-wide locator hit two elements and failed strict mode. The test only ever passed inside the window where the toolbar still read "Stop". Narrowed by visible text; the icon button has none. - repro-7732-gitlab-checks-job-details: the activity-bar label carries a failure suffix ("Checks — Error"), which an exact-name match stops seeing precisely when the checks under test fail. Anchored regex, and bounded the click so the poll retries instead of hanging on a label that flips mid-action. - floating-tab-rename: the panel's open flag is persisted, so after the restart the helper's blind toggle closed the panel it was about to assert on. Made it idempotent. - github-created-issue-start-prefill: starting an issue now routes through the quick-create composer, so the launch command only forms once that is submitted. The spec now drives it. - ssh-config-host-import: P6 waited on "All hosts already in Orca", a string that exists nowhere in src/ and never could have matched. P7 required the tombstoned host to be absent, but listing it behind a "Removed from Orca" badge is deliberate — see the rationale at ssh-config-host-picker.ts:54 and the unit test already pinning it. Both retargeted; P7 still proves the host is excluded from bulk re-adoption, which is what it was for. Verified locally: all seven pass. github-created-issue-start-prefill cannot be verified on a machine whose gh resolves to an Orca terminal-attribution shim, since hydrateShellPath puts that ahead of the fixture's fake gh; CI has no shim. Three further failures are NOT addressed here. Adversarial review found the obvious test-side fix for each would have masked a real product bug, so they are being fixed in the product instead. * fix(e2e): configure the issue spec's git remote before Electron launches The spec added origin inside the test body, after the orcaPage fixture had already launched the app and added the repo. "New GitHub issue" is disabled when no repo is task-eligible, and eligibility is decided by the git remote probe alone: repos.add runs detectRepoIconAndUpstream, and a settled "no remote" writes gitRemoteIdentity = null, which is the ineligible marker. Background enrichment then suppresses re-probing that location for five minutes, so a remote added afterwards can never recover the button. It passed only when the shared seeded repo happened to already carry an origin from an earlier spec in the same worker — github-cli-stall-repro adds one, source-control-create-pr-intent-switch removes one — which is why it passed in the sharded lane and failed in the changed-specs lane. Moved to test.beforeAll, whose worker-scoped testRepoPath runs before the test-scoped Electron fixtures. Reproduced the failure against a fresh origin-less repo, then confirmed the fix on the same, including --repeat-each=2. Note the earlier attribution was wrong: the local failure at this line was never the gh attribution shim. The button's enabled state reads `git remote -v` and never consults gh, so a shim can only bite later, at issue creation. --- ...wser-local-https-certificate-trust.spec.ts | 10 ++++++-- tests/e2e/floating-tab-rename.spec.ts | 10 +++++++- ...github-created-issue-start-prefill.spec.ts | 25 ++++++++++++++++--- .../helpers/source-control-ai-generation.ts | 8 ++++-- tests/e2e/ssh-config-host-import.spec.ts | 12 ++++++--- tests/e2e/tab-create-entry-file-paths.spec.ts | 6 ++--- .../e2e/worktree-jump-palette-filter.spec.ts | 9 +++---- 7 files changed, 59 insertions(+), 21 deletions(-) diff --git a/tests/e2e/browser-local-https-certificate-trust.spec.ts b/tests/e2e/browser-local-https-certificate-trust.spec.ts index c82ec75f959..39e9cd782f2 100644 --- a/tests/e2e/browser-local-https-certificate-trust.spec.ts +++ b/tests/e2e/browser-local-https-certificate-trust.spec.ts @@ -1,4 +1,4 @@ -import type { Page } from '@stablyai/playwright-test' +import type { Locator, Page } from '@stablyai/playwright-test' import { expect, test } from './helpers/orca-app' import { @@ -64,6 +64,12 @@ function browserSlot(page: Page, pageId: string) { return page.locator(`[data-browser-overlay-tab-id="${pageId}"]`) } +// Why: the toolbar reload button is also named "Retry" once a load fails, so match the +// overlay's button by its visible label — the toolbar one is icon-only. +function failureOverlayRetryButton(slot: Locator): Locator { + return slot.getByRole('button', { name: 'Retry' }).filter({ hasText: 'Retry' }) +} + async function readBrowserHeading(page: Page, browserTabId: string): Promise { return page.evaluate(async (targetBrowserTabId) => { const slot = [...document.querySelectorAll('[data-browser-overlay-tab-id]')].find( @@ -144,7 +150,7 @@ test.describe('local HTTPS certificate trust', () => { // The certificate-failure branch keeps its safe recovery actions and the // certificate-specific hint, but never the local-server connectivity hint. await expect(firstSlot.getByRole('button', { name: 'Copy Address' })).toBeVisible() - await expect(firstSlot.getByRole('button', { name: 'Retry' })).toBeVisible() + await expect(failureOverlayRetryButton(firstSlot)).toBeVisible() await expect(firstSlot.getByText(/use a trusted local certificate/i)).toBeVisible() await expect(firstSlot.getByText(/make sure the server is running/i)).toHaveCount(0) await firstSlot.getByRole('button', { name: 'Proceed Anyway (Unsafe)' }).click() diff --git a/tests/e2e/floating-tab-rename.spec.ts b/tests/e2e/floating-tab-rename.spec.ts index 7f9dca4b7b9..94efe4650ee 100644 --- a/tests/e2e/floating-tab-rename.spec.ts +++ b/tests/e2e/floating-tab-rename.spec.ts @@ -80,7 +80,15 @@ async function openFloatingPanel(page: Page): Promise { PANEL_SELECTOR, { timeout: 30_000 } ) - await page.evaluate(() => window.dispatchEvent(new Event('orca-toggle-floating-terminal'))) + // Why: the panel's open flag is persisted (floating-terminal-panel-view-state), so + // after a restart it reopens on its own and a blind toggle would close it again. + const alreadyOpen = await page.evaluate( + (selector) => Boolean(document.querySelector(selector)), + OPEN_PANEL_SELECTOR + ) + if (!alreadyOpen) { + await page.evaluate(() => window.dispatchEvent(new Event('orca-toggle-floating-terminal'))) + } await expect(page.locator(OPEN_PANEL_SELECTOR)).toBeVisible() } diff --git a/tests/e2e/github-created-issue-start-prefill.spec.ts b/tests/e2e/github-created-issue-start-prefill.spec.ts index c3e6d40820a..50d08acc0dd 100644 --- a/tests/e2e/github-created-issue-start-prefill.spec.ts +++ b/tests/e2e/github-created-issue-start-prefill.spec.ts @@ -119,11 +119,17 @@ function configureGitHubRemote(repoPath: string): void { }) } -test('starting a just-created GitHub issue launches Claude with its URL prefilled', async ({ - orcaPage, - testRepoPath -}) => { +// Why: the remote must exist before Electron launches. `repos.add` probes the +// remote once, and a settled "no remote" both makes the repo ineligible for the +// tasks page (disabling "New GitHub issue") and suppresses re-probes for five +// minutes — so adding origin inside the test body can never recover. +test.beforeAll(({ testRepoPath }) => { configureGitHubRemote(testRepoPath) +}) + +test('starting a just-created GitHub issue launches Claude with its URL prefilled', async ({ + orcaPage +}) => { await waitForSessionReady(orcaPage) await waitForActiveWorktree(orcaPage) @@ -167,6 +173,17 @@ test('starting a just-created GitHub issue launches Claude with its URL prefille await orcaPage.getByRole('button', { name: 'Start workspace from issue' }).click() + // Why: starting a GitHub issue now routes through the quick-create composer, + // which carries the linked issue URL into the agent launch command on submit. + const composer = orcaPage.getByRole('dialog', { name: /Create (workspace|worktree)/i }) + await expect(composer).toBeVisible({ timeout: 15_000 }) + const createWorkspaceButton = composer.getByRole('button', { + name: /Create (workspace|worktree)/i + }) + await expect(createWorkspaceButton).toBeEnabled({ timeout: 15_000 }) + await createWorkspaceButton.click() + await expect(composer).toBeHidden({ timeout: 20_000 }) + let terminalText = '' await expect .poll( diff --git a/tests/e2e/helpers/source-control-ai-generation.ts b/tests/e2e/helpers/source-control-ai-generation.ts index 149f6a019d3..f6be16d7342 100644 --- a/tests/e2e/helpers/source-control-ai-generation.ts +++ b/tests/e2e/helpers/source-control-ai-generation.ts @@ -42,12 +42,16 @@ export async function openChecks(page: Page, worktreeId: string): Promise { timeout: 5_000 } ) .toBe(true) - const checksButton = page.getByRole('button', { name: 'Checks', exact: true }) + // Why: the activity-bar label carries an optional shortcut and a failure suffix ("Checks — Error"), + // so an exact 'Checks' name silently stops matching once the active branch has failing checks. + const checksButton = page.getByRole('button', { name: /^Checks(\s|$)/ }) await expect .poll( async () => { if ((await page.evaluate(() => window.__store?.getState().rightSidebarTab)) !== 'checks') { - await checksButton.click() + // Why: the label flips as the checks status arrives; bound the click so the poll retries + // instead of hanging on a locator that stopped matching mid-action. + await checksButton.click({ timeout: 2_000 }).catch(() => undefined) } await page.waitForTimeout(250) return page.evaluate(() => window.__store?.getState().rightSidebarTab) diff --git a/tests/e2e/ssh-config-host-import.spec.ts b/tests/e2e/ssh-config-host-import.spec.ts index 959d180ddde..4fba64f6adf 100644 --- a/tests/e2e/ssh-config-host-import.spec.ts +++ b/tests/e2e/ssh-config-host-import.spec.ts @@ -148,7 +148,7 @@ test.describe('SSH config host import (bulk + settings re-adopt)', () => { await expect( configHostRow(reopened, hosts.bravo).getByText('In Orca', { exact: true }) ).toBeVisible() - await expect(reopened.getByRole('button', { name: 'All hosts already in Orca' })).toBeDisabled() + await expect(reopened.getByRole('button', { name: 'No new hosts to add' })).toBeDisabled() }) // ── P7 ───────────────────────────────────────────────────────────── @@ -164,13 +164,17 @@ test.describe('SSH config host import (bulk + settings re-adopt)', () => { ) const picker = await openSshConfigHostPicker(orcaPage) - // Suppressed aliases are omitted from the picker entirely. - await expect(configHostRow(picker, hosts.alpha)).toHaveCount(0) + // Suppressed aliases stay listed (re-pickable) but never count as new. + const alphaRow = configHostRow(picker, hosts.alpha) + await expect(alphaRow).toBeVisible() + await expect(alphaRow).toBeEnabled() + await expect(alphaRow.getByText('Removed from Orca', { exact: true })).toBeVisible() + await expect(alphaRow.getByText('In Orca', { exact: true })).toHaveCount(0) await expect(configHostRow(picker, hosts.bravo)).toBeVisible() await expect( configHostRow(picker, hosts.bravo).getByText('In Orca', { exact: true }) ).toBeVisible() - await expect(picker.getByRole('button', { name: 'All hosts already in Orca' })).toBeDisabled() + await expect(picker.getByRole('button', { name: 'No new hosts to add' })).toBeDisabled() await expect(picker.getByRole('button', { name: /Add all \d+ to Orca/ })).toHaveCount(0) await returnToAppShell(orcaPage) diff --git a/tests/e2e/tab-create-entry-file-paths.spec.ts b/tests/e2e/tab-create-entry-file-paths.spec.ts index bde87a1f4ae..5f18b8bd0e1 100644 --- a/tests/e2e/tab-create-entry-file-paths.spec.ts +++ b/tests/e2e/tab-create-entry-file-paths.spec.ts @@ -19,9 +19,9 @@ test('new-tab file results prioritize the filename and reveal the full path on h await ensureTerminalVisible(orcaPage) await orcaPage.getByRole('button', { name: 'New tab' }).click({ force: true }) - const input = orcaPage.getByRole('combobox', { - name: 'Open any file, URL, agent, ...' - }) + // Not the placeholder/aria-label: that copy is translated and already drifted + // once. aria-controls points at the results listbox id, which is structural. + const input = orcaPage.locator('input[role="combobox"][aria-controls="tab-create-entry-results"]') await input.fill('secondaryNav') const row = orcaPage.locator('[role="option"]').filter({ hasText: 'Open file' }).first() diff --git a/tests/e2e/worktree-jump-palette-filter.spec.ts b/tests/e2e/worktree-jump-palette-filter.spec.ts index 02a7e08f8ac..90b2c7b5137 100644 --- a/tests/e2e/worktree-jump-palette-filter.spec.ts +++ b/tests/e2e/worktree-jump-palette-filter.spec.ts @@ -6,6 +6,7 @@ const LOCAL_PROJECT = 'E2E Palette Local Project' const REMOTE_PROJECT = 'E2E Palette Remote Project' const REMOTE_WORKSPACE = 'E2E Palette Remote Workspace' const REMOTE_HOST = 'E2E Palette Builder' +const SEARCH_PLACEHOLDER = 'Search chats, terminals, worktrees, settings, and actions...' type PaletteFilterFixture = { localWorktreeId: string; remoteWorktreeId: string } @@ -107,7 +108,7 @@ async function openPalette(page: Page): Promise { } async function searchFixtureWorkspaces(page: Page, fixture: PaletteFilterFixture): Promise { - const input = palette(page).getByPlaceholder('Search worktrees, settings, tabs, and actions...') + const input = palette(page).getByPlaceholder(SEARCH_PLACEHOLDER) await input.fill('E2E Palette') await expect(worktreeRow(page, fixture.localWorktreeId)).toBeVisible() await expect(worktreeRow(page, fixture.remoteWorktreeId)).toBeVisible() @@ -115,7 +116,7 @@ async function searchFixtureWorkspaces(page: Page, fixture: PaletteFilterFixture async function selectRemoteHost(page: Page, useKeyboard = false): Promise { if (useKeyboard) { - const input = palette(page).getByPlaceholder('Search worktrees, settings, tabs, and actions...') + const input = palette(page).getByPlaceholder(SEARCH_PLACEHOLDER) await input.press('Tab') await expect(filterTrigger(page)).toBeFocused() await filterTrigger(page).click() @@ -152,9 +153,7 @@ test.describe('Worktree jump-palette filters', () => { await expect(worktreeRow(orcaPage, fixture.localWorktreeId)).toHaveCount(0) // P2: host and project fields intersect, with the filter-specific empty state. - await palette(orcaPage) - .getByPlaceholder('Search worktrees, settings, tabs, and actions...') - .fill('') + await palette(orcaPage).getByPlaceholder(SEARCH_PLACEHOLDER).fill('') await filterTrigger(orcaPage).click() await palette(orcaPage).getByText('Projects', { exact: true }).click() const projects = palette(orcaPage).getByRole('listbox', { name: 'Projects' }) From a0a654c3a9793970aeb77e6615a9650ba7b5d231 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Tue, 11 Aug 2026 03:18:55 -0700 Subject: [PATCH 007/216] fix(renderer): reject malformed cursor and language-pack inputs (#13451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What this changes Two renderer inputs were trusted because their TypeScript types said they were valid. Both are now validated at the boundary that owns them. **Terminal cursor style.** `normalizeTerminalCursorStyleDefault` preserved any non-null migration-stamped value without checking it against the actual enum, so a runtime value outside `bar | block | underline` survived into `terminal.options.cursorStyle`. It now enum-checks and falls back to `block`. Applied on both read paths (desktop `Store` load, web `getStoredSettings`) and both write paths (`Store.updateSettings`, web `settings.set`), so an unsupported value cannot reach persistence, the `settings:changed` publication, or xterm. **Plugin language packs.** The renderer stored the `listLanguagePacks()` IPC result without a runtime check, so a non-array response was assigned to state and later crashed its first array consumer on `.find`. Ingress now accepts only an array, drops members failing `isPluginLanguagePackRegistration`, keeps valid siblings in their original order, and logs which failure mode occurred. The registration guard requires `resourceLanguage` to equal `pluginLanguageResourceId(id)`, which keeps a pack with a missing or inconsistent identity out of i18next — that shape reproduces as `TypeError: Cannot read properties of undefined (reading 'includes')` with the guard removed. Catalog validation is shape-only on this path (`validatePluginLanguagePackCatalogShape`), so revalidating an already-parsed catalog does not allocate a second copy of it. `isCatalogObject` also now requires a plain-object prototype, and the walk rejects repeated or cyclic object references. ## Root cause: partially known Worth stating plainly, because the fix is defensive rather than causal: - The non-array pack container and the out-of-enum cursor value are both reproduced directly by tests. - Two additional field stacks are *consistent with* an invalid `resourceLanguage` and are blocked by this guard, but the reports do not prove that malformed registrations were their source. - No production writer in current code or history emits an out-of-enum cursor style. The Ghostty config importer (`src/main/ghostty/mapper.ts`) already rejects unsupported `cursor-style` values. The original writer is unidentified. Every in-tree producer supplies valid data, so these guards are no-ops on the current happy path. They are boundary hardening against a mutation we have not located, not a repair of a known writer. ## Trade-offs - Malformed registrations are skipped with one warning rather than surfaced in the UI. Valid siblings keep their identity and order. Note the main-process registry already reports per-plugin parse errors, so this path only catches corruption after main has validated. - Renderer ingress walks each catalog once per lazy load or `contentPacksChanged`. It does not run on render or terminal-output paths, but a maximum-size burst still costs tens of milliseconds synchronously. Pack count and plugin-ID length remain uncapped. - An unsupported cursor value now renders and persists as `block`, so anyone relying on an undocumented third-party value loses it. - Loading settings whose cursor style is absent or invalid now marks state dirty and rewrites once, matching the existing `terminalRightClickToPasteDefaultedForPlatform` pattern above it. ## Compatibility `listLanguagePacks` is local `ipcMain`/`ipcRenderer` only and is not implemented in the web build, so nothing here crosses the remote wire — no RPC parameter, publication schema, stream frame, opcode, or capability changed. Validation is receiver-side at existing boundaries. Nothing is platform-, shell-, PTY-, native-module-, SSH-, WSL-, or worktree-dependent, and the web build normalizes on both read and write. Malformed plugin data is rejected before i18next sees it; no new permission, network path, executable input, or persistence schema was added. ## Verification Locally on the final head: the 5 touched test files pass (591 tests), plus node and web typecheck, oxlint, and oxfmt. All GitHub required checks pass, including Windows packaging, static analysis, Git and wire compatibility, shell contracts, and all 32 Node 24/26 shards. The path-gated E2E job is skipped after its detector passed. Scope note: an earlier revision generalized this hardening to `Project.sourceRepoIds` and profile transfer without field evidence. That scope was removed; the diff is cursor and plugin paths only. --- src/main/persistence.test.ts | 52 +++++ src/main/persistence.ts | 16 ++ .../src/store/plugin-language-packs.test.ts | 203 ++++++++++++++++++ .../src/store/plugin-language-packs.ts | 16 +- src/renderer/src/web/web-preload-api.test.ts | 20 ++ src/renderer/src/web/web-preload-api.ts | 9 + .../plugin-language-pack-artifact.test.ts | 48 +++++ .../plugins/plugin-language-pack-artifact.ts | 91 +++++++- src/shared/terminal-cursor-style-settings.ts | 9 +- 9 files changed, 451 insertions(+), 13 deletions(-) create mode 100644 src/renderer/src/store/plugin-language-packs.test.ts diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 0e507090c1f..2255e3a3b8e 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -5621,6 +5621,42 @@ describe('Store', () => { expect(persisted.settings).not.toHaveProperty('terminalScrollbackBytes') }) + it('normalizes terminal cursor style before persistence and listener broadcasts', async () => { + const store = await createStore() + store.updateSettings({ terminalCursorStyle: 'underline' }) + const listener = vi.fn() + store.onSettingsChanged(listener) + + const invalid = store.updateSettings( + { terminalCursorStyle: 'beam' as never }, + { notifyListeners: true } + ) + + expect(invalid.terminalCursorStyle).toBe('block') + expect(invalid.terminalCursorStyleDefaultedToBlock).toBe(true) + expect(listener).toHaveBeenCalledWith( + { + terminalCursorStyle: 'block' + }, + expect.objectContaining({ terminalCursorStyle: 'block' }), + undefined + ) + + const valid = store.updateSettings( + { terminalCursorStyle: 'underline' }, + { notifyListeners: true } + ) + expect(valid.terminalCursorStyle).toBe('underline') + expect(listener).toHaveBeenLastCalledWith( + { terminalCursorStyle: 'underline' }, + expect.objectContaining({ terminalCursorStyle: 'underline' }), + undefined + ) + + store.flush() + expect((readDataFile() as PersistedState).settings.terminalCursorStyle).toBe('underline') + }) + it('normalizes disabled TUI agents on load and update', async () => { writeFileSync( join(testState.dir, 'orca-data.json'), @@ -7295,6 +7331,22 @@ describe('Store', () => { expect(store.getSettings().terminalCursorStyleDefaultedToBlock).toBe(true) }) + it('replaces an invalid persisted terminal cursor choice after migration', async () => { + writeDataFile({ + schemaVersion: 1, + repos: [], + worktreeMeta: {}, + settings: { terminalCursorStyle: 'beam', terminalCursorStyleDefaultedToBlock: true }, + ui: {}, + githubCache: { pr: {}, issue: {} }, + workspaceSession: {} + }) + const store = await createStore() + expect(store.getSettings().terminalCursorStyle).toBe('block') + store.flush() + expect((readDataFile() as PersistedState).settings.terminalCursorStyle).toBe('block') + }) + it('preserves explicit "false" terminalMacOptionAsAlt through migration', async () => { // 'false' never matched the old default — it was an explicit choice. writeDataFile({ diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 9739d18bbfc..c15cab49aad 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -3244,6 +3244,13 @@ export class Store { parsed.settings ) const migratedTerminalCursorStyle = normalizeTerminalCursorStyleDefault(parsed.settings) + if ( + parsed.settings?.terminalCursorStyle !== + migratedTerminalCursorStyle.terminalCursorStyle || + parsed.settings?.terminalCursorStyleDefaultedToBlock !== true + ) { + this.loadNeedsSave = true + } const migratedTerminalLineHeight = normalizeTerminalLineHeight( parsed.settings?.terminalLineHeight ) @@ -5841,6 +5848,15 @@ export class Store { updates.terminalCustomThemes ) } + if ('terminalCursorStyle' in updates) { + Object.assign( + sanitizedUpdates, + normalizeTerminalCursorStyleDefault( + { terminalCursorStyle: updates.terminalCursorStyle }, + { preserveExplicitValue: true } + ) + ) + } if ('terminalScrollbackRows' in updates) { sanitizedUpdates.terminalScrollbackRows = normalizeDesktopTerminalScrollbackRows( updates.terminalScrollbackRows diff --git a/src/renderer/src/store/plugin-language-packs.test.ts b/src/renderer/src/store/plugin-language-packs.test.ts new file mode 100644 index 00000000000..53a608d89f4 --- /dev/null +++ b/src/renderer/src/store/plugin-language-packs.test.ts @@ -0,0 +1,203 @@ +// @vitest-environment happy-dom + +import { afterEach, describe, expect, it, vi } from 'vitest' +import { i18n, setRendererPluginLanguagePacks } from '../i18n/i18n' +import { pluginLanguageResourceId } from '../../../shared/plugins/plugin-language-pack-artifact' +import { usePluginLanguagePackStore } from './plugin-language-packs' + +afterEach(() => { + vi.restoreAllMocks() + setRendererPluginLanguagePacks([]) + usePluginLanguagePackStore.setState({ packs: [], loaded: false }) + vi.unstubAllGlobals() +}) + +describe('plugin language pack loading', () => { + it('rejects malformed runtime IPC language-pack results at the renderer ingress', async () => { + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi.fn().mockResolvedValue({ malformed: true }) + } + } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + + const state = usePluginLanguagePackStore.getState() + expect(state.loaded).toBe(true) + expect(state.packs).toEqual([]) + expect(() => state.packs.find(() => true)).not.toThrow() + }) + + it('filters malformed language-pack array members without changing valid order', async () => { + const first = { + id: 'plugin:first', + resourceLanguage: pluginLanguageResourceId('plugin:first'), + pluginKey: 'first', + locale: 'pt-BR', + catalog: { greeting: 'Olá' } + } + const second = { + id: 'plugin:second', + resourceLanguage: pluginLanguageResourceId('plugin:second'), + pluginKey: 'second', + locale: 'de', + catalog: {} + } + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi + .fn() + .mockResolvedValue([null, first, { ...first, catalog: [] }, second]) + } + } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + + expect(usePluginLanguagePackStore.getState().packs).toEqual([first, second]) + }) + + it.each([ + ['Date', new Date(0)], + ['Map', new Map([['greeting', 'Hello']])] + ])('rejects a %s language-pack catalog at renderer ingress', async (_name, catalog) => { + const id = 'plugin:malformed' as const + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi.fn().mockResolvedValue([ + { + id, + resourceLanguage: pluginLanguageResourceId(id), + pluginKey: 'malformed', + locale: 'en', + catalog + } + ]) + } + } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + + expect(usePluginLanguagePackStore.getState().packs).toEqual([]) + }) + + it('rejects a structured-cloned cyclic catalog before i18next resource installation', async () => { + const id = 'plugin:cyclic' as const + const catalog: Record = { greeting: 'Hello' } + catalog.self = catalog + const response = structuredClone([ + { + id, + resourceLanguage: pluginLanguageResourceId(id), + pluginKey: 'cyclic', + locale: 'en', + catalog + } + ]) + vi.stubGlobal('window', { + api: { plugins: { listLanguagePacks: vi.fn().mockResolvedValue(response) } } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + const packs = usePluginLanguagePackStore.getState().packs + + expect(() => setRendererPluginLanguagePacks(packs)).not.toThrow() + expect(packs).toEqual([]) + }) + + it('rejects an inconsistent resource language before i18next owns its cleanup', async () => { + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi.fn().mockResolvedValue([ + { + id: 'plugin:bad', + resourceLanguage: 'plugin.bad', + pluginKey: 'bad', + locale: 'en', + catalog: {} + } + ]) + } + } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + setRendererPluginLanguagePacks(usePluginLanguagePackStore.getState().packs) + const removeResourceBundle = vi + .spyOn(i18n, 'removeResourceBundle') + .mockImplementation((language) => { + throw new Error(`invalid resource language: ${language}`) + }) + + expect(() => setRendererPluginLanguagePacks([])).not.toThrow() + expect(removeResourceBundle).not.toHaveBeenCalled() + }) + + it('rejects a missing resource language before i18next resource installation', async () => { + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi.fn().mockResolvedValue([ + { + id: 'plugin:missing-language', + resourceLanguage: undefined, + pluginKey: 'missing-language', + locale: 'en', + catalog: {} + } + ]) + } + } + }) + + await usePluginLanguagePackStore.getState().fetchPacks() + const packs = usePluginLanguagePackStore.getState().packs + + expect(() => setRendererPluginLanguagePacks(packs)).not.toThrow() + expect(packs).toEqual([]) + }) + + it('keeps the latest request when an older language-pack response finishes last', async () => { + const older = { + id: 'plugin:older', + resourceLanguage: pluginLanguageResourceId('plugin:older'), + pluginKey: 'older', + locale: 'de', + catalog: {} + } + const latest = { + id: 'plugin:latest', + resourceLanguage: pluginLanguageResourceId('plugin:latest'), + pluginKey: 'latest', + locale: 'fr', + catalog: {} + } + let resolveOlder!: (value: unknown[]) => void + const olderResponse = new Promise((resolve) => { + resolveOlder = resolve + }) + vi.stubGlobal('window', { + api: { + plugins: { + listLanguagePacks: vi + .fn() + .mockReturnValueOnce(olderResponse) + .mockResolvedValueOnce([latest]) + } + } + }) + + const olderFetch = usePluginLanguagePackStore.getState().fetchPacks() + await usePluginLanguagePackStore.getState().fetchPacks() + resolveOlder([older]) + await olderFetch + + expect(usePluginLanguagePackStore.getState().packs).toEqual([latest]) + }) +}) diff --git a/src/renderer/src/store/plugin-language-packs.ts b/src/renderer/src/store/plugin-language-packs.ts index a237f80f6fc..7c37c53c9c8 100644 --- a/src/renderer/src/store/plugin-language-packs.ts +++ b/src/renderer/src/store/plugin-language-packs.ts @@ -1,6 +1,9 @@ import { useEffect } from 'react' import { create } from 'zustand' -import type { PluginLanguagePackRegistration } from '../../../shared/plugins/plugin-language-pack-artifact' +import { + isPluginLanguagePackRegistration, + type PluginLanguagePackRegistration +} from '../../../shared/plugins/plugin-language-pack-artifact' type PluginLanguagePackState = { packs: PluginLanguagePackRegistration[] @@ -24,7 +27,16 @@ export const usePluginLanguagePackStore = create()((set return } try { - const packs = await api.listLanguagePacks() + const response = await api.listLanguagePacks() + const packs = Array.isArray(response) ? response.filter(isPluginLanguagePackRegistration) : [] + // Why: a non-array response and a rejected member are different upstream bugs; keep them distinguishable in the log. + if (!Array.isArray(response)) { + console.warn(`[plugins] Ignoring non-array language-pack list (${typeof response})`) + } else if (packs.length !== response.length) { + console.warn( + `[plugins] Ignoring ${response.length - packs.length} of ${response.length} malformed language packs` + ) + } if (generation === requestGeneration) { set({ packs, loaded: true }) } diff --git a/src/renderer/src/web/web-preload-api.test.ts b/src/renderer/src/web/web-preload-api.test.ts index 0b85d009839..9fcec640925 100644 --- a/src/renderer/src/web/web-preload-api.test.ts +++ b/src/renderer/src/web/web-preload-api.test.ts @@ -935,6 +935,26 @@ describe('web settings preload API', () => { expect(settings.terminalCursorStyleDefaultedToBlock).toBe(true) }) + it('normalizes terminal cursor style before web settings writes return or persist', async () => { + const { api, storage } = await installApi('Linux') + + const invalid = await api.settings.set({ terminalCursorStyle: 'beam' as never }) + const invalidStored = JSON.parse(storage.getItem('orca.web.settings.v1') ?? '{}') as { + terminalCursorStyle?: string + terminalCursorStyleDefaultedToBlock?: boolean + } + expect(invalid.terminalCursorStyle).toBe('block') + expect(invalid.terminalCursorStyleDefaultedToBlock).toBe(true) + expect(invalidStored.terminalCursorStyle).toBe('block') + expect(invalidStored.terminalCursorStyleDefaultedToBlock).toBe(true) + + const valid = await api.settings.set({ terminalCursorStyle: 'bar' }) + expect(valid.terminalCursorStyle).toBe('bar') + expect(JSON.parse(storage.getItem('orca.web.settings.v1') ?? '{}').terminalCursorStyle).toBe( + 'bar' + ) + }) + it('migrates OSC 52 clipboard writes on for stored web settings once', async () => { // Why: the web store is a second, independent settings store — the constants-level // default flip only reaches profiles that never persisted the old `false` (#10567). diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index eb9fa2e7dbb..8917c32a453 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -705,6 +705,15 @@ function createWebPreloadApi(): Partial { if ('autoRenameBranchFromWorkDefaultedOn' in sanitizedUpdates) { sanitizedUpdates.autoRenameBranchFromWorkDefaultedOn = true } + if ('terminalCursorStyle' in sanitizedUpdates) { + Object.assign( + sanitizedUpdates, + normalizeTerminalCursorStyleDefault( + { terminalCursorStyle: sanitizedUpdates.terminalCursorStyle }, + { preserveExplicitValue: true } + ) + ) + } const next = mergeSettings(getStoredSettings(), sanitizedUpdates, { preserveAutoRenameBranchFromWorkUpdate: 'autoRenameBranchFromWork' in sanitizedUpdates }) diff --git a/src/shared/plugins/plugin-language-pack-artifact.test.ts b/src/shared/plugins/plugin-language-pack-artifact.test.ts index e684bb50095..a313cc33839 100644 --- a/src/shared/plugins/plugin-language-pack-artifact.test.ts +++ b/src/shared/plugins/plugin-language-pack-artifact.test.ts @@ -1,7 +1,11 @@ import { describe, expect, it } from 'vitest' import { + isPluginLanguagePackRegistration, parsePluginLanguagePackArtifact, PLUGIN_LANGUAGE_CATALOG_MAX_DEPTH, + PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES, + validatePluginLanguagePackCatalog, + validatePluginLanguagePackCatalogShape, pluginLanguageResourceId } from './plugin-language-pack-artifact' @@ -163,4 +167,48 @@ describe('plugin language-pack artifacts', () => { error: expect.stringContaining('depth') }) }) + + it('accepts the entry limit and rejects one additional entry', () => { + const catalog = Object.fromEntries( + Array.from({ length: PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES }, (_, index) => [ + `key${index}`, + 'value' + ]) + ) + + expect(validatePluginLanguagePackCatalog(catalog)).toMatchObject({ + ok: true, + entries: PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES + }) + catalog.overflow = 'value' + expect(validatePluginLanguagePackCatalog(catalog)).toMatchObject({ + ok: false, + error: expect.stringContaining(`${PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES} entries`) + }) + }) + + it('shape-validates 16 maximum-entry registrations without returning catalog copies', () => { + const catalog = Object.fromEntries( + Array.from({ length: PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES }, (_, index) => [ + `key${index}`, + 'value' + ]) + ) + const packs = Array.from({ length: 16 }, (_, index) => { + const id = `plugin:maximum-${index}` as const + return { + id, + resourceLanguage: pluginLanguageResourceId(id), + pluginKey: `maximum-${index}`, + locale: 'en', + catalog + } + }) + + expect(validatePluginLanguagePackCatalogShape(catalog)).toEqual({ + ok: true, + entries: PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES + }) + expect(packs.filter(isPluginLanguagePackRegistration)).toHaveLength(16) + }) }) diff --git a/src/shared/plugins/plugin-language-pack-artifact.ts b/src/shared/plugins/plugin-language-pack-artifact.ts index 917adeef69e..7bcfacf47d4 100644 --- a/src/shared/plugins/plugin-language-pack-artifact.ts +++ b/src/shared/plugins/plugin-language-pack-artifact.ts @@ -23,6 +23,24 @@ export type PluginLanguagePackRegistration = { catalog: Record } +export function isPluginLanguagePackRegistration( + value: unknown +): value is PluginLanguagePackRegistration { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return false + } + const pack = value as Record + return ( + typeof pack.id === 'string' && + pack.id.startsWith('plugin:') && + typeof pack.resourceLanguage === 'string' && + pack.resourceLanguage === pluginLanguageResourceId(pack.id as `plugin:${string}`) && + typeof pack.pluginKey === 'string' && + typeof pack.locale === 'string' && + validatePluginLanguagePackCatalogShape(pack.catalog).ok + ) +} + export function pluginLanguageResourceId(id: `plugin:${string}`): `plugin${string}` { let encoded = '' for (let index = 0; index < id.length; index += 1) { @@ -37,15 +55,28 @@ export type PluginLanguagePackParseResult = | { ok: true; catalog: Record; entries: number } | { ok: false; error: string } +export type PluginLanguagePackValidationResult = + | { ok: true; entries: number } + | { ok: false; error: string } + +type PluginLanguagePackWalkResult = + | { ok: true; catalog: Record | null; entries: number } + | { ok: false; error: string } + type CatalogFrame = { source: Record - target: Record + target: Record | null path: string depth: number } function isCatalogObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value) + return ( + typeof value === 'object' && + value !== null && + !Array.isArray(value) && + Object.getPrototypeOf(value) === Object.prototype + ) } function protectedTranslation(path: string): boolean { @@ -59,7 +90,15 @@ function protectedTranslation(path: string): boolean { } function hasUnsafeCatalogKeyCharacter(key: string): boolean { - return key.includes('.') || Array.from(key).some((character) => character.charCodeAt(0) <= 31) + if (key.includes('.')) { + return true + } + for (let index = 0; index < key.length; index += 1) { + if (key.charCodeAt(index) <= 31) { + return true + } + } + return false } export function parsePluginLanguagePackArtifact(raw: string): PluginLanguagePackParseResult { @@ -69,19 +108,43 @@ export function parsePluginLanguagePackArtifact(raw: string): PluginLanguagePack } catch { return { ok: false, error: 'language pack must contain one JSON object' } } - if (!isCatalogObject(json)) { + return validatePluginLanguagePackCatalog(json) +} + +export function validatePluginLanguagePackCatalog(source: unknown): PluginLanguagePackParseResult { + const result = walkPluginLanguagePackCatalog(source, true) + if (!result.ok) { + return result + } + return { ok: true, catalog: result.catalog!, entries: result.entries } +} + +export function validatePluginLanguagePackCatalogShape( + source: unknown +): PluginLanguagePackValidationResult { + const result = walkPluginLanguagePackCatalog(source, false) + return result.ok ? { ok: true, entries: result.entries } : result +} + +function walkPluginLanguagePackCatalog( + source: unknown, + copyCatalog: boolean +): PluginLanguagePackWalkResult { + if (!isCatalogObject(source)) { return { ok: false, error: 'language pack root must be an object' } } - const catalog: Record = {} - const stack: CatalogFrame[] = [{ source: json, target: catalog, path: '', depth: 0 }] + const catalog: Record | null = copyCatalog ? {} : null + const stack: CatalogFrame[] = [{ source, target: catalog, path: '', depth: 0 }] + const seen = new WeakSet([source]) let entries = 0 while (stack.length > 0) { const frame = stack.pop()! if (frame.depth > PLUGIN_LANGUAGE_CATALOG_MAX_DEPTH) { return { ok: false, error: `catalog exceeds depth ${PLUGIN_LANGUAGE_CATALOG_MAX_DEPTH}` } } - for (const [key, value] of Object.entries(frame.source)) { + for (const key of Object.keys(frame.source)) { + const value = frame.source[key] entries += 1 if (entries > PLUGIN_LANGUAGE_CATALOG_MAX_ENTRIES) { return { @@ -110,14 +173,22 @@ export function parsePluginLanguagePackArtifact(raw: string): PluginLanguagePack if (value.length > 8192) { return { ok: false, error: `translation at ${path} exceeds 8192 characters` } } - frame.target[key] = value + if (frame.target) { + frame.target[key] = value + } continue } if (!isCatalogObject(value)) { return { ok: false, error: `translation at ${path} must be a string or object` } } - const child: Record = {} - frame.target[key] = child + if (seen.has(value)) { + return { ok: false, error: `catalog contains a repeated or cyclic object at ${path}` } + } + seen.add(value) + const child: Record | null = frame.target ? {} : null + if (frame.target && child) { + frame.target[key] = child + } stack.push({ source: value, target: child, path, depth: frame.depth + 1 }) } } diff --git a/src/shared/terminal-cursor-style-settings.ts b/src/shared/terminal-cursor-style-settings.ts index 0de07a08e31..ec271a7b972 100644 --- a/src/shared/terminal-cursor-style-settings.ts +++ b/src/shared/terminal-cursor-style-settings.ts @@ -5,6 +5,10 @@ type TerminalCursorStyleSettings = Pick< 'terminalCursorStyle' | 'terminalCursorStyleDefaultedToBlock' > +function isTerminalCursorStyle(value: unknown): value is GlobalSettings['terminalCursorStyle'] { + return value === 'bar' || value === 'block' || value === 'underline' +} + export function normalizeTerminalCursorStyleDefault( settings: Partial | undefined, options: { preserveExplicitValue?: boolean } = {} @@ -15,7 +19,10 @@ export function normalizeTerminalCursorStyleDefault( return { // Why: prior builds persisted the old bar default into profiles; migrate // those inherited values once while preserving later explicit choices. - terminalCursorStyle: defaultedToBlock ? (settings?.terminalCursorStyle ?? 'block') : 'block', + terminalCursorStyle: + defaultedToBlock && isTerminalCursorStyle(settings?.terminalCursorStyle) + ? settings.terminalCursorStyle + : 'block', terminalCursorStyleDefaultedToBlock: true } } From ef56ca8b81b785d75e2989cbc13264080383b9f7 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 11 Aug 2026 11:49:44 -0700 Subject: [PATCH 008/216] fix(feedback): pre-include Orca version and OS in errors and feedback (#13851) * fix(feedback): pre-include Orca version and OS in errors and feedback Make terminal errors and Send Feedback carry easy-to-copy client environment details so bug reports include build and platform context without a follow-up ask. * fix(feedback): satisfy exhaustive-deps in environment prefill hook Destructure hook params so useEffect/useLayoutEffect dependency lists are complete and the changed-code quality gate passes. * fix(preload): stop importing node:os in sandboxed preload Sandboxed Electron preloads cannot require node:os. That import crashed the whole preload script, leaving window.api undefined and taking down App chrome (dock badge, preflight). Keep platform.get on process APIs only, and guard best-effort badge/preflight callers when api is missing. * fix(feedback): count text typed below the env footer Strip only the prefilled Orca/OS/Shell block for Send validation so users who click past the footer and type can still submit. --- src/main/daemon/node-pty-error-hints.test.ts | 2 +- src/main/daemon/pty-subprocess.test.ts | 9 +- src/main/daemon/pty-subprocess.ts | 19 ++- .../local-pty-utils-windows-fallback.test.ts | 34 +++-- src/main/providers/local-pty-utils.test.ts | 16 ++- src/main/providers/local-pty-utils.ts | 26 +++- src/preload/api-types.ts | 3 + src/preload/index.ts | 6 + .../sidebar/SidebarFeedbackDialog.test.tsx | 118 +++++++++++++++- .../sidebar/SidebarFeedbackDialog.tsx | 130 +++++------------- ...se-sidebar-feedback-environment-prefill.ts | 84 +++++++++++ .../sidebar/use-sidebar-feedback-images.ts | 129 +++++++++++++++++ .../terminal-pane/TerminalErrorToast.test.ts | 71 +++++++++- .../terminal-pane/TerminalErrorToast.tsx | 29 +++- .../src/hooks/useUnreadDockBadge.test.ts | 6 + src/renderer/src/hooks/useUnreadDockBadge.ts | 6 +- .../src/lib/client-environment-info.ts | 50 +++++++ src/renderer/src/store/slices/preflight.ts | 5 +- src/renderer/src/web/web-preload-api.ts | 2 + src/shared/client-environment-info.test.ts | 109 +++++++++++++++ src/shared/client-environment-info.ts | 62 +++++++++ 21 files changed, 786 insertions(+), 130 deletions(-) create mode 100644 src/renderer/src/components/sidebar/use-sidebar-feedback-environment-prefill.ts create mode 100644 src/renderer/src/components/sidebar/use-sidebar-feedback-images.ts create mode 100644 src/renderer/src/lib/client-environment-info.ts create mode 100644 src/shared/client-environment-info.test.ts create mode 100644 src/shared/client-environment-info.ts diff --git a/src/main/daemon/node-pty-error-hints.test.ts b/src/main/daemon/node-pty-error-hints.test.ts index e8fb5a0ef8c..f43951f05a6 100644 --- a/src/main/daemon/node-pty-error-hints.test.ts +++ b/src/main/daemon/node-pty-error-hints.test.ts @@ -50,7 +50,7 @@ describe('node-pty diagnostic error hints', () => { it('hints local wrapped spawn errors from pty allocation failures', () => { const message = - 'Failed to spawn shell "/bin/zsh": node-pty: open_slave failed: EMFILE (errno 24, Too many open files) - slave=\'/dev/ttys003\' (shell: /bin/zsh, cwd: /tmp, arch: arm64, platform: darwin 25.0.0). If this persists, please file an issue.' + 'Failed to spawn shell "/bin/zsh": node-pty: open_slave failed: EMFILE (errno 24, Too many open files) - slave=\'/dev/ttys003\' (shell: /bin/zsh, cwd: /tmp, arch: arm64, platform: darwin 25.0.0, orca: 1.4.178). If this persists, please file an issue.' expect(addNodePtyRecoveryHint(message)).toBe(`${PTY_ALLOCATION_HINT} ${message}`) }) diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index 9df17412c57..089595e85b7 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -2613,6 +2613,8 @@ describe('createPtySubprocess', () => { spawnMock.mockImplementation(() => { throw new Error('File not found: ') }) + const previousVersion = process.env.ORCA_APP_VERSION + process.env.ORCA_APP_VERSION = '1.4.178-test' try { expect(() => @@ -2623,9 +2625,14 @@ describe('createPtySubprocess', () => { shellOverride: 'not-a-real-shell.exe' }) ).toThrow( - /Daemon failed to spawn shell "not-a-real-shell\.exe" with cwd ".+": File not found:/ + /Daemon failed to spawn shell "not-a-real-shell\.exe" with cwd ".+": File not found:.*orca: 1\.4\.178-test/ ) } finally { + if (previousVersion === undefined) { + delete process.env.ORCA_APP_VERSION + } else { + process.env.ORCA_APP_VERSION = previousVersion + } if (platform) { Object.defineProperty(process, 'platform', platform) } diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 6fe39f57fba..7566b81f6fe 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines -- Why: daemon PTY spawning must keep platform launch setup, preflight, and lifecycle guards in one execution path. */ import * as pty from 'node-pty' import { statSync } from 'node:fs' +import { release } from 'node:os' import { delimiter, win32 as pathWin32 } from 'node:path' import type { SubprocessHandle } from './session' import { DaemonProtocolError } from './types' @@ -246,16 +247,25 @@ function removeInheritedElectronRunAsNode(env: Record): void { delete env.ELECTRON_RUN_AS_NODE } +function daemonEnvironmentDiagSuffix(): string { + const orca = process.env.ORCA_APP_VERSION?.trim() || '0.0.0-dev' + const systemVersion = + (process as NodeJS.Process & { getSystemVersion?: () => string }).getSystemVersion?.() || + release() + const platform = `${process.platform} ${systemVersion}` + return ` (orca: ${orca}, arch: ${process.arch}, platform: ${platform})` +} + /** * Formats a daemon preflight failure with the same ENOENT details node-pty exposes. */ function formatMissingDaemonPathError(kind: 'helper' | 'cwd', path: string): DaemonProtocolError { const detailName = kind === 'helper' ? 'helper' : 'cwd' const step = kind === 'helper' ? 'posix_spawn' : 'daemon_cwd' + const missingTarget = kind === 'helper' ? 'node-pty install' : 'working directory' + const diag = daemonEnvironmentDiagSuffix() return new DaemonProtocolError( - `Daemon's ${kind === 'helper' ? 'node-pty install' : 'working directory'} is gone ` + - `(worktree deleted?). Restart Orca. node-pty: ${step} failed: ENOENT ` + - `(errno 2, No such file or directory) - ${detailName}='${path}'` + `Daemon's ${missingTarget} is gone (worktree deleted?). Restart Orca. node-pty: ${step} failed: ENOENT (errno 2, No such file or directory) - ${detailName}='${path}'${diag}` ) } @@ -399,8 +409,9 @@ function preflightPosixPtySpawnEnvironment(validationCwd: string): void { */ function formatPtySpawnError(err: unknown, shellPath: string, spawnCwd: string): Error { const message = err instanceof Error ? err.message : String(err) + const diag = daemonEnvironmentDiagSuffix() const formatted = new DaemonProtocolError( - `Daemon failed to spawn shell "${shellPath}" with cwd "${spawnCwd}": ${message}` + `Daemon failed to spawn shell "${shellPath}" with cwd "${spawnCwd}": ${message}${diag}` ) if (err instanceof Error && err.stack) { formatted.stack = err.stack diff --git a/src/main/providers/local-pty-utils-windows-fallback.test.ts b/src/main/providers/local-pty-utils-windows-fallback.test.ts index 0de16d03f17..a71537ba8b2 100644 --- a/src/main/providers/local-pty-utils-windows-fallback.test.ts +++ b/src/main/providers/local-pty-utils-windows-fallback.test.ts @@ -133,18 +133,28 @@ describe('spawnShellWithFallback on Windows', () => { const ptySpawn = vi.fn(() => { throw new Error(ACCESS_DENIED_5) }) as unknown as typeof pty.spawn + const previousVersion = process.env.ORCA_APP_VERSION + process.env.ORCA_APP_VERSION = '1.4.178-test' - expect(() => - spawnShellWithFallback({ - shellPath: PWSH7, - shellArgs: attempts[0].shellArgs, - cols: 80, - rows: 24, - cwd: 'C:\\repo', - env: {}, - ptySpawn, - windowsFallbackAttempts: attempts - }) - ).toThrow(/Failed to spawn shell/) + try { + expect(() => + spawnShellWithFallback({ + shellPath: PWSH7, + shellArgs: attempts[0].shellArgs, + cols: 80, + rows: 24, + cwd: 'C:\\repo', + env: {}, + ptySpawn, + windowsFallbackAttempts: attempts + }) + ).toThrow(/Failed to spawn shell.*orca: 1\.4\.178-test/) + } finally { + if (previousVersion === undefined) { + delete process.env.ORCA_APP_VERSION + } else { + process.env.ORCA_APP_VERSION = previousVersion + } + } }) }) diff --git a/src/main/providers/local-pty-utils.test.ts b/src/main/providers/local-pty-utils.test.ts index bc812e1a46a..6b4846617ca 100644 --- a/src/main/providers/local-pty-utils.test.ts +++ b/src/main/providers/local-pty-utils.test.ts @@ -93,9 +93,21 @@ describe('validateWorkingDirectory', () => { it('rejects a missing native Windows path', () => { existsSyncMock.mockReturnValue(false) + const previousVersion = process.env.ORCA_APP_VERSION + process.env.ORCA_APP_VERSION = '1.4.178-test' - expect(() => validateWorkingDirectory(NATIVE_DIR)).toThrow(/does not exist/) - expect(wslUncDirectoryExistsMock).not.toHaveBeenCalled() + try { + expect(() => validateWorkingDirectory(NATIVE_DIR)).toThrow( + /does not exist.*orca: 1\.4\.178-test/ + ) + expect(wslUncDirectoryExistsMock).not.toHaveBeenCalled() + } finally { + if (previousVersion === undefined) { + delete process.env.ORCA_APP_VERSION + } else { + process.env.ORCA_APP_VERSION = previousVersion + } + } }) it('rejects a native Windows path that exists but is not a directory', () => { diff --git a/src/main/providers/local-pty-utils.ts b/src/main/providers/local-pty-utils.ts index 3e21095d535..cb414d05a8e 100644 --- a/src/main/providers/local-pty-utils.ts +++ b/src/main/providers/local-pty-utils.ts @@ -1,5 +1,6 @@ import { basename, isAbsolute, join } from 'node:path' import { existsSync, accessSync, statSync, chmodSync, constants as fsConstants } from 'node:fs' +import { release } from 'node:os' import type * as pty from 'node-pty' import { isWslUncPath } from '../../shared/wsl-paths' import { wslUncDirectoryExists } from '../wsl' @@ -99,10 +100,26 @@ export function ensureNodePtySpawnHelperExecutable(): void { } } +function formatLocalPtyEnvironmentDiag(extra: Record = {}): string { + const systemVersion = + (process as NodeJS.Process & { getSystemVersion?: () => string }).getSystemVersion?.() || + release() + const parts = { + ...extra, + arch: process.arch, + platform: `${process.platform} ${systemVersion}`, + orca: process.env.ORCA_APP_VERSION?.trim() || '0.0.0-dev' + } + return Object.entries(parts) + .map(([key, value]) => `${key}: ${value}`) + .join(', ') +} + function throwMissingWorkingDirectory(cwd: string): never { throw new Error( `Working directory "${cwd}" does not exist. ` + - `It may have been deleted or is on an unmounted volume.` + `It may have been deleted or is on an unmounted volume ` + + `(${formatLocalPtyEnvironmentDiag({ cwd })}).` ) } @@ -305,12 +322,7 @@ export function spawnShellWithFallback(params: ShellSpawnParams): ShellSpawnResu } } - const diag = [ - `shell: ${shellPath}`, - `cwd: ${cwd}`, - `arch: ${process.arch}`, - `platform: ${process.platform} ${process.getSystemVersion?.() ?? ''}` - ].join(', ') + const diag = formatLocalPtyEnvironmentDiag({ shell: shellPath, cwd }) throw new Error( `Failed to spawn shell "${shellPath}": ${primaryError ?? 'unknown error'} (${diag}). ` + `If this persists, please file an issue.` diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index ea5ba93a99c..6d7bc269b25 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1146,6 +1146,9 @@ export type PreloadApi = { get: () => { platform: NodeJS.Platform osRelease: string + arch: string + /** Login shell or ComSpec when available. */ + shell: string displayServer: 'wayland' | 'x11' | null } } diff --git a/src/preload/index.ts b/src/preload/index.ts index e8152ea1184..cef3ce65085 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -569,9 +569,15 @@ const api = { platform: { get: () => ({ platform: process.platform, + // Why: sandboxed preload cannot require node:os; Electron exposes the OS + // version on process.getSystemVersion when available. osRelease: (process as NodeJS.Process & { getSystemVersion?: () => string }).getSystemVersion?.() ?? '', + arch: process.arch, + // Why: these identify the default shell without probing user config files. + // process.env is available in the sandboxed preload; node:os is not. + shell: process.env.SHELL?.trim() || process.env.ComSpec?.trim() || '', displayServer: getLinuxDisplayServer() }) } satisfies PreloadApi['platform'], diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx index 70db571e77f..1920662056c 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx @@ -5,6 +5,8 @@ import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/re import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ + getPlatform: vi.fn(), + getVersion: vi.fn(), readFeedbackImageFiles: vi.fn(), submit: vi.fn(), toastWarning: vi.fn() @@ -57,14 +59,28 @@ beforeEach(() => { mocks.readFeedbackImageFiles.mockReset() mocks.submit.mockReset() mocks.toastWarning.mockReset() + mocks.getPlatform.mockReset() + mocks.getVersion.mockReset() mocks.submit.mockResolvedValue({ ok: true }) + mocks.getPlatform.mockReturnValue({ + platform: 'darwin', + osRelease: '25.0.0', + arch: 'arm64', + shell: '/bin/zsh', + displayServer: null + }) + mocks.getVersion.mockResolvedValue('1.4.178-rc.2') URL.revokeObjectURL = vi.fn() Object.defineProperty(window, 'api', { configurable: true, value: { feedback: { submit: mocks.submit }, gh: { viewer: vi.fn().mockResolvedValue(null) }, - shell: { openUrl: vi.fn() } + shell: { openUrl: vi.fn() }, + platform: { + get: mocks.getPlatform + }, + updater: { getVersion: mocks.getVersion } } }) }) @@ -73,6 +89,106 @@ afterEach(() => { cleanup() }) +describe('SidebarFeedbackDialog environment prefill', () => { + it('pre-inserts Orca version and OS info when the dialog opens', async () => { + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + + await waitFor(() => { + expect(textarea.value).toContain('Orca: 1.4.178-rc.2') + expect(textarea.value).toContain('OS: darwin 25.0.0 (arm64)') + expect(textarea.value).toContain('Shell: /bin/zsh') + }) + expect((screen.getByRole('button', { name: 'Send' }) as HTMLButtonElement).disabled).toBe(true) + }) + + it('keeps version info when the user types above the prefilled block', async () => { + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + await waitFor(() => expect(textarea.value).toContain('Orca: 1.4.178-rc.2')) + + fireEvent.change(textarea, { + target: { value: `Tabs feel slow\n\n${textarea.value.trim()}` } + }) + fireEvent.click(screen.getByRole('button', { name: 'Send' })) + + await waitFor(() => expect(mocks.submit).toHaveBeenCalledTimes(1)) + const submitted = mocks.submit.mock.calls[0]?.[0].feedback as string + expect(submitted).toContain('Tabs feel slow') + expect(submitted).toContain('Orca: 1.4.178-rc.2') + }) + + it('preserves early typing and appends the footer after version loading finishes', async () => { + let finishVersion: ((version: string) => void) | undefined + mocks.getVersion.mockReturnValue( + new Promise((resolve) => { + finishVersion = resolve + }) + ) + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + + fireEvent.change(textarea, { target: { value: 'Typed before loading' } }) + textarea.focus() + textarea.setSelectionRange(textarea.value.length, textarea.value.length) + await act(async () => finishVersion?.('1.4.178-rc.2')) + + await waitFor(() => expect(textarea.value).toContain('Orca: 1.4.178-rc.2')) + expect(textarea.value).toContain('Typed before loading') + expect(textarea.selectionStart).toBe('Typed before loading'.length) + }) + + it('enables Send when the user types below the prefilled footer', async () => { + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + await waitFor(() => expect(textarea.value).toContain('Orca: 1.4.178-rc.2')) + + fireEvent.change(textarea, { + target: { value: `${textarea.value.trim()}\nText below footer` } + }) + + expect((screen.getByRole('button', { name: 'Send' }) as HTMLButtonElement).disabled).toBe(false) + }) + + it('keeps Send disabled for an edited footer with no user text', async () => { + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + await waitFor(() => expect(textarea.value).toContain('Orca: 1.4.178-rc.2')) + + fireEvent.change(textarea, { + target: { value: '---\nOrca: custom build\nOS: edited' } + }) + + expect((screen.getByRole('button', { name: 'Send' }) as HTMLButtonElement).disabled).toBe(true) + }) + + it('allows a real report after the prefilled footer is deleted', async () => { + render() + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + await waitFor(() => expect(textarea.value).toContain('Orca: 1.4.178-rc.2')) + const send = screen.getByRole('button', { name: 'Send' }) as HTMLButtonElement + + fireEvent.change(textarea, { target: { value: '' } }) + expect(send.disabled).toBe(true) + fireEvent.change(textarea, { target: { value: 'Tabs hang after waking the laptop.' } }) + expect(send.disabled).toBe(false) + }) + + it('still prefills best-effort details when preload lookups fail', async () => { + mocks.getPlatform.mockImplementation(() => { + throw new Error('platform unavailable') + }) + mocks.getVersion.mockRejectedValue(new Error('version unavailable')) + + render() + + await waitFor(() => { + const textarea = screen.getByPlaceholderText('What could we improve?') as HTMLTextAreaElement + expect(textarea.value).toContain('Orca: unknown') + }) + }) +}) + describe('SidebarFeedbackDialog image submission', () => { it('keeps the dialog scrollable within short windows', () => { const { container } = render() diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx index d556f9233a8..1bdd1050a68 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx @@ -17,13 +17,12 @@ import type { GitHubViewer } from '../../../../shared/types' import { translate } from '@/i18n/i18n' import { extractImageFilesFromDataTransfer, - hasAttachableFeedbackImage, - readFeedbackImageFiles, - releaseFeedbackImageDraft, - type FeedbackImageDraft + hasAttachableFeedbackImage } from '@/lib/feedback-image-attachments' +import { stripClientEnvironmentFooter } from '../../../../shared/client-environment-info' import { SidebarFeedbackImageAttachments } from './SidebarFeedbackImageAttachments' -import { useFeedbackImageDrop } from './use-feedback-image-drop' +import { useSidebarFeedbackEnvironmentPrefill } from './use-sidebar-feedback-environment-prefill' +import { useSidebarFeedbackImages } from './use-sidebar-feedback-images' const GITHUB_ISSUES_URL = 'https://github.com/stablyai/orca/issues/' const DISCORD_URL = 'https://discord.gg/fzjDKHxv8Q' @@ -66,98 +65,28 @@ export function SidebarFeedbackDialog({ const [viewer, setViewer] = useState(null) const [isViewerLoading, setIsViewerLoading] = useState(false) const [submitAnonymously, setSubmitAnonymously] = useState(false) - const [images, setImages] = useState([]) - const [pendingImageReadCount, setPendingImageReadCount] = useState(0) const mountedRef = useMountedRef() const feedbackTextareaRef = useRef(null) - const liveImageDraftsRef = useRef([]) + const { + images, + pendingImageReadCount, + isDragActive, + contentRef, + dragHandlers, + handleAddFiles, + handleRemoveImage, + clearImages, + hasPendingImageReads, + getReservedImageSlots + } = useSidebarFeedbackImages({ open, isSubmitting, mountedRef }) - const clearImages = React.useCallback(() => { - liveImageDraftsRef.current.forEach(releaseFeedbackImageDraft) - liveImageDraftsRef.current = [] - setImages([]) - }, []) - - // Why: object URLs for the thumbnails leak until revoked, so drop them when - // the dialog unmounts as well as when an attachment is removed. - React.useEffect( - () => () => { - liveImageDraftsRef.current.forEach(releaseFeedbackImageDraft) - liveImageDraftsRef.current = [] - }, - [] - ) - - const imageCount = images.length - - // Why: committed state lags the in-flight reads, so batches still being read - // count against capacity — otherwise two quick pastes both see room for four. - const pendingImageReadsRef = useRef(0) - - const handleAddFiles = React.useCallback( - (files: readonly File[]) => { - if (files.length === 0) { - return - } - if (isSubmitting) { - toast.warning( - translate( - 'auto.components.sidebar.SidebarFeedbackDialog.attachWhileSending', - 'Wait for the current feedback to finish sending before attaching more images.' - ) - ) - return - } - // Why: read the committed count from the closure rather than a ref. A ref - // synced in an effect can still be stale-low right after an add, which - // over-accepts and gets the whole submission rejected by the main process. - const existingCount = imageCount + pendingImageReadsRef.current - pendingImageReadsRef.current += files.length - setPendingImageReadCount((current) => current + files.length) - void readFeedbackImageFiles(files, existingCount).then( - ({ images: added, errors }) => { - pendingImageReadsRef.current -= files.length - if (!mountedRef.current) { - added.forEach(releaseFeedbackImageDraft) - return - } - setPendingImageReadCount((current) => Math.max(0, current - files.length)) - if (added.length > 0) { - liveImageDraftsRef.current = [...liveImageDraftsRef.current, ...added] - setImages((existing) => [...existing, ...added]) - } - // Why: never drop an attachment without telling the user — that - // silence is what made screenshots vanish in the first place. - errors.forEach((error) => toast.warning(error)) - }, - (error: unknown) => { - pendingImageReadsRef.current -= files.length - console.error('Failed to read feedback image attachments:', error) - if (mountedRef.current) { - setPendingImageReadCount((current) => Math.max(0, current - files.length)) - toast.error( - translate( - 'auto.components.sidebar.SidebarFeedbackDialog.imageReadFailed', - 'Could not read the attached images. Try attaching them again.' - ) - ) - } - } - ) - }, - [imageCount, isSubmitting, mountedRef] - ) - - const handleRemoveImage = React.useCallback((id: string) => { - const removed = liveImageDraftsRef.current.find((image) => image.id === id) - if (removed) { - releaseFeedbackImageDraft(removed) - liveImageDraftsRef.current = liveImageDraftsRef.current.filter((image) => image.id !== id) - } - setImages((current) => current.filter((image) => image.id !== id)) - }, []) - - const { isDragActive, contentRef, dragHandlers } = useFeedbackImageDrop(open, handleAddFiles) + useSidebarFeedbackEnvironmentPrefill({ + open, + feedback, + setFeedback, + textareaRef: feedbackTextareaRef, + mountedRef + }) React.useEffect(() => { if (!open) { @@ -191,11 +120,12 @@ export function SidebarFeedbackDialog({ }, [open]) const handleSubmit = async (): Promise => { - if (isSubmitting || pendingImageReadsRef.current > 0) { + if (isSubmitting || hasPendingImageReads()) { return } const trimmed = feedback.trim() - if (!trimmed) { + const userText = stripClientEnvironmentFooter(feedback).trim() + if (!trimmed || !userText) { toast.warning( translate( 'auto.components.sidebar.SidebarFeedbackDialog.a2fd890d9e', @@ -287,7 +217,7 @@ export function SidebarFeedbackDialog({ // Why: consume the paste only when something is actually attachable. // An unsupported image still routes through for its rejection toast, // but preventing default there would silently eat co-pasted text. - if (hasAttachableFeedbackImage(pasted, imageCount + pendingImageReadsRef.current)) { + if (hasAttachableFeedbackImage(pasted, getReservedImageSlots())) { event.preventDefault() } handleAddFiles(pasted) @@ -430,7 +360,11 @@ export function SidebarFeedbackDialog({ + + + {copyLabel} + + + ) : null} + +

+ + {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 034/216] 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. */} +