fix(mobile): clear native-chat composer optimistically at send time (#10226)

* fix(mobile): clear native-chat composer optimistically at send time

Over relay the send RPC round trip is visible and a lost ack (or a
relay/direct cutover) could strand the sent prompt in the composer
forever: the unconfirmed-send deadline dropped its tracking entry, so a
late transcript echo could never clear the draft.

Clear the draft at send time and restore it only on a definite
rejection. holdUnconfirmedSend now only manages the delivery-unconfirmed
notice; it no longer touches drafts.

* fix(mobile): isolate question answers from composer drafts
This commit is contained in:
Brennan Benson 2026-07-26 12:44:39 -07:00 committed by GitHub
parent 8f5a45401f
commit 8b25cfc0f8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 322 additions and 95 deletions

View File

@ -48,7 +48,7 @@ export function MobileNativeChatOverlay({
onAnswerAsk={controller.handleNativeChatAnswerAsk}
onCancelAsk={controller.handleNativeChatCancelAsk}
question={controller.nativeChatQuestion}
onAnswerQuestion={controller.handleNativeChatSend}
onAnswerQuestion={controller.handleNativeChatQuestionAnswer}
permission={controller.nativeChatPermission}
onRespondPermission={controller.handleNativeChatRespondPermission}
onOpenFile={controller.handleNativeChatOpenFile}

View File

@ -5,6 +5,8 @@ import type { RpcClient } from '../transport/rpc-client'
const acceptSend = vi.fn()
const captureSendOrigin = vi.fn()
const clearDraftForSend = vi.fn()
const restoreRejectedDraft = vi.fn()
const holdUnconfirmedSend = vi.fn()
// The controller composes many session hooks; each is mocked to a minimal shape
@ -21,6 +23,8 @@ vi.mock('./use-mobile-native-chat-drafts', () => ({
setComposerText: vi.fn(),
pending: [],
captureSendOrigin,
clearDraftForSend,
restoreRejectedDraft,
acceptSend,
holdUnconfirmedSend
})
@ -180,6 +184,9 @@ describe('useMobileNativeChatController handleNativeChatSend', () => {
})
expect(accepted).toBe(true)
expect(acceptSend).toHaveBeenCalledWith(ORIGIN, 'look', ['file:///a.jpg'])
// Optimistic clear happens at send time, never a restore on success.
expect(clearDraftForSend).toHaveBeenCalledWith(ORIGIN, 'look')
expect(restoreRejectedDraft).not.toHaveBeenCalled()
})
it('holds an unknown-outcome send without posting the optimistic echo', async () => {
@ -191,6 +198,9 @@ describe('useMobileNativeChatController handleNativeChatSend', () => {
expect(accepted).toBe(true)
expect(acceptSend).not.toHaveBeenCalled()
expect(holdUnconfirmedSend).toHaveBeenCalledWith(ORIGIN, 'look', expect.any(Function))
// Delivery-unknown usually means delivered — keep the composer clear.
expect(clearDraftForSend).toHaveBeenCalledWith(ORIGIN, 'look')
expect(restoreRejectedDraft).not.toHaveBeenCalled()
})
it('preserves the unknown outcome on the WithOutcome surface for paste-first callers', async () => {
@ -214,5 +224,20 @@ describe('useMobileNativeChatController handleNativeChatSend', () => {
expect(accepted).toBe(false)
expect(acceptSend).not.toHaveBeenCalled()
expect(onSendError).toHaveBeenCalledWith('Message not sent')
// A definite rejection puts the optimistically-cleared text back.
expect(restoreRejectedDraft).toHaveBeenCalledWith(ORIGIN, 'look')
})
it('does not restore a rejected question answer into the composer', async () => {
sendWithOutcome.mockResolvedValue('rejected')
let accepted = true
await act(async () => {
accepted = await controller!.handleNativeChatQuestionAnswer('1')
})
expect(accepted).toBe(false)
expect(clearDraftForSend).not.toHaveBeenCalled()
expect(restoreRejectedDraft).not.toHaveBeenCalled()
expect(onSendError).toHaveBeenCalledWith('Message not sent')
})
})

View File

@ -21,13 +21,13 @@ import {
sendMobileNativeChatMessageWithOutcome,
type MobileNativeChatSendOutcome
} from './mobile-native-chat-send'
import { healMobileNativeChatStaleInput } from './mobile-native-chat-stale-input'
import { useMobileNativeChatAnswerSend } from './use-mobile-native-chat-answer-send'
import {
useMobileNativeChatDrafts,
type MobileNativeChatPendingMessage
} from './use-mobile-native-chat-drafts'
import { useMobileNativeChatFileSearch } from './use-mobile-native-chat-file-search'
import { useMobileNativeChatMessageSend } from './use-mobile-native-chat-message-send'
import { useMobileNativeChatSession } from './use-mobile-native-chat-session'
import { useMobileNativeChatPrompts } from './use-mobile-native-chat-prompts'
import { useMobileNativeChatStop } from './use-mobile-native-chat-stop'
@ -62,6 +62,7 @@ export type MobileNativeChatController = {
handleNativeChatStop: () => void
nativeChatFilePaths: string[]
loadNativeChatFiles: (query: string) => void
handleNativeChatQuestionAnswer: (text: string) => Promise<boolean>
handleNativeChatSend: (text: string, images?: string[]) => Promise<boolean>
/** Outcome-preserving send: callers that pasted terminal input beforehand
* (image sends) must see 'unknown' to heal a possibly-orphaned paste. */
@ -123,6 +124,8 @@ export function useMobileNativeChatController(args: {
setComposerText: setChatComposerText,
pending: chatPending,
captureSendOrigin,
clearDraftForSend,
restoreRejectedDraft,
acceptSend,
holdUnconfirmedSend
} = useMobileNativeChatDrafts({
@ -236,66 +239,22 @@ export function useMobileNativeChatController(args: {
worktreeId
})
const handleNativeChatSendWithOutcome = useCallback(
async (text: string, images?: string[]): Promise<MobileNativeChatSendOutcome> => {
const handle = activeHandleRef.current
const origin = captureSendOrigin(text)
if (!client || !handle || !origin || !nativeChatInputLeaseReady) {
onSendError('Message not sent (disconnected)')
return 'rejected'
}
// The composer may still hold an orphaned image paste from an earlier send
// (#10228); submitting on top of it would glue the image onto this message.
// Also covers question-card answers, which reach this send directly.
const healArgs = { client, terminal: handle, deviceToken: deviceTokenRef.current }
if (!(await healMobileNativeChatStaleInput(healArgs))) {
onSendError('Message not sent')
return 'rejected'
}
const outcome = await sendMobileNativeChatMessageWithOutcome({
client,
terminal: handle,
text,
...(deviceTokenRef.current
? { mobileClient: { id: deviceTokenRef.current, type: 'mobile' } }
: {})
})
if (outcome === 'unknown') {
// Why: an ack-lost send usually WAS delivered (issue seen on cellular
// relay) — verify via the transcript echo instead of a false "not sent".
holdUnconfirmedSend(origin, text, () =>
onSendError('Delivery unconfirmed — check chat before retrying')
)
return 'unknown'
}
if (outcome === 'rejected') {
onSendError('Message not sent')
return 'rejected'
}
// `images` are local preview URIs for the optimistic echo only — the actual
// image bytes already rode along as a bracketed paste before this text send.
acceptSend(origin, text, images)
return 'accepted'
},
[
acceptSend,
activeHandleRef,
captureSendOrigin,
client,
deviceTokenRef,
holdUnconfirmedSend,
nativeChatInputLeaseReady,
onSendError
]
)
// Boolean surface for callers with no pre-pasted input: 'unknown' stays true
// (the send usually landed; the optimistic echo is already held unconfirmed).
const handleNativeChatSend = useCallback(
async (text: string, images?: string[]): Promise<boolean> =>
(await handleNativeChatSendWithOutcome(text, images)) !== 'rejected',
[handleNativeChatSendWithOutcome]
)
const {
send: handleNativeChatSend,
sendWithOutcome: handleNativeChatSendWithOutcome,
answerQuestion: handleNativeChatQuestionAnswer
} = useMobileNativeChatMessageSend({
client,
enabled: nativeChatInputLeaseReady,
handleRef: activeHandleRef,
deviceTokenRef,
captureSendOrigin,
clearDraftForSend,
restoreRejectedDraft,
acceptSend,
holdUnconfirmedSend,
onSendError
})
return {
isTabChatView,
@ -319,6 +278,7 @@ export function useMobileNativeChatController(args: {
handleNativeChatStop,
nativeChatFilePaths,
loadNativeChatFiles,
handleNativeChatQuestionAnswer,
handleNativeChatSend,
handleNativeChatSendWithOutcome
}

View File

@ -85,6 +85,11 @@ describe('useMobileNativeChatDrafts', () => {
act(() => state?.setComposerText('from a'))
const originA = state?.captureSendOrigin('from a')
expect(originA).not.toBeNull()
act(() => {
if (originA) {
state?.clearDraftForSend(originA, 'from a')
}
})
await switchTo('b')
act(() => state?.setComposerText('from b'))
@ -101,6 +106,99 @@ describe('useMobileNativeChatDrafts', () => {
expect(state?.pending.map((pending) => pending.text)).toEqual(['from a'])
})
it('clears the composer at send time, before the RPC settles', async () => {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
act(() => {
if (origin) {
state?.clearDraftForSend(origin, 'ping')
}
})
expect(state?.composerText).toBe('')
})
it('restores the text on a definite rejection', async () => {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
act(() => {
if (origin) {
state?.clearDraftForSend(origin, 'ping')
state?.restoreRejectedDraft(origin, 'ping')
}
})
expect(state?.composerText).toBe('ping')
})
it('does not clobber newer edits when restoring a rejected send', async () => {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
act(() => {
if (origin) {
state?.clearDraftForSend(origin, 'ping')
}
})
act(() => state?.setComposerText('newer edit'))
act(() => {
if (origin) {
state?.restoreRejectedDraft(origin, 'ping')
}
})
expect(state?.composerText).toBe('newer edit')
})
it('restores a rejected send onto its originating tab only', async () => {
await mount('a')
act(() => state?.setComposerText('from a'))
const originA = state?.captureSendOrigin('from a')
act(() => {
if (originA) {
state?.clearDraftForSend(originA, 'from a')
}
})
await switchTo('b')
act(() => {
if (originA) {
state?.restoreRejectedDraft(originA, 'from a')
}
})
expect(state?.composerText).toBe('')
await switchTo('a')
expect(state?.composerText).toBe('from a')
})
it('keeps the composer clear when the echo lands after the unconfirmed deadline', async () => {
vi.useFakeTimers()
try {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
act(() => {
if (origin) {
state?.clearDraftForSend(origin, 'ping')
state?.holdUnconfirmedSend(origin, 'ping', vi.fn())
}
})
expect(state?.composerText).toBe('')
// A relay drop can stall the transcript stream past the deadline; the
// delivered prompt must not reappear in the composer when it recovers.
act(() => vi.advanceTimersByTime(25_000))
await act(async () =>
renderer?.update(
createElement(Harness, { tabId: 'a', messages: [userTextMessage('m1', 'ping')] })
)
)
expect(state?.composerText).toBe('')
} finally {
vi.useRealTimers()
}
})
it('clears one pending per landed message so duplicate sends are not all dropped', async () => {
await mount('a')
const origin = state?.captureSendOrigin('ping')
@ -279,25 +377,24 @@ describe('useMobileNativeChatDrafts', () => {
expect(state?.pending).toEqual([])
})
it('does not erase newer edits when an older send settles', async () => {
it('does not erase newer edits when an older send clears', async () => {
await mount('a')
act(() => state?.setComposerText('submitted'))
const origin = state?.captureSendOrigin('submitted')
act(() => state?.setComposerText('new edit'))
act(() => {
if (origin) {
state?.acceptSend(origin, 'submitted')
state?.clearDraftForSend(origin, 'submitted')
}
})
expect(state?.composerText).toBe('new edit')
})
it('clears the draft when an unconfirmed send lands in the transcript', async () => {
it('stays quiet when an unconfirmed send lands in the transcript', async () => {
vi.useFakeTimers()
try {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
const onUnconfirmed = vi.fn()
act(() => {
@ -305,14 +402,12 @@ describe('useMobileNativeChatDrafts', () => {
state?.holdUnconfirmedSend(origin, 'ping', onUnconfirmed)
}
})
expect(state?.composerText).toBe('ping')
await act(async () =>
renderer?.update(
createElement(Harness, { tabId: 'a', messages: [userTextMessage('m1', 'ping')] })
)
)
expect(state?.composerText).toBe('')
act(() => vi.advanceTimersByTime(30_000))
expect(onUnconfirmed).not.toHaveBeenCalled()
@ -399,11 +494,10 @@ describe('useMobileNativeChatDrafts', () => {
expect(state?.pending.map((pending) => pending.images)).toEqual([['file:///b.jpg']])
})
it('clears immediately when the transcript echo beat the ambiguous RPC rejection', async () => {
it('registers no deadline when the transcript echo beat the ambiguous RPC rejection', async () => {
vi.useFakeTimers()
try {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
const onUnconfirmed = vi.fn()
@ -418,7 +512,6 @@ describe('useMobileNativeChatDrafts', () => {
}
})
expect(state?.composerText).toBe('')
expect(vi.getTimerCount()).toBe(0)
act(() => vi.advanceTimersByTime(30_000))
expect(onUnconfirmed).not.toHaveBeenCalled()
@ -427,11 +520,10 @@ describe('useMobileNativeChatDrafts', () => {
}
})
it('surfaces uncertainty and keeps the draft when no echo lands before the deadline', async () => {
it('surfaces uncertainty when no echo lands before the deadline', async () => {
vi.useFakeTimers()
try {
await mount('a')
act(() => state?.setComposerText('ping'))
const origin = state?.captureSendOrigin('ping')
const onUnconfirmed = vi.fn()
act(() => {
@ -444,7 +536,6 @@ describe('useMobileNativeChatDrafts', () => {
expect(onUnconfirmed).not.toHaveBeenCalled()
act(() => vi.advanceTimersByTime(1))
expect(onUnconfirmed).toHaveBeenCalledTimes(1)
expect(state?.composerText).toBe('ping')
} finally {
vi.useRealTimers()
}
@ -631,6 +722,7 @@ describe('useMobileNativeChatDrafts', () => {
expect(origin).toMatchObject({ pendingKey: null })
act(() => {
if (origin) {
state?.clearDraftForSend(origin, 'start the session')
state?.acceptSend(origin, 'start the session')
}
})

View File

@ -46,6 +46,10 @@ export function useMobileNativeChatDrafts(args: {
setComposerText: Dispatch<SetStateAction<string>>
pending: MobileNativeChatPendingMessage[]
captureSendOrigin: (text: string) => MobileNativeChatSendOrigin | null
/** Clear the composer at send time, before the RPC settles. */
clearDraftForSend: (origin: MobileNativeChatSendOrigin, text: string) => void
/** Put the text back after a definite rejection, unless newer edits exist. */
restoreRejectedDraft: (origin: MobileNativeChatSendOrigin, text: string) => void
acceptSend: (origin: MobileNativeChatSendOrigin, text: string, images?: string[]) => void
holdUnconfirmedSend: (
origin: MobileNativeChatSendOrigin,
@ -101,17 +105,28 @@ export function useMobileNativeChatDrafts(args: {
[draftKey, pendingKey]
)
// Why: over relay the send RPC can take seconds (or lose only its ack), and a
// composer that waits for settlement to empty reads as "my prompt didn't
// send". Clear at send time; a definite rejection restores the text below.
const clearDraftForSend = useCallback((origin: MobileNativeChatSendOrigin, text: string) => {
setDrafts((previous) =>
(previous[origin.draftKey] ?? '').trim() === text.trim()
? { ...previous, [origin.draftKey]: '' }
: previous
)
}, [])
const restoreRejectedDraft = useCallback((origin: MobileNativeChatSendOrigin, text: string) => {
// Why: never clobber text the user typed while the rejection was in flight.
setDrafts((previous) =>
(previous[origin.draftKey] ?? '') === '' ? { ...previous, [origin.draftKey]: text } : previous
)
}, [])
const acceptSend = useCallback(
(origin: MobileNativeChatSendOrigin, text: string, images?: string[]) => {
// Why: an RPC may settle after a tab switch; mutate only the tab that
// originated the send, without erasing edits typed after it began.
setDrafts((previous) =>
(previous[origin.draftKey] ?? '').trim() === text.trim()
? { ...previous, [origin.draftKey]: '' }
: previous
)
// Why: the first prompt can be sent before the provider reports a session
// id; clear its draft, but wait for an id before keying an optimistic echo.
// id; wait for an id before keying an optimistic echo.
if (!origin.pendingKey) {
return
}
@ -151,8 +166,9 @@ export function useMobileNativeChatDrafts(args: {
// Why: a relay drop mid-send loses only the ack in the common case — the
// desktop already delivered the message. Hold the send instead of claiming
// failure (which baits a duplicate): clear the draft when the transcript echo
// failure (which baits a duplicate): stay quiet when the transcript echo
// lands, and surface the uncertainty if the deadline passes without one.
// The composer was already cleared at send time, so this never touches drafts.
const unconfirmedRef = useRef<UnconfirmedSend[]>([])
const holdUnconfirmedSend = useCallback(
(origin: MobileNativeChatSendOrigin, text: string, onUnconfirmed: () => void) => {
@ -175,11 +191,6 @@ export function useMobileNativeChatDrafts(args: {
isActiveTranscript &&
findLandedUnconfirmedSends(messagesRef.current, [entry]).length > 0
) {
setDrafts((previous) =>
(previous[origin.draftKey] ?? '').trim() === text.trim()
? { ...previous, [origin.draftKey]: '' }
: previous
)
return
}
entry.deadline = setTimeout(() => {
@ -210,12 +221,6 @@ export function useMobileNativeChatDrafts(args: {
if (entry.deadline !== null) {
clearTimeout(entry.deadline)
}
// Same guard as acceptSend: never erase edits typed after the send began.
setDrafts((previous) =>
(previous[entry.draftKey] ?? '').trim() === entry.text.trim()
? { ...previous, [entry.draftKey]: '' }
: previous
)
}
}, [messages, draftKey, pendingKey])
@ -278,6 +283,8 @@ export function useMobileNativeChatDrafts(args: {
setComposerText,
pending,
captureSendOrigin,
clearDraftForSend,
restoreRejectedDraft,
acceptSend,
holdUnconfirmedSend
}

View File

@ -0,0 +1,141 @@
import { useCallback, type MutableRefObject } from 'react'
import type { RpcClient } from '../transport/rpc-client'
import {
sendMobileNativeChatMessageWithOutcome,
type MobileNativeChatSendOutcome
} from './mobile-native-chat-send'
import { healMobileNativeChatStaleInput } from './mobile-native-chat-stale-input'
import type { MobileNativeChatSendOrigin } from './use-mobile-native-chat-drafts'
export type MobileNativeChatMessageSend = {
/** Composer send that syncs the draft (clear on send, restore on rejection). */
send: (text: string, images?: string[]) => Promise<boolean>
/** Outcome-preserving variant: callers that pasted terminal input beforehand
* (image sends) must see 'unknown' to heal a possibly-orphaned paste. */
sendWithOutcome: (text: string, images?: string[]) => Promise<MobileNativeChatSendOutcome>
/** Answer to an agent question — never touches the composer draft. */
answerQuestion: (text: string) => Promise<boolean>
}
/** The native-chat send seam: one write path shared by composer sends, image
* sends, and question answers, wired to the drafts accounting. */
export function useMobileNativeChatMessageSend(args: {
client: RpcClient | null
enabled: boolean
handleRef: MutableRefObject<string | null>
deviceTokenRef: MutableRefObject<string | null>
captureSendOrigin: (text: string) => MobileNativeChatSendOrigin | null
clearDraftForSend: (origin: MobileNativeChatSendOrigin, text: string) => void
restoreRejectedDraft: (origin: MobileNativeChatSendOrigin, text: string) => void
acceptSend: (origin: MobileNativeChatSendOrigin, text: string, images?: string[]) => void
holdUnconfirmedSend: (
origin: MobileNativeChatSendOrigin,
text: string,
onUnconfirmed: () => void
) => void
onSendError: (message: string) => void
}): MobileNativeChatMessageSend {
const {
client,
enabled,
handleRef,
deviceTokenRef,
captureSendOrigin,
clearDraftForSend,
restoreRejectedDraft,
acceptSend,
holdUnconfirmedSend,
onSendError
} = args
const sendMessage = useCallback(
async (
text: string,
images: string[] | undefined,
syncComposer: boolean
): Promise<MobileNativeChatSendOutcome> => {
const handle = handleRef.current
const origin = captureSendOrigin(text)
if (!client || !handle || !origin || !enabled) {
onSendError('Message not sent (disconnected)')
return 'rejected'
}
// The agent's input may still hold an orphaned image paste from an earlier
// send (#10228); submitting on top of it would glue the image onto this
// message. Healed before the draft clear so a failed heal — which sends
// nothing — leaves the composer exactly as the user left it.
const healArgs = { client, terminal: handle, deviceToken: deviceTokenRef.current }
if (!(await healMobileNativeChatStaleInput(healArgs))) {
onSendError('Message not sent')
return 'rejected'
}
// Why: empty the composer at send time, not on the ack — over relay the
// round trip is visible, and a lost ack must not strand the sent prompt
// in the box. Only a definite rejection puts the text back.
if (syncComposer) {
clearDraftForSend(origin, text)
}
const outcome = await sendMobileNativeChatMessageWithOutcome({
client,
terminal: handle,
text,
...(deviceTokenRef.current
? { mobileClient: { id: deviceTokenRef.current, type: 'mobile' } }
: {})
})
if (outcome === 'unknown') {
// Why: an ack-lost send usually WAS delivered (issue seen on cellular
// relay) — verify via the transcript echo instead of a false "not sent".
holdUnconfirmedSend(origin, text, () =>
onSendError('Delivery unconfirmed — check chat before retrying')
)
return 'unknown'
}
if (outcome === 'rejected') {
if (syncComposer) {
restoreRejectedDraft(origin, text)
}
onSendError('Message not sent')
return 'rejected'
}
// `images` are local preview URIs for the optimistic echo only — the actual
// image bytes already rode along as a bracketed paste before this text send.
acceptSend(origin, text, images)
return 'accepted'
},
[
acceptSend,
captureSendOrigin,
clearDraftForSend,
client,
deviceTokenRef,
enabled,
handleRef,
holdUnconfirmedSend,
onSendError,
restoreRejectedDraft
]
)
const sendWithOutcome = useCallback(
(text: string, images?: string[]) => sendMessage(text, images, true),
[sendMessage]
)
// Boolean surface for callers with no pre-pasted input: 'unknown' stays true
// (the send usually landed; the optimistic echo is already held unconfirmed).
const send = useCallback(
async (text: string, images?: string[]): Promise<boolean> =>
(await sendWithOutcome(text, images)) !== 'rejected',
[sendWithOutcome]
)
// A question answer is not composer text, so it never syncs the draft.
const answerQuestion = useCallback(
async (text: string): Promise<boolean> =>
(await sendMessage(text, undefined, false)) !== 'rejected',
[sendMessage]
)
return { send, sendWithOutcome, answerQuestion }
}

View File

@ -5,7 +5,9 @@ const systemConfigPath = '/home/user/.codex/config.toml'
describe('getCodexConfigSyncWarning', () => {
it('stays silent while syncing normally', () => {
expect(getCodexConfigSyncWarning({ state: 'synced', reason: null, systemConfigPath })).toBeNull()
expect(
getCodexConfigSyncWarning({ state: 'synced', reason: null, systemConfigPath })
).toBeNull()
})
it('stays silent before the status has loaded', () => {