diff --git a/src/main/ipc/feedback-image-attachments.test.ts b/src/main/ipc/feedback-image-attachments.test.ts new file mode 100644 index 000000000..187537cad --- /dev/null +++ b/src/main/ipc/feedback-image-attachments.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from 'vitest' +import { + MAX_FEEDBACK_IMAGE_BYTES, + MAX_FEEDBACK_IMAGE_COUNT, + appendFeedbackImagesToFormData, + feedbackImageFilename, + isSupportedFeedbackImageContentType, + validateFeedbackImages +} from './feedback-image-attachments' + +function image(contentType: string, bytes = 4): { contentType: string; data: Uint8Array } { + return { contentType, data: new Uint8Array(bytes).fill(1) } +} + +describe('isSupportedFeedbackImageContentType', () => { + it.each(['image/png', 'image/jpeg', 'image/webp', 'image/gif'])('accepts %s', (contentType) => { + expect(isSupportedFeedbackImageContentType(contentType)).toBe(true) + }) + + it.each(['image/bmp', 'image/svg+xml', 'application/pdf', ''])('rejects %s', (contentType) => { + expect(isSupportedFeedbackImageContentType(contentType)).toBe(false) + }) + + // Why: the allow-list is an object literal, so a prototype member must not + // pass as a content type and get spliced into the upload filename. + it.each(['constructor', '__proto__', 'toString', 'valueOf', 'hasOwnProperty'])( + 'rejects the inherited member %s', + (contentType) => { + expect(isSupportedFeedbackImageContentType(contentType)).toBe(false) + expect(validateFeedbackImages([image(contentType)])).toBe('Unsupported image type.') + } + ) +}) + +describe('feedbackImageFilename', () => { + it.each([ + ['image/png', 'feedback-image-1.png'], + ['image/jpeg', 'feedback-image-1.jpg'], + ['image/webp', 'feedback-image-1.webp'], + ['image/gif', 'feedback-image-1.gif'] + ])('names a %s attachment %s', (contentType, expected) => { + expect(feedbackImageFilename(0, contentType)).toBe(expected) + }) + + it('numbers attachments from one', () => { + expect(feedbackImageFilename(3, 'image/png')).toBe('feedback-image-4.png') + }) +}) + +describe('appendFeedbackImagesToFormData', () => { + it('frames only the exact typed-array view', async () => { + const backing = new Uint8Array([0, 1, 2, 3]) + const view = backing.subarray(1, 3) + const formData = new FormData() + + appendFeedbackImagesToFormData(formData, [{ contentType: 'image/png', data: view }]) + + const blob = formData.get('feedbackImage') + expect(blob).toBeInstanceOf(Blob) + expect(new Uint8Array(await (blob as Blob).arrayBuffer())).toEqual(new Uint8Array([1, 2])) + }) +}) + +describe('validateFeedbackImages', () => { + it('accepts a full batch of supported images', () => { + expect( + validateFeedbackImages( + Array.from({ length: MAX_FEEDBACK_IMAGE_COUNT }, () => image('image/png')) + ) + ).toBeNull() + }) + + it('rejects more than the supported count', () => { + expect( + validateFeedbackImages( + Array.from({ length: MAX_FEEDBACK_IMAGE_COUNT + 1 }, () => image('image/png')) + ) + ).toBe(`Attach ${MAX_FEEDBACK_IMAGE_COUNT} images or fewer.`) + }) + + it('rejects malformed attachment collections and byte payloads', () => { + expect(validateFeedbackImages('image/png')).toBe('Image attachments must be a list.') + expect(validateFeedbackImages([null])).toBe('Invalid image attachment.') + expect(validateFeedbackImages([{ contentType: 'image/png', data: '8388608' }])).toBe( + 'Invalid image attachment bytes.' + ) + }) + + it('rejects an empty attachment', () => { + expect(validateFeedbackImages([image('image/png', 0)])).toBe('Image attachment is empty.') + }) + + it('rejects an attachment over the byte cap', () => { + expect(validateFeedbackImages([image('image/png', MAX_FEEDBACK_IMAGE_BYTES + 1)])).toBe( + `Each image must be ${MAX_FEEDBACK_IMAGE_BYTES} bytes or fewer.` + ) + }) +}) diff --git a/src/main/ipc/feedback-image-attachments.ts b/src/main/ipc/feedback-image-attachments.ts new file mode 100644 index 000000000..f574a7129 --- /dev/null +++ b/src/main/ipc/feedback-image-attachments.ts @@ -0,0 +1,96 @@ +import { readFetchResponseJsonWithinLimit } from '../../shared/fetch-response-body' + +// Why: mirrors the server allow-list. Slack picks a renderer from the filename +// extension, so every accepted type needs one. +const FEEDBACK_IMAGE_EXTENSIONS: Record = { + 'image/png': 'png', + 'image/jpeg': 'jpg', + 'image/webp': 'webp', + 'image/gif': 'gif' +} + +export const MAX_FEEDBACK_IMAGE_COUNT = 4 +export const MAX_FEEDBACK_IMAGE_BYTES = 8 * 1024 * 1024 +export const MAX_FEEDBACK_IMAGE_RESPONSE_BYTES = 64 * 1024 +export const FEEDBACK_IMAGE_FORM_FIELD = 'feedbackImage' + +export type FeedbackImageAttachment = { + contentType: string + data: Uint8Array +} + +export function isSupportedFeedbackImageContentType(contentType: string): boolean { + // Why: `in` walks the prototype chain, so "constructor" and "__proto__" would + // clear the allow-list and name the upload after an inherited member. + return Object.hasOwn(FEEDBACK_IMAGE_EXTENSIONS, contentType) +} + +export function getSupportedFeedbackImageContentTypes(): string[] { + return Object.keys(FEEDBACK_IMAGE_EXTENSIONS) +} + +export function feedbackImageFilename(index: number, contentType: string): string { + return `feedback-image-${index + 1}.${FEEDBACK_IMAGE_EXTENSIONS[contentType]}` +} + +/** Defence in depth: the renderer validates first, but this channel is reachable directly. */ +export function validateFeedbackImages(images: unknown): string | null { + if (!Array.isArray(images)) { + return 'Image attachments must be a list.' + } + if (images.length > MAX_FEEDBACK_IMAGE_COUNT) { + return `Attach ${MAX_FEEDBACK_IMAGE_COUNT} images or fewer.` + } + for (const image of images) { + if (!image || typeof image !== 'object') { + return 'Invalid image attachment.' + } + if (typeof image.contentType !== 'string') { + return 'Invalid image attachment content type.' + } + if (!isSupportedFeedbackImageContentType(image.contentType)) { + return 'Unsupported image type.' + } + if (!(image.data instanceof Uint8Array)) { + return 'Invalid image attachment bytes.' + } + if (image.data.byteLength === 0) { + return 'Image attachment is empty.' + } + if (image.data.byteLength > MAX_FEEDBACK_IMAGE_BYTES) { + return `Each image must be ${MAX_FEEDBACK_IMAGE_BYTES} bytes or fewer.` + } + } + return null +} + +export function appendFeedbackImagesToFormData( + formData: FormData, + images: FeedbackImageAttachment[] +): void { + for (const [index, image] of images.entries()) { + formData.append( + FEEDBACK_IMAGE_FORM_FIELD, + new Blob([image.data as BlobPart], { type: image.contentType }), + feedbackImageFilename(index, image.contentType) + ) + } +} + +/** Atomic servers omit the image field after both the text and images land. */ +export async function readFeedbackImagesDelivered(response: Response): Promise { + try { + const parsed: unknown = await readFetchResponseJsonWithinLimit( + response, + MAX_FEEDBACK_IMAGE_RESPONSE_BYTES + ) + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + return false + } + if ('imagesDelivered' in parsed) { + return (parsed as { imagesDelivered?: unknown }).imagesDelivered === true + } + return (parsed as { ok?: unknown }).ok === true + } catch {} + return false +} diff --git a/src/main/ipc/feedback.test.ts b/src/main/ipc/feedback.test.ts index a901b4fac..4646ee8a3 100644 --- a/src/main/ipc/feedback.test.ts +++ b/src/main/ipc/feedback.test.ts @@ -16,6 +16,7 @@ vi.mock('electron', () => ({ net: { fetch: (...args: unknown[]) => fetchMock(...args) } })) +import { MAX_FEEDBACK_IMAGE_RESPONSE_BYTES } from './feedback-image-attachments' import { registerFeedbackHandlers, submitFeedback } from './feedback' function okResponse(): Response { @@ -392,4 +393,219 @@ describe('submitFeedback', () => { githubEmail: null }) }) + + describe('image attachments', () => { + function pngImage(bytes = 8): { contentType: string; data: Uint8Array } { + return { contentType: 'image/png', data: new Uint8Array(bytes).fill(1) } + } + + function imageSubmitArgs( + images: { contentType: string; data: Uint8Array }[] + ): Parameters[0] { + return { + feedback: 'images attached', + submissionType: 'feedback', + githubLogin: 'someone', + githubEmail: null, + images + } + } + + function jsonResponse(body: unknown): Response { + return Response.json(body, { status: 202 }) + } + + it('sends attached images as multipart form parts', async () => { + await submitFeedback(imageSubmitArgs([pngImage(), pngImage()])) + + const body = requestInit().body as FormData + expect(body).toBeInstanceOf(FormData) + expect(body.getAll('feedbackImage')).toHaveLength(2) + expect(body.get('feedback')).toBe('images attached') + // Why: multipart must not lose the enrichment fields the JSON lane sends. + expect(body.get('submissionType')).toBe('feedback') + expect(body.get('appVersion')).toBe('1.2.3-test') + }) + + it('keeps the JSON lane when nothing is attached', async () => { + await submitFeedback(imageSubmitArgs([])) + + expect(requestInit().body).not.toBeInstanceOf(FormData) + expect(postedBody().feedback).toBe('images attached') + }) + + it('reports partial delivery when the server could not attach the images', async () => { + fetchMock.mockResolvedValue(jsonResponse({ ok: true, imagesDelivered: false })) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false + }) + }) + + it('accepts the production atomic-success response when it omits the image result', async () => { + fetchMock.mockResolvedValue(jsonResponse({ ok: true })) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: true + }) + }) + + it('reports unconfirmed delivery for a settled non-JSON 2xx', async () => { + fetchMock.mockResolvedValue({ + ok: true, + status: 202, + json: async () => { + throw new SyntaxError('Unexpected token') + } + } as unknown as Response) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false + }) + }) + + it('reports unconfirmed delivery when the response body aborts before the deadline', async () => { + fetchMock.mockResolvedValue({ + ok: true, + status: 202, + json: async () => { + throw new TypeError('terminated') + } + } as unknown as Response) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false + }) + expect(requestInit().signal).toMatchObject({ aborted: false }) + }) + + it('bounds the image-delivery response body', async () => { + fetchMock.mockResolvedValue( + new Response('x'.repeat(MAX_FEEDBACK_IMAGE_RESPONSE_BYTES + 1), { status: 202 }) + ) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false + }) + }) + + it('fails a stalled delivery response body at the attachment timeout', async () => { + vi.useFakeTimers() + fetchMock.mockImplementation((_url: string, init?: RequestInit) => + Promise.resolve({ + ok: true, + status: 202, + json: () => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(new Error('body aborted'))) + }) + } as unknown as Response) + ) + + const result = submitFeedback(imageSubmitArgs([pngImage()])) + await vi.advanceTimersByTimeAsync(60_000) + + await expect(result).resolves.toEqual({ + ok: false, + status: null, + error: 'request timed out after 60 seconds' + }) + expect(requestInit().signal).toMatchObject({ aborted: true }) + }) + + it('rejects unsupported image types before any request is made', async () => { + const result = await submitFeedback( + imageSubmitArgs([{ contentType: 'application/pdf', data: new Uint8Array(4) }]) + ) + + expect(result.ok).toBe(false) + expect(fetchMock).not.toHaveBeenCalled() + }) + + // Why: the renderer screens types first, so this lane only matters for a + // renderer invoking the channel directly — the case the handler guards. + it('rejects a prototype member posing as a content type over IPC', async () => { + registerFeedbackHandlers() + const result = (await handlers.get('feedback:submit')?.(null, { + feedback: 'images attached', + githubLogin: null, + githubEmail: null, + images: [{ contentType: 'constructor', data: new Uint8Array(4).fill(1) }] + })) as { ok: boolean; error?: string } + + expect(result).toMatchObject({ ok: false, error: 'Unsupported image type.' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('rejects malformed IPC bytes before typed-array normalization', async () => { + registerFeedbackHandlers() + const result = (await handlers.get('feedback:submit')?.(null, { + feedback: 'images attached', + githubLogin: null, + githubEmail: null, + images: [{ contentType: 'image/png', data: '8388608' }] + })) as { ok: boolean; error?: string } + + expect(result).toMatchObject({ ok: false, error: 'Invalid image attachment bytes.' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('rejects oversized IPC batches before normalizing their entries', async () => { + registerFeedbackHandlers() + const result = (await handlers.get('feedback:submit')?.(null, { + feedback: 'images attached', + githubLogin: null, + githubEmail: null, + images: Array.from({ length: 5 }, () => ({ + contentType: 'image/png', + data: '8388608' + })) + })) as { ok: boolean; error?: string } + + expect(result).toMatchObject({ ok: false, error: 'Attach 4 images or fewer.' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('rejects more images than the supported count', async () => { + const result = await submitFeedback( + imageSubmitArgs(Array.from({ length: 5 }, () => pngImage())) + ) + + expect(result.ok).toBe(false) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('does not fail a crash report over images it was never going to send', async () => { + // Why: the crash lane discards images, so validating them there would + // abort a crash report the user needs delivered. + await submitFeedback({ + ...diagnosticSubmitArgs(), + images: Array.from({ length: 9 }, () => ({ + contentType: 'application/pdf', + data: new Uint8Array(0) + })) + } as Parameters[0]) + + expect(fetchMock).toHaveBeenCalledTimes(1) + const body = requestInit().body as FormData + expect(body.getAll('feedbackImage')).toHaveLength(0) + expect(body.get('submissionType')).toBe('crash') + }) + + it('drops images from crash submissions', async () => { + await submitFeedback({ + ...diagnosticSubmitArgs(), + images: [pngImage()] + } as Parameters[0]) + + const body = requestInit().body as FormData + expect(body.getAll('feedbackImage')).toHaveLength(0) + expect(body.get('diagnosticBundleSubmissionId')).toBe('bundleabcdefghijklmnop') + }) + }) }) diff --git a/src/main/ipc/feedback.ts b/src/main/ipc/feedback.ts index 09d8b5db0..f0ec8dbe0 100644 --- a/src/main/ipc/feedback.ts +++ b/src/main/ipc/feedback.ts @@ -1,5 +1,13 @@ import os from 'node:os' import { app, ipcMain, net } from 'electron' +import { + appendFeedbackImagesToFormData, + readFeedbackImagesDelivered, + validateFeedbackImages, + type FeedbackImageAttachment +} from './feedback-image-attachments' + +export type { FeedbackImageAttachment } // Why: the production Mac build loads the renderer from a file:// origin, so a // cross-origin POST from fetch() triggers a CORS preflight that the feedback @@ -21,6 +29,7 @@ export type FeedbackSubmitArgs = { submitAnonymously?: boolean githubLogin: string | null githubEmail: string | null + images?: FeedbackImageAttachment[] } export type FeedbackDiagnosticBundleAttachment = { @@ -40,6 +49,7 @@ type FeedbackSubmitBody = { osRelease: string arch: string diagnosticBundle?: FeedbackDiagnosticBundleAttachment + images?: FeedbackImageAttachment[] } export type FeedbackRequestFailure = { @@ -48,7 +58,12 @@ export type FeedbackRequestFailure = { } export type FeedbackSubmitResult = - | { ok: true; diagnosticBundleFailure?: FeedbackRequestFailure } + | { + ok: true + diagnosticBundleFailure?: FeedbackRequestFailure + /** Absent when nothing was attached; false when the text landed but the images did not. */ + imagesDelivered?: boolean + } | ({ ok: false } & FeedbackRequestFailure & { diagnosticBundleFailure?: FeedbackRequestFailure }) @@ -81,18 +96,21 @@ function buildSubmitBody(args: InternalFeedbackSubmitArgs): FeedbackSubmitBody { arch: process.arch, ...(args.submissionType === 'crash' && args.diagnosticBundle ? { diagnosticBundle: args.diagnosticBundle } - : {}) + : {}), + // Why: images are a feedback-only affordance; crash reports already carry + // diagnostic bundles and the server rejects images on that lane. + ...(args.submissionType !== 'crash' && args.images?.length ? { images: args.images } : {}) } } async function postFeedback( url: string, body: FeedbackSubmitBody, - timeoutMs = FEEDBACK_REQUEST_TIMEOUT_MS + timeoutMs = FEEDBACK_REQUEST_TIMEOUT_MS, + readResponse?: (response: Response) => Promise ): Promise { const controller = new AbortController() - // Why: a silent feedback endpoint should not leave IPC or crash-report - // submission flows pending forever. + // Why: a silent endpoint must not leave feedback IPC pending forever. const timeout = setTimeout(() => controller.abort(), timeoutMs) try { const init: RequestInit = { @@ -100,10 +118,18 @@ async function postFeedback( ...feedbackRequestBodyInit(body), signal: controller.signal } - return await net.fetch(url, init) + const response = await net.fetch(url, init) + if (readResponse) { + await readResponse(response) + } + // Why: a response parser may tolerate malformed legacy bodies, but it must + // not turn the deadline's aborted body into a confirmed delivery. + if (controller.signal.aborted) { + throw new Error(`request timed out after ${timeoutMs / 1000} seconds`) + } + return response } catch (error) { - // Why: Electron and Node use different AbortError messages. Normalize our - // client deadline so support logs explain which request budget expired. + // Why: Electron and Node report AbortError differently; keep deadline logs stable. if (controller.signal.aborted) { throw new Error(`request timed out after ${timeoutMs / 1000} seconds`) } @@ -114,7 +140,7 @@ async function postFeedback( } function feedbackRequestBodyInit(body: FeedbackSubmitBody): Pick { - if (!body.diagnosticBundle) { + if (!body.diagnosticBundle && !body.images?.length) { return { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body) @@ -130,22 +156,25 @@ function feedbackRequestBodyInit(body: FeedbackSubmitBody): Pick { + // Why: buildSubmitBody drops images on the crash lane, so validating them + // there would abort a crash report over attachments it never meant to send. + if (args.submissionType !== 'crash' && args.images !== undefined) { + const imageError = validateFeedbackImages(args.images) + if (imageError) { + return { ok: false, status: null, error: imageError } + } + } const body = buildSubmitBody(args) + if (body.images?.length) { + try { + let imagesDelivered = true + const response = await postFeedback( + FEEDBACK_API_URL, + body, + FEEDBACK_ATTACHMENT_REQUEST_TIMEOUT_MS, + async (nextResponse) => { + imagesDelivered = nextResponse.ok ? await readFeedbackImagesDelivered(nextResponse) : true + } + ) + if (response.ok) { + return { ok: true, imagesDelivered } + } + // Why: the text lane retries 5xx, this one does not. Replaying up to + // 32 MiB of attachments on a flaky link costs more than it saves, and the + // dialog keeps the draft and thumbnails so the user can resend. + return { ok: false, ...responseFailure(response) } + } catch (error) { + return { ok: false, ...errorFailure(error) } + } + } if (body.diagnosticBundle) { const bodyWithoutDiagnosticBundle = args.feedbackWithoutDiagnosticBundle !== undefined @@ -283,9 +342,20 @@ export async function submitFeedback( export function registerFeedbackHandlers(): void { ipcMain.removeHandler('feedback:submit') - ipcMain.handle('feedback:submit', (_event, args: FeedbackSubmitArgs) => + ipcMain.handle('feedback:submit', (_event, args: FeedbackSubmitArgs) => { + // Why: validate the raw clone before normalization so a tiny hostile value + // cannot become a large main-process typed-array allocation. + if (args.images !== undefined) { + const imageError = validateFeedbackImages(args.images) + if (imageError) { + return { ok: false, status: null, error: imageError } + } + } // Why: crash submissions are main-only. A compromised renderer can invoke // this channel directly, so force the public feedback lane at the boundary. - submitFeedback({ ...args, submissionType: 'feedback' }) - ) + return submitFeedback({ + ...args, + submissionType: 'feedback' + }) + }) } diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index c38585b0c..7a6d8199b 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1661,7 +1661,10 @@ export type PreloadApi = { submitAnonymously?: boolean githubLogin: string | null githubEmail: string | null - }) => Promise<{ ok: true } | { ok: false; status: number | null; error: string }> + images?: { contentType: string; data: Uint8Array }[] + }) => Promise< + { ok: true; imagesDelivered?: boolean } | { ok: false; status: number | null; error: string } + > } crashReports: { getLatestPending: () => Promise diff --git a/src/preload/index.ts b/src/preload/index.ts index 40153669d..cce974476 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1236,8 +1236,10 @@ const api = { submitAnonymously?: boolean githubLogin: string | null githubEmail: string | null - }): Promise<{ ok: true } | { ok: false; status: number | null; error: string }> => - ipcRenderer.invoke('feedback:submit', args) + images?: { contentType: string; data: Uint8Array }[] + }): Promise< + { ok: true; imagesDelivered?: boolean } | { ok: false; status: number | null; error: string } + > => ipcRenderer.invoke('feedback:submit', args) }, crashReports: { diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx new file mode 100644 index 000000000..70db571e7 --- /dev/null +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx @@ -0,0 +1,233 @@ +// @vitest-environment happy-dom + +import React, { act, type ReactNode } from 'react' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + readFeedbackImageFiles: vi.fn(), + submit: vi.fn(), + toastWarning: vi.fn() +})) + +vi.mock('sonner', () => ({ + toast: { + error: vi.fn(), + success: vi.fn(), + warning: mocks.toastWarning + } +})) + +vi.mock('@/components/ui/dialog', async () => { + const ReactModule = await import('react') + const Section = ({ children }: { children?: ReactNode }) =>
{children}
+ return { + Dialog: ({ children }: { children?: ReactNode }) => <>{children}, + DialogContent: ReactModule.forwardRef< + HTMLDivElement, + React.HTMLAttributes & { + children?: ReactNode + onOpenAutoFocus?: (event: Event) => void + } + >(function DialogContent({ children, onOpenAutoFocus: _onOpenAutoFocus, ...props }, ref) { + return ( +
+ {children} +
+ ) + }), + DialogDescription: Section, + DialogFooter: Section, + DialogHeader: Section, + DialogTitle: Section + } +}) + +vi.mock('@/lib/feedback-image-attachments', async (importOriginal) => { + const actual = await importOriginal>() + return { + ...actual, + readFeedbackImageFiles: mocks.readFeedbackImageFiles + } +}) + +import { SidebarFeedbackDialog } from './SidebarFeedbackDialog' + +beforeEach(() => { + mocks.readFeedbackImageFiles.mockReset() + mocks.submit.mockReset() + mocks.toastWarning.mockReset() + mocks.submit.mockResolvedValue({ ok: true }) + URL.revokeObjectURL = vi.fn() + Object.defineProperty(window, 'api', { + configurable: true, + value: { + feedback: { submit: mocks.submit }, + gh: { viewer: vi.fn().mockResolvedValue(null) }, + shell: { openUrl: vi.fn() } + } + }) +}) + +afterEach(() => { + cleanup() +}) + +describe('SidebarFeedbackDialog image submission', () => { + it('keeps the dialog scrollable within short windows', () => { + const { container } = render() + const content = container.querySelector('.scrollbar-sleek') + + expect(content?.className).toContain('max-h-[calc(100vh-3rem)]') + expect(content?.className).toContain('overflow-y-auto') + }) + + it('waits for in-flight image reads before enabling Send', async () => { + let finishRead: ((value: unknown) => void) | undefined + mocks.readFeedbackImageFiles.mockReturnValue( + new Promise((resolve) => { + finishRead = resolve + }) + ) + const { container } = render() + fireEvent.change(screen.getByPlaceholderText('What could we improve?'), { + target: { value: 'Screenshot attached' } + }) + const file = new File(['image'], 'shot.png', { type: 'image/png' }) + const input = container.querySelector('input[type="file"]') + expect(input).not.toBeNull() + fireEvent.change(input!, { target: { files: [file] } }) + + const send = screen.getByRole('button', { name: 'Send' }) + expect((send as HTMLButtonElement).disabled).toBe(true) + fireEvent.click(send) + expect(mocks.submit).not.toHaveBeenCalled() + + await act(async () => { + finishRead?.({ + images: [ + { + id: 'shot', + name: file.name, + contentType: file.type, + bytes: file.size, + data: new Uint8Array([1]), + previewUrl: 'blob:shot' + } + ], + errors: [] + }) + }) + + await waitFor(() => expect((send as HTMLButtonElement).disabled).toBe(false)) + const remove = screen.getByRole('button', { name: 'Remove shot.png' }) + expect(remove.dataset.slot).toBe('button') + expect(remove.dataset.size).toBe('icon-xs') + fireEvent.click(send) + await waitFor(() => expect(mocks.submit).toHaveBeenCalledTimes(1)) + expect(mocks.submit.mock.calls[0]?.[0].images).toEqual([ + { contentType: 'image/png', data: new Uint8Array([1]) } + ]) + }) + + it('warns when the server cannot confirm image delivery', async () => { + mocks.readFeedbackImageFiles.mockResolvedValue({ + images: [ + { + id: 'shot', + name: 'shot.png', + contentType: 'image/png', + bytes: 1, + data: new Uint8Array([1]), + previewUrl: 'blob:shot' + } + ], + errors: [] + }) + mocks.submit.mockResolvedValue({ ok: true, imagesDelivered: false }) + const onOpenChange = vi.fn() + const { container } = render() + fireEvent.change(screen.getByPlaceholderText('What could we improve?'), { + target: { value: 'Screenshot attached' } + }) + 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' }) + + fireEvent.click(screen.getByRole('button', { name: 'Send' })) + + await waitFor(() => + expect(mocks.toastWarning).toHaveBeenCalledWith( + 'Feedback sent, but image delivery could not be confirmed.' + ) + ) + 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: [], + errors: ['huge.png is larger than 8.0 MB.'] + }) + render() + const textarea = screen.getByPlaceholderText('What could we improve?') + const file = new File(['image'], 'huge.png', { type: 'image/png' }) + Object.defineProperty(file, 'size', { value: 8 * 1024 * 1024 + 1 }) + const paste = new Event('paste', { bubbles: true, cancelable: true }) + Object.defineProperty(paste, 'clipboardData', { + value: { files: [file] } + }) + + fireEvent(textarea, paste) + + expect(paste.defaultPrevented).toBe(false) + expect(mocks.readFeedbackImageFiles).toHaveBeenCalledWith([file], 0) + }) + + it('rejects images added after submission starts instead of clearing them unsent', async () => { + mocks.submit.mockReturnValue(new Promise(() => {})) + const { container } = render() + fireEvent.change(screen.getByPlaceholderText('What could we improve?'), { + target: { value: 'Initial report' } + }) + fireEvent.click(screen.getByRole('button', { name: 'Send' })) + await waitFor(() => expect(mocks.submit).toHaveBeenCalledTimes(1)) + const file = new File(['image'], 'late.png', { type: 'image/png' }) + const input = container.querySelector('input[type="file"]') + + fireEvent.change(input!, { target: { files: [file] } }) + + expect(mocks.readFeedbackImageFiles).not.toHaveBeenCalled() + expect(mocks.toastWarning).toHaveBeenCalledWith( + 'Wait for the current feedback to finish sending before attaching more images.' + ) + }) +}) diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx index f6b67994d..d556f9233 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx @@ -15,6 +15,15 @@ import { useMountedRef } from '@/hooks/useMountedRef' import { cn } from '@/lib/utils' import type { GitHubViewer } from '../../../../shared/types' import { translate } from '@/i18n/i18n' +import { + extractImageFilesFromDataTransfer, + hasAttachableFeedbackImage, + readFeedbackImageFiles, + releaseFeedbackImageDraft, + type FeedbackImageDraft +} from '@/lib/feedback-image-attachments' +import { SidebarFeedbackImageAttachments } from './SidebarFeedbackImageAttachments' +import { useFeedbackImageDrop } from './use-feedback-image-drop' const GITHUB_ISSUES_URL = 'https://github.com/stablyai/orca/issues/' const DISCORD_URL = 'https://discord.gg/fzjDKHxv8Q' @@ -57,8 +66,98 @@ export function SidebarFeedbackDialog({ const [viewer, setViewer] = useState(null) const [isViewerLoading, setIsViewerLoading] = useState(false) const [submitAnonymously, setSubmitAnonymously] = useState(false) + const [images, setImages] = useState([]) + const [pendingImageReadCount, setPendingImageReadCount] = useState(0) const mountedRef = useMountedRef() const feedbackTextareaRef = useRef(null) + const liveImageDraftsRef = useRef([]) + + const clearImages = React.useCallback(() => { + 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( + () => () => { + liveImageDraftsRef.current.forEach(releaseFeedbackImageDraft) + liveImageDraftsRef.current = [] + }, + [] + ) + + const imageCount = images.length + + // Why: committed state lags the in-flight reads, so batches still being read + // count against capacity — otherwise two quick pastes both see room for four. + const pendingImageReadsRef = useRef(0) + + const handleAddFiles = React.useCallback( + (files: readonly File[]) => { + if (files.length === 0) { + return + } + if (isSubmitting) { + toast.warning( + translate( + 'auto.components.sidebar.SidebarFeedbackDialog.attachWhileSending', + 'Wait for the current feedback to finish sending before attaching more images.' + ) + ) + return + } + // Why: read the committed count from the closure rather than a ref. A ref + // synced in an effect can still be stale-low right after an add, which + // over-accepts and gets the whole submission rejected by the main process. + const existingCount = imageCount + pendingImageReadsRef.current + pendingImageReadsRef.current += files.length + setPendingImageReadCount((current) => current + files.length) + void readFeedbackImageFiles(files, existingCount).then( + ({ images: added, errors }) => { + pendingImageReadsRef.current -= files.length + if (!mountedRef.current) { + added.forEach(releaseFeedbackImageDraft) + return + } + 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 + // silence is what made screenshots vanish in the first place. + errors.forEach((error) => toast.warning(error)) + }, + (error: unknown) => { + pendingImageReadsRef.current -= files.length + console.error('Failed to read feedback image attachments:', error) + if (mountedRef.current) { + setPendingImageReadCount((current) => Math.max(0, current - files.length)) + toast.error( + translate( + 'auto.components.sidebar.SidebarFeedbackDialog.imageReadFailed', + 'Could not read the attached images. Try attaching them again.' + ) + ) + } + } + ) + }, + [imageCount, isSubmitting, mountedRef] + ) + + const handleRemoveImage = React.useCallback((id: string) => { + 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) React.useEffect(() => { if (!open) { @@ -92,6 +191,9 @@ export function SidebarFeedbackDialog({ }, [open]) const handleSubmit = async (): Promise => { + if (isSubmitting || pendingImageReadsRef.current > 0) { + return + } const trimmed = feedback.trim() if (!trimmed) { toast.warning( @@ -115,7 +217,11 @@ export function SidebarFeedbackDialog({ feedback: trimmed, submitAnonymously, githubLogin: identity.githubLogin, - githubEmail: identity.githubEmail + githubEmail: identity.githubEmail, + images: images.map((image) => ({ + contentType: image.contentType, + data: image.data + })) }) if (!result.ok) { @@ -123,14 +229,26 @@ export function SidebarFeedbackDialog({ } if (mountedRef.current) { - toast.success( - translate( - 'auto.components.sidebar.SidebarFeedbackDialog.7a46c228b8', - 'Thanks for the feedback.' + // Why: the text reached us but the screenshots did not, so say that + // plainly instead of a blanket success the user would misread. + if (result.imagesDelivered === false) { + toast.warning( + translate( + 'auto.components.sidebar.SidebarFeedbackDialog.imagesNotDelivered', + 'Feedback sent, but image delivery could not be confirmed.' + ) ) - ) + } else { + toast.success( + translate( + 'auto.components.sidebar.SidebarFeedbackDialog.7a46c228b8', + 'Thanks for the feedback.' + ) + ) + } setFeedback('') setSubmitAnonymously(false) + clearImages() onOpenChange(false) } } catch (err) { @@ -153,11 +271,30 @@ export function SidebarFeedbackDialog({ return ( { event.preventDefault() feedbackTextareaRef.current?.focus() }} + // Why: paste is bound on the dialog rather than the textarea so a + // screenshot lands whether or not the caret is in the message box. + onPaste={(event) => { + const pasted = extractImageFilesFromDataTransfer(event.clipboardData) + if (pasted.length === 0) { + return + } + // Why: consume the paste only when something is actually attachable. + // An unsupported image still routes through for its rejection toast, + // but preventing default there would silently eat co-pasted text. + if (hasAttachableFeedbackImage(pasted, imageCount + pendingImageReadsRef.current)) { + event.preventDefault() + } + handleAddFiles(pasted) + }} + // Why: dragenter/leave fire per nested child; the hook counts depth so + // the highlight only clears once the pointer leaves the dialog. + {...dragHandlers} > @@ -237,6 +374,14 @@ export function SidebarFeedbackDialog({ className="min-h-32 w-full rounded-md border border-border bg-background px-3 py-2 text-sm outline-none ring-offset-background placeholder:text-muted-foreground focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2" /> + +
{viewer ? (
@@ -283,7 +428,10 @@ export function SidebarFeedbackDialog({ - +
+ + { + onAddFiles(Array.from(event.target.files ?? [])) + // Why: reset so re-picking the same file still fires onChange. + event.target.value = '' + }} + /> + + {images.length > 0 && ( +
    + {images.map((image) => ( +
  • + {image.name} + + + {formatFeedbackImageSize(image.bytes)} + +
    • + ))} +
+ )} +
+ ) +} diff --git a/src/renderer/src/components/sidebar/use-feedback-image-drop.test.tsx b/src/renderer/src/components/sidebar/use-feedback-image-drop.test.tsx new file mode 100644 index 000000000..ced2153fb --- /dev/null +++ b/src/renderer/src/components/sidebar/use-feedback-image-drop.test.tsx @@ -0,0 +1,185 @@ +// @vitest-environment happy-dom + +/** + * Native file drops never reach React in this app: preload claims them on + * document capture and routes the paths to the editor. These tests pin the + * window-capture interception that keeps a screenshot dropped on the feedback + * dialog from being swallowed by that lane. + */ +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { ORCA_INTERNAL_FILE_DRAG_TYPE } from '../../../../shared/native-file-drop' +import { useFeedbackImageDrop } from './use-feedback-image-drop' + +let container: HTMLDivElement +let root: Root +let preloadDropSpy: ReturnType void>> + +function preloadDropListener(event: Event): void { + preloadDropSpy(event) + // Why: preload consumes the gesture, which is why React's onDrop never runs. + event.preventDefault() + event.stopPropagation() +} + +beforeEach(() => { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + preloadDropSpy = vi.fn() + // Registered before the hook mounts, exactly like preload's document listener. + document.addEventListener('drop', preloadDropListener, true) +}) + +afterEach(() => { + document.removeEventListener('drop', preloadDropListener, true) + act(() => { + root.unmount() + }) + container.remove() + document.body.innerHTML = '' +}) + +function Harness({ + open, + onAddFiles +}: { + open: boolean + onAddFiles: (files: readonly File[]) => void +}): React.JSX.Element { + const { isDragActive, contentRef, dragHandlers } = useFeedbackImageDrop(open, onAddFiles) + return ( +
+ child +
+ ) +} + +async function renderHarness( + open: boolean, + onAddFiles: (files: readonly File[]) => void +): Promise { + await act(async () => { + root.render() + }) +} + +function dragEvent(type: string, files: File[], types: string[] = ['Files']): Event { + const event = new Event(type, { bubbles: true, cancelable: true }) + Object.defineProperty(event, 'dataTransfer', { value: { files, types } }) + return event +} + +function pngFile(name = 'shot.png'): File { + return new File(['x'], name, { type: 'image/png' }) +} + +function dialogChild(): HTMLElement { + const child = container.querySelector('[data-testid="child"]') + if (!child) { + throw new Error('harness child missing') + } + return child as HTMLElement +} + +describe('useFeedbackImageDrop', () => { + it('claims an image dropped on the dialog before preload can route it away', async () => { + const onAddFiles = vi.fn() + await renderHarness(true, onAddFiles) + + const event = dragEvent('drop', [pngFile()]) + act(() => { + dialogChild().dispatchEvent(event) + }) + + expect(onAddFiles).toHaveBeenCalledTimes(1) + expect(onAddFiles.mock.calls[0][0].map((file: File) => file.name)).toEqual(['shot.png']) + expect(event.defaultPrevented).toBe(true) + expect(preloadDropSpy).not.toHaveBeenCalled() + }) + + it('leaves drops outside the dialog to the existing native lane', async () => { + const onAddFiles = vi.fn() + await renderHarness(true, onAddFiles) + + const outside = document.createElement('div') + document.body.appendChild(outside) + act(() => { + outside.dispatchEvent(dragEvent('drop', [pngFile()])) + }) + + expect(onAddFiles).not.toHaveBeenCalled() + expect(preloadDropSpy).toHaveBeenCalledTimes(1) + }) + + it('ignores non-image drops so they keep their existing behavior', async () => { + const onAddFiles = vi.fn() + await renderHarness(true, onAddFiles) + + const event = dragEvent('drop', [new File(['x'], 'notes.txt', { type: 'text/plain' })]) + act(() => { + dialogChild().dispatchEvent(event) + }) + + expect(onAddFiles).not.toHaveBeenCalled() + expect(preloadDropSpy).toHaveBeenCalledTimes(1) + // Why: dragover accepted the drag, so an uncancelled drop navigates the web + // client to the file; preload still gets it because propagation continues. + expect(event.defaultPrevented).toBe(true) + }) + + it('stops listening once the dialog is closed', async () => { + const onAddFiles = vi.fn() + await renderHarness(true, onAddFiles) + await renderHarness(false, onAddFiles) + + act(() => { + dialogChild().dispatchEvent(dragEvent('drop', [pngFile()])) + }) + + expect(onAddFiles).not.toHaveBeenCalled() + expect(preloadDropSpy).toHaveBeenCalledTimes(1) + }) + + // Why: only preload preventDefaults dragover, and the web client has no + // preload — without this the browser rejects the drop and opens the file. + it('accepts the drag on dragover so the drop can fire without preload', async () => { + await renderHarness(true, vi.fn()) + + const event = dragEvent('dragover', []) + act(() => { + dialogChild().dispatchEvent(event) + }) + + expect(event.defaultPrevented).toBe(true) + expect((event as DragEvent).dataTransfer?.dropEffect).toBe('copy') + }) + + it('leaves in-app drags alone on dragover', async () => { + await renderHarness(true, vi.fn()) + + const event = dragEvent('dragover', [], ['Files', ORCA_INTERNAL_FILE_DRAG_TYPE]) + act(() => { + dialogChild().dispatchEvent(event) + }) + + expect(event.defaultPrevented).toBe(false) + }) + + it('highlights from the advertised drag types, which is all a dragenter exposes', async () => { + await renderHarness(true, vi.fn()) + const dialog = container.querySelector('[data-testid="dialog"]') as HTMLElement + + // Why: DataTransfer.files is empty until drop; only `types` is populated. + act(() => { + dialogChild().dispatchEvent(dragEvent('dragenter', [])) + }) + expect(dialog.dataset.dragActive).toBe('true') + + act(() => { + dialogChild().dispatchEvent(dragEvent('dragleave', [])) + }) + expect(dialog.dataset.dragActive).toBe('false') + }) +}) diff --git a/src/renderer/src/components/sidebar/use-feedback-image-drop.ts b/src/renderer/src/components/sidebar/use-feedback-image-drop.ts new file mode 100644 index 000000000..5a0fe5989 --- /dev/null +++ b/src/renderer/src/components/sidebar/use-feedback-image-drop.ts @@ -0,0 +1,101 @@ +import React, { useCallback, useEffect, useRef, useState } from 'react' +import { hasNativeFileDragTypes } from '../../../../shared/native-file-drop' +import { extractImageFilesFromDataTransfer } from '@/lib/feedback-image-attachments' + +type FeedbackImageDragHandlers = { + onDragEnter: (event: React.DragEvent) => void + onDragOver: (event: React.DragEvent) => void + onDragLeave: (event: React.DragEvent) => void +} + +export type FeedbackImageDrop = { + isDragActive: boolean + contentRef: React.RefObject + dragHandlers: FeedbackImageDragHandlers +} + +/** + * Drag-and-drop attachment wiring for the feedback dialog. Native file drops + * never reach React here: preload consumes them on document capture and routes + * the paths to the editor, so the drop is claimed one phase earlier on window. + */ +export function useFeedbackImageDrop( + open: boolean, + onAddFiles: (files: readonly File[]) => void +): FeedbackImageDrop { + const [isDragActive, setIsDragActive] = useState(false) + const contentRef = useRef(null) + const dragDepthRef = useRef(0) + + const reset = useCallback(() => { + dragDepthRef.current = 0 + setIsDragActive(false) + }, []) + + // Why: DataTransfer.files is empty until the drop lands, so the highlight has + // to key off the drag types the OS advertises during the drag itself. + const onDragEnter = useCallback((event: React.DragEvent) => { + if (!hasNativeFileDragTypes(event.dataTransfer.types)) { + return + } + dragDepthRef.current += 1 + setIsDragActive(true) + }, []) + + // Why: the web client has no preload to preventDefault dragover for it, and + // without that the browser refuses the drop and navigates to the dropped file. + const onDragOver = useCallback((event: React.DragEvent) => { + if (!hasNativeFileDragTypes(event.dataTransfer.types)) { + return + } + event.preventDefault() + event.dataTransfer.dropEffect = 'copy' + }, []) + + const onDragLeave = useCallback((event: React.DragEvent) => { + // Why: mirror the enter guard so internal drags can't decrement a counter + // enter never incremented. + if (!hasNativeFileDragTypes(event.dataTransfer.types)) { + return + } + dragDepthRef.current = Math.max(0, dragDepthRef.current - 1) + if (dragDepthRef.current === 0) { + setIsDragActive(false) + } + }, []) + + useEffect(() => { + if (!open) { + return + } + const handleDrop = (event: DragEvent): void => { + const droppedInDialog = contentRef.current?.contains(event.target as Node) ?? false + reset() + if (!droppedInDialog || !hasNativeFileDragTypes(event.dataTransfer?.types)) { + return + } + // Why: dragover accepted this drag, and a drop left uncancelled after that + // is what makes the browser navigate to the file — including the non-image + // drops below, which would otherwise wipe the typed feedback on web. + event.preventDefault() + const images = extractImageFilesFromDataTransfer(event.dataTransfer) + if (images.length === 0) { + return + } + // Why: stop preload's native-drop lane from also opening the screenshot + // in an editor behind the dialog. + event.stopPropagation() + onAddFiles(images) + } + window.addEventListener('drop', handleDrop, true) + window.addEventListener('dragend', reset, true) + return () => { + window.removeEventListener('drop', handleDrop, true) + window.removeEventListener('dragend', reset, true) + // Why: a dialog closed mid-drag would otherwise reopen mid-highlight. + reset() + } + }, [onAddFiles, open, reset]) + + return { isDragActive, contentRef, dragHandlers: { onDragEnter, onDragOver, onDragLeave } } +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 69aafa8d2..c20c32966 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -658,6 +658,20 @@ }, "pluginCommandKeybindings": { "group": "Plugins" + }, + "feedback": { + "image": { + "attachments": { + "fallbackName": "Image attachment", + "unsupportedType": "{{fileName}} is not a supported image type.", + "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." + } + } } }, "hooks": { @@ -4224,7 +4238,10 @@ "7a46c228b8": "Thanks for the feedback.", "a2fd890d9e": "Please enter feedback before submitting.", "f2e42e1307": "Send", - "69969ba364": "Sending…" + "69969ba364": "Sending…", + "imageReadFailed": "Could not read the attached images. Try attaching them again.", + "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "Add project", @@ -5036,6 +5053,11 @@ "developer": "Developer", "parkTerminal": "Park terminal" }, + "SidebarFeedbackImageAttachments": { + "screenshotsHint": "Attach up to {count} screenshots", + "attachImages": "Attach", + "removeImage": "Remove {{fileName}}" + }, "WorkspaceKanbanSearchField": { "bdb753c78d": "No workspaces match", "4d96c209d6": "{{value0}} of {{value1}} workspaces match", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 3e7fa4b28..a0ef85e94 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -635,6 +635,20 @@ }, "pluginCommandKeybindings": { "group": "Plugins" + }, + "feedback": { + "image": { + "attachments": { + "fallbackName": "Image attachment", + "unsupportedType": "{{fileName}} is not a supported image type.", + "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." + } + } } }, "hooks": { @@ -4182,7 +4196,10 @@ "7a46c228b8": "Gracias por el feedback.", "a2fd890d9e": "Introduce feedback antes de enviarlo.", "f2e42e1307": "Enviar", - "69969ba364": "Envío…" + "69969ba364": "Envío…", + "imageReadFailed": "Could not read the attached images. Try attaching them again.", + "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "Agregar proyecto", @@ -5009,6 +5026,11 @@ "developer": "Developer", "parkTerminal": "Park terminal" }, + "SidebarFeedbackImageAttachments": { + "screenshotsHint": "Attach up to {count} screenshots", + "attachImages": "Attach", + "removeImage": "Remove {{fileName}}" + }, "WorkspaceKanbanSearchField": { "bdb753c78d": "Ningún espacio de trabajo coincide", "4d96c209d6": "{{value0}} de {{value1}} espacios de trabajo coinciden", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index a2cad633e..b5c473513 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -635,6 +635,20 @@ }, "pluginCommandKeybindings": { "group": "Plugins" + }, + "feedback": { + "image": { + "attachments": { + "fallbackName": "Image attachment", + "unsupportedType": "{{fileName}} is not a supported image type.", + "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." + } + } } }, "hooks": { @@ -4163,7 +4177,10 @@ "7a46c228b8": "フィードバックありがとうございます。", "a2fd890d9e": "送信する前にフィードバックを入力。", "f2e42e1307": "送信", - "69969ba364": "送信中…" + "69969ba364": "送信中…", + "imageReadFailed": "Could not read the attached images. Try attaching them again.", + "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "プロジェクトの追加", @@ -5009,6 +5026,11 @@ "developer": "Developer", "parkTerminal": "Park terminal" }, + "SidebarFeedbackImageAttachments": { + "screenshotsHint": "Attach up to {count} screenshots", + "attachImages": "Attach", + "removeImage": "Remove {{fileName}}" + }, "WorkspaceKanbanSearchField": { "bdb753c78d": "一致するワークスペースはありません", "4d96c209d6": "{{value1}} 件中 {{value0}} 件のワークスペースが一致します", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 84a2b694f..2d5d50d44 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -635,6 +635,20 @@ }, "pluginCommandKeybindings": { "group": "Plugins" + }, + "feedback": { + "image": { + "attachments": { + "fallbackName": "Image attachment", + "unsupportedType": "{{fileName}} is not a supported image type.", + "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." + } + } } }, "hooks": { @@ -4163,7 +4177,10 @@ "7a46c228b8": "피드백을 보내주셔서 감사합니다.", "a2fd890d9e": "제출하기 전에 피드백을 입력해 주세요.", "f2e42e1307": "보내기", - "69969ba364": "전송 중…" + "69969ba364": "전송 중…", + "imageReadFailed": "Could not read the attached images. Try attaching them again.", + "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "프로젝트 추가", @@ -5009,6 +5026,11 @@ "developer": "Developer", "parkTerminal": "Park terminal" }, + "SidebarFeedbackImageAttachments": { + "screenshotsHint": "Attach up to {count} screenshots", + "attachImages": "Attach", + "removeImage": "Remove {{fileName}}" + }, "WorkspaceKanbanSearchField": { "bdb753c78d": "일치하는 워크스페이스가 없습니다", "4d96c209d6": "워크스페이스 {{value1}}개 중 {{value0}}개 일치", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 6d56383d8..5dc222ce6 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -635,6 +635,20 @@ }, "pluginCommandKeybindings": { "group": "Plugins" + }, + "feedback": { + "image": { + "attachments": { + "fallbackName": "Image attachment", + "unsupportedType": "{{fileName}} is not a supported image type.", + "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." + } + } } }, "hooks": { @@ -4163,7 +4177,10 @@ "7a46c228b8": "感谢您的反馈。", "a2fd890d9e": "请在提交前输入反馈。", "f2e42e1307": "发送", - "69969ba364": "正在发送..." + "69969ba364": "正在发送...", + "imageReadFailed": "Could not read the attached images. Try attaching them again.", + "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "添加项目", @@ -5009,6 +5026,11 @@ "developer": "Developer", "parkTerminal": "Park terminal" }, + "SidebarFeedbackImageAttachments": { + "screenshotsHint": "Attach up to {count} screenshots", + "attachImages": "Attach", + "removeImage": "Remove {{fileName}}" + }, "WorkspaceKanbanSearchField": { "bdb753c78d": "没有匹配的工作区", "4d96c209d6": "{{value1}} 个工作区中有 {{value0}} 个匹配", diff --git a/src/renderer/src/lib/feedback-image-attachments.test.ts b/src/renderer/src/lib/feedback-image-attachments.test.ts new file mode 100644 index 000000000..c7f1200de --- /dev/null +++ b/src/renderer/src/lib/feedback-image-attachments.test.ts @@ -0,0 +1,183 @@ +// @vitest-environment happy-dom + +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + MAX_FEEDBACK_IMAGE_BYTES, + MAX_FEEDBACK_IMAGE_COUNT, + hasAttachableFeedbackImage, + readFeedbackImageFiles +} from './feedback-image-attachments' + +beforeEach(() => { + let next = 0 + URL.createObjectURL = vi.fn(() => `blob:feedback-${(next += 1)}`) + URL.revokeObjectURL = vi.fn() +}) + +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 +} + +describe('hasAttachableFeedbackImage', () => { + it('is true when any file is an allow-listed type', () => { + const svg = new File(['x'], 'a.svg', { type: 'image/svg+xml' }) + expect(hasAttachableFeedbackImage([svg, pngFile('a.png')])).toBe(true) + }) + + // Why: the paste handler only consumes the event when this is true. An + // image/* type outside the allow-list must still fall through so co-pasted + // text is not swallowed, while readFeedbackImageFiles raises its toast. + it('is false when every file is an unsupported image type', () => { + const svg = new File(['x'], 'a.svg', { type: 'image/svg+xml' }) + const bmp = new File(['x'], 'a.bmp', { type: 'image/bmp' }) + expect(hasAttachableFeedbackImage([svg, bmp])).toBe(false) + }) + + it('is false for an empty selection', () => { + expect(hasAttachableFeedbackImage([])).toBe(false) + }) + + it('is false when supported files cannot pass validation', () => { + expect(hasAttachableFeedbackImage([pngFile('empty.png', 0)])).toBe(false) + expect(hasAttachableFeedbackImage([pngFile('huge.png', MAX_FEEDBACK_IMAGE_BYTES + 1)])).toBe( + false + ) + expect(hasAttachableFeedbackImage([pngFile('a.png')], MAX_FEEDBACK_IMAGE_COUNT)).toBe(false) + }) +}) + +describe('readFeedbackImageFiles', () => { + it('reads supported images into drafts with distinct ids', async () => { + const { images, errors } = await readFeedbackImageFiles([pngFile('a.png'), pngFile('a.png')], 0) + + expect(errors).toEqual([]) + expect(images).toHaveLength(2) + expect(new Set(images.map((image) => image.id)).size).toBe(2) + expect(images[0].data.byteLength).toBe(24) + }) + + it('reports an unsupported type instead of skipping it', async () => { + const { images, errors } = await readFeedbackImageFiles( + [new File(['x'], 'notes.pdf', { type: 'application/pdf' })], + 0 + ) + + expect(images).toEqual([]) + expect(errors).toEqual(['notes.pdf is not a supported image type.']) + }) + + it('caps rejection detail so a large drop cannot mount one toast per file', async () => { + const files = Array.from( + { length: 100 }, + (_, index) => new File(['x'], `image-${index}.svg`, { type: 'image/svg+xml' }) + ) + + const { images, errors } = await readFeedbackImageFiles(files, 0) + + expect(images).toEqual([]) + expect(errors).toHaveLength(5) + expect(errors.at(-1)).toBe('96 additional images could not be attached.') + }) + + it('reports an oversized image instead of skipping it', async () => { + const { images, errors } = await readFeedbackImageFiles( + [pngFile('huge.png', MAX_FEEDBACK_IMAGE_BYTES + 1)], + 0 + ) + + expect(images).toEqual([]) + expect(errors).toEqual(['huge.png is larger than 8.0 MB.']) + }) + + it('reports an empty image instead of deferring rejection until submit', async () => { + const { images, errors } = await readFeedbackImageFiles([pngFile('empty.png', 0)], 0) + + expect(images).toEqual([]) + 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')], + MAX_FEEDBACK_IMAGE_COUNT + ) + + expect(images).toEqual([]) + expect(errors).toEqual([`You can attach up to ${MAX_FEEDBACK_IMAGE_COUNT} images.`]) + }) + + it('revokes previews already created when a later read in the batch fails', async () => { + const good = pngFile('good.png') + const broken = pngFile('broken.png') + broken.arrayBuffer = () => Promise.reject(new Error('file went away')) + + await expect(readFeedbackImageFiles([good, broken], 0)).rejects.toThrow('file went away') + expect(URL.revokeObjectURL).toHaveBeenCalledWith('blob:feedback-1') + }) + + it('does not depend on crypto.randomUUID, which LAN web clients do not expose', async () => { + const realCrypto = globalThis.crypto + Object.defineProperty(globalThis, 'crypto', { + configurable: true, + value: { getRandomValues: realCrypto.getRandomValues.bind(realCrypto) } + }) + try { + const { images, errors } = await readFeedbackImageFiles([pngFile('a.png')], 0) + expect(errors).toEqual([]) + expect(images).toHaveLength(1) + } finally { + Object.defineProperty(globalThis, 'crypto', { configurable: true, value: realCrypto }) + } + }) +}) diff --git a/src/renderer/src/lib/feedback-image-attachments.ts b/src/renderer/src/lib/feedback-image-attachments.ts new file mode 100644 index 000000000..69915218c --- /dev/null +++ b/src/renderer/src/lib/feedback-image-attachments.ts @@ -0,0 +1,194 @@ +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 +export const SUPPORTED_FEEDBACK_IMAGE_TYPES = [ + 'image/png', + 'image/jpeg', + 'image/webp', + 'image/gif' +] as const + +export const FEEDBACK_IMAGE_FILE_ACCEPT = SUPPORTED_FEEDBACK_IMAGE_TYPES.join(',') +const MAX_FEEDBACK_IMAGE_DETAIL_ERRORS = 4 + +export type FeedbackImageDraft = { + id: string + name: string + contentType: string + bytes: number + data: Uint8Array + /** Object URL for the thumbnail; revoke with releaseFeedbackImageDraft. */ + previewUrl: string +} + +function isSupportedType(contentType: string): boolean { + return (SUPPORTED_FEEDBACK_IMAGE_TYPES as readonly string[]).includes(contentType) +} + +/** + * Whether a paste should be consumed. Extraction stays broad so unsupported + * image types still reach the rejection toast, but swallowing the paste when + * nothing is attachable would also discard any text riding along on the + * clipboard. + */ +export function hasAttachableFeedbackImage(files: readonly File[], existingCount = 0): boolean { + return ( + existingCount < MAX_FEEDBACK_IMAGE_COUNT && + files.some( + (file) => isSupportedType(file.type) && file.size > 0 && file.size <= MAX_FEEDBACK_IMAGE_BYTES + ) + ) +} + +export function releaseFeedbackImageDraft(draft: FeedbackImageDraft): void { + URL.revokeObjectURL(draft.previewUrl) +} + +export function formatFeedbackImageSize(bytes: number): string { + return bytes >= 1024 * 1024 + ? `${(bytes / (1024 * 1024)).toFixed(1)} MB` + : `${Math.max(1, Math.round(bytes / 1024))} KB` +} + +function feedbackImageDisplayName(file: File): string { + return ( + file.name || translate('auto.lib.feedback.image.attachments.fallbackName', 'Image attachment') + ) +} + +/** + * Converts picked/pasted/dropped files into drafts. Rejections come back as + * messages rather than being skipped, because silently dropping an attachment + * is the exact failure this feature exists to fix. + */ +export async function readFeedbackImageFiles( + files: readonly File[], + existingCount: number +): Promise<{ images: FeedbackImageDraft[]; errors: string[] }> { + const images: FeedbackImageDraft[] = [] + const errors: string[] = [] + let remaining = MAX_FEEDBACK_IMAGE_COUNT - existingCount + let omittedErrorCount = 0 + const addError = (createMessage: () => string): void => { + if (errors.length < MAX_FEEDBACK_IMAGE_DETAIL_ERRORS) { + errors.push(createMessage()) + } else { + omittedErrorCount += 1 + } + } + + try { + for (const file of files) { + const fileName = feedbackImageDisplayName(file) + if (!isSupportedType(file.type)) { + addError(() => + translate( + 'auto.lib.feedback.image.attachments.unsupportedType', + '{{fileName}} is not a supported image type.', + { fileName } + ) + ) + continue + } + if (file.size === 0) { + addError(() => + translate('auto.lib.feedback.image.attachments.empty', '{{fileName}} is empty.', { + fileName + }) + ) + continue + } + if (file.size > MAX_FEEDBACK_IMAGE_BYTES) { + addError(() => + translate( + 'auto.lib.feedback.image.attachments.tooLarge', + '{{fileName}} is larger than {{maxSize}}.', + { + fileName, + maxSize: formatFeedbackImageSize(MAX_FEEDBACK_IMAGE_BYTES) + } + ) + ) + continue + } + if (remaining <= 0) { + addError(() => + translate( + 'auto.lib.feedback.image.attachments.tooMany', + 'You can attach up to {{maxCount}} images.', + { maxCount: MAX_FEEDBACK_IMAGE_COUNT } + ) + ) + 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 + // web client over plain HTTP); createBrowserUuid falls back safely. + id: `${file.name}-${file.size}-${createBrowserUuid()}`, + name: fileName, + contentType: file.type, + bytes: file.size, + data, + previewUrl: URL.createObjectURL(file) + }) + } + } catch (error) { + // Why: a rejected read never returns these drafts, and an un-revoked object + // URL pins its blob for the life of the renderer. + images.forEach(releaseFeedbackImageDraft) + throw error + } + + if (omittedErrorCount > 0) { + errors.push( + translate( + 'auto.lib.feedback.image.attachments.additionalErrors', + '{{count}} additional images could not be attached.', + { count: omittedErrorCount } + ) + ) + } + + return { images, errors } +} + +export function extractImageFilesFromDataTransfer(data: DataTransfer | null): File[] { + if (!data) { + return [] + } + return Array.from(data.files).filter((file) => file.type.startsWith('image/')) +}