diff --git a/mobile/src/session/mobile-structured-send-delivery.test.ts b/mobile/src/session/mobile-structured-send-delivery.test.ts index 6305bf75df57..8431e32a7aa9 100644 --- a/mobile/src/session/mobile-structured-send-delivery.test.ts +++ b/mobile/src/session/mobile-structured-send-delivery.test.ts @@ -169,7 +169,9 @@ describe('mobileStructuredSendDelivery', () => { }) }) - it('never releases an ambiguous id on a later RPC refusal or failure', () => { + it('spends an ambiguous id the host has expired, and says to check the chat', () => { + // The host refuses an expired id on every replay; keeping it would refuse this text forever. + // The earlier attempt may still be in the chat, so the words never say it was not sent. expect( mobileStructuredSendDelivery( { @@ -179,7 +181,25 @@ describe('mobileStructuredSendDelivery', () => { }, true ) - ).toEqual({ outcome: 'rejected', operationIdSpent: false, error: 'Operation expired' }) + ).toEqual({ + outcome: 'rejected', + operationIdSpent: true, + error: + "Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed." + }) + }) + + it('never releases an ambiguous id on any other later RPC refusal or failure', () => { + expect( + mobileStructuredSendDelivery( + { + status: 'refused', + code: 'agent_session_operation_conflict', + message: 'Operation conflict' + }, + true + ) + ).toEqual({ outcome: 'rejected', operationIdSpent: false, error: 'Operation conflict' }) expect( mobileStructuredSendDelivery( { status: 'failed', message: 'Your message was not sent. Send it again.' }, diff --git a/mobile/src/session/mobile-structured-send-delivery.ts b/mobile/src/session/mobile-structured-send-delivery.ts index 466a0d29ff8a..f20dc0971887 100644 --- a/mobile/src/session/mobile-structured-send-delivery.ts +++ b/mobile/src/session/mobile-structured-send-delivery.ts @@ -15,11 +15,11 @@ // message is a new message and must carry a new id. // rejected — a terminal refusal or rejected submission spends a fresh id. A // pending-admission refusal, or any refusal after earlier transport doubt, -// keeps it because neither proves a retained delivery did not happen. The -// one exception is a host that refuses the replay's request shape itself -// (an older host's strict schema turning `delivery` away): that host can -// never accept the replay, so keeping the id would only refuse every later -// send of the same text. +// keeps it because neither proves a retained delivery did not happen. Two +// exceptions spend it anyway, because the host can never accept the replay +// and keeping the id would only refuse every later send of the same text: +// a host that refuses the replay's request shape itself (an older host's +// strict schema turning `delivery` away), and an id the host has expired. // unknown — the one answer that KEEPS its id, whether it came from the host or // from an ack-loss on the way back. The message may be with the provider, so // the retry has to stay a replay. Rotating here is what sent one message to a @@ -28,6 +28,7 @@ import type { AgentJournalSubmission } from '../../../src/shared/agent-session-journal-types' import type { AgentSessionSendResult } from '../../../src/shared/agent-session-wire' import { agentSessionRefusalOperationState } from '../../../src/shared/agent-session-refusal-retry' +import { agentSessionWriteNoticeEnglish } from '../../../src/shared/agent-session-refusal-notice' import { structuredAgentSessionRejectionNotice } from '../../../src/shared/structured-agent-session-send-disposition' import type { MobileNativeChatSendOutcome } from './mobile-native-chat-send' import type { StructuredAgentSessionMutationCallResult } from './mobile-structured-agent-session-rpc' @@ -52,6 +53,15 @@ export function mobileStructuredSendDelivery( if (refusalState === 'unknown') { return { outcome: 'unknown', operationIdSpent: false, error: null } } + if (retained && result.code === 'agent_session_operation_expired') { + // The host refuses this id for good once its day is up, so keeping it would refuse this text + // forever. The earlier attempt may already be in the chat, so the words say to check first. + return { + outcome: 'rejected', + operationIdSpent: true, + error: agentSessionWriteNoticeEnglish(['sendOutcomeLost']) + } + } return { outcome: 'rejected', operationIdSpent: refusalState === 'settled-rejected' && !retained, diff --git a/mobile/src/session/use-mobile-native-chat-drafts.test.ts b/mobile/src/session/use-mobile-native-chat-drafts.test.ts index 2547d9191bec..a693069dd8c7 100644 --- a/mobile/src/session/use-mobile-native-chat-drafts.test.ts +++ b/mobile/src/session/use-mobile-native-chat-drafts.test.ts @@ -142,7 +142,7 @@ describe('useMobileNativeChatDrafts', () => { expect(state?.composerText).toBe('ping') }) - it('does not clobber newer edits when restoring a rejected send', async () => { + it('appends a rejected send after edits typed while it was in flight', async () => { await mount('a') act(() => state?.setComposerText('ping')) const origin = state?.captureSendOrigin('ping') @@ -157,10 +157,10 @@ describe('useMobileNativeChatDrafts', () => { state?.restoreRejectedDraft(origin, 'ping') } }) - expect(state?.composerText).toBe('newer edit') + expect(state?.composerText).toBe('newer edit\n\nping') }) - it('preserves an intentional clear after a newer edit while a rejection is pending', async () => { + it('returns a rejected send even after a newer edit was cleared', async () => { await mount('a') act(() => state?.setComposerText('ping')) const origin = state?.captureSendOrigin('ping') @@ -177,7 +177,7 @@ describe('useMobileNativeChatDrafts', () => { } }) - expect(state?.composerText).toBe('') + expect(state?.composerText).toBe('ping') }) it('restores a rejected send onto its originating tab only', async () => { diff --git a/mobile/src/session/use-mobile-native-chat-drafts.ts b/mobile/src/session/use-mobile-native-chat-drafts.ts index f17e1826a679..027e6a9a1d89 100644 --- a/mobile/src/session/use-mobile-native-chat-drafts.ts +++ b/mobile/src/session/use-mobile-native-chat-drafts.ts @@ -8,6 +8,7 @@ import { type SetStateAction } from 'react' import type { NativeChatMessage } from '../../../src/shared/native-chat-types' +import { appendReturnedDraftText } from '../../../src/shared/returned-draft-text' import { countUserTextOccurrences, findLandedImagePreviewEchoes, @@ -77,7 +78,7 @@ export function useMobileNativeChatDrafts(args: { readSeededLaunchDraftSeed: () => MobileNativeChatLaunchDraftSeed | 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. */ + /** Put the text back after a definite rejection, after whatever the composer holds now. */ restoreRejectedDraft: (origin: MobileNativeChatSendOrigin, text: string) => void acceptSend: (origin: MobileNativeChatSendOrigin, text: string, images?: string[]) => void holdUnconfirmedSend: ( @@ -200,13 +201,12 @@ export function useMobileNativeChatDrafts(args: { }, []) const restoreRejectedDraft = useCallback((origin: MobileNativeChatSendOrigin, text: string) => { - // Why: never clobber text the user typed while the rejection was in flight. - setDrafts((previous) => - draftEditGenerationsRef.current.isCurrent(origin.draftKey, origin.draftEditGeneration) && - (previous[origin.draftKey] ?? '') === '' - ? { ...previous, [origin.draftKey]: text } - : previous - ) + // Appended, so typing done while the send was in flight stays and the returned text isn't dropped. + setDrafts((previous) => { + const current = previous[origin.draftKey] ?? '' + const next = appendReturnedDraftText(current, text) + return next === current ? previous : { ...previous, [origin.draftKey]: next } + }) }, []) const acceptSend = useCallback( diff --git a/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx b/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx index 34b66f1dd1fc..086482537ef2 100644 --- a/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx +++ b/mobile/src/session/use-mobile-structured-agent-session-send.test.tsx @@ -390,7 +390,7 @@ describe('mobile structured send retries', () => { expect(calls().every(([, params]) => !('retryUnknown' in (params as object)))).toBe(true) }) - it('keeps an ambiguous id after the host replay window expires', async () => { + it('sends under a new id once the host has expired an ambiguous one', async () => { let attempts = 0 sendRequest.mockImplementation(async (method) => { if (method !== 'agentSession.send') { @@ -400,6 +400,9 @@ describe('mobile structured send retries', () => { if (attempts === 1) { throw markRpcDeliveryUnknown(new Error('Connection closed')) } + if (attempts === 3) { + return sendResult('accepted') + } return ok({ ok: false, refusal: { @@ -413,11 +416,16 @@ describe('mobile structured send retries', () => { await act(async () => { expect(await hook!.sendWithOutcome('old ambiguity')).toBe('unknown') expect(await hook!.sendWithOutcome('old ambiguity')).toBe('rejected') - expect(await hook!.sendWithOutcome('old ambiguity')).toBe('rejected') + expect(await hook!.sendWithOutcome('old ambiguity')).toBe('accepted') }) expect(calls()).toHaveLength(3) - expect(new Set(sentIds()).size).toBe(1) + const [first, replay, fresh] = sentIds() + expect(replay).toBe(first) + expect(fresh).not.toBe(first) + expect(onSendError).toHaveBeenCalledWith( + "Orca couldn't confirm your message reached the agent. Check the chat, then send it again if needed." + ) }) it('does not retain an id when the action budget expires before dispatch', async () => { diff --git a/src/renderer/src/components/native-chat/native-chat-draft-cache.ts b/src/renderer/src/components/native-chat/native-chat-draft-cache.ts index 333ab7a2c50c..7787c97019d8 100644 --- a/src/renderer/src/components/native-chat/native-chat-draft-cache.ts +++ b/src/renderer/src/components/native-chat/native-chat-draft-cache.ts @@ -5,6 +5,7 @@ import type { JSONContent } from '@tiptap/react' // draft would be lost on every TUI/GUI round-trip. Mirrors the attachment cache // so both halves of an unsent message survive toggles and reconnects. +import { appendReturnedDraftText } from '../../../../shared/returned-draft-text' import { setBoundedScopeCacheEntry } from './native-chat-composer-scope-cache' const draftCache = new Map() @@ -28,10 +29,6 @@ export function writeNativeChatDraftCache(scopeKey: string, draft: string): void }) } -export function appendNativeChatDraftText(draft: string, text: string): string { - return draft === '' ? text : `${draft.trimEnd()}\n\n${text}` -} - // Only a write from outside the composer notifies; its own writes already hold the text. const appendListeners = new Map void>>() @@ -42,7 +39,7 @@ export function appendNativeChatDraftCache(scopeKey: string, text: string): void } writeNativeChatDraftCache( scopeKey, - appendNativeChatDraftText(readNativeChatDraftCache(scopeKey), text) + appendReturnedDraftText(readNativeChatDraftCache(scopeKey), text) ) appendListeners.get(scopeKey)?.forEach((listener) => listener(text)) } diff --git a/src/renderer/src/components/native-chat/structured-agent-session-withdrawn-message-restore.test.tsx b/src/renderer/src/components/native-chat/structured-agent-session-withdrawn-message-restore.test.tsx index 150b520bcd32..404b35133b44 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-withdrawn-message-restore.test.tsx +++ b/src/renderer/src/components/native-chat/structured-agent-session-withdrawn-message-restore.test.tsx @@ -252,11 +252,13 @@ describe('a message the host withdrew at a Stop', () => { unsubscribe() expect(storedAtRestore).toEqual([id]) - // The drop never reached storage: the next mount gives the text back again rather than losing it. + // The drop never reached storage: after a crash, which loses the in-memory draft, the next mount + // gives the text back again rather than losing it. cleanup() + writeNativeChatDraftCache(PANE, '') writeOutbox(SESSION, beforeDrop) renderOutbox().rerender({ submissions: [withdrawn(id)] }) - expect(readNativeChatDraftCache(PANE)).toBe('hello\n\nhello') + expect(readNativeChatDraftCache(PANE)).toBe('hello') }) it('keeps a message refused for any other reason on its Retry, and gives nothing back', async () => { diff --git a/src/renderer/src/components/native-chat/use-native-chat-draft.ts b/src/renderer/src/components/native-chat/use-native-chat-draft.ts index 5653c0a77a85..a5b5d99a143d 100644 --- a/src/renderer/src/components/native-chat/use-native-chat-draft.ts +++ b/src/renderer/src/components/native-chat/use-native-chat-draft.ts @@ -1,6 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' +import { appendReturnedDraftText } from '../../../../shared/returned-draft-text' import { - appendNativeChatDraftText, readNativeChatDraftCache, subscribeToNativeChatDraftAppend, writeNativeChatDraftCache @@ -45,7 +45,7 @@ export function useNativeChatDraft( pendingAppendRef.current = { scopeKey, text: - pending?.scopeKey === scopeKey ? appendNativeChatDraftText(pending.text, text) : text + pending?.scopeKey === scopeKey ? appendReturnedDraftText(pending.text, text) : text } }), [isComposing, scopeKey] @@ -61,7 +61,7 @@ export function useNativeChatDraft( writeNativeChatDraftCache( scopeKey, pending?.scopeKey === scopeKey - ? appendNativeChatDraftText(resolved, pending.text) + ? appendReturnedDraftText(resolved, pending.text) : resolved ) return resolved @@ -74,7 +74,7 @@ export function useNativeChatDraft( const pending = pendingAppendRef.current pendingAppendRef.current = null if (pending?.scopeKey === scopeKey) { - setDraft((previous) => appendNativeChatDraftText(previous, pending.text)) + setDraft((previous) => appendReturnedDraftText(previous, pending.text)) } }, [scopeKey, setDraft]) diff --git a/src/shared/returned-draft-text.test.ts b/src/shared/returned-draft-text.test.ts new file mode 100644 index 000000000000..79f27507cde0 --- /dev/null +++ b/src/shared/returned-draft-text.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from 'vitest' +import { appendReturnedDraftText } from './returned-draft-text' + +describe('appendReturnedDraftText', () => { + it('fills an empty composer with the returned text as it was typed', () => { + expect(appendReturnedDraftText('', 'ping ')).toBe('ping ') + expect(appendReturnedDraftText(' \n', 'ping')).toBe('ping') + }) + + it('puts the returned text after a draft, a blank line apart', () => { + expect(appendReturnedDraftText('newer edit\n', 'ping')).toBe('newer edit\n\nping') + }) + + it('adds nothing when the draft already ends with that text', () => { + expect(appendReturnedDraftText('ping', 'ping')).toBe('ping') + expect(appendReturnedDraftText('first\n\nping\n', 'ping ')).toBe('first\n\nping\n') + expect(appendReturnedDraftText('ping', '')).toBe('ping') + }) + + it('still appends text that only ends a line of the draft', () => { + expect(appendReturnedDraftText('say ping', 'ping')).toBe('say ping\n\nping') + }) +}) diff --git a/src/shared/returned-draft-text.ts b/src/shared/returned-draft-text.ts new file mode 100644 index 000000000000..4dc67520d7fa --- /dev/null +++ b/src/shared/returned-draft-text.ts @@ -0,0 +1,19 @@ +/** + * Puts text a chat handed back after what the composer already holds, a blank line apart. It never + * replaces typed text (a whitespace-only draft counts as empty), and adds nothing when the draft + * already ends with that text, so handing the same text back twice leaves one copy. + */ +export function appendReturnedDraftText(draft: string, text: string): string { + const returned = text.trimEnd() + if (returned.trim() === '') { + return draft + } + const kept = draft.trimEnd() + if (kept === '') { + return text + } + if (kept === returned || kept.endsWith(`\n\n${returned}`)) { + return draft + } + return `${kept}\n\n${text}` +}