diff --git a/src/renderer/src/components/editor/combined-diff-on-demand-load.test.ts b/src/renderer/src/components/editor/combined-diff-on-demand-load.test.ts index 67c5e545c27..6637417e209 100644 --- a/src/renderer/src/components/editor/combined-diff-on-demand-load.test.ts +++ b/src/renderer/src/components/editor/combined-diff-on-demand-load.test.ts @@ -23,25 +23,31 @@ describe('combined diff on-demand loading', () => { ).toBe(false) }) - it('automatically loads tracked diffs when line counts are unavailable', () => { - expect( - shouldLoadCombinedDiffOnDemand({ added: undefined, removed: undefined, area: 'unstaged' }) - ).toBe(false) + it('defers uncounted tracked text files, whose size is unknown', () => { + // A capped status listing or a failed numstat leaves every tracked row + // uncounted; auto-loading them is what froze Monaco before deferral. + expect(shouldLoadCombinedDiffOnDemand({ path: 'src/generated/schema.ts' })).toBe(true) }) it('defers untracked files whose line counts were skipped as too large', () => { - expect(shouldLoadCombinedDiffOnDemand({ area: 'untracked', path: 'data/dump.json' })).toBe(true) + expect(shouldLoadCombinedDiffOnDemand({ path: 'data/dump.json' })).toBe(true) }) - it('defers uncounted untracked svgs, which render as text rather than a preview', () => { - expect(shouldLoadCombinedDiffOnDemand({ area: 'untracked', path: 'assets/map.svg' })).toBe(true) + it('defers uncounted svgs, which render as text rather than a preview', () => { + expect(shouldLoadCombinedDiffOnDemand({ path: 'assets/map.svg' })).toBe(true) }) - it('automatically loads untracked images that report no line counts', () => { - expect(shouldLoadCombinedDiffOnDemand({ area: 'untracked', path: 'docs/Shot.PNG' })).toBe(false) + it('automatically loads uncounted images, which render as a preview', () => { + expect(shouldLoadCombinedDiffOnDemand({ path: 'docs/Shot.PNG' })).toBe(false) }) - it('defers untracked diffs when only additions are reported', () => { + it('automatically loads uncounted non-image binaries of any size', () => { + expect(shouldLoadCombinedDiffOnDemand({ path: 'fixtures/sample.zip' })).toBe(false) + expect(shouldLoadCombinedDiffOnDemand({ path: 'fonts/Inter.woff2' })).toBe(false) + expect(shouldLoadCombinedDiffOnDemand({ path: 'bun.lockb' })).toBe(false) + }) + + it('defers diffs when only additions are reported', () => { expect(shouldLoadCombinedDiffOnDemand({ added: MAX_AUTOMATIC_DIFF_CHANGED_LINES + 1 })).toBe( true ) @@ -52,4 +58,14 @@ describe('combined diff on-demand loading', () => { true ) }) + + it('keeps counted binary-extension rows on the line-count rule', () => { + expect(shouldLoadCombinedDiffOnDemand({ added: 3, path: 'fixtures/sample.zip' })).toBe(false) + expect( + shouldLoadCombinedDiffOnDemand({ + added: MAX_AUTOMATIC_DIFF_CHANGED_LINES + 1, + path: 'fixtures/sample.zip' + }) + ).toBe(true) + }) }) diff --git a/src/renderer/src/components/editor/combined-diff-on-demand-load.ts b/src/renderer/src/components/editor/combined-diff-on-demand-load.ts index ae6103a4ec5..869d2d577c4 100644 --- a/src/renderer/src/components/editor/combined-diff-on-demand-load.ts +++ b/src/renderer/src/components/editor/combined-diff-on-demand-load.ts @@ -1,36 +1,24 @@ -import type { GitStatusEntry } from '../../../../shared/git-status-types' -import { IMAGE_FILE_EXTENSIONS } from '../../../../shared/image-file-extensions' +import { hasBinaryFileExtension } from '../../../../shared/binary-file-extensions' export const MAX_AUTOMATIC_DIFF_CHANGED_LINES = 10_000 -// SVG is excluded: it reads as text, so the diff view renders its source in Monaco -// rather than an image preview — deferral is exactly what an oversized one needs. -const PREVIEWED_IMAGE_EXTENSIONS = IMAGE_FILE_EXTENSIONS.filter((ext) => ext !== '.svg') - -function isPreviewedImagePath(path: string | undefined): boolean { - const lowerPath = path?.toLowerCase() - return ( - lowerPath !== undefined && PREVIEWED_IMAGE_EXTENSIONS.some((ext) => lowerPath.endsWith(ext)) - ) -} - export function shouldLoadCombinedDiffOnDemand({ added, removed, - area, path }: { added?: number removed?: number - area?: GitStatusEntry['area'] path?: string }): boolean { if (added === undefined && removed === undefined) { - // Untracked files lose their counts once they exceed the status-scan size cap - // (MAX_UNTRACKED_LINE_COUNT_BYTES), so an uncounted untracked file is either - // oversized text or binary — both too costly to auto-load. Images stay automatic - // because their preview is the point of the row. - return area === 'untracked' && !isPreviewedImagePath(path) + // A row with no counts is one of: a binary file (git numstat reports '-', + // and the untracked scan skips binaries), an untracked file past the scan's + // size cap (MAX_UNTRACKED_LINE_COUNT_BYTES), or any file in a status pass + // that skipped counting entirely (entry cap hit, numstat failed). Only the + // binary case is cheap to render, and only the path can tell us that here, + // so defer everything Monaco would open as unbounded text. + return !hasBinaryFileExtension(path) } return (added ?? 0) + (removed ?? 0) > MAX_AUTOMATIC_DIFF_CHANGED_LINES } diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts index 39c5c6ca290..df253230fec 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts +++ b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.ts @@ -139,7 +139,6 @@ export function useCombinedDiffViewRestore({ const loadOnDemand = shouldLoadCombinedDiffOnDemand({ added: 'added' in entry ? entry.added : undefined, removed: 'removed' in entry ? entry.removed : undefined, - area: 'area' in entry ? entry.area : undefined, path: entry.path }) return { diff --git a/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts b/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts index 88801dd1c9e..32e0a48608a 100644 --- a/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts +++ b/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts @@ -3,11 +3,7 @@ import type React from 'react' import { elementScroll, useVirtualizer, type Virtualizer } from '@tanstack/react-virtual' import type { ProgrammaticScrollMarks } from '@/hooks/programmatic-scroll-marks' import type { DiffSection } from '../../diff-section-types' -import { - getDiffSectionEstimatedHeight, - isIntrinsicHeightImageDiff, - usesLargeDiffFallbackHeight -} from '../../diff-section-layout' +import { getDiffSectionRowEstimatedHeight } from '../../diff-section-layout' const COMBINED_DIFF_OVERSCAN = 5 @@ -39,19 +35,7 @@ export function useCombinedDiffVirtualizer({ return 88 } - return getDiffSectionEstimatedHeight({ - collapsed: section.collapsed, - measuredContentHeight: sectionHeights[index], - originalContent: section.originalContent, - modifiedContent: section.modifiedContent, - changedLineCount: - section.added === undefined && section.removed === undefined - ? undefined - : (section.added ?? 0) + (section.removed ?? 0), - useIntrinsicImageHeight: isIntrinsicHeightImageDiff(section.diffResult), - isLargeDiffLimited: usesLargeDiffFallbackHeight(section), - lineCounts: section.largeDiffRenderLimit?.lineCounts ?? undefined - }) + return getDiffSectionRowEstimatedHeight(section, sectionHeights[index]) }, overscan: COMBINED_DIFF_OVERSCAN, initialOffset: () => scrollOffsetRef.current, diff --git a/src/renderer/src/components/editor/diff-section-layout.test.ts b/src/renderer/src/components/editor/diff-section-layout.test.ts index dcd2a6990e4..c1b100a2f33 100644 --- a/src/renderer/src/components/editor/diff-section-layout.test.ts +++ b/src/renderer/src/components/editor/diff-section-layout.test.ts @@ -3,11 +3,28 @@ import { getDiffSectionBodyHeight, getLargeDiffFallbackBodyHeight, getDiffSectionEstimatedHeight, + getDiffSectionRowEstimatedHeight, isIntrinsicHeightImageDiff, usesLargeDiffFallbackHeight } from './diff-section-layout' +import type { DiffSection } from './diff-section-types' import type { GitDiffResult } from '../../../../shared/git-diff-compare-types' +const largeTextSection: DiffSection = { + key: 'section', + path: 'big.txt', + status: 'modified', + added: 50_000, + removed: 0, + originalContent: '', + modifiedContent: '', + collapsed: false, + loading: true, + dirty: false, + diffResult: null, + largeDiffRenderLimit: null +} + describe('diff section layout', () => { it('uses Monaco measured content height for text diffs', () => { expect( @@ -26,18 +43,30 @@ describe('diff section layout', () => { it('uses the bounded fallback height for an on-demand diff', () => { expect( - getDiffSectionEstimatedHeight({ - collapsed: false, - measuredContentHeight: undefined, - originalContent: '', - modifiedContent: '', - changedLineCount: 60_000, - useIntrinsicImageHeight: false, - isLoadOnDemand: true - }) + getDiffSectionRowEstimatedHeight( + { ...largeTextSection, loading: false, loadOnDemand: true }, + undefined + ) ).toBe(188) }) + // Why: every viewer's virtualizer must estimate the same height DiffSectionItem + // renders for these rows, or each large row drifts ~100px per measure pass. + it('estimates deferred and in-flight large rows at the rendered fallback height', () => { + expect(getDiffSectionRowEstimatedHeight(largeTextSection, undefined)).toBe(188) + expect(getDiffSectionRowEstimatedHeight(largeTextSection, 3_800_000)).toBe(188) + expect( + getDiffSectionRowEstimatedHeight( + { ...largeTextSection, added: undefined, removed: undefined, path: 'vendor/blob.bin' }, + undefined + ) + ).toBe(88) + }) + + it('estimates a collapsed row as a header, whatever its size', () => { + expect(getDiffSectionRowEstimatedHeight({ ...largeTextSection, collapsed: true }, 500)).toBe(28) + }) + it('falls back to line-count height before Monaco has mounted', () => { expect( getDiffSectionBodyHeight({ diff --git a/src/renderer/src/components/editor/diff-section-layout.ts b/src/renderer/src/components/editor/diff-section-layout.ts index f8b7875c89f..1999925a266 100644 --- a/src/renderer/src/components/editor/diff-section-layout.ts +++ b/src/renderer/src/components/editor/diff-section-layout.ts @@ -39,7 +39,7 @@ export function getLargeDiffFallbackBodyHeight(): number { export function usesLargeDiffFallbackHeight( section: Pick< DiffSection, - 'added' | 'area' | 'largeDiffRenderLimit' | 'loading' | 'loadOnDemand' | 'path' | 'removed' + 'added' | 'largeDiffRenderLimit' | 'loading' | 'loadOnDemand' | 'path' | 'removed' > ): boolean { return ( @@ -93,18 +93,16 @@ export function getDiffSectionEstimatedHeight({ changedLineCount, useIntrinsicImageHeight, lineCounts, - isLargeDiffLimited = false, - isLoadOnDemand = false + isLargeDiffLimited = false }: DiffSectionBodyHeightInput & { collapsed: boolean isLargeDiffLimited?: boolean - isLoadOnDemand?: boolean }): number { if (collapsed) { return DIFF_SECTION_HEADER_HEIGHT } - if (isLargeDiffLimited || isLoadOnDemand) { + if (isLargeDiffLimited) { return DIFF_SECTION_HEADER_HEIGHT + getLargeDiffFallbackBodyHeight() } @@ -120,3 +118,39 @@ export function getDiffSectionEstimatedHeight({ }) ?? MIN_DIFF_SECTION_BODY_HEIGHT) ) } + +/** + * Single virtualizer estimate for a diff row, shared by the worktree and + * PR-review viewers so no viewer estimates a row the row's own layout metrics + * (useDiffSectionLayoutMetrics) would size from the bounded fallback instead. + */ +export function getDiffSectionRowEstimatedHeight( + section: Pick< + DiffSection, + | 'added' + | 'collapsed' + | 'diffResult' + | 'largeDiffRenderLimit' + | 'loading' + | 'loadOnDemand' + | 'modifiedContent' + | 'originalContent' + | 'path' + | 'removed' + >, + measuredContentHeight: number | undefined +): number { + return getDiffSectionEstimatedHeight({ + collapsed: section.collapsed, + measuredContentHeight, + originalContent: section.originalContent, + modifiedContent: section.modifiedContent, + changedLineCount: + section.added === undefined && section.removed === undefined + ? undefined + : (section.added ?? 0) + (section.removed ?? 0), + useIntrinsicImageHeight: isIntrinsicHeightImageDiff(section.diffResult), + isLargeDiffLimited: usesLargeDiffFallbackHeight(section), + lineCounts: section.largeDiffRenderLimit?.lineCounts ?? undefined + }) +} diff --git a/src/renderer/src/components/github-item-dialog/inspect-pull-request/pr-files-combined-diff-viewer.tsx b/src/renderer/src/components/github-item-dialog/inspect-pull-request/pr-files-combined-diff-viewer.tsx index 71772a01754..43c460469cd 100644 --- a/src/renderer/src/components/github-item-dialog/inspect-pull-request/pr-files-combined-diff-viewer.tsx +++ b/src/renderer/src/components/github-item-dialog/inspect-pull-request/pr-files-combined-diff-viewer.tsx @@ -4,10 +4,7 @@ import type { editor as monacoEditor } from 'monaco-editor' import type { DecoratedDiffComment } from '@/components/diff-comments/decorated-diff-comment' import { createCombinedDiffSectionIndexMap } from '../../editor/combined-diff/resolve-changes/combined-diff-section-identity' import { handleCombinedDiffFileTreeNavigation } from '../../editor/combined-diff/browse-files/combined-diff-file-tree-navigation' -import { - getDiffSectionEstimatedHeight, - isIntrinsicHeightImageDiff -} from '@/components/editor/diff-section-layout' +import { getDiffSectionRowEstimatedHeight } from '@/components/editor/diff-section-layout' import type { DiffSection } from '@/components/editor/diff-section-types' import { getCombinedDiffBranchEntriesInTreeOrder } from '../../editor/combined-diff/browse-files/combined-diff-file-tree-filter' import type { CombinedDiffFileTreeEntry } from '../../editor/combined-diff/resolve-changes/combined-diff-section-identity' @@ -239,19 +236,7 @@ function PRFilesCombinedDiffSections({ if (!section) { return 88 } - return getDiffSectionEstimatedHeight({ - collapsed: section.collapsed, - measuredContentHeight: sectionHeights[index], - originalContent: section.originalContent, - modifiedContent: section.modifiedContent, - changedLineCount: - section.added === undefined && section.removed === undefined - ? undefined - : (section.added ?? 0) + (section.removed ?? 0), - useIntrinsicImageHeight: isIntrinsicHeightImageDiff(section.diffResult), - isLargeDiffLimited: section.largeDiffRenderLimit?.limited === true, - lineCounts: section.largeDiffRenderLimit?.lineCounts ?? undefined - }) + return getDiffSectionRowEstimatedHeight(section, sectionHeights[index]) }, overscan: PR_DIFF_OVERSCAN, getItemKey: (index) => { diff --git a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx index 45419accec3..3ef4d097ab5 100644 --- a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx +++ b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx @@ -6,10 +6,7 @@ import { DiffSectionItem } from '@/components/editor/DiffSectionItem' import { CombinedDiffFileTree } from '../../editor/combined-diff/browse-files/combined-diff-file-tree' import { createCombinedDiffSectionIndexMap } from '../../editor/combined-diff/resolve-changes/combined-diff-section-identity' import { handleCombinedDiffFileTreeNavigation } from '../../editor/combined-diff/browse-files/combined-diff-file-tree-navigation' -import { - getDiffSectionEstimatedHeight, - isIntrinsicHeightImageDiff -} from '@/components/editor/diff-section-layout' +import { getDiffSectionRowEstimatedHeight } from '@/components/editor/diff-section-layout' import type { DiffSection } from '@/components/editor/diff-section-types' import { getCombinedDiffBranchEntriesInTreeOrder } from '../../editor/combined-diff/browse-files/combined-diff-file-tree-filter' import type { CombinedDiffFileTreeEntry } from '../../editor/combined-diff/resolve-changes/combined-diff-section-identity' @@ -201,19 +198,7 @@ export function PRFilesCombinedDiffViewer({ if (!section) { return 88 } - return getDiffSectionEstimatedHeight({ - collapsed: section.collapsed, - measuredContentHeight: sectionHeights[index], - originalContent: section.originalContent, - modifiedContent: section.modifiedContent, - changedLineCount: - section.added === undefined && section.removed === undefined - ? undefined - : (section.added ?? 0) + (section.removed ?? 0), - useIntrinsicImageHeight: isIntrinsicHeightImageDiff(section.diffResult), - isLargeDiffLimited: section.largeDiffRenderLimit?.limited === true, - lineCounts: section.largeDiffRenderLimit?.lineCounts ?? undefined - }) + return getDiffSectionRowEstimatedHeight(section, sectionHeights[index]) }, overscan: PR_DIFF_OVERSCAN, getItemKey: (index) => { diff --git a/src/shared/binary-file-extensions.test.ts b/src/shared/binary-file-extensions.test.ts new file mode 100644 index 00000000000..d54cd53638c --- /dev/null +++ b/src/shared/binary-file-extensions.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest' +import { hasBinaryFileExtension } from './binary-file-extensions' + +describe('hasBinaryFileExtension', () => { + it('matches known binary extensions case-insensitively', () => { + expect(hasBinaryFileExtension('docs/Shot.PNG')).toBe(true) + expect(hasBinaryFileExtension('vendor/lib.tar.gz')).toBe(true) + expect(hasBinaryFileExtension('C:\\assets\\theme.woff2')).toBe(true) + }) + + it('treats svg as text because the diff view renders its source', () => { + expect(hasBinaryFileExtension('assets/map.svg')).toBe(false) + }) + + it('rejects text files, dotfiles, and extensionless paths', () => { + expect(hasBinaryFileExtension('src/index.ts')).toBe(false) + expect(hasBinaryFileExtension('.gitignore')).toBe(false) + expect(hasBinaryFileExtension('scripts/.eslintrc')).toBe(false) + expect(hasBinaryFileExtension('Makefile')).toBe(false) + expect(hasBinaryFileExtension(undefined)).toBe(false) + }) + + it('does not match an extension that only appears in a directory name', () => { + expect(hasBinaryFileExtension('build.zip/manifest')).toBe(false) + }) +}) diff --git a/src/shared/binary-file-extensions.ts b/src/shared/binary-file-extensions.ts new file mode 100644 index 00000000000..52186066b23 --- /dev/null +++ b/src/shared/binary-file-extensions.ts @@ -0,0 +1,90 @@ +import { IMAGE_FILE_EXTENSIONS } from './image-file-extensions' + +// SVG is an image format that editors open as source text, so it stays out of +// the binary set even though it lives in IMAGE_FILE_EXTENSIONS. +const TEXT_IMAGE_EXTENSIONS = new Set(['.svg']) + +const NON_IMAGE_BINARY_EXTENSIONS = [ + // Archives + '.7z', + '.bz2', + '.gz', + '.jar', + '.rar', + '.tar', + '.tgz', + '.war', + '.xz', + '.zip', + '.zst', + // Audio and video + '.aac', + '.avi', + '.flac', + '.m4a', + '.mkv', + '.mov', + '.mp3', + '.mp4', + '.ogg', + '.wav', + '.webm', + // Documents + '.doc', + '.docx', + '.pdf', + '.ppt', + '.pptx', + '.xls', + '.xlsx', + // Fonts + '.eot', + '.otf', + '.ttc', + '.ttf', + '.woff', + '.woff2', + // Compiled artifacts and datastores + '.a', + '.bin', + '.class', + '.dll', + '.dylib', + '.exe', + '.idx', + '.lockb', + '.node', + '.o', + '.pack', + '.pyc', + '.pyd', + '.so', + '.sqlite', + '.sqlite3', + '.wasm' +] + +export const BINARY_FILE_EXTENSIONS: readonly string[] = Object.freeze([ + ...IMAGE_FILE_EXTENSIONS.filter((extension) => !TEXT_IMAGE_EXTENSIONS.has(extension)), + ...NON_IMAGE_BINARY_EXTENSIONS +]) + +const BINARY_FILE_EXTENSION_SET = new Set(BINARY_FILE_EXTENSIONS) + +/** + * Extension-only guess at "this file is not text". Content-based detection + * lives in `isBinaryBuffer`; use this only where the bytes are unavailable. + */ +export function hasBinaryFileExtension(filePath: string | undefined): boolean { + if (filePath === undefined) { + return false + } + const lowerPath = filePath.toLowerCase() + const dotIndex = lowerPath.lastIndexOf('.') + const separatorIndex = Math.max(lowerPath.lastIndexOf('/'), lowerPath.lastIndexOf('\\')) + // A leading dot is a dotfile (.gitignore), not an extension. + if (dotIndex <= separatorIndex + 1) { + return false + } + return BINARY_FILE_EXTENSION_SET.has(lowerPath.slice(dotIndex)) +}