From 2248c0168a31fcc3ddcd6f0149b3c4db47aceeac Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 15 May 2026 21:18:00 -0700 Subject: [PATCH] fix: address review findings (#2053) --- .../components/editor/CombinedDiffViewer.tsx | 112 ++++++++++++++---- .../src/components/editor/DiffSectionItem.tsx | 36 +++++- .../src/components/editor/LazySection.tsx | 40 ------- .../editor/diff-section-layout.test.ts | 30 ++++- .../components/editor/diff-section-layout.ts | 23 ++++ 5 files changed, 173 insertions(+), 68 deletions(-) delete mode 100644 src/renderer/src/components/editor/LazySection.tsx diff --git a/src/renderer/src/components/editor/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/CombinedDiffViewer.tsx index 30b5fdb00..486cc0621 100644 --- a/src/renderer/src/components/editor/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/CombinedDiffViewer.tsx @@ -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() const combinedDiffScrollTopCache = new Map() +const COMBINED_DIFF_OVERSCAN = 5 export default function CombinedDiffViewer({ file, @@ -314,6 +317,40 @@ export default function CombinedDiffViewer({ const modifiedEditorsRef = useRef>(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({
{skippedConflictNotice} - {sections.map((section, index) => ( - - ))} +
+ {virtualizer.getVirtualItems().map((virtualItem) => { + const section = sections[virtualItem.index] + if (!section) { + return null + } + + return ( +
+ +
+ ) + })} +
{ 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 ( - +
)} - +
) } diff --git a/src/renderer/src/components/editor/LazySection.tsx b/src/renderer/src/components/editor/LazySection.tsx deleted file mode 100644 index 96d3f105a..000000000 --- a/src/renderer/src/components/editor/LazySection.tsx +++ /dev/null @@ -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(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 ( -
- {children} -
- ) -} diff --git a/src/renderer/src/components/editor/diff-section-layout.test.ts b/src/renderer/src/components/editor/diff-section-layout.test.ts index c86857a8b..a8b91bb5e 100644 --- a/src/renderer/src/components/editor/diff-section-layout.test.ts +++ b/src/renderer/src/components/editor/diff-section-layout.test.ts @@ -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) + }) }) diff --git a/src/renderer/src/components/editor/diff-section-layout.ts b/src/renderer/src/components/editor/diff-section-layout.ts index 4191872ae..a51aca7b2 100644 --- a/src/renderer/src/components/editor/diff-section-layout.ts +++ b/src/renderer/src/components/editor/diff-section-layout.ts @@ -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) + ) +}