Fix combined diff freeze after large diff invalidation (#12615)
* test(diff): repro for STA-3420 combined-diff invalidation freeze Co-authored-by: Orca <help@stably.ai> * Fix diff-view freeze when large diff invalidated by rebase writes Staged-diff sections now reload in-place on external file changes instead of remounting every visible Monaco editor and bumping the virtualizer generation, which wedged the renderer during rebase bursts. * 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 <help@stably.ai> * fix(diff): keep renderedIndicesRef pure during render React Doctor blocks ref mutation during render; sync the on-screen section set in a layout effect instead so static analysis can pass. * Fix unchanged diff-section reload detection for truncated diffs When a diff exceeds render limits, content is pruned to '' for memory. The old check compared content equality, so limited reloads always appeared changed, triggering unnecessary revalidation that froze the UI. Compare render-limit metadata instead — it's the sole change signal and full description of what the fallback banner displays. Also calibrate STA-3420 e2e assertions relative to idle baseline for machine independence instead of absolute thresholds. * fix(diff): defer invalidation reloads for in-flight stale-token loads When a diff section is invalidated while a large-diff load is in-flight: - Don't delete the in-flight load from loadingIndicesRef, since a newer load may own it - Bump the reload token but defer the reload if there's still an in-flight load - Let the in-flight load settle first, then reschedule the reload at settle-time - Prevents the freeze by avoiding race conditions that leave sections stuck loading This fixes STA-3420 where rebase-driven invalidations could hang the diff view. * test(diff): relax STA-3420 burst assertions to inclusive comparisons Switch from strict inequality checks (toBeLessThan, toBeGreaterThan) to inclusive variants (toBeLessThanOrEqual, toBeGreaterThanOrEqual) to allow measurements landing exactly on the threshold boundaries. --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinjing
GitHub
parent
c736031773
commit
bac99c920b
|
|
@ -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<number, number>): 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<Set<number>>(new Set())
|
||||
const sectionsRef = useRef<DiffSection[]>([])
|
||||
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<Map<number, number>>(new Map())
|
||||
const renderedIndicesRef = useRef<Set<number>>(new Set())
|
||||
const reloadTimersRef = useRef<Map<number, number>>(new Map())
|
||||
const loadSectionRef = useRef<(index: number) => Promise<void>>(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}
|
||||
<div className="relative w-full" style={{ height: `${combinedDiffTotalSize}px` }}>
|
||||
{virtualizer.getVirtualItems().map((virtualItem) => {
|
||||
{combinedDiffVirtualItems.map((virtualItem) => {
|
||||
const section = sections[virtualItem.index]
|
||||
if (!section) {
|
||||
return null
|
||||
|
|
|
|||
|
|
@ -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<Extract<LargeDiffRenderLimit, { limited: true }>> = {}
|
||||
): 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<typeof isUnchangedDiffSectionReload>[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)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<LargeDiffRenderLimit, { limited: true }>
|
||||
|
||||
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
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string> {
|
||||
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 })
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
@ -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'])
|
||||
|
|
|
|||
Loading…
Reference in New Issue