From 0ea762ed03eafead674c47cf9205be6ba5d0c0bb Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:19:33 -0700 Subject: [PATCH] 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. --- .../files/mobile-diff-image-preview.test.ts | 31 +++++++++++--- mobile/src/files/mobile-diff-image-preview.ts | 17 ++++---- src/main/git/status.test.ts | 31 ++++++++++++++ src/main/git/status.ts | 42 ++++++++++++++----- src/relay/git-handler-ops.ts | 21 +++++++--- src/relay/git-working-file-read.test.ts | 20 +++++++-- src/relay/git-working-file-read.ts | 32 +++++++++----- src/shared/image-data-uri.test.ts | 4 ++ src/shared/image-data-uri.ts | 6 ++- src/shared/types.ts | 7 ++++ 10 files changed, 167 insertions(+), 44 deletions(-) diff --git a/mobile/src/files/mobile-diff-image-preview.test.ts b/mobile/src/files/mobile-diff-image-preview.test.ts index 3301e22d183..e63f532b28d 100644 --- a/mobile/src/files/mobile-diff-image-preview.test.ts +++ b/mobile/src/files/mobile-diff-image-preview.test.ts @@ -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({ diff --git a/mobile/src/files/mobile-diff-image-preview.ts b/mobile/src/files/mobile-diff-image-preview.ts index ff98c31c108..09d7cd2ef36 100644 --- a/mobile/src/files/mobile-diff-image-preview.ts +++ b/mobile/src/files/mobile-diff-image-preview.ts @@ -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 } diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 8af536a31a6..fd5e16f91c5 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -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') diff --git a/src/main/git/status.ts b/src/main/git/status.ts index a0c103b744c..ae818b52567 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -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 { + 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 } } } diff --git a/src/relay/git-handler-ops.ts b/src/relay/git-handler-ops.ts index 395e31badb3..afe75a2b582 100644 --- a/src/relay/git-handler-ops.ts +++ b/src/relay/git-handler-ops.ts @@ -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 `:` 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 ────────────────────────────────────────────────── diff --git a/src/relay/git-working-file-read.test.ts b/src/relay/git-working-file-read.test.ts index d4184775d19..b45854e2e86 100644 --- a/src/relay/git-working-file-read.test.ts +++ b/src/relay/git-working-file-read.test.ts @@ -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 }) }) }) diff --git a/src/relay/git-working-file-read.ts b/src/relay/git-working-file-read.ts index d2228dd04fe..05cf1ebcdce 100644 --- a/src/relay/git-working-file-read.ts +++ b/src/relay/git-working-file-read.ts @@ -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 } } } diff --git a/src/shared/image-data-uri.test.ts b/src/shared/image-data-uri.test.ts index a5e29446561..8244d2d652b 100644 --- a/src/shared/image-data-uri.test.ts +++ b/src/shared/image-data-uri.test.ts @@ -23,4 +23,8 @@ describe('buildImageDataUri', () => { it('returns null for application/pdf (not an 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() + }) }) diff --git a/src/shared/image-data-uri.ts b/src/shared/image-data-uri.ts index 9217c6b9154..1501473560f 100644 --- a/src/shared/image-data-uri.ts +++ b/src/shared/image-data-uri.ts @@ -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 /RN -// can show — empty content or a non-image mime such as application/pdf. +// 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 /RN ; reject every other mime + // (application/pdf, application/octet-stream, …), not just PDF. + if (!mimeType?.startsWith('image/')) { return null } const cleaned = base64Content.replace(/\s/g, '') diff --git a/src/shared/types.ts b/src/shared/types.ts index 3de8e943d16..14b6f8ecdaa 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -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 }