diff --git a/src/renderer/src/components/editor/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/CombinedDiffViewer.tsx index 00fdc450bbf..a54f2c86a36 100644 --- a/src/renderer/src/components/editor/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/CombinedDiffViewer.tsx @@ -66,6 +66,7 @@ import { import { getCombinedBranchEntries, getCombinedUncommittedEntries, + resolveCombinedUncommittedSnapshotEntries, shouldAutoReloadCombinedDiffFromGitStatus } from './combined-diff-entries' import { getCombinedDiffCommitMessageBody } from './combined-diff-commit-message' @@ -423,10 +424,35 @@ export default function CombinedDiffViewer({ () => file.uncommittedEntriesSnapshot?.filter((e) => e.conflictStatus !== 'unresolved'), [file.uncommittedEntriesSnapshot] ) + const retainedResolvedSnapshotEntries = React.useMemo( + () => + new Map( + sectionsRef.current + .filter((section) => section.area !== undefined) + .map((section) => [ + section.path, + { + path: section.path, + status: section.status as GitStatusEntry['status'], + area: section.area!, + oldPath: section.oldPath, + added: section.added, + removed: section.removed + } + ]) + ), + [sections] + ) const uncommittedEntries = React.useMemo( () => - snapshotEntries ?? getCombinedUncommittedEntries(gitStatusEntries, file.combinedAreaFilter), - [snapshotEntries, gitStatusEntries, file.combinedAreaFilter] + snapshotEntries + ? resolveCombinedUncommittedSnapshotEntries( + snapshotEntries, + gitStatusEntries, + retainedResolvedSnapshotEntries + ) + : getCombinedUncommittedEntries(gitStatusEntries, file.combinedAreaFilter), + [snapshotEntries, gitStatusEntries, retainedResolvedSnapshotEntries, file.combinedAreaFilter] ) const branchEntries = React.useMemo(() => { return getCombinedBranchEntries(file.branchEntriesSnapshot, liveBranchEntries) diff --git a/src/renderer/src/components/editor/combined-diff-entries.test.ts b/src/renderer/src/components/editor/combined-diff-entries.test.ts index 859cd92d882..bc0733a6b07 100644 --- a/src/renderer/src/components/editor/combined-diff-entries.test.ts +++ b/src/renderer/src/components/editor/combined-diff-entries.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { getCombinedBranchEntries, getCombinedUncommittedEntries, + resolveCombinedUncommittedSnapshotEntries, shouldAutoReloadCombinedDiffFromGitStatus } from './combined-diff-entries' import type { GitBranchChangeEntry, GitStatusEntry } from '../../../../shared/types' @@ -50,6 +51,61 @@ describe('getCombinedUncommittedEntries', () => { }) }) +describe('resolveCombinedUncommittedSnapshotEntries', () => { + it('uses the live staged area when a snapshot unstaged file has been staged', () => { + const snapshotEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'unstaged', added: 2, removed: 1 } + ] + const liveEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'staged', added: 2, removed: 1 } + ] + + expect(resolveCombinedUncommittedSnapshotEntries(snapshotEntries, liveEntries)).toEqual([ + { path: 'src/file.ts', status: 'modified', area: 'staged', added: 2, removed: 1 } + ]) + }) + + it('uses the live unstaged area when a snapshot staged file has been unstaged', () => { + const snapshotEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'staged' } + ] + const liveEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'unstaged' } + ] + + expect(resolveCombinedUncommittedSnapshotEntries(snapshotEntries, liveEntries)).toEqual([ + { path: 'src/file.ts', status: 'modified', area: 'unstaged' } + ]) + }) + + it('keeps the snapshot area when that area is still present for a path', () => { + const snapshotEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'unstaged' } + ] + const liveEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'staged' }, + { path: 'src/file.ts', status: 'modified', area: 'unstaged' } + ] + + expect(resolveCombinedUncommittedSnapshotEntries(snapshotEntries, liveEntries)).toEqual( + snapshotEntries + ) + }) + + it('keeps a retained resolved area when live status no longer includes the path', () => { + const snapshotEntries: GitStatusEntry[] = [ + { path: 'src/file.ts', status: 'modified', area: 'unstaged' } + ] + const retained = new Map([ + ['src/file.ts', { path: 'src/file.ts', status: 'modified', area: 'staged' }] + ]) + + expect(resolveCombinedUncommittedSnapshotEntries(snapshotEntries, [], retained)).toEqual([ + { path: 'src/file.ts', status: 'modified', area: 'staged' } + ]) + }) +}) + describe('getCombinedBranchEntries', () => { it('uses an explicitly empty snapshot instead of falling back to live entries', () => { const liveEntries: GitBranchChangeEntry[] = [{ path: 'src/live.ts', status: 'modified' }] diff --git a/src/renderer/src/components/editor/combined-diff-entries.ts b/src/renderer/src/components/editor/combined-diff-entries.ts index e27e5037644..790d6360276 100644 --- a/src/renderer/src/components/editor/combined-diff-entries.ts +++ b/src/renderer/src/components/editor/combined-diff-entries.ts @@ -19,6 +19,46 @@ export function getCombinedUncommittedEntries( }) } +export function resolveCombinedUncommittedSnapshotEntries( + snapshotEntries: readonly GitStatusEntry[], + liveEntries: readonly GitStatusEntry[], + retainedResolvedEntries?: ReadonlyMap +): GitStatusEntry[] { + const liveEntriesByPath = new Map() + for (const liveEntry of liveEntries) { + const entries = liveEntriesByPath.get(liveEntry.path) + if (entries) { + entries.push(liveEntry) + } else { + liveEntriesByPath.set(liveEntry.path, [liveEntry]) + } + } + + return snapshotEntries.map((snapshotEntry) => { + const livePathEntries = liveEntriesByPath.get(snapshotEntry.path) ?? [] + if (livePathEntries.some((liveEntry) => liveEntry.area === snapshotEntry.area)) { + return snapshotEntry + } + + const movedEntry = livePathEntries[0] ?? retainedResolvedEntries?.get(snapshotEntry.path) + if (!movedEntry || movedEntry.area === snapshotEntry.area) { + return snapshotEntry + } + + // Why: a snapshot-backed Changes tab can outlive stage/unstage actions. + // Load the area Git now reports so Monaco doesn't diff identical files. + return { + ...snapshotEntry, + area: movedEntry.area, + status: movedEntry.status, + oldPath: movedEntry.oldPath, + added: movedEntry.added, + removed: movedEntry.removed, + submodule: movedEntry.submodule + } + }) +} + export function getCombinedBranchEntries( snapshotEntries: readonly GitBranchChangeEntry[] | undefined, liveEntries: readonly GitBranchChangeEntry[] @@ -35,7 +75,7 @@ export function shouldAutoReloadCombinedDiffFromGitStatus({ mode: CombinedDiffFileTreeMode hasUncommittedEntriesSnapshot: boolean }): boolean { - // Why: snapshot-backed tabs intentionally preserve the tab-open diff while + // Why: snapshot-backed tabs preserve the tab-open file list while // staging/commit status churns; targeted editor-write reloads still refresh. return mode === 'uncommitted' && !hasUncommittedEntriesSnapshot } diff --git a/tests/e2e/large-diff-freeze-repro.spec.ts b/tests/e2e/large-diff-freeze-repro.spec.ts index 2696e45e767..3ad6bfd14ed 100644 --- a/tests/e2e/large-diff-freeze-repro.spec.ts +++ b/tests/e2e/large-diff-freeze-repro.spec.ts @@ -1,38 +1,13 @@ -import { execFileSync } from 'child_process' -import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'fs' -import os from 'os' -import path from 'path' -import { randomUUID } from 'crypto' +import { rmSync, writeFileSync } from 'fs' import type { Page } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { waitForSessionReady } from './helpers/store' import { getLargeDiffRenderLimit } from '../../src/shared/large-diff-render-limit' - -type IsolatedLargeDiffRepo = { - repoPath: string - relativePath: string - absolutePath: string -} - -function runGit(repoPath: string, args: string[]): void { - execFileSync('git', args, { cwd: repoPath, stdio: 'pipe' }) -} - -function createIsolatedLargeDiffRepo(): IsolatedLargeDiffRepo { - const repoPath = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-large-diff-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 relativePath = path.join('src', `large-diff-${randomUUID()}.ts`) - const absolutePath = path.join(repoPath, relativePath) - writeFileSync(absolutePath, 'export const seed = 1\n') - runGit(repoPath, ['add', '-A']) - runGit(repoPath, ['commit', '-m', 'Initial large diff repro fixture']) - - return { repoPath, relativePath, absolutePath } -} +import { + buildLargeTypeScriptFile, + createIsolatedLargeDiffRepo, + createIsolatedStagedLocaleDiffRepo +} from './large-diff-repro-fixtures' async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise { const repoId = await orcaPage.evaluate(async (pathToRepo: string) => { @@ -92,14 +67,6 @@ async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise { test.describe.configure({ mode: 'serial' }) test.use({ seedTestRepo: false }) @@ -192,4 +159,85 @@ test.describe('Large diff freeze repro', () => { rmSync(fixture.repoPath, { recursive: true, force: true }) } }) + + test('opening stale unstaged combined diffs after staging keeps the renderer responsive', async ({ + orcaPage + }) => { + await waitForSessionReady(orcaPage) + const fixture = createIsolatedStagedLocaleDiffRepo() + + try { + const worktreeId = await addAndActivateRepo(orcaPage, fixture.repoPath) + const measurement = await orcaPage.evaluate( + async ({ wId, repoPath, expectedPaths }) => { + 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 entries = status.entries.filter((entry) => entry.area === 'staged') + const entryPaths = entries.map((entry) => entry.path) + const missing = expectedPaths.filter((path) => !entryPaths.includes(path)) + if (missing.length > 0) { + throw new Error(`staged locale fixture missing entries: ${missing.join(', ')}`) + } + + const staleUnstagedEntries = entries.map((entry) => ({ ...entry, area: 'unstaged' })) + 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() + store.getState().openAllDiffs(wId, repoPath, undefined, 'unstaged', staleUnstagedEntries) + + let editorCount = 0 + let fallbackCount = 0 + while (performance.now() - startedAt < 30_000) { + await new Promise((resolve) => window.setTimeout(resolve, 50)) + editorCount = document.querySelectorAll('.monaco-diff-editor').length + fallbackCount = document.querySelectorAll('[data-testid="large-diff-fallback"]').length + if (editorCount + fallbackCount >= Math.min(entries.length, 5)) { + await new Promise((resolve) => window.setTimeout(resolve, 1_000)) + break + } + } + + window.clearInterval(timer) + const classHits = Array.from( + document.querySelectorAll( + '.monaco-diff-editor .line-insert, .monaco-diff-editor .line-delete, .monaco-diff-editor .char-insert, .monaco-diff-editor .char-delete' + ) + ).length + return { + editorCount, + fallbackCount, + classHits, + maxLagMs, + sampleCount: samples.length, + p95LagMs: samples.length + ? [...samples].sort((a, b) => a - b)[Math.floor(samples.length * 0.95)] + : 0 + } + }, + { wId: worktreeId, repoPath: fixture.repoPath, expectedPaths: fixture.relativePaths } + ) + + console.log(`stale unstaged combined diff measurement ${JSON.stringify(measurement)}`) + expect(measurement.editorCount + measurement.fallbackCount).toBeGreaterThanOrEqual(5) + expect(measurement.classHits).toBeGreaterThan(0) + expect(measurement.maxLagMs).toBeLessThan(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 new file mode 100644 index 00000000000..cbc93c6a585 --- /dev/null +++ b/tests/e2e/large-diff-repro-fixtures.ts @@ -0,0 +1,91 @@ +import { execFileSync } from 'child_process' +import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from 'fs' +import os from 'os' +import path from 'path' +import { randomUUID } from 'crypto' + +export type IsolatedLargeDiffRepo = { + repoPath: string + relativePath: string + absolutePath: string +} + +export type IsolatedStagedLocaleDiffRepo = { + repoPath: string + relativePaths: string[] +} + +function runGit(repoPath: string, args: string[]): void { + execFileSync('git', args, { cwd: repoPath, stdio: 'pipe' }) +} + +export function createIsolatedLargeDiffRepo(): IsolatedLargeDiffRepo { + const repoPath = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-large-diff-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 relativePath = path.join('src', `large-diff-${randomUUID()}.ts`) + const absolutePath = path.join(repoPath, relativePath) + writeFileSync(absolutePath, 'export const seed = 1\n') + runGit(repoPath, ['add', '-A']) + runGit(repoPath, ['commit', '-m', 'Initial large diff repro fixture']) + + return { repoPath, relativePath, absolutePath } +} + +export function buildLargeTypeScriptFile(lineCount: number): string { + const lines: string[] = [] + for (let i = 0; i < lineCount; i += 1) { + lines.push(`export const largeDiffValue${i} = ${i}`) + } + return `${lines.join('\n')}\n` +} + +function buildLocaleLikeJson(fileIndex: number, entryCount: number): string { + const lines = ['{'] + for (let i = 0; i < entryCount; i += 1) { + const value = `locale ${fileIndex} original ${i} `.repeat(5).trim() + const comma = i + 1 === entryCount ? '' : ',' + lines.push(` "entry_${String(i).padStart(5, '0')}": "${value}"${comma}`) + } + lines.push('}') + return `${lines.join('\n')}\n` +} + +function modifyLocaleLikeJson(content: string, fileIndex: number): string { + const lines = content.split('\n') + const changedLineIndex = 3200 + fileIndex + lines[changedLineIndex] = lines[changedLineIndex].replace('original', 'updated') + lines.splice(changedLineIndex + 4, 1) + return lines.join('\n') +} + +export function createIsolatedStagedLocaleDiffRepo(): IsolatedStagedLocaleDiffRepo { + const repoPath = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-staged-locale-repro-'))) + runGit(repoPath, ['init']) + runGit(repoPath, ['config', 'user.email', 'e2e@test.local']) + runGit(repoPath, ['config', 'user.name', 'E2E Test']) + + mkdirSync(path.join(repoPath, 'src', 'locales'), { recursive: true }) + const relativePaths: string[] = [] + for (let fileIndex = 0; fileIndex < 5; fileIndex += 1) { + const relativePath = `src/locales/locale-${fileIndex}.json` + const absolutePath = path.join(repoPath, ...relativePath.split('/')) + const original = buildLocaleLikeJson(fileIndex, 3600) + writeFileSync(absolutePath, original) + relativePaths.push(relativePath) + } + runGit(repoPath, ['add', '-A']) + runGit(repoPath, ['commit', '-m', 'Initial locale fixture']) + + for (let fileIndex = 0; fileIndex < relativePaths.length; fileIndex += 1) { + const absolutePath = path.join(repoPath, ...relativePaths[fileIndex].split('/')) + const original = buildLocaleLikeJson(fileIndex, 3600) + writeFileSync(absolutePath, modifyLocaleLikeJson(original, fileIndex)) + } + runGit(repoPath, ['add', '-A']) + + return { repoPath, relativePaths } +}