From e8044b1b30756ead7e6fd61a8d96c74f3b72b14d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:53:04 -0700 Subject: [PATCH 1/9] fix(windows): restore fresh-profile startup after durable fsync (#14173) Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: DHTheOne <238933622+DHTheOne@users.noreply.github.com> Co-authored-by: Anton Tupitsyn <70199858+PLUTONYY@users.noreply.github.com> Co-authored-by: 7loop <48346764+7loop@users.noreply.github.com> --- .github/workflows/pr.yml | 8 +++- src/shared/secure-file-fsync-flags.test.ts | 52 ++++++++++++++++++++++ src/shared/secure-file.ts | 11 +++-- 3 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 src/shared/secure-file-fsync-flags.test.ts diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 3c9678a5a8d..0c1789bf3ea 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -426,8 +426,12 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile - - name: Test Windows PowerShell CLI boundary - run: pnpm exec vitest run --config config/vitest.config.ts src/main/cli/wsl-cli-powershell-boundary.test.ts + - name: Test Windows-specific boundaries + run: >- + pnpm exec vitest run --config config/vitest.config.ts + src/main/cli/wsl-cli-powershell-boundary.test.ts + src/main/orca-profiles/profile-index-store.test.ts + src/shared/secure-file-fsync-flags.test.ts - name: Build package inputs run: pnpm run build:release diff --git a/src/shared/secure-file-fsync-flags.test.ts b/src/shared/secure-file-fsync-flags.test.ts new file mode 100644 index 00000000000..1c167bffdad --- /dev/null +++ b/src/shared/secure-file-fsync-flags.test.ts @@ -0,0 +1,52 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' + +const openedPaths = vi.hoisted(() => [] as { path: string; flags: string | number }[]) + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + openSync: (path: NodeFs.PathLike, flags: string | number, mode?: NodeFs.Mode) => { + openedPaths.push({ path: String(path), flags }) + return actual.openSync(path, flags, mode) + } + } +}) + +import { bestEffortFsyncDirectorySync, fsyncFileSync } from './secure-file' + +const createdPaths: string[] = [] + +afterEach(() => { + openedPaths.length = 0 + for (const path of createdPaths.splice(0)) { + rmSync(path, { recursive: true, force: true }) + } +}) + +describe('secure file fsync flags', () => { + it('opens files read/write before fsync', () => { + const directory = mkdtempSync(join(tmpdir(), 'orca-file-fsync-')) + createdPaths.push(directory) + const path = join(directory, 'record.json') + writeFileSync(path, '{}') + + fsyncFileSync(path) + + expect(openedPaths).toEqual([{ path, flags: 'r+' }]) + }) + + const posixIt = process.platform === 'win32' ? it.skip : it + posixIt('opens directories read-only before fsync', () => { + const directory = mkdtempSync(join(tmpdir(), 'orca-directory-fsync-')) + createdPaths.push(directory) + + bestEffortFsyncDirectorySync(directory) + + expect(openedPaths).toEqual([{ path: directory, flags: 'r' }]) + }) +}) diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index e3c38c70e09..7b2f646c540 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -133,8 +133,8 @@ export function writeSecureFile( } } -export function fsyncFileSync(path: string): void { - const descriptor = openSync(path, 'r') +function fsyncPathSync(path: string, flags: 'r' | 'r+'): void { + const descriptor = openSync(path, flags) try { fsyncSync(descriptor) } finally { @@ -142,12 +142,17 @@ export function fsyncFileSync(path: string): void { } } +export function fsyncFileSync(path: string): void { + // FlushFileBuffers requires a write-capable handle on Windows. + fsyncPathSync(path, 'r+') +} + export function bestEffortFsyncDirectorySync(directory: string): void { if (process.platform === 'win32') { return } try { - fsyncFileSync(directory) + fsyncPathSync(directory, 'r') } catch (error) { if ( error instanceof Error && From bd129343625722f5a817ddc56c2ae8d7e2b970cb Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:03:07 -0700 Subject: [PATCH 2/9] fix(terminal): repaint a recovered remote pane from the host's retained buffer (#14095) * fix(terminal): repaint a recovered remote pane from the host's retained buffer A remote-runtime pane that loses its stream re-subscribes, but the recovery subscribe only carries new bytes. When the host's push snapshot is empty -- an idle pane, or one whose PTY has exited and is preserved with its buffer -- the transport dropped it and nothing re-armed the restore, so the pane stayed blank until a visibility flip issued the tagged snapshot request. That is why switching worktrees once "fixed" it. Emit onStreamRecovered from the recovery subscribe path only, and mark the hidden-output restore needed so the pane pulls the buffer the host still holds. The initial subscribe is untouched: it already carries the host snapshot, and re-arming there would cost every pane a redundant restore request on open. * perf(terminal): avoid duplicate recovery snapshot replay --- .../terminal-pane/pty-connection.ts | 5 ++ .../terminal-pane/pty-transport-types.ts | 3 + ...ote-hidden-output-restore-outcomes.test.ts | 23 +++++ .../remote-runtime-pty-transport.test.ts | 85 +++++++++++++++++++ .../remote-runtime-pty-transport.ts | 8 ++ 5 files changed, 124 insertions(+) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 907a8af9696..16b2bc34346 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -5828,6 +5828,11 @@ export function connectPanePty( reportRemoteRendererSerializerReady() } }, + onStreamRecovered: (): void => { + if (isCurrent()) { + markHiddenOutputRestoreNeeded() + } + }, onData: (data: string, meta?: PtyDataMeta): void => { if (isCurrent()) { dataCallback(data, meta, generation) diff --git a/src/renderer/src/components/terminal-pane/pty-transport-types.ts b/src/renderer/src/components/terminal-pane/pty-transport-types.ts index fadc7bd0feb..9befbf45a00 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-types.ts @@ -104,6 +104,9 @@ type PtyCallbacks = { /** Called before an adopted PTY can publish buffered/live bytes. */ onReattachDetermined?: () => void onConnect?: () => void + /** A stream re-established after loss carries only new bytes, so the pane must + * re-pull the host's retained buffer or an idle/exited pane paints nothing. */ + onStreamRecovered?: () => void onDisconnect?: () => void onData?: (data: string, meta?: PtyDataMeta) => void onReplayData?: (data: string, meta?: PtyReplayDataMeta) => void diff --git a/src/renderer/src/components/terminal-pane/remote-hidden-output-restore-outcomes.test.ts b/src/renderer/src/components/terminal-pane/remote-hidden-output-restore-outcomes.test.ts index 0c9b286703c..0e98202aab3 100644 --- a/src/renderer/src/components/terminal-pane/remote-hidden-output-restore-outcomes.test.ts +++ b/src/renderer/src/components/terminal-pane/remote-hidden-output-restore-outcomes.test.ts @@ -32,6 +32,7 @@ function leafIdForPane(paneId: number): string { type ConnectCallbacks = { onReattachDetermined?: () => void onConnect?: () => void + onStreamRecovered?: () => void onData?: ( data: string, meta?: { seq?: number; rawLength?: number; background?: boolean; droppedOutput?: boolean } @@ -379,6 +380,7 @@ type RemotePaneDrive = { disposable: { dispose: () => void } deliver: (data: string, seq: number) => void setOutputPaused: (paused: boolean) => void + recoverStream: () => void writtenChunks: () => string[] } @@ -396,9 +398,11 @@ async function connectHiddenRemoteAgentPane( const capturedOutputPauseCallback: { current: ((paused: boolean, supported: boolean) => void) | null } = { current: null } + const capturedStreamRecoveredCallback: { current: (() => void) | null } = { current: null } transport.connect.mockImplementation(async ({ callbacks }: { callbacks?: ConnectCallbacks }) => { capturedDataCallback.current = callbacks?.onData ?? null capturedOutputPauseCallback.current = callbacks?.onOutputPauseChanged ?? null + capturedStreamRecoveredCallback.current = callbacks?.onStreamRecovered ?? null return REMOTE_PTY_ID }) transportFactoryQueue.push(transport) @@ -415,6 +419,7 @@ async function connectHiddenRemoteAgentPane( disposable, deliver: (data, seq) => capturedDataCallback.current?.(data, { seq, rawLength: data.length }), setOutputPaused: (paused) => capturedOutputPauseCallback.current?.(paused, true), + recoverStream: () => capturedStreamRecoveredCallback.current?.(), writtenChunks: () => pane.terminal.write.mock.calls.map(([data]) => String(data)) } } @@ -618,6 +623,24 @@ describe('remote hidden-output restore outcomes', () => { resetAgentStartupDelayedDeliveryForTests() }) + it('[modern] repaints a recovered visible pane from the retained host buffer', async () => { + const serializeBuffer = vi.fn() + const serializeBufferOutcome = vi.fn().mockResolvedValue({ + availability: { kind: 'snapshot' }, + snapshot: HOST_SNAPSHOT + }) + const drive = await connectHiddenRemoteAgentPane(serializeBuffer, serializeBufferOutcome) + ;(drive.deps.isVisibleRef as { current: boolean }).current = true + + drive.recoverStream() + await flushAsyncTicks(20) + + expect(serializeBufferOutcome).toHaveBeenCalledTimes(1) + expect(drive.writtenChunks().join('')).toContain(HOST_SNAPSHOT_MARKER) + expect(serializeBuffer).not.toHaveBeenCalled() + drive.disposable.dispose() + }) + it('[modern] accepts an empty snapshot as successful recovery without a loss banner', async () => { const serializeBuffer = vi.fn() const serializeBufferOutcome = vi.fn().mockResolvedValue({ diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts index 0292d0b1749..4087cf59a15 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts @@ -5054,6 +5054,91 @@ describe('createRemoteRuntimePtyTransport', () => { transport.destroy?.() }) + it('re-arms the retained-buffer restore when a recovery subscribe replays no snapshot', async () => { + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onReplayData = vi.fn() + const onStreamRecovered = vi.fn() + const onConnect = vi.fn() + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'tab-1', + leafId: 'pane:1' + }) + + await transport.connect({ + url: '', + callbacks: { onReplayData, onStreamRecovered, onConnect } + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const firstStreamId = latestSubscribePayload().streamId + emitSnapshot(firstStreamId, 'INITIAL_SNAPSHOT') + subscriptionCallbacks?.onResponse({ + ok: true, + result: { + type: 'subscribed', + streamId: firstStreamId, + capabilities: { outputPause: 1 } + } + }) + await vi.waitFor(() => expect(onConnect).toHaveBeenCalledTimes(1)) + // The first subscribe already carries the host snapshot; re-arming there would cost + // every pane a redundant restore request on open. + expect(onStreamRecovered).not.toHaveBeenCalled() + + subscriptionCallbacks?.onClose?.() + await vi.waitFor(() => expect(runtimeSubscribe).toHaveBeenCalledTimes(2)) + await vi.waitFor(() => + expect( + subscriptionSendBinary.mock.calls + .map((call) => decodeTerminalStreamFrame(call[0])) + .filter((frame) => frame?.opcode === TerminalStreamOpcode.Subscribe) + ).toHaveLength(2) + ) + const reconnectStreamId = latestSubscribePayload().streamId + // An exited-but-preserved pane has nothing to push and will never emit live bytes, + // so without the re-arm the pane stays blank until a visibility flip. + emitSnapshot(reconnectStreamId, '') + subscriptionCallbacks?.onResponse({ + ok: true, + result: { + type: 'subscribed', + streamId: reconnectStreamId, + capabilities: { outputPause: 1 } + } + }) + + await vi.waitFor(() => expect(onStreamRecovered).toHaveBeenCalledTimes(1)) + expect(onReplayData.mock.calls.map((call) => call[0])).toEqual(['INITIAL_SNAPSHOT']) + + subscriptionCallbacks?.onClose?.() + await vi.waitFor(() => expect(runtimeSubscribe).toHaveBeenCalledTimes(3)) + await vi.waitFor(() => + expect( + subscriptionSendBinary.mock.calls + .map((call) => decodeTerminalStreamFrame(call[0])) + .filter((frame) => frame?.opcode === TerminalStreamOpcode.Subscribe) + ).toHaveLength(3) + ) + const populatedReconnectStreamId = latestSubscribePayload().streamId + emitSnapshot(populatedReconnectStreamId, 'RECOVERY_SNAPSHOT') + subscriptionCallbacks?.onResponse({ + ok: true, + result: { + type: 'subscribed', + streamId: populatedReconnectStreamId, + capabilities: { outputPause: 1 } + } + }) + + await vi.waitFor(() => expect(onConnect).toHaveBeenCalledTimes(3)) + expect(onStreamRecovered).toHaveBeenCalledTimes(1) + expect(onReplayData.mock.calls.map((call) => call[0])).toEqual([ + 'INITIAL_SNAPSHOT', + 'RECOVERY_SNAPSHOT' + ]) + transport.destroy?.() + }) + it('backs off before retrying a capacity-rejected terminal stream', async () => { vi.useFakeTimers() try { diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts index 8908b39c63c..6eff59d84ce 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts @@ -1735,6 +1735,7 @@ export function createRemoteRuntimePtyTransport( setAttachmentReady(false) let transportClosed = false let subscriptionAttached = false + let subscriptionSnapshotHadContent = false // Why: viewport handed to subscribe; a resize during the round-trip falls back to the refresh-only one-shot RPC, replayed through the stream below once current. const subscribedViewport = desiredViewport const isCurrentSubscription = (): boolean => @@ -1760,6 +1761,7 @@ export function createRemoteRuntimePtyTransport( onSnapshot: (data, meta) => { // Why: an empty snapshot can still carry a pending mid-escape tail that must replay so the next live chunk completes it. if ((data || meta?.pendingEscapeTailAnsi) && isCurrentSubscription()) { + subscriptionSnapshotHadContent = true if (subscribedPtyId && bufferPtyShutdownReplayData(subscribedPtyId, data)) { return } @@ -1806,6 +1808,12 @@ export function createRemoteRuntimePtyTransport( markRecoveryHealthy() emitRecoveryState() storedCallbacks.onConnect?.() + // Why: a recovery subscribe replays nothing when the host's push snapshot is + // empty (idle or exited pane), so ask for the retained buffer instead of + // waiting for bytes that an exited process will never send. + if (expectedRecoveryEpoch !== undefined && !subscriptionSnapshotHadContent) { + storedCallbacks.onStreamRecovered?.() + } storedCallbacks.onStatus?.('shell') }, onEnd: () => { From f04895f8b487806164f9cd292ba53ae74b77e2d4 Mon Sep 17 00:00:00 2001 From: FurmaPanda Date: Thu, 13 Aug 2026 05:52:47 +0800 Subject: [PATCH 3/9] fix(agent-status): map omp ask tool to blocked sidebar state The omp agent-hook path could never reach the blocked state: the ask-question gate in normalizePiCompatibleEvent required agentType === 'pi', isAskUserQuestionTool only matched ask_user_question/request_user_input (omp's tool is named 'ask'), and extractPiToolFields only derived interactivePrompt for Pi. An omp agent parked on its ask tool therefore showed 'working' in the sidebar with no question card. Widen all three gates to include omp so tool_call / tool_execution_start for 'ask' maps to blocked and carries the pending-question envelope for the live card. --- src/shared/agent-hook-listener.test.ts | 61 ++++++++++++++++---- src/shared/agent-hook-listener.ts | 8 ++- src/shared/agent-question-answered-intent.ts | 9 ++- 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index 298e7a55c79..1d24daa0b47 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -801,7 +801,15 @@ describe('shared agent-hook-listener', () => { expect(tool?.payload.interactivePrompt).toBeUndefined() }) - it('keeps OMP ask_user_question behavior on Pi-compatible events', () => { + it('maps OMP ask_user_question tool_call to blocked with interactivePrompt', () => { + const questions = { + questions: [ + { + question: 'Choose', + options: ['x', 'y'] + } + ] + } const tool = normalizeHookPayload( state, 'omp', @@ -814,24 +822,55 @@ describe('shared agent-hook-listener', () => { payload: { hook_event_name: 'tool_call', tool_name: 'ask_user_question', - tool_input: { - questions: [ - { - question: 'Choose', - options: ['x', 'y'] - } - ] - } + tool_input: questions } }, 'production' ) expect(tool?.payload).toMatchObject({ - state: 'working', + state: 'blocked', agentType: 'omp', toolName: 'ask_user_question' }) - expect(tool?.payload.interactivePrompt).toBeUndefined() + expect(tool?.payload.interactivePrompt).toBe(JSON.stringify(questions)) + }) + + it('maps OMP ask tool_execution_start to blocked with interactivePrompt', () => { + // Why: omp's question tool is named `ask` (not ask_user_question) and its + // options are { label } objects; orca-agent-status emits tool_execution_start + // while the agent is parked on the user's answer, so the pane must go blocked. + const questions = { + questions: [ + { + id: 'route', + question: 'How to proceed?', + options: [{ label: 'Open a PR' }, { label: 'File an issue' }] + } + ] + } + const blocked = normalizeHookPayload( + state, + 'omp', + { + paneKey: PANE_KEY, + tabId: 'tab-1', + worktreeId: 'wt', + env: 'production', + version: '1', + payload: { + hook_event_name: 'tool_execution_start', + tool_name: 'ask', + tool_input: questions + } + }, + 'production' + ) + expect(blocked?.payload).toMatchObject({ + state: 'blocked', + agentType: 'omp', + toolName: 'ask' + }) + expect(blocked?.payload.interactivePrompt).toBe(JSON.stringify(questions)) }) it('captures Pi session ids on Pi-compatible status events', () => { diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index d24fa70e087..e19cdaa4bd6 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -2056,9 +2056,11 @@ function extractPiToolFields( const toolName = readString(hookPayload, 'tool_name') const rawToolInput = hookPayload.tool_input const toolInput = deriveToolInputPreview(toolName, rawToolInput) - // Why: OMP shares this extractor; only derive interactivePrompt for Pi so OMP ask_user_question metadata stays unchanged. + // Why: omp's `ask` parks on the user's answer exactly like Pi's ask_user_question; + // both need the pending-question envelope so the sidebar can render the live card. const interactivePrompt = - agentKind === 'pi' && (eventName === 'tool_call' || eventName === 'tool_execution_start') + (agentKind === 'pi' || agentKind === 'omp') && + (eventName === 'tool_call' || eventName === 'tool_execution_start') ? deriveInteractivePrompt(toolName, rawToolInput, eventName) : undefined return toolUpdate( @@ -3849,7 +3851,7 @@ function normalizePiCompatibleEvent( // Why: gate on the event's own tool_name (not a merged snapshot) so a stale cached ask_user_question can't re-enter blocked. const isPiAskUserQuestion = - agentType === 'pi' && + (agentType === 'pi' || agentType === 'omp') && isAskUserQuestionTool(readString(hookPayload, 'tool_name')) && (eventName === 'tool_call' || eventName === 'tool_execution_start') diff --git a/src/shared/agent-question-answered-intent.ts b/src/shared/agent-question-answered-intent.ts index 97ac36f5cfd..b9fbde1b823 100644 --- a/src/shared/agent-question-answered-intent.ts +++ b/src/shared/agent-question-answered-intent.ts @@ -12,13 +12,16 @@ export type AgentQuestionAnsweredInferenceRequest = { } /** True for the ask-the-user-a-question tool across agents: Claude's - * `AskUserQuestion`, grok/Pi's `ask_user_question`, and Codex ≥0.145's - * `request_user_input` (same questions/options input shape). + * `AskUserQuestion`, grok/Pi's `ask_user_question`, Codex ≥0.145's + * `request_user_input` (same questions/options input shape), and omp's + * `ask` (option-picker questions envelope). * Why: this is the structured "pick an option" prompt whose full input the * clients render as a live card. */ export function isAskUserQuestionTool(toolName: string | undefined): boolean { const normalized = toolName?.replaceAll(/[^a-z0-9]/gi, '').toLowerCase() - return normalized === 'askuserquestion' || normalized === 'requestuserinput' + return ( + normalized === 'askuserquestion' || normalized === 'requestuserinput' || normalized === 'ask' + ) } const QUESTION_ANSWER_ENTER_INPUTS: ReadonlySet = new Set([ From 49606af8f83525cb8f334581a8795e09bfa97271 Mon Sep 17 00:00:00 2001 From: FurmaPanda <93296874+FurmaPanda@users.noreply.github.com> Date: Thu, 13 Aug 2026 06:03:17 +0800 Subject: [PATCH 4/9] Update src/shared/agent-hook-listener.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/shared/agent-hook-listener.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index 1d24daa0b47..9777b539c0f 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -836,9 +836,7 @@ describe('shared agent-hook-listener', () => { }) it('maps OMP ask tool_execution_start to blocked with interactivePrompt', () => { - // Why: omp's question tool is named `ask` (not ask_user_question) and its - // options are { label } objects; orca-agent-status emits tool_execution_start - // while the agent is parked on the user's answer, so the pane must go blocked. + // Why: OMP emits tool_execution_start while it waits for the user's answer. const questions = { questions: [ { From 63a0462b1b8fbd550a3ce70dcea4730031fdad60 Mon Sep 17 00:00:00 2001 From: m4air Date: Wed, 12 Aug 2026 20:12:34 -0700 Subject: [PATCH 5/9] Stop deriving interactive prompt for OMP ask tool Why: OMP ask maps to blocked sidebar state but should not trigger native prompt rendering. Only Pi's ask_user_question needs the interactivePrompt payload for live card display. --- src/shared/agent-hook-listener.test.ts | 59 ++++---------------- src/shared/agent-hook-listener.ts | 17 +++--- src/shared/agent-question-answered-intent.ts | 9 +-- 3 files changed, 22 insertions(+), 63 deletions(-) diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index 9777b539c0f..9d30dc8a844 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -801,52 +801,8 @@ describe('shared agent-hook-listener', () => { expect(tool?.payload.interactivePrompt).toBeUndefined() }) - it('maps OMP ask_user_question tool_call to blocked with interactivePrompt', () => { - const questions = { - questions: [ - { - question: 'Choose', - options: ['x', 'y'] - } - ] - } + it('maps OMP ask to blocked without publishing a native prompt', () => { const tool = normalizeHookPayload( - state, - 'omp', - { - paneKey: PANE_KEY, - tabId: 'tab-1', - worktreeId: 'wt', - env: 'production', - version: '1', - payload: { - hook_event_name: 'tool_call', - tool_name: 'ask_user_question', - tool_input: questions - } - }, - 'production' - ) - expect(tool?.payload).toMatchObject({ - state: 'blocked', - agentType: 'omp', - toolName: 'ask_user_question' - }) - expect(tool?.payload.interactivePrompt).toBe(JSON.stringify(questions)) - }) - - it('maps OMP ask tool_execution_start to blocked with interactivePrompt', () => { - // Why: OMP emits tool_execution_start while it waits for the user's answer. - const questions = { - questions: [ - { - id: 'route', - question: 'How to proceed?', - options: [{ label: 'Open a PR' }, { label: 'File an issue' }] - } - ] - } - const blocked = normalizeHookPayload( state, 'omp', { @@ -858,17 +814,24 @@ describe('shared agent-hook-listener', () => { payload: { hook_event_name: 'tool_execution_start', tool_name: 'ask', - tool_input: questions + tool_input: { + questions: [ + { + question: 'Choose', + options: ['x', 'y'] + } + ] + } } }, 'production' ) - expect(blocked?.payload).toMatchObject({ + expect(tool?.payload).toMatchObject({ state: 'blocked', agentType: 'omp', toolName: 'ask' }) - expect(blocked?.payload.interactivePrompt).toBe(JSON.stringify(questions)) + expect(tool?.payload.interactivePrompt).toBeUndefined() }) it('captures Pi session ids on Pi-compatible status events', () => { diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index e19cdaa4bd6..d7b7422db93 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -2056,11 +2056,9 @@ function extractPiToolFields( const toolName = readString(hookPayload, 'tool_name') const rawToolInput = hookPayload.tool_input const toolInput = deriveToolInputPreview(toolName, rawToolInput) - // Why: omp's `ask` parks on the user's answer exactly like Pi's ask_user_question; - // both need the pending-question envelope so the sidebar can render the live card. + // Why: OMP shares this extractor; only derive interactivePrompt for Pi so OMP ask_user_question metadata stays unchanged. const interactivePrompt = - (agentKind === 'pi' || agentKind === 'omp') && - (eventName === 'tool_call' || eventName === 'tool_execution_start') + agentKind === 'pi' && (eventName === 'tool_call' || eventName === 'tool_execution_start') ? deriveInteractivePrompt(toolName, rawToolInput, eventName) : undefined return toolUpdate( @@ -3849,13 +3847,14 @@ function normalizePiCompatibleEvent( return null } - // Why: gate on the event's own tool_name (not a merged snapshot) so a stale cached ask_user_question can't re-enter blocked. - const isPiAskUserQuestion = - (agentType === 'pi' || agentType === 'omp') && - isAskUserQuestionTool(readString(hookPayload, 'tool_name')) && + // Why: gate on the event's own tool_name so a stale cached question can't re-enter blocked. + const toolName = readString(hookPayload, 'tool_name') + const isPiCompatibleAsk = + ((agentType === 'pi' && isAskUserQuestionTool(toolName)) || + (agentType === 'omp' && toolName === 'ask')) && (eventName === 'tool_call' || eventName === 'tool_execution_start') - const stateName = isPiAskUserQuestion + const stateName = isPiCompatibleAsk ? 'blocked' : eventName === 'before_agent_start' || eventName === 'agent_start' || diff --git a/src/shared/agent-question-answered-intent.ts b/src/shared/agent-question-answered-intent.ts index b9fbde1b823..97ac36f5cfd 100644 --- a/src/shared/agent-question-answered-intent.ts +++ b/src/shared/agent-question-answered-intent.ts @@ -12,16 +12,13 @@ export type AgentQuestionAnsweredInferenceRequest = { } /** True for the ask-the-user-a-question tool across agents: Claude's - * `AskUserQuestion`, grok/Pi's `ask_user_question`, Codex ≥0.145's - * `request_user_input` (same questions/options input shape), and omp's - * `ask` (option-picker questions envelope). + * `AskUserQuestion`, grok/Pi's `ask_user_question`, and Codex ≥0.145's + * `request_user_input` (same questions/options input shape). * Why: this is the structured "pick an option" prompt whose full input the * clients render as a live card. */ export function isAskUserQuestionTool(toolName: string | undefined): boolean { const normalized = toolName?.replaceAll(/[^a-z0-9]/gi, '').toLowerCase() - return ( - normalized === 'askuserquestion' || normalized === 'requestuserinput' || normalized === 'ask' - ) + return normalized === 'askuserquestion' || normalized === 'requestuserinput' } const QUESTION_ANSWER_ENTER_INPUTS: ReadonlySet = new Set([ From 58a926170cd99baf8920a83a86799bfbd307d735 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:20:47 -0700 Subject: [PATCH 6/9] perf(renderer): index the repo catalog compat merge and keep catalog identity (#13803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(renderer): index project host ownership on repo catalog refresh mergeFetchedProjectCompatibilityForHost runs synchronously inside a zustand set() on every repo catalog refresh and was O(projects x (setups + repos)): getProjectHostIds / getExplicitProjectHostIds rescanned all setups and all repos once per project, and mergePreviousProjectMetadata rebuilt a full catalog repo key-set per project. Precompute the indexes once and hand each helper only its own project's slice. No ownership logic is reimplemented — the same resolvers run, just on pre-sliced inputs, memoized per project object. 200 repos 1.1 ms -> 0.2 ms (5.2x) 600 repos 11.2 ms -> 0.7 ms (15.3x) 1200 repos 43.0 ms -> 1.6 ms (26.6x) Verified against 4,000 randomized differential cases (duplicate repo ids, dangling sourceRepoIds, empty-repoId and orphan setups, dropped setups, local/SSH/runtime host mixes) comparing output element provenance and order against the previous implementation, not just deep structure. Each of the three index helpers was negative-controlled: breaking any one of them turns the differential red. Co-authored-by: Orca * perf(renderer): keep the repo filter array identity across no-op refetches Three fetch sites unconditionally reallocated filterRepoIds (`s.filterRepoIds.filter(...)`) on every repo catalog refresh, even when nothing was pruned. Six identity-sensitive subscribers select this array, including App.tsx at the root, so every refresh woke the whole tree. retainValidFilterRepoIds returns the input when every id is still valid; `.every` short-circuits so the common case allocates nothing at all. It lives in its own module rather than ui.ts: repos.ts does not import ui.ts and this codebase has a documented circular-slice-import hazard. The store type widens to `readonly string[]`, which takes three more declaration edits (visible-worktrees, add-repo-skip-finalization, the setter). PersistedUI.filterRepoIds deliberately stays `string[]`: main owns that array, and widening it would fail the ui-state schema-parity assertion unless the zod schema gained `.readonly()`, which Object.freeze()s every parsed `ui.set` payload arriving from a paired client. Copy at the App.tsx IPC boundary instead — one allocation per debounced 150ms persist, not per render. Co-authored-by: Orca * perf(renderer): reconcile projects and host setups across no-op refetches mergeFetchedProjectCompatibilityForHost always allocates — sourceRepoIds is rebuilt per project and fetched setups arrive freshly cloned over IPC — so `projects` and `projectHostSetups` lost both array and element identity on every catalog refresh. #13770's identity work covered only projectGroups and folderWorkspaces. Reconcile both at the merge's single return site (five callers) against `previous`, keyed on what each merge already dedups by: project.id, and getProjectHostSetupOwnerKey for setups. setup.id is deliberately not the setup key — the repo-derived fallback sets id = repo.id, so one repo on two hosts yields two setups sharing an id, and keying on it would splice the wrong host's routing metadata into a row. filterSetupsForPrunedRepoRows had to stop returning `[...setups]` unconditionally: three callers feed its result in as `previous`, so the throwaway copy defeated the reconcile even though element identity survived. Rather than add a third parallel deep-equality helper, generalize reconcileFetchedRepos (#13744) into reconcileCatalogRows and delete repos.ts's isPlainCatalogObject + areCatalogEntriesEqual (#13770). The two were the same algorithm modulo the null-prototype branch; the survivor takes the union of both prototype guards, which is strictly more permissive and so can only reconcile more, never mask a change. repos-catalog-merge-identity.test.ts and repo-identity-reconcile.test.ts stay green unmodified as the proof. Element-level reuse means a consumer mutating a Project or ProjectHostSetup in place would corrupt the previous render's object, so RepoSlice['projects'], ['projectHostSetups'] and ProjectHostSetupProjection widen to readonly. That is the safety mechanism, not polish — it caught two real in-place accumulators (ai-vault-scope-paths and the runtime-host purge in worktrees.ts), which now annotate mutable locals per the repo's readonly precedent. No casts added. Co-authored-by: Orca * test(renderer): cover repo catalog refresh identity A dedicated file — repos.test.ts is already at the max-lines ceiling and these are a distinct concern. The fixture is production-shaped on purpose. A scalar-only repo reconciles even when the structural compare is broken, which is how an earlier version of this work shipped green while being completely inert; addedAt is pinned non-zero because project-host-setup-projection falls back to `repo.addedAt || now`, so a zero timestamp stamps Date.now() into every projection and nothing ever reconciles. Every mocked fetch returns a structuredClone so identity can never match by accident. Every identity assertion was verified red against reverted source: - drop reconcileCatalogRows from the merge return -> no-op-refetch identity and per-element reuse fail - filterSetupsForPrunedRepoRows back to `[...setups]` -> no-op-refetch identity fails - retainValidFilterRepoIds back to `.filter(...)` -> both filter-identity cases fail - areValuesEqual reduced to `a === b` -> all three reconcile cases fail - areValuesEqual forced to `true` -> all six change-propagation cases fail - setup key switched to setup.id -> the two-hosts-one-repo-id case fails Co-authored-by: Orca --------- Co-authored-by: Orca --- src/renderer/src/App.tsx | 4 +- .../automations/AutomationEditorDialog.tsx | 2 +- .../AutomationEditorDialogFooter.tsx | 2 +- .../right-sidebar/ai-vault-scope-paths.ts | 8 +- .../sidebar/add-repo-skip-finalization.ts | 2 +- .../components/sidebar/visible-worktrees.ts | 2 +- src/renderer/src/lib/http-link-routing.ts | 2 +- .../src/store/slices/repo-filter-selection.ts | 10 + .../store/slices/repo-identity-reconcile.ts | 58 ++-- .../slices/repos-refresh-identity.test.ts | 283 ++++++++++++++++++ src/renderer/src/store/slices/repos.ts | 205 +++++++++---- src/renderer/src/store/slices/ui.ts | 4 +- src/renderer/src/store/slices/worktrees.ts | 2 +- src/shared/project-host-setup-projection.ts | 4 +- 14 files changed, 481 insertions(+), 107 deletions(-) create mode 100644 src/renderer/src/store/slices/repo-filter-selection.ts create mode 100644 src/renderer/src/store/slices/repos-refresh-identity.test.ts diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 643b6497e5d..0795d6221a3 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -1416,7 +1416,9 @@ function App(): React.JSX.Element { hideWorkspacesFromOtherDevices, alwaysShowDefaultBranchWorkspace, showDotfilesByWorktree, - filterRepoIds, + // Why: the store keeps this readonly for identity stability, but PersistedUI crosses to + // main, which owns a mutable array — copy at the boundary rather than widening the wire type. + filterRepoIds: [...filterRepoIds], // Why (#9002): activeView is deliberately NOT included here. It used to // ride this same 150ms writer (#8265), which meant every top-level view // switch scheduled a full durable-state save. The narrow preference diff --git a/src/renderer/src/components/automations/AutomationEditorDialog.tsx b/src/renderer/src/components/automations/AutomationEditorDialog.tsx index 90eb5a9df47..6ac0c7bc66c 100644 --- a/src/renderer/src/components/automations/AutomationEditorDialog.tsx +++ b/src/renderer/src/components/automations/AutomationEditorDialog.tsx @@ -62,7 +62,7 @@ type AutomationEditorDialogProps = { canSave: boolean createTarget: AutomationCreateTarget repos: readonly Repo[] - projectHostSetups: ProjectHostSetup[] + projectHostSetups: readonly ProjectHostSetup[] automationYamlHooksByRepoKey: Record getAutomationHooksCacheKey: (repoId: string) => string repoMap: Map diff --git a/src/renderer/src/components/automations/AutomationEditorDialogFooter.tsx b/src/renderer/src/components/automations/AutomationEditorDialogFooter.tsx index ff5a67cc41b..160dac72d8b 100644 --- a/src/renderer/src/components/automations/AutomationEditorDialogFooter.tsx +++ b/src/renderer/src/components/automations/AutomationEditorDialogFooter.tsx @@ -32,7 +32,7 @@ type AutomationEditorDialogFooterProps = { isSaving: boolean canSave: boolean repos: readonly Repo[] - projectHostSetups: ProjectHostSetup[] + projectHostSetups: readonly ProjectHostSetup[] automationYamlHooksByRepoKey: Record getAutomationHooksCacheKey: (repoId: string) => string repoMap: Map diff --git a/src/renderer/src/components/right-sidebar/ai-vault-scope-paths.ts b/src/renderer/src/components/right-sidebar/ai-vault-scope-paths.ts index 7bc85b13093..c9ad8137fc9 100644 --- a/src/renderer/src/components/right-sidebar/ai-vault-scope-paths.ts +++ b/src/renderer/src/components/right-sidebar/ai-vault-scope-paths.ts @@ -3,7 +3,7 @@ import { normalizeRuntimePathForComparison } from '../../../../shared/cross-platform-path' import type { ProjectHostSetupProjection } from '../../../../shared/project-host-setup-projection' -import type { Worktree } from '../../../../shared/types' +import type { ProjectHostSetup, Worktree } from '../../../../shared/types' import { splitWorktreeIdForFilesystem } from '../../../../shared/worktree-id' export function deriveAiVaultWorkspaceScopePaths( @@ -88,10 +88,10 @@ export function deriveAiVaultScopeSessionPaths( function buildProjectSetupsByRepoId( projection?: ProjectHostSetupProjection -): Map { - const setupsByRepoId = new Map() +): Map { + const setupsByRepoId = new Map() for (const setup of projection?.setups ?? []) { - const setups = setupsByRepoId.get(setup.repoId) ?? [] + const setups: ProjectHostSetup[] = setupsByRepoId.get(setup.repoId) ?? [] setups.push(setup) setupsByRepoId.set(setup.repoId, setups) } diff --git a/src/renderer/src/components/sidebar/add-repo-skip-finalization.ts b/src/renderer/src/components/sidebar/add-repo-skip-finalization.ts index 4161f3bb6f9..7c9c69357ce 100644 --- a/src/renderer/src/components/sidebar/add-repo-skip-finalization.ts +++ b/src/renderer/src/components/sidebar/add-repo-skip-finalization.ts @@ -3,7 +3,7 @@ import { isDefaultBranchWorkspace } from './visible-worktrees' export type AddRepoSkipFinalizationState = { activeRepoId: string | null - filterRepoIds: string[] + filterRepoIds: readonly string[] showActiveOnly: boolean hideDefaultBranchWorkspace: boolean showSleepingWorkspaces: boolean diff --git a/src/renderer/src/components/sidebar/visible-worktrees.ts b/src/renderer/src/components/sidebar/visible-worktrees.ts index f85083db406..206d81dbe38 100644 --- a/src/renderer/src/components/sidebar/visible-worktrees.ts +++ b/src/renderer/src/components/sidebar/visible-worktrees.ts @@ -186,7 +186,7 @@ export function computeVisibleWorktreeIds( worktreesByRepo: Record, sortedIds: string[], opts: { - filterRepoIds: string[] + filterRepoIds: readonly string[] showSleepingWorkspaces: boolean tabsByWorktree: Record[]> | null ptyIdsByTabId: Record | null diff --git a/src/renderer/src/lib/http-link-routing.ts b/src/renderer/src/lib/http-link-routing.ts index 4b4a6a22a23..3295183d504 100644 --- a/src/renderer/src/lib/http-link-routing.ts +++ b/src/renderer/src/lib/http-link-routing.ts @@ -40,7 +40,7 @@ type StoreAccessor = () => { setActiveWorktree: (worktreeId: string) => void createBrowserTab: (worktreeId: string, url: string, opts: { activate: boolean }) => unknown repos?: readonly LocalhostLinkRepo[] - projects?: LocalhostLinkProject[] + projects?: readonly LocalhostLinkProject[] worktreesByRepo?: Record allWorktrees?: () => LocalhostLinkWorktree[] workspacePortScan?: { result: WorkspacePortScanResult } | null diff --git a/src/renderer/src/store/slices/repo-filter-selection.ts b/src/renderer/src/store/slices/repo-filter-selection.ts new file mode 100644 index 00000000000..65ae6d344a8 --- /dev/null +++ b/src/renderer/src/store/slices/repo-filter-selection.ts @@ -0,0 +1,10 @@ +// Why: catalog refreshes re-filter this on every fetch; six identity-sensitive subscribers +// (App.tsx at the root among them) re-render on a new array even when nothing was pruned. +export function retainValidFilterRepoIds( + filterRepoIds: readonly string[], + validRepoIds: ReadonlySet +): readonly string[] { + return filterRepoIds.every((repoId) => validRepoIds.has(repoId)) + ? filterRepoIds + : filterRepoIds.filter((repoId) => validRepoIds.has(repoId)) +} diff --git a/src/renderer/src/store/slices/repo-identity-reconcile.ts b/src/renderer/src/store/slices/repo-identity-reconcile.ts index f382ba21e2a..f61107c2673 100644 --- a/src/renderer/src/store/slices/repo-identity-reconcile.ts +++ b/src/renderer/src/store/slices/repo-identity-reconcile.ts @@ -12,7 +12,7 @@ import { getRepoHostIdentity } from './repo-host-identity' // arrays). IPC structured-clone rebuilds those every fetch, and main's hydrateRepo always // reconstructs hookSettings — so a reference compare reports every repo as changed and no repo // ever reconciles. Compare nested plain values structurally; they are small sanitized records. -function areValuesEqual(a: unknown, b: unknown): boolean { +export function areValuesEqual(a: unknown, b: unknown): boolean { if (a === b) { return true } @@ -28,9 +28,11 @@ function areValuesEqual(a: unknown, b: unknown): boolean { ) } // Why: only plain records are safe to walk — anything exotic falls back to reference equality. + const aPrototype = Object.getPrototypeOf(a) + const bPrototype = Object.getPrototypeOf(b) if ( - Object.getPrototypeOf(a) !== Object.prototype || - Object.getPrototypeOf(b) !== Object.prototype + (aPrototype !== Object.prototype && aPrototype !== null) || + (bPrototype !== Object.prototype && bPrototype !== null) ) { return false } @@ -45,41 +47,35 @@ function areValuesEqual(a: unknown, b: unknown): boolean { ) } -function areReposEqual(a: Repo, b: Repo): boolean { - if (a === b) { - return true - } - const keys = Object.keys(a) as (keyof Repo)[] - if (keys.length !== Object.keys(b).length) { - return false - } - for (const key of keys) { - if (!Object.hasOwn(b, key)) { - return false - } - if (!areValuesEqual(a[key], b[key])) { - return false - } - } - return true -} - -export function reconcileFetchedRepos( - previous: readonly Repo[], - next: readonly Repo[] -): readonly Repo[] { - const previousById = new Map(previous.map((repo) => [getRepoHostIdentity(repo), repo])) +/** + * Reuses equal rows from `previous` — and the whole array when nothing moved — so a refetch that + * changed nothing leaves identity-keyed memos and store subscribers untouched. `getIdentity` must + * be the key the producing merge already dedups by, so it is unique within `next`. + */ +export function reconcileCatalogRows( + previous: readonly T[], + next: readonly T[], + getIdentity: (row: T) => string +): readonly T[] { + const previousByIdentity = new Map(previous.map((row) => [getIdentity(row), row])) let identical = next.length === previous.length - const reconciled = next.map((repo, index) => { - const existing = previousById.get(getRepoHostIdentity(repo)) - if (existing && areReposEqual(existing, repo)) { + const reconciled = next.map((row, index) => { + const existing = previousByIdentity.get(getIdentity(row)) + if (existing !== undefined && areValuesEqual(existing, row)) { if (existing !== previous[index]) { identical = false } return existing } identical = false - return repo + return row }) return identical ? previous : reconciled } + +export function reconcileFetchedRepos( + previous: readonly Repo[], + next: readonly Repo[] +): readonly Repo[] { + return reconcileCatalogRows(previous, next, getRepoHostIdentity) +} diff --git a/src/renderer/src/store/slices/repos-refresh-identity.test.ts b/src/renderer/src/store/slices/repos-refresh-identity.test.ts new file mode 100644 index 00000000000..7fd32cca5ed --- /dev/null +++ b/src/renderer/src/store/slices/repos-refresh-identity.test.ts @@ -0,0 +1,283 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Project, Repo } from '../../../../shared/types' +import { createTestStore } from './store-test-helpers' + +// Why: every field here is load-bearing. A scalar-only repo reconciles even when the structural +// compare is broken, which is exactly how an earlier version of this work shipped green and inert. +// addedAt must stay non-zero: project-host-setup-projection falls back to `repo.addedAt || now`, +// so a zero timestamp stamps Date.now() into every projection and nothing ever reconciles. +const repo: Repo = { + id: 'repo-1', + path: '/repo-1', + displayName: 'Repo 1', + badgeColor: '#000000', + addedAt: 1_700_000_000_000, + executionHostId: 'local', + kind: 'git', + repoIcon: { type: 'lucide', name: 'Box' }, + upstream: { owner: 'upstream-owner', repo: 'repo-1', host: 'github.com' }, + gitRemoteIdentity: { + canonicalKey: 'github.com/octocat/repo-1', + remoteName: 'origin', + remoteUrl: 'git@github.com:octocat/repo-1.git' + }, + hookSettings: { mode: 'auto', scripts: { setup: 'echo hi', archive: '' } }, + symlinkPaths: ['.env', 'node_modules'], + importedExternalWorktreePaths: [] +} + +const secondRepo: Repo = { + ...repo, + id: 'repo-2', + path: '/repo-2', + displayName: 'Repo 2', + addedAt: 1_700_000_001_000, + upstream: { owner: 'upstream-owner', repo: 'repo-2', host: 'github.com' }, + gitRemoteIdentity: { + canonicalKey: 'github.com/octocat/repo-2', + remoteName: 'origin', + remoteUrl: 'git@github.com:octocat/repo-2.git' + } +} + +const reposList = vi.fn() +const projectsList = vi.fn() +const listHostSetups = vi.fn() + +// Why: catalogs arrive over IPC, so every fetch must hand back freshly allocated objects — +// otherwise identity would match by accident and prove nothing. +function clone(value: T): T { + return structuredClone(value) +} + +function mockRepos(...rows: readonly Repo[]): void { + reposList.mockImplementation(async () => rows.map(clone)) +} + +beforeEach(() => { + reposList.mockReset() + projectsList.mockReset() + listHostSetups.mockReset() + mockRepos(repo) + projectsList.mockImplementation(async () => []) + listHostSetups.mockImplementation(async () => []) + + vi.stubGlobal('window', { + api: { + repos: { list: reposList }, + projects: { list: projectsList, listHostSetups } + }, + dispatchEvent: vi.fn() + }) +}) + +describe('repo catalog refresh identity', () => { + it('keeps the projects and host setups arrays and entries across a no-op refetch', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + const setups = store.getState().projectHostSetups + expect(projects).toHaveLength(1) + expect(setups).toHaveLength(1) + + await store.getState().fetchRepos() + + expect(store.getState().projects).toBe(projects) + expect(store.getState().projects[0]).toBe(projects[0]) + expect(store.getState().projectHostSetups).toBe(setups) + expect(store.getState().projectHostSetups[0]).toBe(setups[0]) + }) + + it('lets a nested hookSettings change through', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const setups = store.getState().projectHostSetups + + mockRepos({ ...repo, hookSettings: { mode: 'override', scripts: { setup: '', archive: '' } } }) + await store.getState().fetchRepos() + + expect(store.getState().projectHostSetups).not.toBe(setups) + expect(store.getState().projectHostSetups[0]?.hookSettings?.mode).toBe('override') + }) + + it('lets a displayName change through on projects', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + + mockRepos({ ...repo, displayName: 'Renamed' }) + await store.getState().fetchRepos() + + expect(store.getState().projects).not.toBe(projects) + expect(store.getState().projects[0]?.displayName).toBe('Renamed') + }) + + it('lets an array-field change through', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const repos = store.getState().repos + + mockRepos({ ...repo, symlinkPaths: ['.env'] }) + await store.getState().fetchRepos() + + expect(store.getState().repos).not.toBe(repos) + expect(store.getState().repos[0]?.symlinkPaths).toEqual(['.env']) + }) + + it('lets a nested gitRemoteIdentity change through', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + + mockRepos({ + ...repo, + gitRemoteIdentity: { ...repo.gitRemoteIdentity!, remoteName: 'upstream' } + }) + await store.getState().fetchRepos() + + expect(store.getState().projects).not.toBe(projects) + expect(store.getState().projects[0]?.gitRemoteIdentity?.remoteName).toBe('upstream') + }) + + it('treats clearing localWindowsRuntimePreference as a change', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projectId = store.getState().projects[0]!.id + const withPreference: Project = { + ...store.getState().projects[0]!, + localWindowsRuntimePreference: { kind: 'wsl', distro: 'Ubuntu' } + } + store.setState({ projects: [withPreference] }) + + await store.getState().fetchRepos() + + // Why: a local-host refresh is authoritative; an absent key must not read as unchanged. + expect(store.getState().projects[0]).not.toBe(withPreference) + expect(store.getState().projects[0]?.id).toBe(projectId) + expect(store.getState().projects[0]?.localWindowsRuntimePreference).toBeUndefined() + }) + + it('reuses an unchanged setup element while a sibling changes', async () => { + mockRepos(repo, secondRepo) + const store = createTestStore() + await store.getState().fetchRepos() + const setups = store.getState().projectHostSetups + expect(setups).toHaveLength(2) + + mockRepos(repo, { ...secondRepo, displayName: 'Repo 2 renamed' }) + await store.getState().fetchRepos() + + const next = store.getState().projectHostSetups + expect(next).not.toBe(setups) + expect(next[0]).toBe(setups[0]) + expect(next[1]).not.toBe(setups[1]) + }) + + it('does not reuse a setup that moved to a different execution host', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const setups = store.getState().projectHostSetups + expect(setups[0]?.hostId).toBe('local') + + // Why: the repo-derived fallback sets setup.id = repo.id, so the same id on a second host is + // the case that would silently splice the wrong host's routing metadata into the row. + mockRepos({ ...repo, executionHostId: 'ssh:host-a', connectionId: 'host-a' }) + await store.getState().fetchRepos() + + const next = store.getState().projectHostSetups + expect(next[0]).not.toBe(setups[0]) + expect(next[0]?.hostId).toBe('ssh:host-a') + }) + + it('reconciles both setups when one repo id exists on two hosts', async () => { + // Why: the repo-derived fallback sets setup.id = repo.id, so these two setups share an id and + // differ only by host. Keying the reconcile on setup.id instead of the owner key collapses them + // onto one slot and one of the two churns on every refresh. + const sshRepo: Repo = { ...repo, executionHostId: 'ssh:host-a', connectionId: 'host-a' } + mockRepos(repo, sshRepo) + const store = createTestStore() + await store.getState().fetchRepos() + const setups = store.getState().projectHostSetups + expect(setups).toHaveLength(2) + expect(new Set(setups.map((setup) => setup.id)).size).toBe(1) + + await store.getState().fetchRepos() + + expect(store.getState().projectHostSetups).toBe(setups) + expect(store.getState().projectHostSetups[0]).toBe(setups[0]) + expect(store.getState().projectHostSetups[1]).toBe(setups[1]) + }) + + it('drops a project and its setup when its repo disappears', async () => { + mockRepos(repo, secondRepo) + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + + mockRepos(repo) + await store.getState().fetchRepos() + + expect(store.getState().projects).not.toBe(projects) + expect(store.getState().projects).toHaveLength(1) + expect(store.getState().projectHostSetups).toHaveLength(1) + }) + + it('adds a project and its setup when a repo appears', async () => { + const store = createTestStore() + await store.getState().fetchRepos() + const projects = store.getState().projects + + mockRepos(repo, secondRepo) + await store.getState().fetchRepos() + + expect(store.getState().projects).not.toBe(projects) + expect(store.getState().projects).toHaveLength(2) + expect(store.getState().projectHostSetups).toHaveLength(2) + }) +}) + +describe('repo filter identity across catalog refreshes', () => { + it('keeps the filterRepoIds array when a refetch prunes nothing', async () => { + const store = createTestStore() + store.setState({ filterRepoIds: [repo.id] }) + const first = store.getState().filterRepoIds + + await store.getState().fetchRepos() + + // Why: App.tsx at the root and five sidebar consumers select this array by identity. + expect(store.getState().filterRepoIds).toBe(first) + }) + + it('still prunes a filtered repo id that no longer exists', async () => { + const store = createTestStore() + store.setState({ filterRepoIds: [repo.id, 'gone'] }) + + await store.getState().fetchRepos() + + expect(store.getState().filterRepoIds).toEqual([repo.id]) + }) + + it('prunes only the vanished id and reallocates', async () => { + mockRepos(repo) + const store = createTestStore() + store.setState({ filterRepoIds: [repo.id, secondRepo.id] }) + const first = store.getState().filterRepoIds + + await store.getState().fetchRepos() + + expect(store.getState().filterRepoIds).toEqual([repo.id]) + expect(store.getState().filterRepoIds).not.toBe(first) + }) + + it('keeps a filtered id whose repo merely moved to another host', async () => { + const store = createTestStore() + store.setState({ filterRepoIds: [repo.id] }) + const first = store.getState().filterRepoIds + + // Why: the filter is keyed on repo id, not host identity — a rehomed repo is not pruned. + mockRepos({ ...repo, executionHostId: 'ssh:host-a', connectionId: 'host-a' }) + await store.getState().fetchRepos() + + expect(store.getState().filterRepoIds).toBe(first) + }) +}) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 49c53d12a7b..dbc55c8c320 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -47,7 +47,12 @@ import { getProjectGroupSubtreeIds } from '../../../../shared/project-groups' import { isPathInsideOrEqual } from '../../../../shared/cross-platform-path' import { getRepoIdFromWorktreeId } from '../../../../shared/worktree-id' import { selectProjectGroupRemovalTargets } from './project-group-removal-targets' -import { reconcileFetchedRepos } from './repo-identity-reconcile' +import { + areValuesEqual, + reconcileCatalogRows, + reconcileFetchedRepos +} from './repo-identity-reconcile' +import { retainValidFilterRepoIds } from './repo-filter-selection' import { mergeSshRepoReadoptions, reconcileReadoptedSshRepoRows, @@ -792,6 +797,81 @@ function getExplicitProjectHostIds( return hostIds } +function indexProjectHostSetupsByProjectId( + setups: readonly ProjectHostSetup[] +): Map { + const setupsByProjectId = new Map() + for (const setup of setups) { + const existing = setupsByProjectId.get(setup.projectId) + if (existing) { + existing.push(setup) + } else { + setupsByProjectId.set(setup.projectId, [setup]) + } + } + return setupsByProjectId +} + +function getProjectSourceRepos( + project: Project, + reposById: ReadonlyMap +): Repo[] { + const sourceRepos: Repo[] = [] + for (const repoId of project.sourceRepoIds) { + for (const repo of reposById.get(repoId) ?? []) { + sourceRepos.push(repo) + } + } + return sourceRepos +} + +// Why: the host-id resolvers rescan every setup and repo per project; feeding them the project's own slices keeps a catalog refresh linear. +function createProjectHostIdIndex( + setups: readonly ProjectHostSetup[], + reposById: ReadonlyMap, + resolveHostIds: ( + project: Project, + setups: readonly ProjectHostSetup[], + repos: readonly Repo[] + ) => Set +): (project: Project) => ReadonlySet { + const noSetups: readonly ProjectHostSetup[] = [] + const hostIdsByProject = new Map>() + let setupsByProjectId: Map | null = null + return (project) => { + const cached = hostIdsByProject.get(project) + if (cached) { + return cached + } + setupsByProjectId ??= indexProjectHostSetupsByProjectId(setups) + const hostIds = resolveHostIds( + project, + setupsByProjectId.get(project.id) ?? noSetups, + getProjectSourceRepos(project, reposById) + ) + hostIdsByProject.set(project, hostIds) + return hostIds + } +} + +// Why: mergePreviousProjectMetadata scans the whole catalog's repo key set; a view holding only this pair's repos keeps that scan per-project. +function restrictReposToProjectPair( + previous: Project, + current: Project, + reposById: ReadonlyMap +): Map { + const restricted = new Map() + for (const project of [previous, current]) { + for (const repoId of project.sourceRepoIds) { + const matches = reposById.get(repoId) + if (matches) { + restricted.set(repoId, matches) + } + } + } + return restricted +} + function mergeFetchedProjectCompatibilityForHost({ previous, fetched, @@ -819,78 +899,78 @@ function mergeFetchedProjectCompatibilityForHost({ const previousProjectById = new Map(previous.projects.map((project) => [project.id, project])) const reposById = getReposById(repos) const currentRepoIds = new Set(repos.map((repo) => repo.id)) - const projectHasHost = (project: Project, setups: readonly ProjectHostSetup[]): boolean => - getProjectHostIds(project, setups, repos).has(hostId) - const projectHasCurrentOwnerOutsideHost = (project: Project): boolean => - [...getExplicitProjectHostIds(project, projectHostSetups, repos)].some( - (ownerHostId) => ownerHostId !== hostId - ) + const fetchedProjectHostIds = createProjectHostIdIndex( + fetched.projectHostSetups, + reposById, + getProjectHostIds + ) + const previousProjectHostIds = createProjectHostIdIndex( + previous.projectHostSetups, + reposById, + getProjectHostIds + ) + const currentProjectOwnerHostIds = createProjectHostIdIndex( + projectHostSetups, + reposById, + getExplicitProjectHostIds + ) + const projectHasCurrentOwnerOutsideHost = (project: Project): boolean => { + for (const ownerHostId of currentProjectOwnerHostIds(project)) { + if (ownerHostId !== hostId) { + return true + } + } + return false + } const fetchedProjects = fetched.projects .filter((project) => { const previousProject = previousProjectById.get(project.id) // Why: repo-derived compatibility projects include every host; a one-host refresh should only reconcile or prune that host's ownership. return ( - projectHasHost(project, fetched.projectHostSetups) || - (previousProject ? projectHasHost(previousProject, previous.projectHostSetups) : false) + fetchedProjectHostIds(project).has(hostId) || + (previousProject ? previousProjectHostIds(previousProject).has(hostId) : false) ) }) .map((project) => { const previousProject = previousProjectById.get(project.id) return previousProject - ? mergePreviousProjectMetadata(previousProject, project, reposById, hostId) + ? mergePreviousProjectMetadata( + previousProject, + project, + restrictReposToProjectPair(previousProject, project, reposById), + hostId + ) : projectWithCurrentSourceRepoIds(project, currentRepoIds) }) const fetchedProjectIds = new Set(fetchedProjects.map((project) => project.id)) const preservedProjects = previous.projects.filter( (project) => !fetchedProjectIds.has(project.id) && - (!getProjectHostIds(project, previous.projectHostSetups, repos).has(hostId) || - projectHasCurrentOwnerOutsideHost(project)) + (!previousProjectHostIds(project).has(hostId) || projectHasCurrentOwnerOutsideHost(project)) ) + // Why: both merges always allocate (sourceRepoIds is rebuilt per project, and fetched setups + // arrive freshly cloned over IPC), so reconcile against `previous` to recover identity when a + // refresh changed nothing. Each key is what the producing merge already dedups by. return { - projects: mergeProjectCompatibilityProjects( - preservedProjects.map((project) => { - const sourceRepoIds = getSourceRepoIdsOutsideHost(project, reposById, hostId) - return sourceRepoIds.length === project.sourceRepoIds.length - ? project - : { ...project, sourceRepoIds } - }), - fetchedProjects + projects: reconcileCatalogRows( + previous.projects, + mergeProjectCompatibilityProjects( + preservedProjects.map((project) => { + const sourceRepoIds = getSourceRepoIdsOutsideHost(project, reposById, hostId) + return sourceRepoIds.length === project.sourceRepoIds.length + ? project + : { ...project, sourceRepoIds } + }), + fetchedProjects + ), + (project) => project.id ), - projectHostSetups - } -} - -function isPlainCatalogObject(value: unknown): value is Record { - if (typeof value !== 'object' || value === null) { - return false - } - const prototype = Object.getPrototypeOf(value) - return prototype === Object.prototype || prototype === null -} - -// Why: catalog fetches rebuild every entry from IPC, so identity alone never matches; -// structural equality is what lets an unchanged refetch stay a no-op. -function areCatalogEntriesEqual(a: unknown, b: unknown): boolean { - if (a === b) { - return true - } - if (Array.isArray(a) || Array.isArray(b)) { - return ( - Array.isArray(a) && - Array.isArray(b) && - a.length === b.length && - a.every((entry, index) => areCatalogEntriesEqual(entry, b[index])) + projectHostSetups: reconcileCatalogRows( + previous.projectHostSetups, + projectHostSetups, + getProjectHostSetupOwnerKey ) } - if (!isPlainCatalogObject(a) || !isPlainCatalogObject(b)) { - return false - } - const keys = Object.keys(a) - if (keys.length !== Object.keys(b).length) { - return false - } - return keys.every((key) => Object.hasOwn(b, key) && areCatalogEntriesEqual(a[key], b[key])) } // Why: returning `base` unchanged keeps referential-equality selectors quiet, so a @@ -912,7 +992,7 @@ function mergeByIdentity( changed = true continue } - if (areCatalogEntriesEqual(merged[index], entry)) { + if (areValuesEqual(merged[index], entry)) { continue } merged[index] = entry @@ -1203,7 +1283,7 @@ function filterSetupsForPrunedRepoRows( setups: readonly ProjectHostSetup[], mergedRepos: readonly Repo[], reconciledRepos: readonly Repo[] -): ProjectHostSetup[] { +): readonly ProjectHostSetup[] { const survivingOwners = new Set( reconciledRepos.map((repo) => `${getRepoExecutionHostId(repo)}:${repo.id}`) ) @@ -1212,12 +1292,15 @@ function filterSetupsForPrunedRepoRows( .filter((repo) => !survivingOwners.has(`${getRepoExecutionHostId(repo)}:${repo.id}`)) .map((repo) => `${getRepoExecutionHostId(repo)}:${repo.id}`) ) + // Why: this result feeds the compat merge as `previous`, so an unconditional copy would discard + // the identity that merge is about to try to preserve. if (prunedOwners.size === 0) { - return [...setups] + return setups } - return setups.filter( + const filtered = setups.filter( (setup) => !setup.repoId || !prunedOwners.has(`${setup.hostId}:${setup.repoId}`) ) + return filtered.length === setups.length ? setups : filtered } function reconcileReadoptedSshWorktreeState( @@ -1690,8 +1773,8 @@ function getFolderWorkspacePathStatusRequestSnapshotForRead( export type RepoSlice = { repos: readonly Repo[] - projects: Project[] - projectHostSetups: ProjectHostSetup[] + projects: readonly Project[] + projectHostSetups: readonly ProjectHostSetup[] projectGroups: readonly ProjectGroup[] folderWorkspaces: readonly FolderWorkspace[] folderWorkspacePathStatuses: Record @@ -2006,7 +2089,7 @@ export const createRepoSlice: StateCreator = (set, ? {} : { folderWorkspacePathStatuses: {} }), activeRepoId: s.activeRepoId && validRepoIds.has(s.activeRepoId) ? s.activeRepoId : null, - filterRepoIds: s.filterRepoIds.filter((projectId) => validRepoIds.has(projectId)), + filterRepoIds: retainValidFilterRepoIds(s.filterRepoIds, validRepoIds), setupScriptPromptDismissedRepoIds: filterSetupScriptPromptDismissalsToValidRepos( s.setupScriptPromptDismissedRepoIds, validRepoHostIdentities @@ -2090,7 +2173,7 @@ export const createRepoSlice: StateCreator = (set, ...reconcileReadoptedSshWorktreeState(s, s.pendingSshRepoReadoptions), ...mergedProjectCompatibility, activeRepoId: s.activeRepoId && validRepoIds.has(s.activeRepoId) ? s.activeRepoId : null, - filterRepoIds: s.filterRepoIds.filter((projectId) => validRepoIds.has(projectId)), + filterRepoIds: retainValidFilterRepoIds(s.filterRepoIds, validRepoIds), setupScriptPromptDismissedRepoIds: filterSetupScriptPromptDismissalsToValidRepos( s.setupScriptPromptDismissedRepoIds, validRepoHostIdentities @@ -2172,7 +2255,7 @@ export const createRepoSlice: StateCreator = (set, const validRepoHostIdentities = new Set(s.repos.map(getRepoHostIdentity)) return { activeRepoId: s.activeRepoId && validRepoIds.has(s.activeRepoId) ? s.activeRepoId : null, - filterRepoIds: s.filterRepoIds.filter((projectId) => validRepoIds.has(projectId)), + filterRepoIds: retainValidFilterRepoIds(s.filterRepoIds, validRepoIds), setupScriptPromptDismissedRepoIds: filterSetupScriptPromptDismissalsToValidRepos( s.setupScriptPromptDismissedRepoIds, validRepoHostIdentities diff --git a/src/renderer/src/store/slices/ui.ts b/src/renderer/src/store/slices/ui.ts index 1bfa1d68d77..dbbc2977965 100644 --- a/src/renderer/src/store/slices/ui.ts +++ b/src/renderer/src/store/slices/ui.ts @@ -904,8 +904,8 @@ export type UISlice = { showDotfilesByWorktree: Record setShowDotfilesForWorktree: (worktreeId: string, showDotfiles: boolean) => void toggleShowDotfilesForWorktree: (worktreeId: string) => void - filterRepoIds: string[] - setFilterRepoIds: (ids: string[]) => void + filterRepoIds: readonly string[] + setFilterRepoIds: (ids: readonly string[]) => void collapsedGroups: Set toggleCollapsedGroup: (key: string) => void worktreeCardProperties: WorktreeCardProperty[] diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 94a3a692360..9c1fc6691ec 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -6080,7 +6080,7 @@ export const createWorktreeSlice: StateCreator const reposChanged = survivingRepos.length !== s.repos.length // Why: a repoId-less setup on the removed host can still split a surviving project group, so drop every setup it owns. - const survivingSetups: AppState['projectHostSetups'] = [] + const survivingSetups: ProjectHostSetup[] = [] for (const setup of s.projectHostSetups) { if (isRemovedRuntimeHostId(setup.hostId, removed)) { if (setup.repoId) { diff --git a/src/shared/project-host-setup-projection.ts b/src/shared/project-host-setup-projection.ts index c0c129d643a..febf7a16018 100644 --- a/src/shared/project-host-setup-projection.ts +++ b/src/shared/project-host-setup-projection.ts @@ -13,8 +13,8 @@ type ProjectAccumulator = { } export type ProjectHostSetupProjection = { - projects: Project[] - setups: ProjectHostSetup[] + projects: readonly Project[] + setups: readonly ProjectHostSetup[] } function getProjectProviderIdentity( From 05dc845c93547f060ab1bfb4cef480d91249940e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:40:39 -0700 Subject: [PATCH 7/9] Add filter and sorting to automations page (#14158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add filter and sort to automations list - Filter by status (enabled/paused) or last-run outcome (failed/succeeded/never) - Sort by name or last run with toggleable direction - Display last-run status with relative time in new table column * Add filter and sorting to automations list - Move run indexing to parent component for performance (avoid O(rows × runs) on re-render) - Add internationalized labels for sort direction that include the column name - Internationalize automation status labels and use locale-aware string sorting --- .../AutomationListExternalRows.tsx | 4 + .../automations/AutomationListFilterMenu.tsx | 192 +++++++++++++++ .../automations/AutomationListLastRunCell.tsx | 37 +++ .../AutomationListLocalRows.test.tsx | 35 ++- .../automations/AutomationListLocalRows.tsx | 21 +- .../automations/AutomationListSortHeader.tsx | 51 ++++ .../automations/AutomationListTableHeader.tsx | 50 ++++ .../automations/AutomationListToolbar.tsx | 86 +++++++ .../automations/AutomationsListPanel.tsx | 229 +++++++----------- .../automations/AutomationsPage.tsx | 35 ++- .../automations/AutomationsPageSkeleton.tsx | 13 +- .../automation-list-last-run.test.ts | 168 +++++++++++++ .../automations/automation-list-last-run.ts | 152 ++++++++++++ .../automations/automation-list-view.test.ts | 167 +++++++++++++ .../automations/automation-list-view.ts | 191 +++++++++++++++ .../automations/automations-table-layout.ts | 4 +- .../automations/use-automation-list-view.ts | 98 ++++++++ src/renderer/src/i18n/locales/en.json | 25 +- 18 files changed, 1392 insertions(+), 166 deletions(-) create mode 100644 src/renderer/src/components/automations/AutomationListFilterMenu.tsx create mode 100644 src/renderer/src/components/automations/AutomationListLastRunCell.tsx create mode 100644 src/renderer/src/components/automations/AutomationListSortHeader.tsx create mode 100644 src/renderer/src/components/automations/AutomationListTableHeader.tsx create mode 100644 src/renderer/src/components/automations/AutomationListToolbar.tsx create mode 100644 src/renderer/src/components/automations/automation-list-last-run.test.ts create mode 100644 src/renderer/src/components/automations/automation-list-last-run.ts create mode 100644 src/renderer/src/components/automations/automation-list-view.test.ts create mode 100644 src/renderer/src/components/automations/automation-list-view.ts create mode 100644 src/renderer/src/components/automations/use-automation-list-view.ts diff --git a/src/renderer/src/components/automations/AutomationListExternalRows.tsx b/src/renderer/src/components/automations/AutomationListExternalRows.tsx index 25a9f137397..8cd712b09c2 100644 --- a/src/renderer/src/components/automations/AutomationListExternalRows.tsx +++ b/src/renderer/src/components/automations/AutomationListExternalRows.tsx @@ -36,6 +36,8 @@ import { AUTOMATIONS_TABLE_ROW_SELECTED_CLASS } from './automations-table-layout' import { isPortaledRowMenuClick, isRowActivationKey } from './automation-list-row-interaction' +import { getExternalAutomationLastRunSnapshot } from './automation-list-last-run' +import { AutomationListLastRunCell } from './AutomationListLastRunCell' import { AutomationListStatusCell } from './AutomationListStatusCell' import { translate } from '@/i18n/i18n' @@ -85,6 +87,7 @@ export function AutomationListExternalRows({ const nextRunLabel = entry.job.enabled ? formatExternalDate(entry.job.nextRunAt, relativeNow) : translate('auto.components.automations.AutomationsPage.paused', 'Paused') + const lastRunSnapshot = getExternalAutomationLastRunSnapshot(entry.job) return ( @@ -124,6 +127,7 @@ export function AutomationListExternalRows({ {nextRunLabel} + {providerLabel} diff --git a/src/renderer/src/components/automations/AutomationListFilterMenu.tsx b/src/renderer/src/components/automations/AutomationListFilterMenu.tsx new file mode 100644 index 00000000000..54efbc819a0 --- /dev/null +++ b/src/renderer/src/components/automations/AutomationListFilterMenu.tsx @@ -0,0 +1,192 @@ +import React from 'react' +import { ListFilter, X } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuLabel, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, + DropdownMenuSeparator, + DropdownMenuTrigger +} from '@/components/ui/dropdown-menu' +import { translate } from '@/i18n/i18n' +import { + countAutomationListFilters, + EMPTY_AUTOMATION_LIST_FILTER, + type AutomationListFilter, + type AutomationListLastRunFilter, + type AutomationListStatusFilter +} from './automation-list-view' + +function FilterPill({ + label, + value, + onClear +}: { + label: string + value: string + onClear: () => void +}): React.JSX.Element { + return ( + + {label}: + {value} + + + ) +} + +export function AutomationListFilterPills({ + filter, + onChange +}: { + filter: AutomationListFilter + onChange: (next: AutomationListFilter) => void +}): React.JSX.Element | null { + const statusLabel = translate('auto.components.automations.AutomationsPage.tableStatus', 'Status') + const lastRunLabel = translate( + 'auto.components.automations.AutomationsPage.tableLastRun', + 'Last run' + ) + const statusValueLabel = + filter.status === 'enabled' + ? translate('auto.components.automations.AutomationDetail.eaa02014f8', 'Enabled') + : filter.status === 'paused' + ? translate('auto.components.automations.AutomationDetail.b09b2384fd', 'Paused') + : null + const lastRunValueLabel = + filter.lastRun === 'failed' + ? translate('auto.components.automations.AutomationListFilterMenu.failed', 'Failed') + : filter.lastRun === 'succeeded' + ? translate('auto.components.automations.AutomationListFilterMenu.succeeded', 'Succeeded') + : filter.lastRun === 'never' + ? translate('auto.components.automations.AutomationListFilterMenu.neverRan', 'Never ran') + : null + if (!statusValueLabel && !lastRunValueLabel) { + return null + } + return ( + <> + {statusValueLabel ? ( + onChange({ ...filter, status: 'all' })} + /> + ) : null} + {lastRunValueLabel ? ( + onChange({ ...filter, lastRun: 'all' })} + /> + ) : null} + + ) +} + +export function AutomationListFilterMenu({ + filter, + onChange +}: { + filter: AutomationListFilter + onChange: (next: AutomationListFilter) => void +}): React.JSX.Element { + const activeCount = countAutomationListFilters(filter) + const statusLabel = translate('auto.components.automations.AutomationsPage.tableStatus', 'Status') + const lastRunLabel = translate( + 'auto.components.automations.AutomationsPage.tableLastRun', + 'Last run' + ) + + return ( + + + + + + {statusLabel} + + onChange({ ...filter, status: value as AutomationListStatusFilter }) + } + > + + {translate('auto.components.automations.AutomationListFilterMenu.all', 'All')} + + + {translate('auto.components.automations.AutomationDetail.eaa02014f8', 'Enabled')} + + + {translate('auto.components.automations.AutomationDetail.b09b2384fd', 'Paused')} + + + + {lastRunLabel} + + onChange({ ...filter, lastRun: value as AutomationListLastRunFilter }) + } + > + + {translate('auto.components.automations.AutomationListFilterMenu.all', 'All')} + + + {translate('auto.components.automations.AutomationListFilterMenu.failed', 'Failed')} + + + {translate( + 'auto.components.automations.AutomationListFilterMenu.succeeded', + 'Succeeded' + )} + + + {translate( + 'auto.components.automations.AutomationListFilterMenu.neverRan', + 'Never ran' + )} + + + {activeCount > 0 ? ( + <> + + onChange(EMPTY_AUTOMATION_LIST_FILTER)}> + {translate( + 'auto.components.automations.AutomationListFilterMenu.clear', + 'Clear filters' + )} + + + ) : null} + + + ) +} diff --git a/src/renderer/src/components/automations/AutomationListLastRunCell.tsx b/src/renderer/src/components/automations/AutomationListLastRunCell.tsx new file mode 100644 index 00000000000..6b5ddf42d9c --- /dev/null +++ b/src/renderer/src/components/automations/AutomationListLastRunCell.tsx @@ -0,0 +1,37 @@ +import React from 'react' +import { cn } from '@/lib/utils' +import { + formatAutomationLastRunCell, + type AutomationLastRunSnapshot +} from './automation-list-last-run' + +export function AutomationListLastRunCell({ + snapshot, + now +}: { + snapshot: AutomationLastRunSnapshot + now: number +}): React.JSX.Element { + const cell = formatAutomationLastRunCell(snapshot, now) + const failed = cell.tone === 'failed' + return ( + + {cell.tone !== 'never' ? ( + + ) +} diff --git a/src/renderer/src/components/automations/AutomationListLocalRows.test.tsx b/src/renderer/src/components/automations/AutomationListLocalRows.test.tsx index b6c41db55af..221e0e45142 100644 --- a/src/renderer/src/components/automations/AutomationListLocalRows.test.tsx +++ b/src/renderer/src/components/automations/AutomationListLocalRows.test.tsx @@ -6,11 +6,13 @@ import userEvent from '@testing-library/user-event' import { afterEach, describe, expect, it, vi } from 'vitest' import type { Automation, + AutomationRun, ExternalAutomationJob, ExternalAutomationManager } from '../../../../shared/automations-types' import { AutomationListLocalRows } from './AutomationListLocalRows' import { AutomationListExternalRows } from './AutomationListExternalRows' +import { indexLatestAutomationRuns } from './automation-list-last-run' // Why: Tooltip needs a provider in the app; stub so rows render standalone. vi.mock('@/components/ui/tooltip', () => ({ @@ -84,16 +86,42 @@ function makeExternalManager(): ExternalAutomationManager { } } +function makeRun(overrides: Partial = {}): AutomationRun { + return { + id: 'run-1', + automationId: 'automation-1', + title: 'test', + scheduledFor: 1, + status: 'dispatch_failed', + trigger: 'scheduled', + workspaceId: 'worktree-1', + sessionKind: 'terminal', + chatSessionId: null, + terminalSessionId: null, + terminalPaneKey: null, + terminalPtyId: null, + outputSnapshot: null, + precheckResult: null, + usage: null, + error: 'boom', + startedAt: Date.now() - 8 * 60 * 60 * 1000, + dispatchedAt: Date.now() - 8 * 60 * 60 * 1000, + createdAt: Date.now() - 8 * 60 * 60 * 1000, + ...overrides + } +} + function renderLocalRows(handlers: { onSelect: (automationId: string) => void onDelete?: (automation: Automation) => void + runs?: AutomationRun[] }) { return render( { expect(onSelect).not.toHaveBeenCalled() }) + it('shows last-run status and relative time', () => { + renderLocalRows({ onSelect: vi.fn(), runs: [makeRun()] }) + expect(screen.getByText('Failed 8h ago')).toBeTruthy() + }) + it('still selects when the local row itself is clicked', async () => { const onSelect = vi.fn() const { container } = renderLocalRows({ onSelect }) diff --git a/src/renderer/src/components/automations/AutomationListLocalRows.tsx b/src/renderer/src/components/automations/AutomationListLocalRows.tsx index 97ec2c7bb5f..93019241d63 100644 --- a/src/renderer/src/components/automations/AutomationListLocalRows.tsx +++ b/src/renderer/src/components/automations/AutomationListLocalRows.tsx @@ -31,6 +31,8 @@ import type { ProjectHostSetup, Repo, Worktree } from '../../../../shared/types' import type { RuntimeStatus } from '../../../../shared/runtime-types' import type { TaskSourceHostAvailability } from '../task-source-context-summary' import type { AutomationHostTarget } from './automation-host-client' +import { getLocalAutomationLastRunSnapshot } from './automation-list-last-run' +import { AutomationListLastRunCell } from './AutomationListLastRunCell' import { formatAutomationDateTimeWithRelative } from './automation-page-parts' import { getAutomationTargetAvailability } from './automation-target-availability' import { getAgentLabel } from './automation-draft-model' @@ -48,7 +50,7 @@ export function AutomationListLocalRows({ automations, selectedId, isSelectedLocal, - runs, + lastRunByAutomationId, relativeNow, repoMap, worktreeMap, @@ -67,7 +69,7 @@ export function AutomationListLocalRows({ automations: readonly Automation[] selectedId: string | null | undefined isSelectedLocal: boolean - runs: readonly AutomationRun[] + lastRunByAutomationId: ReadonlyMap relativeNow: number repoMap: ReadonlyMap worktreeMap: ReadonlyMap @@ -86,19 +88,6 @@ export function AutomationListLocalRows({ onToggle: (automation: Automation) => void onDelete: (automation: Automation) => void }): React.JSX.Element { - // Why: one pass over runs instead of a full scan per rendered automation — - // this list re-renders on the relativeNow timer. - const lastRunByAutomationId = React.useMemo(() => { - const latest = new Map() - for (const run of runs) { - const existing = latest.get(run.automationId) - if (!existing || run.createdAt > existing.createdAt) { - latest.set(run.automationId, run) - } - } - return latest - }, [runs]) - return ( <> {automations.map((automation) => { @@ -132,6 +121,7 @@ export function AutomationListLocalRows({ ? (hostLabelById.get(hostId) ?? getExecutionHostLabel(hostId)) : getLocalExecutionHostLabel() const lastRun = lastRunByAutomationId.get(automation.id) + const lastRunSnapshot = getLocalAutomationLastRunSnapshot(automation, lastRun) const lastRunCost = lastRun?.usage?.status === 'known' ? formatAutomationCost(lastRun.usage.estimatedCostUsd) @@ -213,6 +203,7 @@ export function AutomationListLocalRows({ {nextRunLabel} + diff --git a/src/renderer/src/components/automations/AutomationListSortHeader.tsx b/src/renderer/src/components/automations/AutomationListSortHeader.tsx new file mode 100644 index 00000000000..2c24a344328 --- /dev/null +++ b/src/renderer/src/components/automations/AutomationListSortHeader.tsx @@ -0,0 +1,51 @@ +import React from 'react' +import { ArrowDown, ArrowUp } from 'lucide-react' +import { cn } from '@/lib/utils' +import { translate } from '@/i18n/i18n' +import type { AutomationListSort, AutomationListSortField } from './automation-list-view' + +export function AutomationListSortHeader({ + field, + label, + sort, + onSort +}: { + field: AutomationListSortField + label: string + sort: AutomationListSort | null + onSort: (field: AutomationListSortField) => void +}): React.JSX.Element { + const active = sort?.field === field + const direction = active ? sort.direction : null + // Why: one interpolated key per direction — word order and punctuation around + // the column name differ per language. + const sortedLabel = + direction === 'asc' + ? translate( + 'auto.components.automations.AutomationListSortHeader.sortedAscending', + '{{value0}}, sorted ascending', + { value0: label } + ) + : direction === 'desc' + ? translate( + 'auto.components.automations.AutomationListSortHeader.sortedDescending', + '{{value0}}, sorted descending', + { value0: label } + ) + : null + return ( + + ) +} diff --git a/src/renderer/src/components/automations/AutomationListTableHeader.tsx b/src/renderer/src/components/automations/AutomationListTableHeader.tsx new file mode 100644 index 00000000000..8adbd127727 --- /dev/null +++ b/src/renderer/src/components/automations/AutomationListTableHeader.tsx @@ -0,0 +1,50 @@ +import React from 'react' +import { cn } from '@/lib/utils' +import { translate } from '@/i18n/i18n' +import { + AUTOMATIONS_TABLE_GRID_CLASS, + AUTOMATIONS_TABLE_HEADER_CLASS +} from './automations-table-layout' +import { AutomationListSortHeader } from './AutomationListSortHeader' +import type { AutomationListSort, AutomationListSortField } from './automation-list-view' + +export function AutomationListTableHeader({ + sort, + onSort +}: { + sort: AutomationListSort | null + onSort: (field: AutomationListSortField) => void +}): React.JSX.Element { + return ( +
+ + + {translate('auto.components.automations.AutomationDetail.18763ded26', 'Schedule')} + + + {translate('auto.components.automations.AutomationsPage.tableProject', 'Project')} + + + {translate('auto.components.automations.AutomationDetail.578ff46987', 'Next run')} + + + {translate('auto.components.automations.AutomationsPage.tableStatus', 'Status')} + + {translate('auto.components.automations.AutomationDetail.2df8970cd5', 'Agent')} + + + {translate('auto.components.automations.AutomationsPage.tableActions', 'Actions')} + +
+ ) +} diff --git a/src/renderer/src/components/automations/AutomationListToolbar.tsx b/src/renderer/src/components/automations/AutomationListToolbar.tsx new file mode 100644 index 00000000000..31e55df9e04 --- /dev/null +++ b/src/renderer/src/components/automations/AutomationListToolbar.tsx @@ -0,0 +1,86 @@ +import React from 'react' +import { Plus, RefreshCw } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' +import { cn } from '@/lib/utils' +import { translate } from '@/i18n/i18n' +import { clampAutomationListSearchQueryInput } from './automation-list-search' +import type { AutomationListFilter } from './automation-list-view' +import { AutomationListFilterMenu, AutomationListFilterPills } from './AutomationListFilterMenu' +import { AutomationListSearchField } from './AutomationListSearchField' +import type { AutomationTemplate } from './automation-templates' + +export function AutomationListToolbar({ + listSearchQuery, + isListSearchQueryTooLarge, + onListSearchQueryChange, + filter, + onFilterChange, + onRefresh, + isRefreshing, + openCreateDialog +}: { + listSearchQuery: string + isListSearchQueryTooLarge: boolean + onListSearchQueryChange: (query: string) => void + filter: AutomationListFilter + onFilterChange: (filter: AutomationListFilter) => void + onRefresh: () => void + isRefreshing: boolean + openCreateDialog: (template?: AutomationTemplate) => void +}): React.JSX.Element { + return ( +
+
+
+ + onListSearchQueryChange(clampAutomationListSearchQueryInput(query)) + } + onClear={() => onListSearchQueryChange('')} + /> + + + + + + + {translate( + 'auto.components.automations.AutomationsPage.19a6e30eae', + 'Refresh automations' + )} + + +
+ +
+ +
+ ) +} diff --git a/src/renderer/src/components/automations/AutomationsListPanel.tsx b/src/renderer/src/components/automations/AutomationsListPanel.tsx index e3dd2b3c090..a792839885c 100644 --- a/src/renderer/src/components/automations/AutomationsListPanel.tsx +++ b/src/renderer/src/components/automations/AutomationsListPanel.tsx @@ -1,7 +1,6 @@ import React from 'react' -import { Plus, RefreshCw } from 'lucide-react' +import { Plus } from 'lucide-react' import { Button } from '@/components/ui/button' -import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { cn } from '@/lib/utils' import type { Automation, @@ -15,29 +14,35 @@ import type { ProjectHostSetup, Repo, Worktree } from '../../../../shared/types' import type { RuntimeStatus } from '../../../../shared/runtime-types' import type { TaskSourceHostAvailability } from '../task-source-context-summary' import type { AutomationHostTarget } from './automation-host-client' -import { clampAutomationListSearchQueryInput } from './automation-list-search' import type { AutomationPaneTab } from './automation-page-state' -import { AutomationListSearchField } from './AutomationListSearchField' import { getAutomationTemplates, type AutomationTemplate } from './automation-templates' -import type { ExternalAutomationListEntry } from './external-automation-list-entries' import { AutomationListLocalRows } from './AutomationListLocalRows' import { AutomationListExternalRows } from './AutomationListExternalRows' -import { - AUTOMATIONS_TABLE_CONTAINER_CLASS, - AUTOMATIONS_TABLE_GRID_CLASS, - AUTOMATIONS_TABLE_HEADER_CLASS -} from './automations-table-layout' +import { AUTOMATIONS_TABLE_CONTAINER_CLASS } from './automations-table-layout' import { translate } from '@/i18n/i18n' +import type { + AutomationListFilter, + AutomationListSort, + AutomationListSortField, + AutomationListViewItem +} from './automation-list-view' +import { AutomationListTableHeader } from './AutomationListTableHeader' +import { AutomationListToolbar } from './AutomationListToolbar' +import { indexLatestAutomationRuns } from './automation-list-last-run' type AutomationsListPanelProps = { hasListItems: boolean hasFilteredListItems: boolean isListSearchActive: boolean + isListFilterActive: boolean listSearchQuery: string isListSearchQueryTooLarge: boolean onListSearchQueryChange: (query: string) => void - filteredAutomations: readonly Automation[] - filteredExternalAutomationEntries: readonly ExternalAutomationListEntry[] + visibleItems: readonly AutomationListViewItem[] + listFilter: AutomationListFilter + onListFilterChange: (filter: AutomationListFilter) => void + listSort: AutomationListSort | null + onListSort: (field: AutomationListSortField) => void // Why: use explicit ids, not resolved detail selection (which falls back to the // first row) — list highlight should only mark a user-chosen row. selectedId: string | null @@ -79,11 +84,15 @@ export function AutomationsListPanel({ hasListItems, hasFilteredListItems, isListSearchActive, + isListFilterActive, listSearchQuery, isListSearchQueryTooLarge, onListSearchQueryChange, - filteredAutomations, - filteredExternalAutomationEntries, + visibleItems, + listFilter, + onListFilterChange, + listSort, + onListSort, selectedId, selectedExternalKey, runs, @@ -111,6 +120,10 @@ export function AutomationsListPanel({ onRefresh, isRefreshing }: AutomationsListPanelProps): React.JSX.Element { + // Why: one pass over runs for the whole list — each row renders its own + // component, so indexing per row would be O(rows × runs) on every re-render. + const lastRunByAutomationId = React.useMemo(() => indexLatestAutomationRuns(runs), [runs]) + return (
) : (
-
-
- - onListSearchQueryChange(clampAutomationListSearchQueryInput(query)) - } - onClear={() => onListSearchQueryChange('')} - /> - - - - - - {translate( - 'auto.components.automations.AutomationsPage.19a6e30eae', - 'Refresh automations' - )} - - -
- -
+
{hasFilteredListItems ? ( <> -
- - {translate('auto.components.automations.AutomationsPage.tableName', 'Name')} - - - {translate( - 'auto.components.automations.AutomationDetail.18763ded26', - 'Schedule' - )} - - - {translate( - 'auto.components.automations.AutomationsPage.tableProject', - 'Project' - )} - - - {translate( - 'auto.components.automations.AutomationDetail.578ff46987', - 'Next run' - )} - - - {translate('auto.components.automations.AutomationsPage.tableStatus', 'Status')} - - - {translate('auto.components.automations.AutomationDetail.2df8970cd5', 'Agent')} - - - {translate( - 'auto.components.automations.AutomationsPage.tableActions', - 'Actions' - )} - -
+
- { - selectExternalKey(null) - selectAutomationId(automationId) - onOpenDetail() - }} - onRunNow={runNow} - onEdit={openEditDialog} - onToggle={toggleAutomation} - onDelete={requestDeleteAutomation} - /> - { - selectAutomationId(null) - selectExternalKey(entryKey) - setActivePaneTab('overview') - onOpenDetail() - }} - onRequestAction={requestExternalAction} - onEdit={openEditExternalDialog} - /> + {visibleItems.map((item) => + item.kind === 'local' ? ( + { + selectExternalKey(null) + selectAutomationId(automationId) + onOpenDetail() + }} + onRunNow={runNow} + onEdit={openEditDialog} + onToggle={toggleAutomation} + onDelete={requestDeleteAutomation} + /> + ) : ( + { + selectAutomationId(null) + selectExternalKey(entryKey) + setActivePaneTab('overview') + onOpenDetail() + }} + onRequestAction={requestExternalAction} + onEdit={openEditExternalDialog} + /> + ) + )}
- ) : isListSearchActive ? ( + ) : isListSearchActive || isListFilterActive ? (
{translate( - 'auto.components.automations.AutomationsPage.noSearchMatches', - 'No automations match your search.' + 'auto.components.automations.AutomationsPage.noListMatches', + 'No automations match.' )}
) : null} diff --git a/src/renderer/src/components/automations/AutomationsPage.tsx b/src/renderer/src/components/automations/AutomationsPage.tsx index c52126ad670..b1fe9c4988b 100644 --- a/src/renderer/src/components/automations/AutomationsPage.tsx +++ b/src/renderer/src/components/automations/AutomationsPage.tsx @@ -106,6 +106,13 @@ import { import { buildExternalAutomationListEntries } from './external-automation-list-entries' import { shouldCloseDetailForLostSelection } from './automation-detail-selection' import { useAutomationListSearch } from './use-automation-list-search' +import { useAutomationListView } from './use-automation-list-view' +import { + EMPTY_AUTOMATION_LIST_FILTER, + nextAutomationListSort, + type AutomationListFilter, + type AutomationListSort +} from './automation-list-view' import { AutomationDeleteDialog, ExternalAutomationDeleteDialog } from './AutomationDeleteDialogs' import { AutomationsListPanel } from './AutomationsListPanel' import { AutomationsDetailPane } from './AutomationsDetailPane' @@ -173,6 +180,8 @@ export default function AutomationsPage(): React.JSX.Element { const [isLoading, setIsLoading] = useState(true) const [isSaving, setIsSaving] = useState(false) const [listSearchQuery, setListSearchQuery] = useState('') + const [listFilter, setListFilter] = useState(EMPTY_AUTOMATION_LIST_FILTER) + const [listSort, setListSort] = useState(null) const [createOpen, setCreateOpen] = useState(false) const [createTarget, setCreateTarget] = useState('orca') const [editingAutomationId, setEditingAutomationId] = useState(null) @@ -273,8 +282,7 @@ export default function AutomationsPage(): React.JSX.Element { isListSearchActive, filteredAutomations, filteredExternalAutomationEntries, - hasListItems, - hasFilteredListItems + hasListItems } = useAutomationListSearch({ listSearchQuery, automations, @@ -285,6 +293,21 @@ export default function AutomationsPage(): React.JSX.Element { selectAutomationId, selectExternalKey }) + const { + visibleItems, + isListFilterActive, + hasVisibleListItems: hasFilteredListItems + } = useAutomationListView({ + automations: filteredAutomations, + externalEntries: filteredExternalAutomationEntries, + runs, + filter: listFilter, + sort: listSort, + selectedId, + selectedExternalKey, + selectAutomationId, + selectExternalKey + }) const selectedExternal = externalAutomationEntries.find((entry) => entry.key === selectedExternalKey) ?? @@ -2015,11 +2038,15 @@ export default function AutomationsPage(): React.JSX.Element { hasListItems={hasListItems} hasFilteredListItems={hasFilteredListItems} isListSearchActive={isListSearchActive} + isListFilterActive={isListFilterActive} listSearchQuery={listSearchQuery} isListSearchQueryTooLarge={isListSearchQueryTooLarge} onListSearchQueryChange={setListSearchQuery} - filteredAutomations={filteredAutomations} - filteredExternalAutomationEntries={filteredExternalAutomationEntries} + visibleItems={visibleItems} + listFilter={listFilter} + onListFilterChange={setListFilter} + listSort={listSort} + onListSort={(field) => setListSort((current) => nextAutomationListSort(current, field))} selectedId={selectedId} selectedExternalKey={selectedExternalKey} runs={runs} diff --git a/src/renderer/src/components/automations/AutomationsPageSkeleton.tsx b/src/renderer/src/components/automations/AutomationsPageSkeleton.tsx index b8de919f1cc..172502b29a1 100644 --- a/src/renderer/src/components/automations/AutomationsPageSkeleton.tsx +++ b/src/renderer/src/components/automations/AutomationsPageSkeleton.tsx @@ -16,12 +16,14 @@ function TableRowSkeleton({ scheduleWidthClass, projectWidthClass, nextWidthClass, + lastRunWidthClass, statusWidthClass }: { nameWidthClass: string scheduleWidthClass: string projectWidthClass: string nextWidthClass: string + lastRunWidthClass: string statusWidthClass: string }): React.JSX.Element { return ( @@ -30,6 +32,7 @@ function TableRowSkeleton({ + @@ -44,6 +47,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-36', projectWidthClass: 'w-28', nextWidthClass: 'w-24', + lastRunWidthClass: 'w-20', statusWidthClass: 'w-16' }, { @@ -52,6 +56,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-28', projectWidthClass: 'w-32', nextWidthClass: 'w-20', + lastRunWidthClass: 'w-24', statusWidthClass: 'w-14' }, { @@ -60,6 +65,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-40', projectWidthClass: 'w-24', nextWidthClass: 'w-28', + lastRunWidthClass: 'w-16', statusWidthClass: 'w-16' }, { @@ -68,6 +74,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-32', projectWidthClass: 'w-36', nextWidthClass: 'w-24', + lastRunWidthClass: 'w-20', statusWidthClass: 'w-14' }, { @@ -76,6 +83,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-24', projectWidthClass: 'w-28', nextWidthClass: 'w-20', + lastRunWidthClass: 'w-24', statusWidthClass: 'w-16' }, { @@ -84,6 +92,7 @@ const TABLE_ROW_SKELETONS = [ scheduleWidthClass: 'w-36', projectWidthClass: 'w-20', nextWidthClass: 'w-24', + lastRunWidthClass: 'w-16', statusWidthClass: 'w-14' } ] as const @@ -100,7 +109,8 @@ export function AutomationsPageSkeleton(): React.JSX.Element { >
- + +
@@ -114,6 +124,7 @@ export function AutomationsPageSkeleton(): React.JSX.Element { + diff --git a/src/renderer/src/components/automations/automation-list-last-run.test.ts b/src/renderer/src/components/automations/automation-list-last-run.test.ts new file mode 100644 index 00000000000..7eb54e123a5 --- /dev/null +++ b/src/renderer/src/components/automations/automation-list-last-run.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it } from 'vitest' +import type { + Automation, + AutomationRun, + ExternalAutomationJob +} from '../../../../shared/automations-types' +import { + formatAutomationLastRunCell, + getExternalAutomationLastRunSnapshot, + getLocalAutomationLastRunSnapshot, + getToneForAutomationRunStatus, + indexLatestAutomationRuns +} from './automation-list-last-run' + +function makeAutomation(overrides: Partial = {}): Automation { + return { + id: 'automation-1', + name: 'Nightly', + prompt: 'run', + precheck: null, + agentId: 'codex', + projectId: 'repo-1', + executionTargetType: 'local', + executionTargetId: 'local', + schedulerOwner: 'local_host_service', + workspaceMode: 'existing', + workspaceId: 'worktree-1', + baseBranch: null, + reuseSession: false, + timezone: 'UTC', + rrule: 'FREQ=DAILY', + dtstart: 1, + enabled: true, + nextRunAt: 2, + missedRunPolicy: 'run_once_within_grace', + missedRunGraceMinutes: 720, + createdAt: 1, + updatedAt: 1, + ...overrides + } +} + +function makeRun(overrides: Partial = {}): AutomationRun { + return { + id: 'run-1', + automationId: 'automation-1', + title: 'Nightly', + scheduledFor: 10, + status: 'completed', + trigger: 'scheduled', + workspaceId: 'worktree-1', + sessionKind: 'terminal', + chatSessionId: null, + terminalSessionId: null, + terminalPaneKey: null, + terminalPtyId: null, + outputSnapshot: null, + precheckResult: null, + usage: null, + error: null, + startedAt: 20, + dispatchedAt: 30, + createdAt: 10, + ...overrides + } +} + +function makeJob(overrides: Partial = {}): ExternalAutomationJob { + return { + id: 'job-1', + managerId: 'manager-1', + provider: 'hermes', + name: 'Digest', + schedule: '0 9 * * 1-5', + rawSchedule: null, + enabled: true, + state: 'enabled', + prompt: null, + promptPreview: '', + nextRunAt: null, + lastRunAt: null, + lastStatus: null, + lastError: null, + workdir: null, + runCount: 0, + runs: [], + ...overrides + } +} + +describe('automation-list-last-run', () => { + it('indexes the newest run per automation', () => { + const latest = indexLatestAutomationRuns([ + makeRun({ id: 'old', createdAt: 10, dispatchedAt: 10 }), + makeRun({ id: 'new', createdAt: 50, dispatchedAt: 50, status: 'dispatch_failed' }), + makeRun({ id: 'other', automationId: 'automation-2', createdAt: 20 }) + ]) + expect(latest.get('automation-1')?.id).toBe('new') + expect(latest.get('automation-2')?.id).toBe('other') + }) + + it('classifies local run statuses', () => { + expect(getToneForAutomationRunStatus('dispatch_failed')).toBe('failed') + expect(getToneForAutomationRunStatus('completed')).toBe('succeeded') + expect(getToneForAutomationRunStatus('dispatched')).toBe('running') + expect(getToneForAutomationRunStatus('skipped_precheck')).toBe('skipped') + }) + + it('prefers the latest run over lastRunAt-only metadata', () => { + const snapshot = getLocalAutomationLastRunSnapshot( + makeAutomation({ lastRunAt: 5 }), + makeRun({ status: 'dispatch_failed', dispatchedAt: 40, createdAt: 40 }) + ) + expect(snapshot.tone).toBe('failed') + expect(snapshot.at).toBe(40) + expect(snapshot.statusLabel).toBe('Failed') + }) + + it('falls back to lastRunAt when no run history exists', () => { + const snapshot = getLocalAutomationLastRunSnapshot(makeAutomation({ lastRunAt: 99 }), undefined) + expect(snapshot).toEqual({ at: 99, tone: 'unknown', statusLabel: '' }) + }) + + it('treats missing local history as never run', () => { + expect(getLocalAutomationLastRunSnapshot(makeAutomation(), undefined).tone).toBe('never') + }) + + it('classifies external last-run status and errors', () => { + expect( + getExternalAutomationLastRunSnapshot( + makeJob({ lastRunAt: '2026-08-12T01:00:00Z', lastStatus: 'failed' }) + ) + ).toMatchObject({ tone: 'failed', statusLabel: 'Failed' }) + expect( + getExternalAutomationLastRunSnapshot( + makeJob({ lastRunAt: '2026-08-12T01:00:00Z', lastStatus: 'ok' }) + ) + ).toMatchObject({ tone: 'succeeded', statusLabel: 'Done' }) + expect( + getExternalAutomationLastRunSnapshot(makeJob({ lastError: 'boom', lastStatus: null })) + ).toMatchObject({ tone: 'failed', statusLabel: 'Failed' }) + expect( + getExternalAutomationLastRunSnapshot( + makeJob({ lastRunAt: '2026-08-12T01:00:00Z', lastStatus: 'ok', lastError: 'stale' }) + ) + ).toMatchObject({ tone: 'succeeded', statusLabel: 'Done' }) + expect(getExternalAutomationLastRunSnapshot(makeJob()).tone).toBe('never') + }) + + it('formats last-run cells as status plus relative time', () => { + const now = Date.parse('2026-08-12T10:00:00Z') + const failed = formatAutomationLastRunCell( + { at: now - 8 * 60 * 60 * 1000, tone: 'failed', statusLabel: 'Failed' }, + now + ) + expect(failed.text).toBe('Failed 8h ago') + expect(failed.tone).toBe('failed') + expect( + formatAutomationLastRunCell({ at: null, tone: 'never', statusLabel: '' }, now).text + ).toBe('Never') + const failedNoTime = formatAutomationLastRunCell( + { at: null, tone: 'failed', statusLabel: 'Failed' }, + now + ) + expect(failedNoTime.text).toBe('Failed') + expect(failedNoTime.tone).toBe('failed') + }) +}) diff --git a/src/renderer/src/components/automations/automation-list-last-run.ts b/src/renderer/src/components/automations/automation-list-last-run.ts new file mode 100644 index 00000000000..7d5127337fa --- /dev/null +++ b/src/renderer/src/components/automations/automation-list-last-run.ts @@ -0,0 +1,152 @@ +import { translate } from '@/i18n/i18n' +import type { + Automation, + AutomationRun, + AutomationRunStatus, + ExternalAutomationJob +} from '../../../../shared/automations-types' +import { + formatAutomationDateTime, + formatAutomationRelativeTime, + getAutomationRunStatusLabel +} from './automation-page-parts' + +export type AutomationLastRunTone = + | 'failed' + | 'succeeded' + | 'running' + | 'skipped' + | 'never' + | 'unknown' + +export type AutomationLastRunSnapshot = { + at: number | null + tone: AutomationLastRunTone + statusLabel: string +} + +const EXTERNAL_FAILED_STATUSES = new Set(['failed', 'fail', 'error', 'dispatch_failed']) +const EXTERNAL_SUCCEEDED_STATUSES = new Set([ + 'ok', + 'completed', + 'success', + 'succeeded', + 'done', + 'healthy' +]) + +export function indexLatestAutomationRuns( + runs: readonly AutomationRun[] +): ReadonlyMap { + const latest = new Map() + for (const run of runs) { + const existing = latest.get(run.automationId) + if (!existing || run.createdAt > existing.createdAt) { + latest.set(run.automationId, run) + } + } + return latest +} + +export function getAutomationRunLastRunAt(run: AutomationRun): number { + return run.dispatchedAt ?? run.startedAt ?? run.createdAt +} + +export function getToneForAutomationRunStatus(status: AutomationRunStatus): AutomationLastRunTone { + if (status === 'dispatch_failed') { + return 'failed' + } + if (status === 'completed') { + return 'succeeded' + } + if (status === 'pending' || status === 'dispatching' || status === 'dispatched') { + return 'running' + } + if (status.startsWith('skipped')) { + return 'skipped' + } + return 'unknown' +} + +export function getLocalAutomationLastRunSnapshot( + automation: Automation, + lastRun: AutomationRun | undefined +): AutomationLastRunSnapshot { + if (lastRun) { + return { + at: getAutomationRunLastRunAt(lastRun), + tone: getToneForAutomationRunStatus(lastRun.status), + statusLabel: getAutomationRunStatusLabel(lastRun.status) + } + } + if (automation.lastRunAt) { + return { + at: automation.lastRunAt, + tone: 'unknown', + statusLabel: '' + } + } + return { at: null, tone: 'never', statusLabel: '' } +} + +function parseExternalLastRunAt(value: string | null): number | null { + if (!value) { + return null + } + const parsed = Date.parse(value) + return Number.isFinite(parsed) ? parsed : null +} + +export function getExternalAutomationLastRunSnapshot( + job: ExternalAutomationJob +): AutomationLastRunSnapshot { + const at = parseExternalLastRunAt(job.lastRunAt) + const raw = job.lastStatus?.trim() ?? '' + const normalized = raw.toLowerCase() + if (!at && !raw && !job.lastError) { + return { at: null, tone: 'never', statusLabel: '' } + } + if (EXTERNAL_SUCCEEDED_STATUSES.has(normalized)) { + return { + at, + tone: 'succeeded', + statusLabel: translate('auto.components.automations.automation.list.last.run.done', 'Done') + } + } + if (EXTERNAL_FAILED_STATUSES.has(normalized) || Boolean(job.lastError)) { + return { + at, + tone: 'failed', + statusLabel: translate( + 'auto.components.automations.automation.list.last.run.failed', + 'Failed' + ) + } + } + if (raw) { + return { at, tone: 'unknown', statusLabel: raw } + } + return { at, tone: 'unknown', statusLabel: '' } +} + +export function formatAutomationLastRunCell( + snapshot: AutomationLastRunSnapshot, + now: number +): { text: string; title: string; tone: AutomationLastRunTone } { + const status = snapshot.statusLabel.trim() + if (snapshot.tone === 'never') { + const never = formatAutomationDateTime(null) + return { text: never, title: never, tone: 'never' } + } + if (snapshot.at == null) { + return { text: status || formatAutomationDateTime(null), title: status, tone: snapshot.tone } + } + const relative = formatAutomationRelativeTime(snapshot.at, now) + const absolute = formatAutomationDateTime(snapshot.at) + const text = status && relative ? `${status} ${relative}` : status || relative || absolute + return { + text, + title: status ? `${status} · ${absolute}` : absolute, + tone: snapshot.tone + } +} diff --git a/src/renderer/src/components/automations/automation-list-view.test.ts b/src/renderer/src/components/automations/automation-list-view.test.ts new file mode 100644 index 00000000000..01a0cfa7799 --- /dev/null +++ b/src/renderer/src/components/automations/automation-list-view.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, it } from 'vitest' +import type { + Automation, + AutomationRun, + ExternalAutomationJob, + ExternalAutomationManager +} from '../../../../shared/automations-types' +import { + applyAutomationListView, + countAutomationListFilters, + isAutomationListFilterActive, + nextAutomationListSort +} from './automation-list-view' +import type { ExternalAutomationListEntry } from './external-automation-list-entries' + +function makeAutomation(overrides: Partial = {}): Automation { + return { + id: 'automation-1', + name: 'Zebra job', + prompt: 'run', + precheck: null, + agentId: 'codex', + projectId: 'repo-1', + executionTargetType: 'local', + executionTargetId: 'local', + schedulerOwner: 'local_host_service', + workspaceMode: 'existing', + workspaceId: 'worktree-1', + baseBranch: null, + reuseSession: false, + timezone: 'UTC', + rrule: 'FREQ=DAILY', + dtstart: 1, + enabled: true, + nextRunAt: 2, + missedRunPolicy: 'run_once_within_grace', + missedRunGraceMinutes: 720, + createdAt: 1, + updatedAt: 1, + ...overrides + } +} + +function makeRun(overrides: Partial = {}): AutomationRun { + return { + id: 'run-1', + automationId: 'automation-1', + title: 'Zebra job', + scheduledFor: 10, + status: 'completed', + trigger: 'scheduled', + workspaceId: 'worktree-1', + sessionKind: 'terminal', + chatSessionId: null, + terminalSessionId: null, + terminalPaneKey: null, + terminalPtyId: null, + outputSnapshot: null, + precheckResult: null, + usage: null, + error: null, + startedAt: 20, + dispatchedAt: 30, + createdAt: 10, + ...overrides + } +} + +function makeExternalEntry( + overrides: Partial = {} +): ExternalAutomationListEntry { + const manager: ExternalAutomationManager = { + id: 'manager-1', + provider: 'hermes', + label: 'Local Hermes', + targetLabel: 'Local', + target: { type: 'local' }, + status: 'available', + error: null, + canManage: true, + jobs: [] + } + const job: ExternalAutomationJob = { + id: 'job-1', + managerId: manager.id, + provider: 'hermes', + name: 'Alpha digest', + schedule: '0 9 * * 1-5', + rawSchedule: null, + enabled: true, + state: 'enabled', + prompt: null, + promptPreview: '', + nextRunAt: null, + lastRunAt: '2026-08-12T01:00:00Z', + lastStatus: 'failed', + lastError: null, + workdir: null, + runCount: 1, + runs: [], + ...overrides + } + return { key: `${manager.id}:${job.id}`, manager, job } +} + +describe('automation-list-view', () => { + it('counts and detects active filters', () => { + expect(isAutomationListFilterActive({ status: 'all', lastRun: 'all' })).toBe(false) + expect(isAutomationListFilterActive({ status: 'paused', lastRun: 'all' })).toBe(true) + expect(countAutomationListFilters({ status: 'paused', lastRun: 'failed' })).toBe(2) + }) + + it('toggles sort direction and defaults last run to newest first', () => { + expect(nextAutomationListSort(null, 'name')).toEqual({ field: 'name', direction: 'asc' }) + expect(nextAutomationListSort(null, 'lastRun')).toEqual({ field: 'lastRun', direction: 'desc' }) + expect(nextAutomationListSort({ field: 'name', direction: 'asc' }, 'name')).toEqual({ + field: 'name', + direction: 'desc' + }) + expect(nextAutomationListSort({ field: 'name', direction: 'asc' }, 'lastRun')).toEqual({ + field: 'lastRun', + direction: 'desc' + }) + }) + + it('filters by enabled state and last-run outcome', () => { + const items = applyAutomationListView({ + automations: [ + makeAutomation({ id: 'paused', name: 'Paused', enabled: false }), + makeAutomation({ id: 'ok', name: 'Healthy' }) + ], + externalEntries: [makeExternalEntry()], + runs: [ + makeRun({ automationId: 'paused', status: 'completed' }), + makeRun({ automationId: 'ok', status: 'dispatch_failed' }) + ], + filter: { status: 'enabled', lastRun: 'failed' }, + sort: null + }) + expect(items.map((item) => item.id)).toEqual(['ok', 'manager-1:job-1']) + }) + + it('sorts by name across local and external rows', () => { + const items = applyAutomationListView({ + automations: [makeAutomation({ name: 'Zebra job' })], + externalEntries: [makeExternalEntry({ name: 'Alpha digest' })], + runs: [], + filter: { status: 'all', lastRun: 'all' }, + sort: { field: 'name', direction: 'asc' } + }) + expect(items.map((item) => item.name)).toEqual(['Alpha digest', 'Zebra job']) + }) + + it('sorts by last run newest first and keeps never-run rows last', () => { + const items = applyAutomationListView({ + automations: [ + makeAutomation({ id: 'old', name: 'Old' }), + makeAutomation({ id: 'never', name: 'Never' }) + ], + externalEntries: [makeExternalEntry({ lastRunAt: '2026-08-12T09:00:00Z' })], + runs: [makeRun({ automationId: 'old', dispatchedAt: Date.parse('2026-08-11T09:00:00Z') })], + filter: { status: 'all', lastRun: 'all' }, + sort: { field: 'lastRun', direction: 'desc' } + }) + expect(items.map((item) => item.id)).toEqual(['manager-1:job-1', 'old', 'never']) + }) +}) diff --git a/src/renderer/src/components/automations/automation-list-view.ts b/src/renderer/src/components/automations/automation-list-view.ts new file mode 100644 index 00000000000..49d44aadb3d --- /dev/null +++ b/src/renderer/src/components/automations/automation-list-view.ts @@ -0,0 +1,191 @@ +import { getIntlLocale } from '@/i18n/i18n' +import type { Automation, AutomationRun } from '../../../../shared/automations-types' +import type { ExternalAutomationListEntry } from './external-automation-list-entries' +import { + getExternalAutomationLastRunSnapshot, + getLocalAutomationLastRunSnapshot, + indexLatestAutomationRuns, + type AutomationLastRunSnapshot +} from './automation-list-last-run' + +export type AutomationListStatusFilter = 'all' | 'enabled' | 'paused' +export type AutomationListLastRunFilter = 'all' | 'failed' | 'succeeded' | 'never' +export type AutomationListSortField = 'name' | 'lastRun' +export type AutomationListSortDirection = 'asc' | 'desc' + +export type AutomationListSort = { + field: AutomationListSortField + direction: AutomationListSortDirection +} + +export type AutomationListViewItem = + | { + kind: 'local' + id: string + name: string + enabled: boolean + lastRunAt: number | null + lastRun: AutomationLastRunSnapshot + automation: Automation + } + | { + kind: 'external' + id: string + name: string + enabled: boolean + lastRunAt: number | null + lastRun: AutomationLastRunSnapshot + entry: ExternalAutomationListEntry + } + +export type AutomationListFilter = { + status: AutomationListStatusFilter + lastRun: AutomationListLastRunFilter +} + +export const EMPTY_AUTOMATION_LIST_FILTER: AutomationListFilter = { + status: 'all', + lastRun: 'all' +} + +export function isAutomationListFilterActive(filter: AutomationListFilter): boolean { + return filter.status !== 'all' || filter.lastRun !== 'all' +} + +export function countAutomationListFilters(filter: AutomationListFilter): number { + return (filter.status !== 'all' ? 1 : 0) + (filter.lastRun !== 'all' ? 1 : 0) +} + +export function defaultAutomationListSortDirection( + field: AutomationListSortField +): AutomationListSortDirection { + return field === 'lastRun' ? 'desc' : 'asc' +} + +export function nextAutomationListSort( + current: AutomationListSort | null, + field: AutomationListSortField +): AutomationListSort { + if (current?.field !== field) { + return { field, direction: defaultAutomationListSortDirection(field) } + } + return { + field, + direction: current.direction === 'asc' ? 'desc' : 'asc' + } +} + +function matchesStatusFilter(enabled: boolean, filter: AutomationListStatusFilter): boolean { + if (filter === 'all') { + return true + } + return filter === 'enabled' ? enabled : !enabled +} + +function matchesLastRunFilter( + snapshot: AutomationLastRunSnapshot, + filter: AutomationListLastRunFilter +): boolean { + if (filter === 'all') { + return true + } + return snapshot.tone === filter +} + +export function buildAutomationListViewItems({ + automations, + externalEntries, + runs +}: { + automations: readonly Automation[] + externalEntries: readonly ExternalAutomationListEntry[] + runs: readonly AutomationRun[] +}): AutomationListViewItem[] { + const lastRunByAutomationId = indexLatestAutomationRuns(runs) + const locals: AutomationListViewItem[] = automations.map((automation) => { + const lastRun = getLocalAutomationLastRunSnapshot( + automation, + lastRunByAutomationId.get(automation.id) + ) + return { + kind: 'local', + id: automation.id, + name: automation.name, + enabled: automation.enabled, + lastRunAt: lastRun.at, + lastRun, + automation + } + }) + const externals: AutomationListViewItem[] = externalEntries.map((entry) => { + const lastRun = getExternalAutomationLastRunSnapshot(entry.job) + return { + kind: 'external', + id: entry.key, + name: entry.job.name, + enabled: entry.job.enabled, + lastRunAt: lastRun.at, + lastRun, + entry + } + }) + return [...locals, ...externals] +} + +export function filterAutomationListViewItems( + items: readonly AutomationListViewItem[], + filter: AutomationListFilter +): AutomationListViewItem[] { + if (!isAutomationListFilterActive(filter)) { + return [...items] + } + return items.filter( + (item) => + matchesStatusFilter(item.enabled, filter.status) && + matchesLastRunFilter(item.lastRun, filter.lastRun) + ) +} + +export function sortAutomationListViewItems( + items: readonly AutomationListViewItem[], + sort: AutomationListSort | null +): AutomationListViewItem[] { + if (!sort) { + return [...items] + } + const next = [...items] + const locale = getIntlLocale() + next.sort((left, right) => { + const compared = + sort.field === 'name' + ? left.name.localeCompare(right.name, locale, { sensitivity: 'base' }) + : (left.lastRunAt ?? 0) - (right.lastRunAt ?? 0) + if (compared !== 0) { + return sort.direction === 'asc' ? compared : -compared + } + return left.id.localeCompare(right.id) + }) + return next +} + +export function applyAutomationListView({ + automations, + externalEntries, + runs, + filter, + sort +}: { + automations: readonly Automation[] + externalEntries: readonly ExternalAutomationListEntry[] + runs: readonly AutomationRun[] + filter: AutomationListFilter + sort: AutomationListSort | null +}): AutomationListViewItem[] { + return sortAutomationListViewItems( + filterAutomationListViewItems( + buildAutomationListViewItems({ automations, externalEntries, runs }), + filter + ), + sort + ) +} diff --git a/src/renderer/src/components/automations/automations-table-layout.ts b/src/renderer/src/components/automations/automations-table-layout.ts index 17f3af46681..b32c09d9fac 100644 --- a/src/renderer/src/components/automations/automations-table-layout.ts +++ b/src/renderer/src/components/automations/automations-table-layout.ts @@ -1,8 +1,8 @@ /** Shared layout classes for the automations list table. * Matches AutomationRunHistory / ExternalAutomationRunTable / Tasks list tables. */ -// Name | Schedule | Project | Next run | Status | Agent | Actions +// Name | Schedule | Project | Next run | Last run | Status | Agent | Actions export const AUTOMATIONS_TABLE_GRID_CLASS = - 'grid grid-cols-[minmax(0,1.5fr)_minmax(7rem,10rem)_minmax(5rem,9rem)_minmax(10rem,1.2fr)_minmax(4.5rem,6rem)_2.5rem_2.5rem]' + 'grid grid-cols-[minmax(0,1.4fr)_minmax(7.5rem,10rem)_minmax(4.5rem,8rem)_minmax(8.5rem,1fr)_minmax(8rem,11rem)_minmax(4.5rem,6rem)_2.5rem_2.5rem]' export const AUTOMATIONS_TABLE_CONTAINER_CLASS = 'rounded-md border border-border/50 bg-muted/20' diff --git a/src/renderer/src/components/automations/use-automation-list-view.ts b/src/renderer/src/components/automations/use-automation-list-view.ts new file mode 100644 index 00000000000..f87550c4079 --- /dev/null +++ b/src/renderer/src/components/automations/use-automation-list-view.ts @@ -0,0 +1,98 @@ +import { useEffect, useMemo } from 'react' +import type { Automation, AutomationRun } from '../../../../shared/automations-types' +import type { ExternalAutomationListEntry } from './external-automation-list-entries' +import { + applyAutomationListView, + isAutomationListFilterActive, + type AutomationListFilter, + type AutomationListSort, + type AutomationListViewItem +} from './automation-list-view' + +export function useAutomationListView({ + automations, + externalEntries, + runs, + filter, + sort, + selectedId, + selectedExternalKey, + selectAutomationId, + selectExternalKey +}: { + automations: readonly Automation[] + externalEntries: readonly ExternalAutomationListEntry[] + runs: readonly AutomationRun[] + filter: AutomationListFilter + sort: AutomationListSort | null + selectedId: string | null + selectedExternalKey: string | null + selectAutomationId: (automationId: string | null) => void + selectExternalKey: (externalKey: string | null) => void +}): { + visibleItems: readonly AutomationListViewItem[] + isListFilterActive: boolean + hasVisibleListItems: boolean +} { + const isListFilterActive = isAutomationListFilterActive(filter) + const visibleItems = useMemo( + () => + applyAutomationListView({ + automations, + externalEntries, + runs, + filter, + sort + }), + [automations, externalEntries, filter, runs, sort] + ) + const hasVisibleListItems = visibleItems.length > 0 + + // Why: search already hides rows; filter/sort must keep highlight + detail on a + // still-visible row so the list never points at something the user cannot see. + useEffect(() => { + if (!isListFilterActive && !sort) { + return + } + const localVisible = + selectedExternalKey === null && + selectedId != null && + visibleItems.some((item) => item.kind === 'local' && item.id === selectedId) + const externalVisible = + selectedExternalKey != null && + visibleItems.some((item) => item.kind === 'external' && item.id === selectedExternalKey) + if (localVisible || externalVisible) { + return + } + const first = visibleItems[0] + if (!first) { + return + } + if (first.kind === 'local') { + if (selectedExternalKey !== null) { + selectExternalKey(null) + } + if (selectedId !== first.id) { + selectAutomationId(first.id) + } + return + } + if (selectedExternalKey !== first.id) { + selectExternalKey(first.id) + } + }, [ + isListFilterActive, + selectAutomationId, + selectExternalKey, + selectedExternalKey, + selectedId, + sort, + visibleItems + ]) + + return { + visibleItems, + isListFilterActive, + hasVisibleListItems + } +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 921f3ab76c5..edf9789cae0 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -14471,7 +14471,9 @@ "tableStatus": "Status", "tableActions": "Actions", "newAutomation": "New Automation", - "rowActions": "Automation actions" + "rowActions": "Automation actions", + "tableLastRun": "Last run", + "noListMatches": "No automations match." }, "AutomationsPageSkeleton": { "55527b7bcf": "Loading automations" @@ -14565,6 +14567,14 @@ "name": "Hourly maintenance check", "prompt": "Check for stuck work, stale generated files, failing validation, and anything that needs human attention. Report only actionable issues." } + }, + "list": { + "last": { + "run": { + "done": "Done", + "failed": "Failed" + } + } } }, "external": { @@ -14597,6 +14607,19 @@ }, "AutomationListLocalRows": { "c92c9463c6": "Automation actions" + }, + "AutomationListFilterMenu": { + "removeFilter": "Remove {{value0}} filter", + "failed": "Failed", + "succeeded": "Succeeded", + "neverRan": "Never ran", + "filters": "Filters", + "all": "All", + "clear": "Clear filters" + }, + "AutomationListSortHeader": { + "sortedAscending": "{{value0}}, sorted ascending", + "sortedDescending": "{{value0}}, sorted descending" } }, "agent": { From e5906837e42c941432e0fb5c84012c7a2288e438 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:40:47 -0700 Subject: [PATCH 8/9] perf(browser): clear cookies in one bulk call that excludes the Google family (STA-4065) (#14131) * perf(browser): bulk clear cookies when an import excludes nothing (STA-4065) PR #14086 replaced the bulk clear with one Electron cookies.remove() per removable cookie so partitioned excluded cookies survive an import. That is required whenever something must be preserved, but it also runs when nothing is excluded, where a single clearStorageData is equivalent and far cheaper: 421ms -> 7.3ms at 1k cookies and 1764ms -> 20.1ms at 3k on Electron 43. The fast path is gated on the same unfiltered cookies.get() the exclusion predicate already reads, which an Electron fixture now proves returns partitioned cookies, so "no exclusions in the snapshot" means "nothing in the jar to preserve". * fix(browser): protect cookie clear fast-path race (STA-4065) * test(browser): keep the no-bulk-clear assertion honest after the clearData switch The mocked session still exposed clearStorageData, so asserting it was never called could no longer fail once the fast path moved to clearData. * docs(browser): stop the fallback comment from asserting a rollback The per-cookie path's rollback is being removed by STA-4061; the comment only needs to say the fallback re-removes what survived. * test(browser): keep the STA-4061 rollback fixture on the per-cookie path The bulk fast path clears a jar with no excluded cookie in one call, which would have made every assertion in the partition-rollback fixture vacuous. Give it a live Google cookie so the partial-failure clear still runs, and assert the bulk call count is zero so a future widening fails loudly. * perf(browser): always bulk clear cookies, not only when nothing is excluded The fast path only fired when the jar held no Google cookie, but this import exists for Google, so a Google cookie is normally present and the common case still paid one native remove per cookie. Measured on Electron 43, clearData excludeOrigins preserves the whole google.com registrable family -- host, leading-dot, subdomain and partitioned cookies -- so the bulk call is safe with cookies to keep, and the gate was never load-bearing for preservation. Drop the caller-supplied exclusion predicate so the preserved set has exactly one source of truth: a predicate that disagreed with NON_TRANSPLANTABLE_DOMAINS would have deleted cookies the bulk call is meant to keep. The per-cookie loop stays as the fallback for a rejected bulk clear, carrying the same exclusion. * fix(browser): re-read the cookie jar after a rejected bulk clear clearData can empty part of the jar before rejecting, so the per-cookie fallback was working from a snapshot taken before the attempt: it retried removals for cookies that were already gone and could not see cookies that arrived during the clear. Re-read on rejection only. Also repairs three fixtures that mocked the session with clearStorageData after the call moved to clearData. The missing method threw, the catch swallowed it, and every one of those tests silently asserted per-cookie behaviour while believing it covered exclusion. Records the registrable-domain invariant at NON_TRANSPLANTABLE_DOMAINS, since excludeOrigins derives one origin per entry, and extends the real-Electron fixture to prove http-only and domain-scoped Google cookies survive alongside the partitioned one. --- ...ser-cookie-import-google-exclusion.test.ts | 20 +- ...import-partition-rollback.electron.test.ts | 28 ++- ...r-cookie-import-partition.electron.test.ts | 21 +- .../browser-cookie-import-policy.test.ts | 229 +++++++++++++----- .../browser/browser-cookie-import-policy.ts | 44 +++- .../browser-cookie-import-replacement.test.ts | 8 +- .../browser/browser-cookie-import.test.ts | 8 +- src/main/browser/browser-cookie-import.ts | 6 +- 8 files changed, 279 insertions(+), 85 deletions(-) diff --git a/src/main/browser/browser-cookie-import-google-exclusion.test.ts b/src/main/browser/browser-cookie-import-google-exclusion.test.ts index 3cc329a14d7..431ca758f2f 100644 --- a/src/main/browser/browser-cookie-import-google-exclusion.test.ts +++ b/src/main/browser/browser-cookie-import-google-exclusion.test.ts @@ -120,7 +120,7 @@ describe('file import excludes the Google cookie family', () => { }) describe('native Chromium import excludes the Google cookie family', () => { - let clearStorageDataMock: ReturnType + let clearDataMock: ReturnType let cookiesGetMock: ReturnType let cookiesRemoveMock: ReturnType let cookiesSetMock: ReturnType @@ -134,7 +134,7 @@ describe('native Chromium import excludes the Google cookie family', () => { execFileSyncMock.mockImplementation(() => { throw new Error('OS browser version lookup unavailable') }) - clearStorageDataMock = vi.fn().mockResolvedValue(undefined) + clearDataMock = vi.fn().mockResolvedValue(undefined) cookiesGetMock = vi.fn().mockResolvedValue([]) cookiesRemoveMock = vi.fn().mockResolvedValue(undefined) cookiesSetMock = vi.fn().mockResolvedValue(undefined) @@ -145,7 +145,7 @@ describe('native Chromium import excludes the Google cookie family', () => { remove: cookiesRemoveMock, set: cookiesSetMock }, - clearStorageData: clearStorageDataMock + clearData: clearDataMock }) platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') }) @@ -166,7 +166,7 @@ describe('native Chromium import excludes the Google cookie family', () => { createChromiumCookieTestDatabase(targetCookiesPath, rows).close() } - it('never clears or reconstructs live Google cookies before importing', async () => { + it('bulk clears while excluding live Google cookies before importing', async () => { const sourceCookiesPath = seedSource([ { domain: '.google.com', name: 'SID', value: 'transplanted-sid' }, { domain: '.example.com', name: 'session', value: 'new' } @@ -186,9 +186,10 @@ describe('native Chromium import excludes the Google cookie family', () => { googleCookiesSkipped: 1, domains: ['example.com'] }) - expect(clearStorageDataMock).not.toHaveBeenCalled() - expect(cookiesRemoveMock).toHaveBeenCalledOnce() - expect(cookiesRemoveMock).toHaveBeenCalledWith('https://example.com/', 'stale') + expect(clearDataMock.mock.calls).toEqual([ + [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + ]) + expect(cookiesRemoveMock).not.toHaveBeenCalled() expect(cookiesSetMock.mock.calls.map(([details]) => details.domain)).toEqual(['.example.com']) }) @@ -239,7 +240,7 @@ describe('native Chromium import excludes the Google cookie family', () => { domains: [] }) expect(execFileSyncMock).not.toHaveBeenCalled() - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).not.toHaveBeenCalled() expect(cookiesSetMock).not.toHaveBeenCalled() }) @@ -284,12 +285,13 @@ describe('native Chromium import excludes the Google cookie family', () => { throw new Error('cookie store unavailable') } }) + clearDataMock.mockRejectedValue(new Error('storage busy')) const result = await importCookiesFromBrowser(chromeBrowser(sourceCookiesPath), 'persist:test') expect(result).toMatchObject({ ok: false }) expect(result.ok || result.reason).toContain('Could not clear existing cookies') - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).toHaveBeenCalledOnce() expect(cookiesRemoveMock.mock.calls.map(([, name]) => name)).toEqual(['removed-first', 'stale']) expect(cookiesSetMock).not.toHaveBeenCalled() expect(setPendingCookieImportMock).not.toHaveBeenCalled() diff --git a/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts b/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts index 5ee11981251..bbd0d9a8a25 100644 --- a/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts +++ b/src/main/browser/browser-cookie-import-partition-rollback.electron.test.ts @@ -20,8 +20,10 @@ type CdpCookie = { name: string; value: string; partitionKey?: Record | undefined clearError: string | null + bulkClearCalls: number remainingChips: CdpCookie[] remainingPlain: CdpCookie[] + remainingExcluded: CdpCookie[] } const EXPECTED_PARTITION_KEY = { @@ -33,7 +35,7 @@ function buildFixtureMain(policyPath: string, resultPath: string): string { return ` const { app, BrowserWindow, session } = require('electron') const { writeFileSync } = require('node:fs') -const { removeAllCookiesExcept, isNonTransplantableCookieDomain } = require(${JSON.stringify(policyPath)}) +const { removeTransplantableCookies } = require(${JSON.stringify(policyPath)}) const resultPath = ${JSON.stringify(resultPath)} let currentStep = 'starting' const mark = (step) => { @@ -76,6 +78,9 @@ async function run() { await targetSession.cookies.set({ url: 'https://plain.example/', name: 'plain', value: 'stale', secure: true }) await targetSession.cookies.set({ url: 'https://victim.example/', name: 'victim', value: 'stale', secure: true }) + // Why: keep a live excluded cookie in the partial-failure fixture so the fallback proves it + // preserves that cookie while deleting the removable ones it can reach. + await targetSession.cookies.set({ url: 'https://accounts.google.com/', name: 'SID', value: 'live', secure: true }) mark('removable cookies set') const store = { @@ -88,9 +93,20 @@ async function run() { } } + // Why: the bulk clear is the ordinary path now, so rejecting it is what routes this fixture onto + // the per-cookie fallback where a partial failure — and the old rollback — could happen at all. + let bulkClearCalls = 0 + const clearSession = { + cookies: store, + clearData: async () => { + bulkClearCalls++ + throw new Error('forced bulk clear failure') + } + } + let clearError = null try { - await removeAllCookiesExcept(store, (cookie) => isNonTransplantableCookieDomain(cookie.domain ?? '')) + await removeTransplantableCookies(clearSession) } catch (error) { clearError = String(error?.message || error) } @@ -104,8 +120,10 @@ async function run() { writeFileSync(resultPath, JSON.stringify({ beforePartitionKey: beforeChips.partitionKey, clearError, + bulkClearCalls, remainingChips: project('chips-auth'), - remainingPlain: project('plain') + remainingPlain: project('plain'), + remainingExcluded: project('SID') })) debug.detach() window.destroy() @@ -164,6 +182,10 @@ describe('non-Google partitioned cookie under a failed Electron cookie clear', ( const result = await runFixture() expect(result.beforePartitionKey).toEqual(EXPECTED_PARTITION_KEY) + // Why (STA-4065): the fallback is only reachable once the bulk clear has been tried and failed; + // if it ever succeeded here the partial-failure assertions below would be vacuous. + expect(result.bulkClearCalls).toBe(1) + expect(result.remainingExcluded.map(({ name }) => name)).toEqual(['SID']) expect(result.clearError).toContain('Could not clear existing cookies') // STA-4061: rollback rebuilt this cookie through cookies.set, which drops partitionKey. expect( diff --git a/src/main/browser/browser-cookie-import-partition.electron.test.ts b/src/main/browser/browser-cookie-import-partition.electron.test.ts index 494ebbd9e2b..e844341e12f 100644 --- a/src/main/browser/browser-cookie-import-partition.electron.test.ts +++ b/src/main/browser/browser-cookie-import-partition.electron.test.ts @@ -74,6 +74,20 @@ async function run() { value: 'remove-me', secure: true }) + // Why: the excluded origin is HTTPS, so prove registrable-domain matching also keeps HTTP. + await targetSession.cookies.set({ + url: 'http://mail.google.com/', + name: 'http-google', + value: 'live-http-value', + secure: false + }) + await targetSession.cookies.set({ + url: 'https://google.com/', + domain: '.google.com', + name: 'domain-google', + value: 'live-domain-value', + secure: true + }) mark('ordinary cookie set') const beforeCookies = (await debug.sendCommand('Network.getAllCookies')).cookies @@ -189,6 +203,11 @@ describe('native Chromium excluded partition cookie under Electron', () => { ok: true, summary: { importedCookies: 1, domains: ['example.com'] } }) - expect(result.remainingNames).toEqual(['imported', 'partitioned-google']) + expect(result.remainingNames).toEqual([ + 'domain-google', + 'http-google', + 'imported', + 'partitioned-google' + ]) }, 90_000) }) diff --git a/src/main/browser/browser-cookie-import-policy.test.ts b/src/main/browser/browser-cookie-import-policy.test.ts index f007c07d8ff..51a1a6107d5 100644 --- a/src/main/browser/browser-cookie-import-policy.test.ts +++ b/src/main/browser/browser-cookie-import-policy.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it, vi, type Mock } from 'vitest' import { DatabaseSync } from 'node:sqlite' import type { Cookie } from 'electron' import { @@ -6,7 +6,7 @@ import { isNonTransplantableCookieDomain, NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, - removeAllCookiesExcept, + removeTransplantableCookies, replaceCookiesForImportedDomains } from './browser-cookie-import-policy' @@ -207,78 +207,193 @@ describe('NON_TRANSPLANTABLE_HOST_KEY_SQL', () => { }) }) -describe('removeAllCookiesExcept', () => { - it('never removes or reconstructs excluded Google cookies', async () => { - const get = vi - .fn() - .mockResolvedValue([ - cookie('.google.com', 'SID'), - cookie('accounts.google.com', 'ACCOUNT'), - cookie('.example.com', 'session'), - cookie('other.test', 'tracker', '/scoped') - ]) - const remove = vi.fn().mockResolvedValue(undefined) - const set = vi.fn().mockResolvedValue(undefined) +type CookieClearMocks = { + get: Mock + remove: Mock + set: Mock + clearData: Mock +} - await removeAllCookiesExcept({ get, remove }, (existingCookie) => - isNonTransplantableCookieDomain(existingCookie.domain ?? '') +describe('removeTransplantableCookies', () => { + const rejectingBulkClear = () => vi.fn().mockRejectedValue(new Error('storage busy')) + + function clearSession(cookies: Cookie[], overrides: Partial = {}) { + const store = { + get: vi.fn().mockResolvedValue(cookies), + remove: vi.fn().mockResolvedValue(undefined), + set: vi.fn().mockResolvedValue(undefined), + ...overrides + } + const clearData = overrides.clearData ?? vi.fn().mockResolvedValue(undefined) + return { + session: { cookies: store, clearData }, + get: store.get, + remove: store.remove, + set: store.set, + clearData + } + } + + // Why (STA-4065): the bulk call is the ordinary path even when the jar holds cookies to keep — + // excludeOrigins preserves the whole google.com family, verified against real Electron. + it('clears a jar holding Google cookies in one call that excludes them', async () => { + const { session, remove, set, clearData } = clearSession([ + cookie('.google.com', 'SID'), + cookie('accounts.google.com', 'ACCOUNT'), + cookie('.example.com', 'session'), + cookie('other.test', 'tracker', '/scoped') + ]) + + await removeTransplantableCookies(session) + + expect(clearData.mock.calls).toEqual([ + [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + ]) + expect(remove).not.toHaveBeenCalled() + expect(set).not.toHaveBeenCalled() + }) + + it('bulk clears in one call when the jar holds nothing to preserve', async () => { + const { session, remove, set, clearData } = clearSession([ + cookie('.example.com', 'session'), + cookie('other.test', 'tracker', '/scoped'), + cookie('notgoogle.com', 'lookalike') + ]) + + await removeTransplantableCookies(session) + + expect(clearData.mock.calls).toEqual([ + [{ dataTypes: ['cookies'], excludeOrigins: ['https://google.com'] }] + ]) + expect(remove).not.toHaveBeenCalled() + expect(set).not.toHaveBeenCalled() + }) + + it('touches nothing when the jar is already empty', async () => { + const { session, remove, clearData } = clearSession([]) + + await removeTransplantableCookies(session) + + expect(clearData).not.toHaveBeenCalled() + expect(remove).not.toHaveBeenCalled() + }) + + it('bulk clears cookies the per-cookie path could never address', async () => { + const { session, clearData } = clearSession([ + { ...cookie('.example.com', 'session'), domain: '' } + ]) + + await removeTransplantableCookies(session) + + expect(clearData).toHaveBeenCalledOnce() + }) + + it('falls back to per-cookie removal when the bulk clear rejects', async () => { + const { session, remove, clearData } = clearSession( + [cookie('.example.com', 'session'), cookie('other.test', 'tracker')], + { clearData: rejectingBulkClear() } ) + await removeTransplantableCookies(session) + + expect(clearData).toHaveBeenCalledOnce() expect(remove.mock.calls).toEqual([ ['https://example.com/', 'session'], - ['https://other.test/scoped', 'tracker'] + ['https://other.test/', 'tracker'] ]) + }) + + it('re-reads the jar after a rejected bulk clear', async () => { + const beforeAttempt = [cookie('.removed.test', 'gone-before-fallback')] + const afterAttempt = [ + cookie('.google.com', 'SID'), + cookie('.survivor.test', 'survived'), + cookie('.arrived.test', 'arrived-during-clear') + ] + const get = vi.fn().mockResolvedValueOnce(beforeAttempt).mockResolvedValueOnce(afterAttempt) + const { session, remove } = clearSession(beforeAttempt, { + get, + clearData: rejectingBulkClear() + }) + + await removeTransplantableCookies(session) + + expect(get).toHaveBeenCalledTimes(2) + expect(remove.mock.calls).toEqual([ + ['https://survivor.test/', 'survived'], + ['https://arrived.test/', 'arrived-during-clear'] + ]) + }) + + // Why: the fallback carries the same exclusion as the bulk call, so a rejected clearData must + // not become the path that finally deletes a live Google session. + it('still preserves Google cookies on the per-cookie fallback', async () => { + const { session, remove, set } = clearSession( + [ + cookie('.google.com', 'SID'), + cookie('accounts.google.com', 'ACCOUNT'), + cookie('.example.com', 'session') + ], + { clearData: rejectingBulkClear() } + ) + + await removeTransplantableCookies(session) + + expect(remove.mock.calls).toEqual([['https://example.com/', 'session']]) expect(set).not.toHaveBeenCalled() }) // Why (STA-4061): reconstructing a removed cookie loses its partition key, and the snapshot // cannot say which cookies had one, so a failed clear must stay failed. it('never reconstructs removed cookies when another removal fails', async () => { - const get = vi - .fn() - .mockResolvedValue([ + const { session, remove, set } = clearSession( + [ cookie('.google.com', 'SID'), cookie('.example.com', 'first', '/one'), cookie('.example.com', 'second', '/two'), cookie('.example.com', 'third', '/three') - ]) - const remove = vi.fn().mockImplementation(async (_url: string, name: string) => { - if (name === 'second') { - throw new Error('store unavailable') + ], + { + clearData: rejectingBulkClear(), + remove: vi.fn().mockImplementation(async (_url: string, name: string) => { + if (name === 'second') { + throw new Error('store unavailable') + } + }) } - }) - const set = vi.fn().mockResolvedValue(undefined) + ) - await expect( - removeAllCookiesExcept({ get, remove }, (existingCookie) => - isNonTransplantableCookieDomain(existingCookie.domain ?? '') - ) - ).rejects.toThrow('the session was left partially cleared') + await expect(removeTransplantableCookies(session)).rejects.toThrow( + 'the session was left partially cleared' + ) expect(remove).toHaveBeenCalledTimes(3) expect(set).not.toHaveBeenCalled() }) it('bounds parallel removals so large cookie jars do not clear serially or fan out', async () => { - const get = vi - .fn() - .mockResolvedValue( - Array.from({ length: 12 }, (_, index) => cookie('.example.com', `${index}`)) - ) let releaseRemovals: (() => void) | undefined const removalsReleased = new Promise((resolve) => { releaseRemovals = resolve }) let active = 0 let maxActive = 0 - const remove = vi.fn().mockImplementation(async () => { - active++ - maxActive = Math.max(maxActive, active) - await removalsReleased - active-- - }) - const set = vi.fn().mockResolvedValue(undefined) + const { session, remove, set } = clearSession( + [ + cookie('.google.com', 'SID'), + ...Array.from({ length: 12 }, (_, index) => cookie('.example.com', `${index}`)) + ], + { + clearData: rejectingBulkClear(), + remove: vi.fn().mockImplementation(async () => { + active++ + maxActive = Math.max(maxActive, active) + await removalsReleased + active-- + }) + } + ) - const clearing = removeAllCookiesExcept({ get, remove }, () => false) + const clearing = removeTransplantableCookies(session) await vi.waitFor(() => expect(remove).toHaveBeenCalledTimes(8)) expect(maxActive).toBe(8) releaseRemovals?.() @@ -289,22 +404,26 @@ describe('removeAllCookiesExcept', () => { }) it('serializes cookies that share Electron removal coordinates', async () => { - const get = vi - .fn() - .mockResolvedValue([ - cookie('.example.com', 'session'), - { ...cookie('example.com', 'session'), hostOnly: true } - ]) let releaseFirst: (() => void) | undefined const firstReleased = new Promise((resolve) => { releaseFirst = resolve }) - const remove = vi - .fn() - .mockImplementationOnce(() => firstReleased) - .mockResolvedValueOnce(undefined) + const { session, remove } = clearSession( + [ + cookie('.google.com', 'SID'), + cookie('.example.com', 'session'), + { ...cookie('example.com', 'session'), hostOnly: true } + ], + { + clearData: rejectingBulkClear(), + remove: vi + .fn() + .mockImplementationOnce(() => firstReleased) + .mockResolvedValueOnce(undefined) + } + ) - const clearing = removeAllCookiesExcept({ get, remove }, () => false) + const clearing = removeTransplantableCookies(session) await vi.waitFor(() => expect(remove).toHaveBeenCalledOnce()) releaseFirst?.() await clearing diff --git a/src/main/browser/browser-cookie-import-policy.ts b/src/main/browser/browser-cookie-import-policy.ts index 27be9512218..6f81407f4a9 100644 --- a/src/main/browser/browser-cookie-import-policy.ts +++ b/src/main/browser/browser-cookie-import-policy.ts @@ -1,4 +1,4 @@ -import type { Cookie, Cookies } from 'electron' +import type { Cookie, Cookies, Session } from 'electron' import { parse as parseDomain } from 'psl' import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' @@ -59,11 +59,16 @@ export function normalizeCookieImportDomain(domain: string): string | null { // it is copied. Signing in directly inside Orca is the only path that produces a working // session, so an import must never write these cookies and never remove them either — the // live session is always more valuable than anything an import could put in its place. +// Entries must be canonical lowercase ASCII (punycode) registrable domains, never subdomains or +// public suffixes, because clearData derives one excluded origin and matches at that boundary. // Adding a site is one entry here. // youtube.com is deliberately NOT listed: YouTube accepts a transplanted session and re-issues // its cookies via the accounts.youtube.com relay, so excluding it would silently drop imports // users actually asked for. const NON_TRANSPLANTABLE_DOMAINS = ['google.com'] as const +const NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS = NON_TRANSPLANTABLE_DOMAINS.map( + (root) => `https://${root}` +) const COOKIE_CLEAR_CONCURRENCY = 8 export function isNonTransplantableCookieDomain(domain: string): boolean { @@ -196,20 +201,49 @@ export async function restoreImportedDomainCookies( } } +// Why (STA-4061): 'set' stays out so the lossy partition-dropping reconstruction cannot return. +export type CookieClearSession = { + cookies: Pick + clearData: Session['clearData'] +} + // Why: Electron cannot round-trip partition identity, so excluded cookies must never be removed. // Why (STA-4061): the same gap forbids rolling a partial clear back. cookies.get() omits // partitionKey and cookies.set() silently drops it, so every reconstruction is a coin flip that // can downgrade a partitioned (CHIPS) cookie into an unpartitioned one — and nothing in the // snapshot says which cookies are at risk. A partially cleared jar is a retryable import failure; // a downgraded cookie is unrecoverable auth-state corruption that survives restart. -export async function removeAllCookiesExcept( - store: Pick, - isExcluded: (cookie: Cookie) => boolean +// Why (STA-4065): the exclusion is module state rather than a parameter so the predicate and the +// origins the bulk clear preserves cannot drift apart — a caller-supplied predicate that disagreed +// with NON_TRANSPLANTABLE_DOMAINS would silently delete a cookie the bulk call is meant to keep. +export async function removeTransplantableCookies( + targetSession: CookieClearSession ): Promise { + const store = targetSession.cookies + const initialCookies = await store.get({}) + if (initialCookies.length === 0) { + return + } + + // Why (STA-4065): measured on Electron 43, excludeOrigins preserves the whole registrable + // family — host, leading-dot, subdomain, and partitioned Google cookies — so one call replaces a + // remove() per cookie even when the jar holds cookies to keep. That is the ordinary case here: + // this import exists for Google, so a Google cookie is usually present. + try { + await targetSession.clearData({ + dataTypes: ['cookies'], + excludeOrigins: NON_TRANSPLANTABLE_CLEAR_EXCLUDED_ORIGINS + }) + return + } catch { + // Why: a rejected bulk clear can still have changed the jar, so the fallback must act on the + // survivors rather than stale removal coordinates from before the attempt. + } + const existingCookies = await store.get({}) const removableGroups = new Map() for (const cookie of existingCookies) { - if (isExcluded(cookie)) { + if (isNonTransplantableCookieDomain(cookie.domain ?? '')) { continue } const domain = cookie.domain ? normalizeCookieDomain(cookie.domain) : null diff --git a/src/main/browser/browser-cookie-import-replacement.test.ts b/src/main/browser/browser-cookie-import-replacement.test.ts index 8b5c07bb057..db70922ccb5 100644 --- a/src/main/browser/browser-cookie-import-replacement.test.ts +++ b/src/main/browser/browser-cookie-import-replacement.test.ts @@ -168,7 +168,7 @@ describe('validated cookie replacement', () => { }) describe('native Chromium integrity-cookie accounting', () => { - let clearStorageDataMock: ReturnType + let clearDataMock: ReturnType let cookiesSetMock: ReturnType let tmpDir: string @@ -180,7 +180,7 @@ describe('native Chromium integrity-cookie accounting', () => { }) clearPendingCookieImportMock.mockClear() setPendingCookieImportMock.mockClear() - clearStorageDataMock = vi.fn().mockResolvedValue(undefined) + clearDataMock = vi.fn().mockResolvedValue(undefined) cookiesSetMock = vi.fn().mockResolvedValue(undefined) sessionFromPartitionMock.mockReset().mockReturnValue({ cookies: { @@ -189,7 +189,7 @@ describe('native Chromium integrity-cookie accounting', () => { remove: vi.fn().mockResolvedValue(undefined), set: cookiesSetMock }, - clearStorageData: clearStorageDataMock + clearData: clearDataMock }) }) @@ -252,7 +252,7 @@ describe('native Chromium integrity-cookie accounting', () => { googleCookiesSkipped: 2, domains: [] }) - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).not.toHaveBeenCalled() expect(cookiesSetMock).not.toHaveBeenCalled() expect(setPendingCookieImportMock).not.toHaveBeenCalled() expect(clearPendingCookieImportMock).not.toHaveBeenCalled() diff --git a/src/main/browser/browser-cookie-import.test.ts b/src/main/browser/browser-cookie-import.test.ts index d2d4ffd4836..90514321d54 100644 --- a/src/main/browser/browser-cookie-import.test.ts +++ b/src/main/browser/browser-cookie-import.test.ts @@ -423,7 +423,7 @@ describe('importCookiesFromBrowser Chromium', () => { let cookiesSetMock: ReturnType let cookiesRemoveMock: ReturnType let cookiesFlushStoreMock: ReturnType - let clearStorageDataMock: ReturnType + let clearDataMock: ReturnType let setUserAgentMock: ReturnType beforeEach(() => { @@ -431,7 +431,7 @@ describe('importCookiesFromBrowser Chromium', () => { cookiesSetMock = vi.fn().mockResolvedValue(undefined) cookiesRemoveMock = vi.fn().mockResolvedValue(undefined) cookiesFlushStoreMock = vi.fn().mockResolvedValue(undefined) - clearStorageDataMock = vi.fn().mockResolvedValue(undefined) + clearDataMock = vi.fn().mockResolvedValue(undefined) setUserAgentMock = vi.fn() appGetPathMock.mockReset() appGetPathMock.mockReturnValue(join(tmpDir, 'userData')) @@ -450,7 +450,7 @@ describe('importCookiesFromBrowser Chromium', () => { remove: cookiesRemoveMock, flushStore: cookiesFlushStoreMock }, - clearStorageData: clearStorageDataMock, + clearData: clearDataMock, setUserAgent: setUserAgentMock }) }) @@ -511,7 +511,7 @@ describe('importCookiesFromBrowser Chromium', () => { ['', '-wal', '-shm'].map((suffix) => readFileSync(sourceCookiesPath + suffix)) ).toEqual(sourceFilesBefore) expect(cookiesRemoveMock).not.toHaveBeenCalled() - expect(clearStorageDataMock).not.toHaveBeenCalled() + expect(clearDataMock).not.toHaveBeenCalled() // Why: STA-3514 — imports must never impersonate the source browser; the // session keeps the engine UA the registry set at startup. expect(setUserAgentMock).not.toHaveBeenCalled() diff --git a/src/main/browser/browser-cookie-import.ts b/src/main/browser/browser-cookie-import.ts index 2a7bed19950..3c3a701f953 100644 --- a/src/main/browser/browser-cookie-import.ts +++ b/src/main/browser/browser-cookie-import.ts @@ -80,7 +80,7 @@ import { NON_TRANSPLANTABLE_HOST_KEY_SQL, normalizeCookieDomain, normalizeCookieImportDomain, - removeAllCookiesExcept, + removeTransplantableCookies, replaceCookiesForImportedDomains, restoreImportedDomainCookies, type CookieImportMode @@ -1780,9 +1780,7 @@ export async function importCookiesFromBrowser( // Why: clear stale cookies first; mixing them with the imported set makes sites reject the // session. Non-transplantable families are exempt — nothing was imported for them, and their // live session is the only one that works. - await removeAllCookiesExcept(targetSession.cookies, (cookie) => - isNonTransplantableCookieDomain(cookie.domain ?? '') - ) + await removeTransplantableCookies(targetSession) diag( ` cleared existing session cookies before loading ${decryptedCookies.length} imported cookies` ) From d3a2bf352fb6bac6561f425e2ee60c963b099ce0 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:55:29 -0700 Subject: [PATCH 9/9] fix(terminal): preserve Fish readiness escape bytes (#14142) --- config/reliability-gates.jsonc | 60 +++++++-- src/main/daemon/session.test.ts | 24 ++++ src/main/daemon/session.ts | 53 +++----- src/main/daemon/shell-ready.test.ts | 15 +++ src/main/providers/local-pty-provider.test.ts | 17 +++ src/main/providers/local-pty-provider.ts | 43 ++----- src/main/shell-ready-marker-scanner.ts | 24 +++- src/main/shell-startup-output-scanner.test.ts | 121 ++++++++++++++++++ src/main/shell-startup-output-scanner.ts | 68 ++++++++++ src/relay/pty-handler.ts | 54 +++----- 10 files changed, 357 insertions(+), 122 deletions(-) create mode 100644 src/main/shell-startup-output-scanner.test.ts create mode 100644 src/main/shell-startup-output-scanner.ts diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index c9bf94a1731..89d8e3e981e 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -17,23 +17,28 @@ "protection": "partial", "owner": "terminal-session-readiness", "layer": "posix-pty-and-shell-startup", - "surfaces": ["queued terminal startup commands", "zsh and bash startup files"], + "surfaces": ["queued terminal startup commands", "zsh, bash, and Fish startup output"], "platforms": ["macos", "linux"], "providers": ["local", "local-daemon", "ssh-daemon", "ssh-relay", "paired-runtime"], - "coveredPlatforms": ["macos"], + "coveredPlatforms": ["macos", "linux"], "coveredProviders": ["local", "local-daemon", "ssh-relay"], - "coverageNotes": "Real macOS node-pty integration covers zsh 5.9 .zshenv/.zprofile and bash 3.2 profile replacement shells, including exec -a names, same-shell and child-process silent reads, a blocking zle-line-init hook, non-shell readline replacements, and a non-shell image renamed zsh. An Ubuntu 20.04 container confirms compatible zsh/bash termios. Deterministic owner contracts cover local, daemon, and SSH relay identity stripping plus provider- and renderer-owned delivery. Renderer-owned relay fallback publishes the existing ready marker, so mixed client/host versions need no new wire capability. Live SSH, paired-runtime, and WSL remain gaps.", - "motivatingLinks": ["https://github.com/stablyai/orca/issues/13767"], - "invariant": "Before user startup files run, the wrapper publishes its shell PID. If a later exec discards the ready marker, prompt fallback releases queued startup input only after the launched shell's actual executable image with that exact PID owns the PTY foreground, emits its post-hook line-editor enable sequence, and exposes zle/readline termios; startup reads, line-init hooks, child foreground processes, and non-shell readline replacements cannot trigger early fallback release.", - "oracle": "Run the real generated wrappers under node-pty with disposable zsh and bash startup files. Plain and exec -a replacement shells must receive one queued command at their first completed line editor without waiting for the 15-second timeout. The identical oracle must time out without the startup identity and prompt fallback. Same-shell reads, child-process reads, a blocking zle-line-init hook, sqlite3, and sqlite3 renamed zsh must not trigger prompt fallback. Relay owner coverage must strip the identity and release through the same prompt contract.", + "coverageNotes": "Real macOS node-pty integration covers zsh 5.9 .zshenv/.zprofile and bash 3.2 profile replacement shells, including exec -a names, same-shell and child-process silent reads, a blocking zle-line-init hook, non-shell readline replacements, and a non-shell image renamed zsh. A real Fish PTY proves marker stripping is byte-exact and does not render bracketed-paste control text. An Ubuntu 20.04 container confirms compatible zsh/bash termios. Deterministic owner contracts cover local, daemon, and SSH relay identity/readiness composition plus provider- and renderer-owned delivery. Renderer-owned relay fallback publishes the existing ready marker, so mixed client/host versions need no new wire capability. Live SSH, paired-runtime, and WSL remain gaps.", + "motivatingLinks": [ + "https://github.com/stablyai/orca/issues/13767", + "https://linear.app/stably/issue/STA-4068" + ], + "invariant": "Shell startup scanning preserves every non-Orca output byte in order across arbitrary PTY chunk boundaries. Before user startup files run, the wrapper publishes its shell PID. If a later exec discards the ready marker, prompt fallback releases queued startup input only after the launched shell's actual executable image with that exact PID owns the PTY foreground, emits its post-hook line-editor enable sequence, and exposes zle/readline termios; startup reads, line-init hooks, child foreground processes, and non-shell readline replacements cannot trigger early fallback release.", + "oracle": "Split Fish output immediately after the ready marker and after the following ESC introducer; local and daemon owners must emit the exact ESC [ ? 2004 h bytes and headless xterm must not render literal control text. Run the real generated wrappers under node-pty with disposable zsh, bash, and Fish startup files. Plain and exec -a replacement shells must receive one queued command at their first completed line editor without waiting for the 15-second timeout. The identical oracle must time out without the startup identity and prompt fallback. Same-shell reads, child-process reads, a blocking zle-line-init hook, sqlite3, and sqlite3 renamed zsh must not trigger prompt fallback. Relay owner coverage must use the same lossless scanner composition and release through the same prompt contract.", "commands": [ - "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/main/line-editor-ready-output-scanner.test.ts src/main/shell-startup-identity-scanner.test.ts src/main/shell-prompt-readiness-probe.test.ts src/shared/pty-slave-line-discipline-echo.test.ts src/shared/shell-process-readiness.test.ts src/main/daemon/session.test.ts src/main/daemon/shell-ready.test.ts src/main/providers/local-pty-shell-ready.test.ts src/main/providers/local-pty-provider.test.ts src/relay/pty-shell-launch.test.ts src/relay/pty-handler.test.ts", - "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/relay/pty-handler.test.ts" + "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/main/line-editor-ready-output-scanner.test.ts src/main/shell-startup-identity-scanner.test.ts src/main/shell-startup-output-scanner.test.ts src/main/shell-prompt-readiness-probe.test.ts src/shared/pty-slave-line-discipline-echo.test.ts src/shared/shell-process-readiness.test.ts src/main/daemon/session.test.ts src/main/daemon/shell-ready.test.ts src/main/providers/local-pty-shell-ready.test.ts src/main/providers/local-pty-provider.test.ts src/relay/pty-shell-launch.test.ts src/relay/pty-handler.test.ts", + "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/relay/pty-handler.test.ts", + "pnpm exec vitest run --config config/vitest.config.ts --maxWorkers=1 src/main/daemon/shell-ready.test.ts src/main/shell-startup-output-scanner.test.ts" ], "testFiles": [ "src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts", "src/main/line-editor-ready-output-scanner.test.ts", "src/main/shell-startup-identity-scanner.test.ts", + "src/main/shell-startup-output-scanner.test.ts", "src/main/shell-prompt-readiness-probe.test.ts", "src/shared/pty-slave-line-discipline-echo.test.ts", "src/shared/shell-process-readiness.test.ts", @@ -45,6 +50,26 @@ "src/relay/pty-handler.test.ts" ], "assertionRefs": [ + { + "file": "src/main/shell-startup-output-scanner.test.ts", + "assertions": [ + "Fish bracketed-paste output stays byte-exact across marker and ESC chunk splits", + "every byte after readiness is preserved across every chunk boundary, even when it resembles an identity marker", + "pre-ready identity and readiness markers strip across every chunk boundary without reordering prompt output", + "incomplete scanner prefixes drain in byte order" + ] + }, + { + "file": "src/main/daemon/session.test.ts", + "assertions": [ + "daemon output is byte-exact for both Fish chunk splits", + "headless xterm never renders literal bracketed-paste control text" + ] + }, + { + "file": "src/main/providers/local-pty-provider.test.ts", + "assertions": ["local PTY output is byte-exact for both Fish chunk splits"] + }, { "file": "src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts", "assertions": [ @@ -66,14 +91,23 @@ } ], "evidenceRuns": [ + { + "date": "2026-08-12", + "runner": "manual", + "platform": "linux", + "command": "pnpm exec vitest run --config config/vitest.config.ts --maxWorkers=1 src/main/daemon/shell-ready.test.ts src/main/shell-startup-output-scanner.test.ts", + "result": "passed", + "durationSeconds": 7, + "summary": "A clean Debian 12 arm64 container with real Fish 3.6 and node-pty passed exact marker-stripped output plus headless-xterm rendering." + }, { "date": "2026-08-12", "runner": "local", "platform": "macos", - "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/main/line-editor-ready-output-scanner.test.ts src/main/shell-startup-identity-scanner.test.ts src/main/shell-prompt-readiness-probe.test.ts src/shared/pty-slave-line-discipline-echo.test.ts src/shared/shell-process-readiness.test.ts src/main/daemon/session.test.ts src/main/daemon/shell-ready.test.ts src/main/providers/local-pty-shell-ready.test.ts src/main/providers/local-pty-provider.test.ts src/relay/pty-shell-launch.test.ts src/relay/pty-handler.test.ts", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/main/daemon/repro-13767-shell-ready-marker-lost-to-exec.test.ts src/main/line-editor-ready-output-scanner.test.ts src/main/shell-startup-identity-scanner.test.ts src/main/shell-startup-output-scanner.test.ts src/main/shell-prompt-readiness-probe.test.ts src/shared/pty-slave-line-discipline-echo.test.ts src/shared/shell-process-readiness.test.ts src/main/daemon/session.test.ts src/main/daemon/shell-ready.test.ts src/main/providers/local-pty-shell-ready.test.ts src/main/providers/local-pty-provider.test.ts src/relay/pty-shell-launch.test.ts src/relay/pty-handler.test.ts", "result": "passed", - "durationSeconds": 4, - "summary": "All real exec, silent-read, line-init-hook, renamed-image, disabled-bracketed-paste, and relay-owner oracles plus identity, foreground, executable-image, protocol, and line-editor contracts passed." + "durationSeconds": 6, + "summary": "473 tests passed with one local Fish skip, including every-boundary scanner composition, real exec, silent-read, line-init-hook, renamed-image, disabled-bracketed-paste, and relay-owner oracles." } ], "runtimeBudget": { @@ -86,11 +120,11 @@ }, "redGreenEvidence": { "status": "complete", - "evidence": "Oracle SHA-256 b13b08006a38b2adc2acb51ae9342909514e55ab4822fac4fbb76cd49b344cc6 is byte-identical across runs. Latest main 09ec516ae5 and the candidate with its production implementation reverted both failed exactly the three zsh .zprofile/.zshenv and bash exec cases by deterministic 5-second timeout; the zle-line-init oracle also exposed unsafe baseline delivery, while all ten other safety controls passed. The candidate passed all 14 on macOS." + "evidence": "The Fish chunk oracle patch SHA-256 fd49a4d3d91bd5f0d10c1b1f44f4cfbd92cef9155a5505b415c8680184cdc765 is byte-identical across runs. Scan target e4c278eb52, latest main 150cc6f50c, and candidate ae7f30318a with shared scanner integration disabled each failed only the ESC-introducer split in both local and daemon owners (2 failures, 161 passes). Candidate ae7f30318a passed all 163 local/daemon cases plus the independent headless-xterm render oracle. The original exec-readiness oracle SHA-256 b13b08006a38b2adc2acb51ae9342909514e55ab4822fac4fbb76cd49b344cc6 remains 14/14 green." }, "performanceBudget": { "required": true, - "evidence": "The normal ready-marker path cancels before external probing. A missing marker triggers a debounced stty read only after a line-editor enable sequence, followed by narrow PID status and executable-image inspection only when termios matches. Slow ordinary startup output triggers zero probes; rejected protocol emissions are capped at four probes per startup. There is no per-command hook or steady-state polling." + "evidence": "Two bounded linear scans partition identity ownership at the exact ready-marker boundary and retain only the existing bounded prefixes. The normal ready-marker path cancels before external probing. A missing marker triggers a debounced stty read only after a line-editor enable sequence, followed by narrow PID status and executable-image inspection only when termios matches. Slow ordinary startup output triggers zero probes; rejected protocol emissions are capped at four probes per startup. There is no per-command hook or steady-state polling." }, "promotionCriteria": [ "Collect Linux, SSH-daemon, and paired-runtime live evidence.", diff --git a/src/main/daemon/session.test.ts b/src/main/daemon/session.test.ts index bc90bf5d97f..5769858f2cf 100644 --- a/src/main/daemon/session.test.ts +++ b/src/main/daemon/session.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { SESSION_FORCE_KILL_RETRY_MS, Session } from './session' +import { HeadlessEmulator } from './headless-emulator' import type { SessionState, ShellReadyState } from './types' import type { TuiAgent } from '../../shared/types' @@ -430,6 +431,29 @@ describe('Session', () => { expect(session.getSnapshot()?.snapshotAnsi).not.toContain('orca-shell-ready') }) + it.each([ + ['after the ready marker', ['\x1b]777;orca-shell-ready\x07', '\x1b[?2004hfish> ']], + ['after the ESC introducer', ['\x1b]777;orca-shell-ready\x07\x1b', '[?2004hfish> ']] + ])('preserves Fish bracketed-paste output split %s', (_boundary, chunks) => { + createSession({ shellReadySupported: true }) + const received: string[] = [] + session.attachClient({ + onData: (data) => received.push(data), + onExit: () => {} + }) + + for (const chunk of chunks) { + subprocess.simulateData(chunk) + } + + const output = received.join('') + expect(output).toBe('\x1b[?2004hfish> ') + const rendered = new HeadlessEmulator({ cols: 80, rows: 24 }) + expect(rendered.writeSync(output)).toBe(true) + expect(rendered.getVisibleLines().join('\n')).not.toContain('[?2004h') + rendered.dispose() + }) + it('publishes an absolute output sequence with live snapshots', () => { createSession() subprocess.simulateData('first') diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index 81343b39744..3771b3194b5 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -8,17 +8,11 @@ import { import { isValidPtySize, normalizePtySize } from './daemon-pty-size' import { PostReadyFlushGate } from './post-ready-flush-gate' import { - createShellReadyScanState, - drainShellReadyHeldBytes, - scanForShellReady, - type ShellReadyScanState -} from '../shell-ready-marker-scanner' -import { - createShellStartupIdentityScanState, - drainShellStartupIdentityHeldBytes, - scanForShellStartupIdentity, - type ShellStartupIdentityScanState -} from '../shell-startup-identity-scanner' + createShellStartupOutputScanState, + drainShellStartupOutputScanState, + scanShellStartupOutput, + type ShellStartupOutputScanState +} from '../shell-startup-output-scanner' import { createShellPromptReadinessProbe, type ShellPromptReadinessProbe @@ -136,8 +130,7 @@ export class Session { private preReadyStdinQueue: string[] = [] private releaseStartupDeviceAttributesResponder: (() => void) | null = null private startupDeviceAttributesQueryFilter: StartupDeviceAttributesQueryFilter | null = null - private shellReadyScanState: ShellReadyScanState | null = null - private shellStartupIdentityScanState: ShellStartupIdentityScanState | null = null + private shellStartupOutputScanState: ShellStartupOutputScanState | null = null private shellStartupPid: number | null = null private shellPromptReadinessProbe: ShellPromptReadinessProbe | null = null private shellReadyTimer: ReturnType | null = null @@ -184,8 +177,7 @@ export class Session { if (opts.shellReadySupported) { this._shellState = 'pending' - this.shellReadyScanState = createShellReadyScanState() - this.shellStartupIdentityScanState = createShellStartupIdentityScanState() + this.shellStartupOutputScanState = createShellStartupOutputScanState() // Why: `write` queues everything until the ready marker, including the renderer's DA1 // reply — and a shell that withholds its first prompt until DA1 is answered (fish) then // never emits the marker that would release it. Answer from the daemon, past the queue. @@ -636,7 +628,7 @@ export class Session { } this.shellPromptReadinessProbe?.dispose() this.shellPromptReadinessProbe = null - this.shellReadyScanState = null + this.shellStartupOutputScanState = null this.preReadyStdinQueue = [] this.postReadyFlushGate.clear() this.disposeSubprocessHandle() @@ -682,18 +674,13 @@ export class Session { } let releaseStartupDeviceAttributes = false - if (this._shellState === 'pending' && this.shellStartupIdentityScanState) { - const scanned = scanForShellStartupIdentity(this.shellStartupIdentityScanState, data) + if (this._shellState === 'pending' && this.shellStartupOutputScanState) { + const scanned = scanShellStartupOutput(this.shellStartupOutputScanState, data) data = scanned.output if (scanned.shellPid) { this.shellStartupPid = scanned.shellPid - this.shellStartupIdentityScanState = null } - } - if (this._shellState === 'pending' && this.shellReadyScanState) { - const scanned = scanForShellReady(this.shellReadyScanState, data) - data = scanned.output - if (scanned.matched) { + if (scanned.ready) { this.transitionToReady(scanned.postMarkerBytesObserved) releaseStartupDeviceAttributes = true } @@ -773,20 +760,11 @@ export class Session { } private releaseHeldShellReadyBytes(): string { - if (!this.shellReadyScanState && !this.shellStartupIdentityScanState) { + if (!this.shellStartupOutputScanState) { return '' } - let heldBytes = this.shellStartupIdentityScanState - ? drainShellStartupIdentityHeldBytes(this.shellStartupIdentityScanState) - : '' - this.shellStartupIdentityScanState = null - if (this.shellReadyScanState && heldBytes) { - heldBytes = scanForShellReady(this.shellReadyScanState, heldBytes).output - } - if (this.shellReadyScanState) { - heldBytes += drainShellReadyHeldBytes(this.shellReadyScanState) - } - this.shellReadyScanState = null + const heldBytes = drainShellStartupOutputScanState(this.shellStartupOutputScanState) + this.shellStartupOutputScanState = null // Why: scanning strips marker bytes before fan-out; if readiness never completes, release any held prefix before timeout/exit discards it. this.startupIngress.accept(heldBytes) return heldBytes @@ -813,8 +791,7 @@ export class Session { private transitionToReady(postMarkerBytesObserved = false): void { this._shellState = 'ready' - this.shellReadyScanState = null - this.shellStartupIdentityScanState = null + this.shellStartupOutputScanState = null this.shellPromptReadinessProbe?.dispose() this.shellPromptReadinessProbe = null if (this.shellReadyTimer) { diff --git a/src/main/daemon/shell-ready.test.ts b/src/main/daemon/shell-ready.test.ts index b7299343579..2d931e7206e 100644 --- a/src/main/daemon/shell-ready.test.ts +++ b/src/main/daemon/shell-ready.test.ts @@ -6,6 +6,12 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync import type * as ShellReadyModule from './shell-ready' import { getZshShellReadyMarkerRegistrationBlock } from '../shell-templates' import { fishRequirementViolation, resolveFishBinary } from '../../shared/fish-binary-requirement' +import { + createShellStartupOutputScanState, + drainShellStartupOutputScanState, + scanShellStartupOutput +} from '../shell-startup-output-scanner' +import { HeadlessEmulator } from './headless-emulator' async function importFreshShellReady(): Promise { vi.resetModules() @@ -296,6 +302,8 @@ describePosix('daemon shell-ready launch config', () => { } }) let output = '' + let scannedOutput = '' + const startupScanState = createShellStartupOutputScanState() let commandWritten = false let erasureProbeWritten = false let queryCarry = '' @@ -321,6 +329,7 @@ describePosix('daemon shell-ready launch config', () => { }, 50) proc.onData((chunk) => { output += chunk + scannedOutput += scanShellStartupOutput(startupScanState, chunk).output // Why: fish stalls its first prompt 10s waiting on these and re-queries // each prompt, so answer every occurrence — an unanswered query makes // fish swallow the post-marker command as its reply. @@ -350,9 +359,15 @@ describePosix('daemon shell-ready launch config', () => { clearTimeout(deadline) clearInterval(sentinelPoll) proc.kill() + scannedOutput += drainShellStartupOutputScanState(startupScanState) expect(output).toContain(SHELL_READY_MARKER_OUTPUT) expect(output.split(SHELL_READY_MARKER_OUTPUT)).toHaveLength(2) + expect(scannedOutput).toBe(output.replace(SHELL_READY_MARKER_OUTPUT, '')) + const rendered = new HeadlessEmulator({ cols: 80, rows: 24 }) + expect(rendered.writeSync(scannedOutput)).toBe(true) + expect(rendered.getVisibleLines().join('\n')).not.toContain('[?2004h') + rendered.dispose() expect(existsSync(sentinel)).toBe(true) // Why: asserts the erase directly rather than inferring it from the marker // count, which only holds once enough prompts have been drawn to expose it. diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 889d75ac7ae..178b60d6caa 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -658,6 +658,23 @@ describe('LocalPtyProvider', () => { } }) + it.each([ + ['after the ready marker', ['\x1b]777;orca-shell-ready\x07', '\x1b[?2004hfish> ']], + ['after the ESC introducer', ['\x1b]777;orca-shell-ready\x07\x1b', '[?2004hfish> ']] + ])('preserves Fish bracketed-paste output split %s', async (_boundary, chunks) => { + process.env.SHELL = '/usr/bin/fish' + const received: string[] = [] + provider.configure({ onData: (_id, data) => received.push(data) }) + + await provider.spawn({ cols: 80, rows: 24, command: 'printf ready' }) + const dataCallback = mockProc.onData.mock.calls[0]?.[0] as (data: string) => void + for (const chunk of chunks) { + dataCallback(chunk) + } + + expect(received.join('')).toBe('\x1b[?2004hfish> ') + }) + it('releases held marker-prefix bytes when local shell readiness times out', async () => { vi.useFakeTimers() const onData = vi.fn() diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 6601a56ac31..3026ded42a3 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -29,9 +29,6 @@ import { prepareMacosTccLoginShell } from './macos-tcc-login-shell' import { getMarkerlessShellLaunchConfig, getShellReadyLaunchConfig, - createShellReadyScanState, - drainShellReadyHeldBytes, - scanForShellReady, writeStartupCommandWhenShellReady, STARTUP_COMMAND_READY_MAX_WAIT_MS } from './local-pty-shell-ready' @@ -76,10 +73,10 @@ import { readPtySlavePath } from '../../shared/pty-slave-line-discipline-echo' import { - createShellStartupIdentityScanState, - drainShellStartupIdentityHeldBytes, - scanForShellStartupIdentity -} from '../shell-startup-identity-scanner' + createShellStartupOutputScanState, + drainShellStartupOutputScanState, + scanShellStartupOutput +} from '../shell-startup-output-scanner' import { createShellPromptReadinessProbe, type ShellPromptReadinessProbe @@ -961,11 +958,8 @@ export class LocalPtyProvider implements IPtyProvider { let shellReadyTimeout: ReturnType | null = null let shellStartupPid: number | null = null let shellPromptReadinessProbe: ShellPromptReadinessProbe | null = null - let shellStartupIdentityScanState = shellReadyLaunch?.supportsReadyMarker - ? createShellStartupIdentityScanState() - : null - const shellReadyScanState = shellReadyLaunch?.supportsReadyMarker - ? createShellReadyScanState() + let shellStartupOutputScanState = shellReadyLaunch?.supportsReadyMarker + ? createShellStartupOutputScanState() : null const shellReadyPromise = args.command ? new Promise((resolve) => { @@ -987,23 +981,17 @@ export class LocalPtyProvider implements IPtyProvider { resolve(signal) } const releaseHeldShellReadyBytes = (): void => { - if (!shellReadyScanState) { + if (!shellStartupOutputScanState) { return } - let heldBytes = shellStartupIdentityScanState - ? drainShellStartupIdentityHeldBytes(shellStartupIdentityScanState) - : '' - shellStartupIdentityScanState = null - if (heldBytes) { - heldBytes = scanForShellReady(shellReadyScanState, heldBytes).output - } - heldBytes += drainShellReadyHeldBytes(shellReadyScanState) + const heldBytes = drainShellStartupOutputScanState(shellStartupOutputScanState) + shellStartupOutputScanState = null if (heldBytes.length === 0) { return } startupIngress.accept(heldBytes) } - if (shellReadyScanState) { + if (shellStartupOutputScanState) { shellPromptReadinessProbe = createShellPromptReadinessProbe({ slavePath: readPtySlavePath(proc), shellPath, @@ -1048,18 +1036,13 @@ export class LocalPtyProvider implements IPtyProvider { const disposables: { dispose: () => void }[] = [] const onDataDisposable = proc.onData((rawData) => { let data = rawData - if (shellStartupIdentityScanState && resolveShellReady) { - const scanned = scanForShellStartupIdentity(shellStartupIdentityScanState, data) + if (shellStartupOutputScanState && resolveShellReady) { + const scanned = scanShellStartupOutput(shellStartupOutputScanState, data) data = scanned.output if (scanned.shellPid) { shellStartupPid = scanned.shellPid - shellStartupIdentityScanState = null } - } - if (shellReadyScanState && resolveShellReady) { - const scanned = scanForShellReady(shellReadyScanState, data) - data = scanned.output - if (scanned.matched) { + if (scanned.ready) { finishShellReady({ postMarkerBytesObserved: scanned.postMarkerBytesObserved }) } } diff --git a/src/main/shell-ready-marker-scanner.ts b/src/main/shell-ready-marker-scanner.ts index 0e6250e1c4e..a8b58499852 100644 --- a/src/main/shell-ready-marker-scanner.ts +++ b/src/main/shell-ready-marker-scanner.ts @@ -11,6 +11,10 @@ export type ShellReadyScanResult = { postMarkerBytesObserved: boolean } +type ShellReadyBoundaryScanResult = ShellReadyScanResult & { + postMarkerOutputIndex: number | null +} + export function createShellReadyScanState(): ShellReadyScanState { return { matchPos: 0, heldBytes: '' } } @@ -22,7 +26,10 @@ export function drainShellReadyHeldBytes(state: ShellReadyScanState): string { return heldBytes } -export function scanForShellReady(state: ShellReadyScanState, data: string): ShellReadyScanResult { +export function scanForShellReadyBoundary( + state: ShellReadyScanState, + data: string +): ShellReadyBoundaryScanResult { let output = '' for (let i = 0; i < data.length; i += 1) { @@ -49,7 +56,8 @@ export function scanForShellReady(state: ShellReadyScanState, data: string): She return { output: output + remaining, matched: true, - postMarkerBytesObserved: remaining.length > 0 + postMarkerBytesObserved: remaining.length > 0, + postMarkerOutputIndex: output.length } } else { output += state.heldBytes @@ -64,5 +72,15 @@ export function scanForShellReady(state: ShellReadyScanState, data: string): She } } - return { output, matched: false, postMarkerBytesObserved: false } + return { + output, + matched: false, + postMarkerBytesObserved: false, + postMarkerOutputIndex: null + } +} + +export function scanForShellReady(state: ShellReadyScanState, data: string): ShellReadyScanResult { + const { postMarkerOutputIndex: _, ...result } = scanForShellReadyBoundary(state, data) + return result } diff --git a/src/main/shell-startup-output-scanner.test.ts b/src/main/shell-startup-output-scanner.test.ts new file mode 100644 index 00000000000..1a0930b9395 --- /dev/null +++ b/src/main/shell-startup-output-scanner.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, it } from 'vitest' +import { + createShellStartupOutputScanState, + drainShellStartupOutputScanState, + scanShellStartupOutput +} from './shell-startup-output-scanner' + +const READY_MARKER = '\x1b]777;orca-shell-ready\x07' +const IDENTITY_MARKER = '\x1b]777;orca-shell-start:12345\x07' + +function scanChunks(chunks: string[]): { + output: string + shellPid: number | null + ready: boolean + postMarkerBytesObserved: boolean +} { + const state = createShellStartupOutputScanState() + let output = '' + let shellPid: number | null = null + let ready = false + let postMarkerBytesObserved = false + + for (const chunk of chunks) { + const scanned = scanShellStartupOutput(state, chunk) + output += scanned.output + shellPid ??= scanned.shellPid + ready ||= scanned.ready + postMarkerBytesObserved ||= scanned.postMarkerBytesObserved + } + return { output, shellPid, ready, postMarkerBytesObserved } +} + +function expectEverySplitToProduce( + input: string, + expected: Omit, 'postMarkerBytesObserved'> +): void { + for (let split = 0; split <= input.length; split += 1) { + const { postMarkerBytesObserved: _, ...scanned } = scanChunks([ + input.slice(0, split), + input.slice(split) + ]) + expect(scanned, `split ${split}`).toEqual(expected) + } +} + +describe('shell startup output scanner', () => { + it.each([ + ['after the ready marker', [READY_MARKER, '\x1b[?2004hfish> ']], + ['after the ESC introducer', [`${READY_MARKER}\x1b`, '[?2004hfish> ']] + ])('preserves Fish output split %s', (_boundary, chunks) => { + const state = createShellStartupOutputScanState() + let output = '' + + for (const chunk of chunks) { + output += scanShellStartupOutput(state, chunk).output + } + + expect(output).toBe('\x1b[?2004hfish> ') + }) + + it('preserves every post-ready byte at every chunk boundary', () => { + expectEverySplitToProduce(`${READY_MARKER}${IDENTITY_MARKER}prompt`, { + output: `${IDENTITY_MARKER}prompt`, + shellPid: null, + ready: true + }) + expectEverySplitToProduce(`${READY_MARKER}\x1b[?2004hfish> `, { + output: '\x1b[?2004hfish> ', + shellPid: null, + ready: true + }) + }) + + it('strips pre-ready markers at every chunk boundary', () => { + expectEverySplitToProduce(`${IDENTITY_MARKER}${READY_MARKER}prompt`, { + output: 'prompt', + shellPid: 12345, + ready: true + }) + }) + + it('strips identity and readiness markers from one chunk', () => { + const state = createShellStartupOutputScanState() + const scanned = scanShellStartupOutput(state, `${IDENTITY_MARKER}${READY_MARKER}prompt`) + + expect(scanned).toEqual({ + output: 'prompt', + shellPid: 12345, + ready: true, + postMarkerBytesObserved: true + }) + }) + + it('counts an identity-held byte as post-marker output', () => { + const state = createShellStartupOutputScanState() + + expect(scanShellStartupOutput(state, `${READY_MARKER}\x1b`)).toEqual({ + output: '\x1b', + shellPid: null, + ready: true, + postMarkerBytesObserved: true + }) + }) + + it('drains every incomplete scanner prefix in byte order', () => { + const state = createShellStartupOutputScanState() + expect(scanShellStartupOutput(state, '\x1b]777;orca-shell-st').output).toBe('') + + expect(drainShellStartupOutputScanState(state)).toBe('\x1b]777;orca-shell-st') + }) + + it('drains simultaneous identity and readiness prefixes in input order', () => { + const state = createShellStartupOutputScanState() + expect(scanShellStartupOutput(state, '\x1b]777;orca-shell-st').output).toBe('') + expect(scanShellStartupOutput(state, '\x1b]777;orca-shell-rea').output).toBe('') + + expect(drainShellStartupOutputScanState(state)).toBe( + '\x1b]777;orca-shell-st\x1b]777;orca-shell-rea' + ) + }) +}) diff --git a/src/main/shell-startup-output-scanner.ts b/src/main/shell-startup-output-scanner.ts new file mode 100644 index 00000000000..b87626c24eb --- /dev/null +++ b/src/main/shell-startup-output-scanner.ts @@ -0,0 +1,68 @@ +import { + createShellReadyScanState, + drainShellReadyHeldBytes, + scanForShellReadyBoundary, + type ShellReadyScanState +} from './shell-ready-marker-scanner' +import { + createShellStartupIdentityScanState, + drainShellStartupIdentityHeldBytes, + scanForShellStartupIdentity, + type ShellStartupIdentityScanState +} from './shell-startup-identity-scanner' + +export type ShellStartupOutputScanState = { + ready: ShellReadyScanState + identity: ShellStartupIdentityScanState | null +} + +export type ShellStartupOutputScanResult = { + output: string + shellPid: number | null + ready: boolean + postMarkerBytesObserved: boolean +} + +export function createShellStartupOutputScanState(): ShellStartupOutputScanState { + return { + ready: createShellReadyScanState(), + identity: createShellStartupIdentityScanState() + } +} + +export function scanShellStartupOutput( + state: ShellStartupOutputScanState, + data: string +): ShellStartupOutputScanResult { + const readiness = scanForShellReadyBoundary(state.ready, data) + const postMarkerOutputIndex = readiness.postMarkerOutputIndex ?? readiness.output.length + const identityInput = readiness.output.slice(0, postMarkerOutputIndex) + let shellPid: number | null = null + let output = identityInput + if (state.identity) { + const identity = scanForShellStartupIdentity(state.identity, identityInput) + output = identity.output + shellPid = identity.shellPid + if (shellPid) { + state.identity = null + } + } + + if (readiness.matched && state.identity) { + output += drainShellStartupIdentityHeldBytes(state.identity) + state.identity = null + } + output += readiness.output.slice(postMarkerOutputIndex) + return { + output, + shellPid, + ready: readiness.matched, + postMarkerBytesObserved: readiness.postMarkerBytesObserved + } +} + +export function drainShellStartupOutputScanState(state: ShellStartupOutputScanState): string { + const output = state.identity ? drainShellStartupIdentityHeldBytes(state.identity) : '' + state.identity = null + return output + drainShellReadyHeldBytes(state.ready) +} diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 6dd3cd8a7fc..e7c09df95de 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -27,19 +27,13 @@ import { } from '../shared/cross-platform-path' import { splitWorktreeId } from '../shared/worktree-id' import { PhysicalExitTracker } from '../shared/physical-exit-tracker' +import { SHELL_READY_MARKER_PREFIX } from '../main/shell-ready-marker-scanner' import { - createShellReadyScanState, - drainShellReadyHeldBytes, - scanForShellReady, - SHELL_READY_MARKER_PREFIX, - type ShellReadyScanState -} from '../main/shell-ready-marker-scanner' -import { - createShellStartupIdentityScanState, - drainShellStartupIdentityHeldBytes, - scanForShellStartupIdentity, - type ShellStartupIdentityScanState -} from '../main/shell-startup-identity-scanner' + createShellStartupOutputScanState, + drainShellStartupOutputScanState, + scanShellStartupOutput, + type ShellStartupOutputScanState +} from '../main/shell-startup-output-scanner' import { createShellPromptReadinessProbe, type ShellPromptReadinessProbe @@ -196,8 +190,7 @@ type ManagedStartupCommand = { providerDelivery: boolean delivered: boolean waitForShellReady: boolean - scanState: ShellReadyScanState | null - identityScanState: ShellStartupIdentityScanState | null + outputScanState: ShellStartupOutputScanState | null shellPid: number | null promptProbe: ShellPromptReadinessProbe | null timer: ReturnType | null @@ -658,17 +651,11 @@ export class PtyHandler { } private drainStartupScanBytes(startup: ManagedStartupCommand): string { - let heldBytes = startup.identityScanState - ? drainShellStartupIdentityHeldBytes(startup.identityScanState) - : '' - startup.identityScanState = null - if (startup.scanState && heldBytes) { - heldBytes = scanForShellReady(startup.scanState, heldBytes).output - } - if (startup.scanState) { - heldBytes += drainShellReadyHeldBytes(startup.scanState) - startup.scanState = null + if (!startup.outputScanState) { + return '' } + const heldBytes = drainShellStartupOutputScanState(startup.outputScanState) + startup.outputScanState = null return heldBytes } @@ -767,18 +754,13 @@ export class PtyHandler { } managed.pty.onData((data: string) => { const startup = managed.startupCommand - if (startup?.identityScanState && !startup.delivered) { - const scanned = scanForShellStartupIdentity(startup.identityScanState, data) + if (startup?.waitForShellReady && startup.outputScanState && !startup.delivered) { + const scanned = scanShellStartupOutput(startup.outputScanState, data) data = scanned.output if (scanned.shellPid) { startup.shellPid = scanned.shellPid - startup.identityScanState = null } - } - if (startup?.waitForShellReady && startup.scanState && !startup.delivered) { - const scanned = scanForShellReady(startup.scanState, data) - data = scanned.output - if (scanned.matched) { + if (scanned.ready) { if (startup.providerDelivery) { this.scheduleStartupCommandResolution(managed, STARTUP_COMMAND_WRITE_DELAY_MS) } else { @@ -1644,13 +1626,9 @@ export class PtyHandler { providerDelivery: shouldProviderDeliverCommand, delivered: false, waitForShellReady: shellLaunch.env.ORCA_SHELL_READY_MARKER === '1', - scanState: + outputScanState: shellLaunch.env.ORCA_SHELL_READY_MARKER === '1' - ? createShellReadyScanState() - : null, - identityScanState: - shellLaunch.env.ORCA_SHELL_READY_MARKER === '1' - ? createShellStartupIdentityScanState() + ? createShellStartupOutputScanState() : null, shellPid: null, promptProbe: null,