From c538a070e54ba2a2e6cd2faf35dfc979b85ffb61 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 13 Jul 2026 06:18:34 -0700 Subject: [PATCH] Rebase custom-agents onto main (5/5): post-merge lint fixes Restore main's dropped onTerminalQueryReply mobile wiring (take-ours regression), and split 3 files pushed over max-lines by the merge into cohesive named modules (mobile-session-diff-line-row, host-client-context-contract, orchestration-handler-test-harness). No max-lines disables or baseline bumps. Co-authored-by: Orca --- .../app/h/[hostId]/session/[worktreeId].tsx | 142 +---------------- .../session/mobile-session-diff-line-row.tsx | 148 ++++++++++++++++++ mobile/src/transport/client-context.tsx | 59 +++---- .../transport/host-client-context-contract.ts | 50 ++++++ .../orchestration-handler-test-harness.ts | 64 ++++++++ src/cli/handlers/orchestration.test.ts | 135 ++++------------ 6 files changed, 312 insertions(+), 286 deletions(-) create mode 100644 mobile/app/h/[hostId]/session/mobile-session-diff-line-row.tsx create mode 100644 mobile/src/transport/host-client-context-contract.ts create mode 100644 src/cli/handlers/orchestration-handler-test-harness.ts diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index b53bdabddb7..5fe65e7e7aa 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -284,6 +284,7 @@ import { QuickCommandsTabButton } from '../../../../src/session/QuickCommandsTab import { styles } from '../../../../src/session/mobile-session-styles' import type { DiffComment } from '../../../../../src/shared/diff-comment-types' import type { TerminalQuickCommand } from '../../../../../src/shared/terminal-quick-command-types' +import { DiffLineRow } from './mobile-session-diff-line-row' import type { AgentLaunchNoticeCode } from '../../../../../src/shared/agent-launch-contract' import type { DiffCommentActions, @@ -308,146 +309,6 @@ import type { const TERMINAL_KEYBOARD_DISMISS_ACTION_SHEET_FALLBACK_MS = 450 -function DiffLineRow({ - line, - title, - index, - comments, - activeCommentLine, - commentDraft, - commentsBusy, - onStartComment, - onCancelComment, - onDraftChange, - onSubmitComment, - onDeleteComment -}: { - line: RenderableDiffLine - title: string - index: number - comments: DiffComment[] - activeCommentLine: number | null - commentDraft: string - commentsBusy: boolean - onStartComment: (lineNumber: number) => void - onCancelComment: () => void - onDraftChange: (value: string) => void - onSubmitComment: (lineNumber: number) => void - onDeleteComment: (commentId: string) => void -}) { - const commentLine = line.newLineNumber - const isCommenting = commentLine !== undefined && activeCommentLine === commentLine - const canComment = commentLine !== undefined - // Why: review notes anchor to the modified side, so show that line number in the single mobile gutter. - const gutterLineNumber = line.newLineNumber ?? line.oldLineNumber ?? '' - return ( - - - {gutterLineNumber} - - - {line.kind === 'add' ? '+ ' : line.kind === 'delete' ? '- ' : ' '} - - - - {canComment ? ( - [ - styles.diffCommentAddButton, - pressed && styles.diffCommentAddButtonPressed, - commentsBusy && styles.diffCommentButtonDisabled - ]} - disabled={commentsBusy} - onPress={() => { - if (commentLine !== undefined) { - onStartComment(commentLine) - } - }} - accessibilityLabel={`Add note on line ${commentLine}`} - > - - - ) : null} - - {comments.length > 0 ? ( - - {comments.map((comment) => ( - - - - Line {comment.lineNumber} - onDeleteComment(comment.id)} - accessibilityLabel={`Delete note on line ${comment.lineNumber}`} - > - - - - {comment.body} - - ))} - - ) : null} - {isCommenting ? ( - - - - - Cancel - - { - if (commentLine !== undefined) { - onSubmitComment(commentLine) - } - }} - > - Save note - - - - ) : null} - - ) -} - function FileReader({ doc, title, @@ -717,7 +578,6 @@ function FileReader({ return renderSourceText(doc.content) } - export default function SessionScreen() { const { hostId, diff --git a/mobile/app/h/[hostId]/session/mobile-session-diff-line-row.tsx b/mobile/app/h/[hostId]/session/mobile-session-diff-line-row.tsx new file mode 100644 index 00000000000..a192693172f --- /dev/null +++ b/mobile/app/h/[hostId]/session/mobile-session-diff-line-row.tsx @@ -0,0 +1,148 @@ +import { Pressable, Text, TextInput, View } from 'react-native' +import { MessageSquare, Plus, X } from 'lucide-react-native' +import { MobileSyntaxSegments } from '../../../../src/components/MobileSyntaxSegments' +import { colors } from '../../../../src/theme/mobile-theme' +import { styles } from '../../../../src/session/mobile-session-styles' +import type { DiffComment } from '../../../../../src/shared/types' +import type { RenderableDiffLine } from '../../../../src/session/mobile-session-route-types' + +export function DiffLineRow({ + line, + title, + index, + comments, + activeCommentLine, + commentDraft, + commentsBusy, + onStartComment, + onCancelComment, + onDraftChange, + onSubmitComment, + onDeleteComment +}: { + line: RenderableDiffLine + title: string + index: number + comments: DiffComment[] + activeCommentLine: number | null + commentDraft: string + commentsBusy: boolean + onStartComment: (lineNumber: number) => void + onCancelComment: () => void + onDraftChange: (value: string) => void + onSubmitComment: (lineNumber: number) => void + onDeleteComment: (commentId: string) => void +}) { + const commentLine = line.newLineNumber + const isCommenting = commentLine !== undefined && activeCommentLine === commentLine + const canComment = commentLine !== undefined + // Why: review notes are anchored to the modified side, so the single mobile + // gutter should show the same line number the note will reference. + const gutterLineNumber = line.newLineNumber ?? line.oldLineNumber ?? '' + return ( + + + {gutterLineNumber} + + + {line.kind === 'add' ? '+ ' : line.kind === 'delete' ? '- ' : ' '} + + + + {canComment ? ( + [ + styles.diffCommentAddButton, + pressed && styles.diffCommentAddButtonPressed, + commentsBusy && styles.diffCommentButtonDisabled + ]} + disabled={commentsBusy} + onPress={() => { + if (commentLine !== undefined) { + onStartComment(commentLine) + } + }} + accessibilityLabel={`Add note on line ${commentLine}`} + > + + + ) : null} + + {comments.length > 0 ? ( + + {comments.map((comment) => ( + + + + Line {comment.lineNumber} + onDeleteComment(comment.id)} + accessibilityLabel={`Delete note on line ${comment.lineNumber}`} + > + + + + {comment.body} + + ))} + + ) : null} + {isCommenting ? ( + + + + + Cancel + + { + if (commentLine !== undefined) { + onSubmitComment(commentLine) + } + }} + > + Save note + + + + ) : null} + + ) +} diff --git a/mobile/src/transport/client-context.tsx b/mobile/src/transport/client-context.tsx index e0975df60c2..ca09376e474 100644 --- a/mobile/src/transport/client-context.tsx +++ b/mobile/src/transport/client-context.tsx @@ -1,17 +1,21 @@ -// Single shared RpcClient per host, collapsing the old per-screen WebSocket connections. -// Design: docs/mobile-shared-client-per-host.md. +// Why: collapses the per-screen WebSocket connection model into a single +// shared RpcClient per host. Implements the design in +// docs/mobile-shared-client-per-host.md. +// +// Lifecycle rules: +// - First request for a host opens its client lazily. +// - Refcount tracks active subscribers; when it drops to zero we schedule +// a 30-second idle close timer. If a new subscriber arrives within that +// window we cancel and reuse the same client. +// - removeHost() forces an immediate close so re-pairing gets a fresh +// transport. +import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react' import { - createContext, - useCallback, - useContext, - useEffect, - useMemo, - useRef, - useState, - type ReactNode -} from 'react' + HostClientContext, + useHostClientContext as useRpcClientContext, + type HostClientContextValue +} from './host-client-context-contract' import type { RpcClient } from './rpc-client' -import type { StableLogicalRpcClient } from './stable-logical-rpc-client' import { subscribeConnectionRevivalTriggers } from './connection-revival-triggers' import { HostClientOpenRegistry } from './host-client-open-registry' import { @@ -20,8 +24,6 @@ import { } from './host-client-acquisition-registry' import { HostOpenRetryScheduler } from './host-open-retry-scheduler' import { openHostClientEntry, type HostClientStoreEntry } from './host-entry-opener' -import { shouldPreserveActiveRelay } from './relay-reconnect-preservation' -import { recordConnectionRevival } from './persisted-connection-log-store' import { createHostClientSelectors, listHostClients, @@ -34,11 +36,10 @@ import { } from './host-client-context-state' import { mountAgentSync } from './agent-sync-connection' import type { ConnectionState, HostProfile } from './types' -import type { RpcClientContextValue } from './rpc-client-context-contract' - type StoreEntry = HostClientStoreEntry - -const Ctx = createContext(null) +export type { HostClientAcquisition } from './host-client-acquisition-registry' +export { useRpcClientContext } +export type RpcClientContextValue = HostClientContextValue export function RpcClientProvider({ children }: { children: ReactNode }) { // Why: entries in a ref so state changes don't re-render the whole tree; propagation goes through per-host listener Sets. @@ -242,13 +243,6 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { const forceReconnect = useCallback( async (hostId: string) => { const entry = storeRef.current.get(hostId) - const logical = entry?.client as Partial | undefined - if (entry && shouldPreserveActiveRelay(entry, logical)) { - // Keep a Relay-active host on its existing recovery state; rebuilding the - // facade starts the unreachable direct endpoint before Relay can race it. - entry.client.notifyForeground('app-resume') - return - } // Why: ownership survives explicit close/re-pair while observers never become synthetic owners. const savedRefCount = acquisitionsRef.current.count(hostId) manualDemandRef.current.add(hostId) @@ -314,8 +308,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { for (const hostId of pendingAcquisitionsRef.current.keys()) { retrySchedulerRef.current?.expedite(hostId) } - for (const [hostId, entry] of storeRef.current) { - recordConnectionRevival(hostId, reason) + for (const entry of storeRef.current.values()) { try { entry.client.notifyForeground(reason) } catch { @@ -325,7 +318,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { }) }, []) - const value = useMemo( + const value = useMemo( () => ({ acquire, release, @@ -358,15 +351,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { ] ) - return {children} -} - -export function useRpcClientContext(): RpcClientContextValue { - const ctx = useContext(Ctx) - if (!ctx) { - throw new Error('useHostClient must be used inside ') - } - return ctx + return {children} } // Primary hook for screens: acquires the shared client on mount, releases on unmount, re-renders on state change. diff --git a/mobile/src/transport/host-client-context-contract.ts b/mobile/src/transport/host-client-context-contract.ts new file mode 100644 index 00000000000..5397689deb6 --- /dev/null +++ b/mobile/src/transport/host-client-context-contract.ts @@ -0,0 +1,50 @@ +// Why: the shared-client context contract lives apart from the provider so +// consumer hooks depend on the shape, not the provider's connection plumbing. +import { createContext, useContext } from 'react' +import type { HostClientAcquisition } from './host-client-acquisition-registry' +import type { RpcClient } from './rpc-client' +import type { MobileConnectionPath } from './stable-logical-rpc-client' +import type { ConnectionState, HostProfile } from './types' + +export type HostClientContextValue = { + acquire: ( + hostId: string, + acquisition: HostClientAcquisition, + host?: HostProfile + ) => RpcClient | null + release: (hostId: string, acquisition: HostClientAcquisition) => void + releaseAndCloseIfUnused: (hostId: string, acquisition: HostClientAcquisition) => void + closeIfUnused: (hostId: string) => void + forceReconnect: (hostId: string) => Promise + refreshHostClient: (hostId: string) => void + forgetHostClient: (hostId: string) => void + disconnectHostClient: (hostId: string) => void + getState: (hostId: string) => ConnectionState + // null means no client entry or pending open exists yet. + getKnownState: (hostId: string) => ConnectionState | null + getReconnectAttempt: (hostId: string) => number + // Why: timestamp (ms epoch) of the last successful 'connected' state + // transition for this host, or null if never connected this session. + // Used by the UI to escalate "Reconnecting…" into a "host appears + // unreachable, re-pair?" prompt. + getLastConnectedAt: (hostId: string) => number | null + getActivePath: (hostId: string) => MobileConnectionPath + getPendingPath: (hostId: string) => MobileConnectionPath | null + subscribeHostState: (hostId: string, listener: (state: ConnectionState) => void) => () => void + getAllClients: () => Array<{ hostId: string; client: RpcClient }> + subscribeAllHosts: (listener: () => void) => () => void + // Why: lets the home screen feed already-loaded HostProfiles in so we + // don't pay loadHosts() latency twice (once in the focus-effect, again + // inside openEntry). + primeHosts: (hosts: HostProfile[]) => void +} + +export const HostClientContext = createContext(null) + +export function useHostClientContext(): HostClientContextValue { + const ctx = useContext(HostClientContext) + if (!ctx) { + throw new Error('useHostClient must be used inside ') + } + return ctx +} diff --git a/src/cli/handlers/orchestration-handler-test-harness.ts b/src/cli/handlers/orchestration-handler-test-harness.ts new file mode 100644 index 00000000000..4e72bbb8c8a --- /dev/null +++ b/src/cli/handlers/orchestration-handler-test-harness.ts @@ -0,0 +1,64 @@ +import { vi } from 'vitest' +import { RuntimeClientError } from '../runtime-client' + +// Why: shared mock/stub harness for the orchestration CLI handler spec, split +// out so the spec file stays under the max-lines budget. The spec's vi.mock +// factories reference these mocks, so this module must not import +// './orchestration' (it would load the mocked modules before these bindings +// initialize). + +export const callMock = vi.fn() +export const getTerminalHandleMock = vi.fn() + +const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE +const originalPaneKey = process.env.ORCA_PANE_KEY + +export function lifecycleGroupRecipientError(type: 'worker_done' | 'heartbeat'): string { + return `${type} messages belong to one exact Dispatch and cannot target a group address.` +} + +export function staleHandleError(): RuntimeClientError { + return new RuntimeClientError('terminal_handle_stale', 'terminal_handle_stale') +} + +// Queues the stale-handle remint chain shared by coordinator commands: +// stale terminal.show → resolvePane returns liveHandle → downstream RPC result. +export function stubStaleHandleRemint(liveHandle: string, downstream: unknown): void { + callMock + .mockRejectedValueOnce(staleHandleError()) + .mockResolvedValueOnce({ result: { terminal: { handle: liveHandle } } }) + .mockResolvedValueOnce(downstream) +} + +// Queues a stale terminal.show followed by a resolvePane remint that fails with `error`. +export function stubStaleHandleRemintFailure(error: RuntimeClientError): void { + callMock.mockRejectedValueOnce(staleHandleError()).mockRejectedValueOnce(error) +} + +// afterEach: restore the terminal-identity env vars a test mutated. +export function restoreTerminalIdentityEnv(): void { + getTerminalHandleMock.mockReset() + if (originalTerminalHandle === undefined) { + delete process.env.ORCA_TERMINAL_HANDLE + } else { + process.env.ORCA_TERMINAL_HANDLE = originalTerminalHandle + } + if (originalPaneKey === undefined) { + delete process.env.ORCA_PANE_KEY + } else { + process.env.ORCA_PANE_KEY = originalPaneKey + } +} + +export type CliFlagMap = Map + +// Builds the standard handler invocation the whole spec shares: json mode, +// fixed cwd, and the callMock-backed client. +export function handlerInvoker( + handler: (ctx: never) => unknown +): (flags: CliFlagMap) => Promise { + return (flags) => + Promise.resolve( + handler({ flags, client: { call: callMock }, cwd: '/tmp/repo', json: true } as never) + ) +} diff --git a/src/cli/handlers/orchestration.test.ts b/src/cli/handlers/orchestration.test.ts index b05ff1f3cc0..4e38a132e3f 100644 --- a/src/cli/handlers/orchestration.test.ts +++ b/src/cli/handlers/orchestration.test.ts @@ -1,12 +1,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -const callMock = vi.fn() -const getTerminalHandleMock = vi.hoisted(() => vi.fn()) -const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE -const originalPaneKey = process.env.ORCA_PANE_KEY -function lifecycleGroupRecipientError(type: 'worker_done' | 'heartbeat'): string { - return `${type} messages belong to one exact Dispatch and cannot target a group address.` -} +import { + callMock, + getTerminalHandleMock, + handlerInvoker, + lifecycleGroupRecipientError, + restoreTerminalIdentityEnv, + staleHandleError, + stubStaleHandleRemint, + stubStaleHandleRemintFailure, + type CliFlagMap +} from './orchestration-handler-test-harness' // Why: isolate the handler's flag-to-param mapping; printResult only writes output. vi.mock('../format', () => ({ printResult: vi.fn() })) @@ -16,37 +19,7 @@ import { ORCHESTRATION_HANDLERS } from './orchestration' import { RuntimeClientError } from '../runtime-client' import { printResult } from '../format' -function staleHandleError(): RuntimeClientError { - return new RuntimeClientError('terminal_handle_stale', 'terminal_handle_stale') -} - -// Queues the stale-handle remint chain shared by coordinator commands: -// stale terminal.show → resolvePane returns liveHandle → downstream RPC result. -function stubStaleHandleRemint(liveHandle: string, downstream: unknown): void { - callMock - .mockRejectedValueOnce(staleHandleError()) - .mockResolvedValueOnce({ result: { terminal: { handle: liveHandle } } }) - .mockResolvedValueOnce(downstream) -} - -// Queues a stale terminal.show followed by a resolvePane remint that fails with `error`. -function stubStaleHandleRemintFailure(error: RuntimeClientError): void { - callMock.mockRejectedValueOnce(staleHandleError()).mockRejectedValueOnce(error) -} - -afterEach(() => { - getTerminalHandleMock.mockReset() - if (originalTerminalHandle === undefined) { - delete process.env.ORCA_TERMINAL_HANDLE - } else { - process.env.ORCA_TERMINAL_HANDLE = originalTerminalHandle - } - if (originalPaneKey === undefined) { - delete process.env.ORCA_PANE_KEY - } else { - process.env.ORCA_PANE_KEY = originalPaneKey - } -}) +afterEach(restoreTerminalIdentityEnv) describe('orchestration send structured payload flags', () => { beforeEach(() => { @@ -56,13 +29,7 @@ describe('orchestration send structured payload flags', () => { delete process.env.ORCA_PANE_KEY }) - const invokeSend = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration send']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) + const invokeSend = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration send']) it('serializes common worker payload fields as JSON', async () => { await invokeSend( @@ -337,29 +304,9 @@ describe('orchestration dispatch coordinator handle', () => { delete process.env.ORCA_PANE_KEY }) - const invokeDispatch = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration dispatch']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) - - const invokeDispatchShow = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration dispatch-show']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) - - const invokeRun = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration coordinator-start']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) + const invokeDispatch = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration dispatch']) + const invokeDispatchShow = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration dispatch-show']) + const invokeRun = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration coordinator-start']) it('remints a stale coordinator env handle from the caller pane key', async () => { process.env.ORCA_TERMINAL_HANDLE = 'term_stale_coord' @@ -493,13 +440,8 @@ describe('orchestration dispatch Forget + raw read CLI handlers (W-T2)', () => { callMock.mockReset() }) - const invoke = (key: string, flags: Map) => - ORCHESTRATION_HANDLERS[key]({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) + const invoke = (key: string, flags: CliFlagMap) => + handlerInvoker(ORCHESTRATION_HANDLERS[key])(flags) it('dispatch-forget invokes dispatchForget with the task and expected failure id', async () => { callMock.mockResolvedValue({ @@ -562,13 +504,7 @@ describe('orchestration task-create caller handle', () => { delete process.env.ORCA_PANE_KEY }) - const invokeTaskCreate = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration task-create']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) + const invokeTaskCreate = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration task-create']) it('records a live env terminal handle as task creator', async () => { process.env.ORCA_TERMINAL_HANDLE = 'term_creator' @@ -713,21 +649,8 @@ describe('orchestration timeout flag validation', () => { delete process.env.ORCA_PANE_KEY }) - const invokeCheck = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration check']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) - - const invokeAsk = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration ask']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) + const invokeCheck = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration check']) + const invokeAsk = handlerInvoker(ORCHESTRATION_HANDLERS['orchestration ask']) it.each(invalidTimeoutValues)('rejects invalid check --timeout-ms: %s', async (_label, value) => { const flags = new Map([ @@ -953,11 +876,9 @@ describe('orchestration task-list brief output', () => { }) vi.mocked(printResult).mockClear() - await ORCHESTRATION_HANDLERS['orchestration task-list']({ - flags: new Map([['brief', true]]), - client: { call: callMock }, - json: true - } as never) + await handlerInvoker(ORCHESTRATION_HANDLERS['orchestration task-list'])( + new Map([['brief', true]]) + ) expect(callMock).toHaveBeenCalledWith( 'orchestration.taskList', @@ -977,11 +898,9 @@ describe('orchestration task-list brief output', () => { callMock.mockReset().mockResolvedValue({ result: { tasks: serverTasks, count: 1 } }) vi.mocked(printResult).mockClear() - await ORCHESTRATION_HANDLERS['orchestration task-list']({ - flags: new Map([['brief', true]]), - client: { call: callMock }, - json: true - } as never) + await handlerInvoker(ORCHESTRATION_HANDLERS['orchestration task-list'])( + new Map([['brief', true]]) + ) const response = vi.mocked(printResult).mock.calls[0]?.[0] as { result: { tasks: { spec: string; spec_truncated: boolean }[] }