From 693c0b94a75efa70939e7d4783933adacffd0b23 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:34:22 -0700 Subject: [PATCH] fix(mobile): harden native chat edge cases --- .../src/session/MobileNativeChatView.test.ts | 70 +++++++++++++++-- mobile/src/session/MobileNativeChatView.tsx | 14 +++- ...use-mobile-native-chat-answer-send.test.ts | 53 +++++++++++-- .../use-mobile-native-chat-answer-send.ts | 4 +- .../use-mobile-native-chat-session.test.ts | 29 +++++++ .../session/use-mobile-native-chat-session.ts | 6 +- src/shared/native-chat-tool-summary.test.ts | 14 +++- src/shared/native-chat-tool-summary.ts | 75 +++++++++++++------ 8 files changed, 222 insertions(+), 43 deletions(-) diff --git a/mobile/src/session/MobileNativeChatView.test.ts b/mobile/src/session/MobileNativeChatView.test.ts index 8ef8646be99..0e7b0d244a3 100644 --- a/mobile/src/session/MobileNativeChatView.test.ts +++ b/mobile/src/session/MobileNativeChatView.test.ts @@ -3,14 +3,22 @@ import { act, create, type ReactTestInstance, type ReactTestRenderer } from 'rea import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { MobileNativeChatView } from './MobileNativeChatView' -vi.mock('react-native', () => ({ - ActivityIndicator: 'ActivityIndicator', - FlatList: 'FlatList', - Pressable: 'Pressable', - StyleSheet: { create: (styles: unknown) => styles, hairlineWidth: 1 }, - Text: 'Text', - View: 'View' -})) +const { scrollToEnd } = vi.hoisted(() => ({ scrollToEnd: vi.fn() })) + +vi.mock('react-native', async () => { + const React = await import('react') + return { + ActivityIndicator: 'ActivityIndicator', + FlatList: React.forwardRef((props: object, ref) => { + React.useImperativeHandle(ref, () => ({ scrollToEnd })) + return React.createElement('FlatList', props) + }), + Pressable: 'Pressable', + StyleSheet: { create: (styles: unknown) => styles, hairlineWidth: 1 }, + Text: 'Text', + View: 'View' + } +}) vi.mock('react-native-safe-area-context', () => ({ useSafeAreaInsets: () => ({ top: 0, bottom: 0, left: 0, right: 0 }) @@ -58,6 +66,7 @@ vi.mock('./MobileNativeChatComposer', async () => { }) type Overrides = { + messages?: Parameters[0]['messages'] sendErrorMessage?: string | null onClearSendError?: () => void inputLockReason?: 'disconnected' | 'waiting' | null @@ -84,6 +93,7 @@ describe('MobileNativeChatView', () => { beforeEach(() => { globalThis.IS_REACT_ACT_ENVIRONMENT = true + scrollToEnd.mockClear() }) afterEach(() => { @@ -113,6 +123,23 @@ describe('MobileNativeChatView', () => { } } + async function update(overrides: Overrides = {}): Promise { + await act(async () => { + renderer?.update( + createElement(MobileNativeChatView, { + messages: [], + status: 'ready', + streamIdentity: 'test-stream', + onSend: vi.fn().mockResolvedValue(true), + pending: [], + composerText: '', + onComposerTextChange: vi.fn(), + ...overrides + }) + ) + }) + } + function banners(): ReactTestInstance[] { return renderer!.root.findAll((node) => node.props.accessibilityRole === 'alert') } @@ -190,4 +217,31 @@ describe('MobileNativeChatView', () => { await act(async () => list.props.ListHeaderComponent.props.onLoadEarlier()) expect(onLoadEarlier).toHaveBeenCalledOnce() }) + + it('does not defeat prepend anchoring when a short list was also at bottom', async () => { + vi.useFakeTimers() + const current = { + id: 'current', + role: 'assistant' as const, + blocks: [{ type: 'text' as const, text: 'current' }], + timestamp: 0, + source: 'transcript' as const + } + const older = { ...current, id: 'older', blocks: [{ type: 'text' as const, text: 'older' }] } + try { + await render({ messages: [current], hasMore: true }) + await act(async () => vi.runAllTimersAsync()) + scrollToEnd.mockClear() + + await update({ messages: [current], hasMore: true, loadingEarlier: true }) + await update({ messages: [older, current], hasMore: false, loadingEarlier: false }) + const list = renderer!.root.find((node) => node.type === 'FlatList') + act(() => list.props.onContentSizeChange()) + await act(async () => vi.runAllTimersAsync()) + + expect(scrollToEnd).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) }) diff --git a/mobile/src/session/MobileNativeChatView.tsx b/mobile/src/session/MobileNativeChatView.tsx index 2f39f8664c6..96dea80efd5 100644 --- a/mobile/src/session/MobileNativeChatView.tsx +++ b/mobile/src/session/MobileNativeChatView.tsx @@ -161,6 +161,11 @@ export function MobileNativeChatView({ const bottomPad = keyboardInset > 0 ? keyboardInset + insets.bottom : insets.bottom const [atBottom, setAtBottom] = useState(true) const sendScrollTimerRef = useRef | null>(null) + const keepHistoryPositionRef = useRef(Boolean(loadingEarlier)) + if (loadingEarlier) { + keepHistoryPositionRef.current = true + } + const keepHistoryPosition = keepHistoryPositionRef.current const { fontScale, pinchGesture } = useMobileNativeChatPinchGesture() useEffect( () => () => { @@ -191,12 +196,17 @@ export function MobileNativeChatView({ // we don't yank the user away while they read history. (Also fires on keyboard // close, which is harmless while atBottom.) useEffect(() => { - if (data.length === 0 || !atBottom) { + if (data.length === 0 || !atBottom || keepHistoryPositionRef.current) { return } const t = setTimeout(() => listRef.current?.scrollToEnd({ animated: true }), 60) return () => clearTimeout(t) }, [data.length, atBottom, keyboardInset]) + useEffect(() => { + if (!loadingEarlier) { + keepHistoryPositionRef.current = false + } + }, [loadingEarlier]) const handleSend = useCallback( async (text: string): Promise => { @@ -297,7 +307,7 @@ export function MobileNativeChatView({ // prepend grows the list upward instead of jumping the reader. maintainVisibleContentPosition={{ minIndexForVisible: 0 }} onContentSizeChange={() => { - if (data.length > 0 && atBottom) { + if (data.length > 0 && atBottom && !keepHistoryPosition) { listRef.current?.scrollToEnd({ animated: false }) } }} 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 1c35ce88508..8cc510fc042 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 @@ -423,9 +423,10 @@ describe('useMobileNativeChatAnswerSend', () => { await expect(answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }])).resolves.toBe(true) }) - it('lets a superseding answer inherit the cancelled chain’s lock and releases it last', async () => { + it('fences a superseding answer after the cancelled chain moved the selector', async () => { + const onSendError = vi.fn() const sendRequest = vi.fn().mockResolvedValue(acceptedResponse()) - await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + await mount({ sendRequest } as unknown as RpcClient, onSendError) const prompt: AskPrompt = { questions: [ @@ -438,22 +439,24 @@ describe('useMobileNativeChatAnswerSend', () => { await act(async () => { first = answerSend?.answerAsk(prompt, [{ indices: [0] }, { indices: [0] }]) }) - // The user changes their mind mid-pacing: the new answer must not be - // rejected by the lock its own predecessor still holds. + // The first digit already advanced Claude to q2. Replaying a from-q1 key + // plan now would answer the wrong question. await act(async () => { second = answerSend?.answerAsk(TABS_OR_SPACES, [{ indices: [1] }]) }) await act(async () => vi.runAllTimersAsync()) await expect(first).resolves.toBe(false) - await expect(second).resolves.toBe(true) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + expect(onSendError).toHaveBeenCalledWith('Answer partly sent — check chat before retrying') // Both chains unwound: the terminal is free for the next composed write. expect(acquireMobileNativeChatTerminalWrite('terminal')).toBe(true) releaseMobileNativeChatTerminalWrite('terminal') }) - it('queues a superseding answer behind an in-flight terminal write', async () => { - let resolveFirst: (response: ReturnType) => void = () => undefined + it('does not write a successor after the prior in-flight key is accepted', async () => { + let resolveFirst: (response: unknown) => void = () => undefined const sendRequest = vi .fn() .mockImplementationOnce( @@ -487,6 +490,42 @@ describe('useMobileNativeChatAnswerSend', () => { resolveFirst(acceptedResponse()) await Promise.resolve() }) + await expect(first).resolves.toBe(false) + await expect(second).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + }) + + it('lets a queued successor continue after the prior key is definitely rejected', async () => { + let resolveFirst: (response: unknown) => void = () => undefined + const sendRequest = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = resolve + }) + ) + .mockResolvedValue(acceptedResponse()) + await mount({ sendRequest } as unknown as RpcClient, vi.fn()) + + 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 () => { + resolveFirst({ + ...acceptedResponse(), + result: { send: { accepted: false } } + }) + await Promise.resolve() + }) + await expect(first).resolves.toBe(false) await expect(second).resolves.toBe(true) expect(sendRequest).toHaveBeenCalledTimes(2) diff --git a/mobile/src/session/use-mobile-native-chat-answer-send.ts b/mobile/src/session/use-mobile-native-chat-answer-send.ts index 105096f8408..7612e71f40d 100644 --- a/mobile/src/session/use-mobile-native-chat-answer-send.ts +++ b/mobile/src/session/use-mobile-native-chat-answer-send.ts @@ -263,7 +263,9 @@ export function useMobileNativeChatAnswerSend(args: { } return groups.length > 0 } finally { - finishTurn(predecessorSafe && !sawUnknownOutcome) + // Any accepted key changed the live selector, so a queued replacement + // cannot safely apply its from-scratch key plan to that new position. + finishTurn(predecessorSafe && !sawUnknownOutcome && !sawAcceptedGroup) 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 e4573111835..2ff3b05a2b4 100644 --- a/mobile/src/session/use-mobile-native-chat-session.test.ts +++ b/mobile/src/session/use-mobile-native-chat-session.test.ts @@ -281,6 +281,35 @@ describe('useMobileNativeChatSession', () => { expect(state?.hasMore).toBe(false) }) + it('clears a stale cursor when a replacement omits paging metadata', async () => { + const sendRequest = vi.fn() + const subscribe: RpcClient['subscribe'] = vi.fn((_method, _params, onData) => { + emit = onData + onData({ + type: 'snapshot', + messages: Array.from({ length: 40 }, (_unused, index) => message(`win-${index}`)), + hasMore: true, + beforeOffset: 100 + }) + return () => {} + }) + await mount({ sendRequest, subscribe } as unknown as RpcClient) + + await act(async () => + emit({ + type: 'replacement', + messages: Array.from({ length: 40 }, (_unused, index) => message(`new-${index}`)) + }) + ) + act(() => state?.loadEarlier()) + + expect(sendRequest).toHaveBeenCalledWith('nativeChat.readSession', { + agent: 'claude', + sessionId: 'session', + limit: 100 + }) + }) + it('surfaces a failed older-history page and clears it on a retry', async () => { const sendRequest = vi .fn() diff --git a/mobile/src/session/use-mobile-native-chat-session.ts b/mobile/src/session/use-mobile-native-chat-session.ts index 0176d3f962a..3c05aef8ad7 100644 --- a/mobile/src/session/use-mobile-native-chat-session.ts +++ b/mobile/src/session/use-mobile-native-chat-session.ts @@ -187,12 +187,14 @@ export function useMobileNativeChatSession(args: { // Only a genuinely fresh window resets the grown read window — an // overlapping reconnect replay keeps the paged-in history and limit. limitRef.current = INITIAL_LIMIT + beforeOffsetRef.current = applied.beforeOffset ?? null + setHasMore(applied.hasMore ?? applied.messages.length >= INITIAL_LIMIT) } setMessages(applied.messages) - if (applied.hasMore != null) { + if (!applied.windowReplaced && applied.hasMore != null) { setHasMore(applied.hasMore) } - if (applied.beforeOffset != null) { + if (!applied.windowReplaced && applied.beforeOffset != null) { beforeOffsetRef.current = applied.beforeOffset } if (applied.cursorInvalidated) { diff --git a/src/shared/native-chat-tool-summary.test.ts b/src/shared/native-chat-tool-summary.test.ts index 23d55cf9d09..bd8def95bb4 100644 --- a/src/shared/native-chat-tool-summary.test.ts +++ b/src/shared/native-chat-tool-summary.test.ts @@ -3,8 +3,10 @@ import type { NativeChatBlock } from './native-chat-types' import { briefToolArg, describeToolInput, + formatToolInput, summarizeToolInput, - summarizeToolRun + summarizeToolRun, + toolFilePath } from './native-chat-tool-summary' describe('describeToolInput', () => { @@ -20,6 +22,16 @@ describe('describeToolInput', () => { expect(describeToolInput({ pattern: 'foo.*bar', glob: '*.ts' })).toBe('foo.*bar') }) + it('normalizes Codex JSON-string arguments for labels, details, and file links', () => { + expect(describeToolInput('{"cmd":"git status --short"}')).toBe('git status --short') + expect(formatToolInput('{"cmd":"git status --short"}')).toBe( + '{\n "cmd": "git status --short"\n}' + ) + expect(toolFilePath('{"file_path":"src/index.ts"}')).toBe('src/index.ts') + expect(briefToolArg('{"cmd":"git status --short"}')).toBe('git status --short') + expect(describeToolInput('{"command":["bash","-lc","make"]}')).toBe('bash -lc make') + }) + it('falls back to the bounded JSON preview for other shapes', () => { expect(describeToolInput({ todos: [{ id: 1 }] })).toBe( summarizeToolInput({ todos: [{ id: 1 }] }) diff --git a/src/shared/native-chat-tool-summary.ts b/src/shared/native-chat-tool-summary.ts index 9bd4cffd2d1..e4f34152941 100644 --- a/src/shared/native-chat-tool-summary.ts +++ b/src/shared/native-chat-tool-summary.ts @@ -20,64 +20,95 @@ function truncatePreview(collapsed: string): string { * argument (command/query/…), else the bounded JSON preview. Keeps raw * `{"file_path":…}` JSON out of the tappable row label. */ export function describeToolInput(input: unknown): string { - const path = toolFilePath(input) + const normalized = normalizeToolInput(input) + const path = toolFilePath(normalized) if (path) { return truncatePreview(path) } - if (input && typeof input === 'object') { - const value = input as Record + if (normalized && typeof normalized === 'object') { + const value = normalized as Record const primary = value.command ?? value.cmd ?? value.query ?? value.pattern ?? value.description ?? value.url - if (typeof primary === 'string' && primary.trim()) { - return summarizeToolInput(primary) + const primarySummary = summarizePrimaryToolArg(primary) + if (primarySummary) { + return primarySummary } } - return summarizeToolInput(input) + return summarizeToolInput(normalized) } -/** Full, pretty-printed tool-call input for the expanded detail view. Strings - * pass through as-is; objects/arrays print as indented JSON so a diff-less call - * (e.g. a question payload) reads cleanly instead of one long minified line. */ +/** Full, pretty-printed tool-call input for the expanded detail view. Structured + * JSON strings and objects print as indented JSON; other strings pass through. */ export function formatToolInput(input: unknown): string { - if (input === null || input === undefined) { + const normalized = normalizeToolInput(input) + if (normalized === null || normalized === undefined) { return '' } - if (typeof input === 'string') { - return input + if (typeof normalized === 'string') { + return normalized } - if (typeof input === 'number' || typeof input === 'boolean') { - return String(input) + if (typeof normalized === 'number' || typeof normalized === 'boolean') { + return String(normalized) } try { - return JSON.stringify(input, null, 2) ?? '' + return JSON.stringify(normalized, null, 2) ?? '' } catch { return '' } } export function toolFilePath(input: unknown): string | null { - if (!input || typeof input !== 'object') { + const normalized = normalizeToolInput(input) + if (!normalized || typeof normalized !== 'object') { return null } - const value = input as Record + const value = normalized as Record const path = value.file_path ?? value.filePath ?? value.path ?? value.notebook_path return typeof path === 'string' && path.length > 0 ? path : null } export function briefToolArg(input: unknown): string { - if (input && typeof input === 'object') { - const value = input as Record + const normalized = normalizeToolInput(input) + if (normalized && typeof normalized === 'object') { + const value = normalized as Record const path = value.file_path ?? value.filePath ?? value.path ?? value.notebook_path if (typeof path === 'string' && path.length > 0) { const parts = path.split(/[\\/]/).filter(Boolean) return parts.at(-1) ?? path } const command = value.command ?? value.cmd ?? value.query ?? value.pattern - if (typeof command === 'string') { - return summarizeToolInput(command).slice(0, 28) + const commandSummary = summarizePrimaryToolArg(command) + if (commandSummary) { + return commandSummary.slice(0, 28) } } - return summarizeToolInput(input).slice(0, 28) + return summarizeToolInput(normalized).slice(0, 28) +} + +function normalizeToolInput(input: unknown): unknown { + if (typeof input !== 'string') { + return input + } + const first = input.trimStart()[0] + if (first !== '{' && first !== '[') { + return input + } + try { + const parsed: unknown = JSON.parse(input) + return parsed !== null && typeof parsed === 'object' ? parsed : input + } catch { + return input + } +} + +function summarizePrimaryToolArg(input: unknown): string | null { + if (typeof input === 'string' && input.trim()) { + return summarizeToolInput(input) + } + if (Array.isArray(input) && input.length > 0 && input.every((part) => typeof part === 'string')) { + return summarizeToolInput(input.join(' ')) + } + return null } export function summarizeToolRun(blocks: readonly NativeChatBlock[]): string {