From e8e7d2ea8f88598cb6e6cde06958d5b3e8a2935f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 1 Apr 2026 15:12:50 -0700 Subject: [PATCH] feat: add PDF file preview support in diff viewer (#252) * fix: address review findings - Add sandbox="allow-same-origin" to PDF iframes to prevent script execution - Wrap atob() in try/catch to gracefully handle corrupt base64 data - Reset imageError state on content change to allow re-rendering - Add click overlay on PDF iframe so popup open works correctly - Compact test mock resets to stay within max-lines lint rule * fix: consolidate binary preview tests to stay within max-lines limit --- src/main/git/status.test.ts | 18 ++ src/main/git/status.ts | 24 ++- src/main/ipc/filesystem.test.ts | 75 ++++---- src/main/ipc/filesystem.ts | 10 +- .../src/components/editor/ImageDiffViewer.tsx | 2 +- .../src/components/editor/ImageViewer.tsx | 182 +++++++++++++----- src/shared/types.ts | 4 +- 7 files changed, 212 insertions(+), 103 deletions(-) diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 6cc8616fe..7fa78d0f9 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -151,6 +151,24 @@ describe('getDiff', () => { expect(result.originalIsBinary).toBe(true) expect(result.modifiedIsBinary).toBe(false) }) + + it('includes preview metadata for pdf diffs', async () => { + const pdfBuffer = Buffer.from([0x25, 0x50, 0x44, 0x46, 0x00]) + execFileAsyncMock.mockResolvedValueOnce({ stdout: pdfBuffer }) + readFileMock.mockResolvedValue(pdfBuffer) + + const result = await getDiff('/repo', 'docs/spec.pdf', false) + + expect(result).toEqual({ + kind: 'binary', + originalContent: pdfBuffer.toString('base64'), + modifiedContent: pdfBuffer.toString('base64'), + originalIsBinary: true, + modifiedIsBinary: true, + isImage: true, + mimeType: 'application/pdf' + }) + }) }) describe('getStatus', () => { diff --git a/src/main/git/status.ts b/src/main/git/status.ts index d1a45f98e..6f52ac3c6 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -579,9 +579,15 @@ async function readWorkingTreeFile(filePath: string): Promise function bufferToBlob(buffer: Buffer, filePath?: string): GitBlobReadResult { const isBinary = isBinaryBuffer(buffer) // Return base64 for recognized image formats so the renderer can display them - const isImage = filePath ? !!IMAGE_MIME_TYPES[path.extname(filePath).toLowerCase()] : false + const isPreviewableBinary = filePath + ? !!PREVIEWABLE_BINARY_MIME_TYPES[path.extname(filePath).toLowerCase()] + : false return { - content: isBinary ? (isImage ? buffer.toString('base64') : '') : buffer.toString('utf-8'), + content: isBinary + ? isPreviewableBinary + ? buffer.toString('base64') + : '' + : buffer.toString('utf-8'), isBinary, exists: true } @@ -605,15 +611,18 @@ function buildDiffResult( filePath?: string ): GitDiffResult { if (originalIsBinary || modifiedIsBinary) { - const mimeType = filePath ? IMAGE_MIME_TYPES[path.extname(filePath).toLowerCase()] : undefined + const mimeType = filePath + ? PREVIEWABLE_BINARY_MIME_TYPES[path.extname(filePath).toLowerCase()] + : undefined return { kind: 'binary', originalContent, modifiedContent, originalIsBinary, modifiedIsBinary, - // Include image metadata so the renderer can show image diffs instead of - // a generic "binary file changed" message. + // Why: binary diff previews were originally image-only, so the renderer + // still checks `isImage` before showing a preview component. Preserve + // that legacy flag for PDFs until the wider contract is renamed. ...(mimeType ? { isImage: true, mimeType } : {}) } as GitDiffResult } @@ -633,7 +642,7 @@ type GitBlobReadResult = { exists: boolean } -const IMAGE_MIME_TYPES: Record = { +const PREVIEWABLE_BINARY_MIME_TYPES: Record = { '.png': 'image/png', '.jpg': 'image/jpeg', '.jpeg': 'image/jpeg', @@ -641,7 +650,8 @@ const IMAGE_MIME_TYPES: Record = { '.svg': 'image/svg+xml', '.webp': 'image/webp', '.bmp': 'image/bmp', - '.ico': 'image/x-icon' + '.ico': 'image/x-icon', + '.pdf': 'application/pdf' } /** diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index 7f8b7687f..921ce6e82 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -89,22 +89,26 @@ describe('registerFilesystemHandlers', () => { beforeEach(() => { handlers.clear() - handleMock.mockReset() - trashItemMock.mockReset() - readdirMock.mockReset() - readFileMock.mockReset() - writeFileMock.mockReset() - statMock.mockReset() - realpathMock.mockReset() - lstatMock.mockReset() - getStatusMock.mockReset() - getDiffMock.mockReset() - getBranchCompareMock.mockReset() - getBranchDiffMock.mockReset() - stageFileMock.mockReset() - unstageFileMock.mockReset() - discardChangesMock.mockReset() - listWorktreesMock.mockReset() + for (const mock of [ + handleMock, + trashItemMock, + readdirMock, + readFileMock, + writeFileMock, + statMock, + realpathMock, + lstatMock, + getStatusMock, + getDiffMock, + getBranchCompareMock, + getBranchDiffMock, + stageFileMock, + unstageFileMock, + discardChangesMock, + listWorktreesMock + ]) { + mock.mockReset() + } handleMock.mockImplementation((channel, handler) => { handlers.set(channel, handler) @@ -157,35 +161,26 @@ describe('registerFilesystemHandlers', () => { expect(writeFileMock).not.toHaveBeenCalled() }) - it('returns base64 content for supported image binaries', async () => { - statMock.mockResolvedValue({ size: 4, isDirectory: () => false, mtimeMs: 123 }) - readFileMock.mockResolvedValue(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00])) - + it.each([ + { ext: 'png', mime: 'image/png', data: [0x89, 0x50, 0x4e, 0x47, 0x00] }, + { ext: 'pdf', mime: 'application/pdf', data: [0x25, 0x50, 0x44, 0x46, 0x00] }, + { + ext: 'svg', + mime: 'image/svg+xml', + data: Array.from(Buffer.from('')) + } + ])('returns base64 content for supported $ext binaries', async ({ ext, mime, data }) => { + const buf = Buffer.from(data) + statMock.mockResolvedValue({ size: buf.length, isDirectory: () => false, mtimeMs: 123 }) + readFileMock.mockResolvedValue(buf) registerFilesystemHandlers(store as never) - await expect( - handlers.get('fs:readFile')!(null, { filePath: '/workspace/repo/image.png' }) + handlers.get('fs:readFile')!(null, { filePath: `/workspace/repo/file.${ext}` }) ).resolves.toEqual({ - content: Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00]).toString('base64'), + content: buf.toString('base64'), isBinary: true, isImage: true, - mimeType: 'image/png' - }) - }) - - it('returns base64 content for supported text-based images', async () => { - statMock.mockResolvedValue({ size: 32, isDirectory: () => false, mtimeMs: 123 }) - readFileMock.mockResolvedValue(Buffer.from('')) - - registerFilesystemHandlers(store as never) - - await expect( - handlers.get('fs:readFile')!(null, { filePath: '/workspace/repo/image.svg' }) - ).resolves.toEqual({ - content: Buffer.from('').toString('base64'), - isBinary: true, - isImage: true, - mimeType: 'image/svg+xml' + mimeType: mime }) }) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 36a3affad..55e0d1c05 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -39,7 +39,7 @@ const MAX_FILE_SIZE = 5 * 1024 * 1024 // 5MB const DEFAULT_SEARCH_MAX_RESULTS = 2000 const MAX_MATCHES_PER_FILE = 100 const SEARCH_TIMEOUT_MS = 15000 -const IMAGE_MIME_TYPES: Record = { +const PREVIEWABLE_BINARY_MIME_TYPES: Record = { '.png': 'image/png', '.jpg': 'image/jpeg', '.jpeg': 'image/jpeg', @@ -47,7 +47,8 @@ const IMAGE_MIME_TYPES: Record = { '.svg': 'image/svg+xml', '.webp': 'image/webp', '.bmp': 'image/bmp', - '.ico': 'image/x-icon' + '.ico': 'image/x-icon', + '.pdf': 'application/pdf' } function normalizeRelativePath(path: string): string { @@ -104,11 +105,14 @@ export function registerFilesystemHandlers(store: Store): void { } const buffer = await readFile(filePath) - const mimeType = IMAGE_MIME_TYPES[extname(filePath).toLowerCase()] + const mimeType = PREVIEWABLE_BINARY_MIME_TYPES[extname(filePath).toLowerCase()] if (mimeType) { return { content: buffer.toString('base64'), isBinary: true, + // Why: the renderer/store contract already keys previewable binary + // rendering off `isImage`. Keep that legacy flag for PDFs too so the + // new preview path stays compatible with existing callers. isImage: true, mimeType } diff --git a/src/renderer/src/components/editor/ImageDiffViewer.tsx b/src/renderer/src/components/editor/ImageDiffViewer.tsx index 8c311dc55..0c0303a50 100644 --- a/src/renderer/src/components/editor/ImageDiffViewer.tsx +++ b/src/renderer/src/components/editor/ImageDiffViewer.tsx @@ -25,7 +25,7 @@ function ImageDiffPane({
{label}
- No image + No preview
) diff --git a/src/renderer/src/components/editor/ImageViewer.tsx b/src/renderer/src/components/editor/ImageViewer.tsx index 402d9792a..17378c470 100644 --- a/src/renderer/src/components/editor/ImageViewer.tsx +++ b/src/renderer/src/components/editor/ImageViewer.tsx @@ -1,5 +1,5 @@ import { Image as ImageIcon, RotateCcw, X, ZoomIn, ZoomOut } from 'lucide-react' -import { type JSX, useMemo, useState } from 'react' +import { type JSX, useEffect, useMemo, useState } from 'react' import { Dialog, DialogContent, DialogTitle } from '@/components/ui/dialog' const FALLBACK_IMAGE_MIME_TYPE = 'image/png' @@ -27,10 +27,8 @@ export default function ImageViewer({ const filename = useMemo(() => filePath.split(/[/\\]/).pop() || filePath, [filePath]) const cleanedContent = useMemo(() => content.replace(/\s/g, ''), [content]) - const dataUrl = useMemo( - () => `data:${mimeType};base64,${cleanedContent}`, - [cleanedContent, mimeType] - ) + const isPdf = mimeType === 'application/pdf' + const [previewUrl, setPreviewUrl] = useState(null) const estimatedSize = useMemo(() => { const bytes = Math.floor((cleanedContent.length * 3) / 4) if (bytes < 1024) { @@ -43,17 +41,89 @@ export default function ImageViewer({ }, [cleanedContent]) const zoomPercent = Math.round(zoom * 100) + useEffect(() => { + // Reset error state so the component re-attempts rendering when inputs change + // (e.g. switching to a different file after a previous corrupt payload). + setImageError(false) + + if (!cleanedContent) { + setPreviewUrl(null) + return + } + + // Why: window.atob() throws a DOMException if cleanedContent contains + // invalid base64 characters (e.g. corrupt or truncated data). We catch + // that so the component degrades to the error state instead of crashing. + let binary: string + try { + binary = window.atob(cleanedContent) + } catch { + setImageError(true) + return + } + + const bytes = new Uint8Array(binary.length) + for (let i = 0; i < binary.length; i += 1) { + bytes[i] = binary.charCodeAt(i) + } + + // Why: large binary previews behave better as object URLs than giant + // inline data URLs. PDFs especially can surface awkward native viewer UI + // when loaded from a data URL, and object URLs avoid keeping megabytes of + // base64 text in the DOM. + const objectUrl = URL.createObjectURL(new Blob([bytes], { type: mimeType })) + setPreviewUrl(objectUrl) + + return () => URL.revokeObjectURL(objectUrl) + }, [cleanedContent, mimeType]) + if (imageError) { return (
-
Failed to load image preview
+
Failed to load file preview
{filename}
) } - const imagePane = ( + if (!previewUrl) { + return ( +
+ Loading preview... +
+ ) + } + + const previewPane = isPdf ? ( +
+