From de958e633282a72e5f1811e53a59b8bc1faffdfa Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:16:13 -0700 Subject: [PATCH] fix(mobile): harden native chat replay boundaries --- .../session/MobileNativeChatMessage.test.ts | 32 ++++++++++++++-- .../src/session/MobileNativeChatMessage.tsx | 11 +++++- .../mobile-native-chat-stream-frame.test.ts | 34 ++++++++++++++++- .../mobile-native-chat-stream-frame.ts | 37 ++++++++++++------- ...use-mobile-native-chat-answer-send.test.ts | 35 ++++++++++++++++++ .../use-mobile-native-chat-answer-send.ts | 17 +++++---- .../use-mobile-native-chat-session.test.ts | 29 +++++++++++++++ 7 files changed, 167 insertions(+), 28 deletions(-) diff --git a/mobile/src/session/MobileNativeChatMessage.test.ts b/mobile/src/session/MobileNativeChatMessage.test.ts index bb687b77ba6..8a0a2582bea 100644 --- a/mobile/src/session/MobileNativeChatMessage.test.ts +++ b/mobile/src/session/MobileNativeChatMessage.test.ts @@ -2,6 +2,7 @@ import { createElement } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { NativeChatMessage } from '../../../src/shared/native-chat-types' +import { MAX_TOOL_RESULT_CHARS } from './mobile-native-chat-message-styles' vi.mock('react-native', async () => { const React = await import('react') @@ -30,7 +31,7 @@ function userMessage(blocks: NativeChatMessage['blocks']): NativeChatMessage { return { id: 'u1', role: 'user', blocks, timestamp: null, source: 'transcript' } } -describe('MobileNativeChatMessage image-ref rendering', () => { +describe('MobileNativeChatMessage', () => { let renderer: ReactTestRenderer | null = null beforeEach(() => { @@ -41,7 +42,10 @@ describe('MobileNativeChatMessage image-ref rendering', () => { renderer = null }) - function render(message: NativeChatMessage): ReactTestRenderer { + function render( + message: NativeChatMessage, + props: { toolsExpanded?: boolean } = {} + ): ReactTestRenderer { const original = console.error const spy = vi.spyOn(console, 'error').mockImplementation((...a) => { if (typeof a[0] === 'string' && a[0].includes('react-test-renderer is deprecated')) { @@ -51,7 +55,7 @@ describe('MobileNativeChatMessage image-ref rendering', () => { }) try { act(() => { - renderer = create(createElement(MobileNativeChatMessage, { message })) + renderer = create(createElement(MobileNativeChatMessage, { message, ...props })) }) } finally { spy.mockRestore() @@ -84,4 +88,26 @@ describe('MobileNativeChatMessage image-ref rendering', () => { .map((node) => String(node.children.join(''))) expect(texts.some((text) => text.includes('/tmp/host.png'))).toBe(true) }) + + it('bounds expanded diff-less tool input before native text layout', () => { + const tree = render( + { + id: 'a1', + role: 'assistant', + blocks: [ + { type: 'tool-call', name: 'CustomTool', input: { payload: 'x'.repeat(100_000) } } + ], + timestamp: null, + source: 'transcript' + }, + { toolsExpanded: true } + ) + + const detail = tree.root + .findAllByType('Text' as never) + .map((node) => String(node.children.join(''))) + .find((text) => text.startsWith('{\n')) + expect(detail).toHaveLength(MAX_TOOL_RESULT_CHARS + 1) + expect(detail?.endsWith('…')).toBe(true) + }) }) diff --git a/mobile/src/session/MobileNativeChatMessage.tsx b/mobile/src/session/MobileNativeChatMessage.tsx index 5d28fc7d207..f2a47e6a56c 100644 --- a/mobile/src/session/MobileNativeChatMessage.tsx +++ b/mobile/src/session/MobileNativeChatMessage.tsx @@ -73,6 +73,13 @@ function ResultBody({ ) } +function formatBoundedToolInput(input: unknown): string { + const detail = formatToolInput(input) + return detail.length > MAX_TOOL_RESULT_CHARS + ? `${detail.slice(0, MAX_TOOL_RESULT_CHARS)}…` + : detail +} + /** One request: a tool call and its result rendered together as a single * expandable line. `defaultExpanded` lets the group toggle open every line. */ function ToolLine({ @@ -135,9 +142,9 @@ function ToolLine({ {callDiff ? : null} {/* Diff-less calls show the full formatted input (pretty JSON), not - the one-line label repeated (desktop parity). */} + the one-line label repeated, bounded like desktop tool detail. */} {!callDiff && call ? ( - {formatToolInput(call.input)} + {formatBoundedToolInput(call.input)} ) : null} {result ? ( diff --git a/mobile/src/session/mobile-native-chat-stream-frame.test.ts b/mobile/src/session/mobile-native-chat-stream-frame.test.ts index ac545a8f311..de189ca42c8 100644 --- a/mobile/src/session/mobile-native-chat-stream-frame.test.ts +++ b/mobile/src/session/mobile-native-chat-stream-frame.test.ts @@ -43,7 +43,12 @@ describe('applyMobileNativeChatStreamFrame', () => { const result = applyMobileNativeChatStreamFrame({ merger, - frame: { type: 'snapshot', messages: [message('b'), message('c'), message('d')] }, + frame: { + type: 'snapshot', + messages: [message('b'), message('c'), message('d')], + hasMore: true, + beforeOffset: 20 + }, limit: 3, replaceSnapshot: false }) @@ -51,7 +56,32 @@ describe('applyMobileNativeChatStreamFrame', () => { expect(result).toMatchObject({ kind: 'messages', messages: [message('b'), message('c'), message('d')], - cursorInvalidated: true + cursorInvalidated: true, + hasMore: true + }) + }) + + it('refreshes paging metadata when a replay still starts at the retained oldest row', () => { + const merger = createNativeChatMerger() + replaceList(merger, [message('a'), message('b')]) + + expect( + applyMobileNativeChatStreamFrame({ + merger, + frame: { + type: 'snapshot', + messages: [message('a'), message('b')], + hasMore: false, + beforeOffset: 0 + }, + limit: 40, + replaceSnapshot: false + }) + ).toEqual({ + kind: 'messages', + messages: [message('a'), message('b')], + hasMore: false, + beforeOffset: 0 }) }) diff --git a/mobile/src/session/mobile-native-chat-stream-frame.ts b/mobile/src/session/mobile-native-chat-stream-frame.ts index c19c5976ff7..bef3dc5ef27 100644 --- a/mobile/src/session/mobile-native-chat-stream-frame.ts +++ b/mobile/src/session/mobile-native-chat-stream-frame.ts @@ -25,19 +25,19 @@ export type AppliedMobileNativeChatFrame = windowReplaced?: boolean } -function replayExtendsRetainedTail( +function replayRetainedTailStart( merger: NativeChatMerger, messages: readonly NativeChatMessage[], hasMore: boolean | undefined -): boolean { +): number | null { const firstIndex = messages[0] ? merger.indexById.get(messages[0].id) : undefined if (firstIndex === undefined) { - return false + return null } // `hasMore: false` is authoritative: retained rows before the replay window // were removed while disconnected, even when the newest IDs still match. if (hasMore === false && firstIndex > 0) { - return false + return null } let expectedIndex = firstIndex let sawNewMessage = false @@ -46,12 +46,12 @@ function replayExtendsRetainedTail( if (existingIndex === undefined) { sawNewMessage = true } else if (sawNewMessage || existingIndex !== expectedIndex) { - return false + return null } else { expectedIndex += 1 } } - return expectedIndex === merger.list.length + return expectedIndex === merger.list.length ? firstIndex : null } /** Applies runtime stream frames while preserving the initial-snapshot versus @@ -79,12 +79,11 @@ export function applyMobileNativeChatStreamFrame(args: { if (!Array.isArray(frame.messages)) { return { kind: 'ignored' } } - const replayExtendsHistory = - frame.type === 'snapshot' && - !replaceSnapshot && - merger.list.length > 0 && - replayExtendsRetainedTail(merger, frame.messages, frame.hasMore) - if (frame.type === 'replacement' || (frame.type === 'snapshot' && !replayExtendsHistory)) { + const replayStartIndex = + frame.type === 'snapshot' && !replaceSnapshot && merger.list.length > 0 + ? replayRetainedTailStart(merger, frame.messages, frame.hasMore) + : null + if (frame.type === 'replacement' || (frame.type === 'snapshot' && replayStartIndex === null)) { replaceList(merger, frame.messages) return { kind: 'messages', @@ -96,11 +95,23 @@ export function applyMobileNativeChatStreamFrame(args: { } const previousFirstId = merger.list[0]?.id const messages = applyAppend(merger, frame.messages, limit) + const cursorInvalidated = Boolean(previousFirstId && messages[0]?.id !== previousFirstId) + const replayStillStartsAtOldest = frame.type === 'snapshot' && replayStartIndex === 0 return { kind: 'messages', messages, // Why: once the bounded live window drops its oldest row, the snapshot's // byte cursor no longer describes the oldest retained message. - ...(previousFirstId && messages[0]?.id !== previousFirstId ? { cursorInvalidated: true } : {}) + ...(cursorInvalidated ? { cursorInvalidated: true } : {}), + // A trimmed replay creates page-able history even if the prior window had + // none; otherwise only a replay sharing our oldest row owns its metadata. + ...(frame.type === 'snapshot' && cursorInvalidated + ? { hasMore: true } + : replayStillStartsAtOldest + ? { + ...(frame.hasMore == null ? {} : { hasMore: frame.hasMore }), + ...(frame.beforeOffset == null ? {} : { beforeOffset: frame.beforeOffset }) + } + : {}) } } diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts index f3eabf1d417..1c35ce88508 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.test.ts @@ -491,4 +491,39 @@ describe('useMobileNativeChatAnswerSend', () => { await expect(second).resolves.toBe(true) expect(sendRequest).toHaveBeenCalledTimes(2) }) + + it('does not write a successor after the prior delivery becomes ambiguous', async () => { + let rejectFirst: (error: Error) => void = () => undefined + const onSendError = vi.fn() + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirst = reject + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, onSendError) + + let first: Promise | undefined + let second: Promise | undefined + await act(async () => { + first = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [0] }]) + await Promise.resolve() + }) + await act(async () => { + second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) + await Promise.resolve() + }) + await act(async () => { + rejectFirst(markRpcDeliveryUnknown(new Error('Connection closed'))) + await Promise.resolve() + }) + + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer unconfirmed — check chat before retrying') + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.ts b/mobile/src/session/use-mobile-native-chat-answer-send.ts index 6b8b38a804d..105096f8408 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.ts @@ -80,8 +80,8 @@ export function useMobileNativeChatAnswerSend(args: { // superseding answer inherits the cancelled chain's hold (it re-enters before // the old chain unwinds), and only the last chain out releases the lock. const writeHoldsRef = useRef(new Map()) - // Successors wait for the prior chain's active RPC before sharing its hold. - const writeTurnsRef = useRef(new Map>()) + // Successors wait for the prior RPC and inherit any delivery ambiguity. + const writeTurnsRef = useRef(new Map>()) const delaysRef = useRef< Set<{ timer: ReturnType; resolve: (completed: boolean) => void }> >(new Set()) @@ -124,9 +124,9 @@ export function useMobileNativeChatAnswerSend(args: { return false } holds.set(handle, heldCount + 1) - const previousTurn = writeTurnsRef.current.get(handle) ?? Promise.resolve() - let finishTurn: () => void = () => undefined - const turn = new Promise((resolve) => { + const previousTurn = writeTurnsRef.current.get(handle) ?? Promise.resolve(true) + let finishTurn: (safeToContinue: boolean) => void = () => undefined + const turn = new Promise((resolve) => { finishTurn = resolve }) writeTurnsRef.current.set(handle, turn) @@ -135,9 +135,10 @@ export function useMobileNativeChatAnswerSend(args: { const generation = generationRef.current let sawUnknownOutcome = false let sawAcceptedGroup = false + let predecessorSafe = true try { - await previousTurn - if (generationRef.current !== generation) { + predecessorSafe = await previousTurn + if (!predecessorSafe || generationRef.current !== generation) { return false } // One budget for the whole answer instead of a fresh timeout per keystroke @@ -262,7 +263,7 @@ export function useMobileNativeChatAnswerSend(args: { } return groups.length > 0 } finally { - finishTurn() + finishTurn(predecessorSafe && !sawUnknownOutcome) if (writeTurnsRef.current.get(handle) === turn) { writeTurnsRef.current.delete(handle) } diff --git a/mobile/src/session/use-mobile-native-chat-session.test.ts b/mobile/src/session/use-mobile-native-chat-session.test.ts index f16aa00a8fc..e4573111835 100644 --- a/mobile/src/session/use-mobile-native-chat-session.test.ts +++ b/mobile/src/session/use-mobile-native-chat-session.test.ts @@ -222,6 +222,35 @@ describe('useMobileNativeChatSession', () => { expect(state?.messages.at(-1)?.id).toBe('live-1') }) + it('enables paging when a replay trims a window that previously had no earlier rows', async () => { + const sendRequest = vi.fn() + const window = Array.from({ length: 40 }, (_unused, index) => message(`win-${index}`)) + const subscribe: RpcClient['subscribe'] = vi.fn((_method, _params, onData) => { + emit = onData + onData({ type: 'snapshot', messages: window, hasMore: false, beforeOffset: 0 }) + return () => {} + }) + await mount({ sendRequest, subscribe } as unknown as RpcClient) + + await act(async () => + emit({ + type: 'snapshot', + messages: [...window.slice(1), message('live-1')], + hasMore: true, + beforeOffset: 10 + }) + ) + + expect(state?.messages[0]?.id).toBe('win-1') + expect(state?.hasMore).toBe(true) + act(() => state?.loadEarlier()) + expect(sendRequest).toHaveBeenCalledWith('nativeChat.readSession', { + agent: 'claude', + sessionId: 'session', + limit: 100 + }) + }) + it('drops paged rows that an authoritative replay says were removed', async () => { const sendRequest = vi.fn().mockResolvedValue({ ok: true,