From e2d60e84e5cc997eabf30f6c11a84846987925a8 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:42:21 -0700 Subject: [PATCH] fix(feedback): bound image preview resources --- .../sidebar/SidebarFeedbackDialog.test.tsx | 26 +++++++++ .../sidebar/SidebarFeedbackDialog.tsx | 30 ++++++---- src/renderer/src/i18n/locales/en.json | 2 + src/renderer/src/i18n/locales/es.json | 2 + src/renderer/src/i18n/locales/ja.json | 2 + src/renderer/src/i18n/locales/ko.json | 2 + src/renderer/src/i18n/locales/zh.json | 2 + .../lib/feedback-image-attachments.test.ts | 55 ++++++++++++++++++- .../src/lib/feedback-image-attachments.ts | 33 ++++++++++- 9 files changed, 138 insertions(+), 16 deletions(-) diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx index bc53ad52b9f..70db571e77f 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx @@ -166,6 +166,32 @@ describe('SidebarFeedbackDialog image submission', () => { expect(onOpenChange).toHaveBeenCalledWith(false) }) + it('releases image previews when the sidebar unmounts the dialog', async () => { + mocks.readFeedbackImageFiles.mockResolvedValue({ + images: [ + { + id: 'shot', + name: 'shot.png', + contentType: 'image/png', + bytes: 1, + data: new Uint8Array([1]), + previewUrl: 'blob:shot' + } + ], + errors: [] + }) + const { container, unmount } = render() + const input = container.querySelector('input[type="file"]') + fireEvent.change(input!, { + target: { files: [new File(['x'], 'shot.png', { type: 'image/png' })] } + }) + await screen.findByRole('button', { name: 'Remove shot.png' }) + + unmount() + + expect(URL.revokeObjectURL).toHaveBeenCalledWith('blob:shot') + }) + it('does not consume text when the pasted image cannot be attached', () => { mocks.readFeedbackImageFiles.mockResolvedValue({ images: [], diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx index 783c8dff058..d556f9233a8 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx @@ -70,17 +70,23 @@ export function SidebarFeedbackDialog({ const [pendingImageReadCount, setPendingImageReadCount] = useState(0) const mountedRef = useMountedRef() const feedbackTextareaRef = useRef(null) + const liveImageDraftsRef = useRef([]) const clearImages = React.useCallback(() => { - setImages((current) => { - current.forEach(releaseFeedbackImageDraft) - return [] - }) + liveImageDraftsRef.current.forEach(releaseFeedbackImageDraft) + liveImageDraftsRef.current = [] + setImages([]) }, []) // Why: object URLs for the thumbnails leak until revoked, so drop them when // the dialog unmounts as well as when an attachment is removed. - React.useEffect(() => clearImages, [clearImages]) + React.useEffect( + () => () => { + liveImageDraftsRef.current.forEach(releaseFeedbackImageDraft) + liveImageDraftsRef.current = [] + }, + [] + ) const imageCount = images.length @@ -117,6 +123,7 @@ export function SidebarFeedbackDialog({ } setPendingImageReadCount((current) => Math.max(0, current - files.length)) if (added.length > 0) { + liveImageDraftsRef.current = [...liveImageDraftsRef.current, ...added] setImages((existing) => [...existing, ...added]) } // Why: never drop an attachment without telling the user — that @@ -142,13 +149,12 @@ export function SidebarFeedbackDialog({ ) const handleRemoveImage = React.useCallback((id: string) => { - setImages((current) => { - const removed = current.find((image) => image.id === id) - if (removed) { - releaseFeedbackImageDraft(removed) - } - return current.filter((image) => image.id !== id) - }) + const removed = liveImageDraftsRef.current.find((image) => image.id === id) + if (removed) { + releaseFeedbackImageDraft(removed) + liveImageDraftsRef.current = liveImageDraftsRef.current.filter((image) => image.id !== id) + } + setImages((current) => current.filter((image) => image.id !== id)) }, []) const { isDragActive, contentRef, dragHandlers } = useFeedbackImageDrop(open, handleAddFiles) diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index a32b4cce24f..be6a703cd46 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -667,6 +667,8 @@ "empty": "{{fileName}} is empty.", "tooLarge": "{{fileName}} is larger than {{maxSize}}.", "tooMany": "You can attach up to {{maxCount}} images.", + "dimensionsTooLarge": "{{fileName}} has dimensions that are too large to preview safely.", + "invalidImage": "{{fileName}} is not a valid supported image.", "additionalErrors": "{{count}} additional images could not be attached." } } diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 5f82047797c..61730b7ee93 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -644,6 +644,8 @@ "empty": "{{fileName}} is empty.", "tooLarge": "{{fileName}} is larger than {{maxSize}}.", "tooMany": "You can attach up to {{maxCount}} images.", + "dimensionsTooLarge": "{{fileName}} has dimensions that are too large to preview safely.", + "invalidImage": "{{fileName}} is not a valid supported image.", "additionalErrors": "{{count}} additional images could not be attached." } } diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index a1feb6a4073..c5f344d2646 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -644,6 +644,8 @@ "empty": "{{fileName}} is empty.", "tooLarge": "{{fileName}} is larger than {{maxSize}}.", "tooMany": "You can attach up to {{maxCount}} images.", + "dimensionsTooLarge": "{{fileName}} has dimensions that are too large to preview safely.", + "invalidImage": "{{fileName}} is not a valid supported image.", "additionalErrors": "{{count}} additional images could not be attached." } } diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 64c1a31a05a..d8fcde66d4c 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -644,6 +644,8 @@ "empty": "{{fileName}} is empty.", "tooLarge": "{{fileName}} is larger than {{maxSize}}.", "tooMany": "You can attach up to {{maxCount}} images.", + "dimensionsTooLarge": "{{fileName}} has dimensions that are too large to preview safely.", + "invalidImage": "{{fileName}} is not a valid supported image.", "additionalErrors": "{{count}} additional images could not be attached." } } diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 8317b3c6b76..e2818360eca 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -644,6 +644,8 @@ "empty": "{{fileName}} is empty.", "tooLarge": "{{fileName}} is larger than {{maxSize}}.", "tooMany": "You can attach up to {{maxCount}} images.", + "dimensionsTooLarge": "{{fileName}} has dimensions that are too large to preview safely.", + "invalidImage": "{{fileName}} is not a valid supported image.", "additionalErrors": "{{count}} additional images could not be attached." } } diff --git a/src/renderer/src/lib/feedback-image-attachments.test.ts b/src/renderer/src/lib/feedback-image-attachments.test.ts index 353931d1742..c7f1200de2c 100644 --- a/src/renderer/src/lib/feedback-image-attachments.test.ts +++ b/src/renderer/src/lib/feedback-image-attachments.test.ts @@ -14,8 +14,21 @@ beforeEach(() => { URL.revokeObjectURL = vi.fn() }) -function pngFile(name: string, size = 8): File { - const file = new File(['x'], name, { type: 'image/png' }) +function pngHeader(width = 1, height = 1): Uint8Array { + const bytes = new Uint8Array(new ArrayBuffer(24)) + bytes.set([137, 80, 78, 71, 13, 10, 26, 10]) + const view = new DataView(bytes.buffer) + view.setUint32(8, 13) + bytes.set([73, 72, 68, 82], 12) + view.setUint32(16, width) + view.setUint32(20, height) + return bytes +} + +function pngFile(name: string, size = 24, dimensions = { width: 1, height: 1 }): File { + const file = new File([pngHeader(dimensions.width, dimensions.height)], name, { + type: 'image/png' + }) Object.defineProperty(file, 'size', { value: size }) return file } @@ -55,7 +68,7 @@ describe('readFeedbackImageFiles', () => { expect(errors).toEqual([]) expect(images).toHaveLength(2) expect(new Set(images.map((image) => image.id)).size).toBe(2) - expect(images[0].data.byteLength).toBe(1) + expect(images[0].data.byteLength).toBe(24) }) it('reports an unsupported type instead of skipping it', async () => { @@ -98,6 +111,42 @@ describe('readFeedbackImageFiles', () => { expect(errors).toEqual(['empty.png is empty.']) }) + it('rejects a raster that would exceed the decoded preview budget', async () => { + const file = pngFile('huge-dimensions.png', 24, { width: 8192, height: 8192 }) + + const { images, errors } = await readFeedbackImageFiles([file], 0) + + expect(images).toEqual([]) + expect(errors).toEqual([ + 'huge-dimensions.png has dimensions that are too large to preview safely.' + ]) + expect(URL.createObjectURL).not.toHaveBeenCalled() + }) + + it('rejects invalid raster bytes instead of mounting a broken preview', async () => { + const file = new File(['not an image'], 'broken.png', { type: 'image/png' }) + + const { images, errors } = await readFeedbackImageFiles([file], 0) + + expect(images).toEqual([]) + expect(errors).toEqual(['broken.png is not a valid supported image.']) + expect(URL.createObjectURL).not.toHaveBeenCalled() + }) + + it('does not count a rejected preview against the attachment limit', async () => { + const files = [ + new File(['not an image'], 'broken.png', { type: 'image/png' }), + ...Array.from({ length: MAX_FEEDBACK_IMAGE_COUNT }, (_, index) => + pngFile(`valid-${index}.png`) + ) + ] + + const { images, errors } = await readFeedbackImageFiles(files, 0) + + expect(images).toHaveLength(MAX_FEEDBACK_IMAGE_COUNT) + expect(errors).toEqual(['broken.png is not a valid supported image.']) + }) + it('reports the overflow once the running count is already at capacity', async () => { const { images, errors } = await readFeedbackImageFiles( [pngFile('a.png')], diff --git a/src/renderer/src/lib/feedback-image-attachments.ts b/src/renderer/src/lib/feedback-image-attachments.ts index 7e949005118..69915218c31 100644 --- a/src/renderer/src/lib/feedback-image-attachments.ts +++ b/src/renderer/src/lib/feedback-image-attachments.ts @@ -1,5 +1,10 @@ import { translate } from '@/i18n/i18n' import { createBrowserUuid } from './browser-uuid' +import { + INVALID_RASTER_IMAGE_PREVIEW_ERROR, + RASTER_IMAGE_PREVIEW_TOO_LARGE_ERROR, + assertRasterImagePreviewWithinLimits +} from '../../../shared/raster-image-preview-limits' export const MAX_FEEDBACK_IMAGE_COUNT = 4 export const MAX_FEEDBACK_IMAGE_BYTES = 8 * 1024 * 1024 @@ -123,6 +128,32 @@ export async function readFeedbackImageFiles( ) break } + const data = new Uint8Array(await file.arrayBuffer()) + try { + assertRasterImagePreviewWithinLimits(data, file.type) + } catch (error) { + if (error instanceof Error && error.message === RASTER_IMAGE_PREVIEW_TOO_LARGE_ERROR) { + addError(() => + translate( + 'auto.lib.feedback.image.attachments.dimensionsTooLarge', + '{{fileName}} has dimensions that are too large to preview safely.', + { fileName } + ) + ) + continue + } + if (error instanceof Error && error.message === INVALID_RASTER_IMAGE_PREVIEW_ERROR) { + addError(() => + translate( + 'auto.lib.feedback.image.attachments.invalidImage', + '{{fileName}} is not a valid supported image.', + { fileName } + ) + ) + continue + } + throw error + } remaining -= 1 images.push({ // Why: crypto.randomUUID is undefined in non-secure browser contexts (LAN @@ -131,7 +162,7 @@ export async function readFeedbackImageFiles( name: fileName, contentType: file.type, bytes: file.size, - data: new Uint8Array(await file.arrayBuffer()), + data, previewUrl: URL.createObjectURL(file) }) }