From 6f3018576d92afe300051a1567d71ac3c40f43de Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:29:17 -0700 Subject: [PATCH] fix(mobile): show review notes' agent launch progress and failures, once "New Agent Session" left the sheet open with no progress for the whole launch (up to a minute while a terminal agent readies), so a second tap started a second agent, and a launch that never started or could not be confirmed rejected an unobserved promise, showing nothing. The sheet now closes on tap, the review screen says "Starting an agent...", one launch runs at a time, and every outcome lands in the review screen's status line. Marking notes sent now reads the screen state when the launch settles, so a note written during the wait is not dropped by the whole-list save. --- ...se-mobile-diff-review-send-actions.test.ts | 100 ++++++++++++++---- .../use-mobile-diff-review-send-actions.ts | 56 +++++++--- 2 files changed, 121 insertions(+), 35 deletions(-) diff --git a/mobile/src/session/use-mobile-diff-review-send-actions.test.ts b/mobile/src/session/use-mobile-diff-review-send-actions.test.ts index 1188c44ecb4..74c018abdc7 100644 --- a/mobile/src/session/use-mobile-diff-review-send-actions.test.ts +++ b/mobile/src/session/use-mobile-diff-review-send-actions.test.ts @@ -3,6 +3,7 @@ import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { DiffComment } from '../../../src/shared/diff-comment-types' import type { RpcClient } from '../transport/rpc-client' +import type { RpcResponse } from '../transport/types' import type { ReviewScreenState } from './mobile-diff-review-screen-model' import { isMobileNativeChatInputStale, @@ -59,30 +60,35 @@ const LAUNCH_CAPABILITIES = [ 'agent.launch.replay-required.v1' ] -// Answers the agent loader's reads and the launch, by method. -function launchClient(promptOutcome: 'handed-to-terminal' | 'not-delivered') { - const reply = (result: unknown) => ({ - id: 'rpc', - ok: true as const, - result, - _meta: { runtimeId: 'r' } +function rpcReply(result: unknown) { + return { id: 'rpc', ok: true as const, result, _meta: { runtimeId: 'r' } } +} + +function launchedReply(promptOutcome: 'handed-to-terminal' | 'not-delivered') { + return rpcReply({ + outcome: { kind: 'terminal', handle: 'term-1' }, + worktreeId: 'wt-1', + receipt: { mode: 'terminal', preferred: 'terminal', reason: 'user_default', detail: 'd' }, + prompt: { delivery: 'submit', outcome: promptOutcome } }) - const sendRequest = vi.fn(async (method: string, _params?: unknown) => { +} + +// Answers the agent loader's reads and the launch, by method. +function launchClient( + promptOutcome: 'handed-to-terminal' | 'not-delivered', + launchReply: () => Promise = async () => launchedReply(promptOutcome) +) { + const sendRequest = vi.fn(async (method: string, _params?: unknown): Promise => { if (method === 'repo.list') { - return reply({ repos: [{ id: 'wt-1' }] }) + return rpcReply({ repos: [{ id: 'wt-1' }] }) } if (method === 'settings.get') { - return reply({ settings: { defaultTuiAgent: 'codex' } }) + return rpcReply({ settings: { defaultTuiAgent: 'codex' } }) } if (method === 'preflight.detectAgents') { - return reply(['codex']) + return rpcReply(['codex']) } - return reply({ - outcome: { kind: 'terminal', handle: 'term-1' }, - worktreeId: 'wt-1', - receipt: { mode: 'terminal', preferred: 'terminal', reason: 'user_default', detail: 'd' }, - prompt: { delivery: 'submit', outcome: promptOutcome } - }) + return launchReply() }) return { client: requestPortRpcClient(sendRequest), sendRequest } } @@ -102,8 +108,10 @@ describe('useMobileDiffReviewSendActions', () => { let setActionError: ReturnType let setSendSheet: ReturnType let saveCommentsAndReviewState: ReturnType + let screenState: ReviewScreenState = READY beforeEach(() => { + screenState = READY clipboardMock.setStringAsync.mockReset().mockResolvedValue(true) resetMobileNativeChatStaleInputForTests() setActionError = vi.fn() @@ -124,7 +132,7 @@ describe('useMobileDiffReviewSendActions', () => { connState: 'connected', hostCapabilities: LAUNCH_CAPABILITIES, worktreeId: 'wt-1', - screenState: READY, + screenState, setActionError, setSendSheet, saveCommentsAndReviewState @@ -318,4 +326,60 @@ describe('useMobileDiffReviewSendActions', () => { "The agent started, but the notes weren't sent. Use Copy Notes to paste them." ) }) + + it('shows a launch that did not start on the review screen instead of rejecting', async () => { + await mount( + launchClient('handed-to-terminal', async () => ({ + id: 'rpc', + ok: false, + error: { code: 'selector_not_found', message: 'Workspace not found' }, + _meta: { runtimeId: 'r' } + })).client + ) + await act(async () => { + await actions?.createTerminalAndSend([COMMENT]) + }) + expect(setSendSheet).toHaveBeenCalledWith(null) + expect(setActionError).toHaveBeenLastCalledWith('Workspace not found') + expect(saveCommentsAndReviewState).not.toHaveBeenCalled() + }) + + it('starts one agent for a double tap and keeps notes written while it starts', async () => { + let answerLaunch: (reply: RpcResponse) => void = () => {} + const { client, sendRequest } = launchClient( + 'handed-to-terminal', + () => + new Promise((resolve) => { + answerLaunch = resolve + }) + ) + await mount(client) + let first: Promise | undefined + await act(async () => { + first = actions?.createTerminalAndSend([COMMENT]) + await actions?.createTerminalAndSend([COMMENT]) + }) + expect(setActionError).toHaveBeenLastCalledWith('Starting an agent...') + const written: DiffComment = { ...COMMENT, id: 'comment-2', body: 'written meanwhile' } + screenState = { ...READY, comments: [COMMENT, written] } + await act(async () => { + renderer?.update(createElement(Harness)) + }) + await act(async () => { + answerLaunch(launchedReply('handed-to-terminal')) + await first + }) + expect( + sendRequest.mock.calls.filter(([method]) => method === 'agent.launchReplay') + ).toHaveLength(1) + expect(saveCommentsAndReviewState).toHaveBeenCalledWith( + [ + expect.objectContaining({ id: 'comment-1', sentAt: expect.any(Number) }), + expect.not.objectContaining({ sentAt: expect.anything() }) + ], + READY.reviewState + ) + expect(saveCommentsAndReviewState.mock.calls[0]?.[0]?.[1]?.id).toBe('comment-2') + expect(setActionError).toHaveBeenLastCalledWith('Review notes sent') + }) }) diff --git a/mobile/src/session/use-mobile-diff-review-send-actions.ts b/mobile/src/session/use-mobile-diff-review-send-actions.ts index fb77ae386bd..42b0688eb05 100644 --- a/mobile/src/session/use-mobile-diff-review-send-actions.ts +++ b/mobile/src/session/use-mobile-diff-review-send-actions.ts @@ -1,4 +1,4 @@ -import { useCallback, type Dispatch, type SetStateAction } from 'react' +import { useCallback, useEffect, useRef, type Dispatch, type SetStateAction } from 'react' import type { DiffComment, MobileDiffReviewState } from '../../../src/shared/diff-comment-types' import type { ConnectionState } from '../transport/types' import type { RpcClient } from '../transport/rpc-client' @@ -65,19 +65,29 @@ export function useMobileDiffReviewSendActions(input: SendActionsInput) { await saveCommentsAndReviewState(nextComments, screenState.reviewState) }, [saveCommentsAndReviewState, screenState]) + // Read when a send settles, not when it was tapped: an agent launch can take a minute, and notes + // written meanwhile must survive the whole-list save below. + const latestScreenStateRef = useRef(screenState) + useEffect(() => { + latestScreenStateRef.current = screenState + }, [screenState]) + // One launch at a time: each tap is a new operation, so a second tap would start a second agent. + const agentLaunchInFlightRef = useRef(false) + const markNotesSent = useCallback( async (comments: readonly DiffComment[]) => { - if (screenState.kind !== 'ready') { + const current = latestScreenStateRef.current + if (current.kind !== 'ready') { return } const next = markMobileDiffCommentsSent( - screenState.comments, + current.comments, new Set(comments.map((comment) => comment.id)), Date.now() ) - await saveCommentsAndReviewState(next, screenState.reviewState) + await saveCommentsAndReviewState(next, current.reviewState) }, - [saveCommentsAndReviewState, screenState] + [saveCommentsAndReviewState] ) const sendPromptToTerminal = useCallback( @@ -116,20 +126,32 @@ export function useMobileDiffReviewSendActions(input: SendActionsInput) { if (!client || connState !== 'connected') { throw new Error('Waiting for desktop...') } - // The desktop asks which agent to use; the review screen has no picker, so this takes the - // desktop's own default resolution with no saved recipe. - const result = await launchAgentWithPrompt({ - client, - hostCapabilities, - worktreeId, - prompt: formatMobileDiffReviewPrompt(comments), - actionId: null, - launchSource: 'notes_send' - }) - if (result.kind === 'not-started' || result.kind === 'unconfirmed') { - throw new Error(result.message) + if (agentLaunchInFlightRef.current) { + return } + agentLaunchInFlightRef.current = true + // Closed up front so the wait (up to a minute for a terminal agent) shows its progress here. setSendSheet(null) + setActionError('Starting an agent...') + let result + try { + // The desktop asks which agent to use; the review screen has no picker, so this takes the + // desktop's own default resolution with no saved recipe. + result = await launchAgentWithPrompt({ + client, + hostCapabilities, + worktreeId, + prompt: formatMobileDiffReviewPrompt(comments), + actionId: null, + launchSource: 'notes_send' + }) + } finally { + agentLaunchInFlightRef.current = false + } + if (result.kind === 'not-started' || result.kind === 'unconfirmed') { + setActionError(result.message) + return + } if (result.kind === 'prompt-not-sent') { // Notes stay unsent so Copy Notes and a later send still carry them. setActionError(