diff --git a/apps/mobile/src/app/(app)/agent-chat/new.tsx b/apps/mobile/src/app/(app)/agent-chat/new.tsx index 5d967f5d7a..72b85ea938 100644 --- a/apps/mobile/src/app/(app)/agent-chat/new.tsx +++ b/apps/mobile/src/app/(app)/agent-chat/new.tsx @@ -106,7 +106,7 @@ function NewSessionScreenBody() { // settles. const initialPrompt = resolvePrefillOverDraft( sharePrefillText, - draftState.settled ? draftState.text : null + draftState.settled ? draftState.value : null ); // Save the new-session draft debounced on every text change, and flush the diff --git a/apps/mobile/src/components/agents/attachment-preview-strip.tsx b/apps/mobile/src/components/agents/attachment-preview-strip.tsx index 762effa8cf..4c1d0ea2aa 100644 --- a/apps/mobile/src/components/agents/attachment-preview-strip.tsx +++ b/apps/mobile/src/components/agents/attachment-preview-strip.tsx @@ -147,6 +147,9 @@ function AttachmentChip({ className="h-full w-full" contentFit="cover" transition={0} + allowDownscaling + recyclingKey={attachment.id} + cachePolicy="memory" /> ) : ( { return refSlots.slots[index] as React.RefObject; }), useState: vi.fn((initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void]), + useImperativeHandle: vi.fn(), }; }); diff --git a/apps/mobile/src/components/agents/chat-composer.test.ts b/apps/mobile/src/components/agents/chat-composer.test.ts index 6395deff09..7932bdca8c 100644 --- a/apps/mobile/src/components/agents/chat-composer.test.ts +++ b/apps/mobile/src/components/agents/chat-composer.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the mocked hook surface and draft-restore contract require a long suite */ /* eslint-disable typescript-eslint/no-deprecated -- react-test-renderer is the DOM-free renderer used to mount React/RN trees under vitest (node env, no jsdom); see src/lib/persist/cache-persistence-mount.test.ts */ /* eslint-disable new-cap -- ChatComposer is called as a plain function, matching repo test convention */ /* eslint-disable require-await, @typescript-eslint/require-await -- the fake hooks and handlers settle without await because they resolve immediately */ @@ -31,6 +32,7 @@ vi.mock('react', async () => { useEffect: vi.fn((fn: React.EffectCallback) => { fn(); }), + useImperativeHandle: vi.fn(() => undefined), useMemo: vi.fn((factory: () => T) => factory()), useRef: vi.fn((initial: T) => { const index = refSlots.cursor; diff --git a/apps/mobile/src/components/agents/chat-composer.tsx b/apps/mobile/src/components/agents/chat-composer.tsx index e74698ec9b..ba8796ddf4 100644 --- a/apps/mobile/src/components/agents/chat-composer.tsx +++ b/apps/mobile/src/components/agents/chat-composer.tsx @@ -7,7 +7,15 @@ import * as Haptics from 'expo-haptics'; import { useActionSheet } from '@expo/react-native-action-sheet'; import { type SlashCommandInfo } from '@kilocode/cloud-agent-sdk'; import { type RemoteCommandState } from '@kilocode/cloud-agent-sdk/remote-command-catalog'; -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { + type Ref, + useCallback, + useEffect, + useImperativeHandle, + useMemo, + useRef, + useState, +} from 'react'; import { AppState, type GestureResponderEvent, @@ -54,7 +62,10 @@ import { import { ChatComposerInputRow } from '@/components/agents/chat-composer-input-row'; import { BlurBar } from '@/components/ui/blur-bar'; import { VoiceInputStatus } from '@/components/voice-input-control'; -import { AGENT_ATTACHMENT_MAX_FILES } from '@/lib/agent-attachments/constants'; +import { + AGENT_ATTACHMENT_MAX_BYTES, + AGENT_ATTACHMENT_MAX_FILES, +} from '@/lib/agent-attachments/constants'; import { type AgentAttachmentSubmissionPayload, type AgentAttachmentWire, @@ -71,6 +82,7 @@ import { type ModeOption } from '@/components/agents/mode-normalize'; import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { resolveMessageInputAppStateTransition } from '@/lib/message-input-app-state'; +import { createFrameCoalescer, type FrameCoalescer } from '@/lib/coalesce-frame'; import { clearDraft as clearStoredDraft, saveDraft } from '@/lib/persist/drafts'; import { useDraftFlushOnBackground } from '@/lib/persist/use-draft-flush'; import { cn } from '@/lib/utils'; @@ -104,6 +116,11 @@ type AndroidDismissKeyboardGesture = { failed: boolean; }; +/** Imperative handle the host uses to set composer text (Retry / Copy to composer). */ +export type ChatComposerControl = { + setText: (text: string) => void; +}; + type ChatComposerProps = { onSend: ( text: string, @@ -162,6 +179,8 @@ type ChatComposerProps = { * text the user typed while identity and the draft were still loading. */ initialDraft?: string; + /** Imperative handle the host binds to call `setText`. */ + controlRef?: Ref; }; export function ChatComposer({ @@ -192,6 +211,7 @@ export function ChatComposer({ autoSend, draftKey, initialDraft, + controlRef, }: Readonly) { const colors = useThemeColors(); const { showActionSheetWithOptions } = useActionSheet(); @@ -277,6 +297,31 @@ export function ChatComposer({ const measureRef = useRef(measure); measureRef.current = measure; + // Coalesce the three derived setters (measure node, hasText, slash command) + // to at most one publication per animation frame. Typing can fire many + // onChangeText calls in a single frame; publishing derived state once per + // frame keeps the send button and slash suggestions from re-rendering on + // every keystroke. The publish closure reads `measureRef` at call time and + // uses the stable `setHasText`/`setSlashCommandInput` setters, so it stays + // valid for the lifetime of the component. + const composerFrameCoalescerRef = useRef | null>(null); + composerFrameCoalescerRef.current ??= createFrameCoalescer(value => { + measureRef.current.setText(value); + setHasText(value.trim().length > 0); + setSlashCommandInput(getSlashCommandCandidate(value)); + }); + const composerFrameCoalescer = composerFrameCoalescerRef.current; + + // Flush the coalescer on unmount so a pending derived-state publication is + // committed before teardown and the scheduled frame callback becomes a + // no-op instead of firing setState after the component is gone. + useEffect( + () => () => { + composerFrameCoalescer.flush(); + }, + [composerFrameCoalescer] + ); + // Flush the debounced draft write when the app leaves `active` and on // unmount, so a backgrounded-then-killed app (or a navigation away) does // not lose the last keystrokes inside the 500 ms window. @@ -352,11 +397,51 @@ export function ChatComposer({ const toolbarDisabled = disabled || isSending; const voiceDisabled = toolbarDisabled; - function handleChangeText(value: string) { + // One place text is written into the live input from an external caller + // (slash-command select, Retry / Copy to composer). Sets text, selection, + // hasText, slash-command state, and the measure node, then persists the + // durable draft exactly like a keystroke. + function applyComposerText(value: string) { + // Drain any pending coalesced typing so a stale value cannot overwrite the + // copied prompt on the next frame. The direct setters below then land the + // copied prompt in one commit. + composerFrameCoalescer.flush(); textRef.current = value; measure.setText(value); setHasText(value.trim().length > 0); - setSlashCommandInput(getSlashCommandCandidate(value)); + setSlashCommandInput(null); + inputRef.current?.setNativeProps({ + text: value, + selection: { start: value.length, end: value.length }, + }); + selectionRef.current = { start: value.length, end: value.length }; + inputRef.current?.focus(); + if (draftKey && userId) { + saveDraft(userId, draftKey, value); + } + } + + // Hold the latest applyComposerText so the imperative handle stays stable + // while the composer does not remount when identity resolves. + const applyComposerTextRef = useRef(applyComposerText); + applyComposerTextRef.current = applyComposerText; + + useImperativeHandle( + controlRef, + () => ({ + setText: (text: string) => { + applyComposerTextRef.current(text); + }, + }), + [] + ); + + function handleChangeText(value: string) { + textRef.current = value; + // Derived state (measure node, hasText, slash command) is coalesced to one + // publication per frame; the live submit-time ref and the debounced draft + // write stay synchronous so neither can lag a keystroke. + composerFrameCoalescer.push(value); // Delivery applies text BEFORE onDelivered fires, so any // handleChangeText after shareDelivered is a user edit. Disarm // so a later gate resolution (upload completion) cannot @@ -381,6 +466,10 @@ export function ChatComposer({ onChangeText: handleChangeText, addCandidates, onDelivered: () => { + // Commit the coalesced `hasText` before the delivery check so the + // auto-send effect sees the delivered text in the same commit, not on + // the next frame. + composerFrameCoalescer.flush(); setAutoSendArmed( shouldArmAutoSendOnDelivery({ autoSend: autoSendRef.current, @@ -440,11 +529,10 @@ export function ChatComposer({ }, onFailure: reason => { toast.error( - reason === 'empty' - ? CLIPBOARD_PASTE_EMPTY_MESSAGE - : describeClassificationFailure('unreadable') + reason === 'empty' ? CLIPBOARD_PASTE_EMPTY_MESSAGE : describeClassificationFailure(reason) ); }, + maxBytes: AGENT_ATTACHMENT_MAX_BYTES, }); const commandList = useMemo( @@ -595,6 +683,11 @@ export function ChatComposer({ : undefined; function clearDraft() { + // Drain any pending coalesced value (a final voice transcript can `push` + // after `submit`'s `flush`). Publishing it here clears `hasPending` so the + // already-scheduled frame callback becomes a no-op; the direct setters + // below then override the published value in the same batched commit. + composerFrameCoalescer.flush(); textRef.current = ''; setHasText(false); setSlashCommandInput(null); @@ -609,7 +702,13 @@ export function ChatComposer({ async function handleSend() { const trimmed = textRef.current.trim(); - if (!control.canSend) { + // Decide admission from live values, not render-time `control.canSend`, + // which can lag behind a same-frame edit. An empty prompt with no ready + // attachment is never sent. + const readyAttachmentsCount = upload.attachments.filter( + attachment => attachment.status === 'uploaded' + ).length; + if ((trimmed.length === 0 && readyAttachmentsCount === 0) || disabled || isSending) { return; } if (upload.isUploading) { @@ -689,20 +788,14 @@ export function ChatComposer({ if (sendLockRef.current.isLocked()) { return; } - const value = `/${command.name} `; - textRef.current = value; - measure.setText(value); - setHasText(true); - setSlashCommandInput(null); - inputRef.current?.setNativeProps({ - text: value, - selection: { start: value.length, end: value.length }, - }); - selectionRef.current = { start: value.length, end: value.length }; - inputRef.current?.focus(); + applyComposerText(`/${command.name} `); } async function submit() { + // Commit any coalesced derived state (hasText, measure, slash command) + // before the send decision, so a submit in the same frame as the last + // keystroke never reads stale derived state. + composerFrameCoalescer.flush(); // `settleVoiceInputBeforeSubmit` is the sole admission owner for the // entire voice-settle + asynchronous send sequence. It acquires the // SubmitLock, sets pending state, waits for the final transcript, runs diff --git a/apps/mobile/src/components/agents/child-session-sheet-state.test.ts b/apps/mobile/src/components/agents/child-session-sheet-state.test.ts index a9c1f6b336..42af4a528c 100644 --- a/apps/mobile/src/components/agents/child-session-sheet-state.test.ts +++ b/apps/mobile/src/components/agents/child-session-sheet-state.test.ts @@ -19,7 +19,19 @@ describe('getChildSessionSheetState', () => { }); it('shows an empty state after successful hydration with no messages', () => { - expect(getChildSessionSheetState({ status: 'ready' }, 0)).toBe('empty'); + expect( + getChildSessionSheetState( + { + status: 'ready', + cursor: null, + hasOlder: false, + isLoadingOlder: false, + olderError: null, + omittedItemCount: 0, + }, + 0 + ) + ).toBe('empty'); }); it('shows an error after failed hydration with no messages', () => { diff --git a/apps/mobile/src/components/agents/child-session-sheet.tsx b/apps/mobile/src/components/agents/child-session-sheet.tsx index d6a0ece619..d3f15a699c 100644 --- a/apps/mobile/src/components/agents/child-session-sheet.tsx +++ b/apps/mobile/src/components/agents/child-session-sheet.tsx @@ -1,7 +1,11 @@ import { type ReactNode } from 'react'; import { Modal, View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; -import { type ChildSessionHydrationState, type StoredMessage } from '@kilocode/cloud-agent-sdk'; +import { + type ChildSessionHydrationState, + type OlderMessagesError, + type StoredMessage, +} from '@kilocode/cloud-agent-sdk'; import { EmptyState } from '@/components/empty-state'; import { QueryError } from '@/components/query-error'; @@ -26,6 +30,11 @@ type ChildSessionSheetProps = { getChildMessages: (sessionId: string) => StoredMessage[]; hydrationState: ChildSessionHydrationState; isStreaming: boolean; + hasOlderMessages: boolean; + isLoadingOlderMessages: boolean; + olderMessagesError: OlderMessagesError | null; + olderMessagesOmittedItemCount: number; + onLoadOlderMessages: () => void; renderPart: RenderPartFn; onOpenChildSession: OpenChildSession; onRetry: () => void; @@ -34,9 +43,6 @@ type ChildSessionSheetProps = { onDismiss?: () => void; }; -// eslint-disable-next-line no-empty-function -- child sessions are hydrated one-shot, no pagination -function noopLoadOlder(): void {} - export function ChildSessionSheet({ visible, sessionId, @@ -44,6 +50,11 @@ export function ChildSessionSheet({ getChildMessages, hydrationState, isStreaming, + hasOlderMessages, + isLoadingOlderMessages, + olderMessagesError, + olderMessagesOmittedItemCount, + onLoadOlderMessages, renderPart, onOpenChildSession, onRetry, @@ -66,11 +77,11 @@ export function ChildSessionSheet({ sessionId={sessionId} items={messages} keyExtractor={message => message.info.id} - hasOlderMessages={false} - isLoadingOlderMessages={false} - olderMessagesError={null} - olderMessagesOmittedItemCount={0} - onLoadOlderMessages={noopLoadOlder} + hasOlderMessages={hasOlderMessages} + isLoadingOlderMessages={isLoadingOlderMessages} + olderMessagesError={olderMessagesError} + olderMessagesOmittedItemCount={olderMessagesOmittedItemCount} + onLoadOlderMessages={onLoadOlderMessages} renderItem={({ item }) => ( diff --git a/apps/mobile/src/components/agents/message-bubble-accessibility.test.ts b/apps/mobile/src/components/agents/message-bubble-accessibility.test.ts index 7c106d19a4..14b8117505 100644 --- a/apps/mobile/src/components/agents/message-bubble-accessibility.test.ts +++ b/apps/mobile/src/components/agents/message-bubble-accessibility.test.ts @@ -53,6 +53,7 @@ vi.mock('./part-renderer', () => ({ vi.mock('./part-types', () => ({ isFilePart: () => false, isTextPart: () => false, + firstHumanText: () => '', })); vi.mock('./use-message-copy', () => ({ useMessageCopy: () => ({ copyMessage: vi.fn() }), @@ -153,8 +154,9 @@ describe('MessageBubble long-press details', () => { }); const { MessageBubble } = await import('./message-bubble'); const message = userMessage('m-long'); + // MessageBubble is wrapped in React.memo; invoke its inner component. // eslint-disable-next-line new-cap - const tree = MessageBubble({ message, onLongPressDetails }); + const tree = MessageBubble.type({ message, onLongPressDetails }); const props = pressableProps(tree); expect(props).not.toBeNull(); const handler = props === null ? undefined : props.onLongPress; diff --git a/apps/mobile/src/components/agents/message-bubble-test-utils.ts b/apps/mobile/src/components/agents/message-bubble-test-utils.ts index a00cc1b668..dbae8ccc0f 100644 --- a/apps/mobile/src/components/agents/message-bubble-test-utils.ts +++ b/apps/mobile/src/components/agents/message-bubble-test-utils.ts @@ -49,8 +49,10 @@ export async function renderBubble( holdQueuedSlot?: boolean ): Promise { const { MessageBubble } = await import('./message-bubble'); + // MessageBubble is wrapped in React.memo; invoke its inner component directly + // to inspect the unrendered element tree. // eslint-disable-next-line new-cap - return MessageBubble({ message, deliveryState, holdQueuedSlot }); + return MessageBubble.type({ message, deliveryState, holdQueuedSlot }); } export function findText(node: unknown, predicate: (text: string) => boolean): boolean { diff --git a/apps/mobile/src/components/agents/message-bubble.test.ts b/apps/mobile/src/components/agents/message-bubble.test.ts index b3912dce49..ccf70fcd89 100644 --- a/apps/mobile/src/components/agents/message-bubble.test.ts +++ b/apps/mobile/src/components/agents/message-bubble.test.ts @@ -1,6 +1,9 @@ /* eslint-disable max-lines -- Queued-badge, delivery, and a11y seams share the direct-invocation MessageBubble harness. */ import { describe, expect, it, vi } from 'vitest'; +import { type MessageDeliveryState, type StoredMessage } from '@kilocode/cloud-agent-sdk'; + +import type * as PartTypes from './part-types'; import { assistantMessage, findElementByType, @@ -33,6 +36,9 @@ vi.mock('@/components/ui/bubble', () => ({ vi.mock('@/components/ui/text', () => ({ Text: ({ children }: { children?: unknown }) => children, })); +vi.mock('@/components/ui/button', () => ({ + Button: 'Button', +})); vi.mock('./chat-markdown-text', () => ({ ChatMarkdownText: () => null, })); @@ -45,10 +51,14 @@ vi.mock('./file-part-renderer', () => ({ vi.mock('./part-renderer', () => ({ PartRenderer: () => null, })); -vi.mock('./part-types', () => ({ - isFilePart: vi.fn(() => false), - isTextPart: vi.fn(() => false), -})); +vi.mock('./part-types', async () => { + const actual = await vi.importActual('./part-types'); + return { + ...actual, + isFilePart: vi.fn(() => false), + isTextPart: vi.fn(() => false), + }; +}); vi.mock('./use-message-copy', () => ({ useMessageCopy: () => ({ copyMessage: vi.fn() }), })); @@ -151,6 +161,214 @@ describe('MessageBubble failed delivery state', () => { }); }); +async function renderBubbleWithHandlers( + message: StoredMessage, + props: { + deliveryState?: MessageDeliveryState; + onRetryMessage?: (m: StoredMessage) => void; + onCopyToComposer?: (text: string) => void; + } +): Promise { + const { MessageBubble } = await import('./message-bubble'); + // eslint-disable-next-line new-cap + return MessageBubble.type({ message, ...props }); +} + +function assistantMessageWithError(id: string, errorName: string): StoredMessage { + const message = assistantMessage(id); + (message.info as { error?: { name: string; data: unknown } }).error = { + name: errorName, + data: { message: 'raw' }, + }; + return message; +} + +describe('MessageBubble failure footer', () => { + it('renders the failed-delivery footer with Retry and Copy to composer', async () => { + const tree = await renderBubbleWithHandlers(userMessage('m-fail'), { + deliveryState: { status: 'failed', error: 'nope', reason: 'exhausted' }, + onRetryMessage: vi.fn<(message: StoredMessage) => void>(), + onCopyToComposer: vi.fn<(text: string) => void>(), + }); + expect(findText(tree, t => t === 'Failed to deliver')).toBe(true); + expect( + findText(tree, t => t === 'We could not deliver this message after several attempts.') + ).toBe(true); + const retry = findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Retry'); + expect(retry).not.toBeNull(); + expect(retry?.props.accessibilityRole).toBe('button'); + const copy = findElementByType( + tree, + 'Button', + p => p.accessibilityLabel === 'Copy to composer' + ); + expect(copy).not.toBeNull(); + expect(copy?.props.accessibilityRole).toBe('button'); + }); + + it('renders the assistant failure footer with Retry and no Copy to composer', async () => { + const tree = await renderBubbleWithHandlers(assistantMessageWithError('m-asst', 'APIError'), { + onRetryMessage: vi.fn<(message: StoredMessage) => void>(), + }); + expect(findText(tree, t => t === 'Response failed')).toBe(true); + const retry = findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Retry'); + expect(retry).not.toBeNull(); + expect( + findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Copy to composer') + ).toBeNull(); + }); + + it('omits the Retry button for a non-retryable assistant error', async () => { + const tree = await renderBubbleWithHandlers( + assistantMessageWithError('m-asst-nr', 'ProviderAuthError'), + { onRetryMessage: vi.fn<(message: StoredMessage) => void>() } + ); + expect(findText(tree, t => t === 'Response failed')).toBe(true); + expect(findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Retry')).toBeNull(); + }); + + it('does not render the footer when no handler is supplied', async () => { + const tree = await renderBubbleWithHandlers(userMessage('m-nohandler'), { + deliveryState: { status: 'failed', error: 'nope', reason: 'exhausted' }, + }); + expect(findText(tree, t => t === 'Failed to deliver')).toBe(false); + }); + + it('names the failure row on the title text without grouping the CTA buttons', async () => { + const tree = await renderBubbleWithHandlers(userMessage('m-a11y'), { + deliveryState: { status: 'failed', error: 'nope', reason: 'interrupted' }, + onRetryMessage: vi.fn<(message: StoredMessage) => void>(), + onCopyToComposer: vi.fn<(text: string) => void>(), + }); + + // The footer container is a plain View: no accessible, no role, no label, + // so Retry and Copy stay individually focusable. + const footer = findElementByType(tree, 'View', p => p.className === 'gap-1 px-4 py-1'); + expect(footer).not.toBeNull(); + expect(footer?.props.accessible).toBeUndefined(); + expect(footer?.props.accessibilityRole).toBeUndefined(); + expect(footer?.props.accessibilityLabel).toBeUndefined(); + + // The row name lives on the title Text, which still announces the row. + const title = findElementByLabel(tree, 'Failed to deliver. Retry available.'); + expect(title).not.toBeNull(); + expect(title?.props.children).toBe('Failed to deliver'); + }); + + it('presses Retry to retry the failed message and Copy to composer to restore the user text', async () => { + const { isTextPart } = await import('./part-types'); + vi.mocked(isTextPart).mockReturnValue(true); + + const message = userMessage('m-press'); + const onRetryMessage = vi.fn<(message: StoredMessage) => void>(); + const onCopyToComposer = vi.fn<(text: string) => void>(); + const tree = await renderBubbleWithHandlers(message, { + deliveryState: { status: 'failed', error: 'nope', reason: 'exhausted' }, + onRetryMessage, + onCopyToComposer, + }); + + const retry = findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Retry'); + expect(retry).not.toBeNull(); + if (!retry) { + throw new Error('expected Retry button'); + } + (retry.props.onPress as () => void)(); + expect(onRetryMessage).toHaveBeenCalledWith(message); + + const copy = findElementByType( + tree, + 'Button', + p => p.accessibilityLabel === 'Copy to composer' + ); + expect(copy).not.toBeNull(); + if (!copy) { + throw new Error('expected Copy to composer button'); + } + (copy.props.onPress as () => void)(); + expect(onCopyToComposer).toHaveBeenCalledWith('hi'); + }); + + it('presses Retry on an assistant row to retry the failed message', async () => { + const message = assistantMessageWithError('m-asst-press', 'APIError'); + const onRetryMessage = vi.fn<(message: StoredMessage) => void>(); + const tree = await renderBubbleWithHandlers(message, { onRetryMessage }); + + const retry = findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Retry'); + expect(retry).not.toBeNull(); + if (!retry) { + throw new Error('expected Retry button'); + } + (retry.props.onPress as () => void)(); + expect(onRetryMessage).toHaveBeenCalledWith(message); + }); +}); + +describe('MessageBubble copy-to-composer human text', () => { + it('passes only the first human text part to Copy, not the synthesized notice', async () => { + const message = userMessage('m-copy-human'); + message.parts = [ + { + id: 'm-copy-human-prompt', + sessionID: 'ses_1', + messageID: 'm-copy-human', + type: 'text', + text: 'prompt', + }, + { + id: 'm-copy-human-notice', + sessionID: 'ses_1', + messageID: 'm-copy-human', + type: 'text', + text: 'binary attachment saved: … path=…', + synthetic: true, + }, + ] as typeof message.parts; + + const onCopyToComposer = vi.fn<(text: string) => void>(); + const tree = await renderBubbleWithHandlers(message, { + deliveryState: { status: 'failed', error: 'nope', reason: 'exhausted' }, + onCopyToComposer, + }); + + const copy = findElementByType( + tree, + 'Button', + p => p.accessibilityLabel === 'Copy to composer' + ); + expect(copy).not.toBeNull(); + if (!copy) { + throw new Error('expected Copy to composer button'); + } + (copy.props.onPress as () => void)(); + expect(onCopyToComposer).toHaveBeenCalledWith('prompt'); + }); + + it('hides Copy for a file-only failed row', async () => { + const message = userMessage('m-copy-file'); + message.parts = [ + { + id: 'm-copy-file-file', + sessionID: 'ses_1', + messageID: 'm-copy-file', + type: 'file', + mime: 'text/plain', + url: 'x', + }, + ] as typeof message.parts; + + const onCopyToComposer = vi.fn<(text: string) => void>(); + const tree = await renderBubbleWithHandlers(message, { + deliveryState: { status: 'failed', error: 'nope', reason: 'exhausted' }, + onCopyToComposer, + }); + + expect( + findElementByType(tree, 'Button', p => p.accessibilityLabel === 'Copy to composer') + ).toBeNull(); + }); +}); + describe('MessageBubble regressions', () => { it('holds badge slot when queued and holdQueuedSlot is set after dequeue', async () => { const message = userMessage('m7'); @@ -274,6 +492,31 @@ function findElementByTypeFn( return null; } +function findElementByLabel( + node: unknown, + label: string +): { type: unknown; props: Record } | null { + if (node == null || typeof node !== 'object') { + return null; + } + const element = node as { type?: unknown; props?: Record }; + if (element.props?.accessibilityLabel === label) { + return element as { type: unknown; props: Record }; + } + const children = element.props?.children; + if (Array.isArray(children)) { + for (const child of children) { + const hit = findElementByLabel(child, label); + if (hit) { + return hit; + } + } + } else if (children && typeof children === 'object') { + return findElementByLabel(children, label); + } + return null; +} + function findProvider( node: unknown, providerType: unknown diff --git a/apps/mobile/src/components/agents/message-bubble.tsx b/apps/mobile/src/components/agents/message-bubble.tsx index cdef515d8b..e5b915b53a 100644 --- a/apps/mobile/src/components/agents/message-bubble.tsx +++ b/apps/mobile/src/components/agents/message-bubble.tsx @@ -1,8 +1,10 @@ +import { memo } from 'react'; import { type MessageDeliveryState, type StoredMessage } from '@kilocode/cloud-agent-sdk'; import { Clock } from '@/components/ui/icons'; import { type AccessibilityActionEvent, Pressable, View } from 'react-native'; import { Bubble } from '@/components/ui/bubble'; +import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; @@ -11,8 +13,9 @@ import { ChatMarkdownText } from './chat-markdown-text'; import { CompactionSeparator } from './compaction-separator'; import { FilePartRenderer } from './file-part-renderer'; import { buildAgentMessageBubbleAccessibilityProps } from './message-bubble-a11y'; +import { selectMessageFailure } from './message-failure-state'; import { PartRenderer } from './part-renderer'; -import { isFilePart, isTextPart } from './part-types'; +import { firstHumanText, isFilePart, isTextPart } from './part-types'; import { useMessageCopy } from './use-message-copy'; import { type OpenChildSession } from './child-session-section'; @@ -33,9 +36,13 @@ type MessageBubbleProps = { * deliveryState !== 'queued'; the hidden slot retains the same height. */ holdQueuedSlot?: boolean; + /** Retries a failed message. The failure footer renders only when supplied. */ + onRetryMessage?: (message: StoredMessage) => void; + /** Copies a failed user message's text back into the composer. */ + onCopyToComposer?: (text: string) => void; }; -export function MessageBubble({ +function MessageBubbleImpl({ message, isLastAssistantMessage, isSessionStreaming, @@ -45,6 +52,8 @@ export function MessageBubble({ deliveryState, onLongPressDetails, holdQueuedSlot, + onRetryMessage, + onCopyToComposer, }: Readonly) { const isUser = message.info.role === 'user'; const { copyMessage } = useMessageCopy(); @@ -78,53 +87,151 @@ export function MessageBubble({ ); } + // Failed-row footer. Renders only when the relevant handler is wired + // (mobile-w2b wires onRetryMessage/onCopyToComposer); a delivery row needs + // Retry or Copy, an assistant row needs Retry only. + const failure = selectMessageFailure({ deliveryState, info: message.info }); + const relevantHandlerWired = + failure?.kind === 'delivery' + ? onRetryMessage !== undefined || onCopyToComposer !== undefined + : onRetryMessage !== undefined; + const userTextContent = isUser + ? message.parts + .filter(isTextPart) + .map(p => p.text) + .join('\n\n') + : ''; + // Copy-to-composer re-sends only the first human-authored text part, so a + // synthesized attachment notice is not copied and a file-only row hides the + // button entirely. + const copyText = isUser ? firstHumanText(message.parts) : ''; + const failureFooter = + failure !== null && relevantHandlerWired ? ( + + + {failure.title} + + {failure.detail} + + {failure.canRetry && onRetryMessage ? ( + + ) : null} + {failure.canCopy && onCopyToComposer && copyText !== '' ? ( + + ) : null} + + + ) : null; + if (isUser) { // Composer, queued-message synthesis, and slash commands emit exactly one // human-authored text part, so the separator separates it from synthesized // attachment notices. - const textContent = message.parts - .filter(isTextPart) - .map(p => p.text) - .join('\n\n'); const fileParts = message.parts.filter(isFilePart); const isQueued = deliveryState?.status === 'queued'; const hasBadgeSlot = isQueued || holdQueuedSlot; const a11y = buildAgentMessageBubbleAccessibilityProps({ isUser: true, canCopy: true }); return ( - - - - - {textContent ? ( - - ) : null} - {fileParts.map(part => ( - - ))} - - - {hasBadgeSlot ? ( - - - - Queued + <> + + + + + {userTextContent ? ( + + ) : null} + {fileParts.map(part => ( + + ))} + + + {hasBadgeSlot ? ( + + + + Queued + - + ) : null} + + {a11y.accessibilityActions.length > 0 ? ( + ) : null} - + + {failureFooter} + + ); + } + + // Assistant messages: render parts sequentially without a bubble. + // Row-rhythm contract: py-1 on each of two adjacent wrappers sums to the + // same value as the gap-2 between parts of one message and the user + // wrapper's py-1 — every adjacent transcript row pair sits one gap apart. + const isStreaming = isLastAssistantMessage && isSessionStreaming; + const a11y = buildAgentMessageBubbleAccessibilityProps({ isUser: false, canCopy: true }); + + return ( + <> + + + + {message.parts.map(part => ( + + ))} + + {a11y.accessibilityActions.length > 0 ? ( ) : null} - ); - } - - // Assistant messages: render parts sequentially without a bubble. - // Row-rhythm contract: py-1 on each of two adjacent wrappers sums to the - // same value as the gap-2 between parts of one message and the user - // wrapper's py-1 — every adjacent transcript row pair sits one gap apart. - const isStreaming = isLastAssistantMessage && isSessionStreaming; - const a11y = buildAgentMessageBubbleAccessibilityProps({ isUser: false, canCopy: true }); - - return ( - - - - {message.parts.map(part => ( - - ))} - - - {a11y.accessibilityActions.length > 0 ? ( - - ) : null} - + {failureFooter} + ); } + +export const MessageBubble = memo(MessageBubbleImpl); diff --git a/apps/mobile/src/components/agents/message-failure-state.test.ts b/apps/mobile/src/components/agents/message-failure-state.test.ts new file mode 100644 index 0000000000..0777ca3f2e --- /dev/null +++ b/apps/mobile/src/components/agents/message-failure-state.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it } from 'vitest'; + +import { type MessageInfo } from '@kilocode/cloud-agent-sdk'; + +import { NON_RETRYABLE_ASSISTANT_ERRORS, selectMessageFailure } from './message-failure-state'; + +function userInfo(): MessageInfo { + return { + id: 'u1', + sessionID: 'ses_1', + role: 'user', + time: { created: 1_761_000_000_000 }, + agent: 'build', + model: { providerID: 'openrouter', modelID: 'anthropic/claude-sonnet-4' }, + }; +} + +type AssistantError = NonNullable['error']>; + +function assistantInfo(errorName: string): MessageInfo { + return { + id: 'a1', + sessionID: 'ses_1', + role: 'assistant', + time: { created: 1_761_000_000_000 }, + // Deliberately carries raw provider text; the helper must never surface it. + error: { name: errorName, data: { message: 'RAW_PROVIDER_TEXT' } } as unknown as AssistantError, + parentID: 'u1', + modelID: 'anthropic/claude-sonnet-4', + providerID: 'kilo', + mode: 'code', + agent: 'build', + path: { cwd: '/', root: '/' }, + cost: 0, + tokens: { total: 0, input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, + }; +} + +function assistantInfoWithoutError(): MessageInfo { + const info = assistantInfo('ProviderAuthError') as Extract; + delete info.error; + return info; +} + +describe('selectMessageFailure', () => { + it('returns null when there is no failed delivery and no assistant error', () => { + expect(selectMessageFailure({ info: userInfo() })).toBeNull(); + expect(selectMessageFailure({ info: assistantInfo('UnknownError') })).not.toBeNull(); + }); + + it('returns null for a queued delivery state', () => { + expect( + selectMessageFailure({ info: userInfo(), deliveryState: { status: 'queued' } }) + ).toBeNull(); + }); + + describe('delivery', () => { + it('maps every reason to fixed copy and never emits raw error text', () => { + const cases = [ + { reason: 'interrupted', detail: 'You stopped this message.' }, + { + reason: 'exhausted', + detail: 'We could not deliver this message after several attempts.', + }, + { reason: 'execution', detail: 'The agent could not run this message.' }, + ] as const; + + for (const { reason, detail } of cases) { + const result = selectMessageFailure({ + info: userInfo(), + deliveryState: { status: 'failed', error: 'RAW_TRANSPORT_TEXT', reason }, + }); + expect(result).not.toBeNull(); + expect(result?.kind).toBe('delivery'); + expect(result?.title).toBe('Failed to deliver'); + expect(result?.detail).toBe(detail); + expect(result?.detail).not.toContain('RAW_TRANSPORT_TEXT'); + expect(result?.canRetry).toBe(true); + expect(result?.canCopy).toBe(true); + } + }); + }); + + describe('assistant', () => { + it('returns null for an assistant info with no error', () => { + expect(selectMessageFailure({ info: assistantInfoWithoutError() })).toBeNull(); + }); + + it('derives fixed copy from a known error name and never emits provider text', () => { + const result = selectMessageFailure({ info: assistantInfo('ProviderAuthError') }); + expect(result).not.toBeNull(); + expect(result?.kind).toBe('assistant'); + expect(result?.title).toBe('Response failed'); + expect(result?.detail).toBe('The provider rejected the request.'); + expect(result?.detail).not.toContain('RAW_PROVIDER_TEXT'); + expect(result?.canCopy).toBe(false); + }); + + it('falls back to the generic line for an unknown error name', () => { + const result = selectMessageFailure({ info: assistantInfo('UnknownError') }); + expect(result?.detail).toBe('The response failed.'); + expect(result?.detail).not.toContain('RAW_PROVIDER_TEXT'); + }); + + it('sets canRetry false only for NON_RETRYABLE_ASSISTANT_ERRORS', () => { + for (const name of NON_RETRYABLE_ASSISTANT_ERRORS) { + const result = selectMessageFailure({ info: assistantInfo(name) }); + expect(result?.canRetry).toBe(false); + } + }); + + it('sets canRetry true for an assistant error outside the non-retryable set', () => { + const result = selectMessageFailure({ info: assistantInfo('APIError') }); + expect(result?.canRetry).toBe(true); + }); + + it('never sets canCopy true for an assistant row', () => { + const result = selectMessageFailure({ info: assistantInfo('APIError') }); + expect(result?.canCopy).toBe(false); + }); + }); +}); diff --git a/apps/mobile/src/components/agents/message-failure-state.ts b/apps/mobile/src/components/agents/message-failure-state.ts new file mode 100644 index 0000000000..09d7c7452c --- /dev/null +++ b/apps/mobile/src/components/agents/message-failure-state.ts @@ -0,0 +1,82 @@ +import { type MessageDeliveryState, type MessageInfo } from '@kilocode/cloud-agent-sdk'; + +/** + * Fixed, safe copy for a failed user-message delivery, keyed by the delivery + * `reason`. Never surfaces raw provider or transport text. + */ +type DeliveryReason = Extract['reason']; + +const DELIVERY_DETAIL_BY_REASON = { + interrupted: 'You stopped this message.', + exhausted: 'We could not deliver this message after several attempts.', + execution: 'The agent could not run this message.', +} as const satisfies Record; + +/** + * Assistant error names that can never be retried. Pinned to the exact names + * in `packages/app-shared/src/opencode.gen.ts`. + */ +export const NON_RETRYABLE_ASSISTANT_ERRORS: readonly string[] = [ + 'ProviderAuthError', + 'MessageAbortedError', + 'ContextOverflowError', +]; + +/** + * Fixed, safe copy for a known assistant error name. Unknown names fall back + * to the generic line. Never surfaces `error.data` or provider message text. + */ +function assistantDetail(errorName: string): string { + switch (errorName) { + case 'ProviderAuthError': { + return 'The provider rejected the request.'; + } + case 'MessageAbortedError': { + return 'The response was stopped.'; + } + case 'ContextOverflowError': { + return 'The conversation is too long for the model.'; + } + default: { + return 'The response failed.'; + } + } +} + +export type MessageFailure = { + kind: 'delivery' | 'assistant'; + title: string; + detail: string; + canRetry: boolean; + canCopy: boolean; +}; + +export function selectMessageFailure(input: { + deliveryState?: MessageDeliveryState; + info: MessageInfo; +}): MessageFailure | null { + const { deliveryState, info } = input; + + if (info.role === 'user' && deliveryState?.status === 'failed') { + return { + kind: 'delivery', + title: 'Failed to deliver', + detail: DELIVERY_DETAIL_BY_REASON[deliveryState.reason], + canRetry: true, + canCopy: true, + }; + } + + if (info.role === 'assistant' && info.error) { + const errorName = info.error.name; + return { + kind: 'assistant', + title: 'Response failed', + detail: assistantDetail(errorName), + canRetry: !NON_RETRYABLE_ASSISTANT_ERRORS.includes(errorName), + canCopy: false, + }; + } + + return null; +} diff --git a/apps/mobile/src/components/agents/new-session-prompt.tsx b/apps/mobile/src/components/agents/new-session-prompt.tsx index e73a2f1976..71585533ba 100644 --- a/apps/mobile/src/components/agents/new-session-prompt.tsx +++ b/apps/mobile/src/components/agents/new-session-prompt.tsx @@ -30,6 +30,7 @@ import { applyVoiceDraftToInput } from '@/lib/voice-input/voice-input-draft'; import { useVoiceInput } from '@/lib/voice-input/use-voice-input'; import { VoiceInputButton, VoiceInputStatus } from '@/components/voice-input-control'; import { describeClassificationFailure } from '@/lib/agent-attachments/validate'; +import { AGENT_ATTACHMENT_MAX_BYTES } from '@/lib/agent-attachments/constants'; import { CLIPBOARD_PASTE_EMPTY_MESSAGE, useClipboardPaste, @@ -181,11 +182,10 @@ export function NewSessionPrompt({ }, onFailure: reason => { toast.error( - reason === 'empty' - ? CLIPBOARD_PASTE_EMPTY_MESSAGE - : describeClassificationFailure('unreadable') + reason === 'empty' ? CLIPBOARD_PASTE_EMPTY_MESSAGE : describeClassificationFailure(reason) ); }, + maxBytes: AGENT_ATTACHMENT_MAX_BYTES, }); function handlePromptInputLayout(event: LayoutChangeEvent) { diff --git a/apps/mobile/src/components/agents/part-types.ts b/apps/mobile/src/components/agents/part-types.ts index 090940db2c..deafd72c9d 100644 --- a/apps/mobile/src/components/agents/part-types.ts +++ b/apps/mobile/src/components/agents/part-types.ts @@ -11,6 +11,16 @@ export function isTextPart(part: Part): part is TextPart { return part.type === 'text'; } +/** + * Returns the first text part's text, or '' when there is none. + * The human-authored prompt is always the first text part, so only `ignored` + * parts are skipped. A file-only message yields an empty string. + */ +export function firstHumanText(parts: readonly Part[]): string { + const part = parts.find((p): p is TextPart => isTextPart(p) && p.ignored !== true); + return part?.text ?? ''; +} + /** CLI snapshot-init progress injected as a synthetic text part (matches kilo-vscode). */ export function isSnapshotProgressPart(part: Part): boolean { return isTextPart(part) && part.synthetic === true && part.text.includes('Initializing snapshot'); diff --git a/apps/mobile/src/components/agents/session-detail-content-helpers.test.ts b/apps/mobile/src/components/agents/session-detail-content-helpers.test.ts new file mode 100644 index 0000000000..3e73f000b2 --- /dev/null +++ b/apps/mobile/src/components/agents/session-detail-content-helpers.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { type MessageDeliveryState, type StoredMessage } from '@kilocode/cloud-agent-sdk'; +import { + countInFlightMessages, + resolveRetryPrompt, + retryMessageAndClear, +} from './session-detail-content-helpers'; +import { assistantMessage, userMessage } from './message-bubble-test-utils'; + +describe('countInFlightMessages', () => { + it('excludes a failed pending row from the in-flight count', () => { + const pending = new Map([ + ['m1', { status: 'queued' }], + ['m2', { status: 'failed', error: 'nope', reason: 'exhausted' }], + ]); + expect(countInFlightMessages(pending)).toBe(1); + }); + + it('returns zero when every pending row failed', () => { + const pending = new Map([ + ['m1', { status: 'failed', error: 'nope', reason: 'interrupted' }], + ]); + expect(countInFlightMessages(pending)).toBe(0); + }); + + it('counts every queued row', () => { + const pending = new Map([ + ['m1', { status: 'queued' }], + ['m2', { status: 'queued' }], + ]); + expect(countInFlightMessages(pending)).toBe(2); + }); +}); + +describe('retryMessageAndClear', () => { + it('clears the failed row when the retry send succeeds', async () => { + const send = vi.fn<() => Promise>().mockResolvedValue(undefined); + const clearFailed = vi.fn<() => void>(); + await retryMessageAndClear(send, clearFailed); + expect(send).toHaveBeenCalledTimes(1); + expect(clearFailed).toHaveBeenCalledTimes(1); + }); + + it('does not clear the failed row when the retry send fails', async () => { + const send = vi + .fn<() => Promise>() + .mockRejectedValue(new Error('Failed to send message')); + const clearFailed = vi.fn<() => void>(); + await retryMessageAndClear(send, clearFailed); + expect(send).toHaveBeenCalledTimes(1); + expect(clearFailed).not.toHaveBeenCalled(); + }); +}); + +describe('resolveRetryPrompt', () => { + it('returns only the first human text part for a user row with a synthetic notice', () => { + const message = userMessage('m1'); + message.parts = [ + { + id: 'm1-prompt', + sessionID: 'ses_1', + messageID: 'm1', + type: 'text', + text: 'prompt', + }, + { + id: 'm1-notice', + sessionID: 'ses_1', + messageID: 'm1', + type: 'text', + text: 'binary attachment saved: … path=…', + synthetic: true, + }, + ] as typeof message.parts; + + expect(resolveRetryPrompt(message, [message])).toBe('prompt'); + }); + + it('returns the synthetic queued prompt text for a user row whose only text part is synthetic', () => { + const message = userMessage('m1b'); + message.parts = [ + { + id: 'm1b-prompt', + sessionID: 'ses_1', + messageID: 'm1b', + type: 'text', + text: 'prompt', + synthetic: true, + }, + ] as typeof message.parts; + + expect(resolveRetryPrompt(message, [message])).toBe('prompt'); + }); + + it('returns null for a file-only user row', () => { + const message = userMessage('m2'); + message.parts = [ + { + id: 'm2-file', + sessionID: 'ses_1', + messageID: 'm2', + type: 'file', + mime: 'text/plain', + url: 'x', + }, + ] as typeof message.parts; + + expect(resolveRetryPrompt(message, [message])).toBeNull(); + }); + + it('returns the preceding user row human text for an assistant failure', () => { + const user = userMessage('m3'); + const assistant = assistantMessage('m4'); + const messages: StoredMessage[] = [user, assistant]; + + expect(resolveRetryPrompt(assistant, messages)).toBe('hi'); + }); + + it('returns null for an assistant row with no preceding user row', () => { + const assistant = assistantMessage('m5'); + expect(resolveRetryPrompt(assistant, [assistant])).toBeNull(); + }); +}); diff --git a/apps/mobile/src/components/agents/session-detail-content-helpers.ts b/apps/mobile/src/components/agents/session-detail-content-helpers.ts new file mode 100644 index 0000000000..544eb28825 --- /dev/null +++ b/apps/mobile/src/components/agents/session-detail-content-helpers.ts @@ -0,0 +1,63 @@ +import { type MessageDeliveryState, type StoredMessage } from '@kilocode/cloud-agent-sdk'; + +import { firstHumanText } from './part-types'; + +/** + * Counts pending messages that are still in flight. A terminal delivery + * failure must not count: after `status === 'failed'` the working spinner and + * wake lock would otherwise stay on forever. + */ +export function countInFlightMessages( + pendingMessages: ReadonlyMap +): number { + let count = 0; + for (const state of pendingMessages.values()) { + if (state.status !== 'failed') { + count += 1; + } + } + return count; +} + +/** + * Re-sends a failed message and clears its failed row only on success. On + * failure the row stays so the user can retry again; the manager has already + * surfaced the failure toast, so the rejection is swallowed here. + */ +export async function retryMessageAndClear( + send: () => Promise, + clearFailed: () => void +): Promise { + try { + await send(); + clearFailed(); + } catch { + // Swallow: the manager already surfaced the failure toast and the failed + // row stays so the user can retry again. + } +} + +/** + * Resolves the retry prompt for a failed row. A user delivery failure re-sends + * the row's own first human-authored text part; an assistant failure re-sends + * the newest preceding user row's. Returns null when there is no human text + * (e.g. a file-only row) or no preceding user row, which suppresses Retry. + */ +export function resolveRetryPrompt( + message: StoredMessage, + messages: readonly StoredMessage[] +): string | null { + if (message.info.role === 'user') { + const text = firstHumanText(message.parts); + return text === '' ? null : text; + } + const index = messages.findIndex(candidate => candidate.info.id === message.info.id); + for (let i = index - 1; i >= 0; i -= 1) { + const candidate = messages[i]; + if (candidate?.info.role === 'user') { + const text = firstHumanText(candidate.parts); + return text === '' ? null : text; + } + } + return null; +} diff --git a/apps/mobile/src/components/agents/session-detail-content.tsx b/apps/mobile/src/components/agents/session-detail-content.tsx index 7a5c6099c1..d294dda48d 100644 --- a/apps/mobile/src/components/agents/session-detail-content.tsx +++ b/apps/mobile/src/components/agents/session-detail-content.tsx @@ -5,7 +5,7 @@ import { type StoredMessage, } from '@kilocode/cloud-agent-sdk'; import { type Href, useFocusEffect, useIsFocused, useRouter } from 'expo-router'; -import { useAtomValue, useSetAtom } from 'jotai'; +import { useAtomValue, useSetAtom, useStore } from 'jotai'; import { MessageSquare } from '@/components/ui/icons'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useKeepAwake } from 'expo-keep-awake'; @@ -15,7 +15,7 @@ import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { toast } from 'sonner-native'; import { getBlockingInteraction } from '@/components/agents/agent-interaction-policy'; -import { ChatComposer } from '@/components/agents/chat-composer'; +import { ChatComposer, type ChatComposerControl } from '@/components/agents/chat-composer'; import { type AgentMode, customModeOptionsFromRuntimeAgents, @@ -58,7 +58,13 @@ import { shouldShowFooterWorkingIndicator, shouldShowSessionFooterRow, } from '@/components/agents/session-working-state'; +import { + countInFlightMessages, + resolveRetryPrompt, + retryMessageAndClear, +} from '@/components/agents/session-detail-content-helpers'; import { shouldKeepSessionAwake } from '@/components/agents/session-keep-awake'; +import { shouldRefetchOnFocus } from '@/components/agents/session-focus-refetch'; import { TranscriptTimeMarker } from '@/components/agents/transcript-time-marker'; import { EmptyState } from '@/components/empty-state'; import { AppAwareKeyboardPaddingView } from '@/components/kilo-chat/app-aware-keyboard-padding'; @@ -162,6 +168,7 @@ export function SessionDetailContent({ visible: false, }); const childSheetReleaseTimeoutRef = useRef | null>(null); + const composerControlRef = useRef(null); const clearChildSheetReleaseTimeout = useCallback(() => { if (childSheetReleaseTimeoutRef.current !== null) { @@ -453,10 +460,16 @@ export function SessionDetailContent({ // Refetch the linked PR on every focus so a link, unlink, or mid-session // decision change surfaces without reopening the session. A pending review // decision gets one 4s follow-up refetch — no polling loop. + const store = useStore(); + // The first focus on a session id is owned by `manager.switchSession`, which + // already fetches the session metadata (including `associatedPr`). Every + // later focus refetches; this ref tracks which id has been seeded. + const seededSessionIdRef = useRef(null); useFocusEffect( useCallback(() => { let cancelled = false; let pendingTimeout: ReturnType | null = null; + let unsubscribe: (() => void) | null = null; const refetch = async (scheduleFollowUp: boolean) => { try { @@ -479,15 +492,54 @@ export function SessionDetailContent({ } }; - void refetch(true); + // Check the current `fetchedSessionData` and, when a review decision is + // pending, schedule the one-shot 4s follow-up. Runs once against the + // current value and again on every later write until the session's data + // has landed (or the effect is cancelled). + const checkAndSchedule = (): boolean => { + if (cancelled) { + return true; + } + const fetched = store.get(manager.atoms.fetchedSessionData); + if (fetched?.kiloSessionId !== sessionId) { + return false; + } + unsubscribe?.(); + unsubscribe = null; + if (fetched.associatedPr?.reviewDecisionPending) { + pendingTimeout = setTimeout(() => { + pendingTimeout = null; + void refetch(false); + }, 4000); + } + return true; + }; + + if (!shouldRefetchOnFocus(seededSessionIdRef.current, sessionId)) { + // First focus: `switchSession` owns the metadata read, so issue no + // request. Seed the ref and keep the pending-decision follow-up. The + // manager's fetch can land before this effect runs (switchSession + // writes first), so check the current value once before subscribing. + // Subscribe only when the data has not landed yet; a match schedules + // at most one follow-up and stops listening. + seededSessionIdRef.current = sessionId; + if (!checkAndSchedule()) { + unsubscribe = store.sub(manager.atoms.fetchedSessionData, () => { + checkAndSchedule(); + }); + } + } else { + void refetch(true); + } return () => { cancelled = true; + unsubscribe?.(); if (pendingTimeout !== null) { clearTimeout(pendingTimeout); } }; - }, [manager, sessionId]) + }, [manager, sessionId, store]) ); useEffect(() => { @@ -589,6 +641,130 @@ export function SessionDetailContent({ } } + const requiresModel = Boolean(fetchedData?.cloudAgentSessionId); + + const handleSend = useCallback( + async ( + text: string, + attachments?: AgentAttachmentWire, + submission?: AgentAttachmentSubmissionPayload + ) => { + if (requiresModel && !(pinned.model ?? currentModel)) { + toast.error('Select a model before sending'); + return; + } + // Pick the wire shape via the same pure helper the unit test covers: + // - cloud-agent → unchanged `{path, files}` (S3a) + // - remote + supportsAttachments → materialize presigned GETs and + // forward as `attachmentParts` (S3b) + // - everything else → no attachment field on the wire + const kind = resolveSendAttachmentKind( + activeSessionType, + supportsAttachments, + attachments !== undefined + ); + if (shouldRefuseSilentAttachmentDrop(kind, attachments !== undefined)) { + const message = + "This session can't receive files. Remove the attachments to send your message."; + toast.error(message); + throw new Error(message); + } + let attachmentParts: Awaited> | undefined = + undefined; + if (kind === 'remote-capable' && submission) { + const result = await buildRemoteAttachmentPartsWithRetryableFeedback( + submission, + buildRemoteAttachmentParts + ); + if (!result.ok) { + // Retryable presign failure: the manager never reached send(), so + // its onSendFailed toast does not fire. Surface the retryable message + // through the same toast channel and throw so the composer keeps the + // draft/attachments for a retry. + toast.error(result.message); + throw new Error(result.message); + } + attachmentParts = result.parts; + } + const sendModel = + activeSessionType === 'cloud-agent' && pinned.model ? pinned.model : currentModel; + const sendVariant = + activeSessionType === 'cloud-agent' && pinned.model + ? (pinned.variant ?? '') + : currentVariant; + // Sync the override to the exact model/variant being sent so the SDK's + // `cloudAgentModelOverride` preference cannot beat the pin on send, and + // a leftover pin cannot beat a user pick. `sendModel` is always truthy + // here (the guard above returns early when no model resolves), so this + // never clears to null on an unpinned send. + if (activeSessionType === 'cloud-agent') { + manager.setCloudAgentModelOverride( + sendModel ? { model: sendModel, ...(sendVariant ? { variant: sendVariant } : {}) } : null + ); + } + // manager.send() reports failures via its own return value (and toasts + // through the manager's onSendFailed hook) rather than rejecting — it + // is the single toast owner for send failures. Throw here, without a + // second toast, purely so the composer's `await onSend(...)` sees the + // rejection and preserves the draft. + const sent = await manager.send({ + payload: { + type: 'prompt', + prompt: text, + mode: currentMode, + model: sendModel, + variant: sendVariant || undefined, + }, + ...(kind === 'cloud' && attachments ? { attachments } : {}), + ...(kind === 'remote-capable' && attachmentParts ? { attachmentParts } : {}), + }); + if (!sent) { + throw new Error('Failed to send message'); + } + captureEvent(MESSAGE_SENT_EVENT, { surface: analyticsSurface }); + }, + [ + manager, + currentMode, + currentModel, + currentVariant, + pinned.model, + pinned.variant, + requiresModel, + activeSessionType, + supportsAttachments, + analyticsSurface, + ] + ); + + const handleCopyToComposer = useCallback((text: string) => { + composerControlRef.current?.setText(text); + }, []); + + const handleRetryMessage = useCallback( + (message: StoredMessage) => { + const prompt = resolveRetryPrompt(message, messages); + if (prompt === null) { + return; + } + // Same guard handleSend opens with: when no model resolves, run the send + // anyway (the user gets the existing toast) and keep the failed row. + if (requiresModel && !(pinned.model ?? currentModel)) { + void handleSend(prompt); + return; + } + void retryMessageAndClear( + async () => { + await handleSend(prompt); + }, + () => { + manager.clearFailedMessage(message.info.id); + } + ); + }, + [messages, requiresModel, pinned.model, currentModel, handleSend, manager] + ); + const renderItem = useCallback( ({ item }: { item: SessionTranscriptItem }) => { if (item.type === 'preparation') { @@ -603,6 +779,8 @@ export function SessionDetailContent({ // so a plain lookup is enough — no render-order guard. const deliveryState = item.message.info.role === 'user' ? pendingMessages.get(item.message.info.id) : undefined; + // Suppress Retry on an assistant failure with no preceding user row. + const retryPrompt = resolveRetryPrompt(item.message, messages); return ( ); }, @@ -625,6 +805,9 @@ export function SessionDetailContent({ handleOpenChildSession, pendingMessages, heldQueuedIds, + messages, + handleRetryMessage, + handleCopyToComposer, ] ); @@ -685,9 +868,15 @@ export function SessionDetailContent({ (fetchedData === null && !statusIndicator && !error) || (fetchedData !== null && fetchedData.kiloSessionId !== sessionId); const shouldBlockMessages = shouldShowLoading; + // Failed delivery entries must not count as in-flight: after a terminal + // delivery failure the working spinner and wake lock would otherwise stay on. + const inFlightMessageCount = useMemo( + () => countInFlightMessages(pendingMessages), + [pendingMessages] + ); const shouldShowWorkingIndicator = shouldShowAgentWorkingIndicator({ isStreaming, - pendingMessageCount: pendingMessages.size, + pendingMessageCount: inFlightMessageCount, }); const hasFooterStatusIndicator = statusIndicator !== null || (cloudStatus !== null && cloudStatus.type !== 'ready'); @@ -744,7 +933,6 @@ export function SessionDetailContent({ /> ); - const requiresModel = Boolean(fetchedData?.cloudAgentSessionId); const blockingInteraction = getBlockingInteraction({ activeQuestion, activePermission }); const hasBlockingInteraction = blockingInteraction !== 'none'; // One number for both kinds: the user must see every waiting request, not @@ -796,100 +984,6 @@ export function SessionDetailContent({ 'Message...'; const keyboardContainerKind = getSessionKeyboardContainerKind(Platform.OS); - const handleSend = useCallback( - async ( - text: string, - attachments?: AgentAttachmentWire, - submission?: AgentAttachmentSubmissionPayload - ) => { - if (requiresModel && !(pinned.model ?? currentModel)) { - toast.error('Select a model before sending'); - return; - } - // Pick the wire shape via the same pure helper the unit test covers: - // - cloud-agent → unchanged `{path, files}` (S3a) - // - remote + supportsAttachments → materialize presigned GETs and - // forward as `attachmentParts` (S3b) - // - everything else → no attachment field on the wire - const kind = resolveSendAttachmentKind( - activeSessionType, - supportsAttachments, - attachments !== undefined - ); - if (shouldRefuseSilentAttachmentDrop(kind, attachments !== undefined)) { - const message = - "This session can't receive files. Remove the attachments to send your message."; - toast.error(message); - throw new Error(message); - } - let attachmentParts: Awaited> | undefined = - undefined; - if (kind === 'remote-capable' && submission) { - const result = await buildRemoteAttachmentPartsWithRetryableFeedback( - submission, - buildRemoteAttachmentParts - ); - if (!result.ok) { - // Retryable presign failure: the manager never reached send(), so - // its onSendFailed toast does not fire. Surface the retryable message - // through the same toast channel and throw so the composer keeps the - // draft/attachments for a retry. - toast.error(result.message); - throw new Error(result.message); - } - attachmentParts = result.parts; - } - const sendModel = - activeSessionType === 'cloud-agent' && pinned.model ? pinned.model : currentModel; - const sendVariant = - activeSessionType === 'cloud-agent' && pinned.model - ? (pinned.variant ?? '') - : currentVariant; - // Sync the override to the exact model/variant being sent so the SDK's - // `cloudAgentModelOverride` preference cannot beat the pin on send, and - // a leftover pin cannot beat a user pick. `sendModel` is always truthy - // here (the guard above returns early when no model resolves), so this - // never clears to null on an unpinned send. - if (activeSessionType === 'cloud-agent') { - manager.setCloudAgentModelOverride( - sendModel ? { model: sendModel, ...(sendVariant ? { variant: sendVariant } : {}) } : null - ); - } - // manager.send() reports failures via its own return value (and toasts - // through the manager's onSendFailed hook) rather than rejecting — it - // is the single toast owner for send failures. Throw here, without a - // second toast, purely so the composer's `await onSend(...)` sees the - // rejection and preserves the draft. - const sent = await manager.send({ - payload: { - type: 'prompt', - prompt: text, - mode: currentMode, - model: sendModel, - variant: sendVariant || undefined, - }, - ...(kind === 'cloud' && attachments ? { attachments } : {}), - ...(kind === 'remote-capable' && attachmentParts ? { attachmentParts } : {}), - }); - if (!sent) { - throw new Error('Failed to send message'); - } - captureEvent(MESSAGE_SENT_EVENT, { surface: analyticsSurface }); - }, - [ - manager, - currentMode, - currentModel, - currentVariant, - pinned.model, - pinned.variant, - requiresModel, - activeSessionType, - supportsAttachments, - analyticsSurface, - ] - ); - const handleSendCommand = useCallback( async (command: string, argumentsText: string) => { // Slash commands ride the same manager.send() pipeline. The manager @@ -976,9 +1070,24 @@ export function SessionDetailContent({ isFocused, isDisconnected: agentStatus.type === 'disconnected', isStreaming, - pendingMessageCount: pendingMessages.size, + pendingMessageCount: inFlightMessageCount, }); + // Child-sheet pagination fields live only on the `ready` hydration state. + // The sheet can render `content` before hydration reports ready (live child + // rows already exist), so default every non-ready state. + const openChildSessionId = childSessionSheet.sheet?.sessionId ?? null; + const openChildHydrationState = + openChildSessionId === null ? null : getChildSessionHydrationState(openChildSessionId); + const childHasOlderMessages = + openChildHydrationState?.status === 'ready' ? openChildHydrationState.hasOlder : false; + const childIsLoadingOlderMessages = + openChildHydrationState?.status === 'ready' ? openChildHydrationState.isLoadingOlder : false; + const childOlderMessagesError = + openChildHydrationState?.status === 'ready' ? openChildHydrationState.olderError : null; + const childOlderMessagesOmittedItemCount = + openChildHydrationState?.status === 'ready' ? openChildHydrationState.omittedItemCount : 0; + return ( @@ -1049,6 +1158,15 @@ export function SessionDetailContent({ getChildMessages={getChildMessages} hydrationState={getChildSessionHydrationState(childSessionSheet.sheet.sessionId)} isStreaming={getChildSessionStreaming(messages, childSessionSheet.sheet.sessionId)} + hasOlderMessages={childHasOlderMessages} + isLoadingOlderMessages={childIsLoadingOlderMessages} + olderMessagesError={childOlderMessagesError} + olderMessagesOmittedItemCount={childOlderMessagesOmittedItemCount} + onLoadOlderMessages={() => { + if (openChildSessionId !== null) { + void manager.loadOlderChildMessages(openChildSessionId); + } + }} renderPart={props => } onOpenChildSession={handleOpenChildSession} onRetry={() => { @@ -1195,7 +1313,8 @@ export function SessionDetailContent({ shareId={shareId} autoSend={autoSend} draftKey={userId ? sessionComposerDraftKey : undefined} - initialDraft={composerDraft.settled ? (composerDraft.text ?? '') : undefined} + initialDraft={composerDraft.settled ? (composerDraft.value ?? '') : undefined} + controlRef={composerControlRef} /> @@ -1279,6 +1398,9 @@ export function SessionDetailContent({ onLoadOlderMessages={() => { void manager.loadOlderMessages(); }} + onReachedBottom={() => { + manager.trimRetainedHistory(); + }} renderItem={renderItem} /> ); diff --git a/apps/mobile/src/components/agents/session-focus-refetch.test.ts b/apps/mobile/src/components/agents/session-focus-refetch.test.ts new file mode 100644 index 0000000000..fb860f5430 --- /dev/null +++ b/apps/mobile/src/components/agents/session-focus-refetch.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from 'vitest'; + +import { shouldRefetchOnFocus } from '@/components/agents/session-focus-refetch'; + +describe('shouldRefetchOnFocus', () => { + it('returns false on the first focus of a session id', () => { + expect(shouldRefetchOnFocus(null, 'session-a')).toBe(false); + }); + + it('returns true on a second focus of the same session id', () => { + expect(shouldRefetchOnFocus('session-a', 'session-a')).toBe(true); + }); + + it('returns false again on a focus of a different session id', () => { + expect(shouldRefetchOnFocus('session-a', 'session-b')).toBe(false); + }); +}); diff --git a/apps/mobile/src/components/agents/session-focus-refetch.ts b/apps/mobile/src/components/agents/session-focus-refetch.ts new file mode 100644 index 0000000000..e90a757d17 --- /dev/null +++ b/apps/mobile/src/components/agents/session-focus-refetch.ts @@ -0,0 +1,12 @@ +/** + * Decides whether a session focus must refetch its linked-PR metadata. + * + * The first focus on a session id is owned by `manager.switchSession`, which + * already fetches the session (including `associatedPr`) and writes it into + * `fetchedSessionDataAtom`. Every later focus on the same id must refetch so a + * link, unlink, or mid-session decision change surfaces without reopening the + * session. + */ +export function shouldRefetchOnFocus(seededSessionId: string | null, sessionId: string): boolean { + return seededSessionId === sessionId; +} diff --git a/apps/mobile/src/components/agents/session-message-list.tsx b/apps/mobile/src/components/agents/session-message-list.tsx index 3b25558970..cde3e397f8 100644 --- a/apps/mobile/src/components/agents/session-message-list.tsx +++ b/apps/mobile/src/components/agents/session-message-list.tsx @@ -43,6 +43,13 @@ type SessionMessageListProps = { * indicators. */ contentBottomInset?: number; + /** + * Optional callback fired when the list returns to the bottom after the + * user scrolled away. Fires only on the false→true transition of + * `isAtBottom`, never on mount. The host uses this to trim retained + * history exactly when the user returns to the live tail. + */ + onReachedBottom?: () => void; }; export function SessionMessageList({ @@ -57,6 +64,7 @@ export function SessionMessageList({ renderItem, ListFooterComponent, contentBottomInset, + onReachedBottom, }: Readonly>) { // FlashList v2 renders the list in chronological order (oldest → newest). // `startRenderingFromBottom` keeps the viewport anchored at the newest @@ -113,6 +121,22 @@ export function SessionMessageList({ inFlightRef.current = false; }, [sessionId]); + // Fire `onReachedBottom` only on the false→true transition of + // `isAtBottom`. The previous-value ref prevents a fire on mount (the list + // starts at the bottom) and on repeat renders while already at the bottom. + // The handler is held in a ref so a new inline callback identity from the + // host never re-runs this effect. + const onReachedBottomRef = useRef(onReachedBottom); + onReachedBottomRef.current = onReachedBottom; + const prevIsAtBottomRef = useRef(isAtBottom); + useEffect(() => { + const prev = prevIsAtBottomRef.current; + prevIsAtBottomRef.current = isAtBottom; + if (isAtBottom && !prev) { + onReachedBottomRef.current?.(); + } + }, [isAtBottom]); + // Non-visual a11y signal for older-page arrival (visual loading skeleton // was removed). Announce only when items were actually prepended. const olderArrivalInitializedRef = useRef(false); diff --git a/apps/mobile/src/components/agents/use-new-session-creator.test.ts b/apps/mobile/src/components/agents/use-new-session-creator.test.ts index 084658bbff..821b22f445 100644 --- a/apps/mobile/src/components/agents/use-new-session-creator.test.ts +++ b/apps/mobile/src/components/agents/use-new-session-creator.test.ts @@ -797,7 +797,7 @@ describe('restored new-session submit', () => { }); }); -type DraftLoadState = { settled: boolean; text: string | null }; +type DraftLoadState = { settled: boolean; value: string | null }; function FencedDraftHarness({ userId, @@ -878,14 +878,14 @@ describe('useFencedDraftLoad generation fencing', () => { firstLoad.resolve(staleText); }); await flushMicrotasks(); - expect(renders.at(-1)).toEqual({ settled: false, text: null }); + expect(renders.at(-1)).toEqual({ settled: false, value: null }); // The current generation's load resolves: it publishes. await act(async () => { secondLoad.resolve(freshText); }); await flushMicrotasks(); - expect(renders.at(-1)).toEqual({ settled: true, text: freshText }); + expect(renders.at(-1)).toEqual({ settled: true, value: freshText }); expect(vi.mocked(loadDraft)).toHaveBeenCalledWith( second.userId, second.entityKey, @@ -920,7 +920,7 @@ describe('useFencedDraftLoad generation fencing', () => { gate.resolve('late draft'); }); await flushMicrotasks(); - expect(renders.at(-1)).toEqual({ settled: false, text: null }); + expect(renders.at(-1)).toEqual({ settled: false, value: null }); }); }); diff --git a/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts b/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts index 8fddcb6ec8..9ba2ed4b52 100644 --- a/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts +++ b/apps/mobile/src/components/pr-review/discussion/reply-input.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- cohesive suite for the Terms gate, reply draft clear, and settle-gate contracts */ // Four-state coverage for the UGC Terms gate (`ensureTermsAcceptedOutcome`). // // - happy: accept succeeds → `accepted`. @@ -9,17 +10,24 @@ // Cancel buttons the gate renders. The gate is a pure async function, so no // React mounting is required. +import * as React from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { ensureTermsAcceptedOutcome } from './reply-input'; +import { ensureTermsAcceptedOutcome, ReplyInput } from './reply-input'; +import { clearDraft } from '@/lib/persist/drafts'; +import { type useReplyToCommentMutation } from '@/lib/pr-review/discussion/use-review-discussion-mutations'; type AlertButton = { text?: string; onPress?: () => void }; type AlertCall = { title: string; message: string; buttons: AlertButton[] }; -const { alertCalls, getTermsStatusMock, acceptTermsMock } = vi.hoisted(() => ({ +const { alertCalls, getTermsStatusMock, acceptTermsMock, draftLoadMock } = vi.hoisted(() => ({ alertCalls: [] as AlertCall[], getTermsStatusMock: vi.fn(), acceptTermsMock: vi.fn(), + draftLoadMock: vi.fn((): { settled: boolean; value: string | null } => ({ + settled: true, + value: null, + })), })); vi.mock('react-native', () => ({ @@ -62,6 +70,51 @@ vi.mock('@/lib/hooks/use-theme-colors', () => ({ useThemeColors: () => ({ mutedForeground: '#000000' }), })); +// `reply-input` imports the durable-draft chain, which pulls in the native +// encrypted-kv → expo-secure-store → expo-modules-core chain that the node +// test environment cannot resolve. Mock the persist chain and the identity +// hook so this suite stays node-only. +vi.mock('@/lib/persist/drafts', () => ({ + saveDraft: vi.fn(), + clearDraft: vi.fn(), + prReplyDraftKey: vi.fn(() => 'pr-reply:key'), + prMergeDraftKey: vi.fn(), + prCommentDraftKey: vi.fn(), +})); + +vi.mock('@/lib/persist/use-draft-load', () => ({ + useFencedDraftLoad: () => draftLoadMock(), +})); + +vi.mock('@/lib/persist/use-draft-flush', () => ({ + useDraftFlushOnBackground: vi.fn(), +})); + +vi.mock('@/lib/hooks/use-current-user-id', () => ({ + useCurrentUserId: () => ({ userId: 'u1', isLoading: false }), +})); + +// `ReplyInput` is mounted by calling it as a plain function (no renderer), so +// the React hook primitives are stubbed to no-op/simple versions, mirroring +// pr-merge-sheet.test.tsx. The pure `ensureTermsAcceptedOutcome` tests above +// do not touch these. +vi.mock('react', async () => { + const actual = await vi.importActual('react'); + return { + ...actual, + useState: vi.fn((initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void]), + useMemo: vi.fn((factory: () => T) => factory()), + useRef: vi.fn((initial: T) => { + const ref: React.RefObject = { current: initial }; + return ref; + }), + useEffect: vi.fn((effect: React.EffectCallback) => { + effect(); + }), + useCallback: vi.fn( unknown>(fn: T) => fn), + }; +}); + /** Drains microtasks so the awaited getTermsStatus/acceptTerms settle. */ async function flush(): Promise { await Promise.resolve(); @@ -187,3 +240,217 @@ describe('ensureTermsAcceptedOutcome', () => { expect(alertCalls).toHaveLength(0); }); }); + +type ReplyMutation = ReturnType; + +function makeReply(mutate: unknown): ReplyMutation { + return { mutate, isPending: false, error: null } as unknown as ReplyMutation; +} + +type FindElementArgs = { + node: unknown; + type: string; + prop: string; + value: unknown; +}; + +function findElement({ node, type, prop, value }: FindElementArgs): React.ReactElement | null { + if (React.isValidElement(node)) { + const element = node; + const props = element.props as Record; + if (element.type === type && props[prop] === value) { + return element; + } + const children = props.children; + if (Array.isArray(children)) { + for (const child of children) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } else if (children !== undefined && children !== null) { + const found = findElement({ node: children, type, prop, value }); + if (found) { + return found; + } + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findElement({ node: child, type, prop, value }); + if (found) { + return found; + } + } + } + return null; +} + +/** Mounts ReplyInput, types a body, and presses the submit button. */ +function mountAndSubmit(reply: ReplyMutation): void { + // eslint-disable-next-line new-cap + const element = ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply, + }); + const input = findElement({ + node: element, + type: 'TextInput', + prop: 'accessibilityLabel', + value: 'Reply body', + }); + if (!input) { + throw new Error('Reply body TextInput not found'); + } + (input.props as { onChangeText?: (value: string) => void }).onChangeText?.('hello'); + const button = findElement({ + node: element, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Submit reply', + }); + if (!button) { + throw new Error('Submit reply Button not found'); + } + (button.props as { onPress?: () => void }).onPress?.(); +} + +describe('ReplyInput draft clear on submit', () => { + beforeEach(() => { + alertCalls.length = 0; + getTermsStatusMock.mockReset(); + acceptTermsMock.mockReset(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('clears the reply draft on a successful reply', async () => { + getTermsStatusMock.mockResolvedValue({ accepted: true, currentVersion: 'v1' }); + const mutate = vi.fn((_input: unknown, options: { onSuccess?: () => void }) => { + options.onSuccess?.(); + }); + mountAndSubmit(makeReply(mutate)); + await flush(); + + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-reply:key'); + }); + + it('does not clear the reply draft on a failed reply', async () => { + getTermsStatusMock.mockResolvedValue({ accepted: true, currentVersion: 'v1' }); + // A failed mutation never invokes onSuccess, so the draft must survive. + const mutate = vi.fn(); + mountAndSubmit(makeReply(mutate)); + await flush(); + + expect(mutate).toHaveBeenCalledTimes(1); + expect(clearDraft).not.toHaveBeenCalled(); + }); + + it('does not clear the reply draft when the terms gate is dismissed', async () => { + getTermsStatusMock.mockResolvedValue({ accepted: false, currentVersion: 'v1' }); + const mutate = vi.fn(); + mountAndSubmit(makeReply(mutate)); + await flush(); + pressButton('Cancel'); + await flush(); + + expect(mutate).not.toHaveBeenCalled(); + expect(clearDraft).not.toHaveBeenCalled(); + }); +}); + +describe('ReplyInput seeds the field from the settled draft during render', () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + function mountReplyInput(): React.ReactElement | null { + return findElement({ + // eslint-disable-next-line new-cap + node: ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply: makeReply(vi.fn()), + }), + type: 'TextInput', + prop: 'accessibilityLabel', + value: 'Reply body', + }); + } + + it('seeds the defaultValue from the settled draft value', () => { + draftLoadMock.mockReturnValue({ settled: true, value: 'saved reply' }); + const input = mountReplyInput(); + if (!input) { + throw new Error('Reply body TextInput not found'); + } + expect((input.props as { defaultValue?: string }).defaultValue).toBe('saved reply'); + }); + + it('seeds an empty field when the settled draft has no value (no stale previous-thread text)', () => { + draftLoadMock.mockReturnValue({ settled: true, value: null }); + const input = mountReplyInput(); + if (!input) { + throw new Error('Reply body TextInput not found'); + } + expect((input.props as { defaultValue?: string }).defaultValue).toBe(''); + }); +}); + +describe('ReplyInput gates input on draft settle', () => { + it('hides the input and disables submit until the draft settles', () => { + draftLoadMock.mockReturnValue({ settled: false, value: null }); + // eslint-disable-next-line new-cap + const hidden = ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply: makeReply(vi.fn()), + }); + expect( + findElement({ + node: hidden, + type: 'TextInput', + prop: 'accessibilityLabel', + value: 'Reply body', + }) + ).toBeNull(); + const button = findElement({ + node: hidden, + type: 'Button', + prop: 'accessibilityLabel', + value: 'Submit reply', + }); + if (!button) { + throw new Error('Submit reply Button not found'); + } + expect((button.props as { disabled?: boolean }).disabled).toBe(true); + + draftLoadMock.mockReturnValue({ settled: true, value: null }); + // eslint-disable-next-line new-cap + const shown = ReplyInput({ + owner: 'octocat', + repo: 'hello', + number: 1, + commentId: 42, + reply: makeReply(vi.fn()), + }); + expect( + findElement({ + node: shown, + type: 'TextInput', + prop: 'accessibilityLabel', + value: 'Reply body', + }) + ).not.toBeNull(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/discussion/reply-input.tsx b/apps/mobile/src/components/pr-review/discussion/reply-input.tsx index 2b50dcd602..36bd4e3f10 100644 --- a/apps/mobile/src/components/pr-review/discussion/reply-input.tsx +++ b/apps/mobile/src/components/pr-review/discussion/reply-input.tsx @@ -12,7 +12,11 @@ import { PrReviewReconnectNotice } from '@/components/pr-review/pr-review-reconn import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { WEB_BASE_URL } from '@/lib/config'; +import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; +import { clearDraft, prReplyDraftKey, saveDraft } from '@/lib/persist/drafts'; +import { useDraftFlushOnBackground } from '@/lib/persist/use-draft-flush'; +import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; import { classifyPrReviewMutationError } from '@/lib/pr-review/classify-pr-review-query-state'; import { type useReplyToCommentMutation } from '@/lib/pr-review/discussion/use-review-discussion-mutations'; import { @@ -157,6 +161,25 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly(null); const [resetKey, setResetKey] = useState(0); + // Durable reply draft, keyed by account and thread. Nothing is saved or + // restored while the user id is unknown. + const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); + const replyDraftKey = prReplyDraftKey(owner, repo, number, commentId); + const draft = useFencedDraftLoad({ userId, isIdentityLoading, entityKey: replyDraftKey }); + useDraftFlushOnBackground(userId, replyDraftKey, true); + + // Seed the field once per identity/thread, during render, before the input + // mounts. The settled gate already unmounts the field on an identity/entity + // change, so re-seeding here (and resetting to empty when there is no draft) + // keeps a reused instance from showing or saving the previous account's or + // thread's text under the new key. + const replySeedKey = `${userId ?? 'anonymous'}\u0000${replyDraftKey}`; + const seededKeyRef = useRef(null); + if (draft.settled && seededKeyRef.current !== replySeedKey) { + seededKeyRef.current = replySeedKey; + bodyRef.current = draft.value ?? ''; + } + // Mirror mutation error into the inline box. Reply is NOT // optimistic, so the user can hit the inline error and retry // without waiting for a re-fetch. @@ -225,6 +248,9 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly { bodyRef.current = ''; + if (userId) { + void clearDraft(userId, replyDraftKey); + } setResetKey(prev => prev + 1); }, } @@ -233,25 +259,30 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly - { - bodyRef.current = value; - if (inlineError) { - setInlineError(null); - setInlineErrorKind(null); - } - }} - multiline - textAlignVertical="top" - className="min-h-16 rounded-md border border-input bg-background px-3 py-2 text-sm leading-5 text-foreground" - /> + {draft.settled ? ( + { + bodyRef.current = value; + if (userId) { + saveDraft(userId, replyDraftKey, value); + } + if (inlineError) { + setInlineError(null); + setInlineErrorKind(null); + } + }} + multiline + textAlignVertical="top" + className="min-h-16 rounded-md border border-input bg-background px-3 py-2 text-sm leading-5 text-foreground" + /> + ) : null} {inlineError && inlineErrorKind !== 'reconnect' ? ( {inlineError} ) : null} @@ -262,6 +293,7 @@ export function ReplyInput({ owner, repo, number, commentId, reply }: Readonly ({ mutateAsync: vi.fn<() => Promise>(), @@ -123,6 +125,33 @@ vi.mock('@/lib/pr-review/merge/merge-commit-defaults', () => ({ defaultCommitMessage: () => '', })); +// The sheet imports the durable-draft chain, which pulls in the native +// encrypted-kv → expo-secure-store → expo-modules-core chain that the node +// test environment cannot resolve. Mock the persist chain and the identity +// hook so this suite stays node-only. +vi.mock('@/lib/persist/drafts', () => ({ + saveDraft: vi.fn(), + clearDraft: vi.fn(), + isMergeDraft: vi.fn(), + prMergeDraftKey: vi.fn( + (owner: string, repo: string, number: number) => `pr-merge:${owner}/${repo}#${number}` + ), + prReplyDraftKey: vi.fn(), + prCommentDraftKey: vi.fn(), +})); + +vi.mock('@/lib/persist/use-draft-load', () => ({ + useFencedDraftLoad: () => ({ settled: true, value: null }), +})); + +vi.mock('@/lib/persist/use-draft-flush', () => ({ + useDraftFlushOnBackground: () => undefined, +})); + +vi.mock('@/lib/hooks/use-current-user-id', () => ({ + useCurrentUserId: () => ({ userId: 'u1', isLoading: false }), +})); + const REF = { owner: 'octocat', repo: 'hello', number: 1 }; const repoSettings: PrOverviewRepoSettings = { @@ -256,6 +285,7 @@ describe('PrMergeSheet performSubmit wiring (P0-B-08)', () => { ); expect(onRefetch).toHaveBeenCalledTimes(1); expect(onDismiss).toHaveBeenCalledTimes(1); + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-merge:octocat/hello#1'); }); it('clean success (merged:true + branchDeleted:true) fires haptic and dismisses without writing a banner', async () => { @@ -278,6 +308,7 @@ describe('PrMergeSheet performSubmit wiring (P0-B-08)', () => { ); expect(onRefetch).toHaveBeenCalledTimes(1); expect(onDismiss).toHaveBeenCalledTimes(1); + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-merge:octocat/hello#1'); }); it('rejected mutation (merged:false) does not fire haptic, refetch, dismiss, or write a banner', async () => { @@ -294,5 +325,52 @@ describe('PrMergeSheet performSubmit wiring (P0-B-08)', () => { expect(Haptics.notificationAsync).not.toHaveBeenCalled(); expect(onRefetch).not.toHaveBeenCalled(); expect(onDismiss).not.toHaveBeenCalled(); + expect(clearDraft).not.toHaveBeenCalled(); + }); + + it('confirmed cancel clears the draft and dismisses', () => { + const onDismiss = vi.fn(); + const props = { ...baseProps, onDismiss }; + // eslint-disable-next-line new-cap + const element = PrMergeSheet(props); + const formBody = findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Merge', + }); + if (!formBody) { + throw new Error('MergeSheetFormBody not found in rendered tree'); + } + const onDismissProp = (formBody.props as { onDismiss?: () => void }).onDismiss; + onDismissProp?.(); + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-merge:octocat/hello#1'); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('auto-merge enable success clears the draft and dismisses', async () => { + const onDismiss = vi.fn(); + const onRefetch = vi.fn().mockResolvedValue(undefined); + const props = { ...baseProps, mode: 'enable-auto-merge' as const, onDismiss, onRefetch }; + + autoMergeMutationMocks.mutateAsync.mockResolvedValueOnce({}); + + // eslint-disable-next-line new-cap + const element = PrMergeSheet(props); + const formBody = findElement({ + node: element, + type: 'MergeSheetFormBody', + prop: 'submitLabel', + value: 'Enable auto-merge', + }); + if (!formBody) { + throw new Error('MergeSheetFormBody not found in rendered tree'); + } + const onConfirm = (formBody.props as { onConfirm?: () => void }).onConfirm; + onConfirm?.(); + await flushMicrotasks(); + + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-merge:octocat/hello#1'); + expect(onDismiss).toHaveBeenCalledTimes(1); }); }); diff --git a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx index 9704bacf84..abba42ad5a 100644 --- a/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx +++ b/apps/mobile/src/components/pr-review/merge/pr-merge-sheet.tsx @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the merge sheet owns the durable merge draft (load, save, seed, clear) beside the existing merge/auto-merge form; the draft wiring stays with the form it persists */ // S8 merge sheet. The orchestrator mounts this inside the // `[owner]/[repo]/[number]/merge.tsx` route; the orchestrator-wired // `PrReviewMergeScreen` fetches the overview DTO, derives the form's @@ -14,7 +15,7 @@ import * as Haptics from 'expo-haptics'; import { Alert, Keyboard, ScrollView, type TextInput, useWindowDimensions } from 'react-native'; -import { useEffect, useMemo, useRef, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { PrFormSheetHeader } from '@/components/pr-review/pr-form-sheet-chrome'; import { @@ -41,6 +42,10 @@ import { defaultCommitMessage, defaultCommitTitle, } from '@/lib/pr-review/merge/merge-commit-defaults'; +import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { clearDraft, isMergeDraft, prMergeDraftKey, saveDraft } from '@/lib/persist/drafts'; +import { useDraftFlushOnBackground } from '@/lib/persist/use-draft-flush'; +import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; type PrMergeSheetMode = 'merge' | 'enable-auto-merge'; @@ -88,6 +93,25 @@ type AutoMergeInput = { commitMessage?: string; }; +/** + * Wraps an uncontrolled-input ref so every `.current` write (the parts file's + * `onChangeText`) also fires `onWrite`. The merge sheet owns the save but the + * input handlers live in `pr-merge-sheet-parts.tsx`; the proxy hooks the write + * without touching that file. + */ +function savingRef(target: { current: T }, onWrite: () => void) { + return new Proxy(target, { + set(obj, prop, value) { + if (prop === 'current') { + obj.current = value as T; + onWrite(); + return true; + } + return Reflect.set(obj, prop, value); + }, + }); +} + export function PrMergeSheet(props: PrMergeSheetProps) { const { owner, @@ -128,6 +152,41 @@ export function PrMergeSheet(props: PrMergeSheetProps) { const titleRef = useRef(defaultCommitTitle(title, number)); const messageRef = useRef(defaultCommitMessage(bodyMarkdown)); const { height: windowHeight } = useWindowDimensions(); + + // Durable merge draft. Identity gates save/restore: nothing is written or + // read while the user id is unknown. The inputs render only once the draft + // settles, seeded from the stored value or today's defaults. + const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); + const mergeDraftKey = prMergeDraftKey(owner, repoName, number); + const draft = useFencedDraftLoad<{ title: string; message: string }>({ + userId, + isIdentityLoading, + entityKey: mergeDraftKey, + validate: isMergeDraft, + }); + // Seed the fields once per identity/destination. The settled gate already + // unmounts the form on an identity/entity change, so re-seeding here (and + // resetting to today's defaults when there is no draft) keeps a reused + // instance from showing or saving the previous account's or PR's text. + const draftSeedKeyRef = useRef(null); + const draftSeedKey = `${userId ?? 'anonymous'}\u0000${mergeDraftKey}`; + if (draft.settled && draftSeedKeyRef.current !== draftSeedKey) { + draftSeedKeyRef.current = draftSeedKey; + titleRef.current = draft.value?.title ?? defaultCommitTitle(title, number); + messageRef.current = draft.value?.message ?? defaultCommitMessage(bodyMarkdown); + } + + const saveMergeDraft = useCallback(() => { + if (userId) { + saveDraft(userId, mergeDraftKey, { title: titleRef.current, message: messageRef.current }); + } + }, [userId, mergeDraftKey]); + // The parts file writes `.current` in its onChangeText handlers; the proxies + // hook those writes into the debounced save. + const titleSaveRef = useMemo(() => savingRef(titleRef, saveMergeDraft), [saveMergeDraft]); + const messageSaveRef = useMemo(() => savingRef(messageRef, saveMergeDraft), [saveMergeDraft]); + useDraftFlushOnBackground(userId, mergeDraftKey, true); + // Half detent (~0.5) vs full: hide delete-branch + tighten message so // Merge/Cancel stay above the closed-sheet limit without scrolling. const [scrollViewportHeight, setScrollViewportHeight] = useState(0); @@ -254,6 +313,11 @@ export function PrMergeSheet(props: PrMergeSheetProps) { if (celebrate) { void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); await onRefetch(); + // The merge consumed the draft; clear it before dismissing so it never + // reappears on the next visit. + if (userId) { + void clearDraft(userId, mergeDraftKey); + } // Dismiss exactly this merge route; `onDismiss` (router.back) leaves the // refreshed PR review screen visible. Do NOT also call router.back() // here or it would pop the review screen too. @@ -299,6 +363,15 @@ export function PrMergeSheet(props: PrMergeSheetProps) { // rather than sending a method the repo does not allow. const noMethodsAllowed = methodOptions.length === 0; + // The footer Cancel is an explicit discard: clear the draft and leave. The + // header back (onBack) is a passive dismiss that keeps the draft. + function handleCancel() { + if (userId) { + void clearDraft(userId, mergeDraftKey); + } + onDismiss(); + } + // PickerSheet invariant: [header, ScrollView]; footer is trailing content. return ( <> @@ -314,27 +387,29 @@ export function PrMergeSheet(props: PrMergeSheetProps) { setScrollViewportHeight(event.nativeEvent.layout.height); }} > - + {draft.settled ? ( + + ) : null} ); diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx new file mode 100644 index 0000000000..a48dd872a7 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.test.tsx @@ -0,0 +1,277 @@ +// Clear-rule coverage for the comment composer's durable draft. The composer +// clears its draft on three committed outcomes — comment post, add-to-review, +// and a confirmed discard — and keeps it on a dismissed-without-confirmation +// discard. `Alert.alert` is captured so the test can press the Discard / +// Keep editing buttons the discard gate renders. +// +// The composer is mounted by calling it as a plain function (no renderer), so +// the React hook primitives are stubbed, mirroring pr-merge-sheet.test.tsx. + +import * as React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PrReviewCommentComposer } from './pr-review-comment-composer'; +import { clearDraft } from '@/lib/persist/drafts'; + +type AlertButton = { text?: string; style?: string; onPress?: () => void }; +type AlertCall = { title: string; message: string; buttons: AlertButton[] }; + +const { alertCalls, createCommentMocks } = vi.hoisted(() => ({ + alertCalls: [] as AlertCall[], + createCommentMocks: { + mutateAsync: vi.fn<() => Promise>(), + isPending: false, + error: null as Error | null, + }, +})); + +vi.mock('react', async () => { + const actual = await vi.importActual('react'); + return { + ...actual, + useState: vi.fn( + (initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void] + ), + useMemo: vi.fn((factory: () => T) => factory()), + useRef: vi.fn((initial: T) => { + const ref: React.RefObject = { current: initial }; + return ref; + }), + useEffect: vi.fn((effect: React.EffectCallback) => { + effect(); + }), + useCallback: vi.fn( unknown>(fn: T) => fn), + }; +}); + +vi.mock('react-native', () => ({ + Alert: { + alert: (title: string, message: string, buttons: AlertButton[]) => { + alertCalls.push({ title, message, buttons }); + }, + }, + Keyboard: { addListener: vi.fn(() => ({ remove: vi.fn() })) }, + ScrollView: 'ScrollView', + View: 'View', + TextInput: 'TextInput', + Platform: { OS: 'ios' }, +})); + +vi.mock('expo-crypto', () => ({ + randomUUID: () => 'not-used', +})); + +vi.mock('expo-haptics', () => ({ + impactAsync: vi.fn(), + notificationAsync: vi.fn(), + ImpactFeedbackStyle: { Light: 'Light' }, + NotificationFeedbackType: { Success: 'Success' }, +})); + +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); + +vi.mock('@/components/pr-review/pr-form-sheet-chrome', () => ({ + PrFormSheetHeader: 'PrFormSheetHeader', + useFormSheetKeyboardVisible: () => false, +})); + +vi.mock('@/components/pr-review/composer-inline-error', () => ({ + ComposerInlineError: 'ComposerInlineError', + useComposerInlineError: () => ({ + inlineError: null, + inlineErrorKind: null, + inlineErrorIsLocal: false, + setInlineError: vi.fn(), + setInlineErrorKind: vi.fn(), + setInlineErrorIsLocal: vi.fn(), + clearBadRequestOnBodyEdit: vi.fn(), + }), +})); + +vi.mock('@/components/pr-review/pr-review-comment-composer-parts', () => ({ + CommentBodyField: 'CommentBodyField', + ComposerFooter: 'ComposerFooter', + composerRangeLabel: (line: number, startLine?: number) => + startLine !== undefined && startLine !== line ? `L${startLine}–L${line}` : `L${line}`, + ContextPreview: 'ContextPreview', +})); + +vi.mock('@/components/pr-review/discussion/reply-input', () => ({ + ensureTermsAcceptedOutcome: vi.fn().mockResolvedValue({ kind: 'accepted' as const }), + TERMS_OUTDATED_COPY: 'outdated', +})); + +vi.mock('@/lib/hooks/use-current-user-id', () => ({ + useCurrentUserId: () => ({ userId: 'u1', isLoading: false }), +})); + +vi.mock('@/lib/persist/drafts', () => ({ + saveDraft: vi.fn(), + clearDraft: vi.fn(), + prCommentDraftKey: vi.fn(() => 'pr-comment:key'), + prReplyDraftKey: vi.fn(), + prMergeDraftKey: vi.fn(), +})); + +vi.mock('@/lib/persist/use-draft-load', () => ({ + useFencedDraftLoad: () => ({ settled: true, value: null }), +})); + +vi.mock('@/lib/persist/use-draft-flush', () => ({ + useDraftFlushOnBackground: vi.fn(), +})); + +vi.mock('@/lib/pr-review/build-suggestion-fence', () => ({ + buildSuggestionFence: () => null, +})); + +vi.mock('@/lib/pr-review/diff-selection-bridge', () => ({ + getDiffSelection: () => null, +})); + +vi.mock('@/lib/pr-review/pending-review-provider', () => ({ + usePendingReview: () => ({ + items: [], + addComment: vi.fn(), + updateComment: vi.fn(), + removeComment: vi.fn(), + clear: vi.fn(), + }), +})); + +vi.mock('@/lib/pr-review/use-pr-review-mutations', () => ({ + useCreateReviewCommentMutation: () => ({ + mutateAsync: createCommentMocks.mutateAsync, + isPending: createCommentMocks.isPending, + error: createCommentMocks.error, + }), +})); + +const baseProps = { + owner: 'octocat', + repo: 'hello', + number: 1, + mode: { kind: 'create', headSha: 'a'.repeat(40) } as const, + path: 'src/a.ts', + side: 'RIGHT' as const, + line: 10, + title: 'Comment', + eyebrow: 'octocat/hello#1', + onDismiss: vi.fn(), +}; + +function findByType(node: unknown, type: string): React.ReactElement | null { + if (React.isValidElement(node)) { + const element = node; + if (element.type === type) { + return element; + } + const children = (element.props as Record).children; + if (Array.isArray(children)) { + for (const child of children) { + const found = findByType(child, type); + if (found) { + return found; + } + } + } else if (children !== undefined && children !== null) { + const found = findByType(children, type); + if (found) { + return found; + } + } + } + if (Array.isArray(node)) { + for (const child of node) { + const found = findByType(child, type); + if (found) { + return found; + } + } + } + return null; +} + +function mountComposer(): React.ReactElement { + // eslint-disable-next-line new-cap + return PrReviewCommentComposer(baseProps); +} + +function typeBody(element: React.ReactElement, text: string): void { + const field = findByType(element, 'CommentBodyField'); + if (!field) { + throw new Error('CommentBodyField not found'); + } + (field.props as { onChangeText?: (value: string) => void }).onChangeText?.(text); +} + +function footerProp(element: React.ReactElement, prop: string): (() => void) | undefined { + const footer = findByType(element, 'ComposerFooter'); + if (!footer) { + throw new Error('ComposerFooter not found'); + } + return (footer.props as Record void) | undefined>)[prop]; +} + +async function flushMicrotasks(): Promise { + await new Promise(resolve => { + setTimeout(resolve, 0); + }); +} + +describe('PrReviewCommentComposer draft clear rules', () => { + beforeEach(() => { + alertCalls.length = 0; + createCommentMocks.mutateAsync.mockReset(); + createCommentMocks.isPending = false; + createCommentMocks.error = null; + vi.clearAllMocks(); + }); + + it('clears the draft on add-to-review', () => { + const element = mountComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onAddToReview')?.(); + + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-comment:key'); + }); + + it('clears the draft on a successful comment post', async () => { + createCommentMocks.mutateAsync.mockResolvedValueOnce({}); + const element = mountComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onCommentNow')?.(); + await flushMicrotasks(); + + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-comment:key'); + }); + + it('clears the draft on a confirmed discard', () => { + const element = mountComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onCancel')?.(); + + const call = alertCalls.at(-1); + if (!call) { + throw new Error('No discard Alert was shown'); + } + call.buttons.find(b => b.style === 'destructive')?.onPress?.(); + + expect(clearDraft).toHaveBeenCalledWith('u1', 'pr-comment:key'); + }); + + it('does not clear the draft on a dismissed-without-confirmation discard', () => { + const element = mountComposer(); + typeBody(element, 'hello'); + footerProp(element, 'onCancel')?.(); + + const call = alertCalls.at(-1); + if (!call) { + throw new Error('No discard Alert was shown'); + } + call.buttons.find(b => b.text === 'Keep editing')?.onPress?.(); + + expect(clearDraft).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx index 402884d1f8..591e08f0fa 100644 --- a/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-comment-composer.tsx @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- the comment composer owns the durable comment draft (load, save, seed, clear) beside the existing create/edit form; the draft wiring stays with the form it persists */ // Comment-composer content. Two modes: // - create: Comment now + Add to review + Insert suggestion (needs headSha). // - edit: single Save updating a queued PendingReviewItem (local-only). @@ -29,6 +30,10 @@ import { ensureTermsAcceptedOutcome, TERMS_OUTDATED_COPY, } from '@/components/pr-review/discussion/reply-input'; +import { useCurrentUserId } from '@/lib/hooks/use-current-user-id'; +import { clearDraft, prCommentDraftKey, saveDraft } from '@/lib/persist/drafts'; +import { useDraftFlushOnBackground } from '@/lib/persist/use-draft-flush'; +import { useFencedDraftLoad } from '@/lib/persist/use-draft-load'; import { buildSuggestionFence } from '@/lib/pr-review/build-suggestion-fence'; import { getDiffSelection } from '@/lib/pr-review/diff-selection-bridge'; import { usePendingReview } from '@/lib/pr-review/pending-review-provider'; @@ -73,6 +78,18 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { const createComment = useCreateReviewCommentMutation({ owner, repo, number }); const isEdit = mode.kind === 'edit'; + // Durable comment draft (create mode only). Edit mode edits an already-queued + // item, durable through the pending-review provider, so no draft there. + const { userId, isLoading: isIdentityLoading } = useCurrentUserId(); + const commentDraftKey = prCommentDraftKey(owner, repo, number, path, side, line, startLine); + const draftUserId = isEdit ? undefined : userId; + const draft = useFencedDraftLoad({ + userId: draftUserId, + isIdentityLoading, + entityKey: commentDraftKey, + }); + useDraftFlushOnBackground(draftUserId, commentDraftKey, true); + // Edit mode ignores the bridge so editing A never shows B's path/lines. const selection = isEdit ? null : getDiffSelection({ owner, repo, number }); @@ -82,6 +99,17 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { const bodyInputRef = useRef(null); const scrollRef = useRef(null); const [hasBody, setHasBody] = useState(() => initialBody.trim().length > 0); + // Seed the refs from the settled draft once per identity/destination, before + // the body field mounts (create mode only). Re-seeding on a key change (and + // resetting to the initial body when there is no draft) keeps a reused + // instance from showing or saving the previous account's or position's text. + const draftSeedKeyRef = useRef(null); + const draftSeedKey = `${draftUserId ?? 'anonymous'}\u0000${commentDraftKey}`; + if (!isEdit && draft.settled && draftSeedKeyRef.current !== draftSeedKey) { + draftSeedKeyRef.current = draftSeedKey; + bodyRef.current = draft.value ?? initialBody; + bodyBaselineRef.current = draft.value ?? initialBody; + } const { inlineError, inlineErrorKind, @@ -113,6 +141,9 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { bodyRef.current = value; setHasBody(value.trim().length > 0); clearBadRequestOnBodyEdit(); + if (draftUserId) { + saveDraft(draftUserId, commentDraftKey, value); + } } function handleAddToReview() { @@ -137,6 +168,9 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { body, commitSha: mode.headSha, }); + if (draftUserId) { + void clearDraft(draftUserId, commentDraftKey); + } void Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light); onDismiss(); } @@ -177,6 +211,9 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { ...(startLine !== undefined ? { startLine, startSide: side } : {}), commitSha: mode.headSha, }); + if (draftUserId) { + void clearDraft(draftUserId, commentDraftKey); + } void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); onDismiss(); } catch { @@ -207,7 +244,16 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { if (dirty) { Alert.alert('Discard comment?', 'Your draft will be lost.', [ { text: 'Keep editing', style: 'cancel' }, - { text: 'Discard', style: 'destructive', onPress: onDismiss }, + { + text: 'Discard', + style: 'destructive', + onPress: () => { + if (draftUserId) { + void clearDraft(draftUserId, commentDraftKey); + } + onDismiss(); + }, + }, ]); return; } @@ -225,6 +271,11 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { bodyRef.current = block; setHasBody(block.trim().length > 0); clearBadRequestOnBodyEdit(); + // Persist the inserted suggestion like a typed change, so a process kill + // after Insert (with no later keystroke) does not lose the suggestion. + if (draftUserId) { + saveDraft(draftUserId, commentDraftKey, block); + } bodyInputRef.current?.setNativeProps({ text: block, selection: { start: block.length, end: block.length }, @@ -278,12 +329,14 @@ export function PrReviewCommentComposer(props: PrReviewCommentComposerProps) { /> Comment - + {isEdit || draft.settled ? ( + + ) : null} {showInsertSuggestion ? (