mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 00:03:08 +00:00
fix(changes-tab): prevent freeze and stale highlights when staging/unstaging
When files are staged/unstaged while the Changes tab is open, the diff viewer was re-rendering all sections, causing UI freezes and highlight flickering. - Add resolveCombinedUncommittedSnapshotEntries() to reconcile snapshot entries with live git status without destroying existing loaded diff content. - Track retainedResolvedSnapshotEntries from current sections to preserve area state when live git status temporarily loses entries. - Use Map-based O(snapshot + live) algorithm instead of nested loops. - Add unit tests for the resolver logic. - Add e2e test with performance measurement for stale unstaged diffs. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -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<GitBranchChangeEntry[]>(() => {
|
||||
return getCombinedBranchEntries(file.branchEntriesSnapshot, liveBranchEntries)
|
||||
|
||||
@@ -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<string, GitStatusEntry>([
|
||||
['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' }]
|
||||
|
||||
@@ -19,6 +19,46 @@ export function getCombinedUncommittedEntries(
|
||||
})
|
||||
}
|
||||
|
||||
export function resolveCombinedUncommittedSnapshotEntries(
|
||||
snapshotEntries: readonly GitStatusEntry[],
|
||||
liveEntries: readonly GitStatusEntry[],
|
||||
retainedResolvedEntries?: ReadonlyMap<string, GitStatusEntry>
|
||||
): GitStatusEntry[] {
|
||||
const liveEntriesByPath = new Map<string, GitStatusEntry[]>()
|
||||
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
|
||||
}
|
||||
|
||||
@@ -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<string> {
|
||||
const repoId = await orcaPage.evaluate(async (pathToRepo: string) => {
|
||||
@@ -92,14 +67,6 @@ async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise<str
|
||||
return worktreeId
|
||||
}
|
||||
|
||||
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`
|
||||
}
|
||||
|
||||
test.describe('Large diff freeze repro', () => {
|
||||
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 })
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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 }
|
||||
}
|
||||
Reference in New Issue
Block a user