From 47389990a5a5f71cfc60da01c49309a4c622df1c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:28:59 -0700 Subject: [PATCH 01/36] fix(mobile): surface disconnected source-control states + keep the mobile WS fallback port stable (STA-1511) (#7855) Co-authored-by: Orca --- .../app/h/[hostId]/history/[worktreeId].tsx | 51 +++++- .../MobileSourceControlContent.tsx | 10 ++ .../mobile-history-screen-state.test.ts | 47 ++++++ .../mobile-history-screen-state.ts | 30 ++++ .../mobile-source-control-styles.ts | 18 ++ mobile/src/transport/client-context.test.ts | 159 ++++++++++++++++++ mobile/src/transport/client-context.tsx | 11 ++ .../rpc/ws-fallback-port-store.test.ts | 35 ++++ .../runtime/rpc/ws-fallback-port-store.ts | 43 +++++ src/main/runtime/rpc/ws-transport.test.ts | 51 ++++++ src/main/runtime/rpc/ws-transport.ts | 40 ++++- src/main/runtime/runtime-rpc.ts | 10 +- 12 files changed, 489 insertions(+), 16 deletions(-) create mode 100644 mobile/src/source-control/mobile-history-screen-state.test.ts create mode 100644 mobile/src/source-control/mobile-history-screen-state.ts create mode 100644 mobile/src/transport/client-context.test.ts create mode 100644 src/main/runtime/rpc/ws-fallback-port-store.test.ts create mode 100644 src/main/runtime/rpc/ws-fallback-port-store.ts diff --git a/mobile/app/h/[hostId]/history/[worktreeId].tsx b/mobile/app/h/[hostId]/history/[worktreeId].tsx index 8ac0eb8a89b..ce7309eff0f 100644 --- a/mobile/app/h/[hostId]/history/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/history/[worktreeId].tsx @@ -3,14 +3,15 @@ import { ActivityIndicator, FlatList, Pressable, StyleSheet, Text, View } from ' import { SafeAreaView, useSafeAreaInsets } from 'react-native-safe-area-context' import { useLocalSearchParams, useRouter } from 'expo-router' import { ChevronDown, ChevronLeft, ChevronRight } from 'lucide-react-native' -import { useHostClient } from '../../../../src/transport/client-context' +import { useForceReconnect, useHostClient } from '../../../../src/transport/client-context' import type { RpcSuccess } from '../../../../src/transport/types' -import { colors, spacing, typography } from '../../../../src/theme/mobile-theme' +import { colors, radii, spacing, typography } from '../../../../src/theme/mobile-theme' import { fetchMobileGitHistory, mapMobileCommitRows, type MobileCommitRow } from '../../../../src/source-control/mobile-git-history' +import { resolveMobileHistoryScreenView } from '../../../../src/source-control/mobile-history-screen-state' import type { GitBranchChangeEntry } from '../../../../../src/shared/types' function firstParam(value: string | string[] | undefined): string { @@ -27,9 +28,11 @@ export default function HistoryScreen() { const router = useRouter() const insets = useSafeAreaInsets() const { client, state: connState } = useHostClient(hostId) + const forceReconnect = useForceReconnect() const [rows, setRows] = useState(null) const [error, setError] = useState(null) + const [reloadNonce, setReloadNonce] = useState(0) const [expanded, setExpanded] = useState(null) const [filesById, setFilesById] = useState>({}) @@ -57,7 +60,20 @@ export default function HistoryScreen() { return () => { active = false } - }, [client, connState, worktreeId]) + }, [client, connState, reloadNonce, worktreeId]) + + const retry = useCallback(() => { + setError(null) + // Why: retrying the fetch is useless while the transport's reconnect loop + // is parked at its backoff cap — revive the connection instead (mirrors + // MobileSourceControlPanel / issue #5049). The load effect re-runs via + // connState once the fresh client connects. + if (connState !== 'connected' && hostId) { + void forceReconnect(hostId) + return + } + setReloadNonce((n) => n + 1) + }, [connState, forceReconnect, hostId]) const toggleCommit = useCallback( (row: MobileCommitRow) => { @@ -130,6 +146,12 @@ export default function HistoryScreen() { [expanded, filesById, toggleCommit] ) + const view = resolveMobileHistoryScreenView({ + connected: client !== null && connState === 'connected', + rows, + error + }) + return ( @@ -138,21 +160,26 @@ export default function HistoryScreen() { Commit History - {error ? ( + {view.kind === 'error' || view.kind === 'waiting' ? ( - {error} + + {view.kind === 'waiting' ? 'Waiting for desktop...' : view.message} + + + Retry + - ) : rows === null ? ( + ) : view.kind === 'loading' ? ( - ) : rows.length === 0 ? ( + ) : view.kind === 'empty' ? ( No commits. ) : ( row.id} contentContainerStyle={{ paddingBottom: spacing.lg + insets.bottom }} @@ -175,6 +202,14 @@ const styles = StyleSheet.create({ title: { color: colors.textPrimary, fontSize: typography.bodySize, fontWeight: '600' }, state: { flex: 1, alignItems: 'center', justifyContent: 'center', padding: spacing.lg }, stateText: { color: colors.textMuted, fontSize: typography.bodySize }, + retryButton: { + marginTop: spacing.md, + paddingHorizontal: spacing.lg, + paddingVertical: spacing.sm, + borderRadius: radii.button, + backgroundColor: colors.bgRaised + }, + retryText: { color: colors.textPrimary, fontSize: typography.bodySize, fontWeight: '600' }, commit: { borderBottomWidth: 1, borderBottomColor: colors.borderSubtle }, commitHeader: { flexDirection: 'row', diff --git a/mobile/src/source-control/MobileSourceControlContent.tsx b/mobile/src/source-control/MobileSourceControlContent.tsx index 1b50db62479..3f1309fad97 100644 --- a/mobile/src/source-control/MobileSourceControlContent.tsx +++ b/mobile/src/source-control/MobileSourceControlContent.tsx @@ -16,6 +16,7 @@ type Props = { export function MobileSourceControlContent({ state }: Props) { const { insets, + connState, busyAction, commitMessage, setCommitMessage, @@ -56,6 +57,15 @@ export function MobileSourceControlContent({ state }: Props) { return ( <> + {connState !== 'connected' ? ( + // Why: once data has loaded the screen looks alive even when the + // desktop link is down, so taps appear to do nothing (STA-1511). + // Surface the reconnect state where the user is looking. + + + Reconnecting to desktop... + + ) : null} diff --git a/mobile/src/source-control/mobile-history-screen-state.test.ts b/mobile/src/source-control/mobile-history-screen-state.test.ts new file mode 100644 index 00000000000..1279f9a940f --- /dev/null +++ b/mobile/src/source-control/mobile-history-screen-state.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest' +import { resolveMobileHistoryScreenView } from './mobile-history-screen-state' +import type { MobileCommitRow } from './mobile-git-history' + +const row: MobileCommitRow = { + id: 'abc123', + shortId: 'abc123', + subject: 'commit', + author: 'author', + parentId: null, + relativeTime: '1m' +} + +describe('resolveMobileHistoryScreenView', () => { + it('shows waiting (not an endless spinner) when disconnected with nothing loaded', () => { + // Regression (STA-1511): the load effect no-ops while disconnected, so + // rows stay null forever — the screen must offer a retry, not a spinner. + expect(resolveMobileHistoryScreenView({ connected: false, rows: null, error: null })).toEqual({ + kind: 'waiting' + }) + }) + + it('spins only while connected and loading', () => { + expect(resolveMobileHistoryScreenView({ connected: true, rows: null, error: null })).toEqual({ + kind: 'loading' + }) + }) + + it('keeps already-loaded rows visible across a connection drop', () => { + expect(resolveMobileHistoryScreenView({ connected: false, rows: [row], error: null })).toEqual({ + kind: 'rows', + rows: [row] + }) + }) + + it('prefers the error state over waiting so failures stay actionable', () => { + expect(resolveMobileHistoryScreenView({ connected: false, rows: null, error: 'boom' })).toEqual( + { kind: 'error', message: 'boom' } + ) + }) + + it('distinguishes an empty history from a pending load', () => { + expect(resolveMobileHistoryScreenView({ connected: true, rows: [], error: null })).toEqual({ + kind: 'empty' + }) + }) +}) diff --git a/mobile/src/source-control/mobile-history-screen-state.ts b/mobile/src/source-control/mobile-history-screen-state.ts new file mode 100644 index 00000000000..8d03f33f028 --- /dev/null +++ b/mobile/src/source-control/mobile-history-screen-state.ts @@ -0,0 +1,30 @@ +import type { MobileCommitRow } from './mobile-git-history' + +export type MobileHistoryScreenView = + | { kind: 'error'; message: string } + | { kind: 'waiting' } + | { kind: 'loading' } + | { kind: 'empty' } + | { kind: 'rows'; rows: MobileCommitRow[] } + +// Why: the history load effect no-ops while the client isn't connected, so +// without an explicit 'waiting' view the screen showed an infinite spinner +// with no retry when opened during a reconnect window (STA-1511). Rows loaded +// before a drop stay visible — stale history beats a blank screen. +export function resolveMobileHistoryScreenView(input: { + connected: boolean + rows: MobileCommitRow[] | null + error: string | null +}): MobileHistoryScreenView { + const { connected, rows, error } = input + if (error) { + return { kind: 'error', message: error } + } + if (rows !== null) { + return rows.length === 0 ? { kind: 'empty' } : { kind: 'rows', rows } + } + if (!connected) { + return { kind: 'waiting' } + } + return { kind: 'loading' } +} diff --git a/mobile/src/source-control/mobile-source-control-styles.ts b/mobile/src/source-control/mobile-source-control-styles.ts index 3d22020651f..ea766731840 100644 --- a/mobile/src/source-control/mobile-source-control-styles.ts +++ b/mobile/src/source-control/mobile-source-control-styles.ts @@ -125,6 +125,24 @@ const baseStyles = StyleSheet.create({ fontWeight: '600', textTransform: 'capitalize' }, + reconnectBanner: { + flexDirection: 'row', + alignItems: 'center', + gap: spacing.sm, + marginHorizontal: spacing.lg, + marginTop: spacing.lg, + marginBottom: -spacing.sm, + paddingHorizontal: spacing.md, + paddingVertical: spacing.sm, + borderRadius: radii.button, + backgroundColor: colors.bgRaised, + borderWidth: StyleSheet.hairlineWidth, + borderColor: colors.statusAmber + }, + reconnectBannerText: { + color: colors.textPrimary, + fontSize: typography.metaSize + }, actionError: { marginTop: spacing.sm, paddingHorizontal: spacing.md, diff --git a/mobile/src/transport/client-context.test.ts b/mobile/src/transport/client-context.test.ts new file mode 100644 index 00000000000..e9a298b6a74 --- /dev/null +++ b/mobile/src/transport/client-context.test.ts @@ -0,0 +1,159 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { ConnectionState } from './types' +import type { RpcClient } from './rpc-client' + +const connectMock = vi.fn() +const loadHostsMock = vi.fn() + +vi.mock('./rpc-client', () => ({ + connect: (...args: unknown[]) => connectMock(...args) +})) +vi.mock('./host-store', () => ({ + loadHosts: () => loadHostsMock() +})) +vi.mock('./connection-revival-triggers', () => ({ + subscribeConnectionRevivalTriggers: () => () => {} +})) + +import { RpcClientProvider, useCloseHost, useHostClient } from './client-context' + +type FakeClient = RpcClient & { + emitState: (state: ConnectionState) => void + closeMock: ReturnType +} + +function makeFakeClient(initialState: ConnectionState): FakeClient { + let state = initialState + const listeners = new Set<(state: ConnectionState) => void>() + const closeMock = vi.fn() + return { + sendRequest: vi.fn(), + subscribe: vi.fn(() => () => {}), + updateTerminalSubscriptionViewport: vi.fn(), + getState: () => state, + getReconnectAttempt: () => 0, + getLastConnectedAt: () => null, + onStateChange: (listener) => { + listeners.add(listener) + return () => listeners.delete(listener) + }, + notifyForeground: vi.fn(), + close: closeMock, + closeMock, + emitState: (next) => { + state = next + for (const listener of listeners) { + listener(next) + } + } + } as FakeClient +} + +const HOST = { + id: 'host-1', + name: 'Host 1', + endpoint: 'ws://127.0.0.1:1', + deviceToken: 'token', + publicKeyB64: 'key', + lastConnected: 0 +} + +type Harness = { + readonly hook: ReturnType + readonly closeHost: (hostId: string) => void + readonly unmount: () => void +} + +function suppressReactTestRendererDeprecationWarning(): () => void { + const originalConsoleError = console.error + const spy = vi.spyOn(console, 'error').mockImplementation((...args) => { + const firstArg = args[0] + if (typeof firstArg === 'string' && firstArg.includes('react-test-renderer is deprecated')) { + return + } + originalConsoleError(...args) + }) + return () => spy.mockRestore() +} + +async function renderHarness(hostId: string): Promise { + let hook: ReturnType | null = null + let closeHost: ((hostId: string) => void) | null = null + let renderer: ReactTestRenderer | null = null + + function Probe(): null { + hook = useHostClient(hostId) + closeHost = useCloseHost() + return null + } + + const restore = suppressReactTestRendererDeprecationWarning() + try { + await act(async () => { + renderer = create(createElement(RpcClientProvider, null, createElement(Probe))) + }) + } finally { + restore() + } + if (!hook || !closeHost || !renderer) { + throw new Error('harness did not render') + } + const mounted = renderer as ReactTestRenderer + return { + get hook() { + if (!hook) { + throw new Error('hook not rendered') + } + return hook + }, + closeHost: (id) => { + if (!closeHost) { + throw new Error('closeHost not rendered') + } + closeHost(id) + }, + unmount: () => mounted.unmount() + } +} + +beforeEach(() => { + connectMock.mockReset() + loadHostsMock.mockReset() +}) + +describe('useHostClient', () => { + it('drops the closed client when the host entry is removed', async () => { + const fake = makeFakeClient('connected') + connectMock.mockReturnValue(fake) + loadHostsMock.mockResolvedValue([HOST]) + + const harness = await renderHarness(HOST.id) + expect(harness.hook.client).toBe(fake) + expect(harness.hook.state).toBe('connected') + + // Regression (STA-1511): closeHost deletes the entry; before the fix the + // hook kept handing out the closed client, so mounted screens kept + // driving requests that could never resolve. + await act(async () => { + harness.closeHost(HOST.id) + }) + expect(fake.closeMock).toHaveBeenCalled() + expect(harness.hook.client).toBeNull() + expect(harness.hook.state).toBe('disconnected') + + harness.unmount() + }) + + it('reports disconnected instead of hanging when the host id is unknown', async () => { + loadHostsMock.mockResolvedValue([]) + + const harness = await renderHarness('missing-host') + expect(connectMock).not.toHaveBeenCalled() + expect(harness.hook.client).toBeNull() + expect(harness.hook.state).toBe('disconnected') + + harness.unmount() + }) +}) diff --git a/mobile/src/transport/client-context.tsx b/mobile/src/transport/client-context.tsx index f889cba51ff..2b8461c582f 100644 --- a/mobile/src/transport/client-context.tsx +++ b/mobile/src/transport/client-context.tsx @@ -133,6 +133,11 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { return null } if (!host) { + // Why: returning silently leaves mounted screens on a permanent + // spinner (STA-1511) — surface 'disconnected' so they can render + // their waiting/retry affordance instead. + notifyHostState(hostId, 'disconnected') + notifyAllHosts() return null } } @@ -406,6 +411,12 @@ export function useHostClient(hostId: string | undefined): { if (found && found.client !== clientRef.current) { clientRef.current = found.client force((n) => n + 1) + } else if (!found && clientRef.current) { + // Why: closeHost deletes the entry without a replacement; holding the + // closed client would let screens keep issuing requests that can never + // resolve (STA-1511). Null it so they render disconnected states. + clientRef.current = null + force((n) => n + 1) } }) const initial = ctx.acquire(hostId) diff --git a/src/main/runtime/rpc/ws-fallback-port-store.test.ts b/src/main/runtime/rpc/ws-fallback-port-store.test.ts new file mode 100644 index 00000000000..2edd314c212 --- /dev/null +++ b/src/main/runtime/rpc/ws-fallback-port-store.test.ts @@ -0,0 +1,35 @@ +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { readWsFallbackPort, writeWsFallbackPort } from './ws-fallback-port-store' + +function makeUserDataPath(): string { + return mkdtempSync(join(tmpdir(), 'ws-fallback-port-test-')) +} + +describe('ws-fallback-port-store', () => { + it('round-trips a persisted fallback port', () => { + const userDataPath = makeUserDataPath() + expect(readWsFallbackPort(userDataPath)).toBeUndefined() + writeWsFallbackPort(userDataPath, 54321) + expect(readWsFallbackPort(userDataPath)).toBe(54321) + }) + + it('ignores corrupt or invalid contents', () => { + const userDataPath = makeUserDataPath() + writeFileSync(join(userDataPath, 'mobile-ws-fallback-port.json'), 'not json', 'utf8') + expect(readWsFallbackPort(userDataPath)).toBeUndefined() + writeFileSync(join(userDataPath, 'mobile-ws-fallback-port.json'), '{"port":-4}', 'utf8') + expect(readWsFallbackPort(userDataPath)).toBeUndefined() + writeFileSync(join(userDataPath, 'mobile-ws-fallback-port.json'), '{"port":"80"}', 'utf8') + expect(readWsFallbackPort(userDataPath)).toBeUndefined() + }) + + it('refuses to persist an invalid port', () => { + const userDataPath = makeUserDataPath() + writeWsFallbackPort(userDataPath, 0) + writeWsFallbackPort(userDataPath, 70000) + expect(readWsFallbackPort(userDataPath)).toBeUndefined() + }) +}) diff --git a/src/main/runtime/rpc/ws-fallback-port-store.ts b/src/main/runtime/rpc/ws-fallback-port-store.ts new file mode 100644 index 00000000000..93871cad631 --- /dev/null +++ b/src/main/runtime/rpc/ws-fallback-port-store.ts @@ -0,0 +1,43 @@ +import { readFileSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' + +// Why: when the preferred WS port is taken (second Orca instance), the OS +// assigns a random port. Paired mobile devices store ws://ip:port endpoints, +// so a port that changes on every restart permanently orphans those pairings +// (STA-1511). Persist the assigned fallback so the same instance re-binds the +// same port next launch. + +const FALLBACK_PORT_FILE = 'mobile-ws-fallback-port.json' + +function isValidPort(value: unknown): value is number { + return typeof value === 'number' && Number.isInteger(value) && value > 0 && value <= 65535 +} + +export function readWsFallbackPort(userDataPath: string): number | undefined { + try { + const raw = readFileSync(join(userDataPath, FALLBACK_PORT_FILE), 'utf8') + const parsed: unknown = JSON.parse(raw) + if ( + typeof parsed === 'object' && + parsed !== null && + isValidPort((parsed as { port?: unknown }).port) + ) { + return (parsed as { port: number }).port + } + } catch { + // Missing or corrupt file — treated as "no previous fallback". + } + return undefined +} + +export function writeWsFallbackPort(userDataPath: string, port: number): void { + if (!isValidPort(port)) { + return + } + try { + writeFileSync(join(userDataPath, FALLBACK_PORT_FILE), JSON.stringify({ port }), 'utf8') + } catch { + // Why: persistence is best-effort — failing to record the port must not + // break transport startup; the cost is a re-pair after the next restart. + } +} diff --git a/src/main/runtime/rpc/ws-transport.test.ts b/src/main/runtime/rpc/ws-transport.test.ts index b9f4c94ddcd..a44469d1fcc 100644 --- a/src/main/runtime/rpc/ws-transport.test.ts +++ b/src/main/runtime/rpc/ws-transport.test.ts @@ -441,4 +441,55 @@ describe('WebSocketTransport', () => { expect(serverClosed).toBe(true) expect(wss.clients.size).toBe(0) }, 5_000) + + // Why: paired mobile devices store ws://ip:port endpoints, so the fallback + // port must stay stable across restarts or pairings go permanently dead + // when the preferred port is held by another instance (STA-1511). + describe('fallback port stability', () => { + async function reserveFreePort(): Promise { + const scratch = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) + await scratch.start() + const port = scratch.resolvedPort + await scratch.stop() + return port + } + + it('retries the persisted fallback port before an OS-assigned one', async () => { + const holder = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) + transports.push(holder) + await holder.start() + const takenPort = holder.resolvedPort + const fallbackPort = await reserveFreePort() + + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: takenPort, + fallbackPort + }) + transports.push(transport) + await transport.start() + expect(transport.resolvedPort).toBe(fallbackPort) + }) + + it('falls back to an OS-assigned port when the persisted port is also taken', async () => { + const holder = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) + const fallbackHolder = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) + transports.push(holder, fallbackHolder) + await holder.start() + await fallbackHolder.start() + const takenPort = holder.resolvedPort + const takenFallbackPort = fallbackHolder.resolvedPort + + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: takenPort, + fallbackPort: takenFallbackPort + }) + transports.push(transport) + await transport.start() + expect(transport.resolvedPort).not.toBe(takenPort) + expect(transport.resolvedPort).not.toBe(takenFallbackPort) + expect(transport.resolvedPort).toBeGreaterThan(0) + }) + }) }) diff --git a/src/main/runtime/rpc/ws-transport.ts b/src/main/runtime/rpc/ws-transport.ts index 37f6094b589..7b1d8b8ead0 100644 --- a/src/main/runtime/rpc/ws-transport.ts +++ b/src/main/runtime/rpc/ws-transport.ts @@ -48,6 +48,11 @@ export type WebSocketTransportOptions = { // Why: the pairing server can also serve the browser client, so users do // not need a second dev/static server once the web bundle is built. staticRoot?: string + // Why: when the preferred port is taken, an unstable OS-assigned port would + // change on every restart and permanently orphan existing mobile pairings + // (their stored ws://ip:port endpoint goes dead — STA-1511). Callers pass + // the previously assigned fallback port so it is retried first. + fallbackPort?: number } export class WebSocketTransport implements RpcTransport { @@ -58,6 +63,7 @@ export class WebSocketTransport implements RpcTransport { private readonly heartbeatIntervalMs: number private readonly preAuthTimeoutMs: number private readonly staticRoot: string | undefined + private readonly fallbackPort: number | undefined private httpServer: HttpsServer | HttpServer | null = null private wss: WebSocketServer | null = null private heartbeatTimer: ReturnType | null = null @@ -81,7 +87,8 @@ export class WebSocketTransport implements RpcTransport { tlsKey, heartbeatIntervalMs, preAuthTimeoutMs, - staticRoot + staticRoot, + fallbackPort }: WebSocketTransportOptions) { this.host = host this.port = port @@ -90,6 +97,7 @@ export class WebSocketTransport implements RpcTransport { this.heartbeatIntervalMs = heartbeatIntervalMs ?? HEARTBEAT_INTERVAL_MS this.preAuthTimeoutMs = preAuthTimeoutMs ?? PRE_AUTH_TIMEOUT_MS this.staticRoot = staticRoot + this.fallbackPort = fallbackPort } onMessage(handler: WebSocketMessageHandler): void { @@ -144,19 +152,37 @@ export class WebSocketTransport implements RpcTransport { // Why: when the preferred port is occupied (e.g. another Orca instance is // already running), fall back to an OS-assigned port so mobile pairing // still works. The QR code reads resolvedPort after start, so it will - // advertise the correct port regardless. + // advertise the correct port regardless. A persisted fallback port is + // retried before port 0 so paired devices keep a stable endpoint across + // restarts (STA-1511). let port = this.port try { await this.tryListen(port) + return } catch (error: unknown) { - if (isEAddressInUse(error) && port !== 0) { - console.warn(`[ws-transport] Port ${port} is in use, falling back to OS-assigned port`) - port = 0 - await this.tryListen(port) - } else { + if (!isEAddressInUse(error) || port === 0) { throw error } } + if ( + this.fallbackPort !== undefined && + this.fallbackPort !== 0 && + this.fallbackPort !== this.port + ) { + try { + console.warn( + `[ws-transport] Port ${port} is in use, retrying previous fallback port ${this.fallbackPort}` + ) + await this.tryListen(this.fallbackPort) + return + } catch (error: unknown) { + if (!isEAddressInUse(error)) { + throw error + } + } + } + console.warn(`[ws-transport] Port ${port} is in use, falling back to OS-assigned port`) + await this.tryListen(0) } private createHttpServer(): HttpServer | HttpsServer { diff --git a/src/main/runtime/runtime-rpc.ts b/src/main/runtime/runtime-rpc.ts index c2f949c2733..8e05aaeec02 100644 --- a/src/main/runtime/runtime-rpc.ts +++ b/src/main/runtime/runtime-rpc.ts @@ -16,6 +16,7 @@ import { errorResponse } from './rpc/errors' import type { RpcMessageContext, RpcTransport } from './rpc/transport' import { UnixSocketTransport } from './rpc/unix-socket-transport' import { WebSocketTransport } from './rpc/ws-transport' +import { readWsFallbackPort, writeWsFallbackPort } from './rpc/ws-fallback-port-store' import type { WebSocket } from 'ws' import { DeviceRegistry, type DeviceScope } from './device-registry' import { loadOrCreateE2EEKeypair, type E2EEKeypair } from './e2ee-keypair' @@ -702,7 +703,11 @@ export class OrcaRuntimeRpcServer { const wsTransport = new WebSocketTransport({ host: '0.0.0.0', port: this.wsPort, - staticRoot: this.webClientRoot + staticRoot: this.webClientRoot, + // Why: keep the fallback port stable across restarts so paired + // devices' stored endpoints stay valid (STA-1511). wsPort 0 means + // the caller explicitly wants a random port (E2E) — don't pin it. + ...(this.wsPort !== 0 ? { fallbackPort: readWsFallbackPort(this.userDataPath) } : {}) }) this.wsTransport = wsTransport @@ -785,6 +790,9 @@ export class OrcaRuntimeRpcServer { }) await wsTransport.start() + if (this.wsPort !== 0 && wsTransport.resolvedPort !== this.wsPort) { + writeWsFallbackPort(this.userDataPath, wsTransport.resolvedPort) + } activeTransports.push(wsTransport) transportsMeta.push({ kind: 'websocket', From c2b074bd154aa200a5bb7274b0412b32fb192b49 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:29:36 -0700 Subject: [PATCH 02/36] fix(terminal): shape Arabic/RTL text on the cell grid via xterm character joiner (#7665) * fix(terminal): shape Arabic/RTL runs on the cell grid via xterm character joiner (#5262) Register a character joiner that groups contiguous RTL runs (tunneling through neutral spaces/digits/punctuation between RTL words) so both the WebGL and DOM renderers draw each run as one string, applying native contextual shaping and BiDi ordering inside the run's grid-aligned cell box. Buffer and PTY stream are untouched; xterm un-joins ranges holding the cursor or a partial selection, keeping cursor visibility and selection cell-accurate. Non-RTL lines early-return via a single charCodeAt sweep. Co-authored-by: Orca * Deregister Arabic shaping joiner when disposing pane Since xterm.js does not automatically remove registered character joiners upon terminal disposal, they must be explicitly deregistered to avoid memory and registration leaks. - Update `registerArabicShapingJoiner` to return a cleanup function. - Store and call the cleanup handler when disposing a managed pane. - Update tests to assert proper registration and deregistration. * fix(terminal): gate RTL joining on live WebGL, tunnel ZWNJ/ZWJ/RLM, lock joiner lifecycle in tests Review findings on the Arabic shaping branch: - The DOM renderer sizes a joined span with one letter-spacing value applied per character, so joined RTL runs blew out row grid alignment whenever the WebGL renderer was unavailable (GPU off, auto policy, context loss). The joiner now only returns ranges while the pane's WebGL addon is live. - ZWNJ/ZWJ/RLM (mandatory in Persian orthography) hard-broke RTL runs, rendering word halves in swapped visual order. They are now transparent inside a run without extending or counting toward it. - openTerminal's existing unicode-ordering test failed at HEAD because the fake terminal lacked registerCharacterJoiner; the harness now implements the joiner API and a new test locks register-on-open/deregister-on-dispose. Co-authored-by: Orca * fix(terminal): keep zero-width RTL marks from opening joined runs, treat ALM as transparent Round-2 review findings: - A combining mark (tashkeel/niqqud) orphaned after an LTR base could open an RTL run mid-cell; xterm rounds that string range to an empty joined cell range and the WebGL renderer draws an empty glyph over the next character. Marks now extend and count only in runs opened by a spacing RTL letter. - ALM (U+061C) sat inside the strong-RTL block scan, so it opened and counted toward runs unlike RLM. It now joins the transparent set. - New test locks openTerminal wiring the joiner predicate to pane.webglAddon (previously () => true would pass the suite). Co-authored-by: Orca * fix(terminal): treat zero-width Cf controls in RTL blocks as run-transparent Round-3 review finding: zero-width format controls that live inside the strong-RTL scan ranges (Arabic number signs U+0600-0605, end of ayah U+06DD, Syriac abbreviation mark U+070F, disputed end of ayah U+08E2) and the BOM/ ZWNBSP U+FEFF are width-0 in xterm. Like combining marks they could open a run mid-cell, producing an empty joined cell range that blanks the following glyph in the WebGL renderer. They are Cf not Mn, so canOpenRtlRun could not catch them. Fold them into the run-transparent set (never open, count, or break a run) and document the upstream standalone-width-0-cell join skew. Co-authored-by: Orca * Document WebGL decoration limitation for Arabic shaping joiner Add comments explaining a known upstream limitation in xterm.js WebGL renderer where search-match highlights inside a joined run render all-or-nothing, similar to ligatures. --------- Co-authored-by: Orca --- .../src/lib/pane-manager/pane-dom-creation.ts | 1 + .../lib/pane-manager/pane-lifecycle.test.ts | 107 ++++++-- .../src/lib/pane-manager/pane-lifecycle.ts | 18 ++ .../lib/pane-manager/pane-manager-types.ts | 3 + .../terminal-arabic-shaping-joiner.test.ts | 250 ++++++++++++++++++ .../terminal-arabic-shaping-joiner.ts | 187 +++++++++++++ 6 files changed, 540 insertions(+), 26 deletions(-) create mode 100644 src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts create mode 100644 src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts diff --git a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts index 43b628ffbd5..365bfd80178 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts @@ -148,6 +148,7 @@ export function createPaneDOM( compositionHandler: null, focusClassSyncCleanup: null, terminalScrollIntentDisposable: null, + arabicShapingJoinerCleanup: null, pendingSplitScrollState: null, pendingSplitScrollRafIds: [], pendingSplitScrollTimerId: null, diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts index 16791a485b4..e96a8efec62 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.test.ts @@ -6,7 +6,7 @@ import { markComplexScriptOutput, resetTerminalWebglSuggestion } from './pane-webgl-renderer' -import { attachLigatures, openTerminal } from './pane-lifecycle' +import { attachLigatures, disposePane, openTerminal } from './pane-lifecycle' import { buildDefaultTerminalOptions, DEFAULT_TERMINAL_FAST_SCROLL_SENSITIVITY, @@ -410,24 +410,27 @@ describe('attachLigatures', () => { }) }) -describe('openTerminal — Unicode 11 ordering', () => { +describe('openTerminal — addon and provider wiring', () => { beforeEach(() => { vi.stubGlobal('requestAnimationFrame', () => 1) + vi.stubGlobal('cancelAnimationFrame', vi.fn()) }) afterEach(() => { vi.unstubAllGlobals() }) - // Why: CJK / emoji / ZWJ widths get baked into the buffer at the active - // unicode version on write. If anything writes bytes through xterm before - // unicode v11 is activated (still on default v6 width tables), wide chars - // lay out as single cells. The bug surfaces as the broken `?`-style glyphs - // users saw on worktree switch. - it('activates unicode 11 before any caller-driven write would be possible', () => { + function createOpenTerminalHarness(): { + pane: ManagedPaneInternal + events: string[] + getRegisteredJoinHandler: () => ((text: string) => [number, number][]) | null + } { const events: string[] = [] + let registeredJoinHandler: ((text: string) => [number, number][]) | null = null - const fitAddon = { fit: vi.fn() } as unknown as ManagedPaneInternal['fitAddon'] + const fitAddon = { + fit: vi.fn() + } as unknown as ManagedPaneInternal['fitAddon'] const searchAddon = {} as unknown as ManagedPaneInternal['searchAddon'] const serializeAddon = {} as unknown as ManagedPaneInternal['serializeAddon'] const unicode11Addon = {} as unknown as ManagedPaneInternal['unicode11Addon'] @@ -493,6 +496,14 @@ describe('openTerminal — Unicode 11 ordering', () => { write: vi.fn(() => { events.push('write') }), + registerCharacterJoiner: vi.fn((handler: (text: string) => [number, number][]) => { + events.push('registerCharacterJoiner') + registeredJoinHandler = handler + return 3 + }), + deregisterCharacterJoiner: vi.fn((joinerId: number) => { + events.push(`deregisterCharacterJoiner:${joinerId}`) + }), unicode: unicodeProxy, buffer: { active: { cursorX: 0, cursorY: 0 } } } as unknown as ManagedPaneInternal['terminal'] @@ -525,26 +536,70 @@ describe('openTerminal — Unicode 11 ordering', () => { debugLabel: null } - try { - openTerminal(pane) + return { pane, events, getRegisteredJoinHandler: () => registeredJoinHandler } + } - expect(fakePaneContainer.appendChild).toHaveBeenCalledWith(fakeTooltip) - expect(fakeXtermContainer.appendChild).not.toHaveBeenCalled() - expect(fakeTerminalElement.appendChild).not.toHaveBeenCalled() - expect(events).toContain('loadAddon:unicode11') - expect(events).toContain('activeVersion=11') + // Why: CJK / emoji / ZWJ widths get baked into the buffer at the active + // unicode version on write. If anything writes bytes through xterm before + // unicode v11 is activated (still on default v6 width tables), wide chars + // lay out as single cells. The bug surfaces as the broken `?`-style glyphs + // users saw on worktree switch. + it('activates unicode 11 before any caller-driven write would be possible', () => { + const { pane, events } = createOpenTerminalHarness() - const unicodeIdx = events.indexOf('activeVersion=11') - const writeIdx = events.indexOf('write') - if (writeIdx !== -1) { - expect(unicodeIdx).toBeLessThan(writeIdx) - } + openTerminal(pane) - const loadUnicodeIdx = events.indexOf('loadAddon:unicode11') - expect(loadUnicodeIdx).toBeLessThan(unicodeIdx) - expect(events.indexOf('open')).toBeLessThan(loadUnicodeIdx) - } finally { - vi.unstubAllGlobals() + expect(pane.container.appendChild).toHaveBeenCalledWith(pane.linkTooltip) + expect(pane.xtermContainer.appendChild).not.toHaveBeenCalled() + expect(pane.terminal.element!.appendChild).not.toHaveBeenCalled() + expect(events).toContain('loadAddon:unicode11') + expect(events).toContain('activeVersion=11') + + const unicodeIdx = events.indexOf('activeVersion=11') + const writeIdx = events.indexOf('write') + if (writeIdx !== -1) { + expect(unicodeIdx).toBeLessThan(writeIdx) } + + const loadUnicodeIdx = events.indexOf('loadAddon:unicode11') + expect(loadUnicodeIdx).toBeLessThan(unicodeIdx) + expect(events.indexOf('open')).toBeLessThan(loadUnicodeIdx) + }) + + // Why: terminal.dispose() does not deregister character joiners, so the + // pane lifecycle must — this locks the register/deregister pairing that + // makes Arabic/RTL shaping (#5262) actually reach a real terminal. + it('registers the Arabic shaping joiner on open and deregisters it on dispose', () => { + const { pane, events } = createOpenTerminalHarness() + + openTerminal(pane) + + expect(events).toContain('registerCharacterJoiner') + expect(events.indexOf('open')).toBeLessThan(events.indexOf('registerCharacterJoiner')) + expect(pane.arabicShapingJoinerCleanup).toBeTypeOf('function') + + disposePane(pane, new Map([[pane.id, pane]])) + + expect(events).toContain('deregisterCharacterJoiner:3') + expect(pane.arabicShapingJoinerCleanup).toBeNull() + }) + + // Why: the DOM renderer misrenders joined spans (per-character + // letter-spacing blowout), so the joiner must only join while this pane's + // WebGL addon is live — locked here against the real openTerminal wiring. + it('joins RTL runs only while the pane has a live WebGL addon', () => { + const { pane, getRegisteredJoinHandler } = createOpenTerminalHarness() + + openTerminal(pane) + const handler = getRegisteredJoinHandler()! + + expect(pane.webglAddon).toBeNull() + expect(handler('مرحبا')).toEqual([]) + + pane.webglAddon = {} as never + expect(handler('مرحبا')).toEqual([[0, 5]]) + + pane.webglAddon = null + expect(handler('مرحبا')).toEqual([]) }) }) diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts index c6727c3c1c1..c984508d463 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts @@ -17,6 +17,7 @@ import { attachTerminalMouseWheelMultiplier } from './pane-terminal-mouse-wheel' import { attachTerminalScrollIntentTracking } from './terminal-scroll-intent' import { attachDomRendererFocusClassSync } from './pane-dom-focus-class-sync' import { attachWebgl, cancelPendingWebglRefresh, disposeWebgl } from './pane-webgl-renderer' +import { registerArabicShapingJoiner } from './terminal-arabic-shaping-joiner' import { resolveCursorAgentImeAnchor } from './terminal-ime-anchor' // --------------------------------------------------------------------------- @@ -72,6 +73,16 @@ export function openTerminal(pane: ManagedPaneInternal): void { // so the activation must stay at this position. activateOrcaTerminalUnicodeProvider(terminal) + // Why: without run-joining, Arabic/Hebrew output renders as disconnected + // letters in reversed order (#5262). Registered up front so restored + // scrollback and reattach replays shape correctly, not just live output. + // Joining tracks the live WebGL renderer: the DOM fallback misrenders + // joined spans (see registerArabicShapingJoiner), so it stays per-cell. + pane.arabicShapingJoinerCleanup = registerArabicShapingJoiner( + terminal, + () => pane.webglAddon != null + ) + // Why: the OS reads the focused textarea's screen rect at compositionstart to // decide where to display the IME candidate window. xterm positions that // textarea from its own cursor, which can be stale or intentionally hidden by @@ -224,6 +235,13 @@ export function disposePane( pane.focusClassSyncCleanup = null pane.terminalScrollIntentDisposable?.dispose() pane.terminalScrollIntentDisposable = null + // Deregister the RTL shaping joiner: terminal.dispose() below does not. + try { + pane.arabicShapingJoinerCleanup?.() + } catch { + /* ignore */ + } + pane.arabicShapingJoinerCleanup = null if (pane.compositionHandler) { pane.terminal.element?.removeEventListener('compositionstart', pane.compositionHandler) pane.terminal.element?.removeEventListener('compositionupdate', pane.compositionHandler) diff --git a/src/renderer/src/lib/pane-manager/pane-manager-types.ts b/src/renderer/src/lib/pane-manager/pane-manager-types.ts index 1ab542bf787..8218de5e856 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-types.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-types.ts @@ -161,6 +161,9 @@ export type ManagedPaneInternal = { focusClassSyncCleanup?: (() => void) | null // Stored so disposePane() can remove user-scroll intent listeners. terminalScrollIntentDisposable?: IDisposable | null + // Stored so disposePane() can deregister the joiner; terminal.dispose() + // does not remove registered character joiners. + arabicShapingJoinerCleanup?: (() => void) | null // Why: splitPane reparents DOM; its delayed restore owns scroll until the // browser settles, so intermediate fits must not compete with it. pendingSplitScrollState: ScrollState | null diff --git a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts new file mode 100644 index 00000000000..6bed3d7c01d --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.test.ts @@ -0,0 +1,250 @@ +import { describe, expect, it } from 'vitest' + +import { + findRtlJoinRanges, + isStrongRtlCodePoint, + registerArabicShapingJoiner +} from './terminal-arabic-shaping-joiner' + +describe('isStrongRtlCodePoint', () => { + it('classifies Arabic and Hebrew letters as strong RTL', () => { + expect(isStrongRtlCodePoint('م'.codePointAt(0)!)).toBe(true) + expect(isStrongRtlCodePoint('ش'.codePointAt(0)!)).toBe(true) + expect(isStrongRtlCodePoint('א'.codePointAt(0)!)).toBe(true) + // Arabic presentation forms (legacy shaped codepoints). + expect(isStrongRtlCodePoint(0xfe8d)).toBe(true) + // Adlam (supplementary plane). + expect(isStrongRtlCodePoint(0x1e900)).toBe(true) + }) + + it('does not classify Latin, box drawing, CJK, or emoji as RTL', () => { + expect(isStrongRtlCodePoint('a'.codePointAt(0)!)).toBe(false) + expect(isStrongRtlCodePoint('│'.codePointAt(0)!)).toBe(false) + expect(isStrongRtlCodePoint('漢'.codePointAt(0)!)).toBe(false) + expect(isStrongRtlCodePoint(0x1f600)).toBe(false) + }) +}) + +describe('findRtlJoinRanges', () => { + it('returns no ranges for plain ASCII text', () => { + expect(findRtlJoinRanges('ls -la | grep foo && echo done')).toEqual([]) + }) + + it('returns no ranges for Latin-1/Cyrillic/Greek text below the RTL floor', () => { + expect(findRtlJoinRanges('café привет αβγ')).toEqual([]) + }) + + it('returns a fresh array on every call so xterm can merge into it safely', () => { + const first = findRtlJoinRanges('plain') + const second = findRtlJoinRanges('plain') + expect(first).not.toBe(second) + }) + + it('joins a single Arabic word as one range', () => { + const text = 'مرحبا' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('joins a multi-word Arabic phrase across spaces as one range', () => { + const text = 'مرحباً هذه مشكلة في اللغة العربية' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('excludes leading and trailing neutrals from the range', () => { + const text = ' مرحبا هذه ' + expect(findRtlJoinRanges(text)).toEqual([[2, 11]]) + }) + + it('stops the run at strong LTR words', () => { + const text = 'مرحبا hello' + expect(findRtlJoinRanges(text)).toEqual([[0, 5]]) + }) + + it('does not pull an adjacent filename into the run', () => { + const text = 'ملف test.txt' + expect(findRtlJoinRanges(text)).toEqual([[0, 3]]) + }) + + it('treats box-drawing characters as run breakers so TUI borders stay per-cell', () => { + const text = '│ مرحبا بكم │' + expect(findRtlJoinRanges(text)).toEqual([[2, 11]]) + }) + + it('produces separate ranges for RTL runs split by LTR text', () => { + const text = 'اهلا and שלום' + expect(findRtlJoinRanges(text)).toEqual([ + [0, 4], + [9, 13] + ]) + }) + + it('skips an isolated single RTL letter (already correct in isolated form)', () => { + expect(findRtlJoinRanges('a م b')).toEqual([]) + }) + + it('joins a letter with its combining tashkeel marks', () => { + const text = 'مَ' + expect(findRtlJoinRanges(text)).toEqual([[0, 2]]) + }) + + it('tunnels through ASCII digits between Arabic words', () => { + const text = 'صفحة 15 من 20 صفحة' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('does not extend a run through trailing digits without a following RTL char', () => { + const text = 'صفحة 15' + expect(findRtlJoinRanges(text)).toEqual([[0, 4]]) + }) + + it('joins Arabic-Indic digits and Arabic punctuation as part of the run', () => { + const text = 'رقم ١٢٣، حسناً؟' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('handles supplementary-plane RTL (Adlam) via surrogate pairs', () => { + const text = '𞤀𞤣𞤤𞤢𞤥' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('breaks runs on CJK and emoji above the scan floor', () => { + const text = 'مرحبا漢بكم' + expect(findRtlJoinRanges(text)).toEqual([ + [0, 5], + [6, 9] + ]) + }) + + it('joins Hebrew words with niqqud points', () => { + const text = 'שָׁלוֹם עוֹלָם' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + // Escapes, not literals: these controls are invisible in source. + const ZWNJ = '\u200c' + const ZWJ = '\u200d' + const RLM = '\u200f' + const LRM = '\u200e' + + it('tunnels through ZWNJ inside a Persian word without splitting the run', () => { + // می‌خواهم — splitting at the ZWNJ would render the word halves in + // swapped visual order. + const text = `می${ZWNJ}خواهم` + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('tunnels through ZWJ and RLM inside an RTL run', () => { + const zwjText = `مر${ZWJ}حب` + expect(findRtlJoinRanges(zwjText)).toEqual([[0, zwjText.length]]) + const rlmText = `سلام${RLM}عليكم` + expect(findRtlJoinRanges(rlmText)).toEqual([[0, rlmText.length]]) + }) + + it('excludes a trailing ZWNJ from the joined range', () => { + expect(findRtlJoinRanges(`مرحبا${ZWNJ}`)).toEqual([[0, 5]]) + }) + + it('does not let ZWNJ start a run or bridge into LTR text', () => { + expect(findRtlJoinRanges(`${ZWNJ}abc`)).toEqual([]) + expect(findRtlJoinRanges(`مرحبا${ZWNJ}abc`)).toEqual([[0, 5]]) + }) + + it('still breaks the run on LRM (strong LTR)', () => { + expect(findRtlJoinRanges(`مرحبا${LRM}بكم`)).toEqual([ + [0, 5], + [6, 9] + ]) + }) + + it('treats ALM as transparent like RLM', () => { + const ALM = '\u061c' + expect(findRtlJoinRanges(`${ALM}${ALM}`)).toEqual([]) + // An isolated letter stays isolated even with a leading direction mark. + expect(findRtlJoinRanges(`${ALM}م`)).toEqual([]) + expect(findRtlJoinRanges(`مرحبا${ALM}`)).toEqual([[0, 5]]) + const text = `مرحبا${ALM}بكم` + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('does not open a run at orphan combining marks after an LTR base', () => { + // Marks render inside their base's cell; a run opened mid-cell maps to an + // empty joined cell range that blanks the following glyph in WebGL. + expect(findRtlJoinRanges('a\u064b\u0651b')).toEqual([]) + expect(findRtlJoinRanges('x\u05b0\u05b1y')).toEqual([]) + }) + + it('lets combining marks extend a run opened by a spacing RTL letter', () => { + const text = 'منَّ هنا' + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) + + it('does not open a run at zero-width Cf controls after an LTR base', () => { + // BOM/ZWNBSP and Arabic number signs are width-0 in xterm, so like + // combining marks a run opened there maps to an empty joined cell range + // that blanks the following glyph in WebGL. + const BOM = '\ufeff' + expect(findRtlJoinRanges(`a${BOM}${BOM}b`)).toEqual([]) + expect(findRtlJoinRanges('x\u0600\u0602y')).toEqual([]) + expect(findRtlJoinRanges(`a${BOM}\u0651b`)).toEqual([]) + }) + + it('keeps zero-width Cf controls from opening or counting an RTL run', () => { + const BOM = '\ufeff' + // A single letter behind a BOM stays isolated (BOM neither opens nor counts). + expect(findRtlJoinRanges(`${BOM}م`)).toEqual([]) + // BOM inside a word does not break the run. + const text = `مرحبا${BOM}بكم` + expect(findRtlJoinRanges(text)).toEqual([[0, text.length]]) + }) +}) + +describe('registerArabicShapingJoiner', () => { + function createJoinerHost(): { + terminal: Parameters[0] + getRegistered: () => ((text: string) => [number, number][]) | null + getDeregistered: () => number | null + } { + let registered: ((text: string) => [number, number][]) | null = null + let deregistered: number | null = null + return { + terminal: { + registerCharacterJoiner(handler: (text: string) => [number, number][]): number { + registered = handler + return 7 + }, + deregisterCharacterJoiner(joinerId: number): void { + deregistered = joinerId + } + }, + getRegistered: () => registered, + getDeregistered: () => deregistered + } + } + + it('registers a joining handler and returns a cleanup that deregisters it', () => { + const host = createJoinerHost() + const cleanup = registerArabicShapingJoiner(host.terminal, () => true) + const text = 'مرحبا' + expect(host.getRegistered()!(text)).toEqual([[0, text.length]]) + expect(host.getDeregistered()).toBeNull() + + // terminal.dispose() does not deregister joiners, so cleanup must. + cleanup() + expect(host.getDeregistered()).toBe(7) + }) + + it('returns no ranges while shaping is inactive (DOM renderer misrenders joined spans)', () => { + const host = createJoinerHost() + let webglLive = false + registerArabicShapingJoiner(host.terminal, () => webglLive) + const handler = host.getRegistered()! + + const inactive = handler('مرحبا') + expect(inactive).toEqual([]) + // Fresh array each call — xterm mutates the handler's result in place. + expect(handler('مرحبا')).not.toBe(inactive) + + webglLive = true + expect(handler('مرحبا')).toEqual([[0, 5]]) + }) +}) diff --git a/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts new file mode 100644 index 00000000000..5b9b5f8dc95 --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-arabic-shaping-joiner.ts @@ -0,0 +1,187 @@ +import type { Terminal } from '@xterm/xterm' + +// Why: xterm draws every cell's glyph in isolation, so Arabic output shows +// disconnected letterforms in logical (reversed) order — upstream has no +// BiDi/shaping support (xtermjs/xterm.js#701, Orca #5262). Joining each RTL +// run into one cell range makes both renderers (WebGL atlas, DOM row factory) +// draw the run as a single string, letting the browser apply contextual +// shaping and BiDi ordering inside the run's grid-aligned cell box. The +// terminal buffer and PTY stream are untouched, and xterm itself un-joins any +// range that holds the cursor or a partially selected span, so cursor +// visibility and selection stay cell-accurate. + +// Every strong-RTL script block sits at or above U+0590, so plain ASCII/Latin +// segments bail out with a single charCodeAt sweep and no per-char decode. +const RTL_SCAN_FLOOR = 0x0590 + +export function isStrongRtlCodePoint(codePoint: number): boolean { + return ( + // Hebrew, Arabic, Syriac, Arabic Sup, Thaana, NKo, Samaritan, Mandaic, + // Syriac Sup, Arabic Extended-B/A — one contiguous strong-RTL span. + (codePoint >= 0x0590 && codePoint <= 0x08ff) || + // Hebrew + Arabic presentation forms (legacy shaped codepoints). + (codePoint >= 0xfb1d && codePoint <= 0xfdff) || + (codePoint >= 0xfe70 && codePoint <= 0xfeff) || + // Historic RTL scripts (Phoenician, Nabataean, …). + (codePoint >= 0x10800 && codePoint <= 0x10fff) || + // Mende Kikakui, Adlam, Arabic Mathematical symbols. + (codePoint >= 0x1e800 && codePoint <= 0x1eeff) + ) +} + +// Neutral characters may sit inside an RTL run (so a multi-word phrase joins +// as one unit and keeps right-to-left word order) but never start or end one: +// ASCII space/digits/punctuation and NBSP. ASCII letters are strong LTR and +// always break a run so paths like `test.txt` never get pulled into one. +function isRunNeutralCharCode(charCode: number): boolean { + if (charCode < 0x20) { + return false + } + if (charCode <= 0x7e) { + const isAsciiLetter = + (charCode >= 0x41 && charCode <= 0x5a) || (charCode >= 0x61 && charCode <= 0x7a) + return !isAsciiLetter + } + return charCode === 0xa0 +} + +// ZWNJ/ZWJ shape within a word (mandatory in Persian/Kurdish orthography) and +// RLM/ALM assert RTL context — breaking the run on them would split one word +// into two joined chunks laid out in swapped visual order. Transparent: never +// opens, closes, extends, or counts toward a run. LRM (U+200E) is strong LTR +// and intentionally still breaks the run. +// The zero-width Cf controls inside the RTL blocks (Arabic number signs +// U+0600–0605, end of ayah U+06DD, Syriac abbreviation mark U+070F, disputed +// end of ayah U+08E2) and BOM/ZWNBSP U+FEFF are width-0 in xterm, so like +// combining marks they must never open or count a run (an empty joined cell +// range blanks the following glyph in WebGL) — and they have no shape to join. +function isRtlRunTransparentCodePoint(codePoint: number): boolean { + return ( + codePoint === 0x200c || + codePoint === 0x200d || + codePoint === 0x200f || + codePoint === 0x061c || + (codePoint >= 0x0600 && codePoint <= 0x0605) || + codePoint === 0x06dd || + codePoint === 0x070f || + codePoint === 0x08e2 || + codePoint === 0xfeff + ) +} + +// Why: a combining mark renders inside its base's cell, so a run opened at a +// mark starts mid-cell — xterm rounds that to an empty joined cell range and +// the WebGL renderer then draws an empty glyph over the next character. Marks +// may extend and count only after a spacing RTL letter opened the run. +const COMBINING_MARK = /\p{Mn}/u +function canOpenRtlRun(codePoint: number): boolean { + return !COMBINING_MARK.test(String.fromCodePoint(codePoint)) +} + +/** + * Character-joiner handler for xterm's registerCharacterJoiner API. Receives + * one attribute-homogeneous segment of a row and returns [start, end) string + * ranges that should render as single joined units. + * + * A run spans from the first strong-RTL code point to the last one of a + * cluster, tunneling through neutral characters between RTL words. Runs with + * fewer than two RTL code points are skipped: an isolated Arabic letter + * already renders in its correct (isolated) form cell-by-cell. + * + * Known upstream limitation (affects every joiner, ligatures too): a + * standalone width-0 cell (e.g. RLM at line start) makes xterm's + * CharacterJoinerService skip the cell without advancing its string index, + * shifting the run's cell range by one — fix belongs upstream in xterm.js. + * + * Known upstream limitation #2: the WebGL renderer un-joins a range for the + * cursor or a partial selection but not for decorations, so a search-match + * highlight inside a joined run renders all-or-nothing — the whole run when + * the match covers the run's first cell, otherwise not at all. Same class as + * ligatures today, extended here to phrase-length runs. + */ +export function findRtlJoinRanges(text: string): [number, number][] { + const length = text.length + let i = 0 + for (; i < length; i++) { + if (text.charCodeAt(i) >= RTL_SCAN_FLOOR) { + break + } + } + // Why: xterm merges other joiners' results into the returned array in + // place, so this must be a fresh array on every call — never a shared + // constant. The non-RTL fast path above keeps the allocation the only cost. + const ranges: [number, number][] = [] + if (i === length) { + return ranges + } + + let runStart = -1 + let runEnd = -1 + let runRtlCount = 0 + const closeRun = (): void => { + if (runStart !== -1 && runRtlCount >= 2) { + ranges.push([runStart, runEnd]) + } + runStart = -1 + runRtlCount = 0 + } + + for (; i < length; i++) { + const unit = text.charCodeAt(i) + if (unit < RTL_SCAN_FLOOR) { + if (runStart !== -1 && !isRunNeutralCharCode(unit)) { + closeRun() + } + continue + } + let codePoint = unit + let codeUnitLength = 1 + if (unit >= 0xd800 && unit <= 0xdbff && i + 1 < length) { + const low = text.charCodeAt(i + 1) + if (low >= 0xdc00 && low <= 0xdfff) { + codePoint = (unit - 0xd800) * 0x400 + (low - 0xdc00) + 0x10000 + codeUnitLength = 2 + } + } + if (isRtlRunTransparentCodePoint(codePoint)) { + // Run-transparent format controls: skip without opening or closing. + } else if (isStrongRtlCodePoint(codePoint)) { + if (runStart !== -1 || canOpenRtlRun(codePoint)) { + if (runStart === -1) { + runStart = i + } + runEnd = i + codeUnitLength + runRtlCount++ + } + } else if (runStart !== -1) { + // Non-RTL above the floor (box drawing, CJK, emoji, …) breaks the run + // so TUI borders and East Asian text keep per-cell rendering. + closeRun() + } + i += codeUnitLength - 1 + } + closeRun() + return ranges +} + +/** Register the RTL shaping joiner on a terminal. Returns a cleanup that + * deregisters the joiner — `Terminal.dispose()` does not remove registered + * character joiners, so disposePane() must call this to avoid leaking the + * registration (xtermjs/xterm.js#3289). */ +export function registerArabicShapingJoiner( + terminal: Pick, + isShapingActive: () => boolean +): () => void { + // Why: the DOM renderer sizes a joined span with one letter-spacing value + // that the browser applies after every character, so a joined run whose + // shaped width differs from its cell budget blows out the whole row's grid + // alignment. Join only while the WebGL renderer is live (checked per render + // call, so context-loss/GPU-setting fallbacks revert to per-cell rendering + // on their own refresh); DOM-rendered panes keep xterm's unshaped default. + const joinerId = terminal.registerCharacterJoiner((text) => + isShapingActive() ? findRtlJoinRanges(text) : [] + ) + return () => { + terminal.deregisterCharacterJoiner(joinerId) + } +} From 9e23b8963beab9d9c96299971ce9b9e769be19f9 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:44:43 -0700 Subject: [PATCH 03/36] fix(agent-status): show waiting for Claude AskUserQuestion, stop stale-title worktree spinners (#7852) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude's AskUserQuestion tool is auto-allowed, so it emits a PreToolUse (not PermissionRequest) while blocked on a human answer. normalizeClaudeEvent mapped that to `working`, so the sidebar showed a spinner that decayed to grey while the question sat. Map the auto-allowed AskUserQuestion PreToolUse to `waiting` instead, mirroring the Kimi/OpenCode handling, so the row and worktree dots read amber "Waiting for input". Separately, a frozen braille-spinner title left by an exited agent (e.g. "⠐ Review branch for regressions" over a shell prompt) kept classifyTitleActivity returning `working`, spinning the worktree dot forever with 0 agents — the row builder rejects the same unattributable title, so no agent row is shown. Gate tabHasStatus's title-derived working/permission on the same agent attribution the row builder uses (resolveAgentTypeFromTerminalTitle), so a title only spins the dot when it would also show a row. Hook-driven status (hasLiveWorking/ hasPermission) is untouched. --- src/renderer/src/lib/worktree-status.test.ts | 63 ++++++++++++++++++++ src/renderer/src/lib/worktree-status.ts | 15 ++++- src/shared/agent-hook-listener.test.ts | 61 +++++++++++++++++++ src/shared/agent-hook-listener.ts | 14 ++++- 4 files changed, 148 insertions(+), 5 deletions(-) diff --git a/src/renderer/src/lib/worktree-status.test.ts b/src/renderer/src/lib/worktree-status.test.ts index 85176224614..e3b88ee082d 100644 --- a/src/renderer/src/lib/worktree-status.test.ts +++ b/src/renderer/src/lib/worktree-status.test.ts @@ -93,6 +93,39 @@ describe('getWorktreeStatus', () => { expect(status).toBe('active') }) + + // Why: an exited agent can leave a frozen braille-spinner frame in the + // title forever (reproduced live: '⠐ Review branch for regressions' over a + // plain shell prompt). The row builder rejects that title as a Claude row + // (no explicit "claude" token), so the sidebar shows 0 agents — the worktree + // dot must agree instead of spinning on the unattributable title alone. + it('does not spin on a bare braille-spinner tab title that yields no agent row', () => { + const status = getWorktreeStatus( + [{ id: 'tab-1', title: '⠐ Review branch for regressions' }], + [], + livePtyMap('tab-1') + ) + + expect(status).toBe('active') + }) + + it('does not spin on a bare braille-spinner pane title that yields no agent row', () => { + const status = getWorktreeStatus([{ id: 'tab-1', title: 'bash' }], [], livePtyMap('tab-1'), { + 'tab-1': { 0: '⠐ Review branch for regressions', 1: 'bash' } + }) + + expect(status).toBe('active') + }) + + it('still spins on an agent-attributable braille-spinner title', () => { + const status = getWorktreeStatus( + [{ id: 'tab-1', title: '⠹ codex fix flaky test' }], + [], + livePtyMap('tab-1') + ) + + expect(status).toBe('working') + }) }) describe('resolveWorktreeStatus', () => { @@ -330,6 +363,36 @@ describe('resolveWorktreeStatus', () => { expect(status).toBe('done') }) + it('keeps a hook-backed working agent spinning despite an unattributable stale title', () => { + // Why: hook-managed agents drive the dot via hasLiveWorking, not the title + // heuristic — the attribution gate must not regress them. + const status = resolveWorktreeStatus({ + tabs: [{ id: 'tab-1', title: '⠐ Review branch for regressions' }], + browserTabs: [], + ptyIdsByTabId: livePtyMap('tab-1'), + hasPermission: false, + hasLiveWorking: true, + hasLiveDone: false, + hasRetainedDone: false + }) + + expect(status).toBe('working') + }) + + it('resolves a stale unattributable spinner title with no hook entry to active, not working', () => { + const status = resolveWorktreeStatus({ + tabs: [{ id: 'tab-1', title: '⠐ Review branch for regressions' }], + browserTabs: [], + ptyIdsByTabId: livePtyMap('tab-1'), + hasPermission: false, + hasLiveWorking: false, + hasLiveDone: false, + hasRetainedDone: false + }) + + expect(status).toBe('active') + }) + it('falls through to active heuristic when nothing else applies', () => { const status = resolveWorktreeStatus({ tabs: [{ id: 'tab-1', title: 'bash' }], diff --git a/src/renderer/src/lib/worktree-status.ts b/src/renderer/src/lib/worktree-status.ts index 0c3c87bf062..c7509767cba 100644 --- a/src/renderer/src/lib/worktree-status.ts +++ b/src/renderer/src/lib/worktree-status.ts @@ -1,3 +1,4 @@ +import { resolveAgentTypeFromTerminalTitle } from '@/components/sidebar/worktree-title-derived-agent-rows' import { classifyTitleActivity } from '@/lib/pane-agent-evidence' import { tabHasLivePty } from '@/lib/tab-has-live-pty' import { resolveRuntimePaneTitleLeafIdFromRoot } from '@/lib/runtime-pane-title-leaf-id' @@ -91,7 +92,7 @@ function tabHasStatus( ) { continue } - if (classifyTitleActivity(title) === status) { + if (classifyTitleActivity(title) === status && titleStatusIsAgentAttributable(title)) { return true } } @@ -103,7 +104,17 @@ function tabHasStatus( if (agentStatusPaneIds && agentStatusPaneIds.size > 0) { return false } - return classifyTitleActivity(tab.title) === status + return classifyTitleActivity(tab.title) === status && titleStatusIsAgentAttributable(tab.title) +} + +// Why: the title heuristic is only a fallback — hook-managed agents promote the +// worktree via hasLiveWorking/hasPermission in resolveWorktreeStatus, not here. +// A bare braille-spinner title (e.g. an exited agent's never-cleared "⠐ task" +// frame) classifies as working yet produces no sidebar agent row, spinning the +// worktree forever with "0 agents". Require the same agent attribution the row +// builder uses, so a title only spins the dot when it would also show a row. +function titleStatusIsAgentAttributable(title: string): boolean { + return resolveAgentTypeFromTerminalTitle(title) !== null } export function getWorktreeStatusLabel(status: WorktreeStatus): string { diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index cedc0efda32..3d4baeac3c4 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -170,6 +170,9 @@ describe('shared agent-hook-listener', () => { ) expect(event?.payload.toolName).toBe('AskUserQuestion') + // Why: the auto-allowed AskUserQuestion PreToolUse is a human-input boundary, + // so it must read as waiting (amber attention) rather than a working spinner. + expect(event?.payload.state).toBe('waiting') const expected = JSON.stringify(questions) expect(event?.payload.interactivePrompt).toBe(expected) // Why: must NOT be truncated to the 160-char toolInput preview cap. @@ -177,6 +180,64 @@ describe('shared agent-hook-listener', () => { expect(event?.payload.interactivePrompt!.length).toBe(expected.length) }) + it('maps Claude AskUserQuestion PreToolUse to waiting, then back to working on answer', () => { + const question = normalizeHookPayload( + state, + 'claude', + { + paneKey: PANE_KEY, + payload: { + hook_event_name: 'PreToolUse', + tool_name: 'AskUserQuestion', + tool_input: { questions: [{ question: 'Pick', options: ['a', 'b'] }] } + } + }, + 'production' + ) + const answered = normalizeHookPayload( + state, + 'claude', + { + paneKey: PANE_KEY, + payload: { + hook_event_name: 'PostToolUse', + tool_name: 'AskUserQuestion', + tool_response: { selected: ['a'] } + } + }, + 'production' + ) + + expect(question?.payload).toMatchObject({ + agentType: 'claude', + state: 'waiting', + toolName: 'AskUserQuestion' + }) + expect(answered?.payload).toMatchObject({ + agentType: 'claude', + state: 'working', + toolName: 'AskUserQuestion' + }) + }) + + it('keeps a normal Claude PreToolUse tool call as working', () => { + const event = normalizeHookPayload( + state, + 'claude', + { + paneKey: PANE_KEY, + payload: { + hook_event_name: 'PreToolUse', + tool_name: 'Bash', + tool_input: { command: 'ls' } + } + }, + 'production' + ) + expect(event?.payload.state).toBe('working') + expect(event?.payload.toolName).toBe('Bash') + }) + it('leaves interactivePrompt undefined for a normal tool call', () => { const event = normalizeHookPayload( state, diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index e73d2ae9dc9..1619e4bd7ec 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -2162,13 +2162,21 @@ function normalizeClaudeEvent( paneKey: string, hookPayload: Record ): ParsedAgentStatusPayload | null { + // Why: Claude's AskUserQuestion tool is auto-allowed, so it emits PreToolUse + // (not PermissionRequest) while blocked on a human answer — Claude posts a + // Notification instead of PermissionRequest, and Orca does not register the + // Notification hook. Treat that PreToolUse as waiting so the sidebar shows the + // amber attention state instead of a working spinner that decays to grey while + // the question sits unanswered. Mirrors normalizeKimiEvent's handling. + const isAskUserQuestion = + eventName === 'PreToolUse' && isAskUserQuestionTool(readString(hookPayload, 'tool_name')) const stateName = eventName === 'UserPromptSubmit' || - eventName === 'PreToolUse' || eventName === 'PostToolUse' || - eventName === 'PostToolUseFailure' + eventName === 'PostToolUseFailure' || + (eventName === 'PreToolUse' && !isAskUserQuestion) ? 'working' - : eventName === 'PermissionRequest' + : eventName === 'PermissionRequest' || isAskUserQuestion ? 'waiting' : eventName === 'Stop' || eventName === 'StopFailure' ? 'done' From 29cbe2d8cb9c3b17dcd9241dfe279c9e3f20d5f2 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:44:54 -0700 Subject: [PATCH 04/36] Show empty sessions by default in AI Vault Panel (#7853) * Show empty sessions by default in AI Vault Panel Change the default state of `hideEmptySessions` from true to false, and update the active filter count calculation to treat false as the new default. * Centralize AI Vault view defaults and fix reset state alignment Consolidate the default values for sorting, grouping, and hiding empty sessions into a single place. This prevents inconsistencies between the initial state, the badge count adjustment checks, and the "Reset" view action, where the reset previously toggled hideEmptySessions to true contrary to the initial false value. * Count agent adjustments by membership instead of list length Ensure that swapping one agent for another (which keeps the total array length the same) is correctly identified as a deviation from the default of having all agents enabled. --- .../components/right-sidebar/AiVaultPanel.tsx | 30 ++++---- .../ai-vault-view-defaults.test.ts | 72 +++++++++++++++++++ .../right-sidebar/ai-vault-view-defaults.ts | 29 ++++++++ 3 files changed, 119 insertions(+), 12 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/ai-vault-view-defaults.test.ts create mode 100644 src/renderer/src/components/right-sidebar/ai-vault-view-defaults.ts diff --git a/src/renderer/src/components/right-sidebar/AiVaultPanel.tsx b/src/renderer/src/components/right-sidebar/AiVaultPanel.tsx index 4a6bccec9db..c03192f4054 100644 --- a/src/renderer/src/components/right-sidebar/AiVaultPanel.tsx +++ b/src/renderer/src/components/right-sidebar/AiVaultPanel.tsx @@ -20,6 +20,12 @@ import { getRestorableAiVaultScope, normalizeAiVaultScopeForContext } from './ai-vault-scope-state' +import { + countAiVaultViewAdjustments, + DEFAULT_AI_VAULT_GROUP, + DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS, + DEFAULT_AI_VAULT_SORT +} from './ai-vault-view-defaults' import { buildAiVaultProjectContext } from './ai-vault-session-projects' import { resolveAiVaultSessionResumeActions, @@ -68,9 +74,9 @@ export default function AiVaultPanel(): React.JSX.Element { useAiVaultOriginalPaneActions() const [query, setQuery] = useState('') const [scope, setScope] = useState(DEFAULT_AI_VAULT_SCOPE) - const [sort, setSort] = useState('updated') - const [group, setGroup] = useState('project') - const [hideEmptySessions, setHideEmptySessions] = useState(true) + const [sort, setSort] = useState(DEFAULT_AI_VAULT_SORT) + const [group, setGroup] = useState(DEFAULT_AI_VAULT_GROUP) + const [hideEmptySessions, setHideEmptySessions] = useState(DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS) const [agents, setAgents] = useState([...AI_VAULT_AGENTS]) const [collapsedGroups, setCollapsedGroups] = useState>(() => new Set()) const userChangedScopeRef = useRef(false) @@ -155,12 +161,12 @@ export default function AiVaultPanel(): React.JSX.Element { targetState: resumeTargetState, agentCmdOverrides }) - const hasAllAgentsSelected = agents.length === AI_VAULT_AGENTS.length - const viewAdjustmentCount = - (hasAllAgentsSelected ? 0 : 1) + - (sort === 'updated' ? 0 : 1) + - (group === 'project' ? 0 : 1) + - (hideEmptySessions ? 0 : 1) + const viewAdjustmentCount = countAiVaultViewAdjustments({ + agents, + sort, + group, + hideEmptySessions + }) // Workspace is the preferred default, but unavailable context still falls back to All. useEffect(() => { @@ -286,9 +292,9 @@ export default function AiVaultPanel(): React.JSX.Element { const resetViewOptions = useCallback(() => { setAgents([...AI_VAULT_AGENTS]) - setSort('updated') - setGroup('project') - setHideEmptySessions(true) + setSort(DEFAULT_AI_VAULT_SORT) + setGroup(DEFAULT_AI_VAULT_GROUP) + setHideEmptySessions(DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS) }, []) const handleScopeChange = useCallback((nextScope: AiVaultScope) => { diff --git a/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.test.ts b/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.test.ts new file mode 100644 index 00000000000..e45bdd38635 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from 'vitest' +import { AI_VAULT_AGENTS } from '../../../../shared/ai-vault-types' +import { + countAiVaultViewAdjustments, + DEFAULT_AI_VAULT_GROUP, + DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS, + DEFAULT_AI_VAULT_SORT +} from './ai-vault-view-defaults' + +describe('ai-vault-view-defaults', () => { + it('shows empty sessions by default', () => { + expect(DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS).toBe(false) + }) + + it('counts zero adjustments for the default view', () => { + expect( + countAiVaultViewAdjustments({ + agents: [...AI_VAULT_AGENTS], + sort: DEFAULT_AI_VAULT_SORT, + group: DEFAULT_AI_VAULT_GROUP, + hideEmptySessions: DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS + }) + ).toBe(0) + }) + + it('treats hiding empty sessions as an adjustment from the new default', () => { + expect( + countAiVaultViewAdjustments({ + agents: [...AI_VAULT_AGENTS], + sort: DEFAULT_AI_VAULT_SORT, + group: DEFAULT_AI_VAULT_GROUP, + hideEmptySessions: true + }) + ).toBe(1) + }) + + it('does not count showing empty sessions as an adjustment', () => { + expect( + countAiVaultViewAdjustments({ + agents: [...AI_VAULT_AGENTS], + sort: DEFAULT_AI_VAULT_SORT, + group: DEFAULT_AI_VAULT_GROUP, + hideEmptySessions: false + }) + ).toBe(0) + }) + + it('counts an equal-length agent swap as an adjustment', () => { + // A swap keeps the array length but drops a default agent (here the first) for a duplicate. + const swapped = [...AI_VAULT_AGENTS.slice(1), AI_VAULT_AGENTS[1]] + expect(swapped).toHaveLength(AI_VAULT_AGENTS.length) + expect( + countAiVaultViewAdjustments({ + agents: swapped, + sort: DEFAULT_AI_VAULT_SORT, + group: DEFAULT_AI_VAULT_GROUP, + hideEmptySessions: DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS + }) + ).toBe(1) + }) + + it('counts other non-default view options independently', () => { + expect( + countAiVaultViewAdjustments({ + agents: ['claude'], + sort: 'created', + group: 'agent', + hideEmptySessions: true + }) + ).toBe(4) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.ts b/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.ts new file mode 100644 index 00000000000..9b1bfd8b27c --- /dev/null +++ b/src/renderer/src/components/right-sidebar/ai-vault-view-defaults.ts @@ -0,0 +1,29 @@ +import { + AI_VAULT_AGENTS, + type AiVaultAgent, + type AiVaultGroup, + type AiVaultSort +} from '../../../../shared/ai-vault-types' + +// Why: hide-empty used to default true; keep initial state, badge count, and Reset view +// on one constant so a default flip cannot leave Reset pointing at the old value. +export const DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS = false +export const DEFAULT_AI_VAULT_SORT: AiVaultSort = 'updated' +export const DEFAULT_AI_VAULT_GROUP: AiVaultGroup = 'project' + +export function countAiVaultViewAdjustments(options: { + agents: readonly AiVaultAgent[] + sort: AiVaultSort + group: AiVaultGroup + hideEmptySessions: boolean +}): number { + // Why: count by membership, not length — an agent swap keeps the array length but + // still deviates from the default of every agent enabled. + const allAgentsEnabled = AI_VAULT_AGENTS.every((agent) => options.agents.includes(agent)) + return ( + (allAgentsEnabled ? 0 : 1) + + (options.sort === DEFAULT_AI_VAULT_SORT ? 0 : 1) + + (options.group === DEFAULT_AI_VAULT_GROUP ? 0 : 1) + + (options.hideEmptySessions === DEFAULT_AI_VAULT_HIDE_EMPTY_SESSIONS ? 0 : 1) + ) +} From 2af8fb886fcb067a6bbd638e73c1125fb43f6122 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:57:07 -0700 Subject: [PATCH 05/36] Collapse Grok rotating working titles to stable label (#7863) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grok Build's working OSC titles interpolate a rotating status or tool phrase between the spinner and its name, causing tab and sidebar titles to fluctuate rapidly. Collapse these working frames to a stable "⠋ Grok" label, while leaving idle and session titles untouched. --- src/renderer/src/lib/agent-status.test.ts | 29 ++++++++++++++++++++ src/shared/terminal-title-agent-type.test.ts | 29 +++++++++++++++++++- src/shared/terminal-title-agent-type.ts | 18 ++++++++++++ src/shared/terminal-title-display.ts | 9 ++++++ 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/lib/agent-status.test.ts b/src/renderer/src/lib/agent-status.test.ts index 3a9215278ee..9f2a7847817 100644 --- a/src/renderer/src/lib/agent-status.test.ts +++ b/src/renderer/src/lib/agent-status.test.ts @@ -390,6 +390,35 @@ describe('normalizeTerminalTitle', () => { expect(normalizeTerminalTitle('bash')).toBe('bash') }) + it('collapses Grok rotating working titles to one stable label', () => { + // Grok interpolates a rotating status/tool phrase between the spinner and its + // name, so each frame is a distinct title; they must all fold to one label. + expect(normalizeTerminalTitle('⠋ - Waiting for response… - grok')).toBe('⠋ Grok') + expect(normalizeTerminalTitle('⠴ - Thinking - grok')).toBe('⠋ Grok') + expect(normalizeTerminalTitle('⠙ - Responding - grok')).toBe('⠋ Grok') + expect(normalizeTerminalTitle('⠦ - Sleep 2s then echo hello-from-grok… - grok')).toBe('⠋ Grok') + expect( + normalizeTerminalTitle('⠹ - Waiting for response… - Execute Shell Command sleep 2 - grok') + ).toBe('⠋ Grok') + // Idempotent: re-normalizing our own collapsed label stays stable. + expect(normalizeTerminalTitle('⠋ Grok')).toBe('⠋ Grok') + }) + + it('leaves Grok idle and session titles unchanged', () => { + // No spinner → not a working frame; the meaningful final title still shows. + expect(normalizeTerminalTitle('grok')).toBe('grok') + expect(normalizeTerminalTitle('Fix the auth bug - grok')).toBe('Fix the auth bug - grok') + // Names "grok" mid-title but ends with another agent → not a Grok frame. + expect(normalizeTerminalTitle('⠋ debugging grok - claude')).toBe('⠋ debugging grok - claude') + // Claude/Codex task text ending in "grok" must not collapse — only the + // Grok Build identity suffix " - grok" is the rotating-frame signal. + expect(normalizeTerminalTitle('⠋ wire up grok')).toBe('⠋ wire up grok') + expect(normalizeTerminalTitle('⠋ Codex is thinking about grok')).toBe( + '⠋ Codex is thinking about grok' + ) + expect(normalizeTerminalTitle('⠋ support for Grok')).toBe('⠋ support for Grok') + }) + it('collapses Pi spinner and idle titles to stable labels', () => { expect(normalizeTerminalTitle('⠋ π - my-project')).toBe('⠋ Pi') expect(normalizeTerminalTitle('π - my-project')).toBe('Pi') diff --git a/src/shared/terminal-title-agent-type.test.ts b/src/shared/terminal-title-agent-type.test.ts index a2baffe2cab..f3a0de9ec25 100644 --- a/src/shared/terminal-title-agent-type.test.ts +++ b/src/shared/terminal-title-agent-type.test.ts @@ -1,5 +1,32 @@ import { describe, expect, it } from 'vitest' -import { resolveExplicitTerminalTitleAgentType } from './terminal-title-agent-type' +import { + isGrokRotatingWorkingTitle, + resolveExplicitTerminalTitleAgentType +} from './terminal-title-agent-type' + +describe('isGrokRotatingWorkingTitle', () => { + it('matches Grok working frames regardless of the rotating middle text', () => { + expect(isGrokRotatingWorkingTitle('⠋ - Waiting for response… - grok')).toBe(true) + expect(isGrokRotatingWorkingTitle('⠴ - Thinking - grok')).toBe(true) + expect(isGrokRotatingWorkingTitle('⠦ - Sleep 2s then echo hello… - grok')).toBe(true) + // Collapsed/stable form must stay matched so re-normalization is idempotent. + expect(isGrokRotatingWorkingTitle('⠋ grok')).toBe(true) + expect(isGrokRotatingWorkingTitle('⠋ Grok')).toBe(true) + }) + + it('ignores non-working, non-Grok, and lookalike titles', () => { + expect(isGrokRotatingWorkingTitle('grok')).toBe(false) // idle bare name, no spinner + expect(isGrokRotatingWorkingTitle('Fix the auth bug - grok')).toBe(false) // session title, no spinner + expect(isGrokRotatingWorkingTitle('⠋ debugging grok - claude')).toBe(false) // trailing name is another agent + expect(isGrokRotatingWorkingTitle('⠋ ~/grok-scratch/ready')).toBe(false) // path fragment, not a trailing token + expect(isGrokRotatingWorkingTitle('⠋ grokking the plan')).toBe(false) // "grok" not a whole trailing token + expect(isGrokRotatingWorkingTitle('⠋ Codex')).toBe(false) + // Task text ending in "grok" is not the Grok Build identity suffix " - grok". + expect(isGrokRotatingWorkingTitle('⠋ wire up grok')).toBe(false) + expect(isGrokRotatingWorkingTitle('⠋ Codex is thinking about grok')).toBe(false) + expect(isGrokRotatingWorkingTitle('⠋ support for Grok')).toBe(false) + }) +}) describe('resolveExplicitTerminalTitleAgentType', () => { it('maps explicit product-name titles to their TuiAgent id', () => { diff --git a/src/shared/terminal-title-agent-type.ts b/src/shared/terminal-title-agent-type.ts index 21c206dff20..b0c0a82a544 100644 --- a/src/shared/terminal-title-agent-type.ts +++ b/src/shared/terminal-title-agent-type.ts @@ -51,6 +51,24 @@ export function isPiTerminalTitle(title: string): boolean { return isLegacyPiCompatibleTitle(title) && !containsBrailleSpinner(title) } +// Why: Grok Build's working OSC titles interpolate a rotating status/tool phrase +// between the spinner and its name — "⠋ - Waiting for response… - grok", +// "⠴ - Thinking - grok", "⠦ - Sleep 2s… - grok" — so every frame is a distinct +// title that flips the tab and sidebar labels. Require the identity suffix +// " - grok" (not arbitrary task text ending in "grok") so Claude/Codex titles +// like "⠋ wire up grok" stay intact. Spinner marks working; no-spinner session +// titles ("Fix the auth bug - grok") pass through. The bare "spinner + grok" +// branch keeps our own collapsed label idempotent under re-normalization. +const GROK_FRAME_IDENTITY_SUFFIX_RE = / - grok\s*$/i +const GROK_COLLAPSED_WORKING_TITLE_RE = /^[\u2800-\u28FF]+\s+grok\s*$/i + +export function isGrokRotatingWorkingTitle(title: string): boolean { + if (!containsBrailleSpinner(title)) { + return false + } + return GROK_FRAME_IDENTITY_SUFFIX_RE.test(title) || GROK_COLLAPSED_WORKING_TITLE_RE.test(title) +} + export function isPiAgentTitle(title: string): boolean { return isLegacyPiCompatibleTitle(title) } diff --git a/src/shared/terminal-title-display.ts b/src/shared/terminal-title-display.ts index a74678e3433..a0f2ad6894c 100644 --- a/src/shared/terminal-title-display.ts +++ b/src/shared/terminal-title-display.ts @@ -10,6 +10,7 @@ import { GEMINI_SILENT_WORKING, GEMINI_WORKING, isGeminiTerminalTitle, + isGrokRotatingWorkingTitle, isPiAgentTitle } from './terminal-title-agent-type' import { @@ -95,5 +96,13 @@ export function normalizeTerminalTitle(title: string): string { } } + // Why: Grok Build interpolates a rotating status/tool phrase between the + // spinner and its name, so its working frames change the title many times per + // turn. Collapse them to one stable label; idle/session titles carry no + // spinner and pass through, so the meaningful final title still shows. + if (isGrokRotatingWorkingTitle(title)) { + return '\u280b Grok' + } + return title } From 070a956a72a6c4ed9b1d77221b7cb735fdafbe12 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:59:28 -0700 Subject: [PATCH 06/36] feat(source-control): add push failure AI recovery (#7826) * feat(source-control): add Fix push failure with AI for pre-push hooks Detect pre-push hook failures separately from auth/transport errors so push toasts and inline messages no longer suggest checking repo access. Mirror the commit-failure recovery flow with a fixPushFailure action, summary panel, details dialog, and agent launch recipe in Settings. Fixes #6497 * feat(source-control): add push failure AI recovery Co-authored-by: dalveytech-vincent --------- Co-authored-by: dalveytech-vincent --- ...source-control-push-failure-ai-recovery.md | 368 +++++++++++++ src/main/git/remote.test.ts | 29 + .../CommitArea.chevron-spinner.test.tsx | 3 + .../CommitArea.generate.test.tsx | 3 + .../CommitArea.primary-icons.test.tsx | 3 + .../right-sidebar/CommitArea.test.tsx | 54 ++ ...rceControl.commit-failure-recovery.test.ts | 4 +- ...ourceControl.push-failure-recovery.test.ts | 46 ++ ...SourceControl.remote-action-errors.test.ts | 36 +- .../right-sidebar/SourceControl.tsx | 508 ++++++------------ .../right-sidebar/push-failure-summary.ts | 6 + .../source-control-action-error.ts | 34 ++ ...e-control-ai-commit-failure-launch.test.ts | 117 +++- ...source-control-ai-commit-failure-launch.ts | 136 +---- .../source-control-ai-controller-types.ts | 1 + .../source-control-ai-prompts.ts | 5 + .../source-control-ai-push-failure-launch.ts | 47 ++ .../source-control-ai-recovery-launch.ts | 178 ++++++ .../source-control-manual-review-url.ts | 4 +- .../source-control-push-recovery.test.ts | 128 +++++ .../source-control-push-recovery.ts | 85 +++ .../source-control-recovery-notice.tsx | 348 ++++++++++++ .../right-sidebar/use-source-control-ai.ts | 81 +-- .../use-source-control-recovery-ai.ts | 139 +++++ .../settings/CommitMessageAiPane.test.tsx | 1 + .../RepositorySourceControlAiSection.test.ts | 1 + .../source-control-action-recipe-options.ts | 4 + src/renderer/src/i18n/locales/en.json | 78 ++- src/renderer/src/i18n/locales/es.json | 78 ++- src/renderer/src/i18n/locales/ja.json | 78 ++- src/renderer/src/i18n/locales/ko.json | 78 ++- src/renderer/src/i18n/locales/zh.json | 78 ++- .../lib/source-control-remote-error.test.ts | 70 +++ .../src/lib/source-control-remote-error.ts | 37 ++ src/renderer/src/store/slices/editor.test.ts | 61 +++ src/renderer/src/store/slices/editor.ts | 19 +- src/shared/git-remote-error.test.ts | 37 ++ src/shared/git-remote-error.ts | 8 + src/shared/source-control-ai-actions.test.ts | 26 + src/shared/source-control-ai-actions.ts | 5 + ...ce-control-commit-failure-agent-command.ts | 18 +- ...urce-control-push-failure-agent-command.ts | 18 + .../source-control-push-failure.test.ts | 136 +++++ src/shared/source-control-push-failure.ts | 272 ++++++++++ ...rce-control-recovery-agent-command.test.ts | 53 ++ .../source-control-recovery-agent-command.ts | 25 + 46 files changed, 2949 insertions(+), 595 deletions(-) create mode 100644 docs/source-control-push-failure-ai-recovery.md create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.push-failure-recovery.test.ts create mode 100644 src/renderer/src/components/right-sidebar/push-failure-summary.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-action-error.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-ai-push-failure-launch.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-ai-recovery-launch.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-push-recovery.test.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-push-recovery.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control-recovery-notice.tsx create mode 100644 src/renderer/src/components/right-sidebar/use-source-control-recovery-ai.ts create mode 100644 src/shared/source-control-push-failure-agent-command.ts create mode 100644 src/shared/source-control-push-failure.test.ts create mode 100644 src/shared/source-control-push-failure.ts create mode 100644 src/shared/source-control-recovery-agent-command.test.ts create mode 100644 src/shared/source-control-recovery-agent-command.ts diff --git a/docs/source-control-push-failure-ai-recovery.md b/docs/source-control-push-failure-ai-recovery.md new file mode 100644 index 00000000000..136b7e0031a --- /dev/null +++ b/docs/source-control-push-failure-ai-recovery.md @@ -0,0 +1,368 @@ +# Source Control Push Failure AI Recovery + +## Problem + +Push, force-push, publish, and sync push-stage failures flow through the shared +remote operation formatter in `src/renderer/src/lib/source-control-remote-error.ts:94` +and the editor-store toast paths in `src/renderer/src/store/slices/editor.ts:3716` +and `src/renderer/src/store/slices/editor.ts:3821`. A local pre-push hook or +lint hook is not an auth, protected-branch, or transport problem, so generic +remote guidance sends the user in the wrong direction. + +PR #7787 proves the behavior is useful, but the prototype needs tightening before +it is maintainable: + +- `src/renderer/src/components/right-sidebar/SourceControl.tsx:1938` and + `src/renderer/src/components/right-sidebar/SourceControl.tsx:5799` repeat + push-failure predicates and feed live branch/file state into a prompt that says + "at failure time". +- `src/renderer/src/components/right-sidebar/use-source-control-recovery-ai.ts:63` + imports push detection again instead of consuming one derived recovery model. +- `src/renderer/src/components/right-sidebar/source-control-ai-commit-failure-launch.ts:24` + and `src/renderer/src/components/right-sidebar/source-control-ai-push-failure-launch.ts:24` + are copied launch flows with action-specific copy. +- `src/shared/source-control-commit-failure-agent-command.ts:6` and + `src/shared/source-control-push-failure-agent-command.ts:6` are identical + command-template builders. +- `src/renderer/src/components/right-sidebar/source-control-fix-split-button.tsx:21` + already contains a reusable split button, while `SourceControl.tsx:6415` + carries a second inline version. +- `src/renderer/src/lib/source-control-remote-error.ts:148` currently treats + any `isSync` error as push-like for hook detection. `syncBranch` calls the + formatter for fetch, upstream-status, pull, and push failures, so the renderer + cannot prove a sync error came from the push stage without an explicit marker. +- `src/renderer/src/components/right-sidebar/SourceControl.push-failure-recovery.test.ts:2` + imports prompt helpers through the large React module instead of shared prompt + modules. + +## Goal + +Add first-class recovery for pre-push hook failures without growing a parallel +push-only subsystem: + +1. Toasts for push, force-push, publish, and sync push-stage failures say + "blocked" for detected pre-push or lint-hook output. +2. Source Control shows a concise Push blocked panel with Details and AI Fix + only for a failure snapshot that is both push-like and hook-like. +3. AI Fix launches the configured `fixPushFailure` Source Control action with a + safe, provider-neutral prompt. +4. Commit-failure recovery and push-failure recovery share launch, + command-template, split-button, and dense error-panel primitives. + +## Non-goals + +- Do not fix server-side pre-receive hooks, hosted CI failures, or provider-side + protected-branch failures from this entry point. +- Do not bypass hooks, add `--no-verify`, push from the launched agent, create a + PR, or assume GitHub-specific terminology. +- Do not add app-wide persistence for this transient failure panel. +- Do not redesign the Source Control panel or the Source Control AI settings + model. +- Do not fold unrelated launch surfaces such as checks recovery into this change + unless a tiny shared helper is already needed for commit/push recovery. + +## Design + +1. Keep push-hook detection and prompt building in shared code. + - `src/shared/source-control-push-failure.ts` owns normalization, bounded + scanning, summary text, Details eligibility, prompt truncation, prompt + rules, and changed-file list bounding. + - Detection must stay conservative: explicit `pre-push`/`prepush`, + `hook declined to push`, hook-runner output in push context, or lint output + in push context. Auth, non-fast-forward, protected branch, pre-receive, + submodule, and generic transport failures stay on the existing remote-error + path. + - Keep the output scan bounded at 64 KiB and the prompt failure-output + section bounded. Also cap prompt file lines to a small constant with an + omitted-count line; do not rely on the git-status cap, which is far too high + for prompt context. + +2. Make remote-error formatting sync-stage aware. + - Extend `RemoteOperationErrorOptions` with an explicit push-stage flag for + sync, for example `isSyncPushStage`. + - Gate push-hook classification on `isPush`, `isForcePush`, `publish`, or + `isSyncPushStage`; do not use bare `isSync` as evidence of a push failure. + - In `syncBranch`, pass the push-stage flag only inside the two inner + `pushRuntimeGit` catch blocks. The outer fetch, upstream-status, and pull + catches still pass `isSync` for Sync-shaped generic copy but must not render + blocked hook copy. + - Carry the same push-stage fact back to the renderer, either by tagging the + thrown `Error` with a narrow exported marker or by wrapping it in a typed + error that preserves the original message/cause. `SourceControl` must set + `syncPushStage` only from that marker, not from `kind === 'sync'` plus + hook-like stderr. + - Keep submodule push messages before hook detection so submodule guidance + remains more specific than AI recovery. + +3. Replace one-off command builders with one shared recovery command builder. + - Add a shared builder that accepts `{ actionId, promptOverride, + commandInputTemplate, basePrompt }` and renders the existing Source Control + AI command template for launch actions. + - Keep compatibility exports for commit and push helpers if nearby tests or + call sites still use those names, but make them wrappers over the generic + builder. + - Move prompt-builder tests to import from shared modules, not + `SourceControl.tsx`. + +4. Capture and derive push recovery state once. + - Extend `SourceControlActionError` with the raw error, a push-stage marker + for sync failures, the branch name at failure time, a bounded status-entry + snapshot at failure time, and a per-worktree sequence token. + - `runRemoteAction` clears the worktree's previous error and increments its + sequence before starting. A catch may write an error only if its sequence + still owns that worktree, so a slow failure cannot overwrite a newer retry + or success. + - Add a focused renderer helper that accepts the captured + `SourceControlActionError` and the current branch name, then returns either + `null` or a model with raw/sanitized detail text, summary, details flag, + kind label, and AI prompt. + - The helper returns `null` unless the operation is `push`, `force_push`, + `publish`, or `sync` with the sync push-stage marker and + `isPushHookFailure(rawError)` passes. + - If the current branch is known and differs from the captured branch, hide or + clear the model instead of launching an agent with wrong branch context. + - Pass that single model to `useSourceControlAi` and `CommitArea`; neither + prop construction nor `CommitArea` should repeat the push-hook predicate. + +5. Use one generic recovery launcher for commit and push failures. + - Replace the separate commit and push launch files with + `source-control-ai-recovery-launch.ts`, or keep the old files as one-line + wrappers if import stability is cheaper. + - The generic launcher owns connection resolution, SSH/local agent discovery, + saved-agent validation, agent-args validation, agent selection, terminal + launch, focus, and success/failure toasts. + - Resolve connection without collapsing local and unresolved states: read + `const worktreeConnectionId = getConnectionId(worktreeId)`, use it when it + is a string or `null`, and only fall back to `sourceRepoConnectionId` when + the worktree lookup returns `undefined`. If the fallback is also + `undefined`, show the workspace-connection error instead of launching + locally. `null` means proven local; a string means SSH/remote. + - Validate saved CLI args before agent detection or terminal creation. On + Windows, keep using PowerShell-safe planning through + `planAgentCliArgsSuffix(..., 'powershell')`. + - Action-specific data is limited to action id, base prompt, empty-prompt + copy, unavailable-agent copy, and success copy. + +6. Keep the recovery hook thin. + - `use-source-control-recovery-ai.ts` builds the commit prompt, accepts the + already-derived push recovery prompt/model, keeps independent loading + flags, and calls the generic launcher. + - It should not import push-hook detection, resolve agents, or duplicate + launch plumbing. + +7. Extract and reuse the recovery UI. + - Reuse or replace the existing + `src/renderer/src/components/right-sidebar/source-control-fix-split-button.tsx` + rather than adding another split-button component. + - Move the dense recovery notice and Details dialog into focused renderer + modules named for source-control recovery. Avoid `helpers`, `utils`, and new + max-lines suppressions. + - Reuse the same notice/dialog component for commit and push recovery with + action-specific labels, summary, details, prompt, saved recipe, and launch + callback. + - Keep normal remote errors filtered out when a push recovery model is + rendered, so the user does not see both Push blocked and generic remote + error copy for the same failure. + +8. Retain `fixPushFailure` in the existing Source Control AI action model. + - The prototype already adds `fixPushFailure` to + `src/shared/source-control-ai-actions.ts`; keep it as a launch action like + `fixCommitFailure`. + - Verify default settings, normalizers, global settings rows, repository + override rows, labels, descriptions, and variable chips all include Push + failure fixes. + - Do not create a push-specific settings store or migration. Existing + `normalizeSourceControlAiSettings` hydrates missing action defaults. + +## Data Flow + +- User clicks Push, Force Push, Publish Branch, or Sync. +- `runRemoteAction` clears that worktree's previous remote error, records a new + sequence token, and starts the editor-store remote action. +- Git push fails and throws. +- `pushBranch` or sync's inner push-stage catch calls + `resolveRemoteOperationErrorMessage` with push-like options. Sync fetch, pull, + and upstream-status failures do not pass the push-stage flag. +- Sync's inner push-stage catch marks the rethrown error as push-stage so + `runRemoteAction` can preserve `syncPushStage` in the captured snapshot. +- The formatter classifies hook output before auth/transport fallbacks only for + push-like options and shows blocked toast copy. +- `SourceControl` catches the same error, and if the sequence still owns the + worktree, stores `{ kind, rawError, message, syncPushStage, branchName, + entriesSnapshot }`. +- The push recovery helper derives one `pushRecovery` model from that snapshot. +- `CommitArea` renders normal remote errors or the Push blocked recovery notice. +- AI Fix builds the `fixPushFailure` command input and launches the selected + agent in the owning local or SSH runtime. + +## Edge Cases + +- Auth, missing repo, protected-branch, pre-receive, non-fast-forward, and + transport failures must not show the Push blocked panel. +- Submodule push errors keep their existing specialized messages before + push-hook detection. +- Sync failures from fetch, upstream-status, or pull stages must not show Push + blocked, even if stderr contains words like "lint" or "hook". +- Create PR intent remote failures use the same `runRemoteAction` plumbing; if + CommitArea is not visible, blocked toast copy is still required but no hidden + panel work is needed. +- A newer remote operation for the same worktree must clear stale recovery state + immediately and prevent an older in-flight failure from writing after it. +- Switching worktrees or branches must not show another worktree or branch's + push failure. +- External edits or another Orca window may make the failure stale. Do not add + cross-window persistence for this feature; rely on local retry/remount/branch + mismatch clearing and list this as residual risk. +- Source Control AI hidden for a repo still shows the blocked notice and Details; + it hides AI Fix. +- SSH worktrees must detect and launch agents on the owning connection, not a + local fallback. +- Windows launch planning must continue to use PowerShell-safe agent args. +- Very large hook output and very large changed-file sets must be bounded before + prompt generation. +- ANSI/control output must not leak into summaries or Details comparison logic. + Details may show sanitized text while preserving enough line breaks for + debugging. +- Empty custom command templates must stay empty so the launcher rejects them + with a clear settings error. +- Prompt text must treat file paths, branch names, and hook output as data, not + instructions. + +## Test Plan + +- Shared unit tests: + - `src/shared/source-control-push-failure.test.ts` covers detection positives, + auth/protected/pre-receive/non-fast-forward negatives, ANSI/control + stripping, bounded scanning, details comparison, prompt output truncation, + prompt file-list capping, and provider-neutral prompt rules. + - Shared command-template tests cover the generic recovery builder plus + commit/push wrappers, including empty templates and prompt overrides. + - `src/shared/source-control-ai-actions.test.ts` covers `fixPushFailure` + label, default template, variables, normalization, and action-list inclusion. +- Remote formatter and store tests: + - `src/renderer/src/lib/source-control-remote-error.test.ts` covers push, + force-push, publish, and sync push-stage blocked copy plus sync non-push + stage negatives, auth/non-fast-forward/protected/pre-receive/submodule + regressions. + - `src/renderer/src/store/slices/editor.test.ts` covers the `pushBranch` toast + path and both sync push-stage and sync non-push-stage toasts. +- Renderer model and UI tests: + - A focused push recovery derivation test covers operation kind, sync + push-stage marker, branch mismatch, stale sequence ownership, snapshot file + entries, summary, details flag, prompt contents, and ordinary remote errors. + - `CommitArea` tests cover the Push blocked notice, Details dialog, AI Fix + visible/hidden, ordinary remote errors, no duplicate remote error when a push + model exists, and unchanged commit-failure recovery rendering. + - Settings tests cover global and repository Source Control AI action rows + showing Push failure fixes. +- Launcher tests: + - Generic recovery launcher tests cover commit and push action ids, invalid CLI + args before detection, empty template rejection, unavailable saved agent, + local versus SSH agent detection, successful launch/focus, and success copy. +- Checks: + - Run targeted vitest files above, `pnpm typecheck`, `pnpm lint`, and + `pnpm check:max-lines-ratchet`. + +## UI Quality Bar + +The Source Control recovery UI must match `docs/STYLEGUIDE.md`: monochrome, +quiet, dense, token-based, and consistent with adjacent commit-failure recovery. +Use existing shadcn `Button`, `DropdownMenu`, and `Dialog` primitives, lucide +icons, `card`/`border`/`destructive`/`muted` tokens, and the existing action +recipe row patterns. The Push blocked notice should keep the summary readable at +narrow right-sidebar widths, avoid nested cards, avoid new color values, show +stable split-button geometry while launching, and keep Details as progressive +disclosure for long hook output. Dialogs and dropdowns must remain usable in +light/dark mode and under SSH latency. + +## Review Screenshots + +1. Source Control after a simulated pre-push hook failure: Push blocked notice + with summary, AI Fix, and Details. +2. Push failure Details dialog showing hook output and Fix with AI. +3. AI Fix customize-launch dialog for `fixPushFailure`. +4. Ordinary push auth/protected/remote failure state: regular remote error, no + Push blocked notice. +5. Sync fetch/pull-stage failure state: Sync-shaped remote error, no Push + blocked notice. +6. Source Control AI hidden for the repo: Push blocked notice and Details, no AI + Fix. +7. Source Control AI settings showing the Push failure fixes launch recipe. +8. Adjacent commit-failure recovery notice still rendering correctly after the + shared UI extraction. + +## Rollout + +1. Tighten shared push-failure classifier/prompt bounds and add the generic + recovery command builder. +2. Make remote formatter/store sync-stage aware and update toast tests. +3. Add the `SourceControlActionError` snapshot/sequence data and one push + recovery derivation helper. +4. Replace duplicated commit/push launchers with the generic recovery launcher + and thin hook wiring. +5. Extract/reuse source-control recovery UI components, including the existing + split-button module. +6. Verify `fixPushFailure` settings/default/normalizer coverage and i18n labels. +7. Update targeted tests and run typecheck, lint, and max-lines ratchet. +8. Validate rendered Electron states with screenshots before opening the PR. + +## Lightweight Eng Review + +- Scope: Reduced from PR #7787's push-specific parallel path to a shared + recovery layer. Keep the feature limited to local pre-push/lint-hook detection, + blocked copy, an inline recovery notice, Details, and AI launch. No persistence + or provider-specific recovery. +- Architecture/data flow: Shared code owns classification, prompt text, and + command-template rendering; the editor store owns stage-aware toast formatting; + `SourceControl` owns transient worktree-scoped failure snapshots and sequence + invalidation; the generic launcher owns local/SSH agent discovery and terminal + launch. +- Failure modes covered: + - Sync stage ambiguity: only the inner push stage may classify as hook-blocked. + - Auth/protected/pre-receive/non-fast-forward/submodule/transport errors stay + out of push recovery. + - Branch or worktree switches suppress stale recovery models. + - Per-worktree sequence tokens prevent older in-flight failures from + overwriting newer retries or successes. + - Empty custom templates, invalid CLI args, unavailable saved agents, missing + worktree context, SSH host detection, and Windows shell planning fail before + terminal launch. + - Large or hostile hook output and file lists are bounded and treated as data + in prompts. +- Test coverage required: + - Shared classifier/prompt/command/action-model tests in `src/shared`. + - Remote formatter and editor-store tests for push, force-push, publish, sync + push-stage, and sync non-push-stage paths. + - Renderer derivation and `CommitArea` tests for Push blocked, Details, AI Fix, + hidden AI actions, stale branch/sequence, ordinary remote errors, and + existing commit failure recovery. + - Generic launcher tests for local/SSH, invalid args, saved-agent unavailable, + empty template, and success launch. + - Settings tests for global and repo Push failure fixes rows. + - Electron validation for all required screenshots; no separate E2E is + required if renderer tests cover the model and Electron validates the real UI. +- Performance/blast radius: No polling, watchers, migrations, or startup work. + Runtime cost is one bounded classifier pass over at most 64 KiB, bounded prompt + output, and a bounded file snapshot only when a remote action fails. Largest + blast radius is the shared recovery UI and launcher used by existing commit + failures, so commit recovery needs tests and a screenshot. +- UI quality bar: Match `docs/STYLEGUIDE.md` and adjacent Source Control density: + token-based error state, stable split-button geometry, progressive Details + disclosure, no nested cards, concise copy, light/dark compatibility, and + visible disabled/loading state under SSH latency. +- Required review screenshots: + 1. Push blocked notice after pre-push hook failure. + 2. Push failure Details dialog with Fix with AI. + 3. `fixPushFailure` customize-launch dialog. + 4. Ordinary push auth/protected/remote failure with no Push blocked panel. + 5. Sync non-push-stage failure with no Push blocked panel. + 6. Source Control AI hidden: Push blocked without AI Fix. + 7. Source Control AI settings showing Push failure fixes. + 8. Existing commit-failure recovery notice after shared UI extraction. +- Residual risks: Classifier false positives remain possible for unusual local + hook-runner output that mentions push context; keep tests biased toward + preserving auth, protected-branch, and non-fast-forward behavior. Failure + panels are renderer-local, so another Orca window or an external terminal can + fix the problem while the old panel remains visible until local retry, remount, + or branch/worktree change. Electron validation may need a mocked or temporary + repo scenario to trigger a real pre-push hook without mutating user data. diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index 4e7b27639f0..166db56079d 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -270,6 +270,35 @@ describe('git remote operations', () => { await expect(gitPush('/repo', false)).rejects.toThrow('fatal: something obscure happened') }) + it('preserves redacted pre-push hook output from failed pushes', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce( + new Error( + [ + 'Command failed: git push https://x-access-token:ghp_secret@github.com/acme/repo.git HEAD', + 'husky - pre-push hook failed', + 'eslint found 2 errors', + "error: failed to push some refs to 'https://ghp_tailSecret@github.com/acme/repo.git'" + ].join('\n') + ) + ) + + let caught: Error | undefined + try { + await gitPush('/repo', false) + } catch (error) { + caught = error as Error + } + + expect(caught).toBeInstanceOf(Error) + expect(caught?.message).toContain('husky - pre-push hook failed') + expect(caught?.message).toContain('eslint found 2 errors') + expect(caught?.message).not.toContain('x-access-token') + expect(caught?.message).not.toContain('ghp_secret') + expect(caught?.message).not.toContain('ghp_tailSecret') + }) + it('strips embedded credentials from push error messages', async () => { gitExecFileAsyncMock .mockRejectedValueOnce(new Error('no branch')) diff --git a/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx index 4501df7ff63..25c898721f0 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx @@ -33,9 +33,11 @@ function baseProps(overrides: Partial = {}) { commitMessage: 'feat: add commit area', commitError: null as string | null, commitFailureRecoveryPrompt: null as string | null, + pushRecovery: null, remoteActionError: null as string | null, isCommitting: inputs.isCommitting, isFixingCommitFailureWithAI: false, + isFixingPushFailureWithAI: false, sourceControlAiActionsVisible: true, aiEnabled: false, aiAgentConfigured: false, @@ -52,6 +54,7 @@ function baseProps(overrides: Partial = {}) { onGenerate: vi.fn(), onCancelGenerate: vi.fn(), onFixCommitFailureWithAI: vi.fn(), + onFixPushFailureWithAI: vi.fn(), onPrimaryAction: vi.fn(), onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void } diff --git a/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx index 7e0471ff392..137c4c3116e 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx @@ -33,9 +33,11 @@ function baseProps(overrides: Partial = {}) { commitMessage: 'feat: add commit area', commitError: null as string | null, commitFailureRecoveryPrompt: null as string | null, + pushRecovery: null, remoteActionError: null as string | null, isCommitting: inputs.isCommitting, isFixingCommitFailureWithAI: false, + isFixingPushFailureWithAI: false, showComposer: true, sourceControlAiActionsVisible: true, aiEnabled: false, @@ -53,6 +55,7 @@ function baseProps(overrides: Partial = {}) { onGenerate: vi.fn(), onCancelGenerate: vi.fn(), onFixCommitFailureWithAI: vi.fn(), + onFixPushFailureWithAI: vi.fn(), onPrimaryAction: vi.fn(), onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void } diff --git a/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx index 20764fe73bc..4c8d9de1e0e 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx @@ -33,9 +33,11 @@ function baseProps(overrides: Partial = {}) { commitMessage: 'feat: add commit area', commitError: null as string | null, commitFailureRecoveryPrompt: null as string | null, + pushRecovery: null, remoteActionError: null as string | null, isCommitting: inputs.isCommitting, isFixingCommitFailureWithAI: false, + isFixingPushFailureWithAI: false, sourceControlAiActionsVisible: true, aiEnabled: false, aiAgentConfigured: false, @@ -52,6 +54,7 @@ function baseProps(overrides: Partial = {}) { onGenerate: vi.fn(), onCancelGenerate: vi.fn(), onFixCommitFailureWithAI: vi.fn(), + onFixPushFailureWithAI: vi.fn(), onPrimaryAction: vi.fn(), onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void } diff --git a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx index 297d441b28a..b360795e662 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx @@ -7,6 +7,7 @@ import { } from './source-control-primary-action' import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' import { TooltipProvider } from '@/components/ui/tooltip' +import { deriveSourceControlPushRecovery } from './source-control-push-recovery' function buildInputs(overrides: Partial = {}): PrimaryActionInputs { return { @@ -31,9 +32,11 @@ function baseProps(overrides: Partial = {}) { commitMessage: 'feat: add commit area', commitError: null as string | null, commitFailureRecoveryPrompt: null as string | null, + pushRecovery: null as ReturnType, remoteActionError: null as string | null, isCommitting: inputs.isCommitting, isFixingCommitFailureWithAI: false, + isFixingPushFailureWithAI: false, sourceControlAiActionsVisible: true, aiEnabled: false, aiAgentConfigured: false, @@ -50,11 +53,35 @@ function baseProps(overrides: Partial = {}) { onGenerate: vi.fn(), onCancelGenerate: vi.fn(), onFixCommitFailureWithAI: vi.fn(), + onFixPushFailureWithAI: vi.fn(), onPrimaryAction: vi.fn(), onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void } } +function buildPushRecovery( + rawError: string +): NonNullable> { + const recovery = deriveSourceControlPushRecovery({ + actionError: { + kind: 'push', + message: 'Push blocked', + rawError, + branchName: 'main', + worktreePath: '/repo', + entriesSnapshot: [], + entriesSnapshotTotalCount: 0, + sequence: 1 + }, + currentBranchName: 'main', + currentSequence: 1 + }) + if (!recovery) { + throw new Error('push recovery was not derived') + } + return recovery +} + function renderCommitArea(props: Parameters[0]): string { return renderToStaticMarkup( @@ -242,6 +269,33 @@ describe('CommitArea', () => { expect(markup).toContain('commit-area-remote-error') }) + it('shows a compact push hook summary and AI fix action when push fails on a hook', () => { + const raw = + "error: failed to push some refs to 'origin'\nhusky - pre-push hook exited with code 1\neslint found 2 errors" + const markup = renderCommitArea({ + ...baseProps(), + pushRecovery: buildPushRecovery(raw) + }) + + expect(markup).toContain('id="commit-area-push-error"') + expect(markup).toContain('Push blocked') + expect(markup).toContain('Lint failed during push.') + expect(markup).not.toContain('eslint found 2 errors') + expect(markup).toContain('aria-label="Fix push failure with AI"') + expect(markup).toContain('Details') + }) + + it('hides push failure AI actions when Source Control AI actions are hidden', () => { + const markup = renderCommitArea({ + ...baseProps(), + pushRecovery: buildPushRecovery('husky - pre-push hook failed'), + sourceControlAiActionsVisible: false + }) + + expect(markup).not.toContain('Fix push failure with AI') + expect(markup).toContain('Push blocked') + }) + it('formats pull policy errors with command options', () => { const markup = renderCommitArea({ ...baseProps(), diff --git a/src/renderer/src/components/right-sidebar/SourceControl.commit-failure-recovery.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.commit-failure-recovery.test.ts index 7bafffbbb30..02aeaa1911a 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.commit-failure-recovery.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControl.commit-failure-recovery.test.ts @@ -1,9 +1,9 @@ import { describe, expect, it } from 'vitest' import { appendCommitFailureCustomInstruction, - buildCommitFailureAgentCommandInput, buildFixCommitFailurePrompt -} from './SourceControl' +} from '../../../../shared/source-control-commit-failure' +import { buildCommitFailureAgentCommandInput } from '../../../../shared/source-control-commit-failure-agent-command' describe('SourceControl commit failure recovery prompt', () => { it('builds a provider-neutral AI prompt for fixing a failed commit hook', () => { diff --git a/src/renderer/src/components/right-sidebar/SourceControl.push-failure-recovery.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.push-failure-recovery.test.ts new file mode 100644 index 00000000000..2ec8633d61c --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.push-failure-recovery.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import { + appendPushFailureCustomInstruction, + buildFixPushFailurePrompt +} from '../../../../shared/source-control-push-failure' +import { buildPushFailureAgentCommandInput } from '../../../../shared/source-control-push-failure-agent-command' + +describe('SourceControl push failure recovery prompt', () => { + it('leaves blank launch templates blank so the launcher can reject them', () => { + expect( + buildPushFailureAgentCommandInput({ + commandInputTemplate: ' ', + basePrompt: 'Fix this push failure.' + }) + ).toBe('') + }) + + it('falls back to the base push-failure prompt when no launch template is saved', () => { + expect( + buildPushFailureAgentCommandInput({ + commandInputTemplate: undefined, + basePrompt: 'Fix this push failure.' + }) + ).toBe('Fix this push failure.') + }) + + it('adds one-time custom instructions before the response contract', () => { + const prompt = buildFixPushFailurePrompt({ + summary: 'Pre-push hook failed.', + error: 'lint failed', + branchName: 'main', + worktreePath: null, + entries: [], + customInstruction: 'Only change TypeScript files.' + }) + + expect(prompt).toContain('Additional user instruction for this fix:') + expect(prompt).toContain('Only change TypeScript files.') + expect(prompt.trim().endsWith('anything left for the user.')).toBe(true) + }) + + it('leaves the base prompt unchanged for empty custom instructions', () => { + const prompt = 'Fix the failed push.' + expect(appendPushFailureCustomInstruction(prompt, ' ')).toBe(prompt) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts index 71f709de658..b04fd1bea15 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControl.remote-action-errors.test.ts @@ -4,7 +4,11 @@ import { clearRemoteActionErrorsForCompletedConflictOperations } from './SourceC describe('SourceControl remote action error reconciliation', () => { it('clears a rebase failure after git status observes the rebase completed', () => { const errors = { - 'wt-1': { kind: 'rebase' as const, message: 'Rebase failed. Could not apply abc123' } + 'wt-1': { + kind: 'rebase' as const, + message: 'Rebase failed. Could not apply abc123', + rawError: 'Rebase failed. Could not apply abc123' + } } expect( @@ -18,7 +22,11 @@ describe('SourceControl remote action error reconciliation', () => { it('keeps a rebase failure while the rebase is still in progress', () => { const errors = { - 'wt-1': { kind: 'rebase' as const, message: 'Rebase failed. Could not apply abc123' } + 'wt-1': { + kind: 'rebase' as const, + message: 'Rebase failed. Could not apply abc123', + rawError: 'Rebase failed. Could not apply abc123' + } } expect( @@ -32,7 +40,11 @@ describe('SourceControl remote action error reconciliation', () => { it('keeps immediate rebase failures that never entered a rebase operation', () => { const errors = { - 'wt-1': { kind: 'rebase' as const, message: 'Rebase blocked - commit first.' } + 'wt-1': { + kind: 'rebase' as const, + message: 'Rebase blocked - commit first.', + rawError: 'Rebase blocked - commit first.' + } } expect( @@ -46,9 +58,21 @@ describe('SourceControl remote action error reconciliation', () => { it('clears pull and sync conflict errors after their merge operation completes', () => { const errors = { - 'wt-pull': { kind: 'pull' as const, message: 'Pull stopped with merge conflicts.' }, - 'wt-sync': { kind: 'sync' as const, message: 'Sync stopped with merge conflicts.' }, - 'wt-fetch': { kind: 'fetch' as const, message: 'Fetch failed. network timeout' } + 'wt-pull': { + kind: 'pull' as const, + message: 'Pull stopped with merge conflicts.', + rawError: 'Pull stopped with merge conflicts.' + }, + 'wt-sync': { + kind: 'sync' as const, + message: 'Sync stopped with merge conflicts.', + rawError: 'Sync stopped with merge conflicts.' + }, + 'wt-fetch': { + kind: 'fetch' as const, + message: 'Fetch failed. network timeout', + rawError: 'Fetch failed. network timeout' + } } expect( diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index b097fab1358..dcd08ea3556 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -11,9 +11,7 @@ import { Loader2, RefreshCw, Settings2, - Sparkle, Sparkles, - SlidersHorizontal, Square, Undo2, Check, @@ -32,7 +30,10 @@ import { type LucideIcon } from 'lucide-react' import { useAppStore } from '@/store' -import { resolveRemoteOperationErrorMessage } from '@/lib/source-control-remote-error' +import { + isSyncPushStageError, + resolveRemoteOperationErrorMessage +} from '@/lib/source-control-remote-error' import { useActiveWorktree, useRepoById, useWorktreeMap } from '@/store/selectors' import { getHostedReviewCacheKey } from '@/store/slices/hosted-review' import { getGitHubPRCacheKey } from '@/store/slices/github-cache-key' @@ -131,7 +132,6 @@ import { toast } from 'sonner' import { SourceControlEntryContextMenu } from './source-control-entry-context-menu' import { Dialog, - DialogClose, DialogContent, DialogDescription, DialogFooter, @@ -219,12 +219,6 @@ import { loadSessionCommitDrafts, saveSessionCommitDrafts } from '@/lib/source-control-commit-draft-session' -import { - getCommitFailureDialogWorktreeKey, - shouldShowCommitFailureDialog, - syncCommitFailureDialogState, - type CommitFailureDialogState -} from './commit-failure-dialog-state' import { hasExpandedCommitFailureDetails, summarizeCommitFailure } from './commit-failure-summary' import { isSourceControlSplitOpenModifier, @@ -303,11 +297,25 @@ import { type PullRequestGenerationContext, type PullRequestGenerationFields } from '@/store/slices/pull-request-generation' +import { + captureSourceControlRecoveryEntrySnapshot, + type SourceControlActionError, + type SourceControlRecoveryStatusEntry +} from './source-control-action-error' +import { + deriveSourceControlPushRecovery, + getSourceControlRecoveryFailureKindLabel, + type SourceControlPushRecovery +} from './source-control-push-recovery' +import { SourceControlRecoveryNotice } from './source-control-recovery-notice' export { appendCommitFailureCustomInstruction, + appendPushFailureCustomInstruction, buildCommitFailureAgentCommandInput, buildFixCommitFailurePrompt, + buildFixPushFailurePrompt, + buildPushFailureAgentCommandInput, buildResolveConflictsPrompt, buildResolvePullRequestConflictsPrompt } from './source-control-ai-prompts' @@ -317,11 +325,6 @@ export { } from './source-control-text-generation-defaults' type AbortConflictOperation = Extract -type AbortActionErrorKind = 'abort_merge' | 'abort_rebase' -export type SourceControlActionError = { - kind: RemoteOpKind | AbortActionErrorKind - message: string -} type SourceControlOperationTarget = RuntimeGitContext & { worktreeId: string pushTarget?: GitPushTarget @@ -725,6 +728,7 @@ function resolveRemoteActionError(kind: RemoteOpKind, error: unknown): string { isPush: kind === 'push', isForcePush: kind === 'force_push', isSync: kind === 'sync', + isSyncPushStage: kind === 'sync' && isSyncPushStageError(error), isFetch: kind === 'fetch', isFastForward: kind === 'fast_forward', isRebase: kind === 'rebase' @@ -1034,6 +1038,7 @@ function SourceControlInner(): React.JSX.Element { const [remoteActionErrors, setRemoteActionErrors] = useState< Record >({}) + const remoteActionErrorSequenceByWorktreeRef = useRef>({}) const previousConflictOperationsRef = useRef>({}) // Why: keep commit-in-flight state per-worktree. A single boolean would be // cleared when the user switched worktrees, letting them double-click Commit @@ -1134,6 +1139,9 @@ function SourceControlInner(): React.JSX.Element { const commitMessage = readCommitDraftForWorktree(commitDrafts, activeWorktreeId) const commitError = commitErrors[activeWorktreeId ?? ''] ?? null const remoteActionError = remoteActionErrors[activeWorktreeId ?? ''] ?? null + const activeRemoteActionSequence = activeWorktreeId + ? (remoteActionErrorSequenceByWorktreeRef.current[activeWorktreeId] ?? null) + : null const [gitHistoryByWorktree, setGitHistoryByWorktree] = useState< Record >({}) @@ -1887,6 +1895,15 @@ function SourceControlInner(): React.JSX.Element { })), [unresolvedConflicts] ) + const pushRecovery = useMemo( + () => + deriveSourceControlPushRecovery({ + actionError: remoteActionError, + currentBranchName: branchName || null, + currentSequence: activeRemoteActionSequence + }), + [activeRemoteActionSequence, branchName, remoteActionError] + ) const { sourceControlAiDiscoveryHostKey, sourceControlAiActionsVisible, @@ -1901,12 +1918,14 @@ function SourceControlInner(): React.JSX.Element { openCommitGenerationDialog, openPullRequestGenerationDialog, isLaunchingCommitFailureAgent, + isLaunchingPushFailureAgent, resolveConflictsPrompt, commitFailureRecoveryPrompt, getLaunchActionRecipe, saveLaunchActionDefault, handleResolveConflictsWithAI, handleFixCommitFailureWithAI, + handleFixPushFailureWithAI, handleSaveCommitMessageGenerationDefaults, handleSavePullRequestGenerationDefaults, openSourceControlAiSettings @@ -1923,6 +1942,7 @@ function SourceControlInner(): React.JSX.Element { worktreePath, commitMessage, commitError, + pushRecoveryPrompt: pushRecovery?.prompt ?? null, updateSettings, updateRepo, openSettingsTarget, @@ -1978,6 +1998,11 @@ function SourceControlInner(): React.JSX.Element { delete commitInFlightRef.current[key] } } + for (const key of Object.keys(remoteActionErrorSequenceByWorktreeRef.current)) { + if (!worktreeMap.has(key)) { + delete remoteActionErrorSequenceByWorktreeRef.current[key] + } + } for (const key of Object.keys(generateInFlightRef.current)) { if (!worktreeMap.has(key)) { delete generateInFlightRef.current[key] @@ -2447,6 +2472,19 @@ function SourceControlInner(): React.JSX.Element { if (!target) { return false } + const sequence = (remoteActionErrorSequenceByWorktreeRef.current[target.worktreeId] ?? 0) + 1 + remoteActionErrorSequenceByWorktreeRef.current[target.worktreeId] = sequence + const targetIsActiveWorktree = target.worktreeId === activeWorktreeId + const recoveryEntrySnapshot = captureSourceControlRecoveryEntrySnapshot( + targetIsActiveWorktree + ? ([ + ...grouped.staged, + ...grouped.unstaged, + ...grouped.untracked + ] satisfies SourceControlRecoveryStatusEntry[]) + : [] + ) + const failureBranchName = targetIsActiveWorktree ? branchName || null : null setRemoteActionErrors((prev) => ({ ...prev, [target.worktreeId]: null })) try { if (kind === 'publish') { @@ -2545,18 +2583,30 @@ function SourceControlInner(): React.JSX.Element { runtimeTargetSettings: target.settings } ) - setRemoteActionErrors((prev) => ({ ...prev, [target.worktreeId]: null })) + if (remoteActionErrorSequenceByWorktreeRef.current[target.worktreeId] === sequence) { + setRemoteActionErrors((prev) => ({ ...prev, [target.worktreeId]: null })) + } return true } catch (error) { // Why: remote action failures are surfaced by editor-slice actions to keep // one consistent toast path and avoid duplicate notifications in the UI. // Keep the latest failure inline too: dropdown-only actions like Fetch can // otherwise look like nothing happened once the menu closes. + if (remoteActionErrorSequenceByWorktreeRef.current[target.worktreeId] !== sequence) { + return false + } setRemoteActionErrors((prev) => ({ ...prev, [target.worktreeId]: { kind, - message: resolveRemoteActionError(kind, error) + message: resolveRemoteActionError(kind, error), + rawError: error instanceof Error ? error.message : String(error), + syncPushStage: kind === 'sync' ? isSyncPushStageError(error) : false, + branchName: failureBranchName, + worktreePath: target.worktreePath, + entriesSnapshot: recoveryEntrySnapshot.entries, + entriesSnapshotTotalCount: recoveryEntrySnapshot.totalCount, + sequence } })) return false @@ -2574,9 +2624,13 @@ function SourceControlInner(): React.JSX.Element { activeRepoSettings, activeWorktree?.pushTarget, activeWorktreeId, + branchName, fetchBranch, fastForwardBranch, effectiveBaseRef, + grouped.staged, + grouped.unstaged, + grouped.untracked, pullBranch, pushBranch, rebaseFromBase, @@ -2639,7 +2693,11 @@ function SourceControlInner(): React.JSX.Element { ) setRemoteActionErrors((prev) => ({ ...prev, - [activeWorktreeId]: { kind: isRebase ? 'abort_rebase' : 'abort_merge', message } + [activeWorktreeId]: { + kind: isRebase ? 'abort_rebase' : 'abort_merge', + message, + rawError: message + } })) } finally { setAbortOperationInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) @@ -5772,10 +5830,12 @@ function SourceControlInner(): React.JSX.Element { commitMessage={commitMessage} commitError={commitError} commitFailureRecoveryPrompt={commitFailureRecoveryPrompt} - remoteActionError={remoteActionError?.message ?? null} + pushRecovery={pushRecovery} + remoteActionError={pushRecovery ? null : (remoteActionError?.message ?? null)} createPrIntentNotice={createPrIntentNotice} isCommitting={isCommitting} isFixingCommitFailureWithAI={isLaunchingCommitFailureAgent} + isFixingPushFailureWithAI={isLaunchingPushFailureAgent} isCreatingPr={isCreatingPr || isCreatePrIntentInFlight} isCreatePrIntentInFlight={isCreatePrIntentInFlight} groupId={activeGroupId ?? activeWorktreeId} @@ -5793,6 +5853,7 @@ function SourceControlInner(): React.JSX.Element { primaryAction={primaryAction} dropdownItems={dropdownItems} fixCommitFailureRecipe={getLaunchActionRecipe('fixCommitFailure')} + fixPushFailureRecipe={getLaunchActionRecipe('fixPushFailure')} onCommitMessageChange={(value) => { if (!activeWorktreeId) { return @@ -5806,6 +5867,7 @@ function SourceControlInner(): React.JSX.Element { onSaveLaunchActionDefault={saveLaunchActionDefault} onOpenSourceControlAiSettings={openSourceControlAiSettings} onFixCommitFailureWithAI={handleFixCommitFailureWithAI} + onFixPushFailureWithAI={handleFixPushFailureWithAI} onPrimaryAction={handlePrimaryClick} onDropdownAction={handleActionInvoke} /> @@ -6365,173 +6427,6 @@ function SourceControlInner(): React.JSX.Element { const SourceControl = React.memo(SourceControlInner) export default SourceControl -type CommitFailureFixSplitButtonProps = { - label: string - worktreeId: string | null - groupId: string | null - connectionId?: string | null - repoId?: string | null - launchPlatform?: NodeJS.Platform - prompt: string | null - isLaunching: boolean - variant: React.ComponentProps['variant'] - size: React.ComponentProps['size'] - iconClassName: string - primaryClassName?: string - chevronClassName?: string - savedAgentId?: TuiAgent | null - savedCommandInputTemplate?: string | null - savedAgentArgs?: string | null - onSaveAgentDefault?: ( - target: SourceControlAiWriteTarget, - actionId: SourceControlLaunchActionId, - recipe: SourceControlActionRecipe - ) => void | Promise - onOpenSettings?: () => void - onFixWithDefaultAgent: (promptOverride?: string) => Promise | boolean - onPromptDelivered: () => void -} - -function CommitFailureFixSplitButton({ - label, - worktreeId, - groupId, - connectionId, - repoId, - launchPlatform, - prompt, - isLaunching, - variant, - size, - iconClassName, - primaryClassName, - chevronClassName, - savedAgentId, - savedCommandInputTemplate, - savedAgentArgs, - onSaveAgentDefault, - onOpenSettings, - onFixWithDefaultAgent, - onPromptDelivered -}: CommitFailureFixSplitButtonProps): React.JSX.Element { - const [composerOpen, setComposerOpen] = useState(false) - const canLaunch = Boolean(worktreeId && groupId && prompt) - const dividerClass = variant === 'default' ? 'border-primary-foreground/20' : 'border-border' - - return ( - <> - -
- - - - -
- - {worktreeId && groupId && prompt ? ( - setComposerOpen(true)} - className="gap-2 rounded-[7px] px-2 py-1.5 text-[12px] leading-5 font-medium" - > - - {translate( - 'auto.components.right.sidebar.SourceControl.f0a2dc9e46', - 'Customize launch...' - )} - - ) : ( - - {translate( - 'auto.components.right.sidebar.SourceControl.9e5ccd00aa', - 'Commit failure context unavailable' - )} - - )} - - - {worktreeId && groupId && prompt ? ( - - ) : null} - - ) -} - -function getCommitFailureKindLabel(summary: string): string | null { - if (/\blint\b/i.test(summary)) { - return 'Lint' - } - - if (/\bhook\b|\bpre-commit\b/i.test(summary)) { - return 'Hook' - } - - return null -} - type CommitAreaProps = { worktreeId: string | null groupId: string | null @@ -6541,10 +6436,12 @@ type CommitAreaProps = { commitMessage: string commitError: string | null commitFailureRecoveryPrompt: string | null + pushRecovery: SourceControlPushRecovery | null remoteActionError: string | null createPrIntentNotice?: CreatePrIntentNotice | null isCommitting: boolean isFixingCommitFailureWithAI: boolean + isFixingPushFailureWithAI: boolean isCreatingPr?: boolean isCreatePrIntentInFlight?: boolean showComposer?: boolean @@ -6561,6 +6458,7 @@ type CommitAreaProps = { primaryAction: PrimaryAction dropdownItems: DropdownEntry[] fixCommitFailureRecipe?: SourceControlActionRecipe + fixPushFailureRecipe?: SourceControlActionRecipe onCommitMessageChange: (message: string) => void onGenerate: () => void onCancelGenerate: () => void @@ -6571,6 +6469,7 @@ type CommitAreaProps = { ) => void | Promise onOpenSourceControlAiSettings?: () => void onFixCommitFailureWithAI: (promptOverride?: string) => Promise | boolean + onFixPushFailureWithAI: (promptOverride?: string) => Promise | boolean onPrimaryAction: () => void onDropdownAction: (kind: DropdownActionKind) => void } @@ -6584,10 +6483,12 @@ export function CommitArea({ commitMessage, commitError, commitFailureRecoveryPrompt, + pushRecovery, remoteActionError, createPrIntentNotice, isCommitting, isFixingCommitFailureWithAI, + isFixingPushFailureWithAI, isCreatingPr = false, isCreatePrIntentInFlight = false, showComposer = true, @@ -6604,12 +6505,14 @@ export function CommitArea({ primaryAction, dropdownItems, fixCommitFailureRecipe, + fixPushFailureRecipe, onCommitMessageChange, onGenerate, onCancelGenerate, onSaveLaunchActionDefault, onOpenSourceControlAiSettings, onFixCommitFailureWithAI, + onFixPushFailureWithAI, onPrimaryAction, onDropdownAction }: CommitAreaProps): React.JSX.Element { @@ -6649,7 +6552,8 @@ export function CommitArea({ [commitError] ) const commitFailureKindLabel = useMemo( - () => (commitFailureSummary ? getCommitFailureKindLabel(commitFailureSummary) : null), + () => + commitFailureSummary ? getSourceControlRecoveryFailureKindLabel(commitFailureSummary) : null, [commitFailureSummary] ) const hasCommitFailureDetails = useMemo( @@ -6659,45 +6563,6 @@ export function CommitArea({ : false, [commitError, commitFailureSummary] ) - // Why: the details dialog is scoped to the worktree, not the exact stderr - // text, so a retried commit can refresh an open dialog with newer output. - const commitFailureWorktreeKey = getCommitFailureDialogWorktreeKey(worktreeId) - const [commitFailureDialogState, setCommitFailureDialogState] = - useState({ - worktreeKey: commitFailureWorktreeKey, - open: false - }) - const isCommitFailureDialogOpen = shouldShowCommitFailureDialog( - commitFailureDialogState, - commitFailureWorktreeKey, - hasCommitFailureDetails - ) - const setCommitFailureDialogOpen = useCallback( - (open: boolean) => { - setCommitFailureDialogState({ worktreeKey: commitFailureWorktreeKey, open }) - }, - [commitFailureWorktreeKey] - ) - const handleFixCommitFailureWithAI = useCallback( - async (promptOverride?: string): Promise => { - const launched = await onFixCommitFailureWithAI(promptOverride) - if (launched) { - setCommitFailureDialogOpen(false) - } - return launched - }, - [onFixCommitFailureWithAI, setCommitFailureDialogOpen] - ) - const handleCommitFailureAgentPromptDelivered = useCallback(() => { - setCommitFailureDialogOpen(false) - }, [setCommitFailureDialogOpen]) - - useEffect(() => { - setCommitFailureDialogState((current) => - syncCommitFailureDialogState(current, commitFailureWorktreeKey, hasCommitFailureDetails) - ) - }, [commitFailureWorktreeKey, hasCommitFailureDetails]) - // Why: most primary-kind labels are anchored by a directional icon so // the affirmative Commit (✓) reads distinctly from the remote-state // labels sharing this slot — Push (↑), Sync (↕), Publish (☁︎↑). Pull is @@ -6718,6 +6583,7 @@ export function CommitArea({ }) const describedBy = [ commitError ? 'commit-area-error' : null, + pushRecovery ? 'commit-area-push-error' : null, remoteActionError ? 'commit-area-remote-error' : null, createPrIntentNotice ? 'commit-area-create-pr-intent' : null, generateError ? 'commit-area-generate-error' : null @@ -6977,138 +6843,66 @@ export function CommitArea({ - {commitError && ( - // Why: role="alert" + aria-live="polite" lets screen readers announce - // commit failures; the id ties the message to the textarea via - // aria-describedby so assistive tech associates the two. -