xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: address review findings (#2053)

This commit is contained in:
Jinjing 2026-05-15 21:18:00 -07:00 committed by GitHub
parent 17f4abfc5e
commit 2248c0168a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 173 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

112
src/renderer/src/components/editor/CombinedDiffViewer.tsx
View File

@ -4,6 +4,7 @@ restore-on-remount caching, and scroll preservation. Splitting those pieces
across smaller files would make the lifecycle edges harder to reason about and
more error-prone than keeping the whole viewer flow together. */
import React, { useState, useEffect, useCallback, useRef, useLayoutEffect } from 'react'
import { useVirtualizer } from '@tanstack/react-virtual'
import type { editor as monacoEditor } from 'monaco-editor'
import { useAppStore } from '@/store'
import { joinPath } from '@/lib/path'
@ -40,6 +41,7 @@ import { Check, Copy, MessageSquare, Send, Trash2 } from 'lucide-react'
import { toast } from 'sonner'
import { DiffSectionItem } from './DiffSectionItem'
import { getCombinedUncommittedEntries } from './combined-diff-entries'
import { getDiffSectionEstimatedHeight, isIntrinsicHeightImageDiff } from './diff-section-layout'
import type { DiffSection } from './diff-section-types'
type CachedCombinedDiffViewState = {
@ -53,6 +55,7 @@ type CachedCombinedDiffViewState = {
const combinedDiffViewStateCache = new Map<string, CachedCombinedDiffViewState>()
const combinedDiffScrollTopCache = new Map<string, number>()
const COMBINED_DIFF_OVERSCAN = 5
export default function CombinedDiffViewer({
file,
@ -314,6 +317,40 @@ export default function CombinedDiffViewer({
const modifiedEditorsRef = useRef<Map<number, monacoEditor.IStandaloneCodeEditor>>(new Map())
const virtualizer = useVirtualizer({
count: sections.length,
getScrollElement: () => scrollContainerRef.current,
estimateSize: (index) => {
const section = sections[index]
if (!section) {
return 88
}
return getDiffSectionEstimatedHeight({
collapsed: section.collapsed,
measuredContentHeight: sectionHeights[index],
originalContent: section.originalContent,
modifiedContent: section.modifiedContent,
useIntrinsicImageHeight: isIntrinsicHeightImageDiff(section.diffResult)
})
},
overscan: COMBINED_DIFF_OVERSCAN,
getItemKey: (index) => {
const section = sections[index]
if (!section) {
return `${index}:${generation}`
}
return `${section.key}:${section.collapsed ? 'collapsed' : 'expanded'}:${generation}`
}
})
useLayoutEffect(() => {
// Why: inline vs side-by-side can change Monaco content heights across
// every loaded row. Re-measure on this explicit mode change, not on every
// section load.
virtualizer.measure()
}, [sideBySide, virtualizer])
const toggleSection = useCallback((index: number) => {
setSections((prev) => prev.map((s, i) => (i === index ? { ...s, collapsed: !s.collapsed } : s)))
}, [])
@ -348,7 +385,21 @@ export default function CombinedDiffViewer({
content
)
setSections((prev) =>
prev.map((s, i) => (i === index ? { ...s, modifiedContent: content, dirty: false } : s))
prev.map((s, i) => {
if (i !== index) {
return s
}
return {
...s,
modifiedContent: content,
dirty: false,
diffResult:
s.diffResult?.kind === 'text'
? { ...s.diffResult, modifiedContent: content }
: s.diffResult
}
})
)
} catch (err) {
console.error('Save failed:', err)
@ -687,26 +738,45 @@ export default function CombinedDiffViewer({
<div ref={scrollContainerRef} className="flex-1 overflow-auto scrollbar-editor">
{skippedConflictNotice}
{sections.map((section, index) => (
<DiffSectionItem
key={`${section.key}:${generation}`}
section={section}
index={index}
isBranchMode={isBranchMode}
sideBySide={sideBySide}
isDark={isDark}
settings={settings}
sectionHeight={sectionHeights[index]}
worktreeId={file.worktreeId}
worktreeRoot={file.filePath}
loadSection={loadSection}
toggleSection={toggleSection}
setSectionHeights={setSectionHeights}
setSections={setSections}
modifiedEditorsRef={modifiedEditorsRef}
handleSectionSaveRef={handleSectionSaveRef}
/>
))}
<div className="relative w-full" style={{ height: `${virtualizer.getTotalSize()}px` }}>
{virtualizer.getVirtualItems().map((virtualItem) => {
const section = sections[virtualItem.index]
if (!section) {
return null
}
return (
<div
key={virtualItem.key}
data-index={virtualItem.index}
ref={virtualizer.measureElement}
className="absolute left-0 top-0 w-full"
// Why: `top` preserves sticky file headers inside each row;
// transform-based virtualization creates a containing block
// that makes long-section headers feel jumpy while scrolling.
style={{ top: `${virtualItem.start}px` }}
>
<DiffSectionItem
section={section}
index={virtualItem.index}
isBranchMode={isBranchMode}
sideBySide={sideBySide}
isDark={isDark}
settings={settings}
sectionHeight={sectionHeights[virtualItem.index]}
worktreeId={file.worktreeId}
worktreeRoot={file.filePath}
loadSection={loadSection}
toggleSection={toggleSection}
setSectionHeights={setSectionHeights}
setSections={setSections}
modifiedEditorsRef={modifiedEditorsRef}
handleSectionSaveRef={handleSectionSaveRef}
/>
</div>
)
})}
</div>
</div>
</div>
<Dialog

36
src/renderer/src/components/editor/DiffSectionItem.tsx
View File

@ -8,7 +8,6 @@ import {
useState,
type MutableRefObject
} from 'react'
import { LazySection } from './LazySection'
import { DiffEditor, type DiffOnMount } from '@monaco-editor/react'
import type { editor as monacoEditor } from 'monaco-editor'
import { monaco } from '@/lib/monaco-setup'
@ -284,6 +283,9 @@ export function DiffSectionItem({
lineNumberOptionsSubRef.current?.dispose()
lineNumberOptionsSubRef.current = null
diffEditorRef.current = null
if (modifiedEditorsRef.current.get(index) === modified) {
modifiedEditorsRef.current.delete(index)
}
setModifiedEditor(null)
setPopover(null)
})
@ -298,14 +300,36 @@ export function DiffSectionItem({
)
modified.onDidChangeModelContent(() => {
const current = modified.getValue()
setSections((prev) =>
prev.map((s, i) => (i === index ? { ...s, dirty: current !== s.modifiedContent } : s))
)
setSections((prev) => {
let changed = false
const next = prev.map((s, i) => {
if (i !== index) {
return s
}
const savedModifiedContent =
s.diffResult?.kind === 'text' ? s.diffResult.modifiedContent : s.modifiedContent
const dirty = current !== savedModifiedContent
if (s.modifiedContent === current && s.dirty === dirty) {
return s
}
changed = true
// Why: virtualized rows unmount when scrolled away, so the draft must
// live in section state instead of only in Monaco's mounted model.
return { ...s, modifiedContent: current, dirty }
})
return changed ? next : prev
})
})
}
useEffect(() => {
loadSection(index)
}, [index, loadSection])
return (
<LazySection key={section.key} index={index} onVisible={loadSection}>
<div className="border-b border-border">
<DiffSectionHeader
path={section.path}
dirty={section.dirty}
@ -399,6 +423,6 @@ export function DiffSectionItem({
)}
</div>
)}
</LazySection>
</div>
)
}

40
src/renderer/src/components/editor/LazySection.tsx
View File

@ -1,40 +0,0 @@
import React, { useEffect, useRef } from 'react'
export function LazySection({
index,
onVisible,
children
}: {
index: number
onVisible: (index: number) => void
children: React.ReactNode
}): React.JSX.Element {
const ref = useRef<HTMLDivElement>(null)
const triggered = useRef(false)
useEffect(() => {
const el = ref.current
if (!el || triggered.current) {
return
}
const observer = new IntersectionObserver(
(entries) => {
if (entries[0]?.isIntersecting && !triggered.current) {
triggered.current = true
onVisible(index)
observer.disconnect()
}
},
{ rootMargin: '200px' }
)
observer.observe(el)
return () => observer.disconnect()
}, [index, onVisible])
return (
<div ref={ref} className="border-b border-border">
{children}
</div>
)
}

30
src/renderer/src/components/editor/diff-section-layout.test.ts
View File

@ -1,5 +1,9 @@
import { describe, expect, it } from 'vitest'
import { getDiffSectionBodyHeight, isIntrinsicHeightImageDiff } from './diff-section-layout'
import {
getDiffSectionBodyHeight,
getDiffSectionEstimatedHeight,
isIntrinsicHeightImageDiff
} from './diff-section-layout'
import type { GitDiffResult } from '../../../../shared/types'
describe('diff section layout', () => {
@ -81,4 +85,28 @@ describe('diff section layout', () => {
expect(isIntrinsicHeightImageDiff(pngDiff)).toBe(true)
expect(isIntrinsicHeightImageDiff(pdfDiff)).toBe(false)
})
it('estimates virtualized expanded section height from diff line count', () => {
expect(
getDiffSectionEstimatedHeight({
collapsed: false,
measuredContentHeight: undefined,
originalContent: 'one',
modifiedContent: 'one\ntwo\nthree',
useIntrinsicImageHeight: false
})
).toBe(104)
})
it('estimates collapsed virtualized sections as header-only rows', () => {
expect(
getDiffSectionEstimatedHeight({
collapsed: true,
measuredContentHeight: 500,
originalContent: 'one',
modifiedContent: 'one\ntwo\nthree',
useIntrinsicImageHeight: false
})
).toBe(28)
})
})

23
src/renderer/src/components/editor/diff-section-layout.ts
View File

@ -3,6 +3,7 @@ import type { GitDiffResult } from '../../../../shared/types'
const DIFF_LINE_HEIGHT = 19
const DIFF_SECTION_PADDING_HEIGHT = 19
const MIN_DIFF_SECTION_BODY_HEIGHT = 60
const DIFF_SECTION_HEADER_HEIGHT = 28
type DiffSectionBodyHeightInput = {
measuredContentHeight: number | undefined
@ -36,3 +37,25 @@ export function getDiffSectionBodyHeight({
DIFF_SECTION_PADDING_HEIGHT
)
}
export function getDiffSectionEstimatedHeight({
collapsed,
measuredContentHeight,
originalContent,
modifiedContent,
useIntrinsicImageHeight
}: DiffSectionBodyHeightInput & { collapsed: boolean }): number {
if (collapsed) {
return DIFF_SECTION_HEADER_HEIGHT
}
return (
DIFF_SECTION_HEADER_HEIGHT +
(getDiffSectionBodyHeight({
measuredContentHeight,
originalContent,
modifiedContent,
useIntrinsicImageHeight
}) ?? MIN_DIFF_SECTION_BODY_HEIGHT)
)
}