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(