From 6dfe63e1e251a917f45cf73f3ad6b783b53eafc5 Mon Sep 17 00:00:00 2001 From: Jakub Wolniewicz <4850809+frizikk@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:35:17 +0200 Subject: [PATCH] fix(desktop): preserve image attachment occurrence ownership --- .../app/chat/composer/attachments.test.tsx | 38 ++++++++++ .../src/app/chat/composer/attachments.tsx | 29 ++++++- .../chat/hooks/use-composer-actions.test.ts | 75 +++++++++++++++++++ .../app/chat/hooks/use-composer-actions.ts | 14 +++- apps/desktop/src/app/chat/session-tile.tsx | 8 +- apps/desktop/src/lib/desktop-fs.test.ts | 21 ++++++ apps/desktop/src/lib/desktop-fs.ts | 6 +- apps/desktop/src/store/composer.test.ts | 18 +++++ apps/desktop/src/store/composer.ts | 15 ++++ 9 files changed, 218 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/attachments.test.tsx b/apps/desktop/src/app/chat/composer/attachments.test.tsx index e356098bf7..917cdb374e 100644 --- a/apps/desktop/src/app/chat/composer/attachments.test.tsx +++ b/apps/desktop/src/app/chat/composer/attachments.test.tsx @@ -139,6 +139,44 @@ describe('AttachmentList', () => { expect(container.querySelector(`img[src="${DATA_URL}"]`)).toBeNull() }) + it('falls back to the original host path after an image was staged for a different filesystem', async () => { + const stagedPath = '/root/.hermes/attachments/photo.png' + const hostPath = 'C:\\Users\\alice\\Pictures\\photo.png' + + const readFileDataUrl = vi.fn(async (path: string) => { + if (path === hostPath) { + return DATA_URL + } + + throw new Error(`not readable: ${path}`) + }) + + Object.defineProperty(window, 'hermesDesktop', { + configurable: true, + value: { readFileDataUrl } + }) + + const image: ComposerAttachment = { + attachedSessionId: 'session-1', + detail: hostPath, + id: 'image:photo.png', + kind: 'image', + label: 'photo.png', + path: stagedPath, + thumbnailUrl: THUMBNAIL_URL + } + + await renderWithI18n() + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /photo\.png/ })) + }) + + expect(readFileDataUrl).toHaveBeenCalledWith(stagedPath) + expect(readFileDataUrl).toHaveBeenCalledWith(hostPath) + expect((await screen.findByRole('dialog')).querySelector('img')?.src).toBe(DATA_URL) + }) + it('still routes a non-image attachment to the preview rail', async () => { $previewTabs.set([]) diff --git a/apps/desktop/src/app/chat/composer/attachments.tsx b/apps/desktop/src/app/chat/composer/attachments.tsx index 46ee988129..759d9ca6f8 100644 --- a/apps/desktop/src/app/chat/composer/attachments.tsx +++ b/apps/desktop/src/app/chat/composer/attachments.tsx @@ -74,7 +74,34 @@ function AttachmentPill({ attachment, onRemove }: { attachment: ComposerAttachme if (attachment.kind === 'image') { try { - const source = lightboxSrc || (attachment.path ? await readDesktopFileDataUrlLocalFirst(attachment.path) : '') + let source = lightboxSrc || '' + + // Upload may replace `path` with a gateway-side staged path while + // `detail` still carries the original host path. If submit then fails, + // keep the surviving chip previewable across split-filesystem setups. + if (!source) { + const paths = [attachment.path, attachment.detail].filter( + (path, index, candidates): path is string => Boolean(path) && candidates.indexOf(path) === index + ) + + let lastError: unknown + + for (const path of paths) { + try { + source = await readDesktopFileDataUrlLocalFirst(path) + + if (source) { + break + } + } catch (error) { + lastError = error + } + } + + if (!source && lastError) { + throw lastError + } + } if (!source) { throw new Error(c.couldNotPreview(attachment.label)) diff --git a/apps/desktop/src/app/chat/hooks/use-composer-actions.test.ts b/apps/desktop/src/app/chat/hooks/use-composer-actions.test.ts index 8eac004725..3fb4b3dbac 100644 --- a/apps/desktop/src/app/chat/hooks/use-composer-actions.test.ts +++ b/apps/desktop/src/app/chat/hooks/use-composer-actions.test.ts @@ -393,6 +393,81 @@ describe('attachImagePath thumbnail separation', () => { expect($composerAttachments.get()).toEqual([]) }) + it('does not apply a removed occurrence thumbnail to the same path when reattached', async () => { + let resolveSecondRead!: (value: string) => void + + const secondRead = new Promise(resolve => { + resolveSecondRead = resolve + }) + + const readFileDataUrl = vi.fn().mockResolvedValueOnce(FULL_RES).mockReturnValueOnce(secondRead) + + ;( + window as unknown as { + hermesDesktop: { readFileDataUrl: typeof readFileDataUrl } + } + ).hermesDesktop = { readFileDataUrl } + + let resolveBitmap!: (bitmap: { close: () => void; height: number; width: number }) => void + + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ blob: async () => new Blob([new Uint8Array([0])], { type: 'image/png' }) })) + ) + vi.stubGlobal( + 'createImageBitmap', + vi.fn( + () => + new Promise<{ close: () => void; height: number; width: number }>(resolve => { + resolveBitmap = resolve + }) + ) + ) + + class MockOffscreenCanvas { + getContext = () => ({ drawImage: vi.fn() }) + convertToBlob = vi.fn(async () => new Blob(['first'], { type: 'image/png' })) + constructor(_width: number, _height: number) {} + } + + vi.stubGlobal('OffscreenCanvas', MockOffscreenCanvas) + + const { result } = renderHook(() => + useComposerActions({ activeSessionId: null, currentCwd: '', requestGateway: vi.fn() }) + ) + + let first!: Promise + let replacement!: Promise + + act(() => { + first = result.current.attachImagePath('/tmp/shot.png') + }) + + await waitFor(() => expect(createImageBitmap).toHaveBeenCalledOnce()) + + await act(async () => { + await result.current.removeAttachment('image:/tmp/shot.png') + }) + + act(() => { + replacement = result.current.attachImagePath('/tmp/shot.png') + }) + + resolveBitmap({ close: vi.fn(), height: 3000, width: 4000 }) + await waitFor(() => expect(readFileDataUrl).toHaveBeenCalledTimes(2)) + + const afterRemovedOccurrenceResolved = $composerAttachments.get()[0] + + resolveSecondRead('data:text/plain;base64,c2Vjb25k') + + await act(async () => { + await Promise.all([first, replacement]) + }) + + expect(afterRemovedOccurrenceResolved?.thumbnailUrl).toBeUndefined() + expect($composerAttachments.get()[0]?.previewUrl).toBe('data:text/plain;base64,c2Vjb25k') + }) + it('retains only the bounded thumbnail after creating a composer image preview', async () => { const readFileDataUrl = vi.fn(async () => FULL_RES) diff --git a/apps/desktop/src/app/chat/hooks/use-composer-actions.ts b/apps/desktop/src/app/chat/hooks/use-composer-actions.ts index 2173378a98..c5a3c80713 100644 --- a/apps/desktop/src/app/chat/hooks/use-composer-actions.ts +++ b/apps/desktop/src/app/chat/hooks/use-composer-actions.ts @@ -12,6 +12,7 @@ import { normalize } from '@/lib/text' import { addComposerAttachment, type ComposerAttachment, + mainComposerScope, removeComposerAttachment, setComposerTerminalSelection, updateComposerAttachment @@ -272,6 +273,7 @@ interface ComposerActionsScope { add: (attachment: ComposerAttachment) => void remove: (id: string) => ComposerAttachment | null update: (attachment: ComposerAttachment) => boolean + updateIfCurrent: (expected: ComposerAttachment, attachment: ComposerAttachment) => boolean target: string } @@ -279,6 +281,7 @@ const MAIN_ACTIONS_SCOPE: ComposerActionsScope = { add: addComposerAttachment, remove: removeComposerAttachment, update: updateComposerAttachment, + updateIfCurrent: (expected, attachment) => mainComposerScope.updateIfCurrent(expected, attachment), target: 'main' } @@ -482,9 +485,14 @@ export function useComposerActions({ // Keep only the bounded thumbnail in composer state. The full source // is read on demand for lightbox/download and separately at submit // for the model, so retaining 72 multi-MB data URLs serves no purpose. - // The user may remove the optimistic chip while an expensive image is - // still decoding. A late result updates in place but never resurrects it. - scope.update(thumbnailUrl ? { ...baseAttachment, thumbnailUrl } : { ...baseAttachment, previewUrl }) + // Bind the late preview to this exact attachment occurrence. The id is + // path-derived, so remove + reattach can create a replacement with the + // same id while this decode is still pending; updating by id alone would + // apply stale pixels to the replacement. + scope.updateIfCurrent( + baseAttachment, + thumbnailUrl ? { ...baseAttachment, thumbnailUrl } : { ...baseAttachment, previewUrl } + ) } return true diff --git a/apps/desktop/src/app/chat/session-tile.tsx b/apps/desktop/src/app/chat/session-tile.tsx index 76ecc3b24c..589630026a 100644 --- a/apps/desktop/src/app/chat/session-tile.tsx +++ b/apps/desktop/src/app/chat/session-tile.tsx @@ -149,7 +149,13 @@ function TileChat({ activeSessionId: runtimeId, currentCwd: cwd, requestGateway, - scope: { add: attachments.add, remove: attachments.remove, target: scope.target, update: attachments.update } + scope: { + add: attachments.add, + remove: attachments.remove, + target: scope.target, + update: attachments.update, + updateIfCurrent: attachments.updateIfCurrent + } }) // ChatView is memo()d — every callback prop must be referentially stable or diff --git a/apps/desktop/src/lib/desktop-fs.test.ts b/apps/desktop/src/lib/desktop-fs.test.ts index bd9ad2df98..2a3d0c54e6 100644 --- a/apps/desktop/src/lib/desktop-fs.test.ts +++ b/apps/desktop/src/lib/desktop-fs.test.ts @@ -9,6 +9,7 @@ import { desktopGitRoot, readDesktopDir, readDesktopFileDataUrl, + readDesktopFileDataUrlLocalFirst, readDesktopFileText, selectDesktopPaths, setDesktopFsRemotePicker @@ -113,6 +114,26 @@ describe('desktop filesystem facade', () => { expect(gitRoot).not.toHaveBeenCalled() }) + it('does not retry the same unreadable path through the local facade', async () => { + const error = new Error('not readable') + + $connection.set({ mode: 'local' } as never) + readFileDataUrl.mockRejectedValueOnce(error) + + await expect(readDesktopFileDataUrlLocalFirst('/missing.png')).rejects.toBe(error) + expect(readFileDataUrl).toHaveBeenCalledOnce() + expect(api).not.toHaveBeenCalled() + }) + + it('falls back from local disk to the active gateway in remote mode', async () => { + $connection.set({ mode: 'remote' } as never) + readFileDataUrl.mockRejectedValueOnce(new Error('not on host')) + + await expect(readDesktopFileDataUrlLocalFirst('/remote/image.png')).resolves.toBe('data:text/plain;base64,cmVtb3Rl') + expect(readFileDataUrl).toHaveBeenCalledOnce() + expect(api).toHaveBeenCalledWith({ path: '/api/fs/read-data-url?path=%2Fremote%2Fimage.png' }) + }) + it('targets the active profile backend so a remote profile never reads local disk', async () => { $connection.set({ mode: 'remote', profile: 'remote-docker' } as never) diff --git a/apps/desktop/src/lib/desktop-fs.ts b/apps/desktop/src/lib/desktop-fs.ts index 1775911b1b..d2ebc3ac10 100644 --- a/apps/desktop/src/lib/desktop-fs.ts +++ b/apps/desktop/src/lib/desktop-fs.ts @@ -121,7 +121,11 @@ export async function readDesktopFileDataUrlLocalFirst(path: string): Promise { expect(updated).toBe(false) expect($composerAttachments.get()).toHaveLength(0) }) + + it('updates only the exact attachment occurrence captured before an async operation', () => { + const scope = createComposerAttachmentScope() + const first = attachment({ id: 'image:a', kind: 'image', path: '/tmp/a.png' }) + const replacement = attachment({ id: 'image:a', kind: 'image', path: '/tmp/a.png' }) + + scope.add(first) + scope.remove(first.id) + scope.add(replacement) + + expect(scope.updateIfCurrent(first, { ...first, thumbnailUrl: 'data:image/png;base64,stale' })).toBe(false) + expect(scope.$attachments.get()).toEqual([replacement]) + expect(scope.updateIfCurrent(replacement, { ...replacement, thumbnailUrl: 'data:image/png;base64,current' })).toBe( + true + ) + expect(scope.$attachments.get()[0]?.thumbnailUrl).toBe('data:image/png;base64,current') + }) }) describe('session drafts', () => { diff --git a/apps/desktop/src/store/composer.ts b/apps/desktop/src/store/composer.ts index 37069491c4..b9cf116c14 100644 --- a/apps/desktop/src/store/composer.ts +++ b/apps/desktop/src/store/composer.ts @@ -59,6 +59,7 @@ export interface ComposerAttachmentScope { remove(id: string): ComposerAttachment | null setUploadState(id: string, uploadState?: ComposerAttachment['uploadState']): void update(attachment: ComposerAttachment): boolean + updateIfCurrent(expected: ComposerAttachment, attachment: ComposerAttachment): boolean } export function createComposerAttachmentScope($attachments = atom([])): ComposerAttachmentScope { @@ -107,6 +108,20 @@ export function createComposerAttachmentScope($attachments = atom item === expected) + + if (index < 0) { + return false + } + + const next = [...current] + next[index] = attachment + $attachments.set(next) + return true } }