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
This commit is contained in:
parent
528e03e8a5
commit
e8e7d2ea8f
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -579,9 +579,15 @@ async function readWorkingTreeFile(filePath: string): Promise<GitBlobReadResult>
|
|||
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<string, string> = {
|
||||
const PREVIEWABLE_BINARY_MIME_TYPES: Record<string, string> = {
|
||||
'.png': 'image/png',
|
||||
'.jpg': 'image/jpeg',
|
||||
'.jpeg': 'image/jpeg',
|
||||
|
|
@ -641,7 +650,8 @@ const IMAGE_MIME_TYPES: Record<string, string> = {
|
|||
'.svg': 'image/svg+xml',
|
||||
'.webp': 'image/webp',
|
||||
'.bmp': 'image/bmp',
|
||||
'.ico': 'image/x-icon'
|
||||
'.ico': 'image/x-icon',
|
||||
'.pdf': 'application/pdf'
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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('<svg xmlns="http://www.w3.org/2000/svg" />'))
|
||||
}
|
||||
])('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('<svg xmlns="http://www.w3.org/2000/svg" />'))
|
||||
|
||||
registerFilesystemHandlers(store as never)
|
||||
|
||||
await expect(
|
||||
handlers.get('fs:readFile')!(null, { filePath: '/workspace/repo/image.svg' })
|
||||
).resolves.toEqual({
|
||||
content: Buffer.from('<svg xmlns="http://www.w3.org/2000/svg" />').toString('base64'),
|
||||
isBinary: true,
|
||||
isImage: true,
|
||||
mimeType: 'image/svg+xml'
|
||||
mimeType: mime
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string, string> = {
|
||||
const PREVIEWABLE_BINARY_MIME_TYPES: Record<string, string> = {
|
||||
'.png': 'image/png',
|
||||
'.jpg': 'image/jpeg',
|
||||
'.jpeg': 'image/jpeg',
|
||||
|
|
@ -47,7 +47,8 @@ const IMAGE_MIME_TYPES: Record<string, string> = {
|
|||
'.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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ function ImageDiffPane({
|
|||
<div className="flex h-full min-h-0 flex-col overflow-hidden rounded-md bg-muted/10">
|
||||
<div className="px-3 py-2 text-xs font-medium text-muted-foreground">{label}</div>
|
||||
<div className="flex flex-1 items-center justify-center bg-muted/20 p-6 text-sm text-muted-foreground">
|
||||
No image
|
||||
No preview
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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<string | null>(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 (
|
||||
<div className="flex h-full flex-col items-center justify-center gap-3 bg-muted/20 p-8 text-sm text-muted-foreground">
|
||||
<ImageIcon size={40} />
|
||||
<div>Failed to load image preview</div>
|
||||
<div>Failed to load file preview</div>
|
||||
<div className="max-w-md break-all text-center text-xs">{filename}</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
const imagePane = (
|
||||
if (!previewUrl) {
|
||||
return (
|
||||
<div className="flex items-center justify-center h-full text-muted-foreground text-sm">
|
||||
Loading preview...
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
const previewPane = isPdf ? (
|
||||
<div
|
||||
className="relative flex flex-1 items-center justify-center overflow-auto bg-muted/20 p-4"
|
||||
title="Open PDF in popup"
|
||||
>
|
||||
<iframe
|
||||
src={previewUrl}
|
||||
title={filename}
|
||||
sandbox="allow-same-origin"
|
||||
className="h-full min-h-[24rem] w-full rounded-md border border-border/60 bg-background"
|
||||
onError={() => setImageError(true)}
|
||||
/>
|
||||
{/* Why: clicks inside an iframe are consumed by the iframe's own document,
|
||||
so the parent div's onClick never fires. This transparent overlay sits
|
||||
on top of the iframe to intercept clicks and open the popup. */}
|
||||
<div
|
||||
className="absolute inset-0 z-10 cursor-pointer"
|
||||
onClick={() => setIsPopupOpen(true)}
|
||||
role="button"
|
||||
tabIndex={0}
|
||||
aria-label="Open PDF in popup"
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === 'Enter' || e.key === ' ') {
|
||||
setIsPopupOpen(true)
|
||||
}
|
||||
}}
|
||||
/>
|
||||
</div>
|
||||
) : (
|
||||
<div
|
||||
className="flex flex-1 items-center justify-center overflow-auto bg-muted/20 p-4 cursor-pointer"
|
||||
onClick={() => setIsPopupOpen(true)}
|
||||
|
|
@ -64,7 +134,7 @@ export default function ImageViewer({
|
|||
style={{ transform: `scale(${zoom})`, transformOrigin: 'center center' }}
|
||||
>
|
||||
<img
|
||||
src={dataUrl}
|
||||
src={previewUrl}
|
||||
alt={filename}
|
||||
className="max-h-full max-w-full object-contain"
|
||||
onLoad={(event) => {
|
||||
|
|
@ -80,46 +150,49 @@ export default function ImageViewer({
|
|||
return (
|
||||
<>
|
||||
<div className="flex h-full min-h-0 flex-col">
|
||||
{imagePane}
|
||||
{previewPane}
|
||||
<div className="flex items-center gap-4 border-t px-4 py-2 text-xs text-muted-foreground">
|
||||
<div className="flex items-center gap-1">
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom((prev) => Math.max(MIN_ZOOM, prev / ZOOM_STEP))}
|
||||
disabled={zoom <= MIN_ZOOM}
|
||||
title="Zoom out"
|
||||
>
|
||||
<ZoomOut size={14} />
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom(1)}
|
||||
disabled={zoom === 1}
|
||||
title="Reset zoom"
|
||||
>
|
||||
<RotateCcw size={14} />
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom((prev) => Math.min(MAX_ZOOM, prev * ZOOM_STEP))}
|
||||
disabled={zoom >= MAX_ZOOM}
|
||||
title="Zoom in"
|
||||
>
|
||||
<ZoomIn size={14} />
|
||||
</button>
|
||||
<span className="ml-1 tabular-nums">{zoomPercent}%</span>
|
||||
</div>
|
||||
{!isPdf && (
|
||||
<div className="flex items-center gap-1">
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom((prev) => Math.max(MIN_ZOOM, prev / ZOOM_STEP))}
|
||||
disabled={zoom <= MIN_ZOOM}
|
||||
title="Zoom out"
|
||||
>
|
||||
<ZoomOut size={14} />
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom(1)}
|
||||
disabled={zoom === 1}
|
||||
title="Reset zoom"
|
||||
>
|
||||
<RotateCcw size={14} />
|
||||
</button>
|
||||
<button
|
||||
type="button"
|
||||
className="rounded p-1 hover:bg-accent hover:text-foreground disabled:opacity-50"
|
||||
onClick={() => setZoom((prev) => Math.min(MAX_ZOOM, prev * ZOOM_STEP))}
|
||||
disabled={zoom >= MAX_ZOOM}
|
||||
title="Zoom in"
|
||||
>
|
||||
<ZoomIn size={14} />
|
||||
</button>
|
||||
<span className="ml-1 tabular-nums">{zoomPercent}%</span>
|
||||
</div>
|
||||
)}
|
||||
<span className="min-w-0 truncate" title={filename}>
|
||||
{filename}
|
||||
</span>
|
||||
{imageDimensions && (
|
||||
{!isPdf && imageDimensions && (
|
||||
<span>
|
||||
{imageDimensions.width} x {imageDimensions.height}
|
||||
</span>
|
||||
)}
|
||||
{isPdf && <span>PDF preview</span>}
|
||||
<span>{estimatedSize}</span>
|
||||
</div>
|
||||
</div>
|
||||
|
|
@ -141,20 +214,29 @@ export default function ImageViewer({
|
|||
</button>
|
||||
</div>
|
||||
<div className="flex h-[calc(100%-4.5rem)] w-full min-h-0 items-center justify-center overflow-auto bg-muted/20 p-4">
|
||||
<div
|
||||
className="flex items-center justify-center"
|
||||
style={{ transform: `scale(${zoom})`, transformOrigin: 'center center' }}
|
||||
>
|
||||
<img
|
||||
src={dataUrl}
|
||||
alt={filename}
|
||||
className="block max-h-full max-w-full object-contain"
|
||||
{isPdf ? (
|
||||
<iframe
|
||||
src={previewUrl}
|
||||
title={filename}
|
||||
sandbox="allow-same-origin"
|
||||
className="h-full w-full rounded-md border border-border/60 bg-background"
|
||||
/>
|
||||
</div>
|
||||
) : (
|
||||
<div
|
||||
className="flex items-center justify-center"
|
||||
style={{ transform: `scale(${zoom})`, transformOrigin: 'center center' }}
|
||||
>
|
||||
<img
|
||||
src={previewUrl}
|
||||
alt={filename}
|
||||
className="block max-h-full max-w-full object-contain"
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
<div className="flex items-center justify-between border-t border-border/60 bg-background/95 px-3 py-2 text-xs text-muted-foreground">
|
||||
<div>Press Esc to close</div>
|
||||
<div className="tabular-nums">{zoomPercent}%</div>
|
||||
<div className="tabular-nums">{isPdf ? 'PDF preview' : `${zoomPercent}%`}</div>
|
||||
</div>
|
||||
</DialogContent>
|
||||
</Dialog>
|
||||
|
|
|
|||
|
|
@ -313,9 +313,9 @@ export type GitDiffBinaryResult = {
|
|||
kind: 'binary'
|
||||
originalContent: string
|
||||
modifiedContent: string
|
||||
/** True when both sides are a recognized image format (PNG, JPG, etc.) */
|
||||
/** Legacy flag used by the renderer for any binary format it can preview, including PDFs. */
|
||||
isImage?: boolean
|
||||
/** MIME type for image rendering, e.g. "image/png" */
|
||||
/** MIME type for binary preview rendering, e.g. "image/png" or "application/pdf" */
|
||||
mimeType?: string
|
||||
} & (
|
||||
| { originalIsBinary: true; modifiedIsBinary: boolean }
|
||||
|
|
|
|||
Loading…
Reference in New Issue