From c36f2ce14ed25df97a9f965720822824ef7d575d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:00:48 -0700 Subject: [PATCH] fix(mobile): stop a superseded answer from clearing the fence banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A chain that finished its key plan reported `true` even after a newer answer superseded it. The route sends through useNativeChatAcceptedAction, whose accepted callback retires the send-error banner — and that callback runs after the successor's fence report, because finishTurn() fires in `finally`, before the chain's own promise settles. So the successful predecessor deterministically wiped the fence message the successor had just raised: every healthy write took that branch, which made the previous commit's report vacuous exactly where it mattered. A superseded chain now reports no success, matching every other supersession checkpoint in this hook. --- ...use-mobile-native-chat-answer-send.test.ts | 169 ++++++++++++++++++ .../use-mobile-native-chat-answer-send.ts | 8 +- 2 files changed, 175 insertions(+), 2 deletions(-) diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts index a0e468ec327..184d1bdcc37 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts @@ -17,6 +17,7 @@ import { resetMobileNativeChatTerminalWritesForTests } from './mobile-native-chat-terminal-write-lock' import { useMobileNativeChatAnswerSend } from './use-mobile-native-chat-answer-send' +import { useNativeChatAcceptedAction } from './use-native-chat-action-outcomes' type AnswerSend = ReturnType @@ -45,8 +46,13 @@ describe('useMobileNativeChatAnswerSend', () => { let mountedClient: RpcClient | null = null let mountedOnSendError: ((message: string) => void) | null = null let mountedAgent: AgentType = 'claude' + // The route sends through useNativeChatAcceptedAction, whose accepted callback + // retires the shared send-error banner (use-mobile-native-chat-controller.ts). + let acceptedAnswerAsk: AnswerSend['answerAsk'] | null = null + let onAccepted = vi.fn() beforeEach(() => { + onAccepted = vi.fn() vi.useFakeTimers() globalThis.IS_REACT_ACT_ENVIRONMENT = true resetMobileNativeChatStaleInputForTests() @@ -57,6 +63,7 @@ describe('useMobileNativeChatAnswerSend', () => { act(() => renderer?.unmount()) renderer = null answerSend = null + acceptedAnswerAsk = null mountedClient = null mountedOnSendError = null mountedAgent = 'claude' @@ -74,6 +81,7 @@ describe('useMobileNativeChatAnswerSend', () => { streamIdentity: 'host\0worktree\0tab\0session', onSendError: mountedOnSendError! }) + acceptedAnswerAsk = useNativeChatAcceptedAction(answerSend.answerAsk, onAccepted) return null } @@ -640,4 +648,165 @@ describe('useMobileNativeChatAnswerSend', () => { expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) releaseMobileNativeChatTerminalWrite('terminal') }) + + it('does not retire the fence banner when the superseded answer lands', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + // The healthy path: the first answer LANDS, which is also the case that fences + // hardest — its key moved the live selector. + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + // A superseded chain reporting success would clear the banner it just raised — + // the accepted hook runs after the fence, so the user would see nothing at all. + expect(onAccepted).not.toHaveBeenCalled() + expect(sendRequest).toHaveBeenCalledTimes(1) + }) + + it('does not retire the fence banner when a superseded pasted answer lands', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError, 'grok') + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = acceptedAnswerAsk?.(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + + // The pasted shape commits with Enter, so a superseded chain is doubly unsafe + // to report as accepted — the answer it committed is not the one on screen. + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(onAccepted).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('fences a third answer behind an already-fenced successor, reporting once', async () => { + const onSendError = vi.fn() + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + let third: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + third = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(1) + + await act(async () => { + settle[0]!(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + await expect(third).resolves.toBe(false) + // The middle chain sent nothing, so only the verdict it INHERITED can stop the + // third from replaying a from-scratch plan onto the advanced selector. + expect(sendRequest).toHaveBeenCalledTimes(1) + // Only the newest chain owns the error surface. + expect(onSendError).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer not sent — check chat before retrying') + }) + + it('queues a late third answer behind the successor already on the wire', async () => { + const settle: Array<(response: unknown) => void> = [] + const sendRequest = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + let first: Promise | undefined + let second: Promise | undefined + let third: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + // Nothing landed, so the successor is cleared and puts its own key on the wire. + await act(async () => { + settle[0]!({ ...acceptedResponse(), result: { send: { accepted: false } } }) + await Promise.resolve() + }) + expect(sendRequest).toHaveBeenCalledTimes(2) + + await act(async () => { + third = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + await Promise.resolve() + }) + // The first chain unwound while the second was mid-write: it must not have + // dropped the second's turn, or this one writes into the same PTY concurrently. + expect(sendRequest).toHaveBeenCalledTimes(2) + + await act(async () => { + settle[1]!(acceptedResponse()) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + await expect(third).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(2) + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.ts b/mobile/src/session/use-mobile-native-chat-answer-send.ts index 8e3a8d7504c..abb79a9b50a 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.ts @@ -247,7 +247,10 @@ export function useMobileNativeChatAnswerSend(args: { if (generationRef.current !== generation) { return false } - return (await sendTerminal(formatAskAnswer(prompt, selections), true)) || fail() + // A superseded chain must not report success either: an accepted answer + // retires the shared send-error banner, wiping the successor's fence. + const sent = (await sendTerminal(formatAskAnswer(prompt, selections), true)) || fail() + return sent && generationRef.current === generation } const groups = resolveNativeChatTranscriptAgent(agentRef.current) === 'codex' @@ -270,7 +273,8 @@ export function useMobileNativeChatAnswerSend(args: { deadline += MOBILE_NATIVE_CHAT_QUESTION_STEP_MS } } - return groups.length > 0 + // Superseded on the last key: same as above, the successor owns the surface. + return groups.length > 0 && generationRef.current === generation } finally { // Any accepted key changed the live selector, so a queued replacement // cannot safely apply its from-scratch key plan to that new position.