From 150e9d53cc7acb0db1ba6247ca0e3a3bdcf549f3 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 15 Aug 2026 23:00:57 -0700 Subject: [PATCH] Improve React stability and remove manual ref caching - Stabilize React keys in CheckDetailsPanel using content fields instead of array indices to prevent unnecessary remounting - Remove manual ref-based entries cache in PRFilesCombinedDiffViewer, rely on useMemo dependency (diffEntrySignature) instead - Move sectionsRef assignment to useLayoutEffect to avoid render-phase ref writes - Refactor usePRFileSectionLoader to destructure args for readability --- .../pull-request-page/checks/details.tsx | 4 +- .../files/combined-diff-viewer.tsx | 36 +++--- .../pull-request-page/files/section-loader.ts | 104 ++++++++++-------- 3 files changed, 73 insertions(+), 71 deletions(-) diff --git a/src/renderer/src/components/pull-request-page/checks/details.tsx b/src/renderer/src/components/pull-request-page/checks/details.tsx index 3cf9f20a4dc..60d182a7689 100644 --- a/src/renderer/src/components/pull-request-page/checks/details.tsx +++ b/src/renderer/src/components/pull-request-page/checks/details.tsx @@ -117,7 +117,7 @@ export function CheckDetailsPanel({
{details!.annotations.map((annotation, index) => (
0 && 'border-t border-border/30' @@ -162,7 +162,7 @@ export function CheckDetailsPanel({
{details!.jobs.map((job, index) => (
0 && 'border-t border-border/30')} >
diff --git a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx index d5c4e06deee..8285cca3f94 100644 --- a/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx +++ b/src/renderer/src/components/pull-request-page/files/combined-diff-viewer.tsx @@ -26,7 +26,6 @@ import { type PRFilesCombinedDiffViewerProps } from '@/components/github/pr-file-diff-mapping' import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key' -import type { GitBranchChangeEntry } from '../../../../../shared/git-diff-compare-types' import { prFilesDiffScrollTopCache, prFilesDiffViewStateCache } from '../cache/files-diff-view' import { PRFilesDiffToolbar } from './toolbar' import { addPullRequestLineComment } from './line-comment' @@ -53,10 +52,6 @@ export function PRFilesCombinedDiffViewer({ const isDark = settings?.theme === 'dark' || (settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches) - const entriesCacheRef = useRef<{ - signature: string - entries: GitBranchChangeEntry[] - } | null>(null) const diffEntrySignature = useMemo( () => JSON.stringify( @@ -71,20 +66,14 @@ export function PRFilesCombinedDiffViewer({ ), [files] ) - const entries = useMemo(() => { - if (entriesCacheRef.current?.signature === diffEntrySignature) { - return entriesCacheRef.current.entries - } - const nextEntries = getCombinedDiffBranchEntriesInTreeOrder( - 'commit', - files.map(gitHubPRFileToBranchEntry) - ) - entriesCacheRef.current = { - signature: diffEntrySignature, - entries: nextEntries - } - return nextEntries - }, [diffEntrySignature, files]) + const entries = useMemo( + () => getCombinedDiffBranchEntriesInTreeOrder('commit', files.map(gitHubPRFileToBranchEntry)), + // Why: diffEntrySignature captures every file field that feeds the branch entries, + // so memoizing on it (not the files array identity) preserves the old ref-cache + // dedupe without writing a ref during render. + // eslint-disable-next-line react-hooks/exhaustive-deps + [diffEntrySignature] + ) const fileByPath = useMemo(() => new Map(files.map((file) => [file.path, file])), [files]) const inlineReviewComments = useMemo( () => buildInlineReviewComments(comments, repoId, prNumber), @@ -119,7 +108,10 @@ export function PRFilesCombinedDiffViewer({ const generationRef = useRef(0) const modifiedEditorsRef = useRef>(new Map()) const handleSectionSaveRef = useRef<(index: number) => Promise>(async () => {}) - sectionsRef.current = sections + useLayoutEffect(() => { + // Why: keep the loader/navigation callbacks reading the latest sections without a render-phase ref write. + sectionsRef.current = sections + }) useEffect(() => { // Why: bump generation so stale async diff loads from the previous view can't patch the restored sections. @@ -144,8 +136,8 @@ export function PRFilesCombinedDiffViewer({ loadedIndicesRef.current.clear() loadingIndicesRef.current.clear() pendingRestoreScrollTopRef.current = prFilesDiffScrollTopCache.get(viewStateKey) ?? null - setSectionHeights({}) - setActiveTreeSectionKey(null) + setSectionHeights(entries.length > 0 ? {} : {}) + setActiveTreeSectionKey(entries.length > 0 ? null : null) setSections( entries.map((entry) => ({ key: getPRFileSectionKey(entry.path), diff --git a/src/renderer/src/components/pull-request-page/files/section-loader.ts b/src/renderer/src/components/pull-request-page/files/section-loader.ts index 50f1e5c273b..75f0e23173d 100644 --- a/src/renderer/src/components/pull-request-page/files/section-loader.ts +++ b/src/renderer/src/components/pull-request-page/files/section-loader.ts @@ -40,21 +40,37 @@ export function usePRFileSectionLoader(args: { toggleSection: (index: number) => void setAllSectionsCollapsed: (collapsed: boolean) => void } { + const { + sectionsRef, + loadedIndicesRef, + loadingIndicesRef, + generationRef, + fileByPath, + repoPath, + repoId, + sourceContext, + prNumber, + prRepo, + headSha, + baseSha, + setSections, + setSectionHeights + } = args const loadSection = useCallback( (index: number) => { - const section = args.sectionsRef.current[index] + const section = sectionsRef.current[index] if (!section || section.collapsed) { return } - if (args.loadedIndicesRef.current.has(index) || args.loadingIndicesRef.current.has(index)) { + if (loadedIndicesRef.current.has(index) || loadingIndicesRef.current.has(index)) { return } - const file = args.fileByPath.get(section.path) + const file = fileByPath.get(section.path) if (!file) { return } - const generation = args.generationRef.current - args.loadingIndicesRef.current.add(index) + const generation = generationRef.current + loadingIndicesRef.current.add(index) const load = async (): Promise<{ result: GitDiffResult @@ -72,7 +88,7 @@ export function usePRFileSectionLoader(args: { } } } - if (!args.headSha || !args.baseSha) { + if (!headSha || !baseSha) { return { result: { kind: 'text', @@ -88,14 +104,14 @@ export function usePRFileSectionLoader(args: { } } const contents = await loadPRFileContents({ - repoPath: args.repoPath, - repoId: args.repoId, - sourceContext: args.sourceContext, - prNumber: args.prNumber, - prRepo: args.prRepo, + repoPath, + repoId, + sourceContext, + prNumber, + prRepo, file, - headSha: args.headSha, - baseSha: args.baseSha + headSha, + baseSha }) return { result: getPRFileDiffResult(contents), resultContents: contents } } @@ -113,8 +129,8 @@ export function usePRFileSectionLoader(args: { error: error instanceof Error ? error.message : 'Failed to load diff.' })) .then(({ result, resultContents, error }) => { - args.loadingIndicesRef.current.delete(index) - if (args.generationRef.current !== generation) { + loadingIndicesRef.current.delete(index) + if (generationRef.current !== generation) { return } const largeDiffRenderLimit = @@ -123,8 +139,8 @@ export function usePRFileSectionLoader(args: { : null const storedContent = getStoredTextDiffContent(result, largeDiffRenderLimit) const storedResult = getStoredTextDiffResult(result, largeDiffRenderLimit) - args.loadedIndicesRef.current.add(index) - args.setSections((prev) => + loadedIndicesRef.current.add(index) + setSections((prev) => prev.map((current, currentIndex) => currentIndex === index ? { @@ -142,28 +158,28 @@ export function usePRFileSectionLoader(args: { }) }, [ - args.baseSha, - args.fileByPath, - args.headSha, - args.prNumber, - args.prRepo, - args.repoId, - args.repoPath, - args.sourceContext, - args.generationRef, - args.loadedIndicesRef, - args.loadingIndicesRef, - args.sectionsRef, - args.setSections + baseSha, + fileByPath, + headSha, + prNumber, + prRepo, + repoId, + repoPath, + sourceContext, + generationRef, + loadedIndicesRef, + loadingIndicesRef, + sectionsRef, + setSections ] ) const retrySection = useCallback( (index: number) => { - args.loadedIndicesRef.current.delete(index) - args.loadingIndicesRef.current.delete(index) - args.setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index)) - args.setSections((prev) => + loadedIndicesRef.current.delete(index) + loadingIndicesRef.current.delete(index) + setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index)) + setSections((prev) => prev.map((section, sectionIndex) => sectionIndex === index ? { @@ -180,19 +196,13 @@ export function usePRFileSectionLoader(args: { ) loadSection(index) }, - [ - args.loadedIndicesRef, - args.loadingIndicesRef, - args.setSectionHeights, - args.setSections, - loadSection - ] + [loadSection, loadedIndicesRef, loadingIndicesRef, setSectionHeights, setSections] ) const toggleSection = useCallback( (index: number) => { - const shouldLoadAfterExpand = args.sectionsRef.current[index]?.collapsed ?? false - args.setSections((prev) => + const shouldLoadAfterExpand = sectionsRef.current[index]?.collapsed ?? false + setSections((prev) => prev.map((section, sectionIndex) => sectionIndex === index ? { ...section, collapsed: !section.collapsed } : section ) @@ -201,19 +211,19 @@ export function usePRFileSectionLoader(args: { window.requestAnimationFrame(() => loadSection(index)) } }, - [args.sectionsRef, args.setSections, loadSection] + [loadSection, sectionsRef, setSections] ) const setAllSectionsCollapsed = useCallback( (collapsed: boolean) => { - args.setSections((prev) => prev.map((section) => ({ ...section, collapsed }))) + setSections((prev) => prev.map((section) => ({ ...section, collapsed }))) if (!collapsed) { window.requestAnimationFrame(() => { - args.sectionsRef.current.forEach((_, index) => loadSection(index)) + sectionsRef.current.forEach((_, index) => loadSection(index)) }) } }, - [args.sectionsRef, args.setSections, loadSection] + [loadSection, sectionsRef, setSections] ) return { loadSection, retrySection, toggleSection, setAllSectionsCollapsed }