From ac34bdacf8cc0165ba3cc75b27418a9e85efbf1d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:54:31 -0700 Subject: [PATCH] fix(mobile): keep image attachments scoped to their chat --- .../session/MobileNativeChatComposer.test.ts | 52 ++++++++++++- .../use-mobile-native-chat-image-send.test.ts | 40 +++++++++- .../use-mobile-native-chat-image-send.ts | 18 +++++ ...-mobile-native-chat-pending-images.test.ts | 77 +++++++++++++++++++ .../use-mobile-native-chat-pending-images.ts | 64 +++++++++++---- 5 files changed, 234 insertions(+), 17 deletions(-) diff --git a/mobile/src/session/MobileNativeChatComposer.test.ts b/mobile/src/session/MobileNativeChatComposer.test.ts index ef2c1012603..26de69e0fe9 100644 --- a/mobile/src/session/MobileNativeChatComposer.test.ts +++ b/mobile/src/session/MobileNativeChatComposer.test.ts @@ -7,6 +7,7 @@ vi.mock('react-native', async () => { const React = await import('react') return { ActivityIndicator: 'ActivityIndicator', + Image: 'Image', Pressable: 'Pressable', ScrollView: ({ children, ...props }: { children?: unknown }) => React.createElement('ScrollView', props, children), @@ -24,7 +25,8 @@ vi.mock('lucide-react-native', () => ({ ArrowUp: 'ArrowUp', ImagePlus: 'ImagePlus', Mic: 'Mic', - Square: 'Square' + Square: 'Square', + X: 'X' })) function suppressRendererWarning(): () => void { @@ -112,6 +114,54 @@ describe('MobileNativeChatComposer', () => { expect(onSend).not.toHaveBeenCalled() }) + it('allows an image-only send once its upload is ready', async () => { + const onSend = vi.fn().mockResolvedValue(true) + const restore = suppressRendererWarning() + try { + await act(async () => { + renderer = create( + createElement(MobileNativeChatComposer, { + value: '', + onChangeText: vi.fn(), + onSend, + pendingImages: [{ id: 'image-1', thumbnailUri: 'file:///image.png', status: 'ready' }] + }) + ) + }) + } finally { + restore() + } + + expect(sendButton().props).toMatchObject({ disabled: false }) + await act(async () => sendButton().props.onPress()) + expect(onSend).toHaveBeenCalledWith('') + }) + + it('blocks send while any pending image is still uploading', async () => { + const onSend = vi.fn().mockResolvedValue(true) + const restore = suppressRendererWarning() + try { + await act(async () => { + renderer = create( + createElement(MobileNativeChatComposer, { + value: 'caption', + onChangeText: vi.fn(), + onSend, + pendingImages: [ + { id: 'image-1', thumbnailUri: 'file:///image.png', status: 'uploading' } + ] + }) + ) + }) + } finally { + restore() + } + + expect(sendButton().props).toMatchObject({ disabled: true }) + await act(async () => sendButton().props.onPress()) + expect(onSend).not.toHaveBeenCalled() + }) + it('moves the caret to the insert point after an autocomplete pick, then releases control', async () => { const restore = suppressRendererWarning() try { diff --git a/mobile/src/session/use-mobile-native-chat-image-send.test.ts b/mobile/src/session/use-mobile-native-chat-image-send.test.ts index 6c83df21fd8..8d042e54076 100644 --- a/mobile/src/session/use-mobile-native-chat-image-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-image-send.test.ts @@ -13,6 +13,7 @@ const onSendError = vi.fn() let readyImages: readonly MobileNativeChatReadyImage[] = [] let leaseReady = true +let activeHandle = 'term-1' describe('useMobileNativeChatImageSend', () => { let renderer: ReactTestRenderer | null = null @@ -23,6 +24,7 @@ describe('useMobileNativeChatImageSend', () => { vi.clearAllMocks() readyImages = [] leaseReady = true + activeHandle = 'term-1' sendRequest.mockResolvedValue({ ok: true, result: { send: { accepted: true } } }) sendText.mockResolvedValue(true) }) @@ -36,7 +38,11 @@ describe('useMobileNativeChatImageSend', () => { function Harness(): null { send = useMobileNativeChatImageSend({ clientRef: { current: client }, - activeHandleRef: { current: 'term-1' }, + activeHandleRef: { + get current() { + return activeHandle + } + }, deviceTokenRef: { current: 'device-9' }, inputLeaseReadyRef: { get current() { @@ -130,4 +136,36 @@ describe('useMobileNativeChatImageSend', () => { expect(consumeImages).not.toHaveBeenCalled() expect(onSendError).toHaveBeenCalledWith('Message not sent (disconnected)') }) + + it('does not send the caption to a terminal selected while image paths were being pasted', async () => { + readyImages = [{ id: 'a', hostPath: '/tmp/a.png' }] + sendRequest.mockImplementation(async () => { + activeHandle = 'term-2' + return { ok: true, result: { send: { accepted: true } } } + }) + render() + + await expect(send!('caption')).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + expect(consumeImages).toHaveBeenCalledWith(['a']) + expect(sendText).not.toHaveBeenCalled() + expect(onSendError).toHaveBeenCalledWith('Message not sent (disconnected)') + }) + + it('does not continue pasting images after the target terminal changes', async () => { + readyImages = [ + { id: 'a', hostPath: '/tmp/a.png' }, + { id: 'b', hostPath: '/tmp/b.png' } + ] + sendRequest.mockImplementation(async () => { + activeHandle = 'term-2' + return { ok: true, result: { send: { accepted: true } } } + }) + render() + + await expect(send!('caption')).resolves.toBe(false) + expect(sendRequest).toHaveBeenCalledTimes(1) + expect(consumeImages.mock.calls).toEqual([[['a']]]) + expect(sendText).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-image-send.ts b/mobile/src/session/use-mobile-native-chat-image-send.ts index f69d61f6cd7..5d2bfbbf0ae 100644 --- a/mobile/src/session/use-mobile-native-chat-image-send.ts +++ b/mobile/src/session/use-mobile-native-chat-image-send.ts @@ -46,10 +46,18 @@ export function useMobileNativeChatImageSend({ onSendError('Message not sent (disconnected)') return false } + const targetIsCurrent = (): boolean => + clientRef.current === client && + activeHandleRef.current === terminal && + inputLeaseReadyRef.current const mobileClient = deviceTokenRef.current ? { id: deviceTokenRef.current, type: 'mobile' as const } : undefined for (const image of images) { + if (!targetIsCurrent()) { + onSendError('Message not sent (disconnected)') + return false + } const accepted = await sendMobileNativeChatMessage({ client, terminal, @@ -66,9 +74,19 @@ export function useMobileNativeChatImageSend({ consumeImages([image.id]) } if (text.trim().length > 0) { + // Why: image RPCs can outlive a tab switch; never send the caption to a + // different terminal than the one that received the image paths. + if (!targetIsCurrent()) { + onSendError('Message not sent (disconnected)') + return false + } return sendText(text) } // Image-only send: the paths are in the TUI input; submit with Enter. + if (!targetIsCurrent()) { + onSendError('Message not sent (disconnected)') + return false + } const submitted = await sendMobileNativeChatMessage({ client, terminal, diff --git a/mobile/src/session/use-mobile-native-chat-pending-images.test.ts b/mobile/src/session/use-mobile-native-chat-pending-images.test.ts index ebd13d6b204..78aaa1e126c 100644 --- a/mobile/src/session/use-mobile-native-chat-pending-images.test.ts +++ b/mobile/src/session/use-mobile-native-chat-pending-images.test.ts @@ -168,4 +168,81 @@ describe('useMobileNativeChatPendingImages', () => { expect(state!.pendingChatImages).toEqual([]) expect(state!.getReadyChatImages()).toEqual([]) }) + + it('does not attach a picker result to a terminal selected while the picker was open', async () => { + let resolvePick: (image: { base64: string; uri: string }) => void = () => {} + pickMobileImage.mockReturnValue( + new Promise((resolve) => { + resolvePick = resolve + }) + ) + render('t1') + + let attachDone: Promise = Promise.resolve() + act(() => { + attachDone = state!.attachPendingChatImage('library') + renderer!.update(createElement(Harness, { activeHandle: 't2' })) + }) + await act(async () => { + resolvePick({ base64: 'AAAA', uri: 'file:///a.jpg' }) + await attachDone + }) + + expect(state!.pendingChatImages).toEqual([]) + expect(saveMobileClipboardImageAsTempFile).not.toHaveBeenCalled() + expect(onSuccess).not.toHaveBeenCalled() + }) + + it('does not report upload success after the pending image was removed', async () => { + pickMobileImage.mockResolvedValue({ base64: 'AAAA', uri: 'file:///a.jpg' }) + let resolveUpload: (path: string) => void = () => {} + saveMobileClipboardImageAsTempFile.mockReturnValue( + new Promise((resolve) => { + resolveUpload = resolve + }) + ) + render() + + let attachDone: Promise = Promise.resolve() + await act(async () => { + attachDone = state!.attachPendingChatImage('library') + await Promise.resolve() + await Promise.resolve() + }) + act(() => state!.removePendingChatImage('chat-image-1')) + await act(async () => { + resolveUpload('/tmp/a.png') + await attachDone + }) + + expect(state!.pendingChatImages).toEqual([]) + expect(onSuccess).not.toHaveBeenCalled() + }) + + it('releases pending state and ignores upload completion after unmount', async () => { + pickMobileImage.mockResolvedValue({ base64: 'AAAA', uri: 'file:///a.jpg' }) + let resolveUpload: (path: string) => void = () => {} + saveMobileClipboardImageAsTempFile.mockReturnValue( + new Promise((resolve) => { + resolveUpload = resolve + }) + ) + render() + + let attachDone: Promise = Promise.resolve() + await act(async () => { + attachDone = state!.attachPendingChatImage('library') + await Promise.resolve() + await Promise.resolve() + }) + act(() => renderer!.unmount()) + renderer = null + await act(async () => { + resolveUpload('/tmp/a.png') + await attachDone + }) + + expect(onSuccess).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/session/use-mobile-native-chat-pending-images.ts b/mobile/src/session/use-mobile-native-chat-pending-images.ts index b862057338f..7b330d87514 100644 --- a/mobile/src/session/use-mobile-native-chat-pending-images.ts +++ b/mobile/src/session/use-mobile-native-chat-pending-images.ts @@ -1,4 +1,4 @@ -import { useCallback, useRef, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import type { RpcClient } from '../transport/rpc-client' import type { ConnectionState } from '../transport/types' import { saveMobileClipboardImageAsTempFile } from './mobile-clipboard-image' @@ -58,14 +58,26 @@ export function useMobileNativeChatPendingImages({ const [entries, setEntries] = useState([]) const entriesRef = useRef(entries) const idCounter = useRef(0) + const mountedRef = useRef(false) + + useEffect(() => { + mountedRef.current = true + return () => { + mountedRef.current = false + // Why: release thumbnail data captured by in-flight uploads instead of retaining it until RPC timeout. + entriesRef.current = [] + } + }, []) const updateEntries = useCallback( (updater: (previous: readonly PendingImageEntry[]) => readonly PendingImageEntry[]) => { - setEntries((previous) => { - const next = updater(previous) - entriesRef.current = next - return next - }) + const previous = entriesRef.current + const next = updater(previous) + if (next === previous) { + return + } + entriesRef.current = next + setEntries(next) }, [] ) @@ -87,11 +99,12 @@ export function useMobileNativeChatPendingImages({ if (!client || !activeHandle || !canAttach) { return } + const targetHandle = activeHandle let entryId: string | null = null try { const picked = await pickMobileImage(source) // Cancelled picker: no entry, no toast. - if (!picked) { + if (!picked || !mountedRef.current || lastHandle.current !== targetHandle) { return } idCounter.current += 1 @@ -109,18 +122,39 @@ export function useMobileNativeChatPendingImages({ } ]) const connectionId = await getConnectionId() + if (!mountedRef.current || lastHandle.current !== targetHandle) { + return + } const hostPath = await saveMobileClipboardImageAsTempFile(client, picked.base64, { connectionId }) - // A handle switch mid-upload cleared the entry; the orphaned host temp - // file is covered by the upload TTL. - updateEntries((previous) => - previous.map((entry) => - entry.id === id ? { ...entry, status: 'ready' as const, hostPath } : entry - ) - ) - onSuccess() + if (!mountedRef.current || lastHandle.current !== targetHandle) { + return + } + let completed = false + updateEntries((previous) => { + const index = previous.findIndex((entry) => entry.id === id) + if (index < 0) { + return previous + } + completed = true + const next = previous.slice() + next[index] = { ...previous[index], status: 'ready', hostPath } + return next + }) + if (completed) { + onSuccess() + } } catch (error) { + // Why: switching targets or removing the tile cancels this attachment; + // late failures must not surface against the current composer. + if ( + !mountedRef.current || + lastHandle.current !== targetHandle || + (entryId !== null && !entriesRef.current.some((entry) => entry.id === entryId)) + ) { + return + } if (entryId !== null) { const failedId = entryId updateEntries((previous) => previous.filter((entry) => entry.id !== failedId))