fix: address review findings (#2124)

This commit is contained in:
Jinjing 2026-05-16 16:29:35 -07:00 committed by GitHub
parent f015202a03
commit 9f70aa160b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 117 additions and 5 deletions

View File

@ -546,6 +546,10 @@ export default function CombinedDiffViewer({
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)
})
},

View File

@ -242,6 +242,15 @@ export function DiffSectionItem({
section.status
]
)
const changedLineCount = useMemo(() => {
if (lineStats) {
return lineStats.added + lineStats.removed
}
if (section.added === undefined && section.removed === undefined) {
return undefined
}
return (section.added ?? 0) + (section.removed ?? 0)
}, [lineStats, section.added, section.removed])
// 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)
@ -249,6 +258,7 @@ export function DiffSectionItem({
measuredContentHeight: sectionHeight,
originalContent: section.originalContent,
modifiedContent: section.modifiedContent,
changedLineCount,
useIntrinsicImageHeight
})
@ -258,6 +268,10 @@ export function DiffSectionItem({
lineNumberOptionsSubRef.current = applyDiffEditorLineNumberOptions(editor, sideBySide)
const modified = editor.getModifiedEditor()
// Why: measuring before Monaco computes hidden unchanged regions records
// full-file height, making virtualized combined diffs jump as rows remount.
let diffLayoutReady = false
let pendingHeightFrame: number | null = null
const updateHeight = (): void => {
const contentHeight = editor.getModifiedEditor().getContentHeight()
setSectionHeights((prev) => {
@ -267,8 +281,28 @@ export function DiffSectionItem({
return { ...prev, [index]: contentHeight }
})
}
modified.onDidContentSizeChange(updateHeight)
updateHeight()
const requestHeightUpdate = (): void => {
if (pendingHeightFrame !== null) {
return
}
pendingHeightFrame = window.requestAnimationFrame(() => {
pendingHeightFrame = null
updateHeight()
})
}
const markDiffLayoutReady = (): void => {
diffLayoutReady = true
requestHeightUpdate()
}
const contentSizeSub = modified.onDidContentSizeChange(() => {
if (diffLayoutReady) {
requestHeightUpdate()
}
})
const diffUpdateSub = editor.onDidUpdateDiff(markDiffLayoutReady)
if (editor.getLineChanges() !== null) {
markDiffLayoutReady()
}
setModifiedEditor(modified)
// Why: Monaco disposes inner editors when the DiffEditor container is
@ -277,6 +311,12 @@ export function DiffSectionItem({
// methods on a disposed editor instance, and avoids `popover` pointing
// at a line in an editor that no longer exists.
modified.onDidDispose(() => {
contentSizeSub.dispose()
diffUpdateSub.dispose()
if (pendingHeightFrame !== null) {
window.cancelAnimationFrame(pendingHeightFrame)
pendingHeightFrame = null
}
lineNumberOptionsSubRef.current?.dispose()
lineNumberOptionsSubRef.current = null
diffEditorRef.current = null

View File

@ -29,6 +29,37 @@ describe('diff section layout', () => {
).toBe(76)
})
it('uses changed-line count before Monaco reports collapsed diff height', () => {
const largeUnchangedFile = Array.from({ length: 10_000 }, (_, index) => `line ${index}`).join(
'\n'
)
expect(
getDiffSectionBodyHeight({
measuredContentHeight: undefined,
originalContent: largeUnchangedFile,
modifiedContent: `${largeUnchangedFile}\nchanged`,
changedLineCount: 1,
useIntrinsicImageHeight: false
})
).toBe(266)
})
it('caps unmeasured text diffs without changed-line stats', () => {
const largeUnchangedFile = Array.from({ length: 10_000 }, (_, index) => `line ${index}`).join(
'\n'
)
expect(
getDiffSectionBodyHeight({
measuredContentHeight: undefined,
originalContent: largeUnchangedFile,
modifiedContent: `${largeUnchangedFile}\nchanged`,
useIntrinsicImageHeight: false
})
).toBe(1539)
})
it('keeps empty text sections visible', () => {
expect(
getDiffSectionBodyHeight({
@ -93,11 +124,29 @@ describe('diff section layout', () => {
measuredContentHeight: undefined,
originalContent: 'one',
modifiedContent: 'one\ntwo\nthree',
changedLineCount: 2,
useIntrinsicImageHeight: false
})
).toBe(104)
})
it('uses changed-line count for large virtualized expanded sections', () => {
const largeUnchangedFile = Array.from({ length: 10_000 }, (_, index) => `line ${index}`).join(
'\n'
)
expect(
getDiffSectionEstimatedHeight({
collapsed: false,
measuredContentHeight: undefined,
originalContent: largeUnchangedFile,
modifiedContent: `${largeUnchangedFile}\nchanged`,
changedLineCount: 1,
useIntrinsicImageHeight: false
})
).toBe(294)
})
it('estimates collapsed virtualized sections as header-only rows', () => {
expect(
getDiffSectionEstimatedHeight({

View File

@ -4,11 +4,14 @@ const DIFF_LINE_HEIGHT = 19
const DIFF_SECTION_PADDING_HEIGHT = 19
const MIN_DIFF_SECTION_BODY_HEIGHT = 60
const DIFF_SECTION_HEADER_HEIGHT = 28
const DIFF_UNCHANGED_CONTEXT_LINE_ESTIMATE = 12
const MAX_UNMEASURED_TEXT_BODY_LINES = 80
type DiffSectionBodyHeightInput = {
measuredContentHeight: number | undefined
originalContent: string
modifiedContent: string
changedLineCount?: number
useIntrinsicImageHeight: boolean
}
@ -20,6 +23,7 @@ export function getDiffSectionBodyHeight({
measuredContentHeight,
originalContent,
modifiedContent,
changedLineCount,
useIntrinsicImageHeight
}: DiffSectionBodyHeightInput): number | undefined {
if (useIntrinsicImageHeight) {
@ -30,11 +34,24 @@ export function getDiffSectionBodyHeight({
return measuredContentHeight + DIFF_SECTION_PADDING_HEIGHT
}
const fullLineCount = Math.max(
originalContent.split('\n').length,
modifiedContent.split('\n').length
)
const estimatedLineCount =
changedLineCount !== undefined
? Math.min(
fullLineCount,
Math.max(2, changedLineCount + DIFF_UNCHANGED_CONTEXT_LINE_ESTIMATE)
)
: Math.min(fullLineCount, MAX_UNMEASURED_TEXT_BODY_LINES)
// Why: combined diffs hide unchanged regions inside Monaco. Before Monaco
// reports its collapsed content height, sizing from full file length makes
// large files flash open and forces the virtualizer to jump on scroll.
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
estimatedLineCount * DIFF_LINE_HEIGHT + DIFF_SECTION_PADDING_HEIGHT
)
}
@ -43,6 +60,7 @@ export function getDiffSectionEstimatedHeight({
measuredContentHeight,
originalContent,
modifiedContent,
changedLineCount,
useIntrinsicImageHeight
}: DiffSectionBodyHeightInput & { collapsed: boolean }): number {
if (collapsed) {
@ -55,6 +73,7 @@ export function getDiffSectionEstimatedHeight({
measuredContentHeight,
originalContent,
modifiedContent,
changedLineCount,
useIntrinsicImageHeight
}) ?? MIN_DIFF_SECTION_BODY_HEIGHT)
)