diff --git a/src/renderer/src/components/editor/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/CombinedDiffViewer.tsx index e2e8da234..2c53bd872 100644 --- a/src/renderer/src/components/editor/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/CombinedDiffViewer.tsx @@ -79,7 +79,10 @@ import { beginCombinedDiffScrollbarDrag, type CombinedDiffScrollbarDragCleanup } from './combined-diff-scrollbar-drag' -import { shouldRequestCombinedDiffSectionLoad } from './combined-diff-section-load-state' +import { + isUnchangedDiffSectionReload, + shouldRequestCombinedDiffSectionLoad +} from './combined-diff-section-load-state' import { translate } from '@/i18n/i18n' type CachedCombinedDiffViewState = { @@ -162,6 +165,15 @@ let combinedDiffSideBySidePreference: boolean | null = null let combinedDiffFileTreeCollapsedPreference: boolean | null = null // Why: local Electron IPC has no RPC timeout; a hung git diff must become a retryable row error, not permanent "Loading...". const COMBINED_DIFF_SECTION_LOAD_TIMEOUT_MS = 30_000 +// Why: git rewrites a path several times during a rebase; refetch once the writes stop. +const COMBINED_DIFF_SECTION_RELOAD_COALESCE_MS = 300 + +function clearPendingSectionReloadTimers(timers: Map): void { + for (const timer of timers.values()) { + window.clearTimeout(timer) + } + timers.clear() +} class CombinedDiffSectionLoadTimeoutError extends Error { constructor() { @@ -296,8 +308,13 @@ export default function CombinedDiffViewer({ const loadingIndicesRef = useRef>(new Set()) const sectionsRef = useRef([]) const generationRef = useRef(0) + // Why: per-section reload token, so a sibling's reload can't discard this section's in-flight load. + const sectionLoadTokensRef = useRef>(new Map()) + const renderedIndicesRef = useRef>(new Set()) + const reloadTimersRef = useRef>(new Map()) const loadSectionRef = useRef<(index: number) => Promise>(async () => {}) const retrySectionRef = useRef<(index: number) => void>(() => {}) + const requestSectionReloadRef = useRef<(index: number) => void>(() => {}) const updateCombinedDiffScrollbar = useCallback(() => { const container = scrollContainerRef.current if (!container || container.scrollHeight <= container.clientHeight + 1) { @@ -564,6 +581,8 @@ export default function CombinedDiffViewer({ setSectionHeights({}) loadedIndicesRef.current.clear() loadingIndicesRef.current.clear() + sectionLoadTokensRef.current.clear() + clearPendingSectionReloadTimers(reloadTimersRef.current) loadSchedulerRef.current.reset() generationRef.current += 1 setGeneration((prev) => prev + 1) @@ -585,6 +604,7 @@ export default function CombinedDiffViewer({ loadingIndicesRef.current.add(index) const gen = generationRef.current + const loadToken = sectionLoadTokensRef.current.get(index) ?? 0 const entries = isAllMode ? allEntries : isBranchMode @@ -682,13 +702,41 @@ export default function CombinedDiffViewer({ })) : null - loadingIndicesRef.current.delete(index) if (generationRef.current !== gen) { + // Why: the generation reset already cleared the in-flight set, and a newer load for this + // index may own the entry now — deleting it here would hide that load from the guard above. + return + } + loadingIndicesRef.current.delete(index) + if ((sectionLoadTokensRef.current.get(index) ?? 0) !== loadToken) { + // Why: an invalidation landed mid-flight and deferred its reload to this settle point, so + // the refetch happens once here instead of racing a second fetch against this one. + requestSectionReloadRef.current(index) return } const storedContent = getStoredTextDiffContent(result, largeDiffRenderLimit) const storedResult = getStoredTextDiffResult(result, largeDiffRenderLimit) loadedIndicesRef.current.add(index) + const current = sectionsRef.current[index] + // A revalidation lands on a section that is already showing content. If the refetch matches + // what's on screen, committing it would swap Monaco models and re-measure for nothing. + const wasShowingContent = current !== undefined && !current.loading + if ( + wasShowingContent && + isUnchangedDiffSectionReload(current, { + diffResult: storedResult, + error, + largeDiffRenderLimit, + originalContent: storedContent.originalContent, + modifiedContent: storedContent.modifiedContent + }) + ) { + return + } + if (wasShowingContent) { + // Why: content really changed, so the old Monaco height no longer describes this row. + setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index)) + } setSections((prev) => { return prev.map((s, i) => i === index @@ -699,7 +747,11 @@ export default function CombinedDiffViewer({ modifiedContent: storedContent.modifiedContent, loading: false, error, - largeDiffRenderLimit + largeDiffRenderLimit, + // Why: models are keyed by path, so a changed refetch must not reuse the old model. + contentGeneration: wasShowingContent + ? (s.contentGeneration ?? 0) + 1 + : s.contentGeneration } : s ) @@ -728,8 +780,12 @@ export default function CombinedDiffViewer({ useEffect(() => { // Why: React StrictMode replays effect cleanup in dev; reset revives the scheduler for the replayed mount. const scheduler = loadSchedulerRef.current + const reloadTimers = reloadTimersRef.current scheduler.reset() - return () => scheduler.dispose() + return () => { + clearPendingSectionReloadTimers(reloadTimers) + scheduler.dispose() + } }, []) // Progressive loading: queue diff content when a section becomes visible. @@ -771,8 +827,14 @@ export default function CombinedDiffViewer({ loadedIndicesRef.current.delete(index) loadingIndicesRef.current.delete(index) invalidateCombinedDiffViewStateCache() - generationRef.current += 1 - setGeneration((prev) => prev + 1) + // Why: reloading one section must not bump the global generation — that is part of + // the virtualizer item key, so it would remount every rendered Monaco editor (STA-3420). + sectionLoadTokensRef.current.set(index, (sectionLoadTokensRef.current.get(index) ?? 0) + 1) + const coalesced = reloadTimersRef.current.get(index) + if (coalesced !== undefined) { + window.clearTimeout(coalesced) + reloadTimersRef.current.delete(index) + } setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index)) setSections((prev) => prev.map((section, sectionIndex) => @@ -840,10 +902,16 @@ export default function CombinedDiffViewer({ if (!section) { return `${index}:${generation}` } - return `${section.key}:${section.collapsed ? 'collapsed' : 'expanded'}:${generation}` + // Why: contentGeneration is per-section, so a single row's reload remounts only that row. + return `${section.key}:${section.collapsed ? 'collapsed' : 'expanded'}:${generation}:${section.contentGeneration ?? 0}` } }) const combinedDiffTotalSize = virtualizer.getTotalSize() + const combinedDiffVirtualItems = virtualizer.getVirtualItems() + // Why: keep render pure (React Doctor); retrySection still needs the on-screen set without the virtualizer as a dep. + useLayoutEffect(() => { + renderedIndicesRef.current = new Set(combinedDiffVirtualItems.map((item) => item.index)) + }, [combinedDiffVirtualItems]) const getCombinedDiffSectionKey = useCallback((section: DiffSection): string => section.key, []) const getCombinedDiffSectionElementKey = useCallback( (element: Element): string | null => @@ -929,8 +997,13 @@ export default function CombinedDiffViewer({ // Why: restore only on structural changes — restoring on measurement churn overwrote scrollTop during active wheel input. const combinedDiffRestoreSignal = useMemo( () => + // Why: a single-section reload drops that row's measured height, so it shifts rows + // below it — still a structural change even though `generation` no longer moves. `${generation}|${sideBySide ? 'sbs' : 'inline'}|${clampRestoreCount}|${sections - .map((section) => `${section.key}:${section.collapsed ? 'c' : 'e'}`) + .map( + (section) => + `${section.key}:${section.collapsed ? 'c' : 'e'}:${section.contentGeneration ?? 0}` + ) .join(',')}`, [clampRestoreCount, generation, sections, sideBySide] ) @@ -971,13 +1044,45 @@ export default function CombinedDiffViewer({ ) const sectionIndexByKeyRef = useRef(sectionIndexByKey) sectionIndexByKeyRef.current = sectionIndexByKey - const requestCombinedDiffSectionReload = useCallback((index: number): void => { - const section = sectionsRef.current[index] - if (!section || section.dirty) { - return - } - retrySectionRef.current(index) - }, []) + // Why: invalidation (rebase/commit/external write) revalidates in place — it must not tear the + // section down first. Clearing content up front forces a Monaco remodel even when the refetched + // diff is identical, which is what wedged the renderer during a rebase (STA-3420). + const requestCombinedDiffSectionReload = useCallback( + (index: number): void => { + const section = sectionsRef.current[index] + if (!section || section.dirty) { + return + } + loadedIndicesRef.current.delete(index) + invalidateCombinedDiffViewStateCache() + sectionLoadTokensRef.current.set(index, (sectionLoadTokensRef.current.get(index) ?? 0) + 1) + if (loadingIndicesRef.current.has(index)) { + // Why: the in-flight load now carries a stale token, so it re-drives this reload when it + // settles. Scheduling one here would fetch the same large diff a second time. + return + } + if (section.collapsed || !renderedIndicesRef.current.has(index)) { + // Why: a rebase invalidates every touched path at once. Refetching off-screen sections is + // unbounded work nobody can see; the row reloads on mount once it scrolls into view. + return + } + // Why: a rebase touches the same path many times over a few seconds. Without coalescing + // each touch refetches a whole diff, and the payload churn alone stalls the renderer. + const pending = reloadTimersRef.current.get(index) + if (pending !== undefined) { + window.clearTimeout(pending) + } + reloadTimersRef.current.set( + index, + window.setTimeout(() => { + reloadTimersRef.current.delete(index) + loadSchedulerRef.current.rerequest(index) + }, COMBINED_DIFF_SECTION_RELOAD_COALESCE_MS) + ) + }, + [invalidateCombinedDiffViewStateCache] + ) + requestSectionReloadRef.current = requestCombinedDiffSectionReload const ensureCombinedDiffSectionLoaded = useCallback((index: number): void => { const section = sectionsRef.current[index] if (!shouldRequestCombinedDiffSectionLoad(section, loadingIndicesRef.current.has(index))) { @@ -1886,7 +1991,7 @@ export default function CombinedDiffViewer({ > {skippedConflictNotice}
- {virtualizer.getVirtualItems().map((virtualItem) => { + {combinedDiffVirtualItems.map((virtualItem) => { const section = sections[virtualItem.index] if (!section) { return null 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 d7cb4a236..996e66c2f 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,63 @@ 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 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 + ? limitedRenderLimit() + : { limited: false, lineCounts: { original: 1, modified: 1 }, characterCount: 2 } +} + +function loaded( + originalContent: string, + modifiedContent: string, + overrides: { + error?: string + limited?: boolean + diffResult?: GitDiffResult + largeDiffRenderLimit?: LargeDiffRenderLimit + } = {} +): Parameters[0] { + return { + diffResult: overrides.diffResult ?? textDiff(originalContent, modifiedContent), + error: overrides.error, + 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', () => { @@ -29,3 +87,100 @@ 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('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', + originalContent: '', + modifiedContent: '', + originalIsBinary: true, + modifiedIsBinary: true + } + expect( + isUnchangedDiffSectionReload( + loaded('', '', { diffResult: binary }), + loaded('', '', { diffResult: binary }) + ) + ).toBe(false) + }) +}) 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 403554d01..7f789ecd9 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 @@ -8,3 +9,73 @@ export function shouldRequestCombinedDiffSectionLoad( ): boolean { return Boolean(section && section.diffResult === null && !section.error && !isLoading) } + +type ReloadedDiffSectionContent = Pick< + DiffSection, + '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. + * + * Why: a rebase fires one watcher event per touched path, and most of those refetch identical diffs. + */ +export function isUnchangedDiffSectionReload( + current: ReloadedDiffSectionContent, + next: ReloadedDiffSectionContent +): boolean { + if (current.error !== next.error) { + return false + } + // Only text diffs compare by content; binary/image results carry data this can't see. + if (current.diffResult?.kind !== 'text' || next.diffResult?.kind !== 'text') { + return 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 new file mode 100644 index 000000000..cb11acf25 --- /dev/null +++ b/tests/e2e/combined-diff-invalidation-freeze-repro.spec.ts @@ -0,0 +1,320 @@ +import { execFileSync } from 'node:child_process' +import { rmSync } from 'node:fs' +import type { Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { waitForSessionReady } from './helpers/store' +import { + createIsolatedManyFileStagedDiffRepo, + createIsolatedStagedLocaleDiffRepo +} from './large-diff-repro-fixtures' + +async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise { + const repoId = await orcaPage.evaluate(async (pathToRepo: string) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const addedRepo = await store.getState().addRepoPath(pathToRepo) + if (!addedRepo) { + throw new Error(`isolated repo not found: ${pathToRepo}`) + } + return addedRepo.id + }, repoPath) + + await expect + .poll( + () => + orcaPage.evaluate(async (targetRepoId: string) => { + const store = window.__store + if (!store) { + return 0 + } + await store.getState().fetchWorktrees(targetRepoId) + return store.getState().worktreesByRepo[targetRepoId]?.length ?? 0 + }, repoId), + { timeout: 30_000, message: 'isolated staged-diff worktree did not load' } + ) + .toBeGreaterThan(0) + + return orcaPage.evaluate( + ({ targetRepoId, pathToRepo }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const state = store.getState() + const worktrees = state.worktreesByRepo[targetRepoId] ?? [] + const worktree = worktrees.find((entry) => entry.path === pathToRepo) ?? worktrees[0] + if (!worktree) { + throw new Error(`isolated worktree not found: ${pathToRepo}`) + } + state.setActiveRepo(targetRepoId) + state.setActiveWorktree(worktree.id) + return worktree.id + }, + { targetRepoId: repoId, pathToRepo: repoPath } + ) +} + +test.describe('Combined diff invalidation freeze repro (STA-3420)', () => { + test.describe.configure({ mode: 'serial' }) + test.use({ seedTestRepo: false }) + + test('committing under an open Staged Changes diff keeps the renderer responsive', async ({ + orcaPage + }) => { + await waitForSessionReady(orcaPage) + const fixture = createIsolatedStagedLocaleDiffRepo() + + try { + const worktreeId = await addAndActivateRepo(orcaPage, fixture.repoPath) + + const opened = await orcaPage.evaluate( + async ({ wId, repoPath }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const status = await window.api.git.status({ worktreePath: repoPath }) + store.getState().setGitStatus(wId, status) + const staged = status.entries.filter((entry) => entry.area === 'staged') + if (staged.length === 0) { + throw new Error('fixture produced no staged entries') + } + // Why: mirrors the Source Control "Staged Changes" tab, which snapshots entries at open. + store.getState().openAllDiffs(wId, repoPath, undefined, 'staged', staged) + + const startedAt = performance.now() + let editorCount = 0 + while (performance.now() - startedAt < 30_000) { + await new Promise((resolve) => window.setTimeout(resolve, 50)) + editorCount = document.querySelectorAll('.monaco-diff-editor').length + if (editorCount > 0) { + await new Promise((resolve) => window.setTimeout(resolve, 1_500)) + editorCount = document.querySelectorAll('.monaco-diff-editor').length + break + } + } + return { stagedCount: staged.length, editorCount } + }, + { wId: worktreeId, repoPath: fixture.repoPath } + ) + console.log(`staged diff opened ${JSON.stringify(opened)}`) + expect(opened.editorCount).toBeGreaterThan(0) + + // Why: the reported freeze starts when the open diff is invalidated by a + // commit/rebase — the snapshot files stop having any staged diff at all. + execFileSync('git', ['commit', '-m', 'Invalidate the open staged diff'], { + cwd: fixture.repoPath, + stdio: 'pipe' + }) + + const measurement = await orcaPage.evaluate( + async ({ wId, repoPath }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + + const intervalMs = 50 + const samples: number[] = [] + let last = performance.now() + let maxLagMs = 0 + const timer = window.setInterval(() => { + const now = performance.now() + const lag = Math.max(0, now - last - intervalMs) + maxLagMs = Math.max(maxLagMs, lag) + samples.push(lag) + last = now + }, intervalMs) + + const startedAt = performance.now() + try { + // Why: the file watcher pushes several status refreshes while git + // rewrites the index; replay that churn instead of a single update. + for (let round = 0; round < 3; round += 1) { + const status = await window.api.git.status({ worktreePath: repoPath }) + store.getState().setGitStatus(wId, status) + await new Promise((resolve) => window.setTimeout(resolve, 700)) + } + await new Promise((resolve) => window.setTimeout(resolve, 3_000)) + } finally { + window.clearInterval(timer) + } + + const sorted = [...samples].sort((a, b) => a - b) + return { + elapsedMs: performance.now() - startedAt, + maxLagMs, + p95LagMs: sorted.length ? sorted[Math.floor(sorted.length * 0.95)] : 0, + sampleCount: samples.length, + editorCount: document.querySelectorAll('.monaco-diff-editor').length, + loadingRowCount: Array.from( + document.querySelectorAll('[data-combined-diff-section-row]') + ).filter((row) => row.textContent?.includes('Loading diff')).length, + sectionRowCount: document.querySelectorAll('[data-combined-diff-section-row]').length + } + }, + { wId: worktreeId, repoPath: fixture.repoPath } + ) + + console.log(`invalidation measurement ${JSON.stringify(measurement)}`) + expect(measurement.maxLagMs).toBeLessThan(1_000) + // Why: staying responsive isn't enough — invalidation must also leave the rows loaded + // instead of parking a section in its loading state. + expect(measurement.loadingRowCount).toBe(0) + expect(measurement.editorCount).toBeGreaterThan(0) + } finally { + rmSync(fixture.repoPath, { recursive: true, force: true }) + } + }) + + test('a rebase-style burst of external file changes keeps the diff responsive and loaded', async ({ + orcaPage + }) => { + test.setTimeout(240_000) + await waitForSessionReady(orcaPage) + // Why: few but very large sections — the reported freeze is a *large* diff view, + // where every remount re-runs Monaco's diff over thousands of changed lines. + const fixture = createIsolatedManyFileStagedDiffRepo(8, 15_000) + + try { + const worktreeId = await addAndActivateRepo(orcaPage, fixture.repoPath) + + const opened = await orcaPage.evaluate( + async ({ wId, repoPath }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const status = await window.api.git.status({ worktreePath: repoPath }) + store.getState().setGitStatus(wId, status) + const staged = status.entries.filter((entry) => entry.area === 'staged') + store.getState().openAllDiffs(wId, repoPath, undefined, 'staged', staged) + + const startedAt = performance.now() + let editorCount = 0 + while (performance.now() - startedAt < 30_000) { + await new Promise((resolve) => window.setTimeout(resolve, 50)) + editorCount = document.querySelectorAll('.monaco-diff-editor').length + if (editorCount > 0) { + await new Promise((resolve) => window.setTimeout(resolve, 1_500)) + editorCount = document.querySelectorAll('.monaco-diff-editor').length + break + } + } + return { stagedCount: staged.length, editorCount } + }, + { wId: worktreeId, repoPath: fixture.repoPath } + ) + console.log(`staged diff opened for burst ${JSON.stringify(opened)}`) + expect(opened.editorCount).toBeGreaterThan(0) + + const measurement = await orcaPage.evaluate( + async ({ wId, repoPath, relativePaths, burstDurationMs }) => { + const intervalMs = 50 + type LagWindow = { maxLagMs: number; p95LagMs: number; sampleCount: number } + const startLagMeter = (): (() => LagWindow) => { + const samples: number[] = [] + let last = performance.now() + let maxLagMs = 0 + const timer = window.setInterval(() => { + const now = performance.now() + maxLagMs = Math.max(maxLagMs, Math.max(0, now - last - intervalMs)) + samples.push(Math.max(0, now - last - intervalMs)) + last = now + }, intervalMs) + return () => { + window.clearInterval(timer) + const sorted = [...samples].sort((a, b) => a - b) + return { + maxLagMs, + p95LagMs: sorted.length ? sorted[Math.floor(sorted.length * 0.95)] : 0, + sampleCount: samples.length + } + } + } + + // Why: opening 8 huge Monaco diffs is itself expensive. Wait for the main thread to go + // quiet first, so the burst window reports invalidation cost and not open cost. + const stopSettle = startLagMeter() + const settleStartedAt = performance.now() + let settleWindows = 0 + let quietWindows = 0 + while (performance.now() - settleStartedAt < 60_000 && quietWindows < 2) { + const stopWindow = startLagMeter() + await new Promise((resolve) => window.setTimeout(resolve, 1_000)) + settleWindows += 1 + quietWindows = stopWindow().maxLagMs < 100 ? quietWindows + 1 : 0 + } + 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() + // 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)) + } + await new Promise((resolve) => window.setTimeout(resolve, burstDurationMs - 3_000)) + const burst = stopBurst() + + const rows = Array.from( + document.querySelectorAll('[data-combined-diff-section-row]') + ) as HTMLElement[] + 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')) + .length + } + }, + { + wId: worktreeId, + repoPath: fixture.repoPath, + 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) + // Why: before the fix this window blocked continuously — p95 3963ms, 16 samples in 23s. + // 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).toBeLessThanOrEqual(measurement.baseline.p95LagMs + 100) + expect(measurement.burst.sampleCount).toBeGreaterThanOrEqual( + 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).toBeLessThanOrEqual( + Math.max(measurement.baseline.maxLagMs, 100) + 1_000 + ) + } finally { + rmSync(fixture.repoPath, { recursive: true, force: true }) + } + }) +}) diff --git a/tests/e2e/large-diff-repro-fixtures.ts b/tests/e2e/large-diff-repro-fixtures.ts index 7790ccccc..7ae422f2b 100644 --- a/tests/e2e/large-diff-repro-fixtures.ts +++ b/tests/e2e/large-diff-repro-fixtures.ts @@ -62,6 +62,53 @@ function modifyLocaleLikeJson(content: string, fileIndex: number): string { return lines.join('\n') } +function buildSourceLikeFile(fileIndex: number, lineCount: number, revision: number): string { + const lines: string[] = [] + for (let i = 0; i < lineCount; i += 1) { + const changed = i % 12 === 0 + lines.push( + changed + ? `export const value_${fileIndex}_${i} = 'rev${revision} ${'payload '.repeat(6).trim()}'` + : `export const value_${fileIndex}_${i} = 'base ${'payload '.repeat(6).trim()}'` + ) + } + return `${lines.join('\n')}\n` +} + +/** Many staged sections, each with a real multi-hunk diff — the shape a rebase invalidates. */ +export function createIsolatedManyFileStagedDiffRepo( + fileCount = 120, + lineCount = 600 +): IsolatedStagedLocaleDiffRepo { + const repoPath = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-many-file-repro-'))) + runGit(repoPath, ['init']) + runGit(repoPath, ['config', 'user.email', 'e2e@test.local']) + runGit(repoPath, ['config', 'user.name', 'E2E Test']) + + mkdirSync(path.join(repoPath, 'src'), { recursive: true }) + const relativePaths: string[] = [] + for (let fileIndex = 0; fileIndex < fileCount; fileIndex += 1) { + const relativePath = path.posix.join('src', `module-${String(fileIndex).padStart(4, '0')}.ts`) + writeFileSync( + path.join(repoPath, ...relativePath.split(path.posix.sep)), + buildSourceLikeFile(fileIndex, lineCount, 0) + ) + relativePaths.push(relativePath) + } + runGit(repoPath, ['add', '-A']) + runGit(repoPath, ['commit', '-m', 'Initial many-file fixture']) + + for (let fileIndex = 0; fileIndex < fileCount; fileIndex += 1) { + writeFileSync( + path.join(repoPath, ...relativePaths[fileIndex].split(path.posix.sep)), + buildSourceLikeFile(fileIndex, lineCount, 1) + ) + } + runGit(repoPath, ['add', '-A']) + + return { repoPath, relativePaths } +} + export function createIsolatedStagedLocaleDiffRepo(): IsolatedStagedLocaleDiffRepo { const repoPath = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-staged-locale-repro-'))) runGit(repoPath, ['init'])