Distinguish read failures from true deletions in binary diff results

- Working-tree stat/readFile errors and relay reads previously collapsed
  onto the same empty-content signal as a genuine deletion, letting
  previewers fall back to stale original bytes on a failed read.
- Add modifiedDeleted/missing flags through status.ts, git-handler-ops,
  and git-working-file-read so only proven deletions trigger the
  original-bytes fallback; failed reads now return null.
- Tighten buildImageDataUri to accept only image/* mimes instead of
  special-casing application/pdf.
This commit is contained in:
Jinjing
2026-07-16 17:19:33 -07:00
parent 1d4ebe0d91
commit 0ea762ed03
10 changed files with 167 additions and 44 deletions
@@ -26,7 +26,26 @@ describe('mobileDiffImageDataUri', () => {
).toBe('data:image/png;base64,bmV3')
})
it('falls back to the original bytes for a deleted image (modified side absent)', () => {
it('falls back to the original bytes for a proven deletion (modifiedDeleted)', () => {
expect(
mobileDiffImageDataUri({
kind: 'binary',
originalContent: 'b2xk',
originalIsBinary: true,
modifiedContent: '',
modifiedIsBinary: false,
modifiedDeleted: true,
isImage: true,
mimeType: 'image/jpeg'
})
).toBe('data:image/jpeg;base64,b2xk')
})
// The reviewer's read-failure case: a relay/SSH read returns an empty modified
// side with modifiedIsBinary false and no modifiedDeleted flag. Without a proven
// deletion, falling back to the original would show a stale pre-change image, so
// "unavailable" (null) is the honest result.
it('returns null on a read failure (empty modified, no modifiedDeleted flag)', () => {
expect(
mobileDiffImageDataUri({
kind: 'binary',
@@ -35,14 +54,14 @@ describe('mobileDiffImageDataUri', () => {
modifiedContent: '',
modifiedIsBinary: false,
isImage: true,
mimeType: 'image/jpeg'
mimeType: 'image/png'
})
).toBe('data:image/jpeg;base64,b2xk')
).toBeNull()
})
// Guards the SSH/relay + >size-cap case: the modified side IS a binary image but
// its bytes arrive empty. Falling back to the original would render the stale
// pre-change image; "unavailable" is the honest result.
// Guards the >size-cap case: the modified side IS a binary image but its bytes
// arrive empty. Falling back to the original would render the stale pre-change
// image; "unavailable" is the honest result.
it('returns null for a modify whose binary modified side is empty (no stale fallback)', () => {
expect(
mobileDiffImageDataUri({
+10 -7
View File
@@ -1,19 +1,22 @@
import { buildImageDataUri } from '../../../src/shared/image-data-uri'
// originalIsBinary/modifiedIsBinary distinguish a genuine deletion from a modify
// whose bytes the host couldn't inline (relay/size-capped).
// modifiedDeleted marks a proven deletion (modified side genuinely absent); an
// empty modifiedContent alone can't, since a relay/SSH read failure also arrives
// empty with modifiedIsBinary false.
export type MobileBinaryDiffResult = {
kind: 'binary'
originalContent?: string
modifiedContent?: string
originalIsBinary?: boolean
modifiedIsBinary?: boolean
modifiedDeleted?: boolean
isImage?: boolean
mimeType?: string
}
// Falls back to the original bytes only for a genuine deletion; a size-capped/relay
// modify returns null instead of the stale pre-change image.
// Falls back to the original bytes only for a proven deletion; a read failure or
// size-capped modify — also empty on the modified side — returns null instead of
// the stale pre-change image.
export function mobileDiffImageDataUri(result: MobileBinaryDiffResult): string | null {
if (result.isImage !== true) {
return null
@@ -22,8 +25,8 @@ export function mobileDiffImageDataUri(result: MobileBinaryDiffResult): string |
if (modified) {
return buildImageDataUri(result.mimeType, modified)
}
if (result.modifiedIsBinary === true) {
return null
if (result.modifiedDeleted === true) {
return buildImageDataUri(result.mimeType, result.originalContent || '')
}
return buildImageDataUri(result.mimeType, result.originalContent || '')
return null
}
+31
View File
@@ -476,6 +476,37 @@ describe('getDiff', () => {
})
})
it('flags a deleted image so previewers can fall back to the original bytes', async () => {
const pngBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00])
gitExecFileAsyncBufferMock.mockResolvedValueOnce({ stdout: pngBuffer })
statMock.mockRejectedValueOnce(Object.assign(new Error('missing'), { code: 'ENOENT' }))
const result = await getDiff('/repo', 'assets/deleted.png', false)
expect(result.kind).toBe('binary')
if (result.kind !== 'binary') {
throw new Error('expected binary diff result')
}
expect(result.modifiedDeleted).toBe(true)
expect(result.originalContent).toBe(pngBuffer.toString('base64'))
expect(result.modifiedContent).toBe('')
})
it('does not treat an unreadable working-tree image as a deletion', async () => {
const pngBuffer = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00])
gitExecFileAsyncBufferMock.mockResolvedValueOnce({ stdout: pngBuffer })
statMock.mockResolvedValueOnce({ isFile: () => true, size: 5 })
readFileMock.mockRejectedValueOnce(new Error('EIO'))
const result = await getDiff('/repo', 'assets/unreadable.png', false)
expect(result.kind).toBe('binary')
if (result.kind !== 'binary') {
throw new Error('expected binary diff result')
}
expect(result.modifiedDeleted).toBeUndefined()
})
it('coalesces concurrent identical staged diff reads while in flight', async () => {
const leftBlob = deferredBuffer('head-content\n')
const rightBlob = deferredBuffer('index-content\n')
+32 -10
View File
@@ -1329,6 +1329,7 @@ async function loadDiff(
let modifiedContent = ''
let originalIsBinary = false
let modifiedIsBinary = false
let modifiedDeleted = false
try {
const leftBlob = staged
@@ -1343,22 +1344,30 @@ async function loadDiff(
const rightBlob = await readGitBlobAtIndexPath(worktreePath, filePath, options)
modifiedContent = rightBlob.content
modifiedIsBinary = rightBlob.isBinary
modifiedDeleted = !rightBlob.exists
} else {
const workingTreeBlob = await readWorkingTreeFile(path.join(worktreePath, filePath))
modifiedContent = workingTreeBlob.content
modifiedIsBinary = workingTreeBlob.isBinary
modifiedDeleted = !workingTreeBlob.exists
}
} catch {
// Fallback
}
return buildDiffResult(
const result = buildDiffResult(
originalContent,
modifiedContent,
originalIsBinary,
modifiedIsBinary,
filePath
)
// Why: mark a proven deletion so previewers can fall back to the original bytes
// without mistaking a read failure's empty modified side for a deletion.
if (result.kind === 'binary' && modifiedDeleted) {
return { ...result, modifiedDeleted: true }
}
return result
}
export async function getBranchCompare(
@@ -1859,20 +1868,33 @@ async function readGitBlobAtOidPath(
}
async function readWorkingTreeFile(filePath: string): Promise<GitBlobReadResult> {
let fileStat
try {
const fileStat = await stat(filePath)
if (!fileStat.isFile()) {
return { content: '', isBinary: false, exists: false }
}
if (fileStat.size > MAX_GIT_SHOW_BYTES) {
// Why: git blob reads are capped through maxBuffer; mirror that bound for
// unstaged working-tree content before readFile can pull in huge assets.
return { content: '', isBinary: true, exists: true }
fileStat = await stat(filePath)
} catch (error) {
// Why: ENOENT means the working-tree file is genuinely gone (a deletion);
// any other stat error is a read failure, which must not be reported as an
// absence since callers fall back to the original bytes only for deletions.
return {
content: '',
isBinary: false,
exists: (error as NodeJS.ErrnoException)?.code !== 'ENOENT'
}
}
if (!fileStat.isFile()) {
return { content: '', isBinary: false, exists: false }
}
if (fileStat.size > MAX_GIT_SHOW_BYTES) {
// Why: git blob reads are capped through maxBuffer; mirror that bound for
// unstaged working-tree content before readFile can pull in huge assets.
return { content: '', isBinary: true, exists: true }
}
try {
const buffer = await readFile(filePath)
return bufferToBlob(buffer, filePath)
} catch {
return { content: '', isBinary: false, exists: false }
// Why: the file exists but could not be read — a read failure, not a deletion.
return { content: '', isBinary: false, exists: true }
}
}
+16 -5
View File
@@ -52,17 +52,19 @@ export async function readBlobAtIndex(
gitBuffer: GitBufferExec,
cwd: string,
filePath: string
): Promise<{ content: string; isBinary: boolean }> {
): Promise<{ content: string; isBinary: boolean; missing: boolean }> {
// Why: Git's `:<path>` syntax expects forward slashes even on Windows.
const gitPath = filePath.replace(/\\/g, '/')
try {
const buf = await gitBuffer(['show', '--end-of-options', `:${gitPath}`], cwd)
return bufferToBlob(buf, filePath)
return { ...bufferToBlob(buf, filePath), missing: false }
} catch (error) {
if (isGitBufferOverflowError(error)) {
return { content: '', isBinary: true }
return { content: '', isBinary: true, missing: false }
}
return { content: '', isBinary: false }
// Why: a non-overflow failure means the path is absent from the index (a
// staged deletion), distinct from the size-capped case handled above.
return { content: '', isBinary: false, missing: true }
}
}
@@ -91,6 +93,7 @@ export async function computeDiff(
let modifiedContent = ''
let originalIsBinary = false
let modifiedIsBinary = false
let modifiedDeleted = false
try {
if (staged) {
@@ -101,6 +104,7 @@ export async function computeDiff(
const right = await readBlobAtIndex(git, worktreePath, filePath)
modifiedContent = right.content
modifiedIsBinary = right.isBinary
modifiedDeleted = right.missing
} else {
const left = compareAgainstHead
? await readBlobAtOid(git, worktreePath, 'HEAD', filePath)
@@ -111,18 +115,25 @@ export async function computeDiff(
const right = await readWorkingDiffFile(path.join(worktreePath, filePath))
modifiedContent = right.content
modifiedIsBinary = right.isBinary
modifiedDeleted = right.missing
}
} catch {
// Fallback to empty
}
return buildDiffResult(
const result = buildDiffResult(
originalContent,
modifiedContent,
originalIsBinary,
modifiedIsBinary,
filePath
)
// Why: mark a proven deletion so previewers can fall back to the original bytes
// without mistaking a read failure's empty modified side for a deletion.
if (result.kind === 'binary' && modifiedDeleted) {
return { ...result, modifiedDeleted: true }
}
return result
}
// ─── Branch compare ──────────────────────────────────────────────────
+17 -3
View File
@@ -21,7 +21,8 @@ describe('readWorkingDiffFile', () => {
await expect(readWorkingDiffFile(filePath)).resolves.toEqual({
content: 'hello',
isBinary: false
isBinary: false,
missing: false
})
})
@@ -32,7 +33,8 @@ describe('readWorkingDiffFile', () => {
await expect(readWorkingDiffFile(filePath)).resolves.toEqual({
content: '',
isBinary: true
isBinary: true,
missing: false
})
})
@@ -44,7 +46,19 @@ describe('readWorkingDiffFile', () => {
await expect(readWorkingDiffFile(filePath)).resolves.toEqual({
content: pngBytes.toString('base64'),
isBinary: true
isBinary: true,
missing: false
})
})
it('reports an absent working-tree file as a deletion', async () => {
tmpDir = await mkdtemp(path.join(tmpdir(), 'relay-working-file-'))
const filePath = path.join(tmpDir, 'deleted.png')
await expect(readWorkingDiffFile(filePath)).resolves.toEqual({
content: '',
isBinary: false,
missing: true
})
})
})
+21 -11
View File
@@ -5,21 +5,31 @@ const MAX_RELAY_DIFF_WORKING_FILE_BYTES = 10 * 1024 * 1024
export async function readWorkingDiffFile(
absPath: string
): Promise<{ content: string; isBinary: boolean }> {
): Promise<{ content: string; isBinary: boolean; missing: boolean }> {
let fileStat
try {
fileStat = await stat(absPath)
} catch (error) {
// Why: ENOENT means the working-tree file is genuinely gone (a deletion);
// any other stat error is a read failure we must not mistake for one, since
// callers fall back to the original bytes only for a proven deletion.
const missing = (error as NodeJS.ErrnoException)?.code === 'ENOENT'
return { content: '', isBinary: false, missing }
}
if (!fileStat.isFile()) {
return { content: '', isBinary: false, missing: true }
}
if (fileStat.size > MAX_RELAY_DIFF_WORKING_FILE_BYTES) {
// Why: mirror local git diff reads, which cap blob transfer at 10MB.
return { content: '', isBinary: true, missing: false }
}
try {
const fileStat = await stat(absPath)
if (!fileStat.isFile()) {
return { content: '', isBinary: false }
}
if (fileStat.size > MAX_RELAY_DIFF_WORKING_FILE_BYTES) {
// Why: mirror local git diff reads, which cap blob transfer at 10MB.
return { content: '', isBinary: true }
}
const buffer = await readFile(absPath)
// Why: bufferToBlob needs the path's extension to know an image is
// previewable; omitting it made every relay-side binary diff empty.
return bufferToBlob(buffer, absPath)
return { ...bufferToBlob(buffer, absPath), missing: false }
} catch {
return { content: '', isBinary: false }
// Why: the file exists but could not be read — a read failure, not a deletion.
return { content: '', isBinary: false, missing: false }
}
}
+4
View File
@@ -23,4 +23,8 @@ describe('buildImageDataUri', () => {
it('returns null for application/pdf (not an <img> source)', () => {
expect(buildImageDataUri('application/pdf', 'JVBER')).toBeNull()
})
it('returns null for a non-image mime such as application/octet-stream', () => {
expect(buildImageDataUri('application/octet-stream', 'AAAA')).toBeNull()
})
})
+4 -2
View File
@@ -2,12 +2,14 @@
// editor ImageViewer and the mobile file preview so both decode images the same
// way. Strips whitespace from the payload (base64 from git diffs and SSH streams
// can arrive line-wrapped) and returns null when there is nothing an <img>/RN
// <Image> can show — empty content or a non-image mime such as application/pdf.
// <Image> can show — empty content or a non-image mime (PDF, octet-stream, …).
export function buildImageDataUri(
mimeType: string | undefined,
base64Content: string
): string | null {
if (!mimeType || mimeType === 'application/pdf') {
// Only image/* renders in an <img>/RN <Image>; reject every other mime
// (application/pdf, application/octet-stream, …), not just PDF.
if (!mimeType?.startsWith('image/')) {
return null
}
const cleaned = base64Content.replace(/\s/g, '')
+7
View File
@@ -3738,6 +3738,13 @@ export type GitDiffBinaryResult = {
isImage?: boolean
/** MIME type for binary preview rendering, e.g. "image/png" or "application/pdf" */
mimeType?: string
/**
* True only when the modified side is a proven deletion (working-tree file gone
* or absent from the index) — distinct from an empty modified side caused by a
* read failure or size cap. Lets previewers fall back to the original bytes for
* a deletion without showing a stale image on a failed read.
*/
modifiedDeleted?: boolean
} & (
| { originalIsBinary: true; modifiedIsBinary: boolean }
| { originalIsBinary: boolean; modifiedIsBinary: true }