From 034fa509707fb947464284c9b46e06d8f6e8a030 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:52:31 -0700 Subject: [PATCH] test(diff): calibrate STA-3420 burst assertions against an idle baseline The burst window's peak lag is dominated by a one-off stall from opening 8x15k-line Monaco editors, which reproduces identically with invalidation disabled. Measure an equal-length idle window first and assert p95, sample coverage, and lag relative to that floor. Adds unit coverage for isUnchangedDiffSectionReload. Co-authored-by: Orca --- .../combined-diff-section-load-state.test.ts | 91 ++++++++++++++++++- ...ned-diff-invalidation-freeze-repro.spec.ts | 53 +++++++---- 2 files changed, 123 insertions(+), 21 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 d7cb4a236ce..8d8f6c6b9ef 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 @@ -1,5 +1,46 @@ import { describe, expect, it } from 'vitest' -import { shouldRequestCombinedDiffSectionLoad } from './combined-diff-section-load-state' +import type { GitDiffResult } from '../../../../shared/types' +import type { LargeDiffRenderLimit } from './large-diff-render-limit' +import { + isUnchangedDiffSectionReload, + shouldRequestCombinedDiffSectionLoad +} from './combined-diff-section-load-state' + +function textDiff(originalContent: string, modifiedContent: string): GitDiffResult { + return { + kind: 'text', + originalContent, + modifiedContent, + originalIsBinary: false, + modifiedIsBinary: false + } +} + +function renderLimit(limited: boolean): LargeDiffRenderLimit { + return limited + ? { + limited: true, + reason: 'line-count', + lineCounts: null, + characterCount: 0, + limits: { maxLinesPerSide: 1, maxCombinedCharacters: 1 } + } + : { limited: false, lineCounts: { original: 1, modified: 1 }, characterCount: 2 } +} + +function loaded( + originalContent: string, + modifiedContent: string, + overrides: { error?: string; limited?: boolean; diffResult?: GitDiffResult } = {} +): Parameters[0] { + return { + diffResult: overrides.diffResult ?? textDiff(originalContent, modifiedContent), + error: overrides.error, + largeDiffRenderLimit: renderLimit(overrides.limited ?? false), + originalContent, + modifiedContent + } +} describe('combined diff section load state', () => { it('requests content when a stale loaded marker has no diff result', () => { @@ -29,3 +70,51 @@ describe('combined diff section load state', () => { ) }) }) + +describe('isUnchangedDiffSectionReload', () => { + it('skips a revalidation that refetched the same text', () => { + expect(isUnchangedDiffSectionReload(loaded('before', 'after'), loaded('before', 'after'))).toBe( + true + ) + }) + + it('commits when either side of the content moved', () => { + expect( + isUnchangedDiffSectionReload(loaded('before', 'after'), loaded('before', 'rebased')) + ).toBe(false) + expect( + isUnchangedDiffSectionReload(loaded('before', 'after'), loaded('rebased', 'after')) + ).toBe(false) + }) + + it('commits when the error state changes in either direction', () => { + expect( + isUnchangedDiffSectionReload(loaded('a', 'b'), loaded('a', 'b', { error: 'boom' })) + ).toBe(false) + expect( + isUnchangedDiffSectionReload(loaded('a', 'b', { error: 'boom' }), loaded('a', 'b')) + ).toBe(false) + }) + + it('commits when the render limit crosses the fallback boundary', () => { + expect( + isUnchangedDiffSectionReload(loaded('a', 'b'), loaded('a', 'b', { limited: true })) + ).toBe(false) + }) + + it('never skips binary results, whose payload it cannot compare', () => { + const binary: GitDiffResult = { + kind: 'binary', + originalContent: '', + modifiedContent: '', + originalIsBinary: true, + modifiedIsBinary: true + } + expect( + isUnchangedDiffSectionReload( + loaded('', '', { diffResult: binary }), + loaded('', '', { diffResult: binary }) + ) + ).toBe(false) + }) +}) diff --git a/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts b/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts index 7e09d3f5ba7..4cbb3ff51fb 100644 --- a/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts +++ b/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts @@ -207,7 +207,7 @@ test.describe('Combined diff invalidation freeze repro (STA-3420)', () => { expect(opened.editorCount).toBeGreaterThan(0) const measurement = await orcaPage.evaluate( - async ({ wId, repoPath, relativePaths }) => { + async ({ wId, repoPath, relativePaths, burstDurationMs }) => { const intervalMs = 50 type LagWindow = { maxLagMs: number; p95LagMs: number; sampleCount: number } const startLagMeter = (): (() => LagWindow) => { @@ -245,27 +245,30 @@ test.describe('Combined diff invalidation freeze repro (STA-3420)', () => { } const settle = { ...stopSettle(), settleWindows } + // Why: settling still leaves occasional multi-hundred-ms stalls from the 8 mounted + // 15k-line Monaco editors. Measure an identical idle window so the burst is judged + // against this machine's floor rather than a fixed number. + const stopBaseline = startLagMeter() + await new Promise((resolve) => window.setTimeout(resolve, burstDurationMs)) + const baseline = stopBaseline() + const stopBurst = startLagMeter() const startedAt = performance.now() - try { - // Why: a rebase rewrites the worktree in bursts. The watcher debounces per - // path, so each notification lands in its OWN task — never batched together. - for (let round = 0; round < 3; round += 1) { - for (const relativePath of relativePaths) { - window.setTimeout(() => { - window.dispatchEvent( - new CustomEvent('orca:editor-external-file-change', { - detail: { worktreeId: wId, worktreePath: repoPath, relativePath } - }) - ) - }, 0) - } - await new Promise((resolve) => window.setTimeout(resolve, 1_000)) + // Why: a rebase rewrites the worktree in bursts. The watcher debounces per + // path, so each notification lands in its OWN task — never batched together. + for (let round = 0; round < 3; round += 1) { + for (const relativePath of relativePaths) { + window.setTimeout(() => { + window.dispatchEvent( + new CustomEvent('orca:editor-external-file-change', { + detail: { worktreeId: wId, worktreePath: repoPath, relativePath } + }) + ) + }, 0) } - await new Promise((resolve) => window.setTimeout(resolve, 15_000)) - } finally { - // no-op: burst meter stopped below so its window covers the awaits above + await new Promise((resolve) => window.setTimeout(resolve, 1_000)) } + await new Promise((resolve) => window.setTimeout(resolve, burstDurationMs - 3_000)) const burst = stopBurst() const rows = Array.from( @@ -274,7 +277,9 @@ test.describe('Combined diff invalidation freeze repro (STA-3420)', () => { return { elapsedMs: performance.now() - startedAt, settle, + baseline, burst, + expectedSampleCount: Math.floor(burstDurationMs / intervalMs), editorCount: document.querySelectorAll('.monaco-diff-editor').length, sectionRowCount: rows.length, stuckLoadingRowCount: rows.filter((row) => row.textContent?.includes('Loading diff')) @@ -284,14 +289,22 @@ test.describe('Combined diff invalidation freeze repro (STA-3420)', () => { { wId: worktreeId, repoPath: fixture.repoPath, - relativePaths: fixture.relativePaths + relativePaths: fixture.relativePaths, + burstDurationMs: 18_000 } ) console.log(`external-change burst measurement ${JSON.stringify(measurement)}`) expect(measurement.stuckLoadingRowCount).toBe(0) expect(measurement.editorCount).toBeGreaterThan(0) - expect(measurement.burst.maxLagMs).toBeLessThan(1_000) + // 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) + // 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( + Math.max(measurement.baseline.maxLagMs, 100) + 1_000 + ) } finally { rmSync(fixture.repoPath, { recursive: true, force: true }) }