Fix full-height image diffs in combined view

Preserve intrinsic image height for binary image diffs in combined View all mode while keeping text diffs on measured Monaco heights.
This commit is contained in:
Neil 2026-05-15 18:46:43 -07:00 committed by GitHub
parent 013f740023
commit ff8accba87
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 210 additions and 34 deletions

View File

@ -23,8 +23,10 @@ import { getDiffCommentPopoverTop } from '../diff-comments/diff-comment-popover-
import { applyDiffEditorLineNumberOptions } from './diff-editor-line-number-options'
import { computeLineStats } from './diff-line-stats'
import { DiffSectionHeader } from './DiffSectionHeader'
import { getDiffSectionBodyHeight, isIntrinsicHeightImageDiff } from './diff-section-layout'
import type { DiffSection } from './diff-section-types'
import type { DiffComment } from '../../../../shared/types'
import { cn } from '@/lib/utils'
import { isDiffComment } from '@/lib/diff-comment-compat'
const ImageDiffViewer = lazy(() => import('./ImageDiffViewer'))
@ -217,6 +219,15 @@ export function DiffSectionItem({
: computeLineStats(section.originalContent, section.modifiedContent, section.status),
[section.loading, section.originalContent, section.modifiedContent, section.status]
)
// Why: image diffs need document-flow height in the combined view; the text
// fallback only knows line counts and would squash screenshots into one row.
const useIntrinsicImageHeight = isIntrinsicHeightImageDiff(section.diffResult)
const sectionBodyHeight = getDiffSectionBodyHeight({
measuredContentHeight: sectionHeight,
originalContent: section.originalContent,
modifiedContent: section.modifiedContent,
useIntrinsicImageHeight
})
const handleOpenInEditor = (e: React.MouseEvent): void => {
e.stopPropagation()
@ -292,20 +303,8 @@ export function DiffSectionItem({
{!section.collapsed && (
<div
className="relative"
style={{
height: sectionHeight
? sectionHeight + 19
: Math.max(
60,
Math.max(
section.originalContent.split('\n').length,
section.modifiedContent.split('\n').length
) *
19 +
19
)
}}
className={cn('relative', useIntrinsicImageHeight && 'overflow-visible')}
style={sectionBodyHeight === undefined ? undefined : { height: sectionBodyHeight }}
>
{popover && (
// Why: key by lineNumber so the popover remounts when the anchor
@ -332,6 +331,7 @@ export function DiffSectionItem({
filePath={section.path}
mimeType={section.diffResult.mimeType}
sideBySide={sideBySide}
layout={useIntrinsicImageHeight ? 'intrinsic' : 'fill'}
/>
) : (
<div className="flex h-full items-center justify-center px-6 text-center">

View File

@ -1,4 +1,5 @@
import { type JSX } from 'react'
import { cn } from '@/lib/utils'
import ImageViewer from './ImageViewer'
type ImageDiffViewerProps = {
@ -7,24 +8,39 @@ type ImageDiffViewerProps = {
filePath: string
mimeType?: string
sideBySide: boolean
layout?: 'fill' | 'intrinsic'
}
function ImageDiffPane({
label,
content,
filePath,
mimeType
mimeType,
layout
}: {
label: string
content: string
filePath: string
mimeType?: string
layout: 'fill' | 'intrinsic'
}): JSX.Element {
const isIntrinsicLayout = layout === 'intrinsic'
if (!content) {
return (
<div className="flex h-full min-h-0 flex-col overflow-hidden rounded-md bg-muted/10">
<div
className={cn(
'flex min-h-0 flex-col overflow-hidden rounded-md bg-muted/10',
isIntrinsicLayout ? 'h-auto' : 'h-full'
)}
>
<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">
<div
className={cn(
'flex items-center justify-center bg-muted/20 p-6 text-sm text-muted-foreground',
isIntrinsicLayout ? 'min-h-32' : 'flex-1'
)}
>
No preview
</div>
</div>
@ -32,10 +48,15 @@ function ImageDiffPane({
}
return (
<div className="flex h-full min-h-0 flex-col overflow-hidden rounded-md bg-muted/10">
<div
className={cn(
'flex min-h-0 flex-col overflow-hidden rounded-md bg-muted/10',
isIntrinsicLayout ? 'h-auto' : 'h-full'
)}
>
<div className="px-3 py-2 text-xs font-medium text-muted-foreground">{label}</div>
<div className="min-h-0 flex-1">
<ImageViewer content={content} filePath={filePath} mimeType={mimeType} />
<div className={cn('min-h-0', isIntrinsicLayout ? 'flex-none' : 'flex-1')}>
<ImageViewer content={content} filePath={filePath} mimeType={mimeType} layout={layout} />
</div>
</div>
)
@ -46,22 +67,30 @@ export default function ImageDiffViewer({
modifiedContent,
filePath,
mimeType,
sideBySide
sideBySide,
layout = 'fill'
}: ImageDiffViewerProps): JSX.Element {
const isIntrinsicLayout = layout === 'intrinsic'
// Why: in inline (single-column) mode the grid defaults to equal row
// heights, which squishes each preview into half the panel. Using
// minmax(32rem, 1fr) ensures content panes are tall enough to show a
// full page, and overflow-y-auto lets the user scroll between them.
// Empty "No preview" panes collapse to auto height.
const gridRowStyle = !sideBySide
? {
gridTemplateRows: `${originalContent ? 'minmax(32rem, 1fr)' : 'auto'} ${modifiedContent ? 'minmax(32rem, 1fr)' : 'auto'}`
}
: undefined
const gridRowStyle =
!sideBySide && !isIntrinsicLayout
? {
gridTemplateRows: `${originalContent ? 'minmax(32rem, 1fr)' : 'auto'} ${modifiedContent ? 'minmax(32rem, 1fr)' : 'auto'}`
}
: undefined
return (
<div
className={`grid h-full min-h-0 gap-3 p-3 ${sideBySide ? 'grid-cols-2' : 'grid-cols-1 overflow-y-auto'}`}
className={cn(
'grid min-h-0 gap-3 p-3',
isIntrinsicLayout ? 'h-auto' : 'h-full',
sideBySide ? 'grid-cols-2' : 'grid-cols-1',
!sideBySide && !isIntrinsicLayout && 'overflow-y-auto'
)}
style={gridRowStyle}
>
<ImageDiffPane
@ -69,12 +98,14 @@ export default function ImageDiffViewer({
content={originalContent}
filePath={filePath}
mimeType={mimeType}
layout={layout}
/>
<ImageDiffPane
label="Modified"
content={modifiedContent}
filePath={filePath}
mimeType={mimeType}
layout={layout}
/>
</div>
)

View File

@ -1,6 +1,7 @@
import { Image as ImageIcon, RotateCcw, X, ZoomIn, ZoomOut } from 'lucide-react'
import { type JSX, useEffect, useMemo, useState } from 'react'
import { Dialog, DialogContent, DialogDescription, DialogTitle } from '@/components/ui/dialog'
import { cn } from '@/lib/utils'
import PdfViewer from './PdfViewer'
const FALLBACK_IMAGE_MIME_TYPE = 'image/png'
@ -12,12 +13,14 @@ type ImageViewerProps = {
content: string
filePath: string
mimeType?: string
layout?: 'fill' | 'intrinsic'
}
export default function ImageViewer({
content,
filePath,
mimeType = FALLBACK_IMAGE_MIME_TYPE
mimeType = FALLBACK_IMAGE_MIME_TYPE,
layout = 'fill'
}: ImageViewerProps): JSX.Element {
const [imageError, setImageError] = useState(false)
const [isPopupOpen, setIsPopupOpen] = useState(false)
@ -29,6 +32,7 @@ export default function ImageViewer({
const filename = useMemo(() => filePath.split(/[/\\]/).pop() || filePath, [filePath])
const cleanedContent = useMemo(() => content.replace(/\s/g, ''), [content])
const isPdf = mimeType === 'application/pdf'
const isIntrinsicLayout = layout === 'intrinsic'
const [previewUrl, setPreviewUrl] = useState<string | null>(null)
const estimatedSize = useMemo(() => {
const bytes = Math.floor((cleanedContent.length * 3) / 4)
@ -70,7 +74,12 @@ export default function ImageViewer({
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">
<div
className={cn(
'flex flex-col items-center justify-center gap-3 bg-muted/20 p-8 text-sm text-muted-foreground',
isIntrinsicLayout ? 'min-h-64' : 'h-full'
)}
>
<ImageIcon size={40} />
<div>Failed to load file preview</div>
<div className="max-w-md break-all text-center text-xs">{filename}</div>
@ -80,7 +89,12 @@ export default function ImageViewer({
if (!previewUrl) {
return (
<div className="flex items-center justify-center h-full text-muted-foreground text-sm">
<div
className={cn(
'flex items-center justify-center text-muted-foreground text-sm',
isIntrinsicLayout ? 'min-h-64' : 'h-full'
)}
>
Loading preview...
</div>
)
@ -88,20 +102,29 @@ export default function ImageViewer({
return (
<>
<div className="flex h-full min-h-0 flex-col">
<div className={cn('flex min-h-0 flex-col', isIntrinsicLayout ? 'h-auto' : 'h-full')}>
<div
className="flex flex-1 items-center justify-center overflow-auto bg-muted/20 p-4 cursor-pointer"
className={cn(
'flex justify-center bg-muted/20 p-4 cursor-pointer',
isIntrinsicLayout ? 'items-start overflow-visible' : 'flex-1 items-center overflow-auto'
)}
onClick={() => setIsPopupOpen(true)}
title="Open image in popup"
>
<div
className="flex items-center justify-center"
className={cn(
'flex justify-center',
isIntrinsicLayout ? 'max-w-full items-start' : 'items-center'
)}
style={{ transform: `scale(${zoom})`, transformOrigin: 'center center' }}
>
<img
src={previewUrl}
alt={filename}
className="max-h-full max-w-full object-contain"
className={cn(
'max-w-full object-contain',
isIntrinsicLayout ? 'block h-auto max-h-none' : 'max-h-full'
)}
onLoad={(event) => {
const img = event.currentTarget
setImageDimensions({ width: img.naturalWidth, height: img.naturalHeight })

View File

@ -0,0 +1,84 @@
import { describe, expect, it } from 'vitest'
import { getDiffSectionBodyHeight, isIntrinsicHeightImageDiff } from './diff-section-layout'
import type { GitDiffResult } from '../../../../shared/types'
describe('diff section layout', () => {
it('uses Monaco measured content height for text diffs', () => {
expect(
getDiffSectionBodyHeight({
measuredContentHeight: 120,
originalContent: '',
modifiedContent: '',
useIntrinsicImageHeight: false
})
).toBe(139)
})
it('falls back to line-count height before Monaco has mounted', () => {
expect(
getDiffSectionBodyHeight({
measuredContentHeight: undefined,
originalContent: 'one',
modifiedContent: 'one\ntwo\nthree',
useIntrinsicImageHeight: false
})
).toBe(76)
})
it('keeps empty text sections visible', () => {
expect(
getDiffSectionBodyHeight({
measuredContentHeight: undefined,
originalContent: '',
modifiedContent: '',
useIntrinsicImageHeight: false
})
).toBe(60)
})
it('treats zero measured height as not laid out yet', () => {
expect(
getDiffSectionBodyHeight({
measuredContentHeight: 0,
originalContent: '',
modifiedContent: '',
useIntrinsicImageHeight: false
})
).toBe(60)
})
it('lets image diffs use intrinsic height in combined diff sections', () => {
expect(
getDiffSectionBodyHeight({
measuredContentHeight: undefined,
originalContent: '',
modifiedContent: '',
useIntrinsicImageHeight: true
})
).toBeUndefined()
})
it('only treats real image MIME types as intrinsic-height previews', () => {
const pngDiff: GitDiffResult = {
kind: 'binary',
originalContent: '',
modifiedContent: 'base64',
originalIsBinary: false,
modifiedIsBinary: true,
isImage: true,
mimeType: 'image/png'
}
const pdfDiff: GitDiffResult = {
kind: 'binary',
originalContent: '',
modifiedContent: 'base64',
originalIsBinary: false,
modifiedIsBinary: true,
isImage: true,
mimeType: 'application/pdf'
}
expect(isIntrinsicHeightImageDiff(pngDiff)).toBe(true)
expect(isIntrinsicHeightImageDiff(pdfDiff)).toBe(false)
})
})

View File

@ -0,0 +1,38 @@
import type { GitDiffResult } from '../../../../shared/types'
const DIFF_LINE_HEIGHT = 19
const DIFF_SECTION_PADDING_HEIGHT = 19
const MIN_DIFF_SECTION_BODY_HEIGHT = 60
type DiffSectionBodyHeightInput = {
measuredContentHeight: number | undefined
originalContent: string
modifiedContent: string
useIntrinsicImageHeight: boolean
}
export function isIntrinsicHeightImageDiff(diffResult: GitDiffResult | null | undefined): boolean {
return diffResult?.kind === 'binary' && diffResult.mimeType?.startsWith('image/') === true
}
export function getDiffSectionBodyHeight({
measuredContentHeight,
originalContent,
modifiedContent,
useIntrinsicImageHeight
}: DiffSectionBodyHeightInput): number | undefined {
if (useIntrinsicImageHeight) {
return undefined
}
if (measuredContentHeight !== undefined && measuredContentHeight > 0) {
return measuredContentHeight + DIFF_SECTION_PADDING_HEIGHT
}
return Math.max(
MIN_DIFF_SECTION_BODY_HEIGHT,
Math.max(originalContent.split('\n').length, modifiedContent.split('\n').length) *
DIFF_LINE_HEIGHT +
DIFF_SECTION_PADDING_HEIGHT
)
}