From 485e1e8f69aeb35184d4bf46c9b7abcd2230d341 Mon Sep 17 00:00:00 2001 From: David Metcalfe <80915+DavidMetcalfe@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:28:58 -0700 Subject: [PATCH] fix(desktop): keep full-res previewUrl separate from downscaled thumbnail Addresses review feedback on #68744: downscaling inside attachmentPreviewDataUrl made previewUrl the 2048px PNG, but current main (d8cb73b4ab2) feeds that field to ImageLightbox and useImageDownload, so large attachments would open and download at reduced resolution. - attachmentPreviewDataUrl returns the full-resolution data URL again - ComposerAttachment gains thumbnailUrl?: string (store/composer.ts) - attachImagePath stores previewUrl (full-res) + thumbnailUrl (downscaled) - Attachment pill renders thumbnailUrl ?? previewUrl; lightbox/download keep the full-res previewUrl - optimisticAttachmentRef prefers thumbnailUrl for the in-flight bubble display ref (same main-thread decode freeze at send time) - Attachment-level regression test in use-composer-actions.test.ts: full-res previewUrl preserved while thumbnailUrl is a separate downscaled value (4000x3000 -> 2048x1536) --- .../src/app/chat/composer/attachments.tsx | 2 +- .../chat/hooks/use-composer-actions.test.ts | 85 ++++++++++++++++++- .../app/chat/hooks/use-composer-actions.ts | 19 +++-- apps/desktop/src/lib/chat-runtime.test.ts | 11 +++ apps/desktop/src/lib/chat-runtime.ts | 5 +- apps/desktop/src/store/composer.ts | 3 + 6 files changed, 114 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/attachments.tsx b/apps/desktop/src/app/chat/composer/attachments.tsx index 8ae78e5140..bd062eec19 100644 --- a/apps/desktop/src/app/chat/composer/attachments.tsx +++ b/apps/desktop/src/app/chat/composer/attachments.tsx @@ -127,7 +127,7 @@ function AttachmentPill({ attachment, onRemove }: { attachment: ComposerAttachme alt={attachment.label} className="size-full object-cover" draggable={false} - src={attachment.previewUrl} + src={attachment.thumbnailUrl ?? attachment.previewUrl} /> ) : ( 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 a7b76e1d09..d2ce050340 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 @@ -1,7 +1,7 @@ import { act, renderHook } from '@testing-library/react' import { afterEach, describe, expect, it, vi } from 'vitest' -import type { ComposerAttachment } from '@/store/composer' +import { $composerAttachments, type ComposerAttachment } from '@/store/composer' import { $connection } from '@/store/session' import { @@ -319,3 +319,86 @@ describe('useComposerActions native image drops', () => { ) }) }) + +describe('attachImagePath thumbnail separation', () => { + // Full-resolution data URL the local bridge returns for a pasted screenshot. + // Content is irrelevant — the mock bitmap below reports 4000×3000 so the + // real downscale path runs (the data URL itself is never decoded in jsdom). + const FULL_RES = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+GkZcAAAAASUVORK5CYII=' + + afterEach(() => { + vi.unstubAllGlobals() + delete (window as unknown as { hermesDesktop?: unknown }).hermesDesktop + $composerAttachments.set([]) + }) + + it('keeps the full-resolution previewUrl and stores a separate downscaled thumbnailUrl', async () => { + const readFileDataUrl = vi.fn(async () => FULL_RES) + + ;(window as unknown as { hermesDesktop: { readFileDataUrl: typeof readFileDataUrl } }).hermesDesktop = { + readFileDataUrl + } + + // Exercise the real downscale path: 4000×3000 bitmap → 2048×1536 canvas. + const drawImage = vi.fn() + const close = vi.fn() + + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ + blob: async () => new Blob([new Uint8Array([0])], { type: 'image/png' }) + })) + ) + vi.stubGlobal('createImageBitmap', vi.fn(async () => ({ width: 4000, height: 3000, close }))) + + class MockOffscreenCanvas { + getContext = () => ({ drawImage }) + convertToBlob = vi.fn(async () => new Blob(['x'], { type: 'image/png' })) + constructor(_width: number, _height: number) {} + } + + vi.stubGlobal('OffscreenCanvas', MockOffscreenCanvas) + + const { result } = renderHook(() => + useComposerActions({ activeSessionId: null, currentCwd: '', requestGateway: vi.fn() }) + ) + + await act(async () => { + await result.current.attachImagePath('/tmp/shot.png') + }) + + const attachment = $composerAttachments.get()[0] + + expect(attachment).toBeDefined() + // Full-resolution bytes stay on `previewUrl` — the same field main feeds + // to ImageLightbox and useImageDownload, so open/download keep full res. + expect(attachment?.previewUrl).toBe(FULL_RES) + // The pill thumbnail is a SEPARATE downscaled value, never the multi-MB + // original (which would re-introduce the main-thread decode freeze). + expect(attachment?.thumbnailUrl).toBeDefined() + expect(attachment?.thumbnailUrl).not.toBe(FULL_RES) + expect(drawImage).toHaveBeenCalledWith(expect.anything(), 0, 0, 2048, 1536) + expect(close).toHaveBeenCalled() + }) + + it('leaves the thumbnail undefined when the preview is not an image', async () => { + const readFileDataUrl = vi.fn(async () => 'data:text/plain;base64,aGVsbG8=') + + ;(window as unknown as { hermesDesktop: { readFileDataUrl: typeof readFileDataUrl } }).hermesDesktop = { + readFileDataUrl + } + + const { result } = renderHook(() => + useComposerActions({ activeSessionId: null, currentCwd: '', requestGateway: vi.fn() }) + ) + + await act(async () => { + await result.current.attachImagePath('/tmp/shot.txt') + }) + + const attachment = $composerAttachments.get()[0] + + expect(attachment?.previewUrl).toBe('data:text/plain;base64,aGVsbG8=') + expect(attachment?.thumbnailUrl).toBeUndefined() + }) +}) 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 ea8afa19be..d3f1577a19 100644 --- a/apps/desktop/src/app/chat/hooks/use-composer-actions.ts +++ b/apps/desktop/src/app/chat/hooks/use-composer-actions.ts @@ -68,13 +68,6 @@ export async function attachmentPreviewDataUrl(filePath: string): Promise5 MB). The full-resolution bytes are still on disk for the model. - if (dataUrl.startsWith('data:image/')) { - return downscaleDataUrlForPreview(dataUrl) - } - return dataUrl } @@ -483,7 +476,17 @@ export function useComposerActions({ const previewUrl = await attachmentPreviewDataUrl(filePath) if (previewUrl) { - scope.add({ ...baseAttachment, previewUrl }) + // Downscale only the pill thumbnail. `previewUrl` must keep the + // full-resolution bytes: current main feeds it to ImageLightbox and + // useImageDownload (attachments.tsx), and the attached-image + // pipeline uploads the on-disk original to the model. The helper + // never rejects — on failure it returns a 1×1 placeholder so the + // pill never renders the multi-MB original. + const thumbnailUrl = previewUrl.startsWith('data:image/') + ? await downscaleDataUrlForPreview(previewUrl) + : undefined + + scope.add({ ...baseAttachment, previewUrl, thumbnailUrl }) } return true diff --git a/apps/desktop/src/lib/chat-runtime.test.ts b/apps/desktop/src/lib/chat-runtime.test.ts index 621286e584..b9038396d3 100644 --- a/apps/desktop/src/lib/chat-runtime.test.ts +++ b/apps/desktop/src/lib/chat-runtime.test.ts @@ -13,6 +13,7 @@ import { } from './chat-runtime' const DATA_URL = 'data:image/png;base64,iVBORw0KGgoAAAANS' +const THUMB_URL = 'data:image/png;base64,dGh1bWI=' function attachment(overrides: Partial & Pick): ComposerAttachment { return { id: 'a', label: 'file.png', ...overrides } @@ -27,6 +28,16 @@ describe('optimisticAttachmentRef', () => { expect(ref).toBe(DATA_URL) }) + it('prefers the downscaled thumbnail for the display ref when present', () => { + const ref = optimisticAttachmentRef( + attachment({ kind: 'image', detail: '/tmp/shot.png', previewUrl: DATA_URL, thumbnailUrl: THUMB_URL }) + ) + + // The bubble is a display-only thumbnail; full-res previewUrl stays for + // lightbox/download and the model gets bytes via the upload pipeline. + expect(ref).toBe(THUMB_URL) + }) + it('falls back to an @image: path ref when no preview is available', () => { expect(optimisticAttachmentRef(attachment({ kind: 'image', detail: '/tmp/shot.png' }))).toBe('@image:/tmp/shot.png') }) diff --git a/apps/desktop/src/lib/chat-runtime.ts b/apps/desktop/src/lib/chat-runtime.ts index 191ed5dcc3..30dc625119 100644 --- a/apps/desktop/src/lib/chat-runtime.ts +++ b/apps/desktop/src/lib/chat-runtime.ts @@ -259,7 +259,10 @@ export function optimisticAttachmentRef(attachment: ComposerAttachment): string } if (attachment.kind === 'image' && attachment.previewUrl?.startsWith('data:')) { - return attachment.previewUrl + // The pill and the in-flight bubble render the downscaled thumbnail; the + // full-resolution `previewUrl` stays for lightbox/download, and the model + // receives bytes via the attached-image upload pipeline, not this ref. + return attachment.thumbnailUrl ?? attachment.previewUrl } return attachmentDisplayText(attachment) diff --git a/apps/desktop/src/store/composer.ts b/apps/desktop/src/store/composer.ts index 1b6847a537..f7a4fbe609 100644 --- a/apps/desktop/src/store/composer.ts +++ b/apps/desktop/src/store/composer.ts @@ -10,6 +10,9 @@ export interface ComposerAttachment { detail?: string refText?: string previewUrl?: string + /** Downscaled data URL for the attachment card's pill only. Keeps the + * full-resolution `previewUrl` for lightbox/download/model bytes. */ + thumbnailUrl?: string path?: string attachedSessionId?: string /** Set while the file/image bytes are being staged into the session