From b3abd40aafe3da52f3b48ae48fc6070f0fe18990 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 4 Aug 2026 21:23:56 -0700 Subject: [PATCH] Fix unchanged diff-section reload detection for truncated diffs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../combined-diff-section-load-state.test.ts | 84 +++++++++++++++++-- .../combined-diff-section-load-state.ts | 45 +++++++++- ...ned-diff-invalidation-freeze-repro.spec.ts | 8 +- 3 files changed, 122 insertions(+), 15 deletions(-) diff --git a/src/renderer/src/components/editor/combined-diff-section-load-state.test.ts b/src/renderer/src/components/editor/combined-diff-section-load-state.test.ts index 8d8f6c6b9ef..996e66c2f5a 100644 --- a/src/renderer/src/components/editor/combined-diff-section-load-state.test.ts +++ b/src/renderer/src/components/editor/combined-diff-section-load-state.test.ts @@ -16,32 +16,49 @@ function textDiff(originalContent: string, modifiedContent: string): GitDiffResu } } +function limitedRenderLimit( + overrides: Partial> = {} +): 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[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', diff --git a/src/renderer/src/components/editor/combined-diff-section-load-state.ts b/src/renderer/src/components/editor/combined-diff-section-load-state.ts index 7599bee5da2..7f789ecd9ed 100644 --- a/src/renderer/src/components/editor/combined-diff-section-load-state.ts +++ b/src/renderer/src/components/editor/combined-diff-section-load-state.ts @@ -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 + +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 diff --git a/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts b/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts index 4cbb3ff51fb..4e4224b343d 100644 --- a/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts +++ b/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts @@ -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(