Fix unchanged diff-section reload detection for truncated diffs

When a diff exceeds render limits, content is pruned to '' for memory.
The old check compared content equality, so limited reloads always
appeared changed, triggering unnecessary revalidation that froze the UI.

Compare render-limit metadata instead — it's the sole change signal
and full description of what the fallback banner displays.

Also calibrate STA-3420 e2e assertions relative to idle baseline for
machine independence instead of absolute thresholds.
This commit is contained in:
Jinjing
2026-08-04 21:23:56 -07:00
parent d65921c13b
commit b3abd40aaf
3 changed files with 122 additions and 15 deletions
@@ -16,32 +16,49 @@ function textDiff(originalContent: string, modifiedContent: string): GitDiffResu
}
}
function limitedRenderLimit(
overrides: Partial<Extract<LargeDiffRenderLimit, { limited: true }>> = {}
): LargeDiffRenderLimit {
return {
limited: true,
reason: 'line-count',
lineCounts: null,
characterCount: 0,
limits: { maxLinesPerSide: 1, maxCombinedCharacters: 1 },
...overrides
}
}
function renderLimit(limited: boolean): LargeDiffRenderLimit {
return limited
? {
limited: true,
reason: 'line-count',
lineCounts: null,
characterCount: 0,
limits: { maxLinesPerSide: 1, maxCombinedCharacters: 1 }
}
? limitedRenderLimit()
: { limited: false, lineCounts: { original: 1, modified: 1 }, characterCount: 2 }
}
function loaded(
originalContent: string,
modifiedContent: string,
overrides: { error?: string; limited?: boolean; diffResult?: GitDiffResult } = {}
overrides: {
error?: string
limited?: boolean
diffResult?: GitDiffResult
largeDiffRenderLimit?: LargeDiffRenderLimit
} = {}
): Parameters<typeof isUnchangedDiffSectionReload>[0] {
return {
diffResult: overrides.diffResult ?? textDiff(originalContent, modifiedContent),
error: overrides.error,
largeDiffRenderLimit: renderLimit(overrides.limited ?? false),
largeDiffRenderLimit: overrides.largeDiffRenderLimit ?? renderLimit(overrides.limited ?? false),
originalContent,
modifiedContent
}
}
// Limited sections store empty content, so every case below turns on render-limit metadata alone.
function limited(largeDiffRenderLimit: LargeDiffRenderLimit) {
return loaded('', '', { largeDiffRenderLimit })
}
describe('combined diff section load state', () => {
it('requests content when a stale loaded marker has no diff result', () => {
expect(
@@ -102,6 +119,55 @@ describe('isUnchangedDiffSectionReload', () => {
).toBe(false)
})
it('skips a limited reload whose render-limit metadata is identical', () => {
const unchanged = limitedRenderLimit({ lineCounts: { original: 400_000, modified: 400_000 } })
expect(isUnchangedDiffSectionReload(limited(unchanged), limited(unchanged))).toBe(true)
})
it('commits when a limited reload moves line counts', () => {
expect(
isUnchangedDiffSectionReload(
limited(limitedRenderLimit({ lineCounts: { original: 400_000, modified: 400_000 } })),
limited(limitedRenderLimit({ lineCounts: { original: 400_000, modified: 401_000 } }))
)
).toBe(false)
expect(
isUnchangedDiffSectionReload(
limited(limitedRenderLimit({ lineCounts: null })),
limited(limitedRenderLimit({ lineCounts: { original: 400_000, modified: 400_000 } }))
)
).toBe(false)
})
it('commits when a limited reload moves the character count or reason', () => {
expect(
isUnchangedDiffSectionReload(
limited(limitedRenderLimit({ characterCount: 7_000_000 })),
limited(limitedRenderLimit({ characterCount: 9_000_000 }))
)
).toBe(false)
expect(
isUnchangedDiffSectionReload(
limited(limitedRenderLimit({ reason: 'line-count' })),
limited(limitedRenderLimit({ reason: 'character-count' }))
)
).toBe(false)
})
it('commits when a limited reload stops reporting line counts as a floor', () => {
expect(
isUnchangedDiffSectionReload(
limited(
limitedRenderLimit({
lineCounts: { original: 120_001, modified: 0 },
lineCountsAreMinimum: { original: true, modified: false }
})
),
limited(limitedRenderLimit({ lineCounts: { original: 120_001, modified: 0 } }))
)
).toBe(false)
})
it('never skips binary results, whose payload it cannot compare', () => {
const binary: GitDiffResult = {
kind: 'binary',
@@ -1,3 +1,4 @@
import type { DiffLineCounts, LargeDiffRenderLimit } from './large-diff-render-limit'
import type { DiffSection } from './diff-section-types'
// Why: `diffResult === null` subsumes a dirty check — `dirty` is only ever set from a mounted
@@ -14,6 +15,30 @@ type ReloadedDiffSectionContent = Pick<
'diffResult' | 'error' | 'largeDiffRenderLimit' | 'originalContent' | 'modifiedContent'
>
type LimitedDiffRenderLimit = Extract<LargeDiffRenderLimit, { limited: true }>
function isSameDiffLineCounts(
current: DiffLineCounts | null,
next: DiffLineCounts | null
): boolean {
if (!current || !next) {
return current === next
}
return current.original === next.original && current.modified === next.modified
}
function isSameLineCountMinimums(
current: LimitedDiffRenderLimit,
next: LimitedDiffRenderLimit
): boolean {
return (
(current.lineCountsAreMinimum?.original ?? false) ===
(next.lineCountsAreMinimum?.original ?? false) &&
(current.lineCountsAreMinimum?.modified ?? false) ===
(next.lineCountsAreMinimum?.modified ?? false)
)
}
/**
* True when a revalidation refetched exactly what the section already displays, so committing it
* would swap Monaco models and re-measure the row for no visible change.
@@ -31,12 +56,24 @@ export function isUnchangedDiffSectionReload(
if (current.diffResult?.kind !== 'text' || next.diffResult?.kind !== 'text') {
return false
}
if (
(current.largeDiffRenderLimit?.limited ?? false) !==
(next.largeDiffRenderLimit?.limited ?? false)
) {
const currentLimit = current.largeDiffRenderLimit
const nextLimit = next.largeDiffRenderLimit
if ((currentLimit?.limited ?? false) !== (nextLimit?.limited ?? false)) {
return false
}
// Why: limited sections prune their content to '', so the content compare below can't see a
// refetch move. The fallback banner renders only this metadata, so it is both the sole change
// signal and the full description of what's on screen.
if (currentLimit?.limited === true && nextLimit?.limited === true) {
return (
currentLimit.reason === nextLimit.reason &&
currentLimit.characterCount === nextLimit.characterCount &&
currentLimit.limits.maxLinesPerSide === nextLimit.limits.maxLinesPerSide &&
currentLimit.limits.maxCombinedCharacters === nextLimit.limits.maxCombinedCharacters &&
isSameDiffLineCounts(currentLimit.lineCounts, nextLimit.lineCounts) &&
isSameLineCountMinimums(currentLimit, nextLimit)
)
}
return (
current.originalContent === next.originalContent &&
current.modifiedContent === next.modifiedContent
@@ -298,8 +298,12 @@ test.describe('Combined diff invalidation freeze repro (STA-3420)', () => {
expect(measurement.stuckLoadingRowCount).toBe(0)
expect(measurement.editorCount).toBeGreaterThan(0)
// Why: before the fix this window blocked continuously — p95 3963ms, 16 samples in 23s.
expect(measurement.burst.p95LagMs).toBeLessThan(100)
expect(measurement.burst.sampleCount).toBeGreaterThan(measurement.expectedSampleCount * 0.85)
// Every limit rides the identical idle window so a slow machine's floor can't fail the test;
// the allowances on top are what the burst itself is permitted to add.
expect(measurement.burst.p95LagMs).toBeLessThan(measurement.baseline.p95LagMs + 100)
expect(measurement.burst.sampleCount).toBeGreaterThan(
Math.min(measurement.baseline.sampleCount, measurement.expectedSampleCount) * 0.85
)
// Why: peak lag tracks the idle floor of this fixture, not invalidation; only a regression
// that adds a full extra second of blocking on top of that floor is this bug returning.
expect(measurement.burst.maxLagMs).toBeLessThan(